fix(evmrpc): stream request-body budget charging to close slowloris gap (PLT-780)

[amir-deris]

Summary

Fixes PLT-780: the EVM HTTP requestSizeLimiter reserved global byte-budget capacity based on the declared Content-Length before the body was read, and held that reservation across the full request. A slow/stalled body (slowloris-style) could pin a large reservation cheaply, exhausting the shared budget and denying the endpoint — including for JWT-protected deployments, since the limiter sat outside the JWT check.

• Charge the global byte budget incrementally as body bytes are read (batched in 64 KiB steps) instead of trusting Content-Length/reserving maxBody up front. A stalled body now pins at most one batch, not its declared size.
• Add body_read_idle_timeout (default 10s): a per-chunk idle guard via http.ResponseController.SetReadDeadline, independent of the overall ReadTimeout, so a stalled read is cut and its reservation released quickly (HTTP 408).
• Reorder the HTTP middleware stack so JWT runs before requestSizeLimiter — unauthenticated clients get 401 without ever touching the shared budget.
• New metrics reasons budget_midread / slow_body on requestRejectedCount for the new rejection paths.

Fixes found while validating

• seiLegacyHTTPGate — always present in the production stack — buffers the whole body via its own io.ReadAll and, on a failed read, was writing its own 400 response with the raw internal error text before the limiter's outcome-based 429/408 could apply. captureResponseWriter now suppresses inner-handler writes once the limiter's outcome is set, so the documented status/message always reaches the client.
• That same gate closes the body immediately after buffering it, before dispatching to the real handler. Because releasing the budget was tied to Body.Close(), this freed the reservation the instant the body finished buffering rather than holding it for the whole request — dropping protection against concurrent slow processing of already-read bodies. Release is now decoupled from Close() and happens once, after the inner handler chain fully returns.

Test plan

• go test ./evmrpc/... and go test ./evmrpc/config/...
• go test ./evmrpc/ -race on the request-limiter/legacy-gate tests
• New regression tests: incremental budget charging, slowloris no longer pins declared size, body read idle timeout (408) + steady slow upload still succeeds, JWT runs before the byte limiter, and budget outcome survives being routed through seiLegacyHTTPGate
• gofmt -s -l / goimports -l clean on all touched files

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 4, 2026, 2:56 PM

[codecov]

Codecov Report

❌ Patch coverage is 89.39394% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.70%. Comparing base (1ecc672) to head (5ee99e3).

Files with missing lines	Patch %	Lines
evmrpc/request_limiter.go	88.42%	10 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3836      +/-   ##
==========================================
- Coverage   61.55%   60.70%   -0.86%     
==========================================
  Files        2361     2269      -92     
  Lines      199445   189055   -10390     
==========================================
- Hits       122778   114768    -8010     
+ Misses      65710    64182    -1528     
+ Partials    10957    10105     -852     

Flag	Coverage Δ
sei-chain-pr	70.89% <89.39%> (?)
sei-db	70.41% <ø> (ø)
sei-db-state-db	?

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

Files with missing lines	Coverage Δ
evmrpc/config/config.go	96.95% <100.00%> (+0.04%)	⬆️
evmrpc/metrics.go	97.46% <ø> (ø)
evmrpc/rpcstack.go	79.80% <100.00%> (+0.34%)	⬆️
evmrpc/server.go	89.33% <100.00%> (+0.04%)	⬆️
evmrpc/request_limiter.go	89.78% <88.42%> (-3.08%)	⬇️

... and 133 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cursor]

PR Summary

Medium Risk
Changes admission control on the public EVM HTTP RPC path (middleware order, byte accounting, and new rejection statuses), which can affect legitimate slow clients and concurrent load behavior under mis-tuned limits.

Overview
Closes the slowloris-style gap on EVM HTTP JSON-RPC where max_concurrent_request_bytes was reserved up front from Content-Length (or maxBody for unknown length) and held for the whole request. requestSizeLimiter now charges the shared budget incrementally in 64 KiB batches as the body is read, returns HTTP 429 if the budget is exhausted mid-read, and releases reservations on failure instead of pinning declared size on stalled uploads.

Adds body_read_idle_timeout (default 10s, evm.body_read_idle_timeout) with per-chunk ResponseController deadlines clamped to ReadTimeout, cutting stalled reads with HTTP 408 and freeing held budget. HTTP middleware is reordered so JWT runs before the byte limiter; metrics gain budget_midread and slow_body on requestRejectedCount, and busy is scoped to WebSocket admission.

captureResponseWriter ensures limiter 429/408 wins over inner responses (e.g. seiLegacyHTTPGate), and budget release is deferred until the inner handler chain returns so concurrent processing stays protected after the body is buffered.

^{Reviewed by Cursor Bugbot for commit 5ee99e3. Bugbot is set up for automated code reviews on this repo. Configure here.}

[seidroid]

The automated review did not complete; see the failing AI Review check for details.

[cursor]

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.}

^{Reviewed by Cursor Bugbot for commit 137724a. Configure here.}

[cursor]

ReadTimeout clamp starts too late

Medium Severity

absDeadline is computed as time.Now().Add(readTimeout) at ServeHTTP entry, after headers are already read. Each SetReadDeadline call replaces the connection deadline, so this clamp can extend past http.Server.ReadTimeout by roughly the header-read duration. With slow headers near ReadHeaderTimeout, body trickling can continue for about another full ReadTimeout.

Additional Locations (1)

• evmrpc/request_limiter.go#L144-L154

 

^{Reviewed by Cursor Bugbot for commit 137724a. Configure here.}

[seidroid]

The incremental-charging redesign is well-reasoned and thoroughly tested, but the branch does not compile (rejectReasonBusy was deleted while still referenced by the WS reason mapping and its test) and the new config golden is recorded in the wrong field order, which will fail TestDefaultsMatchTheRecordedValues. Several smaller issues follow: duplicated/now-stale config documentation, an unsuppressed Flush() that can commit a 200 over the limiter's 429/408, and an undocumented metrics-label rename.

Findings: 4 blocking | 14 non-blocking | 10 posted inline

Blockers

• evmrpc does not compile: rejectReasonBusy was removed from evmrpc/metrics.go but is still referenced at evmrpc/metrics.go:194 (WS admission reason mapping) and evmrpc/metrics_test.go:18-19. Either keep the constant for the WS path or update both call sites. (Confirmed independently; Codex flagged the same thing as its only finding.)
• evmrpc/config/testdata/evm.golden records BodyReadIdleTimeout after WSAdmissionTimeout, but configtest.Dump walks struct fields in declaration order and Config declares BodyReadIdleTimeout (config.go:282) before WSAdmissionTimeout (config.go:289). TestDefaultsMatchTheRecordedValues should fail. Fix by moving the golden line up, or by moving the struct field below WSAdmissionTimeout and regenerating.
• 2 blocking issue(s) flagged inline on specific lines.

Non-blocking

• The reason label vocabulary on evmrpc_requests_rejected_total{protocol="http"} changes: busy is replaced by budget_midread, plus a new slow_body. Any dashboard/alert keyed on reason="busy" for HTTP silently goes to zero. Worth calling out in the PR description / an operator-facing note, since busy remains in use for the WS plane (asymmetric vocabulary).
• Rejection is no longer pre-decode: TestRequestSizeLimiter_budgetExhaustionAndRelease now asserts innerCalls == 2, i.e. over-budget requests do reach the inner handler and its body read. The doc comment on recordRequestRejected ("dropped by pre-decode admission control") and the requestSizeLimiter doc ("bounds peak decode-time memory") are now only partially accurate — worth rewording so the next reader doesn't assume nothing downstream ran.
• The readTimeout clamp is untested: every test and benchmark passes readTimeout = 0, so the absDeadline branch in budgetBody.Read has no coverage. A test with readTimeout > 0 and idleTimeout trickling below it would pin the intended "can't extend past ReadTimeout" behavior.
• The actual wiring in EnableRPC (JWT outside the limiter, NewHTTPHandlerStack(..., nil) so JWT isn't double-applied, h.timeouts.ReadTimeout threaded through) has no test; TestJWTBeforeRequestSizeLimiter composes newJWTHandler(secret, limiter) by hand, which would still pass if EnableRPC regressed. The stack order documented in evmrpc/AGENTS.md is likewise unasserted.
• budgetBody and captureResponseWriter mutate shared state (reserved/unbilled, outcome, wroteHeader) with no synchronization, and the deferred Close()/release() runs on the ServeHTTP goroutine. That is safe for every handler in today's chain (all read the body inline), but any future handler that reads r.Body from a spawned goroutine introduces a data race silently. A one-line comment stating the single-goroutine assumption would help.
• Cursor's second-opinion pass (cursor-review.md) is empty — that review produced no output, so only Codex's pass contributed here.
• 8 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] Removing rejectReasonBusy breaks the build: it is still referenced at metrics.go:194 in mapWSAdmissionRejectReason (for WSAdmissionReasonBudgetWaitTimeout / WSAdmissionReasonFrameAdmissionTimeout) and in metrics_test.go:18-19. The WS plane still genuinely means "busy" — the two new reasons are HTTP-body-read specific — so keeping rejectReasonBusy alongside them is probably the right fix rather than remapping WS onto budget_midread.

