Bound Cmd.Wait so a leaked stdout/stderr pipe can't hang hooks - #4141
Conversation
When a Process's Stdout/Stderr is an io.Writer that is not an *os.File (the agent pipes hook output through an io.Pipe), os/exec allocates an internal OS pipe and a background copy goroutine. Cmd.Wait cannot return until that goroutine sees EOF, which requires every copy of the pipe write-end fd to be closed. If a hook backgrounds a child, or a concurrently-spawned sibling races on the fd, and it inherits the write-end, the pipe never EOFs and Cmd.Wait blocks forever. In CI this surfaced as TestAgentShutdownHook hanging for 10 minutes then panicking with "test timed out". Set Cmd.WaitDelay on the non-PTY path so the post-exit I/O wait is bounded. WaitDelay only starts counting after the process exits, so normal fast or briefly-streaming hooks are unaffected. It is derived from the signal grace period plus a buffer so the agent's existing interrupt-then-group-SIGKILL sequence always wins on context cancellation, leaving cancellation behaviour unchanged. When WaitDelay elapses after an otherwise-clean exit, Wait returns exec.ErrWaitDelay; complete() now treats that as a successful exit rather than a hook failure. Add a deterministic regression test that drives Process.Run with an *io.PipeWriter stdout and a child that backgrounds a grandchild which inherits stdout. The test hangs (times out) without this fix and returns within the bound with it.
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
The hang is bounded, but the clean-exit case is still returned to lifecycle hooks as a failure; I've left one blocking comment on that path.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9948, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
…reported as failures When a hook exits cleanly but a backgrounded grandchild inherits and holds open the output pipe's write-end, os/exec returns exec.ErrWaitDelay once WaitDelay elapses. complete() already treats this as a clean exit and makes Process.Run return nil, but it still stored the raw sentinel in p.waitResult. shell.executeCommand returns Process.WaitResult() to lifecycle hooks whenever Run returned nil, so the sentinel leaked through and a clean hook was still reported/logged as a failure. Clear p.waitResult in the ErrWaitDelay branch so WaitResult() agrees with Run(). Add a shell-level regression test that drives a leaking command through Command.Run (the path lifecycle hooks use) and asserts the returned error is nil; it fails without this change and passes with it. waitDelayBuffer becomes an exported var so the test can shorten the post-exit wait and stay fast; production behaviour is unchanged.
|
Good catch — cleared the Generated by Claude Code |
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous concern is addressed, and I found no new issues. Since this changes shared subprocess wait behavior, I’m leaving this as a comment for human review rather than approving.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9958, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
There was a problem hiding this comment.
I found no new issues after the latest adjustment. This changes shared subprocess wait behavior, so I’m leaving it for human review rather than approving.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9991, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
The shell test override was the only cross-package reference to the exported WaitDelayBuffer. With the default buffer now 1s, shortening it to 500ms in the test is not worth the exported global, so drop the override (the test uses the default) and revert WaitDelayBuffer back to an unexported const waitDelayBuffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XZxHFYHDMB2gJAhnsGBd6x
There was a problem hiding this comment.
I found no new issues after the latest adjustment. This changes shared subprocess wait behavior, so I’m leaving it for human review rather than approving.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9998, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
2 participants
Requested by Dan Niknam, Josh · Slack thread
Description
Fixes an intermittent hang in agent lifecycle hooks (seen in CI as
TestAgentShutdownHook/with_registered_agents_envhanging for 10 minutes, thenpanic: test timed out).Before: the agent pipes hook output through an
io.Pipe, so a hook'sStdout/Stderris an*io.PipeWriterrather than an*os.File. Because of that,os/execallocates an internal OS pipe and a background copy goroutine, andCmd.Waitcannot return until that goroutine sees EOF — which requires every copy of the pipe's write-end fd to be closed. If a hook backgrounds a subprocess (e.g.sleep 30 &), or a concurrently-spawned sibling races on the fd, and that process inherits the write-end, the pipe never EOFs.Cmd.Waitthen blocks indefinitely, the deferred pipe-writer close never runs, the output scanner idles, and the agent/test hangs until a 10-minute timeout.After: the post-exit I/O wait is bounded, so a held-open stdout/stderr pipe can no longer hang
Cmd.Wait. A clean process exit whose only remaining wrinkle is the leaked pipe is treated as success and is not misreported as a hook failure.Context
This is one of a cluster of process/hook flakes that appeared with the Go 1.25 → 1.26 upgrade. The Windows invalid-handle flake and the Linux
fork/exec: bad file descriptorflake are a related but distinct spawn/wait race that is not addressed here (they could not be reproduced) and are tracked separately.Root cause was confirmed from a CI goroutine dump: one goroutine stuck in
bufio.Scanner.Scanatclicommand/agent_start.go, another stuck inos/exec.(*Cmd).Wait→awaitGoroutines, called frominternal/process/process.go.Changes
Approach chosen:
Cmd.WaitDelayon the non-PTY path plusErrWaitDelayhandling (the minimal, lower-risk fix), rather than rewiring theinternal/process/internal/shellplumbing to hand the child realos.Pipe()*os.Fileends.WaitDelaybounds only the wait after the process has exited, so normal fast or briefly-streaming hooks are unaffected, and it fully fixes the reproduction.internal/process/process.gostartWithoutPTYnow setsp.command.WaitDelaybeforeStart.waitDelay()/waitDelayBuffer). This guarantees the agent's own interrupt-then-group-SIGKILL sequence (onContextCancel) always fires beforeos/exec'sWaitDelay-triggered kill of the primary process, so context-cancellation / grace-period behaviour is unchanged; only the post-exit leaked-pipe wait is newly bounded.complete()now inspects the wait result: an*exec.ExitErroris handled as before;exec.ErrWaitDelayon an otherwise-clean exit is logged and treated as a successful (exit 0) completion rather than surfaced as a failure.internal/process/process_test.go+internal/process/main_test.goTestProcessRunDoesNotHangWhenChildLeaksStdout: drivesProcess.Runwith an*io.PipeWriterstdout and a child that backgrounds a grandchild which inherits stdout and lingers. The test hangs / times out without this fix and returns within a bounded time with it. Skipped on Windows (the fd-inheritance mechanism is POSIX-specific); the leaked grandchild is cleaned up viat.Cleanup.Testing
go test). Verified the new test fails (hangs to a 30s bound) onorigin/mainand passes (returns in ~10s) with the fix.go build ./...,go veton touched packages,go test ./internal/process/... -count=3, andgo test ./clicommand/ -run 'TestAgentShutdownHook|TestAgentStartupHook' -count=5all pass.gofmtclean).Disclosures / Credits
Written with Claude Code: it reproduced the hang, implemented the
WaitDelayfix andErrWaitDelayhandling, and added the regression test, under human direction and review.Generated by Claude Code