Compare commits

...

1 Commits

Author SHA1 Message Date
Markus Hartung
7ac61b0f26 fix(cli): update flow now says whether the running process needs a restart (#11885)
Three defects in the same known bug class produced the reported symptom
(dashboard stuck on an old version while `omniroute update` says
"already up to date"):

- `omniroute update --apply` installed files and re-verified them on disk,
  but never restarted anything and its success message implied the running
  process was already current. It now checks the CLI's own PID file and
  prints an explicit instruction (`omniroute restart`, or `omniroute serve`
  when no server is detected) instead of implying the update is live.
- The dashboard's npm-mode Update flow (`/api/system/version`) hardcoded
  `pm2 restart omniroute` at two duplicated call sites and silently degraded
  to a "skipped" status — reading like a completed update — whenever pm2
  wasn't the process manager. A new `restartRunningServer()` helper
  (`src/lib/system/processManagerRestart.ts`) tries OmniRoute's own
  PID-file-managed supervisor first, then pm2, and reports an honest
  "restart-required" outcome otherwise. Both call sites now share one
  `sendRestartStep()` wrapper instead of duplicating the logic a third time.
- `getLatestVersionFromNpmCli()` (backing the dashboard's update banner)
  ran `npm info` without `--prefer-online`, the same stale-npm-cache class
  already fixed in the CLI's own `getLatestVersion()` for #4376 but never
  mirrored here.

The reporter's exact environment (install method, process manager) was
never confirmed, so this fixes the mechanism, not a proven root cause for
that specific user.
2026-08-29 06:39:52 -03:00
9 changed files with 388 additions and 37 deletions

View File

@@ -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}`);

View File

@@ -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`);
}

View File

@@ -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)).

View File

@@ -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<string, unknown>) => void): Promise<void> {
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({

View File

@@ -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<unknown>;
}
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<unknown> {
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<RestartOutcome> {
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.",
};
}
}

View File

@@ -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<string | null> {
/**
* 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<string | null> {
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());

View File

@@ -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<void>) {
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/);
});

View File

@@ -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);
});

View File

@@ -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);
});