Skip to content

perf(net): set TCP_NODELAY on all tunnel and proxy TCP hops#2208

Closed
purp wants to merge 1 commit into
NVIDIA:mainfrom
purp:mey-33-tcp-nodelay
Closed

perf(net): set TCP_NODELAY on all tunnel and proxy TCP hops#2208
purp wants to merge 1 commit into
NVIDIA:mainfrom
purp:mey-33-tcp-nodelay

Conversation

@purp

@purp purp commented Jul 10, 2026

Copy link
Copy Markdown

Summary

The sandbox tunnel added 4–5ms of latency to every small request/response because no socket in the path disabled Nagle's algorithm: each sub-MSS write waited on delayed ACKs at every TCP hop. This PR sets TCP_NODELAY on all latency-sensitive sockets in the tunnel and egress-proxy paths.

Changes

  • Gateway (openshell-server): set nodelay on connections accepted by the public listener — covers gRPC relay frames and WS tunnel writes
  • CLI (openshell-cli): edge tunnel local accept loop and the WebSocket's underlying TCP stream; InsecureTlsConnector (matching tonic's default connector behavior); service_forward_tcp accepted sockets
  • Supervisor (openshell-supervisor-process): connect_in_netns — the direct-tcpip final hop to services inside the sandbox — on both the netns and direct branches
  • Egress proxy (openshell-supervisor-network): accepted sandbox connections, plus all five upstream dial sites consolidated into a new connect_upstream helper that sets nodelay
  • Untouched by design: the sandbox SSH transport rides a unix domain socket (no Nagle), and tonic gRPC client channels enable nodelay by default

Testing

  • mise run pre-commit passes
  • Unit tests added/updated (connect_in_netns_sets_tcp_nodelay, connect_upstream_sets_tcp_nodelay)
  • E2E tests added/updated (if applicable) — mise run e2e:docker smoke passes against a standalone Docker-backed gateway

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable) — no doc changes needed; internal socket-option change with no user-facing behavior or config impact

The sandbox tunnel added 4-5ms 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, egress
  proxy accepted connections, and all upstream CONNECT/HTTP dials
  (via a new connect_upstream helper)

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 MEY-33

Signed-off-by: Jim Meyer <jim@meyer4hire.com>
@purp purp requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners July 10, 2026 07:32
@copy-pr-bot

copy-pr-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@purp purp marked this pull request as draft July 10, 2026 13:57
@purp

purp commented Jul 10, 2026

Copy link
Copy Markdown
Author

Converting to draft. Found a thing while benchmarking last night and left Fable working on it ... and it got a bit excited and sent up a PR.

@purp

purp commented Jul 10, 2026

Copy link
Copy Markdown
Author

I have read the DCO document and I hereby sign the DCO.

@purp

purp commented Jul 10, 2026

Copy link
Copy Markdown
Author

I'm going to kill this PR for now and open an issue instead.

The underlying issue was revealed as I was doing some benchmarking yesterday as part of my learning and understanding process. I think it's a real issue and the fix doesn't look bad, but it feels like it should start as an issue for some discussion that might shape the approach differently. I also want to benchmark this fix and feel confident that it's a worthwhile improvement.

Apologies for the noise. Fable has a couple of new memories now and is busily reviewing its approach as well as the code. 🤖🧠

@purp purp closed this Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant