fix(query-db): clean up empty ownership sets#1672
Conversation
📝 WalkthroughWalkthroughQuery ownership bookkeeping now distinguishes resolved empty results from absent state, removes empty row relationships, clears persisted ownership markers, exposes test-only ownership maps, and adds lifecycle coverage for unloading, expiry, revalidation, and retention cleanup. ChangesQuery ownership lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: |
|
Size Change: 0 B Total Size: 125 kB ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 4.22 kB ℹ️ View Unchanged
|
![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
🤖 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/query-db-collection/src/query.ts`:
- Line 1656: Update collection cleanup to build its key set from
state.observers, queryToRows, and resolvedOwnershipQueries so retained ownership
queries are removed before TTL expiry or revalidation; preserve deletion of
queryToRows, rowToQueries, and resolvedOwnershipQueries for every collected key.
Add a regression test covering collection.cleanup() while a retained placeholder
is pending, asserting all three structures are empty.
🪄 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: 225651f4-0c6e-4d6d-b6cc-a51d4b9b8966
📒 Files selected for processing (4)
.changeset/clean-query-ownership.md.superpowers/sdd/task-2-report.mdpackages/query-db-collection/src/query.tspackages/query-db-collection/tests/query.test.ts
|
|
||
| state.observers.delete(hashedQueryKey) | ||
| queryToRows.delete(hashedQueryKey) | ||
| resolvedOwnershipQueries.delete(hashedQueryKey) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include retained ownership keys in explicit collection cleanup.
Line 1656 is never reached for retained queries removed from state.observers at Line 1726. Since cleanup() only iterates observer keys, cleanup before TTL expiry/revalidation leaves queryToRows, rowToQueries, and resolvedOwnershipQueries retained.
Build the cleanup key set from state.observers, queryToRows, and resolvedOwnershipQueries. Add a regression test that calls collection.cleanup() while a retained placeholder is still pending and asserts all three structures are empty.
As per coding guidelines, “Always add unit tests that reproduce a bug before fixing it to ensure the bug is fixed and prevent regression.”
🤖 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/query-db-collection/src/query.ts` at line 1656, Update collection
cleanup to build its key set from state.observers, queryToRows, and
resolvedOwnershipQueries so retained ownership queries are removed before TTL
expiry or revalidation; preserve deletion of queryToRows, rowToQueries, and
resolvedOwnershipQueries for every collected key. Add a regression test covering
collection.cleanup() while a retained placeholder is pending, asserting all
three structures are empty.
Source: Coding guidelines
There was a problem hiding this comment.
The lifecycle fix (deleting stale entries at the right points) looks right to me. I'd like to suggest a simplification before this lands, though: I don't think we need resolvedOwnershipQueries as a separate structure.
The marker is only ever consulted as knownRows || resolvedOwnershipQueries.has(k),
i.e. a non-empty queryToRows entry already counts as authoritative on its own. The
new set only carries information when the row set is empty — which means we can encode "resolved" as entry presence in queryToRows itself, with an empty Set allowed:
- present entry (possibly empty) = ownership resolved
- absent entry = unresolved
Concretely:
- At the resolution point, replace
resolvedOwnershipQueries.add(k)withqueryToRows.set(k, queryToRows.get(k) ?? new Set()). - In
removeRowOwner, keep the drainedqueryToRowsentry instead of deleting it — a query whose rows are diffed away during revalidation is still resolved and now legitimately owns nothing. (The delete-when-empty stays correct forrowToQueries, since a row with no owners has no "resolved" meaning.) - The two teardown paths (
cleanupPersistedPlaceholderand query cleanup) then just delete the one entry instead of clearing two structures.
The read side reverts to the simpler pre-PR shape (if (knownRows)), and the compound (knownRows || resolvedOwnershipQueries.has(k)) && Array.from(knownRows ?? []) check goes away. More importantly, there's one source of truth: right now three separate mutation sites have to keep resolvedOwnershipQueries in sync with the map's lifecycle, and a missed site in a future change becomes a silent drift bug.
The trade-off is that the invariant becomes implicit — an accidental get-or-create on
queryToRows (like the existing addRowOwner pattern) would silently mark an unresolved query as resolved-empty. I think that's addressable by funneling writes through a couple of small helpers, documenting the presence-means-resolved invariant on the map declaration, and asserting it via the test-only inspection hook this PR already adds.
Memory-wise it's equivalent, so this is purely about keeping the bookkeeping in one place. WDYT?
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/query-db-collection/src/query.ts`:
- Around line 740-741: Update cleanup() to build its query-key set from both
state.observers.keys() and queryToRows keys, then use that combined set when
removing ownership data so retained queries are fully cleared from queryToRows
and rowToQueries during explicit collection cleanup.
🪄 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: 4a3e7916-3963-40c6-9263-cbd68dc9b5ae
📒 Files selected for processing (2)
packages/query-db-collection/src/query.tspackages/query-db-collection/tests/query.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/query-db-collection/tests/query.test.ts
| // queryKey → Set<RowKey>. Entry presence means ownership is resolved; | ||
| // an empty set represents a resolved query that currently owns no rows. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Include retained ownership keys in explicit collection cleanup.
As flagged in a previous review, queryToRows retains ownership keys for queries removed from state.observers during retention. The cleanup() method still only iterates state.observers.keys(), leaving queryToRows and rowToQueries populated for retained queries when an explicit collection cleanup occurs.
Build the cleanup key set by combining state.observers and queryToRows to ensure retained queries are properly cleaned up.
🤖 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/query-db-collection/src/query.ts` around lines 740 - 741, Update
cleanup() to build its query-key set from both state.observers.keys() and
queryToRows keys, then use that combined set when removing ownership data so
retained queries are fully cleared from queryToRows and rowToQueries during
explicit collection cleanup.
2 participants
Summary
Cleans up Query DB ownership bookkeeping so empty row/query ownership sets are not retained, while preserving the semantic distinction between an unresolved query and a resolved query with an empty result.
This keeps ownership state bounded across subset unloads, cache expiry, persisted-row hydration, and empty revalidation without deleting rows that are still shared by another query.
Design
Root cause
rowToQueriesandqueryToRowsused emptySets both as stale bookkeeping and as a marker that a query's ownership had been resolved. Removing those empty entries directly would lose the resolved-empty baseline and could cause hydration or revalidation to infer ownership again incorrectly.Approach
resolvedOwnershipQueries, including valid empty results.Invariants
Non-goals and tradeoffs
Verification
pnpm --filter @tanstack/query-db-collection test -- query.test.tsdb-collection-e2esources that cannot resolve@tanstack/electric-db-collectionin this worktree (3 unrelated source errors).Files
packages/query-db-collection/src/query.ts— compact ownership maps and preserve resolved-empty semantics with bounded lifecycle tracking.packages/query-db-collection/tests/query.test.ts— characterize and verify ownership cleanup, hydration, shared rows, cache expiry, and retained-query cleanup.Summary by CodeRabbit
Bug Fixes
Tests