Repository navigation
fix: uninstall no longer disowns a context its own supervisor set - #471
Merged
Merged
Conversation
#468 is fixed -- EngineSnapshotSaveAndList passed in 72s, down from 154s -- and the suite moved on to NothingLeftBehind, a stage that had also never run in CI: the machine's `skrog` docker context changed: before="" after="npipe:////./pipe/skrog-e2e-suite" the suite must hand back a context it took from another install Third bug in the chain, same shape as the first: a custom --pipe not carried through Skrog's own records. `skrog install` records DockerContextHost -- normally docker_engine. A supervisor started with `--pipe custom` then re-points the SAME shared context at its own endpoint, and nothing writes that back to the manifest. At uninstall, contextIsOurs compared the live endpoint against the install-time value, concluded another install owned it, and left a `skrog` context pointing at a pipe it was about to delete. Every later `docker --context skrog` then fails, on a machine that just uninstalled Skrog. #217 taught this function not to reach too far -- removing a context that belongs to a second install breaks the first. That protection is right and stays. It was simply also not reaching far enough: the context our own supervisor set is ours. served-pipe answers it. It is the durable record of what this install last asked to serve (#429), written at supervisor start and deliberately not cleared on exit -- which is exactly what makes it readable here, after the supervisor is gone. The record added for restart turns out to be the missing fact for uninstall too. Negative-controlled by disabling the new branch: only the new test fails, and TestContextIsNotOursWhenAnotherInstallTookIt still passes, so #217 is not being undone. A second new test pins that directly -- an endpoint neither the install nor its supervisor set is still not ours. One self-inflicted detour worth recording: the first version of these tests was written through a heredoc, which ate a backslash and turned `\.\pipe\...` into `\.\pipe\...`. DockerHostFor then fell through to its generic branch and the test failed against correct code. Same trap as the binfmt registration lines in #462, and the reason those are pinned to LF and piped as bytes.
zcsizmadia
added a commit
that referenced
this pull request
Sep 22, 2026
`skrog uninstall` removed the distro, the data dir, the manifest, the autostart entry and the docker context, and left the always-on process that serves the pipe running. Nothing was stopping it, and nothing could have: on a real install the supervisor is detached. `skrog start` spawns it and calls Process.Release, autostart launches it at logon, skrogw.exe relaunches it after a crash — uninstall is not its parent and never was. The only uninstall that ran beside a live supervisor and still looked clean was the e2e suite's, which kills its own child by handle first. What survived served a pipe into a distro that had just been unregistered, rewrote endpoint records into the state dir being emptied, could re-point the shared `skrog` docker context the uninstall had just decided whether it was allowed to remove (#217, #471), and held skrog.exe open — so on Windows the directory Skrog was installed into could not be deleted, which is what a package-manager uninstall does next. "Removed. Nothing else on the system was modified." was printed over all of it. The mechanism already existed: `restart --supervisor` asks through the request file, waits for the single-instance claim to drop, and relies on the supervisor exiting 0 so skrogw treats it as asked-for rather than a crash. Uninstall needs exactly that minus the replacement, so the wait is now stopSupervisor, shared by both. It runs before the docker-context step — a supervisor stopped after it could hand the context straight back — and a supervisor that will not exit is a warning, not an aborted uninstall.
zcsizmadia
added a commit
that referenced
this pull request
Sep 22, 2026
#475) * e2e: assert no supervisor outlives the install (#474) The suite's only remaining failure is its own temp-dir cleanup: TempDir RemoveAll cleanup: unlinkat ...\001\skrog.exe: Access is denied. A process is still running from the binary it built. It is the supervisor `restart --supervisor` spawned: detached and released by `skrog start`, so the suite has no handle to it, and nothing in `skrog uninstall` stops it. NothingLeftBehind said nothing about it, because until that stage existed the only supervisor the suite ever saw was its own child, killed by handle before the uninstall ran. Every real install has the orphan shape from the first launch, since autostart and skrogw.exe spawn it the same way. This commit is the assertion alone, so it can be watched failing before the fix lands. * fix: uninstall stops the supervisor it leaves running (#474) `skrog uninstall` removed the distro, the data dir, the manifest, the autostart entry and the docker context, and left the always-on process that serves the pipe running. Nothing was stopping it, and nothing could have: on a real install the supervisor is detached. `skrog start` spawns it and calls Process.Release, autostart launches it at logon, skrogw.exe relaunches it after a crash — uninstall is not its parent and never was. The only uninstall that ran beside a live supervisor and still looked clean was the e2e suite's, which kills its own child by handle first. What survived served a pipe into a distro that had just been unregistered, rewrote endpoint records into the state dir being emptied, could re-point the shared `skrog` docker context the uninstall had just decided whether it was allowed to remove (#217, #471), and held skrog.exe open — so on Windows the directory Skrog was installed into could not be deleted, which is what a package-manager uninstall does next. "Removed. Nothing else on the system was modified." was printed over all of it. The mechanism already existed: `restart --supervisor` asks through the request file, waits for the single-instance claim to drop, and relies on the supervisor exiting 0 so skrogw treats it as asked-for rather than a crash. Uninstall needs exactly that minus the replacement, so the wait is now stopSupervisor, shared by both. It runs before the docker-context step — a supervisor stopped after it could hand the context straight back — and a supervisor that will not exit is a warning, not an aborted uninstall. * ci: the nightly e2e schedule comes back on (#448) The suite is green end to end on a hosted runner: run 35670620632, --- PASS: TestAcceptance (334.46s), every stage passing or skipping, and the skips are the optional tooling windows-latest does not carry. #448's condition was "re-enable when the suite can be green", not "when #429 is fixed" — the point of switching it off was that a nightly nobody can read is worse than none. The condition is met rather than waived. Back on 04:20 UTC, the cron #448 asked for.
zcsizmadia
added a commit
that referenced
this pull request
Sep 22, 2026
RELEASING.md asks for the entry in the same PR as the code, and six user-facing changes went in without one. Five of them are the chain the acceptance suite turned up once it actually ran, each hidden behind the one before it, and #429 is listed in 0.6.0's Known issues as still broken: #429 restart --supervisor lost the pipe it was serving #468 a dead dockerd read as "cannot tell", so the supervisor stopped repairing it -- plus the restore diagnostics that found it #471 uninstall disowned a context its own supervisor had set #472 uninstall left the supervisor running, holding skrog.exe open #444 autostart failed on a profile with no Run key The emulation entry also gains the fact a reader needs and it did not carry: the emulator is IN the rootfs, so the feature wants engine 29.8.1-3 or newer, and an older image says `missing interpreter` and runs without emulation rather than registering a handler pointing at nothing. Separately, site/home.md still told visitors code signing "is applied for through the SignPath Foundation, with signing by SignPath.io". The application was declined (#358). README.md, docs/install.md and docs/code-signing.md all say so; the site landing page was the one file left claiming otherwise, and it is the page the docs site opens on. The heading stays [Unreleased]. Dating it is the last commit before the tag, as #427 did for 0.6.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#468 is fixed
EngineSnapshotSaveAndListpassed in 72s, down from 154s. The|| trueon the engine ping was the cause, exactly as the diagnostics said.The suite then moved on to
NothingLeftBehind— a stage that had also never run in CI:Third bug in the chain, each one hidden behind the last. Same shape as the first: a custom
--pipenot carried through Skrog's own records.The bug
skrog installrecordsDockerContextHost— normallydocker_engine. A supervisor started with--pipe customthen re-points the same shared context at its own endpoint, and nothing writes that back to the manifest.At uninstall,
contextIsOurscompared the live endpoint against the install-time value, concluded another install owned it, and left askrogcontext pointing at a pipe it was about to delete. Every laterdocker --context skrogthen fails — on a machine that has just uninstalled Skrog.Why the existing protection stays
#217 taught this function not to reach too far: removing a context belonging to a second install breaks the first, and the symptom (
context "skrog": context not found) looks like Skrog is broken rather than like cleanup overreaching. That reasoning is right and is untouched.It was simply also not reaching far enough. The context our own supervisor set is ours.
served-pipeanswers it — the durable record of what this install last asked to serve (#429), written at supervisor start and deliberately not cleared on exit, which is exactly what makes it readable here after the supervisor is gone. The record added for restart turns out to be the missing fact for uninstall too.Negative-controlled
Disabling the new branch:
Only the new test fails, and a second new test pins the other direction directly: an endpoint neither the install nor its supervisor set is still not ours.
One self-inflicted detour, recorded
The first version of these tests went in through a heredoc, which ate a backslash and turned
\.\pipe\...into\.\pipe\....DockerHostForfell through to its generic branch and the test failed against correct code — I nearly went looking for a bug in the product.Same trap as the binfmt registration lines in #462, and the reason those are pinned to LF and piped as bytes rather than interpolated.
Next
Acceptance suite on main again. If
NothingLeftBehindpasses, that is the whole suite green for the first time and #448 finally unblocks.