Repository: reduxjs/redux-toolkit

Problem statement:
RTK-Query - keepUnusedDataFor has problems with very high values
I use RTK-Query for api fetches in my project, and I've met with an issue I can't fully understand.

Use Case:

I open a modal, once the modal mounts, I call a query endpoint by the auto generated query hook. So this will trigger a couple of actions, first "api/executeQuery/pending", then "api/executeQuery/fulfilled", so far so good, everything works as expected, data is in store, we are happy. Then I close the modal, component unmounts, this triggers yet another 2 actions, first "api/subscriptions/unsubscribeQueryResult", then "api/queries/removeQueryResult", this will do what it says, remove the data, because there are no more subscribers. 

So my goal is to prolong the cache time of that query result, because this modal is being opened frequently, so going through the docs I see "keepUnusedDataFor" option which in theory would do just I what I want. 

Reference from docs: 
**"Once the subscription is removed (e.g. when last component subscribed to the data unmounts), after an amount of time (default 60 seconds), the data will be removed from the cache. The expiration time can be configured with the keepUnusedDataFor"**

As I understand this, for my use-case, the query that ran when the modal was open, should be kept, for at least 60 seconds, till after the component unmounts. But in reality, the "api/subscriptions/unsubscribeQueryResult" and the "api/queries/removeQueryResult" actions run immediately after modal unmounts, there is no 60 seconds delay. I've tried setting it to longer, but was cheap shot obviously, I miss out on something or just not understanding clearly how this should behave.

Ideas?

Package versions:
    "redux": "^4.0.5",
    "react-redux": "^7.2.2",
    "redux-thunk": "^2.3.0",
    "@reduxjs/toolkit": "^1.8.3",



Hints:
That is how it should work. Are you running any mutation that would invalidate that query? Because then instead of refetching it (which would refetch data that is potentially never being displayed again), that data is removed from cache entries without subscribers.
> That is how it should work. Are you running any mutation that would invalidate that query? Because then instead of refetching it (which would refetch data that is potentially never being displayed again), that data is removed from cache entries without subscribers.

No mutation. 

But then this sentence, -  "Once the subscription is removed (e.g. when last component subscribed to the data unmounts), after an amount of time (default 60 seconds), the data will be removed from the cache." - is just not clear at all about the behaviour IMO.

The lifecycle should be: Component unsubscribes (unmounts) -> Wait 60 seconds -> Remove data.
What in reality is: Component unsubscribes (unmounts) -> Remove data. // where did the 60 seconds play part here?
> The lifecycle should be: Component unsubscribes (unmounts) -> Wait 60 seconds -> Remove data.

Yes, that's what it should be and usually is and that's why I'm asking you for a reproduction ;)
> > The lifecycle should be: Component unsubscribes (unmounts) -> Wait 60 seconds -> Remove data.
> 
> Yes, that's what it should be and usually is and that's why I'm asking you for a reproduction ;)

```
export const weatherApi = createApi({
  reducerPath: "weatherApi",
  baseQuery: fetchBaseQuery({ baseUrl }),
  keepUnusedDataFor: 3600000, // 1 hour cache time
  endpoints: (builder) => ({
    getCities: builder.query({
      query: () => `locations/v1/topcities/50?apikey=${apiKey}`,
    }),
    getForecast: builder.query({
      query: (locationKey) =>
        `forecasts/v1/daily/1day/${locationKey}?apikey=${apiKey}`,
    }),
  }),
});
```

```
function Modal() {
  const cities = weatherApi.useGetCitiesQuery();
  return <div>...</div>;
}
```

<img width="600" alt="Screenshot 2022-07-22 at 11 53 53" src="https://user-images.githubusercontent.com/7202966/180414668-84b54128-c1b6-4553-aabd-0a8624d773cf.png">


Please make a CodeSandbox that actually shows the problem - I won't be able to see the problem from snippets. 

This example is very similar to whay you are doing and works perfectly: https://codesandbox.io/s/stoic-pateu-4w8vdr
> Please make a CodeSandbox that actually shows the problem - I won't be able to see the problem from snippets.
> 
> This example is very similar to whay you are doing and works perfectly: https://codesandbox.io/s/stoic-pateu-4w8vdr

https://codesandbox.io/s/rtk-query-keepunuseddatafor-ctqxgc
Ah. It works if you use a realistic value like 3600 - the value is in seconds, not milliseconds, so you're specifying 41 days here.

Of course, that should still work as well so I'm a bit confused. 🤔 

Further reserach shows that browsers get an integer overflow with timers over 20 days: https://developer.mozilla.org/en-US/docs/Web/API/setTimeout#maximum_delay_value

I guess we'll have to add a check for that.
> Ah. It works if you use a realistic value like 3600 - the value is in seconds, not milliseconds, so you're specifying 41 days here.
> 
> Of course, that should still work as well so I'm a bit confused. 🤔
> 
> Further reserach shows that browsers get an integer overflow with timers over 20 days: https://developer.mozilla.org/en-US/docs/Web/API/setTimeout#maximum_delay_value
> 
> I guess we'll have to add a check for that.

