Skip to content

fix(api): guard Upstash rate-limit failures on non-keeper routes - #641

Merged
collinsezedike merged 3 commits into
drydocs:mainfrom
QwinDivy:fix/issue-639-upstash-rate-limit-guard
Aug 29, 2026
Merged

fix(api): guard Upstash rate-limit failures on non-keeper routes#641
collinsezedike merged 3 commits into
drydocs:mainfrom
QwinDivy:fix/issue-639-upstash-rate-limit-guard

Conversation

@QwinDivy

@QwinDivy QwinDivy commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

close #639

Summary

  • checkRateLimit's call to Upstash (ratelimit.limit(ip)) was unguarded on 6 routes (tx/deposit, tx/withdraw, tx/submit, tx/add-trustline, positions/[publicKey], vaults/index, vaults/[vaultId]), unlike the keeper routes which already catch this failure and return a clean 503.
  • All 6 now wrap the call the same way, logging a route-specific message and returning 503 {"error": "Rate limiter unavailable; refusing to run"} instead of crashing unguarded on a transient Upstash timeout.

Test plan

  • pnpm lint && pnpm typecheck pass locally
  • pnpm test (api package): 46/46 pass, including the new regression test asserting a 503 on a rejected checkRateLimit

@vercel

vercel Bot commented Aug 29, 2026

Copy link
Copy Markdown

@QwinDivy is attempting to deploy a commit to the Collins' projects Team on Vercel.

A member of the Team first needs to authorize it.

@drips-wave

drips-wave Bot commented Aug 29, 2026

Copy link
Copy Markdown

@QwinDivy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@collinsezedike collinsezedike left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The fix itself is correct: all 6 remaining routes get the same try/catch → log → 503 pattern already used in the keeper routes, and the new regression test covers it.

CI fails on three checks though:

  • PR Title: "Guard Upstash rate-limit failures on non-keeper routes" doesn't follow the conventional-commit format (type(scope): subject) this repo requires. Please rename to something like fix(api): guard Upstash rate-limit failures on non-keeper routes.
  • Commit Messages: same root cause, the commit header has no type prefix (type may not be empty, subject may not be empty).
  • Lint & Typecheck: prettier --check fails on api/__tests__/handlers.test.ts. Run pnpm format and push.

The PR body's Summary and Test plan sections are also still empty (the raw template). Please fill those in per CONTRIBUTING.md before this is ready for merge.

One thing worth considering, not blocking: the try/catch/503 block is now duplicated verbatim across 8 call sites. Might be worth pushing that into checkRateLimit itself so a future change to the guard doesn't need to be copy-edited into 8 files by hand, but that's a design call, not something I'd hold up this fix for.

@collinsezedike
collinsezedike force-pushed the fix/issue-639-upstash-rate-limit-guard branch from dee5979 to ebc08e2 Compare August 29, 2026 19:05
@collinsezedike collinsezedike changed the title Guard Upstash rate-limit failures on non-keeper routes fix(api): guard Upstash rate-limit failures on non-keeper routes Aug 29, 2026

@collinsezedike collinsezedike left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Merging now.

@collinsezedike
collinsezedike merged commit a1c8348 into drydocs:main Aug 29, 2026
8 of 9 checks passed
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.

[Bug] checkRateLimit's Upstash call is unguarded on all non-keeper routes

2 participants