[seidroid]

[blocker] Ordering doesn't match the struct. configtest.Dump emits fields in declaration order (testutil/configtest/dump.go:90-106), and Config declares BodyReadIdleTimeout at config.go:282, before WSAdmissionTimeout at config.go:289. So the golden needs BodyReadIdleTimeout on the line above WSAdmissionTimeout; as written TestDefaultsMatchTheRecordedValues should fail. (Alternatively move the struct field below WSAdmissionTimeout and regenerate with -update.)

[seidroid]

[suggestion] This appends a second doc block for the same field instead of updating the existing one, and the paragraph above (lines 263-271) is now wrong — it still says "HTTP uses Content-Length weighting and rejects over-budget requests fast (HTTP 429)". It also drops the useful WS/2×-budget context that the old paragraph carried. Please fold the new sentences into the existing comment and delete the stale Content-Length claim.

[seidroid]

[suggestion] Same duplication in the rendered app.toml: operators will now see two consecutive, partly contradictory comment paragraphs for a single max_concurrent_request_bytes key (the first still says "HTTP rejects over-budget requests fast", the second says "charged incrementally ... rejected mid-read"). Merge into one block.

[seidroid]

[suggestion] Flush() doesn't check suppressed(). net/http's (*response).Flush calls WriteHeader(200) when nothing has been written yet, so an inner handler that flushes after the body read failed would commit a 200 to the wire while cw.wroteHeader stays false — the outer http.Error(w, outcome.message, outcome.status) then logs "superfluous WriteHeader" and the client gets 200 instead of the documented 429/408. Suggest an early if w.suppressed() { return }, matching Write/WriteHeader.

Separately: Flush is now declared unconditionally, so cw always satisfies http.Flusher even when the wrapped writer doesn't — a downstream w.(http.Flusher) probe will now succeed and silently no-op.

[seidroid]

[suggestion] absDeadline is anchored at handler-entry time, but http.Server armed its ReadTimeout deadline back when it started reading the request. So now + readTimeout here is strictly later than the server's own deadline — by the header-read plus queueing time — which means the doc comments at line 54-55 and 133-136 ("can never extend past the server's own absolute http.Server.ReadTimeout") overstate what this does; the middleware actually relaxes that deadline slightly. Either soften the comment or derive the anchor from something request-scoped (e.g. capture it in a ConnContext/BaseContext hook at accept time).

[seidroid]

[suggestion] Worth spelling out in this comment: because charging only happens at 64 KiB granularity (charge) or at EOF (flush), a body smaller than 64 KiB — i.e. essentially every real JSON-RPC request — is fully buffered by seiLegacyHTTPGate before a single byte is charged. With max_open_connections = 2000 that leaves up to ~128 MiB of in-flight body bytes unaccounted, on top of the 128 MiB budget, so max_concurrent_request_bytes is now a looser bound on peak memory than the old Content-Length reservation was. That's a deliberate trade for closing the slowloris hole, but it should be stated here (and ideally in the config doc) rather than left implicit.

[seidroid]

[nit] _ = b.flush() discards an errBudgetExhausted here, so trailing bytes charged at close time are silently forgiven and no outcome is set. That's reachable whenever the inner handler stops short of EOF — e.g. seiLegacyHTTPGate's io.LimitReader(r.Body, maxBody+1) returning EOF on its own for an over-length chunked body. Failing open is defensible, but a comment saying so would stop the next reader from treating it as an oversight.

[seidroid]

[nit] On a generic (non-EOF, non-timeout) read error this releases the reservation immediately, even though the inner handler may still be holding and processing the bytes already delivered. That's the exact coupling the PR description sets out to remove ("release is now decoupled from Close() and happens once, after the inner handler chain fully returns"). Since ServeHTTP's deferred release() already covers it, dropping this call would make the policy uniform. It also skips clearReadDeadline(), unlike the EOF and fail paths.

[seidroid]

[nit] No range check here, unlike the immediately preceding max_open_connections block. A negative body_read_idle_timeout casts fine and then silently disables the idle guard (l.bodyReadIdleTimeout > 0 is false), which contradicts the field doc's "Zero disables". Either reject < 0 like max_open_connections does, or document that negative behaves as zero.

[claude]

🟡 In requestSizeLimiter.ServeHTTP (evmrpc/request_limiter.go:88-89), absDeadline is computed as time.Now().Add(l.readTimeout) at handler entry, but by then net/http has already spent up to ReadHeaderTimeout parsing headers before dispatching. Since budgetBody overwrites the connection's own t0+ReadTimeout deadline with this later value on every chunk read, a client that stalls headers near ReadHeaderTimeout and then trickles the body just under body_read_idle_timeout can hold the body-read phase open for up to roughly ReadHeaderTimeout + ReadTimeout instead of ReadTimeout, slightly loosening the slowloris backstop this code is meant to enforce. This is a minor, bounded imprecision (not merge-blocking) — fixing it exactly isn't possible since net/http doesn't expose the true per-request t0 to handlers.

Extended reasoning...

What the bug is. requestSizeLimiter.ServeHTTP computes absDeadline := time.Now().Add(l.readTimeout) at evmrpc/request_limiter.go:88-89, the moment the handler is invoked. Go's net/http arms its own whole-request read deadline earlier and once, inside readRequest: t0 := time.Now() is captured before headers are parsed, and wholeReqDeadline = t0.Add(srv.ReadTimeout) is set on the connection to bound the entire read (headers + body). ServeHTTP only runs after headers are fully parsed and the request is dispatched, so time.Now() at that point is already t0 + headerReadDuration, where headerReadDuration can be as large as ReadHeaderTimeout (default 30s).\n\nHow it manifests. budgetBody.Read (lines ~144-156) calls b.rc.SetReadDeadline(deadline) on every chunk, where deadline = min(now+idleTimeout, absDeadline). This SetReadDeadline call goes straight to the underlying connection's deadline — the same one net/http armed at t0+ReadTimeout. Because absDeadline is anchored at ServeHTTP entry rather than t0, every per-chunk reset clamps to a deadline that is later than net/http's original bound by the header-read duration. The code comment at line ~150 explicitly claims this clamp ensures the deadline "can never extend past the server's own absolute http.Server.ReadTimeout for the request" — that guarantee doesn't hold exactly, because the reference point used is wrong.\n\nWhy nothing else catches this. The per-chunk idle guard (body_read_idle_timeout, default 10s) only fires on gaps longer than the idle timeout between chunks. A client that stalls sending headers for up to ~30s and then sends the body in a slow trickle (e.g., a few bytes every 9s) never triggers the idle guard, and the absolute bound that should stop it at t0+30s has been silently pushed out to ServeHTTP-entry+30s — up to another ~30s later.\n\nStep-by-step proof:\n1. Server runs with defaults: ReadTimeout = 30s, ReadHeaderTimeout = 30s, body_read_idle_timeout = 10s.\n2. A client opens a connection, sends request headers/line but trickles them so header parsing takes ~29s (just under ReadHeaderTimeout). net/http's connection deadline was armed at t0+30s when the read began, i.e. at ~1s after t0 remaining before the original bound would trip.\n3. Headers finish parsing at ~(t0+29s); ServeHTTP is invoked. absDeadline is now computed as (t0+29s) (+) (30s = t0+59s) — roughly double the intended (t0+30s) bound.\n4. The client then sends the body 1 byte at a time every 9s (under the 10s idle timeout). Each Read call resets the connection deadline to (min(now+10s, t0+59s)).\n5. The connection stays open until roughly (t0+59s) instead of being cut at (t0+30s) — nearly double the configured ReadTimeout, worst case approaching (2\times ReadTimeout) when ReadHeaderTimeout \approx ReadTimeout\).\n\n**Impact and why this is a nit, not a blocker.** The connection is still finitely bounded — it is not an unbounded slowloris vector (unlike a related, separately-reported issue where the idle deadline is never cleared/re-clamped after the first chunk). The overrun is capped at the header-read duration, requires an adversarial header-stall to trigger, and the change is still strictly tighter than the pre-PR behavior (which had no per-chunk idle guard at all and could pin a full declared Content-Lengthreservation). There's also no clean fix available:net/httpdoes not expose the true per-requestt0or the connection's already-armed deadline to ahttp.Handler, so recovering the exact original bound isn't possible from user code; the best practical mitigation is to subtract a conservative estimate (e.g. ReadHeaderTimeout) when computing absDeadline, accepting it's an approximation rather than an exact fix.\n\n**Fix suggestion.** Approximate the true anchor by subtracting ReadHeaderTimeout(when configured) fromtime.Now()before addingreadTimeout, e.g. absDeadline := time.Now().Add(l.readTimeout - l.readHeaderTimeout)`, clamped to not go negative. This tightens the worst case back toward the intended bound even though it can't be made exact without upstream net/http support.

