diff --git a/src/selfhost/queue-common.ts b/src/selfhost/queue-common.ts index 7b202f7002..44f46863cb 100644 --- a/src/selfhost/queue-common.ts +++ b/src/selfhost/queue-common.ts @@ -273,11 +273,16 @@ function fallbackObservationCanOverrideExact( exact: AdmissionObservation | null, ): 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; + // 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 { @@ -379,9 +384,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; } diff --git a/test/unit/selfhost-pg-queue.test.ts b/test/unit/selfhost-pg-queue.test.ts index fe842d4589..18550fb82f 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; @@ -755,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; @@ -772,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 468b8a54d0..c3e95d689a 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,10 @@ describe("self-host queue common helpers", () => { ], now, ), - ).toBe(615_000); + ).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", @@ -262,7 +270,7 @@ describe("self-host queue common helpers", () => { ], now, ), - ).toBeNull(); + ).toBe(615_000); expect( githubRateLimitAdmissionDelayMs( "webhook", @@ -308,6 +316,134 @@ 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 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", + 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, + ), + ).toBe(615_000); + }); + + 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..e00459873d 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; @@ -513,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; @@ -551,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;