fix(db): prevent on-demand live query from being marked ready before data loads #1081
Conversation
🦋 Changeset detectedLatest commit: 5c3ab10 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: +88 B (+0.1%) Total Size: 90.2 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 3.47 kB ℹ️ View Unchanged
|
|
Doesn't seem to resolve the issue unfortunately. Also, I'm not sure this is specific to suspense? For example if I have the below with regular useLiveQuery, sometimes const { data, isReady, state, } = useLiveQuery(
q =>
q
.from({ w: workflowRunsCollection })
.where(({ w }) => eq(w.id, 'some_id'))
.findOne(),
[],
);
console.log('run', { haveData: !!data, state, isReady });I've stripped our app down to this one query, and reload the page, and see similar symptom to the suspense version (albeit using isReady as the boundary versus suspense): 
|
|
Out of curiosity, I tried the latest from here and unfortunately the problem persists. So I had Claude add a bunch of debug logs to spots that might be relevant, then reloaded our app, and fed the logs back into claude for diagnosis. Here are the resulting findings from Claude in case helpful (including the debug logs from the actual reproduction): Debug Analysis:
|
|
That changes since my last comment fixed it! There are several adjustments in this PR, unsure which one(s) are the actual fixes, but something in the last couple of commits finally resolved it. Might want to figure out a test that fails before the last couple of commits, and succeeds with the changes from the last couple of commits. |
…in on-demand mode When using useLiveSuspenseQuery with on-demand sync mode, the suspense boundary would sometimes release before the query's data was actually loaded. This happened because the live query collection was marked as ready immediately when the source collection was already ready, even though the loadSubset operation for the specific query hadn't completed. This fix ensures that useLiveSuspenseQuery also suspends while isLoadingSubset is true, waiting for the initial subset load to complete before releasing the suspense boundary.
This test verifies that useLiveSuspenseQuery holds the suspense boundary when isLoadingSubset is true, even if the collection status is 'ready'. The test confirms: 1. WITHOUT the fix: suspense releases prematurely (test fails) 2. WITH the fix: suspense waits for isLoadingSubset to be false (test passes)
…a is loaded In on-demand sync mode, the live query collection was being marked as 'ready' before the subset data finished loading. This caused useLiveQuery to return isReady=true with empty data, and useLiveSuspenseQuery to release suspense prematurely. The fix: 1. Added isLoadingSubset check in updateLiveQueryStatus() to prevent marking ready while subset is loading 2. Added listener for loadingSubset:change events to trigger ready check when subset loading completes 3. Added test case that verifies the correct timing behavior
…race condition The loadingSubset:change listener was registered after subscribeToAllCollections(), which could cause a race condition where the event fires before the listener is registered. This resulted in the live query never becoming ready. Also adds await in electric test to account for async subset loading.
Register the status:change listener BEFORE checking the current subscription status to avoid missing status transitions. Previously, if loadSubset completed very quickly, the status could change from 'loadingSubset' to 'ready' between checking the status and registering the listener, causing the tracked promise to never resolve and the live query to never become ready.
…ery's The previous fix incorrectly checked isLoadingSubset on the live query collection itself, but the loadSubset/trackLoadPromise mechanism runs on SOURCE collections during on-demand sync, so the live query's isLoadingSubset was always false. This fix: - Adds anySourceCollectionLoadingSubset() to check if any source collection has isLoadingSubset=true - Listens for loadingSubset:change events on source collections instead of the live query collection
…rce collections Reverts the change to check source collections' isLoadingSubset, which was causing test timeouts in query-db-collection tests. The live query collection's isLoadingSubset is correctly updated by CollectionSubscriber.trackLoadPromise() which tracks loading on the live query collection itself. Also updates changeset to accurately describe the fix.
…r snapshot trigger The subscription's status:change listener was being registered AFTER the snapshot was triggered (via requestSnapshot/requestLimitedSnapshot). This meant that if the loadSubset promise resolved quickly (or synchronously), the status transition from 'loadingSubset' to 'ready' could be missed entirely. Changes: - Refactored subscribeToChanges() to split subscription creation from snapshot triggering - subscribeToMatchingChanges() and subscribeToOrderedChanges() now return both the subscription AND a triggerSnapshot function - The status listener is registered AFTER getting the subscription but BEFORE calling triggerSnapshot() - Added deferSnapshot option to subscribeChanges() to prevent automatic snapshot request - For non-ordered queries, continue using trackLoadSubsetPromise: false to maintain compatibility with query-db-collection's destroyed observer handling - Updated test for source collection isLoadingSubset independence - Added regression test for the race condition fix
KyleAMathews
force-pushed
the
claude/fix-on-demand-suspense-HKGki
branch
from
12e4d74 to
2ae80a2
Compare
KyleAMathews
changed the title
KyleAMathews
changed the title
- Remove useLiveSuspenseQuery changes (not needed with core fix) - Remove artificial test that manually sets isLoadingSubset after ready - Update changeset to accurately describe the fix - Remove react-db changeset (no react-db source changes) The core fix (deferSnapshot + ready gating) is sufficient. The suspense hook doesn't need additional isLoadingSubset checks because the live query collection won't be marked ready while isLoadingSubset is true. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tests the tricky case where loadSubset returns Promise.resolve() immediately. This proves the race condition fix works even when the status transition happens synchronously, not just with delayed promises. Addresses reviewer feedback to strengthen test coverage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@KyleAMathews I find the new flow a bit cumbersome and error-prone since the caller has to remember to defer the snapshot, register the listener, then trigger the snapshot. I'd prefer if we replace the I asked Cursor what it thinks about this and it agreed and came up with this more detailed plan: Analyzing the PR and evaluating the alternative. Searching for Proposed Solution AnalysisPassing the status listener as an option to
Recommended ImplementationHere's how the API could look: export interface SubscribeChangesOptions {
/** Whether to include the current state as initial changes */
includeInitialState?: boolean
/** Pre-compiled expression for filtering changes */
whereExpression?: BasicExpression<boolean>
/** Listener for subscription status changes - registered before snapshot is requested */
onStatusChange?: (event: SubscriptionStatusChangeEvent) => void
}Then in public subscribeChanges(
callback: (changes: Array<ChangeMessage<TOutput>>) => void,
options: SubscribeChangesOptions = {}
): CollectionSubscription {
this.addSubscriber()
const subscription = new CollectionSubscription(this.collection, callback, {
...options,
onUnsubscribe: () => {
this.removeSubscriber()
this.changeSubscriptions.delete(subscription)
},
})
// Register status listener BEFORE requesting snapshot to avoid race condition
if (options.onStatusChange) {
subscription.on(`status:change`, options.onStatusChange)
}
if (options.includeInitialState) {
subscription.requestSnapshot({ trackLoadSubsetPromise: false })
}
this.changeSubscriptions.add(subscription)
return subscription
}This ensures:
This approach is better than introducing |
|
@kevin-dp implemented |
Simplify the subscription API by replacing the error-prone 3-step deferSnapshot pattern with a cleaner onStatusChange callback option. The listener is now registered internally BEFORE any snapshot is requested, guaranteeing no status transitions are missed regardless of how quickly loadSubset resolves. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
KyleAMathews
left a comment
KyleAMathews
left a comment
There was a problem hiding this comment.
Code Review: PR #1081
Overall Assessment: Approve ✅
This is a solid bug fix that addresses a subtle but impactful race condition. Like the importance of covenant timing in LDS doctrine, the order of operations here matters greatly - and this PR gets it right.
Summary
The PR fixes a race condition where live queries in on-demand sync mode would return isReady=true with empty data. The root cause was that the status:change listener was registered after the snapshot was triggered, potentially missing the loadingSubset → ready transition.
The Fix
Two-pronged approach:
-
onStatusChangecallback option - Passed tosubscribeChanges()and registered BEFORE any snapshot is requested:// changes.ts if (options.onStatusChange) { subscription.on(`status:change`, options.onStatusChange) } // THEN trigger snapshot if (options.includeInitialState) { subscription.requestSnapshot({ trackLoadSubsetPromise: false }) }
-
Ready state gating - Extra check in
updateLiveQueryStatus():if (this.allCollectionsReady() && !this.liveQueryCollection?.isLoadingSubset) { markReady() }
Strengths
-
Correct ordering: The key insight is ensuring listener registration happens before any async work that could trigger status changes.
-
Defense in depth: Both the
onStatusChangepattern and theisLoadingSubsetcheck provide redundant safety. -
Excellent test coverage: Tests specifically target:
- Delayed
loadSubsetresolution - Synchronously resolving
loadSubset- the trickiest case - Race condition verification
- Delayed
-
loadingSubset:changelistener: The listener on the live query collection (collection-config-builder.ts:573-581) ensures we catch the transition even if it happens afterallCollectionsReady()returns true.
Minor Notes
-
subscriptionHolder pattern (collection-subscriber.ts:229-232): Using a holder object to forward-reference the subscription in callbacks is a good pattern for this circular reference scenario.
-
Test in electric-live-query.test.ts: The added
await setTimeout(0)is a good fix - it accounts for the now-async nature of subset loading completion. -
Documentation: The changeset clearly explains the root cause and fix.
Question
The onStatusChange option is marked @internal in the types. Is there value in documenting a recommended pattern for external consumers who might build similar lazy-loading features? Or is this purely an internal concern?
Great bug fix with thorough testing! 🏁
|
🎉 This PR has been released! Thank you for your contribution! |
Summary
Live queries in on-demand sync mode would return
isReady=truewith empty data, or release suspense prematurely. Users see a flash of empty state before data appears.Root Cause
Race condition in
CollectionSubscriber: thestatus:changelistener was registered after the snapshot was triggered. IfloadSubsetresolved quickly, theloadingSubset→readytransition was missed, sotrackLoadPromisewas never called on the live query collection.Approach
1. Deferred snapshot triggering (core fix)
2. Ready state gating (defense in depth)
Key invariant
The status listener must be registered before any async work starts. This is now enforced by the
deferSnapshotoption.Non-goals
useLiveSuspenseQueryhook. The fix at the collection level is sufficient—suspense naturally holds becausestatus !== 'ready'while loading.Trade-offs
deferSnapshotto public API surface, but it's an internal option not meant for external useVerification
New test:
should not mark live query ready while isLoadingSubset is trueFiles changed
packages/db/src/collection/changes.ts- AdddeferSnapshotoptionpackages/db/src/query/live/collection-config-builder.ts- Ready gating +loadingSubset:changelistenerpackages/db/src/query/live/collection-subscriber.ts- Defer snapshot triggeringpackages/db/src/types.ts- AdddeferSnapshottype