fix(graphcache): Deduplicate cache misses of in-flight operations

[JoviDeCroock]

Alternative to #3909. Graphcache reexecutes a query every time its data changes, so a burst of subscription events can send a new request for every second event while the slow query's first request is still in flight. Blocking those reexecutes in the client brings back #3254. Deduplicating every in-flight reexecute in Graphcache stalls queries whose request never completes, and lets the older in-flight result overwrite newer subscription results.

This defers only the requests that subscription results cause while a query's request is in flight, and refetches the query once after that request completes. Other reexecutes, including those caused by mutations, send requests as before. Core is unchanged, apart from a test for the dropped query case.

[changeset-bot]

🦋 Changeset detected

Latest commit: e776d4d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@urql/exchange-graphcache	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[cloudflare-workers-and-pages]

Deploying urql with    Cloudflare Pages

Latest commit:	e776d4d
Status:	✅  Deploy successful!
Preview URL:	https://08bf8601.urql.pages.dev
Branch Preview URL:	https://fix-graphcache-inflight-dedu.urql.pages.dev

View logs

[EDjur]

Thanks, #3912 fixes our case. Our list reads as a miss, and the duplicates go from 3 to 0.

One gap: a partial result still duplicates. With a schema, a read with a missing nullable field goes down the partial path. That path emits a stale result and reexecutes the query as network-only, which skips the new in-flight check. So each reexecute while the request is pending sends another request.

This test (for the deduplication block) fails on this branch with 4 requests instead of 2. It needs buildSchema and introspectionFromSchema imported from graphql.

Test

  it('deduplicates reexecutes of in-flight queries with partial results', async () => {
    const schema = introspectionFromSchema(
      buildSchema(`
        type Query {
          authors: [Author!]!
        }

        type Author {
          id: ID!
          name: String
          bio: String
        }
      `)
    );

    const author = {
      __typename: 'Author',
      id: '123',
      name: 'Author',
      bio: 'Bio',
    };

    const tick = async () => {
      for (let i = 0; i < 20; i++) await Promise.resolve();
    };

    const pending: (() => void)[] = [];
    const onNetwork = vi.fn();
    const network: Exchange = () => ops$ =>
      pipe(
        ops$,
        filter(op => op.kind !== 'teardown'),
        tap(onNetwork),
        mergeMap(op =>
          fromPromise(
            new Promise<OperationResult>(resolve => {
              pending.push(() =>
                resolve({
                  operation: op,
                  data: { __typename: 'Query', authors: [author] },
                  hasNext: false,
                  stale: false,
                })
              );
            })
          )
        )
      );

    const client = createClient({
      url: 'http://0.0.0.0',
      exchanges: [cacheExchange({ schema }), network],
    });

    pipe(
      client.query(
        gql`
          {
            authors {
              id
              name
            }
          }
        `,
        {}
      ),
      subscribe(() => {})
    );
    await tick();
    pending.shift()!();
    await tick();

    // `bio` is nullable and not cached, so this is a partial result and
    // Graphcache sends a network-only request for it
    const operation = client.createRequestOperation('query', {
      key: 2,
      query: gql`
        {
          authors {
            id
            name
            bio
          }
        }
      `,
      variables: {},
    });
    pipe(
      client.executeRequestOperation(operation),
      subscribe(() => {})
    );
    await tick();
    expect(onNetwork).toHaveBeenCalledTimes(2);

    for (let i = 0; i < 4; i++) {
      client.reexecuteOperation(operation);
      await tick();
    }

    expect(onNetwork).toHaveBeenCalledTimes(2);
  });

Skipping that network-only reexecute while inFlightOperations has the key makes it pass. But it breaks looping protection > applies stale to blocked looping queries on the call count (3 → 2), because that test expects a reexecute for a request that's still pending. I'll leave that to you.

[EDjur]

Thanks @JoviDeCroock for this fix - really appreciate it 🙏

It solves our case but just as an FYI it seems to leave one gap for partial results. I'm not sure that matters that much but thought it might be useful context.

[EDjur]

Thanks for the update, the partial case is fixed on my end too.

One more case I ran into while checking it: if a mutation invalidates data while a reload of a query is in flight, the query ends up with data: null.

The reload was sent before the mutation, so its result can't restore the invalidated entity. On main, the refetch that the invalidation triggers is sent after the mutation, and that's what fills it back in. This PR skips that refetch because a request is already in flight. A new query for the same data then doesn't get a result either, since its miss is dropped by the loop protection. It recovers on the next update that touches it.

