diff --git a/src/main/ipc/pty-ssh-undelivered-kill.test.ts b/src/main/ipc/pty-ssh-undelivered-kill.test.ts index 32f4da8356f..2d92ffa1859 100644 --- a/src/main/ipc/pty-ssh-undelivered-kill.test.ts +++ b/src/main/ipc/pty-ssh-undelivered-kill.test.ts @@ -356,8 +356,8 @@ describe('undelivered SSH stops', () => { } }) - // No incarnation means no fence, and an unfenced order can only be discarded or guessed at. - it('records nothing when the PTY incarnation was never learned', async () => { + // A legacy id with no incarnation has no fence, and an unfenced order can only be guessed at. + it('records nothing for a legacy id whose PTY incarnation was never learned', async () => { const store = createKillStore() registerSshPtyProvider( 'ssh-1', @@ -378,6 +378,44 @@ describe('undelivered SSH stops', () => { } }) + // A `pty2:` id is epoch-scoped, so it names one process even when the incarnation was never + // learned: an offline close across a relaunch still owes the kill. + it('records the stop for an epoch-scoped id whose incarnation was never learned', () => { + const store = createKillStore() + const ptyId = 'ssh:ssh-1@@pty2:epoch-a:4' + setPtyOwnership(ptyId, 'ssh-1') + const { kill } = install(store) + + try { + expect(kill(ptyId)).toBe(false) + expect(store.recordSshRemotePtyKillIntent).toHaveBeenCalledWith('ssh-1', 'pty2:epoch-a:4', { + requestedAt: expect.any(Number), + attempts: 0 + }) + } finally { + deletePtyOwnership(ptyId) + } + }) + + // One recorder: the explicit-close receipt promises the retry for an epoch-scoped id too. + it("records an explicit close's unconfirmed stop for an epoch-scoped id with no incarnation", () => { + const store = createKillStore() + const ptyId = 'ssh:ssh-1@@pty2:epoch-b:5' + setPtyOwnership(ptyId, 'ssh-1') + const { recordUnconfirmedStop } = install(store) + + try { + expect(recordUnconfirmedStop(ptyId)).toBe(true) + expect(store.recordSshRemotePtyKillIntent).toHaveBeenCalledTimes(1) + expect(store.recordSshRemotePtyKillIntent).toHaveBeenCalledWith('ssh-1', 'pty2:epoch-b:5', { + requestedAt: expect.any(Number), + attempts: 0 + }) + } finally { + deletePtyOwnership(ptyId) + } + }) + // An explicit close records its order when the stop goes unconfirmed, before the follow-up kill, // so its receipt can promise the reconnect retry from the record itself. it("records an explicit close's unconfirmed stop and says so", () => { diff --git a/src/main/ipc/pty/runtime/undelivered-ssh-kill.ts b/src/main/ipc/pty/runtime/undelivered-ssh-kill.ts index 00d23478eaa..45b80ea1f9e 100644 --- a/src/main/ipc/pty/runtime/undelivered-ssh-kill.ts +++ b/src/main/ipc/pty/runtime/undelivered-ssh-kill.ts @@ -2,6 +2,7 @@ import type { Store } from '../../../persistence' import { parseAppSshPtyId } from '../../../providers/ssh-pty-id' import { ptyIncarnationById, ptyOwnership } from '../provider/ownership-state' import { getRelayPtyId } from '../provider/registry' +import { isEpochScopedRelayPtyId } from '../../../../shared/ssh-pending-pty-kill' export type UndeliveredSshPtyKill = { store: Store | undefined @@ -26,9 +27,9 @@ export type UndeliveredSshPtyKill = { * - **reversible**: see above. * - **no `connectionId`**: a local PTY's owner is this process, so a failed kill has no later host * to ask; there is nothing to replay against. - * - **no incarnation**: the replay fence is the host-minted PTY incarnation, and a relay renumbers - * from `pty-1` on every start. An order we could never safely aim can only be discarded later, - * or worse, guessed at. + * - **no incarnation on a legacy id**: a legacy relay renumbers from `pty-1` on every start, so + * without the host-minted incarnation the order could never be safely aimed. A `pty2:` id is + * epoch-scoped and names one process, so it is recorded without one. * - **an id naming another connection**: `getRelayPtyId` throws on those, and this runs inside * promise `.catch` handlers where that would surface as an unhandled rejection. */ export function recordUndeliveredSshPtyKill(args: UndeliveredSshPtyKill): boolean { @@ -36,19 +37,19 @@ export function recordUndeliveredSshPtyKill(args: UndeliveredSshPtyKill): boolea if (!store || !connectionId || args.reversible) { return false } - const incarnationId = args.incarnationId ?? ptyIncarnationById.get(ptyId) - if (!incarnationId) { - return false - } let relayPtyId: string try { relayPtyId = getRelayPtyId(connectionId, ptyId) } catch { return false } + const incarnationId = args.incarnationId ?? ptyIncarnationById.get(ptyId) + if (!incarnationId && !isEpochScopedRelayPtyId(relayPtyId)) { + return false + } store.recordSshRemotePtyKillIntent(connectionId, relayPtyId, { requestedAt: args.now ?? Date.now(), - incarnationId, + ...(incarnationId ? { incarnationId } : {}), attempts: 0 }) return true diff --git a/src/main/ipc/session.ts b/src/main/ipc/session.ts index cfc60e7ade6..f1f0c1b14e1 100644 --- a/src/main/ipc/session.ts +++ b/src/main/ipc/session.ts @@ -33,12 +33,17 @@ export function registerSessionHandlers(store: Store, runtime: OrcaRuntimeServic // Why: a renderer save cannot shrink membership main owns, so each close commits it explicitly. ipcMain.handle( 'session:close-terminal-surface', - (_event, args: { worktreeId?: unknown; target?: unknown } | undefined) => { + (_event, args: { worktreeId?: unknown; target?: unknown; reason?: unknown } | undefined) => { const target = parseTerminalSurfaceCloseTarget(args?.target) if (typeof args?.worktreeId !== 'string' || !target) { throw new Error('invalid_terminal_surface') } - return runtime.closeTerminalSurfaceFromRenderer(args.worktreeId, target) + // Why only these two: main alone closes a tab for its process exit. + return runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: args.worktreeId, + target, + reason: args.reason === 'cleanup' ? 'cleanup' : 'user' + }) } ) diff --git a/src/main/persistence-ssh-pending-pty-kill.test.ts b/src/main/persistence-ssh-pending-pty-kill.test.ts index 1d9c665c56f..19298b3288f 100644 --- a/src/main/persistence-ssh-pending-pty-kill.test.ts +++ b/src/main/persistence-ssh-pending-pty-kill.test.ts @@ -50,6 +50,19 @@ describe('Store SSH pending PTY kills', () => { ]) }) + it('keeps an epoch-scoped intent with no incarnation across a restart, and drops a legacy one', async () => { + const store = await createStore() + for (const ptyId of ['pty2:epoch-a:1', 'pty-2']) { + store.recordSshRemotePtyKillIntent('ssh-1', ptyId, { requestedAt: NOW, attempts: 0 }) + } + store.flush() + + const reloaded = await createStore() + expect(reloaded.getSshRemotePtyKillIntents('ssh-1', NOW)).toEqual([ + { ptyId: 'pty2:epoch-a:1', intent: { requestedAt: NOW, attempts: 0 } } + ]) + }) + // A kill issued while the provider was already unregistered writes no lease of its own, and that // offline close is the case most likely to strand a remote shell. it('records an intent for a PTY that has no lease row yet', async () => { diff --git a/src/main/persistence/leasing-ssh-ptys/ssh-normalization.ts b/src/main/persistence/leasing-ssh-ptys/ssh-normalization.ts index 58b0cd0473a..a23bd421dd5 100644 --- a/src/main/persistence/leasing-ssh-ptys/ssh-normalization.ts +++ b/src/main/persistence/leasing-ssh-ptys/ssh-normalization.ts @@ -61,7 +61,7 @@ export function normalizeSshRemotePtyLease(value: unknown): SshRemotePtyLease | return null } const now = Date.now() - const pendingKill = normalizeSshPendingPtyKill(raw.pendingKill) + const pendingKill = normalizeSshPendingPtyKill(raw.pendingKill, raw.ptyId) return { targetId: raw.targetId, ptyId: raw.ptyId, diff --git a/src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts b/src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts index 52d1c51e9f2..a499ba10253 100644 --- a/src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts +++ b/src/main/persistence/leasing-ssh-ptys/ssh-pty-kill-intent-operations.ts @@ -126,7 +126,7 @@ export function recordSshRemotePtyKillIntent( const prior = existing.pendingKill // Same incarnation means a repeated close; a recycled relay id starts a new intent lifetime. existing.pendingKill = - prior?.incarnationId === intent.incarnationId + prior && prior.incarnationId === intent.incarnationId ? { ...intent, requestedAt: Math.min(prior.requestedAt, now), diff --git a/src/main/persistence/loading-store/pty-binding-persistence.ts b/src/main/persistence/loading-store/pty-binding-persistence.ts index f902ff7d252..c90544f7502 100644 --- a/src/main/persistence/loading-store/pty-binding-persistence.ts +++ b/src/main/persistence/loading-store/pty-binding-persistence.ts @@ -92,7 +92,10 @@ export class PtyBindingPersistenceOperations { const paneKey = `${args.tabId}:${args.leafId}` const bindingWorktreeId = args.expectedSourceBinding?.worktreeId ?? args.worktreeId const session = sessions.getWorkspaceSession(resolvedHostId) - if (ptyBindingIsRefused(args, session, bindingWorktreeId, paneKey)) { + const partitions = sessions + .getWorkspaceSessionHostIds() + .map((hostId) => sessions.getWorkspaceSession(hostId)) + if (ptyBindingIsRefused(args, session, bindingWorktreeId, paneKey, partitions)) { outcome = 'refused' return { value: false, persist: false } } diff --git a/src/main/persistence/loading-store/pty-binding-refusals.ts b/src/main/persistence/loading-store/pty-binding-refusals.ts index 3c61056bfca..636e69bd936 100644 --- a/src/main/persistence/loading-store/pty-binding-refusals.ts +++ b/src/main/persistence/loading-store/pty-binding-refusals.ts @@ -1,3 +1,4 @@ +import { hasClosedTerminalTabRecord } from '../../../shared/closed-terminal-tab-tombstones' import { isTerminalLeafId } from '../../../shared/stable-pane-id' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { layoutContainsLeafId } from '../restoring-sessions/terminal-layout-normalization' @@ -13,7 +14,7 @@ export type PtyBindingRefusalRequest = { } /** - * The four fences a binding must clear before anything is mutated, so a refusal leaves nothing + * The five fences a binding must clear before anything is mutated, so a refusal leaves nothing * half-written. Order matters: every `false` here is returned before the write path or the * fast lane can run, which is what the relay's lease expiry and the stable-owner throw rely on. */ @@ -21,7 +22,10 @@ export function ptyBindingIsRefused( args: PtyBindingRefusalRequest, session: WorkspaceSessionState, bindingWorktreeId: string, - paneKey: string + paneKey: string, + /** Every host partition: a close is recorded where the tab lived, which need not be where this + * binding lands (a relay reattach binds into `local`). */ + partitions: readonly WorkspaceSessionState[] = [session] ): boolean { if (args.expectedSourceBinding) { const expected = args.expectedSourceBinding @@ -56,6 +60,19 @@ export function ptyBindingIsRefused( return true } } + const existingTab = session.tabsByWorktree?.[bindingWorktreeId]?.find( + (candidate) => candidate.id === args.tabId + ) + // Why: a closed tab's spawn can commit after the close, even after a crash and relaunch; tab + // ids are uuids, so a recorded id is never a new tab. + if ( + !existingTab && + partitions.some((partition) => + hasClosedTerminalTabRecord(partition.closedTerminalTabTombstonesByTabId, args.tabId) + ) + ) { + return true + } // Mirrors the four creating branches of the write path — mint a tab, mint a root leaf, split // the root and graft a leaf, mint a layout — each of which sets `terminalMembershipChanged`. if ( @@ -65,9 +82,6 @@ export function ptyBindingIsRefused( return true } if (args.mayCreate === false) { - const existingTab = session.tabsByWorktree?.[bindingWorktreeId]?.find( - (candidate) => candidate.id === args.tabId - ) const existingLayout = session.terminalLayoutsByTabId?.[args.tabId] const wouldCreateTopology = !existingTab || diff --git a/src/main/persistence/loading-store/terminal-close-async-durability.test.ts b/src/main/persistence/loading-store/terminal-close-async-durability.test.ts index 4e74eea38ae..090d2340932 100644 --- a/src/main/persistence/loading-store/terminal-close-async-durability.test.ts +++ b/src/main/persistence/loading-store/terminal-close-async-durability.test.ts @@ -98,9 +98,11 @@ it.each([ const before = persistedLeafIds() const gate = authority.pause() let acknowledged = false - const closing = runtime.closeTerminalSurfaceFromRenderer(binding.worktreeId, target).then(() => { - acknowledged = true - }) + const closing = runtime + .closeTerminalSurfaceFromRenderer({ worktreeId: binding.worktreeId, target: target }) + .then(() => { + acknowledged = true + }) try { await Promise.race([gate.started.promise, closing]) expect(acknowledged).toBe(false) @@ -140,9 +142,12 @@ it('keeps a renderer close when its durable write fails', async () => { const { authority, runtime, persistedLeafIds, liveLeafIds } = await closeFixture() vi.spyOn(console, 'error').mockImplementation(() => {}) const gate = authority.pause() - const closing = runtime.closeTerminalSurfaceFromRenderer(binding.worktreeId, { - kind: 'tab', - tabId: binding.tabId + const closing = runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: binding.worktreeId, + target: { + kind: 'tab', + tabId: binding.tabId + } }) await Promise.race([gate.started.promise, closing]) gate.finish.reject(new Error('close disk refused')) @@ -165,10 +170,13 @@ it('commits nothing for a pane that restarted while its close waited for the wri ptyId: 'restarted-pty', incarnationId: 'ffffffff-ffff-4fff-8fff-ffffffffffff' }) - const closing = runtime.closeTerminalSurfaceFromRenderer(binding.worktreeId, { - kind: 'pane', - tabId: binding.tabId, - leafId: binding.leafId + const closing = runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: binding.worktreeId, + target: { + kind: 'pane', + tabId: binding.tabId, + leafId: binding.leafId + } }) gate.finish.resolve() await earlierWrite @@ -193,9 +201,12 @@ it('commits a renderer tab close whose split pane bound while the close waited f incarnationId: 'eeeeeeee-eeee-4eee-8eee-eeeeeeeeeeee', expectedSourceBinding: binding }) - const closing = runtime.closeTerminalSurfaceFromRenderer(binding.worktreeId, { - kind: 'tab', - tabId: binding.tabId + const closing = runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: binding.worktreeId, + target: { + kind: 'tab', + tabId: binding.tabId + } }) gate.finish.resolve() await earlierWrite @@ -209,4 +220,8 @@ it('commits a renderer tab close whose split pane bound while the close waited f .getWorkspaceSession() .tabsByWorktree[binding.worktreeId]?.some((tab) => tab.id === binding.tabId) ).toBe(false) + // Why: skipping the owner fence for the layout owner's own close must not skip its record. + expect(store.getWorkspaceSession().closedTerminalTabTombstonesByTabId?.[binding.tabId]).toEqual( + expect.objectContaining({ worktreeId: binding.worktreeId, reason: 'user' }) + ) }) diff --git a/src/main/persistence/runtime-authored-workspace-session-fields.ts b/src/main/persistence/runtime-authored-workspace-session-fields.ts index e87b47cd535..80a9d30b532 100644 --- a/src/main/persistence/runtime-authored-workspace-session-fields.ts +++ b/src/main/persistence/runtime-authored-workspace-session-fields.ts @@ -4,9 +4,9 @@ import type { WorkspaceSessionState } from '../../shared/workspace-session-state * Keeps session fields the renderer persist snapshot does not author across a full write. * * A session write replaces the stored object. Zustand-built payloads omit runtime-owned - * client-hosted pages, and they omit-when-empty the write-once default-terminal-tab marker. - * Without this, those slices vanish on the next desktop write and only show up missing after - * restart. + * client-hosted pages and terminal close records, and they omit-when-empty the write-once + * default-terminal-tab marker. Without this, those slices vanish on the next desktop write and + * only show up missing after restart. * * Callers do not opt in: the Store applies this inside setLocalWorkspaceSession and * setHostWorkspaceSession, so the before-unload stage path inherits it too. @@ -44,6 +44,16 @@ export function preserveRuntimeAuthoredWorkspaceSessionFields( clientHostedBrowserPagesByWorktree: prior.clientHostedBrowserPagesByWorktree } } + // Why: close records are main's alone (its close transaction); a renderer save never carries them. + if ( + next.closedTerminalTabTombstonesByTabId === undefined && + prior?.closedTerminalTabTombstonesByTabId !== undefined + ) { + result = { + ...result, + closedTerminalTabTombstonesByTabId: prior.closedTerminalTabTombstonesByTabId + } + } // Why union: persist snapshots omit this write-once map (empty Zustand slice, omit-when-empty // payload). Treating omission as "never applied" re-spawns default terminals on every attach. return unionWriteOnceDefaultTerminalTabsApplied(result, prior) diff --git a/src/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.ts b/src/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.ts index e2e1e2337fc..73d0e44499f 100644 --- a/src/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.ts +++ b/src/main/runtime/orca-runtime-build-headless-mobile-session-browser-tabs.ts @@ -12,7 +12,9 @@ import type { Tab } from '../../shared/tab-types' import { resolveTerminalCloseTarget, terminalSurfaceCloseMutation, - type PaneCloseResolution + type PaneCloseResolution, + type RendererTerminalClose, + type TerminalSurfaceCloseOptions } from './terminal-surface-close' import type { TerminalPaneCloseTarget, @@ -113,8 +115,7 @@ export class OrcaRuntimeWithBuildHeadlessMobileSessionBrowserTabs extends OrcaRu protected async closeTerminalSurface( worktreeId: string, target: TerminalSurfaceCloseTarget, - // closedByLayoutOwner goes away with D1, once main owns the terminal layout. - options: { allowMissing?: boolean; force?: boolean; closedByLayoutOwner?: boolean } = {} + options: TerminalSurfaceCloseOptions = {} ): Promise { const store = this.store if (!store?.getWorkspaceSession || !store.setWorkspaceSession || !store.runDurableMutation) { @@ -153,8 +154,8 @@ export class OrcaRuntimeWithBuildHeadlessMobileSessionBrowserTabs extends OrcaRu } /** The desktop renderer's close intent: it already guarded, removed and killed; this only reports. */ - async closeTerminalSurfaceFromRenderer(worktreeId: string, target: TerminalSurfaceCloseTarget) { - const options = { allowMissing: true, force: true, closedByLayoutOwner: true } + async closeTerminalSurfaceFromRenderer({ worktreeId, target, reason }: RendererTerminalClose) { + const options = { allowMissing: true, force: true, closedByLayoutOwner: true, reason } await this.closeTerminalSurface(worktreeId, target, options) } diff --git a/src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.ts b/src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.ts index 7239f66d316..5ef34314e42 100644 --- a/src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.ts +++ b/src/main/runtime/orca-runtime-close-headless-mobile-terminal-tab.ts @@ -11,6 +11,7 @@ import { buildHeadlessMobileSessionTabGroups } from './mobile-session-layout-pro import { appendRetiredTerminalSurfaceProofs } from './mobile-session-terminal-retirement-proof' import type { RuntimePtyWorktreeRecord } from './runtime-terminal-state-records' import type { TerminalPaneLayoutNode } from '../../shared/terminal-tab-types' +import type { RuntimeSessionTabCloseReason } from '../../shared/runtime-session-contracts' export class OrcaRuntimeWithCloseHeadlessMobileTerminalTab extends OrcaRuntimeWithCloseStructuredAgentSessionTab { protected async closeHeadlessMobileTerminalTab( @@ -22,6 +23,7 @@ export class OrcaRuntimeWithCloseHeadlessMobileTerminalTab extends OrcaRuntimeWi killPtys?: boolean authorizedPty?: RuntimePtyWorktreeRecord force?: boolean + reason?: RuntimeSessionTabCloseReason } = {} ): Promise { const closedParentTabId = tab.parentTabId @@ -42,7 +44,8 @@ export class OrcaRuntimeWithCloseHeadlessMobileTerminalTab extends OrcaRuntimeWi { kind: 'tab', tabId: closedParentTabId }, { allowMissing: options.allowMissingPersistedTab, - force: options.force + force: options.force, + reason: options.reason } ) if (!acknowledgeRetirement().matches) { diff --git a/src/main/runtime/orca-runtime-close-mobile-session-tab.ts b/src/main/runtime/orca-runtime-close-mobile-session-tab.ts index 7f161f81dc5..3f7047c6487 100644 --- a/src/main/runtime/orca-runtime-close-mobile-session-tab.ts +++ b/src/main/runtime/orca-runtime-close-mobile-session-tab.ts @@ -172,6 +172,7 @@ export class OrcaRuntimeWithCloseMobileSessionTab extends OrcaRuntimeWithRefuseU await this.closeHeadlessMobileTerminalTab(worktreeId, snapshot, tab, { allowMissingPersistedTab: Boolean(ptyCloseAuthority), force: options.force, + reason: options.reason, killPtys: options.localPtyTeardownOwnedExternally !== true && (options.reason === undefined || options.reason === 'user'), diff --git a/src/main/runtime/orca-runtime-terminal-close-no-widening.test.ts b/src/main/runtime/orca-runtime-terminal-close-no-widening.test.ts index 2def6219d29..1b98c67b8be 100644 --- a/src/main/runtime/orca-runtime-terminal-close-no-widening.test.ts +++ b/src/main/runtime/orca-runtime-terminal-close-no-widening.test.ts @@ -175,10 +175,13 @@ function arrange(entry: Entry, condition: Condition): CloseContinuityHarness { async function closePane(entry: Entry, harness: CloseContinuityHarness): Promise { if (entry === 'renderer') { - await harness.runtime.closeTerminalSurfaceFromRenderer(WORKTREE_ID, { - kind: 'pane', - tabId: TAB_ID, - leafId: LEAF_ID + await harness.runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { + kind: 'pane', + tabId: TAB_ID, + leafId: LEAF_ID + } }) return } diff --git a/src/main/runtime/orca-runtime-terminal-close-records.test.ts b/src/main/runtime/orca-runtime-terminal-close-records.test.ts new file mode 100644 index 00000000000..f05ac78957a --- /dev/null +++ b/src/main/runtime/orca-runtime-terminal-close-records.test.ts @@ -0,0 +1,309 @@ +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { getDefaultWorkspaceSession } from '../../shared/constants' +import { + CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS, + MAX_CLOSED_TERMINAL_TAB_TOMBSTONES +} from '../../shared/closed-terminal-tab-tombstones' +import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' +import { Store } from '../persistence/loading-store/store' +import { closeTestStores, createSqliteTestStore } from '../persistence-test-harness' +import { OrcaRuntimeService } from './orca-runtime' +import { + LEAF_ID, + REPO_ID, + TAB_ID, + WORKTREE_ID, + WORKTREE_PATH, + makeSession +} from './__fixtures__/orca-runtime-terminal-close-continuity-fixtures' +import { advanceTerminalTopologyRevision } from './workspace-session-terminal-membership-authority' + +const SSH_REPO_ID = 'ssh-repo' +const SSH_HOST_ID = 'ssh:target-1' +const SSH_WORKTREE_ID = `${SSH_REPO_ID}::/srv/app` +const LATE_TAB_ID = '6f0a5c8e-2b1d-4c3e-9f7a-1d2e3f4a5b6c' + +const directories: string[] = [] +afterEach(async () => { + await closeTestStores() + for (const directory of directories.splice(0)) { + rmSync(directory, { recursive: true, force: true }) + } +}) + +function createPersistedRuntime(session: WorkspaceSessionState = makeSession()) { + const directory = mkdtempSync(join(tmpdir(), 'orca-close-records-')) + directories.push(directory) + const dataFile = join(directory, 'orca-data.json') + const store = createSqliteTestStore(Store, { dataFile }) + store.addRepo({ + id: REPO_ID, + path: WORKTREE_PATH, + displayName: 'Fixture', + badgeColor: 'gray', + addedAt: 1 + }) + store.addRepo({ + id: SSH_REPO_ID, + path: '/srv/app', + displayName: 'Remote', + badgeColor: 'gray', + addedAt: 1, + connectionId: 'target-1' + }) + store.setWorkspaceSession(advanceTerminalTopologyRevision(session, WORKTREE_ID)) + store.flushOrThrow() + return { + store, + runtime: new OrcaRuntimeService(store), + reload: async () => { + store.flush() + store.freezeWrites() + await store.waitForPendingWrite() + return createSqliteTestStore(Store, { dataFile }) + } + } +} + +/** A renderer save: membership as the renderer holds it, and no close records, which it never sends. */ +function rendererSave(session: WorkspaceSessionState): WorkspaceSessionState { + const { + terminalTopologyRevisionByRepoId: _hostPrivate, + closedTerminalTabTombstonesByTabId: _mainOwned, + ...rendererView + } = session + return { ...rendererView, tabsByWorktree: { [WORKTREE_ID]: [] }, terminalLayoutsByTabId: {} } +} + +describe('close records', () => { + it.each(['user', 'cleanup'] as const)( + 'records a %s close in main and keeps it across a renderer save and a reload', + async (reason) => { + const { store, runtime, reload } = createPersistedRuntime() + + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: TAB_ID }, + reason + }) + store.setWorkspaceSession(rendererSave(store.getWorkspaceSession())) + + const reloaded = (await reload()).getWorkspaceSession() + expect(reloaded.tabsByWorktree[WORKTREE_ID]).toEqual([]) + expect(reloaded.closedTerminalTabTombstonesByTabId?.[TAB_ID]).toEqual({ + closedAt: expect.any(Number), + worktreeId: WORKTREE_ID, + reason + }) + } + ) + + // The store keeps main's map only when a write omits it; main's own writes carry it and win. + it("keeps main's own record writes across later store writes", async () => { + const { store, runtime } = createPersistedRuntime() + + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: TAB_ID } + }) + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: LATE_TAB_ID } + }) + store.setWorkspaceSession(rendererSave(store.getWorkspaceSession())) + + expect( + Object.keys(store.getWorkspaceSession().closedTerminalTabTombstonesByTabId ?? {}).sort() + ).toEqual([LATE_TAB_ID, TAB_ID].sort()) + }) + + it("keeps a close's first reason when the renderer's echo closes the same tab again", async () => { + const { store, runtime } = createPersistedRuntime() + + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: LATE_TAB_ID }, + reason: 'cleanup' + }) + const first = store.getWorkspaceSession().closedTerminalTabTombstonesByTabId?.[LATE_TAB_ID] + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: LATE_TAB_ID }, + reason: 'user' + }) + + expect(first?.reason).toBe('cleanup') + expect(store.getWorkspaceSession().closedTerminalTabTombstonesByTabId?.[LATE_TAB_ID]).toEqual( + first + ) + }) + + it('lets a close that removes a listed tab replace a record that tab already had', async () => { + const earlier = Date.now() - 24 * 60 * 60 * 1000 + const { store, runtime } = createPersistedRuntime({ + ...makeSession(), + closedTerminalTabTombstonesByTabId: { + [TAB_ID]: { closedAt: earlier, worktreeId: WORKTREE_ID, reason: 'cleanup' } + } + }) + + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: TAB_ID }, + reason: 'user' + }) + + const record = store.getWorkspaceSession().closedTerminalTabTombstonesByTabId?.[TAB_ID] + expect(store.getWorkspaceSession().tabsByWorktree[WORKTREE_ID]).toEqual([]) + expect(record?.reason).toBe('user') + expect(record?.closedAt).toBeGreaterThan(earlier) + }) + + it('admits a late spawn for a tab whose close record is past the TTL', async () => { + const expired = { + [LATE_TAB_ID]: { + closedAt: Date.now() - CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS - 60_000, + worktreeId: WORKTREE_ID, + reason: 'user' as const + } + } + const { store } = createPersistedRuntime({ + ...makeSession(), + closedTerminalTabTombstonesByTabId: expired + }) + + expect( + await store.persistPtyBinding({ + worktreeId: WORKTREE_ID, + tabId: LATE_TAB_ID, + leafId: LEAF_ID, + ptyId: 'late-pty', + incarnationId: 'late-incarnation' + }) + ).toBe(true) + }) + + it('records nothing for a split pane close, which leaves its tab open', async () => { + const { store, runtime } = createPersistedRuntime() + + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { + kind: 'pane', + tabId: 'unknown-tab', + leafId: LEAF_ID + } + }) + + expect(store.getWorkspaceSession().closedTerminalTabTombstonesByTabId).toBeUndefined() + }) + + // Why: only a resolved tab close records; a pane target never widens here, even on the last pane. + it("records nothing for a pane close aimed at its tab's only pane", async () => { + const { store, runtime } = createPersistedRuntime() + + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { + kind: 'pane', + tabId: TAB_ID, + leafId: LEAF_ID + } + }) + + expect(store.getWorkspaceSession().closedTerminalTabTombstonesByTabId).toBeUndefined() + expect(store.getWorkspaceSession().tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id)).toEqual([ + TAB_ID + ]) + }) + + // The durable half of refusing a late graft: the close lands while the tab's spawn is in + // flight, so main has never listed the tab, and the spawn commits only after a relaunch. + it('refuses a closed tab whose spawn commits after a crash and reload', async () => { + const { runtime, reload } = createPersistedRuntime() + + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: LATE_TAB_ID } + }) + const relaunched = await reload() + + expect( + await relaunched.persistPtyBinding({ + worktreeId: WORKTREE_ID, + tabId: LATE_TAB_ID, + leafId: LEAF_ID, + ptyId: 'late-pty', + incarnationId: 'late-incarnation' + }) + ).toBe(false) + expect( + relaunched.getWorkspaceSession().tabsByWorktree[WORKTREE_ID]?.map((tab) => tab.id) + ).not.toContain(LATE_TAB_ID) + }) + + it('refuses the graft when the close was recorded in another host partition', async () => { + const { store, runtime } = createPersistedRuntime() + store.setWorkspaceSession( + { ...getDefaultWorkspaceSession(), tabsByWorktree: { [SSH_WORKTREE_ID]: [] } }, + SSH_HOST_ID + ) + + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: SSH_WORKTREE_ID, + target: { + kind: 'tab', + tabId: LATE_TAB_ID + } + }) + + expect( + store.getWorkspaceSession(SSH_HOST_ID).closedTerminalTabTombstonesByTabId?.[LATE_TAB_ID] + ).toBeDefined() + // A relay reattach binds into `local`, not the partition the close was recorded in. + expect( + await store.persistPtyBinding({ + worktreeId: SSH_WORKTREE_ID, + tabId: LATE_TAB_ID, + leafId: LEAF_ID, + ptyId: 'ssh:target-1@@pty2:epoch:1', + incarnationId: 'relay-incarnation' + }) + ).toBe(false) + }) + + it('prunes each host partition alone, so local churn evicts no SSH record', async () => { + const { store, runtime } = createPersistedRuntime() + store.setWorkspaceSession( + { ...getDefaultWorkspaceSession(), tabsByWorktree: { [SSH_WORKTREE_ID]: [] } }, + SSH_HOST_ID + ) + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: SSH_WORKTREE_ID, + target: { + kind: 'tab', + tabId: 'ssh-tab' + } + }) + + for (let index = 0; index <= MAX_CLOSED_TERMINAL_TAB_TOMBSTONES; index += 1) { + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { + kind: 'tab', + tabId: `local-${index}` + } + }) + } + + expect( + Object.keys(store.getWorkspaceSession().closedTerminalTabTombstonesByTabId ?? {}) + ).toHaveLength(MAX_CLOSED_TERMINAL_TAB_TOMBSTONES) + expect( + store.getWorkspaceSession(SSH_HOST_ID).closedTerminalTabTombstonesByTabId?.['ssh-tab'] + ).toBeDefined() + }) +}) diff --git a/src/main/runtime/orca-runtime-terminal-surface-close.test.ts b/src/main/runtime/orca-runtime-terminal-surface-close.test.ts index df39087c688..c9bd30b7199 100644 --- a/src/main/runtime/orca-runtime-terminal-surface-close.test.ts +++ b/src/main/runtime/orca-runtime-terminal-surface-close.test.ts @@ -123,7 +123,10 @@ describe('renderer close intents', () => { it('keeps a closed tab closed across a renderer save and a reload', async () => { const { store, runtime, reload } = createPersistedRuntime(makeSession()) - await runtime.closeTerminalSurfaceFromRenderer(WORKTREE_ID, { kind: 'tab', tabId: TAB_ID }) + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { kind: 'tab', tabId: TAB_ID } + }) store.setWorkspaceSession(rendererSaveWithout(store.getWorkspaceSession())) expect((await reload()).tabsByWorktree[WORKTREE_ID]).toEqual([]) @@ -132,10 +135,13 @@ describe('renderer close intents', () => { it('keeps a closed split pane closed across a renderer save and a reload', async () => { const { store, runtime, reload } = createPersistedRuntime(splitSession()) - await runtime.closeTerminalSurfaceFromRenderer(WORKTREE_ID, { - kind: 'pane', - tabId: TAB_ID, - leafId: LEAF_ID + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { + kind: 'pane', + tabId: TAB_ID, + leafId: LEAF_ID + } }) store.setWorkspaceSession(rendererSaveWithout(store.getWorkspaceSession(), LEAF_ID)) @@ -158,10 +164,13 @@ describe('renderer close intents', () => { ])('never widens a pane close into a tab close when %s', async (_case, session) => { const { runtime, reload } = createPersistedRuntime(session()) - await runtime.closeTerminalSurfaceFromRenderer(WORKTREE_ID, { - kind: 'pane', - tabId: TAB_ID, - leafId: LEAF_ID + await runtime.closeTerminalSurfaceFromRenderer({ + worktreeId: WORKTREE_ID, + target: { + kind: 'pane', + tabId: TAB_ID, + leafId: LEAF_ID + } }) expect((await reload()).tabsByWorktree[WORKTREE_ID]).toEqual([ diff --git a/src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-07.spec.ts b/src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-07.spec.ts index 4f89b097cbb..b9f605ccf91 100644 --- a/src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-07.spec.ts +++ b/src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-07.spec.ts @@ -146,6 +146,58 @@ describe('OrcaRuntimeService', () => { ).rejects.toThrow('terminal_topology_conflict') }) + // The client's retirement proofs die with a host restart; the close record does not. + it('refuses to adopt an orphan under a tab the user closed', async () => { + const session = { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { [TEST_WORKTREE_ID]: [] }, + terminalTopologyRevisionByRepoId: { [TEST_REPO_ID]: 7 }, + closedTerminalTabTombstonesByTabId: { + 'tab-closed': { + closedAt: Date.now(), + worktreeId: TEST_WORKTREE_ID, + reason: 'user' as const + } + } + } + const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession(session) + const runtime = new OrcaRuntimeService( + withDurableRuntimeStore({ ...runtimeStore, flushOrThrow: vi.fn() }) + ) + runtime.setPtyController({ + write: () => true, + kill: () => true, + getForegroundProcess: async () => null, + listProcesses: async () => [ + { + id: 'pty-closed', + incarnationId: 'inc-closed', + terminalHandle: 'term_closed', + title: 'shell', + cwd: TEST_WORKTREE_PATH, + worktreeId: TEST_WORKTREE_ID, + wslDistro: null + } + ] + }) + + await expect( + runtime.adoptTerminalOrphans({ + worktree: `id:${TEST_WORKTREE_ID}`, + expectedTopologyRevision: 7, + claims: [ + { + terminal: 'term_closed', + ptyId: 'pty-closed', + incarnationId: 'inc-closed', + tabId: 'tab-closed', + leafId: HEADLESS_LEAF_ID + } + ] + }) + ).rejects.toThrow('terminal_orphan_surface_retired') + }) + it('keeps orphaned list and show writability aligned with the send gate', async () => { const { runtimeStore } = makeRuntimeStoreWithWorkspaceSession({ ...getDefaultWorkspaceSession(), diff --git a/src/main/runtime/runtime-terminal-orphan-adoption.ts b/src/main/runtime/runtime-terminal-orphan-adoption.ts index 24df1cc1cbf..51e56d0b508 100644 --- a/src/main/runtime/runtime-terminal-orphan-adoption.ts +++ b/src/main/runtime/runtime-terminal-orphan-adoption.ts @@ -5,6 +5,7 @@ import type { RuntimeTerminalOrphanAdoptionResult } from '../../shared/runtime-types' import { makePaneKey } from '../../shared/stable-pane-id' +import { hasClosedTerminalTabRecord } from '../../shared/closed-terminal-tab-tombstones' import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' import { terminalOrphanExecutionOwnersEqual } from './terminal-orphan-owner' import type { TerminalWorkspaceLaunchScope } from './runtime-legacy-worker-terminal-recovery-types' @@ -185,7 +186,11 @@ export async function adoptRuntimeTerminalOrphansFromInventory(args: { ) { throw new Error('terminal_orphan_surface_occupied') } - if (session.terminalSurfaceTombstonesByPaneKey?.[paneKey]) { + // Why the close record too: it outlives a host restart, which the client's retirement proofs do not. + if ( + session.terminalSurfaceTombstonesByPaneKey?.[paneKey] || + hasClosedTerminalTabRecord(session.closedTerminalTabTombstonesByTabId, claim.tabId) + ) { throw new Error('terminal_orphan_surface_retired') } for (const snapshot of ports.getMobileSnapshots()) { diff --git a/src/main/runtime/terminal-surface-close.ts b/src/main/runtime/terminal-surface-close.ts index 65756f12e4f..0feee12402c 100644 --- a/src/main/runtime/terminal-surface-close.ts +++ b/src/main/runtime/terminal-surface-close.ts @@ -1,4 +1,9 @@ +import type { RuntimeSessionTabCloseReason } from '../../shared/runtime-session-contracts' import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' +import { + hasClosedTerminalTabRecord, + recordClosedTerminalTabTombstone +} from '../../shared/closed-terminal-tab-tombstones' import type { TerminalLayoutSnapshot, TerminalPaneLayoutNode @@ -91,12 +96,18 @@ export type TerminalSurfaceCloseResult = WorkspaceSessionTerminalTabCloseResult * The membership half of every explicit terminal close: removes a tab, or one pane of a split * tab, and advances the repo's topology revision so a stale renderer save cannot restore it. * A pane close only ever removes that pane; the resolution says why anything else was a no-op. + * Every tab close is recorded in the session it was removed from, the owning host's partition. */ export function closeTerminalSurfaceInWorkspaceSession( session: WorkspaceSessionState, worktreeId: string, target: TerminalSurfaceCloseTarget, - options: { force?: boolean; paneIncarnationId?: string } = {} + options: { + force?: boolean + paneIncarnationId?: string + reason: RuntimeSessionTabCloseReason + now?: number + } ): TerminalSurfaceCloseResult { if (target.kind === 'pane') { const layout = session.terminalLayoutsByTabId[target.tabId] @@ -128,20 +139,48 @@ export function closeTerminalSurfaceInWorkspaceSession( const result = closeTerminalTabInWorkspaceSession(session, worktreeId, target.tabId, { force: options.force }) + if (result.pinned) { + return { ...result, resolution: 'tab' } + } + // Why a tab this session never listed is still recorded: its spawn may commit later and graft it. + const recorded: WorkspaceSessionState = { + ...result.session, + closedTerminalTabTombstonesByTabId: recordClosedTerminalTabTombstone( + result.session.closedTerminalTabTombstonesByTabId, + target.tabId, + { worktreeId, reason: options.reason }, + options.now ?? Date.now() + ) + } return { ...result, - ...(result.closed - ? { session: advanceTerminalTopologyRevision(result.session, worktreeId) } - : {}), + session: result.closed ? advanceTerminalTopologyRevision(recorded, worktreeId) : recorded, resolution: 'tab' } } +/** How one close commits; `reason` is recorded only when the close resolves to the whole tab. */ +export type TerminalSurfaceCloseOptions = { + allowMissing?: boolean + force?: boolean + reason?: RuntimeSessionTabCloseReason + /** The desktop renderer's own close: its layout owner already removed the tab. Goes away with + * D1, once main owns the terminal layout. */ + closedByLayoutOwner?: boolean +} + +/** The desktop renderer's close intent as its IPC delivers it; main alone passes 'pty-exit'. */ +export type RendererTerminalClose = { + worktreeId: string + target: TerminalSurfaceCloseTarget + reason?: 'user' | 'cleanup' +} + /** What one close's durable mutation reads and writes, resolved when the writer admits it. */ export type TerminalSurfaceCloseCommit = { worktreeId: string target: TerminalSurfaceCloseTarget - options: { allowMissing?: boolean; force?: boolean } + options: TerminalSurfaceCloseOptions /** The session as it was when the close was asked, before the writer admitted it. */ requestedSession: WorkspaceSessionState | null | undefined /** The tab's owner identity still matches the one the close was asked against. */ @@ -175,12 +214,23 @@ export function terminalSurfaceCloseMutation( } const result = closeTerminalSurfaceInWorkspaceSession(session, commit.worktreeId, target, { force: commit.options.force, - paneIncarnationId + paneIncarnationId, + reason: commit.options.reason ?? 'user' }) if (result.pinned) { return { value: new Error('terminal_tab_pinned'), persist: false } } if (!result.closed) { + // Why: a tab this session never listed still records its close, so its late spawn is refused; + // an existing record (an echo of that close) needs no second write. + if ( + commit.options.allowMissing && + target.kind === 'tab' && + !hasClosedTerminalTabRecord(session.closedTerminalTabTombstonesByTabId, target.tabId) + ) { + commit.setSession(result.session, hostId) + return { value: undefined } + } return { value: commit.options.allowMissing ? undefined : new Error('tab_not_found'), persist: false diff --git a/src/main/ssh/ssh-pending-pty-kill-replay.test.ts b/src/main/ssh/ssh-pending-pty-kill-replay.test.ts index 7f682c6c1b6..95296aa6434 100644 --- a/src/main/ssh/ssh-pending-pty-kill-replay.test.ts +++ b/src/main/ssh/ssh-pending-pty-kill-replay.test.ts @@ -134,6 +134,28 @@ describe('replayPendingSshPtyKills', () => { expect(terminated).toEqual(['pty-1']) }) + // An offline close across a relaunch never learned the incarnation; the epoch-scoped id is the fence. + it('replays an epoch-scoped stop that carries no incarnation', async () => { + const relayPtyId = 'pty2:epoch-a:4' + const { store, cleared, terminated } = createStoreStub([ + { ptyId: relayPtyId, intent: { requestedAt: NOW, attempts: 0 } } + ]) + const { provider, shutdown } = createProviderStub([{ relayPtyId, incarnationId: 'inc-live' }]) + await replayPendingSshPtyKills({ + targetId: TARGET, + store, + provider, + shouldContinue: () => true, + now: () => NOW + }) + expect(shutdown).toHaveBeenCalledWith(`ssh:ssh-1@@${relayPtyId}`, { + immediate: true, + expectedIncarnationId: undefined + }) + expect(cleared).toEqual([relayPtyId]) + expect(terminated).toEqual([relayPtyId]) + }) + // #16970: a redeployed relay renumbers from pty-1, so this id now names someone else's shell. it('refuses to kill a recycled relay id and expires the lease that named it', async () => { const { store, cleared, terminated, expired, recycled } = createStoreStub([ 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 3c405cd5dff..ce223ae0b9c 100644 --- a/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts +++ b/src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts @@ -458,13 +458,21 @@ describe('SshRelaySession reconnect incarnation ordering', () => { }) it.each([ - { relay: 'current', incarnationId: 'inc-1', tombstonePartition: 'local' }, - { relay: 'current', incarnationId: 'inc-1', tombstonePartition: 'host' }, - { relay: 'legacy', incarnationId: undefined, tombstonePartition: 'local' }, - { relay: 'legacy', incarnationId: undefined, tombstonePartition: 'host' } + { relay: 'current', incarnationId: 'inc-1', tombstonePartition: 'local', retiredBy: 'surface' }, + { relay: 'current', incarnationId: 'inc-1', tombstonePartition: 'host', retiredBy: 'surface' }, + { + relay: 'legacy', + incarnationId: undefined, + tombstonePartition: 'local', + retiredBy: 'surface' + }, + { relay: 'legacy', incarnationId: undefined, tombstonePartition: 'host', retiredBy: 'surface' }, + // A closed tab whose pane is in no tab: the close record is the backstop. + { relay: 'current', incarnationId: 'inc-1', tombstonePartition: 'local', retiredBy: 'close' }, + { relay: 'current', incarnationId: 'inc-1', tombstonePartition: 'host', retiredBy: 'close' } ])( - 'suppresses a $tombstonePartition-partition retired surface from a $relay relay', - async ({ incarnationId, tombstonePartition }) => { + 'suppresses a $tombstonePartition-partition $retiredBy retirement from a $relay relay', + async ({ incarnationId, tombstonePartition, retiredBy }) => { const { mockConn, mockStore, mockPortForward, getMainWindow, mockWindow } = createMockDeps() const attachForReconnect = vi.fn().mockResolvedValue({ ...(incarnationId ? { incarnationId } : {}), @@ -514,10 +522,16 @@ describe('SshRelaySession reconnect incarnation ordering', () => { } } } + const closedTab: ReturnType = { + ...getDefaultWorkspaceSession(), + closedTerminalTabTombstonesByTabId: { [tabId]: { closedAt: Date.now(), worktreeId } } + } vi.mocked(mockStore.getWorkspaceSession).mockImplementation((hostId) => - (hostId ? 'host' : 'local') === tombstonePartition - ? sessionWithTombstone - : getDefaultWorkspaceSession() + (hostId ? 'host' : 'local') !== tombstonePartition + ? getDefaultWorkspaceSession() + : retiredBy === 'close' + ? closedTab + : sessionWithTombstone ) const runtime = { registerPty: vi.fn(), onPtySpawned: vi.fn() } const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 76b7bfb238c..e7911691261 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -119,6 +119,7 @@ import { } from '../../shared/ssh-ai-vault-relay' import { isTerminalLeafId, makePaneKey } from '../../shared/stable-pane-id' import { isValidTerminalTabId } from '../../shared/terminal-tab-id' +import { hasClosedTerminalTabRecord } from '../../shared/closed-terminal-tab-tombstones' import { openSshPtyConsumerSession, type OpenSshPtyConsumerSessionOptions, @@ -2752,9 +2753,19 @@ export class SshRelaySession { if (hasLiveCurrentBinding) { return false } - return [lease.tabId, ...currentTabIds] - .filter((tabId) => isValidTerminalTabId(tabId)) - .some(tombstoneMatches) + // Why only when no tab holds the leaf: a pane moved out of the tab before it closed lives on. + const leaseTabId = lease.tabId + const closedByRecord = + currentTabIds.length === 0 && + candidates.some((candidate) => + hasClosedTerminalTabRecord(candidate?.closedTerminalTabTombstonesByTabId, leaseTabId) + ) + return ( + closedByRecord || + [lease.tabId, ...currentTabIds] + .filter((tabId) => isValidTerminalTabId(tabId)) + .some(tombstoneMatches) + ) } private async suppressRetiredReattachedPty( diff --git a/src/preload/api/workspace-session-api.ts b/src/preload/api/workspace-session-api.ts index 434b3795df1..d621e5dc7db 100644 --- a/src/preload/api/workspace-session-api.ts +++ b/src/preload/api/workspace-session-api.ts @@ -24,6 +24,7 @@ export type WorkspaceSessionApi = { closeTerminalSurface: (args: { worktreeId: string target: TerminalSurfaceCloseTarget + reason?: 'user' | 'cleanup' }) => Promise flush: () => Promise readTerminalScrollback: (args: { ref: string }) => string | null diff --git a/src/renderer/src/components/terminal/initial-terminal-structured-launch.test.tsx b/src/renderer/src/components/terminal/initial-terminal-structured-launch.test.tsx index d9799888a2e..734cef0f795 100644 --- a/src/renderer/src/components/terminal/initial-terminal-structured-launch.test.tsx +++ b/src/renderer/src/components/terminal/initial-terminal-structured-launch.test.tsx @@ -6,18 +6,24 @@ import { useTerminalWatcherEffects } from '../use-terminal-watcher-effects' const mocks = vi.hoisted(() => { const storeTabsByWorktree: Record = {} + const storeClosedRecords: Record = {} return { gate: vi.fn(), resume: vi.fn(), authority: 'none', launchStatus: vi.fn((_worktreeId: string, _provider: string): string => 'idle'), createTab: vi.fn(), - storeTabsByWorktree + storeTabsByWorktree, + storeClosedRecords } }) vi.mock('@/store', () => ({ useAppStore: Object.assign(() => mocks.authority, { - getState: () => ({ activeWorktreeId: 'wt-1', tabsByWorktree: mocks.storeTabsByWorktree }) + getState: () => ({ + activeWorktreeId: 'wt-1', + tabsByWorktree: mocks.storeTabsByWorktree, + closedTerminalTabTombstonesByTabId: mocks.storeClosedRecords + }) }) })) vi.mock('@/lib/worktree-agent-activation-gate', () => ({ @@ -46,6 +52,7 @@ afterEach(async () => { vi.clearAllMocks() mocks.authority = 'none' mocks.storeTabsByWorktree = {} + mocks.storeClosedRecords = {} }) function Watcher({ restored = true, hydrated = false, worktreeId = 'wt-1' } = {}): null { @@ -179,11 +186,22 @@ describe('passive terminal seeding retries until a decision applies', () => { const finishGate = deferredGate() root = createRoot(document.createElement('div')) await act(async () => root?.render()) + // closeTab empties the row and records the close in the same store write. mocks.storeTabsByWorktree = { 'wt-1': [] } + mocks.storeClosedRecords = { 'closed-tab': { worktreeId: 'wt-1', closedAt: Date.now() } } await finishGate('empty') expect(mocks.createTab).not.toHaveBeenCalled() }) + it('seeds an empty row that has no close record, which legacy data leaves', async () => { + const finishGate = deferredGate() + root = createRoot(document.createElement('div')) + await act(async () => root?.render()) + mocks.storeTabsByWorktree = { 'wt-1': [] } + await finishGate('empty') + expect(mocks.createTab).toHaveBeenCalledTimes(1) + }) + it('seeds after leaving and returning to a blocked workspace', async () => { mocks.gate.mockResolvedValue('blocked') root = createRoot(document.createElement('div')) diff --git a/src/renderer/src/components/terminal/initial-terminal-wiring.test.ts b/src/renderer/src/components/terminal/initial-terminal-wiring.test.ts index 8908ecefa40..d56384227cc 100644 --- a/src/renderer/src/components/terminal/initial-terminal-wiring.test.ts +++ b/src/renderer/src/components/terminal/initial-terminal-wiring.test.ts @@ -15,10 +15,10 @@ function readSource(relativePath: string): string { describe('Terminal auto-create wiring', () => { const source = readSource(TERMINAL_PATH) - it('derives the tombstone from the active worktree row at decision time', () => { + it('derives the tombstone from the active worktree row and its close records at decision time', () => { // Why the live store: a render-captured row goes stale while the activation check runs. expect(source).toMatch( - /Object\.hasOwn\(\s*useAppStore\.getState\(\)\.tabsByWorktree,\s*activeWorktreeId\s*\)/ + /isTerminalWorkspaceEmptiedOnPurpose\(\s*useAppStore\.getState\(\),\s*activeWorktreeId\s*\)/ ) }) diff --git a/src/renderer/src/components/terminal/initial-terminal.ts b/src/renderer/src/components/terminal/initial-terminal.ts index b895bf0cbd5..515effce58c 100644 --- a/src/renderer/src/components/terminal/initial-terminal.ts +++ b/src/renderer/src/components/terminal/initial-terminal.ts @@ -2,6 +2,7 @@ export function shouldAutoCreateInitialTerminal( renderableTabCount: number, hasPersistedTerminalState = false ): boolean { - // Why: a missing row means never initialized; an explicit empty row records that the user closed the last terminal. + // Why: desktop callers pass isTerminalWorkspaceEmptiedOnPurpose, so an empty row with no close + // record (legacy data, or a writer that is not a close) still reads as never initialized. return renderableTabCount === 0 && !hasPersistedTerminalState } diff --git a/src/renderer/src/components/use-terminal-watcher-effects.ts b/src/renderer/src/components/use-terminal-watcher-effects.ts index 0de705d9df5..f91c06bd7d7 100644 --- a/src/renderer/src/components/use-terminal-watcher-effects.ts +++ b/src/renderer/src/components/use-terminal-watcher-effects.ts @@ -10,6 +10,7 @@ import { type ParkedTerminalTabWatcherSyncEntry } from './terminal-pane/terminal-parked-tab-watchers' import { useAppStore } from '@/store' +import { isTerminalWorkspaceEmptiedOnPurpose } from '../../../shared/closed-terminal-tab-tombstones' import { gateWorktreeAgentActivation } from '@/lib/worktree-agent-activation-gate' import { createWorkspaceTerminalHostAuthoritySelector } from '@/lib/workspace-terminal-host-authority' import { getStructuredAgentLaunchStatus } from '@/lib/structured-agent-session-launch' @@ -238,10 +239,10 @@ export function useTerminalWatcherEffects(controller: TerminalWatcherController) } // Why: the activation gate reconciles durable/live agent state first; only an actually empty, never-visited workspace receives a default shell. const { renderableTabCount } = reconcileWorktreeTabModel(activeWorktreeId) - // Why (main): a missing row means never initialized, an explicit empty row means the user - // closed the last terminal. Read at decision time: the row can change while the check runs. - const activeWorktreeHasTerminalState = Object.hasOwn( - useAppStore.getState().tabsByWorktree, + // Why (main): only a workspace emptied by a recorded close stays empty; an empty row with no + // record is unknown and seeds. Read at decision time: the row can change while the check runs. + const activeWorktreeHasTerminalState = isTerminalWorkspaceEmptiedOnPurpose( + useAppStore.getState(), activeWorktreeId ) if (shouldAutoCreateInitialTerminal(renderableTabCount, activeWorktreeHasTerminalState)) { diff --git a/src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts b/src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts index a1e82633f98..57964af2e99 100644 --- a/src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts +++ b/src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts @@ -33,32 +33,28 @@ function sessionState(overrides: Partial = {}): Workspace } as WorkspaceSessionState } +// Records ride on `current` here for brevity; the caller passes the renderer's read-only mirror. function merge( current: WorkspaceSessionState, remote: WorkspaceSessionState, - liveTabs: AppState['tabsByWorktree'] = {}, - remoteRevision?: number + liveTabs: AppState['tabsByWorktree'] = {} ): WorkspaceSessionState { return mergeDirectSshRemoteWorkspaceSession( - current, + { ...current, closedTerminalTabTombstonesByTabId: undefined }, remote, new Set([WORKTREE]), liveTabs, new Set(), undefined, - remoteRevision + current.closedTerminalTabTombstonesByTabId ) } -function tombstone(tabId: string, ackRevision?: number): WorkspaceSessionState { +function tombstone(tabId: string): WorkspaceSessionState { return sessionState({ tabsByWorktree: { [WORKTREE]: [] }, closedTerminalTabTombstonesByTabId: { - [tabId]: { - closedAt: Date.now(), - worktreeId: WORKTREE, - ...(ackRevision ? { ackRevision } : {}) - } + [tabId]: { closedAt: Date.now(), worktreeId: WORKTREE } } }) } @@ -77,7 +73,7 @@ describe('direct-SSH pull merge: closed-tab tombstones', () => { remoteSessionIdsByTabId: { toString: 'session-1' } }) - const merged = merge(current, remote, {}, 2) + const merged = merge(current, remote, {}) expect(merged.tabsByWorktree[WORKTREE]?.map((tab) => tab.id)).toEqual(['toString']) expect(Object.hasOwn(merged.terminalLayoutsByTabId, 'toString')).toBe(true) @@ -96,7 +92,7 @@ describe('direct-SSH pull merge: closed-tab tombstones', () => { }) const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [terminalTab('tab-x')] } }) - const merged = merge(current, remote, {}, 2) + const merged = merge(current, remote, {}) expect(merged.tabsByWorktree[WORKTREE]?.map((tab) => tab.id)).toEqual(['tab-x']) }) @@ -109,12 +105,11 @@ describe('direct-SSH pull merge: closed-tab tombstones', () => { remoteSessionIdsByTabId: { ghost: 'pty-1' } }) - const merged = merge(tombstone('ghost'), remote, { [WORKTREE]: [] }, 3) + const merged = merge(tombstone('ghost'), remote, { [WORKTREE]: [] }) expect(merged.tabsByWorktree[WORKTREE]).toEqual([]) expect(merged.terminalLayoutsByTabId.ghost).toBeUndefined() expect(merged.remoteSessionIdsByTabId?.ghost).toBeUndefined() - expect(merged.closedTerminalTabTombstonesByTabId?.ghost).toBeDefined() }) it('does not re-add a tombstoned tab through the host-unknown branch', () => { @@ -127,7 +122,7 @@ describe('direct-SSH pull merge: closed-tab tombstones', () => { }) const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [] } }) - const merged = merge(current, remote, {}, 3) + const merged = merge(current, remote, {}) expect(merged.tabsByWorktree[WORKTREE]).toEqual([]) }) @@ -143,7 +138,7 @@ describe('direct-SSH pull merge: closed-tab tombstones', () => { activeTabIdByWorktree: { [WORKTREE]: 'ghost' } }) - const merged = merge(tombstone('ghost'), remote, { [WORKTREE]: [] }, 3) + const merged = merge(tombstone('ghost'), remote, { [WORKTREE]: [] }) expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toEqual(['survivor']) }) @@ -156,7 +151,7 @@ describe('direct-SSH pull merge: closed-tab tombstones', () => { }) const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [live] } }) - const merged = merge(current, remote, { [WORKTREE]: [live] }, 3) + const merged = merge(current, remote, { [WORKTREE]: [live] }) expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toEqual(['live']) }) @@ -169,24 +164,20 @@ describe('direct-SSH pull merge: closed-tab tombstones', () => { const current = sessionState({ tabsByWorktree: { [WORKTREE]: [agent, closedElsewhere] } }) const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [agent] } }) - const merged = merge(current, remote, { [WORKTREE]: [agent, closedElsewhere] }, 3) + const merged = merge(current, remote, { [WORKTREE]: [agent, closedElsewhere] }) expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toContain('closed-elsewhere') }) - it('retires the tombstone once a newer snapshot stops listing the tab', () => { - const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [] } }) - - const merged = merge(tombstone('ghost', 3), remote, { [WORKTREE]: [] }, 4) + it('neither retires nor writes back a record once the host stops listing the tab', () => { + // Main alone writes records; an acknowledgement used to delete them here, which made a workspace + // the user emptied read as never initialized as soon as the host caught up. + const current = tombstone('ghost') + const merged = merge(current, sessionState({ tabsByWorktree: { [WORKTREE]: [] } }), { + [WORKTREE]: [] + }) expect(merged.closedTerminalTabTombstonesByTabId).toBeUndefined() - }) - - it('keeps the tombstone when the snapshot omits the worktree entirely', () => { - // A snapshot that says nothing about the worktree — including one whose path never resolved — - // is not evidence the host saw the close. - const merged = merge(tombstone('ghost', 3), sessionState(), {}, 4) - - expect(merged.closedTerminalTabTombstonesByTabId?.ghost).toBeDefined() + expect(current.closedTerminalTabTombstonesByTabId?.ghost).toBeDefined() }) }) diff --git a/src/renderer/src/hooks/remote-workspace-session-merge-closed-terminal-tombstone.test.ts b/src/renderer/src/hooks/remote-workspace-session-merge-closed-terminal-tombstone.test.ts index 3764181e71c..ff817913da4 100644 --- a/src/renderer/src/hooks/remote-workspace-session-merge-closed-terminal-tombstone.test.ts +++ b/src/renderer/src/hooks/remote-workspace-session-merge-closed-terminal-tombstone.test.ts @@ -15,9 +15,10 @@ import { useAppStore } from '@/store' * The closed-last-terminal tombstone across a direct-SSH reconnect, end to end. * * The unit-level rule lives in remote-workspace-session-merge-local-survival.test.ts; this asserts - * what that rule is FOR. `Object.hasOwn(tabsByWorktree, worktreeId)` has to keep meaning one thing - * — the merge dropping the key turned "the user closed the last terminal" into "never - * initialized", and the seeding pass then handed the terminal back on every reconnect. + * what that rule is FOR. An empty row plus a close record has to keep meaning one thing — the + * merge dropping the key, or the re-hydration dropping the record, turns "the user closed the last + * terminal" into "never initialized", and the seeding pass then hands the terminal back on every + * reconnect. */ const initialAppStoreState = useAppStore.getState() @@ -26,9 +27,13 @@ afterEach(() => { useAppStore.setState(initialAppStoreState, true) }) -/** The state a workspace lands in once its last terminal is closed: an explicit empty row. */ +/** The state a workspace lands in once its last terminal is closed: an explicit empty row, and + * the close record the renderer mirrors from main. */ function seedClosedLastTerminal(worktreeId: string): void { - useAppStore.setState({ tabsByWorktree: { [worktreeId]: [] } }) + useAppStore.setState({ + tabsByWorktree: { [worktreeId]: [] }, + closedTerminalTabTombstonesByTabId: { closed: { closedAt: Date.now(), worktreeId } } + }) expect(useAppStore.getState().reconcileWorktreeTabModel(worktreeId).renderableTabCount).toBe(0) } diff --git a/src/renderer/src/hooks/remote-workspace-session-merge.ts b/src/renderer/src/hooks/remote-workspace-session-merge.ts index 964c942e0ad..f8bbb0d5ab1 100644 --- a/src/renderer/src/hooks/remote-workspace-session-merge.ts +++ b/src/renderer/src/hooks/remote-workspace-session-merge.ts @@ -1,6 +1,9 @@ import type { TerminalTab } from '../../../shared/terminal-tab-types' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' -import { reconcileClosedTerminalTabTombstones } from '../../../shared/closed-terminal-tab-tombstones' +import { + hasClosedTerminalTabRecord, + type ClosedTerminalTabTombstonesByTabId +} from '../../../shared/closed-terminal-tab-tombstones' import type { ExecutionHostId } from '../../../shared/execution-host' import { worktreeWorkspaceKey } from '../../../shared/workspace-scope' import { splitWorktreeId } from '../../../shared/worktree/id' @@ -33,7 +36,7 @@ export function mergeDirectSshRemoteWorkspaceSession( liveTabsByWorktree: AppState['tabsByWorktree'], preserveLocalTerminalTabIds: ReadonlySet, replaceExecutionHostId?: ExecutionHostId, - remoteRevision?: number, + closedTabRecords?: ClosedTerminalTabTombstonesByTabId, preserveLocalLayoutTabIds: ReadonlySet = new Set() ): WorkspaceSessionState { // Live tabs across the worktrees this snapshot replaces. Close-suppression consults it so a tab @@ -45,7 +48,7 @@ export function mergeDirectSshRemoteWorkspaceSession( // (terminal-tab-close.ts), so a tombstoned id is not live anywhere; // 2. isSuppressedByClose matches the tombstone's own worktreeId, so it cannot reach another // workspace's tab even if an id somehow recurred; - // 3. only closeReason === 'user' ever writes a tombstone. + // 3. only a tab close writes a record; nothing infers one from a tab's absence. // A final sweep of suppression over the assembled tabsByWorktree — including worktrees // omitTargetWorktrees passes through verbatim — is still deliberately absent. const currentTabsById = new Map( @@ -56,10 +59,10 @@ export function mergeDirectSshRemoteWorkspaceSession( const locallyPreservedTabIds = new Set() const localTabsFor = (worktreeId: string): TerminalTab[] => liveTabsByWorktree[worktreeId] ?? current.tabsByWorktree[worktreeId] ?? [] - // Why presence and not length: an explicit empty row is the record that the user closed the last - // terminal (initial-terminal.ts), and localTabsFor cannot tell it from an absent one. Admitting - // only non-empty rows dropped the key, which reads downstream as "never initialized" and seeds a - // fresh terminal on every reconnect. + // Why presence and not length: an explicit empty row, with a close record for the worktree, is how + // a workspace the user emptied reads (initial-terminal.ts), and localTabsFor cannot tell it from an + // absent one. Admitting only non-empty rows dropped the key, which reads downstream as "never + // initialized" and seeds a fresh terminal on every reconnect. const hasLocalTabsRow = (worktreeId: string): boolean => Object.hasOwn(liveTabsByWorktree, worktreeId) || Object.hasOwn(current.tabsByWorktree, worktreeId) @@ -116,36 +119,13 @@ export function mergeDirectSshRemoteWorkspaceSession( .filter(([tabId]) => remoteKnownTabIds.has(tabId)) .map(([, sessionId]) => sessionId) ) - // The other half of the trade below. Absence still cannot say "closed", so this says it instead: - // a tab THIS client watched the user close, whose close never reached the host. Only ids the user - // closed are ever in here, and only until the host's own snapshot stops listing them. - const closedTerminalTabTombstonesByTabId = reconcileClosedTerminalTabTombstones({ - tombstones: current.closedTerminalTabTombstonesByTabId, - acknowledgedWorktreeIds: new Set( - [...replaceWorktreeIds].filter((worktreeId) => - Object.hasOwn(remote.tabsByWorktree, worktreeId) - ) - ), - hostKnownTabIds: remoteKnownTabIds, - hostRevision: remoteRevision, - now: Date.now() - }) - // Why Object.hasOwn and not `in`: the map is a plain object from Object.fromEntries, so `in` - // answers true for every Object.prototype key on an EMPTY map — a host tab whose id is - // `toString` would be filtered, blocked from the host-unknown branch, and stripped of its layout - // and session id. Tab ids are validated only as non-empty and colon-free, and createTab honours - // caller-supplied id hints, so that id is reachable. This is the one place either direction could - // delete a tab the user never closed. - // Why the worktree comparison: the tombstone already carries the worktree it was closed in, so - // scoping on it makes "this suppression cannot reach another workspace's tab" structural rather - // than a property of where the call sites happen to sit. - // Why a live local tab still overrides its own tombstone: an id that is live here means the - // tombstone is stale (a close undone before it persisted), not a revival. Deleting a live pane is - // the one outcome this whole function exists to avoid. + // The other half of the trade below. Absence still cannot say "closed", so main's close record + // says it instead. Read only here: main alone writes records, and nothing acknowledges them away. + // Scoped to the record's own worktree, so suppression cannot reach another workspace's tab. + // Why a live local tab still overrides its record: an id that is live here means the record is + // stale, not a revival. Deleting a live pane is the one outcome this function exists to avoid. const isSuppressedByClose = (tabId: string, worktreeId: string): boolean => - Object.hasOwn(closedTerminalTabTombstonesByTabId, tabId) && - closedTerminalTabTombstonesByTabId[tabId]?.worktreeId === worktreeId && - !currentTabsById.has(tabId) + hasClosedTerminalTabRecord(closedTabRecords, tabId, worktreeId) && !currentTabsById.has(tabId) // Ids this merge actually suppressed, recorded as it walks the worktrees. The layout and // session-id sweeps below have no worktree in scope, so they consult decisions already made // rather than re-deriving one without the scope that makes it safe. @@ -271,10 +251,9 @@ export function mergeDirectSshRemoteWorkspaceSession( // the workspace or its path did not resolve to a local id, and taking that literally drops the // user onto the home screen while their terminals keep running. So it is only overridden when the // workspace they are standing in demonstrably still exists in the merged result. - // Why presence and not length, the same reading `hasLocalTabsRow` above already gives: an - // explicit empty row is the record that the user closed the last terminal, and a workspace they - // emptied still exists in the merged result. Counting rows sent them to the home screen for - // having closed their last tab. + // Why presence and not length, the same reading `hasLocalTabsRow` above already gives: a + // workspace the user emptied keeps an explicit empty row, so it still exists in the merged + // result. Counting rows sent them to the home screen for having closed their last tab. const localActiveWorkspaceSurvives = current.activeWorktreeId != null && replaceWorktreeIds.has(current.activeWorktreeId) && @@ -348,11 +327,7 @@ export function mergeDirectSshRemoteWorkspaceSession( // tabs. Removal is the worktree-teardown path's job, not a reconnect's. ...current.defaultTerminalTabsAppliedByWorktreeId, ...remote.defaultTerminalTabsAppliedByWorktreeId - }, - closedTerminalTabTombstonesByTabId: - Object.keys(closedTerminalTabTombstonesByTabId).length > 0 - ? closedTerminalTabTombstonesByTabId - : undefined + } } } diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-apply-deferred-session-write.test.ts b/src/renderer/src/hooks/remote-workspace-snapshot-apply-deferred-session-write.test.ts index 00b2e8b0c26..020d7d43c92 100644 --- a/src/renderer/src/hooks/remote-workspace-snapshot-apply-deferred-session-write.test.ts +++ b/src/renderer/src/hooks/remote-workspace-snapshot-apply-deferred-session-write.test.ts @@ -213,7 +213,6 @@ describe('session writes deferred by a direct-SSH apply', () => { expect(persist, 'the close made during the apply never reached disk').toHaveBeenCalledTimes(1) const patch = persist.mock.calls[0][0].patch - expect(patch.closedTerminalTabTombstonesByTabId?.['tab-b']?.worktreeId).toBe(WORKTREE_B) expect(patch.tabsByWorktree?.[WORKTREE_B]).toEqual([]) expect(patch.tabsByWorktree?.[WORKTREE_A]).toHaveLength(1) } finally { @@ -321,9 +320,7 @@ describe('session writes deferred by a direct-SSH apply', () => { vi.advanceTimersByTime(5_200) expect(persist, 'a clock step back stranded the deferred write').toHaveBeenCalledTimes(1) - expect( - persist.mock.calls[0][0].patch.closedTerminalTabTombstonesByTabId?.['tab-b'] - ).toBeDefined() + expect(persist.mock.calls[0][0].patch.tabsByWorktree?.[WORKTREE_B]).toEqual([]) } finally { cleanup() } diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts b/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts index e304ff17597..5ca493251ec 100644 --- a/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts +++ b/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts @@ -182,7 +182,7 @@ export async function applyDirectSshRemoteWorkspaceSnapshot({ state.tabsByWorktree, currentRecoveryTabIds(state, authority, worktreeIds), toSshExecutionHostId(authority.targetId), - snapshot.revision, + state.closedTerminalTabTombstonesByTabId, new Set( [...worktreeIds].flatMap((id) => (state.tabsByWorktree[id] ?? []) diff --git a/src/renderer/src/lib/session-write-subscriber-deferred-persist.test.ts b/src/renderer/src/lib/session-write-subscriber-deferred-persist.test.ts index df8bf5217d0..2a65442bd18 100644 --- a/src/renderer/src/lib/session-write-subscriber-deferred-persist.test.ts +++ b/src/renderer/src/lib/session-write-subscriber-deferred-persist.test.ts @@ -104,16 +104,12 @@ describe('session write subscriber defers writes across a closed persistence gat expect(persist).toHaveBeenCalledTimes(1) const patch = persist.mock.calls[0][0].patch + // Close records are main's; the renderer's mirror of them is never written back. + expect(patch.closedTerminalTabTombstonesByTabId).toBeUndefined() expect( - patch.closedTerminalTabTombstonesByTabId, - 'the tombstone written during the apply window was discarded, not deferred' - ).toEqual({ - 'closed-during-apply': { - closedAt: expect.any(Number), - worktreeId: 'wt-remote' - } - }) - expect(patch.activeTabId).toBe('still-open-tab') + patch.activeTabId, + 'the mutation made during the apply window was discarded, not deferred' + ).toBe('still-open-tab') expect(patch.activeRepoId).toBe('repo-1') cleanup() }) @@ -198,7 +194,7 @@ describe('session write subscriber defers writes across a closed persistence gat 'target B lost a tab close because target A was mid-apply' ).toEqual([]) expect(patch.tabsByWorktree?.['wt-target-a']).toHaveLength(1) - expect(patch.closedTerminalTabTombstonesByTabId?.['tab-b']?.worktreeId).toBe('wt-target-b') + expect(patch.closedTerminalTabTombstonesByTabId).toBeUndefined() cleanup() }) diff --git a/src/renderer/src/lib/workspace-session-browser-history.test.ts b/src/renderer/src/lib/workspace-session-browser-history.test.ts index 1a9bde2f605..8c6ef564e25 100644 --- a/src/renderer/src/lib/workspace-session-browser-history.test.ts +++ b/src/renderer/src/lib/workspace-session-browser-history.test.ts @@ -34,8 +34,7 @@ function createSnapshot(browserUrlHistory: BrowserHistoryEntry[]): WorkspaceSess worktreesByRepo: {}, lastKnownRelayPtyIdByTabId: {}, lastVisitedAtByWorktreeId: {}, - defaultTerminalTabsAppliedByWorktreeId: {}, - closedTerminalTabTombstonesByTabId: {} + defaultTerminalTabsAppliedByWorktreeId: {} } } diff --git a/src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts b/src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts deleted file mode 100644 index 522b079aff6..00000000000 --- a/src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { describe, expect, it } from 'vitest' -import { buildWorkspaceSessionPatch } from './workspace-session-patch' -import { SESSION_RELEVANT_FIELDS } from './workspace-session' -import { CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS } from '../../../shared/closed-terminal-tab-tombstones' - -// Why browserPagesByWorkspace is stubbed: buildWorkspaceSessionPatch pre-filters staged browser tabs -// before field dispatch, so the minimal fixture has to carry the map it iterates. -function patchFor( - map: Record -): ReturnType { - return buildWorkspaceSessionPatch( - { browserPagesByWorkspace: {}, closedTerminalTabTombstonesByTabId: map } as never, - ['closedTerminalTabTombstonesByTabId'] - ) -} - -describe('closed-tab tombstones in the persisted session', () => { - it('is a session-relevant field, so a close on its own schedules a write', () => { - expect(SESSION_RELEVANT_FIELDS).toContain('closedTerminalTabTombstonesByTabId') - }) - - it('writes live tombstones and drops ones past the TTL', () => { - const now = Date.now() - expect( - patchFor({ - fresh: { closedAt: now, worktreeId: 'repo-1::/srv/app' }, - stale: { - closedAt: now - CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS - 1, - worktreeId: 'repo-1::/srv/app' - } - }).closedTerminalTabTombstonesByTabId - ).toEqual({ fresh: { closedAt: now, worktreeId: 'repo-1::/srv/app' } }) - }) - - it('omits the field entirely once the map empties', () => { - expect(patchFor({}).closedTerminalTabTombstonesByTabId).toBeUndefined() - }) -}) diff --git a/src/renderer/src/lib/workspace-session-closed-tab-tombstones.ts b/src/renderer/src/lib/workspace-session-closed-tab-tombstones.ts deleted file mode 100644 index 48b1cb1a727..00000000000 --- a/src/renderer/src/lib/workspace-session-closed-tab-tombstones.ts +++ /dev/null @@ -1,10 +0,0 @@ -import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' -import { pruneClosedTerminalTabTombstones } from '../../../shared/closed-terminal-tab-tombstones' - -/** Applies the TTL/cap bound on the way to disk, and omits an empty map like its siblings. */ -export function buildPersistedClosedTerminalTabTombstones( - map: WorkspaceSessionState['closedTerminalTabTombstonesByTabId'] -): WorkspaceSessionState['closedTerminalTabTombstonesByTabId'] { - const pruned = pruneClosedTerminalTabTombstones(map, Date.now()) - return Object.keys(pruned).length > 0 ? pruned : undefined -} diff --git a/src/renderer/src/lib/workspace-session-editor-drafts.test.ts b/src/renderer/src/lib/workspace-session-editor-drafts.test.ts index 690e5917fd1..ba639bb4668 100644 --- a/src/renderer/src/lib/workspace-session-editor-drafts.test.ts +++ b/src/renderer/src/lib/workspace-session-editor-drafts.test.ts @@ -36,7 +36,6 @@ function createSnapshot( lastKnownRelayPtyIdByTabId: {}, lastVisitedAtByWorktreeId: {}, defaultTerminalTabsAppliedByWorktreeId: {}, - closedTerminalTabTombstonesByTabId: {}, ...overrides } } diff --git a/src/renderer/src/lib/workspace-session-liveness.test.ts b/src/renderer/src/lib/workspace-session-liveness.test.ts index 5cabd5b52ff..821c059bfa0 100644 --- a/src/renderer/src/lib/workspace-session-liveness.test.ts +++ b/src/renderer/src/lib/workspace-session-liveness.test.ts @@ -35,7 +35,6 @@ function createSnapshot( lastKnownRelayPtyIdByTabId: {}, lastVisitedAtByWorktreeId: {}, defaultTerminalTabsAppliedByWorktreeId: {}, - closedTerminalTabTombstonesByTabId: {}, ...overrides } } diff --git a/src/renderer/src/lib/workspace-session-patch.ts b/src/renderer/src/lib/workspace-session-patch.ts index 4537e1c570b..dc2947918c2 100644 --- a/src/renderer/src/lib/workspace-session-patch.ts +++ b/src/renderer/src/lib/workspace-session-patch.ts @@ -20,7 +20,6 @@ import { withoutStagedBrowserTabs } from './workspace-session-staged-browser-tab import { buildPersistedUnifiedTabSessionData } from './workspace-session-unified-tabs' import { buildLastVisitedAtByWorktreeId } from './workspace-session-focus-recency' import { buildSleepingAgentSessionData } from './workspace-session-sleeping-agents' -import { buildPersistedClosedTerminalTabTombstones } from './workspace-session-closed-tab-tombstones' type SessionRelevantField = keyof WorkspaceSessionSnapshot @@ -193,11 +192,6 @@ export function buildWorkspaceSessionPatch( ? snapshot.defaultTerminalTabsAppliedByWorktreeId : undefined } - if (changed.has('closedTerminalTabTombstonesByTabId')) { - patch.closedTerminalTabTombstonesByTabId = buildPersistedClosedTerminalTabTombstones( - snapshot.closedTerminalTabTombstonesByTabId - ) - } if (changed.has('sleepingAgentSessionsByPaneKey')) { patch.sleepingAgentSessionsByPaneKey = buildSleepingAgentSessionData(snapshot).sleepingAgentSessionsByPaneKey diff --git a/src/renderer/src/lib/workspace-session-relevant-fields.test.ts b/src/renderer/src/lib/workspace-session-relevant-fields.test.ts index ce26d33ec82..9aadf057964 100644 --- a/src/renderer/src/lib/workspace-session-relevant-fields.test.ts +++ b/src/renderer/src/lib/workspace-session-relevant-fields.test.ts @@ -36,7 +36,6 @@ describe('SESSION_RELEVANT_FIELDS', () => { lastKnownRelayPtyIdByTabId: true, lastVisitedAtByWorktreeId: true, defaultTerminalTabsAppliedByWorktreeId: true, - closedTerminalTabTombstonesByTabId: true, sleepingAgentSessionsByPaneKey: true, clientHostedBrowserCloseIntentsByEnvironment: true, pendingReconnectPtyIdByTabId: true, diff --git a/src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts b/src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts index 19050b9ff67..13cc1baf507 100644 --- a/src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts +++ b/src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts @@ -682,8 +682,7 @@ describe('ssh host partition remote-workspace round trip', () => { new Set([WORKTREE_ID]), read.session.tabsByWorktree, new Set(), - SSH_HOST_ID, - 1 + SSH_HOST_ID ) expect(merged.tabsByWorktree[WORKTREE_ID]?.map((entry) => entry.id)).toEqual(['tab-runtime']) @@ -718,8 +717,7 @@ describe('ssh host partition remote-workspace round trip', () => { new Set([WORKTREE_ID]), read.session.tabsByWorktree, new Set(), - SSH_HOST_ID, - 2 + SSH_HOST_ID ) expect(merged.tabsByWorktree[WORKTREE_ID]?.map((entry) => entry.id)).toEqual(['tab-runtime']) diff --git a/src/renderer/src/lib/workspace-session.ts b/src/renderer/src/lib/workspace-session.ts index e35e2194f8e..4bf57f6d82f 100644 --- a/src/renderer/src/lib/workspace-session.ts +++ b/src/renderer/src/lib/workspace-session.ts @@ -11,7 +11,6 @@ import type { OpenFile } from '../store/slices/editor' import { buildPersistedUnifiedTabSessionData } from './workspace-session-unified-tabs' import { buildLastVisitedAtByWorktreeId } from './workspace-session-focus-recency' import { buildSleepingAgentSessionData } from './workspace-session-sleeping-agents' -import { buildPersistedClosedTerminalTabTombstones } from './workspace-session-closed-tab-tombstones' import { buildActiveConnectionIdsAtShutdown } from './workspace-session-reconnect-targets' import { withoutStagedBrowserTabs } from './workspace-session-staged-browser-tabs' import { buildBrowserSessionData } from './workspace-session-browser-tabs' @@ -57,7 +56,6 @@ export type WorkspaceSessionSnapshot = Pick< | 'lastKnownRelayPtyIdByTabId' | 'lastVisitedAtByWorktreeId' | 'defaultTerminalTabsAppliedByWorktreeId' - | 'closedTerminalTabTombstonesByTabId' > & { activeWorkspaceExecutionHostId?: AppState['activeWorkspaceExecutionHostId'] sleepingAgentSessionsByPaneKey?: AppState['sleepingAgentSessionsByPaneKey'] @@ -100,7 +98,6 @@ export const SESSION_RELEVANT_FIELDS = [ 'lastKnownRelayPtyIdByTabId', 'lastVisitedAtByWorktreeId', 'defaultTerminalTabsAppliedByWorktreeId', - 'closedTerminalTabTombstonesByTabId', 'sleepingAgentSessionsByPaneKey', 'clientHostedBrowserCloseIntentsByEnvironment', 'pendingReconnectPtyIdByTabId', @@ -330,9 +327,6 @@ export function buildWorkspaceSessionPayload( Object.keys(snapshot.defaultTerminalTabsAppliedByWorktreeId).length > 0 ? snapshot.defaultTerminalTabsAppliedByWorktreeId : undefined, - closedTerminalTabTombstonesByTabId: buildPersistedClosedTerminalTabTombstones( - snapshot.closedTerminalTabTombstonesByTabId - ), ...buildSleepingAgentSessionData(snapshot), // Why unconditional rather than omit-when-empty: a full write replaces the persisted object, // so an emptied map has to be written as empty or the last replay never sticks. diff --git a/src/renderer/src/lib/worktree-activation-default-tabs.test.ts b/src/renderer/src/lib/worktree-activation-default-tabs.test.ts index 3b990948340..a6e5e5136a8 100644 --- a/src/renderer/src/lib/worktree-activation-default-tabs.test.ts +++ b/src/renderer/src/lib/worktree-activation-default-tabs.test.ts @@ -21,8 +21,11 @@ describe('ensureWorktreeHasInitialTerminal', () => { expect(store.queueTabSetupSplit).not.toHaveBeenCalled() }) - it('does not recreate a terminal after an explicit empty state was persisted', () => { - const store = createMockStore({ tabsByWorktree: { 'wt-1': [] } }) + it('does not recreate a terminal after the user closed the last one', () => { + const store = createMockStore({ + tabsByWorktree: { 'wt-1': [] }, + closedTerminalTabTombstonesByTabId: { closed: { closedAt: Date.now(), worktreeId: 'wt-1' } } + }) ensureWorktreeHasInitialTerminal(store, 'wt-1') diff --git a/src/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.ts b/src/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.ts index 4d2b43e26bd..13cedba66df 100644 --- a/src/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.ts +++ b/src/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.ts @@ -24,9 +24,12 @@ afterEach(() => { }) /** The state a workspace lands in once its last terminal is closed: the row survives as an - * explicit empty list rather than disappearing. */ + * explicit empty list rather than disappearing, beside the close's record. */ function seedClosedLastTerminal(worktreeId: string): void { - useAppStore.setState({ tabsByWorktree: { [worktreeId]: [] } }) + useAppStore.setState({ + tabsByWorktree: { [worktreeId]: [] }, + closedTerminalTabTombstonesByTabId: { closed: { closedAt: Date.now(), worktreeId } } + }) const { renderableTabCount } = useAppStore.getState().reconcileWorktreeTabModel(worktreeId) expect(renderableTabCount).toBe(0) } diff --git a/src/renderer/src/lib/worktree-activation-store-contract.ts b/src/renderer/src/lib/worktree-activation-store-contract.ts index 63ce180ffed..3608c9def1c 100644 --- a/src/renderer/src/lib/worktree-activation-store-contract.ts +++ b/src/renderer/src/lib/worktree-activation-store-contract.ts @@ -8,9 +8,11 @@ import type { } from '../../../shared/agent-session-resume' import type { WorktreeRuntimeOwnerState } from '@/lib/worktree-runtime-owner' import type { AgentStartedTelemetry } from '@/lib/worktree-startup-payload' +import type { ClosedTerminalTabTombstonesByTabId } from '../../../shared/closed-terminal-tab-tombstones' export type WorktreeActivationStore = Partial & { tabsByWorktree: Record + closedTerminalTabTombstonesByTabId?: ClosedTerminalTabTombstonesByTabId defaultTerminalTabsAppliedByWorktreeId: Record createTab: ( worktreeId: string, diff --git a/src/renderer/src/lib/worktree-initial-terminal-close-records.test.ts b/src/renderer/src/lib/worktree-initial-terminal-close-records.test.ts new file mode 100644 index 00000000000..cd563d9cbe7 --- /dev/null +++ b/src/renderer/src/lib/worktree-initial-terminal-close-records.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from 'vitest' +import { ensureWorktreeHasInitialTerminal } from './worktree-initial-terminal-seeding' +import { + createMockStore, + registerWorktreeActivationReset +} from './worktree-activation-test-harness' +import { + CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS, + MAX_CLOSED_TERMINAL_TAB_TOMBSTONES, + recordClosedTerminalTabTombstone, + type ClosedTerminalTabTombstonesByTabId +} from '../../../shared/closed-terminal-tab-tombstones' + +registerWorktreeActivationReset() + +const WT = 'wt-1' + +/** Startup hydration's seeding decision: no reseed opt-in, so only the row and records decide. */ +function seeds( + tabsByWorktree: Record, + closedTerminalTabTombstonesByTabId?: ClosedTerminalTabTombstonesByTabId, + opts?: { reseedEmptiedWorkspace?: boolean } +): boolean { + const store = createMockStore({ tabsByWorktree, closedTerminalTabTombstonesByTabId }) + ensureWorktreeHasInitialTerminal(store, WT, undefined, undefined, undefined, undefined, opts) + return store.createTab.mock.calls.length > 0 +} + +function closedAt(closedAtMs: number): ClosedTerminalTabTombstonesByTabId { + return { 'closed-tab': { closedAt: closedAtMs, worktreeId: WT, reason: 'user' } } +} + +describe('seeding an empty workspace reads close records', () => { + it('an empty row with a close record stays empty', () => { + expect(seeds({ [WT]: [] }, closedAt(Date.now()))).toBe(false) + }) + + it('a legacy empty row with no record seeds', () => { + expect(seeds({ [WT]: [] })).toBe(true) + }) + + it('an empty row whose only record is past its TTL seeds', () => { + expect( + seeds({ [WT]: [] }, closedAt(Date.now() - CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS - 1)) + ).toBe(true) + }) + + it('an empty row whose record the per-host cap evicted seeds', () => { + const now = Date.now() + let records = closedAt(now - 1_000) + for (let index = 0; index < MAX_CLOSED_TERMINAL_TAB_TOMBSTONES; index += 1) { + records = recordClosedTerminalTabTombstone( + records, + `other-${index}`, + { worktreeId: 'wt-other', reason: 'user' }, + now + ) + } + expect(records['closed-tab']).toBeUndefined() + expect(seeds({ [WT]: [] }, records)).toBe(true) + }) + + // Known weakness until every membership shrink is a close: an older close plus an empty row + // written by something that is not a close still reads as emptied on purpose. + it('a stale record from an earlier close suppresses the startup seed; activation reseeds', () => { + const earlierClose = closedAt(Date.now() - 60_000) + expect(seeds({ [WT]: [] }, earlierClose)).toBe(false) + expect(seeds({ [WT]: [] }, earlierClose, { reseedEmptiedWorkspace: true })).toBe(true) + }) +}) diff --git a/src/renderer/src/lib/worktree-initial-terminal-seeding.ts b/src/renderer/src/lib/worktree-initial-terminal-seeding.ts index f9c2844d472..b0a6296b30e 100644 --- a/src/renderer/src/lib/worktree-initial-terminal-seeding.ts +++ b/src/renderer/src/lib/worktree-initial-terminal-seeding.ts @@ -4,6 +4,7 @@ import type { } from '../../../shared/worktree/launch-types' import type { ExecutionHostId } from '../../../shared/execution-host' import { shouldAutoCreateInitialTerminal } from '@/components/terminal/initial-terminal' +import { isTerminalWorkspaceEmptiedOnPurpose } from '../../../shared/closed-terminal-tab-tombstones' import { createSequencedSetupAgentCommands } from '../../../shared/setup-agent-sequencing' import { getSetupRunnerCommandPlatformForPath } from '../../../shared/setup-runner-command' import { agentKindToTuiAgent } from '../../../shared/agent-kind' @@ -174,7 +175,7 @@ export function ensureWorktreeHasInitialTerminal( // terminal is added; and closeTabPreservingPty (use-terminal-pane-lifecycle.ts) skips both // deactivation hooks for pane moves and retirement, where re-seeding is the wanted outcome. const shouldHonourClosedTerminalTombstone = - Object.hasOwn(store.tabsByWorktree, worktreeId) && opts?.reseedEmptiedWorkspace !== true + isTerminalWorkspaceEmptiedOnPurpose(store, worktreeId) && opts?.reseedEmptiedWorkspace !== true // Why: an execution host that has not answered is not a host with no terminals; seeding into that // gap is what adds a tab per launch (STA-4658). Explicit launch work below is a request to create // a terminal now, so it stays ungated. diff --git a/src/renderer/src/store/slices/worktrees/session/worktree-identity-rename-row-worktree-ids.test.ts b/src/renderer/src/store/slices/worktrees/session/worktree-identity-rename-row-worktree-ids.test.ts index 039b1b5ca91..cb6dd89e026 100644 --- a/src/renderer/src/store/slices/worktrees/session/worktree-identity-rename-row-worktree-ids.test.ts +++ b/src/renderer/src/store/slices/worktrees/session/worktree-identity-rename-row-worktree-ids.test.ts @@ -41,7 +41,7 @@ describe('buildWorktreeRenameState value-owned worktree rows', () => { const next = buildWorktreeRenameState( appState({ closedTerminalTabTombstonesByTabId: { - 'tab-1': { closedAt: 5, worktreeId: OLD, ackRevision: 3 }, + 'tab-1': { closedAt: 5, worktreeId: OLD, reason: 'user' }, 'tab-2': { closedAt: 6, worktreeId: OTHER } } }), @@ -49,7 +49,7 @@ describe('buildWorktreeRenameState value-owned worktree rows', () => { NEW ) expect(next.closedTerminalTabTombstonesByTabId).toEqual({ - 'tab-1': { closedAt: 5, worktreeId: NEW, ackRevision: 3 }, + 'tab-1': { closedAt: 5, worktreeId: NEW, reason: 'user' }, 'tab-2': { closedAt: 6, worktreeId: OTHER } }) }) diff --git a/src/renderer/src/store/terminals/terminal-surface-close-intent.ts b/src/renderer/src/store/terminals/terminal-surface-close-intent.ts index 0ee2d5aead4..25393aa8129 100644 --- a/src/renderer/src/store/terminals/terminal-surface-close-intent.ts +++ b/src/renderer/src/store/terminals/terminal-surface-close-intent.ts @@ -4,11 +4,12 @@ import type { TerminalSurfaceCloseTarget } from '../../../../shared/terminal-sur * cannot shrink it, so without this the close would ride on the PTY exit. */ export function commitTerminalSurfaceClose( worktreeId: string, - target: TerminalSurfaceCloseTarget + target: TerminalSurfaceCloseTarget, + reason?: 'user' | 'cleanup' ): void { // Why optional: an older preload can linger through an in-place renderer reload. void globalThis.window?.api?.session - ?.closeTerminalSurface?.({ worktreeId, target }) + ?.closeTerminalSurface?.({ worktreeId, target, ...(reason ? { reason } : {}) }) ?.catch((error: unknown) => { console.warn('[terminal-close] main did not commit the close', error) }) diff --git a/src/renderer/src/store/terminals/terminal-tab-close-intent.test.ts b/src/renderer/src/store/terminals/terminal-tab-close-intent.test.ts index 076de3babb6..61b51c05337 100644 --- a/src/renderer/src/store/terminals/terminal-tab-close-intent.test.ts +++ b/src/renderer/src/store/terminals/terminal-tab-close-intent.test.ts @@ -62,7 +62,8 @@ describe('closeTab close intent', () => { expect(mockApi.pty.kill).toHaveBeenCalledWith(SSH_PTY) expect(closeTerminalSurface).toHaveBeenCalledWith({ worktreeId: SSH_WORKTREE, - target: { kind: 'tab', tabId: 'ssh-tab' } + target: { kind: 'tab', tabId: 'ssh-tab' }, + reason: 'user' }) }) diff --git a/src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts b/src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts index 8a7d5039c8a..9d7820c7bd3 100644 --- a/src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts +++ b/src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts @@ -2,6 +2,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import type * as AgentStatusModule from '@/lib/agent-status' import { createTestStore, makeTab, makeWorktree, seedStore } from '../slices/store-test-helpers' import { createStoreCascadesMockApi } from '../slices/store-cascades-test-harness' +import { mergeDirectSshRemoteWorkspaceSession } from '@/hooks/remote-workspace-session-merge' +import { buildWorkspaceSessionPayload } from '@/lib/workspace-session' +import { getDefaultWorkspaceSession } from '../../../../shared/constants' vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn(), warning: vi.fn() } @@ -43,38 +46,93 @@ function storeWithBothWorktrees(): ReturnType { return store } -describe('closeTab close tombstones', () => { +const closeTerminalSurface = vi.fn() +Object.assign(mockApi, { session: { closeTerminalSurface } }) + +describe('closeTab close-record mirror', () => { beforeEach(() => { vi.clearAllMocks() + closeTerminalSurface.mockResolvedValue(undefined) mockApi.worktrees.updateMeta.mockResolvedValue({}) }) - it('records the closed tab against the worktree it was closed in', () => { + // Why every workspace kind: main records every close it is told about, and the mirror is what + // the seeding predicate reads before the next launch hydrates main's copy. + it.each([ + ['remote-tab', REMOTE_WORKTREE], + ['local-tab', LOCAL_WORKTREE] + ])('mirrors the record main is told to write for %s', (tabId, worktreeId) => { const store = storeWithBothWorktrees() - store.getState().closeTab('remote-tab') + store.getState().closeTab(tabId) - expect(store.getState().closedTerminalTabTombstonesByTabId['remote-tab']).toEqual({ + expect(store.getState().closedTerminalTabTombstonesByTabId[tabId]).toEqual({ closedAt: expect.any(Number), - worktreeId: REMOTE_WORKTREE + worktreeId, + reason: 'user' + }) + expect(closeTerminalSurface).toHaveBeenCalledWith({ + worktreeId, + target: { kind: 'tab', tabId }, + reason: 'user' }) }) - // A tombstone outlives the host's own record, so the only claim it may ever make is "the user - // closed this". Neither of these closes is that claim. - it.each([['pty-exit'], ['cleanup']] as const)('records nothing for a %s close', (reason) => { + it('mirrors a cleanup close with its reason', () => { const store = storeWithBothWorktrees() - store.getState().closeTab('remote-tab', { reason }) + store.getState().closeTab('remote-tab', { reason: 'cleanup' }) - expect(store.getState().closedTerminalTabTombstonesByTabId['remote-tab']).toBeUndefined() + expect(store.getState().closedTerminalTabTombstonesByTabId['remote-tab']?.reason).toBe( + 'cleanup' + ) }) - it('records nothing for a definitively local worktree, which no pull merge ever reads', () => { + // Main alone closes for a process exit; a paired host owns its own records. + it.each([ + ['a pty-exit close', { reason: 'pty-exit' as const }], + ['a host-owned close', { remoteCloseOwnedByHost: true }] + ])('mirrors nothing for %s', (_label, opts) => { const store = storeWithBothWorktrees() - store.getState().closeTab('local-tab') + store.getState().closeTab('remote-tab', opts) expect(store.getState().closedTerminalTabTombstonesByTabId).toEqual({}) + expect(closeTerminalSurface).not.toHaveBeenCalled() + }) + + // The race the mirror exists for: a host pull lands after closeTab and before main has answered + // the close intent (which never resolves here). The host still lists the tab. + it('keeps a closed SSH tab closed through a pull that lands before main answers', () => { + closeTerminalSurface.mockReturnValue(new Promise(() => {})) + const store = storeWithBothWorktrees() + const hostStillListing = { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + [REMOTE_WORKTREE]: [makeTab({ id: 'remote-tab', worktreeId: REMOTE_WORKTREE })] + } + } + const applyPull = (): void => { + const state = store.getState() + const merged = mergeDirectSshRemoteWorkspaceSession( + buildWorkspaceSessionPayload(state), + hostStillListing, + new Set([REMOTE_WORKTREE]), + state.tabsByWorktree, + new Set(), + undefined, + state.closedTerminalTabTombstonesByTabId + ) + const replaceWorkspaceKeys = [REMOTE_WORKTREE] + store.getState().hydrateWorkspaceSession(merged, { replaceWorkspaceKeys }) + store.getState().hydrateTabsSession(merged, { replaceWorkspaceKeys }) + } + + store.getState().closeTab('remote-tab') + applyPull() + // A second pull: once re-added, a live local tab would override its record for good. + applyPull() + + expect(store.getState().tabsByWorktree[REMOTE_WORKTREE]?.map((tab) => tab.id) ?? []).toEqual([]) }) }) diff --git a/src/renderer/src/store/terminals/terminal-tab-close.ts b/src/renderer/src/store/terminals/terminal-tab-close.ts index 529b19ded0b..8227521d2ec 100644 --- a/src/renderer/src/store/terminals/terminal-tab-close.ts +++ b/src/renderer/src/store/terminals/terminal-tab-close.ts @@ -1,6 +1,4 @@ -import { recordClosedTerminalTabTombstone } from '../../../../shared/closed-terminal-tab-tombstones' import type { TerminalTab } from '../../../../shared/terminal-tab-types' -import { getConnectionIdFromState } from '@/lib/connection-owner-resolution' import { sweepRetiredTerminalTabState } from '../slices/retired-terminal-tab-state-sweep' import { getRecentlyClosedTabPosition, @@ -27,7 +25,9 @@ export function createTerminalTabCloseActions( return { closeTab: (tabId, opts) => { const closeReason = opts?.reason ?? 'user' - const retiresSession = closeReason === 'user' || closeReason === 'cleanup' + // Why narrowed separately: main alone records a close for a process exit. + const intentReason = closeReason === 'pty-exit' ? null : closeReason + const retiresSession = intentReason !== null const retirementPlan = opts?.precomputedRetirementPlan?.tabId === tabId ? opts.precomputedRetirementPlan @@ -69,22 +69,19 @@ export function createTerminalTabCloseActions( next[wId] = after } } - // Why `user` and not retiresSession: a tombstone outlives the host's own record, so the only - // thing it may ever say is "the user closed this". A pty-exit close is the process ending, - // and a `cleanup` close retires a tab the app itself created — neither is that claim. - // Why a non-local worktree only: the tombstone is read solely by the direct-SSH pull merge, - // and a definitively local tab would just consume the map's cap. An unresolved repo - // (undefined, not null) still records — losing the tombstone reinstates the resurrection. + // Why mirrored here and never persisted: main records the close through the intent below, + // and the direct-SSH pull merge reads this synchronously, so a pull landing before main + // answered would otherwise re-add the tab. Goes away when that merge moves to main. const nextClosedTombstones = - closeReason === 'user' && - closedWorktreeId && - getConnectionIdFromState(s, closedWorktreeId) !== null - ? recordClosedTerminalTabTombstone( - s.closedTerminalTabTombstonesByTabId, - tabId, - closedWorktreeId, - Date.now() - ) + intentReason && closedWorktreeId && opts?.remoteCloseOwnedByHost !== true + ? { + ...s.closedTerminalTabTombstonesByTabId, + [tabId]: { + closedAt: Date.now(), + worktreeId: closedWorktreeId, + reason: intentReason + } + } : s.closedTerminalTabTombstonesByTabId // Why: only explicit user closes feed the Cmd+Shift+T reopen stack; cleanup/PTY-exit closes must not pollute undo history. const closedPosition = @@ -250,8 +247,8 @@ export function createTerminalTabCloseActions( : {}) } }) - if (retiresSession && closingWorktreeId && opts?.remoteCloseOwnedByHost !== true) { - commitTerminalSurfaceClose(closingWorktreeId, { kind: 'tab', tabId }) + if (intentReason && closingWorktreeId && opts?.remoteCloseOwnedByHost !== true) { + commitTerminalSurfaceClose(closingWorktreeId, { kind: 'tab', tabId }, intentReason) } // Why shared with the paired snapshot apply: every path that removes a tab owes it the same sweep, and a second copy of the list is how one path silently misses a new entry. sweepRetiredTerminalTabState(get(), tabId, closingWorktreeId) diff --git a/src/renderer/src/store/terminals/workspace-terminal-hydration-patch.ts b/src/renderer/src/store/terminals/workspace-terminal-hydration-patch.ts index 6f2e3d47e75..ef53641d0dc 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-hydration-patch.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-hydration-patch.ts @@ -198,7 +198,7 @@ export function targetScopedWorkspaceHydrationPatch( ), ...state.defaultTerminalTabsAppliedByWorktreeId }, - // Why passed through whole: hydration already unioned it with live store state, and the map is + // Why passed through whole: hydration already fell back to live store state, and the map is // keyed by tab id rather than by workspace key so replaceHydratedRecordKeys has nothing to match. closedTerminalTabTombstonesByTabId: hydrated.closedTerminalTabTombstonesByTabId, automaticAgentResumeClaimsByTabId: replaceHydratedRecordKeys( diff --git a/src/renderer/src/store/terminals/workspace-terminal-hydration.ts b/src/renderer/src/store/terminals/workspace-terminal-hydration.ts index db278e05d92..9cfe4e687e8 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-hydration.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-hydration.ts @@ -191,11 +191,10 @@ export function createWorkspaceTerminalHydrationActions( ...session.defaultTerminalTabsAppliedByWorktreeId, ...s.defaultTerminalTabsAppliedByWorktreeId }, - // Why replace and not union: both callers hand over a map they derived from this store - // synchronously (the pull merge) or from disk before the store had one (startup), so there - // is no local tombstone to lose — and a union would resurrect the ones the merge just - // retired on the host's acknowledgement, which is the whole bound on this map. - closedTerminalTabTombstonesByTabId: session.closedTerminalTabTombstonesByTabId ?? {}, + // Why keep the store's when absent: only main's session carries close records, and the + // pull merge's session does not, so re-hydrating from it must not clear the mirror. + closedTerminalTabTombstonesByTabId: + session.closedTerminalTabTombstonesByTabId ?? s.closedTerminalTabTombstonesByTabId, automaticAgentResumeClaimsByTabId: {}, sleepingAgentSessionsByPaneKey, pendingReconnectWorktreeIds, diff --git a/src/shared/closed-terminal-tab-tombstones.test.ts b/src/shared/closed-terminal-tab-tombstones.test.ts index a2db1828ac3..b28c64d9dd9 100644 --- a/src/shared/closed-terminal-tab-tombstones.test.ts +++ b/src/shared/closed-terminal-tab-tombstones.test.ts @@ -2,37 +2,29 @@ import { describe, expect, it } from 'vitest' import { CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS, MAX_CLOSED_TERMINAL_TAB_TOMBSTONES, + closedTerminalTabTombstoneSchema, + hasClosedTerminalTabRecord, + isTerminalWorkspaceEmptiedOnPurpose, pruneClosedTerminalTabTombstones, recordClosedTerminalTabTombstone, - reconcileClosedTerminalTabTombstones, type ClosedTerminalTabTombstonesByTabId } from './closed-terminal-tab-tombstones' const NOW = 1_800_000_000_000 const WT = 'repo-1::/srv/app' -function reconcile( - tombstones: ClosedTerminalTabTombstonesByTabId, - args: { - acknowledgedWorktreeIds?: string[] - hostKnownTabIds?: string[] - hostRevision?: number - } -): ClosedTerminalTabTombstonesByTabId { - return reconcileClosedTerminalTabTombstones({ - tombstones, - acknowledgedWorktreeIds: new Set(args.acknowledgedWorktreeIds ?? [WT]), - hostKnownTabIds: new Set(args.hostKnownTabIds ?? []), - hostRevision: args.hostRevision, - now: NOW +describe('closed terminal tab records', () => { + it('records the closing worktree, reason and time', () => { + expect( + recordClosedTerminalTabTombstone({}, 'tab-1', { worktreeId: WT, reason: 'cleanup' }, NOW) + ).toEqual({ 'tab-1': { closedAt: NOW, worktreeId: WT, reason: 'cleanup' } }) }) -} -describe('closed terminal tab tombstones', () => { - it('records the closing worktree and time', () => { - expect(recordClosedTerminalTabTombstone({}, 'tab-1', WT, NOW)).toEqual({ - 'tab-1': { closedAt: NOW, worktreeId: WT } - }) + it('treats a record past the TTL as absent, even before its partition prunes it', () => { + const stale = { + 'tab-1': { closedAt: NOW - CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS - 1, worktreeId: WT } + } + expect(hasClosedTerminalTabRecord(stale, 'tab-1', undefined, NOW)).toBe(false) }) it('prunes past the TTL and caps at the newest entries', () => { @@ -57,53 +49,46 @@ describe('closed terminal tab tombstones', () => { expect(capped['tab-0']).toBeDefined() expect(capped[`tab-${MAX_CLOSED_TERMINAL_TAB_TOMBSTONES + 9}`]).toBeUndefined() }) -}) -describe('closed terminal tab tombstone acknowledgement', () => { - const tombstones = { 'tab-1': { closedAt: NOW, worktreeId: WT } } - - it('a snapshot carrying no revision acknowledges nothing', () => { - expect(reconcile(tombstones, { hostRevision: undefined })).toEqual(tombstones) - }) - - it('a snapshot with no row for the tombstone worktree acknowledges nothing', () => { - const kept = reconcile(tombstones, { acknowledgedWorktreeIds: [], hostRevision: 9 }) - expect(kept['tab-1']).toEqual({ closedAt: NOW, worktreeId: WT }) - }) - - it('the first omitting snapshot only stamps the revision — it could predate the close', () => { - const kept = reconcile(tombstones, { hostRevision: 4 }) - expect(kept['tab-1']).toEqual({ closedAt: NOW, worktreeId: WT, ackRevision: 4 }) - }) - - it('a strictly newer omitting snapshot retires the tombstone', () => { - const stamped = reconcile(tombstones, { hostRevision: 4 }) - expect(reconcile(stamped, { hostRevision: 5 })['tab-1']).toBeUndefined() - }) - - it('a repeat of the same revision does not retire it', () => { - const stamped = reconcile(tombstones, { hostRevision: 4 }) - expect(reconcile(stamped, { hostRevision: 4 })['tab-1']).toBeDefined() - }) - - it('a host that still lists the tab re-arms the watermark instead of retiring', () => { - const stamped = reconcile(tombstones, { hostRevision: 4 }) - const stillListed = reconcile(stamped, { hostRevision: 7, hostKnownTabIds: ['tab-1'] }) - expect(stillListed['tab-1']).toEqual({ closedAt: NOW, worktreeId: WT, ackRevision: 7 }) - expect(reconcile(stillListed, { hostRevision: 7 })['tab-1']).toBeDefined() - expect(reconcile(stillListed, { hostRevision: 8 })['tab-1']).toBeUndefined() - }) - - it('a host revision that went backwards never retires', () => { - const stamped = reconcile(tombstones, { hostRevision: 40 }) - const rewound = reconcile(stamped, { hostRevision: 1 }) - expect(rewound['tab-1']).toEqual({ closedAt: NOW, worktreeId: WT, ackRevision: 40 }) - }) - - it('a tab listed under a different worktree still counts as known to the host', () => { - const stamped = reconcile(tombstones, { hostRevision: 4 }) + it('reads an older build record as a close, and an unknown reason as no reason', () => { + expect(closedTerminalTabTombstoneSchema.parse({ closedAt: 1, worktreeId: WT })).toEqual({ + closedAt: 1, + worktreeId: WT + }) expect( - reconcile(stamped, { hostRevision: 5, hostKnownTabIds: ['tab-1'] })['tab-1'] - ).toBeDefined() + closedTerminalTabTombstoneSchema.parse({ closedAt: 1, worktreeId: WT, reason: 'later' }) + ).toEqual({ closedAt: 1, worktreeId: WT, reason: undefined }) + }) +}) + +describe('emptied on purpose', () => { + const closed: ClosedTerminalTabTombstonesByTabId = { 'tab-1': { closedAt: NOW, worktreeId: WT } } + + it('an empty row with a close record in that workspace was emptied on purpose', () => { + expect( + isTerminalWorkspaceEmptiedOnPurpose( + { tabsByWorktree: { [WT]: [] }, closedTerminalTabTombstonesByTabId: closed }, + WT + ) + ).toBe(true) + }) + + it('an empty row with no record is unknown, as is a missing row', () => { + expect(isTerminalWorkspaceEmptiedOnPurpose({ tabsByWorktree: { [WT]: [] } }, WT)).toBe(false) + expect( + isTerminalWorkspaceEmptiedOnPurpose( + { tabsByWorktree: {}, closedTerminalTabTombstonesByTabId: closed }, + WT + ) + ).toBe(false) + }) + + it('a record for another workspace does not count', () => { + expect( + isTerminalWorkspaceEmptiedOnPurpose( + { tabsByWorktree: { other: [] }, closedTerminalTabTombstonesByTabId: closed }, + 'other' + ) + ).toBe(false) }) }) diff --git a/src/shared/closed-terminal-tab-tombstones.ts b/src/shared/closed-terminal-tab-tombstones.ts index 3db0a585987..6c7b6f81d49 100644 --- a/src/shared/closed-terminal-tab-tombstones.ts +++ b/src/shared/closed-terminal-tab-tombstones.ts @@ -1,18 +1,23 @@ import { z } from 'zod' +import type { RuntimeSessionTabCloseReason } from './runtime-session-contracts' -/** A client's own record that the user closed a terminal tab. +const TERMINAL_TAB_CLOSE_REASONS = [ + 'user', + 'cleanup', + 'pty-exit' +] as const satisfies readonly RuntimeSessionTabCloseReason[] + +/** Main's record that a terminal tab was closed, written by its close transaction only. * - * Why it must exist: absence alone cannot distinguish "the host was never told" from "the user - * closed it", so the merge keeps the tab — and a `pty.kill` that died on the transport means the - * host keeps listing it forever. This is the close signal that outlives the failed RPC. - * Safe because tab ids are uuids: a tombstoned id never legitimately returns. */ + * Why it must exist: absence alone cannot distinguish "never told" from "closed", so without it a + * host snapshot or a late spawn commit brings the tab back, and an emptied workspace reads as one + * that was never initialized. Safe because tab ids are uuids: a closed id never legitimately + * returns. Nothing acknowledges it away; it dies by TTL or the per-host cap. */ export type ClosedTerminalTabTombstone = { closedAt: number worktreeId: string - /** Newest host revision seen for this tab's scope since the close. Retirement needs a STRICTLY - * newer snapshot that omits the tab, so a pull already in flight when the user closed cannot - * acknowledge a close it predates. */ - ackRevision?: number + /** Absent on records an older build wrote, when only user closes were recorded. */ + reason?: RuntimeSessionTabCloseReason } export type ClosedTerminalTabTombstonesByTabId = Record @@ -22,11 +27,12 @@ export type ClosedTerminalTabTombstonesByTabId = Record, now: number ): ClosedTerminalTabTombstonesByTabId { - return pruneClosedTerminalTabTombstones({ ...map, [tabId]: { closedAt: now, worktreeId } }, now) + return pruneClosedTerminalTabTombstones({ ...map, [tabId]: { ...record, closedAt: now } }, now) } -function maxAckRevision(a: number | undefined, b: number | undefined): number | undefined { - if (a === undefined) { - return b - } - return b === undefined ? a : Math.max(a, b) +/** Whether a tab id was closed within the TTL, by the record's own worktree. The TTL is checked + * here because pruning only runs when the partition next records a close. Object.hasOwn because + * the map is a plain object: `in` answers true for every Object.prototype key. */ +export function hasClosedTerminalTabRecord( + map: ClosedTerminalTabTombstonesByTabId | undefined, + tabId: string, + worktreeId?: string, + now = Date.now() +): boolean { + const record = map !== undefined && Object.hasOwn(map, tabId) ? map[tabId] : undefined + return ( + record !== undefined && + now - record.closedAt <= CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS && + (worktreeId === undefined || record.worktreeId === worktreeId) + ) } -export type ClosedTerminalTabTombstoneAck = { - tombstones: ClosedTerminalTabTombstonesByTabId | undefined - /** Worktrees this snapshot actually carries a tab row for. A worktree the snapshot says nothing - * about — including one whose path never resolved to a local id — is not evidence of anything, - * so its tombstones are left untouched. */ - acknowledgedWorktreeIds: ReadonlySet - /** Every tab id the snapshot lists, across all worktrees: an id the host still carries anywhere - * has not been acknowledged, whichever worktree it now sits under. */ - hostKnownTabIds: ReadonlySet - hostRevision: number | undefined - now: number -} - -/** Retires tombstones the host has demonstrably seen, and stamps the rest with the revision that - * proved it had not yet. - * - * Retirement takes a snapshot that both covers the tombstone's worktree and is strictly newer than - * the last one that did — one snapshot alone can be the pull that was already in flight when the - * user closed. Absence never deletes here: a snapshot with no revision, or one carrying no row for - * the worktree, retires nothing. */ -export function reconcileClosedTerminalTabTombstones({ - tombstones, - acknowledgedWorktreeIds, - hostKnownTabIds, - hostRevision, - now -}: ClosedTerminalTabTombstoneAck): ClosedTerminalTabTombstonesByTabId { - const pruned = pruneClosedTerminalTabTombstones(tombstones, now) - if (hostRevision === undefined) { - return pruned - } - const kept: ClosedTerminalTabTombstonesByTabId = {} - for (const [tabId, tombstone] of Object.entries(pruned)) { - if (!acknowledgedWorktreeIds.has(tombstone.worktreeId)) { - kept[tabId] = tombstone - continue - } - const observed = tombstone.ackRevision - if (!hostKnownTabIds.has(tabId) && observed !== undefined && hostRevision > observed) { - continue - } - kept[tabId] = { ...tombstone, ackRevision: maxAckRevision(observed, hostRevision) } - } - return kept +/** Emptied on purpose: the workspace's terminal row exists, is empty, and some tab in it was + * closed within the TTL. An empty row with no live record is unknown (legacy data, an expired + * record, or a writer that is not a close), so it reads as never initialized. Any record + * suffices, which is weaker than "the last removal was a close" until every membership shrink is + * a close. */ +export function isTerminalWorkspaceEmptiedOnPurpose( + state: { + tabsByWorktree: Readonly> + closedTerminalTabTombstonesByTabId?: ClosedTerminalTabTombstonesByTabId + }, + worktreeId: string, + now = Date.now() +): boolean { + return ( + Object.hasOwn(state.tabsByWorktree, worktreeId) && + (state.tabsByWorktree[worktreeId]?.length ?? 0) === 0 && + Object.values(state.closedTerminalTabTombstonesByTabId ?? {}).some( + (record) => + record.worktreeId === worktreeId && + now - record.closedAt <= CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS + ) + ) } diff --git a/src/shared/ssh-pending-pty-kill.test.ts b/src/shared/ssh-pending-pty-kill.test.ts index 9b13a193a07..c2ba4cb95f1 100644 --- a/src/shared/ssh-pending-pty-kill.test.ts +++ b/src/shared/ssh-pending-pty-kill.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { decideSshPendingPtyKill, MAX_SSH_PENDING_PTY_KILLS_PER_TARGET, + normalizeSshPendingPtyKill, prunePendingSshPtyKills, SSH_PENDING_PTY_KILL_TTL_MS, type SshPendingPtyKill, @@ -64,6 +65,26 @@ describe('decideSshPendingPtyKill', () => { }) }) +// A close whose incarnation was never learned (the host was offline across a relaunch) still owes +// the kill: a `pty2:` id carries a per-start mint epoch, so the id alone names one process. +describe('a pending kill with no incarnation', () => { + const unfenced = { requestedAt: NOW, attempts: 0 } + + it('is kept for an epoch-scoped relay id and dropped for a legacy one', () => { + expect(normalizeSshPendingPtyKill(unfenced, 'pty2:epoch-a:3')).toEqual(unfenced) + expect(normalizeSshPendingPtyKill(unfenced, 'pty-3')).toBeNull() + }) + + it('replays while the host lists the id and retires once it does not', () => { + expect( + decideSshPendingPtyKill(unfenced, { hostListsPty: true, hostIncarnationId: 'inc-z' }, NOW) + ).toEqual({ action: 'replay' }) + expect( + decideSshPendingPtyKill(unfenced, { hostListsPty: false, hostIncarnationId: undefined }, NOW) + ).toEqual({ action: 'retire', reason: 'host-reports-absent' }) + }) +}) + describe('prunePendingSshPtyKills', () => { it('drops expired entries and caps the rest newest-first', () => { const entries: SshPendingPtyKillEntry[] = [ diff --git a/src/shared/ssh-pending-pty-kill.ts b/src/shared/ssh-pending-pty-kill.ts index 186ecd65acc..89472aee27f 100644 --- a/src/shared/ssh-pending-pty-kill.ts +++ b/src/shared/ssh-pending-pty-kill.ts @@ -12,21 +12,32 @@ import type { SshRemotePtyLease } from './ssh-types' * already the durable, restart-surviving, per-`(targetId, relayPtyId)` record of a remote PTY. */ export type SshPendingPtyKill = { requestedAt: number - /** The host-minted PTY incarnation this kill was aimed at, and the whole fence. + /** The host-minted PTY incarnation this kill was aimed at, and the fence. * - * A relay renumbers from `pty-1` on every start, so `(targetId, relayPtyId)` alone can name a - * DIFFERENT shell after a redeploy — the collision behind #16970. Current relays enforce this - * identity on `pty.shutdown`; the client also proves it from `pty.listProcesses` before replay so - * older relays that ignore the additive field keep the safest available fallback. */ - incarnationId: string + * A legacy relay renumbers from `pty-1` on every start, so `(targetId, relayPtyId)` alone can + * name a DIFFERENT shell after a redeploy — the collision behind #16970. Current relays enforce + * this identity on `pty.shutdown`; the client also proves it from `pty.listProcesses` before + * replay so older relays that ignore the additive field keep the safest available fallback. + * + * Absent only for a `pty2:` id, whose per-start mint epoch makes the id itself the fence. */ + incarnationId?: string /** Replays attempted since. Diagnostic; the TTL, not this, is the bound. */ attempts: number } +/** Whether a relay PTY id names exactly one process: current relays mint `pty2::` with a + * fresh epoch per start, so the id never recurs. Legacy `pty-N` ids restart at 1. */ +export function isEpochScopedRelayPtyId(relayPtyId: string): boolean { + return relayPtyId.startsWith('pty2:') +} + /** Colocated with the type so the two cannot drift. The lease loader is a strict whitelist that * drops anything it does not name, so a record omitted here would be silently stripped on every * launch — the exact failure `closed-terminal-tab-tombstones.ts` records having shipped once. */ -export function normalizeSshPendingPtyKill(value: unknown): SshPendingPtyKill | null { +export function normalizeSshPendingPtyKill( + value: unknown, + relayPtyId: string +): SshPendingPtyKill | null { if (!value || typeof value !== 'object') { return null } @@ -34,7 +45,11 @@ export function normalizeSshPendingPtyKill(value: unknown): SshPendingPtyKill | if (typeof raw.requestedAt !== 'number' || !Number.isFinite(raw.requestedAt)) { return null } - // No incarnation, no fence, and an unfenced kill order is worse than none. + const attempts = typeof raw.attempts === 'number' && raw.attempts >= 0 ? raw.attempts : 0 + if (raw.incarnationId === undefined && isEpochScopedRelayPtyId(relayPtyId)) { + return { requestedAt: raw.requestedAt, attempts } + } + // No fence, and an unfenced kill order is worse than none. if ( typeof raw.incarnationId !== 'string' || !raw.incarnationId || @@ -42,11 +57,7 @@ export function normalizeSshPendingPtyKill(value: unknown): SshPendingPtyKill | ) { return null } - return { - requestedAt: raw.requestedAt, - incarnationId: raw.incarnationId, - attempts: typeof raw.attempts === 'number' && raw.attempts >= 0 ? raw.attempts : 0 - } + return { requestedAt: raw.requestedAt, incarnationId: raw.incarnationId, attempts } } /** Backstop only — host acknowledgement is the normal exit. This covers a target the user never @@ -107,6 +118,10 @@ export function decideSshPendingPtyKill( if (!observation.hostListsPty) { return { action: 'retire', reason: 'host-reports-absent' } } + if (intent.incarnationId === undefined) { + // The id is the fence: only an epoch-scoped id is ever recorded without an incarnation. + return { action: 'replay' } + } if (observation.hostIncarnationId === undefined) { // Why not replay: an unfenced kill against a renumbered relay id destroys a shell nobody asked // to close, which is strictly worse than the leak. Hosts predating the published incarnation diff --git a/src/shared/workspace-session-schema-field-coverage.test.ts b/src/shared/workspace-session-schema-field-coverage.test.ts index 28572703d5a..c7001060b7b 100644 --- a/src/shared/workspace-session-schema-field-coverage.test.ts +++ b/src/shared/workspace-session-schema-field-coverage.test.ts @@ -91,13 +91,16 @@ describe('workspaceSessionStateSchema field coverage', () => { const parsed = parseWorkspaceSession({ ...MINIMAL_SESSION, closedTerminalTabTombstonesByTabId: { - 'tab-1': { closedAt: 1_700_000_000_000, worktreeId: 'repo:wt-1', ackRevision: 4 } + 'tab-1': { closedAt: 1_700_000_000_000, worktreeId: 'repo:wt-1', reason: 'cleanup' }, + // An older build's acknowledgement stamp is dropped; the record itself survives. + 'tab-2': { closedAt: 1_700_000_000_000, worktreeId: 'repo:wt-1', ackRevision: 4 } } }) expect(parsed.ok).toBe(true) expect(parsed.ok && parsed.value.closedTerminalTabTombstonesByTabId).toEqual({ - 'tab-1': { closedAt: 1_700_000_000_000, worktreeId: 'repo:wt-1', ackRevision: 4 } + 'tab-1': { closedAt: 1_700_000_000_000, worktreeId: 'repo:wt-1', reason: 'cleanup' }, + 'tab-2': { closedAt: 1_700_000_000_000, worktreeId: 'repo:wt-1' } }) })