Skip to content

fix(queue): prefer exact github rate-limit admission buckets #2417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 2 commits into from
Jul 1, 2026
Merged

fix(queue): prefer exact github rate-limit admission buckets #2417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d49b8c
fix(queue): prefer exact github rate-limit admission buckets
JSONbored Jul 1, 2026
4456074
fix(queue): stop a healthy fallback from clearing a genuine exact exh…
JSONbored Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/selfhost/queue-common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}

Expand Down
23 changes: 15 additions & 8 deletions test/unit/selfhost-pg-queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -744,18 +747,22 @@ 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;
else process.env.QUEUE_RATE_LIMIT_JITTER_MS = oldJitter;
}
});

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;
Expand All @@ -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;
Expand Down
140 changes: 138 additions & 2 deletions test/unit/selfhost-queue-common.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
jobCoalesceKey,
jobCoalesceSupersededKeyPrefix,
jobPriority,
matchesGitHubRateLimitAdmissionTarget,
nonConsumingRetryDelayMs,
queueBackgroundConcurrency,
queueProcessingTimeoutMs,
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -262,7 +270,7 @@ describe("self-host queue common helpers", () => {
],
now,
),
).toBeNull();
).toBe(615_000);
expect(
githubRateLimitAdmissionDelayMs(
"webhook",
Expand Down Expand Up @@ -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);
Expand Down
37 changes: 22 additions & 15 deletions test/unit/selfhost-sqlite-queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -495,25 +498,20 @@ 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;
vi.useRealTimers();
}
});

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;
Expand Down Expand Up @@ -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;
Expand Down
Loading