Skip to content

fix(signals): isPending companions keep their pending override while the source is in flight (closes #2843)#2845

Closed
yumemi-thomas wants to merge 1 commit into
solidjs:nextfrom
yumemi-thomas:fix/ispending-companion-retention
Closed

fix(signals): isPending companions keep their pending override while the source is in flight (closes #2843)#2845
yumemi-thomas wants to merge 1 commit into
solidjs:nextfrom
yumemi-thomas:fix/ispending-companion-retention

Conversation

@yumemi-thomas

Copy link
Copy Markdown

Closes #2843

Summary

createEffect(() => isPending(() => latest(asyncMemo)), …) — the canonical stale-while-revalidate watcher — sends the scheduler into an infinite loop on the first source write after the async memo settles: Potential Infinite Loop Detected in dev after a visible stutter, and an unbounded hot spin in prod, where the iteration guard doesn't exist. One refetch click is enough. The same loop reproduces in the store-leaf shape from #2831 (isPending(() => latest(() => store.value)) over a refetching derived store) and with two user effects watching the same memo. As a side effect of the same defect, the pending flag never turns true at any point.

Root cause

Traced by instrumenting each edge (per-pass counts during one spin in parentheses):

  1. isPending's probe subscribes the user effect to the source's _pendingSignal.
  2. The latest() shadow computed recomputes (×100k), hits NotReadyError, and lane bookkeeping calls updatePendingSignalsetSignal(pendingSignal, true) (×100k).
  3. The pending signal is an optimisticSignal, so the write registers with the ambient transition — which completes within the same pass, and resolveOptimisticNodes reverts the override back to false, re-notifying subscribers.
  4. The effect re-runs (×100,001), the shadow recomputes, the write happens again — forever. The signal's visible value never leaves false (instrumented: 100,004 consecutive false → true writes), which is also why isPending never reads true.

The pending flag's true is written into a transition that reverts it while the async it describes is still in flight — the revert is the re-notification that sustains the loop.

Fix

The transition-scoped write itself appears intentional — a variant that commits the companion write directly (bypassing the optimistic machinery) also fixes the loop but breaks three behaviors pinned in latest-isPending-consistency.test.ts. So only the revert timing changes: companion signals now carry a revert-deferral policy, and resolveOptimisticNodes consults it before reverting.

  • getPendingSignal attaches sig._deferRevert: the source is resolved at check time (parent/firewall for latest() shadows — a shadow can be held by a wider transition after its own source resolved, so its own state is the wrong thing to consult; and firewall/parent links can be established after the signal first exists) and must be non-disposed and strictly in-flight (STATUS_PENDING && !STATUS_UNINITIALIZED).
  • resolveOptimisticNodes defers nodes whose policy holds: clears their stale _transition pointer (a later companion write would otherwise initTransition() a completed transition back to life — _done === true is not followed by currentTransition), parks them alias-safely in globalQueue._optimisticNodes (initTransition adopts-and-aliases that array, so the park must not push into the array being iterated, and repeated adopt/defer cycles must not accumulate duplicates — verified: the array is back to length 0 after every one of 40 park/settle cycles), and lets them revert through the normal path once the source settles. A source disposed mid-flight falls through to the existing revert-to-false safety net.

Checking the resolved companion source rather than the shadow's transition-held state is also what makes the flag clear when the memo's own async resolves while a sibling still holds the transition.

An alternative shape would scope the companion write to the source's transition at write time instead of deferring the revert — happy to rework in that direction if preferred.

Performance

For apps not using isPending, the new code is unreachable by construction: resolveOptimisticNodes loops over empty arrays and the policy closure only allocates when isPending first touches a source. Measured write/flush paths at parity (±0.2%, overlapping spreads across isolated runs). A 60-cycle refetch soak with mid-flight unrelated flushes (the old kill condition, repeatedly) stays clean, and the GC suite shows no delta versus pristine.

Tests

Seven tests in tests/isPending-companion-retention.test.ts, six failing red-first on current next (the seventh — direct isPending(x) — is the deliberate control pinning that the bug was latest-wrap-specific):

  • the headline loop: pending observable mid-flight, clears at settle, zero uncaught errors
  • pending clears when the memo's own async resolves while a slower sibling is still in flight
  • the companion keeps its override across an unrelated write's flush completing mid-flight (the core defect, isolated)
  • the store-leaf shape and the two-watcher shape (both looped before)
  • the disposal safety net: a source disposed mid-flight reverts the companion to false

Full solid-signals suite: the diff versus pristine is exactly the intended flips (the loop variants plus the two pending-flag behaviors), nothing else; latest-isPending-consistency.test.ts (#2831) passes throughout; solid and @solidjs/web suites (client, hydration, and server configs) rebuilt against the fixed package and re-run at their baselines.

Full disclosure: I wrote this with the help of an AI assistant (Claude Fable 5) and reviewed every line before pushing. All new tests were written first and confirmed failing on the unfixed code, and the root cause was established by instrumenting the running scheduler rather than inferred.

@changeset-bot

changeset-bot Bot commented Jul 7, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 55019b4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@solidjs/signals Patch
test-integration Patch
solid-js Patch
babel-preset-solid Patch
@solidjs/web Patch
@solidjs/html Patch
@solidjs/h Patch
@solidjs/universal Patch
@solidjs/element Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codspeed-hq

codspeed-hq Bot commented Jul 7, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 120 untouched benchmarks


Comparing yumemi-thomas:fix/ispending-companion-retention (55019b4) with next (3755e67)

Open in CodSpeed

@yumemi-thomas yumemi-thomas force-pushed the fix/ispending-companion-retention branch 2 times, most recently from d4325ca to 3f45037 Compare July 7, 2026 13:05
@yumemi-thomas yumemi-thomas force-pushed the fix/ispending-companion-retention branch from 3f45037 to 55019b4 Compare July 7, 2026 13:06
@ryansolid

Copy link
Copy Markdown
Member

Thanks for this — the diagnosis of the revert/re-arm cycle was right, and your regression tests were genuinely useful: we ran them against the companion redesign that landed on next (#2838, which makes the isPending/latest companions purely write-driven with settlement-checkpoint re-derivation) and 6 of 7 pass without any deferral machinery — the loop in #2843 is fixed structurally rather than deferred around.

We're not taking the _deferRevert approach itself: holding the override and probing transitions from generic async bookkeeping conflicts with an invariant we hold internally (async blockers register only from render-effect notification paths), and with the redesign in it would be dead weight. The one test of yours that still fails — a source disposed while in flight leaving its companion latched true — is a real edge you found, and we're fixing it in-model (companion reverts on owner disposal) rather than via deferral. Closing since #2843 is resolved on next; the disposal edge is tracked from your test case. Appreciate the careful work here.

@ryansolid ryansolid closed this Jul 7, 2026
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.

2 participants