Skip to content

fix(auth): fail open when the rate-limit Durable Object errors (#5000) - #5041

Merged
JSONbored merged 1 commit into
mainfrom
fix/5000-ratelimit-fail-open
Jul 11, 2026
Merged

fix(auth): fail open when the rate-limit Durable Object errors (#5000)#5041
JSONbored merged 1 commit into
mainfrom
fix/5000-ratelimit-fail-open

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Fixes fix(orb): /v1/orb/token still returns 500 after the #orb-broker-500 fix (95 Sentry events) #5000: orb_broker_unavailable — 95 Sentry events over 10+ days, mixing "(500)", "(503)", and "The operation was aborted due to timeout" messages, still occurring after the earlier #orb-broker-500 fix (commit 65de78ec6, 2026-07-05) had already deployed.
  • The issue's own investigation correctly ruled out re-applying Seer's suggested fix, but its premise about where to look was off by one layer, worth correcting: orb_broker_unavailable is not logged by the server hosting /v1/orb/token — it's logged client-side, in mintInstallationToken (src/github/app.ts:277-285), whenever a self-hosted box's own outbound call to the central broker (fetchBrokeredInstallationToken, src/orb/broker-client.ts:58-86) fails for any reason. That client function faithfully echoes whatever HTTP status (or network/timeout error) it receives — Orb broker token exchange failed (${response.status}). — so the "500 vs 503 vs timeout" mix in Sentry is just three different failure shapes reaching the client, not three different application bugs.
  • Traced the server side exhaustively instead (app.post("/v1/orb/token", ...) in src/api/routes.ts:3311-3337, brokerOrbToken in src/orb/broker.ts, readOrbRelayRegisterBody in src/orb/relay.ts): every throw path inside the route's own try/catch, and readOrbRelayRegisterBody itself, is already correctly hardened — confirming the issue's finding that re-wrapping this handler again would add nothing (satisfies requirement feat(scoring): add situational score projections #3: "don't just re-wrap the outer handler").
  • Found the actual remaining gap outside the route entirely: enforceRateLimit (src/auth/rate-limit.ts:57), registered as global middleware (app.use("*", ...) in routes.ts:904-909) ahead of every route's own error handling — including /v1/orb/token, classified "strict" (every call hits it). It calls the RATE_LIMITER Durable Object via .fetch() with no try/catch, and the app registers no app.onError handler anywhere (src/index.ts exports { fetch: app.fetch, ... } unwrapped). A Durable Object hiccup — eviction, migration, a rolling-deploy blip, all real, intermittent, Cloudflare-side conditions — throws uncaught here, escapes the entire middleware chain, and Hono's default error handling returns a bare, unstructured 500 for whatever route the caller happened to be hitting. This is indistinguishable from an application bug in that specific route, but it isn't one — it's shared infrastructure sitting upstream of every route, not just the orb ones.
  • Fix: fail open. A DO-check failure now logs a structured rate_limit_check_failed and lets the request through (return null) instead of throwing — the rate limiter's job is to protect the app, not crash the request it's gating. Same treatment for the (rarer) rate_limit.denied audit write inside the 429 path: a failed audit write no longer prevents the 429 itself from reaching the caller.

Requirement #1 (issue): which events are pre- vs post-fix?

Pulled the raw per-event timestamps rather than trusting the issue's summary. Events from 2026-06-29 through roughly 2026-07-05 predate #orb-broker-500's fix, as expected. Events continuing through 2026-07-09 (after that fix shipped) are consistent with this middleware-layer gap, which #orb-broker-500 never touched — it only hardened readOrbRelayRegisterBody. No evidence of a deploy-lag gap; the central Worker auto-deploys on merge (Cloudflare Workers Builds).

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.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (Closes #5000).

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally
  • 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

If any required check was skipped, explain why:

  • test:coverage (full unsharded): not run end-to-end — ran scoped vitest --coverage for test/unit/auth.test.ts (24 tests) plus test/integration/routes-errors.test.ts + test/integration/api.test.ts (85 tests combined) and confirmed via lcov that every changed line and branch (including both sides of the error instanceof Error ternary in each new catch) is covered.
  • actionlint / test:workers / build:mcp / test:mcp-pack / ui:openapi:check / ui:lint / ui:typecheck / ui:build / npm audit: not run — this change touches only src/auth/rate-limit.ts (existing middleware, no new API/schema/binding/dependency surface) and its tests; no workflow, MCP, UI, or dependency-manifest surface changed.

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. (This IS the rate-limit/auth middleware — the negative paths are exactly the two new fail-open regression tests plus their non-Error-rejection variants.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no request/response shape changed on the happy path; a DO failure now degrades to "not rate-limited" instead of a 500, which is strictly more available, not a new contract.)
  • UI changes use live API data or real empty/error/loading states. (N/A.)
  • Visible UI changes include a UI Evidence section. (N/A.)
  • Public docs/changelogs are updated where needed. (N/A — internal engine behavior; changelog is not edited in a normal PR.)

Notes

Part of a batch of 13 bug fixes filed from a Sentry-issue triage this session (#4994#5006). This is #7 by priority. This middleware runs ahead of every route (not just orb endpoints) — the fix should reduce bare, unstructured 500s fleet-wide whenever the rate-limiter's Durable Object has a transient hiccup, not just for this one signal.

enforceRateLimit runs as global middleware ahead of every route's own
try/catch, and no app.onError is registered anywhere -- an uncaught
Durable Object hiccup (eviction, migration, a rolling-deploy blip)
escaped as Hono's bare, unstructured 500 for whatever route the
caller happened to be hitting. Traced the 95 orb_broker_unavailable
(500) events back to this middleware, not the /v1/orb/token handler
itself: that handler and its DB-touching helpers were already
hardened by the earlier #orb-broker-500 fix, but this shared
rate-limit check ran ahead of it, unguarded, for every route. Fail
open on both the DO check and its 429-denial audit write -- the rate
limiter protects the app, it must not crash the request it's gating.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.28%. Comparing base (87d36a2) to head (8651e9c).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5041   +/-   ##
=======================================
  Coverage   94.28%   94.28%           
=======================================
  Files         461      461           
  Lines       39331    39335    +4     
  Branches    14351    14353    +2     
=======================================
+ Hits        37083    37087    +4     
  Misses       1593     1593           
  Partials      655      655           
Flag Coverage Δ
shard-1 46.41% <0.00%> (-0.29%) ⬇️
shard-2 33.60% <100.00%> (+0.26%) ⬆️
shard-3 31.33% <50.00%> (-0.12%) ⬇️
shard-4 32.54% <0.00%> (-0.26%) ⬇️
shard-5 34.31% <0.00%> (+0.88%) ⬆️
shard-6 45.37% <50.00%> (+0.31%) ⬆️

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

Files with missing lines Coverage Δ
src/auth/rate-limit.ts 98.88% <100.00%> (+0.05%) ⬆️
🚀 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 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-11 10:06:59 UTC

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

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): src/auth/rate-limit.ts (matched src/auth/**).

Review summary
This PR wraps the previously unguarded RATE_LIMITER Durable Object `.fetch()` call in try/catch to fail open on transient DO errors, and separately makes the rate_limit.denied audit write best-effort so a failed audit insert can't block a legitimate 429 response. Both changes are traced correctly to the described root cause: `enforceRateLimit` runs as global middleware ahead of every route's own error handling with no `app.onError` registered, so an uncaught DO exception previously surfaced as a bare, unstructured 500 indistinguishable from an app bug — exactly matching the client-side `orb_broker_unavailable` Sentry symptom in #5000. The four new regression tests exercise real, plausible failure modes (DO throw, non-Error throw, audit-write failure, non-Error audit rejection) rather than fabricated states, and both `instanceof Error` branches are covered.

Nits — 7 non-blocking
  • src/auth/rate-limit.ts:64 — the hardcoded `https:​//rate-limit/check` placeholder URL predates this diff and isn't introduced here, but since you're already touching this block it'd be a good time to hoist it into a named constant for readability.
  • The four new tests in test/unit/auth.test.ts are fairly repetitive (Error vs non-Error throw, duplicated for both the DO-fetch and audit-write paths) — consider a small `it.each`/table-driven helper to cut the duplication.
  • console.error/console.warn payloads are hand-built with JSON.stringify inline in two places now — if there's an existing structured-logger helper elsewhere in the codebase, prefer that for consistency.
  • Consider asserting in the new tests that no `x-ratelimit-*` response headers are set when failing open (since `decisionResponse` never resolves in that branch), to lock in that the fail-open path doesn't fabricate rate-limit headers from stale/undefined data.
  • If there's a shared metrics/alerting sink for `console.error` events elsewhere (e.g. for Sentry breadcrumbs), verify `rate_limit_check_failed` will actually surface there so silent DO outages don't go unnoticed now that they're swallowed.
  • 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.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #5000
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: 47 registered-repo PR(s), 39 merged, 428 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 47 PR(s), 428 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
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: not available
  • Official Gittensor activity: 47 PR(s), 428 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 added the manual-review Gittensor contributor context label Jul 11, 2026
@JSONbored
JSONbored merged commit afc7993 into main Jul 11, 2026
18 checks passed
@JSONbored
JSONbored deleted the fix/5000-ratelimit-fail-open branch July 11, 2026 10:13
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. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(orb): /v1/orb/token still returns 500 after the #orb-broker-500 fix (95 Sentry events)

1 participant