fix(cli): verify process identity when validating server pid files
After an ungraceful shutdown removePidFile never runs, so a stale run/openchamber-<port>.pid outlives the process. The kernel can recycle that PID to an unrelated process, and a liveness-only `process.kill(pid, 0)` check then reports OpenChamber as "already running" and aborts startup — an infinite crashloop under systemd Restart=always while the port is actually free (issue #1721). Verify identity, not just liveness, but only where it belongs: - Add isOpenchamberProcessRunning(pid) = liveness + command-line identity, and use it ONLY at the two sites that validate a PID read from a pid file (the "already running" guard and the stale pid-file cleanup sweep). isProcessRunning stays liveness-only for PIDs we know are ours (a freshly spawned daemon child, processes we are stopping), so those paths cannot get a false negative. - Identity works on Linux (/proc/<pid>/cmdline) and macOS (ps -o command=); on Windows or where the command line can't be read it falls back to liveness, so behaviour is unchanged there with no false negatives. - Match the "openchamber" install-path segment (present for both @openchamber/web and a source checkout, foreground and daemon entrypoints alike) so a recycled stranger such as npm-cli.js or agentmemory is not mistaken for us. - Clear the stale pid file once its recorded PID is no longer our process. Adds unit tests for isOpenchamberCmdline and isOpenchamberProcessRunning, covering the recycled-PID cases and a live non-OpenChamber process.
This commit is contained in:
+74
-3
@@ -2390,6 +2390,11 @@ function removeInstanceFile(instanceFilePath) {
|
||||
}
|
||||
}
|
||||
|
||||
// Liveness only — "is *some* process alive with this PID". Use this when the
|
||||
// PID is known to be ours (a child we just spawned, or a process we are
|
||||
// stopping). Do NOT use it to validate a PID read from a pid file: after an
|
||||
// ungraceful shutdown the pid file is stale and the kernel may have recycled
|
||||
// that PID to an unrelated process — see isOpenchamberProcessRunning.
|
||||
function isProcessRunning(pid) {
|
||||
try {
|
||||
process.kill(pid, 0);
|
||||
@@ -2399,6 +2404,64 @@ function isProcessRunning(pid) {
|
||||
}
|
||||
}
|
||||
|
||||
// Best-effort command line for a live PID, used for identity verification.
|
||||
// Returns the cmdline string, '' when the process has no readable cmdline, or
|
||||
// null when identity can't be determined on this platform (caller falls back to
|
||||
// liveness — so behaviour is unchanged where we can't check).
|
||||
function readProcessCmdline(pid) {
|
||||
try {
|
||||
if (process.platform === 'linux') {
|
||||
// /proc/<pid>/cmdline is a NUL-delimited argv list.
|
||||
return fs.readFileSync(`/proc/${pid}/cmdline`, 'utf8').replace(/\0/g, ' ').trim();
|
||||
}
|
||||
if (process.platform === 'darwin') {
|
||||
const result = spawnSync('ps', ['-p', String(pid), '-o', 'command='], {
|
||||
encoding: 'utf8',
|
||||
timeout: 3000,
|
||||
windowsHide: true,
|
||||
});
|
||||
const out = (result.stdout || '').trim();
|
||||
return out.length > 0 ? out : null;
|
||||
}
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
// Windows / other: a process's full command line isn't cheaply available, so
|
||||
// we can't verify identity — fall back to liveness-only.
|
||||
return null;
|
||||
}
|
||||
|
||||
function isOpenchamberCmdline(cmdline) {
|
||||
if (typeof cmdline !== 'string' || cmdline.length === 0) {
|
||||
return false;
|
||||
}
|
||||
// Every install path contains the "openchamber" segment — the npm package
|
||||
// (@openchamber/web) and the source checkout both do, for the foreground
|
||||
// (bin/cli.js) and daemon (server/index.js) entrypoints alike. Matching the
|
||||
// path segment (not a generic "cli.js") keeps a recycled stranger such as
|
||||
// "npm-cli.js" or "agentmemory" from being mistaken for us.
|
||||
return cmdline.toLowerCase().includes('openchamber');
|
||||
}
|
||||
|
||||
// Liveness + identity — "is the OpenChamber instance recorded in a pid file
|
||||
// still the process running under this PID". Use this (not isProcessRunning)
|
||||
// when validating a PID read from a pid file. After an ungraceful shutdown
|
||||
// removePidFile never runs, so the stale PID can be recycled to an unrelated
|
||||
// process; a liveness-only check then reports us as "already running" and aborts
|
||||
// startup, which loops forever under systemd Restart=always (issue #1721).
|
||||
// Where identity can't be determined (Windows, unreadable /proc or ps), we fall
|
||||
// back to liveness so there are no false negatives on those platforms.
|
||||
function isOpenchamberProcessRunning(pid) {
|
||||
if (!isProcessRunning(pid)) {
|
||||
return false;
|
||||
}
|
||||
const cmdline = readProcessCmdline(pid);
|
||||
if (cmdline === null) {
|
||||
return true;
|
||||
}
|
||||
return isOpenchamberCmdline(cmdline);
|
||||
}
|
||||
|
||||
function waitForProcessExit(pid, timeoutMs) {
|
||||
if (!Number.isFinite(pid) || pid <= 0) {
|
||||
return Promise.resolve(true);
|
||||
@@ -2702,7 +2765,7 @@ async function discoverRunningInstances() {
|
||||
if (!Number.isFinite(port) || port <= 0) continue;
|
||||
const pidFilePath = path.join(runDir, file);
|
||||
const pid = readPidFile(pidFilePath);
|
||||
if (!pid || !isProcessRunning(pid)) {
|
||||
if (!pid || !isOpenchamberProcessRunning(pid)) {
|
||||
removePidFile(pidFilePath);
|
||||
removeInstanceFile(path.join(runDir, `openchamber-${port}.json`));
|
||||
continue;
|
||||
@@ -3430,8 +3493,13 @@ const commands = {
|
||||
if (targetPort !== 0) {
|
||||
const pidFilePath = await getPidFilePath(targetPort);
|
||||
const existingPid = readPidFile(pidFilePath);
|
||||
if (existingPid && isProcessRunning(existingPid)) {
|
||||
throw new Error(`OpenChamber is already running on port ${targetPort} (PID: ${existingPid})`);
|
||||
if (existingPid) {
|
||||
if (isOpenchamberProcessRunning(existingPid)) {
|
||||
throw new Error(`OpenChamber is already running on port ${targetPort} (PID: ${existingPid})`);
|
||||
}
|
||||
// Stale pid file from an ungraceful shutdown (PID dead or recycled to an
|
||||
// unrelated process). Clear it so it can't trip later checks.
|
||||
removePidFile(pidFilePath);
|
||||
}
|
||||
|
||||
if (explicitPort && !(await isPortAvailable(targetPort, options.host))) {
|
||||
@@ -5734,6 +5802,9 @@ export {
|
||||
isValidTunnelDoctorResponse,
|
||||
readDesktopLocalPortFromSettings,
|
||||
getPidFilePath,
|
||||
isProcessRunning,
|
||||
isOpenchamberProcessRunning,
|
||||
isOpenchamberCmdline,
|
||||
resolveTunnelProviders,
|
||||
fetchTunnelProvidersFromPort,
|
||||
fetchSystemInfoFromPort,
|
||||
|
||||
@@ -1,9 +1,15 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import path from 'path';
|
||||
import { spawn } from 'child_process';
|
||||
import { pathToFileURL } from 'url';
|
||||
|
||||
import { isModuleCliExecution, normalizeCliEntryPath } from './cli-entry.js';
|
||||
import { assertAuthenticatedNetworkExposure, parseArgs } from './cli.js';
|
||||
import {
|
||||
assertAuthenticatedNetworkExposure,
|
||||
isOpenchamberCmdline,
|
||||
isOpenchamberProcessRunning,
|
||||
parseArgs,
|
||||
} from './cli.js';
|
||||
|
||||
describe('cli args', () => {
|
||||
it('accepts legacy daemon flags as no-ops', () => {
|
||||
@@ -155,3 +161,40 @@ describe('cli entry detection', () => {
|
||||
expect(normalizeCliEntryPath(unresolvedPath, realpath)).toBe(path.resolve(unresolvedPath));
|
||||
});
|
||||
});
|
||||
|
||||
describe('isOpenchamberCmdline', () => {
|
||||
it('accepts OpenChamber CLI and daemon cmdlines', () => {
|
||||
expect(isOpenchamberCmdline('node /x/@openchamber/web/bin/cli.js serve')).toBe(true);
|
||||
expect(isOpenchamberCmdline('node /x/@openchamber/web/server/index.js --port 9090')).toBe(true);
|
||||
expect(isOpenchamberCmdline('bun /home/u/projects/openchamber/packages/web/server/index.js --port 3001')).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects recycled and unrelated processes (issue #1721)', () => {
|
||||
expect(isOpenchamberCmdline('node /home/herjarsa/npm-global/bin/agentmemory')).toBe(false);
|
||||
expect(isOpenchamberCmdline('node /usr/lib/node_modules/npm/bin/npm-cli.js install')).toBe(false);
|
||||
expect(isOpenchamberCmdline('')).toBe(false);
|
||||
expect(isOpenchamberCmdline(null)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isOpenchamberProcessRunning', () => {
|
||||
it('returns false for a dead PID', () => {
|
||||
expect(isOpenchamberProcessRunning(2147483646)).toBe(false);
|
||||
});
|
||||
|
||||
// Identity verification is available on Linux (/proc) and macOS (ps); on those
|
||||
// platforms a live but unrelated process (a recycled stale PID) must read as
|
||||
// not-running so it can't trip the "already running" guard (issue #1721).
|
||||
it.skipIf(process.platform !== 'linux' && process.platform !== 'darwin')(
|
||||
'returns false for a live non-OpenChamber PID',
|
||||
async () => {
|
||||
const child = spawn('sleep', ['30'], { stdio: 'ignore' });
|
||||
try {
|
||||
await new Promise((resolve) => setTimeout(resolve, 150));
|
||||
expect(isOpenchamberProcessRunning(child.pid)).toBe(false);
|
||||
} finally {
|
||||
child.kill('SIGKILL');
|
||||
}
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user