Skip to content

fix(signals): optimistic settle verdicts — combined isPending on the child lane, onSettled after the revert re-derive (#3409, #3411) - #3425

Merged
ryansolid merged 1 commit into
nextfrom
fix/optimistic-settle-verdicts
Sep 14, 2026
Merged

ryansolid merged 1 commit into
nextfrom
fix/optimistic-settle-verdicts

Conversation

@ryansolid

Copy link
Copy Markdown
Member

Fixes #3409, fixes #3411 — the two optimistic-bookkeeping issues from gabbev's latest batch. Both are small, and both were reproduced core-level (manual clock) before fixing; the pins are in optimistic-settle-verdicts.test.ts and fail on next.

#3409 — combined isPending lags the individual probes

isPending(() => [a(), b()]) over two async siblings of an optimistic value reported false while isPending(a) and isPending(b) both reported true, and flipped to true only once a landed.

Traced: the combined effect computes true at the write, but its run is withheld — it ends up on the held value lane. Sequence in assignOrMergeLane: ps(a)'s child lane is assigned → a's parent-lane notification keeps the child (the parent/child carve-out) → ps(b)'s child lane is a sibling, so the two child lanes merge → b's parent-lane notification then hits the "subscriber's lane was merged → replace with the source lane" shortcut, which skips the parent/child check and puts the effect on the held parent. fxA/fxB each have a single child lane and never reach that path.

Fix: delete the shortcut. A merged lane is followed to its root (findLane) like any other and goes through the same parent/child logic — the root is a child of the source root, so the effect keeps it. −33 B on the core floor.

The pin runs through the revert too: there the three indicators clear together when the held reload commits, which matches the plain-signal baseline.

#3411onSettled reads Value: 0, Copy: 1 during the revert

The settle drops the override in the commit pass but only enqueues the reverted subscribers (_resolveOptimisticinsertSubs, likewise the contested/gated replays and store clears); the pass after re-derives them, and reads do not pull (prepareComputed(el, false)). An unowned onSettled is a one-shot in the commit pass's user phase, so it read value already reverted next to copy still holding 1.

Fix: the unowned fire waits for the heap to drain — while dirtyQueue has work it re-enqueues itself (run swaps the queue array, so the re-enqueue lands in the next pass; enqueue keeps the drain alive). That is what settled means: no derivation outstanding. Also covers gabbev's no-action variant (ambient optimistic write reverting at the end of its own flush), pinned separately.

I first tried the other lever — re-running the heap inside finalizePureQueue after _resolveOptimistic so the commit pass itself is consistent. It breaks three things that rely on the next-pass re-derive: the lane frame of an optimistic write completing in its own flush (applied by cleanupCompletedLanes; [0, 2, 0] became [0, 0]), the store optimistic-layer clears (the #3164 test produced a torn RUNTIME ERROR frame), and a gated replay. The internals doc records why the observer-side fix is the right one.

Verification

Co-authored with Claude via Cursor.

…child lane, onSettled after the revert re-derive (#3409, #3411)

optimistic value reported false while `isPending(a)` and `isPending(b)`
both reported true. The combined effect computed `true` but its run was
withheld: its two companion child lanes are siblings and merge, and the
parent lane's next notification then hit assignOrMergeLane's "subscriber's
lane was merged → take the source lane" shortcut, which skipped the
parent/child check and moved the effect onto the held parent lane, where
its verdict waited on the async it reports. A merged lane is now followed
to its root like any other and goes through the same check. -33 B on the
core floor.

revert. The settle drops the override in the commit pass but only
enqueues the reverted subscribers; the pass after re-derives them, and
reads do not pull. The one-shot fired in the commit pass's user phase saw
the source reverted beside a stale memo. The fire now waits for the heap
to drain (re-enqueues itself while `dirtyQueue` has work; `run` swaps the
queue so it lands in the next pass). Forcing the re-derive into the
commit pass instead breaks the lane frame of a write completing in its
own flush and the store clears (#3164 tear) — recorded in the internals
doc.

Pins for both (plus the no-action variant of #3411) in
optimistic-settle-verdicts.test.ts.

Co-authored-by: Claude <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9fa294f

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

This PR includes changesets to release 11 packages
Name Type
@solidjs/signals Patch
test-integration Patch
@solidjs/web Patch
@solidjs/babel-plugin Patch
@solidjs/compiler Patch
@solidjs/diagnostics Patch
@solidjs/element Patch
@solidjs/h Patch
@solidjs/html Patch
solid-js Patch
@solidjs/universal 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

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 34830828074

Coverage remained the same at 71.842%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 1007
Covered Lines: 772
Line Coverage: 76.66%
Relevant Branches: 790
Covered Branches: 519
Branch Coverage: 65.7%
Branches in Coverage %: Yes
Coverage Strength: 15.03 hits per line

💛 - Coveralls

@codspeed

codspeed Bot commented Sep 14, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 160 untouched benchmarks


Comparing fix/optimistic-settle-verdicts (9fa294f) with next (1643d2a)

Open in CodSpeed

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