From af2e42786d167a1ce5d2fafc5cfae3aeef14fdc4 Mon Sep 17 00:00:00 2001 From: Ian Rohde Date: Fri, 11 Sep 2026 10:18:33 -0700 Subject: [PATCH] Fix pulls stuck open after a missed close webhook A closed PR can stay on the board forever. Pulldasher finds out a pull closed from GitHub's pull_request webhook. If that delivery is lost, the pull's row keeps state 'open' until something else refreshes that pull. Nothing does on its own. The startup refresh only lists the pulls GitHub reports as open, and a closed pull is never in that list. https://github.com/iFixit/pulldasher/blob/f924c742c208c2e52ce568b4f0a4998c46693a9c/app.js#L96-L99 https://github.com/iFixit/pulldasher/blob/f924c742c208c2e52ce568b4f0a4998c46693a9c/lib/git-manager.js#L165-L171 On 2026-09-11 the board showed 312 open pulls, and 4 of them were already closed on GitHub, all on 2026-08-28 (times in UTC): iFixit/ops#765 merged 03:38 iFixit/ops#696 merged 03:51 iFixit/DocHarvestor#242 merged 03:53 iFixit/ifixit#64052 closed 16:27 I can't read the server logs. A pulldasher container on the host started at 17:35 that day, and the next pull to close after that (iFixit/fixbot#3276, 17:43) isn't stuck. Now openPulls() ends by loading every pull the DB holds as open and refetching each one the listing left out, which saves its real state. It skips repos whose listing failed, since a failed listing says nothing about which of their pulls closed. It runs once at startup, and from bin/refresh-open-pulls, which calls the same function. When nothing is stale it adds one DB query and no GitHub calls. Solutions considered: 1. Also re-check every hour. That clears a stuck pull without waiting for a restart; we went with startup only. 2. Schedule bin/refresh-all-pulls. It lists every pull ever opened in each repo, 36,659 in ifixit alone. Note: this doesn't cover a pull that closes while the startup refresh is still working through its listing. The refresh saves the state it listed, and DBPull.save() is a plain REPLACE, so if that write comes after the close webhook's write, the pull is open again. Claude-Session: https://claude.ai/code/session_01Jd4HCBx9fmiVVyuziBg1HQ --- lib/db-manager.js | 9 ++++ lib/refresh.js | 73 ++++++++++++++++++++++++++++++-- test/stale-open-pulls.test.js | 78 +++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 4 deletions(-) create mode 100644 test/stale-open-pulls.test.js 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: [] }); +});