This test (for the deduplication block) passes on main and fails on this branch:

Test

  it('refetches in-flight queries that a mutation invalidated', async () => {
    const authorsQuery = gql`
      query {
        authors {
          id
          name
        }
      }
    `;

    const mutation = gql`
      mutation {
        deleteAuthor
      }
    `;

    const author = { __typename: 'Author', id: '123', name: 'Author' };

    let pending: (() => void)[] = [];
    const tick = async () => {
      for (let i = 0; i < 20; i++) await Promise.resolve();
    };
    const flush = async () => {
      await tick();
      const resolvers = pending;
      pending = [];
      resolvers.forEach(resolve => resolve());
      await tick();
    };

    // Network exchange that holds query responses until `flush()`
    const network: Exchange = () => ops$ =>
      pipe(
        ops$,
        filter(op => op.kind !== 'teardown'),
        mergeMap(op =>
          op.kind === 'mutation'
            ? fromValue({
                operation: op,
                data: { __typename: 'Mutation', deleteAuthor: true },
                hasNext: false,
                stale: false,
              })
            : fromPromise(
                new Promise<OperationResult>(resolve => {
                  pending.push(() =>
                    resolve({
                      operation: op,
                      data: { __typename: 'Query', authors: [author] },
                      hasNext: false,
                      stale: false,
                    })
                  );
                })
              )
        )
      );

    const client = createClient({
      url: 'http://0.0.0.0',
      exchanges: [
        cacheExchange({
          updates: {
            Mutation: {
              deleteAuthor: (_data, _args, cache) => {
                cache.invalidate({ __typename: 'Author', id: '123' });
              },
            },
          },
        }),
        network,
      ],
    });

    const onResult = vi.fn();
    pipe(client.query(authorsQuery, {}), subscribe(onResult));
    await flush();

    // Reload the query, then invalidate it while the reload is in-flight
    pipe(
      client.query(authorsQuery, {}, { requestPolicy: 'network-only' }),
      subscribe(() => {})
    );
    await tick();
    pipe(
      client.mutation(mutation, {}),
      subscribe(() => {})
    );
    await tick();

    for (let i = 0; i < 3; i++) await flush();

    // The reload's result is older than the mutation, so it can't restore
    // Author:123, and the query ends up with `data: null`
    const lastResult = onResult.mock.calls[onResult.mock.calls.length - 1][0];
    expect(lastResult.data).toMatchObject({
      authors: [{ id: '123', name: 'Author' }],
    });
  });

[EDjur]

Thanks again @JoviDeCroock - nothing more from me this time!

[EDjur]

Sorry, one more after all. We ran into this one in our app.

If a subscription event invalidates data while a reload is in flight, the query settles on the reload's result, which is from before the event. On main, the refetch that the event triggers goes out and brings the query up to date. This PR skips it because a request is already in flight. Unlike the mutation case the data isn't null, so nothing looks wrong: the query just shows old data until something else refetches it.

Dropping the in-flight mark for subscriptions, as you did for mutations, would bring the duplicate requests back. What worked for us was to remember that the query was invalidated during the flight, and refetch it once when its result lands.

This test (for the deduplication block) passes on main and fails on this branch:

Test

  it('refetches in-flight queries that a subscription invalidated', async () => {
    const authorQuery = gql`
      query {
        author {
          id
          name
        }
      }
    `;

    const subscription = gql`
      subscription {
        authorUpdated
      }
    `;

    // The server answers with its state at the time a request is sent
    let serverName = 'Before';
    let pending: (() => void)[] = [];
    const events = makeSubject<void>();
    const tick = async () => {
      for (let i = 0; i < 20; i++) await Promise.resolve();
    };
    const flush = async () => {
      await tick();
      const resolvers = pending;
      pending = [];
      resolvers.forEach(resolve => resolve());
      await tick();
    };

    const network: Exchange = () => ops$ =>
      pipe(
        ops$,
        filter(op => op.kind !== 'teardown'),
        mergeMap(op => {
          if (op.kind === 'subscription') {
            return pipe(
              events.source,
              map(() => ({
                operation: op,
                data: { __typename: 'Subscription', authorUpdated: true },
                hasNext: true,
                stale: false,
              }))
            );
          }

          const name = serverName;
          return fromPromise(
            new Promise<OperationResult>(resolve => {
              pending.push(() =>
                resolve({
                  operation: op,
                  data: {
                    __typename: 'Query',
                    author: { __typename: 'Author', id: '123', name },
                  },
                  hasNext: false,
                  stale: false,
                })
              );
            })
          );
        })
      );

    const client = createClient({
      url: 'http://0.0.0.0',
      exchanges: [
        cacheExchange({
          updates: {
            Subscription: {
              authorUpdated: (_data, _args, cache) => {
                cache.invalidate({ __typename: 'Author', id: '123' });
              },
            },
          },
        }),
        network,
      ],
    });

    pipe(
      client.subscription(subscription, {}),
      subscribe(() => {})
    );

    const onResult = vi.fn();
    pipe(client.query(authorQuery, {}), subscribe(onResult));
    await flush();

    // Reload the query, then change and invalidate the author while the reload is in-flight
    pipe(
      client.query(authorQuery, {}, { requestPolicy: 'network-only' }),
      subscribe(() => {})
    );
    await tick();
    serverName = 'After';
    events.next();
    await tick();

    for (let i = 0; i < 3; i++) await flush();

    // The reload was sent before the event, so its result is out of date
    const lastResult = onResult.mock.calls[onResult.mock.calls.length - 1][0];
    expect(lastResult.data).toMatchObject({
      author: { id: '123', name: 'After' },
    });
  });

