mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 16:03:47 +00:00
fix(ssh): stop a reconnect deleting local state the host has not seen
Reported from a 60-second manual test: reconnect an SSH workspace and the app drops to the home screen, a second tab running pnpm install is gone entirely, and one launched agent is listed twice. Three symptoms, one cause. The snapshot is applied as the whole truth for the reconnecting target. The tab merge iterates only the host's worktrees, then the result is spread over a gap where every local tab for that worktree has just been dropped — so a tab created locally whose upload has not landed has no branch that keeps it. Not a race: it cannot survive. Same for the pointers, where a snapshot that names no active worktree nulls activeWorktreeId and activeWorkspaceKey, which is the home screen while the user's terminals are still running. So the host is now authoritative for what it knows and not for what it has never been told. A local tab absent from the snapshot is kept, the worktree union is used so a snapshot with no entry for it at all cannot erase it, and a null active worktree only defers to local state when that workspace demonstrably still exists in the merged result. Two guards this change had to earn rather than assume. A null activeTabId is NOT missing information — it is a deliberate deselect that arms the duplicate-tab repair, and my first attempt defeated it and broke that test; it is honoured verbatim now. And preserving by tab id alone reintroduces the duplicate agent, because the host can carry the same session under a new tab id, so the preserve also checks the remote session id — the identity that survives a tab-id change. Testing, which is the part that failed here before. Eight tests fail on the unfixed code and pass on this one, at two levels: the merge decision table, and the real apply path driven through a store. The end-to-end version of the same scenario is deliberately NOT the guard and now says so in its header — measured against unfixed code it only reproduces about one run in three, because the destruction needs the tab created inside the debounced upload's suppression window and nothing external can force that. Its earlier green run is exactly why this shipped.
This commit is contained in:
@@ -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> = {}): TerminalTab {
|
||||
return {
|
||||
id,
|
||||
title: id,
|
||||
type: 'terminal',
|
||||
worktreeId: WORKTREE,
|
||||
...overrides
|
||||
} as TerminalTab
|
||||
}
|
||||
|
||||
function sessionState(overrides: Partial<WorkspaceSessionState> = {}): 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')
|
||||
})
|
||||
})
|
||||
@@ -28,10 +28,31 @@ export function mergeDirectSshRemoteWorkspaceSession(
|
||||
.map((tab) => [tab.id, tab])
|
||||
)
|
||||
const locallyPreservedTabIds = new Set<string>()
|
||||
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: {
|
||||
|
||||
@@ -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<typeof AgentStatusModule>()
|
||||
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<typeof createTestStore>
|
||||
|
||||
async function applySnapshot(store: TestStore, snap: RemoteWorkspaceSnapshot): Promise<void> {
|
||||
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)
|
||||
})
|
||||
})
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user