Skip to content

fix(docs): bound every harness command so a stuck wheels can't hang the run - #4447

Merged
bpamiri merged 2 commits into
developfrom
peter/docs-harness-exec-timeout
Oct 6, 2026
Merged

bpamiri merged 2 commits into
developfrom
peter/docs-harness-exec-timeout

Conversation

@bpamiri

@bpamiri bpamiri commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The docs-harness test "verify-docs reports pass/fail counts" normally takes about 5–11 s. On one CI run (37385398443) it hung:

  • tests 1–50 finished, then nothing was logged until the test's 300 s timeout;
  • the tutorial tests after it hung too, until the job was cancelled 13 minutes later;
  • job cleanup killed an orphaned java process.

Cause (from the code; the hung run's log is silent about which command stuck). lib/exec.mjs runExec(), which runs every wheels call the harness makes:

  • had no default timeout;
  • resolved only on the child's close event, which waits for every holder of its stdout/stderr pipes.

So a wheels call that stalls, or that leaves a process holding its pipes, keeps the harness waiting forever. The first wheels call in that test is resetFixture()'s wheels new.

Fix, in runExec():

  • Default timeout: 240 s, override with WHEELS_EXEC_TIMEOUT_MS (defaultExecTimeout()). It's below the tests' 300 s limit, so a stuck command fails with its own name rather than as an anonymous test timeout. On timeout it kills the command's whole process group (the child is spawned detached, so it leads one; a JVM it started goes too) and returns { code: -1, timedOut: true } with timed out after Ns running <command> in stderr.
  • Exit grace: it returns 2 s after the command exits, even if a leftover process still holds the pipes. That leftover keeps running while the harness does, because a {test:cli} block may legitimately leave a server running (wheels start). Its group is kept in a leftovers set and stopped when the harness exits, since its isolated home is deleted then and a detached group doesn't get the terminal's Ctrl-C.
  • Cleanup on exit: commands still running when the harness process exits are stopped. A detached child no longer receives a terminal Ctrl-C, and this keeps an interrupted run from leaving wheels processes behind. It pairs with the SIGINT/SIGTERM handlers in isolated-home.mjs.

The timeout option callers could already pass now goes through this path instead of spawn()'s own timeout, which signalled only the child.

Tests

New in test/exec.test.mjs, using small node programs in place of wheels:

  • a command that never exits returns in under a second with the named timeout;
  • a command that exits while a grandchild still holds its pipes returns promptly;
  • a timeout also kills that grandchild (its pid is gone);
  • a leftover from a command that exited is stopped when the harness process exits (added in review; it failed before the leftovers fix);
  • WHEELS_EXEC_TIMEOUT_MS sets the default, and an invalid value falls back to 240 s.

RED on develop: all 4 fail. The run itself hung with the old runExec() and had to be stopped, which is the bug.

GREEN: all of exec.test.mjs, 7 / 7. The full pnpm test:docs-harness gives 71 pass, 1 fail. The failure is "tutorial driver walks mini-tutorial end to end" (Reload requires this project's own server), which is #4426, fixed by #4444 (not yet on develop).

🤖 Generated with Claude Code

…he run

runExec() had no default timeout and resolved only when the command's output
pipes closed, which a leftover process can hold open forever: one stuck wheels
call waited out a test's 300 s limit and then hung the verify job.

- A default timeout (240 s, WHEELS_EXEC_TIMEOUT_MS) kills the command's whole
  process group and fails with 'timed out after Ns running <command>'.
- It returns 2 s after the command exits even if a leftover process still holds
  the pipes (not killed: a block may leave a server running).
- Commands still running when the harness exits are stopped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@bpamiri bpamiri added this to the v4.2.0 milestone Oct 5, 2026
@bpamiri

bpamiri commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

rev1-r3 review @ 5b6d5cb: CHANGES (one gap in the leftover-process cleanup)

The timeout path is sound:

  • The default of 240 s sits under the tests' 300 s limit, so a stuck command fails with its own name.
  • detached gives each command its own process group, so process.kill(-pid, SIGKILL) takes down the JVM and anything else the command started.
  • It falls back to the child alone on Windows or when there is no group.
  • The settled guard makes finish() idempotent across timeout, exit+grace, close and error.
  • Exit-plus-2 s-grace fixes the hang where a leftover holds the pipes open. That's a likely mechanism for docs(cli): describe deploy build, logs and prune as they now work #4441's test-51 timeout.

Needs a fix: what happens to the leftover

  • On the exit-grace path, finish() calls running.delete(proc), so the process.on('exit') cleanup never sees that command's process group. And because the group is now detached, a Ctrl-C at the terminal doesn't reach it either. The result:

    • a {test:cli} wheels start server, or any other leftover holding the pipes, now outlives the harness;
    • it keeps running against the throwaway CLI home that isolated-home.mjs deletes on exit.

    Before this PR the child stayed in the terminal's foreground group, so Ctrl-C at least reached it.

    Keeping the leftover alive while the run continues is right, because later blocks may need the server. It just shouldn't survive the harness. Suggested change:

    • on the grace path, move the proc into a leftovers set instead of dropping it;
    • have the exit handler killGroup(…, 'SIGKILL') over both running and leftovers. A process that already exited cleanly throws ESRCH, which is already caught.

    Add one test: a child that starts a long-lived grandchild and exits, then check the grandchild is gone after the harness process exits. For example, spawn a small node wrapper that imports runExec, runs the leftover-producing program, exits, and the test then checks the grandchild's pid with process.kill(pid, 0).

Minor, not blocking: TutorialSession.stopServer() already stops the tutorial's own server explicitly, so this mainly matters for servers that {test:cli} blocks start and for interrupted runs.

Gate: PR CI (18 pending). I'll re-review the fix promptly.

A command that exited while something it started still held its pipes was
dropped from the harness-exit cleanup, so a server a block started outlived
the harness (and its isolated home). Those groups are now kept and stopped
when the harness exits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@bpamiri

bpamiri commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

rev1-r3 review @ 941f018: code APPROVE (final pending PR CI)

  • Commands that finish on the grace path now go into a leftovers set, and the exit handler calls killGroup(…, 'SIGKILL') over both running and leftovers. A server that a block started keeps running for the rest of the harness run and is stopped when the harness exits. The process groups of commands that finished cleanly never reach the set, because close settles first.
  • The new test runs a real "harness" process that calls runExec. The command it runs exits but leaves a grandchild running, then the harness exits, and the test checks the grandchild pid is dead within 5 s. The finally cleans up even if the test fails.

One limitation, not blocking: this reaches processes that stay in the command's process group. A server that calls setsid / starts its own session to daemonize would escape the group kill. If a {test:cli} wheels start still leaks after this, that's where to look, and the tutorial driver's explicit stopServer() remains the backstop for its own server.

Gate: PR CI. I'll post the final when it's green.

@bpamiri

bpamiri commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

rev1-r3 review @ 941f018: APPROVE (final)

PR CI is now 20 pass / 0 fail / 0 pending, with 3 skipping. In verify, the harness step ran 73 tests: 72 pass, 1 fail, 0 cancelled, and it finished: it didn't hang and get cancelled the way the #4441 and #4393 runs did. The 7 exec.test.mjs tests pass, including the leftover-cleanup test. The one failure is tutorial driver walks mini-tutorial end to end, the known wheels reload failure that #4444 fixes. That step is still continue-on-error on this branch, which predates #4444.

Ready to merge. Once both this and #4444 are on develop, the harness step should be fully green and blocking.

@bpamiri
bpamiri merged commit 50a9cc5 into develop Oct 6, 2026
26 checks passed
@bpamiri
bpamiri deleted the peter/docs-harness-exec-timeout branch October 6, 2026 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant