NVIDIA / NVIDIA/OpenShell

feat(build): enforce TCP_NODELAY at TCP dial sites with clippy::disallowed_methods

Open
#2,577 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

state:triage-needed
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 at crates/openshell-supervisor-network/src/upstream_proxy.rs:801 with no nodelay. Patched on the #2220 branch in 95accab9.
  • 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 #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 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:

  1. 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.
  2. 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.
  • 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/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 #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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.