Repository navigation
Conversation
request-constructor-leak-fixture: shrink the body from 16 MiB to 1 MiB. The Request constructor copies the body before the throwing signal coercion runs, so under ASAN the loop cost is O(body size). 1 MiB * 1000 still produces ~1 GiB of leak signature against the 1 GiB threshold while dropping the fixture from ~10s to ~1.5s on debug-ASAN. Also hoist the per-iteration .repeat() into a Buffer.alloc().toString() outside the loop. server-fetch-string-leak-fixture: detect ASAN via bun:internal-for-testing.isASANEnabled() instead of the binary name. A local bun bd debug build is ASAN-instrumented but named bun-debug, so the name check picked the non-ASAN branch and the fixture failed the 64 MB threshold with ~190 MB of quarantine growth. Double the path to 64 KiB and halve iterations so the total leak signature (~512 MiB under ASAN) is unchanged while wall time drops ~25%. response-constructor-leak-fixture: hoist .repeat() to Buffer.alloc(). leaks-test.test.ts: assert the fixture stdout actually prints the RSS line before checking the exit code, drop the blanket 60s timeouts on the two sub-second tests, and keep a 30s ceiling on the server.fetch test which legitimately exceeds the 5s default under debug-ASAN.
|
Warning Review limit reached
Next review available in: 9 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 (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ld margin 1 MiB * 1000 = 1000 MiB sat 24 MiB short of the 1024 MiB threshold on its own, leaving the release lane with ~0.6-3.5% margin once baseline RSS was added. 2 MiB * 1000 = 2000 MiB is ~2x the threshold while server.fetch remains the file's long pole, so wall time is unchanged.
There was a problem hiding this comment.
LGTM — thanks for the 2 MiB bump on the request fixture. The remaining inline note on server-fetch-string-leak-fixture.js is the same margin arithmetic applied to the sibling fixture; it's a nit (a full leak still clears 450 MB) and can be a one-line follow-up.
Extended reasoning...
Overview
Test-only PR touching test/js/bun/http/leaks-test.test.ts and its three leak fixtures. It (1) fixes a genuine local false-positive by replacing the bun-asan name check with bun:internal-for-testing.isASANEnabled() (matching harness.ts:56-62 exactly), (2) trades body/path size against iteration count to speed up debug-ASAN runs, (3) hoists .repeat() out of hot loops per test/CLAUDE.md guidance, and (4) adds stdout-shape assertions before .toSpawn() so an early crash surfaces as a readable failure rather than a bare exit-code mismatch.
Since the last review
My earlier comment flagged that the 1 MiB request body left the leak signature (1000 MiB) below the 1024 MiB threshold. The author bumped it to 2 MiB in bf87edb, restoring a ~2× margin (2000 MiB leak vs 1024 MiB threshold; measured no-leak RSS 69 MB release / 610 MB ASAN). That thread is resolved.
The one new inline finding this run applies the identical reasoning to the server.fetch fixture: halving iterations while doubling the path doesn't fully preserve the leak total because there's ~8 KB fixed per-call overhead, so the ASAN-side margin dropped from ~1.44× to ~1.14×. It's filed as a nit because a full URL-buffer leak still produces ≥512 MiB, which clears the unchanged 450 MB threshold — detection isn't broken, only the tolerance for partial/variance shrank. Given the author already accepted and fixed this exact pattern on the request fixture, I expect the same one-line bump here and don't think it needs to block.
Security risks
None. No runtime code touched; fixtures spawn Bun.serve({port: 0}) locally and never hit the network.
Level of scrutiny
Low-to-medium. Test infrastructure only, but leak fixtures are safety nets and REVIEW.md is explicit that RSS thresholds must sit well below the unfixed leak. I verified: bunRun trims stdout so the new ^RSS: \\d+ MB$ / ^RSS delta: -?\\d+(\\.\\d+)? MB$ anchors match the fixtures' console.log("RSS:", n, "MB") output; the stdout line is printed on both pass and fail paths so .toSpawn() remains the exit-code gate; isASANEnabled() exists in bun:internal-for-testing with the name-check fallback preserved for older comparison binaries.
Other factors
The response fixture's Buffer.alloc(64*1024, "yo de lay ") produces the same 64 KiB string as the old "...66 chars...".repeat(1024) (65536 vs 67584 bytes — close enough for a leak fixture whose threshold is 1 GiB). Dropping the 60 s timeout on the two now-sub-second tests is consistent with the "setDefaultTimeout is a ceiling, not a target" guidance; the 30 s ceiling on the server.fetch test is justified by the comment and the measured 5.5 s ASAN runtime.
… comment Replaced the ~512 MiB estimate with the ~649 MB value measured by reintroducing the Cow->Box::leak regression and running the fixture. The margin against the 450 MB threshold is the same 1.44x as the pre-PR 32KiB*8192 workload.
There was a problem hiding this comment.
LGTM — test-only speedup with both prior margin concerns resolved.
What was reviewed:
bunRuntrims stdout, so the new^RSS: \d+ MB$/^RSS delta: …$anchors match the fixtures'console.logoutput.- ASAN detection via
bun:internal-for-testing.isASANEnabled() mirrorsdetectASAN()intest/harness.ts:56. - Request-fixture body is now 2 MiB (bf87edb) → ~2 GiB leak vs 1 GiB threshold; server-fetch margin was empirically re-measured at ~649 MB vs 450 (1.44×, unchanged) and the comment updated in 0b5f6b1.
- Hoisted
Buffer.alloc().toString()strings preserve per-iteration uniqueness via+ i/+ Math.random().
Extended reasoning...
Overview
Test-only PR touching four files under test/js/bun/http/: the leaks-test.test.ts driver and its three subprocess fixtures. Goals are (1) fix a false-positive on local bun bd builds where the server.fetch fixture branched on process.execPath.includes("bun-asan") and missed debug-ASAN builds named bun-debug, and (2) cut wall time from ~11.7s → ~7.7s under debug-ASAN by shrinking the request-constructor body (16 → 2 MiB), doubling the server.fetch path while halving iterations, and hoisting .repeat() calls out of hot loops into one-time Buffer.alloc(n, fill).toString() per REVIEW.md's harness-conventions guidance. The driver now also asserts the fixture printed its RSS line before checking exit code, and drops blanket 60s timeouts on the two sub-second tests.
Security risks
None. No production code is touched; all changes are to leak-regression test fixtures and their driver. No network, no untrusted input parsing, no auth/crypto.
Level of scrutiny
Moderate — the REVIEW.md concern for leak tests is "never silently weaken an existing safety net" and "RSS thresholds with the bound well below the unfixed leak". Two prior review rounds examined exactly this:
- Request fixture: initial 1 MiB body gave a ~1000 MiB leak signature vs a 1024 MiB threshold (≈1.0×). Author bumped to 2 MiB in bf87edb → ~2000 MiB vs 1024 MiB (~2×), with measured no-leak RSS of 69 MB release / 610 MB debug-ASAN. Resolved.
- Server-fetch fixture: I estimated the halved-iteration/doubled-path change might drop the ASAN leak signature from ~648 → ~512 MB. Author reintroduced the actual regression (
Cow→Box::leak) and measured 645-652 MB across 8 runs — the per-call overhead scales with path size, so the FN margin is unchanged at 1.44× and FP margin improved. Comment updated in 0b5f6b1. Resolved.
I verified bunRun (test/harness.ts:521) returns stdout.trim(), so the new /^RSS: \d+ MB$/ and /^RSS delta: -?\d+(\.\d+)? MB$/ anchors match the fixtures' console.log("RSS:", n, "MB") output without a trailing-newline mismatch. The ASAN detection now matches detectASAN() in harness.ts:56-62 exactly (try isASANEnabled(), fall back to name check). Hoisting the long strings out of the loop is safe because each iteration still produces a unique URL (+ i) and header value (+ Math.random()), so per-iteration allocation behavior in the code under test is preserved.
Other factors
Both inline review threads are marked resolved with concrete follow-up commits. The bug-hunting system found nothing on the current revision. The one residual judgment call — dropping the 60s timeout on the ~2s request test to the 5s default — is backed by the author's debug-ASAN measurements and is trivially adjustable if CI proves otherwise. No thresholds were loosened; the only threshold-adjacent change (30s on server.fetch) is a reduction from 60s with a comment explaining why it exceeds the default.
There was a problem hiding this comment.
LGTM — both earlier margin concerns are addressed (request body bumped to 2 MiB in bf87edb; server.fetch leak signature empirically re-measured at ~649 MB vs the 450 MB threshold in 0b5f6b1), so no leak safety net was loosened.
What was reviewed:
- Request/response fixtures: leak-signature vs threshold margins after the body-size and
.repeat→Buffer.allocchanges. - server-fetch fixture: ASAN detection now mirrors
harness.ts'sdetectASAN(); iteration/path trade-off verified against the author's reintroduced-regression measurements. leaks-test.test.ts: new stdout regexes match the fixtures'console.logoutput afterbunRun's.trim(); timeout drops leave the debug-ASAN long pole covered.
Extended reasoning...
Overview
Test-only change to test/js/bun/http/leaks-test.test.ts and its three fixture scripts. No runtime code touched. The PR speeds the file from ~11.7s to ~7.7s under debug-ASAN, fixes a false-positive on local bun bd runs (ASAN was detected by binary name, missing bun-debug), and strengthens the test assertions to check fixture stdout before the exit-code .toSpawn().
Security risks
None. Only test fixtures and a test file are modified; no auth, crypto, network, or parsing logic.
Level of scrutiny
Moderate for a test-only PR, because these are leak-regression guards and REVIEW.md explicitly warns against silently weakening RSS thresholds. That's exactly what my two earlier inline comments checked. Both were addressed:
- Request fixture (bf87edb): body raised from 1 MiB to 2 MiB, giving a ~2000 MiB leak signature vs the unchanged 1024 MiB threshold (~2×). No-leak RSS measured at 69 MB release / ~610 MB debug-ASAN.
- server.fetch fixture (0b5f6b1): rather than adjusting parameters again, the author reintroduced the original
Cow → Box::leakregression and measured the actual leak delta at 645-652 MB across 8 runs vs the 450 MB threshold (1.44×, same as before) and 323-327 MB no-leak baseline (1.38× FP margin, improved). The fixture comment now records the measured number.
Other factors
- The ASAN-detection change is a straight copy of
test/harness.ts:56-62(require("bun:internal-for-testing").isASANEnabled()with a name-check fallback), so it stays consistent with the harness. - The new
^RSS: \\d+ MB$/^RSS delta: -?\\d+(\\.\\d+)? MB$assertions match the fixtures'console.logoutput exactly (bunRuntrims stdout,| 0yields an integer,.toFixed(1)yields one decimal), so they can't pass vacuously. .repeat(n)→Buffer.alloc(n, fill).toString()follows the repo's stated convention for debug-JSC performance.- Timeouts: dropping 60 s → default on the two sub-second tests is safe (measured ~1-2 s under debug-ASAN); 30 s remains on the ~5.6 s server.fetch test.
- Both prior review threads are marked resolved and there are no other outstanding comments.
|
CI is green on the diff: The only red is |
Cuts
bun bd test test/js/bun/http/leaks-test.test.tsfrom 11.7s to 7.7s on a debug-ASAN build and fixes a false-positive failure in theserver.fetch(string)leak test when run viabun bdlocally.What changed
request-constructor-leak-fixture.js(9.8s → 2.0s)The Request constructor copies the body buffer before the
signaloption is coerced (which is where the fixture'sSymbolthrows), so under ASAN each of the 1000 iterations paid for a 16 MiB poison/copy/unpoison. Shrinking the body to 2 MiB keeps the leak signature at ~2000 MiB against the existing 1024 MiB threshold (~2x margin; observed no-leak RSS: 69 MB release, ~610 MB debug-ASAN) while dropping per-iteration cost ~8x. Also hoisted the per-iteration.repeat(1024)strings intoBuffer.alloc().toString()built once up front.server-fetch-string-leak-fixture.js(bug fix + ~25% faster)The fixture branched on
process.execPath.includes("bun-asan"). A localbun bddebug build is ASAN-instrumented but namedbun-debug, so the fixture took the non-ASAN branch (4096 iterations, 64 MB threshold) and observed ~190 MB of ASAN quarantine growth, failing the test. It now usesrequire("bun:internal-for-testing").isASANEnabled()with the name check as a fallback, matching howharness.tsdetects ASAN.With detection fixed the fixture becomes the file's long pole, so the path is doubled to 64 KiB and iterations halved (8192 → 4096 ASAN, 4096 → 2048 non-ASAN). The total leak signature is unchanged (~512 MiB under ASAN, ~256 MiB release) and the 450 MB / 64 MB thresholds are untouched. Measured no-leak delta under debug-ASAN is 323-329 MB across five runs, well under 450.
response-constructor-leak-fixture.jsHoisted the in-loop
.repeat(1024)to aBuffer.alloc().toString(). Already sub-second; no other change.leaks-test.test.tsRSS: N MB/RSS delta: N MBline before the.toSpawn()exit-code check.server.fetchtest, which legitimately exceeds the 5 s default under debug-ASAN.Why this is correct
No leak threshold was loosened. The only workload changes trade size for iteration count such that the byte total that would leak is preserved (request: 2 MiB × 1000 = 2000 MiB vs 1024 MiB threshold; server.fetch ASAN: 64 KiB × 2 × 4096 = 512 MiB vs 450 MiB threshold). The ASAN-detection change mirrors
test/harness.ts'sdetectASAN().Verification
server.fetch no-leak delta, 5 consecutive debug-ASAN runs:
324.7, 323.7, 329.2, 326.4, 327.6 MB(threshold 450).Release (
USE_SYSTEM_BUN=1): all three pass in 479 ms.[stamp-90s] gate passed · iteration 1 · 4 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file