fix(query-db): clean up empty ownership sets#1672
Conversation
📝 WalkthroughWalkthroughQuery ownership bookkeeping now preserves authoritative empty results, removes empty ownership relationships, clears persisted markers, and validates unloading, expiry, revalidation, retention, and collection cleanup. ChangesQuery ownership lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-db-collection/tests/query.test.ts (1)
5974-5977: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpread
originalSyncto preserve allSyncPluginproperties.When intercepting the
syncmethod of theoriginalSyncplugin, it's safer to spread the rest of theoriginalSyncproperties into the new object. Although core lifecycle methods likecleanupandloadSubsetmay be returned dynamically in theSyncResult, spreading the plugin object ensures any top-level properties (likeidor future plugin hooks) are not inadvertently lost.♻️ Proposed refactor
sync: { + ...originalSync, sync: (params: Parameters<typeof originalSync.sync>[0]) => originalSync.sync({ ...params, metadata: metadataHarness.api }), },🤖 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/tests/query.test.ts` around lines 5974 - 5977, Update the intercepted originalSync plugin object to spread all properties from originalSync before overriding its sync method, preserving top-level SyncPlugin fields such as id and future hooks while retaining the metadata injection behavior.
🤖 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.
Nitpick comments:
In `@packages/query-db-collection/tests/query.test.ts`:
- Around line 5974-5977: Update the intercepted originalSync plugin object to
spread all properties from originalSync before overriding its sync method,
preserving top-level SyncPlugin fields such as id and future hooks while
retaining the metadata injection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b61115a-4091-4b21-9a11-d4033311eb60
📒 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/src/query.ts
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