Repository navigation
Gate remote SSH port scanning on the sidebar ports settings (#6123) - #6136
Merged
Merged
Conversation
Remote SSH workspaces keep spawning short-lived `/usr/bin/ssh` port-scan children even when the user disables the sidebar port/SSH detail rows (`sidebar.showPorts=false`, `sidebar.hideAllDetails=true`). The scans run synchronously on the coordinator's serial queue, which starves the cmux control socket (`cmux ping` → Broken pipe) and slows quit. This is the red half of the two-commit regression structure. It adds the `remotePortScanningEnabled` flag plus an inert `updateRemotePortScanningEnabled` setter (assignment only — the gating itself lands in the follow-up commit), and the `RemotePortScanGatingTests` suite that drives the queue-confined port-scan `*Locked` methods through a spy process runner. The four gating cases fail because nothing consults the flag yet; the two enabled-path sanity cases pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remote-workspace port discovery ran unconditionally: `RemoteSessionCoordinator` scheduled a `DispatchSource` poll timer and shell-activity scan bursts that each spawned `/usr/bin/ssh` synchronously on the coordinator's serial queue, regardless of the sidebar `showPorts`/`hideAllDetails` settings. The reporter set `sidebar.showPorts=false` and `sidebar.hideAllDetails=true` (which only hid the display) and still saw a tight `/usr/bin/ssh` respawn loop that starved the cmux control socket (`cmux ping` → Broken pipe), wedged remote reconnect, slowed quit (termination waited on in-flight 8s scans), and — with the 1Password SSH agent — spiked 1Password CPU. Gate the whole ssh-spawning path on a `remotePortScanningEnabled` flag the app derives from the ports-visibility settings (`showPorts && !hideAllDetails`, mirroring `SidebarWorkspaceAuxiliaryDetailVisibility.resolved`): when ports are not displayed there is nothing for the scans to populate, so polling and bursts are suspended and no ssh is spawned. Disabling tears down the poll timer, in-flight burst, and detected ports; enabling resumes polling (and re-arms one TTY-scoped refresh when no fallback timer covers it). Wiring: - `Workspace.remotePortScanningEnabledFromSettings()` reads the settings; `configureRemoteConnection` pushes the value before `start()` so a fresh coordinator (and reconnects) honor it immediately. - `TabManager.sidebarMetadataSettingsDidChange()` fans the value out to live remote sessions on settings changes, gated to actual transitions. - Document the new backend effect on the `sidebar.showPorts`/`hideAllDetails` schema descriptions (the reporter configures these via cmux.json). Tests: the `RemotePortScanGatingTests` gating cases now pass — disabling stops the poll timer and spawns no ssh, a scan/kick is dropped, toggling off tears down active polling, and re-enabling restarts it. Also serialize the two real-subprocess test suites against each other via `remoteSubprocessTestLock`: they share the process-global fd table, and the added gating suite's parallel load exposed the documented cross-suite fd-recycling window. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesRemote port scan gating by sidebar settings
Sequence Diagram(s)sequenceDiagram
participant Settings as UserDefaults
participant TabManager
participant Workspace
participant RemoteSessionCoordinator
rect rgba(100, 149, 237, 0.5)
Note over TabManager: sidebarMetadataSettingsDidChange()
TabManager->>Settings: read showPorts, hideAllDetails
Settings-->>TabManager: values
TabManager->>TabManager: refreshRemotePortScanningEnablement()
alt value unchanged
TabManager-->>TabManager: early return
else value changed
TabManager->>Workspace: applyRemotePortScanningEnabled(enabled)
Workspace->>RemoteSessionCoordinator: updateRemotePortScanningEnabled(enabled)
end
end
rect rgba(255, 165, 0, 0.5)
Note over RemoteSessionCoordinator: updateRemotePortScanningEnabled(false)
RemoteSessionCoordinator->>RemoteSessionCoordinator: cancel burst/coalesce tasks
RemoteSessionCoordinator->>RemoteSessionCoordinator: cancel bootstrap TTY retry
RemoteSessionCoordinator->>RemoteSessionCoordinator: clear scanned ports, stop polling
RemoteSessionCoordinator->>RemoteSessionCoordinator: publish empty ports snapshot
end
rect rgba(60, 179, 113, 0.5)
Note over RemoteSessionCoordinator: updateRemotePortScanningEnabled(true)
RemoteSessionCoordinator->>RemoteSessionCoordinator: re-arm polling state
RemoteSessionCoordinator->>RemoteSessionCoordinator: request bootstrap TTY or schedule refresh burst
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…t-poll-broken-pipe
The port-scan gating + app wiring grew four files past their recorded budget (RemoteSessionCoordinator+PortScan, Workspace, TabManager, RemoteSessionCoordinator). Refresh the budget to accept the small, necessary growth. Co-Authored-By: Claude Opus 4.8 <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: 1
🤖 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 `@web/data/cmux.schema.json`:
- Around line 656-660: Replace the hard-coded English description strings in the
hideAllDetails and showPorts properties with descriptionKey routing keys (follow
the existing pattern used by other properties in the file). Then add the
corresponding translated message entries for each descriptionKey in all 21
locale files in web/messages/ (en, ja, ar, bs, da, de, es, fr, it, km, ko, no,
pl, pt-BR, ru, th, tr, uk, zh-CN, zh-TW) to enable proper i18n localization.
Ensure the descriptionKey names are consistent between the schema and the locale
files.
🪄 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: a9d23bb3-6681-45eb-8b23-ebd9bd869fcc
📒 Files selected for processing (10)
Packages/CmuxRemoteSession/Package.swiftPackages/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+PortScan.swiftPackages/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftPackages/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemotePlatformProbeScriptTests.swiftPackages/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemotePortScanGatingTests.swiftPackages/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionProcessRunnerTests.swiftPackages/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSubprocessTestLock.swiftSources/TabManager.swiftSources/Workspace.swiftweb/data/cmux.schema.json
Autoreview caught an ssh-spawning path the first cut missed: the bootstrap remote-TTY resolver (`requestBootstrapRemoteTTYIfNeededLocked` and its bounded retry) reads `~/.cmux/relay/<port>.tty` over ssh and runs from `beginConnectionAttemptLocked` and the proxy-ready path regardless of the port-scan flag. Since that TTY is resolved *only* to TTY-scope the port scans (`applyBootstrapRemoteTTY` → `syncRemotePortScanTTYs` + `kickRemotePortScan`), it must respect the same gate — otherwise a remote workspace created while `sidebar.showPorts` is false (or `hideAllDetails` is true), or one with a pending retry when toggled off, still spawns `/usr/bin/ssh`. Gate the resolver and its retry on `remotePortScanningEnabled`, cancel any in-flight retry when scanning is suspended, and re-request the bootstrap TTY when scanning is re-enabled (covering the no-TTY-yet case, alongside the existing refresh-burst for the TTY-known case). Adds two coordinator tests: disabling spawns no resolver ssh; the enabled path spawns one (sanity). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR fixes a control-plane pathology (broken pipe, 1Password CPU spikes, slow quit) caused by
Confidence Score: 5/5Safe to merge — the gating is applied consistently across all three ssh-spawning paths and the serial-queue ordering guarantees the flag is set before start() executes. The flag is derived from the same precedence rule the sidebar already uses (hideAllDetails wins), tear-down on disable is comprehensive (burst, coalesce chain, bootstrap-TTY retry, poll timer, published ports), and re-enable correctly resumes polling and re-requests the bootstrap TTY. Stale burst steps are dropped by the existing generation guard, and the UserDefaults.didChangeNotification firehose is debounced to actual transitions. The deterministic test suite exercises every guarded code path with no wall-clock waits. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Settings change"] --> B["TabManager.sidebarMetadataSettingsDidChange()"]
B --> C["refreshRemotePortScanningEnablement()"]
C --> D{"Value changed?"}
D -- No --> E["No-op"]
D -- Yes --> F["applyRemotePortScanningEnabled(enabled)"]
F --> G["updateRemotePortScanningEnabled (queue.async)"]
G --> H{"enabled?"}
H -- false --> K["suspendRemotePortScanningLocked()"]
H -- true --> L["updateRemotePortPollingStateLocked()"]
L --> N{"TTYs known?"}
N -- No --> O["requestBootstrapRemoteTTYIfNeededLocked()"]
N -- Yes --> P["scheduleRemotePortScanCoalesceLocked()"]
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
21
to
+25
| @Test("Capture survives the pipe read handles being torn down mid-run") | ||
| func captureSurvivesPipeReadHandleTeardown() throws { | ||
| // Serialize against the platform-probe suite; see ``remoteSubprocessTestLock``. | ||
| remoteSubprocessTestLock.lock() | ||
| defer { remoteSubprocessTestLock.unlock() } |
Contributor
There was a problem hiding this comment.
RemoteSessionProcessRunnerTests uses direct lock()/unlock() while RemotePlatformProbeScriptTests uses the withRemoteSubprocessTestLock helper introduced in the same PR. Both patterns are safe (the defer ensures unlock), but using the helper everywhere would make the serialization intent uniform and reduce the chance of a future test accidentally omitting the defer.
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.
Good catch — unified both suites on the direct lock() / defer unlock() idiom and removed the helper (e1b8672), so the serialization intent is consistent.
— Claude Code
Autoreview flagged that extending the raw English `description` for sidebar.showPorts/hideAllDetails renders untranslated on non-English /docs/configuration pages (the page falls back to property.description when there is no descriptionKey). The schema note was a non-essential nicety; revert it so the PR adds no unlocalized user-facing text. The settings' behavior is documented in the PR/commit messages, and a properly localized docs note can be a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Autoreview flagged that the enabled bootstrap-TTY sanity test could race its
own retry: the spy returned empty stdout, so requestBootstrapRemoteTTYIfNeeded
treated the TTY as unresolved and scheduled the 0.5s bootstrap retry. The test
asserted runCount == 1 before cancelling it, so a >0.5s thread deschedule under
CI load could let the retry fire and bump the count to 2.
Return a valid TTY ("ttys005") so resolution succeeds on the first pass and
schedules no retry, and assert the resolved flag — the exact run count can no
longer race a delayed retry.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Greptile flagged that RemotePlatformProbeScriptTests used the withRemoteSubprocessTestLock helper while RemoteSessionProcessRunnerTests used direct lock()/defer unlock(). Unify on the direct lock()/defer idiom in both suites and drop the helper, so the serialization intent is uniform. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-poll-broken-pipe
…6123) Autoreview noted the fd-table serialization missed a third real-subprocess suite: RemoteHostReachabilityProbeTests resolveEndpoint cases shell out to /usr/bin/ssh -G with Process/Pipe in the same target, so they can still race captureSurvivesPipeReadHandleTeardown's mid-run fd teardown. Mark the suite .serialized (matching the other two real-IO suites) and take remoteSubprocessTestLock around the two synchronous ssh -G tests. The async probeTCP cases use sockets (a recycled socket fd only delays, never corrupts, the pipe-capture reader) and cannot hold the NSLock across their await, so they rely on the suite-level .serialized ordering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-poll-broken-pipe
Autoreview noted that suspendRemotePortScanningLocked() cancelled tasks and cleared published ports but left two pieces of hidden scanner bookkeeping intact: remotePortPollBaselinePorts and bootstrapRemoteTTYRetryCount. After a disable/re-enable, host-wide-delta polling could subtract against a pre-disable baseline (surfacing ports that appeared while ports were hidden), and an exhausted bootstrap TTY retry budget would not restart because the schedule guard still saw the old count. Reset both on suspend so re-enabling behaves like a fresh scanner start (matching the updateRemotePortPollingStateLocked teardown and stopAllLocked resets). Adds a toggle-off regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-poll-broken-pipe # Conflicts: # .github/swift-file-length-budget.tsv
3 of 6 tasks
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
1 participant
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 #6123.
The bug
With remote SSH workspaces configured, cmux enters a bad control-plane state:
cmux pingfails withFailed to write to socket (Broken pipe, errno 32), remote reconnect stops working until a full restart, cmux repeatedly spawns short-lived/usr/bin/sshchildren, quit becomes slow, and (with the 1Password SSH agent) 1Password CPU spikes.Crucially, the reporter set the documented sidebar visibility settings and it did not stop the backend scanning:
Root cause (confirmed by inspection + the reporter's sample stacks)
RemoteSessionCoordinatordrives remote listening-port discovery by spawning/usr/bin/sshsynchronously on the coordinator's serial queue from three places:DispatchSourcepoll timer (startRemotePortPollingLocked→pollRemotePortsLocked),kickRemotePortScanLocked→performRemotePortScanLocked→scanRemotePortsByPanelLocked), andrequestBootstrapRemoteTTYIfNeededLocked, used only to TTY-scope the scans).The polling/scan decisions consulted only
configuration.terminalStartupCommandand the tracked TTYs — neversidebar.showPorts/sidebar.hideAllDetails. So those settings only hid the display; the ssh scan loop kept running. With several remote hosts (the reporter reproduced with three) the serial queue is pinned on 8s ssh execs, which starves the control-socket path (the reporter's samples showTerminalController.spawnClientHandleralongside the…PortScan… → RemoteSessionProcessRunner.run → OS_dispatch_semaphore.waitstack) and makes quit wait on in-flight scans.The fix
Gate every ssh-spawning port-discovery path on a queue-confined
remotePortScanningEnabledflag, derived app-side from the ports-visibility settings —showPorts && !hideAllDetails, mirroringSidebarWorkspaceAuxiliaryDetailVisibility.resolved(the same precedence the sidebar uses). When ports are not displayed there is nothing for the scans to populate, so:remotePortPollingModeLocked()returnsnilwhen disabled → the poll timer never starts / is stopped.kickRemotePortScanLockedandperformRemotePortScanLockedshort-circuit → no burst, no ssh.requestBootstrapRemoteTTYIfNeededLocked/scheduleBootstrapRemoteTTYRetryLockedshort-circuit → the bootstrap TTY resolver spawns no ssh either.In the reporter's exact config (
showPorts:false,hideAllDetails:true) the whole pathology is disabled: no ssh loop → no broken pipe, no 1Password churn, prompt quit, reconnect unblocked. Connection-establishing ssh (daemon bootstrap, reverse relay, proxy) is intentionally not gated — only the port-discovery loop is.Wiring
Workspace.remotePortScanningEnabledFromSettings()reads the settings;configureRemoteConnectionpushes the value beforestart(), so a fresh coordinator (and every reconnect) honors it immediately.TabManager.sidebarMetadataSettingsDidChange()fans the value out to live remote sessions when settings change (gated to actual transitions).Tests
New
RemotePortScanGatingTestsdrives the queue-confined*Lockedmethods through a spy process runner (fully deterministic — no wall-clock waits): disabling stops the poll timer and spawns no ssh; a scan/kick is dropped; the bootstrap-TTY resolver spawns no ssh; toggling off tears down active polling and clears detected ports; re-enabling restarts polling; plus enabled-path sanity cases.Two-commit red/green: commit 1 adds the suite + an inert setter (tests fail), commit 2 implements the gating (tests pass). The bootstrap-TTY gating + tests were added in a follow-up after autoreview flagged that path.
I also serialized the two real-subprocess suites (
RemoteSessionProcessRunnerTests,RemotePlatformProbeScriptTests) against each other via a sharedremoteSubprocessTestLock: they share the process-global fd table, and the new suite's parallel load exposed the documented cross-suite fd-recycling window. Verified green across repeated full-suite runs ofswift test --package-path Packages/CmuxRemoteSession(31 tests).Scope notes
cmux.jsonwith no new surface.Localization audit
No user-facing strings are added or changed. The fix gates existing backend behavior on existing settings; the
settings.app.showPorts.subtitlehelp text remains accurate. NoResources/Localizable.xcstringsorweb/messages/*.jsonkeys changed, andweb/data/cmux.schema.jsonis unchanged from base (an earlier description tweak was reverted to avoid adding unlocalized docs text).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests