From a4f13bf407dae0739859a48641b9adc1abeae12c Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Fri, 10 Jul 2026 00:26:59 -0700 Subject: [PATCH 1/3] perf(net): set TCP_NODELAY on all tunnel and proxy TCP hops The sandbox tunnel added ~44 ms of latency to every small request/response because no socket in the path disabled Nagle's algorithm, so sub-MSS writes waited on delayed ACKs at each hop. Set TCP_NODELAY on every latency-sensitive TCP socket: - gateway: accepted connections on the public listener (gRPC relay frames and WS tunnel writes) - CLI: edge tunnel local accept + underlying WebSocket TCP stream, insecure TLS connector (tonic's default connector already does this), and service-forward accepted sockets - supervisor: direct-tcpip connect into the sandbox netns, TCP relay target dials, egress proxy accepted connections, and all upstream CONNECT/HTTP dials (via a new connect_upstream helper) Setting TCP_NODELAY on connect is best-effort: a failure only costs latency, so we log and continue rather than fail the connection. The sandbox SSH transport rides a unix domain socket and gRPC client channels use tonic defaults (nodelay on), so no change is needed there. Fixes #2219 Signed-off-by: Jim Meyer --- crates/openshell-cli/src/edge_tunnel.rs | 20 ++++++++ crates/openshell-cli/src/run.rs | 5 ++ crates/openshell-cli/src/tls.rs | 5 ++ crates/openshell-server/src/lib.rs | 6 +++ .../openshell-supervisor-network/src/proxy.rs | 49 ++++++++++++++----- .../openshell-supervisor-process/src/ssh.rs | 24 ++++++++- .../src/supervisor_session.rs | 40 +++++++++++++-- 7 files changed, 131 insertions(+), 18 deletions(-) diff --git a/crates/openshell-cli/src/edge_tunnel.rs b/crates/openshell-cli/src/edge_tunnel.rs index 814e245f3c..72f54a285c 100644 --- a/crates/openshell-cli/src/edge_tunnel.rs +++ b/crates/openshell-cli/src/edge_tunnel.rs @@ -101,6 +101,11 @@ async fn accept_loop(listener: TcpListener, config: Arc) { match listener.accept().await { Ok((stream, peer)) => { debug!(peer = %peer, "accepted local tunnel connection"); + // Small gRPC frames must flush immediately; Nagle's algorithm + // otherwise adds per-write latency on this loopback hop. + if let Err(e) = stream.set_nodelay(true) { + debug!(peer = %peer, error = %e, "failed to set TCP_NODELAY"); + } let config = Arc::clone(&config); tokio::spawn(async move { if let Err(e) = handle_connection(stream, &config).await { @@ -174,6 +179,21 @@ async fn open_ws(config: &TunnelConfig) -> Result Some(tcp), + MaybeTlsStream::Rustls(tls) => Some(tls.get_ref().0), + _ => None, + }; + if let Some(tcp) = tcp { + if let Err(e) = tcp.set_nodelay(true) { + debug!(error = %e, "failed to set TCP_NODELAY on WebSocket stream"); + } + } else { + debug!("unknown WebSocket stream variant; skipping TCP_NODELAY"); + } + debug!( status = %response.status(), "WebSocket connected to edge" diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 0182e1b668..452ff94d77 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -2891,6 +2891,11 @@ pub async fn service_forward_tcp( let (socket, peer) = accepted .into_diagnostic() .wrap_err("failed to accept local forward connection")?; + // Forwarded request/response bytes are relayed in small frames; + // Nagle's algorithm would add per-write latency. + if let Err(err) = socket.set_nodelay(true) { + tracing::debug!(peer = %peer, error = %err, "failed to set TCP_NODELAY"); + } let mut client = client.clone(); let sandbox_id = sandbox_id.clone(); let target_host = target_host.to_string(); diff --git a/crates/openshell-cli/src/tls.rs b/crates/openshell-cli/src/tls.rs index 10df401a5b..28eee93c88 100644 --- a/crates/openshell-cli/src/tls.rs +++ b/crates/openshell-cli/src/tls.rs @@ -295,6 +295,11 @@ impl tower::Service for InsecureTlsConnector { let port = uri.port_u16().unwrap_or(443); let addr = format!("{host}:{port}"); let tcp = tokio::net::TcpStream::connect(addr).await?; + // set TCP_NODELAY so small gRPC frames are not delayed. Okay if it fails; + // things just go a bit slower in some cases, so we log and continue. + if let Err(e) = tcp.set_nodelay(true) { + debug!(error = %e, "failed to set TCP_NODELAY on TLS connection"); + } let server_name = ServerName::try_from(host)?; let tls_stream = tls_connector.connect(server_name, tcp).await?; Ok(hyper_util::rt::TokioIo::new(tls_stream)) diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index 6462ccbbf3..4cc39dcd84 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -549,6 +549,12 @@ async fn serve_gateway_listener( } }; + // Disable Nagle's algorithm: gateway responses and tunnel relay frames + // are small, latency-sensitive writes that must not wait for delayed ACKs. + if let Err(e) = stream.set_nodelay(true) { + debug!(error = %e, client = %addr, "Failed to set TCP_NODELAY on accepted connection"); + } + spawn_gateway_connection( stream, addr, diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 6d60536362..7f84dc04da 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -266,6 +266,11 @@ impl ProxyHandle { loop { match listener.accept().await { Ok((stream, _addr)) => { + // Proxied frames are relayed as they arrive; Nagle's + // algorithm would delay every small response. + if let Err(e) = stream.set_nodelay(true) { + tracing::debug!(error = %e, "failed to set TCP_NODELAY on accepted proxy connection"); + } let opa = opa_engine.clone(); let cache = identity_cache.clone(); let spid = entrypoint_pid.clone(); @@ -771,9 +776,7 @@ async fn handle_tcp_connection( // addresses (used by rootless Podman with pasta) are not hard-blocked. // Cloud metadata IPs and control-plane ports are still rejected. match resolve_and_check_trusted_gateway(&host, port, gw, sandbox_entrypoint_pid).await { - Ok(addrs) => TcpStream::connect(addrs.as_slice()) - .await - .into_diagnostic()?, + Ok(addrs) => connect_upstream(addrs.as_slice()).await.into_diagnostic()?, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -827,9 +830,7 @@ async fn handle_tcp_connection( match resolve_and_check_allowed_ips(&host, port, &nets, sandbox_entrypoint_pid) .await { - Ok(addrs) => TcpStream::connect(addrs.as_slice()) - .await - .into_diagnostic()?, + Ok(addrs) => connect_upstream(addrs.as_slice()).await.into_diagnostic()?, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -929,9 +930,7 @@ async fn handle_tcp_connection( // the resolved IP in allowed_ips. Always-blocked addresses and // control-plane ports remain denied. match resolve_and_check_declared_endpoint(&host, port, sandbox_entrypoint_pid).await { - Ok(addrs) => TcpStream::connect(addrs.as_slice()) - .await - .into_diagnostic()?, + Ok(addrs) => connect_upstream(addrs.as_slice()).await.into_diagnostic()?, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -981,9 +980,7 @@ async fn handle_tcp_connection( } else { // Default: reject all internal IPs (loopback, RFC 1918, link-local). match resolve_and_reject_internal(&host, port, sandbox_entrypoint_pid).await { - Ok(addrs) => TcpStream::connect(addrs.as_slice()) - .await - .into_diagnostic()?, + Ok(addrs) => connect_upstream(addrs.as_slice()).await.into_diagnostic()?, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -2097,6 +2094,20 @@ async fn write_all(writer: &mut (impl tokio::io::AsyncWrite + Unpin), data: &[u8 Ok(()) } +/// Connect to an upstream address with `TCP_NODELAY` set. +/// +/// Tunneled request/response bytes are relayed as they arrive; without +/// `TCP_NODELAY`, Nagle's algorithm holds back every sub-MSS write and adds +/// milliseconds of latency to each small request/response round trip. Setting +/// it is best-effort: a failure only costs latency, so we log and continue. +async fn connect_upstream(addrs: &[SocketAddr]) -> std::io::Result { + let stream = TcpStream::connect(addrs).await?; + if let Err(e) = stream.set_nodelay(true) { + tracing::debug!(error = %e, "failed to set TCP_NODELAY on upstream connection"); + } + Ok(stream) +} + #[derive(Debug, Clone)] struct L7ConfigSnapshot { config: crate::l7::L7EndpointConfig, @@ -4121,7 +4132,7 @@ async fn handle_forward_proxy( } // 6. Connect upstream - let mut upstream = match TcpStream::connect(addrs.as_slice()).await { + let mut upstream = match connect_upstream(addrs.as_slice()).await { Ok(s) => s, Err(e) => { let event = HttpActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -4379,6 +4390,18 @@ mod tests { use tokio::io::{AsyncRead, AsyncReadExt, AsyncWriteExt}; use tokio::net::{TcpListener, TcpStream}; + /// Regression test: upstream connections must disable Nagle's algorithm, + /// otherwise every small tunneled request waits on delayed ACKs and adds + /// milliseconds of latency per round trip. + #[tokio::test] + async fn connect_upstream_sets_tcp_nodelay() { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let addr = listener.local_addr().expect("local addr"); + + let stream = connect_upstream(&[addr]).await.expect("connect"); + assert!(stream.nodelay().expect("query TCP_NODELAY")); + } + fn websocket_l7_config( protocol: crate::l7::L7Protocol, websocket_credential_rewrite: bool, diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 62d10f374d..1d6ec13668 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -629,13 +629,17 @@ pub async fn connect_in_netns( .await .map_err(|_| std::io::Error::other("netns connect thread panicked"))??; std_stream.set_nonblocking(true)?; - return tokio::net::TcpStream::from_std(std_stream); + let stream = tokio::net::TcpStream::from_std(std_stream)?; + crate::supervisor_session::set_relay_nodelay(&stream); + return Ok(stream); } #[cfg(not(target_os = "linux"))] let _ = netns_fd; - tokio::net::TcpStream::connect(addr).await + let stream = tokio::net::TcpStream::connect(addr).await?; + crate::supervisor_session::set_relay_nodelay(&stream); + Ok(stream) } #[derive(Clone)] @@ -1275,6 +1279,22 @@ mod tests { use super::*; use std::process::Stdio; + /// Regression test: direct-tcpip forwards must disable Nagle's algorithm, + /// otherwise every small response from a sandbox service waits on delayed + /// ACKs and adds milliseconds of latency per round trip. + #[tokio::test] + async fn connect_in_netns_sets_tcp_nodelay() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind listener"); + let addr = listener.local_addr().expect("local addr"); + + let stream = connect_in_netns(&addr.to_string(), None) + .await + .expect("connect"); + assert!(stream.nodelay().expect("query TCP_NODELAY")); + } + /// Verify that dropping the input sender (the operation `channel_eof` /// performs) causes the stdin writer loop to exit and close the child's /// stdin pipe. Without this, commands like `cat | tar xf -` used by diff --git a/crates/openshell-supervisor-process/src/supervisor_session.rs b/crates/openshell-supervisor-process/src/supervisor_session.rs index 7c524676c6..e5b529a6a9 100644 --- a/crates/openshell-supervisor-process/src/supervisor_session.rs +++ b/crates/openshell-supervisor-process/src/supervisor_session.rs @@ -620,10 +620,14 @@ async fn connect_tcp_target( .await .map_err(|_| "netns tcp connect thread panicked")??; stream.set_nonblocking(true)?; - return Ok(tokio::net::TcpStream::from_std(stream)?); + let stream = tokio::net::TcpStream::from_std(stream)?; + set_relay_nodelay(&stream); + return Ok(stream); } - Ok(tokio::net::TcpStream::connect((host.as_str(), port)).await?) + let stream = tokio::net::TcpStream::connect((host.as_str(), port)).await?; + set_relay_nodelay(&stream); + Ok(stream) } #[cfg(not(target_os = "linux"))] @@ -632,7 +636,21 @@ async fn connect_tcp_target( port: u16, _netns_fd: Option, ) -> Result> { - Ok(tokio::net::TcpStream::connect((host.as_str(), port)).await?) + let stream = tokio::net::TcpStream::connect((host.as_str(), port)).await?; + set_relay_nodelay(&stream); + Ok(stream) +} + +/// Disable Nagle's algorithm on a relayed TCP target, best-effort. +/// +/// Relayed request/response bytes are small and latency-sensitive; without +/// `TCP_NODELAY` every sub-MSS write waits on a delayed ACK, adding +/// milliseconds per round trip. A failure here only costs latency, so we log +/// and continue rather than fail the connection. +pub(crate) fn set_relay_nodelay(stream: &tokio::net::TcpStream) { + if let Err(e) = stream.set_nodelay(true) { + debug!(error = %e, "failed to set TCP_NODELAY on relay target"); + } } #[cfg(test)] @@ -674,6 +692,22 @@ mod target_tests { } } + /// Regression test: relayed TCP targets must disable Nagle's algorithm, + /// otherwise every small tunneled request waits on delayed ACKs and adds + /// milliseconds of latency per round trip. + #[tokio::test] + async fn connect_tcp_target_sets_tcp_nodelay() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind listener"); + let addr = listener.local_addr().expect("local addr"); + + let stream = connect_tcp_target(addr.ip().to_string(), addr.port(), None) + .await + .expect("connect"); + assert!(stream.nodelay().expect("query TCP_NODELAY")); + } + #[test] fn tcp_target_allows_loopback_hosts() { validate_tcp_target(&tcp("127.0.0.1", 8080)).expect("ipv4 loopback"); From 9420400a570126780f40eafba6ba23e8cdbd4bb4 Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Sat, 11 Jul 2026 09:54:31 -0700 Subject: [PATCH 2/3] refactor(net): house TCP_NODELAY helper in a shared net module Address review feedback on the TCP_NODELAY change: - Move the shared set-nodelay helper out of supervisor_session into a new crate-private `net` module in openshell-supervisor-process, so ssh and supervisor_session no longer reach across modules through a pub(crate) item. - Make the best-effort comments at each call site terse and consistent. No behavior change; the benchmark ladder reproduces the same numbers. Signed-off-by: Jim Meyer --- crates/openshell-cli/src/edge_tunnel.rs | 9 ++++---- crates/openshell-cli/src/run.rs | 4 ++-- crates/openshell-server/src/lib.rs | 4 ++-- .../openshell-supervisor-network/src/proxy.rs | 18 ++++++--------- .../openshell-supervisor-process/src/lib.rs | 1 + .../openshell-supervisor-process/src/net.rs | 16 ++++++++++++++ .../openshell-supervisor-process/src/ssh.rs | 8 +++---- .../src/supervisor_session.rs | 22 ++++--------------- 8 files changed, 40 insertions(+), 42 deletions(-) create mode 100644 crates/openshell-supervisor-process/src/net.rs diff --git a/crates/openshell-cli/src/edge_tunnel.rs b/crates/openshell-cli/src/edge_tunnel.rs index 72f54a285c..ccecf941b5 100644 --- a/crates/openshell-cli/src/edge_tunnel.rs +++ b/crates/openshell-cli/src/edge_tunnel.rs @@ -101,8 +101,8 @@ async fn accept_loop(listener: TcpListener, config: Arc) { match listener.accept().await { Ok((stream, peer)) => { debug!(peer = %peer, "accepted local tunnel connection"); - // Small gRPC frames must flush immediately; Nagle's algorithm - // otherwise adds per-write latency on this loopback hop. + // set TCP_NODELAY so small gRPC frames are not delayed. Okay if it fails; + // things just go a bit slower in some cases, so we log and continue. if let Err(e) = stream.set_nodelay(true) { debug!(peer = %peer, error = %e, "failed to set TCP_NODELAY"); } @@ -179,8 +179,9 @@ async fn open_ws(config: &TunnelConfig) -> Result Some(tcp), MaybeTlsStream::Rustls(tls) => Some(tls.get_ref().0), diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 452ff94d77..1835f229bb 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -2891,8 +2891,8 @@ pub async fn service_forward_tcp( let (socket, peer) = accepted .into_diagnostic() .wrap_err("failed to accept local forward connection")?; - // Forwarded request/response bytes are relayed in small frames; - // Nagle's algorithm would add per-write latency. + // set TCP_NODELAY so small forwarded writes are not delayed. Okay if it + // fails; things just go a bit slower in some cases, so we log and continue. if let Err(err) = socket.set_nodelay(true) { tracing::debug!(peer = %peer, error = %err, "failed to set TCP_NODELAY"); } diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index 4cc39dcd84..c445e64923 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -549,8 +549,8 @@ async fn serve_gateway_listener( } }; - // Disable Nagle's algorithm: gateway responses and tunnel relay frames - // are small, latency-sensitive writes that must not wait for delayed ACKs. + // set TCP_NODELAY so small gRPC frames are not delayed. Okay if it fails; + // things just go a bit slower in some cases, so we log and continue. if let Err(e) = stream.set_nodelay(true) { debug!(error = %e, client = %addr, "Failed to set TCP_NODELAY on accepted connection"); } diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 7f84dc04da..906bda7586 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -266,8 +266,9 @@ impl ProxyHandle { loop { match listener.accept().await { Ok((stream, _addr)) => { - // Proxied frames are relayed as they arrive; Nagle's - // algorithm would delay every small response. + // set TCP_NODELAY so small relayed writes are not delayed. Okay + // if it fails; things just go a bit slower in some cases, so we + // log and continue. if let Err(e) = stream.set_nodelay(true) { tracing::debug!(error = %e, "failed to set TCP_NODELAY on accepted proxy connection"); } @@ -2094,12 +2095,9 @@ async fn write_all(writer: &mut (impl tokio::io::AsyncWrite + Unpin), data: &[u8 Ok(()) } -/// Connect to an upstream address with `TCP_NODELAY` set. -/// -/// Tunneled request/response bytes are relayed as they arrive; without -/// `TCP_NODELAY`, Nagle's algorithm holds back every sub-MSS write and adds -/// milliseconds of latency to each small request/response round trip. Setting -/// it is best-effort: a failure only costs latency, so we log and continue. +/// Connect to `addrs` with `TCP_NODELAY` set so small relayed writes are not +/// delayed. Best-effort: okay if it fails, things just go a bit slower, so we +/// log and continue. async fn connect_upstream(addrs: &[SocketAddr]) -> std::io::Result { let stream = TcpStream::connect(addrs).await?; if let Err(e) = stream.set_nodelay(true) { @@ -4390,9 +4388,7 @@ mod tests { use tokio::io::{AsyncRead, AsyncReadExt, AsyncWriteExt}; use tokio::net::{TcpListener, TcpStream}; - /// Regression test: upstream connections must disable Nagle's algorithm, - /// otherwise every small tunneled request waits on delayed ACKs and adds - /// milliseconds of latency per round trip. + /// Regression test: the upstream connect path sets `TCP_NODELAY`. #[tokio::test] async fn connect_upstream_sets_tcp_nodelay() { let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); diff --git a/crates/openshell-supervisor-process/src/lib.rs b/crates/openshell-supervisor-process/src/lib.rs index 1a3efd733d..ad6ef2c1ce 100644 --- a/crates/openshell-supervisor-process/src/lib.rs +++ b/crates/openshell-supervisor-process/src/lib.rs @@ -12,6 +12,7 @@ pub mod child_env; pub mod debug_rpc; pub mod log_push; pub mod managed_children; +mod net; pub mod process; pub mod run; pub mod sandbox; diff --git a/crates/openshell-supervisor-process/src/net.rs b/crates/openshell-supervisor-process/src/net.rs new file mode 100644 index 0000000000..8c1fa8cf95 --- /dev/null +++ b/crates/openshell-supervisor-process/src/net.rs @@ -0,0 +1,16 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Small networking helpers shared across the supervisor-process crate. + +use tokio::net::TcpStream; +use tracing::debug; + +/// Set `TCP_NODELAY` on a relayed TCP stream so small writes are not delayed. +/// Okay if it fails; things just go a bit slower in some cases, so we log and +/// continue. +pub fn set_nodelay_best_effort(stream: &TcpStream) { + if let Err(e) = stream.set_nodelay(true) { + debug!(error = %e, "failed to set TCP_NODELAY"); + } +} diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 1d6ec13668..1297e99887 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -630,7 +630,7 @@ pub async fn connect_in_netns( .map_err(|_| std::io::Error::other("netns connect thread panicked"))??; std_stream.set_nonblocking(true)?; let stream = tokio::net::TcpStream::from_std(std_stream)?; - crate::supervisor_session::set_relay_nodelay(&stream); + crate::net::set_nodelay_best_effort(&stream); return Ok(stream); } @@ -638,7 +638,7 @@ pub async fn connect_in_netns( let _ = netns_fd; let stream = tokio::net::TcpStream::connect(addr).await?; - crate::supervisor_session::set_relay_nodelay(&stream); + crate::net::set_nodelay_best_effort(&stream); Ok(stream) } @@ -1279,9 +1279,7 @@ mod tests { use super::*; use std::process::Stdio; - /// Regression test: direct-tcpip forwards must disable Nagle's algorithm, - /// otherwise every small response from a sandbox service waits on delayed - /// ACKs and adds milliseconds of latency per round trip. + /// Regression test: the direct-tcpip connect path sets `TCP_NODELAY`. #[tokio::test] async fn connect_in_netns_sets_tcp_nodelay() { let listener = tokio::net::TcpListener::bind("127.0.0.1:0") diff --git a/crates/openshell-supervisor-process/src/supervisor_session.rs b/crates/openshell-supervisor-process/src/supervisor_session.rs index e5b529a6a9..bf0a77da2b 100644 --- a/crates/openshell-supervisor-process/src/supervisor_session.rs +++ b/crates/openshell-supervisor-process/src/supervisor_session.rs @@ -621,12 +621,12 @@ async fn connect_tcp_target( .map_err(|_| "netns tcp connect thread panicked")??; stream.set_nonblocking(true)?; let stream = tokio::net::TcpStream::from_std(stream)?; - set_relay_nodelay(&stream); + crate::net::set_nodelay_best_effort(&stream); return Ok(stream); } let stream = tokio::net::TcpStream::connect((host.as_str(), port)).await?; - set_relay_nodelay(&stream); + crate::net::set_nodelay_best_effort(&stream); Ok(stream) } @@ -637,22 +637,10 @@ async fn connect_tcp_target( _netns_fd: Option, ) -> Result> { let stream = tokio::net::TcpStream::connect((host.as_str(), port)).await?; - set_relay_nodelay(&stream); + crate::net::set_nodelay_best_effort(&stream); Ok(stream) } -/// Disable Nagle's algorithm on a relayed TCP target, best-effort. -/// -/// Relayed request/response bytes are small and latency-sensitive; without -/// `TCP_NODELAY` every sub-MSS write waits on a delayed ACK, adding -/// milliseconds per round trip. A failure here only costs latency, so we log -/// and continue rather than fail the connection. -pub(crate) fn set_relay_nodelay(stream: &tokio::net::TcpStream) { - if let Err(e) = stream.set_nodelay(true) { - debug!(error = %e, "failed to set TCP_NODELAY on relay target"); - } -} - #[cfg(test)] fn validate_tcp_target(target: &TcpRelayTarget) -> Result<(), String> { normalize_tcp_target_host(target).map(|_| ()) @@ -692,9 +680,7 @@ mod target_tests { } } - /// Regression test: relayed TCP targets must disable Nagle's algorithm, - /// otherwise every small tunneled request waits on delayed ACKs and adds - /// milliseconds of latency per round trip. + /// Regression test: the TCP relay connect path sets `TCP_NODELAY`. #[tokio::test] async fn connect_tcp_target_sets_tcp_nodelay() { let listener = tokio::net::TcpListener::bind("127.0.0.1:0") From 98354f3707169c2140cdad46508e290e51882f26 Mon Sep 17 00:00:00 2001 From: Jim Meyer Date: Mon, 13 Jul 2026 13:55:19 -0700 Subject: [PATCH 3/3] refactor(net): consolidate TCP_NODELAY helpers into openshell_core::net Move the best-effort TCP_NODELAY helpers into the shared openshell_core::net module so every crate dials and configures sockets the same way: - Add set_tcp_nodelay_best_effort (accepted/existing streams) and connect_tcp_nodelay_best_effort (dial + set) with unit tests. - Migrate all call sites in openshell-cli, openshell-server, and the supervisor crates to the shared helpers. - Remove the crate-private net module from openshell-supervisor-process. - Document socket guidance in AGENTS.md (Network Sockets). Signed-off-by: Jim Meyer --- AGENTS.md | 13 ++++ crates/openshell-cli/src/edge_tunnel.rs | 16 +---- crates/openshell-cli/src/run.rs | 7 +- crates/openshell-cli/src/tls.rs | 7 +- crates/openshell-core/src/net.rs | 65 +++++++++++++++++-- crates/openshell-server/src/lib.rs | 7 +- .../openshell-supervisor-network/src/proxy.rs | 37 ++--------- .../openshell-supervisor-process/src/lib.rs | 1 - .../openshell-supervisor-process/src/net.rs | 16 ----- .../openshell-supervisor-process/src/ssh.rs | 5 +- .../src/supervisor_session.rs | 7 +- 11 files changed, 96 insertions(+), 85 deletions(-) delete mode 100644 crates/openshell-supervisor-process/src/net.rs diff --git a/AGENTS.md b/AGENTS.md index 53b4e80496..b982dbf89a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -156,6 +156,19 @@ ocsf_emit!(event); - If you change sandbox infrastructure, ensure the relevant sandbox e2e path succeeds. +## Network Sockets + +- On latency-sensitive TCP streams, disable Nagle's algorithm so small + request/response frames don't stall on delayed ACKs. Use + `openshell_core::net::set_tcp_nodelay_best_effort` on an accepted or + already-connected stream, or `openshell_core::net::connect_tcp_nodelay_best_effort` + when dialing. +- This applies to loopback/localhost TCP too — the delayed-ACK stall is a timer + behavior, not wire latency. +- You should skip it for unix domain sockets (no Nagle). It's not critical for + test-only connections, though using it on any non-UDS TCP stream — tests + included — is fine and preferred. + ## Commits - Always use [Conventional Commits](https://www.conventionalcommits.org/) format for commit messages diff --git a/crates/openshell-cli/src/edge_tunnel.rs b/crates/openshell-cli/src/edge_tunnel.rs index ccecf941b5..99f1f44f63 100644 --- a/crates/openshell-cli/src/edge_tunnel.rs +++ b/crates/openshell-cli/src/edge_tunnel.rs @@ -26,6 +26,7 @@ use futures::stream::{SplitSink, SplitStream}; use futures::{SinkExt, StreamExt}; use miette::{IntoDiagnostic, Result}; +use openshell_core::net::set_tcp_nodelay_best_effort; use std::net::SocketAddr; use std::sync::Arc; use tokio::io::{AsyncReadExt, AsyncWriteExt}; @@ -101,11 +102,7 @@ async fn accept_loop(listener: TcpListener, config: Arc) { match listener.accept().await { Ok((stream, peer)) => { debug!(peer = %peer, "accepted local tunnel connection"); - // set TCP_NODELAY so small gRPC frames are not delayed. Okay if it fails; - // things just go a bit slower in some cases, so we log and continue. - if let Err(e) = stream.set_nodelay(true) { - debug!(peer = %peer, error = %e, "failed to set TCP_NODELAY"); - } + set_tcp_nodelay_best_effort(&stream); let config = Arc::clone(&config); tokio::spawn(async move { if let Err(e) = handle_connection(stream, &config).await { @@ -179,20 +176,13 @@ async fn open_ws(config: &TunnelConfig) -> Result Some(tcp), MaybeTlsStream::Rustls(tls) => Some(tls.get_ref().0), _ => None, }; if let Some(tcp) = tcp { - if let Err(e) = tcp.set_nodelay(true) { - debug!(error = %e, "failed to set TCP_NODELAY on WebSocket stream"); - } - } else { - debug!("unknown WebSocket stream variant; skipping TCP_NODELAY"); + set_tcp_nodelay_best_effort(tcp); } debug!( diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 1835f229bb..2145ddfc23 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -27,6 +27,7 @@ use openshell_bootstrap::{ use openshell_bootstrap::{ GatewayMetadataSource, ListedGateway, gateway_metadata_source, list_gateways_with_source, }; +use openshell_core::net::set_tcp_nodelay_best_effort; use openshell_core::progress::{ PROGRESS_ACTIVE_DETAIL_KEY, PROGRESS_ACTIVE_STEP_KEY, PROGRESS_COMPLETE_LABEL_KEY, PROGRESS_COMPLETE_STEP_KEY, PROGRESS_STEP_PULLING_IMAGE, PROGRESS_STEP_REQUESTING_SANDBOX, @@ -2891,11 +2892,7 @@ pub async fn service_forward_tcp( let (socket, peer) = accepted .into_diagnostic() .wrap_err("failed to accept local forward connection")?; - // set TCP_NODELAY so small forwarded writes are not delayed. Okay if it - // fails; things just go a bit slower in some cases, so we log and continue. - if let Err(err) = socket.set_nodelay(true) { - tracing::debug!(peer = %peer, error = %err, "failed to set TCP_NODELAY"); - } + set_tcp_nodelay_best_effort(&socket); let mut client = client.clone(); let sandbox_id = sandbox_id.clone(); let target_host = target_host.to_string(); diff --git a/crates/openshell-cli/src/tls.rs b/crates/openshell-cli/src/tls.rs index 28eee93c88..2eadafc71a 100644 --- a/crates/openshell-cli/src/tls.rs +++ b/crates/openshell-cli/src/tls.rs @@ -3,6 +3,7 @@ use miette::{IntoDiagnostic, Result, WrapErr}; use openshell_core::auth::EdgeAuthInterceptor; +use openshell_core::net::set_tcp_nodelay_best_effort; use openshell_core::proto::inference_client::InferenceClient; use openshell_core::proto::open_shell_client::OpenShellClient; use rustls::{ @@ -295,11 +296,7 @@ impl tower::Service for InsecureTlsConnector { let port = uri.port_u16().unwrap_or(443); let addr = format!("{host}:{port}"); let tcp = tokio::net::TcpStream::connect(addr).await?; - // set TCP_NODELAY so small gRPC frames are not delayed. Okay if it fails; - // things just go a bit slower in some cases, so we log and continue. - if let Err(e) = tcp.set_nodelay(true) { - debug!(error = %e, "failed to set TCP_NODELAY on TLS connection"); - } + set_tcp_nodelay_best_effort(&tcp); let server_name = ServerName::try_from(host)?; let tls_stream = tls_connector.connect(server_name, tcp).await?; Ok(hyper_util::rt::TokioIo::new(tls_stream)) diff --git a/crates/openshell-core/src/net.rs b/crates/openshell-core/src/net.rs index 59bfdfc5d5..db5e5e4d2c 100644 --- a/crates/openshell-core/src/net.rs +++ b/crates/openshell-core/src/net.rs @@ -1,16 +1,23 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -//! Network IP classification utilities shared across `OpenShell` crates. +//! Shared networking utilities for `OpenShell` crates. //! -//! These helpers enforce the always-blocked IP invariant (loopback, link-local, -//! unspecified) and the broader internal-IP classification (adds RFC 1918 and -//! ULA). They are used by: +//! The IP-classification helpers enforce the always-blocked IP invariant +//! (loopback, link-local, unspecified) and the broader internal-IP +//! classification (adds RFC 1918 and ULA). They are used by: //! - The sandbox proxy for runtime SSRF enforcement //! - The mechanistic mapper for proposal filtering //! - The gateway server for defense-in-depth validation on approval +//! +//! The socket tuning helpers ([`set_tcp_nodelay_best_effort`], +//! [`connect_tcp_nodelay_best_effort`]) help avoid the known latency +//! imposed by conflict between Nagle's algorithm and delayed ACK behaviors. +//! + +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; -use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; +use tokio::net::TcpStream; /// Check if a hostname is a known cloud metadata hostname that resolves to an /// always-blocked metadata service. @@ -221,6 +228,27 @@ fn is_internal_v4(v4: Ipv4Addr) -> bool { false } +/// Enable `TCP_NODELAY` on a stream, logging (not returning) any failure. +/// +/// Disabling Nagle's algorithm keeps small writes from waiting on delayed ACKs. +/// It's a latency optimization: if it fails the connection still works, just a +/// bit slower, so there is nothing for the caller to act on — we log and move on. +pub fn set_tcp_nodelay_best_effort(stream: &TcpStream) { + if let Err(e) = stream.set_nodelay(true) { + tracing::debug!(error = %e, "failed to set TCP_NODELAY"); + } +} + +/// Connect to `addrs`, then enable `TCP_NODELAY` on a best-effort basis, propagating +/// any errors from `TcpStream::connect`. +/// +/// The returned stream is not *guaranteed* to have `TCP_NODELAY` set. +pub async fn connect_tcp_nodelay_best_effort(addrs: &[SocketAddr]) -> std::io::Result { + let stream = TcpStream::connect(addrs).await?; + set_tcp_nodelay_best_effort(&stream); + Ok(stream) +} + #[cfg(test)] mod tests { use super::*; @@ -584,4 +612,31 @@ mod tests { let v6 = Ipv4Addr::new(100, 64, 0, 1).to_ipv6_mapped(); assert!(is_internal_ip(IpAddr::V6(v6))); } + + // -- tcp_nodelay helpers -- + + #[tokio::test] + async fn set_tcp_nodelay_best_effort_enables_nodelay() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind"); + let addr = listener.local_addr().expect("local addr"); + let stream = TcpStream::connect(addr).await.expect("connect"); + + set_tcp_nodelay_best_effort(&stream); + assert!(stream.nodelay().expect("query TCP_NODELAY")); + } + + #[tokio::test] + async fn connect_tcp_nodelay_best_effort_sets_nodelay() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind"); + let addr = listener.local_addr().expect("local addr"); + + let stream = connect_tcp_nodelay_best_effort(&[addr]) + .await + .expect("connect"); + assert!(stream.nodelay().expect("query TCP_NODELAY")); + } } diff --git a/crates/openshell-server/src/lib.rs b/crates/openshell-server/src/lib.rs index c445e64923..ac31684cb2 100644 --- a/crates/openshell-server/src/lib.rs +++ b/crates/openshell-server/src/lib.rs @@ -52,6 +52,7 @@ pub mod tracing_bus; mod ws_tunnel; use metrics_exporter_prometheus::PrometheusBuilder; +use openshell_core::net::set_tcp_nodelay_best_effort; use openshell_core::{ComputeDriverKind, Config, Error, Result}; use std::collections::HashMap; use std::io::ErrorKind; @@ -549,11 +550,7 @@ async fn serve_gateway_listener( } }; - // set TCP_NODELAY so small gRPC frames are not delayed. Okay if it fails; - // things just go a bit slower in some cases, so we log and continue. - if let Err(e) = stream.set_nodelay(true) { - debug!(error = %e, client = %addr, "Failed to set TCP_NODELAY on accepted connection"); - } + set_tcp_nodelay_best_effort(&stream); spawn_gateway_connection( stream, diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index b90bf285ce..ea32f9ece5 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -10,7 +10,10 @@ use crate::policy_local::{POLICY_LOCAL_HOST, PolicyLocalContext}; use miette::{IntoDiagnostic, Result}; use openshell_core::activity::{ActivitySender, try_record_activity}; use openshell_core::denial::DenialEvent; -use openshell_core::net::{is_always_blocked_ip, is_internal_ip, is_link_local_ip}; +use openshell_core::net::{ + connect_tcp_nodelay_best_effort, is_always_blocked_ip, is_internal_ip, is_link_local_ip, + set_tcp_nodelay_best_effort, +}; use openshell_core::policy::ProxyPolicy; use openshell_core::provider_credentials::ProviderCredentialState; use openshell_core::secrets::{self, SecretResolver, rewrite_header_line_checked}; @@ -268,12 +271,7 @@ impl ProxyHandle { loop { match listener.accept().await { Ok((stream, _addr)) => { - // set TCP_NODELAY so small relayed writes are not delayed. Okay - // if it fails; things just go a bit slower in some cases, so we - // log and continue. - if let Err(e) = stream.set_nodelay(true) { - tracing::debug!(error = %e, "failed to set TCP_NODELAY on accepted proxy connection"); - } + set_tcp_nodelay_best_effort(&stream); let opa = opa_engine.clone(); let cache = identity_cache.clone(); let spid = entrypoint_pid.clone(); @@ -1082,7 +1080,7 @@ async fn handle_tcp_connection( return Ok(()); } - let mut upstream = connect_upstream(validated_addrs.as_slice()) + let mut upstream = connect_tcp_nodelay_best_effort(validated_addrs.as_slice()) .await .into_diagnostic()?; @@ -2265,17 +2263,6 @@ async fn write_all(writer: &mut (impl tokio::io::AsyncWrite + Unpin), data: &[u8 Ok(()) } -/// Connect to `addrs` with `TCP_NODELAY` set so small relayed writes are not -/// delayed. Best-effort: okay if it fails, things just go a bit slower, so we -/// log and continue. -async fn connect_upstream(addrs: &[SocketAddr]) -> std::io::Result { - let stream = TcpStream::connect(addrs).await?; - if let Err(e) = stream.set_nodelay(true) { - tracing::debug!(error = %e, "failed to set TCP_NODELAY on upstream connection"); - } - Ok(stream) -} - #[derive(Debug, Clone)] struct L7ConfigSnapshot { config: crate::l7::L7EndpointConfig, @@ -4354,7 +4341,7 @@ async fn handle_forward_proxy( } // 6. Connect upstream - let mut upstream = match connect_upstream(addrs.as_slice()).await { + let mut upstream = match connect_tcp_nodelay_best_effort(addrs.as_slice()).await { Ok(s) => s, Err(e) => { let event = HttpActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -4664,16 +4651,6 @@ mod tests { use tokio::io::{AsyncRead, AsyncReadExt, AsyncWriteExt}; use tokio::net::{TcpListener, TcpStream}; - /// Regression test: the upstream connect path sets `TCP_NODELAY`. - #[tokio::test] - async fn connect_upstream_sets_tcp_nodelay() { - let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); - let addr = listener.local_addr().expect("local addr"); - - let stream = connect_upstream(&[addr]).await.expect("connect"); - assert!(stream.nodelay().expect("query TCP_NODELAY")); - } - #[test] fn endpoint_only_opa_allows_declared_endpoint_without_process_identity() { let policy = include_str!("../data/sandbox-policy.rego"); diff --git a/crates/openshell-supervisor-process/src/lib.rs b/crates/openshell-supervisor-process/src/lib.rs index bff0833284..842b62f9df 100644 --- a/crates/openshell-supervisor-process/src/lib.rs +++ b/crates/openshell-supervisor-process/src/lib.rs @@ -12,7 +12,6 @@ pub mod child_env; pub mod debug_rpc; pub mod log_push; pub mod managed_children; -mod net; pub mod process; pub mod run; pub mod sandbox; diff --git a/crates/openshell-supervisor-process/src/net.rs b/crates/openshell-supervisor-process/src/net.rs deleted file mode 100644 index 8c1fa8cf95..0000000000 --- a/crates/openshell-supervisor-process/src/net.rs +++ /dev/null @@ -1,16 +0,0 @@ -// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -//! Small networking helpers shared across the supervisor-process crate. - -use tokio::net::TcpStream; -use tracing::debug; - -/// Set `TCP_NODELAY` on a relayed TCP stream so small writes are not delayed. -/// Okay if it fails; things just go a bit slower in some cases, so we log and -/// continue. -pub fn set_nodelay_best_effort(stream: &TcpStream) { - if let Err(e) = stream.set_nodelay(true) { - debug!(error = %e, "failed to set TCP_NODELAY"); - } -} diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 0827e52427..3c24efe5ec 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -11,6 +11,7 @@ use crate::sandbox; use miette::{IntoDiagnostic, Result}; use nix::pty::{Winsize, openpty}; use nix::unistd::setsid; +use openshell_core::net::set_tcp_nodelay_best_effort; use openshell_core::policy::SandboxPolicy; use openshell_core::provider_credentials::ProviderCredentialState; use openshell_ocsf::{ @@ -652,7 +653,7 @@ pub async fn connect_in_netns( .map_err(|_| std::io::Error::other("netns connect thread panicked"))??; std_stream.set_nonblocking(true)?; let stream = tokio::net::TcpStream::from_std(std_stream)?; - crate::net::set_nodelay_best_effort(&stream); + set_tcp_nodelay_best_effort(&stream); return Ok(stream); } @@ -660,7 +661,7 @@ pub async fn connect_in_netns( let _ = netns_fd; let stream = tokio::net::TcpStream::connect(addr).await?; - crate::net::set_nodelay_best_effort(&stream); + set_tcp_nodelay_best_effort(&stream); Ok(stream) } diff --git a/crates/openshell-supervisor-process/src/supervisor_session.rs b/crates/openshell-supervisor-process/src/supervisor_session.rs index b1fdc0d894..734b658108 100644 --- a/crates/openshell-supervisor-process/src/supervisor_session.rs +++ b/crates/openshell-supervisor-process/src/supervisor_session.rs @@ -31,6 +31,7 @@ use tokio_stream::StreamExt; use tracing::{debug, warn}; use openshell_core::grpc_client; +use openshell_core::net::set_tcp_nodelay_best_effort; const INITIAL_BACKOFF: Duration = Duration::from_secs(1); const MAX_BACKOFF: Duration = Duration::from_secs(30); @@ -664,12 +665,12 @@ async fn connect_tcp_target( .map_err(|_| "netns tcp connect thread panicked")??; stream.set_nonblocking(true)?; let stream = tokio::net::TcpStream::from_std(stream)?; - crate::net::set_nodelay_best_effort(&stream); + set_tcp_nodelay_best_effort(&stream); return Ok(stream); } let stream = tokio::net::TcpStream::connect((host.as_str(), port)).await?; - crate::net::set_nodelay_best_effort(&stream); + set_tcp_nodelay_best_effort(&stream); Ok(stream) } @@ -680,7 +681,7 @@ async fn connect_tcp_target( _netns_fd: Option, ) -> Result> { let stream = tokio::net::TcpStream::connect((host.as_str(), port)).await?; - crate::net::set_nodelay_best_effort(&stream); + set_tcp_nodelay_best_effort(&stream); Ok(stream) }