-
Notifications
You must be signed in to change notification settings - Fork 158
Fix infinite loop in ORDER BY + LIMIT queries with Electric sync #1173
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
Open
KyleAMathews
wants to merge
10
commits into
main
Choose a base branch
from
claude/debug-electric-infinite-loop-QJD2Z
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add iteration safeguards to prevent infinite loops that can occur when using Electric with large datasets and ORDER BY/LIMIT queries: 1. `maybeRunGraph` while loop (collection-config-builder.ts): - Can loop infinitely when data loading triggers graph updates - Happens when WHERE filters out most data, causing dataNeeded() > 0 - Loading more data triggers updates that get filtered out - Added 10,000 iteration limit with error logging 2. `requestLimitedSnapshot` while loop (subscription.ts): - Can loop if index iteration has issues - Added 10,000 iteration limit with error logging - Removed unused `insertedKeys` tracking 3. `D2.run()` while loop (d2.ts): - Can loop infinitely on circular data flow bugs - Added 100,000 iteration limit with error logging The safeguards log errors to help debug the root cause while preventing the app from freezing.
🦋 Changeset detectedLatest commit: 3a5f05d The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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: |
Contributor
|
Size Change: +451 B (+0.5%) Total Size: 91.4 kB
ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 3.7 kB ℹ️ View Unchanged
|
The infinite loop occurred because `loadMoreIfNeeded` kept trying to load data even when the local index was exhausted. This happened when: 1. TopK had fewer items than limit (WHERE filtered out data) 2. loadMoreIfNeeded tried to load more → no local data found 3. Loop continued indefinitely since TopK still needed data Root cause fix: - Add `localIndexExhausted` flag to CollectionSubscriber - Track when local index has no more data for current cursor - Stop calling loadMoreIfNeeded when exhausted - Reset flag when new data arrives from sync layer (inserts) - requestLimitedSnapshot now returns boolean indicating if data was found Error handling improvements (per review feedback): - D2.run() now throws Error when iteration limit exceeded - Caller catches and calls transitionToError() for proper error state - requestLimitedSnapshot returns false when iteration limit hit - Live query properly shows error state if safeguard limits are hit This fixes the issue for both eager and progressive syncMode.
f47fbd0 to
2a796ff
Compare
These tests verify the localIndexExhausted fix works correctly: 1. Does not infinite loop when WHERE filters out most data - Query wants 10 items, only 2 match - Verifies status !== 'error' (fix works, not just safeguard) 2. Resumes loading when new matching data arrives - Starts with 0 matching items - Insert new matching items - localIndexExhausted resets, loads new data 3. Handles updates that move items out of WHERE clause - Updates change values to no longer match WHERE - TopK correctly refills from remaining matching data
453e7c7 to
e698eb1
Compare
Adds a new test that directly reproduces the infinite loop bug by creating a collection with a custom loadSubset that synchronously injects updates matching the WHERE clause. This simulates Electric's behavior of sending continuous updates during graph execution. The test verifies: - Without the localIndexExhausted fix, loadSubset is called 100+ times (infinite loop) - With the fix, loadSubset is called < 10 times (loop terminates correctly) Also adds additional tests for edge cases around the localIndexExhausted flag. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The removed test synchronously injected data in loadSubset, which artificially creates an infinite loop. Electric's loadSubset is async (uses await stream.fetchSnapshot), so it can't synchronously inject data during the maybeRunGraph loop. The remaining tests verify the localIndexExhausted flag's behavior correctly: - Prevents repeated load attempts when exhausted - Resets when new inserts arrive Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The localIndexExhausted flag was incorrectly resetting when updates arrived because splitUpdates converts updates to delete+insert pairs for D2, and the hasInserts check was seeing those fake inserts. Fix: Check for ORIGINAL inserts before calling splitUpdates, and pass that information to sendChangesToPipelineWithTracking. Now the flag only resets for genuine new inserts from the sync layer. Also adds a test that verifies requestLimitedSnapshot calls are limited after the index is exhausted - with the fix, only 0-4 calls happen after initial load even when 20 updates arrive. Without the fix, it would be ~19 calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Documents the reasoning for external reviewers: 1. splitUpdates fake inserts don't reset the flag 2. Updates to existing rows don't add new rows to scan 3. Edge case "update makes row match WHERE" is handled by filterAndFlipChanges converting unseen key updates to inserts Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
ELI5: Query asks for "top 10 where category='rare'" but only 3 rare items exist locally. System keeps asking "give me more!" but local index has nothing else → infinite loop. Fix: remember when the index is empty and stop asking. Reset when genuinely new data arrives.
Technical: Fixes an infinite loop that occurs when ORDER BY + LIMIT queries can't fill the TopK because WHERE filters out most data, while the sync layer continuously sends updates. Adds
localIndexExhaustedflag to prevent repeated load attempts, plus safety iteration limits as backstops.User impact: Prevents app freezes when using live queries with ORDER BY + LIMIT + WHERE combinations that match few items.
Root Cause
The infinite loop occurs in
maybeRunGraph's while loop:LIMIT 10) + WHERE that matches few items (e.g., 2 items)dataNeeded()returns 8)loadMoreIfNeededcallsloadNextItems, which exhausts the local indexsplitUpdatesconverts updates to delete+insert pairs for D2, and thehasInsertscheck was seeing those fake inserts, resettinglocalIndexExhaustedloadMoreIfNeededkeeps trying to load from the exhausted indexApproach
Primary fix (
localIndexExhaustedflag incollection-subscriber.ts):Secondary fix (check ORIGINAL inserts, not split inserts):
This ensures the flag only resets for genuine new inserts from the sync layer, not for updates converted to delete+insert by
splitUpdates.Safety backstops (iteration limits):
D2.run(): 100,000 iterationsmaybeRunGraph: 10,000 iterationsrequestLimitedSnapshot: 10,000 iterationsKey Invariants
localIndexExhaustedmust only reset on original inserts, not fake inserts fromsplitUpdatesrequestLimitedSnapshotmust returnbooleanindicating if local data was foundNon-goals
limititems)splitUpdatesworks (it's correct for D2 processing)loadSubsetcalls (just preventing infinite calls)Verification
Files Changed
packages/db/src/query/live/collection-subscriber.tslocalIndexExhaustedflag, check original inserts beforesplitUpdates, pass flag to reset logicpackages/db/src/collection/subscription.tsrequestLimitedSnapshotto return boolean, added iteration limitpackages/db/src/query/live/collection-config-builder.tsmaybeRunGraph, proper error handling withtransitionToErrorpackages/db-ivm/src/d2.tsD2.run()packages/db/tests/infinite-loop-prevention.test.tsrequestLimitedSnapshotcalls to verify the fix