Fix e2e flake by not consuming abort signal in apiq query options helper

[david-crespo]

Short version

Passing React Query's abort signal into our API fetch calls means the calls get aborted when queries get canceled due to being unmounted. One inconvenient time for this to happen is when React Strict Mode does its thing in dev mode, causing an unmount and remount in the middle of a prefetch in a loader, which doesn't itself blow up because prefetchQuery eats errors, but components that expect prefetched data do blow up because the prefetch failed to populate the query cache. There isn't much advantage to canceling queries, so we can fix this by just not passing the signal through.

Also read this: https://tanstack.com/query/v5/docs/framework/react/guides/query-cancellation

Medium version

With #2597 we started using an options helper with prefetchQuery that passes the abort signal from React Query through into our API calls, which means they get aborted when RQ cancels a query. The existing prefetchQuery did not pass through the signal.

React's strict mode does everything twice (in development mode only) in order to surface things that should be cleaned up and redone between renders, and when it unmounts and remounts the firewall rules query, that causes RQ to cancel the fetch, which was causing our API call to abort, which means it wasn't in the query cache when usePrefetchedQuery expects it to be. I assume it only happens some of the time because of a race, though I don't understand yet what is racing. It's not a race with the request completing because lowering the mock API's random latency of 200-400ms to 50-100ms does not seem to affect the rate of failure.

In any case, after reading the React Query doc on Query Cancellation, I don't think we actually ever need to pass through the abort signal, whether for prefetches or regular queries. There's very little advantage to canceling a request that's already in flight unless you're trying to avoid downloading some big response. Mutations might make more sense to cancel. We take advantage of this during image upload, for example, where we have 6 requests in flight. It makes sense to cancel outstanding ones rather than letting them complete. Mutations are unaffected by this apiq helper — for now we are still using the old useApiMutation for all mutations.

The story of my pain

Beginning with #2597 (though I didn't notice for a few days) we started having a test flake in the create VPC e2e test (example failure). It was reproducible locally in multiple browsers. It failed about half the time with a prefetch error after the transition to VPC detail for a newly created VPC (line 40).

console/test/e2e/networking.e2e.ts

Lines 33 to 40 in 0b5220a

	// New VPC form
	await page.getByRole('link', { name: 'New Vpc' }).click()
	await page.getByRole('textbox', { name: 'Name', exact: true }).fill('another-vpc')
	await page.getByRole('textbox', { name: 'DNS name' }).fill('another-vpc')
	await page.getByRole('button', { name: 'Create VPC' }).click()
	// now we're on the VPC detail, on the firewall rules tab
	await expect(page.getByRole('heading', { name: 'another-vpc' })).toBeVisible()

The error showed up as a failure of the firewall rules prefetch to seed the cache. The query was there after the prefetch call but stuck in pending:

It turned out this one line from #2597 was enough to cause the problem.

 VpcFirewallRulesTab.loader = async ({ params }: LoaderFunctionArgs) => {
   const { project, vpc } = getVpcSelector(params)
-  await apiQueryClient.prefetchQuery('vpcFirewallRulesView', { query: { project, vpc } })
+  await queryClient.prefetchQuery(apiq('vpcFirewallRulesView', { query: { project, vpc } }))
   return null
 }

See if you can spot the difference between the two implementations involved:

apiq

console/app/api/hooks.ts

Line 111 in 0b5220a

queryFn: ({ signal }) => api[method](params, { signal }).then(handleResult(method)),

apiQueryClient.prefetchQuery

console/app/api/hooks.ts

Line 317 in 0b5220a

queryFn: () => api[method](params).then(handleResult(method)),

CancelledError

The cause was hard to figure out with prefetchQuery because that function eats all errors with a .catch(noop). To see what was actually going wrong, I changed it to a fetchQuery, which does through errors if they happen, and showed me this was a CancelledError coming out RQ cleaning up a subscribe due to commitDoubleInvokeEffectsInDEV, is a strict mode thing.

CancelledError stack trace

Error: CancelledError

CancelledError — retryer.ts:62
cancel — retryer.ts:84
removeObserver — query.ts:329
destroy — queryObserver.ts:143
onUnsubscribe — queryObserver.ts:119
(anonymous function) — subscribable.ts:15
safelyCallDestroy — react-dom.development.js:22971
commitHookEffectListUnmount — react-dom.development.js:23139
invokePassiveEffectUnmountInDEV — react-dom.development.js:25246
invokeEffectsInDev — react-dom.development.js:27390
commitDoubleInvokeEffectsInDEV — react-dom.development.js:27363
flushPassiveEffectsImpl — react-dom.development.js:27095
flushPassiveEffects — react-dom.development.js:27023
commitRootImpl — react-dom.development.js:26974
commitRoot — react-dom.development.js:26721
performSyncWorkOnRoot — react-dom.development.js:26156
flushSyncCallbacks — react-dom.development.js:12042
(anonymous function) — react-dom.development.js:25690

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Dec 11, 2024 0:24am