fix(core): make sure queries revert synchronously (#9601)

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

Author: TkDodo

packages/query-core/src/__tests__/query.test.tsx

@@ -1192,4 +1192,54 @@ describe('query', () => {
     expect(initialDataFn).toHaveBeenCalledTimes(1)
     expect(query.state.data).toBe('initial data')
   })
+
+  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',
+    })
+  })
 })

packages/query-core/src/query.ts

@@ -507,7 +507,15 @@ export class Query<
         | Promise<TData>
         | undefined,
       fn: context.fetchFn as () => Promise<TData>,
-      abort: abortController.abort.bind(abortController),
+      onCancel: (error) => {
+        if (error instanceof CancelledError && error.revert) {
+          this.setState({
+            ...this.#revertState,
+            fetchStatus: 'idle' as const,
+          })
+        }
+        abortController.abort()
+      },
       onFail: (failureCount, error) => {
         this.#dispatch({ type: 'failed', failureCount, error })
       },
@@ -550,13 +558,9 @@ export class Query<
       if (error instanceof CancelledError) {
         if (error.silent) {
           // silent cancellation implies a new fetch is going to be started,
-          // so we hatch onto that promise
+          // so we piggyback onto that promise
           return this.#retryer.promise
         } else if (error.revert) {
-          this.setState({
-            ...this.#revertState,
-            fetchStatus: 'idle' as const,
-          })
           // transform error into reverted state data
           // if the initial fetch was cancelled, we have no data, so we have
           // to get reject with a CancelledError

packages/query-core/src/retryer.ts

@@ -10,7 +10,7 @@ import type { CancelOptions, DefaultError, NetworkMode } from './types'
 interface RetryerConfig<TData = unknown, TError = DefaultError> {
   fn: () => TData | Promise<TData>
   initialPromise?: Promise<TData>
-  abort?: () => void
+  onCancel?: (error: TError) => void
   onFail?: (failureCount: number, error: TError) => void
   onPause?: () => void
   onContinue?: () => void
@@ -86,9 +86,10 @@ export function createRetryer<TData = unknown, TError = DefaultError>(
 
   const cancel = (cancelOptions?: CancelOptions): void => {
     if (!isResolved()) {
-      reject(new CancelledError(cancelOptions))
+      const error = new CancelledError(cancelOptions) as TError
+      reject(error)
 
-      config.abort?.()
+      config.onCancel?.(error)
     }
   }
   const cancelRetry = () => {