Skip to content

fix(github): scope shouldWaitForGitHubRateLimit to the caller's admission key instead of the globally-newest observation #3809

Description

@JSONbored

Context

Found and confirmed live during the 2026-07-06 incident audit (parent: #1667). shouldWaitForGitHubRateLimit (src/github/rate-limit.ts) calls listLatestGitHubRateLimitObservations(env, 10) and picks the single most-recently-observed row where resource === "rest", with no filter on admission key at all — even though github_rate_limit_observations rows already carry an admissionKey column captured at write time, distinguishing per-installation buckets from the shared GITHUB_PUBLIC_TOKEN bucket (used for unrelated ecosystem/registry discovery).

This function gates the scheduled sweep fan-out, the per-repo sweep's rate-limit check, and the top-level cron's decision to even enqueue the sweep trigger job at all. Confirmed today: a heavily-used shared public-token bucket running low (down to ~53-59 remaining, used for registry-wide discovery unrelated to review capability) got picked up as "the" rate-limit reading and incorrectly blocked the scheduled sweep for JSONbored/gittensory's completely separate, healthy installation token (independently confirmed at ~4900/5000 remaining) — continuing to affect sweep resumption even after the wasteful-refresh root cause (#3797, already merged) was fixed.

Notably, the codebase already does this correctly elsewhere: the metrics/dashboard layer's githubRestRateLimitRemainingSamples() (src/github/client.ts) already groups by key_scope and feeds a "by scope" Grafana panel, and the self-hosted Postgres queue's own admission path (githubRateLimitAdmissionTargetForJob/matchesGitHubRateLimitAdmissionTarget, src/selfhost/queue-common.ts) already does proper per-installation-key scoping with a well-reasoned fallback policy. shouldWaitForGitHubRateLimit simply never adopted the same discipline.

Requirements

  • Change shouldWaitForGitHubRateLimit's signature to accept an optional admission-key scope, filtering listLatestGitHubRateLimitObservations to that scope before finding the newest "rest" observation, falling back to the existing fleet-wide behavior only when no scoped observation exists (mirroring matchesGitHubRateLimitAdmissionTarget's existing fallback policy).
  • Update call sites with a concrete installation in scope (the regate sweep, per-repo backfill calls) to pass that installation's admission key.
  • Leave genuinely fleet-wide call sites (the queue consumer's blanket admission check with no specific installation context yet) using the existing unscoped behavior as an explicit, intentional fallback — not an accidental default.
  • Add regression tests: a low-remaining public-token observation must NOT defer a job scoped to a healthy installation key, and vice versa; with no per-key observation at all, behavior falls back to today's global behavior (no regression).

Deliverables

  • Updated shouldWaitForGitHubRateLimit in src/github/rate-limit.ts accepting an admission-key scope.
  • Updated call sites in src/queue/processors.ts and src/selfhost/maintenance-admission.ts.
  • Unit tests covering both cross-bucket false-positive and false-negative scenarios.

Expected outcome

A heavily-used shared public/ecosystem token running low can no longer incorrectly stall maintenance work for an unrelated, healthy installation — closing the second of two compounding bugs found in today's incident (the first, wasteful PR-refresh for uninstalled repos, already shipped in #3797).

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions