Skip to content

Commit 1fb01bb

Browse files
committed
fix(hub,web): unlatch stalled banner on stale events and surface waiting PR (CL-12012)
1 parent 55dcfb1 commit 1fb01bb

7 files changed

Lines changed: 110 additions & 29 deletions

File tree

‎apps/hub/src/integrations/app.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -744,7 +744,7 @@ export function createIntegrationsApp(guards: IntegrationGuards, handlers: Integ
744744
`${WORKFLOW_VERSIONS_PATH}/:tenantId`,
745745
describeRoute({
746746
summary: "Package versions of the workspace's live workflow deployments and the hub's last roll of each host",
747-
description: "Each live deployment, newest first, with the workflow package it pinned and the packages its frozen closure carries. `package` is null for a deployment without a frozen closure; `source` is the asset and commit a source-format deployment runs, else null. `rollouts` holds each host the hub rolled onto its build's release since it started: when that roll ended and, when it failed, the error; a failed roll leaves the host's previous listener running. `stalled` is present while the workspace's pull requests have waited past the hub's alarm age (`TRIAGE_STALLED_AFTER_MS`) for a manager to take them: `since` is when the oldest still waiting was queued, `lastTakeAt` when a manager last took one.",
747+
description: "Each live deployment, newest first, with the workflow package it pinned and the packages its frozen closure carries. `package` is null for a deployment without a frozen closure; `source` is the asset and commit a source-format deployment runs, else null. `rollouts` holds each host the hub rolled onto its build's release since it started: when that roll ended and, when it failed, the error; a failed roll leaves the host's previous listener running. `stalled` is present while the workspace's pull requests have waited past the hub's alarm age (`TRIAGE_STALLED_AFTER_MS`) for a manager to take them: `since` is when the oldest still waiting was queued, `pull` which pull request it is, `count` how many wait, `reason` why if known, and `lastTakeAt` when a manager last took one.",
748748
tags: TAGS,
749749
responses: { 200: json("Live deployments and their versions", DeployedVersions), 401: failures[401], 403: failures[403] },
750750
}),

‎apps/hub/src/manager-state.ts‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -978,7 +978,7 @@ function pullOf(key: string): PullRequestRef {
978978
}
979979

980980
/** How long an overdue wake no manager has taken the claim since holds off the next, while its manager may still be reaching a sidecar. */
981-
const OVERDUE_UNSTARTED_GRACE_MS = 30 * 60_000;
981+
export const OVERDUE_UNSTARTED_GRACE_MS = 30 * 60_000;
982982

983983
/** Whether the claim's last overdue wake is still within its grace: a manager took the claim since, or none has yet. */
984984
function recentlyWoken(claim: Claim, now: Date): boolean {
@@ -1012,13 +1012,23 @@ export async function lastTakes(db: DB["db"]): Promise<{ tenantId: string; sourc
10121012
});
10131013
}
10141014