[seidroid]

The slowloris fix is a sound design (incremental budget charging, per-chunk idle deadline, JWT ahead of the byte limiter) with good test coverage, but it deletes rejectReasonBusy while evmrpc/metrics.go:194 and evmrpc/metrics_test.go:18-19 still reference it, so package evmrpc no longer compiles. Beyond that, the 64 KiB batching leaves up to one batch per in-flight request outside the budget (~2× the intended memory bound at default settings), and the duplicated/contradictory config docs plus a missing CHANGELOG entry need cleanup.

Findings: 2 blocking | 15 non-blocking | 12 posted inline

Blockers

• evmrpc/metrics.go removes the rejectReasonBusy constant but two unchanged references remain (mapWSAdmissionRejectReason at metrics.go:194, and metrics_test.go:18-19), and no "busy" literal exists anywhere else in the tree. Package evmrpc and its test binary will not build. Decide whether WS budget-wait/frame-admission timeouts keep reporting busy (restore the constant) or move to a new value (update the mapper, its doc comment which still says "oversize/busy vocabulary", and the test table).
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• No CHANGELOG.md entry. The immediately preceding, same-subsystem PR (#3818) added an ### Improvements entry covering max_concurrent_request_bytes / ws_admission_timeout, so the convention is established. This PR adds an operator-facing key (body_read_idle_timeout) and changes the HTTP evmrpc_requests_rejected_total{reason=...} vocabulary (busy → budget_midread, plus new slow_body) — the label-value change silently breaks any dashboard or alert keyed on reason="busy",protocol="http" and deserves an explicit note.
• Rejection is no longer pre-decode: over-budget and stalled requests now enter the full inner handler chain (legacy gate → gzip → vhost → cors) and are only rejected after it returns — the updated test asserts this (innerCalls == 2, "rejected request reaches inner handler but fails on body read"). That is inherent to the approach and correctly handled for seiLegacyHTTPGate, but it does mean any inner middleware with side effects on a failed body read now runs for rejected requests; worth confirming none exists (and updating the requestSizeLimiter doc, which still frames the middleware as admission "before the body is buffered or decoded").
• The Cursor second-opinion file (cursor-review.md) is empty — that pass produced no output, so this review reflects only Claude's and Codex's findings.
• Perf: budgetBody.Read calls time.Now() and rc.SetReadDeadline on every Read, so a 5 MiB body read in 32 KiB chunks issues ~160 extra SetReadDeadline syscalls. Consider only re-arming when the remaining time on the previous deadline drops below some fraction of idleTimeout.
• 11 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] Build break. This hunk deletes rejectReasonBusy = "busy", but it is still referenced by unchanged code:

• evmrpc/metrics.go:194 — case rpc.WSAdmissionReasonBudgetWaitTimeout, rpc.WSAdmissionReasonFrameAdmissionTimeout: return rejectReasonBusy
• evmrpc/metrics_test.go:18-19 — the mapping test table

grep -rn '"busy"' --include=*.go . returns nothing, so there is no other declaration: package evmrpc fails to compile with undefined: rejectReasonBusy.

The WS admission path still needs a "budget exhausted" reason. Either keep rejectReasonBusy for WS (HTTP moves to budget_midread, WS stays busy), or pick a new value and update mapWSAdmissionRejectReason, its doc comment ("the same oversize/busy vocabulary HTTP uses" is now stale either way), and metrics_test.go.

[seidroid]

[suggestion] Batching at 64 KiB means each in-flight request can hold up to budgetAcquireBatch - 1 bytes that are never charged: charge() only calls TryAcquire once unbilled >= 64 KiB, and the remainder is only settled by flush() at EOF/Close().

Two consequences worth quantifying in the comment (and possibly bounding):

1. With the defaults (max_concurrent_request_bytes = 128 MiB, max_open_connections = 2000), worst-case concurrently buffered body bytes is ~128 MiB (budget) + 2000 × 64 KiB (~128 MiB uncharged) ≈ 2× the configured bound. Previously the Content-Length reservation made the budget a hard ceiling.
2. Because virtually all real JSON-RPC bodies are < 64 KiB, the common case now reads and buffers the entire body before consulting the budget at all — the 429 arrives after the memory was allocated, not before. The type doc above still claims the middleware "bounds peak decode-time memory"; that only holds for bodies well above one batch now.

This is a deliberate trade (and the idle timeout mitigates the stalled case), but a smaller first batch — charge the first chunk immediately, then switch to 64 KiB steps — would keep the contention win while restoring a tight bound. Same point as Codex's high-severity finding.

[seidroid]

[suggestion] b.inner is dereferenced unguarded, but fail() sets b.inner = nil (line 250-251). Any body consumer that calls Read again after a returned error — legal for io.Reader, and something bufio/decoder wrappers or future middleware can do — panics with a nil-interface method call inside the HTTP handler (net/http recovers per-connection, killing the connection and logging a stack).

Current in-tree consumers (io.ReadAll in seiLegacyHTTPGate, json.Decoder) all stop on first error so this isn't live today, but the guard is one line:

if b.inner == nil {
    return 0, b.stickyErr // or io.ErrClosedPipe
}

[seidroid]

[suggestion] absDeadline is computed at handler entry, but net/http's whole-request read deadline was set to t0 + ReadTimeout where t0 is when the server started reading the request. Handler entry is t0 + header-read time + JWT time, so rc.SetReadDeadline in Read overwrites net/http's deadline with a strictly later one.

An attacker who trickles request headers for nearly ReadHeaderTimeout then trickles the body gets an effective window of roughly ReadHeaderTimeout + ReadTimeout instead of ReadTimeout. Net effect is still a large improvement over no idle guard, but the comment on absDeadline claims it "can never extend past the server's own absolute http.Server.ReadTimeout", which isn't quite true. Either subtract a conservative allowance (e.g. ReadHeaderTimeout) or soften the comment to say it's an approximation of the server deadline. Matches Codex's medium finding.

[seidroid]

[suggestion] captureResponseWriter suppresses WriteHeader/Write but passes Header() straight through to the real writer, so header mutations made by the inner chain before suppression survive into this http.Error.

Concretely: gzipResponseWriter.init() (rpcstack.go:562-566) does hdr.Del("content-length") + hdr.Set("content-encoding", "gzip") on the shared map. If the gzip wrapper ever runs before the outcome is set, the plain-text server busy / request timeout body goes out with Content-Encoding: gzip, and any client that sent Accept-Encoding: gzip fails to decode the 429/408 body.

Today SeiLegacyAllowlist is always non-nil (BuildSeiLegacyEnabledSet returns make(...)) and the gate buffers the body before dispatching, so the gzip wrapper never sees budgetBody — this isn't reachable in the production stack. But it is reachable if the gate is ever bypassed (wrapSeiLegacyHTTP returns inner unchanged for a nil allowlist), which is exactly the fragility this PR is hardening. Cheap fix: clear the header map (for k := range w.Header() { delete(...) }) before http.Error on the outcome path.

[seidroid]

[suggestion] This appends a second description of MaxConcurrentRequestBytes rather than updating the existing one, leaving the doc self-contradictory: lines 267-268 still say "HTTP uses Content-Length weighting and rejects over-budget requests fast (HTTP 429)", which is exactly the behavior this PR removes. The new paragraph also drops the WS half (independent budget, 2× process-wide peak, -32005 on timeout), so neither block is complete on its own.

Edit the original paragraph in place: keep the HTTP/WS split and the 2× note, and replace only the "Content-Length weighting" sentence with the incremental-charging description.

[seidroid]

[suggestion] Same duplication in the app.toml template, which is operator-facing: max_concurrent_request_bytes now carries two overlapping comment paragraphs, and the first still claims HTTP "rejects over-budget requests fast" up front. Merge into one paragraph so operators aren't left guessing which description applies.

[seidroid]

[nit] No range validation: a negative evm.body_read_idle_timeout casts fine and then silently disables the idle guard (l.bodyReadIdleTimeout > 0 is false), losing the slowloris protection this PR adds with no warning. Neighbouring keys in this function reject out-of-range values explicitly (e.g. max_open_connections errors with "must be >= 0 (0 disables the limit)"). Either error on < 0 or document that negative means disabled, as ws_admission_timeout does.

[seidroid]

[suggestion] This comment doesn't match what the code does, and the assertion doesn't test the fix. With budgetAcquireBatch = 64 KiB and maxBody = 1000, each staller reads 32 bytes → unbilled = 32, never reaching a batch, so stallers hold zero charged bytes, not 32. More importantly, budget = 1500 and the 2-byte request needs 2 bytes, so this passes under any charging scheme that isn't Content-Length-based — it doesn't isolate incremental charging.

