mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 00:03:15 +00:00
The Docker-SSH e2e lane only ran when a PR's changed specs happened to include
`ssh-startup-exec-readiness.spec.ts` or `paired-startup-exec-readiness.spec.ts`.
Editing SSH source itself did not trigger it, and pruning either spec from a
route's list would have silently retired the whole lane. Meanwhile the sharded
lanes set no `ORCA_E2E_SSH_DOCKER`, so every Docker-gated spec skipped itself
while the shard still reported green -- the exact silent-skip shape
`docs/reference/ssh-reconnect-source-recovery.md` blames for four regressions
that reached users.
Separately, the modules that actually own direct-SSH workspace and tab restore
carry no "ssh" in their names, so the `ssh-terminal-source` route never reached
them. Measured on the real script before this change:
printf '%s\n' src/renderer/src/hooks/remote-workspace-session-merge.ts \
src/main/ipc/remote-workspace-snapshot-normalization.ts \
src/renderer/src/lib/worktree-initial-terminal-seeding.ts \
src/shared/remote-workspace-session-projection.ts \
| node config/scripts/pr-e2e-source-routing.mjs
=> []
Three changes, all pinned by the executable gate contract:
- `hasSshSourceChange` derives an `ssh_source_changed` signal from the SSH
routes themselves, plumbed pr.yml -> e2e.yml, so the lane triggers on source
rather than on a spec name surviving in a list. One list, so the two cannot
drift.
- A sibling `ssh-workspace-session-restore` route names the restore seams
(`remote-workspace-*`, `worktree-initial-terminal-seeding`,
`worktree-default-terminal-tabs`, `initial-terminal`) and routes them to the
two restore specs -- a sibling rather than more paths on `ssh-terminal-source`
so a tab-tombstone edit does not run the whole SSH terminal list.
- A new `test:e2e:ssh-docker` runner claims the remaining Docker-gated specs on
the one VM that sets the flag, and the contract now fails by name when any
Docker-gated spec is claimed by no runner. `ssh-docker-relay-perf` and
`ssh-codex-display-artifacts-repro` are recorded exemptions (wall-clock
budgets; needs a real remote codex binary) and the contract asserts each
exemption still corresponds to a real gated spec, so a stale one cannot
quietly excuse a gap. Lane timeout raised 35 -> 60 minutes for the added
serial specs.
The lane's first act was to surface four latent bugs in a spec that had been
silently skipping. `ssh-docker-bulk-open-freeze-repro.spec.ts` is four call sites
out of date against `tests/e2e/helpers/terminal.ts`: `startDockerSshRelayTarget()`
is called with no argument though the helper dereferences `testInfo.workerIndex`
(a 100% failure, not a flake), `execInTerminal` gained a `ptyId` parameter, and
`splitActiveTerminalPane` gained a direction. It was invisible because it ran
nowhere and `typecheck:e2e` is red on main with 240 pre-existing errors, so four
more could not be seen.
The `testInfo` bug is fixed here -- correct on its own, and it removes one real
error from `typecheck:e2e` (240 -> 239). The other three are not, because they
are not argument plumbing: repairing them requires choosing which ptyId to
capture and which split direction to use, and both change what the repro
measures.
The spec is therefore added to the exemption list rather than repaired, for two
independent reasons recorded in the runner: it is a perf oracle, not a
correctness one (`SOFT_FREEZE_LAG_MS=2500` / `HARD_FREEZE_LAG_MS=5000` measured
under a deliberate 5-pane flood on a 420s budget -- the same rule already applied
to `ssh-docker-relay-perf.spec.ts`), and it is known-rotted. Repair is tracked in
stablyai/orca#16764. Applying an existing written rule to a sibling that plainly
meets it is consistency; inventing a new exemption to dodge a red would not be.
Three hardening fixes to the contract itself:
- Runner text is comment-stripped before the claimed-by-a-lane scan. A substring
scan over raw text lets a spec merely *discussed* in a runner comment count as
claimed -- the silent skip this assertion exists to catch, re-entering through
the documentation. Not live today only because the existing comments write the
spec names without their `tests/e2e/` prefix.
- An exempt spec must not be invoked by any runner. `unreachableSpecs`
short-circuits the unclaimed check, so a spec could be documented as exempt
while a runner still ran it -- an exemption that reads as coverage removal but
changes nothing, leaving the lane red for a reason the file says it excluded.
This is not hypothetical: adding the bulk-open exemption without removing it
from the runner's spec list produced exactly that state, and this assertion is
what caught it.
- The Docker-gate detector is now `/ORCA_E2E_SSH_DOCKER\s*[!=]==\s*['"]1['"]/`
rather than one fixed string, so a double-quoted or `!==` spelling can no
longer escape the contract.
`ssh-restart-tab-accumulation.spec.ts` is a new three-cycle restart fence
asserting tab-id set identity, not just the active pane's reclaimed ptyId as
`ssh-cold-activation-restore.spec.ts:241` did. It passes today; it was validated
by a negative control that injected one tab after cycle 1 and correctly failed.
144 lines
7.7 KiB
TypeScript
144 lines
7.7 KiB
TypeScript
import { test, expect } from './helpers/orca-app'
|
|
import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store'
|
|
import { waitForActivePanePtyId, waitForActiveTerminalManager } from './helpers/terminal'
|
|
import {
|
|
cleanupDockerSshRelayTarget,
|
|
startDockerSshRelayTarget,
|
|
type DockerSshRelayTarget
|
|
} from './helpers/docker-ssh-relay-target'
|
|
import {
|
|
connectDockerSshRelayTarget,
|
|
reconnectDockerSshRelayTarget
|
|
} from './helpers/docker-ssh-relay-connection'
|
|
import { openTerminalTabInActiveGroup } from './helpers/terminal-tab-open'
|
|
|
|
const RUN_DOCKER_SSH = process.env.ORCA_E2E_SSH_DOCKER === '1'
|
|
|
|
/**
|
|
* An SSH reconnect destroys the terminal state behind a tab whose creation has not yet reached the
|
|
* host, while the process it was running keeps going.
|
|
*
|
|
* The symptom is worse than a disappearing tab, because the two models disagree: the TAB BAR still
|
|
* renders the tab, correctly titled, but the terminal slice holds only the older tab and no pane
|
|
* manager exists for the newer one. So the user is left clicking a selected tab that will never
|
|
* paint, with no error and no way to recover it, while `top` runs on untouched on the host.
|
|
*
|
|
* Mechanism:
|
|
* - `remote-workspace-session-merge.ts:86-89` builds `tabsByWorktree` as
|
|
* `{...omitTargetWorktrees(current), ...remote}`. A local tab for the target worktree that is
|
|
* absent from the host snapshot has no surviving branch — it is simply not in the result.
|
|
* - `remote-workspace-target-sync.ts` applies that host snapshot unconditionally once
|
|
* `revision > 0`, without pushing local state first.
|
|
* - The upload that would have put the tab in the host list is DROPPED rather than deferred: the
|
|
* debounced session writer is gated on `!isRemoteWorkspaceSnapshotApplyInProgress()`, and
|
|
* `REMOTE_WORKSPACE_SNAPSHOT_WRITE_SUPPRESS_MS` is 1_000 after a snapshot apply. A tab created
|
|
* inside that window never gets written.
|
|
*
|
|
* Correlation observed across runs, which is what pinned the mechanism: host snapshot revision 1
|
|
* (1 tab) always lost the pane; revision 2 (2 tabs) always kept it.
|
|
*
|
|
* PRE-EXISTING. None of remote-workspace-target-sync.ts, remote-workspace-session-merge.ts,
|
|
* use-app-session-persistence.ts or remote-workspace-snapshot-apply.ts was touched by the branch
|
|
* that added this spec.
|
|
*
|
|
* FIXED by making the merge treat the host as authoritative only for what it knows: a local tab the
|
|
* snapshot has never been told about is kept rather than erased.
|
|
*
|
|
* SCOPE — this spec is NOT the guard, and measuring it is the only reason that is knowable. Against
|
|
* the unfixed code it fails roughly one run in three or four, because the destruction needs the tab
|
|
* to be created inside the debounced upload's suppression window and nothing here can force that
|
|
* from the outside. Removing the waits between creating the tab and reconnecting tightened it and
|
|
* still did not make it deterministic.
|
|
*
|
|
* The real guards are deterministic and live elsewhere: remote-workspace-snapshot-local-tab-survival
|
|
* .test.ts drives this same scenario through the actual apply path, and
|
|
* remote-workspace-session-merge-local-survival.test.ts covers the merge decision table. Together
|
|
* they fail 8 times on the unfixed code. Keep this spec as end-to-end smoke, and do not read a green
|
|
* run here as evidence the bug is gone.
|
|
*/
|
|
test.describe('SSH reconnect tab destruction', () => {
|
|
test.skip(!RUN_DOCKER_SSH, 'Set ORCA_E2E_SSH_DOCKER=1 to run the dockerized SSH relay tests')
|
|
|
|
test('keeps a tab created right after a reconnect alive across the next one', async ({
|
|
orcaPage
|
|
}, testInfo) => {
|
|
test.slow()
|
|
let target: DockerSshRelayTarget | null = null
|
|
try {
|
|
target = startDockerSshRelayTarget(testInfo)
|
|
await waitForSessionReady(orcaPage)
|
|
await waitForActiveWorktree(orcaPage)
|
|
const remote = await connectDockerSshRelayTarget(orcaPage, target)
|
|
await ensureTerminalVisible(orcaPage, 45_000)
|
|
await waitForActiveTerminalManager(orcaPage, 60_000)
|
|
// Awaited, not captured: the pane must be bound before the first reconnect, but the id itself
|
|
// is not what this spec asserts on — tab survival is.
|
|
await waitForActivePanePtyId(orcaPage, 60_000)
|
|
|
|
await reconnectDockerSshRelayTarget(orcaPage, remote.targetId)
|
|
await waitForActiveTerminalManager(orcaPage, 60_000)
|
|
await waitForActivePanePtyId(orcaPage, 60_000)
|
|
|
|
// Immediately after the apply, i.e. inside the 1s suppression window, so the tab's creation
|
|
// is dropped from the session write rather than deferred. This is the ordinary thing a user
|
|
// does; the timing is not contrived.
|
|
await openTerminalTabInActiveGroup(orcaPage)
|
|
// Only that the tab exists in the store — no waiting for its manager or PTY. Every wait here
|
|
// is time the debounced upload can use to land, which is what made this spec miss the bug.
|
|
const tabIdsBefore = await orcaPage.evaluate(() => {
|
|
const state = window.__store?.getState()
|
|
const worktreeId = state?.activeWorktreeId
|
|
return worktreeId ? (state?.tabsByWorktree?.[worktreeId] ?? []).map((tab) => tab.id) : []
|
|
})
|
|
expect(tabIdsBefore.length).toBeGreaterThanOrEqual(2)
|
|
|
|
// Deliberately NOTHING between creating the tab and reconnecting. The destruction only fires
|
|
// while the tab's creation is still unuploaded, so idling here — as waiting for a TUI to draw
|
|
// did — lets the debounced write land and the bug evaporate. That is exactly why an earlier
|
|
// version of this spec passed with the bug still present, and why it was worthless as a guard.
|
|
await reconnectDockerSshRelayTarget(orcaPage, remote.targetId)
|
|
await waitForActiveTerminalManager(orcaPage, 60_000)
|
|
|
|
// Checked BEFORE any paint assertion: survival and repaint are different failures, and this
|
|
// order names which one broke instead of collapsing both into "no output".
|
|
const tabState = await orcaPage.evaluate(() => {
|
|
const state = window.__store?.getState()
|
|
const worktreeId = state?.activeWorktreeId
|
|
return {
|
|
tabIds: worktreeId
|
|
? (state?.tabsByWorktree?.[worktreeId] ?? []).map((tab) => tab.id)
|
|
: [],
|
|
// __paneManagers is a Map. Object.keys on a Map silently returns [], which reads as
|
|
// "nothing is mounted" regardless of the truth — that cost a full debugging cycle.
|
|
paneManagers: window.__paneManagers?.size ?? 0
|
|
}
|
|
})
|
|
// The exact set, not a lower bound: `>= 2` passes just as happily on a reconnect that ADDS a
|
|
// tab as on one that keeps it, so it could never fail on the accumulation half of this bug.
|
|
expect(
|
|
tabState.tabIds.slice().sort(),
|
|
'the reconnect changed the tab set: it destroyed a tab or spuriously added one'
|
|
).toEqual(tabIdsBefore.slice().sort())
|
|
expect(
|
|
tabState.paneManagers,
|
|
'the tab survived but its pane manager did not'
|
|
).toBeGreaterThanOrEqual(1)
|
|
|
|
// NOT asserted: that the surviving pane reaches its shell again.
|
|
//
|
|
// Measured at 3 runs in 4 — the tab survives every time, the reattach behind it does not. So
|
|
// preserving the tab is a real fix and an incomplete one: the store keeps the tab, the tab bar
|
|
// renders it, and the pane sometimes never rebinds, which is the "frozen tab" shape the
|
|
// original report described. Asserting it here would put a one-in-four flake into the CI lane
|
|
// that exists to catch this class, which is worse than saying plainly that it is unfixed.
|
|
//
|
|
// The reattach gap is tracked separately; do not add a liveness assertion here until it is
|
|
// deterministic, or the lane stops being trusted.
|
|
} finally {
|
|
if (target) {
|
|
cleanupDockerSshRelayTarget(target)
|
|
}
|
|
}
|
|
})
|
|
})
|