Skip to content

fix: end tracking when a selector throws - #667

Open
hugocxl wants to merge 1 commit into
LegendApp:mainfrom
hugocxl:fix/track-selector-throw
Open

hugocxl wants to merge 1 commit into
LegendApp:mainfrom
hugocxl:fix/track-selector-throw

Conversation

@hugocxl

@hugocxl hugocxl commented Oct 1, 2026

Copy link
Copy Markdown

Summary

trackSelector runs beginTracking() → selector → endTracking() with nothing between them guarding against a throw. When the selector throws, endTracking() never runs, and the tracker it created stays behind as tracking.current with trackCount above zero.

That is not a theoretical path. observer renders the whole component as a selector, so any observed component that suspends (a thrown promise from a third-party hook such as TanStack Query's useSuspenseQuery) or throws to an error boundary leaves one of these orphans. From that moment on, every get() anywhere in the app that runs outside a selector (effects, event handlers, plain code) records its node in the orphan's nodes Map, which nothing ever reads or disposes. Each node retains its root, so whole observable trees are pinned for the lifetime of the page.

This PR wraps the selector call in try/finally so tracking always ends, and adds two regression tests.

How we found it

Profiling a React/Capacitor app where a coach switches between athletes, each with a large workout observable (~10k leaf values). Every switch retained the previous athlete's workout, about 13.5 MB each; the heap went 111 → 165 MB after four switches and never came back. With this change the heap is flat. The observer wrapping a useSuspenseQuery call was the trigger, and tracking.current was already a stale tracker right after app load.

The fix

beginTracking();
try {
    value = selector ? computeSelector(selector, getOptions, observeEvent, observeOptions?.fromComputed) : selector;
} finally {
    tracker = tracking.current;
    nodes = tracker!.nodes;
    endTracking();
}

Behaviour is unchanged when the selector returns. When it throws, tracking is balanced and the error (or Suspense promise) still propagates exactly as before, so React handles it the same way. This mirrors what batch() already does around beginBatch/endBatch, and what runInRender does for renderDepth (added in #638 for the same reason: "thrown selectors or nested scopes do not poison later updates").

Tests

  • tests/tracking.test.ts: a selector that throws restores the outer tracking context and later get() calls land there, not in a leaked tracker.
  • tests/react.test.tsx: an observer component that suspends on first render (a promise thrown from inside the observed render, as a data library's suspense hook would) leaves tracking.current undefined, both while suspended and after it resolves.

Both tests fail on main and pass with this change. Assertions compare booleans and sizes on purpose: printing a leaked tracker in a failure message walks its proxies forever.

eslint and prettier pass locally. The full jest suite passes except for one flake of the wall-clock Lazy activation perf2 test under the parallel run (37 ms against its 25 ms local budget); it measures a plain get() that never enters trackSelector, and it passes in isolation on both main and this branch. tsc --noEmit reports only the pre-existing errors in src/sync-plugins/supabase.ts and tanstack-query.ts, none in the touched files.

🤖 Generated with Claude Code

trackSelector ran beginTracking → selector → endTracking with nothing
guarding the throw path. A selector that throws — an observer's render
suspending on a thrown promise, or erroring to a boundary — skipped
endTracking and left its tracker as tracking.current with trackCount
above zero. Every later untracked get() then recorded its node in that
orphan forever, retaining whole observable trees.

Wrap the selector in try/finally so tracking always ends, and add
regression tests at the trackSelector and observer + Suspense levels.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant