Skip to content

fix(signals): re-adopt the queue batch when an action completes - #2950

Merged
ryansolid merged 1 commit into
solidjs:nextfrom
brenelz:fix/action-done-window-batch-adoption
Jul 28, 2026
Merged

fix(signals): re-adopt the queue batch when an action completes#2950
ryansolid merged 1 commit into
solidjs:nextfrom
brenelz:fix/action-done-window-batch-adoption

Conversation

@brenelz

@brenelz brenelz commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

When an async-generator action completes, done() restored the active transition with a bare setActiveTransition(ctx), leaving globalQueue._batch as a detached ambient batch until the scheduled flush ran. Anything registered in that microtask window landed in a batch nothing would ever finalize (the same window as the #2916 race, whose fix only protected that batch's _pendingNodes):

  • A completed action's writes were silently lost when another in-flight action resumed inside the window: mergeTransitionState moves every list except _pendingNodes (relying on the batch-adoption pass in initTransition, which only sees the queue's batch — here the detached ambient one), so the restored transition's held writes were orphaned. Dev invariant INV-7 ("value can never commit", fix(server): reset sync memo child state on re-pull so hydration keys stay aligned #2827 class).
  • A bare optimistic write in the window never reverted — it landed in the detached batch's _optimisticNodes, which the completing flush never resolves and the end-of-flush reschedule check never sees. Dev invariant INV-6.
  • An affects() mark registered in the window could leak forever, leaving isPending stuck true with an explicit flush() as a no-op. Dev invariant INV-10.

Fix

One line (plus a comment): done() now goes through globalQueue.initTransition(ctx) — the same merge-and-adopt path every other transition-resumption site (async resolution, optimistic replay, zombie recompute) already uses. That adopts the ambient batch into the restored transition, so window registrations land somewhere that settles. With the batch adopted, the next action's initTransition also transfers and re-stamps the restored transition's pending nodes, which closes the merge orphan without touching mergeTransitionState.

One deliberate semantic alignment: an ordinary write landing after done() but before the flush now joins the active transaction — the normal rule everywhere else a transaction is active — instead of staying ambient. The #2916 keep-the-ambient-batch branch in flush still passes its race test and remains as insurance.

Tests

tests/action-done-window.test.ts covers all three symptoms by polling microtasks until the restored transition is observable and injecting the work exactly in the window. The first two fail deterministically on next without the fix (INV-7 firing); the third symptom needs additional in-flight work to become observable but is guarded here as well. Full @solidjs/signals suite (1,149 tests) and test-types pass with the fix.

🤖 Generated with Claude Code

done() restored the active transition with a bare setActiveTransition,
leaving globalQueue._batch as a detached ambient batch until the scheduled
flush. Anything registered in that microtask window was stranded with
nothing to finalize it:

- a completed action's held writes were silently lost when another action
  resumed in the window — the transition merge moves every list except
  _pendingNodes, and the batch-adoption pass only sees the queue's batch
  (INV-7, solidjs#2827 class)
- a bare optimistic write never reverted (INV-6)
- an affects() mark could leak, leaving isPending stuck true (INV-10)

Completing an action now goes through initTransition, the same
merge-and-adopt path every other transition-resumption site already uses.
With the batch adopted, the next action's initTransition also transfers
and re-stamps the restored transition's pending nodes, closing the merge
orphan without touching mergeTransitionState.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 82d61b4

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
solid-js Patch
babel-preset-solid Patch
@solidjs/web Patch
@solidjs/html Patch
@solidjs/h Patch
@solidjs/universal Patch
@solidjs/element Patch
hackernews-spa-example Patch
hackernews-example 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 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 117 skipped benchmarks1


Comparing brenelz:fix/action-done-window-batch-adoption (82d61b4) with next (63cd066)

Open in CodSpeed

Footnotes

  1. 117 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@ryansolid
ryansolid merged commit 9af6670 into solidjs:next Jul 28, 2026
4 of 5 checks passed
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