From 22b00a7cd280969af0820d266fb0f1c061240c3b Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 29 Jun 2026 13:35:43 -0700 Subject: [PATCH] fix(terminal): report PTY's applied size so dropped resizes self-heal (split-mount desync) (#6785) Co-authored-by: Orca --- src/main/daemon/daemon-checkpoint-file.ts | 22 ++++ src/main/daemon/daemon-pty-adapter.test.ts | 40 ++++++ src/main/daemon/daemon-pty-adapter.ts | 18 +++ src/main/daemon/daemon-pty-router.ts | 4 + src/main/daemon/daemon-server.ts | 3 + src/main/daemon/headless-emulator.ts | 8 ++ src/main/daemon/session.ts | 11 ++ src/main/daemon/terminal-host.ts | 11 ++ src/main/daemon/types.ts | 35 +++-- src/main/ipc/pty.test.ts | 101 ++++++++++++++ src/main/ipc/pty.ts | 28 +++- src/main/providers/local-pty-provider.ts | 13 ++ src/main/providers/types.ts | 13 ++ .../terminal-pane/pty-connection.ts | 7 + .../terminal-pane/pty-size-reconcile.test.ts | 123 ++++++++++++++++++ .../terminal-pane/pty-size-reconcile.ts | 61 ++++++++- .../e2e/terminal-column-desync-repro.spec.ts | 36 +++++ 17 files changed, 507 insertions(+), 27 deletions(-) create mode 100644 src/main/daemon/daemon-checkpoint-file.ts diff --git a/src/main/daemon/daemon-checkpoint-file.ts b/src/main/daemon/daemon-checkpoint-file.ts new file mode 100644 index 000000000..d7e4bd7a6 --- /dev/null +++ b/src/main/daemon/daemon-checkpoint-file.ts @@ -0,0 +1,22 @@ +import type { TerminalOscLinkRange } from '../../shared/terminal-osc-link-ranges' +import type { TerminalModes } from './types' + +/** On-disk shape of checkpoint.json. Written by history-manager, read by + * history-reader — one type so the generation pairing with output.log's + * header (see terminal-history-log.ts) cannot silently diverge between the + * writer and the consumer. */ +export type TerminalCheckpointFile = { + snapshotAnsi: string + scrollbackAnsi: string + oscLinks?: TerminalOscLinkRange[] + rehydrateSequences: string + cwd: string | null + cols: number + rows: number + modes: TerminalModes + scrollbackLines: number + /** Ties this checkpoint to the output.log whose header carries the same + * generation. Absent on checkpoints written before incremental logs. */ + generation?: number + checkpointedAt: string +} diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index c140a42cc..775342351 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -178,6 +178,46 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { }) }) + describe('getAppliedSize', () => { + it('reports the spawn dims before any resize', async () => { + const { id } = await adapter.spawn({ cols: 80, rows: 24 }) + expect(await adapter.getAppliedSize(id)).toEqual({ cols: 80, rows: 24 }) + }) + + it('reflects the size the daemon actually applied after a resize', async () => { + const { id } = await adapter.spawn({ cols: 80, rows: 24 }) + adapter.resize(id, 120, 40) + await waitFor(() => vi.mocked(lastSubprocess.resize).mock.calls.length > 0) + expect(await adapter.getAppliedSize(id)).toEqual({ cols: 120, rows: 40 }) + }) + + // Why: this is the regression the fix targets. resize() is a fire-and-forget + // notify; a resize that arrives after the session exited is silently dropped + // daemon-side. getAppliedSize must keep reporting the last size the PTY + // genuinely took (the stale pre-exit dims) rather than the dropped request — + // so the renderer can tell its requested size never landed. The old + // requested-size cache would have masked the drop. + it('does not advance when a resize is dropped after the session exited', async () => { + const { id } = await adapter.spawn({ cols: 200, rows: 50 }) + + // Simulate the child exiting, then a late narrow resize racing in. The + // daemon Session.resize early-returns for an exited session, so the child + // is never resized and the applied size never becomes the requested 80×24. + lastSubprocess._simulateExit(0) + await new Promise((r) => setTimeout(r, 50)) + + adapter.resize(id, 80, 24) + await new Promise((r) => setTimeout(r, 50)) + + // The drop must be visible: the subprocess was never resized to the narrow + // dims the renderer requested, and getAppliedSize never reports 80 cols — + // it stays wide (or null once reaped), never masking the drop as "applied". + expect(lastSubprocess.resize).not.toHaveBeenCalledWith(80, 24) + const applied = await adapter.getAppliedSize(id) + expect(applied?.cols).not.toBe(80) + }) + }) + describe('shutdown', () => { it('kills the session', async () => { const { id } = await adapter.spawn({ cols: 80, rows: 24 }) diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index b2daef2a7..29825792d 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -362,6 +362,24 @@ export class DaemonPtyAdapter implements IPtyProvider { return this.initialCwds.get(id) ?? '' } + // Why: resize() is a fire-and-forget notify, so a resize can be dropped + // daemon-side (session not yet alive, exited, invalid dims, cold-restore + // snapshot-col coercion) without the renderer knowing. This reads the size + // the daemon actually applied so the renderer can detect that drift on resume + // and re-assert. Null (RPC failure / unknown session) means "cannot confirm", + // which the renderer treats as a cue to re-forward once. + async getAppliedSize(id: string): Promise<{ cols: number; rows: number } | null> { + try { + const result = await this.client.request<{ size: { cols: number; rows: number } | null }>( + 'getSize', + { sessionId: id } + ) + return result.size ?? null + } catch { + return null + } + } + async clearBuffer(id: string): Promise { await this.client.request('clearScrollback', { sessionId: id }) this.markSessionDirty(id) diff --git a/src/main/daemon/daemon-pty-router.ts b/src/main/daemon/daemon-pty-router.ts index 14fe2d3dd..2212635d1 100644 --- a/src/main/daemon/daemon-pty-router.ts +++ b/src/main/daemon/daemon-pty-router.ts @@ -96,6 +96,10 @@ export class DaemonPtyRouter implements IPtyProvider { return this.adapterFor(id).getInitialCwd(id) } + async getAppliedSize(id: string): Promise<{ cols: number; rows: number } | null> { + return (await this.adapterFor(id).getAppliedSize?.(id)) ?? null + } + async clearBuffer(id: string): Promise { await this.adapterFor(id).clearBuffer(id) } diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index 1f37ce73a..6872b5154 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -377,6 +377,9 @@ export class DaemonServer { case 'getSnapshot': return { snapshot: this.host.getSnapshot(request.payload.sessionId) } + case 'getSize': + return { size: this.host.getAppliedSize(request.payload.sessionId) } + case 'takePendingOutput': // Why no await before this call: with includeSnapshot, drain and // serialize must share one synchronous turn — an intervening await diff --git a/src/main/daemon/headless-emulator.ts b/src/main/daemon/headless-emulator.ts index 5376df92f..e2e396170 100644 --- a/src/main/daemon/headless-emulator.ts +++ b/src/main/daemon/headless-emulator.ts @@ -126,6 +126,14 @@ export class HeadlessEmulator { this.terminal.resize(cols, rows) } + // Why: Session.resize applies this emulator and the node-pty subprocess + // together behind the same dead/invalid-size gate, so the emulator's dims are + // an accurate proxy for the size the child actually took — and stay stale + // when a resize is dropped, which is exactly the drop the renderer must detect. + getAppliedSize(): { cols: number; rows: number } { + return { cols: this.terminal.cols, rows: this.terminal.rows } + } + getSnapshot(opts: { scrollbackRows?: number } = {}): TerminalSnapshot { const modes = this.getModes() const snapshotAnsi = this.normalizeSnapshotAnsiForModes( diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index dd6f22719..551b8ad27 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -226,6 +226,17 @@ export class Session { return this.emulator.getSnapshot() } + // Why: the size the PTY actually applied (emulator dims, which Session.resize + // advances atomically with the subprocess), so the renderer can detect a + // resize that was dropped here (exited/disposed/invalid) instead of trusting + // its own last-requested size. Null on a disposed session. + getAppliedSize(): { cols: number; rows: number } | null { + if (this._disposed) { + return null + } + return this.emulator.getAppliedSize() + } + /** Drains the records accumulated since the last take. Runs synchronously — * when includeSnapshot is set, the serialize happens in the same turn so no * PTY data can land between the drain and the snapshot (which would later diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index dc4d3c7c6..6571d14c3 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -260,6 +260,17 @@ export class TerminalHost { return session.getSnapshot() } + // Why: read-only readback of the size the PTY actually applied (null-not-throw + // like getSnapshot). The renderer compares this against xterm to detect a + // resize that was dropped/coerced daemon-side and re-assert it. + getAppliedSize(sessionId: string): { cols: number; rows: number } | null { + const session = this.sessions.get(sessionId) + if (!session || !session.isAlive) { + return null + } + return session.getAppliedSize() + } + // Why: same null-not-throw semantics as getSnapshot — incremental // checkpoints are best-effort against sessions that may have just exited. takePendingOutput( diff --git a/src/main/daemon/types.ts b/src/main/daemon/types.ts index 10964feff..868afa816 100644 --- a/src/main/daemon/types.ts +++ b/src/main/daemon/types.ts @@ -43,25 +43,10 @@ export type TerminalModes = { alternateScreen: boolean } -/** On-disk shape of checkpoint.json. Written by history-manager, read by - * history-reader — one type so the generation pairing with output.log's - * header (see terminal-history-log.ts) cannot silently diverge between the - * writer and the consumer. */ -export type TerminalCheckpointFile = { - snapshotAnsi: string - scrollbackAnsi: string - oscLinks?: TerminalOscLinkRange[] - rehydrateSequences: string - cwd: string | null - cols: number - rows: number - modes: TerminalModes - scrollbackLines: number - /** Ties this checkpoint to the output.log whose header carries the same - * generation. Absent on checkpoints written before incremental logs. */ - generation?: number - checkpointedAt: string -} +// The on-disk checkpoint.json shape lives in daemon-checkpoint-file.ts (it +// depends only on TerminalModes here) — re-exported so existing importers of +// `./types` keep working. +export type { TerminalCheckpointFile } from './daemon-checkpoint-file' // ─── NDJSON Protocol Messages ─────────────────────────────────────── @@ -225,6 +210,17 @@ export type GetSnapshotRequest = { } } +// Why: read-only readback of the size the PTY actually applied (vs the size the +// renderer last requested via the fire-and-forget resize notify). Lets the +// renderer's resume drift-check re-assert a resize the daemon dropped/coerced. +export type GetSizeRequest = { + id: string + type: 'getSize' + payload: { + sessionId: string + } +} + // ─── Incremental checkpoint records (v13+) ────────────────────────── // Why: the 5s checkpoint used to re-serialize the full emulator buffer per // tick, stalling the daemon's PTY pump for O(buffer). Incremental checkpoints @@ -283,6 +279,7 @@ export type DaemonRequest = | SystemResolverHealthRequest | PtySpawnHealthRequest | GetSnapshotRequest + | GetSizeRequest | TakePendingOutputRequest // ─── RPC Responses (Daemon → Client, on control socket) ──────────── diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index b5dffb1a2..dfbd9c188 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -3391,6 +3391,107 @@ describe('registerPtyHandlers', () => { expect(provider.getForegroundProcess).not.toHaveBeenCalled() }) + // Why: regression for the Claude-Code split-pane garbled-render desync. resize + // is fire-and-forget for daemon-backed PTYs, so a corrective narrow resize can + // be dropped while the renderer believes it landed. pty:getSize must report the + // size the PTY ACTUALLY applied (so the renderer's resume drift-check re-asserts + // the dropped resize) rather than the size last requested. This models a daemon + // provider whose resize is dropped but whose getAppliedSize stays at the wide + // spawn size; pty:getSize must surface the wide (applied) size, not the narrow + // (requested) one. + describe('pty:getSize reports applied size, not requested size', () => { + function setupProviderWithAppliedSize(args: { + applied: { cols: number; rows: number } | null + resize?: (cols: number, rows: number) => void + getAppliedSize?: (id: string) => Promise<{ cols: number; rows: number } | null> + }): void { + setLocalPtyProvider({ + spawn: vi.fn(async (opts: { sessionId?: string }) => ({ + id: opts.sessionId ?? 'daemon-pty' + })), + write: vi.fn(), + resize: vi.fn(args.resize ?? (() => {})), + getAppliedSize: vi.fn(args.getAppliedSize ?? (async () => args.applied)), + kill: vi.fn(), + shutdown: vi.fn(), + onData: vi.fn(() => vi.fn()), + onExit: vi.fn(() => vi.fn()), + listProcesses: vi.fn(async () => []), + getForegroundProcess: vi.fn(async () => null) + } as never) + } + + const resizeListener = (): ((event: unknown, args: unknown) => void) => { + const call = onMock.mock.calls.find((entry: unknown[]) => entry[0] === 'pty:resize') + if (!call) { + throw new Error('missing pty:resize listener') + } + return call[1] as (event: unknown, args: unknown) => void + } + + it('returns the applied (wide) size after a dropped narrow resize', async () => { + // The daemon keeps the PTY at its wide spawn size; the narrow resize is + // silently dropped (provider.resize is a no-op fire-and-forget). + setupProviderWithAppliedSize({ applied: { cols: 200, rows: 50 } }) + handlers.clear() + registerPtyHandlers(mainWindow as never) + const spawn = await handlers.get('pty:spawn')!(null, { cols: 200, rows: 50, env: {} }) + const id = (spawn as { id: string }).id + + // Renderer forwards a corrective narrow resize; it is dropped daemon-side. + resizeListener()(mainWindowIpcEvent, { id, cols: 80, rows: 24 }) + + // pty:getSize must surface the applied wide size so the renderer detects + // drift (xterm=80 vs PTY=200) and re-asserts — NOT the requested 80. + const reported = await handlers.get('pty:getSize')!(null, { id }) + expect(reported).toEqual({ cols: 200, rows: 50 }) + }) + + it('falls back to the requested size when the provider cannot report applied size', async () => { + // No getAppliedSize (e.g. SSH relay): the requested-size cache is the only + // signal, so getSize returns it — preserving prior behavior, not a regression. + setupProviderWithAppliedSize({ applied: null, getAppliedSize: undefined }) + setLocalPtyProvider({ + spawn: vi.fn(async (opts: { sessionId?: string }) => ({ + id: opts.sessionId ?? 'daemon-pty' + })), + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + shutdown: vi.fn(), + onData: vi.fn(() => vi.fn()), + onExit: vi.fn(() => vi.fn()), + listProcesses: vi.fn(async () => []), + getForegroundProcess: vi.fn(async () => null) + } as never) + handlers.clear() + registerPtyHandlers(mainWindow as never) + const spawn = await handlers.get('pty:spawn')!(null, { cols: 200, rows: 50, env: {} }) + const id = (spawn as { id: string }).id + resizeListener()(mainWindowIpcEvent, { id, cols: 80, rows: 24 }) + + const reported = await handlers.get('pty:getSize')!(null, { id }) + expect(reported).toEqual({ cols: 80, rows: 24 }) + }) + + it('falls back to the requested size when getAppliedSize throws', async () => { + // A dead daemon/relay must never throw across the IPC boundary or block. + setupProviderWithAppliedSize({ + applied: null, + getAppliedSize: async () => { + throw new Error('daemon unreachable') + } + }) + handlers.clear() + registerPtyHandlers(mainWindow as never) + const spawn = await handlers.get('pty:spawn')!(null, { cols: 100, rows: 30, env: {} }) + const id = (spawn as { id: string }).id + + const reported = await handlers.get('pty:getSize')!(null, { id }) + expect(reported).toEqual({ cols: 100, rows: 30 }) + }) + }) + it('injects ORCA_TERMINAL_HANDLE for non-local PTY providers', async () => { const spawn = vi.fn(async () => ({ id: 'remote-pty' })) registerSshPtyProvider('ssh-1', { diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index a64371fb3..4190b2136 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -3241,15 +3241,29 @@ export function registerPtyHandlers( // Why: the renderer forwards resizes fire-and-forget and otherwise has no way // to learn the PTY's actual size. A resize dropped main-side (suppression - // window, mobile-driver gate, or a provider no-op) leaves the renderer - // believing it synced when it did not, so a later same-cols layout never - // re-forwards and the TUI stays garbled. Exposing the last APPLIED size lets - // the renderer detect true drift on resume and re-assert. ptySizes is only - // written when a resize actually lands (and at spawn), so it is the - // authoritative "what the PTY believes it is". + // window, mobile-driver gate, or a provider no-op) OR daemon/SSH-side (the + // remote resize notify is unacked and can be silently dropped — session not + // yet alive, exited, invalid dims, cold-restore snapshot-col coercion) leaves + // the renderer believing it synced when it did not, so a later same-cols + // layout never re-forwards and the TUI stays garbled. ptySizes records only + // the REQUESTED size, so it cannot reveal such a drop. Prefer the provider's + // APPLIED size (node-pty's cached winsize / the daemon emulator's dims, which + // track the subprocess resize) so the renderer's resume drift-check sees the + // truth; fall back to ptySizes only when the provider can't report (no + // getAppliedSize, e.g. SSH relay, or an unknown id) — a null then reads as + // "cannot confirm", which the renderer treats as a cue to re-forward once. ipcMain.handle( 'pty:getSize', - (_event, args: { id: string }): { cols: number; rows: number } | null => { + async (_event, args: { id: string }): Promise<{ cols: number; rows: number } | null> => { + try { + const applied = await tryGetProviderForPty(args.id)?.getAppliedSize?.(args.id) + if (applied) { + return applied + } + } catch { + // Fall through to the requested-size cache on any provider/RPC failure + // so a dead daemon/relay never blocks or throws across the IPC boundary. + } return ptySizes.get(args.id) ?? null } ) diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index e1a1d560a..907e49bc0 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -817,6 +817,19 @@ export class LocalPtyProvider implements IPtyProvider { ptyProcesses.get(id)?.resize(cols, rows) } + // Why: node-pty caches the last winsize it applied on the IPty handle, so its + // cols/rows are the authoritative applied size (node-pty clamps invalid dims + // and a resize on a dead handle is a no-op, neither of which the requested + // size in ptySizes would reflect). The renderer's resume drift-check compares + // against this to re-assert a resize the PTY never actually took. + async getAppliedSize(id: string): Promise<{ cols: number; rows: number } | null> { + const proc = ptyProcesses.get(id) + if (!proc || proc.cols <= 0 || proc.rows <= 0) { + return null + } + return { cols: proc.cols, rows: proc.rows } + } + async shutdown(id: string, _opts: { immediate?: boolean; keepHistory?: boolean }): Promise { const proc = ptyProcesses.get(id) if (!proc) { diff --git a/src/main/providers/types.ts b/src/main/providers/types.ts index 2dc212885..0b148bf6a 100644 --- a/src/main/providers/types.ts +++ b/src/main/providers/types.ts @@ -105,6 +105,19 @@ export type IPtyProvider = { hasPty?: (id: string) => boolean write(id: string, data: string): void resize(id: string, cols: number, rows: number): void + /** + * The size the PTY has ACTUALLY applied, not the last size requested. + * resize() is fire-and-forget for remote providers (daemon/SSH `notify`), + * so a resize can be silently dropped (session not yet alive, dead handle, + * cold-restore snapshot-cols coercion) while the caller still believes it + * landed. This is the readback the renderer's resume drift-check compares + * against to detect — and re-assert past — such drops. Returns null when the + * provider cannot confirm the applied size (unknown id, relay unreachable); + * callers treat null as "cannot confirm" and re-forward once. Optional so + * providers without an authoritative size source can omit it. + */ + getAppliedSize?: (id: string) => Promise<{ cols: number; rows: number } | null> + shutdown(id: string, opts: { immediate?: boolean; keepHistory?: boolean }): Promise sendSignal(id: string, signal: string): Promise getCwd(id: string): Promise diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 7ec8ac3df..a8e2118c2 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -2283,6 +2283,13 @@ export function connectPanePty( return cols > 0 && rows > 0 ? { cols, rows } : null }, resize: (cols, rows) => transport.resize(cols, rows), + // Why: confirm the PTY actually applied the size we forwarded before the + // reconcile hands off. transport.resize is fire-and-forget for daemon/SSH + // PTYs, so the loop can otherwise settle on a size the PTY dropped, leaving + // it pinned wide while xterm shows narrow — the mount-time desync. Skip + // remote-runtime PTYs (separate viewport channel; pty:getSize never tracks + // them) so they fall back to the grid-stable handoff. + getAppliedSize: isRemoteRuntimePtyId(ptyId) ? undefined : () => window.api.pty.getSize(ptyId), requestFrame: (callback) => requestAnimationFrame(callback), cancelFrame: (handle) => { if (typeof cancelAnimationFrame === 'function') { diff --git a/src/renderer/src/components/terminal-pane/pty-size-reconcile.test.ts b/src/renderer/src/components/terminal-pane/pty-size-reconcile.test.ts index 9f02527d5..441304bca 100644 --- a/src/renderer/src/components/terminal-pane/pty-size-reconcile.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-size-reconcile.test.ts @@ -312,4 +312,127 @@ describe('reconcilePtySizeAcrossFrames', () => { expect(resize.mock.calls.length).toBe(callsBefore) expect(scheduler.pending()).toBe(0) }) + + // Why: the grid being stable only proves what the loop SENT held steady, not + // what the PTY APPLIED. transport.resize is fire-and-forget for daemon/SSH + // PTYs, so the loop can settle on a size the PTY dropped — the mount-time twin + // of the resume drift. getAppliedSize lets the loop confirm before handing off. + describe('applied-size verification before handoff', () => { + /** Drain frames, flushing microtasks between each so async getAppliedSize + * promises resolve and influence the next frame (mirrors real rAF timing). */ + async function runAsync( + scheduler: ReturnType, + maxFrames = 1000 + ): Promise { + let ran = 0 + while (scheduler.pending() > 0 && ran < maxFrames) { + scheduler.run(1) + ran += 1 + // Let any getAppliedSize().then(...) settle before the next frame. + await Promise.resolve() + await Promise.resolve() + } + } + + it('keeps converging when the PTY drops the resize (applied stays wide)', async () => { + const scheduler = createFrameScheduler() + const resize = vi.fn() + // xterm settles narrow immediately, but the PTY never applies it: every + // applied-size read reports the stale wide spawn width. + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + reconcilePtySizeAcrossFrames({ + spawnCols: 203, + spawnRows: 50, + isAlive: () => true, + isParked: () => false, + isAuthoritative: () => true, + measure: pane.measure, + resize, + getAppliedSize: async () => ({ cols: 203, rows: 50 }), + requestFrame: scheduler.requestFrame, + cancelFrame: scheduler.cancelFrame + }) + await runAsync(scheduler, 400) + + // The loop must have re-forwarded the narrow size more than once (the + // initial settle plus at least one verify-driven re-forward) and only + // terminated at the hard cap, never falsely handing off on a dropped size. + const narrowForwards = resize.mock.calls.filter((c) => c[0] === 79 && c[1] === 50) + expect(narrowForwards.length).toBeGreaterThan(1) + }) + + it('hands off once the applied size matches the forwarded grid', async () => { + const scheduler = createFrameScheduler() + const resize = vi.fn() + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + let applied = { cols: 203, rows: 50 } + // The PTY applies the narrow size after the first corrective forward. + reconcilePtySizeAcrossFrames({ + spawnCols: 203, + spawnRows: 50, + isAlive: () => true, + isParked: () => false, + isAuthoritative: () => true, + measure: pane.measure, + resize: vi.fn((cols, rows) => { + resize(cols, rows) + applied = { cols, rows } + }), + getAppliedSize: async () => applied, + requestFrame: scheduler.requestFrame, + cancelFrame: scheduler.cancelFrame + }) + await runAsync(scheduler, 400) + + // It converges and then STOPS (no pending frames) well before the hard cap. + expect(resize).toHaveBeenLastCalledWith(79, 50) + expect(scheduler.pending()).toBe(0) + }) + + it('hands off when applied size cannot be confirmed (null read)', async () => { + const scheduler = createFrameScheduler() + const resize = vi.fn() + const pane = createTimelinePane(() => ({ cols: 79, rows: 50 })) + reconcilePtySizeAcrossFrames({ + spawnCols: 203, + spawnRows: 50, + isAlive: () => true, + isParked: () => false, + isAuthoritative: () => true, + measure: pane.measure, + resize, + // A provider that cannot confirm applied size must not wedge the loop. + getAppliedSize: async () => null, + requestFrame: scheduler.requestFrame, + cancelFrame: scheduler.cancelFrame + }) + await runAsync(scheduler, 400) + expect(scheduler.pending()).toBe(0) + }) + + it('does not verify or re-forward while parked — mobile drives at phone dims', async () => { + const scheduler = createFrameScheduler() + const resize = vi.fn() + // A mobile-driven PTY legitimately sits at phone dims (≠ our desktop grid). + // The parked gate must suppress the verify entirely so we never spin + // re-forwarding a desktop size the mobile gate would drop. + const getAppliedSize = vi.fn(async () => ({ cols: 40, rows: 30 })) + const pane = createTimelinePane(() => ({ cols: 120, rows: 40 })) + reconcilePtySizeAcrossFrames({ + spawnCols: 120, + spawnRows: 40, + isAlive: () => true, + isParked: () => true, + isAuthoritative: () => true, + measure: pane.measure, + resize, + getAppliedSize, + requestFrame: scheduler.requestFrame, + cancelFrame: scheduler.cancelFrame + }) + await runAsync(scheduler, 400) + expect(getAppliedSize).not.toHaveBeenCalled() + expect(resize).not.toHaveBeenCalled() + }) + }) }) diff --git a/src/renderer/src/components/terminal-pane/pty-size-reconcile.ts b/src/renderer/src/components/terminal-pane/pty-size-reconcile.ts index d8c290d69..a77d49b66 100644 --- a/src/renderer/src/components/terminal-pane/pty-size-reconcile.ts +++ b/src/renderer/src/components/terminal-pane/pty-size-reconcile.ts @@ -55,6 +55,18 @@ export type PtySizeReconcileOptions = { measure: () => PtySizeReconcileDimensions | null /** Forward the settled size to the PTY (authoritative — bypasses visibility). */ resize: (cols: number, rows: number) => void + /** + * Read the size the PTY has ACTUALLY applied (vs what this loop last sent). + * Optional. resize() is fire-and-forget for remote (daemon/SSH) PTYs, so the + * loop can settle on a size it sent that the PTY silently dropped — the + * mount-time twin of the resume drift the visibility-resume re-assert heals. + * Before handing off, the loop reads this once; if it diverges from the + * last-sent grid it re-forwards and keeps converging. Returns null when the + * applied size cannot be confirmed (treated as "synced enough to hand off" so + * a provider without a readback, or a transient failure, cannot wedge the + * loop until MAX_FRAMES). + */ + getAppliedSize?: () => Promise /** Schedule the next frame; mirrors requestAnimationFrame's id contract. */ requestFrame: (callback: () => void) => number cancelFrame: (handle: number) => void @@ -92,6 +104,12 @@ export function reconcilePtySizeAcrossFrames( let lastSentRows = options.spawnRows let pendingFrame: number | null = null let cancelled = false + // One-shot applied-size verification before handoff. `verifyInFlight` prevents + // re-issuing the async read every frame while it resolves; `appliedVerified` + // is the terminal "the PTY confirmed our size (or can't be read)" flag that + // lets the loop stop. A re-forward on divergence clears it so we re-verify. + let verifyInFlight = false + let appliedVerified = options.getAppliedSize === undefined const tick = (): void => { pendingFrame = null @@ -110,6 +128,7 @@ export function reconcilePtySizeAcrossFrames( lastSentCols = measured.cols lastSentRows = measured.rows authoritativeStableFrames = 0 + appliedVerified = options.getAppliedSize === undefined } else if (options.isAuthoritative()) { // Only stability seen *under authority* counts toward handoff — a grid // that merely held steady while hidden is not a safe stopping point. @@ -124,7 +143,47 @@ export function reconcilePtySizeAcrossFrames( // layout settle. Once the pane has been visible AND its grid has held steady // for the settle window, the live onResize/ResizeObserver path owns any // further change, so we hand off. The hard cap guarantees termination. - const settled = authoritativeStableFrames >= POST_SPAWN_RECONCILE_SETTLE_FRAMES + const gridStable = authoritativeStableFrames >= POST_SPAWN_RECONCILE_SETTLE_FRAMES + // Why verify before handoff: the grid being stable only proves what we SENT + // held steady, not what the PTY APPLIED. For a fire-and-forget remote resize + // the PTY can be pinned at a stale (wide) size while xterm reflowed narrow — + // the mount-time form of the bug that garbles alt-screen TUIs. Read the + // applied size once; re-forward and keep converging on divergence. Skip while + // parked: a mobile-driven PTY legitimately sits at phone dims (≠ our desktop + // grid), and verifying there would spin the loop re-forwarding a size the + // mobile gate correctly drops — the same reason measure/forward skip parked. + if ( + gridStable && + !appliedVerified && + !verifyInFlight && + !options.isParked() && + options.getAppliedSize + ) { + verifyInFlight = true + void options + .getAppliedSize() + .then((applied) => { + if (cancelled || !options.isAlive()) { + return + } + if (applied && (applied.cols !== lastSentCols || applied.rows !== lastSentRows)) { + // The PTY never took our size — re-forward and keep the loop running. + options.resize(lastSentCols, lastSentRows) + authoritativeStableFrames = 0 + } else { + // Applied matches, or cannot be confirmed (null) — safe to hand off. + appliedVerified = true + } + }) + .catch(() => { + // A failed read must not wedge the loop until MAX_FRAMES. + appliedVerified = true + }) + .finally(() => { + verifyInFlight = false + }) + } + const settled = gridStable && appliedVerified if (!settled && frame < POST_SPAWN_RECONCILE_MAX_FRAMES) { pendingFrame = options.requestFrame(tick) } diff --git a/tests/e2e/terminal-column-desync-repro.spec.ts b/tests/e2e/terminal-column-desync-repro.spec.ts index 54fc3be17..1dad7c22b 100644 --- a/tests/e2e/terminal-column-desync-repro.spec.ts +++ b/tests/e2e/terminal-column-desync-repro.spec.ts @@ -87,6 +87,16 @@ async function readRenderedColsForPty(page: Page, ptyId: string): Promise { + return page.evaluate(async (ptyId) => { + const size = await window.api?.pty?.getSize?.(ptyId) + return size?.cols ?? 0 + }, ptyId) +} + type ColumnSnapshot = { xtermCols: number; ptyCols: number } async function readColumnSnapshot(page: Page, ptyId: string): Promise { @@ -150,6 +160,32 @@ test.describe('Terminal column desync repro', () => { ).toBe(wide.xtermCols) }) + // Why: guards the applied-size IPC contract the desync fix relies on. The + // renderer's resume/handoff drift-check compares xterm against pty:getSize; if + // pty:getSize reports the renderer's last-REQUESTED size (the old intent-only + // behavior) instead of the size the PTY actually APPLIED, a dropped resize is + // invisible and the TUI stays garbled. So pty:getSize must equal the real + // in-PTY process.stdout.columns, not just xterm. + test('pty:getSize reports the size the PTY actually applied', async ({ orcaPage }) => { + test.setTimeout(120_000) + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + await closeRightSidebarAndFeatureTips(orcaPage) + await ensureTerminalVisible(orcaPage) + const ptyId = await settleTerminal(orcaPage) + + await orcaPage.setViewportSize({ width: 900, height: 800 }) + await orcaPage.waitForTimeout(500) + + const ptyCols = await readPtyCols(orcaPage, ptyId) + const reportedCols = await readReportedPtyCols(orcaPage, ptyId) + expect( + reportedCols, + `pty:getSize reported ${reportedCols} but the PTY's process.stdout.columns is ${ptyCols}; ` + + `getSize must reflect the APPLIED size so the drift-check can detect a dropped resize` + ).toBe(ptyCols) + }) + test('PTY columns re-sync after the terminal is resized while hidden', async ({ orcaPage }) => { test.setTimeout(120_000) await waitForSessionReady(orcaPage)