From fc2945177baef9ef3a7cc6af22be8666ea32be51 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 5 Jul 2026 00:02:15 -0700 Subject: [PATCH] fix(queue): retry PR public-surface publish on transient GitHub failures A rate-limit blip, GitHub 5xx, or momentary token issue during the comment/check-run/label publish attempts was swallowed and only audited, so the job still completed "successfully" even though the review never reached the PR, with nothing left to retry it. Classify each publish failure as transient or permanent at catch time (errorMessage() already discards the status code needed to reclassify later), and when nothing published at all and at least one failure was transient, throw a RetryableJobError so the queue retries the whole job. A permanent 4xx keeps today's swallow-and-audit behavior. --- src/queue/processors.ts | 44 ++++++++++- test/unit/queue.test.ts | 164 +++++++++++++++++++++++++++++++++++----- 2 files changed, 186 insertions(+), 22 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 31b8ea45ac..9478309761 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -129,6 +129,7 @@ import { getGithubUserCreatedAt, getInstallationId, getRepositoryCollaboratorPermission, + githubErrorStatus, GITTENSORY_GATE_CHECK_NAME, isGitHubRateLimitedError, isForeignAppInstallation, @@ -5716,8 +5717,21 @@ type PublicSurfaceOutput = "comment" | "label" | "check_run" | "gate_check_run"; type PublicSurfaceOutputFailure = { output: PublicSurfaceOutput; error: string; + // Captured AT CATCH TIME, not reconstructed later: errorMessage() already reduces `error` to a plain string by + // the time it lands here, discarding the `.status`/`.response` shape isGitHubTransientPublishError needs. A + // permission_missing check-run push (no live error object) is correctly "false" via the default below. + transient: boolean; }; +// A revoked/expired installation token mid-request, a GitHub 5xx, or a rate-limit blip are all momentary — the +// job should retry, not silently drop a computed review. A 4xx auth/permission/not-found error is not: retrying +// forever would never converge, so it keeps today's swallow-and-audit behavior. +function isGitHubTransientPublishError(error: unknown): boolean { + if (isGitHubRateLimitedError(error)) return true; + const status = githubErrorStatus(error); + return status !== null && status >= 500; +} + // Intentionally writes to check_summaries only, not audit_events (#2908): this fires on every successful gate- // check publish, which is a very high-frequency event (every review pass, potentially several times per PR as // it iterates) -- check_summaries is the purpose-built, already-queryable canonical record for "when was this @@ -6926,6 +6940,22 @@ export async function enrichOpenPullRequestsWithChangedFiles(env: Env, repoFullN }); } +// GITTENSORY-5: a transient publish failure (rate limit / GitHub 5xx / momentary token issue) used to be +// swallowed and only audited — the job still completed "successfully" from the queue's point of view, so a +// review that computed real output silently never reached the PR, with no retry. Extending RetryableJobError +// (same shape as RetryablePullRequestFreshnessUnavailableError / PrActuationLockContendedError above) makes the +// queue retry the whole job instead. Thrown only when NOTHING published at all (see finishPublicSurfacePublication) +// and at least one failure was transient — a permanent 4xx keeps today's swallow-and-audit behavior. +class RetryablePublicSurfacePublishFailedError extends RetryableJobError { + constructor(repoFullName: string, prNumber: number) { + super(`public-surface publish failed transiently for ${repoFullName}#${prNumber}; retrying`, { + retryAfterMs: 60_000, + retryKind: "public_surface_publish_transient", + }); + this.name = "RetryablePublicSurfacePublishFailedError"; + } +} + async function maybePublishPrPublicSurface( env: Env, installationId: number, @@ -7367,6 +7397,13 @@ async function maybePublishPrPublicSurface( head_sha: advisory.headSha, failedOutputs: failedOutputs.map((failure) => failure.output), }); + // At least one output failed for a reason that can plausibly clear on its own (rate limit / 5xx / momentary + // token issue) — retry the whole job instead of leaving the review permanently unposted. A mix of transient + // and permanent failures still retries: the permanent one re-fails identically next pass and re-audits, but + // the transient one gets the chance it needs, and nothing here is published twice (publishedOutputs is empty). + if (failedOutputs.some((failure) => failure.transient)) { + throw new RetryablePublicSurfacePublishFailedError(repoFullName, pr.number); + } } if (gateSurfaceIncomplete) { await recordAuditEvent(env, { @@ -8524,6 +8561,7 @@ async function maybePublishPrPublicSurface( failedOutputs.push({ output: "check_run", error: checkRunResult.warning, + transient: false, }); await recordAuditEvent(env, { eventType: "github_app.check_run_permission_missing", @@ -8539,7 +8577,7 @@ async function maybePublishPrPublicSurface( } } catch (error) { const message = errorMessage(error); - failedOutputs.push({ output: "check_run", error: message }); + failedOutputs.push({ output: "check_run", error: message, transient: isGitHubTransientPublishError(error) }); await recordPublicSurfaceOutputFailure( env, "check_run", @@ -8824,7 +8862,7 @@ async function maybePublishPrPublicSurface( incr("gittensory_reviews_published_total", { repo: repoFullName }); } catch (error) { const message = errorMessage(error); - failedOutputs.push({ output: "comment", error: message }); + failedOutputs.push({ output: "comment", error: message, transient: isGitHubTransientPublishError(error) }); await recordPublicSurfaceOutputFailure( env, "comment", @@ -8879,7 +8917,7 @@ async function maybePublishPrPublicSurface( publishedOutputs.push("label"); } catch (error) { const message = errorMessage(error); - failedOutputs.push({ output: "label", error: message }); + failedOutputs.push({ output: "label", error: message, transient: isGitHubTransientPublishError(error) }); await recordPublicSurfaceOutputFailure( env, "label", diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 63f3193ec2..c0f26b5412 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -14171,7 +14171,7 @@ describe("queue processors", () => { expect(audit?.detail).toMatch(/Checks: write permission is missing/i); }); - it("audits advisory context check publish failures without blocking webhook processing", async () => { + it("audits advisory context check publish failures AND retries the job (GitHub 5xx is transient, GITTENSORY-5)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( env, @@ -14212,7 +14212,7 @@ describe("queue processors", () => { pull_request: { number: 25, title: "Context check", state: "open", user: { login: "contributor" }, head: { sha: "context500" }, labels: [], body: "No issue needed." }, }, }), - ).resolves.toBeUndefined(); + ).rejects.toMatchObject({ retryKind: "public_surface_publish_transient" }); const outputFailure = await env.DB.prepare("select event_type, detail from audit_events where event_type = ?") .bind("github_app.pr_check_run_publish_failed") @@ -14224,7 +14224,9 @@ describe("queue processors", () => { .first<{ detail: string; metadata_json: string }>(); expect(aggregate).toMatchObject({ detail: "check_run" }); expect(aggregate?.metadata_json).toContain('"output":"check_run"'); - // The total publish failure (nothing reached the PR) escalates to Sentry at error level, not just the ledger. + expect(aggregate?.metadata_json).toContain('"transient":true'); + // The total publish failure (nothing reached the PR) escalates to Sentry at error level, not just the ledger — + // this still fires BEFORE the retryable throw, so the failure stays observable even though the job also retries. expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "publish", repo: "JSONbored/gittensory" })); captureSpy.mockRestore(); }); @@ -14357,7 +14359,7 @@ describe("queue processors", () => { expect(published?.metadata_json).toContain('"output":"comment"'); }); - it("records an aggregate public-surface failure when no configured output publishes", async () => { + it("records an aggregate public-surface failure when no configured output publishes (permanent failure, no retry)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( env, @@ -14384,29 +14386,150 @@ describe("queue processors", () => { if (url.includes("/users/oktofeesh1/repos")) return Response.json([]); if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.includes("/issues/31/comments") && method === "GET") return Response.json([]); - if (url.includes("/issues/31/comments") && method === "POST") return new Response("comment failed", { status: 503 }); + // A 403 with no rate-limit signal (permissions revoked, not a burst limit) is PERMANENT: retrying forever + // would never converge, so this must keep today's swallow-and-audit behavior, not throw a retryable error. + if (url.includes("/issues/31/comments") && method === "POST") return new Response(JSON.stringify({ message: "Resource not accessible by integration" }), { status: 403 }); return new Response("not found", { status: 404 }); }); - await processJob(env, { - type: "github-webhook", - deliveryId: "all-public-outputs-failed", - eventName: "pull_request", - payload: { - action: "opened", - installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, - repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, - pull_request: { number: 31, title: "Miner work", state: "open", user: { login: "oktofeesh1" }, labels: [], body: "Fixes #1" }, - }, - }); + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "all-public-outputs-failed", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, + pull_request: { number: 31, title: "Miner work", state: "open", user: { login: "oktofeesh1" }, labels: [], body: "Fixes #1" }, + }, + }), + ).resolves.toBeUndefined(); const aggregate = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?") .bind("github_app.pr_public_surface_failed") .first<{ detail: string; metadata_json: string }>(); expect(aggregate).toMatchObject({ detail: "comment" }); expect(aggregate?.metadata_json).toContain('"output":"comment"'); + expect(aggregate?.metadata_json).toContain('"transient":false'); + const published = await env.DB.prepare("select event_type from audit_events where event_type = ?").bind("github_app.pr_public_surface_published").all(); + expect(published.results).toEqual([]); + const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("all-public-outputs-failed").first<{ status: string }>(); + expect(webhookRow?.status).toBe("processed"); + }); + + it("retries the whole job when a transient GitHub 5xx drops every public-surface output (GITTENSORY-5)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicSurface: "comment_only", + checkRunMode: "off", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([{ githubUsername: "oktofeesh1", githubId: "123", totalPrs: 2, totalMergedPrs: 2, isEligible: true, credibility: 1 }]); + if (url === "https://api.gittensor.io/miners/123") return Response.json({ repositories: [] }); + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1" }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/32/comments") && method === "GET") return Response.json([]); + // GitHub 5xx during publish: momentary, not the caller's fault — the job must retry, not silently drop the + // review the same way JSONbored/awesome-claude#4251 did (Sentry GITTENSORY-5). + if (url.includes("/issues/32/comments") && method === "POST") return new Response("upstream unavailable", { status: 502 }); + return new Response("not found", { status: 404 }); + }); + + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "transient-publish-failure", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, + pull_request: { number: 32, title: "Miner work", state: "open", user: { login: "oktofeesh1" }, labels: [], body: "Fixes #1" }, + }, + }), + ).rejects.toMatchObject({ retryKind: "public_surface_publish_transient" }); + + // The failure IS still audited (observability doesn't regress) — it just also throws so the queue retries. + const aggregate = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?") + .bind("github_app.pr_public_surface_failed") + .first<{ detail: string; metadata_json: string }>(); + expect(aggregate).toMatchObject({ detail: "comment" }); + expect(aggregate?.metadata_json).toContain('"transient":true'); const published = await env.DB.prepare("select event_type from audit_events where event_type = ?").bind("github_app.pr_public_surface_published").all(); expect(published.results).toEqual([]); + // The webhook row is marked "error", not "processed" — a thrown job is exactly what lets the queue retry it. + const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("transient-publish-failure").first<{ status: string }>(); + expect(webhookRow?.status).toBe("error"); + }); + + it("leaves a fully successful public-surface publish unaffected by the transient-retry check", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicSurface: "comment_only", + checkRunMode: "off", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([{ githubUsername: "oktofeesh1", githubId: "123", totalPrs: 2, totalMergedPrs: 2, isEligible: true, credibility: 1 }]); + if (url === "https://api.gittensor.io/miners/123") return Response.json({ repositories: [] }); + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1" }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/33/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/33/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + return new Response("not found", { status: 404 }); + }); + + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "public-surface-clean-publish", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, + pull_request: { number: 33, title: "Miner work", state: "open", user: { login: "oktofeesh1" }, labels: [], body: "Fixes #1" }, + }, + }), + ).resolves.toBeUndefined(); + + const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("public-surface-clean-publish").first<{ status: string }>(); + expect(webhookRow?.status).toBe("processed"); + const failed = await env.DB.prepare("select event_type from audit_events where event_type = ?").bind("github_app.pr_public_surface_failed").all(); + expect(failed.results).toEqual([]); + const published = await env.DB.prepare("select metadata_json from audit_events where event_type = ?").bind("github_app.pr_public_surface_published").first<{ metadata_json: string }>(); + expect(published?.metadata_json).toContain('"publishedOutputs":["comment"]'); + expect(published?.metadata_json).toContain('"failedOutputs":[]'); }); it("keeps repository and PR webhook processing internal when installation context is absent", async () => { @@ -14561,7 +14684,9 @@ describe("queue processors", () => { if (url.includes("/labels") && method === "GET") return Response.json([]); if (url.includes("/labels") && method === "POST") { calls.labels += 1; - return new Response("label failed", { status: 503 }); + // A permanent failure (permissions gap, not a momentary blip) — this test is about duplicate-comment + // suppression on a label-only surface, not about retry classification, so it must stay non-transient. + return new Response(JSON.stringify({ message: "Resource not accessible by integration" }), { status: 403 }); } return new Response("not found", { status: 404 }); }); @@ -14580,13 +14705,14 @@ describe("queue processors", () => { }), ).resolves.toBeUndefined(); - // gittensor context-label apply (fails 503, recorded) + the best-effort type-label create attempt (also 503, + // gittensor context-label apply (fails 403, recorded) + the best-effort type-label create attempt (also 403, // swallowed). The context-label failure is still recorded below; the type label never drops the recording. expect(calls).toEqual({ comments: 0, labels: 2 }); const outputFailure = await env.DB.prepare("select event_type, detail from audit_events where event_type = ?") .bind("github_app.pr_label_publish_failed") .first<{ event_type: string; detail: string }>(); - expect(outputFailure).toMatchObject({ event_type: "github_app.pr_label_publish_failed", detail: "label failed" }); + expect(outputFailure?.event_type).toBe("github_app.pr_label_publish_failed"); + expect(outputFailure?.detail).toMatch(/Resource not accessible by integration/); const aggregate = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?") .bind("github_app.pr_public_surface_failed") .first<{ detail: string; metadata_json: string }>();