Skip to content

fix(github): scope rate-limit admission checks to the caller's own bucket - #3825

Merged
loopover-orb[bot] merged 1 commit into
mainfrom
fix/ratelimit-admission-key-scoping
Jul 6, 2026
Merged

fix(github): scope rate-limit admission checks to the caller's own bucket#3825
loopover-orb[bot] merged 1 commit into
mainfrom
fix/ratelimit-admission-key-scoping

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • shouldWaitForGitHubRateLimit read the globally-newest REST rate-limit observation across every installation and the shared public/registry token, regardless of which bucket the caller actually draws from. Every installation and the shared public token draw from DIFFERENT GitHub-side REST buckets, so this could (a) let a healthy, recently-observed OTHER bucket mask a genuinely exhausted caller bucket, or (b) let an exhausted, recently-observed OTHER bucket wrongly throttle a caller whose own bucket has plenty of headroom. This was a real contributor to today's incident, where registry/ecosystem discovery traffic (the shared public token) and installed-repo review traffic (the installation token) share this same observation table.
  • Adds an optional admissionKey parameter to listLatestGitHubRateLimitObservations and shouldWaitForGitHubRateLimit, scoping the read to that bucket when given (an index on (admission_key, observed_at) already existed for this — it just wasn't used).
  • Threads the caller's own admission key into every call site where one is naturally available (7 of 8): both queue/processors.ts sweep/regate paths (already had installationId), all three github/backfill.ts segment/hydration paths (one already received admissionKey as an unused parameter — this was a one-line bug there specifically), upstream/commit.ts (already computes the same key for its own fetch one line below), and both message-scoped index.ts queue-consumer checks plus queue/dlq.ts's redrive check (via the existing githubRateLimitAdmissionKeyForJob helper).
  • The one remaining call site (index.ts's pre-dispatch scheduled-sweep-trigger throttle) has no natural single installation in scope at that point in the code (it runs before any specific repo/job is chosen) — left unscoped, preserving its exact prior behavior. This is not a correctness gap: the real per-repo/per-installation admission check still happens downstream at the now-scoped queue/processors.ts call sites once a specific job is dispatched.

Closes #3809.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes. (Touches 7 source files because the fixed function has 8 real call sites across the codebase; fixing a subset would leave some callers still exposed to the exact bug this PR closes.)
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/session/CORS surface touched.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP surface changed.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — backend-only change, no UI touched.)
  • Visible UI changes include a UI Evidence section below. (N/A — no visible UI change.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

Notes

  • Fixes several existing tests that seeded a rate-limit observation without the admission key matching the installation/token the exercised code path actually uses — those now seed the correct scoped key so they keep testing the same throttle/proceed scenario under the new scoped read.

…cket (#3809)

shouldWaitForGitHubRateLimit read the globally-newest REST rate-limit
observation across every installation and the shared public/registry
token, regardless of which bucket the caller actually draws from. Since
each installation and the shared public token have separate GitHub-side
REST buckets, a healthy recent observation from one bucket could mask
another bucket's real exhaustion, or an exhausted recent observation from
one bucket could wrongly throttle a caller whose own bucket has headroom.

Add an optional admissionKey parameter to shouldWaitForGitHubRateLimit and
listLatestGitHubRateLimitObservations, and thread each caller's own
already-computed admission key through at every call site where one is
naturally available. One call site genuinely has no installation in scope
yet (a pre-dispatch scheduler check) and is left unscoped, unchanged from
before -- the real per-installation check still happens downstream once a
specific job is dispatched.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.41%. Comparing base (fd8b3da) to head (61accf7).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3825   +/-   ##
=======================================
  Coverage   93.41%   93.41%           
=======================================
  Files         325      325           
  Lines       32844    32845    +1     
  Branches    12030    12032    +2     
=======================================
+ Hits        30680    30681    +1     
  Misses       1530     1530           
  Partials      634      634           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.54% <100.00%> (ø)
src/github/backfill.ts 96.76% <100.00%> (ø)
src/github/rate-limit.ts 100.00% <100.00%> (ø)
src/index.ts 94.73% <100.00%> (ø)
src/queue/dlq.ts 100.00% <100.00%> (ø)
src/queue/processors.ts 94.44% <100.00%> (+<0.01%) ⬆️
src/upstream/commit.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 6, 2026
@loopover-orb

loopover-orb Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-06 13:31:02 UTC

12 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR fixes a real bucket-conflation bug: shouldWaitForGitHubRateLimit previously read whichever REST observation was globally newest across all installations and the shared public token, letting one bucket's health mask or falsely throttle an unrelated bucket. The fix adds an optional admissionKey to listLatestGitHubRateLimitObservations/shouldWaitForGitHubRateLimit (backed by an existing (admission_key, observed_at) index) and threads the caller's own key through 7 of 8 call sites, leaving the one site with no natural single bucket unscoped (preserving prior behavior there). The new rate-limit.test.ts cases directly reproduce both directions of the original bug (exhausted-other-bucket-throttles-healthy-caller and healthy-other-bucket-masks-exhausted-caller) and assert the fix, plus a fallback test pinning byte-identical old behavior when admissionKey is omitted — this is exactly the kind of regression coverage the described incident calls for.

Nits — 6 non-blocking
  • src/github/backfill.ts's two segment functions call `repoAdmissionKeyForToken(env, repo, token)`, a name distinct from the `githubRateLimitAdmissionKeyForToken` shown in the (unchanged) import block in this diff — confirm `repoAdmissionKeyForToken` is actually defined/imported elsewhere in this 1539-line file, since the diff doesn't show its definition.
  • `githubRateLimitAdmissionKeyForJob` is imported from a queue-common-style module in src/index.ts and separately from `../selfhost/queue-common` in src/queue/dlq.ts — worth confirming these resolve to the same implementation rather than two parallel definitions that could drift.
  • The PR description itself flags the linked issue coverage as only 'partial' per the external brief — worth double-checking the issue is actually closed/fully addressed before merge per this repo's issue-linkage convention.
  • Consider a short code comment or shared helper name so `repoAdmissionKeyForToken` in backfill.ts and `githubRateLimitAdmissionKeyForToken`/`ForPublicToken` in github/client.ts are visibly related, since the near-identical naming across the diff is easy to confuse during future edits.
  • Since src/index.ts's pre-dispatch scheduled-sweep-trigger throttle is intentionally left globally-scoped, a one-line comment there (mirroring the ones added elsewhere) would help a future reader understand why it's the one exception rather than an oversight.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #3809
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 54 registered-repo PR(s), 46 merged, 399 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 54 PR(s), 399 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 54 PR(s), 399 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit ca6a6d7 into main Jul 6, 2026
10 checks passed
@loopover-orb
loopover-orb Bot deleted the fix/ratelimit-admission-key-scoping branch July 6, 2026 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant