From f1cc636f053db4c90cee571067d0a4ee063bd76f Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 8 Aug 2026 14:22:05 -0700 Subject: [PATCH] fix(terminal): dedupe PTY ownership at hydration instead of dropping rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reland of #13060 (reverted by #13098). The Grok pane restored at a stale width because a session can persist the same PTY id on two terminal rows. Restoring both made two panes mirror one PTY, and the hidden mirror's fit resized the shared PTY to its stale dimensions. #13060 fixed this by filtering non-canonical rows out of hydration, which also deleted legacy rows that owned live panes of their own — a row whose layout listed one shared leaf lost all its unshared leaves with it. Instead, keep every row and assign each PTY id a single owner (canonical unified-tab row first, then sortOrder/createdAt). Non-owners keep their row and spawn a fresh PTY on activation rather than mirror-mounting one they do not own. --- .../terminal-hydration-pty-ownership.test.ts | 70 ++++ .../terminal-hydration-pty-ownership.ts | 57 ++++ ...inals-hydration-duplicate-pty-rows.test.ts | 308 ++++++++++++++++++ src/renderer/src/store/slices/terminals.ts | 55 +++- 4 files changed, 486 insertions(+), 4 deletions(-) create mode 100644 src/renderer/src/store/slices/terminal-hydration-pty-ownership.test.ts create mode 100644 src/renderer/src/store/slices/terminal-hydration-pty-ownership.ts create mode 100644 src/renderer/src/store/slices/terminals-hydration-duplicate-pty-rows.test.ts diff --git a/src/renderer/src/store/slices/terminal-hydration-pty-ownership.test.ts b/src/renderer/src/store/slices/terminal-hydration-pty-ownership.test.ts new file mode 100644 index 00000000000..2ce0c3d31c1 --- /dev/null +++ b/src/renderer/src/store/slices/terminal-hydration-pty-ownership.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest' +import { resolveHydrationPtyOwnership } from './terminal-hydration-pty-ownership' + +const row = ( + tabId: string, + ptyIds: string[], + overrides: { + isCanonical?: boolean + sortOrder?: number + createdAt?: number + } = {} +) => ({ + tabId, + isCanonical: overrides.isCanonical ?? false, + sortOrder: overrides.sortOrder ?? 0, + createdAt: overrides.createdAt ?? 0, + ptyIds +}) + +describe('resolveHydrationPtyOwnership', () => { + it('gives the canonical row ownership of a PTY a stale row also claims', () => { + const ownership = resolveHydrationPtyOwnership([ + row('stale', ['pty-1'], { sortOrder: 0 }), + row('canonical', ['pty-1'], { isCanonical: true, sortOrder: 9 }) + ]) + + expect(ownership.ownsPtyId('canonical', 'pty-1')).toBe(true) + expect(ownership.ownsPtyId('stale', 'pty-1')).toBe(false) + expect(ownership.ownsAnyPtyId('stale')).toBe(false) + }) + + it('keeps every row that owns a distinct PTY', () => { + const ownership = resolveHydrationPtyOwnership([ + row('canonical', ['pty-1'], { isCanonical: true }), + row('legacy', ['pty-2']) + ]) + + expect(ownership.ownsPtyId('legacy', 'pty-2')).toBe(true) + expect(ownership.ownsAnyPtyId('legacy')).toBe(true) + }) + + it('splits ownership per PTY so a partial overlap keeps the unshared leaf', () => { + const ownership = resolveHydrationPtyOwnership([ + row('canonical', ['shared'], { isCanonical: true }), + row('legacy', ['shared', 'own']) + ]) + + expect(ownership.ownsPtyId('legacy', 'shared')).toBe(false) + expect(ownership.ownsPtyId('legacy', 'own')).toBe(true) + expect(ownership.ownsAnyPtyId('legacy')).toBe(true) + }) + + it('falls back to sort order then createdAt when no row is canonical', () => { + const ownership = resolveHydrationPtyOwnership([ + row('later', ['pty-1'], { sortOrder: 1 }), + row('earlier', ['pty-1'], { sortOrder: 0 }) + ]) + + expect(ownership.ownsPtyId('earlier', 'pty-1')).toBe(true) + expect(ownership.ownsPtyId('later', 'pty-1')).toBe(false) + }) + + it('resolves ties deterministically regardless of input order', () => { + const forward = resolveHydrationPtyOwnership([row('b', ['pty-1']), row('a', ['pty-1'])]) + const reverse = resolveHydrationPtyOwnership([row('a', ['pty-1']), row('b', ['pty-1'])]) + + expect(forward.ownsPtyId('a', 'pty-1')).toBe(true) + expect(reverse.ownsPtyId('a', 'pty-1')).toBe(true) + }) +}) diff --git a/src/renderer/src/store/slices/terminal-hydration-pty-ownership.ts b/src/renderer/src/store/slices/terminal-hydration-pty-ownership.ts new file mode 100644 index 00000000000..7e0f2606d9d --- /dev/null +++ b/src/renderer/src/store/slices/terminal-hydration-pty-ownership.ts @@ -0,0 +1,57 @@ +/** + * Assigns each persisted PTY id a single owning terminal row at hydration time. + * + * Why: a session can persist the same PTY id on more than one terminal row (a + * stale row left behind by an interrupted close, a legacy row shadowing its + * canonical unified-tab row). Restoring both makes two panes mirror one PTY, and + * the hidden mirror's fit resizes the shared PTY to its stale dimensions — the + * visible pane then wraps at the wrong width. Rows still restore; only the + * duplicate PTY binding is dropped, so the non-owner spawns a fresh PTY instead. + */ + +export type HydrationPtyOwnershipRow = { + tabId: string + /** Row is referenced by a unified tab, i.e. it is the one the user can see. */ + isCanonical: boolean + sortOrder: number + createdAt: number + ptyIds: readonly string[] +} + +export type HydrationPtyOwnership = { + ownsPtyId: (tabId: string, ptyId: string) => boolean + ownsAnyPtyId: (tabId: string) => boolean +} + +function compareRows(a: HydrationPtyOwnershipRow, b: HydrationPtyOwnershipRow): number { + if (a.isCanonical !== b.isCanonical) { + return a.isCanonical ? -1 : 1 + } + return ( + a.sortOrder - b.sortOrder || + a.createdAt - b.createdAt || + (a.tabId < b.tabId ? -1 : a.tabId > b.tabId ? 1 : 0) + ) +} + +export function resolveHydrationPtyOwnership( + rows: readonly HydrationPtyOwnershipRow[] +): HydrationPtyOwnership { + const ownerTabIdByPtyId = new Map() + const ownedPtyCountByTabId = new Map() + + for (const row of [...rows].sort(compareRows)) { + for (const ptyId of row.ptyIds) { + if (ownerTabIdByPtyId.has(ptyId)) { + continue + } + ownerTabIdByPtyId.set(ptyId, row.tabId) + ownedPtyCountByTabId.set(row.tabId, (ownedPtyCountByTabId.get(row.tabId) ?? 0) + 1) + } + } + + return { + ownsPtyId: (tabId, ptyId) => ownerTabIdByPtyId.get(ptyId) === tabId, + ownsAnyPtyId: (tabId) => (ownedPtyCountByTabId.get(tabId) ?? 0) > 0 + } +} diff --git a/src/renderer/src/store/slices/terminals-hydration-duplicate-pty-rows.test.ts b/src/renderer/src/store/slices/terminals-hydration-duplicate-pty-rows.test.ts new file mode 100644 index 00000000000..3db957e3f32 --- /dev/null +++ b/src/renderer/src/store/slices/terminals-hydration-duplicate-pty-rows.test.ts @@ -0,0 +1,308 @@ +import { describe, expect, it, vi } from 'vitest' +import type { WorkspaceSessionState } from '../../../../shared/types' +import { getDefaultWorkspaceSession } from '../../../../shared/constants' +import { buildWorkspaceSessionPayload } from '@/lib/workspace-session' +import { createTestStore, makeLayout, makeTab, makeWorktree, seedStore } from './store-test-helpers' + +vi.mock('sonner', () => ({ + toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } +})) +vi.mock('@/runtime/sync-runtime-graph', () => ({ + scheduleRuntimeGraphSync: vi.fn() +})) +vi.mock('@/components/terminal-pane/pty-transport', () => ({ + registerEagerPtyBuffer: vi.fn(), + ensurePtyDispatcher: vi.fn() +})) + +const apiProxy = (): unknown => + new Proxy(() => undefined, { + get: (_target, prop) => (prop === 'then' ? undefined : apiProxy()), + apply: () => Promise.resolve(null) + }) + +// @ts-expect-error -- mocked browser preload API +globalThis.window = { api: apiProxy() } + +const WORKTREE_ID = 'repo1::/wt-1' + +function seedWorktree(store: ReturnType): void { + seedStore(store, { + worktreesByRepo: { + repo1: [makeWorktree({ id: WORKTREE_ID, repoId: 'repo1', path: '/wt-1' })] + } + }) +} + +function baseSession(overrides: Partial): WorkspaceSessionState { + return { + ...getDefaultWorkspaceSession(), + activeRepoId: 'repo1', + activeWorktreeId: WORKTREE_ID, + activeWorktreeIdsOnShutdown: [WORKTREE_ID], + ...overrides + } +} + +function unifiedTerminalTab(entityId: string, label: string, sortOrder: number) { + return { + id: `${entityId}-unified`, + entityId, + groupId: 'group-1', + worktreeId: WORKTREE_ID, + contentType: 'terminal' as const, + label, + customLabel: null, + color: null, + sortOrder, + createdAt: sortOrder + 1 + } +} + +describe('hydrateWorkspaceSession duplicate PTY rows', () => { + it('gives the canonical Grok row sole ownership of a PTY a stale row also claims', () => { + const store = createTestStore() + seedWorktree(store) + const grokPtyId = 'daemon-session-grok' + + store.getState().hydrateWorkspaceSession( + baseSession({ + tabsByWorktree: { + [WORKTREE_ID]: [ + makeTab({ + id: 'stale-tab', + worktreeId: WORKTREE_ID, + ptyId: grokPtyId, + sortOrder: 0 + }), + makeTab({ + id: 'grok-tab', + worktreeId: WORKTREE_ID, + ptyId: grokPtyId, + sortOrder: 1 + }) + ] + }, + terminalLayoutsByTabId: { + 'stale-tab': { + ...makeLayout(), + ptyIdsByLeafId: { 'terminal:stale': grokPtyId } + }, + 'grok-tab': { + ...makeLayout(), + ptyIdsByLeafId: { 'terminal:grok': grokPtyId } + } + }, + unifiedTabs: { + [WORKTREE_ID]: [unifiedTerminalTab('grok-tab', 'Grok', 0)] + } + }) + ) + + const state = store.getState() + // The stale row still restores — it just cannot mirror-mount (and resize) Grok's PTY. + expect(state.tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + 'stale-tab', + 'grok-tab' + ]) + expect(state.pendingReconnectPtyIdByTabId).toEqual({ + 'grok-tab': grokPtyId + }) + expect(state.pendingReconnectTabByWorktree[WORKTREE_ID]).toEqual(['grok-tab']) + expect(state.terminalLayoutsByTabId['grok-tab']?.ptyIdsByLeafId).toEqual({ + 'terminal:grok': grokPtyId + }) + expect(state.terminalLayoutsByTabId['stale-tab']?.ptyIdsByLeafId).toEqual({}) + // Regression (#13098): the row must survive the next persist too, not just hydration. + expect( + buildWorkspaceSessionPayload(state).tabsByWorktree[WORKTREE_ID]?.map((t) => t.id) + ).toEqual(['stale-tab', 'grok-tab']) + }) + + it('strips a duplicated relay session id from the non-canonical row only', () => { + const store = createTestStore() + seedWorktree(store) + const relayId = 'relay-session-grok' + + store.getState().hydrateWorkspaceSession( + baseSession({ + tabsByWorktree: { + [WORKTREE_ID]: [ + makeTab({ id: 'stale-tab', worktreeId: WORKTREE_ID, sortOrder: 0 }), + makeTab({ id: 'grok-tab', worktreeId: WORKTREE_ID, sortOrder: 1 }) + ] + }, + remoteSessionIdsByTabId: { 'stale-tab': relayId, 'grok-tab': relayId }, + unifiedTabs: { + [WORKTREE_ID]: [unifiedTerminalTab('grok-tab', 'Grok', 0)] + } + }) + ) + + const state = store.getState() + expect(state.tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + 'stale-tab', + 'grok-tab' + ]) + expect(state.pendingReconnectPtyIdByTabId).toEqual({ 'grok-tab': relayId }) + expect(state.pendingReconnectTabByWorktree[WORKTREE_ID]).toEqual(['grok-tab']) + }) + + it('restores non-canonical legacy rows that own their own PTY (regression: #13098)', () => { + const store = createTestStore() + seedWorktree(store) + + store.getState().hydrateWorkspaceSession( + baseSession({ + tabsByWorktree: { + [WORKTREE_ID]: [ + makeTab({ + id: 'grok-tab', + worktreeId: WORKTREE_ID, + ptyId: 'pty-grok', + sortOrder: 0 + }), + makeTab({ + id: 'legacy-tab', + worktreeId: WORKTREE_ID, + ptyId: 'pty-legacy', + sortOrder: 1 + }) + ] + }, + terminalLayoutsByTabId: { + 'grok-tab': { + ...makeLayout(), + ptyIdsByLeafId: { 'terminal:grok': 'pty-grok' } + }, + 'legacy-tab': { + ...makeLayout(), + ptyIdsByLeafId: { 'terminal:legacy': 'pty-legacy' } + } + }, + // Only the Grok tab has a unified row; the legacy tab must survive anyway. + unifiedTabs: { + [WORKTREE_ID]: [unifiedTerminalTab('grok-tab', 'Grok', 0)] + } + }) + ) + + const state = store.getState() + expect(state.tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + 'grok-tab', + 'legacy-tab' + ]) + expect(state.pendingReconnectTabByWorktree[WORKTREE_ID]).toEqual(['grok-tab', 'legacy-tab']) + expect(state.pendingReconnectPtyIdByTabId).toEqual({ + 'grok-tab': 'pty-grok', + 'legacy-tab': 'pty-legacy' + }) + expect(state.terminalLayoutsByTabId['legacy-tab']?.ptyIdsByLeafId).toEqual({ + 'terminal:legacy': 'pty-legacy' + }) + }) + + it('restores every row of a legacy session that has no unified tabs at all', () => { + const store = createTestStore() + seedWorktree(store) + + store.getState().hydrateWorkspaceSession( + baseSession({ + tabsByWorktree: { + [WORKTREE_ID]: [ + makeTab({ + id: 'legacy-a', + worktreeId: WORKTREE_ID, + ptyId: 'pty-a', + sortOrder: 0 + }), + makeTab({ + id: 'legacy-b', + worktreeId: WORKTREE_ID, + ptyId: 'pty-b', + sortOrder: 1 + }) + ] + }, + terminalLayoutsByTabId: { + 'legacy-a': { + ...makeLayout(), + ptyIdsByLeafId: { 'terminal:a': 'pty-a' } + }, + 'legacy-b': { + ...makeLayout(), + ptyIdsByLeafId: { 'terminal:b': 'pty-b' } + } + } + }) + ) + + const state = store.getState() + expect(state.tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + 'legacy-a', + 'legacy-b' + ]) + expect(state.pendingReconnectPtyIdByTabId).toEqual({ + 'legacy-a': 'pty-a', + 'legacy-b': 'pty-b' + }) + expect(state.pendingReconnectTabByWorktree[WORKTREE_ID]).toEqual(['legacy-a', 'legacy-b']) + expect(state.terminalLayoutsByTabId['legacy-b']?.ptyIdsByLeafId).toEqual({ + 'terminal:b': 'pty-b' + }) + }) + + it('keeps a split leaf that only the legacy row owns while dropping the shared one', () => { + const store = createTestStore() + seedWorktree(store) + const sharedPtyId = 'daemon-session-shared' + + store.getState().hydrateWorkspaceSession( + baseSession({ + tabsByWorktree: { + [WORKTREE_ID]: [ + makeTab({ + id: 'grok-tab', + worktreeId: WORKTREE_ID, + ptyId: sharedPtyId, + sortOrder: 0 + }), + makeTab({ + id: 'legacy-tab', + worktreeId: WORKTREE_ID, + sortOrder: 1 + }) + ] + }, + terminalLayoutsByTabId: { + 'grok-tab': { + ...makeLayout(), + ptyIdsByLeafId: { 'terminal:grok': sharedPtyId } + }, + 'legacy-tab': { + ...makeLayout(), + ptyIdsByLeafId: { + 'terminal:mirror': sharedPtyId, + 'terminal:own': 'pty-own' + } + } + }, + unifiedTabs: { + [WORKTREE_ID]: [unifiedTerminalTab('grok-tab', 'Grok', 0)] + } + }) + ) + + const state = store.getState() + expect(state.tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + 'grok-tab', + 'legacy-tab' + ]) + expect(state.terminalLayoutsByTabId['legacy-tab']?.ptyIdsByLeafId).toEqual({ + 'terminal:own': 'pty-own' + }) + expect(state.terminalLayoutsByTabId['grok-tab']?.ptyIdsByLeafId).toEqual({ + 'terminal:grok': sharedPtyId + }) + }) +}) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 7167e1cfc4d..a59cf11d1b4 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -58,6 +58,7 @@ import { forgetForegroundTerminalTabs } from '@/lib/foreground-terminal-tabs' import { terminalLayoutEqual } from '@/lib/terminal-layout-equality' import { forgetAgentStartupDeliveriesForTabs } from '@/lib/agent-startup-delivery-guards' import { clearTransientTerminalState, emptyLayoutSnapshot } from './terminal-helpers' +import { resolveHydrationPtyOwnership } from './terminal-hydration-pty-ownership' import { getRecentlyClosedTabPosition, pushClosedTerminalTabSnapshot, @@ -492,6 +493,17 @@ function uniquePtyIds(ptyIds: readonly (string | null | undefined)[]): string[] return [...new Set(ptyIds.filter((ptyId): ptyId is string => Boolean(ptyId)))] } +function collectPersistedTerminalPtyIds( + session: WorkspaceSessionState, + tab: TerminalTab +): string[] { + return uniquePtyIds([ + tab.ptyId, + session.remoteSessionIdsByTabId?.[tab.id], + ...Object.values(session.terminalLayoutsByTabId[tab.id]?.ptyIdsByLeafId ?? {}) + ]) +} + function resolvePrimaryLayoutPtyId(layout: TerminalLayoutSnapshot): string | null { const ptyIdsByLeafId = layout.ptyIdsByLeafId ?? {} const activePtyId = layout.activeLeafId ? ptyIdsByLeafId[layout.activeLeafId] : undefined @@ -3938,6 +3950,27 @@ export const createTerminalSlice: StateCreator validWorktreeIds.add(folderWorkspaceKey(workspace.id)) } addAdditionalValidWorkspaceKeys(validWorktreeIds, options) + // Why: PTY ids are globally unique, so resolve ownership across every restored workspace at once. + const ptyOwnership = resolveHydrationPtyOwnership( + Object.entries(session.tabsByWorktree) + .filter(([worktreeId]) => validWorktreeIds.has(worktreeId)) + .flatMap(([worktreeId, tabs]) => { + const canonicalTerminalIds = new Set( + (session.unifiedTabs?.[worktreeId] ?? []).flatMap((tab) => + tab.contentType === 'terminal' ? [tab.entityId] : [] + ) + ) + return tabs + .filter((tab) => isValidTerminalTabId(tab.id)) + .map((tab) => ({ + tabId: tab.id, + isCanonical: canonicalTerminalIds.has(tab.id), + sortOrder: tab.sortOrder, + createdAt: tab.createdAt, + ptyIds: collectPersistedTerminalPtyIds(session, tab) + })) + }) + ) // Why: suppress restored mounts so only real activity updates Recent. const tabsByWorktree: Record = Object.fromEntries( Object.entries(session.tabsByWorktree) @@ -4039,7 +4072,13 @@ export const createTerminalSlice: StateCreator for (const worktreeId of pendingReconnectWorktreeIds) { const rawTabs = session.tabsByWorktree[worktreeId] ?? [] const liveTabIds = rawTabs - .filter((t) => (t.ptyId || remoteSessionIds[t.id]) && validTabIds.has(t.id)) + .filter( + (t) => + (t.ptyId || remoteSessionIds[t.id]) && + validTabIds.has(t.id) && + // Why: a row whose every PTY id belongs to another row has nothing to reattach to. + ptyOwnership.ownsAnyPtyId(t.id) + ) .map((t) => t.id) if (liveTabIds.length > 0) { pendingReconnectTabByWorktree[worktreeId] = liveTabIds @@ -4060,7 +4099,7 @@ export const createTerminalSlice: StateCreator } const rawTabs = session.tabsByWorktree[worktreeId] ?? [] for (const tab of rawTabs) { - if (tab.ptyId && validTabIds.has(tab.id)) { + if (tab.ptyId && validTabIds.has(tab.id) && ptyOwnership.ownsPtyId(tab.id, tab.ptyId)) { pendingReconnectPtyIdByTabId[tab.id] = tab.ptyId } } @@ -4068,7 +4107,7 @@ export const createTerminalSlice: StateCreator // Why: remote PTY reattach uses the relay's pty.attach RPC, not the local daemon; the loop above skips SSH repos, so no overlap. for (const [tabId, sessionId] of Object.entries(remoteSessionIds)) { - if (validTabIds.has(tabId)) { + if (validTabIds.has(tabId) && ptyOwnership.ownsPtyId(tabId, sessionId)) { pendingReconnectPtyIdByTabId[tabId] = sessionId } } @@ -4191,7 +4230,15 @@ export const createTerminalSlice: StateCreator ) } const tab = tabById.get(tabId) - const sanitized = tab ? sanitizeTerminalLayoutPaneTitles(normalized, tab) : normalized + const titled = tab ? sanitizeTerminalLayoutPaneTitles(normalized, tab) : normalized + // Why: drop leaf bindings owned by another row so this layout cannot mirror-mount and resize a PTY it does not own. + const ownedLeafEntries = Object.entries(titled.ptyIdsByLeafId ?? {}).filter( + ([, ptyId]) => (ptyId ? ptyOwnership.ownsPtyId(tabId, ptyId) : true) + ) + const sanitized = + ownedLeafEntries.length === Object.keys(titled.ptyIdsByLeafId ?? {}).length + ? titled + : { ...titled, ptyIdsByLeafId: Object.fromEntries(ownedLeafEntries) } const activeLeafId = sanitized.root ? resolvePtyBoundActiveLeafId({ root: sanitized.root,