fix(review): record getAppInstallation calls in the shared rate-limit store (#4506)

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<NonNullable<GitHubWebhookPayload["installation"]>> {
   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"

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)

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) {

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(

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<import("../../src/types").JobMessage>;
+
+    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) => {