fix: a replacement supervisor keeps the pipe it was serving - #446
Merged
Merged
Conversation
Closes #429. This is the last thing making the nightly acceptance run red, so a red nightly means something new again. `skrog restart --supervisor` rebuilds the supervisor's arguments rather than forwarding them, and the served pipe was not among what it carried. A supervisor started with `--pipe <custom>` came back on the default, DOCKER_HOST stopped working, and the error named a missing FILE rather than a moved pipe. The watchdog path had the same gap: skrogw relaunches through spawnSupervisor too, so a crash lost the pipe the same way. spawnSupervisor is the single choke point for both, so the fix lives there and needs no new CLI surface. Only a CUSTOM pipe is preserved, and the two exclusions are the fix ------------------------------------------------------------------ - the DEFAULT is not pinned. Normal selection takes it again when free and falls back correctly when Docker Desktop has taken it; pinning would turn that graceful fallback into a hard failure to bind. - the FALLBACK is not pinned either, because selection re-derives it -- it is only ever chosen when the default is taken. Pinning would make the fallback STICKY, so a machine that stopped running Desktop would never take the default pipe back, and "plain docker just works" is the thing the default pipe buys. What is left is a pipe someone asked for by name, which is exactly what was being lost. Ordinary installs see no change at all. Two mistakes worth recording ----------------------------- The first comparison trimmed `\\.\pipe\` off the recorded value and compared it against DefaultPipeName -- which ALREADY CONTAINS that prefix. Nothing ever matched, so both exclusions were dead and the fallback would have become sticky. The table test caught it; pipeEq now normalises both sides. The second is the one this release keeps finding. The --pipe append started out inline in spawnSupervisor, and TestCustomPipeToPreserve passed with that append deleted: a correct helper that nothing called. So the argument list itself is now built by supervisorCommand and asserted directly. Verified as a negative control -- with the wiring removed: args [supervise --state-dir ...] carry no --pipe; the replacement supervisor would re-select and DOCKER_HOST would stop working The helper-only test still passes in that state, which is precisely why it is not the one that matters.
This was referenced Sep 20, 2026
zcsizmadia
added a commit
that referenced
this pull request
Sep 20, 2026
) Reopens and actually fixes #429. #446 did not, and the nightly said so. #446 had spawnSupervisor read the recorded endpoint to decide whether to forward --pipe. But runSupervise clears that record on a clean exit: defer func() { supervise.ClearEndpoint(opts.StateDir) }() and `restart --supervisor` exits the old supervisor CLEANLY before spawning the replacement. So the sequence was: 1. old supervisor exits -> endpoint record deleted 2. spawnSupervisor -> ReadEndpoint -> nothing 3. replacement re-selects from scratch The fix read the record after the thing that writes it had removed it. It worked for a hard kill, where the record survives -- which is why the test passed. That test wrote an endpoint and called supervisorCommand directly, modelling the crash path rather than the restart sequence: the right assertion about the wrong scenario. runRestart now captures the pipe BEFORE recycling and carries it through runStartPreserving to spawnSupervisor. The recorded read stays as the fallback, because for a supervisor that died hard it is the only thing that remembers. runStartPreserving rather than a `--pipe` flag on `start`: this is not something a user asks for, it is one command carrying a value across a teardown that would otherwise erase it, and a CLI flag would document a decision the caller never makes. Verification ------------ TestRestartSequencePreservesTheCustomPipe walks the real ordering -- record written, captured, CLEARED, then the replacement built -- and fails against the exact code #446 shipped: args [supervise --state-dir ...] carry pipe "", want "\\.\pipe\skrog-e2e-suite" TestCrashRestartUsesTheRecordedPipe covers the other direction, so the fallback cannot rot. Not claiming this closes #429 until the acceptance suite says so. That claim is what was wrong last time.
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.
Closes #429. This is the last thing making the nightly acceptance run red, so a red nightly means something new again.
skrog restart --supervisorrebuilds the supervisor's arguments rather than forwarding them, and the served pipe was not among what it carried. A supervisor started with--pipe <custom>came back on the default,DOCKER_HOSTstopped working, and the error named a missing file rather than a moved pipe. The watchdog path had the same gap —skrogwrelaunches throughspawnSupervisortoo, so a crash lost the pipe the same way.spawnSupervisoris the single choke point for both, so the fix lives there and needs no new CLI surface.Only a custom pipe is preserved, and the exclusions are the fix
dockerjust works" is what the default pipe buys.What is left is a pipe someone asked for by name — exactly what was being lost. Ordinary installs see no change at all.
Two mistakes worth recording
The comparison was dead. The first version trimmed
\\.\pipe\off the recorded value and compared it againstDefaultPipeName— which already contains that prefix. Nothing ever matched, so both exclusions were inert and the fallback would have become sticky. The table test caught it;pipeEqnow normalises both sides.The helper was tested, not the wiring. The
--pipeappend started out inline inspawnSupervisor, andTestCustomPipeToPreservepassed with that append deleted — a correct helper that nothing called, which is the shape of three other defects in this release. The argument list is now built bysupervisorCommandand asserted directly.Negative control, wiring removed:
The helper-only test still passes in that state, which is precisely why it is not the one that matters.