Skip to content

Close the ambient transaction window when a transaction opens without writes (#3141) - #3145

Merged
ryansolid merged 2 commits into
nextfrom
fix/ambient-transaction-capture
Aug 31, 2026
Merged

Close the ambient transaction window when a transaction opens without writes (#3141)#3145
ryansolid merged 2 commits into
nextfrom
fix/ambient-transaction-capture

Conversation

@ryansolid

Copy link
Copy Markdown
Member

Summary

Fixes #3141.

  • The defect: parking of an incomplete transaction is flush-driven, but a transaction opened with no writes before its first suspension — an action whose first statements only await — scheduled nothing. activeTransition and the adopted batch stayed armed across the async gap, and the next unrelated work to arrive was adopted into a transaction it had nothing to do with. In 2.0.0-rc.4 | optimistic store, read in an effect, changes the visible rendering. and has inconsistent value when logged and when rendered. #3141's playground: the optimistic store's authoritative landing (on a still-live lane, since the generator keeps running past the yield) froze until the stranger 10s action settled; an unowned optimistic push rode that transaction instead of reverting at the flush; and deep()/per-key readers split-brained — the DOM showed [1,2,3,1111] (stale base + override) while the deep observer reported [3,2,1,1111].
  • The fix: initTransition ends by scheduling a flush. The ambient window closes in one flush regardless of whether the transaction wrote anything. This is enforcement of the existing A26 containment ruling (SPEC-ASYNC-SEMANTICS, 2026-07-17, 2.0.0-beta.19: action(): writes after an internal await escape the transaction and commit mid-flight #2913), which explicitly rejected holding activeTransition open across await windows because "unrelated ambient writes… would be captured into the action's transaction and held until it settles" — pre-fix, that rejected behavior happened anyway through this gap. A26's accepted consequence (pre-await _transition stamps rejoining) is untouched.
  • Regression pins: a deterministic gated signals test (optimistic-ambient-capture.test.ts) built on the two exact ingredients the capture needs — a bare-macrotask action open, and a store flight that stays pending past its yield (fails pre-fix on the captured landing, passes post-fix); a faithful web/jsdom port of the playground with real scaled timers and order-based assertions; and an observer-invariance suite. Spec + internals docs updated with the enforcement note.
  • Also lands an orphaned tests-only pin from the navigate() from flight-data consumer can silently drop a relative-Location redirect (reliable with absolute URLs) #3107 router thread (transition commit when a handled downstream async error resolves it).

Follow-ups filed separately: the store's truth-flight owning its own transaction, and the deep()-vs-per-key committed-read seam.

Test plan

  • optimistic-ambient-capture.test.ts fails pre-fix (landing captured), passes post-fix
  • Full signals suite (1434) x4, web suite (672), solid-js, compiler — green
  • Playground port reproduces pre-fix at both signals and web layers; correct timeline post-fix

Made with Cursor

@changeset-bot

changeset-bot Bot commented Aug 31, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0d3e2d1

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
solid-js Patch
@solidjs/babel-plugin Patch
@solidjs/compiler Patch
@solidjs/html Patch
@solidjs/h Patch
@solidjs/universal Patch
@solidjs/element Patch
@solidjs/diagnostics 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

coveralls commented Aug 31, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 33370625606

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage remained the same at 71.652%

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: 1003
Covered Lines: 767
Line Coverage: 76.47%
Relevant Branches: 789
Covered Branches: 517
Branch Coverage: 65.53%
Branches in Coverage %: Yes
Coverage Strength: 14.54 hits per line

💛 - Coveralls

@codspeed-hq

codspeed-hq Bot commented Aug 31, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 18.34%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 133 untouched benchmarks
⏩ 132 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
merge 225.1 µs 329.2 µs -31.63%
merge 265.2 µs 366.5 µs -27.65%
reconcile: deep tree, 10 of ~12k paths subscribed 3.8 ms 3.4 ms +10.11%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing fix/ambient-transaction-capture (0d3e2d1) with next (28a1eaf)

Open in CodSpeed

Footnotes

  1. 132 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 and others added 2 commits August 31, 2026 00:55
… writes (#3141)

Parking is flush-driven, but a transaction opened with no writes before its
first suspension — an action whose first statements only await — scheduled
nothing, so activeTransition and the adopted batch stayed armed across the
async gap and captured the next unrelated work to arrive: an optimistic
store's landing on a still-live lane was held until the stranger action
settled, an unowned optimistic write rode that transaction instead of
reverting, and deep()/per-key readers split-brained meanwhile. initTransition
now guarantees a flush, enforcing the A26 containment ruling: the ambient
window is one flush, period.

Co-authored-by: Cursor <cursoragent@cursor.com>
… thread

A source write entangled with downstream async that rejects must still
commit — the error boundary handling the rejection is the transition
resolving, not it failing. This pin came out of the router navigation
investigation and was never landed; tests only, no changeset.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ryansolid
ryansolid force-pushed the fix/ambient-transaction-capture branch from 7b61904 to 0d3e2d1 Compare August 31, 2026 07:56
@ryansolid
ryansolid merged commit 2023daa into next Aug 31, 2026
6 of 8 checks passed
@ryansolid
ryansolid deleted the fix/ambient-transaction-capture branch August 31, 2026 09:40
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