From 4f4cbb6b00c2eca2cfa7d6bcd84a1060f49cfeb7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 6 Sep 2026 07:35:34 -0700 Subject: [PATCH] test: validate complete SSH relay perf suite with existing recovery fix --- .github/workflows/e2e.yml | 356 ++-------------------- src/relay/relay-pty-source-activation.ts | 11 +- src/relay/relay-pty-source-publication.ts | 7 +- tests/e2e/ssh-docker-relay-perf.spec.ts | 84 ++++- 4 files changed, 100 insertions(+), 358 deletions(-) diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index a94a7ea2ba5..18e360a45ec 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -1,350 +1,30 @@ -name: E2E - -run-name: E2E ${{ inputs.ref || github.ref }} - -# Why: checkout + artifact upload only; callers can only further restrict. +name: Docker SSH full performance validation +on: + workflow_dispatch: permissions: contents: read - -on: - workflow_call: - inputs: - ref: - description: Ref to check out (defaults to the calling workflow's ref) - required: false - type: string - test_files: - description: JSON array of changed specs; empty runs the full suite - required: false - type: string - ssh_source_changed: - description: '"true" when the PR touches SSH execution source; gates the Docker-SSH lane' - required: false - type: string - workflow_dispatch: - inputs: - ref: - description: Ref to check out (defaults to the workflow ref) - required: false - type: string - test_files: - description: JSON array of specs to run; empty runs the full suite - required: false - type: string - schedule: - # Why: GitHub cron uses UTC; these slots map to 10am and 3pm - # America/Phoenix for the default-branch E2E run. - - cron: '0 17,22 * * *' - jobs: - build: - name: build e2e app - runs-on: ubuntu-latest - timeout-minutes: 10 - - steps: - - name: Checkout - uses: actions/checkout@v6 - with: - ref: ${{ inputs.ref || github.ref }} - - # Why: the build's plain-Node daemon smoke load resolves node-pty. - - uses: ./.github/actions/install-node-dependencies - with: - native-runtime: node - - # Why: building here avoids parallel builds inside Playwright globalSetup; - # paired-browser specs also need the standalone web bundle. - - name: Build E2E outputs - env: - VITE_EXPOSE_STORE: 'true' - run: | - status=0 - pnpm run build:relay & - relay_pid=$! - npx electron-vite build --mode e2e || status=1 - pnpm run build:web-from-renderer || status=1 - wait "$relay_pid" || status=1 - exit "$status" - - - name: Upload E2E build output - uses: actions/upload-artifact@v7 - with: - name: e2e-build-out - path: out/ - # Why: build-relay.mjs writes each relay's marker as `out/relay//.version`, - # and upload-artifact drops dotfiles by default — consumers then fail SSH specs with - # "local relay build is missing its version marker". - include-hidden-files: true - retention-days: 1 - if-no-files-found: error - - # Build Electron-native dependencies once per workflow. Consumer shards restore - # this immutable cache instead of compiling the same ABI concurrently. - prepare-native-cache: - name: prepare Electron native cache - runs-on: ubuntu-latest - timeout-minutes: 15 - - steps: - - name: Checkout - uses: actions/checkout@v6 - with: - ref: ${{ inputs.ref || github.ref }} - - - name: Install native build tools - run: sudo apt-get update && sudo apt-get install -y build-essential python3 - - - uses: ./.github/actions/install-node-dependencies - with: - native-runtime: electron - - e2e: - name: e2e ${{ matrix.shard_name }} - needs: [build, prepare-native-cache] - if: inputs.test_files == '' + ssh: runs-on: ubuntu-latest timeout-minutes: 30 - strategy: - fail-fast: false - matrix: - include: - # Fourteen scheduled runs averaged 24.6 minutes per shard; shards 4 - # and 9 repeatedly hit the 30-minute cap. A 12-way trial still left - # one 30-minute shard, so 14 gives the suite enough failure headroom. - - shard: '1/14' - shard_name: 1-of-14 - - shard: '2/14' - shard_name: 2-of-14 - - shard: '3/14' - shard_name: 3-of-14 - - shard: '4/14' - shard_name: 4-of-14 - - shard: '5/14' - shard_name: 5-of-14 - - shard: '6/14' - shard_name: 6-of-14 - - shard: '7/14' - shard_name: 7-of-14 - - shard: '8/14' - shard_name: 8-of-14 - - shard: '9/14' - shard_name: 9-of-14 - - shard: '10/14' - shard_name: 10-of-14 - - shard: '11/14' - shard_name: 11-of-14 - - shard: '12/14' - shard_name: 12-of-14 - - shard: '13/14' - shard_name: 13-of-14 - - shard: '14/14' - shard_name: 14-of-14 - steps: - - name: Checkout - uses: actions/checkout@v6 + - uses: actions/checkout@v6 with: - ref: ${{ inputs.ref || github.ref }} - - # Native cache misses need the compiler, Electron needs Xvfb, and paired - # Quick Open needs ripgrep. Install them in one apt transaction per shard. - - name: Install native build and headless UI tools - run: sudo apt-get update && sudo apt-get install -y build-essential fonts-noto-cjk python3 ripgrep xvfb zsh openbox x11-utils - + persist-credentials: false + - name: Install headless tools + run: sudo apt-get update && sudo apt-get install -y build-essential openssh-client python3 ripgrep xvfb zsh openbox x11-utils - uses: ./.github/actions/install-node-dependencies with: native-runtime: electron - - - name: Download E2E build output - uses: actions/download-artifact@v8 + - name: Build relay and Electron + run: | + pnpm run build:relay + pnpm exec electron-vite build --mode e2e + - name: Run existing relay performance scenarios + run: xvfb-run --auto-servernum bash .github/scripts/e2e-with-window-manager.sh env SKIP_BUILD=1 ORCA_E2E_SSH_DOCKER=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm exec playwright test --config tests/playwright.config.ts tests/e2e/ssh-docker-relay-perf.spec.ts --project=electron-headless --workers=1 --repeat-each=3 + - uses: actions/upload-artifact@v7 + if: always() with: - name: e2e-build-out - path: out/ - - # Why: the Electron suite is wall-clock constrained on OSS runners, but - # multiple Electron apps on one Xvfb VM contend on git/Chromium resources. - # Sharding keeps each VM at one Playwright worker while splitting the - # headless suite across separate runners. - # SKIP_BUILD makes Playwright globalSetup reuse the single build job's - # artifact instead of starting five concurrent electron-vite builds. - # ORCA_E2E_FORWARD_APP_LOGS keeps startup failures visible when Electron - # launches but never creates a BrowserWindow. - - name: Run E2E tests (${{ matrix.shard_name }}) - run: xvfb-run --auto-servernum bash .github/scripts/e2e-with-window-manager.sh env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 ORCA_E2E_WEB_CLIENT=1 ORCA_RELAY_PATH="$GITHUB_WORKSPACE/out/relay" pnpm run test:e2e --shard=${{ matrix.shard }} - - # Why: Playwright retains traces/screenshots only on failure. Uploading - # them as an artifact makes post-mortem debugging on CI possible without - # re-running locally. - - name: Upload Playwright traces - if: failure() - uses: actions/upload-artifact@v7 - with: - name: playwright-traces-${{ matrix.shard_name }} + name: relay-perf-full-traces path: test-results/ - retention-days: 7 - if-no-files-found: ignore - - changed-e2e: - name: changed e2e specs - needs: [build, prepare-native-cache] - if: inputs.test_files != '' - runs-on: ubuntu-latest - # Why 45: pr.yml now maps SSH source edits onto Docker-backed specs, so this lane can - # pay a container image build plus ~22 serial SSH tests on top of the changed specs. - timeout-minutes: 45 - - steps: - - name: Checkout - uses: actions/checkout@v6 - with: - ref: ${{ inputs.ref || github.ref }} - - - name: Install native build and headless UI tools - # Why ripgrep: Quick Open's bounded host-side search requires rg instead of an - # unbounded inventory fallback; the paired fixture exercises that real boundary. - # Why openssh-client: the Docker-SSH fixture shells out to ssh/ssh-keygen, and this - # lane now receives those specs from pr.yml's SSH source mapping. - run: sudo apt-get update && sudo apt-get install -y build-essential fonts-noto-cjk openssh-client python3 ripgrep xvfb zsh openbox x11-utils - - - uses: ./.github/actions/install-node-dependencies - with: - native-runtime: electron - - - name: Download E2E build output - uses: actions/download-artifact@v8 - with: - name: e2e-build-out - path: out/ - - - name: Run changed E2E specs - env: - TEST_FILES_JSON: ${{ inputs.test_files }} - run: | - # Why the native IME spec is dropped: it test.skip()s itself without - # ORCA_E2E_NATIVE_IBUS_HANGUL, which this lane cannot set because it has no ibus - # session. Running it here reported a green skip as coverage. - mapfile -t TEST_FILES < <(jq -r '.[] | select( - . != "tests/e2e/ssh-startup-exec-readiness.spec.ts" and - . != "tests/e2e/paired-startup-exec-readiness.spec.ts" and - . != "tests/e2e/terminal-ibus-hangul-native.spec.ts" - )' <<<"$TEST_FILES_JSON") - if [ "${#TEST_FILES[@]}" -eq 0 ]; then - echo "Changed specs are all owned by dedicated lanes." - exit 0 - fi - E2E_ENV=(SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 ORCA_E2E_WEB_CLIENT=1 ORCA_RELAY_PATH="$GITHUB_WORKSPACE/out/relay") - # Second clause: a spec that reads ORCA_E2E_SSH_DOCKER test.skip()s itself without it, so - # naming only one trigger silently skipped every other Docker-SSH spec in this lane. - # The first clause stays because that spec needs Docker without referencing the variable. - if printf '%s\n' "${TEST_FILES[@]}" | grep -qx 'tests/e2e/ephemeral-vm-provisioned-root.spec.ts' \ - || grep -l 'ORCA_E2E_SSH_DOCKER' "${TEST_FILES[@]}" >/dev/null 2>&1; then - E2E_ENV+=(ORCA_E2E_SSH_DOCKER=1) - fi - E2E_PROJECT_ARGS=() - if grep -l '@headful' "${TEST_FILES[@]}" >/dev/null; then - E2E_PROJECT_ARGS+=(--project=electron-headful) - fi - xvfb-run --auto-servernum bash .github/scripts/e2e-with-window-manager.sh env "${E2E_ENV[@]}" \ - pnpm run test:e2e "${TEST_FILES[@]}" --workers=1 "${E2E_PROJECT_ARGS[@]}" - - - name: Upload Playwright traces - if: failure() - uses: actions/upload-artifact@v7 - with: - name: playwright-traces-changed - path: test-results/ - retention-days: 7 - if-no-files-found: ignore - - ssh-docker-watcher-isolation: - name: ssh docker watcher isolation - needs: [build, prepare-native-cache] - # effect of one route listing a startup-readiness spec — pruning that spec would have - # silently retired the whole lane. The signal is now derived from the SSH routes directly. - # The two spec clauses stay for their honest purpose: changed-e2e hands these specs to this - # lane, so editing one must still run it here. - if: >- - inputs.test_files == '' || - inputs.ssh_source_changed == 'true' || - contains(inputs.test_files, 'tests/e2e/ssh-startup-exec-readiness.spec.ts') || - contains(inputs.test_files, 'tests/e2e/paired-startup-exec-readiness.spec.ts') - runs-on: ubuntu-latest - # Why 60: this lane now also runs the remaining Docker-SSH specs serially. They average - # ~18s but several budget 4-10 minutes per test, so a slow run lands far above the old 35 - # — and the sharded lanes already show that a lane which times out is a lane nobody trusts. - timeout-minutes: 60 - - steps: - - name: Checkout - uses: actions/checkout@v6 - with: - ref: ${{ inputs.ref || github.ref }} - - - name: Install native build and headless UI tools - run: sudo apt-get update && sudo apt-get install -y build-essential fonts-noto-cjk openssh-client python3 ripgrep xvfb zsh openbox x11-utils - - - uses: ./.github/actions/install-node-dependencies - with: - native-runtime: electron - - - name: Download E2E build output - uses: actions/download-artifact@v8 - with: - name: e2e-build-out - path: out/ - - # Why: this is the release-path proof that the deployed Linux relay keeps - # its PTY and explorer live across a real watcher SIGSEGV. - - name: Run Docker SSH watcher isolation E2E - run: xvfb-run --auto-servernum bash .github/scripts/e2e-with-window-manager.sh env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm run test:e2e:ssh-docker-watcher-isolation - - # Why: Playwright empties test-results/ when it starts, so each step here used to - # destroy the previous step's traces. Only the last lane's failure was ever - # diagnosable from the artifact; set each lane aside before the next one runs. - - name: Keep watcher-isolation traces - if: always() - run: | - if [ -d test-results ]; then - mkdir -p e2e-traces - mv test-results "e2e-traces/watcher-isolation" - fi - - # Why always(): this lane gates SSH parking/retention plus startup-exec - # readiness across live SSH, headed paired, and headless serve topologies. - - name: Run Docker SSH terminal parking + startup readiness E2E - if: always() - run: xvfb-run --auto-servernum bash .github/scripts/e2e-with-window-manager.sh env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm run test:e2e:ssh-docker-terminal-parking - - - name: Keep terminal-parking traces - if: always() - run: | - if [ -d test-results ]; then - mkdir -p e2e-traces - mv test-results "e2e-traces/terminal-parking" - fi - - # Why here rather than the sharded lanes: the shards set no ORCA_E2E_SSH_DOCKER, so every - # spec below skipped itself while the shard still reported green. Running them on this one - # VM pays the fixture image build once instead of ten times, and keeps an SSH regression - # legible as an SSH-named failure. - - name: Run remaining Docker SSH E2E - if: always() - run: xvfb-run --auto-servernum bash .github/scripts/e2e-with-window-manager.sh env SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 pnpm run test:e2e:ssh-docker - - - name: Keep remaining-ssh-docker traces - if: always() - run: | - if [ -d test-results ]; then - mkdir -p e2e-traces - mv test-results "e2e-traces/remaining-ssh-docker" - fi - - - name: Upload watcher isolation traces - if: failure() - uses: actions/upload-artifact@v7 - with: - name: playwright-traces-ssh-docker-watcher-isolation - path: e2e-traces/ - retention-days: 7 - if-no-files-found: ignore + retention-days: 3 diff --git a/src/relay/relay-pty-source-activation.ts b/src/relay/relay-pty-source-activation.ts index d4ab4e06726..cab3c44fd85 100644 --- a/src/relay/relay-pty-source-activation.ts +++ b/src/relay/relay-pty-source-activation.ts @@ -4,7 +4,10 @@ import type { PtySourceRecoveryResult } from '../shared/pty-source-recovery-contract' import type { PtySourceReceivingActivation } from '../shared/pty-source-receiving-activation' -import type { PtySourceDeliveryIdentity } from '../shared/pty-source-credit-contract' +import type { + PtySourceDeliveryIdentity, + PtySourceDeliverySnapshot +} from '../shared/pty-source-credit-contract' import type { RequestContext } from './dispatcher' import type { RelayPtySourceDeliveryRecord, @@ -12,6 +15,12 @@ import type { } from './relay-pty-source-send-scheduler' import type { SshPtyConsumerSessionAdapter } from './ssh-pty-consumer-session-adapter' +export function boundedPtyRecoveryEnd(snapshot: PtySourceDeliverySnapshot): number { + const { receivedEndSu, creditedEndSu, windowSu } = snapshot + // Oversized quarantine cannot earn credit; fence at the checkpoint and drain it live. + return receivedEndSu - creditedEndSu > windowSu ? creditedEndSu : receivedEndSu +} + export function createPtySourceReceivingActivation( identity: PtySourceDeliveryIdentity, checkpointSourceEndSu: number, diff --git a/src/relay/relay-pty-source-publication.ts b/src/relay/relay-pty-source-publication.ts index 16b6e81c61b..c1959fd24d1 100644 --- a/src/relay/relay-pty-source-publication.ts +++ b/src/relay/relay-pty-source-publication.ts @@ -7,6 +7,7 @@ import type { PtySourceReceivingActivation } from '../shared/pty-source-receivin import { createPtySourceReceivingActivation, pendingPtySourceRecoveryResult, + boundedPtyRecoveryEnd, registerCanceledPtySourceRetirement, registerPtySourceActivationSettlement, samePtySourceRecoveryRequest @@ -66,9 +67,7 @@ export class RelayPtySourcePublication { recovery?: PtySourceRecoveryRequest ): false | 'opened' | 'rotated' | 'existing' | PtySourceRecoveryResult { let current = this.deliveries.get(id) - // A superseded request can find the delivery its own replacement opened: releasing that fence - // resumes a send the replacement is still rotating, and cancelling it blanks the pane that owns - // it. So every bail-out below acts only on a record this caller still owns. + // Only release this caller's delivery; its replacement may still be rotating. const owned = current?.clientId === context?.clientId ? current : undefined if (!context?.onResponseSettled) { this.sender.releaseRotationFence(owned) @@ -141,7 +140,7 @@ export class RelayPtySourcePublication { identity = rotation.identity displayEnd = current.displayEnd recoveryCheckpointSourceEndSu = recovery.acceptedSourceEndSu - recoveryEndSu = snapshot.receivedEndSu + recoveryEndSu = boundedPtyRecoveryEnd(this.session.sourceDeliverySnapshot(identity)) recoveryWasSealed = snapshot.state === 'sealed-unsettled' this.counters.rotated++ } catch (error) { diff --git a/tests/e2e/ssh-docker-relay-perf.spec.ts b/tests/e2e/ssh-docker-relay-perf.spec.ts index fe39cfef709..0a34d381bac 100644 --- a/tests/e2e/ssh-docker-relay-perf.spec.ts +++ b/tests/e2e/ssh-docker-relay-perf.spec.ts @@ -1,3 +1,4 @@ +import type { Page } from '@stablyai/playwright-test' import { test, expect } from './helpers/orca-app' import { ensureTerminalVisible, waitForActiveWorktree, waitForSessionReady } from './helpers/store' import { @@ -32,6 +33,13 @@ type TypingMeasurement = { worstLatencyMs: number } +type SshRelayLoad = { + stopped: boolean + fileReads: number[] + gitRefreshes: number + errors: string[] +} + type SshPtyAckGateSnapshot = { gatedPtyCount: number heldAckCount: number @@ -63,14 +71,16 @@ function remoteTypingLoadScript(runId: string): string { 'let seq = 0', 'let frame = 0', 'let bg = null', + "let lastAck = ''", `process.stdout.write('REMOTE_TUI_READY_${runId}\\n')`, - "setTimeout(() => { bg = setInterval(() => { frame += 1; process.stdout.write('BG_' + frame + '_' + 'x'.repeat(4096) + '\\n') }, 8) }, 500)", + "setTimeout(() => { bg = setInterval(() => { frame += 1; process.stdout.write('BG_' + frame + '_' + 'x'.repeat(4096) + '\\n' + lastAck + '\\n') }, 8) }, 500)", "process.stdin.on('data', (chunk) => {", ' if (chunk.includes(String.fromCharCode(3))) { if (bg) clearInterval(bg); process.exit(0) }', ' for (const char of chunk) {', " if (char === '\\r' || char === '\\n') continue", ' seq += 1', - ` process.stdout.write('\\x1b[20;2HREMOTE_KEY_${runId}_' + seq + '_' + char + '\\n')`, + ` lastAck = 'REMOTE_KEY_${runId}_' + seq + '_' + char`, + " process.stdout.write('\\x1b[20;2H' + lastAck + '\\n')", ' }', '})' ].join(';') @@ -298,23 +308,41 @@ test.describe('Docker SSH relay perf', () => { // refreshes, mirroring file preview + source-control churn while typing. await orcaPage.evaluate( ({ targetId, files, repoPath }) => { - const state = { stopped: false, reads: 0, errors: [] as string[] } + const state: SshRelayLoad = { + stopped: false, + fileReads: files.map(() => 0), + gitRefreshes: 0, + errors: [] + } ;(window as unknown as { __sshRelayLoad: typeof state }).__sshRelayLoad = state - const loop = async (run: () => Promise): Promise => { + const loop = async ( + run: () => Promise, + completed: () => void + ): Promise => { while (!state.stopped) { try { await run() - state.reads += 1 + completed() } catch (err) { state.errors.push(String(err)) await new Promise((r) => setTimeout(r, 100)) } } } - for (const filePath of files) { - void loop(() => window.api.fs.readFile({ filePath, connectionId: targetId })) - } - void loop(() => window.api.git.status({ worktreePath: repoPath, connectionId: targetId })) + files.forEach((filePath, index) => { + void loop( + () => window.api.fs.readFile({ filePath, connectionId: targetId }), + () => { + state.fileReads[index] += 1 + } + ) + }) + void loop( + () => window.api.git.status({ worktreePath: repoPath, connectionId: targetId }), + () => { + state.gitRefreshes += 1 + } + ) }, { targetId: remote.targetId, @@ -322,24 +350,39 @@ test.describe('Docker SSH relay perf', () => { repoPath: DOCKER_SSH_RELAY_REMOTE_REPO_PATH } ) - // Let the bulk load ramp before measuring. - await orcaPage.waitForTimeout(1_000) + await expect + .poll( + async () => + orcaPage.evaluate(() => { + const load = (window as unknown as { __sshRelayLoad: SshRelayLoad }).__sshRelayLoad + if (load.errors.length > 0) { + throw new Error(load.errors.join('\n')) + } + return load.fileReads.every((reads) => reads > 0) && load.gitRefreshes > 0 + }), + { message: 'Both file streams and Git refreshes must engage before measuring latency' } + ) + .toBe(true) const measurement = await measureRemoteTyping(orcaPage, ptyId, runId) const load = await orcaPage.evaluate(() => { const state = ( window as unknown as { - __sshRelayLoad: { stopped: boolean; reads: number; errors: string[] } + __sshRelayLoad: SshRelayLoad } ).__sshRelayLoad state.stopped = true - return { reads: state.reads, errors: state.errors.slice(0, 3) } + return { + fileReads: state.fileReads, + gitRefreshes: state.gitRefreshes, + errors: state.errors.slice(0, 3) + } }) const summary = `median=${measurement.medianLatencyMs.toFixed(1)}ms ` + `worst=${measurement.worstLatencyMs.toFixed(1)}ms ` + - `bulkReads=${load.reads} ` + + `fileReads=${load.fileReads.join(',')} gitRefreshes=${load.gitRefreshes} ` + `samples=${measurement.latencies.map((value) => value.toFixed(1)).join(',')}` console.log(`[docker-ssh-relay-perf:busy] ${summary}`) testInfo.annotations.push({ @@ -350,11 +393,22 @@ test.describe('Docker SSH relay perf', () => { // The load must actually have been streaming and error-free, otherwise // the latency numbers prove nothing. expect(load.errors).toEqual([]) - expect(load.reads).toBeGreaterThan(0) + for (const reads of load.fileReads) { + expect(reads).toBeGreaterThan(0) + } + expect(load.gitRefreshes).toBeGreaterThan(0) expect(measurement.medianLatencyMs).toBeLessThan(MAX_MEDIAN_KEY_LATENCY_MS) expect(measurement.worstLatencyMs).toBeLessThan(MAX_WORST_KEY_LATENCY_MS) await stopRemoteLoad(orcaPage, ptyId) } finally { + await orcaPage + .evaluate(() => { + const load = (window as unknown as { __sshRelayLoad?: SshRelayLoad }).__sshRelayLoad + if (load) { + load.stopped = true + } + }) + .catch(() => undefined) cleanupDockerSshRelayTarget(target) } })