fix: a replacement supervisor keeps the pipe it was serving

cmd/skrog/pipepreserve_test.go

@@ -0,0 +1,133 @@
+package main
+
+import (
+	"path/filepath"
+	"slices"
+	"testing"
+
+	"github.com/wslkit/skrog/internal/pipeproxy"
+	"github.com/wslkit/skrog/internal/supervise"
+)
+
+// `restart --supervisor` rebuilds the supervisor's arguments rather than
+// forwarding them, and the served pipe was not among what it carried (#429).
+// A supervisor started with --pipe <custom> came back on the default, so
+// DOCKER_HOST stopped working with an error naming a missing file rather than
+// a moved pipe. The watchdog relaunch went through the same path.
+//
+// The two exclusions are the interesting part, and they are what keep this
+// from breaking the ordinary install.
+func TestCustomPipeToPreserve(t *testing.T) {
+	for _, tc := range []struct {
+		name     string
+		recorded string
+		want     string
+		why      string
+	}{
+		{
+			name:     "a pipe someone asked for by name is kept",
+			recorded: `\\.\pipe\skrog-e2e-suite`,
+			want:     `\\.\pipe\skrog-e2e-suite`,
+			why:      "this is the case that was being lost",
+		},
+		{
+			// DefaultPipeName and FallbackPipeName are already full
+			// `\\.\pipe\...` paths. Concatenating another prefix is how the
+			// first draft of this test silently passed nothing to compare.
+			name:     "the default is NOT pinned",
+			recorded: pipeproxy.DefaultPipeName,
+			want:     "",
+			why: "normal selection takes it again when free and falls back when " +
+				"Docker Desktop has it; pinning would turn that fallback into a failure to bind",
+		},
+		{
+			name:     "the fallback is NOT pinned",
+			recorded: pipeproxy.FallbackPipeName,
+			want:     "",
+			why: "selection re-derives it, and pinning would make the fallback sticky — " +
+				"a machine that stopped running Desktop would never take the default back",
+		},
+		{
+			name:     "case does not matter",
+			recorded: `\\.\pipe\DOCKER_ENGINE`,
+			want:     "",
+			why:      "Windows pipe names are case-insensitive",
+		},
+		{
+			// A bare name, in case anything ever records one: the comparison
+			// must not depend on the prefix being present.
+			name:     "a bare default name is still the default",
+			recorded: "docker_engine",
+			want:     "",
+			why:      "pipeEq trims the prefix on both sides",
+		},
+		{
+			name:     "no record means choose normally",
+			recorded: "",
+			want:     "",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			dir := t.TempDir()
+			if tc.recorded != "" {
+				if err := supervise.WriteEndpoint(dir, supervise.Endpoint{Pipe: tc.recorded}); err != nil {
+					t.Fatal(err)
+				}
+			}
+			if got := customPipeToPreserve(dir); got != tc.want {
+				t.Errorf("customPipeToPreserve = %q, want %q\n  %s", got, tc.want, tc.why)
+			}
+		})
+	}
+}
+
+// The wiring, not just the helper.
+//
+// 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) {
+	self := filepath.Join(t.TempDir(), "skrog.exe")
+
+	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)
+		}
+	})
+
+	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") {
+			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") {
+			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") {
+			t.Errorf("args %v lost --state-dir", args)
+		}
+	})
+}

cmd/skrog/supervise.go

@@ -9,6 +9,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"sync"
 	"time"
 
@@ -880,10 +881,7 @@ func spawnSupervisor(stateDir string) error {
 	if err != nil {
 		return err
 	}
-	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}
-	}
+	target, args := supervisorCommand(self, stateDir)
 	cmd := exec.Command(target, args...)
 	configureDetached(cmd)
 	if err := cmd.Start(); err != nil {
@@ -893,6 +891,73 @@ func spawnSupervisor(stateDir string) error {
 	return cmd.Process.Release()
 }
 
+// supervisorCommand builds what spawnSupervisor launches.
+//
+// Split out from spawnSupervisor so the ARGUMENTS can be tested without
+// starting a process. That distinction is not pedantry: the first version of
+// this fix put the --pipe wiring inline, and the test for
+// 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) {
+	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 != "" {
+		args = append(args, "--pipe", pipe)
+	}
+	return target, args
+}
+
+// customPipeToPreserve returns the pipe a replacement supervisor must keep
+// serving, or "" to let it choose normally (#429).
+//
+// `skrog restart --supervisor` rebuilds the argument list rather than
+// forwarding it, and the pipe was not among what it carried. So a supervisor
+// started with `--pipe <custom>` came back on the DEFAULT pipe, DOCKER_HOST
+// stopped working, and the error named a missing file rather than a moved
+// pipe. Same for the watchdog path: skrogw relaunches through here too, so a
+// crash lost the pipe the same way.
+//
+// Only a genuinely custom pipe is preserved, and the two exclusions are what
+// keep this from breaking the ordinary case:
+//
+//   - The DEFAULT pipe is not preserved. Normal selection takes it again when
+//     it is free, and correctly falls back when something else (Docker
+//     Desktop) has taken it in the meantime. Pinning it would turn that
+//     graceful fallback into a hard failure to bind.
+//   - The FALLBACK pipe is not preserved either, because normal selection
+//     re-derives it: it is only ever chosen when the default is taken. Pinning
+//     it 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 the case
+// that was being lost.
+func customPipeToPreserve(stateDir string) string {
+	e, ok := supervise.ReadEndpoint(stateDir)
+	if !ok || e.Pipe == "" {
+		return ""
+	}
+	// Both sides are normalised. DefaultPipeName and FallbackPipeName are full
+	// `\\.\pipe\...` paths, and a recorded endpoint should be too -- but
+	// comparing one against the other's bare name silently matches nothing,
+	// which makes every exclusion below a no-op and the fallback sticky.
+	// Trimming both is what makes the comparison mean what it reads as.
+	if pipeEq(e.Pipe, pipeproxy.DefaultPipeName) || pipeEq(e.Pipe, pipeproxy.FallbackPipeName) {
+		return ""
+	}
+	return e.Pipe
+}
+
+// pipeEq compares two pipe names, tolerating the `\\.\pipe\` prefix on either
+// side. Windows pipe names are case-insensitive.
+func pipeEq(a, b string) bool {
+	const prefix = `\\.\pipe\`
+	return strings.EqualFold(strings.TrimPrefix(a, prefix), strings.TrimPrefix(b, prefix))
+}
+
 // statsFlushInterval is how often the supervisor publishes its counters. Five
 // seconds keeps a reading current enough to act on while costing one small
 // atomic write; supervise.Stats.Fresh() allows six times that before calling a