mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
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 <old id>` -- 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 <id>: 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:<target>`. 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<keyof WorkspaceSessionState, ...>`; 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`.
199 lines
8.1 KiB
TypeScript
199 lines
8.1 KiB
TypeScript
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<string[]> {
|
|
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<string[]> {
|
|
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<void> {
|
|
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<void> {
|
|
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> | void
|
|
): Promise<void> {
|
|
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)
|
|
})
|
|
})
|
|
})
|