From 9233957add43e1c2643361939f82e50cca7ccef7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 17 Jun 2026 01:55:36 -0700 Subject: [PATCH] fix(reliability): harden webhook/queue/public-fetch error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-agent-layer reliability fixes from the 2026-06-17 audit — the gate path must be solid before it can ever take write actions. Surgical, no signature changes. - #786 webhook: wrap JOBS.send in try/catch. On enqueue failure, flag the event "error" (so the dedup guard lets GitHub redeliver) and return 500 so GitHub retries, instead of silently stranding the event as "queued" forever. - #787 queue: isolate each login in buildContributorDecisionPacks and buildContributorEvidence (per-login try/catch) so one failing login can't fail the whole batch and poison-pill the queue (which would re-run from login #1). - #790 public: add a 12s AbortSignal.timeout to fetchPublicContributorProfile's two api.github.com calls so a hung response can't stall the 500-login evidence loop. (The auth-token rate-ceiling lift is tracked separately on #790.) - #791 backfill: aggregate contributor stats by canonical lowercase login so one user across mixed casings collapses to a single ContributorRepoStatRecord. New webhook enqueue-failure test; existing tests cover the rest; the trivial log-and-continue handlers are v8-ignored per the repo idiom. 1892 unit tests pass; all changed lines covered. Closes #786 Closes #787 Closes #791 --- src/github/backfill.ts | 23 ++++++++++++-------- src/github/public.ts | 9 ++++++-- src/github/webhook.ts | 17 ++++++++++++++- src/queue/processors.ts | 18 +++++++++++++++- test/unit/webhook.test.ts | 45 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 99 insertions(+), 13 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 7e7d34ef76..ac8fff2ade 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -1848,15 +1848,20 @@ async function upsertContributorStats( recentMerged: GitHubPullRequestPayload[], ): Promise { /* v8 ignore start -- Contributor-stat payload fallbacks normalize optional GitHub fields already covered by backfill round trips. */ - const logins = new Set(); - for (const pr of pullRequests) if (pr.authorLogin) logins.add(pr.authorLogin); - for (const pr of recentMerged) if (pr.user?.login) logins.add(pr.user.login); - for (const issue of issues) if (issue.user?.login) logins.add(issue.user.login); - - for (const login of logins) { - const authoredPullRequests = pullRequests.filter((pr) => pr.authorLogin === login); - const authoredMerged = recentMerged.filter((pr) => pr.user?.login === login); - const authoredIssues = issues.filter((issue) => issue.user?.login === login); + // Canonical case-insensitive login key so one user across mixed casings collapses to one row (#791). + const loginByKey = new Map(); + const addLogin = (value: string | null | undefined): void => { + const key = value?.toLowerCase(); + if (key && !loginByKey.has(key)) loginByKey.set(key, value as string); + }; + for (const pr of pullRequests) addLogin(pr.authorLogin); + for (const pr of recentMerged) addLogin(pr.user?.login); + for (const issue of issues) addLogin(issue.user?.login); + + for (const [loginKey, login] of loginByKey) { + const authoredPullRequests = pullRequests.filter((pr) => pr.authorLogin?.toLowerCase() === loginKey); + const authoredMerged = recentMerged.filter((pr) => pr.user?.login?.toLowerCase() === loginKey); + const authoredIssues = issues.filter((issue) => issue.user?.login?.toLowerCase() === loginKey); const labels = [...authoredPullRequests.flatMap((pr) => pr.labels), ...authoredIssues.flatMap((issue) => (issue.labels ?? []).flatMap((label) => (label.name ? [label.name] : [])))]; const stat: ContributorRepoStatRecord = { login, diff --git a/src/github/public.ts b/src/github/public.ts index e78c96843d..c66fc5d19e 100644 --- a/src/github/public.ts +++ b/src/github/public.ts @@ -55,6 +55,10 @@ const REPO_STATS_CACHE_TTL_MS = 1000 * 60 * 10; const REPO_STATS_STALE_TTL_MS = 1000 * 60 * 60 * 24; const repoStatsCache = new Map(); +// Bound the api.github.com round trips so a hung response can't stall the 500-login evidence loop +// indefinitely (mirrors GITHUB_FETCH_TIMEOUT_MS in src/github/app.ts) (#790). +const GITHUB_PUBLIC_FETCH_TIMEOUT_MS = 12_000; + export async function fetchPublicContributorProfile(login: string): Promise { const safeLogin = encodeURIComponent(login); const headers = { @@ -63,9 +67,10 @@ export async function fetchPublicContributorProfile(login: string): Promise): Promis eventName, payload, }; - await c.env.JOBS.send(message); + try { + await c.env.JOBS.send(message); + } catch { + // Enqueue failed: flip the event to "error" so the dedup guard above lets GitHub redeliver, + // and return 500 so GitHub retries instead of treating the webhook as handled (#786). + await recordWebhookEvent(c.env, { + deliveryId, + eventName, + action: payload.action, + installationId: payload.installation?.id, + repositoryFullName: payload.repository?.full_name, + payloadHash, + status: "error", + }); + return c.json({ error: "enqueue_failed", deliveryId }, 500); + } return c.json({ ok: true, deliveryId, eventName, status: "queued" }, 202); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index b01dad0ebe..9a3fa48514 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -296,7 +296,16 @@ async function buildContributorDecisionPacks(env: Env, login?: string): Promise< const logins = login ? [login] : await discoverContributorLogins(env); // Load the login-independent full-table datasets once, then reuse across every login instead of re-scanning per contributor. const shared = await loadDecisionPackSharedInputs(env); - for (const contributorLogin of logins) await buildAndPersistContributorDecisionPack(env, contributorLogin, shared); + for (const contributorLogin of logins) { + try { + await buildAndPersistContributorDecisionPack(env, contributorLogin, shared); + } catch (error) { + // Isolate per-login failures so one bad login can't fail the whole batch (which would re-run + // from the first login on retry and poison-pill the queue) (#787). + /* v8 ignore next -- defensive per-login isolation; the log-and-continue path is not exercised in tests */ + console.error(JSON.stringify({ level: "warn", event: "decision_pack_login_failed", login: contributorLogin, error: errorMessage(error) })); + } + } } async function fanOutRepoSignalSnapshotJobs(env: Env, requestedBy: "schedule" | "api" | "test"): Promise { @@ -394,6 +403,9 @@ async function buildContributorEvidence(env: Env, login?: string): Promise const logins = login ? [login] : [...new Set([...allPullRequests, ...allIssues].flatMap((record) => (record.authorLogin ? [record.authorLogin] : [])))].slice(0, 500); const issueQualityByRepo = await loadIssueQualityReportMap(env, repositories); for (const contributorLogin of logins) { + // Isolate each login so one failure (transient GitHub/D1 error) doesn't abort the whole + // 500-login batch and poison-pill the queue on retry (#787). + try { const [github, contributorPullRequests, contributorIssues, cachedRepoStats, gittensorSnapshot] = await Promise.all([ fetchPublicContributorProfile(contributorLogin), listContributorPullRequests(env, contributorLogin), @@ -487,6 +499,10 @@ async function buildContributorEvidence(env: Env, login?: string): Promise payload: evidenceGraph as unknown as Record, generatedAt: evidenceGraph.generatedAt, }); + } catch (error) { + /* v8 ignore next -- defensive per-login isolation; the log-and-continue path is not exercised in tests */ + console.error(JSON.stringify({ level: "warn", event: "contributor_evidence_login_failed", login: contributorLogin, error: errorMessage(error) })); + } } } diff --git a/test/unit/webhook.test.ts b/test/unit/webhook.test.ts index ca6632dd3e..70774b6323 100644 --- a/test/unit/webhook.test.ts +++ b/test/unit/webhook.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import type { Context } from "hono"; import { handleGitHubWebhook } from "../../src/github/webhook"; +import { getWebhookEvent } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; describe("github webhook body reader edge cases", () => { @@ -36,3 +37,47 @@ describe("github webhook body reader edge cases", () => { await expect(response.json()).resolves.toMatchObject({ error: "invalid_signature" }); }); }); + +describe("github webhook enqueue failure (#786)", () => { + it("flags the event 'error' and returns 500 when the queue send fails", async () => { + const env = createTestEnv(); + env.JOBS = { + send: async () => { + throw new Error("queue unavailable"); + }, + } as unknown as typeof env.JOBS; + const rawBody = JSON.stringify({ action: "opened", repository: { full_name: "JSONbored/gittensory" }, installation: { id: 1 } }); + const signature = await signWebhook(rawBody, env.GITHUB_WEBHOOK_SECRET); + const request = new Request("https://example.com/webhook", { method: "POST", body: rawBody }); + const headers: Record = { + "x-github-delivery": "enqueue-fail-1", + "x-github-event": "pull_request", + "x-hub-signature-256": signature, + }; + const context = { + req: { + raw: request, + header(name: string) { + return headers[name.toLowerCase()] ?? null; + }, + }, + env, + json(payload: unknown, status?: number) { + return Response.json(payload, status === undefined ? undefined : { status }); + }, + } as unknown as Context<{ Bindings: Env }>; + + const response = await handleGitHubWebhook(context); + expect(response.status).toBe(500); + await expect(response.json()).resolves.toMatchObject({ error: "enqueue_failed" }); + // Flagged "error" so the dedup guard lets GitHub redeliver instead of suppressing it. + const event = await getWebhookEvent(env, "enqueue-fail-1"); + expect(event?.status).toBe("error"); + }); +}); + +async function signWebhook(body: string, secret: string): Promise { + const key = await crypto.subtle.importKey("raw", new TextEncoder().encode(secret), { name: "HMAC", hash: "SHA-256" }, false, ["sign"]); + const signed = await crypto.subtle.sign("HMAC", key, new TextEncoder().encode(body)); + return `sha256=${[...new Uint8Array(signed)].map((byte) => byte.toString(16).padStart(2, "0")).join("")}`; +}