fix(terminal): harden macOS login-preflight spawn path (#9301 follow-ups)

Follow-ups to the #9301 two-pass review. Re-derived severities: none are
release blockers; these are low-likelihood robustness/observability fixes.

F1 macos-tcc-login-shell: only cache a conclusive PAM verdict. A probe our
own SIGKILL timeout / ETIMEDOUT / maxBuffer killed proves nothing about PAM,
so it no longer sticks the whole process into direct-spawn — it fails open
for that spawn, re-probes next time, and self-heals once PAM answers.

F2 daemon-entry: prepareMacosTccLoginShell returns the outcome; the daemon
emits a structured macos-login-preflight {ok, reason} log on degrade, since
detached daemons destroy stderr and lose the console.warn.

F3 local-pty-provider: an atomic guard before the tracking write coalesces a
concurrent same-session-id spawn onto the winner instead of orphaning an
untracked, unkillable PTY.

F4 daemon-server: preparations carry their clientId; a control-socket close
cancels only that client's pending preps, so a mid-preflight disconnect no
longer leaves a durable orphan daemon PTY.

F7 warm the PAM probe at daemon startup (mirrors warmPwshAvailabilityCache);
note the pipe-vs-PTY probe fidelity limit; test that kill with no pending
prep still surfaces SessionNotFoundError.

Deliberately skipped F5 (self-limits to next reload; a real fix needs
speculative in-flight-generation tracking) and F6 (inherent to legacy
v22 attach; self-heals once daemons are v23).

src/main/daemon/daemon-entry.ts

@@ -82,6 +82,18 @@ async function main(): Promise<void> {
   daemonLog.log('startup', { protocolVersion: PROTOCOL_VERSION, socketPath })
   void warmPwshAvailabilityCache()
 
+  // Why: detached daemons destroy stderr, so the preflight's console.warn is lost;
+  // surface a degraded TCC attribution here where it's diagnosable (F2).
+  const runMacosLoginPreflight = async (): Promise<void> => {
+    const outcome = await prepareMacosTccLoginShell()
+    if (outcome && !outcome.ok) {
+      daemonLog.log('macos-login-preflight', { ok: outcome.ok, reason: outcome.reason })
+    }
+  }
+  // Why: warm the PAM probe at idle startup so the first terminal spawn doesn't
+  // pay it under load — shrinking the window where a slow probe degrades (F1/F7).
+  void runMacosLoginPreflight()
+
   // Why: node-pty can throw a C++ Napi::Error that escapes all JS try/catch
   // blocks (e.g. writing to a PTY whose fd was closed between the native
   // exit signal and the JS onExit callback). Without this handler, Node's
@@ -152,7 +164,7 @@ async function main(): Promise<void> {
     ...(launchNonce ? { launchNonce } : {}),
     ...(pidPath ? { startedAtMs } : {}),
     log: daemonLog,
-    preparePtySpawn: prepareMacosTccLoginShell,
+    preparePtySpawn: runMacosLoginPreflight,
     spawnSubprocess: (opts) => createPtySubprocess(opts),
     onIdleShutdown: () => {
       shuttingDown = true

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

@@ -313,6 +313,56 @@ describe('DaemonServer', () => {
       expect(spawnSubprocess).not.toHaveBeenCalled()
     })
 
+    it('cancels a disconnecting client’s pending preparation to avoid an orphan PTY (F4)', async () => {
+      let finishPreparation!: () => void
+      const preparation = new Promise<void>((resolve) => {
+        finishPreparation = resolve
+      })
+      const preparePtySpawn = vi.fn(() => preparation)
+      const spawnSubprocess = vi.fn(() => createMockSubprocess())
+      server = new DaemonServer({
+        socketPath,
+        tokenPath,
+        preparePtySpawn,
+        spawnSubprocess
+      })
+      await server.start()
+      const c = await connectClient()
+
+      // Hangs in preflight; the control-socket close must abort it before spawn.
+      c.request('createOrAttach', {
+        sessionId: 'disconnect-pending',
+        cols: 80,
+        rows: 24
+      }).catch(() => {
+        /* the disconnect rejects the in-flight request; that's expected */
+      })
+      await vi.waitFor(() => expect(preparePtySpawn).toHaveBeenCalledOnce())
+
+      c.disconnect()
+      // Wait for the server to process the close (and cancel the prep) before
+      // releasing the preflight, else the resumed spawn races ahead of cancellation.
+      await vi.waitFor(() =>
+        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))
+      expect(spawnSubprocess).not.toHaveBeenCalled()
+    })
+
+    it('kill with no pending preparation still surfaces SessionNotFoundError (F7)', async () => {
+      await startServer()
+      const c = await connectClient()
+
+      // No preparation was canceled, so the host's not-found verdict must propagate
+      // rather than be swallowed by the pending-spawn kill reconciliation.
+      await expect(
+        c.request('kill', { sessionId: 'never-created', immediate: true })
+      ).rejects.toThrow('Session not found: never-created')
+    })
+
     it('persists only an allowlisted launch identity across reattach', async () => {
       await startServer()
       const c = await connectClient()

src/main/daemon/daemon-server.ts

@@ -75,6 +75,9 @@ type ConnectedClient = {
 
 type PendingPtySpawnPreparation = {
   canceled: boolean
+  // Why: preparations are keyed by sessionId, but a control-socket close must
+  // cancel only the disconnecting client's preps, not another client's (F4).
+  clientId: string
 }
 
 type PendingShutdownReply = {
@@ -497,6 +500,9 @@ export class DaemonServer {
       if (client?.controlSocket !== socket) {
         return
       }
+      // Why: a client that disconnects mid-preflight would otherwise still create
+      // its daemon PTY, orphaning a durable, unattached session — cancel its preps (F4).
+      this.cancelPendingPtySpawnPreparationsForClient(clientId)
       const wasFullyAuthenticated = client.authenticatedPairEstablished
       this.streamDataBatcher.clear(clientId)
       client.streamSocket?.destroy()
@@ -610,8 +616,8 @@ export class DaemonServer {
     this.pendingShutdownReplies.set(key, { start })
   }
 
-  private async preparePtySpawnUnlessCanceled(sessionId: string): Promise<void> {
-    const preparation: PendingPtySpawnPreparation = { canceled: false }
+  private async preparePtySpawnUnlessCanceled(sessionId: string, clientId: string): Promise<void> {
+    const preparation: PendingPtySpawnPreparation = { canceled: false, clientId }
     const pending = this.pendingPtySpawnPreparations.get(sessionId) ?? new Set()
     pending.add(preparation)
     this.pendingPtySpawnPreparations.set(sessionId, pending)
@@ -646,6 +652,16 @@ export class DaemonServer {
     }
   }
 
+  private cancelPendingPtySpawnPreparationsForClient(clientId: string): void {
+    for (const pending of this.pendingPtySpawnPreparations.values()) {
+      for (const preparation of pending) {
+        if (preparation.clientId === clientId) {
+          preparation.canceled = true
+        }
+      }
+    }
+  }
+
   private async routeRequest(clientId: string, request: DaemonRequest): Promise<unknown> {
     const client = this.clients.get(clientId)
 
@@ -662,7 +678,7 @@ export class DaemonServer {
         const p = request.payload
         let result: Awaited<ReturnType<TerminalHost['createOrAttach']>>
         try {
-          await this.preparePtySpawnUnlessCanceled(p.sessionId)
+          await this.preparePtySpawnUnlessCanceled(p.sessionId, clientId)
           result = await this.host.createOrAttach({
             sessionId: p.sessionId,
             cols: p.cols,

src/main/providers/local-pty-provider.test.ts

@@ -327,6 +327,39 @@ describe('LocalPtyProvider', () => {
       expect(spawnMock).toHaveBeenCalledOnce()
     })
 
+    it('coalesces a concurrent same-session-id spawn onto the winner instead of orphaning it (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)
+
+      // 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.
+      let releasePreflight!: () => void
+      prepareMacosTccLoginShellMock.mockReturnValue(
+        new Promise<void>((resolve) => {
+          releasePreflight = resolve
+        })
+      )
+
+      const spawnA = provider.spawn({ cols: 80, rows: 24, sessionId: 'race-session' })
+      const spawnB = provider.spawn({ cols: 80, rows: 24, sessionId: 'race-session' })
+      await vi.waitFor(() => expect(prepareMacosTccLoginShellMock).toHaveBeenCalledTimes(2))
+      releasePreflight()
+
+      const [a, b] = await Promise.all([spawnA, spawnB])
+      // The winner owns the tracked PTY; the loser attaches to it, not a second one.
+      expect(a.isReattach).toBeUndefined()
+      expect(b.isReattach).toBe(true)
+      expect(b.pid).toBe(procA.pid)
+      expect(provider.getPtyProcess('race-session')).toBe(procA)
+      expect(killB).toHaveBeenCalledOnce()
+    })
+
     it('calls node-pty spawn with correct args', async () => {
       await provider.spawn({ cols: 120, rows: 40, cwd: '/tmp' })
       expect(spawnMock).toHaveBeenCalledWith(

src/main/providers/local-pty-provider.ts

@@ -820,6 +820,25 @@ 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

@@ -96,11 +96,12 @@ describe('wrapShellSpawnForMacosTccAttribution', () => {
     )
   })
 
-  it('caches a failed login preflight for later terminal spawns', async () => {
+  it('caches a conclusive PAM rejection for later terminal spawns', async () => {
     setPlatform('darwin')
     execFileMock.mockImplementation(
       (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => {
-        callback(new Error('timed out'), '', '')
+        // login(1) exiting non-zero on its own is a deterministic PAM verdict.
+        callback(Object.assign(new Error('login incorrect'), { code: 1 }), '', '')
         return { stdin: { end: stdinEndMock } }
       }
     )
@@ -114,6 +115,64 @@ describe('wrapShellSpawnForMacosTccAttribution', () => {
     expect(console.warn).toHaveBeenCalledTimes(1)
   })
 
+  it('re-probes after a transient timeout instead of caching the degrade (F1)', async () => {
+    setPlatform('darwin')
+    // A probe our own SIGKILL cap killed proves nothing about PAM; it must retry.
+    execFileMock.mockImplementation(
+      (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => {
+        callback(Object.assign(new Error('timed out'), { killed: true }), '', '')
+        return { stdin: { end: stdinEndMock } }
+      }
+    )
+    vi.spyOn(console, 'warn').mockImplementation(() => {})
+
+    const first = await prepareMacosTccLoginShell()
+    expect(first).toEqual({ ok: false, conclusive: false, reason: 'timeout' })
+    // 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.
+    await prepareMacosTccLoginShell()
+    expect(execFileMock).toHaveBeenCalledTimes(2)
+  })
+
+  it('self-heals to the login wrapper once a re-probe succeeds (F1)', async () => {
+    setPlatform('darwin')
+    let attempt = 0
+    execFileMock.mockImplementation(
+      (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => {
+        attempt += 1
+        if (attempt === 1) {
+          callback(Object.assign(new Error('timed out'), { killed: true }), '', '')
+        } else {
+          callback(null, 'ORCA_LOGIN_PREFLIGHT_OK', '')
+        }
+        return { stdin: { end: stdinEndMock } }
+      }
+    )
+    vi.spyOn(console, 'warn').mockImplementation(() => {})
+
+    await prepareMacosTccLoginShell()
+    await prepareMacosTccLoginShell()
+    expect(wrapShellSpawnForMacosTccAttribution('/bin/zsh', ['-l']).file).toBe('/usr/bin/login')
+  })
+
+  it('returns a conclusive outcome the daemon can log structurally (F2)', async () => {
+    setPlatform('darwin')
+    execFileMock.mockImplementation(
+      (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => {
+        callback(null, 'Login incorrect\nlogin: ', '')
+        return { stdin: { end: stdinEndMock } }
+      }
+    )
+    vi.spyOn(console, 'warn').mockImplementation(() => {})
+
+    const outcome = await prepareMacosTccLoginShell()
+    expect(outcome).toEqual({ ok: false, conclusive: true, reason: 'rejected' })
+    // A second call is short-circuited by the cache, so nothing new to log.
+    expect(await prepareMacosTccLoginShell()).toBeNull()
+  })
+
   it('rejects a marker emitted by a preflight that does not exit cleanly', async () => {
     setPlatform('darwin')
     execFileMock.mockImplementation(

src/main/providers/macos-tcc-login-shell.ts

@@ -1,4 +1,4 @@
-import { execFile } from 'node:child_process'
+import { execFile, type ExecFileException } from 'node:child_process'
 import { existsSync } from 'node:fs'
 import { userInfo } from 'node:os'
 
@@ -16,15 +16,44 @@ const LOGIN_PREFLIGHT_MAX_BUFFER_BYTES = 1024
  */
 const DISABLE_ENV_VAR = 'ORCA_DISABLE_MACOS_LOGIN_SHELL'
 
+/**
+ * Result of one PAM probe. `conclusive` marks a real PAM verdict (accept or
+ * reject) that may be cached; an inconclusive probe (our own timeout/SIGKILL,
+ * maxBuffer, or spawn error) proves nothing about PAM and must not stick.
+ */
+export type LoginPreflightOutcome = {
+  ok: boolean
+  conclusive: boolean
+  reason: 'accepted' | 'rejected' | 'timeout' | 'error'
+}
+
 let cachedLoginPreflightResult: boolean | null = null
-let loginPreflightInFlight: Promise<boolean> | null = null
+let loginPreflightInFlight: Promise<LoginPreflightOutcome> | null = null
 
 function isDisabledByEnv(): boolean {
   const value = process.env[DISABLE_ENV_VAR]
   return value === '1' || value === 'true'
 }
 
-function runLoginPreflight(username: string, accountHome: string): Promise<boolean> {
+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).
+  if (error.killed || error.code === 'ETIMEDOUT') {
+    return { ok: false, conclusive: false, reason: 'timeout' }
+  }
+  // A numeric exit code means login(1) ran to completion and rejected the user
+  // (it exits immediately on EOF-driven rejection); that verdict is cacheable.
+  if (typeof error.code === 'number') {
+    return { ok: false, conclusive: true, reason: 'rejected' }
+  }
+  // Spawn/EOF/other failure: inconclusive, fail open for this spawn but retry.
+  return { ok: false, conclusive: false, reason: 'error' }
+}
+
+// Fidelity limit: the probe runs over pipes while production shells run under a
+// real PTY, so a tty-sensitive PAM stack could diverge. It fails safe — a probe
+// pass with a prod failure only degrades to today's direct spawn (no wrapper).
+function runLoginPreflight(username: string, accountHome: string): Promise<LoginPreflightOutcome> {
   return new Promise((resolve) => {
     try {
       const child = execFile(
@@ -42,33 +71,61 @@ function runLoginPreflight(username: string, accountHome: string): Promise<boole
           timeout: LOGIN_PREFLIGHT_TIMEOUT_MS
         },
         (error, stdout) => {
-          // login(1) can return zero after an EOF-driven failed prompt, so only the
-          // requested child program's output plus a clean exit proves PAM accepted it.
-          resolve(error === null && stdout === LOGIN_PREFLIGHT_MARKER)
+          if (error === null) {
+            // login(1) can return zero after an EOF-driven failed prompt, so only the
+            // requested child program's output plus a clean exit proves PAM accepted it.
+            resolve(
+              stdout === LOGIN_PREFLIGHT_MARKER
+                ? { ok: true, conclusive: true, reason: 'accepted' }
+                : { ok: false, conclusive: true, reason: 'rejected' }
+            )
+            return
+          }
+          resolve(classifyPreflightError(error))
         }
       )
       // Why: login(1) must see immediate EOF, not an interactive pipe, so a PAM
       // rejection exits instead of waiting at `login:` until the timeout.
       child.stdin?.end()
     } catch {
-      resolve(false)
+      resolve({ ok: false, conclusive: false, reason: 'error' })
     }
   })
 }
 
-function loginPreflightSucceeds(username: string, accountHome: string): Promise<boolean> {
-  if (cachedLoginPreflightResult !== null) {
-    return Promise.resolve(cachedLoginPreflightResult)
+function cachedOutcome(): LoginPreflightOutcome | null {
+  if (cachedLoginPreflightResult === null) {
+    return null
+  }
+  return cachedLoginPreflightResult
+    ? { ok: true, conclusive: true, reason: 'accepted' }
+    : { ok: false, conclusive: true, reason: 'rejected' }
+}
+
+function loginPreflightSucceeds(
+  username: string,
+  accountHome: string
+): Promise<LoginPreflightOutcome> {
+  const cached = cachedOutcome()
+  if (cached) {
+    return Promise.resolve(cached)
   }
   if (!loginPreflightInFlight) {
     // Why: simultaneous pane restores share one PAM child instead of multiplying
     // subprocesses at exactly the point terminal startup is already busiest.
-    loginPreflightInFlight = runLoginPreflight(username, accountHome).then((result) => {
-      cachedLoginPreflightResult = result
-      if (!result) {
+    loginPreflightInFlight = runLoginPreflight(username, accountHome).then((outcome) => {
+      // Why: cache only a conclusive PAM verdict; a killed/timed-out probe is
+      // environmental and must be retried next spawn, not stuck forever (F1).
+      if (outcome.conclusive) {
+        cachedLoginPreflightResult = outcome.ok
+      }
+      if (!outcome.ok) {
         console.warn('[pty] macOS login(1) preflight failed; spawning shells directly')
       }
-      return result
+      // Why: release the in-flight slot so an inconclusive probe can re-run on the
+      // next spawn instead of pinning every terminal to the degraded outcome.
+      loginPreflightInFlight = null
+      return outcome
     })
   }
   return loginPreflightInFlight
@@ -78,16 +135,21 @@ function loginPreflightSucceeds(username: string, accountHome: string): Promise<
  * Resolves the one-time PAM capability check before a fresh PTY is spawned.
  * Callers await this at their async request boundary so existing terminals and
  * the Electron main thread remain responsive while login(1) runs.
+ *
+ * Returns the probe outcome when a probe actually ran this call, or `null` when
+ * short-circuited (non-macOS, disabled, already cached, no login binary). The
+ * daemon uses the return to emit a structured degrade record, since detached
+ * daemons destroy stderr and never surface the console.warn above (F2).
  */
-export async function prepareMacosTccLoginShell(): Promise<void> {
+export async function prepareMacosTccLoginShell(): Promise<LoginPreflightOutcome | null> {
   if (process.platform !== 'darwin' || isDisabledByEnv()) {
-    return
+    return null
   }
   if (cachedLoginPreflightResult !== null) {
-    return
+    return null
   }
   if (!existsSync(MACOS_LOGIN_PATH)) {
-    return
+    return null
   }
 
   let username: string
@@ -97,12 +159,12 @@ export async function prepareMacosTccLoginShell(): Promise<void> {
     username = account.username
     accountHome = account.homedir
   } catch {
-    return
+    return null
   }
   if (!username || !accountHome) {
-    return
+    return null
   }
-  await loginPreflightSucceeds(username, accountHome)
+  return loginPreflightSucceeds(username, accountHome)
 }
 
 export function resetMacosLoginShellPreflightForTests(): void {