Fix flaky Windows CI: widen post-exit wait bound for output draining - #4207
Fix flaky Windows CI: widen post-exit wait bound for output draining#4207lox wants to merge 1 commit into
Conversation
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found no code issues in this pass. This changes shared subprocess wait and exit-status semantics, including the deliberate false-success trade-off for unreadable Windows statuses, so it needs human review.
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 13142, 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.
1166849 to
4cce25f
Compare
lox
changed the title
There was a problem hiding this comment.
I found no code issues in this revision. This changes the shared non-PTY subprocess wait path used by hooks and jobs, so it needs human review. The current agent build is failing, but its job logs weren't available to me.
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 13145, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
PR #4141 added a 1s WaitDelay buffer after a process exits, after which os/exec force-closes the output pipes. On slow Windows CI workers, 1s is sometimes not enough to finish draining hook output that was already written, causing lifecycle hook tests (TestAgentStartupHook etc.) to flake with missing log lines. Widen the post-exit buffer to 30s (it only delays completion when a child process leaked the output pipe; a clean exit with closed pipes still completes immediately), and add a per-process WaitDelay override so the leaked-pipe tests can keep a short delay and stay fast. Amp-Thread-ID: https://ampcode.com/threads/T-019fe0a2-982c-74fa-830c-5aa0c249369e Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
4cce25f to
d10bf0c
Compare
There was a problem hiding this comment.
I found no code issues in this revision. It changes the shared non-PTY subprocess wait path used by hooks and jobs, so it still warrants human review. Focused process/shell tests and vet passed locally; the agent CI build is still running.
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 13150, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
3 participants
What
Widens the post-exit
WaitDelaybuffer from 1s to 30s, and adds a per-processWaitDelayoverride so tests that intentionally leak the output pipe can keep a short delay and stay fast.Why
Windows AMD64 CI on
mainhas been flaking on lifecycle hook tests (TestAgentStartupHook,TestAgentShutdownHook,TestBuildCancel) with missing log output, e.g. an expected[info] hello worldline never appearing.#4141 introduced a 1s
Cmd.WaitDelaybuffer that starts once the process exits. When it expires,os/execforce-closes the stdout/stderr pipes — discarding output that was written but not yet drained. On slow Windows CI workers, 1s is sometimes not enough, so hook output gets truncated and the assertions fail.This is reproducible deterministically: shrinking the buffer to 1ms on Linux produces exactly the missing-output diffs seen in CI.
How
waitDelayBuffergoes from 1s to 30s. This buffer only delays completion when a child process has leaked a copy of the output pipe's write end (the case Bound Cmd.Wait so a leaked stdout/stderr pipe can't hang hooks #4141 addressed); a clean exit with closed pipes still completes immediately, so normal jobs see no slowdown.process.Config.WaitDelayallows overriding the buffer per process, with test-only plumbing ininternal/shellso the leaked-pipe tests run with ~2s instead of 30s.Note:
mainalso shows a separate, rarer Windows flake (GetExitCodeProcess: The handle is invalid) that first appeared with the Go 1.26.5 bump (#4134) and matches theCmd.Waitraces in golang/go#78046. That is not addressed here — it's handled by downgrading Go until a released Go contains the upstream fixes (see companion PR).