Skip to content

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

Merged
JSONbored merged 2 commits into
mainfrom
fix/ratelimit-admission-webhook-backlog
Jul 1, 2026
Merged

fix(queue): prefer exact github rate-limit admission buckets#2417
JSONbored merged 2 commits into
mainfrom
fix/ratelimit-admission-webhook-backlog

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Fixes a self-host bug where a newer unkeyed/null legacy GitHub rate-limit observation could override an older-but-healthy exact installation-keyed observation purely on recency, deferring an entire installation's github-webhook queue even though that installation's own REST budget was fine.
  • Observed on a live VPS: ~147 pending github-webhook jobs stuck with last_error = "github rate-limit webhook admission", while github_rate_limit_observations showed an exhausted unkeyed row racing ahead of a healthy installation:<id> row.
  • fallbackObservationCanOverrideExact now only lets a newer fallback win when doing so is more permissive than the exact reading (clearing a stale exhaustion) — never when it would introduce a delay the exact observation alone would not have. A fallback still governs admission when no exact observation exists at all.
  • matchesGitHubRateLimitAdmissionTarget had the same failure class in the reactive (post-failure) defer path: a confirmed rate-limit error on a job with no admission key unconditionally parked every other pending job regardless of its own key. It now only parks other null-keyed candidates, matching the precedent already used for keyed blocked targets.

Test plan

  • npx vitest run test/unit/selfhost-queue-common.test.ts test/unit/selfhost-pg-queue.test.ts test/unit/selfhost-sqlite-queue.test.ts — all passing, including new regressions for: healthy exact + newer exhausted fallback (no delay), no exact + exhausted fallback (delay), exact-alone exhausted (delay), a newer fallback still clearing a stale exact exhaustion (invariant), background-kind parity, and the matchesGitHubRateLimitAdmissionTarget null-blocked-target scoping fix
  • npm run test:coverage — full suite green, 100% patch coverage on changed src/** lines/branches
  • npm run typecheck — clean
  • git diff --check — clean
  • npm run test:ci + npm audit --audit-level=moderate — clean

@dosubot dosubot Bot added the size:S label Jul 1, 2026
@loopover-orb

loopover-orb Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-01 20:49:44 UTC

4 files · 1 AI reviewer · no blockers · readiness 86/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

Review summary
The queue helper now makes exact GitHub rate-limit observations authoritative once present, so an unkeyed/null fallback can neither suppress a healthy keyed observation nor clear a keyed exhaustion; the reactive target matcher also stops a null-key failure from parking concretely keyed work. The visible implementation matches the new regression expectations in the unit, SQLite, and Postgres tests, and I do not see a reachable break in the changed code. The notable review point is that the PR description still claims a newer fallback may clear a stale exact exhaustion, while the diff intentionally removes that behavior.

Nits — 5 non-blocking
  • nit: The PR description/test plan contradicts `src/selfhost/queue-common.ts:286` and the updated tests by saying a newer fallback still clears a stale exact exhaustion; update the PR text so future maintainers do not preserve the wrong invariant.
  • nit: The expanded comments in `src/selfhost/queue-common.ts:276` and the regression tests are much longer than the branch logic they explain; keep one production comment with the invariant and move incident-level detail to the test name or PR discussion.
  • nit: `test/unit/selfhost-pg-queue.test.ts:750` uses a broad negative `toHaveBeenCalledWith(..., expect.anything())`, which is weaker than asserting the job ran and no rate-limit deferral metric/query was emitted through the queue-facing API.
  • Update the PR summary to say fallback governs only when no exact observation exists, matching `fallbackObservationCanOverrideExact` and the new invariant tests.
  • In `src/selfhost/queue-common.ts`, consider reducing the fallback/exact comment to the trust boundary: unkeyed observations are not proven to share the keyed bucket, so exact observations win in both directions.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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 (size label size:S; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 67 registered-repo PR(s), 57 merged, 589 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 67 PR(s), 589 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 registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 67 PR(s), 589 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Triage stale or unlinked PRs.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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 added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 1, 2026
@JSONbored JSONbored self-assigned this Jul 1, 2026
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.83%. Comparing base (f98f555) to head (4456074).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2417      +/-   ##
==========================================
- Coverage   95.83%   95.83%   -0.01%     
==========================================
  Files         224      224              
  Lines       25006    25001       -5     
  Branches     9096     9092       -4     
==========================================
- Hits        23964    23959       -5     
  Misses        428      428              
  Partials      614      614              
Files with missing lines Coverage Δ
src/selfhost/queue-common.ts 92.73% <100.00%> (-0.13%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JSONbored added 2 commits July 1, 2026 13:39
A newer unkeyed/null legacy rate-limit observation could override an
older-but-healthy exact installation-keyed observation purely on recency,
deferring an entire installation's webhook queue even though that
installation's own budget was fine. This happened repeatedly on self-host:
~147 pending github-webhook jobs piled up with last_error 'github
rate-limit webhook admission' while the unkeyed fallback bucket (a
different, unrelated consumer) cycled through exhaustion and reset faster
than the installation-scoped exact observation was refreshed.

fallbackObservationCanOverrideExact now only lets a newer fallback win when
doing so is MORE permissive than the exact reading (clearing a stale
exhaustion) — never when it would introduce a delay the exact observation
alone would not have. A fallback still governs when no exact observation
exists at all.

matchesGitHubRateLimitAdmissionTarget had the same failure class in the
reactive (post-failure) defer path: a confirmed rate-limit error on a job
with no admission key (legacy/unknown actor work) unconditionally parked
every OTHER pending job regardless of its own key. It now only parks other
null-keyed candidates, matching the precedent already used for keyed
blocked targets.
…austion too

Review feedback on the initial fix correctly identified that the new rule
was asymmetric without justification: a null/unkeyed observation is not
proven to report on the SAME budget as an exact installation key, so its
signal is equally untrustworthy in BOTH directions, not just when it would
introduce a new restriction. Simplify to: once an exact observation exists
for an admission key, it alone governs (its own reset_at already bounds how
long an exhaustion can block admission); the fallback only ever applies
when no exact observation exists at all.
@JSONbored
JSONbored force-pushed the fix/ratelimit-admission-webhook-backlog branch from 89d09c1 to 4456074 Compare July 1, 2026 20:42
@JSONbored
JSONbored merged commit 28b345a into main Jul 1, 2026
9 checks passed
@JSONbored
JSONbored deleted the fix/ratelimit-admission-webhook-backlog branch July 1, 2026 20:52
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 1, 2026
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

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant