Fix Windows SessionEnd hang and taskkill false failures

plugins/codex/scripts/lib/broker-endpoint.mjs

@@ -13,7 +13,7 @@ export function createBrokerEndpoint(sessionDir, platform = process.platform) {
     return `pipe:\\\\.\\pipe\\${pipeName}`;
   }
 
-  return `unix:${path.join(sessionDir, "broker.sock")}`;
+  return `unix:${path.posix.join(sessionDir, "broker.sock")}`;
 }
 
 export function parseBrokerEndpoint(endpoint) {

plugins/codex/scripts/lib/broker-lifecycle.mjs

@@ -40,19 +40,30 @@ export async function waitForBrokerEndpoint(endpoint, timeoutMs = 2000) {
   return false;
 }
 
-export async function sendBrokerShutdown(endpoint) {
+export async function sendBrokerShutdown(endpoint, timeoutMs = 2000) {
   await new Promise((resolve) => {
     const socket = connectToEndpoint(endpoint);
+    const timer = setTimeout(() => {
+      socket.destroy();
+      resolve();
+    }, timeoutMs);
     socket.setEncoding("utf8");
     socket.on("connect", () => {
       socket.write(`${JSON.stringify({ id: 1, method: "broker/shutdown", params: {} })}\n`);
     });
     socket.on("data", () => {
+      clearTimeout(timer);
       socket.end();
       resolve();
     });
-    socket.on("error", resolve);
-    socket.on("close", resolve);
+    socket.on("error", () => {
+      clearTimeout(timer);
+      resolve();
+    });
+    socket.on("close", () => {
+      clearTimeout(timer);
+      resolve();
+    });
   });
 }
 

plugins/codex/scripts/lib/process.mjs

@@ -50,8 +50,13 @@ export function binaryAvailable(command, versionArgs = ["--version"], options =
   return { available: true, detail: result.stdout.trim() || result.stderr.trim() || "ok" };
 }
 
-function looksLikeMissingProcessMessage(text) {
-  return /not found|no running instance|cannot find|does not exist|no such process/i.test(text);
+function isProcessAlive(pid, killImpl) {
+  try {
+    killImpl(pid, 0);
+    return true;
+  } catch (error) {
+    return error?.code !== "ESRCH";
+  }
 }
 
 export function terminateProcessTree(pid, options = {}) {
@@ -64,6 +69,12 @@ export function terminateProcessTree(pid, options = {}) {
   const killImpl = options.killImpl ?? process.kill.bind(process);
 
   if (platform === "win32") {
+    // Probe the root before calling taskkill instead of parsing its localized "not found"
+    // message: a root that is already gone is reported as not delivered in every language.
+    if (!isProcessAlive(pid, killImpl)) {
+      return { attempted: false, delivered: false, method: null };
+    }
+
     const result = runCommandImpl("taskkill", ["/PID", String(pid), "/T", "/F"], {
       cwd: options.cwd,
       env: options.env
@@ -73,9 +84,13 @@ export function terminateProcessTree(pid, options = {}) {
       return { attempted: true, delivered: true, method: "taskkill", result };
     }
 
-    const combinedOutput = `${result.stderr}\n${result.stdout}`.trim();
-    if (!result.error && looksLikeMissingProcessMessage(combinedOutput)) {
-      return { attempted: true, delivered: false, method: "taskkill", result };
+    // taskkill /T walks the tree, then terminates each entry; a descendant that exits in
+    // between (short-lived git/cmd helpers) makes taskkill report "not supported" and a
+    // non-zero status even though the root was killed. Trust the root's liveness instead.
+    // `delivered` therefore describes the root only: like the process-group SIGTERM on
+    // other platforms, it does not prove that every descendant is gone.
+    if (!result.error && !isProcessAlive(pid, killImpl)) {
+      return { attempted: true, delivered: true, method: "taskkill", result };
     }
 
     if (result.error?.code === "ENOENT") {

tests/broker-lifecycle.test.mjs

@@ -0,0 +1,43 @@
+import fs from "node:fs";
+import net from "node:net";
+import os from "node:os";
+import path from "node:path";
+import test from "node:test";
+import assert from "node:assert/strict";
+
+import { sendBrokerShutdown } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs";
+import { createBrokerEndpoint, parseBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs";
+
+test("sendBrokerShutdown times out when the broker accepts but never responds", async (t) => {
+  const sessionDir = fs.mkdtempSync(path.join(os.tmpdir(), "codex-plugin-broker-test-"));
+  const endpoint = createBrokerEndpoint(sessionDir);
+  const { path: endpointPath } = parseBrokerEndpoint(endpoint);
+  const sockets = new Set();
+  const server = net.createServer((socket) => {
+    sockets.add(socket);
+    socket.on("close", () => sockets.delete(socket));
+  });
+
+  t.after(async () => {
+    for (const socket of sockets) {
+      socket.destroy();
+    }
+    await new Promise((resolve) => server.close(resolve));
+    if (process.platform !== "win32" && fs.existsSync(endpointPath)) {
+      fs.unlinkSync(endpointPath);
+    }
+    fs.rmSync(sessionDir, { recursive: true, force: true });
+  });
+
+  await new Promise((resolve, reject) => {
+    server.once("error", reject);
+    server.listen(endpointPath, resolve);
+  });
+
+  const startedAt = Date.now();
+  await sendBrokerShutdown(endpoint, 50);
+  const elapsedMs = Date.now() - startedAt;
+
+  assert.ok(elapsedMs >= 40, `expected timeout path, completed after ${elapsedMs}ms`);
+  assert.ok(elapsedMs < 1000, `shutdown timeout exceeded test budget: ${elapsedMs}ms`);
+});

tests/git.test.mjs

@@ -132,13 +132,27 @@ test("collectReviewContext skips untracked directories in working tree review",
   assert.match(context.content, /### \.claude\/worktrees\/agent-test\/\n\(skipped: directory\)/);
 });
 
-test("collectReviewContext skips broken untracked symlinks instead of crashing", () => {
+test("collectReviewContext skips broken untracked symlinks instead of crashing", (t) => {
   const cwd = makeTempDir();
   initGitRepo(cwd);
   fs.writeFileSync(path.join(cwd, "app.js"), "console.log('v1');\n");
   run("git", ["add", "app.js"], { cwd });
   run("git", ["commit", "-m", "init"], { cwd });
-  fs.symlinkSync("missing-target", path.join(cwd, "broken-link"));
+  try {
+    fs.symlinkSync("missing-target", path.join(cwd, "broken-link"));
+  } catch (error) {
+    if (process.platform === "win32" && error?.code === "EPERM") {
+      t.skip("Windows requires Developer Mode or elevated privileges to create this symlink fixture.");
+      return;
+    }
+    throw error;
+  }
+
+  const untracked = run("git", ["ls-files", "--others", "--exclude-standard"], { cwd }).stdout;
+  if (!untracked.split(/\r?\n/).includes("broken-link")) {
+    t.skip("Git does not report broken symlinks as untracked in this environment.");
+    return;
+  }
 
   const target = resolveReviewTarget(cwd, {});
   const context = collectReviewContext(cwd, target);

tests/process.test.mjs

@@ -19,8 +19,9 @@ test("terminateProcessTree uses taskkill on Windows", () => {
         error: null
       };
     },
-    killImpl() {
-      throw new Error("kill fallback should not run");
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
     }
   });
 
@@ -32,7 +33,8 @@ test("terminateProcessTree uses taskkill on Windows", () => {
   assert.equal(outcome.method, "taskkill");
 });
 
-test("terminateProcessTree treats missing Windows processes as already stopped", () => {
+test("terminateProcessTree uses liveness instead of localized taskkill output", () => {
+  let livenessChecks = 0;
   const outcome = terminateProcessTree(1234, {
     platform: "win32",
     runCommandImpl(command, args) {
@@ -41,15 +43,202 @@ test("terminateProcessTree treats missing Windows processes as already stopped",
         args,
         status: 128,
         signal: null,
-        stdout: "ERROR: The process \"1234\" not found.",
-        stderr: "",
+        stdout: "",
+        stderr: "Erreur : le processus \"1234\" est introuvable.",
         error: null
       };
+    },
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
+      livenessChecks += 1;
+      if (livenessChecks === 1) {
+        return;
+      }
+      const error = new Error("ESRCH");
+      error.code = "ESRCH";
+      throw error;
     }
   });
 
   assert.equal(outcome.attempted, true);
+  assert.equal(outcome.delivered, true);
   assert.equal(outcome.method, "taskkill");
   assert.equal(outcome.result.status, 128);
-  assert.match(outcome.result.stdout, /not found/i);
+  assert.equal(livenessChecks, 2);
+});
+
+test("terminateProcessTree reports delivery when taskkill only failed on already-exiting descendants", () => {
+  let livenessChecks = 0;
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      return {
+        command,
+        args,
+        status: 128,
+        signal: null,
+        stdout: "SUCCESS: The process with PID 1234 has been terminated.",
+        stderr:
+          "ERROR: The process with PID 5678 (child process of PID 1234) could not be terminated.\n" +
+          "Reason: The operation attempted is not supported.",
+        error: null
+      };
+    },
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
+      livenessChecks += 1;
+      if (livenessChecks === 1) {
+        return;
+      }
+      const error = new Error("ESRCH");
+      error.code = "ESRCH";
+      throw error;
+    }
+  });
+
+  assert.equal(outcome.attempted, true);
+  assert.equal(outcome.delivered, true);
+  assert.equal(outcome.method, "taskkill");
+  assert.equal(outcome.result.status, 128);
+  assert.equal(livenessChecks, 2);
+});
+
+test("terminateProcessTree still throws when taskkill fails and the root process survives", () => {
+  assert.throws(
+    () =>
+      terminateProcessTree(1234, {
+        platform: "win32",
+        runCommandImpl(command, args) {
+          return {
+            command,
+            args,
+            status: 128,
+            signal: null,
+            stdout: "",
+            stderr: "ERROR: The process with PID 1234 could not be terminated.\nReason: Access is denied.",
+            error: null
+          };
+        },
+        killImpl(pid, signal) {
+          assert.equal(pid, 1234);
+          assert.equal(signal, 0);
+        }
+      }),
+    /could not be terminated/
+  );
+});
+
+test("terminateProcessTree skips taskkill when the Windows process is already absent", () => {
+  let taskkillCalled = false;
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      taskkillCalled = true;
+      return {
+        command,
+        args,
+        status: 128,
+        signal: null,
+        stdout: "",
+        stderr: "Erreur : le processus \"1234\" est introuvable.",
+        error: null
+      };
+    },
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
+      const error = new Error("ESRCH");
+      error.code = "ESRCH";
+      throw error;
+    }
+  });
+
+  assert.equal(taskkillCalled, false);
+  assert.deepEqual(outcome, {
+    attempted: false,
+    delivered: false,
+    method: null
+  });
+});
+
+test("terminateProcessTree does not treat a Windows preflight permission error as missing", () => {
+  let taskkillCalled = false;
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      taskkillCalled = true;
+      return {
+        command,
+        args,
+        status: 0,
+        signal: null,
+        stdout: "",
+        stderr: "",
+        error: null
+      };
+    },
+    killImpl(pid, signal) {
+      assert.equal(pid, 1234);
+      assert.equal(signal, 0);
+      const error = new Error("EPERM");
+      error.code = "EPERM";
+      throw error;
+    }
+  });
+
+  assert.equal(taskkillCalled, true);
+  assert.equal(outcome.delivered, true);
+  assert.equal(outcome.method, "taskkill");
+});
+
+test("terminateProcessTree preserves the Windows ENOENT fallback", () => {
+  const killCalls = [];
+  const outcome = terminateProcessTree(1234, {
+    platform: "win32",
+    runCommandImpl(command, args) {
+      const error = new Error("ENOENT");
+      error.code = "ENOENT";
+      return {
+        command,
+        args,
+        status: 0,
+        signal: null,
+        stdout: "",
+        stderr: "",
+        error
+      };
+    },
+    killImpl(pid, signal) {
+      killCalls.push([pid, signal]);
+    }
+  });
+
+  assert.deepEqual(killCalls, [
+    [1234, 0],
+    [1234, undefined]
+  ]);
+  assert.deepEqual(outcome, {
+    attempted: true,
+    delivered: true,
+    method: "kill"
+  });
+});
+
+test("terminateProcessTree leaves the non-Windows process-group path unchanged", () => {
+  const killCalls = [];
+  const outcome = terminateProcessTree(1234, {
+    platform: "linux",
+    killImpl(pid, signal) {
+      killCalls.push([pid, signal]);
+    }
+  });
+
+  assert.deepEqual(killCalls, [[-1234, "SIGTERM"]]);
+  assert.deepEqual(outcome, {
+    attempted: true,
+    delivered: true,
+    method: "process-group"
+  });
 });