Repository navigation
Conversation
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>
Contributor
ApprovabilityVerdict: Approved at 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. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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'sstopfinalizer 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 -9hits it too. Full report, timeline and live evidence: #17438.Change
The tunnel spawn now:
-Tinstead of-N;sh -c 'exec cat >/dev/null', quoted with the existingquoteRemoteArg;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
catthen sees EOF, and ssh exits once its forwarded connections close.The graceful
stoppath 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:
stop: only helps graceful shutdown, not SIGKILL, OOM or a crash.detached: false: doesn't help, because the launcher signals only the server pid.bootService.ts).This lifetime coupling is enforced by the kernel and covers every way the server can die.
Out of scope, for separate PRs if wanted:
packages/ssh/src/tunnel.ts(same orphan class, still-n -N).stop.ensureReadyinterrupt path noted on the issue.Note on #17440 (ControlMaster opt-out, same spawn): the two fixes are logically independent. Their edits to
SshDeviceHost.test.tsare 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 runssh. The fake spawner runs the tunnel's remote command locally through the real Node spawner. The test then:It uses no sleeps or timeouts. The existing test now detects the tunnel by
-L, since-Nis gone.main'sSshDeviceHost.ts:AssertionError: expected 'ignore' to be 'pipe',Tests 1 failed | 1 passed (2).Tests 2 passed (2), 5 of 5 runs.vp checkon 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
-ooptions, and a-Lforward. It is then SIGKILLed.Not checked:
t3 __service-launcherwith this build.The remote
sh -cform is the same one bootstrap already relies on.Model and harness: Claude Opus 5.5 (
claude-opus-5-5) in Claude Code, viat3 triage. Adversarial review by Claude Fable 5.1 before opening.🤖 Generated with Claude Code