Devtools + async persistence + custom queryKeyHashFn doesn't work

[lauri865]

Describe the bug

When using both persistence and a custom queryKeyHashFn, the DevTools will crash on initial load with error:

TypeError: Cannot read properties of undefined (reading 'fetchStatus')
    at getQueryStatusColor

Busting the cache on each load "fixes" it. And so does removing the custom queryKeyHashFn.

Just upgraded from v4, and this worked fine then. I also tried with a createSyncStoragePersister, and that works completely fine as well.

Your minimal, reproducible example

https://codesandbox.io/p/devbox/musing-williamson-vgj34y?file=%2Fsrc%2FApp.jsx%3A90%2C29

Steps to reproduce

1. Wrap any app in PersistQueryClientProvider with an indexdb persister example from the docs
2. Add a custom queryKeyHashFn to queryClient defaultOptions.queries.queryKeyHashFn. E.g. return (queryKey)=>hashKey(queryKey) + "_test"
3. Reload the page

It works when there's no cache being loaded. It doesn't when something is loaded from the cache.

Expected behavior

Devtools should support a custom queryKeyHashFn with async persistence.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

Macos, latest chrome

Tanstack Query adapter

react-query

TanStack Query version

5.22.2

TypeScript version

No response

Additional context

No response

[lauri865]

Also works fine with the experimental_createPersister.

[lauri865]

Here's a reproduction: https://codesandbox.io/p/devbox/musing-williamson-vgj34y?file=%2Fsrc%2FApp.jsx%3A90%2C29

Works on the first load, but not on the second. Doesn't display any queries. In my actual case after a few reloads the devtools crash.

