Make replay-guard stall release probe-certified instead of time-based

The previous stall watchdog blindly released the input guard after 10s. If a replay were genuinely still parsing on a starved machine, that early release could leak xterm's auto-replies into the shell — and into agent TUIs, where a leaked ESC reads as the user pressing Escape. Replace the blind release with a probe: when a completion looks overdue, enqueue an empty write behind the replay. xterm parses writes in order, so every outcome is provably safe: - probe parses after the replay completion ran: normal release already happened; probe is a no-op. - probe parses but the replay completion never ran: all replay bytes have parsed, no further auto-replies can exist — the completion was genuinely lost. Release + breadcrumb. - probe never parses (bounded wait): the pipeline is wedged, and a dead parser can never emit auto-replies, so releasing cannot leak input. Release + breadcrumb naming the pane as needing recovery. While the probe is pending — a slow-but-alive replay — the guard now HOLDS instead of releasing early; that case is pinned by a regression test. Co-authored-by: Orca <help@stably.ai>
2026-10-06 00:02:43 +00:00 · 2026-07-02 17:52:46 -04:00
parent 92abd8cfa7
commit bdf798b439
2 changed files with 170 additions and 51 deletions
@@ -191,28 +191,55 @@ describe('replay-guard', () => {
  })
 })

-describe('replay-guard watchdog', () => {
-  it('force-releases the guard when xterm never completes the write (wedged pipeline repro)', () => {
-    // Why: a sync throw escaping xterm's WriteBuffer loop drops the pending
-    // write completion forever (xterm-write-buffer-stall.repro.test.ts).
-    // Without the watchdog the guard latches and pty-connection.ts onData
-    // silently eats every keystroke on a live pane.
+describe('replay-guard stall handling (probe-certified release)', () => {
+  it('HOLDS the guard while a slow replay is still parsing — a probe is queued, never a blind release', () => {
+    // Why this is the load-bearing safety test: a time-based release here
+    // would leak xterm auto-replies into the shell (and a leaked ESC into an
+    // agent TUI reads as the user pressing Escape). The guard must only
+    // release when the pipeline itself proves the replay parsed.
+    vi.useFakeTimers()
+    const ref = makeRef()
+    const { pane, terminal } = makeFakePane(1)
+
+    replayIntoTerminal(pane, ref, 'slow but alive', 1_000)
+    expect(isPaneReplaying(ref, 1)).toBe(true)
+
+    // Stall check fires: an empty probe write is enqueued behind the replay.
+    vi.advanceTimersByTime(1_000)
+    expect(terminal.lastData).toEqual(['slow but alive', ''])
+    // Probe is pending → replay genuinely still parsing → guard holds.
+    expect(isPaneReplaying(ref, 1)).toBe(true)
+    vi.advanceTimersByTime(999)
+    expect(isPaneReplaying(ref, 1)).toBe(true)
+
+    // Parsing finishes: FIFO runs the replay completion first (normal
+    // release), then the probe completion as a no-op.
+    terminal.flush()
+    expect(isPaneReplaying(ref, 1)).toBe(false)
+    expect(ref.current.has(1)).toBe(false)
+    expect(mocks.recordRendererCrashBreadcrumb).not.toHaveBeenCalled()
+
+    vi.advanceTimersByTime(120_000)
+    expect(ref.current.has(1)).toBe(false)
+  })
+
+  it('releases when the probe parses but the replay completion was lost, and reports it', () => {
    vi.useFakeTimers()
    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
    try {
      const ref = makeRef()
-      const { pane } = makeFakePane(1)
+      const { pane, terminal } = makeFakePane(1)

-      replayIntoTerminal(pane, ref, 'restored bytes', 10_000)
-      expect(isPaneReplaying(ref, 1)).toBe(true)
+      replayIntoTerminal(pane, ref, 'restored bytes', 1_000)
+      terminal.pendingCallbacks.shift() // xterm lost the replay's completion
+      vi.advanceTimersByTime(1_000) // stall check → probe enqueued

-      vi.advanceTimersByTime(9_999)
-      expect(isPaneReplaying(ref, 1)).toBe(true)
-
-      vi.advanceTimersByTime(1)
+      // The probe's completion firing certifies every earlier replay byte
+      // parsed — releasing now cannot leak auto-replies.
+      terminal.flush()
      expect(isPaneReplaying(ref, 1)).toBe(false)
      expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith(
-        'terminal_replay_guard_watchdog_release',
+        'terminal_replay_guard_lost_completion',
        { paneId: 1 }
      )
    } finally {
@@ -220,28 +247,68 @@ describe('replay-guard watchdog', () => {
    }
  })

-  it('does not double-decrement when the completion arrives after the watchdog fired', () => {
+  it('releases after the probe itself never parses (wedged pipeline) and reports it', () => {
+    vi.useFakeTimers()
+    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+    try {
+      const ref = makeRef()
+      const { pane } = makeFakePane(1)
+
+      replayIntoTerminal(pane, ref, 'restored bytes', 1_000)
+      vi.advanceTimersByTime(1_000) // stall check → probe enqueued
+      expect(isPaneReplaying(ref, 1)).toBe(true)
+
+      // A wedged parser will never run the probe callback — and can never
+      // emit auto-replies either, so this bounded release cannot leak input.
+      vi.advanceTimersByTime(1_000)
+      expect(isPaneReplaying(ref, 1)).toBe(false)
+      expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith(
+        'terminal_replay_guard_wedged_release',
+        { paneId: 1 }
+      )
+    } finally {
+      errorSpy.mockRestore()
+    }
+  })
+
+  it('releases immediately when the probe write throws (terminal disposed mid-replay)', () => {
    vi.useFakeTimers()
    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
    try {
      const ref = makeRef()
      const { pane, terminal } = makeFakePane(1)

-      // First replay's completion is lost; second replay is healthy but slow.
-      replayIntoTerminal(pane, ref, 'lost completion', 1_000)
-      replayIntoTerminal(pane, ref, 'slow completion', 60_000)
-      terminal.pendingCallbacks.shift() // drop the first completion entirely
-      expect(isPaneReplaying(ref, 1)).toBe(true)
-
+      replayIntoTerminal(pane, ref, 'restored bytes', 1_000)
+      terminal.write = () => {
+        throw new Error('terminal disposed')
+      }
      vi.advanceTimersByTime(1_000)
-      // Watchdog released only the lost engagement; the healthy one still holds.
-      expect(isPaneReplaying(ref, 1)).toBe(true)
+      expect(isPaneReplaying(ref, 1)).toBe(false)
+      expect(mocks.recordRendererCrashBreadcrumb).toHaveBeenCalledWith(
+        'terminal_replay_guard_wedged_release',
+        { paneId: 1 }
+      )
+    } finally {
+      errorSpy.mockRestore()
+    }
+  })

-      terminal.flush()
+  it('keeps overlapping engagements independent through a lost completion', () => {
+    vi.useFakeTimers()
+    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+    try {
+      const ref = makeRef()
+      const { pane, terminal } = makeFakePane(1)
+
+      replayIntoTerminal(pane, ref, 'lost completion', 1_000)
+      replayIntoTerminal(pane, ref, 'healthy completion', 60_000)
+      terminal.pendingCallbacks.shift() // drop only the first completion
+      vi.advanceTimersByTime(1_000) // first engagement's probe enqueued
+
+      terminal.flush() // healthy completion + probe both parse
      expect(isPaneReplaying(ref, 1)).toBe(false)
      expect(ref.current.has(1)).toBe(false)

-      // A very late duplicate release must be a no-op.
      vi.advanceTimersByTime(120_000)
      expect(ref.current.has(1)).toBe(false)
    } finally {
@@ -249,7 +316,7 @@ describe('replay-guard watchdog', () => {
    }
  })

-  it('does not fire the watchdog after a normal completion', () => {
+  it('never probes after a normal completion', () => {
    vi.useFakeTimers()
    const ref = makeRef()
    const { pane, terminal } = makeFakePane(1)
@@ -259,10 +326,11 @@ describe('replay-guard watchdog', () => {
    expect(isPaneReplaying(ref, 1)).toBe(false)

    vi.advanceTimersByTime(60_000)
+    expect(terminal.lastData).toEqual(['healthy'])
    expect(mocks.recordRendererCrashBreadcrumb).not.toHaveBeenCalled()
  })

-  it('resolves replayIntoTerminalAsync via the watchdog so restore chains cannot hang', async () => {
+  it('resolves replayIntoTerminalAsync via the wedged path so restore chains cannot hang', async () => {
    vi.useFakeTimers()
    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
    try {
@@ -275,7 +343,7 @@ describe('replay-guard watchdog', () => {
        resolved = true
      })

-      await vi.advanceTimersByTimeAsync(1_000)
+      await vi.advanceTimersByTimeAsync(2_000)
      expect(resolved).toBe(true)
      expect(isPaneReplaying(ref, 1)).toBe(false)
    } finally {
@@ -28,41 +28,60 @@ import { recordRendererCrashBreadcrumb } from '@/lib/crash-diagnostics'

 export type ReplayingPanesRef = React.RefObject<Map<number, number>>

-// Why a watchdog: the decrement above only runs when xterm completes the
-// write. A wedged WriteBuffer (sync throw escaping a parse handler or a
-// write-completion callback — see xterm-write-buffer-stall.repro.test.ts) or
-// a disposed-terminal race can drop that completion forever, leaving the
-// guard latched on a live pane — which silently eats every keystroke
-// (Discord #performance / issue #2836). Real replays parse in well under a
-// second, so a 10s ceiling only ever fires on a genuinely lost completion.
-const REPLAY_GUARD_WATCHDOG_MS = 10_000
+// Why stall handling exists: the decrement above only runs when xterm
+// completes the write. A wedged WriteBuffer (sync throw escaping a parse
+// handler or a write-completion callback — see
+// xterm-write-buffer-stall.repro.test.ts) or a disposed-terminal race can
+// drop that completion forever, leaving the guard latched on a live pane —
+// which silently eats every keystroke (Discord #performance / issue #2836).
+//
+// Why release is probe-certified, never time-based: a blind timeout release
+// while a slow replay is still parsing would let xterm's auto-replies leak
+// into the shell — and into agent TUIs, where a leaked ESC reads as the user
+// pressing Escape. Instead, when a completion looks overdue we enqueue an
+// empty probe write. xterm parses writes in order, so only three states are
+// possible, and release is provably safe in every state that releases:
+//   1. probe completes, replay callback already ran   → normal release won.
+//   2. probe completes, replay callback never ran     → every replay byte has
+//      parsed (FIFO), so no further auto-replies can exist; the completion
+//      was genuinely lost. Release.
+//   3. probe never completes                          → the pipeline is
+//      wedged; a dead parser can never emit auto-replies, so releasing after
+//      a bounded wait cannot leak anything — and the pane needs recovery,
+//      which the breadcrumb reports.
+// While the probe is pending (slow-but-alive replay), the guard HOLDS.
+const REPLAY_GUARD_STALL_CHECK_MS = 10_000

 export function isPaneReplaying(ref: ReplayingPanesRef, paneId: number): boolean {
  return (ref.current.get(paneId) ?? 0) > 0
 }

+type ReplayGuardWriteTarget = Pick<ManagedPane['terminal'], 'write'>
+
 /**
 * Engage the replay counter for one write and return the release function.
 * Release runs exactly once — from xterm's write completion or, failing
- * that, from the watchdog — so a lost completion cannot latch the guard.
+ * that, from the probe-certified stall path — so a lost completion cannot
+ * latch the guard.
 */
 function engageReplayGuard(
  map: Map<number, number>,
  paneId: number,
-  watchdogMs: number,
+  terminal: ReplayGuardWriteTarget,
+  stallCheckMs: number,
  onRelease?: () => void
 ): () => void {
  map.set(paneId, (map.get(paneId) ?? 0) + 1)
  let released = false
-  let watchdog: ReturnType<typeof setTimeout> | null = null
-  const release = (reason: 'parsed' | 'watchdog'): void => {
+  let timer: ReturnType<typeof setTimeout> | null = null
+  const release = (reason: 'parsed' | 'lost-completion' | 'wedged'): void => {
    if (released) {
      return
    }
    released = true
-    if (watchdog !== null) {
-      clearTimeout(watchdog)
-      watchdog = null
+    if (timer !== null) {
+      clearTimeout(timer)
+      timer = null
    }
    const remaining = (map.get(paneId) ?? 1) - 1
    if (remaining <= 0) {
@@ -70,15 +89,36 @@ function engageReplayGuard(
    } else {
      map.set(paneId, remaining)
    }
-    if (reason === 'watchdog') {
+    if (reason === 'lost-completion') {
      console.error(
-        `[terminal] replay guard force-released for pane ${paneId} — xterm never completed the replay write (wedged write pipeline?)`
+        `[terminal] replay guard released for pane ${paneId} — the probe write parsed but the replay completion never arrived (lost write callback)`
      )
-      recordRendererCrashBreadcrumb('terminal_replay_guard_watchdog_release', { paneId })
+      recordRendererCrashBreadcrumb('terminal_replay_guard_lost_completion', { paneId })
+    } else if (reason === 'wedged') {
+      console.error(
+        `[terminal] replay guard released for pane ${paneId} — the probe write never parsed (wedged xterm write pipeline; pane likely needs recovery)`
+      )
+      recordRendererCrashBreadcrumb('terminal_replay_guard_wedged_release', { paneId })
    }
    onRelease?.()
  }
-  watchdog = setTimeout(() => release('watchdog'), watchdogMs)
+  const probeForStall = (): void => {
+    if (released) {
+      return
+    }
+    try {
+      // FIFO certification: this callback can only run after every replay
+      // byte queued before it has parsed (state 2 above).
+      terminal.write('', () => release('lost-completion'))
+    } catch {
+      // write threw (terminal disposed mid-replay): nothing will ever parse,
+      // so no auto-replies can leak.
+      release('wedged')
+      return
+    }
+    timer = setTimeout(() => release('wedged'), stallCheckMs)
+  }
+  timer = setTimeout(probeForStall, stallCheckMs)
  return () => release('parsed')
 }

@@ -90,12 +130,17 @@ export function replayIntoTerminal(
  pane: ManagedPane,
  replayingPanesRef: ReplayingPanesRef,
  data: string,
-  watchdogMs: number = REPLAY_GUARD_WATCHDOG_MS
+  stallCheckMs: number = REPLAY_GUARD_STALL_CHECK_MS
 ): void {
  if (!data) {
    return
  }
-  const releaseParsed = engageReplayGuard(replayingPanesRef.current, pane.id, watchdogMs)
+  const releaseParsed = engageReplayGuard(
+    replayingPanesRef.current,
+    pane.id,
+    pane.terminal,
+    stallCheckMs
+  )
  // Why: hidden/snapshot replay bypasses the live foreground write path, but
  // WebGL/canvas renderers still need a post-parse repaint to drop stale cells.
  writeForegroundTerminalChunk(pane.terminal, data, {
@@ -109,7 +154,7 @@ export function replayIntoTerminalAsync(
  pane: ManagedPane,
  replayingPanesRef: ReplayingPanesRef,
  data: string,
-  watchdogMs: number = REPLAY_GUARD_WATCHDOG_MS
+  stallCheckMs: number = REPLAY_GUARD_STALL_CHECK_MS
 ): Promise<void> {
  if (!data) {
    return Promise.resolve()
@@ -117,7 +162,13 @@ export function replayIntoTerminalAsync(
  return new Promise((resolve) => {
    // Why resolve on either release path: callers await this to sequence
    // restore steps; a lost write completion must not hang the restore chain.
-    const releaseParsed = engageReplayGuard(replayingPanesRef.current, pane.id, watchdogMs, resolve)
+    const releaseParsed = engageReplayGuard(
+      replayingPanesRef.current,
+      pane.id,
+      pane.terminal,
+      stallCheckMs,
+      resolve
+    )
    writeForegroundTerminalChunk(pane.terminal, data, {
      forceViewportRefresh: true,
      followupViewportRefresh: true,