mirror of
https://github.com/stablyai/orca.git
synced 2026-09-21 16:02:20 +00:00
Neil
2b391652b1
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`.
96 lines
4.6 KiB
JavaScript
96 lines
4.6 KiB
JavaScript
import { spawnSync } from 'node:child_process'
|
|
|
|
const rawExtraArgs = process.argv.slice(2)
|
|
const extraArgs = rawExtraArgs[0] === '--' ? rawExtraArgs.slice(1) : rawExtraArgs
|
|
const pnpm = process.platform === 'win32' ? 'pnpm.cmd' : 'pnpm'
|
|
const env = {
|
|
...process.env,
|
|
ORCA_E2E_SSH_DOCKER: '1',
|
|
ORCA_E2E_WEB_CLIENT: '1'
|
|
}
|
|
|
|
// Why: Node's CVE-2024-27980 hardening rejects .cmd spawns without shell on Windows.
|
|
const spawnOptions = {
|
|
stdio: 'inherit',
|
|
env,
|
|
shell: process.platform === 'win32'
|
|
}
|
|
|
|
const runtime = spawnSync(pnpm, ['run', 'ensure:electron-runtime'], spawnOptions)
|
|
|
|
if (runtime.status !== 0) {
|
|
process.exit(runtime.status ?? 1)
|
|
}
|
|
|
|
// Why one explicit list: these specs self-skip without ORCA_E2E_SSH_DOCKER and no sharded lane
|
|
// sets it, so a spec in no runner runs nowhere. The gate contract proves every flag-reading
|
|
// spec is claimed here, by the watcher-isolation or parking runner, or by a listed exclusion.
|
|
//
|
|
// Deliberately absent, and therefore still covered by no CI trigger:
|
|
// ssh-docker-relay-perf.spec.ts — wall-clock latency thresholds; flaky budgets here would
|
|
// cost the lane its credibility. NOTE: a runner script test:e2e:ssh-docker-perf exists in
|
|
// package.json but NO workflow invokes it, so this spec currently runs in no CI lane at
|
|
// all. Recorded as a real gap, not as coverage living somewhere else.
|
|
// ssh-codex-display-artifacts-repro.spec.ts — installs a real remote codex binary that CI
|
|
// runners do not have (observed as `spawn codex ENOENT`). Runs in no CI lane at all.
|
|
// ssh-docker-bulk-open-freeze-repro.spec.ts — two reasons, both disqualifying:
|
|
// (a) it is a perf oracle, not a correctness one: SOFT_FREEZE_LAG_MS=2500 /
|
|
// HARD_FREEZE_LAG_MS=5000 measured by a renderer lag probe under a deliberate
|
|
// 5-pane output flood on a 420s budget. Same rule as ssh-docker-relay-perf above.
|
|
// (b) it is ROTTED: four call sites are out of date against terminal.ts's current
|
|
// helpers — execInTerminal gained a ptyId parameter and splitActiveTerminalPane
|
|
// gained a direction, so it cannot compile, let alone pass. Repairing it needs two
|
|
// semantic decisions (which ptyId to capture, which split direction) that change
|
|
// what the repro measures. Tracked in stablyai/orca#16764.
|
|
//
|
|
// Why both projects: ssh-port-forward-lifecycle is @headful, which the headless project
|
|
// grep-inverts away.
|
|
//
|
|
// Known gaps in SSH e2e coverage, recorded here because nothing else names them:
|
|
// - The job that runs this is still called `ssh-docker-watcher-isolation`, though watcher
|
|
// isolation is now one spec of many. Renaming it changes the GitHub check name and can
|
|
// break required-check config, so the name understates the job on purpose.
|
|
// - E2E does not gate merges: `verify.needs` in pr.yml omits `e2e` while the suite is red on
|
|
// main. Nothing in this lane blocks a PR yet. pr.yml's Require-successful-checks comment
|
|
// has the exact wiring to flip it, and the gate contract asserts the current state.
|
|
// - Five specs and one unit test are gated on env vars no workflow sets, so they run nowhere
|
|
// and are not Docker-gated, which puts them outside this file's contract:
|
|
// local-ssh-browser-routing (ORCA_E2E_LOCAL_SSH_BROWSER)
|
|
// ssh-client-hosted-browser-drop-reconnect (ORCA_E2E_SSH_CLIENT_HOSTED_BROWSER)
|
|
// nested-runtime-ssh-lifecycle, nested-runtime-ssh-routing (ORCA_E2E_NESTED_RUNTIME_SSH)
|
|
// ssh-localhost (ORCA_E2E_SSH_LOCALHOST)
|
|
// ssh-browser-network-execution-route.docker.unit.test.ts (ORCA_RUN_DOCKER_SSH_BROWSER_E2E)
|
|
// Runner scripts for the first four sit unused in package.json; no workflow calls them.
|
|
const result = spawnSync(
|
|
pnpm,
|
|
[
|
|
'exec',
|
|
'playwright',
|
|
'test',
|
|
'tests/e2e/pty-input-write-queue-ssh.spec.ts',
|
|
'tests/e2e/ssh-ai-vault-session-history.spec.ts',
|
|
'tests/e2e/ssh-cold-activation-restore.spec.ts',
|
|
'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',
|
|
'tests/e2e/ssh-restart-tab-accumulation.spec.ts',
|
|
'tests/e2e/ssh-skill-installation.spec.ts',
|
|
'tests/e2e/ssh-terminal-window-wake-stale-grid-repro.spec.ts',
|
|
'--config',
|
|
'tests/playwright.config.ts',
|
|
'--project',
|
|
'electron-headless',
|
|
'--project',
|
|
'electron-headful',
|
|
'--workers=1',
|
|
...extraArgs
|
|
],
|
|
spawnOptions
|
|
)
|
|
|
|
process.exit(result.status ?? 1)
|