From fecb1b665dd7f3f6b91fc8d6183a31232f4c17c8 Mon Sep 17 00:00:00 2001 From: Zoltan Csizmadia Date: Sun, 20 Sep 2026 09:09:06 -0500 Subject: [PATCH] fix: capture the pipe before the supervisor deletes its own record 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. --- cmd/skrog/pipepreserve_test.go | 92 +++++++++++++++++++++++++--------- cmd/skrog/supervise.go | 49 +++++++++++++++--- 2 files changed, 110 insertions(+), 31 deletions(-) diff --git a/cmd/skrog/pipepreserve_test.go b/cmd/skrog/pipepreserve_test.go index 0c5c61c..d43be8f 100644 --- a/cmd/skrog/pipepreserve_test.go +++ b/cmd/skrog/pipepreserve_test.go @@ -81,44 +81,86 @@ func TestCustomPipeToPreserve(t *testing.T) { } } -// The wiring, not just the helper. +// pipeArg returns the value after --pipe, or "" when there is none. +func pipeArg(args []string) string { + i := slices.Index(args, "--pipe") + if i < 0 || i+1 >= len(args) { + return "" + } + return args[i+1] +} + +// The RESTART sequence, which is the one that was broken (#429). // -// The first version of this fix had the --pipe append inline in -// spawnSupervisor, and TestCustomPipeToPreserve passed with that append -// deleted -- a correct helper that nothing called. Three other defects in this -// release had exactly that shape, so the argument list itself gets asserted. -func TestSupervisorCommandForwardsACustomPipe(t *testing.T) { +// The first attempt at this fix read the recorded endpoint inside +// spawnSupervisor. That passed a test which wrote an endpoint and called +// supervisorCommand directly — and still failed in the product, because +// runSupervise DELETES its endpoint record on a clean exit, and +// `restart --supervisor` exits it cleanly before spawning the replacement. +// The test asserted the right thing about the wrong scenario. +// +// So this models the ordering: record present, supervisor exits and clears it, +// THEN the replacement is built. +func TestRestartSequencePreservesTheCustomPipe(t *testing.T) { self := filepath.Join(t.TempDir(), "skrog.exe") + dir := t.TempDir() + const custom = `\\.\pipe\skrog-e2e-suite` - t.Run("a custom pipe is forwarded", func(t *testing.T) { - dir := t.TempDir() - const custom = `\\.\pipe\skrog-e2e-suite` - if err := supervise.WriteEndpoint(dir, supervise.Endpoint{Pipe: custom}); err != nil { - t.Fatal(err) - } - _, args := supervisorCommand(self, dir) - if !slices.Contains(args, "--pipe") { - t.Fatalf("args %v carry no --pipe; the replacement supervisor would re-select and "+ - "DOCKER_HOST would stop working (#429)", args) - } - i := slices.Index(args, "--pipe") - if i+1 >= len(args) || args[i+1] != custom { - t.Errorf("args %v: --pipe does not carry the recorded pipe %q", args, custom) - } - }) + // 1. A supervisor is serving a custom pipe. + if err := supervise.WriteEndpoint(dir, supervise.Endpoint{Pipe: custom}); err != nil { + t.Fatal(err) + } + // 2. restart --supervisor captures it before tearing anything down. + captured := customPipeToPreserve(dir) + + // 3. The old supervisor exits cleanly, which clears the record. + if err := supervise.ClearEndpoint(dir); err != nil { + t.Fatal(err) + } + if got := customPipeToPreserve(dir); got != "" { + t.Fatalf("the record survived a clean exit (%q); this test no longer models the bug", got) + } + + // 4. The replacement is built. Reading the record here finds nothing, + // which is exactly why the captured value has to be carried. + _, args := supervisorCommand(self, dir, captured) + if got := pipeArg(args); got != custom { + t.Errorf("args %v carry pipe %q, want %q — the replacement will re-select and "+ + "DOCKER_HOST stops working (#429)", args, got, custom) + } +} + +// The crash path: a supervisor killed hard runs no cleanup, so its record +// survives and is the only thing that remembers the pipe. No captured value is +// available there, so the recorded fallback has to work. +func TestCrashRestartUsesTheRecordedPipe(t *testing.T) { + self := filepath.Join(t.TempDir(), "skrog.exe") + dir := t.TempDir() + const custom = `\\.\pipe\skrog-e2e-suite` + if err := supervise.WriteEndpoint(dir, supervise.Endpoint{Pipe: custom}); err != nil { + t.Fatal(err) + } + _, args := supervisorCommand(self, dir, "") // nothing captured + if got := pipeArg(args); got != custom { + t.Errorf("args %v carry pipe %q, want the recorded %q", args, got, custom) + } +} + +func TestSupervisorCommandLeavesTheOrdinaryCaseAlone(t *testing.T) { + self := filepath.Join(t.TempDir(), "skrog.exe") t.Run("the default pipe is not forwarded", func(t *testing.T) { dir := t.TempDir() if err := supervise.WriteEndpoint(dir, supervise.Endpoint{Pipe: pipeproxy.DefaultPipeName}); err != nil { t.Fatal(err) } - if _, args := supervisorCommand(self, dir); slices.Contains(args, "--pipe") { + if _, args := supervisorCommand(self, dir, ""); slices.Contains(args, "--pipe") { t.Errorf("args %v pin the default pipe; normal selection must stay free to fall back", args) } }) t.Run("no endpoint means no --pipe", func(t *testing.T) { - if _, args := supervisorCommand(self, t.TempDir()); slices.Contains(args, "--pipe") { + if _, args := supervisorCommand(self, t.TempDir(), ""); slices.Contains(args, "--pipe") { t.Errorf("args %v carry --pipe with nothing recorded", args) } }) @@ -126,7 +168,7 @@ func TestSupervisorCommandForwardsACustomPipe(t *testing.T) { // The state dir must always survive, whatever happens to the pipe. t.Run("the state dir is always passed", func(t *testing.T) { dir := t.TempDir() - if _, args := supervisorCommand(self, dir); !slices.Contains(args, "--state-dir") { + if _, args := supervisorCommand(self, dir, ""); !slices.Contains(args, "--state-dir") { t.Errorf("args %v lost --state-dir", args) } }) diff --git a/cmd/skrog/supervise.go b/cmd/skrog/supervise.go index ede5d03..3015aa8 100644 --- a/cmd/skrog/supervise.go +++ b/cmd/skrog/supervise.go @@ -432,7 +432,18 @@ func optsWithResolvedStateDir(opts provision.Options) provision.Options { return opts } -func runStart(args []string) int { +func runStart(args []string) int { return runStartPreserving(args, "") } + +// runStartPreserving is runStart with the pipe a replacement supervisor must +// keep serving (#429). +// +// Separate from runStart, rather than a `--pipe` flag on `start`, because this +// is not something a user asks for: it is `restart --supervisor` carrying a +// value across a teardown that would otherwise erase it. A CLI flag would +// document a decision the caller never makes. +// +// Empty means "choose normally", which is every path except that one. +func runStartPreserving(args []string, preservePipe string) int { fs := flag.NewFlagSet("start", flag.ContinueOnError) stateDir := fs.String("state-dir", "", "override Skrog's state directory") timeout := fs.Duration("timeout", 2*time.Minute, "how long to wait for the engine") @@ -474,7 +485,7 @@ running, and waits for the engine to answer. if !supervise.Held(opts.StateDir) { fmt.Fprintln(os.Stderr, " starting the supervisor in the background") - if err := spawnSupervisor(opts.StateDir); err != nil { + if err := spawnSupervisor(opts.StateDir, preservePipe); err != nil { fmt.Fprintf(os.Stderr, "skrog: launching supervisor: %v\n", err) return exitError } @@ -642,9 +653,21 @@ supervisor itself is misbehaving, or after replacing skrog.exe on disk. } if *supervisor { dir := optsWithResolvedStateDir(provision.Options{StateDir: *stateDir}).StateDir + + // Captured BEFORE recycling, because the supervisor deletes its own + // endpoint record on the way out -- runSupervise clears it in a defer, + // so a clean exit is precisely the case where the record is gone by + // the time anything downstream could read it (#429). + // + // Reading it afterwards is what the first attempt at this did. It + // worked for a hard kill, where the record survives, and not for the + // restart it was written for. + keepPipe := customPipeToPreserve(dir) + if code := recycleSupervisor(dir); code != exitOK { return code } + return runStartPreserving(pass, keepPipe) } return runStart(pass) } @@ -876,12 +899,12 @@ func fileExists(path string) bool { // seconds of pipe downtime rather than every docker command until the next // `skrog start` (#166). Falling back to spawning supervise directly keeps a // single-binary checkout working, just without the watchdog. -func spawnSupervisor(stateDir string) error { +func spawnSupervisor(stateDir, preservePipe string) error { self, err := selfexe.Path() if err != nil { return err } - target, args := supervisorCommand(self, stateDir) + target, args := supervisorCommand(self, stateDir, preservePipe) cmd := exec.Command(target, args...) configureDetached(cmd) if err := cmd.Start(); err != nil { @@ -899,12 +922,26 @@ func spawnSupervisor(stateDir string) error { // customPipeToPreserve passed with the wiring deleted — a correct helper // nothing called, which is the exact shape of three other defects in this // release. -func supervisorCommand(self, stateDir string) (target string, args []string) { +func supervisorCommand(self, stateDir, preservePipe string) (target string, args []string) { target, args = self, []string{"supervise", "--state-dir", stateDir} if launcher := filepath.Join(filepath.Dir(self), "skrogw.exe"); fileExists(launcher) { target, args = launcher, []string{"--state-dir", stateDir} } - if pipe := customPipeToPreserve(stateDir); pipe != "" { + + // Two sources, and the order matters. + // + // An explicit value comes from `restart --supervisor`, which read the + // endpoint BEFORE tearing the old supervisor down — the record is gone by + // now, because runSupervise clears it on a clean exit. + // + // The recorded fallback covers the other case: a supervisor that died + // hard ran no cleanup, so its record survives and is the only thing that + // remembers which pipe was being served. + pipe := preservePipe + if pipe == "" { + pipe = customPipeToPreserve(stateDir) + } + if pipe != "" { args = append(args, "--pipe", pipe) } return target, args