mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 16:03:26 +00:00
fix(terminal): quarantine input after a dead-endpoint recovery
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.
This commit is contained in:
@@ -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')
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
},
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
@@ -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<string, Quarantine>()
|
||||
|
||||
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()
|
||||
}
|
||||
Reference in New Issue
Block a user