fix(daemon): stop an in-flight pid publish from being read as a dead version

publishDaemonPidFile creates the record before writing it (writeFileSync with
flag 'wx'), so a concurrent launch can read a live daemon's record as empty. A
two-process probe observed the empty window on 7 of 2273 reads.

An empty record was not treated as corrupt at all: the parser's legacy
bare-integer fallback coerces it to pid 0 (Number('') === 0) with appVersion
null, so the scan skipped it as a pre-relocation daemon, left its version
unpinned, and the prune reclaimed a running daemon's host image -- the exact
destructive outcome this change exists to prevent, reached without any
'unverifiable' verdict. A pid that is not a positive integer names no process
(process.kill(0, 0) probes the caller's own process group), so it is now a
veto rather than a skip.

Quarantine additionally refuses any record written in the last minute: an
in-flight publish is by definition fresh, while a record left corrupt by a
dead writer ages past the floor and is quarantined on a later launch. Fixed
locally rather than in parseDaemonPidFile, whose null result also drives an
unlink in daemon-stale-kill.

Each gate is pinned by a test that fails individually when it is reverted.
This commit is contained in:
Brennan Benson
2026-08-28 06:21:03 -07:00
parent d87c20c206
commit 82272b41a4
3 changed files with 98 additions and 3 deletions
@@ -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
+9 -1
View File
@@ -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)
@@ -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}`)