You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR #2220 centralizes TCP_NODELAY into openshell_core::net (set_tcp_nodelay_best_effort, connect_tcp_nodelay_best_effort) and adds a snippet to AGENTS.md telling contributors to use them. Nothing enforces it. A new TcpStream::connect on a latency-sensitive path is a silent regression: the code works, tests pass, and the ~40 ms delayed-ACK stall only shows up as "exec feels laggy" much later.
This is not hypothetical. During the three weeks #2220 has been open, one merge from main brought two independent instances:
main's crates/openshell-sdk/src/transport.rs and crates/openshell-sdk/src/edge_tunnel.rs still contain zero nodelay calls on their production socket paths. Patched on the perf(net): set TCP_NODELAY on latency-sensitive TCP hops #2220 branch in 08cac6f9.
In fairness to both authors: the AGENTS.md guidance ships with#2220 and is not on main yet, so there was no in-repo rule to follow. That is the argument for the lint rather than against it — a prose convention is discovered at review time, by a reviewer who has to remember it, and only if the reviewer is looking at the right hunk. Both of these were caught by a human re-grepping the tree after a merge, which does not scale.
Proposed Design
Add a workspace-root clippy.toml (none exists today; clippy config lives only in Cargo.toml [workspace.lints.clippy], which cannot express disallowed-methods):
disallowed-methods = [
{ path = "tokio::net::TcpStream::connect", reason = "use openshell_core::net::connect_tcp_nodelay_best_effort so Nagle is disabled on the dialed stream" },
{ path = "std::net::TcpStream::connect", reason = "use openshell_core::net::connect_std_tcp_nodelay_best_effort, or call set_tcp_nodelay_best_effort after TcpStream::from_std" },
]
clippy::disallowed_methods is warn-by-default but inert until configured, so adding the file is what turns it on. tasks/rust.toml:14-15 runs cargo clippy --workspace --all-targets -- -D warnings, so this fails CI on violation, and it covers test targets too. The second clippy invocation lints the separate e2e/rust workspace but runs from the repo root, so the root clippy.toml applies there as well.
Disposition of existing call sites
A current sweep finds 54 TcpStream::connect call expressions in crates/ and e2e/: 13 production, 41 test-side (in-file #[cfg(test)] modules plus tests/ and e2e/). The 13 production sites break down as:
Disposition
Sites
Action
Already correct async dials — dial + set_tcp_nodelay_best_effort on the next line
8
Reroute to connect_tcp_nodelay_best_effort; the raw call collapses into openshell-core
netns dials — std::net::TcpStream::connect inside a setns worker thread, converted with from_std, then nodelay
The netns pair is the only real design question. Both must dial inside the network namespace on a blocking thread after setns, so the async helper cannot be used. Two options:
Add a blocking sibling connect_std_tcp_nodelay_best_effort(addrs) -> io::Result<std::net::TcpStream> to openshell_core::net, keeping the allow-list at one site. Preferred — it makes the std path a first-class supported case rather than an exception.
Two targeted #[allow(clippy::disallowed_methods)] with comments. Cheaper, but each allow is a place the convention can rot.
For the 41 test-side sites, AGENTS.md already says nodelay on test TCP streams is "fine and preferred", so the default is to reroute them to the helper rather than blanket-allow. Tests that only probe reachability get an #[allow] on the same terms as production probes. Clippy has no per-target config for disallowed-methods, so the alternative — exempting tests — would mean module-level allows on roughly 15 files, which is the same edit count with none of the benefit.
Known coverage gap — state it explicitly
This lint guards the dial path only. Accepted sockets need nodelay too and never go through connect; there are 10 production .accept() sites today. The issue should not claim full coverage. A follow-up could add an accept_tcp_nodelay_best_effort wrapper and disallow TcpListener::accept, but that is more invasive and belongs in its own change. Note the tonic-served gRPC listeners (Kubernetes and VM drivers) are already covered — tonic 0.14 defaults server tcp_nodelay to true.
Definition of done
clippy.toml added with both TcpStream::connect paths and actionable reason strings
All 13 production sites resolved by reroute or documented #[allow]
Test-side sites resolved
mise run ci green
AGENTS.md "Network Sockets" section updated to say the rule is lint-enforced, and to name the accept-path gap the lint does not cover
Alternatives Considered
Documentation only (status quo after #2220). This is what we have, and the two post-merge fixes above are the evidence it is insufficient on its own. Keep the docs — they explain why — but they are not a gate.
A grep-based CI check instead of clippy. Would catch accepted sockets too, since it is not tied to a method path. Rejected as the primary mechanism: it cannot distinguish a dial whose stream is used for real I/O from one that is dropped, has no in-editor feedback, and duplicates a lint the toolchain already provides with a per-site reason. Worth revisiting only if the accept-path gap turns out to matter more than the dial path.
#[deny] on the crate roots rather than workspace clippy.toml.disallowed-methods is config, not a lint level, so this is not actually available — the path list has to live in clippy.toml.
Do it inside #2220. Rejected there and still the right call: clippy.toml is workspace-global, so turning the lint on forces a disposition for all 54 sites in the same review as the performance change. Splitting keeps #2220 reviewable and gives this work its own reviewed decisions.
Agent Investigation
Swept the merge of origin/main (eb380d71) into the #2220 branch:
54 TcpStream::connect call expressions in crates/ + e2e/; 13 production, 41 test-side. Classified by locating the first #[cfg(test)] in each file, so in-file test modules are counted as test-side.
Confirmed origin/main has zero nodelay calls in upstream_proxy.rs, sdk/transport.rs, and sdk/edge_tunnel.rs — i.e. the drift described above is present on main right now and is fixed only on the perf(net): set TCP_NODELAY on latency-sensitive TCP hops #2220 branch.
No clippy.toml exists in the repo today.
tasks/rust.toml:14-15 confirms --all-targets -- -D warnings for both the main workspace and e2e/rust.
Depends on #2220, which delivers connect_tcp_nodelay_best_effort — the single central dial helper this lint redirects contributors to. This work should land after it.
Checklist
I've reviewed existing issues and the architecture docs
This is a design proposal, not a "please build this" request
Problem Statement
PR #2220 centralizes TCP_NODELAY into
openshell_core::net(set_tcp_nodelay_best_effort,connect_tcp_nodelay_best_effort) and adds a snippet toAGENTS.mdtelling contributors to use them. Nothing enforces it. A newTcpStream::connecton a latency-sensitive path is a silent regression: the code works, tests pass, and the ~40 ms delayed-ACK stall only shows up as "exec feels laggy" much later.This is not hypothetical. During the three weeks #2220 has been open, one merge from
mainbrought two independent instances:77e5c322(feat(sandbox,gateway): route sandbox egress through corporate HTTP proxy #2245, corporate HTTP proxy, 2026-07-24) added a production dial atcrates/openshell-supervisor-network/src/upstream_proxy.rs:801with no nodelay. Patched on the perf(net): set TCP_NODELAY on latency-sensitive TCP hops #2220 branch in95accab9.main'scrates/openshell-sdk/src/transport.rsandcrates/openshell-sdk/src/edge_tunnel.rsstill contain zero nodelay calls on their production socket paths. Patched on the perf(net): set TCP_NODELAY on latency-sensitive TCP hops #2220 branch in08cac6f9.In fairness to both authors: the
AGENTS.mdguidance ships with #2220 and is not onmainyet, so there was no in-repo rule to follow. That is the argument for the lint rather than against it — a prose convention is discovered at review time, by a reviewer who has to remember it, and only if the reviewer is looking at the right hunk. Both of these were caught by a human re-grepping the tree after a merge, which does not scale.Proposed Design
Add a workspace-root
clippy.toml(none exists today; clippy config lives only inCargo.toml [workspace.lints.clippy], which cannot expressdisallowed-methods):clippy::disallowed_methodsis warn-by-default but inert until configured, so adding the file is what turns it on.tasks/rust.toml:14-15runscargo clippy --workspace --all-targets -- -D warnings, so this fails CI on violation, and it covers test targets too. The second clippy invocation lints the separatee2e/rustworkspace but runs from the repo root, so the rootclippy.tomlapplies there as well.Disposition of existing call sites
A current sweep finds 54
TcpStream::connectcall expressions incrates/ande2e/: 13 production, 41 test-side (in-file#[cfg(test)]modules plustests/ande2e/). The 13 production sites break down as:set_tcp_nodelay_best_efforton the next lineconnect_tcp_nodelay_best_effort; the raw call collapses intoopenshell-corestd::net::TcpStream::connectinside asetnsworker thread, converted withfrom_std, then nodelaysupervisor-process/src/supervisor_session.rs:740,supervisor-process/src/ssh.rs:659)openshell-core/src/net.rs:304)#[allow]with reason; this is the one legitimate raw dialcli/src/ssh.rs:493,e2e/rust/src/harness/port.rs:27)#[allow]with a reason commentThe netns pair is the only real design question. Both must dial inside the network namespace on a blocking thread after
setns, so the async helper cannot be used. Two options:connect_std_tcp_nodelay_best_effort(addrs) -> io::Result<std::net::TcpStream>toopenshell_core::net, keeping the allow-list at one site. Preferred — it makes the std path a first-class supported case rather than an exception.#[allow(clippy::disallowed_methods)]with comments. Cheaper, but each allow is a place the convention can rot.For the 41 test-side sites,
AGENTS.mdalready says nodelay on test TCP streams is "fine and preferred", so the default is to reroute them to the helper rather than blanket-allow. Tests that only probe reachability get an#[allow]on the same terms as production probes. Clippy has no per-target config fordisallowed-methods, so the alternative — exempting tests — would mean module-level allows on roughly 15 files, which is the same edit count with none of the benefit.Known coverage gap — state it explicitly
This lint guards the dial path only. Accepted sockets need nodelay too and never go through
connect; there are 10 production.accept()sites today. The issue should not claim full coverage. A follow-up could add anaccept_tcp_nodelay_best_effortwrapper and disallowTcpListener::accept, but that is more invasive and belongs in its own change. Note the tonic-served gRPC listeners (Kubernetes and VM drivers) are already covered — tonic 0.14 defaults servertcp_nodelaytotrue.Definition of done
clippy.tomladded with bothTcpStream::connectpaths and actionablereasonstrings#[allow]mise run cigreenAGENTS.md"Network Sockets" section updated to say the rule is lint-enforced, and to name the accept-path gap the lint does not coverAlternatives Considered
Documentation only (status quo after #2220). This is what we have, and the two post-merge fixes above are the evidence it is insufficient on its own. Keep the docs — they explain why — but they are not a gate.
A grep-based CI check instead of clippy. Would catch accepted sockets too, since it is not tied to a method path. Rejected as the primary mechanism: it cannot distinguish a dial whose stream is used for real I/O from one that is dropped, has no in-editor feedback, and duplicates a lint the toolchain already provides with a per-site
reason. Worth revisiting only if the accept-path gap turns out to matter more than the dial path.#[deny]on the crate roots rather than workspaceclippy.toml.disallowed-methodsis config, not a lint level, so this is not actually available — the path list has to live inclippy.toml.Do it inside #2220. Rejected there and still the right call:
clippy.tomlis workspace-global, so turning the lint on forces a disposition for all 54 sites in the same review as the performance change. Splitting keeps #2220 reviewable and gives this work its own reviewed decisions.Agent Investigation
Swept the merge of
origin/main(eb380d71) into the #2220 branch:TcpStream::connectcall expressions incrates/+e2e/; 13 production, 41 test-side. Classified by locating the first#[cfg(test)]in each file, so in-file test modules are counted as test-side.origin/mainhas zero nodelay calls inupstream_proxy.rs,sdk/transport.rs, andsdk/edge_tunnel.rs— i.e. the drift described above is present onmainright now and is fixed only on the perf(net): set TCP_NODELAY on latency-sensitive TCP hops #2220 branch.clippy.tomlexists in the repo today.tasks/rust.toml:14-15confirms--all-targets -- -D warningsfor both the main workspace ande2e/rust.Depends on #2220, which delivers
connect_tcp_nodelay_best_effort— the single central dial helper this lint redirects contributors to. This work should land after it.Checklist