Repository navigation
fix(cli): a check that is stopped by a signal prints the report it has so far - #6282
Conversation
…s so far Killed by SIGTERM or SIGINT, check closed Chrome and exited 143 or 130 with nothing on stdout, so a caller that stops a long check got no result at all. The signal now ends the check the way an expired deadline does: findings already made are kept, one finding says the check stopped before finishing, and the report is printed before the process exits with the signal's code.
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: APPROVE · 0 blocker · 1 important · 2 nits
The stop path works: SIGTERM/SIGINT mid-check prints the partial report with the stopped finding, then exits 143/130. The one gap is an unhandled rejection when the signal lands before the first deadline.run() (see inline); it only affects telemetry, not the exit code or the report.
- nit
checkPipeline.ts:1769(withFindingCrops, outside the diff): with--snapshots, a stopped check's crop step rejects withCheckStoppedError. That error is not aCheckDeadlineError, so it falls through tofinding crops skippedon stderr andtrackCommandFailure("check-finding-crops"). A user's Ctrl-C then counts as a crop failure in the rollout metric that comment describes. HandleCheckStoppedErrornext toCheckDeadlineError.
Scope
Read all 8 files at a906f57, plus launch.ts closeManagedBrowsers/registerBrowserSessionCleanup, the cli.ts exit and unhandledRejection handlers, and the runCheckPipeline stop path. checkDeadline, check, and launch tests pass locally (138). I confirmed the inline finding with a scratch test (not pushed). No prior reviews. I did not pull CI (the state is BLOCKED, read here as the review gate).
| this.abort.abort(error); | ||
| } | ||
|
|
||
| stop = (signal: string): void => { |
There was a problem hiding this comment.
important — A stop that lands before the first run() rejects this.expiry while nothing is listening to it. Example: SIGTERM during await prelaunched in runCheckPipeline, which is common when lint finishes before the prelaunched Chrome is up. After that, run() returns Promise.reject(this.expired) and never attaches to expiry, so Node raises unhandledRejection. I reproduced it with a scratch test: prelaunchBrowser resolves after 200 ms and SIGTERM is sent at 50 ms. The report still prints, but the handler in cli.ts:490 sets commandFailed = true and emits an unhandled_rejection CLI error event, so every early Ctrl-C shows up as a crash in telemetry. Before this PR the idle timer only armed after start(), inside run(), so the gap is new. Fix: in the constructor, mark the promise handled (this.expiry.catch(() => undefined);), and add a test that stops before run().
| const args = parseArgs(rawArgs, CHECK_COMMAND_ARGS); | ||
| const asJson = args.json === true; | ||
|
|
||
| const reported = untilReported(); |
There was a problem hiding this comment.
nit — The PR body says a signal during lint "still exits immediately with no report, as before". That is no longer true. This cleanup is registered before lint, so shutdown now waits up to 5 s for the print. If lint finishes inside that window, the pipeline goes on, launchManagedBrowser throws "The CLI is stopping; no browser can start.", and the report prints with that message as a check_runtime_failure instead of the stopped finding. Both outcomes are bounded and harmless. Please correct the body, or register the stop listener before lint so this case also gets the stopped finding.
Edit accuracy: accurate 271 (base branch 271), smooth 185 of thoseThe gate passes. Quarantined, measured but not gated (0) |
What
A
checkthat is stopped by SIGTERM or SIGINT now prints the report it has so far, including one finding that says it stopped before finishing, and then exits 143 or 130 as before.Why
Stopped by a signal,
checkclosed Chrome and exited 143 or 130 with nothing on stdout. A caller that stops a long check (a package-script runner, a CI step with its own time limit) got no result at all, not even the findings the check had already made.How
browser/launch.tsalready owns SIGINT and SIGTERM. It gainsonStopSignal(listener), called when a stop signal arrives and before the browsers close, so a command is told while its page is still there.CheckDeadline.stop(signal)expires the check at once, the way an idle deadline does. The existing path then builds the report from the findings kept so far and adds the finding "Check stopped before finishing (SIGTERM)." No second reporting path.checkregisters a shutdown step that waits for the report to be printed (up to 5 s) before the process exits, so the exit cannot cut the report off.loadCheckHtmlis renamedloadCheckHtmlWithDeferredMedia, so the name says what the comment did.Not covered: a signal that arrives before
checkhas started its deadline (while linting) still exits immediately with no report, as before.Test plan
On a 4-core Linux box,
check --jsonon a 500 s project, killed 8 s in:CheckDeadline.stopends a pending call at once and keeps the partial results; a command-level test sends SIGTERM mid-check and asserts the report is printed before exit 143.checkDeadline,checkandlaunchtests (9, 127 and 2 tests).