Repository navigation
fix(frontend): render chat templates off the async runtime (restores #15092); make the CallHome listen backlog configurable - #15270
Conversation
…4096 `tokio::net::TcpListener::bind` listens with mio's `LISTEN_BACKLOG_SIZE`. On 2026-08-24 commit 138668b ("feat(mm-routing): add Kimi K3 support", #13371) bumped the workspace pin from `mio = "=1.1.1"` to `"=1.2.2"` as an incidental lockfile refresh. mio 1.1.1 passed `-1` on Linux, which the kernel clamps to `net.core.somaxconn`; mio 1.2.2 hardcodes `128`. Every `dynamo-runtime` listener silently lost 32x of accept-queue depth on hosts with `somaxconn = 4096`. The CallHome listener is the one that matters: every response stream is its own connection (`tcp/client.rs` `create_response_stream` -> `TcpStream::connect`, no pooling, called once per request from `ingress/push_handler.rs`), so its accept rate scales with in-flight requests. Measured on a 2,934-concurrency disaggregated run, sampling `sk_ack_backlog` from /proc/net/tcp every 500ms: the CallHome listener reached depths of 404 and 669, 3-5x past the 128 limit. Adds `DYN_TCP_LISTEN_BACKLOG` (default 4096, kernel-capped at `somaxconn`) and binds through `tokio::net::TcpSocket` so the value is applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe TCP CallHome listener now uses a configurable listen backlog. The setting defaults to 4096 when it is missing or invalid. The listener creates and binds a socket with the parsed backlog. ChangesTCP listener backlog
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Merge Risk: ⚪ Minimal · up to The CallHome response-stream listener now uses a configurable listen backlog, defaulting to 4096 and falling back to the default for invalid values. On the supported Unix targets, socket options match the previous behavior. No blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
…nse doc Address review feedback on #15270: DYN_TCP_LISTEN_BACKLOG was missing from RESTORE_RUNTIME_ENV_NAMES, so a snapshot-restored worker would silently ignore a target pod's configured backlog (and couldn't clear a stale pre-snapshot value). Add it alongside the existing DYN_TCP_RESPONSE_STREAM_HOST/PORT pair and extend the apply-and-clear test. Also condense the DYN_TCP_LISTEN_BACKLOG doc comment, which duplicated the CallHome/mio rationale already documented beside DEFAULT_TCP_LISTEN_BACKLOG in tcp.rs, and add short docstrings to the two new backlog-parsing helpers. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
…wlist The Go compatibility allowlist in deploy/operator/internal/checkpoint/compatibility.go must mirror Python's RESTORE_RUNTIME_ENV_NAMES (components/src/dynamo/common/snapshot/constants.py). Commit 92b7ba4 added DYN_TCP_LISTEN_BACKLOG to the Python side but missed the Go side, breaking TestSnapshotRestoreEnvironmentNamesMatchPythonRuntime. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
`OpenAIPreprocessor::apply_template` renders the chat template synchronously on the tokio worker thread running the request. Tokenization was already moved to the blocking pool for exactly this reason (see the comment on `encode_with_timing`: "a synchronous encode here stalls the frontend tokio runtime for seconds, starving the I/O tasks that share the runtime"), but the render was not. With a burst of a few thousand long conversations the renders saturate the runtime's worker threads and hyper's I/O tasks stop being scheduled: request bodies stay unread in kernel receive buffers, receive windows close, and clients with a TCP user timeout abort the upload before the frontend reads it. The accept loop starves for the same reason, so connections also queue or fail before reaching the application. `off_runtime()` wraps the render in `tokio::task::block_in_place` on the multi-thread runtime, which hands the current worker to the blocking pool and lets tokio start a replacement for the async tasks. On the current-thread runtime (unit tests) it runs inline, since `block_in_place` is unavailable there. `block_in_place` rather than `spawn_blocking` because `apply_template` borrows the request generically (`&R`), so there is no `'static` value to move. This restores the change from the closed PR #15092. It was closed for lack of measurable effect, but that assessment compared throughput on runs that already passed. Measured against the failure rate it was meant to address, on a 2,934-concurrency agentic workload: 3 passes / 9 without it, 5 passes / 6 with it (Fisher one-tailed p = 0.084). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
…test Per review: the parse_tcp_listen_backlog/tcp_listen_backlog doc comments only restated what the function signature, filter, and fallback already say (the inline listen(2) i32::MAX comment is kept). Also drop test_bind_listener_accepts_connections — its bind/connect/accept path is already exercised by the existing TcpStreamServer round-trip tests, which call through bind_listener via the tcp_listener call site. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The previously raised redundant narration remains at
lib/llm/src/preprocessor.rs:1391-1392; it only compares this helper with tokenization and does not preserve an additional runtime constraint. - Original discussion: The unresolved
block_in_placeconcern remains: each concurrent template render occupies its Tokio worker while Tokio creates replacement workers, bypassing the configuredDYN_RUNTIME_MAX_BLOCKING_THREADSlimit. A burst of long renders can therefore grow process threads and memory without the configured backpressure. Move rendering to a bounded blocking executor after owning the required request data. - Original discussion: Verified still present:
off_runtimeusesblock_in_placefor every concurrent template render, bypassing the configured bounded blocking executor. A burst of long renders can retain caller threads and cause Tokio to create replacement workers per blocked render, allowing thread and memory growth instead of applying the configured blocking-work backpressure. - Original discussion: Verified still present:
off_runtimeusesblock_in_place, which replaces blocked worker threads outside the configuredDYN_RUNTIME_MAX_BLOCKING_THREADSlimit. A burst of long template renders can therefore create unbounded replacement workers and exhaust process thread or memory capacity rather than applying the configured backpressure.
|
/ok to test 4b58878 |
jthomson04
left a comment
There was a problem hiding this comment.
Approving the code change, with two caveats:
-
Remove unsupported claims before merge. This includes the code comment that template rendering leaves request bodies unread for “tens of seconds,” and categorical claims that the changes “stop” the failures. State the observed results and their limits. The B → E comparison uses different builds, so it does not isolate the template change or establish its mechanism.
-
Non-blocking follow-up: determine the cause. As discussed in the evidence request on Slack and the follow-up question, we still need to establish why this small amount of template work affects the failure rate, and whether the same mechanism applies elsewhere in the request path. Use comparisons on the same build and direct measurements during the failing burst to distinguish rendering cost, scheduling delay, and other causes. Keep the LIFO explanation marked as a hypothesis until that evidence exists.
Source review only; I did not run tests or inspect CI.
|
/ok to test 4b58878 |
|
/ok to test d35f4a0 |
Overview:
Two changes that together stop an intermittent warmup failure on a 2,934-concurrency agentic workload (DeepSeek-V4-Pro, disaggregated 5P×6 / 1D×16, 46 GPUs):
fix(runtime): the CallHome listen backlog goes back to 4096, the behavior before the mio 1.2.2 bump silently cut it to 128.fix(frontend): restores the closed PR fix(frontend): render the chat template off the async runtime #15092, which renders the chat template viablock_in_place.In our runs neither change was enough alone (3/8 and 2/6 passing); together, 11/12 passed.
On cost: this PR does not claim that chat-template rendering is expensive. Measured renders are cheap: a mean of about 1.5–2.6 ms, and under 1% take more than 50 ms even during the burst. The case for #15092 is the failure-rate result below. How it helps is a hypothesis, stated as one in "Why #15092 might matter".
Details:
What fails
At AIPerf's warmup gate, about 3,600 requests start within 0.5 s, and a single error on a root request aborts the whole run. The two arms without #15092 fail in two different ways, one for each fix:
ClientOSError(104)connection resets (3–23 at once)ClientOSError(110)timeout400for that requestThe measured experiment
29 runs of the same workload on the same topology and hardware. The listen backlog was set with an
LD_PRELOADlisten(2)shim, so it could be held fixed independently of the build. Each run is scored at the warmup gate.map_whilemap_while(C → E)map_whileis an unrelatedfilter_map→map_whilechange inaddressed_router.rsthat was present in some test builds. It is not part of this PR and adds nothing here. Arm D is therefore effectively "#15092 with the backlog at 128", and it passed only 2/6.Why #15092 might matter (hypothesis, not measured)
tokio-1.52.4/src/runtime/scheduler/multi_thread/worker.rs:114-120, 451-453).block_in_placefirst moves that task to the stealable queue and hands the worker to a replacement thread (worker.rs:454-457).Builder::disable_lifo_slot: "task 'scheduled' time is high but the runtime is underutilized" (rt: make the LIFO slot in the multi-threaded scheduler stealable tokio-rs/tokio#4941). That matches this workload, where 352 workers are mostly idle.What is not shown:
110above.110itself. It fires about 0.4 s after the request is sent, far below any TCP timeout:tcp_retries2≈ 15 min, SYN retries ≈ 2 min, AIPerf'sTCP_USER_TIMEOUT30 s. A packet capture on the HTTP port across the gate would settle it.block_in_placeis used rather thanspawn_blockingbecauseapply_templateborrows the request generically (&R), so there is no'staticvalue to move into a spawned closure. It does not create unbounded threads: the replacement comes from the same capped blocking pool asspawn_blocking.Why #15092 was closed, and why this revisits it
#15092 was closed after measuring the render as negligible, and after a throughput comparison showed no change. Our data agrees on both counts: the render is cheap, and throughput of runs that pass is unchanged. What that comparison could not see is how often runs fail at the warmup gate, which is what changes here.
The backlog change
tokio::net::TcpListener::bindlistens with mio's backlog. On 2026-08-24, commit138668ba94("feat(mm-routing): add Kimi K3 support", #13371) bumped the workspace pin frommio = "=1.1.1"to"=1.2.2". mio 1.1.1 passed-1on Linux (sys/mod.rs:132, "Silently capped to /proc/sys/net/core/somaxconn"), while mio 1.2.2 hardcodes128. Everydynamo-runtimelistener silently lost 32× of accept-queue depth on hosts withsomaxconn = 4096. #15080 restored this for the HTTP listener, but the CallHome listener was missed.CallHome is the exposed one. Every response stream is its own connection (
tcp/client.rscreate_response_stream→TcpStream::connect, no pooling, once per request), so its accept rate scales with in-flight requests. Samplingsk_ack_backlogfrom/proc/net/tcpevery 500 ms on live runs, the CallHome listener reached depths of 404 and 669, against the 128 limit.Adds
DYN_TCP_LISTEN_BACKLOG(default 4096, capped by the kernel atsomaxconn) and binds viatokio::net::TcpSocket.Listeners deliberately NOT changed
The same regression affects five other production binds. All were measured, and none is included:
runtime/.../shared_tcp_endpoint.rsruntime/system_status_server.rshost:portstring, which would need new multi-address resolution codellm/http/service/service_v2.rs:1082rl_routerisNoneunless configured)llm/block_manager/metrics_kvbm.rs/metrics, driven by scrape ratellm/kv_dc_relay/wan/grpc/server.rsCaveats
1.5.0.dev20260910; arms C, D and E used local builds at7540ca80f. So B → E measures fix(frontend): render the chat template off the async runtime #15092 plus a newer build, and no run separates the two.Where should the reviewer start?
lib/llm/src/preprocessor.rs:off_runtimeand its single call site inpreprocess_request_with_optionslib/runtime/src/pipeline/network/tcp.rs:DEFAULT_TCP_LISTEN_BACKLOG,parse_tcp_listen_backlog,bind_listenerlib/runtime/src/pipeline/network/tcp/server.rs:763: the single call-site changeenvironment_names.rs,snapshot/constants.py,checkpoint/compatibility.go:DYN_TCP_LISTEN_BACKLOGregistration and snapshot-restore allowlistsA note on TCP semantics.
net.ipv4.tcp_abort_on_overflow=0, an overflowing accept queue usually shows up as delay. The kernel drops the client's final ACK and relies on SYN-ACK retransmission (1 s, 3 s, 7 s). A lab test of 3,000 dials at backlog 128 with no acceptor gaveListenOverflows +11484and zero client-side resets.tcp_syncookies=1. There, a connection can complete via a SYN cookie while the accept queue is full, and its later data segments can then be answered with a reset. That is the leading explanation for arm A's resets, but it has not been confirmed with a packet capture.Validation
cargo test -p dynamo-llm --lib --no-default-features -- preprocessor: 222 passedcargo test -p dynamo-runtime --lib -- backlog:test_tcp_listen_backlog_env_varcovers default, zero, invalid, overflow,i32::MAXand whitespace input. The bind/accept path is covered by the existingTcpStreamServerround-trip tests. The standalonetest_bind_listener_accepts_connectionswas removed in review as a duplicate.cargo build -p dynamo-runtimeandcargo build -p dynamo-llm --no-default-features: cleancargo fmt --all -- --check: cleancargo clippy -p dynamo-runtime --no-deps --lib -- -D warnings: one pre-existing failure,unused variable: join_reuseportinquic_response.rs(a macOS-only cfg path), which reproduces identically on unmodifiedmainsk_ack_backlogsampled at 500 ms.Related Issues
🚫 This PR is NOT linked to an issue:
🤖 Generated with Claude Code