1015-
/** When the oldest pull request mail of the tenant's host that no manager has taken was recorded, among those recorded after `after`. */
1016-
export async function oldestPendingMail(db: DB["db"], tenantId: string, source: RepoSource, after: string | undefined): Promise<string | undefined> {
1015+
const EVENT_KEY = /^evt:[^:]+:([^#]+)#(\d+):/;
1016+
1017+
/** The oldest pull request mail of each pull request on the tenant's host that no manager has taken, among those recorded after `after`. */
1018+
export async function pendingMails(db: DB["db"], tenantId: string, source: RepoSource, after: string | undefined): Promise<{ repo: string; number: number; at: string }[]> {
10171019
const since = after === undefined ? undefined : sql`(${managerState.state}->>'receivedAt')::timestamptz > ${after}::timestamptz`;
10181020
const ofHost = like(managerState.key, `evt:${source}:%`);
1019-
const [row] = await db.select({ at: sql<string | null>`min(${managerState.state}->>'receivedAt')` }).from(managerState)
1021+
const rows = await db.select({ key: managerState.key, at: sql<string>`${managerState.state}->>'receivedAt'` }).from(managerState)
10201022
.where(and(eq(managerState.tenantId, tenantId), sql`${managerState.kind} = 'event'`, sql`${managerState.state}->>'consumedBy' IS NULL`, sql`${managerState.state}->>'type' = 'webhook'`, ofHost, since));
1021-
return row?.at ?? undefined;
1023+
const oldest = new Map<string, { repo: string; number: number; at: string }>();
1024+
for (const row of rows) {
1025+
const [, repo, num] = EVENT_KEY.exec(row.key) ?? [];
1026+
if (repo === undefined || num === undefined) continue;
1027+
const pr = `${repo}#${num}`;
1028+
const prior = oldest.get(pr);
1029+
if (prior === undefined || Date.parse(row.at) < Date.parse(prior.at)) oldest.set(pr, { repo, number: Number(num), at: row.at });
1030+
}
1031+
return [...oldest.values()];
10221032
}
10231033

10241034
/** The tenant's pull requests whose claim holds a reviewer with no result. */

‎apps/hub/src/triage-liveness.postgres.test.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ test("a mail no manager takes raises the alarm once past the alarm age, and a ta
112112
await hub.check();
113113
await hub.check();
114114
expect(hub.alarms()).toEqual(["triage_stalled github"]);
115-
expect(hub.stalled()).toMatchObject([{ source: "github" }]);
115+
expect(hub.stalled()).toMatchObject([{ source: "github", pull: "acme/widgets#7", count: 1 }]);
116116

117117
await hub.took(PR);
118118
await hub.check();
@@ -159,6 +159,7 @@ test("a head whose mail finds no listener stays stalled while the reconciler ret
159159
hub.advance(STALLED_AFTER_MS + 60_000);
160160
await hub.check();
161161
expect(hub.alarms()).toEqual(["triage_stalled github"]);
162+
expect(hub.stalled()).toMatchObject([{ source: "github", pull: "acme/widgets#7", count: 1, reason: triageStopped("github") }]);
162163

163164
for (let retry = 0; retry < 3; retry++) {
164165
hub.advance(60 * 60_000);
@@ -265,3 +266,22 @@ test("a hub that sends no email says so once and still raises and clears the ala
265266
expect(hub.logged.filter((entry) => entry["msg"] === "triage_stall_not_emailed").map((entry) => entry["reason"])).toEqual(["SMTP_URL is not set"]);
266267
});
267268
}, 60_000);
269+
270+
test("a raised alarm clears without a newer take once its pull request is no longer waiting in the store, and ignores disabled repositories", async () => {
271+
await withDatabase(async (db) => {
272+
const queuedAt = new Date().toISOString();
273+
const store = memoryStore([{ number: 7, headSha: "h1", status: "queued", attempts: 2, firstSeenAt: queuedAt, queuedAt, updatedAt: queuedAt }]);
274+
const hub = stack(db, store);
275+
await mailOf(PR, "d1", db);
276+
await mailOf({ source: "github", repo: "acme/disabled", number: 1 }, "d2", db);
277+
278+
hub.advance(STALLED_AFTER_MS + 60_000);
279+
await hub.check();
280+
expect(hub.stalled()).toMatchObject([{ source: "github", pull: "acme/widgets#7", count: 1, reason: "run never started" }]);
281+
282+
await store.save(TENANT, RECORD.name, [{ number: 7, headSha: "h1", status: "failed", error: "run never started", attempts: 5, firstSeenAt: queuedAt, queuedAt, updatedAt: hub.now().toISOString() }]);
283+
await hub.check();
284+
expect(hub.alarms()).toEqual(["triage_stalled github", "triage_stall_cleared github"]);
285+
expect(hub.stalled()).toEqual([]);
286+
});
287+
}, 60_000);

‎apps/hub/src/triage-liveness.ts‎

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,12 @@ import { REPO_SOURCES, repoPolicy, type RepoSource } from "@corbits/triage-contr
1111
import { repoRecords, sourceOf, triageNs, triageStateKey } from "./github/tenant-config.js";
1212
import { undelivered } from "./github/triage-queue.js";
1313
import type { TriageStateStore } from "./github/triage-state-store.js";
14-
import { lastTakes, oldestPendingMail } from "./manager-state.js";
14+
import { lastTakes, OVERDUE_UNSTARTED_GRACE_MS, pendingMails } from "./manager-state.js";
1515

16-
/** A stalled host: since when its oldest waiting pull request waits, and when one of its managers last took one. */
17-
export type Stalled = { source: RepoSource; since: string; lastTakeAt?: string };
16+
/** A stalled host: since when its oldest waiting pull request waits, which pull request it is, how many wait, why if known, and when one of its managers last took one. */
17+
export type Stalled = { source: RepoSource; since: string; lastTakeAt?: string; pull?: string; count?: number; reason?: string };
18+
19+
type WaitingPull = { pull: string; at: number; reason?: string };
1820

1921
/** What the workspace's owners and admins are emailed when a host's alarm is raised, or cleared. */
2022
export type StallNotice = { to: string[]; workspace: string; source: RepoSource; since: string; cleared: boolean };
@@ -104,35 +106,67 @@ export function createTriageLiveness(deps: TriageLivenessDeps) {
104106
* When each waiting pull request of each host began to wait: a mail or queued head since the host's last take, and
105107
* any head whose mail did not go out, which no take can have picked up, until a run of it is seen.
106108
*/
107-
async function waiting(tenant: LivenessTenant): Promise<Map<RepoSource, number[]>> {
108-
const byHost = new Map<RepoSource, number[]>();
109+
async function waiting(tenant: LivenessTenant): Promise<Map<RepoSource, WaitingPull[]>> {
110+
const byHost = new Map<RepoSource, WaitingPull[]>(REPO_SOURCES.map((source) => [source, []]));
111+
const mails = new Map<RepoSource, { repo: string; number: number; at: string }[]>();
109112
for (const source of REPO_SOURCES) {
110-
const mail = await oldestPendingMail(deps.db, tenant.id, source, deps.takes.last(tenant.id, source));
111-
byHost.set(source, mail === undefined ? [] : [Date.parse(mail)]);
113+
mails.set(source, await pendingMails(deps.db, tenant.id, source, deps.takes.last(tenant.id, source)));
112114
}
115+
const now = deps.now().getTime();
113116
for (const record of repoRecords(triageNs(tenant.config))) {
114117
if (!record.connected || !repoPolicy(record).enabled) continue;
115118
const source = sourceOf(record);
116119
const taken = deps.takes.last(tenant.id, source);
117-
const { rows } = await deps.store.load(tenant.id, triageStateKey(record));
120+
const { rows, version } = await deps.store.load(tenant.id, triageStateKey(record));
121+
const queued = new Set<number>();
118122
for (const row of rows) {
119123
const since = row.undeliveredSince ?? row.queuedAt;
120124
if (since === undefined || (row.status !== "queued" && !undelivered(row, source))) continue;
125+
queued.add(row.number);
121126
const unsent = row.undeliveredSince !== undefined || undelivered(row, source);
122-
if (unsent || taken === undefined || Date.parse(since) > Date.parse(taken)) byHost.get(source)?.push(Date.parse(since));
127+
if (unsent || taken === undefined || Date.parse(since) > Date.parse(taken)) {
128+
const reason = row.error ?? (row.attempts > 1 && row.runId === undefined ? "run never started" : undefined);
129+
byHost.get(source)?.push({ pull: `${record.name}#${row.number}`, at: Date.parse(since), ...(reason !== undefined && { reason }) });
130+
}
131+
}
132+
for (const mail of mails.get(source) ?? []) {
133+
if (mail.repo !== record.name || queued.has(mail.number)) continue;
134+
const at = Date.parse(mail.at);
135+
if (now - at > OVERDUE_UNSTARTED_GRACE_MS) continue;
136+
const ofPull = rows.filter((each) => each.number === mail.number);
137+
const unstored = ofPull.length === 0 && version === null && rows.length === 0;
138+
const newer = ofPull.length > 0 && ofPull.every((each) => at > Date.parse(each.updatedAt));
139+
if (unstored || newer) byHost.get(source)?.push({ pull: `${record.name}#${mail.number}`, at });
123140
}
124141
}
125142
return byHost;
126143
}
127144

128-
/** Raises the host's alarm once it stalls, and clears it only once one of its managers opened a pass since and none waits. */
129-
async function settle(tenantId: string, source: RepoSource, times: readonly number[]): Promise<void> {
145+
function stalledOf(source: RepoSource, pulls: readonly WaitingPull[], taken: string | undefined): Stalled | undefined {
146+
const now = deps.now().getTime();
147+
const byPull = new Map<string, WaitingPull>();
148+
for (const item of pulls) {
149+
if (now - item.at < deps.stalledAfterMs) continue;
150+
const prior = byPull.get(item.pull);
151+
if (prior === undefined || item.at < prior.at) byPull.set(item.pull, item);
152+
}
153+
const overdue = [...byPull.values()].sort((a, b) => a.at - b.at || a.pull.localeCompare(b.pull));
154+
const oldest = overdue[0];
155+
if (oldest === undefined) return undefined;
156+
return {
157+
source,
158+
since: new Date(oldest.at).toISOString(),
159+
...(taken !== undefined && { lastTakeAt: taken }),
160+
pull: oldest.pull,
161+
count: overdue.length,
162+
...(oldest.reason !== undefined && { reason: oldest.reason }),
163+
};
164+
}
165+
166+
/** Raises the host's alarm once it stalls, and clears it once nothing on the host waits past the alarm age. */
167+
async function settle(tenantId: string, source: RepoSource, pulls: readonly WaitingPull[]): Promise<void> {
130168
const key = hostKey(tenantId, source);
131-
const taken = deps.takes.last(tenantId, source);
132-
const oldest = times.length === 0 ? undefined : Math.min(...times);
133-
const stalled: Stalled | undefined = oldest === undefined || deps.now().getTime() - oldest < deps.stalledAfterMs
134-
? undefined
135-
: { source, since: new Date(oldest).toISOString(), ...(taken !== undefined && { lastTakeAt: taken }) };
169+
const stalled = stalledOf(source, pulls, deps.takes.last(tenantId, source));
136170
const raised = alarms.get(key);
137171
if (raised === undefined) {
138172
if (stalled === undefined) return;
@@ -141,7 +175,6 @@ export function createTriageLiveness(deps: TriageLivenessDeps) {
141175
await email(tenantId, stalled, false);
142176
return;
143177
}
144-
if (taken === undefined || Date.parse(taken) <= Date.parse(raised.since)) return;
145178
if (stalled !== undefined) {
146179
alarms.set(key, stalled);
147180
return;

‎apps/web/src/components/TriageStalledNotice.dom.test.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,12 @@ test("the workspace is told which host has pull requests waiting for Triage only
2020
expect(status?.textContent).toBe("");
2121

2222
const since = new Date("2026-10-27T10:42:00Z");
23+
const at = since.toLocaleTimeString([], { hour: "2-digit", minute: "2-digit" });
2324
act(() => root?.render(<MemoryRouter><TriageStalledNotice stalled={[{ source: "bitbucket-datacenter", since: since.toISOString() }]} /></MemoryRouter>));
2425
expect(document.querySelector("[role=status]")).toBe(status);
25-
expect(status?.textContent).toContain(`Pull requests on Bitbucket Data Center have waited since ${since.toLocaleTimeString([], { hour: "2-digit", minute: "2-digit" })} for Triage to start.`);
26+
expect(status?.textContent).toContain(`Pull requests on Bitbucket Data Center have waited since ${at} for Triage to start.`);
2627
expect(document.querySelector("a")?.getAttribute("href")).toBe("/settings/triage");
28+
29+
act(() => root?.render(<MemoryRouter><TriageStalledNotice stalled={[{ source: "github", since: since.toISOString(), pull: "acme/widgets#7", count: 3, reason: "run never started" }]} /></MemoryRouter>));
30+
expect(status?.textContent).toContain(`acme/widgets#7 and 2 other pull requests on GitHub have waited since ${at} for Triage to start: run never started.`);
2731
});

‎apps/web/src/components/TriageStalledNotice.tsx‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,26 @@
11
import { Link } from "react-router-dom";
2+
import { counted } from "@corbits/common";
23
import type { DeployedVersions } from "@corbits/triage-contracts";
34
import { hostFullName } from "../lib/pull-link.ts";
45
import { useTriageStalled } from "../lib/tenant-entities.ts";
56

67
type Stalled = NonNullable<DeployedVersions["stalled"]>[number];
78

8-
export function stalledText({ source, since }: Stalled): string {
9-
return `Pull requests on ${hostFullName(source)} have waited since ${new Date(since).toLocaleTimeString([], { hour: "2-digit", minute: "2-digit" })} for Triage to start.`;
9+
function waitingSubject({ source, pull, count }: Stalled): string {
10+
const host = hostFullName(source);
11+
if (pull === undefined) return `Pull requests on ${host} have`;
12+
if (count !== undefined && count > 1) return `${pull} and ${counted(count - 1, "other pull request", "other pull requests")} on ${host} have`;
13+
return `${pull} on ${host} has`;
14+
}
15+
16+
function reasonTail(reason: string | undefined): string {
17+
if (reason === undefined) return ".";
18+
return reason.endsWith(".") ? `: ${reason}` : `: ${reason}.`;
19+
}
20+
21+
export function stalledText(stalled: Stalled): string {
22+
const at = new Date(stalled.since).toLocaleTimeString([], { hour: "2-digit", minute: "2-digit" });
23+
return `${waitingSubject(stalled)} waited since ${at} for Triage to start${reasonTail(stalled.reason)}`;
1024
}
1125

1226
/** Tells everyone in the workspace while its pull requests on a host wait for Triage to start. */

‎packages/triage-contracts/src/hub-responses.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,8 +226,8 @@ export const DeployedVersions = type({
226226
* because it was full (`overflow`).
227227
*/
228228
sidecar: type({ active: "number", waiting: "number", cap: "number", overflow: "number" }),
229-
/** Each host whose pull requests have waited for a manager past the hub's alarm age: since when the oldest waits, and when a manager of that host last took one. */
230-
"stalled?": type({ source: type.enumerated(...REPO_SOURCES), since: "string", "lastTakeAt?": "string" }).array(),
229+
/** Each host whose pull requests have waited for a manager past the hub's alarm age: since when the oldest waits, which pull request it is, how many wait, why if known, and when a manager of that host last took one. */
230+
"stalled?": type({ source: type.enumerated(...REPO_SOURCES), since: "string", "lastTakeAt?": "string", "pull?": "string", "count?": "number", "reason?": "string" }).array(),
231231
});
232232
export type DeployedVersions = typeof DeployedVersions.infer;
233233

0 commit comments

Comments
 (0)