From eabe795bf7882610d11b7c6bc30c7bdf9d1ca8f9 Mon Sep 17 00:00:00 2001 From: Som Samantray Date: Sat, 15 Aug 2026 21:24:34 +0530 Subject: [PATCH 1/3] fix: tolerate EPERM when unlinking broker teardown files (#633) --- .../codex/scripts/lib/broker-lifecycle.mjs | 18 ++- tests/broker-lifecycle.test.mjs | 151 ++++++++++++++++++ 2 files changed, 166 insertions(+), 3 deletions(-) create mode 100644 tests/broker-lifecycle.test.mjs diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819..7a8b8f6c 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -95,7 +95,11 @@ export function saveBrokerSession(cwd, session) { export function clearBrokerSession(cwd) { const stateFile = resolveBrokerStateFile(cwd); if (fs.existsSync(stateFile)) { - fs.unlinkSync(stateFile); + try { + fs.unlinkSync(stateFile); + } catch { + // Ignore locked or already-removed broker state files during cleanup. + } } } @@ -180,11 +184,19 @@ export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessi } if (pidFile && fs.existsSync(pidFile)) { - fs.unlinkSync(pidFile); + try { + fs.unlinkSync(pidFile); + } catch { + // Ignore locked or already-removed broker pid files during teardown. + } } if (logFile && fs.existsSync(logFile)) { - fs.unlinkSync(logFile); + try { + fs.unlinkSync(logFile); + } catch { + // Ignore locked or already-removed broker log files during teardown. + } } if (endpoint) { diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs new file mode 100644 index 00000000..246a627c --- /dev/null +++ b/tests/broker-lifecycle.test.mjs @@ -0,0 +1,151 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { + clearBrokerSession, + saveBrokerSession, + teardownBrokerSession +} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; +import { createBrokerEndpoint, parseBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs"; +import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs"; + +function makeTempDir() { + return fs.mkdtempSync(path.join(os.tmpdir(), "codex-plugin-broker-test-")); +} + +function epermError() { + const error = new Error("EPERM: operation not permitted, unlink"); + error.code = "EPERM"; + return error; +} + +function throwEpermFor(target, originalUnlinkSync) { + return (filePath) => { + if (filePath === target) { + throw epermError(); + } + return originalUnlinkSync(filePath); + }; +} + +test("teardownBrokerSession removes pid and log files on the happy path", (t) => { + const sessionDir = makeTempDir(); + t.after(() => fs.rmSync(sessionDir, { recursive: true, force: true })); + const pidFile = path.join(sessionDir, "broker.pid"); + const logFile = path.join(sessionDir, "broker.log"); + fs.writeFileSync(pidFile, "1234\n", "utf8"); + fs.writeFileSync(logFile, "log line\n", "utf8"); + + teardownBrokerSession({ pidFile, logFile, sessionDir }); + + assert.equal(fs.existsSync(pidFile), false); + assert.equal(fs.existsSync(logFile), false); +}); + +test("teardownBrokerSession tolerates EPERM on the pid file and still removes the log file", (t) => { + const sessionDir = makeTempDir(); + t.after(() => fs.rmSync(sessionDir, { recursive: true, force: true })); + const pidFile = path.join(sessionDir, "broker.pid"); + const logFile = path.join(sessionDir, "broker.log"); + fs.writeFileSync(pidFile, "1234\n", "utf8"); + fs.writeFileSync(logFile, "log line\n", "utf8"); + + const originalUnlinkSync = fs.unlinkSync; + fs.unlinkSync = throwEpermFor(pidFile, originalUnlinkSync); + t.after(() => { + fs.unlinkSync = originalUnlinkSync; + }); + + teardownBrokerSession({ pidFile, logFile, sessionDir }); + + assert.equal(fs.existsSync(pidFile), true); + assert.equal(fs.existsSync(logFile), false); +}); + +test("teardownBrokerSession tolerates EPERM on the log file and completes teardown", (t) => { + const sessionDir = makeTempDir(); + t.after(() => fs.rmSync(sessionDir, { recursive: true, force: true })); + const pidFile = path.join(sessionDir, "broker.pid"); + const logFile = path.join(sessionDir, "broker.log"); + fs.writeFileSync(pidFile, "1234\n", "utf8"); + fs.writeFileSync(logFile, "log line\n", "utf8"); + + const originalUnlinkSync = fs.unlinkSync; + fs.unlinkSync = throwEpermFor(logFile, originalUnlinkSync); + t.after(() => { + fs.unlinkSync = originalUnlinkSync; + }); + + teardownBrokerSession({ pidFile, logFile, sessionDir }); + + assert.equal(fs.existsSync(pidFile), false); + assert.equal(fs.existsSync(logFile), true); +}); + +test("teardownBrokerSession is a no-op for missing pid and log files", (t) => { + const sessionDir = makeTempDir(); + t.after(() => fs.rmSync(sessionDir, { recursive: true, force: true })); + const pidFile = path.join(sessionDir, "broker.pid"); + const logFile = path.join(sessionDir, "broker.log"); + + teardownBrokerSession({ pidFile, logFile, sessionDir }); + + assert.equal(fs.existsSync(pidFile), false); + assert.equal(fs.existsSync(logFile), false); +}); + +test("teardownBrokerSession runs remaining cleanup when the pid unlink fails", (t) => { + const sessionDir = makeTempDir(); + t.after(() => fs.rmSync(sessionDir, { recursive: true, force: true })); + const pidFile = path.join(sessionDir, "broker.pid"); + const logFile = path.join(sessionDir, "broker.log"); + const endpoint = createBrokerEndpoint(sessionDir, process.platform); + const endpointTarget = parseBrokerEndpoint(endpoint); + fs.writeFileSync(pidFile, "1234\n", "utf8"); + fs.writeFileSync(logFile, "log line\n", "utf8"); + if (endpointTarget.kind === "unix") { + fs.writeFileSync(endpointTarget.path, "", "utf8"); + } + + const originalUnlinkSync = fs.unlinkSync; + fs.unlinkSync = throwEpermFor(pidFile, originalUnlinkSync); + t.after(() => { + fs.unlinkSync = originalUnlinkSync; + }); + + teardownBrokerSession({ endpoint, pidFile, logFile, sessionDir }); + + assert.equal(fs.existsSync(logFile), false); + if (endpointTarget.kind === "unix") { + assert.equal(fs.existsSync(endpointTarget.path), false); + } +}); + +test("clearBrokerSession tolerates EPERM on the state file and does not throw", (t) => { + const dataDir = makeTempDir(); + const cwd = makeTempDir(); + t.after(() => fs.rmSync(dataDir, { recursive: true, force: true })); + t.after(() => fs.rmSync(cwd, { recursive: true, force: true })); + process.env.CLAUDE_PLUGIN_DATA = dataDir; + t.after(() => { + delete process.env.CLAUDE_PLUGIN_DATA; + }); + + saveBrokerSession(cwd, { endpoint: "pipe://test", pid: 1234 }); + const stateFile = path.join(resolveStateDir(cwd), "broker.json"); + assert.equal(fs.existsSync(stateFile), true); + + const originalUnlinkSync = fs.unlinkSync; + fs.unlinkSync = throwEpermFor(stateFile, originalUnlinkSync); + t.after(() => { + fs.unlinkSync = originalUnlinkSync; + }); + + clearBrokerSession(cwd); + + fs.unlinkSync = originalUnlinkSync; + assert.doesNotThrow(() => saveBrokerSession(cwd, { endpoint: "pipe://test", pid: 4321 })); +}); From a53957f80c6759ac8b55fa22246a554376356b55 Mon Sep 17 00:00:00 2001 From: Som Samantray Date: Sat, 15 Aug 2026 21:24:39 +0530 Subject: [PATCH 2/3] docs: add plan for broker teardown EPERM guard --- ...8-15-001-fix-broker-teardown-eperm-plan.md | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 docs/plans/2026-08-15-001-fix-broker-teardown-eperm-plan.md diff --git a/docs/plans/2026-08-15-001-fix-broker-teardown-eperm-plan.md b/docs/plans/2026-08-15-001-fix-broker-teardown-eperm-plan.md new file mode 100644 index 00000000..975b4cdd --- /dev/null +++ b/docs/plans/2026-08-15-001-fix-broker-teardown-eperm-plan.md @@ -0,0 +1,100 @@ +--- +title: Guard broker teardown unlinks against EPERM - Plan +type: fix +date: 2026-08-15 +artifact_contract: ce-unified-plan/v1 +artifact_readiness: implementation-ready +product_contract_source: ce-plan-bootstrap +execution: code +--- + +# Guard broker teardown unlinks against EPERM - Plan + +## Goal Capsule + +- **Objective:** Make `teardownBrokerSession` immune to `unlinkSync` failures on the pid and log files, so an EPERM on Windows cannot fail the caller's job. +- **Authority:** Fixes openai/codex-plugin-cc issue #633 (duplicate #626). Scope is that issue plus the adjacent unguarded unlink on the same propagation path; PR must not include the Compound Engineering badge. +- **Execution profile:** Single implementation unit, one commit, mirrored from the three existing guards in the same function. +- **Stop conditions:** Unit tests cover the EPERM paths; full `npm test` and `npm run build` pass; job records no longer flip to `failed` when broker teardown hits a locked file. +- **Tail ownership:** `ce-work` owns implementation; commit/PR per repo conventions. + +## Product Contract + +### Summary + +`teardownBrokerSession` in `plugins/codex/scripts/lib/broker-lifecycle.mjs` performs five cleanup steps. Three are wrapped in bare `try { ... } catch { ... }` guards that tolerate failure. The pid-file and log-file `unlinkSync` calls are guarded only by `fs.existsSync`, so a failed unlink throws. On Windows, deleting a file another process holds open fails with `EPERM`, which propagates out of `ensureBrokerSession` (stale-broker replacement path) through `CodexAppServerClient.connect` into `withAppServer`, and the caller's job is marked `failed` before any Codex work begins. The fix wraps the two unlinks in the same guard the other three steps already use, and applies the same guard to the broker state-file unlink in `clearBrokerSession`, which runs immediately after on the same propagation path. + +### Problem Frame + +Windows file deletion is advisory on locked files: `unlinkSync` throws `EPERM` when another process holds the file open. Broker pid and log files are exactly the files another process may hold. Today a cleanup that is meant to be best-effort becomes the root cause of a job failure with an error unrelated to the user's request. + +### Requirements + +- R1. `teardownBrokerSession` must not throw when unlinking the pid file fails, including on `EPERM`. +- R2. `teardownBrokerSession` must not throw when unlinking the log file fails, including on `EPERM`. +- R3. A failed pid or log unlink must not prevent the remaining cleanup steps from running. +- R4. Success-path behavior is unchanged: existing pid and log files are still removed when removal succeeds. +- R5. `clearBrokerSession` must not throw when unlinking the broker state file fails, including on `EPERM`. + +### Scope Boundaries + +- In scope: the two unguarded unlinks inside `teardownBrokerSession`, the unguarded state-file unlink in `clearBrokerSession` (same unguarded pattern, one call later on the same propagation path), and a unit test proving the guards. +- The PR will close both #633 and its duplicate #626. +- Deferred to follow-up work: sibling unguarded unlinks of the same EPERM class outside the broker lifecycle — `removeFileIfExists`/`removeJobFile` in `plugins/codex/scripts/lib/state.mjs` and the `shutdown()` unlinks in `plugins/codex/scripts/app-server-broker.mjs`. + +## Planning Contract + +### Key Technical Decisions + +- KTD1. Mirror the existing guard pattern: each unlink becomes `try { fs.unlinkSync(...) } catch { // Ignore ... }` with a comment naming what is tolerated, matching the three guarded steps in the same function and the same shape used by `terminateProcessTree` for platform-tolerant cleanup. Chosen over introducing a shared `safeUnlink` helper or a `removeFileIfExists` wrapper: the repo already owns this exact pattern in the same function, and a new helper would be a wider change with no additional benefit. +- KTD2. The fix lives inside `teardownBrokerSession` and `clearBrokerSession`, not at call sites. Chosen over guarding the call sites (`ensureBrokerSession` stale-broker path, `ensureBrokerSession` spawn-failure path, `session-lifecycle-hook.mjs` `handleSessionEnd`): teardown is best-effort by contract, and one guard in each function covers every caller, including the hook path the issue does not enumerate. +- KTD3. Test by patching `fs.unlinkSync` directly in a new unit test file. Chosen over adding a mock framework or a dependency-injection seam: the repo has no mock framework (`node:test` + `assert/strict` only), tests within a file run serially, and `node --test` isolates test files in separate processes, so a temporary global patch of `fs.unlinkSync` is safe. + +### Assumptions + +- A failed unlink for any reason (not only `EPERM`) is safe to tolerate: the unlink is best-effort cleanup, and the existing guards already ignore all error codes. +- Keeping the bare `catch {}` (no error binding) matches repo style; no logging is added, consistent with the three existing guards. +- The unit test runs on all platforms including CI (Linux): the EPERM is simulated by the fs patch, so the test is platform-independent. + +## Implementation Units + +### U1. Guard the broker teardown unlinks against EPERM + +- **Goal:** Wrap the pid-file and log-file `unlinkSync` calls in `teardownBrokerSession`, plus the state-file unlink in `clearBrokerSession`, in the same bare `try { ... } catch { // Ignore ... }` guard the other cleanup steps already use, and prove the EPERM path with a unit test. +- **Requirements:** R1, R2, R3, R4, R5 +- **Dependencies:** none +- **Files:** + - Modify `plugins/codex/scripts/lib/broker-lifecycle.mjs` (pid/log unlink blocks in `teardownBrokerSession` and the state-file unlink in `clearBrokerSession`, currently guarded only by `fs.existsSync`) + - Create `tests/broker-lifecycle.test.mjs` (new unit test file) +- **Approach:** + 1. In `teardownBrokerSession`, wrap the pid-file unlink in `try { ... } catch { // Ignore locked or already-removed broker pid files during teardown. }`. + 2. Wrap the log-file unlink the same way, with a matching comment. + 3. In `clearBrokerSession`, wrap the state-file unlink in the same guard, with a matching comment. + 4. Keep the `fs.existsSync` pre-checks unchanged. + 5. Add the unit test: patch `fs.unlinkSync` per test case (path-targeted when a scenario needs only one file to fail), restore it in `t.after`. +- **Patterns to follow:** the three existing guarded steps in `teardownBrokerSession` (`killProcess`, endpoint socket unlink, `rmdirSync`); test style of `tests/process.test.mjs` (`node:test`, `assert/strict`), with the `t.after` cleanup pattern from `tests/runtime.test.mjs`. +- **Test scenarios:** + 1. Happy path: temp pid and log files exist; teardown removes both and does not throw. + 2. Error path: `fs.unlinkSync` throws `EPERM` for the pid file only (path-targeted patch); teardown completes without throwing and the log file is still removed. + 3. Error path: `fs.unlinkSync` throws `EPERM` for the log file only; teardown completes without throwing; the session dir may survive (its removal tolerates `ENOTEMPTY`). + 4. Edge case: pid and log paths do not exist; teardown attempts no unlink and does not throw. + 5. Integration: with the pid-file unlink failing, the endpoint-socket unlink and session-dir removal steps still run (remaining cleanup is not skipped). + 6. Error path: `clearBrokerSession` hits `EPERM` on the state file; it completes without throwing and the broker record is still cleared. +- **Verification:** the new test file passes with `npm test`; `npm run build` (loose checkJs typecheck) is clean. + +## Verification Contract + +| Check | Command | Applies to | Done signal | +|---|---|---|---| +| Unit suite | `npm test` (`node --test tests/*.test.mjs`) | U1 | All tests pass, including the new `tests/broker-lifecycle.test.mjs` cases; the EPERM scenarios fail before the guard change and pass after | +| Typecheck | `npm run build` | U1 | Clean exit; this is a loose checkJs pass (`strict: false`) that adds no meaningful type-error surface for this change, so the unit test is the authoritative gate; note the `prebuild` step requires the `codex` CLI on PATH | +| CI parity | `.github/workflows/pull-request-ci.yml` runs `npm test` + `npm run build` on ubuntu-latest | whole change | Same commands pass locally | + +## Definition of Done + +- The pid and log unlinks in `teardownBrokerSession` and the state-file unlink in `clearBrokerSession` are wrapped in bare catch guards with `// Ignore ...` comments. +- `tests/broker-lifecycle.test.mjs` covers happy path, both EPERM paths, the state-file EPERM path, missing-file edge case, and remaining-cleanup integration; red before the change, green after. +- `npm test` and `npm run build` pass. +- The EPERM failure mode is confirmed by simulation (platform-independent test); a manual Windows re-run of the issue's repro is recommended to confirm the reported symptom is gone, but is not blocking. +- No dead-end or experimental code remains in the diff; the change is one focused unit. +- PR body follows repo PR guidelines (no template exists; commit style is `imperative summary (#NNN)`), references #633 (and #626), contains no Compound Engineering badge, and includes security/agent disclosure as required by the target repo's expectations. From f52ef366a28a4648216c3d18f6c6e7e69e4edb38 Mon Sep 17 00:00:00 2001 From: Som Samantray Date: Sat, 15 Aug 2026 21:33:21 +0530 Subject: [PATCH 3/3] fix(review): assert state file survives EPERM in clear test --- tests/broker-lifecycle.test.mjs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index 246a627c..f6f5b4a7 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/tests/broker-lifecycle.test.mjs @@ -146,6 +146,7 @@ test("clearBrokerSession tolerates EPERM on the state file and does not throw", clearBrokerSession(cwd); + assert.equal(fs.existsSync(stateFile), true); + fs.unlinkSync = originalUnlinkSync; - assert.doesNotThrow(() => saveBrokerSession(cwd, { endpoint: "pipe://test", pid: 4321 })); });