diff --git a/src/main/daemon/daemon-host-relocation.test.ts b/src/main/daemon/daemon-host-relocation.test.ts index e5f7b1dceb0..0d2323fd449 100644 --- a/src/main/daemon/daemon-host-relocation.test.ts +++ b/src/main/daemon/daemon-host-relocation.test.ts @@ -6,6 +6,7 @@ import { readFileSync, readdirSync, rmSync, + utimesSync, writeFileSync } from 'node:fs' import os from 'node:os' @@ -267,6 +268,13 @@ describe('getRelocatedDaemonHost', () => { }) }) +// Why: quarantine refuses records written in the last minute, because an in-flight publish is +// indistinguishable from a torn one. Age a record so it stands for a settled corrupt record. +function ageRecordPastQuarantineFloor(recordPath: string): void { + const aged = new Date(Date.now() - 5 * 60_000) + utimesSync(recordPath, aged, aged) +} + describe('pruneOldDaemonHosts', () => { it('removes unpinned non-current version dirs, keeping current and pinned', () => { const root = join(localAppDataDir, 'Orca', 'daemon-host') @@ -437,6 +445,7 @@ describe('pruneOldDaemonHosts', () => { // writer of a mid-digits tear died mid-write, so quarantine must not consult any probe. const pidPath = join(runtimeDir, 'daemon-v7.pid') writeFileSync(pidPath, '{"pid":123') + ageRecordPastQuarantineFloor(pidPath) const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { throw Object.assign(new Error('no such process'), { code: 'ESRCH' }) }) @@ -464,6 +473,7 @@ describe('pruneOldDaemonHosts', () => { // Trusting it would re-create for this one record the eternal veto pruning must not have. const pidPath = join(runtimeDir, 'daemon-v7.pid') writeFileSync(pidPath, '{"pid":4') + ageRecordPastQuarantineFloor(pidPath) const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { throw Object.assign(new Error('access denied'), { code: 'EPERM' }) }) @@ -485,6 +495,57 @@ describe('pruneOldDaemonHosts', () => { killSpy.mockRestore() }) + it('never quarantines a corrupt record that was just written', () => { + // A live daemon's record is created before it is written (writeFileSync 'wx'), so a + // concurrent launch can read it as empty. Quarantining it would strand the running daemon's + // record and let the NEXT launch reclaim its host image. + const root = join(localAppDataDir, 'Orca', 'daemon-host') + const runtimeDir = join(userDataDir, 'daemon') + mkdirSync(join(root, '1.0.0'), { recursive: true }) + mkdirSync(runtimeDir, { recursive: true }) + const pidPath = join(runtimeDir, 'daemon-v7.pid') + writeFileSync(pidPath, '') + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + const evidence = collectPinnedDaemonVersions(runtimeDir) + + expect(evidence).toEqual({ + status: 'unverifiable', + reason: + 'the daemon pid file could not be parsed and was written too recently to quarantine: daemon-v7.pid' + }) + expect(existsSync(pidPath)).toBe(true) + expect(existsSync(join(runtimeDir, 'daemon-v7.pid.corrupt'))).toBe(false) + pruneOldDaemonHosts(evidence) + expect(existsSync(join(root, '1.0.0'))).toBe(true) + warnSpy.mockRestore() + }) + + it('never treats a settled empty record as a valid pre-relocation daemon', () => { + // Number('') === 0, so the parser's legacy bare-integer fallback accepts an empty record as + // pid 0 with appVersion null. Skipping it as "pins no host dir" would leave the version + // unpinned and let the prune below reclaim a live daemon's host image. Aged past the + // quarantine floor so this pins the pid guard rather than the freshness guard. + const root = join(localAppDataDir, 'Orca', 'daemon-host') + const runtimeDir = join(userDataDir, 'daemon') + mkdirSync(join(root, '1.0.0'), { recursive: true }) + mkdirSync(runtimeDir, { recursive: true }) + const pidPath = join(runtimeDir, 'daemon-v7.pid') + writeFileSync(pidPath, ' ') + ageRecordPastQuarantineFloor(pidPath) + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + const evidence = collectPinnedDaemonVersions(runtimeDir) + + expect(evidence).toEqual({ + status: 'unverifiable', + reason: 'the daemon pid file could not be parsed and was quarantined: daemon-v7.pid' + }) + pruneOldDaemonHosts(evidence) + expect(existsSync(join(root, '1.0.0'))).toBe(true) + warnSpy.mockRestore() + }) + it('vetoes pruning while a pid salvaged from a corrupt record still answers', () => { const root = join(localAppDataDir, 'Orca', 'daemon-host') const runtimeDir = join(userDataDir, 'daemon') @@ -518,6 +579,7 @@ describe('pruneOldDaemonHosts', () => { mkdirSync(runtimeDir, { recursive: true }) const pidPath = join(runtimeDir, 'daemon-v7.pid') writeFileSync(pidPath, 'not a daemon record') + ageRecordPastQuarantineFloor(pidPath) const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) // Launch with the corrupt record: prune skips once, and the record is quarantined in place diff --git a/src/main/daemon/daemon-host-relocation.ts b/src/main/daemon/daemon-host-relocation.ts index 2846f16016e..a7e8f2b6db2 100644 --- a/src/main/daemon/daemon-host-relocation.ts +++ b/src/main/daemon/daemon-host-relocation.ts @@ -326,7 +326,15 @@ export function collectPinnedDaemonVersions(runtimeDir: string): PinnedDaemonVer } } const parsed = parseDaemonPidFile(contents) - if (!parsed) { + // Why not just `!parsed`: the parser's legacy bare-integer fallback coerces an empty or + // whitespace-only record to pid 0 (Number('') === 0), which is the exact shape a concurrent + // read sees while a live daemon publishes its record — writeFileSync 'wx' creates the file + // before writing it. Such a record would otherwise pass as a valid pre-relocation daemon, + // skip on appVersion === null, and leave its version unpinned, so the prune below would + // reclaim a running daemon's host image. A pid that is not a positive integer names no + // process — process.kill(0, 0) probes the caller's own process group, never a daemon — so + // it is not liveness evidence and must veto rather than be skipped. + if (!parsed || !Number.isInteger(parsed.pid) || parsed.pid <= 0) { return { status: 'unverifiable', reason: quarantineCorruptDaemonPidRecord(runtimeDir, entry.name, contents) diff --git a/src/main/daemon/daemon-pid-record-quarantine.ts b/src/main/daemon/daemon-pid-record-quarantine.ts index b4bbbfdafa0..577e56578d2 100644 --- a/src/main/daemon/daemon-pid-record-quarantine.ts +++ b/src/main/daemon/daemon-pid-record-quarantine.ts @@ -1,8 +1,18 @@ -import { renameSync } from 'node:fs' +import { renameSync, statSync } from 'node:fs' import { join } from 'node:path' import { salvagePidFromCorruptDaemonRecord } from './daemon-pid-file-parse' import { inspectProcessLiveness } from './daemon-process-inspection' +/** + * A record that was just written is never quarantined: publishDaemonPidFile creates the record + * before writing it (writeFileSync with flag 'wx'), so a concurrent launch can read a LIVE + * daemon's record as empty or torn. Renaming it aside would strand that daemon's record, and the + * next launch — seeing a complete listing with no record for its version — would reclaim the + * running daemon's host image. An in-flight publish is by definition fresh; a record left corrupt + * by a dead writer ages past this floor and is quarantined on a later launch. + */ +const QUARANTINE_MIN_RECORD_AGE_MS = 60_000 + /** * A pid record that parses to nothing would otherwise veto daemon-host pruning on every future * launch: nothing ever rewrites a retired protocol version's pid file, so the veto never expires. @@ -23,8 +33,23 @@ export function quarantineCorruptDaemonPidRecord( console.warn(`[daemon] Keeping corrupt daemon pid record: ${reason}`) return reason } + const recordPath = join(runtimeDir, name) + let modifiedAtMs: number try { - renameSync(join(runtimeDir, name), join(runtimeDir, `${name}.corrupt`)) + modifiedAtMs = statSync(recordPath).mtimeMs + } catch { + const reason = `the daemon pid file could not be parsed or aged: ${name}` + console.warn(`[daemon] ${reason}`) + return reason + } + // A future mtime (clock adjustment) reads as negative age and is treated as fresh. + if (Date.now() - modifiedAtMs < QUARANTINE_MIN_RECORD_AGE_MS) { + const reason = `the daemon pid file could not be parsed and was written too recently to quarantine: ${name}` + console.warn(`[daemon] Keeping corrupt daemon pid record: ${reason}`) + return reason + } + try { + renameSync(recordPath, join(runtimeDir, `${name}.corrupt`)) } catch { const reason = `the daemon pid file could not be parsed or quarantined: ${name}` console.warn(`[daemon] ${reason}`)