perf(net): set TCP_NODELAY on latency-sensitive TCP hops

[purp]

Summary

Every small request/response through the sandbox request path paid a fixed ~44 ms penalty because no socket in the path disabled Nagle's algorithm, so sub-MSS ("tinygram") writes stalled waiting on TCP delayed ACKs at each hop. This sets TCP_NODELAY on every latency-sensitive TCP socket in the tunnel, egress-proxy, interactive-exec, and metadata paths — cutting the marginal cost of a small governed request from ~44 ms to ~0.3–0.5 ms (within a few hundred microseconds of the host loopback floor) with no regression on bulk transfers.

Opening as a draft to invite discussion of the approach before it's marked ready.

Related Issue

Fixes #2219

Changes

• gateway (relay): set TCP_NODELAY on accepted connections on the public listener (gRPC relay frames and WS tunnel writes).
• gateway (exec): set TCP_NODELAY on the single-use SSH-over-relay loopback bridge — the accepted client connection plus both russh client dials (interactive and non-interactive exec). This is the most tinygram-heavy path in the system: interactive keystrokes and line-buffered PTY output.
• CLI: edge-tunnel local accept + the WebSocket's underlying TCP stream, the insecure TLS connector (matching tonic's default connector), and service-forward accepted sockets. The edge tunnel now also logs when the WebSocket rides an unrecognized MaybeTlsStream variant, so a future TLS-backend change can't silently skip nodelay.
• supervisor: direct-tcpip connect into the sandbox netns, TCP relay target dials (connect_tcp_target), egress-proxy accepted connections, and all upstream CONNECT/HTTP dials (via a new connect_upstream helper).
• sandbox (metadata): set TCP_NODELAY on accepted connections to the IMDS-style metadata server — the credential/identity endpoint an agent polls.
• Setting TCP_NODELAY is best-effort: a failure only costs latency, so we log and continue rather than fail the connection.
• Regression tests assert TCP_NODELAY is set on the proxy upstream, direct-tcpip, and TCP-relay connect paths. The exec-bridge and metadata sites route through the same already-tested helpers (set_tcp_nodelay_best_effort / connect_tcp_nodelay_best_effort), so their coverage is by construction.

The sandbox SSH transport rides a unix domain socket and gRPC client channels use tonic defaults (nodelay on), so no change is needed there.

Correction (addressing review): the supervisor's SSH daemon rides a unix domain socket (correctly untouched), and gRPC client and server channels use tonic defaults with nodelay on (verified in tonic 0.14 source — this covers the driver gRPC servers too). What the original claim missed is the gateway-side exec bridge: a loopback TCP hop that carries the most tinygram-heavy traffic in the system, which now sets TCP_NODELAY on all three of its sockets.

Coverage re-sweep

After the review I independently re-traced every production TCP accept/connect site in the workspace. Conclusion: the exec bridge and the metadata server were the only remaining missed hops, both now fixed. Also confirmed:

• The CLI's two OAuth login-callback loopback servers are structurally identical (hand-rolled hyper::serve_connection over an accepted loopback TcpStream) but are one-shot, human-in-the-loop redirects — the ~40 ms stall is imperceptible next to browser + human latency, so they're intentionally left as-is.
• The Kubernetes and VM driver gRPC servers hand accept to tonic's .serve(); tonic 0.14 defaults server tcp_nodelay to true, so they were already covered.
• Everything else is covered by this PR, rides a unix domain socket (no Nagle), is a throwaway reachability probe, or is test-only.

Testing

• mise run pre-commit passes
• Unit tests added/updated (nodelay regression tests for the three connect hops; exec/metadata sites covered by construction via the tested helpers)
• E2E tests added/updated (if applicable) — not applicable; validated with a component-isolation benchmark ladder instead (below)

A/B benchmark ladder (medians over 30 reps/cell; macOS arm64, Docker compute driver). Tools and full results are on a temporary branch: relay-latency-benchmarking.

