Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesThe HTTP body leak tests now use settled memory measurements, fully drain streaming bodies, share HTTP/1 and HTTP/2 scenarios, and verify intentional body retention with a Suggested reviewers: HTTP body leak testing
Merge Risk: ⚪ Minimal · up to This test-only change reduces runtime and makes memory-leak assertions more stable while preserving positive-control coverage; 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 clearly explains the problem, implementation, verification results, background, and scope. It provides the required change summary and verification information, although it uses custom headings instead of the template headings. Comment |
|
Status: test-only change, rebased onto #40137 (which added an HTTP/2 pass to this file), ready for review. Supersedes #35265 (closed). CI: How the timing was checked, on the same machine and build each time:
Leak-free scenarios measured between -21 and 10 MB of growth across all of these runs (threshold 32 MB, 64 MB on debug without ASAN). The Self-review: the fixture keeps the settle helper (Bun.shrink() deletes compiled code, so it is for a measurement child, not the test runner), the ASAN fixture gets the 32 MB quarantine cap that fetch-leak.test.ts already uses, and the thresholds were tightened accordingly. |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites the leak-measurement methodology (10× fewer requests, new settled-RSS sampling via Bun.shrink(), tightened thresholds) for a test that guards several past body-leak regressions, and because leak thresholds are platform-sensitive (verified on Linux and Windows x64 but not macOS or Windows aarch64), a human look and a full CI pass across all lanes would still be worthwhile. It also conflicts with #35265, so a maintainer should pick a direction.
Checked: rss is exported from harness and resolvable from the fixture subprocess; Bun.shrink() exists (BunObject.rs:1089); count / checkpoints is integral for all three counts; the /retain control runs last so its retained bodies don't affect earlier scenarios' endMB assertions; all seven original scenarios remain and the release growth threshold is tightened (64→32 MB), not loosened.
Extended reasoning...
Overview
Test-only change to test/js/bun/http/serve-body-leak.test.ts and its fixture. Replaces a fixed 50 ms post-GC RSS sample with a Bun.gc(true) + Bun.shrink() + yield loop that repeats until the reading stops falling, cuts per-scenario request volume from 10 000/3 000 to 960/400, restructures scenarios into a data table, adds status/body assertions on every response, and adds a /retain positive control that deliberately leaks bodies to prove the measurement flags it. Thresholds are re-derived: growth ≤ 32 MB (release) / 64 MB (ASAN), absolute RSS ≤ 256 MB (release) / 768 MB (ASAN|debug).
Security risks
None. No production code touched; the fixture is a local Bun.serve({ port: 0 }) driven only by the test.
Level of scrutiny
Moderate-to-high for a test change. This test is the regression guard for #10265, #13704 and #14191 (body-sized leaks in Bun.serve). The rewrite reduces request volume ~10× and changes the measurement methodology and thresholds. The PR description shows careful root-cause analysis (mimalloc/JSC scavenger timing, ASAN quarantine drain) and empirical verification on Linux release, Linux debug+ASAN, and Windows x64 release, plus a positive control demonstrating an 11× (release) / 1.6× (ASAN) signal-to-threshold margin. However, leak tests are notoriously CI-machine-sensitive; macOS and Windows aarch64 (the lane cited as slowest) were not verified locally, so a full CI pass should be confirmed before merge.
Other factors
- All seven original scenarios are preserved; the release growth threshold is tightened (64→32 MB) and the absolute RSS cap tightened (512→256 MB), so per REVIEW.md this is not silently weakening a safety net.
- The positive control satisfies REVIEW.md's "prove the test fails for the right reason" — it asserts the same measurement flags a real body-sized leak.
- Verified
rssexport intest/harness.ts:2278(the fixture's inline darwin branch is now shared),Bun.shrink()insrc/runtime/api/BunObject.rs:1089, and that other fixtures in the same directory already import fromharnessin a spawned subprocess. requestCount / checkpointsand480 / checkpointsare all integers, sosendRequestsreceives whole counts.- The PR explicitly conflicts with open PR #35265 (a competing de-flake of the same file); a maintainer should decide which to land.
|
Windows 11 aarch64 is now verified too (canary 1.4.1, same machine for both runs): 54.8 s before, 3.9 s after over 3 runs. Every leak-free sample read 36 to 37 MB and the |
|
Updated 1:34 PM PT - Aug 27th, 2026
❌ @robobun, your commit f8b2cdf has 3 failures in
🧪 To try this PR locally: bunx bun-pr 40088That installs a local version of the PR into your bun-40088 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it redesigns the leak-measurement methodology (settled-RSS loop, new empirically-tuned thresholds, 10× fewer requests) and explicitly conflicts with #35265's approach to the same flake, a human look would still be worthwhile — particularly to confirm the macOS CI lane and pick between the two approaches.
What was reviewed:
- All seven original scenarios are preserved with the same routes; thresholds tightened (64→32 MB release) rather than loosened, and a
/retainpositive control now proves the measurement still flags a real body leak. rssfrom harness and theASAN_OPTIONS: quarantine_size_mb=32pattern match existing usage infetch-leak.test.tsand elsewhere.settledMemoryUsage()loop is bounded (10 iterations) and yields viasetImmediatesoBun.shrink()'s deferred scavenge runs; fixture importingharnessmatches sibling fixtures in the same dir.
Extended reasoning...
Overview
Test-only rewrite of test/js/bun/http/serve-body-leak.test.ts and its fixture. Replaces the "gc + sleep(50) + read RSS" sample with a settled-RSS loop (Bun.gc(true) + Bun.shrink() + yield until the reading stops falling), cuts per-scenario request volume from 10,000/3,000 to 960/320, restructures scenarios as data, adds response-body verification, caps the ASAN quarantine to 32 MB for the fixture, and adds a /retain positive-control route that the test asserts the measurement flags.
Security risks
None. Test-only change with no production code paths, no new external inputs, no auth/crypto.
Level of scrutiny
Medium-to-high. Leak tests are among the flakiest in the suite and their thresholds are per-platform empirical constants. REVIEW.md is explicit that de-flakes must keep asserting the original property and that RSS thresholds need careful isASAN/isDebug branching with the bound well below the unfixed leak. This PR does both — thresholds are tightened, all scenarios kept, and the positive control demonstrates the reduced count still separates a body-sized leak by ~6–11× on release. But the methodology change is substantial enough that it is not a mechanical edit.
Other factors
- The PR description states this conflicts with open PR #35265, which takes a different approach (median-baseline) to the same incomplete-streaming flake. A maintainer should choose between them.
- macOS was explicitly left to CI; the author verified Linux release, Linux debug+ASAN, Windows x64 and Windows aarch64 by hand. Given macOS uses the
memoryFootprintpath inharness.rss, its behavior with the new settled loop is unverified. - The
maxGrowthMB = isDebug && !isASAN ? 64 : 32branch andmaxRssMB = isASAN ? 512 : 256are new empirically-derived constants; the in-code comments justify them, but they are the kind of tuning that historically needs a CI round-trip before landing. - No bugs surfaced from the automated review; the fixture's
harnessimport matches sibling fixtures,Bun.shrink()is a real binding, and the ASAN_OPTIONS construction mirrorsfetch-leak.test.ts:80exactly.
There was a problem hiding this comment.
LGTM — test-only de-flake with a positive control proving the tightened threshold still catches the target leak class.
What was reviewed:
- Verified
Bun.shrink()is a real API (BunObject.rs:1089) andrssis exported from harness; other fixtures in this dir already import harness so path resolution works in spawned children. - Checked the
ASAN_OPTIONSquarantine cap matches the pattern infetch-leak.test.ts:753verbatim. - Confirmed the
/retainpositive control runs last so its retained bodies don't inflateendMBfor the leak-free scenarios, and that it asserts onlygrowthMB(notendMB). - Traced the sensitivity change: the historical leaks this file guards (#10265, #13704, #14191) are all body-sized (512 KB/request), which the new 32 MB threshold detects with 5-11× margin per the control's 180 MB reading.
Extended reasoning...
Overview
This PR rewrites test/js/bun/http/serve-body-leak.test.ts and its fixture to cut runtime from 26-55s down to 3-10s across platforms while removing a documented flake source (measurement noise from reading RSS before allocator scavengers return freed pages). The fixture now reports a settled RSS via Bun.gc(true) + Bun.shrink() + event-loop yield, iterated until the reading stabilizes. Request count drops from 10,000 to 960 (320 on ASAN/debug), thresholds tighten from 64 MB to 32 MB, and a new /retain positive-control route proves the measurement flags a body-sized leak (180+ MB observed vs 32 MB threshold on every verified build). No runtime code is touched.
Security risks
None. Test-only change to a leak regression test and its local fixture subprocess. No auth, crypto, network egress, or user-facing surface.
Level of scrutiny
Moderate. The concern with leak-test rewrites is silently weakening a safety net (REVIEW.md: "Never silently weaken... an existing test"). This PR addresses that head-on: (1) the positive control asserts the same measurement flags the exact leak class the scenarios guard; (2) the PR body enumerates the historical leaks (#10265, #13704, #14191) and shows they are all 512 KB/request, which the new threshold catches with wide margin; (3) the old test was already flaking at its 64 MB threshold with ±50 MB noise, so its effective sensitivity was no better. The de-flake keeps asserting the protected property while removing noise at the source rather than papering over it with a median or looser bound.
Other factors
- Verified across Linux release/debug+ASAN/release-asan/debug-no-ASAN and Windows x64/aarch64 with per-run numbers in the PR body; macOS was deferred to CI (build #103740), and the
rssharness helper already handles darwin viaBun.unsafe.memoryFootprint. - Follows established repo patterns: the ASAN
quarantine_size_mb=32env construction is copied exactly fromfetch-leak.test.ts, and importingharnessfrom a spawned fixture is already done by two sibling fixtures in the same directory. - The
settledMemoryUsageloop is bounded (10 iterations), the checkpoint arithmetic (960/4, 320/4, 480/4) yields integers, andsendRequestshandles non-multiples ofconcurrencyviaMath.min. The/retaintest is declared last inside the describe so its retained memory can't affect earlier scenarios'endMBassertion, and the direct-stream abort test below the describe uses its own subprocesses. - Bug hunter found no issues; no prior human review comments to address; no CODEOWNER on test files.
The fixture now reports a settled RSS (Bun.gc(true) plus Bun.shrink(), repeated until the reading stops falling) instead of a reading taken 50 ms after Bun.gc(true), which still counted the previous batch's garbage. Under ASAN the fixture gets a 32 MB quarantine so RSS tracks live memory. Each scenario sends 960 requests (320 on ASAN/debug) with 4 checkpoints and a 32 MB growth threshold; every response is checked for status and body. A /retain route keeps every body alive as a positive control that the same measurement flags a body-sized leak. The HTTP/2 pass runs the same scenarios with the same counts.
ec571b1 to
f8b2cdf
Compare
|
Related: #41560 is a minimal change for the x64-asan failure of this file on main (ASAN quarantine and page release in the fixture). It does not change the request volume or the bounds. |
Problem
serve-body-leak.test.tstook 55 s on Windows 11 aarch64 and 26 to 47 s elsewhere (build #103422): 80,000 requests of 512 KB. Bun.serve: HTTP/2 support viahttp2: true#40137 then added an HTTP/2 pass, and main now takes 46 s on Linux release and 190 s on debug+ASAN here.Bun.gc(true), but freed memory leaves RSS only when background scavengers run, 100 ms or more later. Leak-free samples swung by 50 MB, and scenarios fail from that noise alone (streaming: leak 66 and 70 on Linux; incomplete-streaming: leak 75 on Linux, 84 on Windows).Fix
Bun.gc(true)plusBun.shrink(), repeated until the reading stops falling. Under ASAN the fixture gets a 32 MB quarantine, asfetch-leak.test.tsdoes. Leak-free samples now stay within 3 MB on every build./retain, keeps every body alive and asserts the same measurement flags it (180 MB on every build, over both protocols).bun bd test test/js/bun/http/serve-body-leak.test.ts189.8 s to 53.7 s. Before the HTTP/2 pass existed: Windows x64 41.7 s to 3.3 s, Windows 11 aarch64 54.8 s to 3.8 s, release-asan 33.5 s to 9.5 s. Supersedes test(serve-body-leak): warm up with incomplete-streaming and baseline against the median sample #35265.Background
Bun.shrink()is JSC'sshrinkFootprintWhenIdle: once the running JS returns to the event loop it runs a full GC, deletes compiled code, and scavenges synchronously. Release builds link JSC against Bun's mimalloc, so the scavenge reaches every freed page.Notes
Why the old samples were noisy. Probing the fixture after a batch with
Bun.gc(true)and polling RSS every 5 ms: the drop arrives anywhere between 10 and 100+ ms later, and sometimes not within 150 ms (incomplete-streaming settled at 72 MB vs 40 MB). WithBun.shrink()and one event-loop yield, the drop is in the first poll every time.Bun.gc(true)callsmi_collect(false)before the collection, so the memory that collection frees is left to the 100 ms purge delay of the mimalloc scavenger thread (vendor/mimalloc/src/scavenger.c); JSC-side memory waits for bmalloc's scavenger.VM::shrinkFootprintWhenIdle(vendor/WebKit/Source/JavaScriptCore/runtime/VM.cpp) runsdeleteAllCode,collectNow(Sync, Full)andWTF::releaseFastMallocFreeMemory()once the outermost entry scope pops, which thesetImmediateyield in the fixture guarantees. The release WebKit prebuilt is built withUSE_MIMALLOCandUSE_EXTERNAL_MIMALLOC, so that scavenge ismi_collect(true)on Bun's own mimalloc and purges Bun-native frees as well. The debug and ASAN prebuilts use libpas, so there it reaches the JSC side only.Why the helper stays in the fixture.
Bun.shrink()deletes all compiled code, so it is for a measurement child, not the test runner's VM. The fixture comment says so.Baseline runs before the rebase (main without the HTTP/2 pass, this machine). Linux release
USE_SYSTEM_BUN=1: 36.7 s, 7 pass 1 fail (incomplete-streaming, leak 75 > 64, samples[52, 80, 82, 103, 60, 66, 72, 56, 146]MB). Linux debug+ASANbun bd test: 99.7 s, 8 pass, end_memory up to 753 MB against the 768 MB bound. Linux release-asan: 33.5 s, 7 pass 1 fail (streaming, leak 70). Windows x64 release (canary 1.4.1): 41.7 s, 7 pass 1 fail (incomplete-streaming, leak 84). Windows 11 aarch64 release (canary 1.4.1): 54.8 s, 8 pass.Runs after the rebase onto #40137 (HTTP/1.1 and HTTP/2 passes, 17 tests). Main on this machine: Linux release 45.8 s with the streaming scenario failing at leak 66, debug+ASAN 189.8 s. This PR: Linux release 6.73 to 6.89 s over 3 runs, growth between -5 and 3 MB, controls 179 to 183 MB; debug+ASAN 53.7 s, growth between -21 and 7 MB, controls 185 MB (HTTP/1.1) and 169 MB (HTTP/2). The HTTP/2 fixture settles near 450 MB under debug+ASAN.
Runs before the rebase (HTTP/1.1 pass only, 9 tests). Linux release: 3.02 to 3.17 s over 5 runs, per-scenario growth between -1 and 2 MB, control 180 to 181 MB. Linux debug+ASAN: 25.5 s, growth between -1 and 10 MB, control 186 MB, RSS 408 to 415 MB. Linux release-asan: 9.5 s over 2 runs, growth between -2 and 5 MB, control 185 MB, RSS near 297 MB. Linux debug without ASAN (
bun bd --asan=off): 15.8 s over 2 runs, growth between -6 and 3 MB (one earlier run read 18 MB on one sample, see below), control 183 MB. Windows x64: 3.21 to 3.38 s over 3 runs, growth 0 to 1 MB, control 181 MB. Windows 11 aarch64: 3.73 to 3.85 s over 3 runs, growth 0 to 1 MB, control 180 MB.Leaks each scenario guards. #10268 (issue #10265) fixed a server that kept the whole body of a request the handler never read, and added the ignore, buffering, streaming, incomplete-streaming and echo scenarios. #13704 fixed a leak of the body buffered through
req.bodybeforereq.text()and added the body-getter scenario. #14191 added thereq.json()scenario. All of these are body-sized (512 KB per request) leaks; the/retaincontrol reproduces that class and measures 180 MB on every build, 1:1 with the retained bytes.ASAN. With the default 256 MB quarantine the settled readings oscillated by up to 27 MB (the quarantine recycles down to 90% when it fills) and a retained body showed up as 0.6 to 0.9 of its size while freed regions were released on the allocator's 5 s interval; the fixture plateaued near 600 MB. With
quarantine_size_mb=32(the captest/js/web/fetch/fetch-leak.test.tsalready uses for its fixtures) the readings stay within 3 MB, the control reads 185 MB, and the plateau is 300 MB (release-asan) or 410 MB (debug). The cap only shortens how long freed memory is held for use-after-free detection; the scenarios still run under ASAN.Debug without ASAN. JSC uses libpas there, so
Bun.shrink()does not purge Bun-native mimalloc frees and a sample can include up to a batch of freed body buffers (one run read +18 MB on one checkpoint). That build gets a 64 MB growth threshold.MIMALLOC_PURGE_DELAY=0for the fixture would remove the effect but costs about 24% of the fixture's throughput on release, where it is not needed, so it was not used.Scenarios. All seven routes exercise distinct body consumption paths (ignored,
text(),json(),bodygetter thentext(), reader drain, partial read, pipe to a Response), so none is a subset of another and none was dropped. Running scenarios concurrently would mix their RSS, and two fixtures only cut 10% (release) to 36% (debug) of the request time in a probe, so they stay sequential on one fixture. Batch size 16 to 80 made no difference to throughput.Unchanged. The direct-stream abort test keeps its
Bun.sleep(20)before exit: it is a one-time 20 ms in a child whose measurement is LeakSanitizer's exit report (50 ms on release, 4 to 5 s on ASAN for two child processes).Relation to #35265 and #39660. #35265 moves the warmup to incomplete-streaming and baselines against the median sample to absorb the same noise. This PR removes the noise at the source, warms every route, and rewrites the same lines, so the two conflict and this one supersedes it. #39660 appends LSan cases to this test file and merges clean with this PR.
[stamp-90s] gate passed · iteration 2 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file