From a0b36733f2f40cbfe6f9f04bbf4f5a7bf077956a Mon Sep 17 00:00:00 2001 From: Rohan Poudel <66029221+rohanpoudel2@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:50:03 -0600 Subject: [PATCH 1/2] fix(runtime): reclaim credential-home locks held by a reused PID recoverStaleCredentialHomeLock decided staleness from process.kill alone, so a reused PID was indistinguishable from the original owner and the lock was never reclaimed. The 30 s stale-age escape hatch only applied in the else branch of the PID check, which exempted a lock naming any live PID from expiry no matter how old it was. Acquisition then polled every 25 ms with no timeout. PR #448 fixed the malformed-PID half of this and left reused PIDs as separate work. Ownership is now proven by freshness rather than by PID liveness. A held lock refreshes its directory mtime on an unreferenced 5 s heartbeat, and one 30 s horizon now applies on every path, so a lock whose heartbeat has lapsed is reclaimed whoever owns that PID now. A definitively exited owner still recovers immediately. Because a suspended machine wakes with a stale-looking mtime while its owner is still alive, recovery of a lock whose PID looks alive waits one heartbeat interval and abandons the reclaim if the mtime advances. Owners that cannot be identified at all skip that wait, so exited and malformed owners recover as quickly as before. --- sdk/typescript/src/runtime.ts | 62 ++++-- sdk/typescript/tests-ts/runtime.test.ts | 266 ++++++++++++++++++++++-- 2 files changed, 292 insertions(+), 36 deletions(-) diff --git a/sdk/typescript/src/runtime.ts b/sdk/typescript/src/runtime.ts index 15063adf..2ce27a75 100644 --- a/sdk/typescript/src/runtime.ts +++ b/sdk/typescript/src/runtime.ts @@ -24,6 +24,7 @@ import { rm, rmdir, stat, + utimes, writeFile, } from "node:fs/promises"; import { homedir, tmpdir } from "node:os"; @@ -74,6 +75,7 @@ const MAX_ZIP_EXPANDED_SIZE = 512 * 1024 * 1024; const MODEL_UNSAFE_PATH = /[\u0000-\u001f\u007f-\u009f\u2028\u2029]/u; const CREDENTIAL_LOCK_NAME = ".codex-security-scan.lock"; const CREDENTIAL_LOGOUT_MARKER = ".codex-security-logged-out"; +const CREDENTIAL_LOCK_HEARTBEAT_MILLISECONDS = 5_000; const CREDENTIAL_LOCK_POLL_MILLISECONDS = 25; const INCOMPLETE_CREDENTIAL_LOCK_MILLISECONDS = 30_000; const MAX_PROCESS_ID = 2_147_483_647; @@ -1065,7 +1067,7 @@ export async function acquireCodexSecurityCredentialHomeLock( throw error; }); if (existingLock !== null) { - if (await recoverStaleCredentialHomeLock(lock)) continue; + if (await recoverStaleCredentialHomeLock(lock, signal)) continue; await delay(CREDENTIAL_LOCK_POLL_MILLISECONDS, undefined, { signal }); continue; } @@ -1078,7 +1080,7 @@ export async function acquireCodexSecurityCredentialHomeLock( await mkdir(lock, { mode: 0o700 }); } catch (error) { if (nodeErrorCode(error) !== "EEXIST") throw error; - if (await recoverStaleCredentialHomeLock(lock)) continue; + if (await recoverStaleCredentialHomeLock(lock, signal)) continue; await delay(CREDENTIAL_LOCK_POLL_MILLISECONDS, undefined, { signal }); continue; } @@ -1094,9 +1096,18 @@ export async function acquireCodexSecurityCredentialHomeLock( throw error; } + const heartbeat = setInterval(async () => { + try { + const now = new Date(); + await utimes(lock, now, now); + } catch {} + }, CREDENTIAL_LOCK_HEARTBEAT_MILLISECONDS); + heartbeat.unref(); + let released = false; return async () => { if (released) return; + clearInterval(heartbeat); await requireSecureCredentialHome(codexHome, { ...securityOptions, expectedDevice, @@ -1116,7 +1127,10 @@ export async function acquireCodexSecurityCredentialHomeLock( } } -async function recoverStaleCredentialHomeLock(lock: string): Promise { +async function recoverStaleCredentialHomeLock( + lock: string, + signal?: AbortSignal, +): Promise { const metadata = await lstat(lock).catch((error: unknown) => { if (nodeErrorCode(error) === "ENOENT") return null; throw error; @@ -1135,17 +1149,13 @@ async function recoverStaleCredentialHomeLock(lock: string): Promise { if (nodeErrorCode(error) !== "ENOENT" && !(error instanceof SyntaxError)) { throw error; } - if ( - Date.now() - metadata.mtimeMs < - INCOMPLETE_CREDENTIAL_LOCK_MILLISECONDS - ) { - return false; - } } // Only positive signed-32-bit PIDs identify an owner. Other values can name // process groups or fail argument validation, so use the stale-age check. const ownerPid = isRecord(owner) ? owner["pid"] : undefined; + let ownerExited = false; + let ownerIsAlive = false; if ( typeof ownerPid === "number" && Number.isInteger(ownerPid) && @@ -1154,20 +1164,38 @@ async function recoverStaleCredentialHomeLock(lock: string): Promise { ) { try { process.kill(ownerPid, 0); - return false; + ownerIsAlive = true; } catch (error) { - if (nodeErrorCode(error) !== "ESRCH") { - if (nodeErrorCode(error) === "EPERM") return false; - throw error; - } + if (nodeErrorCode(error) === "ESRCH") ownerExited = true; + else if (nodeErrorCode(error) === "EPERM") ownerIsAlive = true; + else throw error; } - } else if ( - Date.now() - metadata.mtimeMs < - INCOMPLETE_CREDENTIAL_LOCK_MILLISECONDS + } + if ( + !ownerExited && + Date.now() - metadata.mtimeMs < INCOMPLETE_CREDENTIAL_LOCK_MILLISECONDS ) { return false; } + if (ownerIsAlive) { + await delay(CREDENTIAL_LOCK_HEARTBEAT_MILLISECONDS, undefined, { signal }); + const refreshedMetadata = await lstat(lock).catch((error: unknown) => { + if (nodeErrorCode(error) === "ENOENT") return null; + throw error; + }); + if (refreshedMetadata === null) return true; + if ( + !refreshedMetadata.isDirectory() || + refreshedMetadata.isSymbolicLink() + ) { + throw new OutputDirectoryError( + `Codex Security credential-home lock is not a directory: ${lock}`, + ); + } + if (refreshedMetadata.mtimeMs > metadata.mtimeMs) return false; + } + const quarantine = `${lock}.stale-${randomUUID()}`; try { await rename(lock, quarantine); diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index 7dbf80c8..d252a804 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -29,6 +29,7 @@ import { sep, } from "node:path"; import { promisify } from "node:util"; +import * as timersPromises from "node:timers/promises"; import { brotliDecompressSync } from "node:zlib"; import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; import { strToU8, zipSync } from "fflate"; @@ -97,6 +98,55 @@ async function temporaryDirectory( return path; } +async function plantCredentialHomeLock( + home: string, + pid: number, + modifiedAt?: Date, +): Promise { + const lock = join(home, ".codex-security-scan.lock"); + await mkdir(lock, { mode: 0o700 }); + await writeFile( + join(lock, "owner.json"), + `${JSON.stringify({ pid, token: "planted-owner" })}\n`, + { mode: 0o600 }, + ); + if (modifiedAt !== undefined) await utimes(lock, modifiedAt, modifiedAt); + return lock; +} + +async function acquireCredentialHomeLockWithTimeout( + home: string, +): Promise<() => Promise> { + const controller = new AbortController(); + const timeout = setTimeout( + () => controller.abort(new DOMException("timed out", "AbortError")), + 10_000, + ); + timeout.unref(); + try { + return await acquireCodexSecurityCredentialHomeLock( + home, + controller.signal, + ); + } finally { + clearTimeout(timeout); + } +} + +function abortCredentialLockWaitWhenOwnerIsInspected( + controller: AbortController, +): ReturnType { + const killProcess = process.kill.bind(process); + return spyOn(process, "kill").mockImplementation((( + pid: number, + signal: number, + ) => { + const result = killProcess(pid, signal); + controller.abort(new DOMException("canceled", "AbortError")); + return result; + }) as typeof process.kill); +} + async function plugin(root: string, version = "1.2.3"): Promise { const path = join(root, "plugin"); await mkdir(join(path, ".codex-plugin"), { recursive: true }); @@ -2281,24 +2331,87 @@ describe("runtime directories and plugin Python boundary", () => { expect(existsSync(join(home, ".codex-security-scan.lock"))).toBe(false); }); - test("cancels a scan waiting for the persistent credential-home lock", async () => { + test("keeps a fresh live credential-home lock and cancels the waiter", async () => { const root = await temporaryDirectory(); const home = await prepareCodexSecurityCredentialHome({ CODEX_SECURITY_STATE_DIR: join(root, "state"), }); const release = await acquireCodexSecurityCredentialHomeLock(home); const controller = new AbortController(); - const waiting = acquireCodexSecurityCredentialHomeLock( - home, - controller.signal, - ); - controller.abort(new DOMException("canceled", "AbortError")); + const inspectOwner = + abortCredentialLockWaitWhenOwnerIsInspected(controller); try { + const waiting = acquireCodexSecurityCredentialHomeLock( + home, + controller.signal, + ); await expect(waiting).rejects.toMatchObject({ name: "AbortError" }); + expect(inspectOwner).toHaveBeenCalledWith(process.pid, 0); + expect(existsSync(join(home, ".codex-security-scan.lock"))).toBe(true); } finally { + inspectOwner.mockRestore(); await release(); } + expect(existsSync(join(home, ".codex-security-scan.lock"))).toBe(false); + }); + + test("heartbeats a held credential-home lock across the stale horizon", async () => { + const root = await temporaryDirectory(); + const home = await prepareCodexSecurityCredentialHome({ + CODEX_SECURITY_STATE_DIR: join(root, "state"), + }); + const lock = join(home, ".codex-security-scan.lock"); + let heartbeat: (() => Promise) | undefined; + const unref = mock(() => undefined); + const interval = { unref } as unknown as NodeJS.Timeout; + let cleared = false; + const setHeartbeat = spyOn(globalThis, "setInterval").mockImplementation((( + callback: () => Promise, + milliseconds: number, + ) => { + expect(milliseconds).toBe(5_000); + heartbeat = callback; + return interval; + }) as typeof setInterval); + const clearHeartbeat = spyOn( + globalThis, + "clearInterval", + ).mockImplementation(((timer: NodeJS.Timeout) => { + if (timer === interval) cleared = true; + }) as typeof clearInterval); + let release: (() => Promise) | undefined; + let inspectOwner: ReturnType | undefined; + + try { + release = await acquireCodexSecurityCredentialHomeLock(home); + expect(unref).toHaveBeenCalledTimes(1); + expect(heartbeat).toBeDefined(); + + const stale = new Date(Date.now() - 60_000); + await utimes(lock, stale, stale); + await heartbeat!(); + expect(Date.now() - (await stat(lock)).mtimeMs).toBeLessThan(30_000); + + const controller = new AbortController(); + inspectOwner = abortCredentialLockWaitWhenOwnerIsInspected(controller); + const waiting = acquireCodexSecurityCredentialHomeLock( + home, + controller.signal, + ); + await expect(waiting).rejects.toMatchObject({ name: "AbortError" }); + expect(existsSync(lock)).toBe(true); + } finally { + inspectOwner?.mockRestore(); + try { + if (release !== undefined) await release(); + } finally { + clearHeartbeat.mockRestore(); + setHeartbeat.mockRestore(); + } + } + expect(cleared).toBe(true); + expect(existsSync(lock)).toBe(false); }); test("does not rewrite Windows credential ACLs while polling a held lock", async () => { @@ -2363,6 +2476,131 @@ describe("runtime directories and plugin Python boundary", () => { expect(existsSync(lock)).toBe(false); }); + test("recovers a stale credential-home lock naming a live process", async () => { + const root = await temporaryDirectory(); + const home = await prepareCodexSecurityCredentialHome({ + CODEX_SECURITY_STATE_DIR: join(root, "state"), + }); + const stale = new Date(Date.now() - 10 * 60_000); + const lock = await plantCredentialHomeLock(home, process.pid, stale); + let release: (() => Promise) | undefined; + + try { + release = await acquireCredentialHomeLockWithTimeout(home); + expect(existsSync(lock)).toBe(true); + } finally { + if (release !== undefined) await release(); + } + expect(existsSync(lock)).toBe(false); + }); + + test("recovers a stale credential-home lock when PID inspection is denied", async () => { + const root = await temporaryDirectory(); + const home = await prepareCodexSecurityCredentialHome({ + CODEX_SECURITY_STATE_DIR: join(root, "state"), + }); + const stale = new Date(Date.now() - 10 * 60_000); + const lock = await plantCredentialHomeLock(home, process.pid, stale); + const inspectOwner = spyOn(process, "kill").mockImplementation((() => { + const error = new Error( + "operation not permitted", + ) as NodeJS.ErrnoException; + error.code = "EPERM"; + throw error; + }) as typeof process.kill); + let release: (() => Promise) | undefined; + + try { + release = await acquireCredentialHomeLockWithTimeout(home); + expect(existsSync(lock)).toBe(true); + } finally { + inspectOwner.mockRestore(); + if (release !== undefined) await release(); + } + expect(existsSync(lock)).toBe(false); + }); + + test("abandons stale recovery when the owner heartbeat advances", async () => { + if ( + runTestInSubprocess( + import.meta.path, + "abandons stale recovery when the owner heartbeat advances", + ) + ) { + return; + } + const root = await temporaryDirectory(); + const home = await prepareCodexSecurityCredentialHome({ + CODEX_SECURITY_STATE_DIR: join(root, "state"), + }); + const lock = join(home, ".codex-security-scan.lock"); + const controller = new AbortController(); + let heartbeat: (() => Promise) | undefined; + const interval = { + unref: mock(() => undefined), + } as unknown as NodeJS.Timeout; + const setHeartbeat = spyOn(globalThis, "setInterval").mockImplementation((( + callback: () => Promise, + milliseconds: number, + ) => { + expect(milliseconds).toBe(5_000); + if (heartbeat === undefined) heartbeat = callback; + else controller.abort(new DOMException("lock stolen", "AbortError")); + return interval; + }) as typeof setInterval); + const clearHeartbeat = spyOn( + globalThis, + "clearInterval", + ).mockImplementation((() => undefined) as typeof clearInterval); + const realDelay = timersPromises.setTimeout; + let waitedForHeartbeat = false; + mock.module("node:timers/promises", () => ({ + ...timersPromises, + setTimeout: (async ( + milliseconds: number, + value?: unknown, + options?: { signal?: AbortSignal }, + ) => { + expect(options?.signal).toBe(controller.signal); + if (milliseconds === 5_000) { + waitedForHeartbeat = true; + expect(heartbeat).toBeDefined(); + await heartbeat!(); + return value; + } + controller.abort(new DOMException("canceled", "AbortError")); + return await realDelay(0, value, { signal: controller.signal }); + }) as typeof timersPromises.setTimeout, + })); + let release: (() => Promise) | undefined; + + try { + release = await acquireCodexSecurityCredentialHomeLock(home); + const stale = new Date(Date.now() - 10 * 60_000); + await utimes(lock, stale, stale); + + const waiting = acquireCodexSecurityCredentialHomeLock( + home, + controller.signal, + ); + await expect(waiting).rejects.toMatchObject({ name: "AbortError" }); + expect(waitedForHeartbeat).toBe(true); + expect(Date.now() - (await stat(lock)).mtimeMs).toBeLessThan(30_000); + expect(existsSync(lock)).toBe(true); + } finally { + mock.module("node:timers/promises", () => ({ + ...timersPromises, + setTimeout: realDelay, + })); + try { + if (release !== undefined) await release(); + } finally { + clearHeartbeat.mockRestore(); + setHeartbeat.mockRestore(); + } + } + }); + test("recovers credential-home locks whose owner names no process", async () => { const root = await temporaryDirectory(); const home = await prepareCodexSecurityCredentialHome({ @@ -2379,19 +2617,9 @@ describe("runtime directories and plugin Python boundary", () => { const aged = new Date(Date.now() - 10 * 60_000); await utimes(lock, aged, aged); - // Bound acquisition so a false live-owner result cannot hang the test. - const abort = new AbortController(); - const timer = setTimeout(() => abort.abort(), 5_000); - try { - const release = await acquireCodexSecurityCredentialHomeLock( - home, - abort.signal, - ); - expect(existsSync(lock)).toBe(true); - await release(); - } finally { - clearTimeout(timer); - } + const release = await acquireCredentialHomeLockWithTimeout(home); + expect(existsSync(lock)).toBe(true); + await release(); expect(existsSync(lock)).toBe(false); } }); From f78b86e110d20726302c831ded0bc31720156d63 Mon Sep 17 00:00:00 2001 From: Rohan Poudel <66029221+rohanpoudel2@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:14:10 -0600 Subject: [PATCH 2/2] test(runtime): stop the heartbeat tests from intercepting foreign timers The credential-lock heartbeat tests replaced the global timer functions and asserted inside the mock bodies. Bun runs every test file in one process, so while those spies were installed any unrelated setInterval call received the mock: one test failed an assertion attributed to whichever test happened to be running, and the other aborted its own lock wait from the mock's else branch. Neither could be triggered by the code under test. Capture only the heartbeat registration and delegate every other timer call to the real implementation, then assert the interval, its unreferenced timer, and release-time clearing from the test body. The two near-identical mock setups become one helper. No production change and no change in what the tests cover. --- sdk/typescript/tests-ts/runtime.test.ts | 111 ++++++++++++++---------- 1 file changed, 65 insertions(+), 46 deletions(-) diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index d252a804..179db5b5 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -147,6 +147,55 @@ function abortCredentialLockWaitWhenOwnerIsInspected( }) as typeof process.kill); } +function captureCredentialLockHeartbeatTimer() { + const realSetInterval = globalThis.setInterval.bind(globalThis); + const realClearInterval = globalThis.clearInterval.bind(globalThis); + const unref = mock(() => undefined); + const interval = { unref } as unknown as NodeJS.Timeout; + let callback: (() => Promise) | undefined; + let milliseconds: number | undefined; + let cleared = false; + const setIntervalSpy = spyOn(globalThis, "setInterval").mockImplementation((( + candidate: (...args: unknown[]) => void, + delay?: number, + ...args: unknown[] + ) => { + if (delay === 5_000 && callback === undefined) { + callback = candidate as () => Promise; + milliseconds = delay; + return interval; + } + return realSetInterval(candidate, delay, ...args); + }) as typeof setInterval); + const clearIntervalSpy = spyOn( + globalThis, + "clearInterval", + ).mockImplementation(((timer: NodeJS.Timeout) => { + if (timer === interval) { + cleared = true; + return; + } + realClearInterval(timer); + }) as typeof clearInterval); + + return { + get callback() { + return callback; + }, + get cleared() { + return cleared; + }, + get milliseconds() { + return milliseconds; + }, + restore() { + clearIntervalSpy.mockRestore(); + setIntervalSpy.mockRestore(); + }, + unref, + }; +} + async function plugin(root: string, version = "1.2.3"): Promise { const path = join(root, "plugin"); await mkdir(join(path, ".codex-plugin"), { recursive: true }); @@ -2362,35 +2411,19 @@ describe("runtime directories and plugin Python boundary", () => { CODEX_SECURITY_STATE_DIR: join(root, "state"), }); const lock = join(home, ".codex-security-scan.lock"); - let heartbeat: (() => Promise) | undefined; - const unref = mock(() => undefined); - const interval = { unref } as unknown as NodeJS.Timeout; - let cleared = false; - const setHeartbeat = spyOn(globalThis, "setInterval").mockImplementation((( - callback: () => Promise, - milliseconds: number, - ) => { - expect(milliseconds).toBe(5_000); - heartbeat = callback; - return interval; - }) as typeof setInterval); - const clearHeartbeat = spyOn( - globalThis, - "clearInterval", - ).mockImplementation(((timer: NodeJS.Timeout) => { - if (timer === interval) cleared = true; - }) as typeof clearInterval); + const heartbeatTimer = captureCredentialLockHeartbeatTimer(); let release: (() => Promise) | undefined; let inspectOwner: ReturnType | undefined; try { release = await acquireCodexSecurityCredentialHomeLock(home); - expect(unref).toHaveBeenCalledTimes(1); - expect(heartbeat).toBeDefined(); + expect(heartbeatTimer.milliseconds).toBe(5_000); + expect(heartbeatTimer.unref).toHaveBeenCalledTimes(1); + expect(heartbeatTimer.callback).toBeDefined(); const stale = new Date(Date.now() - 60_000); await utimes(lock, stale, stale); - await heartbeat!(); + await heartbeatTimer.callback!(); expect(Date.now() - (await stat(lock)).mtimeMs).toBeLessThan(30_000); const controller = new AbortController(); @@ -2406,11 +2439,10 @@ describe("runtime directories and plugin Python boundary", () => { try { if (release !== undefined) await release(); } finally { - clearHeartbeat.mockRestore(); - setHeartbeat.mockRestore(); + heartbeatTimer.restore(); } } - expect(cleared).toBe(true); + expect(heartbeatTimer.cleared).toBe(true); expect(existsSync(lock)).toBe(false); }); @@ -2535,25 +2567,10 @@ describe("runtime directories and plugin Python boundary", () => { }); const lock = join(home, ".codex-security-scan.lock"); const controller = new AbortController(); - let heartbeat: (() => Promise) | undefined; - const interval = { - unref: mock(() => undefined), - } as unknown as NodeJS.Timeout; - const setHeartbeat = spyOn(globalThis, "setInterval").mockImplementation((( - callback: () => Promise, - milliseconds: number, - ) => { - expect(milliseconds).toBe(5_000); - if (heartbeat === undefined) heartbeat = callback; - else controller.abort(new DOMException("lock stolen", "AbortError")); - return interval; - }) as typeof setInterval); - const clearHeartbeat = spyOn( - globalThis, - "clearInterval", - ).mockImplementation((() => undefined) as typeof clearInterval); + const heartbeatTimer = captureCredentialLockHeartbeatTimer(); const realDelay = timersPromises.setTimeout; let waitedForHeartbeat = false; + let observedDelaySignal: AbortSignal | undefined; mock.module("node:timers/promises", () => ({ ...timersPromises, setTimeout: (async ( @@ -2561,11 +2578,10 @@ describe("runtime directories and plugin Python boundary", () => { value?: unknown, options?: { signal?: AbortSignal }, ) => { - expect(options?.signal).toBe(controller.signal); + observedDelaySignal = options?.signal; if (milliseconds === 5_000) { waitedForHeartbeat = true; - expect(heartbeat).toBeDefined(); - await heartbeat!(); + await heartbeatTimer.callback!(); return value; } controller.abort(new DOMException("canceled", "AbortError")); @@ -2576,6 +2592,9 @@ describe("runtime directories and plugin Python boundary", () => { try { release = await acquireCodexSecurityCredentialHomeLock(home); + expect(heartbeatTimer.milliseconds).toBe(5_000); + expect(heartbeatTimer.unref).toHaveBeenCalledTimes(1); + expect(heartbeatTimer.callback).toBeDefined(); const stale = new Date(Date.now() - 10 * 60_000); await utimes(lock, stale, stale); @@ -2585,6 +2604,7 @@ describe("runtime directories and plugin Python boundary", () => { ); await expect(waiting).rejects.toMatchObject({ name: "AbortError" }); expect(waitedForHeartbeat).toBe(true); + expect(observedDelaySignal).toBe(controller.signal); expect(Date.now() - (await stat(lock)).mtimeMs).toBeLessThan(30_000); expect(existsSync(lock)).toBe(true); } finally { @@ -2595,8 +2615,7 @@ describe("runtime directories and plugin Python boundary", () => { try { if (release !== undefined) await release(); } finally { - clearHeartbeat.mockRestore(); - setHeartbeat.mockRestore(); + heartbeatTimer.restore(); } } });