diff --git a/go/cmd/compass-app/embedded.go b/go/cmd/compass-app/embedded.go index 1e36c4b0..3eb1d2eb 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,37 @@ 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) (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 { + // 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() { + _ = 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 +185,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 +234,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..2f251693 100644 --- a/go/cmd/compass-app/embedded_test.go +++ b/go/cmd/compass-app/embedded_test.go @@ -300,6 +300,47 @@ 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 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 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, 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) + } +} + // 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