fix(gc): error side tables get move/finalize hooks + a user-props root scanner#5978
Conversation
|
Warning Review limit reached
Next review available in: 13 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR adds error side-table GC support: new ChangesError side tables GC integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GC as GC
participant Types as gc/types.rs
participant Diagnostics as diagnostics_gc.rs
participant GCInit as gc_init
participant Visitor as RuntimeRootVisitor
GC->>Types: gc_type_after_payload_move(old_user, new_user)
Types->>Diagnostics: error_side_tables_owner_moved(old_user, new_user)
GC->>Types: gc_type_finalize_unmarked_payload(user_ptr)
Types->>Diagnostics: error_side_tables_clear_dead(user_ptr)
GC->>Diagnostics: finalize_dead_copied_minor_from_space_errors()
Diagnostics->>Diagnostics: identify dead GC_TYPE_ERROR entries and clear them
GCInit->>Diagnostics: gc_register_mutable_root_scanner(scan_error_user_props_roots_mut)
Diagnostics->>Visitor: visit_nanbox_u64_slot for ERROR_USER_PROPS Bits entries
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…t scanner
Errors are MOVABLE arena objects, and all seven address-keyed side tables
in node_submodules::diagnostics (ERROR_MESSAGE_{CODES,SYSCALLS,ERRNOS,
PATHS,DESTS,HOSTNAMES} + ERROR_USER_PROPS) were GC-blind — the in-code
'stale entries are harmless' comment was wrong three ways:
1. A MOVED error's entries stayed keyed by the old address, so err.code /
err.syscall / user-assigned props VANISHED after an evacuating cycle.
→ GcMoveHookKind::ErrorSideTables rekeys all seven on move.
2. A SWEPT error's entries persisted, so a fresh error allocated at the
recycled address INHERITED the dead error's codes/props.
→ GcFinalizeHookKind::ErrorSideTables (old-gen sweep), the clear-dead
side-table arm, and a copied-minor from-space finalize mirroring the
map/set pattern.
3. An OBJECT-valued user prop (err.cause = {...}) was stored as raw bits
invisible to GC — collectable while reachable through the error, and
left dangling after its referent moved.
→ scan_error_user_props_roots_mut registered as a mutable-root scanner
(tag-aware visit_nanbox_u64_slot; numeric/boolean bits untouched).
Three unit tests: move-rekey (stale key also proven gone), dead-entry
cleanup, and object-valued-prop root+rewrite.
… stay under the 2000-line gate
…class inventory gate)
a06b89e to
8b9aada
Compare
1 participant
Third GC deep-set slice from the 2026-07-02 audit. Errors are MOVABLE arena objects, and all seven address-keyed side tables in
node_submodules::diagnostics(ERROR_MESSAGE_{CODES,SYSCALLS,ERRNOS,PATHS,DESTS,HOSTNAMES}+ERROR_USER_PROPS) were GC-blind — the in-code 'stale entries are harmless' comment was wrong three ways:err.code/err.syscall/ user-assigned props vanished after an evacuating cycle. →GcMoveHookKind::ErrorSideTablesrekeys all seven tables on move.GcFinalizeHookKind::ErrorSideTables(old-gen sweep) + the clear-dead side-table arm + a copied-minor from-space finalize mirroring the map/set pattern.err.cause = {...}stored raw bits — the referent was collectable while reachable through the error, and dangled after a move (the devils-advocate suite's err.cause failure signature). →scan_error_user_props_roots_mutregistered as a mutable-root scanner;visit_nanbox_u64_slotis tag-aware so numeric/boolean bits are untouched.Three unit tests (move-rekey incl. stale-key-gone, dead-entry cleanup, object-valued-prop root+rewrite) plus the type-metadata contract update. Full GC unit suite green (386).
Code-only; metadata folded at merge.
Summary by CodeRabbit