From 70d21ab80d170fb0983d3f52b3b694110546ab00 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 03:04:43 -0700 Subject: [PATCH] fix(selfhost): heal restart-orphaned active reviews at boot, not 15-25 minutes later A container recreation kills any in-flight review pass -- the process, its provider subprocess, its locks -- but the pass's active_review_tracking row stays 'active'. While it exists, every new pass for that head skips with "AI review already in progress" (aiReviewLockContendedResult), INCLUDING a maintainer's forced re-runs from the panel checkbox. The existing reconciliation sweep only touches rows older than 15 minutes and runs on a 10-minute cron, so each deploy wedged every mid-review PR head for 15-25 minutes. Observed live on the ORB (2026-07-29, five deploys in one day): a recreate landed 3 seconds after a review pass started, and the maintainer's re-ticks bounced off the orphan until it was terminalized by hand. At boot the age heuristic is unnecessary: started_at < process boot time is EXACT -- no review survives a restart -- so the sweep can heal immediately with zero risk to a live pass. Runs right after env construction, before the server listens; fail-safe (a failure logs and boot proceeds; the cron sweep remains the backstop). --- src/db/repositories.ts | 26 +++++++++++++++++++++++ src/server.ts | 20 ++++++++++++++++++ test/unit/db-persistence.test.ts | 36 ++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index c64a802fac..ec9bed2e76 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -6560,6 +6560,32 @@ export async function listStaleActiveReviewTracking( .where(and(eq(activeReviewTracking.status, "active"), lt(activeReviewTracking.startedAt, olderThanIso))); } +/** + * Boot-time orphan sweep (#deploy-orphaned-reviews): terminalize every 'active' row whose review started + * BEFORE this process booted. No in-flight review survives a restart -- the pass, its provider subprocess, + * and its locks all died with the old process -- so any such row is definitionally an orphan, and while it + * exists every new pass for that head skips with "AI review already in progress" (aiReviewLockContendedResult). + * + * WHY NOT LEAVE IT TO runActiveReviewReconciliation: that sweep only touches rows older than + * STALE_ACTIVE_REVIEW_MIN_AGE_MS (15 min) and runs on a 10-minute cron, so a container recreation wedged + * every mid-review PR head for 15-25 minutes -- observed live on the ORB (2026-07-29): five deploys in one + * day each orphaned the in-flight review, and even the maintainer's forced re-runs bounced off the stale + * row. At boot the age heuristic is unnecessary: started_at < process boot time is EXACT, not a guess, so + * this neither waits nor risks killing a genuinely live pass (there are none yet). + * + * Returns the terminalized (repo, PR) pairs so the caller can log one line per healed row. + */ +export async function terminalizeActiveReviewsFromBeforeBoot( + env: Env, + bootIso: string, +): Promise> { + return getDb(env.DB) + .update(activeReviewTracking) + .set({ status: "terminal", updatedAt: nowIso() }) + .where(and(eq(activeReviewTracking.status, "active"), lt(activeReviewTracking.startedAt, bootIso))) + .returning({ repoFullName: activeReviewTracking.repoFullName, pullNumber: activeReviewTracking.pullNumber }); +} + // Review memory (#2178, data-model slice of #1964). Hard per-repo cap on stored suppression signals — mirrors // rag.ts's MAX_CHUNKS_PER_REPO discipline (bound a repo-controlled, unboundedly-growable store). A repo that // keeps dismissing NEW finding shapes evicts its OLDEST suppression first rather than growing forever. diff --git a/src/server.ts b/src/server.ts index 8076d6f721..8a88d8b72e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -887,6 +887,26 @@ async function main(): Promise { } as unknown as Env; markEnvReady(); + // #deploy-orphaned-reviews: heal rows this very restart orphaned, BEFORE any webhook can bounce off them. + // Fail-safe: a failure here must never block boot -- the 10-minute reconciliation sweep remains the backstop. + try { + const { terminalizeActiveReviewsFromBeforeBoot } = await import("./db/repositories"); + const healed = await terminalizeActiveReviewsFromBeforeBoot(env, new Date().toISOString()); + for (const row of healed) { + console.warn( + JSON.stringify({ + level: "warn", + event: "active_review_boot_orphan_terminalized", + repo: row.repoFullName, + pr: row.pullNumber, + }), + ); + } + } catch (error) { + console.error(JSON.stringify({ level: "error", event: "active_review_boot_sweep_failed", message: String(error).slice(0, 200) })); + } + + // Fleet-mode credential resolution (#9543): registered HERE, after `env` is fully constructed, because the // lookup needs the DB binding -- src/selfhost/ai.ts must never import the DB layer itself, so the closure is // injected instead. Safe to register after createSelfHostAi() above: the resolver is only ever invoked at diff --git a/test/unit/db-persistence.test.ts b/test/unit/db-persistence.test.ts index 90e914e3dc..bc9c370511 100644 --- a/test/unit/db-persistence.test.ts +++ b/test/unit/db-persistence.test.ts @@ -16,6 +16,7 @@ import { persistRepoGithubTotalsSnapshot, persistSignalSnapshot, startActiveReviewTracking, + terminalizeActiveReviewsFromBeforeBoot, terminalizeActiveReviewTracking, updateUpstreamDriftReportIssue, upsertBounty, @@ -600,3 +601,38 @@ describe("active-review tracking (#review-evasion-protection)", () => { }); }); }); + +describe("terminalizeActiveReviewsFromBeforeBoot (#deploy-orphaned-reviews)", () => { + it("REGRESSION: terminalizes a row orphaned by a restart, so the head is not wedged for 15-25 minutes", async () => { + // Observed live on the ORB (2026-07-29): a container recreation 3 seconds after a review pass started + // left its tracking row 'active'; every later pass for that head -- including the maintainer's forced + // re-runs -- skipped with "AI review already in progress" until the 10-minute sweep's 15-minute age + // floor finally cleared it. At boot, started_at < boot time is exact: no review survives a restart. + const env = createTestEnv(); + await startActiveReviewTracking(env, { repoFullName: "owner/repo", pullNumber: 7, headSha: "sha7", authorLogin: "alice", deliveryId: "d-7" }); + // The process "boots" strictly after the row was written. + const bootIso = new Date(Date.now() + 1000).toISOString(); + const healed = await terminalizeActiveReviewsFromBeforeBoot(env, bootIso); + expect(healed).toEqual([{ repoFullName: "owner/repo", pullNumber: 7 }]); + const row = await env.DB.prepare("select status from active_review_tracking where repo_full_name = ? and pull_number = ?").bind("owner/repo", 7).first<{ status: string }>(); + expect(row?.status).toBe("terminal"); + }); + + it("INVARIANT: never touches a review that started AFTER boot — a live pass is not an orphan", async () => { + const env = createTestEnv(); + const bootIso = new Date(Date.now() - 60_000).toISOString(); + await startActiveReviewTracking(env, { repoFullName: "owner/repo", pullNumber: 8, headSha: "sha8", authorLogin: "bob", deliveryId: "d-8" }); + expect(await terminalizeActiveReviewsFromBeforeBoot(env, bootIso)).toEqual([]); + const row = await env.DB.prepare("select status from active_review_tracking where repo_full_name = ? and pull_number = ?").bind("owner/repo", 8).first<{ status: string }>(); + expect(row?.status).toBe("active"); + }); + + it("INVARIANT: already-terminal rows are untouched (no resurrect, no double-report)", async () => { + const env = createTestEnv(); + await startActiveReviewTracking(env, { repoFullName: "owner/repo", pullNumber: 9, headSha: "sha9", authorLogin: "carol", deliveryId: "d-9" }); + const boot = new Date(Date.now() + 1000).toISOString(); + expect(await terminalizeActiveReviewsFromBeforeBoot(env, boot)).toHaveLength(1); + // Second boot sweep: nothing left to heal, and nothing re-reported. + expect(await terminalizeActiveReviewsFromBeforeBoot(env, boot)).toEqual([]); + }); +});