From c96b8b22a1a0e03c38950e461c15ee9fa6d0c2e5 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 22:30:56 -0700 Subject: [PATCH 1/6] fix(hub): send a head whose mail reached no listener again on the next pass A head whose mail found no listener, a listener still starting, or a listener the hub holds no route to is marked unrouted. The reconciler queues it again on its next pass with a listener up, without backoff, and those tries add nothing to its attempts. --- apps/hub/src/github/coalescer.ts | 4 ++-- apps/hub/src/github/reconcile-plan.test.ts | 7 ++++++ apps/hub/src/github/reconcile-plan.ts | 11 ++++++---- apps/hub/src/github/triage-queue.ts | 22 +++++++++++++++++-- apps/hub/src/github/triage-reconciler.ts | 4 ++-- packages/triage-contracts/src/triage-state.ts | 4 ++++ 6 files changed, 42 insertions(+), 10 deletions(-) diff --git a/apps/hub/src/github/coalescer.ts b/apps/hub/src/github/coalescer.ts index f5e37df6f..9f9886b29 100644 --- a/apps/hub/src/github/coalescer.ts +++ b/apps/hub/src/github/coalescer.ts @@ -22,7 +22,7 @@ import { GithubRateLimited } from "./github-usage.js"; import type { PrMail } from "./normalize.js"; import { triageStateKey, type RepoRecord } from "./tenant-config.js"; import type { OpenHeadReader } from "./tenant-open-heads.js"; -import { deliveryFailure, failHead, queueHead, restoreHead, type QueuedHead, type Remembered } from "./triage-queue.js"; +import { failDelivery, failHead, queueHead, restoreHead, type QueuedHead, type Remembered } from "./triage-queue.js"; import { TriageStateConflictError, type TriageStateStore } from "./triage-state-store.js"; import { createKeyedSerial } from "../keyed-serial.js"; @@ -165,7 +165,7 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { } catch (err) { if (err instanceof GithubRateLimited) return drop(open, entry, "github_rate_limited"); deps.log({ level: "error", msg: "forward_failed", ...entry, error: String(err) }); - await failHead(deps.store, head, deliveryFailure(err), deps.now()); + await failDelivery(deps.store, head, err, deps.now()); return; } deps.log({ level: "info", msg: "forwarded", ...entry, workflow: latest.workflow }); diff --git a/apps/hub/src/github/reconcile-plan.test.ts b/apps/hub/src/github/reconcile-plan.test.ts index f543ccd3c..9b117ae77 100644 --- a/apps/hub/src/github/reconcile-plan.test.ts +++ b/apps/hub/src/github/reconcile-plan.test.ts @@ -102,6 +102,13 @@ describe("reconcile plan", () => { expect(status(capped, 1)).toMatchObject({ status: "failed", attempts: policy.maxAttempts }); }); + test("a head whose mail reached no listener goes out again at once, without backoff or spending an attempt", () => { + const unrouted = row(1, { status: "failed", attempts: policy.maxAttempts, error: "delivery failed: run mail not routable", unrouted: true, queuedAt: at(0).toISOString() }); + const retried = plan([pr(1)], [unrouted], at(0)); + expect(enqueued(retried)).toEqual([1]); + expect(status(retried, 1)).toEqual({ ...row(1, {}), status: "queued", attempts: policy.maxAttempts, queuedAt: at(0).toISOString(), updatedAt: at(0).toISOString() }); + }); + test("a head's last attempt is decided even while the host's checks are pending", () => { const failed = (attempts: number) => row(1, { status: "failed", attempts, error: "unconfirmed checks: no-merge-conflicts", firstSeenAt: at(0).toISOString(), queuedAt: at(0).toISOString() }); expect(plan([pr(1)], [failed(policy.maxAttempts - 2)], at(41)).enqueue).toEqual([{ number: 1, reason: "unconfirmed checks: no-merge-conflicts" }]); diff --git a/apps/hub/src/github/reconcile-plan.ts b/apps/hub/src/github/reconcile-plan.ts index f301353f6..b384641fd 100644 --- a/apps/hub/src/github/reconcile-plan.ts +++ b/apps/hub/src/github/reconcile-plan.ts @@ -113,7 +113,7 @@ type Observation = { status: PrTriageRow["status"]; run?: ObservedRun; runId?: s function withStatus(row: PrTriageRow, { status, run, runId = run?.runId, error }: Observation, now: string): PrTriageRow { if (row.status === status && row.runId === runId && row.error === error) return row; // Every status observed here follows a mail that went out. - const { runId: _run, error: _error, undeliveredSince: _undelivered, ...rest } = row; + const { runId: _run, error: _error, undeliveredSince: _undelivered, unrouted: _unrouted, ...rest } = row; return { ...rest, status, @@ -206,6 +206,8 @@ function uncapped(row: PrTriageRow, now: Date, policy: ReconcilePolicy): PrTriag function isDue(row: PrTriageRow, pr: OpenPr, runs: readonly ObservedRun[], now: Date, policy: ReconcilePolicy): boolean { if (row.status === "stale") return row.queuedAt === undefined || now.getTime() - ms(row.queuedAt) >= policy.baseMoveWindowMs; if (row.status === "new") return now.getTime() - ms(pr.updatedAt) >= policy.webhookGraceMs; + // Mail that reached no listener says nothing about the head, so it goes out again on the first pass a listener is up. + if (row.status === "failed" && row.unrouted) return true; if (row.status !== "failed" || row.attempts >= policy.maxAttempts) return false; const last = row.queuedAt === undefined ? -Infinity : ms(row.queuedAt); const verdict = runs.find((run) => run.runId === row.runId); @@ -289,10 +291,11 @@ export function planTenant({ repos, runs, now, policy }: TenantPlanInput): Map { + const unrouted = reachedNoRun(err); + return replaceQueued(store, head, function failed(row) { + const at = now.toISOString(); + return { ...row, status: "failed", error: deliveryFailure(err), undeliveredSince: row.undeliveredSince ?? row.queuedAt ?? at, ...(unrouted && { unrouted: true }), updatedAt: at }; + }); +} + /** Marks a still queued head failed after its mail did not go out, so the reconciler queues it again; it keeps when its mail first did not. */ export function failHead(store: TriageStateStore, head: QueuedHead, error: string, now: Date): Promise { return replaceQueued(store, head, function failed(row) { diff --git a/apps/hub/src/github/triage-reconciler.ts b/apps/hub/src/github/triage-reconciler.ts index 77d2f56fb..b5f419f63 100644 --- a/apps/hub/src/github/triage-reconciler.ts +++ b/apps/hub/src/github/triage-reconciler.ts @@ -9,7 +9,7 @@ import { hasEnabledRules, repoPolicy, type PrTriageRow, type RepoPolicy, type Re import { mailPayload, unreadablePayload, type PolicyUnreadable } from "./bridge.js"; import type { TriageJob } from "./triage-queue.js"; import { GithubRateLimited } from "./github-usage.js"; -import { deliveryFailure, failHead } from "./triage-queue.js"; +import { failDelivery } from "./triage-queue.js"; import type { PolicyReader, ResolvedPack } from "./repo-policy.js"; import { inFlight, planTenant, requeue, runKey, type OpenPr, type QueuedPr, type ReconcilePolicy, type RepoPlan } from "./reconcile-plan.js"; import { summarizeChecks } from "@corbits/github-reads"; @@ -252,7 +252,7 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { } catch (err) { if (err instanceof GithubRateLimited) throw err; deps.log({ level: "error", msg: "forward_failed", tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha, error: String(err) }); - await failHead(deps.store, { tenantId: pass.tenantId, repo: key, number: queued.number, headSha }, deliveryFailure(err), deps.now()); + await failDelivery(deps.store, { tenantId: pass.tenantId, repo: key, number: queued.number, headSha }, err, deps.now()); continue; } deps.log({ level: "info", msg: "triage_requeued", tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha, reason: queued.reason }); diff --git a/packages/triage-contracts/src/triage-state.ts b/packages/triage-contracts/src/triage-state.ts index 97ffacb60..4f75f9b2c 100644 --- a/packages/triage-contracts/src/triage-state.ts +++ b/packages/triage-contracts/src/triage-state.ts @@ -28,6 +28,8 @@ export type PrTriageRow = { staleBefore?: string; updatedAt: string; error?: string; + /** On a failed head whose mail reached no listener: it goes out again as soon as one is up, and those tries add nothing to `attempts`. */ + unrouted?: true; }; export type TriageState = { @@ -66,6 +68,7 @@ function parseRow(raw: unknown): PrTriageRow | null { const undeliveredSince = optionalString(row.undeliveredSince); const staleBefore = optionalString(row.staleBefore); const error = optionalString(row.error); + if (row.unrouted !== undefined && row.unrouted !== true) return null; return { number, headSha, @@ -79,6 +82,7 @@ function parseRow(raw: unknown): PrTriageRow | null { ...(staleBefore !== undefined && { staleBefore }), updatedAt, ...(error !== undefined && { error }), + ...(row.unrouted === true && { unrouted: true as const }), }; } From 2fb8d2a794b81d0ff4cb97b3ad221a25bd9be2f2 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 22:36:53 -0700 Subject: [PATCH 2/6] fix(hub): mail unrouted heads within seconds of a listener running A pass or coalescer mail that left a head unrouted asks for another pass after the 15-second retry, and a roll starts a pass as soon as its listener runs, logging workflow_release_live, before it waits for the listeners it replaces to stop. --- apps/hub/src/github/coalescer.test.ts | 1 + apps/hub/src/github/coalescer.ts | 4 +++- apps/hub/src/github/reconcile-loop.ts | 5 +++-- apps/hub/src/github/triage-queue.ts | 8 +++++--- apps/hub/src/github/triage-reconciler.ts | 15 +++++++++++---- apps/hub/src/server.ts | 8 +++++++- .../src/workflow-release/rollout.postgres.test.ts | 1 + apps/hub/src/workflow-release/rollout.ts | 9 ++++++++- 8 files changed, 39 insertions(+), 12 deletions(-) diff --git a/apps/hub/src/github/coalescer.test.ts b/apps/hub/src/github/coalescer.test.ts index 9008fd318..acae8fb0a 100644 --- a/apps/hub/src/github/coalescer.test.ts +++ b/apps/hub/src/github/coalescer.test.ts @@ -97,6 +97,7 @@ function harness(rows: PrTriageRow[] = [], overrides: Partial = { async sendMail(job) { mails.push(job.payload as Record); }, + retry() {}, async openHeadFor() { return async function openHead() { return { headSha: "sha1", draft: false }; diff --git a/apps/hub/src/github/coalescer.ts b/apps/hub/src/github/coalescer.ts index 9f9886b29..82a2e1b26 100644 --- a/apps/hub/src/github/coalescer.ts +++ b/apps/hub/src/github/coalescer.ts @@ -57,6 +57,8 @@ export type Accepted = "queued" | "coalesced"; export type CoalescerDeps = { store: TriageStateStore; sendMail: (job: TriageJob) => Promise; + /** Asks the reconciler for a pass soon, after a head's mail reached no listener. */ + retry: () => void; openHeadFor: (tenantId: string) => Promise; window: CoalesceWindow; setTimer: SetTimer; @@ -165,7 +167,7 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { } catch (err) { if (err instanceof GithubRateLimited) return drop(open, entry, "github_rate_limited"); deps.log({ level: "error", msg: "forward_failed", ...entry, error: String(err) }); - await failDelivery(deps.store, head, err, deps.now()); + if (await failDelivery(deps.store, head, err, deps.now())) deps.retry(); return; } deps.log({ level: "info", msg: "forwarded", ...entry, workflow: latest.workflow }); diff --git a/apps/hub/src/github/reconcile-loop.ts b/apps/hub/src/github/reconcile-loop.ts index ea0cc1edc..e99c3d198 100644 --- a/apps/hub/src/github/reconcile-loop.ts +++ b/apps/hub/src/github/reconcile-loop.ts @@ -2,7 +2,7 @@ import type { ReconcileOutcome } from "./triage-reconciler.js"; export type ReconcileLoopOptions = { intervalMs: number; - /** First retry after a pass that could not deliver; doubles up to `retryMaxMs`. */ + /** First retry after a pass that could not deliver; doubles up to `retryMaxMs`. A pass that left a head unrouted is repeated this soon, every time. */ retryMs: number; retryMaxMs: number; schedule?: (run: () => void, ms: number) => unknown; @@ -44,9 +44,10 @@ export function createReconcileLoop( entry.scheduled = schedule(retryKick, ms); } - /** The sooner of the backoff, while a pass could not deliver, and the lift of a GitHub rate limit it ran into. */ + /** The soonest of the backoff, while a pass could not deliver, a pass left a head unrouted, and the lift of a GitHub rate limit it ran into. */ function delayOf(outcome: ReconcileOutcome): number | undefined { const delays: number[] = []; + if (outcome.unrouted) delays.push(options.retryMs); if (outcome.retry) { delays.push(retryIn); retryIn = Math.min(retryIn * 2, options.retryMaxMs); diff --git a/apps/hub/src/github/triage-queue.ts b/apps/hub/src/github/triage-queue.ts index f1e62d694..d11344fee 100644 --- a/apps/hub/src/github/triage-queue.ts +++ b/apps/hub/src/github/triage-queue.ts @@ -120,14 +120,16 @@ async function replaceQueued(store: TriageStateStore, head: QueuedHead, next: (r /** * Marks a still queued head failed after its mail did not go out, so the reconciler queues it again; it keeps when its - * mail first did not. Mail that reached no run leaves the head unrouted, so it goes out again as soon as a listener is up. + * mail first did not. Mail that reached no run leaves the head unrouted, so it goes out again as soon as a listener is + * up; true when it did. */ -export function failDelivery(store: TriageStateStore, head: QueuedHead, err: unknown, now: Date): Promise { +export async function failDelivery(store: TriageStateStore, head: QueuedHead, err: unknown, now: Date): Promise { const unrouted = reachedNoRun(err); - return replaceQueued(store, head, function failed(row) { + await replaceQueued(store, head, function failed(row) { const at = now.toISOString(); return { ...row, status: "failed", error: deliveryFailure(err), undeliveredSince: row.undeliveredSince ?? row.queuedAt ?? at, ...(unrouted && { unrouted: true }), updatedAt: at }; }); + return unrouted; } /** Marks a still queued head failed after its mail did not go out, so the reconciler queues it again; it keeps when its mail first did not. */ diff --git a/apps/hub/src/github/triage-reconciler.ts b/apps/hub/src/github/triage-reconciler.ts index b5f419f63..a53496698 100644 --- a/apps/hub/src/github/triage-reconciler.ts +++ b/apps/hub/src/github/triage-reconciler.ts @@ -58,8 +58,9 @@ type LoadedRepo = EnabledRepo & (ResolvedPack | PolicyUnreadable) & { prs: OpenP /** * `retry` asks for another pass soon: a repository's or a tenant's pass failed. * `retryAt` is when a GitHub rate limit the pass ran into lifts; no sooner pass could read anything. + * `unrouted` asks for another pass shortly: a head's mail reached no listener, which a replacement may soon be. */ -export type ReconcileOutcome = { retry: boolean; retryAt?: number }; +export type ReconcileOutcome = { retry: boolean; retryAt?: number; unrouted?: true }; function enabledRepos(config: unknown, source: RepoSource): EnabledRepo[] { return repoRecords(triageNs(config)).flatMap(function enabled(record) { @@ -111,6 +112,8 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { let limitedUntil: number | undefined; /** The limit last logged, so one limit is logged once rather than per repository per pass. */ let loggedLimit: number | undefined; + /** Whether this pass left a head unrouted. */ + let unrouted = false; /** False for a GitHub rate limit, which is noted for the pass's outcome rather than logged and retried. */ function reconcileFailed(entry: Record, err: unknown): boolean { @@ -252,11 +255,13 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { } catch (err) { if (err instanceof GithubRateLimited) throw err; deps.log({ level: "error", msg: "forward_failed", tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha, error: String(err) }); - await failDelivery(deps.store, { tenantId: pass.tenantId, repo: key, number: queued.number, headSha }, err, deps.now()); + if (await failDelivery(deps.store, { tenantId: pass.tenantId, repo: key, number: queued.number, headSha }, err, deps.now())) unrouted = true; continue; } deps.log({ level: "info", msg: "triage_requeued", tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha, reason: queued.reason }); } + // An unrouted head the in-flight limit kept back this pass. + if (claimed.rows.some((row) => row.unrouted)) unrouted = true; } /** True when the pass is worth repeating soon: a repository's pass failed. */ @@ -313,6 +318,7 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { return async function reconcileTriage(): Promise { let retry = false; limitedUntil = undefined; + unrouted = false; for (const tenant of await deps.tenants()) { try { if (await reconcileTenant(tenant)) retry = true; @@ -320,9 +326,10 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { if (reconcileFailed({ tenantId: tenant.id }, err)) retry = true; } } - if (limitedUntil === undefined) return { retry }; + const outcome: ReconcileOutcome = unrouted ? { retry, unrouted: true } : { retry }; + if (limitedUntil === undefined) return outcome; if (limitedUntil !== loggedLimit) deps.log({ level: "warn", msg: "triage_github_rate_limited", resetAt: new Date(limitedUntil).toISOString() }); loggedLimit = limitedUntil; - return { retry, retryAt: limitedUntil }; + return { ...outcome, retryAt: limitedUntil }; }; } diff --git a/apps/hub/src/server.ts b/apps/hub/src/server.ts index fe6516902..556278de8 100644 --- a/apps/hub/src/server.ts +++ b/apps/hub/src/server.ts @@ -579,6 +579,9 @@ const workflowRollout = createWorkflowRollout({ status(tenantId, runId) { return composition.workflowLifecycleService.getStatus(tenantId, runId); }, + live() { + triageLoop?.kick(); + }, lock: createWorkspaceLock(composition.db.$client), sleep: Bun.sleep, now, @@ -680,8 +683,10 @@ async function reconcileAll(): Promise { const outcomes: ReconcileOutcome[] = []; for (const reconcile of reconcilers) outcomes.push(await reconcile()); const retry = outcomes.some((outcome) => outcome.retry); + const unrouted = outcomes.some((outcome) => outcome.unrouted); const limits = outcomes.flatMap((outcome) => outcome.retryAt ?? []); - return limits.length === 0 ? { retry } : { retry, retryAt: Math.min(...limits) }; + const outcome: ReconcileOutcome = { retry, ...(unrouted && { unrouted: true }) }; + return limits.length === 0 ? outcome : { ...outcome, retryAt: Math.min(...limits) }; } triageLoop = createReconcileLoop(reconcileAll, { intervalMs: triageReconcileIntervalMs(env), retryMs: TRIAGE_RETRY_MS, retryMaxMs: TRIAGE_RETRY_MAX_MS }, logJson); const githubPrTriage = createGithubPrTriage({ @@ -755,6 +760,7 @@ function setTimer(fire: () => Promise, ms: number): () => void { coalescer = createCoalescer({ store: triageStateStore, sendMail: sendTriageMail, + retry: kickTriage, openHeadFor: tenantOpenHead, window: triageCoalesceWindow(env, reconcilePolicy), setTimer, diff --git a/apps/hub/src/workflow-release/rollout.postgres.test.ts b/apps/hub/src/workflow-release/rollout.postgres.test.ts index d6bbcefaf..b5373985f 100644 --- a/apps/hub/src/workflow-release/rollout.postgres.test.ts +++ b/apps/hub/src/workflow-release/rollout.postgres.test.ts @@ -114,6 +114,7 @@ async function workspace(name: string) { const run = fake.runs.find((row) => row.runId === runId); if (run) run.cancelling = true; }, + live() {}, async status(_tenant, runId) { if (fake.statusFails > 0) { fake.statusFails -= 1; diff --git a/apps/hub/src/workflow-release/rollout.ts b/apps/hub/src/workflow-release/rollout.ts index 69726fbad..9ea5c5375 100644 --- a/apps/hub/src/workflow-release/rollout.ts +++ b/apps/hub/src/workflow-release/rollout.ts @@ -37,6 +37,8 @@ type RolloutDeps = PublishDeps & { cancel: (tenantId: string, runId: string) => Promise; /** Stock's lifecycle status of the deployment; null once it is gone. */ status: (tenantId: string, runId: string) => Promise; + /** A host's listener runs, so mail that reached no listener can go out to it now. */ + live: (tenantId: string, source: RepoSource) => void; /** Takes the workspace's lock across hub processes; undefined while another holds it. */ lock: (tenantId: string) => Promise; sleep: (ms: number) => Promise; @@ -171,11 +173,16 @@ export function createWorkflowRollout(deps: RolloutDeps) { // deploy that listener, so it neither waits on it nor stops it. if (!redeploy && newest && runsRelease(newest, listener)) { await settle(tenantId, source, newest.runId, "workflow_release_current"); - return (await deps.state(newest.runId)) === "live"; + if ((await deps.state(newest.runId)) !== "live") return false; + deps.live(tenantId, source); + return true; } const runId = await deps.deploy({ ...publisher, assetId: listener.assetId, commitSha: listener.commitSha, packageName: chain.packageName, entry: chain.entry, offerings }); deps.log({ level: "info", msg: "workflow_release_deployed", tenantId, source, deployment: runId, assetId: listener.assetId, commitSha: listener.commitSha }); await waitUntilStarted(tenantId, runId); + // The listener a roll replaces is no longer mailed once this one runs, so mail waits only on this one starting. + deps.log({ level: "info", msg: "workflow_release_live", tenantId, source, deployment: runId }); + deps.live(tenantId, source); await settle(tenantId, source, runId, "workflow_release_rolled"); return true; } From b2db0b265e7e3a72f15510b7bcc109db28fdc9b0 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 22:52:28 -0700 Subject: [PATCH 3/6] test(chain): a pull request opened while its listener is replaced gets its verdict The scenario stops the listener through stock's stop-run route on the default five-minute pass, opens a pull request while none runs, and checks the head is mailed within 15 seconds of the replacement running and gets its verdict. docs/ARCHITECTURE.md describes unrouted heads. --- docs/ARCHITECTURE.md | 17 +++++++++- tests/chain/chain.ts | 8 +++-- tests/chain/unrouted.test.ts | 60 ++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 4 deletions(-) create mode 100644 tests/chain/unrouted.test.ts diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 7fbb3c5cb..6288d3d99 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -83,7 +83,10 @@ GitHub ──webhook──► Hub ──spawn──► Sidecar ──► listene tenant system sender, the same path cron uses, naming the host as `repoSource`. A workspace with no live listener runs nothing: the head fails saying Triage is not running on that host and that Settings → - Triage shows why. Every window is mailed when it closes, whatever the + Triage shows why. A head whose mail reached no listener, because the host + had none, the listener was still starting or the hub held no route to + it, is also marked `unrouted` and goes out again (see the reconciler). + Every window is mailed when it closes, whatever the head's CI: the pull request's manager waits for CI itself (see `triage-manager-github` below), and a window that sees a check finish reaches that manager as one more event. @@ -345,6 +348,18 @@ any other tenant or an unknown sender. on: a listener as `listener_cancel_stuck`, which every reconcile pass logs again while stock has given up on it, and a replaced workflow once as `rollout_cancel_stuck` with its name. + + Mail is addressed to a listener's run, and stock holds none for a run that + has gone, so a head whose mail reached no listener is held in its + triage-state row instead: the row fails with `unrouted`. The reconciler + queues an unrouted head on its first pass with a listener up, without + backoff, and those tries add nothing to the head's attempts. A coalescer + mail that reached no listener starts a pass at once, whose listener check + rolls a workspace left with none; a pass that left a head unrouted runs + again 15 seconds later; and a roll starts a pass as soon as its listener + runs, logging `workflow_release_live`, before it waits for the listeners it + replaces to stop. A pull request opened while its listener is replaced is + therefore mailed as soon as the new one runs. - `triage-manager-github` and `triage-manager-bitbucket-datacenter` (`triageManager`): one live run per pull request, which lives until its head has its final verdict, and the only workflow that writes to the pull request. It takes diff --git a/tests/chain/chain.ts b/tests/chain/chain.ts index 69906eebe..c494e52fd 100644 --- a/tests/chain/chain.ts +++ b/tests/chain/chain.ts @@ -29,6 +29,7 @@ export const REREAD = `${OWNER}/reread`; export const UNWOKEN = `${OWNER}/unwoken`; export const DISCONNECTED = `${OWNER}/disconnected`; export const UNSET = `${OWNER}/unset`; +export const UNROUTED = `${OWNER}/unrouted`; // Interchange's `liveWorkflowRunStatuses`: an anchor is born `deployed` and flips to `running` on its first trigger. const LIVE_DEPLOYMENT_STATUSES = new Set(["deployed", "running"]); @@ -68,8 +69,8 @@ export function pullOf(number: number, head: string): Pull { export type Claim = { status: "live" | "ended"; runId?: string; history: { head: string; runId: string; doneAt: string }[] }; -/** This test file's stack, started before its tests and stopped after them; call it once, at the file's top level. */ -export function chainStack() { +/** This test file's stack, started before its tests and stopped after them; call it once, at the file's top level. `env` overrides the hub's. */ +export function chainStack(env: Record = {}) { setDefaultTimeout(20 * 60_000); const github = fakeGithub(); const model = stubModel(decide); @@ -90,9 +91,10 @@ export function chainStack() { github.addRepo(UNWOKEN, policy([MAX_FILES])); github.addRepo(DISCONNECTED, policy([MAX_FILES])); github.addRepo(UNSET, null); + github.addRepo(UNROUTED, policy([MAX_FILES])); const [created] = await Promise.all([createDatabase(), buildPortal(portalDir)]); database = created; - hub = await startHub(database, github, portalDir); + hub = await startHub(database, github, portalDir, env); workspace = await signUp(hub, database); // Until a scenario switches it to policy, only posting runs on its own, so a Do in a policy is held. await saveAutomationMode(workspace.transport, workspace.tenantId, "ask"); diff --git a/tests/chain/unrouted.test.ts b/tests/chain/unrouted.test.ts new file mode 100644 index 000000000..c5f7e27dc --- /dev/null +++ b/tests/chain/unrouted.test.ts @@ -0,0 +1,60 @@ +// A pull request opened while its listener is being replaced. +import { expect, test } from "bun:test"; +import { TRIAGE_LISTENERS, VERDICT_LABELS } from "@corbits/triage-contracts"; +import { chainStack, UNROUTED, isLiveDeployment, sha, pullOf, comments, labelled, once } from "./chain.ts"; + +// The default pass interval, so only the hub's short retry and its pass on a listener starting can mail the head in time. +const RECONCILE_INTERVAL_MS = 5 * 60_000; +const UNROUTED_RETRY_MS = 15_000; +const REPLACED_MS = 3 * 60_000; + +const chain = chainStack({ TRIAGE_RECONCILE_INTERVAL_MS: String(RECONCILE_INTERVAL_MS) }); +const { github, pushed, until, rows } = chain; + +/** The GitHub listener's live deployments, oldest first. */ +async function listeners(): Promise { + const found = await rows(`select r.id, r.status, r.cancellation_requested_at from workflow_run r join workflow_definition d on d.id = r.definition_id left join workflow_run_launch_spec s on s.anchor_run_id = r.id where r.id = r.anchor_run_id and coalesce(s.frozen_approval_bundle -> 'projection' ->> 'id', d.name) = '${TRIAGE_LISTENERS.github}' order by r.created_at`); + return found.filter((row) => isLiveDeployment(String(row["status"])) && row["cancellation_requested_at"] === null).map((row) => String(row["id"])); +} + +function logged(msg: string): Array> { + return chain.hub.logs.filter((line) => line.includes(`"msg":"${msg}"`)).map((line) => JSON.parse(line) as Record); +} + +function msOf(entry: Record): number { + return Date.parse(String(entry["ts"])); +} + +const unrouted = once(async function unrouted() { + const [listener] = await listeners(); + if (!listener) throw new Error("The GitHub listener is not live."); + const { transport, tenantId } = chain.workspace; + // Stopped through stock's stop-run route, the listener's run is gone until the hub rolls the workspace again. + await transport.fetch("DELETE", `/api/tenants/${tenantId}/workflows/runs/${listener}`); + const head = sha("unrouted-1"); + const missed = github.misses.length; + await pushed(UNROUTED, pullOf(1, head)); + const failed = await until("the pull request's mail to reach no listener", function seen() { + return logged("forward_failed").find((entry) => entry["repo"] === UNROUTED); + }); + const live = await until("a replacement listener to run", function seen() { + return logged("workflow_release_live").find((entry) => entry["deployment"] !== listener); + }, REPLACED_MS); + const sent = await until("the pull request's mail to go out again", function seen() { + return logged("triage_requeued").find((entry) => entry["repo"] === UNROUTED && entry["pr"] === 1); + }); + await until("the verdict", function posted() { + return comments(github.writesTo(UNROUTED, 1)).length > 0 ? true : undefined; + }); + return { listener, failed, live, sent, writes: github.writesTo(UNROUTED, 1), misses: github.misses.slice(missed) }; +}); + +test("a pull request opened while its listener is replaced is mailed to the replacement within the short retry of it running, and gets its verdict", async () => { + const { listener, failed, live, sent, writes, misses } = await unrouted(); + expect(msOf(failed)).toBeLessThan(msOf(live)); + expect(live["deployment"]).not.toBe(listener); + expect(msOf(sent) - msOf(live)).toBeLessThanOrEqual(UNROUTED_RETRY_MS); + expect(misses).toEqual([]); + expect(comments(writes)).toEqual([expect.stringContaining("**Triage: Pass**")]); + expect(labelled(writes)).toEqual([VERDICT_LABELS.pass]); +}); From c829c5daab3436e6bcd78a9690f00445474c0fcd Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 28 Oct 2026 00:35:00 -0700 Subject: [PATCH 4/6] fix(hub): hold only mail the listener took none of, record nothing for it, and bound its short retry Mail a live manager did not take fails as usual. The intake checks stock's router before recording an event for a pull request with no manager up, and a run Interchange refuses mail to as stopping, ended or gone is held like an unroutable one. A head gets the 15-second retry for at most 20 passes in a row, then logs triage_unrouted_waiting once and waits for the ordinary pass until a listener takes its mail or a roll finds its listener running. Only a pass that tried and failed asks for the short retry, a base move clears the flag, and unrouted: false parses as absent. --- apps/hub/src/github/coalescer.ts | 5 +- apps/hub/src/github/deployment.ts | 23 ++++++- apps/hub/src/github/held-dos.ts | 4 +- .../src/github/interleave.postgres.test.ts | 3 +- apps/hub/src/github/triage-queue.ts | 17 +++--- apps/hub/src/github/triage-reconciler.test.ts | 20 +++++- apps/hub/src/github/triage-reconciler.ts | 61 +++++++++++++++++-- apps/hub/src/manager-intake.postgres.test.ts | 12 +++- apps/hub/src/manager-intake.ts | 28 ++++++++- apps/hub/src/server.ts | 21 ++++++- docs/ARCHITECTURE.md | 30 ++++++--- packages/triage-contracts/src/triage-state.ts | 1 - 12 files changed, 186 insertions(+), 39 deletions(-) diff --git a/apps/hub/src/github/coalescer.ts b/apps/hub/src/github/coalescer.ts index 82a2e1b26..386216c0f 100644 --- a/apps/hub/src/github/coalescer.ts +++ b/apps/hub/src/github/coalescer.ts @@ -166,8 +166,9 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { await deps.sendMail({ ...head, record: latest.record, payload }); } catch (err) { if (err instanceof GithubRateLimited) return drop(open, entry, "github_rate_limited"); - deps.log({ level: "error", msg: "forward_failed", ...entry, error: String(err) }); - if (await failDelivery(deps.store, head, err, deps.now())) deps.retry(); + const unrouted = await failDelivery(deps.store, head, err, deps.now()); + deps.log({ level: unrouted ? "info" : "error", msg: "forward_failed", ...entry, error: String(err) }); + if (unrouted) deps.retry(); return; } deps.log({ level: "info", msg: "forwarded", ...entry, workflow: latest.workflow }); diff --git a/apps/hub/src/github/deployment.ts b/apps/hub/src/github/deployment.ts index f2f37ab2d..641626356 100644 --- a/apps/hub/src/github/deployment.ts +++ b/apps/hub/src/github/deployment.ts @@ -27,16 +27,37 @@ export class DeploymentNotReadyError extends Error { } } +/** Interchange refused mail to a run that is stopping, ended or gone, so none was sent. */ +export class RunUnavailableError extends Error {} + +/** The host's listener took no mail: it was still starting, stopping or gone, or the hub held no route to it. Nothing was sent. */ +export class ListenerUnroutedError extends NoLiveDeploymentError { + constructor(source: RepoSource, address: string, cause?: unknown) { + super(TRIAGE_LISTENERS[source]); + this.message = cause === undefined ? `listener ${address} is not routable` : `listener ${address} took no mail: ${String(cause)}`; + this.cause = cause; + } +} + +/** Run states Interchange refuses mail in. */ +const UNAVAILABLE_RUN_CODES = new Set(["workflow_run_stopping", "workflow_run_terminal"]); + /** Interchange's sender gate refused the mail; a worker copied from a listener deployed before the gate keeps refusing until the listener is redeployed. */ export class SenderRefusedError extends Error { readonly code = MAIL_ADMISSION_DENIED_CODE; } -/** @corbits/webhooks 0.3.0 ignores `notReady` and routes the mail without grants; this fails it so the caller retries. Drop once the package handles it. */ +/** + * @corbits/webhooks 0.3.0 ignores `notReady` and routes the mail without grants; this fails it so the caller retries. + * Drop once the package handles it. A run Interchange refuses mail to as stopping, ended or gone fails as + * `RunUnavailableError`, since the package reports it only as a plain error. + */ export function readyMaterializer(materialize: RunTriggerMaterialize): RunTriggerMaterialize { return async function materializeReady(args) { const grants = await materialize(args); if (grants.outcome === "notReady") throw new DeploymentNotReadyError(); + if (grants.outcome === "skip") throw new RunUnavailableError(`${args.agentAddress} is not a live deployment`); + if (grants.outcome === "rejected" && UNAVAILABLE_RUN_CODES.has(grants.code)) throw new RunUnavailableError(grants.message); return grants; }; } diff --git a/apps/hub/src/github/held-dos.ts b/apps/hub/src/github/held-dos.ts index 32743a279..183f347b5 100644 --- a/apps/hub/src/github/held-dos.ts +++ b/apps/hub/src/github/held-dos.ts @@ -16,7 +16,7 @@ import { doRefused, failure, type PortalCredentialDeps } from "./portal-credenti import type { Member } from "../integrations/app.js"; import type { PullRequestRef } from "../manager-state.js"; import type { VerdictRecordStore } from "./verdict-record-store.js"; -import { DeploymentNotReadyError } from "./deployment.js"; +import { DeploymentNotReadyError, ListenerUnroutedError } from "./deployment.js"; import { repoRecords, sourceOf, triageNs, type RepoRecord } from "./tenant-config.js"; import { verdictFor } from "./triage-runs.js"; @@ -141,7 +141,7 @@ export function createGithubHeldDos(deps: GithubHeldDosDeps) { } catch (err) { deps.log({ level: "error", msg: "held_do_approval_failed", ...logged, error: String(err) }); if (err instanceof DeploymentNotReadyError) return failure(409, err.code, err.message); - const unroutable = isRunTriggerUnroutable(err); + const unroutable = err instanceof ListenerUnroutedError || isRunTriggerUnroutable(err); return failure(unroutable ? 503 : 502, unroutable ? "service_not_running" : "hub_unavailable", `Could not send the approval: ${errorText(err)}. Try again.`); } deps.log({ level: "info", msg: "held_do_approved", ...logged }); diff --git a/apps/hub/src/github/interleave.postgres.test.ts b/apps/hub/src/github/interleave.postgres.test.ts index 13a689fdf..bb2eb3a36 100644 --- a/apps/hub/src/github/interleave.postgres.test.ts +++ b/apps/hub/src/github/interleave.postgres.test.ts @@ -9,7 +9,7 @@ import type { CheckRun } from "@corbits/github-reads"; import { emptyPack, type CheckPack, type PrTriageRow } from "@corbits/triage-contracts"; import { createGithubPrTriage } from "./pr-triage.js"; import { DEFAULT_RECONCILE_POLICY } from "./reconcile-plan.js"; -import { createTriageReconciler } from "./triage-reconciler.js"; +import { createTriageReconciler, createUnroutedTries } from "./triage-reconciler.js"; import { createTriageStateStore, type TriageStateStore } from "./triage-state-store.js"; import type { ObservedRuns } from "./manager-runs.js"; @@ -72,6 +72,7 @@ function reconciler(store: TriageStateStore, heads: number[], deliver: Deliver, store, readCheckPack: async () => ({ status: "ok", pack, automatic: [], source: { file: { path: ".corbits-triage.yml", sha: "art_pack-1" } } }), send: async ({ payload }) => deliver(payload), + unroutedTries: createUnroutedTries(), policy: DEFAULT_RECONCILE_POLICY, now: () => NOW, log: () => {}, diff --git a/apps/hub/src/github/triage-queue.ts b/apps/hub/src/github/triage-queue.ts index d11344fee..98904625c 100644 --- a/apps/hub/src/github/triage-queue.ts +++ b/apps/hub/src/github/triage-queue.ts @@ -3,9 +3,8 @@ // mail does not go out. Shared by the manual-run route and the webhook // coalescer. import { setTimeout as sleep } from "node:timers/promises"; -import { isRunTriggerUnroutable } from "@corbits/webhooks"; import { triageStopped, type PrTriageRow, type RepoSource } from "@corbits/triage-contracts"; -import { DeploymentNotReadyError, TriageStoppedError } from "./deployment.js"; +import { ListenerUnroutedError, TriageStoppedError } from "./deployment.js"; import { BASE_MOVED, type ReconcilePolicy } from "./reconcile-plan.js"; import type { RepoRecord } from "./tenant-config.js"; import { TriageStateConflictError, type TriageStateStore, type TriageStateVersion } from "./triage-state-store.js"; @@ -22,9 +21,9 @@ export function undelivered(row: PrTriageRow, source: RepoSource): boolean { return row.status === "failed" && row.error !== undefined && (row.error === triageStopped(source) || row.error.startsWith(DELIVERY_FAILED)); } -/** The mail reached no run: the host had no listener, the listener was still starting, or the hub held no route to it. */ -function reachedNoRun(err: unknown): boolean { - return err instanceof TriageStoppedError || err instanceof DeploymentNotReadyError || isRunTriggerUnroutable(err); +/** The mail reached no listener: the host had none, or its listener took no mail. A manager that took none is an ordinary failure. */ +function reachedNoListener(err: unknown): boolean { + return err instanceof TriageStoppedError || err instanceof ListenerUnroutedError; } /** A pull request mail for the host's listener. */ @@ -124,7 +123,7 @@ async function replaceQueued(store: TriageStateStore, head: QueuedHead, next: (r * up; true when it did. */ export async function failDelivery(store: TriageStateStore, head: QueuedHead, err: unknown, now: Date): Promise { - const unrouted = reachedNoRun(err); + const unrouted = reachedNoListener(err); await replaceQueued(store, head, function failed(row) { const at = now.toISOString(); return { ...row, status: "failed", error: deliveryFailure(err), undeliveredSince: row.undeliveredSince ?? row.queuedAt ?? at, ...(unrouted && { unrouted: true }), updatedAt: at }; @@ -163,8 +162,10 @@ export async function markBaseMoved(store: TriageStateStore, tenantId: string, r if (state.rows.every((row) => row.number === except)) return 0; const rows = state.rows.map(function moved(row): PrTriageRow { if (row.number === except) return row; - if (!staled(row)) return { ...row, staleBefore: at, updatedAt: at }; - return { ...row, status: "stale", attempts: 0, error: BASE_MOVED, staleBefore: at, updatedAt: at }; + // The base move decides when each head goes out again, one held for a listener included. + const { unrouted: _unrouted, ...rest } = row; + if (!staled(row)) return { ...rest, staleBefore: at, updatedAt: at }; + return { ...rest, status: "stale", attempts: 0, error: BASE_MOVED, staleBefore: at, updatedAt: at }; }); try { await store.save(tenantId, repo, rows, state.version); diff --git a/apps/hub/src/github/triage-reconciler.test.ts b/apps/hub/src/github/triage-reconciler.test.ts index 79c83df90..231684dcb 100644 --- a/apps/hub/src/github/triage-reconciler.test.ts +++ b/apps/hub/src/github/triage-reconciler.test.ts @@ -6,9 +6,9 @@ import { unsettledBy } from "@corbits/triage-workflows/logic/settled"; import { DEFAULT_RECONCILE_POLICY, planTenant, runKey, type ObservedRun, type OpenPr } from "./reconcile-plan.js"; import { triageReconcilePolicy } from "../env.js"; import { TriageStateConflictError, type TriageStateStore } from "./triage-state-store.js"; -import type { LiveDeployment } from "./deployment.js"; +import { ListenerUnroutedError, type LiveDeployment } from "./deployment.js"; import type { HostedVerdict } from "./verdict-record-store.js"; -import { createTriageReconciler, type ReconcileOutcome } from "./triage-reconciler.js"; +import { createTriageReconciler, createUnroutedTries, type ReconcileOutcome } from "./triage-reconciler.js"; import { createReconcileLoop } from "./reconcile-loop.js"; import { createRepoActivity, type RepoActivity } from "./repo-activity.js"; import { GithubRateLimited } from "./github-usage.js"; @@ -116,6 +116,7 @@ function harness(runs: Runs | (() => Runs), prs: OpenPr[] | Record> = []; const listings: string[] = []; const now = options.now ?? (() => NOW); + const unroutedTries = createUnroutedTries(); const reconcile = createTriageReconciler({ source: "github", tenants: async function tenants() { @@ -152,6 +153,7 @@ function harness(runs: Runs | (() => Runs), prs: OpenPr[] | Record }); }, + unroutedTries, policy: triageReconcilePolicy({ ...(options.maxInFlight !== undefined && { TRIAGE_MAX_IN_FLIGHT: options.maxInFlight }) }), now, log: (entry) => logged.push(entry), @@ -172,7 +174,7 @@ function harness(runs: Runs | (() => Runs), prs: OpenPr[] | Record { @@ -480,6 +482,18 @@ describe("triage reconciler", () => { expect(saved.get(REPO)?.find((row) => row.number === 1)).toMatchObject({ status: "failed", error: "delivery failed: Error: 502 Bad Gateway" }); }); + test("a head whose mail reaches no listener asks for the short retry for 19 passes in a row, then logs once and waits for the ordinary pass until its listener runs", async () => { + const sendErrors: Record = { 1: new ListenerUnroutedError("github", formatRunAddress("run_gone", DOMAIN)) }; + const { reconcile, saved, logged, unroutedTries } = harness({}, heads(1, 1), { sendErrors }); + const outcomes: ReconcileOutcome[] = []; + for (let pass = 0; pass < 21; pass += 1) outcomes.push(await reconcile()); + expect(outcomes.map((outcome) => outcome.unrouted === true)).toEqual([...Array(19).fill(true), false, false]); + expect(logged.filter((entry) => entry["msg"] === "triage_unrouted_waiting")).toEqual([{ level: "warn", msg: "triage_unrouted_waiting", tenantId: TENANT_ID, repo: REPO, pr: 1, headSha: "sha1", tries: 20 }]); + expect(saved.get(REPO)?.find((row) => row.number === 1)).toMatchObject({ status: "failed", unrouted: true, attempts: 1 }); + unroutedTries.reset(TENANT_ID, "github"); + expect((await reconcile()).unrouted).toBe(true); + }); + test("queued heads that have not started hold the budget until their runs complete", async () => { const logs: Runs = { [REPO]: {} }; const { reconcile, delivered, saved } = harness(logs, heads(1, 12)); diff --git a/apps/hub/src/github/triage-reconciler.ts b/apps/hub/src/github/triage-reconciler.ts index a53496698..2fbdf863d 100644 --- a/apps/hub/src/github/triage-reconciler.ts +++ b/apps/hub/src/github/triage-reconciler.ts @@ -23,6 +23,46 @@ import { TriageStateConflictError, type LoadedTriageState, type TriageStateStore export type ReconcileTenant = { id: string; domain: string; config: unknown }; +/** Passes in a row, about five minutes of short retries, that a head whose mail reached no listener is retried after the short retry. */ +const UNROUTED_SHORT_TRIES = 20; + +/** + * How many passes in a row each head's mail reached no listener, in memory: a restart starts the short retries over. + * A head past `UNROUTED_SHORT_TRIES` waits for the ordinary pass until a listener takes its mail or a roll's listener runs. + */ +export function createUnroutedTries() { + const tries = new Map(); + + function hostOf(tenantId: string, source: RepoSource): string { + return `${tenantId}\n${source}\n`; + } + + function keyOf(tenantId: string, source: RepoSource, repo: string, number: number, headSha: string): string { + return `${hostOf(tenantId, source)}${repo}\n${runKey(number, headSha)}`; + } + + return { + /** Counts a pass whose mail for the head reached no listener; how many in a row. */ + missed(tenantId: string, source: RepoSource, repo: string, number: number, headSha: string): number { + const key = keyOf(tenantId, source, repo, number, headSha); + const count = (tries.get(key) ?? 0) + 1; + tries.set(key, count); + return count; + }, + /** A listener took the head's mail. */ + took(tenantId: string, source: RepoSource, repo: string, number: number, headSha: string): void { + tries.delete(keyOf(tenantId, source, repo, number, headSha)); + }, + /** The host's listener runs, so each of its heads gets its short retries again. */ + reset(tenantId: string, source: RepoSource): void { + const host = hostOf(tenantId, source); + for (const key of tries.keys()) if (key.startsWith(host)) tries.delete(key); + }, + }; +} + +export type UnroutedTries = ReturnType; + export type TriageReconcilerDeps = { /** The host whose repositories this reconciler brings to a triaged head. */ source: RepoSource; @@ -46,6 +86,7 @@ export type TriageReconcilerDeps = { store: TriageStateStore; readCheckPack: PolicyReader; send: (job: TriageJob) => Promise; + unroutedTries: UnroutedTries; policy: ReconcilePolicy; now: () => Date; log: (entry: Record) => void; @@ -112,7 +153,7 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { let limitedUntil: number | undefined; /** The limit last logged, so one limit is logged once rather than per repository per pass. */ let loggedLimit: number | undefined; - /** Whether this pass left a head unrouted. */ + /** Whether a head this pass mailed reached no listener and keeps its short retry. */ let unrouted = false; /** False for a GitHub rate limit, which is noted for the pass's outcome rather than logged and retried. */ @@ -250,18 +291,26 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { const headSha = headShaOf(queued); const mailed = { items: [{ prNumber: queued.number, headSha }], ...(queued.noWait && { noWait: true }) }; const payload = "pack" in repo ? mailPayload(repo.record, repo.policy, repo, mailed) : unreadablePayload(repo.record, repo.policy, repo, mailed); + const entry = { tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha }; try { await deps.send({ tenantId: pass.tenantId, repo: key, number: queued.number, headSha, record: repo.record, payload }); } catch (err) { if (err instanceof GithubRateLimited) throw err; - deps.log({ level: "error", msg: "forward_failed", tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha, error: String(err) }); - if (await failDelivery(deps.store, { tenantId: pass.tenantId, repo: key, number: queued.number, headSha }, err, deps.now())) unrouted = true; + const missed = await failDelivery(deps.store, { tenantId: pass.tenantId, repo: key, number: queued.number, headSha }, err, deps.now()); + deps.log({ level: missed ? "info" : "error", msg: "forward_failed", ...entry, error: String(err) }); + if (missed) noteUnrouted(entry); continue; } - deps.log({ level: "info", msg: "triage_requeued", tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha, reason: queued.reason }); + deps.unroutedTries.took(pass.tenantId, deps.source, repo.record.name, queued.number, headSha); + deps.log({ level: "info", msg: "triage_requeued", ...entry, reason: queued.reason }); } - // An unrouted head the in-flight limit kept back this pass. - if (claimed.rows.some((row) => row.unrouted)) unrouted = true; + } + + /** Asks for the short retry while the head has tries left, and logs once when it falls back to the ordinary pass. */ + function noteUnrouted(entry: { tenantId: string; repo: string; pr: number; headSha: string }): void { + const tries = deps.unroutedTries.missed(entry.tenantId, deps.source, entry.repo, entry.pr, entry.headSha); + if (tries < UNROUTED_SHORT_TRIES) unrouted = true; + if (tries === UNROUTED_SHORT_TRIES) deps.log({ level: "warn", msg: "triage_unrouted_waiting", ...entry, tries }); } /** True when the pass is worth repeating soon: a repository's pass failed. */ diff --git a/apps/hub/src/manager-intake.postgres.test.ts b/apps/hub/src/manager-intake.postgres.test.ts index bb22abf7c..527d7b073 100644 --- a/apps/hub/src/manager-intake.postgres.test.ts +++ b/apps/hub/src/manager-intake.postgres.test.ts @@ -8,6 +8,7 @@ import type { PullRequestToReview } from "./pull-request-reader.js"; import { mailPayload } from "./github/bridge.js"; import { createPauseGate } from "./github/pr-workflow.js"; import { createManagerIntake, createManagerWake, createOverdueWake, createSupersede } from "./manager-intake.js"; +import { ListenerUnroutedError } from "./github/deployment.js"; import { recordEvent } from "./manager-state.js"; import { migrateTriageTables } from "./triage-db.js"; @@ -48,7 +49,7 @@ async function passed(_tenantId: string, _record: unknown, _number: number, requ const OPEN = [{ number: 7, headSha: "h1", draft: false }, { number: 9, headSha: "h9", draft: true }]; -function stack(db: Db, listener: string | undefined, pull = PULL, gate: Gate = passed, open = OPEN) { +function stack(db: Db, listener: string | undefined, pull = PULL, gate: Gate = passed, open = OPEN, routable = true) { const mailed: { address: string; payload: unknown }[] = []; const logged: Record[] = []; const cancelled: { runId: string; reason: string }[] = []; @@ -64,6 +65,7 @@ function stack(db: Db, listener: string | undefined, pull = PULL, gate: Gate = p const toListener = createManagerIntake({ db, listener: async () => listener, + routable: async () => routable, readPull: async () => pull, openHeads: async () => open, withAppLogin: async (_tenantId, payload) => ({ ...payload, appLogin: "triage[bot]" }), @@ -94,6 +96,14 @@ test("PostgreSQL: a workspace without a listener records nothing and mails nothi }); }, 60_000); +test("PostgreSQL: mail for a pull request with no manager up, while the hub holds no route to the listener, records and mails nothing", async () => { + await withDatabase(async function unrouted(db) { + const { toListener, mailed } = stack(db, LISTENER, PULL, passed, OPEN, false); + await expect(toListener(TENANT, requeued(7, "h1"))).rejects.toBeInstanceOf(ListenerUnroutedError); + expect({ events: (await events(db)).length, mailed }).toEqual({ events: 0, mailed: [] }); + }); +}, 60_000); + test("PostgreSQL: each pull request a mail names is recorded as its own event, a webhook once however often it is delivered and any other mail every time, and the listener is mailed the heads", async () => { await withDatabase(async function listener(db) { const { toListener, mailed } = stack(db, LISTENER); diff --git a/apps/hub/src/manager-intake.ts b/apps/hub/src/manager-intake.ts index a438c5fe1..59d6746b9 100644 --- a/apps/hub/src/manager-intake.ts +++ b/apps/hub/src/manager-intake.ts @@ -5,9 +5,11 @@ import { randomUUID } from "node:crypto"; import { eq } from "drizzle-orm"; import { schema, type DB } from "@intx/db"; +import { isRunTriggerUnroutable } from "@corbits/webhooks"; import { asRecord } from "@corbits/common"; import { APPROVE_DO_KIND, repoSourceOf, type RepoSource } from "@corbits/triage-contracts"; import { reviewRequestOf, type PullRequestToReview } from "./pull-request-reader.js"; +import { DeploymentNotReadyError, ListenerUnroutedError, RunUnavailableError } from "./github/deployment.js"; import { repoRecords, sourceOf, triageNs, type RepoRecord } from "./github/tenant-config.js"; import { dropReviewer, liveManager, overduePulls, recordEvent, recordOverdueWake, reviewedPulls, superseded, type PullRequestRef } from "./manager-state.js"; @@ -15,6 +17,8 @@ export type ManagerIntakeDeps = { db: DB["db"]; /** The address of the workspace's live listener for the host, if it has one. */ listener: (tenantId: string, source: RepoSource) => Promise; + /** Whether the hub routes mail to the listener at this address now. */ + routable: (address: string) => Promise; /** The pull request as its host shows it now, with its diff. */ readPull: (tenantId: string, record: RepoRecord, number: number) => Promise; /** The repository's open pull requests and their heads, which a backlog mail stands for. */ @@ -70,9 +74,19 @@ async function mailedLive(deps: Delivery, tenantId: string, domain: string, pr: return true; } -/** Mails the listener the heads of `repo` it deploys a manager for. */ +/** The listener took no mail, so none was sent. */ +function reachedNoListener(err: unknown): boolean { + return isRunTriggerUnroutable(err) || err instanceof DeploymentNotReadyError || err instanceof RunUnavailableError; +} + +/** Mails the listener the heads of `repo` it deploys a manager for; mail the listener took none of fails as `ListenerUnroutedError`. */ async function deployFor(deps: Delivery, tenantId: string, address: string, source: RepoSource, repo: string, heads: readonly { number: number; head: string }[]): Promise { - await deps.deliver(tenantId, address, { kind: "pr", repo, ...(source !== "github" && { repoSource: source }), tenantId, items: heads.map((pull) => ({ prNumber: pull.number, headSha: pull.head })) }); + try { + await deps.deliver(tenantId, address, { kind: "pr", repo, ...(source !== "github" && { repoSource: source }), tenantId, items: heads.map((pull) => ({ prNumber: pull.number, headSha: pull.head })) }); + } catch (err) { + if (reachedNoListener(err)) throw new ListenerUnroutedError(source, address, err); + throw err; + } } /** @@ -180,6 +194,14 @@ export function createManagerIntake(deps: ManagerIntakeDeps) { return Array.isArray(items) ? itemsOf(request, items) : named(request, prNumber, headSha); } + /** Whether any of the pull requests has no manager up. */ + async function withoutManager(tenantId: string, source: RepoSource, repo: string, pulls: readonly Named[]): Promise { + for (const pull of pulls) { + if (!(await liveManager(deps.db, tenantId, { source, repo, number: pull.number }))) return true; + } + return false; + } + /** The head's review request when the policy turns code review on; a head that moved gets its own at its own intake. */ async function withReviewRequest(tenantId: string, record: RepoRecord, pull: Named): Promise> { const { request } = pull; @@ -205,6 +227,8 @@ export function createManagerIntake(deps: ManagerIntakeDeps) { if (!address || typeof repo !== "string") return false; const record = await recordOf(tenantId, repo, source); const pulls = await pullsOf(tenantId, record, request); + // A pull request with no manager up is deployed by the listener, so mail the listener cannot take records nothing. + if (!(await deps.routable(address)) && (await withoutManager(tenantId, source, repo, pulls))) throw new ListenerUnroutedError(source, address); const domain = domainOf(address); const start: Named[] = []; try { diff --git a/apps/hub/src/server.ts b/apps/hub/src/server.ts index 556278de8..2bba305b5 100644 --- a/apps/hub/src/server.ts +++ b/apps/hub/src/server.ts @@ -15,8 +15,9 @@ import { resolve } from "node:path"; import { authorize } from "@intx/authz"; import { createGrantStore, createWorkflowRunLaunchSpecStore, resolveSenderPrincipal, schema, type DB, type ResolvedSenderPrincipal } from "@intx/db"; import { createEnvKeyCredentialCipher } from "@intx/crypto"; -import { hexDecode } from "@intx/types"; +import { deriveWorkflowRunId, hexDecode } from "@intx/types"; import { generateId } from "@intx/hub-common"; +import { SidecarIdentityValidationError } from "@intx/hub-sessions"; import { eq } from "drizzle-orm"; import { getPath } from "hono/utils/url"; import { createMailTriggeredRunGrantsMaterializer, createRequireGrant, HUB_CONDITION_REGISTRY } from "@intx/hub-api"; @@ -111,7 +112,7 @@ import { fileRefusal, previewRepoPolicy, readRepoPolicy } from "./github/repo-po import { createAutomation, grantEnabledKinds } from "./github/automation.js"; import { createReconcileLoop } from "./github/reconcile-loop.js"; import { githubUsage } from "./github/github-usage.js"; -import { createTriageReconciler, type ReconcileOutcome } from "./github/triage-reconciler.js"; +import { createTriageReconciler, createUnroutedTries, type ReconcileOutcome } from "./github/triage-reconciler.js"; import { createRepoActivity } from "./github/repo-activity.js"; import { createTenantGithub, createTenantHeadChecks, createTenantOpenHead, createTenantOpenHeads, createTenantOpenPrs } from "./github/tenant-open-heads.js"; import { createTriageStateStore } from "./github/triage-state-store.js"; @@ -309,6 +310,16 @@ function liveListenerOf(tenantId: string, source: RepoSource): Promise { return (await liveListenerOf(tenantId, source))?.address; } +/** Whether stock's router routes the listener's allocation generation now; one whose sidecar's identity could not be checked is left to the send. */ +async function listenerRoutable(address: string): Promise { + const target = await deployedAllocation(composition.db, deriveWorkflowRunId(address)); + if (!target) return false; + try { + return await composition.sidecarRouter.isAllocatedWorkflowActive(target); + } catch (error) { + return error instanceof SidecarIdentityValidationError; + } +} const { supersede, supersedeAll } = createSupersede({ db: composition.db, async cancel(tenantId, runId, reason) { @@ -319,6 +330,7 @@ const { supersede, supersedeAll } = createSupersede({ const toListener = createManagerIntake({ db: composition.db, listener: listenerAddress, + routable: listenerRoutable, readPull: function readPull(tenantId, record, number) { return pullRequests.withDiff(tenantId, record, number); }, @@ -554,6 +566,7 @@ async function workspacePublisher(tenantId: string): Promise { } // The portal build writes the workflows under its public files, which the hub serves from PORTAL_DIR once built. const workflowBundleDir = resolve(env.PORTAL_DIR ?? resolve(ROOT, "apps/web/public"), "packages"); +const unroutedTries = createUnroutedTries(); const workflowRollout = createWorkflowRollout({ db: composition.db, assets: composition.assetService, @@ -579,7 +592,8 @@ const workflowRollout = createWorkflowRollout({ status(tenantId, runId) { return composition.workflowLifecycleService.getStatus(tenantId, runId); }, - live() { + live(tenantId, source) { + unroutedTries.reset(tenantId, source); triageLoop?.kick(); }, lock: createWorkspaceLock(composition.db.$client), @@ -664,6 +678,7 @@ function reconcilerFor(source: RepoSource) { store: triageStateStore, readCheckPack, send: sendTriageMail, + unroutedTries, policy: reconcilePolicy, now, log: logJson, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 6288d3d99..9c1714a76 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -351,15 +351,27 @@ any other tenant or an unknown sender. Mail is addressed to a listener's run, and stock holds none for a run that has gone, so a head whose mail reached no listener is held in its - triage-state row instead: the row fails with `unrouted`. The reconciler - queues an unrouted head on its first pass with a listener up, without - backoff, and those tries add nothing to the head's attempts. A coalescer - mail that reached no listener starts a pass at once, whose listener check - rolls a workspace left with none; a pass that left a head unrouted runs - again 15 seconds later; and a roll starts a pass as soon as its listener - runs, logging `workflow_release_live`, before it waits for the listeners it - replaces to stop. A pull request opened while its listener is replaced is - therefore mailed as soon as the new one runs. + triage-state row instead: the row fails with `unrouted`. That is mail sent + when the host had no listener, or that its listener took none of because + it was still starting, stopping or gone, or the hub held no route to it; + mail a live manager did not take fails as usual. Before recording a pull + request's event, the intake asks stock's router whether it routes the + listener, when a pull request named has no manager up, so mail the + listener cannot take records nothing. The reconciler queues an unrouted + head on its first pass with a listener up, without backoff, and those tries + add nothing to the head's attempts. A coalescer mail that reached no + listener starts a pass at once, whose listener check rolls a workspace + left with none, and a pass whose mail for a head reached no listener runs + again 15 seconds later, for at most 20 passes in a row per head; past that + the head logs `triage_unrouted_waiting` once and waits for the ordinary + pass. A listener taking the head's mail, or a roll finding the host's + listener running, starts those 20 passes over. A roll starts a pass as + soon as its new listener runs, logging `workflow_release_live`, before it + waits for the listeners it replaces to stop, and also when it finds the + host's listener already current and running. A base branch move clears + `unrouted`, since it decides when each head goes out again. A pull request + opened while its listener is replaced is therefore mailed as soon as the + new one runs. - `triage-manager-github` and `triage-manager-bitbucket-datacenter` (`triageManager`): one live run per pull request, which lives until its head has its final verdict, and the only workflow that writes to the pull request. It takes diff --git a/packages/triage-contracts/src/triage-state.ts b/packages/triage-contracts/src/triage-state.ts index 4f75f9b2c..2080b2cb2 100644 --- a/packages/triage-contracts/src/triage-state.ts +++ b/packages/triage-contracts/src/triage-state.ts @@ -68,7 +68,6 @@ function parseRow(raw: unknown): PrTriageRow | null { const undeliveredSince = optionalString(row.undeliveredSince); const staleBefore = optionalString(row.staleBefore); const error = optionalString(row.error); - if (row.unrouted !== undefined && row.unrouted !== true) return null; return { number, headSha, From e5e6022c4c8027a457f570b0994709096ea7c8b7 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 28 Oct 2026 00:38:44 -0700 Subject: [PATCH 5/6] test(chain): a held head's spent attempt does not hold it back once the replacement listener runs The scenario freezes the listener's sidecar until stock drops it, opens a pull request whose webhook and reconciler mail both reach no listener, stops the listener, and checks the head is mailed within 45 seconds of the replacement running and gets its verdict. --- tests/chain/unrouted.test.ts | 51 +++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/tests/chain/unrouted.test.ts b/tests/chain/unrouted.test.ts index c5f7e27dc..a6dbbcfc7 100644 --- a/tests/chain/unrouted.test.ts +++ b/tests/chain/unrouted.test.ts @@ -5,16 +5,19 @@ import { chainStack, UNROUTED, isLiveDeployment, sha, pullOf, comments, labelled // The default pass interval, so only the hub's short retry and its pass on a listener starting can mail the head in time. const RECONCILE_INTERVAL_MS = 5 * 60_000; -const UNROUTED_RETRY_MS = 15_000; +/** The short retry, and one more for a pass that finds the new listener not yet ready. */ +const MAILED_MS = 45_000; +/** Stock drops a silent sidecar's socket after its one-minute ping timeout. */ +const DISCONNECT_MS = 90_000; const REPLACED_MS = 3 * 60_000; const chain = chainStack({ TRIAGE_RECONCILE_INTERVAL_MS: String(RECONCILE_INTERVAL_MS) }); const { github, pushed, until, rows } = chain; -/** The GitHub listener's live deployments, oldest first. */ -async function listeners(): Promise { - const found = await rows(`select r.id, r.status, r.cancellation_requested_at from workflow_run r join workflow_definition d on d.id = r.definition_id left join workflow_run_launch_spec s on s.anchor_run_id = r.id where r.id = r.anchor_run_id and coalesce(s.frozen_approval_bundle -> 'projection' ->> 'id', d.name) = '${TRIAGE_LISTENERS.github}' order by r.created_at`); - return found.filter((row) => isLiveDeployment(String(row["status"])) && row["cancellation_requested_at"] === null).map((row) => String(row["id"])); +/** The GitHub listener's live deployments not being stopped, oldest first, and the pid of the local sidecar each runs on. */ +async function listeners(): Promise<{ id: string; pid: number }[]> { + const found = await rows(`select r.id, r.status, r.cancellation_requested_at, a.external_ref from workflow_run r join workflow_definition d on d.id = r.definition_id left join workflow_run_launch_spec s on s.anchor_run_id = r.id join sidecar_allocation a on a.anchor_run_id = r.id where r.id = r.anchor_run_id and coalesce(s.frozen_approval_bundle -> 'projection' ->> 'id', d.name) = '${TRIAGE_LISTENERS.github}' order by r.created_at`); + return found.filter((row) => isLiveDeployment(String(row["status"])) && row["cancellation_requested_at"] === null).map((row) => ({ id: String(row["id"]), pid: Number(row["external_ref"]) })); } function logged(msg: string): Array> { @@ -28,32 +31,44 @@ function msOf(entry: Record): number { const unrouted = once(async function unrouted() { const [listener] = await listeners(); if (!listener) throw new Error("The GitHub listener is not live."); - const { transport, tenantId } = chain.workspace; - // Stopped through stock's stop-run route, the listener's run is gone until the hub rolls the workspace again. - await transport.fetch("DELETE", `/api/tenants/${tenantId}/workflows/runs/${listener}`); const head = sha("unrouted-1"); const missed = github.misses.length; - await pushed(UNROUTED, pullOf(1, head)); - const failed = await until("the pull request's mail to reach no listener", function seen() { - return logged("forward_failed").find((entry) => entry["repo"] === UNROUTED); - }); + // A frozen sidecar stops pinging, so stock drops its socket and the listener's route while its run stays live. + process.kill(listener.pid, "SIGSTOP"); + try { + await until("stock to drop the listener's sidecar", function dropped() { + return chain.hub.logs.some((line) => line.includes("Sidecar") && line.includes("disconnected")) ? true : undefined; + }, DISCONNECT_MS); + await pushed(UNROUTED, pullOf(1, head)); + // The webhook's mail and then the reconciler's both reach no listener; a head not held for one would now back off. + await until("the reconciler's mail to reach no listener too", function seen() { + return logged("forward_failed").filter((entry) => entry["repo"] === UNROUTED).length >= 2 ? true : undefined; + }); + const { transport, tenantId } = chain.workspace; + // Stopped through stock's stop-run route, the listener's run is gone until the hub rolls the workspace again. + await transport.fetch("DELETE", `/api/tenants/${tenantId}/workflows/runs/${listener.id}`); + } finally { + process.kill(listener.pid, "SIGCONT"); + } const live = await until("a replacement listener to run", function seen() { - return logged("workflow_release_live").find((entry) => entry["deployment"] !== listener); + return logged("workflow_release_live").find((entry) => entry["deployment"] !== listener.id); }, REPLACED_MS); const sent = await until("the pull request's mail to go out again", function seen() { return logged("triage_requeued").find((entry) => entry["repo"] === UNROUTED && entry["pr"] === 1); - }); + }, REPLACED_MS); await until("the verdict", function posted() { return comments(github.writesTo(UNROUTED, 1)).length > 0 ? true : undefined; }); + const failed = logged("forward_failed").filter((entry) => entry["repo"] === UNROUTED); return { listener, failed, live, sent, writes: github.writesTo(UNROUTED, 1), misses: github.misses.slice(missed) }; }); -test("a pull request opened while its listener is replaced is mailed to the replacement within the short retry of it running, and gets its verdict", async () => { +test("a pull request opened while its listener is replaced, whose mail already spent an attempt, is mailed to the replacement within a short retry of it running and gets its verdict", async () => { const { listener, failed, live, sent, writes, misses } = await unrouted(); - expect(msOf(failed)).toBeLessThan(msOf(live)); - expect(live["deployment"]).not.toBe(listener); - expect(msOf(sent) - msOf(live)).toBeLessThanOrEqual(UNROUTED_RETRY_MS); + expect(failed.length).toBeGreaterThanOrEqual(2); + expect(failed.every((entry) => msOf(entry) < msOf(live) && entry["level"] === "info")).toBe(true); + expect(live["deployment"]).not.toBe(listener.id); + expect(msOf(sent) - msOf(live)).toBeLessThanOrEqual(MAILED_MS); expect(misses).toEqual([]); expect(comments(writes)).toEqual([expect.stringContaining("**Triage: Pass**")]); expect(labelled(writes)).toEqual([VERDICT_LABELS.pass]); From 7c63e0d1afda49411e2ba0820ab86e9d0e3176c3 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 28 Oct 2026 00:55:26 -0700 Subject: [PATCH 6/6] fix(hub): hold a person's triage request while the listener is replaced A backlog start, Triage again and a held Do's approval made while the listener takes no mail answer 202: their heads are written as queued triage rows marked unrouted, and the reconciler mails them once a listener is up. The approval is recorded first, so nothing a person did is dropped. The 409 deployment_not_ready answer, the 503 for listener mail during the gap and the portal's readiness polling are removed; a host with no listener still answers 503. A head's short-retry count is forgotten once its row is no longer held. --- apps/hub/src/github/backlog-triage.test.ts | 21 ++++++ apps/hub/src/github/backlog-triage.ts | 32 ++++++++-- apps/hub/src/github/deployment.ts | 1 - .../github-read-access.postgres.test.ts | 57 ++++++++++++++++- apps/hub/src/github/held-dos.ts | 26 ++++++-- .../src/github/interleave.postgres.test.ts | 51 +++++++++++++++ apps/hub/src/github/pr-triage.test.ts | 1 + apps/hub/src/github/pr-triage.ts | 11 +++- apps/hub/src/github/triage-queue.ts | 11 +++- apps/hub/src/github/triage-reconciler.test.ts | 10 +++ apps/hub/src/github/triage-reconciler.ts | 14 +++- apps/hub/src/manager-intake.postgres.test.ts | 12 +++- apps/hub/src/manager-intake.ts | 7 +- apps/hub/src/server.ts | 27 ++++---- apps/web/src/lib/hub-api.test.ts | 64 ++++--------------- apps/web/src/lib/hub-api.ts | 30 --------- apps/web/src/lib/portal.tsx | 14 +--- docs/ARCHITECTURE.md | 14 +++- .../Corbits Triage User Manual.md | 2 +- docs/SELF_HOST.md | 5 +- 20 files changed, 278 insertions(+), 132 deletions(-) diff --git a/apps/hub/src/github/backlog-triage.test.ts b/apps/hub/src/github/backlog-triage.test.ts index b92ece440..f62757b12 100644 --- a/apps/hub/src/github/backlog-triage.test.ts +++ b/apps/hub/src/github/backlog-triage.test.ts @@ -5,6 +5,25 @@ import { checkPackName, emptyPack } from "@corbits/triage-contracts"; const PACK = { ...emptyPack("octocat/hello"), checks: { "no-merge-conflicts": { enabled: true } } }; import { createBacklogStart, createGithubBacklog } from "./backlog-triage.js"; import { NoLiveDeploymentError } from "./deployment.js"; +import { DEFAULT_RECONCILE_POLICY } from "./reconcile-plan.js"; + +/** A listener takes every backlog here, so no head is held. */ +const NOT_HELD = { + async openHeads(): Promise { + throw new Error("no head is held"); + }, + store: { + async load(): Promise { + throw new Error("no head is held"); + }, + async save(): Promise { + throw new Error("no head is held"); + }, + }, + policy: DEFAULT_RECONCILE_POLICY, + now: () => new Date(), + retry() {}, +}; /** No deployment holds the credential in these tests. */ async function noKeyPush(): Promise {} @@ -32,6 +51,7 @@ function backlog(repo: Record, sent: unknown[], sendMail = asyn return { status: "ok", pack: PACK, automatic: ["labels"], source: SOURCE }; }, sendMail, + ...NOT_HELD, log() {}, }); } @@ -60,6 +80,7 @@ test("a Bitbucket Data Center repository's backlog goes to its listener like a G async sendMail(_tenant, payload) { sent.push(payload); }, + ...NOT_HELD, log() {}, }); const started = await start(MEMBER.tenantId, { name: "TRI/widgets", source: "bitbucket-datacenter", connected: true, enabled: true }); diff --git a/apps/hub/src/github/backlog-triage.ts b/apps/hub/src/github/backlog-triage.ts index 664d4c908..00a428332 100644 --- a/apps/hub/src/github/backlog-triage.ts +++ b/apps/hub/src/github/backlog-triage.ts @@ -4,13 +4,15 @@ import { type } from "arktype"; import { errorText } from "@corbits/common"; import { triageStopped } from "@corbits/triage-contracts"; -import { isRunTriggerUnroutable } from "@corbits/webhooks"; import { mailPayload } from "./bridge.js"; import type { PolicyReader } from "./repo-policy.js"; -import { DeploymentNotReadyError, NoLiveDeploymentError } from "./deployment.js"; +import { ListenerUnroutedError, NoLiveDeploymentError } from "./deployment.js"; +import type { ReconcilePolicy } from "./reconcile-plan.js"; +import { holdHead } from "./triage-queue.js"; +import type { TriageStateStore } from "./triage-state-store.js"; import { failure, type PortalCredentialDeps } from "./portal-credential.js"; import { grantedTriageRecord, triageInputs } from "./pr-triage.js"; -import { sourceOf, type RepoRecord } from "./tenant-config.js"; +import { sourceOf, triageStateKey, type RepoRecord } from "./tenant-config.js"; import type { Member } from "../integrations/app.js"; export const BacklogBody = type({ repo: /^[\w.-]+\/[\w.-]+$/ }); @@ -18,6 +20,13 @@ export const BacklogBody = type({ repo: /^[\w.-]+\/[\w.-]+$/ }); export type BacklogStartDeps = { readCheckPack: PolicyReader; sendMail: (tenantId: string, payload: unknown) => Promise; + /** The repository's open pull requests and their heads, held when the listener takes no mail. */ + openHeads: (tenantId: string, record: RepoRecord) => Promise<{ number: number; headSha: string; draft: boolean }[]>; + store: TriageStateStore; + policy: ReconcilePolicy; + now: () => Date; + /** Asks the reconciler for a pass soon. */ + retry: () => void; log: (entry: Record) => void; }; @@ -26,6 +35,16 @@ export type GithubBacklogDeps = PortalCredentialDeps & BacklogStartDeps; export type BacklogStart = (tenantId: string, record: RepoRecord) => Promise; export function createBacklogStart(deps: BacklogStartDeps): BacklogStart { + /** Each open head goes out once a listener is up, as the backlog mail would have sent it. */ + async function holdOpenHeads(tenantId: string, record: RepoRecord, err: ListenerUnroutedError): Promise { + const now = deps.now(); + for (const pull of (await deps.openHeads(tenantId, record)).filter((open) => !open.draft)) { + await holdHead(deps.store, { tenantId, repo: triageStateKey(record), number: pull.number, headSha: pull.headSha }, err, now, deps.policy); + } + deps.retry(); + deps.log({ level: "info", msg: "backlog_held", tenantId, repo: record.name, error: String(err) }); + } + return async function startBacklog(tenantId, record) { const repo = record.name; const inputs = await triageInputs(deps, tenantId, record); @@ -33,8 +52,11 @@ export function createBacklogStart(deps: BacklogStartDeps): BacklogStart { try { await deps.sendMail(tenantId, { ...mailPayload(record, inputs.policy, inputs.pack, {}), kind: "backlog" }); } catch (err) { - if (err instanceof DeploymentNotReadyError) return failure(409, err.code, err.message); - if (err instanceof NoLiveDeploymentError || isRunTriggerUnroutable(err)) return failure(503, "service_not_running", triageStopped(sourceOf(record))); + if (err instanceof ListenerUnroutedError) { + await holdOpenHeads(tenantId, record, err); + return Response.json({ status: "started" }, { status: 202 }); + } + if (err instanceof NoLiveDeploymentError) return failure(503, "service_not_running", triageStopped(sourceOf(record))); deps.log({ level: "error", msg: "backlog_request_failed", tenantId, repo, error: String(err) }); return failure(502, "hub_unavailable", `Could not start triage of ${repo}: ${errorText(err)}. Try again.`); } diff --git a/apps/hub/src/github/deployment.ts b/apps/hub/src/github/deployment.ts index 641626356..3ffd37378 100644 --- a/apps/hub/src/github/deployment.ts +++ b/apps/hub/src/github/deployment.ts @@ -21,7 +21,6 @@ export class TriageStoppedError extends NoLiveDeploymentError { } export class DeploymentNotReadyError extends Error { - readonly code = "deployment_not_ready"; constructor() { super("The workflow deployment is still starting."); } diff --git a/apps/hub/src/github/github-read-access.postgres.test.ts b/apps/hub/src/github/github-read-access.postgres.test.ts index 3e710ccff..19fe02262 100644 --- a/apps/hub/src/github/github-read-access.postgres.test.ts +++ b/apps/hub/src/github/github-read-access.postgres.test.ts @@ -6,11 +6,14 @@ import { HUB_CONDITION_REGISTRY } from "@intx/hub-api"; import { createEnvKeyCredentialCipher } from "@intx/crypto"; import { createDB, createGrantStore, dropSchema, runMigrations, schema } from "@intx/db"; import { credentialAad } from "@intx/types"; -import { TRIAGE_ROLE, triageStopped, type ActionKind } from "@corbits/triage-contracts"; +import { TRIAGE_ROLE, triageStopped, type ActionKind, type PrTriageRow } from "@corbits/triage-contracts"; import { doEffectId } from "@corbits/triage-workflows/logic/actions"; import { guardedApp } from "../integrations/test-app.js"; import { createAuditLog } from "../audit-log.js"; +import { ListenerUnroutedError } from "./deployment.js"; import { createGithubHeldDos } from "./held-dos.js"; +import { DEFAULT_RECONCILE_POLICY } from "./reconcile-plan.js"; +import type { TriageStateStore } from "./triage-state-store.js"; import { createGithubOpenPulls, createOpenPullsCache } from "./open-pulls.js"; import { createTenantOpenPrs } from "./tenant-open-heads.js"; import { createGithubPrActions } from "./pr-actions.js"; @@ -106,6 +109,24 @@ async function authorizePortal(principalId: string, tenantId: string, resource: } const deps = { db: handle.db, cipher, onKeyMoved: noKeyPush, authorize: authorizePortal, githubApiOrigin }; +/** Triage rows kept in memory, with the version the store would give each write. */ +function triageRows(): TriageStateStore & { rows: PrTriageRow[] } { + let version = 0; + const kept = { + rows: [] as PrTriageRow[], + async load() { + return { rows: kept.rows, version: version === 0 ? null : { artifactId: "art_state", version } }; + }, + async save(_tenantId: string, _repo: string, rows: PrTriageRow[]) { + kept.rows = rows; + version += 1; + return { artifactId: "art_state", version }; + }, + }; + return kept; +} +/** The listener takes every approval here, so no head is held. */ +const NOT_HELD = { store: triageRows(), policy: DEFAULT_RECONCILE_POLICY, now: () => new Date(), retry() {} }; const list = createTenantOpenPrs({ ...deps, tenantBitbucket: async () => undefined }); const openPulls = createGithubOpenPulls({ ...deps, list, cache: createOpenPullsCache(() => new Date()) }); const reads = createRepoReads(() => new Date()); @@ -341,6 +362,7 @@ test("PostgreSQL: approving a held Do needs its kind's grant and hands the liste let recorded: Array> = []; const heldDos = createGithubHeldDos({ ...deps, + ...NOT_HELD, runReader: { async readRunEvents() { return events; } }, verdicts: { async forPull() { return recorded as VerdictRecord[]; } }, async toListener(_tenantId, payload) { @@ -376,6 +398,38 @@ test("PostgreSQL: approving a held Do needs its kind's grant and hands the liste expect(await approve("usr_approver", ship)).toMatchObject({ status: 503, body: { error: { code: "service_not_running", message: triageStopped("github") } } }); }); +test("PostgreSQL: a held Do approved while the listener is starting answers 202, and its head is queued for the reconciler to mail once the listener is up", async () => { + const target = { labels: ["api"] }; + const tag = { list: "pass" as const, index: 0, kind: "labels" as const, target, held: true, effectId: doEffectId({ repo: REPO, number: 8, headSha: HEAD, list: "pass", index: 0, kind: "labels", target }) }; + const verdict = { repo: REPO, number: 8, headSha: HEAD, request: { repo: REPO, number: 8, labels: [], owned: [], comment: "" }, actions: [tag] }; + const events = [{ seq: 1, type: "StepCompleted", body: { type: "StepCompleted", seq: 1, stepId: "evaluate", output: { ref: `inline:${JSON.stringify(verdict)}` } } }]; + let kicked = 0; + const store = triageRows(); + const heldDos = createGithubHeldDos({ + ...deps, + ...NOT_HELD, + store, + retry() { + kicked += 1; + }, + runReader: { async readRunEvents() { return events; } }, + verdicts: { async forPull() { return [{ runId: "wfr_gap", degraded: null, runStatus: "completed" }] as VerdictRecord[]; } }, + async toListener() { + throw new ListenerUnroutedError("github", "wfr_listener@tenant.example"); + }, + async passRequest() { + return undefined; + }, + log() {}, + }); + await handle.db.insert(schema.workflowDefinition).values({ id: "wfd_gap", tenantId: TENANT, name: "pr-triage", status: "stopped" }); + await handle.db.insert(schema.workflowRun).values({ id: "wfr_gap", definitionId: "wfd_gap", anchorRunId: "wfr_gap", tenantId: TENANT, status: "completed" }); + const res = await heldDos({ runId: "wfr_gap", repo: REPO, number: 8, effectId: tag.effectId, kind: "labels" }, member("usr_approver")); + expect({ status: res.status, body: await res.json() }).toEqual({ status: 202, body: { status: "sent" } }); + expect(kicked).toBe(1); + expect(store.rows.map((row) => `${row.number}:${row.headSha}:${row.status}:${row.unrouted === true}`)).toEqual([`8:${HEAD}:failed:true`]); +}); + test("PostgreSQL: a held Do of a verdict a manager reported is approved from the recorded verdict and the mirror request its pass kept, and goes to the listener; once the run's newer verdict replaced it, it is superseded", async () => { await handle.db.insert(schema.workflowDefinition).values({ id: "wfd_manager", tenantId: TENANT, name: "triage-manager-github", status: "stopped" }); await handle.db.insert(schema.workflowRun).values({ id: "wfr_manager", definitionId: "wfd_manager", anchorRunId: "wfr_manager", tenantId: TENANT, status: "running" }); @@ -387,6 +441,7 @@ test("PostgreSQL: a held Do of a verdict a manager reported is approved from the const listened: unknown[] = []; const heldDos = createGithubHeldDos({ ...deps, + ...NOT_HELD, runReader: { async readRunEvents() { return [{ seq: 0, type: "RunStarted", body: { type: "RunStarted", seq: 0 } }]; } }, verdicts: { async forPull() { return recorded as VerdictRecord[]; } }, async toListener(_tenantId, payload) { diff --git a/apps/hub/src/github/held-dos.ts b/apps/hub/src/github/held-dos.ts index 183f347b5..9fa3c8fdb 100644 --- a/apps/hub/src/github/held-dos.ts +++ b/apps/hub/src/github/held-dos.ts @@ -4,7 +4,6 @@ // the pull request's manager writes it at most once under its effect id. import { and, eq } from "drizzle-orm"; import { type } from "arktype"; -import { isRunTriggerUnroutable } from "@corbits/webhooks"; import { ACTION_KINDS, AGENT_DO, APPROVE_DO_KIND, DoList, repoPolicy, triageStopped, type ActionKind, type ResolvedTarget } from "@corbits/triage-contracts"; import { schema } from "@intx/db"; import { formatRunAddress } from "@intx/types"; @@ -16,8 +15,11 @@ import { doRefused, failure, type PortalCredentialDeps } from "./portal-credenti import type { Member } from "../integrations/app.js"; import type { PullRequestRef } from "../manager-state.js"; import type { VerdictRecordStore } from "./verdict-record-store.js"; -import { DeploymentNotReadyError, ListenerUnroutedError } from "./deployment.js"; -import { repoRecords, sourceOf, triageNs, type RepoRecord } from "./tenant-config.js"; +import { ListenerUnroutedError } from "./deployment.js"; +import type { ReconcilePolicy } from "./reconcile-plan.js"; +import { holdHead } from "./triage-queue.js"; +import type { TriageStateStore } from "./triage-state-store.js"; +import { repoRecords, sourceOf, triageNs, triageStateKey, type RepoRecord } from "./tenant-config.js"; import { verdictFor } from "./triage-runs.js"; export const GITHUB_HELD_DOS_PATH = "/api/integrations/github-held-dos"; @@ -45,6 +47,12 @@ export type GithubHeldDosDeps = PortalCredentialDeps & { toListener: (tenantId: string, payload: unknown) => Promise; /** The mirror request a manager's pass kept with the verdict it posted on the head. */ passRequest: (tenantId: string, pr: PullRequestRef, runId: string, head: string) => Promise | undefined>; + /** Holds the head of an approval its listener took no mail for, which the reconciler mails once one is up. */ + store: TriageStateStore; + policy: ReconcilePolicy; + now: () => Date; + /** Asks the reconciler for a pass soon. */ + retry: () => void; log: (entry: Record) => void; }; @@ -139,10 +147,14 @@ export function createGithubHeldDos(deps: GithubHeldDosDeps) { try { if (!(await deps.toListener(tenantId, mailed))) return failure(503, "service_not_running", triageStopped(repoSource)); } catch (err) { - deps.log({ level: "error", msg: "held_do_approval_failed", ...logged, error: String(err) }); - if (err instanceof DeploymentNotReadyError) return failure(409, err.code, err.message); - const unroutable = err instanceof ListenerUnroutedError || isRunTriggerUnroutable(err); - return failure(unroutable ? 503 : 502, unroutable ? "service_not_running" : "hub_unavailable", `Could not send the approval: ${errorText(err)}. Try again.`); + if (!(err instanceof ListenerUnroutedError)) { + deps.log({ level: "error", msg: "held_do_approval_failed", ...logged, error: String(err) }); + return failure(502, "hub_unavailable", `Could not send the approval: ${errorText(err)}. Try again.`); + } + // The approval is recorded; the head goes out with it once a listener is up. + await holdHead(deps.store, { tenantId, repo: triageStateKey(record), number, headSha: approval.step.headSha }, err, deps.now(), deps.policy); + deps.retry(); + deps.log({ level: "info", msg: "held_do_approval_held", ...logged, error: String(err) }); } deps.log({ level: "info", msg: "held_do_approved", ...logged }); return Response.json({ status: "sent", ...(warning && { warning }) }, { status: 202 }); diff --git a/apps/hub/src/github/interleave.postgres.test.ts b/apps/hub/src/github/interleave.postgres.test.ts index bb2eb3a36..83d4fc576 100644 --- a/apps/hub/src/github/interleave.postgres.test.ts +++ b/apps/hub/src/github/interleave.postgres.test.ts @@ -7,6 +7,8 @@ import { createDB, runMigrations, schema } from "@intx/db"; import { runArtifactMigrations } from "@corbits/artifacts"; import type { CheckRun } from "@corbits/github-reads"; import { emptyPack, type CheckPack, type PrTriageRow } from "@corbits/triage-contracts"; +import { createBacklogStart } from "./backlog-triage.js"; +import { ListenerUnroutedError } from "./deployment.js"; import { createGithubPrTriage } from "./pr-triage.js"; import { DEFAULT_RECONCILE_POLICY } from "./reconcile-plan.js"; import { createTriageReconciler, createUnroutedTries } from "./triage-reconciler.js"; @@ -53,6 +55,7 @@ function route(store: TriageStateStore, deliver: Deliver) { managerRuns: noRuns, store, send: async ({ payload }) => deliver(payload), + retry() {}, policy: DEFAULT_RECONCILE_POLICY, now: () => NOW, log: () => {}, @@ -142,3 +145,51 @@ test("PostgreSQL: a failed mail whose first restore is refused still puts the he expect((await handle({ repo: REPO, number: 8 }, { tenantId: TENANT, principalId: "member-1" })).status).toBe(202); }); }, 120_000); + +test("PostgreSQL: a backlog started while the listener is starting answers 202, and its open heads are queued and mailed once the listener is up", async () => { + await withStore(async (store) => { + let kicked = 0; + const start = createBacklogStart({ + readCheckPack: async () => ({ status: "ok", pack: PACK, automatic: [], source: { file: { path: ".corbits-triage.yml", sha: "art_pack-1" } } }), + async sendMail() { + throw new ListenerUnroutedError("github", DEPLOYMENT.address); + }, + openHeads: async () => [{ number: 8, headSha: "abc123", draft: false }, { number: 10, headSha: "sha10", draft: false }, { number: 11, headSha: "sha11", draft: true }], + store, + policy: DEFAULT_RECONCILE_POLICY, + now: () => NOW, + retry() { + kicked += 1; + }, + log: () => {}, + }); + const res = await start(TENANT, { name: REPO, connected: true, enabled: true }); + expect({ status: res.status, body: await res.json() }).toEqual({ status: 202, body: { status: "started" } }); + expect(kicked).toBe(1); + expect((await store.load(TENANT, REPO)).rows.map((row) => `${row.number}:${row.status}:${row.unrouted === true}`)).toEqual(["8:failed:true", "10:failed:true"]); + + const mails: Array<{ items?: Array<{ prNumber: number }> }> = []; + await reconciler(store, [8, 10], async (payload) => { + mails.push(payload as never); + })(); + expect(mails.flatMap((mail) => mail.items?.map((item) => item.prNumber) ?? [])).toEqual([8, 10]); + expect((await store.load(TENANT, REPO)).rows.map((row) => `${row.number}:${row.status}`)).toEqual(["8:queued", "10:queued"]); + }); +}, 120_000); + +test("PostgreSQL: a maintainer's triage request while the listener is starting answers 202 and the head is mailed once the listener is up", async () => { + await withStore(async (store) => { + const handle = route(store, async () => { + throw new ListenerUnroutedError("github", DEPLOYMENT.address); + }); + const res = await handle({ repo: REPO, number: 8 }, { tenantId: TENANT, principalId: "member-1" }); + expect({ status: res.status, body: await res.json() }).toEqual({ status: 202, body: { status: "queued", headSha: "abc123" } }); + expect((await store.load(TENANT, REPO)).rows.map((row) => `${row.number}:${row.status}:${row.unrouted === true}`)).toEqual(["8:failed:true"]); + + const mails: unknown[] = []; + await reconciler(store, [8], async (payload) => { + mails.push(payload); + })(); + expect(mailsFor8(mails)).toBe(1); + }); +}, 120_000); diff --git a/apps/hub/src/github/pr-triage.test.ts b/apps/hub/src/github/pr-triage.test.ts index 113d6434b..8c0b205c7 100644 --- a/apps/hub/src/github/pr-triage.test.ts +++ b/apps/hub/src/github/pr-triage.test.ts @@ -78,6 +78,7 @@ function handler(repo: Record, s: Stub, overrides: Partial { s.delivered.push({ payload }); }, + retry() {}, policy: DEFAULT_RECONCILE_POLICY, now: () => NOW, log: () => {}, diff --git a/apps/hub/src/github/pr-triage.ts b/apps/hub/src/github/pr-triage.ts index b8b95c12c..232e2a17e 100644 --- a/apps/hub/src/github/pr-triage.ts +++ b/apps/hub/src/github/pr-triage.ts @@ -18,7 +18,7 @@ import { isStuck, runKey, type ObservedRun, type ReconcilePolicy } from "./recon import { repoRecords, sourceOf, triageNs, triageStateKey, type RepoRecord } from "./tenant-config.js"; import type { LiveDeployment } from "./deployment.js"; import type { ObservedRuns } from "./manager-runs.js"; -import { isQueuedSince, isRunningSince, queueHead, restoreHead } from "./triage-queue.js"; +import { failDelivery, isQueuedSince, isRunningSince, queueHead, reachedNoListener, restoreHead } from "./triage-queue.js"; import type { TriageStateStore } from "./triage-state-store.js"; export const GITHUB_PR_TRIAGE_PATH = "/api/integrations/github-triage"; @@ -34,6 +34,8 @@ export type GithubPrTriageDeps = PortalCredentialDeps & { managerRuns: ManagerRuns; store: TriageStateStore; send: (job: TriageJob) => Promise; + /** Asks the reconciler for a pass soon. */ + retry: () => void; policy: ReconcilePolicy; now: () => Date; log: (entry: Record) => void; @@ -120,6 +122,13 @@ export function createGithubPrTriage(deps: GithubPrTriageDeps) { try { await deps.send({ ...head, record, payload: mailPayload(record, policy, pack, { prNumber: number, headSha, noWait: true }) }); } catch (err) { + if (reachedNoListener(err)) { + // The head stays queued for the reconciler, which mails it once a listener is up. + await failDelivery(deps.store, head, err, now); + deps.retry(); + deps.log({ level: "info", msg: "triage_request_held", tenantId, principalId, repo, pr: number, headSha, error: String(err) }); + return Response.json({ status: "queued", headSha }, { status: 202 }); + } deps.log({ level: "error", msg: "triage_request_failed", tenantId, repo, pr: number, headSha, error: String(err) }); try { await restoreHead(deps.store, head, queued.remembered); diff --git a/apps/hub/src/github/triage-queue.ts b/apps/hub/src/github/triage-queue.ts index 98904625c..deb89cfc0 100644 --- a/apps/hub/src/github/triage-queue.ts +++ b/apps/hub/src/github/triage-queue.ts @@ -22,7 +22,7 @@ export function undelivered(row: PrTriageRow, source: RepoSource): boolean { } /** The mail reached no listener: the host had none, or its listener took no mail. A manager that took none is an ordinary failure. */ -function reachedNoListener(err: unknown): boolean { +export function reachedNoListener(err: unknown): boolean { return err instanceof TriageStoppedError || err instanceof ListenerUnroutedError; } @@ -131,6 +131,15 @@ export async function failDelivery(store: TriageStateStore, head: QueuedHead, er return unrouted; } +/** + * Holds a head a person asked for whose mail reached no listener: queued, then failed unrouted, so the reconciler mails + * it as soon as a listener is up. A head a run is on is left to that run. + */ +export async function holdHead(store: TriageStateStore, head: QueuedHead, err: unknown, now: Date, policy: ReconcilePolicy): Promise { + const queued = await queueHead(store, head, now, (prior) => isRunningSince(prior, now, policy)); + if (queued.status === "queued") await failDelivery(store, head, err, now); +} + /** Marks a still queued head failed after its mail did not go out, so the reconciler queues it again; it keeps when its mail first did not. */ export function failHead(store: TriageStateStore, head: QueuedHead, error: string, now: Date): Promise { return replaceQueued(store, head, function failed(row) { diff --git a/apps/hub/src/github/triage-reconciler.test.ts b/apps/hub/src/github/triage-reconciler.test.ts index 231684dcb..fc5b5b622 100644 --- a/apps/hub/src/github/triage-reconciler.test.ts +++ b/apps/hub/src/github/triage-reconciler.test.ts @@ -494,6 +494,16 @@ describe("triage reconciler", () => { expect((await reconcile()).unrouted).toBe(true); }); + test("a held head's short retries are forgotten once it is no longer held, as after the webhook's mail went out", async () => { + const sendErrors: Record = { 1: new ListenerUnroutedError("github", formatRunAddress("run_gone", DOMAIN)) }; + const { reconcile, saved, unroutedTries } = harness({}, heads(1, 1), { sendErrors }); + for (let pass = 0; pass < 20; pass += 1) await reconcile(); + const { unrouted: _unrouted, ...row } = saved.get(REPO)!.find((candidate) => candidate.number === 1)!; + saved.set(REPO, [{ ...row, status: "queued", queuedAt: NOW.toISOString() }]); + await reconcile(); + expect(unroutedTries.missed(TENANT_ID, "github", REPO, 1, "sha1")).toBe(1); + }); + test("queued heads that have not started hold the budget until their runs complete", async () => { const logs: Runs = { [REPO]: {} }; const { reconcile, delivered, saved } = harness(logs, heads(1, 12)); diff --git a/apps/hub/src/github/triage-reconciler.ts b/apps/hub/src/github/triage-reconciler.ts index 2fbdf863d..59d681647 100644 --- a/apps/hub/src/github/triage-reconciler.ts +++ b/apps/hub/src/github/triage-reconciler.ts @@ -37,8 +37,12 @@ export function createUnroutedTries() { return `${tenantId}\n${source}\n`; } + function repoOf(tenantId: string, source: RepoSource, repo: string): string { + return `${hostOf(tenantId, source)}${repo}\n`; + } + function keyOf(tenantId: string, source: RepoSource, repo: string, number: number, headSha: string): string { - return `${hostOf(tenantId, source)}${repo}\n${runKey(number, headSha)}`; + return `${repoOf(tenantId, source, repo)}${runKey(number, headSha)}`; } return { @@ -53,6 +57,11 @@ export function createUnroutedTries() { took(tenantId: string, source: RepoSource, repo: string, number: number, headSha: string): void { tries.delete(keyOf(tenantId, source, repo, number, headSha)); }, + /** Forgets the repository's heads but `held`, by run key: one closed, replaced by a new commit or mailed meanwhile. */ + keep(tenantId: string, source: RepoSource, repo: string, held: ReadonlySet): void { + const prefix = repoOf(tenantId, source, repo); + for (const key of tries.keys()) if (key.startsWith(prefix) && !held.has(key.slice(prefix.length))) tries.delete(key); + }, /** The host's listener runs, so each of its heads gets its short retries again. */ reset(tenantId: string, source: RepoSource): void { const host = hostOf(tenantId, source); @@ -284,6 +293,9 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { async function deliverRepo(pass: TenantPass, repo: LoadedRepo, plan: RepoPlan): Promise { const key = triageStateKey(repo.record); + const open = new Set(repo.prs.map((pr) => runKey(pr.number, pr.headSha))); + const held = repo.stored.rows.filter((row) => row.unrouted && open.has(headKey(row))).map(headKey); + deps.unroutedTries.keep(pass.tenantId, deps.source, repo.record.name, new Set(held)); const claimed = await claim(pass.tenantId, key, repo.stored, plan); if (claimed === undefined) return; const headShaOf = (queued: QueuedPr) => claimed.rows.find((row) => row.number === queued.number)!.headSha; diff --git a/apps/hub/src/manager-intake.postgres.test.ts b/apps/hub/src/manager-intake.postgres.test.ts index 527d7b073..5c163e2b4 100644 --- a/apps/hub/src/manager-intake.postgres.test.ts +++ b/apps/hub/src/manager-intake.postgres.test.ts @@ -8,7 +8,7 @@ import type { PullRequestToReview } from "./pull-request-reader.js"; import { mailPayload } from "./github/bridge.js"; import { createPauseGate } from "./github/pr-workflow.js"; import { createManagerIntake, createManagerWake, createOverdueWake, createSupersede } from "./manager-intake.js"; -import { ListenerUnroutedError } from "./github/deployment.js"; +import { ListenerUnroutedError, RunUnavailableError } from "./github/deployment.js"; import { recordEvent } from "./manager-state.js"; import { migrateTriageTables } from "./triage-db.js"; @@ -70,6 +70,7 @@ function stack(db: Db, listener: string | undefined, pull = PULL, gate: Gate = p openHeads: async () => open, withAppLogin: async (_tenantId, payload) => ({ ...payload, appLogin: "triage[bot]" }), async deliver(_tenantId, address, payload) { + if (!routable) throw new RunUnavailableError("the listener is not running"); mailed.push({ address, payload }); }, supersede, @@ -104,6 +105,15 @@ test("PostgreSQL: mail for a pull request with no manager up, while the hub hold }); }, 60_000); +test("PostgreSQL: a held Do's approval while the hub holds no route to the listener is recorded, so the manager the listener later starts for the head runs it", async () => { + await withDatabase(async function approvedInGap(db) { + const { toListener, mailed } = stack(db, LISTENER, PULL, passed, OPEN, false); + const approval = { kind: APPROVE_DO_KIND, runId: "run_m", principalId: "prn_a", list: "pass", index: 0, request: { repo: RECORD.name, number: 7 }, step: { effectId: "e1", headSha: "h1" } }; + await expect(toListener(TENANT, approval)).rejects.toBeInstanceOf(ListenerUnroutedError); + expect({ events: (await events(db)).map((row) => `${row.state.head} ${String(row.state.payload["kind"])}`), mailed }).toEqual({ events: ["h1 approve-do"], mailed: [] }); + }); +}, 60_000); + test("PostgreSQL: each pull request a mail names is recorded as its own event, a webhook once however often it is delivered and any other mail every time, and the listener is mailed the heads", async () => { await withDatabase(async function listener(db) { const { toListener, mailed } = stack(db, LISTENER); diff --git a/apps/hub/src/manager-intake.ts b/apps/hub/src/manager-intake.ts index 59d6746b9..a75093e2a 100644 --- a/apps/hub/src/manager-intake.ts +++ b/apps/hub/src/manager-intake.ts @@ -227,8 +227,11 @@ export function createManagerIntake(deps: ManagerIntakeDeps) { if (!address || typeof repo !== "string") return false; const record = await recordOf(tenantId, repo, source); const pulls = await pullsOf(tenantId, record, request); - // A pull request with no manager up is deployed by the listener, so mail the listener cannot take records nothing. - if (!(await deps.routable(address)) && (await withoutManager(tenantId, source, repo, pulls))) throw new ListenerUnroutedError(source, address); + // A pull request with no manager up is deployed by the listener, so mail the listener cannot take records nothing; its + // triage row holds it. A held Do's approval is recorded all the same, since no row holds what a person approved. + if (request["kind"] !== APPROVE_DO_KIND && !(await deps.routable(address)) && (await withoutManager(tenantId, source, repo, pulls))) { + throw new ListenerUnroutedError(source, address); + } const domain = domainOf(address); const start: Named[] = []; try { diff --git a/apps/hub/src/server.ts b/apps/hub/src/server.ts index 2bba305b5..450ed3674 100644 --- a/apps/hub/src/server.ts +++ b/apps/hub/src/server.ts @@ -118,7 +118,7 @@ import { createTenantGithub, createTenantHeadChecks, createTenantOpenHead, creat import { createTriageStateStore } from "./github/triage-state-store.js"; import { markBaseMoved, type TriageJob } from "./github/triage-queue.js"; import { createGithubManifestIntegration } from "./github/manifest.js"; -import { keepExistingWorkspacesAsking, prepareCorbitsTriagePatch, tenantManager } from "./github/tenant-config.js"; +import { keepExistingWorkspacesAsking, prepareCorbitsTriagePatch, tenantManager, type RepoRecord } from "./github/tenant-config.js"; import { githubRead } from "@corbits/github-tool"; import { bitbucketRead } from "@corbits/bitbucket-datacenter-tool"; @@ -337,11 +337,7 @@ const toListener = createManagerIntake({ withAppLogin: function appLoginOf(tenantId, payload) { return withAppLogin(composition.db, tenantId, payload); }, - openHeads: async function openHeads(tenantId, record) { - const read = await tenantOpenHeads(tenantId); - if (!read) throw new Error("no active source credential"); - return read(record); - }, + openHeads: openHeadsOf, gate: function gateOf(tenantId, record, number, request) { return pauseGate(tenantId, record, number, request); }, @@ -704,6 +700,7 @@ async function reconcileAll(): Promise { return limits.length === 0 ? outcome : { ...outcome, retryAt: Math.min(...limits) }; } triageLoop = createReconcileLoop(reconcileAll, { intervalMs: triageReconcileIntervalMs(env), retryMs: TRIAGE_RETRY_MS, retryMaxMs: TRIAGE_RETRY_MAX_MS }, logJson); +const kickTriage = triageLoop.kick; const githubPrTriage = createGithubPrTriage({ ...credentialDeps, authorize: authorizePortal, @@ -717,6 +714,7 @@ const githubPrTriage = createGithubPrTriage({ managerRuns, store: triageStateStore, send: sendTriageMail, + retry: kickTriage, policy: reconcilePolicy, now, log: logJson, @@ -730,9 +728,12 @@ const githubHeldDos = createGithubHeldDos({ passRequest: function managerPassRequest(tenantId, pr, runId, head) { return passRequest(composition.db, tenantId, pr, runId, head); }, + store: triageStateStore, + policy: reconcilePolicy, + now, + retry: kickTriage, log: logJson, }); -const kickTriage = triageLoop.kick; const releaseDisconnected = createHostRelease({ connectedSources: function connectedSourcesOf(tenantId) { return connectedSources(composition.db, tenantId); @@ -787,9 +788,15 @@ async function recheckOpenPulls(tenantId: string, repo: string, except: number | if (due > 0) kickTriage(); return due; } +async function openHeadsOf(tenantId: string, record: RepoRecord) { + const read = await tenantOpenHeads(tenantId); + if (!read) throw new Error("no active source credential"); + return read(record); +} async function sendBacklogMail(tenantId: string, payload: unknown) { await sendToListener(tenantId, payload); } +const backlogStartDeps = { readCheckPack, sendMail: sendBacklogMail, openHeads: openHeadsOf, store: triageStateStore, policy: reconcilePolicy, now, retry: kickTriage, log: logJson }; const deliveryCache = new DeliveryCache({ filePath: `${env.HUB_DATA_DIR}/delivery-cache.json` }); const bridge = createBridgeHandler({ db: credentialDeps.db, @@ -801,7 +808,7 @@ const bridge = createBridgeHandler({ recheckOpenPulls, refreshPolicyFile: policyFile.onPush, heard: repoActivity.heard, - startBacklog: createBacklogStart({ readCheckPack, sendMail: sendBacklogMail, log: logJson }), + startBacklog: createBacklogStart(backlogStartDeps), repoChanged: repoReads.changed, installationChanged: function installationChanged(tenantId) { void renewGithubToken(tenantId); @@ -879,9 +886,7 @@ const integrations = createIntegrationsApp({ db: composition.db, getSession: com backlog: createGithubBacklog({ ...credentialDeps, authorize: authorizePortal, - readCheckPack, - sendMail: sendBacklogMail, - log: logJson, + ...backlogStartDeps, }), openPulls: githubOpenPulls, prDetails: githubPrDetails, diff --git a/apps/web/src/lib/hub-api.test.ts b/apps/web/src/lib/hub-api.test.ts index ae463cfd7..13e230ee7 100644 --- a/apps/web/src/lib/hub-api.test.ts +++ b/apps/web/src/lib/hub-api.test.ts @@ -10,7 +10,6 @@ import { removeRepository, saveRepoSettings, startBacklogTriage, - untilDeploymentReady, patchAppConfig, type HubApproval, type Membership, @@ -456,66 +455,25 @@ describe("repository policy config", () => { }); }); -describe("deployment not ready", () => { - function notReady() { - return new ApiError(409, "deployment_not_ready", "The workflow deployment is still starting."); - } - - function harness(answers: ApiError[]) { - const posted: string[] = []; - const waits: string[] = []; - const slept: number[] = []; +describe("backlog start", () => { + function transport(answer: ApiError | undefined, posted: string[]): Transport { function unsubscribe() {} - function subscribe() { - return unsubscribe; - } - const transport: Transport = { + return { async fetch(method, path) { posted.push(`${method} ${path}`); - const refused = answers.shift(); - if (refused) throw refused; + if (answer) throw answer; return { status: "started" } as never; }, - subscribe, + subscribe: () => unsubscribe, }; - async function request() { - return startBacklogTriage(transport, "tenant", "acme/widgets"); - } - function onWait() { - waits.push("waiting"); - } - async function sleep(ms: number) { - slept.push(ms); - } - function run() { - return untilDeploymentReady(request, onWait, sleep); - } - return { posted, waits, slept, run }; } - test("a trigger refused while the deployment is not ready is retried with backoff and announced once", async () => { - const { posted, waits, slept, run } = harness([notReady()]); - await run(); - expect(posted).toHaveLength(2); - expect(waits).toEqual(["waiting"]); - expect(slept).toEqual([1_000]); - }); - - test("another conflict is thrown without a retry", async () => { - const conflict = new ApiError(409, "repo_not_enabled", "Triage is disabled for this repository. Enable it first."); - const { posted, waits, run } = harness([conflict]); - await expect(run()).rejects.toBe(conflict); - expect(posted).toHaveLength(1); - expect(waits).toEqual([]); - }); - - test("once the budget is spent the last refusal is thrown", async () => { - const last = notReady(); - const { posted, waits, slept, run } = harness([notReady(), notReady(), notReady(), notReady(), last]); - await expect(run()).rejects.toBe(last); - expect(posted).toHaveLength(5); - expect(waits).toEqual(["waiting"]); - expect(slept).toEqual([1_000, 2_000, 4_000, 8_000]); + test("is one request: the hub holds the heads while the listener is starting, so the portal neither waits nor retries", async () => { + const posted: string[] = []; + await startBacklogTriage(transport(undefined, posted), "tenant", "acme/widgets"); + const stopped = new ApiError(503, "service_not_running", "Triage is not running."); + await expect(startBacklogTriage(transport(stopped, posted), "tenant", "acme/widgets")).rejects.toBe(stopped); + expect(posted).toEqual(["POST /api/integrations/github-triage/tenant/backlog", "POST /api/integrations/github-triage/tenant/backlog"]); }); }); diff --git a/apps/web/src/lib/hub-api.ts b/apps/web/src/lib/hub-api.ts index 2520fa320..90478c2ea 100644 --- a/apps/web/src/lib/hub-api.ts +++ b/apps/web/src/lib/hub-api.ts @@ -747,36 +747,6 @@ export function currentDeployment(deployments: readonly DeployedWorkflow[], work return deployments.find((deployment) => deployment.workflow === workflow && !deployment.cancelling); } -export const WAITING_FOR_WORKFLOW = "Waiting for the workflow to be ready…"; - -// A deployment answers 409 `deployment_not_ready` until it records its credential resolution, seconds after it is deployed. -const NOT_READY_BACKOFF_MS = [1_000, 2_000, 4_000, 8_000]; - -function isDeploymentNotReady(cause: unknown): boolean { - return cause instanceof ApiError && cause.status === 409 && cause.code === "deployment_not_ready"; -} - -function pause(ms: number): Promise { - return new Promise((resolve) => setTimeout(resolve, ms)); -} - -export async function untilDeploymentReady( - attempt: () => Promise, - onWait: () => void, - sleep: (ms: number) => Promise = pause, -): Promise { - for (const [index, ms] of NOT_READY_BACKOFF_MS.entries()) { - try { - return await attempt(); - } catch (cause) { - if (!isDeploymentNotReady(cause)) throw cause; - } - if (index === 0) onWait(); - await sleep(ms); - } - return attempt(); -} - export async function startBacklogTriage(transport: Transport, tenantId: string, repo: string): Promise { const clean = validateRepo(repo); BacklogStarted.assert(await transport.fetch("POST", `/api/integrations/github-triage/${enc(requireTenantId(tenantId))}/backlog`, { repo: clean })); diff --git a/apps/web/src/lib/portal.tsx b/apps/web/src/lib/portal.tsx index 4f91db52c..c3c08c2b4 100644 --- a/apps/web/src/lib/portal.tsx +++ b/apps/web/src/lib/portal.tsx @@ -21,8 +21,6 @@ import { startBacklogTriage, githubPrAction, triagePullRequest as requestPullRequestTriage, - untilDeploymentReady, - WAITING_FOR_WORKFLOW, type InvitedAccess, type PrGithubWriteInput, type PrGithubWriteResult, @@ -246,22 +244,14 @@ export function PortalProvider({ children }: { children: ReactNode }) { [notify, refresh, requireSnapshot], ); - const waitForWorkflow = useCallback(function waitForWorkflow() { - notify(WAITING_FOR_WORKFLOW); - }, [notify]); - const runBacklog = useCallback( async function runBacklog(repo: string, message = "Backlog triage started.") { const current = requireSnapshot(); - const transport = createHubTransport(); - async function start() { - return startBacklogTriage(transport, current.workspace.tenantId, repo); - } - await untilDeploymentReady(start, waitForWorkflow); + await startBacklogTriage(createHubTransport(), current.workspace.tenantId, repo); notify(message); refresh(); }, - [notify, refresh, requireSnapshot, waitForWorkflow], + [notify, refresh, requireSnapshot], ); const triagePullRequest = useCallback( diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9c1714a76..a9354bec7 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -357,7 +357,16 @@ any other tenant or an unknown sender. mail a live manager did not take fails as usual. Before recording a pull request's event, the intake asks stock's router whether it routes the listener, when a pull request named has no manager up, so mail the - listener cannot take records nothing. The reconciler queues an unrouted + listener cannot take records nothing. A held Do's approval is the + exception: it is recorded all the same, since nothing a person did is + dropped and no row holds it, and the manager the listener later starts for + the head runs it. A request a person makes while the listener takes no + mail answers 202 and its heads are held the same way: Triage again holds + its head, backlog triage holds each open pull request's head, drafts left + out, and a held Do's approval holds the head it was made on. Each starts a + pass at once. The held head is triaged again when its mail goes out, on + the same head, so the judge's verdict for it is reused and the verdict + comment is replaced in place. The reconciler queues an unrouted head on its first pass with a listener up, without backoff, and those tries add nothing to the head's attempts. A coalescer mail that reached no listener starts a pass at once, whose listener check rolls a workspace @@ -1195,7 +1204,8 @@ read it, and passes the rest of the request to stock. nothing: Triage again, Review again, a held Do's approval and backlog triage answer 503 `service_not_running`, and a webhook's head fails, each saying Triage is not running on that host and that Settings → Triage shows - why. + why. While the host's listener is starting or being replaced, Review again + answers the same 503; the others are held as above. - Webhook replay protection uses the `@corbits/webhooks` table and a delivery-id cache in `HUB_DATA_DIR`. diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index 42fe8bf2f..24ac96b34 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -99,7 +99,7 @@ Every repository has its own page under **Repositories** in the menu under your ### Turning triage on -The switch labeled **Triage pull requests in this repository** is the master control. While it is off, Triage ignores the repository entirely and the rest of the page is grayed out. You can turn it on before the policy has any rule: the page then says **Triage is on. It starts once the policy has a rule.** and nothing is checked until it does. Once the policy has a rule, Triage checks every open pull request, and does so again each time a change to the policy file reaches the default branch. From then on it reacts to each new pull request event as it happens. On GitHub, the link **Triage open pull requests again** re-checks everything that is currently open, which is useful after you change the policy. When the run finishes, each pull request's new result replaces its old one on Triage. Drafts are skipped until they are marked ready for review; one without a result shows **Waiting: draft** and **Draft: Not evaluated** instead of **Not triaged yet** and **Not evaluated**. +The switch labeled **Triage pull requests in this repository** is the master control. While it is off, Triage ignores the repository entirely and the rest of the page is grayed out. You can turn it on before the policy has any rule: the page then says **Triage is on. It starts once the policy has a rule.** and nothing is checked until it does. Once the policy has a rule, Triage checks every open pull request, and does so again each time a change to the policy file reaches the default branch. From then on it reacts to each new pull request event as it happens. On GitHub, the link **Triage open pull requests again** re-checks everything that is currently open, which is useful after you change the policy. If Triage is starting or being updated at that moment, the open pull requests are checked as soon as it is running; there is nothing to click again. When the run finishes, each pull request's new result replaces its old one on Triage. Drafts are skipped until they are marked ready for review; one without a result shows **Waiting: draft** and **Draft: Not evaluated** instead of **Not triaged yet** and **Not evaluated**. When the policy has judgement rules and the workspace has no decision model, the page says **This policy needs a model before its judgement rules can run.** **Add one in Settings** opens Settings → Triage, where you save it. A policy whose rules all run without a model shows no such line, but Triage still does not start on any repository until a model is saved. diff --git a/docs/SELF_HOST.md b/docs/SELF_HOST.md index 5d4b62202..a44ff63ca 100644 --- a/docs/SELF_HOST.md +++ b/docs/SELF_HOST.md @@ -448,6 +448,5 @@ any roll that failed. - Run one hub replica: its triage queue lives in memory. - Deleting a principal that still owns a credential is refused (Interchange migration 0101). -- A trigger right after a deploy or restart can answer `409 - deployment_not_ready` until the deployment has resolved its credentials; - retry it. +- A code review requested while the listener is starting or being replaced + answers `503 service_not_running`; retry it once the listener is up.