fix(lgtm): keep evaluating the remaining pull requests when one evaluation fails

__tests__/bundle/cronLgtmFailureArms.test.ts

@@ -83,15 +83,35 @@ describe('dist/index.js schedule lgtm job failure arms', () => {
     ])
   })
 
-  it('fails the run when a listed pull request cannot be re-read for evaluation, and stops paging', async () => {
-    routePage([openPr(['lgtm'])])
+  it('keeps evaluating the later pull requests and pages when one cannot be re-read, and fails the run naming it', async () => {
+    gh.route('GET', new RegExp(`^${repo}/pulls\\?`), (req) => {
+      const page = new URL(req.path, gh.url).searchParams.get('page')
+      return { status: 200, body: page === '1' ? [openPr(['lgtm'])] : page === '2' ? [openPr(['lgtm'], { number: 4 })] : [] }
+    })
     gh.route('GET', `${repo}/pulls/2`, { status: 500, body: { message: 'Internal Server Error' } })
+    gh.route('GET', `${repo}/pulls/4`, { status: 200, body: { ...openPr(['lgtm'], { number: 4 }), mergeable: true, mergeable_state: 'clean' } })
+    gh.commitStatuses(repo, head, [{ context: 'prow/lgtm', state: 'success' }])
+    gh.route('PUT', `${repo}/pulls/4/merge`, { status: 200, body: { merged: true } })
 
     const result = await runCron()
 
     expect(result.status, result.stdout).toBe(1)
-    expect(result.errors.some(e => e.includes('error handling cron job: TypeError: error processing pr: HttpError: Internal Server Error'))).toBe(true)
-    expect(gh.requestsMatching('PUT', /./)).toEqual([])
-    expectRequests(configReads(), [page1, ownersProbe, `GET ${repo}/pulls/2`])
+    expect(result.errors.some(e => e.includes('could not evaluate pr #2: HttpError: Internal Server Error'))).toBe(true)
+    expect(result.errors.some(e => e.includes('error handling cron job: Error: 1 pull request(s) could not be merged: #2 (could not evaluate: HttpError: Internal Server Error)'))).toBe(true)
+    expect(result.stdout).toContain('merged pr #4')
+    const merges = gh.requestsMatching('PUT', /./)
+    expect(merges).toHaveLength(1)
+    expect(merges[0].path).toBe(`${repo}/pulls/4/merge`)
+    expectRequests(configReads(), [
+      page1,
+      ownersProbe,
+      `GET ${repo}/pulls/2`,
+      page2,
+      `GET ${repo}/pulls/4`,
+      `GET ${repo}/commits/${head}/status?per_page=100`,
+      queueRead,
+      `PUT ${repo}/pulls/4/merge`,
+      `GET ${repo}/pulls?state=open&page=3`,
+    ])
   })
 })

__tests__/cronJobTest/lgtm.test.ts

@@ -506,20 +506,46 @@ describe('cronLgtm', () => {
     expect(setFailed).not.toHaveBeenCalled()
   })
 
