fix(terminal): quarantine input after a replaced PTY endpoint (STA-2373) (#10811)
Follow-up to #10065, which merged with this failure mode known and deferred. Recovery from a dead daemon socket reattaches to a fresh shell. Keystrokes in flight during the ~1.1s window are dropped, but everything typed after reattach 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` arrives as `cho hi; rm -rf x` — zsh fails `cho` and still runs `rm -rf x`. Before #10065 the whole line was lost, so the executing tail is new. Quarantine the remainder of the interrupted line instead. Keyed by tab, not pane: recovery destroys the xterm being typed into and the successor pane receives the tail. Armed only from the onWriteUnavailable path; a stalled-pipeline remount keeps the same live shell, where quarantining would eat a real command. Disarms on the line terminator (CR/LF/Ctrl-C, dropped too since that is the byte that would submit the mangled line), a 700ms idle gap, or a 5s cap. The cap must not be shortened: the tail itself takes ~2.5s to type, so a shorter cap fires mid-tail and delivers the dangerous remainder to the fresh shell. The onData check sits after the query-reply branch so CPR/DSR replies still reach the shell. Live A/B QA on macOS: the bug reproduces verbatim without the fix (marker file created by the surviving tail) and is suppressed with it, with the next command still working — proving suppression rather than a dead pane.
This commit is contained in:
parent
4109f4eec5
commit
8db61248ec
|
|
@ -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')
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
|
|
@ -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<string, QuarantineEntry>()
|
||||
|
||||
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()
|
||||
}
|
||||
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue