From 065d9984079a343a8a0f77669e6f06bb91393bcc Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 29 Aug 2026 15:27:53 -0300 Subject: [PATCH] fix(cli): update flow now says whether the running process needs a restart (#11885) (#12005) Fixes three defects in the "update doesn't restart the running process" bug class: CLI update guidance now detects a live server and tells the operator to restart instead of implying the update is already live; the dashboard's Update button tries OmniRoute's own PID-file supervisor before falling back to pm2 instead of hardcoding pm2 and silently skipping; getLatestVersionFromNpmCli now uses --prefer-online (same fix pattern as #4376). TDD throughout, 63/63 targeted regression tests pass. --- bin/cli/commands/update.mjs | 40 +++++- bin/cli/io.mjs | 4 + .../11885-update-restart-live-process.md | 1 + src/app/api/system/version/route.ts | 48 +++---- src/lib/system/processManagerRestart.ts | 123 ++++++++++++++++++ src/lib/system/versionCheck.ts | 20 ++- .../cli-update-restart-guidance-11885.test.ts | 75 +++++++++++ ...stem-process-manager-restart-11885.test.ts | 80 ++++++++++++ ...-version-check-prefer-online-11885.test.ts | 34 +++++ 9 files changed, 388 insertions(+), 37 deletions(-) create mode 100644 changelog.d/fixes/11885-update-restart-live-process.md create mode 100644 src/lib/system/processManagerRestart.ts create mode 100644 tests/unit/cli-update-restart-guidance-11885.test.ts create mode 100644 tests/unit/system-process-manager-restart-11885.test.ts create mode 100644 tests/unit/system-version-check-prefer-online-11885.test.ts diff --git a/bin/cli/commands/update.mjs b/bin/cli/commands/update.mjs index c1722f4bcb..9b9848ba6e 100644 --- a/bin/cli/commands/update.mjs +++ b/bin/cli/commands/update.mjs @@ -1,4 +1,4 @@ -import { printHeading, printInfo, printSuccess, printError } from "../io.mjs"; +import { printHeading, printInfo, printSuccess, printError, printWarning } from "../io.mjs"; import { homedir } from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -6,6 +6,7 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; import { t } from "../i18n.mjs"; import { npmBin, npmExecOptions } from "../npm-exec.mjs"; +import { readPidFile, isPidRunning } from "../utils/pid.mjs"; const execFileAsync = promisify(execFile); @@ -79,6 +80,39 @@ export async function createBackup() { } } +// #11885: `--apply` installs the new files (npm install -g) and re-reads +// package.json from disk to confirm it, but a long-lived server process keeps +// serving whatever it loaded at its last start — Node caches a `require()`d +// package.json per resolved path for the life of the process. A later +// `omniroute update` then correctly reports "already up to date" (the files +// ARE current) while the running server is still stale, matching the reported +// symptom. `--apply` never restarted anything and its success message ("Run +// `omniroute --version` to verify.") implied the update was already live. +// +// `restart.mjs`'s `runRestartCommand()` stops then re-spawns the server in the +// foreground (via `serve.mjs::runServe`), which can block the calling terminal +// and is a materially bigger behavior change than this fix warrants to invoke +// unconditionally and unattended from `--apply`. Instead, detect whether a +// CLI-managed server is currently running (the same PID file `stop.mjs`/ +// `restart.mjs` already trust) and print an explicit, prominent instruction — +// honest about what did and didn't happen — rather than silently assuming. +export async function isServerProcessRunning(deps = { readPidFile, isPidRunning }) { + const pid = deps.readPidFile("server"); + return Boolean(pid && deps.isPidRunning(pid)); +} + +export async function printPostApplyGuidance(latest, deps = { readPidFile, isPidRunning }) { + const running = await isServerProcessRunning(deps); + if (running) { + printWarning(`Files updated to ${latest}, but the running server is still on the old version.`); + printInfo(" Run `omniroute restart` now to apply this update."); + } else { + printInfo(`No running OmniRoute server was detected via the CLI's PID file.`); + printInfo(` Start it with \`omniroute serve\` (or restart your existing process) to run ${latest}.`); + } + printInfo("`omniroute --version` will keep reporting the old version until the process restarts."); +} + export function registerUpdate(program) { program .command("update") @@ -210,8 +244,8 @@ export async function runUpdateCommand(opts = {}) { console.log(" or reorder PATH so the global bin comes first."); return 1; } - printSuccess(`Updated to version ${latest}`); - printInfo("Run `omniroute --version` to verify."); + printSuccess(`Installed omniroute@${latest} to disk.`); + await printPostApplyGuidance(latest); return 0; } catch (err) { printError(`Update failed: ${err.message}`); diff --git a/bin/cli/io.mjs b/bin/cli/io.mjs index b72f7c4310..e87758f624 100644 --- a/bin/cli/io.mjs +++ b/bin/cli/io.mjs @@ -81,3 +81,7 @@ export function printInfo(message) { export function printError(message) { console.log(`\x1b[31m✖ ${message}\x1b[0m`); } + +export function printWarning(message) { + console.log(`\x1b[33m⚠ ${message}\x1b[0m`); +} diff --git a/changelog.d/fixes/11885-update-restart-live-process.md b/changelog.d/fixes/11885-update-restart-live-process.md new file mode 100644 index 0000000000..972c346204 --- /dev/null +++ b/changelog.d/fixes/11885-update-restart-live-process.md @@ -0,0 +1 @@ +- **fix(cli):** `omniroute update --apply` now tells you explicitly whether a running server was detected and, if so, that you must run `omniroute restart` to apply the update — it never restarted anything and previously implied the update was already live once files were installed. The dashboard's npm-mode Update flow (`/api/system/version`) now tries OmniRoute's own PID-file-managed supervisor before falling back to pm2, and reports an honest "restart required" step instead of a silent pm2-only "skipped" that read like a completed update. The server-side latest-version lookup backing the dashboard's update banner also gained `--prefer-online`, closing the same stale-npm-cache class already fixed in the CLI's own copy for #4376 ([#11885](https://github.com/diegosouzapw/OmniRoute/issues/11885)). diff --git a/src/app/api/system/version/route.ts b/src/app/api/system/version/route.ts index 6e2f1e2f36..f3ec9c26d4 100644 --- a/src/app/api/system/version/route.ts +++ b/src/app/api/system/version/route.ts @@ -24,6 +24,7 @@ import { resolveLatestVersionCached, } from "@/lib/system/versionCheck"; import { resolveGlobalOmniroutePath } from "@/lib/system/globalPackagePath"; +import { restartRunningServer } from "@/lib/system/processManagerRestart"; // #5542 — On Windows npm is `npm.cmd`; Node ≥24 refuses to execFile a `.cmd` without // a shell (nodejs/node#52554 → "spawn npm ENOENT"). buildNpmExecOptions enables the // shell on win32 only; SERVICE_VERSION_PATTERN keeps the shell-joined version safe. @@ -41,6 +42,20 @@ function getCurrentVersion(): string { } } +/** + * Shared restart step for both npm-mode update flows (source-checkout and global-install + * below). #11885: this used to hardcode `pm2 restart omniroute` in each branch separately + * and silently report "skipped" — reading like a completed update — whenever pm2 wasn't + * the process manager. `restartRunningServer()` tries OmniRoute's own PID-file-managed + * supervisor first, then pm2, and this wrapper turns its honest "restart-required" outcome + * into an SSE step the dashboard renders as a warning instead of a false "done". + */ +async function sendRestartStep(send: (data: Record) => void): Promise { + send({ step: "restart", status: "running", message: "Restarting service..." }); + const outcome = await restartRunningServer(); + send({ step: "restart", status: outcome.status, message: outcome.message }); +} + async function getNews() { try { const res = await fetch(NEWS_JSON_URL, { next: { revalidate: 3600 } }); @@ -259,20 +274,7 @@ export async function POST(req: NextRequest) { ); send({ step: "rebuild", status: "done", message: "Build complete" }); - send({ step: "restart", status: "running", message: "Restarting service..." }); - try { - await execFileAsync("pm2", ["restart", "omniroute", "--update-env"], { - timeout: 30_000, - cwd: PROJECT_ROOT, - }); - send({ step: "restart", status: "done", message: "Service restarted" }); - } catch { - send({ - step: "restart", - status: "skipped", - message: "PM2 not available — manual restart needed", - }); - } + await sendRestartStep(send); send({ step: "complete", @@ -341,22 +343,8 @@ export async function POST(req: NextRequest) { ); send({ step: "rebuild", status: "done", message: "Native modules rebuilt" }); - // Step 3: Restart PM2 - send({ step: "restart", status: "running", message: "Restarting service via PM2..." }); - try { - await execFileAsync("pm2", ["restart", "omniroute", "--update-env"], { - timeout: 30000, - cwd: PROJECT_ROOT, - }); - send({ step: "restart", status: "done", message: "Service restarted" }); - } catch { - // PM2 may not be available (Docker/manual setups) - send({ - step: "restart", - status: "skipped", - message: "PM2 not available — manual restart needed", - }); - } + // Step 3: Restart + await sendRestartStep(send); clearLatestVersionCache(); send({ diff --git a/src/lib/system/processManagerRestart.ts b/src/lib/system/processManagerRestart.ts new file mode 100644 index 0000000000..caa62fa541 --- /dev/null +++ b/src/lib/system/processManagerRestart.ts @@ -0,0 +1,123 @@ +/** + * Restart mechanism detection for the npm-mode dashboard "Update" flow. + * + * #11885: `src/app/api/system/version/route.ts` hardcoded `pm2 restart omniroute` as the + * ONLY restart mechanism, at two near-identical branches. OmniRoute ships its OWN + * supervisor (`bin/cli/runtime/processSupervisor.mjs`, started by `omniroute serve` / + * `omniroute serve --daemon`) with PID-file management (`bin/cli/utils/pid.mjs`) as an + * alternative to pm2 — so on any install that isn't pm2-managed, the restart step + * silently degraded to a "skipped" status while the install step still reported "done", + * which reads like a completed live update even though nothing restarted. + * + * This module tries OmniRoute's own PID-file-managed supervisor first, then falls back + * to pm2, and returns an honest "restart-required" outcome instead of a silent no-op + * when neither is detected. + */ +import path from "node:path"; +import fs from "node:fs"; +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; +import { resolveDataDir } from "@/lib/dataPaths"; + +const execFileAsync = promisify(execFile); + +export type RestartMethod = "own-supervisor" | "pm2" | "none"; +export type RestartStatus = "done" | "restart-required"; + +export interface RestartOutcome { + method: RestartMethod; + status: RestartStatus; + message: string; +} + +type PidService = "server" | "supervisor"; + +export interface RestartManagerDeps { + readPidFile?: (service: PidService) => number | null; + isPidRunning?: (pid: number) => boolean; + killProcess?: (pid: number) => void; + execPm2?: (args: string[]) => Promise; +} + +function defaultReadPidFile(service: PidService): number | null { + try { + const file = path.join(resolveDataDir(), service, ".pid"); + if (!fs.existsSync(file)) return null; + const raw = fs.readFileSync(file, "utf8").trim(); + const pid = parseInt(raw, 10); + return Number.isFinite(pid) ? pid : null; + } catch { + return null; + } +} + +function defaultIsPidRunning(pid: number): boolean { + if (!pid) return false; + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function defaultKillProcess(pid: number): void { + process.kill(pid, "SIGTERM"); +} + +function defaultExecPm2(args: string[]): Promise { + return execFileAsync("pm2", args, { timeout: 30_000 }); +} + +/** + * Attempt to restart the running OmniRoute server. + * + * Order: + * 1. OmniRoute's own PID-file-managed supervisor — SIGTERM the supervised "server" + * child ONLY, never the supervisor process itself. The supervisor's exit handler + * (`ServerSupervisor.handleExit`) treats an unexpected child exit as a crash and + * respawns it, which IS the restart we want. SIGTERM to the SUPERVISOR instead + * sets its `isShuttingDown` flag and intentionally skips the respawn (see + * `bin/cli/commands/stop.mjs`) — that would stop the service, not restart it. + * 2. pm2, for installs that use it instead of the CLI's own supervisor. + * 3. Neither detected — return an honest "restart-required" outcome rather than a + * silently-skipped step that reads like nothing was wrong. + */ +export async function restartRunningServer( + deps: RestartManagerDeps = {} +): Promise { + const readPidFile = deps.readPidFile ?? defaultReadPidFile; + const isPidRunning = deps.isPidRunning ?? defaultIsPidRunning; + const killProcess = deps.killProcess ?? defaultKillProcess; + const execPm2 = deps.execPm2 ?? defaultExecPm2; + + const supervisorPid = readPidFile("supervisor"); + const serverPid = readPidFile("server"); + const supervisorAlive = Boolean(supervisorPid && isPidRunning(supervisorPid)); + const serverAlive = Boolean(serverPid && isPidRunning(serverPid)); + + if (supervisorAlive && serverAlive && serverPid) { + try { + killProcess(serverPid); + return { + method: "own-supervisor", + status: "done", + message: "Restarted via the OmniRoute supervisor (server process recycled).", + }; + } catch { + // Fall through to pm2 / restart-required below. + } + } + + try { + await execPm2(["restart", "omniroute", "--update-env"]); + return { method: "pm2", status: "done", message: "Service restarted via pm2." }; + } catch { + return { + method: "none", + status: "restart-required", + message: + "Files were updated, but no supported process manager (OmniRoute's own supervisor or pm2) was detected — restart the server manually to apply the update.", + }; + } +} diff --git a/src/lib/system/versionCheck.ts b/src/lib/system/versionCheck.ts index 3e0dfd83d9..e7dea38ec8 100644 --- a/src/lib/system/versionCheck.ts +++ b/src/lib/system/versionCheck.ts @@ -53,14 +53,26 @@ let latestVersionCacheGeneration = 0; // for back-compat with existing server-side importers. export { normalizeVersion, isNewer } from "./versionCompare"; -/** Latest published version via the `npm` CLI (fast when npm is on PATH, e.g. source installs). */ -export async function getLatestVersionFromNpmCli(): Promise { +/** + * Latest published version via the `npm` CLI (fast when npm is on PATH, e.g. source installs). + * + * `execFn` is injectable for tests (same pattern as the CLI's own + * `bin/cli/commands/update.mjs::getLatestVersion()`). + */ +export async function getLatestVersionFromNpmCli( + execFn: typeof execFileAsync = execFileAsync +): Promise { try { // #5542 — win32 npm is npm.cmd; execFile without a shell throws "spawn npm ENOENT" // on Node ≥24 (nodejs/node#52554). buildNpmExecOptions enables the shell on win32. - const { stdout } = await execFileAsync( + // #11885 — `--prefer-online` forces npm to revalidate its HTTP cache against the + // registry. Without it `npm info` can return a stale cached version, the same known + // bug class already fixed in the CLI's own copy for #4376 (see that fix's comment in + // bin/cli/commands/update.mjs::getLatestVersion()) but never mirrored here — this is + // the function backing the dashboard's "Update Available" banner. + const { stdout } = await execFn( "npm", - ["info", "omniroute", "version", "--json"], + ["info", "omniroute", "version", "--json", "--prefer-online"], buildNpmExecOptions(process.platform, { timeoutMs: LOOKUP_TIMEOUT_MS }) ); const parsed = JSON.parse(String(stdout).trim()); diff --git a/tests/unit/cli-update-restart-guidance-11885.test.ts b/tests/unit/cli-update-restart-guidance-11885.test.ts new file mode 100644 index 0000000000..7db9902fc3 --- /dev/null +++ b/tests/unit/cli-update-restart-guidance-11885.test.ts @@ -0,0 +1,75 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +const update = await import("../../bin/cli/commands/update.mjs"); + +// #11885: a user's dashboard reported an old version, running `omniroute update` +// said "already up to date" (the on-disk package.json HAD been updated by an +// earlier `--apply` run), but the long-lived server process kept serving the old +// version because `--apply` never restarts anything — it only re-reads the +// package.json on disk and prints "Run `omniroute --version` to verify", which +// implies the running install is now current when it is not. + +test("isServerProcessRunning: true when the CLI-managed server pid is alive (#11885)", async () => { + const running = await update.isServerProcessRunning({ + readPidFile: (service: string) => (service === "server" ? 12345 : null), + isPidRunning: (pid: number) => pid === 12345, + }); + assert.equal(running, true); +}); + +test("isServerProcessRunning: false when there is no pid file (#11885)", async () => { + const running = await update.isServerProcessRunning({ + readPidFile: () => null, + isPidRunning: () => true, + }); + assert.equal(running, false); +}); + +test("isServerProcessRunning: false when the pid file is stale (#11885)", async () => { + const running = await update.isServerProcessRunning({ + readPidFile: () => 999, + isPidRunning: () => false, + }); + assert.equal(running, false); +}); + +function captureLogs(fn: () => Promise) { + const logs: string[] = []; + const origLog = console.log; + console.log = (...args: unknown[]) => { + logs.push(args.map(String).join(" ")); + }; + return fn() + .then(() => logs) + .finally(() => { + console.log = origLog; + }); +} + +test("printPostApplyGuidance tells the user to run `omniroute restart` when a server is running, and stops implying the update is already live (#11885)", async () => { + const logs = await captureLogs(() => + update.printPostApplyGuidance("3.9.0", { + readPidFile: () => 42, + isPidRunning: () => true, + }) + ); + const joined = logs.join("\n"); + assert.match(joined, /omniroute restart/); + assert.doesNotMatch( + joined, + /^.*Updated to version 3\.9\.0.*$/m, + "must not claim the running process is already updated" + ); +}); + +test("printPostApplyGuidance tells the user to start the server when none is detected (#11885)", async () => { + const logs = await captureLogs(() => + update.printPostApplyGuidance("3.9.0", { + readPidFile: () => null, + isPidRunning: () => false, + }) + ); + const joined = logs.join("\n"); + assert.match(joined, /omniroute serve/); +}); diff --git a/tests/unit/system-process-manager-restart-11885.test.ts b/tests/unit/system-process-manager-restart-11885.test.ts new file mode 100644 index 0000000000..7b1cf4c47f --- /dev/null +++ b/tests/unit/system-process-manager-restart-11885.test.ts @@ -0,0 +1,80 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { restartRunningServer } from "@/lib/system/processManagerRestart"; + +// #11885: the dashboard's npm-mode Update flow (src/app/api/system/version/route.ts) +// hardcoded `pm2 restart omniroute` as the ONLY restart mechanism, at two +// near-identical branches. When pm2 isn't the process manager (OmniRoute's own +// `omniroute serve --daemon` supervisor, or plain `npm run start`) it silently +// degraded to a "skipped" status while the install step still reported "done" — +// reading like a completed live update when the running server never restarted. + +test("restartRunningServer: uses OmniRoute's own supervisor when both the supervisor and server pids are alive", async () => { + const killed = []; + const pm2Calls = []; + const outcome = await restartRunningServer({ + readPidFile: (service) => (service === "supervisor" ? 111 : service === "server" ? 222 : null), + isPidRunning: (pid) => pid === 111 || pid === 222, + killProcess: (pid) => killed.push(pid), + execPm2: async (args) => { + pm2Calls.push(args); + }, + }); + + assert.equal(outcome.method, "own-supervisor"); + assert.equal(outcome.status, "done"); + // Must SIGTERM the supervised SERVER child, never the supervisor itself — killing + // the supervisor sets its isShuttingDown flag and intentionally skips the respawn + // (see bin/cli/commands/stop.mjs), which would NOT produce a restart. + assert.deepEqual(killed, [222]); + assert.equal(pm2Calls.length, 0, "must not fall back to pm2 when the own supervisor path succeeds"); +}); + +test("restartRunningServer: falls back to pm2 when no OmniRoute supervisor is detected", async () => { + const pm2Calls = []; + const outcome = await restartRunningServer({ + readPidFile: () => null, + isPidRunning: () => false, + killProcess: () => { + throw new Error("must not be called"); + }, + execPm2: async (args) => { + pm2Calls.push(args); + }, + }); + + assert.equal(outcome.method, "pm2"); + assert.equal(outcome.status, "done"); + assert.equal(pm2Calls.length, 1); + assert.deepEqual(pm2Calls[0], ["restart", "omniroute", "--update-env"]); +}); + +test("restartRunningServer: falls back to pm2 when the supervisor pid is stale", async () => { + const pm2Calls = []; + const outcome = await restartRunningServer({ + readPidFile: (service) => (service === "supervisor" ? 999 : null), + isPidRunning: () => false, + killProcess: () => { + throw new Error("must not be called"); + }, + execPm2: async (args) => { + pm2Calls.push(args); + }, + }); + + assert.equal(outcome.method, "pm2"); +}); + +test("restartRunningServer: returns an honest restart-required outcome when neither is available (no silent 'skipped')", async () => { + const outcome = await restartRunningServer({ + readPidFile: () => null, + isPidRunning: () => false, + execPm2: async () => { + throw new Error("pm2: command not found"); + }, + }); + + assert.equal(outcome.method, "none"); + assert.equal(outcome.status, "restart-required"); + assert.match(outcome.message, /restart/i); +}); diff --git a/tests/unit/system-version-check-prefer-online-11885.test.ts b/tests/unit/system-version-check-prefer-online-11885.test.ts new file mode 100644 index 0000000000..5e8d7ec358 --- /dev/null +++ b/tests/unit/system-version-check-prefer-online-11885.test.ts @@ -0,0 +1,34 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { getLatestVersionFromNpmCli } from "@/lib/system/versionCheck"; + +// #11885: a user's dashboard showed 3.8.10 while the "3.8.49 available" banner kept +// firing, yet `omniroute update` on the box said "already up to date". Part of the +// same known bug class as #4376 (already fixed in bin/cli/commands/update.mjs's own +// getLatestVersion()): `npm info omniroute version --json` without `--prefer-online` +// can serve a stale cached "latest" from npm's local HTTP cache. The server-side copy +// used by the dashboard's update banner never got the same fix. +test("getLatestVersionFromNpmCli passes --prefer-online to bypass the stale npm cache (#11885, same class as #4376)", async () => { + let capturedArgs: string[] | null = null; + const fakeExec = async (_cmd: string, args: string[]) => { + capturedArgs = args; + return { stdout: JSON.stringify("3.8.49"), stderr: "" }; + }; + const latest = await getLatestVersionFromNpmCli(fakeExec as never); + assert.equal(latest, "3.8.49"); + assert.ok(capturedArgs, "exec must be invoked"); + assert.ok( + (capturedArgs as string[]).includes("--prefer-online"), + `expected --prefer-online in npm args, got: ${JSON.stringify(capturedArgs)}` + ); + assert.ok((capturedArgs as string[]).includes("info")); + assert.ok((capturedArgs as string[]).includes("omniroute")); + assert.ok((capturedArgs as string[]).includes("version")); +}); + +test("getLatestVersionFromNpmCli returns null when npm is unavailable", async () => { + const latest = await getLatestVersionFromNpmCli(async () => { + throw new Error("npm not found"); + }); + assert.equal(latest, null); +});