The PR's core security claim — a stalled multi-MiB body pins at most one 64 KiB batch — has no test. A version with maxBody = 1 MiB, budget = 2 MiB, stallers trickling ~100 KiB each and then stalling, followed by a 1 MiB request that must still be admitted, would actually fail on the old design and pass on the new one.

[seidroid]

[nit] Both new benchmarks pass 10*time.Second for bodyReadIdleTimeout but drive the handler with httptest.NewRecorder(), for which http.ResponseController.SetReadDeadline returns ErrNotSupported. The per-Read deadline arming — the main new cost on the hot path — is therefore never measured, so these numbers understate the idle-guard overhead. Either benchmark through httptest.NewServer or pass 0 and note the idle path isn't covered.

[claude]

Beyond the inline findings, I also checked seidroid's Flush()-suppression concern in captureResponseWriter: in the always-present production chain (limiter → seiLegacyHTTPGate → ...), every body read — gated or not — goes through the gate's own single io.ReadAll, whose error path calls http.Error (not Flush), and that write is already correctly suppressed by cw.WriteHeader's suppressed() check. So the described 200-leak path isn't reachable through the standard stack.

Extended reasoning...

seidroid flagged that captureResponseWriter.Flush() doesn't check suppressed(), which could in theory let an inner handler commit an implicit 200 to the wire via http.Flusher.Flush() after a mid-read budget/timeout failure, ahead of the limiter's own 429/408. I traced the actual request path: wrapSeiLegacyHTTP is always wired in production (BuildSeiLegacyEnabledSet never returns nil, even with zero legacy APIs enabled), and its ServeHTTP buffers the whole body with one io.ReadAll before dispatching to any inner handler; on a read error from that call it responds via http.Error, not Flush, and that write is already suppressed by cw.WriteHeader's suppressed() check. So for the composed production stack this specific leak path isn't reachable, though captureResponseWriter.Flush() itself remains unguarded in isolation.

[claude]

🟡 The PR appends a new doc paragraph for MaxConcurrentRequestBytes instead of replacing the old one, so the struct comment (and the rendered app.toml via ConfigTemplate) now shows two back-to-back paragraphs: the original still claims HTTP uses Content-Length weighting and a 2x HTTP/WS budget split, while the new one correctly describes incremental mid-read charging for HTTP.

Extended reasoning...

What's wrong. The diff to evmrpc/config/config.go inserts a second doc block directly above MaxConcurrentRequestBytes (the new paragraph starting "MaxConcurrentRequestBytes bounds the total size... charged incrementally as body bytes are read... rejected mid-read (HTTP 429)") without deleting the pre-existing paragraph immediately above it. That original paragraph still asserts "HTTP uses Content-Length weighting and rejects over-budget requests fast (HTTP 429)" and describes an HTTP+WS 2x budget split. These two blocks now directly contradict each other on how HTTP admission works.

Why it's stale. The entire point of this PR is to stop trusting Content-Length and charge the HTTP budget incrementally as bytes are read (see the new budgetBody type in request_limiter.go, which charges in 64 KiB batches via charge()/flush() rather than reserving r.ContentLength up front). So the "Content-Length weighting" sentence in the first paragraph is now factually wrong for HTTP — the code it describes no longer exists. The WS-related content in that same paragraph (the 2x budget / independent WS+HTTP semaphores) is still accurate, since server.go still wires SetWSConcurrentRequestBytes independently, so a full rewrite has to preserve that context rather than just deleting the whole block.

Same duplication reaches app.toml. ConfigTemplate (lines ~970-979 in the diff) mirrors the struct doc verbatim: the pre-existing "max_concurrent_request_bytes bounds total request bytes admitted concurrently... HTTP rejects over-budget requests fast" block is immediately followed by the newly appended "charged incrementally as body bytes are read... rejected mid-read" block, both commenting the same max_concurrent_request_bytes = ... line. Any operator who runs seid init or regenerates app.toml will see these two contradictory paragraphs stacked on top of each other.

Why nothing catches this today. This is a doc comment only — there's no test or lint rule that checks doc-comment prose against implementation behavior, so go build/go test/golangci-lint all pass cleanly despite the contradiction.

Proof walkthrough.

1. Before this PR: MaxConcurrentRequestBytes doc says HTTP uses r.ContentLength as the weight and rejects fast if it exceeds the budget.
2. This PR's diff to request_limiter.go removes exactly that mechanism — requestSizeLimiter.ServeHTTP no longer reads r.ContentLength as a weight; instead budgetBody.Read charges the semaphore in 64 KiB increments as the body is actually read, and budgetBody.flush() charges the remainder on EOF.
3. The PR's diff to config.go adds a new paragraph describing this new incremental behavior correctly, but leaves the old paragraph (with the now-false Content-Length claim) in place directly above it.
4. Result: both the Go doc comment and the generated app.toml show two consecutive paragraphs over the same field, one of which is simply wrong post-merge.

Fix. Fold the new incremental-charging language into the existing comment (in both the struct field doc and ConfigTemplate), dropping only the stale "Content-Length weighting ... fast" sentence for HTTP while keeping the still-accurate WS/2x-budget context.

This is documentation-only — nothing breaks at build, test, or runtime — so it shouldn't block merge, but it should be cleaned up since it leaves operators reading two contradictory explanations of the same config key.

[claude]

🟡 body_read_idle_timeout is read via cast.ToDurationE with no range check, unlike the immediately preceding max_open_connections block which rejects negative values. A negative body_read_idle_timeout casts fine and silently disables the per-chunk idle guard (request_limiter.go only arms it when bodyReadIdleTimeout > 0), contradicting the field doc which only documents zero as the disabling value. Fix by rejecting values < 0 the same way max_open_connections does, or by documenting that negative behaves the same as zero.

Extended reasoning...

What the bug is. In ReadConfig (evmrpc/config/config.go:696-700), the flagBodyReadIdleTimeout branch only calls cast.ToDurationE(v) and stores the result — there is no sign check. This sits directly beneath the flagMaxOpenConnections block (lines 688-695), which does the extra work of validating cfg.MaxOpenConnections < 0 and returning an error naming the flag. The two fields are conceptually parallel (both are "0 disables X" knobs), so the asymmetry is a real inconsistency in how the two nearby blocks are written, not an intentional design choice.

How it manifests. BodyReadIdleTimeout casts a negative TOML/env/flag value (e.g. "-5s" or -5) into a negative time.Duration without error. That value flows into requestSizeLimiter via RPCEndpointConfig.bodyReadIdleTimeout (rpcstack.go / server.go), and budgetBody.Read in evmrpc/request_limiter.go only arms the per-chunk idle deadline when b.idleTimeout > 0. A negative value fails that check exactly like zero does, so the per-chunk idle guard is silently disabled — the request falls back to relying solely on http.Server.ReadTimeout as the backstop.

Why nothing else catches it. The field's own doc comment (config.go:282-285) states only "Zero disables the per-chunk idle guard," which implies that zero is a deliberate, documented sentinel and anything else (including negative) is expected to behave like a normal duration. There is no validation anywhere in the read path, the config characterization tests (config_test.go, config_fuzz_test.go), or the golden file that would catch an operator setting a negative value — the fuzz suite for this key only checks that the cast itself doesn't error, not that the resulting value is sane.

Impact. This is not a crash or a security hole: a negative value produces exactly the same outcome as the documented "0 disables" behavior, and http.Server.ReadTimeout remains in place as the whole-request backstop regardless. It's best characterized as a validation/documentation gap — an operator who mistypes a negative duration gets silent unexpected behavior (the idle guard vanishes) instead of a clear startup error, but nothing downstream misbehaves or corrupts state.

Step-by-step proof:

1. Set evm.body_read_idle_timeout = "-5s" in app.toml (or equivalently pass -5 * time.Second through AppOptions).
2. ReadConfig reaches the flagBodyReadIdleTimeout branch, calls cast.ToDurationE("-5s"), which succeeds and returns cfg.BodyReadIdleTimeout = -5 * time.Second. No error is returned, unlike what happens one block earlier for evm.max_open_connections = -1.
3. NewEVMHTTPServer passes this value through to RPCEndpointConfig.bodyReadIdleTimeout, which HTTPServer.EnableRPC forwards into newRequestSizeLimiter(...).
4. On every request, requestSizeLimiter.ServeHTTP wraps r.Body in a budgetBody only checking l.bodyReadIdleTimeout > 0 to decide whether idle enforcement applies; with -5s that condition is false, so budgetBody.Read never calls SetReadDeadline for idle purposes.
5. The operator believes they have a 5-second-ish idle guard (or just a typo of the sign) but in fact get the same behavior as body_read_idle_timeout = 0, i.e. no per-chunk stall protection beyond the overall ReadTimeout.

Suggested fix. Mirror the flagMaxOpenConnections pattern: after the cast, add if cfg.BodyReadIdleTimeout < 0 { return cfg, fmt.Errorf(...) }. Alternatively, if silently treating negative as "disabled" is acceptable, update the doc comment to say so explicitly instead of only documenting zero.

