Skip to content

Remove iroh rate limiting: make limiter optional and fail open - #8714

Merged
azooz2003-bit merged 2 commits into
mainfrom
feat-remove-iroh-rate-limit
Jul 23, 2026
Merged

azooz2003-bit merged 2 commits into
mainfrom
feat-remove-iroh-rate-limit

Conversation

@azooz2003-bit

@azooz2003-bit azooz2003-bit commented Jul 23, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

CMUX_IROH_RATE_LIMIT_ID was required on prod, and routeHandler.ts ran the Vercel firewall check on every authed iroh op whenever that env was set. When the rate-limit rule was deleted from the Vercel firewall (the owner removed iroh limiting), the .well-known/vercel/rate-limit-api/<id> check returns 404 → firewall.ts maps it to error: "not-found" → the old if (error) branch returned 503 iroh_service_unavailable on every discover/challenge/register, for all accounts, before the broker was ever reached.

That took off-tailnet iroh discovery down entirely: the review Mac could not publish an iroh binding, and phones off the tailnet could not discover it. /api/devices (a plain registry read) kept returning 200, which is why only the iroh endpoints were affected.

Fix

  • env: make CMUX_IROH_RATE_LIMIT_ID optional (z.string().min(1).optional()), matching the existing optional rate-limit IDs (CMUX_PUSH_RATE_LIMIT_ID, CMUX_RELAY_PREFERENCES_RATE_LIMIT_ID). Unsetting the var now skips the firewall gate instead of failing env validation at boot.
  • routeHandler: treat a missing rule ("not-found") as "no limit" and fail open (continue to the broker) instead of 503. A deleted rule means the operator removed the limit, not that the service is down. Genuine unavailability (firewall timeout / unexpected status) still fails closed via the existing catch, and "blocked"/rateLimited still return 429.

Deploying this clears the outage on its own, even before the env var is unset, and prevents a deleted rule from ever bricking iroh again. After deploy the owner can unset CMUX_IROH_RATE_LIMIT_ID in Vercel prod to fully remove the gate.

Test

Adds fails open when the configured rate-limit rule no longer exists — asserts a not-found result reaches the broker and returns 200. Existing fail-closed tests (firewall reject / never-settles → 503) and the 429/blocked path are unchanged. Full file: 14 pass, bun run typecheck clean.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Make iroh rate limiting optional and fail open when the Vercel firewall rule is missing. This prevents 503s and restores off‑tailnet iroh discovery when a rate-limit rule is deleted.

  • Bug Fixes
    • Made CMUX_IROH_RATE_LIMIT_ID optional; when unset, the firewall check is skipped.
    • Treated firewall error: "not-found" as “no limit” and continued to the broker; still return 429 for blocked/rate-limited and fail closed on timeouts/unexpected errors.
    • Added a regression test that asserts not-found reaches the broker and the firewall check ran, returning 200.

Written for commit feaeb36. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Iroh requests now fail open when the configured rate-limit rule is missing, instead of returning a service-unavailable error.
    • Rate-limited or blocked requests continue to receive the appropriate rejection responses.
    • The Iroh rate-limit configuration is now optional; leaving it unset disables rate limiting.
  • Tests

    • Added coverage confirming requests succeed when the rate-limit rule is not found.

CMUX_IROH_RATE_LIMIT_ID was required on prod, and routeHandler ran the
Vercel firewall check whenever it was set. When the rate-limit rule was
deleted, the .well-known check returns 404 -> "not-found", and the old
`if (error)` branch turned that into a 503 iroh_service_unavailable on
every authed discover/challenge/register for every account. That blocked
off-tailnet iroh discovery entirely (the review Mac could not publish an
iroh binding; phones could not discover it).

Two changes so the limit can be removed cleanly:
- env: make CMUX_IROH_RATE_LIMIT_ID optional (z.string().min(1).optional()),
  matching CMUX_PUSH_RATE_LIMIT_ID / CMUX_RELAY_PREFERENCES_RATE_LIMIT_ID.
  Unsetting the var now skips the firewall gate instead of failing env
  validation at boot.
