test(fetch-leak): pin mimalloc purge state in the HiveRef Request-body test - #36981
test(fetch-leak): pin mimalloc purge state in the HiveRef Request-body test#36981robobun wants to merge 1 commit into
Conversation
|
Status: ready for review. Reproduced the flake signature in Buildkite annotations for builds 88734, 88735, 88783, 88819, 89090, 89215, 89250 (68-74 MB against the 64 MB bound, every one green on retry), then reproduced the mechanism locally: a 300ms stall before the baseline read turns the delta into a flat 61-85 MB step (mimalloc purge-state mismatch between the two rss() reads), and MIMALLOC_PURGE_DELAY=0 pins it at 0 in every run. The bound stays at 64 MB. CI: builds 89261 and 89296 both came back 195/196 jobs green with fetch-leak.test.ts passing on every lane. The one failed job in each (debian 13 x64-asan) is a pre-existing AsyncLocalStorage-tracking.test.ts leak report that also fails on main and is unrelated to this change. |
|
Warning Review limit reached
Next review available in: 1 minute 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 request-body leak test now documents observed post-GC RSS values and increases the non-ASAN RSS threshold from 64 MB to 128 MB. The ASAN threshold remains 320 MB. ChangesFetch leak test
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only threshold bump backed by cited CI flakes.
What was reviewed:
- The new 128 MB bound clears the observed 68-74 MB noise band while staying ~16× below the ~2 GiB signal a real skipped-deinit leak would produce, so the invariant the test guards is still asserted.
- ASAN branch (320 MB) is unchanged; the updated comment records the CI lanes and observed noise band per the leak-test guidance in REVIEW.md.
- No production code touched; only the
Stringvariant is affected (the only one in the loop).
Extended reasoning...
Overview
This PR touches a single test file, test/js/web/fetch/fetch-leak.test.ts, changing one integer constant (the non-ASAN RSS-delta threshold in the "Request body HiveRef pool returns slot via Body.Value.deinit" test) from 64 MB to 128 MB, and updating the adjacent comment to record why. No runtime, native, or built-in JS code is modified.
Security risks
None. This is a test-only threshold adjustment with no bearing on production code paths, auth, crypto, or user-facing behavior.
Level of scrutiny
Low. The change is mechanical and self-contained: a leak-test RSS bound is being widened to absorb documented CI allocator noise. The key review question per REVIEW.md's de-flaking guidance — "keep asserting the property the original assertion protected" — is satisfied: the test guards against a skipped Body.Value.deinit() on hive-pool return, which the test's own comment quantifies as ~2 GiB of retained bodies (32 × 512 × 128 KiB). The new 128 MB bound is ~16× below that, so a real regression still fails hard. The bound also remains branched on isASAN as the leak-test guidance requires.
Other factors
- The PR description cites seven specific Buildkite builds (88734, 88735, 88783, 88819, 89090, 89215, 89250) across four platforms where the flake reproduced at 68-74 MB and went green on retry, giving concrete evidence this is allocator high-water noise on loaded runners rather than a masked leak.
- The rationale is sound: each cycle holds 64 MiB of live bodies simultaneously (512 × 128 KiB), so a 64 MB post-GC RSS delta was sitting right at the working-set size with zero headroom. 128 MB gives ~73% headroom over the worst observed noise.
- The comment update records the observed noise band and affected lanes, which is exactly what REVIEW.md asks for when adjusting leak thresholds.
- No prior reviewer comments to address; no CODEOWNER-gated paths.
…y test The String case flaked on loaded CI lanes with 68-74 MB against its 64 MB RSS bound, always green on retry. The overage is not growth: Bun.gc() runs mimalloc's collect before the JSC sweep, so the 64 MiB of bodies a measurement GC frees stays committed until a purge (default delay 100ms) that the synchronous cycle loop may never reach, and whether each rss() read lands on purged or unpurged state produces a flat one-working-set offset between baseline and final. Set MIMALLOC_PURGE_DELAY=0 for the spawned process so frees decommit eagerly and rss() measures live memory: the delta settles at 0 on every run, including with a simulated 300ms descheduling before the baseline read, which reliably reproduced the 61-85 MB CI band without the env var. Purge only affects freed pages, so a skipped Body.Value.deinit still presents at full size (retaining 4 of the 32 cycles measures +260 MB). The 64 MB bound is unchanged.
d01afb8 to
f859853
Compare
robobun
changed the title
|
Note for reviewers: the walkthrough and pre-merge warnings above describe the first revision of this PR (d01afb8, which raised the bound to 128 MB). The current revision (f859853) does not change the threshold; it keeps the 64 MB bound and adds MIMALLOC_PURGE_DELAY=0 to the spawned process env, as the PR description says. The summary was generated while the incremental review was rate limited, so it reflects the old diff. |
|
@coderabbitai review |
|
There was a problem hiding this comment.
LGTM — test-only de-flake that pins mimalloc purge timing instead of widening the threshold.
What was reviewed
- The 64 MB non-ASAN bound is unchanged, so the assertion the test protects is not weakened.
{ ...bunEnv, MIMALLOC_PURGE_DELAY: "0" }matches the harness convention and the same pattern already used intest/napi/napi.test.tsandtest/cli/install/bun-create.test.ts.- Purge only decommits freed pages, so a real
Body.Value.deinitskip would still keep the ~2 GiB of retained bodies resident and blow past 64 MB — the env var cannot mask the regression.
Extended reasoning...
Overview
This PR touches a single test file, test/js/web/fetch/fetch-leak.test.ts, adding MIMALLOC_PURGE_DELAY: "0" to the env of the spawned subprocess in the "Request body HiveRef pool returns slot via Body.Value.deinit" test, plus an explanatory comment. No runtime code is changed and the leak threshold stays at 64 MB (320 MB under ASAN).
Security risks
None. This is a test-only environment-variable change to a spawned child process. No auth, crypto, or user-facing surface is touched.
Level of scrutiny
Low. The change is a targeted de-flake of a leak-detection test. The key REVIEW.md concern for de-flaking — "keep asserting the property the original assertion protected" — is satisfied: the bound is unchanged, and the fix operates at the allocator layer (purge only decommits freed pages), so a genuine leak where bodies are never freed would still manifest at full size (~2 GiB against a 64 MB bound). This is the same style of allocator-layer fix the file already uses for ASAN quarantine (quarantine_size_mb).
Other factors
bunEnvis correctly spread before the override, per harness convention.MIMALLOC_PURGE_DELAYis an established pattern in this repo (test/napi/napi.test.ts:608,622,test/cli/install/bun-create.test.ts:457), so the vendored mimalloc is known to honor it.- The PR description documents a deterministic local repro (300ms stall before the baseline read → flat 61-85 MB offset) and verification that the env var pins delta to 0 on release and 7 MB on debug+ASAN.
- The earlier revision that raised the bound to 128 MB was replaced with this measurement fix, which is the right call per the review guide (fix the measurement, don't widen thresholds over allocator behavior).
- No prior reviewer comments to address; no CODEOWNERS on test files.
|
Superseded by #37209, which fixes the same HiveRef flake by shrinking the per-Request bodies to 16 KiB so the purge-timing offset this PR diagnosed (one cycle of freed pages still committed) drops to 8 MiB against the 64 MiB bound. The diagnosis here is what confirmed that margin, and #37209 credits it. Closing. |
1 participant
What does this PR do?
The "Request body HiveRef pool returns slot via Body.Value.deinit (does not leak)" String case in
test/js/web/fetch/fetch-leak.test.tsgates on a 64 MB RSS delta and has been flaking on loaded CI lanes with marginal overages, always going green on retry:Observed in the last ~550 Buildkite builds: 88734 (macOS 13 x64, 74 MB), 88735 (Ubuntu 25.04 x64, 74 MB), 88783/88819/89090/89250 (Windows 11 aarch64, 68 MB), 89215 (Alpine 3.23 x64, 74 MB).
Cause
The overage is not memory growth.
Bun.gc()runs mimalloc's collect before the JSC sweep (garbage_collectin VirtualMachine.rs), so the 64 MiB of bodies each measurement GC frees stays committed until a later purge, and mimalloc's purge delay defaults to 100ms, which the test's synchronous cycle loop may never service. Whether the baseline and finalrss()reads each land on purged or unpurged state is timing dependent, and the mismatch shows up as a flat offset of about one cycle's working set, not as a ramp:MIMALLOC_PURGE_DELAY=0, delta is -2 to 0 in every run, with or without the sleepThis also explains why the CI values cluster at exactly 68 and 74 MB per platform instead of varying with load.
Fix
Set
MIMALLOC_PURGE_DELAY=0in the spawned process env so frees decommit eagerly andrss()measures live memory. The 64 MB bound is unchanged. This mirrors the allocator-layer fix the same file already uses for ASAN retention (cappingquarantine_size_mbin the readable-stream test) rather than widening thresholds over allocator behavior.The env var cannot mask the regression the test guards: purge only decommits freed pages, and a skipped
Body.Value.deinitkeeps bodies resident. Retaining just 4 of the 32 cycles measures +260 MB against the 64 MB bound; the full leak is ~2 GiB.An earlier revision of this PR raised the bound to 128 MB instead; review of the mechanism showed the noise band is purge timing, not an irreducible high-water, so this revision fixes the measurement and keeps the bound where it was.
How did you verify your code works?
USE_SYSTEM_BUN=1 bun test test/js/web/fetch/fetch-leak.test.ts -t 'Request body'passes 3/3 with deltaMB 0 (baseline RSS drops from ~100 MB to ~39 MB now that it reflects live memory)bun bd test(debug+ASAN) passes with deltaMB 7 against the unchanged 320 MB ASAN boundMIMALLOC_*via getenv on unix and windows;MIMALLOC_VERBOSE=1showsoption 'purge_delay': 0)no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.