This exact issue was independently flagged as a nit by seidroid[bot] in the PR timeline, and by all three verifying agents, all of whom agreed it is real but not merge-blocking.

[seidroid]

Solid, well-tested rework of the HTTP request-byte budget from up-front Content-Length reservation to incremental charging, plus a per-chunk idle guard and a JWT/limiter reorder. One blocker: the new BodyReadIdleTimeout row is recorded in the wrong position in evm.golden, which fails the configtest characterization suite. Several accuracy/doc issues around the new accounting also need attention (two confirmed by Codex).

Findings: 2 blocking | 15 non-blocking | 10 posted inline

Blockers

• evmrpc/config/testdata/evm.golden records BodyReadIdleTimeout in the wrong position, so TestDefaultsMatchTheRecordedValues fails (details in the inline comment). Worth noting the failure will be hard to read: goldenDiff only prints lines whose key = value differ, so a pure reordering prints an empty diff under the "defaults no longer match" message.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• The Cursor review file (cursor-review.md) is empty — that second-opinion pass produced no output, so only Codex's two findings (both incorporated below) were available to merge.
• Metrics/alerting break: reason="busy" is no longer emitted on the HTTP plane at all (it is now WS-only, per the updated comment in evmrpc/metrics.go:29). Any existing dashboard or alert on evmrpc_requests_rejected_total{protocol="http",reason="busy"} will silently read zero after this ships. Worth a release note pointing operators at budget_midread / slow_body.
• NewHTTPHandlerStack's JwtSecret parameter is now always nil at its only in-tree call site (evmrpc/rpcstack.go:352), so the JWT branch inside it is dead for this repo. Consider dropping the parameter (or a comment noting it is retained for external callers) so the next reader doesn't assume both JWT wiring paths are live.
• budgetBody.fail takes readErr and discards it (_ = readErr), so the underlying cause of a 429/408 is never surfaced anywhere. Either log it at debug level or drop the parameter.
• budgetBody.Close ignores the flush() error (_ = b.flush()), so trailing sub-batch bytes on a partially-read-then-closed body are dropped from accounting rather than rejected. Probably the right call, but it's silent — worth one line of comment saying so explicitly.
• captureResponseWriter is now unconditionally in the HTTP chain and only forwards Flush + Unwrap; it drops http.Hijacker and io.ReaderFrom. Nothing in the current JSON-RPC HTTP stack needs them (WS upgrades are routed to wsHandler before this handler), but it's a constraint future middleware will trip over — worth a comment on the type.
• 9 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] This row is in the wrong position and will fail TestDefaultsMatchTheRecordedValues.

configtest.Dump walks struct fields in declaration order (testutil/configtest/dump.go:90 — for i := range v.NumField(), no sorting for structs), and CheckDefaults compares the whole joined dump against the golden with exact string equality (testutil/configtest/golden.go:143-145).

In Config the new field is declared before WSAdmissionTimeout:

MaxConcurrentRequestBytes  // config.go:276
BodyReadIdleTimeout        // config.go:282
WSAdmissionTimeout         // config.go:289
MaxOpenConnections         // config.go:295

so the dump emits BodyReadIdleTimeout on the line above WSAdmissionTimeout, while this file has the two swapped. Move BodyReadIdleTimeout = time.Duration(10s) up one line (or regenerate with go test ./evmrpc/config/ -update).

[seidroid]

[suggestion] Batching the charge means bytes below the batch boundary are never charged until EOF, so the budget's stated invariant ("bounds the total size of request bodies admitted concurrently") now holds only within ±budgetAcquireBatch per in-flight read. Two concrete consequences:

1. Any body smaller than 64 KiB is charged entirely post-hoc, in flush() at EOF — i.e. it is fully buffered in memory before the budget is consulted. TestRequestSizeLimiter_budgetExhaustionAndRelease demonstrates exactly this: the second 1000-byte request is read into memory in full and only then rejected.
2. Requests mid-read each hold up to budgetAcquireBatch - 1 uncharged bytes. With the shipped defaults (max_open_connections = 2000, max_concurrent_request_bytes = 128 MiB) worst-case unaccounted buffering is ~125 MiB — roughly doubling the effective concurrent-bytes ceiling the operator configured.

This is Codex's high-severity finding; I agree it's real but read it as a bounded design trade-off (the old code was strictly better on memory and strictly worse on slowloris) rather than a merge blocker. Two options worth considering: charge one batch up front on the first read (so every in-flight read holds ≥ one batch), and/or scale budgetAcquireBatch off the budget rather than hardcoding 64 KiB. At minimum, document the slop bound and its interaction with max_open_connections here and in the max_concurrent_request_bytes config comment, since that comment still promises a hard bound.

[seidroid]

[suggestion] absDeadline starts at handler entry, but http.Server.ReadTimeout starts when the connection is accepted — net/http sets wholeReqDeadline = t0 + ReadTimeout where t0 is before headers are read. Because SetReadDeadline below overwrites that deadline, the effective bound becomes headerReadTime + ReadTimeout. With rpc.DefaultHTTPTimeouts (ReadHeaderTimeout 30s, ReadTimeout 30s) a client that spends 30s dribbling headers then trickles the body just under idleTimeout gets ~60s, not the configured 30s.

That contradicts the doc claim on newRequestSizeLimiter ("can never extend past the server's own absolute http.Server.ReadTimeout") and on the absDeadline field. Codex flagged this too. Simplest correct-ish fix: subtract the elapsed header time you can observe (e.g. clamp with readTimeout - readHeaderTimeout when ReadHeaderTimeout > 0), or soften both comments to say the clamp is approximate and can exceed ReadTimeout by up to the header-read duration.

[seidroid]

[suggestion] captureResponseWriter suppresses the inner handler's status and body but not its header mutations — those went straight to the shared w.Header() map. http.Error here only sets Content-Type/X-Content-Type-Options and deletes Content-Length, so anything else the inner chain set survives onto the 429/408.

The reachable case is EnableRPC with a nil SeiLegacyAllowlist (no gate, so NewGzipHandler is the direct inner): geth's rpc handler sets headers and then reads the body, and on the failing read its error write reaches gzipResponseWriter.init(), which does hdr.Set("content-encoding", "gzip") on the real header map before WriteHeader is suppressed. Result: a 429 carrying Content-Encoding: gzip with an uncompressed plaintext body, which gzip-capable clients cannot decode.

Not reachable through NewEVMHTTPServer today, since BuildSeiLegacyEnabledSet always returns a non-nil map and the gate buffers the body before dispatching — so this is defensive. Still cheap to close: clear the header map (for k := range w.Header() { w.Header().Del(k) }) before http.Error, since the outcome response is unconditionally canonical.

[seidroid]

