fix(terminal): bound preflight recovery work

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<string, Set<unknown>>
+}
+
+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<void>((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()
+  })
+})

src/main/daemon/daemon-server.test.ts

@@ -49,6 +49,7 @@ function createMockSubprocess(): SubprocessHandle & {
 
 type DaemonServerPrivate = {
   server: Server | null
+  pendingPtySpawnPreparations: Map<string, Set<unknown>>
   host: {
     kill: (sessionId: string, opts?: { immediate?: boolean }) => void | Promise<void>
   }
@@ -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()
     })
 

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()

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 () => {

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)

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')
   })
 

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<LoginPreflightOutcome> | 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<LoginPreflightOutcome
   if (cachedLoginPreflightResult !== null) {
     return null
   }
+  // Why: a persistently hung probe must not add 500 ms and a subprocess to every terminal spawn.
+  if (transientLoginPreflightFailure && Date.now() < transientLoginPreflightFailure.retryAtMs) {
+    return null
+  }
   if (!existsSync(MACOS_LOGIN_PATH)) {
     return null
   }
@@ -170,6 +191,7 @@ export async function prepareMacosTccLoginShell(): Promise<LoginPreflightOutcome
 export function resetMacosLoginShellPreflightForTests(): void {
   cachedLoginPreflightResult = null
   loginPreflightInFlight = null
+  transientLoginPreflightFailure = null
 }
 
 /**