From 54e9bad9f2b2deef1641afae43efb6b051ade868 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:41:44 -0700 Subject: [PATCH] fix(daemon): close the hang-shaped gap in login-session death detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The conclusive-PAM-verdict trigger had one blind failure shape: login(1) hanging at the prompt past the probe bound (killed → inconclusive forever → the watch never fires). Three changes close it: - The death-watch probe gets its own 4s bound (the 500ms preflight bound exists for spawn-path latency, which doesn't apply off-path), so a slow-but-answering PAM stack isn't misread as a hang. - An inconclusive pipe probe escalates to a PTY-hosted probe via script(1) — a dead session's PAM stack may only misbehave under a real tty (the pipe-vs-PTY fidelity limit the preflight documents). - A streak of timeout-killed probes (which a live session never produces) is a second retirement trigger, at a higher threshold (5) and still gated on the degraded resolver, logged with a distinct cause so field logs discriminate the two paths. Every dead-session behavior — fast reject, prompt-then-EOF, or hang — now fires retirement; all inconclusive states still fail toward preserving the daemon. --- src/main/daemon/daemon-entry.ts | 8 +- .../macos-login-session-death-watch.test.ts | 94 ++++++++++++++++++- .../daemon/macos-login-session-death-watch.ts | 74 +++++++++++---- .../providers/macos-tcc-login-shell.test.ts | 52 ++++++++++ src/main/providers/macos-tcc-login-shell.ts | 69 +++++++++++++- 5 files changed, 268 insertions(+), 29 deletions(-) diff --git a/src/main/daemon/daemon-entry.ts b/src/main/daemon/daemon-entry.ts index 907f40ede42..3cc24093a06 100644 --- a/src/main/daemon/daemon-entry.ts +++ b/src/main/daemon/daemon-entry.ts @@ -171,7 +171,8 @@ async function main(): Promise { // Why: a dead macOS login session cannot be fabricated without root (PAM owns // audit-session teardown), so e2e drives the oracles from a verdict file: - // 'alive' → accepted/healthy, 'dead' → rejected/unhealthy, else inconclusive. + // 'alive' → accepted/healthy, 'dead' → rejected/unhealthy, 'hang' → + // timeout-inconclusive/unhealthy (the hang-shaped trigger), else inconclusive. const e2eProbeFile = process.env.ORCA_E2E_LOGIN_SESSION_PROBE_FILE const readE2eVerdict = (): string => { try { @@ -196,7 +197,10 @@ async function main(): Promise { } : probeMacosLoginSessionAlive, readResolverHealth: e2eProbeFile - ? async () => (readE2eVerdict() === 'dead' ? 'unhealthy' : 'healthy') + ? async () => { + const verdict = readE2eVerdict() + return verdict === 'dead' || verdict === 'hang' ? 'unhealthy' : 'healthy' + } : readCurrentProcessMacSystemResolverHealth, ...(e2eProbeFile ? { diff --git a/src/main/daemon/macos-login-session-death-watch.test.ts b/src/main/daemon/macos-login-session-death-watch.test.ts index 38205f6abfd..410ce2ccce9 100644 --- a/src/main/daemon/macos-login-session-death-watch.test.ts +++ b/src/main/daemon/macos-login-session-death-watch.test.ts @@ -114,7 +114,11 @@ describe('MacosLoginSessionDeathWatch', () => { await clock.advance(10_000) // recheck → rejection 2 expect(onRetire).not.toHaveBeenCalled() await clock.advance(10_000) // recheck → rejection 3 → retire - expect(onRetire).toHaveBeenCalledWith({ rejections: 3, resolverHealth: 'unhealthy' }) + expect(onRetire).toHaveBeenCalledWith({ + cause: 'pam-rejections', + rejections: 3, + resolverHealth: 'unhealthy' + }) }) it('never retires when the session was never conclusively accepted', async () => { @@ -143,19 +147,99 @@ describe('MacosLoginSessionDeathWatch', () => { expect(onRetire).not.toHaveBeenCalled() }) - it('ignores inconclusive probes for the rejection streak', async () => { + it('keeps the rejection streak across interleaved inconclusive probes', async () => { const { watch, clock, onRetire } = createWatch({ outcomes: [ACCEPTED, REJECTED, INCONCLUSIVE, REJECTED, INCONCLUSIVE, REJECTED] }) watch.start() await drainMicrotasks() await clock.advance(120_000) // rejection 1 - await clock.advance(10_000) // inconclusive — streak holds at 1, periodic reschedule - await clock.advance(120_000) // rejection 2 + await clock.advance(10_000) // inconclusive timeout — streak holds, fast recheck + await clock.advance(10_000) // rejection 2 await clock.advance(10_000) // inconclusive - await clock.advance(120_000) // rejection 3 → retire + await clock.advance(10_000) // rejection 3 → retire expect(onRetire).toHaveBeenCalledTimes(1) expect(onRetire.mock.calls[0][0].rejections).toBe(3) + expect(onRetire.mock.calls[0][0].cause).toBe('pam-rejections') + }) + + it('retires on a hang-shaped timeout streak once armed, with a degraded resolver', async () => { + const { watch, clock, onRetire } = createWatch({ + outcomes: [ACCEPTED, INCONCLUSIVE, INCONCLUSIVE, INCONCLUSIVE, INCONCLUSIVE, INCONCLUSIVE] + }) + watch.start() + await drainMicrotasks() + await clock.advance(120_000) // timeout 1 → fast recheck + await clock.advance(10_000) // timeout 2 + await clock.advance(10_000) // timeout 3 + await clock.advance(10_000) // timeout 4 + expect(onRetire).not.toHaveBeenCalled() + await clock.advance(10_000) // timeout 5 → retire + expect(onRetire).toHaveBeenCalledWith({ + cause: 'probe-timeouts', + rejections: 0, + resolverHealth: 'unhealthy' + }) + }) + + it('never counts timeouts toward retirement while unarmed', async () => { + const { watch, clock, onRetire, probe } = createWatch({ + outcomes: Array.from({ length: 8 }, () => INCONCLUSIVE) + }) + watch.start() + await drainMicrotasks() + for (let i = 0; i < 7; i++) { + await clock.advance(120_000) + } + expect(probe.mock.calls.length).toBeGreaterThanOrEqual(7) + expect(onRetire).not.toHaveBeenCalled() + }) + + it('resets the timeout streak on any conclusive verdict', async () => { + const { watch, clock, onRetire } = createWatch({ + outcomes: [ + ACCEPTED, + INCONCLUSIVE, + INCONCLUSIVE, + INCONCLUSIVE, + INCONCLUSIVE, + ACCEPTED, + INCONCLUSIVE, + INCONCLUSIVE, + INCONCLUSIVE, + INCONCLUSIVE + ] + }) + watch.start() + await drainMicrotasks() + await clock.advance(120_000) // timeout 1 + for (let i = 0; i < 3; i++) { + await clock.advance(10_000) // timeouts 2-4 + } + await clock.advance(10_000) // acceptance → both streaks reset + await clock.advance(120_000) // timeout 1 again + for (let i = 0; i < 3; i++) { + await clock.advance(10_000) // timeouts 2-4 + } + expect(onRetire).not.toHaveBeenCalled() + }) + + it('suppresses timeout-streak retirement while the resolver is healthy', async () => { + const { watch, clock, onRetire, setResolverHealth } = createWatch({ + outcomes: [ACCEPTED, ...Array.from({ length: 8 }, () => INCONCLUSIVE)] + }) + setResolverHealth('healthy') + watch.start() + await drainMicrotasks() + await clock.advance(120_000) // timeout 1 + for (let i = 0; i < 4; i++) { + await clock.advance(10_000) // timeouts 2-5 → threshold, suppressed + } + expect(onRetire).not.toHaveBeenCalled() + setResolverHealth('unhealthy') + await clock.advance(10_000) // held at threshold → next timeout re-verdicts → retire + expect(onRetire).toHaveBeenCalledTimes(1) + expect(onRetire.mock.calls[0][0].cause).toBe('probe-timeouts') }) it('suppresses retirement while the system resolver is healthy, then retires when it degrades', async () => { diff --git a/src/main/daemon/macos-login-session-death-watch.ts b/src/main/daemon/macos-login-session-death-watch.ts index cd97e809048..e49e5b9091d 100644 --- a/src/main/daemon/macos-login-session-death-watch.ts +++ b/src/main/daemon/macos-login-session-death-watch.ts @@ -6,6 +6,10 @@ import type { SystemResolverHealth } from './types' // rejection storm (PAM db reload, OS update) from retiring a daemon whose login // session is still alive; a real logout rejects conclusively on every probe. const REQUIRED_CONSECUTIVE_REJECTIONS = 3 +// Why: a healthy session answers the PAM probe in milliseconds; probes that die by +// timeout repeatedly are a hang-shaped death signature (PAM blocking on a torn-down +// session). Higher threshold than rejections because load can also slow probes. +const REQUIRED_CONSECUTIVE_TIMEOUTS = 5 const PERIODIC_PROBE_MS = 120_000 const REJECTION_RECHECK_MS = 10_000 const PTY_EXIT_DEBOUNCE_MS = 2_000 @@ -24,7 +28,11 @@ export type MacosLoginSessionDeathWatchOptions = { /** Fresh PAM probe (cache-bypassing); null when the login wrapper doesn't apply on this host. */ probeLoginSession: () => Promise readResolverHealth: () => Promise - onRetire: (details: { rejections: number; resolverHealth: SystemResolverHealth }) => void + onRetire: (details: { + cause: 'pam-rejections' | 'probe-timeouts' + rejections: number + resolverHealth: SystemResolverHealth + }) => void log: DaemonFileLog /** Direct-construction seam for deterministic tests; production uses real timers. */ clock?: WatchClock @@ -49,9 +57,11 @@ export type MacosLoginSessionDeathWatchOptions = { * * The oracle is the existing TCC login-shell PAM probe: it conclusively accepts * while the session is valid — including fast-user-switched-away sessions — and - * conclusively rejects once the session is destroyed. Retirement additionally - * requires the in-process system resolver to be degraded, so a PAM anomaly - * alone can never kill a healthy daemon. + * conclusively rejects once the session is destroyed. A hang-shaped death + * (probes repeatedly dying by timeout, which a live session never does) is a + * second, higher-threshold trigger. Retirement additionally requires the + * in-process system resolver to be degraded, so a PAM anomaly alone can never + * kill a healthy daemon. */ export class MacosLoginSessionDeathWatch { private readonly probeLoginSession: MacosLoginSessionDeathWatchOptions['probeLoginSession'] @@ -69,6 +79,7 @@ export class MacosLoginSessionDeathWatch { // a host where the wrapper never worked has no death signal to trust. private armed = false private consecutiveRejections = 0 + private consecutiveTimeouts = 0 private lastProbeStartedAtMs: number | null = null private probeInFlight = false private stopped = false @@ -178,9 +189,28 @@ export class MacosLoginSessionDeathWatch { return } if (!outcome.conclusive) { + if (this.armed && outcome.reason === 'timeout') { + // Why: healthy sessions answer in milliseconds even under load; a streak of + // SIGKILL-by-bound probes is PAM blocking on a torn-down session (the one + // failure shape a conclusive-verdict trigger would miss). + this.consecutiveTimeouts++ + this.log.log('login-session-probe-timeout', { + trigger, + timeouts: this.consecutiveTimeouts + }) + if (this.consecutiveTimeouts >= REQUIRED_CONSECUTIVE_TIMEOUTS) { + await this.retireIfResolverDegraded('probe-timeouts', () => { + this.consecutiveTimeouts = REQUIRED_CONSECUTIVE_TIMEOUTS - 1 + }) + return + } + this.scheduleNextProbe(this.rejectionRecheckMs) + return + } this.scheduleNextProbe(this.periodicProbeMs) return } + this.consecutiveTimeouts = 0 if (outcome.ok) { if (!this.armed) { this.log.log('login-session-watch-armed', { trigger }) @@ -204,21 +234,9 @@ export class MacosLoginSessionDeathWatch { this.scheduleNextProbe(this.rejectionRecheckMs) return } - const resolverHealth = await this.readResolverHealth() - if (this.stopped || this.retired) { - return - } - if (resolverHealth === 'healthy') { - // Why: a live session always has DNS configuration; PAM rejecting while the - // resolver works means the anomaly is PAM-side, so hold at the threshold and - // keep re-verdicting instead of killing live terminals. - this.log.log('login-session-retire-suppressed', { resolverHealth }) + await this.retireIfResolverDegraded('pam-rejections', () => { this.consecutiveRejections = REQUIRED_CONSECUTIVE_REJECTIONS - 1 - this.scheduleNextProbe(this.rejectionRecheckMs) - return - } - this.retired = true - this.onRetire({ rejections: this.consecutiveRejections, resolverHealth }) + }) } catch (error) { // Why: a probe failure is diagnostic only; an escaped rejection would trip the // daemon's fatal unhandled-error path and kill live terminals. @@ -228,4 +246,24 @@ export class MacosLoginSessionDeathWatch { this.probeInFlight = false } } + + private async retireIfResolverDegraded( + cause: 'pam-rejections' | 'probe-timeouts', + holdAtThreshold: () => void + ): Promise { + const resolverHealth = await this.readResolverHealth() + if (this.stopped || this.retired) { + return + } + if (resolverHealth === 'healthy') { + // Why: a live session always has DNS configuration; the PAM anomaly alone must + // hold at the threshold and keep re-verdicting instead of killing live terminals. + this.log.log('login-session-retire-suppressed', { cause, resolverHealth }) + holdAtThreshold() + this.scheduleNextProbe(this.rejectionRecheckMs) + return + } + this.retired = true + this.onRetire({ cause, rejections: this.consecutiveRejections, resolverHealth }) + } } diff --git a/src/main/providers/macos-tcc-login-shell.test.ts b/src/main/providers/macos-tcc-login-shell.test.ts index 2f8b0eb5bb2..09c81413fbc 100644 --- a/src/main/providers/macos-tcc-login-shell.test.ts +++ b/src/main/providers/macos-tcc-login-shell.test.ts @@ -426,4 +426,56 @@ describe('probeMacosLoginSessionAlive', () => { expect(await probeMacosLoginSessionAlive()).toBeNull() expect(execFileMock).not.toHaveBeenCalled() }) + + it('escalates an inconclusive pipe probe to a PTY probe and accepts its verdict', async () => { + setPlatform('darwin') + execFileMock.mockImplementation( + (file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + if (file === '/usr/bin/script') { + // PTYs echo control sequences around the marker. + callback(null, '[?1049h\r\nORCA_LOGIN_PREFLIGHT_OK\r\n', '') + } else { + callback(Object.assign(new Error('killed'), { killed: true }), '', '') + } + return { stdin: { end: stdinEndMock } } + } + ) + const outcome = await probeMacosLoginSessionAlive() + expect(outcome).toEqual({ ok: true, conclusive: true, reason: 'accepted' }) + expect(execFileMock.mock.calls.map((c) => c[0])).toEqual(['/usr/bin/login', '/usr/bin/script']) + expect(wrapShellSpawnForMacosTccAttribution('/bin/zsh', ['-l']).file).toBe('/usr/bin/login') + }) + + it('treats a PTY-probe rejection as conclusive and flips the wrapper off', async () => { + setPlatform('darwin') + await prepareMacosTccLoginShell() + execFileMock.mockImplementation( + (file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + if (file === '/usr/bin/script') { + callback(Object.assign(new Error('login incorrect'), { code: 1 }), '', '') + } else { + callback(Object.assign(new Error('killed'), { killed: true }), '', '') + } + return { stdin: { end: stdinEndMock } } + } + ) + const outcome = await probeMacosLoginSessionAlive() + expect(outcome).toEqual({ ok: false, conclusive: true, reason: 'rejected' }) + expect(wrapShellSpawnForMacosTccAttribution('/bin/zsh', ['-l']).file).toBe('/bin/zsh') + }) + + it('stays inconclusive when both pipe and PTY probes time out', async () => { + setPlatform('darwin') + await prepareMacosTccLoginShell() + execFileMock.mockImplementation( + (_file: string, _args: string[], _options: unknown, callback: ExecFileCallback) => { + callback(Object.assign(new Error('killed'), { killed: true }), '', '') + return { stdin: { end: stdinEndMock } } + } + ) + const outcome = await probeMacosLoginSessionAlive() + expect(outcome).toEqual({ ok: false, conclusive: false, reason: 'timeout' }) + // Inconclusive must not disturb the cached acceptance. + 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 2e5c607d567..c01e4a74664 100644 --- a/src/main/providers/macos-tcc-login-shell.ts +++ b/src/main/providers/macos-tcc-login-shell.ts @@ -5,7 +5,11 @@ import { userInfo } from 'node:os' const MACOS_LOGIN_PATH = '/usr/bin/login' const MACOS_ENV_PATH = '/usr/bin/env' const MACOS_PRINTF_PATH = '/usr/bin/printf' +const MACOS_SCRIPT_PATH = '/usr/bin/script' const LOGIN_PREFLIGHT_TIMEOUT_MS = 500 +// Why: the death-watch probe runs off the spawn path, so it can afford a bound +// that outlasts a PAM stack answering slowly rather than misreading it as a hang. +const LOGIN_SESSION_WATCH_PROBE_TIMEOUT_MS = 4_000 const LOGIN_PREFLIGHT_MARKER = 'ORCA_LOGIN_PREFLIGHT_OK' const LOGIN_PREFLIGHT_MAX_BUFFER_BYTES = 1024 const LOGIN_PREFLIGHT_RETRY_BASE_MS = 5_000 @@ -63,7 +67,11 @@ function classifyPreflightError(error: ExecFileException): LoginPreflightOutcome // 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 { +function runLoginPreflight( + username: string, + accountHome: string, + timeoutMs = LOGIN_PREFLIGHT_TIMEOUT_MS +): Promise { return new Promise((resolve) => { try { const child = execFile( @@ -78,7 +86,7 @@ function runLoginPreflight(username: string, accountHome: string): Promise { if (error === null) { @@ -103,6 +111,54 @@ function runLoginPreflight(username: string, accountHome: string): Promise { + return new Promise((resolve) => { + try { + const child = execFile( + MACOS_SCRIPT_PATH, + [ + '-q', + '/dev/null', + MACOS_LOGIN_PATH, + '-flpq', + username, + MACOS_PRINTF_PATH, + LOGIN_PREFLIGHT_MARKER + ], + { + cwd: accountHome, + encoding: 'utf8', + killSignal: 'SIGKILL', + maxBuffer: LOGIN_PREFLIGHT_MAX_BUFFER_BYTES, + timeout: timeoutMs + }, + (error, stdout) => { + if (error === null) { + // Why includes(): the PTY echoes control sequences around the marker. + resolve( + stdout.includes(LOGIN_PREFLIGHT_MARKER) + ? { ok: true, conclusive: true, reason: 'accepted' } + : { ok: false, conclusive: true, reason: 'rejected' } + ) + return + } + resolve(classifyPreflightError(error)) + } + ) + child.stdin?.end() + } catch { + resolve({ ok: false, conclusive: false, reason: 'error' }) + } + }) +} + function cachedOutcome(): LoginPreflightOutcome | null { if (cachedLoginPreflightResult === null) { return null @@ -199,7 +255,9 @@ export function resetMacosLoginShellPreflightForTests(): void { * cached verdict and the transient backoff, and writes any conclusive verdict * back into the cache — so a daemon whose login session died stops wrapping * spawns in `login(1)` (which would only mint "Login incorrect" zombies) even - * before retirement completes. Returns null when the wrapper doesn't apply. + * before retirement completes. Escalates to a PTY-hosted probe when the pipe + * probe is inconclusive, since a dead session's PAM stack may only misbehave + * under a real tty. Returns null when the wrapper doesn't apply. */ export async function probeMacosLoginSessionAlive(): Promise { if (process.platform !== 'darwin' || isDisabledByEnv() || !existsSync(MACOS_LOGIN_PATH)) { @@ -217,7 +275,10 @@ export async function probeMacosLoginSessionAlive(): Promise