diff --git a/src/main/daemon/daemon-preflight-client-replacement.test.ts b/src/main/daemon/daemon-preflight-client-replacement.test.ts new file mode 100644 index 00000000000..a3cdf633bdd --- /dev/null +++ b/src/main/daemon/daemon-preflight-client-replacement.test.ts @@ -0,0 +1,80 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { mkdtempSync, rmSync } from 'node:fs' +import { DaemonClient } from './client' +import { DaemonServer } from './daemon-server' +import type { SubprocessHandle } from './session' + +type DaemonServerPrivate = { + pendingPtySpawnPreparations: Map> +} + +function createMockSubprocess(): SubprocessHandle { + return { + pid: 55555, + getForegroundProcess: vi.fn(() => null), + confirmForegroundProcess: vi.fn(async () => null), + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + forceKill: vi.fn(), + signal: vi.fn(), + onData: vi.fn(), + onExit: vi.fn(), + dispose: vi.fn() + } +} + +describe('daemon preflight client replacement', () => { + let dir: string + let server: DaemonServer + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'daemon-preflight-replacement-')) + }) + + afterEach(async () => { + await server?.shutdown() + rmSync(dir, { recursive: true, force: true }) + }) + + it('cancels preparation when a reconnect replaces the owning control socket', async () => { + let finishPreparation!: () => void + const preparation = new Promise((resolve) => { + finishPreparation = resolve + }) + const preparePtySpawn = vi.fn(() => preparation) + const spawnSubprocess = vi.fn(() => createMockSubprocess()) + const socketPath = join(dir, 'daemon.sock') + const tokenPath = join(dir, 'daemon.token') + server = new DaemonServer({ socketPath, tokenPath, preparePtySpawn, spawnSubprocess }) + await server.start() + + const original = new DaemonClient({ socketPath, tokenPath }) + const replacement = new DaemonClient({ socketPath, tokenPath }) + ;(original as unknown as { clientId: string }).clientId = 'reused-client-id' + ;(replacement as unknown as { clientId: string }).clientId = 'reused-client-id' + await original.ensureConnected() + original + .request('createOrAttach', { + sessionId: 'replacement-pending', + cols: 80, + rows: 24 + }) + .catch(() => { + /* replacement disconnects the original request */ + }) + await vi.waitFor(() => expect(preparePtySpawn).toHaveBeenCalledOnce()) + + await replacement.ensureConnected() + finishPreparation() + await vi.waitFor(() => + expect((server as unknown as DaemonServerPrivate).pendingPtySpawnPreparations.size).toBe(0) + ) + expect(spawnSubprocess).not.toHaveBeenCalled() + + replacement.disconnect() + original.disconnect() + }) +}) diff --git a/src/main/daemon/daemon-server.test.ts b/src/main/daemon/daemon-server.test.ts index 9e0f1155f05..bf7077ee774 100644 --- a/src/main/daemon/daemon-server.test.ts +++ b/src/main/daemon/daemon-server.test.ts @@ -49,6 +49,7 @@ function createMockSubprocess(): SubprocessHandle & { type DaemonServerPrivate = { server: Server | null + pendingPtySpawnPreparations: Map> host: { kill: (sessionId: string, opts?: { immediate?: boolean }) => void | Promise } @@ -346,9 +347,9 @@ describe('DaemonServer', () => { expect((server as unknown as DaemonServerPrivate).clients.size).toBe(0) ) finishPreparation() - await vi.waitFor(() => expect(preparePtySpawn).toHaveBeenCalledOnce()) - // Give the resumed preparation a beat to (not) spawn. - await new Promise((resolve) => setTimeout(resolve, 20)) + await vi.waitFor(() => + expect((server as unknown as DaemonServerPrivate).pendingPtySpawnPreparations.size).toBe(0) + ) expect(spawnSubprocess).not.toHaveBeenCalled() }) diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index 2cf8c32446c..2d367a7688e 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -462,6 +462,8 @@ export class DaemonServer { this.clients.set(hello.clientId, client) this.setupControlSocket(socket, hello.clientId) if (previous) { + // Why: reconnect reuses clientId before stale close fires; cancel the old owner's preflight at handoff. + this.cancelPendingPtySpawnPreparationsForClient(hello.clientId) this.recordFullyAuthenticatedDisconnect(previous.authenticatedPairEstablished) // Why: tear down the old sockets after installing the new owner so a stale close can't delete the replacement. previous.streamSocket?.destroy() diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index ca6661d6ae8..8609d97d18f 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -327,15 +327,10 @@ describe('LocalPtyProvider', () => { expect(spawnMock).toHaveBeenCalledOnce() }) - it('coalesces a concurrent same-session-id spawn onto the winner instead of orphaning it (F3)', async () => { + it('coalesces a concurrent same-session-id spawn before launching a redundant shell (F3)', async () => { spawnMock.mockClear() - // Distinct procs so we can prove the redundant shell is torn down, not tracked. const procA = { ...mockProc, pid: 1001 } - // Hold the kill spy separately: destroyPtyProcess neutralizes proc.kill after - // calling it, so the reference on procB is swapped out post-teardown. - const killB = vi.fn() - const procB = { ...mockProc, pid: 2002, kill: killB } - spawnMock.mockReturnValueOnce(procA).mockReturnValueOnce(procB) + spawnMock.mockReturnValueOnce(procA) // Hold both spawns past the existence check and inside the preflight so they // race to register the same id; release only after both are parked. @@ -347,7 +342,7 @@ describe('LocalPtyProvider', () => { ) const spawnA = provider.spawn({ cols: 80, rows: 24, sessionId: 'race-session' }) - const spawnB = provider.spawn({ cols: 80, rows: 24, sessionId: 'race-session' }) + const spawnB = provider.spawn({ cols: 132, rows: 44, sessionId: 'race-session' }) await vi.waitFor(() => expect(prepareMacosTccLoginShellMock).toHaveBeenCalledTimes(2)) releasePreflight() @@ -357,7 +352,8 @@ describe('LocalPtyProvider', () => { expect(b.isReattach).toBe(true) expect(b.pid).toBe(procA.pid) expect(provider.getPtyProcess('race-session')).toBe(procA) - expect(killB).toHaveBeenCalledOnce() + expect(spawnMock).toHaveBeenCalledOnce() + expect(procA.resize).toHaveBeenCalledWith(132, 44) }) it('calls node-pty spawn with correct args', async () => { diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index 39f46f2e47e..e83b5103d7a 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -365,6 +365,24 @@ function normalizeLocalCallerSessionId(sessionId: string | undefined): string | return requested } +function reattachLocalPty(id: string, cols: number, rows: number): PtySpawnResult | null { + const existing = ptyProcesses.get(id) + if (!existing) { + return null + } + try { + existing.resize(cols, rows) + } catch { + /* Existing PTY may reject resize during teardown; still return the live handle. */ + } + return { + id, + pid: existing.pid, + ...(ptyWslDistroById.has(id) ? { wslDistro: ptyWslDistroById.get(id) ?? null } : {}), + isReattach: true + } +} + /** * Normalizes node-pty foreground process strings to executable basenames. */ @@ -499,20 +517,9 @@ export class LocalPtyProvider implements IPtyProvider { if (pendingShutdown) { await pendingShutdown.promise } - const existing = ptyProcesses.get(reattachId) + const existing = reattachLocalPty(reattachId, args.cols, args.rows) if (existing) { - const existingWslDistro = ptyWslDistroById.get(reattachId) - try { - existing.resize(args.cols, args.rows) - } catch { - /* Existing PTY may reject resize during teardown; still return the live handle. */ - } - return { - id: reattachId, - pid: existing.pid, - ...(ptyWslDistroById.has(reattachId) ? { wslDistro: existingWslDistro ?? null } : {}), - isReattach: true - } + return existing } } const id = allocatePtyId(reattachId ?? undefined) @@ -787,6 +794,11 @@ export class LocalPtyProvider implements IPtyProvider { } await prepareLocalPtySpawn(id) + // Why: another same-id request can win while this one awaits preflight; attach before launching a redundant shell. + const concurrentWinner = reattachId ? reattachLocalPty(id, args.cols, args.rows) : null + if (concurrentWinner) { + return concurrentWinner + } const spawnResult = spawnShellWithFallback({ shellPath, shellArgs, @@ -820,25 +832,6 @@ export class LocalPtyProvider implements IPtyProvider { const spawnedShellIsWsl = process.platform === 'win32' && pathWin32.basename(shellPath).toLowerCase() === 'wsl.exe' const spawnedWslDistro = spawnedShellIsWsl ? (launchWslDistro ?? undefined) : null - // Why: a concurrent spawn for the same stable session id can register while we - // await the preflight; the tracking write below is synchronous from that await, - // so a live id here means we lost the race. Tear down this redundant shell and - // attach to the winner instead of orphaning an untracked, unkillable PTY (F3). - if (reattachId && ptyProcesses.has(id)) { - const winner = ptyProcesses.get(id) - try { - proc.kill() - } catch { - /* the redundant shell may already be gone */ - } - destroyPtyProcess(proc, { alreadyKilled: true }) - return { - id, - pid: winner?.pid ?? proc.pid, - ...(ptyWslDistroById.has(id) ? { wslDistro: ptyWslDistroById.get(id) ?? null } : {}), - isReattach: true - } - } createPtyPhysicalExit(id) ptyProcesses.set(id, proc) ptyInitialCwd.set(id, cwd) diff --git a/src/main/providers/macos-tcc-login-shell.test.ts b/src/main/providers/macos-tcc-login-shell.test.ts index 7ea47834eaf..565d40da9dd 100644 --- a/src/main/providers/macos-tcc-login-shell.test.ts +++ b/src/main/providers/macos-tcc-login-shell.test.ts @@ -51,6 +51,7 @@ describe('wrapShellSpawnForMacosTccAttribution', () => { } else { process.env.ORCA_DISABLE_MACOS_LOGIN_SHELL = origDisable } + vi.restoreAllMocks() vi.clearAllMocks() }) @@ -115,8 +116,10 @@ describe('wrapShellSpawnForMacosTccAttribution', () => { expect(console.warn).toHaveBeenCalledTimes(1) }) - it('re-probes after a transient timeout instead of caching the degrade (F1)', async () => { + it('backs off repeated transient timeouts instead of delaying every terminal spawn (F1)', async () => { setPlatform('darwin') + let now = 1_000 + vi.spyOn(Date, 'now').mockImplementation(() => now) // A probe our own SIGKILL cap killed proves nothing about PAM; it must retry. execFileMock.mockImplementation( (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { @@ -131,13 +134,27 @@ describe('wrapShellSpawnForMacosTccAttribution', () => { // Degraded probe fails open to a direct shell for this spawn... expect(wrapShellSpawnForMacosTccAttribution('/bin/zsh', ['-l']).file).toBe('/bin/zsh') - // ...but a later spawn re-runs the probe rather than reusing the timeout. + // ...without making every subsequent terminal pay the same 500 ms timeout. + await prepareMacosTccLoginShell() + expect(execFileMock).toHaveBeenCalledOnce() + + now += 5_000 await prepareMacosTccLoginShell() expect(execFileMock).toHaveBeenCalledTimes(2) + + // Repeated failures back off further rather than spawning login(1) every 5 seconds. + now += 5_000 + await prepareMacosTccLoginShell() + expect(execFileMock).toHaveBeenCalledTimes(2) + now += 5_000 + await prepareMacosTccLoginShell() + expect(execFileMock).toHaveBeenCalledTimes(3) }) it('self-heals to the login wrapper once a re-probe succeeds (F1)', async () => { setPlatform('darwin') + let now = 1_000 + vi.spyOn(Date, 'now').mockImplementation(() => now) let attempt = 0 execFileMock.mockImplementation( (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { @@ -154,6 +171,10 @@ describe('wrapShellSpawnForMacosTccAttribution', () => { await prepareMacosTccLoginShell() await prepareMacosTccLoginShell() + expect(execFileMock).toHaveBeenCalledOnce() + now += 5_000 + await prepareMacosTccLoginShell() + expect(execFileMock).toHaveBeenCalledTimes(2) expect(wrapShellSpawnForMacosTccAttribution('/bin/zsh', ['-l']).file).toBe('/usr/bin/login') }) diff --git a/src/main/providers/macos-tcc-login-shell.ts b/src/main/providers/macos-tcc-login-shell.ts index 177cb335352..bd92cacaac5 100644 --- a/src/main/providers/macos-tcc-login-shell.ts +++ b/src/main/providers/macos-tcc-login-shell.ts @@ -8,6 +8,8 @@ const MACOS_PRINTF_PATH = '/usr/bin/printf' const LOGIN_PREFLIGHT_TIMEOUT_MS = 500 const LOGIN_PREFLIGHT_MARKER = 'ORCA_LOGIN_PREFLIGHT_OK' const LOGIN_PREFLIGHT_MAX_BUFFER_BYTES = 1024 +const LOGIN_PREFLIGHT_RETRY_BASE_MS = 5_000 +const LOGIN_PREFLIGHT_RETRY_MAX_MS = 5 * 60_000 /** * Env escape hatch to force the plain (unwrapped) spawn. Set to `1`/`true` if a @@ -29,12 +31,20 @@ export type LoginPreflightOutcome = { let cachedLoginPreflightResult: boolean | null = null let loginPreflightInFlight: Promise | null = null +let transientLoginPreflightFailure: { failureCount: number; retryAtMs: number } | null = null function isDisabledByEnv(): boolean { const value = process.env[DISABLE_ENV_VAR] return value === '1' || value === 'true' } +function loginPreflightRetryDelayMs(failureCount: number): number { + return Math.min( + LOGIN_PREFLIGHT_RETRY_MAX_MS, + LOGIN_PREFLIGHT_RETRY_BASE_MS * 2 ** Math.max(0, failureCount - 1) + ) +} + function classifyPreflightError(error: ExecFileException): LoginPreflightOutcome { // Why: our SIGKILL timeout cap (and maxBuffer, which also kills) is an // environmental slow-path, not a PAM verdict — retry, don't cache (F1). @@ -118,6 +128,13 @@ function loginPreflightSucceeds( // environmental and must be retried next spawn, not stuck forever (F1). if (outcome.conclusive) { cachedLoginPreflightResult = outcome.ok + transientLoginPreflightFailure = null + } else { + const failureCount = (transientLoginPreflightFailure?.failureCount ?? 0) + 1 + transientLoginPreflightFailure = { + failureCount, + retryAtMs: Date.now() + loginPreflightRetryDelayMs(failureCount) + } } if (!outcome.ok) { console.warn('[pty] macOS login(1) preflight failed; spawning shells directly') @@ -148,6 +165,10 @@ export async function prepareMacosTccLoginShell(): Promise