Repository navigation
Count signal-killed renderers apart and check the exit path statically - #3
Merged
Merged
Conversation
The og-image stress test failed macOS Node 24 once on #2: two of 24 renderers were killed by a signal under --stress-concurrent-allocation. None hung. A killed child never reached its exit, so it is now reported as unmeasured, and the test skips as unmeasured if more than a third are killed. All 24 still run at once: with process.exit() reinstated, 24 at once hung 15 of 96 children and one at a time hung none. A new static test reads the renderer and fails if it calls process.exit() or calls run() outside a try/catch. It failed on both hazards reinstated.
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
1 participant
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.
Fixes the macOS Node 24 failure on #2's CI run 38101706076, which passed on rerun:
bin/test/docsite-og-image-entry.test.mjs, "the renderer exits under concurrent-allocation stress".What failed
The test starts 24
bin/docsite-og-image.mjs --checkprocesses at once under--stress-concurrent-allocation. It guards against the Node 24/26process.exit()deadlock (nodejs/node#64274). On the failing run, 22 children exited 1 as expected and 2 were killed by a signal. None hung. The test treated the two killed children as failures, but neither ever reached its exit, so neither says anything about the exit path.Changes
Signal-killed children are counted apart. A hung child still fails the test, because that is the deadlock. A child killed by a signal is reported as unmeasured in a test diagnostic. If more than a third of the 24 are killed, the test skips with the reason, because too few exits were measured to count as a pass.
All 24 still run at once. Contention is what exposes the deadlock. With
process.exit(run())reinstated in the renderer and 24 runs per round, I measured:A lighter version of the test would mostly stop catching the regression.
A new static test, "the renderer never calls process.exit() and catches every throw from run()". It strips comments with
stripCommentsfrombin/lib/tracked-imports.mjs, then fails if the renderer callsprocess.exit(or callsrun()outside atry/catchthat setsprocess.exitCode. It runs in about 1 ms, and it catches a regression on every run, including rounds where the stress test happens to miss it. The test also runs the check on a renderer that has both hazards, and on a comment that only namesprocess.exit(). That shows the check fires on the first and ignores the second.Verification
docsite-og-image-entry.test.mjspassed 7/7 on three consecutive runs. The stress test took 1.6–2.7 s locally.process.exit(run())reinstated, the static test reports both hazards. With thetry/catchremoved, it reports the uncaughtrun().tracked-importspasses 19/0 against this commit.