-
Notifications
You must be signed in to change notification settings - Fork 161
fix(db): track loadSubset promise for on-demand live queries #1192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When a live query without orderBy/limit subscribes to an on-demand collection, the subscription was passing includeInitialState: true to subscribeChanges, which internally called requestSnapshot with trackLoadSubsetPromise: false. This prevented the subscription status from transitioning to 'loadingSubset', causing the live query to be marked ready before data actually loaded. The fix changes subscribeToMatchingChanges to manually call requestSnapshot() after creating the subscription (with default tracking enabled), ensuring the loadSubset promise is properly tracked and the live query waits for data before becoming ready. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This follow-up fix addresses edge cases that caused tests to fail after the initial isReady fix: 1. lifecycle.ts: Call pending onFirstReady callbacks during cleanup - Prevents preload() promises from hanging when cleanup happens - Ensures clean termination of pending preload operations 2. query.ts: Add fallback cache checks in createQueryFromOpts - Check QueryClient cache when observer state is out of sync - Handle cases where observer was deleted but data is still cached - Prevents hangs during cleanup/recreate cycles 3. Test updates: - Updated where clause tests to use synchronous loadSubset - These tests verify where clause passing, not async loading behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 133a697 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 |
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: +671 B (+0.73%) Total Size: 92 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 3.7 kB ℹ️ View Unchanged
|
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
fca9084 to
46d4ec5
Compare
- Fix potential memory leak in trackCollectionLoading by registering cleanup callback for early unsubscribe scenarios - Add error handling for onFirstReady callbacks during cleanup to ensure all callbacks are attempted even if one throws Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Previously, when multiple live queries subscribed to the same source collection, only the first would correctly track loading state due to the `!wasLoadingBefore` guard. This caused subsequent live queries to incorrectly report `isReady` before data finished loading. The fix removes this guard since each live query needs its own loading state tracking regardless of whether another query already triggered loading. Also adds a regression test for this scenario. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Verified this locally. Working great! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code quality looks good to me. Just left a small comment.
There are quite some changes to subtle parts of the code, not sure i understand all of them. The changes may have some important implications and complicate this already complicated code a bit more by introducing yet some more state variables loadRequestedForCurrentCursor, lastLoadRequestKey, seenKeys. We know that previous bugs have been caused by all these state variables and some code paths not handling/resetting them properly. So we need to be careful about that.
| // The onStatusChange listener will catch the transition to 'ready'. | ||
| if (subscription.status === `loadingSubset`) { | ||
| trackLoadPromise(subscription) | ||
| if (!this.subscriptionLoadingPromises.has(subscription)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is duplicated on line 93. It used to call trackLoadPromise. We should extract this duplicate code into a function (if trackLoadPromise isn't the right one to use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call — working on adding a helper to make the state changes cleaner/less bug prone
- Remove redundant has() check before Set.add() (idempotent operation) - Replace nested ternary with clearer if-statement for minValues Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
🎉 This PR has been released! Thank you for your contribution! |
Summary
Fixes
isReadytracking for on-demand live queries. This PR addresses two related issues:setWindowAlso fixes
preload()promises hanging when cleanup occurs before the collection becomes ready.Root Cause
Issue 1: Non-ordered queries (original fix)
For non-ordered live queries,
subscribeChangesis called withincludeInitialState: true, which internally callsrequestSnapshot({ trackLoadSubsetPromise: false }). This disables the subscription's status tracking—the subscription never transitions toloadingSubsetstatus, so the live query'sisReadystayed true during loading.Issue 2: Ordered paging hang (this session)
requestLimitedSnapshotusedlimitedSnapshotRowCountto compute offset for offset-based backends. That counter only advanced when the local index produced rows. WhenloadSubsetresolved asynchronously (cached query observer) without local rows at request time, the counter stayed at 0. SubsequentloadNextItemscalls kept requesting offset 0, re-writing the same rows and schedulingloadMoreagain, leading to an infinite loop.History: Introduced in commit b3b1940 (2025-12-12) when limitedSnapshotRowCount/offset pagination was added; the counter never advanced for async loadSubset results. The hang has been possible since then in on-demand ordered/limited queries (especially with multiple live queries or window moves).
Approach
For non-ordered queries
The fix couldn't simply change
trackLoadSubsetPromisetotruebecause that breaks truncate handling (must-refetch scenarios). The truncate buffering logic depends on the existing status behavior.Instead, the fix tracks loading at the collection level (
collection-subscriber.ts): After subscribing, checks if the source collection'sisLoadingSubsetchanged. If so, listens for theloadingSubset:changeevent and tracks a promise on the live query collection forisReady.For ordered paging
Keep offset in sync (
subscription.ts): SynclimitedSnapshotRowCountwithsentKeys.sizeafter callbacks—advances the offset even when async loadSubset results arrive without local index rowsDeduplicate by request parameters (
collection-subscriber.ts): TracklastLoadRequestKey(serialized cursor + window) to prevent repeated identical requests while still allowing window moves (different offset/limit)Track seen keys (
collection-subscriber.ts): UseseenKeysto detect genuinely new rows (as opposed to repeated updates for the same keys) and reset the dedup flag when new keys arrivePass orderBy/limit to snapshot requests: Ensures each live query with different parameters gets its own cache entry in on-demand source collections
Cleanup handling
onFirstReadycallbacks during cleanup (lifecycle.ts) sopreload()promises resolve instead of hangingKey Invariants
isReadymust returnfalsewhileloadSubsetis in progresspreload()must always resolve (either when ready OR during cleanup)setWindow) must trigger new data fetches even if cursor hasn't advancedRegression Tests Added
live-query-collection: advances offset after async loadSubset when initial window is emptylive-query-collection: window moves across identical orderBy values request new offsetse2e pagination: multiple live queries with distinct windows + paging a second queryVerification
All tests pass:
Files Changed
packages/db/src/collection/subscription.ts- Sync offset counter with sentKeys, add onLoadSubsetResult/trackLoadSubsetPromise optionspackages/db/src/query/live/collection-subscriber.ts- Deduplicate by request parameters, track seen keys, direct load promise trackingpackages/db/src/query/live/collection-config-builder.ts- Pass orderBy/limit to snapshot requestspackages/db/src/collection/lifecycle.ts- Call onFirstReady callbacks during cleanup