diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index 3b8c434c9..ee0880d26 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -4803,6 +4803,42 @@ describe('connectPanePty', () => { _resetTerminalPaneRecoveryForTests() }) + it('quarantines the interrupted line after a write-unavailable remount, but never device replies', async () => { + const { connectPanePty } = await import('./pty-connection') + const { _resetTerminalPaneRecoveryForTests } = await import('./terminal-pane-recovery') + _resetTerminalPaneRecoveryForTests() + const remountTerminalTabForRecovery = vi.fn<(tabId: string) => boolean>(() => true) + mockStoreState = { ...mockStoreState, remountTerminalTabForRecovery } as StoreState + const transport = createMockTransport('daemon-pty') + let writeUnavailable: (() => void) | undefined + transport.connect.mockImplementation(async ({ callbacks }: { callbacks: ConnectCallbacks }) => { + writeUnavailable = callbacks.onWriteUnavailable + return { id: 'daemon-pty' } + }) + transportFactoryQueue.push(transport) + const pane = createPane(1) + + connectPanePty(pane as never, createManager(1) as never, createDeps() as never) + await flushAsyncTicks(6) + writeUnavailable?.() + await flushAsyncTicks(6) + expect(remountTerminalTabForRecovery).toHaveBeenCalledWith('tab-1') + + // The surviving tail of `echo hi; rm -rf x`: reaching the fresh shell would + // let the user's own Enter run `rm -rf x` (#10065 follow-up). + sendTerminalInputThroughPane(pane, 'cho hi; rm -rf x') + expect(transport.sendInput).not.toHaveBeenCalledWith('cho hi; rm -rf x') + // A program that queries during reattach hangs if its reply is dropped. + sendTerminalInputThroughPane(pane, '\x1b[3;1R') + expect(transport.sendInputImmediate).toHaveBeenCalledWith('\x1b[3;1R') + sendTerminalInputThroughPane(pane, '\r') + expect(transport.sendInput).not.toHaveBeenCalledWith('\r') + // The terminator disarmed it, so the next real command reaches the shell. + sendTerminalInputThroughPane(pane, 'ls\r') + expect(transport.sendInput).toHaveBeenCalledWith('ls\r') + _resetTerminalPaneRecoveryForTests() + }) + it('recovers a wedged write pipeline after accepted input without renderer output', async () => { vi.useFakeTimers() const { connectPanePty } = await import('./pty-connection') diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index b5ec43b2e..fdeccfd7b 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -87,6 +87,7 @@ import { registerTerminalPaneRecoveryInstance, requestTerminalPaneRecovery } from './terminal-pane-recovery' +import { shouldDropQuarantinedTerminalInput } from './terminal-input-quarantine' import { isDocumentVisibilityProvenStale, registerStaleDocumentVisibilityRecovery @@ -3662,7 +3663,11 @@ export function connectPanePty( // and a disconnected remote pane would otherwise remount-churn on every // cooldown window while typing. Local panes keep the lenient gate. requireAuthoritativeLiveness: - Boolean(transport.getConnectionId?.()) || isRemoteRuntimePtyId(undeliverablePtyId) + Boolean(transport.getConnectionId?.()) || isRemoteRuntimePtyId(undeliverablePtyId), + // Why only the rejected path: it is the only one whose remount can land on + // a fresh shell. A stalled-pipeline remount always reattaches to the same + // shell, so its half-typed line is still on screen and intact. + endpointReplaced: providerRejected }) } // Why: the write-pipeline health watch (scheduler stall probe, replay-guard @@ -3743,6 +3748,15 @@ export function connectPanePty( sendDesktopQueryReplyImmediate(data) return } + // Why after the query-reply branch: device replies are not user input and + // must always reach the shell, or a program querying during reattach hangs. + // Why at all: a replaced endpoint reattaches to a fresh shell, so the tail + // of the interrupted line would be submitted by the user's own Enter and a + // compound command could run its surviving half (#10065 follow-up). + if (shouldDropQuarantinedTerminalInput(deps.tabId, data)) { + clearPendingTerminalInputIntent() + return + } const intent = pendingTerminalInputIntent // Why: real xterm can deliver the terminal byte even when our DOM keydown // listener missed the press. Exact Ctrl+C/Escape bytes are still safe to diff --git a/src/renderer/src/components/terminal-pane/terminal-input-quarantine.test.ts b/src/renderer/src/components/terminal-pane/terminal-input-quarantine.test.ts new file mode 100644 index 000000000..63679bec8 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-input-quarantine.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect, beforeEach } from 'vitest' +import { + _resetTerminalInputQuarantineForTests, + armTerminalInputQuarantine, + isTerminalInputQuarantined, + shouldDropQuarantinedTerminalInput +} from './terminal-input-quarantine' + +const TAB = 'tab-1' +// Re-attach measured at ~1.1s in STA-2373 live QA; the tail lands just after. +const REATTACH_MS = 1_100 + +beforeEach(() => { + _resetTerminalInputQuarantineForTests() +}) + +describe('terminal input quarantine', () => { + it('passes input through when nothing is armed', () => { + expect(shouldDropQuarantinedTerminalInput(TAB, 'e', 0)).toBe(false) + expect(isTerminalInputQuarantined(TAB)).toBe(false) + }) + + it('drops the surviving tail of an interrupted line and its Enter', () => { + armTerminalInputQuarantine(TAB, 0) + // The tail of `echo hi; rm -rf x` after the head was eaten by recovery. + let at = REATTACH_MS + for (const char of 'cho hi; rm -rf x') { + expect(shouldDropQuarantinedTerminalInput(TAB, char, at)).toBe(true) + at += 30 + } + // The user's own Enter would have submitted the mangled line. + expect(shouldDropQuarantinedTerminalInput(TAB, '\r', at)).toBe(true) + expect(isTerminalInputQuarantined(TAB)).toBe(false) + }) + + it('lets the next command through once the terminator disarmed it', () => { + armTerminalInputQuarantine(TAB, 0) + expect(shouldDropQuarantinedTerminalInput(TAB, 'x', REATTACH_MS)).toBe(true) + expect(shouldDropQuarantinedTerminalInput(TAB, '\r', REATTACH_MS + 30)).toBe(true) + expect(shouldDropQuarantinedTerminalInput(TAB, 'l', REATTACH_MS + 60)).toBe(false) + }) + + it.each([ + ['carriage return', '\r'], + ['newline', '\n'], + ['ctrl-c', '\x03'] + ])('treats %s as the line terminator', (_label, terminator) => { + armTerminalInputQuarantine(TAB, 0) + expect(shouldDropQuarantinedTerminalInput(TAB, terminator, REATTACH_MS)).toBe(true) + expect(isTerminalInputQuarantined(TAB)).toBe(false) + }) + + it('drops a pasted tail that carries its terminator mid-chunk', () => { + armTerminalInputQuarantine(TAB, 0) + expect(shouldDropQuarantinedTerminalInput(TAB, 'cho hi; rm -rf x\r', REATTACH_MS)).toBe(true) + expect(isTerminalInputQuarantined(TAB)).toBe(false) + }) + + it('releases on an idle gap once a quarantined byte has been seen', () => { + armTerminalInputQuarantine(TAB, 0) + // Burst tail, no Enter — the user was still mid-line when the endpoint died. + expect(shouldDropQuarantinedTerminalInput(TAB, 'c', REATTACH_MS)).toBe(true) + expect(shouldDropQuarantinedTerminalInput(TAB, 'h', REATTACH_MS + 40)).toBe(true) + // Then they notice, pause, and type a real command. + expect(shouldDropQuarantinedTerminalInput(TAB, 'l', REATTACH_MS + 40 + 700)).toBe(false) + expect(isTerminalInputQuarantined(TAB)).toBe(false) + }) + + // The accepted cost of the cap, pinned so it is not "fixed" by shortening it: + // over-quarantine is visible (nothing echoes) and recoverable by retyping, + // while under-quarantine executes half a command. A tail takes seconds to + // type, so a cap short enough to spare this command would cut that tail + // mid-line and deliver its remainder to the fresh shell. + it('eats a fresh command typed inside the cap, terminator included', () => { + armTerminalInputQuarantine(TAB, 0) + // No byte yet, so the idle gate cannot release the first keystroke however + // long the user waited; from there normal typing never opens a 700ms gap. + let at = 1_500 + for (const char of 'ls -la\r') { + expect(shouldDropQuarantinedTerminalInput(TAB, char, at)).toBe(true) + at += 150 + } + }) + + it('does not let the idle gate fire on the re-attach delay itself', () => { + armTerminalInputQuarantine(TAB, 0) + // First quarantined byte arrives well past the idle window, because the + // remount itself took that long. It is still the interrupted line. + expect(shouldDropQuarantinedTerminalInput(TAB, 'c', REATTACH_MS)).toBe(true) + }) + + it('releases at the absolute cap so input can never wedge', () => { + armTerminalInputQuarantine(TAB, 0) + expect(shouldDropQuarantinedTerminalInput(TAB, 'a', 4_999)).toBe(true) + armTerminalInputQuarantine(TAB, 0) + expect(shouldDropQuarantinedTerminalInput(TAB, 'a', 5_000)).toBe(false) + expect(isTerminalInputQuarantined(TAB)).toBe(false) + }) + + it('keeps quarantine per tab', () => { + armTerminalInputQuarantine(TAB, 0) + expect(shouldDropQuarantinedTerminalInput('tab-2', 'a', REATTACH_MS)).toBe(false) + expect(shouldDropQuarantinedTerminalInput(TAB, 'a', REATTACH_MS)).toBe(true) + }) + + it('prunes expired entries for tabs that closed mid-quarantine', () => { + armTerminalInputQuarantine('closed-tab', 0) + armTerminalInputQuarantine(TAB, 5_000) + expect(isTerminalInputQuarantined('closed-tab')).toBe(false) + expect(isTerminalInputQuarantined(TAB)).toBe(true) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-input-quarantine.ts b/src/renderer/src/components/terminal-pane/terminal-input-quarantine.ts new file mode 100644 index 000000000..416227702 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-input-quarantine.ts @@ -0,0 +1,88 @@ +// Why this module exists: when a dead daemon endpoint is replaced (STA-2373, +// #10065), the pane remounts and re-attaches to a *fresh* shell. Keystrokes in +// flight during that window are dropped, but everything typed after re-attach +// lands on the new shell — so the surviving tail of a half-sent line is +// submitted by the user's own Enter. `echo hi; rm -rf x` can arrive as +// `cho hi; rm -rf x`: zsh fails `cho` and still runs `rm -rf x`. Before #10065 +// the whole line was silently lost, so the executing tail is a new risk. +// +// Quarantine drops the remainder of the interrupted line so a mangled command +// can never be submitted. It must be keyed by tab, not by pane: recovery +// destroys the xterm that was being typed into, and the successor pane is the +// one that receives the tail. + +/** Absolute cap from arming. A safety valve only — input must never wedge, + * even if the terminator never arrives and the pane keeps receiving data. */ +const QUARANTINE_MAX_MS = 5_000 +/** A gap this long proves the interrupted burst ended. Re-attach takes ~1.1s, + * so the tail arrives back-to-back (<100ms apart) while a human reacting to a + * recovered pane takes far longer — that gap is what separates the two. */ +const QUARANTINE_IDLE_MS = 700 + +/** CR/LF submit the line; Ctrl-C abandons it. Dropping the terminator itself is + * the point — it is the byte that would have submitted the mangled line. */ +const LINE_TERMINATORS = ['\r', '\n', '\x03'] + +type QuarantineEntry = { + armedAt: number + /** null until the first quarantined byte, so the idle gate cannot fire on the + * re-attach delay itself. */ + lastInputAt: number | null +} + +const quarantineByTabId = new Map() + +function containsLineTerminator(data: string): boolean { + return LINE_TERMINATORS.some((terminator) => data.includes(terminator)) +} + +/** Arm only when the endpoint stopped accepting writes and may have been + * replaced. A recovery that always keeps the same live shell has no mangled + * line to suppress, and quarantining there would eat a legitimate command. */ +export function armTerminalInputQuarantine(tabId: string, now: number = Date.now()): void { + // Prune first: entries are only meaningful for QUARANTINE_MAX_MS, and a tab + // closed mid-quarantine would otherwise leave one behind forever. + for (const [otherTabId, entry] of quarantineByTabId) { + if (now - entry.armedAt >= QUARANTINE_MAX_MS) { + quarantineByTabId.delete(otherTabId) + } + } + quarantineByTabId.set(tabId, { armedAt: now, lastInputAt: null }) +} + +export function isTerminalInputQuarantined(tabId: string): boolean { + return quarantineByTabId.has(tabId) +} + +/** + * True when `data` belongs to the interrupted line and must not reach the fresh + * shell. Disarms on the line terminator, on an idle gap that proves the burst + * ended, and on the absolute cap. + */ +export function shouldDropQuarantinedTerminalInput( + tabId: string, + data: string, + now: number = Date.now() +): boolean { + const entry = quarantineByTabId.get(tabId) + if (!entry) { + return false + } + if (now - entry.armedAt >= QUARANTINE_MAX_MS) { + quarantineByTabId.delete(tabId) + return false + } + if (entry.lastInputAt !== null && now - entry.lastInputAt >= QUARANTINE_IDLE_MS) { + quarantineByTabId.delete(tabId) + return false + } + entry.lastInputAt = now + if (containsLineTerminator(data)) { + quarantineByTabId.delete(tabId) + } + return true +} + +export function _resetTerminalInputQuarantineForTests(): void { + quarantineByTabId.clear() +} diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts index 7e0486523..51ccff409 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.test.ts @@ -5,6 +5,7 @@ import { registerTerminalPaneRecoveryInstance, requestTerminalPaneRecovery } from './terminal-pane-recovery' +import { isTerminalInputQuarantined } from './terminal-input-quarantine' const mocks = vi.hoisted(() => ({ remountTerminalTabForRecovery: vi.fn<(tabId: string) => boolean>(() => true), @@ -478,4 +479,55 @@ describe('requestTerminalPaneRecovery', () => { expect.anything() ) }) + + // Why: quarantine suppresses real keystrokes, so arming it on a recovery that + // kept the same shell would eat a legitimate command (#10065 follow-up). + describe('input quarantine arming', () => { + it('arms after a replaced endpoint so the mangled line cannot be submitted', async () => { + const result = await requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: 'pty-1', + reason: 'input-undeliverable', + endpointReplaced: true + }) + + expect(result).toBe(true) + expect(isTerminalInputQuarantined('tab-1')).toBe(true) + }) + + it('does not arm when the same live shell is reattached', async () => { + const result = await requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: 'pty-1', + reason: 'input-undeliverable' + }) + + expect(result).toBe(true) + expect(isTerminalInputQuarantined('tab-1')).toBe(false) + }) + + it('does not arm for a stalled write pipeline', async () => { + await requestTerminalPaneRecovery({ + tabId: 'tab-1', + ptyId: 'pty-1', + reason: 'write-stalled' + }) + + expect(isTerminalInputQuarantined('tab-1')).toBe(false) + }) + + it('does not arm when the remount never happened', async () => { + mocks.remountTerminalTabForRecovery.mockReturnValue(false) + + const result = await requestTerminalPaneRecovery({ + tabId: 'tab-gone', + ptyId: 'pty-1', + reason: 'input-undeliverable', + endpointReplaced: true + }) + + expect(result).toBe(false) + expect(isTerminalInputQuarantined('tab-gone')).toBe(false) + }) + }) }) diff --git a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts index 791234b23..ca3d23dae 100644 --- a/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts +++ b/src/renderer/src/components/terminal-pane/terminal-pane-recovery.ts @@ -1,5 +1,9 @@ import { useAppStore } from '@/store' import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' +import { + _resetTerminalInputQuarantineForTests, + armTerminalInputQuarantine +} from './terminal-input-quarantine' // Why this module exists: a terminal pane can die renderer-side while its PTY // stays alive — a wedged xterm WriteBuffer (issue #2836), a disposed xterm @@ -34,6 +38,12 @@ type RecoveryRequest = { * registry doesn't own, and treating null as "proceed" would let a * disconnected remote pane churn reconnects on every cooldown window. */ requireAuthoritativeLiveness?: boolean + /** The provider rejected the write because its endpoint stopped accepting + * writes, so re-attach MAY land on a *fresh* shell (a respawn; a transient + * socket drop reconnects to the same sessions). Only this path can mangle the + * in-flight line, and only it may quarantine input — a recovery that always + * keeps the same live shell would have a legitimate command eaten. */ + endpointReplaced?: boolean } // Why a cap exists: recovery must never loop. If the remounted pane wedges @@ -261,6 +271,11 @@ export async function requestTerminalPaneRecovery(request: RecoveryRequest): Pro // A remount replaces every pane xterm in the tab; a previously scheduled // retry would only re-remount the fresh, healthy panes. cancelPendingRecoveryRetry(request.tabId) + if (request.endpointReplaced) { + // Why here and not at request time: arming before the remount is certain + // would suppress input on a pane that never recovered. + armTerminalInputQuarantine(request.tabId) + } console.error( `[terminal] recovering pane tab ${request.tabId} — ${request.reason} with a live PTY (${request.ptyId ?? 'unbound'}); remounting to rebuild the renderer` ) @@ -280,4 +295,5 @@ export function _resetTerminalPaneRecoveryForTests(): void { clearTimeout(pendingRetry.timer) } pendingRetryByTabId.clear() + _resetTerminalInputQuarantineForTests() }