From ce54ff3aa0323a68d5aa74f21e31beefad971f32 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 12 Jul 2026 23:46:17 -0700 Subject: [PATCH] fix(terminal): stop stale PTY resize after worktree reveal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The visibility-resume size readback captures xterm's pre-reveal grid as its resize target while the applied-size read is in flight. A reveal fit or snapshot-restore resize can change the grid mid-flight without queuing a newer request, so the resolved callback "repaired" the PTY back to the pre-reveal grid — an idle TUI (Claude Code) then redraws for the wrong grid until a manual resize (refs #7951, #7240). Instrumented traces show the stale capture on every reveal; correctness relied on the read winning FIFO against the fit's resize, which busy daemons and SSH/relay round-trips lose. Re-measure xterm at resolve time and re-run against the fresh grid instead of forwarding a stale target. Adds an e2e seam that delays the readback dispatch to reproduce the losing ordering, unit repros that fail without the guard, and a hidden-resize/reveal-cycle e2e spec. --- .../pty-applied-size-read-e2e-delay.ts | 15 + .../terminal-pane/pty-connection.ts | 11 +- .../pty-size-reassertion.test.ts | 108 ++++++- .../terminal-pane/pty-size-reassertion.ts | 8 + src/renderer/src/env.d.ts | 1 + ...inal-reveal-stale-pty-resize-repro.spec.ts | 296 ++++++++++++++++++ 6 files changed, 437 insertions(+), 2 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/pty-applied-size-read-e2e-delay.ts create mode 100644 tests/e2e/terminal-reveal-stale-pty-resize-repro.spec.ts diff --git a/src/renderer/src/components/terminal-pane/pty-applied-size-read-e2e-delay.ts b/src/renderer/src/components/terminal-pane/pty-applied-size-read-e2e-delay.ts new file mode 100644 index 00000000000..a56e093a309 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-applied-size-read-e2e-delay.ts @@ -0,0 +1,15 @@ +import { e2eConfig } from '@/lib/e2e-config' + +// Why: e2e seam for the stale-reveal-resize regression spec. The visibility +// resume readback (pty-size-reassertion) is only safe when the applied-size +// read is processed before the reveal fit's PTY resize; a busy daemon or +// SSH/relay round-trip breaks that ordering in the field. The spec sets this +// window global to reproduce the losing ordering deterministically. Gated on +// exposeStore so packaged builds ignore it. +export function getAppliedSizeReadE2eDelayMs(): number { + if (!e2eConfig.exposeStore || typeof window === 'undefined') { + return 0 + } + const delayMs = window.__e2ePtyAppliedSizeReadDelayMs + return typeof delayMs === 'number' && Number.isFinite(delayMs) && delayMs > 0 ? delayMs : 0 +} diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index ae752d77d97..add578b7e2c 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -47,6 +47,7 @@ import { getFitOverrideForPty, bindPanePtyId } from '@/lib/pane-manager/mobile-f import { isPtyLocked } from '@/lib/pane-manager/mobile-driver-state' import { reconcilePtySizeAcrossFrames, type PtySizeReconcileHandle } from './pty-size-reconcile' import { shouldClaimRemoteDesktopViewport } from './remote-desktop-viewport-claim' +import { getAppliedSizeReadE2eDelayMs } from './pty-applied-size-read-e2e-delay' import { createPtySizeReassertion } from './pty-size-reassertion' import { isPaneReplaying, replayIntoTerminal, replayIntoTerminalAsync } from './replay-guard' import { @@ -3499,7 +3500,15 @@ export function connectPanePty( shouldSuppressDesktopResize: () => shouldSuppressDesktopPtyResize(), fit: () => safeFit(pane), getTerminalDimensions: () => ({ cols: pane.terminal.cols, rows: pane.terminal.rows }), - getAppliedSize: (ptyId) => window.api.pty.getSize(ptyId), + getAppliedSize: async (ptyId) => { + // Why: e2e seam — delays the read past the reveal fit to reproduce the + // busy-daemon/SSH ordering; returns 0 outside e2e builds. + const delayMs = getAppliedSizeReadE2eDelayMs() + if (delayMs > 0) { + await new Promise((resolve) => setTimeout(resolve, delayMs)) + } + return window.api.pty.getSize(ptyId) + }, forwardResize: forwardPtyResize }) let pendingForegroundGridDriftCheckRaf: number | null = null diff --git a/src/renderer/src/components/terminal-pane/pty-size-reassertion.test.ts b/src/renderer/src/components/terminal-pane/pty-size-reassertion.test.ts index 5ad5accfe5f..e733f927cd3 100644 --- a/src/renderer/src/components/terminal-pane/pty-size-reassertion.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-size-reassertion.test.ts @@ -70,7 +70,8 @@ describe('createPtySizeReassertion', () => { reassertion.request() await flushAsyncTicks() - expect(calls).toEqual(['fit', 'measure', 'read-applied']) + // The trailing measure is the resolve-time staleness check. + expect(calls).toEqual(['fit', 'measure', 'read-applied', 'measure']) }) it('does not duplicate the resize when fit already triggered xterm onResize', async () => { @@ -217,6 +218,111 @@ describe('createPtySizeReassertion', () => { expect(forwardResize).not.toHaveBeenCalledWith(100, 40) }) + it('suppresses a stale forward when xterm was refit while the read was in flight', async () => { + // Regression: a reveal-time fit (or snapshot-restore xterm resize) changes + // the grid after the target was captured, with no second request() to + // guard it. The resolved callback must not resize the PTY back to the + // pre-reveal grid. + let dims = { cols: 80, rows: 24 } + let resolveRead: (value: { cols: number; rows: number }) => void = () => {} + const getAppliedSize = vi + .fn<() => Promise<{ cols: number; rows: number } | null>>() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveRead = resolve + }) + ) + .mockResolvedValue({ cols: 145, rows: 78 }) + const forwardResize = vi.fn() + const reassertion = createPtySizeReassertion({ + isDisposed: () => false, + getPtyId: () => 'pty-1', + isRemotePtyId: () => false, + shouldSuppressDesktopResize: () => false, + fit: vi.fn(), + getTerminalDimensions: () => dims, + getAppliedSize, + forwardResize + }) + + reassertion.request({ fit: false }) + dims = { cols: 145, rows: 78 } + resolveRead({ cols: 145, rows: 78 }) + await flushAsyncTicks() + + expect(forwardResize).not.toHaveBeenCalledWith(80, 24) + expect(forwardResize).not.toHaveBeenCalled() + }) + + it('re-runs against the fresh grid when the PTY kept the old size across a mid-flight refit', async () => { + let dims = { cols: 80, rows: 24 } + let resolveRead: (value: { cols: number; rows: number }) => void = () => {} + const getAppliedSize = vi + .fn<() => Promise<{ cols: number; rows: number } | null>>() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveRead = resolve + }) + ) + .mockResolvedValue({ cols: 80, rows: 24 }) + const forwardResize = vi.fn() + const reassertion = createPtySizeReassertion({ + isDisposed: () => false, + getPtyId: () => 'pty-1', + isRemotePtyId: () => false, + shouldSuppressDesktopResize: () => false, + fit: vi.fn(), + getTerminalDimensions: () => dims, + getAppliedSize, + forwardResize + }) + + reassertion.request({ fit: false }) + dims = { cols: 145, rows: 78 } + resolveRead({ cols: 80, rows: 24 }) + await flushAsyncTicks() + + expect(getAppliedSize).toHaveBeenCalledTimes(2) + expect(forwardResize).toHaveBeenCalledTimes(1) + expect(forwardResize).toHaveBeenCalledWith(145, 78) + }) + + it('does not forward a stale target when the readback fails after a mid-flight refit', async () => { + let dims = { cols: 80, rows: 24 } + let rejectRead: (reason: Error) => void = () => {} + const getAppliedSize = vi + .fn<() => Promise<{ cols: number; rows: number } | null>>() + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectRead = reject + }) + ) + .mockRejectedValue(new Error('unavailable')) + const forwardResize = vi.fn() + const reassertion = createPtySizeReassertion({ + isDisposed: () => false, + getPtyId: () => 'pty-1', + isRemotePtyId: () => false, + shouldSuppressDesktopResize: () => false, + fit: vi.fn(), + getTerminalDimensions: () => dims, + getAppliedSize, + forwardResize + }) + + reassertion.request({ fit: false }) + dims = { cols: 145, rows: 78 } + rejectRead(new Error('unavailable')) + await flushAsyncTicks() + + expect(forwardResize).not.toHaveBeenCalledWith(80, 24) + // The unguarded fallback resize still happens, but at the fresh grid. + expect(forwardResize).toHaveBeenCalledWith(145, 78) + }) + it('forwards once when applied-size readback fails', async () => { const forwardResize = vi.fn() const reassertion = createPtySizeReassertion({ diff --git a/src/renderer/src/components/terminal-pane/pty-size-reassertion.ts b/src/renderer/src/components/terminal-pane/pty-size-reassertion.ts index 78a2e8cf5aa..7f127b67083 100644 --- a/src/renderer/src/components/terminal-pane/pty-size-reassertion.ts +++ b/src/renderer/src/components/terminal-pane/pty-size-reassertion.ts @@ -63,6 +63,14 @@ export function createPtySizeReassertion(options: PtySizeReassertionOptions): Pt if (pending) { return } + // Why: a reveal fit or snapshot-restore resize can change xterm while the + // applied-size read is in flight without queuing a request; forwarding the + // captured target would resize the PTY back to the pre-reveal grid, so + // re-run against the fresh grid instead. + if (!dimensionsMatch(options.getTerminalDimensions(), target)) { + pending = true + return + } if (dimensionsMatch(actual, target)) { return } diff --git a/src/renderer/src/env.d.ts b/src/renderer/src/env.d.ts index 676fa253529..e07b5d6a32b 100644 --- a/src/renderer/src/env.d.ts +++ b/src/renderer/src/env.d.ts @@ -73,6 +73,7 @@ declare global { parkedTabIds: () => string[] } __monacoEditorE2E?: MonacoE2EProbe + __e2ePtyAppliedSizeReadDelayMs?: number } } diff --git a/tests/e2e/terminal-reveal-stale-pty-resize-repro.spec.ts b/tests/e2e/terminal-reveal-stale-pty-resize-repro.spec.ts new file mode 100644 index 00000000000..801317ad79f --- /dev/null +++ b/tests/e2e/terminal-reveal-stale-pty-resize-repro.spec.ts @@ -0,0 +1,296 @@ +/** + * Repro: stale PTY resize after worktree reveal (issues #7951 / #7240 family). + * + * Symptom (field reports + internal, v1.4.136): returning to a worktree + * garbles the bottom of an idle TUI (Claude Code) until a manual window + * resize. + * + * Mechanism: on reveal, noteVisibilityResume() requests a PTY size readback + * that captures xterm's pre-reveal grid as the resize target + * (pty-size-reassertion.ts). The reveal fit then refits xterm and resizes the + * PTY while that read is in flight, with no follow-up request queued. + * Instrumented traces show this stale-capture interleaving on EVERY reveal; + * correctness then hinges solely on the applied-size read being processed + * before the fit's resize (local FIFO luck). When the read loses that race — + * busy daemon serializing reveal snapshots, SSH/relay round-trips — the + * resolved callback sees applied != captured target and "repairs" the PTY + * back to the pre-reveal grid. An idle TUI redraws for the wrong grid and + * nothing heals it: no output means no grid-drift check, and no later layout + * change means no ResizeObserver request. + * + * Test 1 drives the choreography with the natural read ordering (documents + * the FIFO-lucky path). Test 2 delays the readback dispatch (e2e seam in + * pty-applied-size-read-e2e-delay.ts) to model the losing ordering — on an + * unpatched build the stale forward fires live (verified via instrumented + * traces: FORWARD of the pre-reveal grid over the freshly fitted one). On + * fast idle machines a follow-up reassertion heals it within tens of ms, so + * the deterministic regression guard for the stale forward itself lives in + * pty-size-reassertion.test.ts; this spec catches the persistent-desync + * variant and keeps the full reveal path exercised under the field ordering. + */ + +import type { Page, TestInfo } from '@stablyai/playwright-test' +import { writeFileSync } from 'node:fs' +import path from 'node:path' +import { test, expect } from './helpers/orca-app' +import { + ensureTerminalVisible, + getActiveWorktreeId, + getAllWorktreeIds, + switchToWorktree, + waitForActiveWorktree, + waitForSessionReady +} from './helpers/store' +import { + sendToTerminal, + waitForActivePanePtyId, + waitForActiveTerminalManager +} from './helpers/terminal' +import { waitForPtyShellEcho } from './terminal-pty-readiness' + +type StaleResizeReproWindow = Window & { + __paneManagers?: Map< + string, + { + getPanes?: () => { + container?: { dataset?: { ptyId?: string } } + terminal?: { cols?: number; rows?: number } + }[] + } + > + __e2ePtyAppliedSizeReadDelayMs?: number +} + +type GridSnapshot = { + xterm: { cols: number; rows: number } | null + applied: { cols: number; rows: number } | null +} + +type CycleFailure = { + cycle: number + snapshot: GridSnapshot +} + +const CONVERGE_TIMEOUT_MS = 6_000 +// Why: sweep the applied-size read delay across the gap between the reveal +// fit's PTY resize landing in the daemon and the ResizeObserver follow-up +// request — the window where the stale forward fires. Fast idle machines +// rescue within ~30-50ms; the field (SSH/relay, loaded frames) may never. +const GETSIZE_DELAY_SWEEP_MS = [15, 20, 25, 30, 40, 60] +const VIEWPORTS = [ + { width: 1280, height: 800 }, + { width: 940, height: 640 }, + { width: 1120, height: 760 } +] + +function bottomBarTuiScript(runId: string): string { + // Why: redraw ONLY on SIGWINCH, like an idle Claude Code session. Periodic + // output would trigger the foreground grid-drift check, which heals the + // desync and masks the bug the field reports hit while idle. + return [ + 'const draw = () => {', + ' const rows = process.stdout.rows || 24', + ' const cols = process.stdout.columns || 80', + ` const bar = 'BOTTOM_BAR_${runId} rows=' + rows + ' cols=' + cols + ' ' + '='.repeat(200)`, + " process.stdout.write('\\x1b7\\x1b[' + rows + ';1H\\x1b[2K' + bar.slice(0, Math.max(1, cols - 1)) + '\\x1b8')", + '}', + "process.stdout.on('resize', draw)", + 'draw()', + 'setTimeout(() => process.exit(0), 600000)' + ].join('\n') +} + +async function readGridSnapshot(page: Page, ptyId: string): Promise { + return page.evaluate(async (ptyId) => { + const win = window as StaleResizeReproWindow + let xterm: { cols: number; rows: number } | null = null + for (const manager of win.__paneManagers?.values() ?? []) { + for (const pane of manager.getPanes?.() ?? []) { + if (pane.container?.dataset?.ptyId === ptyId) { + xterm = { cols: pane.terminal?.cols ?? 0, rows: pane.terminal?.rows ?? 0 } + } + } + } + // Why: the delay seam only affects the product's own readback wiring in + // pty-connection, so probing window.api.pty.getSize directly stays fast. + const applied = (await window.api?.pty?.getSize?.(ptyId)) ?? null + return { xterm, applied } + }, ptyId) +} + +async function closeRightSidebarAndFeatureTips(page: Page): Promise { + await page.evaluate(() => { + const store = window.__store + if (!store) { + return + } + store.getState().markFeatureTipsSeen(['orca-cli', 'cmd-j-palette', 'voice-dictation']) + if (store.getState().rightSidebarOpen) { + store.getState().setRightSidebarOpen(false) + } + }) +} + +function gridsConverged(snapshot: GridSnapshot): boolean { + return ( + snapshot.xterm !== null && + snapshot.applied !== null && + snapshot.xterm.cols > 0 && + snapshot.xterm.rows > 0 && + snapshot.applied.cols === snapshot.xterm.cols && + snapshot.applied.rows === snapshot.xterm.rows + ) +} + +/** Arm the e2e seam that delays the reassertion's applied-size read dispatch + * past the reveal fit — the ordering a busy daemon or SSH/relay round-trip + * produces in the field (pty-applied-size-read-e2e-delay.ts). */ +async function armSlowAppliedSizeRead(page: Page, delayMs: number): Promise { + await page.evaluate((delayMs) => { + ;(window as StaleResizeReproWindow).__e2ePtyAppliedSizeReadDelayMs = delayMs + }, delayMs) +} + +type CycleDriverArgs = { + page: Page + testInfo: TestInfo + testRepoPath: string + cycles: number + label: string + delaySweepMs?: readonly number[] + onRevealSample?: (sample: { cycle: number; snapshot: GridSnapshot }) => void +} + +async function driveHiddenResizeRevealCycles(args: CycleDriverArgs): Promise { + const { page, testInfo, testRepoPath, cycles, label } = args + await waitForSessionReady(page) + const firstWorktreeId = await waitForActiveWorktree(page) + const secondWorktreeId = (await getAllWorktreeIds(page)).find((id) => id !== firstWorktreeId) + test.skip(!secondWorktreeId, 'stale-resize repro needs the seeded secondary worktree') + if (!secondWorktreeId) { + return [] + } + + await page.setViewportSize(VIEWPORTS[0]) + await closeRightSidebarAndFeatureTips(page) + + await switchToWorktree(page, secondWorktreeId) + await ensureTerminalVisible(page) + await waitForActiveTerminalManager(page, 30_000) + const ptyId = await waitForActivePanePtyId(page) + await waitForPtyShellEcho(page, ptyId, 15_000) + + const runId = Math.random().toString(36).slice(2, 10) + const scriptPath = path.join(testRepoPath, `.orca-bottom-bar-${runId}.cjs`) + writeFileSync(scriptPath, bottomBarTuiScript(runId)) + await sendToTerminal(page, ptyId, `node ${JSON.stringify(scriptPath)}\r`) + await expect + .poll(() => readGridSnapshot(page, ptyId).then(gridsConverged), { + timeout: 15_000, + message: `${label}: applied PTY size should match xterm before cycling` + }) + .toBe(true) + + const failures: CycleFailure[] = [] + let viewportIndex = 0 + for (let cycle = 0; cycle < cycles; cycle += 1) { + if (args.delaySweepMs) { + await armSlowAppliedSizeRead(page, args.delaySweepMs[cycle % args.delaySweepMs.length]) + } + await switchToWorktree(page, firstWorktreeId) + await expect.poll(() => getActiveWorktreeId(page), { timeout: 10_000 }).toBe(firstWorktreeId) + // Why: the field shape — the window changes while the idle TUI worktree + // is hidden; hidden xterm refits drop their PTY forwards + // (isRendererPtyResizeAuthoritative), so the reveal-time readback is the + // sole owner of the correction. + viewportIndex = (viewportIndex + 1) % VIEWPORTS.length + await page.setViewportSize(VIEWPORTS[viewportIndex]) + await page.waitForTimeout(400) + + await switchToWorktree(page, secondWorktreeId) + await expect.poll(() => getActiveWorktreeId(page), { timeout: 10_000 }).toBe(secondWorktreeId) + + // Why: sample tightly right after reveal — a stale forward that a later + // follow-up request heals is still the corruption firing (the TUI redraws + // for the wrong grid in that window); record every desynced sample. + for (let sample = 0; sample < 20; sample += 1) { + const snapshot = await readGridSnapshot(page, ptyId) + if (!gridsConverged(snapshot) && snapshot.xterm !== null && snapshot.applied !== null) { + args.onRevealSample?.({ cycle, snapshot }) + } + await page.waitForTimeout(10) + } + + let lastSnapshot: GridSnapshot = { xterm: null, applied: null } + const deadline = Date.now() + CONVERGE_TIMEOUT_MS + let converged = false + while (Date.now() < deadline) { + lastSnapshot = await readGridSnapshot(page, ptyId) + if (gridsConverged(lastSnapshot)) { + converged = true + break + } + await page.waitForTimeout(150) + } + if (!converged) { + failures.push({ cycle, snapshot: lastSnapshot }) + if (failures.length <= 3) { + const screenshotPath = testInfo.outputPath(`${label}-cycle-${cycle}.png`) + await page.screenshot({ path: screenshotPath, fullPage: true }) + await testInfo.attach(`${label}-cycle-${cycle}.png`, { + path: screenshotPath, + contentType: 'image/png' + }) + } + } + } + return failures +} + +test.describe('Terminal reveal stale PTY resize repro', () => { + test('applied PTY size converges across hidden-resize/reveal cycles (natural read ordering)', async ({ + orcaPage, + testRepoPath + }, testInfo: TestInfo) => { + test.setTimeout(300_000) + const failures = await driveHiddenResizeRevealCycles({ + page: orcaPage, + testInfo, + testRepoPath, + cycles: 8, + label: 'natural-order' + }) + expect( + failures, + `PTY applied size stayed desynced from xterm after reveal: ${JSON.stringify(failures)}` + ).toEqual([]) + }) + + test('the PTY is never resized back to its stale pre-reveal grid under slow applied-size reads', async ({ + orcaPage, + testRepoPath + }, testInfo: TestInfo) => { + test.setTimeout(300_000) + const staleSamples: { cycle: number; snapshot: GridSnapshot }[] = [] + const failures = await driveHiddenResizeRevealCycles({ + page: orcaPage, + testInfo, + testRepoPath, + cycles: 12, + label: 'slow-read', + delaySweepMs: GETSIZE_DELAY_SWEEP_MS, + onRevealSample: (sample) => staleSamples.push(sample) + }) + if (staleSamples.length > 0) { + console.log(`[repro] desynced post-reveal samples: ${JSON.stringify(staleSamples)}`) + } + expect( + staleSamples, + `PTY applied size diverged from xterm after reveal (stale resize fired): ${JSON.stringify(staleSamples)}` + ).toEqual([]) + expect( + failures, + `PTY stayed desynced from xterm after reveal: ${JSON.stringify(failures)}` + ).toEqual([]) + }) +})