Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions plugins/codex/scripts/lib/broker-lifecycle.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ export async function ensureBrokerSession(cwd, options = {}) {
pidFile: existing.pidFile ?? null,
logFile: existing.logFile ?? null,
sessionDir: existing.sessionDir ?? null,
pid: existing.pid ?? null,
killProcess: options.killProcess ?? null
});
clearBrokerSession(cwd);
Expand Down Expand Up @@ -153,7 +152,7 @@ export async function ensureBrokerSession(cwd, options = {}) {
pidFile,
logFile,
sessionDir,
pid: child.pid ?? null,
ownedPid: child.pid ?? null,
killProcess: options.killProcess ?? null
});
return null;
Expand All @@ -170,10 +169,10 @@ export async function ensureBrokerSession(cwd, options = {}) {
return session;
}

export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessionDir = null, pid = null, killProcess = null }) {
if (Number.isFinite(pid) && killProcess) {
export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessionDir = null, ownedPid = null, killProcess = null }) {
if (Number.isFinite(ownedPid) && killProcess) {
try {
killProcess(pid);
killProcess(ownedPid);
} catch {
// Ignore missing or already-exited broker processes.
}
Expand Down
2 changes: 0 additions & 2 deletions plugins/codex/scripts/session-lifecycle-hook.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ async function handleSessionEnd(input) {
const pidFile = brokerSession?.pidFile ?? null;
const logFile = brokerSession?.logFile ?? null;
const sessionDir = brokerSession?.sessionDir ?? null;
const pid = brokerSession?.pid ?? null;

if (brokerEndpoint) {
await sendBrokerShutdown(brokerEndpoint);
Expand All @@ -107,7 +106,6 @@ async function handleSessionEnd(input) {
pidFile,
logFile,
sessionDir,
pid,
killProcess: terminateProcessTree
});
clearBrokerSession(cwd);
Expand Down
30 changes: 30 additions & 0 deletions tests/broker-lifecycle.test.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import test from "node:test";
import assert from "node:assert/strict";

import { teardownBrokerSession } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs";

test("teardownBrokerSession does not treat persisted pid metadata as process ownership", () => {
const killed = [];

teardownBrokerSession({
pid: 1234,
killProcess(pid) {
killed.push(pid);
}
});

assert.deepEqual(killed, []);
});

test("teardownBrokerSession may terminate a process explicitly owned by the caller", () => {
const killed = [];

teardownBrokerSession({
ownedPid: 1234,
killProcess(pid) {
killed.push(pid);
}
});

assert.deepEqual(killed, [1234]);
});
38 changes: 38 additions & 0 deletions tests/runtime.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -1801,6 +1801,44 @@ test("cancel sends turn interrupt to the shared app-server before killing a brok
assert.equal(cleanup.status, 0, cleanup.stderr);
});

test("session end does not signal a stale broker pid", (t) => {
const repo = makeTempDir();
initGitRepo(repo);

const sleeper = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], {
detached: true,
stdio: "ignore"
});
sleeper.unref();
t.after(() => {
try {
process.kill(-sleeper.pid, "SIGTERM");
} catch {
// Ignore an already-exited process.
}
});

saveBrokerSession(repo, {
endpoint: null,
pid: sleeper.pid,
pidFile: null,
logFile: null,
sessionDir: null
});

const result = run("node", [SESSION_HOOK, "SessionEnd"], {
cwd: repo,
input: JSON.stringify({
hook_event_name: "SessionEnd",
cwd: repo
})
});

assert.equal(result.status, 0, result.stderr);
assert.doesNotThrow(() => process.kill(sleeper.pid, 0));
assert.equal(loadBrokerSession(repo), null);
});

test("session end fully cleans up jobs for the ending session", async (t) => {
const repo = makeTempDir();
initGitRepo(repo);
Expand Down