From 69befad13ecc902047bf447067db923e9f76c1eb Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:09:09 -0700 Subject: [PATCH] Harden layout validation, watcher lifecycle, and connection robustness (#7924) * Harden layout validation, watcher lifecycle, and connection robustness - Throw instead of silently skipping when the packaged daemon-entry is missing, preventing layout regressions from passing build checks. - Terminate idle parcel-watcher processes to reclaim native handles and avoid crash-prone native node module teardowns on shutdown. - Bind the persisted WS fallback port first to prevent orphaning active mobile pairings when the preferred port becomes free again. - Cap concurrent disk reads for restored dirty tab verification at three to prevent startup connection bottlenecks on remote SSH workspaces. * Queue file IDs instead of snapshots in restored conflict scans This avoids using stale file snapshots (e.g., outdated disk signatures) if a tab is saved, closed, or re-baselined while waiting in the queue behind the concurrency limit. The live state is now fetched from the store and validated immediately before initiating the disk read. --- config/electron-builder.config.cjs | 11 ++- .../scripts/electron-builder-config.test.mjs | 9 ++ .../scripts/verify-packaged-daemon-entry.cjs | 28 ++++-- .../verify-packaged-daemon-entry.test.mjs | 57 ++++++++++++ package.json | 4 +- src/main/index.ts | 4 +- src/main/ipc/parcel-watcher-process.test.ts | 86 +++++++++++++++++- src/main/ipc/parcel-watcher-process.ts | 26 ++++++ .../runtime/rpc/ws-fallback-port-store.ts | 4 +- src/main/runtime/rpc/ws-transport.test.ts | 71 +++++++++++++++ src/main/runtime/rpc/ws-transport.ts | 62 ++++++------- src/main/runtime/runtime-rpc.ts | 6 +- .../editor-restored-tab-conflict-scan.test.ts | 88 +++++++++++++++++++ .../editor-restored-tab-conflict-scan.ts | 48 +++++++++- 14 files changed, 453 insertions(+), 51 deletions(-) create mode 100644 config/scripts/verify-packaged-daemon-entry.test.mjs diff --git a/config/electron-builder.config.cjs b/config/electron-builder.config.cjs index 0148b3cba..61f38c081 100644 --- a/config/electron-builder.config.cjs +++ b/config/electron-builder.config.cjs @@ -2,7 +2,10 @@ const { chmodSync, existsSync, readdirSync } = require('node:fs') const { execFileSync } = require('node:child_process') const { join, resolve } = require('node:path') const electronBuilderNativeRebuild = require('./scripts/electron-builder-native-rebuild.cjs') -const { verifyPackagedDaemonEntryBoots } = require('./scripts/verify-packaged-daemon-entry.cjs') +const { + assertPackagedDaemonEntryExists, + verifyPackagedDaemonEntryBoots +} = require('./scripts/verify-packaged-daemon-entry.cjs') const { createPackagedRuntimeNodeModuleResources, prunePackagedRuntimeNodeModules, @@ -141,8 +144,12 @@ module.exports = { if (context.arch === hostArchEnum || context.arch === 4) { verifyPackagedDaemonEntryBoots(resourcesDir) } else { + // Why: a cross-arch slice can't be booted by the host Node, but the + // unpacked entry must still exist — its absence is a layout regression + // regardless of arch, so only the boot is skipped, not the check. + assertPackagedDaemonEntryExists(resourcesDir) console.log( - `[verify-packaged-daemon-entry] skipped cross-arch slice (target ${context.arch}, host ${process.arch})` + `[verify-packaged-daemon-entry] skipped boot on cross-arch slice (target ${context.arch}, host ${process.arch})` ) } chmodUnixCliLaunchers(resourcesDir, context.electronPlatformName) diff --git a/config/scripts/electron-builder-config.test.mjs b/config/scripts/electron-builder-config.test.mjs index 850b8810f..cd46c8003 100644 --- a/config/scripts/electron-builder-config.test.mjs +++ b/config/scripts/electron-builder-config.test.mjs @@ -353,6 +353,15 @@ describe('electron-builder config', () => { const launcherPath = join(resourcesDir, 'bin', 'orca-ide') await mkdir(join(resourcesDir, 'bin'), { recursive: true }) await mkdir(join(resourcesDir, 'node_modules', 'zod', 'src'), { recursive: true }) + // Why: afterPack now fails hard when the unpacked daemon entry is + // missing, so the fixture must carry one like a real package layout. + const unpackedMainDir = join(resourcesDir, 'app.asar.unpacked', 'out', 'main') + await mkdir(unpackedMainDir, { recursive: true }) + await writeFile( + join(unpackedMainDir, 'daemon-entry.js'), + 'console.error("Usage: daemon-entry "); process.exit(1)\n', + 'utf8' + ) await writeFile(launcherPath, '#!/usr/bin/env bash\n', { encoding: 'utf8', mode: 0o644 }) await electronBuilderConfig.afterPack({ diff --git a/config/scripts/verify-packaged-daemon-entry.cjs b/config/scripts/verify-packaged-daemon-entry.cjs index 6ad91285c..8f5e80282 100644 --- a/config/scripts/verify-packaged-daemon-entry.cjs +++ b/config/scripts/verify-packaged-daemon-entry.cjs @@ -2,6 +2,24 @@ const { existsSync } = require('node:fs') const { spawnSync } = require('node:child_process') const { join } = require('node:path') +// Why: `asarUnpack` in config/electron-builder.config.cjs lists +// out/main/daemon-entry.js on every platform, and the packaged daemon fork +// (src/main/daemon/daemon-init.ts) resolves exactly this unpacked path. A +// missing entry means the package layout regressed, so the check throws +// instead of skipping — a silent skip false-passed exactly the layout bug +// this gate exists to catch. +function assertPackagedDaemonEntryExists(resourcesDir) { + const entryPath = join(resourcesDir, 'app.asar.unpacked', 'out', 'main', 'daemon-entry.js') + if (!existsSync(entryPath)) { + throw new Error( + `[verify-packaged-daemon-entry] missing unpacked daemon entry at ${entryPath} — ` + + `asarUnpack expects out/main/daemon-entry.js on every platform, so the packaged ` + + `daemon cannot be forked from this layout` + ) + } + return entryPath +} + // Why: v1.4.129-rc.1 shipped a terminal daemon that could not load (an electron // `require` leaked into its bundle) while every build check passed. This boots // the PACKAGED daemon-entry under plain Node against the asar-unpacked layout, @@ -14,13 +32,7 @@ const { join } = require('node:path') // /resources elsewhere). execPath defaults to the packaging Node. function verifyPackagedDaemonEntryBoots(resourcesDir, options = {}) { const execPath = options.execPath || process.execPath - const entryPath = join(resourcesDir, 'app.asar.unpacked', 'out', 'main', 'daemon-entry.js') - if (!existsSync(entryPath)) { - // Why: some targets/layouts do not unpack here; skip rather than fail so - // the hook stays safe across platforms it has not verified. - console.log(`[verify-packaged-daemon-entry] skipped — no unpacked entry at ${entryPath}`) - return - } + const entryPath = assertPackagedDaemonEntryExists(resourcesDir) const result = spawnSync(execPath, [entryPath], { encoding: 'utf8', timeout: 10_000 }) if (result.error) { @@ -43,4 +55,4 @@ function verifyPackagedDaemonEntryBoots(resourcesDir, options = {}) { console.log('[verify-packaged-daemon-entry] OK — packaged daemon-entry loads under plain Node') } -module.exports = { verifyPackagedDaemonEntryBoots } +module.exports = { assertPackagedDaemonEntryExists, verifyPackagedDaemonEntryBoots } diff --git a/config/scripts/verify-packaged-daemon-entry.test.mjs b/config/scripts/verify-packaged-daemon-entry.test.mjs new file mode 100644 index 000000000..8705953ba --- /dev/null +++ b/config/scripts/verify-packaged-daemon-entry.test.mjs @@ -0,0 +1,57 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { createRequire } from 'node:module' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' + +const require = createRequire(import.meta.url) +const { + assertPackagedDaemonEntryExists, + verifyPackagedDaemonEntryBoots +} = require('./verify-packaged-daemon-entry.cjs') + +describe('verify-packaged-daemon-entry', () => { + let resourcesDir + + beforeEach(() => { + resourcesDir = mkdtempSync(join(tmpdir(), 'orca-daemon-entry-verify-')) + }) + + afterEach(() => { + rmSync(resourcesDir, { recursive: true, force: true }) + }) + + function writePackagedEntry(source) { + const entryDir = join(resourcesDir, 'app.asar.unpacked', 'out', 'main') + mkdirSync(entryDir, { recursive: true }) + writeFileSync(join(entryDir, 'daemon-entry.js'), source) + } + + // Why: a silent skip on a missing entry false-passed exactly the packaged + // layout regression this gate exists to catch (rc.1 daemon-load incident). + it('throws when the unpacked daemon entry is missing', () => { + expect(() => assertPackagedDaemonEntryExists(resourcesDir)).toThrow( + /missing unpacked daemon entry/ + ) + expect(() => verifyPackagedDaemonEntryBoots(resourcesDir)).toThrow( + /missing unpacked daemon entry/ + ) + }) + + it('passes when the packaged entry loads and reaches argv parsing', () => { + writePackagedEntry('console.error("Usage: daemon-entry "); process.exit(1)\n') + expect(() => verifyPackagedDaemonEntryBoots(resourcesDir)).not.toThrow() + }) + + it('fails when the packaged entry cannot resolve its module graph', () => { + writePackagedEntry('require("orca-module-that-does-not-exist")\n') + expect(() => verifyPackagedDaemonEntryBoots(resourcesDir)).toThrow( + /failed to load under plain Node/ + ) + }) + + it('fails when the packaged entry never reaches argv parsing', () => { + writePackagedEntry('process.exit(0)\n') + expect(() => verifyPackagedDaemonEntryBoots(resourcesDir)).toThrow(/did not reach argv parsing/) + }) +}) diff --git a/package.json b/package.json index 7112a4f39..a24b1d1d6 100644 --- a/package.json +++ b/package.json @@ -69,8 +69,8 @@ "build:unpack": "pnpm run build && pnpm run ensure:electron-runtime && electron-builder --config config/electron-builder.config.cjs --dir", "build:win": "pnpm run build:desktop && pnpm run ensure:electron-runtime && electron-builder --config config/electron-builder.config.cjs --win", "build:icons": "bash resources/icon-source/generate.sh", - "build:mac": "pnpm run build:desktop && pnpm run build:computer-macos && pnpm run ensure:electron-runtime && electron-builder --config config/electron-builder.config.cjs --mac", - "build:mac:release": "node config/scripts/verify-macos-release-env.mjs && ORCA_MAC_RELEASE=1 pnpm run build:desktop && ORCA_MAC_RELEASE=1 pnpm run build:computer-macos && pnpm run ensure:electron-runtime && ORCA_MAC_RELEASE=1 electron-builder --config config/electron-builder.config.cjs --mac", + "build:mac": "pnpm run build:desktop && pnpm run build:computer-macos && pnpm run build:notification-status-macos && pnpm run ensure:electron-runtime && electron-builder --config config/electron-builder.config.cjs --mac", + "build:mac:release": "node config/scripts/verify-macos-release-env.mjs && ORCA_MAC_RELEASE=1 pnpm run build:desktop && ORCA_MAC_RELEASE=1 pnpm run build:computer-macos && ORCA_MAC_RELEASE=1 pnpm run build:notification-status-macos && pnpm run ensure:electron-runtime && ORCA_MAC_RELEASE=1 electron-builder --config config/electron-builder.config.cjs --mac", "build:linux": "pnpm run build:desktop && pnpm run ensure:electron-runtime && electron-builder --config config/electron-builder.config.cjs --linux AppImage deb", "test:e2e": "pnpm run ensure:electron-runtime && npx playwright test --config tests/playwright.config.ts --project electron-headless", "test:e2e:floating-mobile-emulator": "pnpm run ensure:electron-runtime && npx playwright test tests/e2e/floating-mobile-emulator-tab.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1", diff --git a/src/main/index.ts b/src/main/index.ts index 9e80bcc29..8c2a5a4e1 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -2047,7 +2047,9 @@ app.whenReady().then(async () => { // a random OS-assigned port — breaking deterministic mobile pairing/repro // scripts against the dev instance. Pin the first dev instance to 6769 so // ws://127.0.0.1:6769 is stable; a second dev instance still falls back via - // ws-transport's EADDRINUSE handler. + // ws-transport's EADDRINUSE handler. Note: once an instance has ever fallen + // back, the persisted fallback port is re-bound in preference to 6769 + // (STA-1511) until mobile-ws-fallback-port.json is removed from userData. const devWsPort = is.dev && !isE2E ? 6769 : undefined let serveOptions: ServeOptions | null = null try { diff --git a/src/main/ipc/parcel-watcher-process.test.ts b/src/main/ipc/parcel-watcher-process.test.ts index cb5caf2ea..4df3b666f 100644 --- a/src/main/ipc/parcel-watcher-process.test.ts +++ b/src/main/ipc/parcel-watcher-process.test.ts @@ -103,8 +103,13 @@ describe('subscribeViaWatcherProcess', () => { it('resolves unsubscribe on the child ack', async () => { const promise = subscribeViaWatcherProcess('/repo', vi.fn(), {}) const child = currentChild() - const id = ackSubscribe(child) + const id = ackSubscribe(child, 0) const subscription = await promise + // Why: hold a second subscription so this unsubscribe exercises the ack + // path instead of the last-subscriber idle kill. + const keepAlivePromise = subscribeViaWatcherProcess('/other', vi.fn(), {}) + ackSubscribe(child) + await keepAlivePromise const unsubPromise = subscription.unsubscribe() expect(child.sent.at(-1)).toEqual({ op: 'unsubscribe', id }) @@ -115,8 +120,11 @@ describe('subscribeViaWatcherProcess', () => { it('resolves a pending unsubscribe when the child dies', async () => { const promise = subscribeViaWatcherProcess('/repo', vi.fn(), {}) const child = currentChild() - ackSubscribe(child) + ackSubscribe(child, 0) const subscription = await promise + const keepAlivePromise = subscribeViaWatcherProcess('/other', vi.fn(), {}) + ackSubscribe(child) + await keepAlivePromise const unsubPromise = subscription.unsubscribe() child.connected = false @@ -187,6 +195,80 @@ describe('subscribeViaWatcherProcess', () => { ) }) + it('kills the idle child after the last unsubscribe and respawns on the next subscribe', async () => { + const promise = subscribeViaWatcherProcess('/repo', vi.fn(), {}) + const first = currentChild() + ackSubscribe(first) + const subscription = await promise + + await expect(subscription.unsubscribe()).resolves.toBeUndefined() + expect(first.kill).toHaveBeenCalledTimes(1) + + // Why: the deliberate idle kill must not count as a crash — no respawn + // and no crash-fuse advance when the killed child's exit event lands. + first.connected = false + first.emit('exit', null, 'SIGTERM') + expect(forkMock).toHaveBeenCalledTimes(1) + + const respawnPromise = subscribeViaWatcherProcess('/repo2', vi.fn(), {}) + expect(forkMock).toHaveBeenCalledTimes(2) + const second = currentChild() + expect(second).not.toBe(first) + expect(second.sent[0]).toMatchObject({ op: 'subscribe', dir: '/repo2' }) + ackSubscribe(second) + await expect(respawnPromise).resolves.toMatchObject({ unsubscribe: expect.any(Function) }) + }) + + it('kills the idle child when the last pending subscribe fails', async () => { + const promise = subscribeViaWatcherProcess('/gone', vi.fn(), {}) + const first = currentChild() + first.emit('message', { + op: 'subscribe-failed', + id: first.sent[0].id, + message: 'Error opening directory' + }) + await expect(promise).rejects.toThrow('Error opening directory') + expect(first.kill).toHaveBeenCalledTimes(1) + + const respawnPromise = subscribeViaWatcherProcess('/repo', vi.fn(), {}) + expect(forkMock).toHaveBeenCalledTimes(2) + ackSubscribe(currentChild()) + await expect(respawnPromise).resolves.toMatchObject({ unsubscribe: expect.any(Function) }) + }) + + it('keeps the child alive when a subscribe fails while other subscriptions remain', async () => { + const firstPromise = subscribeViaWatcherProcess('/a', vi.fn(), {}) + const child = currentChild() + ackSubscribe(child) + await firstPromise + + const failingPromise = subscribeViaWatcherProcess('/gone', vi.fn(), {}) + child.emit('message', { op: 'subscribe-failed', id: child.sent.at(-1)!.id, message: 'boom' }) + await expect(failingPromise).rejects.toThrow('boom') + expect(child.kill).not.toHaveBeenCalled() + }) + + it('keeps the child alive while other subscriptions remain', async () => { + const firstPromise = subscribeViaWatcherProcess('/a', vi.fn(), {}) + const child = currentChild() + ackSubscribe(child) + const first = await firstPromise + const secondPromise = subscribeViaWatcherProcess('/b', vi.fn(), {}) + ackSubscribe(child) + const second = await secondPromise + + const firstUnsub = first.unsubscribe() + expect(child.kill).not.toHaveBeenCalled() + expect(child.sent.at(-1)).toMatchObject({ op: 'unsubscribe' }) + + // The last unsubscribe kills the idle child, which also completes the + // still-pending unsubscribe ack from the first record. + const secondUnsub = second.unsubscribe() + expect(child.kill).toHaveBeenCalledTimes(1) + await expect(firstUnsub).resolves.toBeUndefined() + await expect(secondUnsub).resolves.toBeUndefined() + }) + it('uses the in-process watcher under vitest', async () => { vi.stubEnv('VITEST', 'true') const unsubscribe = vi.fn().mockResolvedValue(undefined) diff --git a/src/main/ipc/parcel-watcher-process.ts b/src/main/ipc/parcel-watcher-process.ts index 25e52794a..58ad60970 100644 --- a/src/main/ipc/parcel-watcher-process.ts +++ b/src/main/ipc/parcel-watcher-process.ts @@ -183,6 +183,9 @@ function handleChildMessage(message: WatcherToHostMessage): void { } else { record.callback(new Error(message.message), []) } + // Why: a failed last subscribe empties the records map without any + // unsubscribe ever being called — tear down the idle child here too. + killWatcherChildIfIdle() return } if (message.op === 'events') { @@ -243,6 +246,25 @@ function handleChildGone(proc: ChildProcess): void { } } +// Why: the last record gone leaves the child idle until app shutdown. Killing +// it releases native handles without running watcher.node's crash-prone async +// teardown (same rationale as disposeWatcherProcess) and reclaims the process; +// the next subscribe forks a fresh child. `child = null` before kill so the +// exit event neither counts as a crash nor respawns. Process death also +// completes any still-pending unsubscribe acks. +function killWatcherChildIfIdle(): void { + const proc = child + if (!proc || records.size > 0) { + return + } + child = null + for (const resolve of pendingUnsubscribes.values()) { + resolve() + } + pendingUnsubscribes.clear() + proc.kill() +} + function makeSubscription(record: SubscriptionRecord): WatcherProcessSubscription { return { unsubscribe: (): Promise => { @@ -253,6 +275,10 @@ function makeSubscription(record: SubscriptionRecord): WatcherProcessSubscriptio if (!proc?.connected) { return Promise.resolve() } + if (records.size === 0) { + killWatcherChildIfIdle() + return Promise.resolve() + } return new Promise((resolve) => { pendingUnsubscribes.set(record.id, resolve) sendToChild(proc, { op: 'unsubscribe', id: record.id }) diff --git a/src/main/runtime/rpc/ws-fallback-port-store.ts b/src/main/runtime/rpc/ws-fallback-port-store.ts index 93871cad6..beedcea8c 100644 --- a/src/main/runtime/rpc/ws-fallback-port-store.ts +++ b/src/main/runtime/rpc/ws-fallback-port-store.ts @@ -5,7 +5,9 @@ import { join } from 'node:path' // assigns a random port. Paired mobile devices store ws://ip:port endpoints, // so a port that changes on every restart permanently orphans those pairings // (STA-1511). Persist the assigned fallback so the same instance re-binds the -// same port next launch. +// same port next launch — the transport binds a persisted fallback BEFORE the +// preferred port, so pairings survive even when the preferred port is free +// again. const FALLBACK_PORT_FILE = 'mobile-ws-fallback-port.json' diff --git a/src/main/runtime/rpc/ws-transport.test.ts b/src/main/runtime/rpc/ws-transport.test.ts index a44469d1f..391e5e182 100644 --- a/src/main/runtime/rpc/ws-transport.test.ts +++ b/src/main/runtime/rpc/ws-transport.test.ts @@ -454,6 +454,77 @@ describe('WebSocketTransport', () => { return port } + it('binds the persisted fallback port even when the preferred port is free', async () => { + // Why: regression for the STA-1511 follow-up — devices paired while the + // fallback port was active store ws://ip:. A later launch that + // finds the preferred port free must still bind the fallback, or those + // pairings go permanently dead until the user re-pairs. + const preferredPort = await reserveFreePort() + const fallbackPort = await reserveFreePort() + + const transport = new WebSocketTransport({ + host: '127.0.0.1', + port: preferredPort, + fallbackPort + }) + transports.push(transport) + await transport.start() + expect(transport.resolvedPort).toBe(fallbackPort) + }) + + it('binds the preferred port when the persisted fallback is taken', async () => { + const fallbackHolder = new WebSocketTransport({ host: '127.0.0.1', port: 0 }) + transports.push(fallbackHolder) + await fallbackHolder.start() + const takenFallbackPort = fallbackHolder.resolvedPort + const preferredPort = await reserveFreePort() + + const transport = new WebSocketTransport({ + host: '127.0.0.1', + port: preferredPort, + fallbackPort: takenFallbackPort + }) + transports.push(transport) + await transport.start() + expect(transport.resolvedPort).toBe(preferredPort) + }) + + it('falls through to the preferred port when the fallback bind fails with a non-EADDRINUSE error', async () => { + // Why: a persisted fallback can land in an OS-reserved range on a later + // launch (Windows Hyper-V excluded ports → EACCES). That must degrade to + // the preferred port instead of disabling the transport for the session. + const preferredPort = await reserveFreePort() + const fallbackPort = await reserveFreePort() + const transport = new WebSocketTransport({ + host: '127.0.0.1', + port: preferredPort, + fallbackPort + }) + transports.push(transport) + const withListen = transport as unknown as { tryListen(port: number): Promise } + const realTryListen = withListen.tryListen.bind(transport) + withListen.tryListen = (port: number) => + port === fallbackPort + ? Promise.reject(Object.assign(new Error('listen EACCES'), { code: 'EACCES' })) + : realTryListen(port) + + await transport.start() + expect(transport.resolvedPort).toBe(preferredPort) + }) + + it('still throws when the preferred port fails with a non-EADDRINUSE error', async () => { + const transport = new WebSocketTransport({ + host: '127.0.0.1', + port: await reserveFreePort() + }) + transports.push(transport) + const withListen = transport as unknown as { tryListen(port: number): Promise } + withListen.tryListen = () => + Promise.reject(Object.assign(new Error('listen EACCES'), { code: 'EACCES' })) + + await expect(transport.start()).rejects.toThrow('listen EACCES') + }) + it('retries the persisted fallback port before an OS-assigned one', async () => { const holder = new WebSocketTransport({ host: '127.0.0.1', port: 0 }) transports.push(holder) diff --git a/src/main/runtime/rpc/ws-transport.ts b/src/main/runtime/rpc/ws-transport.ts index 7b1d8b8ea..8b88c7a82 100644 --- a/src/main/runtime/rpc/ws-transport.ts +++ b/src/main/runtime/rpc/ws-transport.ts @@ -48,10 +48,11 @@ export type WebSocketTransportOptions = { // Why: the pairing server can also serve the browser client, so users do // not need a second dev/static server once the web bundle is built. staticRoot?: string - // Why: when the preferred port is taken, an unstable OS-assigned port would - // change on every restart and permanently orphan existing mobile pairings - // (their stored ws://ip:port endpoint goes dead — STA-1511). Callers pass - // the previously assigned fallback port so it is retried first. + // Why: paired mobile devices store the full ws://ip:port endpoint. Once a + // fallback port has been assigned and persisted, devices paired while it was + // active point at it, so it must be bound FIRST on later launches — binding + // the (now free) preferred port instead would strand those pairings + // (STA-1511). Callers pass the previously assigned fallback port here. fallbackPort?: number } @@ -149,39 +150,40 @@ export class WebSocketTransport implements RpcTransport { return } - // Why: when the preferred port is occupied (e.g. another Orca instance is - // already running), fall back to an OS-assigned port so mobile pairing - // still works. The QR code reads resolvedPort after start, so it will - // advertise the correct port regardless. A persisted fallback port is - // retried before port 0 so paired devices keep a stable endpoint across - // restarts (STA-1511). - let port = this.port - try { - await this.tryListen(port) - return - } catch (error: unknown) { - if (!isEAddressInUse(error) || port === 0) { - throw error - } - } - if ( - this.fallbackPort !== undefined && - this.fallbackPort !== 0 && - this.fallbackPort !== this.port - ) { + // Why: a persisted fallback port is bound FIRST — devices paired while it + // was active store ws://ip: and would be permanently stranded if + // a later launch grabbed the (now free) preferred port instead (STA-1511). + // Without a persisted fallback the preferred port is tried first. On + // EADDRINUSE each candidate falls through to the next, ending at port 0 + // (OS-assigned) so mobile pairing still works when everything is taken. + // The QR code reads resolvedPort after start, so it always advertises the + // port that actually bound. + const persistedFallbackPort = + this.fallbackPort !== undefined && this.fallbackPort !== 0 && this.fallbackPort !== this.port + ? this.fallbackPort + : undefined + const candidatePorts = + persistedFallbackPort !== undefined ? [persistedFallbackPort, this.port] : [this.port] + for (const port of candidatePorts) { try { - console.warn( - `[ws-transport] Port ${port} is in use, retrying previous fallback port ${this.fallbackPort}` - ) - await this.tryListen(this.fallbackPort) + await this.tryListen(port) return } catch (error: unknown) { - if (!isEAddressInUse(error)) { + // Why: a persisted fallback can become unbindable for reasons beyond + // EADDRINUSE (e.g. Windows reserves dynamic-range ports for Hyper-V + // after a reboot → EACCES). Any fallback failure must degrade to the + // next candidate — aborting would disable the transport every launch + // while the store still names that port. Only preferred-port failures + // other than EADDRINUSE are fatal. + if (port !== persistedFallbackPort && (!isEAddressInUse(error) || port === 0)) { throw error } + console.warn( + `[ws-transport] Failed to bind port ${port} (${error instanceof Error ? error.message : String(error)}), trying next candidate` + ) } } - console.warn(`[ws-transport] Port ${port} is in use, falling back to OS-assigned port`) + console.warn('[ws-transport] All configured ports failed to bind, using an OS-assigned port') await this.tryListen(0) } diff --git a/src/main/runtime/runtime-rpc.ts b/src/main/runtime/runtime-rpc.ts index 8e05aaeec..f8b3c75c7 100644 --- a/src/main/runtime/runtime-rpc.ts +++ b/src/main/runtime/runtime-rpc.ts @@ -705,8 +705,10 @@ export class OrcaRuntimeRpcServer { port: this.wsPort, staticRoot: this.webClientRoot, // Why: keep the fallback port stable across restarts so paired - // devices' stored endpoints stay valid (STA-1511). wsPort 0 means - // the caller explicitly wants a random port (E2E) — don't pin it. + // devices' stored endpoints stay valid (STA-1511) — the transport + // binds a persisted fallback before the preferred port. wsPort 0 + // means the caller explicitly wants a random port (E2E) — don't + // pin it. ...(this.wsPort !== 0 ? { fallbackPort: readWsFallbackPort(this.userDataPath) } : {}) }) this.wsTransport = wsTransport diff --git a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts index 8c50887a0..b0a381668 100644 --- a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts +++ b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.test.ts @@ -250,6 +250,43 @@ describe('attachRestoredTabConflictScan', () => { } }) + it('caps concurrent verification reads and drains the queue without dropping tabs', async () => { + // Why: a restored session with many dirty tabs must not fire one disk + // read per tab at once — on SSH/remote runtimes that competes with + // connection recovery. The cap is 3; the rest queue and all complete. + const pendingReads: ((value: { content: string; isBinary: boolean }) => void)[] = [] + mocks.readRuntimeFileContent.mockImplementation( + () => + new Promise<{ content: string; isBinary: boolean }>((resolve) => { + pendingReads.push(resolve) + }) + ) + const store = createEditorStore() + for (let i = 0; i < 6; i++) { + openRestoredDirtyTab(store, `/repo/file-${i}.ts`, 'original baseline') + } + + const detach = attachRestoredTabConflictScan(store) + try { + expect(mocks.readRuntimeFileContent).toHaveBeenCalledTimes(3) + + pendingReads.shift()!({ content: 'original baseline', isBinary: false }) + await vi.advanceTimersByTimeAsync(10) + expect(mocks.readRuntimeFileContent).toHaveBeenCalledTimes(4) + + while (pendingReads.length > 0) { + pendingReads.shift()!({ content: 'original baseline', isBinary: false }) + await vi.advanceTimersByTimeAsync(10) + } + expect(mocks.readRuntimeFileContent).toHaveBeenCalledTimes(6) + for (const file of store.getState().openFiles) { + expect(file.pendingDiskBaselineVerification).toBeUndefined() + } + } finally { + detach() + } + }) + it('does not mark a tab that was saved while the read was in flight', async () => { let resolveRead: (value: { content: string; isBinary: boolean }) => void = () => {} mocks.readRuntimeFileContent.mockReturnValue( @@ -270,4 +307,55 @@ describe('attachRestoredTabConflictScan', () => { detach() } }) + + it('verifies a queued tab against its live baseline, not the stale queued snapshot', async () => { + // Why: ids are paths, so a tab re-baselined (saved / reopened) while it + // waits behind the concurrency cap must be compared against its current + // baseline. Queuing the OpenFile snapshot would mark the live tab using the + // old lastKnownDiskSignature. + const pending: { + filePath: string + resolve: (value: { content: string; isBinary: boolean }) => void + }[] = [] + mocks.readRuntimeFileContent.mockImplementation( + ({ filePath }: { filePath: string }) => + new Promise<{ content: string; isBinary: boolean }>((resolve) => { + pending.push({ filePath, resolve }) + }) + ) + const store = createEditorStore() + // Three reads fill the concurrency cap; the fourth waits in the queue. + for (let i = 0; i < 4; i++) { + openRestoredDirtyTab(store, `/repo/file-${i}.ts`, 'stale baseline') + } + + const detach = attachRestoredTabConflictScan(store) + try { + expect(mocks.readRuntimeFileContent).toHaveBeenCalledTimes(3) + + // The queued tab is re-baselined to match current disk while it waits. + store + .getState() + .setLastKnownDiskSignature('/repo/file-3.ts', getDiskBaselineSignature('current disk')) + + // Drain the in-flight reads; each frees a slot so the queued tab dispatches. + for (const filePath of ['/repo/file-0.ts', '/repo/file-1.ts', '/repo/file-2.ts']) { + pending + .find((p) => p.filePath === filePath)! + .resolve({ content: 'stale baseline', isBinary: false }) + await vi.advanceTimersByTimeAsync(10) + } + + pending + .find((p) => p.filePath === '/repo/file-3.ts')! + .resolve({ content: 'current disk', isBinary: false }) + await vi.advanceTimersByTimeAsync(10) + + const tab = store.getState().openFiles.find((f) => f.id === '/repo/file-3.ts') + expect(tab?.externalMutation).toBeUndefined() + expect(tab?.pendingDiskBaselineVerification).toBeUndefined() + } finally { + detach() + } + }) }) diff --git a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts index bc4029321..26d7e4db0 100644 --- a/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts +++ b/src/renderer/src/components/editor/editor-restored-tab-conflict-scan.ts @@ -29,13 +29,23 @@ type AppStoreApi = Pick, 'getState' | 'subscribe'> const VERIFY_RETRY_MS = 2_000 const VERIFY_SLOW_RETRY_MS = 15_000 const VERIFY_FAST_ATTEMPTS = 30 +// Why: a session restored with many dirty tabs would otherwise fire one disk +// read per tab simultaneously — on SSH/remote runtimes that's N concurrent +// 15s-timeout RPCs competing with connection recovery at startup. Verifies +// beyond the cap queue and start as slots free up; none are dropped. +const MAX_CONCURRENT_VERIFY_READS = 3 export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { - // Why: dedupes in-flight verifications; the store's pending flag is the - // durable "needs verification" signal. + // Why: dedupes queued + in-flight verifications; the store's pending flag is + // the durable "needs verification" signal. const inFlightFileIds = new Set() const attemptsByFileId = new Map() const retryTimers = new Set>() + // Why: queue ids, not OpenFile snapshots. Ids are paths, so a snapshot can go + // stale (close/reopen or save at the same path) while it waits for a slot; + // the live file is re-read at dispatch instead. + const verifyQueue: string[] = [] + let activeVerifyReads = 0 let disposed = false // Why: distinguishes "file was deleted while the app was closed" (a @@ -144,6 +154,36 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { } } + const pumpVerifyQueue = (): void => { + while (!disposed && activeVerifyReads < MAX_CONCURRENT_VERIFY_READS && verifyQueue.length > 0) { + const fileId = verifyQueue.shift()! + // Why: re-read the live file when the slot opens. A queued id may now + // resolve to a reopened or saved same-path tab, so re-validate it against + // the current state before spending a disk read; skipping frees the dedupe + // marker so a later scan can re-queue it if it needs verification again. + const file = store.getState().openFiles.find((f) => f.id === fileId) + if ( + !file || + !file.pendingDiskBaselineVerification || + !file.isDirty || + !file.lastKnownDiskSignature || + file.externalMutation === 'changed' || + !canAutoSaveOpenFile(file) + ) { + inFlightFileIds.delete(fileId) + continue + } + activeVerifyReads += 1 + const onSettled = (): void => { + activeVerifyReads -= 1 + pumpVerifyQueue() + } + // Why: verify() never rejects by design, but a rejection here must still + // release the slot or the queue stalls permanently. + void verify(file).then(onSettled, onSettled) + } + } + const scan = (): void => { if (disposed) { return @@ -160,8 +200,9 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { continue } inFlightFileIds.add(file.id) - void verify(file) + verifyQueue.push(file.id) } + pumpVerifyQueue() } let previousOpenFiles = store.getState().openFiles @@ -182,5 +223,6 @@ export function attachRestoredTabConflictScan(store: AppStoreApi): () => void { clearTimeout(timer) } retryTimers.clear() + verifyQueue.length = 0 } }