fix(query-core): prevent duplicate abort event listeners in infinite queries #9959
Conversation
…queries
- Add listenerAttached flag to prevent multiple event listener registrations
- Add { once: true } option for automatic cleanup
- Add test to verify no duplicate listeners when signal is accessed multiple times
Fixes memory leak when signal property is accessed multiple times during infinite query pagination.
🦋 Changeset detectedLatest commit: 26cc893 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 |
WalkthroughPatches Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-22T09:06:05.219ZApplied to files:
🔇 Additional comments (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/calm-goats-punch.md(1 hunks)packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx(1 hunks)packages/query-core/src/infiniteQueryBehavior.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: TkDodo
Repo: TanStack/query PR: 9612
File: packages/query-async-storage-persister/src/asyncThrottle.ts:0-0
Timestamp: 2025-09-02T17:57:33.184Z
Learning: When importing from tanstack/query-core in other TanStack Query packages like query-async-storage-persister, a workspace dependency "tanstack/query-core": "workspace:*" needs to be added to the package.json.
📚 Learning: 2025-11-22T09:06:05.219Z
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
Applied to files:
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (2)
packages/query-core/src/queriesObserver.ts (1)
observer(270-276)packages/query-core/src/infiniteQueryObserver.ts (1)
InfiniteQueryObserver(26-185)
🔇 Additional comments (2)
.changeset/calm-goats-punch.md (1)
1-5: Changeset description matches the behavioral fixPatch bump and summary line correctly capture the memory-leak fix scope for
@tanstack/query-core. No issues from a release-notes perspective.packages/query-core/src/infiniteQueryBehavior.ts (1)
23-45: Abort-listener guard looks correct and fixes the leakScoping
listenerAttachedinsidefetchFnand guardingaddEventListener('abort', ...)with it ensures at most one abort listener per fetch, even when multiple pages are fetched or multiple contexts are created. Using{ once: true }also keeps the listener lifecycle tight after an abort. This aligns with the intended cancellation semantics while removing the memory-leak vector.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (1)
492-548: New abort-listener regression test is effective; minor robustness/clarity nitsThe overall shape of this test looks good: spying on
AbortSignal.prototype.addEventListenerbefore creating the observer will correctly see the real abort listener registrations, and asserting against the total count will fail if we regress back to “one listener persignalaccess”.A couple of small, non-blocking tweaks you might consider:
- The
previousCallCount/listenersForThisPagelogic is computed before thecontext.signalaccesses and never used in any assertion, so it doesn’t actually reflect “listeners added for this page” and just adds noise. You could simplify the test by dropping that bookkeeping and returning a constant page value instead.expect(totalAbortListeners).toBe(3)bakes in the current implementation detail “exactly one abort listener per page”. For the leak this PR is targeting, what really matters is that we don’t exceed one registration per page despite accessingsignalthree times. To keep the test resilient to future internal refactors (e.g. sharing a single signal across pages while still avoiding duplicates), you could loosen this totoBeLessThanOrEqual(3)while still catching the regression from 3 → 9 calls.For example:
- let previousCallCount = 0 - - const queryFnSpy = vi.fn().mockImplementation((context) => { - const currentCallCount = addEventListenerSpy.mock.calls.filter( - (call) => call[0] === 'abort' - ).length - - // Calculate how many listeners were added for this page - const listenersForThisPage = currentCallCount - previousCallCount - previousCallCount = currentCallCount - - // Access signal multiple times to trigger the getter repeatedly - // This simulates code that might reference the signal property multiple times - context.signal - context.signal - context.signal - - return `page-${listenersForThisPage}` - }) + const queryFnSpy = vi.fn().mockImplementation((context) => { + // Access signal multiple times to trigger the getter repeatedly + // This simulates code that might reference the signal property multiple times + context.signal + context.signal + context.signal + + return 'page' + }) @@ - expect(totalAbortListeners).toBe(3) + expect(totalAbortListeners).toBeLessThanOrEqual(3)These are just to reduce cognitive load and make the test a bit more future-proof; the existing test already exercises the leak scenario meaningfully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: TkDodo
Repo: TanStack/query PR: 9612
File: packages/query-async-storage-persister/src/asyncThrottle.ts:0-0
Timestamp: 2025-09-02T17:57:33.184Z
Learning: When importing from tanstack/query-core in other TanStack Query packages like query-async-storage-persister, a workspace dependency "tanstack/query-core": "workspace:*" needs to be added to the package.json.
📚 Learning: 2025-11-22T09:06:05.219Z
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
Applied to files:
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx
kamja44
force-pushed
the
fix/infinite-query-memory-leak
branch
from
f22f67b to
fd66a1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (1)
493-539: Abort-listener regression test correctly targets the leak; consider slightly tightening itThis now spies at the right level (
AbortSignal.prototype.addEventListener) and the “3 pages × 3 signal reads vs ≤3 abort listeners” check will actually catch the previous leak, which is great.If you want to harden it a bit more (optional):
- Use
expect(totalAbortListeners).toBe(3)instead of<= 3so a future change that stops attaching any listener at all doesn’t silently pass (cancellation behavior is covered elsewhere, but this would make the intent self-contained here).- Wrap the body in a
try/finallyto always runaddEventListenerSpy.mockRestore()andunsubscribe()even if an earlier assertion fails, avoiding potential cross-test contamination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: TkDodo
Repo: TanStack/query PR: 9612
File: packages/query-async-storage-persister/src/asyncThrottle.ts:0-0
Timestamp: 2025-09-02T17:57:33.184Z
Learning: When importing from tanstack/query-core in other TanStack Query packages like query-async-storage-persister, a workspace dependency "tanstack/query-core": "workspace:*" needs to be added to the package.json.
📚 Learning: 2025-11-22T09:06:05.219Z
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
Applied to files:
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx
kamja44
force-pushed
the
fix/infinite-query-memory-leak
branch
from
fd66a1f to
edb2bcc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx (1)
493-541: Regression test correctly exercises and guards the abort-listener leak fixSpying on
AbortSignal.prototype.addEventListenerand counting only'abort'registrations gives a robust signal that the getter no longer attaches multiple listeners per access; driving three pages and assertingtotalAbortListeners === 3will reliably fail if the old behavior (3× per access) is reintroduced. Thetry/finallywithmockRestore()andunsubscribe()also keeps the test well-isolated.If you want an extra bit of clarity (optional), you could assert the number of page fetches alongside the listener count, e.g. after the two
fetchNextPage()calls:await observer.fetchNextPage() await observer.fetchNextPage() + // Sanity check: we fetched 3 pages (initial + 2 next pages) + expect(queryFnSpy).toHaveBeenCalledTimes(3) + // Count total abort listeners registered const totalAbortListeners = addEventListenerSpy.mock.calls.filter( (call) => call[0] === 'abort' ).length
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: TkDodo
Repo: TanStack/query PR: 9612
File: packages/query-async-storage-persister/src/asyncThrottle.ts:0-0
Timestamp: 2025-09-02T17:57:33.184Z
Learning: When importing from tanstack/query-core in other TanStack Query packages like query-async-storage-persister, a workspace dependency "tanstack/query-core": "workspace:*" needs to be added to the package.json.
📚 Learning: 2025-11-22T09:06:05.219Z
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
Applied to files:
packages/query-core/src/__tests__/infiniteQueryBehavior.test.tsx
…teners
Based on code review feedback, the previous test approach had a flaw:
- Signal destructuring ({signal}) invokes the getter before addEventListener could be spied
- The test was not actually catching duplicate listener registrations
New approach:
- Spy on AbortSignal.prototype.addEventListener before query execution
- Access signal multiple times within queryFn to trigger getter repeatedly
- Verify that only 3 abort listeners are registered (1 per page) instead of 9 (3 per page)
This test now properly validates the memory leak fix and will fail if the
duplicate listener prevention is removed.
kamja44
force-pushed
the
fix/infinite-query-memory-leak
branch
from
edb2bcc to
26cc893
Compare
1 participant
🎯 Changes
Fixed a memory leak in infiniteQueryBehavior.ts where abort event listeners were being registered multiple times when the signal property was accessed repeatedly during pagination. Root Cause: The addSignalProperty getter was registering a new abort event listener on every signal access without checking if a listener was already attached. This pattern differed from query.ts, which correctly handles this scenario. Solution:
Files Changed:
Impact:
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.