Repository navigation
feat(server_utils): opt-in CRLF heartbeat so long HTTP/1.1 model calls survive Global Accelerator's 340s idle timeout - #3117
Conversation
…sts alive Adds HeartbeatTCPConnector, an aiohttp.TCPConnector subclass that writes a bare CRLF on every in-flight connection every N seconds, and wires it into the global aiohttp client behind a new config field `global_aiohttp_crlf_heartbeat_seconds` (default 0 = off, identical to today). Why: some network intermediaries close TCP connections that carry no payload for a fixed period and ignore TCP keepalive probes (e.g. AWS Global Accelerator, 340s). Non-streaming model calls that take longer than that to produce their first response byte fail with ServerDisconnectedError. aiohttp is HTTP/1.1-only so HTTP/2 PING is not available; a CRLF between requests is the HTTP/1.1 equivalent: it is TLS payload to the intermediary, and servers must ignore empty lines before a request-line (RFC 9112 §2.2; cf. RFC 5626 §4.4.1). Measured against an echo endpoint behind AWS Global Accelerator with a 400s server-side delay: unchanged aiohttp failed at 340s; with the heartbeat (60s) the request returned 200. Connection reuse verified after single CRLFs and bursts of ten. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
b2000ae to
13671a1
Compare
There was a problem hiding this comment.
Initial review:
Feature offered is a good alternative way to ensure model calls survive network intermediaries.
Once per interval, when _beat_once() actually runs, it executes as one uninterrupted step: nothing else on the event loop (other requests, other responses) can make progress until that single call returns. Cost is O(N) in the number of in-flight connections, including a TLS-encrypted write per connection.
Changes requested:
The PR asserts this is "negligible at 16k concurrency" but that's not benchmarked, the evidence sections test connection survival against the 340s timeout, not _beat_once()'s cost under load - please correct me if I'm mistaken here. Also, our target concurrency is much higher.
Suggest:
benchmark _beat_once()'s actual duration at realistic concurrency before merging.
Clarify in the module and class docstrings that the heartbeat is driven by a single background task per connector which makes one pass over the acquired set per interval; no per-connection tasks or timers are created. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
…eartbeat writes The heartbeat pass over the acquired set was a single synchronous loop. At 16,000 in-flight TLS connections that blocks the event loop for ~260 ms per pass (measured: aiohttp 3.14.3, Python 3.13, OpenSSL 3.5, c5.9xlarge). Make _beat_once async and yield after every `batch_size` writes (default 128) so the worst-case stall is one batch, a few milliseconds, at the same CPU cost. Adds a `batch_size` constructor argument (validated >= 1), documents the cost model, and adds a unit test asserting that a pass over many connections yields between batches. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com> Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
|
Thanks @kajalj22, both points were worth chasing, and you were right on the second. On "one task per acquired connection": the connector runs exactly one background task per connector, regardless of connection count. Each interval that task makes one pass over the connector's acquired set and writes On "negligible at 16k, not benchmarked": correct, that sentence was an estimate, not a measurement, and measuring showed the cost was not negligible in the way that matters. The pass was a single synchronous loop, so at 16,000 in-flight TLS connections it blocked the event loop for ~260 ms; at 100,000, for ~2.1 s. CPU was small (3.5 % of one core at 100k with a 60 s interval), but a multi-second loop stall once a minute is not acceptable in a process serving other requests. Change (commit 3): Full method, machine details and the table (off / unbatched / batched at 16k, 50k, 100k, with CPU, loop lag p99 and max, pass time and RSS) are in the "Benchmark" section of the PR description; the unbatched rows are kept there as the before measurement. Client and server were separate pods on separate c5.9xlarge nodes, TLS 1.3, aiohttp 3.14.3, Python 3.13.15, with a 10 ms ticker on the same event loop measuring lag. Caveat: this measures the heartbeat pass in isolation with idle in-flight connections, not under Gym's real request mix. Happy to run it against a Gym workload if you can point me at a representative setup. |
|
Thanks for the benchmarking and identifying the cost of the changes @balajinvda This is great progress! Especially the yields on the batch size - this amortizes the cost well and will limit the pressure on the event loop. Now we can benchmark the solution with an actual training run to see the numbers when the event loop is under pressure |
|
PR looks good - pending the benchmark data from a training run |
7e7b016 to
6b9af97
Compare
What does this PR do?
Adds an opt-in CRLF heartbeat to the global aiohttp client so that long-running, non-streaming model calls survive network intermediaries that drop idle TCP connections. Default off; no behaviour change unless
global_aiohttp_crlf_heartbeat_secondsis set.Why
Some hosted-inference endpoints sit behind intermediaries that close any TCP connection carrying no payload for a fixed period and that ignore TCP keepalive probes. AWS Global Accelerator is the common example: its idle timeout is fixed at 340 s and not configurable (AWS docs). A judge or reward-model call that takes longer than that to produce its first response byte fails with
ServerDisconnectedErrorat ~340 s on every retry. Gym already enables TCP keepalive on its sockets; that does not help here because the intermediary does not count keepalive probes.HTTP/2 clients handle this with PING frames. aiohttp is HTTP/1.1-only, and Gym intentionally standardises on aiohttp (
CLAUDE.md). The HTTP/1.1 equivalent is to write a bareCRLFon the connection while the request is in flight:How
nemo_gym/aiohttp_heartbeat.py:HeartbeatTCPConnector(TCPConnector). Exactly one background task per connector, not per connection. Everyheartbeatseconds it makes a single pass over the connector's acquired set (requests in flight, including the wait before response headers) and callstransport.write(b"\r\n")on each, yielding to the event loop after everybatch_sizewrites (default 128) so a large pass cannot stall other coroutines. No per-connection tasks, timers or callbacks; idle pooled connections are untouched.heartbeat=0makes it identical toTCPConnector.nemo_gym/server_utils.py: new config fieldglobal_aiohttp_crlf_heartbeat_seconds: float = 0;set_global_aiohttp_clientbuildsHeartbeatTCPConnector(heartbeat=cfg....)instead ofTCPConnector. Everything else (socket factory, limits,request(), retries, tracing) is unchanged.tests/unit_tests/test_aiohttp_heartbeat.py: local slow HTTP/1.1 server that counts and skips stray CRLFs. Asserts heartbeats are written while a response is pending, the server ignores them, the body is intact, the same connection serves the next request, nothing is written while idle, and the default is off.Enable with:
Evidence
Measured against a generic echo endpoint behind AWS Global Accelerator with a 400 s server-side delay, aiohttp 3.14.x,
ClientTimeout()with no limits (Gym's configuration):ServerDisconnectedErrorat 340 sHeartbeatTCPConnector(heartbeat=60)request()from this branch,global_aiohttp_crlf_heartbeat_seconds: 0(default)ServerDisconnectedErrorat 340.1 srequest()from this branch,global_aiohttp_crlf_heartbeat_seconds: 60Connection reuse was verified after single CRLFs and after a burst of ten CRLFs on the same connection (next request 200). A bare space instead of CRLF breaks the next request (400), so the connector only ever writes CRLF.
Evidence: instrumented traces
Raw HTTP/1.1 over TLS to an NVCF echo function behind a generic AWS Global Accelerator endpoint (anycast IP redacted), server-side delay 400 s,
nvcf-poll-seconds: 1200. After each heartbeat the client reads the kernel'sTCP_INFO.bytes_sentfor the socket: each CRLF leaves the host as one 31-byte TLS record. Times are UTC.Heartbeat every 60 s: response arrives at 401 s
Heartbeat off: the accelerator closes the connection at 340 s
Same endpoint, same delay, same client; the only difference is the heartbeat.
Evidence: does the CRLF ever corrupt a response or the next request?
A concern raised during evaluation was that a CRLF written while a response is still streaming could be read by an intermediary as a pipelined request and cut the body, or make the next request on that connection fail with HTTP 400. Tested end to end against a second production gateway path (a Go
net/httporigin behind an AWS ALB and Global Accelerator; Go's server itself does answer400to a bare CRLF ahead of a request-line, so this path is the adversarial case), aiohttp 3.14 + this connector, one pooled connection so every request follows the previous request's CRLFs, heartbeat every 0.2 s (300x the recommended cadence, so CRLFs land mid-body on every request), non-streaming chat completions from a 235B model:4,313 CRLFs landed mid-body or ahead of the next request-line on reused connections; every chunked body arrived complete and the following request on the same connection was always accepted. The load balancer discards the empty line before it reaches the origin, as RFC 9112 §2.2 intends.
Known side effect on AWS ALB (found in the load balancer's own access logs, not visible to the client). The ALB does not discard a CRLF that arrives while a request is pending; it holds it as the first byte of the next request. A real request that follows is accepted (1,947 connection reuses in the 600-concurrent run all returned 200). If the client instead closes that connection while idle, the ALB logs the dangling CRLF as an incomplete request:
elb_status_code 400, no target, 0 bytes. In the 600-concurrent heartbeat run (2,420 requests, 0 client-side errors) the ALB logged 616 such entries, all when connections went idle at the end of the run and aiohttp's keep-alive cleanup closed them. Nobody receives these 400s; they show up only in the load balancer's 4xx metrics. Operators of an ALB-fronted endpoint should expect roughly one per heartbeat-carrying connection that is closed idle.Benchmark: event-loop cost of the heartbeat pass at scale
Addresses the review question about the cost of the pass under load (the earlier "negligible at 16k" wording was not measured; this is).
Setup. Client and server in separate pods on separate nodes of a Kubernetes dev cluster, TLS 1.3 over the pod network. The server accepts HTTP/1.1 requests and never answers, so every connection stays acquired (worst case for the heartbeat). The client opens N concurrent
POSTs through the connector and then, for 130 s, an independent coroutine sleeps 10 ms in a loop and records how late it wakes (event-loop lag). Heartbeat interval 60 s (the production value), so two full passes fall in the window. Three variants: heartbeat off (baseline), the unbatched single synchronous pass as first pushed in this PR (since replaced), and the batched pass now in the PR (batch_size=128). Passes that ran while connections were still being established are excluded from the pass-time figures. Numbers are from one run each; RSS is the client process's peak resident set after all connections are up.Reading it.
Machine. Client pod on a
c5.9xlargenode (Intel Xeon Platinum 8124M @ 3.00 GHz, 36 logical CPUs; pod limited to 16 CPUs / 52 GiB), Ubuntu 24.04, kernel 6.14, Kubernetes 1.34. Python 3.13.15, aiohttp 3.14.3, OpenSSL 3.5.7. Loop lag sampled by a 10 ms ticker on the same event loop as the connector, as it would be in a Gym server process.Benchmark scripts (
bench_beat.py,tls_hold_server.py) are available on request.Notes for reviewers
BaseConnector._create_connection,BaseConnector._acquired,ResponseHandler.transport. Verified on 3.14.x; the unit test guards them.Checklist
tests/unit_tests/test_aiohttp_heartbeat.py; passes together withtest_server_utils.py).pre-commit run --files ...: end-of-file, trailing whitespace, ruff, ruff-format all Passed).git commit -s).🤖 Generated with Claude Code