[JoviDeCroock]

Hah, yes I had that one fixed locally but looks like I forgot to push

[EDjur]

One more, sorry, but this one isn't caused by this PR and shouldn't hold it up. It happens on main too. I'm raising it here because a fix probably builds on the in-flight tracking this PR adds.

If a mutation updates a query while its network-only request is in flight, Graphcache reexecutes the query as cache-first and emits the cache hit as a final result. The query resolves with the cached data before its request completes, so anything new on the server is missing. Without another subscriber it takes a second mutation during the flight, and the pending request is then cancelled.

On this branch, marking cache results stale while inFlightOperations has the key makes the test below pass, and the rest of the suite stays green.

This test (for cacheExchange.test.ts) fails on main and on this branch:

Test

  it('does not resolve network-only queries from the cache while their request is in-flight', async () => {
    const authorsQuery = gql`
      query {
        authors {
          id
          name
        }
      }
    `;

    const mutation = gql`
      mutation {
        updateAuthor {
          id
          name
        }
      }
    `;

    // The server answers with its state at the time a request is sent
    const serverAuthors = [{ __typename: 'Author', id: '123', name: 'Author' }];
    let pending: (() => void)[] = [];
    const tick = async () => {
      for (let i = 0; i < 20; i++) await Promise.resolve();
    };
    const flush = async () => {
      await tick();
      const resolvers = pending;
      pending = [];
      resolvers.forEach(resolve => resolve());
      await tick();
    };

    const network: Exchange = () => ops$ =>
      pipe(
        ops$,
        filter(op => op.kind !== 'teardown'),
        mergeMap(op => {
          if (op.kind === 'mutation') {
            return fromValue({
              operation: op,
              data: {
                __typename: 'Mutation',
                updateAuthor: {
                  __typename: 'Author',
                  id: '123',
                  name: 'Renamed',
                },
              },
              hasNext: false,
              stale: false,
            });
          }

          const authors = [...serverAuthors];
          return fromPromise(
            new Promise<OperationResult>(resolve => {
              pending.push(() =>
                resolve({
                  operation: op,
                  data: { __typename: 'Query', authors },
                  hasNext: false,
                  stale: false,
                })
              );
            })
          );
        })
      );

    const client = createClient({
      url: 'http://0.0.0.0',
      exchanges: [cacheExchange({}), network],
    });

    // A component shows the list
    pipe(
      client.query(authorsQuery, {}),
      subscribe(() => {})
    );
    await flush();

    // An author is added on the server, and the list is reloaded
    serverAuthors.push({ __typename: 'Author', id: '456', name: 'New' });
    let reloaded: OperationResult | undefined;
    client
      .query(authorsQuery, {}, { requestPolicy: 'network-only' })
      .toPromise()
      .then(result => {
        reloaded = result;
      });
    await tick();

    // A mutation updates an author while the reload is in-flight
    pipe(
      client.mutation(mutation, {}),
      subscribe(() => {})
    );
    await tick();

    // The reload resolves with the cached list before its request completes
    expect(reloaded).toBeUndefined();

    await flush();
    expect(reloaded!.data.authors).toHaveLength(2);
  });

[JoviDeCroock]

Talking to a bot is becoming tiring mate 😅

[EDjur]

It never stops does it 🙃

Appreciate you putting this PR together 🫶