Document MCP per-client rate limiting and the durable quota hook#571
Conversation
…per#1633) Companion to HarperFast/harper#1633 (fixes HarperFast/harper#1610): rateLimit.perClientPerSecond/perClientBurst/identityHeader and the mcp.<profile>.quota.* hook with the counter-table example. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds documentation for MCP per-client rate limiting and durable quota configurations, including an example of a custom quota hook implementation. The review feedback correctly identifies an issue in the example code where the quota counter is incremented and written to the database before checking the limit, which could lead to excessive database write I/O under spam; a code suggestion is provided to check the limit first.
| static async allowMcpCall({ identity, tool, user, profile, sessionId }) { | ||
| const id = identity ?? 'unknown'; | ||
| const existing = await McpQuota.get(id); | ||
| const used = (existing?.used ?? 0) + 1; | ||
| await McpQuota.put({ id, used }); | ||
| if (used > DAILY_LIMIT) { | ||
| return { allowed: false, message: 'daily quota reached', retryAfterSeconds: 3600 }; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
In the current implementation, the quota counter is incremented and written to the database (McpQuota.put) before checking if the limit has been exceeded. This means that once a client exceeds their daily quota, every subsequent blocked request will still trigger a database write. A malicious or misconfigured client spamming the endpoint could cause excessive database write I/O.
Additionally, checking the limit before incrementing prevents the counter from growing infinitely beyond the daily limit.
We should check the limit first, and only perform the database write if the quota is not yet exceeded.
| static async allowMcpCall({ identity, tool, user, profile, sessionId }) { | |
| const id = identity ?? 'unknown'; | |
| const existing = await McpQuota.get(id); | |
| const used = (existing?.used ?? 0) + 1; | |
| await McpQuota.put({ id, used }); | |
| if (used > DAILY_LIMIT) { | |
| return { allowed: false, message: 'daily quota reached', retryAfterSeconds: 3600 }; | |
| } | |
| return true; | |
| } | |
| static async allowMcpCall({ identity, tool, user, profile, sessionId }) { | |
| const id = identity ?? 'unknown'; | |
| const existing = await McpQuota.get(id); | |
| const used = existing?.used ?? 0; | |
| if (used >= DAILY_LIMIT) { | |
| return { allowed: false, message: 'daily quota reached', retryAfterSeconds: 3600 }; | |
| } | |
| await McpQuota.put({ id, used: used + 1 }); | |
| return true; | |
| } |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-571 This preview will update automatically when you push new commits. |
|
Reviewed via Claude review-queue (full review: documentation-571-27fb8cc.md). Well-written companion docs for harper#1633 — cross-checked every documented key/behavior against the implementation and they match, except one drift: Suggested addition: "Floored at 1 whole token — a fractional |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-571 This preview will update automatically when you push new commits. |
|
Reviewed via Claude review-queue (re-review, full review: documentation-571-0ac01aa.md). The commit pushed since my last review only adds a race-safety caveat to the quota-hook section — good addition, accurate and well-placed — but it doesn't touch the |
…ed to main) kriszyp review: the floor semantics were claimed but not written; and harper#1633 landed on main rather than v5.1, so the feature ships in 5.2.0, not 5.1.18. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both addressed in b9c98ad: the |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-571 This preview will update automatically when you push new commits. |
🧹 Preview CleanupThe preview deployment for this PR has been removed. |
3 participants
Companion docs for HarperFast/harper#1633 (fixes HarperFast/harper#1610).
Changes to
reference/mcp/configuration.mdmcp.<profile>.rateLimit.*entries:perClientPerSecond,perClientBurst(with the one-token floor semantics), andidentityHeader(with the proxy-must-strip warning).mcp.<profile>.quota.*section: the config keys, a complete counter-table example matching the reporter's use case, denial shape (quota_exceeded+retryAfterSeconds), and the fail-closed + runs-after-buckets semantics.Note for review
Version references say "5.1.18+" assuming harper#1633 (stacked on harper#1613) ships in the patch after 5.1.17 — confirm at merge time. Hold merging until harper#1633 lands.
Generated by an LLM (Claude Fable 5).