diff --git a/go/internal/runtime/clispawn.go b/go/internal/runtime/clispawn.go index 892e9fe3c..b5403cad3 100644 --- a/go/internal/runtime/clispawn.go +++ b/go/internal/runtime/clispawn.go @@ -33,7 +33,15 @@ type cliEngine struct { // Only a spawn failure, this call's own timeout, or parent-context cancellation // is a non-nil error. func (e cliEngine) spawnCapture(ctx context.Context, summary string, args []string, stdin *string) (stdout, stderr []byte, exitCode int, err error) { - cctx, cancel := context.WithTimeout(ctx, e.timeout) + return e.spawnCaptureWithCancellation(ctx, summary, args, stdin, false) +} + +func (e cliEngine) spawnCaptureWithCancellation(ctx context.Context, summary string, args []string, stdin *string, detached bool) (stdout, stderr []byte, exitCode int, err error) { + commandCtx := ctx + if detached { + commandCtx = context.WithoutCancel(ctx) + } + cctx, cancel := context.WithTimeout(commandCtx, e.timeout) defer cancel() //nolint:gosec // G204: this is the container-engine seam — spawning the @@ -55,7 +63,8 @@ func (e cliEngine) spawnCapture(ctx context.Context, summary string, args []stri cmd.Stdin = strings.NewReader(*stdin) } - if runErr := cmd.Run(); runErr != nil { + runErr := cmd.Run() + if runErr != nil { switch { case cctx.Err() == context.DeadlineExceeded && ctx.Err() == nil: // This call's own timeout fired (not the parent): the process was @@ -75,10 +84,14 @@ func (e cliEngine) spawnCapture(ctx context.Context, summary string, args []stri return out.Bytes(), errBuf.Bytes(), 0, nil } -// run runs ` `, requiring a zero exit (a non-zero becomes a -// CommandError). For fire-and-check operations like create/start/stop/remove. -func (e cliEngine) run(ctx context.Context, summary string, args []string) ([]byte, error) { - stdout, stderr, exitCode, err := e.spawnCapture(ctx, summary, args, nil) +// runDetached is run without caller cancellation, bounded only by the timeout: +// a signal mid netns setup can leave pasta running without resolv.conf. +func (e cliEngine) runDetached(ctx context.Context, summary string, args []string) ([]byte, error) { + return e.runWithCancellation(ctx, summary, args, true) +} + +func (e cliEngine) runWithCancellation(ctx context.Context, summary string, args []string, detached bool) ([]byte, error) { + stdout, stderr, exitCode, err := e.spawnCaptureWithCancellation(ctx, summary, args, nil, detached) if err != nil { return nil, err } @@ -92,6 +105,12 @@ func (e cliEngine) run(ctx context.Context, summary string, args []string) ([]by return stdout, nil } +// run runs ` `, requiring a zero exit (a non-zero becomes a +// CommandError). For fire-and-check operations like create/start/stop/remove. +func (e cliEngine) run(ctx context.Context, summary string, args []string) ([]byte, error) { + return e.runWithCancellation(ctx, summary, args, false) +} + // stopWithClientScript runs "$@" with stdin relayed by a watcher in the same // process group. Killing the engine client closes that stdin, and the watcher // then SIGKILLs the group; a natural exit kills the watcher and keeps the status. diff --git a/go/internal/runtime/podman.go b/go/internal/runtime/podman.go index 7c9dabc03..87f94d600 100644 --- a/go/internal/runtime/podman.go +++ b/go/internal/runtime/podman.go @@ -569,7 +569,7 @@ func (p *PodmanCLI) VerifyNoPastaForwards(ctx context.Context, image string) err if mode != "pasta" && !strings.HasPrefix(mode, "pasta:") { return nil } - _, err = p.run(ctx, "podman run (pasta canary)", []string{ + _, err = p.runDetached(ctx, "podman run (pasta canary)", []string{ "run", "--rm", "--network", pastaCanaryNetwork, probeEntrypoint, image, }) if cmdErr, ok := errors.AsType[*CommandError](err); ok && strings.Contains(cmdErr.Stderr, "conflicts with previous options") { @@ -605,7 +605,7 @@ func parsePodmanVersion(s string) (major, minor int, err error) { // Start starts a created container. func (p *PodmanCLI) Start(ctx context.Context, id WorkloadID) error { - _, err := p.run(ctx, "podman start", []string{"start", id.String()}) + _, err := p.runDetached(ctx, "podman start", []string{"start", id.String()}) return err } diff --git a/go/internal/runtime/podman_test.go b/go/internal/runtime/podman_test.go index eb0341970..fcdbaaa6f 100644 --- a/go/internal/runtime/podman_test.go +++ b/go/internal/runtime/podman_test.go @@ -629,3 +629,82 @@ esac }) } } + +// A cancelled caller must not kill podman start: a signal mid netns setup +// leaves pasta running without resolv.conf. The real outcome is returned. +func TestPodmanStartSurvivesCallerCancellation(t *testing.T) { + dir := t.TempDir() + started := filepath.Join(dir, "started") + proceed := filepath.Join(dir, "go") + doneMarker := filepath.Join(dir, "done") + prog := filepath.Join(dir, "podman-stub.sh") + script := fmt.Sprintf(`#!/bin/sh +if [ "$1" != "start" ]; then exit 91; fi +echo $$ > %q +while [ ! -e %q ]; do :; done +: > %q +`, started, proceed, doneMarker) + if err := os.WriteFile(prog, []byte(script), 0o755); err != nil { + t.Fatalf("writing stub: %v", err) + } + t.Cleanup(func() { killRecordedPid(t, started) }) + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + cli := NewPodmanCLI().WithProgram(prog) + result := make(chan error, 1) + go func() { result <- cli.Start(ctx, WorkloadID("container")) }() + pid := waitForRecordedPid(t, started) + cancel() + // A cancelled exec.CommandContext kills at once; the stub stays gated, so + // any return here is the kill. The bound can only miss a regression. + select { + case err := <-result: + t.Fatalf("Start returned %v while the stub was still gated: cancellation killed podman", err) + case <-time.After(200 * time.Millisecond): + } + if err := syscall.Kill(pid, 0); err != nil { + t.Fatalf("stub pid %d after cancellation: kill(0) = %v, want alive", pid, err) + } + if err := os.WriteFile(proceed, nil, 0o600); err != nil { + t.Fatalf("creating go marker: %v", err) + } + + select { + case err := <-result: + if err != nil { + t.Errorf("Start error = %v, want nil (the stub exited 0)", err) + } + case <-time.After(10 * time.Second): + t.Fatal("podman start did not return after the stub was released") + } + if _, err := os.Stat(doneMarker); err != nil { + t.Errorf("stub did not finish after caller cancellation: %v", err) + } +} + +func TestPodmanStartTimeoutKillsHungProcess(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "child.pid") + prog := filepath.Join(t.TempDir(), "podman-stub.sh") + script := fmt.Sprintf("#!/bin/sh\necho $$ > %q\nexec sleep 30\n", pidFile) + if err := os.WriteFile(prog, []byte(script), 0o755); err != nil { + t.Fatalf("writing stub: %v", err) + } + t.Cleanup(func() { killRecordedPid(t, pidFile) }) + cli := NewPodmanCLI().WithProgram(prog).WithTimeout(100 * time.Millisecond) + result := make(chan error, 1) + go func() { result <- cli.Start(t.Context(), WorkloadID("container")) }() + pid := waitForRecordedPid(t, pidFile) + + select { + case err := <-result: + if _, ok := errors.AsType[*TimeoutError](err); !ok { + t.Fatalf("Start error = %v (%T), want *TimeoutError", err, err) + } + case <-time.After(5 * time.Second): + t.Fatal("podman start did not return after its command timeout") + } + if err := syscall.Kill(pid, 0); !errors.Is(err, syscall.ESRCH) { + t.Fatalf("after start timeout, kill(%d, 0) = %v, want ESRCH", pid, err) + } +} diff --git a/go/internal/stack/adapters/postgres_container.go b/go/internal/stack/adapters/postgres_container.go index 87545a515..a1fdd200c 100644 --- a/go/internal/stack/adapters/postgres_container.go +++ b/go/internal/stack/adapters/postgres_container.go @@ -292,9 +292,11 @@ func newPodmanExec() *podmanExec { return &podmanExec{program: "podman", timeout: containerCommandTimeout} } -// run runs `podman ` to completion, requiring a zero exit. +// run runs `podman ` to completion, requiring a zero exit. Caller +// cancellation is not delivered: a signal mid netns setup can leave pasta +// running without resolv.conf, breaking every later podman run. func (e *podmanExec) run(ctx context.Context, args []string) error { - return e.fireAndCheck(ctx, args) + return e.fireAndCheckWithCancellation(ctx, args, true) } // wait blocks until the named container exits (`podman wait `). A @@ -387,18 +389,27 @@ func (e *podmanExec) exists(ctx context.Context, name string) (bool, error) { // fireAndCheck runs `podman ` under the command timeout, folding a // non-zero exit into an error carrying the captured stderr for diagnosis. func (e *podmanExec) fireAndCheck(ctx context.Context, args []string) error { - cctx, cancel := context.WithTimeout(ctx, e.timeout) + return e.fireAndCheckWithCancellation(ctx, args, false) +} + +func (e *podmanExec) fireAndCheckWithCancellation(ctx context.Context, args []string, detached bool) error { + commandCtx := ctx + if detached { + commandCtx = context.WithoutCancel(ctx) + } + cctx, cancel := context.WithTimeout(commandCtx, e.timeout) defer cancel() cmd := exec.CommandContext(cctx, e.program, args...) //nolint:gosec // G204: the container seam — program is the operator-set engine and args are Stack-built from a state-dir-derived spec, neither attacker-controlled cmd.WaitDelay = podmanWaitDelay var stderr strings.Builder cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { + runErr := cmd.Run() + if runErr != nil { msg := strings.TrimSpace(stderr.String()) if msg == "" { - return fmt.Errorf("podman %s: %w", args[0], err) + return fmt.Errorf("podman %s: %w", args[0], runErr) } - return fmt.Errorf("podman %s: %w: %s", args[0], err, msg) + return fmt.Errorf("podman %s: %w: %s", args[0], runErr, msg) } return nil } diff --git a/go/internal/stack/adapters/postgres_container_test.go b/go/internal/stack/adapters/postgres_container_test.go index 669c151f6..c1428f889 100644 --- a/go/internal/stack/adapters/postgres_container_test.go +++ b/go/internal/stack/adapters/postgres_container_test.go @@ -5,10 +5,13 @@ package adapters import ( "context" "errors" + "fmt" "os" "path/filepath" "reflect" + "strconv" "strings" + "syscall" "testing" "time" @@ -350,3 +353,129 @@ func TestStopSecondsRounding(t *testing.T) { } } } + +// A cancelled caller must not kill podman run: a signal mid netns setup leaves +// pasta running without resolv.conf. The real outcome is returned, so a +// container that did start is still recorded for teardown. +func TestPodmanRunSurvivesCallerCancellation(t *testing.T) { + dir := t.TempDir() + started := filepath.Join(dir, "started") + proceed := filepath.Join(dir, "go") + doneMarker := filepath.Join(dir, "done") + prog := filepath.Join(dir, "podman-stub.sh") + script := fmt.Sprintf(`#!/bin/sh +if [ "$1" != "run" ]; then exit 91; fi +echo $$ > %q.tmp && mv %q.tmp %q +while [ ! -e %q ]; do :; done +: > %q +`, started, started, started, proceed, doneMarker) + if err := os.WriteFile(prog, []byte(script), 0o700); err != nil { + t.Fatalf("writing stub: %v", err) + } + + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + cli := &podmanExec{program: prog, timeout: 10 * time.Second} + result := make(chan error, 1) + go func() { result <- cli.run(ctx, []string{"run", "--detach", "image"}) }() + waitForPodmanMarker(t, started) + pid := readPid(t, started) + t.Cleanup(func() { _ = syscall.Kill(pid, syscall.SIGKILL) }) + cancel() + // A cancelled exec.CommandContext kills at once; the stub stays gated, so + // any return here is the kill. The bound can only miss a regression. + select { + case err := <-result: + t.Fatalf("run returned %v while the stub was still gated: cancellation killed podman", err) + case <-time.After(200 * time.Millisecond): + } + if err := syscall.Kill(pid, 0); err != nil { + t.Fatalf("stub pid %d after cancellation: kill(0) = %v, want alive", pid, err) + } + if err := os.WriteFile(proceed, nil, 0o600); err != nil { + t.Fatalf("creating go marker: %v", err) + } + + select { + case err := <-result: + if err != nil { + t.Errorf("run error = %v, want nil (the stub exited 0)", err) + } + case <-time.After(10 * time.Second): + t.Fatal("podman run did not return after the stub was released") + } + if _, err := os.Stat(doneMarker); err != nil { + t.Errorf("stub did not finish after caller cancellation: %v", err) + } +} + +func readPid(t *testing.T, path string) int { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading pid file: %v", err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err != nil { + t.Fatalf("parsing pid %q: %v", data, err) + } + return pid +} + +func TestPodmanRunTimeoutKillsHungProcess(t *testing.T) { + pidFile := filepath.Join(t.TempDir(), "child.pid") + prog := filepath.Join(t.TempDir(), "podman-stub.sh") + script := fmt.Sprintf("#!/bin/sh\necho $$ > %q\nexec sleep 30\n", pidFile) + if err := os.WriteFile(prog, []byte(script), 0o700); err != nil { + t.Fatalf("writing stub: %v", err) + } + cli := &podmanExec{program: prog, timeout: 100 * time.Millisecond} + result := make(chan error, 1) + go func() { result <- cli.run(t.Context(), []string{"run", "--detach", "image"}) }() + waitForPodmanMarker(t, pidFile) + data, err := os.ReadFile(pidFile) + if err != nil { + t.Fatalf("reading child pid: %v", err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(data))) + if err != nil { + t.Fatalf("parsing child pid %q: %v", data, err) + } + t.Cleanup(func() { + if err := syscall.Kill(pid, syscall.SIGKILL); err != nil && !errors.Is(err, syscall.ESRCH) { + t.Errorf("killing recorded podman process %d: %v", pid, err) + } + }) + + select { + case err := <-result: + if err == nil { + t.Fatal("run returned nil after the command timeout") + } + case <-time.After(5 * time.Second): + t.Fatal("podman run did not return after its command timeout") + } + if err := syscall.Kill(pid, 0); !errors.Is(err, syscall.ESRCH) { + t.Fatalf("after run timeout, kill(%d, 0) = %v, want ESRCH", pid, err) + } +} + +func waitForPodmanMarker(t *testing.T, path string) { + t.Helper() + timer := time.NewTimer(10 * time.Second) + defer timer.Stop() + ticker := time.NewTicker(5 * time.Millisecond) + defer ticker.Stop() + for { + if _, err := os.Stat(path); err == nil { + return + } else if !os.IsNotExist(err) { + t.Fatalf("checking marker %q: %v", path, err) + } + select { + case <-timer.C: + t.Fatalf("marker %q was not created before the deadline", path) + case <-ticker.C: + } + } +}