diff --git a/src/dns-system.mjs b/src/dns-system.mjs index 390d31f..d71e3c8 100644 --- a/src/dns-system.mjs +++ b/src/dns-system.mjs @@ -362,14 +362,32 @@ export async function readPid(path = pidfilePath()) { } } -/** Is that pid actually ours and alive? A stale pidfile must not read as running. */ -export function isAlive(pid) { +/** + * Is that pid alive? A stale pidfile must not read as running. + * + * "Alive" and "ours" are different questions, and answering the first with the + * second cost a machine its resolver. `process.kill(pid, 0)` fails two ways: + * ESRCH for a pid that is gone, and EPERM for one that is there but belongs to + * another user. Catching both as "dead" was wrong in precisely the case this + * tool manufactures — `dns enable` escalates, so the bridge is root's while + * every later `status` asking after it is not. + * + * A live root-owned bridge therefore read as a stale pidfile for the rest of + * its life: `stop` deleted the file and reported it cleared while the daemon + * kept running, and `start` saw nothing there and put a second bridge on + * 127.0.0.1 underneath the working one on 0.0.0.0 — the shadowing outage + * `bridgePresence` describes, arrived at by believing our own liveness check. + * + * So only ESRCH is dead. EPERM is alive and someone else's, which the caller + * needs told rather than papered over. + */ +export function isAlive(pid, kill = (target) => process.kill(target, 0)) { if (!pid) return false; try { - process.kill(pid, 0); + kill(pid); return true; - } catch { - return false; + } catch (error) { + return error?.code === "EPERM"; } } diff --git a/test/dns-daemon-verify.test.mjs b/test/dns-daemon-verify.test.mjs index 755a6c0..daf323a 100644 --- a/test/dns-daemon-verify.test.mjs +++ b/test/dns-daemon-verify.test.mjs @@ -305,3 +305,45 @@ test("an already-running daemon still short-circuits without spawning", async () }); assert.deepEqual(result, { started: false, pid: process.pid, alreadyRunning: true }); }); + +/* ----------------------------------------------- alive, but not ours */ + +// `process.kill(pid, 0)` has two failure modes and they mean opposite things. +// Collapsing them is not academic: `dns enable` escalates, so the bridge it +// starts is root's and every unprivileged `status` afterwards got EPERM, +// called it dead, and advised a fix that starts a second bridge on top of the +// live one. Observed as `moshcode dns status` reporting "stale pidfile for +// 641911" on a Kubuntu desktop whose bridge was running the whole time. + +const errno = (code) => Object.assign(new Error(code), { code }); + +test("EPERM means alive and someone else's, not dead", () => { + const denied = () => { + throw errno("EPERM"); + }; + assert.equal(isAlive(4242, denied), true, "a pid we may not signal is still a pid that exists"); +}); + +test("ESRCH is the only failure that means dead", () => { + const gone = () => { + throw errno("ESRCH"); + }; + assert.equal(isAlive(4242, gone), false); +}); + +test("a real process this test cannot signal reads as alive", () => { + // pid 1 always exists and, unprivileged, cannot be signalled — the exact + // shape of a root-owned bridge. Running as root it simply succeeds, so this + // holds either way rather than depending on who runs the suite. + assert.equal(isAlive(1), true); +}); + +test("a pidfile naming another user's live process is running, not stale", async () => { + const dir = await scratch(); + const path = join(dir, "moshpit-dns.pid"); + await writeFile(path, "1\n"); + + // The whole bug in one assertion: `stale: true` here is what sent people to + // `dns enable` and took their resolver down. + assert.deepEqual(await daemonStatus(path), { running: true, pid: 1, stale: false }); +});