From 62663954030713b5186bc0ddb1d1ba947a0c8b3e Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Sun, 11 Oct 2026 17:56:31 -0700 Subject: [PATCH 1/2] Repair lost closes in repos outside config.repos A PR closed on GitHub could stay open on the board for good when it lives in a repo that isn't in config.repos. iFixit/ops#1055 merged 2026-09-26 and still shows in PR jail on prod. The stale-open check (refreshStaleOpenPulls, run at startup and every hour) only listed config.repos, then skipped every DB-open pull whose repo wasn't listed. Org repos like ops, fixbot and ifixit-cli get their pulls from the org webhook alone, so a lost `closed` webhook there was never repaired. 21 prod rows are like this today: 17 ops, 3 ifixit-cli, 1 ifixit-schooner-fw, all closed 9/26-9/30, the stretch when prod's webhooks wrote into dev's DB. Now the check also lists the open pulls of every other repo the DB holds open pulls in (one call per 30 open pulls per repo, about 25 repos on prod) and refetches the ones GitHub no longer lists. It still adds nothing new from those repos, and a repo whose listing fails is skipped. bin/repair-stale-open-pulls fixes the existing rows without waiting for a deploy: it lists them, and with --apply refetches each. Co-Authored-By: Claude Opus 5.5 --- bin/repair-stale-open-pulls | 61 +++++++++++++++++++++++++++++++++++ lib/refresh.js | 57 ++++++++++++++++++++++++++------ test/stale-open-pulls.test.js | 38 ++++++++++++++++++++++ 3 files changed, 147 insertions(+), 9 deletions(-) create mode 100755 bin/repair-stale-open-pulls diff --git a/bin/repair-stale-open-pulls b/bin/repair-stale-open-pulls new file mode 100755 index 00000000..b75e8e5b --- /dev/null +++ b/bin/repair-stale-open-pulls @@ -0,0 +1,61 @@ +#!/usr/bin/env node +/** + * Pulls whose `closed` webhook never landed stay open in the DB. Until the + * stale-open check listed repos outside config.repos, nothing repaired them + * there: 21 PRs closed 2026-09-26 to 09-30, while prod wrote into dev's DB, + * stayed open on prod (iFixit/ops#1055 in PR jail). The hourly repair now + * catches these; this fixes them without waiting for a deploy. + * + * It lists the open pulls of every repo the DB holds open pulls in and, with + * --apply, refetches each pull GitHub no longer lists, which stores its real + * state: + * + * docker exec pulldasher bin/repair-stale-open-pulls [--apply] + * + * Without --apply it only lists. GitHub calls are paced, since the board + * shares the token's hourly quota. + */ +import db from '../lib/db.js'; +import dbManager from '../lib/db-manager.js'; +import git from '../lib/git-manager.js'; +import { createPacedRefresh, findStaleOpenPulls, findUnlistedRepos } from '../lib/refresh.js'; + +const apply = process.argv.includes('--apply'); +// at most 40 GitHub calls a minute, well under the 5000-an-hour quota +const PACE_MS = 1500; +const pause = () => new Promise(resolve => setTimeout(resolve, PACE_MS)); + +try { + const dbOpen = await dbManager.getOpenPullIds(); + const listedRepos = []; + const listedPulls = []; + for (const repo of findUnlistedRepos(dbOpen, [])) { + await pause(); + try { + listedPulls.push(...(await git.getOpenPulls(repo))); + listedRepos.push(repo); + } catch (err) { + console.error('Skipped %s (could not list): %s', repo, err.message || err); + } + } + const stale = findStaleOpenPulls(dbOpen, listedPulls, listedRepos); + console.log( + '%s of %s stored-open pulls are no longer open on GitHub', + stale.length, + dbOpen.length + ); + stale.forEach(({ repo, number }) => console.log(' %s#%s', repo, number)); + + if (apply) { + const refresh = createPacedRefresh(); + for (const { repo, number } of stale) { + await pause(); + await refresh.pull(repo, number).catch(() => (process.exitCode = 1)); + } + console.log('Refreshed %s pulls.', stale.length); + } else if (stale.length) { + console.log('Dry run: rerun with --apply to refresh them.'); + } +} finally { + db.end(); +} diff --git a/lib/refresh.js b/lib/refresh.js index a7135a8d..1aedce39 100644 --- a/lib/refresh.js +++ b/lib/refresh.js @@ -1,6 +1,7 @@ import gitManager, { observeRateLimit } from './git-manager.js'; import dbManager from './db-manager.js'; import utils from './utils.js'; +import config from './config-loader.js'; import NotifyQueue from 'notify-queue'; import debug from './debug.js'; import Promise from 'bluebird'; @@ -146,12 +147,16 @@ export function createRefresh({ pacer = noopPacer } = {}) { ) .then(result => drainThrough(pullQueue)(result).then(report => - refreshStaleOpenPulls(pullQueue, pacer, result.items, listedRepos).then( - staleFailures => ({ - failedRepos: report.failedRepos, - failedItems: report.failedItems.concat(staleFailures), - }) - ) + refreshStaleOpenPulls( + pullQueue, + pacer, + result.items, + listedRepos, + result.failedRepos + ).then(staleFailures => ({ + failedRepos: report.failedRepos, + failedItems: report.failedItems.concat(staleFailures), + })) ) ); }, @@ -191,7 +196,8 @@ export function createRefresh({ pacer = noopPacer } = {}) { pullQueue, pacer, listing.items, - listedRepos + listedRepos, + listing.failedRepos ); return { failedRepos: report.failedRepos, @@ -410,11 +416,28 @@ export function findMissingOpenPulls(dbOpenPulls, listedPulls) { return listedPulls.filter(pull => !known.has(key(pull.base.repo.full_name, pull.number))); } +/** + * The repos the DB holds open pulls in that the caller didn't try to list: org + * repos left out of `config.repos`, whose pulls arrive by webhook only. + */ +export function findUnlistedRepos(dbOpenPulls, triedRepos) { + const tried = new Set(triedRepos.map(repo => repo.toLowerCase())); + const unlisted = new Map(); + dbOpenPulls.forEach(({ repo }) => { + if (!tried.has(repo.toLowerCase()) && !unlisted.has(repo.toLowerCase())) { + unlisted.set(repo.toLowerCase(), repo); + } + }); + return [...unlisted.values()]; +} + /** * 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. + * was lost stays open in the DB until refetched. Repos outside `config.repos` get + * listed here too, or a lost close there stays open forever. Never rejects: + * app.js ignores it. */ -async function refreshStaleOpenPulls(queue, pacer, listedPulls, listedRepos) { +async function refreshStaleOpenPulls(queue, pacer, listedPulls, listedRepos, failedRepos) { let dbOpenPulls; try { dbOpenPulls = await dbManager.getOpenPullIds(); @@ -422,6 +445,22 @@ async function refreshStaleOpenPulls(queue, pacer, listedPulls, listedRepos) { console.error('Failed to load open pulls from the DB: %s', (err && err.message) || err); return []; } + const unlisted = findUnlistedRepos( + dbOpenPulls, + config.repos.map(repo => repo.name).concat(listedRepos, failedRepos) + ); + if (unlisted.length) { + listedRepos = listedRepos.slice(); + const extra = await utils.forEachRepo( + repo => + gitManager.getOpenPulls(repo, pacer).then(pulls => { + listedRepos.push(repo); + return pulls; + }), + { repos: unlisted.map(name => ({ name })) } + ); + listedPulls = listedPulls.concat(extra.items); + } const stale = findStaleOpenPulls(dbOpenPulls, listedPulls, listedRepos); refreshDebug('refreshing %s pulls open in the DB but not on GitHub', stale.length); const responses = []; diff --git a/test/stale-open-pulls.test.js b/test/stale-open-pulls.test.js index f3876120..98ebea85 100644 --- a/test/stale-open-pulls.test.js +++ b/test/stale-open-pulls.test.js @@ -84,3 +84,41 @@ test("openPulls still resolves when the DB can't list open pulls", async (t) => assert.equal(getPull.mock.callCount(), 0); assert.deepEqual(result, { failedRepos: [], failedItems: [] }); }); + +// Org repos left out of config.repos only get pulls by webhook, so a lost close +// there stayed open for good (iFixit/ops#1055). The repair lists them too, and a +// repo whose listing fails proves nothing about its pulls. +test("reconcileOpenPulls refetches stale open pulls in repos outside config.repos", async (t) => { + const listed = []; + t.mock.method(gitManager, "getOpenPulls", (repo) => { + listed.push(repo); + if (repo === "test/gone") return Promise.reject(new Error("404")); + return Promise.resolve(repo === "Test/Untracked" ? [githubPull(repo, 6)] : []); + }); + t.mock.method(dbManager, "getOpenPullIds", () => + Promise.resolve([ + { repo: "Test/Untracked", number: 5 }, + { repo: "test/untracked", number: 6 }, + { repo: "test/gone", number: 7 }, + ]) + ); + const fetched = []; + t.mock.method(gitManager, "getPull", (repo, number) => { + fetched.push(`${repo}#${number}`); + return Promise.resolve({ ...githubPull(repo, number), state: "closed" }); + }); + t.mock.method(gitManager, "parse", (response) => Promise.resolve(response)); + t.mock.method(dbManager, "updateAllPullData", () => Promise.resolve()); + + const result = await createRefresh().reconcileOpenPulls(); + + assert.deepEqual(listed.sort(), [ + "Test/Untracked", + "test/gone", + "test/repo-a", + "test/repo-b", + "test/repo-c", + ]); + assert.deepEqual(fetched, ["Test/Untracked#5"]); + assert.deepEqual(result, { failedRepos: [], failedItems: [] }); +}); From bdc918f197885fafc1acb5d42f8d97bffd149b44 Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Sun, 11 Oct 2026 18:03:38 -0700 Subject: [PATCH 2/2] Report failed listings, check the repair's saves From review: - A repo outside config.repos whose listing failed was dropped from the report, so bin/refresh-open-pulls exited 0. refreshStaleOpenPulls now resolves to { failedRepos, failedItems } and both callers add them up. - bin/repair-stale-open-pulls --apply printed "Refreshed N pulls" and exited 0 even when a save failed, since refresh.pull logs a failed parse or DB write without rejecting. It now reads the DB back, lists the pulls still open and exits 1 if there are any. A repo it can't list also exits 1. - Drop the failedRepos parameter: callers only list config.repos, so skipping config.repos already covered it. Co-Authored-By: Claude Opus 5.5 --- bin/repair-stale-open-pulls | 10 ++++++-- lib/refresh.js | 44 +++++++++++++++-------------------- test/stale-open-pulls.test.js | 4 ++-- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/bin/repair-stale-open-pulls b/bin/repair-stale-open-pulls index b75e8e5b..6d64d253 100755 --- a/bin/repair-stale-open-pulls +++ b/bin/repair-stale-open-pulls @@ -36,6 +36,7 @@ try { listedRepos.push(repo); } catch (err) { console.error('Skipped %s (could not list): %s', repo, err.message || err); + process.exitCode = 1; } } const stale = findStaleOpenPulls(dbOpen, listedPulls, listedRepos); @@ -50,9 +51,14 @@ try { const refresh = createPacedRefresh(); for (const { repo, number } of stale) { await pause(); - await refresh.pull(repo, number).catch(() => (process.exitCode = 1)); + await refresh.pull(repo, number).catch(() => {}); } - console.log('Refreshed %s pulls.', stale.length); + // refresh.pull logs a failed save without rejecting, so read the DB back. + const open = new Set((await dbManager.getOpenPullIds()).map(p => p.repo + '#' + p.number)); + const left = stale.filter(({ repo, number }) => open.has(repo + '#' + number)); + console.log('Refreshed %s pulls; %s still open in the DB.', stale.length, left.length); + left.forEach(({ repo, number }) => console.log(' %s#%s', repo, number)); + if (left.length) process.exitCode = 1; } else if (stale.length) { console.log('Dry run: rerun with --apply to refresh them.'); } diff --git a/lib/refresh.js b/lib/refresh.js index 1aedce39..38dfd446 100644 --- a/lib/refresh.js +++ b/lib/refresh.js @@ -147,16 +147,12 @@ export function createRefresh({ pacer = noopPacer } = {}) { ) .then(result => drainThrough(pullQueue)(result).then(report => - refreshStaleOpenPulls( - pullQueue, - pacer, - result.items, - listedRepos, - result.failedRepos - ).then(staleFailures => ({ - failedRepos: report.failedRepos, - failedItems: report.failedItems.concat(staleFailures), - })) + refreshStaleOpenPulls(pullQueue, pacer, result.items, listedRepos).then( + stale => ({ + failedRepos: report.failedRepos.concat(stale.failedRepos), + failedItems: report.failedItems.concat(stale.failedItems), + }) + ) ) ); }, @@ -192,16 +188,10 @@ export function createRefresh({ pacer = noopPacer } = {}) { items: missing, failedRepos: listing.failedRepos, }); - const staleFailures = await refreshStaleOpenPulls( - pullQueue, - pacer, - listing.items, - listedRepos, - listing.failedRepos - ); + const stale = await refreshStaleOpenPulls(pullQueue, pacer, listing.items, listedRepos); return { - failedRepos: report.failedRepos, - failedItems: report.failedItems.concat(staleFailures), + failedRepos: report.failedRepos.concat(stale.failedRepos), + failedItems: report.failedItems.concat(stale.failedItems), }; }, }; @@ -434,21 +424,24 @@ export function findUnlistedRepos(dbOpenPulls, triedRepos) { /** * A closed pull never appears in the open-pulls listing, so one whose close webhook * was lost stays open in the DB until refetched. Repos outside `config.repos` get - * listed here too, or a lost close there stays open forever. Never rejects: + * listed here too, or a lost close there stays open forever. Resolves to the + * repos it couldn't list and the pulls it couldn't refresh. Never rejects: * app.js ignores it. */ -async function refreshStaleOpenPulls(queue, pacer, listedPulls, listedRepos, failedRepos) { +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 []; + return { failedRepos: [], failedItems: [] }; } + // Callers only list config.repos, so every other repo gets listed here. const unlisted = findUnlistedRepos( dbOpenPulls, - config.repos.map(repo => repo.name).concat(listedRepos, failedRepos) + config.repos.map(repo => repo.name) ); + let failedRepos = []; if (unlisted.length) { listedRepos = listedRepos.slice(); const extra = await utils.forEachRepo( @@ -460,6 +453,7 @@ async function refreshStaleOpenPulls(queue, pacer, listedPulls, listedRepos, fai { repos: unlisted.map(name => ({ name })) } ); listedPulls = listedPulls.concat(extra.items); + failedRepos = extra.failedRepos; } const stale = findStaleOpenPulls(dbOpenPulls, listedPulls, listedRepos); refreshDebug('refreshing %s pulls open in the DB but not on GitHub', stale.length); @@ -480,8 +474,8 @@ async function refreshStaleOpenPulls(queue, pacer, listedPulls, listedRepos, fai } } if (responses.length === 0) { - return failedItems; + return { failedRepos, failedItems }; } const report = await drainThrough(queue)({ items: responses, failedRepos: [] }); - return failedItems.concat(report.failedItems); + return { failedRepos, failedItems: failedItems.concat(report.failedItems) }; } diff --git a/test/stale-open-pulls.test.js b/test/stale-open-pulls.test.js index 98ebea85..159d69c8 100644 --- a/test/stale-open-pulls.test.js +++ b/test/stale-open-pulls.test.js @@ -87,7 +87,7 @@ test("openPulls still resolves when the DB can't list open pulls", async (t) => // Org repos left out of config.repos only get pulls by webhook, so a lost close // there stayed open for good (iFixit/ops#1055). The repair lists them too, and a -// repo whose listing fails proves nothing about its pulls. +// repo whose listing fails proves nothing about its pulls, but is reported. test("reconcileOpenPulls refetches stale open pulls in repos outside config.repos", async (t) => { const listed = []; t.mock.method(gitManager, "getOpenPulls", (repo) => { @@ -120,5 +120,5 @@ test("reconcileOpenPulls refetches stale open pulls in repos outside config.repo "test/repo-c", ]); assert.deepEqual(fetched, ["Test/Untracked#5"]); - assert.deepEqual(result, { failedRepos: [], failedItems: [] }); + assert.deepEqual(result, { failedRepos: ["test/gone"], failedItems: [] }); });