A job command outlives its own timeout whenever it leaves a grandchild holding the pipe #892

New issue

Closed

opened 2026-08-17 05:04:54 +00:00 by coilyco-ops · 0 comments

coilyco-ops commented

2026-08-17 05:04:54 +00:00

Member

Found by Angie (ENG, claude seat) while adding execution telemetry on #890. A test written to check the timeout still fired caught this instead.

A job command can run indefinitely past its deadline

WardCommandRunner.Run bounds the command with context.WithTimeout and exec.CommandContext. That kills the direct child on expiry. It does not close the pipe, and CombinedOutput blocks until every writer to that pipe is gone - including a grandchild that inherited it.

So a command whose shell forks outlives its own timeout for as long as the grandchild runs.

Measured, with the timeout set to 50 milliseconds:

--- FAIL: TestTheCommandTimeoutStillApplies (30.01s)
    the timeout took 30.012232209s to fire, so it does not bound wall clock

A 50ms deadline held the call for 30 seconds, which was exactly the child's sleep. The deadline bounded nothing.

Why this is not theoretical

ward exec <verb> runs builds and test suites. Those spawn children as their whole purpose, and a compiler or a test runner inheriting stdout is the normal case rather than the exotic one.

defaultCommandTimeout is 10 minutes. On this path that is the number a reader would quote and not the number that applies, and the real bound is however long the grandchild takes.

It also depends on the shell, which is why it was invisible: sh -c "sleep 10" where sh execs the sleep replaces the process and the kill works. Where sh forks, it does not. The same command bounds correctly on one host and not on another, and CI and my laptop disagreed about it - which is how it surfaced.

The fix

exec.Cmd.WaitDelay, which is what it exists for: after the context ends the command, it bounds how long Wait will keep waiting on I/O, then closes the pipes and returns.

command.WaitDelay = commandKillGrace

Same test after, with a 5 second grace:

--- PASS: TestTheCommandTimeoutStillApplies (5.05s)

SIRENS_ECHO_COMMAND_KILL_GRACE defaults to 5s. Shipped in the same change as #890's telemetry rather than separately, because the test that proves the telemetry is the test that found this, and leaving it asserting a property the code did not have would have been worse than either.

What this does not fix

The grandchild is not reaped. WaitDelay unblocks this service and closes the pipes; the orphan keeps running until it finishes on its own. Bounding that needs a process group and a group kill, which is a bigger change with its own failure modes and wants its own decision.

For the deadline that a job's own timeout is supposed to enforce, unblocking the harness is the part that matters, and an orphan on a workspace that gets removed anyway is a smaller problem than a job that never ends. Naming it rather than implying the whole thing is closed.

Related - #890 (where this was found), and the ward exec job kind it applies to.

Found by Angie (ENG, `claude` seat) while adding execution telemetry on #890. A test written to check the timeout still fired caught this instead. ## A job command can run indefinitely past its deadline `WardCommandRunner.Run` bounds the command with `context.WithTimeout` and `exec.CommandContext`. That kills the **direct child** on expiry. It does not close the pipe, and `CombinedOutput` blocks until every writer to that pipe is gone - including a **grandchild** that inherited it. So a command whose shell forks outlives its own timeout for as long as the grandchild runs. Measured, with the timeout set to 50 milliseconds: ``` --- FAIL: TestTheCommandTimeoutStillApplies (30.01s) the timeout took 30.012232209s to fire, so it does not bound wall clock ``` **A 50ms deadline held the call for 30 seconds**, which was exactly the child's sleep. The deadline bounded nothing. ## Why this is not theoretical `ward exec <verb>` runs builds and test suites. Those spawn children as their whole purpose, and a compiler or a test runner inheriting stdout is the normal case rather than the exotic one. `defaultCommandTimeout` is 10 minutes. On this path that is the number a reader would quote and not the number that applies, and the real bound is however long the grandchild takes. It also depends on the shell, which is why it was invisible: `sh -c "sleep 10"` where `sh` **execs** the sleep replaces the process and the kill works. Where `sh` **forks**, it does not. The same command bounds correctly on one host and not on another, and CI and my laptop disagreed about it - which is how it surfaced. ## The fix `exec.Cmd.WaitDelay`, which is what it exists for: after the context ends the command, it bounds how long `Wait` will keep waiting on I/O, then closes the pipes and returns. ``` command.WaitDelay = commandKillGrace ``` Same test after, with a 5 second grace: ``` --- PASS: TestTheCommandTimeoutStillApplies (5.05s) ``` `SIRENS_ECHO_COMMAND_KILL_GRACE` defaults to 5s. Shipped in the same change as #890's telemetry rather than separately, because the test that proves the telemetry is the test that found this, and leaving it asserting a property the code did not have would have been worse than either. ## What this does not fix **The grandchild is not reaped.** `WaitDelay` unblocks this service and closes the pipes; the orphan keeps running until it finishes on its own. Bounding that needs a process group and a group kill, which is a bigger change with its own failure modes and wants its own decision. For the deadline that a job's own timeout is supposed to enforce, unblocking the harness is the part that matters, and an orphan on a workspace that gets removed anyway is a smaller problem than a job that never ends. Naming it rather than implying the whole thing is closed. **Related** - #890 (where this was found), and the `ward exec` job kind it applies to.

coilyco-ops added the

labels

2026-08-17 05:05:17 +00:00

coilyco-ops referenced this issue from a commit

2026-08-17 05:08:06 +00:00

fix(execution): make the command timeout bound wall clock

coilyco-ops referenced this issue

2026-08-17 05:08:28 +00:00