[suggestion] Gating on !cw.wroteHeader also gates the metric: if the inner chain wrote a header before the failing read, the mid-read rejection is neither reported to the client (correct — can't be) nor counted (not correct). Consider recording outcome.reason unconditionally when outcome.status != 0, and keeping only http.Error behind the wroteHeader check, so budget_midread / slow_body can't silently under-report.

[seidroid]

[suggestion] This new paragraph is appended to the comment block that already documents MaxConcurrentRequestBytes (lines 263-271), so the field now carries two overlapping descriptions — and the retained first one still says "HTTP uses Content-Length weighting", which this PR makes false. The new paragraph also drops the WebSocket half and the "2× this value process-wide" note, both still accurate and worth keeping.

Merge into one block: keep the HTTP/WS split and the 2× note, and replace "HTTP uses Content-Length weighting and rejects over-budget requests fast (HTTP 429)" with the incremental-charging description.

[seidroid]

[suggestion] Same duplication in the rendered app.toml: operators will see two consecutive # max_concurrent_request_bytes ... paragraphs, the first of which (lines 967-972) describes the old fast-reject behavior. Fold the incremental-charging wording into the existing block rather than appending a second one.

[seidroid]

[nit] No range validation, unlike the neighboring reads: a negative body_read_idle_timeout silently disables the idle guard (b.idleTimeout > 0 is false) instead of erroring the way max_open_connections and max_concurrent_request_bytes do. Suggest if cfg.BodyReadIdleTimeout < 0 { return cfg, fmt.Errorf("%s must be >= 0 (0 disables the guard), got %s", flagBodyReadIdleTimeout, cfg.BodyReadIdleTimeout) }, and adding a negative-value row to evmKeys/the fuzz targets so the choice is pinned either way.

[seidroid]

[nit] The comment overstates the guarantee: after reading 32 bytes each staller has unbilled = 32 and reserved = 0, so it holds zero budget, not 32 bytes — nothing is charged until a 64 KiB batch fills or EOF/Close flushes. The test's assertion is right; only the explanation is off. Rewording it to "stallers hold no charged budget yet (below the 64 KiB batch), not maxBody" would also make the accounting slop noted on budgetAcquireBatch visible to the next reader.

[seidroid]

[nit] require.* here runs on the httptest.Server handler goroutine, not the test goroutine. require calls t.FailNow() → runtime.Goexit(), which is documented as valid only from the test goroutine; on failure this kills the handler mid-response and the test reports a confusing timeout/ReadResponse error instead of the real assertion. Same pattern in TestRequestSizeLimiter_bodyReadIdleTimeout's handler.

Use assert.* inside the handler (it returns rather than Goexits) or stash the values in an atomic/channel and assert after Do returns. TestRequestSizeLimiter_incrementalBudgetOnLargeBody is fine — it calls ServeHTTP synchronously on the test goroutine.

[claude]

The compile-break I flagged earlier (rejectReasonBusy undefined) has been fixed by the latest commit. However, the config golden-file test is still broken and hasn't been addressed: go test ./evmrpc/config/... fails TestDefaultsMatchTheRecordedValues because evm.golden lists WSAdmissionTimeout before BodyReadIdleTimeout, while the Config struct declares BodyReadIdleTimeout first. This needs a one-line swap (or struct reorder + golden regen) before merge.

Extended reasoning...

I re-ran the evmrpc/config test suite against the current PR head and confirmed TestDefaultsMatchTheRecordedValues still fails with the field-order mismatch between the Config struct and evm.golden that was previously flagged inline on this PR — this has not been fixed even though the separate compile-break (rejectReasonBusy) was. This is a deterministic, reproducible test failure that blocks go test ./evmrpc/config/... and the corresponding CI job, so the PR is not mergeable as-is. No new code-review findings beyond this; I did not re-run the full bug-hunting sweep since the bug tracker for this run reported no new findings.

[seidroid]

Solid, well-documented hardening of the EVM HTTP request-size limiter: incremental budget charging, a per-chunk idle guard, JWT moved ahead of the byte budget, and decoupled budget release are all implemented coherently and covered by new tests. No blocking correctness or security defects found, but there is a residual sub-batch accounting hole (the budget can be over-admitted by ~max_open_connections × 64 KiB), a latent nil-body panic, and stale/duplicated max_concurrent_request_bytes documentation that this PR makes factually wrong.

Findings: 0 blocking | 17 non-blocking | 11 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion pass produced no output (cursor-review.md is empty), so only Codex's two findings were available to merge; both are addressed inline (the sub-64 KiB slack and the handler-entry absDeadline).
• Metrics contract change: HTTP over-budget rejections previously reported reason="busy" and now report budget_midread / slow_body (busy is now WS-only). Any dashboard/alert keyed on evmrpc_requests_rejected_total{protocol="http",reason="busy"} will silently go to zero — worth calling out in the release notes alongside the new body_read_idle_timeout key.
• Design shift worth recording: admission control is no longer pre-dispatch. The inner handler chain is now always entered and the 429/408 is only emitted after it fully returns, so the protection depends on every inner handler propagating body-read errors promptly (true today because seiLegacyHTTPGate buffers the body first). A note in evmrpc/AGENTS.md next to the new middleware-order section would keep future middleware from breaking that assumption.
• budgetBody's mutable state (reserved/unbilled/inner) and the shared limiterOutcome are unsynchronized: correctness relies on the body being read and closed only from the handler goroutine, and on outcome being read after ServeHTTP returns. That holds for the current stack; a one-line comment stating the assumption would make it enforceable in review.
• NewHTTPHandlerStack's JwtSecret parameter is now dead for the only in-tree caller (EnableRPC passes nil). Consider dropping the parameter or documenting that JWT is wired outside the stack so nobody re-introduces the inner JWT wrapper.
• I could not execute go build/go test in this review environment, so the assessment is static; the PR reports go test ./evmrpc/... and -race runs passing.
• 11 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] Batch charging only bills in whole 64 KiB steps, so up to budgetAcquireBatch - 1 bytes per in-flight request are never charged until EOF/Close(). A client that sends 65,535 bytes and then stalls holds that memory (buffered by seiLegacyHTTPGate's io.ReadAll) against a zero budget charge. With defaults (max_open_connections = 2000, 64 KiB batch) that is ~125 MiB of buffered request bytes on top of the 128 MiB max_concurrent_request_bytes budget, i.e. the configured memory bound can be roughly doubled — the guarantee the old Content-Length reservation did provide.

It is bounded (and body_read_idle_timeout caps how long each staller survives), so not a merge blocker, but consider charging the first partial batch as soon as any byte is read:

b.unbilled += n
for b.unbilled >= budgetAcquireBatch { ... }
if b.reserved == 0 && b.unbilled > 0 { // first bytes of this body
    if !b.budget.TryAcquire(b.unbilled) { return errBudgetExhausted }
    b.reserved, b.unbilled = b.unbilled, 0
}

That keeps the batching benefit for large uploads while making the residual slack per stalled connection O(one read) instead of O(64 KiB). If the current tradeoff is deliberate, please state the bound (max_open_connections × budgetAcquireBatch) in the type comment so operators can size the budget accordingly. (Merges Codex P1.)

[seidroid]

[suggestion] fail() and Close() both set b.inner = nil, but Read dereferences it unconditionally — a Read after a rejection or after Close() panics with a nil interface dereference instead of returning an error. net/http's own body returns http: invalid Read on closed Body here. Nothing in the current chain re-reads after an error (io.ReadAll and json.Decoder both stop), so this is latent rather than live, but a defensive middleware shouldn't turn a misbehaving inner handler into a panic. Suggest:

if b.inner == nil {
    return 0, errors.New("evmrpc: read on closed request body")
}

[seidroid]

[suggestion] When the inner handler already wrote a header before the body read failed (cw.wroteHeader == true), this branch is skipped entirely: the client keeps the inner status, all subsequent writes are silently dropped by captureResponseWriter (so the response body is truncated with no error indication), and recordRequestRejected is never called — so the new budget_midread / slow_body counters under-report exactly the awkward cases. Consider recording the metric unconditionally when outcome.status != 0 and only gating the http.Error on !cw.wroteHeader.

[seidroid]

[nit] Suppression covers Write/WriteHeader but not Header() (not overridden), so headers the inner handler already set survive into the limiter's http.Error. Most notably gzipResponseWriter.init() does hdr.Set("content-encoding", "gzip") on the real header map on its first write; if that runs before the outcome is set, the client receives a plain-text 429/408 body advertised as gzip. Not reachable in production today (the always-present seiLegacyHTTPGate fully buffers the body before dispatching to the gzip handler, so no inner write can precede the outcome), but clearing the header map on the suppressed path before http.Error would make the documented status/message self-contained:

for k := range w.Header() { delete(w.Header(), k) }

[seidroid]

[nit] absDeadline is time.Now() at handler entry, but net/http derives its ReadTimeout deadline from when it started reading the request (accept time). So the clamp sits later than the server's own deadline and each SetReadDeadline call can extend the effective overall read window by the header-read time (bounded by ReadHeaderTimeout, 30s by default here). Small and not exploitable for much, but if you want the clamp to be a true backstop, derive it from something request-scoped that predates header parsing (e.g. a ConnContext/BaseContext timestamp) or subtract ReadHeaderTimeout. (Codex P2 — agreed, with the magnitude bounded as above; note CheckTimeouts guarantees ReadTimeout >= 1s, so the clamp is always armed in practice.)

[seidroid]

[suggestion] Same duplication in the generated app.toml: every new node's config file will now carry two consecutive comment paragraphs for max_concurrent_request_bytes, the first of which ("HTTP rejects over-budget requests fast", implicitly Content-Length based, per the struct doc) is stale after this change. Merge into a single paragraph.

[seidroid]

[nit] A negative body_read_idle_timeout silently disables the idle guard (l.bodyReadIdleTimeout > 0 is false) with no warning. Neighboring reads are explicit about this: max_open_connections errors on negatives, and ws_admission_timeout documents that negatives fall back to the go-ethereum default. Either reject < 0 here or state "negative behaves as 0 (disabled)" in the struct/template comment so operators can't accidentally turn the new protection off.

[seidroid]

[suggestion] This assembles newJWTHandler(secret, limiter) by hand, which is the very wiring under test — it will keep passing if someone reverts EnableRPC to NewHTTPHandlerStack(..., config.JwtSecret) and drops the outer JWT wrapper. Since the ordering fix is one of the PR's headline security changes, consider driving it through EnableRPC (e.g. build an HTTPServer, EnableRPC with a JwtSecret, then assert an unauthenticated oversize/over-budget request gets 401 and the limiter's counters/inner handler stay untouched).

[seidroid]

[nit] require.* here runs on the httptest.Server handler goroutine, so a failure calls t.FailNow() off the test goroutine (documented as invalid; it can abort the wrong goroutine and mask the failure). Same pattern in TestRequestSizeLimiter_bodyReadIdleTimeout's handler. Prefer recording the observed body/error into a variable (or assert.*) and asserting after the response is read.

