Skip to content

fix: capture the pipe before the supervisor deletes its own record - #447

Merged
zcsizmadia merged 1 commit into
mainfrom
fix/preserve-pipe-across-clean-exit
Sep 20, 2026
Merged

zcsizmadia merged 1 commit into
mainfrom
fix/preserve-pipe-across-clean-exit

Conversation

@zcsizmadia

Copy link
Copy Markdown
Collaborator

#446 did not fix #429, and the nightly said so. Same failure, byte for byte.

#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:

  1. old supervisor exits → endpoint record deleted
  2. spawnSupervisorReadEndpoint → nothing
  3. replacement re-selects from scratch

I 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: it modelled the crash path, not the restart sequence. The right assertion about the wrong scenario.

The fix

runRestart 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 which pipe was served.

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. 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 exactly the code #446 shipped:

args [supervise --state-dir ...] carry pipe "", want "\\.\pipe\skrog-e2e-suite"

TestCrashRestartUsesTheRecordedPipe covers the other direction so the fallback cannot rot.

I am not claiming this closes #429 until the acceptance suite says so. Making that claim without running it is what was wrong last time.

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.
@zcsizmadia
zcsizmadia merged commit a58b19e into main Sep 20, 2026
5 checks passed
@zcsizmadia
zcsizmadia deleted the fix/preserve-pipe-across-clean-exit branch September 20, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

restart --supervisor loses the served pipe; the replacement re-selects from scratch

1 participant