fix(ext-net): give the pump's in-flight dispatch GC custody (#8259) - #8265
Conversation
Every arm of js_ext_net_drain_pending snapshotted its listeners into a bare Vec<i64> and, for some arms, allocated a payload before the call loop. Both steps can collect — the allocation directly, every call* by running user JS — and the evacuating arms then MOVE the closures: scan_net_roots rewrites the canonical statics::listeners() slots, but a bare local snapshot is invisible to it, so callback N and the already-built payload were dereferenced at their OLD addresses once callback N-1's JS forced a collection. That is exit-139 in test_gap_gc_net_once_flags_rekey under force_verify (#8259): the once handler churns 30k objects between the two 'data' callbacks. ServerListening/ServerClose were worse: they REMOVE their callbacks from the table before firing, so nothing rooted them at all during dispatch — a full sweep could free, not just move, them. Fix is the same custody pattern #8216 used for ext-http's H2 pump: park the snapshot (+ at most one NaN-boxed payload per frame) in scanned thread-locals (dispatch_custody.rs), re-read each slot immediately before use so the copying GC's rewrite is observed, pop on drop. All ten dispatch loops converted; scan_net_roots visits the custody stacks; the census call-graph walk certifies the holders through that path.
|
Warning Review limit reached
Next review available in: 13 minutes Limit details: You’ve used all 8 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fragment filenames are PR-keyed so in-flight PRs never collide; this one carried #8260's number. Claude-Session: https://claude.ai/code/session_01AHvBYz7E6wWKv8kmvLLGpj
|
Merging. This is the most carefully evidenced GC PR I've reviewed today — a few notes on what I verified independently rather than took on trust. The "not a vacuous pass" line is the part that matters, and it is the check most GC PRs skip. Six copy-minor records under the matrix env means the fix was actually exercised, not merely un-crashed; a green arm that ran zero copying minors would have proven nothing. Same for the pre-fix arm reproducing The one failing test is genuinely pre-existing. I did not take that on trust: reverting Verified here: One fix pushed: the changelog fragment was named One note on my side, not yours. #8253 added |
Fixes #8259.
Root cause
Every arm of
js_ext_net_drain_pendingsnapshotted its listeners into a bareVec<i64>(listeners_for) and, for some arms, allocated a payload (alloc_buffer/alloc_string/build_error_object) before the call loop. Both steps can collect — the allocation directly, everycall*by running arbitrary user JS — and the evacuating arms then move the closures:scan_net_rootsrewrites the canonicalstatics::listeners()slots, but a bare local snapshot is invisible to it, so callback N (and the already-built payload) were dereferenced at their old addresses once callback N−1's JS forced a collection. The #8259 witness'sonce('data')handler churns 30k objects between the two'data'callbacks — deterministic by construction.ServerListening/ServerClosewere worse: they remove their callbacks from the table before firing (one-shot semantics), so during dispatch nothing rooted them at all — a full sweep could free, not just move, them.Not a regression from #8216 — its scanner fix is correct; its witness (which had never completed a CI run: the PR's own gc-stress was cancelled in the queue saturation) was the first thing to ever execute this path under forced evacuation.
Fix
The same custody pattern #8216 introduced for ext-http's H2 pump: park the snapshot (+ at most one NaN-boxed payload per frame) in scanned thread-locals (
dispatch_custody.rs), re-read each slot immediately before use so the copying GC's rewrite is observed, pop on drop (RAII, nests for re-entrant pumps). All ten dispatch loops converted.scan_net_rootsvisits the custody stacks; the root-holder census certifies the new holders through that call path.Validation
force_verifyenv (PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1), same freshly built release compiler, auto-opt archive cache wiped between arms:TypeError: value is not a function(macOS spelling of the Linux SIGSEGV — same stale-closure deref), stdout emptyonce fired: 1 of 2 events(byte-correct vs node)cargo test -p perry-ext-netotherwise unchanged (the one pre-existing macOS-host relocation-expectation failure fails identically pre/post-fix and passes on CI Linux)gc_runtime_root_holders.pyandcheck_thread_locals.pyboth green with no inventory edits — the census reaches the new holders via the registered scannercargo fmt --checkcleanOnce merged,
gc-stress(which redspr-gateon every rebased PR via this cell today) goes back to green; PR #8244's failure was this.