Repository navigation
Conversation
The fleet listener accepts connections from anyone, since enrollment runs before a device has a client certificate. Tonic sets no TLS handshake timeout, so a client could hold a connection open forever by never sending its ClientHello. Add a fleet_limits module for the listener's limits, starting with a handshake timeout, and apply it to the fleet TLS configuration.
No fleet request had a time limit, so a handler that never completed held its connection and resources indefinitely. Set a request timeout on the fleet server. Tonic applies it until the handler produces its response; a streaming RPC responds once the stream opens, so long-lived Connect streams are not cut off. Tests cover both.
A device that disappears without closing its connection (sleep, network change) left the controller holding that connection and its Connect stream until the operating system gave up on it. Enable HTTP/2 keepalive PINGs and TCP keepalive on the fleet server, so connections whose peer no longer answers are closed. This is the controller-side counterpart of the agent's own keepalive.
One connection could open any number of HTTP/2 streams and run any number of requests at once, taking an unbounded share of the server. Cap concurrent requests and concurrent HTTP/2 streams per connection. Requests beyond the cap wait rather than fail, so a well-behaved client sharing a connection is only slowed down.
…ment The fleet service relied on tonic's default 4 MiB decode limit for every call, including the unauthenticated enrollment RPCs, and set no limit on responses. Set the 4 MiB limit explicitly in both directions for device calls, and cap enrollment requests at 64 KiB: they carry only a hostname, a PKCE challenge and a CSR. Tonic configures limits per service, so enrollment calls are routed to their own instance of the generated server.
BeginEnrollment and CompleteEnrollment run before a device has a certificate and call the identity provider, so anyone who can reach the fleet listener could flood them. Check an enrollment limiter first in both handlers: a cap on enrollment requests in progress across all clients, and a per-address token bucket. Rejections return RESOURCE_EXHAUSTED; devices already retry enrollment with backoff. The per-address budget is generous because many devices can share an address behind NAT; the in-flight cap is the main protection.
Nothing limited how many connections the fleet listener accepted. Client certificates are optional on it, so anyone could finish the TLS handshake and hold idle connections open by answering keepalive PINGs, until the controller ran out of file descriptors. Accept fleet connections through a wrapper that closes a new connection when the total or per-address cap is reached, and releases its slots when the connection closes. The per-address cap is high because many devices can share an address behind NAT. The wrapper keeps tonic's TCP connect info, so client addresses and device certificates are unchanged, and it now sets the TCP keepalive the server builder used to.
…imit An enrolled device could open any number of Connect streams and send messages on them as fast as it liked, each one stored in the database. Keep at most one Connect stream per device: opening a new stream tells the device's previous stream to close with ABORTED. The newest stream wins, so a device reconnecting after sleep is never blocked by a stream the controller has not yet noticed is dead. Each stream also gets a message budget far above normal use (heartbeats, periodic inventory, one telemetry event per tool event); a stream over budget is closed with RESOURCE_EXHAUSTED and the device reconnects with backoff. The registry and the budget have unit tests. Wiring them into Connect is not covered end to end, since that needs a device certificate and OIDC.
The controller limits fleet connections and enrollment per client address. The chart's LoadBalancer Service used Kubernetes' default externalTrafficPolicy (Cluster), under which traffic can arrive from node addresses, turning the per-address limits into shared ones. Add service.externalTrafficPolicy, defaulting to Local, rendered only for LoadBalancer and NodePort Services. Setting it to "" restores the Kubernetes default.
… the cap After the TLS handshake, nothing closed a connection that never sent the HTTP/2 preface (hyper's protocol detection has no deadline), or one that sent it and answered keepalive PINGs without ever making a request. A client could hold such connections for free; enough of them filled the connection caps, after which every device reconnect was refused. Track each connection's request activity: a layer marks the connection busy from the start of a request until its response body is dropped, so an open Connect stream keeps it busy. A connection is closed if it makes no request within a short deadline after being accepted, or stays idle too long after its last request. At the total cap, the longest-idle connection is closed to admit a new one; a new connection is refused only when every open connection is busy. The connection handling moves to fleet_limits/connections.rs. Adds http-body and tower-layer, both already in the lockfile through tonic.
…ice certificate The 64 KiB cap covered only the enrollment RPCs. Tonic decodes a unary request before its handler runs, and the device RPCs check the client certificate in the handler, so a caller without a certificate could still send 4 MiB requests to them. Route every request without a device certificate to the small-cap service instance, not only enrollment. Tests now use mutual TLS in the production shape (device CA, client certificate optional), which also shows device certificates still reach handlers through the connection wrapper.
A separate task forwarded daemon config pushes to each Connect response and held a sender for it. When the device half-closed its request stream or sent a mismatched hello, the inbound task exited and unregistered the stream, but the response stayed open on that other task. So a device could keep many streams per connection despite the one-stream rule, and every forwarding task (and its channel) outlived its stream until the next config change, including after ordinary reconnects. Forward config pushes from the stream's own loop, so it is the only holder of the response sender, and also stop when the device goes away. A mismatched hello now ends the stream with PERMISSION_DENIED instead of silently. Not covered end to end: Connect needs a device certificate and OIDC, which the test harness does not provide.
…a device opens Connect The message budget lived on each Connect stream, so a device could reset it by reconnecting. Each new stream also checked the access token with the identity provider before anything limited it. Keep the message budget with the device, so a reconnect keeps what is left of it, and give each device a budget for opening Connect streams (20 at once, then one a minute). The open budget is taken from the device certificate before the access token is checked.
…eer table Per-address limits keyed on the full IPv6 address, so one host with a /64 could open the per-address connection cap and get a fresh enrollment budget from every address it holds. Count IPv6 clients by their /64 instead, and IPv4-mapped addresses as IPv4. The enrollment peer table dropped only refilled buckets when it was full, so a client cycling through addresses kept it full and made every new peer scan the whole table. Keep it in two generations instead: when the current one fills it replaces the previous one, and a peer seen in the previous one moves back with its bucket.
…iling CompleteEnrollment shared BeginEnrollment's per-address budget and failed at once when every in-flight slot was taken. By then the user has signed in through the browser, and a refusal makes them start over; behind a busy NAT that can happen to every device on it. CompleteEnrollment no longer spends the per-address budget. It needs an enrollment that BeginEnrollment started, which already spent it, and an unknown enrollment fails at a map lookup. It now waits up to 10 seconds for an in-flight slot before it is refused.
…per interval Connections closed at the connection caps were logged at debug, so an operator had no sign that a NAT or a load balancer was pushing devices over a per-address cap. The enrollment and Connect refusals were logged at warn once per request, so a flood would flood the log too. Log every refusal at warn, but at most once per 10 seconds for each kind, with the number of events the line stands for.
The fleet limits were constants. The ones that depend on the deployment can be wrong for it: a NAT or a load balancer that hides client addresses puts many devices behind one address, a large fleet needs more connections, and large inventories need larger messages. Add a fleetLimits section to the controller configuration for the connection caps, the device message size, the enrollment limits and the device message budget. Unset fields keep the defaults, and zero values are rejected. The Helm chart passes fleetLimits through to controller.yaml. Rename the per-device message budget fields to match.
Defaulting the Service to externalTrafficPolicy: Local drops traffic that reaches a node without a controller pod. NodePort clients that connect to another node, and load balancers that do not health-check nodes, would lose the controller after an install or a rollout. Leave the policy unset by default, and document Local (or a higher fleetLimits.connectionsPerAddress) for when the controller sees node addresses instead of devices' own.
…equest is cancelled BeginEnrollment with device authorization reserves a pending-enrollment slot before it calls the identity provider, and released it only when that call returned an error. The call has no timeout of its own, so when the fleet request timeout (or a client that disconnects) cancelled the request while the provider was slow, the slot stayed taken for the full ten-minute enrollment lifetime. A slow provider could fill the table that way. Release the reservation when it is dropped, unless the device got its grant.
A response held the connection busy until hyper dropped its body. An HTTP/2 client that grants no flow-control window never lets the server send a body, so any call that returns one (the gateway JWKS route needs no credentials) kept the connection busy forever: never idle-closed, never evicted, while the client answers keepalive PINGs. A few addresses could fill the connection cap that way. Only Connect, whose response is the device's session, still keeps the connection busy until hyper drops its response body, normally when the stream ends. Any other request counts as in progress while its handler runs, so a stalled response body leaves the connection idle and subject to the idle close and eviction.
…al cap At the total connection cap, a new connection evicted the longest-idle connection before the per-address cap was checked. An address already at its cap could keep opening TCP connections (admission runs before TLS), each one closing some other client's idle connection and then being refused itself. The per-address cap is now checked first. Registration is split out of `admit` so the ordering can be tested without sockets.
`start_request` looked the connection up under the registry lock but incremented its in-flight count after releasing it, so `evict_idle` could see the connection idle and evict it just as a request started, ending that request. On the way out, the count dropped to zero before the connection was queued as idle with its new epoch, leaving a window in which an older queue entry still looked current. Both updates now happen under the lock `evict_idle` holds. The window is a few instructions wide, so there is no deterministic test for it.
The OIDC HTTP client had no timeout, so an IdP that accepted a connection and never answered stalled whatever was waiting on it. A Connect stream waits on UserInfo at its periodic access-token check, and since that stream's task now also forwards config pushes and reacts to a newer stream from the same device, a hung IdP stalled all of those too. An enrollment waited until the fleet request timeout cancelled it, which can consume the pending enrollment and make the user sign in again. Every IdP request now gives up after 10 seconds (the JWKS fetch keeps its own limit of the same length). A timeout has no HTTP status, so the stream check treats it as the IdP being unavailable and keeps the stream.
The fleet limit tests served HTTP/2 only, but the controller also accepts HTTP/1, so its connections first go through hyper-util's protocol detection: the read that held connections open before the HTTP/2 preface. The test server now accepts HTTP/1 the same way. New tests: - a TLS connection that finishes the handshake and sends nothing is closed (the original hold, end to end with a raw rustls client); - a TLS connection is closed once idle after its last request; - enrollment gets the small message limit even from a client with a device certificate, so the path rule is tested apart from the certificate rule.
…t budget With the default external traffic policy, devices behind one node address share that address's enrollment budget as well as its connection limit, so a large rollout can be held to about one enrollment per second per node. Say so, say that Local does not help behind a load balancer that proxies connections, and name the enrollment settings to raise.
…ects zero maxMessageBytes also caps the controller's responses, and devices accept at most 4 MiB however high it is set, so the description now says what raising it does. The schema gave the integer fleetLimits fields a minimum of 0 while the parser rejects 0; it now says 1.
…se tests Three idle-close tests set the first-request deadline to 5 seconds, the same as the bound each test waits within. With request tracking broken, those connections were still closed by the first-request deadline just before the bound, so a slow run could let the regression pass. The deadline is now 60 seconds, so only the idle close can end the connection in time.
…ends A Connect stream kept its connection busy until hyper dropped the response body. A device that grants no HTTP/2 flow-control window never lets the server send that body, so after the session ended (the device half-closed, or opened a newer stream) hyper kept the body and the connection stayed busy: never idle-closed, never evicted, while the device answered PINGs. Each such connection cost one stream-open token, about 20 per device certificate and then one a minute. The request layer now puts a SessionBusy handle in a Connect request's extensions. Connect claims a release from it and holds that in its session task, so the connection goes idle when the session ends or when hyper drops the body, whichever comes first. A handler that claims no release keeps the previous behavior.
…opped reading The Connect session task sent config pushes and its closing statuses with `send().await` on an 8-message channel. A device that stopped reading could fill it, and the task then waited forever: it no longer saw a newer stream from the same device, the client going away, or the access-token check, and it held the session (and so the connection) open. Sends in the session loop now use `try_send`. A config push that finds the channel full closes the stream, and the device reconnects to get the current configuration. A closing status that does not fit is dropped, since the stream ends anyway. This path needs a device certificate and OIDC to exercise, so it has no test.
…t service FleetAgentService::new filled its enrollment and Connect limits from the built-in defaults, and startup replaced them with the configured ones through `with_fleet_limits`. A code path that built the service without that call would silently ignore `fleetLimits` in controller.yaml. `new` now takes the limits, so every caller has to choose them, and `with_fleet_limits` is gone.
…eet limits The test set two of the eight settings, so dropping a copy from `configured` left that setting silently ignored with every test passing. It now names every setting with a non-default value and compares the whole result.
A throttled warning counts every refusal since the last warning, from any client or device, and reports it with the first refusal after the interval. It named only the client or device that triggered it, as if the count were that one's. The fields are now `latest_address` or `latest_device_id` and `refused_since_last_warning` (or `closed_since_last_warning`).
…ck order The connection caps comment said a connection over either cap is closed on accept; at the total cap an idle connection is evicted instead. The device-grant comment described releasing the reservation by hand, which its Drop impl now does. BeginEnrollment spends the address's budget before it looks for a slot on purpose: requests refused for want of a slot still count, so the addresses flooding the slots cannot keep their budget to take the slots back as they free up.
`enrollmentBurst` and `deviceMessageBurst` said "at once", which reads like a limit on requests in progress; that is what `enrollmentInFlight` is.
…ry read A busy connection has no idle deadline, but each read armed its timer for an idle period from now, so a connection with an open Connect stream reset its timer on every read. The guard of the connection's last request already wakes it when it goes idle, so a busy connection now arms nothing.
The shared helper said untouched limits stayed at their production values, but it shortened the handshake, request and keepalive limits for every test. Tests meant to show a connection staying open then depended on a 100 ms PING reply, and keepalive woke every idle connection often enough that no test noticed when a finished request failed to wake its connection. Each test now starts from the defaults and lowers what it exercises; the stalled Connect test now fails if that wake is missing.
…e call Starting the fleet listener took six calls on FleetLimits, two of which had to share one connection registry, and any of them could be left out or mismatched without a compile error: separate registries closed every connection, open Connect streams included, 20 s after accept; serving with `serve(addr)` dropped the connection caps; skipping `tls` dropped the handshake timeout. The tests rebuilt this wiring themselves, so they would not have noticed. `FleetLimits::listener` now binds the listener with the TLS handshake timeout, the request and keepalive limits, the request layer and the connection registry, and `FleetListener::serve` adds the fleet service with its message limits. The tests serve through the same two calls. The defaults are no longer public: limits come from the controller configuration through `FleetLimits::from_config`, so code outside the module cannot pass the unconfigured defaults where the configured limits belong. A bind failure now names the fleet listener and its address.
Enrollment and Connect were recognized by method name in two separate matches, so a new or renamed pre-certificate RPC would silently get neither the enrollment treatment nor a review of its limits. RPC_CLASSES now classifies every FleetAgent method as enrollment, device or Connect, and is_enrollment and is_connect read it. A test compares the table with the RPCs declared in fleet.proto and fails, naming the RPC, when one is missing, renamed or listed twice.
…ener starts tonic builds the fleet listener's TLS configuration from rustls's process-wide default provider and has no way to pass one. rustls picks that default by itself only when exactly one of its aws-lc-rs and ring backends is compiled in. A workspace-wide test build enables both (ring comes from the agent's test dependencies), so building the listener's TLS configuration panicked there, and the fleet TLS tests failed in CI. install_rustls_provider makes aws-lc-rs the default unless one is already set. The controller calls it at the start of application::run, and the fleet test server calls it too, so each TLS test works on its own whichever backends the build enables.
Collaborator
Author
Review guideThe tour file at the bottom of this comment is an 11-stop walkthrough of the critical code in this PR, for the RepoTrail VS Code extension. Each stop opens one piece of the diff with notes, and a few stops end with a review question. Tests, the generated schema, the Helm templates and logging are left out. Open it from your clone. With RepoTrail 0.13.0 or later and your clone of this repo open in VS Code, open the tour in VS Code. The extension asks before it contacts GitHub, and again before it fetches this PR's commits into your clone. If the link doesn't open VS Code, run: code --open-url 'vscode://repotrail.repotrail/review?pr=agentdesktop-dev/agentdesktop/237&comment=6781746011&head=ba24b92e1f2a002dfb1248f134b78b8b5014a579'Or use a fresh checkout. Paste the prompt below into your coding agent. It checks out this PR into a temporary directory, saves the tour file there, and opens a VS Code workspace on it. You need Prompt for your agentTour file: pr-237-fleet-api-limits.json{
"repoTrailTour": 2,
"exportedAt": "2026-11-10T21:04:56.000Z",
"plan": {
"kind": "pr-diff",
"title": "#237: fleet API limits",
"summary": "Review guide for #237, which limits what clients can hold on the controller's fleet gRPC API. 11 stops on the critical code only; tests, the generated schema, Helm templates and logging are left out. Stops 1-4: the limits and how the listener applies them. Stops 5-7: connection admission, idle close, and what keeps a connection busy. Stop 8: enrollment. Stops 9-10: Connect. Stop 11: configuration. Each stop diffs the PR head (ba24b92) against main at 05fa406.",
"roots": {
"pr": {
"name": "agentdesktop-pr-237",
"pathHint": "agentdesktop-pr-237"
}
},
"steps": [
{
"root": "pr",
"file": "crates/controller/src/fleet_limits.rs",
"range": {
"startLine": 43,
"startColumn": 1,
"endLine": 140,
"endColumn": 1
},
"title": "Every limit and its default",
"explanation": "Before this PR the fleet listener ran on tonic's defaults: no TLS handshake or request timeout, no connection cap, and nothing to stop a client from holding a connection open without using it. The listener has to accept clients without a certificate, because enrollment runs before a device has one.\n\n`FleetLimits` holds every bound, and `FLEET_LIMITS` the defaults:\n- **Time:** TLS handshake 10 s, request 30 s, HTTP/2 keepalive 30 s / 20 s, TCP keepalive 60 s, first request 20 s, idle connection 120 s.\n- **Counts:** 50,000 connections in total and 4,096 per client address; 32 requests and 64 streams per connection.\n- **Message size:** 4 MiB for devices, 64 KiB without a device certificate.\n- **Enrollment:** 64 in flight; per address, 60 in a burst, then one a second; a completion waits up to 10 s for a slot.\n- **`Connect`, per device:** 600 messages in a burst, then 20 a second; 20 stream opens in a burst, then one a minute.\n\nEight of these can be set under `fleetLimits` in `controller.yaml` (stop 11). The rest are fixed.\n\n**Review question:** are these defaults right for the largest fleet we expect? The connection cap also needs a file-descriptor limit above it.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/application.rs",
"range": {
"startLine": 109,
"startColumn": 1,
"endLine": 148,
"endColumn": 1
},
"title": "Startup: limits from config, and one call builds the listener",
"explanation": "Startup builds the limits once from `controller.yaml` with `FleetLimits::from_config`, then uses them in three calls:\n- **`FleetAgentService::new(..., &limits)`** gives the service its enrollment limiter and per-device `Connect` state. The argument is required, so no startup path can run on the defaults by accident.\n- **`limits.listener(...)`** binds the fleet listener with every connection and request limit (stop 3).\n- **`fleet_listener.serve(service, routes)`** adds the fleet service with its message limits.\n\nThe defaults constant is private to `fleet_limits`, so code outside it gets limits only from config.\n\n**TLS provider.** At the top of `run` (line 30), `install_rustls_provider()` makes aws-lc-rs rustls's process-wide provider. tonic builds the listener's TLS configuration from the process default and can't be given one, and rustls won't pick a default when a build enables both of its backends.\n\nThe gateway JWKS routes and usage ingest share this server. They get the connection and request limits, but not the fleet message caps.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/fleet_limits.rs",
"range": {
"startLine": 161,
"startColumn": 1,
"endLine": 256,
"endColumn": 1
},
"title": "listener() and serve(): the only way to build the fleet listener",
"explanation": "`listener` applies every server-wide limit:\n- **The TLS handshake timeout.** tonic sets none, so a client that never sent a ClientHello held its connection forever.\n- **The request timeout.** It covers only the handler that produces the response, so open `Connect` streams aren't cut off.\n- **HTTP/2 keepalive, and the per-connection request and stream caps.**\n- **`accept_http1(true)`,** unchanged from main: agentgateway fetches the JWKS over HTTP/1.\n- **The request-tracking layer** (stop 7), which shares one connection registry with the accepting socket (stop 5).\n\nIt then binds with `nodelay` and TCP keepalive. TLS is optional only so the tests can use plain HTTP/2; production always passes it.\n\n`FleetListener` keeps its server and connections private, so they can't be served separately. `serve` takes the fleet service itself and wraps it with `fleet_service`, so it can't be added without its message limits. Leaving out the tracking layer doesn't compile, because the server's type names it.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/fleet_limits.rs",
"range": {
"startLine": 258,
"startColumn": 1,
"endLine": 359,
"endColumn": 1
},
"title": "Message caps by certificate, and a class for every RPC",
"explanation": "tonic sets message limits per service, not per method. So `fleet_service` builds two `FleetAgentServer`s around one service: `unauthenticated` decodes at most 64 KiB, `devices` up to 4 MiB. `call` sends enrollment RPCs, and any request without a client certificate, to the small one. Without the certificate check, a client with no certificate could make tonic decode a 4 MiB body for a device RPC before the handler rejected it.\n\n**`RPC_CLASSES`** lists every `FleetAgent` method as Enrollment, Device or Connect, and the path checks read it. A test compares the table with `fleet.proto`, so a new or renamed RPC fails CI until it is classified. That matters for #233 and #236, which add or rename enrollment RPCs. An Enrollment RPC's handler must still take a limiter permit itself (stop 8); the table can't do that.\n\n**Encoding.** Both copies also encode at most 4 MiB, which covers daemon config pushes on `Connect`. Devices already decode at tonic's 4 MiB default.\n\n`has_device_certificate` only checks that a certificate was presented. The TLS handshake already rejects any certificate the device CA didn't sign.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/fleet_limits/connections.rs",
"range": {
"startLine": 127,
"startColumn": 1,
"endLine": 266,
"endColumn": 1
},
"title": "Connection caps, and evicting idle connections at the cap",
"explanation": "The module doc at the top of this file explains why connections are tracked at all. Each accepted connection is registered before tonic sees it (`admit`, then `register`):\n1. **Over the per-address cap** (4,096 per IPv4 address or IPv6 /64), it is closed at once. This check comes first, so an address over its cap can't evict another client's connection only to be refused itself.\n2. **At the total cap** (50,000), the connection idle the longest is closed to make room (`evict_idle`). The new connection is closed only when no connection is idle.\n\nIdle connections queue in the order they went idle, and `idle_epoch` marks entries that have gone stale. Eviction is amortized O(1), and the queue is pruned when stale entries pile up.\n\nThe per-address cap is high because many devices can share an address behind NAT, or seem to behind a load balancer that rewrites client addresses (stop 11).\n\n**Review question:** at the cap, is evicting the longest-idle connection the behavior we want, rather than refusing the new one?",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/fleet_limits/connections.rs",
"range": {
"startLine": 346,
"startColumn": 1,
"endLine": 419,
"endColumn": 1
},
"title": "Closing connections that never make a request, or go idle",
"explanation": "Every read on an accepted connection first calls `check_open`. It fails the read, which ends the connection, once the connection has been evicted or its idle deadline has passed:\n- **`first_request`, 20 s after accept,** until the first request starts. This closes a client that finishes TLS and then sends nothing, not even the HTTP/2 preface; hyper-util's protocol detection waits for that read with no deadline of its own.\n- **`idle_connection`, 120 s after the last request finished.**\n- **No deadline while a request is in progress.** When the last request finishes, its guard wakes the connection through the stored waker, and the deadline is recomputed.\n\nTLS reads go through this wrapper too, so the close works before the HTTP/2 preface. The close sends no GOAWAY.\n\nOn HTTP/2, h2 stops reading while it can't flush its own frames. For a peer that stops reading its socket, the HTTP/2 keepalive timeout closes the connection instead.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/fleet_limits/connections.rs",
"range": {
"startLine": 481,
"startColumn": 1,
"endLine": 672,
"endColumn": 1
},
"title": "What keeps a connection busy, and why Connect is different",
"explanation": "The `TrackRequests` layer marks a connection busy for each request. When it stops counting is the subtle part:\n- **Any request but `Connect`** is busy only while its handler runs, not while hyper sends the response. A client that grants no HTTP/2 flow-control window can stop hyper from ever sending a body, and hyper keeps the body until it can. If the body kept the connection busy, such a client could pin connections forever, never idle-closed or evicted.\n- **`Connect`** is busy for the whole session. Its guard goes into a shared `SessionBusy`: the response body holds one release, and the handler holds another in its session task (stop 10). The connection goes idle at the first release, so a stalled response can't keep it busy after the session ends.\n\n`start_request` and `RequestGuard::drop` update the in-flight count under the registry lock that eviction holds, so a connection is never evicted once its request has started.\n\n**Trade-off:** a large unary response still being delivered to a very slow client can be idle-closed after 120 s.\n\n**Known gap:** nothing tests that the real `connect()` claims its release, because the tests use a stub `Connect` that claims its own. Without the claim, `Connect` falls back to the body's release.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/fleet_limits.rs",
"range": {
"startLine": 361,
"startColumn": 1,
"endLine": 494,
"endColumn": 1
},
"title": "Enrollment: a per-address budget and a global in-flight cap",
"explanation": "Enrollment runs before a device has a certificate, so anyone can call it, and each call reaches the identity provider. `begin_enrollment` and `complete_enrollment` in `service.rs` take a permit from `EnrollmentLimiter` before any other work:\n- **`admit` (BeginEnrollment):** a token bucket per client address (60 in a burst, then one a second), then one of 64 global in-flight slots, refused at once when they are full. The bucket is spent first on purpose: a request refused for want of a slot still counts, so flooding addresses can't save their budget for when slots free up.\n- **`admit_completion` (CompleteEnrollment):** no per-address budget, because the enrollment ID is a one-time secret from BeginEnrollment. It waits up to 10 s for a slot instead of failing, because the user has already signed in.\n\nAddresses are keyed by IPv4 address or IPv6 /64 (`address_key`). The bucket table keeps two generations of 10,000 addresses, so it stays bounded while active addresses keep their budgets.\n\n**Also in `oidc.rs`:** every identity provider request now times out after 10 s, and `begin_device` releases its pending-enrollment slot if the request is cancelled.\n\n**Review question:** a slow identity provider while slots are busy can still let the 30 s request timeout cancel a `CompleteEnrollment` after its pending entry is consumed, so the user has to sign in again. Is that acceptable?",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/fleet_limits.rs",
"range": {
"startLine": 507,
"startColumn": 1,
"endLine": 655,
"endColumn": 1
},
"title": "Connect: one stream per device, and per-device budgets",
"explanation": "`DeviceStreams` keeps each device's `Connect` state:\n- **One stream per device; the newest wins.** `open` registers the stream and signals the device's previous one, which then ends with `ABORTED`. A device that reconnects after sleep is never blocked by a dead stream the controller hasn't noticed yet.\n- **Message budget:** 600 in a burst, then 20 a second. It belongs to the device, so reconnecting doesn't reset it. A stream over budget is closed with `RESOURCE_EXHAUSTED`.\n- **Open budget:** 20 opens in a burst, then one a minute, checked before the access token goes to the identity provider (stop 10).\n\nOnce the table reaches 100,000 devices (or twice `max_connections`), devices without an open stream are dropped. Only devices with a certificate from the device CA get an entry.\n\n**Known limitation:** two devices sharing one identity (from a cloned disk image, say) keep replacing each other's stream. The open budget caps how often, and the refusals are logged, but nothing detects the clone.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/controller/src/service.rs",
"range": {
"startLine": 254,
"startColumn": 1,
"endLine": 402,
"endColumn": 1
},
"title": "The Connect handler: budgets first, and one task owns the stream",
"explanation": "In order:\n1. The device ID is read from the client certificate, and `admit_open` spends the open budget before the access token is checked with the identity provider.\n2. The handler claims its `SessionBusy` release (stop 7) and moves it into the session task.\n3. `device_streams.open` registers the stream, replacing any older one.\n4. One task owns the response stream. Before, a separate task forwarded config pushes and kept the response open after the device had gone.\n\nThe loop ends when the device closes its side, a newer stream replaces this one, the message budget runs out, the hello doesn't match the certificate, or the 5-minute access-token recheck fails.\n\n**No waiting on the device.** Every send in the loop is `try_send` on an 8-message channel. Otherwise a device that stopped reading would hold the task, and with it the session and its connection, forever. A config push that finds the channel full ends the stream; the daemon reconnects and is sent the current config first.\n\nThe recheck calls the identity provider, which now times out after 10 s. An unreachable provider keeps the stream open, because `UNAVAILABLE` isn't treated as a revocation.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
},
{
"root": "pr",
"file": "crates/core/src/config.rs",
"range": {
"startLine": 578,
"startColumn": 1,
"endLine": 639,
"endColumn": 1
},
"title": "fleetLimits in controller.yaml, and behind a load balancer",
"explanation": "Eight limits can be set under `fleetLimits`: the two connection caps, the message size, the enrollment in-flight cap and per-address budget, and the device message budget. Unset fields keep their defaults, unknown keys are rejected, and zero values are rejected when the config loads. Like the rest of `controller.yaml`, a change needs a restart.\n\n**Behind a load balancer.** The per-address limits work only if the controller sees devices' own addresses. With Kubernetes' default `externalTrafficPolicy: Cluster`, it may see node addresses instead, and many devices then share one address's budget. The Helm chart adds `service.externalTrafficPolicy` (unset by default) and passes `fleetLimits` through. Its values comments and `deploy/README.md` say when to set `Local`, and when to raise `connectionsPerAddress` or the enrollment budget instead.\n\n**Review question:** are these the right eight to expose? The timeouts, the per-connection caps, the open budget and the completion wait stay fixed.",
"viewMode": "diff",
"diff": {
"baseRef": "05fa4065ce12ebfdb0b8b33e46b781e0a937acb7",
"headRef": "ba24b92e1f2a002dfb1248f134b78b8b5014a579",
"scope": "file"
}
}
]
}
} |
yuval-k
reviewed
Nov 11, 2026
Comment on lines
+79
to
+80
| pub requests_per_connection: usize, | ||
| pub streams_per_connection: u32, |
Contributor
There was a problem hiding this comment.
why do we need both requests_per_connection and streams_per_connection?
Collaborator
Author
There was a problem hiding this comment.
We didn't, really. The request limit (32) only capped handlers running at once, and it sat under the stream limit (64), which already caps every open stream, including ones whose handler has returned. I dropped it in 03bd372. That raises how many handlers one connection can run at once from 32 to 64, which is still bounded.
| /// How long `CompleteEnrollment` waits for an in-flight slot before it is | ||
| /// refused. It waits rather than failing at once because the user has | ||
| /// already signed in, and a refusal makes them start over. | ||
| pub enrollment_completion_wait: Duration, |
Collaborator
Author
There was a problem hiding this comment.
Agreed. Once #236 removes the pending enrollment, enrollment_completion_wait and admit_completion can go. The RPC_CLASSES test fails until the new RPCs are classified, so whichever of the two lands second has to update the enrollment limits.
| // The listener and the request layer share one registry: the listener | ||
| // admits and closes connections, and the layer marks them busy. | ||
| let connections = Connections::new(self); | ||
| let mut server = Server::builder() |
Contributor
There was a problem hiding this comment.
tonic passes None for http2_max_local_error_reset_streams by default, and hyper's setter overwrites its own default (1024) with it, so h2 ends up with no limit on locally-reset streams (the MadeYouReset mitigation). The tonic doc says it "will default to whatever the default in hyper is", but it doesn't; same on tonic master.
Can we set .http2_max_local_error_reset_streams(Some(1024)) explicitly here, with a comment saying why?
Collaborator
Author
There was a problem hiding this comment.
Confirmed: tonic 0.14.9 passes None through hyper 1.12 to h2 0.4.19, which then has no limit. It's set to 1024 in d915119, with a comment. There's also a test that sends self-dependent PRIORITY frames and expects GOAWAY ENHANCE_YOUR_CALM; without the setting, it times out.
| /// HTTP/2 streams it may open. Further requests wait, so one connection | ||
| /// cannot occupy an unbounded share of the server. | ||
| pub requests_per_connection: usize, | ||
| pub streams_per_connection: u32, |
Contributor
There was a problem hiding this comment.
This is a concurrent limit (SETTINGS_MAX_CONCURRENT_STREAMS), not a per-connection total. Can we rename it to concurrent_streams_per_connection?
Also worth saying in the doc why it's needed: tonic's default is None, which overrides hyper's default of 200 and leaves streams unlimited. And it bounds streams whose handler already returned (open Connect streams, responses the client never reads), which the request concurrency limit doesn't.
Collaborator
Author
There was a problem hiding this comment.
Renamed, and the doc now covers both points (tonic's None removing hyper's 200, and streams counting until they close), in 03bd372.
Contributor
There was a problem hiding this comment.
Design question: is the custom connection tracking worth it? I checked tonic, hyper, tower and agentgateway. Nothing there covers per-address caps, HTTP/2 idle/first-request timeouts or idle eviction, so if we want those, this code is justified.
The simpler option is what agentgateway does: a global connection cap (atomic permit, close over the limit) + max_connection_age + keepalive, and drop the idle tracking and SessionBusy. That's weaker against floods: one address can fill the cap, and a silent connection holds its slot for the full age.
Either is fine with me, but can the PR description say which trade-off we're making and why?
Collaborator
Author
There was a problem hiding this comment.
Keeping it. The PR description now has a "Why custom connection tracking" section on the trade-off: per-address caps and idle/first-request closes, against agentgateway's global cap plus max age, where one address can fill the cap.
| } | ||
| message = inbound.message() => match message { | ||
| Ok(Some(message)) => { | ||
| if !device_stream.take_message(time::Instant::now()) { |
Contributor
There was a problem hiding this comment.
Agent telemetry is one message per tool event with no sampling, so a busy device can stay over 20/s for longer than the burst. It then gets disconnected, reconnects after 1s, the queued telemetry (channel of 256) uses up the budget again, and every reconnect spends an open token (20, then 1/min). The agent also marks enrollment failed on any session error, so this shows up as a scary status in the UI.
Instead of closing the stream, could we apply backpressure? When the budget is empty, stop reading inbound until a token refills. HTTP/2 flow control then slows the agent's send().await, and its 256-slot telemetry queue absorbs the burst. Nothing is lost, there's no reconnect, and a misbehaving device is still held to 20/s. Every message is a DB write, so the database stays protected the same way.
Something like a guard on the inbound.message() branch plus a sleep-until-refill branch in the select!, so config pushes, revalidation and supersede handling keep working while the device is throttled. Closing could stay as a much higher hard limit for clearly abusive devices.
Collaborator
Author
There was a problem hiding this comment.
Done in df701e1. Once the budget is spent, the loop stops reading until it refills (a guard on the inbound.message() branch plus a sleep branch), so config pushes, revalidation and supersede keep running.
I didn't add a hard close limit. With reads gated, the controller never reads faster than the budget, so a higher limit would have nothing to count. And h2 already closes a client that sends past its flow-control window.
One side effect: a throttled device's heartbeats queue behind its telemetry (up to the 1 MiB stream window), so it can show as offline while it catches up. Batching telemetry in the agent would fix the cause; I can file that separately.
| /// The fleet gRPC service, with a smaller request limit for enrollment and for | ||
| /// callers without a device certificate. Tonic sets message limits per service, | ||
| /// so those requests go to their own instance of the generated server. | ||
| pub struct FleetService<T> { |
Contributor
There was a problem hiding this comment.
nit: only fleet_service() constructs this. Can it be private (or pub(crate))? That keeps the generic out of the public API.
| FLEET_LIMITS.configured(config) | ||
| } | ||
|
|
||
| fn configured(mut self, config: &ControllerFleetLimitsConfig) -> Self { |
Contributor
There was a problem hiding this comment.
Each configurable default is now written in four places: FLEET_LIMITS, ControllerFleetLimitsConfig::default(), the "Defaults to …" doc text, and the generated schema. configured() copies the fields one by one, and two tests (the_configuration_defaults_match_the_default_limits, configured_limits_replace_every_deployment_limit) exist only to keep the copies in sync.
FleetLimits holds only ints and Durations, so the crate split doesn't force the copy. Could we embed the config instead?
pub struct FleetLimits {
pub configured: ControllerFleetLimitsConfig,
pub tls_handshake: Duration,
// ...the internal limits as today
}Then the defaults live only in ControllerFleetLimitsConfig::default(), from_config is a single assignment, and configured() and both sync tests go away. FLEET_LIMITS would become a fn unless the config's Default is made const.
Collaborator
Author
There was a problem hiding this comment.
Done in bbed8a9. The config's defaults are a const DEFAULT now, so FLEET_LIMITS stays a const, and configured() and both sync tests are gone.
| } | ||
|
|
||
| /// Refills `bucket` for the time since its last use, then takes one token. | ||
| fn take_token(bucket: &mut Bucket, now: Instant, refill: Duration, burst: f64) -> bool { |
Contributor
There was a problem hiding this comment.
why isnt this a method of bucket?
| if admitted { | ||
| Ok(()) | ||
| } else { | ||
| if let Some(refused) = STREAM_OPENS.event(Instant::now()) { |
Contributor
There was a problem hiding this comment.
for this and all other limits - can we add a metrics so we can see/track rejections? this way users can alert on them if needed
Collaborator
Author
There was a problem hiding this comment.
Agreed that we want these. The controller has no metrics yet, so I'd rather add an endpoint and these counters in a follow-up than grow this PR. Until then, each limit logs a throttled warning with a count.
…er reset
Tonic passes `None` for `http2_max_local_error_reset_streams` unless it is
set, which overwrites hyper's and h2's default of 1024 and leaves no limit
on the streams a client can make the server reset by breaking the protocol
("MadeYouReset"). Set it explicitly to that default.
The per-connection request limit (32) only bounded handlers running at once, below the stream limit (64) that already bounds every open stream, including ones whose handler has returned. Drop it, rename the stream limit to say it counts concurrent streams, and document why it is set.
Only FleetListener::serve builds it, so its generic stays out of the public API.
…mits FleetLimits copied each configurable limit from ControllerFleetLimitsConfig and repeated its default, and two tests kept the copies in sync. Hold the configuration as one field instead, so its defaults live only in the configuration (now a const, so FLEET_LIMITS stays a const), and drop the copy and both tests.
…sing its stream A device that spent its Connect message budget had its stream closed. The agent sends one telemetry message per tool call, so a busy device could pass the budget, reconnect, spend it again from its queued telemetry, use up its stream-open budget, and report enrollment as failed meanwhile. Instead, stop reading the device's messages until the budget refills. HTTP/2 flow control then makes the device wait to send, so it stays connected and is still held to the same rate; the agent's own queue takes the backlog, and refuses new events once full. Config pushes, access-token revalidation and stream replacement keep running in the controller while reads wait. Throttled reads are logged with the throttled warnings.
A client that sends HTTP/2 PINGs without reading the replies fills the server's send buffer. h2 flushes each PONG before it reads the next frame, so once the socket's send side is full the connection stops reading. The first-request deadline, the idle deadline and eviction were checked only on reads, so over plain TCP none of them fired: the connection held its slot, and an eviction aimed at it pushed the open count past maxConnections. Check them on writes and flushes as well, so closing a connection no longer depends on the layers above reading from it. In testing, a stalled TLS connection still closed at its first-request deadline without this change. Shutdown stays unchecked so a connection being closed can still shut down cleanly. The new tests flood PINGs without reading, against the first-request deadline and at the total connection cap. Both fail without this change.
A client without a certificate could send a request's HEADERS and never its body. tonic waits for the body before the handler runs, so the request kept its connection busy, safe from the idle deadline and from eviction, until the 30 s request timeout, and the client could open the next one before then. At the defaults, 13 addresses could hold every connection. Count a request as busy only when the caller has a device certificate. Other requests still count as activity, and their connections stay under the caps, the deadlines and the request timeout, but they can be evicted. Connect requires a device certificate, so device sessions stay protected. At the per-address cap, close that address's longest-idle connection instead of refusing the new one, as the total cap already does. Otherwise an anonymous client that shares an address with devices, such as a node address under the Cluster traffic policy, could fill the address's slots and lock those devices out. Tests that used a plain-TCP Connect as the busy connection now use a device certificate.
…ndlers No test ran the controller's own Connect or enrollment handlers behind the fleet listener. A handler could stop holding what keeps its limit in force and every test would still pass: - If Connect dropped its session release early, the connection would go idle while the session was still open. The idle deadline would then close every device stream that stays quiet longer than the idle limit. - If BeginEnrollment or CompleteEnrollment dropped its in-flight permit once admitted, enrollmentInFlight would no longer limit the requests waiting on the identity provider. Add tests that run FleetAgentService against an identity provider on a local port. A device session keeps its connection busy through quiet spells longer than the idle limit and frees it when the session ends. Each enrollment handler holds its slot while it waits on the identity provider and frees it when it ends. The test device certificate now carries a device identity, which the Connect handler requires, and the serve helpers accept any fleet service.
8 tasks
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The fleet listener had no limits, so one client could use up the controller's connections, memory or identity-provider calls. This adds:
Connectsession keeps its connection until the session ends.maxMessageBytesfor enrolled devices, 64 KiB for callers without a device certificate.CompleteEnrollmentwaits for a slot instead of failing. Identity-provider requests time out after 10 s.fleetLimitssection incontroller.yaml; Helm values anddeploy/README.mdsay when to raise the limits.Why custom connection tracking
Tonic, hyper and tower have no per-address connection cap, no HTTP/2 first-request or idle timeout, and no idle eviction. The simpler option, which agentgateway uses, is a global connection cap with a maximum connection age and keepalive. With that, one address can fill the global cap, and a connection that never makes a request holds its slot for the full age. Every device has to reach this listener before it has a certificate, so this PR takes the stronger option: per-address caps, closing connections that make no request or go idle, and evicting the longest-idle connection at the cap. The cost is
fleet_limits/connections.rs, which tracks each connection and marks it busy while a request, or aConnectsession, is in progress.Not covered
LimitNOFILEraised when the controller runs directly under systemd or launchd.Connectstream; the open budget caps how often.CompleteEnrollmentpast the 30 s request timeout, and the user then has to sign in again.Connectchanges are tested with unit tests and a stub, not end to end.