Fix always-false instanceof RequestError checks and dedupe retry logic (#62510)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot-Session: 9fa62c70-6b0e-40df-8a9e-88ec6807a878

Author: 3 people

src/workflows/git-utils.ts

@@ -1,9 +1,8 @@
 import crypto from 'crypto'
 import fs from 'fs/promises'
 
-import { RequestError } from '@octokit/request-error'
-
-import { retryingGithub } from './github'
+import { isRequestError, retryingGithub } from '@/workflows/github'
+import { octoSecondaryRatelimitRetry } from '@/workflows/secondary-ratelimit-retry'
 const github = retryingGithub()
 
 // https://docs.github.com/rest/reference/git#get-a-reference
@@ -31,7 +30,7 @@ export async function hasMatchingRef(owner: string, repo: string, ref: string) {
     })
     return true
   } catch (err) {
-    if (err instanceof RequestError && err.status === 404) {
+    if (isRequestError(err, 404)) {
       return false
     }
     console.log('error getting matching ref', owner, repo, ref)
@@ -220,11 +219,13 @@ async function searchCode(
   }
 
   try {
-    const { data } = await secondaryRateLimitRetry(github.rest.search.code, {
-      q,
-      per_page: perPage,
-      page: currentPage,
-    })
+    const { data } = await octoSecondaryRatelimitRetry(() =>
+      github.rest.search.code({
+        q,
+        per_page: perPage,
+        page: currentPage,
+      }),
+    )
     if (cache) {
       await fs.writeFile(tempFilename, JSON.stringify(data))
       console.log(`Wrote search results to ${tempFilename}`)
@@ -237,52 +238,6 @@ async function searchCode(
   }
 }
 
-async function secondaryRateLimitRetry<T, TArgs = Record<string, unknown>>(
-  callable: (args: TArgs) => Promise<T>,
-  args: TArgs,
-  maxAttempts = 10,
-  sleepTime = 1000,
-): Promise<T> {
-  try {
-    const response = await callable(args)
-    return response
-  } catch (err: unknown) {
-    // If you get a secondary rate limit error (403) you'll get a data
-    // response that includes:
-    //
-    //  {
-    //    documentation_url: 'https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits',
-    //    message: 'You have exceeded a secondary rate limit. Please wait a few minutes before you try again.'
-    //  }
-    //
-    // Let's look for that an manually self-recurse, under certain conditions
-    const lookFor = 'You have exceeded a secondary rate limit.'
-    if (
-      err instanceof RequestError &&
-      err.status === 403 &&
-      typeof err.response?.data === 'object' &&
-      err.response?.data !== null &&
-      'message' in err.response.data &&
-      typeof (err.response.data as Record<string, unknown>).message === 'string' &&
-      (err.response.data as Record<string, string>).message.includes(lookFor) &&
-      maxAttempts > 0
-    ) {
-      console.warn(
-        `Got secondary rate limit blocked. Sleeping for ${
-          sleepTime / 1000
-        } seconds. (attempts left: ${maxAttempts})`,
-      )
-      return new Promise((resolve) => {
-        setTimeout(() => {
-          resolve(secondaryRateLimitRetry(callable, args, maxAttempts - 1, sleepTime * 2))
-        }, sleepTime)
-      })
-    }
-
-    throw err
-  }
-}
-
 // Recursively gets the contents of a directory within a repo. Returns an
 // array of file contents. This function could be modified to return an array
 // of objects that include the path and the content of the file if needed

src/workflows/github.ts

@@ -29,3 +29,18 @@ export function retryingGithub(token?: string) {
     auth: `token ${token || apiToken}`,
   })
 }
+
+// Duck-typing instead of `instanceof RequestError` on purpose.
+// `@octokit/request` throws a RequestError built from its own nested copy of
+// `@octokit/request-error`, which is a different module instance than the
+// top-level one. The two classes are not identical, so `instanceof` always
+// returns false across that boundary.
+export function isRequestError(
+  error: unknown,
+  status?: number,
+): error is Error & { status: number } {
+  if (!(error instanceof Error) || !('status' in error)) return false
+  const errorStatus = (error as { status: unknown }).status
+  if (typeof errorStatus !== 'number') return false
+  return status === undefined || errorStatus === status
+}

src/workflows/secondary-ratelimit-retry.ts

@@ -1,3 +1,5 @@
+import { isRequestError } from '@/workflows/github'
+
 const DEFAULT_SLEEPTIME = parseInt(process.env.SECONDARY_RATELIMIT_RETRY_SLEEPTIME || '30000', 10)
 const DEFAULT_ATTEMPTS = parseInt(process.env.SECONDARY_RATELIMIT_RETRY_ATTEMPTS || '5', 10)
 
@@ -14,14 +16,8 @@ export async function octoSecondaryRatelimitRetry<T>(
     try {
       return await fn()
     } catch (error) {
-      // Use duck-typing instead of `instanceof RequestError` because octokit
-      // bundles its own copy of @octokit/request-error in dist-bundle/index.js,
-      // so the class reference differs from the top-level package and instanceof
-      // always returns false across the module boundary.
       if (
-        error instanceof Error &&
-        'status' in error &&
-        (error as { status: number }).status === 403 &&
+        isRequestError(error, 403) &&
         /You have exceeded a secondary rate limit/.test(error.message)
       ) {
         if (tries < attempts) {

src/workflows/tests/github.ts

@@ -0,0 +1,57 @@
+import { createRequire } from 'node:module'
+
+import { describe, expect, test } from 'vitest'
+import { RequestError } from '@octokit/request-error'
+
+import { isRequestError } from '@/workflows/github'
+
+// `@octokit/request` throws errors built from whichever copy of
+// `@octokit/request-error` resolves from its own location, which npm may or may
+// not hoist to the top level. Resolve it the same way Node would so this test
+// keeps working either way. When it is a separate copy, `instanceof` against the
+// top-level class fails, and that is the whole reason `isRequestError` exists.
+const requireFromRequest = createRequire(createRequire(import.meta.url).resolve('@octokit/request'))
+const nested = await import(requireFromRequest.resolve('@octokit/request-error'))
+
+function makeError(RequestErrorClass: typeof RequestError, status: number, message: string): Error {
+  return new RequestErrorClass(message, status, {
+    request: { method: 'GET', url: 'https://api.github.com/x', headers: {} },
+    response: { status, url: 'https://api.github.com/x', headers: {}, data: { message } },
+  })
+}
+
+describe('isRequestError', () => {
+  test('matches an error thrown from the copy @octokit/request uses', () => {
+    const err = makeError(nested.RequestError, 404, 'Not Found')
+    // When npm does not hoist to a single copy, this is the case a plain
+    // `instanceof RequestError` gets wrong.
+    if (nested.RequestError !== RequestError) {
+      expect(err instanceof RequestError).toBe(false)
+    }
+    expect(isRequestError(err)).toBe(true)
+    expect(isRequestError(err, 404)).toBe(true)
+  })
+
+  test('matches an error from the top-level request-error copy', () => {
+    const err = makeError(RequestError, 403, 'Forbidden')
+    expect(isRequestError(err, 403)).toBe(true)
+  })
+
+  test('respects the status argument', () => {
+    const err = makeError(nested.RequestError, 404, 'Not Found')
+    expect(isRequestError(err, 403)).toBe(false)
+  })
+
+  test('rejects non-request errors and non-errors', () => {
+    expect(isRequestError(new Error('nope'))).toBe(false)
+    expect(isRequestError({ status: 404 })).toBe(false)
+    expect(isRequestError(undefined)).toBe(false)
+    expect(isRequestError(null)).toBe(false)
+    expect(isRequestError('404')).toBe(false)
+  })
+
+  test('rejects an error whose status is not a number', () => {
+    const err = Object.assign(new Error('weird'), { status: '404' })
+    expect(isRequestError(err)).toBe(false)
+  })
+})