diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index df266857fc9..9f514936cf4 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -187,12 +187,9 @@ export class SshPtyProvider implements IPtyProvider { expectedIncarnationId?: string ): Promise { // Why: reconnect owns replay delivery so stale/duplicate attach results can - // be filtered before they reach the renderer. Pane identity is deliberately - // not sent — see reattachSshPtySession; the relay's copy is frozen at spawn, - // so it rejected panes that had merely moved tabs. - // The shell's own identity IS sent when the caller knows it. This is the path that reconnects - // every known pty after a relay comes back, which is exactly when ids have been recycled, so - // leaving it unfenced would leave the main reconnect open to attaching somebody else's shell. + // be filtered before they reach the renderer. Pane identity stays unsent (the relay froze it at + // spawn, so it rejected moved panes); the shell's own identity is sent instead. This path + // reattaches every pty after a relay returns — exactly when ids have been recycled. const params = { id: this.toRelayPtyId(id), suppressReplayNotification: true, diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index d476701134d..b617f4e727a 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -336,13 +336,10 @@ export type PtyExitListener = (event: { id: string; paneKey?: string }) => void type PtyIdentity = { paneKey?: string; tabId?: string } -/* Why there is no attach-time pane-identity comparison here any more: it keyed on the paneKey and - * tabId frozen at spawn, so moving a pane to another tab made the relay refuse its own live shell, - * and refuse by saying "not found" — which the client read as death and answered by resuming the - * agent a second time. Cross-generation id collisions, the thing it was actually for, are rejected - * by the incarnation comparison in `attach` instead: that is the shell's own identity, so it - * follows the pane wherever it moves. `attachIdentity` is still recorded and still round-trips - * through serialize/revive; it just no longer gates an attach. */ +/* No attach-time pane-identity comparison any more: it froze paneKey/tabId at spawn, so a moved + * pane was refused its own live shell — as "not found", which read as death. Recycled ids, its real + * purpose, are caught by the incarnation comparison in `attach` instead. `attachIdentity` is still + * recorded and round-trips through serialize/revive; it no longer gates an attach. */ /** Returns env to merge into the PTY's spawn env. Receives spawn context so augmenters can derive per-PTY identity from paneKey. * `command` is the renderer-chosen agent launch command (`pi`, `omp`, …); undefined for CLI-launched bare shells. */ export type PtyEnvAugmenter = (ctx: { @@ -367,13 +364,9 @@ export class PtyHandler { private outputFlushTimer: ReturnType | null = null private pendingOutputByPty = new Map() private pendingExitByPty = new Map() - /** - * Shells this process WATCHED exit, kept after the exit is published. Attach can then answer - * "it exited" instead of "not found", and only that first-hand answer is proof: a relay that - * merely never knew the id may be a replacement whose predecessor's shells are still running. - * Bounded and oldest-evicted — losing one costs a respawn the user has to ask for, never a wrong - * one. A crash loses them all, which correctly reads as no knowledge. - */ + /** Exits this process WATCHED, kept past publication so attach can prove death rather than report + * an unknown id. Bounded and oldest-evicted; losing one costs a user-asked respawn, never a + * wrong one, and a crash losing all of them correctly reads as no knowledge. */ private exitedPtys = new Map() private pausedOutputPtys = new Set() private consumerPausedOutputPtys = new Set() @@ -754,6 +747,16 @@ export class PtyHandler { }) } + /** Proof must name the caller's own shell: present and matching. Both gone-paths ask this, so a + * change to the rule cannot reach one and miss the other. */ + private exitProofAnswersCaller( + expectedIncarnationId: string | undefined, + exitedIncarnationId: string, + clientUnderstandsExitProof: boolean + ): boolean { + return clientUnderstandsExitProof && expectedIncarnationId === exitedIncarnationId + } + /** Oldest-evicted: a Map iterates in insertion order, so the first key is the oldest. */ private rememberPtyExit(id: string, code: number, incarnationId: string): void { this.exitedPtys.delete(id) @@ -1619,30 +1622,23 @@ export class PtyHandler { const id = params.id as string const expectedIncarnationId = typeof params.expectedIncarnationId === 'string' ? params.expectedIncarnationId : undefined - // Why gated: what the host answers reaches clients that predate this reply, and one of those - // reads an unrecognized attach error as neither death nor recovery — it strands the pane. A - // client that understands the proof says so; everyone else keeps the wording they already act - // on. Nothing is lost by staying quiet, because an older client could not have used it. + // Gated because the host's answer reaches clients predating it, which read an unrecognized + // attach error as neither death nor recovery and strand the pane. const clientUnderstandsExitProof = params.exitProofSupported === true const managed = this.ptys.get(id) // Why: after dispose, pty.kill is a POSIX no-op; treat disposed as not-found so failures aren't silent. if (!managed || managed.disposed) { - // `disposed` is a teardown flag, not an observed exit, so only a genuinely absent pty may be + // `disposed` is teardown, not an observed exit, so only a genuinely absent pty may be // answered from what this process watched exit. if (!managed) { const exited = this.exitedPtys.get(id) - // A remembered exit for a DIFFERENT shell says nothing about the caller's: answer the - // ordinary unknown instead, or a recycled id would report somebody else's death as ours. - // The expectation must be PRESENT and match. A caller that names no shell is not asking - // about this one, and answering it anyway is how a remembered exit becomes a claim about - // somebody else's shell: relay A is killed leaving pty-1 alive and orphaned, relay B mints - // its own pty-1 and that one exits, and a pane with no recorded identity would be told its - // shell is gone — then replace a process that is still running. A pane with nothing to - // compare gets the disconnected card instead, which is the safe direction. if ( - clientUnderstandsExitProof && exited && - expectedIncarnationId === exited.incarnationId + this.exitProofAnswersCaller( + expectedIncarnationId, + exited.incarnationId, + clientUnderstandsExitProof + ) ) { throw new Error(formatPtyExitedError(id, exited.code, exited.incarnationId)) } @@ -1660,19 +1656,19 @@ export class PtyHandler { disposeManagedPty(managed) this.removePty(id) this.clearPtyFlowState(id) - // First-hand proof: this process held the shell and its pid is gone. Saying "not found" - // here throws away the one observation that distinguishes a dead shell from an unknown one. - // The reap above happens either way — a dead shell should be cleared no matter who asked — - // but the ANSWER still depends on whose shell it was. Under this id a replacement relay may - // hold a different shell entirely, and its death says nothing about the caller's, which may - // be running orphaned under the relay this one replaced. Same rule as the remembered exit: - // proof needs a present, matching expectation. + // The reap runs whoever asked, but the ANSWER depends on whose shell died: under this id a + // replacement relay may hold a different one entirely, and the caller's may be orphaned and + // alive under the relay this one replaced. this.rememberPtyExit(id, PTY_EXIT_CODE_OBSERVED_GONE, managed.incarnationId) if (expectedIncarnationId && expectedIncarnationId !== managed.incarnationId) { throw new Error(`PTY "${id}" identity mismatch`) } throw new Error( - clientUnderstandsExitProof && expectedIncarnationId === managed.incarnationId + this.exitProofAnswersCaller( + expectedIncarnationId, + managed.incarnationId, + clientUnderstandsExitProof + ) ? formatPtyExitedError(id, PTY_EXIT_CODE_OBSERVED_GONE, managed.incarnationId) : `PTY "${id}" not found` ) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 4528cb2e6ef..d98e5ffefd3 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -13,6 +13,7 @@ import { scheduleRuntimeGraphSync } from '@/runtime/sync-runtime-graph' import { useAppStore } from '@/store' import { getWorktreeMapFromState } from '@/store/selectors' import { parseWorkspaceKey } from '../../../../shared/workspace-scope' +import { SSH_SESSION_EXPIRED_ERROR } from '../../../../shared/ssh-pty-failure-tokens' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants' import { isEphemeralSetupTerminalWorktreeId } from '../../../../shared/ephemeral-setup-terminal-worktree-id' import { TERMINAL_PAIRED_PARKING_RUNTIME_CAPABILITY } from '../../../../shared/protocol-version' @@ -341,7 +342,7 @@ import { directSshAuthoritiesEqual } from '@/store/slices/direct-ssh-terminal-au import { isProvenSshSessionGoneError } from './reattach-failure-classification' const pendingSpawnByPaneKey = new Map>() -const SSH_SESSION_EXPIRED_ERROR = 'SSH_SESSION_EXPIRED' + // Why: relay requests expire at 30s; leave one second for their fallback before re-arming locally. const DIRECT_SSH_PANE_RETRY_SETTLEMENT_TIMEOUT_MS = 31_000 const REMOTE_PTY_ID_PREFIX = 'remote:' diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index b302f631f86..bf7c6fbbbd4 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -13,6 +13,7 @@ import { iterateTerminalInputChunks } from '../../../../shared/terminal-input' import { isRuntimeOwnedSshTargetId } from '../../../../shared/execution-host' +import { SSH_SESSION_EXPIRED_ERROR } from '../../../../shared/ssh-pty-failure-tokens' import { ptyDataHandlers, ptyReplayHandlers, @@ -71,7 +72,6 @@ export type { } from './pty-transport-types' export { extractLastOscTitle } from '../../../../shared/agent-detection' -const SSH_SESSION_EXPIRED_ERROR = 'SSH_SESSION_EXPIRED' // Why: main rejects a session reattached under a different SSH connection with this phrase; treat as stale (spawn fresh), not a crash. const SSH_PTY_CONNECTION_MISMATCH_MARKER = 'belongs to SSH connection' const STALE_TITLE_TIMEOUT = 3000 // ms before stale working title is cleared diff --git a/src/shared/ssh-pty-failure-tokens.ts b/src/shared/ssh-pty-failure-tokens.ts index fac7cd53512..9ecb38e1e94 100644 --- a/src/shared/ssh-pty-failure-tokens.ts +++ b/src/shared/ssh-pty-failure-tokens.ts @@ -14,14 +14,9 @@ export const SSH_PTY_IDENTITY_MISMATCH_ERROR = 'SSH_PTY_IDENTITY_MISMATCH' /** The shell is alive; only its output source must be re-established. */ export const SSH_SOURCE_RESTORE_REQUIRED_ERROR = 'SSH_SOURCE_RESTORE_REQUIRED' -/** - * The relay WATCHED this shell exit. That is first-hand knowledge from the process that owned it, - * and the only answer that proves death: a relay which merely never knew the id may be a - * replacement whose predecessor's shells are still running. - * - * Deliberately worded so it cannot be mistaken for the ordinary unknown — the phrase - * `PTY "" not found` is what an older client maps to expiry, and expiry authorizes a respawn. - */ +/** The relay WATCHED this exit, so it proves death — unlike an unknown id, which a replacement + * relay also answers for shells still running. Worded to never read as `not found`, which older + * clients map to expiry, and expiry authorizes a respawn. */ export const SSH_PTY_EXITED_ERROR = 'SSH_PTY_EXITED' /** The carrier is the message: the relay's error transport drops structured payloads. */ @@ -29,12 +24,8 @@ export function formatPtyExitedError(id: string, code: number, incarnationId: st return `${SSH_PTY_EXITED_ERROR}: ${encodeURIComponent(id)} code=${code} incarnation=${encodeURIComponent(incarnationId)}` } -/** - * Anchored on the WHOLE grammar, not the token. This answer authorizes replacing a shell, so a - * substring test would let any text that merely quotes the token — a log line, a wrapped error, a - * shell printing it — stand in for the relay's own observation. Ids and incarnations are - * percent-encoded at the source so neither can contain a space and forge the fields after it. - */ +/** Whole grammar, not the token: this authorizes replacing a shell, so any text merely quoting the + * token must not qualify. Fields are percent-encoded at the source, so none can forge the next. */ const SSH_PTY_EXITED_MESSAGE = new RegExp( `(?:^|[^A-Z_])${SSH_PTY_EXITED_ERROR}: ([^\\s]+) code=(-?\\d+) incarnation=([^\\s]+)` ) @@ -43,11 +34,8 @@ export function isSshPtyExitedMessage(message: string): boolean { return SSH_PTY_EXITED_MESSAGE.test(message) } -/** - * The shell the proof is about. A caller must check this against the shell it asked for before - * acting: the host enforces the same rule, but the host is the party whose answer is in question, - * and versions differ. An unverifiable proof is simply not proof. - */ +/** The shell the proof names. Callers must check it themselves: the host applies the same rule, but + * the host is the party whose answer is in question and versions differ. */ export function parsePtyExitedError( message: string ): { id: string; code: number; incarnationId: string } | null {