test: shrink worker_heap_snapshot_gc release workload, assert fixture completion count - #36952
test: shrink worker_heap_snapshot_gc release workload, assert fixture completion count#36952robobun wants to merge 3 commits into
Conversation
… completion count The 15x300 release workload was sized as a probabilistic crash guard for the #30185 cross-thread HandleSet race. That detection relied on the GC controller collecting every ~16ms of event-loop activity, which made the parent's strong-handle scans overlap the worker's task teardown. #35356 removed those per-tick collections, and the reintroduced bug now survives the full 15x300 workload with zero detections in 18000 iterations (it was caught at ~60% per process before). The loop is kept as a functional check of the cross-VM round-trip at 15x25 in release; the ASAN lane keeps 100 iterations for memory-bug coverage. The fixture now validates ITERS and reports the completed iteration count, and the test asserts the exact count so a fixture that silently exits early fails instead of passing.
WalkthroughThe fixture now validates snapshot payloads and completed iterations. The test selects numeric iteration counts by build mode, checks the completion output, and uses a fixed 60-second timeout. ChangesHeap snapshot GC validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs — the fixture/test changes are internally consistent (ITERS validation, ok <count> assertion, retry path still yields completed === iters). Deferring to a human because this deliberately shrinks a memory-safety regression guard ~12x on the basis that it can no longer detect the reintroduced bug, with no replacement guard in this PR; that trade-off is a maintainer call.
What was reviewed:
- Retry path (
ERR_WORKER_NOT_RUNNING→i--; continue) still converges tocompleted === iters, so the new exact-count assertion won't false-fail on worker restarts. itersis now a number and correctlyString()-ed into env; the debug/ASAN/release branching (1x5 / 1x100 / 15x25) matches the PR description.Number.isIntegerguard rejects unset/NaN/non-integerITERSand the fixture exits nonzero, so a lost env var now fails the test instead of passing vacuously.
Extended reasoning...
Overview
Test-only change to two files under test/js/node/worker_threads/. The release-lane workload for the #30185 cross-thread HandleSet race guard drops from 15×300 to 15×25 iterations (~34s → ~1.3s), the fixture now validates ITERS and prints ok <completed>, and the test asserts the exact count. Header comments are rewritten to state that the test is now a functional round-trip check rather than a probabilistic crash guard, because #35356 removed the per-tick GC collections that made the race observable from JS.
Security risks
None. No production code is touched; this is a test workload resize plus a strengthened assertion in a subprocess-spawning fixture.
Level of scrutiny
Moderate-to-high, despite being test-only. REVIEW.md is explicit: "Never silently weaken, skip, or delete an existing test or safety net." This PR is not silent — it comes with an unusually thorough empirical justification (bug reintroduced, 0 detections in 18k iterations across multiple GC-scheduling shapes, forced-overlap instrumentation confirming the race is still real). The argument is that the safety net already catches nothing on current main, so shrinking it loses no coverage. That reasoning is sound on its face, but accepting "this guard no longer works so shrink it" versus "make it work again (or add the deterministic HandleSet-thread-affinity assertion the PR itself suggests)" is a coverage-philosophy decision a maintainer should own, not a bot.
Other factors
- The assertion is genuinely strengthened: the old fixture printed bare
okeven ifITERSwas unset (loop ran zero times); the new one throws on invalidITERSand the test asserts the exact completed count. - I traced the
ERR_WORKER_NOT_RUNNINGretry:i--; continuere-runs the iteration without touchingcompleted, so the final count still equalsitersand the new assertion holds under worker restarts. - Debug (1×5) and ASAN (1×100) paths are unchanged in size; only plain release drops. Timeouts (60s slow / 120s release) are unchanged and remain generous for the smaller workload.
- No prior human or claude[bot] reviews on the timeline; only a CodeRabbit rate-limit notice.
|
The trade-off called out in the review is intentional, so to make the maintainer call easier: The 12x reduction does not remove working coverage. The sizing existed to catch one specific race, and that detection is already gone: the reintroduced #30185 bug survives the full 15x300 workload on current main (0 detections in 18,000 iterations; full matrix in the PR body). Keeping the old size keeps the cost of the guard without its effect, on every release lane of every build. A working replacement also cannot live in this test. The race window is a few instructions of worker-side handle teardown per 40ms round-trip, and since #35356 changed the GC cadence there is no JS-constructible schedule that reliably lands a strong-handle scan inside it (that is what the 6 fixture shapes in the measurement table were trying to do). The effective replacement is a deterministic detector in the engine, for example an assertion that HandleSet mutation happens on the owning thread, which would have caught #30185 at iteration 1 of the existing debug lane. That is a vendored-JSC change that deserves its own PR and review rather than riding along with a test resize. If keeping 15x300 until such a detector exists is preferred, I can drop the resize from this PR and keep just the assertion strengthening; the measurements stand either way. |
|
Updated 4:58 AM PT - Aug 5th, 2026
❌ @robobun, your commit 3f8663f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36952That installs a local version of the PR into your bun-36952 --bun |
|
CI status for build 89151: 194/196 jobs passed. The two failed jobs are both the debian 13 x64-asan test shards, failing on two tests that also fail on main and are untouched by this change (test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts with an ExceptionScope SIGABRT, and test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts with RSA keygen leak reports); both are already reported for main triage. The resized worker_heap_snapshot_gc test passed on every lane that runs it, including alpine release and the ASAN lane. Remaining failures in the annotation list passed on retry. |
… stale 120s timeout Review follow-ups: the fixture header now describes the current reqId-map design (#31216) instead of the superseded raw-pointer fix, the mid-loop comment no longer claims a GC/teardown overlap this PR measured to be gone, every iteration fails on an empty snapshot stream (one payload per process is parsed as JSON), and the 120s release timeout arm sized for the old 15x300 workload collapses into a single 60s ceiling.
|
Pushed review follow-ups in 2ca44ba:
Verification re-run after the changes: debug path passes in ~23s locally, release path 10/10 green runs at ~1.2s each (timing unchanged by the payload checks). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js`:
- Around line 31-32: Update the ITERS validation before makeWorker() to parse
the raw environment value, require a safe positive integer within a documented
maximum, and reject values such as 1e100 before creating the worker. Include the
accepted range and the original rejected value in the validation error message.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38f6e780-e8cb-48ac-b4c4-3246dbe42c07
📒 Files selected for processing (2)
test/js/node/worker_threads/heap-snapshot-gc-race-fixture.jstest/js/node/worker_threads/worker_heap_snapshot_gc.test.ts
There was a problem hiding this comment.
No bugs found, but this deliberately reduces a stress test's release workload 12x (15x300 → 15x25) on the basis that the probabilistic guard no longer fires post-#35356. The measurements are thorough and the assertion strengthening is a clear improvement, but the author explicitly flags the resize as a maintainer call (offering to keep 15x300 until a deterministic HandleSet-thread-affinity detector lands) — that trade-off should be signed off by a human.
What was reviewed:
- Fixture correctness: ITERS validation,
completedcounting vs thei--retry path,chunks?.pushfirst-iteration-only collection, and theok ${iters}stdout match — all consistent. - The
itersstring→number change flows correctly throughString(iters)in env and the template-literal assertion. - Stream draining:
once("end")without an error→reject wire is unchanged from the original fixture, not a regression here.
Extended reasoning...
Overview
Test-only change to test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts and its fixture. Three parts: (1) shrink the release-lane workload from 15 processes × 300 iterations to 15 × 25; (2) strengthen assertions — the fixture now validates ITERS, prints ok <count>, checks every stream is non-empty, and JSON-parses one payload per process, while the test asserts the exact completion count alongside stderr/exitCode/signalCode; (3) rewrite header comments to accurately describe what the test guards today, and unify the timeout to 60s.
Security risks
None. Test and fixture code only; no runtime, auth, crypto, or user-facing surface touched.
Level of scrutiny
Medium-high, specifically because REVIEW.md calls out "never silently weaken, skip, or delete an existing test or safety net" as a blocker. This PR is not silent — it provides extensive revert-and-count measurements showing the 15x300 workload detects the reintroduced #30185 bug 0 times in 18,000 iterations on current main (vs ~60% per-process before #35356 changed GC cadence), and documents six alternative fixture shapes that also fail to reproduce. The reasoning is sound: the guard's cost remains but its effect is already gone. However, the author explicitly frames this as a maintainer decision and offers to drop the resize if the preference is to keep the old size until a deterministic detector (e.g., a HandleSet thread-ownership assertion in JSC) exists. That is the right framing, and it is exactly the kind of coverage-vs-CI-cost trade-off a human maintainer should confirm rather than a bot.
Other factors
The mechanical changes look correct: iters moved from string to number and is re-stringified for env and interpolated in the stdout assertion; the ERR_WORKER_NOT_RUNNING retry path decrements i without incrementing completed, so the final count matches iters; the debug/ASAN precedence (isDebug ? 5 : isASAN ? 100 : 25) preserves the old ordering. CI (build 89151) passed on every lane that runs this test; the two ASAN failures are pre-existing on main and unrelated. The assertion-strengthening half of the PR is unambiguously good and could land on its own.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js`:
- Around line 30-33: The ITERS validation currently permits impractically large
safe integers; update the validation near the `iters` check to define and
enforce a named maximum of 300, while preserving positive integer values through
300. Update the error message to state the accepted range and keep the existing
5, 25, 100, and 300 iteration modes valid.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bd113c96-260b-427c-9be4-2617316bc11c
📒 Files selected for processing (1)
test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js
There was a problem hiding this comment.
No bugs found — the mechanical changes (ITERS validation before worker creation, ok <count> completion assertion, non-empty stream + first-payload JSON check, flat 60s timeout) are all strict improvements and look correct. Deferring because the 15×300 → 15×25 release resize is a coverage-vs-CI-time trade-off the author explicitly frames as a maintainer call ("if keeping 15x300 until such a detector exists is preferred, I can drop the resize"), and REVIEW.md treats weakening a safety net as something a human should sign off on even when well-justified.
What was reviewed:
- Fixture: ITERS is validated with
Number.isSafeIntegerand> 0beforemakeWorker(); the raw env value is echoed in the error — CodeRabbit's concern is addressed. - Stream handling:
datahandler is registered before awaitingend, so no chunks are dropped;chunks?.pushcorrectly no-ops after the first payload is parsed. - Test:
itersis now numeric and stringified only at the env boundary, so theok ${iters}assertion matches the fixture'sok ${completed}; debug/ASAN paths keep their prior 5/100 counts and 60s ceiling.
Extended reasoning...
Overview
Test-only change to two files under test/js/node/worker_threads/. The fixture now validates ITERS as a positive safe integer before spawning the worker, counts completed round-trips, asserts every snapshot stream is non-empty, JSON-parses the first payload per process, and prints ok <count>. The test drops the release iteration count from 300 to 25 (debug 5 and ASAN 100 unchanged), asserts the exact completion count in stdout, and replaces the branched 60s/120s timeout with a flat 60s. Comments are rewritten to state that post-#35356 the workload no longer detects the #30185 race it was originally sized for.
Security risks
None. No production code is touched; the fixture reads only process.env.ITERS (set by the test itself) and the changes tighten validation rather than loosen it.
Level of scrutiny
Medium. The code changes themselves are simple and self-contained, and CI (build 89151) shows the resized test green on every lane including alpine release and ASAN. However, the substantive decision — dropping a 12× stress-test workload that was designed as a probabilistic crash guard — falls squarely under REVIEW.md's "never silently weaken an existing test or safety net" rule. The PR is anything but silent (the empirical case is thorough: 0 detections in 18k iterations with the bug reintroduced, and a full table of alternative fixture shapes tried), but the author's own follow-up comment explicitly presents this as a choice for a maintainer to make and offers to keep the assertion-strengthening without the resize. That's the right framing, and a human should be the one to accept it.
Other factors
- The one CodeRabbit inline (validate ITERS before creating the worker, reject unsafe integers) is resolved in 3f8663f: validation now uses
Number.isSafeInteger, runs beforemakeWorker(), and echoes the rejected raw value. - I checked that registering the
datalistener afterBun.gc(true)but before awaitingendcannot miss chunks — the stream is returned paused and the handler is attached synchronously before any await, so all data events are observed. - The
iterstype change from string to number is consistent end-to-end:String(iters)at the env boundary andok ${iters}in the assertion both produce "25"/"100"/"5" matching the fixture'sok ${completed}. - The flat 60s timeout is safe for the unchanged debug/ASAN paths (previously also 60s) and generous for the ~1.3s release path; it also bounds livelock detection as the comment notes.
…ctors Review of the WebKit bump surfaced two gaps: Bun__StrongRef__new/set/delete mutate the StrongRootBlock list that the Srb marking constraint scans, with the same API-lock requirement as JSC's HandleSet but no assertion. Add the mirror asserts, checking the owner VM (the block's), so a thread holding some other VM's lock still trips them. Strong.rs's existing TLS check covers only the delete path and only proves the thread has a VM, not the owner VM's lock; the C++ Bun::StrongRef deleter bypasses it entirely. Neither detector had a liveness test, which is how the previous probabilistic guard for #30185 died silently (#35356, measured in #36952). jscInternals gains a debug-only crossThreadStrongHandleMutation hook that violates the contract from a spawned thread, and the new test asserts the child aborts with each guard's message. If a future WebKit bump or binding refactor drops an assertion, the test fails instead of the coverage silently disappearing.
1 participant
What does this PR do?
worker_heap_snapshot_gc.test.tstakes ~34s on alpine 3.23 x64 (build 89119) and ~12s locally on a 12-core release build, because its release path runs 15 concurrent processes x 300getHeapSnapshot()round-trips as a probabilistic crash guard for the #30185 cross-thread HandleSet race (panic: Segmentation fault at address 0x10in the "Sh" Strong Handles marking constraint).Before resizing anything I re-did the revert-and-count verification that sized this test (same method as the one documented in #35200: reintroduce a by-value
Strong<JSPromise>capture ingetHeapSnapshot's outer cross-thread lambda, build release, count detections). The result is qualitatively different from July:The race window itself is still real: with the reintroduced bug plus instrumentation that forces the worker-side handle teardown to overlap a parent GC, every process corrupts (mix of segfaults at 0x10 and livelocks on the torn
SentinelLinkedList). What disappeared is the scheduling coincidence that let the unmodified workload hit the window: detection relied on the GC controller collecting every ~16ms of event-loop activity, and #35356 (merged Jul 29) removed those per-tick collections. The fixture's ownBun.gc(true)runs strictly between round-trips, so it almost never overlaps the worker's task teardown.I tried to rebuild the overlap from JS before giving up on it: synchronous GC pumping during the round-trip, async
Bun.gc(false)pumping,BUN_GC_TIMER_INTERVAL=1/16idle collections, and stretching the strong-handle list with pools of in-flight cross-VM requests. Best case across ~30,000 bugged iterations was a single segfault; most shapes produced zero. The per-mutation window is a few instructions once per ~40ms round-trip, and nothing constructible from JS reliably lands a strong-handle scan inside it on current main.So the 34s release workload no longer buys the detection it was sized for, on any lane. This PR:
ITERSand printok <completed count>, and makes the test assert the exact count, so a fixture that silently exits early (for exampleITERSlost in env plumbing, which previously made the loop a no-op that still printedok) fails instead of passing. stderr and signal are asserted alongside, and a run against an instrumented binary that writes to stderr confirms the assertion fails the test.A deterministic guard for this bug class (for example a debug assertion that
HandleSetmutation happens on the owning thread, or thread-sanitizer coverage) is the right long-term replacement for scheduling-dependent crash probing, and is out of scope here.Verification
Timing, local (16 visible cores / 12-core quota):
bun test(15x300 -> 15x25)bun bd test(debug+ASAN path, 1x5, unchanged)Expected CI effect: the ~34s alpine/debian/ubuntu release lanes drop to a few seconds; debug and ASAN lanes unchanged.
Stability and assertion checks:
ITERS: exits 1 (validation works).Full detection measurements on main @ b58cd46 with the bug reintroduced
All runs: 15 concurrent fixture processes under the bunEnv test environment, release build, per-process outcomes classified as completed / segfault / livelock (killed at deadline).
Bun.gc(true)after each await)BUN_GC_TIMER_INTERVAL=16BUN_GC_TIMER_INTERVAL=1Bun.gc(true)+ setImmediate loop through drain)Bun.gc(false))getHeapStatistics()strong handlesm_strongListfor 4ms after posting)The forced-overlap row is what shows the race itself is alive; everything above it shows the unmodified bug shape no longer meets a strong-handle scan under any JS-constructible GC schedule.
no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.