Conversation
…T tier-up The WebKit upgrade in #33133 shifted JSC tier-up timing so DFG/FTL compilation for this hot loop now often lands in round 2 instead of finishing in round 1. That inflated the round-2 vs round-1 RSS delta from <8 MB to 8-13 MB on ~40% of release CI runs since build 67387. This is not a retention regression: heapSize is flat at ~2 MB across five rounds, every tracked object count (Timeout, Promise, AsyncGenerator, Function) is delta=0, and rounds 3-5 show RSS going down. With BUN_JSC_useJIT=0 the round-2 delta is <2 MB and process RSS drops from ~65 MB to ~33 MB, so the extra RSS is JIT code pages. Run one unmeasured warmup round before the two measured rounds so the 8 MB release bound holds at steady state (0/50 local runs; r3-r2 ranged -2.2..+2.0 MB over 30 probes vs -0.0..+11.0 MB for r2-r1). Also assert heapSize itself is flat, and drop debug/ASAN ITER to 1500 so total work (3x1500) stays close to the previous 2x2000.
|
Updated 8:07 PM PT - Jul 19th, 2026
❌ @robobun, your commit e038b51 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34752That installs a local version of the PR into your bun-34752 --bun |
WalkthroughChangesMongoDB pattern leak test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Build 75932 (e038b51): |
heapStats().heapSize is vm.heap.size() (live-cell bytes after a sync full GC), not subject to the bmalloc scavenger which only decommits OS pages. Measured r2-r1 delta across 30 release + 3 debug/ASAN runs: -38..+8 KB; the simulated listener leak at ITER=1500 produces ~56 MB. 256 KB sits 32x above the observed max and 220x below the leak.
|
Self-review caught that the heapSize comment was wrong (the bmalloc scavenger decommits OS pages and affects RSS, not |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/net/net-mongodb-pattern-leak.test.ts`:
- Around line 202-203: Update the heap measurement reference in the test to use
heapStats().heapSize instead of vm.heap.size(), and remove the run-specific
measurement ranges and leak-size figures from the surrounding comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 896e91b9-0f01-4c29-a837-73684aaa431a
📒 Files selected for processing (1)
test/js/node/net/net-mongodb-pattern-leak.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — de-flake that strengthens rather than weakens the leak test.
What was reviewed:
- RSS bound (8 MB release / 32 MB debug) and all object-count / listener assertions are unchanged; only the measurement window moves past JIT tier-up.
- New
heapSizedelta check: 256 KB bound is data-derived (32× observed max, ~220× below the simulated-leak signal) and branches correctly on the same snapshot afterBun.gc(true). - Debug/ASAN ITER 2000→1500 keeps total iterations (3×1500) at or above the previous 2×2000, so leak sensitivity isn't reduced; release ITER unchanged.
Extended reasoning...
Overview
Test-only change to test/js/node/net/net-mongodb-pattern-leak.test.ts (12 insertions, 6 deletions). Adds an unmeasured warmup round before the two measured rounds so the RSS delta is taken at steady state instead of during JIT tier-up, adds a heapStats().heapSize delta assertion, and reduces debug/ASAN per-round iterations to keep total wall-clock under the existing 60 s timeout now that there are three rounds.
Security risks
None. No production code is touched; the test uses a local in-process TCP/TLS echo server on port: 0 with rejectUnauthorized: false against the harness's self-signed cert, all pre-existing.
Level of scrutiny
Low-to-moderate. The main risk with de-flake PRs is silently weakening the safety net (REVIEW.md: "When de-flaking, keep asserting the property the original assertion protected"). This PR does the opposite: the 8 MB release RSS bound is kept, every existing object-count and listener assertion is preserved, and a strictly tighter heapSize check is added. The PR description shows the empirical basis (JIT-off run halves RSS and eliminates the r2−r1 delta; r3−r2 range −2.2..+2.0 MB over 30 runs; simulated listener leak still fails on Function count and heapSize). The competing approach in #33725 (widen 8→24 MB) is explicitly rejected in favor of preserving the bound.
Other factors
- Debug/ASAN ITER drop 2000→1500 is compensated by the extra round (3×1500 = 4500 ≥ 2×2000 = 4000 total iterations), and per-round deltas are what's asserted, so per-round leak sensitivity is 1500/2000 = 75% of before on debug/ASAN — but the new 256 KB heapSize check is far more sensitive than the object-count thresholds it augments, so net coverage improves. Release ITER is unchanged at 5000.
- CI: green on all completed lanes per the build-status comment; the one unrelated red (
test-net-connect-memleak.json alpine) is failing on main and tracked separately. - The only inline review comment (CodeRabbit, about the
vm.heap.size()reference in a comment) was resolved and withdrawn; the code correctly usesheapStats().heapSize. - No prior claude[bot] reviews on this PR.
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
None of those three apply: this is a test-only change to how |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-20, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
net-mongodb-pattern-leak.test.tsstarted failing its 8 MB release RSS delta check on ~40% of PR builds after #33133 (WebKit upgrade, merged July 1). Sampling Buildkite annotations: 1/29 builds flaked in the week before #33133, 15/37 in the days after. Observed deltas 8.4-12.3 MB.Why this is not a memory regression
Five-round instrumented runs on current main:
heapSizeand every tracked object count are flat; RSS in rounds 3-5 goes down. WithBUN_JSC_useJIT=0total RSS drops from ~65 MB to ~33 MB and the r2-r1 delta is <2 MB on every run. The extra RSS is JIT code pages: the newer JSC's tier-up thresholds put more DFG/FTL compilation into round 2, where the old build had finished it in round 1.Over 30 local release runs the r2-r1 delta ranged -0.0..+11.0 MB (one would have failed the 8 MB bound), while r3-r2 ranged -2.2..+2.0 MB.
Change
heapSizedelta assertion (<1 MB), which is the precise signal for JS-heap retention.ITER2000 -> 1500 so total work (3x1500) stays near the previous 2x2000 under the existing 60 s timeout.Verification
onData.close): Function count 15000 > 20, both cases fail as expected.vs #33725
#33725 widens the release bound 8 MB -> 24 MB. That also stops the flake, but loosens the native-leak backstop. This keeps the bound where it was by measuring after JIT tier-up has settled, and adds the heapSize check the original test was missing. The
node-net.test.tsawait fix in #33725 is independent and still worth taking.[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