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,