fix: one state file, many instances — an orphan you cannot stop

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/<pid>/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 |

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,21 +103,37 @@ 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;
+    },
   }));
 }
 
 // Must run before any server setup below, same reasoning as `update` above:
 // `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);

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<boolean> {
+  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<number> {
 
   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<number> {
 
 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<number> {
 
   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":

src/server/lifecycle/state.ts

@@ -92,6 +92,8 @@ export async function writeState(dir: string, s: PaddockState): Promise<void> {
 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<void> {
   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<void> {
+  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,

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<number> {
     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/<pid>/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;
 }

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",