Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 6 additions & 10 deletions src/selfhost/pg-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1179,16 +1179,12 @@ export function createPgQueue(
}
// Per-installation GitHub-fetch concurrency admission (#selfhost-installation-concurrency), the last-mile
// gate: only reached by a job that already passed rate-limit admission and (if applicable) maintenance-
// lane admission above, immediately before it actually claims a dispatch slot. Explicitly excludes
// foreground-priority jobs (mirrors the maintenance-admission guard above) -- isGitHubBudgetBackgroundJob
// (which installationConcurrencyKeyForJob is built on) answers "does this job draw GitHub rate-limit
// BUDGET", which is also true for a live (non-sweep, non-manual) agent-regate-pr job; that job is still
// FOREGROUND priority and must never be deferred by this policy, so the exclusion is a separate, explicit
// check here rather than folded into the key resolver itself. installationConcurrencyKey is null for
// background jobs whose payload carries no resolvable installationId too -- those fall through unaffected.
const installationConcurrencyKey = isForegroundJobPriority(Number(job.priority))
? null
: installationConcurrencyKeyForJob(message);
// lane admission above, immediately before it actually claims a dispatch slot. installationConcurrencyKeyForJob
// already excludes the truly-foreground agent-regate-pr job type by construction (not by priority -- see its
// own doc comment for why agent-regate-sweep's priority-8/floor-8 collision rules out a priority-based
// guard here). installationConcurrencyKey is null for background jobs whose payload carries no resolvable
// installationId too -- those fall through unaffected.
const installationConcurrencyKey = installationConcurrencyKeyForJob(message);
if (installationConcurrencyKey) {
const decision = evaluateInstallationConcurrencyAdmission(
installationConcurrencyConfig,
Expand Down
13 changes: 8 additions & 5 deletions src/selfhost/queue-common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -406,13 +406,16 @@ export function githubRateLimitAdmissionKeyForJob(message: JobMessage): GitHubRa
// THIS job under, or null when the job either makes no GitHub calls isGitHubBudgetBackgroundJob cares about, or
// carries no resolvable installationId. Reusing githubRateLimitAdmissionKeyForJob (rather than inventing a
// second key function) keeps the rate-limit-admission key and the concurrency-admission key for the same job
// always identical by construction. NOTE: isGitHubBudgetBackgroundJob is true for a live (non-sweep, non-manual)
// always identical by construction. isGitHubBudgetBackgroundJob is true for a live (non-sweep, non-manual)
// agent-regate-pr job too, since that job DOES draw GitHub rate-limit budget under this key -- but that job is
// still FOREGROUND priority. This function deliberately does NOT filter foreground jobs out itself (it answers
// "what key would this job's GitHub calls draw against", not "should a background-only policy apply to it") --
// the caller (pg-queue.ts/sqlite-queue.ts) is responsible for its own `!isForegroundJobPriority(...)` guard
// before ever calling this, exactly mirroring how isMaintenanceJobType is similarly guarded at its own call site.
// still FOREGROUND priority (AGENT_REGATE_PRIORITY, 9) and must never be deferred by this policy, so it is
// excluded here BY TYPE. This is deliberately NOT a priority-based exclusion (e.g. `!isForegroundJobPriority`):
// agent-regate-sweep's own row priority (8, PRIORITY_BY_TYPE) collides with FOREGROUND_QUEUE_PRIORITY_FLOOR
// (also 8), so a priority-floor guard would silently exempt sweep fan-out too -- exactly the background job this
// policy exists to bound (#selfhost-installation-concurrency-sweep-gap). Filtering by type instead of priority
// keeps this key resolver correct regardless of how any job type's priority is tuned in the future.
export function installationConcurrencyKeyForJob(message: JobMessage): GitHubRateLimitAdmissionKey | null {
if (message.type === "agent-regate-pr") return null;
return isGitHubBudgetBackgroundJob(message) ? githubRateLimitAdmissionKeyForJob(message) : null;
}

Expand Down
16 changes: 6 additions & 10 deletions src/selfhost/sqlite-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -911,16 +911,12 @@ export function createSqliteQueue(
}
// Per-installation GitHub-fetch concurrency admission (#selfhost-installation-concurrency), the last-mile
// gate: only reached by a job that already passed rate-limit admission and (if applicable) maintenance-
// lane admission above, immediately before it actually claims a dispatch slot. Explicitly excludes
// foreground-priority jobs (mirrors the maintenance-admission guard above) -- isGitHubBudgetBackgroundJob
// (which installationConcurrencyKeyForJob is built on) answers "does this job draw GitHub rate-limit
// BUDGET", which is also true for a live (non-sweep, non-manual) agent-regate-pr job; that job is still
// FOREGROUND priority and must never be deferred by this policy, so the exclusion is a separate, explicit
// check here rather than folded into the key resolver itself. installationConcurrencyKey is null for
// background jobs whose payload carries no resolvable installationId too -- those fall through unaffected.
const installationConcurrencyKey = isForegroundJobPriority(job.priority)
? null
: installationConcurrencyKeyForJob(message);
// lane admission above, immediately before it actually claims a dispatch slot. installationConcurrencyKeyForJob
// already excludes the truly-foreground agent-regate-pr job type by construction (not by priority -- see its
// own doc comment for why agent-regate-sweep's priority-8/floor-8 collision rules out a priority-based
// guard here). installationConcurrencyKey is null for background jobs whose payload carries no resolvable
// installationId too -- those fall through unaffected.
const installationConcurrencyKey = installationConcurrencyKeyForJob(message);
if (installationConcurrencyKey) {
const decision = evaluateInstallationConcurrencyAdmission(
installationConcurrencyConfig,
Expand Down
16 changes: 9 additions & 7 deletions test/unit/selfhost-installation-concurrency-admission.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,13 +205,15 @@ describe("installationConcurrencyKeyForJob", () => {
expect(installationConcurrencyKeyForJob(foregroundWebhook)).toBeNull();
});

// REGRESSION (caught while writing this test): isGitHubBudgetBackgroundJob is true for a live (non-sweep,
// non-manual) agent-regate-pr job too -- it DOES draw GitHub rate-limit budget under this key -- so this
// pure resolver correctly returns a non-null key here. The "foreground jobs are never gated by this policy"
// guarantee lives at the pg-queue.ts/sqlite-queue.ts call site (an explicit !isForegroundJobPriority(...)
// guard before this function is ever called), NOT inside this key resolver -- see the queue backend tests.
it("returns the admission key for a foreground agent-regate-pr job (it DOES draw budget under this key -- foreground exclusion happens at the call site, not here)", () => {
expect(installationConcurrencyKeyForJob(foregroundRegate)).toBe("installation:42");
// #selfhost-installation-concurrency-sweep-gap: isGitHubBudgetBackgroundJob is true for a live (non-sweep,
// non-manual) agent-regate-pr job too -- it DOES draw GitHub rate-limit budget under this key -- but that job
// is still FOREGROUND priority (9) and must never be deferred by this policy. The exclusion is applied here,
// BY TYPE, rather than by priority at the queue-backend call site: agent-regate-sweep's own row priority (8)
// collides with FOREGROUND_QUEUE_PRIORITY_FLOOR (also 8), so a priority-based guard at the call site would
// have silently exempted sweep fan-out too -- exactly the background job this policy exists to bound.
it("returns null for a foreground agent-regate-pr job (excluded by type, not by priority), but a key for agent-regate-sweep despite the same priority-8 floor collision", () => {
expect(installationConcurrencyKeyForJob(foregroundRegate)).toBeNull();
expect(installationConcurrencyKeyForJob(scheduledSweep)).toBe("installation:42");
});

it("returns the admission key for a GITHUB_BUDGET_BACKGROUND_TYPES job carrying installationId", () => {
Expand Down
63 changes: 58 additions & 5 deletions test/unit/selfhost-pg-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3142,11 +3142,10 @@ describe("createPgQueue (durable #977)", () => {
else process.env.GITHUB_INSTALLATION_CONCURRENCY_LIMIT = oldLimit;
});

// backfill-repo-segment (not agent-regate-sweep) is used as the background fixture throughout: unlike every
// other GITHUB_BUDGET_BACKGROUND_TYPES member, agent-regate-sweep's OWN row priority (8, PRIORITY_BY_TYPE)
// equals FOREGROUND_QUEUE_PRIORITY_FLOOR, so it is ALSO foreground-priority and therefore already exempt from
// this policy via the isForegroundJobPriority guard -- a genuinely background-priority (0) type is needed to
// actually exercise the limiter.
// backfill-repo-segment is used as the background fixture throughout these general-behavior cases;
// agent-regate-sweep gets its own dedicated regression test below (#selfhost-installation-concurrency-sweep-gap)
// because its row priority (8, PRIORITY_BY_TYPE) equals FOREGROUND_QUEUE_PRIORITY_FLOOR (also 8) -- a priority-
// based exclusion guard would have silently exempted it, which is exactly the gap that test guards against.

it("a second concurrent background job for the SAME installation is deferred at the limit", async () => {
process.env.GITHUB_INSTALLATION_CONCURRENCY_LIMIT = "1";
Expand Down Expand Up @@ -3189,6 +3188,60 @@ describe("createPgQueue (durable #977)", () => {
}
});

// Regression (#selfhost-installation-concurrency-sweep-gap): agent-regate-sweep's own row priority (8,
// PRIORITY_BY_TYPE) equals FOREGROUND_QUEUE_PRIORITY_FLOOR (also 8), so a priority-based exclusion guard
// (`isForegroundJobPriority(job.priority) ? null : ...`) would classify it as foreground and silently exempt
// it from this policy entirely -- exactly the background sweep/backfill fan-out this limiter exists to bound.
// installationConcurrencyKeyForJob must exclude ONLY agent-regate-pr by type, not by priority, so sweep jobs
// are still admission-checked like every other GITHUB_BUDGET_BACKGROUND_TYPES member.
it("a second concurrent agent-regate-sweep job for the SAME installation is deferred at the limit", async () => {
process.env.GITHUB_INSTALLATION_CONCURRENCY_LIMIT = "1";
const m = makePool();
// enqueueJob's fabricated row carries no priority column, which defaults to falsy/NaN under
// isForegroundJobPriority -- indistinguishable from a real background job either way, so it can't prove
// this regression. Use the REAL sweep priority (8, PRIORITY_BY_TYPE) via enqueueResult directly: under the
// old priority-based exclusion this collides with FOREGROUND_QUEUE_PRIORITY_FLOOR (also 8) and would wrongly
// exempt the job, so only the type-based fix in installationConcurrencyKeyForJob makes this test pass.
m.enqueueResult({
rows: [{ id: "1", payload: JSON.stringify({ type: "agent-regate-sweep", installationId: 42 }), attempts: 0, job_key: "sweep:42:a", priority: 8 }],
rowCount: 1,
});
m.enqueueResult({
rows: [{ id: "2", payload: JSON.stringify({ type: "agent-regate-sweep", installationId: 42 }), attempts: 0, job_key: null, priority: 8 }],
rowCount: 1,
});
let release!: () => void;
const gate = new Promise<void>((resolve) => {
release = resolve;
});
let started = 0;
const q = createPgQueue(
m.pool,
async () => {
started++;
await gate;
},
{ concurrency: 2, backgroundConcurrency: 2, pollIntervalMs: 100_000 },
);
await q.init();
try {
q.start();
for (let i = 0; i < 20 && started < 1; i += 1) await new Promise((r) => setTimeout(r, 10));
await new Promise((r) => setTimeout(r, 30));
expect(started).toBe(1);
expect(m.pool.query).toHaveBeenCalledWith(
expect.stringContaining("SET status='pending', run_after=GREATEST"),
expect.arrayContaining([expect.stringContaining("installation concurrency admission deferred: concurrency_high")]),
);
expect(await renderMetrics()).toContain(
'gittensory_jobs_installation_concurrency_deferred_by_reason_total{job_type="agent-regate-sweep",reason="concurrency_high"} 1',
);
} finally {
release();
await q.stop();
}
});

it("a background job for a DIFFERENT installation is admitted concurrently with one already at its own limit", async () => {
process.env.GITHUB_INSTALLATION_CONCURRENCY_LIMIT = "1";
const m = makePool();
Expand Down
55 changes: 49 additions & 6 deletions test/unit/selfhost-sqlite-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3550,12 +3550,12 @@ describe("createSqliteQueue (durable #980)", () => {
else process.env.GITHUB_INSTALLATION_CONCURRENCY_LIMIT = oldLimit;
});

// backfill-repo-segment (not agent-regate-sweep) is used as the background fixture throughout: unlike every
// other GITHUB_BUDGET_BACKGROUND_TYPES member, agent-regate-sweep's OWN row priority (8, PRIORITY_BY_TYPE)
// equals FOREGROUND_QUEUE_PRIORITY_FLOOR, so it is ALSO foreground-priority and therefore already exempt from
// this policy via the isForegroundJobPriority guard -- a genuinely background-priority (0) type is needed to
// actually exercise the limiter. q.binding.send(...) computes real priority via jobPriority(), so (unlike a
// hand-built mock row) every job here carries an authentic priority value.
// backfill-repo-segment is used as the background fixture throughout these general-behavior cases;
// agent-regate-sweep gets its own dedicated regression test below (#selfhost-installation-concurrency-sweep-gap)
// because its row priority (8, PRIORITY_BY_TYPE) equals FOREGROUND_QUEUE_PRIORITY_FLOOR (also 8) -- a priority-
// based exclusion guard would have silently exempted it, which is exactly the gap that test guards against.
// q.binding.send(...) computes real priority via jobPriority(), so (unlike a hand-built mock row) every job
// here carries an authentic priority value.

it("a second concurrent background job for the SAME installation is deferred at the limit", async () => {
process.env.GITHUB_INSTALLATION_CONCURRENCY_LIMIT = "1";
Expand Down Expand Up @@ -3601,6 +3601,49 @@ describe("createSqliteQueue (durable #980)", () => {
}
});

// Regression (#selfhost-installation-concurrency-sweep-gap): agent-regate-sweep's own row priority (8,
// PRIORITY_BY_TYPE) equals FOREGROUND_QUEUE_PRIORITY_FLOOR (also 8), so a priority-based exclusion guard
// (`isForegroundJobPriority(job.priority) ? null : ...`) would classify it as foreground and silently exempt
// it from this policy entirely -- exactly the background sweep/backfill fan-out this limiter exists to bound.
// installationConcurrencyKeyForJob must exclude ONLY agent-regate-pr by type, not by priority, so sweep jobs
// are still admission-checked like every other GITHUB_BUDGET_BACKGROUND_TYPES member.
it("a second concurrent agent-regate-sweep job for the SAME installation is deferred at the limit", async () => {
process.env.GITHUB_INSTALLATION_CONCURRENCY_LIMIT = "1";
const driver = makeDriver();
let release!: () => void;
const gate = new Promise<void>((resolve) => {
release = resolve;
});
let started = 0;
const q = createSqliteQueue(
driver,
async () => {
started++;
await gate;
},
{ concurrency: 2, backgroundConcurrency: 2, pollIntervalMs: 100_000 },
);
await q.binding.send({ type: "agent-regate-sweep", installationId: 42, repoFullName: "owner/a" } as unknown as JobMessage);
await q.binding.send({ type: "agent-regate-sweep", installationId: 42, repoFullName: "owner/b" } as unknown as JobMessage);
try {
q.start();
for (let i = 0; i < 20 && started < 1; i += 1) await new Promise((r) => setTimeout(r, 10));
await new Promise((r) => setTimeout(r, 30));
expect(started).toBe(1);
const row = driver.query(
"SELECT last_error FROM _selfhost_jobs WHERE status='pending' AND payload LIKE '%agent-regate-sweep%'",
[],
).rows[0] as { last_error: string } | undefined;
expect(row?.last_error).toContain("installation concurrency admission deferred: concurrency_high");
expect(await renderMetrics()).toContain(
'gittensory_jobs_installation_concurrency_deferred_by_reason_total{job_type="agent-regate-sweep",reason="concurrency_high"} 1',
);
} finally {
release();
await q.stop();
}
});

it("a background job for a DIFFERENT installation is admitted concurrently with one already at its own limit", async () => {
process.env.GITHUB_INSTALLATION_CONCURRENCY_LIMIT = "1";
const driver = makeDriver();
Expand Down
Loading