From 67429a4c2c26e49d5c2c188a9261a96a346f68b9 Mon Sep 17 00:00:00 2001 From: TheRealDinghyDog Date: Fri, 10 Jul 2026 21:52:30 +0200 Subject: [PATCH] Verify server PID identity before teardown kills (#31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stale persisted server records were torn down by signalling the stored PID and its process group with no check that the PID still belonged to the plugin's `opencode serve` — after OS PID reuse this could SIGTERM an unrelated process. Teardown now re-reads the PID's live command line (cross-platform, via the same helper the task-worker guard uses) and signals only when it still looks like the plugin-owned `opencode serve` for the session's port. The port is derived from the persisted URL when the explicit field is absent, so records written before this change remain verifiable instead of falling open. Anything unverifiable is left untouched; only the stale metadata is cleared, reported as { killSkipped: true, reason, diagnostic } — `skipped` keeps its existing "teardown did not run" meaning (leases/lock timeouts), so the SessionEnd hook still clears the record correctly. Cancel logs the skip. The matcher requires an opencode-ish executable token plus `serve --port `, and re-splits shell-wrapped Windows command lines (cmd.exe /c "opencode serve ...") so win32 servers do not become unkillable orphans. The spawn-time command line is recorded for forensics; verification always matches the live value. Drafted by OpenCode via the plugin's own rescue path (issue #31 task), then reviewed and reworked: fail-closed for legacy records, URL-derived port, de-duplicated cleanup, return-shape fix for the SessionEnd hook, Windows shell-blob matching, and expanded tests. Co-Authored-By: Claude Fable 5 --- plugins/opencode/scripts/lib/process.mjs | 40 ++++ .../opencode/scripts/lib/server-lifecycle.mjs | 92 +++++++- .../opencode/scripts/opencode-companion.mjs | 5 +- tests/server-lifecycle.test.mjs | 223 +++++++++++++++++- 4 files changed, 345 insertions(+), 15 deletions(-) diff --git a/plugins/opencode/scripts/lib/process.mjs b/plugins/opencode/scripts/lib/process.mjs index f04a1b5..32773e9 100644 --- a/plugins/opencode/scripts/lib/process.mjs +++ b/plugins/opencode/scripts/lib/process.mjs @@ -174,6 +174,46 @@ export function commandLineLooksLikeTaskWorker(commandLine, { jobId } = {}) { ); } +function tokensContainPort(tokens, port) { + const expected = String(port ?? ""); + if (!expected) { + return false; + } + + for (let index = 0; index < tokens.length; index += 1) { + if (tokens[index] === "--port" && tokens[index + 1] === expected) { + return true; + } + if (tokens[index] === `--port=${expected}`) { + return true; + } + } + return false; +} + +export function commandLineLooksLikeOpencodeServe(commandLine, { port } = {}) { + // On win32 the server is spawned through a shell, so `ps`/PowerShell can + // report the whole invocation as one quoted blob (`cmd.exe /c "opencode + // serve --port N"`). Re-split compound tokens so the matcher sees the real + // arguments on every platform. + const tokens = commandLineTokens(commandLine).flatMap((token) => + /\s/.test(token) ? token.split(/\s+/).filter(Boolean) : [token] + ); + // Require an opencode-ish executable token in addition to `serve --port N`. + // Matches the real binary (/opt/homebrew/bin/opencode), test fixtures + // (/opencode), and Windows shims (opencode.cmd). + if (!tokens.some((token) => path.basename(token).toLowerCase().startsWith("opencode"))) { + return false; + } + if (!tokens.includes("serve")) { + return false; + } + if (!tokensContainPort(tokens, port)) { + return false; + } + return true; +} + export function readProcessCommandLine(pid, options = {}) { if (!Number.isFinite(pid)) { return null; diff --git a/plugins/opencode/scripts/lib/server-lifecycle.mjs b/plugins/opencode/scripts/lib/server-lifecycle.mjs index e01992b..50be8f5 100644 --- a/plugins/opencode/scripts/lib/server-lifecycle.mjs +++ b/plugins/opencode/scripts/lib/server-lifecycle.mjs @@ -7,6 +7,7 @@ import process from "node:process"; import { spawn } from "node:child_process"; import { OpencodeServerClient } from "./opencode-server.mjs"; +import { commandLineLooksLikeOpencodeServe, readProcessCommandLine } from "./process.mjs"; import { atomicWriteFile, resolveStateDir } from "./state.mjs"; export const SERVER_URL_ENV = "OPENCODE_COMPANION_SERVER_URL"; @@ -432,7 +433,7 @@ export async function ensureServer(cwd, options = {}) { const staleExisting = loadServerSession(cwd); if (staleExisting) { - const { url, pidFile, logFile, sessionDir, pid, external, password, username } = staleExisting; + const { url, pidFile, logFile, sessionDir, pid, external, password, username, port } = staleExisting; // The server lock is already held here; intentionally omit cwd so teardown // uses the unlocked path even if the persisted session schema grows. await teardownServerSession({ @@ -444,7 +445,9 @@ export async function ensureServer(cwd, options = {}) { external, password, username, - killProcess: options.killProcess ?? null + port, + killProcess: options.killProcess ?? null, + readProcessCommandLineImpl: options.readProcessCommandLineImpl ?? null }); clearServerSession(cwd); } @@ -467,6 +470,10 @@ export async function ensureServer(cwd, options = {}) { env: options.env ?? process.env, password }); + // Recorded for forensics (compare against the live command line when an + // identity-mismatch teardown skip is investigated); verification itself + // matches the LIVE command line against `opencode serve --port `. + const pidCommandLine = readProcessCommandLine(child.pid, options); const ready = await waitForServerHealth(url, options.timeoutMs ?? 10000, { password, @@ -481,7 +488,9 @@ export async function ensureServer(cwd, options = {}) { pid: child.pid ?? null, password, username: OWNED_SERVER_USERNAME, - killProcess: options.killProcess ?? null + port, + killProcess: options.killProcess ?? null, + readProcessCommandLineImpl: options.readProcessCommandLineImpl ?? null }); return null; } @@ -494,7 +503,9 @@ export async function ensureServer(cwd, options = {}) { sessionDir, external: false, password, - username: OWNED_SERVER_USERNAME + username: OWNED_SERVER_USERNAME, + port, + pidCommandLine }; const leasedSession = addServerLease(session, options); saveServerSession(cwd, leasedSession); @@ -504,6 +515,37 @@ export async function ensureServer(cwd, options = {}) { } } +function parseServerUrlPort(url) { + try { + const port = Number(new URL(String(url ?? "")).port); + return Number.isInteger(port) && port > 0 ? port : null; + } catch { + return null; + } +} + +// Fail-closed PID identity check (issue #31): only signal a PID when its live +// command line still looks like the plugin-owned `opencode serve` instance for +// this session's port. The port always exists for owned sessions — it is part +// of the persisted URL — so records written before the explicit identity +// fields existed remain verifiable. Anything unverifiable is left untouched; +// only the stale metadata is cleared. +function verifyServerPidIdentity(pid, { url = null, port = null, readProcessCommandLineImpl = null } = {}) { + const expectedPort = Number.isFinite(port) ? Number(port) : parseServerUrlPort(url); + if (!Number.isFinite(expectedPort)) { + return { verified: false, reason: "identity-unverified" }; + } + const readCommandLine = readProcessCommandLineImpl ?? readProcessCommandLine; + const commandLine = readCommandLine(pid); + if (!commandLine) { + return { verified: false, reason: "identity-unverified" }; + } + if (!commandLineLooksLikeOpencodeServe(commandLine, { port: expectedPort })) { + return { verified: false, reason: "identity-mismatch" }; + } + return { verified: true, reason: null }; +} + async function teardownServerSessionUnlocked({ url = null, pidFile = null, @@ -513,7 +555,9 @@ async function teardownServerSessionUnlocked({ external = false, password = null, username = null, - killProcess = null + killProcess = null, + port = null, + readProcessCommandLineImpl = null } = {}) { if (url && !external) { try { @@ -526,11 +570,17 @@ async function teardownServerSessionUnlocked({ } } + let killSkippedReason = null; if (!external && Number.isFinite(pid)) { - try { - killServerPid(pid, killProcess); - } catch { - // Ignore teardown failures during Claude session shutdown. + const identity = verifyServerPidIdentity(pid, { url, port, readProcessCommandLineImpl }); + if (identity.verified) { + try { + killServerPid(pid, killProcess); + } catch { + // Ignore teardown failures during Claude session shutdown. + } + } else { + killSkippedReason = identity.reason; } } @@ -550,6 +600,18 @@ async function teardownServerSessionUnlocked({ } } + if (killSkippedReason) { + // Metadata is cleared (so callers still clear the session record), but the + // process was deliberately left untouched. `skipped` keeps its existing + // meaning of "teardown did not run at all" (leases / lock timeouts). + return { + skipped: false, + killSkipped: true, + reason: killSkippedReason, + diagnostic: `Left PID ${pid} untouched (${killSkippedReason}); cleared stale OpenCode server metadata only.` + }; + } + return { skipped: false }; } @@ -568,7 +630,9 @@ export async function teardownServerSession({ external = false, password = null, username = null, - killProcess = null + killProcess = null, + port = null, + readProcessCommandLineImpl = null } = {}) { if (!cwd) { return teardownServerSessionUnlocked({ @@ -580,7 +644,9 @@ export async function teardownServerSession({ external, password, username, - killProcess + killProcess, + port, + readProcessCommandLineImpl }); } @@ -615,7 +681,9 @@ export async function teardownServerSession({ external: Boolean(session?.external ?? external), password: session?.password ?? password, username: session?.username ?? username, - killProcess + killProcess, + port: session?.port ?? port, + readProcessCommandLineImpl }; const result = await teardownServerSessionUnlocked(teardownTarget); if (session) { diff --git a/plugins/opencode/scripts/opencode-companion.mjs b/plugins/opencode/scripts/opencode-companion.mjs index fb4be68..4a09b2c 100644 --- a/plugins/opencode/scripts/opencode-companion.mjs +++ b/plugins/opencode/scripts/opencode-companion.mjs @@ -1122,11 +1122,14 @@ async function handleCancel(argv) { ? serverExternal === false : interrupt.serverExternal !== true; if (ownedServerTeardown) { - await teardownServerSession({ + const teardownResult = await teardownServerSession({ cwd: workspaceRoot, ignoreCurrentProcessLease: Boolean(interrupt.serverUrl && !serverUrl), ...(teardownServerUrl ? { url: teardownServerUrl } : {}) }); + if (teardownResult?.killSkipped) { + appendLogLine(cancelLogFile, `Skipped OpenCode server kill: ${teardownResult.reason}.`); + } } } catch (error) { appendLogLine( diff --git a/tests/server-lifecycle.test.mjs b/tests/server-lifecycle.test.mjs index 1d77341..0e85419 100644 --- a/tests/server-lifecycle.test.mjs +++ b/tests/server-lifecycle.test.mjs @@ -9,6 +9,7 @@ import assert from "node:assert/strict"; import { makeTempDir } from "./helpers.mjs"; import { ensureServer, isServerHealthy, loadServerSession, saveServerSession, teardownServerSession } from "../plugins/opencode/scripts/lib/server-lifecycle.mjs"; import { resolveStateDir } from "../plugins/opencode/scripts/lib/state.mjs"; +import { commandLineLooksLikeOpencodeServe } from "../plugins/opencode/scripts/lib/process.mjs"; async function canListenLocalhost() { return new Promise((resolve) => { @@ -233,7 +234,8 @@ test("teardownServerSession can ignore only this process lease", async () => { ignoreCurrentProcessLease: true, killProcess: (pid) => { killedPid = pid; - } + }, + readProcessCommandLineImpl: () => "opencode serve --hostname 127.0.0.1 --port 1" }); assert.equal(result.skipped, false); @@ -443,7 +445,8 @@ test("teardownServerSession expires a lease whose pid reports EPERM", async () = pid: session.pid, killProcess: (pid) => { killedPid = pid; - } + }, + readProcessCommandLineImpl: () => "opencode serve --hostname 127.0.0.1 --port 1" }); assert.equal(result.skipped, false); @@ -459,6 +462,222 @@ test("teardownServerSession expires a lease whose pid reports EPERM", async () = } }); +test("teardownServerSession signals the process when the persisted identity matches the live command line", async () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const session = { + url: "http://127.0.0.1:1", + pid: 123456, + pidFile: null, + logFile: null, + sessionDir: null, + external: false, + port: 8080, + pidCommandLine: "opencode serve --hostname 127.0.0.1 --port 8080", + leases: [] + }; + saveServerSession(workspace, session); + + let killedPid = null; + try { + const result = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => "opencode serve --hostname 127.0.0.1 --port 8080" + }); + + assert.equal(result.skipped, false); + assert.equal(killedPid, session.pid); + assert.equal(loadServerSession(workspace), null); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("teardownServerSession does NOT signal and clears the record when the live command line differs (PID reused)", async () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const session = { + url: "http://127.0.0.1:1", + pid: 123456, + pidFile: null, + logFile: null, + sessionDir: null, + external: false, + port: 8080, + pidCommandLine: "opencode serve --hostname 127.0.0.1 --port 8080", + leases: [] + }; + saveServerSession(workspace, session); + + let killedPid = null; + try { + const result = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => "node /opt/unrelated/server.js --port 3000" + }); + + // The teardown itself ran (metadata cleared, record removed); only the + // kill was withheld — `skipped` keeps meaning "teardown did not run". + assert.equal(result.skipped, false); + assert.equal(result.killSkipped, true); + assert.equal(result.reason, "identity-mismatch"); + assert.match(result.diagnostic, /Left PID 123456 untouched/); + assert.equal(killedPid, null); + assert.equal(loadServerSession(workspace), null); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("teardownServerSession does NOT signal when the command line cannot be read", async () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + const session = { + url: "http://127.0.0.1:1", + pid: 123456, + pidFile: null, + logFile: null, + sessionDir: null, + external: false, + port: 8080, + pidCommandLine: "opencode serve --hostname 127.0.0.1 --port 8080", + leases: [] + }; + saveServerSession(workspace, session); + + let killedPid = null; + try { + const result = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => null + }); + + assert.equal(result.skipped, false); + assert.equal(result.killSkipped, true); + assert.equal(result.reason, "identity-unverified"); + assert.equal(killedPid, null); + assert.equal(loadServerSession(workspace), null); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("teardownServerSession verifies legacy records via the port derived from the persisted url", async () => { + const workspace = makeTempDir(); + const pluginDataDir = makeTempDir(); + const previousPluginDataDir = process.env.CLAUDE_PLUGIN_DATA; + process.env.CLAUDE_PLUGIN_DATA = pluginDataDir; + // A record written before the identity fields existed: no `port`, no + // `pidCommandLine`. Verification must still work (the URL carries the port) + // instead of falling open and killing blind. + const session = { + url: "http://127.0.0.1:43117", + pid: 123456, + pidFile: null, + logFile: null, + sessionDir: null, + external: false, + leases: [] + }; + saveServerSession(workspace, session); + + try { + let killedPid = null; + const matched = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => "/usr/local/bin/opencode serve --hostname 127.0.0.1 --port 43117" + }); + assert.equal(matched.skipped, false); + assert.equal(matched.killSkipped, undefined); + assert.equal(killedPid, session.pid); + assert.equal(loadServerSession(workspace), null); + + // Same legacy record, but the PID now belongs to something else: no kill. + saveServerSession(workspace, session); + killedPid = null; + const reused = await teardownServerSession({ + cwd: workspace, + url: session.url, + pid: session.pid, + killProcess: (pid) => { + killedPid = pid; + }, + readProcessCommandLineImpl: () => "postgres: writer process" + }); + assert.equal(reused.killSkipped, true); + assert.equal(reused.reason, "identity-mismatch"); + assert.equal(killedPid, null); + assert.equal(loadServerSession(workspace), null); + } finally { + if (previousPluginDataDir == null) { + delete process.env.CLAUDE_PLUGIN_DATA; + } else { + process.env.CLAUDE_PLUGIN_DATA = previousPluginDataDir; + } + } +}); + +test("commandLineLooksLikeOpencodeServe matches shell-wrapped Windows command lines", () => { + assert.equal( + commandLineLooksLikeOpencodeServe('C:\\Windows\\system32\\cmd.exe /c "opencode serve --hostname 127.0.0.1 --port 5150"', { + port: 5150 + }), + true + ); + assert.equal( + commandLineLooksLikeOpencodeServe("/opt/homebrew/bin/opencode serve --hostname 127.0.0.1 --port 5150", { port: 5150 }), + true + ); + // Same port, different program: must not match. + assert.equal( + commandLineLooksLikeOpencodeServe("node /srv/other-tool serve --port 5150", { port: 5150 }), + false + ); + // Right program, wrong port: must not match. + assert.equal( + commandLineLooksLikeOpencodeServe("/opt/homebrew/bin/opencode serve --hostname 127.0.0.1 --port 5151", { port: 5150 }), + false + ); +}); + test("saveServerSession preserves the existing session when its atomic rename fails", () => { const workspace = makeTempDir(); const pluginDataDir = makeTempDir();