starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-10-24 00:04:01 +00:00
Code Issues Packages Projects Releases Wiki Activity

revert: drop the snapshot-first reconnect paint; unproven and it regressed

Browse Source 
Reverts e6541fe9b8, its follow-up c497a26788, and the rename f680a0b1cb.

The reasoning behind it still looks right — a byte tail cannot rebuild an
alt-screen application, a grid snapshot can, and that is what tmux repaints on
attach. What I could never do is show it fixing the reported symptom. Two
attempts to reproduce the corruption on a build WITHOUT it both restored
correctly: freshly started Claude Code and Codex, and an alt-screen `less`
scrolled 4000 lines so its full paint had aged out of the relay's 100KB tail.

Meanwhile it cost two real regressions. It fired on mounts that were not
reconnects, leaving a new tab with no prompt and a placeholder title, which a
user hit within minutes. The fix for that consumed the eligibility flag with the
one it was derived from — and after it, a reconnected Claude Code came back as
fragments of a frame, the exact symptom the change was meant to remove. So the
consume-once semantics that stop stale paints and the repaint a reconnect needs
are in direct tension, and I do not yet understand the ordering well enough to
satisfy both.

Shipping an unproven change that has already broken two things twice is worse
than shipping the blank-pane fix alone, which IS reproduced, A/B'd and visually
verified. The TUI corruption goes back to open — but now with something it never
had before: a reproduction. It shows up on a reconnect against a Claude Code
that has been running a while, not one just started, which is why my earlier
checks kept passing.