Keep-alive marginal cost per small request:

Path	baseline	this PR	host floor
L4 passthrough, 100-req conn	43.58 ms	0.26 ms	0.06 ms
L7 terminate+inspect, 100-req conn	44.49 ms	0.45 ms	0.06 ms

No regression (worst cases): 10 MB response 58.6 → 59.0 ms (unchanged); 10 MB request 90.4 → 46.0 ms (Nagle-throttled uploads got ~2× faster); L7 body-rewrite rung 1 KB 63.7 → 14.9 ms.

The exec bridge and metadata server were not separately benchmarked; both are loopback TCP hops subject to the same delayed-ACK stall the ladder measures.

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated (if applicable) — not applicable; this is a socket-option tuning with no change to subsystem boundaries or data flow

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[purp]

Still in progress. I've been using this to learn more about OpenShell, and what I've learned is that this fix is not central enough yet. I think one more good iteration from me and Claude will get us to a decent place.

[TaylorMutch]

Sounds good @purp !

[purp]

Okay, I think I'm happy with this for the moment. Initial implementation was a bit too shallow and copy-paste-y across many files, so we drove the change down into openshell-core and it seems much cleaner.

Some notes:

The _best_effort suffix: I don't love it, but I don't know Rust well enough to know if there's some convention I should use instead. Claude and I had an argument about this, and it convinced me that this was the best we could do. Read a fair bit about Rust error handling as control flow and I think I agree, at least for now.

Claude's take:

The helpers set TCP_NODELAY but deliberately swallow-and-log any failure — disabling Nagle is a latency optimization, and a failed set_nodelay still leaves a working connection, so there's nothing for the caller to act on. Rust has no punctuation convention for this (unlike Ruby's foo/foo!); fallibility is expressed in the type, and a function that returns () reads as "this succeeded." The _best_effort suffix is what tells a reader the flag is attempted, not guaranteed. Note the naming is deliberately asymmetric with the return types: connect_tcp_nodelay_best_effort returns io::Result because the connect can genuinely fail and must be checked, while the _best_effort part refers only to the nodelay tweak. We avoided try_set_nodelay, since try_ in Rust idiom means "returns a Result you must handle" — the opposite of what these do.

It's trivial to imagine someone adding a new TcpStream::connect call instead of connect_tcp_nodelay_best_effort (or without calling set_tcp_nodelay_best_effort on their stream just after). Claude and I discussed this and agreed to hold off on the obvious mechanism, clippy::disallowed_methods, as being a bit too broad and potentially impactful to ride along in this change; if that's the right thing to do, we'll put up a separate PR for the work and get on it. In the meantime we added a snippet to AGENTS.md to steer them towards it.

Claude's take:

It's the right tool for enforcement, but the blast radius makes it a separate, deliberate change. A repo-wide grep finds TcpStream::connect at ~30 sites — mostly legitimate test clients, plus several production server-side dials (server/src/tls.rs, grpc/sandbox.rs, lib.rs) that this performance work never touched. clippy.toml is workspace-global, so every one of those would need an #[allow] or a reroute, and turning it on forces a decision on whether those untouched dials should get nodelay — scope well beyond this change. It also only guards the dial path, not accepted sockets (which need nodelay too but don't go through connect), so it'd give a false sense of full coverage. What this PR does deliver is the prerequisite: a single central connect_tcp_nodelay_best_effort for dialing, which makes the lint possible to add later as its own reviewed change.

Ready for review.

[russellb]

Thanks for the PR! I'm taking a look at this one now.

[russellb]

🤖 Agent-assisted code review — produced by an AI coding agent (Claude) reviewing this PR at its head commit. Findings were verified by tracing production TCP accept/connect sites on the branch.

1. Missed hop: the gateway's exec loopback bridge

