teardownBrokerSession: unguarded pid/log unlinkSync throws EPERM on Windows and fails the whole job - the other four cleanup steps in the same function are already guarded

Version: plugin 1.0.6 (main @ db52e28, files verified unchanged at HEAD) · codex-cli 0.146.0 · Windows 11 Pro 26200 · Node 24.

Summary. teardownBrokerSession performs six cleanup steps. Four are wrapped so that a failure cannot escape; the pid-file and log-file unlinkSync calls are not. On Windows, deleting a file that another process still holds open — or that another process is deleting concurrently — fails with EPERM (delete-pending surfaces as access-denied, not ENOENT). Because ensureBrokerSession calls this function on its stale-broker replacement path with no try/catch, that EPERM propagates out of ensureBrokerSession → CodexAppServerClient.connect → withAppServer, where the direct-spawn fallback declines to handle it, and the caller's job is marked failed. The result is a background task that dies during broker setup, before any Codex work begins, with an error that has nothing to do with the user's request.

Observed. A background task job record, verbatim (user path redacted):

{
  "id": "task-msmupx6o-ab2fp3",
  "status": "failed",
  "errorMessage": "EPERM: operation not permitted, unlink 'C:\\Users\\<user>\\AppData\\Local\\Temp\\cxc-1GUA6t\\broker.pid'"
}

Lifetime 2.16 s — it never reached a turn. It was one of a batch of concurrent task --background dispatches against a single workspace; a sibling launched in the same millisecond completed normally.

cxc-1GUA6t is a broker session dir from createBrokerSessionDir(), and broker.pid is the pidFile built at broker-lifecycle.mjs:134. Only two sites in the plugin unlink that path: app-server-broker.mjs:111-112 and broker-lifecycle.mjs:182-183. The first is inside shutdown(server), which is declared and called only within app-server-broker.mjs (:162, :237, :242) and reachable nowhere else — the file is never imported, only spawned as a script by spawnBrokerProcess — so shutdown runs exclusively in the broker process and cannot write a task job's errorMessage. Since the message is recorded in the job's error, it was thrown in the job's process, i.e. at broker-lifecycle.mjs:183.

Where. plugins/codex/scripts/lib/broker-lifecycle.mjs at HEAD — note the guarded/unguarded asymmetry:

export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessionDir = null, pid = null, killProcess = null }) {
  if (Number.isFinite(pid) && killProcess) {
    try { killProcess(pid); } catch { /* … */ }                     // 174-180  GUARDED
  }
  if (pidFile && fs.existsSync(pidFile)) { fs.unlinkSync(pidFile); } // 182-184  UNGUARDED
  if (logFile && fs.existsSync(logFile)) { fs.unlinkSync(logFile); } // 186-188  UNGUARDED
  if (endpoint) { try { /* … */ } catch { /* … */ } }               // 190-199  GUARDED
  if (resolvedSessionDir && fs.existsSync(resolvedSessionDir)) {
    try { fs.rmdirSync(resolvedSessionDir); } catch { /* … */ }      // 202-208  GUARDED
  }
}

(The two unguarded steps also use existsSync-then-unlinkSync. That check-then-act gap is a separate correctness wart — it yields ENOENT, not the EPERM observed here — so it is worth removing but is not the mechanism of this bug.)

Why it reaches the user as a failed job.

1. ensureBrokerSession (:113-171) is an unsynchronized load → probe → teardown → spawn → save. When the recorded broker fails its 150 ms readiness probe (isBrokerEndpointReady, :107), every concurrent caller enters the if (existing) branch at :119-129 and calls teardownBrokerSession on the same pidFile. This call is not wrapped.
2. CodexAppServerClient.connect (lib/app-server.mjs:344) awaits ensureBrokerSession, so the throw happens before a client object exists.
3. withAppServer (lib/codex.mjs:621-632) cannot recover it:

   const brokerRequested = client?.transport === "broker" || Boolean(process.env[BROKER_ENDPOINT_ENV]);
   const shouldRetryDirect =
     (client?.transport === "broker" && error?.rpcCode === BROKER_BUSY_RPC_CODE) ||
     (brokerRequested && (error?.code === "ENOENT" || error?.code === "ECONNREFUSED"));
   if (!shouldRetryDirect) { throw error; }
   

   client is still null and BROKER_ENDPOINT_ENV is unset, so brokerRequested is false and the EPERM is rethrown. runAppServerTurn routes every task through withAppServer, so runTrackedJob catches it and stamps the job failed.

The readiness-timeout path at :150-160 calls the same unguarded teardown and has the same exposure.

Suggested fix. Make the two unlinks match the four cleanup steps around them: best-effort, never able to fail the caller. fs.rmSync(path, { force: true }) also removes the existsSync check-then-act gap by ignoring ENOENT outright, leaving only the Windows EPERM to the catch.

--- a/plugins/codex/scripts/lib/broker-lifecycle.mjs
+++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs
@@
+function removeBrokerFile(filePath) {
+  if (!filePath) {
+    return;
+  }
+  try {
+    fs.rmSync(filePath, { force: true });
+  } catch {
+    // A concurrent teardown may already be deleting this path, and on Windows a still-open
+    // handle surfaces as EPERM rather than ENOENT. Cleanup must never fail its caller:
+    // ensureBrokerSession calls teardown unguarded, so a throw here fails the whole job.
+  }
+}
+
 export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessionDir = null, pid = null, killProcess = null }) {
   if (Number.isFinite(pid) && killProcess) {
     try {
       killProcess(pid);
     } catch {
       // Ignore missing or already-exited broker processes.
     }
   }
 
-  if (pidFile && fs.existsSync(pidFile)) {
-    fs.unlinkSync(pidFile);
-  }
-
-  if (logFile && fs.existsSync(logFile)) {
-    fs.unlinkSync(logFile);
-  }
+  removeBrokerFile(pidFile);
+  removeBrokerFile(logFile);
 
   if (endpoint) {

Test. In tests/broker-lifecycle.test.mjs: stub fs.rmSync to throw an EPERM for the pid path and assert (a) teardownBrokerSession does not throw, and (b) it still proceeds to the endpoint and session-dir cleanup — the unguarded version aborts before both.

Scope note. This does not fix the underlying unsynchronized ensureBrokerSession — already reported as race 3 of #286, whose observable consequence there is leaked brokers (I see 92 leaked cxc-* session dirs against one live broker, consistent with it). This issue is the separate, smaller point that the racing teardown can also throw, and that nothing between it and the job record absorbs it. Fixing only #286 would remove the trigger; guarding the unlinks removes the fatality, and is eight lines. Also related but distinct: #566 (teardown deletes the pid file without killing the process), #402 (shouldRetryDirect misses a clean connection close), #404 (readiness timeouts, which govern how often the teardown branch is taken).

Start in plugins/codex/scripts/lib/broker-lifecycle.mjs and inspect teardownBrokerSession's pid-file and log-file cleanup alongside the guarded steps. Run tests/broker-lifecycle.test.mjs, covering an EPERM from the pid path and confirming endpoint and session-directory cleanup still proceeds; done means teardown no longer throws during that case.