Repository navigation
Conversation
…reams The release cases in h2-conformance.test.ts fail on a debug build with `Expected: <= 3, Received: 4` (or 5). A heap snapshot shows the surviving streams as direct roots with reason JITWorkList, not as leaks. A DFG compile plan roots the arguments of the call that started it (the stream, as `this`). A finished plan stays in the worklist until the main thread reaches its next tier-up check. In liveCount() that check comes at about pass 11, one pass after the 10-pass plateau exit gives up. liveCount() now stops as soon as the count is within GC_STRAGGLERS and does not stop on a plateau. It still stops after 10 passes that collected nothing at all, so the leak these cases guard against fails fast with `Received: 16`.
|
Updated 12:25 PM PT - Sep 11th, 2026
✅ @robobun, your commit a91069632163d915b4ea6ef6a40a13abf6a2f73d passed in 🧪 To try this PR locally: bunx bun-pr 42357That installs a local version of the PR into your bun-42357 --bun |
|
Status: reproduced on linux x64 with the debug build from |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughThe HTTP/2 conformance test updates ChangesHTTP/2 conformance test
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The test now tolerates expected GC stragglers while retaining bounded leak detection, with no merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The review above has no actionable items, and no review threads are open. The format and lint checks pass. The test build (Buildkite 114382) is in progress. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a well-justified de-flake that keeps the assertion strength intact.
What was reviewed:
- The new loop condition stops at
<= GC_STRAGGLERS(the assertion bound) instead of chasing 0, so no additional pass can flip a pass into a fail; the 50-pass cap is unchanged. - The
pass === 10 && last === refs.lengthearly exit still fires on the actual regression these tests guard (all streams retained →Received: 16), so leak detection is not weakened and stays fast. - The retention is attributed to
JITWorkListroots (concurrent-compilemustHandleValues), not the conservative stack scan — a real, named retainer, so this doesn't run afoul of the "never blame the scanner" rule. - Checked all four
liveCountcall sites: each asserts<= GC_STRAGGLERS, so the changed exit condition is sound for every caller.
Extended reasoning...
Overview
This PR touches a single test-helper function, liveCount, in test/js/node/http2/h2-conformance.test.ts. The helper drives GC passes and counts surviving WeakRefs for the stream-release leak tests. The old version stopped early when the surviving count plateaued for 10 consecutive passes; the new version instead runs until the count drops to GC_STRAGGLERS (3) or fewer, keeps the 50-pass cap, and only bails early if after 10 passes literally nothing was collected (the actual leak signature). The doc comment is expanded to name the mechanism: a concurrent JIT compile plan roots the arguments (including this, the stream) of the call that triggered tier-up until the main thread installs the compiled result — on debug builds this pins 4–9 streams for a dozen passes, which the old plateau heuristic misread as a stable state.
Security risks
None. This is a test-only change to a GC-polling helper; no runtime code, no user-facing surface, no I/O or parsing of untrusted input is touched.
Level of scrutiny
Low-to-moderate, focused on REVIEW.md's de-flake rules: does the change weaken the assertion, and is the retention mechanism real (not a stack-scanner blame)? On both counts the change holds up. The assertion bound (<= GC_STRAGGLERS = 3) is unchanged and remains well below the unfixed leak (16 or 32 streams). The PR description verifies that reverting the fix under test still fails with Received: 16 in ~1–1.6s, so the early-exit path still catches the regression fast. The retainer is named as JITWorkList with heap-snapshot evidence and JSC source references (operationOptimize → mustHandleValues → Plan::checkLivenessAndVisitChildren), which is a legitimate GC root distinct from the conservative stack scan — CLAUDE.md rule 15 is not implicated. The new loop is also faster in the common case (stops at ≤3 instead of spending 10 extra passes chasing 0), which addresses the secondary timeout-under-load flake noted in the PR.
Other factors
No CODEOWNERS entry covers this file. All four liveCount call sites assert <= GC_STRAGGLERS, so terminating the loop at that threshold is sound for every consumer. The 50-pass hard cap bounds the worst case even if a partial leak (some but not all streams retained) doesn't trip the pass-10 early exit. The added comment is load-bearing — it names a non-obvious JSC mechanism the next reader would otherwise spend real effort rediscovering.
|
The second automated review has no findings either. It confirms that each of the four |
|
The summary above was regenerated for an empty commit. It has no new findings, and the diff is the same as at c5d83e4. No review threads are open. Build 114382: |
|
Data from #41305, in case it helps here. The Command, on a linux x64 debug+ASAN build, 2 lanes in parallel:
The block is at |
h2-conformance.test.ts is the same as on main again. That file also holds cases that count live stream objects after a GC, and those fail now and then on a debug build (#42357). The new file carries its own raw server, like h2-push-refusal-staged.test.ts.
This branch has not been deployed
1 participant
Problem
test/js/node/http2/h2-conformance.test.tsfails withExpected: <= 3,Received: 4(or 5): 7 of 12 runs with 2 cores.JITWorkList. It is not a leak and not the stack scan.liveCount()stops when the count does not move for 10 passes. The streams that a compile plan roots go at about pass 11.Fix
liveCount()stops when the count is withinGC_STRAGGLERS, not on a plateau. The assertion is<= GC_STRAGGLERS, so a later pass cannot change the result. The 50-pass limit stays.Received: 16.Background
JITWorklist.Http2Streammethod,thisis the stream). The GC marks them as roots until the plan is installed.liveCount()polls.Notes
Reproduction.
bun bd, thentaskset -c <2 cpus> ./build/debug/bun-debug test test/js/node/http2/h2-conformance.test.ts. Unmodified file: 7 of 12 runs fail (Received: 4or5). WithBUN_JSC_useConcurrentJIT=0the unmodified file passes 8 of 8 in the same setup. With all 12 cores about 1 run in 5 to 10 shows a plateau above 3.Snapshot.
generateHeapSnapshotForDebugging()on a fresh turn, after the count held above 3 for three passes. Reset case (32 streams, 9 alive): 8ServerHttp2Streamcells are direct roots with reasonJITWorkList, the ninth hangs off aJITWorkList-rootedWritableState(Object -[onwrite]-> bound function -> stream). Compat case: 3ServerHttp2StreamdirectJITWorkListroots. Client case: 5ClientHttp2StreamdirectJITWorkListroots. The stalled stream showsStrongHandles, as expected. No survivor was without a reported root.Mechanism in JSC (oven-sh/WebKit at
dfd696443b):operationOptimize(jit/JITOperations.cpp) fillsmustHandleValueswith every parameter of the triggering frame (numParameters()includesthis), also for a compile at function entry, and hands them toDFG::compile.DFG::Plan::checkLivenessAndVisitChildrenappends them withappendUnbarriered.JITWorklist::visitWeakReferencesruns that for every plan inm_plans(queued, in compilation, ready) from the "JIT Worklist" marking constraint.JITWorklistThread::workmoves a finished plan tom_readyPlansand notifies nobody on the main thread.completeAllReadyPlansForVMis what finalizes it. Its callers are the tier-up slow paths (operationOptimize,jitCompileAndSetHeuristicsinllint/LLIntSlowPaths.cpp, the FTL triggers) andHeap::completeAllJITPlans(delete all code).Stand-alone demonstration (debug build): call
function hot(o)with 400 distinct objects, then only collect. Exactly one object survives (the argument of the call that started the compile, for example #147) whilenumberOfDFGCompiles(hot)is 0. It goes on the pass after the count turns 1. WithBUN_JSC_useConcurrentJIT=0nothing survives.Why pass 11. Traces of the loop (count logged per pass, loop extended to 400 passes): a group of 4 to 6 holds from pass 0 and goes in one pass at pass 11 to 14, at 1.0 s with 12 cores, 2.4 to 2.8 s with 2 cores, 3.6 s with 1 core. So the release follows the pass count, not the time. The loop's own code makes the tier-up checks: the
filtercallback runs 16 times per pass and reaches the LLInt threshold near pass 6 and the DFG threshold near pass 11. In the 32-stream case the steps start at pass 4 to 6. A single straggler that misses those checks goes at pass 60 to 62 in every run (thefilterloop's own DFG threshold). Code older than 5 to 15 s is discarded at a full GC, so each case starts this schedule again.Second effect. The old loop tried to reach 0, so a case with 1 to 3 pinned stragglers spent 10 more passes for nothing (about 1 s with 12 cores, 2 s with 2). Under CPU load (24 busy loops on 12 cpus) that alone pushed the unmodified file over the default 5 s timeout in 3 of 8 runs. The new loop returns at once in that state.
Runs of the fixed file (debug build, whole file unless noted): 12 of 12 with 2 cores, 6 of 6 with 1 core, 15 of 15 with 12 cores, 8 of 8 with 24 busy loops on 12 cpus, 12 of 12 with
-t "stream release after a queued END_STREAM"and 2 cores.Leak check. Removed
self.free_resources::<false>(client)fromStream::flush_queue(h2_frame_parser.rs:1651), rebuilt: the five queued cases fail withExpected: <= 3, Received: 16in 1.0 to 1.6 s each, the reset case passes (other path). Restored, rebuilt: 70 pass.Related. #34640 (with oven-sh/WebKit#308) proposes that JSC stops rooting must-handle values. It is open. It found the same
JITWorkListroots behind the N-1/N stall intest-gc-http-client*. With it no plan would root a stream, and this loop would return after the first pass. This change does not depend on it.Not done. A longer plateau has no bound to derive it from (the release waits for a tier-up check, not for time).
getProtectedObjects()in place of collection (#39609) and a child process with the concurrent JIT off both remove the JIT from the picture, but they replace the design that #40966 settled on. This change keeps that design.[auto-merge] gate passed · iteration 1 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file