Fix duplicate single-flight revalidation#575
Conversation
|
ryansolid
left a comment
There was a problem hiding this comment.
The diagnosis is right and I can confirm the double pass from reading the code: after the transport consumer applies X-Revalidate and seeds the cache, handleResponse sees the unwrapped plain value, falls into else data = response, and calls applyResponseMetadata(undefined, ...) — which invalidates every cache entry (cacheKeyOp(undefined, ...)) and refetches. So the bug and the test coverage are solid. Thanks for running it down.
The guard condition is the problem: isServerFunction(fn) && router.singleFlight assumes the consumer ran, but there are reachable cases where a server function + single-flight-enabled router still settles with a plain value without the consumer ever being invoked — and in those cases this PR silently drops the default revalidation the old code performed:
- The collector returns
undefined.foldFlightDatain@solidjs/webonly sets theX-Single-Flightresponse header when the hook returns data. The router's owncreateFlightDataCollectorreturnsundefinedfor: norefererheader (e.g.Referrer-Policy: no-referrer, non-browser callers), raw body-carryingResponseoutcomes, cross-origin redirect targets, and — most commonly — when the data-only preload pass collects nothing. That last one hits any app whose queries run in components rather than routepreloads: collection comes back empty, no header, transport returns the decoded plain value without touching the consumer, and the mutation then triggers no revalidation at all. Those component-level queries stay stale. - No
collectFlightDatahook configured server-side (integration without single-flight support): same plain-value path.
Suggestion: instead of predicting from the function's identity, track whether the consumer actually ran during this invocation. The consumer is awaited inside fetchServerFunction before the mutation promise resolves, so the sequencing is reliable:
let flightApplications = 0;
export function setupFlightDataConsumer(router: RouterContext) {
return subscribeFlightData<Record<string, any>>((data, { response }) => {
flightApplications++;
return applyResponseMetadata(response, router.navigatorFactory(), data);
});
}then in invoke, capture flightApplications before running the mutation and pass metadataHandled: flightApplications !== before to handleResponse, keeping your if (!metadataHandled || metadata || flightData) shape. That fixes the duplicate pass identically in the happy path, preserves default revalidation in every fallback path, and drops the need for isServerFunction and the explicit flag on createServerFormAction entirely (a wrapper losing the metadata brand stops mattering). Two overlapping mutations could theoretically cross-attribute a consumer run, but that's a far smaller window than the cases above and errs toward a redundant revalidation rather than a missed one — worth a comment, not machinery.
Would also be good to extend the "no flight payload" test to assert the plain-value server-function path (consumer never invoked) still revalidates.
Replaces the isServerFunction && router.singleFlight guard from #575 with a counter on the flight-data consumer: an action compares it across its mutation to learn whether this response's metadata was actually applied. Predicting from the function's identity missed every response the server returned without flight data (no referrer, empty collection pass, no server-side collector), silently dropping the default revalidation those paths rely on. Adds a regression test for the bare-value server response. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Landed on |
|
Yeah i did this pr with 5.6 sol instead of fable. I think fable would have done better. At least it helped narrow down the issue |
Summary
Problem
The single-flight transport consumer applies
X-Revalidatemetadata and seeds the query cache before returning the unwrapped mutation value. The action layer then treated that plain value as a fresh action result and applied default metadata handling again. That second pass invalidated the newly seeded cache and intermittently issued a follow-up query GET.Verification
pnpm test(306 client tests, 28 server tests, type tests)pnpm build