[seidroid]

[nit] These two idle-timeout tests are wall-clock sensitive: 50 ms idle + 200 ms wait here, and steadySlowUploadSucceeds writes 64-byte chunks every 50 ms against a 200 ms idle timeout for ~400 ms total. On a loaded CI runner (tests run with -race) a single >200 ms scheduling gap flips that test to a 408. Consider widening the idle timeout / shrinking the chunk interval ratio (e.g. idle 1s, chunk every 100 ms) to buy margin without lengthening the test much.

Superseded: latest AI review found no blocking issues.

[claude]

🟡 captureResponseWriter.Flush() forwards to the real ResponseWriter unconditionally, unlike WriteHeader/Write which both check suppressed() first. Since net/http's Flush() implicitly commits an empty HTTP 200 if no header was written yet, a suppressed inner-handler Flush() (e.g. go-ethereum's error-response encoder) can send a premature 200 OK before requestSizeLimiter's own http.Error(429/408) runs, silently downgrading the intended rejection status. This path isn't reachable through the shipped NewEVMHTTPServer wiring today (the sei-legacy gate always buffers the body first), but the fix is a one-line guard matching the sibling methods, so it's worth closing given this PR is specifically hardening this suppression mechanism.

Extended reasoning...

What the bug is. captureResponseWriter (request_limiter.go:270-303) is new code introduced by this PR to suppress the inner HTTP handler's response once the byte-limiter's own outcome (429/408) has been decided, so the limiter's canonical status/message always wins. WriteHeader and Write both correctly check w.suppressed() and no-op when it's true. Flush(), however, forwards straight to the real http.ResponseWriter's Flush() unconditionally, with no suppression check at all.

How it manifests. Go's net/http (*response).Flush() implicitly calls WriteHeader(http.StatusOK) if no header has been written yet on that connection. So if the inner handler chain calls Flush() on a suppressed captureResponseWriter before the limiter gets to run its own http.Error(w, outcome.message, outcome.status), the real writer commits an empty 200 OK first. The limiter's subsequent http.Error call is then a "superfluous WriteHeader" no-op — the client receives HTTP 200 with an empty body instead of the intended 429 Too Many Requests / 408 Request Timeout.

