Skip to content
Merged
39 changes: 33 additions & 6 deletions src/codex/account-store.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -654,7 +654,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;
Comment thread
luvs01 marked this conversation as resolved.
Comment thread
luvs01 marked this conversation as resolved.
Comment thread
luvs01 marked this conversation as resolved.
} catch {
return false;
}
}
}

Expand Down Expand Up @@ -690,11 +697,31 @@ export async function withCodexRefreshFileLock<T>(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: 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 {
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;
}
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions structure/catalog.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ 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 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.
- **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
Expand Down
2 changes: 2 additions & 0 deletions structure/codex-home.md
Original file line number Diff line number Diff line change
@@ -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. Failed path-identity probes leave the lock for stale recovery and preserve the refresh callback outcome.

## Codex home

`src/codex/paths.ts` resolves Codex state from `CODEX_HOME` when set and valid, otherwise from
Expand Down
2 changes: 2 additions & 0 deletions structure/config.md
Original file line number Diff line number Diff line change
@@ -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 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.

Expand Down
2 changes: 1 addition & 1 deletion structure/gui-and-management-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -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, 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:<port>`, or `http://127.0.0.1:<management port>` 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
Expand Down
2 changes: 2 additions & 0 deletions structure/ops/docs-and-release.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# 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.

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.

Expand Down
10 changes: 8 additions & 2 deletions structure/providers/openai-tiers.md
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,12 @@ 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. 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.

## Sidecars, management, and UI

Expand Down Expand Up @@ -560,8 +566,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
Expand Down
2 changes: 2 additions & 0 deletions structure/runtime.md
Original file line number Diff line number Diff line change
@@ -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. 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.

Expand Down
2 changes: 2 additions & 0 deletions structure/subagents.md
Original file line number Diff line number Diff line change
@@ -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, and a failed path probe cannot mask the callback outcome.

## Plaintext V2 agent messages

`src/responses/plaintext-v2-agent-messages.ts` owns the experimental, configuration-only
Expand Down
115 changes: 114 additions & 1 deletion tests/codex-integration/codex-account-store.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
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 * 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";
import { setAsyncIcaclsRunnerForTests, setIcaclsRunnerForTests } from "../../src/lib/windows-secret-acl";
Expand Down Expand Up @@ -624,6 +625,118 @@ 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("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<typeof fs.fstatSync>) => {
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();
}
});

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<typeof fs.statSync>) => {
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,
Expand Down
Loading