-  it('fails the run when evaluating a pr throws instead of returning a verdict', async () => {
+  it('keeps evaluating the other prs when evaluating one throws, and fails the run naming it', async () => {
     utils.setupJobsEnv('lgtm')
     const context = new utils.MockContext(pullReqOpenedEvent)
-    routePulls([lgtmPr(7)])
-    const merge = new utils.ObserveRequest()
+    routePulls([lgtmPr(7), lgtmPr(8)])
+    const broken = new utils.ObserveRequest()
+    const merged = observeMerge(8)
     server.use(
       http.get(`${utils.api}/repos/Codertocat/Hello-World/pulls/7`, utils.mockResponse(500, { message: 'Server Error' })),
-      http.put(`${utils.api}/repos/Codertocat/Hello-World/pulls/7/merge`, utils.mockResponse(200, null, merge)),
+      http.put(`${utils.api}/repos/Codertocat/Hello-World/pulls/7/merge`, utils.mockResponse(200, null, broken)),
     )
 
     const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {})
+    const error = vi.spyOn(core, 'error').mockImplementation(() => {})
+    await expect(handleCronJobs(context)).resolves.not.toThrow()
+    await expect(broken.notCalled()).resolves.toBe('not called')
+    await expect(merged.called()).resolves.toBe('called')
+    expect(error).toHaveBeenCalledWith(expect.stringContaining('could not evaluate pr #7: HttpError: Server Error'))
+    expect(setFailed).toHaveBeenCalledTimes(1)
+    expect(setFailed).toHaveBeenCalledWith(expect.stringContaining('1 pull request(s) could not be merged: #7 (could not evaluate: HttpError: Server Error)'))
+    expect(setFailed).not.toHaveBeenCalledWith(expect.stringContaining('#8'))
+  })
+
+  it('keeps paging after a pr whose evaluation throws, and names every pr that failed', async () => {
+    utils.setupJobsEnv('lgtm')
+    const context = new utils.MockContext(pullReqOpenedEvent)
+    const pages: Record<string, typeof listPullReqs> = { 1: [lgtmPr(7)], 2: [lgtmPr(9)] }
+    server.use(
+      http.get(`${utils.api}/repos/Codertocat/Hello-World/pulls`, ({ request }) => {
+        const page = new URL(request.url).searchParams.get('page') ?? ''
+        return utils.mockResponse(200, pages[page] ?? [])({ request })
+      }),
+      http.get(`${utils.api}/repos/Codertocat/Hello-World/pulls/7`, utils.mockResponse(500, { message: 'Server Error' })),
+      servePullsByNumber([lgtmPr(9)]),
+      http.put(`${utils.api}/repos/Codertocat/Hello-World/pulls/9/merge`, utils.mockResponse(405, { message: 'Pull Request is not mergeable' })),
+    )
+
+    const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {})
+    vi.spyOn(core, 'error').mockImplementation(() => {})
     await expect(handleCronJobs(context)).resolves.not.toThrow()
-    await expect(merge.notCalled()).resolves.toBe('not called')
     expect(setFailed).toHaveBeenCalledTimes(1)
-    expect(setFailed).toHaveBeenCalledWith(expect.stringContaining('error processing pr'))
+    expect(setFailed).toHaveBeenCalledWith(expect.stringContaining('2 pull request(s) could not be merged: #7 (could not evaluate: HttpError: Server Error), #9 (Pull Request is not mergeable)'))
   })
 })

dist/index.js

