Skip to content

fix(session): read the ssh partition a release before writing to it - #19955

Closed
nwparker wants to merge 1 commit into
nwparker/ssh-remote-integration-stackfrom
nwparker/ssh-partition-read-first
Closed

nwparker wants to merge 1 commit into
nwparker/ssh-remote-integration-stackfrom
nwparker/ssh-partition-read-first

Conversation

@nwparker

@nwparker nwparker commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​29 $\color{#cf222e}{\Huge{\mathbf{−}}}$​12 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​17
Prod 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​149 $\color{#cf222e}{\Huge{\mathbf{−}}}$​5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​144

Splits the SSH session-partition move into two releases. This one ships the read; a follow-up ships the write.

A reviewer will reasonably ask why a fix is being deliberately half-shipped, so the argument is below and in docs/reference/ssh-session-partition-move.md.

The read is the fix; the write is cleanup

Measured: with the write left on local, 84 of 86 tests in the SSH partition suites still pass, and the 2 failures are pure write-destination assertions (expected 'local' to be 'ssh:target-1'). All three tests in the remote-workspace round trip group — the group that is #12721 — pass, as do restores an unsaved hot-exit draft, which no other channel can recover, all four contested-bare-id rules, and five of six closed-last-terminal-tombstone tests.

Why shipping both at once is unsafe

  1. Every previously shipped build reads SSH session state out of local alone and never enumerates ssh:* partitions (listKnownSshHostIds is new).
  2. A client that has moved the rows therefore looks empty for every SSH workspace on the older build — unsaved dirtyDraftContent included.
  3. That build's publish then omits the workspace, and the relay applies replace-session as a wholesale snapshot overwrite (src/relay/workspace-session-handler.ts:147-153 — session: patch.session, no merge), so the host snapshot forgets it too.

Exposure is launch-and-quit, not "use an SSH workspace." Measured by running the shipping boot hydration and the shipping quit checkpoint over a pre-stack-shaped store:

partitions written -> ["local","ssh:target-1"]
local tabs   -> {}          # was {"repo-remote::/remote/wt":[{"id":"tab-1",...}]}
local drafts -> []          # was ["repo-remote::/remote/wt"] with dirtyDraftContent
ssh tabs     -> {"repo-remote::/remote/wt":[{"id":"tab-1",...}]}

No SSH workspace opened, no connection, no multiplexer, worktreesByRepo empty. Routing derives from buildRepoHostById(state.repos) — persisted catalog fields — so an offline target still moves.

What is and is not lost

Deliberately stated narrowly; the first two points came out of checking rather than assuming, and both cut against the alarming reading:

  • Client-side this is invisibility, not destruction. parseWorkspaceSessionsByHostId keeps any valid non-local host id — byte-identical in the older build — so the ssh:<targetId> partition survives the downgrade on disk and returns on re-upgrade.
  • Running remote agents are not reaped. No path from a lost host-snapshot entry to a pty.kill.
  • The older build publishes an omission (tabsByWorktreePath: {}), not path: []. Not the authoritative-empty-list shape of SSH host session partitions are written but never read — tab lists round-trip as empty and remote sync permanently deletes them #12721; 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 from 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. A fix that turns off the thing it ships with is worse than the bug.
  • Leave the legacy local copy behind rather than replacing it. Same trap, same reason.
  • A marker an 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, producing the damaging empty export. There is no cheaper fence.

The change

clientWorkspaceSessionWritePartitionHostId (src/shared/workspace-session-partition-owner.ts) maps ssh:* back to local, with two call sites: sessionPartitionHostFor and the routing tail of buildHostSessionRouting. The main-process runtime is untouched — it already wrote ssh:<targetId> before any of this.

Four destination assertions now say local, each naming the doc that flips them. Where a destination assertion was guarding a real invariant, the invariant is now asserted destination-independently so it holds in both releases:

  • keeps an SSH claimant out of the rotating runtime partition keeps expect(partition).not.toBe(RUNTIME_HOST) — the property that matters — alongside the release-specific destination.
  • does not strand the runtime co-claimant still asserts the runtime partition carries runtime-tab; only where the SSH row lands changed.

Verification

  • SSH partition suites: 92 passed (5 files).
  • Wide sweep src/renderer/src/lib src/main/persistence src/shared: 14014 passed, 119 skipped. Two failures were palette-match-performance and source-tree-scan perf budgets under load — both pass isolated, neither touches the session store.
  • pnpm tc clean (canaried), check:code-quality:changed 0 new findings.

Follow-up

docs/reference/ssh-session-partition-move.md carries the N+1 checklist: the four assertions to flip, the release-note line, the fleet condition (N must be broadly adopted first, since N+1's safety is that a downgrade lands on a build that already reads ssh:*), and the swallowed partition write in patchWorkspaceSessionByHost — the awaited local patch is the write that removes these rows, while the write meant to receive them is void-ed with a console.warn. Today that only risks runtime:* rows; from N+1 it would lose an SSH workspace's tabs and drafts outright. Also flagged as a comment at the call site.

Rebase status (2026-09-11) — deliberately NOT rebased

Left on nwparker/ssh-remote-integration-stack at 798a37bcba2f8ffdd970ff9473a0eeef477d8eef, unchanged. Head is still 22aaebf4fee284ca21b8540b4fba8569ea3ef2e5. No force-push was made and no SHA moved.

Why. Two reasons, both checked rather than assumed:

  1. It cannot target main. Cherry-picking this PR s single commit onto main at 5fa62feda7c264c6eda99ba4788ffebab9aae26c fails with CONFLICT (modify/delete): src/renderer/src/lib/workspace-session-ssh-partition-round-trip.test.ts deleted in HEAD — that suite does not exist on main; it is introduced by the partition stack this branch sits on. Three more files conflict on top of that.
  2. Its base branch has no PR. nwparker/ssh-remote-integration-stack is 59 commits ahead of main and is not one of the PRs in this pass. Rebasing it would mean force-pushing 59 commits of un-reviewed integration history, and this PR still could not merge afterwards because its base has nothing to merge into.

The branch is not stale in a way a rebase would fix; it is blocked on its base getting a PR (or on the read-first commit being re-cut against whichever stack PR carries the partition suites). That is an author call, not a rebase.

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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — the two-release split is well-reasoned and the destination-independent invariants survive the flip. One rough edge worth a look inline.

Reviewed changes

  • Staged client-write revert — clientWorkspaceSessionWritePartitionHostId maps ssh:* back to local, replacing workspaceSessionPartitionHostId at the two renderer write-routing sites, so the read on ssh:<targetId> ships ahead of the write.
  • Destination-independent test invariants — the four flipped assertions keep the properties that matter (not.toBe(RUNTIME_HOST), runtime co-claimant retention, tombstone-as-explicit-empty-row) and only move which partition each lands in.
  • Cross-release documentation — new docs/reference/ssh-session-partition-move.md carries the downgrade-safety argument, the N+1 fleet condition, and the swallowed-partition-write follow-up; .gitignore allow-lists it and a warning comment marks the void-ed partition write.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

executionHostId: string | null | undefined
): ExecutionHostId {
const partition = workspaceSessionPartitionHostId(executionHostId)
return parseExecutionHostId(partition)?.kind === 'ssh' ? LOCAL_EXECUTION_HOST_ID : partition

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This collapses every ssh:* host to local, not just local + ssh:* — two distinct SSH hosts claiming the same bare id (STA-4343, one repo on two devboxes) now report non-separable through contestedPartitionHosts and both route to local instead of one shadowing the other. The read side already conflates distinct SSH partitions (they are kept out of the claimant set), so this is plausibly intended, but the docstring only names the local + ssh:* case — worth a one-line note clarifying the two-SSH-host case for whoever lands N+1.

@nwparker

Copy link
Copy Markdown
Contributor Author

Closing as superseded. Its base branch has no PR of its own and is 470 commits behind main, so it cannot land stacked. More importantly its own design re-creates the defect it targets: adoption is gap-fill only, so boot 1 writes adopted rows to local, and from boot 2 onward the populated local row blocks adoption while the runtime keeps writing ssh:<targetId> — invisible again. Re-cut onto main it also has a hole (restoredPrimary returns before the tail it patches). #19572 has since landed, so re-derive against current main if this is picked up again.

@nwparker nwparker closed this Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant