Repository navigation
Stream cmuxd-remote bootstrap upload over ssh instead of scp - #6829
Closed
austinywang wants to merge 12 commits into
Closed
austinywang wants to merge 12 commits into
austinywang wants to merge 12 commits into
Conversation
Regression coverage for #6207. The cmuxd-remote bootstrap upload uses scp, whose path semantics frequently differ from the interactive SSH session (chroot/jailed sftp subtrees), so a destination valid over SSH can be unreachable or resolve differently under scp. These two tests now assert the binary is streamed over the SSH channel itself (`ssh -T … 'cat > tmp'`) and that scp is never invoked. They fail against the current scp-based upload; the following commit makes them pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes #6207. The cmuxd-remote bootstrap uploaded the daemon binary with scp. scp/sftp sessions are frequently chrooted or jailed to a subtree that differs from the interactive SSH session, so the absolute `$HOME/.cmux/...` destination that is valid over SSH can be unreachable — or resolve to the wrong place — under scp. Users on such hosts hit "dest open ... No such file or directory" and the bootstrap never completes. Upload the binary by piping it over the SSH channel itself (`ssh -T … 'cat > tmp'`) instead. Because it travels over the same transport as every other bootstrap command, the remote path semantics are guaranteed to match. `-T` suppresses any forced pseudo-terminal so tty line-ending translation cannot corrupt the binary stream. The mkdir-before and atomic chmod+mv finalize steps are unchanged. The drag-and-drop file uploads (`uploadDroppedFilesLocked`, `DetectedSSHSession`) keep scp: they target `/tmp` (not the jailed home subtree) and may transfer arbitrarily large files, which scp streams from disk rather than buffering in memory. Also harden RemoteSessionProcessRunner's stdin handling, now that it carries a multi-MB binary: write on a background thread (so the timeout/termination path stays authoritative), suppress SIGPIPE on the fd, and loop over partial writes so a remote that closes the channel early surfaces as a nonzero exit status instead of crashing the app. Previously stdin was only ever a few tiny bytes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR streams remote daemon uploads over SSH instead of scp, changes process stdin handling to asynchronous writes with ChangesRemote SSH stdin streaming and upload
Sequence Diagram(s)sequenceDiagram
participant uploadRemoteDaemonBinaryLocked as "RemoteSessionCoordinator.uploadRemoteDaemonBinaryLocked"
participant sshExec as "sshExec"
participant remoteCat as "sh -c 'cat > <remoteTempPath>'"
participant finalize as "chmod and mv"
uploadRemoteDaemonBinaryLocked->>sshExec: stdin = Data(contentsOf: local daemon binary)
sshExec->>remoteCat: ssh -T ... shell command
remoteCat-->>sshExec: writes the uploaded bytes
uploadRemoteDaemonBinaryLocked->>finalize: chmod then mv temp file into place
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR replaces the
Confidence Score: 5/5The change is safe to merge: it narrows a well-understood path mismatch by routing the binary through the same SSH transport used for every other bootstrap command, and the runner hardening is carefully scoped with correct fd lifetime management. The core upload change is straightforward: reads the binary once, pipes it over an SSH channel with -T, and falls back to the same atomic chmod && mv finalize. The runner hardening (dup/SIGPIPE/partial-write loop) covers the only new failure mode — a multi-MB write through a pipe — and the synchronous fallback on dup failure ensures stdin is never silently dropped. The two previously-flagged issues (0-byte daemon install on dup failure, and duplicate NSError code 31) were addressed in the commits cited in the thread replies before this review ran. Both bootstrap tests are now deterministic, and the large-stdin test directly exercises the backpressured writer path. No new blocking primitives, ambient globals, or test seams appear in production sources. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant B as bootstrapDaemonLocked
participant U as uploadRemoteDaemonBinaryLocked
participant R as RemoteSessionProcessRunner.run
participant BG as Background Writer (DispatchQueue.utility)
participant SSH as ssh process
participant Remote as Remote Shell (sh -c cat > tmp)
B->>U: localBinary URL
U->>U: Data(contentsOf: localBinary)
U->>R: "run(stdin=binaryData, timeout=120)"
R->>SSH: Process.run()
R->>BG: DispatchQueue.global.async writeStdinAndClose(binaryData, fd)
BG->>SSH: Darwin.write(fd) loop partial writes EINTR retry
SSH-->>Remote: binary bytes over SSH channel
Remote-->>SSH: exit 0
BG->>BG: Darwin.close(fd) on EPIPE or completion
SSH-->>R: process exits exitSemaphore.signal()
R-->>U: "RemoteCommandResult(status=0)"
U->>U: sshExec chmod 755 tmp and mv tmp dest
U-->>B: success
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant B as bootstrapDaemonLocked
participant U as uploadRemoteDaemonBinaryLocked
participant R as RemoteSessionProcessRunner.run
participant BG as Background Writer (DispatchQueue.utility)
participant SSH as ssh process
participant Remote as Remote Shell (sh -c cat > tmp)
B->>U: localBinary URL
U->>U: Data(contentsOf: localBinary)
U->>R: "run(stdin=binaryData, timeout=120)"
R->>SSH: Process.run()
R->>BG: DispatchQueue.global.async writeStdinAndClose(binaryData, fd)
BG->>SSH: Darwin.write(fd) loop partial writes EINTR retry
SSH-->>Remote: binary bytes over SSH channel
Remote-->>SSH: exit 0
BG->>BG: Darwin.close(fd) on EPIPE or completion
SSH-->>R: process exits exitSemaphore.signal()
R-->>U: "RemoteCommandResult(status=0)"
U->>U: sshExec chmod 755 tmp and mv tmp dest
U-->>B: success
Reviews (8): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+194
to
+201
| let writeDescriptor = Darwin.dup(pipe.fileHandleForWriting.fileDescriptor) | ||
| try? pipe.fileHandleForWriting.close() | ||
| if writeDescriptor >= 0 { | ||
| _ = Darwin.fcntl(writeDescriptor, F_SETNOSIGPIPE, 1) | ||
| DispatchQueue.global(qos: .utility).async { | ||
| Self.writeStdinAndClose(stdin, to: writeDescriptor) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
dup fails
Darwin.dup can fail (returns -1) if the process is out of file descriptors. When it does, the write end of the stdin pipe is already closed (line 195 ran first), but the if writeDescriptor >= 0 guard simply skips the background writer without throwing. The spawned ssh process immediately reads EOF, cat > remoteTempPath completes successfully with 0 bytes, uploadResult.status == 0 passes, and the chmod && mv finalize runs — atomically installing an empty cmuxd-remote on the remote host. The failure only surfaces later as a cryptic "failed to start remote daemon" from the hello handshake, with no trace of the root cause.
Contributor
Author
There was a problem hiding this comment.
Fixed in 263ba76. The async path now only closes the original write handle once the dup succeeds; if dup fails it falls back to a synchronous write on the original handle (writeStdinSynchronouslyAndClose), so stdin is never silently emptied and a 0-byte daemon can't be installed.
Contributor
Author
There was a problem hiding this comment.
Verified on current head cdef145: dup failure no longer drops stdin. RemoteSessionProcessRunner falls back to writeStdinSynchronouslyAndClose on the original pipe handle, so the SSH cat upload cannot succeed with an implicit empty stream from a skipped writer.
— Claude Code
Comment on lines
334
to
341
| let binaryData: Data | ||
| do { | ||
| binaryData = try Data(contentsOf: localBinary) | ||
| } catch { | ||
| throw NSError(domain: "cmux.remote.daemon", code: 31, userInfo: [ | ||
| NSLocalizedDescriptionKey: "failed to read cmuxd-remote for upload: \(error.localizedDescription)", | ||
| ]) | ||
| } |
Contributor
There was a problem hiding this comment.
Both the new "failed to read cmuxd-remote for upload" branch and the pre-existing "failed to upload cmuxd-remote" branch throw NSError(domain: "cmux.remote.daemon", code: 31, ...). Any caller that routes or counts on the error code (retry logic, telemetry, tests) cannot distinguish a local file-read failure from an SSH upload failure. Code 33 is free (30 = mkdir, 31 = upload SSH exit, 32 = finalize).
Suggested change
| let binaryData: Data | |
| do { | |
| binaryData = try Data(contentsOf: localBinary) | |
| } catch { | |
| throw NSError(domain: "cmux.remote.daemon", code: 31, userInfo: [ | |
| NSLocalizedDescriptionKey: "failed to read cmuxd-remote for upload: \(error.localizedDescription)", | |
| ]) | |
| } | |
| let binaryData: Data | |
| do { | |
| binaryData = try Data(contentsOf: localBinary) | |
| } catch { | |
| throw NSError(domain: "cmux.remote.daemon", code: 33, userInfo: [ | |
| NSLocalizedDescriptionKey: "failed to read cmuxd-remote for upload: \(error.localizedDescription)", | |
| ]) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Author
There was a problem hiding this comment.
Fixed in ab6f5ee. The local Data(contentsOf:) failure now propagates its system-localized Cocoa error instead of being re-wrapped in a second code: 31 NSError, so the read failure and the remote upload failure are no longer conflated under one code.
Contributor
Author
There was a problem hiding this comment.
Verified on current head cdef145: this is no longer using a duplicate NSError code for local read failure. The bootstrap path now lets Data(contentsOf:) throw the local Cocoa file error and keeps code 31 for SSH upload failure only.
— Claude Code
…ootstrap-upload-method-is
Mechanical `python3 scripts/swift_file_length_budget.py --write-budget` after merging origin/main. Accounts for the +44 lines in WorkspaceRemoteConnectionTests.swift (rewritten bootstrap-upload tests) and +8 lines in RemoteSessionCoordinator+Bootstrap.swift (ssh-pipe upload). Other entries are the script tightening budgets for files that shrank on main since the last refresh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pure helpers belong at file scope, not as a private static func attached to a type (avoids the static-as-namespace pattern). Behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
testPersistentPTYBootstrapReinstallsOldDaemonMissingPTYCapability previously let buildLocalDaemonBinary fall through to the dev-only `go build` path, which needs a Go toolchain that the app-host CI runner does not have — so the upload was never reached and the test timed out at ~2s (it failed identically on main, masked by CI's retry/expected-failure handling). Supply the daemon binary via the explicit-override env var instead, so the binary is a real on-disk file. The bootstrap still reaches the hello handshake, sees the missing persistent-PTY capability, and reinstalls; the test captures that second (reinstall) upload and asserts it streams over ssh, not scp (#6207). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mechanical scripts/swift_file_length_budget.py --write-budget after the deterministic reinstall test rewrite (+20 lines in WorkspaceRemoteConnectionTests.swift). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 2053-2058: The SSH upload test setup in
WorkspaceRemoteConnectionTests should also assert that the upload command
includes the -T flag, not just cat > and scp rejection. Update the existing
upload command capture/assertion around the remoteProcessScript and
uploadCommand checks so the reinstall test and the other SSH upload regression
tests explicitly verify -T is present in the arguments for the upload path,
using the same assertion pattern already used elsewhere in this file.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift`:
- Around line 194-201: The stdin setup path in RemoteSessionProcessRunner should
not silently continue when dup() or F_SETNOSIGPIPE fails. Update the
writeDescriptor setup around writeStdinAndClose/stdin to follow the same failure
handling used for launch and cancellation errors: if dup() returns an invalid
descriptor or fcntl(F_SETNOSIGPIPE) fails, terminate the running process, call
finishCaptureAndCloseReadHandles(), and throw an appropriate POSIXError instead
of proceeding with an EOF or SIGPIPE-prone session.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator`+Bootstrap.swift:
- Around line 338-340: The upload-read failure text in
RemoteSessionCoordinator+Bootstrap is user-facing but is being inserted directly
into NSLocalizedDescriptionKey, so it must be localized. Update the NSError
construction in the bootstrap path to use String(localized:defaultValue:) for
the message, and add the corresponding key/value entries to
Resources/Localizable.xcstrings for the supported locales.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2346113-1cc4-4fba-834e-302d89930cb5
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+Bootstrap.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionProcessRunnerTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
Reverts the testDaemonBootstrapUpload... method rename. The cmuxTests shards
are a sha256-ordered bin-pack over test identifiers
(scripts/ci/cmux_unit_test_shard.py); renaming one method reshuffles unrelated
GUI suites across shards and relocated BrowserPanelWebViewLifecycleTests into a
shard ordering where its pre-existing flaky WebKit SIGSEGV
(testBackgroundPreloadIsConsumedByInitialNavigation) fails xcodebuild's
crash-recovery. That same crash occurs on main too, but main's shard layout
recovers it ("0 unexpected"); the reshuffled layout did not.
Keeping the original identifier makes the cmuxTests shard distribution
byte-identical to main, so this PR no longer surfaces that unrelated flake. The
test body is unchanged and still verifies the ssh-pipe upload from #6207.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex review caught a real failure mode in the async stdin path: if dup() of the pipe write end fails (e.g. fd exhaustion), the code closed the original handle and let the child read an empty stream. For the cmuxd-remote upload that means `cat > tmp` succeeds with 0 bytes and chmod/mv installs an empty daemon, masking the real local resource error. Only close the original handle once the duplicate is secured; if dup fails, fall back to a synchronous write on the original handle so required stdin is never silently lost. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Let a local Data(contentsOf:) failure propagate its system-localized Cocoa error instead of wrapping it in a second NSError reusing daemon code 31 (Greptile: duplicate code; CodeRabbit: localize new message). No custom code, no new English diagnostic. - Both bootstrap-upload regression tests now assert the ssh invocation includes -T (disables pseudo-terminal allocation so the binary stream is not mangled), per CodeRabbit. The dup-failure silent-empty-stdin concern (Greptile P1 / CodeRabbit) was already fixed by the synchronous fallback in the previous commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ootstrap-upload-method-is # Conflicts: # .github/swift-file-length-budget.tsv
…ootstrap-upload-method-is
lawrencecchen
added
the
stale-revisit
This branch was successfully 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
3 participants
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.
Fixes #6207.
Problem
The
cmux sshbootstrap uploads thecmuxd-remotebinary withscp. But scp/sftp sessions are frequently chrooted or jailed to a subtree that differs from the interactive SSH session, so the absolute$HOME/.cmux/bin/cmuxd-remote/<version>/<os>-<arch>/cmuxd-remotedestination that is valid over SSH can be unreachable — or resolve to the wrong place — under scp. Affected users (e.g. HPC logins with a jailed sftp subsystem) hit:and the bootstrap never completes. This is the long-standing fragility tracked in #3689 / #3880.
Fix
Upload the binary by piping it over the SSH channel itself instead of scp, exactly as the reporter suggested:
Because the upload travels over the same transport as every other bootstrap command (probe,
mkdir -p, finalizechmod && mv,serve --stdiohello), the remote path semantics are guaranteed to match the interactive SSH session — no scp/sftp jail mismatch is possible.-Tsuppresses any forced pseudo-terminal so tty line-ending translation cannot corrupt the binary stream. Themkdir -pbefore and the atomicchmod 755 && mvfinalize are unchanged, so the install stays atomic.Why drag-and-drop uploads still use scp
uploadDroppedFilesLockedandDetectedSSHSessionkeep scp deliberately: they target/tmp(not the jailed home subtree the issue is about) and may transfer arbitrarily large user files, which scp streams from disk rather than buffering in memory.Runner hardening
RemoteSessionProcessRunnernow carries a multi-MB binary through stdin (previously stdin was only ever a few bytes), so its stdin handling is hardened:F_SETNOSIGPIPE) and loop over partial writes, so a remote that closes the channel early (e.g. a failed redirect target) surfaces as a nonzero exit status instead of crashing the app.Tests
Two-commit red/green:
ssh … 'cat >'(with the absolute$HOMEpath and the binary bytes on stdin) and that scp is never invoked. They fail against the scp-based upload.RemoteSessionProcessRunnertest that pipes a ~1.1 MiB stdin payload through/bin/catto exercise the backpressured partial-write loop.Localization
No user-facing UI strings added. The one new
NSErrordetail (failed to read cmuxd-remote for upload) follows the existing bare-English diagnostic convention shared by every sibling daemon-bootstrap error in this file; the localized layer is the separateRemoteDaemonStrings/RemoteSessionStringsseam, which is unchanged.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Stream the
cmuxd-remotebootstrap upload over SSH stdin instead of scp to avoid jailed scp/sftp path mismatches. Fixes failed installs on chrooted sftp hosts and aligns paths with the interactive SSH session (fixes #6207).Bug Fixes
ssh -T … 'sh -c "cat > <tmp>"', thenmkdir -pand atomicchmod && mv(120s timeout).-Tto prevent binary stream mangling; scp is never used for daemon upload.Data(contentsOf:)as Cocoa file errors (no custom wrapping).cat >SSH pipe with-T, exact daemon bytes are sent; reinstall flow also streams over SSH; deterministic reinstall viaCMUX_REMOTE_DAEMON_BINARY; large-stdin path covered; original test name kept. Drag‑and‑drop to/tmpstill uses scp.Refactors
RemoteSessionProcessRunnerstdin: background writer withF_SETNOSIGPIPE, partial‑write loop, and dup()-failure synchronous fallback so input is never dropped. Writer moved to a file‑scope helper.Written for commit cdef145. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes