Skip to content

Bound frozen RPC router batch allocations - #4038

Open
masih wants to merge 1 commit into
mainfrom
masih/forzen-aloc-not
Open

Bound frozen RPC router batch allocations#4038
masih wants to merge 1 commit into
mainfrom
masih/forzen-aloc-not

Conversation

@masih

@masih masih commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Limit JSON-RPC batches to 1,000 calls by default and reject oversized
batches during streaming decode with one bounded error response. Add a
configurable 30-second HTTP write timeout to release slow-client handlers.

Fixes PLT-1085

Limit JSON-RPC batches to 1,000 calls by default and reject oversized
batches during streaming decode with one bounded error response. Add a
configurable 30-second HTTP write timeout to release slow-client handlers.
@masih
masih marked this pull request as ready for review August 27, 2026 17:31
@cursor

cursor Bot commented Aug 27, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Edge hardening in the RPC router only; may reject or time out abusive or very slow clients, with tunable flags.

Overview
Adds resource limits to frozen-rpc-router: JSON-RPC batches default to 1,000 calls (--batch-request-limit) and HTTP responses get a 30s write deadline (--write-timeout on the server).

Batch handling no longer unmarshals the whole array up front. decodeBatchCalls streams the JSON array, stops once the limit is exceeded, and returns a single -32600 / batch too large error instead of proxying or splitting an oversized batch.

README and config/router tests cover defaults, flag validation, and the oversized-batch rejection path.

Reviewed by Cursor Bugbot for commit ea3a257. Bugbot is set up for automated code reviews on this repo. Configure here.

@masih masih added backport release/v6.6 Backport to release v6.6 backport release/v6.7 Backport to release v6.7 labels Aug 27, 2026
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 59.09091% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.25%. Comparing base (45bad90) to head (ea3a257).

Files with missing lines Patch % Lines
cmd/frozen-rpc-router/router.go 55.55% 8 Missing and 8 partials ⚠️
cmd/frozen-rpc-router/main.go 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4038      +/-   ##
==========================================
- Coverage   61.24%   60.25%   -1.00%     
==========================================
  Files        2153     2054      -99     
  Lines      188393   176741   -11652     
==========================================
- Hits       115387   106495    -8892     
+ Misses      62274    60479    -1795     
+ Partials    10732     9767     -965     
Flag Coverage Δ
sei-chain-pr 62.12% <59.09%> (?)
sei-db 69.80% <ø> (ø)
sei-db-state-db ?

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

Files with missing lines Coverage Δ
cmd/frozen-rpc-router/config.go 83.33% <100.00%> (+2.38%) ⬆️
cmd/frozen-rpc-router/main.go 0.00% <0.00%> (ø)
cmd/frozen-rpc-router/router.go 65.40% <55.55%> (+1.18%) ⬆️

... and 99 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.

@github-actions

Copy link
Copy Markdown

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 27, 2026, 5:38 PM

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The batch-limit change is a clean, behavior-preserving rewrite of serveBatch's decode path with good unit coverage, and config validation/plumbing is consistent. The only concern is the new mandatory 30s http.Server.WriteTimeout, which caps total handler time (not just write stalls) for a router whose workload is historical/archive queries.

Findings: 0 blocking | 1 non-blocking | 1 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • None at the file/PR level.
  • 1 suggestion(s)/nit(s) flagged inline on specific lines.

Addr: cfg.listenAddress,
Handler: router,
ReadHeaderTimeout: 10 * time.Second,
WriteTimeout: cfg.writeTimeout,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] http.Server.WriteTimeout is not a write-stall timeout: the deadline is armed when the request headers are read, so it bounds handler execution plus response write. For this router that means any single request whose upstream takes longer than 30s — debug_traceBlockByNumber, debug_getRawReceipts, a wide eth_getLogs, or a large mixed batch fanned out across frozen nodes — has its connection torn down mid-response, even though the client is fast and the upstream would have answered. That is exactly the archive-style workload this router fronts, and there is currently no upstream http.Client timeout to keep such calls under the budget.

Two things worth reconsidering:

  1. parseConfig rejects --write-timeout 0, so an operator cannot restore the previous (unbounded) behavior. Allowing 0 to mean "no write timeout" — matching http.Server's own convention — would keep the safe default while leaving an escape hatch. If a mandatory timeout is intended, that is worth stating explicitly, since it is a behavior change for existing deployments.
  2. The README's "HTTP response writes time out after 30 seconds" reads as a write-only deadline and will mislead operators tuning it; wording it as a total per-request deadline would set the right expectation.

For what it's worth, this does not affect the documented WebSocket passthrough: net/http's (*conn).hijackLocked calls rwc.SetDeadline(time.Time{}) before returning the connection, so ReverseProxy's upgrade path is left with no deadline. Only non-hijacked HTTP requests are affected.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ea3a257. Configure here.

Addr: cfg.listenAddress,
Handler: router,
ReadHeaderTimeout: 10 * time.Second,
WriteTimeout: cfg.writeTimeout,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Write timeout drops WebSocket connections

High Severity

http.Server.WriteTimeout is an absolute write deadline on the TCP connection, not an idle-write timeout. Non-POST traffic, including WebSocket upgrades, is reverse-proxied via liveProxy, and httputil.ReverseProxy hijacks without clearing that deadline. Default 30s therefore tears down live-node subscriptions. --write-timeout also rejects 0, so the deadline cannot be disabled.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ea3a257. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport release/v6.6 Backport to release v6.6 backport release/v6.7 Backport to release v6.7 non-app-hash-breaking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants