[PersistQueryClientProvider] isRestoring is always true with idb-keyval

[Choo-Xing-Yu]

Describe the bug

Hi, I've recently faced with an online issue whereby a small percentage (~1%) of my users are stuck in a loading screen (which I render when isRestoring is true).

I've got an alarm regarding JSErrors spiking, mostly coming from "Can't find variable: IndexedDB" and "Unable to open database file on disk".

I peeked at the source code and convinced myself its fine due to the finally block eventually setting isRestoring to false.
However I was looking at main branch instead of the version my project was relying on... (sad day :( )

main branch

  persistQueryClientRestore(options)
    .then(() => refs.current.onSuccess?.())
    .catch(() => refs.current.onError?.())
    .finally(() => {
      setIsRestoring(false)
    })

My project relies on v5.18.1, whose code snippet looks like this

v5.18.1

  persistQueryClientRestore(options).then(async () => {
    try {
      await refs.current.onSuccess?.()
    } finally {
      setIsRestoring(false)
    }
  })

The configuration I have setup is this:

import { get, set, del } from 'idb-keyval'
import {
  PersistedClient,
  Persister,
} from '@tanstack/react-query-persist-client'

/**
 * Creates an Indexed DB persister
 * @see https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API
 */
export function createIDBPersister(idbValidKey: IDBValidKey) {
  return {
    persistClient: (client: PersistedClient) => set(idbValidKey, client),
    restoreClient: () => get<PersistedClient>(idbValidKey),
    removeClient: () => del(idbValidKey),
  } satisfies Persister;
}

I was able to hotfix the problem by straight up removing isRestoring + persisterOptions.

I've spent the past 2 days staring at the source code of PersistQueryProvider and possibly found a scenario where isRestoring is never set back to false.

Even though v5.76.2 solves this, I'd argue that async persist storage should come with a timeout, (perhaps set to 5000ms) when restoring/persisting.
Historically at our scale we've encountered async processes never terminating before, which could replicate the above issue (isRestoring never false).

Let me know whether you guys agree with adding a timeout for async persistance, I can raise a PR + unit tests for it

Your minimal, reproducible example

https://codesandbox.io/p/sandbox/async-storage-restoring-hung-vlx2xj

Steps to reproduce

1. Use PersistQueryClientProvider with IDB from document https://tanstack.com/query/latest/docs/framework/react/plugins/persistQueryClient#building-a-persister
2. removeClient throw a synchronous error
3. isRestoring is always true

Expected behavior

As a user, i expect isRestoring to be set to false if any of the Persister method fails (persistClient, restoreClient, removeClient)

Platform

• Android, iOS. More prevalent on iOS
• Replicable on Chrome

Tanstack Query adapter

react-query

TanStack Query version

v5.18.1

TypeScript version

v5.3.3

[TkDodo]

This was fixed in v5.73.1

I think it works fine in the latest version: https://codesandbox.io/p/devbox/async-storage-restoring-hung-forked-r5htvc?workspaceId=ws_XiBCv7MgHSszKyXHXG5ray

[Choo-Xing-Yu]

@TkDodo

This was fixed in v5.73.1

I think it works fine in the latest version: https://codesandbox.io/p/devbox/async-storage-restoring-hung-forked-r5htvc?workspaceId=ws_XiBCv7MgHSszKyXHXG5ray

Even though v5.76.2 solves this, I'd argue that async persist storage should come with a timeout, (perhaps set to 5000ms) when restoring/persisting.
Historically at our scale we've encountered async processes never terminating before, which could replicate the above issue (isRestoring never false).

how about this? Adding a timeout for persistence/restoration to prevent cases where an the async process never resolved

[TkDodo]

you should be able to handle this in your custom idb persister.

[Choo-Xing-Yu]

you should be able to handle this in your custom idb persister.

you're not wrong.

perhaps this is more of a feature request. building it in improves the robustness, as idt anyone would expect restoration to take significant time (eg > 5s)

[TkDodo]

Idk what you mean with “persisting storage should come with a timeout” ? Are you talking about writing to the storage, or do you want a timeout for restore?

If you mean restore, I don’t see why. If the restore takes 5 seconds but still has value for you because fetching those things from the API would take even longer, why should we error out? The restore process needs to return a Promise, and it can fail that promise if it wants to. If it returns a never ending process, that’s an issue with the persisters’ implementation. We also don’t timeout for the queryFn btw.

[Choo-Xing-Yu]

Idk what you mean with “persisting storage should come with a timeout” ? Are you talking about writing to the storage, or do you want a timeout for restore?

timeout for restoration, so there's an upper-bound (best if configurable) for isRestoring to be set to false.

If you mean restore, I don’t see why. If the restore takes 5 seconds but still has value for you because fetching those things from the API would take even longer, why should we error out? The restore process needs to return a Promise, and it can fail that promise if it wants to. If it returns a never ending process, that’s an issue with the persisters’ implementation. We also don’t timeout for the queryFn btw.

this scenario means isRestoring might not be suitable to determine whether a page is currently loading correct?
I've made the assumption that restoration of persisted data will always be quicker than fetching an api (~2s @ PCT75) which is why I've used it on a component level to render a top-level loading. (this is my bad)

Right now I'm looking for ways to prevent isRestoring to always be stuck as true, and this is a potential scenario.

[TkDodo]

again, you can do this in the restore function. Just fail the promise after 5 seconds.