fix(query-core): stop orphaned streamedQuery chunks from corrupting the cache#11068
fix(query-core): stop orphaned streamedQuery chunks from corrupting the cache#11068a-y-ibrahim wants to merge 4 commits into
Conversation
…he cache When a streamedQuery-backed query is refetched while a previous stream is still flowing, the old fetch is cancelled but its streamFn keeps running unless it explicitly reads context.signal. Many streamFns can't do that (e.g. a third-party SDK with no AbortSignal support), so the orphaned stream kept calling setQueryData after the new fetch had already reset the cache, interleaving stale chunks into what should be fresh data. Track the current stream per Query independently of signal consumption, so an orphaned stream notices it has been superseded and stops writing regardless of whether its streamFn ever touches the signal.
Minor consistency cleanup, no behavior change: query is always Query | undefined here (Array.prototype.find never returns null), so !!query and query !== undefined are equivalent; use the same !!query idiom the file already uses a few lines above instead of mixing two styles.
|
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)
📝 WalkthroughWalkthrough
ChangesStreamed query supersession
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Prevents stale/orphaned streamedQuery() streams from continuing to write chunks into the cache after a refetch cancels and restarts the query, even when the streamFn does not consume context.signal.
Changes:
- Track the “current” active stream per
Queryinstance using aWeakMaptoken, and stop writing when a stream has been superseded. - Guard both per-chunk cache writes and
replace-mode finalization writes against superseded streams. - Add a regression test covering refetch cancellation when the stream function never consumes the abort signal, plus a changeset for a patch release.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/query-core/src/streamedQuery.ts | Adds per-Query active-stream tracking and prevents superseded streams from writing to cache. |
| packages/query-core/src/tests/streamedQuery.test.tsx | Adds a regression test ensuring orphaned streams can’t corrupt fresh refetch results. |
| .changeset/stop-orphaned-streamed-query-writes.md | Declares a patch release note for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-core/src/streamedQuery.ts (1)
133-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
replace-mode finalize guard with a regression test.The new orphaned-stream test only exercises
refetchMode: 'reset', which validates the loop-break guard (line 118) but not thisisReplaceRefetchfinalization path. Since this guard specifically blocks a stale invocation's accumulatedresultfrom overwriting the cache, an analogous test withrefetchMode: 'replace'(reusing the pending-promise pattern from the existing test) would lock in this behavior too.🤖 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-core/src/streamedQuery.ts` at line 133, Add a regression test for the orphaned-stream scenario using refetchMode: 'replace', reusing the existing pending-promise setup. Assert that a stale invocation does not finalize or overwrite the cache with its accumulated result, covering the isReplaceRefetch && !cancelled && !isSuperseded() guard in the streamed query flow.
🤖 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.
Nitpick comments:
In `@packages/query-core/src/streamedQuery.ts`:
- Line 133: Add a regression test for the orphaned-stream scenario using
refetchMode: 'replace', reusing the existing pending-promise setup. Assert that
a stale invocation does not finalize or overwrite the cache with its accumulated
result, covering the isReplaceRefetch && !cancelled && !isSuperseded() guard in
the streamed query flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bacb1de2-5ba9-492c-93e6-83dc1336a0ef
📒 Files selected for processing (3)
.changeset/stop-orphaned-streamed-query-writes.mdpackages/query-core/src/__tests__/streamedQuery.test.tsxpackages/query-core/src/streamedQuery.ts
…uery Address review feedback: the existing orphaned-stream test only exercised the loop-break guard (default refetchMode 'reset'), not the separate isReplaceRefetch finalize-write guard. Add a dedicated test with three overlapping refetches in refetchMode 'replace', confirming a superseded stream's accumulated result never overwrites the current correct one. Verified this test fails (writes a stale empty result over the correct one) if the isSuperseded() check is removed from the finalize condition, and passes with it in place.
|
Addressed the CodeRabbit nitpick in 9497dfa: added a dedicated test covering the refetchMode 'replace' finalize guard (a superseded stream's accumulated result must not overwrite the current one), verified it fails without the isSuperseded() check on that path and passes with it. |
🎯 Changes
When a
streamedQuery-backed query is refetched while a previous stream is still flowing, the old fetch is cancelled but itsstreamFnkeeps running unless it explicitly readscontext.signal. ManystreamFns can't do that (e.g. a third-party SDK with noAbortSignalsupport), so the orphaned stream kept callingsetQueryDataafter the new fetch had already reset the cache, interleaving stale chunks into what should be fresh data.This tracks the current stream per
Queryindependently of signal consumption, so an orphaned stream notices it has been superseded and stops writing, regardless of whether itsstreamFnever touches the signal.Reproduced and fixed with a new test (
should not let an orphaned stream write to the cache after a refetch when the signal is not consumed) that fails on currentmainand passes with this change.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit