diff --git a/.gitignore b/.gitignore index 368a5797024..e79cba5d020 100644 --- a/.gitignore +++ b/.gitignore @@ -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 diff --git a/docs/reference/ssh-session-partition-move.md b/docs/reference/ssh-session-partition-move.md new file mode 100644 index 00000000000..e4bc0ad43d5 --- /dev/null +++ b/docs/reference/ssh-session-partition-move.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:` (#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:` 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:` 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:` 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:`, 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:` 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. diff --git a/src/renderer/src/lib/workspace-session-host-contention.test.ts b/src/renderer/src/lib/workspace-session-host-contention.test.ts index 8cfd9b6b1f1..5cebf148a5c 100644 --- a/src/renderer/src/lib/workspace-session-host-contention.test.ts +++ b/src/renderer/src/lib/workspace-session-host-contention.test.ts @@ -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']) }) diff --git a/src/renderer/src/lib/workspace-session-host-contention.ts b/src/renderer/src/lib/workspace-session-host-contention.ts index 797a2928766..d3af6384f1e 100644 --- a/src/renderer/src/lib/workspace-session-host-contention.ts +++ b/src/renderer/src/lib/workspace-session-host-contention.ts @@ -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 diff --git a/src/renderer/src/lib/workspace-session-host-persistence.ts b/src/renderer/src/lib/workspace-session-host-persistence.ts index 8fad7c67e79..07d180cde02 100644 --- a/src/renderer/src/lib/workspace-session-host-persistence.ts +++ b/src/renderer/src/lib/workspace-session-host-persistence.ts @@ -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:`. 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) }) diff --git a/src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts b/src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts index 65720ace472..dc346c754d5 100644 --- a/src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts +++ b/src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts @@ -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:`, 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:` 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:` 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 () => { diff --git a/src/shared/workspace-session-partition-owner.ts b/src/shared/workspace-session-partition-owner.ts index 4f8196e504c..f12f0892f13 100644 --- a/src/shared/workspace-session-partition-owner.ts +++ b/src/shared/workspace-session-partition-owner.ts @@ -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:` 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 +}