The e2e coverage stays. It asserts what the relay fix guarantees — a marker
surviving a reconnect, and a tab opened afterwards reaching a shell that answers
— and neither of those depends on this change.
This commit is contained in:
Neil
2026-08-15 00:32:49 -07:00
parent f680a0b1cb
commit bf88c867ec
3 changed files with 21 additions and 169 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/renderer/src/components/terminal-pane/pty-connection.ts
+12 -39
View File
@@ -7968,18 +7968,7 @@ export function connectPanePty(
return true
}
// A reconnect remounts the pane (tab.generation is its React key), so like a park it always
// paints into a terminal that holds nothing. directSshRetryAttempt is exactly that mount:
// it is only set when a pending or live SSH pane retry matches this connection AND this tab
// generation.
const isSshReconnectRemount = Boolean(directSshRetryAttempt)
// MUTABLE, and consumed with mountFollowsTerminalPark below. A const here is a bug: the payload
// path clears mountFollowsTerminalPark after the first reattach so a later in-place reconnect on
// the SAME mount cannot repaint, and a snapshot written then lands on a terminal that already
// has live content. Reading a frozen copy reintroduced exactly that.
let paintsIntoEmptyTerminal = mountFollowsTerminalPark || isSshReconnectRemount
let sshModelSnapshotPrefetch: {
let parkedSshSnapshotPrefetch: {
ptyId: string
fetch: () => Promise<PtyBufferSnapshot | null>
} | null = null
@@ -7989,13 +7978,7 @@ export function connectPanePty(
): (() => Promise<PtyBufferSnapshot | null>) =>
memoizeSshReattachModelSnapshotProbe(async (): Promise<PtyBufferSnapshot | null> => {
const sshParkingEnabled = useAppStore.getState().settings?.terminalSshViewParking !== false
if (
!shouldFetchSshReattachModelSnapshot({
ptyId,
sshParkingEnabled,
isSshReconnectRemount
})
) {
if (!shouldFetchSshReattachModelSnapshot({ ptyId, sshParkingEnabled })) {
return null
}
const snapshot = await resolveSshReattachModelSnapshotWithTimeout(
@@ -8004,12 +7987,8 @@ export function connectPanePty(
})
)
return snapshot &&
decideSshReattachPaintSource({
ptyId,
sshParkingEnabled,
isSshReconnectRemount,
snapshot
}) === 'main-model-snapshot'
decideSshReattachPaintSource({ ptyId, sshParkingEnabled, snapshot }) ===
'main-model-snapshot'
? snapshot
: null
})
@@ -8017,20 +7996,17 @@ export function connectPanePty(
const getSshMainModelSnapshotProbe = (
ptyId: string
): (() => Promise<PtyBufferSnapshot | null>) => {
if (sshModelSnapshotPrefetch?.ptyId !== ptyId) {
sshModelSnapshotPrefetch = { ptyId, fetch: createSshMainModelSnapshotProbe(ptyId) }
if (parkedSshSnapshotPrefetch?.ptyId !== ptyId) {
parkedSshSnapshotPrefetch = { ptyId, fetch: createSshMainModelSnapshotProbe(ptyId) }
}
return sshModelSnapshotPrefetch.fetch
return parkedSshSnapshotPrefetch.fetch
}
// Renamed from prepaintParkedSshSnapshot: parking is no longer the only mount that needs it.
// A reconnect remount arrives with an empty terminal for the same reason a park does, and only
// a grid snapshot restores a full-screen application — see decideSshReattachPaintSource.
const prepaintSshModelSnapshot = (ptyId: string | null): void => {
const prepaintParkedSshSnapshot = (ptyId: string | null): void => {
const parsedPtyId = ptyId ? parseAppSshPtyId(ptyId) : null
if (
!ptyId ||
!paintsIntoEmptyTerminal ||
!mountFollowsTerminalPark ||
parsedPtyId?.connectionId !== connectionId ||
!capturedDirectSshRetryLeaseMatches()
) {
@@ -8039,7 +8015,7 @@ export function connectPanePty(
const capturedGeneration = authoritativeReattachGeneration
const isCurrent = (): boolean =>
!disposed &&
paintsIntoEmptyTerminal &&
mountFollowsTerminalPark &&
authoritativeReattachGeneration === capturedGeneration &&
capturedDirectSshRetryLeaseMatches()
const fetchSnapshot = getSshMainModelSnapshotProbe(ptyId)
@@ -8234,9 +8210,6 @@ export function connectPanePty(
mountFollowsTerminalPark &&
(connectResult?.isReattach === true || isRemoteRuntimePtyId(ptyId))
mountFollowsTerminalPark = false
// Consumed together: the prepaint's own guard reads this, and both reasons a mount paints into
// an empty terminal are spent by the first reattach.
paintsIntoEmptyTerminal = false
// Why: ordinary parking destroys xterm. Rebuild from the authoritative
// host snapshot before releasing queued live bytes; null falls back to
// the subscribe screen without keeping the old xterm mounted.
@@ -8611,7 +8584,7 @@ export function connectPanePty(
const legacyWorkerOwnsPane = isLegacyWorkerAutomaticResumeBlocked()
if (gate.enterDeferredFlow && (!legacyWorkerOwnsPane || !gate.sshConnected)) {
// Paint main's parked model while SSH recovery continues off the render path.
prepaintSshModelSnapshot(pendingSessionId)
prepaintParkedSshSnapshot(pendingSessionId)
void (async () => {
// Why: for a passphrase target with no cached credential, don't auto-fire ssh.connect — a prompt popping just from focusing a tab / Cmd+J would surprise the user.
// Wait for a user-initiated connect first; no-passphrase targets return false here and auto-connect as before.
@@ -8974,7 +8947,7 @@ export function connectPanePty(
if (deferredReattachSessionId) {
allowInitialIdleCacheSeed = true
recordPtyConnectDiagnostic(`pane=${pane.id} -> REATTACH ${deferredReattachSessionId}`)
prepaintSshModelSnapshot(deferredReattachSessionId)
prepaintParkedSshSnapshot(deferredReattachSessionId)
// Why: pre-signal (declare) before the reattach connect so the cooperation gate suppresses the daemon seed for this paneKey; Electron preserves IPC order.
// See docs/mobile-prefer-renderer-scrollback.md (Renderer-side prerequisite requirement #4).
src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.test.ts
-96
View File
@@ -48,40 +48,6 @@ describe('shouldFetchSshReattachModelSnapshot', () => {
shouldFetchSshReattachModelSnapshot({ ptyId: LOCAL_PTY_ID, sshParkingEnabled: true })
).toBe(false)
})
// A reconnect remount paints into an empty terminal exactly as a park does, and main keeps its
// model regardless of the parking setting — so turning view parking off must not strand a
// reconnect on the relay byte tail, which cannot rebuild a full-screen app.
it('fetches for a reconnect remount even with SSH parking disabled', () => {
expect(
shouldFetchSshReattachModelSnapshot({
ptyId: SSH_PTY_ID,
sshParkingEnabled: false,
isSshReconnectRemount: true
})
).toBe(true)
})
// The pty gate is independent of the reason: a local pty has no relay and no SSH model.
it('still refuses a non-SSH pty on a reconnect remount', () => {
expect(
shouldFetchSshReattachModelSnapshot({
ptyId: LOCAL_PTY_ID,
sshParkingEnabled: false,
isSshReconnectRemount: true
})
).toBe(false)
})
it('is unchanged for an ordinary mount that is neither parked nor reconnecting', () => {
expect(
shouldFetchSshReattachModelSnapshot({
ptyId: SSH_PTY_ID,
sshParkingEnabled: false,
isSshReconnectRemount: false
})
).toBe(false)
})
})
describe('memoizeSshReattachModelSnapshotProbe', () => {
@@ -188,66 +154,4 @@ describe('decideSshReattachPaintSource', () => {
})
).toBe('relay-replay')
})
describe('a reconnect remount', () => {
// The defect this closes: a reconnect disposes the pane's xterm (tab.generation is its React
// key), so the pane repaints from the relay byte tail — which cannot rebuild an alt-screen app.
// Claude Code came back as fragments of a frame until the user resized it.
it('paints from the main model even with SSH parking disabled', () => {
expect(
decideSshReattachPaintSource({
ptyId: SSH_PTY_ID,
sshParkingEnabled: false,
isSshReconnectRemount: true,
snapshot: headless
})
).toBe('main-model-snapshot')
})
// Every safety gate below the eligibility check still applies — widening WHY the model is
// trusted must not widen WHAT is trusted, or a reconnect paints a blank pane.
it.each([
['a null snapshot', null],
['a renderer-sourced snapshot', { data: 'screen', source: 'renderer' as const }],
['a sourceless snapshot', { data: 'screen' }],
['an empty headless snapshot', { data: '', source: 'headless' as const }],
[
'a headless snapshot holding only a dangling escape',
{ data: '', source: 'headless' as const, pendingEscapeTailAnsi: '\u001b[' }
]
])('still degrades to relay replay for %s', (_label, snapshot) => {
expect(
decideSshReattachPaintSource({
ptyId: SSH_PTY_ID,
sshParkingEnabled: false,
isSshReconnectRemount: true,
snapshot
})
).toBe('relay-replay')
})
// An alt-screen app can carry all its content in scrollback with an empty screen frame; that is
// the shape a TUI reconnect actually produces, so it must not read as empty.
it('accepts a snapshot whose content is entirely scrollback', () => {
expect(
decideSshReattachPaintSource({
ptyId: SSH_PTY_ID,
sshParkingEnabled: false,
isSshReconnectRemount: true,
snapshot: { data: '', source: 'headless', scrollbackAnsi: 'history' }
})
).toBe('main-model-snapshot')
})
it('still refuses a non-SSH pty', () => {
expect(
decideSshReattachPaintSource({
ptyId: LOCAL_PTY_ID,
sshParkingEnabled: false,
isSshReconnectRemount: true,
snapshot: headless
})
).toBe('relay-replay')
})
})
})
src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts
+9 -34
View File
@@ -33,41 +33,20 @@ export async function resolveSshReattachModelSnapshotWithTimeout<T>(
}
/**
* Which payload paints an SSH reattach. Only main's headless model is trusted: a
* 'renderer'-sourced snapshot serializes a mounted xterm, which no longer exists
* once the pane parked — anything but a non-empty headless snapshot degrades to
* the relay replay, never a blank paint. Emptiness is judged on the composed
* CONTENT (scrollback + screen): an alt-screen snapshot can carry all content in
* scrollbackAnsi with an empty screen frame.
*
* Two situations trust the model, for the same underlying reason — the pane is
* painting into a terminal that holds nothing:
*
* - PARKING (the original C1 gate), where the pane was unmounted on purpose.
* - AN SSH RECONNECT, where a reconnect bumps tab.generation, which is the
* pane's React key, so TerminalPane remounts and the old xterm is disposed
* with its buffer.
*
* The reconnect case is not optional polish. Relay replay is a byte TAIL, and a
* tail cannot reconstruct a full-screen application: it can begin mid-escape and
* it misses the alt-screen enter, the clears and the absolute positioning that
* built the frame. Replaying one paints fragments of a frame — which is what a
* reconnected Claude Code showed until the user resized it. The model snapshot
* is a serialized GRID, which is what tmux repaints on attach and the only
* payload that restores a TUI correctly. Main keeps that model regardless of the
* parking setting, so the reconnect case does not inherit its gate.
* Which payload paints an SSH reattach (C1 SSH-parking design gate). Only
* main's headless model is trusted: a 'renderer'-sourced snapshot serializes a
* mounted xterm, which no longer exists once the pane parked — anything but a
* non-empty headless snapshot degrades to the relay replay, never a blank paint.
* Emptiness is judged on the composed CONTENT (scrollback + screen): an
* alt-screen snapshot can carry all content in scrollbackAnsi with an empty
* screen frame.
*/
export function decideSshReattachPaintSource(args: {
ptyId: string
sshParkingEnabled: boolean
/** This mount replaces a terminal disposed by an SSH reconnect remount. */
isSshReconnectRemount?: boolean
snapshot: SshReattachModelSnapshot | null
}): SshReattachPaintSource {
if (
(!args.sshParkingEnabled && !args.isSshReconnectRemount) ||
parseAppSshPtyId(args.ptyId) === null
) {
if (!args.sshParkingEnabled || parseAppSshPtyId(args.ptyId) === null) {
return 'relay-replay'
}
if (!args.snapshot || args.snapshot.source !== 'headless') {
@@ -86,12 +65,8 @@ export function decideSshReattachPaintSource(args: {
export function shouldFetchSshReattachModelSnapshot(args: {
ptyId: string
sshParkingEnabled: boolean
isSshReconnectRemount?: boolean
}): boolean {
return (
(args.sshParkingEnabled || args.isSshReconnectRemount === true) &&
parseAppSshPtyId(args.ptyId) !== null
)
return args.sshParkingEnabled && parseAppSshPtyId(args.ptyId) !== null
}
/**