fix(serve): exit explicitly after a graceful shutdown

CHANGELOG.md

@@ -53,6 +53,18 @@ split, begin at v1.0.0 — see [VERSIONING.md](VERSIONING.md) for exactly what
 
 ### Fixed
 
+- **`dune serve` did not exit after a graceful shutdown if anything was still
+  running.** On SIGTERM it closed the listener, drained, printed `shutdown
+  complete`, and then just returned, so the process lived only as long as the
+  event loop had work. One ref'd plugin timer (`@dune/plugin-admin`'s collab
+  eviction interval, fixed in 3.1.3) was enough to keep it alive forever at 0%
+  CPU with no listener — an "idle" instance that a *second* SIGTERM was needed
+  to end. `serve` now exits explicitly once the drain is done (single-site and
+  multi-site), so no plugin timer can hold the process open. Covered by a test
+  that runs the real `dune serve`, gives it a plugin holding a ref'd timer, and
+  checks that one SIGTERM ends it. (The multi-site shutdown path has the same
+  change but no end-to-end test.)
+
 - **Co-located audio and video could not seek, and would not play in
   Safari/iOS.** Media files were always answered with a full `200` body and no
   `Accept-Ranges` header, and a `Range` request was ignored. Media responses

src/cli/serve.ts

@@ -96,6 +96,21 @@ async function drainInFlight(
   }
 }
 
+/**
+ * End the process once a graceful shutdown has finished draining.
+ *
+ * Returning from `serveCommand()` and letting the event loop empty is not
+ * enough: any plugin timer or handle that is still ref'd keeps the process
+ * alive, and it then sits there forever at 0% CPU with no listener (an "idle"
+ * instance that a second SIGTERM is needed to end). plugin-admin's collab
+ * eviction interval did exactly this until 3.1.3. By this point the listener
+ * is closed and in-flight requests are drained (or the drain deadline has
+ * passed), so there is nothing left worth waiting for.
+ */
+function exitAfterShutdown(): void {
+  Deno.exit(0);
+}
+
 // ---------------------------------------------------------------------------
 // Main command
 // ---------------------------------------------------------------------------
@@ -181,6 +196,7 @@ export async function serveCommand(root: string, options: ServeOptions = {}) {
     cleanup();
     watchdog.stop();
     console.log("[dune] shutdown complete");
+    exitAfterShutdown();
     return;
   }
 
@@ -397,4 +413,5 @@ export async function serveCommand(root: string, options: ServeOptions = {}) {
   cleanup();
   watchdog.stop();
   console.log("[dune] shutdown complete");
+  exitAfterShutdown();
 }

tests/cli/serve_exit_after_shutdown_test.ts