You are right, I was misreading the description about the unit, and the number was too great. Thank you very much for spotting it.
Hmm. This is labeled as a 1.9 item, but we should put this out as a 1.8.x bugfix before then.

Patch:
diff --git a/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts b/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts
--- a/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts
+++ b/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts
@@ -28,6 +28,13 @@ declare module '../../endpointDefinitions' {
   }
 }
 
+// Per https://developer.mozilla.org/en-US/docs/Web/API/setTimeout#maximum_delay_value , browsers store
+// `setTimeout()` timer values in a 32-bit int. If we pass a value in that's larger than that,
+// it wraps and ends up executing immediately.
+// Our `keepUnusedDataFor` values are in seconds, so adjust the numbers here accordingly.
+export const THIRTY_TWO_BIT_MAX_INT = 2_147_483_647
+export const THIRTY_TWO_BIT_MAX_TIMER_SECONDS = 2_147_483_647 / 1_000 - 1
+
 export const build: SubMiddlewareBuilder = ({ reducerPath, api, context }) => {
   const { removeQueryResult, unsubscribeQueryResult } = api.internalActions
 
@@ -87,6 +94,14 @@ export const build: SubMiddlewareBuilder = ({ reducerPath, api, context }) => {
       ] as QueryDefinition<any, any, any, any>
       const keepUnusedDataFor =
         endpointDefinition?.keepUnusedDataFor ?? config.keepUnusedDataFor
+      // Prevent `setTimeout` timers from overflowing a 32-bit internal int, by
+      // clamping the max value to be at most 1000ms less than the 32-bit max.
+      // Look, a 24.8-day keepalive ought to be enough for anybody, right? :)
+      // Also avoid negative values too.
+      const finalKeepUnusedDataFor = Math.max(
+        0,
+        Math.min(keepUnusedDataFor, THIRTY_TWO_BIT_MAX_TIMER_SECONDS)
+      )
 
       const currentTimeout = currentRemovalTimeouts[queryCacheKey]
       if (currentTimeout) {
@@ -99,7 +114,7 @@ export const build: SubMiddlewareBuilder = ({ reducerPath, api, context }) => {
           api.dispatch(removeQueryResult({ queryCacheKey }))
         }
         delete currentRemovalTimeouts![queryCacheKey]
-      }, keepUnusedDataFor * 1000)
+      }, finalKeepUnusedDataFor * 1000)
     }
   }
 }


Test patch:
diff --git a/packages/toolkit/src/query/tests/cacheCollection.test.ts b/packages/toolkit/src/query/tests/cacheCollection.test.ts
--- a/packages/toolkit/src/query/tests/cacheCollection.test.ts
+++ b/packages/toolkit/src/query/tests/cacheCollection.test.ts
@@ -2,6 +2,10 @@ import { createApi, fetchBaseQuery } from '@reduxjs/toolkit/query'
 import { configureStore } from '@reduxjs/toolkit'
 import { waitMs } from './helpers'
 import type { Middleware, Reducer } from 'redux'
+import {
+  THIRTY_TWO_BIT_MAX_INT,
+  THIRTY_TWO_BIT_MAX_TIMER_SECONDS,
+} from '../core/buildMiddleware/cacheCollection'
 
 beforeAll(() => {
   jest.useFakeTimers('legacy')
@@ -52,6 +56,35 @@ test(`query: await cleanup, keepUnusedDataFor set`, async () => {
   expect(onCleanup).toHaveBeenCalled()
 })
 
+test(`query: handles large keepUnuseDataFor values over 32-bit ms`, async () => {
+  const { store, api } = storeForApi(
+    createApi({
+      baseQuery: fetchBaseQuery({ baseUrl: 'https://example.com' }),
+      endpoints: (build) => ({
+        query: build.query<unknown, string>({
+          query: () => '/success',
+        }),
+      }),
+      keepUnusedDataFor: THIRTY_TWO_BIT_MAX_TIMER_SECONDS - 10,
+    })
+  )
+
+  store.dispatch(api.endpoints.query.initiate('arg')).unsubscribe()
+
+  // Shouldn't have been called right away
+  jest.advanceTimersByTime(1000), await waitMs()
+  expect(onCleanup).not.toHaveBeenCalled()
+
+  // Shouldn't have been called any time in the next few minutes
+  jest.advanceTimersByTime(1_000_000), await waitMs()
+  expect(onCleanup).not.toHaveBeenCalled()
+
+  // _Should_ be called _wayyyy_ in the future (like 24.8 days from now)
+  jest.advanceTimersByTime(THIRTY_TWO_BIT_MAX_TIMER_SECONDS * 1000),
+    await waitMs()
+  expect(onCleanup).toHaveBeenCalled()
+})
+
 describe(`query: await cleanup, keepUnusedDataFor set`, () => {
   const { store, api } = storeForApi(
     createApi({