The triggering code path. In the vendored go-ethereum fork (rpc/http.go's newHTTPServerConn), the JSON-RPC codec's error encoder does w.Write(encdata) followed by if f, ok := w.(http.Flusher); ok { f.Flush() } whenever it writes an isErrorResponse=true response. A mid-decode body-read failure (e.g. budgetBody hitting errBudgetExhausted or an idle-timeout) causes rpc/server.go's serveSingleRequest to treat the read failure as a decode error and route through exactly that encoder. w.Write on captureResponseWriter correctly no-ops once outcome is set, but the encoder's subsequent f.Flush() call reaches captureResponseWriter.Flush(), which is unguarded and commits the premature 200.

Why nothing else prevents this. The suppression mechanism this PR adds is specifically meant to guarantee "the documented status/message always reaches the client" (per the PR description itself). WriteHeader/Write enforce that invariant; Flush is simply missing the same one-line check, so the invariant has a gap for any inner-handler code path that flushes instead of (or in addition to) writing.

Reachability caveat. This requires the byte-read failure to occur inside the JSON-RPC codec's own body read, which in the current production wiring (NewEVMHTTPServer) doesn't happen: BuildSeiLegacyEnabledSet always returns a non-nil allowlist, so wrapSeiLegacyHTTP always installs seiLegacyHTTPGate, which buffers the entire body via io.ReadAll and replaces r.Body with a buffered copy before dispatching to the inner rpc codec — so the codec never reads budgetBody directly and never observes a stalled/budget-exhausted read. The path is only reachable if SeiLegacyAllowlist is nil (gate bypassed), which is a documented, supported code path but not exercised by the current callers. This mirrors the same defensive-only reachability already noted for the sibling gzip Content-Encoding header-leak finding on Header(), but the mechanism and impact are distinct: this one corrupts the response status/body via Flush(), not the encoding header via Header().

Step-by-step proof.

1. Configure NewEVMHTTPServer (hypothetically) with SeiLegacyAllowlist = nil, so wrapSeiLegacyHTTP returns the inner gzip → vhost → cors → rpc.Server chain unwrapped.
2. A client sends a request with a declared Content-Length but stalls or exceeds the byte budget mid-body.
3. budgetBody.Read hits errBudgetExhausted (or an idle-timeout), calls b.fail(...), which sets outcome.status = 429 (or 408) and returns the error up through io.Reader.
4. go-ethereum's jsonCodec.readBatch → serveSingleRequest treats this as a decode/read error and calls codec.writeJSON(ctx, resp, true) (isErrorResponse=true).
5. The error encoder in rpc/http.go calls w.Write(encdata) — correctly suppressed by captureResponseWriter.Write, no bytes reach the real writer, wroteHeader stays false — then calls f.Flush().
6. captureResponseWriter.Flush() forwards unconditionally to the real net/http ResponseWriter.Flush(), which implicitly calls WriteHeader(200) since nothing was written yet.
7. Back in requestSizeLimiter.ServeHTTP, outcome.status != 0 && !cw.wroteHeader is true (the flag is only set inside WriteHeader/Write, not Flush), so it calls http.Error(w, outcome.message, outcome.status) — but the real writer already sent 200, so this is a no-op "superfluous WriteHeader" call logged by net/http, and the client has already received an empty 200 OK.

Fix. Add the same suppressed() guard to Flush() that WriteHeader/Write already have:

func (w *captureResponseWriter) Flush() {
    if w.suppressed() {
        return
    }
    if f, ok := w.ResponseWriter.(http.Flusher); ok {
        f.Flush()
    }
}

This is a one-line, low-risk change that closes the gap in the suppression invariant this PR introduces.

[claude]

🟡 In requestSizeLimiter.ServeHTTP, recordRequestRejected(outcome.reason) is gated behind !cw.wroteHeader, so if the inner handler already wrote a header/body before a later mid-read body failure (budget exhausted or idle timeout), the metric increment is silently skipped along with the (correctly suppressed) duplicate HTTP response. This means evmrpc_requests_rejected_total{reason=budget_midread|slow_body} can undercount for any handler that writes output before finishing its body read. It's currently unreachable in the production stack (seiLegacyHTTPGate always buffers the whole body before writing anything), so this is metrics-only and latent rather than functionally broken today; the cheap fix is to record the reason unconditionally when outcome.status != 0 and gate only http.Error on !cw.wroteHeader.

Extended reasoning...

What the bug is. request_limiter.go's requestSizeLimiter.ServeHTTP ends with:

if outcome.status != 0 && !cw.wroteHeader {
    recordRequestRejected(r.Context(), outcome.reason)
    http.Error(w, outcome.message, outcome.status)
}

Both the metric increment and the client-facing http.Error are gated behind the same condition, !cw.wroteHeader. cw.wroteHeader is set to true only when the inner handler chain calls WriteHeader/Write on the captureResponseWriter before outcome.status has been set (i.e. before suppressed() returns true). If a body-read failure (budget exhaustion via budgetBody.charge/flush, or an idle-timeout via isReadIdleTimeout) happens after the inner handler has already written a header/body, cw.wroteHeader is already true, so the whole branch is skipped — not just the duplicate HTTP response (correct: a response is already in flight and can't be overwritten) but also recordRequestRejected, which has no principled reason to be suppressed.

Why suppressing the metric is wrong even though suppressing the response is right. The wroteHeader guard exists to prevent http.Error from calling WriteHeader a second time on a connection that already committed a response — that's a real constraint of net/http. But recordRequestRejected is just an OpenTelemetry counter increment; it has no such constraint. Tying it to the same condition as the client-response suppression conflates two independent concerns and makes the metric's accuracy a function of how far along the client response was, rather than whether a rejection actually occurred.

Step-by-step proof of the accounting gap (this is a code-property proof; see the reachability caveat below for why it doesn't fire today):

1. A hypothetical (or future) inner handler writes a response header via w.WriteHeader(200) before it has fully consumed r.Body. At this point captureResponseWriter.WriteHeader runs with suppressed() == false (no outcome yet), so it forwards the call and sets cw.wroteHeader = true.
2. The handler then continues reading the body. budgetBody.Read calls charge(), which fails because the global max_concurrent_request_bytes semaphore is exhausted, so budgetBody.fail(rejectReasonBudgetMidread, 429, "server busy", ...) sets outcome.status = 429.
3. The inner handler returns. Back in ServeHTTP, the check outcome.status != 0 && !cw.wroteHeader evaluates to 429 != 0 && !true → false.
4. Neither http.Error (correctly — the 200 header is already on the wire, so a 429 can't be sent) nor recordRequestRejected(r.Context(), "budget_midread") (incorrectly) runs.
5. evmrpc_requests_rejected_total{reason="budget_midread"} is not incremented even though a genuine budget-exhaustion rejection occurred.

Reachability today. All verifiers who checked this agreed it is not reachable through the current production middleware stack: seiLegacyHTTPGate (always present, since BuildSeiLegacyEnabledSet never returns nil) does io.ReadAll on the entire body before writing anything, and geth's rpc.Server likewise decodes the full body before writing a response. So in practice, a mid-read budget/idle failure always occurs before cw.wroteHeader can become true, and the metric is recorded correctly today. One verifier refuted the finding on exactly this basis. I'm not treating that as a reason to abstain, though: the code-level coupling between an intentional response-suppression guard and an unrelated metrics call is real and independently confirmed by three verifiers and by seidroid's own inline review comment on this line ("Gating on !cw.wroteHeader also gates the metric... the mid-read rejection is neither reported to the client (correct) nor counted (not correct)") — it's a latent defect that would silently break observability the moment any handler in the chain (including a future streaming or gzip-first handler) writes before finishing its body read, with no test or lint that would catch the regression.

Fix. Split the two concerns: record outcome.reason unconditionally whenever outcome.status != 0, and keep only the http.Error call behind !cw.wroteHeader:

if outcome.status != 0 {
    recordRequestRejected(r.Context(), outcome.reason)
    if !cw.wroteHeader {
        http.Error(w, outcome.message, outcome.status)
    }
}

This is a one-line reorder with no behavioral change to the client-visible response, and it decouples the observability signal from the response-suppression mechanics.

[claude]

🟡 Incremental 64 KiB batch charging in budgetBody.charge() means bytes below the batch threshold sit uncharged until flush() at EOF/Close, so every in-flight request can hold up to ~64 KiB unaccounted. With shipped defaults (max_open_connections=2000), worst-case unaccounted buffering is ~125 MiB on top of the 128 MiB charged budget — roughly 2x the operator-configured bound — and bodies under 64 KiB (the common JSON-RPC case) are fully buffered before the budget is consulted at all. This is a deliberate, documented trade-off for closing the slowloris gap, so not a blocker, but worth quantifying/tightening the config doc comment, which still promises a hard total-bytes bound.

Extended reasoning...

budgetBody.charge() (request_limiter.go:210-220) only calls budget.TryAcquire once b.unbilled reaches budgetAcquireBatch (64 KiB); everything below that threshold accumulates in b.unbilled uncharged until flush() runs at EOF (the io.EOF branch in Read) or in Close(). This means every concurrently in-flight request body can hold up to budgetAcquireBatch - 1 (~64 KiB) bytes that the shared semaphore never accounts for.

The concrete consequence: with the shipped defaults (max_open_connections = 2000, max_concurrent_request_bytes = 128 MiB), a worst case of ~2000 connections each trickling just under 64 KiB before stalling can hold ~125 MiB of body bytes in the Go heap that the semaphore reports as completely free — on top of whatever the charged 128 MiB budget itself already holds. That's roughly double the peak-memory bound the operator configured via max_concurrent_request_bytes, and it directly contradicts both the field's doc comment ("bounds the total size...admitted for processing concurrently") and the requestSizeLimiter type doc ("bounds peak decode-time memory"). A related consequence is that any body under 64 KiB — the overwhelming majority of real JSON-RPC requests — is charged entirely post-hoc in flush() at EOF, meaning it's fully read into memory before the budget is consulted at all; a 429 (if one occurs) arrives after the allocation, not before, for the common case.

I want to be upfront that this is very likely an intentional, documented trade-off rather than an oversight: budgetAcquireBatch's own comment states it bounds "what a slow/stalled body pins (at most one batch, not the declared Content-Length)," and the requestSizeLimiter type doc explicitly says bytes are "charged in batches as body bytes arrive" with "stalled uploads hold at most one batch." Charging every byte immediately would eliminate the slop but would reintroduce the exact per-read semaphore contention this PR's batching is designed to avoid — that's the whole point of switching from a hard Content-Length reservation to incremental charging in the first place. The worst-case overcommit is also bounded (roughly 2x, not unbounded), and the pre-existing worst-case peak memory bound (max_open_connections * maxBody, via http.MaxBytesReader and io.ReadAll in seiLegacyHTTPGate) was already far larger than either the 128 MiB or 256 MiB figures here, so this doesn't introduce a new order-of-magnitude memory risk.

Proof walkthrough: (1) two connections stall after each has read exactly 32 bytes from a body — unbilled = 32 on each, reserved = 0, so the semaphore shows zero bytes charged even though 64 bytes are sitting in the Go heap; this is exactly what TestRequestSizeLimiter_slowlorisDoesNotPinDeclaredSize exercises, and it's also what the reworded test comment in request_limiter_test.go:242 (per seidroid's review) should say instead of claiming stallers hold 32 bytes of charged budget. (2) Scale that same trickle-then-stall pattern to 2000 concurrent connections just under the 64 KiB batch boundary: ~125 MiB is buffered in memory with the budget semaphore showing that much less usage. (3) A normal 2 KB JSON-RPC POST is read to io.EOF in one Read call; charge() never fires because unbilled never reaches 64 KiB, so the entire 2 KB is buffered by io.ReadAll in the inner handler, and only flush() in the EOF branch consults the budget — after the memory is already allocated.

Given the deliberate trade-off and bounded impact, this doesn't need to block merge, but it's worth either (a) documenting the slop bound explicitly in the budgetAcquireBatch comment and the max_concurrent_request_bytes config doc (which currently still promises a harder bound than the code delivers), or (b) charging a small first batch immediately on the first read so every in-flight request holds at least some charged bytes, tightening the bound without reintroducing full per-byte contention.

[claude]

🟡 budgetBody.fail() sets b.inner = nil on budget-exhaustion or idle-timeout, but Read() has no nil guard afterward (unlike Close(), which does). A second Read() call after a returned error — legal per the io.Reader contract — will panic with a nil-interface method call inside the HTTP handler goroutine. Not reachable through today's callers (io.ReadAll, json.Decoder both stop on first error), so this is a latent robustness gap rather than an active exploit; a one-line nil guard mirroring Close() closes it.

Extended reasoning...

budgetBody (evmrpc/request_limiter.go) wraps r.Body to charge the global byte budget incrementally and enforce the per-chunk idle timeout. On the budget-exhaustion path (mid-read charge() failure or flush() failure at EOF) and on the idle-timeout path, Read() calls fail(), which closes the inner reader and unconditionally sets b.inner = nil (request_limiter.go, fail()). Read() itself has no nil check before dereferencing b.inner: n, err := b.inner.Read(p) runs unconditionally at the top of every call. Close() was written defensively (if b.inner == nil { return nil }) but that guard was never mirrored in Read().

The io.Reader contract explicitly permits callers to invoke Read again after a previous call returned a non-EOF error — many bufio readers, io.Copy-style helpers, and generic decoding/retry wrappers do exactly this. If any such caller (now or in a future refactor) calls budgetBody.Read a second time after a fail()-triggered error, the method call on b.inner (now a nil io.ReadCloser interface) panics with a nil pointer dereference inside the HTTP handler goroutine. net/http recovers panics per-connection, so this would abort that one connection (with a logged stack trace) rather than crash the process, but it's still an unhandled panic caused by new code in this PR.

Today's actual body consumers do not trigger it: io.ReadAll in seiLegacyHTTPGate and the JSON-RPC json.Decoder both stop reading on the first returned error, so no in-tree caller currently issues that second Read. All three verifiers who examined this independently converged on the same conclusion: real defect, currently unreachable, one-line fix. This finding was also independently flagged by seidroid's inline review comment on this same line.

Step-by-step proof:

1. Suppose max_concurrent_request_bytes is nearly exhausted and a client sends a body ≥ 64 KiB (crossing a budgetAcquireBatch boundary).
2. budgetBody.Read reads a chunk, calls charge(), which calls budget.TryAcquire(64*1024) — this fails because the budget is exhausted.
3. Read calls b.fail(rejectReasonBudgetMidread, ...), which releases any reserved bytes, closes and nils out b.inner, and records the 429 outcome, then returns (n, errBudgetExhausted).
4. A hypothetical (or future) caller — e.g., a bufio.Reader-wrapped consumer, or a retry-on-error decoder — calls budgetBody.Read again believing the stream might still yield more data or wanting to confirm EOF.
5. This second call runs n, err := b.inner.Read(p) with b.inner == nil, which is a method call on a nil interface value and panics with a nil pointer dereference, killing the HTTP connection from within net/http's handler goroutine.

Fix: add the same guard already has, e.g. if b.inner == nil { return 0, io.ErrClosedPipe } (or a dedicated sticky error) at the top of Read.

All independent verifiers rated this nit: it's a genuine latent defect in newly-added code, matching the asymmetry between Read and Close, but no current in-tree caller triggers it, so it doesn't cause a concrete failure today.

_{🔬 also observed by seidroid}