Repository navigation
Conversation
…2s on ASAN CI) On the x64-asan CI lane, scripts/runner.node.mjs sets ASAN_OPTIONS=...detect_leaks=1 and BUN_DESTRUCT_VM_ON_EXIT=1 for the test process. bunEnv spreads process.env, so the spawned subprocess in the "without an error handler" case inherited both and spent ~24s doing a leak scan at exit (Buildkite timestamps show the first 6 in-process tests finish in 690ms, the 7th in 24.2s). Override detect_leaks=0 and BUN_DESTRUCT_VM_ON_EXIT=0 for that spawn: it is a behavior probe (500 + stderr + exit 1), not a leak test, and the outer test process still runs under LSan. Also run the describe block concurrently so the in-process cases overlap the spawn, and assert the static-route case never invokes error(). Local debug+ASAN with the CI runner env (detect_leaks=1, GC level 1, integrity audit 1.0, exception checks, VM destruct): before: 9.22s 9.44s 9.64s 9.27s 9.82s (avg 9.48s) after: 4.24s 5.10s 4.02s 4.20s 3.26s (avg 4.16s)
|
Warning Review limit reached
Next review available in: 17 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 (1)
Comment |
|
Updated 9:14 PM PT - Jul 25th, 2026
❌ @robobun, your commit ba44a3a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35744That installs a local version of the PR into your bun-35744 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only CI speedup following an established pattern.
What was reviewed:
- The
ASAN_OPTIONSappend +BUN_DESTRUCT_VM_ON_EXIT=0override matches the precedent inbun-server.test.ts:1534andwebsocket-server-upgrade-reentrant.test.ts:85; last-wins semantics meandetect_leaks=0correctly overrides the inherited CI value. describe.concurrentis safe: every case owns its ownport: 0server, error array/mock, andcachedResponse; the describe-levelalreadyUsedError/reusedBodiesare read-only.- The added
errormock on the static-route case strengthens the assertion (mirrors the existing fresh-Response case), andbunEnvis spread rather than replaced.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/http/serve-reused-response.test.ts, to cut ASAN-lane wall time from ~26s to ~2s. Three changes: (1) the subprocess test appends detect_leaks=0 to ASAN_OPTIONS and sets BUN_DESTRUCT_VM_ON_EXIT=0 so the spawned -e child doesn't run a full LSan scan at exit; (2) the describe block is switched to describe.concurrent; (3) the static-route test gains an error mock + not.toHaveBeenCalled() assertion to match the neighboring fresh-Response test. No production code is touched.
Security risks
None. Test-only change to environment variables passed to a spawned test subprocess and to test concurrency/assertions.
Level of scrutiny
Low. This is a targeted test-performance fix with a well-documented root cause (CI runner sets detect_leaks=1 in the outer env, bunEnv forwards it via process.env because harness.ts only defaults it with ??=). The fix pattern — [bunEnv.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":") — is copied verbatim from bun-server.test.ts and websocket-server-upgrade-reentrant.test.ts, so it's an established convention, not a novel workaround.
Other factors
- Not weakening a safety net: REVIEW.md flags silent weakening of CI verification. Here the reasoning is stated (behavior probe, not a leak test; outer process still under LSan) and matches two existing precedents in the same test directory.
- Concurrency safety: I checked each test in the block — every one creates its own server with
await using, its ownerrorsarray orjest.fn()mock, and (forit.each) its owncachedResponse inside the callback. The only describe-scoped state is the read-onlyalreadyUsedErrorobject andreusedBodiesfactory map, so concurrent execution is safe. - Assertion strengthening: the new
expect(error).not.toHaveBeenCalled()on the static-route case is additive and mirrors the existing pattern one test up. - Env handling:
{...bunEnv, ...}correctly spreads the base env per REVIEW.md's harness-convention guidance. - No prior reviewer comments to address; PR description confirms
USE_SYSTEM_BUN=1still fails 5/7, so the tests remain load-bearing.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #33704. That PR only adds If #33704 lands first I'll rebase over the one-line conflict. |
…he win The cited precedents (bun-server.test.ts, websocket-server-upgrade-reentrant.test.ts) only override ASAN_OPTIONS. Isolated timing of the spawned -e script under the CI env: 8.25s with detect_leaks=1, 1.09s with detect_leaks=0 alone, 0.77s with detect_leaks=0 + DESTRUCT_VM=0. The extra ~300ms is not worth introducing the repo's only DESTRUCT_VM=0 line and losing VM-teardown crash coverage in the subprocess.
There was a problem hiding this comment.
LGTM — test-only CI speedup, no assertions weakened.
What was reviewed:
ASAN_OPTIONSoverride matches the exact pattern already inbun-server.test.ts:1534andwebsocket-server-upgrade-reentrant.test.ts:85;bunEnv.ASAN_OPTIONSis always populated on ASAN viaharness.ts:90, and.filter(Boolean)handles the non-ASAN case.describe.concurrentis safe here: each case creates its ownport: 0server viaawait using, and the only shared state (reusedBodies,alreadyUsedError) is read-only / factory-per-call.- Static-route case gains an
errormock assertion — strictly strengthens coverage, mirrors the sibling "fresh Response" case.
Extended reasoning...
Overview
Test-only change to test/js/bun/http/serve-reused-response.test.ts (single file, +12/-2). Three independent tweaks: (1) append detect_leaks=0 to the spawned subprocess's ASAN_OPTIONS so it doesn't run a ~24s LSan pass at exit, (2) switch describe → describe.concurrent, (3) add a jest.fn() error handler + not.toHaveBeenCalled() assertion to the static-route test to match its sibling. No production code, no test deletions, no skipped assertions.
Security risks
None. This is test infrastructure only; the subprocess env override affects a locally-spawned bunExe() child and only appends to an existing sanitizer option string.
Level of scrutiny
Low. The core question per REVIEW.md is whether this silently weakens a safety net. It does not: the subprocess in question asserts on stdout content, stderr content, and exit code — it was never a leak test, and the outer test process continues to run under full LSan on the ASAN lane. The [bunEnv.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":") construction is copied verbatim from two existing call sites in the suite, so it follows harness convention exactly. The PR description's mention of BUN_DESTRUCT_VM_ON_EXIT=0 was dropped in commit 98a9d26 ("detect_leaks=0 alone carries the win"), which explains why it's absent from the final diff.
Other factors
For describe.concurrent I checked that no test mutates shared state: reusedBodies values are factory functions invoked per-test, alreadyUsedError is a frozen-shape literal used only in toEqual, and every server is await using with port: 0. The added error mock in the static-route case is a strict tightening (previously the test would have passed even if error() fired). jest is already imported at the top of the file. The PR author confirmed USE_SYSTEM_BUN=1 still fails 5/7, so the tests remain load-bearing.
There was a problem hiding this comment.
LGTM — test-only CI-speed fix that follows the established detect_leaks=0 override pattern.
What was reviewed:
describe.concurrentsafety: each case owns its ownport: 0server viaawait using, and the sharedreusedBodies/alreadyUsedErrorare read-only — no cross-test state.ASAN_OPTIONSoverride matches the idiom inbun-server.test.ts/websocket-server-upgrade-reentrant.test.ts; later options win so appendingdetect_leaks=0correctly overrides the inheriteddetect_leaks=1.- Static-route test now also asserts
errornot called — strictly strengthens coverage; no assertions weakened or removed. - The two CI failures on #81093 are infra (x64-android build step, 25.04 agent creation), unrelated to this test-only diff.
Extended reasoning...
Overview
Test-only change to test/js/bun/http/serve-reused-response.test.ts (13 insertions, 2 deletions). Three modifications: (1) describe → describe.concurrent, (2) the subprocess spawn appends detect_leaks=0 to ASAN_OPTIONS so it doesn't inherit LSan from the CI runner env, (3) the static-route case gains an error mock and asserts it was never called, mirroring the neighboring "fresh Response" case.
Security risks
None. No runtime code, no auth/crypto/permissions surface. The env change scopes leak-detection off for one subprocess behavior probe; the outer test process still runs under LSan, so overall leak coverage for the file is unchanged.
Level of scrutiny
Low. This is a mechanical CI-speed change following an existing repo idiom — the exact [bunEnv.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":") form already appears in bun-server.test.ts, websocket-server-upgrade-reentrant.test.ts, fs.watchFile.test.ts, and bun-upgrade.test.ts. harness.ts sets bunEnv.ASAN_OPTIONS with ??=, confirming the PR's diagnosis that CI's detect_leaks=1 flows through. ASAN option strings are last-wins, so appending overrides correctly.
The describe.concurrent conversion is safe: every test constructs its own server with port: 0 under await using, the it.each factories return fresh Response instances per invocation, and the only shared identifiers (alreadyUsedError, reusedBodies) are immutable. The one added assertion strictly tightens coverage.
Other factors
The PR body includes a passing bun bd test run showing all 7 tests green post-change. The two Buildkite failures on the latest commit are an x64-android build step exit and a 25.04 agent-creation failure — infrastructure, not test regressions caused by this diff. The noted overlap with #33704 is a one-line describe.concurrent conflict the author has acknowledged; it doesn't affect correctness here.
|
CI status: both builds (#81093, #81472) failed on infra before reaching the x64-asan test lane.
This diff is test-only (13 lines in one |
What
test/js/bun/http/serve-reused-response.test.tswas the worst file on thedebian 13 x64-asanlane at 26s wall time (build #80083,Ran 7 tests across 1 file. [25.72s]), despite the seven tests themselves doing almost no work.Why it was slow
Buildkite's per-line timestamps for that run break down as:
The first six in-process tests finish in 690ms. The seventh spawns
bunExe() -e <script>withenv: bunEnv. On the ASAN lanescripts/runner.node.mjssets the outer process's env to includeASAN_OPTIONS=...:detect_leaks=1:abort_on_error=1,LSAN_OPTIONS=malloc_context_size=30:...andBUN_DESTRUCT_VM_ON_EXIT=1.bunEnvspreadsprocess.envand only defaultsASAN_OPTIONSwith??=, so the subprocess inherits the full leak-scan configuration and spends ~24s in LeakSanitizer at exit.Local debug+ASAN reproduces the same shape for the spawned
-escript:detect_leaks=1+BUN_DESTRUCT_VM_ON_EXIT=1(CI)detect_leaks=0alonedetect_leaks=0alone carries the win; VM teardown is left enabled so the subprocess keeps its teardown crash coverage.Fix
detect_leaks=0to the subprocess'sASAN_OPTIONS. This is a behavior probe (500 response + stderr message + exit 1), not a leak test; the outer test process still runs under LSan. The[bunEnv.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":")form matchestest/js/bun/http/bun-server.test.tsandtest/js/bun/websocket/websocket-server-upgrade-reentrant.test.ts.describe.concurrent): every case owns its ownport: 0server and shares no state, so the six in-process cases now overlap the subprocess instead of queueing behind each other.error()is never invoked, matching the "fresh Response" case.No tests removed or skipped;
USE_SYSTEM_BUN=1 bun teststill fails 5/7 (the fix in #33118 is still exercised).Timing
Local debug+ASAN with the CI runner env (
BUN_GARBAGE_COLLECTOR_LEVEL=1,BUN_JSC_randomIntegrityAuditRate=1.0,BUN_JSC_validateExceptionChecks=1,BUN_JSC_dumpSimulatedThrows=1,BUN_DESTRUCT_VM_ON_EXIT=1,detect_leaks=1):On the actual CI lane the LSan scan is ~3x slower than local, so the expected drop is from ~26s to ~2-3s.
[stamp-90s] gate passed · iteration 3 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 3
evidence per changed file