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:

{
  "id": "task-msmupx6o-ab2fp3",
  "status": "failed",
  "errorMessage": "EPERM: operation not permitted, unlink 'C:\\Users\\LZong\\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; its siblings launched in the same millisecond
(task-msmupx6o-wz89pw) 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:

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. */ }   // 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 { /* … */ fs.unlinkSync(target.path); } catch { /* Ignore … already-removed … */ }          // 190-199  GUARDED
  }

  const resolvedSessionDir = /* … */;
  if (resolvedSessionDir && fs.existsSync(resolvedSessionDir)) {
    try { fs.rmdirSync(resolvedSessionDir); } catch { /* Ignore non-empty or missing directories. */ } // 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 (broker-lifecycle.mjs: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(cwd, …), 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 TOCTOU 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.unlinkSync/fs.rmSync to throw an EPERM
error 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. A/B
against the unfixed tree: unfixed throws, fixed does not.

Scope note. This does not fix the underlying unsynchronized ensureBrokerSession — that is
already reported as race 3 of #286, whose observable consequence there is leaked brokers (we see
92 leaked cxc-* session dirs against one live broker on this machine, 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 is willing to absorb 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 — same function, different error class), #404 (readiness timeouts, which govern how
often the teardown branch is taken).

Start in plugins/codex/scripts/lib/broker-lifecycle.mjs, especially teardownBrokerSession and its callers in ensureBrokerSession. Then run tests/broker-lifecycle.test.mjs and add coverage for an EPERM while removing the pid file, confirming endpoint and session-directory cleanup still proceeds without throwing.