From 1558364b496e2e920f7074ca4a0752544b265178 Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Wed, 24 Jun 2026 17:23:20 +0300 Subject: [PATCH] fix(cli): verify process identity when validating server pid files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After an ungraceful shutdown removePidFile never runs, so a stale run/openchamber-.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//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. --- packages/web/bin/cli.js | 77 ++++++++++++++++++++++++++++++++++-- packages/web/bin/cli.test.js | 45 ++++++++++++++++++++- 2 files changed, 118 insertions(+), 4 deletions(-) diff --git a/packages/web/bin/cli.js b/packages/web/bin/cli.js index dbc15dba..d7fd5fdc 100755 --- a/packages/web/bin/cli.js +++ b/packages/web/bin/cli.js @@ -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//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, diff --git a/packages/web/bin/cli.test.js b/packages/web/bin/cli.test.js index 7c4fea16..c4b21afb 100644 --- a/packages/web/bin/cli.test.js +++ b/packages/web/bin/cli.test.js @@ -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'); + } + } + ); +});