fix(query-db-collection): cancel idle on-demand queries#1573
Open
superposition wants to merge 1 commit into
Open
fix(query-db-collection): cancel idle on-demand queries#1573superposition wants to merge 1 commit into
superposition wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR implements active request cancellation in query collections: when tracked queries become unused, the library now cancels in-flight TanStack Query requests via their ChangesQuery Collection Request Cancellation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-db-collection/src/query.ts (1)
1581-1590:⚠️ Potential issue | 🟠 MajorFix observer reuse after cancellation to avoid immediate AbortError rejection
cleanupQueryIfIdle()’sif (hasListeners)branch cancels viacancelQueryByHash()and returns without removingstate.observersfor that query key. On a fast remount,createQueryFromOpts()will reuse the existing observer and immediately reject whenobserver.getCurrentResult().isErroris set—so a canceled preload can surface as an immediate error instead of starting a fresh request.Suggested fix
const cancelQueryByHash = (hashedQueryKey: string) => { const key = hashToQueryKey.get(hashedQueryKey) if (!key) { return } - void queryClient.cancelQueries({ queryKey: key, exact: true }) + void queryClient.cancelQueries({ queryKey: key, exact: true, silent: true }) }Add regression:
preload()→cleanup()while in flight →preload()again on the same key beforegcTimeexpires should start a new request and not reject immediately withAbortError.🤖 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` around lines 1581 - 1590, cleanupQueryIfIdle() cancels the in-flight query via cancelQueryByHash(hashedQueryKey) but leaves the stale observer in state.observers causing createQueryFromOpts() to reuse an observer whose observer.getCurrentResult().isError immediately rejects; after cancelQueryByHash(hashedQueryKey) remove the corresponding observer entry from state.observers for hashedQueryKey (while still setting queryRefCounts.set(hashedQueryKey, 0) so invalidate/resubscribe paths still work) so a subsequent preload() will create a fresh observer instead of reusing the aborted one.
🤖 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/tests/query.test.ts`:
- Around line 5328-5371: The test creates a dedicated QueryClient instance
(customQueryClient) with a long gcTime but never clears it; wrap the test logic
that uses customQueryClient (the block that builds config, options, collection,
query, preloadPromise and awaits cleanup) in a try/finally and call
customQueryClient.clear() in the finally so the client's GC timers are
cancelled; reference the QueryClient constructor usage (customQueryClient), the
QueryCollectionConfig config, and the created collection/query so you clear the
exact client after the test finishes.
---
Outside diff comments:
In `@packages/query-db-collection/src/query.ts`:
- Around line 1581-1590: cleanupQueryIfIdle() cancels the in-flight query via
cancelQueryByHash(hashedQueryKey) but leaves the stale observer in
state.observers causing createQueryFromOpts() to reuse an observer whose
observer.getCurrentResult().isError immediately rejects; after
cancelQueryByHash(hashedQueryKey) remove the corresponding observer entry from
state.observers for hashedQueryKey (while still setting
queryRefCounts.set(hashedQueryKey, 0) so invalidate/resubscribe paths still
work) so a subsequent preload() will create a fresh observer instead of reusing
the aborted one.
🪄 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: 07c20a16-3142-463a-9a57-1d136f4a9abe
📒 Files selected for processing (4)
.changeset/query-collection-cancel-idle.mddocs/collections/query-collection.mdpackages/query-db-collection/src/query.tspackages/query-db-collection/tests/query.test.ts
Comment on lines
+5328
to
+5371
| const customQueryClient = new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { | ||
| gcTime: 5 * 60 * 1000, | ||
| retry: false, | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| const config: QueryCollectionConfig<TestItem> = { | ||
| id: `in-flight-abort-test`, | ||
| queryClient: customQueryClient, | ||
| queryKey: baseQueryKey, | ||
| queryFn, | ||
| getKey, | ||
| startSync: true, | ||
| syncMode: `on-demand`, | ||
| } | ||
|
|
||
| const options = queryCollectionOptions(config) | ||
| const collection = createCollection(options) | ||
|
|
||
| const query = createLiveQueryCollection({ | ||
| query: (q) => q.from({ item: collection }).select(({ item }) => item), | ||
| }) | ||
|
|
||
| const preloadPromise = query.preload().catch((error) => error) | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(queryFn).toHaveBeenCalledTimes(1) | ||
| expect(capturedSignal).toBeDefined() | ||
| }) | ||
|
|
||
| expect(capturedSignal!.aborted).toBe(false) | ||
|
|
||
| await query.cleanup() | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect(capturedSignal!.aborted).toBe(true) | ||
| }) | ||
|
|
||
| await preloadPromise | ||
| expect(collection.size).toBe(0) | ||
| }) |
Contributor
There was a problem hiding this comment.
Clear the dedicated QueryClient in a finally block.
This test creates a separate client with a 5-minute gcTime, but it never clears it. That leaves cache GC timers alive outside the shared afterEach() cleanup and can make this spec leak into later tests.
Suggested fix
- const customQueryClient = new QueryClient({
+ const customQueryClient = new QueryClient({
defaultOptions: {
queries: {
gcTime: 5 * 60 * 1000,
retry: false,
},
},
})
-
- const config: QueryCollectionConfig<TestItem> = {
- id: `in-flight-abort-test`,
- queryClient: customQueryClient,
- queryKey: baseQueryKey,
- queryFn,
- getKey,
- startSync: true,
- syncMode: `on-demand`,
- }
-
- const options = queryCollectionOptions(config)
- const collection = createCollection(options)
-
- const query = createLiveQueryCollection({
- query: (q) => q.from({ item: collection }).select(({ item }) => item),
- })
-
- const preloadPromise = query.preload().catch((error) => error)
-
- await vi.waitFor(() => {
- expect(queryFn).toHaveBeenCalledTimes(1)
- expect(capturedSignal).toBeDefined()
- })
-
- expect(capturedSignal!.aborted).toBe(false)
-
- await query.cleanup()
-
- await vi.waitFor(() => {
- expect(capturedSignal!.aborted).toBe(true)
- })
-
- await preloadPromise
- expect(collection.size).toBe(0)
+ try {
+ const config: QueryCollectionConfig<TestItem> = {
+ id: `in-flight-abort-test`,
+ queryClient: customQueryClient,
+ queryKey: baseQueryKey,
+ queryFn,
+ getKey,
+ startSync: true,
+ syncMode: `on-demand`,
+ }
+
+ const options = queryCollectionOptions(config)
+ const collection = createCollection(options)
+
+ const query = createLiveQueryCollection({
+ query: (q) => q.from({ item: collection }).select(({ item }) => item),
+ })
+
+ const preloadPromise = query.preload().catch((error) => error)
+
+ await vi.waitFor(() => {
+ expect(queryFn).toHaveBeenCalledTimes(1)
+ expect(capturedSignal).toBeDefined()
+ })
+
+ expect(capturedSignal!.aborted).toBe(false)
+
+ await query.cleanup()
+
+ await vi.waitFor(() => {
+ expect(capturedSignal!.aborted).toBe(true)
+ })
+
+ await preloadPromise
+ expect(collection.size).toBe(0)
+ } finally {
+ customQueryClient.clear()
+ }📝 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.
Suggested change
| const customQueryClient = new QueryClient({ | |
| defaultOptions: { | |
| queries: { | |
| gcTime: 5 * 60 * 1000, | |
| retry: false, | |
| }, | |
| }, | |
| }) | |
| const config: QueryCollectionConfig<TestItem> = { | |
| id: `in-flight-abort-test`, | |
| queryClient: customQueryClient, | |
| queryKey: baseQueryKey, | |
| queryFn, | |
| getKey, | |
| startSync: true, | |
| syncMode: `on-demand`, | |
| } | |
| const options = queryCollectionOptions(config) | |
| const collection = createCollection(options) | |
| const query = createLiveQueryCollection({ | |
| query: (q) => q.from({ item: collection }).select(({ item }) => item), | |
| }) | |
| const preloadPromise = query.preload().catch((error) => error) | |
| await vi.waitFor(() => { | |
| expect(queryFn).toHaveBeenCalledTimes(1) | |
| expect(capturedSignal).toBeDefined() | |
| }) | |
| expect(capturedSignal!.aborted).toBe(false) | |
| await query.cleanup() | |
| await vi.waitFor(() => { | |
| expect(capturedSignal!.aborted).toBe(true) | |
| }) | |
| await preloadPromise | |
| expect(collection.size).toBe(0) | |
| }) | |
| const customQueryClient = new QueryClient({ | |
| defaultOptions: { | |
| queries: { | |
| gcTime: 5 * 60 * 1000, | |
| retry: false, | |
| }, | |
| }, | |
| }) | |
| try { | |
| const config: QueryCollectionConfig<TestItem> = { | |
| id: `in-flight-abort-test`, | |
| queryClient: customQueryClient, | |
| queryKey: baseQueryKey, | |
| queryFn, | |
| getKey, | |
| startSync: true, | |
| syncMode: `on-demand`, | |
| } | |
| const options = queryCollectionOptions(config) | |
| const collection = createCollection(options) | |
| const query = createLiveQueryCollection({ | |
| query: (q) => q.from({ item: collection }).select(({ item }) => item), | |
| }) | |
| const preloadPromise = query.preload().catch((error) => error) | |
| await vi.waitFor(() => { | |
| expect(queryFn).toHaveBeenCalledTimes(1) | |
| expect(capturedSignal).toBeDefined() | |
| }) | |
| expect(capturedSignal!.aborted).toBe(false) | |
| await query.cleanup() | |
| await vi.waitFor(() => { | |
| expect(capturedSignal!.aborted).toBe(true) | |
| }) | |
| await preloadPromise | |
| expect(collection.size).toBe(0) | |
| } finally { | |
| customQueryClient.clear() | |
| } |
🤖 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 5328 - 5371,
The test creates a dedicated QueryClient instance (customQueryClient) with a
long gcTime but never clears it; wrap the test logic that uses customQueryClient
(the block that builds config, options, collection, query, preloadPromise and
awaits cleanup) in a try/finally and call customQueryClient.clear() in the
finally so the client's GC timers are cancelled; reference the QueryClient
constructor usage (customQueryClient), the QueryCollectionConfig config, and the
created collection/query so you clear the exact client after the test finishes.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes #350
Summary
QueryFunctionContext.signalaborts when the last on-demand live query subscriber cleans upctx.signalto abortable request clientsTest plan
corepack pnpm --filter @tanstack/electric-db-collection... buildcorepack pnpm --filter @tanstack/query-db-collection buildcorepack pnpm --filter @tanstack/query-db-collection test -- tests/query.test.tscorepack pnpm test:sherifcorepack pnpm exec prettier --check packages/query-db-collection/src/query.ts packages/query-db-collection/tests/query.test.ts docs/collections/query-collection.md .changeset/query-collection-cancel-idle.md && git diff --checkcorepack pnpm exec eslint packages/query-db-collection/src/query.ts packages/query-db-collection/tests/query.test.ts(no errors; existing warnings only)Known existing failure:
corepack pnpm test:docsfails becausedocs/reference/index.mdlinks to missingdocs/reference/functions/caseWhen.md, unrelated to this change.Summary by CodeRabbit
Documentation
queryFnreceives TanStack Query's signal for proper cleanup in on-demand collections.Improvements