diff --git a/src/main/daemon/daemon-entry.ts b/src/main/daemon/daemon-entry.ts index 12c5ad8dd29..6488ffce048 100644 --- a/src/main/daemon/daemon-entry.ts +++ b/src/main/daemon/daemon-entry.ts @@ -82,6 +82,18 @@ async function main(): Promise { 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 => { + 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 { ...(launchNonce ? { launchNonce } : {}), ...(pidPath ? { startedAtMs } : {}), log: daemonLog, - preparePtySpawn: prepareMacosTccLoginShell, + preparePtySpawn: runMacosLoginPreflight, spawnSubprocess: (opts) => createPtySubprocess(opts), onIdleShutdown: () => { shuttingDown = true diff --git a/src/main/daemon/daemon-server.test.ts b/src/main/daemon/daemon-server.test.ts index aedf2384e08..9e0f1155f05 100644 --- a/src/main/daemon/daemon-server.test.ts +++ b/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((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() diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index 0226977a1ec..2cf8c32446c 100644 --- a/src/main/daemon/daemon-server.ts +++ b/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 { - const preparation: PendingPtySpawnPreparation = { canceled: false } + private async preparePtySpawnUnlessCanceled(sessionId: string, clientId: string): Promise { + 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 { const client = this.clients.get(clientId) @@ -662,7 +678,7 @@ export class DaemonServer { const p = request.payload let result: Awaited> try { - await this.preparePtySpawnUnlessCanceled(p.sessionId) + await this.preparePtySpawnUnlessCanceled(p.sessionId, clientId) result = await this.host.createOrAttach({ sessionId: p.sessionId, cols: p.cols, diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index 3eaa8c557d6..ca6661d6ae8 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/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((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( diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index b1907a2bdfb..39f46f2e47e 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/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) diff --git a/src/main/providers/macos-tcc-login-shell.test.ts b/src/main/providers/macos-tcc-login-shell.test.ts index ee43bf922f5..7ea47834eaf 100644 --- a/src/main/providers/macos-tcc-login-shell.test.ts +++ b/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( diff --git a/src/main/providers/macos-tcc-login-shell.ts b/src/main/providers/macos-tcc-login-shell.ts index ad8bbfa960d..177cb335352 100644 --- a/src/main/providers/macos-tcc-login-shell.ts +++ b/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 | null = null +let loginPreflightInFlight: Promise | null = null function isDisabledByEnv(): boolean { const value = process.env[DISABLE_ENV_VAR] return value === '1' || value === 'true' } -function runLoginPreflight(username: string, accountHome: string): Promise { +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 { return new Promise((resolve) => { try { const child = execFile( @@ -42,33 +71,61 @@ function runLoginPreflight(username: string, accountHome: string): Promise { - // 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 { - 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 { + 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 { +export async function prepareMacosTccLoginShell(): Promise { 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 { 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 {