PLT-1125: Bound concurrent in-flight RPCs and connections per IP on the gRPC query plane - #4078
PLT-1125: Bound concurrent in-flight RPCs and connections per IP on the gRPC query plane#4078amir-deris wants to merge 4 commits into
Conversation
Per-IP admission on the gRPC query plane charges one token at the HTTP/2 HEADERS frame and records the admitted IP on the RPC context. A client can spend a token on headers alone, withhold the request body while the bucket refills, repeat across streams, and release every body at once. The token bucket smooths arrival rate; it caps no concurrency. Two orthogonal controls close that, each useful on its own. Per-IP connection cap. ConnLimitListener bounds simultaneously-open connections per client address on :9090 and :9091, wrapping the raw listener inside the global cap so a refused connection never spends a global slot. Over-cap connections are closed rather than surfaced as an Accept error, which would turn one client's excess into an outage for everyone else. It applies whether or not rate limiting is enabled. Per-IP in-flight RPC cap. RateLimitTapHandle takes a slot at the HEADERS frame, after the token check, and InFlightStatsHandler returns it on stats.End -- the only hook that brackets a stream the tap admitted, whatever ends it. An interceptor cannot: grpc-go reaches one only after decoding the request message, which is precisely the event a stockpiling client withholds. Two failure modes shape the implementation. grpc-go answers an unknown or malformed method name without emitting stats at all, so acquisition is gated on the registered method set: a leaked slot fails closed and locks an IP out permanently, which is worse than the burst. And gRPC-Web reaches the same handleStream through ServeHTTP and emits the same stats events without ever running the tap, so RateLimitHTTPMiddleware releases its own slot under a defer and leaves no context marker, keeping the stats handler from returning it twice. New keys, all read behind presence guards: [grpc] max-connections-per-ip, [grpc-web] max-connections-per-ip, and [grpc] max-in-flight-per-ip, each defaulting to 100. Rejections are counted by rpc_inflight_rejected_total and rpc_connection_rejected_total, siblings of rpc_rate_limit_rejected_total so an operator can tell the controls apart. collectRejectionMetrics now shares one reader across the package. It installed a fresh meter provider per test, but the otel global takes only the first, so a second caller left the earlier test collecting nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4078 +/- ##
==========================================
- Coverage 61.23% 60.22% -1.02%
==========================================
Files 2177 2072 -105
Lines 190632 178396 -12236
==========================================
- Hits 116729 107434 -9295
+ Misses 62892 60994 -1898
+ Partials 11011 9968 -1043
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Preserve pre-upgrade behavior unless an operator opts in to a per-IP connection cap. Update CHANGELOG to match. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview Connections: New In-flight RPCs: New Config, Reviewed by Cursor Bugbot for commit 41a7c6a. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The per-IP in-flight slot design (tap acquire + stats.End release, with the unknown-method exemption to avoid leaks) is careful and well tested, and the listener/config wiring is clean. The blocking issue is that the new connection cap keys on the raw TCP peer and therefore ignores trusted-proxy-cidrs, contradicting both its godoc and the CHANGELOG upgrade guide in a way that would lead an operator behind an ingress to cap their entire client population at one allowance.
Findings: 1 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
[grpc-web] max-connections-per-iphas no test:TestStartGRPCServer_ConnectionsPerIPCapRefusesExcessonly covers the :9090 listener, and it runs withrate-limiting-enabled = true, so the CHANGELOG's claim that the connection cap applies "regardless ofrate-limiting-enabled" (the wiring is outside the rate-limit branch in both start functions) is not pinned by anything. A case withRateLimitingEnabled: falseplus a positiveMaxConnectionsPerIP, and one drivingStartGRPCWebwithGRPCWeb.MaxConnectionsPerIPset, would cover both promises. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The three blocking/non-blocking findings from the previous review are all genuinely addressed: the connection cap's TCP-peer keying is now stated plainly in both the godoc and the CHANGELOG upgrade guide, :9091 refusals carry the new PlaneGRPCWeb label, the stats handler is gated on max-in-flight-per-ip > 0, and the two previously untested promises ([grpc-web] max-connections-per-ip, and the cap applying with rate-limiting-enabled = false) now have dedicated tests. I traced the slot lifecycle against grpc-go v1.57.1 and found no leak or double-release path: maxStreams is checked before inTapHandle, handle(s) always runs once the tap admits, processUnaryRPC/processStreamingRPC emit stats.End from stream.Context() (which carries the post-tap marker), and the unknown-method exemption covers the one path that emits nothing. Remaining findings are documentation accuracy only.
Findings: 0 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| // their clobber leaves no trace. The durations are declared as durations and written into a file as text, | ||
| // which is the shape the reader parses back. | ||
| // Most gRPC settings preserve their in-code defaults when the key is absent because the reader only reads | ||
| // them after an IsSet check. The connection-age durations are the exception: they are read unconditionally |
There was a problem hiding this comment.
[suggestion] This rewrite makes a claim the code does not support. Four grpc keys are read with no IsSet guard and no clamp: grpc.enable, grpc.address, grpc.keepalive-permit-without-stream, and grpc.rate-limiting-enabled (sei-cosmos/server/config/config.go:731, :732, :742, :746). The connection-age durations are not "the exception" — they are the exception among the unguarded keys in that their absent-key value coincides with the declared default.
That matters most for grpc.enable, which is the key the paragraph directly above this one is about: it is declared true for a full node and an archive node and false for a validator and a seed, and because it is read unconditionally an absent key casts to false and clobbers the declared true. A reader who takes this paragraph at face value concludes the mode rule survives an absent key, which is the opposite of what happens. The text this replaced accounted for those four by arithmetic ("Nine of these fifteen keys... Two more are durations..."), so the gap was at least visible; the new phrasing closes it with a false generalization.
The two keys this PR adds are both IsSet-guarded, so the honest update is to keep the shape of the old claim rather than broaden it — name the unguarded set, and say which members of it are rescued by a coinciding zero and which are not.
| # client can spend its bucket on headers alone, withhold the request bodies, and release | ||
| # them together. Excess requests are rejected with ResourceExhausted. | ||
| # It shares the same per-address keying as the buckets, and applies only when | ||
| # rate-limiting-enabled is true. Zero disables it. |
There was a problem hiding this comment.
[suggestion] This comment tells an operator that max-in-flight-per-ip "applies only when rate-limiting-enabled is true" but never says it also caps gRPC-Web. It does: RateLimitHTTPMiddleware draws on the same pool via AcquireInFlight (sei-cosmos/server/grpc/rate_limit.go:151), so a :9091 request consumes an address's [grpc] max-in-flight-per-ip allowance and can starve :9090.
The asymmetry is what makes this actionable rather than pedantic: rate-limiting-enabled three lines below explicitly says "It governs gRPC-Web (:9091) as well as native gRPC (:9090), and both planes draw from the same per-IP buckets", and trusted-proxy-cidrs below that says "on both :9090 and gRPC-Web (:9091)". An operator who has learned from the neighbours that cross-plane scope is called out where it exists will read this key's silence as :9090-only, and will size it against native-gRPC concurrency alone. The CHANGELOG does state "Both planes draw from the same per-IP pool", but app.toml is what gets read at tuning time.
A clause on the existing line covers it. GRPCConfig.MaxInFlightPerIP in config.go has the same omission and is worth the same clause.
| } | ||
| return context.WithValue(ctx, admittedIPKey{}, ip), nil | ||
| ctx = context.WithValue(ctx, admittedIPKey{}, ip) | ||
| return acquireInFlightSlot(ctx, registry, ip, info.FullMethodName) |
There was a problem hiding this comment.
[suggestion] Adding this call makes the last sentence of RateLimitTapHandle's godoc false: "It executes on the connection's I/O goroutine, which is why it does nothing beyond the bucket check." The handler now also does a known-method map lookup and a mutex-protected counter increment.
That sentence is load-bearing rather than descriptive — it exists to tell the next person that work added here runs on the goroutine reading HTTP/2 frames for the whole connection, so anything that blocks stalls every stream on it. Leaving it stating a bound the code no longer respects loses that warning exactly when the precedent for adding work has been set. Restate it as the constraint it is (bounded, non-blocking work only) and let it cover both the bucket check and the slot acquisition.
Superseded: latest AI review found no blocking issues.
3 participants
Closes PLT-1125.
Impact
Caps how much load a single client IP can place on the gRPC query plane (
:9090and:9091):max-connections-per-ip(default 0, unlimited). Set a positive value to cap one address's share of the global connection budget.max-in-flight-per-ip), complementing the per-IP token bucket from PLT-1072: Wire rate limiter into native gRPC (:9090) unary+stream interceptors #4021.Both planes share the same per-IP pools. Rejections are counted separately from rate-limit rejections (
rpc_connection_rejected_total,rpc_inflight_rejected_total).Operators: clients behind a shared egress or reaching the node over
127.0.0.1share one allowance when a positive cap is set.