diff --git a/src/renderer/src/components/terminal-pane/terminal-layout-leaf-claims.ts b/src/renderer/src/components/terminal-pane/terminal-layout-leaf-claims.ts new file mode 100644 index 00000000000..37dd3901be0 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-layout-leaf-claims.ts @@ -0,0 +1,37 @@ +import type { TerminalPaneLayoutNode } from '../../../../shared/terminal-tab-types' +import { collectLeafIdsInOrder } from './terminal-layout-leaf-ids' + +export type TerminalLayoutLeafClaims = { + root: TerminalPaneLayoutNode | null + activeLeafId?: string | null + ptyIdsByLeafId?: Record +} + +/** + * Leaf ids this layout holds, read generously: its tree, or — for a rootless layout, which binds + * its sole pane off-tree — every leaf it binds. Use this to ask "does some pane already hold + * this?", where over-counting only costs a reveal that adopts instead of minting. + * + * A binding whose leaf has left a rooted tree reattaches nothing, so it is excluded either way. + */ +export function collectOwnedLeafIds(layout: TerminalLayoutLeafClaims): Set { + return new Set( + layout.root ? collectLeafIdsInOrder(layout.root) : Object.keys(layout.ptyIdsByLeafId ?? {}) + ) +} + +/** + * Leaf ids this layout may take from another tab, read narrowly. Same as the owned set for a + * rooted layout, but a rootless one proves only its sole off-tree pane, or the one its + * `activeLeafId` names: a never-pruned map holds more than it owns, and claiming those evicts + * the live row that really owns them (#13098). Mirrors the `owned`/`claimable` split in + * `terminal-session-row-hydration.ts`, which is the guard that caught #13060. + */ +export function collectClaimableLeafIds(layout: TerminalLayoutLeafClaims): Set { + if (layout.root) { + return new Set(collectLeafIdsInOrder(layout.root)) + } + const boundLeafIds = Object.keys(layout.ptyIdsByLeafId ?? {}) + const provenLeafId = boundLeafIds.length === 1 ? boundLeafIds[0] : layout.activeLeafId + return new Set(boundLeafIds.filter((leafId) => leafId === provenLeafId)) +} diff --git a/src/renderer/src/lib/terminal-pty-pane-owner.ts b/src/renderer/src/lib/terminal-pty-pane-owner.ts index 4a71a3e7dd1..9f5846c788a 100644 --- a/src/renderer/src/lib/terminal-pty-pane-owner.ts +++ b/src/renderer/src/lib/terminal-pty-pane-owner.ts @@ -1,4 +1,4 @@ -import { collectLeafIdsInOrder } from '@/components/terminal-pane/terminal-layout-leaf-ids' +import { collectOwnedLeafIds } from '@/components/terminal-pane/terminal-layout-leaf-claims' import type { AppState } from '@/store/types' /** No `tabsByWorktree`: ownership is tab-keyed, so no worktree key participates. */ @@ -21,12 +21,7 @@ export type TerminalPtyPaneOwnerOptions = { preferTabId?: string } -/** - * The leaf a tab's layout binds to `ptyId`, or null. - * - * A binding whose leaf has left the tree reattaches nothing, so it must not outrank a live - * pane (#13098). Rootless layouts bind their sole pane off-tree and keep counting. - */ +/** The leaf a tab's layout binds to `ptyId`, or null when no leaf it owns holds that binding. */ function findLayoutBoundLeafId( state: TerminalPtyPaneOwnerState, tabId: string, @@ -36,9 +31,9 @@ function findLayoutBoundLeafId( if (!layout?.ptyIdsByLeafId) { return null } - const leafIdsInTree = layout.root ? new Set(collectLeafIdsInOrder(layout.root)) : null + const ownedLeafIds = collectOwnedLeafIds(layout) for (const [leafId, boundPtyId] of Object.entries(layout.ptyIdsByLeafId)) { - if (boundPtyId === ptyId && (!leafIdsInTree || leafIdsInTree.has(leafId))) { + if (boundPtyId === ptyId && ownedLeafIds.has(leafId)) { return leafId } } diff --git a/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.test.ts b/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.test.ts index 36689dd64d5..d83f84beda9 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.test.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.test.ts @@ -11,6 +11,7 @@ const SINGLE_TAB = 'tab-single' const SHARED_LEAF = '10cb5648-8a54-41c0-a6a4-ef0028d93599' const OTHER_LEAF = 'df8913c9-fd8a-420a-a7d6-17daf0ed30f0' const ELSEWHERE_LEAF = '96cdf7ea-9c83-4ba5-a41b-c425955e6606' +const CARRIER_LEAF = 'b7f0e2a1-4c6d-4f8a-9b2e-5d3c1a0f7e64' const SHARED_PTY = 'wt-1@@shared' function tab(id: string, sortOrder: number, createdAt = 1_000 + sortOrder): TerminalTab { @@ -188,19 +189,58 @@ describe('resolveDuplicateTerminalLayoutBindings', () => { }) it('ignores a binding whose pane already left the tree', () => { - const ghost: TerminalLayoutSnapshot = { - root: null, - activeLeafId: null, + // A rooted layout whose map still names a leaf its tree dropped: the binding reattaches + // nothing, so it must not outrank the pane that still mounts the pty. + const stranded: TerminalLayoutSnapshot = { + root: { type: 'leaf', leafId: CARRIER_LEAF }, + activeLeafId: CARRIER_LEAF, expandedLeafId: null, ptyIdsByLeafId: { [ELSEWHERE_LEAF]: SHARED_PTY } } - const healed = heal({ [SINGLE_TAB]: ghost, [SPLIT_TAB]: splitLayout() }, [ + const healed = heal({ [SINGLE_TAB]: stranded, [SPLIT_TAB]: splitLayout() }, [ tab(SINGLE_TAB, 0), tab(SPLIT_TAB, 1) ]) - // The ghost reattaches nothing, so it must not outrank the pane that still mounts the pty. expect(healed[SPLIT_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + expect(healed[SINGLE_TAB]).toBe(stranded) + }) + + it('heals a rootless layout, which binds its sole pane off-tree', () => { + // Ownership resolution counts these bindings, so the self-heal has to as well, or a + // duplicate the resolver can see is one hydration can never repair. + const rootless: TerminalLayoutSnapshot = { + root: null, + activeLeafId: SHARED_LEAF, + expandedLeafId: null, + ptyIdsByLeafId: { [SHARED_LEAF]: SHARED_PTY } + } + const healed = heal({ [SPLIT_TAB]: splitLayout(), [SINGLE_TAB]: rootless }, [ + tab(SPLIT_TAB, 0), + tab(SINGLE_TAB, 1) + ]) + + expect(healed[SPLIT_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + expect(healed[SINGLE_TAB]!.ptyIdsByLeafId).toEqual({}) + expect(healed[SINGLE_TAB]!.root).toBeNull() + }) + + it('refuses a rootless layout a second, never-pruned binding it cannot prove', () => { + // Only the sole off-tree pane, or the one activeLeafId names, proves a rootless claim. + // Claiming the rest would evict the row that really owns the pty, which is #13098. + const overreaching: TerminalLayoutSnapshot = { + root: null, + activeLeafId: ELSEWHERE_LEAF, + expandedLeafId: null, + ptyIdsByLeafId: { [ELSEWHERE_LEAF]: 'wt-1@@own', [SHARED_LEAF]: SHARED_PTY } + } + const healed = heal({ [SINGLE_TAB]: overreaching, [SPLIT_TAB]: splitLayout() }, [ + tab(SINGLE_TAB, 0), + tab(SPLIT_TAB, 1) + ]) + + expect(healed[SPLIT_TAB]!.ptyIdsByLeafId?.[SHARED_LEAF]).toBe(SHARED_PTY) + expect(healed[SINGLE_TAB]).toBe(overreaching) }) it('returns the same object when no tab collides', () => { diff --git a/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.ts b/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.ts index fd1547551f0..2aa70e553cf 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-layout-duplicate-bindings.ts @@ -1,10 +1,8 @@ import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/terminal-tab-types' import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' import { detachTerminalLayoutLeaf } from '@/components/terminal-pane/terminal-layout-leaf-detach' -import { - collectLeafIdsInOrder, - resolvePtyBoundActiveLeafId -} from '@/components/terminal-pane/terminal-layout-leaf-ids' +import { resolvePtyBoundActiveLeafId } from '@/components/terminal-pane/terminal-layout-leaf-ids' +import { collectClaimableLeafIds } from '@/components/terminal-pane/terminal-layout-leaf-claims' type TerminalLayoutOwnerRanking = { canonicalTabIds: ReadonlySet @@ -47,18 +45,19 @@ function compareOwnerTabIds(a: string, b: string, ranking: TerminalLayoutOwnerRa return a < b ? -1 : a > b ? 1 : 0 } -/** Leaf ids this layout claims as pane identities. A binding off the tree reattaches nothing. */ +// Why the claimable set and not the owned one: a winner here takes a binding away from a loser, +// and a rootless layout's never-pruned map would evict the row that really owns it (#13098). function collectHeldLeafIds(layout: TerminalLayoutSnapshot): string[] { - return [...new Set(collectLeafIdsInOrder(layout.root))] + return [...collectClaimableLeafIds(layout)] } -/** PTY ids this layout can actually reattach: bound to a leaf its own tree still mounts (#13098). */ -function collectMountedPtyIds(layout: TerminalLayoutSnapshot): string[] { - const leafIdsInTree = new Set(collectLeafIdsInOrder(layout.root)) +/** PTY ids this layout may claim, bound to a leaf it proves it holds. */ +function collectClaimablePtyIds(layout: TerminalLayoutSnapshot): string[] { + const claimableLeafIds = collectClaimableLeafIds(layout) return [ ...new Set( Object.entries(layout.ptyIdsByLeafId ?? {}) - .filter(([leafId]) => leafIdsInTree.has(leafId)) + .filter(([leafId]) => claimableLeafIds.has(leafId)) .map(([, ptyId]) => ptyId) ) ] @@ -156,7 +155,7 @@ export function resolveDuplicateTerminalLayoutBindings(args: { return resolveDuplicateHolders( resolveDuplicateHolders(args.layoutsByTabId, ranking, collectHeldLeafIds, surrenderLeafId), ranking, - collectMountedPtyIds, + collectClaimablePtyIds, unbindPtyId ) }