[SLOP(claude-opus-4-8)] feat(pegboard-envoy): rate-limit envoy ws ingress and cap get_pages, trim unused metrics - #5327
Conversation
|
PR Review This adds a leaky-bucket rate limiter to the pegboard-envoy WS ingress path (mirroring the gateway limiter), caps get_pages request size, and drops the prepopulate() metric-initialization helpers. Findings engine/packages/pegboard-envoy/src/ws_to_tunnel_task.rs (task_inner, around line 486) - Rate-limit tokens are silently lost to unrelated worker completions, over-throttling ingress below the configured rate recv = async {
rate_limit.acquire().await;
recv_msg(&mut ws_rx, &mut ws_to_tunnel_abort_rx, &mut term_signal).await
} => { ... }
worker = task_manager.join_next(), if !task_manager.workers.is_empty() => { ... }rate_limit state (requests_remaining, accum_drip, last_acquire) lives in the outer loop, not inside this future. When the bucket has tokens, acquire() decrements synchronously and returns Ready in the same poll, so the combined future proceeds straight into recv_msg(...).await, which then goes Pending waiting on the socket. Because tokio::select! inside loop recreates every branch each iteration, if worker = task_manager.join_next() resolves first (which it does routinely: tunnel_message_task, kv_task, etc. are spawned per request/key and complete often on a busy multiplexed envoy connection), the still-pending recv future is dropped. The token it already consumed is gone; no message was ever processed for it. Under real traffic this means the bucket drains from background task churn unrelated to actual WS message volume, so the effective inbound rate ends up throttled well below envoy_websocket_rate_limit_requests / drip_rate_us, scaling with how busy the connection background workers are rather than with actual message volume. That is the opposite of the intended behavior (a large burst allowance for a connection multiplexing an entire runner). The gateway2 version of this same refactor (pegboard-gateway2/src/ws_to_tunnel_task.rs) does not have this problem because its only sibling branch is ws_to_tunnel_abort_rx.changed(), which fires at most once per connection (at shutdown), so at most one token is ever wasted there. A fix likely needs to track whether a token has already been consumed for the in-flight recv attempt (e.g. a bool in the outer loop scope) so a cancelled-and-retried iteration does not call acquire() again, or otherwise decouple waiting for a permit from the recv attempt potentially being cancelled by an unrelated branch. engine/packages/pegboard-envoy/src/ws_to_tunnel_task.rs (task_inner, around line 486) - Abort/term signals still cannot interrupt a sleeping acquire() ws_to_tunnel_abort_rx and term_signal are only polled inside recv_msg, which runs after rate_limit.acquire() resolves. Since acquire() and recv_msg() are sequenced in one future, while the leaky bucket is asleep waiting for a token neither signal can preempt it; the outer select can only escape via the unrelated worker branch, which does not check for abort/term either. With the default envoy_websocket_rate_limit_drip_rate_us of 200, the delay is negligible, but it couples graceful-shutdown/abort responsiveness to the rate-limit config: an operator raising the drip rate to throttle an overloaded runner harder would also slow down how quickly that connection responds to shutdown. Worth hoisting abort/term as outer select! branches (as gateway2 does) rather than nesting them inside recv_msg. Test coverage No new tests cover the envoy rate limiter or the MAX_GET_PAGES_PER_REQUEST cap (engine/packages/pegboard-envoy/tests/ has no assertions touching either). Given this is explicitly a defense against an untrusted runner (per CLAUDE.md trust boundary between envoy and pegboard-envoy), and given the token-loss bug above, a test that drives concurrent worker completions alongside WS messages and asserts on effective throughput would have caught this. Minor / non-blocking
|
NathanFlurry
changed the title
MasterPtato
changed the title
…ress and cap get_pages, trim unused metrics
e9df8e7 to
5c7f16e
Compare
1 participant
No description provided.