diff --git a/CHANGELOG.md b/CHANGELOG.md index 08040a46..15942d53 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/cli/serve.ts b/src/cli/serve.ts index af7f8e6c..20a2ff6a 100644 --- a/src/cli/serve.ts +++ b/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(); } diff --git a/tests/cli/serve_exit_after_shutdown_test.ts b/tests/cli/serve_exit_after_shutdown_test.ts new file mode 100644 index 00000000..2392b843 --- /dev/null +++ b/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 => ({ + "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 => ({ + "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, port: number): Promise { + { + const root = await Deno.makeTempDir({ prefix: "dune_test_serve_exit_" }); + let child: Deno.ChildProcess | undefined; + let output = ""; + let drains: Promise | 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) => { + 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), +});