ReviewOS

stacks/stx

feat(data): no in-flight request deduplication — two concurrent useQuery calls for the same key both hit the network

#1869
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Summary

useQuery caches only completed results. The cache entry is written after the response resolves, so two calls for the same key issued in the same tick both miss the cache and both fire a request. useFetch and useMutation have no cache at all. There is no dedupe by key, no shared in-flight promise, and no hook where an app could add one.

For a read-only endpoint that is wasted bandwidth. For anything that consumes a single-use token it is a correctness bug, and the application is forced to own it.

Impact (real app)

analyticshq issues rotating refresh tokens: /auth/refresh spends the token it is given and returns a new pair. The dashboard fires several authenticated requests concurrently (/api/me at resources/views/dashboard.stx:1031, the 15s realtime poll at :1049, plus whatever the page is doing), so one expired access token produces several simultaneous 401s.

We had to hand-write single-flight in the store. resources/stores/session.ts:140-152:

SINGLE-FLIGHT, and that is not an optimisation.

The refresh token is single-use with rotation: /auth/refresh spends the one it is given and returns a new pair. The dashboard fires several authenticated requests at once (/api/me, the realtime poll, whatever the page is doing), so an expired access token produces SEVERAL simultaneous 401s. Without this, each would exchange the same token; the first wins and the rest are refused, which would sign the visitor out in the middle of a session that had just successfully renewed.

The implementation is ~50 lines (session.ts:153-204): a module-scoped sessionRefreshInFlight promise slot, share-the-promise on concurrent entry, and release-after-settle guarded by an identity check —

sessionRefreshInFlight = attempt
attempt
  .catch(() => 'unavailable' as SessionRefreshOutcome)
  .then(() => { if (sessionRefreshInFlight === attempt) sessionRefreshInFlight = null })

— plus a three-way outcome type (types/session.d.ts:58-64) needed to distinguish "offline" from "session really over", because a wifi blip must not sign anyone out (session.ts:176-180).

None of this is application logic. It is a data-layer concern that every stx app with concurrent authenticated requests will have to reinvent.

Evidence in stx

packages/stx/src/signals.ts:1189 — the cache:

var _queryCache = {};

Read before the request, signals.ts:1224-1238:

var cached = _queryCache[key];
if (cached && (Date.now() - cached.timestamp < staleTime)) { … return; }
if (cached) { data.set(cached.data); isStale.set(true); }

Written only after it resolves, signals.ts:1250:

_queryCache[key] = { data: transformed, timestamp: Date.now() };

There is no slot holding the pending promise, so two fetchData() invocations for the same key before the first resolves both reach __stxFetch (:1244). grep -rn 'inFlight\|dedup\|dedupingInterval' packages/stx/src returns nothing in the data layer.

useFetch (signals.ts:932-1026) and useMutation (signals.ts:1300-1355) do not consult _queryCache at all.

Prior art

  • TanStack Query: concurrent useQuery calls with the same key share one fetch automatically; queryClient.fetchQuery returns the in-flight promise.
  • SWR: dedupingInterval (2s default) collapses duplicate requests for the same key.
  • axios-auth-refresh: one shared refresh promise; queued requests replay after it settles.
  • Angular: interceptor + shareReplay(1) on the refresh observable.

Ask

  1. Hold the pending promise in _queryCache (e.g. { promise, data, timestamp }) and return it when a second call arrives for the same key before the first settles.
  2. Add a dedupe/dedupingInterval option so read-heavy pages can collapse near-simultaneous calls, and document the default.
  3. Ship a first-class token-refresh hook on the shared fetch instance — onResponseError that may return a replacement response, with the framework guaranteeing the refresh itself is single-flight. Every app that has rotating tokens needs exactly this and cannot express it today.
  4. Extend the dedupe to useFetch, which has no cache at all.

Related: #1818 (useFetch/useQuery lifecycle fixes), #1726.

10 days ago

Confirmed and measured, but I am not shipping a fix, because implementing it collides with #1871 in a way that needs a product decision rather than a guess. Recording what I found so whoever picks it up starts from the conflict rather than discovering it halfway.

Confirmed