- routeHandler: treat a missing rule ("not-found") as "no limit" and fail
  open (continue to the broker) instead of 503. A deleted rule means the
  operator removed the limit, not that the service is down. Genuine
  unavailability (timeout / unexpected status) still fails closed via the
  existing catch.

This makes the deploy itself clear the outage even before the env var is
unset, and prevents a deleted rule from ever bricking iroh again. Adds a
regression test asserting not-found fails open with a 200.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Iroh rate-limit configuration is now optional. When the configured firewall rule is missing, the route logs a warning and continues the discovery request instead of returning 503.

Changes

Iroh rate-limit handling

Layer / File(s) Summary
Optional rate-limit configuration
web/app/env.ts
CMUX_IROH_RATE_LIMIT_ID is now an optional non-empty string; leaving it unset disables Iroh rate limiting.
Missing-rule fail-open handling
web/services/iroh/routeHandler.ts, web/tests/iroh-route-handler.test.ts
Missing firewall rules produce a warning and allow discovery to return HTTP 200; a route-boundary test verifies this behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed No Swift files were changed; the PR only touches a TypeScript test, so Swift actor-isolation rules are not implicated.
Cmux Swift Blocking Runtime ✅ Passed PR changes only TypeScript files (env, route handler, test); no Swift files or blocking-sync primitives were introduced.
Cmux Browser Automation Off-Main ✅ Passed The diff only touches an iroh route test and contains no browser/WebKit/socketWorkerMethods/mainActor automation changes, so the off-main browser rule isn’t implicated.
Cmux Expensive Synchronous Load ✅ Passed PR diff changes only web/*.ts files; no Swift files or SwiftUI/main-actor sync-load paths are touched, so the rule is not applicable.
Cmux Cache Substitution Correctness ✅ Passed PASS: this PR only makes iroh firewall gating optional/fail-open; it does not swap any fresh authoritative persistence/history/snapshot read for cached or opportunistic data.
Cmux No Hacky Sleeps ✅ Passed PR diff adds no fixed waits/timers; the only touched file is a test and the existing route timeout is unchanged, bounded, and cancellation-aware.
Cmux Algorithmic Complexity ✅ Passed PASS: env.ts only makes one env var optional; routeHandler adds a constant-time not-found branch, and the new test is fixture-only.
Cmux Swift Concurrency ✅ Passed PASS: The PR only changes web TypeScript files; no Swift files or Swift concurrency patterns are introduced or expanded.
Cmux Swift @Concurrent ✅ Passed The PR diff touches only web/app/env.ts, web/services/iroh/routeHandler.ts, and web/tests/iroh-route-handler.test.ts; no Swift files changed, so the rule is not applicable.
Cmux Swift Package Boundaries ✅ Passed PR changes only web/app and test TS files; no Swift diff, so the Swift package-boundary rule is not applicable.
Cmux Swiftpm Lockfiles ✅ Passed Diff only changes a web test; no cmux-owned Package.swift/.gitignore/workflow/Xcode project or Package.resolved files changed, so the rule doesn’t apply.
Cmux Swift Logging ✅ Passed No Swift runtime files were changed; the diff is a TypeScript test, so the Swift logging rule is not applicable.
Cmux User-Facing Error Privacy ✅ Passed PASS: The patch only makes the env optional and adds an internal warning/test; it doesn’t add or alter any user-facing error body or recovery copy.
Cmux Full Internationalization ✅ Passed Changed env, route logic, and tests only; no user-facing localized UI/data copy or locale files were added or modified.
Cmux Swiftui State Layout ✅ Passed No SwiftUI views/state were changed; touched files are env, iroh route handling, and tests, with no ObservableObject/GeometryReader/render-time mutation patterns.
Cmux Architecture Rethink ✅ Passed PASS: the diff only changes a TypeScript test file; no Swift files or Swift architectural-rethink patterns are present, so the rule isn't implicated.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: HEAD only changes web/tests/iroh-route-handler.test.ts; no Swift files or auxiliary-window code were touched, so the Swift close-shortcut rule is not applicable.
Cmux Source Artifacts ✅ Passed All changed paths are hand-written source/test files; no logs, build output, temp dirs, or other artifact paths were added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed PR diff only changes web TS files; no production Swift Sources/** seam or debug hook was introduced.
Cmux No Ambient Global State ✅ Passed PASS: The PR only changes web TypeScript files; no production Swift code or new ambient global state was added.
Title check ✅ Passed The title clearly summarizes the main change: making iroh rate limiting optional and failing open.
Description check ✅ Passed The description covers the problem, fix, and testing, but it omits the demo video and checklist sections from the template.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-remove-iroh-rate-limit

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a production outage where deleting the Vercel firewall rate-limit rule for iroh caused every discover/challenge/register request to return 503, by making the not-found result fail open instead of returning a service-unavailable error. It also makes CMUX_IROH_RATE_LIMIT_ID optional so unsetting the env var skips the gate entirely, matching the existing pattern for other rate-limit IDs.

  • routeHandler.ts: replaces the old catch-all if (error) → 503 with a narrow if (error === "not-found") → warn + fall through, preserving fail-closed behaviour for timeouts and unexpected statuses via the existing catch.
  • env.ts: changes CMUX_IROH_RATE_LIMIT_ID from requireVercelNonPreviewValue to z.string().min(1).optional(), consistent with CMUX_PUSH_RATE_LIMIT_ID and CMUX_RELAY_PREFERENCES_RATE_LIMIT_ID.
  • Test: adds a regression case asserting that a not-found firewall result reaches the broker and returns 200.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to the firewall error-handling path and restores the intended fail-open behaviour for a deleted rule while keeping the fail-closed path intact for genuine unavailability.

The logic change is correct: the only reachable error values after the blocked check are not-found and undefined, so replacing the old catch-all with an explicit not-found guard covers the full type space. The catch block still returns 503 for throws, blocked/rateLimited still returns 429, and the env change is consistent with existing optional rate-limit IDs. The new test verifies the fail-open path end-to-end.

No files require special attention.

Important Files Changed

Filename Overview
web/app/env.ts Makes CMUX_IROH_RATE_LIMIT_ID optional to match other rate-limit env vars; when unset the firewall gate is skipped.
web/services/iroh/routeHandler.ts Replaces catch-all error → 503 branch with a narrow not-found → warn + fail-open path; blocked/rateLimited still 429, throws still 503.
web/tests/iroh-route-handler.test.ts Adds regression test asserting not-found firewall result reaches the broker and returns 200; existing fail-closed and 429 tests unchanged.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming iroh request] --> B{CMUX_IROH_RATE_LIMIT_ID set?}
    B -- No --> G[Proceed to broker]
    B -- Yes --> C[Run Vercel firewall check]
    C -- throws / timeout --> D[503 iroh_service_unavailable]
    C -- rateLimited=true or error=blocked --> E[429 rate_limited]
    C -- error=not-found --> F[warn + fail open]
    C -- no error --> G
    F --> G
    G --> H[Broker call]
    H -- success --> I[200/201 response]
    H -- error --> J[4xx/5xx error response]
Loading

Reviews (2): Last reviewed commit: "Assert firewall check ran in the fail-op..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@web/tests/iroh-route-handler.test.ts`:
- Around line 68-94: Update the test “fails open when the configured rate-limit
rule no longer exists” to track a firewallCalled flag within the injected
firewall.check implementation, then assert it is true alongside the existing
response and broker assertions. Preserve the current not-found result and
fail-open behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2f9a0eff-636d-4253-8511-5e78ed21dc40

📥 Commits

Reviewing files that changed from the base of the PR and between 38b8ca7 and 8d7e5bc.

📒 Files selected for processing (3)
  • web/app/env.ts
  • web/services/iroh/routeHandler.ts
  • web/tests/iroh-route-handler.test.ts

Comment thread web/tests/iroh-route-handler.test.ts
Addresses CodeRabbit: the not-found fail-open test would still pass if
handleIrohRoute skipped the injected firewall entirely. Track a
firewallCalled flag inside check and assert it, so the regression proves
the not-found path specifically (firewall ran, returned not-found,
handler failed open to the broker).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant