[PR #109] [MERGED] perf(net): disable Nagle on the API server, proxy and envd clients #184

New issue

Closed

opened 2026-10-29 14:20:46 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:20:46 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/109
Author: @cleverhu
Created: 8/3/2026
Status: ✅ Merged
Merged: 8/6/2026
Merged by: @yingdi-shan

Base: main ← Head: perf/tcp-nodelay

📝 Commits (1)

5b7d1dd perf(net): disable Nagle on the API server, proxy and envd clients

📊 Changes

3 files changed (+22 additions, -3 deletions)

View changed files

📝 src/api/proxy.rs (+3 -0)
📝 src/bin/server.rs (+9 -1)
📝 thirdparty/envd/src/transport.rs (+10 -2)

📄 Description

What

Set TCP_NODELAY on every hop of the sandbox command path: the API server's accepted connections, the sandbox proxy's connector, and both envd client connectors (h1 and h2).

Why

Short-lived commands executed through a sandbox had a hard ~40 ms latency floor that was independent of how long the command itself ran.

envd streams a command's lifecycle as a burst of tiny Connect-RPC frames. With Nagle enabled, the sender holds the second small frame until the peer acknowledges the first, and the peer's kernel delays that ACK by ~40 ms. Every hop on this path exchanges small frames only, so the coalescing win Nagle trades latency for never materializes.

Packet captures showed exactly one ~40 ms gap per command, and the first packet after the gap was a bare ACK from the client back to the node — the signature of delayed ACK meeting Nagle. The effect is a floor rather than an additive delay:

true                 exited=43 ms    <- floor
echo hi              exited=44 ms    <- floor
echo hi; sleep 0.2   exited=208 ms   <- only +8 ms
sleep 0.5            exited=508 ms   <- only +8 ms

A minimal two-small-writes-on-one-connection reproduction confirms the mechanism and the fix:

server socket	median	samples (ms)
`TCP_NODELAY` off	41.0 ms	0, 41, 41, 41, 41, 41, 41, 42, 41, 41, 41, 0
`TCP_NODELAY` on	0.1 ms	all 0

The leading and trailing zeros are the kernel's quickack window, which also explains the reported "fast after idle, slow when run back to back" behavior.

None. Submitted directly per CONTRIBUTING: the scope is a socket option on an existing code path with no API or format impact. Happy to open an issue first if maintainers prefer.

Scope and non-goals

Included: TCP_NODELAY at the three places that carry envd RPC traffic.

Excluded:

WebSocket/PTY connections — tokio-tungstenite already sets the option.
TCP keepalive on the proxy connector, which addresses a different problem and is sent as a separate PR.
Any change to timeouts, pooling, or retry behavior.

Design and behavior changes

src/bin/server.rs uses axum::serve::ListenerExt::tap_io to set the option on each accepted connection; a failure is logged at warn and the connection proceeds, since a missing socket option is a latency problem rather than a correctness one.

src/api/proxy.rs and thirdparty/envd/src/transport.rs set it on their HttpConnectors. The envd change is factored into a small nodelay_connector() helper because both the h1 and h2 clients need it.

No change to protocol framing, ordering, or error handling. Disabling Nagle can increase packet count for senders that write many small chunks, which is precisely the traffic pattern being fixed here, and these are short-lived localhost-to-VM connections.

Compatibility and operations

Public API or generated protocol: N/A, no wire format or endpoint change.
Configuration or defaults: N/A, no new knobs. The option is applied unconditionally.
Snapshot manifest, artifact layout, or storage format: N/A.
Upgrade and rollback: transparent both ways; nothing is persisted.
Host requirements, permissions, ports, or dependencies: N/A. axum's ListenerExt is already available in the pinned version.

Validation

make fmt
make clippy
make test-unit
Relevant Rust integration tests
make -C services test (required when services/ changes)
Generated clients/server regenerated with the documented make target
Documentation updated
Benchmarks or performance comparison completed

Commands and results:

$ cargo fmt --all -- --check
(clean)

$ cargo clippy --workspace --all-targets -- -D warnings
Finished `dev` profile

$ cargo check --workspace --all-targets
Finished `dev` profile

Mechanism A/B (minimal reproduction, server writes two small frames on one connection):
  server TCP_NODELAY=off   median 41.0 ms
  server TCP_NODELAY=on    median  0.1 ms

Skipped checks and reasons:

make test-unit not run: no existing unit test observes socket options, and this change adds no branch to test. The behavior is only observable at the TCP layer, which the reproduction above covers.
Integration tests not run: they require root, /dev/kvm, and a provisioned host; unavailable in the environment used here.
End-to-end latency numbers on a deployed cluster are not included. The evidence above is the packet capture plus the isolated reproduction. Anyone with a running node can confirm with scripts/bench.py or by timing a trivial command over the API before and after.

Risks and reviewer notes

Low risk. The main reviewer question is whether disabling Nagle is right for all proxied traffic rather than just the RPC control frames. It is: the proxy carries sandbox HTTP traffic over short-lived localhost-to-VM connections, where the bandwidth saving from coalescing is irrelevant and the latency cost is paid on every request.

Second question: tap_io failures are swallowed with a warn. Failing the connection instead would turn a performance regression into an outage, which seems worse.

Most important file: src/bin/server.rs.

Checklist

The PR contains one coherent change and no unrelated formatting or refactoring.
New behavior is covered by tests, or I explained why testing is impractical.
Logs and examples contain no credentials, tokens, or private registry information.
I did not manually edit generated code without updating its source and regenerating it.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/109 **Author:** [@cleverhu](https://github.com/cleverhu) **Created:** 8/3/2026 **Status:** ✅ Merged **Merged:** 8/6/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `perf/tcp-nodelay` --- ### 📝 Commits (1) - [`5b7d1dd`](https://github.com/kvcache-ai/AgentENV/commit/5b7d1dd48f9e7b1c03dc435fefefaa1c7486bbc3) perf(net): disable Nagle on the API server, proxy and envd clients ### 📊 Changes **3 files changed** (+22 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `src/api/proxy.rs` (+3 -0) 📝 `src/bin/server.rs` (+9 -1) 📝 `thirdparty/envd/src/transport.rs` (+10 -2) </details> ### 📄 Description ## What Set `TCP_NODELAY` on every hop of the sandbox command path: the API server's accepted connections, the sandbox proxy's connector, and both envd client connectors (h1 and h2). ## Why Short-lived commands executed through a sandbox had a hard ~40 ms latency floor that was independent of how long the command itself ran. envd streams a command's lifecycle as a burst of tiny Connect-RPC frames. With Nagle enabled, the sender holds the second small frame until the peer acknowledges the first, and the peer's kernel delays that ACK by ~40 ms. Every hop on this path exchanges small frames only, so the coalescing win Nagle trades latency for never materializes. Packet captures showed exactly one ~40 ms gap per command, and the first packet after the gap was a bare ACK from the client back to the node — the signature of delayed ACK meeting Nagle. The effect is a floor rather than an additive delay: ```text true exited=43 ms <- floor echo hi exited=44 ms <- floor echo hi; sleep 0.2 exited=208 ms <- only +8 ms sleep 0.5 exited=508 ms <- only +8 ms ``` A minimal two-small-writes-on-one-connection reproduction confirms the mechanism and the fix: | server socket | median | samples (ms) | | --- | ---: | --- | | `TCP_NODELAY` off | 41.0 ms | 0, 41, 41, 41, 41, 41, 41, 42, 41, 41, 41, 0 | | `TCP_NODELAY` on | 0.1 ms | all 0 | The leading and trailing zeros are the kernel's quickack window, which also explains the reported "fast after idle, slow when run back to back" behavior. ## Related issue None. Submitted directly per CONTRIBUTING: the scope is a socket option on an existing code path with no API or format impact. Happy to open an issue first if maintainers prefer. ## Scope and non-goals Included: `TCP_NODELAY` at the three places that carry envd RPC traffic. Excluded: - WebSocket/PTY connections — `tokio-tungstenite` already sets the option. - TCP keepalive on the proxy connector, which addresses a different problem and is sent as a separate PR. - Any change to timeouts, pooling, or retry behavior. ## Design and behavior changes `src/bin/server.rs` uses `axum::serve::ListenerExt::tap_io` to set the option on each accepted connection; a failure is logged at `warn` and the connection proceeds, since a missing socket option is a latency problem rather than a correctness one. `src/api/proxy.rs` and `thirdparty/envd/src/transport.rs` set it on their `HttpConnector`s. The envd change is factored into a small `nodelay_connector()` helper because both the h1 and h2 clients need it. No change to protocol framing, ordering, or error handling. Disabling Nagle can increase packet count for senders that write many small chunks, which is precisely the traffic pattern being fixed here, and these are short-lived localhost-to-VM connections. ## Compatibility and operations - Public API or generated protocol: N/A, no wire format or endpoint change. - Configuration or defaults: N/A, no new knobs. The option is applied unconditionally. - Snapshot manifest, artifact layout, or storage format: N/A. - Upgrade and rollback: transparent both ways; nothing is persisted. - Host requirements, permissions, ports, or dependencies: N/A. `axum`'s `ListenerExt` is already available in the pinned version. ## Validation - [x] `make fmt` - [x] `make clippy` - [ ] `make test-unit` - [ ] Relevant Rust integration tests - [ ] `make -C services test` (required when `services/` changes) - [ ] Generated clients/server regenerated with the documented `make` target - [ ] Documentation updated - [x] Benchmarks or performance comparison completed Commands and results: ```text $ cargo fmt --all -- --check (clean) $ cargo clippy --workspace --all-targets -- -D warnings Finished `dev` profile $ cargo check --workspace --all-targets Finished `dev` profile Mechanism A/B (minimal reproduction, server writes two small frames on one connection): server TCP_NODELAY=off median 41.0 ms server TCP_NODELAY=on median 0.1 ms ``` Skipped checks and reasons: - `make test-unit` not run: no existing unit test observes socket options, and this change adds no branch to test. The behavior is only observable at the TCP layer, which the reproduction above covers. - Integration tests not run: they require root, `/dev/kvm`, and a provisioned host; unavailable in the environment used here. - End-to-end latency numbers on a deployed cluster are **not** included. The evidence above is the packet capture plus the isolated reproduction. Anyone with a running node can confirm with `scripts/bench.py` or by timing a trivial command over the API before and after. ## Risks and reviewer notes Low risk. The main reviewer question is whether disabling Nagle is right for *all* proxied traffic rather than just the RPC control frames. It is: the proxy carries sandbox HTTP traffic over short-lived localhost-to-VM connections, where the bandwidth saving from coalescing is irrelevant and the latency cost is paid on every request. Second question: `tap_io` failures are swallowed with a `warn`. Failing the connection instead would turn a performance regression into an outage, which seems worse. Most important file: `src/bin/server.rs`. ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 14:20:46 +00:00