diff --git a/src/main/daemon/daemon-health.test.ts b/src/main/daemon/daemon-health.test.ts index b6833d8bcc7..8c220db0d19 100644 --- a/src/main/daemon/daemon-health.test.ts +++ b/src/main/daemon/daemon-health.test.ts @@ -22,16 +22,8 @@ import { startTimesWithinTolerance } from './daemon-health' import type { SubprocessHandle } from './session' -import type { DaemonPtyOwnership } from './daemon-live-pty-evidence' // Why: the veto's production default is otherwise never exercised — every other test injects it. -const { inspectDaemonPtyOwnershipMock } = vi.hoisted(() => ({ - inspectDaemonPtyOwnershipMock: vi.fn(async (_pid: number) => 'unknown' as DaemonPtyOwnership) -})) - -vi.mock('./daemon-live-pty-evidence', () => ({ - inspectDaemonPtyOwnership: inspectDaemonPtyOwnershipMock -})) function createMockSubprocess(): SubprocessHandle { return { @@ -597,151 +589,3 @@ describe('killStaleDaemon ownership decisions', () => { } ) }) - -describe.skipIf(process.platform === 'win32')('killStaleDaemon live PTY ownership veto', () => { - let dir: string - let socketPath: string - let tokenPath: string - let stubPid: number - let stubExited: Promise - - beforeEach(async () => { - dir = mkdtempSync(join(tmpdir(), 'daemon-health-pty-veto-test-')) - socketPath = daemonTestSocketPath(dir) - tokenPath = join(dir, 'daemon.token') - // A real process is required: identity 'match' is decided from the live command line. - const child = spawn( - process.execPath, - [ - '-e', - "console.log('ready'); setInterval(() => {}, 1000)", - 'daemon-entry', - socketPath, - tokenPath - ], - { stdio: ['ignore', 'pipe', 'ignore'] } - ) - await new Promise((resolve, reject) => { - child.once('error', reject) - child.stdout?.once('data', () => resolve()) - }) - stubPid = child.pid as number - stubExited = new Promise((resolve) => child.once('exit', () => resolve())) - writeFileSync( - getDaemonPidPath(dir), - serializeDaemonPidFile({ pid: stubPid, startedAtMs: null, launchNonce: 'daemon-a' }), - { mode: 0o600 } - ) - }) - - afterEach(async () => { - try { - process.kill(stubPid, 'SIGKILL') - } catch { - // Already gone. - } - await stubExited - rmSync(dir, { recursive: true, force: true }) - }) - - function terminationSignals(killSpy: { mock: { calls: unknown[][] } }): unknown[][] { - return killSpy.mock.calls.filter(([, sig]) => sig === 'SIGTERM' || sig === 'SIGKILL') - } - - it('sends no signal at all when the daemon still owns live PTYs', async () => { - // Why: a wedged daemon cannot list its own sessions, so descendants are the - // only evidence left that replacing it would destroy running agents. - const inspectPtyOwnership = vi.fn(async () => 'owns-live-ptys' as const) - const killSpy = vi.spyOn(process, 'kill') - - try { - await expect( - killStaleDaemon(dir, socketPath, tokenPath, undefined, { - probeEndpoint: async () => 'missing', - inspectPtyOwnership, - preserveWhenOwningLivePtys: true - }) - ).resolves.toEqual({ killed: false, liveOwnerSurvived: true }) - expect(inspectPtyOwnership).toHaveBeenCalledWith(stubPid) - expect(terminationSignals(killSpy)).toEqual([]) - } finally { - killSpy.mockRestore() - } - expect(() => process.kill(stubPid, 0)).not.toThrow() - expect(readFileSync(getDaemonPidPath(dir), 'utf8')).toContain('daemon-a') - }) - - it('still SIGTERMs a wedged daemon that owns no live PTYs', async () => { - // #8689: nothing to lose, so an unresponsive daemon must stay replaceable. - const killSpy = vi.spyOn(process, 'kill') - - try { - await expect( - killStaleDaemon(dir, socketPath, tokenPath, undefined, { - probeEndpoint: async () => 'missing', - inspectPtyOwnership: async () => 'no-live-ptys', - preserveWhenOwningLivePtys: true - }) - ).resolves.toEqual({ killed: true, liveOwnerSurvived: false }) - expect(killSpy).toHaveBeenCalledWith(stubPid, 'SIGTERM') - } finally { - killSpy.mockRestore() - } - }) - - it('still SIGTERMs when PTY ownership cannot be determined', async () => { - // Residual by design: only positive evidence of live PTYs vetoes the kill. - const killSpy = vi.spyOn(process, 'kill') - - try { - await expect( - killStaleDaemon(dir, socketPath, tokenPath, undefined, { - probeEndpoint: async () => 'missing', - inspectPtyOwnership: async () => 'unknown', - preserveWhenOwningLivePtys: true - }) - ).resolves.toEqual({ killed: true, liveOwnerSurvived: false }) - expect(killSpy).toHaveBeenCalledWith(stubPid, 'SIGTERM') - } finally { - killSpy.mockRestore() - } - }) - - it('routes the opt-in through the real ownership inspector when no hook is injected', async () => { - // Why: production passes no hook, so a default that never sees live PTYs would ship a - // veto that silently never fires — indistinguishable from having no veto at all. - inspectDaemonPtyOwnershipMock.mockResolvedValueOnce('owns-live-ptys') - const killSpy = vi.spyOn(process, 'kill') - - try { - await expect( - killStaleDaemon(dir, socketPath, tokenPath, undefined, { - probeEndpoint: async () => 'missing', - preserveWhenOwningLivePtys: true - }) - ).resolves.toEqual({ killed: false, liveOwnerSurvived: true }) - expect(inspectDaemonPtyOwnershipMock).toHaveBeenCalledWith(stubPid) - expect(terminationSignals(killSpy)).toEqual([]) - } finally { - killSpy.mockRestore() - } - }) - - it('never consults PTY ownership without the opt-in, so restarts stay killable', async () => { - const inspectPtyOwnership = vi.fn(async () => 'owns-live-ptys' as const) - const killSpy = vi.spyOn(process, 'kill') - - try { - await expect( - killStaleDaemon(dir, socketPath, tokenPath, undefined, { - probeEndpoint: async () => 'missing', - inspectPtyOwnership - }) - ).resolves.toEqual({ killed: true, liveOwnerSurvived: false }) - expect(inspectPtyOwnership).not.toHaveBeenCalled() - expect(killSpy).toHaveBeenCalledWith(stubPid, 'SIGTERM') - } finally { - killSpy.mockRestore() - } - }) -}) diff --git a/src/main/daemon/daemon-health.ts b/src/main/daemon/daemon-health.ts index 2a3f3c90295..9a1fa33ccff 100644 --- a/src/main/daemon/daemon-health.ts +++ b/src/main/daemon/daemon-health.ts @@ -10,7 +10,6 @@ import { } from '../../shared/process-output-field-scanner' import { isStartupDiagnosticsEnabled, logStartupDiagnostic } from '../startup/startup-diagnostics' import { encodeNdjson } from './ndjson' -import { inspectDaemonPtyOwnership, type DaemonPtyOwnership } from './daemon-live-pty-evidence' import { getDaemonPidPath, unlinkDaemonPidFileWhen, @@ -613,7 +612,7 @@ export async function getDaemonLaunchIdentity( return commandLine.includes(expectedEntryPath) ? 'match' : 'mismatch' } -async function readVerifiedDaemonPid( +export async function readVerifiedDaemonPid( runtimeDir: string, socketPath: string, tokenPath: string, @@ -732,18 +731,6 @@ async function waitForProcessExit(pid: number, timeoutMs: number): Promise Promise - inspectPtyOwnership?: (daemonPid: number) => Promise -} - -export type StaleDaemonKillOptions = { - /** - * Refuse to signal a daemon whose process still owns live PTY children. - * - * Only for callers replacing on *unverifiable* state. Callers that positively - * confirmed zero live sessions — or that are carrying out an explicit user - * restart — must leave this off, or the daemon becomes unkillable. - */ - preserveWhenOwningLivePtys?: boolean } export type StaleDaemonKillOutcome = { @@ -762,12 +749,9 @@ export async function killStaleDaemon( socketPath: string, tokenPath: string, protocolVersion = PROTOCOL_VERSION, - // Why: one trailing bag rather than two optional params — a caller that skipped the - // hooks slot silently landed its options in it, disabling the veto with no type error. - options?: StaleDaemonKillTestHooks & StaleDaemonKillOptions + testHooks?: StaleDaemonKillTestHooks ): Promise { - const probeEndpoint = options?.probeEndpoint ?? probeSocketConnect - const inspectPtyOwnership = options?.inspectPtyOwnership ?? inspectDaemonPtyOwnership + const probeEndpoint = testHooks?.probeEndpoint ?? probeSocketConnect const pidPath = getDaemonPidPath(runtimeDir, protocolVersion) let killedDaemon = false let liveOwnerSurvived = false @@ -802,18 +786,6 @@ export async function killStaleDaemon( } if (parsedPid && identity === 'match') { const { pid, startedAtMs } = parsedPid - // Why: identity is already 'match', so these descendants provably belong to this - // daemon incarnation — not to a recycled pid. A wedged daemon cannot list its own - // sessions, and treating that silence as "empty" is what killed live agents. - if (options?.preserveWhenOwningLivePtys) { - const ownership = await inspectPtyOwnership(pid) - if (ownership === 'owns-live-ptys') { - console.warn( - '[daemon] Preserving daemon that could not report its sessions: its process still owns live terminal processes' - ) - return { killed: false, liveOwnerSurvived: true } - } - } try { process.kill(pid, 'SIGTERM') } catch (error) { diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index 67ef30db7c1..78b48330dc6 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -28,6 +28,8 @@ const { getDaemonLaunchIdentityMock, isDaemonStaleForCurrentBundleMock, killStaleDaemonMock, + readVerifiedDaemonPidMock, + inspectDaemonPtyOwnershipMock, getProcessStartedAtMsMock, parseDaemonPidFileMock, replaceDaemonPidFileMock, @@ -91,6 +93,10 @@ const { const getMacDaemonTccAttributionHealthMock = vi.fn(async () => 'unknown') const getDaemonLaunchIdentityMock = vi.fn(() => 'match') const isDaemonStaleForCurrentBundleMock = vi.fn(() => false) + const inspectDaemonPtyOwnershipMock = vi.fn( + async (): Promise<'owns-live-ptys' | 'no-live-ptys' | 'unknown'> => 'unknown' + ) + const readVerifiedDaemonPidMock = vi.fn(async (): Promise<{ pid: number } | null> => null) const killStaleDaemonMock = vi.fn(async () => ({ killed: true, liveOwnerSurvived: false @@ -196,6 +202,8 @@ const { getDaemonLaunchIdentityMock, isDaemonStaleForCurrentBundleMock, killStaleDaemonMock, + readVerifiedDaemonPidMock, + inspectDaemonPtyOwnershipMock, getProcessStartedAtMsMock, parseDaemonPidFileMock, replaceDaemonPidFileMock, @@ -265,6 +273,17 @@ vi.mock('electron', () => ({ } })) +// Map the existing boolean socket double onto the canonical three-valued probe: +// present ⇒ something is serving, absent ⇒ positively dead. +vi.mock('./daemon-live-pty-evidence', () => ({ + inspectDaemonPtyOwnership: inspectDaemonPtyOwnershipMock +})) + +vi.mock('./daemon-endpoint-probe', async (importOriginal) => ({ + ...(await importOriginal>()), + probeSocketConnect: async (p: string) => (probeSocketExistsMock(p) ? 'connected' : 'missing') +})) + vi.mock('fs', () => ({ mkdirSync: vi.fn(), existsSync: (p: string) => probeSocketExistsMock(p) || p.includes('.pid'), @@ -289,6 +308,7 @@ vi.mock('./daemon-health', () => ({ healthCheckDaemon: healthCheckDaemonMock, isDaemonStaleForCurrentBundle: isDaemonStaleForCurrentBundleMock, killStaleDaemon: killStaleDaemonMock, + readVerifiedDaemonPid: readVerifiedDaemonPidMock, getProcessStartedAtMs: getProcessStartedAtMsMock, parseDaemonPidFile: parseDaemonPidFileMock })) @@ -3156,9 +3176,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(killStaleDaemonMock).toHaveBeenCalledWith( FAKE_RUNTIME_DIR, '/fake/socket', - '/fake/token', - PROTOCOL_VERSION, - { preserveWhenOwningLivePtys: true } + '/fake/token' ) expect(forkMock).toHaveBeenCalled() // The launcher probes the full grace budget: 1 initial probe + WEDGED_DAEMON_GRACE_RETRIES retries. @@ -3178,11 +3196,9 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { } }) - it('adopts a wedged daemon that still owns live PTYs instead of killing its agents', async () => { - // Why: the regression that motivated the veto. A daemon too wedged to answer listSessions - // still hosts every running agent; killing it destroys work that no restore can recover. - // killStaleDaemon owns the veto, so the launcher's contract is to opt in here and to route - // the refusal into degraded adoption rather than forking a second daemon beside it. + it('holds a wedged daemon that still owns live terminals instead of killing its agents', async () => { + // Why hold and not adopt: a daemon that cannot answer listSessions cannot answer a hello, + // so there is nothing to adopt. The kill must never be attempted — its agents are alive. const mod = await importFresh() await mod.initDaemonPtyProvider() @@ -3193,22 +3209,19 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { disconnect: vi.fn() } } - let daemonClientConstructionCount = 0 daemonClientMock.mockImplementation(function MockWedgedDaemonClient() { - daemonClientConstructionCount++ return { ensureConnected: vi.fn(async () => { - if (daemonClientConstructionCount <= 2 + WEDGED_DAEMON_GRACE_RETRIES) { - throw new Error('Hello response timed out') - } + throw new Error('Hello response timed out') }), getDaemonIdentity: vi.fn(readLaunchedDaemonIdentity), request: vi.fn(), disconnect: vi.fn() } }) - // The real killStaleDaemon refuses and reports the owner alive; this is that verdict. - killStaleDaemonMock.mockResolvedValueOnce({ killed: false, liveOwnerSurvived: true }) + // The daemon is identity-verified and its process still owns live terminals. + readVerifiedDaemonPidMock.mockResolvedValue({ pid: 4242 }) + inspectDaemonPtyOwnershipMock.mockResolvedValue('owns-live-ptys') const launcher = spawnerInstances[0].launcher as ( socketPath: string, @@ -3218,27 +3231,16 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { probeSocketExistsMock.mockReturnValue(true) netConnectMock.mockImplementation(stubAliveSocketConnect) - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) try { const handle = await launcher('/fake/socket', '/fake/token') - expect(killStaleDaemonMock).toHaveBeenCalledWith( - FAKE_RUNTIME_DIR, - '/fake/socket', - '/fake/token', - PROTOCOL_VERSION, - { preserveWhenOwningLivePtys: true } - ) - // No replacement daemon beside the one still hosting the agents. + expect(handle.mode).toBe('held') + expect(killStaleDaemonMock).not.toHaveBeenCalled() expect(forkMock).not.toHaveBeenCalled() - expect(handle.mode).toBe('degraded-new-pty-fallback') - // A refusal is not a replacement, so the replace verdict must not be announced. - expect(warnSpy).not.toHaveBeenCalledWith( - expect.stringContaining('Replacing daemon that failed the health check') - ) } finally { - warnSpy.mockRestore() daemonClientMock.mockImplementation(answeringDefault) + readVerifiedDaemonPidMock.mockResolvedValue(null) + inspectDaemonPtyOwnershipMock.mockResolvedValue('unknown') } }) diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index 2b7d885bd8a..da43c410930 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -15,9 +15,12 @@ import { unlinkOwnedDaemonPidFile, type DaemonLauncher, type DaemonPidFile, + type DaemonLaunchMode, type DaemonProcessHandle } from './daemon-spawner' import { DAEMON_EXIT_ENDPOINT_OCCUPIED } from './daemon-endpoint-ownership' +import { endpointIsProvenDead, probeSocketConnect } from './daemon-endpoint-probe' +import { resolveDaemonOccupancy } from './daemon-occupancy' import { DaemonPtyAdapter, type DaemonRespawnReason } from './daemon-pty-adapter' import { DaemonPtyRouter } from './daemon-pty-router' import { DaemonClient } from './client' @@ -34,6 +37,7 @@ import { checkDaemonHealth, isDaemonStaleForCurrentBundle, killStaleDaemon, + readVerifiedDaemonPid, parseDaemonPidFile, type MacDaemonTccAttributionHealth } from './daemon-health' @@ -193,7 +197,7 @@ async function getAliveDaemonSessionCount( function createPreservedDaemonHandle( runtimeDir: string, protocolVersion = PROTOCOL_VERSION, - mode?: 'degraded-new-pty-fallback' + mode?: DaemonLaunchMode ): DaemonProcessHandle { const handle: DaemonProcessHandle = { shutdown: async () => { @@ -460,11 +464,12 @@ function createOutOfProcessLauncher( | { reason: Parameters[0] liveSessionCount: number | null + /** Rendered once the outcome is known; only the health-check branch announces. */ + verdict?: string + announce?: boolean } | undefined let confirmedReplacement = false - let healthCheckReplacementVerdict: string | null = null - let healthCheckReplacementAnnounced = false let adoptionClient: DaemonClient | null = new DaemonClient({ socketPath, tokenPath @@ -477,9 +482,16 @@ function createOutOfProcessLauncher( adoptionClient.disconnect() adoptionClient = null } - const preserveDaemon = async ( - mode?: 'degraded-new-pty-fallback' - ): Promise => { + /** + * Keep the incumbent without talking to it. No adoption, so no lease: the lease only + * cancels an adoption watchdog, which cannot fire on a daemon that owns sessions. + */ + const holdIncumbentDaemon = (): DaemonProcessHandle => { + adoptionClient?.disconnect() + adoptionClient = null + return createPreservedDaemonHandle(runtimeDir, PROTOCOL_VERSION, 'held') + } + const preserveDaemon = async (mode?: DaemonLaunchMode): Promise => { const connectedClient = adoptionClient ?? undefined adoptionClient = null return holdDaemonAdoptionLease( @@ -579,65 +591,69 @@ function createOutOfProcessLauncher( } } } else { - // Why: a busy machine can time out the health check on a live daemon; re-verify with a session list before killing its sessions. - let liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) + // Why: a busy machine can time out the health check on a live daemon; re-verify what + // it is hosting before killing its sessions. + const verifiedPid = await readVerifiedDaemonPid(runtimeDir, socketPath, tokenPath) + let occupancy = await resolveDaemonOccupancy({ + socketPath, + tokenPath, + recordedPid: verifiedPid?.pid ?? null + }) // Why: a wedged-but-connectable daemon (Windows update relaunch) may still own live sessions, so grace-retry before replacing; a permanent wedge (#8689) exhausts the grace, and 'rejected' skips it (handshake refused = never adoptable). let graceRetry = 0 while ( - liveSessionCount === null && + occupancy.state === 'unknown' && health !== 'rejected' && graceRetry < WEDGED_DAEMON_GRACE_RETRIES && - (await probeSocket(socketPath)) + !endpointIsProvenDead(await probeSocketConnect(socketPath)) ) { - liveSessionCount = await getAliveDaemonSessionCount(socketPath, tokenPath) + occupancy = await resolveDaemonOccupancy({ + socketPath, + tokenPath, + recordedPid: verifiedPid?.pid ?? null + }) graceRetry++ } - if (liveSessionCount !== null && liveSessionCount > 0) { + if (occupancy.state === 'occupied') { + // Why hold rather than adopt: a count we could only get from the process table means + // the daemon never answered us, so it cannot answer a hello either and adoption would + // fail. Holding keeps its terminals alive and routes fresh ones locally until the + // wedge clears or the user restarts. + if (occupancy.liveSessions === null) { + console.warn( + '[daemon] DEGRADED MODE: holding a daemon that cannot report its sessions but still owns live terminal processes. Killing it would end them; fresh terminals run on the local provider WITHOUT daemon persistence until it recovers or you restart it (Manage Sessions → Restart).' + ) + return holdIncumbentDaemon() + } + const owned = `${occupancy.liveSessions} live session${occupancy.liveSessions === 1 ? '' : 's'}` if (health === 'pty-spawn-unhealthy') { console.warn( - `[daemon] DEGRADED MODE: preserving daemon that failed the PTY spawn health check because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}. Existing sessions keep working; fresh terminals run on the local provider WITHOUT daemon persistence until you restart the daemon (Manage Sessions → Restart).` + `[daemon] DEGRADED MODE: preserving daemon that failed the PTY spawn health check because it owns ${owned}. Existing sessions keep working; fresh terminals run on the local provider WITHOUT daemon persistence until you restart the daemon (Manage Sessions → Restart).` ) return preserveDaemon('degraded-new-pty-fallback') } console.warn( - `[daemon] Preserving daemon that failed the health check because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}` + `[daemon] Preserving daemon that failed the health check because it owns ${owned}` ) return preserveDaemon() } // Why: a cold start reaches this same fall-through with nothing to replace, so stay - // quiet unless something actually answered — a count, a socket that survived a grace - // retry, or a refused hello. The confirmed-kill case is added below because the one - // replacement that destroys user work, an unverifiable count from a daemon that never - // answered, satisfies none of those and used to be killed in total silence. - healthCheckReplacementAnnounced = - liveSessionCount !== null || graceRetry > 0 || health === 'rejected' - healthCheckReplacementVerdict = `health=${health}, liveSessions=${liveSessionCount ?? 'unverifiable'}, graceRetries=${graceRetry}` + // quiet unless something actually answered — a verified count, a socket that survived + // a grace retry, or a refused hello. // Why: telemetry gates on confirmedReplacement below — the post-kill truth — so a // cold start that killed nothing never reports a replacement. pendingReplacement = { reason: 'failed_health_check', - liveSessionCount + liveSessionCount: occupancy.liveSessions, + verdict: `health=${health}, occupancy=${occupancy.state}, graceRetries=${graceRetry}`, + announce: occupancy.state === 'empty' || graceRetry > 0 || health === 'rejected' } } // Why: a raw socket can outlive a broken daemon; kill by PID before respawn so the new daemon doesn't race the stale one. adoptionClient?.disconnect() adoptionClient = null - // Why these three conditions: the veto answers "we could not verify" (a daemon that - // answered listSessions with 0 must stay replaceable, as must an explicit user - // restart); a 'rejected' daemon answered and refused the handshake, so it can never be - // adopted and preserving it would loop forever; and an endpoint nothing can reach - // serves no one, so its PTYs are already unreachable and holding it costs the app a - // daemon it could otherwise replace. - const canPreserveLivePtys = - pendingReplacement?.liveSessionCount === null && - health !== 'rejected' && - (await probeSocket(socketPath)) - const killOutcome = canPreserveLivePtys - ? await killStaleDaemon(runtimeDir, socketPath, tokenPath, PROTOCOL_VERSION, { - preserveWhenOwningLivePtys: true - }) - : await killStaleDaemon(runtimeDir, socketPath, tokenPath) + const killOutcome = await killStaleDaemon(runtimeDir, socketPath, tokenPath) if (killOutcome.liveOwnerSurvived) { // Why: forking beside a daemon we could not prove dead is precisely how the endpoint // owner and the session host diverge. But refusing outright would leave the user with @@ -654,7 +670,7 @@ function createOutOfProcessLauncher( // so keep a lease-free handle: the lease only cancels the adoption watchdog, which // cannot fire on a daemon that still owns sessions, and throwing here would cost the // app its daemon handle — taking Manage Sessions → Restart down with it. - if (await probeSocket(socketPath)) { + if (!endpointIsProvenDead(await probeSocketConnect(socketPath))) { return createPreservedDaemonHandle( runtimeDir, PROTOCOL_VERSION, @@ -667,12 +683,9 @@ function createOutOfProcessLauncher( ) } } - if ( - healthCheckReplacementVerdict && - (healthCheckReplacementAnnounced || killOutcome.killed) - ) { + if (pendingReplacement?.verdict && (pendingReplacement.announce || killOutcome.killed)) { console.warn( - `[daemon] Replacing daemon that failed the health check (${healthCheckReplacementVerdict})` + `[daemon] Replacing daemon that failed the health check (${pendingReplacement.verdict})` ) } confirmedReplacement = killOutcome.killed || confirmedReplacement @@ -1021,27 +1034,17 @@ export async function initDaemonPtyProvider( let routedAdapter: DaemonProvider = newAdapter try { // Why: the launcher's temporary pair closes only after this permanent pair is established, leaving no adoption gap. - // Why tolerated in degraded mode: it exists for a daemon we deliberately kept without - // being able to talk to it, so a lease it cannot grant must not abort init — that would - // leave the app with no spawner at all, and restartDaemon() throws without one. - let permanentLeaseHeld = true - if (launchMode === 'degraded-new-pty-fallback') { - permanentLeaseHeld = await newAdapter.establishLifecycleLease().then( - () => true, - () => false - ) - } else { + // Why skipped when held: we deliberately never talked to that daemon, so there is no + // handshake to complete and no temporary lease to hand over. Attempting one would throw + // and abort init, leaving the app with no spawner — and restartDaemon() throws without one. + if (launchMode !== 'held') { await newAdapter.establishLifecycleLease() - } - // Why conditional: releasing the launcher's temporary pair with no permanent pair - // established is exactly the adoption gap the ordering above exists to prevent. - if (permanentLeaseHeld) { releaseDaemonAdoptionLease(newSpawner.getHandle()) } legacyAdapters = await createLegacyDaemonAdapters(runtimeDir) routedAdapter = - launchMode === 'degraded-new-pty-fallback' + launchMode === 'degraded-new-pty-fallback' || launchMode === 'held' ? new DegradedDaemonPtyProvider({ current: newAdapter, legacy: legacyAdapters, diff --git a/src/main/daemon/daemon-occupancy.test.ts b/src/main/daemon/daemon-occupancy.test.ts new file mode 100644 index 00000000000..a534a3b95f1 --- /dev/null +++ b/src/main/daemon/daemon-occupancy.test.ts @@ -0,0 +1,137 @@ +import { describe, expect, it, vi } from 'vitest' +import { resolveDaemonOccupancy, type DaemonOccupancyDeps } from './daemon-occupancy' +import type { inspectDaemonPtyOwnership } from './daemon-live-pty-evidence' + +const SOCKET_PATH = '/tmp/orca-daemon.sock' +const TOKEN_PATH = '/tmp/orca-daemon.token' +const DAEMON_PID = 4242 + +type Ownership = Awaited> + +function ipcAnswers(count: number | null) { + return vi.fn>(async () => count) +} + +function ownershipIs(ownership: Ownership) { + return vi.fn(async () => ownership) +} + +function resolve( + deps: DaemonOccupancyDeps, + recordedPid: number | null = DAEMON_PID +): Promise>> { + return resolveDaemonOccupancy({ + socketPath: SOCKET_PATH, + tokenPath: TOKEN_PATH, + recordedPid, + deps + }) +} + +describe('resolveDaemonOccupancy with a daemon that answered', () => { + it('reports occupied with the counted sessions, without consulting the process table', async () => { + const listSessions = ipcAnswers(3) + const inspectPtyOwnership = ownershipIs('no-live-ptys') + + await expect(resolve({ listSessions, inspectPtyOwnership })).resolves.toEqual({ + state: 'occupied', + liveSessions: 3 + }) + expect(listSessions).toHaveBeenCalledWith(SOCKET_PATH, TOKEN_PATH) + // The daemon's own reply is authoritative; process-table evidence could only muddy it. + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('reports empty on a count of zero, without consulting the process table', async () => { + // The one state that licenses a kill, and only the daemon itself can establish it. + const listSessions = ipcAnswers(0) + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect(resolve({ listSessions, inspectPtyOwnership })).resolves.toEqual({ + state: 'empty', + liveSessions: 0 + }) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) + + it('reports occupied for a single session', async () => { + await expect( + resolve({ listSessions: ipcAnswers(1), inspectPtyOwnership: ownershipIs('unknown') }) + ).resolves.toEqual({ state: 'occupied', liveSessions: 1 }) + }) +}) + +describe('resolveDaemonOccupancy when the daemon could not answer', () => { + it('raises to occupied on process-table evidence, keyed to the recorded pid', async () => { + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect(resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership })).resolves.toEqual( + { + state: 'occupied', + liveSessions: null + } + ) + expect(inspectPtyOwnership).toHaveBeenCalledWith(DAEMON_PID) + }) + + it('stays unknown — never empty — when the process table shows no live PTYs', async () => { + // The asymmetry the module exists for: the table may only ever *raise* the answer. + // A daemon too wedged to list its sessions is exactly as likely to be hosting them, + // and ps can miss PTYs it never observed. Reading this as 'empty' would license + // killing live agents unrecoverably; 'unknown' is the residual, not permission. + const inspectPtyOwnership = ownershipIs('no-live-ptys') + + await expect(resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership })).resolves.toEqual( + { + state: 'unknown', + liveSessions: null + } + ) + expect(inspectPtyOwnership).toHaveBeenCalledWith(DAEMON_PID) + }) + + it('stays unknown when the process table could not be read', async () => { + await expect( + resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership: ownershipIs('unknown') }) + ).resolves.toEqual({ state: 'unknown', liveSessions: null }) + }) + + it('stays unknown without inspecting an unverified pid', async () => { + // A pid we could not tie back to this daemon may have been recycled; its children + // would be some other process's, and counting them is evidence about the wrong tree. + const inspectPtyOwnership = ownershipIs('owns-live-ptys') + + await expect( + resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership }, null) + ).resolves.toEqual({ state: 'unknown', liveSessions: null }) + expect(inspectPtyOwnership).not.toHaveBeenCalled() + }) +}) + +describe('resolveDaemonOccupancy when an injected dep throws', () => { + it('propagates an inspector rejection rather than degrading to unknown', async () => { + // Pins current behavior, and it is a gap: the shipped inspector swallows its own + // failures, so this only bites an injected or future-refactored one. A throw here + // escapes into the launch path instead of landing on the safe 'unknown' residual. + const inspectPtyOwnership = vi.fn(async () => { + throw new Error('process table read exploded') + }) + + await expect(resolve({ listSessions: ipcAnswers(null), inspectPtyOwnership })).rejects.toThrow( + 'process table read exploded' + ) + }) + + it('propagates a failing listSessions dep, which the real one never does', async () => { + // countLiveSessionsOverIpc catches internally and returns null; an injected dep is + // not held to that, and the module adds no guard of its own. + await expect( + resolve({ + listSessions: async () => { + throw new Error('socket vanished') + }, + inspectPtyOwnership: ownershipIs('owns-live-ptys') + }) + ).rejects.toThrow('socket vanished') + }) +}) diff --git a/src/main/daemon/daemon-occupancy.ts b/src/main/daemon/daemon-occupancy.ts new file mode 100644 index 00000000000..c0757cada68 --- /dev/null +++ b/src/main/daemon/daemon-occupancy.ts @@ -0,0 +1,69 @@ +import { DaemonClient } from './client' +import { inspectDaemonPtyOwnership } from './daemon-live-pty-evidence' +import { PROTOCOL_VERSION, type ListSessionsResult } from './types' + +/** + * How much work a daemon is hosting, and how sure we are. + * + * The distinction that matters: only the daemon itself can prove it is *empty*. + * The OS process table can prove work exists, but a table that shows nothing may + * simply have failed to observe it — so it may only ever add protection, never + * license a kill. 'unknown' is the residual, and it is not permission. + */ +export type DaemonOccupancy = + | { state: 'occupied'; liveSessions: number | null } + | { state: 'empty'; liveSessions: 0 } + | { state: 'unknown'; liveSessions: null } + +export type DaemonOccupancyDeps = { + listSessions?: (socketPath: string, tokenPath: string) => Promise + inspectPtyOwnership?: typeof inspectDaemonPtyOwnership +} + +/** Live session count over the daemon's own socket; null when it could not answer. */ +async function countLiveSessionsOverIpc( + socketPath: string, + tokenPath: string +): Promise { + const client = new DaemonClient({ socketPath, tokenPath, protocolVersion: PROTOCOL_VERSION }) + try { + await client.ensureConnected() + const result = await client.request('listSessions', undefined) + return result.sessions.filter((session) => session.isAlive).length + } catch { + return null + } finally { + client.disconnect() + } +} + +/** + * Ask the daemon first — a reply is authoritative both ways. Only when it cannot + * answer do we fall back to the process table, and then only to *raise* the answer + * to 'occupied'. A blind or empty-looking table stays 'unknown', because a daemon + * too wedged to list its sessions is exactly as likely to be hosting them. + * + * `recordedPid` must already be identity-verified, or the evidence could describe + * a recycled pid's children rather than this daemon's terminals. + */ +export async function resolveDaemonOccupancy(args: { + socketPath: string + tokenPath: string + recordedPid: number | null + deps?: DaemonOccupancyDeps +}): Promise { + const { socketPath, tokenPath, recordedPid, deps = {} } = args + const counted = await (deps.listSessions ?? countLiveSessionsOverIpc)(socketPath, tokenPath) + if (counted !== null) { + return counted > 0 + ? { state: 'occupied', liveSessions: counted } + : { state: 'empty', liveSessions: 0 } + } + if (recordedPid === null) { + return { state: 'unknown', liveSessions: null } + } + const ownership = await (deps.inspectPtyOwnership ?? inspectDaemonPtyOwnership)(recordedPid) + return ownership === 'owns-live-ptys' + ? { state: 'occupied', liveSessions: null } + : { state: 'unknown', liveSessions: null } +} diff --git a/src/main/daemon/daemon-spawner.ts b/src/main/daemon/daemon-spawner.ts index 6a6d377cc2f..80eaba32088 100644 --- a/src/main/daemon/daemon-spawner.ts +++ b/src/main/daemon/daemon-spawner.ts @@ -28,8 +28,15 @@ export type DaemonPidFile = { spawnerExecPath?: string } +/** + * 'degraded-new-pty-fallback' — adopted, but it cannot spawn fresh PTYs. + * 'held' — deliberately kept without adopting it: it owns live terminals but cannot + * answer a handshake, so there is no lease to take and none must be attempted. + */ +export type DaemonLaunchMode = 'degraded-new-pty-fallback' | 'held' + export type DaemonProcessHandle = { - mode?: 'degraded-new-pty-fallback' + mode?: DaemonLaunchMode releaseAdoptionLease?(): void shutdown(): Promise }