test(fetch-leak): keep the RSS leak thresholds clear of measurement noise - #37209
test(fetch-leak): keep the RSS leak thresholds clear of measurement noise#37209dylan-conway wants to merge 5 commits into
Conversation
…oise Two RSS-based checks in fetch-leak.test.ts failed intermittently without anything leaking: - "Request body HiveRef pool returns slot": each cycle keeps 512 Requests with 128 KiB bodies alive at once (64 MiB), and memory freed by gc() is not necessarily returned to the OS by the time rss() is sampled, so baseline and final each carry up to one cycle's live set of slack. The 64 MiB threshold equalled that noise (observed deltas on a non-leaking build: -73..-10 MiB locally, +68/+74 in CI). Use 16 KiB bodies: one cycle is 8 MiB, a real leak is still 256 MiB, the threshold stays 64. - "should not leak using readable stream": the fixture allows RSS to grow 5 MiB between iteration 250 and 500. The regression it guards retained the 128 KiB body per request (~32 MiB over that window); a non-leaking run drifts a few MiB (CI failures were at 5.05-5.1). Allow 16 MiB. No-Verification-Needed: test-only change
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe fetch leak tests adjust payload sizes, leak thresholds, ASAN quarantine limits, and memory-growth allowances for allocator and ASAN quarantine noise. ChangesFetch leak test calibration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:56 PM PT - Aug 8th, 2026
✅ @robobun, your commit 0d9df3db4a96906510b02ee351f220d0bca5e1b5 passed in 🧪 To try this PR locally: bunx bun-pr 37209That installs a local version of the PR into your bun-37209 --bun |
|
@robobun adopt |
|
Adopted. Both review catches addressed: every RSS bound in the two tests now sits below its leak signal, verified under the debug ASAN build clean (1-7 MiB drift) and with simulated leaks (+76 and +278 MiB against 24 and 128 MiB bounds). Numbers in the PR body. Watching CI. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
…l, widen the stream allowance to 20 MiB The 16 KiB bodies shrank the guarded HiveRef leak signature to 256 MiB, which the 320 MiB ASAN bound sat above, so the ASAN lane could no longer fail on the regression it guards. Cap the child's quarantine like the readable-stream test already does and bound at 128 MiB: clean ASAN runs measure deltaMB 5-7, a simulated retained-body leak measures +278. Windows lanes measured up to 14 MiB of non-leak RSS drift over the fixture-6 sample window (scan in #33988), leaving the 16 MiB allowance only 2 MiB of headroom; move it to 20 MiB, still well under the 32 MiB the guarded regression produces.
|
On the duplicate candidates:
|
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/web/fetch/fetch-leak.test.ts:748-753— Same class as the line 715 finding: the new comment states the guarded regression retains ~32 MiB over the 250 sampled iterations, but the ASAN branch of this threshold is still 64 MiB — so underbun-asanthe #23697 regression (~36-40 MiB observed with redzones/shadow, quarantine already saturated at 32 MB before the sample point) would pass. This is pre-existing (the PR only changed"5"→"16"and the comment; non-ASAN lanes still catch it), but since line 715 needs the same fix it's worth tightening here too — e.g.isASAN ? "24" : "16", or drop the branch entirely given the quarantine cap.Extended reasoning...
What the fixture measures
fetch-leak-test-fixture-6.jsfetches a 128 KiB body 500 times throughgetReader(), samples RSS ati == 250, then after all 500 iterations +Bun.gc(true)assertsfinal - sample <= MAX_MEMORY_INCREASE. So the measured window is the second ~250 iterations. The regression this test guards (#23697) retained the 128 KiB body per request — 250 × 128 KiB ≈ 31 MiB raw over that window, which the PR's new comment now states explicitly on the line above the threshold.Why the ASAN branch is vacuous
The threshold is
isASAN ? "64" : "16". On the ASAN lane:- Redzones on 128 KiB allocations cap at ~2 KiB each (~1.5%), and shadow memory is 1/8 of the allocation → the retained 31 MiB reads as ~36-37 MiB in RSS.
- The child's
ASAN_OPTIONSalready capsquarantine_size_mb=32(set a few lines below). 250 iterations of ≥128 KiB freed churn saturates that quarantine well beforei == 250, so quarantine contributes ~0 additional growth over the measured window — its RSS contribution is already baked into the sample.
So if #23697 reappeared, the ASAN lane would observe a delta of ~36-40 MiB, which is under the 64 MiB allowance, and the test would pass with the leak present. REVIEW.md: "RSS thresholds branch on
isASAN/isDebugwith the bound well below the unfixed leak" — 64 is above ~32, not below it.Step-by-step proof (ASAN lane, regression reintroduced)
- Iterations 0-250 leak 250 × 128 KiB ≈ 31 MiB of retained bodies; quarantine fills to its 32 MB cap and stays there.
rssSamplecaptures both. - Iterations 251-499 leak a further ~31 MiB of bodies; quarantine stays at 32 MB (steady-state, freed churn recycles through it).
Bun.gc(true)runs; leaked bodies are unreachable-from-JS but still pinned natively, so they stay resident.final - rssSample≈ 31 MiB raw + ~5-6 MiB ASAN overhead ≈ 36-40 MiB.36-40 <= 64→ assertion passes → child printsdone, exits 0 → parent test passes with the leak present.
The non-ASAN branch (threshold 16) would catch it (31 > 16), so overall coverage is not lost — only the ASAN lane's assertion is unable to fail on the guarded regression.
Why this is a nit, not blocking
The
isASAN ? "64"value and the ~32 MiB leak signal both pre-date this PR — the diff on this line is"5"→"16"(non-ASAN branch only) plus the rewritten comment. Unlike the line 715 case (where shrinking the body from 128 KiB → 16 KiB actively dropped a previously-valid ASAN threshold from load-bearing to vacuous), here the PR did not weaken the assertion; it only made the pre-existing gap visible by documenting the ~32 MiB figure directly beside a 64 MiB bound. Non-ASAN lanes still catch the regression. Nothing this PR does makes anything worse on this line.Why it's still worth fixing here
REVIEW.md: "Fix the whole class in the same PR". The already-flagged line 715 issue is the same class (ASAN threshold ≥ leak signal), and the author will be editing thresholds in this file anyway. Tightening this one in the same pass costs one character.
Suggested fix
Either scale the ASAN bound to sit between the observed noise and the leak signal — e.g.
isASAN ? "24" : "16"— or, sincequarantine_size_mb=32is capped and saturated before the sample point (so quarantine contributes no delta), drop the branch and use a single threshold for both lanes.
![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/web/fetch/fetch-leak.test.ts`:
- Around line 724-728: Update the ASAN_OPTIONS construction in the fetch leak
test to normalize inherited options before appending quarantine_size_mb=32.
Remove or replace any existing quarantine_size setting from bunEnv.ASAN_OPTIONS,
then append the 32 MB value while preserving unrelated options and the existing
non-ASAN behavior.
🪄 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: 4a921dd2-1c0d-47c2-9534-a537f5c3fcee
📒 Files selected for processing (1)
test/js/web/fetch/fetch-leak.test.ts
… leak signal Same class as the HiveRef bound: the 64 MiB ASAN allowance sat above the ~32 MiB the #23697 regression produces over the sampled window. With the quarantine capped at 32 MB it saturates before the sample point; measured under the debug ASAN build, clean runs drift 1-3 MiB and a simulated regression (every read chunk retained) measures +76 MiB. Bound at 24.
|
The follow-up on the stream fixture ASAN allowance is addressed in 3734c39: measured under the debug ASAN build, the capped quarantine saturates before the sample point, so clean runs drift 1-3 MiB while a simulated regression (every read chunk retained) measures +76 MiB. The allowance moves from 64 to 24 MiB, below the ~32 MiB signal, same treatment as the HiveRef bound. |
There was a problem hiding this comment.
LGTM — both earlier review catches (HiveRef ASAN bound 320>256, stream ASAN bound 64>32) are addressed in 23d51ac and 3734c39, and every threshold now sits between its measured noise floor and its leak signal. The remaining inline note is a stale-comment nit only.
Extended reasoning...
Overview
Test-only de-flake of two RSS-delta leak checks in test/js/web/fetch/fetch-leak.test.ts:
- HiveRef pool test: body shrunk 128 KiB → 16 KiB so one cycle's live set (the measurement noise floor) drops from 64 MiB to 8 MiB against an unchanged 64 MiB non-ASAN threshold; ASAN threshold scaled 320→128 MiB and the child's quarantine capped at 32 MB. Leak signal is 256 MiB; simulated leak measured +278 MiB.
- Readable-stream fixture: non-ASAN allowance 5→20 MiB (Windows lanes observed up to 14 MiB non-leak drift), ASAN allowance 64→24 MiB. Guarded regression signal is ~32 MiB; simulated leak under ASAN measured +76 MiB.
No production code touched.
Security risks
None. Test threshold and payload-size adjustments only; no auth, crypto, network-facing surface, or input handling.
Level of scrutiny
Medium — de-flaking by adjusting RSS thresholds is exactly the class REVIEW.md calls out for "keep asserting the property the original assertion protected" and "bound well below the unfixed leak". I raised both instances where an ASAN bound sat above its shrunken signal in earlier rounds; both are now fixed with measured verification (clean 5-7 / 1-3 MiB vs. simulated-leak +278 / +76 MiB against 128 / 24 MiB bounds). Every branch of every touched threshold now demonstrably sits between noise and signal.
Other factors
- The
...(isASAN && { ASAN_OPTIONS: ... })spread is the file's existing idiom (lines ~80, 501, 573, 767, 968) and correctly no-ops whenisASANis false. - CodeRabbit's
quarantine_sizenormalization concern was withdrawn after robobun showed the inherited value never contains the deprecated flag. - Overlapping PRs (#36981 superseded/closed, #33988's Windows measurements incorporated) are accounted for in the PR body.
- The one remaining finding is a nit: the pre-existing comment on the
ASAN_OPTIONSline still says "over the 64 MB allowance" after the allowance moved to 24. Prose only; the quarantine cap it justifies is still correct (97 > 24). Not merge-blocking.
There was a problem hiding this comment.
LGTM — all three earlier review points are addressed (ASAN bound scaled to 128 below the 256 MiB HiveRef signal with quarantine capped; stream-fixture ASAN allowance dropped to 24 below its ~32 MiB signal; stale "64 MB allowance" comment de-numbered).
What was reviewed:
- Both RSS bounds now sit below their guarded leak signals on every lane (64/128 < 256 MiB HiveRef; 20/24 < 32 MiB stream), backed by the simulated-leak measurements in the PR body.
- The 16 KiB body shrink keeps the noise floor (one 8 MiB cycle) well under the 64 MiB threshold without changing request count or the >256 hive/fallback coverage.
- The new
ASAN_OPTIONSquarantine cap matches the existing idiom used elsewhere in this file;bunEnv.ASAN_OPTIONSsources don't set the deprecatedquarantine_size.
Extended reasoning...
Overview
Test-only de-flaking change to test/js/web/fetch/fetch-leak.test.ts, touching two RSS-delta leak checks that were flaking in ~13 of the last 80 main builds. For the HiveRef pool test, it shrinks the per-Request body from 128 KiB to 16 KiB (dropping the per-cycle noise floor from 64 MiB to 8 MiB against an unchanged 64 MiB non-ASAN threshold), scales the ASAN threshold from 320 to 128 MiB, and caps the child's ASAN quarantine at 32 MB. For the readable-stream fixture, it widens the non-ASAN allowance from 5 to 20 MiB (Windows lanes observed up to 14 MiB of non-leak drift) and tightens the ASAN allowance from 64 to 24 MiB. Comments are updated to reflect the new arithmetic. No production code is touched.
Security risks
None. This is a test-threshold adjustment in a leak regression test; no auth, crypto, parsing, or user-facing surface is involved.
Level of scrutiny
Moderate — the REVIEW.md rule that matters here is "when de-flaking, keep asserting the property the original assertion protected" and "RSS thresholds branch on isASAN with the bound well below the unfixed leak". Three earlier review rounds on this PR flagged exactly those concerns (ASAN bounds sitting above the shrunken leak signals, plus a stale comment); all three are now fixed and verified by the author with simulated-leak measurements under debug+ASAN (+278 MiB vs 128 bound; +76 MiB vs 24 bound). Every threshold in the diff now demonstrably sits between measured noise and the guarded regression's signal.
Other factors
The PR body is unusually thorough, with per-platform measurement tables and cross-references to the two overlapping PRs (#36981 closed as superseded, #33988's Windows scan used to size the 20 MiB allowance). The new ASAN_OPTIONS construction copies the file's existing idiom verbatim (five other call sites do the same append-and-strip-leading-colon). CodeRabbit's one finding (deprecated quarantine_size collision) was withdrawn after robobun showed the harness/CI never set it. No outstanding human reviewer comments. The bug hunting system found nothing on the current revision.
2 participants
What does this PR do?
test/js/web/fetch/fetch-leak.test.tswas flaky in 13 of the last ~80 main builds through two RSS-based checks. Neither is a leak; both compare an RSS delta against a threshold that sits inside the measurement's own noise. This is the "adjust a memory threshold" kind of change, so the numbers:1.
Request body HiveRef pool returns slot via Body.Value.deinit (does not leak) > String(Windows arm64, alpine x64)Each
cycle()builds 512 Requests with unique 128 KiB string bodies — 64 MiB live at once, deliberately >256 to hit both the hive and the fallback path — drops them and callsBun.gc(true). The test takesrss()after 8 warm-up cycles and again after 32 more and requires the delta ≤ 64 MiB. But memory freed bygc()isn't necessarily back with the OS whenrss()is read (decommit is lazy/asynchronous), so each sample carries up to one cycle's live set of slack. On a current release build, same script, no leak:The noise amplitude with 128 KiB bodies is ~70 MiB in either direction — the CI failures (+68, +74) are the same thing with the signs the other way round — and the threshold is 64. Rather than raise the threshold, shrink the bodies to 16 KiB: one cycle is then 8 MiB (noise ≤ ~5 MiB observed), a genuine "deinit() skipped" leak is still 32 × 512 × 16 KiB = 256 MiB, and the 64 MiB threshold now sits well clear of both. Request count and structure are unchanged.
The ASAN bound scales down alongside the signal, 320 to 128 MiB (review catch: 320 would sit above the now-256 MiB leak signature, making the ASAN lane unable to fail). The child's quarantine is capped at 32 MB, the same idiom the readable-stream test already uses, so quarantine retention cannot approach the bound. Under the debug ASAN build, clean runs measure deltaMB 5-7; a simulated leak (Requests retained instead of dropped) measures +278 MiB, comfortably past 128.
2.
should not leak using readable stream(alpine x64, debian aarch64, Windows x64)The fixture fetches a 128 KiB body 500 times through
getReader()and allows RSS to growMAX_MEMORY_INCREASE= 5 MiB over the second half. The regression it was added for (#23697, handing the whole response buffer to the stream asowned_and_done) retained the body per request — ~32 MiB over those 250 iterations. A non-leaking run's RSS still drifts over that window (allocator high-water / lazily-freed pages): +1–2 MiB on idle Linux, ~+2 MiB loaded, and every CI failure was at +5.05–5.1. Sampling every 250 iterations out to 4000 shows it plateauing (…51.8 → 55.9 → 55.9 → 55.9 → 56.2), i.e. warm-up, not growth. The allowance moves from 5 to 20 MiB: the Buildkite scan in #33988 saw non-leak growth of 6 to 14 MiB on Windows lanes over this same window, so 20 keeps headroom over the worst observation while staying well under the 32 MiB the guarded regression produces. (macOS runs usememoryFootprint, which stays flat at ~8.5 MiB throughout, which is why darwin never hit this.)The ASAN branch gets the same treatment as the HiveRef bound (review catch): 64 sat above the ~32 MiB signal. The capped quarantine saturates before the sample point, so the measured window is clean of quarantine growth; under the debug ASAN build, clean runs drift 1-3 MiB and a simulated regression (every read chunk retained) measures +76 MiB. The allowance there is 24 MiB.
Overlapping PRs
MIMALLOC_PURGE_DELAY=0; its diagnosis (mimalloc's purge delay leaves one cycle's freed working set committed, producing the flat 68/74 MB offsets) matches the table above and confirms the body-shrink math. Superseded by this PR, closed.How did you verify your code works?
Measurements above (release binaries, macOS arm64 + Linux x64).
bun bd test test/js/web/fetch/fetch-leak.test.tspasses under debug+ASAN: the HiveRef test holds at deltaMB 5-7 across repeated runs (simulated leak +278 MiB against the 128 MiB bound), the readable-stream fixture at 1-3 MiB across repeated runs (simulated leak +76 MiB against the 24 MiB bound).no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.