From 115b8bd10261861150bffb1fa9f64c989bc033c4 Mon Sep 17 00:00:00 2001 From: lntvan166 Date: Fri, 21 Aug 2026 09:07:30 +0700 Subject: [PATCH] =?UTF-8?q?fix:=20one=20state=20file,=20many=20instances?= =?UTF-8?q?=20=E2=80=94=20an=20orphan=20you=20cannot=20stop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `paddock stop` answered "not running" while a paddock held the port, so `paddock stop && paddock start` failed on "port 8787 is already in use". The operator's report was "always have this issue", and it was not a version mix-up: it is one bug with two halves. There is ONE state file per config dir, but paddock is per-PORT — a `PADDOCK_PORT=…` instance, a `--demo` on a spare port, a dev or test server. `recordState` wrote unconditionally, so the second instance to start silently took over the first's record; `removeState` deleted unconditionally, so the first instance to EXIT wiped the file. Whatever still held the dashboard's port was then untrackable for the rest of its life. paddock's own advice walks an operator into it: "port 8787 is already in use … choose another port: PADDOCK_PORT=8788 paddock". Do that, then stop the second instance, and the first is orphaned. (`paddock tunnel` is NOT one of these cases, though an earlier draft of this message said so — it serves the dashboard itself and its preflight refuses to start beside a recorded instance. Verified by running it against a live one.) Reproduced end to end before fixing: start A on one port, start B on another, Ctrl-C B, and A serves on with `status` and `stop` both saying "not running" and `start` refusing A's port. FIRST INSTANCE WINS. `recordState` refuses to write over a `running` record belonging to a different pid, and says so. Only that case blocks the write — a stale record, a mismatch, garbage and this pid's own earlier record must all stay claimable, or one leftover file would lock out every future start. A conflict check that THROWS also refuses, and is caught: this runs at top level right after the bind, where an escaping rejection would kill a paddock that is already serving, and "cannot tell who holds the record" must not become "overwrite it". Servers now clear their record with `removeOwnState`, which deletes only a record it can positively identify as its own; `removeState` stays unconditional for `stop` and `status`, which delete only after deciding the record is theirs to clear. That fixes new orphans. For the ones already out there — a SIGKILL leaves no cleanup either — `status` and `stop` now probe the port when no record exists and name what is serving, instead of reporting a live process as "not running". `stop` returns non-zero there so `stop && start` halts with the reason rather than at the bind. No signal is sent: the pid lived in the record that is gone, and finding a pid from a port is platform-specific, so it hands over the command instead of guessing at a kill. Separately, `paddock update` replaces the binary but cannot restart the process running it — `/proc//exe` reads "… (deleted)" and the old build keeps serving. It now reports the pid, port and version still running, with the restart command. Told, NOT done, by the operator's choice: restarting would drop every connected phone mid-session to finish a command they ran for the binary's sake. One existing test changed rather than an assertion: tests/cli.test.ts spawned verbs without PADDOCK_PORT, so they inherited 8787 — where the developer's own paddock listens — and the new probe legitimately found it. It now asks the OS for a free port, the same fix docs/gotchas.md already records for port ranges that "look unused". make check / make check-clean clean bun test 1023 pass, 0 fail, 101 files Verified live: B refuses to record over A and says why, B's Ctrl-C leaves A's record intact, A stays trackable and stoppable. Against the real orphan on this machine, `status` went from "paddock — not running" to naming the untracked instance and the version it was still serving; that instance was then restarted and is tracked again on 0.8.2. Co-Authored-By: Claude Opus 5 --- docs/gotchas.md | 2 + src/server/index.ts | 30 +++++-- src/server/lifecycle/commands.ts | 73 ++++++++++++++++ src/server/lifecycle/state.ts | 77 ++++++++++++++++ src/server/update.ts | 36 ++++++++ tests/cli.test.ts | 8 ++ tests/lifecycle-server-state.test.ts | 52 +++++++++++ tests/lifecycle-state.test.ts | 126 +++++++++++++++++++++++++++ tests/lifecycle-status.test.ts | 23 +++++ tests/lifecycle-stop.test.ts | 59 +++++++++++++ tests/update.test.ts | 54 ++++++++++++ 11 files changed, 533 insertions(+), 7 deletions(-) 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"); +});