From 7d6e723b5bca9cef2be1ef65d3ae7d906919b310 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:28:04 -0700 Subject: [PATCH] fix(review): add missing GitHub-budget rate-limit admission for 5 maintenance job types (#4505) reconcile-open-prs was missing from GITHUB_BUDGET_BACKGROUND_TYPES and had no jobCoalesceKey case, so once GITTENSORY_PR_RECONCILIATION is enabled its real paginated per-repo GitHub REST calls would never yield to an exhausted shared budget, and every 10-min tick would insert a duplicate row instead of coalescing into one still-pending scan. Fixing this one prompted a systematic audit of every MAINTENANCE_JOB_TYPES member against GITHUB_BUDGET_BACKGROUND_TYPES (the module's own header comment claims universal coverage), which found four more of the same gap: - refresh-installation-health: getAppInstallation (a direct, unprotected `GET /app/installations/{id}` REST call) per installation, plus resolveRepositorySettings per installed repo. Runs every 30 min, UNCONDITIONALLY -- the most severe of the five, since it is exercised in every deployment today, not just after an operator opts into a flag. - backlog-convergence-sweep: resolveRepositorySettings per repo in both the fan-out and per-repo handler. Runs every 30 min, unconditional for self-hosted runtimes. - selftune: resolveRepositorySettings per registered repo. Hourly, flag-gated. - generate-review-recap: loadRepoFocusManifest directly. Not yet cron-enqueued (manual/API trigger only today), and had no jobCoalesceKey case at all -- every trigger inserted a fresh duplicate row. All five now yield to shouldWaitForGitHubRateLimit at dequeue time and coalesce a repeated enqueue into an already-pending/processing row. The maintenance-admission.ts header comment is corrected to describe which job types get GitHub-budget admission (the subset making real GitHub calls) vs. only local-load admission (the purely-internal sweeps), rather than claiming universal coverage. --- src/selfhost/maintenance-admission.ts | 17 ++++++---- src/selfhost/queue-common.ts | 28 +++++++++++++++ test/unit/index.test.ts | 37 ++++++++++++++++++++ test/unit/selfhost-queue-common.test.ts | 45 ++++++++++++++++++++++++- 4 files changed, 119 insertions(+), 8 deletions(-) diff --git a/src/selfhost/maintenance-admission.ts b/src/selfhost/maintenance-admission.ts index 90383bd0d5..3d3bfefeda 100644 --- a/src/selfhost/maintenance-admission.ts +++ b/src/selfhost/maintenance-admission.ts @@ -3,13 +3,16 @@ // FOREGROUND_QUEUE_PRIORITY_FLOOR, see queue-common.ts) -- must always win a resource race against periodic // maintenance sweeps (contributor evidence, burden forecasts, RAG re-indexing, drift scans, product rollups, // notifications...). Those sweeps already run on a conservative cadence (every 30min/hourly/6-hourly, see -// index.ts's enqueueScheduledJobs) and already yield to an EXHAUSTED GitHub REST budget -// (shouldWaitForGitHubRateLimit) -- this module adds an ORTHOGONAL signal: is the box itself under load RIGHT -// NOW (a live-work backlog, an aging live job, a hot host CPU), independent of whether GitHub's API happens to -// be rate-limited. The queue backends (sqlite-queue.ts / pg-queue.ts) consult this at CLAIM time, the same way -// they already consult GitHub rate-limit admission: a denied maintenance job is pushed back to 'pending' with -// a jittered future run_after -- its original enqueue time is left untouched, so the age-based trickle below -// still works -- never dropped and never run early. +// index.ts's enqueueScheduledJobs); the subset that makes real GitHub REST calls ALSO yields to an EXHAUSTED +// GitHub REST budget (shouldWaitForGitHubRateLimit) via isGitHubBudgetBackgroundJob / GITHUB_BUDGET_BACKGROUND_TYPES +// (queue-common.ts) -- purely-internal sweeps that touch no GitHub API (product-usage rollups, retention +// pruning, notification delivery, and similar) have no such budget to yield to and correctly aren't in that set. +// This module adds an ORTHOGONAL signal on top of whichever of those a job type already has: is the box itself +// under load RIGHT NOW (a live-work backlog, an aging live job, a hot host CPU), independent of whether GitHub's +// API happens to be rate-limited. The queue backends (sqlite-queue.ts / pg-queue.ts) consult this at CLAIM time, +// the same way they already consult GitHub rate-limit admission where applicable: a denied maintenance job is +// pushed back to 'pending' with a jittered future run_after -- its original enqueue time is left untouched, so +// the age-based trickle below still works -- never dropped and never run early. // // TRICKLE: a maintenance job that has been pending since `maxDeferAgeMs` is force-admitted regardless of // current pressure, so a box under SUSTAINED load can never starve maintenance work forever -- it just runs at diff --git a/src/selfhost/queue-common.ts b/src/selfhost/queue-common.ts index d8fa4d95c2..40b7fcb200 100644 --- a/src/selfhost/queue-common.ts +++ b/src/selfhost/queue-common.ts @@ -95,6 +95,29 @@ const GITHUB_BUDGET_BACKGROUND_TYPES = new Set([ "refresh-contributor-activity", "build-burden-forecasts", "rag-index-repo", + // #4505: found via a systematic audit of every MAINTENANCE_JOB_TYPES member against this set (prompted by + // reconcile-open-prs below) -- each of these five genuinely makes real GitHub REST calls (directly, or + // transitively via resolveRepositorySettings -> loadRepoFocusManifest's cache-miss fetch of .gittensory.json) + // but was missing from this set, contradicting this module's own header comment. + // + // runOpenPrReconciliation makes real, potentially large paginated GitHub REST calls per watched repo (up to + // RECONCILE_OPEN_PRS_MAX_PAGES per repo, plus a catch-up fetch per missing PR found). Flag-gated OFF by + // default today (GITTENSORY_PR_RECONCILIATION) -- this closes the gap before anyone enables it. + "reconcile-open-prs", + // fanOutBacklogConvergenceSweepJobs / sweepRepoBacklogConvergence both call resolveRepositorySettings per + // repo. Runs every 30 min, unconditional for self-hosted runtimes -- active in production today. + "backlog-convergence-sweep", + // selfTuneRepos calls resolveRepositorySettings per registered repo to check acting-autonomy + the per-repo + // opt-out. Hourly, flag-gated OFF by default (GITTENSORY_REVIEW_SELFTUNE). + "selftune", + // refreshInstallationHealthRecords calls getAppInstallation (a direct, unprotected `GET /app/installations/{id}` + // REST call) per installation, PLUS resolveRepositorySettings per installed repo. Runs every 30 min, + // UNCONDITIONAL (not behind any flag) -- the most severe of these five, since it is exercised in every + // deployment today, not just after an operator opts into a flag. + "refresh-installation-health", + // runReviewRecapJob calls loadRepoFocusManifest directly for its one repo. Not yet cron-enqueued (manual/API + // trigger only today, per its own doc comment), but still worth gating against a rapid repeated manual trigger. + "generate-review-recap", ]); const PRIORITY_BY_TYPE = new Map([ ["agent-regate-pr", AGENT_REGATE_PRIORITY], @@ -915,6 +938,7 @@ export function jobCoalesceKey(payload: string): string | null { case "ops-alerts": case "selftune": case "retry-orb-relay": + case "reconcile-open-prs": return type; case "backfill-registered-repos": return keyOf( @@ -941,6 +965,10 @@ export function jobCoalesceKey(payload: string): string | null { ); case "generate-signal-snapshots": case "build-burden-forecasts": + // #4505: no case existed for this single-repo job type at all, so it fell through to the untyped `null` + // below -- every enqueue (repeated manual/API triggers today; a future cron trigger per its own doc + // comment) inserted a fresh duplicate row instead of coalescing into an already-pending/processing one. + case "generate-review-recap": return keyOf(type, normalizedRepo(message.repoFullName) ?? "all"); case "build-contributor-evidence": case "build-contributor-decision-packs": { diff --git a/test/unit/index.test.ts b/test/unit/index.test.ts index 397a845e1f..99fe30bfed 100644 --- a/test/unit/index.test.ts +++ b/test/unit/index.test.ts @@ -234,6 +234,43 @@ describe("worker entrypoint", () => { expect(retries).toEqual([]); }); + it("INVARIANT (#4505): pre-yields a reconcile-open-prs job while the shared GitHub REST budget is exhausted", async () => { + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-06-24T12:00:00.000Z")); + const env = createTestEnv(); + // reconcile-open-prs has no per-installation field, so it draws from the SAME shared (no-admissionKey) + // observation refresh-registry's own equivalent test above uses. + await recordGitHubRateLimitObservation(env, { repoFullName: "owner/repo", resource: "rest", path: "/x", statusCode: 200, limitValue: 5000, remaining: 5, resetAt: "2026-06-24T12:30:00.000Z", observedAt: "2026-06-24T12:00:00.000Z" }); + const acked: string[] = []; + const retries: Array<{ delaySeconds?: number } | undefined> = []; + const requeued: Array<{ message: import("../../src/types").JobMessage; delaySeconds?: number }> = []; + env.JOBS = { + async send(message: import("../../src/types").JobMessage, options?: { delaySeconds?: number }) { + requeued.push({ message, ...(options?.delaySeconds === undefined ? {} : { delaySeconds: options.delaySeconds }) }); + }, + } as unknown as Queue; + const batch = { + messages: [ + { + id: "reconcile-tick", + body: { type: "reconcile-open-prs", requestedBy: "schedule" }, + ack: () => acked.push("reconcile-tick"), + retry: (options?: { delaySeconds?: number }) => retries.push(options), + }, + ], + } as unknown as MessageBatch; + + await worker.queue(batch, env); + + // Pre-yielded, not run: acked (not retried, preserving retry budget) and re-queued after the reset -- + // BEFORE this fix, reconcile-open-prs was missing from GITHUB_BUDGET_BACKGROUND_TYPES, so this exhausted + // budget would have been silently ignored and runOpenPrReconciliation would have run immediately. + expect(acked).toEqual(["reconcile-tick"]); + expect(retries).toEqual([]); + expect(requeued).toEqual([{ message: { type: "reconcile-open-prs", requestedBy: "schedule" }, delaySeconds: 900 }]); // delayUntil clamps to [30, 900] + vi.useRealTimers(); + }); + it("runs scheduled jobs through waitUntil", async () => { const env = createTestEnv(); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { diff --git a/test/unit/selfhost-queue-common.test.ts b/test/unit/selfhost-queue-common.test.ts index 0752ad88ac..1dae96f4df 100644 --- a/test/unit/selfhost-queue-common.test.ts +++ b/test/unit/selfhost-queue-common.test.ts @@ -267,7 +267,36 @@ describe("self-host queue common helpers", () => { expect(isGitHubBudgetBackgroundJob({ type: "agent-regate-sweep", requestedBy: "schedule" })).toBe(true); expect(isGitHubBudgetBackgroundJob({ type: "backfill-repo-segment", requestedBy: "schedule", repoFullName: "owner/repo", segment: "open_pull_requests" })).toBe(true); expect(isGitHubBudgetBackgroundJob({ type: "rag-index-repo", requestedBy: "schedule" })).toBe(true); - expect(isGitHubBudgetBackgroundJob({ type: "refresh-installation-health", requestedBy: "schedule" })).toBe(false); + }); + + it("REGRESSION (#4505): every maintenance job type confirmed to make real GitHub REST calls is GitHub-budget-gated", () => { + // refreshInstallationHealthRecords calls getAppInstallation (a direct REST call) per installation, plus + // resolveRepositorySettings per installed repo -- runs every 30 min, UNCONDITIONALLY, in every deployment. + expect(isGitHubBudgetBackgroundJob({ type: "refresh-installation-health", requestedBy: "schedule" })).toBe(true); + // fanOutBacklogConvergenceSweepJobs / sweepRepoBacklogConvergence call resolveRepositorySettings per repo. + expect(isGitHubBudgetBackgroundJob({ type: "backlog-convergence-sweep", requestedBy: "schedule" })).toBe(true); + // selfTuneRepos calls resolveRepositorySettings per registered repo. + expect(isGitHubBudgetBackgroundJob({ type: "selftune", requestedBy: "schedule" })).toBe(true); + // runReviewRecapJob calls loadRepoFocusManifest directly. + expect(isGitHubBudgetBackgroundJob({ type: "generate-review-recap", requestedBy: "schedule", repoFullName: "owner/repo" })).toBe(true); + // reconcile-open-prs: runOpenPrReconciliation makes large paginated GitHub REST calls per watched repo. + expect(isGitHubBudgetBackgroundJob({ type: "reconcile-open-prs", requestedBy: "schedule" })).toBe(true); + }); + + it("REGRESSION (#4505): maintenance job types confirmed to make NO GitHub REST calls stay OFF the GitHub budget (never wrongly gated)", () => { + // Verified local-only (D1 reads/writes, or dispatching an already-gated job type) during the #4505 audit -- + // asserting these stay false catches a future accidental over-broad addition to GITHUB_BUDGET_BACKGROUND_TYPES, + // which would make a purely-internal job wait on a GitHub rate-limit budget it never draws from. + expect(isGitHubBudgetBackgroundJob({ type: "refresh-registry", requestedBy: "schedule" })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "refresh-scoring-model", requestedBy: "schedule" })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "repair-data-fidelity", requestedBy: "schedule" })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "rollup-product-usage", requestedBy: "schedule" })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "prune-retention", requestedBy: "schedule", dryRun: false })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "generate-weekly-value-report", requestedBy: "schedule" })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "generate-maintainer-recap", requestedBy: "schedule" })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "generate-signal-snapshots", requestedBy: "schedule" })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "ops-alerts", requestedBy: "schedule" })).toBe(false); + expect(isGitHubBudgetBackgroundJob({ type: "sweep-liveness-watchdog", requestedBy: "schedule" })).toBe(false); }); describe("isScheduledRegateSweepJob", () => { @@ -1011,6 +1040,20 @@ describe("self-host queue common helpers", () => { ).toBeNull(); }); + it("REGRESSION (#4505): generate-review-recap coalesces per-repo instead of falling through to null (previously had no case at all)", () => { + expect(jobCoalesceKey(payload({ type: "generate-review-recap", requestedBy: "api", repoFullName: "JSONbored/Gittensory" }))).toBe("generate-review-recap:jsonbored/gittensory"); + // A second trigger for the SAME repo produces the identical key, so pg-queue.ts's pending-only coalesce + // path merges it into the first instead of inserting a duplicate row. + expect(jobCoalesceKey(payload({ type: "generate-review-recap", requestedBy: "api", repoFullName: "jsonbored/gittensory" }))).toBe("generate-review-recap:jsonbored/gittensory"); + // A DIFFERENT repo gets a distinct key -- never coalesced together. + expect(jobCoalesceKey(payload({ type: "generate-review-recap", requestedBy: "api", repoFullName: "owner/other-repo" }))).toBe("generate-review-recap:owner/other-repo"); + }); + + it("REGRESSION (#4505): refresh-installation-health and selftune coalesce to a single global slot (unaffected by the GitHub-budget fix)", () => { + expect(jobCoalesceKey(payload({ type: "refresh-installation-health", requestedBy: "schedule" }))).toBe("refresh-installation-health"); + expect(jobCoalesceKey(payload({ type: "selftune", requestedBy: "schedule" }))).toBe("selftune"); + }); + it("orders per-PR re-gate jobs by GitHub PR creation time, with a deterministic legacy fallback", () => { expect( jobClaimSortKey(