fix(core): make sure queries revert synchronously #9601
Conversation
the switch from the callback approach to async/await with catch was necessary to be able to transform errors into revert-state data for imperative query calls; however, this was a change in behavior because catch in async/await doesn't happen immediately - it runs in the next microtask. That opened up an opportunity for a slight race condition if we re-start a fetch in-between. And who does that? Of course, React Strict Mode. This PR moves the actual state reverting back to a callback (so it happens synchronously), while still keeping the try/catch refactoring merely to transform the promise and the usual error handling
WalkthroughIntroduces a new cancellation flow by replacing Retryer abort with onCancel, adjusts revert-on-cancel handling in query logic, and adds a test validating cancellation when resubscribing during an in-flight fetch. No other production files are modified. Changes
Sequence Diagram(s)sequenceDiagram
participant Obs1 as Observer #1
participant Query as Query
participant Retryer as Retryer
participant AC as AbortController
participant QFn as queryFn
Obs1->>Query: subscribe()
activate Query
Query->>AC: new AbortController()
Query->>Retryer: createRetryer({ onCancel })
Retryer->>QFn: invoke(queryFn, { signal: AC.signal })
activate QFn
Obs1-->>Query: unsubscribe() (during in-flight)
Retryer->>Retryer: cancel()
Retryer-->>Query: onCancel(CancelledError)
Note right of Query: If error.revert === true<br/>revert transient state
Query->>AC: abort()
deactivate QFn
Retryer-->>Obs1: reject(CancelledError)
participant Obs2 as Observer #2
Obs2->>Query: subscribe()
Query->>AC: new AbortController()
Query->>Retryer: createRetryer({ onCancel })
Retryer->>QFn: invoke(queryFn, { signal: AC.signal })
QFn-->>Retryer: resolve(data1)
Retryer-->>Query: success(data1)
Query-->>Obs2: notify(status: success, fetchStatus: idle, data1)
deactivate Query
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit 23d4346
☁️ Nx Cloud last updated this comment at |
|
Sizes for commit 23d4346:
|
![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 (2)
packages/query-core/src/retryer.ts (1)
13-13: Type-safety and ordering: make onCancel explicitly handle CancelledError and run before reject.
- onCancel always receives a CancelledError; tightening the type clarifies intent and avoids downstream casts.
- Running onCancel before reject ensures synchronous side-effects (like revert) occur prior to any queued catch handlers, making the fix more robust across environments.
Apply this diff:
@@ interface RetryerConfig<TData = unknown, TError = DefaultError> { - onCancel?: (error: TError) => void + onCancel?: (error: CancelledError & TError) => void @@ - const cancel = (cancelOptions?: CancelOptions): void => { - if (!isResolved()) { - const error = new CancelledError(cancelOptions) as TError - reject(error) - - config.onCancel?.(error) - } - } + const cancel = (cancelOptions?: CancelOptions): void => { + if (!isResolved()) { + const error = new CancelledError(cancelOptions) as CancelledError & TError + // Run synchronous cancel side-effects (e.g., revert) before any queued catch handlers. + config.onCancel?.(error) + reject(error) + } + }Also applies to: 87-93
packages/query-core/src/query.ts (1)
510-518: Defensive revert: guard against undefined revertState and narrow error type.Today #revertState is set before creating the retryer, but a small guard makes this future-proof and avoids spreading undefined by accident.
Apply this diff:
- onCancel: (error) => { - if (error instanceof CancelledError && error.revert) { - this.setState({ - ...this.#revertState, - fetchStatus: 'idle' as const, - }) - } - abortController.abort() - }, + onCancel: (error) => { + if (error instanceof CancelledError) { + if (error.revert && this.#revertState) { + this.setState({ ...this.#revertState, fetchStatus: 'idle' as const }) + } + abortController.abort() + } + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/query-core/src/__tests__/query.test.tsx(1 hunks)packages/query-core/src/query.ts(2 hunks)packages/query-core/src/retryer.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/query-core/src/__tests__/query.test.tsx (3)
packages/query-core/src/utils.ts (2)
sleep(355-359)hashQueryKeyByOptions(203-209)packages/query-core/src/query.ts (1)
Query(159-678)packages/query-core/src/retryer.ts (1)
CancelledError(58-66)
packages/query-core/src/query.ts (1)
packages/query-core/src/retryer.ts (1)
CancelledError(58-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/query-core/src/retryer.ts (1)
10-21: LGTM: onCancel hook restores synchronous revert without touching retry semantics.Replacing abort with onCancel cleanly separates transport aborting from cancellation-side effects, aligning with the PR goal of synchronous revert. No behavioral regressions spotted in retry loop or pause/continue paths.
packages/query-core/src/query.ts (1)
561-563: Comment clarity is good; matches the new behavior.The “piggyback onto that promise” note reads well and aligns with the silent cancel path. No change needed.
| test('should not override fetching state when revert happens after new observer subscribes', async () => { | ||
| const key = queryKey() | ||
| let count = 0 | ||
|
|
||
| const queryFn = vi.fn(async ({ signal: _signal }) => { | ||
| // Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path | ||
| await sleep(50) | ||
| return 'data' + count++ | ||
| }) | ||
|
|
||
| const query = new Query({ | ||
| client: queryClient, | ||
| queryKey: key, | ||
| queryHash: hashQueryKeyByOptions(key), | ||
| options: { queryFn }, | ||
| }) | ||
|
|
||
| const observer1 = new QueryObserver(queryClient, { | ||
| queryKey: key, | ||
| queryFn, | ||
| }) | ||
|
|
||
| query.addObserver(observer1) | ||
| const promise1 = query.fetch() | ||
|
|
||
| await vi.advanceTimersByTimeAsync(10) | ||
|
|
||
| query.removeObserver(observer1) | ||
|
|
||
| const observer2 = new QueryObserver(queryClient, { | ||
| queryKey: key, | ||
| queryFn, | ||
| }) | ||
|
|
||
| query.addObserver(observer2) | ||
|
|
||
| query.fetch() | ||
|
|
||
| await expect(promise1).rejects.toBeInstanceOf(CancelledError) | ||
| await vi.waitFor(() => expect(query.state.fetchStatus).toBe('idle')) | ||
|
|
||
| expect(queryFn).toHaveBeenCalledTimes(2) | ||
|
|
||
| expect(query.state).toMatchObject({ | ||
| fetchStatus: 'idle', | ||
| status: 'success', | ||
| data: 'data1', | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix flakiness and expectation mismatch in new revert-sync test.
- With fake timers active, waitFor won’t progress the 50ms sleep; advance timers instead to let the second fetch resolve.
- Assert that after a microtask tick the state remains "fetching" (proves revert no longer overrides a new fetch).
- The counter logic returns "data0" for the first completed call; make it pre-increment so the second fetch yields "data1" as asserted.
Apply this diff:
@@
- test('should not override fetching state when revert happens after new observer subscribes', async () => {
+ test('should not override fetching state when revert happens after new observer subscribes', async () => {
@@
- let count = 0
+ let count = 0
@@
- const queryFn = vi.fn(async ({ signal: _signal }) => {
- // Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path
- await sleep(50)
- return 'data' + count++
- })
+ const queryFn = vi.fn(async ({ signal: _signal }) => {
+ // Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path
+ await sleep(50)
+ return 'data' + ++count
+ })
@@
- query.fetch()
+ query.fetch()
+ // Let any queued catch handlers run; fetching state must remain
+ await Promise.resolve()
+ expect(query.state.fetchStatus).toBe('fetching')
@@
- await expect(promise1).rejects.toBeInstanceOf(CancelledError)
- await vi.waitFor(() => expect(query.state.fetchStatus).toBe('idle'))
+ await expect(promise1).rejects.toBeInstanceOf(CancelledError)
+ await vi.advanceTimersByTimeAsync(50)
+ expect(query.state.fetchStatus).toBe('idle')
@@
- expect(query.state).toMatchObject({
+ expect(query.state).toMatchObject({
fetchStatus: 'idle',
status: 'success',
- data: 'data1',
+ data: 'data1',
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/query-core/src/__tests__/query.test.tsx around lines 1196 to 1244,
the test is flaky because fake timers prevent the 50ms sleep from advancing, the
test doesn’t assert the intermediate fetching state after the revert/new fetch
overlap, and the counter returns the wrong data; replace the vi.waitFor that
expects the 50ms sleep to complete with vi.advanceTimersByTimeAsync(50) so the
second fetch can resolve, add a microtask tick (e.g., await Promise.resolve() or
equivalent) and assert query.state.fetchStatus === 'fetching' immediately after
to prove revert didn’t override the new fetch, and change the counter increment
to pre-increment so the second resolved call yields "data1" to match the final
expectation.
|
Hi @TkDodo , I was thinking about the synchronous revert approach and wondered if it might face a similar challenge to what you mentioned in my PR? When onCancel executes synchronously during removeObserver, wouldn't the revert happen immediately before any new observer can be added? Here's a test case that might help illustrate this scenario test('should prevent revert when disabled observer is added after removal', async () => {
const key = queryKey()
const queryFn = vi.fn(async ({ signal: _signal }) => {
await sleep(50)
return 'data'
})
const query = new Query({
client: queryClient,
queryKey: key,
queryHash: hashQueryKeyByOptions(key),
options: { queryFn },
})
const observer1 = new QueryObserver(queryClient, {
queryKey: key,
queryFn,
})
query.addObserver(observer1)
const promise1 = query.fetch()
await vi.advanceTimersByTimeAsync(10)
query.removeObserver(observer1)
const observer2 = new QueryObserver(queryClient, {
queryKey: key,
queryFn,
enabled: false,
})
query.addObserver(observer2)
await expect(promise1).rejects.toBeInstanceOf(CancelledError)
expect(query.state.fetchStatus).toBe('fetching')
expect(query.state.data).toBeUndefined()
})It seems like the synchronous execution might revert the state before the disabled observer gets added, which could lose the fetch state even though no new fetch was initiated. Perhaps checking |
yes, it would, but that is what we want. The flow is this:
now, the new observer can start from a “clean state” and will get us into before, we had this flow:
the test case you showed is wrong because |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9601 +/- ##
===========================================
+ Coverage 45.15% 59.26% +14.11%
===========================================
Files 208 137 -71
Lines 8323 5565 -2758
Branches 1886 1494 -392
===========================================
- Hits 3758 3298 -460
+ Misses 4118 1963 -2155
+ Partials 447 304 -143 🚀 New features to boost your workflow:
|
2 participants
the switch from the callback approach to async/await with catch was necessary to be able to transform errors into revert-state data for imperative query calls; however, this was a change in behavior because catch in async/await doesn't happen immediately - it runs in the next microtask. That opened up an opportunity for a slight race condition if we re-start a fetch in-between. And who does that? Of course, React Strict Mode.
This PR moves the actual state reverting back to a callback (so it happens synchronously), while still keeping the try/catch refactoring merely to transform the promise and the usual error handling
fixes #9579
Summary by CodeRabbit