Moving devtools under WaitForRestore resolves it in the reproduction, but not in my actual app for some reason (no idea if it's the amount of data or latency from queries being triggered).

Edit: I understand the difference now. One of my queries defines a custom queryKeyHashFn, which triggers the problem regardless of WaitForRestore.

[lauri865]

Seems like getStatusColor is called with an undefined query. Fwiw, just adding a check for whether query is defined is a quick fix for the problem.

The root of problem is likely somewhere here: https://github.com/TanStack/query/blob/8104a407cc9347fbf74b64bcb5adc77c2f8daaea/packages/query-devtools/src/Devtools.tsx#L784

Or here: https://github.com/TanStack/query/blob/8104a407cc9347fbf74b64bcb5adc77c2f8daaea/packages/query-devtools/src/Devtools.tsx#L1324C15-L1326C21

[lauri865]

Seems like DevTools causes the defaultOptions.queries.queryKeyHashFn to be called against a cached query (which is the wrong queryKeyHashFn for this particular query, as it's overriden in the useQuery options). This happens even if I remove that single query from the page. Why would devtools need to rehash a cached query key to begin with, and for 4 times in a row?

This is what the console looks like when I disable devtools:

[lauri865]

Seems like the culprit is: https://github.com/TanStack/query/blob/8104a407cc9347fbf74b64bcb5adc77c2f8daaea/packages/query-devtools/src/Devtools.tsx#L1365C3-L1372C4

Which calls queryCache.find, which in turns calls matchQuery, which for whatever reason rehashes all existing queries: https://github.com/TanStack/query/blob/8104a407cc9347fbf74b64bcb5adc77c2f8daaea/packages/query-core/src/utils.ts#L88

This line in particular: https://github.com/TanStack/query/blob/8104a407cc9347fbf74b64bcb5adc77c2f8daaea/packages/query-core/src/utils.ts#L103

Shouldn't hashing happen only at the time of fetching to make sure we can trust the hash to be exactly what params were passed to the queryFn?

Wouldn't this cause issues and race conditions all over the app (outside of devtools)? Seems like Devtools just highlights the issue because it calls find against all queries, but this could happen in userland as well – active query changes e.g. queryKey, optimistic update tries to find relevant queries, find method rehashes data of a different query, and we end up with a corrupted cache.

For devtools in particular, it's a problem, since it rehashes a cached key, which may be generated by a useQueryOptions queryKeyHashFn, which cannot be serialized. Hence any persisted data will get corrupted by this and not match against memory cache.

Wdyt @TkDodo?

Seems related to #6103, but understand why it's been difficult to reproduce.

[TkDodo]

I've tried your reproduction but it doesn't crash for me. What do I need to do to make it crash please?

[TkDodo]

Shouldn't hashing happen only at the time of fetching to make sure we can trust the hash to be exactly what params were passed to the queryFn?

queries are stored according to their hash. in order to find a query by queryKey in the cache, we need to hash the key and then compare it. This is indeed troublesome if different hashing functions are used per query. Since find only takes a queryKey, we wouldn't know how to hash that key - so we hash it with the options of the key we compare against.

I think the recommendation would generally be to setup query key hashing globally if you need to.

[lauri865]

I've tried your reproduction but it doesn't crash for me. What do I need to do to make it crash please?

Load it once (make sure devtools are open), and then refresh the preview window. It will not load up any queries (cached or subsequent) into the devtool. The toggle devtools button will noy work.

I haven't done a reproduction on the "crash", by which I mean the devtools don't load at all (even empty). But it usually happens after the third refresh, but may require more queries.

[lauri865]

Shouldn't hashing happen only at the time of fetching to make sure we can trust the hash to be exactly what params were passed to the queryFn?

queries are stored according to their hash. in order to find a query by queryKey in the cache, we need to hash the key and then compare it. This is indeed troublesome if different hashing functions are used per query. Since find only takes a queryKey, we wouldn't know how to hash that key - so we hash it with the options of the key we compare against.

I think the recommendation would generally be to setup query key hashing globally if you need to.

Well, this approach seems to still cause issues (as expressed in the related issue). Since active query queryKey can change (e.g. filters change), then rehashing the key arbitrarily can cause race conditions, as well as is not the most performant solution on larger caches. I posit that there's still absolutely no valid reason the rehash a cached queryKey.

[TkDodo]

I'm not sure I understand that, we're not "re-hashing" anything in the matchQuery function. Here's what's happening:

let's say we have the following cache entries:

['posts', 23]
['todos', 'list'],
['todos', 5],

all these 3 queries are stored in the cache, and are already hashed. let's assume posts has a custom hashFn, the others do not. Here's what's stored:

['posts', 23]     ---> hash: "myprefix-['posts', 23]"
['todos', 'list'] ---> hash: "['todos', 'list']"
['todos', 5]      ---> hash: "['todos', 5]"

now when the user says something like: queryCache.find({ queryKey: ['todos', 5] }), we need to hash that key that comes in somehow. Since queryCache.find doesn't pass in how to hash that key, the only thing we can do is to hash it with the function that we know on each query. So when we compare it against ['posts', 23], we would hash it with the function provided for ['posts', 23] to see if it matches.

Please let me know where you think this can cause race conditions because I don't really see it.

[lauri865]

Ah, you're absolutely right. I misunderstood the code / was thrown off by the console logs – sorry for my misunderstanding. I thought it was calling hashQueryKeyByOptions against all the items of the queryCache, but instead it's rehashing the queryKey filter based on the options of all the queries in cache, so o(n), which in most cases should be o(1), unless there's a custom hashQueryKeyByOptions defined for the query (rare if any cases usually). But in practice it probably (hopefully) gets JIT'ed away and not have a significant performance penalty anyways.

So, the problem really only exists on Devtools level.

(e) => e.query.queryHash === props.query.queryHash doesn't pick up the cached query, and queryState returns undefined, which in turn breaks getQueryStatusLabel that expects queryState to exist.

To fix, we could just fall back to QueryRow component prop's query.state, which by definition has to be defined. Happy to create a PR, but I have never worked with Solid before personally, and don't quite understand why there needs to be 4 subsequent calls to the queryCache.find to read the properties of the same query. Would be much easier to add the fallback otherwise (in 1 place, instead of 4, and 3 of which already have nullish coalescing).

Alternatively, a more elegant fix potentially would be a function to find a query in the cache based on the queryHash rather than the queryKey. But not sure if that should exist only on the devtools level or would also be useful in query-core.

I updated the reproduction, and it should break more reliably now (refresh the preview and check the console + query list will be empty even though the counter at the top shows otherwise): https://codesandbox.io/p/devbox/musing-williamson-vgj34y

[TkDodo]

Alternatively, a more elegant fix potentially would be a function to find a query in the cache based on the queryHash rather than the queryKey. But not sure if that should exist only on the devtools level or would also be useful in query-core.

this can be achieved with queryClient.getQueryCache().get(queryHash) instead of using queryClient.find

[lauri865]

Is there a reason why Devtools don't use that? 🤔

[TkDodo]

I guess not. It's a more low-level API, but if it fixes the issue, please feel free to PR the change. It should also be faster.

[TkDodo]

@lauri865 if you want to contribute a fix, please let me know. Otherwise I'll close this issue as stale.