Repository navigation
Conversation
4x fewer requests (16,384 -> 4,096) with the same leak signal: the fixture now exposes a GC-then-report RSS endpoint, and the test compares post-GC RSS after a 1024-request warmup against the final sample instead of checking absolute RSS against an isASAN-branched threshold. A leaked 15 KB req.url over the 3072 requests in between is ~45 MB; on a fixed build the samples are flat within a few MB. Also pipes stderr and asserts it is empty, drops the 1s RSS setInterval noise from the fixture, and fails fast if the fixture exits before sending its URL.
|
Warning Review limit reached
Next review available in: 2 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 |
|
Updated 9:30 AM PT - Jul 26th, 2026
❌ @robobun, your commit f97a6cd has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35921That installs a local version of the PR into your bun-35921 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — well-reasoned test speedup that tightens the leak assertion rather than weakening it.
What was reviewed:
- Verified the growth math: samples[1] is after 1024 requests, samples[7] after 4096 → 3072×15 KB ≈ 45 MB leaked vs a 20 MB bound, matching the PR's empirical 48.8 MB reproduction.
- Checked that
proc.exited.then(reject)afterproc.kill()is a no-op on the already-settled withResolvers promise (no unhandled rejection). - Confirmed the fixture's only stdout is the one startup line, so piping without draining until after
kill()cannot deadlock on a full pipe. - Routing by
req.methodkeeps the GET handler free ofreq.urlaccess, preserving the #16787 scenario.
Extended reasoning...
Overview
Test-only change to test/js/bun/http/req-url-leak.test.ts and its fixture. Replaces an absolute-RSS ceiling (16 K requests, < 450/150 MB) with a post-GC RSS-growth assertion (4 K requests, < 20 MB), cutting runtime ~3-4×. The fixture gains a POST = double-GC + report-RSS endpoint; the leak-triggering GET path is unchanged aside from the method dispatch. Ancillary hardening: stderr piped and asserted empty, fast-fail if the fixture dies before IPC, dropped the 1 s setInterval log noise.
Security risks
None. Test-only, spawns a local Bun.serve({port: 0}) fixture, no external network, no production code touched.
Level of scrutiny
Moderate for a test rewrite of a leak regression guard — the risk is silently defanging the check. The PR description addresses this head-on: the author reintroduced the leak (pinned url strings) and observed 48.8 MB growth under ASAN vs the 20 MB bound, and observed ±3 MB flat on the fixed build. That satisfies REVIEW.md's "bound well below the unfixed leak" and "keep asserting the property the original assertion protected" rules. The switch from absolute RSS to post-GC delta is exactly the pattern the review guide recommends for leak tests, and it removes the per-variant isASAN threshold branch.
Other factors
proc.exited.then(reject)racingproc.kill(): harmless — the withResolvers promise is settled by the IPC message long before kill, so the later reject is a no-op.- Pipe safety: the fixture writes one stdout line at startup and nothing more (setInterval removed), so not draining stdout/stderr until after kill cannot hit the ~64 KB buffer.
samples[1]as baseline (after 1024 requests) is a sensible allocator-warmup skip; array always has 8 entries so indexing is safe.- Timeout tightened to 30 s on ASAN/debug based on the CI lane's observed 15 s → ~4-5 s; the 98 s local figure is explicitly a capped-container outlier.
- No prior reviews or outstanding comments; CI build in progress.
|
CI build #82401 (f97a6cd): all 193 test jobs passed. The test ran on The only red job is Diff is green and ready for review. |
6 windows instead of 8 keeps a ~30 MB leak signal over a 15 MB bound (fixed build sits at ~5 MB) and lets the ASAN/debug timeout stay at the original 90 s on slow hosts; CI ASAN is ~2-3 s.
There was a problem hiding this comment.
LGTM — test-only speedup with a tighter, better-justified leak bound. One minor diagnosability nit inline (early-exit stderr), non-blocking.
Extended reasoning...
Overview
This PR touches only test/js/bun/http/req-url-leak.test.ts and its fixture. It swaps the #16787 regression test's absolute-RSS check (maxRSS < (isASAN ? 450 : 150) MB over 16,384 requests) for a post-GC RSS growth check (growth < 15 MB over 3,072 requests), sampling RSS via a new POST = Bun.gc(true) + report endpoint routed by method so the long-URL GET handler keeps the original scenario (no req.url access). It also pipes/asserts stderr, wires proc.exited → reject for fail-fast on fixture crash, drops the noisy 1s setInterval RSS log, and widens the timeout gate to isASAN || isDebug.
Security risks
None. Test-only diff, no production code touched, no new network surface (fixture still binds port: 0 on localhost).
Level of scrutiny
Moderate — the REVIEW.md rule that matters here is "never silently weaken an existing test". The PR description makes the case explicitly and empirically: with the leak simulated (pinning URL strings), 2048 × 15 KB ≈ 30 MB of growth; without it, post-GC samples are flat within ~5 MB under ASAN. CI build #82361 confirms 4.7 MB growth on debian 13 x64-asan, so the 15 MB bound sits comfortably between fixed and broken with margin on both sides. This is a tightening, not a weakening — the old absolute bound had ~80 MB of ASAN slack and needed per-variant calibration.
Other factors
- Checked that the
req.method === "POST"branch runs before the GET path but does not touchreq.url, so the #16787 scenario (URL string retained without ever being read) is preserved. - Checked the happy-path
proc.exited.then(reject)race: by the timeproc.kill()fires, the withResolvers promise is already resolved with the URL, so the laterejectis a no-op. - Checked
samples[1]vssamples.at(-1)indexing: 6 windows → indices 0-5, delta spans windows 1→5 = 2048 requests, matching the 30 MB leak-case arithmetic. - Pipes are drained concurrently with
proc.exitedon the happy path per repo convention. - The one inline nit (stderr not drained on the early-exit reject path) is a small diagnosability regression on a ~never-hit path; the test still fails fast with a clear message, which is strictly better than the old timeout-hang.
The stderr pipe is now started once up front and awaited on both the early-exit reject path and the happy-path assertion, so a fixture startup crash surfaces its stack trace in the test failure instead of only the exit code.
There was a problem hiding this comment.
LGTM — the earlier stderr-on-early-exit nit is addressed in f97a6cd.
What was reviewed
- Growth-delta methodology: warmup baseline at
samples[1], 15 MB bound sits between the ~30 MB leak signal and ~5 MB observed noise; CI run confirms 4.7 MB on ASAN. - Pipe handling:
stderr.text()started once up front and awaited on both the early-exit reject and the happy-path assertion; stdout drained afterkill(); latereject()after IPC resolve is a no-op on the settled promise. - Fixture: routing by
req.methodleaves the long-URL GET path's body unchanged (noreq.urlaccess);process.send?.preserves standalone-run behavior.
Extended reasoning...
Overview
Test-only change to test/js/bun/http/req-url-leak.test.ts and its fixture. Replaces a 16,384-request absolute-RSS threshold (< 450/150 MB branched on ASAN) with a 3,072-request post-GC RSS growth delta (< 15 MB, no per-variant branching). The fixture gains a POST → Bun.gc(true) + report-RSS endpoint routed by method so the GET handler body is unchanged from the #16787 repro. Also: stderr piped and asserted empty, stdout/stdin no longer inherited, fail-fast reject if the fixture dies before IPC, and the noisy 1 s setInterval log removed.
Security risks
None. No production code touched; the fixture serves on port: 0 to localhost only.
Level of scrutiny
Low-to-medium. This is a leak-regression test rewrite, so the key question is whether the new assertion still fails on the original bug. The PR description states the author verified linear growth by pinning URL strings in the fixture under ASAN, and the arithmetic checks out: 2048 requests × 15 KB ≈ 30 MB of growth vs a 15 MB bound, with observed fixed-build growth of 2.9–4.7 MB across debug/ASAN and CI. That is a tighter signal-to-bound ratio than the old absolute check's ~80 MB of slack. The warmup baseline (samples[1], after 1024 requests) follows the repo's leak-test guidance about measuring after a warmup window.
Other factors
- My prior review's only finding (stderr discarded on the early-exit reject path) was addressed:
proc.stderr.text()is now started once and awaited in both the reject message and the happy-pathexpect. The thread is resolved. - Checked that the post-resolve
reject()(fired whenproc.exitedsettles afterproc.kill()) is harmless — the withResolvers promise is already settled by the IPCresolve, so it's a no-op and cannot produce an unhandled rejection. - Checked pipe backpressure: stdout receives a single "Server started on …" line (well under 64 KB) before being drained at the end; stderr is drained continuously from line 18.
- The fixture now evaluates
req.methodon every request before falling through to the GET path; that's a distinct accessor fromreq.urland doesn't materialize the URL string, so the #16787 scenario is preserved. - CI build #82361 ran the test in 3.5 s on
debian 13 x64-asan(down from ~15 s) with 4.7 MB growth; the only red job (binary-size) is unrelated to a test-only diff.
This branch has not been deployed
1 participant
What
Speeds up
test/js/bun/http/req-url-leak.test.tsby ~5× without weakening (actually tightening) the leak check.Why this is correct
The original test sends 16,384 GETs with a 15 KB URL and asserts
maxRSS < (isASAN ? 450 : 150) MB. That absolute-RSS threshold has to be wide enough to cover ASAN quarantine/shadow memory and future baseline drift, so it needs a lot of requests to push a real leak clearly past it.Asserting post-GC RSS growth instead removes the platform-dependent baseline from the equation. The fixture now exposes a
POST=Bun.gc(true)+ report-RSS endpoint (routed by method so the long-URLGEThandler stays byte-for-byte the #16787 scenario: noreq.urlaccess, just await-a-microtask + reply). The test samples RSS after GC every 512 requests and compares the final sample against the one after the first 1024 (allocator warmed up):So a
< 15 MBgrowth bound sits comfortably between the two at a fifth of the requests. It also drops theisASANthreshold branching, since growth is comparable across build variants.Timing
maxRSS < (isASAN ? 450 : 150) MBstderr == "",growth < 15 MBCI
debian 13 x64-asanon build #82361 (at 4096 req): 3.5 s vs ~15 s before; 3072 req should land around 2-3 s there. ASAN/debug timeout stays at the original 90 s.Assertion improvements
stderris now piped and asserted empty (was"inherit", so a fixture warning/crash only showed in CI logs)stdout/stdinno longer inheritedproc.exited.then(reject)fails fast with a clear message if the fixture dies before IPCing its URL, instead of hanging until the timeoutsetIntervalRSS log is gone (was pure noise in test output)[stamp-90s] gate passed · iteration 1 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file