diff --git a/docs/gotchas.md b/docs/gotchas.md index 5e8dc75..6d149a2 100644 --- a/docs/gotchas.md +++ b/docs/gotchas.md @@ -11,6 +11,8 @@ one, recorded here so they are not reintroduced. | Sensitive paths in access logs | Payload sent as a GET query string | POST bodies only | | A subprocess's log lines flash and vanish | Child output and a once-a-second `ESC[H ESC[J` repaint share stdout | Buffer the child's lines while the display owns the screen, print the tail on every failure path — never silence them | | The screen claims a tunnel is up while it is dying | `^C` signals the whole process group, so the child begins shutdown before the draw timer is cleared | Clear the block in teardown; a stale frame asserting the opposite is worse than no frame | +| `stop` says "not running" while the port stays held, for ever | ONE state file per config dir, but paddock is per-PORT: a second instance (`PADDOCK_PORT=…`, a `--demo`, `paddock tunnel`) overwrote the first's record on start, and the first to EXIT deleted the file outright — so the instance holding the dashboard's port became permanently untrackable, and `stop && start` walked into "port already in use" | First instance wins: `recordState` refuses to write over a *running* record for a different pid, and servers clear their record with `removeOwnState`, which deletes only their own. `status`/`stop` probe the port when no record exists and name what is serving, because "not running" was a lie told about a live process | +| `paddock update` looks complete but the dashboard stays on the old version | Replacing the binary does not restart the process running it — `/proc//exe` reads "… (deleted)" and it serves the old build until bounced | `update` reports the pid, port and version still running and the exact restart command. Told, NOT done: restarting would drop every connected phone mid-session to finish a command run for the binary's sake | | A shutdown the operator ASKED for is reported as the tunnel failing | Same process group: the child dies at the same moment the teardown runs, and the race watching `child.exited` cannot tell a requested death from a crash — so `^C` printed `cloudflared exited 143 — the URL is gone` plus a tail that, on a tunnel up for half an hour, was its SUCCESSFUL startup prechecks | Gate that branch on `stopping`. Quieting a diagnosis is only safe where there is nothing to diagnose: the live shutdown lines, the closing report, and a failed kill's warning and non-zero exit all remain | | The URL is the fourth line printed | Boot diagnostics log as they happen, and the port is not bound until after them | Collect boot facts, emit one summary line, then the banner (`boot-log.ts`) | | Service worker silently disabled | Auth check gates every route including `/sw.js` | No app token; Access is the gate — its cookie rides a same-origin fetch, a bearer token has nothing to ride | diff --git a/src/server/index.ts b/src/server/index.ts index 5974478..cc8e982 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -21,7 +21,7 @@ import { hubWebSocket, tryUpgradeWs, type WsData } from "@server/ws/serve"; import { publicHostsFrom } from "@server/origin"; import { buildIdFrom } from "@server/build-id"; import { SettingsStore, defaultConfigDir, isConfigured } from "@server/settings/store"; -import { recordState, removeState } from "@server/lifecycle/state"; +import { checkState, recordState, removeOwnState } from "@server/lifecycle/state"; import { runStart, runStatus, runStop } from "@server/lifecycle/commands"; import { sendTelegram } from "@server/notify/telegram"; import { Notifier, fanOut } from "@server/notify/notifier"; @@ -103,6 +103,20 @@ if (command === "update") { arch: process.arch, current: VERSION, checkOnly: flags.has("--check"), + /** + * Whatever is still serving the binary that is about to be replaced. The + * state file is the only thing that knows, and `update.ts` deliberately + * does not read it — see `UpdateOpts.running`. + */ + running: async () => { + // Default logger, NOT a silencer: `checkState` announces a state file + // it had to ignore, and swallowing that here would hide a broken record + // at the one moment an operator is already watching output. + const got = await checkState(defaultConfigDir()); + return got.kind === "running" + ? { pid: got.state.pid, port: got.state.port, version: got.state.version } + : null; + }, })); } @@ -110,14 +124,16 @@ if (command === "update") { // `status` should not open a herdr socket or bind a port just to answer a // question that only needs the state file and a signal-0 probe. if (command === "status") { - process.exit(await runStatus({ dir: defaultConfigDir() })); + process.exit(await runStatus({ dir: defaultConfigDir(), port: PORT })); } // Must run before any server setup below, same reasoning as `update` and // `status` above: `stop` should not open a herdr socket or bind a port just // to signal a pid it reads from the state file. if (command === "stop") { - process.exit(await runStop({ dir: defaultConfigDir(), force: flags.has("--force") })); + process.exit(await runStop({ + dir: defaultConfigDir(), port: PORT, force: flags.has("--force"), + })); } // Must run before any server setup below, same reasoning as the three verbs @@ -731,7 +747,7 @@ for (const signal of ["SIGINT", "SIGTERM"] as const) { console.error(`paddock: shutdown step failed (${String(e)})`); code = 1; } - await removeState(stateDir).catch((e) => + await removeOwnState(stateDir, process.pid).catch((e) => console.error(`paddock: could not clear state file (${String(e)})`), ); process.exit(code); @@ -774,10 +790,10 @@ if (command === "tunnel") { // `runTunnel` turns the failures it recognises into exit codes, so this is // the unrecognised remainder. The state file is cleared BEFORE the rethrow // regardless: a throw out of a top-level await ends the process without - // running either `removeState` below, and the residue is a state file + // running either `removeOwnState` below, and the residue is a state file // describing a process that has gone — which `paddock status` then reports // as running. The error itself is rethrown untouched, stack and all. - await removeState(stateDir).catch((e) => + await removeOwnState(stateDir, process.pid).catch((e) => console.error(`paddock: could not clear state file (${String(e)})`), ); throw err; @@ -786,7 +802,7 @@ if (command === "tunnel") { // cloudflared died. A Ctrl-C exits from the handler above instead. The state // file is cleared here too, or an instance that closed its own tunnel would // leave `paddock status` describing a process that no longer exists. - await removeState(stateDir).catch((e) => + await removeOwnState(stateDir, process.pid).catch((e) => console.error(`paddock: could not clear state file (${String(e)})`), ); process.exit(code); diff --git a/src/server/lifecycle/commands.ts b/src/server/lifecycle/commands.ts index 8675018..e2e5e63 100644 --- a/src/server/lifecycle/commands.ts +++ b/src/server/lifecycle/commands.ts @@ -11,9 +11,71 @@ import { SettingsStore } from "@server/settings/store"; import { tunnelHint } from "@server/tunnel/preflight"; import { say } from "@server/term"; +/** + * What answers on a port, if anything paddock-shaped does. + * + * `null` for "nothing there" — the ordinary case, and the reason the default + * implementation catches: nothing listening is ECONNREFUSED, which is an + * ANSWER here, not a fault. It refuses to call a stranger paddock: only a + * `/api/health` body with `ok: true` and a version string counts, so an + * unrelated server on the port is reported as nothing rather than as an + * instance the operator should go hunting for. + */ +export type Listener = (port: number) => Promise<{ version: string } | null>; + +export const httpListener: Listener = async (port) => { + try { + const res = await fetch(`http://127.0.0.1:${port}/api/health`, { + signal: AbortSignal.timeout(1_500), + }); + if (!res.ok) return null; + const body = (await res.json()) as { ok?: unknown; version?: unknown }; + if (body.ok !== true || typeof body.version !== "string") return null; + return { version: body.version }; + } catch { + return null; + } +}; + +/** + * Report an instance that is serving but that NO record describes, or `false` + * if there is no such thing. + * + * "paddock — not running" is a lie when something is holding the port, and it + * is the lie that wasted the operator's time: `stop` said nothing was running, + * so `start` was the obvious next move, and it failed on a port they had just + * been told was free. A record goes missing while its process lives when the + * process was SIGKILLed, or — before the ownership guards in `state.ts` — when + * another paddock run deleted it. + * + * No signal is sent. The pid is not known here (the record is what carried it, + * and it is gone) and finding a pid from a port is platform-specific, so this + * hands over the command that does it rather than guessing at a kill. + */ +async function reportUntracked( + port: number, + listener: Listener, + log: (line: string) => void, +): Promise { + const found = await listener(port); + if (found === null) return false; + log(`paddock ${found.version} — serving on 127.0.0.1:${port}, but NOT tracked`); + log(" Nothing on disk describes it, so paddock cannot stop it. Its state"); + log(" file is gone: killed with SIGKILL, or removed by another paddock run"); + log(" on this machine (a bug in versions before 0.8.3)."); + log(" Find it and stop it by hand:"); + log(` ss -ltnp | grep :${port} # Linux`); + log(` lsof -nP -iTCP:${port} -sTCP:LISTEN # macOS`); + return true; +} + export interface StatusOpts { dir: string; + /** The port an untracked instance would be holding. */ + port?: number; probe?: Probe; + /** Injected so a test can drive the untracked-instance path. */ + listener?: Listener; log?: (line: string) => void; now?: () => number; } @@ -59,6 +121,8 @@ export async function runStatus(o: StatusOpts): Promise { switch (got.kind) { case "none": + if (o.port !== undefined && + await reportUntracked(o.port, o.listener ?? httpListener, log)) return 1; log("paddock — not running"); return 1; case "unreadable": @@ -96,6 +160,10 @@ export async function runStatus(o: StatusOpts): Promise { export interface StopOpts { dir: string; + /** The port an untracked instance would be holding. */ + port?: number; + /** Injected so a test can drive the untracked-instance path. */ + listener?: Listener; force?: boolean; probe?: Probe; log?: (line: string) => void; @@ -146,6 +214,11 @@ export async function runStop(o: StopOpts): Promise { switch (got.kind) { case "none": + // Non-zero when something IS there: `paddock stop && paddock start` must + // halt here, with the reason, instead of walking into a bind failure on + // a port it was just told was free. + if (o.port !== undefined && + await reportUntracked(o.port, o.listener ?? httpListener, log)) return 1; log("paddock — not running"); return 0; case "unreadable": diff --git a/src/server/lifecycle/state.ts b/src/server/lifecycle/state.ts index 1153b51..7879073 100644 --- a/src/server/lifecycle/state.ts +++ b/src/server/lifecycle/state.ts @@ -92,6 +92,8 @@ export async function writeState(dir: string, s: PaddockState): Promise { export interface RecordStateDeps { /** Injected so a test can drive the "cannot identify myself" path. */ capture?: (pid: number) => string | null; + /** Injected so a test can drive the "someone else is already here" path. */ + probe?: Probe; log?: (line: string) => void; warn?: (line: string) => void; } @@ -121,6 +123,58 @@ export async function recordState( const log = deps.log ?? console.info; const warn = deps.warn ?? termWarn; + /** + * ONE state file, MANY possible instances. This is the check that keeps the + * file describing an instance that actually exists. + * + * paddock is per-PORT — `PADDOCK_PORT=8788 paddock`, a `--demo` on a spare + * port, a dev or test server — but the state file is per CONFIG DIR. (NOT + * `paddock tunnel`: it serves the dashboard itself and its preflight refuses + * to start beside a recorded instance.) And the operator is walked into it by + * paddock's OWN advice: "port 8787 is already in use … choose another port: + * PADDOCK_PORT=8788 paddock". Writing unconditionally meant the second + * instance + * to start silently took over the record of the first, and (with the + * matching `removeOwnState`) the first to EXIT deleted the file outright. + * The instance still holding the dashboard's port was then untrackable for + * the rest of its life: `paddock status` and `paddock stop` both answered + * "not running" while the port stayed bound and `paddock start` refused it. + * Reproduced end to end; the operator's report was "always have this issue". + * + * FIRST INSTANCE WINS, and the loser is told. Only a `running` record for a + * DIFFERENT pid blocks the write: a `stale` record (the ordinary restart), a + * `mismatch`, garbage, or this pid's own earlier record must all still be + * claimable, or one leftover file would lock out every future start. + */ + // Wrapped for the same reason as the capture below, and it is not + // theoretical: `checkState` calls `probe.isAlive` and `probe.argsOf`, and + // the default probe reaches for `ps` where /proc does not exist — which + // THROWS when `ps` is absent. This runs at top level right after the bind, + // so an escaping rejection would kill a paddock that is already serving. + // + // "Cannot tell who holds the record" is answered as a CONFLICT, not as a + // free pass: overwriting on an error we could not interpret is exactly the + // mis-tracking this check exists to stop, and this function's stated + // preference is to be untracked-and-announced over mis-tracked. + let held: StateCheck; + try { + held = await checkState(dir, deps.probe ?? systemProbe, log); + } catch (e) { + warn( + `paddock: could not check for an instance already recorded in ${dir} (${String(e)}) — ` + + `not recording pid ${s.pid}, so \`paddock status\` and \`paddock stop\` will not find it`, + ); + return false; + } + if (held.kind === "running" && held.state.pid !== s.pid) { + warn( + `paddock: pid ${held.state.pid} is already recorded on port ${held.state.port} — ` + + `not recording pid ${s.pid} on port ${s.port} over it, so \`paddock status\` and ` + + "`paddock stop` will keep pointing at the older instance", + ); + return false; + } + // Inside the guard, not outside it: `capturedArgs` falls back to // Bun.spawnSync(["ps", ...]), which THROWS if `ps` is absent rather than // returning a non-zero exit. This function is called at top level right @@ -160,6 +214,29 @@ export async function removeState(dir: string): Promise { await rm(stateFile(dir), { force: true }); } +/** + * Remove the state file only if it describes THIS process. + * + * What every exiting SERVER must call. `removeState` stays unconditional for + * `stop` and `status`, which delete only after deciding the record is stale, + * mismatched or theirs to clear — but an exiting server knows nothing about + * whose record is on disk, and a second-port instance shutting down used to + * delete the record of the instance still serving the dashboard. See the + * conflict check in `recordState` for the whole failure. + * + * Reads with `checkState` rather than parsing here, so "unusable file" and + * "no file" reach this the same way they reach everything else. Anything it + * cannot positively identify as this pid's own record is LEFT ALONE: a file + * this function is unsure about is exactly the file it must not delete. + */ +export async function removeOwnState(dir: string, pid: number): Promise { + const held = await checkState(dir, systemProbe, () => {}); + const mine = + (held.kind === "running" || held.kind === "stale" || held.kind === "mismatch") && + held.state.pid === pid; + if (mine) await removeState(dir); +} + export async function checkState( dir: string, probe: Probe = systemProbe, diff --git a/src/server/update.ts b/src/server/update.ts index 7b47e29..7f1258a 100644 --- a/src/server/update.ts +++ b/src/server/update.ts @@ -69,6 +69,13 @@ export interface UpdateOpts { checkOnly?: boolean; fetchImpl?: typeof fetch; log?: (s: string) => void; + /** + * The instance still serving, if any — pid, port, and the version IT is + * running. Injected rather than read here: this module knows about releases + * and binaries, and the state file belongs to `lifecycle/`. `index.ts` + * supplies the real one. + */ + running?: () => Promise<{ pid: number; port: number; version: string } | null>; } /** Returns a process exit code. Never throws for an expected failure. */ @@ -186,5 +193,34 @@ export async function runUpdate(o: UpdateOpts): Promise { return 1; } log(`paddock: updated to ${latest}`); + + /** + * A replaced binary does not restart the process running it. + * + * `update` swapped the file; an instance already serving keeps running from + * the REPLACED inode — `/proc//exe` reads "… (deleted)" — and goes on + * answering the old version until someone bounces it. Nothing said so, so + * the update looked complete while the dashboard stayed on the old build + * indefinitely. + * + * Told, not done. Restarting here would drop every connected phone mid + * session to finish a command the operator ran for the binary's sake, and a + * dashboard taken down without warning is a worse surprise than a version + * that lags until they choose the moment. + * + * Only when the running instance is on a DIFFERENT version: after the + * restart it reports the new one, and repeating the hint then would send an + * operator to bounce an instance that is already current. + */ + const live = await o.running?.(); + if (live !== null && live !== undefined && live.version !== latest) { + log(""); + log( + `paddock: pid ${live.pid} on port ${live.port} is still serving ${live.version} ` + + "from the replaced binary.", + ); + log(" restart it to pick this up:"); + log(" paddock stop && paddock start"); + } return 0; } diff --git a/tests/cli.test.ts b/tests/cli.test.ts index c26693b..10ca1a6 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -2,6 +2,7 @@ import { afterAll, expect, test } from "bun:test"; import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; +import { freePort } from "./support/port"; import { parseArgs, parseDuration, USAGE } from "@server/cli"; test("bare invocation serves — the Docker CMD and every doc depend on it", () => { @@ -190,6 +191,13 @@ function runVerb(verb: string, extraArgs: string[] = []) { PADDOCK_NO_UPDATE_CHECK: "1", PADDOCK_CONFIG_DIR: CONFIG, PADDOCK_HERDR_SOCKET: join(CONFIG, "no-such-herdr.sock"), + // An OS-assigned free port, not the default 8787. `status` and `stop` + // now probe the port for an instance no state file describes, and the + // default port is exactly where the developer's OWN paddock is + // listening — so inheriting it made this file's outcome depend on + // whether the machine happened to be running one. Same guess + // `docs/gotchas.md` already records for port ranges that "look unused". + PADDOCK_PORT: String(freePort()), }, stdout: "pipe", stderr: "pipe", diff --git a/tests/lifecycle-server-state.test.ts b/tests/lifecycle-server-state.test.ts index 98b062a..097298f 100644 --- a/tests/lifecycle-server-state.test.ts +++ b/tests/lifecycle-server-state.test.ts @@ -116,3 +116,55 @@ test("an unwritable config dir does not kill an already-bound paddock", async () const stderrText = await new Response(proc.stderr).text(); expect(stderrText).toContain("could not record state"); }, 60_000); + +test("a second paddock on ANOTHER port neither takes over nor deletes the first's state", async () => { + // THE ORPHAN BUG, at the level it actually bit: two instances that both bind + // fine. `PADDOCK_PORT=8788 paddock`, a `--demo` on a spare port and a dev + // server are separate serving processes sharing ONE state file, so + // B used to overwrite A's record on start and delete the file outright on + // exit. A then held its port for the rest of its life with nothing tracking + // it: `stop` said "not running", and `start` refused the port it held. + const cfg = await mkdtemp(join(tmpdir(), "paddock-cfg-")); + const base = { + ...process.env, PADDOCK_CONFIG_DIR: cfg, PADDOCK_NO_UPDATE_CHECK: "1", + }; + const portA = freePort(); + const portB = freePort(); + + const a = Bun.spawn(["bun", "src/server/index.ts", "--demo"], + { env: { ...base, PADDOCK_PORT: String(portA) }, stdout: "pipe", stderr: "pipe" }); + try { + let aState: string | null = null; + for (let i = 0; i < 60 && aState === null; i++) { + try { aState = await readFile(stateFile(cfg), "utf8"); } catch { await Bun.sleep(100); } + } + expect(aState, "instance A's state file never appeared").not.toBeNull(); + expect(JSON.parse(aState!).pid).toBe(a.pid); + + const b = Bun.spawn(["bun", "src/server/index.ts", "--demo"], + { env: { ...base, PADDOCK_PORT: String(portB) }, stdout: "pipe", stderr: "pipe" }); + // B really is serving — this is not the losing-bind case. + let bUp = false; + for (let i = 0; i < 60 && !bUp; i++) { + try { bUp = (await fetch(`http://127.0.0.1:${portB}/api/health`)).ok; } catch { await Bun.sleep(100); } + } + expect(bUp, "instance B never came up on its own port").toBe(true); + + // First instance wins the record. + expect(JSON.parse(await readFile(stateFile(cfg), "utf8")).pid).toBe(a.pid); + + b.kill("SIGTERM"); + await b.exited; + // And B's exit must leave A's record alone. This is the half that made the + // orphan permanent. + const afterB = await readFile(stateFile(cfg), "utf8"); + expect(JSON.parse(afterB).pid, "B's shutdown deleted or rewrote A's record").toBe(a.pid); + expect(JSON.parse(afterB).port).toBe(portA); + + // A is still both serving and trackable. + expect((await fetch(`http://127.0.0.1:${portA}/api/health`)).ok).toBe(true); + } finally { + a.kill("SIGTERM"); + await a.exited; + } +}, 60_000); diff --git a/tests/lifecycle-state.test.ts b/tests/lifecycle-state.test.ts index 7294cb1..7bf2094 100644 --- a/tests/lifecycle-state.test.ts +++ b/tests/lifecycle-state.test.ts @@ -7,6 +7,7 @@ import { capturedArgs, checkState, recordState, + removeOwnState, removeState, stateFile, writeState, @@ -263,3 +264,128 @@ test("an argsOf that throws is 'cannot tell', not 'someone else owns this pid'", "nothing may be deleted on a 'cannot tell'", ).toBe(true); }); + +// --- one state file, many instances ---------------------------------------- +// +// THE ORPHAN BUG. There is one state file per config dir, but paddock is +// per-port: `PADDOCK_PORT=8788 paddock`, a `--demo` on another port, and dev +// or test servers are all separate serving processes writing this same file. +// (`paddock tunnel` is NOT one of them — it serves the dashboard itself and +// refuses to start beside a recorded instance.) +// Unguarded, the second to start overwrote the first's record and the first to +// EXIT deleted the file outright — so a long-running instance became +// permanently untrackable, and `stop` answered "not running" while the port +// stayed held and `start` refused it. Reproduced end to end before this +// guard existed. + +test("recordState refuses to overwrite a DIFFERENT live instance's record", async () => { + const d = await dir(); + await writeState(d, state({ pid: 4242, port: 8787, args: "paddock" })); + const said: string[] = []; + + // A second instance, on another port, whose own identity captures fine. + const ok = await recordState( + d, + { pid: 5150, port: 8788, version: "0.8.2", startedAt: 1_700_000_000_001 }, + { + capture: () => "paddock", + // The incumbent is alive and is what it says it is. + probe: probe(true, "paddock"), + warn: (l) => said.push(l), + log: (l) => said.push(l), + }, + ); + + expect(ok).toBe(false); + // The incumbent's record is intact — that is the whole point. + expect(JSON.parse(await readFile(stateFile(d), "utf8")).pid).toBe(4242); + // And the refusal is announced: an untracked instance the operator was not + // told about is how this became invisible in the first place. + expect(said.join("\n")).toContain("8787"); + expect(said.join("\n")).toContain("4242"); +}); + +test("recordState still claims a record left by a DEAD instance", async () => { + // The ordinary restart. A stale record must not lock the file for ever. + const d = await dir(); + await writeState(d, state({ pid: 4242 })); + const ok = await recordState( + d, + { pid: 5150, port: 8787, version: "0.8.2", startedAt: 1 }, + { capture: () => "paddock", probe: probe(false, null), warn: () => {}, log: () => {} }, + ); + expect(ok).toBe(true); + expect(JSON.parse(await readFile(stateFile(d), "utf8")).pid).toBe(5150); +}); + +test("recordState overwrites its OWN earlier record", async () => { + // Same pid, re-recording: not a conflict, and refusing would leave a live + // instance describing itself with stale facts. + const d = await dir(); + await writeState(d, state({ pid: 4242, port: 8787, version: "0.8.1" })); + const ok = await recordState( + d, + { pid: 4242, port: 8787, version: "0.8.2", startedAt: 2 }, + { capture: () => "paddock", probe: probe(true, "paddock"), warn: () => {}, log: () => {} }, + ); + expect(ok).toBe(true); + expect(JSON.parse(await readFile(stateFile(d), "utf8")).version).toBe("0.8.2"); +}); + +test("removeOwnState leaves a record that belongs to another instance", async () => { + // The deletion half of the same bug. A demo or spare-port run exiting must not + // untrack the instance that actually holds the dashboard's port. + const d = await dir(); + await writeState(d, state({ pid: 4242, port: 8787 })); + await removeOwnState(d, 5150); + expect(existsSync(stateFile(d))).toBe(true); + expect(JSON.parse(await readFile(stateFile(d), "utf8")).pid).toBe(4242); +}); + +test("removeOwnState removes its own record", async () => { + const d = await dir(); + await writeState(d, state({ pid: 4242 })); + await removeOwnState(d, 4242); + expect(existsSync(stateFile(d))).toBe(false); +}); + +test("removeOwnState is quiet when there is no record at all", async () => { + // Every exit path calls it, including those that never recorded anything. + const d = await dir(); + await removeOwnState(d, 4242); + expect(existsSync(stateFile(d))).toBe(false); +}); + +test("recordState survives a conflict check that THROWS, and still refuses", async () => { + // recordState runs at top level immediately after the bind, and its contract + // is that it never throws: an escaping rejection there kills a paddock that + // is already serving — the failure this file's other guards exist to + // prevent. The conflict check added a second way to throw (a probe reaching + // for `ps`, a filesystem that fails in a new way) and it must be caught like + // the identity capture below it. + const d = await dir(); + await writeState(d, state({ pid: 4242 })); + const said: string[] = []; + const ok = await recordState( + d, + { pid: 5150, port: 8788, version: "0.8.2", startedAt: 1 }, + { + capture: () => "paddock", + probe: { + isAlive: () => { throw new Error("ps: command not found"); }, + argsOf: () => "paddock", + }, + warn: (l) => said.push(l), + log: (l) => said.push(l), + }, + ); + + // Refused, not crashed. "Cannot tell who holds the record" must not become + // "overwrite it": mis-tracking a live instance is the bug this guards. + expect(ok).toBe(false); + // And the incumbent's record is untouched. + expect(JSON.parse(await readFile(stateFile(d), "utf8")).pid).toBe(4242); + // Announced — an untracked instance nobody was told about is how this + // became invisible in the first place. + expect(said.join("\n")).toContain("ps: command not found"); +}); diff --git a/tests/lifecycle-status.test.ts b/tests/lifecycle-status.test.ts index 6ad6042..e265f0a 100644 --- a/tests/lifecycle-status.test.ts +++ b/tests/lifecycle-status.test.ts @@ -75,3 +75,26 @@ test("an unreadable state file is reported as such, distinct from absence", asyn expect(line).not.toContain("not running"); expect(line).toContain("could not"); }); + +test("status names an untracked instance instead of calling it 'not running'", async () => { + // A record can vanish while the process it described keeps serving — a + // SIGKILL leaves no cleanup behind. Reporting that as "not running" points + // the operator at the wrong problem: they go on to `start`, which fails on a + // port they were just told nothing was using. + const d = await mkdtemp(join(tmpdir(), "paddock-status-")); + const said: string[] = []; + const code = await runStatus({ + dir: d, + port: 8787, + log: (l) => said.push(l), + listener: async () => ({ version: "0.8.1" }), + }); + + // Still non-zero — it was non-zero before, and an instance nothing can stop + // is not a healthy "running" either. + expect(code).toBe(1); + const text = said.join("\n"); + expect(text).not.toContain("not running"); + expect(text).toContain("0.8.1"); + expect(text).toContain("8787"); +}); diff --git a/tests/lifecycle-stop.test.ts b/tests/lifecycle-stop.test.ts index 4353c15..6045b76 100644 --- a/tests/lifecycle-stop.test.ts +++ b/tests/lifecycle-stop.test.ts @@ -391,3 +391,62 @@ test("a cleanup failure does not turn a successful stop into a stack trace", asy await chmod(d, 0o700); } }); + +// --- an instance nothing is tracking -------------------------------------- +// +// A record can go missing while the process it described is still serving: it +// was killed with SIGKILL (no shutdown, no cleanup), or — before the ownership +// guards in state.ts — another paddock run deleted it. "paddock — not running" +// is then a LIE told about a process holding the port, and `stop` returning 0 +// let `paddock stop && paddock start` walk straight into "port already in +// use". Reported end to end by the operator: "always have this issue". + +test("stop reports an untracked instance still holding the port, and refuses success", async () => { + const d = await dir(); // no state file at all + const said: string[] = []; + const code = await runStop({ + dir: d, + port: 8787, + log: (l) => said.push(l), + listener: async (port) => (port === 8787 ? { version: "0.8.1" } : null), + }); + + // NOT 0. `stop && start` must stop here rather than fail at the bind. + expect(code).toBe(1); + const text = said.join("\n"); + expect(text).not.toContain("not running"); + expect(text).toContain("0.8.1"); + expect(text).toContain("8787"); + // Actionable, on both platforms paddock ships binaries for. + expect(text).toContain("ss -ltnp"); + expect(text).toContain("lsof"); +}); + +test("stop still says plainly that nothing is running when nothing answers", async () => { + const d = await dir(); + const said: string[] = []; + const code = await runStop({ + dir: d, port: 8787, log: (l) => said.push(l), listener: async () => null, + }); + expect(code).toBe(0); + expect(said.join("\n")).toBe("paddock — not running"); +}); + +test("stop does not probe a port when a state file already answers", async () => { + // The probe is a fallback for a MISSING record, not an extra opinion. A + // recorded instance must be stopped by pid, exactly as before. + const d = await dir(); + await writeState(d, s); + let probed = 0; + const sent: string[] = []; + await runStop({ + dir: d, + port: 8787, + probe: dyingProbe(1, "paddock").probe, + signal: (_p, sig) => sent.push(sig), + log: () => {}, + listener: async () => { probed += 1; return { version: "0.8.1" }; }, + }); + expect(sent).toContain("SIGTERM"); + expect(probed).toBe(0); +}); diff --git a/tests/update.test.ts b/tests/update.test.ts index 7e5ab67..9dc884a 100644 --- a/tests/update.test.ts +++ b/tests/update.test.ts @@ -250,3 +250,57 @@ test("a failed download names the HTTP status for each half, not just 'download expect(await readFile(self, "utf8")).toBe("OLD BINARY"); expect((await readdir(dir)).sort()).toEqual(["paddock"]); }); + +// --- the instance still running the binary that was just replaced ---------- +// +// `update` swaps the file on disk; a paddock already running keeps serving +// from the REPLACED inode (`/proc//exe` reads "… (deleted)") until it is +// restarted. Nothing said so, so `paddock update` looked complete while the +// dashboard went on serving the old version indefinitely. Observed: an +// instance still answering 0.8.1 long after `paddock: updated to 0.8.2`. + +test("update tells the operator to restart an instance still on the old version", async () => { + const body = "NEW BINARY"; + const h = await harness(body, await sha(body)); + const said: string[] = []; + const code = await runUpdate({ + selfPath: h.self, platform: "linux", arch: "x64", + current: "0.1.0", fetchImpl: h.fetchImpl, log: (l) => said.push(l), + running: async () => ({ pid: 3558072, port: 8787, version: "0.1.0" }), + }); + + expect(code).toBe(0); + const text = said.join("\n"); + expect(text).toContain("3558072"); + expect(text).toContain("8787"); + expect(text).toContain("0.1.0"); + // The exact command, because "restart it" leaves the operator guessing which + // of stop/start/kill is meant. + expect(text).toContain("paddock stop && paddock start"); +}); + +test("update says nothing about restarting when the running instance is already new", async () => { + // The ordinary second run. A hint here would send the operator to bounce a + // dashboard that is already serving the new binary. + const body = "NEW BINARY"; + const h = await harness(body, await sha(body)); + const said: string[] = []; + await runUpdate({ + selfPath: h.self, platform: "linux", arch: "x64", + current: "0.1.0", fetchImpl: h.fetchImpl, log: (l) => said.push(l), + running: async () => ({ pid: 111, port: 8787, version: "9.9.9" }), + }); + expect(said.join("\n")).not.toContain("paddock stop && paddock start"); +}); + +test("update says nothing about restarting when nothing is running", async () => { + const body = "NEW BINARY"; + const h = await harness(body, await sha(body)); + const said: string[] = []; + await runUpdate({ + selfPath: h.self, platform: "linux", arch: "x64", + current: "0.1.0", fetchImpl: h.fetchImpl, log: (l) => said.push(l), + running: async () => null, + }); + expect(said.join("\n")).not.toContain("restart"); +});