Fix pulls stuck open after a missed close webhook

lib/db-manager.js

@@ -392,6 +392,15 @@ const dbManager = {
          .then(filterNulls);
    },
 
+   /**
+    * Returns a promise which resolves to `{ repo, number }` for every pull the
+    * DB holds as open.
+    */
+   getOpenPullIds: function () {
+      dbDebug('Calling getOpenPullIds');
+      return db.query('SELECT repo, number FROM pulls WHERE state = ?', ['open']);
+   },
+
    /**
     * Returns a promise which resolves to a pull's number for the given head
     * commit sha.

lib/refresh.js

@@ -112,11 +112,26 @@ export function createRefresh({ pacer = noopPacer } = {}) {
 
       openPulls: function refreshOpenPulls(repos) {
          refreshDebug('refresh all open pulls');
+         const listedRepos = [];
          return utils
-            .forEachRepo(repo => gitManager.getOpenPulls(repo, pacer), {
-               repos: repos,
-            })
-            .then(drainThrough(pullQueue));
+            .forEachRepo(
+               repo =>
+                  gitManager.getOpenPulls(repo, pacer).then(pulls => {
+                     listedRepos.push(repo);
+                     return pulls;
+                  }),
+               { repos: repos }
+            )
+            .then(result =>
+               drainThrough(pullQueue)(result).then(report =>
+                  refreshStaleOpenPulls(pullQueue, pacer, result.items, listedRepos).then(
+                     staleFailures => ({
+                        failedRepos: report.failedRepos,
+                        failedItems: report.failedItems.concat(staleFailures),
+                     })
+                  )
+               )
+            );
       },
    };
 }
@@ -306,3 +321,53 @@ export function processPullItem(response, next, { parse, updateAllPullData, onFa
          next();
       });
 }
+
+/**
+ * Skips repos whose listing failed, which proves nothing about their pulls.
+ * Repo names compare case-insensitively, like GitHub's.
+ */
+export function findStaleOpenPulls(dbOpenPulls, listedPulls, listedRepos) {
+   const key = (repo, number) => repo.toLowerCase() + '#' + number;
+   const listed = new Set(listedPulls.map(pull => key(pull.base.repo.full_name, pull.number)));
+   const repos = new Set(listedRepos.map(repo => repo.toLowerCase()));
+   return dbOpenPulls.filter(
+      pull => repos.has(pull.repo.toLowerCase()) && !listed.has(key(pull.repo, pull.number))
+   );
+}
+
+/**
+ * A closed pull never appears in the open-pulls listing, so one whose close webhook
+ * was lost stays open in the DB until refetched. Never rejects: app.js ignores it.
+ */
+async function refreshStaleOpenPulls(queue, pacer, listedPulls, listedRepos) {
+   let dbOpenPulls;
+   try {
+      dbOpenPulls = await dbManager.getOpenPullIds();
+   } catch (err) {
+      console.error('Failed to load open pulls from the DB: %s', (err && err.message) || err);
+      return [];
+   }
+   const stale = findStaleOpenPulls(dbOpenPulls, listedPulls, listedRepos);
+   refreshDebug('refreshing %s pulls open in the DB but not on GitHub', stale.length);
+   const responses = [];
+   const failedItems = [];
+   for (const { repo, number } of stale) {
+      await pacer.gate();
+      try {
+         responses.push(await gitManager.getPull(repo, number));
+      } catch (err) {
+         console.error(
+            'Failed to fetch pull %s in repo %s from the GitHub API: %s',
+            number,
+            repo,
+            (err && err.message) || err
+         );
+         failedItems.push({ repo: repo, number: number });
+      }
+   }
+   if (responses.length === 0) {
+      return failedItems;
+   }
+   const report = await drainThrough(queue)({ items: responses, failedRepos: [] });
+   return failedItems.concat(report.failedItems);
+}

test/stale-open-pulls.test.js

@@ -0,0 +1,78 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import gitManager from "../lib/git-manager.js";
+import dbManager from "../lib/db-manager.js";
+import { createRefresh, findStaleOpenPulls } from "../lib/refresh.js";
+
+const githubPull = (repo, number) => ({ number, base: { repo: { full_name: repo } } });
+
+test("findStaleOpenPulls keeps DB-open pulls missing from a successful listing", () => {
+  const dbOpenPulls = [
+    { repo: "test/repo-a", number: 1 },
+    { repo: "test/repo-a", number: 2 },
+    { repo: "test/repo-b", number: 3 },
+    { repo: "Test/Repo-C", number: 4 },
+  ];
+
+  const stale = findStaleOpenPulls(
+    dbOpenPulls,
+    [githubPull("test/repo-a", 1)],
+    ["test/repo-a", "test/repo-c"]
+  );
+
+  assert.deepEqual(stale, [
+    { repo: "test/repo-a", number: 2 },
+    { repo: "Test/Repo-C", number: 4 },
+  ]);
+});
+
+// A pull whose close webhook was lost stays open in the DB. openPulls refetches it,
+// skipping repos whose listing failed and reporting a refetch that fails.
+test("openPulls refetches pulls the DB holds as open that GitHub no longer lists", async (t) => {
+  t.mock.method(gitManager, "getOpenPulls", (repo) => {
+    if (repo === "test/repo-b") return Promise.reject(new Error("transient 502"));
+    return Promise.resolve(repo === "test/repo-a" ? [githubPull(repo, 1)] : []);
+  });
+  t.mock.method(dbManager, "getOpenPullIds", () =>
+    Promise.resolve([
+      { repo: "test/repo-a", number: 1 },
+      { repo: "test/repo-a", number: 2 },
+      { repo: "test/repo-b", number: 3 },
+      { repo: "test/repo-c", number: 4 },
+    ])
+  );
+  const fetched = [];
+  t.mock.method(gitManager, "getPull", (repo, number) => {
+    fetched.push(`${repo}#${number}`);
+    if (number === 4) return Promise.reject(new Error("transient 500"));
+    return Promise.resolve({ ...githubPull(repo, number), state: "closed" });
+  });
+  const saved = [];
+  t.mock.method(gitManager, "parse", (response) => Promise.resolve(response));
+  t.mock.method(dbManager, "updateAllPullData", (pull) => {
+    saved.push(`${pull.base.repo.full_name}#${pull.number}`);
+    return Promise.resolve();
+  });
+
+  const result = await createRefresh().openPulls();
+
+  assert.deepEqual(fetched, ["test/repo-a#2", "test/repo-c#4"]);
+  assert.deepEqual(saved, ["test/repo-a#1", "test/repo-a#2"]);
+  assert.deepEqual(result, {
+    failedRepos: ["test/repo-b"],
+    failedItems: [{ repo: "test/repo-c", number: 4 }],
+  });
+});
+
+// The server calls openPulls at startup without handling its promise, so a DB
+// failure while looking for stale pulls must not reject.
+test("openPulls still resolves when the DB can't list open pulls", async (t) => {
+  t.mock.method(gitManager, "getOpenPulls", () => Promise.resolve([]));
+  t.mock.method(dbManager, "getOpenPullIds", () => Promise.reject(new Error("db down")));
+  const getPull = t.mock.method(gitManager, "getPull", () => Promise.resolve(null));
+
+  const result = await createRefresh().openPulls();
+
+  assert.equal(getPull.mock.callCount(), 0);
+  assert.deepEqual(result, { failedRepos: [], failedItems: [] });
+});