From 4993029ea7e241492e8795ed3695a2b0ee7d16f4 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 26 Jul 2026 15:15:18 -0700 Subject: [PATCH] fix(terminal): quarantine input after a dead-endpoint recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit STA-2373 recovery replaces a dead daemon's shell with a fresh one, but the user is usually mid-line when the daemon dies. Keystrokes racing the death are dropped and everything typed after re-attach lands on the NEW shell, so it receives the tail of a line whose head no longer exists. Measured live: after killing the daemon and sending digits 1-9 one per ~1.1s, the shell ran `23456789` — only `1` was consumed as the recovery trigger. That is harmless for a simple command, but a compound one is not: losing the head of `echo hi; rm -rf x` yields `cho hi; rm -rf x`, where zsh fails `cho` and then still executes `rm -rf x` after the `;`. Quarantine input from the dead-endpoint signal until the next line boundary, swallowing that boundary so the fresh shell starts at a clean prompt. State is module-scoped by tabId because recovery remounts the pane, discarding any closure state exactly when it would be needed. A 15s deadline guarantees input is never permanently dead if the user never submits. --- .../terminal-pane/pty-connection.test.ts | 52 ++++++++++++ .../terminal-pane/pty-connection.ts | 15 ++++ .../terminal-input-quarantine.test.ts | 82 +++++++++++++++++++ .../terminal-input-quarantine.ts | 77 +++++++++++++++++ 4 files changed, 226 insertions(+) create mode 100644 src/renderer/src/components/terminal-pane/terminal-input-quarantine.test.ts create mode 100644 src/renderer/src/components/terminal-pane/terminal-input-quarantine.ts 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 ca3b9cb777c..0ed2f6c3e04 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -4779,6 +4779,58 @@ describe('connectPanePty', () => { expect(window.api.agentStatus.inferInterrupt).not.toHaveBeenCalled() }) + it('quarantines input after a dead-endpoint signal so the fresh shell never runs a line fragment', async () => { + const { connectPanePty } = await import('./pty-connection') + const { _resetTerminalPaneRecoveryForTests } = await import('./terminal-pane-recovery') + const { _resetTerminalInputQuarantineForTests } = await import('./terminal-input-quarantine') + _resetTerminalPaneRecoveryForTests() + _resetTerminalInputQuarantineForTests() + 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) + const sendInputAccepted = transport.sendInputAccepted + expect(sendInputAccepted).toBeTypeOf('function') + transport.sendInput.mockClear() + sendInputAccepted?.mockClear() + + // Why revert-sensitive: the daemon died mid-line, so `echo hi; ` is already + // gone. Without the quarantine this tail reaches the replacement shell, zsh + // fails `cho hi` and then still executes `rm -rf x` after the `;`. + sendTerminalInputThroughPane(pane, 'cho hi; rm -rf x') + await flushAsyncTicks(2) + expect(transport.sendInput).not.toHaveBeenCalled() + expect(sendInputAccepted).not.toHaveBeenCalled() + + // The submit for that mangled line is swallowed too, leaving a clean prompt. + sendTerminalInputThroughPane(pane, '\r') + await flushAsyncTicks(2) + expect(transport.sendInput).not.toHaveBeenCalled() + expect(sendInputAccepted).not.toHaveBeenCalled() + + // ...and the next line the user types is delivered normally. + sendTerminalInputThroughPane(pane, 'ls') + await flushAsyncTicks(2) + const delivered = [ + ...transport.sendInput.mock.calls.flat(), + ...(sendInputAccepted?.mock.calls.flat() ?? []) + ] + expect(delivered).toContain('ls') + _resetTerminalInputQuarantineForTests() + _resetTerminalPaneRecoveryForTests() + }) + it('remounts a connected pane when main reports its daemon write unavailable', async () => { const { connectPanePty } = await import('./pty-connection') const { _resetTerminalPaneRecoveryForTests } = await import('./terminal-pane-recovery') diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index af467597005..e1caf63f958 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -84,6 +84,10 @@ import { registerTerminalPaneRecoveryInstance, requestTerminalPaneRecovery } from './terminal-pane-recovery' +import { + armTerminalInputQuarantine, + shouldQuarantineTerminalInput +} from './terminal-input-quarantine' import { isDocumentVisibilityProvenStale, registerStaleDocumentVisibilityRecovery @@ -3717,6 +3721,13 @@ export function connectPanePty( sendDesktopQueryReplyImmediate(data) return } + // Why here: after the query-reply bypass, so xterm's own auto-replies still + // flow, but before any send path — a quarantined pane must not deliver the + // tail of a line whose head died with the old daemon (STA-2373 follow-up). + if (shouldQuarantineTerminalInput(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 @@ -5329,6 +5340,10 @@ export function connectPanePty( }, onWriteUnavailable: (): void => { if (isCurrent()) { + // Arm before requesting recovery: the remount that recovery triggers + // builds a fresh connection, so only module-scoped quarantine state + // survives to gate the new pane's input. + armTerminalInputQuarantine(deps.tabId) requestRecoveryForUndeliverableInput(true) } }, 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 00000000000..1e7713d0c60 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-input-quarantine.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, beforeEach } from 'vitest' +import { + armTerminalInputQuarantine, + shouldQuarantineTerminalInput, + isTerminalInputQuarantined, + releaseTerminalInputQuarantine, + _resetTerminalInputQuarantineForTests +} from './terminal-input-quarantine' + +describe('terminal input quarantine', () => { + beforeEach(() => { + _resetTerminalInputQuarantineForTests() + }) + + it('passes input through when nothing armed it', () => { + expect(shouldQuarantineTerminalInput('tab-1', 'l')).toBe(false) + }) + + it('drops the tail of a line whose head died with the old daemon', () => { + // Why revert-sensitive: without the quarantine these characters reach the + // replacement shell, which then runs the fragment of a command the user + // never completed (STA-2373: `echo hi; rm -rf x` -> `cho hi; rm -rf x`, + // where zsh fails `cho` but still executes `rm -rf x` after the `;`). + armTerminalInputQuarantine('tab-1') + for (const ch of 'cho hi; rm -rf x') { + expect(shouldQuarantineTerminalInput('tab-1', ch)).toBe(true) + } + }) + + it('swallows the submit that would have run the mangled line, then releases', () => { + armTerminalInputQuarantine('tab-1') + expect(shouldQuarantineTerminalInput('tab-1', 'x')).toBe(true) + expect(shouldQuarantineTerminalInput('tab-1', '\r')).toBe(true) + expect(isTerminalInputQuarantined('tab-1')).toBe(false) + // The next line is typed against a clean prompt and must reach the shell. + expect(shouldQuarantineTerminalInput('tab-1', 'l')).toBe(false) + }) + + it('treats a newline as a line boundary too', () => { + armTerminalInputQuarantine('tab-1') + expect(shouldQuarantineTerminalInput('tab-1', '\n')).toBe(true) + expect(isTerminalInputQuarantined('tab-1')).toBe(false) + }) + + it('releases a chunk that carries its own newline, so pasted input is not stuck', () => { + armTerminalInputQuarantine('tab-1') + expect(shouldQuarantineTerminalInput('tab-1', 'abc\r')).toBe(true) + expect(isTerminalInputQuarantined('tab-1')).toBe(false) + }) + + it('quarantines each tab independently', () => { + armTerminalInputQuarantine('tab-1') + expect(shouldQuarantineTerminalInput('tab-1', 'a')).toBe(true) + expect(shouldQuarantineTerminalInput('tab-2', 'a')).toBe(false) + }) + + it('releases on the deadline so input is never permanently dead', () => { + // Why: the normal release is the user's next Enter. Someone who switches away + // mid-line never sends one, and a pane that silently eats input forever would + // be a worse bug than the one being fixed. + const t0 = 1_000_000 + armTerminalInputQuarantine('tab-1', t0) + expect(shouldQuarantineTerminalInput('tab-1', 'a', t0 + 14_999)).toBe(true) + expect(shouldQuarantineTerminalInput('tab-1', 'a', t0 + 15_000)).toBe(false) + expect(isTerminalInputQuarantined('tab-1')).toBe(false) + }) + + it('keeps the original deadline when the same incident re-arms', () => { + // Both the provider fan-out and the per-write catch signal the written pane, + // so arming happens more than once per death; that must not extend the window. + const t0 = 1_000_000 + armTerminalInputQuarantine('tab-1', t0) + armTerminalInputQuarantine('tab-1', t0 + 10_000) + expect(shouldQuarantineTerminalInput('tab-1', 'a', t0 + 15_000)).toBe(false) + }) + + it('can be released explicitly', () => { + armTerminalInputQuarantine('tab-1') + releaseTerminalInputQuarantine('tab-1') + expect(shouldQuarantineTerminalInput('tab-1', 'a')).toBe(false) + }) +}) 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 00000000000..1ba8d954823 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-input-quarantine.ts @@ -0,0 +1,77 @@ +// Why this module exists: STA-2373 recovery replaces a dead daemon's shell with a +// fresh one, but the user is usually mid-line when the daemon dies. The keystrokes +// that raced the death are dropped, and everything typed after re-attach lands on +// the NEW shell — so the shell receives the *tail* of a line whose head no longer +// exists. Typing `echo hi; rm -rf x` and losing the head yields `cho hi; rm -rf x`, +// where zsh fails `cho` and then still runs `rm -rf x`, because `;` separates +// commands. Partially executing a command the user never completed is worse than +// dropping it, so quarantine input until the line boundary and start clean. +// +// State lives at module scope keyed by tabId, not in the pane closure: recovery +// remounts the pane, which builds a brand-new connection: closure state would be +// discarded exactly when the quarantine needs to apply. + +// Why a cap: the release condition is the user's next Enter, which normally arrives +// within seconds. If they instead switch away mid-line and never submit, input must +// not stay dead — release on a timer so the worst case is bounded. +const QUARANTINE_MAX_MS = 15_000 + +type Quarantine = { armedAt: number } + +const quarantineByTabId = new Map() + +function isLineBoundary(data: string): boolean { + return data.includes('\r') || data.includes('\n') +} + +/** + * Arm after a dead-endpoint recovery so the fresh shell cannot receive the tail of + * a line whose head was dropped. Idempotent: re-arming keeps the original deadline + * so repeated signals for the same incident cannot extend the window indefinitely. + */ +export function armTerminalInputQuarantine(tabId: string, now = Date.now()): void { + if (quarantineByTabId.has(tabId)) { + return + } + quarantineByTabId.set(tabId, { armedAt: now }) +} + +/** + * Returns true when this input must be dropped instead of reaching the shell. + * + * Consumes the quarantine on a line boundary (the Enter that would have submitted + * the mangled line is itself swallowed, leaving the fresh shell at a clean prompt) + * and on deadline expiry. + */ +export function shouldQuarantineTerminalInput( + tabId: string, + data: string, + now = Date.now() +): boolean { + const quarantine = quarantineByTabId.get(tabId) + if (!quarantine) { + return false + } + if (now - quarantine.armedAt >= QUARANTINE_MAX_MS) { + quarantineByTabId.delete(tabId) + return false + } + if (isLineBoundary(data)) { + // Drop this one too: it is the submit for a line the shell never fully saw. + quarantineByTabId.delete(tabId) + return true + } + return true +} + +export function isTerminalInputQuarantined(tabId: string): boolean { + return quarantineByTabId.has(tabId) +} + +export function releaseTerminalInputQuarantine(tabId: string): void { + quarantineByTabId.delete(tabId) +} + +export function _resetTerminalInputQuarantineForTests(): void { + quarantineByTabId.clear() +}