mirror of
https://github.com/stablyai/orca.git
synced 2026-09-28 08:02:43 +00:00
fix(session): read the ssh partition a release before writing to it
The SSH partition move has two halves, and only one of them is the fix. Reading both partitions IS the repair for #12721: the remote merge can only refuse to delete tabs this client actually holds, and hydrating them out of `ssh:<targetId>` is what arms that defence. Moving the write is cleanup that collapses the #12723 double-ownership. Shipping both at once is what a downgrade cannot survive. Every previously shipped build reads SSH session state out of `local` alone and never enumerates `ssh:*`, so a client that has moved the rows looks empty to the older build -- unsaved editor drafts included -- and that build's publish then omits the workspace, which the relay applies as a wholesale replace-session overwrite. Exposure is launch-and-quit, not 'use an SSH workspace': routing reads the persisted repo catalog, so an offline target still moves on the quit checkpoint. So this release reads `ssh:<targetId>` and keeps writing SSH state where every shipped build looks for it. The four destination assertions now say `local` and each names the file that flips them; the invariants around them -- draft survival, tombstone semantics, contested-id rules, and an SSH claimant staying out of the rotating runtime partition -- are asserted destination-independently and hold in both releases. docs/reference/ssh-session-partition-move.md carries the argument and the N+1 checklist, including the fleet condition that gates it.
This commit is contained in:
@@ -120,6 +120,7 @@ docs/**
|
||||
!docs/reference/remote-wire-compatibility.md
|
||||
!docs/reference/renderer-agent-status-performance.md
|
||||
!docs/reference/ssh-execution-boundary.md
|
||||
!docs/reference/ssh-session-partition-move.md
|
||||
!docs/reference/ssh-host-key-verification.md
|
||||
!docs/reference/ssh-reconnect-source-recovery.md
|
||||
!docs/reference/windows-setup-shell.md
|
||||
|
||||
@@ -0,0 +1,103 @@
|
||||
# Moving SSH workspace session state into its own partition
|
||||
|
||||
A worktree's durable session state (terminal tabs, editor files including unsaved drafts, browser
|
||||
workspaces, tab groups, layouts, visit recency) is stored per execution host. `local` is the legacy
|
||||
single blob; `runtime:*` and `ssh:*` each own a partition under `workspaceSessionsByHostId`.
|
||||
|
||||
SSH worktrees were split across two of these: the renderer wrote `local`, the main-process runtime
|
||||
read-modify-wrote `ssh:<targetId>` (#12723). Neither reader reunited them, so whatever landed on
|
||||
the unread side round-tripped as **absence**, and the `replace-session` upload converted absence
|
||||
into deletion on the host (#12721, #18173).
|
||||
|
||||
The repair moves SSH state into `ssh:<targetId>` on both sides. It ships in **two releases**, and
|
||||
the ordering is load-bearing.
|
||||
|
||||
## Why two releases
|
||||
|
||||
Both halves are needed for the end state, but only one of them is the fix:
|
||||
|
||||
- **Reading both partitions is the repair.** `mergeDirectSshRemoteWorkspaceSession` can only refuse
|
||||
to delete tabs _this client actually holds_ — hydrating them out of `ssh:<targetId>` is what arms
|
||||
that defence. With the read alone, an older client's empty publish no longer deletes anything and
|
||||
this client republishes the real list.
|
||||
- **Moving the write is cleanup.** It collapses the double-ownership so one workspace stops being
|
||||
written twice.
|
||||
|
||||
Shipping the write move in the same release is what a downgrade cannot survive:
|
||||
|
||||
1. Every previously shipped build reads SSH session state out of `local` alone, and never
|
||||
enumerates `ssh:*` partitions.
|
||||
2. A client that has moved the rows therefore looks **empty** for every SSH workspace on the older
|
||||
build — including unsaved `dirtyDraftContent`.
|
||||
3. That build's publish then **omits** the workspace, and the relay applies `workspace.patch` /
|
||||
`replace-session` as a **wholesale snapshot overwrite**
|
||||
(`src/relay/workspace-session-handler.ts`), so the host snapshot forgets it too.
|
||||
|
||||
**Exposure is launch-and-quit**, not "use an SSH workspace". Routing derives from the persisted
|
||||
repo catalog (`buildRepoHostById` over `state.repos`), so an offline target with no active
|
||||
multiplexer still moves on the shutdown checkpoint.
|
||||
|
||||
### What is and is not lost
|
||||
|
||||
Measured, so the severity is not overstated:
|
||||
|
||||
- Client-side this is **invisibility, not destruction**. `parseWorkspaceSessionsByHostId` keeps any
|
||||
valid non-`local` host id, in the older build too, so the `ssh:<targetId>` partition survives the
|
||||
downgrade on disk and returns on re-upgrade.
|
||||
- **Running remote agents are not reaped.** There is no path from a lost host-snapshot entry to a
|
||||
`pty.kill`.
|
||||
- The older build publishes an **omission**, not `path: []`. It is not the authoritative-empty-list
|
||||
shape of #12721, and the next pull reads omission as `unverifiable`.
|
||||
- The unrecoverable residue is therefore only what lived in the host snapshot and **never** in this
|
||||
client's `ssh:*` partition — tabs created by another paired device, or minted host-side while
|
||||
this client was away.
|
||||
|
||||
### Rejected alternatives
|
||||
|
||||
- **Dual-write a compatibility copy into `local`.** A populated base `tabsByWorktree` row is
|
||||
exactly what makes `workspacesTheBaseOwns` refuse to adopt, so this silently disables the
|
||||
adoption repair it ships alongside.
|
||||
- **Leave the legacy `local` copy behind instead of replacing it.** Same trap, same reason.
|
||||
- **A marker the older build would happen to honour.** Every input to its publish gate is host- or
|
||||
connection-derived (`hydratedTargetIds`, `expectedRevision`, `hostObservationToken`,
|
||||
`getActiveMultiplexer`); nothing on local disk reaches it. And the export inverts: making a
|
||||
worktree fail to resolve _excludes_ it rather than withholding the publish, which produces the
|
||||
damaging empty export.
|
||||
|
||||
## Release N (this change)
|
||||
|
||||
Reads `ssh:<targetId>`, keeps writing SSH state to `local`.
|
||||
|
||||
- `clientWorkspaceSessionWritePartitionHostId` in `src/shared/workspace-session-partition-owner.ts`
|
||||
maps `ssh:*` back to `local`. Two call sites: `sessionPartitionHostFor`
|
||||
(`workspace-session-host-contention.ts`) and the routing tail of `buildHostSessionRouting`
|
||||
(`workspace-session-host-persistence.ts`).
|
||||
- The main-process runtime is unchanged — it already wrote `ssh:<targetId>` before any of this.
|
||||
|
||||
## Release N+1 checklist
|
||||
|
||||
**Fleet condition: do not ship until release N is broadly adopted.** N+1's safety is exactly that a
|
||||
user downgrading from it lands on a build that already reads `ssh:*`. Any still-running build older
|
||||
than N has the full exposure above.
|
||||
|
||||
1. Delete `clientWorkspaceSessionWritePartitionHostId` and both call sites; they revert to
|
||||
`workspaceSessionPartitionHostId`.
|
||||
2. Flip these four assertions, each of which carries a pointer to this file:
|
||||
- `workspace-session-ssh-partition-round-trip.test.ts` — "routes the reunited workspace to the
|
||||
partition every shipped build reads": `LOCAL_EXECUTION_HOST_ID` → `SSH_HOST_ID`, and rename it
|
||||
back to "…to the partition that owns it".
|
||||
- `workspace-session-ssh-partition-round-trip.test.ts` — "writes an SSH workspace emptied by
|
||||
this build into the partition it writes to": swap the `local` / `SSH_HOST_ID` expectations.
|
||||
- `workspace-session-host-contention.test.ts` — "keeps an SSH claimant out of the rotating
|
||||
runtime partition": `LOCAL_EXECUTION_HOST_ID` → `SSH_HOST`. Keep the `not.toBe(RUNTIME_HOST)`
|
||||
assertion; it is the invariant and holds in both releases.
|
||||
- `workspace-session-host-contention.test.ts` — "does not strand the runtime co-claimant when
|
||||
the SSH row is written": the SSH row moves from the local write (`hostId === undefined`) to a
|
||||
`SSH_HOST` write.
|
||||
3. Release-note line: _"SSH workspace session state now persists in its own store partition.
|
||||
Downgrading below <release N> after this update will hide SSH workspaces' tabs and editor
|
||||
state until you upgrade again."_
|
||||
4. Fix, or accept with eyes open, the swallowed partition write in `patchWorkspaceSessionByHost`:
|
||||
the awaited `local` patch is what removes these rows from `local`, while the partition write
|
||||
meant to receive them is `void`-ed with a `console.warn`. Today that only risks `runtime:*`
|
||||
rows; from N+1 a swallowed rejection loses an SSH workspace's tabs and unsaved drafts outright.
|
||||
@@ -8,7 +8,7 @@ import { describe, expect, it, vi, type Mock } from 'vitest'
|
||||
import { getDefaultWorkspaceSession } from '../../../shared/constants'
|
||||
import type { TerminalTab } from '../../../shared/terminal-tab-types'
|
||||
import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types'
|
||||
import type { ExecutionHostId } from '../../../shared/execution-host'
|
||||
import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../../../shared/execution-host'
|
||||
import { folderWorkspaceKey } from '../../../shared/workspace-scope'
|
||||
import {
|
||||
indexWorktreeHostClaims,
|
||||
@@ -332,8 +332,14 @@ describe('read-time primary is the one the write path honours', () => {
|
||||
|
||||
it('keeps an SSH claimant out of the rotating runtime partition', () => {
|
||||
// Why this shape: the claims catalog sorts `runtime:` before `ssh:`, so a plain sort sent the
|
||||
// SSH workspace's rows into the runtime partition. It now persists in its own.
|
||||
expect(buildHostIdByWorktreeId(sshVersusRuntimeState())(SHARED_ID)).toBe(SSH_HOST)
|
||||
// SSH workspace's rows into the runtime partition, which a re-created environment then takes
|
||||
// over. That is the invariant, and it holds in both releases of the partition move.
|
||||
const partition = buildHostIdByWorktreeId(sshVersusRuntimeState())(SHARED_ID)
|
||||
|
||||
expect(partition).not.toBe(RUNTIME_HOST)
|
||||
// Release N writes SSH state to `local`, where every shipped build reads it; N+1 flips this
|
||||
// to SSH_HOST. See docs/reference/ssh-session-partition-move.md.
|
||||
expect(partition).toBe(LOCAL_EXECUTION_HOST_ID)
|
||||
})
|
||||
|
||||
it('does not strand the runtime co-claimant when the SSH row is written', async () => {
|
||||
@@ -351,11 +357,15 @@ describe('read-time primary is the one the write path honours', () => {
|
||||
})
|
||||
)
|
||||
|
||||
// The claim under test is that neither claimant's row is lost to the other, which is
|
||||
// destination-independent: the runtime co-claimant keeps its own partition either way.
|
||||
const runtimeWrite = set.mock.calls.find(([, hostId]) => hostId === RUNTIME_HOST)?.[0]
|
||||
expect(runtimeWrite?.tabsByWorktree[SHARED_ID]?.map((entry) => entry.id)).toEqual([
|
||||
'runtime-tab'
|
||||
])
|
||||
const sshWrite = set.mock.calls.find(([, hostId]) => hostId === SSH_HOST)?.[0]
|
||||
// Release N puts the SSH claimant's row in the local write (no hostId argument); N+1 moves it
|
||||
// to a SSH_HOST write. See docs/reference/ssh-session-partition-move.md.
|
||||
const sshWrite = set.mock.calls.find(([, hostId]) => hostId === undefined)?.[0]
|
||||
expect(sshWrite?.tabsByWorktree[SHARED_ID]?.map((entry) => entry.id)).toEqual(['ssh-tab'])
|
||||
})
|
||||
|
||||
|
||||
@@ -7,7 +7,7 @@ import {
|
||||
} from '../../../shared/execution-host'
|
||||
import { normalizeWorkspaceSessionKeyToWorkspaceId } from '../../../shared/workspace-scope'
|
||||
import { WORKSPACE_SESSION_FIELD_OWNERSHIP } from '../../../shared/workspace-session-host-field-ownership'
|
||||
import { workspaceSessionPartitionHostId } from '../../../shared/workspace-session-partition-owner'
|
||||
import { clientWorkspaceSessionWritePartitionHostId } from '../../../shared/workspace-session-partition-owner'
|
||||
import {
|
||||
isWorkspaceSessionRecord,
|
||||
type WorkspaceSessionRecord
|
||||
@@ -90,9 +90,12 @@ export function indexWorktreeHostClaims(
|
||||
return claims
|
||||
}
|
||||
|
||||
/** The partition a host's session rows live in: every non-'local' host owns its own. */
|
||||
/** The partition a host's session rows live in, from the client's point of view. Release N: every
|
||||
* `runtime:*` host owns its own, and `local` + every `ssh:*` host still share one blob — which is
|
||||
* why `contestedPartitionHosts` below can say a claimant set collapsing to one partition is not
|
||||
* separable. N+1 flips this to `workspaceSessionPartitionHostId`. */
|
||||
export function sessionPartitionHostFor(hostId: ExecutionHostId): ExecutionHostId {
|
||||
return workspaceSessionPartitionHostId(hostId)
|
||||
return clientWorkspaceSessionWritePartitionHostId(hostId)
|
||||
}
|
||||
|
||||
/** Distinct partitions a set of claimants spans. Fewer than two means persistence cannot tell the
|
||||
|
||||
@@ -9,7 +9,7 @@ import {
|
||||
parseExecutionHostId,
|
||||
type ExecutionHostId
|
||||
} from '../../../shared/execution-host'
|
||||
import { workspaceSessionPartitionHostId } from '../../../shared/workspace-session-partition-owner'
|
||||
import { clientWorkspaceSessionWritePartitionHostId } from '../../../shared/workspace-session-partition-owner'
|
||||
import { parseWorkspaceKey } from '../../../shared/workspace-scope'
|
||||
import { getRepoIdFromWorktreeId } from '../../../shared/worktree/id'
|
||||
import {
|
||||
@@ -193,7 +193,9 @@ export function buildHostSessionRouting(state: HostPersistenceState): HostSessio
|
||||
if (!repoHostId) {
|
||||
return LOCAL_EXECUTION_HOST_ID
|
||||
}
|
||||
return workspaceSessionPartitionHostId(repoHostId)
|
||||
// Release N writes SSH state to 'local', where every shipped build looks for it, while the read
|
||||
// path above already reunites `ssh:<targetId>`. N+1 flips this to the real owner partition.
|
||||
return clientWorkspaceSessionWritePartitionHostId(repoHostId)
|
||||
}
|
||||
return { hostIdByWorktreeId, claims }
|
||||
}
|
||||
@@ -228,6 +230,13 @@ export function patchWorkspaceSessionByHost(
|
||||
const localWrite = api.patch(local)
|
||||
for (const [hostId, slice] of nonLocalHostSessionEntries(slices)) {
|
||||
// Why: a failed runtime-partition write must not reject the local chain.
|
||||
//
|
||||
// Known asymmetry, and it grows in N+1: the local patch above is the write that REMOVES these
|
||||
// rows from `local`, and it is awaited, while the partition write that is supposed to receive
|
||||
// them is swallowed. Today that only risks `runtime:*` rows; once SSH state routes here too,
|
||||
// a swallowed rejection loses that workspace's tabs and unsaved drafts outright. Closing it
|
||||
// needs the local write to be conditional on the partition write, which the debounced hot path
|
||||
// cannot express as-is.
|
||||
void api.patch(slice as WorkspaceSessionPatch, hostId).catch((err) => {
|
||||
console.warn(`[session] host partition patch failed for ${hostId}:`, err)
|
||||
})
|
||||
|
||||
@@ -13,7 +13,7 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import type { SleepingAgentSessionRecord } from '../../../shared/agent-session-resume'
|
||||
import { getDefaultWorkspaceSession } from '../../../shared/constants'
|
||||
import type { ExecutionHostId } from '../../../shared/execution-host'
|
||||
import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../../../shared/execution-host'
|
||||
import {
|
||||
exportRemoteWorkspaceSession,
|
||||
importRemoteWorkspaceSession
|
||||
@@ -143,7 +143,12 @@ describe('ssh host partition hydration', () => {
|
||||
expect(read.session.activeTabIdByWorktree?.[WORKTREE_ID]).toBeUndefined()
|
||||
})
|
||||
|
||||
it('routes the reunited workspace back to the partition that owns it', async () => {
|
||||
it('routes the reunited workspace to the partition every shipped build reads', async () => {
|
||||
// Release N: the read above reunites `ssh:<targetId>`, but the WRITE still lands in `local`.
|
||||
// Flipping both at once is what a downgrade cannot survive -- a previous build reads SSH
|
||||
// session state out of `local` alone, so a moved workspace looks empty to it and its publish
|
||||
// then omits the workspace, which `replace-session` applies as a wholesale host overwrite.
|
||||
// N+1 flips this assertion to SSH_HOST_ID; see docs/reference/ssh-session-partition-move.md.
|
||||
const { buildHostIdByWorktreeId } = await import('./workspace-session-host-persistence')
|
||||
|
||||
const hostIdByWorktreeId = buildHostIdByWorktreeId({
|
||||
@@ -151,7 +156,7 @@ describe('ssh host partition hydration', () => {
|
||||
worktreesByRepo: {}
|
||||
})
|
||||
|
||||
expect(hostIdByWorktreeId(WORKTREE_ID)).toBe(SSH_HOST_ID)
|
||||
expect(hostIdByWorktreeId(WORKTREE_ID)).toBe(LOCAL_EXECUTION_HOST_ID)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -560,15 +565,17 @@ describe('ssh host partition and the closed-last-terminal tombstone', () => {
|
||||
return { restored: read.session, partitions }
|
||||
}
|
||||
|
||||
it('writes an SSH workspace emptied by this build into the partition that owns it', async () => {
|
||||
// The precondition the whole non-recurrence claim rests on: the tombstone lands in
|
||||
// `ssh:<targetId>` and `local` keeps no row, so the legacy shape cannot be regenerated.
|
||||
it('writes an SSH workspace emptied by this build into the partition it writes to', async () => {
|
||||
// The precondition the non-recurrence claim rests on is that the tombstone survives as an
|
||||
// explicit empty row in ONE partition, not which partition that is. Release N writes it to
|
||||
// `local`; N+1 flips these two assertions to `ssh:<targetId>` and to `local` holding no row.
|
||||
// The behavioural claim is asserted by the three tests below and is destination-independent.
|
||||
const { partitions } = await roundTripSession(
|
||||
session({ tabsByWorktree: { [WORKTREE_ID]: [] } })
|
||||
)
|
||||
|
||||
expect(partitions[SSH_HOST_ID]?.tabsByWorktree?.[WORKTREE_ID]).toEqual([])
|
||||
expect(Object.hasOwn(partitions.local?.tabsByWorktree ?? {}, WORKTREE_ID)).toBe(false)
|
||||
expect(partitions.local?.tabsByWorktree?.[WORKTREE_ID]).toEqual([])
|
||||
expect(Object.hasOwn(partitions[SSH_HOST_ID]?.tabsByWorktree ?? {}, WORKTREE_ID)).toBe(false)
|
||||
})
|
||||
|
||||
it('restores that tombstone as an explicit empty row, not a deleted key', async () => {
|
||||
|
||||
@@ -23,3 +23,31 @@ export function workspaceSessionPartitionHostId(
|
||||
): ExecutionHostId {
|
||||
return parseExecutionHostId(executionHostId)?.id ?? LOCAL_EXECUTION_HOST_ID
|
||||
}
|
||||
|
||||
/**
|
||||
* Release N of the SSH partition move: where the CLIENT writes a worktree's session.
|
||||
*
|
||||
* `workspaceSessionPartitionHostId` above is the destination this is converging on, and the READ
|
||||
* side already uses it — boot hydration enumerates `ssh:<targetId>` and
|
||||
* `adoptStrandedHostPartitionSession` reunites it with `local`. Moving the WRITE in the same
|
||||
* release is the part a downgrade cannot survive. Every shipped build reads SSH session state out
|
||||
* of `local` alone, so a client that has moved it looks empty to the previous version, and that
|
||||
* version's publish then OMITS the workspace — which the relay applies as a wholesale
|
||||
* `replace-session` snapshot overwrite (src/relay/workspace-session-handler.ts), so the host
|
||||
* forgets it too.
|
||||
*
|
||||
* Exposure is launch-and-quit, not "use an SSH workspace": routing reads the persisted repo
|
||||
* catalog, so an offline target with no multiplexer still moves on the quit checkpoint.
|
||||
*
|
||||
* Shipping the read alone is not a half-fix. Reading both partitions IS the repair for #12721 —
|
||||
* the merge can only refuse to delete tabs this client actually holds, and hydrating them is what
|
||||
* arms that defence. Moving the write collapses the #12723 double-ownership, which is cleanup.
|
||||
*
|
||||
* N+1 deletes this function and its two call sites; see docs/reference/ssh-session-partition-move.md.
|
||||
*/
|
||||
export function clientWorkspaceSessionWritePartitionHostId(
|
||||
executionHostId: string | null | undefined
|
||||
): ExecutionHostId {
|
||||
const partition = workspaceSessionPartitionHostId(executionHostId)
|
||||
return parseExecutionHostId(partition)?.kind === 'ssh' ? LOCAL_EXECUTION_HOST_ID : partition
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user