fix(ssh): restart remote terminal sessions after SSH reconnection#1471
fix(ssh): restart remote terminal sessions after SSH reconnection#1471arnestrickmann merged 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR implements SSH reconnection recovery for remote terminal sessions by wiring together three layers: a custom DOM event (
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/renderer/terminal/TerminalSessionManager.ts | Adds isPtyActive(), restart(), cleanupPtyListeners(), and deduplicates connectPty() with a stored promise. The onPtyStarted listener is pushed to this.disposables (not ptyListenerDisposables), meaning it is not cleaned up by cleanupPtyListeners() between restart cycles — stale listeners can accumulate if the PTY exits before onPtyStarted fires. The ptyConnectPromise deduplication also silently drops the hasExistingSession=true flag on concurrent calls. |
| src/renderer/terminal/SessionRegistry.ts | Adds auto-restart of inactive remote sessions on reattach via getOrCreate. The restart is fire-and-forget (void existing.restart()), which is safe because restart() has internal deduplication. The setTheme and attach calls still proceed synchronously on the returned session, which is the correct behavior. |
| src/renderer/components/TerminalPane.tsx | Adds a useEffect that listens for SSH_CONNECTION_RESTORED_EVENT and calls sessionRef.current?.restart(). The use of sessionRef (a ref, not a closure value) is correct — the handler always accesses the latest session. The dependency on remote?.connectionId ensures the listener is correctly re-registered when the connection ID changes. |
| src/renderer/hooks/useRemoteProject.ts | Introduces connectionStateRef to track the previous state and detect 'disconnected→connected' transitions inside updateConnectionState, then dispatches dispatchSshConnectionRestored. The ref-based previous-state detection is correct. A fragile invariant exists: connectionStateRef and connectionState must always be updated together through updateConnectionState; any future direct call to setConnectionState would desync them. |
| src/renderer/lib/sshEvents.ts | New file exporting a custom DOM event name, a typed detail interface, and a safe dispatch helper. The typeof window guard makes it safe in SSR/test environments. |
| src/test/renderer/sessionRegistry.test.ts | New test file covering the two primary cases in SessionRegistry.getOrCreate: inactive remote sessions restart on reattach, and active sessions do not. Missing coverage for multiple successive reconnections and the TerminalPane event listener path. |
Sequence Diagram
sequenceDiagram
participant SSH as SSH Monitor (main)
participant Hook as useRemoteProject
participant Window as window (DOM events)
participant Pane as TerminalPane
participant Registry as SessionRegistry
participant TSM as TerminalSessionManager
SSH->>Hook: sshGetState → 'connected'
Note over Hook: previousState = 'disconnected'
Hook->>Hook: connectionStateRef.current = 'connected'
Hook->>Window: dispatchSshConnectionRestored({ connectionId })
par TerminalPane path
Window-->>Pane: SSH_CONNECTION_RESTORED_EVENT
Pane->>TSM: sessionRef.current.restart()
and SessionRegistry path (on next attach)
Pane->>Registry: terminalSessionRegistry.attach(options)
Registry->>TSM: existing.isPtyActive() → false
Registry->>TSM: existing.restart()
end
Note over TSM: restartPromise deduplication<br/>prevents concurrent restarts
TSM->>TSM: cleanupPtyListeners()
TSM->>TSM: connectPty(hasExistingSession=true)
TSM-->>SSH: ptyStartDirect({ resume: true })
SSH-->>TSM: { ok: true }
TSM->>TSM: ptyStarted = true
TSM->>TSM: setupPtyDataListener(id)
TSM->>Pane: emitReady()
Last reviewed commit: 10ca3c1
![greptile-apps[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| offStarted = undefined; | ||
| } | ||
| }); | ||
| if (offStarted) this.disposables.push(offStarted); |
There was a problem hiding this comment.
onPtyStarted listener accumulates in disposables across restart cycles
offStarted is pushed into this.disposables, but cleanupPtyListeners() — called by restart() — only drains this.ptyListenerDisposables. This means that if the PTY process exits before onPtyStarted has fired (i.e., the listener hasn't had a chance to self-remove from disposables via removeOffStartedFromDisposables()), the stale entry remains in this.disposables when the next restart adds a new offStarted.
When onPtyStarted eventually fires for the new restart, all stale listeners whose closured payload?.id === id will match and:
- Each fires
sendSizeIfStarted()— sending redundant resize packets per stale entry. - Each calls its own
offStarted?.()to unsubscribe (so IPC subscriptions do get cleaned up), but only after all the extra side-effects have already run.
This PR intentionally separated ptyListenerDisposables from disposables for exactly this kind of restart-safe cleanup. The offStarted listener belongs in the same category.
| if (offStarted) this.disposables.push(offStarted); | |
| if (offStarted) this.ptyListenerDisposables.push(offStarted); |
| if (this.ptyConnectPromise) { | ||
| return this.ptyConnectPromise; | ||
| } |
There was a problem hiding this comment.
ptyConnectPromise deduplication silently drops the hasExistingSession=true argument
When restart() calls connectPty(true) and this.ptyConnectPromise is already non-null (i.e., a concurrent connectPty(false) call from the initial start-up path is still in flight), the second call immediately returns the cached promise. That cached promise was created with resume: false in its closure, so the backend receives a "start fresh" instruction instead of "resume existing tmux session".
While this requires a narrow race (the initial connectPty(false) must still be awaiting resolution at exactly the moment the PTY exit event fires and restart() is called), it is observable when the SSH connection is flaky during the first connect. The typical symptom would be the terminal appearing blank after reconnection instead of restoring the previous session.
Consider logging a warning when the cached promise is returned in order to make this easier to diagnose:
if (this.ptyConnectPromise) {
if (hasExistingSession) {
log.warn('terminalSession:connectPty deduplication dropped resume flag', { id: this.options.taskId });
}
return this.ptyConnectPromise;
}| const [host, setHost] = useState<string | null>(null); | ||
| const healthCheckIntervalRef = useRef<NodeJS.Timeout | null>(null); | ||
| const isMountedRef = useRef(true); | ||
| const connectionStateRef = useRef<ConnectionState>(connectionState); |
There was a problem hiding this comment.
connectionStateRef not kept in sync with connectionState when connectionState changes outside updateConnectionState
connectionStateRef is initialized once from connectionState and then updated exclusively inside updateConnectionState. However, there is one code path that calls setConnectionState directly without going through updateConnectionState — the initial useState lazy initializer. If the module-level connectionStateCache returns 'connected' for the project, connectionState starts as 'connected' while connectionStateRef.current is also initialized to 'connected'. This is fine.
The real risk is subtler: setConnectionState is the React state setter, and connectionStateRef is the mutable mirror. They are only kept in sync through updateConnectionState. If any future code path calls setConnectionState directly (bypassing updateConnectionState), connectionStateRef would become stale and the transition guard previousState !== 'connected' could fire spuriously (false positive restore event) or miss a real transition (false negative). This is a fragile invariant.
Consider enforcing it explicitly with a private helper that wraps both updates, and removing the direct export of setConnectionState from the hook's closure.
| it('restarts an existing remote session when its PTY is no longer active', () => { | ||
| const theme = { base: 'dark' as const }; | ||
| const container = {} as HTMLElement; | ||
|
|
||
| const first = terminalSessionRegistry.attach({ | ||
| taskId: 'task-1', | ||
| container, | ||
| remote: { connectionId: 'ssh-1' }, | ||
| initialSize: { cols: 120, rows: 32 }, | ||
| theme, | ||
| }); | ||
|
|
||
| expect(managerInstances).toHaveLength(1); | ||
| expect(first).toBe(managerInstances[0]); | ||
|
|
||
| managerInstances[0].isPtyActive.mockReturnValue(false); | ||
|
|
||
| const second = terminalSessionRegistry.attach({ | ||
| taskId: 'task-1', | ||
| container, | ||
| remote: { connectionId: 'ssh-1' }, | ||
| initialSize: { cols: 120, rows: 32 }, | ||
| theme, | ||
| }); | ||
|
|
||
| expect(second).toBe(first); | ||
| expect(managerInstances[0].restart).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('does not restart an active session on reattach', () => { | ||
| const theme = { base: 'dark' as const }; | ||
| const container = {} as HTMLElement; | ||
|
|
||
| terminalSessionRegistry.attach({ | ||
| taskId: 'task-2', | ||
| container, | ||
| remote: { connectionId: 'ssh-2' }, | ||
| initialSize: { cols: 120, rows: 32 }, | ||
| theme, | ||
| }); | ||
|
|
||
| terminalSessionRegistry.attach({ | ||
| taskId: 'task-2', | ||
| container, | ||
| remote: { connectionId: 'ssh-2' }, | ||
| initialSize: { cols: 120, rows: 32 }, | ||
| theme, | ||
| }); | ||
|
|
||
| expect(managerInstances[0].restart).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test suite missing coverage for multiple successive reconnection cycles
The tests verify a single restart trigger but don't cover the scenario where the SSH connection drops and restores more than once within the lifetime of a session. Key cases worth adding:
- Multi-reconnect cycle: PTY exits → restart → PTY exits again → restart again. Verify that
restartis called the expected number of times with no stale side-effects (e.g.,sendSizeIfStartedequivalent not called multiple times). - Concurrent restart deduplication: Calling
terminalSessionRegistry.attach()twice in rapid succession whenisPtyActive()returnsfalseshould result in exactly onerestart()invocation (dedup viarestartPromise). TerminalPaneevent listener path: TheSSH_CONNECTION_RESTORED_EVENThandler inTerminalPaneindependently callsrestart(). A test covering both triggers firing for the same session would confirm the promise deduplication is effective end-to-end.
# Conflicts: # src/main/services/ptyManager.ts
1 participant
Summary
restart()method toTerminalSessionManagerthat re-establishes the PTY connection for dead remote sessionsemdash:ssh-connection-restoredevent when SSH transitions from disconnected to connected, listened to byTerminalPaneto trigger session restartconnectPty()andrestart()calls with stored promisesptyListenerDisposables) so they can be cleaned up and re-registered on restart without disposing the full sessionSessionRegistry, auto-restart inactive remote sessions on reattachTest plan
sessionRegistry.test.tspasses: inactive remote sessions restart on reattach, active sessions do not