Conversation
Every test in the file does a synchronous GC per iteration at counts sized for release builds, so under a debug build all four run past the default 5s timeout (92s, 7.0s, 7.5s, 47.9s on debug+ASAN) and the file always fails under `bun bd test`. Pick smaller counts when isDebug; release builds keep the existing counts.
|
Warning Review limit reached
Next review available in: 3 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 6:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 56e5cfd has some failures in 🧪 To try this PR locally: bunx bun-pr 38400That installs a local version of the PR into your bun-38400 --bun |
|
Status: fix pushed, waiting on CI. Reproduced on main with a debug+ASAN build ( |
There was a problem hiding this comment.
LGTM — test-only change that reduces loop counts under isDebug while leaving release/CI counts untouched.
What was reviewed
- Confirmed
isDebugis exported fromtest/harness.tsand that release-build iteration counts are byte-identical to before. - Checked this follows the same pattern as #38259 / #36929 / #37835 / #37488; the added comment explains why debug builds don't need the repetition (refcount
debug_assertfires on first imbalance). - Verified the diff removes no assertions and adds no per-test timeout override.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/util/error-gc-test.test.js, and only changes loop iteration counts to be conditional on isDebug from the test harness. Under release builds (what CI runs), every count is exactly what it was before (100×1000, 1000, 1000, 1000). Under debug builds, counts drop to 5×100, 100, 100, and 10 so the file completes within the default 5s per-test timeout instead of taking ~2.5 minutes and being reported as timed out. A four-line comment is added explaining why the reduced counts are safe on debug builds (the WTFStringImplStruct::ref/deref debug_assert catches an unbalanced refcount on the first occurrence, so repetition is only needed on release where the failure mode is a later reuse/double-free).
Security risks
None. Test-only change to iteration counts in a GC stress test; no source code, no user-facing behavior, no I/O paths, no auth/crypto.
Level of scrutiny
Low. This is a mechanical test-sizing change following an established repo pattern (four cited precedent PRs doing the same thing to other slow test files). Release-build behavior — and therefore CI coverage — is completely unchanged. The one review concern for this class of change ("does the reduced count still catch the bug it guards against?") is directly addressed in the PR description: the author reintroduced the original bug (Bun::toString instead of Bun::toStringRef in ZigException.cpp) and confirmed the reduced-count file still crashes on the first error under a debug build. That satisfies the REVIEW.md "when de-flaking, keep asserting the property the original assertion protected" rule.
Other factors
isDebugis confirmed present intest/harness.ts:29.- No assertions were removed or weakened; no
setDefaultTimeoutor per-test timeout was added (per test/CLAUDE.md guidance). - The added comment is load-bearing (explains the non-obvious reason lower counts are safe on debug), not narration.
- No prior human or bot review comments to address; the bug-hunting system found nothing.
Problem
bun bd test test/js/bun/util/error-gc-test.test.jsalways fails on a debug build, on a clean checkout of main:test/js/bun/util/error-gc-test.test.jsare sized for release builds. Test 1 inspects an error 1000 times and runs twoBun.gc(true)per outer iteration, 100 times over; tests Fix calling #private() functions in classes #2 and Copy source lines when generating error messages #3 create an error and callBun.gc()1000 times; test Support import assertions #4 runs a failingreadFileSync,Bun.inspectand twoBun.gc(true)1000 times. On a debug+ASAN buildBun.inspect(err)costs about 0.9ms (0.03ms on release) andBun.gc(true)about 20ms (1ms on release), so the four tests take 17x to 38x longer than on a release build of the same machine (2.4s / 0.4s / 0.45s / 2.3s).scripts/runner.node.mjspasses its own--timeout(90s, tripled under ASAN). It breaks every localbun bd testrun of the file, and nothing can be added to the file and pass underbun bd test.Fix
isDebugfromtest/harness.ts: test 1 runs 5 errors x 100 inspects (was 100 x 1000), tests Fix calling #private() functions in classes #2 and Copy source lines when generating error messages #3 run 100 iterations (was 1000), test Support import assertions #4 runs 10 (was 1000). Release builds keep the existing counts, so CI (release and release ASAN lanes) runs exactly the work it ran before.Bun::toStringRefinsrc/jsc/bindings/ZigException.cpp). Debug builds assert on the refcount the first time it goes wrong (debug_assert!(old > 0)inWTFStringImplStruct::ref/deref,src/bun_alloc/lib.rs), so the repetition only buys something on release builds, where a string freed early is noticed only once its memory is reused or freed again. Checked by reintroducing the bug: with line 125 ofZigException.cppchanged to the non-ref'ingBun::toString, the reduced file still crashes inside the first error of test 1 (details below).bun bd test test/js/bun/util/error-gc-test.test.js(debug+ASAN): 4 pass, 797ms / 740ms / 758ms / 783ms. The same command on the unmodified file is the failure quoted above.USE_SYSTEM_BUN=1 bun test test/js/bun/util/error-gc-test.test.js(release, unchanged counts): 4 pass, 2.46s / 0.48s / 0.53s / 2.63s, the same work as before the change.src/to stash for a fail-before run; the before/after above is the test file itself.Background
isDebug(test/harness.ts) is true when the bun running the tests reports a debug version, which is whatbun bdbuilds. The release ASAN builds CI runs are not debug builds and keep the full counts: they have no refcount assertion, and ASAN alone is not a substitute for the repetition, because the strings live in libpas (WebKit's allocator, which ASAN does not instrument) rather than in ASAN's heap; the libpas panic in the experiment below is that allocator reporting the double free on an ASAN build.Bun.gc(true)is a synchronous full collection that also throws away compiled code (JSC__VM__runGCinsrc/jsc/bindings/bindings.cpp);Bun.gc()only requests an asynchronous one. Thegc(true)calls are what make tests 1 and Support import assertions #4 the slow ones.Reintroducing the bug against the reduced counts
src/jsc/bindings/ZigException.cpp:125changed fromBun::toStringRef(functionName)toBun::toString(functionName)(no ref, the pre-c6f6db95ff behavior), rebuilt withbun bd, thenbun bd test test/js/bun/util/error-gc-test.test.jswith this PR's counts. The run dies before test 1 reports a result:The
src/change was reverted before the verification runs above.