Make a signalled shutdown report the signal, not a crash - #152
Merged
Conversation
A worker killed with SIGTERM is supposed to exit 143, and on Node 20 it exits 1 with a stack trace on stderr. SIGHUP and SIGINT do the same; they were simply unasserted. Both runtimes are inside the declared engines range, and this reproduces on macOS as readily as on Linux — it has been invisible only because every local run and both Windows workflows use Node 22. The terminal shutdown a signal starts kills every in-flight system renderer child on purpose, and each kill rejects the render its caller was awaiting. Nothing is left to catch those rejections, because the caller is exactly what the shutdown is dismantling, so Node applies its default policy and makes them fatal. On Node 20 that fatal path runs before the deliberate process.exit; on Node 22 the exit wins. Neither ordering is promised on either runtime, so the contract was holding by luck on both. Node delivers this as uncaughtException with origin "unhandledRejection", including the entry module's own rejected top-level await, which never reaches an "unhandledRejection" listener at all. An unhandledRejection handler is therefore a no-op here, which is worth recording because it is the obvious fix and it does nothing. Two halves, each load-bearing. runSystemCommand's abnormal-close branch now rejects with the typed shutdown limit when renderer shutdown has begun, rather than inventing a renderer failure for a child we killed ourselves; a child that failed on its own, or timed out, or overflowed, still rejects exactly as before. The signal handler then records the exit code up front and installs a guard that swallows only faults carrying that typed shutdown reason. Anything else — a cleanup that could not be proven, or a bug — is re-raised on Node's default path and stands the deliberate exit down, so it still crashes with exit 1 and Node's own report instead of being buried under a tidy 143. The first half is what makes the second narrow enough to be safe: a blanket suppressor would fix the exit code and hide real faults. Not settling the promise at all during shutdown was considered and is wrong, not merely risky: withPrivateSystemRenderWorkspace deregisters only when the operation settles and beginPdfjsWorkerSystemShutdown awaits that cleanup, so a never-settling render deadlocks the shutdown it is part of. The test covers all three signals, reads the exit status and the host's real stderr off the process rather than restating either, and requires stderr to be empty. With the product reverted it fails three ways on Node 20 and passes on Node 22, which is the whole point: only the runtime that loses the race can see it. Mirrored into the share runtime. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by the Node matrix in #150 on its first real run.
The defect
A worker killed with SIGTERM is supposed to exit 143. On Node 20 it exits 1, with a stack trace on stderr. SIGHUP and SIGINT do the same — they were simply unasserted.
Both runtimes are inside the declared
^20.19.0 || >=22.12.0range, and this reproduces on macOS as readily as on Linux. It has been invisible only because every local run and both Windows workflows use Node 22.Two user-visible consequences: a supervisor distinguishing "terminated by request" from "crashed" misclassifies an orderly shutdown, and the log reads like a crash too.
Cause
The terminal shutdown a signal starts kills every in-flight renderer child on purpose, and each kill rejects the render its caller was awaiting. Nothing is left to catch those rejections — the caller is exactly what the shutdown is dismantling — so Node makes them fatal. On Node 20 that fatal path runs before the deliberate
process.exit; on Node 22 the exit wins.Neither ordering is promised on either runtime. The contract was holding by luck on both.
One detail worth recording: Node delivers this as
uncaughtExceptionwithorigin === "unhandledRejection", including the entry module's own rejected top-level await, which never reaches anunhandledRejectionlistener at all. Adding one — the obvious fix — is a no-op. That was written first and watched to not work.The fix, two halves, each load-bearing
runSystemCommand's abnormal-close branch rejects with the existing typed shutdown limit when renderer shutdown has begun, rather than inventing a renderer failure for a child we killed ourselves. Nested insidecode !== 0 || signal !== nulland placed after the timeout/overflow branches, so a clean finish still resolves and those still win. A genuine non-shutdown failure rejects byte-identically to today.Half 1 is what makes half 2 narrow enough to be safe. Measured: half 1 alone does not fix it (a typed rejection is still a fatal rejection); a blanket suppressor fixes the exit code and hides real faults.
Never settling the promise during shutdown was considered and is wrong, not merely risky:
withPrivateSystemRenderWorkspacederegisters only when the operation settles, andbeginPdfjsWorkerSystemShutdownawaits that cleanup — a never-settling render deadlocks the shutdown it is part of.Test
The single SIGTERM case becomes an
it.eachover all three signals, reads exit status and the host's real stderr off the process rather than restating either, and requires stderr to be exactly empty. No new file, so no inventory change.Non-vacuity, independently re-run on Node 20: with the fix 15/15 green; with the product reverted and the test kept, 3 red on the three signals. On Node 22 the reverted code passes — only the runtime that loses the race can see it.
Verification
test:node-native62 passed on both runtimes. Share mirrordiff -qclean,test:contract:sharegreen. Layout oracle untouched —server/pdfjs-worker.jsis not in the generator's source set.Full Node 22 gate: 2,402 passed, the only failures being the three
source-identitysuites hitting the clean-tree gate on a dirty worktree (26/26 when stashed).Honest gap: no Node 20 full gate came back green apart from those clean-tree failures, because the machine was loaded (load average 12–28 from back-to-back gates).
extraction-phase1failed in all three Node 20 runs but a different test each time, in a different way, has no import path topdfjs-worker, and passes 21/21 in isolation. Two controls place it outside this change: cleanorigin/masteron Node 20 gives exactly one failure (the target defect), and master plus one unrelated untracked file gives the 12 clean-tree failures plus the target. One Node 20 run on a quiet machine or in CI closes it.Merge before #150 — the CI matrix stays red on Node 20 until this lands.
🤖 Generated with Claude Code