Skip to content

fix(server): device-host SSH tunnels exit when the server dies - #17448

Open
kcazin wants to merge 1 commit into
pingdotgg:mainfrom
kcazin:fix/device-host-tunnel-orphan
Open

kcazin wants to merge 1 commit into
pingdotgg:mainfrom
kcazin:fix/device-host-tunnel-orphan

Conversation

@kcazin

@kcazin kcazin commented Oct 9, 2026

Copy link
Copy Markdown

Problem

The SSH device-host tunnel runs ssh -N -L … with stdin ignored. Effect's Node spawner detaches children on POSIX, so the tunnel sits in its own session, and only the host's stop finalizer ever ends it. When the server dies before that finalizer runs, the tunnel is orphaned for good, keeping its loopback listeners and an SSH session to the device host. The new server never knows about it.

The service launcher hits this on self-update: it SIGKILLs the old server 5 s after SIGTERM, and device-host teardown only starts after orchestration's shutdown reconciliation. A crash, an OOM kill or kill -9 hits it too. Full report, timeline and live evidence: #17438.

Change

The tunnel spawn now:

  • uses -T instead of -N;
  • runs a remote sh -c 'exec cat >/dev/null', quoted with the existing quoteRemoteArg;
  • gets stdin: "pipe" instead of "ignore".

Nothing ever writes to that stdin, and the server holds the pipe's only write end, since libuv pipes are close-on-exec. When the server exits for any reason, including SIGKILL, the kernel closes that end. The remote cat then sees EOF, and ssh exits once its forwarded connections close.

The graceful stop path is unchanged: it closes the scope and kills the process group. So are the forwards, ExitOnForwardFailure, ServerAlive*, bootstrap, and the detached default.

Scope and approval

Fixes #17438, which maintainers have triaged (bug, via-triage). The maintainer-bot analysis on the issue lists this direction: "tying ssh lifetime to the server (piped stdin, no -N)".

I chose it over the other directions on that list:

  • Closing tunnels earlier in shutdown, or bounding stop: only helps graceful shutdown, not SIGKILL, OOM or a crash.
  • detached: false: doesn't help, because the launcher signals only the server pid.
  • A launcher group or cgroup kill: would break agents intentionally surviving updates (bootService.ts).
  • A startup pid ledger: needs more machinery and only cleans up at the next boot.

This lifetime coupling is enforced by the kernel and covers every way the server can die.

Out of scope, for separate PRs if wanted:

  • The environment tunnel in packages/ssh/src/tunnel.ts (same orphan class, still -n -N).
  • Shutdown ordering and the 45 s remote stop.
  • The ensureReady interrupt path noted on the issue.

Note on #17440 (ControlMaster opt-out, same spawn): the two fixes are logically independent. Their edits to SshDeviceHost.test.ts are on adjacent lines, though, so whichever lands second needs a trivial rebase that keeps the tunnel detection on -L.

Verification

Focused test. I added "ends the tunnel when the server's end of its stdin pipe closes" to SshDeviceHost.test.ts; it is skipped on Windows because it runs sh. The fake spawner runs the tunnel's remote command locally through the real Node spawner. The test then:

  • asserts the tunnel requests a stdin pipe;
  • writes 1 MiB through it, which only completes while the command keeps reading, because a command that has exited fails the write with EPIPE;
  • ends stdin, as the server's death does, and awaits exit code 0.

It uses no sleeps or timeouts. The existing test now detects the tunnel by -L, since -N is gone.

  • With main's SshDeviceHost.ts: AssertionError: expected 'ignore' to be 'pipe', Tests 1 failed | 1 passed (2).
  • With the fix: Tests 2 passed (2), 5 of 5 runs.
  • vp check on both files: clean. vp run --filter t3 typecheck: 0 errors.

Live check. Linux, OpenSSH 10.5p1, against a real macOS device host. A Node parent spawns the tunnel the way the server does: detached, the same -o options, and a -L forward. It is then SIGKILLed.

== old tunnel (-N, stdin ignored)
ssh before kill: pid=2588459 ppid=2588450 pgid=2588459 sid=2588459
kill -9 parent at 17:30:12.381
ssh SURVIVED 5 s later: pid=2588459 ppid=1 (ssh)
listening on 47121: yes

== new tunnel (-T, stdin pipe, remote cat)
ssh before kill: pid=2588535 ppid=2588525 pgid=2588535 sid=2588535
remote cat processes while up: 1
kill -9 parent at 17:30:18.668
ssh exited at 17:30:18.872; listening on 47122: no
remote cat processes after: 0

Not checked:

  • A full end-to-end self-update under t3 __service-launcher with this build.
  • Windows or macOS as the server host.
  • Remote login shells other than the device host's own.

