Repository navigation
test: make worker_heap_snapshot_gc.test.ts race the parent's StrongSet instead of relying on volume - #41192
test: make worker_heap_snapshot_gc.test.ts race the parent's StrongSet instead of relying on volume#41192robobun wants to merge 1 commit into
Conversation
…t instead of relying on volume The release lane ran 15 processes of 300 getHeapSnapshot() round-trips (24s on alpine x64) as a probabilistic guard for #30185. With the bug reintroduced, that workload detects nothing: the parent VM idles while the worker builds the snapshot, so the worker's unlocked allocate and free on the parent's StrongSet never overlap a parent-side operation. The fixture now keeps the parent allocating and freeing Strong handles while each snapshot is in flight (structuredClone() converts its transfer list into Strong<JSObject> handles before it validates the entries). On a release build with the bug reintroduced, a round-trip corrupts the parent VM about half of the time and 100 of 100 processes crashed within 10 round-trips. The release lane now runs 5 processes of 10 round-trips (0.4s locally, was 9.4s). The fixture also validates ITERS, rethrows worker errors, parses the first payload as a V8 heap snapshot with a non-zero node count, and prints the completed count and the worker exit code, which the test asserts exactly.
|
Status: ready for review. How the numbers were produced: the bug was reintroduced locally in Supersedes #36952 (closed). |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughChangesThe regression fixture now combines worker heap snapshots with parent-thread structured-clone transfer-list churn. It validates worker lifecycle, snapshot streams, V8 payloads, completion counts, and exit codes. The test harness uses separate debug and release workloads with a fixed 60-second timeout. Heap snapshot GC race
Merge Risk: ⚪ Minimal · up to This PR updates a targeted worker heap-snapshot race test and its assertions without changing production behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, verification results, test configuration, and scope. It does not use the template headings exactly, but it contains the required content, including how the change was verified. Comment |
|
The automated review above has no action items, so the diff is unchanged. Buildkite is still running. I will post the release-lane timing for this file (alpine x64 was 24s) once that build finishes. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only rewrite that trades volume for a targeted race window, with detection and timing measurements stated in the header comments.
What was reviewed:
- Fixture control flow:
settledis polled viasetImmediate(no sleeps), stdout/stderr/exit are drained concurrently in the wrapper, and the combined.toEqualstill assertssignalCode: nullso a crash surfaces as a test failure rather than a timeout. - Churn mechanism:
structuredClone(0, { transfer: [64 plain objects] })requires onlyDataCloneErrorto escape the catch; anything else rethrows, so the fixture is not silently swallowing a behavior change there. The dependency onconvertTransferListallocating before validation is called out in the header. - The explicit 60_000ms timeout is not new (the old test used 60_000/120_000) and is now tighter; the removed
isASANbranch drops the workload from 1×100 to 5×10, which is smaller in total round-trips. - Changed files are not covered by CODEOWNERS.
Extended reasoning...
Overview
This PR touches two test files only: test/js/node/worker_threads/heap-snapshot-gc-race-fixture.js and test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts. No production code changes. The fixture is rewritten to actively churn the parent VM's StrongSet (via structuredClone with an invalid transfer list of 64 plain objects) while each getHeapSnapshot() round-trip is in flight, replacing the previous idle-parent-plus-Bun.gc(true) approach. It adds strict ITERS validation, wires worker error/exit handlers, parses the first snapshot payload to assert V8-format structure (node_count × node_fields === nodes.length), asserts every stream is non-empty, and prints a summary line the wrapper matches exactly. The wrapper reduces the workload from 15×300 (release) to 5×10 (release/ASAN) or 1×5 (debug), drops the isASAN import, and tightens the timeout from 60_000/120_000 to a flat 60_000.
Security risks
None. This is a test fixture and test wrapper; no user-facing surface, no parsing of untrusted input, no network, no filesystem writes outside the harness, no dependency changes.
Level of scrutiny
Low-to-moderate. Test-only changes to an existing stress test warrant checking that the new test still fails for the right reason and follows harness conventions, but do not need the scrutiny of runtime code. The PR description provides empirical hit-rate measurements (0/30 detection on the old fixture with the bug reintroduced vs. 100/100 within 10 round-trips on the new one) and states the bun bd test / release timing. The fixture polls a settled flag via setImmediate rather than sleeping, wires failure events, and the wrapper drains pipes concurrently and asserts a single combined object per attempt — all matching the patterns REVIEW.md and test/CLAUDE.md ask for. The one documented fragility (the churn goes silent if convertTransferList ever validates before converting) is called out in the fixture header and framed as covered by the follow-up deterministic assert in #36958; since the test being replaced already detects nothing per the PR's measurements, this is a strict improvement.
Other factors
The explicit per-test timeout technically conflicts with test/CLAUDE.md's "no timeouts" rule, but it pre-exists this PR (the old test had 60_000/120_000), the new value is stricter, and the comment justifies it as the time-to-red for a hang-mode regression — root CLAUDE.md permits per-test timeouts for outliers. The worker.on("error", e => { throw e }) pattern in the fixture is acceptable in a spawned subprocess: an uncaught exception crashes the fixture with stderr output, which the wrapper's stderr: "" / exitCode: 0 assertion catches. CODEOWNERS does not cover these paths. No prior review comments or objections exist in the timeline. The bug hunt exited on dry_streak with no findings.
|
Thanks, nothing to change from that review. Buildkite build 109405 is at 161 of 181 jobs passed with the rest running. The per-lane timing for this file follows when it completes. |
Problem
worker_heap_snapshot_gc.test.tstakes 24s on alpine x64 (build 109310): 15 processes x 300getHeapSnapshot()round-trips, a probabilistic guard for worker: fix cross-thread HandleSet race in getHeapSnapshot #30185.src/jsc/bindings/webcore/JSWorker.cpp:755, a by-valueStrong<JSPromise>capture), 30 of 30 processes survive 300 round-trips. The parent idles during the snapshot, so the worker's unlockedStrongSetcalls never overlap a parent-side one.Fix
convertTransferListmakes aStrong<JSObject>perstructuredClone()transfer entry before validation, so a plain-object list is pure allocate/free churn.ITERS, rethrows worker errors, parses the first payload as a V8 heap snapshot with a non-zero node count, and printsok snapshots=N workerExitCode=1, matched exactly.bun bd test test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts, 22s to 25s, unchanged (5 debug snapshots of 3s). Release: 0.4s, was 9.4s.Background
JSC::Strong<T>is a GC root slot from the VM'sStrongSet, a slab allocator that expects the VM lock but does not check it. Upgrade WebKit to 47f7250137c6 #39371 replaced the oldHandleSetlist with it, so the torn-list crash of worker: fix cross-thread HandleSet race in getHeapSnapshot #30185 cannot happen today.getHeapSnapshot()site only. The deterministic guard for all five cross-VM sites is a lock-held assert inStrongSet(Bump WebKit: assert the VM's API lock is held on HandleSet mutation #36958, needs a re-port). test(worker_threads): speed up worker_heap_snapshot_gc via getHeapStatistics for the race loop #35200 and test: shrink worker_heap_snapshot_gc release workload, assert fixture completion count #36952 resized this test without restoring detection. Supersedes test: shrink worker_heap_snapshot_gc release workload, assert fixture completion count #36952.Notes
Bug reintroduction used for every measurement (local only, not in this PR): add
JSC::Strong<JSC::JSPromise> bugStrong(vm, promise);beforepostTaskToWorkerGlobalScopeinjsWorkerPrototypeFunction_getHeapSnapshotBodyand capturebugStrongby value in both the worker-side lambda and the completion lambda it posts back.Release build (local, non-LTO, 16 cores), 100 processes per row unless noted, 10 concurrent:
Bun.gc(true)per round-trip)What remains after #39371 is allocator against allocator, which only a busy parent exposes. Failure modes seen, all on the parent thread inside
structuredClone():SIGSEGVinJSC::StrongSet::tryAllocateFromCurrent(bogus slot pointer, for example0x300000B15DE0),SIGABRTfrom aRELEASE_ASSERTreached throughJSC::StrongSet::didFreeSlot(the used count and block bookkeeping are release-checked inStrongBlock::decrementUsedCountandStrongSet::appendAvailable), and once aTypeErrorbecause a corrupted handle changed an unrelated cell.Churn shapes compared at 5 round-trips, 40 processes each: a transfer list of fresh
ArrayBuffers that clone cleanly crashed 15; a transfer list of plain objects (this PR) crashed 40. The clean clone spends most of each call serializing, so the parent is inside the allocator far less often. The plain-object shape depends onconvertTransferListconverting beforeSerializedScriptValue::createvalidates. The fixture header says so.Timing detail:
bun bd test21.6s and 24.6s after, 20.9s and 24.0s before. Releasebun test0.41s to 0.47s after, 9.39s and 9.49s before (16 cores).Debug+ASAN (
bun bd) with the bug: 7 of 16 processes crashed at 5 round-trips, 3 of 4 at 10. A debug snapshot takes about 3s, so the debug lane stays at 1 process of 5 round-trips.Fixed build, test file: 30 of 30 runs pass on 16 cores, 10 of 10 pinned to 2 cores (1.2s to 1.4s). Bugged build: 30 of 30 runs fail on 16 cores, 10 of 10 pinned to 2 cores. Pinned to 1 core the bugged build fails only 2 of 5 runs: without true parallelism the parent has to be preempted inside the few instructions of an allocate, which is rare. CI runners have more than one core (the old 4500 round-trips finished in 24s there).
The assertion changes were checked against a fixture edited to exit after 9 round-trips (
snapshots=9fails the match) and one that writes a line to stderr (thestderr: ""match fails).Self-reviewed: 4 concerns raised, 4 addressed (assert attribution in both headers, scope and follow-up stated in the headers, the validation-order dependency documented, related PRs cited). Parameterizing the fixture over the other four cross-VM sites is a follow-up, not part of this PR.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/worker_threads/worker_heap_snapshot_gc.test.ts