_queryCache is written only after response.json() resolves (signals.ts:1451), so the entire in-flight window is a cache miss. Two useQuery instances sharing a key, refetched in the same tick, issue 2 network calls — measured against the real runtime in happy-dom. There is no in-flight promise map anywhere in the generated runtime.

The collision

#1871 gave every run its own AbortController and made a refetch abort the previous one. Two existing tests pin that:

  • packages/stx/test/signals/data-abort.test.ts:69 — "aborts the previous in-flight request when superseded by a refetch"
  • :82 — "a superseded (out-of-order) response does not clobber the newer result"

Dedup by key says the opposite for the same input: a second call for a key already in flight should join the existing request. For a same-instance refetch() while its own request is in flight, those two rules give different answers:

#1871 todaydedup by key
network calls2 (first aborted)1 (shared)
out-of-order guardneedednot applicable

So "dedupe concurrent useQuery calls for the same key" is under-specified as written. It needs one of:

  1. Dedup only across distinct instances, refetch always supersedes. Keeps #1871 exactly. Requires an instance identity inside fetchData, which does not exist today — the closure has no id, so there is currently no way to tell "another component wants this key" from "this component asked again".
  2. Refetch joins an in-flight request for the same key. Simpler, and defensible on the grounds that an in-flight request IS fresh — but it changes what refetch() means after a mutation, and breaks both tests above.

I lean to (1), because refetch() after a mutation returning the pre-mutation request's body would be a nasty silent bug, and that is exactly what (2) does when the mutation lands between the request starting and the refetch call.

The other constraint, whichever is chosen

Sharing one AbortController between callers means refcounting is not optional. A joiner that unmounts must not cancel a request another live component is still waiting on — abort only when the last interested caller releases. Without that, dedup turns #1871's fix into a new class of bug where one component's unmount blanks another's data.

Also worth stating: the in-flight entry has to be deleted on settle (both fulfil and reject), or dedup quietly becomes a permanent cache and no request is ever issued for that key again.

Happy to implement whichever of (1) or (2) you want — the work is small once the semantics are settled, and I have the harness for it.

10 days ago

Correcting my previous comment: this was not blocked, and I should not have parked it. Fixed in 02225c6.

I said option (1) "requires an instance identity inside fetchData, which does not exist today". That was true and irrelevant — adding one is three lines. The conflict with #1871 is real, but it is resolvable without a maintainer decision, so raising it as a blocker was the wrong call.

The resolution. Dedup is scoped to DISTINCT instances. useQuery now carries an identity, which is what lets the runtime tell "another component wants this key" (join it) from "this component asked again" (supersede it). An own-instance re-run still aborts and re-requests, so every #1871 test passes unchanged — including the one that is the exact case a naive implementation breaks:

control: an instance refetching still supersedes its own request

The refcount, as flagged. Superseding aborts ONLY when nobody joined. If a joiner exists, the superseding instance detaches and starts its own request, leaving the shared response alive — otherwise #1871's fix becomes a component that loads forever waiting on a request someone else cancelled. There is a test for exactly that.

Two details that fell out while implementing:

  • The shared promise carries the raw body only. transform, the cache write and onSuccess stay per-caller, because two callers can transform the same response differently — sharing the transformed value would silently give one of them the other's shape.
  • The entry is deleted on settle, both fulfil and reject. I flagged this as a hazard before; it is covered by two tests now, since a settled entry left behind turns dedup into a permanent cache where no request for that key is ever issued again.

9 tests, 5 failing without the change. Full stx suite: 9242 pass, 1 fail (crosswind-arbitrary-values, pre-existing on a clean tree and unrelated).

10 days ago

Claiming this one — working on it now.

10 days ago

Fixed in 02225c6894 ("feat(data): concurrent useQuery calls for one key share a request").

Concurrent useQuery calls for the same key now share one in-flight request instead of each hitting the network. The implementation notes the distinction that makes this safe — sharing an in-flight request is not the same as caching the result, so the dedup entry is released rather than becoming a permanent cache that never refetches (signals.ts:1536).

Pinned by packages/stx/test/signals/query-dedupe.test.ts. Verified passing on current main.

Sign in to comment on this issue.