Skip to content

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

Description

@purp

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:

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 perf(net): set TCP_NODELAY on latency-sensitive TCP hops #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 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    state:triage-neededOpened without agent diagnostics and needs triage

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions