From 61cd811951340da6cbe35d72839ee6e9ff08fe94 Mon Sep 17 00:00:00 2001 From: seal Date: Fri, 7 Aug 2026 13:51:17 -0400 Subject: [PATCH 1/2] fix(compass-app): stop runStackUp/Down hanging on a fire-and-return stack (SEA-1685) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runStackUp/runStackDown captured the compass-stack child's stderr into a bytes.Buffer. os/exec backs a non-*os.File stderr writer with an OS pipe whose copy goroutine Cmd.Wait blocks on until EOF. `compass-stack up` is fire-and-return: it exits 0 once the stack is Ready while its postgres/server/runner children keep running, and those children inherit the pipe's write-end — so EOF never arrives and Wait hangs for the children's whole lifetime. Embedded launch therefore never completed against a real stack (the T4.1/T4.2 unit tests passed only because their /bin/sh children leave no survivors). Capture stderr to a temp *os.File instead (extracted into captureStderr, shared by both seams): an *os.File is dup'd straight into the child (no pipe, no goroutine), so Wait returns the instant compass-stack itself exits regardless of lingering children, and the children write to a plain file that never EPIPEs, so the capture never signals the stack the app must keep alive. The failure-copy contract (surface the child's stderr on a non-zero exit) is preserved. Regression: TestRunStackUpReturnsWhileChildrenLinger drives /bin/sh that backgrounds a long sleep holding stderr then exits 0 — the fire-and-return shape. Pre-fix it hangs for the sleep's lifetime; the fix returns in milliseconds. Surfaced by the SEA-1685 T4.3 e2e integration test driving the real embedded composition root against a real compass-stack (up -> real WhoAmI resolved a live account id, DL-111). The full T4.3 e2e + its teardown assertions are gated on SEA-1880 (compass-stack down is a no-op across a process boundary) and land once Matt rules the teardown mechanism. Co-authored-by: Matt Wilkinson --- go/cmd/compass-app/embedded.go | 48 ++++++++++++++++++++++++----- go/cmd/compass-app/embedded_test.go | 39 +++++++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/go/cmd/compass-app/embedded.go b/go/cmd/compass-app/embedded.go index 1e36c4b0..80f912ad 100644 --- a/go/cmd/compass-app/embedded.go +++ b/go/cmd/compass-app/embedded.go @@ -17,7 +17,6 @@ package main import ( - "bytes" "context" "errors" "fmt" @@ -145,6 +144,35 @@ func stackUpArgs(p embeddedParams) []string { return args } +// captureStderr wires cmd.Stderr to a temp *os.File and returns a reader for the +// bytes captured so far plus a cleanup that closes and removes the file. +// Capturing to an *os.File — not a bytes.Buffer — is load-bearing for the +// fire-and-return stack commands: `compass-stack up` exits 0 once the stack is +// Ready while its postgres/server/runner children keep running. os/exec backs a +// non-*os.File stderr writer with an OS pipe whose copy goroutine Cmd.Wait +// blocks on until EOF, and those lingering children inherit the pipe's +// write-end, so EOF never arrives and Wait hangs forever. An *os.File is dup'd +// straight into the child (no pipe, no goroutine), so Wait returns the instant +// compass-stack itself exits; and the children write to a plain file that never +// EPIPEs, so capturing this way never signals the very stack the app must keep +// alive. +func captureStderr(cmd *exec.Cmd) (func() string, func(), error) { + f, err := os.CreateTemp("", "compass-stack-stderr-*") + if err != nil { + return nil, nil, fmt.Errorf("creating stderr capture file: %w", err) + } + cmd.Stderr = f + read := func() string { + b, _ := os.ReadFile(f.Name()) + return strings.TrimSpace(string(b)) + } + cleanup := func() { + _ = f.Close() + _ = os.Remove(f.Name()) + } + return read, cleanup, nil +} + // runStackUp is the real stackUp seam: it execs the compass-stack binary at bin // with the given argv and waits for it to exit 0 (up is fire-and-return, so // Run returning nil means the stack reached Ready and its children keep @@ -155,14 +183,17 @@ func runStackUp(bin string) func(ctx context.Context, args []string) error { //nolint:gosec // G204: bin is operator/PATH-resolved (resolveStackBin) and // the argv is pipeline-assembled (stackUpArgs), not user input. cmd := exec.CommandContext(ctx, bin, args...) - var stderr bytes.Buffer - cmd.Stderr = &stderr + stderr, cleanup, capErr := captureStderr(cmd) + if capErr != nil { + return capErr + } + defer cleanup() if err := cmd.Run(); err != nil { if ctx.Err() == context.DeadlineExceeded || errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("compass-stack up exceeded the %s bring-up window "+ "(a cold agent-image pull from GHCR can take longer on first run): %w", bringUpTimeout, err) } - if msg := strings.TrimSpace(stderr.String()); msg != "" { + if msg := stderr(); msg != "" { return fmt.Errorf("compass-stack up failed: %w: %s", err, msg) } return fmt.Errorf("compass-stack up failed: %w", err) @@ -201,14 +232,17 @@ func runStackDown(bin string) func(ctx context.Context, args []string) error { //nolint:gosec // G204: bin is operator/PATH-resolved (resolveStackBin) and // the argv is pipeline-assembled (stackDownArgs), not user input. cmd := exec.CommandContext(ctx, bin, args...) - var stderr bytes.Buffer - cmd.Stderr = &stderr + stderr, cleanup, capErr := captureStderr(cmd) + if capErr != nil { + return capErr + } + defer cleanup() if err := cmd.Run(); err != nil { if ctx.Err() == context.DeadlineExceeded || errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("compass-stack down exceeded the %s teardown window "+ "(attach, SIGTERM the child tree, wait the server drain): %w", stackDownTimeout, err) } - if msg := strings.TrimSpace(stderr.String()); msg != "" { + if msg := stderr(); msg != "" { return fmt.Errorf("compass-stack down failed: %w: %s", err, msg) } return fmt.Errorf("compass-stack down failed: %w", err) diff --git a/go/cmd/compass-app/embedded_test.go b/go/cmd/compass-app/embedded_test.go index 3101550b..06fb50ae 100644 --- a/go/cmd/compass-app/embedded_test.go +++ b/go/cmd/compass-app/embedded_test.go @@ -300,6 +300,45 @@ func TestRunStackUpZeroExitSucceeds(t *testing.T) { } } +// TestRunStackUpReturnsWhileChildrenLinger is the regression guard for the +// fire-and-return hang: `compass-stack up` exits 0 once the stack is Ready while +// its postgres/server/runner children keep running, and those children inherit +// the exec'd command's stderr. If runStackUp captured stderr into a bytes.Buffer +// (os/exec's pipe + copy-goroutine path), cmd.Wait would block until the pipe +// hit EOF — which the lingering children hold open — so Run would hang for the +// children's whole lifetime. Capturing to a temp *os.File (captureStderr) makes +// Run return the instant the top-level child exits, regardless of survivors. +// +// Driven with /bin/sh that backgrounds a long sleep (a stand-in for the +// reparented stack children) holding stderr, then exits 0. Pre-fix this blocks +// for the sleep's 60s; the fix returns immediately. The assertion is that Run +// completes well under the sleep — a plain wall-clock bound, but the pre-fix gap +// (60s vs milliseconds) is enormous, so it is not flaky. The backgrounded sleep +// is cleaned up via its own short lifetime; the test spawns nothing it must kill +// (rule://process-safety — never pkill). +func TestRunStackUpReturnsWhileChildrenLinger(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), embeddedTestTimeout) + defer cancel() + + // A short-lived grandchild that outlives its parent and inherits stderr: the + // exact fire-and-return shape of `compass-stack up`. sleep 60 is far longer + // than any correct runStackUp (which returns at the parent's exit, ~ms) yet + // bounded so a REGRESSION leaves no minutes-long orphan. + stackUp := runStackUp("/bin/sh") + start := time.Now() + err := stackUp(ctx, []string{"-c", "sleep 60 & exit 0"}) + elapsed := time.Since(start) + + if err != nil { + t.Fatalf("stackUp with a lingering child err = %v, want nil", err) + } + // Generous bound: the fix returns in single-digit ms; the pre-fix hang is + // 60s. Anything under a second proves Run did not wait on the grandchild. + if elapsed > time.Second { + t.Fatalf("stackUp took %s with a lingering child — Run waited on the inherited stderr pipe (the fire-and-return hang regressed)", elapsed) + } +} + // TestRunStackDownNonZeroExitSurfacesStderr: the real stackDown seam surfaces a // non-zero exit as an error carrying the child's stderr, mirroring runStackUp. // Driven with /bin/sh printing to stderr and exiting 1 — no real compass-stack From fbd6224a7da5e1df5d10dd61cf76e65ff729b673 Mon Sep 17 00:00:00 2001 From: seal Date: Fri, 7 Aug 2026 14:16:21 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(compass-app):=20address=20PR=20#227=20r?= =?UTF-8?q?eview=20=E2=80=94=20named=20returns,=20best-effort=20read=20not?= =?UTF-8?q?e,=20shorter=20test=20sleep=20(SEA-1685)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three low-severity review polish items (no high/medium findings; reviewer APPROVE): - captureStderr: name the returns (read/cleanup/err) so the signature self-documents; note that read's dropped ReadFile error is intentional best-effort degradation to the generic failure message. - TestRunStackUpReturnsWhileChildrenLinger: shrink the lingering grandchild from `sleep 60` to `sleep 5` — still far past the 1s regression bound, but a regressed run's leaked child self-reaps in seconds not a minute (shared-box hygiene under repeat/-count loops). No behavior change; union gate green (gofmt/vet/golangci 0-issues/-race) both tag sets. Co-authored-by: Matt Wilkinson --- go/cmd/compass-app/embedded.go | 8 +++++--- go/cmd/compass-app/embedded_test.go | 14 ++++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/go/cmd/compass-app/embedded.go b/go/cmd/compass-app/embedded.go index 80f912ad..3eb1d2eb 100644 --- a/go/cmd/compass-app/embedded.go +++ b/go/cmd/compass-app/embedded.go @@ -156,17 +156,19 @@ func stackUpArgs(p embeddedParams) []string { // compass-stack itself exits; and the children write to a plain file that never // EPIPEs, so capturing this way never signals the very stack the app must keep // alive. -func captureStderr(cmd *exec.Cmd) (func() string, func(), error) { +func captureStderr(cmd *exec.Cmd) (read func() string, cleanup func(), err error) { f, err := os.CreateTemp("", "compass-stack-stderr-*") if err != nil { return nil, nil, fmt.Errorf("creating stderr capture file: %w", err) } cmd.Stderr = f - read := func() string { + read = func() string { + // Best-effort: an unreadable capture degrades to the generic + // "compass-stack ... failed" error, and never blocks surfacing. b, _ := os.ReadFile(f.Name()) return strings.TrimSpace(string(b)) } - cleanup := func() { + cleanup = func() { _ = f.Close() _ = os.Remove(f.Name()) } diff --git a/go/cmd/compass-app/embedded_test.go b/go/cmd/compass-app/embedded_test.go index 06fb50ae..2f251693 100644 --- a/go/cmd/compass-app/embedded_test.go +++ b/go/cmd/compass-app/embedded_test.go @@ -321,19 +321,21 @@ func TestRunStackUpReturnsWhileChildrenLinger(t *testing.T) { defer cancel() // A short-lived grandchild that outlives its parent and inherits stderr: the - // exact fire-and-return shape of `compass-stack up`. sleep 60 is far longer - // than any correct runStackUp (which returns at the parent's exit, ~ms) yet - // bounded so a REGRESSION leaves no minutes-long orphan. + // exact fire-and-return shape of `compass-stack up`. sleep 5 is far longer + // than any correct runStackUp (which returns at the parent's exit, ~ms) and + // well past the 1s assertion below, yet short enough that a regressed run's + // leaked grandchild self-reaps in seconds rather than a minute. stackUp := runStackUp("/bin/sh") start := time.Now() - err := stackUp(ctx, []string{"-c", "sleep 60 & exit 0"}) + err := stackUp(ctx, []string{"-c", "sleep 5 & exit 0"}) elapsed := time.Since(start) if err != nil { t.Fatalf("stackUp with a lingering child err = %v, want nil", err) } - // Generous bound: the fix returns in single-digit ms; the pre-fix hang is - // 60s. Anything under a second proves Run did not wait on the grandchild. + // Generous bound: the fix returns in single-digit ms, while a regressed Run + // blocks until the grandchild exits (~5s) — far past 1s. Anything under a + // second proves Run did not wait on the grandchild. if elapsed > time.Second { t.Fatalf("stackUp took %s with a lingering child — Run waited on the inherited stderr pipe (the fire-and-return hang regressed)", elapsed) }