Repository navigation
Conversation
…d ASAN builds The "should not crash under intensive usage" test runs 10,000 iterations of two custom-matcher expect() calls. On a debug+ASAN build that takes 4 to 6 seconds, so it intermittently hits the default 5 second per-test timeout. Run 1,000 iterations there and keep 10,000 on release builds.
|
Warning Review limit reached
Next review available in: 9 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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:05 PM PT - Aug 16th, 2026
✅ @robobun, your commit aa8b1a5a2392bd66cb860a74fe078a4519a22548 passed in 🧪 To try this PR locally: bunx bun-pr 39359That installs a local version of the PR into your bun-39359 --bun |
|
Status: test-only change, ready for review.
|
There was a problem hiding this comment.
LGTM — test-only de-flake that shrinks the workload on instrumented builds instead of raising the timeout, matching repo convention.
What was reviewed:
isDebugandisASANare both exported fromtest/harness.ts; theisDebug || isASAN ? small : largepattern is already used in ~10 other test files.- Release builds keep the full 10,000 iterations, so non-instrumented coverage is unchanged.
- The property the test protects (no crash across many custom-matcher calls + forced full GC) is still asserted at 1,000 iterations under ASAN.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/test/expect-extend.test.js, changing two lines: it adds isASAN and isDebug to the existing harness import, and replaces the hardcoded 10000 loop bound in the "should not crash under intensive usage" test with isDebug || isASAN ? 1000 : 10000. No runtime code, no other tests, no assertions are changed.
Security risks
None. This is a test-only iteration-count adjustment with no effect on shipped code, no new inputs, and no changes to any security-relevant path.
Level of scrutiny
Low. The change is mechanical and follows an established idiom — grep shows the identical isDebug || isASAN ? … : … scaling pattern in bundler_splitting.test.ts, html-rewriter.test.js, string-decoder.test.js, worker_heap_snapshot_gc.test.ts, and others. Both harness exports were verified to exist. The repo's own review guidance explicitly prefers shrinking workload over raising per-test timeouts, and this PR does exactly that.
Other factors
The test's intent is preserved: it is a crash/UAF stress test, and 1,000 iterations under ASAN (which is what surfaces UAF) plus the trailing Bun.gc(true) still exercises the same lifetime path the original 10,000 did — the PR description confirms via BUN_JSC_logGC=1 that no in-loop collection fires at either count, so the forced full collection after the loop is the load-bearing check in both cases. Release lanes retain the original 10,000 iterations. No prior reviewer comments are outstanding, and the bug-hunting system found nothing.
CI always passes its own --timeout (90s, 270s on the ASAN lane), so the 5s default only applies to local bun bd test runs, which are debug builds. Keep the release ASAN lane at the original 10,000 iterations.
|
Revised in aa8b1a5: the iteration count is now gated on The 5s default timeout that this test was hitting only applies to direct |
There was a problem hiding this comment.
LGTM — test-only iteration-count reduction on debug builds, matching the established pattern (e.g. sqlite-sql.test.ts:2237). The inline nit about the PR title/description wording is non-blocking.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/test/expect-extend.test.js, changing the "should not crash under intensive usage" test to run 1,000 iterations on debug builds instead of 10,000 (release builds keep 10,000). The diff is 3 lines: import isDebug from harness, compute iterations, and use it as the loop bound. No runtime code is touched.
Security risks
None. Test-only change to an iteration count constant; no new inputs, no I/O, no external surface.
Level of scrutiny
Low. This is the canonical de-flaking pattern the repo guidelines explicitly prescribe ("shrink the workload" over raising timeouts), and the exact isDebug ? 1000 : 10000 shape already exists verbatim at test/js/sql/sqlite-sql.test.ts:2237. isDebug is exported from test/harness.ts:29. The test's invariant (custom matchers don't crash/UAF under repeated use, followed by a forced full GC) is preserved at 1,000 iterations — the PR description documents that no in-loop GC fires at either count, so the post-loop Bun.gc(true) is the lifetime check in both cases, and ASAN still instruments every iteration.
Other factors
The one finding is a nit: commit aa8b1a5 intentionally narrowed the gate from isDebug || isASAN to isDebug only, but the PR title/description still say "debug and ASAN". Release-ASAN at 10,000 iterations is well under the timeout (release is ~20ms; even 5× ASAN overhead is <200ms), so the code is correct as-is — only the description wording is stale. This does not block merge. No prior human reviews or outstanding comments to address.
Problem
bun bd test test/js/bun/test/expect-extend.test.jsintermittently fails "should not crash under intensive usage" withthis test timed out after 5000ms.expect-extend.test.js:331) runs 10,000 iterations of two custom-matcherexpect()calls. On a debug build that loop takes 4.0s to 6.2s (9 runs of the file: timed out at 5493ms, 5659ms and 6166ms, two more passed at 4964ms and 4987ms), against bun test's 5s default per-test timeout (src/options_types/context.rs:483). The same loop takes 13ms to 23ms on a release build..buildkite/ci.mjs), and its runner passes its own--timeoutto everybun testinvocation (90s, tripled to 270s on the ASAN lane;scripts/runner.node.mjs:976-1002and:1969-1974), so the 5s default only applies when the file is run directly under a debug build, i.e.bun bd test. bun test: a matcher blocked on a promise that never settles fails with the test timeout #39089 ran into the same timeout while verifying an unrelated change on a debug build.Fix
isDebug, 10,000 otherwise (test/js/bun/test/expect-extend.test.js). Same shape as the other debug-sized loops in the tree (test/js/bun/yaml/yaml.test.ts:3121,test/js/web/html/FormData.test.ts:754), and the convention intest/CLAUDE.mdis to shrink the work on slow builds rather than set a timeout.isDebugonly, notisDebug || isASAN: every build with the problem is a debug build, and the CI release ASAN lane has a 270s budget and has not been timing out, so it keeps the original 10,000 iterations. CI behavior is unchanged by this PR; only debug builds run the shorter loop.Expectobject, calls the custom matcher through its wrapper, and runs the custom asymmetric matcher throughtoEqual; theBun.gc(true)after the loop still collects everything the loop allocated; and the tests after it keep using the matchers registered at the top of the file, so a wrapper or matcher context freed by that collection would still fail the file at either count. WithBUN_JSC_logGC=1no collection starts inside the loop at either count (about 0.5KB allocated per iteration, under JSC's collection threshold), so the forced collection after the loop is the lifetime check at 10,000 iterations as well as at 1,000. The debug build is still ASAN-instrumented for each of the 1,000 iterations.bun bd test test/js/bun/test/expect-extend.test.js: the test takes 468ms to 613ms on the debug build (13 of 13 timed runs across both revisions of this PR pass, all withBUN_GARBAGE_COLLECTOR_LEVEL=1as CI sets it), down from 4.0s to 6.2s; the file reports 2088expect()calls.USE_SYSTEM_BUN=1 bun test test/js/bun/test/expect-extend.test.js(release) still reports 20088expect()calls, i.e. the 10,000-iteration path.Background
isDebug(test/harness.ts:29) isBun.version.includes("debug"): true forbun bdbuilds, false for release builds including the release ASAN build CI's ASAN lane runs.debug_assertionsare on: among other things every refcounted object created by the matcher path captures a stack trace inThreadLock::lock(src/bun_core/util.rs:2142). The release ASAN profile also enables those assertions (scripts/build/rust.ts:742-750), but that lane is optimized code under a 270s per-test budget, so it does not need the shorter loop.bun testwithBUN_GARBAGE_COLLECTOR_LEVEL=1(scripts/runner.node.mjs). In that mode every matcher call requests a collection on the way out (apply_custom_matcherandpost_matchinsrc/runtime/test_runner/expect.rs).withoutAggressiveGC, which already wraps this loop, turns that off for the duration of the loop, so the loop time above is the matcher work itself.Timings (linux-x64, 16 cores, debug+ASAN build of 8326d1b)
Before,
bun bd test test/js/bun/test/expect-extend.test.js, 9 runs:After (
isDebug ? 1000 : 10000), same command, 5 runs withBUN_GARBAGE_COLLECTOR_LEVEL=1:The first revision of this PR gated on
isDebug || isASAN; on the same debug build it measured 470ms to 613ms over 8 runs (identical code path, since a debug build is both). It was changed toisDebugonly so that the CI release ASAN lane, which has a 270s per-test budget and no observed timeout, keeps its 10,000 iterations. On that lane the first revision ran the whole file in 346ms (Buildkite build 99581).Release (
USE_SYSTEM_BUN=1, bun 1.4.0), 10,000 iterations, 4 runs: 13.51ms, 22.68ms, 22.82ms, 14.16ms.GC activity inside the loop, standalone copy of the test with
BUN_JSC_logGC=1on the debug build: 0 collections started inside the loop at 1,000, 2,000 and 10,000 iterations (at 10,000 the only in-loop log lines are the end of an eden collection that started before the loop); in every case theBun.gc(true)after the loop runs a FullCollection and full sweep. Loop time in that copy: 510ms at 1,000, 1194ms at 2,000, 4156ms at 10,000.[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file