diff --git a/lib/tui/session.mjs b/lib/tui/session.mjs index 3a8c583..1054618 100644 --- a/lib/tui/session.mjs +++ b/lib/tui/session.mjs @@ -15,14 +15,48 @@ import { tmpdir } from "node:os"; import { randomUUID } from "node:crypto"; import { readTuiTranscript } from "./transcript.mjs"; -export const SESSION_PREFIX = "ocp-tui-"; // per-proxy namespace (coexistence rule) +// F7 fix (audit finding, LOW): the prefix used to be a bare, host-wide constant +// ("ocp-tui-"), so a SECOND OCP instance on the same host (e.g. a temporary +// verification instance stood up alongside production — a real pattern used during +// PR #144/#146 verification) would boot-reap and potentially kill-server the OTHER +// instance's LIVE sessions: the coexistence guard below only ever spared foreign +// PRODUCT prefixes (olp-tui-*), never a second ocp-tui-* instance on a different port. +// +// Fix: scope the prefix to the instance's own listen port. The port is the natural +// stable per-instance discriminator on one host (two OCP instances cannot share a +// port), so `ocp-tui--` uniquely namespaces this instance's sessions and makes +// a same-host sibling OCP instance look exactly like a foreign product (olp-tui-*) to +// the coexistence guard — its `ocp-tui--*` sessions never match our own +// prefix and are therefore never reaped/kill-server'd by us. +// +// LEGACY_SESSION_PREFIX / LEGACY_SESSION_NAME_RE describe the OLD bare-prefix shape +// (pre-this-fix), retained ONLY for the boot-time legacy-zombie migration handled in +// reapStaleTuiSessions (see comment there). No code path in this version ever CREATES +// a legacy-shaped session name again — sessionPrefixForPort() is the only session-name +// prefix constructor used going forward. +export const LEGACY_SESSION_PREFIX = "ocp-tui-"; +// Exact legacy shape: LEGACY_SESSION_PREFIX + sessionId.slice(0, 8), where sessionId is +// a randomUUID() — so the suffix is always exactly 8 lowercase hex characters with NO +// further separator. The new port-scoped shape always inserts a "-" between the port +// digits and the 8-hex suffix (see sessionPrefixForPort), so this regex can never match +// a new-shape name: a new-shape suffix is `-<8 hex>` (contains a literal +// "-"), which `[0-9a-f]{8}$` anchored immediately after the prefix cannot satisfy. +export const LEGACY_SESSION_NAME_RE = /^ocp-tui-[0-9a-f]{8}$/; + +// Build this instance's own session-name prefix, scoped by its listen port so a +// second OCP instance on the same host (different port) is never mistaken for "ours". +export function sessionPrefixForPort(port) { + return `ocp-tui-${port}-`; +} + const TMUX = process.env.OCP_TUI_TMUX_BIN || "tmux"; const defaultTmux = (args, opts = {}) => spawnSync(TMUX, args, { encoding: "utf8", ...opts }); -// Kill ONLY our own stale sessions. Scoped to SESSION_PREFIX so a co-hosted -// OLP test instance's `olp-tui-*` sessions are never touched. +// Kill ONLY our own stale sessions. Scoped to sessionPrefixForPort(port) so a co-hosted +// OLP test instance's `olp-tui-*` sessions — AND a co-hosted second OCP instance's +// `ocp-tui--*` sessions — are never touched (F7 fix). // // Defunct-reaping (PI231 incident): the pane's `claude` process is a child of the // long-lived tmux SERVER daemon, NOT of the OCP node process — `tmux new-session -d` @@ -36,23 +70,40 @@ const defaultTmux = (args, opts = {}) => // merely re-signalling — is to stop the tmux server: when the server exits, the kernel // reparents its surviving children to init (PID 1), which reaps them immediately. // -// So after killing our own sessions, if the server has NO sessions left of ANY prefix -// (i.e. nothing we could disrupt — no co-hosted `olp-tui-*` or other instance), we -// `kill-server` to flush the defunct backlog. If ANY non-ocp session remains we leave the -// server running (coexistence rule, ADR 0007) and let the next boot/periodic sweep retry -// once the server is otherwise idle. -export function reapStaleTuiSessions({ tmux = defaultTmux } = {}) { +// `port` (required) is this instance's own listen port (server.mjs's PORT / lib/constants.mjs +// DEFAULT_PORT resolution) — the SPOT for "which sessions are ours." +// +// `includeLegacy` (default false): when true, sessions matching the exact OLD bare-prefix +// shape (LEGACY_SESSION_NAME_RE) are ALSO treated as ours for kill-session purposes. This is +// the boot-time legacy migration: an operator upgrading past this fix could otherwise be left +// with orphaned bare-prefix zombie sessions from the PREVIOUS (pre-fix) process generation of +// this SAME instance, since no live instance of the new version ever creates that shape again +// — a legacy-shaped session found at boot is therefore presumed to be this instance's own +// leftover, not a stranger's. Passed true ONLY from the one-time boot-reap call site in +// server.mjs; the periodic idle-reap sweep does NOT set it, so a lingering legacy session +// during steady-state is conservatively treated as foreign (correctly blocking kill-server) +// rather than assumed to be ours on every 15-minute tick. Residual (accepted, documented): +// if a genuinely-still-running PRE-FIX OCP instance is coexisting on the same host at the +// exact moment a new instance boots, its live legacy-shaped session could be reaped — the +// same class of residual risk the audit finding itself accepts ("no live instance of the new +// version creates them"); this PR does not regress that scenario, it only removes the far +// more common same-version collision (the actual F7 finding). +export function reapStaleTuiSessions({ tmux = defaultTmux, port, includeLegacy = false } = {}) { const r = tmux(["list-sessions", "-F", "#{session_name}"]); if (!r || r.status !== 0) return 0; // no tmux server / no sessions const names = String(r.stdout || "").split("\n").map((s) => s.trim()).filter(Boolean); + const ownPrefix = sessionPrefixForPort(port); let killed = 0; let othersRemain = false; for (const name of names) { - if (name.startsWith(SESSION_PREFIX)) { + const isOwn = name.startsWith(ownPrefix); + const isLegacyOwn = includeLegacy && LEGACY_SESSION_NAME_RE.test(name); + if (isOwn || isLegacyOwn) { tmux(["kill-session", "-t", name]); killed++; } else { - othersRemain = true; // a session we do NOT own (e.g. olp-tui-*) — never kill-server + othersRemain = true; // a session we do NOT own (olp-tui-*, a sibling ocp-tui--*, + // or — outside includeLegacy — a legacy-shaped name) — never kill-server } } // Reap defunct `claude` zombies: safe ONLY when the server is now ours-only/empty. @@ -358,12 +409,15 @@ export async function runTuiTurn({ home, realHome, cwd, + port, wallclockMs = 120000, entrypointMode = "cli", tmux = defaultTmux, }) { const sessionId = randomUUID(); - const tmuxName = SESSION_PREFIX + sessionId.slice(0, 8); + // Port-scoped session name (F7 fix) — see sessionPrefixForPort / reapStaleTuiSessions + // for why this instance's own listen port is the namespace discriminator. + const tmuxName = sessionPrefixForPort(port) + sessionId.slice(0, 8); const ehome = home || process.env.HOME; // HOME claude runs under (scratch or real) const rhome = realHome || process.env.HOME; // real home (OAuth + onboarded config source) diff --git a/server.mjs b/server.mjs index 8341070..ae669b1 100644 --- a/server.mjs +++ b/server.mjs @@ -671,7 +671,13 @@ const TUI_REAP_INTERVAL_MS = 15 * 60 * 1000; const tuiReapInterval = TUI_MODE ? setInterval(() => { if (tuiSemaphore.inflight > 0 || tuiSemaphore.queued > 0) return; // a turn is live — defer try { - const n = reapStaleTuiSessions(); + // F7 fix: scope to THIS instance's own port; a sibling ocp-tui--* session + // (a second OCP instance on the same host) is treated as foreign, same as olp-tui-*. + // includeLegacy is NOT set here — see reapStaleTuiSessions' comment: the periodic sweep + // conservatively treats any lingering bare-prefix legacy session as foreign so it can + // never trigger kill-server on a steady-state tick; only the one-time boot reap below + // claims legacy-shaped zombies. + const n = reapStaleTuiSessions({ port: PORT }); if (n) logEvent("info", "tui_reaped_stale_sessions", { count: n, trigger: "periodic" }); } catch (e) { logEvent("error", "tui_periodic_reap_failed", { error: e.message }); } }, TUI_REAP_INTERVAL_MS) : null; @@ -1168,6 +1174,8 @@ function callClaudeTui(model, messages, _conversationId, _keyName) { home: TUI_HOME, realHome: process.env.HOME, cwd: TUI_CWD, + port: PORT, // F7 fix: port-scopes the tmux session name so a sibling OCP instance on a + // different port never collides with this instance's reap/kill-server logic. wallclockMs: TUI_WALLCLOCK_MS, entrypointMode: TUI_ENTRYPOINT, }).then(({ text, entrypoint, truncated }) => { @@ -2631,7 +2639,11 @@ server.listen(PORT, BIND_ADDRESS, () => { : "credentials.json (no CLAUDE_CODE_OAUTH_TOKEN — see Troubleshooting #401)"; console.log(` TUI-mode: ON home=${TUI_HOME} cwd=${TUI_CWD} auth=${tuiAuth} wallclock=${TUI_WALLCLOCK_MS}ms maxConcurrent=${TUI_MAX_CONCURRENT}`); try { - const n = reapStaleTuiSessions(); + // F7 fix: scope to THIS instance's own port (see reapStaleTuiSessions). includeLegacy: + // true ONLY here — the one-time boot reap is the designated point to claim orphaned + // bare-prefix ("ocp-tui-") zombie sessions left by a PRE-fix process generation + // of this same instance (no live post-fix instance ever creates that shape again). + const n = reapStaleTuiSessions({ port: PORT, includeLegacy: true }); if (n) logEvent("info", "tui_reaped_stale_sessions", { count: n }); } catch {} } diff --git a/test-features.mjs b/test-features.mjs index 57befb7..2a25c10 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -1652,12 +1652,23 @@ await asyncTest("readTuiTranscript throws when no text and cap elapses", async ( }); // ── TUI session reaper ─────────────────────────────────────────────────── -import { reapStaleTuiSessions, SESSION_PREFIX, buildTuiCmd } from "./lib/tui/session.mjs"; +import { reapStaleTuiSessions, sessionPrefixForPort, LEGACY_SESSION_PREFIX, LEGACY_SESSION_NAME_RE, buildTuiCmd } from "./lib/tui/session.mjs"; console.log("\nTUI session reaper:"); -test("SESSION_PREFIX is ocp-tui-", () => { - assert.equal(SESSION_PREFIX, "ocp-tui-"); +// F7 fix: the session prefix is instance-scoped by listen port so a second OCP +// instance on the same host (different port) is never mistaken for "ours". +test("sessionPrefixForPort embeds the port (F7 instance scoping)", () => { + assert.equal(sessionPrefixForPort(3456), "ocp-tui-3456-"); + assert.equal(sessionPrefixForPort(4000), "ocp-tui-4000-"); + assert.notEqual(sessionPrefixForPort(3456), sessionPrefixForPort(4000)); +}); + +test("LEGACY_SESSION_NAME_RE matches only the exact old bare-prefix shape, never the new shape", () => { + assert.ok(LEGACY_SESSION_NAME_RE.test(`${LEGACY_SESSION_PREFIX}a1b2c3d4`), "legacy 8-hex shape matches"); + assert.ok(!LEGACY_SESSION_NAME_RE.test("ocp-tui-3456-a1b2c3d4"), "new port-scoped shape must NOT match legacy regex"); + assert.ok(!LEGACY_SESSION_NAME_RE.test("ocp-tui-a1b2c3"), "too-short suffix must not match"); + assert.ok(!LEGACY_SESSION_NAME_RE.test("ocp-tui-a1b2c3d4extra"), "trailing extra chars must not match"); }); console.log("\nTUI command construction (proxy-purity / #4):"); @@ -1764,22 +1775,40 @@ test("buildTuiCmd OCP_TUI_FULL_TOOLS=1 grants -p-equivalent tool surface (single } }); -test("reaper kills ONLY ocp-tui- sessions, never olp-tui-", () => { +test("reaper kills ONLY this instance's own port-scoped sessions, never olp-tui-", () => { const killed = []; const fakeTmux = (args) => { - if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-aaaa\nolp-tui-bbbb\nmisc\nocp-tui-cccc\n" }; + if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-3456-aaaa\nolp-tui-bbbb\nmisc\nocp-tui-3456-cccc\n" }; if (args[0] === "kill-session") { killed.push(args[args.indexOf("-t") + 1]); return { status: 0 }; } return { status: 0, stdout: "" }; }; - const n = reapStaleTuiSessions({ tmux: fakeTmux }); + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456 }); assert.equal(n, 2); - assert.equal(killed.join(","), "ocp-tui-aaaa,ocp-tui-cccc"); + assert.equal(killed.join(","), "ocp-tui-3456-aaaa,ocp-tui-3456-cccc"); assert.ok(!killed.includes("olp-tui-bbbb"), "olp-tui-bbbb must never be killed"); }); +// F7 fix: a second OCP instance on the same host (different port) must be treated exactly +// like a foreign product prefix — never reaped, never allowed to trigger kill-server. +test("reaper treats a sibling OCP instance on a DIFFERENT port as foreign (F7)", () => { + const killed = []; + const calls = []; + const fakeTmux = (args) => { + calls.push(args.join(" ")); + if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-3456-aaaa\nocp-tui-9999-bbbb\n" }; + if (args[0] === "kill-session") { killed.push(args[args.indexOf("-t") + 1]); return { status: 0 }; } + return { status: 0, stdout: "" }; + }; + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456 }); + assert.equal(n, 1, "killed only the own-port session"); + assert.equal(killed.join(","), "ocp-tui-3456-aaaa"); + assert.ok(!killed.includes("ocp-tui-9999-bbbb"), "sibling instance's session (port 9999) must NEVER be killed"); + assert.ok(!calls.includes("kill-server"), "kill-server MUST NOT fire — sibling instance's session still live"); +}); + test("reaper returns 0 when tmux status !== 0 (no server)", () => { const fakeTmux = (_args) => ({ status: 1, stdout: "" }); - const n = reapStaleTuiSessions({ tmux: fakeTmux }); + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456 }); assert.equal(n, 0); }); @@ -1790,7 +1819,7 @@ test("reaper returns 0 for empty session list", () => { if (args[0] === "kill-session") { killed.push(args[args.indexOf("-t") + 1]); return { status: 0 }; } return { status: 0, stdout: "" }; }; - const n = reapStaleTuiSessions({ tmux: fakeTmux }); + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456 }); assert.equal(n, 0); assert.equal(killed.length, 0); }); @@ -1803,10 +1832,10 @@ test("reaper kill-servers when the server is ours-only (flush defunct claude zom const calls = []; const fakeTmux = (args) => { calls.push(args.join(" ")); - if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-aaaa\nocp-tui-bbbb\n" }; + if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-3456-aaaa\nocp-tui-3456-bbbb\n" }; return { status: 0, stdout: "" }; }; - const n = reapStaleTuiSessions({ tmux: fakeTmux }); + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456 }); assert.equal(n, 2, "killed both of our sessions"); assert.ok(calls.includes("kill-server"), "kill-server fired — reaps the defunct backlog"); }); @@ -1815,10 +1844,10 @@ test("reaper does NOT kill-server when a foreign (non-ocp) session remains (coex const calls = []; const fakeTmux = (args) => { calls.push(args.join(" ")); - if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-aaaa\nolp-tui-bbbb\n" }; + if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-3456-aaaa\nolp-tui-bbbb\n" }; return { status: 0, stdout: "" }; }; - const n = reapStaleTuiSessions({ tmux: fakeTmux }); + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456 }); assert.equal(n, 1, "killed only our own session"); assert.ok(!calls.includes("kill-server"), "kill-server MUST NOT fire — would disrupt olp-tui-*"); }); @@ -1826,10 +1855,61 @@ test("reaper does NOT kill-server when a foreign (non-ocp) session remains (coex test("reaper does NOT kill-server when there is no server (status !== 0)", () => { const calls = []; const fakeTmux = (args) => { calls.push(args.join(" ")); return { status: 1, stdout: "" }; }; - reapStaleTuiSessions({ tmux: fakeTmux }); + reapStaleTuiSessions({ tmux: fakeTmux, port: 3456 }); assert.ok(!calls.includes("kill-server"), "no server → no kill-server (early return)"); }); +// Legacy migration (F7): pre-fix versions created bare-prefix `ocp-tui-` sessions with +// no port segment. includeLegacy is the boot-only opt-in that claims these as our own leftover +// zombies; the periodic sweep never sets it, so a lingering legacy session cannot trigger +// kill-server on a routine 15-minute tick. +console.log("\nTUI legacy-prefix migration (boot-only reap, F7):"); + +test("reaper leaves legacy bare-prefix sessions untouched by default (includeLegacy unset)", () => { + const killed = []; + const calls = []; + const fakeTmux = (args) => { + calls.push(args.join(" ")); + if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-3456-aaaa\nocp-tui-deadbeef\n" }; + if (args[0] === "kill-session") { killed.push(args[args.indexOf("-t") + 1]); return { status: 0 }; } + return { status: 0, stdout: "" }; + }; + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456 }); + assert.equal(n, 1, "killed only the own-port session"); + assert.ok(!killed.includes("ocp-tui-deadbeef"), "legacy session must NOT be reaped without includeLegacy"); + assert.ok(!calls.includes("kill-server"), "legacy session blocks kill-server when not claimed"); +}); + +test("reaper claims legacy bare-prefix sessions when includeLegacy=true (boot-time migration)", () => { + const killed = []; + const calls = []; + const fakeTmux = (args) => { + calls.push(args.join(" ")); + if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-3456-aaaa\nocp-tui-deadbeef\n" }; + if (args[0] === "kill-session") { killed.push(args[args.indexOf("-t") + 1]); return { status: 0 }; } + return { status: 0, stdout: "" }; + }; + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456, includeLegacy: true }); + assert.equal(n, 2, "both own-port and legacy sessions reaped"); + assert.ok(killed.includes("ocp-tui-deadbeef"), "legacy session claimed as our own leftover"); + assert.ok(calls.includes("kill-server"), "kill-server fires once no foreign/unclaimed session remains"); +}); + +test("reaper with includeLegacy=true still spares a sibling instance's port-scoped session", () => { + const killed = []; + const calls = []; + const fakeTmux = (args) => { + calls.push(args.join(" ")); + if (args[0] === "list-sessions") return { status: 0, stdout: "ocp-tui-3456-aaaa\nocp-tui-deadbeef\nocp-tui-9999-zzzz\n" }; + if (args[0] === "kill-session") { killed.push(args[args.indexOf("-t") + 1]); return { status: 0 }; } + return { status: 0, stdout: "" }; + }; + const n = reapStaleTuiSessions({ tmux: fakeTmux, port: 3456, includeLegacy: true }); + assert.equal(n, 2, "own-port + legacy reaped, sibling instance untouched"); + assert.ok(!killed.includes("ocp-tui-9999-zzzz"), "sibling instance session must never be claimed as legacy"); + assert.ok(!calls.includes("kill-server"), "sibling instance's live session still blocks kill-server"); +}); + // ── TUI home preparation (scratch vs real) ─────────────────────────────── import { prepareTuiHome, ensureTuiCwdTrusted } from "./lib/tui/session.mjs"; import { mkdtempSync as hMkdtemp, mkdirSync as hMkdir, writeFileSync as hWrite, readFileSync as hRead, existsSync as hExists, readlinkSync as hReadlink } from "node:fs";