From d23ecef30118bf7ccd297d7d1aad37d516a67d47 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Thu, 1 Oct 2026 10:39:24 -0700 Subject: [PATCH] feat(session): retry failed renderer session writes and verify local folder PTYs (#16741 T2 P9) (#24406) * feat(session): retry failed renderer session writes and verify local folder PTYs (#16741 T2 P9) The renderer's session write subscriber now waits for the local session patch to be accepted by main. While a write is in flight no second write starts; if it rejects, the fields it carried go back into the pending set (newer edits to the same fields are not overwritten) and the next store or gate wake retries them instead of silently dropping them. Boot PTY hydration verifies a folder workspace as local only when its id is unique and it is pinned local, or its legacy scope resolves local with no remote candidate repo. A folder under an SSH project group is never verified. Ported by hunk from #16741 (a68b6f3531). * test(memory): build typed folder-workspace fixtures instead of asserting types --------- Co-authored-by: m4air --- ...cal-pty-registry-folder-workspaces.test.ts | 87 ++++++++++ .../memory/hydrate-local-pty-registry.test.ts | 12 +- src/main/memory/hydrate-local-pty-registry.ts | 20 +-- .../verified-local-folder-workspaces.test.ts | 121 ++++++++++++++ .../verified-local-folder-workspaces.ts | 40 +++++ .../app-shell/use-app-session-persistence.ts | 3 +- ...on-write-subscriber-acknowledgment.test.ts | 129 ++++++++++++++ ...ession-write-subscriber-allocation.test.ts | 8 +- .../src/lib/session-write-subscriber.ts | 47 +++++- ...ssion-split-pane-retirement-fences.test.ts | 157 ++++++++++++++++++ 10 files changed, 600 insertions(+), 24 deletions(-) create mode 100644 src/main/memory/hydrate-local-pty-registry-folder-workspaces.test.ts create mode 100644 src/main/memory/verified-local-folder-workspaces.test.ts create mode 100644 src/main/memory/verified-local-folder-workspaces.ts create mode 100644 src/renderer/src/lib/session-write-subscriber-acknowledgment.test.ts create mode 100644 src/renderer/src/runtime/web-session-split-pane-retirement-fences.test.ts diff --git a/src/main/memory/hydrate-local-pty-registry-folder-workspaces.test.ts b/src/main/memory/hydrate-local-pty-registry-folder-workspaces.test.ts new file mode 100644 index 00000000000..c67e4772bee --- /dev/null +++ b/src/main/memory/hydrate-local-pty-registry-folder-workspaces.test.ts @@ -0,0 +1,87 @@ +import { beforeEach, expect, it, vi } from 'vitest' +import type { FolderWorkspace } from '../../shared/folder-workspace-types' +import type { SessionInfo } from '../daemon/types' +import type { Store } from '../persistence' + +const getDaemonProviderMock = vi.fn() +vi.mock('../daemon/daemon-init', () => ({ + getDaemonProvider: () => getDaemonProviderMock() +})) +vi.mock('../project-runtime-git-options', () => ({ + getLocalProjectWorktreeGitOptions: () => ({}) +})) +const listLocalRepoWorktreesStrictMock = vi.fn() +vi.mock('../repo-worktrees', () => ({ + listLocalRepoWorktreesStrict: (...args: unknown[]) => listLocalRepoWorktreesStrictMock(...args) +})) + +function makeStore(folderWorkspaces: FolderWorkspace[]): Store { + const store: Partial = { + getRepos: () => [], + getFolderWorkspaces: () => folderWorkspaces, + getProjectGroups: () => [], + getAllWorktreeMeta: () => ({}), + getAllWorktreeMetaForHost: () => ({}) + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: hydration reads only the store members stubbed here. + return store as Store +} + +// Why fresh modules: the hydrator memoizes its pass and the registry is module-scoped. +async function loadFresh() { + vi.resetModules() + const hydrateMod = await import('./hydrate-local-pty-registry') + const registryMod = await import('./pty-registry') + return { + hydrate: hydrateMod.hydrateLocalPtyRegistryAtBoot, + listRegisteredPtys: registryMod.listRegisteredPtys + } +} + +beforeEach(() => { + getDaemonProviderMock.mockReset() + listLocalRepoWorktreesStrictMock.mockReset() +}) + +it('hydrates surviving true folder workspace PTYs without enumerating Git', async () => { + const { hydrate, listRegisteredPtys } = await loadFresh() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: ownership reads only id and executionHostId. + const workspace = { id: 'folder-workspace-1', executionHostId: 'local' } as FolderWorkspace + const session = { sessionId: 'folder:folder-workspace-1@@cafebabe', pid: 4242 } + getDaemonProviderMock.mockReturnValue({ listSessions: vi.fn().mockResolvedValue([session]) }) + + await hydrate(makeStore([workspace])) + + expect(listRegisteredPtys()).toEqual([ + expect.objectContaining({ + ptyId: 'folder:folder-workspace-1@@cafebabe', + worktreeId: 'folder:folder-workspace-1', + pid: 4242 + }) + ]) + expect(listLocalRepoWorktreesStrictMock).not.toHaveBeenCalled() +}) + +it.each(['deleted', 'remote', 'ssh'] as const)( + 'rechecks folder ownership after inventory when the catalog becomes %s', + async (change) => { + const { hydrate, listRegisteredPtys } = await loadFresh() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: ownership reads only id and executionHostId. + const folders = [{ id: 'folder-1', executionHostId: 'local' } as FolderWorkspace] + getDaemonProviderMock.mockReturnValue({ + listSessions: vi.fn().mockImplementation(async (): Promise[]> => { + if (change === 'deleted') { + folders.splice(0) + } else { + folders[0].executionHostId = change === 'ssh' ? 'ssh:target-1' : 'runtime:environment-1' + } + return [{ sessionId: 'folder:folder-1@@cafebabe', pid: 4242 }] + }) + }) + + await hydrate(makeStore(folders)) + + expect(listRegisteredPtys()).toEqual([]) + expect(listLocalRepoWorktreesStrictMock).not.toHaveBeenCalled() + } +) diff --git a/src/main/memory/hydrate-local-pty-registry.test.ts b/src/main/memory/hydrate-local-pty-registry.test.ts index b7dd5e1d233..ffa2f703e03 100644 --- a/src/main/memory/hydrate-local-pty-registry.test.ts +++ b/src/main/memory/hydrate-local-pty-registry.test.ts @@ -60,7 +60,8 @@ function makeStore( kind?: Repo['kind'] path?: string }[] = [], - worktreeMeta: Record = {} + worktreeMeta: Record = {}, + folderWorkspaces: FolderWorkspace[] = [] ): Store { const built: Repo[] = repos.map((r) => ({ id: r.id, @@ -72,15 +73,18 @@ function makeStore( executionHostId: r.executionHostId ?? null, kind: r.kind })) - return { + const store: Partial = { getRepos: () => built, - getFolderWorkspaces: (): FolderWorkspace[] => [], + getFolderWorkspaces: () => folderWorkspaces, + getProjectGroups: () => [], getAllWorktreeMeta: () => worktreeMeta, getAllWorktreeMetaForHost: (hostId) => Object.fromEntries( Object.entries(worktreeMeta).filter(([, meta]) => !meta.hostId || meta.hostId === hostId) ) - } as Store + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: hydration reads only the store members stubbed above. + return store as Store } function makeProvider(sessions: SessionInfo[]): Pick { diff --git a/src/main/memory/hydrate-local-pty-registry.ts b/src/main/memory/hydrate-local-pty-registry.ts index 4e5063b5fc1..6c70dc4c0e6 100644 --- a/src/main/memory/hydrate-local-pty-registry.ts +++ b/src/main/memory/hydrate-local-pty-registry.ts @@ -2,7 +2,6 @@ import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../../shared/ex import { throwIfSignalAborted, waitForPromiseWithSignal } from '../../shared/abort-signal-reason' import { mapSettledWithConcurrency } from '../../shared/map-with-concurrency' import { parsePtySessionId } from '../../shared/pty-session-id-format' -import { folderWorkspaceToWorktree } from '../../shared/folder-workspace-worktree' import { isFolderRepo } from '../../shared/repo-kind' import type { Repo } from '../../shared/repo-types' import { splitWorktreeId, worktreeIdComparisonKey } from '../../shared/worktree/id' @@ -17,6 +16,7 @@ import { readAllWorktreeMetaForHost } from '../persistence/host-qualified-worktr import { getLocalProjectWorktreeGitOptions } from '../project-runtime-git-options' import { listLocalRepoWorktreesStrict } from '../repo-worktrees' import { listRegisteredPtys, registerPty } from './pty-registry' +import { getVerifiedLocalFolderWorkspaceKeys } from './verified-local-folder-workspaces' type HydrationStore = Store @@ -165,6 +165,11 @@ async function hydrateLocalPtyRegistry( } throwIfSignalAborted(signal) + const verifiedLocalFolderKeys = getVerifiedLocalFolderWorkspaceKeys({ + folderWorkspaces: store.getFolderWorkspaces(), + projectGroups: store.getProjectGroups(), + repos: store.getRepos() + }) for (const info of inventory.sessions) { throwIfSignalAborted(signal) if (alreadyRegistered.has(info.sessionId)) { @@ -186,7 +191,7 @@ async function hydrateLocalPtyRegistry( return complete function isVerifiedLocalWorktree(worktreeId: string): boolean { - if (verifiedFolderWorktreeIds.has(worktreeId)) { + if (verifiedLocalFolderKeys.has(worktreeId) || verifiedFolderWorktreeIds.has(worktreeId)) { return true } const key = worktreeIdComparisonKey(worktreeId) @@ -220,17 +225,6 @@ function getVerifiedFolderWorktreeIds( repoCatalog: LocalRepoCatalog ): Set { const verified = new Set() - const folders = store.getFolderWorkspaces() - const counts = new Map() - for (const folder of folders) { - counts.set(folder.id, (counts.get(folder.id) ?? 0) + 1) - } - for (const folder of folders) { - const worktree = folderWorkspaceToWorktree(folder) - if (counts.get(folder.id) === 1 && worktree.hostId === LOCAL_EXECUTION_HOST_ID) { - verified.add(worktree.id) - } - } const metadata = readAllWorktreeMetaForHost(store, LOCAL_EXECUTION_HOST_ID) for (const [worktreeId, meta] of Object.entries(metadata)) { const parsed = splitWorktreeId(worktreeId) diff --git a/src/main/memory/verified-local-folder-workspaces.test.ts b/src/main/memory/verified-local-folder-workspaces.test.ts new file mode 100644 index 00000000000..594e701561b --- /dev/null +++ b/src/main/memory/verified-local-folder-workspaces.test.ts @@ -0,0 +1,121 @@ +import { describe, expect, it } from 'vitest' +import type { FolderWorkspaceHostState } from '../../shared/folder-workspace-execution-host' +import type { FolderWorkspace } from '../../shared/folder-workspace-types' +import type { ProjectGroup } from '../../shared/project-group-types' +import type { Repo } from '../../shared/repo-types' +import { getVerifiedLocalFolderWorkspaceKeys } from './verified-local-folder-workspaces' + +const folder = (patch: Partial = {}): FolderWorkspace => ({ + id: 'folder-1', + projectGroupId: 'group-1', + name: 'Workspace', + folderPath: '/workspace', + linkedTask: null, + comment: '', + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + createdAt: 0, + lastActivityAt: 0, + updatedAt: 0, + ...patch +}) +const repo = (patch: Partial = {}): Repo => ({ + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000000', + addedAt: 0, + ...patch +}) +const projectGroup = (patch: Partial = {}): ProjectGroup => ({ + id: 'group-1', + name: 'Group', + parentPath: null, + parentGroupId: null, + createdFrom: 'manual', + tabOrder: 0, + isCollapsed: false, + color: null, + createdAt: 0, + updatedAt: 0, + ...patch +}) +const state = (patch: Partial = {}): FolderWorkspaceHostState => ({ + folderWorkspaces: [folder()], + projectGroups: [], + repos: [], + ...patch +}) + +describe('verified local folder workspace keys', () => { + it.each([undefined, null, 'local'] as const)('accepts local ownership %s', (executionHostId) => { + expect( + getVerifiedLocalFolderWorkspaceKeys( + state({ + folderWorkspaces: [folder({ executionHostId })] + }) + ) + ).toEqual(new Set(['folder:folder-1'])) + }) + + it.each(['ssh:target-1', 'runtime:environment-1', 'invalid', ''])( + 'rejects nonlocal or invalid ownership %s', + (executionHostId) => { + expect( + getVerifiedLocalFolderWorkspaceKeys( + state({ + folderWorkspaces: [ + // Why Object.assign: invalid stored ids must reach the parser without widening the type. + Object.assign(folder(), { executionHostId }) + ] + }) + ) + ).toEqual(new Set()) + } + ) + + it('honors explicit local ownership over legacy scope', () => { + expect( + getVerifiedLocalFolderWorkspaceKeys( + state({ + folderWorkspaces: [folder({ executionHostId: 'local', connectionId: 'target-1' })], + repos: [repo({ executionHostId: 'runtime:environment-1' })] + }) + ) + ).toEqual(new Set(['folder:folder-1'])) + }) + + it.each([ + state({ folderWorkspaces: [] }), + state({ folderWorkspaces: [folder(), folder()] }), + state({ + folderWorkspaces: [ + folder({ executionHostId: 'local' }), + folder({ executionHostId: 'runtime:environment-1' }) + ] + }), + state({ folderWorkspaces: [folder({ connectionId: 'target-1' })] }), + state({ projectGroups: [projectGroup({ connectionId: 'target-1' })] }), + state({ repos: [repo({ executionHostId: 'runtime:environment-1' })] }), + state({ repos: [repo({ executionHostId: 'ssh:target-1' })] }), + state({ repos: [repo(), repo({ id: 'repo-2', connectionId: 'target-1' })] }), + state({ repos: [repo(), repo({ id: 'repo-2', executionHostId: 'runtime:environment-1' })] }) + ])('rejects missing, duplicate, remote, or ambiguous scope %#', (catalog) => { + expect(getVerifiedLocalFolderWorkspaceKeys(catalog)).toEqual(new Set()) + }) + + it('accepts local inferred scope without unrelated runtime repos affecting it', () => { + expect( + getVerifiedLocalFolderWorkspaceKeys( + state({ + repos: [ + repo(), + repo({ id: 'remote', path: '/elsewhere', executionHostId: 'runtime:environment-1' }) + ] + }) + ) + ).toEqual(new Set(['folder:folder-1'])) + }) +}) diff --git a/src/main/memory/verified-local-folder-workspaces.ts b/src/main/memory/verified-local-folder-workspaces.ts new file mode 100644 index 00000000000..eae3ee39967 --- /dev/null +++ b/src/main/memory/verified-local-folder-workspaces.ts @@ -0,0 +1,40 @@ +import { getRepoExecutionHostId, parseExecutionHostId } from '../../shared/execution-host' +import { + findFolderWorkspaceCandidateRepos, + resolveFolderWorkspaceHost, + type FolderWorkspaceHostState +} from '../../shared/folder-workspace-execution-host' +import { folderWorkspaceKey } from '../../shared/workspace-scope' + +export function getVerifiedLocalFolderWorkspaceKeys(state: FolderWorkspaceHostState): Set { + const counts = new Map() + for (const workspace of state.folderWorkspaces) { + counts.set(workspace.id, (counts.get(workspace.id) ?? 0) + 1) + } + const keys = new Set() + for (const workspace of state.folderWorkspaces) { + if (counts.get(workspace.id) !== 1) { + continue + } + const pin = parseExecutionHostId(workspace.executionHostId) + if (workspace.executionHostId != null) { + if (pin?.kind === 'local') { + keys.add(folderWorkspaceKey(workspace.id)) + } + continue + } + if (resolveFolderWorkspaceHost(state, workspace.id).kind !== 'local') { + continue + } + // The shared legacy resolver projects runtime ownership as local; attribution cannot. + if ( + findFolderWorkspaceCandidateRepos(state, workspace.id).some( + (repo) => getRepoExecutionHostId(repo) !== 'local' + ) + ) { + continue + } + keys.add(folderWorkspaceKey(workspace.id)) + } + return keys +} diff --git a/src/renderer/src/app-shell/use-app-session-persistence.ts b/src/renderer/src/app-shell/use-app-session-persistence.ts index 4d1e9987a09..6758c058b58 100644 --- a/src/renderer/src/app-shell/use-app-session-persistence.ts +++ b/src/renderer/src/app-shell/use-app-session-persistence.ts @@ -125,11 +125,11 @@ export function useAppSessionPersistence(): void { store: useAppStore, shouldSchedulePersist: () => !isDirectSshRemoteWorkspaceApplyInProgress(), subscribeToPersistGateOpen: onDirectSshRemoteWorkspaceApplyWindowClosed, + onPersistError: (error) => console.warn('[session] Local session patch failed:', error), persist: ({ patch }) => { const state = useAppStore.getState() // Why: route each host's worktree-scoped slice to its own partition; return the local write so the remote-workspace upload chain below keeps its ordering. const localWrite = patchWorkspaceSessionByHost(window.api.session, patch, state) - void localWrite const uploadAuthorities = captureRemoteWorkspaceUploadAuthorities(state) const pendingLayoutEdits = state.pendingDirectSshLayoutEditsByTabId if (uploadAuthorities.length > 0) { @@ -193,6 +193,7 @@ export function useAppSessionPersistence(): void { } })() } + return localWrite } }) }, []) diff --git a/src/renderer/src/lib/session-write-subscriber-acknowledgment.test.ts b/src/renderer/src/lib/session-write-subscriber-acknowledgment.test.ts new file mode 100644 index 00000000000..ccc239a97be --- /dev/null +++ b/src/renderer/src/lib/session-write-subscriber-acknowledgment.test.ts @@ -0,0 +1,129 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore, type AppState } from '@/store' +import { + createSessionWriteSubscriber, + type WorkspaceSessionWrite +} from './session-write-subscriber' + +function acknowledgment() { + let resolve!: () => void + let reject!: (error: unknown) => void + const promise = new Promise((yes, no) => { + resolve = yes + reject = no + }) + return { promise, resolve, reject } +} + +describe('session write acknowledgments', () => { + let initial: AppState + let dispose: (() => void) | undefined + beforeEach(() => { + initial = useAppStore.getState() + vi.useFakeTimers() + }) + afterEach(() => { + dispose?.() + dispose = undefined + vi.useRealTimers() + useAppStore.setState(initial, true) + }) + + function setup() { + const first = acknowledgment() + const persist = vi + .fn<(write: WorkspaceSessionWrite) => void | Promise>() + .mockImplementationOnce(() => first.promise) + const onPersistError = vi.fn() + let wake: (() => void) | undefined + dispose = createSessionWriteSubscriber({ + store: useAppStore, + persist, + onPersistError, + shouldSchedulePersist: () => true, + subscribeToPersistGateOpen: (listener) => { + wake = listener + return () => { + wake = undefined + } + } + }) + useAppStore.setState({ + workspaceSessionReady: true, + hydrationSucceeded: true, + activeTabId: 'old' + }) + vi.advanceTimersByTime(200) + return { first, persist, onPersistError, wake: () => wake?.() } + } + + it('serializes writes and preserves a newer edit to the in-flight field', async () => { + const { first, persist } = setup() + useAppStore.setState({ activeTabId: 'new' }) + await vi.advanceTimersByTimeAsync(200) + expect(persist).toHaveBeenCalledTimes(1) + first.resolve() + await vi.advanceTimersByTimeAsync(200) + expect(persist).toHaveBeenCalledTimes(2) + expect(persist.mock.calls[1][0].patch.activeTabId).toBe('new') + }) + + it.each(['gate', 'store'] as const)( + 'retains rejected intent until a %s wake and rebuilds fresh state', + async (source) => { + const { first, persist, onPersistError, wake } = setup() + useAppStore.setState({ activeTabId: 'new' }) + first.reject(new Error('retirement publication deferred')) + await vi.advanceTimersByTimeAsync(10_000) + expect(persist).toHaveBeenCalledTimes(1) + expect(onPersistError).toHaveBeenCalledTimes(1) + expect(vi.getTimerCount()).toBe(0) + if (source === 'gate') { + wake() + } else { + useAppStore.getState().setCacheTimerStartedAt('tab:pane', Date.now()) + } + await vi.advanceTimersByTimeAsync(200) + expect(persist).toHaveBeenCalledTimes(2) + expect(persist.mock.calls[1][0].patch.activeTabId).toBe('new') + expect(persist.mock.calls[1][0].patch).toHaveProperty('activeRepoId') + } + ) + + it.each(['resolve', 'reject'] as const)( + 'does not resume after disposal and late %s', + async (outcome) => { + const { first, persist, onPersistError, wake } = setup() + useAppStore.setState({ activeTabId: 'new' }) + dispose?.() + if (outcome === 'resolve') { + first.resolve() + } else { + first.reject(new Error('late failure')) + } + wake() + await vi.advanceTimersByTimeAsync(10_000) + expect(persist).toHaveBeenCalledTimes(1) + expect(onPersistError).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + } + ) + + it('retains a synchronous throw for a later wake', async () => { + const persist = vi.fn<(write: WorkspaceSessionWrite) => void>().mockImplementationOnce(() => { + throw new Error('write refused') + }) + dispose = createSessionWriteSubscriber({ store: useAppStore, persist }) + useAppStore.setState({ + workspaceSessionReady: true, + hydrationSucceeded: true, + activeTabId: 'retained' + }) + await vi.advanceTimersByTimeAsync(200) + expect(vi.getTimerCount()).toBe(0) + useAppStore.getState().setCacheTimerStartedAt('tab:pane', Date.now()) + await vi.advanceTimersByTimeAsync(200) + expect(persist).toHaveBeenCalledTimes(2) + expect(persist.mock.calls[1][0].patch.activeTabId).toBe('retained') + }) +}) diff --git a/src/renderer/src/lib/session-write-subscriber-allocation.test.ts b/src/renderer/src/lib/session-write-subscriber-allocation.test.ts index 3f2c5ce284c..2532002f9e7 100644 --- a/src/renderer/src/lib/session-write-subscriber-allocation.test.ts +++ b/src/renderer/src/lib/session-write-subscriber-allocation.test.ts @@ -50,7 +50,9 @@ function createHarness() { }, getState: () => state }, - persist: (payload) => persisted.push(payload) + persist: (payload) => { + persisted.push(payload) + } }) return { dispose, @@ -174,7 +176,9 @@ describe('session write subscriber allocation', () => { }, getState: () => state }, - persist: (payload) => persisted.push(payload), + persist: (payload) => { + persisted.push(payload) + }, shouldSchedulePersist: () => gateOpen, subscribeToPersistGateOpen: () => () => {} }) diff --git a/src/renderer/src/lib/session-write-subscriber.ts b/src/renderer/src/lib/session-write-subscriber.ts index 7e7e9cf7360..581d9e875c6 100644 --- a/src/renderer/src/lib/session-write-subscriber.ts +++ b/src/renderer/src/lib/session-write-subscriber.ts @@ -101,7 +101,8 @@ export type SessionWriteSubscriberDeps = { subscribe: (listener: (state: AppState) => void) => () => void getState: () => AppState } - persist: (payload: WorkspaceSessionWrite) => void + persist: (payload: WorkspaceSessionWrite) => void | Promise + onPersistError?: (error: unknown) => void debounceMs?: number } & SessionWritePersistGate @@ -114,11 +115,14 @@ export type SessionWriteSubscriberDeps = { export function createSessionWriteSubscriber({ store, persist, + onPersistError, shouldSchedulePersist, subscribeToPersistGateOpen, debounceMs = 150 }: SessionWriteSubscriberDeps): () => void { let timer: ReturnType | null = null + let disposed = false + let writing = false // Why: the subscriber fires on every store update (agent status, usage // refreshes, runtime title ticks, …). Without this gate each fire reset // the debounce, and when it finally expired buildWorkspaceSessionPayload @@ -133,14 +137,17 @@ export function createSessionWriteSubscriber({ let prevUnifiedTabsSource: UnifiedTabsByWorktree | null = null // Why: this set is the only record that a mutation still owes a write — `prev` has already // advanced past it, and change detection is identity-based, so a field dropped from here can - // never be re-detected. It is retired only by a flush that reached `persist` (or found nothing - // left to write), and by unsubscribe. A closed gate never retires it. + // never be re-detected. In-flight fields remain owned by that write until acknowledgment; + // failures merge them back without overwriting newer same-field edits. const pendingChangedFields = new Set() const terminalTabsProjection = createTerminalSessionTabsProjection() const unifiedTabsProjection = createUnifiedSessionTabsProjection() const flushPendingWrite = (): void => { timer = null + if (disposed || writing || pendingChangedFields.size === 0) { + return + } // Why: rebuild from the freshest store state rather than the snapshot // captured when this timer was scheduled. Today this is equivalent // because buildWorkspaceSessionPayload reads only SESSION_RELEVANT_FIELDS @@ -165,10 +172,41 @@ export function createSessionWriteSubscriber({ if (Object.keys(patch).length === 0) { return } - persist({ patch }) + writing = true + const settle = (failed: boolean, error?: unknown): void => { + writing = false + if (disposed) { + return + } + if (failed) { + for (const field of changed) { + pendingChangedFields.add(field) + } + // A rejection waits for the next store/gate wake, not a retry loop. + onPersistError?.(error) + } else if (pendingChangedFields.size > 0) { + armFlushTimer() + } + } + try { + const result = persist({ patch }) + if (result && typeof result.then === 'function') { + void result.then( + () => settle(false), + (error: unknown) => settle(true, error) + ) + } else { + settle(false) + } + } catch (error) { + settle(true, error) + } } const armFlushTimer = (): void => { + if (disposed || writing) { + return + } if (timer !== null) { clearTimeout(timer) } @@ -272,6 +310,7 @@ export function createSessionWriteSubscriber({ }) return () => { + disposed = true unsub() unsubGateOpen?.() if (timer !== null) { diff --git a/src/renderer/src/runtime/web-session-split-pane-retirement-fences.test.ts b/src/renderer/src/runtime/web-session-split-pane-retirement-fences.test.ts new file mode 100644 index 00000000000..345a0db60d4 --- /dev/null +++ b/src/renderer/src/runtime/web-session-split-pane-retirement-fences.test.ts @@ -0,0 +1,157 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { RuntimeMobileSessionTabsResult } from '../../../shared/runtime-types' +import { collectLeafIds } from '../components/terminal-pane/terminal-pane-layout-tree' +import { + planTerminalLiveLayoutRemovals, + selectRetiredPaneIds, + trackRetiredLeafIds +} from '../components/terminal-pane/terminal-live-layout-reconciliation' +import { applyFreshWebSessionTabsSnapshot } from './web-session-tabs-sync' +import { + clearWebSessionTerminalOrphanRecoveryForTests, + recoverWebSessionTerminalOrphansBeforeApply +} from './web-session-terminal-orphan-recovery' +import { + ENV, + LEAF_ID, + SECOND_LEAF_ID, + makeSnapshot, + makeState, + resetWebSessionTabsSyncTestState +} from './web-session-tabs-sync-test-harness' + +vi.mock('../store', () => ({ useAppStore: { setState: vi.fn() } })) +vi.mock('@/hooks/agent-hook-completion-notifications', () => ({ + observeAgentHookCompletionForNotification: vi.fn() +})) + +const TAB_ID = 'web-terminal-host-tab-1' +const mountedLeaves = [LEAF_ID, SECOND_LEAF_ID] + +function snapshot(version: number, leaves = mountedLeaves): RuntimeMobileSessionTabsResult { + return makeSnapshot( + leaves.map((leafId) => ({ + type: 'terminal' as const, + id: `host-tab-1::${leafId}`, + parentTabId: 'host-tab-1', + leafId, + title: 'shell', + isActive: leafId === LEAF_ID, + status: 'ready' as const, + terminal: `terminal-${leafId}` + })), + { snapshotVersion: version } + ) +} + +function retiredSnapshot(): RuntimeMobileSessionTabsResult { + return { + ...snapshot(3, [LEAF_ID]), + retiredTerminalSurfaces: [ + { + parentTabId: 'host-tab-1', + leafId: SECOND_LEAF_ID, + terminal: `terminal-${SECOND_LEAF_ID}`, + ptyId: 'native-second', + incarnationId: 'inc-second' + } + ] + } +} + +function createReconciliation() { + let state = makeState() + let previousLayoutLeafIds: ReadonlySet = new Set() + let retiredLeafIds: ReadonlySet = new Set() + const mounted = new Set(mountedLeaves) + const call = vi.fn(async () => { + throw new Error('execution host unavailable') + }) + + function plan(secondPtyId: string | null): number[] { + const root = state.terminalLayoutsByTabId[TAB_ID]?.root + expect(root).toBeDefined() + const layoutLeafIds = new Set(root ? collectLeafIds(root) : []) + retiredLeafIds = trackRetiredLeafIds({ + retiredLeafIds, + previousLayoutLeafIds, + layoutLeafIds, + mountedLeafIds: mounted + }) + previousLayoutLeafIds = layoutLeafIds + return selectRetiredPaneIds(planTerminalLiveLayoutRemovals(root, mounted, retiredLeafIds), { + paneCount: mounted.size, + paneIdForLeaf: (leaf) => (leaf === LEAF_ID ? 1 : 2), + ptyIdForPane: (pane) => (pane === 1 ? 'remote:first' : secondPtyId) + }) + } + + return { + call, + plan, + removeSecond: () => mounted.delete(SECOND_LEAF_ID), + leaves: () => collectLeafIds(state.terminalLayoutsByTabId[TAB_ID].root!), + async receive(incoming: RuntimeMobileSessionTabsResult) { + const recovered = await recoverWebSessionTerminalOrphansBeforeApply(state, incoming, ENV, { + call + }) + expect(recovered).not.toBeNull() + if (recovered) { + state = { ...state, ...applyFreshWebSessionTabsSnapshot(state, recovered, ENV) } + } + } + } +} + +describe('host snapshot fences before split-pane retirement', () => { + beforeEach(() => { + resetWebSessionTabsSyncTestState() + clearWebSessionTerminalOrphanRecoveryForTests() + }) + + it('keeps a null-transport pane when an older layout arrives', async () => { + const view = createReconciliation() + await view.receive(snapshot(2)) + expect(view.plan('remote:second')).toEqual([]) + await view.receive(retiredSnapshotWithVersion(1)) + expect(view.leaves()).toEqual(mountedLeaves) + expect(view.plan(null)).toEqual([]) + }) + + it('retains a missing pane when a newer layout cannot verify the host inventory', async () => { + const view = createReconciliation() + await view.receive(snapshot(2)) + view.plan('remote:second') + await view.receive(snapshot(3, [LEAF_ID])) + expect(view.call).toHaveBeenCalled() + expect(view.leaves()).toContain(SECOND_LEAF_ID) + expect(view.plan(null)).toEqual([]) + }) + + it('defers proven retirement until the transport clears and does not close twice', async () => { + const view = createReconciliation() + await view.receive(snapshot(2)) + view.plan('remote:second') + await view.receive(retiredSnapshot()) + expect(view.leaves()).toEqual([LEAF_ID]) + expect(view.plan('remote:second')).toEqual([]) + expect(view.plan(null)).toEqual([2]) + view.removeSecond() + expect(view.plan(null)).toEqual([]) + }) + + it('clears deferred retirement when the host reintroduces the leaf before detach', async () => { + const view = createReconciliation() + await view.receive(snapshot(2)) + view.plan('remote:second') + await view.receive(retiredSnapshot()) + expect(view.plan('remote:second')).toEqual([]) + await view.receive(snapshot(4)) + expect(view.leaves()).toEqual(mountedLeaves) + expect(view.plan(null)).toEqual([]) + }) +}) + +function retiredSnapshotWithVersion(snapshotVersion: number): RuntimeMobileSessionTabsResult { + return { ...retiredSnapshot(), snapshotVersion } +}