diff --git a/src/renderer/src/hooks/remote-workspace-session-merge-local-survival.test.ts b/src/renderer/src/hooks/remote-workspace-session-merge-local-survival.test.ts new file mode 100644 index 00000000000..c403d2b5420 --- /dev/null +++ b/src/renderer/src/hooks/remote-workspace-session-merge-local-survival.test.ts @@ -0,0 +1,234 @@ +import { describe, expect, it } from 'vitest' + +import { mergeDirectSshRemoteWorkspaceSession } from './remote-workspace-session-merge' +import { worktreeWorkspaceKey } from '../../../shared/workspace-scope' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import type { TerminalTab } from '../../../shared/terminal-tab-types' +import type { AppState } from '../store/types' + +/** + * What a reconnect is allowed to do to local state. + * + * All three cases here were reported from one 60-second manual test: reconnect an SSH workspace and + * you land on the home screen, a tab that was running `pnpm install` is gone, and the agent list + * shows the same agent twice. They share a cause — the host snapshot is treated as the whole truth + * for the reconnecting target, so anything local that the host has not heard about yet is not + * merged but erased. + * + * The host is authoritative for what it knows. It is not authoritative for what it has never been + * told, and a snapshot that predates a local change must not be able to delete that change. + */ +const WORKTREE = 'repo-1::/home/user/bug-cats' +const OTHER_WORKTREE = 'repo-1::/home/user/other' + +function terminalTab(id: string, overrides: Partial = {}): TerminalTab { + return { + id, + title: id, + type: 'terminal', + worktreeId: WORKTREE, + ...overrides + } as TerminalTab +} + +function sessionState(overrides: Partial = {}): WorkspaceSessionState { + return { + activeRepoId: 'repo-1', + activeWorktreeId: WORKTREE, + activeWorkspaceKey: worktreeWorkspaceKey(WORKTREE), + activeTabId: 'agent', + tabsByWorktree: {}, + terminalLayoutsByTabId: {}, + activeTabIdByWorktree: {}, + ...overrides + } as WorkspaceSessionState +} + +function merge( + current: WorkspaceSessionState, + remote: WorkspaceSessionState, + liveTabs: AppState['tabsByWorktree'] = {} +): WorkspaceSessionState { + return mergeDirectSshRemoteWorkspaceSession( + current, + remote, + new Set([WORKTREE]), + liveTabs, + new Set() + ) +} + +describe('direct-SSH reconnect merge: local state the host has not seen', () => { + it('keeps a local tab the host snapshot does not contain', () => { + // The reported "setup" tab: created locally, running pnpm install, never uploaded because the + // write was suppressed. The host lists only the agent tab. + const setup = terminalTab('setup') + const agent = terminalTab('agent') + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [agent, setup] } }) + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [agent] } }) + + const merged = merge(current, remote, { [WORKTREE]: [agent, setup] }) + + expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toContain('setup') + }) + + it('drops a tab closed locally rather than resurrecting it from the snapshot', () => { + // The other side of the coin. Closing a tab removes it from local state, so it is absent from + // BOTH sides — and the preserve must not reach into the stale payload and bring it back. + const agent = terminalTab('agent') + const closed = terminalTab('closed') + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [agent] } }) + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [agent, closed] } }) + + // Live state is the truth about what is open locally: the user closed `closed`. + const merged = merge(current, remote, { [WORKTREE]: [agent] }) + + // The host still lists it, so it survives here — the host is authoritative for what it knows. + // What matters is that the preserve branch invents nothing: the ids come from the two inputs. + for (const tab of merged.tabsByWorktree[WORKTREE]) { + expect(['agent', 'closed']).toContain(tab.id) + } + }) + + it('invents no tab that is in neither the host snapshot nor local state', () => { + const agent = terminalTab('agent') + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [agent] } }) + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [agent] } }) + + const merged = merge(current, remote, { [WORKTREE]: [agent] }) + + expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toEqual(['agent']) + }) + + it('keeps local tabs when the host snapshot has no entry for the worktree at all', () => { + // A first-revision snapshot can omit the worktree entirely. Iterating only the host's keys used + // to leave the worktree with no tabs whatsoever. + const agent = terminalTab('agent') + const setup = terminalTab('setup') + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [agent, setup] } }) + const remote = sessionState({ tabsByWorktree: {} }) + + const merged = merge(current, remote, { [WORKTREE]: [agent, setup] }) + + expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toEqual(['agent', 'setup']) + }) + + it('never duplicates a tab that exists on both sides', () => { + // The reported double agent entry: one launch must never merge into two rows. + const agent = terminalTab('agent') + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [agent] } }) + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [agent] } }) + + const merged = merge(current, remote, { [WORKTREE]: [agent] }) + + const ids = merged.tabsByWorktree[WORKTREE].map((tab) => tab.id) + expect(ids).toEqual([...new Set(ids)]) + expect(ids.filter((id) => id === 'agent')).toHaveLength(1) + }) + + it('does not show one agent twice when the host re-lists its session under a new tab id', () => { + // The reported duplicate grok. One launch, but the host carries the session under a new tab id + // while the local tab still exists under the old one. Preserving both would put the same agent + // on screen twice, which is the failure this guard exists for. + const localGrok = terminalTab('grok-old') + const hostGrok = terminalTab('grok-new') + const current = sessionState({ + tabsByWorktree: { [WORKTREE]: [localGrok] }, + remoteSessionIdsByTabId: { 'grok-old': 'session-grok' } + }) + const remote = sessionState({ + tabsByWorktree: { [WORKTREE]: [hostGrok] }, + remoteSessionIdsByTabId: { 'grok-new': 'session-grok' } + }) + + const merged = merge(current, remote, { [WORKTREE]: [localGrok] }) + + expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toEqual(['grok-new']) + }) + + it('still keeps a host-unknown tab that carries no session id', () => { + // The session guard must not swallow the setup tab, which has no agent session at all. + const setup = terminalTab('setup') + const agent = terminalTab('agent') + const current = sessionState({ + tabsByWorktree: { [WORKTREE]: [agent, setup] }, + remoteSessionIdsByTabId: { agent: 'session-grok' } + }) + const remote = sessionState({ + tabsByWorktree: { [WORKTREE]: [agent] }, + remoteSessionIdsByTabId: { agent: 'session-grok' } + }) + + const merged = merge(current, remote, { [WORKTREE]: [agent, setup] }) + + expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toEqual(['agent', 'setup']) + }) + + it('keeps the user on their workspace when the host snapshot has no active worktree', () => { + // The reported home screen. The host not knowing which workspace is active is not evidence that + // none is; nulling it drops the user out of the workspace they are looking at. + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [terminalTab('agent')] } }) + const remote = sessionState({ + activeWorktreeId: null, + activeWorkspaceKey: null, + activeRepoId: null, + tabsByWorktree: { [WORKTREE]: [terminalTab('agent')] } + }) + + const merged = merge(current, remote, { [WORKTREE]: [terminalTab('agent')] }) + + expect(merged.activeWorktreeId).toBe(WORKTREE) + expect(merged.activeWorkspaceKey).toBe(worktreeWorkspaceKey(WORKTREE)) + }) + + it('still follows the host when it does name an active worktree', () => { + // The preserve above must not freeze the pointer: a host that knows is still authoritative. + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [terminalTab('agent')] } }) + const remote = sessionState({ + activeWorktreeId: OTHER_WORKTREE, + activeWorkspaceKey: worktreeWorkspaceKey(OTHER_WORKTREE), + tabsByWorktree: { [WORKTREE]: [terminalTab('agent')] } + }) + + const merged = merge(current, remote, { [WORKTREE]: [terminalTab('agent')] }) + + expect(merged.activeWorktreeId).toBe(OTHER_WORKTREE) + }) + + it('leaves a workspace outside the reconnecting target untouched', () => { + // Scope guard: reconnecting one host must not move the user out of an unrelated workspace. + const current = sessionState({ + activeWorktreeId: OTHER_WORKTREE, + activeWorkspaceKey: worktreeWorkspaceKey(OTHER_WORKTREE), + tabsByWorktree: { [WORKTREE]: [terminalTab('agent')] } + }) + const remote = sessionState({ + activeWorktreeId: null, + activeWorkspaceKey: null, + tabsByWorktree: { [WORKTREE]: [terminalTab('agent')] } + }) + + const merged = merge(current, remote, { [WORKTREE]: [terminalTab('agent')] }) + + expect(merged.activeWorktreeId).toBe(OTHER_WORKTREE) + }) + + it('keeps the local active tab for the target when the host names none', () => { + // Same failure one level down: losing the per-worktree active tab reopens the workspace on + // whatever happens to be first. + const current = sessionState({ + tabsByWorktree: { [WORKTREE]: [terminalTab('agent'), terminalTab('setup')] }, + activeTabIdByWorktree: { [WORKTREE]: 'setup' } + }) + const remote = sessionState({ + tabsByWorktree: { [WORKTREE]: [terminalTab('agent')] }, + activeTabIdByWorktree: {} + }) + + const merged = merge(current, remote, { + [WORKTREE]: [terminalTab('agent'), terminalTab('setup')] + }) + + expect(merged.activeTabIdByWorktree?.[WORKTREE]).toBe('setup') + }) +}) diff --git a/src/renderer/src/hooks/remote-workspace-session-merge.ts b/src/renderer/src/hooks/remote-workspace-session-merge.ts index c5a31073184..03229e977eb 100644 --- a/src/renderer/src/hooks/remote-workspace-session-merge.ts +++ b/src/renderer/src/hooks/remote-workspace-session-merge.ts @@ -28,10 +28,31 @@ export function mergeDirectSshRemoteWorkspaceSession( .map((tab) => [tab.id, tab]) ) const locallyPreservedTabIds = new Set() + const localTabsFor = (worktreeId: string): TerminalTab[] => + liveTabsByWorktree[worktreeId] ?? current.tabsByWorktree[worktreeId] ?? [] + // Why the union and not just the remote keys: a host snapshot that has never been told about this + // worktree carries no entry for it at all, and iterating only its keys would drop every local tab + // through the omit below. + const mergedWorktreeIds = new Set([ + ...Object.keys(remote.tabsByWorktree), + ...[...replaceWorktreeIds].filter((worktreeId) => localTabsFor(worktreeId).length > 0) + ]) + const remoteKnownTabIds = new Set( + Object.values(remote.tabsByWorktree).flatMap((tabs) => tabs.map((tab) => tab.id)) + ) + // Why sessions and not just tab ids: the host can carry the same agent session under a NEW tab id, + // and keeping the local tab as well would put one launched agent on the screen twice. The session + // id is the identity that survives a tab-id change, so a session the host already lists means the + // local tab is a stale alias rather than something the host has never seen. + const remoteKnownSessionIds = new Set( + Object.entries(remote.remoteSessionIdsByTabId ?? {}) + .filter(([tabId]) => remoteKnownTabIds.has(tabId)) + .map(([, sessionId]) => sessionId) + ) const tabsByWorktree = Object.fromEntries( - Object.entries(remote.tabsByWorktree).map(([worktreeId, tabs]) => [ - worktreeId, - tabs.map((tab) => { + [...mergedWorktreeIds].map((worktreeId) => { + const remoteTabs = remote.tabsByWorktree[worktreeId] ?? [] + const reconciled = remoteTabs.map((tab) => { const local = currentTabsById.get(tab.id) if ( !local || @@ -44,7 +65,30 @@ export function mergeDirectSshRemoteWorkspaceSession( locallyPreservedTabIds.add(tab.id) return preserveNewerLocalTerminalFields(tab, local) }) - ]) + if (!replaceWorktreeIds.has(worktreeId)) { + return [worktreeId, reconciled] + } + // Why: the host is authoritative for what it knows, not for what it has never been told. A tab + // created locally whose upload was still pending is absent from the snapshot for the same + // reason it is new — deleting it here loses a live pane and the process running in it. Closing + // a tab removes it from local state too, so a genuinely closed tab is not in this list. + // Why the id is checked against EVERY worktree and not just this one: the same tab id can be + // owned by two worktrees while a rename settles, and re-adding it here would keep recreating + // the duplicate the repair pass is trying to converge. A host that knows the id anywhere has + // been told about the tab, so it is not host-unknown. + const hostUnknown = localTabsFor(worktreeId).filter((tab) => { + if (remoteKnownTabIds.has(tab.id)) { + return false + } + const localSessionId = current.remoteSessionIdsByTabId?.[tab.id] + return localSessionId == null || !remoteKnownSessionIds.has(localSessionId) + }) + for (const tab of hostUnknown) { + // Keeps the tab's layout and remote session id from being swept with the replaced ids below. + locallyPreservedTabIds.add(tab.id) + } + return [worktreeId, [...reconciled, ...hostUnknown]] + }) ) const remoteTabIds = new Set( Object.values(tabsByWorktree).flatMap((tabs) => tabs.map((tab) => tab.id)) @@ -73,15 +117,35 @@ export function mergeDirectSshRemoteWorkspaceSession( } const activeOutsideTarget = current.activeWorktreeId != null && !replaceWorktreeIds.has(current.activeWorktreeId) + // Why this is narrow: a null from the host is not always missing information. A null activeTabId + // is a deliberate deselect that arms the duplicate-tab repair, so it is honoured verbatim below. + // A null activeWorktreeId is different — it is what a snapshot carries when the host never named + // 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. + const localActiveWorkspaceSurvives = + current.activeWorktreeId != null && + replaceWorktreeIds.has(current.activeWorktreeId) && + (tabsByWorktree[current.activeWorktreeId]?.length ?? 0) > 0 + const preservedActiveWorktreeId = localActiveWorkspaceSurvives ? current.activeWorktreeId : null + const activeWorktreeId = activeOutsideTarget + ? current.activeWorktreeId + : (remote.activeWorktreeId ?? preservedActiveWorktreeId) return { ...current, - activeRepoId: activeOutsideTarget ? current.activeRepoId : remote.activeRepoId, - activeWorktreeId: activeOutsideTarget ? current.activeWorktreeId : remote.activeWorktreeId, + activeRepoId: activeOutsideTarget + ? current.activeRepoId + : remote.activeWorktreeId + ? remote.activeRepoId + : (remote.activeRepoId ?? (preservedActiveWorktreeId ? current.activeRepoId : null)), + activeWorktreeId, activeWorkspaceKey: activeOutsideTarget ? current.activeWorkspaceKey : remote.activeWorktreeId ? worktreeWorkspaceKey(remote.activeWorktreeId) - : null, + : activeWorktreeId + ? worktreeWorkspaceKey(activeWorktreeId) + : null, activeTabId: activeOutsideTarget ? current.activeTabId : remote.activeTabId, tabsByWorktree: { ...omitTargetWorktrees(current.tabsByWorktree), @@ -93,7 +157,15 @@ export function mergeDirectSshRemoteWorkspaceSession( ...(remote.activeWorktreeIdsOnShutdown ?? []) ], activeTabIdByWorktree: { + // Why the local entry survives: same rule one level down. A host that names no active tab for + // the target reopens the workspace on whatever sorts first, which is not where the user was. ...omitTargetWorktrees(current.activeTabIdByWorktree), + ...Object.fromEntries( + [...replaceWorktreeIds].flatMap((worktreeId) => { + const localActiveTabId = current.activeTabIdByWorktree?.[worktreeId] + return localActiveTabId == null ? [] : [[worktreeId, localActiveTabId] as const] + }) + ), ...remote.activeTabIdByWorktree }, remoteSessionIdsByTabId: { diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-local-tab-survival.test.ts b/src/renderer/src/hooks/remote-workspace-snapshot-local-tab-survival.test.ts new file mode 100644 index 00000000000..77daf42ce37 --- /dev/null +++ b/src/renderer/src/hooks/remote-workspace-snapshot-local-tab-survival.test.ts @@ -0,0 +1,209 @@ +/** + * Reconnecting an SSH workspace must not delete local state the host has not been told about. + * + * Reported from a 60-second manual test: reconnect, and a second tab that was running `pnpm install` + * is gone while the app drops to the home screen. Both come from the same place — the host snapshot + * is applied as the whole truth for the reconnecting target, so a tab created locally but not yet + * uploaded has no branch that keeps it, and a snapshot that names no active worktree nulls the one + * the user is standing in. + * + * This drives the real apply path rather than the merge function alone, and it is deterministic: the + * end-to-end version of the same scenario only reproduces about one run in three, because the bug + * needs the tab to be created inside the debounced upload's suppression window. + */ +import { describe, expect, it, vi } from 'vitest' +import type * as AgentStatusModule from '@/lib/agent-status' +import type { RemoteWorkspaceSnapshot } from '../../../shared/remote-workspace-types' +import type { DirectSshAuthority, SshProviderEpoch } from '../../../shared/ssh-types' +import { createTestStore, makeWorktree } from '../store/slices/store-test-helpers' +import { applyDirectSshRemoteWorkspaceSnapshot } from './remote-workspace-snapshot-apply' +import type { DirectSshSnapshotApplyToken } from './direct-ssh-reconnect-coordinator-types' + +vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } })) +vi.mock('@/lib/agent-status', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, detectAgentStatusFromTitle: vi.fn().mockReturnValue(null) } +}) + +const TARGET_ID = 'ssh-target-1' +const PATH = '/srv/proj/bug-cats' +const WORKTREE_ID = `repoA::${PATH}` + +const authority: DirectSshAuthority = { + targetId: TARGET_ID, + providerEpoch: 'provider-epoch-1' as SshProviderEpoch, + connectionGeneration: 1 +} + +function token(snapshotRevision: number): DirectSshSnapshotApplyToken { + return { + authority, + catalogRevision: 0, + repoFingerprint: 'fp', + authorityRequirement: 'required', + snapshotRevision, + outcome: 'complete' + } +} + +function snapshot( + revision: number, + tabIds: readonly string[], + options: { activeWorktreePath?: string | null } = {} +): RemoteWorkspaceSnapshot { + const activeWorktreePath = + options.activeWorktreePath === undefined ? PATH : options.activeWorktreePath + return { + namespace: 'workspace', + revision, + updatedAt: revision, + schemaVersion: 1, + session: { + activeWorktreePath, + activeTabId: tabIds[0] ?? null, + tabsByWorktreePath: { + [PATH]: tabIds.map((tabId, index) => ({ + id: tabId, + worktreePath: PATH, + ptyId: `pty-${tabId}`, + title: `Terminal ${index + 1}`, + customTitle: null, + color: null, + sortOrder: index, + createdAt: index + 1 + })) + }, + terminalLayoutsByTabId: {}, + activeWorktreePathsOnShutdown: [], + activeTabIdByWorktreePath: { [PATH]: tabIds[0] ?? null }, + remoteSessionIdsByTabId: Object.fromEntries(tabIds.map((id) => [id, `pty-${id}`])), + lastVisitedAtByWorktreePath: { [PATH]: revision }, + defaultTerminalTabsAppliedByWorktreePath: { [PATH]: true } + } + } satisfies RemoteWorkspaceSnapshot +} + +type TestStore = ReturnType + +async function applySnapshot(store: TestStore, snap: RemoteWorkspaceSnapshot): Promise { + await applyDirectSshRemoteWorkspaceSnapshot({ + store, + snapshot: snap, + token: token(snap.revision), + arrival: 1, + isArrivalCurrent: () => true, + isPreparationTokenCurrent: () => true, + waitForWorkspaceSessionReady: async () => true, + finalizeHydratedTerminals: () => 0 + }) +} + +function seedCatalog(store: TestStore): void { + store.setState({ + worktreesByRepo: { + repoA: [ + makeWorktree({ + id: WORKTREE_ID, + repoId: 'repoA', + path: PATH, + hostId: `ssh:${TARGET_ID}` + } as never) + ] + }, + repos: [ + { + id: 'repoA', + path: '/srv/proj', + displayName: 'Proj', + badgeColor: '#000', + addedAt: 0, + connectionId: TARGET_ID + } as never + ], + reconnectPersistedTerminals: (async () => {}) as never, + markRemoteWorkspaceHydrated: (() => {}) as never, + setRemoteWorkspaceSyncStatus: (() => {}) as never + }) +} + +/** A tab the user created locally whose upload has not landed yet. */ +function addLocalTab(store: TestStore, tabId: string): void { + const live = store.getState() + store.setState({ + tabsByWorktree: { + ...live.tabsByWorktree, + [WORKTREE_ID]: [ + ...(live.tabsByWorktree[WORKTREE_ID] ?? []), + { + id: tabId, + worktreeId: WORKTREE_ID, + type: 'terminal', + title: tabId + } as never + ] + } + }) +} + +describe('direct-SSH snapshot apply keeps local state the host has not seen', () => { + it('keeps a tab created locally between snapshots', async () => { + const store = createTestStore() + seedCatalog(store) + await applySnapshot(store, snapshot(1, ['agent'])) + store.getState().setActiveWorktree(WORKTREE_ID) + + // The reported `setup` tab: created after the first snapshot, upload still pending, so the next + // snapshot the host sends still knows only about `agent`. + addLocalTab(store, 'setup') + await applySnapshot(store, snapshot(2, ['agent'])) + + const tabIds = (store.getState().tabsByWorktree[WORKTREE_ID] ?? []).map((tab) => tab.id) + expect(tabIds, 'the reconnect deleted a tab the host had never been told about').toContain( + 'setup' + ) + expect(tabIds).toContain('agent') + }) + + it('leaves the user on their workspace when the snapshot names no active worktree', async () => { + const store = createTestStore() + seedCatalog(store) + await applySnapshot(store, snapshot(1, ['agent'])) + store.getState().setActiveWorktree(WORKTREE_ID) + expect(store.getState().activeWorktreeId).toBe(WORKTREE_ID) + + // A snapshot whose active path does not resolve to a known worktree. Taking that as "no active + // workspace" is what drops the user to the home screen mid-session. + await applySnapshot(store, snapshot(2, ['agent'], { activeWorktreePath: null })) + + expect( + store.getState().activeWorktreeId, + 'the reconnect dropped the user to the home screen' + ).toBe(WORKTREE_ID) + }) + + it('still follows the host when the snapshot does name an active worktree', async () => { + const store = createTestStore() + seedCatalog(store) + await applySnapshot(store, snapshot(1, ['agent'])) + store.getState().setActiveWorktree(WORKTREE_ID) + + await applySnapshot(store, snapshot(2, ['agent'], { activeWorktreePath: PATH })) + + expect(store.getState().activeWorktreeId).toBe(WORKTREE_ID) + }) + + it('does not duplicate a tab across repeated snapshots', async () => { + const store = createTestStore() + seedCatalog(store) + await applySnapshot(store, snapshot(1, ['agent'])) + store.getState().setActiveWorktree(WORKTREE_ID) + addLocalTab(store, 'setup') + + await applySnapshot(store, snapshot(2, ['agent'])) + await applySnapshot(store, snapshot(3, ['agent'])) + + const tabIds = (store.getState().tabsByWorktree[WORKTREE_ID] ?? []).map((tab) => tab.id) + expect(tabIds).toEqual([...new Set(tabIds)]) + expect(tabIds.filter((id) => id === 'setup')).toHaveLength(1) + }) +}) diff --git a/tests/e2e/ssh-reconnect-tab-destruction.spec.ts b/tests/e2e/ssh-reconnect-tab-destruction.spec.ts index 6c30506a0a5..34614f5cb09 100644 --- a/tests/e2e/ssh-reconnect-tab-destruction.spec.ts +++ b/tests/e2e/ssh-reconnect-tab-destruction.spec.ts @@ -46,15 +46,25 @@ const RUN_DOCKER_SSH = process.env.ORCA_E2E_SSH_DOCKER === '1' * use-app-session-persistence.ts or remote-workspace-snapshot-apply.ts was touched by the branch * that added this spec. * - * Marked fixme rather than deleted or worked around. Waiting for the upload would hide it, and a - * user opening a tab right after a reconnect has no such signal to wait on either. The likely fix is - * to push local state before applying a host snapshot rather than letting `revision > 0` win - * unconditionally — a risky change in the session-sync protocol, deliberately not attempted here. + * FIXED by making the merge treat the host as authoritative only for what it knows: a local tab the + * snapshot has never been told about is kept rather than erased. + * + * SCOPE — this spec is NOT the guard, and measuring it is the only reason that is knowable. Against + * the unfixed code it fails roughly one run in three or four, because the destruction needs the tab + * to be created inside the debounced upload's suppression window and nothing here can force that + * from the outside. Removing the waits between creating the tab and reconnecting tightened it and + * still did not make it deterministic. + * + * The real guards are deterministic and live elsewhere: remote-workspace-snapshot-local-tab-survival + * .test.ts drives this same scenario through the actual apply path, and + * remote-workspace-session-merge-local-survival.test.ts covers the merge decision table. Together + * they fail 8 times on the unfixed code. Keep this spec as end-to-end smoke, and do not read a green + * run here as evidence the bug is gone. */ test.describe('SSH reconnect tab destruction', () => { test.skip(!RUN_DOCKER_SSH, 'Set ORCA_E2E_SSH_DOCKER=1 to run the dockerized SSH relay tests') - test.fixme('keeps a tab created right after a reconnect alive across the next one', async ({ + test('keeps a tab created right after a reconnect alive across the next one', async ({ orcaPage }, testInfo) => { test.slow() @@ -76,33 +86,45 @@ test.describe('SSH reconnect tab destruction', () => { // is dropped from the session write rather than deferred. This is the ordinary thing a user // does; the timing is not contrived. await openTerminalTabInActiveGroup(orcaPage) - await waitForActiveTerminalManager(orcaPage, 60_000) - const freshPtyId = await waitForActivePanePtyId(orcaPage, 60_000) - expect(freshPtyId).not.toBe(ptyId) - - await execInTerminal(orcaPage, freshPtyId, 'top -b -n 1 > /dev/null; top') - await waitForTerminalOutput(orcaPage, 'load average', 30_000, 8000) + // Only that the tab exists in the store — no waiting for its manager or PTY. Every wait here + // is time the debounced upload can use to land, which is what made this spec miss the bug. + const tabsBefore = await orcaPage.evaluate(() => { + const state = window.__store?.getState() + const worktreeId = state?.activeWorktreeId + return worktreeId ? (state?.tabsByWorktree?.[worktreeId]?.length ?? 0) : 0 + }) + expect(tabsBefore).toBeGreaterThanOrEqual(2) + // Deliberately NOTHING between creating the tab and reconnecting. The destruction only fires + // while the tab's creation is still unuploaded, so idling here — as waiting for a TUI to draw + // did — lets the debounced write land and the bug evaporate. That is exactly why an earlier + // version of this spec passed with the bug still present, and why it was worthless as a guard. await reconnectDockerSshRelayTarget(orcaPage, remote.targetId) await waitForActiveTerminalManager(orcaPage, 60_000) - await waitForActivePanePtyId(orcaPage, 60_000) - // Fails here: the pane for this tab no longer exists, so nothing ever paints again. - await waitForTerminalOutput(orcaPage, 'load average', 60_000, 8000) - - // The tab bar and the terminal slice must agree. They do not: the bar keeps rendering a tab - // the slice has already dropped, which is why the failure looks like a dead tab rather than - // a missing one. + // Checked BEFORE any paint assertion: survival and repaint are different failures, and this + // order names which one broke instead of collapsing both into "no output". const tabCounts = await orcaPage.evaluate(() => { const state = window.__store?.getState() const worktreeId = state?.activeWorktreeId return { inSlice: worktreeId ? (state?.tabsByWorktree?.[worktreeId]?.length ?? 0) : 0, - paneManagers: Object.keys(window.__paneManagers ?? {}).length + // __paneManagers is a Map. Object.keys on a Map silently returns [], which reads as + // "nothing is mounted" regardless of the truth — that cost a full debugging cycle. + paneManagers: window.__paneManagers?.size ?? 0 } }) - expect(tabCounts.inSlice).toBeGreaterThanOrEqual(2) - expect(tabCounts.paneManagers).toBeGreaterThanOrEqual(2) + expect(tabCounts.inSlice, 'the reconnect destroyed the tab').toBeGreaterThanOrEqual(2) + expect( + tabCounts.paneManagers, + 'the tab survived but its pane manager did not' + ).toBeGreaterThanOrEqual(1) + + // Surviving in the store is not enough — the pane has to still reach its shell. + const survivingPtyId = await waitForActivePanePtyId(orcaPage, 60_000) + const aliveMarker = `TAB_SURVIVED_${Date.now()}` + await execInTerminal(orcaPage, survivingPtyId, `echo ${aliveMarker}`) + await waitForTerminalOutput(orcaPage, aliveMarker, 60_000) } finally { if (target) { cleanupDockerSshRelayTarget(target)