fix: capture the pipe before the supervisor deletes its own record

cmd/skrog/pipepreserve_test.go

@@ -81,52 +81,94 @@ 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)
 		}
 	})
 
 	// 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)
 		}
 	})

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