start_single_use_ssh_proxy_over_relay (crates/openshell-server/src/grpc/sandbox.rs:1846) binds a loopback TcpListener, and copy_bidirectionals the accepted client_conn (:1853) against the relay DuplexStream; the russh client dials into it at :1725 and :1896. This is the production ExecSandbox transport for both non-interactive (run_exec_with_russh, called :1573) and interactive (run_interactive_exec_with_russh, called :1652) exec.

None of the three sockets gets TCP_NODELAY. russh::client::connect_stream takes a pre-connected socket and sets no socket options, so the caller owns this. It's a loopback TCP hop — which this PR explicitly argues still suffers the stall — carrying the most tinygram-heavy traffic in the system (keystrokes, line-buffered PTY output). A ~40 ms per-roundtrip stall here is directly perceptible at an interactive prompt.

The PR note "the sandbox SSH transport rides a unix domain socket… so no change is needed there" holds for the supervisor's SSH daemon (ssh.rs:83 is a UnixListener, correctly untouched) but does not cover this gateway-side TCP bridge.

Fix: set_tcp_nodelay_best_effort(&client_conn) after the accept at :1853, and on the stream after each connect at :1725/:1896. If exec is intentionally deferred, narrow the "all tunnel and proxy TCP hops" claim to say so — exec is a first-class latency path.

2. Uncovered production accept: sandbox metadata server

crates/openshell-sandbox/src/metadata_server.rs:71 accepts production TCP for the IMDS-style credential/identity endpoint. Low-frequency, so likely out of scope — but it's a sandbox-side small-request path an agent hits. Either cover it or add a one-line "intentionally excluded" note.

3. edge_tunnel.rs silently skips unknown TLS variants

The MaybeTlsStream match falls through _ => None, skipping nodelay for any variant that isn't Plain/Rustls. Correct today (rustls is the only backend), but a debug! in that arm would surface a silent miss if the TLS backend ever changes.

---

Everything else in the relay path checks out: proxy.rs dials (:1083, :4344) and accept (:274), the gateway listener (server/lib.rs:550), the supervisor connect paths, and the UDS-based drivers are all handled correctly; the remaining connect/accept sites are test-only.

[johntmyers]

/ok to test 98354f3

[purp]

All three confirmed and addressed in the incoming revision.

1. Exec loopback bridge — fixed. TCP_NODELAY is now set on all three sockets: the accepted client_conn in start_single_use_ssh_proxy_over_relay, and the russh client dials in both run_exec_with_russh and run_interactive_exec_with_russh. This is a first-class latency path — interactive keystrokes and line-buffered PTY output are exactly the tinygram traffic the stall punishes. The PR body's "SSH transport rides a UDS" note is being corrected to distinguish the supervisor's SSH daemon (unix socket, untouched) from this gateway-side loopback bridge.

2. Metadata server — fixed rather than excluded. set_tcp_nodelay_best_effort on the accepted connection in metadata_server.rs. Low-frequency but agent-facing, and the one-liner is consistent with the guidance this PR adds to prefer nodelay on any non-UDS TCP stream.

3. edge_tunnel.rs unknown TLS variant — added a debug! in the _ arm so a future TLS-backend change surfaces the silent miss instead of skipping nodelay quietly.

Independent re-sweep. I re-traced every production TCP accept/connect site in the workspace to confirm nothing else was missed. The exec bridge and the metadata server were the only remaining hops. For completeness:

• The CLI's two OAuth login-callback loopback servers are the same shape (hand-rolled hyper::serve_connection over an accepted loopback socket) but are one-shot, human-in-the-loop redirects, so the stall is imperceptible — left as-is.
• The Kubernetes and VM driver gRPC servers hand accept to tonic's .serve(); tonic 0.14 defaults server tcp_nodelay to true (verified in-source), so they were already covered.
• Everything else is covered by this PR, rides a unix domain socket (no Nagle), is a throwaway reachability probe, or is test-only.