From 18217f48b2dbf765199341bede7e9ad57a10f4b9 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 28 Aug 2026 03:11:06 -0700 Subject: [PATCH] fix(ssh): do not leave a replayable kill order behind a reversible stop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worktree sleep stops through stopAndWait and marks those stops reversible; when one does not land the pane stays live and the user keeps using it. An order recorded there would come back on a later handshake and kill that terminal. Only killPtyFromRuntimeController — where the client gives the PTY up for good — records one, and it skips any PTY a reversible stop owns. --- config/reliability-gates.jsonc | 8 +-- src/main/ipc/pty-ssh-undelivered-kill.test.ts | 59 ++++++++++++++++++- src/main/ipc/pty/runtime/kill.ts | 19 +++--- .../ipc/pty/runtime/undelivered-ssh-kill.ts | 46 ++++++++------- 4 files changed, 97 insertions(+), 35 deletions(-) diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 77e9d6e79ed..ce712d1c01b 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3333,7 +3333,7 @@ "durationSeconds": 1.5, "summary": "175/175 at the relocated paths on the unified tool authority, replacing the 2026-08-27 record of the same command at 174/174. That run killed twelve of fourteen mutants as sole deltas — dropping the document branch from the one authority door, removing either browsing-registration refusal or the mint-side refusal that is its converse, deleting the registration notification or widening it to the worktree-wide and any-tab waits, disposing revoked state by grant instead of by page, dropping the hosting-renderer or destroyed-contents check, rebuilding the renderer tool target from the grant, and deleting the teardown identity check under a re-mint — and reported two as ill-posed rather than as kills. The relocation changed no file it names except their directory." }, -{ + { "date": "2026-08-27", "runner": "local", "platform": "macos", @@ -3378,7 +3378,7 @@ "durationSeconds": 0.17, "summary": "22/22 on the candidate that stops a previewed document writing into this desktop's Downloads folder. The hole was read out of the shipping path, not guessed: the preview partition took the shared browser partition policies whole, including will-download, and a preview guest joins no browser-page registry, so handleGuestWillDownload resolved no owner context, routeBrowserClientDownload answered local for a non-client-hosted contents, and the item got a reserved Downloads name with no prompt and no tab to attribute it to. Downloads are now the one policy the preview partition does not inherit. Red-green with each mutant as the sole delta: making the installer ignore the deny option routes the preview partition's download to the router again, and making the preview protocol installer stop asking for the deny leaves the option unset. The absence half is paired with a presence precondition in the same run — an ordinary browsing partition installed beside the preview one in one test still reaches the download router, so a deny that fenced everything, or a download path that had been removed entirely, would fail rather than pass quietly." }, -{ + { "date": "2026-08-27", "runner": "local", "platform": "macos", @@ -3441,7 +3441,7 @@ "durationSeconds": 0.36, "summary": "The four surfaces the trusted-click route is built from passed 63/63: the preload's click policy over a real document, including an untrusted dispatched click, an untrusted middle click, an SVG animated href resolved against the document so a relative, rooted or fragment SVG link behaves exactly like the identical HTML anchor, fragment and percent-encoded fragment targets and a trusted middle click; the guest policy's deny-only sinks, its refusal of a navigation that arrives before a document has bound the guest to a grant, and its report gate for an unfocused guest, an unregistered sender, a missing or revoked grant and non-web URLs; will-attach-webview pinning the preview preload in both directions; and the click channel handing main its own sender without a trusted-renderer check." }, -{ + { "date": "2026-08-26", "runner": "local", "platform": "macos", @@ -3468,7 +3468,7 @@ "durationSeconds": 66, "summary": "The journey passed 3/3 again under --repeat-each=3 on the candidate that clears grants on every fresh shell document and gates the mint channel on the trusted renderer, so neither change disturbs a live preview: the document still rendered, both browser counts held at baseline, and the target=_blank press still routed out as an Orca browser tab." }, -{ + { "date": "2026-08-26", "runner": "local", "platform": "macos", diff --git a/src/main/ipc/pty-ssh-undelivered-kill.test.ts b/src/main/ipc/pty-ssh-undelivered-kill.test.ts index e33981f7f6b..76e24dbb186 100644 --- a/src/main/ipc/pty-ssh-undelivered-kill.test.ts +++ b/src/main/ipc/pty-ssh-undelivered-kill.test.ts @@ -90,6 +90,8 @@ describe('undelivered SSH stops', () => { function install(store: ReturnType): { kill: (ptyId: string) => boolean + stopAndWait: (ptyId: string, opts?: { keepHistory?: boolean }) => Promise + markReversibleStops: (ptyIds: readonly string[]) => () => void runtime: ReturnType['runtime'] } { const { runtime } = installController(handlers as never) @@ -103,8 +105,15 @@ describe('undelivered SSH stops', () => { ) const controller = runtime.setPtyController.mock.calls[0]?.[0] as { kill: (ptyId: string) => boolean + stopAndWait: (ptyId: string, opts?: { keepHistory?: boolean }) => Promise + markReversibleStops: (ptyIds: readonly string[]) => () => void + } + return { + kill: controller.kill, + stopAndWait: controller.stopAndWait, + markReversibleStops: controller.markReversibleStops, + runtime } - return { kill: controller.kill, runtime } } // The leak: the shutdown died on the transport, the verdict is correctly `unverifiable`, and @@ -181,6 +190,54 @@ describe('undelivered SSH stops', () => { } }) + // Worktree sleep stops through stopAndWait and marks those stops reversible: when one does not + // land, the pane stays live and the user keeps using it. A durable order recorded here would come + // back on a later handshake and kill that terminal out from under them. + it('records nothing for a reversible stopAndWait that could not be delivered', async () => { + const store = createKillStore() + registerSshPtyProvider( + 'ssh-1', + sshProviderStub(async () => { + throw new Error('socket closed') + }) + ) + setPtyOwnership(SCOPED_PTY_ID, 'ssh-1') + restorePtyIncarnation(SCOPED_PTY_ID, 'inc-e') + const { stopAndWait } = install(store) + + try { + await expect(stopAndWait(SCOPED_PTY_ID, { keepHistory: true })).resolves.toBe(false) + expect(store.recordSshRemotePtyKillIntent).not.toHaveBeenCalled() + } finally { + unregisterSshPtyProvider('ssh-1') + deletePtyOwnership(SCOPED_PTY_ID) + } + }) + + it('records nothing while a reversible stop owns the PTY', async () => { + const store = createKillStore() + registerSshPtyProvider( + 'ssh-1', + sshProviderStub(async () => { + throw new Error('socket closed') + }) + ) + setPtyOwnership(SCOPED_PTY_ID, 'ssh-1') + restorePtyIncarnation(SCOPED_PTY_ID, 'inc-f') + const { kill, markReversibleStops } = install(store) + const release = markReversibleStops([SCOPED_PTY_ID]) + + try { + kill(SCOPED_PTY_ID) + await new Promise((resolve) => setTimeout(resolve, 0)) + expect(store.recordSshRemotePtyKillIntent).not.toHaveBeenCalled() + } finally { + release() + unregisterSshPtyProvider('ssh-1') + deletePtyOwnership(SCOPED_PTY_ID) + } + }) + it('records nothing for a local PTY, which has no later host to ask', async () => { const store = createKillStore() setPtyOwnership('local-pty', null) diff --git a/src/main/ipc/pty/runtime/kill.ts b/src/main/ipc/pty/runtime/kill.ts index 5d234a3b21f..1cfcff8d426 100644 --- a/src/main/ipc/pty/runtime/kill.ts +++ b/src/main/ipc/pty/runtime/kill.ts @@ -36,7 +36,7 @@ export function killPtyFromRuntimeController( // not revive a terminal the user explicitly closed. const incarnationId = finishPtyShutdown(ptyId, connectionId, store) // The relay was never asked, so the remote shell is still running. Keep the order. - recordUndeliveredSshPtyKill({ store, ptyId, connectionId, incarnationId }) + recordUndeliveredSshPtyKill(deps, ptyId, connectionId, incarnationId) runtime?.onPtyExit(ptyId, -1, incarnationId) rememberSyntheticKillExit(ptyId) sendPtyExitToRenderer({ @@ -95,7 +95,7 @@ export function killPtyFromRuntimeController( } // Outside the `retired` guard: the remote process outlives this client's bookkeeping // either way, and the intent is what the next handshake replays. - recordUndeliveredSshPtyKill({ store, ptyId, connectionId }) + recordUndeliveredSshPtyKill(deps, ptyId, connectionId) }) return true } @@ -115,7 +115,7 @@ export function killPtyFromRuntimeController( } runtime?.onPtyExit(ptyId, -1, ptyIncarnationById.get(ptyId)) } - recordUndeliveredSshPtyKill({ store, ptyId, connectionId }) + recordUndeliveredSshPtyKill(deps, ptyId, connectionId) }) return true } @@ -198,6 +198,14 @@ export function markReversibleStopsFromRuntimeController( } } +/** + * Deliberately records no undelivered-stop intent, unlike `killPtyFromRuntimeController`. + * + * Only a caller that gives the PTY up for good may leave a replayable kill order behind. This one + * hands its failures back instead: worktree sleep marks these stops reversible and leaves the pane + * live and usable when one does not land, so a durable order recorded here would come back on a + * later handshake and destroy a terminal the user had gone back to using. + */ export async function stopAndWaitPtyFromRuntimeController( deps: PtyRuntimeControllerDeps, ptyId: string, @@ -251,7 +259,6 @@ export async function stopAndWaitPtyFromRuntimeController( // Why: an absent SSH provider means there is no live target left to // await, but the relay lease must still be tombstoned. const incarnationId = finishPtyShutdown(ptyId, connectionId, store) - recordUndeliveredSshPtyKill({ store, ptyId, connectionId, incarnationId }) runtime?.onPtyExit(ptyId, -1, incarnationId) rememberSyntheticKillExit(ptyId) sendPtyExitToRenderer({ @@ -281,15 +288,12 @@ export async function stopAndWaitPtyFromRuntimeController( console.warn( `[pty] Failed to stop PTY ${ptyId}: ${err instanceof Error ? err.message : String(err)}` ) - recordUndeliveredSshPtyKill({ store, ptyId, connectionId }) return false } } try { if (!(await verifyPtyStopped(provider, ptyId, opts))) { runtime?.markPtyLivenessLive?.(ptyId) - // The host still lists it, so the stop did not take. Same intent, replayed the same way. - recordUndeliveredSshPtyKill({ store, ptyId, connectionId }) return false } } catch (err) { @@ -304,7 +308,6 @@ export async function stopAndWaitPtyFromRuntimeController( err instanceof Error ? err.message : String(err) }` ) - recordUndeliveredSshPtyKill({ store, ptyId, connectionId }) return false } const incarnationId = finishPtyShutdown(ptyId, connectionId, store) diff --git a/src/main/ipc/pty/runtime/undelivered-ssh-kill.ts b/src/main/ipc/pty/runtime/undelivered-ssh-kill.ts index fe087673713..f85fb97ace6 100644 --- a/src/main/ipc/pty/runtime/undelivered-ssh-kill.ts +++ b/src/main/ipc/pty/runtime/undelivered-ssh-kill.ts @@ -1,38 +1,40 @@ -import type { Store } from '../../../persistence' import { ptyIncarnationById } from '../provider/ownership-state' import { getRelayPtyId } from '../provider/registry' +import type { PtyRuntimeControllerDeps } from './controller-deps' /** Durably records a stop this client asked for on an SSH host and could not confirm, so the next * handshake to that host can replay it. Without this the remote PTY — a child of the detached * relay daemon, not of the ssh channel — outlives the failed RPC forever. * - * Silent no-ops are deliberate, not defensive padding: - * - no `connectionId`: a local PTY's owner is this process, so a failed kill has no later host to - * ask; there is nothing to replay against. - * - no incarnation: the replay fence is the host-minted PTY incarnation, and a relay renumbers - * from `pty-1` on every start. Recording an intent we could never safely aim would leave a kill - * order that can only be discarded, or worse, guessed at. */ -export function recordUndeliveredSshPtyKill(args: { - store: Store | undefined - ptyId: string - connectionId: string | null | undefined - /** Pass the value `finishPtyShutdown` returned: it clears the live map, so a caller that already - * ran it can no longer look the incarnation up. */ - incarnationId?: string - now?: number -}): void { - const { store, ptyId, connectionId } = args - if (!store || !connectionId) { + * The silent no-ops are the safety rules, not defensive padding: + * - **no `connectionId`**: a local PTY's owner is this process, so a failed kill has no later host + * to ask; there is nothing to replay against. + * - **no incarnation**: the replay fence is the host-minted PTY incarnation, and a relay renumbers + * from `pty-1` on every start. An order we could never safely aim can only be discarded later, + * or worse, guessed at. + * - **a reversible stop owns this PTY**: worktree sleep leaves the pane live and usable when a + * stop does not land, so an order recorded here would come back on a later handshake and kill a + * terminal the user had gone back to using. */ +export function recordUndeliveredSshPtyKill( + deps: Pick, + ptyId: string, + connectionId: string | null | undefined, + /** Pass what `finishPtyShutdown` returned: it clears the live map, so a caller that already ran + * it can no longer look the incarnation up. */ + incarnationId?: string, + now = Date.now() +): void { + const { store } = deps + if (!store || !connectionId || deps.reversibleStopOwnersByPtyId.has(ptyId)) { return } - const incarnationId = args.incarnationId ?? ptyIncarnationById.get(ptyId) - if (!incarnationId) { + const incarnation = incarnationId ?? ptyIncarnationById.get(ptyId) + if (!incarnation) { return } - const now = args.now ?? Date.now() store.recordSshRemotePtyKillIntent(connectionId, getRelayPtyId(connectionId, ptyId), { requestedAt: now, - incarnationId, + incarnationId: incarnation, attempts: 0 }) }