From 5d49b8cb922ede314f27d9300b41a11f30e1dcbf Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 13:20:07 -0700 Subject: [PATCH 1/2] fix(queue): prefer exact github rate-limit admission buckets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A newer unkeyed/null legacy rate-limit observation could override an older-but-healthy exact installation-keyed observation purely on recency, deferring an entire installation's webhook queue even though that installation's own budget was fine. This happened repeatedly on self-host: ~147 pending github-webhook jobs piled up with last_error 'github rate-limit webhook admission' while the unkeyed fallback bucket (a different, unrelated consumer) cycled through exhaustion and reset faster than the installation-scoped exact observation was refreshed. fallbackObservationCanOverrideExact now only lets a newer fallback win when doing so is MORE permissive than the exact reading (clearing a stale exhaustion) — never when it would introduce a delay the exact observation alone would not have. A fallback still governs when no exact observation exists at all. matchesGitHubRateLimitAdmissionTarget had the same failure class in the reactive (post-failure) defer path: a confirmed rate-limit error on a job with no admission key (legacy/unknown actor work) unconditionally parked every OTHER pending job regardless of its own key. It now only parks other null-keyed candidates, matching the precedent already used for keyed blocked targets. --- src/selfhost/queue-common.ts | 27 +++-- test/unit/selfhost-pg-queue.test.ts | 11 +- test/unit/selfhost-queue-common.test.ts | 130 +++++++++++++++++++++++- test/unit/selfhost-sqlite-queue.test.ts | 18 ++-- 4 files changed, 163 insertions(+), 23 deletions(-) diff --git a/src/selfhost/queue-common.ts b/src/selfhost/queue-common.ts index 7b202f7002..557a29e026 100644 --- a/src/selfhost/queue-common.ts +++ b/src/selfhost/queue-common.ts @@ -269,15 +269,26 @@ function rateLimitAdmissionDelayForObservation( } function fallbackObservationCanOverrideExact( + kind: GitHubRateLimitAdmissionKind, fallback: AdmissionObservation | null, exact: AdmissionObservation | null, + nowMs: number, ): boolean { if (!fallback) return false; if (!exact) return true; const fallbackMs = observationMs(fallback); const exactMs = observationMs(exact); - if (fallbackMs === null) return false; - return exactMs === null || fallbackMs > exactMs; + if (fallbackMs === null || exactMs === null || fallbackMs <= exactMs) return false; + // The fallback is a newer observation, but recency alone must not let it move admission from + // permissive to restrictive: a null/unkeyed fallback row is frequently a DIFFERENT bucket (a public + // token, another consumer's traffic, or a pre-migration write that never carried an admission_key), + // so a newer-but-exhausted fallback pinning an otherwise-healthy, concretely-keyed installation + // bucket is a false positive, not a real signal about that installation's budget. Only let a newer + // fallback override when it is MORE permissive than the exact reading (clearing a stale exhaustion) — + // never when it would introduce a delay the exact observation alone would not have. + const exactDelay = rateLimitAdmissionDelayForObservation(kind, exact, nowMs); + const fallbackDelay = rateLimitAdmissionDelayForObservation(kind, fallback, nowMs); + return exactDelay !== null && fallbackDelay === null; } export function githubRateLimitAdmissionKeyForJob(message: JobMessage): GitHubRateLimitAdmissionKey | null { @@ -379,9 +390,13 @@ export function matchesGitHubRateLimitAdmissionTarget( blocked: GitHubRateLimitAdmissionTarget, ): boolean { if (candidate === null) return false; - // Null-key GitHub jobs are legacy/unknown actor work; park them with a depleted known bucket, - // and park all GitHub-budget work when the depleted bucket itself is unknown. - if (blocked.admissionKey === null) return true; + // A null-key CANDIDATE is legacy/unknown-actor work whose true bucket we can't prove is unaffected, + // so it still parks alongside any confirmed exhaustion (known-keyed or null-keyed alike). But a + // null-key BLOCKED target (the job that actually failed had no admissionKey) does NOT justify + // parking every OTHER concretely-keyed installation's work too -- we only know ONE unscoped bucket + // is exhausted, not that a SPECIFIC installation's own budget is affected. Scoping this the same way + // as a keyed blocked target avoids the same false-positive class as a stale unkeyed observation + // pinning a healthy installation's webhooks (mirrors fallbackObservationCanOverrideExact above). return candidate.admissionKey === blocked.admissionKey || candidate.admissionKey === null; } @@ -405,7 +420,7 @@ export function githubRateLimitAdmissionDelayMs( fallback = newerRateLimitObservation(fallback, candidate); } } - const observation = fallbackObservationCanOverrideExact(fallback, exact) + const observation = fallbackObservationCanOverrideExact(kind, fallback, exact, nowMs) ? fallback : exact; return rateLimitAdmissionDelayForObservation(kind, observation, nowMs); diff --git a/test/unit/selfhost-pg-queue.test.ts b/test/unit/selfhost-pg-queue.test.ts index fe842d4589..e9b6311ef1 100644 --- a/test/unit/selfhost-pg-queue.test.ts +++ b/test/unit/selfhost-pg-queue.test.ts @@ -727,7 +727,10 @@ describe("createPgQueue (durable #977)", () => { } }); - it("pre-yields from legacy repo exhaustion before older healthy exact observations", async () => { + it("REGRESSION: a newer legacy unkeyed exhaustion does not pin a healthy exact installation observation (self-host webhook backlog)", async () => { + // Before the fix: a stale/legacy null-admission_key row that happened to be observed MORE RECENTLY + // than the installation's own (healthy) exact reading would win purely on recency, deferring every + // webhook for a perfectly healthy installation. The exact reading must govern here. vi.useFakeTimers({ toFake: ["Date"] }); vi.setSystemTime(new Date("2026-06-24T12:00:00.000Z")); const oldJitter = process.env.QUEUE_RATE_LIMIT_JITTER_MS; @@ -744,10 +747,10 @@ describe("createPgQueue (durable #977)", () => { await q.drain(); - expect(seen).toEqual([]); - expect(m.pool.query).toHaveBeenCalledWith( + expect(seen).toEqual(["github-webhook"]); + expect(m.pool.query).not.toHaveBeenCalledWith( expect.stringContaining("SET status='pending', run_after=GREATEST"), - [Date.parse("2026-06-24T12:10:15.000Z"), "github rate-limit webhook admission", "webhook"], + expect.anything(), ); } finally { if (oldJitter === undefined) delete process.env.QUEUE_RATE_LIMIT_JITTER_MS; diff --git a/test/unit/selfhost-queue-common.test.ts b/test/unit/selfhost-queue-common.test.ts index 468b8a54d0..97fa845949 100644 --- a/test/unit/selfhost-queue-common.test.ts +++ b/test/unit/selfhost-queue-common.test.ts @@ -18,6 +18,7 @@ import { jobCoalesceKey, jobCoalesceSupersededKeyPrefix, jobPriority, + matchesGitHubRateLimitAdmissionTarget, nonConsumingRetryDelayMs, queueBackgroundConcurrency, queueProcessingTimeoutMs, @@ -241,6 +242,10 @@ describe("self-host queue common helpers", () => { now, ), ).toBeNull(); + // A newer unkeyed/legacy fallback must NOT suppress a healthy exact installation observation, even + // though it is the most recently observed row -- the fallback is very likely an unrelated bucket + // (a public token, another consumer, or a pre-migration write), and recency alone is not evidence + // that THIS installation's own budget is exhausted (the incident this regression guards against). expect( githubRateLimitAdmissionDelayMs( "webhook", @@ -251,7 +256,7 @@ describe("self-host queue common helpers", () => { ], now, ), - ).toBe(615_000); + ).toBeNull(); expect( githubRateLimitAdmissionDelayMs( "webhook", @@ -308,6 +313,129 @@ describe("self-host queue common helpers", () => { ).toBe(615_000); }); + describe("fallback vs exact admission precedence (self-host webhook backlog regression)", () => { + const now = Date.parse("2026-06-24T12:00:00.000Z"); + const key = githubRateLimitAdmissionKeyForInstallation(123); + + it("REGRESSION: a healthy, newer-enough exact installation observation is never suppressed by a newer unkeyed exhausted fallback", () => { + expect( + githubRateLimitAdmissionDelayMs( + "webhook", + key, + [ + { admission_key: key, remaining: 4000, reset_at: "2026-06-24T12:20:00.000Z", observed_at: "2026-06-24T11:59:30.000Z" }, + { admission_key: null, remaining: 0, reset_at: "2026-06-24T12:01:00.000Z", observed_at: "2026-06-24T12:00:00.000Z" }, + ], + now, + ), + ).toBeNull(); + }); + + it("REGRESSION: no exact installation observation + an exhausted unkeyed fallback still defers webhook admission", () => { + expect( + githubRateLimitAdmissionDelayMs( + "webhook", + key, + [{ admission_key: null, remaining: 0, reset_at: "2026-06-24T12:10:00.000Z", observed_at: "2026-06-24T12:00:00.000Z" }], + now, + ), + ).toBe(615_000); + }); + + it("REGRESSION: an exhausted exact installation observation alone still defers webhook admission", () => { + expect( + githubRateLimitAdmissionDelayMs( + "webhook", + key, + [{ admission_key: key, remaining: 0, reset_at: "2026-06-24T12:10:00.000Z", observed_at: "2026-06-24T12:00:00.000Z" }], + now, + ), + ).toBe(615_000); + }); + + it("INVARIANT: a newer unkeyed fallback can still CLEAR a stale exhausted exact observation (recovery is allowed; only new restriction is not)", () => { + expect( + githubRateLimitAdmissionDelayMs( + "webhook", + key, + [ + { admission_key: key, remaining: 0, reset_at: "2026-06-24T12:10:00.000Z", observed_at: "2026-06-24T11:59:00.000Z" }, + { admission_key: null, remaining: 4000, reset_at: "2026-06-24T12:20:00.000Z", observed_at: "2026-06-24T12:00:00.000Z" }, + ], + now, + ), + ).toBeNull(); + }); + + it("background admission observes the same precedence: a newer exhausted fallback cannot suppress a healthy exact background observation", () => { + expect( + githubRateLimitAdmissionDelayMs( + "background", + key, + [ + { admission_key: key, remaining: 4000, reset_at: "2026-06-24T12:20:00.000Z", observed_at: "2026-06-24T11:59:30.000Z" }, + { admission_key: null, remaining: 0, reset_at: "2026-06-24T12:01:00.000Z", observed_at: "2026-06-24T12:00:00.000Z" }, + ], + now, + ), + ).toBeNull(); + }); + }); + + describe("matchesGitHubRateLimitAdmissionTarget", () => { + const installationKey = githubRateLimitAdmissionKeyForInstallation(123); + const otherInstallationKey = githubRateLimitAdmissionKeyForInstallation(456); + + it("returns false for a candidate that is not GitHub-budget work at all", () => { + expect(matchesGitHubRateLimitAdmissionTarget(null, { kind: "webhook", admissionKey: installationKey })).toBe(false); + }); + + it("matches a candidate sharing the same admission key as a keyed blocked target", () => { + expect( + matchesGitHubRateLimitAdmissionTarget( + { kind: "webhook", admissionKey: installationKey }, + { kind: "webhook", admissionKey: installationKey }, + ), + ).toBe(true); + }); + + it("still conservatively matches a null-keyed (legacy/unknown) candidate against a keyed blocked target", () => { + expect( + matchesGitHubRateLimitAdmissionTarget( + { kind: "webhook", admissionKey: null }, + { kind: "webhook", admissionKey: installationKey }, + ), + ).toBe(true); + }); + + it("does not match a DIFFERENT concretely-keyed candidate against a keyed blocked target", () => { + expect( + matchesGitHubRateLimitAdmissionTarget( + { kind: "webhook", admissionKey: otherInstallationKey }, + { kind: "webhook", admissionKey: installationKey }, + ), + ).toBe(false); + }); + + it("REGRESSION: a null-keyed blocked target no longer parks EVERY concretely-keyed candidate (only null-keyed ones)", () => { + // Before the fix, a confirmed rate-limit failure on a job with NO admission key (legacy/unknown + // actor work) would defer every OTHER pending job regardless of its own key -- the same false + // positive class as a stale unkeyed observation pinning a healthy installation's webhooks. + expect( + matchesGitHubRateLimitAdmissionTarget( + { kind: "webhook", admissionKey: installationKey }, + { kind: "webhook", admissionKey: null }, + ), + ).toBe(false); + expect( + matchesGitHubRateLimitAdmissionTarget( + { kind: "webhook", admissionKey: null }, + { kind: "webhook", admissionKey: null }, + ), + ).toBe(true); + }); + }); + it("uses the newest local REST rate-limit observation for admission control", async () => { const now = Date.parse("2026-06-24T12:00:00.000Z"); const key = githubRateLimitAdmissionKeyForInstallation(123); diff --git a/test/unit/selfhost-sqlite-queue.test.ts b/test/unit/selfhost-sqlite-queue.test.ts index fe7a5533b5..282b1c1de3 100644 --- a/test/unit/selfhost-sqlite-queue.test.ts +++ b/test/unit/selfhost-sqlite-queue.test.ts @@ -457,7 +457,10 @@ describe("createSqliteQueue (durable #980)", () => { } }); - it("pre-yields from legacy repo exhaustion before older healthy exact observations", async () => { + it("REGRESSION: a newer legacy unkeyed exhaustion does not pin a healthy exact installation observation (self-host webhook backlog)", async () => { + // Before the fix: a stale/legacy null-admission_key row that happened to be observed MORE RECENTLY + // than the installation's own (healthy) exact reading would win purely on recency, deferring every + // webhook for a perfectly healthy installation. The exact reading must govern here. vi.useFakeTimers({ toFake: ["Date"] }); vi.setSystemTime(new Date("2026-06-24T12:00:00.000Z")); const oldJitter = process.env.QUEUE_RATE_LIMIT_JITTER_MS; @@ -495,17 +498,8 @@ describe("createSqliteQueue (durable #980)", () => { await q.binding.send({ type: "github-webhook", deliveryId: "fresh", eventName: "pull_request", payload: { installation: { id: 123 }, repository: { full_name: "owner/repo" } } }); await q.drain(); - expect(seen).toEqual([]); - const row = driver.query( - "SELECT status, attempts, run_after, last_error FROM _selfhost_jobs", - [], - ).rows[0] as { status: string; attempts: number; run_after: number; last_error: string }; - expect(row).toMatchObject({ - status: "pending", - attempts: 0, - run_after: Date.parse("2026-06-24T12:10:15.000Z"), - last_error: "github rate-limit webhook admission", - }); + expect(seen).toEqual(["github-webhook"]); + expect(q.stats()).not.toHaveProperty("gittensory_jobs_rate_limit_deferred_total"); } finally { if (oldJitter === undefined) delete process.env.QUEUE_RATE_LIMIT_JITTER_MS; else process.env.QUEUE_RATE_LIMIT_JITTER_MS = oldJitter; From 4456074ed74639008deaeeef42f1ed58b962ddf2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 13:37:58 -0700 Subject: [PATCH 2/2] fix(queue): stop a healthy fallback from clearing a genuine exact exhaustion too Review feedback on the initial fix correctly identified that the new rule was asymmetric without justification: a null/unkeyed observation is not proven to report on the SAME budget as an exact installation key, so its signal is equally untrustworthy in BOTH directions, not just when it would introduce a new restriction. Simplify to: once an exact observation exists for an admission key, it alone governs (its own reset_at already bounds how long an exhaustion can block admission); the fallback only ever applies when no exact observation exists at all. --- src/selfhost/queue-common.ts | 28 ++++++++++--------------- test/unit/selfhost-pg-queue.test.ts | 12 +++++++---- test/unit/selfhost-queue-common.test.ts | 14 ++++++++++--- test/unit/selfhost-sqlite-queue.test.ts | 19 ++++++++++++++--- 4 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/selfhost/queue-common.ts b/src/selfhost/queue-common.ts index 557a29e026..44f46863cb 100644 --- a/src/selfhost/queue-common.ts +++ b/src/selfhost/queue-common.ts @@ -269,26 +269,20 @@ function rateLimitAdmissionDelayForObservation( } function fallbackObservationCanOverrideExact( - kind: GitHubRateLimitAdmissionKind, fallback: AdmissionObservation | null, exact: AdmissionObservation | null, - nowMs: number, ): boolean { if (!fallback) return false; - if (!exact) return true; - const fallbackMs = observationMs(fallback); - const exactMs = observationMs(exact); - if (fallbackMs === null || exactMs === null || fallbackMs <= exactMs) return false; - // The fallback is a newer observation, but recency alone must not let it move admission from - // permissive to restrictive: a null/unkeyed fallback row is frequently a DIFFERENT bucket (a public - // token, another consumer's traffic, or a pre-migration write that never carried an admission_key), - // so a newer-but-exhausted fallback pinning an otherwise-healthy, concretely-keyed installation - // bucket is a false positive, not a real signal about that installation's budget. Only let a newer - // fallback override when it is MORE permissive than the exact reading (clearing a stale exhaustion) — - // never when it would introduce a delay the exact observation alone would not have. - const exactDelay = rateLimitAdmissionDelayForObservation(kind, exact, nowMs); - const fallbackDelay = rateLimitAdmissionDelayForObservation(kind, fallback, nowMs); - return exactDelay !== null && fallbackDelay === null; + // A null/unkeyed fallback row is frequently a DIFFERENT bucket entirely (a public token, another + // consumer's traffic, or a pre-migration write that never carried an admission_key) -- we have no + // evidence it reports on the SAME budget as this admission key. That untrustworthiness applies + // regardless of which direction the fallback's reading points: it must not suppress a healthy exact + // observation (the original bug), but it must equally not CLEAR a genuine exact exhaustion either -- + // both are the same category of false signal, just pointing opposite ways. Once an exact observation + // exists for this key, it alone governs; the exact reading's own reset_at already bounds how long an + // exhaustion can block admission, so there is no correctness reason to let an unrelated bucket + // override it in either direction. Fallback governs ONLY when no exact observation exists at all. + return !exact; } export function githubRateLimitAdmissionKeyForJob(message: JobMessage): GitHubRateLimitAdmissionKey | null { @@ -420,7 +414,7 @@ export function githubRateLimitAdmissionDelayMs( fallback = newerRateLimitObservation(fallback, candidate); } } - const observation = fallbackObservationCanOverrideExact(kind, fallback, exact, nowMs) + const observation = fallbackObservationCanOverrideExact(fallback, exact) ? fallback : exact; return rateLimitAdmissionDelayForObservation(kind, observation, nowMs); diff --git a/test/unit/selfhost-pg-queue.test.ts b/test/unit/selfhost-pg-queue.test.ts index e9b6311ef1..18550fb82f 100644 --- a/test/unit/selfhost-pg-queue.test.ts +++ b/test/unit/selfhost-pg-queue.test.ts @@ -758,7 +758,11 @@ describe("createPgQueue (durable #977)", () => { } }); - it("does not keep webhook admission closed from stale exact rows after a newer healthy legacy observation", async () => { + it("REGRESSION: a newer healthy legacy observation does not clear a genuine exact installation exhaustion", async () => { + // An unkeyed/legacy fallback is not proven to report on the SAME budget as the exact installation + // key, so it must not "clear" a real exhaustion any more than it should be able to suppress a + // healthy exact reading -- both directions trust an unrelated bucket's signal over this + // installation's own. The exact observation's own reset_at already bounds the wait. vi.useFakeTimers({ toFake: ["Date"] }); vi.setSystemTime(new Date("2026-06-24T12:00:00.000Z")); const oldJitter = process.env.QUEUE_RATE_LIMIT_JITTER_MS; @@ -775,10 +779,10 @@ describe("createPgQueue (durable #977)", () => { await q.drain(); - expect(seen).toEqual(["github-webhook"]); - expect(m.pool.query).not.toHaveBeenCalledWith( + expect(seen).toEqual([]); + expect(m.pool.query).toHaveBeenCalledWith( expect.stringContaining("SET status='pending', run_after=GREATEST"), - expect.anything(), + [Date.parse("2026-06-24T12:10:15.000Z"), "github rate-limit webhook admission", "webhook"], ); } finally { if (oldJitter === undefined) delete process.env.QUEUE_RATE_LIMIT_JITTER_MS; diff --git a/test/unit/selfhost-queue-common.test.ts b/test/unit/selfhost-queue-common.test.ts index 97fa845949..c3e95d689a 100644 --- a/test/unit/selfhost-queue-common.test.ts +++ b/test/unit/selfhost-queue-common.test.ts @@ -257,6 +257,9 @@ describe("self-host queue common helpers", () => { now, ), ).toBeNull(); + // A newer unkeyed/legacy fallback must not CLEAR a genuine exact exhaustion either -- it is the + // same untrustworthy, unrelated-bucket signal as the suppression case above, just pointing the + // other way. The exact reading's own reset_at already bounds how long this can block admission. expect( githubRateLimitAdmissionDelayMs( "webhook", @@ -267,7 +270,7 @@ describe("self-host queue common helpers", () => { ], now, ), - ).toBeNull(); + ).toBe(615_000); expect( githubRateLimitAdmissionDelayMs( "webhook", @@ -353,7 +356,12 @@ describe("self-host queue common helpers", () => { ).toBe(615_000); }); - it("INVARIANT: a newer unkeyed fallback can still CLEAR a stale exhausted exact observation (recovery is allowed; only new restriction is not)", () => { + it("INVARIANT: a newer unkeyed fallback cannot CLEAR a genuine exact exhaustion either -- an untrusted bucket is untrusted in both directions", () => { + // A null/unkeyed fallback is not proven to report on the SAME budget as this admission key, so + // it must not move admission in EITHER direction once an exact observation exists: it can't + // suppress a healthy exact reading (the original bug), and it equally can't manufacture an early + // "recovery" for a genuinely exhausted one. The exact reading's own reset_at already bounds the + // wait. expect( githubRateLimitAdmissionDelayMs( "webhook", @@ -364,7 +372,7 @@ describe("self-host queue common helpers", () => { ], now, ), - ).toBeNull(); + ).toBe(615_000); }); it("background admission observes the same precedence: a newer exhausted fallback cannot suppress a healthy exact background observation", () => { diff --git a/test/unit/selfhost-sqlite-queue.test.ts b/test/unit/selfhost-sqlite-queue.test.ts index 282b1c1de3..e00459873d 100644 --- a/test/unit/selfhost-sqlite-queue.test.ts +++ b/test/unit/selfhost-sqlite-queue.test.ts @@ -507,7 +507,11 @@ describe("createSqliteQueue (durable #980)", () => { } }); - it("does not keep webhook admission closed from stale exact rows after a newer healthy legacy observation", async () => { + it("REGRESSION: a newer healthy legacy observation does not clear a genuine exact installation exhaustion", async () => { + // An unkeyed/legacy fallback is not proven to report on the SAME budget as the exact installation + // key, so it must not "clear" a real exhaustion any more than it should be able to suppress a + // healthy exact reading -- both directions trust an unrelated bucket's signal over this + // installation's own. The exact observation's own reset_at already bounds the wait. vi.useFakeTimers({ toFake: ["Date"] }); vi.setSystemTime(new Date("2026-06-24T12:00:00.000Z")); const oldJitter = process.env.QUEUE_RATE_LIMIT_JITTER_MS; @@ -545,8 +549,17 @@ describe("createSqliteQueue (durable #980)", () => { await q.binding.send({ type: "github-webhook", deliveryId: "fresh", eventName: "pull_request", payload: { installation: { id: 123 }, repository: { full_name: "owner/repo" } } }); await q.drain(); - expect(seen).toEqual(["github-webhook"]); - expect(q.stats()).not.toHaveProperty("gittensory_jobs_rate_limit_deferred_total"); + expect(seen).toEqual([]); + const row = driver.query( + "SELECT status, attempts, run_after, last_error FROM _selfhost_jobs", + [], + ).rows[0] as { status: string; attempts: number; run_after: number; last_error: string }; + expect(row).toMatchObject({ + status: "pending", + attempts: 0, + run_after: Date.parse("2026-06-24T12:10:15.000Z"), + last_error: "github rate-limit webhook admission", + }); } finally { if (oldJitter === undefined) delete process.env.QUEUE_RATE_LIMIT_JITTER_MS; else process.env.QUEUE_RATE_LIMIT_JITTER_MS = oldJitter;