diff --git a/lib/db-manager.js b/lib/db-manager.js index 8a81fe90..fa10d41d 100644 --- a/lib/db-manager.js +++ b/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. diff --git a/lib/refresh.js b/lib/refresh.js index 6ea243bb..e09fec5e 100644 --- a/lib/refresh.js +++ b/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); +} diff --git a/test/stale-open-pulls.test.js b/test/stale-open-pulls.test.js new file mode 100644 index 00000000..fea759a6 --- /dev/null +++ b/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: [] }); +});