test(ci): bound dispatch fixture subprocess lifetime - #219
Conversation
Replace unbounded synchronous fixture execution with async process-group supervision, an explicit deadline, bounded output, and close-before-cleanup. Cover descendants retaining pipes after a successful shell exit. The full serial suite passes; preserve separately recorded local parallel-run timeouts.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P3
|
Codex review: needs maintainer review before merge. Reviewed September 3, 2026, 1:26 PM ET / 17:26 UTC. ClawSweeper reviewWhat this changesThe PR replaces an unbounded synchronous Bash test fixture with an asynchronous POSIX process-group supervisor that limits runtime and captured output, with regressions for inherited-pipe descendants and output overflow. Merge readiness✅ Ready for maintainer review Keep open: current main still uses the unbounded synchronous fixture, while this exact head adds a narrowly scoped supervisor with sufficient real process-tree proof. The prior current-main refresh is incorporated; no introduced correctness or security defect was found. Priority: P3 Review scores
Verification
How this fits togetherThe dispatch-workflow test runs the CI dispatch shell fragment against a stubbed GitHub client and verifies the resulting payload. Its fixture creates a shell process and temporary files; the new supervisor bounds that test-only process tree before cleanup. flowchart LR
A[Workflow shell fragment] --> B[Dispatch test fixture]
B --> C[Shell process group]
C --> D[Shell and descendants]
D --> E[Bounded output capture]
B --> F[Deadline or size limit]
F --> G[Process-group termination]
E --> H[Dispatch payload assertions]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the private POSIX fixture supervisor so descendant-held pipes become a bounded diagnostic failure, while retaining the existing Windows skip and unchanged CI dispatch behavior. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main runs the fixture synchronously without a deadline, and the supplied real Bash/Node capture demonstrates the descendant-held-pipe condition the new supervisor bounds. Is this the best way to solve the issue? Yes: keeping process-group termination, the timeout, and output cap local to this fixture fixes the hang without changing the workflow or production package. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9edca2ca1e17. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
Resolve the changelog insertion while preserving both entries. The supervisor is unchanged from its reviewed runtime capture. Keep the separately recorded local fixture timeout as a failure; refreshed hosted CI remains the landing gate.
clawsweeper
Bot
added
proof: sufficient
Bring the unchanged reviewed supervisor onto current main. Preserve local timing failures and rely on fresh hosted validation before landing; no workflow or production behavior is changed by this PR.
|
Landed as 8739325; merged tree Reexecuted The runtime capture in the PR body exercised the unchanged committed helper with real macOS arm64 / Node 24.20.0 processes, actual process-group signals, and the real ten-second timer. The shell-exited/live-descendant case timed out at 10,005 ms, observed closure, verified the descendant gone, and removed scratch. The overflow case terminated at 57 ms with 983,040 captured bytes, below the 1 MiB limit. Windows retains the existing skip for this POSIX behavior fixture. The standalone full serial suite passed 7,082 / 80 skipped, and build/docs/boundary/package checks passed. Local parallel attempts and a later combined-base serial attempt did not all pass: the latter recorded 7,107 passed / 1 fixture timeout / 80 skipped. Those failures remain in the campaign evidence. This repair bounds and cleans up stalled fixtures; it does not claim to eliminate or conclusively identify the underlying intermittent local Bash stall. After an initial quota error, the existing GitHub cache returned the exact merged-main results: ci, coverage, and CodeQL all completed successfully at No production code, workflow, dependency, global test timeout, release, or registry publication changed. |
1 participant
Summary
Bound the POSIX workflow-dispatch test fixture's subprocess lifetime without changing the workflow or production code.
The fixture used
spawnSync("bash", ...)with no subprocess deadline. During local validation it stalled inside Node's synchronous process runner while Bash descendants waited, so Vitest's own timer could not interrupt it. That attempt required stopping the task-owned child and is preserved as a failed run. The underlying shell scheduling cause has not been conclusively identified.The fixture now uses asynchronous
spawnin a dedicated POSIX process group, a 10-second deadline, and a 1 MiB output bound. On failure it signals its owned process group once and awaitsclosebefore temporary-directory cleanup. The deadline stays active after the shell'sexitevent because a descendant can keep the pipes open. A timeout cannot be treated as success merely because the shell exited zero.The behavioral test's outer budget covers its two sequential bounded fixture invocations plus cleanup margin; no repository-wide test timeout, production timeout, workflow timeout, or workload was changed. Windows retains the existing skip for the POSIX behavioral fixture.
Regression proof
SIGTERM. The test waits for readiness and the parent's exit, invokes the registered deadline callback, and verifies timeout failure plus closure after process-groupSIGKILL. A real timer remains as a cleanup backstop.The subprocess supervisor and tests are local to this test file; no new exported helper or dependency is introduced.
Validation and limitations
CI=1 pnpm test --maxWorkers=1— 201 suites passed / 2 skipped; 7,082 tests passed / 80 skipped.pnpm pack:check, andgit diff --checkpassed.Default parallel full checks did not pass locally and are not claimed as passing: separate runs hit the fixture deadline, a pre-existing sidecar process-exit test deadline, and varying archive test deadlines. The serial verification changed only test-file concurrency, not test bodies, internal stress concurrency, or product deadlines. All failed attempts remain in the campaign evidence. Initial exact-head hosted CI and first updated-base CI passed the normal repository configuration. The changelog conflicts are resolved. Current head
4178ed22d60f40bfc453cb80bb423d1826817ab0is based on merged main9edca2ca1e1708e9536587b221de739b439029e1; final exact-head CI is running. The supervisor itself is byte-identical to the captured runtime helper.A later local serial run on the combined base also reached this fixture's deadline (7,107 passed / 1 timeout / 80 skipped), and a paired focused run alternated between passing and timing out. Those failures are retained too. This patch bounds and cleans up such failures; it is not claimed to eliminate or conclusively explain the underlying intermittent local shell stall.
Inspectable runtime capture
After-fix execution used the exact
startFixturehelper from committed head408598672aa2936605c6aefcdfa93eced4063abb, extracted without semantic changes and stripped of TypeScript annotations by Node. Helper SHA-256:d6ecf91cbb24bb52a6bed8a8e2ff1d3e66e6454b811b4c0ed0e8a00221449ae2. Environment: physical macOS arm64, Node 24.20.0. Actual Bash/Node processes, the real 10,000ms deadline, and actual process-group signals were used—no mocked timer or signal.Captured terminal results (only synthetic fixture state):
{"case":"normal","status":0,"signal":null,"timedOut":false,"error":null,"elapsedMs":17,"closeObserved":true,"stdoutBytes":11,"descendantGone":null,"scratchRemoved":true} {"case":"exited-shell-live-descendant","status":0,"signal":null,"timedOut":true,"error":null,"elapsedMs":10005,"closeObserved":true,"stdoutBytes":27,"descendantGone":true,"scratchRemoved":true} {"case":"output-overflow","status":null,"signal":"SIGKILL","timedOut":false,"error":"dispatch fixture output exceeded 1 MiB","elapsedMs":57,"closeObserved":true,"stdoutBytes":983040,"descendantGone":true,"scratchRemoved":true}The second case confirms that a successful Bash exit is not mistaken for fixture completion while a live descendant holds the pipes. Its actual deadline fired at approximately ten seconds, closure was observed, the descendant was verified gone, and only then was scratch removed. The overflow case terminated before the deadline, retained less than 1 MiB, and likewise verified closure, descendant termination, and cleanup.
This is a separate validation repair discovered while verifying #217 and #218. Their lock implementations and frozen Windows proof candidate were left unchanged.