Repository: apollographql/apollo-client

Problem statement:
`useBackgroundQuery` hangs on loading when refetching after error
### Issue Description

When an error is encountered in `useBackgroundQuery`/`useReadQuery`, an error is thrown to the nearest suspense boundary. In some cases, users may want to execute a `refetch` from the error boundary to try the query again. When executing `refetch`, the component rendering `useReadQuery` is re-suspended and the Suspense boundary is displayed. In some cases, the query hangs on the loading state after the refetch. From my digging into this issue, this appears to happen when an error is returned again from the server after the refetch. The query _is_ executed through the network, but the result is never reported to `useReadQuery`. This does _not_ appear to happen when a refetch occurs and a successful result is returned after the refetch.

I've attached a repo with a reproduction of this issue.

### Link to Reproduction

https://github.com/jerelmiller/apollo-client-issue-11179

### Reproduction Steps

1. Run the demo app
2. Reload the page until an error message is displayed (the synthetic error is returned 50% of the time)
3. Hit the "retry" button
4. If the response is successful, note that it works as intended.

Now bump the synthetic error rate to `1` to always trigger this:

```diff
  query ArtistsQuery {
-    me @synthetics(timeout: 1000, errorRate: 0.5) {
+    me @synthetics(timeout: 1000, errorRate: 1) {
      albums {
        edges {
          node {
            id
            name
          }
        }
      }
    }
  }
```

1. Click the "retry" button and notice that the "Loading" message is stuck there


Hints:

