JSONbored · JSONbored · Jul 28, 2026 · Jul 28, 2026
@@ -204,7 +204,7 @@ async function actorHint(c: Context<{ Bindings: Env }>): Promise<string> {
 async function rateLimitIdentity(c: Context<{ Bindings: Env }>): Promise<string> {
   const ipIdentity = `ip:${await hashToken(clientIp(c))}`;
 
-  const installationIdentity = await installationRateLimitIdentity(c);
+  const installationIdentity = await installationRateLimitIdentity(c, ipIdentity);
   if (installationIdentity) return installationIdentity;
 
   if (isPreAuthRateLimitPath(c.req.path)) return ipIdentity;
@@ -222,11 +222,23 @@ async function rateLimitIdentity(c: Context<{ Bindings: Env }>): Promise<string>
 const INSTALLATION_KEYED_WEBHOOK_PATHS = new Set(["/v1/github/webhook", "/v1/orb/webhook"]);
 const INSTALLATION_KEYED_ORB_BEARER_PATHS = new Set(["/v1/orb/token", "/v1/orb/relay/register", "/v1/orb/relay/pull"]);
 
-async function installationRateLimitIdentity(c: Context<{ Bindings: Env }>): Promise<string | null> {
+async function installationRateLimitIdentity(c: Context<{ Bindings: Env }>, ipIdentity: string): Promise<string | null> {
   const path = c.req.path;
   if (INSTALLATION_KEYED_WEBHOOK_PATHS.has(path)) {
     const installationId = await peekWebhookInstallationId(c);
-    return installationId === null ? null : `installation:${installationId}`;
+    // #9469: the installation id here is read from the body BEFORE signature verification, so it is
+    // ATTACKER-CONTROLLED. Keying on it alone let anyone pin a chosen installation's bucket by POSTing
+    // `{"installation":{"id":<victim>}}` 10 times a minute -- GitHub's genuine deliveries then 429, and
+    // GitHub does not auto-redeliver, so the engine went deaf for that installation at a cost of one
+    // unauthenticated request every six seconds. The prior threat note correctly observed that a spoofed id
+    // grants no ACCESS, but sharing the bucket IS the attack.
+    //
+    // Folding in the connecting IP means an attacker and GitHub can never occupy the same bucket, while
+    // #4891's requirement still holds: many brokered tenants behind ONE shared egress no longer collide,
+    // because the installation id still separates them. It also restores headroom that #4891 removed by
+    // accident -- GitHub delivers from many egress IPs, so a CI storm now spreads across per-IP buckets
+    // again instead of contending for a single 10/60s window.
+    return installationId === null ? null : `installation:${installationId}:${ipIdentity}`;
   }
   if (path === "/v1/orb/relay") {
     // Single-tenant per deployment: the bound enrollment secret (never the request body) IS this deployment's

@@ -9,6 +9,7 @@ import {
   clearGitHubResponseCacheForTest,
   forcedSelfhostMode,
   githubHeaders,
+  githubRateLimitAdmissionKeyForAppJwt,
   githubRateLimitAdmissionKeyForInstallation,
   makeInstallationOctokit,
   timeoutFetch,
@@ -43,9 +44,11 @@ export {
 } from "../review/check-names";
 export type { CachedGitHubResponse, GitHubResponseCache } from "./client";
 export {
+  exceedsInlineRetryBudget,
   isCacheableGithubUrl,
   isRateLimitedResponse,
   rateLimitRetryMs,
+  retryAfterMs,
   setGitHubResponseCache,
 } from "./client";
 export {
@@ -403,7 +406,10 @@ export async function getAppInstallation(
   installationId: number,
 ): Promise<NonNullable<GitHubWebhookPayload["installation"]>> {
   const jwt = await createAppJwt(env);
-  const admissionKey = githubRateLimitAdmissionKeyForInstallation(installationId);
+  // #9494: this call authenticates with the App JWT, whose rate-limit bucket is separate from the
+  // installation's -- so its headers must NOT be recorded under `installation:{id}`. See
+  // githubRateLimitAdmissionKeyForAppJwt's own doc for the 30-minute false-clear this produced.
+  const admissionKey = githubRateLimitAdmissionKeyForAppJwt();
   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

@@ -121,6 +121,18 @@ export function githubRateLimitAdmissionKeyForPublicToken(): GitHubRateLimitAdmi
   return "public-token";
 }
 
+/**
+ * #9494: the App JWT has its OWN 5000/hr bucket, entirely separate from any installation token's. Recording a
+ * JWT-authenticated response's headers under `installation:{id}` (as getAppInstallation did) periodically wrote
+ * a HEALTHY remaining count over a genuinely exhausted installation's newest observation -- and the admission
+ * readers take only the newest REST row per key. `refresh-installation-health` runs unconditionally every 30
+ * minutes, so an exhausted installation's parked jobs were re-admitted on that cadence, burned their retries
+ * against 403s, and re-parked, for the whole exhaustion window.
+ */
+export function githubRateLimitAdmissionKeyForAppJwt(): GitHubRateLimitAdmissionKey {
+  return "app-jwt";
+}
+
 /** The SINGLE token→admission-key resolver, so every GitHub read attributes consistently and a token can never
  *  travel without its matching key: the public bucket for the shared public token, the installation bucket for an
  *  installation token with a known installation id, else undefined (unattributed). Callers pass whichever token
@@ -420,16 +432,43 @@ export async function isRateLimitedResponse(response: Response): Promise<boolean
   }
 }
 
+/** The server-instructed wait in ms when Retry-After is present and valid, else null. Kept separate from
+ *  {@link rateLimitRetryMs} so a caller can ask "would honoring this exceed what we're willing to sleep
+ *  inline?" WITHOUT the cap already having been applied (#9494). */
+export function retryAfterMs(response: Response): number | null {
+  const retryAfterHeader = response.headers.get("retry-after");
+  if (retryAfterHeader == null) return null;
+  const retryAfter = Number(retryAfterHeader);
+  if (!Number.isFinite(retryAfter) || retryAfter < 0) return null;
+  return retryAfter * 1000;
+}
+
+/**
+ * #9494: a secondary-limit Retry-After is typically 60s, but the inline budget is 8s -- so the old code
+ * truncated the server's instruction and retried up to 3 times inside the window GitHub explicitly asked us to
+ * stay out of, which its own docs warn can extend or escalate a secondary block. When the instructed wait
+ * exceeds the inline cap the right move is to stop retrying here and let the response surface: the QUEUE layer
+ * already honors Retry-After properly, with jitter (githubRateLimitRetryDelayMs / rateLimitRetryDelayWithJitter,
+ * src/selfhost/queue-common.ts), and defers sibling jobs for the same admission target too.
+ */
+export function exceedsInlineRetryBudget(response: Response): boolean {
+  const instructed = retryAfterMs(response);
+  return instructed !== null && instructed > GITHUB_RATE_LIMIT_MAX_DELAY_MS;
+}
+
 /** How long to wait before the next rate-limit retry: honor a valid Retry-After (seconds), else exponential
  *  backoff — each capped so a review can never stall on one call. A sustained PRIMARY limit (reset up to an hour
- *  out) simply exhausts the few inline retries and the queue retries the job later. Exported for tests. */
+ *  out) simply exhausts the few inline retries and the queue retries the job later. Exported for tests.
+ *
+ *  #9494: jittered. Up to QUEUE_CONCURRENCY workers can trip the same limit within milliseconds of each other;
+ *  a fixed 500/1000/2000 ladder had them all retry in lockstep, concentrating every attempt into the same
+ *  instants. The jitter is deterministic in `attempt` only insofar as it is bounded -- the randomness is what
+ *  spreads the herd, and a rate-limit retry has no replay/determinism contract to preserve. */
 export function rateLimitRetryMs(response: Response, attempt: number): number {
-  const retryAfterHeader = response.headers.get("retry-after");
-  if (retryAfterHeader != null) {
-    const retryAfter = Number(retryAfterHeader);
-    if (Number.isFinite(retryAfter) && retryAfter >= 0) return Math.min(retryAfter * 1000, GITHUB_RATE_LIMIT_MAX_DELAY_MS);
-  }
-  return Math.min(500 * 2 ** attempt, GITHUB_RATE_LIMIT_MAX_DELAY_MS);
+  const instructed = retryAfterMs(response);
+  if (instructed !== null) return Math.min(instructed, GITHUB_RATE_LIMIT_MAX_DELAY_MS);
+  const base = Math.min(500 * 2 ** attempt, GITHUB_RATE_LIMIT_MAX_DELAY_MS);
+  return Math.min(base + Math.floor(Math.random() * (base / 2)), GITHUB_RATE_LIMIT_MAX_DELAY_MS);
 }
 
 function responseFromCached(hit: CachedGitHubResponse, replayKind: "hit" | "coalesced"): Response {
@@ -483,6 +522,10 @@ async function fetchWithGitHubRetry(input: RequestInfo | URL, init?: GitHubTimeo
       attempt >= GITHUB_RATE_LIMIT_MAX_RETRIES ? "exhausted" : "scheduled",
     );
     if (attempt >= GITHUB_RATE_LIMIT_MAX_RETRIES) break;
+    // #9494: GitHub asked for longer than we are willing to sleep inline -- surface the response instead of
+    // retrying inside the window it told us to avoid. The queue's own rate-limit path honors the full
+    // Retry-After with jitter and defers sibling jobs for the same target.
+    if (exceedsInlineRetryBudget(response)) break;
     await sleep(rateLimitRetryMs(response, attempt));
   }
   return response;

@@ -10,6 +10,37 @@ interface RateLimitBody {
   windowSeconds?: number;
 }
 
+/**
+ * #9493: INCR-then-conditional-EXPIRE is two round trips, so a process death, a dropped connection, or an
+ * EXPIRE that simply errored between them left the key with **no TTL**. The fixed window then never resets:
+ * the counter climbs forever and, once past the limit, every request on that key is denied until someone
+ * deletes it by hand. The worst key is `strict:/v1/github/webhook:installation:<id>` — a stuck key means
+ * every webhook delivery for that installation is 429'd indefinitely, and GitHub does not auto-redeliver.
+ * The old code even papered over the symptom: a `PTTL` of -1 fell back to a full window, so the 429 kept
+ * promising "retry in 60s" forever.
+ *
+ * One script makes the increment and its expiry atomic, and repairs a key that somehow still has no TTL
+ * (`t < 0`) instead of leaving it immortal — so a key stranded by the old code self-heals on next contact
+ * rather than needing manual intervention. Returns `[count, ttlMs]`.
+ */
+const INCREMENT_WINDOW_SCRIPT = `
+local count = redis.call('INCR', KEYS[1])
+if count == 1 then
+  redis.call('PEXPIRE', KEYS[1], ARGV[1])
+end
+local ttl = redis.call('PTTL', KEYS[1])
+if ttl < 0 then
+  redis.call('PEXPIRE', KEYS[1], ARGV[1])
+  ttl = tonumber(ARGV[1])
+end
+return { count, ttl }
+`;
+
+async function incrementWindow(redis: Redis, key: string, windowSeconds: number): Promise<[number, number]> {
+  const reply = (await redis.eval(INCREMENT_WINDOW_SCRIPT, 1, key, String(windowSeconds * 1000))) as [number, number];
+  return [Number(reply[0]), Number(reply[1])];
+}
+
 export function createRedisRateLimiter(redis: Redis): DurableObjectNamespace {
   const stub = {
     // A DO stub's fetch is called fetch-style: `.fetch(url, init)`. On Workers the runtime builds the Request;
@@ -21,9 +52,9 @@ export function createRedisRateLimiter(redis: Redis): DurableObjectNamespace {
         return Response.json({ error: "invalid_rate_limit_request" }, { status: 400 });
       }
       const k = `ratelimit:${body.key}`;
-      const count = await redis.incr(k);
-      if (count === 1) await redis.expire(k, body.windowSeconds); // start the window on first hit
-      const ttlMs = await redis.pttl(k);
+      const [count, ttlMs] = await incrementWindow(redis, k, body.windowSeconds);
+      // PTTL is now guaranteed positive by the script's own repair arm, but keep the fallback: a future
+      // transport/serialization change that yields a non-numeric reply must not produce a NaN resetAt.
       const resetMs = ttlMs > 0 ? ttlMs : body.windowSeconds * 1000;
       const allowed = count <= body.limit;
       const decision = {

@@ -373,37 +373,46 @@ describe("private-beta auth and rate limiting", () => {
     expect(observedKeys[0]).toMatch(/^strict:\/v1\/auth\/github\/token:ip:/);
   });
 
-  it("keys /v1/github/webhook and /v1/orb/webhook by the payload's installation.id, not IP (#4891) -- a shared hosted egress path can't collide two tenants' buckets", async () => {
+  it("keys /v1/github/webhook and /v1/orb/webhook by installation.id AND the connecting IP (#4891, #9469)", async () => {
     const observedKeys: string[] = [];
     const env = rateLimitTestEnv({}, observedKeys);
 
-    // Two DIFFERENT installations from the SAME connecting IP (a shared hosted egress) get independent buckets.
+    // #4891's requirement, unchanged: two DIFFERENT installations from the SAME connecting IP (a shared hosted
+    // egress) must get independent buckets, so one tenant's traffic can never exhaust another's.
     await expect(
       enforceRateLimit(fakeContext(env, "/v1/github/webhook", { "cf-connecting-ip": "203.0.113.9" }, { installation: { id: 111 } }), "strict"),
     ).resolves.toBeNull();
     await expect(
       enforceRateLimit(fakeContext(env, "/v1/github/webhook", { "cf-connecting-ip": "203.0.113.9" }, { installation: { id: 222 } }), "strict"),
     ).resolves.toBeNull();
     expect(observedKeys).toHaveLength(2);
-    expect(observedKeys[0]).toBe("strict:/v1/github/webhook:installation:111");
-    expect(observedKeys[1]).toBe("strict:/v1/github/webhook:installation:222");
-
-    // The SAME installation from two DIFFERENT IPs shares one bucket.
+    expect(observedKeys[0]).not.toBe(observedKeys[1]);
+    expect(observedKeys[0]).toContain("installation:111");
+    expect(observedKeys[1]).toContain("installation:222");
+
+    // #9469: the SAME installation from two DIFFERENT IPs must NOT share a bucket. installation.id is read from
+    // the body BEFORE signature verification, so it is attacker-controlled -- when it alone keyed the bucket,
+    // anyone could pin a victim installation's 10/60s window with unsigned requests and GitHub's real
+    // deliveries would 429 (and GitHub does not auto-redeliver). Folding in the connecting IP means an attacker
+    // and GitHub can never occupy the same bucket.
     observedKeys.length = 0;
     await expect(
       enforceRateLimit(fakeContext(env, "/v1/github/webhook", { "cf-connecting-ip": "203.0.113.9" }, { installation: { id: 333 } }), "strict"),
     ).resolves.toBeNull();
     await expect(
       enforceRateLimit(fakeContext(env, "/v1/github/webhook", { "cf-connecting-ip": "198.51.100.50" }, { installation: { id: 333 } }), "strict"),
     ).resolves.toBeNull();
-    expect(observedKeys[0]).toBe(observedKeys[1]);
+    expect(observedKeys[0]).not.toBe(observedKeys[1]);
+    // Both still carry the installation identity -- the tenant separation #4891 added is not lost.
+    expect(observedKeys[0]).toContain("installation:333");
+    expect(observedKeys[1]).toContain("installation:333");
 
-    // /v1/orb/webhook shares the same installation-keying logic.
+    // /v1/orb/webhook shares the same keying logic.
     observedKeys.length = 0;
     await expect(
       enforceRateLimit(fakeContext(env, "/v1/orb/webhook", { "cf-connecting-ip": "203.0.113.9" }, { installation: { id: 444 } }), "strict"),
     ).resolves.toBeNull();
-    expect(observedKeys[0]).toBe("strict:/v1/orb/webhook:installation:444");
+    expect(observedKeys[0]).toMatch(/^strict:\/v1\/orb\/webhook:installation:444:ip:/);
   });
 
   it("falls back to IP-keying /v1/github/webhook when installation.id can't be resolved (#4891)", async () => {

@@ -19,6 +19,7 @@ import {
   isGitHubBadCredentialsError,
   isGitHubRateLimitedError,
   isRateLimitedResponse,
+  exceedsInlineRetryBudget,
   rateLimitRetryMs,
   setGitHubResponseCache,
   setInstallationTokenStore,
@@ -2634,8 +2635,13 @@ describe("GitHub check runs", () => {
 
     // 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.
+    // #9494: recorded under `app-jwt`, NOT `installation:123`. This call authenticates with the App JWT, whose
+    // 5000/hr bucket is separate from the installation's -- filing its healthy remaining count under the
+    // installation key overwrote a genuinely exhausted installation's newest observation every 30 minutes
+    // (refresh-installation-health's cadence), re-admitting its parked jobs to burn retries against 403s.
+    // #4506's requirement (the call is VISIBLE to admission reads) is unchanged; only its attribution is fixed.
     expect(await listLatestGitHubRateLimitObservations(env)).toEqual(
-      expect.arrayContaining([expect.objectContaining({ path: "/app/installations/123", statusCode: 200, remaining: 4321, admissionKey: "installation:123" })]),
+      expect.arrayContaining([expect.objectContaining({ path: "/app/installations/123", statusCode: 200, remaining: 4321, admissionKey: "app-jwt" })]),
     );
   });
 
@@ -3023,9 +3029,29 @@ describe("GitHub rate-limit handling (#ratelimit-resilience)", () => {
       expect(rateLimitRetryMs(new Response("", { headers: { "retry-after": "9999" } }), 0)).toBe(8000);
       expect(rateLimitRetryMs(new Response("", { headers: { "retry-after": "0" } }), 0)).toBe(0);
     });
-    it("falls back to exponential backoff when Retry-After is absent or invalid", () => {
-      expect(rateLimitRetryMs(new Response("", {}), 2)).toBe(2000); // 500 * 2^2, no header
-      expect(rateLimitRetryMs(new Response("", { headers: { "retry-after": "soon" } }), 0)).toBe(500); // invalid → 500 * 2^0
+    it("falls back to jittered exponential backoff when Retry-After is absent or invalid (#9494)", () => {
+      // Jitter spreads the herd: up to QUEUE_CONCURRENCY workers can trip the same limit within milliseconds,
+      // and a fixed ladder had them all retry in lockstep. The base is unchanged; the added spread is [0, base/2).
+      const noHeader = rateLimitRetryMs(new Response("", {}), 2); // base 500 * 2^2
+      expect(noHeader).toBeGreaterThanOrEqual(2000);
+      expect(noHeader).toBeLessThan(3000);
+      const invalidHeader = rateLimitRetryMs(new Response("", { headers: { "retry-after": "soon" } }), 0); // base 500 * 2^0
+      expect(invalidHeader).toBeGreaterThanOrEqual(500);
+      expect(invalidHeader).toBeLessThan(750);
+    });
+
+    it("honors a Retry-After within the inline budget exactly, without jitter (#9494)", () => {
+      // A server-instructed wait is authoritative -- jitter applies only to our own invented backoff.
+      expect(rateLimitRetryMs(new Response("", { headers: { "retry-after": "3" } }), 0)).toBe(3000);
+    });
+
+    it("reports a Retry-After beyond the inline budget so the caller stops retrying inline (#9494)", () => {
+      // A secondary-limit Retry-After is typically 60s against an 8s inline cap. Retrying inside the window
+      // GitHub asked us to avoid can extend the block; the queue honors the full wait with jitter instead.
+      expect(exceedsInlineRetryBudget(new Response("", { headers: { "retry-after": "60" } }))).toBe(true);
+      expect(exceedsInlineRetryBudget(new Response("", { headers: { "retry-after": "3" } }))).toBe(false);
+      expect(exceedsInlineRetryBudget(new Response("", {}))).toBe(false);
+      expect(exceedsInlineRetryBudget(new Response("", { headers: { "retry-after": "soon" } }))).toBe(false);
     });
   });