Skip to content

fix(discovery-index): treat body-signalled secondary rate limits as retryable - #10249

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-10030
Jul 31, 2026
Merged

fix(discovery-index): treat body-signalled secondary rate limits as retryable#10249
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-10030

Conversation

@bitfathers94

Copy link
Copy Markdown
Contributor

fix(discovery-index): treat body-signalled secondary rate limits as retryable

isRateLimitStatus only inspected status/retry-after/x-ratelimit-remaining, so
a GitHub secondary rate limit (403 with body-only signalling, no retry-after,
remaining still non-zero) fell through as a permanent failure and silently
truncated the fan-out's candidate set. Port the body-regex check from
src/github/client.ts's isRateLimitedResponse, reading via clone().text() so
callers can still read the response body on the retried-and-succeeded path.

Closes #10030

…etryable

isRateLimitStatus only inspected status/retry-after/x-ratelimit-remaining, so
a GitHub secondary rate limit (403 with body-only signalling, no retry-after,
remaining still non-zero) fell through as a permanent failure and silently
truncated the fan-out's candidate set. Port the body-regex check from
src/github/client.ts's isRateLimitedResponse, reading via clone().text() so
callers can still read the response body on the retried-and-succeeded path.
@bitfathers94
bitfathers94 requested a review from JSONbored as a code owner July 31, 2026 14:19
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-31 14:25:59 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The PR converts isRateLimitStatus/isRetryableStatus to async so a cloned response body can be regex-checked for GitHub's secondary-rate-limit/abuse-detection phrasing, mirroring the existing check in src/github/client.ts, and updates the single call site to await it. The change is correctly threaded through fetchWithRetry (the only caller), uses response.clone() so the body remains readable by downstream json() calls, and is covered by new tests for both wording variants, an unreadable-clone fallback, and a regression test for the truncated-fan-out scenario described in the issue.

Nits — 5 non-blocking
  • packages/discovery-index/src/github-client.ts:76 — the bare regex `/secondary rate limit|\babuse\b|api rate limit exceeded/i` is inlined; consider extracting it as a named constant (as the brief notes) so the two GitHub client implementations (src/github/client.ts and this one) can be kept in sync more easily, and to self-document why these three phrases are checked.
  • test/unit/discovery-index/github-client.test.ts — the four new tests are a good, non-fabricated set (they exercise the real fetchWithRetry path), but there's no test for a 403 with a body that matches none of the three phrases and no rate-limit headers, i.e. the plain-403 fallthrough combined with a readable-but-irrelevant body; worth adding for full branch coverage of the new regex line.
  • packages/discovery-index/src/github-client.ts:64-70 — the try/catch around `response.clone().text()` is marked `v8 ignore` as unreachable in practice, which is a reasonable call given clone() on a real fetch Response should not fail, but it's still worth a one-line comment on *why* the test at line 250 uses a hand-rolled fake Response rather than a real one (since real Response objects can't easily be made to reject `.text()`).
  • Extract the regex in github-client.ts:64 into a shared constant or module (ideally reused from src/github/client.ts's isRateLimitedResponse) to avoid the two implementations drifting apart if GitHub adds new wording.
  • Add a test case for a 403 whose body is readable but doesn't match any of the three phrases, to lock in the non-retry fallthrough now that the function has a new async branch.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #10030
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 ⚠️ 20/25 Preflight is ready, but the PR body does not name the validation run.
Contributor workload ✅ 10/10 Author activity: 140 registered-repo PR(s), 99 merged, 3 issue(s).
Contributor context ✅ Confirmed Gittensor contributor bitfathers94; Gittensor profile; 140 PR(s), 3 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff makes isRateLimitStatus/isRetryableStatus async and adds the same clone().text() regex body check as src/github/client.ts, with try/catch degrading to false, and fetchWithRetry awaits the async check while leaving recordRateLimit/backoff/metrics untouched; tests cover the required secondary-limit retry, unreadable-clone fallback, and non-truncation regression scenarios.

Review context
  • Author: bitfathers94
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript
  • Official Gittensor activity: 140 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add validation command/output.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

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

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

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.61%. Comparing base (78891b8) to head (d6e5d8a).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #10249    +/-   ##
========================================
  Coverage   80.60%   80.61%            
========================================
  Files         283      284     +1     
  Lines       59101    59210   +109     
  Branches     7000     7033    +33     
========================================
+ Hits        47641    47730    +89     
- Misses      11167    11188    +21     
+ Partials      293      292     -1     
Flag Coverage Δ
backend 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/discovery-index/src/github-client.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@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.

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

@loopover-orb
loopover-orb Bot merged commit 9a84fcd into JSONbored:main Jul 31, 2026
8 checks passed
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.

discovery-index(github-client): treat a body-signalled secondary rate limit as retryable, matching src/github/client.ts

1 participant