diff --git a/src/github/webhook-coalesce.ts b/src/github/webhook-coalesce.ts index de424974b..1eb2d793a 100644 --- a/src/github/webhook-coalesce.ts +++ b/src/github/webhook-coalesce.ts @@ -40,15 +40,39 @@ const PUSH_COALESCE_ACTION = "synchronize"; export const PUSH_COALESCE_QUIET_WINDOW_SECONDS = 45; /** - * The delay to enqueue this delivery with. Non-zero ONLY for a push, whose key above is built to coalesce. + * A CI-completion burst's window (#10127). Same 45s as a push, and for the same structural reason: a key with no + * window coalesces nothing. Deliberately under CI_COALESCE_WINDOW_SECONDS (60s, processors.ts) so the downstream + * per-PR re-review throttle stays the outer bound rather than being pre-empted by this one. + */ +const CI_COMPLETION_QUIET_WINDOW_SECONDS = 45; + +/** + * Label and review-surface bursts (#10127). Shorter than the CI window because both carry a human in the loop -- + * a maintainer removing a hold label expects the gate to notice promptly -- while still being long enough to + * collapse a same-batch burst, which is what these actually produce (the Orb's relay drains on a fixed interval, + * so a batch's events arrive within milliseconds of each other). + */ +const SURFACE_COALESCE_QUIET_WINDOW_SECONDS = 15; + +/** + * The delay to enqueue this delivery with. * * On a queue that coalesces (self-host pg/sqlite, which is where the ORB runs) this is what CREATES the window: - * without a delay the first push is claimed immediately and later pushes find no pending row to merge into. On - * Cloudflare Queues, which has no job_key coalescing, it is a plain 45s deferral of push-triggered reviews and - * nothing more -- correct, just not a saving. + * without a delay the first delivery is claimed immediately and later siblings find no pending row to merge into. + * On Cloudflare Queues, which has no job_key coalescing, it is a plain deferral and nothing more -- correct, just + * not a saving. + * + * #10127: this used to return non-zero for a push and ZERO for everything else, which made every other key + * `githubWebhookCoalesceKey` computes inert -- they were derived, attached to the job, and could never merge into + * anything. The Orb showed the cost: 1,727 `check_suite.completed` deliveries in a day and a half, and 2.98 + * decision records per distinct head SHA, each repeat buying a full prologue and a fresh gate pass over state + * that had not changed. + * + * Derived from the SAME plan as the key, so a family can no longer have one without the other. That coupling is + * the actual fix; the numbers are just tuning. */ export function githubWebhookCoalesceDelaySeconds(eventName: string, payload: GitHubWebhookPayload): number { - return eventName === "pull_request" && payload.action === PUSH_COALESCE_ACTION ? PUSH_COALESCE_QUIET_WINDOW_SECONDS : 0; + return planGithubWebhookCoalesce(eventName, payload)?.quietWindowSeconds ?? 0; } // #selfhost-backlog-convergence: every "labeled"/"unlabeled" delivery re-syncs the PR row @@ -72,10 +96,24 @@ const REVIEW_SURFACE_ACTIONS_BY_EVENT: Record> = { pull_request_review_thread: new Set(["resolved", "unresolved"]), }; +/** One coalescable family: the job key siblings merge on, and the quiet window that lets them. + * + * #10127: these two used to be computed by separate functions and drifted immediately -- four of the five + * families had a key and no window, so they never coalesced. Returning both together makes that state + * unrepresentable: adding a family means naming its window, because the type demands one. */ +type WebhookCoalescePlan = { key: string; quietWindowSeconds: number }; + export function githubWebhookCoalesceKey( eventName: string, payload: GitHubWebhookPayload, ): string | null { + return planGithubWebhookCoalesce(eventName, payload)?.key ?? null; +} + +function planGithubWebhookCoalesce( + eventName: string, + payload: GitHubWebhookPayload, +): WebhookCoalescePlan | null { const action = typeof payload.action === "string" ? payload.action : ""; const repo = normalizedRepo(payload.repository?.full_name); @@ -95,7 +133,13 @@ export function githubWebhookCoalesceKey( .filter((value): value is number => value !== null) .sort((a, b) => a - b) .join(","); - return `github-webhook:ci-completed:${repo}@${headSha}${pullNumbers ? `#${pullNumbers}` : ""}`; + // Interchangeable by construction: whichever delivery wins the race re-fetches the SAME already-settled CI + // state, so collapsing a suite-completion burst loses nothing. The highest-volume family on the Orb by an + // order of magnitude, and the reason #10127 exists. + return { + key: `github-webhook:ci-completed:${repo}@${headSha}${pullNumbers ? `#${pullNumbers}` : ""}`, + quietWindowSeconds: CI_COMPLETION_QUIET_WINDOW_SECONDS, + }; } if (eventName === "pull_request" && isCoalescablePullRequestAction(action)) { const pr = @@ -103,22 +147,42 @@ export function githubWebhookCoalesceKey( normalizedNumber((payload as { number?: unknown }).number); if (pr === null) return null; // #9479: a push is keyed on the PR alone, so the NEXT push collapses into it. See PUSH_COALESCE_ACTION. - if (action === PUSH_COALESCE_ACTION) return `github-webhook:pr-push:${repo}#${pr}`; + if (action === PUSH_COALESCE_ACTION) { + return { key: `github-webhook:pr-push:${repo}#${pr}`, quietWindowSeconds: PUSH_COALESCE_QUIET_WINDOW_SECONDS }; + } const headSha = normalizedSha(payload.pull_request?.head?.sha); - return `github-webhook:pr-refresh:${repo}#${pr}${headSha ? `@${headSha}` : ""}`; + // Window deliberately ZERO, unlike every other family here (#10127). These are LIFECYCLE transitions -- + // opened, reopened, ready_for_review -- each of which happens once and starts the clock a contributor is + // waiting on. They are keyed so a genuine same-instant duplicate still collapses, but delaying a PR's first + // review to save a redelivery would trade a real cost (time-to-first-verdict) for an imaginary one: at 203 + // `opened` deliveries in a day and a half these are not a burst source in the first place. + return { + key: `github-webhook:pr-refresh:${repo}#${pr}${headSha ? `@${headSha}` : ""}`, + quietWindowSeconds: 0, + }; } if (eventName === "pull_request" && COALESCABLE_PULL_REQUEST_LABEL_ACTIONS.has(action)) { const pr = normalizedNumber(payload.pull_request?.number) ?? normalizedNumber((payload as { number?: unknown }).number); - return pr !== null ? `github-webhook:pr-label:${repo}#${pr}` : null; + // The bot's OWN disposition-label writes come straight back as deliveries here + // (shouldProcessPullRequestPublicSurface treats a disposition label as a re-sync trigger), so a pass that + // adds a label and clears a stale sibling bought two full re-evaluations of state it had just written. + return pr !== null + ? { key: `github-webhook:pr-label:${repo}#${pr}`, quietWindowSeconds: SURFACE_COALESCE_QUIET_WINDOW_SECONDS } + : null; } const reviewSurfaceActions = REVIEW_SURFACE_ACTIONS_BY_EVENT[eventName]; if (reviewSurfaceActions?.has(action)) { const pr = normalizedNumber(payload.pull_request?.number); const headSha = normalizedSha(payload.pull_request?.head?.sha); + // Payload-interchangeable within a family (see REVIEW_SURFACE_ACTIONS_BY_EVENT), and a review submission + // routinely lands with several comment/thread deliveries in the same instant. return pr !== null - ? `github-webhook:${eventName}:${repo}#${pr}${headSha ? `@${headSha}` : ""}` + ? { + key: `github-webhook:${eventName}:${repo}#${pr}${headSha ? `@${headSha}` : ""}`, + quietWindowSeconds: SURFACE_COALESCE_QUIET_WINDOW_SECONDS, + } : null; } return null; diff --git a/test/unit/github-webhook-coalesce.test.ts b/test/unit/github-webhook-coalesce.test.ts index c5fb9106d..814887e6d 100644 --- a/test/unit/github-webhook-coalesce.test.ts +++ b/test/unit/github-webhook-coalesce.test.ts @@ -309,20 +309,61 @@ describe("force-push debounce (#9479)", () => { } }); - it("REGRESSION: only a push carries the trailing quiet window; every other delivery is enqueued immediately", () => { + it("a push carries its trailing quiet window", () => { expect(githubWebhookCoalesceDelaySeconds("pull_request", push(7, "a".repeat(40)))).toBe(PUSH_COALESCE_QUIET_WINDOW_SECONDS); expect(PUSH_COALESCE_QUIET_WINDOW_SECONDS).toBeGreaterThan(0); + }); - for (const action of ["opened", "reopened", "edited", "ready_for_review", "closed", "labeled"] as const) { - expect( - githubWebhookCoalesceDelaySeconds("pull_request", { - action, - repository: { full_name: "JSONbored/Loopover" }, - pull_request: { number: 7, head: { sha: "a".repeat(40) } }, - } as GitHubWebhookPayload), - ).toBe(0); + it("INVARIANT (#10127): every key-bearing delivery carries a window, except the lifecycle actions that opt out", () => { + // The bug this replaces: the delay function returned non-zero for a push and ZERO for everything else, so + // four of the five families computed a coalesce key that could never merge into anything -- a job with no + // window is claimed before a sibling can arrive. On the Orb that was 1,727 `check_suite.completed` + // deliveries in a day and a half and 2.98 decision records per head SHA. + // + // Asserted as "key implies window" over the real families rather than as a list of expected numbers, so a + // family added later cannot quietly reintroduce a keyed-but-inert delivery. + const keyed: Array<[string, GitHubWebhookPayload]> = [ + ["check_suite", { action: "completed", repository: { full_name: "JSONbored/Loopover" }, check_suite: { head_sha: "a".repeat(40), pull_requests: [{ number: 7 }] } } as unknown as GitHubWebhookPayload], + ["check_run", { action: "completed", repository: { full_name: "JSONbored/Loopover" }, check_run: { head_sha: "a".repeat(40), pull_requests: [{ number: 7 }] } } as unknown as GitHubWebhookPayload], + ["pull_request", push(7, "a".repeat(40))], + ["pull_request", { action: "labeled", repository: { full_name: "JSONbored/Loopover" }, pull_request: { number: 7 } } as GitHubWebhookPayload], + ["pull_request", { action: "unlabeled", repository: { full_name: "JSONbored/Loopover" }, pull_request: { number: 7 } } as GitHubWebhookPayload], + ["pull_request_review_comment", { action: "created", repository: { full_name: "JSONbored/Loopover" }, pull_request: { number: 7 } } as GitHubWebhookPayload], + ["pull_request_review_thread", { action: "resolved", repository: { full_name: "JSONbored/Loopover" }, pull_request: { number: 7 } } as GitHubWebhookPayload], + ]; + for (const [eventName, payload] of keyed) { + const label = `${eventName}:${String(payload.action)}`; + expect(githubWebhookCoalesceKey(eventName, payload), label).not.toBeNull(); + expect(githubWebhookCoalesceDelaySeconds(eventName, payload), label).toBeGreaterThan(0); + } + }); + + it("INVARIANT (#10127): a LIFECYCLE action is keyed but never delayed — time-to-first-verdict is not tradeable", () => { + // Deliberately the one family that keeps a zero window. `opened` / `reopened` / `ready_for_review` each + // happen once and start the clock a contributor is waiting on; they are also not a burst source (203 + // `opened` deliveries in the day and a half that produced 1,727 CI completions). The key stays so a genuine + // same-instant duplicate still collapses. + for (const action of ["opened", "reopened", "ready_for_review", "edited"] as const) { + const payload = { + action, + repository: { full_name: "JSONbored/Loopover" }, + pull_request: { number: 7, head: { sha: "a".repeat(40) } }, + } as GitHubWebhookPayload; + expect(githubWebhookCoalesceDelaySeconds("pull_request", payload), action).toBe(0); } + }); + + it("an unkeyed delivery is never delayed — a delay with no key is pure latency", () => { + // `closed` has no coalesce key (merge/close has its own non-coalesced handling), so it must not pick one up. + expect( + githubWebhookCoalesceDelaySeconds("pull_request", { + action: "closed", + repository: { full_name: "JSONbored/Loopover" }, + pull_request: { number: 7, head: { sha: "a".repeat(40) } }, + } as GitHubWebhookPayload), + ).toBe(0); // A same-named action on an unrelated event family must not pick up the delay either. expect(githubWebhookCoalesceDelaySeconds("check_suite", { action: "synchronize", repository: { full_name: "JSONbored/Loopover" } } as unknown as GitHubWebhookPayload)).toBe(0); + expect(githubWebhookCoalesceDelaySeconds("issues", { action: "labeled", repository: { full_name: "JSONbored/Loopover" } } as unknown as GitHubWebhookPayload)).toBe(0); }); });