[Narek-Khachikyan]

Summary

Retired links in old space can retain younger stopped effects through stale prevSub / nextSub pointers across minor GCs, until a major GC reclaims them. Clear those subscriber pointers after removal, deferring cleanup until the outermost active propagation finishes so an onTrigger callback cannot truncate a saved traversal. Refresh subscriber heads after callbacks or computed evaluation can remove them. Preserve prevDep, nextDep, and unlink()'s returned dependency successor.

Targets minor: the affected 3.6 reactivity implementation differs from stable 3.5.

Closes #16176

Evidence

• Before: detachment assertions and the stopped-payload minor-GC regression fail without the fix. After: focused effect/computed/scope/GC coverage passes (166 passed, 2 existing todo), including removal during onTrigger, computed evaluation, and scope resume.
• Final head, Node 26: CI=true pnpm test-unit --maxWorkers=2 passes all 288 files: 9334 passed, 2 expected fail, 3 skipped, 17 todo. Explicit GC run: 5 passed, 2 existing todo, no skips; the minor-GC regression ran.
• Typecheck, lint, formatting, and diff checks pass.

Merge Danger

Reversible internal reactivity change; no exported API change. Cleanup adds work to propagation. The final default 30 ms computed timing guard passed, but earlier local runs failed around 35–75 ms; CI uses the unchanged 100 ms bound. No zero-overhead claim.

Earlier Node 24 deterministic coverage passed, but minor-GC capability was skipped there. No final Node 24/22 full matrix, browser/e2e, ecosystem, bundle-size, or built-declaration validation was run locally.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 2d8fe7fa-81df-4309-b1f7-755193e7eceb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[volynetstyle]

Thanks for taking this further :)
I think the resulting fix exposes a slightly deeper issue than the original GC retention itself.

The original invariant I was aiming for was simple:

once a Link is removed from the graph, it should no longer retain pointers into the live subscriber list.

But the current propagation algorithm cannot always satisfy that immediately, because a removed Link may still be part of the traversal state. In other words, prevSub / nextSub are not only graph topology here; a logically dead Link can still act as a continuation for an active propagation.

That is why this PR now needs a second lifetime:

live Link
  -> logically unlinked, but still retained by propagation
  -> physically detached after the outermost propagation finishes

and therefore a new mechanism:

propagationDepth
unlinkedDuringPropagation

Conceptually, this is a small deferred-reclamation layer protecting the live traversal from Links that are already dead from the graph's point of view.

I don't think that makes this PR wrong. The added tests around onTrigger, computed evaluation and nested propagation are exactly the cases that need to be covered if the current traversal design is kept.

But it does reveal an important coupling in the representation: graph links are also being used as ephemeral execution state. Clearing stale topology therefore becomes observable to the traversal algorithm.

A design where traversal continuation is captured separately before entering user/reactive code would not need dead Links to remain partially traversable. For example, the general shape would be:

const next = link.nextSub
// code here may unlink `link`
link = next

or an explicit work stack containing the continuation itself, rather than later recovering it from a Link that may already have been removed.

So I would view propagationDepth + unlinkedDuringPropagation not only as the fix for #16176, but also as evidence of a more general invariant worth documenting:

persistent graph topology and temporary traversal state currently overlap.

That overlap is also why the PR has to refresh dep.subs / sub.subs after callbacks and computed evaluation: user code is allowed to mutate the same structure that the propagation walk is currently using as its continuation.

The current patch looks like a reasonable compatibility-preserving solution. I just think the need for deferred reclamation is itself useful information about the architecture, and worth keeping in mind if system.ts is refactored further.

[Narek-Khachikyan]

Graph membership and traversal continuation have different lifetimes in the current implementation. A walk can already hold the next link when user code removes it; saving the current link's successor alone does not cover removal of that saved successor. The first-onTrigger-stops-second regression protects exactly that case. This patch preserves those continuation edges only until the outermost synchronous propagation exits, with cleanup in finally for nested and throwing paths. The subscriber-head refreshes address the same mutation boundary. Separating continuation state from topology would be a broader traversal change; I have kept this fix within the existing model.

[volynetstyle]

Graph membership and traversal continuation have different lifetimes in the current implementation

You're right that saving only link.nextSub is not sufficient if that saved successor can itself be removed before the traversal resumes. The stronger distinction I had in mind is that traversal continuation must remain valid independently of subsequent topology mutations, not merely be copied out of the current Link. So I agree with the scope of this patch: given the current traversal model, deferred reclamation is the compatibility-preserving fix. My broader point was only that needing it exposes the coupling clearly: propagation is allowed to retain logically removed topology because that topology still carries continuation state.