@@ -32176,7 +32176,7 @@ function setCommandEcho(enabled) {
  */
 function setFailed(message) {
     process.exitCode = ExitCode.Failure;
-    error(message);
+    core_error(message);
 }
 //-----------------------------------------------------------------------
 // Logging Commands
@@ -32199,7 +32199,7 @@ function core_debug(message) {
  * @param message error issue message. Errors will be converted to string via toString()
  * @param properties optional properties to add to the annotation.
  */
-function error(message, properties = {}) {
+function core_error(message, properties = {}) {
     command_issueCommand('error', toCommandProperties(properties), message instanceof Error ? message.toString() : message);
 }
 /**
@@ -42936,12 +42936,12 @@ async function react(octokit, context) {
     }
 }
 async function refuse(octokit, context, issueNumber, msg, cause = new Error(msg)) {
-    error(msg);
+    core_error(msg);
     try {
         await createComment(octokit, context, issueNumber, msg);
     }
     catch (commentE) {
-        error(`Could not comment with an auth error: ${commentE}`);
+        core_error(`Could not comment with an auth error: ${commentE}`);
     }
     throw cause;
 }
@@ -43374,7 +43374,7 @@ async function write(result, name, done, action) {
     }
     catch (e) {
         const message = e instanceof Error ? e.message : String(e);
-        error(`label-sync: could not sync ${name}: ${message}`);
+        core_error(`label-sync: could not sync ${name}: ${message}`);
         result.failures.push({ name, message });
     }
 }
@@ -43800,7 +43800,7 @@ async function evaluateMerge(octokit, context, number, source, lgtm = defaultLgt
     if (outcome.status === 403 && first.fork && await explainForkWorkflows(octokit, context, number, first)) {
         return skip(number, 'fork pull request with workflow changes: the token may not merge it');
     }
-    error(`could not merge pr #${number}: ${outcome.message}`);
+    core_error(`could not merge pr #${number}: ${outcome.message}`);
     return outcome;
 }
 let warnedMergeMethodIgnored = false;
@@ -43895,11 +43895,11 @@ async function evaluateInQueue(octokit, context, number, tide, first, queue) {
             return skip(number, `not ready for the merge queue: ${outcome.message}`);
         case 'forbidden': {
             const message = `cannot add pr #${number} to the merge queue: the token may not enqueue (grant contents: write and pull-requests: write, or pass a token that can — see automatic-merging.md#merge-queues): ${outcome.message}`;
-            error(message);
+            core_error(message);
             return { result: 'failed', message };
         }
         default:
-            error(`could not enqueue pr #${number}: ${outcome.message}`);
+            core_error(`could not enqueue pr #${number}: ${outcome.message}`);
             return { result: 'failed', message: outcome.message };
     }
 }
@@ -44120,7 +44120,8 @@ function pullNumber(context) {
  * through the shared merge path: the lgtm binding, GitHub's mergeability,
  * then the merge. It is the backstop of the event-driven tide handlers.
  * Every PR is attempted; once all pages are processed the run fails
- * if any merge was refused, listing the affected PRs.
+ * if any merge was refused or any evaluation threw, listing the affected PRs.
+ * Only a listing that cannot be read stops the run early.
  *
  * @param currentPage - the page to return from the github api
  * @param context - The github actions event context
@@ -44150,7 +44151,7 @@ async function cronLgtm(currentPage, context, progress = { jobsDone: 0, failures
         }
         return progress.jobsDone;
     }
-    const results = await Promise.all(prs.map(async (pr) => {
+    await Promise.all(prs.map(async (pr) => {
         info(`processing pr: ${pr.number}`);
         if (pr.state === 'closed') {
             return;
@@ -44164,14 +44165,11 @@ async function cronLgtm(currentPage, context, progress = { jobsDone: 0, failures
             }
         }
         catch (error) {
-            return error;
+            // collected like a refused merge, so one PR that cannot be evaluated does not stop the others
+            core_error(`could not evaluate pr #${pr.number}: ${error}`);
+            progress.failures.push({ number: pr.number, message: `could not evaluate: ${error}` });
         }
     }));
-    for (const result of results) {
-        if (result instanceof Error) {
-            throw new TypeError(`error processing pr: ${result}`);
-        }
-    }
     // Recurse, continue to next page
     return await cronLgtm(currentPage + 1, context, progress);
 }
@@ -45608,12 +45606,12 @@ async function assertReviewer(octokit, context, issueNumber, commenterId) {
 }
 // refuse logs and replies with msg, then fails the run with cause (or msg)
 async function lgtm_refuse(octokit, context, issueNumber, msg, cause = new Error(msg)) {
-    error(msg);
+    core_error(msg);
     try {
         await createComment(octokit, context, issueNumber, msg);
     }
     catch (commentE) {
-        error(`Could not comment with an auth error: ${commentE}`);
+        core_error(`Could not comment with an auth error: ${commentE}`);
     }
     throw cause;
 }
@@ -45756,12 +45754,12 @@ async function authorize(octokit, context, issueNumber, commenterLogin) {
 }
 // refuse logs and replies with msg, then fails the run with cause (or msg)
 async function approve_refuse(octokit, context, issueNumber, msg, cause = new Error(msg)) {
-    error(msg);
+    core_error(msg);
     try {
         await createComment(octokit, context, issueNumber, msg);
     }
     catch (commentE) {
-        error(`Could not comment with an auth error: ${commentE}`);
+        core_error(`Could not comment with an auth error: ${commentE}`);
     }
     throw cause;
 }
@@ -46918,7 +46916,7 @@ async function run() {
         const context = github_context;
         const handler = Object.hasOwn(eventHandlers, context.eventName) ? eventHandlers[context.eventName] : undefined;
         if (!handler) {
-            error(`${context.eventName} not yet supported`);
+            core_error(`${context.eventName} not yet supported`);
             return;
         }
         await handler(context);

docs/automatic-merging.md

@@ -206,8 +206,11 @@ jobs:
 Locked and closed PRs are skipped. Every eligible PR is attempted, so one un-mergeable PR does
 not stop the others. A PR GitHub reports as not mergeable is skipped with the state logged; a
 stale `lgtm` is stripped. Each refused merge is logged as an error annotation
-(`could not merge pr #<n>: <reason>`); once all pages are processed the run fails if any merge
-was refused, listing the PRs: `2 pull request(s) could not be merged: #1 (Pull Request is not mergeable), #7 (...)`.
+(`could not merge pr #<n>: <reason>`), and so is a PR whose evaluation fails (`could not evaluate
+pr #<n>: <error>`, for example a `500` reading it); the remaining PRs and pages are still
+evaluated. Once all pages are processed the run fails if any merge was refused or any evaluation
+failed, listing the PRs: `2 pull request(s) could not be merged: #1 (Pull Request is not mergeable), #7 (could not evaluate: ...)`.
+Only a listing of the open pull requests that cannot be read stops the run early.
 With event-driven merging in place an hourly or daily schedule is plenty; drop the cron entirely
 if a missed event is acceptable.
 

docs/cron-jobs.md

@@ -9,7 +9,7 @@ evaluated in this run`); event handlers never dedupe.
 
 Jobs | Description | Permissions
 --- | --- | ---
-`lgtm` | Backstop for [event-driven merging](./automatic-merging.md#event-driven-merging): catches events GitHub dropped or runs that were skipped; comment commands (`/lgtm`, `/approve`, `/unhold`) evaluate the PR themselves and do not need it. Pages through the repository's open pull requests, following pages until one comes back empty, and sends every one that passes the [merge gate](./automatic-merging.md#the-merge-gate) on its listed labels (`tide.labels` present, no `tide.missing_labels`; by default `lgtm` and none of `do-not-merge/*`, `needs-rebase`, `hold`) through the shared merge path: the [`lgtm` binding](./automatic-merging.md#lgtm-is-bound-to-a-commit) (a stale `lgtm` is stripped, not merged), then `mergeable_state` (`clean` and `has_hooks` merge). A PR that fails the gate on its listed labels costs no further call. On a branch that requires a merge queue the PR is enqueued instead (`enqueued pr #<n>`), which counts as done ([merge queues](./automatic-merging.md#merge-queues)). Skips locked and closed PRs and logs why each other PR was skipped; a refused merge fails the run listing those PRs. With the events subscribed an hourly or daily schedule is enough, or drop the job. | `contents: write`, `pull-requests: write`, `statuses: write`
+`lgtm` | Backstop for [event-driven merging](./automatic-merging.md#event-driven-merging): catches events GitHub dropped or runs that were skipped; comment commands (`/lgtm`, `/approve`, `/unhold`) evaluate the PR themselves and do not need it. Pages through the repository's open pull requests, following pages until one comes back empty, and sends every one that passes the [merge gate](./automatic-merging.md#the-merge-gate) on its listed labels (`tide.labels` present, no `tide.missing_labels`; by default `lgtm` and none of `do-not-merge/*`, `needs-rebase`, `hold`) through the shared merge path: the [`lgtm` binding](./automatic-merging.md#lgtm-is-bound-to-a-commit) (a stale `lgtm` is stripped, not merged), then `mergeable_state` (`clean` and `has_hooks` merge). A PR that fails the gate on its listed labels costs no further call. On a branch that requires a merge queue the PR is enqueued instead (`enqueued pr #<n>`), which counts as done ([merge queues](./automatic-merging.md#merge-queues)). Skips locked and closed PRs and logs why each other PR was skipped; a refused merge or a PR whose evaluation fails (`could not evaluate pr #<n>: <error>`) does not stop the others, and fails the run at the end listing those PRs. With the events subscribed an hourly or daily schedule is enough, or drop the job. | `contents: write`, `pull-requests: write`, `statuses: write`
 `sweep` | For the [`pull_request` install mode](./installing.md#without-pull_request_target): evaluates every open pull request updated within [`sweep.lookback`](#sweep) the way the `pull_request` and `pull_request_review` handlers would have, which those events cannot do for fork pull requests (read-only token). `needs-*` rules, OWNERS labels, reviewers, approval, the pending runs of PRs labeled `ok-to-test`, the merge path. | `contents: write`, `issues: write`, `pull-requests: write`, `statuses: write`, `actions: write`
 `label-sync` | Creates the labels the prow configuration describes and updates the color or description of those that drifted. Never deletes or renames a label. | `contents: read`, `issues: write`
 

src/cronJobs/lgtm.ts

@@ -42,7 +42,8 @@ interface MergePolicy {
  * through the shared merge path: the lgtm binding, GitHub's mergeability,
  * then the merge. It is the backstop of the event-driven tide handlers.
  * Every PR is attempted; once all pages are processed the run fails
- * if any merge was refused, listing the affected PRs.
+ * if any merge was refused or any evaluation threw, listing the affected PRs.
+ * Only a listing that cannot be read stops the run early.
  *
  * @param currentPage - the page to return from the github api
  * @param context - The github actions event context
@@ -81,7 +82,7 @@ export async function cronLgtm(
     return progress.jobsDone
   }
 
-  const results = await Promise.all(
+  await Promise.all(
     prs.map(async (pr) => {
       core.info(`processing pr: ${pr.number}`)
       if (pr.state === 'closed') {
@@ -98,17 +99,13 @@ export async function cronLgtm(
         }
       }
       catch (error) {
-        return error
+        // collected like a refused merge, so one PR that cannot be evaluated does not stop the others
+        core.error(`could not evaluate pr #${pr.number}: ${error}`)
+        progress.failures.push({ number: pr.number, message: `could not evaluate: ${error}` })
       }
     }),
   )
 
-  for (const result of results) {
-    if (result instanceof Error) {
-      throw new TypeError(`error processing pr: ${result}`)
-    }
-  }
-
   // Recurse, continue to next page
   return await cronLgtm(currentPage + 1, context, progress)
 }