From 2b391652b1842e4c4931b33454a501d3fbc47c70 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:47:15 -0700 Subject: [PATCH 1/3] fix(terminal): a close the host never heard must survive the reconnect (#16752) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An enterprise user: "Every day I open orca and it opens more tabs daily at a linear scale." Three reports over a week, told on 08-19 that a PR had fixed it, reported twice more after. STA-4658 (P0), GH #12447, #15136, #10342, #9585. One install held 39 zombie tab records. The revived tab's sleeping-agent record still holds the pre-close session id, so it boots `claude --resume ` -- two agents on one transcript. ## The chain, measured Reproduced deterministically in `ssh-lost-kill-tab-resurrection.spec.ts`: close an SSH tab, kill the relay daemon in the container so `pty.kill` rejects with a transport-class error, reconnect. drop 2 resurrected the closed tab : baseline=1 drop1=1 drop2=2 (closed tab returned) drop3=1 The trigger is narrow and had to be measured rather than assumed: killed relay daemon reproduces **6 of 6 runs**; an orderly `ssh.disconnect` **passes**. Only an ungraceful loss -- network partition, host reboot, relay crash, a laptop sleeping mid-session -- strands the close with the RPC rejecting on a transport-class error. Both variants live in the spec behind one `runResurrectionCycles` parameterized solely by the disruption, so the difference is attributable to that single variable. What actually carries the tab back, from the pull path (`workspace.get` -> `getRemoteSnapshot`, `remote-workspace-relay-sync.ts:29`): pullSnapshot rev=3 tabs={repo:["16c4a3e1","06aba6b6"]} pullSnapshot rev=4 tabs={repo:["16c4a3e1","ff72768e"]} <- ff72768e IS the resurrected tab pullSnapshot rev=5 tabs={repo:["16c4a3e1","ff72768e","da21b76c"]} The client uploaded the session containing the tab; the user closed it; the kill RPC rejected so the close never reached the host; the host's snapshot still lists it; the client pulls it back and the merge restores it -- **correctly, by its own rule that the host is authoritative for what it knows.** A pane then mounts, respawns, and takes the recycled pty id. Client-side correlation from the same run, two controls and one positive in one run differing in exactly one variable: | Tab | Close events observed | Resurrected? | |---|---|---| | `6305cc07` | `user` + `pty-exit` | No | | `ed56f66c` | `user` + `pty-exit` | No | | `2036e760` | `user` only | **YES** | ## The fix `src/shared/closed-terminal-tab-tombstones.ts` (99 lines). A client-recorded close is first-party intent and must survive until the host acknowledges it. Per `docs/reference/ssh-execution-boundary.md` the remote verdict is `unverifiable` -- which may not authorise declaring the process dead, but equally must not authorise resurrecting the tab. This is SSH-v3 principle P2, "durable tombstones with a monotonic per-scope revision", reusing the existing `RemoteWorkspaceSnapshot.revision` rather than adding a twelfth per-tab identity field (the codebase carries eleven, 784 refs, that SSH-v3 Phase 3 deletes). - **Recorded** only on `closeReason === 'user'` (`terminal-tab-close.ts:69`). - **Suppresses** a host-sourced tab only when `tabId in tombstones && !currentTabsById.has(tabId)` -- a live local tab always wins, because deleting a live pane is the one outcome the merge exists to avoid. - **Retires** on positive acknowledgement: `!hostKnownTabIds.has(tabId) && hostRevision > observed`. Strictly newer, so a pull already in flight at close time cannot ack a close it predates. - Three never-retire guards: no revision retires nothing; a worktree the snapshot has no row for retires nothing; the first omitting snapshot only stamps the watermark. - TTL (30d) + cap (500) are **backstops** for a target the user never returns to, not the mechanism. - **Client-local only** -- never crosses the wire, so there is no mixed-version exposure. - Suppression is scoped to `replaceWorktreeIds`, which is what makes the live-tab check meaningful. A final whole-map sweep over the assembled `tabsByWorktree` would break that (a live tab is absent from `currentTabsById` outside the scope and would look suppressible); it is deliberately not there, and the comment at the top of the function says so. ## Evidence The load-bearing evidence is an A/B control on one tree, not the oracle's assertion. Flipping `isSuppressedByClose` to `false` -- one character -- reproduces the resurrection on demand: --repeat-each=2: 1) drop 2 resurrected the closed tab ab0e305d-…: baseline=1 drop1=1 drop2=2 2) drop 2 resurrected the closed tab 51533e34-…: baseline=1 drop1=1 drop2=2 2 failed With suppression on: **0 occurrences of "resurrected the closed tab" across five runs plus one independent run by a second agent.** Provenance verified positively, not by mtime: `closedTerminalTabTombstonesByTabId` appears 13x across 3 renderer chunks including `store-Do3KBvRE.js`; for every red control run `mayCreate` appeared 0 times in `out/main/index.js`. At the unit layer, disabling the same predicate: 3 failed | 39 passed. Restored: 42 passed; 287 across the workspace-session, terminal-store, remote-workspace, shared-tombstone and profile suites; 24 in the four tombstone suites. ## The oracle spec: GREEN in the full lane `ssh-lost-kill-tab-resurrection.spec.ts` passes both tests at this commit. Full Docker-SSH lane, clean tree: BUILD_SHA=49bb96e0b4c DIRTY=0 PROVENANCE tombstone=13 hasLocalTabsRow=2 hostAuthority=4 mayCreate=3 14 specs / 20 tests -> 17 passed, 2 failed, 1 skipped (10.7m) [12/20] :178 does not resurrect tabs whose kill was lost to a killed relay daemon PASSED [13/20] :190 does not resurrect tabs closed while the host is disconnected PASSED grep -c "resurrected the closed tab" (whole lane) -> 0 It passes WITHOUT PR 7 in the build (`mayCreate` present, `SshPtyAbsentFromRelayError` absent), so the bug-2 fix below is not required for it. Test 1 fails intermittently in ISOLATED single-spec runs, where a third defect blocks its cycle-2 setup. The resurrection assertion itself has never failed with this fix in place -- the intermittent failure is always a setup failure, never a resurrected tab. A reviewer running the spec alone may see it red; that is not this fix regressing. Three defects sit under STA-3374 and should not be conflated: - Bug 1 -- the closed tab resurrects. Fixed here. - Bug 2 -- `ssh-pty-session-reattach.ts:227-231` rewrites the relay's `PTY "pty-1" not found` into a bare `SSH_SESSION_EXPIRED`, so `isPtyAlreadyGoneError`'s `/PTY ".+" not found/` cannot match and `attachStablePaneOwner:242`'s already-correct fallback never runs. Owned by PR 7 (`nwparker/ssh-07-absent-from-relay`). Not required for the oracle above. - Bug 3 -- after the daemon is killed and the client launches a replacement, the client's OWN SSH transport drops and does not reconnect within 60s: no "delay step 2/9", no handshake failure, nothing. `ssh-connection.ts:1533` only logs on an SSH-level close. Unfixed, its own ticket. This is what makes test 1 intermittent in isolation. Discriminator for bug 3, measured in the isolated runs (the lane above ran without `ORCA_E2E_FORWARD_APP_LOGS=1`, so it was not re-confirmed there): `[ssh-relay] Socket probe result:` reads "DEAD" on every cycle of test 1 (daemon killed, a NEW relay must be launched) and "ALIVE" on every cycle of test 2 (daemon survived). Whenever a new daemon must be launched, the SSH transport drops afterwards and does not recover. An earlier reading blamed `kill.ts:82-84` for skipping `finishPtyShutdown` on a non-already-gone error. That was eliminated by direct test: the implied fix, `markSshRemotePtyLease(…, 'expired')` in that branch, was implemented, changed nothing, and was reverted rather than shipped unproven. Recorded so the path is not re-walked. The `SSH_SESSION_EXPIRED` rejection is real but fires during cycle 1 for the baseline pane, after which cycle 1 completes; the 60s silence begins only after `Relay channel lost ..., triggering reconnect`. The spec is claimed by the Docker-SSH lane, and that lane does not gate merges today. ## Persistence: the tombstone must survive a relaunch `closedTerminalTabTombstonesByTabId` is declared on `WorkspaceSessionState` but was missing from `workspaceSessionStateSchema` (`src/shared/workspace-session-schema.ts`), which is the load boundary for BOTH partitions -- `normalize-loaded-state-collections.ts` for `local` and `workspace-session-partitions.ts` for `ssh:`. Zod strips unknown keys and the write side does not validate, so the map reached disk and was discarded on the next launch. Measured with the repo's own parser: input : closedTerminalTabTombstonesByTabId: { 'tab-1': {...} } ok = true tombstones after parse = undefined That made the fix ineffective in the exact reported scenario: close an SSH tab with the transport down, QUIT, relaunch, reconnect -- the merge runs with an empty map, the host still lists the tab, and it resurrects. "Every day I open orca and it opens more tabs" is a claim about restarts. Neither the green oracle nor the A/B control could see it: both run entirely inside one app process. It also made the 30-day TTL and the 500 cap unreachable. Fixed by adding the field with a `salvagingRecord` matching its sibling `terminalSurfaceTombstonesByPaneKey`, so one malformed entry drops that entry rather than the map. `workspace-session-schema.ts` was one line under its 300-line max-lines limit, so adding the field required room rather than a suppression (the project forbids max-lines disables and per-file bumps). Two value schemas were extracted to modules named after what they contain: `terminal-tab-id-schema.ts` and `terminal-surface-tombstone-schema.ts`. The closed-tab tombstone's own schema is colocated with its type in `closed-terminal-tab-tombstones.ts`, which is where it belongs -- omitting it from the session schema is exactly the drift that caused this bug. `workspace-session-schema-field-coverage.test.ts` is the ratchet. Two sibling tables already pin themselves with `satisfies Record`; this schema had no such guard and is the one that fell behind. The new file adds both halves -- a `satisfies` list that makes a forgotten field a compile error, and a runtime assertion that names it -- plus a `parseWorkspaceSession` round-trip. Without the schema entry: 3 failed. With it: 3 passed. ## A host tab the user never closed could be deleted `tabId in closedTerminalTabTombstonesByTabId` answers true for every `Object.prototype` key even on an EMPTY map, because the map is a plain object from `Object.fromEntries`. A host tab whose id is `toString` was filtered from the reconciled list, blocked from the host-unknown branch, and stripped of its layout and session id. Tab ids are validated only as non-empty and colon-free, and `createTab` honours caller-supplied id hints, so the id is reachable rather than theoretical. This was the only path in either direction that could delete a tab the user never closed. Now `Object.hasOwn`, as the same file already uses elsewhere. Suppression is also scoped structurally: `isSuppressedByClose` compares the tombstone's stored `worktreeId`, which it already carried, so it cannot reach another workspace's tab. The two sweeps that have no worktree in scope (`terminalLayoutsByTabId`, `remoteSessionIdsByTabId`) now consult the set of ids this merge actually suppressed rather than re-deriving a verdict without that scope. The scope comment at the top of the function was also wrong and is corrected. It claimed every use of suppression sits inside `replaceWorktreeIds`; it does not -- the tabs pass walks all of `orderedWorktreeIds` and the two sweeps cover the whole remote maps. What actually makes it safe is that `closeTab` strips the id from every worktree row before recording the tombstone, plus the worktree match above, plus `closeReason === 'user'` being the only writer. Real guarantee, different from the documented one. ## Divergences from open PR #16571 #16571 implements the same concept. Three deliberate changes: 1. It never retires on acknowledgement -- TTL+cap only, so it never converges. Ack retirement added. 2. It crosses the wire and lets a HOST-sourced tombstone delete a LOCAL tab in a final whole-map sweep. After #14361 that is the wrong risk; dropped. This also removes the mixed-version regression its own body flags. 3. Its hydration unions rather than replaces the map -- a union resurrects every tombstone the merge just retired, so it never converges. Its `activeTabId` nulling is also dropped as redundant: `workspace-terminal-hydration.ts:99-105,126-138` already revalidates both pointers against the tab rows it just built, and nulling twice would add a second rule that has to stay in step with the first. ## Can a tab the user did NOT close disappear? No, but the guarantee needs stating precisely. The only writer is `recordClosedTerminalTabTombstone` (`terminal-tab-close.ts:69`), reachable only on `closeReason === 'user'`; suppression additionally requires the tab not be live locally. Reopen (`recently-closed-tabs.ts:122-166`) calls `createTab` and restores cwd/shell/title/color/position, never the old id. **Caveat, stated because the slogan is not literally true:** `createTab` honours a caller-supplied id hint (`terminal-tab-creation.ts:53-65`, used by `useIpcEvents` for host-admitted tabs), so "tab ids are uuids that never recur" does not hold in this codebase. The guarantee rests on the `closeReason === 'user'` writer plus the live-local-tab check, not on id uniqueness. ## Risk Renderer-side, client-local, no wire change. The blast radius is `mergeDirectSshRemoteWorkspaceSession` and the persisted session field. Worst case if the ack logic were wrong in the retiring direction: a tombstone outlives its usefulness and suppresses a host tab whose id the host re-issues -- bounded by the live-local-tab check, the 30d TTL and the 500 cap. Worst case in the other direction is today's behaviour. `profile-project-session-field-disposition.ts` records the new field as `notRepoScoped` / `notTransferred` residue, bounded by the same TTL and cap. ## Verify pnpm test src/shared/closed-terminal-tab-tombstones.test.ts \ src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts \ src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts \ src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts To reproduce the bug this fixes, set `isSuppressedByClose` to `() => false` in `remote-workspace-session-merge.ts` and run `pnpm test:e2e:ssh-docker -- tests/e2e/ssh-lost-kill-tab-resurrection.spec.ts --repeat-each=2`. --- config/scripts/run-ssh-docker-e2e.mjs | 1 + ...ofile-project-session-field-disposition.ts | 7 + ...ace-session-merge-close-tombstones.test.ts | 192 +++++++++++++++++ .../hooks/remote-workspace-session-merge.ts | 101 +++++++-- .../hooks/remote-workspace-snapshot-apply.ts | 3 +- .../workspace-session-browser-history.test.ts | 3 +- ...pace-session-closed-tab-tombstones.test.ts | 38 ++++ ...workspace-session-closed-tab-tombstones.ts | 10 + .../workspace-session-editor-drafts.test.ts | 1 + .../workspace-session-host-field-ownership.ts | 5 +- .../src/lib/workspace-session-host-split.ts | 4 +- .../lib/workspace-session-liveness.test.ts | 1 + .../src/lib/workspace-session-patch.ts | 6 + .../workspace-session-relevant-fields.test.ts | 1 + src/renderer/src/lib/workspace-session.ts | 6 + src/renderer/src/store/slices/terminals.ts | 1 + .../src/store/terminals/terminal-state.ts | 2 + .../terminal-tab-close-tombstone.test.ts | 80 +++++++ .../src/store/terminals/terminal-tab-close.ts | 22 ++ .../workspace-terminal-hydration-patch.ts | 4 + .../terminals/workspace-terminal-hydration.ts | 5 + .../closed-terminal-tab-tombstones.test.ts | 109 ++++++++++ src/shared/closed-terminal-tab-tombstones.ts | 111 ++++++++++ .../terminal-surface-tombstone-schema.ts | 14 ++ src/shared/terminal-tab-id-schema.ts | 10 + ...pace-session-schema-field-coverage.test.ts | 116 ++++++++++ src/shared/workspace-session-schema.ts | 21 +- src/shared/workspace-session-state-types.ts | 4 + .../ssh-lost-kill-tab-resurrection.spec.ts | 198 ++++++++++++++++++ 29 files changed, 1041 insertions(+), 35 deletions(-) create mode 100644 src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts create mode 100644 src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts create mode 100644 src/renderer/src/lib/workspace-session-closed-tab-tombstones.ts create mode 100644 src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts create mode 100644 src/shared/closed-terminal-tab-tombstones.test.ts create mode 100644 src/shared/closed-terminal-tab-tombstones.ts create mode 100644 src/shared/terminal-surface-tombstone-schema.ts create mode 100644 src/shared/terminal-tab-id-schema.ts create mode 100644 src/shared/workspace-session-schema-field-coverage.test.ts create mode 100644 tests/e2e/ssh-lost-kill-tab-resurrection.spec.ts diff --git a/config/scripts/run-ssh-docker-e2e.mjs b/config/scripts/run-ssh-docker-e2e.mjs index d5bab9aca0a..a723a9a6ad0 100644 --- a/config/scripts/run-ssh-docker-e2e.mjs +++ b/config/scripts/run-ssh-docker-e2e.mjs @@ -73,6 +73,7 @@ const result = spawnSync( 'tests/e2e/ssh-cold-hydration-gap-tab-seeding.spec.ts', 'tests/e2e/ssh-docker-reconnect-pane-restore.spec.ts', 'tests/e2e/ssh-external-image-preview.spec.ts', + 'tests/e2e/ssh-lost-kill-tab-resurrection.spec.ts', 'tests/e2e/ssh-pi-compatible-agent-title.spec.ts', 'tests/e2e/ssh-port-forward-lifecycle.spec.ts', 'tests/e2e/ssh-reconnect-tab-destruction.spec.ts', diff --git a/src/main/orca-profiles/profile-project-session-field-disposition.ts b/src/main/orca-profiles/profile-project-session-field-disposition.ts index d1f9dffa6c4..994a65af68a 100644 --- a/src/main/orca-profiles/profile-project-session-field-disposition.ts +++ b/src/main/orca-profiles/profile-project-session-field-disposition.ts @@ -121,6 +121,13 @@ export const WORKSPACE_SESSION_FIELD_DISPOSITION = { terminalSurfaceTombstonesByPaneKey: { onRepoRemoval: 'prunedByBespokeRule', onTransfer: 'copiedByBespokeRule' + }, + // Residue: keyed by tab id and pruned by neither path, like remoteSessionIdsByTabId. Bounded + // anyway -- every write and every pull merge runs it through the TTL and cap in + // shared/closed-terminal-tab-tombstones.ts, and a resolved host retires its own entries. + closedTerminalTabTombstonesByTabId: { + onRepoRemoval: 'notRepoScoped', + onTransfer: 'notTransferred' } } as const satisfies Record diff --git a/src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts b/src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts new file mode 100644 index 00000000000..a1e82633f98 --- /dev/null +++ b/src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts @@ -0,0 +1,192 @@ +import { describe, expect, it } from 'vitest' + +import { mergeDirectSshRemoteWorkspaceSession } from './remote-workspace-session-merge' +import { worktreeWorkspaceKey } from '../../../shared/workspace-scope' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import type { TerminalTab } from '../../../shared/terminal-tab-types' +import type { AppState } from '../store/types' + +/** + * The one thing a close tombstone is allowed to do, and everything it is not. + * + * The bug it exists for: the `pty.kill` for a closed SSH tab rejects with a transport error, so the + * close never reaches the host, the host keeps listing the tab, and every reconnect re-inserts it. + * The bug it must never cause is the opposite one — PR #14361 was reverted for deleting live tabs — + * so every case below that is not "an id this client watched the user close" keeps the tab. + */ +const WORKTREE = 'repo-1::/home/user/bug-cats' + +function terminalTab(id: string, overrides: Partial = {}): TerminalTab { + return { id, title: id, type: 'terminal', worktreeId: WORKTREE, ...overrides } as TerminalTab +} + +function sessionState(overrides: Partial = {}): WorkspaceSessionState { + return { + activeRepoId: 'repo-1', + activeWorktreeId: WORKTREE, + activeWorkspaceKey: worktreeWorkspaceKey(WORKTREE), + activeTabId: 'agent', + tabsByWorktree: {}, + terminalLayoutsByTabId: {}, + activeTabIdByWorktree: {}, + ...overrides + } as WorkspaceSessionState +} + +function merge( + current: WorkspaceSessionState, + remote: WorkspaceSessionState, + liveTabs: AppState['tabsByWorktree'] = {}, + remoteRevision?: number +): WorkspaceSessionState { + return mergeDirectSshRemoteWorkspaceSession( + current, + remote, + new Set([WORKTREE]), + liveTabs, + new Set(), + undefined, + remoteRevision + ) +} + +function tombstone(tabId: string, ackRevision?: number): WorkspaceSessionState { + return sessionState({ + tabsByWorktree: { [WORKTREE]: [] }, + closedTerminalTabTombstonesByTabId: { + [tabId]: { + closedAt: Date.now(), + worktreeId: WORKTREE, + ...(ackRevision ? { ackRevision } : {}) + } + } + }) +} + +describe('direct-SSH pull merge: closed-tab tombstones', () => { + it('does not suppress a host tab whose id collides with an Object.prototype key', () => { + // `tabId in map` answers true for every prototype key even on an EMPTY map, so a host tab named + // `toString` was filtered out, blocked from the host-unknown branch, and stripped of its layout + // and session id — the one path in this function that could delete a tab the user never closed. + // Tab ids are validated only as non-empty and colon-free, and createTab honours caller-supplied + // id hints, so the id is reachable rather than theoretical. + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [] } }) + const remote = sessionState({ + tabsByWorktree: { [WORKTREE]: [terminalTab('toString')] }, + terminalLayoutsByTabId: { toString: { root: { type: 'leaf', paneId: 'p' } } as never }, + remoteSessionIdsByTabId: { toString: 'session-1' } + }) + + const merged = merge(current, remote, {}, 2) + + expect(merged.tabsByWorktree[WORKTREE]?.map((tab) => tab.id)).toEqual(['toString']) + expect(Object.hasOwn(merged.terminalLayoutsByTabId, 'toString')).toBe(true) + expect(merged.remoteSessionIdsByTabId?.toString).toBe('session-1') + }) + + it('does not suppress a tombstoned id under a DIFFERENT worktree', () => { + // The tombstone carries the worktree it was closed in, so suppression cannot reach another + // workspace's tab. Structural, rather than a property of where the call sites happen to sit. + const other = 'repo-1::/home/user/other' + const current = sessionState({ + tabsByWorktree: { [WORKTREE]: [] }, + closedTerminalTabTombstonesByTabId: { + 'tab-x': { closedAt: Date.now(), worktreeId: other } + } + }) + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [terminalTab('tab-x')] } }) + + const merged = merge(current, remote, {}, 2) + + expect(merged.tabsByWorktree[WORKTREE]?.map((tab) => tab.id)).toEqual(['tab-x']) + }) + + it('does not re-insert a tombstoned tab the host still lists', () => { + const ghost = terminalTab('ghost') + const remote = sessionState({ + tabsByWorktree: { [WORKTREE]: [ghost] }, + terminalLayoutsByTabId: { ghost: { type: 'single', tabId: 'ghost' } as never }, + remoteSessionIdsByTabId: { ghost: 'pty-1' } + }) + + const merged = merge(tombstone('ghost'), remote, { [WORKTREE]: [] }, 3) + + expect(merged.tabsByWorktree[WORKTREE]).toEqual([]) + expect(merged.terminalLayoutsByTabId.ghost).toBeUndefined() + expect(merged.remoteSessionIdsByTabId?.ghost).toBeUndefined() + expect(merged.closedTerminalTabTombstonesByTabId?.ghost).toBeDefined() + }) + + it('does not re-add a tombstoned tab through the host-unknown branch', () => { + // The stale persisted payload still lists it, and the host has never heard of it. Without the + // tombstone that combination is exactly what the host-unknown branch exists to preserve. + const ghost = terminalTab('ghost') + const current = sessionState({ + tabsByWorktree: { [WORKTREE]: [ghost] }, + closedTerminalTabTombstonesByTabId: { ghost: { closedAt: Date.now(), worktreeId: WORKTREE } } + }) + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [] } }) + + const merged = merge(current, remote, {}, 3) + + expect(merged.tabsByWorktree[WORKTREE]).toEqual([]) + }) + + it('suppresses only the tombstoned tab, leaving its siblings and the pointers alone', () => { + // The active-tab pointers deliberately still name the ghost: hydration validates both of them + // against the rows below and nulls what they no longer name, so the merge leaves them be. + const ghost = terminalTab('ghost') + const survivor = terminalTab('survivor') + const remote = sessionState({ + tabsByWorktree: { [WORKTREE]: [ghost, survivor] }, + activeTabId: 'ghost', + activeTabIdByWorktree: { [WORKTREE]: 'ghost' } + }) + + const merged = merge(tombstone('ghost'), remote, { [WORKTREE]: [] }, 3) + + expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toEqual(['survivor']) + }) + + it('a live local tab beats a stale tombstone for its id', () => { + const live = terminalTab('live') + const current = sessionState({ + tabsByWorktree: { [WORKTREE]: [live] }, + closedTerminalTabTombstonesByTabId: { live: { closedAt: Date.now(), worktreeId: WORKTREE } } + }) + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [live] } }) + + const merged = merge(current, remote, { [WORKTREE]: [live] }, 3) + + expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toEqual(['live']) + }) + + it('leaves a tab alone when no tombstone names it', () => { + // The pre-existing absence-is-not-closed trade, restated: a host that still lists a tab closed + // on some other client keeps it here, because absence is all this merge would have to go on. + const agent = terminalTab('agent') + const closedElsewhere = terminalTab('closed-elsewhere') + const current = sessionState({ tabsByWorktree: { [WORKTREE]: [agent, closedElsewhere] } }) + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [agent] } }) + + const merged = merge(current, remote, { [WORKTREE]: [agent, closedElsewhere] }, 3) + + expect(merged.tabsByWorktree[WORKTREE].map((tab) => tab.id)).toContain('closed-elsewhere') + }) + + it('retires the tombstone once a newer snapshot stops listing the tab', () => { + const remote = sessionState({ tabsByWorktree: { [WORKTREE]: [] } }) + + const merged = merge(tombstone('ghost', 3), remote, { [WORKTREE]: [] }, 4) + + expect(merged.closedTerminalTabTombstonesByTabId).toBeUndefined() + }) + + it('keeps the tombstone when the snapshot omits the worktree entirely', () => { + // A snapshot that says nothing about the worktree — including one whose path never resolved — + // is not evidence the host saw the close. + const merged = merge(tombstone('ghost', 3), sessionState(), {}, 4) + + expect(merged.closedTerminalTabTombstonesByTabId?.ghost).toBeDefined() + }) +}) diff --git a/src/renderer/src/hooks/remote-workspace-session-merge.ts b/src/renderer/src/hooks/remote-workspace-session-merge.ts index 070b827d3ae..fa247d8bbbd 100644 --- a/src/renderer/src/hooks/remote-workspace-session-merge.ts +++ b/src/renderer/src/hooks/remote-workspace-session-merge.ts @@ -1,5 +1,6 @@ import type { TerminalTab } from '../../../shared/terminal-tab-types' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import { reconcileClosedTerminalTabTombstones } from '../../../shared/closed-terminal-tab-tombstones' import type { ExecutionHostId } from '../../../shared/execution-host' import { worktreeWorkspaceKey } from '../../../shared/workspace-scope' import { splitWorktreeId } from '../../../shared/worktree/id' @@ -26,8 +27,21 @@ export function mergeDirectSshRemoteWorkspaceSession( replaceWorktreeIds: ReadonlySet, liveTabsByWorktree: AppState['tabsByWorktree'], preserveLocalTerminalTabIds: ReadonlySet, - replaceExecutionHostId?: ExecutionHostId + replaceExecutionHostId?: ExecutionHostId, + remoteRevision?: number ): WorkspaceSessionState { + // Live tabs across the worktrees this snapshot replaces. Close-suppression consults it so a tab + // that is still live locally always beats its own tombstone. + // What actually keeps suppression from deleting a tab the user did not close — the earlier claim + // that "every use is inside replaceWorktreeIds" was wrong, since the tabsByWorktree pass below + // walks all of orderedWorktreeIds and the layout/session sweeps cover the whole remote maps: + // 1. closeTab strips the id from EVERY worktree row before recording the tombstone + // (terminal-tab-close.ts), so a tombstoned id is not live anywhere; + // 2. isSuppressedByClose matches the tombstone's own worktreeId, so it cannot reach another + // workspace's tab even if an id somehow recurred; + // 3. only closeReason === 'user' ever writes a tombstone. + // A final sweep of suppression over the assembled tabsByWorktree — including worktrees + // omitTargetWorktrees passes through verbatim — is still deliberately absent. const currentTabsById = new Map( [...replaceWorktreeIds] .flatMap((worktreeId) => liveTabsByWorktree[worktreeId] ?? []) @@ -96,22 +110,64 @@ export function mergeDirectSshRemoteWorkspaceSession( .filter(([tabId]) => remoteKnownTabIds.has(tabId)) .map(([, sessionId]) => sessionId) ) + // The other half of the trade below. Absence still cannot say "closed", so this says it instead: + // a tab THIS client watched the user close, whose close never reached the host. Only ids the user + // closed are ever in here, and only until the host's own snapshot stops listing them. + const closedTerminalTabTombstonesByTabId = reconcileClosedTerminalTabTombstones({ + tombstones: current.closedTerminalTabTombstonesByTabId, + acknowledgedWorktreeIds: new Set( + [...replaceWorktreeIds].filter((worktreeId) => + Object.hasOwn(remote.tabsByWorktree, worktreeId) + ) + ), + hostKnownTabIds: remoteKnownTabIds, + hostRevision: remoteRevision, + now: Date.now() + }) + // Why Object.hasOwn and not `in`: the map is a plain object from Object.fromEntries, so `in` + // answers true for every Object.prototype key on an EMPTY map — a host tab whose id is + // `toString` would be filtered, blocked from the host-unknown branch, and stripped of its layout + // and session id. Tab ids are validated only as non-empty and colon-free, and createTab honours + // caller-supplied id hints, so that id is reachable. This is the one place either direction could + // delete a tab the user never closed. + // Why the worktree comparison: the tombstone already carries the worktree it was closed in, so + // scoping on it makes "this suppression cannot reach another workspace's tab" structural rather + // than a property of where the call sites happen to sit. + // Why a live local tab still overrides its own tombstone: an id that is live here means the + // tombstone is stale (a close undone before it persisted), not a revival. Deleting a live pane is + // the one outcome this whole function exists to avoid. + const isSuppressedByClose = (tabId: string, worktreeId: string): boolean => + Object.hasOwn(closedTerminalTabTombstonesByTabId, tabId) && + closedTerminalTabTombstonesByTabId[tabId]?.worktreeId === worktreeId && + !currentTabsById.has(tabId) + // Ids this merge actually suppressed, recorded as it walks the worktrees. The layout and + // session-id sweeps below have no worktree in scope, so they consult decisions already made + // rather than re-deriving one without the scope that makes it safe. + const suppressedTabIds = new Set() const tabsByWorktree = Object.fromEntries( orderedWorktreeIds.map((worktreeId) => { const remoteTabs = remote.tabsByWorktree[worktreeId] ?? [] - const reconciled = remoteTabs.map((tab) => { - const local = currentTabsById.get(tab.id) - if ( - !local || - ((local.generation ?? 0) <= (tab.generation ?? 0) && - !local.pendingActivationSpawn && - !preserveLocalTerminalTabIds.has(tab.id)) - ) { - return tab - } - locallyPreservedTabIds.add(tab.id) - return preserveNewerLocalTerminalFields(tab, local) - }) + const reconciled = remoteTabs + .filter((tab) => { + if (!isSuppressedByClose(tab.id, worktreeId)) { + return true + } + suppressedTabIds.add(tab.id) + return false + }) + .map((tab) => { + const local = currentTabsById.get(tab.id) + if ( + !local || + ((local.generation ?? 0) <= (tab.generation ?? 0) && + !local.pendingActivationSpawn && + !preserveLocalTerminalTabIds.has(tab.id)) + ) { + return tab + } + locallyPreservedTabIds.add(tab.id) + return preserveNewerLocalTerminalFields(tab, local) + }) for (const tab of reconciled) { emittedTabIds.add(tab.id) } @@ -135,6 +191,10 @@ export function mergeDirectSshRemoteWorkspaceSession( if (remoteKnownTabIds.has(tab.id) || emittedTabIds.has(tab.id)) { return false } + if (isSuppressedByClose(tab.id, worktreeId)) { + suppressedTabIds.add(tab.id) + return false + } const localSessionId = current.remoteSessionIdsByTabId?.[tab.id] return localSessionId == null || !remoteKnownSessionIds.has(localSessionId) }) @@ -186,7 +246,7 @@ export function mergeDirectSshRemoteWorkspaceSession( ), ...Object.fromEntries( Object.entries(remote.terminalLayoutsByTabId).filter( - ([tabId]) => !locallyPreservedTabIds.has(tabId) + ([tabId]) => !locallyPreservedTabIds.has(tabId) && !suppressedTabIds.has(tabId) ) ) } @@ -243,6 +303,9 @@ export function mergeDirectSshRemoteWorkspaceSession( return localActiveTabId == null ? [] : [[worktreeId, localActiveTabId] as const] }) ), + // Why a suppressed id is left in place here: hydration validates both active-tab pointers + // against the tab rows it just built and nulls anything they no longer name, so nulling twice + // would only add a second rule that has to stay in step with that one. ...remote.activeTabIdByWorktree }, remoteSessionIdsByTabId: { @@ -253,7 +316,7 @@ export function mergeDirectSshRemoteWorkspaceSession( ), ...Object.fromEntries( Object.entries(remote.remoteSessionIdsByTabId ?? {}).filter( - ([tabId]) => !locallyPreservedTabIds.has(tabId) + ([tabId]) => !locallyPreservedTabIds.has(tabId) && !suppressedTabIds.has(tabId) ) ) }, @@ -268,7 +331,11 @@ export function mergeDirectSshRemoteWorkspaceSession( // tabs. Removal is the worktree-teardown path's job, not a reconnect's. ...current.defaultTerminalTabsAppliedByWorktreeId, ...remote.defaultTerminalTabsAppliedByWorktreeId - } + }, + closedTerminalTabTombstonesByTabId: + Object.keys(closedTerminalTabTombstonesByTabId).length > 0 + ? closedTerminalTabTombstonesByTabId + : undefined } } diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts b/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts index 16929fd6379..dfb626d5339 100644 --- a/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts +++ b/src/renderer/src/hooks/remote-workspace-snapshot-apply.ts @@ -119,7 +119,8 @@ export async function applyDirectSshRemoteWorkspaceSnapshot({ worktreeIds, state.tabsByWorktree, currentRecoveryTabIds(state, authority, worktreeIds), - toSshExecutionHostId(authority.targetId) + toSshExecutionHostId(authority.targetId), + snapshot.revision ) if (!isArrivalCurrent(authority.targetId, arrival) || !isPreparationTokenCurrent(token)) { return diff --git a/src/renderer/src/lib/workspace-session-browser-history.test.ts b/src/renderer/src/lib/workspace-session-browser-history.test.ts index c8c4c53480e..152642f65ae 100644 --- a/src/renderer/src/lib/workspace-session-browser-history.test.ts +++ b/src/renderer/src/lib/workspace-session-browser-history.test.ts @@ -32,7 +32,8 @@ function createSnapshot(browserUrlHistory: BrowserHistoryEntry[]): WorkspaceSess worktreesByRepo: {}, lastKnownRelayPtyIdByTabId: {}, lastVisitedAtByWorktreeId: {}, - defaultTerminalTabsAppliedByWorktreeId: {} + defaultTerminalTabsAppliedByWorktreeId: {}, + closedTerminalTabTombstonesByTabId: {} } } diff --git a/src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts b/src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts new file mode 100644 index 00000000000..522b079aff6 --- /dev/null +++ b/src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from 'vitest' +import { buildWorkspaceSessionPatch } from './workspace-session-patch' +import { SESSION_RELEVANT_FIELDS } from './workspace-session' +import { CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS } from '../../../shared/closed-terminal-tab-tombstones' + +// Why browserPagesByWorkspace is stubbed: buildWorkspaceSessionPatch pre-filters staged browser tabs +// before field dispatch, so the minimal fixture has to carry the map it iterates. +function patchFor( + map: Record +): ReturnType { + return buildWorkspaceSessionPatch( + { browserPagesByWorkspace: {}, closedTerminalTabTombstonesByTabId: map } as never, + ['closedTerminalTabTombstonesByTabId'] + ) +} + +describe('closed-tab tombstones in the persisted session', () => { + it('is a session-relevant field, so a close on its own schedules a write', () => { + expect(SESSION_RELEVANT_FIELDS).toContain('closedTerminalTabTombstonesByTabId') + }) + + it('writes live tombstones and drops ones past the TTL', () => { + const now = Date.now() + expect( + patchFor({ + fresh: { closedAt: now, worktreeId: 'repo-1::/srv/app' }, + stale: { + closedAt: now - CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS - 1, + worktreeId: 'repo-1::/srv/app' + } + }).closedTerminalTabTombstonesByTabId + ).toEqual({ fresh: { closedAt: now, worktreeId: 'repo-1::/srv/app' } }) + }) + + it('omits the field entirely once the map empties', () => { + expect(patchFor({}).closedTerminalTabTombstonesByTabId).toBeUndefined() + }) +}) diff --git a/src/renderer/src/lib/workspace-session-closed-tab-tombstones.ts b/src/renderer/src/lib/workspace-session-closed-tab-tombstones.ts new file mode 100644 index 00000000000..48b1cb1a727 --- /dev/null +++ b/src/renderer/src/lib/workspace-session-closed-tab-tombstones.ts @@ -0,0 +1,10 @@ +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import { pruneClosedTerminalTabTombstones } from '../../../shared/closed-terminal-tab-tombstones' + +/** Applies the TTL/cap bound on the way to disk, and omits an empty map like its siblings. */ +export function buildPersistedClosedTerminalTabTombstones( + map: WorkspaceSessionState['closedTerminalTabTombstonesByTabId'] +): WorkspaceSessionState['closedTerminalTabTombstonesByTabId'] { + const pruned = pruneClosedTerminalTabTombstones(map, Date.now()) + return Object.keys(pruned).length > 0 ? pruned : undefined +} diff --git a/src/renderer/src/lib/workspace-session-editor-drafts.test.ts b/src/renderer/src/lib/workspace-session-editor-drafts.test.ts index ff740b6f5f8..c1292a9c74d 100644 --- a/src/renderer/src/lib/workspace-session-editor-drafts.test.ts +++ b/src/renderer/src/lib/workspace-session-editor-drafts.test.ts @@ -34,6 +34,7 @@ function createSnapshot( lastKnownRelayPtyIdByTabId: {}, lastVisitedAtByWorktreeId: {}, defaultTerminalTabsAppliedByWorktreeId: {}, + closedTerminalTabTombstonesByTabId: {}, ...overrides } } diff --git a/src/renderer/src/lib/workspace-session-host-field-ownership.ts b/src/renderer/src/lib/workspace-session-host-field-ownership.ts index b24e7118c02..0303fda7d4d 100644 --- a/src/renderer/src/lib/workspace-session-host-field-ownership.ts +++ b/src/renderer/src/lib/workspace-session-host-field-ownership.ts @@ -49,7 +49,10 @@ export const WORKSPACE_SESSION_FIELD_OWNERSHIP = { terminalPtyIncarnationsByPaneKey: 'paneKeyed', // Why: this host-issued fence must never collide while unified renderer state merges equal repo ids across hosts. terminalTopologyRevisionByRepoId: 'hostPrivate', - terminalSurfaceTombstonesByPaneKey: 'surfaceTombstoneKeyed' + terminalSurfaceTombstonesByPaneKey: 'surfaceTombstoneKeyed', + // Why not tabKeyed: the tab is already gone, so worktreeIdByTabId can never resolve it. Routing by + // the record's own worktreeId is the same problem terminalSurfaceTombstonesByPaneKey has. + closedTerminalTabTombstonesByTabId: 'surfaceTombstoneKeyed' } as const satisfies Record // Why: an unclassified persisted field would otherwise disappear from every non-local host. diff --git a/src/renderer/src/lib/workspace-session-host-split.ts b/src/renderer/src/lib/workspace-session-host-split.ts index ab374474b73..ab9325621f4 100644 --- a/src/renderer/src/lib/workspace-session-host-split.ts +++ b/src/renderer/src/lib/workspace-session-host-split.ts @@ -49,7 +49,9 @@ export type HostIdByWorktreeId = (worktreeId: string) => ExecutionHostId * - browserWorkspaceKeyed: Record keyed by browser-workspace id; follows the * page record's own worktreeId. * - fileKeyed: Record keyed by editor file id; follows the open file's worktree. - * - sleepingAgentKeyed: Record keyed by pane key; follows the record's worktreeId. */ + * - sleepingAgentKeyed: Record keyed by pane key; follows the record's worktreeId. + * - surfaceTombstoneKeyed: Record under an opaque key whose value names its own worktreeId -- + * the only routing available once the tab or pane it describes is gone. */ type SplitContext = { hostIdByWorktreeId: HostIdByWorktreeId worktreeIdByTabId: Map diff --git a/src/renderer/src/lib/workspace-session-liveness.test.ts b/src/renderer/src/lib/workspace-session-liveness.test.ts index f714230c2f9..6c10d2d0c54 100644 --- a/src/renderer/src/lib/workspace-session-liveness.test.ts +++ b/src/renderer/src/lib/workspace-session-liveness.test.ts @@ -33,6 +33,7 @@ function createSnapshot( lastKnownRelayPtyIdByTabId: {}, lastVisitedAtByWorktreeId: {}, defaultTerminalTabsAppliedByWorktreeId: {}, + closedTerminalTabTombstonesByTabId: {}, ...overrides } } diff --git a/src/renderer/src/lib/workspace-session-patch.ts b/src/renderer/src/lib/workspace-session-patch.ts index 5558aab409a..b67d7f3ebe4 100644 --- a/src/renderer/src/lib/workspace-session-patch.ts +++ b/src/renderer/src/lib/workspace-session-patch.ts @@ -19,6 +19,7 @@ import { withoutStagedBrowserTabs } from './workspace-session-staged-browser-tab import { buildPersistedUnifiedTabSessionData } from './workspace-session-unified-tabs' import { buildLastVisitedAtByWorktreeId } from './workspace-session-focus-recency' import { buildSleepingAgentSessionData } from './workspace-session-sleeping-agents' +import { buildPersistedClosedTerminalTabTombstones } from './workspace-session-closed-tab-tombstones' type SessionRelevantField = keyof WorkspaceSessionSnapshot @@ -160,6 +161,11 @@ export function buildWorkspaceSessionPatch( ? snapshot.defaultTerminalTabsAppliedByWorktreeId : undefined } + if (changed.has('closedTerminalTabTombstonesByTabId')) { + patch.closedTerminalTabTombstonesByTabId = buildPersistedClosedTerminalTabTombstones( + snapshot.closedTerminalTabTombstonesByTabId + ) + } if (changed.has('sleepingAgentSessionsByPaneKey')) { patch.sleepingAgentSessionsByPaneKey = buildSleepingAgentSessionData(snapshot).sleepingAgentSessionsByPaneKey diff --git a/src/renderer/src/lib/workspace-session-relevant-fields.test.ts b/src/renderer/src/lib/workspace-session-relevant-fields.test.ts index abed4b44e01..e20f4195d3e 100644 --- a/src/renderer/src/lib/workspace-session-relevant-fields.test.ts +++ b/src/renderer/src/lib/workspace-session-relevant-fields.test.ts @@ -34,6 +34,7 @@ describe('SESSION_RELEVANT_FIELDS', () => { lastKnownRelayPtyIdByTabId: true, lastVisitedAtByWorktreeId: true, defaultTerminalTabsAppliedByWorktreeId: true, + closedTerminalTabTombstonesByTabId: true, sleepingAgentSessionsByPaneKey: true, clientHostedBrowserCloseIntentsByEnvironment: true } diff --git a/src/renderer/src/lib/workspace-session.ts b/src/renderer/src/lib/workspace-session.ts index 59debdfc9a7..73eaa482606 100644 --- a/src/renderer/src/lib/workspace-session.ts +++ b/src/renderer/src/lib/workspace-session.ts @@ -10,6 +10,7 @@ import type { OpenFile } from '../store/slices/editor' import { buildPersistedUnifiedTabSessionData } from './workspace-session-unified-tabs' import { buildLastVisitedAtByWorktreeId } from './workspace-session-focus-recency' import { buildSleepingAgentSessionData } from './workspace-session-sleeping-agents' +import { buildPersistedClosedTerminalTabTombstones } from './workspace-session-closed-tab-tombstones' import { buildActiveConnectionIdsAtShutdown } from './workspace-session-reconnect-targets' import { withoutStagedBrowserTabs } from './workspace-session-staged-browser-tabs' import { buildBrowserSessionData } from './workspace-session-browser-tabs' @@ -53,6 +54,7 @@ export type WorkspaceSessionSnapshot = Pick< | 'lastKnownRelayPtyIdByTabId' | 'lastVisitedAtByWorktreeId' | 'defaultTerminalTabsAppliedByWorktreeId' + | 'closedTerminalTabTombstonesByTabId' > & { activeWorkspaceExecutionHostId?: AppState['activeWorkspaceExecutionHostId'] sleepingAgentSessionsByPaneKey?: AppState['sleepingAgentSessionsByPaneKey'] @@ -90,6 +92,7 @@ export const SESSION_RELEVANT_FIELDS = [ 'lastKnownRelayPtyIdByTabId', 'lastVisitedAtByWorktreeId', 'defaultTerminalTabsAppliedByWorktreeId', + 'closedTerminalTabTombstonesByTabId', 'sleepingAgentSessionsByPaneKey', 'clientHostedBrowserCloseIntentsByEnvironment' ] as const satisfies readonly (keyof WorkspaceSessionSnapshot)[] @@ -303,6 +306,9 @@ export function buildWorkspaceSessionPayload( Object.keys(snapshot.defaultTerminalTabsAppliedByWorktreeId).length > 0 ? snapshot.defaultTerminalTabsAppliedByWorktreeId : undefined, + closedTerminalTabTombstonesByTabId: buildPersistedClosedTerminalTabTombstones( + snapshot.closedTerminalTabTombstonesByTabId + ), ...buildSleepingAgentSessionData(snapshot), // Why unconditional rather than omit-when-empty: a full write replaces the persisted object, // so an emptied map has to be written as empty or the last replay never sticks. diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 4286dd0ac53..1e1a4b3e699 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -49,6 +49,7 @@ export const createTerminalSlice: StateCreator workspaceSessionReady: false, restoredRuntimeHostIdByWorkspaceSessionKey: {}, defaultTerminalTabsAppliedByWorktreeId: {}, + closedTerminalTabTombstonesByTabId: {}, hydrationSucceeded: false, pendingReconnectWorktreeIds: [], pendingReconnectTabByWorktree: {}, diff --git a/src/renderer/src/store/terminals/terminal-state.ts b/src/renderer/src/store/terminals/terminal-state.ts index 5f754cc6463..4d5cbea8256 100644 --- a/src/renderer/src/store/terminals/terminal-state.ts +++ b/src/renderer/src/store/terminals/terminal-state.ts @@ -1,3 +1,4 @@ +import type { ClosedTerminalTabTombstonesByTabId } from '../../../../shared/closed-terminal-tab-tombstones' import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../shared/terminal-tab-types' import type { TuiAgent } from '../../../../shared/tui-agent' import type { SetupSplitDirection } from '../../../../shared/worktree/launch-types' @@ -90,6 +91,7 @@ export type TerminalState = { workspaceSessionReady: boolean restoredRuntimeHostIdByWorkspaceSessionKey: Record defaultTerminalTabsAppliedByWorktreeId: Record + closedTerminalTabTombstonesByTabId: ClosedTerminalTabTombstonesByTabId hydrationSucceeded: boolean pendingReconnectWorktreeIds: string[] pendingReconnectTabByWorktree: Record diff --git a/src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts b/src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts new file mode 100644 index 00000000000..8a7d5039c8a --- /dev/null +++ b/src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts @@ -0,0 +1,80 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import type * as AgentStatusModule from '@/lib/agent-status' +import { createTestStore, makeTab, makeWorktree, seedStore } from '../slices/store-test-helpers' +import { createStoreCascadesMockApi } from '../slices/store-cascades-test-harness' + +vi.mock('sonner', () => ({ + toast: { info: vi.fn(), success: vi.fn(), error: vi.fn(), warning: vi.fn() } +})) + +vi.mock('@/components/terminal-pane/pty-dispatcher', () => ({ + restorePtyDataHandlersAfterFailedShutdown: vi.fn(), + unregisterPtyDataHandlers: vi.fn<() => unknown[]>(() => []) +})) + +vi.mock('@/lib/agent-status', async (importOriginal) => ({ + ...(await importOriginal()), + detectAgentStatusFromTitle: vi.fn().mockReturnValue(null) +})) + +const mockApi = createStoreCascadesMockApi() + +const REMOTE_WORKTREE = 'remote-repo::/srv/app' +const LOCAL_WORKTREE = 'local-repo::/tmp/app' + +function storeWithBothWorktrees(): ReturnType { + const store = createTestStore() + seedStore(store, { + repos: [ + { id: 'remote-repo', path: '/srv/app', name: 'app', connectionId: 'ssh-1' }, + { id: 'local-repo', path: '/tmp/app', name: 'app' } + ] as never, + worktreesByRepo: { + 'remote-repo': [ + makeWorktree({ id: REMOTE_WORKTREE, repoId: 'remote-repo', path: '/srv/app' }) + ], + 'local-repo': [makeWorktree({ id: LOCAL_WORKTREE, repoId: 'local-repo', path: '/tmp/app' })] + }, + tabsByWorktree: { + [REMOTE_WORKTREE]: [makeTab({ id: 'remote-tab', worktreeId: REMOTE_WORKTREE })], + [LOCAL_WORKTREE]: [makeTab({ id: 'local-tab', worktreeId: LOCAL_WORKTREE })] + } + }) + return store +} + +describe('closeTab close tombstones', () => { + beforeEach(() => { + vi.clearAllMocks() + mockApi.worktrees.updateMeta.mockResolvedValue({}) + }) + + it('records the closed tab against the worktree it was closed in', () => { + const store = storeWithBothWorktrees() + + store.getState().closeTab('remote-tab') + + expect(store.getState().closedTerminalTabTombstonesByTabId['remote-tab']).toEqual({ + closedAt: expect.any(Number), + worktreeId: REMOTE_WORKTREE + }) + }) + + // A tombstone outlives the host's own record, so the only claim it may ever make is "the user + // closed this". Neither of these closes is that claim. + it.each([['pty-exit'], ['cleanup']] as const)('records nothing for a %s close', (reason) => { + const store = storeWithBothWorktrees() + + store.getState().closeTab('remote-tab', { reason }) + + expect(store.getState().closedTerminalTabTombstonesByTabId['remote-tab']).toBeUndefined() + }) + + it('records nothing for a definitively local worktree, which no pull merge ever reads', () => { + const store = storeWithBothWorktrees() + + store.getState().closeTab('local-tab') + + expect(store.getState().closedTerminalTabTombstonesByTabId).toEqual({}) + }) +}) diff --git a/src/renderer/src/store/terminals/terminal-tab-close.ts b/src/renderer/src/store/terminals/terminal-tab-close.ts index cea5f0bc474..cda4303ebd0 100644 --- a/src/renderer/src/store/terminals/terminal-tab-close.ts +++ b/src/renderer/src/store/terminals/terminal-tab-close.ts @@ -1,4 +1,6 @@ +import { recordClosedTerminalTabTombstone } from '../../../../shared/closed-terminal-tab-tombstones' import type { TerminalTab } from '../../../../shared/terminal-tab-types' +import { getConnectionIdFromState } from '@/lib/connection-owner-resolution' import { sweepRetiredTerminalTabState } from '../slices/retired-terminal-tab-state-sweep' import { getRecentlyClosedTabPosition, @@ -58,6 +60,23 @@ export function createTerminalTabCloseActions( next[wId] = after } } + // Why `user` and not retiresSession: a tombstone outlives the host's own record, so the only + // thing it may ever say is "the user closed this". A pty-exit close is the process ending, + // and a `cleanup` close retires a tab the app itself created — neither is that claim. + // Why a non-local worktree only: the tombstone is read solely by the direct-SSH pull merge, + // and a definitively local tab would just consume the map's cap. An unresolved repo + // (undefined, not null) still records — losing the tombstone reinstates the resurrection. + const nextClosedTombstones = + closeReason === 'user' && + closedWorktreeId && + getConnectionIdFromState(s, closedWorktreeId) !== null + ? recordClosedTerminalTabTombstone( + s.closedTerminalTabTombstonesByTabId, + tabId, + closedWorktreeId, + Date.now() + ) + : s.closedTerminalTabTombstonesByTabId // Why: only explicit user closes feed the Cmd+Shift+T reopen stack; cleanup/PTY-exit closes must not pollute undo history. const closedPosition = closedWorktreeId && closedTab @@ -207,6 +226,9 @@ export function createTerminalTabCloseActions( ...(nextSleepingAgentSessionsByPaneKey !== s.sleepingAgentSessionsByPaneKey ? { sleepingAgentSessionsByPaneKey: nextSleepingAgentSessionsByPaneKey } : {}), + ...(nextClosedTombstones !== s.closedTerminalTabTombstonesByTabId + ? { closedTerminalTabTombstonesByTabId: nextClosedTombstones } + : {}), // Why: skip writing unreadTerminalTabs when unchanged to avoid a no-op state allocation that re-evaluates full-state selectors. Mirrors tabs.ts. ...(nextUnreadTerminalTabs !== s.unreadTerminalTabs ? { unreadTerminalTabs: nextUnreadTerminalTabs } diff --git a/src/renderer/src/store/terminals/workspace-terminal-hydration-patch.ts b/src/renderer/src/store/terminals/workspace-terminal-hydration-patch.ts index 46fb906a9b9..6da518e019b 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-hydration-patch.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-hydration-patch.ts @@ -19,6 +19,7 @@ export type WorkspaceHydrationPatch = Pick< | 'worktreesByRepo' | 'lastVisitedAtByWorktreeId' | 'defaultTerminalTabsAppliedByWorktreeId' + | 'closedTerminalTabTombstonesByTabId' | 'automaticAgentResumeClaimsByTabId' | 'sleepingAgentSessionsByPaneKey' | 'pendingReconnectWorktreeIds' @@ -176,6 +177,9 @@ export function targetScopedWorkspaceHydrationPatch( hydrated.defaultTerminalTabsAppliedByWorktreeId, workspaceKeys ), + // Why passed through whole: hydration already unioned it with live store state, and the map is + // keyed by tab id rather than by workspace key so replaceHydratedRecordKeys has nothing to match. + closedTerminalTabTombstonesByTabId: hydrated.closedTerminalTabTombstonesByTabId, automaticAgentResumeClaimsByTabId: replaceHydratedRecordKeys( state.automaticAgentResumeClaimsByTabId, hydrated.automaticAgentResumeClaimsByTabId, diff --git a/src/renderer/src/store/terminals/workspace-terminal-hydration.ts b/src/renderer/src/store/terminals/workspace-terminal-hydration.ts index 6d6169a22f4..efa70c8b480 100644 --- a/src/renderer/src/store/terminals/workspace-terminal-hydration.ts +++ b/src/renderer/src/store/terminals/workspace-terminal-hydration.ts @@ -178,6 +178,11 @@ export function createWorkspaceTerminalHydrationActions( lastVisitedAtByWorktreeId: session.lastVisitedAtByWorktreeId ?? {}, defaultTerminalTabsAppliedByWorktreeId: session.defaultTerminalTabsAppliedByWorktreeId ?? {}, + // Why replace and not union: both callers hand over a map they derived from this store + // synchronously (the pull merge) or from disk before the store had one (startup), so there + // is no local tombstone to lose — and a union would resurrect the ones the merge just + // retired on the host's acknowledgement, which is the whole bound on this map. + closedTerminalTabTombstonesByTabId: session.closedTerminalTabTombstonesByTabId ?? {}, automaticAgentResumeClaimsByTabId: {}, sleepingAgentSessionsByPaneKey, pendingReconnectWorktreeIds, diff --git a/src/shared/closed-terminal-tab-tombstones.test.ts b/src/shared/closed-terminal-tab-tombstones.test.ts new file mode 100644 index 00000000000..a2db1828ac3 --- /dev/null +++ b/src/shared/closed-terminal-tab-tombstones.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it } from 'vitest' +import { + CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS, + MAX_CLOSED_TERMINAL_TAB_TOMBSTONES, + pruneClosedTerminalTabTombstones, + recordClosedTerminalTabTombstone, + reconcileClosedTerminalTabTombstones, + type ClosedTerminalTabTombstonesByTabId +} from './closed-terminal-tab-tombstones' + +const NOW = 1_800_000_000_000 +const WT = 'repo-1::/srv/app' + +function reconcile( + tombstones: ClosedTerminalTabTombstonesByTabId, + args: { + acknowledgedWorktreeIds?: string[] + hostKnownTabIds?: string[] + hostRevision?: number + } +): ClosedTerminalTabTombstonesByTabId { + return reconcileClosedTerminalTabTombstones({ + tombstones, + acknowledgedWorktreeIds: new Set(args.acknowledgedWorktreeIds ?? [WT]), + hostKnownTabIds: new Set(args.hostKnownTabIds ?? []), + hostRevision: args.hostRevision, + now: NOW + }) +} + +describe('closed terminal tab tombstones', () => { + it('records the closing worktree and time', () => { + expect(recordClosedTerminalTabTombstone({}, 'tab-1', WT, NOW)).toEqual({ + 'tab-1': { closedAt: NOW, worktreeId: WT } + }) + }) + + it('prunes past the TTL and caps at the newest entries', () => { + expect( + pruneClosedTerminalTabTombstones( + { + fresh: { closedAt: NOW - 1_000, worktreeId: WT }, + stale: { closedAt: NOW - CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS - 1, worktreeId: WT } + }, + NOW + ) + ).toEqual({ fresh: { closedAt: NOW - 1_000, worktreeId: WT } }) + + const overflowing = Object.fromEntries( + Array.from({ length: MAX_CLOSED_TERMINAL_TAB_TOMBSTONES + 10 }, (_, index) => [ + `tab-${index}`, + { closedAt: NOW - index, worktreeId: WT } + ]) + ) + const capped = pruneClosedTerminalTabTombstones(overflowing, NOW) + expect(Object.keys(capped)).toHaveLength(MAX_CLOSED_TERMINAL_TAB_TOMBSTONES) + expect(capped['tab-0']).toBeDefined() + expect(capped[`tab-${MAX_CLOSED_TERMINAL_TAB_TOMBSTONES + 9}`]).toBeUndefined() + }) +}) + +describe('closed terminal tab tombstone acknowledgement', () => { + const tombstones = { 'tab-1': { closedAt: NOW, worktreeId: WT } } + + it('a snapshot carrying no revision acknowledges nothing', () => { + expect(reconcile(tombstones, { hostRevision: undefined })).toEqual(tombstones) + }) + + it('a snapshot with no row for the tombstone worktree acknowledges nothing', () => { + const kept = reconcile(tombstones, { acknowledgedWorktreeIds: [], hostRevision: 9 }) + expect(kept['tab-1']).toEqual({ closedAt: NOW, worktreeId: WT }) + }) + + it('the first omitting snapshot only stamps the revision — it could predate the close', () => { + const kept = reconcile(tombstones, { hostRevision: 4 }) + expect(kept['tab-1']).toEqual({ closedAt: NOW, worktreeId: WT, ackRevision: 4 }) + }) + + it('a strictly newer omitting snapshot retires the tombstone', () => { + const stamped = reconcile(tombstones, { hostRevision: 4 }) + expect(reconcile(stamped, { hostRevision: 5 })['tab-1']).toBeUndefined() + }) + + it('a repeat of the same revision does not retire it', () => { + const stamped = reconcile(tombstones, { hostRevision: 4 }) + expect(reconcile(stamped, { hostRevision: 4 })['tab-1']).toBeDefined() + }) + + it('a host that still lists the tab re-arms the watermark instead of retiring', () => { + const stamped = reconcile(tombstones, { hostRevision: 4 }) + const stillListed = reconcile(stamped, { hostRevision: 7, hostKnownTabIds: ['tab-1'] }) + expect(stillListed['tab-1']).toEqual({ closedAt: NOW, worktreeId: WT, ackRevision: 7 }) + expect(reconcile(stillListed, { hostRevision: 7 })['tab-1']).toBeDefined() + expect(reconcile(stillListed, { hostRevision: 8 })['tab-1']).toBeUndefined() + }) + + it('a host revision that went backwards never retires', () => { + const stamped = reconcile(tombstones, { hostRevision: 40 }) + const rewound = reconcile(stamped, { hostRevision: 1 }) + expect(rewound['tab-1']).toEqual({ closedAt: NOW, worktreeId: WT, ackRevision: 40 }) + }) + + it('a tab listed under a different worktree still counts as known to the host', () => { + const stamped = reconcile(tombstones, { hostRevision: 4 }) + expect( + reconcile(stamped, { hostRevision: 5, hostKnownTabIds: ['tab-1'] })['tab-1'] + ).toBeDefined() + }) +}) diff --git a/src/shared/closed-terminal-tab-tombstones.ts b/src/shared/closed-terminal-tab-tombstones.ts new file mode 100644 index 00000000000..cb4d822e5da --- /dev/null +++ b/src/shared/closed-terminal-tab-tombstones.ts @@ -0,0 +1,111 @@ +import { z } from 'zod' + +/** A client's own record that the user closed a terminal tab. + * + * Why it has to exist: the direct-SSH pull merge cannot tell "the host was never told about this + * tab" from "the user closed it" by absence alone, so it keeps the tab (see the trade documented in + * remote-workspace-session-merge.ts). When the `pty.kill` RPC rejects with a transport error the + * close never reaches the host at all, the host keeps listing the tab, and every reconnect + * re-inserts it. The tombstone is the missing close signal — first-party intent that outlives the + * failed RPC — so the merge can drop that one tab without ever deleting a tab the host merely has + * not heard about yet. + * + * Tab ids are uuids and reopen mints a fresh one, so a tombstoned id never legitimately returns. */ +export type ClosedTerminalTabTombstone = { + closedAt: number + worktreeId: string + /** Newest host revision seen for this tab's scope since the close. Retirement needs a STRICTLY + * newer snapshot that omits the tab, so a pull already in flight when the user closed cannot + * acknowledge a close it predates. */ + ackRevision?: number +} + +export type ClosedTerminalTabTombstonesByTabId = Record + +/** Colocated with the type so the two cannot drift. Must survive a relaunch: the whole point of a + * tombstone is that it outlives a `pty.kill` the transport never delivered, and a restart is the + * common case — it was omitted from the session schema once, and the map was silently stripped on + * every launch. */ +export const closedTerminalTabTombstoneSchema = z.object({ + closedAt: z.number().int().nonnegative(), + worktreeId: z.string().min(1), + ackRevision: z.number().int().nonnegative().optional() +}) + +/** Backstops only — host acknowledgement is the normal exit. These cover a target the user never + * reconnects to, whose tombstones would otherwise never be retired. */ +export const CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS = 30 * 24 * 60 * 60 * 1000 +export const MAX_CLOSED_TERMINAL_TAB_TOMBSTONES = 500 + +export function pruneClosedTerminalTabTombstones( + map: ClosedTerminalTabTombstonesByTabId | undefined, + now: number +): ClosedTerminalTabTombstonesByTabId { + const entries = Object.entries(map ?? {}).filter( + ([, tombstone]) => now - tombstone.closedAt <= CLOSED_TERMINAL_TAB_TOMBSTONE_TTL_MS + ) + entries.sort(([, a], [, b]) => b.closedAt - a.closedAt) + return Object.fromEntries(entries.slice(0, MAX_CLOSED_TERMINAL_TAB_TOMBSTONES)) +} + +export function recordClosedTerminalTabTombstone( + map: ClosedTerminalTabTombstonesByTabId | undefined, + tabId: string, + worktreeId: string, + now: number +): ClosedTerminalTabTombstonesByTabId { + return pruneClosedTerminalTabTombstones({ ...map, [tabId]: { closedAt: now, worktreeId } }, now) +} + +function maxAckRevision(a: number | undefined, b: number | undefined): number | undefined { + if (a === undefined) { + return b + } + return b === undefined ? a : Math.max(a, b) +} + +export type ClosedTerminalTabTombstoneAck = { + tombstones: ClosedTerminalTabTombstonesByTabId | undefined + /** Worktrees this snapshot actually carries a tab row for. A worktree the snapshot says nothing + * about — including one whose path never resolved to a local id — is not evidence of anything, + * so its tombstones are left untouched. */ + acknowledgedWorktreeIds: ReadonlySet + /** Every tab id the snapshot lists, across all worktrees: an id the host still carries anywhere + * has not been acknowledged, whichever worktree it now sits under. */ + hostKnownTabIds: ReadonlySet + hostRevision: number | undefined + now: number +} + +/** Retires tombstones the host has demonstrably seen, and stamps the rest with the revision that + * proved it had not yet. + * + * Retirement takes a snapshot that both covers the tombstone's worktree and is strictly newer than + * the last one that did — one snapshot alone can be the pull that was already in flight when the + * user closed. Absence never deletes here: a snapshot with no revision, or one carrying no row for + * the worktree, retires nothing. */ +export function reconcileClosedTerminalTabTombstones({ + tombstones, + acknowledgedWorktreeIds, + hostKnownTabIds, + hostRevision, + now +}: ClosedTerminalTabTombstoneAck): ClosedTerminalTabTombstonesByTabId { + const pruned = pruneClosedTerminalTabTombstones(tombstones, now) + if (hostRevision === undefined) { + return pruned + } + const kept: ClosedTerminalTabTombstonesByTabId = {} + for (const [tabId, tombstone] of Object.entries(pruned)) { + if (!acknowledgedWorktreeIds.has(tombstone.worktreeId)) { + kept[tabId] = tombstone + continue + } + const observed = tombstone.ackRevision + if (!hostKnownTabIds.has(tabId) && observed !== undefined && hostRevision > observed) { + continue + } + kept[tabId] = { ...tombstone, ackRevision: maxAckRevision(observed, hostRevision) } + } + return kept +} diff --git a/src/shared/terminal-surface-tombstone-schema.ts b/src/shared/terminal-surface-tombstone-schema.ts new file mode 100644 index 00000000000..6cd3cfe1bd3 --- /dev/null +++ b/src/shared/terminal-surface-tombstone-schema.ts @@ -0,0 +1,14 @@ +import { z } from 'zod' + +import { terminalTabIdSchema } from './terminal-tab-id-schema' + +/** The value schema for `WorkspaceSessionState.terminalSurfaceTombstonesByPaneKey`. Extracted from + * workspace-session-schema.ts, which is at its max-lines limit. */ +export const terminalSurfaceTombstoneSchema = z.object({ + worktreeId: z.string(), + parentTabId: terminalTabIdSchema, + leafId: z.string(), + ptyId: z.string(), + incarnationId: z.string().min(1).max(128), + retiredAt: z.number().finite().nonnegative() +}) diff --git a/src/shared/terminal-tab-id-schema.ts b/src/shared/terminal-tab-id-schema.ts new file mode 100644 index 00000000000..6dacbe7d8df --- /dev/null +++ b/src/shared/terminal-tab-id-schema.ts @@ -0,0 +1,10 @@ +import { z } from 'zod' + +import { isValidTerminalTabId } from './terminal-tab-id' + +/** Shared so the session schema and the tombstone value schemas validate tab ids identically. + * Extracted from workspace-session-schema.ts, which is at its max-lines limit. */ +export const terminalTabIdSchema = z + .string() + .min(1) + .refine(isValidTerminalTabId, 'terminal tab id must not contain ":"') diff --git a/src/shared/workspace-session-schema-field-coverage.test.ts b/src/shared/workspace-session-schema-field-coverage.test.ts new file mode 100644 index 00000000000..4ace00825e4 --- /dev/null +++ b/src/shared/workspace-session-schema-field-coverage.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from 'vitest' +import type { z } from 'zod' + +import { parseWorkspaceSession, workspaceSessionStateSchema } from './workspace-session-schema' +import type { WorkspaceSessionState } from './workspace-session-state-types' + +/** + * `workspaceSessionStateSchema` is the load boundary for BOTH partitions — + * `normalize-loaded-state-collections.ts` for `local`, `workspace-session-partitions.ts` for + * `ssh:`. Zod strips unknown keys and the write side does not validate, so a field declared + * on WorkspaceSessionState but missing here reaches disk and is silently discarded on the next + * launch. + * + * That failure is invisible to any test living inside one app process, which is how the closed-tab + * tombstone map shipped unpersisted: its unit suites, its e2e oracle and an A/B control were all + * green, and the fix still did not survive a quit-and-relaunch. + * + * Two sibling tables (`profile-project-session-field-disposition.ts`, + * `workspace-session-host-field-ownership.ts`) already pin themselves with + * `satisfies Record`. This schema had no such guard and is the one + * that fell behind, so the ratchet below is the same shape. + */ +const PERSISTED_WORKSPACE_SESSION_FIELDS = { + activeRepoId: true, + activeWorkspaceKey: true, + activeWorkspaceExecutionHostId: true, + activeWorktreeId: true, + activeTabId: true, + tabsByWorktree: true, + terminalLayoutsByTabId: true, + activeWorktreeIdsOnShutdown: true, + openFilesByWorktree: true, + activeFileIdByWorktree: true, + markdownFrontmatterVisible: true, + browserTabsByWorktree: true, + browserPagesByWorkspace: true, + activeBrowserTabIdByWorktree: true, + clientHostedBrowserPagesByWorktree: true, + clientHostedBrowserCloseIntentsByEnvironment: true, + activeTabTypeByWorktree: true, + browserUrlHistory: true, + activeTabIdByWorktree: true, + unifiedTabs: true, + tabGroups: true, + tabGroupLayouts: true, + activeGroupIdByWorktree: true, + activeConnectionIdsAtShutdown: true, + remoteSessionIdsByTabId: true, + lastVisitedAtByWorktreeId: true, + defaultTerminalTabsAppliedByWorktreeId: true, + sleepingAgentSessionsByPaneKey: true, + terminalPtyIncarnationsByPaneKey: true, + terminalTopologyRevisionByRepoId: true, + terminalSurfaceTombstonesByPaneKey: true, + closedTerminalTabTombstonesByTabId: true +} satisfies Record + +const MINIMAL_SESSION = { + activeRepoId: null, + activeWorktreeId: null, + activeTabId: null, + tabsByWorktree: {}, + terminalLayoutsByTabId: {} +} + +describe('workspaceSessionStateSchema field coverage', () => { + it('parses every field declared on WorkspaceSessionState', () => { + // The runtime half. `satisfies` above already makes a forgotten field a compile error; this + // reports it by name, and catches the reverse case where the list is updated but the schema + // is not. + // Why the cast: the export is annotated `z.ZodType`, which hides + // `.shape`. The value is a z.object; this reads its keys without widening the public type. + const schemaKeys = new Set( + Object.keys( + (workspaceSessionStateSchema as unknown as z.ZodObject>).shape + ) + ) + const missing = Object.keys(PERSISTED_WORKSPACE_SESSION_FIELDS).filter( + (field) => !schemaKeys.has(field) + ) + + expect(missing, `declared on WorkspaceSessionState but absent from the load schema`).toEqual([]) + }) + + it('round-trips the closed-tab tombstone map through parseWorkspaceSession', () => { + // The regression this file exists for. Recorded on close, written to disk, stripped on the next + // launch — so a close the transport never delivered resurrected after a quit-and-relaunch, + // which is precisely the reported shape ("every day I open orca and it opens more tabs"). + const parsed = parseWorkspaceSession({ + ...MINIMAL_SESSION, + closedTerminalTabTombstonesByTabId: { + 'tab-1': { closedAt: 1_700_000_000_000, worktreeId: 'repo:wt-1', ackRevision: 4 } + } + }) + + expect(parsed.ok).toBe(true) + expect(parsed.ok && parsed.value.closedTerminalTabTombstonesByTabId).toEqual({ + 'tab-1': { closedAt: 1_700_000_000_000, worktreeId: 'repo:wt-1', ackRevision: 4 } + }) + }) + + it('salvages a malformed tombstone entry rather than dropping the whole map', () => { + const parsed = parseWorkspaceSession({ + ...MINIMAL_SESSION, + closedTerminalTabTombstonesByTabId: { + 'tab-good': { closedAt: 1, worktreeId: 'repo:wt-1' }, + 'tab-bad': { closedAt: 'nope', worktreeId: 'repo:wt-1' } + } + }) + + expect(parsed.ok).toBe(true) + expect( + Object.keys((parsed.ok && parsed.value.closedTerminalTabTombstonesByTabId) || {}) + ).toEqual(['tab-good']) + }) +}) diff --git a/src/shared/workspace-session-schema.ts b/src/shared/workspace-session-schema.ts index f67a8ae0270..2cc23106f9e 100644 --- a/src/shared/workspace-session-schema.ts +++ b/src/shared/workspace-session-schema.ts @@ -12,12 +12,14 @@ * Only a payload that is not a session at all falls back to defaults. */ import { z } from 'zod' +import { closedTerminalTabTombstoneSchema } from './closed-terminal-tab-tombstones' import type { WorkspaceKey } from './folder-workspace-types' import type { TabGroupLayoutNode } from './tab-types' import type { TerminalPaneLayoutNode } from './terminal-tab-types' import type { TuiAgent } from './tui-agent' import type { WorkspaceSessionState } from './workspace-session-state-types' -import { isValidTerminalTabId } from './terminal-tab-id' +import { terminalTabIdSchema } from './terminal-tab-id-schema' +import { terminalSurfaceTombstoneSchema } from './terminal-surface-tombstone-schema' import { parseExecutionHostId, type ExecutionHostId } from './execution-host' import { isTuiAgent } from './tui-agent-config' import { isWorkspaceKey } from './workspace-scope' @@ -35,10 +37,6 @@ import { salvagedField, salvagedOptional, salvagingArray, salvagingRecord } from // ─── Terminal pane layout (recursive) ─────────────────────────────── const terminalPaneSplitDirectionSchema = z.enum(['vertical', 'horizontal']) -const terminalTabIdSchema = z - .string() - .min(1) - .refine(isValidTerminalTabId, 'terminal tab id must not contain ":"') const workspaceKeySchema = z.custom( (value) => typeof value === 'string' && isWorkspaceKey(value) ) @@ -190,15 +188,6 @@ const tabGroupLayoutNodeSchema: z.ZodType = z.lazy(() => // ─── Workspace session ────────────────────────────────────────────── -const terminalSurfaceTombstoneSchema = z.object({ - worktreeId: z.string(), - parentTabId: terminalTabIdSchema, - leafId: z.string(), - ptyId: z.string(), - incarnationId: z.string().min(1).max(128), - retiredAt: z.number().finite().nonnegative() -}) - const worktreeIdSchema = z.string() export const workspaceSessionStateSchema: z.ZodType = z.object({ @@ -314,6 +303,10 @@ export const workspaceSessionStateSchema: z.ZodType = z.o terminalSurfaceTombstonesByPaneKey: salvagedOptional( 'terminalSurfaceTombstonesByPaneKey', salvagingRecord(z.string(), terminalSurfaceTombstoneSchema) + ), + closedTerminalTabTombstonesByTabId: salvagedOptional( + 'closedTerminalTabTombstonesByTabId', + salvagingRecord(terminalTabIdSchema, closedTerminalTabTombstoneSchema) ) }) diff --git a/src/shared/workspace-session-state-types.ts b/src/shared/workspace-session-state-types.ts index 5e1191f4b08..271941b21c5 100644 --- a/src/shared/workspace-session-state-types.ts +++ b/src/shared/workspace-session-state-types.ts @@ -6,6 +6,7 @@ import type { TerminalLayoutSnapshot, TerminalTab } from './terminal-tab-types' import type { BrowserHistoryEntry, BrowserPage, BrowserWorkspace } from './browser-workspace-types' import type { ClientHostedBrowserCloseIntent } from './client-hosted-browser-close-intent' import type { PersistedClientHostedBrowserPage } from './client-hosted-browser-page-record' +import type { ClosedTerminalTabTombstonesByTabId } from './closed-terminal-tab-tombstones' /** Minimal subset of OpenFile persisted across restarts. * Only edit-mode files are saved — diffs, conflict reviews, and other @@ -124,6 +125,9 @@ export type WorkspaceSessionState = { retiredAt: number } > + /** Terminal tabs this client watched the user close, kept until the host's own snapshot stops + * listing them. See shared/closed-terminal-tab-tombstones.ts for why absence alone cannot say it. */ + closedTerminalTabTombstonesByTabId?: ClosedTerminalTabTombstonesByTabId } export type WorkspaceSessionPatch = Partial diff --git a/tests/e2e/ssh-lost-kill-tab-resurrection.spec.ts b/tests/e2e/ssh-lost-kill-tab-resurrection.spec.ts new file mode 100644 index 00000000000..0d5b4cf4a62 --- /dev/null +++ b/tests/e2e/ssh-lost-kill-tab-resurrection.spec.ts @@ -0,0 +1,198 @@ +import type { Page, TestInfo } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { waitForActiveWorktree, waitForSessionReady } from './helpers/store' +import { waitForActivePanePtyId, waitForActiveTerminalManager } from './helpers/terminal' +import { createRemoteTerminalTab } from './helpers/docker-ssh-relay-terminal-tabs' +import { + cleanupDockerSshRelayTarget, + startDockerSshRelayTarget, + type DockerSshRelayTarget +} from './helpers/docker-ssh-relay-target' +import { + readDockerSshRelayProcessSnapshot, + terminateDockerSshRelay +} from './helpers/docker-ssh-relay-processes' +import { + connectDockerSshRelayTarget, + disconnectDockerSshRelayTarget, + reconnectDisconnectedDockerSshRelayTarget +} from './helpers/docker-ssh-relay-connection' + +const RUN_DOCKER_SSH = process.env.ORCA_E2E_SSH_DOCKER === '1' +const DROP_CYCLES = 3 + +test.use({ seedTestRepo: false }) + +async function readWorktreeTabIds(page: Page, worktreeId: string): Promise { + return page.evaluate( + (id) => (window.__store?.getState().tabsByWorktree[id] ?? []).map((tab) => tab.id), + worktreeId + ) +} + +/** Poll until the tab set stops changing, so a resurrection that lands late still counts. */ +async function waitForSettledTabIds(page: Page, worktreeId: string): Promise { + let latest: string[] = [] + let previousKey = '' + let agreements = 0 + await expect + .poll( + async () => { + latest = await readWorktreeTabIds(page, worktreeId) + const key = latest.join() + agreements = key === previousKey ? agreements + 1 : 0 + previousKey = key + return agreements + }, + { timeout: 60_000, intervals: [1_000], message: 'the tab set never stopped changing' } + ) + .toBeGreaterThanOrEqual(3) + return latest +} + +/** + * Kill the relay daemon so the next RPC rejects with a transport-class error. + * + * Why this and not a disconnect: `pty.kill` has to FAIL, not succeed against a dead session. + * A transport rejection ("Multiplexer disposed" / CONNECTION_LOST) does not match + * isPtyAlreadyGoneError, so the lease is never marked terminated and nothing retries it — + * that unterminated lease is what the next reattach mistakes for a live PTY. + */ +function dropRelayTransport(target: DockerSshRelayTarget): void { + const snapshot = readDockerSshRelayProcessSnapshot(target) + if (!snapshot) { + throw new Error('No Docker SSH relay process group to terminate') + } + terminateDockerSshRelay(target, snapshot) +} + +async function dumpAuthority(page: Page, worktreeId: string, label: string): Promise { + const d = await page.evaluate(async (id) => { + const persisted = await window.api.session.get() + const state = window.__store?.getState() + const repoId = Object.entries(state?.worktreesByRepo ?? {}).find(([, ws]) => + ws.some((w) => w.id === id) + )?.[0] + return { + repoId: repoId?.slice(0, 8) ?? null, + topologyRev: persisted.terminalTopologyRevisionByRepoId ?? null, + tombstones: Object.keys(persisted.terminalSurfaceTombstonesByPaneKey ?? {}).length, + storeTabs: (state?.tabsByWorktree[id] ?? []).length + } + }, worktreeId) + console.log(`[auth ${label}] ${JSON.stringify(d)}`) +} + +async function closeTerminalTab(page: Page, tabId: string): Promise { + await page.evaluate((id) => { + const state = window.__store?.getState() + if (!state) { + throw new Error('Store unavailable') + } + state.closeTab(id) + }, tabId) +} + +/** + * Run N cycles of: make a tab, disrupt the transport, close the tab, reconnect. + * + * `disrupt` is the only variable — the two callers differ solely in HOW the transport goes away, + * so a difference in outcome is attributable to that and nothing else. + */ +async function runResurrectionCycles( + page: Page, + testInfo: TestInfo, + disrupt: (target: DockerSshRelayTarget, targetId: string) => Promise | void +): Promise { + let target: DockerSshRelayTarget | null = null + try { + target = startDockerSshRelayTarget(testInfo) + await waitForSessionReady(page) + const remote = await connectDockerSshRelayTarget(page, target) + await expect + .poll(() => waitForActiveWorktree(page), { timeout: 30_000 }) + .toBe(remote.worktreeId) + await waitForActiveTerminalManager(page, 60_000) + await waitForActivePanePtyId(page, 60_000) + const baseline = await waitForSettledTabIds(page, remote.worktreeId) + + const perCycle: { closedTabId: string; afterIds: string[] }[] = [] + for (let cycle = 0; cycle < DROP_CYCLES; cycle += 1) { + // Created while the transport is healthy — the disruption has to land between close and + // reattach, not before the tab has a PTY to leave a lease behind. + const beforeCreate = await waitForSettledTabIds(page, remote.worktreeId) + await createRemoteTerminalTab(page, remote.worktreeId) + const withExtra = await waitForSettledTabIds(page, remote.worktreeId) + // Diffed against the PREVIOUS cycle's tabs, not the baseline: once a cycle resurrects a tab, + // a baseline diff picks that survivor instead of the tab this cycle just made, and every + // later cycle would close the same stale tab and measure nothing. + const closedTabId = withExtra.find((tabId) => !beforeCreate.includes(tabId)) + if (!closedTabId) { + throw new Error('The extra SSH tab was never added') + } + + await dumpAuthority(page, remote.worktreeId, `cycle${cycle + 1}-before-close`) + await disrupt(target, remote.targetId) + await closeTerminalTab(page, closedTabId) + await reconnectDisconnectedDockerSshRelayTarget(page, remote.targetId) + await waitForActiveTerminalManager(page, 60_000) + perCycle.push({ + closedTabId, + afterIds: await waitForSettledTabIds(page, remote.worktreeId) + }) + await dumpAuthority(page, remote.worktreeId, `cycle${cycle + 1}-after-reconnect`) + } + + const growth = perCycle + .map( + (entry, index) => + `drop${index + 1}=${entry.afterIds.length}${entry.afterIds.includes(entry.closedTabId) ? ' (closed tab returned)' : ''}` + ) + .join(' ') + const summary = `baseline=${baseline.length} ${growth}` + for (const [index, entry] of perCycle.entries()) { + expect( + entry.afterIds, + `drop ${index + 1} resurrected the closed tab ${entry.closedTabId}: ${summary}` + ).not.toContain(entry.closedTabId) + } + expect( + perCycle.map((entry) => entry.afterIds.length), + `tabs accumulated across dropped-transport closes: ${summary}` + ).toEqual(perCycle.map(() => baseline.length)) + } finally { + cleanupDockerSshRelayTarget(target) + } +} + +test.describe('SSH lost kill tab resurrection', () => { + test.skip(!RUN_DOCKER_SSH, 'Set ORCA_E2E_SSH_DOCKER=1 to run Docker-backed SSH tests.') + test.skip(process.platform === 'win32', 'Docker SSH restore uses POSIX SSH tooling.') + + // STA-3374. A tab closed while the transport is down leaves an unterminated remote lease: the + // rejected `pty.kill` is a transport error, not an already-gone one, so nothing retires it. The + // relay also restarts its pty counter at pty-1, so a later tab is handed the same id and + // upsertSshRemotePtyLease — keyed on (targetId, ptyId) alone — collides with that stale lease + // instead of minting a fresh one. The next reattach then re-mints the tab through + // pty-binding-persistence.ts:145-160, and the resurrected tab never retires. + test('does not resurrect tabs whose kill was lost to a killed relay daemon', async ({ + orcaPage + }, testInfo) => { + test.setTimeout(600_000) + await runResurrectionCycles(orcaPage, testInfo, (target) => { + dropRelayTransport(target) + }) + }) + + // The same close, reached the way a user reaches it: disconnect the host, close the tab, come + // back. No daemon is killed. If this resurrects too, the bug needs no process death at all — a + // laptop lid and a dropped link are enough. + test('does not resurrect tabs closed while the host is disconnected', async ({ + orcaPage + }, testInfo) => { + test.setTimeout(600_000) + await runResurrectionCycles(orcaPage, testInfo, async (_target, targetId) => { + await disconnectDockerSshRelayTarget(orcaPage, targetId) + }) + }) +}) From 25fa6a9920e52d146fe9825eafa7533c034f98de Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 27 Aug 2026 19:49:00 -0700 Subject: [PATCH 2/3] fix(ssh): a relay that reports a PTY absent must authorise a fresh spawn (#16763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(ssh): a relay that reports a PTY absent must authorise a fresh spawn reattachSshPtySession flattened the relay's `PTY "" not found` into a bare `SSH_SESSION_EXPIRED`, discarding the evidence. isPtyAlreadyGoneError classifies on that message via /PTY ".+" not found/, so it could never match — and attachStablePaneOwner's existing already-gone fallback (retire the pane binding, onPtyExit, spawn fresh) is gated on exactly that predicate. The recovery machinery was already correct; only the classification was broken, which is why `pty:spawn` rejected outright after a relay restart renumbered its PTYs from pty-1. SshPtyAbsentFromRelayError carries the verdict the message cannot: a reachable relay answered for this exact id and reported it absent. Both thrown messages stay byte-identical to the previous ones, so all six message-based consumers are untouched; only callers that can act on the stronger verdict test the class. Deliberately not raised for a transport failure, timeout, disposed multiplexer, identity mismatch, or restoreRequired — none of those observe the process, and treating them as absence would orphan live remote work. * test(ssh): cover the absence fallback against real persistence The first cut passed no store/worktreeId, so attachStablePaneOwner's guard short-circuited and retireTerminalSurfaceFromPersistence — which deletes the parent tab and its layout when the retired leaf is the last one — never ran. retirePersistedStablePaneOwner and attachStablePaneOwner had no coverage at all. --- .../stable-pane-relay-absence-respawn.test.ts | 165 ++++++++++++++++++ src/main/ipc/pty/provider/liveness.ts | 13 +- src/main/providers/ssh-pty-errors.ts | 22 +++ .../ssh-pty-relay-absence-verdict.test.ts | 78 +++++++++ .../providers/ssh-pty-session-reattach.ts | 15 +- 5 files changed, 287 insertions(+), 6 deletions(-) create mode 100644 src/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.ts create mode 100644 src/main/providers/ssh-pty-relay-absence-verdict.test.ts diff --git a/src/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.ts b/src/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.ts new file mode 100644 index 00000000000..41831d68297 --- /dev/null +++ b/src/main/ipc/pty/pane/stable-pane-relay-absence-respawn.test.ts @@ -0,0 +1,165 @@ +import { describe, expect, it, vi } from 'vitest' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' +import { + SSH_SESSION_EXPIRED_ERROR, + SshPtyAbsentFromRelayError +} from '../../../providers/ssh-pty-errors' +import type { Store } from '../../../persistence' +import type { IPtyProvider } from '../../../providers/types' +import { spawnForStablePane, type StablePaneOwner } from './stable-owner' + +const LEAF = '1b3f2c4d-5e6a-4b7c-8d9e-0f1a2b3c4d5e' +const SIBLING_LEAF = '2c4d3e5f-6a7b-4c8d-9e0f-1a2b3c4d5e6f' +const WORKTREE = 'worktree-1' +const OWNER: StablePaneOwner = { + tabId: 'tab-1', + leafId: LEAF, + ptyId: 'ssh:conn-1@@pty-1', + hasPersistedBinding: true +} + +function spawnAfterAttachRejection( + error: unknown, + persistence?: { store: Store; worktreeId: string } +): { + run: () => ReturnType + spawn: ReturnType +} { + const spawn = vi + .fn() + .mockRejectedValueOnce(error) + .mockResolvedValueOnce({ id: 'ssh:conn-1@@pty-2', isReattach: false }) + return { + spawn, + run: () => + spawnForStablePane({ + runtime: undefined, + provider: { spawn } as unknown as IPtyProvider, + spawnOptions: { cols: 80, rows: 24 }, + owner: OWNER, + connectionId: 'conn-1', + resolveOwner: () => null, + ...(persistence ? { store: persistence.store, worktreeId: persistence.worktreeId } : {}) + }) + } +} + +/** Real retirement against an in-memory session, so `retireTerminalSurfaceFromPersistence` runs. */ +function sessionStore(leaves: string[]): { store: Store; read: () => WorkspaceSessionState } { + let session = { + tabsByWorktree: { + [WORKTREE]: [{ id: 'tab-1', worktreeId: WORKTREE, ptyId: 'ssh:conn-1@@pty-1' }] + }, + terminalLayoutsByTabId: { + 'tab-1': { + root: leaves.reduce>( + (node, leafId) => + Object.keys(node).length === 0 + ? { type: 'leaf', leafId } + : { type: 'split', direction: 'row', first: node, second: { type: 'leaf', leafId } }, + {} + ), + activeLeafId: leaves[0], + ptyIdsByLeafId: Object.fromEntries( + leaves.map((leafId, index) => [leafId, `ssh:conn-1@@pty-${index + 1}`]) + ) + } + }, + terminalPtyIncarnationsByPaneKey: {} + } as unknown as WorkspaceSessionState + return { + read: () => session, + store: { + getWorkspaceSession: () => session, + setWorkspaceSession: (next: WorkspaceSessionState) => { + session = next + }, + flushOrThrow: () => {} + } as unknown as Store + } +} + +describe('stable pane adoption after the relay reports the PTY absent', () => { + it('spawns fresh once the relay has positively answered for that id', async () => { + const { run, spawn } = spawnAfterAttachRejection( + new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`) + ) + + const result = await run() + + expect(spawn).toHaveBeenCalledTimes(2) + expect(spawn.mock.calls[0]?.[0]).toMatchObject({ attachOnly: true }) + expect(spawn.mock.calls[1]?.[0]).not.toHaveProperty('sessionId') + expect(result.owner).toBeNull() + }) + + // A restarted relay renumbers from pty-1, so the message alone cannot distinguish absence from a + // lost link — only the class may authorise abandoning the binding. + it.each([ + ['an expired session with no relay evidence', `${SSH_SESSION_EXPIRED_ERROR}: pty-1`], + ['a lost link', 'SSH connection lost, reconnecting...'], + ['a request timeout', 'Request "pty.attach" timed out after 10000ms'] + ])('keeps the binding and refuses to respawn after %s', async (_label, message) => { + const { run, spawn } = spawnAfterAttachRejection(new Error(message)) + + await expect(run()).rejects.toThrow(message) + expect(spawn).toHaveBeenCalledTimes(1) + }) + + // Why a real store: without one the `args.worktreeId` guard short-circuits and the retirement — + // which can delete the parent tab and its layout — never runs. "Reconnect lost every tab" is the + // regression this subsystem was reverted for twice, so the composition needs its own coverage. + describe('with persistence actually reached', () => { + it('retires only the absent leaf and leaves the tab and its sibling bound', async () => { + const { store, read } = sessionStore([LEAF, SIBLING_LEAF]) + const { run, spawn } = spawnAfterAttachRejection( + new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`), + { store, worktreeId: WORKTREE } + ) + + const result = await run() + + expect(spawn).toHaveBeenCalledTimes(2) + expect(result.owner).toBeNull() + const session = read() + expect(session.tabsByWorktree[WORKTREE]?.map((tab) => tab.id)).toEqual(['tab-1']) + const layout = session.terminalLayoutsByTabId['tab-1'] + expect(layout).toBeDefined() + expect(layout?.ptyIdsByLeafId?.[SIBLING_LEAF]).toBe('ssh:conn-1@@pty-2') + expect(layout?.ptyIdsByLeafId?.[LEAF]).toBeUndefined() + }) + + // Pins current behaviour rather than blessing it: retiring the LAST leaf drops the tab from + // persistence, and the fallback returns owner=null so main does not re-persist a binding — tab + // survival then rests entirely on the renderer. If that ever regresses, this is the tripwire. + it('drops the tab when the absent leaf was the only one, leaving re-persistence to the renderer', async () => { + const { store, read } = sessionStore([LEAF]) + const { run, spawn } = spawnAfterAttachRejection( + new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`), + { store, worktreeId: WORKTREE } + ) + + const result = await run() + + expect(spawn).toHaveBeenCalledTimes(2) + expect(result.owner).toBeNull() + const session = read() + expect(session.tabsByWorktree[WORKTREE]).toEqual([]) + expect(session.terminalLayoutsByTabId['tab-1']).toBeUndefined() + }) + + it('leaves persistence untouched when the failure is not positive absence', async () => { + const { store, read } = sessionStore([LEAF, SIBLING_LEAF]) + const before = JSON.stringify(read()) + const { run, spawn } = spawnAfterAttachRejection( + new Error('SSH connection lost, reconnecting...'), + { store, worktreeId: WORKTREE } + ) + + await expect(run()).rejects.toThrow('SSH connection lost') + + expect(spawn).toHaveBeenCalledTimes(1) + expect(JSON.stringify(read())).toBe(before) + }) + }) +}) diff --git a/src/main/ipc/pty/provider/liveness.ts b/src/main/ipc/pty/provider/liveness.ts index a8c005cce06..a5ff137f914 100644 --- a/src/main/ipc/pty/provider/liveness.ts +++ b/src/main/ipc/pty/provider/liveness.ts @@ -3,7 +3,10 @@ import type { AgentSessionOwnerBinding } from '../../../../shared/agent-session- import { agentSessionOwnerBindingsEqual } from '../../../../shared/claimed-agent-pty-owner' import { addNodePtyRecoveryHint } from '../../../daemon/node-pty-error-hints' import type { Store } from '../../../persistence' -import { isSshPtyNotFoundError } from '../../../providers/ssh-pty-errors' +import { + isSshPtyAbsentFromRelayError, + isSshPtyNotFoundError +} from '../../../providers/ssh-pty-errors' import type { IPtyProvider } from '../../../providers/types' import { markClaudePtyExited } from '../../../claude-accounts/live-pty-gate' import { ptyIncarnationById, ptyOwnership } from './ownership-state' @@ -54,7 +57,13 @@ export function normalizeNodePtySpawnError(err: unknown): Error { export function isPtyAlreadyGoneError(err: unknown): boolean { const message = err instanceof Error ? err.message : String(err) - return isSshPtyNotFoundError(err) || /Session not found/i.test(message) + return ( + isSshPtyNotFoundError(err) || + // Why: the reattach path rewrites the relay's wording to SSH_SESSION_EXPIRED and only this + // class preserves that the relay itself answered "absent" rather than the link dropping. + isSshPtyAbsentFromRelayError(err) || + /Session not found/i.test(message) + ) } export function delay(ms: number): Promise { diff --git a/src/main/providers/ssh-pty-errors.ts b/src/main/providers/ssh-pty-errors.ts index a8c551f228b..fb1244c7b4b 100644 --- a/src/main/providers/ssh-pty-errors.ts +++ b/src/main/providers/ssh-pty-errors.ts @@ -10,3 +10,25 @@ export function isSshPtyIdentityMismatchError(error: unknown): boolean { const message = error instanceof Error ? error.message : String(error) return message.includes(SSH_PTY_IDENTITY_MISMATCH_ERROR) || /identity mismatch/i.test(message) } + +/** + * A reachable relay answered for this exact PTY id and reported it absent — positive evidence of + * absence from the execution host, so `exited` rather than `unverifiable` + * (docs/reference/ssh-execution-boundary.md). Deliberately NOT raised for a transport failure, a + * request timeout, a disposed multiplexer, an identity mismatch (the id names a live PTY belonging + * to another pane), or `restoreRequired` (the PTY is live, only its source stream is not) — none of + * those observe the process, and treating them as absence orphans live remote work. + * + * Carries the same `SSH_SESSION_EXPIRED` message so message-based consumers are unaffected; only + * callers that can act on the stronger verdict test the class. + */ +export class SshPtyAbsentFromRelayError extends Error { + constructor(message: string) { + super(message) + this.name = 'SshPtyAbsentFromRelayError' + } +} + +export function isSshPtyAbsentFromRelayError(error: unknown): boolean { + return error instanceof SshPtyAbsentFromRelayError +} diff --git a/src/main/providers/ssh-pty-relay-absence-verdict.test.ts b/src/main/providers/ssh-pty-relay-absence-verdict.test.ts new file mode 100644 index 00000000000..9e5b9a555ac --- /dev/null +++ b/src/main/providers/ssh-pty-relay-absence-verdict.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it, vi } from 'vitest' +import { + SSH_PTY_IDENTITY_MISMATCH_ERROR, + SSH_SESSION_EXPIRED_ERROR, + isSshPtyAbsentFromRelayError +} from './ssh-pty-errors' +import { SshPtyProvider } from './ssh-pty-provider' + +function providerRejectingAttachWith(error: unknown): SshPtyProvider { + const mux = { + request: vi.fn().mockRejectedValue(error), + notify: vi.fn(), + onNotification: vi.fn().mockReturnValue(vi.fn()) + } + return new SshPtyProvider('conn-1', mux as never) +} + +async function reattachRejection(error: unknown): Promise { + const provider = providerRejectingAttachWith(error) + return await provider.spawn({ cols: 80, rows: 24, sessionId: 'pty-1' }).then( + () => undefined, + (rejection: unknown) => rejection + ) +} + +describe('SSH PTY relay absence verdict', () => { + it('reports a relay-delivered not-found as positive absence', async () => { + const rejection = await reattachRejection(new Error('PTY "pty-1" not found')) + + expect(isSshPtyAbsentFromRelayError(rejection)).toBe(true) + expect((rejection as Error).message).toBe(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`) + }) + + it('does not report absence when the id names a live PTY owned by another pane', async () => { + const rejection = await reattachRejection( + new Error('PTY "pty-1" not found (identity mismatch)') + ) + + expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false) + // Byte-identical to the pre-change message: only the class is new, so the toast humanizer, + // both spawn-execute lease paths, and the renderer's substring checks are untouched. + expect((rejection as Error).message).toBe( + `${SSH_SESSION_EXPIRED_ERROR}: pty-1 ${SSH_PTY_IDENTITY_MISMATCH_ERROR}` + ) + }) + + // Loss of contact never observes the process: each of these must stay `unverifiable`. + it.each([ + ['a lost link', 'SSH connection lost, reconnecting...'], + ['a disposed multiplexer', 'Multiplexer disposed'], + ['a request timeout', 'Request "pty.attach" timed out after 10000ms'] + ])('does not report absence for %s', async (_label, message) => { + const rejection = await reattachRejection(new Error(message)) + + expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false) + expect((rejection as Error).message).toBe(message) + }) + + it('does not report absence when the PTY is live but its source needs restoring', async () => { + const mux = { + request: vi.fn().mockResolvedValue({ + incarnationId: 'incarnation-1', + sourceRecovery: { status: 'restoreRequired', reason: 'checkpointUnavailable' } + }), + notify: vi.fn(), + onNotification: vi.fn().mockReturnValue(vi.fn()) + } + const provider = new SshPtyProvider('conn-1', mux as never) + + const rejection = await provider.spawn({ cols: 80, rows: 24, sessionId: 'pty-1' }).then( + () => undefined, + (error: unknown) => error + ) + + expect(isSshPtyAbsentFromRelayError(rejection)).toBe(false) + expect((rejection as Error).message).toContain(SSH_SESSION_EXPIRED_ERROR) + }) +}) diff --git a/src/main/providers/ssh-pty-session-reattach.ts b/src/main/providers/ssh-pty-session-reattach.ts index 3a7b8b15c79..d6da283cab9 100644 --- a/src/main/providers/ssh-pty-session-reattach.ts +++ b/src/main/providers/ssh-pty-session-reattach.ts @@ -3,6 +3,7 @@ import { isPtyIncarnationId, type PtyIncarnationId } from '../../shared/pty-inca import { SSH_PTY_IDENTITY_MISMATCH_ERROR, SSH_SESSION_EXPIRED_ERROR, + SshPtyAbsentFromRelayError, isSshPtyIdentityMismatchError, isSshPtyNotFoundError } from './ssh-pty-errors' @@ -225,10 +226,16 @@ export async function reattachSshPtySession(args: { // Why: an expired relay lease must be surfaced distinctly so the renderer clears its binding. console.warn(`[ssh-pty] pty.attach FAILED for ${args.sessionId}:`, error) if (isSshPtyNotFoundError(error)) { - const mismatchMarker = isSshPtyIdentityMismatchError(error) - ? ` ${SSH_PTY_IDENTITY_MISMATCH_ERROR}` - : '' - throw new Error(`${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId}${mismatchMarker}`) + if (isSshPtyIdentityMismatchError(error)) { + // The id names a LIVE PTY owned by another pane, so this is not evidence of absence. + throw new Error( + `${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId} ${SSH_PTY_IDENTITY_MISMATCH_ERROR}` + ) + } + // Why the class: the relay answered for this exact id, so callers holding a pane binding may + // retire it and spawn fresh. Plain `SSH_SESSION_EXPIRED` cannot say that — a restarted relay + // renumbers from pty-1, so the message alone is indistinguishable from a lost link. + throw new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: ${relaySessionId}`) } throw error } From 6b4537fa0198638499cea5c94792b5a13ba56d9b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 27 Aug 2026 20:04:57 -0700 Subject: [PATCH 3/3] wip: ssh-hidden-output (#16941) --- ...-output-restore-unavailable-banner.test.ts | 404 ++++++++++++++++++ .../hidden-output-restore-drain.ts | 22 +- .../remote-execution-host-pty.ts | 15 + 3 files changed, 434 insertions(+), 7 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/direct-ssh-hidden-output-restore-unavailable-banner.test.ts create mode 100644 src/renderer/src/components/terminal-pane/pty-connection/remote-execution-host-pty.ts diff --git a/src/renderer/src/components/terminal-pane/direct-ssh-hidden-output-restore-unavailable-banner.test.ts b/src/renderer/src/components/terminal-pane/direct-ssh-hidden-output-restore-unavailable-banner.test.ts new file mode 100644 index 00000000000..432b6fb9692 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/direct-ssh-hidden-output-restore-unavailable-banner.test.ts @@ -0,0 +1,404 @@ +import type * as React from 'react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { createDeferred, flushAsyncTicks } from './pty-connection-test-async' +import { + createManager, + createMockTransport, + createPane, + type ConnectCallbacks, + type MockTransport +} from './pty-connection-test-pane-fixtures' +import { buildPaneConnectionDeps } from './pty-connection-test-deps' +import { createInitialStoreState } from './pty-connection-test-store-fixtures' +import type { StoreState } from './pty-connection-test-store-state' +import { + installTerminalTestGlobals, + restoreTerminalTestGlobals +} from './pty-connection-test-environment' + +// Direct-SSH sibling of remote-hidden-output-restore-unavailable-banner.repro.test.ts. +// +// Reported by a WSL2 user driving remote Coder workspaces over direct SSH: +// "[Orca skipped hidden terminal output because main recovery was unavailable.]" +// multiple times a day (v1.4.190). +// +// Topology modeled: a direct-SSH PTY carries an "ssh:@@" id, +// so isRemoteRuntimePtyId() is FALSE and canUseHiddenOutputSnapshot() routes +// recovery through window.api.pty.getMainBufferSnapshot. That handler serializes +// main's model of a stream whose bytes crossed the relay, and it answers null +// whenever the snapshot cannot be produced in time (ownership settle deadline, +// provider acquisition timeout, post-gap provider snapshot requirement). +// +// Per docs/reference/ssh-execution-boundary.md a null from a remote execution +// host is `unverifiable`, never proof the bytes are gone — the same rule the +// "remote:" path already honours via the retry/re-arm budget. +// +// Repro command: +// pnpm exec vitest run --config config/vitest.config.ts \ +// src/renderer/src/components/terminal-pane/direct-ssh-hidden-output-restore-unavailable-banner.test.ts + +const scheduleRuntimeGraphSync = vi.fn() +const shouldSeedCacheTimerOnInitialTitle = vi.fn(() => false) +const toastInfo = vi.fn() +const notifyCodexPaneBoundForStaleSweep = vi.fn() + +let mockStoreState: StoreState +let transportFactoryQueue: MockTransport[] = [] +let createdTransportOptions: Record[] = [] +let storeSubscribers: ((state: StoreState) => void)[] = [] + +vi.mock('@/runtime/sync-runtime-graph', () => ({ + scheduleRuntimeGraphSync +})) + +vi.mock('@/store', () => ({ + useAppStore: { + getState: () => mockStoreState, + subscribe: (listener: (state: StoreState) => void) => { + storeSubscribers.push(listener) + return () => { + storeSubscribers = storeSubscribers.filter((candidate) => candidate !== listener) + } + } + } +})) + +vi.mock('@/lib/agent-status', async (importOriginal) => { + const { buildAgentStatusModuleMock } = await import('./pty-connection-test-environment') + return buildAgentStatusModuleMock(await importOriginal>()) +}) + +vi.mock('./cache-timer-seeding', () => ({ + shouldSeedCacheTimerOnInitialTitle +})) + +vi.mock('sonner', () => ({ + toast: { + info: toastInfo + } +})) + +vi.mock('@/lib/codex-stale-pane-sweep', () => ({ + notifyCodexPaneBoundForStaleSweep +})) + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useCallback: unknown>(fn: T): T => fn + } +}) + +vi.mock('./pty-transport', () => ({ + createIpcPtyTransport: vi.fn((options: Record) => { + createdTransportOptions.push(options) + const nextTransport = transportFactoryQueue.shift() + if (!nextTransport) { + throw new Error('No mock transport queued') + } + return nextTransport + }) +})) + +vi.mock('./remote-runtime-pty-transport', () => ({ + createRemoteRuntimePtyTransport: vi.fn( + (_environmentId: string, options: Record) => { + createdTransportOptions.push(options) + const nextTransport = transportFactoryQueue.shift() + if (!nextTransport) { + throw new Error('No mock transport queued') + } + return nextTransport + } + ) +})) + +vi.mock('./pty-dispatcher', async (importOriginal) => { + const actual = await importOriginal>() + return { + ...actual, + getEagerPtyBufferHandle: vi.fn(() => undefined) + } +}) + +// ── Scenario identities ───────────────────────────────────────────────────── +// Real direct-SSH app PTY id shape (shared/ssh-pty-id.ts). +const SSH_PTY_ID = 'ssh:coder-ws@@pty-1' +// Overflows the renderer's hidden background queue (2MB lossy cap), dropping the +// backlog and latching main-model restore. +const HIDDEN_BYTES = 'x'.repeat(2 * 1024 * 1024 + 1) +const LIVE_AGENT_CHUNK = 'LIVE_AGENT_CHUNK\r\n' +const HOST_SNAPSHOT_MARKER = 'HOST_SNAPSHOT_HIDDEN_AGENT_CONTENT' +const HOST_SNAPSHOT = { + data: `${HOST_SNAPSHOT_MARKER}\r\n`, + cols: 120, + rows: 40, + seq: HIDDEN_BYTES.length + LIVE_AGENT_CHUNK.length, + source: 'headless' +} +const BANNER_FRAGMENT = 'main recovery was unavailable' + +type HostSnapshot = typeof HOST_SNAPSHOT + +type SshPaneDrive = { + transport: MockTransport + pane: ReturnType + deps: ReturnType + disposable: { dispose: () => void } + deliver: (data: string, seq: number) => void + writtenChunks: () => string[] +} + +function stubMainBufferSnapshot( + impl: () => Promise +): ReturnType { + const getMainBufferSnapshot = vi.fn(impl) + ;(window.api.pty as unknown as Record).getMainBufferSnapshot = + getMainBufferSnapshot + return getMainBufferSnapshot +} + +async function connectHiddenDirectSshPane(): Promise { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport(SSH_PTY_ID) + const capturedDataCallback: { + current: ((data: string, meta?: { seq?: number; rawLength?: number }) => void) | null + } = { current: null } + transport.connect.mockImplementation(async ({ callbacks }: { callbacks?: ConnectCallbacks }) => { + capturedDataCallback.current = callbacks?.onData ?? null + return SSH_PTY_ID + }) + transportFactoryQueue.push(transport) + const pane = createPane(1) + const manager = createManager(1) + const deps = buildPaneConnectionDeps(() => mockStoreState, { + isVisibleRef: { current: false } + }) + const disposable = connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks(6) + expect(capturedDataCallback.current).not.toBeNull() + return { + transport, + pane, + deps, + disposable, + deliver: (data, seq) => capturedDataCallback.current?.(data, { seq, rawLength: data.length }), + writtenChunks: () => pane.terminal.write.mock.calls.map(([data]) => String(data)) + } +} + +function driveHiddenBacklogThenReveal(drive: SshPaneDrive): void { + drive.deliver(HIDDEN_BYTES, HIDDEN_BYTES.length) + ;(drive.deps.isVisibleRef as { current: boolean }).current = true + drive.deliver(LIVE_AGENT_CHUNK, HIDDEN_BYTES.length + LIVE_AGENT_CHUNK.length) +} + +async function advanceThroughNullRetryBudget(): Promise { + for (let retry = 0; retry < 3; retry++) { + vi.advanceTimersByTime(50) + vi.advanceTimersByTime(0) + await flushAsyncTicks(10) + } +} + +async function allowSelfHealWindow(): Promise { + for (let step = 0; step < 10; step += 1) { + await vi.advanceTimersByTimeAsync(500) + await flushAsyncTicks(20) + } +} + +function observeFinalPaneState(drive: SshPaneDrive): { + rawHiddenBacklogWritten: boolean + liveAgentChunkWritten: boolean + unavailableBannerWritten: boolean + hiddenOutputRecoveredFromHostSnapshot: boolean +} { + const joined = drive.writtenChunks().join('') + return { + rawHiddenBacklogWritten: joined.includes('x'.repeat(1024)), + liveAgentChunkWritten: joined.includes('LIVE_AGENT_CHUNK'), + unavailableBannerWritten: joined.includes(BANNER_FRAGMENT), + hiddenOutputRecoveredFromHostSnapshot: joined.includes(HOST_SNAPSHOT_MARKER) + } +} + +describe('direct-SSH hidden-output restore abandonment', () => { + beforeEach(() => { + vi.resetModules() + vi.clearAllMocks() + transportFactoryQueue = [] + createdTransportOptions = [] + storeSubscribers = [] + mockStoreState = createInitialStoreState(() => mockStoreState) + installTerminalTestGlobals() + // The reporter's client is WSL2, but the banner path is platform-independent; + // pin darwin so the Windows synchronized-output scan stays out of the writes. + ;(window.api.platform as unknown as Record).get = vi.fn(() => ({ + platform: 'darwin', + osRelease: '25.0.0' + })) + }) + + afterEach(async () => { + await restoreTerminalTestGlobals() + }) + + it('[invariant] must not declare recovery unavailable when the SSH host snapshot is one retry tick away', async () => { + const getMainBufferSnapshot: ReturnType = stubMainBufferSnapshot(async () => + getMainBufferSnapshot.mock.calls.length <= 4 ? null : HOST_SNAPSHOT + ) + const drive = await connectHiddenDirectSshPane() + vi.useFakeTimers() + driveHiddenBacklogThenReveal(drive) + await flushAsyncTicks(20) + expect(getMainBufferSnapshot).toHaveBeenCalledTimes(1) + + await advanceThroughNullRetryBudget() + await allowSelfHealWindow() + + expect(observeFinalPaneState(drive)).toEqual({ + rawHiddenBacklogWritten: false, + liveAgentChunkWritten: true, + unavailableBannerWritten: false, + hiddenOutputRecoveredFromHostSnapshot: true + }) + drive.disposable.dispose() + }) + + it('[invariant] must not turn a merely-slow SSH host snapshot into a loss banner and a permanent scrollback gap', async () => { + const slowSnapshot = createDeferred() + const getMainBufferSnapshot = stubMainBufferSnapshot(async () => HOST_SNAPSHOT) + getMainBufferSnapshot.mockReturnValueOnce(slowSnapshot.promise) + const drive = await connectHiddenDirectSshPane() + vi.useFakeTimers() + driveHiddenBacklogThenReveal(drive) + await flushAsyncTicks(20) + + // The 750ms foreground deadline elapses while main is still serializing. + vi.advanceTimersByTime(750) + vi.advanceTimersByTime(0) + await flushAsyncTicks(10) + slowSnapshot.resolve(HOST_SNAPSHOT) + await flushAsyncTicks(20) + await allowSelfHealWindow() + + expect(observeFinalPaneState(drive)).toEqual({ + rawHiddenBacklogWritten: false, + liveAgentChunkWritten: true, + unavailableBannerWritten: false, + hiddenOutputRecoveredFromHostSnapshot: true + }) + drive.disposable.dispose() + }) + + it('[invariant] a permanently silent SSH host banners once after a bounded number of re-arms and then stops requesting', async () => { + const getMainBufferSnapshot = stubMainBufferSnapshot(async () => null) + const drive = await connectHiddenDirectSshPane() + vi.useFakeTimers() + driveHiddenBacklogThenReveal(drive) + await flushAsyncTicks(20) + await advanceThroughNullRetryBudget() + + expect(drive.writtenChunks().join('')).toContain('LIVE_AGENT_CHUNK') + + for (let step = 0; step < 6; step += 1) { + await allowSelfHealWindow() + } + const bannerCount = drive + .writtenChunks() + .filter((data) => data.includes(BANNER_FRAGMENT)).length + expect(bannerCount).toBe(1) + const settledRequests = getMainBufferSnapshot.mock.calls.length + expect(settledRequests).toBeGreaterThan(4) + expect(settledRequests).toBeLessThan(40) + + for (let step = 0; step < 4; step += 1) { + await allowSelfHealWindow() + } + expect(getMainBufferSnapshot.mock.calls.length).toBe(settledRequests) + expect(drive.writtenChunks().filter((data) => data.includes(BANNER_FRAGMENT)).length).toBe(1) + drive.disposable.dispose() + }) + + // The re-arm budget bounds one stall; live bytes are not evidence the host can + // answer again. Continuous output on the SAME ssh: id must not re-open recovery + // or refresh the budget, or the bounded re-arm becomes a permanent ~2s repaint loop. + it('[invariant] continuous live output on a silent SSH host cannot refresh the re-arm budget into a repaint loop', async () => { + const getMainBufferSnapshot = stubMainBufferSnapshot(async () => null) + const drive = await connectHiddenDirectSshPane() + vi.useFakeTimers() + driveHiddenBacklogThenReveal(drive) + await flushAsyncTicks(20) + await advanceThroughNullRetryBudget() + + let seq = HIDDEN_BYTES.length + LIVE_AGENT_CHUNK.length + for (let step = 0; step < 60; step += 1) { + const chunk = `LIVE_TICK_${step}\r\n` + seq += chunk.length + drive.deliver(chunk, seq) + await vi.advanceTimersByTimeAsync(300) + await flushAsyncTicks(20) + } + + const joined = drive.writtenChunks().join('') + expect(joined).toContain('LIVE_TICK_0') + expect(joined).toContain('LIVE_TICK_59') + expect(joined.split(BANNER_FRAGMENT).length - 1).toBe(1) + const settledRequests = getMainBufferSnapshot.mock.calls.length + expect(settledRequests).toBeLessThan(40) + + for (let step = 0; step < 30; step += 1) { + const chunk = `LIVE_TAIL_${step}\r\n` + seq += chunk.length + drive.deliver(chunk, seq) + await vi.advanceTimersByTimeAsync(300) + await flushAsyncTicks(20) + } + expect(getMainBufferSnapshot.mock.calls.length).toBe(settledRequests) + expect(drive.writtenChunks().join('').split(BANNER_FRAGMENT).length - 1).toBe(1) + drive.disposable.dispose() + }) + + // The re-arm budget bounds ONE stall. It only clears on a successful snapshot or + // a PTY change, so without an explicit reset at the banner a single long outage + // leaves a long-lived SSH pane permanently at zero tolerance. + it('[invariant] a spent re-arm budget must not leave the SSH pane at zero tolerance for its next hidden episode', async () => { + const revivedSnapshot = createDeferred() + let hostIsSilent = true + stubMainBufferSnapshot(async () => (hostIsSilent ? null : revivedSnapshot.promise)) + const drive = await connectHiddenDirectSshPane() + vi.useFakeTimers() + driveHiddenBacklogThenReveal(drive) + await flushAsyncTicks(20) + await advanceThroughNullRetryBudget() + for (let step = 0; step < 6; step += 1) { + await allowSelfHealWindow() + } + expect(drive.writtenChunks().filter((data) => data.includes(BANNER_FRAGMENT))).toHaveLength(1) + + // Second episode on the SAME ssh: id: the host is alive again but its snapshot + // lands just past the 750ms foreground deadline. + hostIsSilent = false + let seq = HIDDEN_BYTES.length + LIVE_AGENT_CHUNK.length + ;(drive.deps.isVisibleRef as { current: boolean }).current = false + seq += HIDDEN_BYTES.length + drive.deliver(HIDDEN_BYTES, seq) + ;(drive.deps.isVisibleRef as { current: boolean }).current = true + seq += LIVE_AGENT_CHUNK.length + drive.deliver(LIVE_AGENT_CHUNK, seq) + await flushAsyncTicks(20) + vi.advanceTimersByTime(750) + vi.advanceTimersByTime(0) + await flushAsyncTicks(10) + revivedSnapshot.resolve(HOST_SNAPSHOT) + await flushAsyncTicks(20) + await allowSelfHealWindow() + + const joined = drive.writtenChunks().join('') + expect(joined.split(BANNER_FRAGMENT).length - 1).toBe(1) + expect(joined).toContain(HOST_SNAPSHOT_MARKER) + drive.disposable.dispose() + }) +}) diff --git a/src/renderer/src/components/terminal-pane/pty-connection/hidden-output-restore-drain.ts b/src/renderer/src/components/terminal-pane/pty-connection/hidden-output-restore-drain.ts index 81e26731783..2ee5bf2550b 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/hidden-output-restore-drain.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/hidden-output-restore-drain.ts @@ -9,6 +9,7 @@ import { } from './hidden-output-restore-limits' import { shouldWritePtyOutputForeground } from './foreground-output-scan' import { isRemoteRuntimePtyId } from './paired-parked-terminal-restore' +import { isRemoteExecutionHostPtyId } from './remote-execution-host-pty' import type { ConnectPanePtySession } from './connect-pane-pty-session' @@ -132,17 +133,24 @@ export function bindHiddenOutputRestoreDrain(session: ConnectPanePtySession): vo }, HIDDEN_OUTPUT_RESTORE_FOREGROUND_TIMEOUT_MS) } - // Trades the loss banner for one bounded post-suppression repaint from the - // host's authoritative buffer. Ordered before the state reset so the repaint - // timer arms against the live ptyId (mirrors the flood-abandon call sites). + // Trades the loss banner for one bounded post-suppression repaint from whatever + // answers for this PTY — the paired runtime's own buffer, or main's model of the + // direct-SSH stream. Ordered before the state reset so the repaint timer arms + // against the live ptyId (mirrors the flood-abandon call sites). session.rearmRemoteHiddenOutputRestoreInsteadOfWarning = function ( ptyId: string, reason: string ): boolean { - if ( - !isRemoteRuntimePtyId(ptyId) || - session.hiddenOutputRestoreRemoteAbandonCycles >= HIDDEN_OUTPUT_RESTORE_REMOTE_REARM_MAX - ) { + if (!isRemoteExecutionHostPtyId(ptyId)) { + return false + } + if (session.hiddenOutputRestoreRemoteAbandonCycles >= HIDDEN_OUTPUT_RESTORE_REMOTE_REARM_MAX) { + // Why the reset: the budget bounds ONE stall, and returning false ends that + // stall with the loss banner. Without this, the counter only ever clears on a + // successful snapshot or a PTY change, so a single ~10s outage leaves a + // long-lived SSH pane at zero tolerance — every later hidden episode banners + // on the first null even though that null is still only `unverifiable`. + session.hiddenOutputRestoreRemoteAbandonCycles = 0 return false } session.hiddenOutputRestoreRemoteAbandonCycles += 1 diff --git a/src/renderer/src/components/terminal-pane/pty-connection/remote-execution-host-pty.ts b/src/renderer/src/components/terminal-pane/pty-connection/remote-execution-host-pty.ts new file mode 100644 index 00000000000..cab98833914 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-connection/remote-execution-host-pty.ts @@ -0,0 +1,15 @@ +import { parseAppSshPtyId } from '../../../../../shared/ssh-pty-id' + +import { isRemoteRuntimePtyId } from './paired-parked-terminal-restore' + +// Why: hidden-output recovery for both remote shapes is answered across a link — +// paired runtimes ("remote:", host-owned buffer) and direct SSH +// ("ssh:@@", main's relay-fed model, whose serialize can block on +// host RPCs). A null/late snapshot from either is `unverifiable`, never proof +// the bytes are gone (docs/reference/ssh-execution-boundary.md). +export function isRemoteExecutionHostPtyId(ptyId: string | null | undefined): boolean { + if (typeof ptyId !== 'string') { + return false + } + return isRemoteRuntimePtyId(ptyId) || parseAppSshPtyId(ptyId) !== null +}