From 0054bbdea4f802d24dc47b2c1ab8703eb4e4effb Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 22:23:17 -0700 Subject: [PATCH] fix(review): record getAppInstallation calls in the shared rate-limit store (#4506) refreshInstallationHealthRecords makes one real GitHub REST call per installation via getAppInstallation, but that call never fed the D1-persisted rate-limit observation store shouldWaitForGitHubRateLimit reads for queue-dequeue admission -- only #4505's job-level throttle protected it, with no visibility into its own consumption. --- src/github/app.ts | 41 +++++++++++++++++++++++++++------ src/github/backfill.ts | 7 ++++-- src/index.ts | 7 ++++-- test/unit/github-app.test.ts | 25 +++++++++++++++++++- test/unit/index.test.ts | 44 ++++++++++++++++++++++++++++++++++++ 5 files changed, 112 insertions(+), 12 deletions(-) diff --git a/src/github/app.ts b/src/github/app.ts index d506b92d42..9c405a1aa1 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -3,7 +3,7 @@ import { fetchBrokeredInstallationToken, isOrbBrokerMode, } from "../orb/broker-client"; -import { updateInstallationPermissions } from "../db/repositories"; +import { recordGitHubRateLimitObservation, updateInstallationPermissions } from "../db/repositories"; import { recordClockSkewFromResponse } from "../selfhost/clock-skew"; import { clearGitHubResponseCacheForTest, @@ -373,12 +373,33 @@ export async function getAppInstallation( installationId: number, ): Promise> { const jwt = await createAppJwt(env); - const response = await timeoutFetch( - `https://api.github.com/app/installations/${installationId}`, - { - headers: githubHeaders(`Bearer ${jwt}`), - }, - ); + const admissionKey = githubRateLimitAdmissionKeyForInstallation(installationId); + const path = `/app/installations/${installationId}`; + // Deliberately NOT passed as timeoutFetch's githubRateLimitAdmissionKey option here: that option also drives the + // GET response-cache key (responseCacheKey, ./client), and `installation:{id}` collides across different calling + // Apps for the same installation id -- this endpoint's response depends on WHICH App's JWT is asking, so the + // cache must stay keyed off the Authorization header (the no-admission-key fallback) to preserve per-App-identity + // isolation (#1940). recordGitHubRateLimitObservation below records the SAME admissionKey directly instead. + const response = await timeoutFetch(`https://api.github.com${path}`, { + headers: githubHeaders(`Bearer ${jwt}`), + }); + // #4506: refreshInstallationHealthRecords's per-installation loop (backfill.ts) makes one of these calls per + // installation, but this call lives outside backfill.ts's module boundary -- and backfill.ts already imports + // getAppInstallation FROM this file, so importing its recordGitHubResponse back here would cycle. Mirrors that + // function's header-parsing inline instead (see its doc comment, backfill.ts, for the full write-path rationale). + // Recorded before the ok-check: an exhausted/rate-limited (non-2xx) response's headers are exactly the signal + // shouldWaitForGitHubRateLimit needs to back off later jobs. + const resetHeader = response.headers.get("x-ratelimit-reset"); + await recordGitHubRateLimitObservation(env, { + repoFullName: null, + admissionKey, + resource: "rest", + path, + statusCode: response.status, + limitValue: parseNullableRateLimitHeader(response.headers.get("x-ratelimit-limit")), + remaining: parseNullableRateLimitHeader(response.headers.get("x-ratelimit-remaining")), + resetAt: resetHeader && Number.isFinite(Number(resetHeader)) ? new Date(Number(resetHeader) * 1000).toISOString() : undefined, + }); if (!response.ok) { const body = await response.text(); throw new Error( @@ -393,6 +414,12 @@ export async function getAppInstallation( return payload; } +function parseNullableRateLimitHeader(value: string | null): number | undefined { + if (!value) return undefined; + const parsed = Number(value); + return Number.isFinite(parsed) ? parsed : undefined; +} + export type GitHubRepositoryCollaboratorPermission = | "admin" | "maintain" diff --git a/src/github/backfill.ts b/src/github/backfill.ts index ee6e918604..b6f7f08f94 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -4438,8 +4438,11 @@ function summarizeSegments( }; } -// #2543: this is the ONLY call site of recordGitHubRateLimitObservation -- one row per outbound GitHub REST/ -// GraphQL response. DELIBERATELY left un-batched (documented decision, not an oversight): the write rate is +// #2543: this is the primary call site of recordGitHubRateLimitObservation -- one row per outbound GitHub +// REST/GraphQL response made through this module's bulk-sync path. (getAppInstallation, ../github/app.ts, +// records its own installation-lookup call directly for #4506 -- it lives outside this module's boundary and +// this module already imports getAppInstallation FROM app.ts, so importing this function back would cycle.) +// DELIBERATELY left un-batched (documented decision, not an oversight): the write rate is // bounded by GitHub's own REST budget for a single App installation (~5000/hour ≈ 1.4/s sustained, further // capped in practice by QUEUE_CONCURRENCY's small worker-pool size), nowhere near a volume where single-row // Postgres INSERTs meaningfully pressure the connection pool. shouldWaitForGitHubRateLimit (rate-limit.ts) diff --git a/src/index.ts b/src/index.ts index e2457f5c22..fb87e4dab2 100644 --- a/src/index.ts +++ b/src/index.ts @@ -166,8 +166,11 @@ async function enqueueScheduledJobs(env: Env, controller: ScheduledController): // SKIPS this 30-min tick and hands the remaining budget to webhooks (which drive timely reviews); the next // 30-min tick retries, and after the bucket resets the backfill resumes. Queue depth is deliberately not a // suppressor here: unrelated pending work can stay nonzero for long periods, while rate admission on the - // queued jobs is the precise throttle. The cheap single-call health jobs (repair-data-fidelity, - // refresh-installation-health) stay unconditional. + // queued jobs is the precise throttle. repair-data-fidelity (a cheap, local-only D1 scan that only + // dispatches already-gated jobs) stays unconditional. refresh-installation-health is NOT a single-call job + // -- refreshInstallationHealthRecords makes one real GitHub REST call (getAppInstallation) per installation, + // sequentially -- but it is likewise left unconditional here since its calls now yield to the shared budget + // at dequeue time (GITHUB_BUDGET_BACKGROUND_TYPES, #4505/#4506). if (selfHostedReviews && !sweepThrottledUntil) { jobs.push({ type: "backfill-registered-repos", requestedBy: "schedule", mode: isFullSyncWindow ? "full" : "light" }); } else if (selfHostedReviews) { diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 520595941d..22d22b03a5 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -26,7 +26,7 @@ import { } from "../../src/github/app"; import type { Advisory } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; -import { getInstallation, upsertInstallation } from "../../src/db/repositories"; +import { getInstallation, listLatestGitHubRateLimitObservations, upsertInstallation } from "../../src/db/repositories"; import { clockSkewSecondsSample, resetClockSkewForTest } from "../../src/selfhost/clock-skew"; beforeEach(() => { @@ -2322,6 +2322,29 @@ describe("GitHub check runs", () => { }); }); + it("INVARIANT (#4506): getAppInstallation's call is recorded into the shared rate-limit observation store", async () => { + const privateKey = await generatePrivateKeyPem(); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.endsWith("/app/installations/123")) { + return Response.json( + { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "selected", permissions: {}, events: [] }, + { headers: { "x-ratelimit-remaining": "4321", "x-ratelimit-reset": "1779976046" } }, + ); + } + return new Response("not found", { status: 404 }); + }); + + await getAppInstallation(env, 123); + + // Unlike a bare fetch, this is now visible to shouldWaitForGitHubRateLimit's admission reads -- before #4506, + // this call passed no rate-limit-admission option at all, so it never reached recordGitHubRateLimitObservation. + expect(await listLatestGitHubRateLimitObservations(env)).toEqual( + expect.arrayContaining([expect.objectContaining({ path: "/app/installations/123", statusCode: 200, remaining: 4321, admissionKey: "installation:123" })]), + ); + }); + it("surfaces live GitHub App installation fetch failures", async () => { const privateKey = await generatePrivateKeyPem(); vi.stubGlobal( diff --git a/test/unit/index.test.ts b/test/unit/index.test.ts index 99fe30bfed..91e1f20942 100644 --- a/test/unit/index.test.ts +++ b/test/unit/index.test.ts @@ -271,6 +271,50 @@ describe("worker entrypoint", () => { vi.useRealTimers(); }); + it("REGRESSION (#4506): pre-yields the whole refresh-installation-health job -- zero per-installation fetches -- while the shared GitHub REST budget is exhausted", async () => { + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-06-24T12:00:00.000Z")); + const env = createTestEnv(); + // refresh-installation-health carries no installationId (it loops over every installation internally), so + // githubRateLimitAdmissionKeyForJob resolves it to `null` -- its dequeue-time check reads the unscoped + // (admissionKey: undefined) latest-observations window, which any recent exhausted REST observation trips. + await recordGitHubRateLimitObservation(env, { repoFullName: null, admissionKey: "installation:1", resource: "rest", path: "/app/installations/1", statusCode: 200, limitValue: 5000, remaining: 5, resetAt: "2026-06-24T12:30:00.000Z", observedAt: "2026-06-24T12:00:00.000Z" }); + const fetchCalls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + fetchCalls.push(input.toString()); + return new Response("unexpected fetch", { status: 500 }); + }); + const acked: string[] = []; + const retries: Array<{ delaySeconds?: number } | undefined> = []; + const requeued: Array<{ message: import("../../src/types").JobMessage; delaySeconds?: number }> = []; + env.JOBS = { + async send(message: import("../../src/types").JobMessage, options?: { delaySeconds?: number }) { + requeued.push({ message, ...(options?.delaySeconds === undefined ? {} : { delaySeconds: options.delaySeconds }) }); + }, + } as unknown as Queue; + const batch = { + messages: [ + { + id: "installation-health-tick", + body: { type: "refresh-installation-health", requestedBy: "schedule" }, + ack: () => acked.push("installation-health-tick"), + retry: (options?: { delaySeconds?: number }) => retries.push(options), + }, + ], + } as unknown as MessageBatch; + + await worker.queue(batch, env); + + // Pre-yielded before refreshInstallationHealthRecords' per-installation loop ever starts -- proving the + // dequeue-time gate defers the ENTIRE job (not just individual calls within it) when the budget is exhausted, + // regardless of how many installations it would otherwise have fetched. + expect(fetchCalls).toEqual([]); + expect(acked).toEqual(["installation-health-tick"]); + expect(retries).toEqual([]); + expect(requeued).toEqual([{ message: { type: "refresh-installation-health", requestedBy: "schedule" }, delaySeconds: 900 }]); // delayUntil clamps to [30, 900] + vi.useRealTimers(); + }); + it("runs scheduled jobs through waitUntil", async () => { const env = createTestEnv(); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {