fix(evmrpc): don't charge an innocent client's per-IP bucket on mid-read budget exhaustion - #3935
fix(evmrpc): don't charge an innocent client's per-IP bucket on mid-read budget exhaustion#3935amir-deris wants to merge 6 commits into
Conversation
amir-deris
changed the title
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryMedium Risk Overview
Adds Reviewed by Cursor Bugbot for commit 2ad70de. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1c57165. Configure here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3935 +/- ##
==========================================
- Coverage 59.50% 58.51% -1.00%
==========================================
Files 2326 2230 -96
Lines 198890 188338 -10552
==========================================
- Hits 118359 110208 -8151
+ Misses 69285 67727 -1558
+ Partials 11246 10403 -843
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix correctly stops rateLimitMiddleware from debiting an innocent client's per-IP bucket when the shared byte budget is exhausted mid-read, and the new composed-stack test genuinely fails without it. One gap: the deferral is keyed on error shape rather than on the outer limiter actually owning the failure, so with both max_concurrent_request_bytes and body_read_idle_timeout set to 0 a ReadTimeout now returns silently instead of producing a status and a metric.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The PR's second stated goal — no more double-counting of
evmrpc_requests_rejected_totalunder two reasons — isn't covered by a test.TestRateLimitMiddleware_ParseErrorRecordsRejectedMetricalready shows thesdkmetric.NewManualReaderpattern for this package; asserting exactly one datapoint withbudget_midread(and noread_error) for the mid-read failure would pin the metric half of the fix, which is otherwise only pinned by the status code. - Consider recording the new cross-layer rule in
evmrpc/AGENTS.md's middleware-order section:requestSizeLimiterowns the response and rejection reason forbudget_midread/slow_body, andrateLimitMiddlewaremust not charge admission for those. That ownership split is now load-bearing for two files and isn't derivable from either one alone. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The errSlowBody sentinel closes the gap from the previous review — the deferral now keys on ownership (only budgetBody can produce either sentinel) rather than error shape, so the "both guards disabled" silent-200 path is gone, and the new composed-stack test genuinely pins the innocent-IP fix. Remaining notes are non-blocking: bundling slow_body (client-attributable) with budget_midread (server-attributable) also drops the fail-closed admission charge for slowloris clients, and neither the metric de-duplication nor the errSlowBody path itself is covered by a test.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The PR's second stated goal — no more double-counting of
evmrpc_requests_rejected_totalunder two reasons — still isn't covered by a test.TestRateLimitMiddleware_ParseErrorRecordsRejectedMetricalready shows thesdkmetric.NewManualReaderpattern in this package; asserting exactly one datapoint withbudget_midreadand none withread_errorfor the mid-read failure would pin the metric half of the fix, which is otherwise only pinned indirectly by the status code. - Only the
errBudgetExhaustedbranch is exercised.errSlowBodyis a new value flowing through a second layer, and nothing asserts that a body-read idle timeout now yields 408 + exactly oneslow_bodydatapoint with the per-IP bucket untouched.TestRequestSizeLimiter_bodyReadIdleTimeoutcovers the limiter alone, not the composed stack. - Consider recording the cross-layer rule in
evmrpc/AGENTS.md's middleware-order section (which already describes the 408/429 split at lines 19-24):requestSizeLimiterowns the response and rejection reason forbudget_midread/slow_body, andrateLimitMiddlewaremust not charge admission or record a reason for those. The sentinels make it enforceable in code, but the ownership split spans two files and isn't derivable from either alone. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
| if isRequestBodyTooLarge(err) { | ||
| m.rejectAdmission(r.Context(), w, ip, rejectReasonOversize, http.StatusRequestEntityTooLarge, "request body too large") | ||
| return | ||
| } | ||
| if errors.Is(err, errBudgetExhausted) { | ||
| // Server-side capacity event; outer limiter already owns the response. | ||
| return | ||
| } | ||
| if errors.Is(err, errSlowBody) { | ||
| // Client-caused stall: still charge the per-IP bucket. | ||
| _ = m.gate.chargeAdmissionRejection(r.Context(), ip) | ||
| return | ||
| } | ||
| m.rejectAdmission(r.Context(), w, ip, rejectReasonReadError, http.StatusBadRequest, "bad request") | ||
| return | ||
| } |
There was a problem hiding this comment.
Outside the scope of your PR, but a bit strange that we seem to be swallowing errors and not logging them.
There was a problem hiding this comment.
Thanks for feedback. Regarding logging, I think that opens the gate for more DOS problems due to writing logs to disk. It seems adding metrics around them could be a more suitable approach. Perhaps we can revisit this once it is rolled out and we see how much rate limiting traffic we get
|
@seidroid review |
3 participants
Describe your changes and provide context
rateLimitMiddlewareand the outerrequestSizeLimiterboth watch the same request body, but disagreed about who owns a mid-read failure. When the global byte budget (max_concurrent_request_bytes) or the body-read idle timeout was exhausted whilereadBoundedBodywas reading, the error surfaced torateLimitMiddlewareas a generic read failure. It responded by charging the requesting client's own per-IP token bucket (chargeAdmissionRejection) and recording aread_errorrejection metric — on top of thebudget_midread/slow_bodyreason and metric the outerrequestSizeLimiterhad already recorded for the same request.Two problems followed:
evmrpc_requests_rejected_totaldouble-counted the same rejected request under two different reasons.Flagged in review on #3836: #3836 (comment)
Fix
rateLimitMiddlewarenow recognizeserrBudgetExhausted/ a read-idle-timeout error fromreadBoundedBodyand returns without charging admission or recording its own rejection reason, deferring entirely torequestSizeLimiter, which already owns the correct reason and the client-visible response.Testing performed to validate your change
TestComposedStack_BudgetMidreadDoesNotChargeInnocentIP, which holds the global budget open with one client's in-flight request and verifies a second, distinct client's request that fails mid-read due to budget exhaustion does not consume that client's own per-IP token (a follow-up request from the same IP still succeeds underburst=1).go test ./evmrpc/...passes.gofmt -s/goimportsclean on changed files.