Repository navigation
Conversation
…rowth
The "aborting in-flight streaming fetch() responses does not retain the
buffered body off-heap" test has been flaking on 86 of the last 399
Buildkite builds, almost entirely on darwin (14/26 x64+aarch64):
LEAK: RSS grew 57.8MB over 60 aborts (> 55MB)
Observed values 55.1-69.8 MB, median 59.1, against the 55 MB threshold.
The retained ByteStream buffer per iteration is bounded by one recv()
(LIBUS_RECV_BUFFER_LENGTH = 512 KB) plus the kernel socket receive
buffer, so the leak signature scales with the platform's SO_RCVBUF.
Measured on the darwin CI hosts, the 60-iteration RSS growth is
43-58 MB fixed vs 75-81 MB unfixed: the 55 MB threshold sits inside
the noise band and the separation is too small to be reliable.
Both fixed and unfixed builds grow the JS heap linearly in the held
Response/Reader objects (~0.28 MB/iter); subtracting that leaves an
off-heap component that is flat (~25-42 MB) on a fixed build at any
iteration count, and grows per iteration (~0.6 MB) on an unfixed one.
At 120 iterations:
darwin aarch64 fixed off-heap 24.3-42.3 MB (n=20)
darwin x64 fixed off-heap 18.1-29.2 MB (n=10)
linux x64 fixed off-heap 39.1-41.2 MB (n=5)
darwin aarch64 unfixed off-heap 104.0-112.6 MB (n=5)
Gate on off-heap growth at 120 iterations with a 64 MB threshold.
Runtime goes from ~0.5 s to ~1.0 s release, ~3 s debug+ASAN.
|
Warning Review limit reached
Next review available in: 15 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 (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — well-measured test de-flake that isolates the off-heap signal instead of raw RSS.
What was reviewed:
- Fixture and its sole caller updated consistently (env var rename, iteration count,
held=assertion); no staleMAX_GROWTH_MBreferences remain. - The new metric still asserts the original property (ByteStream buffer not retained off-heap) and the PR verifies it fails on a pre-#32662 build, so the regression guard is preserved.
- Threshold (64 MB) sits between the measured fixed max (42.3 MB) and unfixed min (104 MB) with wide margins on both sides; wall time stays ~1s release / ~3s ASAN.
Extended reasoning...
Overview
Test-only de-flake of fetch-leak.test.ts "aborting in-flight streaming fetch() responses does not retain the buffered body off-heap" and its fixture fetch-abort-stream-leak-fixture.ts. The change switches the leak gate from raw RSS growth to RSS growth − JS-heap growth, doubles iterations (60→120), and moves the threshold from 55 MB (inside darwin's noise band) to 64 MB (between measured fixed 24-42 MB and unfixed 104-113 MB). No production code is touched.
Security risks
None. Test fixture only; no auth, crypto, or user-facing code paths.
Level of scrutiny
Low-to-medium. It's a leak-test threshold adjustment, and the primary REVIEW.md concern for de-flakes is whether the property the original assertion protected is still asserted. It is: the fixture still holds every Response/reader reachable and gates on off-heap growth, which is exactly where the #32659 leak lived. The PR description shows the new metric was validated against a pre-fix canary (5/5 fail) and 20/20 pass on the flakiest lane, satisfying the "bound well below the unfixed leak" and "still fails when the fix is reverted" rules.
Other factors
- Verified there are no other consumers of the fixture or the renamed
MAX_GROWTH_MBenv var; the rename is complete. heap0is captured at the same point asrss0(after the initialBun.gc(true)), so the subtraction is apples-to-apples.- Doubling iterations keeps wall time within the ~1s-per-test budget (~1.0s release, ~3s debug+ASAN per the description).
- The comment in the fixture documents the mechanism (why subtraction isolates the signal), which is durable non-obvious content per the repo's comment guidance.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #33988. That PR widens thresholds on three different tests in this file ( Of the 124 fetch-leak.test.ts flaky occurrences in the last 399 builds, 86 are the test this PR targets and 12 are the tests #33988 targets. |
|
Updated 4:32 PM PT - Jul 27th, 2026
❌ @robobun, your commit 398930d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36148That installs a local version of the PR into your bun-36148 --bun |
There was a problem hiding this comment.
LGTM — test-only de-flake that tightens the leak metric rather than just widening a threshold.
What was reviewed:
- Metric change (RSS − JS-heap growth) correctly isolates the off-heap ByteStream buffer the original test was written to catch; PR verified 5/5 failures on a pre-#32662 canary, so the assertion still protects the same property.
- Env var rename
MAX_GROWTH_MB→MAX_OFFHEAP_MBis consistent between fixture and test; no other references in the repo. - 64 MB threshold sits well between measured fixed (≤42.3 MB) and unfixed (≥104 MB) distributions per the PR's CI-host measurements.
Extended reasoning...
Overview
This PR de-flakes the "aborting in-flight streaming fetch() responses does not retain the buffered body off-heap" test in test/js/web/fetch/fetch-leak.test.ts and its fixture fetch-abort-stream-leak-fixture.ts. The test has flaked on 86/399 recent Buildkite builds (mostly darwin) because the 55 MB raw-RSS threshold sits inside darwin's noise band. The fix changes the gating metric from raw RSS growth to RSS growth − JS-heap growth, doubles iterations to 120, and sets a 64 MB threshold with a documented ~60 MB gap between fixed and unfixed measurements.
Security risks
None. Test-only change; no runtime code touched.
Level of scrutiny
Low-to-medium. This is a leak-test threshold adjustment, and the REVIEW.md guidance for de-flaking is explicit: "keep asserting the property the original assertion protected — branch per-platform rather than dropping precision." This PR does better than that — instead of widening or per-platform branching, it refines the metric to remove a confounding variable (JS-heap growth from held Response/reader objects, identical on fixed and unfixed builds), then re-derives a threshold with empirical measurements on the actual CI hosts. The PR description confirms the new gate still fails 5/5 on a pre-fix canary, so the regression it guards against is still caught.
Other factors
- The env var rename is fully contained (grep confirms only the two touched files reference it).
heapStats().heapSizeis already imported and used elsewhere in the file for the baseline, so the API is known-good.- Wall time increases from ~0.5s to ~1.0s release / ~3s debug+ASAN, which is acceptable for a leak test and well within the file's existing budget.
- The comments added to both files explain the why (mechanism + measured bounds) rather than restating the code, matching the repo's comment guidance.
- No outstanding reviewer comments; CodeRabbit was rate-limited and did not review.
|
Diff is green. The changed test ("aborting in-flight streaming fetch() responses does not retain the buffered body off-heap") passed on every lane in both build 83623 and build 83637. Remaining CI red is unrelated to this change:
Ready for review. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-27, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
test/js/web/fetch/fetch-leak.test.ts"aborting in-flight streaming fetch() responses does not retain the buffered body off-heap" (added in #32662) has flaked on 86 of the last 399 Buildkite builds, almost all on the darwin lanes:86 observed values span 55.1 to 69.8 MB, median 59.1, against a 55 MB threshold. The sibling behavioural test ("discards the buffered body and errors the reader") is unaffected.
Cause
The leaked datum is the native
ByteStreambuffer, whose per-iteration size is bounded by onerecv()(LIBUS_RECV_BUFFER_LENGTH= 512 KB) plus the kernel socket receive buffer. The fixture therefore retains a platform-dependent amount per iteration, and on the darwin CI hosts the 60-iteration RSS growth comes out as 43-58 MB fixed vs 75-81 MB unfixed. The 55 MB threshold was derived from a Windows measurement (32.8 MB noise) and sits inside darwin's noise band; there is no value between the two distributions that separates them reliably.Both fixed and unfixed builds grow the JS heap linearly in the held
Response/reader objects (~0.28 MB/iteration, identical on both). Subtracting JS-heap growth from RSS growth isolates the off-heap component, which is flat on a fixed build at any iteration count (allocator and transport overhead only) and grows per iteration on an unfixed one (each retainedByteStreambuffer).Fix
Gate on
RSS growth - JS-heap growthat 120 iterations with a 64 MB threshold. Measured on the CI hosts:The highest fixed value (42.3 MB) is 21 MB below the threshold; the lowest unfixed (104.0 MB) is 40 MB above. Runs under 6-way concurrent load on the darwin host stayed at 25.3-39.4 MB.
Verification
bun bd test test/js/web/fetch/fetch-leak.test.ts -t "aborting in-flight streaming"passes locally. 20/20 fixture runs pass on darwin aarch64 with the latest canary; 5/5 fail with a pre-#32662 canary. Wall time goes from ~0.5 s to ~1.0 s release, ~3 s debug+ASAN.no test proof · iteration 1 · Platform-specific test-only change; deferring to CI.