From 3fd291ba83cb08e3bc64ea11168c82e1135f9e3f Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 2 Aug 2026 23:39:59 -0700 Subject: [PATCH] fix(ssh): reattach may never create terminal panes (STA-3077, part 1/5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconnecting an SSH worktree could add terminals the user never opened. `restoreReattachedPtyRuntime` calls `Store.persistPtyBinding`, which is a *creating* write: when the lease's leafId is absent from the durable layout it splits the root and grafts a new leaf, resurrecting a pane as a side effect of reattaching its PTY. Add `persistPtyBinding(args, hostId, { mayCreate: false })`. The predicate already existed inside the function — `terminalMembershipChanged` is set by the four creating branches and only those — so the mode reuses it and the function's own `restoreSession()` rollback rather than duplicating a non-creating writer. Both flush points are guarded, including the legacy non-terminal-leafId early return. A refusal is deliberately non-destructive: it skips `registerPty`, leaves the lease `detached`, and retires nothing. An absence in one durable store can justify "do not create"; it can never justify "destroy" — the local partition is not trustworthy as proof of pane deletion, because writes are dropped for the duration of a reconnect's snapshot apply and a pane can be live on screen having been restored from the remote host. Reorder `persistPtyBinding` before `registerPty`, since `registerPty` publishes a mobile surface and would otherwise create the UI a refusal exists to prevent. A persistence *throw* still binds: under `mayCreate: false` the refusal check precedes both flush points, so reaching a flush proves membership did not change and the pane exists. Part 1 of 5, ordered by blast radius. This commit can only decline to create; it destroys nothing and ships safely on its own. --- src/main/persistence.test.ts | 246 ++++++++++++++++++ src/main/persistence.ts | 23 +- ...elay-session-reconnect-incarnation.test.ts | 27 +- src/main/ssh/ssh-relay-session.ts | 54 ++-- 4 files changed, 324 insertions(+), 26 deletions(-) diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 81faa5f6297..3ee4a49dfcb 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -9140,6 +9140,252 @@ describe('Store', () => { }) }) + // STA-3077 I2: reattach may never create UI. `mayCreate: false` binds only to panes that already + // exist durably, and a refusal is non-destructive — an absence justifies "do not create", never "destroy". + it('refuses every creating branch under mayCreate:false and leaves the session untouched', async () => { + const cases = [ + { + name: 'tab absent', + session: getDefaultWorkspaceSession(), + tabId: 'tab-missing' + }, + { + name: 'layout.root absent', + session: { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })] + }, + terminalLayoutsByTabId: { + tab1: { + root: null, + activeLeafId: null, + expandedLeafId: null, + ptyIdsByLeafId: {} + } + } + }, + tabId: 'tab1' + }, + { + name: 'leaf absent from layout', + session: { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })] + }, + terminalLayoutsByTabId: { + tab1: { + root: { type: 'leaf', leafId: TEST_LEAF_1 }, + activeLeafId: TEST_LEAF_1, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-1' } + } + } + }, + tabId: 'tab1' + }, + { + name: 'layout absent entirely', + session: { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })] + } + }, + tabId: 'tab1' + } + ] + + for (const testCase of cases) { + const store = await createStore() + store.setWorkspaceSession(testCase.session as ReturnType) + const before = structuredClone(store.getWorkspaceSession()) + + const outcome = store.persistPtyBinding( + { + worktreeId: 'wt1', + tabId: testCase.tabId, + leafId: TEST_LEAF_2, + ptyId: 'pty-2', + incarnationId: 'incarnation-2' + }, + undefined, + { mayCreate: false } + ) + + expect(outcome, testCase.name).toBe('refused') + // Deep-equal covers the layout, the tab list, the incarnation map and the tombstone clear at once. + expect(store.getWorkspaceSession(), testCase.name).toEqual(before) + } + }) + + it('binds under mayCreate:false when the pane exists, without bumping topology', async () => { + const store = await createStore() + store.setWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })] + }, + terminalLayoutsByTabId: { + tab1: { + root: { type: 'leaf', leafId: TEST_LEAF_1 }, + activeLeafId: TEST_LEAF_1, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-1' } + } + }, + terminalTopologyRevisionByRepoId: { wt1: 1 } + }) + + const outcome = store.persistPtyBinding( + { worktreeId: 'wt1', tabId: 'tab1', leafId: TEST_LEAF_1, ptyId: 'pty-rebound' }, + undefined, + { mayCreate: false } + ) + + expect(outcome).toBe('bound') + const session = store.getWorkspaceSession() + expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({ + [TEST_LEAF_1]: 'pty-rebound' + }) + // A reattach is not a host-admitted spawn after a retirement, so the revision must not move. + expect(session.terminalTopologyRevisionByRepoId?.wt1).toBe(1) + }) + + it('guards the legacy non-terminal-leafId flush point under mayCreate:false', async () => { + const legacyLeafId = 'pane-legacy-1' + const boundStore = await createStore() + boundStore.setWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })] + } + }) + expect( + boundStore.persistPtyBinding( + { worktreeId: 'wt1', tabId: 'tab1', leafId: legacyLeafId, ptyId: 'pty-2' }, + undefined, + { mayCreate: false } + ) + ).toBe('bound') + + const refusedStore = await createStore() + refusedStore.setWorkspaceSession(getDefaultWorkspaceSession()) + expect( + refusedStore.persistPtyBinding( + { worktreeId: 'wt1', tabId: 'tab-missing', leafId: legacyLeafId, ptyId: 'pty-2' }, + undefined, + { mayCreate: false } + ) + ).toBe('refused') + expect(refusedStore.getWorkspaceSession().tabsByWorktree.wt1).toBeUndefined() + }) + + it('leaves the default creating mode unchanged when mayCreate is omitted', async () => { + // Issue #217 regression gate: the creating behavior is load-bearing for the spawn path. + const store = await createStore() + store.setWorkspaceSession(getDefaultWorkspaceSession()) + + expect( + store.persistPtyBinding({ + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_1, + ptyId: 'pty-1' + }) + ).toBe('bound') + + const session = store.getWorkspaceSession() + expect(session.tabsByWorktree.wt1).toEqual([expect.objectContaining({ id: 'tab1' })]) + expect(session.terminalLayoutsByTabId.tab1.root).toEqual({ + type: 'leaf', + leafId: TEST_LEAF_1 + }) + + expect( + store.persistPtyBinding({ + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_2, + ptyId: 'pty-2' + }) + ).toBe('bound') + expect(store.getWorkspaceSession().terminalLayoutsByTabId.tab1.root).toEqual({ + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: TEST_LEAF_1 }, + second: { type: 'leaf', leafId: TEST_LEAF_2 } + }) + }) + + // The ruling as an executable assertion: no absence, anywhere, may retire a lease. Two earlier + // revisions of the design proposed the destructive version; this is the gate against re-deriving it. + it('never retires a lease on any absence under mayCreate:false', async () => { + const absences = [ + { + name: 'leaf absent from an existing layout', + session: { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })] + }, + terminalLayoutsByTabId: { + tab1: { + root: { type: 'leaf', leafId: TEST_LEAF_1 }, + activeLeafId: TEST_LEAF_1, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-1' } + } + } + }, + tabId: 'tab1' + }, + { + name: 'tab absent from a hydrated worktree', + session: { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })] + } + }, + tabId: 'tab-missing' + }, + { + name: 'worktree with no session entry at all', + session: getDefaultWorkspaceSession(), + tabId: 'tab-missing' + } + ] + + for (const absence of absences) { + const store = await createStore() + store.setWorkspaceSession(absence.session as ReturnType) + store.upsertSshRemotePtyLease({ + targetId: 'target-1', + ptyId: 'pty-2', + worktreeId: 'wt1', + tabId: absence.tabId, + leafId: TEST_LEAF_2, + state: 'detached' + }) + const sessionBefore = structuredClone(store.getWorkspaceSession()) + + const outcome = store.persistPtyBinding( + { worktreeId: 'wt1', tabId: absence.tabId, leafId: TEST_LEAF_2, ptyId: 'pty-2' }, + undefined, + { mayCreate: false } + ) + + expect(outcome, absence.name).toBe('refused') + expect( + store.getSshRemotePtyLeases().find((lease) => lease.ptyId === 'pty-2')?.state, + absence.name + ).toBe('detached') + expect(store.getWorkspaceSession(), absence.name).toEqual(sessionBefore) + } + }) + it('keeps worktree deletion authoritative against stale writes and later same-path reuse', async () => { const store = await createStore() store.setWorktreeMeta('wt1', { displayName: 'Worktree' }) diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 668b2b4d27f..33e0170536f 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -6619,6 +6619,8 @@ export class Store { } // Why: sync-flush the pty binding before pty:spawn returns to close the spawn/persist SIGKILL race (Issue #217). + // `mayCreate: false` binds only to panes that already exist: reattach may never create UI (STA-3077 I2). + // An absence here justifies refusing to create; it never justifies destroying — the caller leaves the lease alone. persistPtyBinding( args: { worktreeId: string @@ -6628,8 +6630,10 @@ export class Store { incarnationId?: string startupCwd?: string }, - hostId?: string | null - ): void { + hostId?: string | null, + options?: { mayCreate?: boolean } + ): 'bound' | 'refused' { + const mayCreate = options?.mayCreate ?? true const resolvedHostId = this.resolveHostId(hostId) const session = this.getWorkspaceSession(resolvedHostId) if (resolvedHostId !== LOCAL_EXECUTION_HOST_ID) { @@ -6702,6 +6706,12 @@ export class Store { } if (!isTerminalLeafId(args.leafId)) { // Why: keep legacy renderer-local pane ids out of durable leaf-keyed layout state after the UUID migration. + // Legacy ids have no durable leaf-keyed layout to prove pane existence, so a minted tab is the only + // creation reachable here — refuse it rather than guess. + if (!mayCreate && terminalMembershipChanged) { + restoreSession() + return 'refused' + } advanceTopologyAfterMembershipChange() try { this.flushOrThrow() @@ -6709,7 +6719,7 @@ export class Store { restoreSession() throw err } - return + return 'bound' } const layout = session.terminalLayoutsByTabId?.[args.tabId] if (layout) { @@ -6750,6 +6760,12 @@ export class Store { } } } + // Why: the clone at sessionBeforeBinding predates the incarnation write, so refusing also rolls back + // terminalPtyIncarnationsByPaneKey and the tombstone clear — we are declining the bind entirely. + if (!mayCreate && terminalMembershipChanged) { + restoreSession() + return 'refused' + } advanceTopologyAfterMembershipChange() try { this.flushOrThrow() @@ -6757,6 +6773,7 @@ export class Store { restoreSession() throw err } + return 'bound' } // ── SSH Targets ──────────────────────────────────────────────────── diff --git a/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts b/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts index c34c4298dd8..ac374428bf5 100644 --- a/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts +++ b/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts @@ -413,13 +413,22 @@ describe('SshRelaySession reconnect incarnation ordering', () => { incarnationId }) expect(runtime.onPtySpawned).not.toHaveBeenCalled() - expect(mockStore.persistPtyBinding).toHaveBeenCalledWith({ - worktreeId: 'worktree-1', - tabId: 'tab-1', - leafId: INCARNATION_LEAF_ID, - ptyId: APP_PTY_ID, - incarnationId - }) + // Reattach binds non-creatively (STA-3077 I2): it may never mint a tab, layout, or leaf. + expect(mockStore.persistPtyBinding).toHaveBeenCalledWith( + { + worktreeId: 'worktree-1', + tabId: 'tab-1', + leafId: INCARNATION_LEAF_ID, + ptyId: APP_PTY_ID, + incarnationId + }, + undefined, + { mayCreate: false } + ) + // The store rules before registerPty publishes a surface, so a refusal creates no UI. + expect(vi.mocked(mockStore.persistPtyBinding).mock.invocationCallOrder[0]).toBeLessThan( + vi.mocked(runtime.registerPty).mock.invocationCallOrder[0]! + ) expect(vi.mocked(mockStore.persistPtyBinding).mock.invocationCallOrder[0]).toBeLessThan( vi.mocked(mockStore.markSshRemotePtyLeasesAttachedAsync).mock.invocationCallOrder[0]! ) @@ -527,7 +536,9 @@ describe('SshRelaySession reconnect incarnation ordering', () => { }) expect(setPtyOwnership).toHaveBeenCalledWith(APP_PTY_ID, 'target-1') expect(mockStore.persistPtyBinding).toHaveBeenCalledWith( - expect.objectContaining({ ptyId: APP_PTY_ID, incarnationId: currentIncarnationId }) + expect.objectContaining({ ptyId: APP_PTY_ID, incarnationId: currentIncarnationId }), + undefined, + { mayCreate: false } ) expect(mockWindow.webContents.send).toHaveBeenCalledWith('pty:replay', { id: APP_PTY_ID, diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 5f97828b635..f5a7302237d 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -2124,15 +2124,21 @@ export class SshRelaySession { return } setPtyOwnership(appPtyId, this.targetId) + let restoreOutcome: 'bound' | 'refused' = 'bound' if (attachResult.incarnationId) { restorePtyIncarnation(appPtyId, attachResult.incarnationId) - this.restoreReattachedPtyRuntime( + restoreOutcome = this.restoreReattachedPtyRuntime( appPtyId, attachResult.incarnationId, activeLeaseByPtyId.get(ptyId) ) } - attachedLeaseIds.add(ptyId) + // Why: the relay attach succeeded, so activation bookkeeping proceeds; only the durable bind was + // refused, and a refused bind must not mark the lease attached. It stays detached and is retried + // next reconnect — retirement needs positive evidence, which an absent pane is not. + if (restoreOutcome === 'bound') { + attachedLeaseIds.add(ptyId) + } pendingReattach.activated = true recoveryActivationLease?.commit() recoveryActivationLease = undefined @@ -2191,31 +2197,49 @@ export class SshRelaySession { this.runtime?.acceptPtyIncarnationForExit(appPtyId, ptyIncarnation) } + // Returns 'refused' when the pane no longer exists durably, so the caller leaves the lease detached. private restoreReattachedPtyRuntime( appPtyId: string, incarnationId: string, lease: SshPtyLease | undefined - ): void { + ): 'bound' | 'refused' { if (lease?.worktreeId && lease.tabId && lease.leafId) { + let outcome: 'bound' | 'refused' + try { + // Why: persist decides first — registerPty publishes a mobile surface, so binding before the + // store rules would create the very UI a refusal exists to prevent. + outcome = this.store.persistPtyBinding( + { + worktreeId: lease.worktreeId, + tabId: lease.tabId, + leafId: lease.leafId, + ptyId: appPtyId, + incarnationId + }, + undefined, + { mayCreate: false } + ) + } catch (error) { + console.error('[ssh-relay-session] Failed to persist reconnect incarnation:', error) + // Why: under mayCreate:false the refusal check precedes both flush points, so a throw proves + // membership did NOT change — the pane exists and only the disk write failed. Keep the live PTY. + outcome = 'bound' + } + if (outcome === 'refused') { + console.log( + `[ssh-relay-session] Reattach refused for ${appPtyId}: pane ${lease.tabId}:${lease.leafId} is not in durable layout; leaving lease detached` + ) + return 'refused' + } this.runtime?.registerPty(appPtyId, lease.worktreeId, this.targetId, { tabId: lease.tabId, leafId: lease.leafId, incarnationId }) - try { - this.store.persistPtyBinding({ - worktreeId: lease.worktreeId, - tabId: lease.tabId, - leafId: lease.leafId, - ptyId: appPtyId, - incarnationId - }) - } catch (error) { - console.error('[ssh-relay-session] Failed to persist reconnect incarnation:', error) - } - return + return 'bound' } this.runtime?.onPtySpawned(appPtyId, incarnationId, { awaitsRegistration: false }) + return 'bound' } private async attachPtyWithRetry(