feat(build): enforce TCP_NODELAY at TCP dial sites with clippy::disallowed_methods
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 8.7k
- Forks
- 1.3k
- Avg merge
- 2d 11h
- Merged PRs (30d)
- 253
Description
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 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:
77e5c322(#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 #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 #2220 branch in08cac6f9.
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 |
2 (supervisor-process/src/supervisor_session.rs:740, supervisor-process/src/ssh.rs:659) |
Needs a decision — see below |
| Helper internals | 1 (openshell-core/src/net.rs:304) |
#[allow] with reason; this is the one legitimate raw dial |
| Reachability probes — stream dropped immediately, no I/O, nodelay is meaningless | 2 (cli/src/ssh.rs:493, e2e/rust/src/harness/port.rs:27) |
#[allow] with a reason comment |
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>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. - 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.tomladded with bothTcpStream::connectpaths and actionablereasonstrings - All 13 production sites resolved by reroute or documented
#[allow] - Test-side sites resolved
-
mise run cigreen -
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::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. - All 10 production dials that carry real traffic currently set nodelay on the #2220 branch. The 3 that do not are intentional: the helper's own dial and two reachability probes.
- Confirmed
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 #2220 branch. - No
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
- I've reviewed existing issues and the architecture docs
- This is a design proposal, not a "please build this" request
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with tasks/rust.toml:14-15 and the existing network helpers in crates/openshell-core/src/net.rs, then inspect the listed TcpStream::connect sites, including supervisor-process/src/supervisor_session.rs, supervisor-process/src/ssh.rs, cli/src/ssh.rs, and e2e/rust/src/harness/port.rs. Resolve the netns design choice, add the workspace clippy.toml, update AGENTS.md, and run mise run ci until all production and test-side sites are handled.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- build-system, ci-cd, tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100