The remote sh -c form is the same one bootstrap already relies on.

Model and harness: Claude Opus 5.5 (claude-opus-5-5) in Claude Code, via t3 triage. Adversarial review by Claude Fable 5.1 before opening.

🤖 Generated with Claude Code

The device-host tunnel ran `ssh -N` with stdin ignored in its own
session, so only the host's stop finalizer could end it. When the
server died before that finalizer ran (the service launcher SIGKILLs
it 5 s into a self-update; also OOM or a crash), the tunnel was
orphaned with its forwards and SSH session. Give the tunnel a stdin
pipe and a remote command that exits on EOF: the server holds the only
write end, so once it is gone ssh exits as soon as its forwarded
connections close.

Fixes pingdotgg#17438

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 9, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 66b12ec

Macroscope's review found this PR approvable — This is a focused SSH tunnel lifecycle fix that ties the remote command to the server’s stdin lifetime, with existing forwarding behavior otherwise preserved. The accompanying test directly exercises tunnel termination when stdin closes, and no broader production surfaces or configuration defaults are changed.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 9, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a42b8dab-6613-448f-be56-59a71fad72ae

📥 Commits

Reviewing files that changed from the base of the PR and between 43f8a8d and 66b12ec.


📒 Files selected for processing (2)
  • apps/server/src/device/SshDeviceHost.test.ts
  • apps/server/src/device/SshDeviceHost.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.



📝 Walkthrough

Walkthrough

The SSH forwarding process now runs a remote command that reads from piped stdin. The test suite checks that the command exits after receiving EOF.

Changes

SSH Tunnel Lifetime

Layer / File(s) Summary
Pipe-driven tunnel exit
apps/server/src/device/SshDeviceHost.ts, apps/server/src/device/SshDeviceHost.test.ts
The forwarding process runs a remote command that reads stdin, with piped stdin replacing ignored stdin. The hub and optional daemon forwarding arguments remain unchanged. A live test checks that the command exits successfully after receiving 1 MiB and then stopping the host. The mock identifies tunnel commands by -L.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant SshDeviceHost
  participant SSH
  participant RemoteShell
  participant Cat
  SshDeviceHost->>SSH: Start forwarding process with piped stdin
  SSH->>RemoteShell: Run remote shell command
  RemoteShell->>Cat: Execute cat reading stdin
  SshDeviceHost->>SSH: Close stdin when the server exits
  SSH->>Cat: Forward EOF
  Cat-->>SSH: Exit after EOF
Loading

Suggested reviewers: juliusmarminge


Merge Risk

Merge Risk: ⚪ Minimal · up to 66b12

The tunnel exited in the reported abrupt-shutdown check, and no concrete merge-blocking issue remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 66b12

The change aims to reduce orphaned SSH tunnels without broadening network access or remote-command authority. Normal cleanup remains unchanged. Confidence is limited because the added test exercises a local shell rather than real SSH forwarding during abrupt server termination.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The directly affected capability is each configured device-host SSH session and its local hub or daemon listeners. The inspected change does not enlarge destination selection, listener bindings, or credential authority; it attempts to shorten capability lifetime after server death.

Trust Boundaries and Controls

  • observed — The forwarding process retains batch-mode authentication, configured identity selection, ExitOnForwardFailure, and server-alive settings. The added EOF reader executes under the same configured remote SSH account already used for bootstrap.

Resilience and Maintainability Implications

  • observed — The added non-Windows test uses a real local shell to verify stdin consumption and successful exit after EOF. It substitutes that shell for SSH, so it does not establish listener release after SIGKILL, behavior with surviving forwarded connections, or interruption and recovery races.

Hardening Proposals

  • proposed — Validate the capability-lifetime guarantee with real SSH and abrupt parent termination, including an independently held forwarded connection. Check listener release and subsequent server recovery rather than treating remote-command exit alone as complete tunnel cleanup.



🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: device-host SSH tunnels now exit when the server dies.
Description check Passed The description is complete and focused. It explains the problem, implementation, scope, issue reference, verification results, limitations, and agent details. It also identifies untested scenarios.
Linked Issues check Passed For directly linked issue #17438, SshDeviceHost.ts changes the device-host tunnel from -N to -T, adds the remote sh -c 'exec cat >/dev/null' command, and sets stdin: "pipe". This couples SSH…
Out of Scope Changes check Passed The changed files are limited to the device-host tunnel implementation and its focused test. The implementation preserves the existing forwards, SSH keepalive options, forward-failure option, graceful…


✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Server self-update leaks the old server's SSH device-host tunnel

1 participant