test(fetch): deflake cyclic-reference leak tests by asserting on objectTypeCounts delta - #36759
Conversation
…ctTypeCounts delta These four tests assert on heapStats().objectTypeCounts.ReadableStream (or Response/Request) against an absolute threshold. In CI they run under 'bun test --parallel --isolate', where each worker process runs several test files against the same JSC VM and heapStats() is VM-wide, so the count includes streams left over from whichever file the worker ran immediately before. The previous file's global is unprotected after the isolation swap, but a stale pointer on the runner loop's stack keeps it pinned through conservative root scanning for one more file boundary. This surfaced as a flaky failure on Windows 11 aarch64 (build 86911) with 'Received: 109..111' against the '<= 100' threshold, but only because that lane's worker scheduling happened to place a stream-heavy file directly before these; the mechanism is platform-independent. Take a baseline count before the loop and assert on the delta, matching the pattern already used in fetch-stream-cancel-leak.test.ts and others.
WalkthroughChangesFetch leak test baselines
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only deflake that switches four leak tests from absolute objectTypeCounts thresholds to baseline+delta.
What was reviewed:
- Confirmed the baseline/delta pattern matches existing usage in
fetch-stream-cancel-leak.test.tsands3-stream-cancel-leak.test.ts. - Checked that assertion strength is preserved: loops create 500–10000 objects, so a real leak still blows past the unchanged 100/50 thresholds; only cross-file VM noise from
--parallel --isolateis subtracted out. - Verified no production code is touched and the added
Bun.gc(true)before each baseline read stabilizes the starting count.
Extended reasoning...
Overview
This PR modifies four test files in test/js/web/fetch/ to fix flakiness under bun test --parallel --isolate. Each test previously asserted an absolute heapStats().objectTypeCounts value against a threshold; now each records a baseline before the loop (after a forced GC) and asserts on the delta. No production code, thresholds, or loop iteration counts are changed.
Security risks
None. Test-only change with no effect on runtime behavior, no new inputs, no external network access.
Level of scrutiny
Low. This is a mechanical deflake applying an already-established pattern (fetch-stream-cancel-leak.test.ts, s3-stream-cancel-leak.test.ts, pipeTo-signal-leak.test.ts all use the same baseline/delta shape). The key REVIEW.md concern for deflakes — "keep asserting the property the original assertion protected" — is satisfied: the loops allocate 500–10000 objects, so a genuine leak produces a delta far above the unchanged 100/50 bounds. If prior-file objects happen to be collected mid-test the delta can go negative, which still correctly passes toBeLessThanOrEqual.
Other factors
The PR description gives a concrete cross-platform repro (--parallel=1 --isolate with a 120-stream polluter file) demonstrating the failure mode is real cross-file VM pollution rather than a masked regression. The author also swept test/ for the same pattern and reports the remaining objectTypeCounts checks are already delta-based or subprocess-isolated. No prior reviewer comments to address.
The extra heapStats() read for the baseline is cheap, so the leading full GC is unnecessary; the delta assertion holds whether or not the prior test's garbage has been swept yet. request-cyclic-reference's second test (async pull + two Requests + .body access per iteration) already overruns the 5s default timeout under debug+ASAN on main at 10000 iterations. Scale to 2000 under ASAN; a real cycle leak still leaves ~2000 streams against the 100 threshold.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/web/fetch/fetch-cyclic-reference.test.ts`:
- Around line 12-13: Run Bun.gc(true) immediately before every listed baseline
capture in fetch-cyclic-reference.test.ts (both Request and ReadableStream
baselines), request-cyclic-reference.test.ts, response-cyclic-reference.test.ts,
and server-response-stream-leak.test.ts. Preserve the existing countOf and
baseline logic while ensuring each heapStats objectTypeCounts baseline is
captured only after garbage collection.
- Line 8: Replace the seven heap-stat lookup fallbacks using || 0 with nullish
fallbacks ?? 0, preserving zero-valued counts. Apply this to countOf in
test/js/web/fetch/fetch-cyclic-reference.test.ts:8-8,
request-cyclic-reference.test.ts:9-9, response-cyclic-reference.test.ts:8-8, and
server-response-stream-leak.test.ts:10-11.
🪄 Autofix (Beta)
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: 3fc06e3f-1b39-41de-b778-187b07578fd9
📒 Files selected for processing (4)
test/js/web/fetch/fetch-cyclic-reference.test.tstest/js/web/fetch/request-cyclic-reference.test.tstest/js/web/fetch/response-cyclic-reference.test.tstest/js/web/fetch/server-response-stream-leak.test.ts
There was a problem hiding this comment.
LGTM — test-only deflake switching absolute objectTypeCounts thresholds to baseline+delta.
What was reviewed:
- Delta pattern matches the existing precedent in
fetch-stream-cancel-leak.test.ts(baseline before loop, assert onafter - baseline). - ASAN iteration reduction (10000→2000) in
request-cyclic-reference.test.tsstill leaves a ~20× margin over the 100 threshold if the cycle leak regresses, andisASANis the harness export — this shrinks the workload rather than raising the timeout, per repo guidance. - Checked that a negative delta (GC frees pre-existing streams from the prior file) can't cause a false pass masking a real leak: a regression leaves ~iterations objects behind, which dwarfs any baseline.
Extended reasoning...
Overview
Test-only change to four leak-regression tests in test/js/web/fetch/. Each previously asserted heapStats().objectTypeCounts.ReadableStream <= N as an absolute; under bun test --parallel --isolate the VM is shared across files in a worker, so objects pinned by a conservatively-scanned stale global from the previous file inflate the count. The fix captures a baseline before the loop and asserts on the delta. request-cyclic-reference.test.ts additionally scales iterations to 2000 under ASAN via isASAN from harness.
Security risks
None. No production code, no inputs, no external I/O changes.
Level of scrutiny
Low. This is a mechanical deflake of test assertions following a pattern already established in the same directory (fetch-stream-cancel-leak.test.ts) and elsewhere. The PR description names the specific flaky build, gives a deterministic local repro (--parallel=1 --isolate with a 120-stream polluter file), and confirms the sweep found no other absolute-threshold objectTypeCounts checks in parallel-allowlisted dirs.
Other factors
REVIEW.md's de-flaking rule — "keep asserting the property the original assertion protected" — is satisfied: the property is "N iterations don't leave >threshold streams behind", and 2000 leaked streams vs a 100 threshold is still an unambiguous failure. The ASAN scaling follows "shrink the workload" over "raise the timeout". isASAN is confirmed exported from test/harness.ts:54. No prior reviews or unresolved comments on the PR.
|
CI at 8eb9db5 (build 87694): all five changed files pass on every lane. Remaining reds are unrelated to this diff: |
RSS-delta leak tests are sensitive to process-wide allocator state. In a --parallel --isolate worker that has already run other files, the step-1 Bun.gc(true) frees the previous file's global into ASAN's quarantine right before the baseline read, and the async Bun.gc() in the inner loop falls behind on a larger JSC heap, pushing peak RSS past the threshold (78MB vs 64 on debian x64-asan in build 87678; also flaking on both Windows lanes against the 30MB non-ASAN threshold). Matches the existing excludeFiles entries for fs-leak, fetch-leak, streams-leak, html-rewriter-leak, express-memory-leak and node-tls-getpeercert-leak.
There was a problem hiding this comment.
LGTM — test-only deflake converting absolute objectTypeCounts assertions to baseline-delta, matching the established pattern in fetch-stream-cancel-leak.test.ts and siblings. The inline note on parallel-allowlist.json is a durability nit, not a blocker.
Extended reasoning...
Overview
This PR touches four leak-detection test files under test/js/web/fetch/ and adds one entry to test/parallel-allowlist.json. The core change replaces absolute heapStats().objectTypeCounts.X <= N assertions with baseline-captured-before-loop delta assertions, so VM-wide object counts carried over from a prior test file in the same --parallel --isolate worker no longer trip the threshold. request-cyclic-reference.test.ts additionally scales iterations from 10000 to 2000 under isASAN to stay under the default per-test timeout. No runtime/native code is touched.
Security risks
None. Test-only; no user-facing surface, no auth/crypto/permissions paths, no external inputs.
Level of scrutiny
Low. This is a mechanical deflake applying a pattern already used elsewhere in the same directory (fetch-stream-cancel-leak.test.ts, s3-stream-cancel-leak.test.ts). The key REVIEW.md concern for deflakes — "keep asserting the property the original assertion protected" — is satisfied: a real cycle-collection regression leaks on the order of iterations objects (500–10000), which dwarfs both the ~100-object cross-file carryover and the 50/100 thresholds, so the delta form still fails loudly on a real leak. The carryover objects are pinned by a conservative stack root through the next file (per the PR description), so they appear in both baseline and final reads and cancel out cleanly. The ASAN iteration reduction leaves ~20× headroom (2000 leaked vs 100 threshold).
Other factors
- All four files were run under
bun bd testper the PR evidence block (7 pass, 0 fail). - Both CodeRabbit threads (
|| 0vs?? 0, and pre-baselineBun.gc(true)) were addressed with sound reasoning by the author and withdrawn/resolved. - The one remaining finding — the hand-edited
excludeFilesentry in the generatedparallel-allowlist.jsonwill be dropped on the next regeneration — is real but bounded: worst case, the excluded test re-enters the parallel bucket, flakes once, and the next regeneration re-excludes it via the flake-annotation path. It does not affect correctness of this PR's primary change and is self-correcting, so it does not block approval. - No prior claude[bot] reviews on this PR.
… more in-process leak tests The runner reads only parallel-allowlist.json, so a denylist entry keeps a file out of the batch only once a regen has copied it into excludeFiles. Assert that every denylisted file in a listed dir is in excludeFiles (and that denylist entries are real files), so a by-hand denylist addition or a table produced by an older script fails in CI instead of running the file in the batch. Against the table from before #39373 the check names the 97 files that PR's body describes as batch-flaking. Add the three in-process RSS/heapStats leak tests whose batch failures predate the window the denylist was collected from (98887..99420), so the next regen cannot promote them: shell/leak (#36585: failed in the batch in 340 of 400 builds), request-clone-leak (excluded by hand in #36759 after batch failures on x64-asan and Windows) and zlib/leak (build 86483). All three are already in excludeFiles; this only makes the exclusion stick.
2 participants
What
request-cyclic-reference.test.tsandresponse-cyclic-reference.test.tswere flaky on Windows 11 aarch64 (build 86911) with:They pass when run alone; they only fail in the CI parallel batch.
request-clone-leak.test.tswas flaking the same way on debian x64-asan and both Windows lanes (RSS delta 78MB vs the 64MB ASAN threshold).Why
bun test --parallel --isolateruns several test files per worker process against the same JSC VM.--isolateswaps theJSGlobalObjectper file andgcUnprotect()s the old one, but a stale pointer to the just-swapped global sits on the runner loop's stack, and JSC's conservative root scanner pins it through the next file.The four
objectTypeCountstests asserted on an absolute VM-wide count, so streams from the previous file in the worker counted against the threshold. Repro on any platform:request-clone-leak.test.tsis RSS-based and already uses a delta, but RSS reflects process-wide allocator state: the step-1Bun.gc(true)frees the previous file's global into ASAN quarantine right before the baseline read, and the asyncBun.gc()in the inner loop falls behind on a larger JSC heap, so peak RSS overshoots.The Windows-aarch64-only appearance is scheduling luck: workers pull files from a shared queue, so which file precedes these in a given worker depends on timing.
Fix
{request,response,fetch}-cyclic-reference.test.ts,server-response-stream-leak.test.ts: take a baseline count before the loop and assert on the delta (matchesfetch-stream-cancel-leak.test.tsetc). Verified all four pass with a 120-stream polluter in front under--parallel=1 --isolate.request-cyclic-reference.test.ts: scale to 2000 iterations under ASAN (the second test was already overrunning the 5s default under debug+ASAN on main at 10000).request-clone-leak.test.ts: added toparallel-allowlist.jsonexcludeFilesso it runs solo, matching the existing entries forfs-leak,fetch-leak,streams-leak,html-rewriter-leak,express-memory-leak,node-tls-getpeercert-leak.Swept the rest of
test/: every otherobjectTypeCountscheck in a parallel-allowlisted directory already uses baseline/delta or runs in a spawned subprocess.[stamp-90s] gate passed · iteration 3 · 5 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 3
evidence per changed file