From 99fcbe8f00ec09c9ecd43cfe3669ab39ada679c0 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 20:23:43 -0700 Subject: [PATCH] fix(ssh): keep a live pane published when only its membership record is stale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My call to leave this open was wrong, and the premise behind it was false. `mayCreate: false` does NOT refuse only when the pane has no durable tab — it also refuses when the tab is live and the layout is absent, or when the leaf is missing from the layout. In those cases mobile and CLI already publish the tab, so returning early left a visible pane wired to a stale pty while its shell ran on unreachable. The refusal is deterministic on unchanged state, so it re-refused every reconnect and every relaunch; leaf UUIDs are not user-addressable, so recovery meant editing state by hand. persistPtyBinding now reports WHY it refused. 'noTab' keeps the original guard — publishing a pane with no tab would announce something that does not exist — and 'noMembership' registers against the live tab instead. The pane key is routed in that case too: dropping routing on top of the binding orphaned the shell from the surfaces that could still show it. Mutation-proofed: reporting 'noTab' unconditionally reddens the live-tab test. --- src/main/ipc/pty.ts | 13 +++++- src/main/persistence.ts | 18 ++++++++ .../ssh-partition-upgrade-tab-loss.test.ts | 45 +++++++++++++++++++ src/main/ssh/ssh-relay-session.ts | 19 ++++++-- 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index aacfd9f04ce..94252a72e84 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -1,5 +1,6 @@ /* eslint-disable max-lines -- Why: PTY IPC is centralized in one main-process module so spawn env scoping, lifecycle cleanup, process inspection, and renderer IPC stay behind one audited boundary. */ import { parsePtyExitedError } from '../../shared/ssh-pty-failure-tokens' +import type { PtyBindingRefusalReason } from '../persistence' import { join, delimiter } from 'node:path' import { randomUUID } from 'node:crypto' import { statSync } from 'node:fs' @@ -581,9 +582,11 @@ export function bindPaneShell(args: { mayCreate?: boolean expectedBinding?: { ptyId: string; incarnationId?: string } expectedSourceBinding?: PtyBindingSourceExpectation -}): { bound: boolean; tabId: string } { +}): { bound: boolean; tabId: string; refusalReason?: PtyBindingRefusalReason } { const tabId = args.tabId + const refusal: { reason?: PtyBindingRefusalReason } = {} const bound = args.store?.persistPtyBinding({ + refusal, worktreeId: args.worktreeId, tabId, leafId: args.leafId, @@ -595,7 +598,13 @@ export function bindPaneShell(args: { ...(args.expectedSourceBinding ? { expectedSourceBinding: args.expectedSourceBinding } : {}) }) if (bound === false) { - return { bound: false, tabId } + // Why route the pane key even on refusal, unless the tab is gone: a refusal for stale + // membership leaves a LIVE tab whose pane is already listed. Dropping its routing on top of + // its binding orphans the shell from the surfaces that can still show it. + if (refusal.reason !== 'noTab') { + rememberPaneKeyForPty(args.ptyId, makePaneKey(tabId, args.leafId)) + } + return { bound: false, tabId, ...(refusal.reason ? { refusalReason: refusal.reason } : {}) } } rememberPaneKeyForPty(args.ptyId, makePaneKey(tabId, args.leafId)) return { bound: true, tabId } diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 851c09988af..9a1fdfd899f 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -2853,6 +2853,10 @@ export type PtyBindingSourceExpectation = { /** Caps the hoist ledger. Far above any realistic open-tab count, so a live tab is never evicted. */ const MAX_HOISTED_TAB_IDS = 512 +/** Why a `mayCreate: false` binding write was refused. `noTab` means the pane has no durable tab, + * so publishing it anywhere would be publishing something that does not exist. */ +export type PtyBindingRefusalReason = 'noTab' | 'noMembership' + /** Expired leases are unreachable from reattach; kept only long enough to outlive any in-flight * recovery window by orders of magnitude. */ const EXPIRED_SSH_LEASE_RETENTION_MS = 7 * 24 * 60 * 60 * 1000 @@ -7055,11 +7059,15 @@ export class Store { /** Reattach passes false: an absent durable pane never authorizes creating UI. * Defaults true so the spawn path keeps its force-quit-race branches. */ mayCreate?: boolean + /** Filled in when the write is refused, so a caller can tell "no such tab" — where anything + * it publishes would be a ghost — from "the tab is live but its membership is stale". */ + refusal?: { reason?: PtyBindingRefusalReason } expectedSourceBinding?: PtyBindingSourceExpectation }, hostId?: string | null ): boolean { const mayCreate = args.mayCreate ?? true + let refusalReason: PtyBindingRefusalReason | undefined const resolvedHostId = this.resolveHostId(hostId) const session = this.getWorkspaceSession(resolvedHostId) const paneKey = `${args.tabId}:${args.leafId}` @@ -7152,6 +7160,7 @@ export class Store { tab.ptyId = args.ptyId } else { terminalMembershipChanged = true + refusalReason = 'noTab' // Why: pty:spawn can beat the debounced writer; persist a minimal tab so hydration won't prune the binding as orphaned. const nextTabs = [ ...(tabs ?? []), @@ -7174,6 +7183,9 @@ export class Store { } if (!mayCreate && terminalMembershipChanged) { restoreSession() + if (args.refusal) { + args.refusal.reason = refusalReason ?? 'noMembership' + } return false } if (!isTerminalLeafId(args.leafId)) { @@ -7191,12 +7203,14 @@ export class Store { if (layout) { if (!layout.root) { terminalMembershipChanged = true + refusalReason ??= 'noMembership' // Why: createTab can persist an empty layout before TerminalPane mounts; the sync binding still needs a durable root. layout.root = { type: 'leaf', leafId: args.leafId } layout.activeLeafId = args.leafId layout.expandedLeafId = null } else if (!layoutContainsLeafId(layout.root, args.leafId)) { terminalMembershipChanged = true + refusalReason ??= 'noMembership' // Why: splitPane spawns before its snapshot reaches main; add a minimal leaf so a crash can't strand the pane's binding. layout.root = { type: 'split', @@ -7215,6 +7229,7 @@ export class Store { } } else { terminalMembershipChanged = true + refusalReason ??= 'noMembership' // Why: first tab spawn — persist a minimal layout so a SIGKILL before the renderer snapshot can't lose ptyIdsByLeafId. session.terminalLayoutsByTabId = { ...session.terminalLayoutsByTabId, @@ -7228,6 +7243,9 @@ export class Store { } if (!mayCreate && terminalMembershipChanged) { restoreSession() + if (args.refusal) { + args.refusal.reason = refusalReason ?? 'noMembership' + } return false } advanceTopologyFence() diff --git a/src/main/ssh-partition-upgrade-tab-loss.test.ts b/src/main/ssh-partition-upgrade-tab-loss.test.ts index 02b70e1673b..8976780344a 100644 --- a/src/main/ssh-partition-upgrade-tab-loss.test.ts +++ b/src/main/ssh-partition-upgrade-tab-loss.test.ts @@ -275,4 +275,49 @@ describe('an SSH pane whose membership was persisted into the ssh partition', () 'a tab whose pane had not mounted yet was stranded in the partition' ).toHaveLength(1) }) + + // The refusal reason decides whether a still-live pane keeps its routing and its mobile/CLI + // surface. Getting it wrong in either direction is a real failure: 'noTab' on a live tab + // orphans a running shell, and 'noMembership' on a missing tab publishes something that does + // not exist. + describe('why a mayCreate:false write was refused', () => { + it('reports noTab when the pane has no durable tab at all', async () => { + const store = await createStore() + const refusal: { reason?: string } = {} + + const bound = store.persistPtyBinding({ + worktreeId: WORKTREE, + tabId: TAB, + leafId: LEAF, + ptyId: PTY, + mayCreate: false, + refusal + } as never) + + expect(bound).toBe(false) + expect(refusal.reason).toBe('noTab') + }) + + it('reports noMembership when the tab is live but the leaf is not in its layout', async () => { + const store = await createStore() + // A live tab with a layout that does not contain the leaf being bound. + store.setWorkspaceSession(paneSession() as never) + const refusal: { reason?: string } = {} + + const bound = store.persistPtyBinding({ + worktreeId: WORKTREE, + tabId: TAB, + leafId: LEAF_TWO, + ptyId: PTY, + mayCreate: false, + refusal + } as never) + + expect(bound).toBe(false) + expect( + refusal.reason, + 'a live tab was reported as missing, which orphans its running shell' + ).toBe('noMembership') + }) + }) }) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index ea56dd88786..a292184352a 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -1,6 +1,7 @@ /* oxlint-disable max-lines */ // Why: single authority for all relay lifecycle state per SSH target (previously scattered across module Maps/Sets with duplicated paths). +import type { PtyBindingRefusalReason } from '../persistence' import { randomUUID } from 'node:crypto' import type { BrowserWindow } from 'electron' import { deployAndLaunchRelay } from './ssh-relay-deploy' @@ -2602,7 +2603,8 @@ export class SshRelaySession { // live layout outranks it. Resolved before the write, so a throw cannot lose it and leave the // graph registered under the tab the pane left. const tabId = resolvePaneShellTabId(this.store, lease.leafId) ?? lease.tabId - let bind: { bound: boolean; tabId: string } | null = null + let bind: { bound: boolean; tabId: string; refusalReason?: PtyBindingRefusalReason } | null = + null try { bind = bindPaneShell({ store: this.store, @@ -2616,14 +2618,23 @@ export class SshRelaySession { } catch (error) { console.error('[ssh-relay-session] Failed to persist reconnect incarnation:', error) } - if (bind?.bound === false) { - // Unresolved, not dead: the remote shell keeps running and stays - // reattachable once a durable pane names it again. + if (bind?.bound === false && bind.refusalReason === 'noTab') { + // No durable tab owns this leaf, so there is nothing to publish it against — registering + // would announce a pane that does not exist. Unresolved, not dead: the shell keeps running + // and stays reattachable once a durable pane names it again. console.info( `[ssh-relay-session] No durable pane owns ${appPtyId} on ${this.targetId}; left running unbound.` ) return } + if (bind?.bound === false) { + // The tab IS live — only its membership record is stale, and the mobile/CLI listing already + // publishes that tab. Returning here left a visible pane wired to a stale pty while its + // shell ran on unreachable, recoverable only by editing state by hand. + console.info( + `[ssh-relay-session] Stale membership for ${appPtyId} on ${this.targetId}; registering against its live tab.` + ) + } // The tab the bind resolved, not the lease's frozen one — a moved pane must not be recorded // in the graph under the tab it left while its record and fence name the tab it is in. this.runtime?.registerPty(appPtyId, lease.worktreeId, this.targetId, {