Retry requests on connection errors and additional behavior (#2230)

* wip

* Comments and clean up

* Fixed retry delay, additional no-retry codes, comments, clean up

* Catch errors fetching page metadata

* Move retry delay to the SSR prefetch query client

* Fixed retry delay on the server. Update test.

* 3 retries

* Refactor for query client

Author: jonkafton

frontends/api/src/ssr/prefetch.ts

@@ -1,16 +1,66 @@
 import { QueryClient, dehydrate } from "@tanstack/react-query"
 import type { Query } from "@tanstack/react-query"
 
+const MAX_RETRIES = 3
+const NO_RETRY_CODES = [400, 401, 403, 404, 405, 409, 422]
+
+type MaybeHasStatus = {
+  response?: {
+    status?: number
+  }
+}
+
+const getPrefetchQueryClient = () => {
+  return new QueryClient({
+    defaultOptions: {
+      queries: {
+        /**
+         * React Query's default retry logic is only active in the browser.
+         * Here we explicitly configure it to retry MAX_RETRIES times on
+         * the server, with an exclusion list of statuses that we expect not
+         * to succeed on retry.
+         *
+         * Includes status undefined as we want to retry on network errors
+         */
+        retry: (failureCount, error) => {
+          const status = (error as MaybeHasStatus)?.response?.status
+          const isNetworkError = status === undefined || status === 0
+
+          if (isNetworkError || !NO_RETRY_CODES.includes(status)) {
+            return failureCount < MAX_RETRIES
+          }
+          return false
+        },
+
+        /**
+         * By default, React Query gradually applies a backoff delay, though it is
+         * preferable that we do not significantly delay initial page renders (or
+         * indeed pages that are Statically Rendered during the build process) and
+         * instead allow the request to fail quickly so it can be subsequently
+         * fetched on the client.
+         */
+        retryDelay: 1000,
+      },
+    },
+  })
+}
+
 /* Utility to avoid repetition in server components
  * Optionally pass the queryClient returned from a previous prefetch
  * where queries are dependent on previous results
  */
 export const prefetch = async (
   queries: (Query | unknown)[],
-  queryClient?: QueryClient,
-) => {
-  queryClient = queryClient || new QueryClient()
 
+  /**
+   * The SSR QueryClient is transient - it is created only for prefetch
+   * while API requests are made to server render the page and discarded
+   * as the dehydrated state is produced and sent to the client.
+   *
+   * Create a new query client if one is not provided.
+   */
+  queryClient = getPrefetchQueryClient(),
+) => {
   await Promise.all(
     queries
       .filter(Boolean)

frontends/main/src/app/c/[channelType]/[name]/page.tsx

@@ -71,17 +71,18 @@ const Page: React.FC = async ({
   const channel = queryClient.getQueryData<UnitChannel>(
     channelQueries.detailByType(channelType, name).queryKey,
   )
-  const offerors = queryClient
-    .getQueryData<PaginatedLearningResourceOfferorDetailList>(
-      offerorQueries.list({}).queryKey,
-    )!
-    .results.reduce(
-      (memo, offeror) => ({
-        ...memo,
-        [offeror.code]: offeror,
-      }),
-      [],
-    )
+  const offerors =
+    queryClient
+      .getQueryData<PaginatedLearningResourceOfferorDetailList>(
+        offerorQueries.list({}).queryKey,
+      )
+      ?.results.reduce(
+        (memo, offeror) => ({
+          ...memo,
+          [offeror.code]: offeror,
+        }),
+        [],
+      ) ?? {}
 
   const constantSearchParams = getConstantSearchParams(channel?.search_filter)
 

frontends/main/src/app/getQueryClient.test.tsx

@@ -13,7 +13,12 @@ const getWrapper = () => {
 }
 
 test.each([
+  { status: 0, retries: 3 },
+  { status: 404, retries: 0 },
+  { status: 405, retries: 0 },
   { status: 408, retries: 3 },
+  { status: 409, retries: 0 },
+  { status: 422, retries: 0 },
   { status: 429, retries: 3 },
   { status: 502, retries: 3 },
   { status: 503, retries: 3 },
@@ -24,6 +29,7 @@ test.each([
     allowConsoleErrors()
     const wrapper = getWrapper()
     const queryFn = jest.fn().mockRejectedValue({ response: { status } })
+
     const { result } = renderHook(
       () =>
         useQuery({

frontends/main/src/app/getQueryClient.ts

@@ -8,36 +8,54 @@ type MaybeHasStatus = {
   }
 }
 
-const RETRY_STATUS_CODES = [408, 429, 502, 503, 504]
 const MAX_RETRIES = 3
-const THROW_ERROR_CODES: (number | undefined)[] = [404, 403, 401]
+const THROW_ERROR_CODES = [400, 401, 403]
+const NO_RETRY_CODES = [400, 401, 403, 404, 405, 409, 422]
 
 const makeQueryClient = (): QueryClient => {
   return new QueryClient({
     defaultOptions: {
       queries: {
         refetchOnWindowFocus: false,
         staleTime: Infinity,
-        // Throw runtime errors instead of marking query as errored.
-        // The runtime error will be caught by an error boundary.
-        // For now, only do this for 404s, 403s, and 401s. Other errors should
-        // be handled locally by components.
+
+        /**
+         * Throw runtime errors instead of marking query as errored.
+         * The runtime error will be caught by an error boundary.
+         * For now, only do this for 404s, 403s, and 401s. Other errors should
+         * be handled locally by components.
+         */
         throwOnError: (error) => {
           const status = (error as MaybeHasStatus)?.response?.status
-          return THROW_ERROR_CODES.includes(status)
+          return THROW_ERROR_CODES.includes(status ?? 0)
         },
+
         retry: (failureCount, error) => {
           const status = (error as MaybeHasStatus)?.response?.status
+          const isNetworkError = status === undefined || status === 0
+
           /**
            * React Query's default behavior is to retry all failed queries 3
-           * times. Many things (e.g., 403, 404) are not worth retrying. Let's
-           * just retry some explicit whitelist of status codes.
+           * times. Many things (e.g. 403, 404) are not worth retrying. Let's
+           * exclude these.
+           *
+           * Includes statuses undefined and 0 as we want to retry on network errors.
            */
-          if (status !== undefined && RETRY_STATUS_CODES.includes(status)) {
+          if (isNetworkError || !NO_RETRY_CODES.includes(status)) {
             return failureCount < MAX_RETRIES
           }
           return false
         },
+
+        /**
+         * By default, React Query gradually applies a backoff delay, though it is
+         * preferable that we do not significantly delay initial page renders on
+         * the server and instead allow the request to fail quickly so it can be
+         * subsequently fetched on the client. Note that we aim to prefetch any API
+         * content needed to render the page, so we don't generally expect the retry
+         * rules above to be in use on the server.
+         */
+        retryDelay: isServer ? 1000 : undefined,
       },
     },
   })

frontends/main/src/common/metadata.ts

@@ -1,6 +1,7 @@
 import { RESOURCE_DRAWER_PARAMS } from "@/common/urls"
 import { learningResourcesApi } from "api/clients"
 import type { Metadata } from "next"
+import * as Sentry from "@sentry/nextjs"
 import handleNotFound from "./handleNotFound"
 
 const DEFAULT_OG_IMAGE = "/images/learn-og-image.jpg"
@@ -32,16 +33,25 @@ export const getMetadataAsync = async ({
     RESOURCE_DRAWER_PARAMS.resource
   ]
   if (learningResourceId) {
-    const { data } = await handleNotFound(
-      learningResourcesApi.learningResourcesRetrieve({
-        id: Number(learningResourceId),
-      }),
-    )
+    try {
+      const { data } = await handleNotFound(
+        learningResourcesApi.learningResourcesRetrieve({
+          id: Number(learningResourceId),
+        }),
+      )
 
-    title = data?.title
-    description = data?.description?.replace(/<\/[^>]+(>|$)/g, "") ?? ""
-    image = data?.image?.url || image
-    imageAlt = image === data?.image?.url ? imageAlt : data?.image?.alt || ""
+      title = data?.title
+      description = data?.description?.replace(/<\/[^>]+(>|$)/g, "") ?? ""
+      image = data?.image?.url || image
+      imageAlt = image === data?.image?.url ? imageAlt : data?.image?.alt || ""
+    } catch (error) {
+      Sentry.captureException(error)
+      console.error(
+        "Error fetching learning resource for page metadata",
+        learningResourceId,
+        error,
+      )
+    }
   }
 
   return standardizeMetadata({