mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 16:03:49 +00:00
fix(ssh): let the replay veto a stale alternate-screen belief
Adversarial review of the previous commit found a case where it is worse than the bug it fixes, and it is the exact inverse of what that commit reasoned about. The model reports alternateScreen from bytes it consumed, and it never consumes the outage. So if a full-screen app EXITS during the disconnect — an agent finishes, a command ends, the process dies — the model still says alternate. The gate then painted a frozen frame of an application that no longer exists and, via the else-if chain, discarded the replay carrying the shell's real output. Frozen and wrong beats fragments, which were at least current bytes. The replay is the only witness to the outage, so it now gets a veto: its last 47/1047/1049 transition, if any, outranks the model's belief. Leaving reset means the frame is gone and the tail wins; re-entering means the model is right after all. Same review found the width-mismatch guard drops the alt frame and leaves a cleared screen for the app to repaint — free for a park with no tail to lose, but here it meant discarding a usable one for a blank pane, so that degrades too. Both vetoes are skipped when there is no replay, where they would only trade a stale frame for an empty one. Extracted as sshReconnectPaintsFromModel rather than more inline ternary, because every interesting case is a disagreement between a stale belief and a replay — awkward to stage end-to-end, trivial to state as a table. 14 unit tests, including the two that fail against the previous commit. The e2e comment is corrected in the same spirit: top redraws itself, so it never discriminated the paint source and should not have claimed to. Also from the review: the kitty flag stack was left stale on this path, since the app's pushes during the outage exist only in the replay we discard — scanned now, after the snapshot so the outage layers on the pre-outage baseline. And the consume-once comment asserted an invariant that does not exist; followsDirectSshReconnect is a const captured per connect, bounded by connectStarted and the gates rather than by the read. Corrected rather than restructured. Known and NOT fixed, because it predates this work and is a behavior change of its own: the model probe is gated on the terminalSshViewParking kill switch, so turning off view parking also silently disables this repaint. Defaults on.
This commit is contained in:
@@ -271,7 +271,8 @@ import {
|
||||
decideSshReattachPaintSource,
|
||||
memoizeSshReattachModelSnapshotProbe,
|
||||
resolveSshReattachModelSnapshotWithTimeout,
|
||||
shouldFetchSshReattachModelSnapshot
|
||||
shouldFetchSshReattachModelSnapshot,
|
||||
sshReconnectPaintsFromModel
|
||||
} from './ssh-reattach-model-restore'
|
||||
import { readInFlightCommandCodeTurn } from './parked-terminal-command-status'
|
||||
import {
|
||||
@@ -8221,10 +8222,14 @@ export function connectPanePty(
|
||||
mountFollowsTerminalPark &&
|
||||
(connectResult?.isReattach === true || isRemoteRuntimePtyId(ptyId))
|
||||
// An SSH reconnect remounts the pane (tab.generation is its React key), so it also paints into
|
||||
// a fresh xterm — but unlike a park it may only use the model for a FULL-SCREEN app. See the
|
||||
// alternate-screen gate below for why. Consumed with the flag above: directSshRetryAttempt
|
||||
// stays true for the whole tab generation, not just this mount, so reading it directly would
|
||||
// re-arm on every later remount.
|
||||
// a fresh xterm — but unlike a park it may only use the model for a FULL-SCREEN app. See
|
||||
// sshReconnectPaintsFromModel for why.
|
||||
//
|
||||
// NOT consume-once, unlike mountFollowsTerminalPark: followsDirectSshReconnect is captured per
|
||||
// connectPanePty and never cleared, so this re-arms if one connect reaches handleReattachResult
|
||||
// twice. Bounded by connectStarted and by the emptiness/alt-screen gates rather than by the
|
||||
// read itself. It still reads the PENDING retry rather than directSshRetryAttempt, which also
|
||||
// accepts the live binding and so stays truthy for every later remount of the generation.
|
||||
const reconnectMayUseModel = followsDirectSshReconnect && !revealFollowsTerminalPark
|
||||
mountFollowsTerminalPark = false
|
||||
// Why: ordinary parking destroys xterm. Rebuild from the authoritative
|
||||
@@ -8327,14 +8332,11 @@ export function connectPanePty(
|
||||
// model still holds, but an in-place reattach (network reconnect, wake,
|
||||
// reload) already has that replay in hand, so probing would only delay its
|
||||
// paint by the timeout. Memoized, so this is never a second probe.
|
||||
// Why an SSH reconnect may only use the model on the ALTERNATE SCREEN: the reconnect replay
|
||||
// reaches the renderer without passing through main's model (forwardReattachReplay and the
|
||||
// inline attach replay both bypass onPtyData), so at this moment the model is stale by
|
||||
// exactly the outage. For a full-screen app that is the right trade — a byte tail cannot
|
||||
// rebuild a frame it no longer contains, while a grid can, and the SIGWINCH the restore
|
||||
// already sends makes the app repaint the delta itself. For a scrolling shell it is the
|
||||
// wrong trade: the tail holds output the model never saw, and preferring the grid would
|
||||
// drop it for good. A park has no such hole, so it keeps using the model either way.
|
||||
// An SSH reconnect may use the model only under sshReconnectPaintsFromModel: the reconnect
|
||||
// replay reaches the renderer without passing through main's model (forwardReattachReplay
|
||||
// and the inline attach replay both bypass onPtyData), so the model is stale by exactly
|
||||
// the outage and the replay is the only witness to it. A park has no such hole, so it
|
||||
// keeps using the model either way.
|
||||
const revealSnapshot = revealFollowsTerminalPark
|
||||
? (prefetchedParkModelSnapshot ??
|
||||
(isRemoteRuntimePtyId(ptyId) ? null : await fetchSshMainModelReattachSnapshot()))
|
||||
@@ -8343,8 +8345,16 @@ export function connectPanePty(
|
||||
!revealSnapshot && reconnectMayUseModel
|
||||
? await fetchSshMainModelReattachSnapshot()
|
||||
: null
|
||||
const paintsReconnectFromModel = sshReconnectPaintsFromModel({
|
||||
snapshot: reconnectSnapshot,
|
||||
replay: connectResult?.replay,
|
||||
altFrameWouldBeSkipped: shouldSkipAltFrameForWidthMismatch(
|
||||
reconnectSnapshot?.cols,
|
||||
readProposedTerminalCols(pane)
|
||||
)
|
||||
})
|
||||
const modelSnapshot =
|
||||
revealSnapshot ?? (reconnectSnapshot?.alternateScreen ? reconnectSnapshot : null)
|
||||
revealSnapshot ?? (paintsReconnectFromModel ? reconnectSnapshot : null)
|
||||
if (!isCurrentReattachPayload()) {
|
||||
return
|
||||
}
|
||||
@@ -8373,6 +8383,13 @@ export function connectPanePty(
|
||||
kittyKeyboardFlags: modelSnapshot.kittyKeyboardFlags,
|
||||
snapshotSeq: modelSnapshot.seq
|
||||
})
|
||||
if (paintsReconnectFromModel && connectResult?.replay) {
|
||||
// Why after the snapshot: painting the model discards the replay, but the app's kitty
|
||||
// pushes during the outage exist ONLY there. Snapshot first for the pre-outage
|
||||
// baseline, then the replay layers the outage on top — otherwise Option/Alt chords
|
||||
// encode against a stale flag stack.
|
||||
kittyKeyboardModes.scanReplay(connectResult.replay)
|
||||
}
|
||||
// Why shared: park+reveal of an alt-screen TUI needs the same
|
||||
// ?1049l/?1049h rebuild as applyMainBufferSnapshot (main strips
|
||||
// the ?1049h marker when splitting scrollbackAnsi) — inlined here
|
||||
|
||||
@@ -61,6 +61,59 @@ export function decideSshReattachPaintSource(args: {
|
||||
return contentLength === 0 ? 'relay-replay' : 'main-model-snapshot'
|
||||
}
|
||||
|
||||
/**
|
||||
* The last alternate-screen transition in a relay replay, or null if it holds none.
|
||||
*
|
||||
* A reconnect replay is the ONLY record of what happened while the client was gone — main's model
|
||||
* never saw those bytes — so it is the authority on which screen the app is on now, however stale
|
||||
* the rest of it is. 47 and 1047 count alongside 1049: older apps still use them, and any of the
|
||||
* three leaving reset means the frame is gone.
|
||||
*/
|
||||
export function lastAlternateScreenTransition(
|
||||
replay: string | undefined
|
||||
): 'entered' | 'exited' | null {
|
||||
if (!replay) {
|
||||
return null
|
||||
}
|
||||
let transition: 'entered' | 'exited' | null = null
|
||||
// A tail can begin mid-escape, so match forward rather than assuming a well-formed start.
|
||||
for (const match of replay.matchAll(/\x1b\[\?([0-9;]*)([hl])/g)) {
|
||||
if (match[1].split(';').some((mode) => mode === '47' || mode === '1047' || mode === '1049')) {
|
||||
transition = match[2] === 'h' ? 'entered' : 'exited'
|
||||
}
|
||||
}
|
||||
return transition
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether an SSH RECONNECT paints main's model instead of the relay tail.
|
||||
*
|
||||
* Only for a full-screen app, and only when the replay does not contradict the model. A tail cannot
|
||||
* rebuild a frame whose start it no longer contains, while a grid can; a scrolling shell is the
|
||||
* opposite, since the tail holds outage output the model never saw and the grid would drop it.
|
||||
*
|
||||
* The `exited` veto is the case this gate exists to refuse: if the app left the alternate screen
|
||||
* while the client was gone, the model still reports alternateScreen because it never consumed
|
||||
* those bytes. Painting it would freeze a frame of an application that no longer exists AND discard
|
||||
* the replay carrying the shell's real output — strictly worse than the fragments this fix targets.
|
||||
*/
|
||||
export function sshReconnectPaintsFromModel(args: {
|
||||
snapshot: { alternateScreen?: boolean } | null
|
||||
replay: string | undefined
|
||||
altFrameWouldBeSkipped: boolean
|
||||
}): boolean {
|
||||
if (!args.snapshot?.alternateScreen) {
|
||||
return false
|
||||
}
|
||||
if (!args.replay) {
|
||||
// Nothing to degrade to, so the vetoes below would only trade a stale frame for a blank one.
|
||||
return true
|
||||
}
|
||||
// The alt frame is dropped for a width mismatch, leaving a cleared screen the app must repaint.
|
||||
// A park could afford that with no tail to lose; here it would mean discarding a usable one.
|
||||
return !args.altFrameWouldBeSkipped && lastAlternateScreenTransition(args.replay) !== 'exited'
|
||||
}
|
||||
|
||||
/** Skip the snapshot fetch entirely when the paint could never use it. */
|
||||
export function shouldFetchSshReattachModelSnapshot(args: {
|
||||
ptyId: string
|
||||
|
||||
@@ -0,0 +1,115 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import {
|
||||
lastAlternateScreenTransition,
|
||||
sshReconnectPaintsFromModel
|
||||
} from './ssh-reattach-model-restore'
|
||||
|
||||
/**
|
||||
* The gate deciding whether an SSH RECONNECT repaints from main's grid or the relay byte tail.
|
||||
*
|
||||
* Worth testing directly rather than through a reattach: the inputs are a stale belief (the model's
|
||||
* alternate-screen flag, which predates the outage) and the only witness to the outage (the replay).
|
||||
* Every interesting case is a disagreement between those two, which is awkward to stage end-to-end
|
||||
* and trivial to state here.
|
||||
*/
|
||||
const ALT_ON = '\x1b[?1049h'
|
||||
const ALT_OFF = '\x1b[?1049l'
|
||||
|
||||
describe('lastAlternateScreenTransition', () => {
|
||||
it('reports nothing for a replay with no screen change', () => {
|
||||
expect(lastAlternateScreenTransition('plain output\r\n$ ')).toBeNull()
|
||||
expect(lastAlternateScreenTransition('')).toBeNull()
|
||||
expect(lastAlternateScreenTransition(undefined)).toBeNull()
|
||||
})
|
||||
|
||||
it('reports the LAST transition, not the first', () => {
|
||||
expect(lastAlternateScreenTransition(`${ALT_ON}frame${ALT_OFF}$ `)).toBe('exited')
|
||||
expect(lastAlternateScreenTransition(`${ALT_OFF}$ vim\r\n${ALT_ON}frame`)).toBe('entered')
|
||||
})
|
||||
|
||||
it('counts the legacy 47 and 1047 modes', () => {
|
||||
// Older apps still use these, and any of the three leaving means the frame is gone.
|
||||
expect(lastAlternateScreenTransition('\x1b[?47l')).toBe('exited')
|
||||
expect(lastAlternateScreenTransition('\x1b[?1047h')).toBe('entered')
|
||||
})
|
||||
|
||||
it('finds the mode inside a multi-parameter DECSET', () => {
|
||||
expect(lastAlternateScreenTransition('\x1b[?1049;1006;2004h')).toBe('entered')
|
||||
})
|
||||
|
||||
it('ignores unrelated private modes', () => {
|
||||
// Bracketed paste and cursor visibility must not be read as screen changes.
|
||||
expect(lastAlternateScreenTransition('\x1b[?2004h\x1b[?25l')).toBeNull()
|
||||
// 10491 merely CONTAINS 1049 as a substring.
|
||||
expect(lastAlternateScreenTransition('\x1b[?10491h')).toBeNull()
|
||||
})
|
||||
|
||||
it('still scans a tail that begins mid-escape', () => {
|
||||
// The relay tail is cut at a byte offset, so its first bytes are routinely a partial sequence.
|
||||
expect(lastAlternateScreenTransition(`49;1006h garbage ${ALT_OFF}`)).toBe('exited')
|
||||
})
|
||||
})
|
||||
|
||||
describe('sshReconnectPaintsFromModel', () => {
|
||||
const args = (overrides: Partial<Parameters<typeof sshReconnectPaintsFromModel>[0]> = {}) => ({
|
||||
snapshot: { alternateScreen: true },
|
||||
replay: 'some output',
|
||||
altFrameWouldBeSkipped: false,
|
||||
...overrides
|
||||
})
|
||||
|
||||
it('paints a full-screen app from the grid', () => {
|
||||
// The reported bug: a tail cannot rebuild a frame whose start it no longer contains.
|
||||
expect(sshReconnectPaintsFromModel(args())).toBe(true)
|
||||
})
|
||||
|
||||
it('leaves a scrolling shell on the tail', () => {
|
||||
// Inverted trade: the tail holds outage output the model never saw, and the grid would drop it.
|
||||
expect(sshReconnectPaintsFromModel(args({ snapshot: { alternateScreen: false } }))).toBe(false)
|
||||
expect(sshReconnectPaintsFromModel(args({ snapshot: {} }))).toBe(false)
|
||||
expect(sshReconnectPaintsFromModel(args({ snapshot: null }))).toBe(false)
|
||||
})
|
||||
|
||||
it('refuses the grid when the app LEFT the alternate screen during the outage', () => {
|
||||
// The model never consumed the outage, so it still claims alternateScreen. Painting it would
|
||||
// freeze a frame of an application that no longer exists and discard the shell's real output.
|
||||
expect(
|
||||
sshReconnectPaintsFromModel(args({ replay: `${ALT_OFF}\r\n$ echo done\r\ndone\r\n$ ` }))
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('keeps the grid when the app re-entered the alternate screen after leaving', () => {
|
||||
expect(sshReconnectPaintsFromModel(args({ replay: `${ALT_OFF}$ vim\r\n${ALT_ON}frame` }))).toBe(
|
||||
true
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps the grid when the replay says nothing about the screen', () => {
|
||||
// The common case: an idle TUI whose tail carries only redraws.
|
||||
expect(sshReconnectPaintsFromModel(args({ replay: 'redraw redraw' }))).toBe(true)
|
||||
})
|
||||
|
||||
it('degrades to the tail when the alt frame would be dropped for a width mismatch', () => {
|
||||
// That guard leaves only a cleared screen for the app to repaint. A park can afford it with no
|
||||
// tail to lose; here it would mean having discarded a usable one for a blank pane.
|
||||
expect(sshReconnectPaintsFromModel(args({ altFrameWouldBeSkipped: true }))).toBe(false)
|
||||
})
|
||||
|
||||
it('still paints the grid when there is no tail to degrade to', () => {
|
||||
// Both vetoes exist to protect a usable replay. With none, they would only trade a stale frame
|
||||
// for a blank one.
|
||||
expect(
|
||||
sshReconnectPaintsFromModel(args({ replay: undefined, altFrameWouldBeSkipped: true }))
|
||||
).toBe(true)
|
||||
expect(sshReconnectPaintsFromModel(args({ replay: '', altFrameWouldBeSkipped: true }))).toBe(
|
||||
true
|
||||
)
|
||||
})
|
||||
|
||||
it('refuses a normal-buffer model even with no tail', () => {
|
||||
expect(
|
||||
sshReconnectPaintsFromModel(args({ snapshot: { alternateScreen: false }, replay: '' }))
|
||||
).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -113,10 +113,16 @@ test.describe('SSH reconnect pane restore', () => {
|
||||
const freshContent = await getTerminalContent(orcaPage, 8000)
|
||||
expect(freshContent).not.toContain(marker)
|
||||
|
||||
// A FULL-SCREEN app is the third case, and the one a byte tail cannot serve: a tail can begin
|
||||
// mid-escape and misses the alt-screen enter and absolute positioning that built the frame, so
|
||||
// replaying it paints fragments. `top` is used because it is present on any Linux image and
|
||||
// repaints on a fixed interval, so a coherent frame after the reconnect is unambiguous.
|
||||
// A FULL-SCREEN app is the third case: a reconnect must leave a TUI pane alive and drawing,
|
||||
// not blank or frozen.
|
||||
//
|
||||
// SCOPE, because it is easy to over-read: this does NOT prove which payload painted the pane.
|
||||
// top redraws itself every few seconds, so these assertions pass whichever way the paint-source
|
||||
// gate decided — including with it reverted. What discriminates model-vs-tail is unit-level, in
|
||||
// ssh-reconnect-model-paint-gate.test.ts, because the interesting cases are disagreements
|
||||
// between main's pre-outage alt-screen belief and a replay produced during the outage, which
|
||||
// is not something this fixture can stage. Kept anyway: it is the only coverage that a
|
||||
// reconnected TUI pane recovers at all.
|
||||
await execInTerminal(orcaPage, freshPtyId, 'top -b -n 1 > /dev/null; top')
|
||||
await waitForTerminalOutput(orcaPage, 'load average', 30_000, 8000)
|
||||
|
||||
@@ -124,8 +130,6 @@ test.describe('SSH reconnect pane restore', () => {
|
||||
await waitForActiveTerminalManager(orcaPage, 60_000)
|
||||
await waitForActivePanePtyId(orcaPage, 60_000)
|
||||
|
||||
// The header only survives as a whole if a grid was repainted; a tail that lost the frame
|
||||
// start shows rows without it.
|
||||
await waitForTerminalOutput(orcaPage, 'load average', 60_000, 8000)
|
||||
const tuiContent = await getTerminalContent(orcaPage, 8000)
|
||||
expect(tuiContent).toContain('PID')
|
||||
|
||||
Reference in New Issue
Block a user