test: failing repro for preload hanging after cleanup (#1576)#1605
test: failing repro for preload hanging after cleanup (#1576)#1605kevin-dp wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a single test case to ChangesRegression test for preload-after-cleanup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: |
kevin-dp
force-pushed
the
test/live-query-preload-after-cleanup
branch
from
a98f7c8 to
b8f487f
Compare
![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
🧹 Nitpick comments (1)
packages/db/tests/query/live-query-collection.test.ts (1)
597-619: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider asserting intermediate error state for clarity.
The test verifies the happy-path expectations (status returns to
ready, size is correct), but doesn't assert the intermediate error state that occurs during the bug scenario. According to the relevant code snippet, whenusersCollection.cleanup()is called while the live query depends on it, the live query should transition to an error state.Adding an assertion for the error state after line 611 would make the bug scenario more explicit and help document the expected state transitions.
📋 Optional assertion for error state
// Tear down the source collection and the live query, e.g. when switching // to a different data set at runtime. await usersCollection.cleanup() + // The live query should transition to error when its source is cleaned up + expect(activeUsers.status).toBe(`error`) + await activeUsers.cleanup() expect(activeUsers.status).toBe(`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/db/tests/query/live-query-collection.test.ts` around lines 597 - 619, The test verifies the happy path but omits an assertion for the intermediate error state that should occur when the source collection is cleaned up while the live query still depends on it. Add an assertion to check that activeUsers.status transitions to an error state immediately after the usersCollection.cleanup() call and before the activeUsers.cleanup() call. This will make the bug scenario and state transitions more explicit and help document the expected behavior when a live query's source collection is cleaned up.
🤖 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/db/tests/query/live-query-collection.test.ts`:
- Around line 616-618: The await activeUsers.preload() call lacks timeout
protection and could hang indefinitely if the isInErrorState flag bug exists.
Wrap the preload() call with a 2-second timeout mechanism (such as using
Promise.race with a timeout promise or Jest's timeout utilities) to ensure the
test fails fast rather than blocking CI indefinitely if the preload operation
does not complete within the specified time frame.
---
Nitpick comments:
In `@packages/db/tests/query/live-query-collection.test.ts`:
- Around line 597-619: The test verifies the happy path but omits an assertion
for the intermediate error state that should occur when the source collection is
cleaned up while the live query still depends on it. Add an assertion to check
that activeUsers.status transitions to an error state immediately after the
usersCollection.cleanup() call and before the activeUsers.cleanup() call. This
will make the bug scenario and state transitions more explicit and help document
the expected behavior when a live query's source collection is cleaned up.
🪄 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: 6f9a78e7-f350-42d7-89a9-f788abf94e15
📒 Files selected for processing (1)
packages/db/tests/query/live-query-collection.test.ts
| await activeUsers.preload() | ||
| expect(activeUsers.status).toBe(`ready`) | ||
| expect(activeUsers.size).toBe(2) |
There was a problem hiding this comment.
Add timeout protection to prevent indefinite test hang.
According to the PR objectives, this regression test captures a bug where preload() hangs indefinitely after cleanup due to the isInErrorState flag never being reset. Without timeout protection, if the bug exists, this await activeUsers.preload() call will block CI indefinitely.
The PR objectives mention including a 2-second timeout guard to fail fast. Consider adding a timeout mechanism here.
⏱️ Proposed timeout protection
// Preloading again restarts sync and resolves once the data is loaded.
- await activeUsers.preload()
+ await Promise.race([
+ activeUsers.preload(),
+ new Promise((_, reject) =>
+ setTimeout(() => reject(new Error('preload() timed out after cleanup')), 2000)
+ ),
+ ])
expect(activeUsers.status).toBe(`ready`)
expect(activeUsers.size).toBe(2)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await activeUsers.preload() | |
| expect(activeUsers.status).toBe(`ready`) | |
| expect(activeUsers.size).toBe(2) | |
| await Promise.race([ | |
| activeUsers.preload(), | |
| new Promise((_, reject) => | |
| setTimeout(() => reject(new Error('preload() timed out after cleanup')), 2000) | |
| ), | |
| ]) | |
| expect(activeUsers.status).toBe(`ready`) | |
| expect(activeUsers.size).toBe(2) |
🤖 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/db/tests/query/live-query-collection.test.ts` around lines 616 -
618, The await activeUsers.preload() call lacks timeout protection and could
hang indefinitely if the isInErrorState flag bug exists. Wrap the preload() call
with a 2-second timeout mechanism (such as using Promise.race with a timeout
promise or Jest's timeout utilities) to ensure the test fails fast rather than
blocking CI indefinitely if the preload operation does not complete within the
specified time frame.
Add a regression test asserting that a live query loads its data again when it is preloaded after the live query and its source collection were cleaned up (e.g. when switching data sets at runtime). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kevin-dp
force-pushed
the
test/live-query-preload-after-cleanup
branch
from
b8f487f to
3dd7098
Compare
|
Closing this as reproduced. It's fixed in #1606 |
1 participant
Reproduction for #1576
preloadaftercleanupon a collection hangs forever.This PR adds a failing unit test that reproduces the issue, so CI captures the buggy behaviour. The fix is in a follow-up PR stacked on top of this branch.
What it reproduces
When a source collection is cleaned up, any live query that depends on it transitions to an error state. Once everything is torn down and the live query is
preload()-ed again (the "switching profiles without a page refresh" use case from the issue), the preload promise never resolves.Root cause
In
packages/db/src/query/live/collection-config-builder.ts,transitionToError()latchesisInErrorState = true, and that flag is never reset. The config-builder instance is reused across sync sessions, so when sync restarts viapreload(),updateLiveQueryStatus()returns early andmarkReady()is never called — the preload hangs.Test
packages/db/tests/live-query-preload-after-cleanup.test.ts— fails fast (2s timeout guard) instead of hanging CI.🤖 Generated with Claude Code
Summary by CodeRabbit