From bba32bd00ca8d39102a0727906157c11316b2fe8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 7 Aug 2026 22:33:50 -0700 Subject: [PATCH] fix(daemon): let the publisher replace a dead endpoint, not a third party (#12882) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Terminals froze app-wide several times daily, needing a manual pkill. libuv unlinks the pathname a server bound to when it closes, with no ownership check, so a departing daemon deleted whichever socket then sat at the canonical path — including a live replacement's. The replacement kept hosting PTYs no client could reach. #12709 fixed that mechanism; this replaces the shape around it. Two invariants: only a daemon publishing itself onto the canonical endpoint may mutate that entry, and only by replacing one it has itself just proven dead; and no actor removes a name it did not create. Publish binds a private name, takes the canonical one with an exclusive link, and on EEXIST proves the incumbent dead by connecting before replacing it in a single rename. Only 'connected' means occupied and only refused/missing prove death — a timeout proves nothing and declines. Deletes the claim sweeper, the reclaim tail of killStaleDaemon, and three unfenced unlinkSync(socketPath) calls in the launcher. Measured: rename exposed no gap across 6,525 darwin / 8,004 linux probes of a live handover, where unlink-then-link gapped on 200 of 200. Verified on all three platforms: full suite on macOS and Linux, and daemon restart e2e on a real windows-2022 host. Contract in src/main/daemon/AGENTS.md. --- config/scripts/daemon-boot-smoke.mjs | 26 +- .../daemon-endpoint-handover-smoke.mjs | 222 +++-- src/main/daemon/AGENTS.md | 50 ++ .../daemon/daemon-endpoint-ownership.test.ts | 100 ++- src/main/daemon/daemon-endpoint-ownership.ts | 330 +++++--- src/main/daemon/daemon-endpoint-probe.ts | 80 ++ .../daemon/daemon-endpoint-publish.test.ts | 770 ++++++++++++++++++ ...emon-endpoint-reachability-test-harness.ts | 22 + .../daemon/daemon-endpoint-windows.test.ts | 78 ++ src/main/daemon/daemon-entry.ts | 12 + .../daemon-health-endpoint-entry.test.ts | 39 + .../daemon-health-socket-cleanup.test.ts | 1 + src/main/daemon/daemon-health.test.ts | 295 +++---- src/main/daemon/daemon-health.ts | 197 +++-- src/main/daemon/daemon-idle-shutdown.test.ts | 37 +- src/main/daemon/daemon-init.test.ts | 49 +- src/main/daemon/daemon-init.ts | 104 ++- src/main/daemon/daemon-pty-adapter.ts | 9 +- .../daemon-server-error-handling.test.ts | 456 +++++++++++ src/main/daemon/daemon-server.test.ts | 12 +- src/main/daemon/daemon-server.ts | 257 ++++-- src/main/daemon/daemon-spawner.test.ts | 212 ++--- src/main/daemon/daemon-spawner.ts | 57 +- tests/e2e/daemon-lifecycle-retirement.spec.ts | 21 +- 24 files changed, 2770 insertions(+), 666 deletions(-) create mode 100644 src/main/daemon/AGENTS.md create mode 100644 src/main/daemon/daemon-endpoint-probe.ts create mode 100644 src/main/daemon/daemon-endpoint-publish.test.ts create mode 100644 src/main/daemon/daemon-endpoint-reachability-test-harness.ts create mode 100644 src/main/daemon/daemon-endpoint-windows.test.ts create mode 100644 src/main/daemon/daemon-health-endpoint-entry.test.ts create mode 100644 src/main/daemon/daemon-server-error-handling.test.ts diff --git a/config/scripts/daemon-boot-smoke.mjs b/config/scripts/daemon-boot-smoke.mjs index 22c78a21c..f43fd8462 100644 --- a/config/scripts/daemon-boot-smoke.mjs +++ b/config/scripts/daemon-boot-smoke.mjs @@ -54,6 +54,19 @@ function makeSocketPath(userDataDir) { return join(userDataDir, 'daemon.sock') } +// 'connected' only when something actually answers; a dead entry left on the name is not it. +function probeEndpoint(socketPath) { + return new Promise((resolveProbe) => { + const socket = connect(socketPath) + const settle = (result) => { + socket.destroy() + resolveProbe(result) + } + socket.on('connect', () => settle('connected')) + socket.on('error', () => settle('unreachable')) + }) +} + function runDaemonRpc(socketPath, tokenPath, protocolVersion, request, timeoutMs) { return new Promise((resolveRpc, rejectRpc) => { let settled = false @@ -267,11 +280,16 @@ async function main() { if (existsSync(pidPath)) { throw new Error('daemon left its PID ownership record behind after shutdown') } - if (process.platform !== 'win32' && existsSync(socketPath)) { - throw new Error('daemon left its endpoint socket behind after shutdown') + // Why not "the entry is gone": a departing daemon deliberately leaves its endpoint behind + // for the next publisher to replace in one rename. What must be true is that nothing + // answers there any more. + if (process.platform !== 'win32' && (await probeEndpoint(socketPath)) === 'connected') { + throw new Error('daemon still answers its endpoint after shutdown') } - // The private bind name is consumed by the publish; nothing may linger in the runtime dir. - const leaked = readdirSync(userDataDir).filter((entry) => entry.startsWith('.b')) + // The private bind name is consumed by the publish, and nothing sweeps the runtime dir any + // more, so a leak here is permanent. Match what the code actually generates rather than a + // literal prefix: this check silently matched nothing after the namespace moved from .b. + const leaked = readdirSync(userDataDir).filter((entry) => /^\.[a-z][0-9a-f]{10}$/.test(entry)) if (leaked.length > 0) { throw new Error(`daemon leaked private bind names: ${leaked.join(', ')}`) } diff --git a/config/scripts/daemon-endpoint-handover-smoke.mjs b/config/scripts/daemon-endpoint-handover-smoke.mjs index b6c39a5b5..729b332c6 100644 --- a/config/scripts/daemon-endpoint-handover-smoke.mjs +++ b/config/scripts/daemon-endpoint-handover-smoke.mjs @@ -1,11 +1,9 @@ /** * Endpoint handover smoke — guards the split-brain failure with real daemon processes. * - * The failure it reproduces: a daemon whose endpoint name is reclaimed while it is still - * alive used to delete the *replacement's* socket when it finally exited, because libuv - * unlinks the pathname a server bound to with no ownership check. The replacement stayed - * alive hosting PTYs that nothing could reach — terminals that acknowledge input and never - * run it, and that a user cannot fix by restarting the app. + * Two starting daemons race to replace one dead endpoint entry. The loser may exit after the + * winner has published, but its close must not remove the winner's canonical socket. The survivor + * must remain reachable through that path with its own token. * * Unix only: Windows named pipes are not directory entries, so the mechanism cannot occur. * @@ -14,17 +12,36 @@ import { fork } from 'node:child_process' import { connect } from 'node:net' import { randomUUID } from 'node:crypto' -import { existsSync, mkdtempSync, rmSync, statSync, unlinkSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, rmSync, statSync } from 'node:fs' import { tmpdir } from 'node:os' import { join, resolve } from 'node:path' const repoRoot = resolve(import.meta.dirname, '..', '..') const entryPath = join(repoRoot, 'out', 'main', 'daemon-entry.js') + +// Why read it from source: the launcher keys adoption of a live incumbent on this exact code, +// and hardcoding it here would let the two drift silently — which is the failure the assertion +// below exists to catch. +const DAEMON_EXIT_ENDPOINT_OCCUPIED = Number( + readFileSync(join(repoRoot, 'src/main/daemon/daemon-endpoint-ownership.ts'), 'utf8').match( + /DAEMON_EXIT_ENDPOINT_OCCUPIED = (\d+)/ + )?.[1] +) const READY_TIMEOUT_MS = 20_000 const EXIT_TIMEOUT_MS = 15_000 +const REACHABILITY_TIMEOUT_MS = 2_000 const log = (msg) => console.log(`[endpoint-handover-smoke] ${msg}`) +function readProtocolVersion() { + const source = readFileSync(join(repoRoot, 'src/main/daemon/daemon-protocol-version.ts'), 'utf8') + const match = source.match(/PROTOCOL_VERSION\s*=\s*(\d+)/) + if (!match) { + throw new Error('could not read daemon protocol version') + } + return Number(match[1]) +} + function bootDaemon(tag, dir, socketPath) { const tokenPath = join(dir, `${tag}.token`) const pidPath = join(dir, `${tag}.pid`) @@ -53,7 +70,7 @@ function bootDaemon(tag, dir, socketPath) { child.stderr?.on('data', (chunk) => { stderr += chunk.toString('utf8') }) - return new Promise((resolveReady, rejectReady) => { + const ready = new Promise((resolveReady, rejectReady) => { const timer = setTimeout( () => rejectReady(new Error(`daemon ${tag} never signaled ready.\nstderr:\n${stderr}`)), READY_TIMEOUT_MS @@ -61,7 +78,7 @@ function bootDaemon(tag, dir, socketPath) { child.on('message', (msg) => { if (msg && typeof msg === 'object' && msg.type === 'ready') { clearTimeout(timer) - resolveReady({ child, tokenPath, pidPath }) + resolveReady() } }) child.on('exit', (code) => { @@ -69,28 +86,80 @@ function bootDaemon(tag, dir, socketPath) { rejectReady(new Error(`daemon ${tag} exited with ${code}.\nstderr:\n${stderr}`)) }) }) + return { child, tokenPath, pidPath, ready } } function isReachable(socketPath) { return new Promise((resolveReachable) => { const socket = connect({ path: socketPath }) - socket.on('connect', () => { + socket.once('connect', () => { socket.destroy() resolveReachable(true) }) - socket.on('error', () => resolveReachable(false)) + socket.once('error', () => { + socket.destroy() + resolveReachable(false) + }) }) } +function isDaemonReachable(socketPath, tokenPath, protocolVersion) { + if (!existsSync(tokenPath)) { + return Promise.resolve(false) + } + const token = readFileSync(tokenPath, 'utf8').trim() + return new Promise((resolveReachable) => { + let buffer = '' + let settled = false + const socket = connect({ path: socketPath }) + const finish = (reachable) => { + if (settled) { + return + } + settled = true + clearTimeout(timer) + socket.destroy() + resolveReachable(reachable) + } + const timer = setTimeout(() => finish(false), REACHABILITY_TIMEOUT_MS) + socket.once('error', () => finish(false)) + socket.once('connect', () => { + socket.write( + `${JSON.stringify({ + type: 'hello', + version: protocolVersion, + token, + clientId: randomUUID(), + role: 'control' + })}\n` + ) + }) + socket.on('data', (chunk) => { + buffer += chunk.toString('utf8') + const newlineIndex = buffer.indexOf('\n') + if (newlineIndex === -1) { + return + } + try { + const message = JSON.parse(buffer.slice(0, newlineIndex)) + finish(message.type === 'hello' && message.ok === true) + } catch { + finish(false) + } + }) + }) +} + +/** Resolves with the child's exit code, so callers can assert on it. */ function killAndWait(child) { if (child.exitCode !== null || child.signalCode !== null) { - return Promise.resolve() + return Promise.resolve(child.exitCode) } return new Promise((resolveExit, rejectExit) => { const timer = setTimeout(() => rejectExit(new Error('daemon did not exit')), EXIT_TIMEOUT_MS) - child.on('exit', () => { + child.on('exit', (code) => { clearTimeout(timer) - resolveExit() + resolveExit(code) }) child.kill('SIGTERM') }) @@ -107,46 +176,103 @@ async function main() { const dir = mkdtempSync(join(tmpdir(), 'orca-endpoint-handover-')) const socketPath = join(dir, 'daemon.sock') - let replaced - let replacement + const protocolVersion = readProtocolVersion() + const daemons = [] try { - replaced = await bootDaemon('replaced', dir, socketPath) - const replacedInode = statSync(socketPath).ino - log('daemon A published the endpoint') + const departed = bootDaemon('departed', dir, socketPath) + daemons.push(departed) + await departed.ready + const deadInode = statSync(socketPath).ino + await killAndWait(departed.child) + if (!existsSync(socketPath) || statSync(socketPath).ino !== deadInode) { + throw new Error('departing daemon did not leave its dead endpoint entry in place') + } + if (await isReachable(socketPath)) { + throw new Error('departed daemon remains reachable') + } + log('daemon A departed and left a dead endpoint entry') - // Reclaim the endpoint name the way daemon replacement does, while A is still alive. - unlinkSync(socketPath) - replacement = await bootDaemon('replacement', dir, socketPath) - const replacementInode = statSync(socketPath).ino - if (replacedInode === replacementInode) { - throw new Error('daemon B did not publish a distinct endpoint') - } - if (!(await isReachable(socketPath))) { - throw new Error('daemon B is not reachable through the canonical endpoint') - } - log('daemon B took over the endpoint and is reachable') - - // A exits long after losing the endpoint. This is the step that used to break B. - await killAndWait(replaced.child) - await new Promise((r) => setTimeout(r, 300)) - - if (!existsSync(socketPath) || statSync(socketPath).ino !== replacementInode) { - throw new Error("daemon A's late exit deleted daemon B's endpoint") - } - if (!(await isReachable(socketPath))) { - throw new Error('daemon B became unreachable after daemon A exited') - } - if (!existsSync(replacement.pidPath)) { - throw new Error("daemon A's late exit removed daemon B's ownership record") - } - if (existsSync(replaced.pidPath)) { - throw new Error('daemon A left its own ownership record behind') + const racers = [bootDaemon('racer-b', dir, socketPath), bootDaemon('racer-c', dir, socketPath)] + daemons.push(...racers) + const readiness = await Promise.allSettled(racers.map((daemon) => daemon.ready)) + if (readiness.every((result) => result.status === 'rejected')) { + throw new Error( + `neither racing daemon published the endpoint:\n${readiness + .map((result) => (result.status === 'rejected' ? result.reason.message : '')) + .join('\n')}` + ) } - log('PASS: the endpoint owner and the session host stayed the same daemon') + const owners = [] + for (const daemon of racers) { + if (await isDaemonReachable(socketPath, daemon.tokenPath, protocolVersion)) { + owners.push(daemon) + } + } + if (owners.length !== 1) { + throw new Error(`expected one reachable racing daemon, found ${owners.length}`) + } + const survivor = owners[0] + const loser = racers.find((daemon) => daemon !== survivor) + const survivorInode = statSync(socketPath).ino + if (survivorInode === deadInode) { + throw new Error('survivor did not replace the dead endpoint entry') + } + log('racing daemon published over the dead entry and is reachable') + + await killAndWait(loser.child) + if (!existsSync(socketPath) || statSync(socketPath).ino !== survivorInode) { + throw new Error("losing racer's exit removed the survivor's endpoint") + } + if (!(await isDaemonReachable(socketPath, survivor.tokenPath, protocolVersion))) { + throw new Error("survivor became unreachable after the losing racer's exit") + } + if (!existsSync(survivor.pidPath)) { + throw new Error('survivor lost its ownership record') + } + if (existsSync(loser.pidPath)) { + throw new Error('losing racer left its ownership record behind') + } + + // Why a second, non-racing phase: above, both racers are awaited to ready-or-exit before a + // winner is identified, so the loser has usually already gone and killing it proves little. + // With a known-live incumbent the interleaving is forced rather than hoped for: the newcomer + // must find the endpoint occupied, refuse to take it, and damage nothing on its way out. + const survivorInodeBeforeBlocked = statSync(socketPath).ino + const blocked = bootDaemon('blocked', dir, socketPath) + daemons.push(blocked) + await blocked.ready.then( + () => { + throw new Error('a daemon published onto an endpoint a live daemon already owned') + }, + () => { + // Expected: it cannot publish onto a live owner's name, so it exits instead. + } + ) + // Why pin the code: the launcher keys adoption of a live incumbent on exactly this exit + // code, so a silent change to it would strand a concurrently starting app on local + // non-persistent terminals with nothing failing. + const blockedExit = await killAndWait(blocked.child) + if (blockedExit !== DAEMON_EXIT_ENDPOINT_OCCUPIED) { + throw new Error( + `a daemon that lost the endpoint exited ${blockedExit}, not ${DAEMON_EXIT_ENDPOINT_OCCUPIED}` + ) + } + if (!existsSync(socketPath) || statSync(socketPath).ino !== survivorInodeBeforeBlocked) { + throw new Error("a daemon that could not publish removed the live owner's endpoint") + } + if (!(await isDaemonReachable(socketPath, survivor.tokenPath, protocolVersion))) { + throw new Error('live owner became unreachable after a newcomer failed to publish') + } + if (existsSync(blocked.pidPath)) { + throw new Error('a daemon that could not publish left an ownership record behind') + } + log('a newcomer refused the live owner’s endpoint and left it intact') + + log('PASS: the racing survivor remains reachable through the canonical endpoint') } finally { - for (const daemon of [replaced, replacement]) { - if (daemon && daemon.child.exitCode === null && daemon.child.signalCode === null) { + for (const daemon of daemons) { + if (daemon.child.exitCode === null && daemon.child.signalCode === null) { try { daemon.child.kill('SIGKILL') } catch { diff --git a/src/main/daemon/AGENTS.md b/src/main/daemon/AGENTS.md new file mode 100644 index 000000000..8e1dfa6aa --- /dev/null +++ b/src/main/daemon/AGENTS.md @@ -0,0 +1,50 @@ +# AGENTS.md — Terminal Daemon + +## Endpoint Ownership: Who May Touch the Socket Path + +Two invariants govern the daemon's canonical socket path. Read this before changing anything that +links, renames, unlinks or stats it — or that treats its existence as evidence a daemon is running. + +> **Only a daemon publishing itself onto the canonical endpoint may mutate that directory entry, +> and only by replacing an entry it has itself just proven dead.** +> +> **No actor removes a name it did not create.** + +**Why it exists.** `net.Server.close()` unlinks the pathname it bound with no ownership check, so a +departing daemon deleted whichever socket then sat at the canonical path — including a live +replacement's. The replacement stayed alive hosting PTYs no client could reach, which reads to the +user as terminals that accept keystrokes and never run them. Seven review rounds against the older +"launcher reclaims a dead process's name" shape produced twenty-three defects, all the same +interleaving: a third party observing liveness at T and acting on the directory entry at T+1. + +**The protocol** (`daemon-endpoint-ownership.ts`): bind a private `.p` name → try an exclusive +`link` → on `EEXIST` prove the incumbent dead by connecting → re-check the entry hasn't changed +hands → probe once more → `rename` in one syscall → verify we kept it. + +## Traps That Already Cost Us + +- **Never collapse "can't tell" into "dead."** Only `connected` means occupied; only + `refused`/`missing` prove death. A timeout or `EPERM` proves nothing and must decline — treating + it as death deletes an endpoint still serving every terminal on the host. +- **`link` first, never an unconditional `rename`.** `rename` replaces whatever it finds, so it + would let a starting daemon destroy a healthy one. `link` fails loudly and forces the liveness + question. +- **`rename`, never `unlink`-then-`link`.** The latter leaves the name absent between two calls; + measured across a live handover it gapped on essentially every observation, where `rename` gapped + on none in ~14,500 probes. +- **Do not identify an entry by `birthtimeMs`.** Node documents it as sometimes holding the ctime, + filesystems without a birth time report the epoch, and its granularity is often coarser than the + events it must separate. Three attempts to patch around this produced three more defects; inode + recycling is now settled by asking whether anything is *serving*. +- **Do not add a sweeper.** Deciding whether someone else's leftover is safe to delete is the + question this design retired; the last one produced five defects, including deleting a live + listener's only pathname. Every actor removes its own scratch name on each non-crash path. +- **Scratch namespaces must stay out of released builds' patterns.** Shipped versions sweep + `^\.b[0-9a-f]{10}$` on age alone with no liveness check, which is why the bind name is `.p`. + Deleting our sweeper does not un-ship theirs. +- **Never remove the endpoint on shutdown.** A departing daemon leaves a dead entry; the next + publisher replaces it in one rename. + +**Residual risk.** The final probe and the `rename` are two syscalls, and POSIX has no +rename-if-target-is-inode-X. The harm is separately unreachable: a daemon never creates a session +on an endpoint it no longer holds (`daemon-server.ts`), and it drains rather than serving on. diff --git a/src/main/daemon/daemon-endpoint-ownership.test.ts b/src/main/daemon/daemon-endpoint-ownership.test.ts index 8f4e4b82f..743e382ce 100644 --- a/src/main/daemon/daemon-endpoint-ownership.test.ts +++ b/src/main/daemon/daemon-endpoint-ownership.test.ts @@ -5,6 +5,7 @@ import { mkdirSync, mkdtempSync, readFileSync, + renameSync, rmSync, unlinkSync, writeFileSync @@ -14,7 +15,11 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { DaemonServer } from './daemon-server' import { getDaemonSocketPath, publishDaemonPidFile } from './daemon-spawner' -import { readDaemonSocketIdentity } from './daemon-endpoint-ownership' +import { + getDaemonSocketBindPath, + readDaemonEndpointOwnershipState, + readDaemonSocketIdentity +} from './daemon-endpoint-ownership' import type { SubprocessHandle } from './session' function connectsTo(socketPath: string): Promise { @@ -43,6 +48,37 @@ function createMockSubprocess(): SubprocessHandle { } } +describe('endpoint ownership identity rules', () => { + it.skipIf(process.platform === 'win32')( + 'reports lost when the name resolves to a different inode', + async () => { + // The loss that is real: another daemon's socket now holds the name. + const dir = mkdtempSync(join(tmpdir(), 'endpoint-identity-lost-')) + const socketPath = join(dir, 'daemon.sock') + const ours = createServer((socket) => socket.end()) + const usurper = createServer((socket) => socket.end()) + try { + const ourBind = getDaemonSocketBindPath(socketPath) + await new Promise((resolve) => ours.listen(ourBind, resolve)) + linkSync(ourBind, socketPath) + unlinkSync(ourBind) + const owned = readDaemonSocketIdentity(socketPath) + + const theirBind = join(dir, '.usurp') + await new Promise((resolve) => usurper.listen(theirBind, resolve)) + unlinkSync(socketPath) + linkSync(theirBind, socketPath) + + expect(readDaemonEndpointOwnershipState(socketPath, owned)).toBe('lost') + } finally { + await new Promise((resolve) => ours.close(() => resolve())) + await new Promise((resolve) => usurper.close(() => resolve())) + rmSync(dir, { recursive: true, force: true }) + } + } + ) +}) + describe('daemon endpoint ownership publication', () => { let dir: string let socketPath: string @@ -79,9 +115,7 @@ describe('daemon endpoint ownership publication', () => { expect(publishEndpointOwnership).toHaveBeenCalledOnce() expect(readFileSync(tokenPath, 'utf8')).toBe('previous-token') - if (process.platform !== 'win32') { - expect(existsSync(socketPath)).toBe(false) - } + await expect(connectsTo(socketPath)).resolves.toBe(false) }) it('rolls back exact PID ownership when token publication fails', async () => { @@ -111,11 +145,6 @@ describe('daemon endpoint ownership publication', () => { it.skipIf(process.platform === 'win32')( 'keeps a replacement endpoint when the daemon it replaced closes late', async () => { - // Why: this is the split-brain mechanism. libuv unlinks the pathname a server bound to - // when that server closes, with no ownership check — so a daemon exiting late used to - // delete whichever socket then sat at the canonical path. The replacement stayed alive - // hosting PTYs that no client could reach, which is what "terminals ack but never run" - // looked like from the user's seat. const replacedPidPath = join(dir, 'replaced.pid') const replaced = new DaemonServer({ socketPath, @@ -130,36 +159,28 @@ describe('daemon endpoint ownership publication', () => { }), spawnSubprocess: () => createMockSubprocess() }) - await replaced.start() + const replacement = createServer((socket) => socket.end()) + try { + await replaced.start() + const replacementBind = getDaemonSocketBindPath(socketPath) + await new Promise((resolve) => replacement.listen(replacementBind, resolve)) + renameSync(replacementBind, socketPath) + const replacementIdentity = readDaemonSocketIdentity(socketPath) - // A replacement reclaims the endpoint the way killStaleDaemon does. - unlinkSync(socketPath) - const pidPath = join(dir, 'replacement.pid') - server = new DaemonServer({ - socketPath, - tokenPath, - pidPath, - launchNonce: 'replacement-daemon', - publishEndpointOwnership: () => - publishDaemonPidFile(pidPath, { - pid: process.pid, - startedAtMs: 2_000, - launchNonce: 'replacement-daemon' - }), - spawnSubprocess: () => createMockSubprocess() - }) - await server.start() - const replacementIdentity = readDaemonSocketIdentity(socketPath) + await replaced.shutdown() - // The daemon that lost the endpoint now exits, long after the handover. - await replaced.shutdown() - - expect(existsSync(socketPath)).toBe(true) - expect(readDaemonSocketIdentity(socketPath)).toEqual(replacementIdentity) - // The replacement is still reachable through the canonical name. - await expect(connectsTo(socketPath)).resolves.toBe(true) - // The late exit also must not remove the replacement's ownership record. - expect(existsSync(pidPath)).toBe(true) + expect(readDaemonSocketIdentity(socketPath)).toEqual(replacementIdentity) + await expect(connectsTo(socketPath)).resolves.toBe(true) + } finally { + await replaced.shutdown() + await new Promise((resolve) => { + if (!replacement.listening) { + resolve() + return + } + replacement.close(() => resolve()) + }) + } } ) @@ -221,8 +242,11 @@ describe('daemon endpoint ownership publication', () => { ) it.skipIf(process.platform === 'win32')( - 'refuses a second listener when a live daemon socket path was removed', + 'refuses a duplicate launch through the exclusive PID record', async () => { + // Why the PID record and not the socket: this asserts the ownership record is exclusive, + // which is what rejects here. It does not prove a second listener was never transiently + // published — endpoint exclusivity is covered in daemon-endpoint-publish.test.ts. const pidPath = join(dir, 'daemon.pid') server = new DaemonServer({ socketPath, diff --git a/src/main/daemon/daemon-endpoint-ownership.ts b/src/main/daemon/daemon-endpoint-ownership.ts index 31e228447..eb43a1fdd 100644 --- a/src/main/daemon/daemon-endpoint-ownership.ts +++ b/src/main/daemon/daemon-endpoint-ownership.ts @@ -1,66 +1,244 @@ -/* Ownership of the daemon's canonical endpoint name and of the scratch entries the - rename-claim protocol leaves behind. Kept apart from daemon-spawner so the rule that - decides who may serve on the socket path is readable on its own. */ +/* Who may serve on the daemon's canonical endpoint name: only a daemon publishing itself onto it, + and only by replacing an entry it has itself just proven dead. Rationale and traps: AGENTS.md. */ import { randomBytes } from 'node:crypto' -import { existsSync, linkSync, readdirSync, renameSync, statSync, unlinkSync } from 'node:fs' +import { linkSync, lstatSync, renameSync, statSync, unlinkSync } from 'node:fs' import { dirname, join } from 'node:path' +import { endpointIsProvenDead, type SocketProbeOutcome } from './daemon-endpoint-probe' -/** The exact directory entry a daemon owns. Compared before any endpoint removal. */ +// Retried only when another publisher demonstrably took the name, so a small bound suffices. +const PUBLISH_ATTEMPTS = 3 + +/** A daemon endpoint, identified by its directory entry. dev+ino only; birth time is unreliable. */ export type DaemonSocketIdentity = { dev: bigint; ino: bigint } /** * A private, same-directory name to bind before publishing the canonical endpoint. * - * Why: `sockaddr_un.sun_path` caps a Unix socket path at ~104 bytes, so this must not extend - * the canonical path — it replaces the basename with a shorter one, which keeps the bind name - * strictly shorter than the endpoint the caller already requires to fit. + * `.p`, not the `.b` this used to be: released builds sweep that pattern on age alone. Replaces the + * basename rather than extending the path, so the ~104-byte `sockaddr_un.sun_path` budget holds. */ export function getDaemonSocketBindPath(socketPath: string): string { - return join(dirname(socketPath), `.b${randomBytes(5).toString('hex')}`) + return join(dirname(socketPath), `.p${randomBytes(5).toString('hex')}`) } +/** + * Only `occupied` establishes that someone else owns the endpoint, and only then may the caller + * adopt rather than fork. `lost` and `inconclusive` establish no owner, so the caller must decline. + */ +export class DaemonEndpointUnavailableError extends Error { + constructor(readonly reason: 'occupied' | 'lost' | 'inconclusive') { + super(`Daemon endpoint unavailable: ${reason}`) + this.name = 'DaemonEndpointUnavailableError' + } +} + +/** + * Stand-down signal: a live daemon owns the endpoint, so the launcher should adopt it. An exit code + * because the launcher settles its wait on process exit, which an IPC message can lose the race to. + * 20 avoids Node's reserved 1-13, where a corrupt bundle's exit would read as a false stand-down. + */ +export const DAEMON_EXIT_ENDPOINT_OCCUPIED = 20 + +/** Shared so the client's retry allowlist cannot drift from the server's refusal wording. */ +export const DAEMON_ENDPOINT_LOST_MESSAGE = 'Daemon no longer owns its endpoint; reconnect' + +export type DaemonEndpointPublishOutcome = + /** The endpoint name is ours. */ + | { status: 'published'; identity: DaemonSocketIdentity | null } + /** A live daemon owns it. Adopt that daemon; never fork beside it. */ + | { status: 'occupied' } + /** We published, and another daemon replaced us moments later. We must not serve. */ + | { status: 'lost' } + /** The incumbent could not be classified, so it must be left alone. */ + | { status: 'inconclusive' } + /** * Publishes a bound listener under the canonical endpoint name. * - * Why: Node/libuv unlinks the pathname a server bound to when that server closes, - * with no ownership check — a daemon exiting late therefore deletes whichever socket - * currently sits at that path, including a live replacement's. Binding a unique path - * and hard-linking it into place instead means libuv only ever unlinks the private - * bind name, and the exclusive link doubles as the kernel-enforced endpoint claim. + * Bind privately, then link: libuv unlinks the pathname a server bound to when it closes, with no + * ownership check, so binding a unique name means it can only ever unlink our own. `link` before + * `rename` because `rename` replaces whatever it finds — `link` fails with EEXIST and forces the + * liveness question, and on the common path it is itself the kernel-enforced exclusive claim. */ -export function publishDaemonSocketPath( +export async function publishDaemonEndpoint( boundPath: string, - canonicalPath: string -): DaemonSocketIdentity | null { + canonicalPath: string, + probeEndpoint: (path: string) => Promise +): Promise { + if (process.platform === 'win32') { + // Named pipes are exclusive by name and vanish with the process; listen is the whole protocol. + return { status: 'published', identity: null } + } + // Stat the bound name, not the canonical one: the link shares the inode, so nothing racing the + // canonical name can corrupt this reading. Failing here is cheap; startup protects nothing yet. + const identity = readDaemonSocketIdentity(boundPath) + if (!identity) { + throw new Error(`Cannot identify the bound daemon endpoint at ${boundPath}`) + } + // Losing the name mid-protocol is not an error, it just invalidates the evidence; re-run. + for (let attempt = 0; attempt < PUBLISH_ATTEMPTS; attempt++) { + try { + linkSync(boundPath, canonicalPath) + } catch (error) { + const noHardLinks = isLinkUnsupportedError(error) + if (!isFileExistsError(error) && !noHardLinks) { + throw error + } + // Without hard links we lose `link`'s exclusivity but not the death proof, the continuity + // re-check, or the post-publish verification — and that last one is what keeps replacing an + // unclaimable name safe rather than a silent overwrite. + const blocked = await replaceProvenDeadEndpoint( + boundPath, + canonicalPath, + probeEndpoint, + noHardLinks + ) + if (blocked === 'evidence-stale') { + continue + } + return blocked ?? confirmPublishedEndpoint(canonicalPath, identity) + } + try { + unlinkSync(boundPath) + } catch { + // Inert: clients resolve the canonical link, and the bind name is unique to us. + } + return confirmPublishedEndpoint(canonicalPath, identity) + } + // Inconclusive, not occupied: being outrun says the name keeps changing hands, not that anything + // is serving it — no probe ever connected. + return { status: 'inconclusive' } +} + +/** A probe that threw classified nothing, which is never proof of death. */ +async function probeEndpointSafely( + canonicalPath: string, + probeEndpoint: (path: string) => Promise +): Promise { + try { + return await probeEndpoint(canonicalPath) + } catch { + return 'unknown' + } +} + +/** + * Replaces an occupied endpoint name, but only once nothing can be serving it. `rename`, not + * unlink-then-link: the latter leaves the name absent between two calls, and every concurrent + * observer can land in that gap and conclude something false. + */ +async function replaceProvenDeadEndpoint( + boundPath: string, + canonicalPath: string, + probeEndpoint: (path: string) => Promise, + absentIsStable: boolean +): Promise { + // Captured first: `rename` replaces whatever is at the name, so without something to compare, a + // probe that stalled while another daemon published would license destroying that daemon. + const proven = readDaemonEndpointEntryIdentity(canonicalPath) + const outcome = await probeEndpointSafely(canonicalPath, probeEndpoint) + if (outcome === 'connected') { + return { status: 'occupied' } + } + if (!endpointIsProvenDead(outcome)) { + // Collapsing "could not classify" into "dead" deletes endpoints that are still serving. + return { status: 'inconclusive' } + } + if ( + !isSameEndpointEntry(proven, readDaemonEndpointEntryIdentity(canonicalPath), absentIsStable) + ) { + // The name changed hands while we probed, so the death proof describes an entry that is gone. + return 'evidence-stale' + } + // Ask again rather than compare metadata: the entry we proved dead can be unlinked and its inode + // number handed straight back to a replacement, which then matches on dev+ino and looks like + // continuity. Whether anything is *serving* is the property that matters, and connecting asks it. + const stillDead = await probeEndpointSafely(canonicalPath, probeEndpoint) + if (stillDead === 'connected') { + return { status: 'occupied' } + } + if (!endpointIsProvenDead(stillDead)) { + // Same three-way split: 'occupied' would send the launcher to adopt a daemon that may not exist. + return { status: 'inconclusive' } + } + renameSync(boundPath, canonicalPath) + // null means "the name is ours now" — the caller still has to confirm it kept it. + return null +} + +/** Same directory entry. No birth-time term — see AGENTS.md on why that field cannot carry it. */ +function isSameInode(a: DaemonSocketIdentity, b: DaemonSocketIdentity): boolean { + return a.dev === b.dev && a.ino === b.ino +} + +/** + * Absent-and-still-absent counts as unchanged; one side absent does not. The entry compared here is + * believed dead, so its inode can be freed and reissued — being wrong costs one retry here, but + * would retire a serving daemon at the ownership check, which is why only this side may use it. + */ +function isSameEndpointEntry( + a: DaemonSocketIdentity | null, + b: DaemonSocketIdentity | null, + absentIsStable: boolean +): boolean { + if (!a || !b) { + // Where an entry demonstrably existed, two unreadable stats prove nothing. Where the + // filesystem has no hard links, absent-then-absent is stable and there is nothing to destroy. + return absentIsStable && !a && !b + } + return isSameInode(a, b) +} + +/** + * Confirms the name we just took is still ours: two daemons can prove the same entry dead and both + * replace it, and the loser must never serve. dev+ino is decisive here because our own listener + * holds the inode open while we ask, so the number cannot be recycled underneath us. + */ +function confirmPublishedEndpoint( + canonicalPath: string, + identity: DaemonSocketIdentity +): DaemonEndpointPublishOutcome { + let published: DaemonSocketIdentity | null = null + try { + const stats = statSync(canonicalPath, { bigint: true }) + published = { dev: stats.dev, ino: stats.ino } + } catch (error) { + // The name is gone or unreadable, so we have no evidence we are reachable. + return isMissingFileError(error) ? { status: 'lost' } : { status: 'inconclusive' } + } + // The fresh reading: this is what the watchdog compares against, so it must describe the entry + // as it now stands, not as it was before the link or rename that published it. + return isSameInode(published, identity) + ? { status: 'published', identity: published } + : { status: 'lost' } +} + +/** + * The directory entry itself, not what it resolves to. `lstat`, because a dangling symlink occupies + * the name but `stat` follows it, fails, and reports absent — which reads as "changed hands". + */ +function readDaemonEndpointEntryIdentity(socketPath: string): DaemonSocketIdentity | null { if (process.platform === 'win32') { - // Named pipes are not directory entries; the pipe name itself is exclusive. return null } - // Why: stat the bound name first — the link shares the inode, so a racing unlink of the - // canonical name cannot erase our identity and leave the endpoint unwatched and uncleanable. - const identity = readDaemonSocketIdentity(boundPath) try { - linkSync(boundPath, canonicalPath) - } catch (error) { - if (isFileExistsError(error)) { - throw error - } - // Why: a filesystem without hard links must not stop the daemon from starting. Rename - // still moves the bind name out from under libuv, which preserves the property that - // matters most — a late close cannot delete a replacement's endpoint. Exclusivity - // degrades to check-then-act here, which is no weaker than binding the path directly. - if (existsSync(canonicalPath)) { - throw error - } - renameSync(boundPath, canonicalPath) - return identity - } - try { - unlinkSync(boundPath) + const stats = lstatSync(socketPath, { bigint: true }) + return { dev: stats.dev, ino: stats.ino } } catch { - // Inert: clients resolve the canonical link, and the bind name is unique to us. + return null } - return identity +} + +/** + * Whether `link` failed because the filesystem cannot do it at all, not because the name was taken. + * Some FUSE filesystems accept a bound socket and `rename` but refuse hard links. + */ +function isLinkUnsupportedError(error: unknown): boolean { + if (typeof error !== 'object' || error === null || !('code' in error)) { + return false + } + const code = (error as NodeJS.ErrnoException).code + return code === 'EPERM' || code === 'EOPNOTSUPP' || code === 'ENOTSUP' || code === 'ENOSYS' } function isFileExistsError(error: unknown): boolean { @@ -79,13 +257,6 @@ export function readDaemonSocketIdentity(socketPath: string): DaemonSocketIdenti } } -export function daemonSocketIdentityMatches( - a: DaemonSocketIdentity | null, - b: DaemonSocketIdentity | null -): boolean { - return a !== null && b !== null && a.dev === b.dev && a.ino === b.ino -} - /** 'indeterminate' is deliberately distinct from 'lost': only positive evidence may retire a daemon. */ export type DaemonEndpointOwnershipState = 'owned' | 'lost' | 'indeterminate' @@ -98,10 +269,11 @@ export function readDaemonEndpointOwnershipState( } try { const stats = statSync(socketPath, { bigint: true }) - return stats.dev === owned.dev && stats.ino === owned.ino ? 'owned' : 'lost' + // dev+ino suffices: `owned` is our own socket and its listener is open whenever this runs, so + // the kernel cannot recycle that number. A false loss is sticky and retires a healthy daemon. + return isSameInode({ dev: stats.dev, ino: stats.ino }, owned) ? 'owned' : 'lost' } catch (error) { - // Why: a stat that failed for any reason other than "the entry is gone" proves nothing. - // Treating EACCES or EIO as lost ownership would retire a perfectly healthy daemon. + // Anything but "the entry is gone" proves nothing; EACCES would retire a healthy daemon. return isMissingFileError(error) ? 'lost' : 'indeterminate' } } @@ -109,61 +281,3 @@ export function readDaemonEndpointOwnershipState( function isMissingFileError(error: unknown): boolean { return typeof error === 'object' && error !== null && 'code' in error && error.code === 'ENOENT' } - -/** Removes the canonical endpoint name only while it still resolves to our own listener. */ -export function unlinkOwnedDaemonSocketPath( - socketPath: string, - owned: DaemonSocketIdentity | null -): boolean { - if (process.platform === 'win32' || !owned) { - return false - } - if (!daemonSocketIdentityMatches(readDaemonSocketIdentity(socketPath), owned)) { - return false - } - try { - unlinkSync(socketPath) - return true - } catch { - return false - } -} - -const ABANDONED_DAEMON_CLAIM_PATTERN = - /(?:\.(?:cleanup|replace)-\d+-[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|^\.b[0-9a-f]{10})$/ - -const ABANDONED_DAEMON_CLAIM_MIN_AGE_MS = 60 * 60 * 1000 - -/** - * Reclaims claim/bind scratch names left behind when a rename-claim or bind publish - * could not remove its own temporary entry. Age-gated so a claim in flight is never touched. - */ -export function sweepAbandonedDaemonClaims( - runtimeDir: string, - minAgeMs = ABANDONED_DAEMON_CLAIM_MIN_AGE_MS, - now = Date.now() -): number { - let swept = 0 - let entries: string[] - try { - entries = readdirSync(runtimeDir) - } catch { - return 0 - } - for (const entry of entries) { - if (!ABANDONED_DAEMON_CLAIM_PATTERN.test(entry)) { - continue - } - const claimPath = join(runtimeDir, entry) - try { - if (now - statSync(claimPath).mtimeMs < minAgeMs) { - continue - } - unlinkSync(claimPath) - swept++ - } catch { - // Best-effort; a locked or already-removed claim is retried on a future launch. - } - } - return swept -} diff --git a/src/main/daemon/daemon-endpoint-probe.ts b/src/main/daemon/daemon-endpoint-probe.ts new file mode 100644 index 000000000..38c98580a --- /dev/null +++ b/src/main/daemon/daemon-endpoint-probe.ts @@ -0,0 +1,80 @@ +/* Classifies what, if anything, is serving the daemon's endpoint. Kept in its own module so + both the publishing daemon and the launcher can ask, without the ownership rules having to + depend on the health checker that also uses them. */ +import { connect } from 'node:net' +import { lstatSync } from 'node:fs' + +const ENDPOINT_PROBE_TIMEOUT_MS = 500 + +/** + * 'connected' — something is listening. 'missing'/'refused' — nothing is, and the endpoint + * name is safe to replace. 'unknown' — the probe itself failed (timeout on a loaded host, + * EPERM); the endpoint must be left alone because absence of proof is not proof of death. + */ +export type SocketProbeOutcome = 'connected' | 'missing' | 'refused' | 'unknown' + +/** Positive proof that nothing is serving the endpoint. A timed-out probe proves nothing. */ +export function endpointIsProvenDead(outcome: SocketProbeOutcome): boolean { + return outcome === 'refused' || outcome === 'missing' +} + +function isMissingFileError(error: unknown): boolean { + return typeof error === 'object' && error !== null && 'code' in error && error.code === 'ENOENT' +} + +export function probeSocketConnect(socketPath: string): Promise { + return new Promise((resolve) => { + let occupiedUnixEntry = false + if (process.platform !== 'win32') { + try { + // Why lstat: existsSync follows symlinks, so a dangling one reads as absent while it + // still occupies the name — an endpoint nothing can serve and no publish can take. + lstatSync(socketPath) + occupiedUnixEntry = true + } catch (error) { + resolve(isMissingFileError(error) ? 'missing' : 'unknown') + return + } + } + const sock = connect({ path: socketPath }) + let settled = false + const cleanup = (): void => { + clearTimeout(timer) + sock.off('connect', onConnect) + sock.off('error', onError) + } + const settle = (result: SocketProbeOutcome): void => { + if (settled) { + return + } + settled = true + cleanup() + resolve(result) + } + const onConnect = (): void => { + settle('connected') + sock.destroy() + } + const onError = (error: NodeJS.ErrnoException): void => { + settle( + // Why both: a non-socket occupying the name reports ENOTSOCK on macOS but + // ECONNREFUSED on Linux. Either way nothing can ever serve it. + error.code === 'ECONNREFUSED' || error.code === 'ENOTSOCK' + ? 'refused' + : error.code === 'ENOENT' + ? // An entry we just saw that now refuses to resolve is a dangling symlink: + // occupied, unservable, and replaceable — not an absent name. + occupiedUnixEntry + ? 'refused' + : 'missing' + : 'unknown' + ) + } + const timer = setTimeout(() => { + settle('unknown') + sock.destroy() + }, ENDPOINT_PROBE_TIMEOUT_MS) + sock.on('connect', onConnect) + sock.on('error', onError) + }) +} diff --git a/src/main/daemon/daemon-endpoint-publish.test.ts b/src/main/daemon/daemon-endpoint-publish.test.ts new file mode 100644 index 000000000..dc14adca3 --- /dev/null +++ b/src/main/daemon/daemon-endpoint-publish.test.ts @@ -0,0 +1,770 @@ +import type * as NodeFs from 'node:fs' +import { + linkSync, + unlinkSync, + mkdtempSync, + renameSync, + rmSync, + statSync, + symlinkSync, + writeFileSync +} from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { createConnection, createServer, type Server } from 'node:net' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + getDaemonSocketBindPath, + publishDaemonEndpoint, + readDaemonSocketIdentity +} from './daemon-endpoint-ownership' +import { probeSocketConnect } from './daemon-endpoint-probe' + +const unixIt = it.skipIf(process.platform === 'win32') + +type Listener = { server: Server; connections: () => number } + +function makeTempDir(): string { + return mkdtempSync(join(tmpdir(), 'orca-p-')) +} + +async function listen(socketPath: string): Promise { + let connectionCount = 0 + const server = createServer((socket) => { + connectionCount += 1 + socket.end() + }) + await new Promise((resolve, reject) => { + server.once('error', reject) + server.listen(socketPath, resolve) + }) + return { server, connections: () => connectionCount } +} + +async function close(server: Server): Promise { + if (!server.listening) { + return + } + await new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())) + }) +} + +async function expectReachable(socketPath: string): Promise { + await new Promise((resolve, reject) => { + const socket = createConnection(socketPath) + socket.once('connect', () => { + socket.end() + resolve() + }) + socket.once('error', reject) + }) + await new Promise((resolve) => setImmediate(resolve)) +} + +async function publishListener(boundPath: string, canonicalPath: string): Promise { + const outcome = await publishDaemonEndpoint(boundPath, canonicalPath, probeSocketConnect) + expect(outcome.status).toBe('published') +} + +afterEach(() => { + vi.doUnmock('node:fs') + vi.resetModules() +}) + +describe('publishDaemonEndpoint', () => { + unixIt('publishes a listener when the canonical endpoint is free', async () => { + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + const outcome = await publishDaemonEndpoint(boundPath, canonicalPath, probeSocketConnect) + + expect(outcome).toMatchObject({ status: 'published' }) + await expectReachable(canonicalPath) + expect(newcomer.connections()).toBe(1) + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('leaves a live incumbent in place', async () => { + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const incumbentPath = getDaemonSocketBindPath(canonicalPath) + const newcomerPath = getDaemonSocketBindPath(canonicalPath) + const incumbent = await listen(incumbentPath) + const newcomer = await listen(newcomerPath) + try { + await publishListener(incumbentPath, canonicalPath) + + const outcome = await publishDaemonEndpoint(newcomerPath, canonicalPath, probeSocketConnect) + + expect(outcome).toEqual({ status: 'occupied' }) + await expectReachable(canonicalPath) + expect(incumbent.connections()).toBe(2) + expect(newcomer.connections()).toBe(0) + } finally { + await Promise.all([close(incumbent.server), close(newcomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('replaces an incumbent that has stopped listening', async () => { + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const incumbentPath = getDaemonSocketBindPath(canonicalPath) + const newcomerPath = getDaemonSocketBindPath(canonicalPath) + const incumbent = await listen(incumbentPath) + const newcomer = await listen(newcomerPath) + try { + await publishListener(incumbentPath, canonicalPath) + await close(incumbent.server) + + const outcome = await publishDaemonEndpoint(newcomerPath, canonicalPath, probeSocketConnect) + + expect(outcome).toMatchObject({ status: 'published' }) + await expectReachable(canonicalPath) + expect(newcomer.connections()).toBe(1) + } finally { + await Promise.all([close(incumbent.server), close(newcomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt( + 'never replaces a daemon that published while the death proof was being gathered', + async () => { + // Why: the proof describes the entry that was probed, not whatever holds the name by the + // time we act on it. A publisher stalled between the two would otherwise destroy an + // established daemon it never proved dead — the original bug, reached by a narrow window. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const stalePath = getDaemonSocketBindPath(canonicalPath) + const winnerPath = getDaemonSocketBindPath(canonicalPath) + const latecomerPath = getDaemonSocketBindPath(canonicalPath) + const stale = await listen(stalePath) + const winner = await listen(winnerPath) + const latecomer = await listen(latecomerPath) + try { + // A dead entry both publishers will legitimately prove dead. + await publishListener(stalePath, canonicalPath) + await close(stale.server) + + // The winner takes the name while the latecomer is still probing the dead entry. Only on + // the first probe: the retry must see a live incumbent, which is the point. + const winnerIdentity = readDaemonSocketIdentity(winnerPath) + let raced = false + const probe = async (path: string) => { + const outcome = await probeSocketConnect(path) + if (!raced) { + raced = true + renameSync(winnerPath, canonicalPath) + } + return outcome + } + const outcome = await publishDaemonEndpoint(latecomerPath, canonicalPath, probe) + + // The latecomer must back off, and the winner must still own a reachable endpoint. + // Two connections to the winner: the latecomer's retry probe, then expectReachable. + expect(outcome).toEqual({ status: 'occupied' }) + await expectReachable(canonicalPath) + expect(winner.connections()).toBe(2) + expect(latecomer.connections()).toBe(0) + expect(readDaemonSocketIdentity(canonicalPath)).toEqual(winnerIdentity) + } finally { + await Promise.all([close(stale.server), close(winner.server), close(latecomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + } + ) + + unixIt('leaves an incumbent untouched when probing is inconclusive', async () => { + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const incumbentPath = getDaemonSocketBindPath(canonicalPath) + const newcomerPath = getDaemonSocketBindPath(canonicalPath) + const incumbent = await listen(incumbentPath) + const newcomer = await listen(newcomerPath) + try { + await publishListener(incumbentPath, canonicalPath) + const before = statSync(canonicalPath, { bigint: true }) + const probe = vi.fn(async () => 'unknown' as const) + + const outcome = await publishDaemonEndpoint(newcomerPath, canonicalPath, probe) + + const after = statSync(canonicalPath, { bigint: true }) + expect(outcome).toEqual({ status: 'inconclusive' }) + expect(probe).toHaveBeenCalledWith(canonicalPath) + expect({ dev: after.dev, ino: after.ino }).toEqual({ dev: before.dev, ino: before.ino }) + await expectReachable(canonicalPath) + expect(incumbent.connections()).toBe(1) + expect(newcomer.connections()).toBe(0) + } finally { + await Promise.all([close(incumbent.server), close(newcomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('replaces a regular file occupying the endpoint', async () => { + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + writeFileSync(canonicalPath, 'stale') + + const outcome = await publishDaemonEndpoint(boundPath, canonicalPath, probeSocketConnect) + + expect(outcome).toMatchObject({ status: 'published' }) + await expectReachable(canonicalPath) + expect(newcomer.connections()).toBe(1) + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('replaces a dangling symlink occupying the endpoint', async () => { + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + symlinkSync(join(directory, 'x'), canonicalPath) + + const outcome = await publishDaemonEndpoint(boundPath, canonicalPath, probeSocketConnect) + + expect(outcome).toMatchObject({ status: 'published' }) + await expectReachable(canonicalPath) + expect(newcomer.connections()).toBe(1) + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('treats a probe that throws as inconclusive rather than as proof of death', async () => { + // Why: a probe that failed classified nothing. Letting a thrown error fall through to the + // dead branch would replace an endpoint that may well be serving. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const incumbentPath = getDaemonSocketBindPath(canonicalPath) + const newcomerPath = getDaemonSocketBindPath(canonicalPath) + const incumbent = await listen(incumbentPath) + const newcomer = await listen(newcomerPath) + try { + await publishListener(incumbentPath, canonicalPath) + const before = readDaemonSocketIdentity(canonicalPath) + const probe = vi.fn(async () => { + throw new Error('probe blew up') + }) + + const outcome = await publishDaemonEndpoint(newcomerPath, canonicalPath, probe) + + expect(outcome).toEqual({ status: 'inconclusive' }) + expect(readDaemonSocketIdentity(canonicalPath)).toEqual(before) + await expectReachable(canonicalPath) + expect(newcomer.connections()).toBe(0) + } finally { + await Promise.all([close(incumbent.server), close(newcomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('still publishes on a filesystem that refuses hard links', async () => { + // Why: some POSIX and FUSE filesystems accept a bound Unix socket and rename but reject + // hard links. Requiring the link would mean no daemon persistence at all there, which is a + // capability the previous implementation had. Replacing is safe here only because the + // post-publish verification exists to catch a loser — it did not when this was first removed. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + linkSync: () => { + throw Object.assign(new Error('injected EPERM'), { code: 'EPERM' }) + } + } + }) + vi.resetModules() + const { publishDaemonEndpoint: publishWithoutLinks } = + await import('./daemon-endpoint-ownership') + + const outcome = await publishWithoutLinks(boundPath, canonicalPath, probeSocketConnect) + + expect(outcome).toMatchObject({ status: 'published' }) + await expectReachable(canonicalPath) + expect(newcomer.connections()).toBe(1) + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('does not fall back to replacing when a link fails for another reason', async () => { + // Why: only "this filesystem cannot do hard links" licenses giving up link's exclusivity. + // An ENOSPC or EIO must surface, not silently downgrade to a replace. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + linkSync: () => { + throw Object.assign(new Error('injected EIO'), { code: 'EIO' }) + } + } + }) + vi.resetModules() + const { publishDaemonEndpoint: publishWithBrokenLink } = + await import('./daemon-endpoint-ownership') + + await expect( + publishWithBrokenLink(boundPath, canonicalPath, probeSocketConnect) + ).rejects.toMatchObject({ code: 'EIO' }) + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('will not replace an occupied entry whose continuity it could not read', async () => { + // Why: every stat failure collapses to null, so two unreadable reads bracketing a positive + // death probe would otherwise compare equal and authorise a rename with no evidence the + // entry is still the one proven dead. The probe is injected here so the only entry reads + // are the continuity ones, which makes the failure unambiguous. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const deadBind = getDaemonSocketBindPath(canonicalPath) + const newcomerPath = getDaemonSocketBindPath(canonicalPath) + const dead = await listen(deadBind) + const newcomer = await listen(newcomerPath) + try { + await publishListener(deadBind, canonicalPath) + await close(dead.server) + const occupant = statSync(canonicalPath, { bigint: true }) + + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + lstatSync: (target: string, options?: { bigint?: boolean }) => { + if (target === canonicalPath) { + throw Object.assign(new Error('injected EIO'), { code: 'EIO' }) + } + return actual.lstatSync(target, options as never) + } + } + }) + vi.resetModules() + const { publishDaemonEndpoint: publishBlind } = await import('./daemon-endpoint-ownership') + + const outcome = await publishBlind(newcomerPath, canonicalPath, async () => 'refused') + + expect(outcome).toEqual({ status: 'inconclusive' }) + const after = statSync(canonicalPath, { bigint: true }) + expect({ dev: after.dev, ino: after.ino }).toEqual({ dev: occupant.dev, ino: occupant.ino }) + } finally { + await Promise.all([close(dead.server), close(newcomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('records an identity the ownership watchdog can still match afterwards', async () => { + // Why this matters and why no ordinary test catches it: the recorded identity is what the + // watchdog later compares the entry against, and that comparison includes birthtimeMs. Node + // documents birthtimeMs as sometimes holding the ctime instead — libuv fills it from st_ctim + // on Linux kernels without statx. link and rename both bump ctime, so an identity read + // BEFORE publishing could never match the entry again on such a host: the daemon would + // declare itself lost on its first session and stand down permanently. Simulated here by + // reporting ctime as birthtime, which is exactly what those platforms do. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + // What a host whose birth time is really the ctime reports. + const asCtimeBirthtime = (stats: { ctimeMs: number; ctimeNs?: bigint }) => ({ + ...stats, + birthtimeMs: stats.ctimeMs, + ...(stats.ctimeNs === undefined ? {} : { birthtimeNs: stats.ctimeNs }) + }) + return { + ...actual, + statSync: (target: string, options?: { bigint?: boolean }) => + asCtimeBirthtime(actual.statSync(target, options as never) as never), + lstatSync: (target: string, options?: { bigint?: boolean }) => + asCtimeBirthtime(actual.lstatSync(target, options as never) as never) + } + }) + vi.resetModules() + const { publishDaemonEndpoint: publishOnCtimeFs, readDaemonEndpointOwnershipState } = + await import('./daemon-endpoint-ownership') + + const outcome = await publishOnCtimeFs(boundPath, canonicalPath, probeSocketConnect) + expect(outcome).toMatchObject({ status: 'published' }) + + // The watchdog must still recognise the endpoint as ours. + const owned = (outcome as { identity: unknown }).identity + expect(readDaemonEndpointOwnershipState(canonicalPath, owned as never)).toBe('owned') + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('will not rename over a live daemon indistinguishable by directory entry', async () => { + // The case the re-probe exists for: the entry proved dead is unlinked and its inode number + // handed straight back to a replacement, so the continuity check sees the same dev+ino and + // cannot tell them apart. Birth time was meant to separate them and cannot be relied on — + // it may be the ctime, the epoch, or coarser than the events it must separate. Recycling + // cannot be provoked on demand, so identity is pinned to a constant here, which is exactly + // what a recycled inode number looks like to this code. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const deadBind = getDaemonSocketBindPath(canonicalPath) + const livePath = getDaemonSocketBindPath(canonicalPath) + const latecomerPath = getDaemonSocketBindPath(canonicalPath) + const dead = await listen(deadBind) + const live = await listen(livePath) + const latecomer = await listen(latecomerPath) + try { + await publishListener(deadBind, canonicalPath) + await close(dead.server) + const frozen = statSync(canonicalPath, { bigint: true }) + + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + lstatSync: (target: string, options?: { bigint?: boolean }) => + target === canonicalPath ? frozen : actual.lstatSync(target, options as never) + } + }) + vi.resetModules() + const { publishDaemonEndpoint: publishAgainstRecycled } = + await import('./daemon-endpoint-ownership') + + let swapped = false + const probe = async (path: string) => { + const outcome = await probeSocketConnect(path) + if (!swapped) { + swapped = true + unlinkSync(canonicalPath) + linkSync(livePath, canonicalPath) + } + return outcome + } + + const outcome = await publishAgainstRecycled(latecomerPath, canonicalPath, probe) + + // Identity says unchanged; something is serving, so it must not be replaced. + expect(outcome).toEqual({ status: 'occupied' }) + await expectReachable(canonicalPath) + expect(latecomer.connections()).toBe(0) + } finally { + await Promise.all([close(dead.server), close(live.server), close(latecomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('declines rather than claims an owner when the second probe proves nothing', async () => { + // Why: the probe taken immediately before replacing has exactly the authority of the first + // one. A timeout or an EPERM there proves no live incumbent, and reporting 'occupied' would + // send the launcher off to adopt a daemon that may not exist. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const deadBind = getDaemonSocketBindPath(canonicalPath) + const newcomerPath = getDaemonSocketBindPath(canonicalPath) + const dead = await listen(deadBind) + const newcomer = await listen(newcomerPath) + try { + await publishListener(deadBind, canonicalPath) + await close(dead.server) + const deadEntry = readDaemonSocketIdentity(canonicalPath) + + // Dead on the first ask, unclassifiable on the second. + let asked = 0 + const probe = async () => { + asked += 1 + return asked === 1 ? ('refused' as const) : ('unknown' as const) + } + + const outcome = await publishDaemonEndpoint(newcomerPath, canonicalPath, probe) + + expect(outcome).toEqual({ status: 'inconclusive' }) + expect(readDaemonSocketIdentity(canonicalPath)).toEqual(deadEntry) + expect(newcomer.connections()).toBe(0) + } finally { + await Promise.all([close(dead.server), close(newcomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('declines on an unclassifiable first probe even if the second proves death', async () => { + // Why pinned: the second probe re-establishes death immediately before the rename, so the + // first probe's guard is no longer what makes replacing safe — it is what keeps the protocol + // conservative. Without it an endpoint that could not be classified at all would still be + // replaced, on the strength of one later reading. Declining on any doubt is the contract. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const deadBind = getDaemonSocketBindPath(canonicalPath) + const newcomerPath = getDaemonSocketBindPath(canonicalPath) + const dead = await listen(deadBind) + const newcomer = await listen(newcomerPath) + try { + await publishListener(deadBind, canonicalPath) + await close(dead.server) + const deadEntry = readDaemonSocketIdentity(canonicalPath) + + // Unclassifiable first, decisively dead second. + let asked = 0 + const probe = async () => { + asked += 1 + return asked === 1 ? ('unknown' as const) : ('refused' as const) + } + + const outcome = await publishDaemonEndpoint(newcomerPath, canonicalPath, probe) + + expect(outcome).toEqual({ status: 'inconclusive' }) + expect(readDaemonSocketIdentity(canonicalPath)).toEqual(deadEntry) + expect(newcomer.connections()).toBe(0) + } finally { + await Promise.all([close(dead.server), close(newcomer.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('refuses to serve a bound endpoint it cannot identify', async () => { + // Why: without the bound identity we can neither verify the publish nor arm the ownership + // watchdog, so we would serve a name we could never check. Startup has nothing to protect. + const directory = makeTempDir() + try { + await expect( + publishDaemonEndpoint( + join(directory, '.bmissing'), + join(directory, 'd'), + probeSocketConnect + ) + ).rejects.toThrow(/Cannot identify the bound daemon endpoint/) + } finally { + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('reports lost when the name it took disappears before verification', async () => { + // Why not 'published': the entry we took is gone, so nothing resolves to this listener. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + unlinkSync: (target: string) => { + actual.unlinkSync(target) + if (target === boundPath) { + actual.unlinkSync(canonicalPath) + } + } + } + }) + vi.resetModules() + const { publishDaemonEndpoint: publishWithRemover } = + await import('./daemon-endpoint-ownership') + + await expect( + publishWithRemover(boundPath, canonicalPath, probeSocketConnect) + ).resolves.toEqual({ status: 'lost' }) + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('declines rather than publishes when the endpoint cannot be verified', async () => { + // Why fail closed: an unreadable canonical entry is not evidence we are reachable, and a + // starting daemon loses nothing by declining. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + // Why inject the failure directly rather than dropping directory permissions: a + // permission-based setup induces nothing when the suite runs as root, and the test would + // then pass without ever reaching the branch it names. + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + let published = false + return { + ...actual, + unlinkSync: (target: string) => { + actual.unlinkSync(target) + if (target === boundPath) { + published = true + } + }, + statSync: (target: string, options?: { bigint?: boolean }) => { + if (published && target === canonicalPath) { + throw Object.assign(new Error('injected EACCES'), { code: 'EACCES' }) + } + return actual.statSync(target, options as never) + } + } + }) + vi.resetModules() + const { publishDaemonEndpoint: publishWithBlockedStat } = + await import('./daemon-endpoint-ownership') + + await expect( + publishWithBlockedStat(boundPath, canonicalPath, probeSocketConnect) + ).resolves.toEqual({ status: 'inconclusive' }) + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('reports lost when another listener replaces it before verification', async () => { + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const competitorPath = getDaemonSocketBindPath(canonicalPath) + const competitorLink = join(directory, '.r') + const newcomer = await listen(boundPath) + const competitor = await listen(competitorPath) + try { + writeFileSync(canonicalPath, 'stale') + vi.doMock('node:fs', async () => { + const actual = await vi.importActual('node:fs') + return { + ...actual, + renameSync: (source: string, destination: string) => { + actual.renameSync(source, destination) + if (source === boundPath && destination === canonicalPath) { + actual.renameSync(competitorLink, canonicalPath) + } + } + } + }) + vi.resetModules() + const { publishDaemonEndpoint: publishWithRacer } = + await import('./daemon-endpoint-ownership') + // Why the competitor only takes the name from inside our own rename: publishing during + // the probe is caught earlier now, by the pre-rename evidence check. 'lost' is + // specifically the window between taking the name and verifying we still hold it. + // Idempotent: the protocol probes again immediately before replacing, so this runs twice. + let linked = false + const probe = async () => { + if (!linked) { + linked = true + linkSync(competitorPath, competitorLink) + } + return 'refused' as const + } + + const outcome = await publishWithRacer(boundPath, canonicalPath, probe) + + expect(outcome).toEqual({ status: 'lost' }) + await expectReachable(canonicalPath) + expect(competitor.connections()).toBe(1) + expect(newcomer.connections()).toBe(0) + } finally { + await Promise.all([close(newcomer.server), close(competitor.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('propagates link errors other than EEXIST', async () => { + const directory = makeTempDir() + const boundPath = join(directory, '.b') + const canonicalPath = join(directory, 'x', 'd') + const newcomer = await listen(boundPath) + try { + const probe = vi.fn(async () => 'missing' as const) + + await expect(publishDaemonEndpoint(boundPath, canonicalPath, probe)).rejects.toMatchObject({ + code: 'ENOENT' + }) + expect(probe).not.toHaveBeenCalled() + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('leaves the published endpoint behind when its own owner closes', async () => { + // Why this and not a "late close" staging: closing is what makes an incumbent replaceable, + // so the two cannot be ordered against each other at this level. The property that made the + // original bug possible is testable directly — libuv unlinks the pathname a server BOUND + // to, so a daemon that published by linking a private name must leave the canonical entry + // intact when it closes. Ordering a close after a replacement publishes needs the full + // lifecycle and is covered in daemon-endpoint-ownership.test.ts. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const incumbentPath = getDaemonSocketBindPath(canonicalPath) + const replacementPath = getDaemonSocketBindPath(canonicalPath) + const incumbent = await listen(incumbentPath) + const replacement = await listen(replacementPath) + try { + const published = await publishDaemonEndpoint( + incumbentPath, + canonicalPath, + probeSocketConnect + ) + expect(published).toMatchObject({ status: 'published' }) + + await close(incumbent.server) + + // The entry survives its owner's close — dead, but still the name to be replaced. + expect(readDaemonSocketIdentity(canonicalPath)).not.toBeNull() + await expect(probeSocketConnect(canonicalPath)).resolves.toBe('refused') + + const outcome = await publishDaemonEndpoint( + replacementPath, + canonicalPath, + probeSocketConnect + ) + expect(outcome).toMatchObject({ status: 'published' }) + await expectReachable(canonicalPath) + expect(replacement.connections()).toBe(1) + } finally { + await Promise.all([close(incumbent.server), close(replacement.server)]) + rmSync(directory, { recursive: true, force: true }) + } + }) + + unixIt('returns the identity that actually holds the canonical name', async () => { + // Why: callers arm the ownership watchdog with this value, so an identity that does not + // describe the published entry makes every later ownership check meaningless. + const directory = makeTempDir() + const canonicalPath = join(directory, 'd') + const boundPath = getDaemonSocketBindPath(canonicalPath) + const newcomer = await listen(boundPath) + try { + const outcome = await publishDaemonEndpoint(boundPath, canonicalPath, probeSocketConnect) + + expect(outcome.status).toBe('published') + expect(outcome.status === 'published' ? outcome.identity : null).toEqual( + readDaemonSocketIdentity(canonicalPath) + ) + } finally { + await close(newcomer.server) + rmSync(directory, { recursive: true, force: true }) + } + }) +}) diff --git a/src/main/daemon/daemon-endpoint-reachability-test-harness.ts b/src/main/daemon/daemon-endpoint-reachability-test-harness.ts new file mode 100644 index 000000000..6298a58f6 --- /dev/null +++ b/src/main/daemon/daemon-endpoint-reachability-test-harness.ts @@ -0,0 +1,22 @@ +/* Asks the question a user would: can anything actually be reached at the endpoint? + Tests must not use file existence for this — a shut-down daemon deliberately leaves its + socket entry behind for the next publisher to replace. */ +import { probeSocketConnect } from './daemon-endpoint-probe' + +const ENDPOINT_UNREACHABLE_TIMEOUT_MS = 2_000 +const ENDPOINT_POLL_MS = 20 + +/** + * Why reuse the production probe: it is the same classifier the daemon publishes against, so a + * test cannot drift from what the daemon itself treats as a reachable endpoint. + */ +export async function waitForEndpointUnreachable(socketPath: string): Promise { + const deadline = Date.now() + ENDPOINT_UNREACHABLE_TIMEOUT_MS + while ((await probeSocketConnect(socketPath)) === 'connected') { + if (Date.now() >= deadline) { + return false + } + await new Promise((resolve) => setTimeout(resolve, ENDPOINT_POLL_MS)) + } + return true +} diff --git a/src/main/daemon/daemon-endpoint-windows.test.ts b/src/main/daemon/daemon-endpoint-windows.test.ts new file mode 100644 index 000000000..08f40df5b --- /dev/null +++ b/src/main/daemon/daemon-endpoint-windows.test.ts @@ -0,0 +1,78 @@ +/* The win32 branches of endpoint ownership, exercised on any host. + Every other endpoint test is skipIf(win32), so these paths had no coverage anywhere: on POSIX + they are skipped by the guard, and PR CI never runs the suite on Windows at all. What they must + guarantee is that the whole mechanism is inert on named pipes — the pipe name is already + exclusive and a dead daemon's pipe ceases to exist, so a successful listen is the protocol. */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { + publishDaemonEndpoint, + readDaemonEndpointOwnershipState, + readDaemonSocketIdentity +} from './daemon-endpoint-ownership' + +describe('daemon endpoint ownership on win32', () => { + const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform') + let dir: string + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'daemon-endpoint-win32-')) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + }) + + afterEach(() => { + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform) + } + rmSync(dir, { recursive: true, force: true }) + }) + + it('publishes without probing or touching the filesystem', async () => { + const boundPath = join(dir, 'bound.sock') + const canonicalPath = join(dir, 'daemon.sock') + writeFileSync(boundPath, '') + const probe = vi.fn() + + const outcome = await publishDaemonEndpoint(boundPath, canonicalPath, probe) + + expect(outcome).toEqual({ status: 'published', identity: null }) + // A pipe name cannot be link/renamed, and probing one we are about to bind proves nothing. + expect(probe).not.toHaveBeenCalled() + expect(existsSync(canonicalPath)).toBe(false) + }) + + it('publishes even where the canonical name is already taken', async () => { + const boundPath = join(dir, 'bound.sock') + const canonicalPath = join(dir, 'daemon.sock') + writeFileSync(boundPath, '') + writeFileSync(canonicalPath, 'someone else') + const probe = vi.fn() + + // Why this is safe rather than the split brain it would be on POSIX: on Windows the daemon + // binds the canonical pipe name directly, so listen() has already failed if one is taken. + // Reaching here means we hold it. + await expect(publishDaemonEndpoint(boundPath, canonicalPath, probe)).resolves.toEqual({ + status: 'published', + identity: null + }) + expect(probe).not.toHaveBeenCalled() + }) + + it('reports no identity, so nothing can later claim ownership was lost', () => { + const realFile = join(dir, 'daemon.sock') + writeFileSync(realFile, '') + + expect(readDaemonSocketIdentity(realFile)).toBeNull() + }) + + it('never reports lost ownership, so a daemon is never retired on Windows', () => { + const missing = join(dir, 'gone.sock') + + // 'indeterminate' and not 'lost' is what keeps the watchdog and the session guard inert: a + // named pipe has no directory entry to compare, so absence here is not evidence of takeover. + expect(readDaemonEndpointOwnershipState(missing, null)).toBe('indeterminate') + expect(readDaemonEndpointOwnershipState(missing, { dev: 1n, ino: 2n })).toBe('indeterminate') + }) +}) diff --git a/src/main/daemon/daemon-entry.ts b/src/main/daemon/daemon-entry.ts index 9ad6b1564..addfa50b1 100644 --- a/src/main/daemon/daemon-entry.ts +++ b/src/main/daemon/daemon-entry.ts @@ -13,6 +13,10 @@ import { warmWindowsConptyOnce } from './windows-conpty-warmup' import { warmPwshAvailabilityCache } from '../pwsh' import { createDaemonFileLog, createNoopDaemonFileLog } from './daemon-file-log' import { PROTOCOL_VERSION } from './types' +import { + DAEMON_EXIT_ENDPOINT_OCCUPIED, + DaemonEndpointUnavailableError +} from './daemon-endpoint-ownership' import { prepareMacosTccLoginShell, probeMacosLoginSessionAlive @@ -328,6 +332,14 @@ const isDirectExecution = !process.env.VITEST if (isDirectExecution) { main().catch((err) => { console.error('[daemon] Fatal:', err) + if (err instanceof DaemonEndpointUnavailableError && err.reason === 'occupied') { + // Why an exit code and not the IPC message: process.send only proves the write left this + // process, not that the parent dispatched 'message' before it observed the exit — and the + // parent settles the launch on exit. A code rides the same event that ends the wait, so it + // cannot lose that race. The message is still sent best-effort for log detail. + process.send?.({ type: 'endpoint-unavailable', reason: err.reason }) + process.exit(DAEMON_EXIT_ENDPOINT_OCCUPIED) + } process.exit(1) }) } diff --git a/src/main/daemon/daemon-health-endpoint-entry.test.ts b/src/main/daemon/daemon-health-endpoint-entry.test.ts new file mode 100644 index 000000000..d75e8eedc --- /dev/null +++ b/src/main/daemon/daemon-health-endpoint-entry.test.ts @@ -0,0 +1,39 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { lstatSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { killStaleDaemon } from './daemon-health' + +describe.skipIf(process.platform === 'win32')('killStaleDaemon endpoint entries', () => { + let dir: string + let socketPath: string + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'daemon-endpoint-entry-')) + socketPath = join(dir, 'daemon.sock') + }) + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }) + }) + + it('leaves a regular file for the publishing daemon to replace', async () => { + writeFileSync(socketPath, 'not a socket') + + await expect(killStaleDaemon(dir, socketPath, join(dir, 'daemon.token'))).resolves.toEqual({ + killed: false, + liveOwnerSurvived: false + }) + expect(readFileSync(socketPath, 'utf8')).toBe('not a socket') + }) + + it('leaves a dangling symlink for the publishing daemon to replace', async () => { + symlinkSync(join(dir, 'missing-target'), socketPath) + + await expect(killStaleDaemon(dir, socketPath, join(dir, 'daemon.token'))).resolves.toEqual({ + killed: false, + liveOwnerSurvived: false + }) + expect(lstatSync(socketPath).isSymbolicLink()).toBe(true) + }) +}) diff --git a/src/main/daemon/daemon-health-socket-cleanup.test.ts b/src/main/daemon/daemon-health-socket-cleanup.test.ts index 072944108..48e548bff 100644 --- a/src/main/daemon/daemon-health-socket-cleanup.test.ts +++ b/src/main/daemon/daemon-health-socket-cleanup.test.ts @@ -79,6 +79,7 @@ describe('daemon health socket listener cleanup', () => { const result = killStaleDaemon(dir, socketPath, tokenPath) await vi.advanceTimersByTimeAsync(500) + // The publisher re-probes before replacing, so an unknown launcher hint can allow a fork. await expect(result).resolves.toEqual({ killed: false, liveOwnerSurvived: false }) expect(socket.listenerCount('connect')).toBe(0) expect(socket.listenerCount('error')).toBe(0) diff --git a/src/main/daemon/daemon-health.test.ts b/src/main/daemon/daemon-health.test.ts index 8d2ae9db1..287225058 100644 --- a/src/main/daemon/daemon-health.test.ts +++ b/src/main/daemon/daemon-health.test.ts @@ -1,11 +1,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + import { spawn } from 'node:child_process' -import { existsSync, linkSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { basename, join } from 'node:path' -import { createServer, connect, Socket, type Server } from 'node:net' +import { createServer, connect, type Server } from 'node:net' import { DaemonServer } from './daemon-server' import { getDaemonPidPath, serializeDaemonPidFile } from './daemon-spawner' +import type { SocketProbeOutcome } from './daemon-endpoint-probe' import { checkDaemonHealth, E2E_FORCE_DAEMON_HEALTH_UNREACHABLE_ENV, @@ -168,30 +170,29 @@ describe('daemon health', () => { } }) - it('does not unlink a live socket when the pid file does not match this daemon', async () => { - if (process.platform === 'win32') { - return - } - - const server = createServer((socket) => socket.end()) - await new Promise((resolve, reject) => { - server.once('error', reject) - server.listen(socketPath, () => { - server.off('error', reject) - resolve() + it.skipIf(process.platform === 'win32')( + 'does not unlink a live socket when the pid file does not match this daemon', + async () => { + const server = createServer((socket) => socket.end()) + await new Promise((resolve, reject) => { + server.once('error', reject) + server.listen(socketPath, () => { + server.off('error', reject) + resolve() + }) }) - }) - writeFileSync(getDaemonPidPath(dir), String(process.pid), { mode: 0o600 }) + writeFileSync(getDaemonPidPath(dir), String(process.pid), { mode: 0o600 }) - try { - await expect(killStaleDaemon(dir, socketPath, tokenPath)).resolves.toMatchObject({ - killed: false - }) - await expect(canConnect(socketPath)).resolves.toBe(true) - } finally { - await closeServer(server) + try { + await expect(killStaleDaemon(dir, socketPath, tokenPath)).resolves.toMatchObject({ + killed: false + }) + await expect(canConnect(socketPath)).resolves.toBe(true) + } finally { + await closeServer(server) + } } - }) + ) }) describe('parseDaemonPidFile', () => { @@ -353,10 +354,7 @@ describe('startTimeMatches', () => { expect(startTimeMatches(process.pid, actual + 500)).toBe(true) }) - it('returns false for start times outside tolerance', () => { - if (process.platform === 'win32') { - return - } + it.skipIf(process.platform === 'win32')('returns false for start times outside tolerance', () => { const actual = getProcessStartedAtMs(process.pid) if (actual === null) { return @@ -417,41 +415,40 @@ describe('killStaleDaemon pid identity guards', () => { rmSync(dir, { recursive: true, force: true }) }) - it('does not SIGTERM when the saved startedAtMs mismatches the current process', async () => { - if (process.platform === 'win32') { - return - } - - // Why: seed a pid file that claims the daemon is `process.pid` (us) but - // was started 1 hour ago. Our real start time is "now," so startTimeMatches - // returns false and isDaemonProcess rejects. killStaleDaemon must not call - // process.kill in that case. - const bogusStartedAtMs = Date.now() - 60 * 60 * 1000 - writeFileSync( - getDaemonPidPath(dir), - serializeDaemonPidFile({ pid: process.pid, startedAtMs: bogusStartedAtMs }), - { mode: 0o600 } - ) - - // isDaemonProcess uses process.kill(pid, 0) as a liveness probe; that's - // expected and not a real kill. We only care that no actual termination - // signal is sent. - const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) - try { - await expect(killStaleDaemon(dir, socketPath, tokenPath)).resolves.toMatchObject({ - killed: false - }) - const terminationSignals = killSpy.mock.calls.filter( - ([, sig]) => sig === 'SIGTERM' || sig === 'SIGKILL' + it.skipIf(process.platform === 'win32')( + 'does not SIGTERM when the saved startedAtMs mismatches the current process', + async () => { + // Why: seed a pid file that claims the daemon is `process.pid` (us) but + // was started 1 hour ago. Our real start time is "now," so startTimeMatches + // returns false and isDaemonProcess rejects. killStaleDaemon must not call + // process.kill in that case. + const bogusStartedAtMs = Date.now() - 60 * 60 * 1000 + writeFileSync( + getDaemonPidPath(dir), + serializeDaemonPidFile({ pid: process.pid, startedAtMs: bogusStartedAtMs }), + { mode: 0o600 } ) - expect(terminationSignals).toEqual([]) - } finally { - killSpy.mockRestore() + + // isDaemonProcess uses process.kill(pid, 0) as a liveness probe; that's + // expected and not a real kill. We only care that no actual termination + // signal is sent. + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + try { + await expect(killStaleDaemon(dir, socketPath, tokenPath)).resolves.toMatchObject({ + killed: false + }) + const terminationSignals = killSpy.mock.calls.filter( + ([, sig]) => sig === 'SIGTERM' || sig === 'SIGKILL' + ) + expect(terminationSignals).toEqual([]) + } finally { + killSpy.mockRestore() + } } - }) + ) }) -describe('killStaleDaemon endpoint reclamation', () => { +describe('killStaleDaemon ownership decisions', () => { let dir: string let socketPath: string let tokenPath: string @@ -476,99 +473,123 @@ describe('killStaleDaemon endpoint reclamation', () => { }) } - it('preserves the pid record and the endpoint when the owner cannot be proven gone', async () => { - if (process.platform === 'win32') { - return + it.each<{ outcome: SocketProbeOutcome; liveOwnerSurvived: boolean }>([ + { outcome: 'connected', liveOwnerSurvived: true }, + { outcome: 'refused', liveOwnerSurvived: false }, + { outcome: 'missing', liveOwnerSurvived: false }, + { outcome: 'unknown', liveOwnerSurvived: false } + ])( + 'reports liveOwnerSurvived=$liveOwnerSurvived for a $outcome endpoint', + async ({ outcome, liveOwnerSurvived }) => { + const probeEndpoint = vi.fn(async () => outcome) + + await expect( + killStaleDaemon(dir, socketPath, tokenPath, undefined, { probeEndpoint }) + ).resolves.toEqual({ killed: false, liveOwnerSurvived }) + expect(probeEndpoint).toHaveBeenCalledOnce() + expect(probeEndpoint).toHaveBeenCalledWith(socketPath) } + ) - // Why: isDaemonProcess matches on the command line, so a child carrying the daemon - // entry plus this endpoint's paths is adopted as the recorded owner by the first probe. - const child = spawn( - process.execPath, - ['-e', 'setInterval(() => {}, 1000)', 'daemon-entry', socketPath, tokenPath], - { stdio: 'ignore' } - ) - await new Promise((resolve, reject) => { - child.once('spawn', resolve) - child.once('error', reject) + it('preserves a daemon that cannot be proven dead', async () => { + const record = serializeDaemonPidFile({ + pid: process.pid, + startedAtMs: null, + launchNonce: 'live-owner' + }) + writeFileSync(getDaemonPidPath(dir), record, { mode: 0o600 }) + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }) }) - const childPid = child.pid as number - const childExited = new Promise((resolve) => child.once('exit', () => resolve())) - const server = createServer((socket) => socket.end()) - await listenOnSocketPath(server, socketPath) - writeFileSync( - getDaemonPidPath(dir), - serializeDaemonPidFile({ pid: childPid, startedAtMs: null }), - { mode: 0o600 } - ) - const realKill = process.kill.bind(process) - const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) - // Why: the recorded owner vanishes mid-wait, so the pre-SIGKILL identity re-probe - // fails and only the still-answering endpoint proves a daemon is alive. - const identityBreaker = setTimeout(() => realKill(childPid, 'SIGKILL'), 800) try { - await expect(killStaleDaemon(dir, socketPath, tokenPath)).resolves.toEqual({ - killed: false, - liveOwnerSurvived: true - }) - expect(existsSync(getDaemonPidPath(dir))).toBe(true) - expect(existsSync(socketPath)).toBe(true) - await expect(canConnect(socketPath)).resolves.toBe(true) + await expect( + killStaleDaemon(dir, socketPath, tokenPath, undefined, { + probeEndpoint: async () => 'unknown' + }) + ).resolves.toEqual({ killed: false, liveOwnerSurvived: true }) + expect(readFileSync(getDaemonPidPath(dir), 'utf8')).toBe(record) } finally { - clearTimeout(identityBreaker) killSpy.mockRestore() - try { - realKill(childPid, 'SIGKILL') - } catch { - // Already gone. - } - await childExited - await closeServer(server) } }) - it('unlinks a stale endpoint file once connects to it are refused', async () => { - if (process.platform === 'win32') { - return - } + it('removes a malformed pid record', async () => { + writeFileSync(getDaemonPidPath(dir), '{truncated', { mode: 0o600 }) - // Why: hard-linking the bound inode leaves the endpoint name behind after the - // listener closes — exactly the entry a crashed daemon leaves for connect to refuse. - const bindPath = join(dir, '.bstale') - const server = createServer((socket) => socket.end()) - await listenOnSocketPath(server, bindPath) - linkSync(bindPath, socketPath) - await closeServer(server) - - await expect(killStaleDaemon(dir, socketPath, tokenPath)).resolves.toEqual({ - killed: false, - liveOwnerSurvived: false + await killStaleDaemon(dir, socketPath, tokenPath, undefined, { + probeEndpoint: async () => 'missing' }) - expect(existsSync(socketPath)).toBe(false) + + expect(existsSync(getDaemonPidPath(dir))).toBe(false) }) - it('keeps the endpoint file when the connect probe neither connects nor is refused', async () => { - if (process.platform === 'win32') { - return - } + it('removes a legacy bare-integer pid record', async () => { + writeFileSync(getDaemonPidPath(dir), String(999_999), { mode: 0o600 }) - writeFileSync(socketPath, '') - // A connect that never settles leaves the probe to time out, which is not proof - // the endpoint is dead — net.connect itself cannot be spied on in ESM. - const connectSpy = vi - .spyOn(Socket.prototype, 'connect') - .mockImplementation(function (this: Socket) { - return this - }) - try { - await expect(killStaleDaemon(dir, socketPath, tokenPath)).resolves.toEqual({ - killed: false, - liveOwnerSurvived: false - }) - expect(existsSync(socketPath)).toBe(true) - } finally { - connectSpy.mockRestore() - } + await killStaleDaemon(dir, socketPath, tokenPath, undefined, { + probeEndpoint: async () => 'missing' + }) + + expect(existsSync(getDaemonPidPath(dir))).toBe(false) }) + + it.skipIf(process.platform === 'win32')( + "leaves a replacement's pid record after killing the recorded owner", + async () => { + const child = spawn( + process.execPath, + [ + '-e', + "process.on('SIGTERM', () => {}); console.log('ready'); setInterval(() => {}, 1000)", + 'daemon-entry', + socketPath, + tokenPath + ], + { stdio: ['ignore', 'pipe', 'ignore'] } + ) + // The handler must exist before SIGTERM to hold the fencing window open. + await new Promise((resolve, reject) => { + child.once('error', reject) + child.stdout?.once('data', () => resolve()) + }) + const childPid = child.pid as number + const childExited = new Promise((resolve) => child.once('exit', () => resolve())) + writeFileSync( + getDaemonPidPath(dir), + serializeDaemonPidFile({ pid: childPid, startedAtMs: null, launchNonce: 'daemon-a' }), + { mode: 0o600 } + ) + + const replacementRecord = serializeDaemonPidFile({ + pid: process.pid, + startedAtMs: 2_000, + launchNonce: 'daemon-b' + }) + const replacement = createServer((socket) => socket.end()) + const handover = setTimeout(() => { + void listenOnSocketPath(replacement, socketPath).then(() => { + writeFileSync(getDaemonPidPath(dir), replacementRecord, { mode: 0o600 }) + }) + }, 300) + + try { + await expect(killStaleDaemon(dir, socketPath, tokenPath)).resolves.toEqual({ + killed: true, + liveOwnerSurvived: true + }) + expect(readFileSync(getDaemonPidPath(dir), 'utf8')).toBe(replacementRecord) + await expect(canConnect(socketPath)).resolves.toBe(true) + } finally { + clearTimeout(handover) + try { + process.kill(childPid, 'SIGKILL') + } catch { + // Already gone. + } + await childExited + await closeServer(replacement) + } + } + ) }) diff --git a/src/main/daemon/daemon-health.ts b/src/main/daemon/daemon-health.ts index b6d264c95..4efded40d 100644 --- a/src/main/daemon/daemon-health.ts +++ b/src/main/daemon/daemon-health.ts @@ -1,7 +1,7 @@ /* oxlint-disable max-lines -- Why: pid validation shares process-identity helpers with kill escalation so the SIGKILL safety checks stay co-located. */ import { execFile, execFileSync } from 'node:child_process' -import { existsSync, readFileSync, unlinkSync } from 'node:fs' +import { existsSync, readFileSync } from 'node:fs' import { connect, type Socket } from 'node:net' import { promisify } from 'node:util' import { @@ -10,7 +10,16 @@ import { } from '../../shared/process-output-field-scanner' import { isStartupDiagnosticsEnabled, logStartupDiagnostic } from '../startup/startup-diagnostics' import { encodeNdjson } from './ndjson' -import { getDaemonPidPath } from './daemon-spawner' +import { + getDaemonPidPath, + unlinkDaemonPidFileWhen, + unlinkOwnedDaemonPidFile +} from './daemon-spawner' +import { + endpointIsProvenDead, + probeSocketConnect, + type SocketProbeOutcome +} from './daemon-endpoint-probe' import { PROTOCOL_VERSION, type HelloMessage, @@ -54,52 +63,6 @@ export type ParsedDaemonPid = { spawnerExecPath: string | null } -/** - * 'connected' — something is listening. 'missing'/'refused' — nothing is, and the endpoint - * name is safe to reclaim. 'unknown' — the probe itself failed (timeout on a loaded host, - * EPERM); the endpoint must be left alone because absence of proof is not proof of death. - */ -type SocketProbeOutcome = 'connected' | 'missing' | 'refused' | 'unknown' - -function probeSocketConnect(socketPath: string): Promise { - return new Promise((resolve) => { - if (process.platform !== 'win32' && !existsSync(socketPath)) { - resolve('missing') - return - } - const sock = connect({ path: socketPath }) - let settled = false - const cleanup = (): void => { - clearTimeout(timer) - sock.off('connect', onConnect) - sock.off('error', onError) - } - const settle = (result: SocketProbeOutcome): void => { - if (settled) { - return - } - settled = true - cleanup() - resolve(result) - } - const onConnect = (): void => { - settle('connected') - sock.destroy() - } - const onError = (error: NodeJS.ErrnoException): void => { - settle( - error.code === 'ECONNREFUSED' ? 'refused' : error.code === 'ENOENT' ? 'missing' : 'unknown' - ) - } - const timer = setTimeout(() => { - settle('unknown') - sock.destroy() - }, 500) - sock.on('connect', onConnect) - sock.on('error', onError) - }) -} - export function checkDaemonHealth(socketPath: string, tokenPath: string): Promise { return new Promise((resolve) => { if (process.env[E2E_FORCE_DAEMON_HEALTH_UNREACHABLE_ENV] === '1') { @@ -553,45 +516,57 @@ async function queryWindowsProcessIdentity(pid: number): Promise { +): Promise { try { process.kill(pid, 0) - } catch { - return false + } catch (error) { + // Why: only ESRCH proves the process is gone. EPERM means it exists and belongs to + // another user — reading that as absence deletes a live daemon's ownership. + return isNoSuchProcessError(error) ? 'mismatch' : 'unknown' } + const verdict = (matches: boolean): DaemonProcessIdentity => (matches ? 'match' : 'mismatch') + if (process.platform === 'win32') { const identity = await queryWindowsProcessIdentity(pid) if (identity === null) { - return false + return 'unknown' } // Why: image names are too broad after PID reuse. Match the daemon entry // plus the exact socket/token args so we only kill the daemon for this // userData protocol endpoint. - return ( + return verdict( commandLineMatchesDaemon(identity.commandLine, socketPath, tokenPath) && - startTimesWithinTolerance(identity.startedAtMs, startedAtMs, WIN32_START_TIME_TOLERANCE_MS) + startTimesWithinTolerance(identity.startedAtMs, startedAtMs, WIN32_START_TIME_TOLERANCE_MS) ) } try { const cmdline = readFileSync(`/proc/${pid}/cmdline`, 'utf8') - return ( + return verdict( commandLineMatchesDaemon(cmdline, socketPath, tokenPath) && startTimeMatches(pid, startedAtMs) ) } catch { const identity = getPsProcessIdentity(pid) if (!identity) { - return false + return 'unknown' } - return ( + return verdict( commandLineMatchesDaemon(identity.commandLine, socketPath, tokenPath) && - startTimesWithinTolerance(identity.startedAtMs, startedAtMs, START_TIME_TOLERANCE_MS) + startTimesWithinTolerance(identity.startedAtMs, startedAtMs, START_TIME_TOLERANCE_MS) ) } } @@ -654,7 +629,12 @@ async function readVerifiedDaemonPid( if ( !parsedPid || - !(await isDaemonProcess(parsedPid.pid, socketPath, tokenPath, parsedPid.startedAtMs)) + (await inspectDaemonProcessIdentity( + parsedPid.pid, + socketPath, + tokenPath, + parsedPid.startedAtMs + )) !== 'match' ) { return null } @@ -744,6 +724,15 @@ async function waitForProcessExit(pid: number, timeoutMs: number): Promise Promise +} + export type StaleDaemonKillOutcome = { /** A daemon was positively confirmed gone. Drives replacement telemetry. */ killed: boolean @@ -759,17 +748,43 @@ export async function killStaleDaemon( runtimeDir: string, socketPath: string, tokenPath: string, - protocolVersion = PROTOCOL_VERSION + protocolVersion = PROTOCOL_VERSION, + testHooks?: StaleDaemonKillTestHooks ): Promise { + const probeEndpoint = testHooks?.probeEndpoint ?? probeSocketConnect const pidPath = getDaemonPidPath(runtimeDir, protocolVersion) let killedDaemon = false let liveOwnerSurvived = false + // Why: identity is resolved once, and the record we may later remove is captured here so + // cleanup can be fenced to this exact incarnation rather than to whatever occupies the path + // by the time we get there. + let recordedOwner: ParsedDaemonPid | null = null try { const parsedPid = parseDaemonPidFile(readFileSync(pidPath, 'utf8')) + recordedOwner = parsedPid + const identity = parsedPid + ? await inspectDaemonProcessIdentity( + parsedPid.pid, + socketPath, + tokenPath, + parsedPid.startedAtMs + ) + : 'mismatch' if ( parsedPid && - (await isDaemonProcess(parsedPid.pid, socketPath, tokenPath, parsedPid.startedAtMs)) + identity === 'unknown' && + !endpointIsProvenDead(await probeEndpoint(socketPath)) ) { + // Why: the inspection failed, which is not evidence that this PID is someone else's. + // The endpoint is the tiebreaker, and it only settles the question when it positively + // proves nothing is serving. A probe that merely timed out is not proof either, so + // two inconclusive signals must preserve ownership rather than combine into a licence. + console.warn( + '[daemon] Preserving daemon that could not be inspected: reason=identity_probe_failed' + ) + return { killed: false, liveOwnerSurvived: true } + } + if (parsedPid && identity === 'match') { const { pid, startedAtMs } = parsedPid try { process.kill(pid, 'SIGTERM') @@ -797,19 +812,23 @@ export async function killStaleDaemon( // window is long enough for the pid to be recycled if the original // daemon died during the wait. Without this, we'd SIGKILL an unrelated // process that happens to now own the same pid. - if (!(await isDaemonProcess(pid, socketPath, tokenPath, startedAtMs))) { - // Why: a failed identity probe has two causes with opposite correct actions — - // the pid really was recycled (daemon dead, reclaim the endpoint), or the probe - // itself failed under load (`ps` has a 2s timeout). The endpoint settles it: if - // something still answers the socket, a daemon is alive and must be preserved. - if ((await probeSocketConnect(socketPath)) === 'connected') { + const recheck = await inspectDaemonProcessIdentity(pid, socketPath, tokenPath, startedAtMs) + if (recheck === 'mismatch') { + // Why: the pid provably no longer belongs to our daemon, so it is gone regardless + // of what the endpoint says. + console.warn('[daemon] Skipping SIGKILL for stale daemon: reason=pid_recycled') + exited = true + } else if (recheck === 'unknown') { + // Why: the inspection failed under load. Only an endpoint that proves nothing is + // serving may license reclaiming — a timed-out probe is not a second opinion. + if (endpointIsProvenDead(await probeEndpoint(socketPath))) { + console.warn('[daemon] Skipping SIGKILL for stale daemon: reason=pid_recycled') + exited = true + } else { console.warn( '[daemon] Preserving daemon that could not be identified: reason=identity_probe_failed' ) liveOwnerSurvived = true - } else { - console.warn('[daemon] Skipping SIGKILL for stale daemon: reason=pid_recycled') - exited = true } } else { try { @@ -844,24 +863,28 @@ export async function killStaleDaemon( return { killed: killedDaemon, liveOwnerSurvived } } - try { - unlinkSync(pidPath) - } catch { - // Best-effort + // Why: remove only the record belonging to the daemon we just dealt with. An unfenced + // unlink deletes whatever occupies the path now, which after a slow kill can be a + // replacement's freshly published ownership. + if (recordedOwner) { + unlinkOwnedDaemonPidFile(pidPath, recordedOwner.pid, recordedOwner.launchNonce) + } else { + // Why: a record we cannot parse names no owner to fence against, but leaving it in place + // fails the next daemon's exclusive publish with EEXIST — no daemon at all. Reclaim it + // under the same rename claim, and only while it is still unparseable, so a valid record + // published in the meantime is left alone. + unlinkDaemonPidFileWhen(pidPath, (content) => parseDaemonPidFile(content) === null) } - const socketOutcome = await probeSocketConnect(socketPath) - // Why: only positive evidence of a dead endpoint authorizes reclaiming the name. A probe - // that merely timed out leaves a live daemon's endpoint in place instead of unlinking it - // and forking a duplicate onto the freed path. - const endpointIsReclaimable = - killedDaemon || socketOutcome === 'refused' || socketOutcome === 'missing' - if (process.platform !== 'win32' && existsSync(socketPath) && endpointIsReclaimable) { - try { - unlinkSync(socketPath) - } catch { - // Best-effort - } + // Why this only reads: removing an endpoint on another daemon's behalf is the whole defect + // class this design retires. A replacement takes the name itself, by replacing an entry it + // has proven dead in one rename. So the only question left here is whether something is + // still answering — and only a positive answer withholds the fork. + // An unclassifiable entry is no longer a reason to refuse: the publisher probes again and + // will not overwrite anything it cannot prove dead, which makes this judgement a hint + // rather than a correctness dependency. + if ((await probeEndpoint(socketPath)) === 'connected') { + return { killed: killedDaemon, liveOwnerSurvived: true } } return { killed: killedDaemon, liveOwnerSurvived } } diff --git a/src/main/daemon/daemon-idle-shutdown.test.ts b/src/main/daemon/daemon-idle-shutdown.test.ts index fa12ba500..06071e820 100644 --- a/src/main/daemon/daemon-idle-shutdown.test.ts +++ b/src/main/daemon/daemon-idle-shutdown.test.ts @@ -5,6 +5,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { DaemonClient } from './client' +import { waitForEndpointUnreachable } from './daemon-endpoint-reachability-test-harness' import { DaemonPtyAdapter } from './daemon-pty-adapter' import { DaemonServer } from './daemon-server' import { PROTOCOL_VERSION } from './types' @@ -171,25 +172,27 @@ describe('current daemon lifecycle retirement', () => { await server.start() } - it('retires immediately after an unexpected empty disconnect and removes owned artifacts', async () => { - const launchNonce = 'launch-a' - writeFileSync( - pidPath, - serializeDaemonPidFile({ pid: process.pid, startedAtMs: null, launchNonce }) - ) - await startServer({ launchNonce }) - const client = new DaemonClient({ socketPath, tokenPath }) - await client.ensureConnected() - client.disconnect() - await waitFor(() => onIdleShutdown.mock.calls.length === 1) + it.skipIf(process.platform === 'win32')( + 'retires immediately after an unexpected empty disconnect and removes owned artifacts', + async () => { + const launchNonce = 'launch-a' + writeFileSync( + pidPath, + serializeDaemonPidFile({ pid: process.pid, startedAtMs: null, launchNonce }) + ) + await startServer({ launchNonce }) + const client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + client.disconnect() + await waitFor(() => onIdleShutdown.mock.calls.length === 1) - expect(clock.pendingCount).toBe(0) - expect(existsSync(tokenPath)).toBe(false) - expect(existsSync(pidPath)).toBe(false) - if (process.platform !== 'win32') { - expect(existsSync(socketPath)).toBe(false) + expect(clock.pendingCount).toBe(0) + expect(existsSync(tokenPath)).toBe(false) + expect(existsSync(pidPath)).toBe(false) + // Why: the dead entry remains for the next publisher to replace. + expect(await waitForEndpointUnreachable(socketPath)).toBe(true) } - }) + ) it('retires a fresh daemon that is never adopted by a full client pair', async () => { await startServer() diff --git a/src/main/daemon/daemon-init.test.ts b/src/main/daemon/daemon-init.test.ts index 77016298a..aa9180f59 100644 --- a/src/main/daemon/daemon-init.test.ts +++ b/src/main/daemon/daemon-init.test.ts @@ -359,7 +359,6 @@ vi.mock('./daemon-spawner', () => ({ `/fake/daemon/daemon-v${version ?? PROTOCOL_VERSION}.pid`, serializeDaemonPidFile: (obj: unknown) => JSON.stringify(obj), replaceDaemonPidFile: replaceDaemonPidFileMock, - sweepAbandonedDaemonClaims: vi.fn(() => 0), unlinkOwnedDaemonPidFile: unlinkOwnedDaemonPidFileMock })) @@ -1315,6 +1314,54 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => { expect(trackDaemonReplacedMock).toHaveBeenCalledWith('different_app_path', 0) }) + it('adopts the winner when a launched daemon loses the endpoint race', async () => { + // Why: losing the publish race is an expected outcome, not a crash. Reporting it as a + // startup failure strands this app on local non-persistent PTYs beside a healthy daemon. + const mod = await importFresh() + await mod.initDaemonPtyProvider(undefined, { macosLoginSessionWatch: true }) + + const launcher = spawnerInstances[0].launcher as ( + socketPath: string, + tokenPath: string + ) => Promise<{ shutdown(): Promise }> + // Force the replace-and-launch path; otherwise the launcher adopts the healthy daemon and + // never forks, and this test would pass without exercising anything. + getDaemonLaunchIdentityMock.mockReturnValueOnce('mismatch') + forkMock.mockImplementationOnce(() => { + const handlers: Record void)[]> = { + message: [], + error: [], + exit: [] + } + return { + pid: 24680, + on(event: string, cb: (arg?: unknown) => void) { + handlers[event]?.push(cb) + // Why the exit code and not the message: the launcher settles on exit, so keying + // adoption off the notification alone could lose that race. + if (event === 'exit') { + queueMicrotask(() => cb(20)) + } + return this + }, + off(event: string, cb: (arg?: unknown) => void) { + handlers[event] = handlers[event]?.filter((handler) => handler !== cb) ?? [] + return this + }, + kill: vi.fn(), + disconnect: vi.fn(), + unref: vi.fn() + } + }) + + // A handle rather than a rejection means the incumbent was adopted. + await expect(launcher('/fake/socket', '/fake/token')).resolves.toMatchObject({ + shutdown: expect.any(Function) + }) + // Guard against passing for the wrong reason: the adoption must follow a real launch. + expect(forkMock).toHaveBeenCalledTimes(1) + }) + it('replaces a healthy daemon whose macOS TCC attribution is severed when it has no live sessions', async () => { const mod = await importFresh() await mod.initDaemonPtyProvider(undefined, { macosLoginSessionWatch: true }) diff --git a/src/main/daemon/daemon-init.ts b/src/main/daemon/daemon-init.ts index 136828288..7b95d4a01 100644 --- a/src/main/daemon/daemon-init.ts +++ b/src/main/daemon/daemon-init.ts @@ -17,7 +17,7 @@ import { type DaemonPidFile, type DaemonProcessHandle } from './daemon-spawner' -import { sweepAbandonedDaemonClaims } from './daemon-endpoint-ownership' +import { DAEMON_EXIT_ENDPOINT_OCCUPIED } from './daemon-endpoint-ownership' import { DaemonPtyAdapter, type DaemonRespawnReason } from './daemon-pty-adapter' import { DaemonPtyRouter } from './daemon-pty-router' import { DaemonClient } from './client' @@ -729,7 +729,8 @@ function createOutOfProcessLauncher( // Wait for the daemon to signal readiness via IPC let launchedIdentity: DaemonEndpointIdentity | null = null - await new Promise((resolve, reject) => { + let endpointUnavailableReason: string | null = null + const startupSignal = new Promise((resolve, reject) => { let timer: ReturnType | undefined let settled = false function cleanupStartupListeners(): void { @@ -772,6 +773,17 @@ function createOutOfProcessLauncher( reject(startupError) } function onReadyMessage(msg: unknown): void { + if ( + msg && + typeof msg === 'object' && + (msg as { type?: string }).type === 'endpoint-unavailable' + ) { + // Why: the child lost the endpoint race rather than crashing. Record it so the + // launcher can adopt the winner instead of reporting a generic startup failure. + endpointUnavailableReason = (msg as { reason?: string }).reason ?? 'occupied' + void fail(new Error(`Daemon could not take the endpoint: ${endpointUnavailableReason}`)) + return + } if (msg && typeof msg === 'object' && (msg as { type?: string }).type === 'ready') { if (settled) { return @@ -802,6 +814,11 @@ function createOutOfProcessLauncher( } function onStartupExit(code: number | null): void { + if (code === DAEMON_EXIT_ENDPOINT_OCCUPIED) { + // Why here and not only on the IPC message: the exit is the event this wait settles + // on, so keying off it cannot lose to a notification still in the channel. + endpointUnavailableReason = 'occupied' + } void fail(new Error(`Daemon exited during startup with code ${code}`)) } @@ -814,6 +831,32 @@ function createOutOfProcessLauncher( child.on('exit', onStartupExit) }) + try { + await startupSignal + } catch (error) { + if (endpointUnavailableReason !== 'occupied') { + throw error + } + // Why adopt rather than retry: another daemon proved it owns the endpoint and is + // answering on it. Forking again would lose the same race, and reporting a startup + // failure strands this app on local non-persistent PTYs beside a healthy daemon. + console.warn( + '[daemon] Endpoint was taken by another daemon during startup — adopting it instead' + ) + // Why pidPath: adopting reconciles the PID record against the identity the daemon + // reports over hello, repairing a record that names the wrong incarnation. Every other + // adoption path passes it; this one skipped it, so the incumbent we adopt here was the + // only one whose record never got that repair. + return await holdDaemonAdoptionLease( + createPreservedDaemonHandle(runtimeDir), + socketPath, + tokenPath, + undefined, + undefined, + pidPath + ) + } + try { if (!launchedIdentity) { throw new Error('Daemon readiness identity is incomplete') @@ -855,6 +898,23 @@ function createOutOfProcessLauncher( } } catch (error) { adoptionClient?.disconnect() + adoptionClient = null + // Why: the launcher may now fork onto an endpoint it could not classify, because the + // publisher is the real guard — and that guard works by refusing to overwrite what it + // cannot prove dead, so the child exits instead of splitting the brain. Correct, but + // giving up here costs the user every persistent session for the whole run. Something + // answering the endpoint now is a daemon worth adopting, not a reason to fall back to + // local PTYs. + if (await probeSocket(socketPath)) { + console.warn( + '[daemon] DEGRADED MODE: adopting the daemon that owns the endpoint after a replacement could not publish onto it. Existing sessions keep working; fresh terminals run on the local provider WITHOUT daemon persistence until you restart the daemon (Manage Sessions → Restart).' + ) + try { + return await preserveDaemon('degraded-new-pty-fallback') + } catch { + // It stopped answering between the probe and the adoption; report the launch failure. + } + } throw error } } @@ -872,11 +932,6 @@ export async function initDaemonPtyProvider( } const runtimeDir = getRuntimeDir() - // Why: rename-claim and bind scratch names are unlinked by their owner, but a failed unlink - // leaves one behind forever. Sweep before launching so a failed launch is still reclaimed; - // age-gated so a claim still in flight is never disturbed. - sweepAbandonedDaemonClaims(runtimeDir) - const newSpawner = new DaemonSpawner({ runtimeDir, launcher: createOutOfProcessLauncher(runtimeDir, options.macosLoginSessionWatch ?? false) @@ -1243,14 +1298,6 @@ export async function cleanupDaemonForProtocol( // Endpoint absence doesn't prove the PID record belongs to the current protocol; leave artifact cleanup to the owning daemon. return { cleaned: false, killedCount: 0 } } - // Best-effort remove a stale socket so a future launch doesn't hit EADDRINUSE on bind. - if (process.platform !== 'win32' && existsSync(socketPath)) { - try { - unlinkSync(socketPath) - } catch { - // Best-effort - } - } try { unlinkSync(pidPath) } catch { @@ -1277,8 +1324,16 @@ export async function cleanupDaemonForProtocol( didRequestShutdown = true } catch { // Previous-protocol daemons may be wedged or too old for the RPC path; fall back to PID cleanup (only unlinks a live socket after proving the process is killed). - didKillStaleDaemon = (await killStaleDaemon(runtimeDir, socketPath, tokenPath, protocolVersion)) - .killed + const killOutcome = await killStaleDaemon(runtimeDir, socketPath, tokenPath, protocolVersion) + didKillStaleDaemon = killOutcome.killed + if (killOutcome.liveOwnerSurvived) { + // Why: something still owns the endpoint. Returning as if it were cleaned lets restart + // fork a replacement that cannot publish onto the held name, leaving the user with no + // daemon instead of the one still running. + throw new DaemonEndpointOwnershipError( + 'Daemon cleanup aborted: the existing daemon could not be confirmed stopped' + ) + } } finally { client.disconnect() } @@ -1291,14 +1346,6 @@ export async function cleanupDaemonForProtocol( return { cleaned: true, killedCount } } - // Defensively unlink the socket: the daemon normally removes it after `shutdown`, but on some crash paths it lingers and blocks a later rebind. - if (didRequestShutdown && process.platform !== 'win32' && existsSync(socketPath)) { - try { - unlinkSync(socketPath) - } catch { - // Best-effort - } - } try { unlinkSync(pidPath) } catch { @@ -1356,13 +1403,6 @@ export async function createLegacyDaemonAdapters( // Best-effort } } - if (process.platform !== 'win32' && existsSync(socketPath)) { - try { - unlinkSync(socketPath) - } catch { - // Best-effort - } - } } continue } diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index 92f8abe58..03fbcc1e2 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -3,6 +3,7 @@ import { basename } from 'node:path' import { existsSync, readFileSync } from 'node:fs' import { readFile } from 'node:fs/promises' import { DaemonClient } from './client' +import { DAEMON_ENDPOINT_LOST_MESSAGE } from './daemon-endpoint-ownership' import { getMacDaemonSystemResolverHealth, parseDaemonPidFile, @@ -2492,7 +2493,8 @@ function isUnknownRequestTypeError(err: unknown): boolean { // Why: syscall='connect' distinguishes a dead-socket ENOENT/ECONNREFUSED from token-file ENOENT (no syscall); // message strings incl. wedged-daemon "Hello response timed out" (#8689) also warrant a respawn. -function isDaemonGoneError(err: unknown): boolean { +/** Exported so a test can pin it against the server's refusal wording, which it must match. */ +export function isDaemonGoneError(err: unknown): boolean { if (!(err instanceof Error)) { return false } @@ -2505,7 +2507,10 @@ function isDaemonGoneError(err: unknown): boolean { msg === 'Connection lost' || msg === 'Not connected' || msg === 'Hello response timed out' || - msg === 'Daemon temporarily unavailable; reconnect' + msg === 'Daemon temporarily unavailable; reconnect' || + // Why retry: the daemon refused because the endpoint now resolves elsewhere. Reconnecting + // reaches whoever owns it; surfacing this to the user would strand the request instead. + msg === DAEMON_ENDPOINT_LOST_MESSAGE ) } diff --git a/src/main/daemon/daemon-server-error-handling.test.ts b/src/main/daemon/daemon-server-error-handling.test.ts new file mode 100644 index 000000000..25d1e2585 --- /dev/null +++ b/src/main/daemon/daemon-server-error-handling.test.ts @@ -0,0 +1,456 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { chmodSync, linkSync, mkdtempSync, rmSync, unlinkSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { createServer, type Server } from 'node:net' +import { DaemonServer } from './daemon-server' +import { DaemonClient } from './client' +import { isDaemonGoneError } from './daemon-pty-adapter' +import { DAEMON_ENDPOINT_LOST_MESSAGE } from './daemon-endpoint-ownership' +import { getDaemonSocketPath } from './daemon-spawner' +import type { SubprocessHandle } from './session' +import { waitForEndpointUnreachable } from './daemon-endpoint-reachability-test-harness' + +// A killed process must actually report its exit: teardown waits +// IMMEDIATE_KILL_PHYSICAL_EXIT_TIMEOUT_MS for one that never does. +function createMockSubprocess(): SubprocessHandle { + let notifyExit: ((code: number) => void) | null = null + const exit = (): void => notifyExit?.(0) + return { + pid: 44444, + getForegroundProcess: () => null, + write() {}, + resize() {}, + kill: exit, + forceKill: exit, + signal() {}, + onData() {}, + onExit(callback) { + notifyExit = callback + }, + dispose() {} + } +} + +describe('daemon server error handling', () => { + let dir: string + let socketPath: string + let tokenPath: string + let server: DaemonServer + let client: DaemonClient | null = null + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'daemon-server-errors-')) + socketPath = getDaemonSocketPath(dir) + tokenPath = join(dir, 'test.token') + }) + + afterEach(async () => { + client?.disconnect() + client = null + await server?.shutdown() + rmSync(dir, { recursive: true, force: true }) + }) + + it.skipIf(process.platform === 'win32')( + 'refuses to create a session on an endpoint it no longer owns', + async () => { + // Why this matters: publishing cannot be made atomic against a publisher preempted between + // proving an entry dead and replacing it, so this daemon can lose the endpoint at any + // moment. If it accepted a session in that window the session would be reachable by + // nobody — a terminal that acknowledges input and never runs it, which is the original + // bug. Refusing makes that outcome unreachable rather than merely short-lived. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + + client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + // Sanity: a session is creatable while ownership holds. + await expect( + client.request('createOrAttach', { sessionId: 'before', cols: 80, rows: 24 }) + ).resolves.toMatchObject({ isNew: true }) + + // Another daemon takes the canonical name, exactly as a late publisher's rename would. + const usurper = createServer() + const usurperBind = join(dir, '.u') + await new Promise((resolve) => usurper.listen(usurperBind, resolve)) + try { + unlinkSync(socketPath) + linkSync(usurperBind, socketPath) + + await expect( + client.request('createOrAttach', { sessionId: 'after', cols: 80, rows: 24 }) + ).rejects.toThrow(/no longer owns its endpoint/) + + // And it stands down rather than lingering as an unreachable host. + const daemon = server as unknown as { retirementRequested: boolean } + expect(daemon.retirementRequested).toBe(true) + } finally { + await new Promise((resolve) => usurper.close(() => resolve())) + } + } + ) + + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'keeps serving sessions when endpoint ownership cannot be read', + async () => { + // Why: an unreadable stat proves nothing. Refusing sessions on it would take a daemon that + // is serving every terminal on the machine offline because of a transient EACCES or EIO. + // Only positive evidence of loss may refuse. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + const daemon = server as unknown as { + hasLostEndpointOwnership: () => boolean + retirementRequested: boolean + } + + try { + // Drop search permission on the directory so the ownership stat fails EACCES. + chmodSync(dir, 0o600) + + expect(daemon.hasLostEndpointOwnership()).toBe(false) + expect(daemon.retirementRequested).toBe(false) + } finally { + chmodSync(dir, 0o700) + } + } + ) + + it.skipIf(process.platform === 'win32')( + 'still attaches to a session it already hosts after losing the endpoint', + async () => { + // Why attach is exempt: it reaches a terminal already on this daemon, over a connection + // that already exists, so it strands nothing. Refusing it would break the drain a retiring + // daemon depends on to let live sessions finish. Note this must run without a prior + // refusal, which would null the owned identity and disarm the guard anyway. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + await client.request('createOrAttach', { sessionId: 'live', cols: 80, rows: 24 }) + + const usurper = createServer() + const usurperBind = join(dir, '.u2') + await new Promise((resolve) => usurper.listen(usurperBind, resolve)) + try { + unlinkSync(socketPath) + linkSync(usurperBind, socketPath) + + // The guard is still armed here — no creation has been refused yet. + const daemon = server as unknown as { hasLostEndpointOwnership: () => boolean } + expect(daemon.hasLostEndpointOwnership()).toBe(true) + + await expect( + client.request('createOrAttach', { + sessionId: 'live', + cols: 80, + rows: 24, + attachOnly: true + }) + ).resolves.toMatchObject({ isNew: false }) + } finally { + await new Promise((resolve) => usurper.close(() => resolve())) + } + } + ) + + it.skipIf(process.platform === 'win32')( + 'never reopens session creation after the endpoint was lost', + async () => { + // Why: retiring nulls the owned identity, so an ownership check inferred from it answers + // "not lost" forever after. A stream socket accepted before the takeover can then finish + // its hello, clear the pending retirement, and reopen creation on a daemon nothing can + // reach — reinstating the exact outcome the guard exists to prevent. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + // Keep one session alive: with nothing to drain the daemon now stands down the moment it + // loses the endpoint, which is correct but would end the test before it can assert. + await client.request('createOrAttach', { sessionId: 'draining', cols: 80, rows: 24 }) + + const usurper = createServer() + const usurperBind = join(dir, '.u3') + await new Promise((resolve) => usurper.listen(usurperBind, resolve)) + try { + unlinkSync(socketPath) + linkSync(usurperBind, socketPath) + + await expect( + client.request('createOrAttach', { sessionId: 'a', cols: 80, rows: 24 }) + ).rejects.toThrow(/no longer owns its endpoint/) + + // Simulate a late client completing its handshake and clearing pending retirement. + const daemon = server as unknown as { + retirementRequested: boolean + hasLostEndpointOwnership: () => boolean + } + daemon.retirementRequested = false + + // The loss must still be remembered, so creation stays closed. + expect(daemon.hasLostEndpointOwnership()).toBe(true) + await expect( + client.request('createOrAttach', { sessionId: 'b', cols: 80, rows: 24 }) + ).rejects.toThrow(/no longer owns its endpoint/) + } finally { + await new Promise((resolve) => usurper.close(() => resolve())) + } + } + ) + + it('treats the endpoint-lost refusal as a reconnectable error', () => { + // Why pinned: the server refuses so the client can reconnect to whoever owns the endpoint. + // If the client's retry predicate does not recognise the refusal, it surfaces to the user + // and the request dead-ends — barely better than the strand the refusal exists to avoid. + expect(isDaemonGoneError(new Error(DAEMON_ENDPOINT_LOST_MESSAGE))).toBe(true) + expect(isDaemonGoneError(new Error('something else entirely'))).toBe(false) + }) + + it.skipIf(process.platform === 'win32')( + 'stands down once drained even while a pre-takeover client stays connected', + async () => { + // Why connections stop counting after loss: retirement drains and then exits, but idleness + // normally waits for every client to disconnect. A client that connected before the + // takeover can hold that open indefinitely, so the daemon would outlive its last session + // as an orphan nothing can route to. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + + const usurper = createServer() + const usurperBind = join(dir, '.u4') + await new Promise((resolve) => usurper.listen(usurperBind, resolve)) + try { + unlinkSync(socketPath) + linkSync(usurperBind, socketPath) + + // Losing the endpoint with nothing left to drain must stand the daemon down, even + // though this client is still connected. + const daemon = server as unknown as { + requestRetirementForLostEndpoint: () => void + idleShutdownState: string + } + expect(daemon.idleShutdownState).toBe('running') + daemon.requestRetirementForLostEndpoint() + + // Drained and unreachable: it must begin standing down rather than wait for this + // client, which can never make it routable again. + expect(daemon.idleShutdownState).not.toBe('running') + } finally { + await new Promise((resolve) => usurper.close(() => resolve())) + } + } + ) + + it.skipIf(process.platform === 'win32')( + 'still answers the refusal when losing the endpoint also begins shutdown', + async () => { + // Why: the guard retires synchronously, and with nothing to drain that begins shutdown + // before the reply is written. The client must still receive the refusal it can retry on; + // a dropped connection here would surface as an opaque failure instead of a reconnect. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + + const usurper = createServer() + const usurperBind = join(dir, '.u5') + await new Promise((resolve) => usurper.listen(usurperBind, resolve)) + try { + unlinkSync(socketPath) + linkSync(usurperBind, socketPath) + + // No sessions, so retiring drains immediately and shutdown starts inside this call. + await expect( + client.request('createOrAttach', { sessionId: 'x', cols: 80, rows: 24 }) + ).rejects.toThrow(/no longer owns its endpoint/) + } finally { + await new Promise((resolve) => usurper.close(() => resolve())) + } + } + ) + + it.skipIf(process.platform === 'win32')( + 'records endpoint loss even when already retiring for another reason', + async () => { + // Why: a daemon can already be retiring because its last authenticated client dropped + // while a session runs. Losing the endpoint is a different fact, and gating one on the + // other meant the loss went unrecorded — after which a later hello cleared the retirement + // and put a daemon that demonstrably could not be reached back into ordinary service. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + await client.request('createOrAttach', { sessionId: 'keepalive', cols: 80, rows: 24 }) + + const usurper = createServer() + const usurperBind = join(dir, '.u6') + await new Promise((resolve) => usurper.listen(usurperBind, resolve)) + try { + unlinkSync(socketPath) + linkSync(usurperBind, socketPath) + + const daemon = server as unknown as { + retirementRequested: boolean + endpointOwnershipLost: boolean + checkEndpointOwnership: () => void + } + // Retirement is already pending for an unrelated reason. + daemon.retirementRequested = true + + daemon.checkEndpointOwnership() + daemon.checkEndpointOwnership() + + // The loss must be recorded regardless, so a later hello cannot undo it. + expect(daemon.endpointOwnershipLost).toBe(true) + } finally { + await new Promise((resolve) => usurper.close(() => resolve())) + } + } + ) + + it.skipIf(process.platform === 'win32')( + 'does not retire on losses separated by a demonstrably owned observation', + async () => { + // Why: the watchdog retires on CONSECUTIVE losses, but an admission-time ownership read is + // just as authoritative. When it did not reset the streak, two losses with a positive + // owned reading between them counted as consecutive — permanently poisoning a healthy, + // reachable daemon into refusing every later session. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + const daemon = server as unknown as { + checkEndpointOwnership: () => void + hasLostEndpointOwnership: () => boolean + endpointOwnershipLost: boolean + endpointOwnershipLossStreak: number + } + // A second name for our own socket inode, so ownership can be handed back. + const ourAlias = join(dir, '.ours') + linkSync(socketPath, ourAlias) + + const usurper = createServer() + const usurperBind = join(dir, '.u7') + await new Promise((resolve) => usurper.listen(usurperBind, resolve)) + try { + // First loss. + unlinkSync(socketPath) + linkSync(usurperBind, socketPath) + daemon.checkEndpointOwnership() + expect(daemon.endpointOwnershipLossStreak).toBe(1) + expect(daemon.endpointOwnershipLost).toBe(false) + + // Ownership demonstrably returns, observed through the admission path. + unlinkSync(socketPath) + linkSync(ourAlias, socketPath) + expect(daemon.hasLostEndpointOwnership()).toBe(false) + expect(daemon.endpointOwnershipLossStreak).toBe(0) + + // A later isolated loss is confirmation #1, not #2, so it must not retire. + unlinkSync(socketPath) + linkSync(usurperBind, socketPath) + daemon.checkEndpointOwnership() + expect(daemon.endpointOwnershipLost).toBe(false) + } finally { + await new Promise((resolve) => usurper.close(() => resolve())) + } + } + ) + + it('keeps serving after an operational server error instead of dying', async () => { + // Why: an unhandled 'error' on a net.Server is an uncaught exception. Detaching the startup + // listener once start() settled left a daemon hosting every terminal on the machine one + // failed accept away from termination. + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + await server.start() + const daemon = server as unknown as { server: Server | null } + + expect(daemon.server?.listenerCount('error')).toBe(1) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + // Twice on purpose: a one-shot listener survives the first error and dies on the second, + // so a single emit cannot tell a permanent handler from `once`. + expect(() => daemon.server?.emit('error', new Error('EMFILE: accept failed'))).not.toThrow() + expect(() => + daemon.server?.emit('error', new Error('EMFILE: accept failed again')) + ).not.toThrow() + expect(warn).toHaveBeenCalledTimes(2) + expect(daemon.server?.listenerCount('error')).toBe(1) + } finally { + warn.mockRestore() + } + + client = new DaemonClient({ socketPath, tokenPath }) + await client.ensureConnected() + expect(client.isConnected()).toBe(true) + }) + + it.skipIf(process.platform === 'win32')( + 'stands down instead of serving when the server errors while publication is in flight', + async () => { + // Why: publishing awaits a liveness probe, so a server error can land mid-flight and the + // rejection alone cannot stop it. Going on to serve would leave a live published daemon + // behind a caller told startup failed — and a caller that responds by launching a + // replacement recreates the split brain. + // A dead entry on the canonical name forces publish down the probe-then-rename path, so + // the error below lands inside the async window rather than before it. + const stale = createServer() + const stalePath = join(dir, '.bstale00001') + await new Promise((resolve) => stale.listen(stalePath, resolve)) + linkSync(stalePath, socketPath) + await new Promise((resolve) => stale.close(() => resolve())) + + server = new DaemonServer({ + socketPath, + tokenPath, + spawnSubprocess: () => createMockSubprocess() + }) + const started = server.start() + const daemon = server as unknown as { server: Server | null } + await vi.waitFor(() => expect(daemon.server?.listening).toBe(true)) + daemon.server?.emit('error', new Error('injected accept failure')) + + await expect(started).rejects.toThrow('injected accept failure') + // Why wait: start() rejects the moment the error lands, while publication is still in + // flight. Reporting failure must end with the daemon actually not serving. + await vi.waitFor(() => expect(daemon.server).toBeNull()) + await expect(waitForEndpointUnreachable(socketPath)).resolves.toBe(true) + } + ) +}) diff --git a/src/main/daemon/daemon-server.test.ts b/src/main/daemon/daemon-server.test.ts index 053c50510..1ec4b7803 100644 --- a/src/main/daemon/daemon-server.test.ts +++ b/src/main/daemon/daemon-server.test.ts @@ -9,6 +9,7 @@ import { encodeNdjson } from './ndjson' import { PROTOCOL_VERSION, type DaemonRequest } from './types' import type { SubprocessHandle } from './session' import { getDaemonPidPath, getDaemonSocketPath, serializeDaemonPidFile } from './daemon-spawner' +import { waitForEndpointUnreachable } from './daemon-endpoint-reachability-test-harness' const confirmForegroundProcessMock = vi.fn(async () => 'droid') @@ -156,13 +157,6 @@ describe('DaemonServer', () => { expect(token.length).toBeGreaterThan(0) }) - it('removes the startup error listener after listening', async () => { - await startServer() - - const daemon = server as unknown as DaemonServerPrivate - expect(daemon.server?.listenerCount('error')).toBe(0) - }) - it('accepts client connections', async () => { await startServer() const c = await connectClient() @@ -916,6 +910,7 @@ describe('DaemonServer', () => { await expect(c.ensureConnected()).rejects.toThrow() }) + // Runs everywhere: a closed Windows pipe classifies as missing, not connected. it('still terminates via the shutdown RPC when disposal cannot prove physical exit', async () => { await startServer() const daemon = server as unknown as DaemonServerPrivate & { @@ -931,7 +926,8 @@ describe('DaemonServer', () => { await expect(c.request('shutdown', { killSessions: true })).resolves.toEqual({}) await waitFor(() => daemon.server === null) - await waitFor(() => !existsSync(socketPath)) + // Why not existsSync: the dead entry remains for the next publisher to replace. + expect(await waitForEndpointUnreachable(socketPath)).toBe(true) const late = new DaemonClient({ socketPath, tokenPath }) await expect(late.ensureConnected()).rejects.toThrow() }) diff --git a/src/main/daemon/daemon-server.ts b/src/main/daemon/daemon-server.ts index b69641e64..51d9d0913 100644 --- a/src/main/daemon/daemon-server.ts +++ b/src/main/daemon/daemon-server.ts @@ -24,12 +24,15 @@ import { isTuiAgent } from '../../shared/tui-agent-config' import { parsePtyStartupIngressIntent } from '../../shared/pty-startup-ingress' import { unlinkOwnedDaemonPidFile, unlinkOwnedDaemonTokenFile } from './daemon-spawner' import { + DAEMON_ENDPOINT_LOST_MESSAGE, + DaemonEndpointUnavailableError, + type DaemonEndpointOwnershipState, getDaemonSocketBindPath, - publishDaemonSocketPath, + publishDaemonEndpoint, readDaemonEndpointOwnershipState, - unlinkOwnedDaemonSocketPath, type DaemonSocketIdentity } from './daemon-endpoint-ownership' +import { probeSocketConnect } from './daemon-endpoint-probe' import { CLEAN_DISCONNECT_PROTOCOL_VERSION, PROTOCOL_VERSION, @@ -123,8 +126,15 @@ export class DaemonServer { private appVersion: string | null private spawnerExecPath: string | null private ownedSocketIdentity: DaemonSocketIdentity | null = null + /** Set once start() has been rejected, so async publication can tell it is no longer wanted. */ + private startupFailure: Error | null = null private endpointOwnershipTimer: ReturnType | null = null private endpointOwnershipLossStreak = 0 + /** + * Sticky, and not inferred from `ownedSocketIdentity`: retiring nulls that, so a socket accepted + * before the takeover could finish its hello and reopen sessions on an unreachable daemon. + */ + private endpointOwnershipLost = false private protocolVersion: number private onIdleShutdown: () => void private onRpcShutdown: () => void @@ -235,69 +245,112 @@ export class DaemonServer { async start(): Promise { return new Promise((resolve, reject) => { this.server = createServer((socket) => this.handleConnection(socket)) - const onListenError = (err: Error): void => { + // Permanent, not `once`: an unhandled 'error' is an uncaught exception, so a post-startup + // accept failure (EMFILE) would kill a daemon hosting every terminal. + let startupSettled = false + const onServerError = (err: Error): void => { + if (startupSettled) { + // The daemon is serving; an operational error must not read as a startup failure. + this.log.log('server-error', { message: err.message }) + console.warn(`[daemon] Socket server error: ${err.message}`) + return + } + startupSettled = true + // Publishing is async, so the continuation reads this and tears down instead of arming + // a daemon whose caller was already told startup failed. + this.startupFailure = err reject(err) } - this.server.once('error', onListenError) + this.server.on('error', onServerError) - // Why: bind a private name and hard-link it into place, so libuv's close-time unlink - // can only ever remove our own bind name — never a replacement daemon's endpoint. + // Private bind name, linked into place: libuv's close-time unlink can then only ever + // remove our own name, never a replacement daemon's endpoint. const bindPath = process.platform === 'win32' ? this.socketPath : getDaemonSocketBindPath(this.socketPath) this.server.listen(bindPath, () => { - // Why: drop the startup error listener after bind so it doesn't retain this closure. - this.server?.off('error', onListenError) try { - // Why: tighten the mode on the private bind name so the endpoint is never reachable - // at the canonical path with default permissions, even briefly. + // So the endpoint is never briefly reachable with default permissions. chmodSync(bindPath, 0o600) } catch { // Best-effort on platforms that support it } - let publishedOwnership = false - try { - // Why: the exclusive link is the endpoint claim, and the PID/nonce record must - // exist before the token makes this listener adoptable. - this.ownedSocketIdentity = publishDaemonSocketPath(bindPath, this.socketPath) - this.publishEndpointOwnership() - publishedOwnership = true - writeFileSync(this.tokenPath, this.token, { mode: 0o600 }) - } catch (error) { - // Why: roll back only a record we actually wrote. Losing the endpoint claim means - // the record at that path belongs to the incumbent daemon, and even the ownership- - // checked unlink briefly renames it aside — enough to strand a live daemon's record. - if (publishedOwnership && this.pidPath && this.launchNonce) { - unlinkOwnedDaemonPidFile(this.pidPath, process.pid, this.launchNonce) - } - unlinkOwnedDaemonSocketPath(this.socketPath, this.ownedSocketIdentity) - this.ownedSocketIdentity = null + const abandonStartup = (error: unknown): void => { + startupSettled = true const server = this.server this.server = null - // Why: settle before close — an already-accepted connection defers the close - // callback indefinitely, and start() has no timeout of its own. + // Settle before close: an accepted connection defers the close callback indefinitely. reject(error) server?.close() if (process.platform !== 'win32') { try { unlinkSync(bindPath) } catch { - // Already consumed by a successful link, or never created. + // Already consumed by a successful link or rename, or never created. } } - return } - if (this.protocolVersion >= CLEAN_DISCONNECT_PROTOCOL_VERSION) { - // Why: a parent crash before the first full client pair must not leave an empty daemon alive forever. - this.armInitialAdoptionTimeout() - } - this.startEndpointOwnershipWatch() - resolve() + void this.publishAndArm(bindPath).then(() => { + // The server can fail while publishing awaits its probe; resolving then would strand a + // live daemon behind a caller told startup failed, who would launch a replacement. + if (this.startupFailure) { + this.retireUnstartedDaemon() + abandonStartup(this.startupFailure) + return + } + // Serving from here, so later server errors are logged, not treated as startup failure. + startupSettled = true + resolve() + }, abandonStartup) }) }) } + /** + * Takes the canonical endpoint, then makes this listener adoptable — in that order. Never rolled + * back: an aborting daemon just closes, and the next publisher replaces the dead entry. + */ + private async publishAndArm(bindPath: string): Promise { + const outcome = await publishDaemonEndpoint(bindPath, this.socketPath, probeSocketConnect) + if (outcome.status !== 'published') { + // The only point the design declines to serve, so a field regression surfaces here. + this.log.log('endpoint-publish-declined', { reason: outcome.status }) + console.warn(`[daemon] Endpoint unavailable at startup: reason=${outcome.status}`) + throw new DaemonEndpointUnavailableError(outcome.status) + } + this.ownedSocketIdentity = outcome.identity + let publishedOwnership = false + try { + // The PID/nonce record must exist before the token makes this listener adoptable. + this.publishEndpointOwnership() + publishedOwnership = true + writeFileSync(this.tokenPath, this.token, { mode: 0o600 }) + } catch (error) { + // Roll back only a record we wrote; anything else at that path belongs to another daemon. + if (publishedOwnership && this.pidPath && this.launchNonce) { + unlinkOwnedDaemonPidFile(this.pidPath, process.pid, this.launchNonce) + } + this.ownedSocketIdentity = null + throw error + } + if (this.protocolVersion >= CLEAN_DISCONNECT_PROTOCOL_VERSION) { + // A parent crash before the first client pair must not strand an empty daemon forever. + this.armInitialAdoptionTimeout() + } + this.startEndpointOwnershipWatch() + } + + /** + * Stands down a daemon that published after its startup was already reported failed. The + * endpoint stays, as on every exit path — removing it could delete a replacement's name. + */ + private retireUnstartedDaemon(): void { + this.stopEndpointOwnershipWatch() + this.cancelInitialAdoptionTimer() + this.unlinkOwnedEndpointArtifacts() + } + async shutdown(): Promise { if (this.shutdownPromise) { return this.shutdownPromise @@ -326,14 +379,13 @@ export class DaemonServer { } private unlinkOwnedEndpointArtifacts(): void { - // Why: ownership checks prevent removing a late replacement's token, PID record or endpoint. + // Ownership-checked so a late replacement's token or PID record is never removed. unlinkOwnedDaemonTokenFile(this.tokenPath, this.token) if (this.pidPath && this.launchNonce) { unlinkOwnedDaemonPidFile(this.pidPath, process.pid, this.launchNonce) } - // Why: we bound a private name, so libuv unlinks nothing at the canonical path — this - // is the only removal of our endpoint, and it is skipped once someone else owns it. - unlinkOwnedDaemonSocketPath(this.socketPath, this.ownedSocketIdentity) + // The endpoint is deliberately left: fencing its removal against a replacement that published + // meanwhile was this component's largest source of defects, and a dead entry costs nothing. this.ownedSocketIdentity = null } @@ -345,7 +397,7 @@ export class DaemonServer { () => this.checkEndpointOwnership(), DaemonServer.ENDPOINT_OWNERSHIP_POLL_MS ) - // Why: a liveness poll must never be the reason the process cannot exit. + // A liveness poll must never be the reason the process cannot exit. this.endpointOwnershipTimer.unref() } @@ -360,42 +412,73 @@ export class DaemonServer { /** * Retires a daemon that no longer owns the canonical endpoint. * - * Why: a daemon whose endpoint name was taken over keeps hosting PTYs that no client can - * reach through the socket, which reads to the user as terminals that acknowledge input and - * never run it. Retirement drains rather than kills: live sessions finish, and the process - * exits once idle instead of surviving as an unreachable orphan. + * Such a daemon keeps hosting PTYs no client can reach — terminals that take input and never + * run it. Retirement drains rather than kills: sessions finish and the process exits once idle. */ private checkEndpointOwnership(): void { if (process.platform === 'win32' || !this.ownedSocketIdentity || this.shutdownPromise) { return } - const state = readDaemonEndpointOwnershipState(this.socketPath, this.ownedSocketIdentity) - if (state === 'owned') { - this.endpointOwnershipLossStreak = 0 - return - } - if (state === 'indeterminate') { - // Why: an inconclusive stat proves nothing. Retiring on EACCES or EIO would take down a - // daemon that is still serving every terminal on the machine. Reset the streak too, so - // the confirmations we act on are consecutive rather than merely cumulative. - this.endpointOwnershipLossStreak = 0 + // An inconclusive stat resets too: EACCES must not retire a daemon that is still serving. + if (this.observeEndpointOwnership() !== 'lost') { return } this.endpointOwnershipLossStreak++ - // Why: a replacement publishes by unlink-then-link, so a single observation can land in - // that gap. Require the loss to persist before acting on it. + // Two, though a single rename leaves no gap to misread: this is a backstop, not the detector. if (this.endpointOwnershipLossStreak < DaemonServer.ENDPOINT_OWNERSHIP_LOSS_CONFIRMATIONS) { return } - if (this.retirementRequested) { - return + this.requestRetirementForLostEndpoint() + } + + /** + * Whether the endpoint demonstrably no longer resolves to this daemon. Only positive evidence + * counts — treating an unreadable stat as loss would refuse sessions on a healthy daemon. + */ + private hasLostEndpointOwnership(): boolean { + if (this.endpointOwnershipLost) { + return true } - this.log.log('endpoint-ownership-lost', { socketPath: this.socketPath }) - console.warn( - '[daemon] Endpoint ownership lost to another daemon — retiring once existing sessions end' - ) + return this.observeEndpointOwnership() === 'lost' + } + + /** + * Reads ownership and keeps the watchdog's loss streak honest. Both callers come through here + * because the watchdog retires on *consecutive* losses: a read that skipped the streak let two + * losses separated by an owned observation count as consecutive, poisoning a healthy daemon. + */ + private observeEndpointOwnership(): DaemonEndpointOwnershipState { + // The running check, not just `shutdownPromise`: both shutdown routes close the server before + // assigning it, and in that window the listener is gone while the recorded identity is not. + if ( + process.platform === 'win32' || + !this.ownedSocketIdentity || + this.idleShutdownState !== 'running' + ) { + return 'indeterminate' + } + const state = readDaemonEndpointOwnershipState(this.socketPath, this.ownedSocketIdentity) + if (state !== 'lost') { + this.endpointOwnershipLossStreak = 0 + } + return state + } + + private requestRetirementForLostEndpoint(): void { + // Recorded before any dedup and not gated on retirement: folding the two together left the + // loss unrecorded when already retiring, and a later hello returned the daemon to service. + const alreadyLost = this.endpointOwnershipLost + this.endpointOwnershipLost = true this.ownedSocketIdentity = null + if (!alreadyLost) { + this.log.log('endpoint-ownership-lost', { socketPath: this.socketPath }) + console.warn( + '[daemon] Endpoint ownership lost to another daemon — retiring once existing sessions end' + ) + } this.retirementRequested = true + // The identity it compares against was just cleared, so every later tick early-returns. + this.stopEndpointOwnershipWatch() this.reevaluateIdleShutdown() } @@ -436,21 +519,26 @@ export class DaemonServer { return new Promise((resolve) => { // Why: close synchronously before any awaited cleanup so no new transport enters after the empty proof. server.close(() => { - // Why: libuv unlinks the path this server bound, which is our private bind name and - // is already gone. The canonical endpoint is removed by unlinkOwnedEndpointArtifacts, - // under an ownership check, so closing late cannot delete a replacement's endpoint. + // Why: libuv unlinks the path this server bound, which is our private bind name and is + // already gone. Nothing removes the canonical endpoint — a departing daemon leaves it + // for the next publisher to replace — so closing late cannot delete a replacement's. resolve() }) }) } private isIdle(): boolean { - return ( - this.transportSockets.size === 0 && - this.clients.size === 0 && - this.createOrAttachInFlight === 0 && - this.host.listSessions().length === 0 - ) + if (this.createOrAttachInFlight > 0 || this.host.listSessions().length > 0) { + return false + } + // Why open connections stop counting once the endpoint is lost: they belong to clients that + // reached this daemon before the takeover, and holding them open cannot make it reachable to + // anyone new. Waiting for them turns a drained retirement into an orphan that outlives its + // usefulness — the sessions are gone and nothing can route a new one here. + if (this.endpointOwnershipLost) { + return true + } + return this.transportSockets.size === 0 && this.clients.size === 0 } private reevaluateIdleShutdown(): void { @@ -651,10 +739,15 @@ export class DaemonServer { client.authenticatedPairEstablished = true // Why: one-shot health probes authenticate only a control socket; they are not fresh app activity. this.onAuthenticatedClientPair() - // A complete app connection (unlike a probe) re-owns the endpoint and cancels pending retirement. + // A complete app connection (unlike a probe) re-owns the endpoint and cancels pending + // retirement — but not a retirement caused by losing the endpoint itself. A client that + // connected before the takeover cannot make this daemon reachable again, and treating it + // as re-ownership would reopen session creation on a daemon nothing can find. this.initialAdoptionDeadlineMs = null - this.retirementRequested = false this.cancelInitialAdoptionTimer() + if (!this.endpointOwnershipLost) { + this.retirementRequested = false + } } } @@ -880,9 +973,21 @@ export class DaemonServer { // Why: a control-only replacement can't own terminal admission or erase the prior client's retirement request. throw new Error('Daemon client connection is incomplete; reconnect') } - this.createOrAttachInFlight++ const p = request.payload const attachOnly = p.attachOnly === true + // Why check here and not only on the watchdog: publishing cannot be made atomic against + // a publisher preempted between proving an entry dead and replacing it, so this daemon + // can lose the endpoint at any moment. The watchdog notices within a poll, which is far + // too late if a session was accepted in between — that session is then reachable by + // nobody, and the user sees a terminal that acknowledges input and never runs it. + // Why creation only: an attach reaches a session this daemon already hosts, over a + // connection that already exists. Refusing that would break the drain a retiring daemon + // depends on, and it strands nothing — the session is already here. + if (!attachOnly && this.hasLostEndpointOwnership()) { + this.requestRetirementForLostEndpoint() + throw new Error(DAEMON_ENDPOINT_LOST_MESSAGE) + } + this.createOrAttachInFlight++ let routedSessionId = p.sessionId let result: Awaited> try { diff --git a/src/main/daemon/daemon-spawner.test.ts b/src/main/daemon/daemon-spawner.test.ts index 0c5b98785..e80da4ac2 100644 --- a/src/main/daemon/daemon-spawner.test.ts +++ b/src/main/daemon/daemon-spawner.test.ts @@ -1,12 +1,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { randomUUID } from 'node:crypto' import { tmpdir } from 'node:os' -import { join } from 'node:path' -import { chmodSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { basename, join } from 'node:path' +import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { createServer, connect, type Server } from 'node:net' import { DaemonSpawner, + getDaemonArtifactHoldClaimPath, getDaemonPidPath, + getDaemonPidSwapClaimPath, getDaemonSocketPath, getDaemonTokenPath, publishDaemonPidFile, @@ -15,10 +16,10 @@ import { } from './daemon-spawner' import { getDaemonSocketBindPath, - publishDaemonSocketPath, - sweepAbandonedDaemonClaims, - unlinkOwnedDaemonSocketPath + publishDaemonEndpoint, + readDaemonSocketIdentity } from './daemon-endpoint-ownership' +import { probeSocketConnect } from './daemon-endpoint-probe' import { startDaemon, type DaemonHandle } from './daemon-main' import { DaemonClient } from './client' import type { SubprocessHandle } from './session' @@ -351,6 +352,27 @@ function connectsToSocketPath(socketPath: string): Promise { } describe('daemon socket publication', () => { + it('keeps every scratch namespace outside the released sweeper pattern', () => { + // Why pinned, and why all three: builds already in the field sweep these names on age alone + // with no liveness or ownership check, and deleting our sweeper does not un-ship theirs. A + // bind name is a live listener's only pathname; a claim briefly holds the only copy of a + // live daemon's token or PID record. Renaming any of them back into the released pattern + // must fail here rather than in the field. + const RELEASED_SWEEPER_PATTERN = + /(?:\.(?:cleanup|replace)-\d+-[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|^\.b[0-9a-f]{10})$/ + + for (let i = 0; i < 20; i++) { + const names = [ + basename(getDaemonSocketBindPath(getDaemonSocketPath('/tmp/orca-daemon'))), + basename(getDaemonPidSwapClaimPath('/tmp/orca-daemon/daemon-v32.pid')), + basename(getDaemonArtifactHoldClaimPath('/tmp/orca-daemon/daemon-v32.token')) + ] + for (const name of names) { + expect(name).not.toMatch(RELEASED_SWEEPER_PATTERN) + } + } + }) + it('keeps the bind name shorter than the canonical endpoint', () => { // sockaddr_un caps the path, so the private bind name must never extend it. const canonicalPath = getDaemonSocketPath('/tmp/orca-daemon-runtime') @@ -358,101 +380,91 @@ describe('daemon socket publication', () => { expect(getDaemonSocketBindPath(canonicalPath).length).toBeLessThan(canonicalPath.length) }) - it('publishes a bound listener under the canonical endpoint and reclaims only its own entry', async () => { - if (process.platform === 'win32') { - return - } - const dir = createTestDir() - const canonicalPath = getDaemonSocketPath(dir) - const first = createServer((socket) => socket.end()) - const second = createServer((socket) => socket.end()) - try { - const firstBindPath = getDaemonSocketBindPath(canonicalPath) - await listenOnSocketPath(first, firstBindPath) - - const firstIdentity = publishDaemonSocketPath(firstBindPath, canonicalPath) - expect(firstIdentity).not.toBeNull() - expect(existsSync(canonicalPath)).toBe(true) - expect(existsSync(firstBindPath)).toBe(false) - await expect(connectsToSocketPath(canonicalPath)).resolves.toBe(true) - - const secondBindPath = getDaemonSocketBindPath(canonicalPath) - await listenOnSocketPath(second, secondBindPath) - let publishError: NodeJS.ErrnoException | null = null + it.skipIf(process.platform === 'win32')( + 'keeps a live incumbent reachable when a second listener publishes', + async () => { + const dir = createTestDir() + const canonicalPath = getDaemonSocketPath(dir) + const incumbent = createServer((socket) => socket.end()) + const newcomer = createServer((socket) => socket.end()) try { - publishDaemonSocketPath(secondBindPath, canonicalPath) - } catch (error) { - publishError = error as NodeJS.ErrnoException + const incumbentBind = getDaemonSocketBindPath(canonicalPath) + await listenOnSocketPath(incumbent, incumbentBind) + const incumbentOutcome = await publishDaemonEndpoint( + incumbentBind, + canonicalPath, + probeSocketConnect + ) + expect(incumbentOutcome.status).toBe('published') + const incumbentIdentity = readDaemonSocketIdentity(canonicalPath) + + const newcomerBind = getDaemonSocketBindPath(canonicalPath) + await listenOnSocketPath(newcomer, newcomerBind) + await expect( + publishDaemonEndpoint(newcomerBind, canonicalPath, probeSocketConnect) + ).resolves.toEqual({ status: 'occupied' }) + + expect(readDaemonSocketIdentity(canonicalPath)).toEqual(incumbentIdentity) + await expect(connectsToSocketPath(canonicalPath)).resolves.toBe(true) + } finally { + await closeSocketServer(incumbent) + await closeSocketServer(newcomer) + rmSync(dir, { recursive: true, force: true }) } - expect(publishError?.code).toBe('EEXIST') - - expect(unlinkOwnedDaemonSocketPath(canonicalPath, firstIdentity)).toBe(true) - expect(existsSync(canonicalPath)).toBe(false) - - // The endpoint name now resolves to a different listener's inode. - const secondIdentity = publishDaemonSocketPath(secondBindPath, canonicalPath) - expect(secondIdentity).not.toEqual(firstIdentity) - expect(unlinkOwnedDaemonSocketPath(canonicalPath, firstIdentity)).toBe(false) - expect(existsSync(canonicalPath)).toBe(true) - await expect(connectsToSocketPath(canonicalPath)).resolves.toBe(true) - } finally { - await closeSocketServer(first) - await closeSocketServer(second) - rmSync(dir, { recursive: true, force: true }) } - }) -}) - -describe('sweepAbandonedDaemonClaims', () => { - const claimNames = [ - `daemon-v${PROTOCOL_VERSION}.pid.cleanup-123-${randomUUID()}`, - `daemon-v${PROTOCOL_VERSION}.pid.replace-123-${randomUUID()}`, - '.b0123456789' - ] - const preservedNames = [`daemon-v${PROTOCOL_VERSION}.pid`, `daemon-v${PROTOCOL_VERSION}.token`] - - function seedClaimDir(): string { - const dir = createTestDir() - for (const name of [...claimNames, ...preservedNames]) { - writeFileSync(join(dir, name), 'x') - } - return dir - } - - it('removes aged claim and bind scratch names without touching daemon artifacts', () => { - const dir = seedClaimDir() - try { - expect(sweepAbandonedDaemonClaims(dir, undefined, Date.now() + 24 * 60 * 60 * 1000)).toBe( - claimNames.length - ) - - for (const name of claimNames) { - expect(existsSync(join(dir, name))).toBe(false) - } - for (const name of preservedNames) { - expect(existsSync(join(dir, name))).toBe(true) - } - } finally { - rmSync(dir, { recursive: true, force: true }) - } - }) - - it('leaves freshly written claims alone so an in-flight claim is never stolen', () => { - const dir = seedClaimDir() - try { - expect(sweepAbandonedDaemonClaims(dir)).toBe(0) - - for (const name of [...claimNames, ...preservedNames]) { - expect(existsSync(join(dir, name))).toBe(true) - } - } finally { - rmSync(dir, { recursive: true, force: true }) - } - }) - - it('returns zero for an unreadable runtime dir', () => { - expect(sweepAbandonedDaemonClaims(join(tmpdir(), `daemon-sweep-missing-${randomUUID()}`))).toBe( - 0 - ) - }) + ) + + it.skipIf(process.platform === 'win32')( + 'leaves an unclassifiable incumbent untouched', + async () => { + const dir = createTestDir() + const canonicalPath = getDaemonSocketPath(dir) + const newcomer = createServer((socket) => socket.end()) + try { + writeFileSync(canonicalPath, 'incumbent') + const newcomerBind = getDaemonSocketBindPath(canonicalPath) + await listenOnSocketPath(newcomer, newcomerBind) + + await expect( + publishDaemonEndpoint(newcomerBind, canonicalPath, async () => 'unknown') + ).resolves.toEqual({ status: 'inconclusive' }) + expect(readFileSync(canonicalPath, 'utf8')).toBe('incumbent') + } finally { + await closeSocketServer(newcomer) + rmSync(dir, { recursive: true, force: true }) + } + } + ) + + it.skipIf(process.platform === 'win32')( + 'replaces a dead incumbent with a reachable listener', + async () => { + const dir = createTestDir() + const canonicalPath = getDaemonSocketPath(dir) + const incumbent = createServer((socket) => socket.end()) + const replacement = createServer((socket) => socket.end()) + try { + const incumbentBind = getDaemonSocketBindPath(canonicalPath) + await listenOnSocketPath(incumbent, incumbentBind) + await publishDaemonEndpoint(incumbentBind, canonicalPath, probeSocketConnect) + await closeSocketServer(incumbent) + await expect(connectsToSocketPath(canonicalPath)).resolves.toBe(false) + + const replacementBind = getDaemonSocketBindPath(canonicalPath) + await listenOnSocketPath(replacement, replacementBind) + const outcome = await publishDaemonEndpoint( + replacementBind, + canonicalPath, + probeSocketConnect + ) + + expect(outcome.status).toBe('published') + await expect(connectsToSocketPath(canonicalPath)).resolves.toBe(true) + } finally { + await closeSocketServer(incumbent) + await closeSocketServer(replacement) + rmSync(dir, { recursive: true, force: true }) + } + } + ) }) diff --git a/src/main/daemon/daemon-spawner.ts b/src/main/daemon/daemon-spawner.ts index 544245c52..6a6d377cc 100644 --- a/src/main/daemon/daemon-spawner.ts +++ b/src/main/daemon/daemon-spawner.ts @@ -132,8 +132,25 @@ export function publishDaemonPidFile(pidPath: string, pidFile: DaemonPidFile): v }) } +/** + * Scratch names for the two claim protocols. + * + * Why `.swap`/`.hold` and not the `.cleanup`/`.replace` these once were: released builds carry a + * sweeper matching `\.(?:cleanup|replace)-\d+-$` that deletes on age alone, with no + * liveness or ownership check. A claim briefly holds the ONLY copy of a live daemon's token or + * PID record, so an old build starting while a claimant is paused would destroy it with no way + * to restore. Exported so a test can pin them against that released pattern. + */ +export function getDaemonPidSwapClaimPath(pidPath: string): string { + return `${pidPath}.swap-${process.pid}-${randomUUID()}` +} + +export function getDaemonArtifactHoldClaimPath(filePath: string): string { + return `${filePath}.hold-${process.pid}-${randomUUID()}` +} + export function replaceDaemonPidFile(pidPath: string, pidFile: DaemonPidFile): boolean { - const claimedPath = `${pidPath}.replace-${process.pid}-${randomUUID()}` + const claimedPath = getDaemonPidSwapClaimPath(pidPath) let claimedExisting = false try { renameSync(pidPath, claimedPath) @@ -174,21 +191,47 @@ export function replaceDaemonPidFile(pidPath: string, pidFile: DaemonPidFile): b export function unlinkOwnedDaemonPidFile( pidPath: string, expectedPid: number, - expectedLaunchNonce: string + // Why: records written before launch nonces existed carry none. Matching on PID alone is + // weaker, but it still fences against removing a replacement's record, which is the point. + expectedLaunchNonce: string | null ): boolean { return claimAndUnlinkOwnedFile(pidPath, (content) => { try { - const parsed = JSON.parse(content) as { - pid?: unknown - launchNonce?: unknown + const parsed: unknown = JSON.parse(content.trim()) + // Why: the oldest records are a bare integer, not an object. Rejecting them left the + // file in place, and the replacement's exclusive publish then failed with EEXIST — + // trading a stale record for a daemon that cannot start at all. + if (typeof parsed === 'number') { + return expectedLaunchNonce === null && parsed === expectedPid } - return parsed.pid === expectedPid && parsed.launchNonce === expectedLaunchNonce + if (!parsed || typeof parsed !== 'object') { + return false + } + const record = parsed as { pid?: unknown; launchNonce?: unknown } + if (record.pid !== expectedPid) { + return false + } + return expectedLaunchNonce === null + ? record.launchNonce === undefined || record.launchNonce === null + : record.launchNonce === expectedLaunchNonce } catch { return false } }) } +/** + * Removes a PID record whose content still satisfies `matches`, under the same rename claim + * used for owned records. Lets an unparseable record be reclaimed without risking a valid + * replacement record that appeared in the meantime. + */ +export function unlinkDaemonPidFileWhen( + pidPath: string, + matches: (content: string) => boolean +): boolean { + return claimAndUnlinkOwnedFile(pidPath, matches) +} + export function unlinkOwnedDaemonTokenFile(tokenPath: string, expectedToken: string): boolean { return claimAndUnlinkOwnedFile(tokenPath, (content) => content.trim() === expectedToken) } @@ -197,7 +240,7 @@ function claimAndUnlinkOwnedFile( filePath: string, ownsContent: (content: string) => boolean ): boolean { - const claimedPath = `${filePath}.cleanup-${process.pid}-${randomUUID()}` + const claimedPath = getDaemonArtifactHoldClaimPath(filePath) try { // Why: rename claims one exact directory entry before inspection, so a replacement // installed afterward stays at the canonical path and cannot be unlinked by us. diff --git a/tests/e2e/daemon-lifecycle-retirement.spec.ts b/tests/e2e/daemon-lifecycle-retirement.spec.ts index f52208899..d081c0dd1 100644 --- a/tests/e2e/daemon-lifecycle-retirement.spec.ts +++ b/tests/e2e/daemon-lifecycle-retirement.spec.ts @@ -1,6 +1,7 @@ import { fork, type ChildProcess } from 'node:child_process' import { randomUUID } from 'node:crypto' import { existsSync, mkdirSync, mkdtempSync, rmSync } from 'node:fs' +import { connect } from 'node:net' import { tmpdir } from 'node:os' import path from 'node:path' import { build } from 'esbuild' @@ -14,6 +15,20 @@ import { } from '../../src/main/daemon/daemon-spawner' import { PROTOCOL_VERSION } from '../../src/main/daemon/types' +// Why a connect and not existsSync: a departing daemon leaves its endpoint entry on disk by +// design, so presence no longer distinguishes a live daemon from a dead one. +function connectsTo(socketPath: string): Promise { + return new Promise((resolve) => { + const socket = connect({ path: socketPath }) + const settle = (reachable: boolean): void => { + socket.destroy() + resolve(reachable) + } + socket.once('connect', () => settle(true)) + socket.once('error', () => settle(false)) + }) +} + type FixtureDaemon = { child: ChildProcess protocolVersion: number @@ -213,7 +228,11 @@ test('v22 stays reattachable while v24 retires after its last empty client disco expect(existsSync(current.tokenPath)).toBe(false) expect(existsSync(current.pidPath)).toBe(false) if (process.platform !== 'win32') { - expect(existsSync(current.socketPath)).toBe(false) + // Why reachability and not absence: a departing daemon deliberately leaves its endpoint + // entry behind for the next publisher to replace in one rename. Removing it would mean + // fencing that removal against a replacement, which is the defect class this design + // retired. What must be true is that nothing answers there any more. + await expect(connectsTo(current.socketPath)).resolves.toBe(false) } expect(legacy.child.exitCode).toBeNull() } catch (error) {