From d10bf0cd0dd79da66673e9b0a82378a72e05e3ab Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 8 Aug 2026 10:00:40 +0000 Subject: [PATCH] Fix flaky Windows CI: widen post-exit wait bound for output draining 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 --- internal/process/process.go | 19 ++++++++++++++++++- internal/process/process_test.go | 2 ++ internal/shell/export_test.go | 6 ++++++ internal/shell/shell.go | 6 ++++++ internal/shell/shell_test.go | 2 ++ 5 files changed, 34 insertions(+), 1 deletion(-) diff --git a/internal/process/process.go b/internal/process/process.go index 44827071be..4d44aaee29 100644 --- a/internal/process/process.go +++ b/internal/process/process.go @@ -29,7 +29,15 @@ const termType = "xterm-256color" // fires before os/exec's WaitDelay-triggered kill, keeping cancellation // behaviour unchanged, while still bounding the post-exit I/O wait so a // leaked stdout/stderr pipe can never hang Cmd.Wait indefinitely. -const waitDelayBuffer = 1 * time.Second +// +// The bound is deliberately generous: when it expires, os/exec force-closes +// the output pipes and any not-yet-drained output is silently lost (Wait +// returns ErrWaitDelay, which complete() treats as a clean exit). A 1s buffer +// proved too tight on loaded Windows CI machines, where the post-exit drain of +// fast hooks intermittently exceeded it and hook output went missing. Hitting +// this bound should only ever mean a genuinely leaked pipe write-end, not a +// slow-but-healthy drain. +const waitDelayBuffer = 30 * time.Second // afterPTYStartHook lets tests force work to happen after the PTY helper // returns so they can verify raw-mode ordering around process startup. @@ -99,6 +107,12 @@ type Config struct { SignalGracePeriod time.Duration Started chan struct{} Done chan struct{} + + // WaitDelay, when positive, overrides the derived Cmd.WaitDelay bound + // (SignalGracePeriod + waitDelayBuffer) for the non-PTY path. Production + // code leaves this zero; tests use it to keep leaked-pipe regression + // tests fast. + WaitDelay time.Duration } // Process is an operating system level process @@ -315,6 +329,9 @@ func (p *Process) startWithoutPTY(context.Context) (func(), error) { // cancellation; this keeps cancellation behaviour identical to before while // still bounding the post-exit I/O wait for the leaked-pipe case. func (p *Process) waitDelay() time.Duration { + if p.conf.WaitDelay > 0 { + return p.conf.WaitDelay + } return max(p.conf.SignalGracePeriod, 0) + waitDelayBuffer } diff --git a/internal/process/process_test.go b/internal/process/process_test.go index bf669643ee..c00e8b3314 100644 --- a/internal/process/process_test.go +++ b/internal/process/process_test.go @@ -460,6 +460,8 @@ func TestProcessRunDoesNotHangWhenChildLeaksStdout(t *testing.T) { Env: []string{"TEST_MAIN=leak-stdout"}, Stdout: w, Stderr: w, + // Shorten the post-exit I/O bound (default 30s) so the test stays fast. + WaitDelay: time.Second, }) // Ensure the leaked grandchild is cleaned up regardless of outcome. diff --git a/internal/shell/export_test.go b/internal/shell/export_test.go index 842e921af4..cca03a5c38 100644 --- a/internal/shell/export_test.go +++ b/internal/shell/export_test.go @@ -5,3 +5,9 @@ import "time" func Round(d time.Duration) time.Duration { return round(d) } + +// WithProcessWaitDelay is a test-only shell option that overrides the derived +// process.Config.WaitDelay bound, keeping leaked-pipe regression tests fast. +func WithProcessWaitDelay(d time.Duration) NewShellOpt { + return func(s *Shell) { s.processWaitDelay = d } +} diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 39cc5fed49..a449a63ba6 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -68,6 +68,10 @@ type Shell struct { // Amount of time to wait between sending the InterruptSignal and SIGKILL signalGracePeriod time.Duration + // Test-only override for the derived process.Config.WaitDelay bound + // (see WithProcessWaitDelay in export_test.go). Zero means "use default". + processWaitDelay time.Duration + // stdin is an optional input stream used by Run() and friends. // It remains unexported on the assumption that it's not useful except via // CloneWithStdin to get a clone prepared for a single command that needs @@ -151,6 +155,7 @@ func (s *Shell) CloneWithStdin(r io.Reader) *Shell { wd: s.wd, interruptSignal: s.interruptSignal, signalGracePeriod: s.signalGracePeriod, + processWaitDelay: s.processWaitDelay, } } @@ -576,6 +581,7 @@ func (s *Shell) buildCommand(name string, arg ...string) (process.Config, error) Dir: s.wd, InterruptSignal: s.interruptSignal, SignalGracePeriod: s.signalGracePeriod, + WaitDelay: s.processWaitDelay, }, nil } diff --git a/internal/shell/shell_test.go b/internal/shell/shell_test.go index 05bec456c8..e4befe1260 100644 --- a/internal/shell/shell_test.go +++ b/internal/shell/shell_test.go @@ -757,6 +757,8 @@ func TestRunDoesNotReportCleanHookAsFailedWhenChildLeaksStdout(t *testing.T) { sh, err := shell.New( shell.WithStdout(w), shell.WithLogger(shell.DiscardLogger), + // Shorten the post-exit I/O bound (default 30s) so the test stays fast. + shell.WithProcessWaitDelay(time.Second), ) if err != nil { t.Fatalf("shell.New() error = %v", err)