Skip to content

Fix datagram port leak, 40ms stream stalls and several pilotctl failure paths - #490

Merged
TeoSlayer merged 3 commits into
mainfrom
hosted-node-fixes
Oct 1, 2026
Merged

TeoSlayer merged 3 commits into
mainfrom
hosted-node-fixes

Conversation

@TeoSlayer

Copy link
Copy Markdown
Collaborator

Pull Request

Summary

Fixes found by running a node in a 1 GB container under load and profiling it: a port leak that stops a node dialing after ~16k datagrams, two timers that stalled every new connection and every file chunk with both ends idle, and several pilotctl paths that reported success or left files behind.

Changes

Daemon

  • Datagram source ports are released. Only closing a connection cleared an ephemeral port, and a datagram has no connection. After ~16,384 datagrams every dial failed with what looked like an unreachable peer, until restart.
  • DelayedACKTimeout 40ms → 5ms. A write held by Nagle waits for the data before it to be ACKed, and a lone or odd segment's ACK waited 40ms: 40ms on the first exchange of every connection, and one stall per 48KB file chunk (~1.5 MB/s on any link).
    • The Nagle hold itself is kept on purpose. I tried sending tails unheld and ACKing short segments at once: 150 MB/s in-process, but in a two-container lab transfers timed out, and on a stock-kernel socket buffer four concurrent 20 MB transfers took 34–40s instead of 1.4s. The stream layer's loss recovery cannot absorb that burst.
  • A dial is woken when its handshake completes (conn.DialCh) instead of on the next 10ms poll, which stays as a backstop.
  • The tunnel socket requests 4 MB kernel buffers, best effort (capped by net.core.rmem_max/wmem_max).
  • The info reply builds typed rows instead of a map per peer and connection. pilotctl send-message asks for it on every send; it was 77% of daemon CPU under send load. A test asserts the JSON is byte-identical to the maps'.

pilotctl

  • send-message exits non-zero when the receiver answers ERR ... (as send-file already did).
  • appstore install removes its unpack directory on success, failure and fatal exit. Seven installs had left seven copies, 60 MB.
  • appstore call waits up to 15s for a just-installed app's socket; a suspended app is reported as suspended.
  • received --clear also removes interrupted transfers in .partial (new cleared_partial field).
  • A dial that fails for lack of local ports says so instead of pointing at the peer.

Measured (two containers on one bridge, 1 CPU each)

Before After
Connect and exchange one message (in-process test) 55 ms 6 ms
60 MB file, either direction 31 s 3.5 s
Four concurrent 20 MB transfers 3 of 4 failed (needs pilot-protocol/dataexchange#45) 1.4 s, at both 7.5 MB and stock 212 KB rmem_max
Dial after 17,000 datagrams ephemeral ports exhausted ok

Test Plan

  • go build ./... succeeds
  • go vet ./... clean
  • go test ./pkg/... ./cmd/... ./internal/... -short and go test -parallel 4 -count=1 ./tests/ pass with GOWORK=off
  • New tests fail without the fix and pass with it: TestDatagramsDoNotExhaustEphemeralPorts, TestFirstExchangeOnAConnectionIsNotDelayed, TestLargeWritesDoNotStallOnDelayedACK (both timing tests run serially and judge the fastest run, so machine load does not flip them)
  • TestInfoRowsMarshalLikeTheMapsTheyReplaced, TestWaitForAppSocket
  • Pre-commit hooks not run locally
  • Not covered by an automated test: the install cleanup and received --clear changes (verified by hand in the lab), and behaviour over a lossy or high-latency link — the lab is a local bridge

Checklist

  • New code includes the SPDX license header
  • No secrets or credentials committed
  • go.mod / go.sum unchanged
  • CHANGELOG updated
  • Linked issue or context: none

🤖 Generated with Claude Code

…re paths

Found by running a node in a 1 GB container under load and profiling it.

Daemon
- A datagram's source port was never released: only closing a connection
  cleared an ephemeral port, and a datagram has none. After ~16k datagrams
  every dial failed until restart. The port is released once the datagram
  is sent.
- DelayedACKTimeout 40ms -> 5ms. A write held by Nagle waits for the data
  before it to be ACKed; a lone or odd segment's ACK waited 40ms. That was
  40ms on the first exchange of every connection and one stall per 48KB
  file chunk (~1.5 MB/s on any link). The hold itself stays: removing it
  bursts a full window into the peer's socket buffer and collapses under
  loss.
- A dial is woken when its handshake completes (conn.DialCh) instead of
  on the next 10ms poll; the poll remains as a backstop.
- The tunnel socket requests 4 MB kernel buffers (best effort).
- The info reply builds typed rows instead of a map per peer/connection;
  it was most of the daemon's CPU under send load. JSON is unchanged.

pilotctl
- send-message exits non-zero when the receiver answers "ERR ...".
- appstore install removes its unpack directory, on success and failure.
- appstore call waits up to 15s for a just-installed app's socket, and
  reports a suspended app as such.
- received --clear also removes interrupted transfers in .partial.
- A dial that fails for lack of local ports says so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@TeoSlayer

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (it conflicted, so most checks had not run). One conflict, in appstore call's socket check: main added the renamed-app hint in the lines this PR replaces with the socket wait. Both are kept — wait for the socket, then on failure report a renamed app, then a suspended one, then the generic message.

Re-run after the rebase with GOWORK=off: build, vet and the unit suite pass. The full integration suite has one failure, TestManualSnapshotTrigger, which binds the fixed address 127.0.0.1:18080; on the machine I ran it on another process (OrbStack) is listening there. It is unrelated to this change.

Comment thread cmd/pilotctl/appstore.go
// The bundle was unpacked into a temporary directory for this
// install alone. It was never removed, so every install — failed or
// not — left a copy of the app behind in $TMPDIR.
removeUnpacked := func() { _ = os.RemoveAll(bundleDir) }
Comment thread cmd/pilotctl/appstore.go
}
}
}
if _, serr := os.Stat(filepath.Join(appDir, ".suspended")); serr == nil {
Comment thread cmd/pilotctl/appstore.go
func waitForAppSocket(appDir, sockPath string, wait time.Duration) error {
deadline := time.Now().Add(wait)
for {
_, err := os.Stat(sockPath)
Comment thread cmd/pilotctl/appstore.go
if err == nil {
return nil
}
if _, merr := os.Stat(filepath.Join(appDir, "manifest.json")); merr != nil {
Comment thread cmd/pilotctl/appstore.go
if _, merr := os.Stat(filepath.Join(appDir, "manifest.json")); merr != nil {
return err // not installed
}
if _, serr := os.Stat(filepath.Join(appDir, ".suspended")); serr == nil {
Teo Calin and others added 2 commits October 1, 2026 18:34
`appstore audit` lists the supervisor's events (exit codes), not the
reason; the app's stderr goes to the daemon's log.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@TeoSlayer
TeoSlayer merged commit 1503f88 into main Oct 1, 2026
14 of 15 checks passed
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.

2 participants