From a70c3d274a963fe7fd24d15dd1d78a954c62a3ac Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Mon, 14 Sep 2026 10:59:09 +0900 Subject: [PATCH 1/7] fix(codex): scope refresh lock acquisition and release to file identity Two windows let one Codex credential refresh delete another live refresh lock. isRefreshLockStale treated any unreadable lock as stale. The owner creates the file with openSync(path, "wx") and writes its metadata immediately after, so a live lock is briefly empty; a waiter that looked during that window deleted the lock and ran a second concurrent refresh against the same grant. The unreadable case now ages the file itself and only reports stale past the same 60s window, and a lock that has already disappeared reports not stale so the waiter simply retries the create. The release path unlinked by name. If a waiter had reclaimed the path and a second owner recreated it, the first owner deleted the second owner's live lock on its way out. Release now compares the fd identity captured before close against the current path and unlinks only its own file, falling back to the previous behavior when the identity cannot be read. Both cases are pinned in tests/codex-integration/codex-account-store.test.ts and both fail before this change. --- src/codex/account-store.ts | 27 +++++++++-- structure/catalog.md | 2 + structure/providers/openai-tiers.md | 4 ++ .../codex-account-store.test.ts | 46 ++++++++++++++++++- 4 files changed, 74 insertions(+), 5 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 4b151707a1..721288850b 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -1,5 +1,5 @@ import { createHash } from "node:crypto"; -import { closeSync, existsSync, readFileSync, mkdirSync, openSync, unlinkSync, writeFileSync } from "node:fs"; +import { closeSync, existsSync, fstatSync, readFileSync, mkdirSync, openSync, statSync, unlinkSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { ConfigMutationLockError, @@ -612,7 +612,14 @@ function isRefreshLockStale(path: string): boolean { const parsed = JSON.parse(readFileSync(path, "utf-8")) as { acquiredAt?: unknown }; return typeof parsed.acquiredAt !== "number" || Date.now() - parsed.acquiredAt > REFRESH_LOCK_STALE_MS; } catch { - return true; + // The owner creates the file and writes its metadata in two steps, so a live lock is + // briefly unreadable. Age the file itself instead of calling that window stale, which + // let a waiter delete a lock whose owner was still inside its critical section. + try { + return Date.now() - statSync(path).mtimeMs > REFRESH_LOCK_STALE_MS; + } catch { + return false; + } } } @@ -648,9 +655,21 @@ export async function withCodexRefreshFileLock(lockKey: string, signal: Abort try { return await fn(); } finally { - if (fd != null) closeSync(fd); + // Release only the lock this call created. If a waiter reclaimed the path as stale and a + // new owner recreated it, unlinking by name would delete the live lock of that owner. + let owned: { dev: number; ino: number } | null = null; + if (fd != null) { + try { + const info = fstatSync(fd); + owned = { dev: info.dev, ino: info.ino }; + } catch { + owned = null; + } + closeSync(fd); + } try { - unlinkSync(path); + const current = statSync(path); + if (!owned || (current.dev === owned.dev && current.ino === owned.ino)) unlinkSync(path); } catch (err) { if (errCode(err) !== "ENOENT") throw err; } diff --git a/structure/catalog.md b/structure/catalog.md index 47a827426b..41d4b5140d 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -234,6 +234,8 @@ Pool mode routes across main plus added Codex credentials. Key rules: generation it started from still holds; a lost race raises a generation-conflict error rather than overwriting the newer credential (`src/codex/account-store.ts`). Callers handle that error; they do not assume a silent retry. + The lock itself is identity-scoped: a not-yet-readable lock counts as held until it ages out, + and a holder releases only the file it created, so a reclaimed path is not deleted twice. Warmup issues a bounded request with a fallback model so a cold account reports usability before a real turn depends on it (`src/codex/warmup.ts`). diff --git a/structure/providers/openai-tiers.md b/structure/providers/openai-tiers.md index a1824746dc..f2f99bd266 100644 --- a/structure/providers/openai-tiers.md +++ b/structure/providers/openai-tiers.md @@ -408,6 +408,10 @@ Pool mode needs stable public names and a store that survives concurrent refresh - The credential store is generation-guarded and refresh-locked (`src/codex/account-store.ts`): a refresh persists only if the generation it started from still holds, and a lost race raises a generation-conflict error instead of overwriting the newer credential. + The lock is held and released by file identity rather than by path. A lock that exists but is + not yet readable counts as held until it ages past the stale window, because its owner creates + the file and writes its metadata as two steps, and a holder deletes the lock only while the + path still resolves to the file it created. ## Sidecars, management, and UI diff --git a/tests/codex-integration/codex-account-store.test.ts b/tests/codex-integration/codex-account-store.test.ts index e44ce355e0..dfa47b4dcc 100644 --- a/tests/codex-integration/codex-account-store.test.ts +++ b/tests/codex-integration/codex-account-store.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test, beforeEach, afterEach, spyOn } from "bun:test"; import { createHash } from "node:crypto"; -import { existsSync, mkdtempSync, readdirSync, readFileSync, unlinkSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, readdirSync, readFileSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl"; @@ -624,6 +624,50 @@ describe("codex-account-store CRUD", () => { } }); + test("a refresh lock that is still being initialized is not reclaimed as stale", async () => { + const { getValidCodexToken, saveCodexAccountCredential } = await import("../../src/codex/account-store"); + saveCodexAccountCredential("refresh-empty-lock", { accessToken: "old", refreshToken: "empty-r", expiresAt: 0, chatgptAccountId: "acc" }); + // The owner creates the lock file and writes its metadata as two steps, so a live lock is + // briefly unreadable. Treating that window as stale let a waiter delete a lock whose owner + // was still inside its critical section, and both then ran the refresh. + const lockPath = refreshLockPathForToken("empty-r"); + writeFileSync(lockPath, ""); + let fetchCalls = 0; + const originalFetch = globalThis.fetch; + globalThis.fetch = (async () => { + fetchCalls += 1; + return new Response(JSON.stringify({ access_token: "new", expires_in: 3600 }), { status: 200 }); + }) as typeof fetch; + + try { + const pending = getValidCodexToken("refresh-empty-lock"); + await new Promise(resolve => setTimeout(resolve, 200)); + expect(existsSync(lockPath)).toBe(true); + expect(fetchCalls).toBe(0); + unlinkSync(lockPath); + const result = await pending; + expect(result.accessToken).toBe("new"); + expect(fetchCalls).toBe(1); + } finally { + globalThis.fetch = originalFetch; + } + }); + + test("releasing a refresh lock leaves a lock another owner recreated in place", async () => { + const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); + const lockKey = "recreated-owner"; + const lockPath = join(TEST_DIR, `codex-refresh-${createHash("sha256").update(lockKey).digest("hex").slice(0, 32)}.lock`); + await withCodexRefreshFileLock(lockKey, new AbortController().signal, async () => { + // A waiter reclaimed this path and a second owner took it over while we held it. + renameSync(lockPath, `${lockPath}.reclaimed`); + writeFileSync(lockPath, JSON.stringify({ acquiredAt: Date.now(), pid: 999_001 }) + "\n"); + }); + expect(existsSync(lockPath)).toBe(true); + expect((JSON.parse(readFileSync(lockPath, "utf-8")) as { pid: number }).pid).toBe(999_001); + unlinkSync(lockPath); + unlinkSync(`${lockPath}.reclaimed`); + }); + test("same refresh grant joins a live flight", async () => { const { getCodexAccountCredential, From 79d579233d18ad56f5c4dd37a9fc0dd12a027298 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Mon, 14 Sep 2026 11:16:42 +0900 Subject: [PATCH 2/7] fix(codex): preserve refresh locks when owner identity is unknown --- src/codex/account-store.ts | 14 +++++++---- structure/catalog.md | 3 ++- structure/providers/openai-tiers.md | 4 +++- .../codex-account-store.test.ts | 23 +++++++++++++++++++ 4 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 721288850b..34620eb8fa 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -657,19 +657,23 @@ export async function withCodexRefreshFileLock(lockKey: string, signal: Abort } finally { // Release only the lock this call created. If a waiter reclaimed the path as stale and a // new owner recreated it, unlinking by name would delete the live lock of that owner. - let owned: { dev: number; ino: number } | null = null; + let owned: { dev: bigint; ino: bigint } | null = null; if (fd != null) { try { - const info = fstatSync(fd); - owned = { dev: info.dev, ino: info.ino }; + const info = fstatSync(fd, { bigint: true }); + if (info.dev >= 0n && info.ino > 0n) { + owned = { dev: info.dev, ino: info.ino }; + } } catch { owned = null; } closeSync(fd); } try { - const current = statSync(path); - if (!owned || (current.dev === owned.dev && current.ino === owned.ino)) unlinkSync(path); + const current = statSync(path, { bigint: true }); + // An unreadable or unusable identity never authorizes removing the current path. + // Leave it for stale-lock recovery instead of deleting a possible replacement owner. + if (owned && current.dev === owned.dev && current.ino === owned.ino) unlinkSync(path); } catch (err) { if (errCode(err) !== "ENOENT") throw err; } diff --git a/structure/catalog.md b/structure/catalog.md index 41d4b5140d..725ccde6fc 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -235,7 +235,8 @@ Pool mode routes across main plus added Codex credentials. Key rules: than overwriting the newer credential (`src/codex/account-store.ts`). Callers handle that error; they do not assume a silent retry. The lock itself is identity-scoped: a not-yet-readable lock counts as held until it ages out, - and a holder releases only the file it created, so a reclaimed path is not deleted twice. + and release requires a usable matching descriptor identity. Unknown identity leaves the path + for stale recovery; stat followed by unlink does not provide atomic compare-and-delete. Warmup issues a bounded request with a fallback model so a cold account reports usability before a real turn depends on it (`src/codex/warmup.ts`). diff --git a/structure/providers/openai-tiers.md b/structure/providers/openai-tiers.md index f2f99bd266..c6a4cd8749 100644 --- a/structure/providers/openai-tiers.md +++ b/structure/providers/openai-tiers.md @@ -411,7 +411,9 @@ Pool mode needs stable public names and a store that survives concurrent refresh The lock is held and released by file identity rather than by path. A lock that exists but is not yet readable counts as held until it ages past the stale window, because its owner creates the file and writes its metadata as two steps, and a holder deletes the lock only while the - path still resolves to the file it created. + path still resolves to the file it created. If descriptor identity is unavailable or unusable, + release leaves the path for stale-lock recovery. The stat/unlink pair is not an atomic + compare-and-delete, so this check alone does not eliminate concurrent replacement races. ## Sidecars, management, and UI diff --git a/tests/codex-integration/codex-account-store.test.ts b/tests/codex-integration/codex-account-store.test.ts index dfa47b4dcc..3cdfb1a0af 100644 --- a/tests/codex-integration/codex-account-store.test.ts +++ b/tests/codex-integration/codex-account-store.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test, beforeEach, afterEach, spyOn } from "bun:test"; import { createHash } from "node:crypto"; +import * as fs from "node:fs"; import { existsSync, mkdtempSync, readdirSync, readFileSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -668,6 +669,28 @@ describe("codex-account-store CRUD", () => { unlinkSync(`${lockPath}.reclaimed`); }); + test("refresh release preserves the path when descriptor identity cannot be read", async () => { + const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); + const lockKey = "unknown-owner"; + const lockPath = join(TEST_DIR, `codex-refresh-${createHash("sha256").update(lockKey).digest("hex").slice(0, 32)}.lock`); + const original = fs.fstatSync; + let released = false; + const probe = spyOn(fs, "fstatSync").mockImplementation((...args: Parameters) => { + if (released) throw new Error("identity probe unavailable"); + return original(...args); + }); + try { + await withCodexRefreshFileLock(lockKey, new AbortController().signal, async () => { + renameSync(lockPath, `${lockPath}.reclaimed`); + writeFileSync(lockPath, "replacement-owner"); + released = true; + }); + expect(readFileSync(lockPath, "utf8")).toBe("replacement-owner"); + } finally { + probe.mockRestore(); + } + }); + test("same refresh grant joins a live flight", async () => { const { getCodexAccountCredential, From 3880e74f8e4c3625f8d560d22630a2549ebcf4dc Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Mon, 14 Sep 2026 16:30:35 +0900 Subject: [PATCH 3/7] docs(codex): clarify default cache affinity --- structure/providers/openai-tiers.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/structure/providers/openai-tiers.md b/structure/providers/openai-tiers.md index c6a4cd8749..43f1d1d789 100644 --- a/structure/providers/openai-tiers.md +++ b/structure/providers/openai-tiers.md @@ -546,8 +546,8 @@ consulted, and they run in `resolveCodexAccountForThreadDetailed` ahead of it. A with no recorded refusal is deliberately not a release path on its own — stickiness until the account actually refuses is intended — but it does surrender the binding as soon as a sibling with headroom exists. Unbound assignment is untouched and still takes the coolest eligible account, -because a fresh request has no warm prefix to lose. `pool.cacheAffinity` remains the stronger -opt-in, raising the bar from the threshold to genuine exhaustion. +because a fresh request has no warm prefix to lose. `pool.cacheAffinity` is enabled by default, +raising the bar from the threshold to genuine exhaustion. Two call sites need the rule — the live path in `reevaluateAffinityQuota` and the side-effect-free `previewReusableAffinityAccount` that subagent fallback reads — and they share one helper rather From aa3afb5c4943f79f3f3a3c8310e21e3d2e58ffd9 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Tue, 15 Sep 2026 10:46:49 +0900 Subject: [PATCH 4/7] docs: synchronize refresh-lock source owners --- structure/codex-home.md | 2 ++ structure/config.md | 2 ++ structure/gui-and-management-api.md | 2 +- structure/ops/docs-and-release.md | 2 ++ structure/runtime.md | 2 ++ structure/subagents.md | 2 ++ 6 files changed, 11 insertions(+), 1 deletion(-) diff --git a/structure/codex-home.md b/structure/codex-home.md index 339358ea9b..b1f9e7f4dd 100644 --- a/structure/codex-home.md +++ b/structure/codex-home.md @@ -1,5 +1,7 @@ # Codex Home +A lock in the Codex credential store is governed by [descriptor identity and age](catalog.md#accounts-namespaces-and-pool-rotation), so the mere presence of its filename is neither acquisition nor release authority. + ## Codex home `src/codex/paths.ts` resolves Codex state from `CODEX_HOME` when set and valid, otherwise from diff --git a/structure/config.md b/structure/config.md index 901aa582d6..885227a422 100644 --- a/structure/config.md +++ b/structure/config.md @@ -1,5 +1,7 @@ # Config Surface +Configuration consumers retain the [refresh-lock ownership boundary](catalog.md#accounts-namespaces-and-pool-rotation); failing to establish a usable matching lock identity does not authorize deleting its path. + The configuration-only [plaintext V2 contract](subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/gui-and-management-api.md b/structure/gui-and-management-api.md index 9eb9f1fa74..825ea150e4 100644 --- a/structure/gui-and-management-api.md +++ b/structure/gui-and-management-api.md @@ -5,7 +5,7 @@ is scoped to canonical ChatGPT Responses forwarding; other source-area behavior ## Dashboard serving -The bundled React dashboard is built into `gui/dist` and served by the same Bun proxy. `ocx gui` starts +Account refresh actions follow the [credential refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a held unreadable lock is distinct from one this process may release. The bundled React dashboard is built into `gui/dist` and served by the same Bun proxy. `ocx gui` starts the proxy when needed and opens `http://localhost:`, or `http://127.0.0.1:` when `hub.managementIngress.enabled` is true — see [the hub management dashboard address](runtime.md#hub-management-dashboard-address). All ordinary HTTP responses (excluding successful WebSocket upgrades) include `X-Frame-Options: DENY` and diff --git a/structure/ops/docs-and-release.md b/structure/ops/docs-and-release.md index f4474c6419..9977b54f71 100644 --- a/structure/ops/docs-and-release.md +++ b/structure/ops/docs-and-release.md @@ -1,5 +1,7 @@ # Docs And Release +Refresh-lock validation covers fresh unreadable locks and descriptor-matched release in `tests/codex-integration/codex-account-store.test.ts`; the [catalog contract](../catalog.md#accounts-namespaces-and-pool-rotation) explicitly does not promise atomic compare-and-delete. + The configuration-only [plaintext V2 contract](../subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/runtime.md b/structure/runtime.md index 9d97326b34..91a56aa088 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -1,5 +1,7 @@ # Runtime +OAuth refresh coordination follows the [refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a fresh unreadable lock remains held, and release requires matching descriptor identity. + The configuration-only [plaintext V2 contract](subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/subagents.md b/structure/subagents.md index 70c93a9ac5..ba4e2e227c 100644 --- a/structure/subagents.md +++ b/structure/subagents.md @@ -1,5 +1,7 @@ # Subagents And Multi-Agent Surface +Concurrent refreshes triggered by independent agent work share the [credential refresh-lock contract](catalog.md#accounts-namespaces-and-pool-rotation); unknown lock identity remains available for stale recovery rather than immediate removal. + ## Plaintext V2 agent messages `src/responses/plaintext-v2-agent-messages.ts` owns the experimental, configuration-only From 52779efaac464ee972b80ebdb3ea0e5e8baaba40 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:36:47 +0900 Subject: [PATCH 5/7] fix(codex): preserve refresh result when lock identity probe fails --- src/codex/account-store.ts | 16 ++++--- structure/catalog.md | 2 +- structure/codex-home.md | 2 +- structure/config.md | 2 +- structure/gui-and-management-api.md | 2 +- structure/ops/docs-and-release.md | 2 +- structure/providers/openai-tiers.md | 2 +- structure/runtime.md | 2 +- structure/subagents.md | 2 +- .../codex-account-store.test.ts | 46 +++++++++++++++++++ 10 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 45d2de536c..7c3cdc6414 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -711,13 +711,17 @@ export async function withCodexRefreshFileLock(lockKey: string, signal: Abort } closeSync(fd); } + let current: { dev: bigint; ino: bigint } | null = null; try { - const current = statSync(path, { bigint: true }); - // An unreadable or unusable identity never authorizes removing the current path. - // Leave it for stale-lock recovery instead of deleting a possible replacement owner. - if (owned && current.dev === owned.dev && current.ino === owned.ino) unlinkSync(path); - } catch (err) { - if (errCode(err) !== "ENOENT") throw err; + const info = statSync(path, { bigint: true }); + if (info.dev >= 0n && info.ino > 0n) current = { dev: info.dev, ino: info.ino }; + } catch { + // Unknown path identity leaves the lock for stale recovery without masking fn(). + } + if (owned && current && current.dev === owned.dev && current.ino === owned.ino) { + try { unlinkSync(path); } catch (err) { + if (errCode(err) !== "ENOENT") throw err; + } } } } diff --git a/structure/catalog.md b/structure/catalog.md index cb0f370ec9..da6ef17f4c 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -243,7 +243,7 @@ Pool mode routes across main plus added Codex credentials. Key rules: they do not assume a silent retry. The lock itself is identity-scoped: a not-yet-readable lock counts as held until it ages out, and release requires a usable matching descriptor identity. Unknown identity leaves the path - for stale recovery; stat followed by unlink does not provide atomic compare-and-delete. + for stale recovery without replacing the callback outcome when the path probe fails; confirmed-owner unlink errors other than `ENOENT` still propagate. Stat followed by unlink does not provide atomic compare-and-delete. - **Authentication identity, quota domain, and cache domain are tracked separately** (`src/routing/identity-domains.ts`). `classifyCredential` returns all three with provenance: `pool.credentialGroups` supplies operator-declared quota domains, a small built-in table diff --git a/structure/codex-home.md b/structure/codex-home.md index b1f9e7f4dd..36d27420de 100644 --- a/structure/codex-home.md +++ b/structure/codex-home.md @@ -1,6 +1,6 @@ # Codex Home -A lock in the Codex credential store is governed by [descriptor identity and age](catalog.md#accounts-namespaces-and-pool-rotation), so the mere presence of its filename is neither acquisition nor release authority. +A lock in the Codex credential store is governed by [descriptor identity and age](catalog.md#accounts-namespaces-and-pool-rotation), so the mere presence of its filename is neither acquisition nor release authority. Failed path-identity probes leave the lock for stale recovery and preserve the refresh callback outcome. ## Codex home diff --git a/structure/config.md b/structure/config.md index 885227a422..4fb8529ee6 100644 --- a/structure/config.md +++ b/structure/config.md @@ -1,6 +1,6 @@ # Config Surface -Configuration consumers retain the [refresh-lock ownership boundary](catalog.md#accounts-namespaces-and-pool-rotation); failing to establish a usable matching lock identity does not authorize deleting its path. +Configuration consumers retain the [refresh-lock ownership boundary](catalog.md#accounts-namespaces-and-pool-rotation); failing to establish a usable matching lock identity does not authorize deleting its path or replacing the refresh callback outcome with a path-probe error. The configuration-only [plaintext V2 contract](subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/gui-and-management-api.md b/structure/gui-and-management-api.md index 825ea150e4..c32c2373c8 100644 --- a/structure/gui-and-management-api.md +++ b/structure/gui-and-management-api.md @@ -5,7 +5,7 @@ is scoped to canonical ChatGPT Responses forwarding; other source-area behavior ## Dashboard serving -Account refresh actions follow the [credential refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a held unreadable lock is distinct from one this process may release. The bundled React dashboard is built into `gui/dist` and served by the same Bun proxy. `ocx gui` starts +Account refresh actions follow the [credential refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a held unreadable lock is distinct from one this process may release, and path-probe errors preserve the callback outcome. The bundled React dashboard is built into `gui/dist` and served by the same Bun proxy. `ocx gui` starts the proxy when needed and opens `http://localhost:`, or `http://127.0.0.1:` when `hub.managementIngress.enabled` is true — see [the hub management dashboard address](runtime.md#hub-management-dashboard-address). All ordinary HTTP responses (excluding successful WebSocket upgrades) include `X-Frame-Options: DENY` and diff --git a/structure/ops/docs-and-release.md b/structure/ops/docs-and-release.md index 9977b54f71..11c8edfe66 100644 --- a/structure/ops/docs-and-release.md +++ b/structure/ops/docs-and-release.md @@ -1,6 +1,6 @@ # Docs And Release -Refresh-lock validation covers fresh unreadable locks and descriptor-matched release in `tests/codex-integration/codex-account-store.test.ts`; the [catalog contract](../catalog.md#accounts-namespaces-and-pool-rotation) explicitly does not promise atomic compare-and-delete. +Refresh-lock validation covers fresh unreadable locks, descriptor-matched release, path-probe failures preserving callback outcomes, and confirmed-owner unlink error handling in `tests/codex-integration/codex-account-store.test.ts`; the [catalog contract](../catalog.md#accounts-namespaces-and-pool-rotation) explicitly does not promise atomic compare-and-delete. The configuration-only [plaintext V2 contract](../subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/providers/openai-tiers.md b/structure/providers/openai-tiers.md index 8eb31f164c..ca2fe984ef 100644 --- a/structure/providers/openai-tiers.md +++ b/structure/providers/openai-tiers.md @@ -432,7 +432,7 @@ Pool mode needs stable public names and a store that survives concurrent refresh not yet readable counts as held until it ages past the stale window, because its owner creates the file and writes its metadata as two steps, and a holder deletes the lock only while the path still resolves to the file it created. If descriptor identity is unavailable or unusable, - release leaves the path for stale-lock recovery. The stat/unlink pair is not an atomic + release leaves the path for stale-lock recovery. Path-probe errors preserve the callback outcome; confirmed-owner unlink errors other than `ENOENT` still propagate. The stat/unlink pair is not an atomic compare-and-delete, so this check alone does not eliminate concurrent replacement races. ## Sidecars, management, and UI diff --git a/structure/runtime.md b/structure/runtime.md index 91a56aa088..6bd8b1add6 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -1,6 +1,6 @@ # Runtime -OAuth refresh coordination follows the [refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a fresh unreadable lock remains held, and release requires matching descriptor identity. +OAuth refresh coordination follows the [refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a fresh unreadable lock remains held, and release requires matching descriptor identity. A failed path-identity probe preserves the refresh callback outcome. The configuration-only [plaintext V2 contract](subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/subagents.md b/structure/subagents.md index ba4e2e227c..8bf9f04328 100644 --- a/structure/subagents.md +++ b/structure/subagents.md @@ -1,6 +1,6 @@ # Subagents And Multi-Agent Surface -Concurrent refreshes triggered by independent agent work share the [credential refresh-lock contract](catalog.md#accounts-namespaces-and-pool-rotation); unknown lock identity remains available for stale recovery rather than immediate removal. +Concurrent refreshes triggered by independent agent work share the [credential refresh-lock contract](catalog.md#accounts-namespaces-and-pool-rotation); unknown lock identity remains available for stale recovery rather than immediate removal, and a failed path probe cannot mask the callback outcome. ## Plaintext V2 agent messages diff --git a/tests/codex-integration/codex-account-store.test.ts b/tests/codex-integration/codex-account-store.test.ts index 3cdfb1a0af..945086afc4 100644 --- a/tests/codex-integration/codex-account-store.test.ts +++ b/tests/codex-integration/codex-account-store.test.ts @@ -691,6 +691,52 @@ describe("codex-account-store CRUD", () => { } }); + for (const code of ["EACCES", "EIO"]) { + for (const callbackFails of [false, true]) { + test(`refresh release preserves the callback outcome after ${code} path probe failure (${callbackFails})`, async () => { + const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); + const lockKey = `path-probe-${code}-${callbackFails}`; + const lockPath = join(TEST_DIR, `codex-refresh-${createHash("sha256").update(lockKey).digest("hex").slice(0, 32)}.lock`); + const original = fs.statSync; + const callbackError = new Error("refresh failed"); + let released = false; + const probe = spyOn(fs, "statSync").mockImplementation((...args: Parameters) => { + if (released && args[0] === lockPath) throw Object.assign(new Error("path probe unavailable"), { code }); + return original(...args); + }); + try { + const pending = withCodexRefreshFileLock(lockKey, new AbortController().signal, async () => { + released = true; + if (callbackFails) throw callbackError; + return "refreshed"; + }); + if (callbackFails) await expect(pending).rejects.toBe(callbackError); + else expect(await pending).toBe("refreshed"); + expect(existsSync(lockPath)).toBe(true); + } finally { probe.mockRestore(); } + }); + } + } + + test.each(["ENOENT", "EACCES"])("refresh release preserves confirmed-owner unlink handling for %s", async (code) => { + const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); + const lockKey = `unlink-${code}`; + const lockPath = join(TEST_DIR, `codex-refresh-${createHash("sha256").update(lockKey).digest("hex").slice(0, 32)}.lock`); + const original = fs.unlinkSync; + const unlinkError = Object.assign(new Error("unlink failed"), { code }); + let attempts = 0; + const probe = spyOn(fs, "unlinkSync").mockImplementation((path) => { + if (path === lockPath) { attempts++; throw unlinkError; } + return original(path); + }); + try { + const pending = withCodexRefreshFileLock(lockKey, new AbortController().signal, async () => "refreshed"); + if (code === "ENOENT") expect(await pending).toBe("refreshed"); + else await expect(pending).rejects.toBe(unlinkError); + expect(attempts).toBe(1); + } finally { probe.mockRestore(); } + }); + test("same refresh grant joins a live flight", async () => { const { getCodexAccountCredential, From 8e14d34c3a765306fd2b3ba36cc697394b03b96d Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:02:47 +0900 Subject: [PATCH 6/7] fix(codex): serialize refresh lock metadata and clean failed acquisition --- src/codex/account-store.ts | 95 +++++++++++-------- structure/catalog.md | 2 +- structure/codex-home.md | 2 +- structure/config.md | 2 +- structure/gui-and-management-api.md | 2 +- structure/ops/docs-and-release.md | 2 +- structure/providers/openai-tiers.md | 2 +- structure/runtime.md | 2 +- structure/subagents.md | 2 +- .../codex-account-store.test.ts | 66 +++++++++++++ 10 files changed, 131 insertions(+), 46 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 7c3cdc6414..7d78e998c3 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -665,6 +665,33 @@ function isRefreshLockStale(path: string): boolean { } } +function releaseCodexRefreshFileLock(path: string, fd: number): void { + let owned: { dev: bigint; ino: bigint } | null = null; + try { + const info = fstatSync(fd, { bigint: true }); + if (info.dev >= 0n && info.ino > 0n) owned = { dev: info.dev, ino: info.ino }; + } catch { /* Unknown descriptor identity never authorizes unlink. */ } + closeSync(fd); + try { + withConfigMutationLockSync(() => { + let current: { dev: bigint; ino: bigint } | null = null; + try { + const info = statSync(path, { bigint: true }); + if (info.dev >= 0n && info.ino > 0n) current = { dev: info.dev, ino: info.ino }; + } catch { /* Keep the lock and the callback outcome when the path probe fails. */ } + if (owned && current && current.dev === owned.dev && current.ino === owned.ino) { + try { unlinkSync(path); } catch (err) { + if (errCode(err) !== "ENOENT") throw err; + } + } + }); + } catch (err) { + // The descriptor is already closed. A busy/unavailable metadata transaction leaves the + // path for stale recovery rather than masking the completed refresh with cleanup failure. + if (!(err instanceof ConfigMutationLockError)) throw err; + } +} + export async function withCodexRefreshFileLock(lockKey: string, signal: AbortSignal, fn: () => Promise): Promise { hardenConfigDir(); const dir = getConfigDir(); @@ -676,53 +703,45 @@ export async function withCodexRefreshFileLock(lockKey: string, signal: Abort while (fd == null) { if (signal.aborted) throw signal.reason; try { - fd = openSync(path, "wx", 0o600); - writeFileSync(fd, JSON.stringify({ acquiredAt: Date.now(), pid: process.pid }) + "\n"); - break; - } catch (err) { - if (errCode(err) !== "EEXIST") throw err; - if (isRefreshLockStale(path)) { + // Serialize only metadata operations, never the async refresh callback. Cooperating + // contenders cannot reclaim a successor between stale observation and path mutation. + withConfigMutationLockSync(() => { try { - unlinkSync(path); - } catch (unlinkErr) { - if (errCode(unlinkErr) !== "ENOENT") throw unlinkErr; + fd = openSync(path, "wx", 0o600); + writeFileSync(fd, JSON.stringify({ acquiredAt: Date.now(), pid: process.pid }) + "\n"); + } catch (err) { + if (fd != null) { + const failedFd = fd; + fd = null; + try { releaseCodexRefreshFileLock(path, failedFd); } catch { /* Preserve write failure. */ } + throw err; + } + if (errCode(err) !== "EEXIST") throw err; + if (isRefreshLockStale(path)) { + try { unlinkSync(path); } catch (unlinkErr) { + if (errCode(unlinkErr) !== "ENOENT") throw unlinkErr; + } + } } - continue; + }); + } catch (err) { + // A failed SQLite commit can follow successful file creation; it still owns an fd. + if (fd != null) { + const failedFd = fd; + fd = null; + try { releaseCodexRefreshFileLock(path, failedFd); } catch { /* Preserve admission failure. */ } } - if (Date.now() >= deadline) throw new CodexCredentialRefreshLockTimeoutError(); - await sleep(REFRESH_LOCK_POLL_MS, signal); + if (!(err instanceof ConfigMutationLockError)) throw err; } + if (fd != null) break; + if (Date.now() >= deadline) throw new CodexCredentialRefreshLockTimeoutError(); + await sleep(REFRESH_LOCK_POLL_MS, signal); } try { return await fn(); } finally { - // Release only the lock this call created. If a waiter reclaimed the path as stale and a - // new owner recreated it, unlinking by name would delete the live lock of that owner. - let owned: { dev: bigint; ino: bigint } | null = null; - if (fd != null) { - try { - const info = fstatSync(fd, { bigint: true }); - if (info.dev >= 0n && info.ino > 0n) { - owned = { dev: info.dev, ino: info.ino }; - } - } catch { - owned = null; - } - closeSync(fd); - } - let current: { dev: bigint; ino: bigint } | null = null; - try { - const info = statSync(path, { bigint: true }); - if (info.dev >= 0n && info.ino > 0n) current = { dev: info.dev, ino: info.ino }; - } catch { - // Unknown path identity leaves the lock for stale recovery without masking fn(). - } - if (owned && current && current.dev === owned.dev && current.ino === owned.ino) { - try { unlinkSync(path); } catch (err) { - if (errCode(err) !== "ENOENT") throw err; - } - } + releaseCodexRefreshFileLock(path, fd); } } diff --git a/structure/catalog.md b/structure/catalog.md index da6ef17f4c..9e91c415bc 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -243,7 +243,7 @@ Pool mode routes across main plus added Codex credentials. Key rules: they do not assume a silent retry. The lock itself is identity-scoped: a not-yet-readable lock counts as held until it ages out, and release requires a usable matching descriptor identity. Unknown identity leaves the path - for stale recovery without replacing the callback outcome when the path probe fails; confirmed-owner unlink errors other than `ENOENT` still propagate. Stat followed by unlink does not provide atomic compare-and-delete. + for stale recovery without replacing the callback outcome when the path probe fails; confirmed-owner unlink errors other than `ENOENT` still propagate. Acquisition, stale reclamation and identity-checked release run inside the existing synchronous SQLite config-mutation transaction; the async refresh callback runs outside it. A failed metadata write closes its descriptor and removes only a matching owned path. Busy release coordination preserves the callback outcome and leaves the path for stale recovery. This serializes cooperating writers; stat/unlink is not atomic against non-cooperating filesystem writers. - **Authentication identity, quota domain, and cache domain are tracked separately** (`src/routing/identity-domains.ts`). `classifyCredential` returns all three with provenance: `pool.credentialGroups` supplies operator-declared quota domains, a small built-in table diff --git a/structure/codex-home.md b/structure/codex-home.md index 36d27420de..838ccc5feb 100644 --- a/structure/codex-home.md +++ b/structure/codex-home.md @@ -1,6 +1,6 @@ # Codex Home -A lock in the Codex credential store is governed by [descriptor identity and age](catalog.md#accounts-namespaces-and-pool-rotation), so the mere presence of its filename is neither acquisition nor release authority. Failed path-identity probes leave the lock for stale recovery and preserve the refresh callback outcome. +A lock in the Codex credential store is governed by [descriptor identity and age](catalog.md#accounts-namespaces-and-pool-rotation), so the mere presence of its filename is neither acquisition nor release authority. Failed path-identity probes leave the lock for stale recovery and preserve the refresh callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. ## Codex home diff --git a/structure/config.md b/structure/config.md index 4fb8529ee6..abf4a1e07c 100644 --- a/structure/config.md +++ b/structure/config.md @@ -1,6 +1,6 @@ # Config Surface -Configuration consumers retain the [refresh-lock ownership boundary](catalog.md#accounts-namespaces-and-pool-rotation); failing to establish a usable matching lock identity does not authorize deleting its path or replacing the refresh callback outcome with a path-probe error. +Configuration consumers retain the [refresh-lock ownership boundary](catalog.md#accounts-namespaces-and-pool-rotation); failing to establish a usable matching lock identity does not authorize deleting its path or replacing the refresh callback outcome with a path-probe error. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. The configuration-only [plaintext V2 contract](subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/gui-and-management-api.md b/structure/gui-and-management-api.md index c32c2373c8..0113b286c9 100644 --- a/structure/gui-and-management-api.md +++ b/structure/gui-and-management-api.md @@ -5,7 +5,7 @@ is scoped to canonical ChatGPT Responses forwarding; other source-area behavior ## Dashboard serving -Account refresh actions follow the [credential refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a held unreadable lock is distinct from one this process may release, and path-probe errors preserve the callback outcome. The bundled React dashboard is built into `gui/dist` and served by the same Bun proxy. `ocx gui` starts +Account refresh actions follow the [credential refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a held unreadable lock is distinct from one this process may release, and path-probe errors preserve the callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. The bundled React dashboard is built into `gui/dist` and served by the same Bun proxy. `ocx gui` starts the proxy when needed and opens `http://localhost:`, or `http://127.0.0.1:` when `hub.managementIngress.enabled` is true — see [the hub management dashboard address](runtime.md#hub-management-dashboard-address). All ordinary HTTP responses (excluding successful WebSocket upgrades) include `X-Frame-Options: DENY` and diff --git a/structure/ops/docs-and-release.md b/structure/ops/docs-and-release.md index 11c8edfe66..3ee5843b00 100644 --- a/structure/ops/docs-and-release.md +++ b/structure/ops/docs-and-release.md @@ -1,6 +1,6 @@ # Docs And Release -Refresh-lock validation covers fresh unreadable locks, descriptor-matched release, path-probe failures preserving callback outcomes, and confirmed-owner unlink error handling in `tests/codex-integration/codex-account-store.test.ts`; the [catalog contract](../catalog.md#accounts-namespaces-and-pool-rotation) explicitly does not promise atomic compare-and-delete. +Refresh-lock validation covers fresh unreadable locks, descriptor-matched release, path-probe failures preserving callback outcomes, and confirmed-owner unlink error handling in `tests/codex-integration/codex-account-store.test.ts`; the [catalog contract](../catalog.md#accounts-namespaces-and-pool-rotation) explicitly does not promise atomic compare-and-delete. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. The configuration-only [plaintext V2 contract](../subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/providers/openai-tiers.md b/structure/providers/openai-tiers.md index ca2fe984ef..7f1e9eaf23 100644 --- a/structure/providers/openai-tiers.md +++ b/structure/providers/openai-tiers.md @@ -433,7 +433,7 @@ Pool mode needs stable public names and a store that survives concurrent refresh the file and writes its metadata as two steps, and a holder deletes the lock only while the path still resolves to the file it created. If descriptor identity is unavailable or unusable, release leaves the path for stale-lock recovery. Path-probe errors preserve the callback outcome; confirmed-owner unlink errors other than `ENOENT` still propagate. The stat/unlink pair is not an atomic - compare-and-delete, so this check alone does not eliminate concurrent replacement races. + compare-and-delete against non-cooperating writers. Cooperating acquisition, stale reclamation and release serialize inside the synchronous config-mutation transaction, released before the async callback. Failed metadata writes close their descriptor and clean only a matching owned path. ## Sidecars, management, and UI diff --git a/structure/runtime.md b/structure/runtime.md index 6bd8b1add6..97cc074a09 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -1,6 +1,6 @@ # Runtime -OAuth refresh coordination follows the [refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a fresh unreadable lock remains held, and release requires matching descriptor identity. A failed path-identity probe preserves the refresh callback outcome. +OAuth refresh coordination follows the [refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a fresh unreadable lock remains held, and release requires matching descriptor identity. A failed path-identity probe preserves the refresh callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. The configuration-only [plaintext V2 contract](subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/subagents.md b/structure/subagents.md index 8bf9f04328..44fa3cd86b 100644 --- a/structure/subagents.md +++ b/structure/subagents.md @@ -1,6 +1,6 @@ # Subagents And Multi-Agent Surface -Concurrent refreshes triggered by independent agent work share the [credential refresh-lock contract](catalog.md#accounts-namespaces-and-pool-rotation); unknown lock identity remains available for stale recovery rather than immediate removal, and a failed path probe cannot mask the callback outcome. +Concurrent refreshes triggered by independent agent work share the [credential refresh-lock contract](catalog.md#accounts-namespaces-and-pool-rotation); unknown lock identity remains available for stale recovery rather than immediate removal, and a failed path probe cannot mask the callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. ## Plaintext V2 agent messages diff --git a/tests/codex-integration/codex-account-store.test.ts b/tests/codex-integration/codex-account-store.test.ts index 945086afc4..8407e1b597 100644 --- a/tests/codex-integration/codex-account-store.test.ts +++ b/tests/codex-integration/codex-account-store.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test, beforeEach, afterEach, spyOn } from "bun:test"; import { createHash } from "node:crypto"; +import { Database } from "bun:sqlite"; import * as fs from "node:fs"; import { existsSync, mkdtempSync, readdirSync, readFileSync, renameSync, unlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; @@ -737,6 +738,71 @@ describe("codex-account-store CRUD", () => { } finally { probe.mockRestore(); } }); + test("refresh stale reclamation excludes a second SQLite writer until acquisition finishes", async () => { + const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); + const key = "serialized-stale"; + const path = join(TEST_DIR, `codex-refresh-${createHash("sha256").update(key).digest("hex").slice(0, 32)}.lock`); + writeFileSync(path, JSON.stringify({ acquiredAt: 0 })); + const db = new Database(join(TEST_DIR, "config-mutation.sqlite"), { create: true }); + const original = fs.unlinkSync; + let blocked = false; + const probe = spyOn(fs, "unlinkSync").mockImplementation((candidate) => { + if (candidate === path && !blocked) { + try { db.exec("BEGIN IMMEDIATE"); db.exec("ROLLBACK"); } + catch (error) { blocked = (error as { code?: string }).code === "SQLITE_BUSY"; } + } + return original(candidate); + }); + try { + await withCodexRefreshFileLock(key, new AbortController().signal, async () => { + expect(blocked).toBe(true); + // The callback must not hold the metadata transaction across network/async work. + db.exec("BEGIN IMMEDIATE"); db.exec("ROLLBACK"); + }); + expect(existsSync(path)).toBe(false); + } finally { probe.mockRestore(); db.close(); } + }); + + test.each([false, true])("refresh metadata failure closes its descriptor and preserves replacement=%s", async (replacement) => { + const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); + const key = `metadata-write-${replacement}`; + const path = join(TEST_DIR, `codex-refresh-${createHash("sha256").update(key).digest("hex").slice(0, 32)}.lock`); + const original = fs.writeFileSync; + const failure = Object.assign(new Error("metadata write failed"), { code: "EIO" }); + let descriptor: number | undefined; + let called = false; + const probe = spyOn(fs, "writeFileSync").mockImplementation((...args: Parameters) => { + if (typeof args[0] === "number") { + descriptor = args[0]; + if (replacement) { renameSync(path, `${path}.reclaimed`); original(path, "successor"); } + throw failure; + } + return original(...args); + }); + try { + await expect(withCodexRefreshFileLock(key, new AbortController().signal, async () => { called = true; })).rejects.toBe(failure); + expect(called).toBe(false); + expect(descriptor).toBeDefined(); + expect(() => fs.fstatSync(descriptor!)).toThrow(); + expect(existsSync(path)).toBe(replacement); + if (replacement) expect(readFileSync(path, "utf8")).toBe("successor"); + } finally { probe.mockRestore(); } + }); + + test("refresh release keeps its result and lock when metadata coordination is busy", async () => { + const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); + const key = "release-coordination-busy"; + const path = join(TEST_DIR, `codex-refresh-${createHash("sha256").update(key).digest("hex").slice(0, 32)}.lock`); + const db = new Database(join(TEST_DIR, "config-mutation.sqlite"), { create: true }); + try { + expect(await withCodexRefreshFileLock(key, new AbortController().signal, async () => { + db.exec("BEGIN IMMEDIATE"); + return "refreshed"; + })).toBe("refreshed"); + expect(existsSync(path)).toBe(true); + } finally { db.exec("ROLLBACK"); db.close(); } + }); + test("same refresh grant joins a live flight", async () => { const { getCodexAccountCredential, From 5b8b070e0a3b0d9bcf138e5124dee1adb8792e55 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Tue, 15 Sep 2026 14:28:52 +0900 Subject: [PATCH 7/7] fix(codex): keep refresh lock descriptor alive through release --- src/codex/account-store.ts | 7 ++-- structure/catalog.md | 2 +- structure/codex-home.md | 2 +- structure/config.md | 2 +- structure/gui-and-management-api.md | 2 +- structure/ops/docs-and-release.md | 2 +- structure/providers/openai-tiers.md | 2 +- structure/runtime.md | 2 +- structure/subagents.md | 2 +- .../codex-account-store.test.ts | 40 +++++++++++++++++++ 10 files changed, 51 insertions(+), 12 deletions(-) diff --git a/src/codex/account-store.ts b/src/codex/account-store.ts index 7d78e998c3..84ad976f74 100644 --- a/src/codex/account-store.ts +++ b/src/codex/account-store.ts @@ -671,7 +671,6 @@ function releaseCodexRefreshFileLock(path: string, fd: number): void { const info = fstatSync(fd, { bigint: true }); if (info.dev >= 0n && info.ino > 0n) owned = { dev: info.dev, ino: info.ino }; } catch { /* Unknown descriptor identity never authorizes unlink. */ } - closeSync(fd); try { withConfigMutationLockSync(() => { let current: { dev: bigint; ino: bigint } | null = null; @@ -686,10 +685,10 @@ function releaseCodexRefreshFileLock(path: string, fd: number): void { } }); } catch (err) { - // The descriptor is already closed. A busy/unavailable metadata transaction leaves the - // path for stale recovery rather than masking the completed refresh with cleanup failure. + // Keep the descriptor alive through comparison/unlink so its inode cannot be recycled. + // Unavailable coordination leaves the path without masking the completed refresh. if (!(err instanceof ConfigMutationLockError)) throw err; - } + } finally { closeSync(fd); } } export async function withCodexRefreshFileLock(lockKey: string, signal: AbortSignal, fn: () => Promise): Promise { diff --git a/structure/catalog.md b/structure/catalog.md index 9e91c415bc..18fa416f62 100644 --- a/structure/catalog.md +++ b/structure/catalog.md @@ -243,7 +243,7 @@ Pool mode routes across main plus added Codex credentials. Key rules: they do not assume a silent retry. The lock itself is identity-scoped: a not-yet-readable lock counts as held until it ages out, and release requires a usable matching descriptor identity. Unknown identity leaves the path - for stale recovery without replacing the callback outcome when the path probe fails; confirmed-owner unlink errors other than `ENOENT` still propagate. Acquisition, stale reclamation and identity-checked release run inside the existing synchronous SQLite config-mutation transaction; the async refresh callback runs outside it. A failed metadata write closes its descriptor and removes only a matching owned path. Busy release coordination preserves the callback outcome and leaves the path for stale recovery. This serializes cooperating writers; stat/unlink is not atomic against non-cooperating filesystem writers. + for stale recovery without replacing the callback outcome when the path probe fails; confirmed-owner unlink errors other than `ENOENT` still propagate. Acquisition, stale reclamation and identity-checked release run inside the existing synchronous SQLite config-mutation transaction; the async refresh callback runs outside it. Release keeps the descriptor open through identity comparison and any unlink, then closes it. Failed metadata writes remove only a matching owned path after successful coordination; unknown identity, failed probes or unavailable coordination retain the path for stale recovery. Busy release coordination preserves the callback outcome and leaves the path for stale recovery. This serializes cooperating writers; stat/unlink is not atomic against non-cooperating filesystem writers. - **Authentication identity, quota domain, and cache domain are tracked separately** (`src/routing/identity-domains.ts`). `classifyCredential` returns all three with provenance: `pool.credentialGroups` supplies operator-declared quota domains, a small built-in table diff --git a/structure/codex-home.md b/structure/codex-home.md index 838ccc5feb..5a95c396b5 100644 --- a/structure/codex-home.md +++ b/structure/codex-home.md @@ -1,6 +1,6 @@ # Codex Home -A lock in the Codex credential store is governed by [descriptor identity and age](catalog.md#accounts-namespaces-and-pool-rotation), so the mere presence of its filename is neither acquisition nor release authority. Failed path-identity probes leave the lock for stale recovery and preserve the refresh callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. +A lock in the Codex credential store is governed by [descriptor identity and age](catalog.md#accounts-namespaces-and-pool-rotation), so the mere presence of its filename is neither acquisition nor release authority. Failed path-identity probes leave the lock for stale recovery and preserve the refresh callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; release keeps the descriptor open through identity comparison and any unlink, then closes it. Failed metadata writes remove only a matching owned path after successful coordination; unknown identity, failed probes or unavailable coordination retain the path for stale recovery. Async refresh work holds no metadata transaction. ## Codex home diff --git a/structure/config.md b/structure/config.md index abf4a1e07c..d704d6eb6b 100644 --- a/structure/config.md +++ b/structure/config.md @@ -1,6 +1,6 @@ # Config Surface -Configuration consumers retain the [refresh-lock ownership boundary](catalog.md#accounts-namespaces-and-pool-rotation); failing to establish a usable matching lock identity does not authorize deleting its path or replacing the refresh callback outcome with a path-probe error. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. +Configuration consumers retain the [refresh-lock ownership boundary](catalog.md#accounts-namespaces-and-pool-rotation); failing to establish a usable matching lock identity does not authorize deleting its path or replacing the refresh callback outcome with a path-probe error. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; release keeps the descriptor open through identity comparison and any unlink, then closes it. Failed metadata writes remove only a matching owned path after successful coordination; unknown identity, failed probes or unavailable coordination retain the path for stale recovery. Async refresh work holds no metadata transaction. The configuration-only [plaintext V2 contract](subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/gui-and-management-api.md b/structure/gui-and-management-api.md index 0113b286c9..af1c588a5f 100644 --- a/structure/gui-and-management-api.md +++ b/structure/gui-and-management-api.md @@ -5,7 +5,7 @@ is scoped to canonical ChatGPT Responses forwarding; other source-area behavior ## Dashboard serving -Account refresh actions follow the [credential refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a held unreadable lock is distinct from one this process may release, and path-probe errors preserve the callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. The bundled React dashboard is built into `gui/dist` and served by the same Bun proxy. `ocx gui` starts +Account refresh actions follow the [credential refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a held unreadable lock is distinct from one this process may release, and path-probe errors preserve the callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; release keeps the descriptor open through identity comparison and any unlink, then closes it. Failed metadata writes remove only a matching owned path after successful coordination; unknown identity, failed probes or unavailable coordination retain the path for stale recovery. Async refresh work holds no metadata transaction. The bundled React dashboard is built into `gui/dist` and served by the same Bun proxy. `ocx gui` starts the proxy when needed and opens `http://localhost:`, or `http://127.0.0.1:` when `hub.managementIngress.enabled` is true — see [the hub management dashboard address](runtime.md#hub-management-dashboard-address). All ordinary HTTP responses (excluding successful WebSocket upgrades) include `X-Frame-Options: DENY` and diff --git a/structure/ops/docs-and-release.md b/structure/ops/docs-and-release.md index 3ee5843b00..7b9ce3c91b 100644 --- a/structure/ops/docs-and-release.md +++ b/structure/ops/docs-and-release.md @@ -1,6 +1,6 @@ # Docs And Release -Refresh-lock validation covers fresh unreadable locks, descriptor-matched release, path-probe failures preserving callback outcomes, and confirmed-owner unlink error handling in `tests/codex-integration/codex-account-store.test.ts`; the [catalog contract](../catalog.md#accounts-namespaces-and-pool-rotation) explicitly does not promise atomic compare-and-delete. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. +Refresh-lock validation covers fresh unreadable locks, descriptor-matched release, path-probe failures preserving callback outcomes, and confirmed-owner unlink error handling in `tests/codex-integration/codex-account-store.test.ts`; the [catalog contract](../catalog.md#accounts-namespaces-and-pool-rotation) explicitly does not promise atomic compare-and-delete. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; release keeps the descriptor open through identity comparison and any unlink, then closes it. Failed metadata writes remove only a matching owned path after successful coordination; unknown identity, failed probes or unavailable coordination retain the path for stale recovery. Async refresh work holds no metadata transaction. The configuration-only [plaintext V2 contract](../subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/providers/openai-tiers.md b/structure/providers/openai-tiers.md index 7f1e9eaf23..ddc03cfcb0 100644 --- a/structure/providers/openai-tiers.md +++ b/structure/providers/openai-tiers.md @@ -433,7 +433,7 @@ Pool mode needs stable public names and a store that survives concurrent refresh the file and writes its metadata as two steps, and a holder deletes the lock only while the path still resolves to the file it created. If descriptor identity is unavailable or unusable, release leaves the path for stale-lock recovery. Path-probe errors preserve the callback outcome; confirmed-owner unlink errors other than `ENOENT` still propagate. The stat/unlink pair is not an atomic - compare-and-delete against non-cooperating writers. Cooperating acquisition, stale reclamation and release serialize inside the synchronous config-mutation transaction, released before the async callback. Failed metadata writes close their descriptor and clean only a matching owned path. + compare-and-delete against non-cooperating writers. Cooperating acquisition, stale reclamation and release serialize inside the synchronous config-mutation transaction, released before the async callback. Release keeps its descriptor open through identity comparison and any unlink, then closes it. Failed metadata writes remove only a matching owned path after successful coordination; unknown identity, failed probes or unavailable coordination retain the path for stale recovery. ## Sidecars, management, and UI diff --git a/structure/runtime.md b/structure/runtime.md index 97cc074a09..b8a3ef1b4f 100644 --- a/structure/runtime.md +++ b/structure/runtime.md @@ -1,6 +1,6 @@ # Runtime -OAuth refresh coordination follows the [refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a fresh unreadable lock remains held, and release requires matching descriptor identity. A failed path-identity probe preserves the refresh callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. +OAuth refresh coordination follows the [refresh-lock identity contract](catalog.md#accounts-namespaces-and-pool-rotation): a fresh unreadable lock remains held, and release requires matching descriptor identity. A failed path-identity probe preserves the refresh callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; release keeps the descriptor open through identity comparison and any unlink, then closes it. Failed metadata writes remove only a matching owned path after successful coordination; unknown identity, failed probes or unavailable coordination retain the path for stale recovery. Async refresh work holds no metadata transaction. The configuration-only [plaintext V2 contract](subagents.md#plaintext-v2-agent-messages) is scoped to canonical ChatGPT Responses forwarding; other source-area behavior described here is unchanged. diff --git a/structure/subagents.md b/structure/subagents.md index 44fa3cd86b..2e81e6c276 100644 --- a/structure/subagents.md +++ b/structure/subagents.md @@ -1,6 +1,6 @@ # Subagents And Multi-Agent Surface -Concurrent refreshes triggered by independent agent work share the [credential refresh-lock contract](catalog.md#accounts-namespaces-and-pool-rotation); unknown lock identity remains available for stale recovery rather than immediate removal, and a failed path probe cannot mask the callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; failed metadata writes close and clean only their owned file, and async refresh work holds no metadata transaction. +Concurrent refreshes triggered by independent agent work share the [credential refresh-lock contract](catalog.md#accounts-namespaces-and-pool-rotation); unknown lock identity remains available for stale recovery rather than immediate removal, and a failed path probe cannot mask the callback outcome. Cooperating lock metadata changes serialize through the existing SQLite mutation transaction; release keeps the descriptor open through identity comparison and any unlink, then closes it. Failed metadata writes remove only a matching owned path after successful coordination; unknown identity, failed probes or unavailable coordination retain the path for stale recovery. Async refresh work holds no metadata transaction. ## Plaintext V2 agent messages diff --git a/tests/codex-integration/codex-account-store.test.ts b/tests/codex-integration/codex-account-store.test.ts index 8407e1b597..a71db9b875 100644 --- a/tests/codex-integration/codex-account-store.test.ts +++ b/tests/codex-integration/codex-account-store.test.ts @@ -670,6 +670,46 @@ describe("codex-account-store CRUD", () => { unlinkSync(`${lockPath}.reclaimed`); }); + test.each([false, true])("refresh release prevents inode reuse before comparison (callback failure=%s)", async (callbackFails) => { + const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); + const key = `release-inode-reuse-${callbackFails}`; + const path = join(TEST_DIR, `codex-refresh-${createHash("sha256").update(key).digest("hex").slice(0, 32)}.lock`); + const originalFstat = fs.fstatSync; + const originalStat = fs.statSync; + let fd: number | undefined; + let owned: ReturnType | undefined; + let openDuringComparison = false; + const descriptor = spyOn(fs, "fstatSync").mockImplementation((...args: Parameters) => { + fd = args[0]; + owned = originalFstat(...args); + return owned; + }); + const probe = spyOn(fs, "statSync").mockImplementation((...args: Parameters) => { + if (args[0] === path && fd !== undefined && owned) { + try { originalFstat(fd); openDuringComparison = true; } catch { /* Descriptor closed early. */ } + // Model an allocator reusing the unlinked owner's inode only after its last fd closes. + // Holding that fd alive must prevent this ABA regardless of the host filesystem. + if (!openDuringComparison) return owned; + } + return originalStat(...args); + }); + const failure = new Error("original refresh failure"); + try { + const pending = withCodexRefreshFileLock(key, new AbortController().signal, async () => { + unlinkSync(path); + writeFileSync(path, "successor"); + if (callbackFails) throw failure; + return "refreshed"; + }); + if (callbackFails) await expect(pending).rejects.toBe(failure); + else expect(await pending).toBe("refreshed"); + expect(openDuringComparison).toBe(true); + expect(readFileSync(path, "utf8")).toBe("successor"); + expect(fd).toBeDefined(); + expect(() => originalFstat(fd!)).toThrow(); + } finally { descriptor.mockRestore(); probe.mockRestore(); } + }); + test("refresh release preserves the path when descriptor identity cannot be read", async () => { const { withCodexRefreshFileLock } = await import("../../src/codex/account-store"); const lockKey = "unknown-owner";