fix(db): nested toArray includes drop children when sibling groups share a correlation key (#1501)#1607
Conversation
…#1501) Adds a failing test reproducing TanStack#1501: with three collection levels (products -> priceRanges -> region), when two priceRanges in different parent groups share the same deepest correlation key (regionId === 1), one of the two nested `region` arrays comes back empty. The nested pipeline buffer is shared by reference across per-parent-group states (createPerEntryIncludesStates) and drainNestedBuffers deletes a buffer entry after routing it to the first matching parent group, so the sibling that drains second finds nothing. Note: the minimal repro in the issue does not trigger the bug as written (its dummy `eq(p.id, _.id)` correlation against a single-row anchor with findOne collapses to one product, so the two overlapping siblings never coexist in the output). This test puts both sibling groups in the result so the collision actually occurs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNested include routing now tracks multiple parents per deepest correlation key, replays drained nested rows from snapshots, and adds regression coverage for shared-key three-level includes across preload, insert, delete, live collection, and materialized variants. ChangesNested toArray fan-out and snapshotting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)packages/db/src/query/live/collection-config-builder.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.10][ERROR]: unable to find a config; path packages/db/tests/query/includes.test.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.10][ERROR]: unable to find a config; path Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
…ation key (TanStack#1501) With 3+ levels of nested toArray includes, when two children in different parent groups shared the same deepest correlation key, only one received the nested rows and the other came back empty. Two compounding causes: - nestedRoutingIndex mapped each nested correlation key to a single parent group (last-writer-wins), and the shared buffer entry was deleted after routing to the first match, so sibling groups sharing the key were dropped. - the nested pipeline does not re-emit already-materialized rows, so a parent group that starts referencing an existing correlation key after the rows were drained (e.g. a sibling inserted after the initial load) saw nothing. Fixes: - nestedRoutingIndex now maps a nested correlation key to a Set of parent groups; drainNestedBuffers fans buffered grandchild changes out to every ready parent group before dropping the buffer entry. - a per-level cumulative snapshot of net-present grandchild rows seeds late-arriving parent groups from what their siblings already received. Routing-index inserts/deletes and parent-delete cleanup are updated to maintain the per-key parent sets. Adds tests covering initial load, a sibling inserted after load, and deleting one of two siblings that share a correlation key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kevin-dp
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/db/tests/query/includes.test.ts (1)
5078-5079: 🩺 Stability & Availability | 🔵 TrivialReplace the fixed 50ms sleeps with a deterministic wait helper.
flushPromises()is already available in the test utilities, and the same change should be applied to the matching delete case at line 5160.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/includes.test.ts` around lines 5078 - 5079, The tests in `includes.test.ts` use fixed 50ms sleeps after `priceRanges.insert(...)`, which should be replaced with the deterministic `flushPromises()` helper already available in the test utilities. Update the insert case and the matching delete case to await `flushPromises()` instead of `setTimeout`, keeping the same behavior while removing timing flakiness.Source: Coding guidelines
packages/db/src/query/live/collection-config-builder.ts (1)
1153-1158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid adding new
anyto snapshot state.
SnapshotRow.valueand the new helper signature can useunknownhere; the code only stores/replays the value and does not need unchecked access. As per coding guidelines,**/*.{ts,tsx}: Avoid usinganytypes; useunknowninstead when the type is truly unknown.♻️ Proposed type tightening
type SnapshotRow = { - value: any + value: unknown orderByIndex: string | undefined /** Net multiplicity (inserts − deletes) currently materialized for this row */ count: number } @@ function accumulateSnapshot( setup: NestedIncludesSetup, nestedCorrelationKey: unknown, - childChanges: Map<unknown, Changes<any>>, + childChanges: Map<unknown, Changes<unknown>>, ): void {Also applies to: 1373-1377
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/query/live/collection-config-builder.ts` around lines 1153 - 1158, The snapshot state is introducing an unnecessary any type in SnapshotRow.value and the related helper signature, even though the value is only stored and replayed. Tighten the types in collection-config-builder by changing SnapshotRow and the affected helper used around the snapshot logic to use unknown instead of any, and keep the existing handling unchanged except for type-safe narrowing where needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/src/query/live/collection-config-builder.ts`:
- Around line 1387-1399: Clone snapshot values before assigning them into row
state in collection-config-builder’s snapshot/replay logic, because row.value is
currently sharing the same object as changes.value and later mutation of
changes.value can strip routing metadata. Update the row initialization and
replay paths in the affected update/merge flow (including the shared logic
around the count/orderByIndex handling) to store a cloned copy instead of the
original object, so later cleanup cannot affect previously buffered rows and
nested routing can be rebuilt correctly.
---
Nitpick comments:
In `@packages/db/src/query/live/collection-config-builder.ts`:
- Around line 1153-1158: The snapshot state is introducing an unnecessary any
type in SnapshotRow.value and the related helper signature, even though the
value is only stored and replayed. Tighten the types in
collection-config-builder by changing SnapshotRow and the affected helper used
around the snapshot logic to use unknown instead of any, and keep the existing
handling unchanged except for type-safe narrowing where needed.
In `@packages/db/tests/query/includes.test.ts`:
- Around line 5078-5079: The tests in `includes.test.ts` use fixed 50ms sleeps
after `priceRanges.insert(...)`, which should be replaced with the deterministic
`flushPromises()` helper already available in the test utilities. Update the
insert case and the matching delete case to await `flushPromises()` instead of
`setTimeout`, keeping the same behavior while removing timing flakiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ace73dfd-754c-45cb-8106-836a50331bd6
📒 Files selected for processing (3)
.changeset/nested-toarray-shared-buffer-overlap.mdpackages/db/src/query/live/collection-config-builder.tspackages/db/tests/query/includes.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/nested-toarray-shared-buffer-overlap.md
| row = { | ||
| value: changes.value, | ||
| orderByIndex: changes.orderByIndex, | ||
| count: 0, | ||
| } | ||
| snap.set(childKey, row) | ||
| } | ||
| row.count += changes.inserts - changes.deletes | ||
| if (changes.inserts > 0) { | ||
| row.value = changes.value | ||
| if (changes.orderByIndex !== undefined) { | ||
| row.orderByIndex = changes.orderByIndex | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clone snapshot values before storing or replaying them.
row.value currently aliases changes.value; later cleanup deletes INCLUDES_ROUTING from changes.value at Line 1984. A late replay can then seed a row without routing metadata, preventing deeper nested routing from being rebuilt via Lines 1544-1547.
🐛 Proposed fix
+function cloneSnapshotValue<T>(value: T): T {
+ if (value == null || typeof value !== `object`) return value
+ if (Array.isArray(value)) return [...value] as T
+ return { ...(value as Record<PropertyKey, unknown>) } as T
+}
+
function accumulateSnapshot(
setup: NestedIncludesSetup,
nestedCorrelationKey: unknown,
childChanges: Map<unknown, Changes<any>>,
@@
row = {
- value: changes.value,
+ value: cloneSnapshotValue(changes.value),
orderByIndex: changes.orderByIndex,
count: 0,
}
@@
if (changes.inserts > 0) {
- row.value = changes.value
+ row.value = cloneSnapshotValue(changes.value)
if (changes.orderByIndex !== undefined) {
row.orderByIndex = changes.orderByIndex
}
@@
byChild.set(childKey, {
deletes: 0,
inserts: row.count,
- value: row.value,
+ value: cloneSnapshotValue(row.value),
orderByIndex: row.orderByIndex,
})Also applies to: 1436-1443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/src/query/live/collection-config-builder.ts` around lines 1387 -
1399, Clone snapshot values before assigning them into row state in
collection-config-builder’s snapshot/replay logic, because row.value is
currently sharing the same object as changes.value and later mutation of
changes.value can strip routing metadata. Update the row initialization and
replay paths in the affected update/merge flow (including the shared logic
around the count/orderByIndex handling) to store a cloned copy instead of the
original object, so later cleanup cannot affect previously buffered rows and
nested routing can be rebuilt correctly.
|
Here's a complete explanation of the bugs and the fix. This PR implements the snapshot accumulation fix but it means we store each distinct nested row in that snapshot. The explanation below also documents a potential alternative. The exampleFor each product you want its price ranges, and for each price range its region. Three levels deep. Our problem data:
How includes actually compute (the important part)You might imagine the engine runs the "region" query fresh for each price range. It doesn't. That would be slow and wouldn't update reactively. Instead: There is ONE shared "region" pipeline. It runs once over all regions and emits region rows tagged with a correlation key — the value that says "who do I belong to". For region, that key is Just one entry. Even though two price ranges want region 1, the pipeline produces it once, keyed by Results land in a "buffer" — a temporary mailbox: Then a "drain" step delivers buffered rows to the right parents. This is the routing step. To deliver region 1 to the correct price range, the engine keeps a routing index: Each price-range group has its own private copy of region results (its own little child collection), so the final tree can show each one its own Why it brokeThe whole design quietly assumed one nested key belongs to one parent. Two things follow from that assumption, and both are wrong when two siblings share Bug 1 — the routing index only remembered one parent. It's a plain key→value map, so the second write clobbers the first. Now Bug 2 — late arrivals get nothing. Say T-Shirt's priceRange exists at load, but Hoodie's priceRange 3 is inserted later. By then the region pipeline already emitted region 1 (at load), it was drained, and the buffer entry was deleted. When priceRange 3 shows up, the pipeline does not re-emit region 1 — from its point of view nothing about regions changed; it already produced that row. So the buffer is empty and there's nothing to deliver to Hoodie. Empty array again. Both of these were confirmed with debug logging before writing the fix. The fix — two matching piecesFix 1: let one nested key map to many parentsThe routing index value changes from a single parent to a Set of parents:
Fix 2: remember delivered rows so late arrivals can be seededBecause the pipeline won't re-emit region 1 for a price range added later, the engine needs its own memory of "what region rows currently exist for regionId 1". That's the new snapshot:
One-line summaryThe nested-includes system assumed each nested key had exactly one parent and treated delivered rows as throwaway. The fix makes a nested key able to feed multiple parents (Set instead of single value + fan-out on drain), and keeps a snapshot of currently-existing rows so parents that show up late can be handed the same data their siblings already received. Is the snapshot a memory problem?Partly a fair concern, but it's bounded, not unbounded:
So it's not a leak and not multiplied by parents. But it is a real added cost: roughly O(distinct nested rows) extra — essentially one more index over the deepest level, for the lifetime of the live query. For a query with a very large nested collection, that's non-trivial memory. The alternative: copy from an existing sibling instead of keeping a snapshotThe key realizationWhen does a late-arriving parent need seeding at all? Only when the region rows were already delivered and the buffer entry deleted in an earlier flush, and the pipeline won't re-emit them. But "already delivered to whom?" — to a sibling. So whenever seeding is needed, at least one sibling already has the rows materialized in its own private copy. That's the insight the alternative exploits: you don't need a separate stash of the data, because a sibling is already holding a perfectly good copy of it. Go ask the sibling. How "copy from sibling" worksWalk through Hoodie's priceRange 3 arriving late, referencing
So instead of consulting a snapshot, step 2 reaches into a sibling's already-materialized collection. What it costs you in codeThe snapshot stored exactly what the delivery machinery needs: for each row, its
then iterate the sibling's collection and reconstruct insert-changes from each row. It's the same idea as You also need a small guard: "pick a sibling that is actually populated right now." The routing-index Set gives you candidates, but you'd skip the newcomer itself and any sibling that happens to be empty, and copy from the first good one. (As argued above, a good one is essentially guaranteed to exist whenever seeding is needed — if none did, the buffer path would still be handling it.) The two approaches compared
How to chooseThe snapshot trades memory for simplicity and predictability: one clean structure, consulted in one place, no "go find a healthy donor" logic. Copy-from-sibling trades that memory away but pays in code complexity and a reconstruction step on each late insert. For typical includes (modest nested result sets) the snapshot's overhead is negligible and the simpler code wins. If you expect very large nested collections where doubling an index matters, copy-from-sibling is the better fit — and it's a localized swap: only |
…lize includes The nested-includes routing fix is independent of how each level is materialized. Add regression tests proving sibling parent groups that share a deepest correlation key resolve their grandchildren when the nested levels are left as live Collections (no wrapper) and when wrapped with materialize(), mirroring the existing toArray coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/tests/query/includes.test.ts`:
- Around line 5171-5261: The current live/materialize regression tests only
verify preload behavior; extend the existing nested-collection coverage in the
relevant `it(...)` cases to assert post-load mutations as well. Use the same
`createLiveQueryCollection`, `toTree`, and nested `q.from(...)` setup to add
insert/delete assertions for the live and materialized variants, and include
async stale-order/race scenarios that exercise the same shared-correlation-key
path after initial load. Keep the new assertions anchored to the existing
collection setup so the bug is reproduced across both materialization modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e827313e-ce5c-41ab-a147-be0bec3dfe49
📒 Files selected for processing (1)
packages/db/tests/query/includes.test.ts
|
Had a quick discussion with @samwillis on the 2 alternatives. We decided to go for correctness and simplicity so we pick the snapshot approach. |
Add a post-load insert assertion to the shared-correlation-key materialize() regression test: inserting a sibling group that references an already-materialized correlation key must be seeded via the cumulative snapshot without disturbing the existing group's nested rows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
samwillis
left a comment
samwillis
left a comment
There was a problem hiding this comment.
I found one remaining routing hole in the new Set<parentCorrelationKey> approach.
A single parent correlation key can be backed by multiple child rows that share the same nested correlation key. The new route set dedupes those rows down to one parent entry, so when one of the child rows is deleted the delete path removes the whole parent route even though another child row in that same parent group still needs it. After that, later grandchild updates/deletes for the shared nested key no longer reach the surviving child.
The problematic path is around packages/db/src/query/live/collection-config-builder.ts:1591, where the delete path does parents.delete(correlationKey) without knowing whether another child row in that parent group still references the same nested key.
I pushed a minimal failing repro here:
- Branch: https://github.com/samwillis/db/tree/codex/pr-1607-same-parent-routing-repro
- Commit: samwillis@21ac924
The repro adds one product with two price ranges sharing regionId: 1, deletes one price range, then updates region 1. The surviving price range still shows Europe instead of Renamed Europe.
Targeted command used:
PATH=/Users/samwillis/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:$PATH pnpm exec vitest --run tests/query/includes.test.ts -t 'keeps routing when one of multiple same-parent siblings sharing a nested key is deleted'This likely needs per-parent route multiplicity/refcounting, or tracking routes at the child-key level, so deleting one sibling does not remove routing for the surviving sibling.
The Set<parentCorrelationKey> routing index introduced by the previous commit collapses multiple child rows in the same parent group that share a nested correlation key into a single route entry. Deleting one such sibling emptied the entry and dropped the whole route, so the surviving sibling stopped receiving grandchild changes (reported by @samwillis). Track the referencing child keys per (nestedKey, parentGroup) so the parent route is only dropped once its last referencing child row is gone. Also fix a routing hole this exposed: an update that changes a child row's nested correlation key (e.g. a price range's regionId) only carries the new key, so the row's stale reference under the old key was never released — a later sibling delete then mis-routed grandchild changes. A per-setup childKey -> nestedKey map records each row's current nested key so updates can release the old reference, scoped per nested setup so a change to one nested include never disturbs another on the same child. Tests cover: same-parent siblings sharing a key with one deleted, an update that changes the nested key followed by a sibling delete, and isolation between two nested includes on the same child row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Great catch @samwillis — confirmed and fixed in 33bfcaa. I reproduced your repro against the branch and it failed exactly as you described: deleting one of two same-parent siblings that share a nested correlation key dropped the route for the survivor, so later grandchild updates never reached it. Root cause was exactly what you identified at Fix: the routing index now tracks the referencing child keys per While verifying, I found a second routing hole your report exposed: an update that changes a child row's nested correlation key (e.g. a price range's New regression tests cover all three: your same-parent-sibling-delete case, the update-then-sibling-delete case, and cross-include isolation. Full Separately — and not introduced by this PR — I noticed that an atomic mutual swap (two same-parent siblings exchanging nested keys in a single tick) produces wrong results; it reproduces on |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/tests/query/includes.test.ts`:
- Around line 5179-5184: The test currently uses fixed `setTimeout(50)` sleeps,
which makes the async behavior timing-sensitive and flaky. In the affected
`includes.test.ts` cases, replace those waits with state-based synchronization
by polling or awaiting the expected collection state after each
`currencies.update(...)` mutation, using the existing test helpers or a
repo-local wait helper instead of arbitrary delays. Keep the assertions anchored
around the relevant `currencies` updates so the test verifies ordering and
completion directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e8a55a3-f66e-4f39-aed2-6091dd08e82f
📒 Files selected for processing (2)
packages/db/src/query/live/collection-config-builder.tspackages/db/tests/query/includes.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/query/live/collection-config-builder.ts
| await new Promise((r) => setTimeout(r, 50)) | ||
|
|
||
| currencies.update(9, (draft) => { | ||
| draft.code = `USD` | ||
| }) | ||
| await new Promise((r) => setTimeout(r, 50)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace fixed sleeps with state-based synchronization.
These setTimeout(50) waits make the regressions timing-sensitive: slow CI can still flake, and fast runs can hide ordering problems instead of asserting them. Please wait on the expected collection state (or a repo-local polling helper) after each mutation rather than sleeping for an arbitrary duration. As per coding guidelines, **/*.test.{ts,tsx,js}: “Test corner cases including … async race conditions …”.
Also applies to: 5269-5279, 5431-5436
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/tests/query/includes.test.ts` around lines 5179 - 5184, The test
currently uses fixed `setTimeout(50)` sleeps, which makes the async behavior
timing-sensitive and flaky. In the affected `includes.test.ts` cases, replace
those waits with state-based synchronization by polling or awaiting the expected
collection state after each `currencies.update(...)` mutation, using the
existing test helpers or a repo-local wait helper instead of arbitrary delays.
Keep the assertions anchored around the relevant `currencies` updates so the
test verifies ordering and completion directly.
Source: Coding guidelines
samwillis
left a comment
samwillis
left a comment
There was a problem hiding this comment.
Thanks for the update. The previous same-parent repro is fixed now: the added test keeps routing when one of multiple same-parent siblings sharing a nested key is deleted passes on ea83de909.
I found one remaining routing hole in the new child-key refcounting approach, though. nestedRoutingIndex is still shared across all sibling nested includes at a level (nestedCorrelationKey -> parent -> childKeys), while nestedRoutingChildToNested is per nested setup. When two sibling nested includes use the same nested correlation value for the same child row, cleanup for one include can delete the shared route that the other include still needs.
Concrete failing case:
- A price range has
regionId: 1andcurrencyId: 1. - The child select defines nested includes in this order:
currencyfirst,regionsecond. - Updating only
regionIdfrom1to2makes thecurrencysetup observe key1unchanged first, then theregionsetup removes child key1from the shared route for nested key1. - Because the route is shared and only keyed by child key, the later removal deletes the route needed by
currency. - A subsequent currency update no longer reaches the surviving
currencyinclude, so the row still showsEURinstead ofUSD.
The problematic shape is around packages/db/src/query/live/collection-config-builder.ts:1216 and the deletion at packages/db/src/query/live/collection-config-builder.ts:1556: the route refcount is not scoped by nested setup/include field, so equal correlation values from different nested includes collide.
Temporary repro I ran locally:
it(`keeps sibling include routing when an earlier include still references the old nested key`, async () => {
// one price range: regionId: 1, currencyId: 1
// select order: currency first, region second
priceRanges.update(1, (draft) => {
draft.regionId = 2
})
await new Promise((r) => setTimeout(r, 50))
currencies.update(1, (draft) => {
draft.code = `USD`
})
await new Promise((r) => setTimeout(r, 50))
// Fails: currency remains EUR
})Targeted command:
PATH=/Users/samwillis/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:$PATH pnpm exec vitest --run tests/query/includes.test.ts -t 'keeps sibling include routing when an earlier include still references the old nested key'This likely needs the route reference tracking to be scoped per nested setup/include field as well as by parent and child key, or otherwise include setup identity in the routing key/refcount so sibling includes with equal correlation values cannot remove each other's routes.
2 participants
Fixes #1501.
The bug
With three (or more) levels of nested
toArrayincludes (e.g.products → priceRanges → region), when two children in different parent groups share the same deepest correlation key, only one of them receives the nested rows — the other comes back as an empty array:Root cause
The nested pipeline writes results into a buffer shared across per-parent-group states, and routing assumes one parent per nested correlation key. Two compounding issues:
nestedRoutingIndexmapped each nested correlation key to a single parent group (last-writer-wins), anddrainNestedBuffersdeleted the buffer entry after routing to the first match — so a sibling group sharing the key was dropped.Fix
nestedRoutingIndexnow maps a nested correlation key to a Set of parent groups;drainNestedBuffersfans buffered grandchild changes out to every ready parent group before dropping the buffer entry. Routing-index inserts/deletes and parent-delete cleanup maintain the per-key parent sets.Tests
Three tests under
nested toArray includes (depth 3+)cover initial load, a sibling inserted after load, and deleting one of two siblings that share a correlation key. Fulltests/query/suite (1596 tests) passes.🤖 Generated with Claude Code
Summary by CodeRabbit
toArrayand live nested includes behavior when sibling parent groups share the same deepest correlation key, preventing dropped or mis-associated child results.regionunderpriceRangesunderproducts) for both live and materialized collection modes, including preload, post-load insert, updates, and multiple delete scenarios.