fix(signals): unchanged store key read under an adoption hold does not hold the reader (#3706) - #3707
Conversation
🦋 Changeset detectedLatest commit: 8187029 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
4db98b4 to
207e88a
Compare
|
…t hold the reader (solidjs#3706) A derived store adopting a new backing while a transaction is still open puts the whole container under an adoption hold, and readSource's adoption-hold arm entered every deriving read of that container into the transaction, whichever key was read. In the report, confirming one optimistic move while another was pending landed the authoritative array under the still-open shared transaction, and a memo reading `drag() === cards[0].id` beside an independent `drag` signal was held until the second move settled, so isPending(drag) went true. The second reproduction (a nested <Show> preview over `cards.find(...)`) reaches the same arm through an inherited array method. The adoption hold is now key-scoped like the fold hold (solidjs#3688): the keys an adoption changed against the held view are recorded once per adoption (heldKeys, computed on the first held read so re-pointed children compare as one slot: own on both backings, never an accessor, same enumerability, the store's slot equality) and readSource answers a get/has/descriptor read with one set lookup; a key the adoption left alone derives nothing from the hold, no transaction entry and no stale replay. Keyless reads, chained backings, a chained held view, optimistic families and a swapped or non-plain prototype (inherited accessors read through `this`) hold the whole container. Not covered, pinned as an expected failure: a row first read after the adoption inside a deriving memo (the issue's third reproduction) has no target for slot equality, so the container hold stands. Size: +554 B minified on the store read path (the per-read compare variant measured +327 B); brotli +89..+203 B, four caps ratcheted (+ createStore 16.71 -> 16.86 KB, hydrating + every store 30.63 -> 30.76 KB, base page 45.77 -> 45.85 KB, live page 49.96 -> 50.16 KB).
207e88a to
1d62e4b
Compare
|
Switched to the recorded-keys variant and rebased onto
The recorded-keys version is the cheaper read path (one set lookup per held read) and now covers descriptor enumerability and inherited accessors, both pinned; the per-read version is the smaller bundle. One note on placement: the record is computed on the first held read after each adoption rather than inside |
…tes since the hold began (solidjs#3706) The record of keys an adoption changed was recomputed whenever the target's backing changed identity. A mainline setter write during the hold folds on the clone path and replaces the backing, so the key it wrote was counted as the adoption's and held with the adopting transaction: untracked reads kept returning the pre-hold value, and a memo reading the key made an independent signal pending. - adoptPB records the adopted object; the keys are diffed against that object, once, on the first held read (its children are re-pointed by then, so slot equality holds), and replace the entry, so the record no longer retains a backing once computed. - getNode's born-holding path consults the record: a fresh node for a key the adoption left unchanged is not born holding. - readSource checks the key before the context-free / children-forbidden arm, so every reader of an unchanged key is served the backing. Pins for the mainline-write case in three forms (tracked memo, untracked read, a fresh node after the reader drops and re-reads the key); each fails on the previous source. INTERNALS-STORE-STATE states the key scope of the adoption hold. Docblock and comment fixes. The third-shape expected-failure pins reference the solidjs#3706 follow-up. Size ledger re-measured against next @ 7f9bd7a (+578..582 B minified on the store read path); the frozen page caps rise to 45.91 / 50.09 KB, accepted by the maintainer. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
Fixes #3706.
Confirming one optimistic move while another was still pending held a later, independent
dragwrite for the second move's lifetime:latest(drag)became"0",drag()stayed unset andisPending(drag)went true. The second reproduction on the issue (a nested<Show>preview overcards.find(...)) is the same defect reached through an inherited array method.Root cause: when a derived store adopts a new backing while a transaction is still open, the whole container goes under an adoption hold, and the adoption-hold arm of
readSourceentered every deriving read of that container into the transaction, whichever key was read. The fold-hold gate gained per-key precision in #3688 through the trap's written-key record; the adoption hold had no such scoping. In the reproduction the open transaction comes from the two moves being started together, so the second had joined the first's transaction and the first's confirmation landed the authoritative array under a transaction that stayed pending. The memo readcards[0].id, a key the adoption did not change, was held anyway, and took the whole flush, including thedragwrite, with it.Change: the adoption hold is key-scoped the way the fold hold is — the unit of a store hold is the key the transaction touched, not the container.
adoptPBrecords the adopted object (heldKeys, a WeakMap keyed on the target; the target shape's named-field cap rules out a new field), and the keys that adoption changed against the held view are diffed against that object once, on the first held read, then replace the entry.readSourceanswers a get/has/descriptor read with one set lookup, the samehas(key)shape as the fold gate'swkcheck, and a key the adoption left unchanged is served the backing for every reader (including context-free and children-forbidden ones) and is not born holding ingetNode. "Changed" is decided once: own on both backings, never an accessor, same enumerability, the store's slot equality so a re-ingested row is the same logical slot. The diff runs on the first held read rather than insideadoptPBbecause a parent's adoption runs before its children are re-pointed and a diff taken there would call every re-ingested row changed; it runs against the adopted object rather than the live backing because a mainline setter write during the hold replaces the backing, and its key is not the adoption's. Diffing against the held view (the pre-hold committed backing) on each re-adoption gives the union directly: a key that changed and changed back reads the same from either backing. Keyless reads (ownKeys,$TRACK,deep()), chained backings, a chained held view, optimistic families and a swapped or non-plain prototype (inherited accessors read throughthis) hold the whole container. Descriptor enumerability and inherited accessors are covered, with pins;writablealone is not compared.Finished on the branch (maintainer side)
The maintainer chose the recorded-keys variant (one
has(key)rule shared withwk, plus the enumerability and inherited-accessor coverage) and accepted its size cost. Review found one blocker, fixed here rather than sent back:target.vchanged identity, so it measured "changed since the hold began", not "changed by the adoption". WithcreateStore(() => server(), …)adoptingsaved: trueunder an open action,setStore(s => void (s.stable = "edited"))leftuntrack(() => store.stable)at"same", and a memo reading${n()}:${store.stable}made an unrelatednpending. Fixed by anchoring the record to the adopted object, consulting it on the born-holding path ingetNode, and checking the key beforereadSource's context-free / children-forbidden arm. Three pins (tracked memo, untracked read, fresh node after the reader drops and re-reads the key) fail on the previous source; each of the three fix parts is load-bearing for at least one of them.INTERNALS-STORE-STATE.mdstates the key scope of the adoption hold; docblock and comment fixes instore.ts; the third-shape expected-failure pins reference the Confirming one overlapping optimistic-store action holds an independent signal update #3706 follow-up.next(frames cap bump, published-declarations timeout fix) and re-measured the size ledger.Public API changes
None.
Sizes
Final, Rolldown harness against
next@ 7f9bd7a:next+ createStoreCore floor 0 B; every other scenario unchanged.
Variant comparison from the original investigation (against
next@ fb4e637, before the anchoring fix):+ createStore(16,708 B)The recorded-keys variant is the cheaper read path (one set lookup per held read against two
hasOwn, up to four accessor probes and two WeakMap gets) and covers two more cases; the per-read variant is the smaller bundle.Size-Exception: adoption-hold key record on the store read path, accepted by the maintainer — +578..582 B minified, brotli +130..+143 B across the four store-bearing scenarios; frozen page caps 45.77 -> 45.91 KB and 49.96 -> 50.09 KB.
Not covered, pinned as an expected failure and tracked as the #3706 follow-up: the third reproduction on the issue, one action writing the rows before its yield with the preview mounting afterwards, so the row is first read after the adoption inside a deriving memo (
<Show when={cards.find(...)}>). It has no store target on either side, so slot equality cannot call it the same row and the container hold stands. I tried a positional lazy materialization (create the child under the parent's hold with the old raw as its held view, recognize it insameLogicalSlot, seed the parent's slot node with the adopted raw); it passed the pins, but review reproduced three defects against it: a reorder of unread rows read[a, a]from a handler,cards[0]resolved to different proxies by reader context because committed-view readers wrap the old raw, and a child created under alatest()hold never cleared it. The same read from a render effect (a stale reader) publishes fine; only the deriving-memo form holds.How did you test this change?
Signals pin
packages/signals/tests/adoption-unchanged-key-read-3706.test.ts(19 cases: 17 passing, 2 expected failures). Passing: the issue shape, thecards.findform, a stale render effect, presence reads, a plain derived store, a mainline setter write during the held adoption in three forms (tracked memo, untracked read, fresh node), A29 contrasts for a changed key, a changed leaf of a lazily read row, an added or deleted key, a prototype swap, reordered unread rows, a shallow store, a descriptor read whose enumerability changed and an inherited accessor readingthis, plus a no-store-read control. Expected failures: the third shape in both forms. Web specs:packages/web/test/optimistic-confirm-holds-signal-3706.spec.tsx(first playground as a memo x confirm-first matrix; only memo + confirm-first failed before),packages/web/test/optimistic-nested-show-drag-3706.spec.tsx(second playground; thefindvariant failed against the base store source, the plain-span control passed on both) andpackages/web/test/optimistic-lazy-show-preview-3706.spec.tsx(third playground;findexpected-fail,cards[0].idfrom the insert effect and a plain control pass).Five review rounds plus four
codex exec review --base origin/nextruns on the original branch, then a maintainer-side review. Findings that reproduced against the gate (a setter write to another key during the held adoption being hidden, prototype swaps, a mainline write during the hold being held) are fixed and pinned; the findings against the lazy materialization (reorder aliasing, split proxy identity, an uncleared latest-pull hold) are why it was dropped.🤖 Generated with Claude Code; finished with Cursor