diff --git a/apps/ade-cli/src/adeRpcServer.ts b/apps/ade-cli/src/adeRpcServer.ts index f25054e09..90d171ff2 100644 --- a/apps/ade-cli/src/adeRpcServer.ts +++ b/apps/ade-cli/src/adeRpcServer.ts @@ -5265,13 +5265,24 @@ async function runTool(args: { if (name === "pr_get_review_comments") { const prId = assertNonEmptyString(toolArgs.prId, "prId"); const prSvc = requirePrService(runtime); + // A failed thread read is reported, not flattened to `[]`. Swallowing it + // let "GitHub refused" render as "No actionable PR comments." — the same + // failure-reads-as-empty conflation this lane removed from `getChecks`, and + // the one an agent reads right before deciding a PR is clean. + let reviewThreadsUnavailable: string | null = null; const [comments, reviews, checks, reviewThreads] = await Promise.all([ prSvc.getComments(prId), prSvc.getReviews(prId), prSvc.getChecks(prId), - prSvc.getReviewThreads(prId).catch(() => []), + prSvc.getReviewThreads(prId).catch((error: unknown) => { + reviewThreadsUnavailable = error instanceof Error ? error.message : String(error); + return []; + }), ]); - return summarizePrReviewComments(prId, comments, reviews, checks, reviewThreads); + return { + ...summarizePrReviewComments(prId, comments, reviews, checks, reviewThreads), + ...(reviewThreadsUnavailable ? { reviewThreadsUnavailable } : {}), + }; } if (name === "pr_rerun_failed_checks") { diff --git a/apps/ade-cli/src/headlessLinearServices.test.ts b/apps/ade-cli/src/headlessLinearServices.test.ts index e571908aa..1436fd3c7 100644 --- a/apps/ade-cli/src/headlessLinearServices.test.ts +++ b/apps/ade-cli/src/headlessLinearServices.test.ts @@ -281,6 +281,51 @@ describe("headlessLinearServices", () => { } }); + it("times out a response body that stalls after headers arrive", async () => { + // The header timer is cleared the moment headers land, so a body that + // stalls mid-stream used to leave the read pending forever: the transport + // failure was never recorded and the poller tick that awaited it never + // completed. The desktop owner has always bounded this phase. + vi.useFakeTimers(); + const environment = isolateHeadlessGithubAuth("ade-headless-github-body-timeout-", { + emptyGhConfig: true, + }); + const previousFetch = globalThis.fetch; + const fetchImpl = vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + // Headers arrive; the body never does, until the request is aborted. + const signal = init?.signal; + return { + ok: true, + status: 200, + headers: new Headers({ "content-type": "application/json" }), + text: () => new Promise((_resolve, reject) => { + signal?.addEventListener("abort", () => { + const error = new Error("The operation was aborted"); + error.name = "AbortError"; + reject(error); + }, { once: true }); + }), + } as unknown as Response; + }) as unknown as typeof fetch; + globalThis.fetch = fetchImpl; + const githubService = createHeadlessGitHubService( + "/tmp/ade-project", + { debug() {}, info() {}, warn() {}, error() {} } as any, + { fetchImpl }, + ); + try { + githubService.setToken("ghp_body_timeout_token"); + const pending = githubService.apiRequest({ method: "GET", path: "/user" }); + const settled = pending.then(() => "resolved").catch((error: unknown) => String(error)); + await vi.advanceTimersByTimeAsync(120_000); + await expect(settled).resolves.toContain("response body timed out"); + } finally { + globalThis.fetch = previousFetch; + vi.useRealTimers(); + environment.restore?.(); + } + }); + it("clears only changed PAT health when headless credentials change", () => { const environment = isolateHeadlessGithubAuth("ade-headless-github-pat-health-", { emptyGhConfig: true, diff --git a/apps/ade-cli/src/headlessLinearServices.ts b/apps/ade-cli/src/headlessLinearServices.ts index c4263b2ef..6811f3c57 100644 --- a/apps/ade-cli/src/headlessLinearServices.ts +++ b/apps/ade-cli/src/headlessLinearServices.ts @@ -33,6 +33,7 @@ import type { GitHubCredentialVerification, GitHubRepoRef, GitHubRateLimitState, + GitHubRequestBudget, GitHubStatus, CtoAttentionState, } from "../../desktop/src/shared/types"; @@ -89,6 +90,7 @@ import { clearGithubCredentialHealth, githubBackgroundRequestPauseUntilMs, githubCredentialCooldown, + githubRequestBudget, githubCredentialNonRateLimitCooldown, githubCredentialRateLimitCooldown, githubCredentialInventoryKey, @@ -616,6 +618,51 @@ function parseNextGitHubLink(linkHeader: string | null): string | null { } const GITHUB_API_TIMEOUT_MS = 20_000; +const GITHUB_API_BODY_TIMEOUT_MS = 30_000; + +function githubTimeoutError(phase: "request" | "response body"): Error { + return new Error( + `GitHub API ${phase} timed out. Check network access on this machine.`, + ); +} + +/** + * Bound the body read on the same controller that owns the stream. + * + * The header timer is cleared the moment headers arrive, so without this a + * response whose body stalls mid-stream never settles: the caller's + * transport-failure record never runs, and the poller tick that awaited it + * never completes either. The desktop owner has always bounded this phase; the + * two owners have to agree, because the daemon is the one that actually polls + * in a packaged build. + */ +function boundGitHubResponseBody( + response: Response, + controller: AbortController, + release: () => void, +): Response { + const readText = response.text.bind(response); + let bodyTimedOut = false; + Object.defineProperty(response, "text", { + configurable: true, + value: async (): Promise => { + const timer = setTimeout(() => { + bodyTimedOut = true; + controller.abort(); + }, GITHUB_API_BODY_TIMEOUT_MS); + try { + return await readText(); + } catch (error) { + if (bodyTimedOut) throw githubTimeoutError("response body"); + throw error; + } finally { + clearTimeout(timer); + release(); + } + }, + }); + return response; +} async function fetchGitHub( input: string | URL, @@ -627,20 +674,25 @@ async function fetchGitHub( const abortFromUpstream = (): void => controller.abort(upstreamSignal?.reason); if (upstreamSignal?.aborted) abortFromUpstream(); else upstreamSignal?.addEventListener("abort", abortFromUpstream, { once: true }); + const release = (): void => { + upstreamSignal?.removeEventListener("abort", abortFromUpstream); + }; const timer = setTimeout(() => controller.abort(), GITHUB_API_TIMEOUT_MS); + let response: Response; try { - return await fetchImpl(input, { ...init, signal: controller.signal }); + response = await fetchImpl(input, { ...init, signal: controller.signal }); } catch (error) { + release(); if (error instanceof Error && error.name === "AbortError") { - throw new Error( - "GitHub API request timed out. Check network access on this machine.", - ); + throw githubTimeoutError("request"); } throw error; } finally { clearTimeout(timer); - upstreamSignal?.removeEventListener("abort", abortFromUpstream); } + // `release` now runs when the body settles, not here — the upstream abort has + // to stay wired through the body phase for it to be cancellable at all. + return boundGitHubResponseBody(response, controller, release); } export function createHeadlessGitHubService( @@ -1232,6 +1284,26 @@ export function createHeadlessGitHubService( releaseConditionalRequest = conditional.release; } } + // A request that never got an answer from GitHub — a hang, a timeout, a + // DNS or TLS failure, a body that stalls mid-stream — used to throw + // straight out of here recording nothing. That is the outage shape this + // lane targets, and with no record the request budget reported no failure + // kind, so the caller's ladder could not climb past its flat unclassified + // rung. Recorded with a null rate limit so it cannot clobber the real + // quota numbers, and the kinds this produces (`network` / `unknown`) + // carry no cooldown, so it can never park a credential the user's next + // action needs. + const recordTransportFailure: (error: unknown) => never = (error) => { + recordGithubOperationFailure( + candidate, + classifyGitHubAuthFailure({ + message: error instanceof Error ? error.message : String(error), + }).authFailure, + null, + ); + throw error; + }; + let response: Response; try { response = await requestGitHub(url, { @@ -1239,6 +1311,8 @@ export function createHeadlessGitHubService( headers, body: args.body == null ? undefined : JSON.stringify(args.body), }); + } catch (error) { + recordTransportFailure(error); } finally { releaseConditionalRequest?.(); } @@ -1254,9 +1328,13 @@ export function createHeadlessGitHubService( method: args.method, headers, body: args.body == null ? undefined : JSON.stringify(args.body), - }); + }).catch(recordTransportFailure); } - const text = await response.text(); + // The body is read after the header timer is cleared, so a socket error + // mid-body surfaces here rather than above — same shape, same record. + // (Unlike the desktop owner, this transport arms no body timeout, so a + // body that stalls without erroring simply never settles.) + const text = await response.text().catch(recordTransportFailure); let data: unknown = text; try { data = text.trim().length ? JSON.parse(text) : {}; @@ -1922,6 +2000,14 @@ export function createHeadlessGitHubService( githubOperationCredentialCandidates(inventory.candidates, "read"), ); }, + /** + * Runtime-owned twin of the desktop service's budget read. The daemon owns + * GitHub access for runtime-bound windows, so implementing this only on the + * desktop side would leave the shipping build's poll governor un-gated. + */ + async getRequestBudget(): Promise { + return githubRequestBudget(); + }, async getRemoteStatus() { const origin = await readGitOriginAsync(projectRoot); return { diff --git a/apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts b/apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts index 11c88ae6f..acef33221 100644 --- a/apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts +++ b/apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts @@ -238,6 +238,7 @@ describe("createSyncRemoteCommandService", () => { "agentChat.getEventHistoryPage", "github.getStatus", "github.getRemoteStatus", + "github.getRequestBudget", "ai.getStatus", "prs.list", "prs.listOpenForRepo", @@ -2088,6 +2089,7 @@ describe("createSyncRemoteCommandService", () => { "history.listOperations", "github.getStatus", "github.getRemoteStatus", + "github.getRequestBudget", "github.publishCurrentProject", "projectConfig.get", "projectConfig.save", diff --git a/apps/ade-cli/src/services/sync/syncRemoteCommandService.ts b/apps/ade-cli/src/services/sync/syncRemoteCommandService.ts index 9fdc4f2a5..ce24ec081 100644 --- a/apps/ade-cli/src/services/sync/syncRemoteCommandService.ts +++ b/apps/ade-cli/src/services/sync/syncRemoteCommandService.ts @@ -108,6 +108,7 @@ import type { GitRevertArgs, GitGetUserIdentityArgs, GitHubRepoRef, + GitHubRequestBudget, GitHubStatus, GitStashPushArgs, GitStashRefArgs, @@ -5338,6 +5339,11 @@ function registerMiscRemoteCommands({ args, register }: RemoteCommandRegistratio })); register("github.getRemoteStatus", { viewerAllowed: true, observesAbort: true }, async (): Promise<{ repo: GitHubRepoRef | null; hasOrigin: boolean }> => requireService(args.githubService, "GitHub service not available.").getRemoteStatus()); + // The web client's PR timers run in the browser but spend THIS machine's + // quota. Without this registration its adapter falls back to an all-null + // budget and its 5-second checks loop never sees the reserve. + register("github.getRequestBudget", { viewerAllowed: true, observesAbort: true }, async (): Promise => + requireService(args.githubService, "GitHub service not available.").getRequestBudget()); register("github.publishCurrentProject", { viewerAllowed: true }, async (payload): Promise => { const { owner, name, description, isPrivate } = parsePublishCurrentProjectArgs(payload); return await requireService(args.githubService, "GitHub service not available.").publishCurrentProject({ diff --git a/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts b/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts index 0a919a26a..3cf67b9ac 100644 --- a/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts +++ b/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts @@ -276,6 +276,51 @@ describe("rightPaneFormatters", () => { expect(body).toContain("CI: not run"); }); + it("shows a failed checks read as a failure, not as an empty suite", () => { + // `prService.getChecks` now REJECTS when neither checks source could be + // read, and `/pr checks` catches that into `{ error }`. Rendering that as + // "No PR checks." would tell the reader the commit ran nothing — the same + // false all-clear the host-side change exists to stop. + const body = formatPrChecks({ error: "GitHub API rate limit exceeded" }); + + expect(body).toContain("could not be read"); + expect(body).toContain("GitHub API rate limit exceeded"); + expect(body).not.toContain("No PR checks."); + }); + + it("shows a failed comments read as a failure", () => { + const body = formatPrComments({ error: "fetch failed" }); + + expect(body).toContain("could not be read"); + expect(body).toContain("fetch failed"); + expect(body).not.toContain("No actionable PR comments."); + }); + + it("names the sources that failed in a partial PR review read", () => { + // `/pr review` fires three reads and catches each independently, so a + // partial outage is the common case. The sources that answered still + // render; the ones that did not must not report a count of zero. + const body = formatPrReview({ + reviews: [{ state: "approved", reviewer: "octocat" }], + threads: { error: "GitHub is unavailable" }, + comments: { error: "GitHub is unavailable" }, + }); + + expect(body).toContain("1 review"); + expect(body).toContain("threads unavailable"); + expect(body).toContain("comments unavailable"); + expect(body).toContain("GitHub is unavailable"); + expect(body).not.toContain("0 threads"); + expect(body).not.toContain("No PR reviews or comments."); + }); + + it("keeps the empty verdict when every PR review read succeeded", () => { + const body = formatPrReview({ reviews: [], threads: [], comments: [] }); + + expect(body).toContain("No PR reviews or comments."); + expect(body).not.toContain("could not be read"); + }); + it("summarizes PR review comments and threads", () => { const body = formatPrComments({ summary: { checksStatus: "passing", actionableComments: 2 }, @@ -300,6 +345,33 @@ describe("rightPaneFormatters", () => { expect(body).not.toContain("\"reviewThreads\""); }); + it("names a failed review-thread read instead of reporting nothing to action", () => { + // The aggregate preserves a thread-read failure rather than flattening it + // to `[]`. Rendering that as "No actionable PR comments." tells an agent a + // PR is clean on the strength of a read that never happened. + const body = formatPrComments({ + summary: { checksStatus: "passing", actionableComments: 0 }, + reviewThreads: [], + comments: [], + reviewThreadsUnavailable: "GitHub API request failed (HTTP 503)", + }); + + expect(body).toContain("Could not be read"); + expect(body).toContain("review threads: GitHub API request failed (HTTP 503)"); + expect(body).not.toContain("No actionable PR comments."); + }); + + it("still reports a genuinely empty comment set as empty", () => { + const body = formatPrComments({ + summary: { checksStatus: "passing", actionableComments: 0 }, + reviewThreads: [], + comments: [], + }); + + expect(body).toContain("No actionable PR comments."); + expect(body).not.toContain("Could not be read"); + }); + it("summarizes full PR review data", () => { const body = formatPrReview({ reviews: [{ reviewer: "maintainer", state: "changes_requested", body: "Needs a test." }], diff --git a/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts b/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts index 830b00b21..f00a6f076 100644 --- a/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts +++ b/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts @@ -71,6 +71,23 @@ function unwrapPrValue(value: unknown): { root: JsonRecord; pr: JsonRecord } | n }; } +/** + * The message from a PR read that failed, or null when the value is real data. + * + * The `/pr` dispatch catches a rejected read into `{ error }` rather than + * blowing the pane away, so every PR formatter has to tell "GitHub refused" + * apart from "there is nothing to show". Without this the two collapse: an + * `{ error }` carries no rows, so the formatters below printed "No PR checks." + * for an outage — the exact conflation `prService.getChecks` stopped making + * when it started rejecting instead of returning `[]`. A reader cannot act on + * a lie that reads as a fact, and "no checks ran" invites a merge. + */ +function prReadFailure(value: unknown): string | null { + const root = unwrapStructured(value); + if (!isRecord(root)) return null; + return asString(root.error); +} + function firstRecordArray(value: unknown, keys: string[]): JsonRecord[] { const root = unwrapStructured(value); if (Array.isArray(root)) return root.filter(isRecord); @@ -354,6 +371,8 @@ export function formatPrMergeState(value: unknown): string { } export function formatPrChecks(value: unknown): string { + const failure = prReadFailure(value); + if (failure) return `PR checks · could not be read — ${failure}`; const root = unwrapStructured(value); const rollup = isRecord(root) ? pickString(root, ["checksStatus"]) : null; const reason = isRecord(root) ? pickString(root, ["checksReason"]) : null; @@ -399,13 +418,43 @@ export function formatPrChecks(value: unknown): string { ].join("\n"); } +/** reviews + threads + comments — the three independent reads behind `/pr review`. */ +const PR_REVIEW_SOURCES = 3; + export function formatPrReview(value: unknown): string { const root = unwrapStructured(value); const reviews = firstRecordArray(root, ["reviews"]); const threads = firstRecordArray(root, ["reviewThreads", "threads"]); const comments = firstRecordArray(root, ["comments", "issueComments"]); + // Three independent reads land here, each caught into its own `{ error }`, + // so a partial outage is normal: name the sources that failed rather than + // reporting their absence as "0 reviews". + const failures: Array<[label: string, message: string]> = []; + for (const [label, part] of [ + ["reviews", isRecord(root) ? root.reviews : null], + ["threads", isRecord(root) ? (root.reviewThreads ?? root.threads) : null], + ["comments", isRecord(root) ? (root.comments ?? root.issueComments) : null], + ] satisfies Array<[string, unknown]>) { + const message = prReadFailure(part); + if (message) failures.push([label, message]); + } + const rootFailure = prReadFailure(root); + if (rootFailure) return `PR review · could not be read — ${rootFailure}`; + if (failures.length === PR_REVIEW_SOURCES) { + return [ + "PR review · could not be read", + ...failures.map(([label, message]) => ` ✗ ${label}: ${message}`), + ].join("\n"); + } + // A source that failed has no count to report — printing `0 reviews` would + // state as fact the very thing the read could not establish. + const failed = new Set(failures.map(([label]) => label)); + const headerCount = (label: string, noun: string, count: number): string => + failed.has(label) ? `${noun}s unavailable` : formatCount(noun, count); const lines = [ - `PR review · ${formatCount("review", reviews.length)} · ${formatCount("thread", threads.length)} · ${formatCount("comment", comments.length)}`, + `PR review · ${headerCount("reviews", "review", reviews.length)}` + + ` · ${headerCount("threads", "thread", threads.length)}` + + ` · ${headerCount("comments", "comment", comments.length)}`, ]; if (reviews.length) { lines.push("", "Reviews"); @@ -431,11 +480,18 @@ export function formatPrReview(value: unknown): string { lines.push(`- ${commentPreview(comment)}`); } } - if (!reviews.length && !threads.length && !comments.length) lines.push("", "No PR reviews or comments."); + if (failures.length > 0) { + lines.push("", "Could not be read"); + for (const [label, message] of failures) lines.push(` ✗ ${label}: ${message}`); + } else if (!reviews.length && !threads.length && !comments.length) { + lines.push("", "No PR reviews or comments."); + } return lines.join("\n"); } export function formatPrComments(value: unknown): string { + const failure = prReadFailure(value); + if (failure) return `PR comments · could not be read — ${failure}`; const root = unwrapStructured(value); const summary = isRecord(root) && isRecord(root.summary) ? root.summary : null; const threads = firstRecordArray(root, ["reviewThreads", "threads"]); @@ -444,6 +500,10 @@ export function formatPrComments(value: unknown): string { summary ? checksStatusWord(pickString(summary, ["checksStatus"])) : null, summary ? `${asString(summary.actionableComments) ?? "0"} actionable` : null, ].filter(Boolean); + // The aggregate reports a failed thread read rather than flattening it to an + // empty list, so a partial answer is normal here: name the source that failed + // instead of letting "GitHub refused" read as "nothing to action". + const threadsUnavailable = isRecord(root) ? asString(root.reviewThreadsUnavailable) : null; const lines = [`PR comments${headerParts.length ? ` · ${headerParts.join(" · ")}` : ""}`]; if (threads.length) { lines.push("", "Review threads"); @@ -460,7 +520,11 @@ export function formatPrComments(value: unknown): string { lines.push(`- ${commentPreview(comment)}`); } } - if (!threads.length && !comments.length) lines.push("", "No actionable PR comments."); + if (threadsUnavailable) { + lines.push("", "Could not be read", ` ✗ review threads: ${threadsUnavailable}`); + } else if (!threads.length && !comments.length) { + lines.push("", "No actionable PR comments."); + } return lines.join("\n"); } diff --git a/apps/desktop/src/main/services/adeActions/registry.ts b/apps/desktop/src/main/services/adeActions/registry.ts index dfd3d2e89..6eb971c5c 100644 --- a/apps/desktop/src/main/services/adeActions/registry.ts +++ b/apps/desktop/src/main/services/adeActions/registry.ts @@ -818,6 +818,7 @@ export const ADE_ACTION_ALLOWLIST: Partial { expect(githubCredentialCooldown(appCandidate)).toBeNull(); }); + describe("githubRequestBudget", () => { + // The budget is what lets a *foreground* poller honour the same reserve the + // background PR poller already respects. Before it existed the reserve was + // enforced in exactly one place, so the renderer's 5s checks loop drained + // the quota to zero while a 500-request reserve nominally protected it. + const RESET_AT = "2026-08-17T13:00:00.000Z"; + + function recordCoreQuota(remaining: number): void { + recordGithubCredentialSuccess(ghCandidate, new Headers({ + "x-ratelimit-limit": "5000", + "x-ratelimit-remaining": String(remaining), + "x-ratelimit-used": String(5000 - remaining), + "x-ratelimit-reset": String(Date.parse(RESET_AT) / 1_000), + "x-ratelimit-resource": "core", + })); + } + + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-08-17T12:00:00.000Z")); + }); + + it("gates automatic requests once the core quota reaches the reserve", () => { + recordCoreQuota(GITHUB_BACKGROUND_RATE_LIMIT_RESERVE); + expect(githubRequestBudget(Date.now(), [ghCandidate]).pausedUntil) + .toBe(new Date(Date.parse(RESET_AT)).toISOString()); + }); + + it("answers from every known credential when given none, without a network or subprocess call", () => { + // The unscoped form is what the IPC action calls. Resolving a credential + // inventory to scope it is NOT free — it can shell out to `gh auth token`, + // decrypt the credential store, or refresh an App user token over the + // network — and this read runs on a timer and on every failed poll group. + recordCoreQuota(GITHUB_BACKGROUND_RATE_LIMIT_RESERVE); + expect(githubRequestBudget().pausedUntil) + .toBe(new Date(Date.parse(RESET_AT)).toISOString()); + }); + + it("leaves automatic requests running while quota is above the reserve", () => { + recordCoreQuota(GITHUB_BACKGROUND_RATE_LIMIT_RESERVE + 1); + expect(githubRequestBudget(Date.now(), [ghCandidate]).pausedUntil).toBeNull(); + }); + + it("reports a GitHub outage as a typed kind without parking the credential", () => { + // `service_unavailable` deliberately carries no cooldown — a 5xx is not + // the credential's fault — but the kind still has to reach the caller, or + // an outage looks identical to a healthy GitHub that returned nothing. + recordGithubOperationFailure(ghCandidate, { + kind: "service_unavailable", + message: "No server is currently available to service your request.", + retryAt: null, + }, { + limit: 5000, + remaining: 4321, + used: 679, + resetAt: RESET_AT, + resource: "core", + }); + + const budget = githubRequestBudget(Date.now(), [ghCandidate]); + expect(budget.failureKind).toBe("service_unavailable"); + expect(budget.pausedUntil).toBeNull(); + expect(githubCredentialCooldown(ghCandidate)).toBeNull(); + }); + + it("reports the failure asking for the longest stand-down", () => { + recordGithubOperationFailure(appCandidate, { + kind: "permission_denied", + message: "Resource not accessible", + retryAt: null, + }, { limit: 5000, remaining: 4000, used: 1000, resetAt: null, resource: "core" }); + recordGithubOperationFailure(ghCandidate, { + kind: "rate_limited", + message: "API rate limit exceeded", + retryAt: RESET_AT, + }, { limit: 5000, remaining: 0, used: 5000, resetAt: RESET_AT, resource: "core" }); + + const budget = githubRequestBudget(Date.now(), [appCandidate, ghCandidate]); + expect(budget.failureKind).toBe("rate_limited"); + expect(budget.retryAt).toBe(RESET_AT); + }); + + it("ignores the search bucket, which PR reads do not spend", () => { + recordGithubCredentialSuccess(ghCandidate, new Headers({ + "x-ratelimit-limit": "30", + "x-ratelimit-remaining": "0", + "x-ratelimit-reset": String(Date.parse(RESET_AT) / 1_000), + "x-ratelimit-resource": "search", + })); + expect(githubRequestBudget(Date.now(), [ghCandidate]).pausedUntil).toBeNull(); + }); + + it("ranks a credential-scoped failure above a local network fault", () => { + // The severity order is a contract with `ladderBaseMs` in + // `renderer/components/prs/state/githubPollGovernor.ts`: it must report + // the kind that asks for the LONGER stand-down. `network` outranking + // `invalid_token` here would have handed the governor the 30s base when + // the other credential warranted 60s. + recordGithubOperationFailure(appCandidate, { + kind: "invalid_token", + message: "Bad credentials", + retryAt: null, + }, { limit: 5000, remaining: 4000, used: 1000, resetAt: null, resource: "core" }); + recordGithubOperationFailure(ghCandidate, { + kind: "network", + message: "fetch failed", + retryAt: null, + }, { limit: 5000, remaining: 3000, used: 2000, resetAt: null, resource: "core" }); + + expect(githubRequestBudget(Date.now(), [appCandidate, ghCandidate]).failureKind) + .toBe("invalid_token"); + }); + + it("clears the reported failure once GitHub answers again", () => { + recordGithubOperationFailure(ghCandidate, { + kind: "service_unavailable", + message: "Bad gateway", + retryAt: null, + }, { limit: 5000, remaining: 4321, used: 679, resetAt: RESET_AT, resource: "core" }); + expect(githubRequestBudget(Date.now(), [ghCandidate]).failureKind) + .toBe("service_unavailable"); + + recordCoreQuota(4320); + expect(githubRequestBudget(Date.now(), [ghCandidate]).failureKind).toBeNull(); + }); + + it("reports a failure that arrived without any rate-limit headers", () => { + // The failures that matter most here — a hung request, a DNS failure, an + // edge 5xx — carry no `x-ratelimit-*` at all, so they land under the + // `unknown` resource with no limit. Filtering the kind scan by the quota + // bucket dropped exactly those, which left the classified ladder inert + // for the outage shape it was written for: the governor saw no kind and + // held a flat short rung instead of climbing to its ceiling. + recordGithubOperationFailure(ghCandidate, { + kind: "network", + message: "GitHub API request timed out. Check network access on this machine.", + retryAt: null, + }, null); + + expect(githubRequestBudget(Date.now(), [ghCandidate]).failureKind).toBe("network"); + // ...and it must still not park the credential a user action needs. + expect(githubCredentialCooldown(ghCandidate)).toBeNull(); + }); + + it("still ignores the search bucket for the reported kind", () => { + recordGithubOperationFailure(ghCandidate, { + kind: "rate_limited", + message: "API rate limit exceeded", + retryAt: RESET_AT, + }, { limit: 30, remaining: 0, used: 30, resetAt: RESET_AT, resource: "search" }); + + expect(githubRequestBudget(Date.now(), [ghCandidate]).failureKind).toBeNull(); + }); + + it("does not let a transport blip un-park a credential already on cooldown", () => { + // Kinds differ in how long they park a credential and share one resource + // entry, so overwriting unconditionally let a `network` failure (which + // deliberately gets no cooldown) clear the five minutes a rejected token + // had just earned — sending the next request straight back at it. + recordGithubOperationFailure(ghCandidate, { + kind: "invalid_token", + message: "Bad credentials", + retryAt: null, + }, null); + expect(githubCredentialCooldown(ghCandidate)?.failure.kind).toBe("invalid_token"); + + recordGithubOperationFailure(ghCandidate, { + kind: "network", + message: "fetch failed", + retryAt: null, + }, null); + // The deadline AND the reason survive together. Keeping the park but + // relabelling it "network" would report a rejected token as a GitHub + // problem — exactly the misattribution that hides the reconnect the user + // actually needs. + expect(githubCredentialCooldown(ghCandidate)?.failure.kind).toBe("invalid_token"); + }); + + it("keeps the preserved cooldown failure stale after a later weaker failure", () => { + // Preserving the older reason must not restamp its clock. Otherwise a + // long-dead `invalid_token` stays inside the freshness window forever as + // long as transient blips keep arriving on the same credential, and the + // budget keeps reporting it process-wide — defeating the recency bound. + recordGithubOperationFailure(ghCandidate, { + kind: "invalid_token", + message: "Bad credentials", + retryAt: null, + }, null); + expect(githubRequestBudget(Date.now(), [ghCandidate]).failureKind).toBe("invalid_token"); + + // Two minutes later — past the freshness window, still inside the + // five-minute cooldown the rejected token earned. + vi.setSystemTime(new Date(Date.now() + 2 * 60_000)); + recordGithubOperationFailure(ghCandidate, { + kind: "network", + message: "fetch failed", + retryAt: null, + }, null); + + // The cooldown is still the one the rejected token earned... + expect(githubCredentialCooldown(ghCandidate)?.failure.kind).toBe("invalid_token"); + // ...but it is no longer recent enough to drive anyone's poll cadence. + expect(githubRequestBudget(Date.now(), [ghCandidate]).failureKind).toBeNull(); + }); + + it("stops reporting a failure kind once it is no longer recent", () => { + // A failure is otherwise cleared only by a success on the SAME credential + // and resource, so a permanently-bad one (revoked PAT, stale + // GITHUB_TOKEN) would keep its kind for the life of the process while + // ADE served every request from the next credential — and, read + // unscoped, would push every project's poll ladder onto the longer base + // on a perfectly healthy GitHub. + recordGithubOperationFailure(ghCandidate, { + kind: "invalid_token", + message: "Bad credentials", + retryAt: null, + }, { limit: 5000, remaining: 4000, used: 1000, resetAt: null, resource: "core" }); + expect(githubRequestBudget(Date.now(), [ghCandidate]).failureKind).toBe("invalid_token"); + + vi.setSystemTime(new Date(Date.now() + 10 * 60_000)); + expect(githubRequestBudget(Date.now(), [ghCandidate]).failureKind).toBeNull(); + }); + + it("answers with an unknown budget when nothing has been recorded", () => { + expect(githubRequestBudget(Date.now(), [ghCandidate])).toEqual({ + pausedUntil: null, + failureKind: null, + retryAt: null, + }); + }); + }); + it.each(["graphql", "search"])( "applies an invalid-token cooldown recorded for %s to every API resource", (resource) => { diff --git a/apps/desktop/src/main/services/github/githubCredentialHealth.ts b/apps/desktop/src/main/services/github/githubCredentialHealth.ts index cbb919362..435772438 100644 --- a/apps/desktop/src/main/services/github/githubCredentialHealth.ts +++ b/apps/desktop/src/main/services/github/githubCredentialHealth.ts @@ -6,6 +6,7 @@ import type { GitHubCredentialState, GitHubRateLimitState, GitHubRepoRef, + GitHubRequestBudget, } from "../../../shared/types"; import { githubRateLimitRetryAtMs, @@ -26,6 +27,21 @@ const SECONDARY_RATE_LIMIT_COOLDOWN_MS = 60_000; // instant GitHub recovers. const REPOSITORY_ACCESS_TTL_MS = 2 * 60_000; export const GITHUB_BACKGROUND_RATE_LIMIT_RESERVE = 500; +/** + * How recent a recorded failure must be for {@link githubRequestBudget} to + * report its kind. + * + * A failure is otherwise cleared only by a success on the SAME credential and + * resource, so a permanently-bad one (a stale `GITHUB_TOKEN`, a revoked PAT, a + * fork the App cannot see) keeps its kind for the life of the process while + * ADE happily serves every request from the next credential in the chain. The + * budget is read unscoped, so that stale kind would become the process-wide + * answer and push every project's poll ladder onto the longer base on a + * perfectly healthy GitHub. Callers read the budget immediately after a failure + * they just observed, so a window a little wider than their refresh cadence is + * all the kind needs to be useful. + */ +const REQUEST_BUDGET_FAILURE_FRESHNESS_MS = 90_000; export type GithubCredentialCandidate = { source: GitHubCredentialSource; @@ -38,6 +54,8 @@ type CredentialResourceHealth = { failure: GitHubAuthFailure | null; rateLimit: GitHubRateLimitState | null; cooldownUntilMs: number; + /** When {@link failure} was recorded. Zero when there is no failure. */ + failureAtMs: number; }; type CredentialHealth = { @@ -146,6 +164,7 @@ export function recordGithubCredentialSuccess( failure: null, rateLimit: rateLimit ?? current?.rateLimit ?? null, cooldownUntilMs: 0, + failureAtMs: 0, })), userLogin: normalizedLogin(userLogin ?? candidate.userLogin ?? existing?.userLogin), }); @@ -164,11 +183,54 @@ export function recordGithubCredentialProbeSuccess( failure: null, rateLimit: rateLimit ?? current?.rateLimit ?? null, cooldownUntilMs: 0, + failureAtMs: 0, })), userLogin: normalizedLogin(userLogin ?? candidate.userLogin ?? existing?.userLogin), }); } +/** + * Reconcile a new failure with a cooldown that is still running. + * + * A later failure must never *shorten* one. Kinds differ in how long they park + * a credential — a rejected token gets five minutes, a transport error + * deliberately gets none — and they share a resource entry, so overwriting + * unconditionally let a transient network blip un-park a credential ADE had + * already decided was broken, sending the next request straight back at it. + * + * When the older cooldown wins, its *reason* is kept with it. The deadline and + * the reason describe the same decision, and splitting them produced an + * incoherent state: a rejected token parked for five minutes but reported as a + * network problem, which is precisely the misattribution that hides the + * reconnect the user actually needs. + */ +function reconcileCooldown( + current: CredentialResourceHealth | undefined, + failure: GitHubAuthFailure, + cooldownUntilMs: number, + nowMs: number, +): Pick { + const live = (current?.cooldownUntilMs ?? 0) > nowMs ? current!.cooldownUntilMs : 0; + if (live > cooldownUntilMs && current?.failure) { + // The preserved failure keeps its ORIGINAL timestamp. Restamping it to now + // would refresh a stale reason's clock on every later failure, so a + // long-dead `invalid_token` would stay inside + // `REQUEST_BUDGET_FAILURE_FRESHNESS_MS` indefinitely as long as transient + // blips kept arriving — and `githubRequestBudget` would keep reporting it + // process-wide, which is exactly what that bound exists to prevent. + return { + failure: current.failure, + cooldownUntilMs: live, + failureAtMs: current.failureAtMs, + }; + } + return { + failure, + cooldownUntilMs: Math.max(cooldownUntilMs, live), + failureAtMs: nowMs, + }; +} + function recordGithubFailure( candidate: GithubCredentialCandidate, failure: GitHubAuthFailure, @@ -178,11 +240,11 @@ function recordGithubFailure( const digest = githubCredentialTokenDigest(candidate.token); const existing = healthByTokenDigest.get(digest); const userLogin = normalizedLogin(candidate.userLogin ?? existing?.userLogin); + const nowMs = Date.now(); const next: CredentialHealth = { resources: updateResourceHealth(existing, rateLimit, (current) => ({ - failure, + ...reconcileCooldown(current, failure, cooldownUntilMs, nowMs), rateLimit: rateLimit ?? current?.rateLimit ?? null, - cooldownUntilMs, })), userLogin, }; @@ -198,9 +260,8 @@ function recordGithubFailure( const resources = new Map(candidateHealth.resources); const current = resources.get(resource); resources.set(resource, { - failure, + ...reconcileCooldown(current, failure, cooldownUntilMs, nowMs), rateLimit: rateLimit ?? current?.rateLimit ?? null, - cooldownUntilMs, }); healthByTokenDigest.set(candidateDigest, { ...candidateHealth, @@ -361,22 +422,39 @@ export function githubCredentialStates(args: { }); } +/** + * Whether a quota bucket is one PR reads actually spend from. Search has its + * own small independent bucket and must never pause PR refresh; `unknown` only + * counts when the reported limit is large enough to be a primary bucket. + */ +function protectsPullRequestReads( + resource: string, + rateLimit: GitHubRateLimitState | null, +): boolean { + return resource === "core" + || resource === "graphql" + || (resource === "unknown" && (rateLimit?.limit ?? 0) >= 1_000); +} + +function healthEntriesFor( + candidates?: readonly GithubCredentialCandidate[], +): CredentialHealth[] { + return candidates + ? candidates + .map((candidate) => healthFor(candidate)) + .filter((health): health is CredentialHealth => health != null) + : [...healthByTokenDigest.values()]; +} + export function githubBackgroundRequestPauseUntilMs( nowMs = Date.now(), candidates?: readonly GithubCredentialCandidate[], ): number | null { let pauseUntilMs: number | null = null; - const healthEntries = candidates - ? candidates.map((candidate) => healthFor(candidate)).filter(Boolean) - : [...healthByTokenDigest.values()]; - for (const health of healthEntries) { - if (!health) continue; + for (const health of healthEntriesFor(candidates)) { for (const [resource, resourceHealth] of health.resources) { const rateLimit = resourceHealth.rateLimit; - const protectsPullRequestReads = resource === "core" - || resource === "graphql" - || (resource === "unknown" && (rateLimit?.limit ?? 0) >= 1_000); - if (!protectsPullRequestReads) continue; + if (!protectsPullRequestReads(resource, rateLimit)) continue; const remaining = rateLimit?.remaining; const resetAt = rateLimit?.resetAt ? Date.parse(rateLimit.resetAt) : NaN; if (remaining == null || remaining > GITHUB_BACKGROUND_RATE_LIMIT_RESERVE) continue; @@ -386,3 +464,76 @@ export function githubBackgroundRequestPauseUntilMs( } return pauseUntilMs; } + +/** + * Rank of a failure kind by how long a stand-down it justifies. + * + * Contract with `ladderBaseMs` in + * `renderer/components/prs/state/githubPollGovernor.ts`: the budget reports the + * worst kind currently recorded, and "worst" has to mean the same thing in both + * modules or a multi-credential chain reports the kind that asks for the + * *shorter* wait. Change one, change both. + */ +const REQUEST_BUDGET_FAILURE_SEVERITY: Record = { + rate_limited: 5, + service_unavailable: 4, + invalid_token: 3, + permission_denied: 2, + network: 1, + unknown: 0, +}; + +/** + * The reserve and the worst recorded failure kind, for automatic GitHub readers + * deciding their cadence before spending a request. + * + * Takes no credential inventory, and that is load-bearing rather than a + * shortcut: resolving one can shell out to `gh auth token`, decrypt the + * credential store (a PowerShell subprocess under DPAPI on Windows), or refresh + * an expired App user token *over the network* — and this runs on a timer and + * again on every failed poll group, during exactly the outage it exists to + * survive. Reading every credential this process knows rather than one + * project's is also the safe direction: the primary quota is per-account, so + * over-throttling is conservative and under-throttling is the bug. It matches + * `prPollingService`, which calls `githubBackgroundRequestPauseUntilMs()` + * unscoped for the same reason. + */ +export function githubRequestBudget( + nowMs = Date.now(), + candidates?: readonly GithubCredentialCandidate[], +): GitHubRequestBudget { + const pauseUntilMs = githubBackgroundRequestPauseUntilMs(nowMs, candidates); + let failure: GitHubAuthFailure | null = null; + for (const health of healthEntriesFor(candidates)) { + for (const [resource, resourceHealth] of health.resources) { + // Deliberately NOT `protectsPullRequestReads`. That filter exists to stop + // the tiny `search` bucket from pausing PR refresh, and its `limit >= 1000` + // clause makes it a statement about a quota bucket. A failure kind is a + // statement about GitHub, and the failures that matter most here — a hung + // request, a DNS failure, an edge 5xx — carry no `x-ratelimit-*` headers + // at all, so they land under `unknown` with no limit and were being + // dropped. That left the classified ladder inert for exactly the outage + // shape it was written for: the governor saw `failureKind: null` and held + // a flat 30-second rung instead of climbing to the five-minute ceiling. + if (resource === "search") continue; + const current = nowMs - resourceHealth.failureAtMs <= REQUEST_BUDGET_FAILURE_FRESHNESS_MS + ? resourceHealth.failure + : null; + if ( + current + && ( + !failure + || REQUEST_BUDGET_FAILURE_SEVERITY[current.kind] + > REQUEST_BUDGET_FAILURE_SEVERITY[failure.kind] + ) + ) { + failure = current; + } + } + } + return { + pausedUntil: pauseUntilMs == null ? null : new Date(pauseUntilMs).toISOString(), + failureKind: failure?.kind ?? null, + retryAt: failure?.retryAt ?? null, + }; +} diff --git a/apps/desktop/src/main/services/github/githubRateLimit.ts b/apps/desktop/src/main/services/github/githubRateLimit.ts index 05310568c..46d1050d9 100644 --- a/apps/desktop/src/main/services/github/githubRateLimit.ts +++ b/apps/desktop/src/main/services/github/githubRateLimit.ts @@ -217,6 +217,21 @@ export function classifyGitHubGraphqlCredentialFailure( return null; } +/** + * The classified failure kind carried by a thrown GitHub request error, or null + * when the error is not one. + * + * Duck-typed rather than `instanceof`: the error classes that carry + * `authFailure` are module-private to their request helpers, and there are two + * of them (the desktop service and the headless twin), so a nominal check would + * silently answer null for whichever owner it was not written against. + */ +export function githubAuthFailureKindOf(error: unknown): GitHubAuthFailure["kind"] | null { + if (error instanceof GitHubRateLimitError) return "rate_limited"; + const kind = (error as { authFailure?: { kind?: unknown } } | null)?.authFailure?.kind; + return typeof kind === "string" ? kind as GitHubAuthFailure["kind"] : null; +} + export function githubRateLimitResetAtMs(rateLimit: GitHubRateLimitState | null): number | null { if (!rateLimit?.resetAt) return null; const parsed = Date.parse(rateLimit.resetAt); diff --git a/apps/desktop/src/main/services/github/githubService.ts b/apps/desktop/src/main/services/github/githubService.ts index 9e303eace..4f1ce3849 100644 --- a/apps/desktop/src/main/services/github/githubService.ts +++ b/apps/desktop/src/main/services/github/githubService.ts @@ -16,6 +16,7 @@ import type { GitHubCredentialVerification, GitHubRateLimitState, GitHubRepoRef, + GitHubRequestBudget, GitHubStatus, } from "../../../shared/types"; import { resolveAdeLayout } from "../../../shared/adeLayout"; @@ -60,6 +61,7 @@ import { clearGithubCredentialHealth, githubBackgroundRequestPauseUntilMs, githubCredentialCooldown, + githubRequestBudget, githubCredentialNonRateLimitCooldown, githubCredentialRateLimitCooldown, githubCredentialInventoryKey, @@ -1384,6 +1386,26 @@ export function createGithubService({ } } + // A request that never got an answer from GitHub — a hang, a timeout, a + // DNS or TLS failure, a body that stalls mid-stream — used to throw + // straight out of here recording nothing. That is the outage shape this + // lane targets, and with no record the request budget reported no failure + // kind, so the caller's ladder could not climb past its flat unclassified + // rung. Recorded with a null rate limit so it cannot clobber the real + // quota numbers, and the kinds this produces (`network` / `unknown`) + // carry no cooldown, so it can never park a credential the user's next + // action needs. + const recordTransportFailure: (error: unknown) => never = (error) => { + recordGithubOperationFailure( + candidate, + classifyGitHubAuthFailure({ + message: error instanceof Error ? error.message : String(error), + }).authFailure, + null, + ); + throw error; + }; + let response: Response; try { response = await fetchGitHub(url.toString(), { @@ -1391,6 +1413,8 @@ export function createGithubService({ headers, body: args.body != null ? JSON.stringify(args.body) : undefined, }); + } catch (error) { + recordTransportFailure(error); } finally { releaseConditionalRequest?.(); } @@ -1409,10 +1433,12 @@ export function createGithubService({ method: args.method, headers, body: args.body != null ? JSON.stringify(args.body) : undefined, - }); + }).catch(recordTransportFailure); } - const text = await response.text(); + // The body has its own timeout, so a response that stalls mid-stream + // fails here rather than above — same shape, same record. + const text = await response.text().catch(recordTransportFailure); let data: unknown = text; try { data = text.trim().length ? JSON.parse(text) : {}; @@ -2365,6 +2391,18 @@ export function createGithubService({ ); }, + /** + * Zero-network read of the reserve + last classified failure, for automatic + * GitHub readers that must decide their cadence *before* spending a + * request. Callers poll this on a timer and on every failed poll group, so + * it deliberately does NOT resolve a credential inventory — see + * `githubRequestBudget` for why that is not free and why answering from + * every known credential is the safe direction. + */ + async getRequestBudget(): Promise { + return githubRequestBudget(); + }, + getAppUserAuthStatus(): GitHubAppUserAuthStatus { return appUserAuth.getAuthStatus(); }, diff --git a/apps/desktop/src/main/services/ipc/registerIpc.ts b/apps/desktop/src/main/services/ipc/registerIpc.ts index 9945f2230..50f6dda40 100644 --- a/apps/desktop/src/main/services/ipc/registerIpc.ts +++ b/apps/desktop/src/main/services/ipc/registerIpc.ts @@ -257,6 +257,7 @@ import type { GitHubAppUserAuthStatus, GitHubAutolink, GitHubRepoRef, + GitHubRequestBudget, GitHubSetTokenResult, GitHubStatus, AdeAccountStatus, @@ -9653,6 +9654,12 @@ export function registerIpc({ return ctx.githubService.clearAppUserAuth(); }); + // Zero-network read, so pollers can consult the reserve on a timer. + ipcMain.handle(IPC.githubGetRequestBudget, async (): Promise => { + const ctx = getCtx(); + return await ctx.githubService.getRequestBudget(); + }); + const resolveGithubRepoRef = async ( githubService: ReturnType, arg?: { owner?: string; name?: string } | null diff --git a/apps/desktop/src/main/services/prs/prService.test.ts b/apps/desktop/src/main/services/prs/prService.test.ts index 48db674a7..6af609e4e 100644 --- a/apps/desktop/src/main/services/prs/prService.test.ts +++ b/apps/desktop/src/main/services/prs/prService.test.ts @@ -4034,6 +4034,80 @@ describe("prService.refresh", () => { }); }); + function makeOutageGithubService(message: string) { + return makeGithubService({ + apiRequest: vi.fn(async () => { + throw new Error(message); + }), + }); + } + + it("reports a background sweep that failed because GitHub is unusable", async () => { + // The poller derives its exponential backoff from whether a tick threw. + // The sweep used to run through a best-effort helper that swallowed every + // per-row failure, so an outage in which GitHub refused all of them still + // read as a clean tick and the poller kept its normal cadence for the hour. + const firstRow = makePrRow({ id: "pr-bad-1", github_pr_number: 91 }); + const secondRow = makePrRow({ id: "pr-bad-2", github_pr_number: 92 }); + const { service } = buildService({ + db: makeRefreshDb([firstRow, secondRow]), + githubService: makeOutageGithubService("No server is currently available to service your request."), + }); + + await expect(service.refresh()).rejects.toThrow(/No server is currently available/); + }); + + it("reports a background sweep that failed because GitHub never answered", async () => { + // A very common outage shape is requests that HANG rather than answer. + // Those reject with a bare transport error carrying no classified + // authFailure and matching none of GitHub's 5xx bodies — while each one has + // already been counted against the quota. + const row = makePrRow({ id: "pr-timeout", github_pr_number: 91 }); + const { service } = buildService({ + db: makeRefreshDb([row]), + githubService: makeOutageGithubService( + "GitHub API request timed out. Check network access on this machine.", + ), + }); + + await expect(service.refresh()).rejects.toThrow(/timed out/); + }); + + it("does not report a sweep whose only stale row is individually unreachable", async () => { + // Candidates are the rows whose `last_synced_at` is stale, and a row that + // permanently 404s never refreshes it — so it becomes the ONLY candidate on + // every later sweep. Treating "every row failed" as "GitHub is down" would + // pin a perfectly healthy poller at max backoff forever. + const goneRow = makePrRow({ id: "pr-gone", github_pr_number: 91 }); + const { service, logger } = buildService({ + db: makeRefreshDb([goneRow]), + githubService: makeRefreshGithubService(new Set([91])), + }); + + await expect(service.refresh()).resolves.toEqual(expect.any(Array)); + expect(logger.warn).toHaveBeenCalledWith( + "prs.background_refresh_rows_failed", + expect.objectContaining({ error: "refresh failed for #91" }), + ); + }); + + it("keeps a background sweep successful when GitHub answered for any row", async () => { + // Per-row tolerance is still right: one deleted or inaccessible PR must not + // stop the sweep or put the poller on a backoff. + const okRow = makePrRow({ id: "pr-ok", github_pr_number: 90 }); + const badRow = makePrRow({ id: "pr-bad", github_pr_number: 91 }); + const { service, logger } = buildService({ + db: makeRefreshDb([okRow, badRow]), + githubService: makeRefreshGithubService(new Set([91])), + }); + + await expect(service.refresh()).resolves.toEqual(expect.any(Array)); + expect(logger.warn).toHaveBeenCalledWith("prs.refresh_failed", { + prId: "pr-bad", + error: "refresh failed for #91", + }); + }); + it("still rejects explicit single-PR refresh failures", async () => { const failingRow = makePrRow({ id: "pr-bad", github_pr_number: 91 }); const { service, logger } = buildService({ @@ -4109,6 +4183,66 @@ describe("prService.linkToLane", () => { }); }); +describe("prService.getChecks", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + function makeChecksGithubService(fail: { status?: boolean; checkRuns?: boolean }) { + return makeGithubService({ + apiRequest: vi.fn(async (args: { path: string }) => { + if (args.path === "/repos/test-owner/test-repo/pulls/90") { + return { data: makeGitHubPull({ number: 90, head: { ref: "feature", sha: "head-sha" } }) }; + } + if (args.path === "/repos/test-owner/test-repo/commits/head-sha/status") { + if (fail.status) throw new Error("GitHub API request failed (HTTP 503)"); + return { data: { state: "success", statuses: [] } }; + } + if (args.path === "/repos/test-owner/test-repo/commits/head-sha/check-runs") { + if (fail.checkRuns) throw new Error("GitHub API request failed (HTTP 502)"); + return { data: { check_runs: [] } }; + } + throw new Error(`Unexpected GitHub API path: ${args.path}`); + }), + }); + } + + function buildChecksService(fail: { status?: boolean; checkRuns?: boolean }) { + const row = makePrRow({ id: "pr-checks", github_pr_number: 90 }); + const db = makeMockDb(); + db.get.mockImplementation((sql: string, params: unknown[]) => ( + String(sql).includes("from pull_requests") && String(sql).includes("where id = ?") + ? (params[0] === row.id ? row : null) + : null + )); + db.all.mockImplementation(() => []); + return buildService({ db, githubService: makeChecksGithubService(fail) }); + } + + it("rejects when neither checks source could be read", async () => { + // A failed fetch used to collapse to `[]`, which is byte-identical to "this + // commit has no checks yet". The PR detail pane reads exactly that + // distinction to decide whether CI has settled, so a swallowed failure held + // its 5-second poll open for the whole 2026-08-17 GitHub outage and burned + // the account's entire hourly quota. A total failure must be visible. + const { service } = buildChecksService({ status: true, checkRuns: true }); + await expect(service.getChecks("pr-checks")).rejects.toThrow(/HTTP 50\d/); + }); + + it("returns what it could read when only one source failed", async () => { + // Partial degradation still answers: one working source is more than + // nothing, and blanking the Checks tab because half of GitHub is unhappy + // would remove capability the user still has. + const { service } = buildChecksService({ checkRuns: true }); + await expect(service.getChecks("pr-checks")).resolves.toEqual([]); + }); + + it("reports an authoritatively empty commit as empty, not as a failure", async () => { + const { service } = buildChecksService({}); + await expect(service.getChecks("pr-checks")).resolves.toEqual([]); + }); +}); + describe("prService.getActionRuns", () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 9dde2959c..4458e8458 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -165,6 +165,8 @@ import { isGithubRequestError, markGithubRequestError, } from "./githubReadBackoff"; +import { isGithubServiceUnavailable } from "../../../shared/githubServiceHealth"; +import { githubAuthFailureKindOf, isTransientGithubProbeFailure } from "../github/githubRateLimit"; import { shouldAttemptAdminMergeForRestError } from "./resolverUtils"; import { deletePullRequestRowsByIds } from "./pullRequestRowCleanup"; import { @@ -1225,6 +1227,25 @@ function parseIsoMs(value: string | null | undefined): number { return Number.isFinite(parsed) ? parsed : 0; } +/** + * Whether an error means GitHub is unusable right now, as opposed to one row + * being unreachable. A 404/403 on a single PR is a fact about that PR. + * + * The transport check is not optional: a common outage shape is requests that + * *hang* rather than answer, and those reject with a bare `fetch failed`, + * `ENOTFOUND`, or ADE's own timeout — carrying no classified `authFailure` and + * matching none of GitHub's 5xx bodies — while each one has already been + * counted against the quota. `isTransientGithubProbeFailure` is the same + * predicate `classifyGitHubAuthFailure` uses to produce the `network` kind + * accepted above, so the two cannot disagree. + */ +function isGithubWideFailure(error: unknown): boolean { + const kind = githubAuthFailureKindOf(error); + if (kind === "rate_limited" || kind === "service_unavailable" || kind === "network") return true; + const message = getErrorMessage(error); + return isGithubServiceUnavailable({ message }) || isTransientGithubProbeFailure(message); +} + function isBackgroundRefreshCandidate(row: PullRequestRow, nowMs: number): boolean { const state = String(row.state ?? "").toLowerCase(); const isActive = state === "open" || state === "draft"; @@ -5410,29 +5431,15 @@ export function createPrService({ }); } if (failures[0] && refreshed.length === 0) { - throw failures[0].reason; + // Prefer a reason that says GitHub itself is down. The background sweep's + // caller decides whether to back off from this one error, and a mixed + // batch whose first row happens to be a permanent 404 would otherwise + // hide the outage behind it. + throw (failures.find((failure) => isGithubWideFailure(failure.reason)) ?? failures[0]).reason; } return refreshed; }; - const refreshRowsBestEffort = async (rows: PullRequestRow[]): Promise => { - const seen = new Set(); - const uniqueRows = rows.filter((row) => { - if (seen.has(row.id)) return false; - seen.add(row.id); - return true; - }); - for (let i = 0; i < uniqueRows.length; i += REFRESH_CONCURRENCY) { - await Promise.all(uniqueRows.slice(i, i + REFRESH_CONCURRENCY).map(async (row) => { - try { - await refreshOne(row.id); - } catch (error) { - logger.warn("prs.refresh_failed", { prId: row.id, error: getErrorMessage(error) }); - } - })); - } - }; - /** * Best-effort GraphQL fetch of the authoritative merge box. Returns null on * any failure (permissions, schema drift, transient errors) so callers fall @@ -5704,16 +5711,44 @@ export function createPrService({ return status; }; + /** + * A commit's checks, as GitHub reports them right now. + * + * A total failure rejects, never returns `[]`. The two are indistinguishable + * to every caller — an empty array is also the honest answer for "no checks + * yet" — and the renderer's CI poll uses exactly that distinction to decide + * whether the pipeline has settled, so a swallowed failure reads as CI that + * has not started and holds the poll open indefinitely. Callers that would + * rather show stale checks than nothing catch this, which is now an explicit + * choice at each call site instead of a silent default here. + * + * A *partial* failure still returns: one working source beats nothing, and + * `computeStatus` — not this — owns the rollup verdict that must not be + * recomputed from an incomplete picture. + */ const getChecksByCoords = async (coords: PrGithubCoords): Promise => { const repo: GitHubRepoRef = { owner: coords.repoOwner, name: coords.repoName }; const prNumber = Number(coords.githubPrNumber); const pr = await fetchPr(repo, prNumber); const headSha = asString(pr?.head?.sha); if (!headSha) return rememberActivityInput("checks", repo, prNumber, [] as PrCheck[]); + let combinedStatusError: unknown = null; + let checkRunsError: unknown = null; const [combinedStatus, checkRuns] = await Promise.all([ - bestEffort("getChecks.fetchCombinedStatus", fetchCombinedStatus(repo, headSha), { state: "", statuses: [] }), - bestEffort("getChecks.fetchCheckRuns", fetchCheckRuns(repo, headSha), [] as any[]), + bestEffort( + "getChecks.fetchCombinedStatus", + fetchCombinedStatus(repo, headSha), + { state: "", statuses: [] }, + (error) => { combinedStatusError = error; }, + ), + bestEffort( + "getChecks.fetchCheckRuns", + fetchCheckRuns(repo, headSha), + [] as any[], + (error) => { checkRunsError = error; }, + ), ]); + if (combinedStatusError && checkRunsError) throw checkRunsError; const out: PrCheck[] = []; const seen = new Set(); @@ -11188,7 +11223,23 @@ export function createPrService({ .filter((row) => hotPrIds.has(row.id)) .sort(compareBackgroundRefreshPriority); const candidates = [...hotCandidates, ...staleCandidates]; - await refreshRowsBestEffort(candidates); + // Tolerate a row that fails, but let a GitHub-wide failure surface: the + // sweep used to run through a best-effort helper that swallowed + // everything, so an outage in which every row failed still read as a + // clean tick and `prPollingService` never engaged its backoff. + // + // "Every row failed" alone is NOT that evidence. Candidates are the rows + // whose `last_synced_at` is stale, and a row that permanently 404s (repo + // renamed, fork access lost, PR hard-deleted) never refreshes it — so it + // becomes the only candidate on every later sweep, and an unconditional + // rethrow would pin a perfectly healthy poller at max backoff forever. + // Only a failure that says GitHub itself is unusable counts. + try { + await refreshPrIds(candidates.map((row) => row.id)); + } catch (error) { + if (isGithubWideFailure(error)) throw error; + logger.warn("prs.background_refresh_rows_failed", { error: getErrorMessage(error) }); + } return withGithubStackMemberships(listRows().map(rowToSummary)); }, diff --git a/apps/desktop/src/preload/global.d.ts b/apps/desktop/src/preload/global.d.ts index 84f4d7901..3f3d37f9a 100644 --- a/apps/desktop/src/preload/global.d.ts +++ b/apps/desktop/src/preload/global.d.ts @@ -346,6 +346,7 @@ import type { GitHubAppUserAuthStatus, GitHubAutolink, GitHubRepoRef, + GitHubRequestBudget, GitHubSetTokenResult, GitHubStatus, AdeAccountStatus, @@ -2695,6 +2696,9 @@ declare global { sessionId: string; }) => Promise; clearAppUserAuth: () => Promise; + // Optional: an older remote runtime does not implement the budget read. + // Callers must feature-detect and fall back to their own local backoff. + getRequestBudget?: () => Promise; detectRepo: () => Promise<{ owner: string; name: string } | null>; listRepoAutolinks: (args?: { owner?: string; diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index 202b14b41..f035a87c7 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -277,6 +277,7 @@ import type { GitHubAppUserAuthStatus, GitHubAutolink, GitHubRepoRef, + GitHubRequestBudget, GitHubSetTokenResult, GitHubStatus, AdeAccountStatus, @@ -9135,6 +9136,12 @@ const adeBridge = { : githubStatusCache.get(), ); }, + // Deliberately not cached: a stale "you may proceed" is the answer that + // burned the quota. The read is free, so there is nothing to save. + getRequestBudget: async (): Promise => + callProjectRuntimeActionOr("github", "getRequestBudget", {}, () => + ipcRenderer.invoke(IPC.githubGetRequestBudget), + ), getRemoteStatus: async (opts?: { forceRefresh?: boolean; }): Promise<{ repo: GitHubRepoRef | null; hasOrigin: boolean }> => { diff --git a/apps/desktop/src/renderer/browserMock.ts b/apps/desktop/src/renderer/browserMock.ts index fda44a29d..c4258271b 100644 --- a/apps/desktop/src/renderer/browserMock.ts +++ b/apps/desktop/src/renderer/browserMock.ts @@ -5972,6 +5972,7 @@ if (typeof window !== "undefined" && shouldInstallBrowserMock(window)) { checkedAt: new Date().toISOString(), error: null, }), + getRequestBudget: resolved({ pausedUntil: null, failureKind: null, retryAt: null }), detectRepo: resolved({ owner: "arul28", name: "ADE" }), getAppInstallationStatus: resolved({ repo: { owner: "arul28", name: "ADE" }, diff --git a/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx b/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx index a2e2f24ea..b71401ab8 100644 --- a/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx +++ b/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx @@ -514,8 +514,11 @@ export function PrDetailPane({ setTimelineFilters, setAiSummaryDismissed, regeneratePrAiSummary, - isGithubRateLimited, - noteGithubRateLimit, + isGithubPollStoodDown, + noteGithubReadFailure, + noteGithubReadSuccess, + githubPollPeriodFor, + githubPollGeneration, } = usePrs(); const initialSnapshotHydration = snapshotHydration?.prId === pr.id ? snapshotHydration : null; const initialPaneWarmCache = readDetailPaneWarmCache(pr.id); @@ -1072,13 +1075,6 @@ export function PrDetailPane({ }; }, [activeTab, deepLinkState.eventId, fetchActivity, pr.id, updateDetailPaneWarmCache]); - // Adaptive refresh for the PR detail readiness signals. - // - // Cadence: ~5s while the CI tab is open AND something is still queued or - // running, 60s otherwise. It stops entirely when the window is hidden, and on - // the CI tab once everything is terminal — there is nothing left to observe. - // `getChecks` is included: it used to be missing, so third-party checks - // (CodeRabbit, Vercel, …) never refreshed while the tab was open. const [windowVisible, setWindowVisible] = React.useState( () => (typeof document === "undefined" ? true : document.visibilityState !== "hidden"), ); @@ -1089,17 +1085,35 @@ export function PrDetailPane({ return () => document.removeEventListener("visibilitychange", onVisibility); }, []); + // Adaptive refresh for the PR detail readiness signals. + // + // Cadence: ~5s while the CI tab is open AND something is still queued or + // running, 60s otherwise. It stops entirely when the window is hidden, and on + // the CI tab once everything is terminal — there is nothing left to observe. + // `getChecks` is included: it used to be missing, so third-party checks + // (CodeRabbit, Vercel, …) never refreshed while the tab was open. + // + // The 5s rung is the most expensive loop in the app — roughly seven to ten + // GitHub REST requests per tick, i.e. a whole hourly quota if it runs for an + // hour, which is exactly what happened on 2026-08-17. It is now braked from + // two sides: `getChecks` rejects instead of returning an empty list a failure + // is indistinguishable from, and the shared governor turns any rejection or + // the quota reserve into a longer period for this timer. React.useEffect(() => { if (!windowVisible) return undefined; const checksTabOpen = activeTab === "checks"; if (checksTabOpen && checksTerminal) return undefined; - const periodMs = checksTabOpen && !checksTerminal ? checksPollPeriodMs() : 60_000; + const basePeriodMs = checksTabOpen && !checksTerminal ? checksPollPeriodMs() : 60_000; + // Stretch the timer itself rather than waking every 5s to return early: a + // guard that has to be re-checked on every tick is one refactor away from + // being missed, and a slower interval cannot be. + const periodMs = githubPollPeriodFor(basePeriodMs); let cancelled = false; const id = window.setInterval(() => { - // Honour PrsContext's shared GitHub rate-limit backoff rather than - // hammering the API alongside it. - if (isGithubRateLimited()) return; + // Second line of defence — the period above already reflects the + // stand-down, but a pause armed between ticks lands here. + if (isGithubPollStoodDown()) return; const activityPromise = activeTab === "overview" ? fetchActivity() : Promise.resolve(null); @@ -1110,14 +1124,17 @@ export function PrDetailPane({ fetchChecks(), ]).then(([arResult, thrResult, actResult, checksResult]) => { if (cancelled) return; - for (const result of [arResult, thrResult, actResult, checksResult]) { - if (result.status !== "rejected") continue; - const message = String((result.reason as Error | undefined)?.message ?? result.reason); - if (message.includes("rate limit") || message.includes("API rate")) { - noteGithubRateLimit(); - return; - } + const results = [arResult, thrResult, actResult, checksResult]; + // ANY rejection stands the loop down. The previous message-substring + // test matched neither the 5xx responses of a GitHub outage nor a 403 + // rate-limit body, so the brake never armed while the quota drained. + if (results.some((result) => result.status === "rejected")) { + noteGithubReadFailure(); + } else { + noteGithubReadSuccess(); } + // Whatever DID resolve is still applied: a partial answer keeps the + // pane current instead of freezing it on the last complete one. if (arResult.status === "fulfilled") { setActionRuns(arResult.value); updateDetailPaneWarmCache({ actionRuns: arResult.value }); @@ -1142,7 +1159,8 @@ export function PrDetailPane({ }; }, [ activeTab, checksTerminal, fetchActionRuns, fetchActivity, fetchChecks, fetchReviewThreadsApi, - isGithubRateLimited, noteGithubRateLimit, pr.id, updateDetailPaneWarmCache, windowVisible, + githubPollGeneration, githubPollPeriodFor, isGithubPollStoodDown, noteGithubReadFailure, + noteGithubReadSuccess, pr.id, updateDetailPaneWarmCache, windowVisible, ]); // While GitHub is still computing mergeability for the selected PR, re-poll @@ -1155,28 +1173,51 @@ export function PrDetailPane({ React.useEffect(() => { if (!mergeabilityComputing) return undefined; // Unmapped PRs have a synthetic `gh:` id that getStatus(prId) can't resolve, - // so re-poll them by coords instead. - const pollStatus = (): Promise => { - if (isUnmapped && coordsRef.current && typeof window.ade.prs.getStatusByGithub === "function") { - return window.ade.prs.getStatusByGithub(coordsRef.current); + // so re-poll them by coords instead. With neither reader available there is + // nothing to ask, so the loop never starts — it used to resolve `null` + // without making a request, which then recorded a governor *success* and + // cleared the stand-down for every other loop on the surface. + const readByCoords = isUnmapped && typeof window.ade.prs.getStatusByGithub === "function" + ? window.ade.prs.getStatusByGithub + : null; + const readById = typeof window.ade.prs.getStatus === "function" + ? window.ade.prs.getStatus + : null; + if (!(isUnmapped ? readByCoords : readById)) return undefined; + const pollStatus = (): Promise | null => { + // Re-read the ref at tick time: it is reassigned every render, and a tick + // landing between the render that nulled it and the effect cleanup would + // otherwise pass null straight to the host. + // An unmapped PR asks by coords or asks nothing: its `pr.id` is the + // synthetic `gh:owner/repo#n`, which `getStatus` rejects locally. That + // local rejection would arm the shared stand-down for every other loop on + // the surface — a GitHub brake tripped by something GitHub never saw. + if (isUnmapped) { + const coords = coordsRef.current; + return readByCoords && coords ? readByCoords(coords) : null; } - if (typeof window.ade.prs.getStatus === "function") return window.ade.prs.getStatus(pr.id); - return Promise.resolve(null); + return readById ? readById(pr.id) : null; }; let cancelled = false; - let attempts = 0; - // ~1 minute ceiling either way, then defer to the background poll. - const pollPeriodMs = mergeabilityPollPeriodMs(); - const MAX_ATTEMPTS = Math.round(60_000 / pollPeriodMs); + // Stands down with the governor like every other automatic loop, by + // lengthening its period rather than skipping ticks. The ceiling is + // wall-clock rather than an attempt count: skipped attempts against a + // counter would have turned "~1 minute, then defer to the background poll" + // into hours of a live 2.5s timer during an outage. + const pollPeriodMs = githubPollPeriodFor(mergeabilityPollPeriodMs()); + const deadlineAtMs = Date.now() + 60_000; const seqAtStart = detailLoadSeqRef.current; const id = window.setInterval(() => { - if (attempts >= MAX_ATTEMPTS) { + if (Date.now() >= deadlineAtMs) { window.clearInterval(id); return; } - attempts += 1; - pollStatus() + if (isGithubPollStoodDown()) return; + const request = pollStatus(); + if (!request) return; + request .then((next) => { + noteGithubReadSuccess(); // Drop if cancelled, empty, or a newer detail load superseded us. if (cancelled || !next || seqAtStart !== detailLoadSeqRef.current) return; setPolledStatus(next); @@ -1185,13 +1226,19 @@ export function PrDetailPane({ window.clearInterval(id); } }) - .catch(() => {}); + .catch(() => { + noteGithubReadFailure(); + }); }, pollPeriodMs); return () => { cancelled = true; window.clearInterval(id); }; - }, [mergeabilityComputing, isUnmapped, pr.id, updateDetailPaneWarmCache]); + }, [ + mergeabilityComputing, isUnmapped, githubPollGeneration, githubPollPeriodFor, + isGithubPollStoodDown, noteGithubReadFailure, noteGithubReadSuccess, pr.id, + updateDetailPaneWarmCache, + ]); // ---- Action helper to reduce repetitive try/catch/finally ---- const runAction = async (fn: () => Promise) => { diff --git a/apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx b/apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx index f42e186bc..cc4aea1c6 100644 --- a/apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx +++ b/apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx @@ -6,6 +6,7 @@ import userEvent from "@testing-library/user-event"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { AutoRebaseLaneStatus, + GitHubRequestBudget, PrAiSummary, PrConflictAnalysis, PrDeployment, @@ -1444,3 +1445,237 @@ function makeFakePr(id: string, overrides: Partial = {}): PrWit ...overrides, }; } + +// --------------------------------------------------------------------------- +// GitHub poll governor wiring +// +// `githubPollGovernor.test.ts` proves the ladder maths; these prove the +// provider is actually plumbed to it — that a failed PR read arms it and that +// the runtime's 500-request reserve reaches the foreground timers at all, which +// was the gap that let one open PR detail pane spend 5,001 core requests in an +// hour during the 2026-08-17 GitHub outage. +// --------------------------------------------------------------------------- +const CHECKS_BASE_PERIOD_MS = 5_000; + +function makeGovernorPr(id: string) { + return { + id, + laneId: `lane-${id}`, + projectId: "proj-1", + repoOwner: "octocat", + repoName: "hello-world", + githubPrNumber: 42, + githubUrl: `https://github.com/octocat/hello-world/pull/${id}`, + githubNodeId: `node-${id}`, + title: `PR ${id}`, + state: "open", + baseBranch: "main", + headBranch: `feature/${id}`, + checksStatus: "none", + reviewStatus: "none", + additions: 1, + deletions: 1, + lastSyncedAt: null, + createdAt: "2026-08-17T12:00:00.000Z", + updatedAt: "2026-08-17T12:00:00.000Z", + conflictAnalysis: null, + creationStrategy: "manual", + }; +} + +function governorBudget(overrides: Partial = {}): GitHubRequestBudget { + return { pausedUntil: null, failureKind: null, retryAt: null, ...overrides }; +} + +function installGovernorAde(options: { + getChecks?: ReturnType; + getRequestBudget?: unknown; +}) { + globalThis.window.ade = { + prs: { + refresh: vi.fn().mockResolvedValue(undefined), + listWithConflicts: vi.fn().mockResolvedValue([makeGovernorPr("pr-1"), makeGovernorPr("pr-2")]), + onEvent: vi.fn(() => () => {}), + listSnapshots: vi.fn().mockResolvedValue([]), + getStatus: vi.fn().mockResolvedValue({ state: "open" }), + getChecks: options.getChecks ?? vi.fn().mockResolvedValue([]), + getReviews: vi.fn().mockResolvedValue([]), + getComments: vi.fn().mockResolvedValue([]), + getReviewThreads: vi.fn().mockResolvedValue([]), + getDeployments: vi.fn().mockResolvedValue([]), + getAiSummary: vi.fn().mockResolvedValue(null), + getMergeContext: vi.fn().mockResolvedValue({ + prId: "pr-1", + groupId: null, + groupType: null, + sourceLaneIds: ["lane-pr-1"], + targetLaneId: null, + integrationLaneId: null, + members: [], + }), + }, + github: options.getRequestBudget === undefined + ? {} + : { getRequestBudget: options.getRequestBudget }, + lanes: { + list: vi.fn().mockResolvedValue([]), + listAutoRebaseStatuses: vi.fn().mockResolvedValue([]), + onAutoRebaseEvent: vi.fn(() => () => {}), + onLifecycleEvent: vi.fn(() => () => {}), + }, + rebase: { + scanNeeds: vi.fn().mockResolvedValue([]), + onEvent: vi.fn(() => () => {}), + }, + } as never; +} + +function GovernorHarness() { + const { + isGithubPollStoodDown, + githubPollPeriodFor, + loading, + setSelectedPrId, + selectedPrId, + } = usePrs(); + return ( +
+ + +
{loading ? "loading" : "idle"}
+
{selectedPrId ?? ""}
+
{isGithubPollStoodDown() ? "paused" : "running"}
+
{githubPollPeriodFor(CHECKS_BASE_PERIOD_MS)}
+
+ ); +} + +async function renderHarness(options: Parameters[0]) { + installGovernorAde(options); + render( + + + , + ); + await waitFor(() => { + expect(screen.getByTestId("loading").textContent).toBe("idle"); + }); +} + +describe("PrsContext GitHub poll governor", () => { + afterEach(() => { + cleanup(); + globalThis.window.ade = originalAde; + window.location.hash = ""; + window.history.replaceState(null, "", "/"); + }); + + it("does not hold the fast checks cadence open when the checks read fails", async () => { + // The regression: `getChecks` rejecting during the GitHub outage left the + // pane looking like CI had not started, so the 5s loop kept running at + // ~7-10 requests a tick until the quota was gone. + const getChecks = vi.fn().mockRejectedValue(new Error("GitHub API request failed (HTTP 503)")); + await renderHarness({ getChecks }); + + await userEvent.click(screen.getByRole("button", { name: "select pr-1" })); + + await waitFor(() => { + expect(getChecks).toHaveBeenCalled(); + expect(screen.getByTestId("paused").textContent).toBe("paused"); + }); + expect(Number(screen.getByTestId("period").textContent)) + .toBeGreaterThan(CHECKS_BASE_PERIOD_MS); + }); + + it("keeps the fast cadence while GitHub is answering", async () => { + await renderHarness({}); + await userEvent.click(screen.getByRole("button", { name: "select pr-1" })); + + await waitFor(() => { + expect(screen.getByTestId("selected-pr-id").textContent).toBe("pr-1"); + }); + expect(screen.getByTestId("paused").textContent).toBe("running"); + expect(Number(screen.getByTestId("period").textContent)).toBe(CHECKS_BASE_PERIOD_MS); + }); + + it("stands foreground reads down at the 500-request reserve", async () => { + // The reserve was previously enforced in exactly one place — the background + // PR poller — while every foreground read went straight to + // `githubService.apiRequest` with no gate at all. + const pausedUntil = new Date(Date.now() + 30 * 60_000).toISOString(); + await renderHarness({ getRequestBudget: vi.fn().mockResolvedValue(governorBudget({ pausedUntil })) }); + + await waitFor(() => { + expect(screen.getByTestId("paused").textContent).toBe("paused"); + }); + expect(Number(screen.getByTestId("period").textContent)) + .toBeGreaterThan(CHECKS_BASE_PERIOD_MS); + }); + + it("leaves foreground reads running while quota is healthy", async () => { + const getRequestBudget = vi.fn().mockResolvedValue(governorBudget()); + await renderHarness({ getRequestBudget }); + + // Assert the budget was actually consulted: "running" is also the initial + // state, so without this the test would pass even if it were never read. + await waitFor(() => { + expect(getRequestBudget).toHaveBeenCalled(); + }); + expect(screen.getByTestId("paused").textContent).toBe("running"); + expect(Number(screen.getByTestId("period").textContent)).toBe(CHECKS_BASE_PERIOD_MS); + }); + + it("keeps the quota reserve armed across a successful user-driven read", async () => { + // User actions are ungated on purpose, so their successes reach the + // governor. When the reserve shared one field with the failure ladder, a + // single PR open wiped it and handed the automatic loops their 5s cadence + // back with the quota still below the reserve. + const pausedUntil = new Date(Date.now() + 30 * 60_000).toISOString(); + await renderHarness({ + getRequestBudget: vi.fn().mockResolvedValue(governorBudget({ pausedUntil })), + }); + await waitFor(() => { + expect(screen.getByTestId("paused").textContent).toBe("paused"); + }); + + await userEvent.click(screen.getByRole("button", { name: "select pr-1" })); + await waitFor(() => { + expect(screen.getByTestId("selected-pr-id").textContent).toBe("pr-1"); + }); + + expect(screen.getByTestId("paused").textContent).toBe("paused"); + }); + + it("survives a runtime that cannot answer the budget read", async () => { + // An older remote runtime has no `github.getRequestBudget` action. Losing + // the reserve signal must not also lose the local failure ladder. + const getChecks = vi.fn().mockRejectedValue(new Error("GitHub API request failed (HTTP 503)")); + await renderHarness({ getChecks, getRequestBudget: undefined }); + + await userEvent.click(screen.getByRole("button", { name: "select pr-1" })); + + await waitFor(() => { + expect(screen.getByTestId("paused").textContent).toBe("paused"); + }); + }); + + it("keeps the stand-down when the user selects a different PR", async () => { + // The old backoff was reset on every PR selection, so clicking around a + // stuck tab — the natural reaction — disarmed the only brake. GitHub being + // down is account-wide, not per-PR. Selecting a *different* PR is what + // re-runs the effect that used to clear it. + const getChecks = vi.fn().mockRejectedValue(new Error("GitHub API request failed (HTTP 503)")); + await renderHarness({ getChecks }); + + await userEvent.click(screen.getByRole("button", { name: "select pr-1" })); + await waitFor(() => { + expect(screen.getByTestId("paused").textContent).toBe("paused"); + }); + + await userEvent.click(screen.getByRole("button", { name: "select pr-2" })); + await waitFor(() => { + expect(screen.getByTestId("selected-pr-id").textContent).toBe("pr-2"); + }); + expect(screen.getByTestId("paused").textContent).toBe("paused"); + }); +}); diff --git a/apps/desktop/src/renderer/components/prs/state/PrsContext.tsx b/apps/desktop/src/renderer/components/prs/state/PrsContext.tsx index 303fa1142..602d5f1e9 100644 --- a/apps/desktop/src/renderer/components/prs/state/PrsContext.tsx +++ b/apps/desktop/src/renderer/components/prs/state/PrsContext.tsx @@ -36,6 +36,7 @@ import { resolveRouteRebaseSelection } from "../shared/rebaseNeedUtils"; import { selectActiveProjectRoot, useAppStore } from "../../../state/appStore"; import { refreshPrsCoalesced } from "../../../lib/prReadCache"; import { useDebouncedLaneLifecycleRefresh } from "../../../hooks/useLaneListInvalidation"; +import { useGithubPollGovernor, type GithubPollGovernor } from "./useGithubPollGovernor"; type PrTab = "normal" | "integration" | "rebase"; @@ -152,11 +153,7 @@ type PrsContextValue = PrsState & { setAiSummaryDismissed: (prId: string, dismissed: boolean) => void; regeneratePrAiSummary: (prId: string) => Promise; setViewerLogin: (login: string | null) => void; - /** True while the shared GitHub rate-limit backoff is in effect. */ - isGithubRateLimited: () => boolean; - /** Records a GitHub rate-limit hit, pausing polling for the shared window. */ - noteGithubRateLimit: () => void; -}; +} & GithubPollGovernor; const PrsContext = createContext(null); @@ -165,7 +162,6 @@ const LS_REASONING_KEY = "ade:prs:resolverReasoningLevel"; const LS_PERMISSION_KEY = "ade:prs:resolverPermissions"; const LS_DISMISSED_SUMMARIES_KEY = "ade:prs:dismissedAiSummaries"; const LS_TIMELINE_FILTERS_KEY = "ade:prs:timelineFiltersByPrId"; -const GITHUB_RATE_LIMIT_BACKOFF_MS = 5 * 60_000; const PRS_CONTEXT_CACHE_TTL_MS = 120_000; const PRS_DETAIL_CACHE_TTL_MS = 60_000; const PRS_CONTEXT_DEFAULT_CACHE_KEY = "__default_project__"; @@ -858,23 +854,25 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi }; }, [active, error, refreshCore, refreshErrorRetryCount]); + // One brake for every automatic PR read on this surface. + const { + isGithubPollStoodDown, + noteGithubReadFailure, + noteGithubReadSuccess, + githubPollPeriodFor, + githubPollGeneration, + } = useGithubPollGovernor(active); + // Silently refresh detail data for the given PR (no loading state). // Returns early if a fetch is already in progress or the PR is no longer selected. - const rateLimitedUntilRef = React.useRef(0); - // Shared with any other PR surface that polls GitHub (e.g. the detail pane's - // adaptive checks refresh) so they all back off together. - const isGithubRateLimited = useCallback(() => Date.now() < rateLimitedUntilRef.current, []); - const noteGithubRateLimit = useCallback(() => { - rateLimitedUntilRef.current = Date.now() + GITHUB_RATE_LIMIT_BACKOFF_MS; - }, []); const refreshDetailSilently = useCallback((prId: string) => { if (detailFetchInProgress.current) return; // Bail if the PR we were asked to refresh is no longer the active one if (selectedPrIdRef.current !== prId) return; // Guard: don't fetch details for a PR that's not in the list if (!prsRef.current.some((p) => p.id === prId)) return; - // Skip if we're rate-limited - if (Date.now() < rateLimitedUntilRef.current) return; + // Skip while the shared governor is standing down. + if (isGithubPollStoodDown()) return; detailFetchInProgress.current = true; Promise.allSettled([ @@ -887,18 +885,17 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi // Only apply if this PR is still selected if (selectedPrIdRef.current !== prId) return; - // Check for rate-limit errors in any rejected result - for (const result of [statusResult, checksResult, reviewsResult, commentsResult]) { - if (result.status === "rejected") { - const msg = String(result.reason?.message ?? result.reason); - if (msg.includes("rate limit") || msg.includes("API rate")) { - noteGithubRateLimit(); - console.warn("[PrsContext] GitHub rate limit hit — pausing detail polling for 5 min"); - return; // Don't apply partial results during rate limiting - } - } + const results = [statusResult, checksResult, reviewsResult, commentsResult]; + // ANY rejection arms the governor. Classifying by message here is what + // let a whole-outage of 5xx responses poll at full speed; the kind and + // the quota reserve come from the runtime's request budget instead. + if (results.some((result) => result.status === "rejected")) { + noteGithubReadFailure(); + } else { + noteGithubReadSuccess(); } - if (![statusResult, checksResult, reviewsResult, commentsResult].some((result) => result.status === "fulfilled")) { + if (!results.some((result) => result.status === "fulfilled")) { + // Nothing new to apply. Whatever is on screen stays on screen. return; } @@ -936,12 +933,17 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi .finally(() => { detailFetchInProgress.current = false; }); - }, [noteGithubRateLimit]); - + }, [isGithubPollStoodDown, noteGithubReadFailure, noteGithubReadSuccess]); + + // Reached only from `refresh()`, which is user-driven (the Refresh button and + // post-mutation re-reads). Deliberately NOT gated on the governor: the + // 500-request reserve exists so an explicit user action still works while + // automatic polling stands down, and a manual retry is the escape hatch from + // a stale backoff. It still reports its outcome, so a success here clears the + // ladder for the automatic loops too. const refreshSelectedPrDetail = useCallback(async (prId: string) => { if (selectedPrIdRef.current !== prId) return; if (!prsRef.current.some((p) => p.id === prId)) return; - if (Date.now() < rateLimitedUntilRef.current) return; detailFetchInProgress.current = true; try { @@ -953,15 +955,11 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi ]); if (selectedPrIdRef.current !== prId) return; - for (const result of [statusResult, checksResult, reviewsResult, commentsResult]) { - if (result.status === "rejected") { - const msg = String(result.reason?.message ?? result.reason); - if (msg.includes("rate limit") || msg.includes("API rate")) { - noteGithubRateLimit(); - console.warn("[PrsContext] GitHub rate limit hit — pausing detail polling for 5 min"); - return; - } - } + const results = [statusResult, checksResult, reviewsResult, commentsResult]; + if (results.some((result) => result.status === "rejected")) { + noteGithubReadFailure(); + } else { + noteGithubReadSuccess(); } if (statusResult.status === "fulfilled") setDetailStatus((prev) => (jsonEqual(prev, statusResult.value) ? prev : statusResult.value)); @@ -978,7 +976,7 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi } finally { detailFetchInProgress.current = false; } - }, [noteGithubRateLimit]); + }, [noteGithubReadFailure, noteGithubReadSuccess]); const refresh = useCallback(async (args: PrRefreshArgs = {}) => { const githubRefreshArgs = normalizePrRefreshArgs(args); @@ -996,13 +994,22 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi }, [refreshCore, refreshSelectedPrDetail]); // Load detail data when selected PR changes, then poll every 60s. - // Reset rate-limit backoff on each mount / PR change so stale backoff - // from a previous session doesn't block the first fetch. + // + // The governor is deliberately NOT reset here. It used to be cleared on every + // PR selection, which meant clicking between PRs — the natural thing to do + // when the tab looks stuck — disarmed the one brake on GitHub polling. A + // GitHub outage or an exhausted quota is account-wide, not per-PR, so the + // stand-down has to survive selection. Opening a PR is still a user action + // and its first load runs regardless of the governor; only the repeating + // timers below stand down. + // + // The governor's state lives in the provider, so leaving the PRs tab or + // switching project does still drop the failure ladder. The quota reserve + // self-heals on the next mount (the governor reads the runtime's budget + // immediately), and the ladder re-arms on the first failure, so the worst + // case is one poll group spent re-learning what ADE already knew. useEffect(() => { if (!active) return; - // Reset rate-limit backoff whenever the selected PR changes (including - // on remount) so stale backoff from a previous session is cleared. - rateLimitedUntilRef.current = 0; if (!selectedPrId) { detailStatePrIdRef.current = null; @@ -1094,10 +1101,6 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi setDetailDeployments([]); setDetailAiSummary(null); } - const isPrRateLimitError = (error: unknown): boolean => { - const msg = String((error as { message?: unknown } | null)?.message ?? error); - return msg.includes("rate limit") || msg.includes("API rate"); - }; const yieldToPaint = () => new Promise((resolve) => { const ric = (window as unknown as { @@ -1143,7 +1146,6 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi let primarySettledCount = 0; let primaryFulfilledCount = 0; - let rateLimited = false; const primaryRequestCount = 4; const markPrimarySettled = (fulfilled: boolean) => { primarySettledCount += 1; @@ -1153,11 +1155,18 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi } if (selectedPrIdRef.current === prId && primarySettledCount === primaryRequestCount) { detailFetchInProgress.current = false; - if (!rateLimited && primaryFulfilledCount === primaryRequestCount) { + // Report the group's outcome once, not per piece. The four settle in + // nondeterministic order, so a single piece resolving last (from the + // host's conditional cache, say) would otherwise clear a stand-down + // its three failing siblings had just armed. This matches the + // aggregate rule the repeating loops already use. + if (primaryFulfilledCount === primaryRequestCount) { + noteGithubReadSuccess(); detailStatePrIdRef.current = prId; detailLoadedAtByPrIdRef.current[prId] = Date.now(); setDetailLiveDataPrId(prId); } else { + noteGithubReadFailure(); setDetailLiveDataPrId(null); } } @@ -1171,7 +1180,6 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi promise .then((value) => { if (cancelled || selectedPrIdRef.current !== prId) return; - if (rateLimited) return; fulfilled = true; if (value != null && (!Array.isArray(value) || value.length > 0)) { liveDetailApplied = true; @@ -1181,20 +1189,22 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi }) .catch((error: unknown) => { if (cancelled || selectedPrIdRef.current !== prId) return; - if (isPrRateLimitError(error)) { - rateLimited = true; - noteGithubRateLimit(); - console.warn("[PrsContext] GitHub rate limit hit - pausing detail polling for 5 min"); - if (snapshotForRequest?.prId === prId) { - setDetailStatus(snapshotForRequest.status); - setDetailChecks(snapshotForRequest.checks); - setDetailReviews(snapshotForRequest.reviews); - setDetailComments(snapshotForRequest.comments); - } - setDetailLiveDataPrId(null); - } else { - console.warn(`[PrsContext] Failed to load PR ${name}:`, error); + console.warn(`[PrsContext] Failed to load PR ${name}:`, error); + // Fall back to the cached snapshot for EVERY failure, not just a + // recognised rate limit. A 5xx used to leave the pane empty and + // then keep polling for more of the same; showing what ADE already + // knows is the point of degrading the request rate instead of the + // feature. Only while nothing live has landed yet, though — the + // four pieces settle in any order, and rewinding all four to the + // snapshot because the last one failed would discard fresher data + // its siblings already applied. + if (!liveDetailApplied && snapshotForRequest?.prId === prId) { + setDetailStatus(snapshotForRequest.status); + setDetailChecks(snapshotForRequest.checks); + setDetailReviews(snapshotForRequest.reviews); + setDetailComments(snapshotForRequest.comments); } + setDetailLiveDataPrId(null); setDetailBusy(false); }) .finally(() => { @@ -1216,158 +1226,58 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi setDetailComments(value); }); }; - if (hasFreshDetailCache) { - setDetailLiveDataPrId(prId); - setDetailBusy(false); - startSecondaryDetailFetch(); + // Every path below ends the same way: keep the detail live on a 60s tick. + // This one skips its ticks while the governor is standing down rather than + // lengthening its period — 60s is already at the safe end, so there is no + // request volume to win, and the base cadence is what should resume the + // moment GitHub does. + const startDetailPolling = () => { const intervalId = window.setInterval(() => { refreshDetailSilently(prId); }, 60_000); return () => { cancelled = true; window.clearInterval(intervalId); - rateLimitedUntilRef.current = 0; }; + }; + if (hasFreshDetailCache) { + setDetailLiveDataPrId(prId); + setDetailBusy(false); + startSecondaryDetailFetch(); + return startDetailPolling(); } if (hasFreshSnapshotPrefill) { setDetailBusy(false); startProgressivePrimaryFetch({ background: true }); startSecondaryDetailFetch(); - const intervalId = window.setInterval(() => { - refreshDetailSilently(prId); - }, 60_000); - return () => { - cancelled = true; - window.clearInterval(intervalId); - rateLimitedUntilRef.current = 0; - }; + return startDetailPolling(); } - if (!hasFreshDetailCache && !hasFreshSnapshotPrefill && typeof window.ade.prs.listSnapshots === "function") { - setDetailBusy(true); - void window.ade.prs.listSnapshots({ prId }).then((snapshots) => { - if (cancelled || selectedPrIdRef.current !== prId || liveDetailApplied) return; - const snapshot = snapshots[0]; - if (snapshot) { - applySnapshotPrefill(snapshot); - startProgressivePrimaryFetch({ background: true }); - startSecondaryDetailFetch(); - } else { - startProgressivePrimaryFetch(); - startSecondaryDetailFetch({ reset: true }); - } - }).catch(() => { - if (!cancelled) { - startProgressivePrimaryFetch(); - startSecondaryDetailFetch({ reset: true }); - } - }); - const intervalId = window.setInterval(() => { - refreshDetailSilently(prId); - }, 60_000); - return () => { - cancelled = true; - window.clearInterval(intervalId); - rateLimitedUntilRef.current = 0; - }; - } - + // A caller without `listSnapshots` (an older adapter or a partial test + // stub) is treated as one that answered "no snapshots". setDetailBusy(true); - detailFetchInProgress.current = true; - - let primarySettledCount = 0; - let primaryFulfilledCount = 0; - let rateLimited = false; - const primaryRequestCount = 4; - const markPrimarySettled = (fulfilled: boolean) => { - primarySettledCount += 1; - if (fulfilled) primaryFulfilledCount += 1; - if (selectedPrIdRef.current === prId && primarySettledCount === 1) { - detailFetchInProgress.current = false; + const listSnapshots = window.ade.prs.listSnapshots; + const snapshotsPromise = typeof listSnapshots === "function" + ? listSnapshots({ prId }) + : Promise.resolve([]); + void snapshotsPromise.then((snapshots) => { + if (cancelled || selectedPrIdRef.current !== prId || liveDetailApplied) return; + const snapshot = snapshots[0]; + if (snapshot) { + applySnapshotPrefill(snapshot); + startProgressivePrimaryFetch({ background: true }); + startSecondaryDetailFetch(); + } else { + startProgressivePrimaryFetch(); + startSecondaryDetailFetch({ reset: true }); } - if (selectedPrIdRef.current === prId && primarySettledCount === primaryRequestCount) { - detailFetchInProgress.current = false; - setDetailLiveDataPrId(!rateLimited && primaryFulfilledCount > 0 ? prId : null); + }).catch(() => { + if (!cancelled) { + startProgressivePrimaryFetch(); + startSecondaryDetailFetch({ reset: true }); } - }; - const isRateLimitError = (error: unknown): boolean => { - const msg = String((error as { message?: unknown } | null)?.message ?? error); - return msg.includes("rate limit") || msg.includes("API rate"); - }; - const loadPrimaryPiece = ( - name: string, - promise: Promise, - apply: (value: T) => void, - ) => { - let fulfilled = false; - promise - .then((value) => { - if (cancelled || selectedPrIdRef.current !== prId) return; - if (rateLimited) return; - fulfilled = true; - if (value != null && (!Array.isArray(value) || value.length > 0)) { - liveDetailApplied = true; - } - detailStatePrIdRef.current = prId; - detailLoadedAtByPrIdRef.current[prId] = Date.now(); - apply(value); - setDetailLiveDataPrId(prId); - setDetailBusy(false); - }) - .catch((error: unknown) => { - if (cancelled || selectedPrIdRef.current !== prId) return; - if (isRateLimitError(error)) { - rateLimited = true; - noteGithubRateLimit(); - console.warn("[PrsContext] GitHub rate limit hit — pausing detail polling for 5 min"); - if (snapshotForRequest?.prId === prId) { - setDetailStatus(snapshotForRequest.status); - setDetailChecks(snapshotForRequest.checks); - setDetailReviews(snapshotForRequest.reviews); - setDetailComments(snapshotForRequest.comments); - } - setDetailLiveDataPrId(null); - } else { - console.warn(`[PrsContext] Failed to load PR ${name}:`, error); - } - setDetailBusy(false); - }) - .finally(() => { - if (cancelled) return; - markPrimarySettled(fulfilled); - }); - }; - - loadPrimaryPiece("status", window.ade.prs.getStatus(prId), (value) => { - setDetailStatus(value ?? null); - }); - loadPrimaryPiece("checks", window.ade.prs.getChecks(prId), (value) => { - setDetailChecks(value); - }); - loadPrimaryPiece("reviews", window.ade.prs.getReviews(prId), (value) => { - setDetailReviews(value); }); - loadPrimaryPiece("comments", window.ade.prs.getComments(prId), (value) => { - setDetailComments(value); - }); - - // Progressive secondary fetch (deployments, AI summary) — yields - // to the main paint so the primary header + checks render first. - startSecondaryDetailFetch({ reset: true }); - - // After the initial fetch, poll every 60 seconds for fresh detail data. - // GitHub rate limit is 5000/hour (~83/min) and each detail refresh uses ~10 API calls, - // so polling faster than 60s risks exhausting the rate limit. - const intervalId = window.setInterval(() => { - refreshDetailSilently(prId); - }, 60_000); - - return () => { - cancelled = true; - window.clearInterval(intervalId); - // Reset rate-limit backoff on cleanup so remounts start fresh - rateLimitedUntilRef.current = 0; - }; - }, [active, noteGithubRateLimit, refreshDetailSilently, selectedPrId]); + return startDetailPolling(); + }, [active, noteGithubReadFailure, noteGithubReadSuccess, refreshDetailSilently, selectedPrId]); useEffect(() => { if (!active || !selectedPrId) return; @@ -1593,8 +1503,11 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi setAiSummaryDismissed, regeneratePrAiSummary, setViewerLogin, - isGithubRateLimited, - noteGithubRateLimit, + isGithubPollStoodDown, + noteGithubReadFailure, + noteGithubReadSuccess, + githubPollPeriodFor, + githubPollGeneration, }), // Note: setActiveTab, setSelectedPrId, setSelectedRebaseItemId, // setMergeMethod, setInlineTerminal, and setViewerLogin are intentionally excluded from this dependency @@ -1641,8 +1554,11 @@ export function PrsProvider({ active = true, children }: { active?: boolean; chi setTimelineFilters, setAiSummaryDismissed, regeneratePrAiSummary, - isGithubRateLimited, - noteGithubRateLimit, + isGithubPollStoodDown, + noteGithubReadFailure, + noteGithubReadSuccess, + githubPollPeriodFor, + githubPollGeneration, ], ); diff --git a/apps/desktop/src/renderer/components/prs/state/githubPollGovernor.test.ts b/apps/desktop/src/renderer/components/prs/state/githubPollGovernor.test.ts new file mode 100644 index 000000000..4b67e60fd --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/state/githubPollGovernor.test.ts @@ -0,0 +1,263 @@ +import { describe, expect, it } from "vitest"; +import type { GitHubRequestBudget } from "../../../../shared/types"; +import { + GITHUB_POLL_BACKOFF_BASE_MS, + GITHUB_POLL_BACKOFF_CONFIRMED_BROKEN_BASE_MS, + GITHUB_POLL_BACKOFF_MAX_MS, + applyGithubRequestBudget, + githubPollBackoffMs, + githubPollPeriodMs, + initialGithubPollGovernorState, + isGithubPollPaused, + noteGithubPollFailure, + noteGithubPollSuccess, +} from "./githubPollGovernor"; + +const NOW = Date.parse("2026-08-17T12:00:00.000Z"); +const BASE_PERIOD_MS = 5_000; + +function budget(overrides: Partial = {}): GitHubRequestBudget { + return { pausedUntil: null, failureKind: null, retryAt: null, ...overrides }; +} + +function periodFor(state: Parameters[0], nowMs = NOW): number { + return githubPollPeriodMs(state, BASE_PERIOD_MS, nowMs); +} + +describe("githubPollGovernor", () => { + it("does not hold the fast poll open after any rejection, classified or not", () => { + // The whole regression in one assertion. The old brake only fired on a + // message containing "rate limit", so the 5xx responses of the outage + // polled at full speed; and an errored `getChecks` was indistinguishable + // from "CI has not started yet", so the pane's stop condition never fired. + let state = initialGithubPollGovernorState; + expect(periodFor(state)).toBe(BASE_PERIOD_MS); + + state = noteGithubPollFailure(state, NOW); + expect(isGithubPollPaused(state, NOW)).toBe(true); + expect(periodFor(state)).toBe(GITHUB_POLL_BACKOFF_BASE_MS); + }); + + it("does not climb the ladder for failures GitHub was never proven to have seen", () => { + // A runtime reconnect, an IPC blip, or a local "PR not found" all surface + // as a bare rejection. They should cost one rung, not five minutes of + // staleness on the surface whose whole value is liveness. + let state = initialGithubPollGovernorState; + for (let i = 0; i < 10; i += 1) state = noteGithubPollFailure(state, NOW); + expect(periodFor(state)).toBe(GITHUB_POLL_BACKOFF_BASE_MS); + }); + + it("climbs and caps once the failure is attributed to GitHub", () => { + let state = applyGithubRequestBudget( + noteGithubPollFailure(initialGithubPollGovernorState, NOW), + budget({ failureKind: "service_unavailable" }), + NOW, + ); + expect(periodFor(state)).toBe(GITHUB_POLL_BACKOFF_CONFIRMED_BROKEN_BASE_MS); + + for (let i = 0; i < 20; i += 1) state = noteGithubPollFailure(state, NOW); + expect(periodFor(state)).toBe(GITHUB_POLL_BACKOFF_MAX_MS); + }); + + it("doubles per consecutive attributed failure and caps", () => { + expect(githubPollBackoffMs(0, "service_unavailable")).toBe(0); + expect(githubPollBackoffMs(1, "service_unavailable")) + .toBe(GITHUB_POLL_BACKOFF_CONFIRMED_BROKEN_BASE_MS); + expect(githubPollBackoffMs(2, "service_unavailable")) + .toBe(GITHUB_POLL_BACKOFF_CONFIRMED_BROKEN_BASE_MS * 2); + expect(githubPollBackoffMs(50, "service_unavailable")).toBe(GITHUB_POLL_BACKOFF_MAX_MS); + // A named rate limit with no reset instant goes straight to the ceiling. + expect(githubPollBackoffMs(1, "rate_limited")).toBe(GITHUB_POLL_BACKOFF_MAX_MS); + }); + + it("never shortens an armed stand-down when a parallel request also fails", () => { + const resetAtMs = NOW + 45 * 60_000; + const armed = applyGithubRequestBudget( + noteGithubPollFailure(initialGithubPollGovernorState, NOW), + budget({ failureKind: "rate_limited", retryAt: new Date(resetAtMs).toISOString() }), + NOW, + ); + expect(armed.ladderPausedUntilMs).toBe(resetAtMs); + expect(noteGithubPollFailure(armed, NOW).ladderPausedUntilMs).toBe(resetAtMs); + }); + + it("clears the ladder on the first success", () => { + let state = noteGithubPollFailure(initialGithubPollGovernorState, NOW); + state = noteGithubPollFailure(state, NOW); + state = noteGithubPollSuccess(state, NOW); + expect(state).toEqual(initialGithubPollGovernorState); + expect(isGithubPollPaused(state, NOW)).toBe(false); + }); + + it("returns the same object when a success finds nothing to clear", () => { + // Referential stability matters: the hook bumps a render generation when + // the stand-down changes, and a healthy poll must not churn it. + expect(noteGithubPollSuccess(initialGithubPollGovernorState, NOW)) + .toBe(initialGithubPollGovernorState); + }); + + describe("quota reserve", () => { + const pausedUntil = new Date(NOW + 30 * 60_000).toISOString(); + + it("stands the loop down when the reserve is armed", () => { + // The 500-request reserve was enforced in exactly one place — the + // background poller — while the foreground loops that actually drained + // the quota ignored it entirely. + const state = applyGithubRequestBudget( + initialGithubPollGovernorState, + budget({ pausedUntil }), + NOW, + ); + expect(isGithubPollPaused(state, NOW)).toBe(true); + expect(periodFor(state)).toBe(GITHUB_POLL_BACKOFF_MAX_MS); + }); + + it("is dropped by a success once it has elapsed, without needing the budget", () => { + // The drop also lives in `applyGithubRequestBudget`, but recovery must + // not depend on the budget action still answering (a runtime restart, a + // downgraded host) — otherwise the timer keeps its stretched period. + const reserved = applyGithubRequestBudget( + initialGithubPollGovernorState, + budget({ pausedUntil }), + NOW, + ); + const afterResetMs = Date.parse(pausedUntil) + 1; + const recovered = noteGithubPollSuccess(reserved, afterResetMs); + expect(recovered.reservePausedUntilMs).toBe(0); + expect(recovered).not.toBe(reserved); + }); + + it("survives a success, because a request does not refill the quota", () => { + // User actions are ungated on purpose, so their successes reach the + // governor. When the reserve shared one field with the failure ladder, + // every PR open or Refresh click wiped it and handed the automatic loops + // their 5-second cadence back with the quota still under 500 — leaking + // the reserve roughly a minute at a time, repeatably. + const reserved = applyGithubRequestBudget( + initialGithubPollGovernorState, + budget({ pausedUntil }), + NOW, + ); + const afterSuccess = noteGithubPollSuccess(reserved, NOW); + expect(isGithubPollPaused(afterSuccess, NOW)).toBe(true); + expect(afterSuccess.reservePausedUntilMs).toBe(Date.parse(pausedUntil)); + }); + + it("drops an elapsed reserve so the caller's cadence visibly recovers", () => { + // Readers already treat a past instant as "not paused", but the hook only + // rebuilds a timer at the faster cadence when a field CHANGES. Carried + // monotonically, this field never changed after the reset, so a pane that + // stood down at five minutes stayed there for the rest of the session on + // a healthy GitHub — degrading without ever coming back. + const reserved = applyGithubRequestBudget( + initialGithubPollGovernorState, + budget({ pausedUntil }), + NOW, + ); + expect(reserved.reservePausedUntilMs).toBe(Date.parse(pausedUntil)); + + const afterResetMs = Date.parse(pausedUntil) + 1; + const recovered = applyGithubRequestBudget(reserved, budget(), afterResetMs); + expect(recovered.reservePausedUntilMs).toBe(0); + expect(recovered).not.toBe(reserved); + expect(periodFor(recovered, afterResetMs)).toBe(BASE_PERIOD_MS); + }); + + it("lifts by itself at the quota reset", () => { + const state = applyGithubRequestBudget( + initialGithubPollGovernorState, + budget({ pausedUntil }), + NOW, + ); + const afterResetMs = Date.parse(pausedUntil) + 1; + expect(isGithubPollPaused(state, afterResetMs)).toBe(false); + expect(periodFor(state, afterResetMs)).toBe(BASE_PERIOD_MS); + }); + + it("lets automatic reads proceed while quota is healthy", () => { + const state = applyGithubRequestBudget(initialGithubPollGovernorState, budget(), NOW); + expect(isGithubPollPaused(state, NOW)).toBe(false); + expect(periodFor(state)).toBe(BASE_PERIOD_MS); + }); + }); + + describe("typed failure kind", () => { + it("re-derives the ladder once the kind is known", () => { + // The kind cannot ride on the rejection — IPC flattens an error to its + // message — so it arrives here, from the credential health recorded in + // the process that made the request. + const failed = noteGithubPollFailure(initialGithubPollGovernorState, NOW); + expect(failed.ladderPausedUntilMs).toBe(NOW + GITHUB_POLL_BACKOFF_BASE_MS); + + const classified = applyGithubRequestBudget( + failed, + budget({ failureKind: "service_unavailable" }), + NOW, + ); + expect(classified.failureKind).toBe("service_unavailable"); + expect(classified.ladderPausedUntilMs) + .toBe(NOW + GITHUB_POLL_BACKOFF_CONFIRMED_BROKEN_BASE_MS); + }); + + it("waits for the reset instant GitHub named rather than guessing", () => { + const resetAtMs = NOW + 42 * 60_000; + const state = applyGithubRequestBudget( + noteGithubPollFailure(initialGithubPollGovernorState, NOW), + budget({ failureKind: "rate_limited", retryAt: new Date(resetAtMs).toISOString() }), + NOW, + ); + expect(state.ladderPausedUntilMs).toBe(resetAtMs); + }); + + it("never clears an armed ladder just because quota looks fine", () => { + const failed = noteGithubPollFailure(initialGithubPollGovernorState, NOW); + const applied = applyGithubRequestBudget(failed, budget(), NOW); + expect(applied.ladderPausedUntilMs).toBe(failed.ladderPausedUntilMs); + }); + + it("does not put a healthy loop on a ladder for another surface's failure", () => { + const applied = applyGithubRequestBudget( + initialGithubPollGovernorState, + budget({ failureKind: "service_unavailable" }), + NOW, + ); + expect(isGithubPollPaused(applied, NOW)).toBe(false); + expect(applied.failureKind).toBeNull(); + }); + + it("does not let a late budget response resurrect a kind a success cleared", () => { + const cleared = noteGithubPollSuccess( + noteGithubPollFailure(initialGithubPollGovernorState, NOW), + NOW, + ); + const applied = applyGithubRequestBudget( + cleared, + budget({ failureKind: "service_unavailable" }), + NOW, + ); + expect(applied.failureKind).toBeNull(); + expect(isGithubPollPaused(applied, NOW)).toBe(false); + }); + + it("is inert when the runtime cannot answer", () => { + // An older remote runtime has no budget action. Callers must keep their + // own local ladder rather than losing the brake entirely. + const failed = noteGithubPollFailure(initialGithubPollGovernorState, NOW); + expect(applyGithubRequestBudget(failed, null, NOW)).toBe(failed); + expect(applyGithubRequestBudget(failed, undefined, NOW)).toBe(failed); + }); + }); + + it("returns to the base cadence once the stand-down elapses", () => { + const state = noteGithubPollFailure(initialGithubPollGovernorState, NOW); + const afterMs = state.ladderPausedUntilMs + 1; + expect(isGithubPollPaused(state, afterMs)).toBe(false); + expect(periodFor(state, afterMs)).toBe(BASE_PERIOD_MS); + }); + + it("never polls faster than the caller's own base cadence", () => { + const state = noteGithubPollFailure(initialGithubPollGovernorState, NOW); + // A 5-minute base loop must not be sped up to 30s by a short stand-down. + expect(githubPollPeriodMs(state, 5 * 60_000, NOW)).toBe(5 * 60_000); + }); +}); diff --git a/apps/desktop/src/renderer/components/prs/state/githubPollGovernor.ts b/apps/desktop/src/renderer/components/prs/state/githubPollGovernor.ts new file mode 100644 index 000000000..fa07727cc --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/state/githubPollGovernor.ts @@ -0,0 +1,274 @@ +import type { GitHubAuthFailureKind, GitHubRequestBudget } from "../../../../shared/types"; + +/** + * The shared brake on every *automatic* GitHub read the PRs surface makes. + * + * Why this exists, concretely. On 2026-08-17 GitHub had a multi-hour outage. + * ADE's PR detail pane polls readiness signals every 5 seconds while the Checks + * tab is open and something is still running, and each tick costs roughly seven + * to ten REST requests (a pull, an Actions runs page, up to twelve job reads, a + * combined status, a check-runs page). That is 5,000+ requests an hour — the + * whole quota — and two independent defects let it run the full hour: + * + * 1. The loop's stop condition is "at least one check exists and all of them + * settled". A failed checks fetch was swallowed into an empty array, which + * is byte-identical to "CI has not started yet", so the loop stayed in its + * fast cadence forever instead of the ~10 minutes a real CI run takes. + * 2. The only brake was `msg.includes("rate limit") || msg.includes("API rate")` + * on the rejection message. Every response during the outage was a 5xx, + * which matches neither substring, so the backoff never armed — while every + * failed request still consumed quota. + * + * The governor replaces both with typed state: + * + * - **Any** rejected automatic read arms an exponential stand-down, so a + * failure ADE cannot classify still slows the loop down. There is no + * substring anywhere. + * - The classified {@link GitHubAuthFailureKind} chooses the *first* rung: a + * corroborated GitHub outage starts further out than a one-off blip, because + * retrying an outage in 30 seconds is a request spent on nothing. + * - {@link GitHubRequestBudget.pausedUntil} carries the 500-request reserve the + * background PR poller already honours. Foreground timers stand down at the + * same line and resume by themselves at the quota reset, which is what keeps + * the reserve available for the merge the user is actually trying to do. + * + * What it deliberately does NOT do is stop. Every rung is a longer cadence, not + * a dead loop, and nothing here clears, hides, or blanks data — a paused poll + * leaves the last-known checks, activity, and threads on screen. ADE showing + * what it already knows is the whole point of degrading the request rate + * instead of the feature. + */ + +/** First stand-down after a single failure ADE could not attribute to GitHub. */ +export const GITHUB_POLL_BACKOFF_BASE_MS = 30_000; +/** + * First stand-down when GitHub has given a definite answer that a fast retry + * cannot change: GitHub itself is failing (5xx), or it rejected the credential. + * During an incident the fast retry is the thing burning the quota. + */ +export const GITHUB_POLL_BACKOFF_CONFIRMED_BROKEN_BASE_MS = 60_000; +/** + * Ceiling for the ladder. Matches `prPollingService`'s `MAX_INTERVAL_MS`, so a + * degraded foreground never asks GitHub harder than the degraded background. + */ +export const GITHUB_POLL_BACKOFF_MAX_MS = 5 * 60_000; + +export type GithubPollGovernorState = { + /** Consecutive failed automatic reads. Zero after any success. */ + consecutiveFailures: number; + /** + * Epoch ms the failure ladder is standing down until. Cleared by a success, + * because GitHub answering is proof the ladder's premise no longer holds. + */ + ladderPausedUntilMs: number; + /** + * Epoch ms the 500-request quota reserve is standing down until — tracked + * SEPARATELY from the ladder, and deliberately NOT cleared by a success. + * + * They were one field first, and that quietly leaked the reserve: a user + * action (opening a PR, hitting Refresh) is ungated on purpose, so its + * success reset the whole governor and every automatic loop went back to its + * 5-second cadence with the quota still below 500. The reserve is a fact + * about how much quota is left, which a successful request does not change — + * only the quota reset does, and the budget reports that instant. + */ + reservePausedUntilMs: number; + /** Why the governor last stood down, for the cadence decision and logging. */ + failureKind: GitHubAuthFailureKind | null; +}; + +export const initialGithubPollGovernorState: GithubPollGovernorState = { + consecutiveFailures: 0, + ladderPausedUntilMs: 0, + reservePausedUntilMs: 0, + failureKind: null, +}; + +/** The later of the two independent stand-downs. */ +function standDownUntilMs(state: GithubPollGovernorState): number { + return Math.max(state.ladderPausedUntilMs, state.reservePausedUntilMs); +} + +/** + * How long the FIRST stand-down lasts, by how definite GitHub's answer was. + * + * This ordering is a contract with `REQUEST_BUDGET_FAILURE_SEVERITY` in + * `main/services/github/githubCredentialHealth.ts`: when several credentials + * have each recorded a different failure, the budget reports the one whose kind + * asks for the longest stand-down, and it can only do that if its severity + * ranking agrees with this function. Change one, change both. + */ +function ladderBaseMs(kind: GitHubAuthFailureKind | null): number { + switch (kind) { + case "rate_limited": + // No ladder: a rate limit with no named reset goes straight to the + // ceiling. GitHub's guidance is to stop until the quota resets, and when + // it named a reset instant `noteGithubPollFailure` waits for that instead. + return GITHUB_POLL_BACKOFF_MAX_MS; + case "service_unavailable": + case "invalid_token": + case "permission_denied": + return GITHUB_POLL_BACKOFF_CONFIRMED_BROKEN_BASE_MS; + default: + // `network` and `unknown`: ADE does not know that a fast retry is + // pointless, so it stays on the short base. + return GITHUB_POLL_BACKOFF_BASE_MS; + } +} + +/** + * `base * 2^(n-1)`, capped. The cap is what makes this safe to leave running + * for hours: at the ceiling a stuck PR detail pane costs at most twelve poll + * groups an hour instead of seven hundred and twenty. + */ +export function githubPollBackoffMs( + consecutiveFailures: number, + kind: GitHubAuthFailureKind | null, +): number { + if (consecutiveFailures <= 0) return 0; + const base = ladderBaseMs(kind); + // An unclassified failure does not climb. ADE does not know GitHub was even + // reached — a runtime reconnect, an IPC blip, or a local "PR not found" all + // land here — and punishing those up to the five-minute ceiling would cost + // liveness on the one surface whose whole value is liveness. A failure the + // budget later attributes to GitHub climbs normally, because by then ADE + // knows what it is looking at. + if (kind == null) return base; + const grown = base * Math.pow(2, Math.min(consecutiveFailures - 1, 8)); + return Math.min(grown, GITHUB_POLL_BACKOFF_MAX_MS); +} + +function parseIsoMs(value: string | null | undefined): number | null { + if (!value) return null; + const parsed = Date.parse(value); + return Number.isFinite(parsed) ? parsed : null; +} + +/** + * Record a failed automatic read. + * + * It takes no failure kind on purpose. The caller only ever has a bare + * rejection — both transports between it and GitHub flatten an error to its + * message — so a `kind` parameter here could only ever be filled by the + * substring guessing this module exists to delete. The kind arrives afterwards, + * as data, through {@link applyGithubRequestBudget}, and the ladder is + * re-derived then. Until it does, the short base applies, which is the + * conservative direction: a real outage gets longer, never shorter. + */ +export function noteGithubPollFailure( + state: GithubPollGovernorState, + nowMs: number, +): GithubPollGovernorState { + const consecutiveFailures = state.consecutiveFailures + 1; + return { + ...state, + consecutiveFailures, + // Never shorten an existing stand-down: a second failure arriving from a + // parallel request must not reset a longer pause to a shorter one. + ladderPausedUntilMs: Math.max( + state.ladderPausedUntilMs, + nowMs + githubPollBackoffMs(consecutiveFailures, state.failureKind), + ), + }; +} + +/** + * Record a successful automatic read. GitHub answering is the fact that + * matters, so one success clears the whole ladder — the same rule the service's + * `githubReadBackoff` uses. + */ +export function noteGithubPollSuccess( + state: GithubPollGovernorState, + nowMs: number, +): GithubPollGovernorState { + // A still-future reserve survives on purpose — see `reservePausedUntilMs` — + // but an elapsed one is dropped here as well as in `applyGithubRequestBudget`, + // so recovery never depends on the budget action still answering. + const reservePausedUntilMs = state.reservePausedUntilMs > nowMs ? state.reservePausedUntilMs : 0; + if ( + state.consecutiveFailures === 0 + && state.ladderPausedUntilMs === 0 + && state.failureKind === null + && reservePausedUntilMs === state.reservePausedUntilMs + ) { + return state; + } + return { ...initialGithubPollGovernorState, reservePausedUntilMs }; +} + +/** + * Fold the runtime's request budget into the governor: the quota reserve, and + * the typed failure kind that a bare rejection could not carry. + * + * One-directional. A budget can only ever *extend* a stand-down — "quota is + * fine" says nothing about whether GitHub is answering, so it never clears a + * ladder armed by observed failures. + */ +export function applyGithubRequestBudget( + state: GithubPollGovernorState, + budget: GitHubRequestBudget | null | undefined, + nowMs: number, +): GithubPollGovernorState { + if (!budget) return state; + // Only failures ADE actually observed put it on a ladder, so a kind reported + // while this surface has none of its own is another surface's problem — and, + // importantly, a budget response still in flight when a success cleared the + // ladder must not resurrect the kind it carried. + const observedFailure = state.consecutiveFailures > 0; + const failureKind = observedFailure ? budget.failureKind ?? state.failureKind : null; + // Re-derive only when the kind actually CHANGED. Re-deriving on an unchanged + // kind would re-arm from `nowMs` on every 60s budget poll and push the pause + // out forever — a livelock that never lets the loop retry. + const ladderUntilMs = observedFailure && failureKind !== state.failureKind + ? nowMs + githubPollBackoffMs(state.consecutiveFailures, failureKind) + : 0; + // The one place a GitHub-named reset instant is honoured. GitHub's guidance + // is explicit: when `x-ratelimit-remaining` is 0, do not make another request + // until `x-ratelimit-reset`. Waiting exactly that long beats guessing. + const retryAtMs = failureKind === "rate_limited" ? parseIsoMs(budget.retryAt) ?? 0 : 0; + // Drop a reserve that has already elapsed instead of carrying it forward. + // Every reader already treats a past instant as "not paused", but the hook + // only re-renders — and so only rebuilds a timer at the faster cadence — when + // one of these fields *changes*. Carried monotonically, the field never + // changed after the quota reset, so a pane that stood down at 5 minutes + // stayed there for the rest of the session on a healthy GitHub. Degrading and + // not coming back is the one failure mode this whole module exists to avoid. + const carriedReserveMs = state.reservePausedUntilMs > nowMs ? state.reservePausedUntilMs : 0; + const next: GithubPollGovernorState = { + ...state, + failureKind, + reservePausedUntilMs: Math.max(carriedReserveMs, parseIsoMs(budget.pausedUntil) ?? 0), + ladderPausedUntilMs: Math.max(state.ladderPausedUntilMs, ladderUntilMs, retryAtMs), + }; + if ( + next.failureKind === state.failureKind + && next.reservePausedUntilMs === state.reservePausedUntilMs + && next.ladderPausedUntilMs === state.ladderPausedUntilMs + ) return state; + return next; +} + +export function isGithubPollPaused( + state: GithubPollGovernorState, + nowMs: number, +): boolean { + return standDownUntilMs(state) > nowMs; +} + +/** + * The interval an automatic PR read loop should actually run at. + * + * Returning a longer *period* rather than skipping ticks matters: a 5-second + * timer that returns early still wakes the renderer 720 times an hour, and — as + * the outage showed — a single missing guard turns each of those wakeups back + * into a request. Slowing the timer removes the opportunity entirely. + */ +export function githubPollPeriodMs( + state: GithubPollGovernorState, + basePeriodMs: number, + nowMs: number, +): number { + const remainingPauseMs = standDownUntilMs(state) - nowMs; + if (remainingPauseMs <= 0) return basePeriodMs; + return Math.max(basePeriodMs, Math.min(remainingPauseMs, GITHUB_POLL_BACKOFF_MAX_MS)); +} diff --git a/apps/desktop/src/renderer/components/prs/state/useGithubPollGovernor.ts b/apps/desktop/src/renderer/components/prs/state/useGithubPollGovernor.ts new file mode 100644 index 000000000..a6bd8915e --- /dev/null +++ b/apps/desktop/src/renderer/components/prs/state/useGithubPollGovernor.ts @@ -0,0 +1,126 @@ +import { useCallback, useEffect, useRef, useState } from "react"; +import { + applyGithubRequestBudget, + githubPollPeriodMs, + initialGithubPollGovernorState, + isGithubPollPaused, + noteGithubPollFailure, + noteGithubPollSuccess, + type GithubPollGovernorState, +} from "./githubPollGovernor"; + +/** + * How often the renderer re-reads the runtime's GitHub request budget while the + * PRs surface is active. The read is zero-network (it inspects in-memory + * credential health), so this only bounds how stale the reserve signal can be; + * a failed read refreshes it immediately rather than waiting for this tick. + */ +const GITHUB_REQUEST_BUDGET_REFRESH_MS = 60_000; + +export type GithubPollGovernor = { + /** + * True while automatic GitHub reads must stand down — because a read failed, + * or because the runtime's 500-request quota reserve is armed. User-initiated + * actions deliberately do not check this, so the reserve stays available for + * the work the user came to do. + */ + isGithubPollStoodDown: () => boolean; + /** + * Records a failed automatic GitHub read. Called for ANY rejection — the + * classified failure kind cannot ride on the error (both transports between + * here and GitHub flatten it to a message) and arrives separately through the + * runtime's request budget. + */ + noteGithubReadFailure: () => void; + /** Records a successful automatic GitHub read, clearing the ladder. */ + noteGithubReadSuccess: () => void; + /** + * Poll period an automatic PR read loop should use right now: its own base + * cadence while GitHub is healthy, the governor's stand-down while it is not. + */ + githubPollPeriodFor: (basePeriodMs: number) => number; + /** + * Bumped whenever the stand-down changes, so timer effects can depend on it + * and rebuild their interval at the new cadence. + */ + githubPollGeneration: number; +}; + +/** + * One shared brake for every automatic PR read on the PRs surface — the + * provider's own 60s detail refresh and the detail pane's adaptive checks and + * mergeability loops — so they stand down together instead of each hammering a + * GitHub that is already refusing. + * + * The state lives in a ref rather than React state because the loops read it + * inside interval callbacks, where a stale closure would silently reinstate the + * unbraked behaviour. `githubPollGeneration` is the only part rendered, and it + * changes only when the stand-down does, so a bumped failure count cannot + * re-render every PR consumer. + * + * See `githubPollGovernor.ts` for the incident this exists to prevent. + */ +export function useGithubPollGovernor(active: boolean): GithubPollGovernor { + const stateRef = useRef(initialGithubPollGovernorState); + const [githubPollGeneration, setGithubPollGeneration] = useState(0); + + const commit = useCallback((next: GithubPollGovernorState) => { + const previous = stateRef.current; + if (next === previous) return; + stateRef.current = next; + // Only a change in an actual stand-down can change a caller's cadence; a + // bumped failure count on its own must not re-render every PR consumer. + if ( + next.ladderPausedUntilMs !== previous.ladderPausedUntilMs + || next.reservePausedUntilMs !== previous.reservePausedUntilMs + ) { + setGithubPollGeneration((value) => value + 1); + } + }, []); + + const refreshBudget = useCallback(async () => { + // Optional on purpose: an older remote runtime has no budget action, and a + // renderer that threw here would lose the local failure ladder too. + const budget = await window.ade?.github?.getRequestBudget?.().catch(() => null); + if (!budget) return; + commit(applyGithubRequestBudget(stateRef.current, budget, Date.now())); + }, [commit]); + + const noteGithubReadFailure = useCallback(() => { + commit(noteGithubPollFailure(stateRef.current, Date.now())); + // Pull the classified kind and the quota reserve from the process that made + // the request; the rejection itself cannot carry them across IPC. + void refreshBudget(); + }, [commit, refreshBudget]); + + const noteGithubReadSuccess = useCallback(() => { + commit(noteGithubPollSuccess(stateRef.current, Date.now())); + }, [commit]); + + const isGithubPollStoodDown = useCallback( + () => isGithubPollPaused(stateRef.current, Date.now()), + [], + ); + + const githubPollPeriodFor = useCallback( + (basePeriodMs: number) => githubPollPeriodMs(stateRef.current, basePeriodMs, Date.now()), + [], + ); + + useEffect(() => { + if (!active) return undefined; + void refreshBudget(); + const id = window.setInterval(() => { + void refreshBudget(); + }, GITHUB_REQUEST_BUDGET_REFRESH_MS); + return () => window.clearInterval(id); + }, [active, refreshBudget]); + + return { + isGithubPollStoodDown, + noteGithubReadFailure, + noteGithubReadSuccess, + githubPollPeriodFor, + githubPollGeneration, + }; +} diff --git a/apps/desktop/src/renderer/webclient/adapter/misc.ts b/apps/desktop/src/renderer/webclient/adapter/misc.ts index 2e12c347d..7f30a0695 100644 --- a/apps/desktop/src/renderer/webclient/adapter/misc.ts +++ b/apps/desktop/src/renderer/webclient/adapter/misc.ts @@ -562,6 +562,13 @@ export function createMiscNamespaces(infra: AdapterInfra): MiscNamespaces { startAppUserDeviceAuth: async () => ({ ok: false, error: "unsupported" }), pollAppUserDeviceAuth: async () => ({ status: "expired" }), clearAppUserAuth: async () => ({ authenticated: false, user: null }), + // Routed to the host: the paired machine spends the quota, so its reserve + // is the one these pollers must respect. + getRequestBudget: (opts?: unknown) => call("github.getRequestBudget", opts, { + pausedUntil: null, + failureKind: null, + retryAt: null, + }), detectRepo: async () => null, listRepoAutolinks: async () => [], getAppInstallationStatus: async () => ({ installed: false, state: "unknown" }), diff --git a/apps/desktop/src/shared/ipc.ts b/apps/desktop/src/shared/ipc.ts index ca7c2f2b8..948465392 100644 --- a/apps/desktop/src/shared/ipc.ts +++ b/apps/desktop/src/shared/ipc.ts @@ -637,6 +637,7 @@ export const IPC = { githubSetToken: "ade.github.setToken", githubClearToken: "ade.github.clearToken", githubStatusChanged: "ade.github.statusChanged", + githubGetRequestBudget: "ade.github.getRequestBudget", githubGetAppUserAuthStatus: "ade.github.getAppUserAuthStatus", githubStartAppUserDeviceAuth: "ade.github.startAppUserDeviceAuth", githubPollAppUserDeviceAuth: "ade.github.pollAppUserDeviceAuth", diff --git a/apps/desktop/src/shared/types/git.ts b/apps/desktop/src/shared/types/git.ts index fc5fd4c6e..db1271075 100644 --- a/apps/desktop/src/shared/types/git.ts +++ b/apps/desktop/src/shared/types/git.ts @@ -325,6 +325,40 @@ export type GitHubAuthFailure = { retryAt: string | null; }; +export type GitHubAuthFailureKind = GitHubAuthFailure["kind"]; + +/** + * What every *automatic* GitHub reader needs to know before it spends a + * request, delivered as data rather than as an error message — a rejection + * cannot carry it, because Electron IPC and the runtime's JSON-RPC both flatten + * an error to its message. + * + * Zero-network to produce, so it is safe to consult on a timer and while GitHub + * is refusing. Every field is optional-by-nullability: a runtime that does not + * implement the read leaves clients on their own local backoff rather than + * breaking them. See `docs/features/pull-requests/README.md`, "Keeping + * automatic GitHub reads inside the quota". + */ +export type GitHubRequestBudget = { + /** + * The quota reset instant, set once an available credential reaches the + * 500-request background reserve; null while requests may proceed. Callers + * resume by themselves because the value is the instant the quota refills. + */ + pausedUntil: string | null; + /** + * The worst failure currently recorded on a PR-read resource — worst meaning + * the one that justifies the longest stand-down — or null when the most + * recent request succeeded, or when the failure is old enough that it no + * longer describes the request the caller just made. `service_unavailable` carries no `pausedUntil` + * (a GitHub 5xx must never park a credential) but still tells a poller to + * lengthen its cadence. + */ + failureKind: GitHubAuthFailureKind | null; + /** Retry instant GitHub supplied for {@link failureKind}, when it gave one. */ + retryAt: string | null; +}; + export type GitHubCredentialSource = "environment" | "app" | "gh" | "pat"; export type GitHubCredentialCapability = "read" | "write"; diff --git a/apps/desktop/src/shared/types/sync.ts b/apps/desktop/src/shared/types/sync.ts index 0e6823a4e..80bfae04f 100644 --- a/apps/desktop/src/shared/types/sync.ts +++ b/apps/desktop/src/shared/types/sync.ts @@ -1926,6 +1926,7 @@ export type SyncRemoteCommandAction = | "history.listOperations" | "github.getStatus" | "github.getRemoteStatus" + | "github.getRequestBudget" | "github.publishCurrentProject" | "projectConfig.get" | "projectConfig.save" diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index eff0cba73..d7eb36841 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -803,6 +803,18 @@ ade.github.* # PR list, review, merge, checks. Also exposes # getAppUserAuthStatus / startAppUserDeviceAuth / # pollAppUserDeviceAuth / clearAppUserAuth (start/poll/ # clear are CTO-only actions in the ADE Actions registry). + # getRequestBudget is the zero-network read the PR + # surface's poll governor consults on a slow timer + # and after every failed read: the 500-request quota + # reserve plus the last classified failure kind. + # It exists on all three + # transports (IPC, the `github` action domain, the + # `github.getRequestBudget` sync remote command) because + # a rejection cannot carry a typed kind across either + # boundary. Optional on the client -- an older remote + # runtime that omits it leaves callers on their own + # local backoff. See + # features/pull-requests/README.md. ade.prs.* # stacked PR queue, integration, rebase/issue # resolver sessions, and merge readiness ade.conflicts.* # risk matrix, simulation, proposals diff --git a/docs/features/pull-requests/README.md b/docs/features/pull-requests/README.md index 860746448..af36154df 100644 --- a/docs/features/pull-requests/README.md +++ b/docs/features/pull-requests/README.md @@ -181,8 +181,8 @@ GitHub access and relay dependencies: | File | Responsibility | |------|---------------| -| `apps/desktop/src/main/services/github/githubService.ts`, `apps/ade-cli/src/headlessLinearServices.ts` | Desktop-local and runtime-owned GitHub request paths. Both build the environment → App → GitHub CLI → PAT read chain, skip the read-only App for writes, retry compatible credentials after auth/permission/rate failures, and expose the active/fallback sources through `GitHubStatus`. | -| `apps/desktop/src/main/services/github/githubCredentialHealth.ts`, `githubRateLimit.ts` | Token-digest health keyed by REST/GraphQL resource, five-minute invalid/permission cooldowns, rate-limit reset handling, same-account primary-quota propagation, and the 500-request background reserve. `classifyGitHubAuthFailure` maps GitHub 5xx (and GitHub's own outage bodies) to `service_unavailable` ahead of the transient-network check, and that kind is deliberately given **no** credential cooldown — the credential is not the problem, so parking it would fail the user's next local merge or PR read for the whole window. | +| `apps/desktop/src/main/services/github/githubService.ts`, `apps/ade-cli/src/headlessLinearServices.ts` | Desktop-local and runtime-owned GitHub request paths. Both build the environment → App → GitHub CLI → PAT read chain, skip the read-only App for writes, retry compatible credentials after auth/permission/rate failures, and expose the active/fallback sources through `GitHubStatus`. Both also record a **transport** failure — a hang, timeout, DNS/TLS error, or a body that stalls mid-stream, caught at the header read *and* at the body read — before rethrowing it, with a null rate limit so it cannot clobber real quota numbers. Without that record the request budget reported no kind at all for the outage shape it exists to survive. Both expose `getRequestBudget()`; implementing it only on the desktop side would leave the shipping runtime-bound build's poll governor un-gated. | +| `apps/desktop/src/main/services/github/githubCredentialHealth.ts`, `githubRateLimit.ts` | Token-digest health keyed by REST/GraphQL resource, five-minute invalid/permission cooldowns, rate-limit reset handling, same-account primary-quota propagation, and the 500-request background reserve. `classifyGitHubAuthFailure` maps GitHub 5xx (and GitHub's own outage bodies) to `service_unavailable` ahead of the transient-network check, and that kind is deliberately given **no** credential cooldown — the credential is not the problem, so parking it would fail the user's next local merge or PR read for the whole window. `githubRequestBudget()` exposes the reserve plus the worst *recent* failure kind as a zero-network `GitHubRequestBudget`, which is how foreground pollers honour the same reserve. The reserve half uses the quota-bucket filter (`core` / `graphql` / a large-limit `unknown`); the kind half deliberately does not, skipping only the independent `search` bucket, and bounds what it reports by `REQUEST_BUDGET_FAILURE_FRESHNESS_MS` (90 s). `REQUEST_BUDGET_FAILURE_SEVERITY` ranks the kinds and is a stated two-way contract with `ladderBaseMs` in the renderer's poll governor. See [Keeping automatic GitHub reads inside the quota](#keeping-automatic-github-reads-inside-the-quota). | | `apps/desktop/src/main/services/github/githubStatusPage.ts`, `apps/desktop/src/shared/githubServiceHealth.ts` | GitHub-outage attribution. See [Telling a GitHub outage apart from a broken credential](#telling-a-github-outage-apart-from-a-broken-credential). The shared module is the pure half — Statuspage `summary.json` parsing into `GitHubServiceHealth`, the ADE-relevant component allowlist, and `isGithubServiceUnavailable`; the main-process module owns the failure-triggered lookup, its cache, and `attachGitHubServiceHealth`. | | `apps/desktop/src/shared/githubOperationCredential.ts`, `apps/desktop/src/shared/types/git.ts` | Capability-aware credential order and the optional status DTOs for source state, fallback, write availability, background-pause time, and corroborated service health. `resolveGithubStatusCredentials` stops walking the chain on `service_unavailable` alongside `network` / `unknown`: a GitHub 5xx says nothing about the credential, so the next candidate would fail identically and only add load to a failing service. | | `apps/desktop/src/main/services/automations/automationIngressService.ts`, `apps/ade-cli/src/bootstrap.ts`, `apps/desktop/src/main/main.ts` | Relay cursor drain and targeted reconciliation, relay-health tracking, and injection of relay/quota state into the runtime-owned or desktop-local PR poller. | @@ -206,7 +206,9 @@ Renderer components (`apps/desktop/src/renderer/components/prs/`): | File | Responsibility | |------|---------------| | `PRsPage.tsx` | Top-level tab shell (GitHub vs Workflows) with URL-driven state. Consumes create-PR handoff params from either router search or hash search (`create=1`, `sourceLaneId` / `laneId`, `target=primary`) and the `prs.create` dialog bus props, then opens `CreatePrModal` with matching initial values without persisting the one-shot route as the last PR route. | -| `state/PrsContext.tsx` | PR data provider (list, selection, GitHub stacks, and rebase needs). Selected-PR primary reads apply progressively as status/check/review/comment requests resolve, so one slow piece does not hold the whole detail pane busy; cached snapshots stay visible during GitHub rate limits. | +| `state/PrsContext.tsx` | PR data provider (list, selection, GitHub stacks, and rebase needs). Selected-PR primary reads apply progressively as status/check/review/comment requests resolve, so one slow piece does not hold the whole detail pane busy; cached snapshots stay visible whenever a live read fails, not only during rate limits. Exposes the shared GitHub poll governor to the PR surface. | +| `state/githubPollGovernor.ts` | Pure state machine shared by every automatic PR read on the surface: a stand-down armed by any rejection (one rung while unclassified, exponential once attributed to GitHub) plus the runtime's 500-request quota reserve, tracked as an independent stand-down that a success does not clear. Returns a poll *period* so timers slow down rather than waking and returning early. See [Keeping automatic GitHub reads inside the quota](#keeping-automatic-github-reads-inside-the-quota). | +| `state/useGithubPollGovernor.ts` | The provider-side hook that drives it: holds the state in a ref (interval callbacks would otherwise read a stale closure), exposes `isGithubPollStoodDown` / `noteGithubReadFailure` / `noteGithubReadSuccess` / `githubPollPeriodFor`, bumps a render generation only when a stand-down actually changes, and refreshes `ade.github.getRequestBudget` on a slow timer and after every failure. | | `prsRouteState.ts` | URL ↔ page state mapping plus project-scoped last-route storage. When a project root is known, the PRs tab reads only that project's stored route and does not fall back to the legacy global route from another project. | | `CreatePrModal.tsx` | Single/integration PR creation with lane warnings, branch name validation, and optional initial values for single-PR handoffs from lane/chat surfaces. Normal PRs default the title to `source lane -> target lane`; a `target: "primary"` handoff resolves the base branch from the primary lane (falling back to `main`). | | `tabs/NormalTab.tsx` | Normal PR list | @@ -446,6 +448,7 @@ See [Which machine answers a PR read](#which-machine-answers-a-pr-read). - `ade.prs.retargetBase` — re-point an individual PR's base branch - `ade.prs.getGitHubSnapshot` — repository PR snapshot for the active GitHub repo. The DTO still carries `externalPullRequests` and accepts `includeExternalClosed` for compatibility, but the current service returns repo PRs only and the renderer ignores legacy cross-repo external items. Args are `{ force?, includeExternalClosed?, historyPageLimit?, automaticRefresh? }`. `force` means "do not serve me the cache"; `automaticRefresh: true` additionally says "this force came from a timer, not a person", which keeps the call inside the [GitHub read failure ladder](#github-read-failure-ladder). The same three fields flow through all three transports that reach the service — in-process IPC (`registerIpc.ts`), the runtime action registry, and the sync remote command service (`prs.getGitHubSnapshot`) — so a remote-bound window behaves like a local one. - `ade.prs.simulateIntegration`, `ade.prs.createIntegrationLaneForProposal`, `ade.prs.commitIntegration`, `ade.prs.cleanupIntegrationWorkflow` +- `ade.github.getRequestBudget` — zero-network `GitHubRequestBudget` (quota-reserve pause + worst recent failure kind + GitHub's own retry instant) for automatic readers deciding their cadence before spending a request. Registered on all three transports: in-process IPC (`registerIpc.ts`), the `github` ADE action domain (`getRequestBudget`), and the sync remote command `github.getRequestBudget` — the last so the hosted web client, whose timers run in the browser but whose requests spend the paired machine's quota, honours that machine's reserve. Optional on the client (`window.ade.github.getRequestBudget?.()`): an older remote runtime that cannot answer leaves callers on their own local backoff rather than losing the brake. See [Keeping automatic GitHub reads inside the quota](#keeping-automatic-github-reads-inside-the-quota). - `ade.github.listRepoAutolinks` / `ade.github.createRepoAutolink` — read and create GitHub repo autolink references (the `key_prefix` + `url_template` rules that turn issue identifiers like `ADE-123` into GitHub-rendered hyperlinks). Used by the Linear setup flow so a project's Linear identifiers become clickable in PR bodies. `createRepoAutolink` requires `urlTemplate` to contain `` and busts the autolinks ETag cache after a successful POST. Integration merge-into flow uses these existing channels with widened @@ -1091,6 +1094,185 @@ corroboration, a bare `service_unavailable` still renders its own honest copy ("GitHub isn't responding", pointing at ADE Settings rather than an external link) and never suggests reconnecting. +## Keeping automatic GitHub reads inside the quota + +The PRs surface makes GitHub requests from two places: the background +`prPollingService`, and the renderer's own timers. Only the first was ever +throttled. On 2026-08-17, during a multi-hour GitHub outage, the second spent +5,001 core requests in one hour and hit the 5,000/hour primary limit, which +blocked the user's real work — a merge — until the quota reset. + +**How one open PR spent a whole hourly quota.** `PrDetailPane` polls readiness +signals every 5 seconds while the Checks tab is open and something is still +queued or running. A tick costs roughly seven to ten REST requests: a pull, an +Actions runs page, up to `PR_ACTION_RUNS_LIMIT` (12) job reads, a combined +status, and a check-runs page. At 720 ticks an hour that is the entire quota. +It normally cannot run for an hour, because CI settles in about ten minutes and +`checksTerminal` stops the loop. Three defects removed every limit at once: + +1. **A failed checks fetch looked like an empty one.** `getChecksByCoords` ran + both sources under `bestEffort`, so a 5xx became `[]` — byte-identical to + "this commit has no checks yet". The loop's stop condition is "at least one + check exists and all of them settled", so it never fired. +2. **The brake could not see the outage.** The only backoff was + `msg.includes("rate limit") || msg.includes("API rate")` on the rejection + message. Every response during the outage was a 5xx, which matches neither + substring, so nothing armed — while every failed request still spent quota. + Selecting a different PR (the natural reaction to a stuck tab) also reset + what little backoff there was. +3. **The reserve protected only the background poller.** + `GITHUB_BACKGROUND_RATE_LIMIT_RESERVE` (500) was enforced in exactly one + place, `prPollingService`. Every renderer read went straight to + `githubService.apiRequest` with no gate, which is how the quota reached zero + despite a 500-request reserve existing. + +**What replaced them.** The design constraint is that ADE must not degrade +*functionality* when GitHub is down — only its request rate. Nothing below +blanks a pane, hides a PR, or stops polling; every rung is a longer cadence, and +recovery is automatic. + +- `prService.getChecks` / `getChecksByGithub` **reject** when neither checks + source could be read, and return what they got when only one failed. Callers + that prefer stale checks to none catch it explicitly instead of inheriting a + silent `[]`. A rejection also means `upsertSnapshotRow` cannot overwrite a + good cached snapshot with a fabricated empty one, and the mobile aggregate + files checks under `unavailableParts` instead of reporting a false empty. +- `renderer/components/prs/state/githubPollGovernor.ts` is one shared brake for + every automatic PR read on the surface, driven from the provider by + `useGithubPollGovernor`. **Any** rejection arms a stand-down; there is no + substring test anywhere. The fast loops take their stand-down as a longer + timer *period* (`githubPollPeriodFor`) rather than as an early return, because + a guard re-checked on every tick is one refactor away from being missed; they + keep the in-tick `isGithubPollStoodDown()` check only as a second line of + defence for a pause armed between ticks. The provider's own 60 s detail poll + is the exception and simply skips its ticks — 60 s is already at the safe end, + so there is no request volume to win by stretching it, and the base cadence + should resume the moment GitHub does. One success clears the ladder, and the + stand-down survives PR selection — a GitHub outage is account-wide, not + per-PR. + + An **unclassified** failure — no kind at all, distinct from a classified + `unknown` — buys one flat 30 s rung and does not climb: a runtime + reconnect, an IPC blip, or a local `PR not found` all reach the governor as a + bare rejection, and letting those ride to the five-minute ceiling would cost + liveness on the one surface whose whole value is liveness. Once the budget + attributes the failure to GitHub the ladder is re-derived — only when the kind + actually *changed*, since re-arming from `now` on every 60 s budget poll would + push the pause out forever — and climbs `base * 2^(n-1)` to a 5-minute + ceiling that matches `prPollingService`'s `MAX_INTERVAL_MS`. The base is the + kind's, not one number: 60 s for a definite answer a fast retry cannot change + (`service_unavailable` / `invalid_token` / `permission_denied`), 30 s for + `network` and `unknown`, and `rate_limited` skips the ladder entirely — it + goes straight to the ceiling, or to the reset instant GitHub named when that + is further out still. + + The failure ladder and the **quota reserve** are tracked as two independent + stand-downs; an elapsed reserve is dropped rather than carried — by the budget + fold *and* by a recorded success, so recovery never depends on the budget read + still answering — which is what makes the renderer re-render and rebuild its + timers at the fast cadence when the quota resets. Degrading without ever + coming back is the one failure mode this whole module exists to avoid. Only + the ladder is cleared by a success. They were one field + first, which quietly leaked the reserve: user actions are ungated on purpose, + so a single PR open or Refresh click reset the governor and handed every + automatic loop its 5-second cadence back with the quota still below 500. A + successful request does not refill the quota — only the reset does, and the + budget reports that instant. +- The **typed** failure kind cannot ride on the rejection (Electron IPC and the + runtime's JSON-RPC both flatten an error to its message), so it arrives as + data: `ade.github.getRequestBudget` returns the `GitHubRequestBudget` — the + reserve pause plus the `GitHubAuthFailure["kind"]` that + `classifyGitHubAuthFailure` already recorded on the credential, including the + `service_unavailable` kind from + [the outage taxonomy](#telling-a-github-outage-apart-from-a-broken-credential). + A kind meaning "GitHub itself is failing" starts the ladder at 60 s instead of + 30 s; a `rate_limited` kind waits for the reset instant GitHub named, which is + GitHub's own documented guidance. +- The budget read is **zero-network and zero-subprocess** — it inspects + in-memory credential health only — so consulting it costs nothing and stays + correct while GitHub is refusing. That is why it takes no credential + inventory: resolving one can shell out to `gh auth token`, decrypt the + credential store (a PowerShell subprocess under DPAPI on Windows), or refresh + an expired App user token *over the network*, and this read runs on a timer + and again on every failed poll group. Answering from every credential the + process knows rather than one project's is also the safe direction — the + primary quota is per-account, so over-throttling is conservative and + under-throttling is the bug — and it matches `prPollingService`, which calls + `githubBackgroundRequestPauseUntilMs()` unscoped for the same reason. The + reported *failure kind* is bounded by recency for that reason too + (`REQUEST_BUDGET_FAILURE_FRESHNESS_MS`, 90 s — comfortably wider than the + hook's 60 s refresh): a failure is otherwise cleared only by a success on the + same credential and resource, so a permanently-bad one (a stale + `GITHUB_TOKEN`, a revoked PAT, a fork the App cannot see) would become the + process-wide answer and push every project's ladder onto the longer base on a + healthy GitHub. When several credentials each hold a different failure the + budget reports the *worst* one, ranked by `REQUEST_BUDGET_FAILURE_SEVERITY` — + which has to agree with the governor's `ladderBaseMs` ordering or a + multi-credential chain reports the kind asking for the shorter wait. Change + one, change both. + + A request that never gets an answer from GitHub — a hang, a timeout, a DNS or + TLS failure, a response body that stalls mid-stream — is recorded as a failure + by both owners before it is rethrown. The body phase matters as much as the + header phase: on desktop the body carries its own timeout, and on both owners + a socket error mid-body surfaces there rather than at the header read. It + used to throw straight out of the request helper, recording nothing, so the + budget reported no kind and the governor could not climb past its flat + unclassified rung — inert for exactly the outage shape it targets. The kind + scan also deliberately does *not* reuse the reserve's quota-bucket filter: + these failures carry no `x-ratelimit-*` headers, so they land under an + `unknown` bucket with no limit and were being dropped by it. + + It is implemented in **both** GitHub service owners (desktop `githubService` + and the daemon's `createHeadlessGitHubService`), because the runtime-bound + production build reaches GitHub through the second one, and registered on the + **sync remote-command** surface as well — the hosted web client's timers run + in the browser but its GitHub requests are spent by the paired machine's + quota, so an unregistered command would have left the web client's 5-second + loop permanently un-gated. It is optional on the client: an older remote + runtime that cannot answer leaves callers on their local ladder rather than + losing the brake entirely. +- **User-initiated work is deliberately exempt.** The Refresh button and + post-mutation re-reads (`refreshSelectedPrDetail`) bypass the stand-down + entirely — preserving quota for explicit user actions is what the reserve is + *for*, and a manual retry is the escape hatch from a stale backoff. This + mirrors the `force` exemption in the + [GitHub read failure ladder](#github-read-failure-ladder). +- A failed detail read now falls back to the cached snapshot for **every** + failure, not only a recognised rate limit, so the pane keeps showing what ADE + already knows instead of going empty and then polling for more of the same. +- `prService.refresh()`'s background sweep now runs its candidates through + `refreshPrIds` and lets a failure that means **GitHub itself is unusable** + reach `prPollingService`. It used to run through a best-effort helper that + swallowed every per-row failure and returned void, so a sweep where GitHub + refused everything still read as a clean tick: `consecutiveFailures` stayed at + zero and `computeBackoffMs` never engaged. + + Two conditions have to hold, and both are deliberate. `refreshPrIds` throws + only when **no** row refreshed — one healthy row is proof GitHub is answering, + so a mixed batch is not an outage. The sweep then rethrows only if the reason + is GitHub-wide: a classified `rate_limited` / `service_unavailable` / + `network` kind, one of GitHub's own 5xx bodies, or a transport failure + (`isGithubWideFailure`, which falls back to `isTransientGithubProbeFailure` + because a common outage shape is requests that *hang* rather than answer, and + those carry no classification at all). Anything else is logged as + `prs.background_refresh_rows_failed` and the tick counts as clean. + + That second condition is why "every row failed" is not sufficient on its own. + Candidates are the rows whose `last_synced_at` is stale, and a row that + permanently 404s (repo renamed, fork access lost, PR hard-deleted) never + refreshes it — so it becomes the only candidate on every later sweep, and an + unconditional rethrow would pin a perfectly healthy poller at max backoff + forever. For the same reason `refreshPrIds` prefers a GitHub-wide reason when + it picks which failure to throw, rather than the first one in the batch. + +No new UI ships with this. A corroborated outage already collapses the GitHub +banner family into one neutral incident notice +([what the UI does during a corroborated outage](#what-the-ui-does-during-a-corroborated-outage)), +and an uncorroborated `service_unavailable` still renders its own honest copy. A +per-pane staleness chip would duplicate both without telling the user anything +they could act on. + ## Background polling `prPollingService` runs inside the process that backs the window's runtime — @@ -1701,6 +1883,25 @@ markdown layout cost for offscreen or folded content. — and a degraded GitHub is polled *harder* than a healthy one, because every caller the cache was absorbing turns back into a live request. The floor is the fix, not the doubling. +- **An automatic GitHub read must never classify a failure by message text.** + Electron IPC and the runtime's JSON-RPC both flatten a thrown error to its + message, so a renderer substring test is the only classification that *looks* + available — and it is the one that failed. `msg.includes("rate limit")` matched + none of the 5xx responses of the 2026-08-17 outage, so the PR detail pane's + 5-second poll ran unbraked for an hour and spent the account's whole 5,000/hour + core quota. Arm the backoff on **any** rejection and take the typed kind from + `ade.github.getRequestBudget`, which reports what `classifyGitHubAuthFailure` + recorded in the process that actually made the request. +- **A swallowed GitHub failure that returns an empty result is a quota bug, not + just a display bug.** `getChecks` returning `[]` for a failed fetch was + indistinguishable from "CI has not started yet", which is exactly the state the + detail pane's stop condition treats as "keep polling fast". Any new best-effort + read whose emptiness feeds a loop's termination test has to distinguish the two. +- **Every foreground GitHub timer must consult the poll governor.** The + 500-request reserve was enforced only in `prPollingService`, so the renderer + loops drained the quota the reserve was supposed to protect. When adding a new + automatic PR read, gate it on `isGithubPollStoodDown()` and derive its interval + from `githubPollPeriodFor(base)`; user-initiated actions stay exempt on purpose. - **A lookup that falls back to cached data must not report success upstream.** A failed per-branch PR lookup returns `null`, not `[]`, so the snapshot can never fold "we could not ask" into a confirmed-empty result and drop a lane's diff --git a/docs/features/sync-and-multi-device/remote-commands.md b/docs/features/sync-and-multi-device/remote-commands.md index af1f47924..44784e260 100644 --- a/docs/features/sync-and-multi-device/remote-commands.md +++ b/docs/features/sync-and-multi-device/remote-commands.md @@ -492,6 +492,14 @@ a boolean. **GitHub** (`github.*`) - `getStatus`, `getRemoteStatus`, `publishCurrentProject` +- `getRequestBudget` — the host's zero-network GitHub request budget (quota + reserve pause, worst recent failure kind, retry instant). `viewerAllowed`, + because it only reports how hard the caller may poll. Registered for the + hosted web client specifically: its PR timers run in the browser but their + GitHub requests are spent from *this* machine's quota, so without the + registration its adapter falls back to an all-null budget and its 5-second + checks loop never sees the reserve. See + [pull requests](../pull-requests/README.md#keeping-automatic-github-reads-inside-the-quota). **Project config** (`projectConfig.*`) - `get`, `save`