From 39424f952dfe8db64f38c254991c5b2742b6ef1b Mon Sep 17 00:00:00 2001 From: Zoltan Csizmadia Date: Sun, 20 Sep 2026 08:54:36 -0500 Subject: [PATCH] fix: a replacement supervisor keeps the pipe it was serving 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 ` 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. --- cmd/skrog/pipepreserve_test.go | 133 +++++++++++++++++++++++++++++++++ cmd/skrog/supervise.go | 73 +++++++++++++++++- 2 files changed, 202 insertions(+), 4 deletions(-) create mode 100644 cmd/skrog/pipepreserve_test.go diff --git a/cmd/skrog/pipepreserve_test.go b/cmd/skrog/pipepreserve_test.go new file mode 100644 index 0000000..0c5c61c --- /dev/null +++ b/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 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) + } + }) +} diff --git a/cmd/skrog/supervise.go b/cmd/skrog/supervise.go index 1cd308c..ede5d03 100644 --- a/cmd/skrog/supervise.go +++ b/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 ` 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