From bf88c867eca7bbfc77c01c6da8a88471c48f6247 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:32:49 -0700 Subject: [PATCH] revert: drop the snapshot-first reconnect paint; unproven and it regressed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts e6541fe9b87, its follow-up c497a26788d, and the rename f680a0b1cb9. 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. --- .../terminal-pane/pty-connection.ts | 51 +++------- .../ssh-reattach-model-restore.test.ts | 96 ------------------- .../ssh-reattach-model-restore.ts | 43 ++------- 3 files changed, 21 insertions(+), 169 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 961a03c4d90..14a514cfee5 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -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 } | null = null @@ -7989,13 +7978,7 @@ export function connectPanePty( ): (() => Promise) => memoizeSshReattachModelSnapshotProbe(async (): Promise => { 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) => { - 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). diff --git a/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.test.ts b/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.test.ts index a4b824630c3..faedd69c971 100644 --- a/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.test.ts +++ b/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.test.ts @@ -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') - }) - }) }) diff --git a/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts b/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts index 1bdf4681212..c641332758c 100644 --- a/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts +++ b/src/renderer/src/components/terminal-pane/ssh-reattach-model-restore.ts @@ -33,41 +33,20 @@ export async function resolveSshReattachModelSnapshotWithTimeout( } /** - * 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 } /**