From 4b4c2fa213cef21ada5e079fc0621b527a9b2a23 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:22:58 -0700 Subject: [PATCH] fix(daemon): retire stale bundle after session drain --- src/main/daemon/daemon-health.ts | 60 +++++++-- src/main/daemon/daemon-init.test.ts | 89 +++++++------ src/main/daemon/daemon-init.ts | 6 +- src/main/daemon/daemon-pty-adapter.test.ts | 118 ++++++++++++++++-- src/main/daemon/daemon-pty-adapter.ts | 69 +++++++++- ...daemon-tcc-attribution-main-thread.test.ts | 40 +++++- 6 files changed, 320 insertions(+), 62 deletions(-) diff --git a/src/main/daemon/daemon-health.ts b/src/main/daemon/daemon-health.ts index 6eba934afe7..613088d799e 100644 --- a/src/main/daemon/daemon-health.ts +++ b/src/main/daemon/daemon-health.ts @@ -671,6 +671,11 @@ async function readVerifiedDaemonPid( return parsedPid } +let cachedDaemonBundleStaleness: { + key: string + pending: Promise +} | null = null + export async function isDaemonStaleForCurrentBundle( runtimeDir: string, socketPath: string, @@ -678,19 +683,56 @@ export async function isDaemonStaleForCurrentBundle( currentAppVersion: string, protocolVersion = PROTOCOL_VERSION ): Promise { - const parsedPid = await readVerifiedDaemonPid(runtimeDir, socketPath, tokenPath, protocolVersion) - if (!parsedPid) { - return false + let cacheKey: string | null = null + try { + cacheKey = JSON.stringify([ + runtimeDir, + socketPath, + tokenPath, + currentAppVersion, + protocolVersion, + readFileSync(getDaemonPidPath(runtimeDir, protocolVersion), 'utf8') + ]) + } catch { + // Retry the verified read below so a transient PID-file race does not become sticky. } - if (parsedPid.appVersion !== null) { - return parsedPid.appVersion !== currentAppVersion + if (cacheKey && cachedDaemonBundleStaleness?.key === cacheKey) { + return (await cachedDaemonBundleStaleness.pending) ?? false } - // Why: older packaged daemons do not carry a reliable build-generation - // marker. Replacing them once prevents archive-preserved mtimes from - // reusing stale native modules across the first metadata-aware upgrade. - return true + const pending = (async (): Promise => { + const parsedPid = await readVerifiedDaemonPid( + runtimeDir, + socketPath, + tokenPath, + protocolVersion + ) + if (!parsedPid) { + return null + } + + if (parsedPid.appVersion !== null) { + return parsedPid.appVersion !== currentAppVersion + } + + // Why: older packaged daemons do not carry a reliable build-generation + // marker. Replacing them once prevents archive-preserved mtimes from + // reusing stale native modules across the first metadata-aware upgrade. + return true + })() + if (cacheKey) { + cachedDaemonBundleStaleness = { key: cacheKey, pending } + } + const stale = await pending + if ( + stale === null && + cachedDaemonBundleStaleness?.key === cacheKey && + cachedDaemonBundleStaleness.pending === pending + ) { + cachedDaemonBundleStaleness = null + } + return stale ?? false } // 'severed': macOS can no longer resolve the daemon's TCC responsible process, so diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index baea37a88ea..361ffb76468 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -237,7 +237,10 @@ type MockAdapter = { socketPath: string tokenPath: string historyPath?: string - respawn?: (reason: 'daemon_died' | 'unhealthy_resolver') => Promise + packagedAppVersion?: string | null + respawn?: ( + reason: 'daemon_died' | 'unhealthy_resolver' | 'stale_bundle' | 'severed_tcc_attribution' + ) => Promise protocolVersion?: number } getActiveSessionIds: ReturnType @@ -570,11 +573,21 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { setLocalPtyProviderMock.mock.invocationCallOrder[0] ) expect(adoptionLeaseReleases[0]).toHaveBeenCalledOnce() + expect(adapterInstances[0].options.packagedAppVersion).toBeNull() expect(adapterInstances[0].establishLifecycleLease.mock.invocationCallOrder[0]).toBeLessThan( adoptionLeaseReleases[0].mock.invocationCallOrder[0] ) }) + it('passes the packaged app version to runtime stale-bundle retirement', async () => { + const mod = await importFresh() + isPackagedMock.mockReturnValue(true) + + await mod.initDaemonPtyProvider() + + expect(adapterInstances[0].options.packagedAppVersion).toBe('1.2.3') + }) + it('uses daemon-owned idle retirement when a fresh launch fails permanent adoption', async () => { const mod = await importFresh() ensureRunningOverrides.push(async () => ({ @@ -1984,46 +1997,48 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { // STA-2376 regression: dropping the adapter's last authenticated client is enough to make an idle // daemon self-retire, so by the time the launcher runs there is nothing to kill and its own - // confirmed-kill gate reports nothing. The attributed reason is what keeps the runtime resolver - // replacement on the wire — and keeps it off the failed_health_check bucket it would otherwise land in. - it('reports the runtime resolver replacement even after the daemon self-retired', async () => { - const mod = await importFresh() - await mod.initDaemonPtyProvider() - const adapterOptions = adapterInstances[0].options - trackDaemonReplacedMock.mockClear() + // confirmed-kill gate reports nothing. The attributed reason keeps the replacement on the wire. + it.each(['unhealthy_resolver', 'stale_bundle'] as const)( + 'reports the %s replacement even after the daemon self-retired', + async (reason) => { + const mod = await importFresh() + await mod.initDaemonPtyProvider() + const adapterOptions = adapterInstances[0].options + trackDaemonReplacedMock.mockClear() - // The daemon is gone before the launcher looks: nothing answers, nothing left to kill. - checkDaemonHealthMock.mockResolvedValue('unreachable') - killStaleDaemonMock - .mockResolvedValueOnce({ killed: false, liveOwnerSurvived: false }) - .mockResolvedValueOnce({ killed: false, liveOwnerSurvived: false }) - forkMock.mockImplementationOnce(() => { - throw new Error('stop after replacement decision') - }) - const launcher = spawnerInstances[0].launcher as ( - socketPath: string, - tokenPath: string - ) => Promise<{ shutdown(): Promise }> + // The daemon is gone before the launcher looks: nothing answers, nothing left to kill. + checkDaemonHealthMock.mockResolvedValue('unreachable') + killStaleDaemonMock + .mockResolvedValueOnce({ killed: false, liveOwnerSurvived: false }) + .mockResolvedValueOnce({ killed: false, liveOwnerSurvived: false }) + forkMock.mockImplementationOnce(() => { + throw new Error('stop after replacement decision') + }) + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> - await adapterOptions.respawn?.('unhealthy_resolver') - expect(trackDaemonReplacedMock).not.toHaveBeenCalled() + await adapterOptions.respawn?.(reason) + expect(trackDaemonReplacedMock).not.toHaveBeenCalled() - await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow( - 'stop after replacement decision' - ) - expect(trackDaemonReplacedMock).toHaveBeenCalledTimes(1) - expect(trackDaemonReplacedMock).toHaveBeenCalledWith('unhealthy_resolver', 0) + await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow( + 'stop after replacement decision' + ) + expect(trackDaemonReplacedMock).toHaveBeenCalledTimes(1) + expect(trackDaemonReplacedMock).toHaveBeenCalledWith(reason, 0) - // One-shot: a later unrelated launch must not inherit the attribution. - trackDaemonReplacedMock.mockClear() - forkMock.mockImplementationOnce(() => { - throw new Error('stop after replacement decision') - }) - await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow( - 'stop after replacement decision' - ) - expect(trackDaemonReplacedMock).not.toHaveBeenCalled() - }) + // One-shot: a later unrelated launch must not inherit the attribution. + trackDaemonReplacedMock.mockClear() + forkMock.mockImplementationOnce(() => { + throw new Error('stop after replacement decision') + }) + await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow( + 'stop after replacement decision' + ) + expect(trackDaemonReplacedMock).not.toHaveBeenCalled() + } + ) // STA-2376: the attribution covers the case the confirmed-kill gate cannot see; it must not // overwrite a reason this launch proved against the daemon it actually removed. Otherwise a diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index 3787f1c7cec..594c201a986 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -962,6 +962,7 @@ export async function initDaemonPtyProvider( pidPath: getDaemonPidPath(runtimeDir), profileScope: runtimeDir, runtimeDir, + packagedAppVersion: app.isPackaged ? app.getVersion() : null, historyPath: getHistoryDir(), // Why: on daemon death, ensureConnected() detects the dead socket and calls this to fork a replacement before retrying. respawn: async (reason: DaemonRespawnReason) => { @@ -976,7 +977,7 @@ export async function initDaemonPtyProvider( if (!restartInFlight) { trackDaemonRetired('died_respawn') } - } else if (reason === 'unhealthy_resolver' || reason === 'severed_tcc_attribution') { + } else { // Must reach the launcher below without an await in between; see the consume site. attributedReplaceReason = reason } @@ -1197,6 +1198,7 @@ async function runRestartDaemon(): Promise { pidPath: getDaemonPidPath(runtimeDir), profileScope: runtimeDir, runtimeDir, + packagedAppVersion: app.isPackaged ? app.getVersion() : null, historyPath: getHistoryDir(), respawn: async (reason: DaemonRespawnReason) => { // Why: attribute rather than emit — the launcher below is the one that completes the @@ -1210,7 +1212,7 @@ async function runRestartDaemon(): Promise { if (!restartInFlight) { trackDaemonRetired('died_respawn') } - } else if (reason === 'unhealthy_resolver' || reason === 'severed_tcc_attribution') { + } else { // Must reach the launcher below without an await in between; see the consume site. attributedReplaceReason = reason } diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index d94cb9fe891..c74007d2a6f 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -24,16 +24,19 @@ import { getDaemonSocketPath, serializeDaemonPidFile } from './daemon-spawner' import { PtyWriteUnavailableError } from '../providers/pty-write-unavailable-error' import { TERMINAL_HISTORY_INLINE_SEED_CODE_UNITS } from './terminal-history-seed-chunks' -const { getMacDaemonSystemResolverHealthMock, getMacDaemonTccAttributionHealthMock } = vi.hoisted( - () => ({ - getMacDaemonSystemResolverHealthMock: vi.fn( - async (): Promise<'unknown' | 'unhealthy'> => 'unknown' - ), - getMacDaemonTccAttributionHealthMock: vi.fn( - async (): Promise<'intact' | 'severed' | 'unknown'> => 'unknown' - ) - }) -) +const { + getMacDaemonSystemResolverHealthMock, + getMacDaemonTccAttributionHealthMock, + isDaemonStaleForCurrentBundleMock +} = vi.hoisted(() => ({ + getMacDaemonSystemResolverHealthMock: vi.fn( + async (): Promise<'unknown' | 'unhealthy'> => 'unknown' + ), + getMacDaemonTccAttributionHealthMock: vi.fn( + async (): Promise<'intact' | 'severed' | 'unknown'> => 'unknown' + ), + isDaemonStaleForCurrentBundleMock: vi.fn(async () => false) +})) const itOnPosix = process.platform === 'win32' ? it.skip : it @@ -42,7 +45,8 @@ vi.mock('./daemon-health', async (importOriginal) => { return { ...actual, getMacDaemonSystemResolverHealth: getMacDaemonSystemResolverHealthMock, - getMacDaemonTccAttributionHealth: getMacDaemonTccAttributionHealthMock + getMacDaemonTccAttributionHealth: getMacDaemonTccAttributionHealthMock, + isDaemonStaleForCurrentBundle: isDaemonStaleForCurrentBundleMock } }) @@ -147,6 +151,8 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { getMacDaemonSystemResolverHealthMock.mockResolvedValue('unknown') getMacDaemonTccAttributionHealthMock.mockReset() getMacDaemonTccAttributionHealthMock.mockResolvedValue('unknown') + isDaemonStaleForCurrentBundleMock.mockReset() + isDaemonStaleForCurrentBundleMock.mockResolvedValue(false) }) it('reports whether its daemon protocol can participate in agent claims', () => { @@ -4109,6 +4115,96 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { respawnAdapter.dispose() }) + it('preserves a stale packaged daemon that still owns live sessions before a new spawn', async () => { + const respawnFn = vi.fn() + const respawnAdapter = new DaemonPtyAdapter({ + socketPath, + tokenPath, + runtimeDir: dir, + packagedAppVersion: '1.4.178', + respawn: respawnFn + }) + await respawnAdapter.spawn({ cols: 80, rows: 24, isNewSession: true }) + isDaemonStaleForCurrentBundleMock.mockResolvedValueOnce(true) + + const next = await respawnAdapter.spawn({ cols: 80, rows: 24, isNewSession: true }) + + expect(isDaemonStaleForCurrentBundleMock).toHaveBeenCalledWith( + dir, + socketPath, + tokenPath, + '1.4.178', + respawnAdapter.protocolVersion + ) + expect(respawnFn).not.toHaveBeenCalled() + expect(next.id).toBeDefined() + + respawnAdapter.dispose() + }) + + it('preserves a stale packaged daemon when its live session inventory is unavailable', async () => { + const respawnFn = vi.fn() + const respawnAdapter = new DaemonPtyAdapter({ + socketPath, + tokenPath, + runtimeDir: dir, + packagedAppVersion: '1.4.178', + respawn: respawnFn + }) + const internals = respawnAdapter as unknown as { + client: { request: (type: string, payload?: unknown) => Promise } + } + const originalRequest = internals.client.request.bind(internals.client) + vi.spyOn(internals.client, 'request').mockImplementation((type, payload) => { + if (type === 'listSessions') { + return Promise.reject(new Error('inventory unavailable')) + } + return originalRequest(type, payload) + }) + isDaemonStaleForCurrentBundleMock.mockResolvedValueOnce(true) + + const next = await respawnAdapter.spawn({ cols: 80, rows: 24, isNewSession: true }) + + expect(respawnFn).not.toHaveBeenCalled() + expect(next.id).toBeDefined() + + respawnAdapter.dispose() + }) + + it('coalesces stale-bundle retirement before concurrent fresh sessions', async () => { + let respawnServer: DaemonServer | undefined + const respawnFn = vi.fn(async () => { + await server.shutdown() + rmSync(socketPath, { force: true }) + respawnServer = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await respawnServer.start() + }) + const respawnAdapter = new DaemonPtyAdapter({ + socketPath, + tokenPath, + runtimeDir: dir, + packagedAppVersion: '1.4.178', + respawn: respawnFn + }) + isDaemonStaleForCurrentBundleMock.mockResolvedValue(true) + + const replacements = await Promise.all([ + respawnAdapter.spawn({ cols: 80, rows: 24, isNewSession: true }), + respawnAdapter.spawn({ cols: 80, rows: 24, isNewSession: true }) + ]) + + expect(respawnFn).toHaveBeenCalledTimes(1) + expect(respawnFn).toHaveBeenCalledWith('stale_bundle') + expect(replacements.every((replacement) => replacement.id)).toBe(true) + + respawnAdapter.dispose() + await respawnServer?.shutdown() + }) + it('preserves a severed-TCC daemon that still owns live sessions before a new spawn', async () => { const respawnFn = vi.fn() const respawnAdapter = new DaemonPtyAdapter({ diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index 57d1216109f..2ea8179846e 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -7,6 +7,7 @@ import { DAEMON_ENDPOINT_LOST_MESSAGE } from './daemon-endpoint-ownership' import { getMacDaemonSystemResolverHealth, getMacDaemonTccAttributionHealth, + isDaemonStaleForCurrentBundle, parseDaemonPidFile, type ParsedDaemonPid } from './daemon-health' @@ -141,11 +142,17 @@ export type DaemonPtyAdapterOptions = { historyPath?: string /** Runtime profile directory used to verify daemon TCC attribution. */ runtimeDir?: string - /** Forks a fresh daemon after endpoint death or a confirmed resolver-health replacement. */ + /** Current packaged version, or null for unpackaged builds. */ + packagedAppVersion?: string | null + /** Forks a fresh daemon after endpoint death or a confirmed health replacement. */ respawn?: (reason: DaemonRespawnReason) => Promise void)> } -export type DaemonRespawnReason = 'daemon_died' | 'unhealthy_resolver' | 'severed_tcc_attribution' +export type DaemonRespawnReason = + | 'daemon_died' + | 'unhealthy_resolver' + | 'stale_bundle' + | 'severed_tcc_attribution' export type DaemonIdentityChangeEvent = { previous: DaemonEndpointIdentity @@ -234,10 +241,12 @@ export class DaemonPtyAdapter implements IPtyProvider { private historyReader: HistoryReader | null private respawnFn: DaemonPtyAdapterOptions['respawn'] | null private runtimeDir: string | null + private packagedAppVersion: string | null private pendingRespawnAdoptionRelease: (() => void) | null = null private respawnAdoptionClosed = false // Why: concurrent spawn() calls hitting a dead daemon would each fork their own; this promise coalesces respawns so only the first forks and the rest await it. private respawnPromise: Promise | null = null + private staleBundleReplacementPromise: Promise | null = null private writeRecoveryPromise: Promise | null = null private writeRecoveryAttempted = false private dataListeners: ((payload: { @@ -355,6 +364,7 @@ export class DaemonPtyAdapter implements IPtyProvider { this.historyReader = opts.historyPath ? new HistoryReader(opts.historyPath) : null this.respawnFn = opts.respawn ?? null this.runtimeDir = opts.runtimeDir ?? opts.profileScope ?? null + this.packagedAppVersion = opts.packagedAppVersion ?? null this.supportsCheckpoints = this.protocolVersion >= 4 this.supportsIncrementalCheckpoints = this.protocolVersion >= 13 this.supportsProducerFlowControl = this.protocolVersion >= 19 @@ -526,6 +536,7 @@ export class DaemonPtyAdapter implements IPtyProvider { if (opts.isNewSession) { await this.replaceUnhealthyMacResolverDaemonBeforeNewPty() + await this.replaceStaleBundleDaemonBeforeNewPty() await this.replaceSeveredMacTccDaemonBeforeNewPty() } @@ -2665,6 +2676,60 @@ export class DaemonPtyAdapter implements IPtyProvider { await this.respawnPromise } + /** Replace a stale packaged daemon only after its live sessions drain. */ + private async replaceStaleBundleDaemonBeforeNewPty(): Promise { + if (!this.respawnFn || !this.runtimeDir || !this.packagedAppVersion) { + return + } + if (!this.staleBundleReplacementPromise) { + this.staleBundleReplacementPromise = this.replaceStaleBundleDaemonOnce( + this.runtimeDir, + this.packagedAppVersion + ).finally(() => { + this.staleBundleReplacementPromise = null + }) + } + await this.staleBundleReplacementPromise + } + + private async replaceStaleBundleDaemonOnce( + runtimeDir: string, + packagedAppVersion: string + ): Promise { + const stale = await isDaemonStaleForCurrentBundle( + runtimeDir, + this.socketPath, + this.tokenPath, + packagedAppVersion, + this.protocolVersion + ) + if (!stale) { + return + } + + const daemonLiveSessionCount = await this.getDaemonLiveSessionCount() + const liveSessionCount = Math.max(this.activeSessionIds.size, daemonLiveSessionCount ?? 0) + if (daemonLiveSessionCount === null || liveSessionCount > 0) { + console.warn( + daemonLiveSessionCount === null + ? '[daemon] Packaged daemon is stale - preserving it because live session state could not be verified' + : `[daemon] Packaged daemon is stale - preserving it because it owns ${liveSessionCount} live session${liveSessionCount === 1 ? '' : 's'}` + ) + return + } + + this.fanoutSyntheticExits(-1) + if (!this.respawnPromise) { + this.respawnPromise = this.doRespawn( + '[daemon] Packaged daemon is stale - respawning from the current app bundle', + 'stale_bundle' + ).finally(() => { + this.respawnPromise = null + }) + } + await this.respawnPromise + } + /** Replace a TCC-severed daemon only after its live sessions drain. */ private async replaceSeveredMacTccDaemonBeforeNewPty(): Promise { // Why no platform gate: getMacDaemonTccAttributionHealth returns 'unknown' off macOS. diff --git a/src/main/daemon/daemon-tcc-attribution-main-thread.test.ts b/src/main/daemon/daemon-tcc-attribution-main-thread.test.ts index 699ba6d4568..6178b589f90 100644 --- a/src/main/daemon/daemon-tcc-attribution-main-thread.test.ts +++ b/src/main/daemon/daemon-tcc-attribution-main-thread.test.ts @@ -43,7 +43,8 @@ vi.mock('node:fs', async (importOriginal) => { }) const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') -const { getMacDaemonTccAttributionHealth } = await import('./daemon-health') +const { getMacDaemonTccAttributionHealth, isDaemonStaleForCurrentBundle } = + await import('./daemon-health') describe('macOS daemon TCC attribution main-thread cost', () => { let dir: string @@ -90,6 +91,43 @@ describe('macOS daemon TCC attribution main-thread cost', () => { ) } + it('deduplicates bundle-staleness identity inspection and invalidates by generation', async () => { + writePidRecord('1.2.2') + + await expect( + Promise.all([ + isDaemonStaleForCurrentBundle(dir, socketPath, tokenPath, '1.2.3'), + isDaemonStaleForCurrentBundle(dir, socketPath, tokenPath, '1.2.3') + ]) + ).resolves.toEqual([true, true]) + await expect(isDaemonStaleForCurrentBundle(dir, socketPath, tokenPath, '1.2.3')).resolves.toBe( + true + ) + expect(execFileMock).toHaveBeenCalledTimes(1) + + writePidRecord('1.2.3') + await expect(isDaemonStaleForCurrentBundle(dir, socketPath, tokenPath, '1.2.3')).resolves.toBe( + false + ) + expect(execFileMock).toHaveBeenCalledTimes(2) + expect(execFileSyncMock).not.toHaveBeenCalled() + }) + + it('retries an indeterminate bundle-staleness identity inspection', async () => { + writePidRecord('1.2.2') + psError.value = new Error('ps unavailable') + await expect(isDaemonStaleForCurrentBundle(dir, socketPath, tokenPath, '1.2.3')).resolves.toBe( + false + ) + + psError.value = null + await expect(isDaemonStaleForCurrentBundle(dir, socketPath, tokenPath, '1.2.3')).resolves.toBe( + true + ) + expect(execFileMock).toHaveBeenCalledTimes(2) + expect(execFileSyncMock).not.toHaveBeenCalled() + }) + it('deduplicates identity inspection by daemon generation without a synchronous spawn', async () => { writePidRecord('1.2.2')