Repository navigation
Keep SSH sessions alive when closing a pane - #3566
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a shell helper ChangesSignal Exit Handling & Tests
sequenceDiagram
participant Test
participant Shell as "Shell Environment"
participant Trap as "Signal Trap Handler"
participant CMUX as "cmux (stub logger)"
participant Server as "Mock JSON-RPC Server"
Test->>Shell: Set env (CMUX_TEST_*, CMUX_TEST_SIGNAL)
Test->>Server: Start mock workspace RPC
Test->>CMUX: Start stub cmux (background)
Test->>Shell: Execute SSH startup command
Shell->>Trap: Receive signal (HUP/INT/TERM)
Trap->>Trap: Call cmux_ssh_signal_exit(status 129/130/143)
Trap->>Shell: Disable further traps and exit with recorded status
CMUX->>Test: Log entries (if any)
Test->>CMUX: Assert no "ssh-session-end" logged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 SummaryFixes #3556 by splitting the single
Confidence Score: 5/5Safe to merge — the trap split is minimal and correct, the race window between subprocess exit and trap clearance is negligible, and the regression test exercises all three signal paths end-to-end. The production change touches four lines of shell embedded in Swift string literals. The signal handler atomically clears all traps before calling exit, so the EXIT cleanup can never fire on a pane-close signal. The normal-exit path still calls cleanup explicitly then clears traps, so there is no double-invocation path. The new test generates the real startup command from the live CLI, injects a fake cmux sentinel, and confirms HUP/INT/TERM each produce the right status code without touching ssh-session-end. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant P as Pane / tmux
participant W as SSH Wrapper Script
participant S as ssh subprocess
participant C as cmux (ssh-session-end)
Note over W: EXIT trap → cmux_ssh_session_end
Note over W: HUP/INT/TERM → cmux_ssh_signal_exit
W->>S: command ssh ...
S-->>W: (normal exit, status N)
W->>W: cmux_ssh_status=$?
W->>W: trap - EXIT HUP INT TERM
W->>C: cmux_ssh_session_end (cleanup runs)
W-->>P: exit $cmux_ssh_status
P->>W: TERM (pane closed)
Note over W: Signal queued while ssh runs
S-->>W: (fake ssh exits)
W->>W: cmux_ssh_signal_exit 143
W->>W: trap - EXIT HUP INT TERM
Note over C: ssh-session-end NOT called
W-->>P: exit 143
Reviews (6): Last reviewed commit: "Keep SSH transport alive on pane-close s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
austinywang
force-pushed
the
issue-3556-ssh-pane-close-kills-session
branch
2 times, most recently
from
May 5, 2026 18:37
90a2058 to
30880d0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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`:
- Line 3675: The file exceeds the Swift file-length budget due to the added
test; extract the test function
testSSHPaneCloseSignalDoesNotReportSessionEndToSharedTransport (and any small
shared helpers it uses) into a new test file (e.g.,
SSHStartupWrapperTests.swift) or an existing SSH-focused test file, update
imports and test target membership, and ensure any shared fixtures/mocks
referenced by that function are either moved or made available via internal
helpers so the test compiles and runs in its new file.
- Around line 3685-3688: The fake CLI currently logs every cmux invocation (the
script written by writeShellFile uses CMUX_TEST_SESSION_END_LOG and prints
"$*"), but the test asserts recordedCalls.isEmpty with a message about "must not
call ssh-session-end"; change the assertion to filter recordedCalls for only
lines containing the literal "ssh-session-end" (e.g., let sessionEndCalls =
recordedCalls.filter { $0.contains("ssh-session-end") } and assert
sessionEndCalls.isEmpty) and update the failure message to reference
"ssh-session-end" specifically; alternatively, if you prefer to keep the
existing assertion semantics, rename CMUX_TEST_SESSION_END_LOG to
CMUX_TEST_ALL_CALLS_LOG and update the assertion message to "must not call cmux
at all".
- Line 3692: The test loop that verifies non-session-end trapping currently
iterates only over signals ["HUP", "TERM"] (the for loop in
WorkspaceRemoteConnectionTests.swift) and omits "INT"; update the loop to
include "INT" (i.e. iterate over ["HUP", "INT", "TERM"]) so the INT trap is also
tested and cannot regress to calling ssh-session-end.
- Around line 3685-3689: The call to writeShellFile from
CLINotifyProcessIntegrationTests fails because writeShellFile is declared
private inside WorkspaceRemoteConnectionTests; change the declaration of
writeShellFile to fileprivate so both WorkspaceRemoteConnectionTests and
CLINotifyProcessIntegrationTests in this file can call it (or alternatively add
a duplicate private helper inside CLINotifyProcessIntegrationTests if you prefer
separation). After making that visibility change, address the file-size budget
overflow (~4172 > 4122) by either trimming/reducing test content in this file or
requesting a repository/test budget increase so the file can remain this large.
🪄 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: 4f263db3-2c45-467c-89e9-62e417bbb195
📒 Files selected for processing (2)
CLI/cmux.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
| try fileManager.setAttributes([.posixPermissions: 0o700], ofItemAtPath: fakeCLI.path) | ||
|
|
||
| let cli = CMUXCLI(args: []) | ||
| for signal in ["HUP", "TERM"] { |
There was a problem hiding this comment.
SIGINT omitted from the signal loop
The PR description, commit message, and AI summary all state that HUP/INT/TERM are now routed through the new non-session-end trap. The loop at line 3692 tests only ["HUP", "TERM"], leaving INT unverified. If the INT trap is ever accidentally reverted to call ssh-session-end, this test will not catch it.
✅ Proposed fix
- for signal in ["HUP", "TERM"] {
+ for signal in ["HUP", "INT", "TERM"] {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for signal in ["HUP", "TERM"] { | |
| for signal in ["HUP", "INT", "TERM"] { |
🤖 Prompt for 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.
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift` at line 3692, The test loop
that verifies non-session-end trapping currently iterates only over signals
["HUP", "TERM"] (the for loop in WorkspaceRemoteConnectionTests.swift) and omits
"INT"; update the loop to include "INT" (i.e. iterate over ["HUP", "INT",
"TERM"]) so the INT trap is also tested and cannot regress to calling
ssh-session-end.
austinywang
force-pushed
the
issue-3556-ssh-pane-close-kills-session
branch
2 times, most recently
from
May 5, 2026 18:42
ea2ebaf to
ee3a6e5
Compare
austinywang
dismissed
coderabbitai[bot]’s stale review
May 5, 2026 18:43
Resolved in later commits; all inline threads addressed and stale review was on old commit a5a9508.
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 `@cmuxTests/SSHStartupSignalLifecycleTests.swift`:
- Line 9: Add an explicit empty deinitializer to the
SSHStartupSignalLifecycleTests XCTestCase class to satisfy the required_deinit
SwiftLint rule; locate the class declaration (SSHStartupSignalLifecycleTests)
and add a deinit { } implementation (or include any necessary teardown logic
inside that deinit) so the class no longer triggers the rule.
🪄 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: 6fd07dcc-b732-4a5d-a0b4-281a372018f3
📒 Files selected for processing (3)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojcmuxTests/SSHStartupSignalLifecycleTests.swift
austinywang
force-pushed
the
issue-3556-ssh-pane-close-kills-session
branch
from
May 5, 2026 18:46
ee3a6e5 to
66a62bc
Compare
austinywang
force-pushed
the
issue-3556-ssh-pane-close-kills-session
branch
from
May 5, 2026 18:56
66a62bc to
1015d7a
Compare
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 `@cmuxTests/SSHStartupSignalLifecycleTests.swift`:
- Around line 42-58: The test should also assert the wrapper exited with the
signal-derived code (128 + signal) to ensure the cmux_ssh_signal_exit contract
is honored: after runProcess(...) capture result.status and assert it equals 128
+ the numeric value of the signal used in startupCommand (or use an explicit
mapping for signals 1->129, 2->130, 15->143), e.g. add an
XCTAssertEqual(result.status, expectedExitCode) alongside the existing
assertions so failures that swallow or incorrectly handle the signal are
detected; reference runProcess, startupCommand, result.status and the
cmux_ssh_signal_exit contract when implementing.
🪄 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: 92f63732-226c-4d3d-a2dc-4954501d5dad
📒 Files selected for processing (3)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojcmuxTests/SSHStartupSignalLifecycleTests.swift
Pane closes can deliver HUP or TERM to the SSH startup wrapper while sibling panes still depend on the same ProxyCommand-backed transport. This regression test executes the generated reusable SSH startup command with a fake cmux binary and records whether signal-driven teardown reports ssh-session-end. Constraint: Do not run local tests; CI must prove this red commit fails before the fix. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest execution intentionally skipped per task instruction
Pane close tears down the terminal process, not the shared SSH workspace transport. The SSH startup wrapper now distinguishes signal-driven wrapper exit from the SSH command finishing, so HUP/INT/TERM no longer emit ssh-session-end and cannot request ControlMaster cleanup while sibling panes still rely on the transport. Constraint: Regression introduced by SSH lifecycle cleanup that treated pane-close signals as remote session completion. Rejected: Disable remote session-end cleanup entirely | real SSH command exits still need workspace demotion and ControlMaster cleanup when the last session is actually gone. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest execution intentionally skipped per task instruction
austinywang
force-pushed
the
issue-3556-ssh-pane-close-kills-session
branch
from
May 5, 2026 19:09
1015d7a to
d80e4b0
Compare
austinywang
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
May 5, 2026 19:24
Stale CodeRabbit review on an older commit; the requested signal-exit-status assertion was added in the current commit and the latest CodeRabbit review approved it.
Merged
This was referenced May 9, 2026
This was referenced May 14, 2026
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 #3556.\n\n## Summary\n- adds a regression test proving SSH pane-close HUP/TERM signals must not emit ssh-session-end\n- keeps ssh-session-end cleanup for real SSH command completion, but suppresses it for signal-driven wrapper exit\n\n## Verification\n- git diff --check\n- local tests not run per task instruction
Note
Medium Risk
Changes SSH startup wrapper signal/trap behavior, which can affect remote session cleanup and exit status handling; covered by a new regression test but still impacts a user-critical connection lifecycle path.
Overview
Prevents pane-close signals from triggering
ssh-session-endby splitting the SSH startup wrapper traps:EXITstill runscmux_ssh_session_end, whileHUP/INT/TERMnow clear traps and exit with signal-derived statuses (129/130/143) to avoid tearing down shared SSH transports.Adds
SSHStartupSignalLifecycleTests(wired into the Xcode project) to assert that signal-driven exits do not emitssh-session-endand that the wrapper returns the expected exit codes.Reviewed by Cursor Bugbot for commit d80e4b0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Keep the shared SSH transport alive when a pane closes by handling
HUP/INT/TERMin the startup wrapper without emittingssh-session-end; real SSH exits still run cleanup. Signal exits now return 129/130/143 and never tear down ControlMaster (fixes #3556).cmux_ssh_signal_exit;EXITruns cleanup, whileHUP/INT/TERMclear traps and exit with 129/130/143 without callingssh-session-end.SSHStartupSignalLifecycleTests(wired intocmuxTests) using fakecmux/sshto assert nossh-session-endon pane-close signals and correct exit statuses.Written for commit d80e4b0. Summary will update on new commits.
Summary by CodeRabbit