Patch:
diff --git a/.size-limit.cjs b/.size-limit.cjs
--- a/.size-limit.cjs
+++ b/.size-limit.cjs
@@ -1,7 +1,7 @@
 const checks = [
   {
     path: "dist/apollo-client.min.cjs",
-    limit: "38107",
+    limit: "38190",
   },
   {
     path: "dist/main.cjs",
@@ -10,7 +10,7 @@ const checks = [
   {
     path: "dist/index.js",
     import: "{ ApolloClient, InMemoryCache, HttpLink }",
-    limit: "31980",
+    limit: "32044",
   },
   ...[
     "ApolloProvider",
diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts
--- a/src/core/ObservableQuery.ts
+++ b/src/core/ObservableQuery.ts
@@ -931,6 +931,32 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`,
     return this.reobserveAsConcast(newOptions, newNetworkStatus).promise;
   }
 
+  public resubscribeAfterError(
+    onNext: (value: ApolloQueryResult<TData>) => void,
+    onError?: (error: any) => void,
+    onComplete?: () => void
+  ): ObservableSubscription;
+
+  public resubscribeAfterError(
+    observer: Observer<ApolloQueryResult<TData>>
+  ): ObservableSubscription;
+
+  public resubscribeAfterError(...args: [any, any?, any?]) {
+    // If `lastError` is set in the current when the subscription is re-created,
+    // the subscription will immediately receive the error, which will
+    // cause it to terminate again. To avoid this, we first clear
+    // the last error/result from the `observableQuery` before re-starting
+    // the subscription, and restore the last value afterwards so that the
+    // subscription has a chance to stay open.
+    const last = this.last;
+    this.resetLastResults();
+
+    const subscription = this.subscribe(...args);
+    this.last = last;
+
+    return subscription;
+  }
+
   // (Re)deliver the current result to this.observers without applying fetch
   // policies or making network requests.
   private observe() {
diff --git a/src/react/cache/QueryReference.ts b/src/react/cache/QueryReference.ts
--- a/src/react/cache/QueryReference.ts
+++ b/src/react/cache/QueryReference.ts
@@ -248,6 +248,12 @@ export class InternalQueryReference<TData = unknown> {
   }
 
   private handleError(error: ApolloError) {
+    this.subscription.unsubscribe();
+    this.subscription = this.observable.resubscribeAfterError(
+      this.handleNext,
+      this.handleError
+    );
+
     switch (this.status) {
       case "loading": {
         this.status = "idle";
diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts
--- a/src/react/hooks/useQuery.ts
+++ b/src/react/hooks/useQuery.ts
@@ -209,21 +209,8 @@ class InternalState<TData, TVariables extends OperationVariables> {
           };
 
           const onError = (error: Error) => {
-            const last = obsQuery["last"];
             subscription.unsubscribe();
-            // Unfortunately, if `lastError` is set in the current
-            // `observableQuery` when the subscription is re-created,
-            // the subscription will immediately receive the error, which will
-            // cause it to terminate again. To avoid this, we first clear
-            // the last error/result from the `observableQuery` before re-starting
-            // the subscription, and restore it afterwards (so the subscription
-            // has a chance to stay open).
-            try {
-              obsQuery.resetLastResults();
-              subscription = obsQuery.subscribe(onNext, onError);
-            } finally {
-              obsQuery["last"] = last;
-            }
+            subscription = obsQuery.resubscribeAfterError(onNext, onError);
 
             if (!hasOwnProperty.call(error, "graphQLErrors")) {
               // The error is not a GraphQL error


Test patch:
diff --git a/src/react/hooks/__tests__/useBackgroundQuery.test.tsx b/src/react/hooks/__tests__/useBackgroundQuery.test.tsx
--- a/src/react/hooks/__tests__/useBackgroundQuery.test.tsx
+++ b/src/react/hooks/__tests__/useBackgroundQuery.test.tsx
@@ -3503,6 +3503,258 @@ describe("useBackgroundQuery", () => {
         },
       ]);
     });
+
+    it("can refetch after error is encountered", async () => {
+      type Variables = {
+        id: string;
+      };
+
+      interface Data {
+        todo: {
+          id: string;
+          name: string;
+          completed: boolean;
+        };
+      }
+      const user = userEvent.setup();
+
+      const query: TypedDocumentNode<Data, Variables> = gql`
+        query TodoItemQuery($id: ID!) {
+          todo(id: $id) {
+            id
+            name
+            completed
+          }
+        }
+      `;
+
+      const mocks = [
+        {
+          request: { query, variables: { id: "1" } },
+          result: {
+            data: null,
+            errors: [new GraphQLError("Oops couldn't fetch")],
+          },
+          delay: 10,
+        },
+        {
+          request: { query, variables: { id: "1" } },
+          result: {
+            data: { todo: { id: "1", name: "Clean room", completed: true } },
+          },
+          delay: 10,
+        },
+      ];
+
+      const client = new ApolloClient({
+        link: new MockLink(mocks),
+        cache: new InMemoryCache(),
+      });
+
+      function App() {
+        return (
+          <ApolloProvider client={client}>
+            <Parent />
+          </ApolloProvider>
+        );
+      }
+
+      function SuspenseFallback() {
+        return <p>Loading</p>;
+      }
+
+      function Parent() {
+        const [queryRef, { refetch }] = useBackgroundQuery(query, {
+          variables: { id: "1" },
+        });
+
+        return (
+          <Suspense fallback={<SuspenseFallback />}>
+            <ErrorBoundary
+              onReset={() => refetch()}
+              fallbackRender={({ error, resetErrorBoundary }) => (
+                <>
+                  <button onClick={resetErrorBoundary}>Retry</button>
+                  <div>{error.message}</div>
+                </>
+              )}
+            >
+              <Todo queryRef={queryRef} />
+            </ErrorBoundary>
+          </Suspense>
+        );
+      }
+
+      function Todo({ queryRef }: { queryRef: QueryReference<Data> }) {
+        const {
+          data: { todo },
+        } = useReadQuery(queryRef);
+
+        return (
+          <div data-testid="todo">
+            {todo.name}
+            {todo.completed && " (completed)"}
+          </div>
+        );
+      }
+
+      render(<App />);
+
+      // Disable error message shown in the console due to an uncaught error.
+      // TODO: need to determine why the error message is logged to the console
+      // as an uncaught error since other tests do not require this.
+      const consoleSpy = jest
+        .spyOn(console, "error")
+        .mockImplementation(() => {});
+
+      expect(screen.getByText("Loading")).toBeInTheDocument();
+
+      expect(
+        await screen.findByText("Oops couldn't fetch")
+      ).toBeInTheDocument();
+
+      consoleSpy.mockRestore();
+
+      const button = screen.getByText("Retry");
+
+      await act(() => user.click(button));
+
+      expect(screen.getByText("Loading")).toBeInTheDocument();
+
+      await waitFor(() => {
+        expect(screen.getByTestId("todo")).toHaveTextContent(
+          "Clean room (completed)"
+        );
+      });
+    });
+
+    it("throws errors on refetch after error is encountered after first fetch with error", async () => {
+      type Variables = {
+        id: string;
+      };
+
+      interface Data {
+        todo: {
+          id: string;
+          name: string;
+          completed: boolean;
+        };
+      }
+      const user = userEvent.setup();
+
+      const query: TypedDocumentNode<Data, Variables> = gql`
+        query TodoItemQuery($id: ID!) {
+          todo(id: $id) {
+            id
+            name
+            completed
+          }
+        }
+      `;
+
+      const mocks = [
+        {
+          request: { query, variables: { id: "1" } },
+          result: {
+            data: null,
+            errors: [new GraphQLError("Oops couldn't fetch")],
+          },
+          delay: 10,
+        },
+        {
+          request: { query, variables: { id: "1" } },
+          result: {
+            data: null,
+            errors: [new GraphQLError("Oops couldn't fetch again")],
+          },
+          delay: 10,
+        },
+      ];
+
+      const client = new ApolloClient({
+        link: new MockLink(mocks),
+        cache: new InMemoryCache(),
+      });
+
+      function App() {
+        return (
+          <ApolloProvider client={client}>
+            <Parent />
+          </ApolloProvider>
+        );
+      }
+
+      function SuspenseFallback() {
+        return <p>Loading</p>;
+      }
+
+      function Parent() {
+        const [queryRef, { refetch }] = useBackgroundQuery(query, {
+          variables: { id: "1" },
+        });
+
+        return (
+          <Suspense fallback={<SuspenseFallback />}>
+            <ErrorBoundary
+              onReset={() => refetch()}
+              fallbackRender={({ error, resetErrorBoundary }) => (
+                <>
+                  <button onClick={resetErrorBoundary}>Retry</button>
+                  <div>{error.message}</div>
+                </>
+              )}
+            >
+              <Todo queryRef={queryRef} />
+            </ErrorBoundary>
+          </Suspense>
+        );
+      }
+
+      function Todo({ queryRef }: { queryRef: QueryReference<Data> }) {
+        const {
+          data: { todo },
+        } = useReadQuery(queryRef);
+
+        return (
+          <div data-testid="todo">
+            {todo.name}
+            {todo.completed && " (completed)"}
+          </div>
+        );
+      }
+
+      render(<App />);
+
+      // Disable error message shown in the console due to an uncaught error.
+      // TODO: need to determine why the error message is logged to the console
+      // as an uncaught error since other tests do not require this.
+      const consoleSpy = jest
+        .spyOn(console, "error")
+        .mockImplementation(() => {});
+
+      expect(screen.getByText("Loading")).toBeInTheDocument();
+
+      expect(
+        await screen.findByText("Oops couldn't fetch")
+      ).toBeInTheDocument();
+
+      const button = screen.getByText("Retry");
+
+      await act(() => user.click(button));
+
+      expect(screen.getByText("Loading")).toBeInTheDocument();
+
+      await waitFor(() => {
+        expect(
+          screen.getByText("Oops couldn't fetch again")
+        ).toBeInTheDocument();
+      });
+
+      expect(screen.queryByText("Loading")).not.toBeInTheDocument();
+
+      consoleSpy.mockRestore();
+    });
+
     it("`refetch` works with startTransition to allow React to show stale UI until finished suspending", async () => {
       type Variables = {
         id: string;
diff --git a/src/react/hooks/__tests__/useSuspenseQuery.test.tsx b/src/react/hooks/__tests__/useSuspenseQuery.test.tsx
--- a/src/react/hooks/__tests__/useSuspenseQuery.test.tsx
+++ b/src/react/hooks/__tests__/useSuspenseQuery.test.tsx
@@ -3506,6 +3506,7 @@ describe("useSuspenseQuery", () => {
   });
 
   it("tears down subscription when throwing an error", async () => {
+    jest.useFakeTimers();
     const consoleSpy = jest.spyOn(console, "error").mockImplementation();
 
     const { query, mocks } = useErrorCase({
@@ -3523,8 +3524,14 @@ describe("useSuspenseQuery", () => {
 
     await waitFor(() => expect(renders.errorCount).toBe(1));
 
+    // The query was never retained since the error was thrown before the
+    // useEffect coud run. We need to wait for the auto dispose timeout to kick
+    // in before we check whether the observable was cleaned up
+    jest.advanceTimersByTime(30_000);
+
     expect(client.getObservableQueries().size).toBe(0);
 
+    jest.useRealTimers();
     consoleSpy.mockRestore();
   });