@@ -0,0 +1,125 @@
+/**
+ * `dune serve` must exit after a graceful shutdown, whatever plugins left
+ * running.
+ *
+ * Graceful shutdown closes the listener and prints "shutdown complete", but
+ * serveCommand() used to just return, so the process lived only as long as the
+ * event loop had work. One plugin timer that was never unref'd (plugin-admin's
+ * collab eviction interval, until 3.1.3) kept it alive forever: an "idle" dune
+ * process with no port, at 0% CPU, that a single SIGTERM never ended.
+ *
+ * Runs the real `serve` command as a subprocess. The plugin below holds a
+ * ref'd timer on purpose, standing in for any plugin that does, so this test
+ * does not depend on which plugin-admin version is installed.
+ */
+
+import { assertEquals } from "https://deno.land/std@0.224.0/assert/mod.ts";
+import { dirname, join } from "@std/path";
+
+const READY_TIMEOUT_MS = 60_000;
+const EXIT_TIMEOUT_MS = 15_000;
+
+/** A plugin whose timer is ref'd and never cleared. */
+const HOLD_PLUGIN = `setInterval(() => {}, 3_600_000);\nexport default { name: "hold", version: "1.0.0", hooks: {} };\n`;
+// Fresh reads compilerOptions from the site root on the first build.
+const DENO_JSON = JSON.stringify({ compilerOptions: { jsx: "react-jsx", jsxImportSource: "preact" } });
+
+const singleSite = (port: number): Record<string, string> => ({
+  "config/site.yaml": `title: Exit Test\nurl: http://localhost:${port}\nplugins:\n  - src: ./plugins/hold.ts\n`,
+  "deno.json": DENO_JSON,
+  "content/01.home/default.md": "---\ntitle: Home\n---\nhi\n",
+  "plugins/hold.ts": HOLD_PLUGIN,
+});
+
+// Multi-site mode (config/sites.yaml at the install root) has its own shutdown path.
+const multiSite = (port: number): Record<string, string> => ({
+  "config/sites.yaml": "sites:\n  - id: main\n    root: ../main\n    default: true\n",
+  "deno.json": DENO_JSON,
+  "main/config/site.yaml": `title: Exit Test\nurl: http://localhost:${port}\nplugins:\n  - src: ./plugins/hold.ts\n`,
+  "main/deno.json": DENO_JSON,
+  "main/content/01.home/default.md": "---\ntitle: Home\n---\nhi\n",
+  "main/plugins/hold.ts": HOLD_PLUGIN,
+});
+
+async function expectExitAfterSigterm(files: Record<string, string>, port: number): Promise<void> {
+  {
+    const root = await Deno.makeTempDir({ prefix: "dune_test_serve_exit_" });
+    let child: Deno.ChildProcess | undefined;
+    let output = "";
+    let drains: Promise<unknown> | undefined;
+    try {
+      for (const [rel, text] of Object.entries(files)) {
+        await Deno.mkdir(dirname(join(root, rel)), { recursive: true });
+        await Deno.writeTextFile(join(root, rel), text);
+      }
+      const duneDenoJson = join(import.meta.dirname!, "..", "..", "deno.json");
+      const cliPath = join(import.meta.dirname!, "..", "..", "src", "cli.ts");
+
+      child = new Deno.Command(Deno.execPath(), {
+        args: ["run", "-A", `--config=${duneDenoJson}`, cliPath, "serve", "--root", root, "--port", String(port)],
+        env: { ...Deno.env.toObject(), DUNE_CONFIG_APPLIED: "1" },
+        stdout: "piped",
+        stderr: "piped",
+      }).spawn();
+      const decoder = new TextDecoder();
+      const drain = async (s: ReadableStream<Uint8Array>) => {
+        for await (const c of s) output += decoder.decode(c);
+      };
+      drains = Promise.all([drain(child.stdout), drain(child.stderr)]);
+
+      // Ready?
+      let ready = false;
+      const readyDeadline = Date.now() + READY_TIMEOUT_MS;
+      while (Date.now() < readyDeadline) {
+        try {
+          const res = await fetch(`http://localhost:${port}/`, { signal: AbortSignal.timeout(1500) });
+          await res.body?.cancel();
+          ready = true;
+          break;
+        } catch {
+          await new Promise((r) => setTimeout(r, 250));
+        }
+      }
+      assertEquals(ready, true, `server never came up:\n${output}`);
+
+      // One SIGTERM, as a service manager or a plain `kill` would send.
+      child.kill("SIGTERM");
+      const outcome = await Promise.race([
+        child.status.then((s) => ({ exited: true as const, code: s.code })),
+        new Promise<{ exited: false }>((r) => setTimeout(() => r({ exited: false }), EXIT_TIMEOUT_MS)),
+      ]);
+
+      assertEquals(
+        outcome.exited,
+        true,
+        `process was still alive ${EXIT_TIMEOUT_MS}ms after SIGTERM (a ref'd plugin timer kept it running):\n${output}`,
+      );
+      assertEquals((outcome as { code: number }).code, 0);
+      assertEquals(output.includes("shutdown complete"), true, output);
+      child = undefined;
+    } finally {
+      if (child) {
+        try {
+          child.kill("SIGKILL");
+        } catch { /* already gone */ }
+        await child.status.catch(() => {});
+      }
+      await drains?.catch(() => {});
+      await Deno.remove(root, { recursive: true }).catch(() => {});
+    }
+  }
+}
+
+Deno.test({
+  name: "dune serve exits after SIGTERM even when a plugin holds a ref'd timer",
+  sanitizeOps: false,
+  sanitizeResources: false,
+  fn: () => expectExitAfterSigterm(singleSite(18_952), 18_952),
+});
+
+Deno.test({
+  name: "dune serve exits after SIGTERM in multi-site mode too",
+  sanitizeOps: false,
+  sanitizeResources: false,
+  fn: () => expectExitAfterSigterm(multiSite(18_953), 18_953),
+});