Repository navigation
Recover stale restored SSH PTY sessions - #5689
lawrencecchen wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughMissing persistent SSH PTY session errors now exit with code 253 and trigger a fallback attach command. CLI detects missing-session patterns, the command builder generates an optional fallback when enabled, and the retry loop conditionally runs it on 253 failure and warns the user. ChangesMissing PTY Session Fallback Attach Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (18 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 |
Greptile SummaryThis PR recovers stale restored SSH PTY sessions by adding exit code 253 for "session not found" errors, branching the startup shell script to fall back to a fresh remote shell on 253, and cleaning up stale remote relay listeners on port-forwarding startup failures.
Confidence Score: 4/5Safe to merge for the happy path; the stale-listener cleanup branch silently suppresses relay errors in an edge case where cleanup completes as a noop. The core SSH PTY recovery flow (exit 253, fallback command, 253 shell handler) is correct and well-tested. The one real defect is in startReverseRelayLocked: cleanupStaleRemoteRelayListenerLocked returns true for both killed-a-process and SSH-succeeded-but-nothing-to-kill (noop), and the caller unconditionally suppresses publishDaemonStatus for both. A port that stays occupied after a noop cleanup causes an infinite silent retry loop with no user-visible error. This affects only the stale-listener code path, not the primary PTY restore path. Sources/Workspace.swift — specifically the didCleanStaleListener branch that gates publishDaemonStatus. Important Files Changed
Reviews (2): Last reviewed commit: "Recover stale SSH relay listeners on rel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func remotePTYErrorIndicatesMissingSession(_ message: String) -> Bool { | ||
| let lowered = message.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() | ||
| guard !lowered.isEmpty else { return false } | ||
| return lowered.contains("pty_session_not_found") || | ||
| (lowered.contains("persistent ssh pty session") && lowered.contains("not running")) || | ||
| (lowered.contains("persistent pty session") && lowered.contains("not running")) | ||
| } |
There was a problem hiding this comment.
remotePTYErrorIndicatesMissingSession repeats the exact same three contains clauses already present in userFacingRemotePTYErrorMessage (lines 10467–10470). If a new server error pattern is added to userFacingRemotePTYErrorMessage to emit the "no longer running" message but the same pattern is not mirrored here, exit code 253 won't fire, the fallback shell won't trigger, and the regression is silent. Consider refactoring so remotePTYErrorIndicatesMissingSession delegates to (or shares a constant with) userFacingRemotePTYErrorMessage to keep the two in sync.
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!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81fb9fb6cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let attachCommand = "\"$cmux_ssh_attach_cli\" --socket \"$CMUX_SOCKET_PATH\" ssh-pty-attach --wait\(requireExistingFlag) --workspace \"$CMUX_WORKSPACE_ID\" --session-id \"$cmux_ssh_attach_session_id\" --attachment-id \"${CMUX_SURFACE_ID:-}\"\(commandB64Flag)" | ||
| lines += retryingAttachLines(command: attachCommand) | ||
| let fallbackCommand = requireExisting && !commandB64Flag.isEmpty | ||
| ? "\"$cmux_ssh_attach_cli\" --socket \"$CMUX_SOCKET_PATH\" ssh-pty-attach --wait --workspace \"$CMUX_WORKSPACE_ID\" --session-id \"$cmux_ssh_attach_session_id\" --attachment-id \"${CMUX_SURFACE_ID:-}\"\(commandB64Flag)" |
There was a problem hiding this comment.
When a restored attach exits 253, the first ssh-pty-attach process still runs its failed-attach cleanup and sends workspace.remote.pty_attach_end; that path calls Workspace.markRemotePTYAttachEnded, which removes the surface from activeRemoteTerminalSurfaceIds and clears remotePTYSessionIDsByPanelId. This fallback then starts a new PTY on the same surface, but there is no corresponding path that re-adds the restored session ID, so the recovered terminal is no longer treated as a remote PTY and the next session snapshot will drop the persistent PTY state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bdf5944. Configure here.
| " if [ \"$cmux_ssh_attach_status\" -eq 253 ]; then", | ||
| " if [ -t 2 ]; then printf '\\n\\033[33m[cmux] persisted SSH PTY session is gone; starting a new remote shell.\\033[0m\\n' >&2 || true; fi", | ||
| " \(missingSessionFallbackCommand)", | ||
| " exit \"$?\"", |
There was a problem hiding this comment.
Fallback attach skips bridge retries
Medium Severity
After exit code 253, the stale-session fallback runs a second ssh-pty-attach and immediately exits with its status. That bypasses the surrounding retry loop that handles exit codes 254 and 255 for the primary attach. A recovered remote shell can therefore stop reattaching on transient bridge closes that the main path would retry.
Reviewed by Cursor Bugbot for commit bdf5944. Configure here.
| if !didCleanStaleListener { | ||
| publishDaemonStatus( | ||
| .error, | ||
| detail: "Remote SSH relay unavailable: \(startupFailure) (retry in \(retrySeconds)s)" | ||
| ) | ||
| } | ||
| scheduleReverseRelayRestartLocked(remotePath: remotePath, delay: retryDelay) |
There was a problem hiding this comment.
cleanupStaleRemoteRelayListenerLocked returns true for two distinct cases: (1) it killed a stale process, and (2) the SSH exec succeeded but found no process to kill (noop — empty stdout, logged as remoteListener.cleanupNoop). In the noop case didCleanStaleListener is still true, so publishDaemonStatus(.error, ...) is skipped and the 0.25 s retry fires. On the next attempt the same "remote port forwarding failed for listen port X" error triggers reverseRelayStartupFailureIndicatesStaleRemoteListener again → cleanup runs again → noop again → error suppressed again. There is no retry limit in scheduleReverseRelayRestartLocked, so a port that stays occupied by something the cleanup script can’t find produces an infinite silent retry loop: the relay never connects but the user sees no error and has no way to diagnose the failure.
The fix is to distinguish the noop case — only suppress status publication (and use the fast 0.25 s delay) when the cleanup actually terminated a process (non-empty stdout / a richer boolean signal from the helper), and fall back to publishing the status normally for the noop case.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-stalepty-tests test -only-testing:cmuxTests/TabManagerSessionSnapshotTests/testSessionSnapshotRestoresSplitPersistentSSHPTYWithoutDefaultAttachScaffold -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/testSSHPTYAttachRequireExistingSessionNotFoundFailsWithoutWaitRetrypassed.xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-stalepty-tests test -only-testing:cmuxTests/TabManagerSessionSnapshotTests/testSessionSnapshotRestoresPersistentSSHPTYSessionAfterRelaunchpassed.tests_v2/test_ssh_remote_detachable_pty.pypassed against tagged buildsptyand/tmp/cmux-debug-spty.sock.Issues
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches remote SSH PTY attach exit semantics, restore startup scripts, and reverse-relay recovery—important for session continuity but scoped with tests and distinct exit codes.
Overview
Recovers stale persistent SSH PTY sessions after restart/update by treating a missing PTY as a distinct outcome and optionally starting a fresh remote shell instead of failing or looping on reconnect.
ssh-pty-attachnow exits with 253 when the persisted PTY is gone (viaremotePTYErrorIndicatesMissingSession), and restored startup scripts handle that code: they show a user message and run a fallback attach without--require-existing, using an embedded--command-b64remote shell when a relay port is available. Transient bridge closes still retry on 254/255 only.Restored panel attach commands now pass
restoredRemoteShellCommand(relayPort:)asremoteCommandso the fallback can bootstrap a new interactive shell on the same relay.Reverse SSH relay startup detects “remote port forwarding failed for listen port N” as a stale remote listener, runs
cleanupStaleRemoteRelayListenerLocked, uses a shorter retry when cleanup succeeds, and skips publishing a noisy error status when cleanup handled the stale case.Tests and the detachable-PTY integration test were updated for exit 253, fallback shell wiring, stale-listener detection, and more reliable
surface_idresolution after attach.Reviewed by Cursor Bugbot for commit bdf5944. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Detects and recovers from stale restored SSH PTY sessions and stale SSH relay listeners. Falls back to a new remote shell when the persisted PTY is gone, cleans stuck relay ports on relaunch, and keeps the retry loop for transient bridge closures with same-session reattach when the PTY still exists.
ssh-pty-attachreturns exit code 253 when the PTY session is missing and shows a clearer error.--command-b64; retries only on 254/255.surface_idresolution.Written for commit bdf5944. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests