fix(codex): serialize native-main refresh on the CODEX_HOME claim (rebase of #3112) - #3183
Conversation
Reimplements #3000 (author @MarcTCruz) for the #2999 lock-scope half. The refresh lock is keyed on the grant fingerprint and lives under OPENCODEX_HOME (src/codex/account-store.ts:420-422, via getConfigDir). The file it protects is auth.json under CODEX_HOME, which every OpenCodex install on the machine shares regardless of its own home. Two proxies with distinct OPENCODEX_HOMEs therefore took two unrelated locks and refreshed the one credential concurrently; the loser published its rotated grant over the winner's and the provider then rejected it. The outer lock is now withNativeMainExclusiveClaim on resolveNativeProfileContext(), which is the CODEX_HOME coordination the other native-main paths already use (.opencodex-native-main.claim.sqlite). No new primitive, no FFI. Why not #3000's approach: it introduces src/lib/atomic-file-preserving-replace.ts, which dlopens libc.so.6 / libSystem.B.dylib / kernel32.dll for renameat2 / renamex_np / ReplaceFileW and throws "No rename fallback is safe" on anything else. musl names its libc libc.so, not libc.so.6, so publication would crash on Alpine. It also throws MainAccountTokenRefreshError("transient") on an aborted signal BEFORE persistRefreshedMainAuthJson, so a late cancel discards a grant the provider has already rotated -- the only live refresh token, dropped. The existing check-then-rename guard is left as it is: atomicWriteFile with assertMainAuthJsonSnapshotUnchanged in both beforeRename and validateBeforeRename refuses rather than overwrites, and the covering test (refuses to overwrite an external auth writer after refresh) already passes. Lock order is claim (machine-wide) then fingerprint lock (per-grant), never the reverse: two processes holding different fingerprint locks and then reaching for the same claim would deadlock. Mutation-checked: dropping the claim wrapper fails exactly the new test (3 pass / 1 fail), restored to 4/0. Closes #2999.
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaa8b84634
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await withNativeMainExclusiveClaim( | ||
| resolveNativeProfileContext(), |
There was a problem hiding this comment.
When Codex integration startup is disabled (for example, clientIntegrations.codex: false or runtimeRole: "hub"), proxy routes can still use the native-main credential without the startup owner gate. On WSL with CODEX_HOME under /mnt/<drive> or on a Windows UNC home, nativeMainOwnerFilesystemSupported rejects this claim, so the new unconditional wrapper throws NATIVE_MAIN_CLAIM_UNAVAILABLE before invoking the refresh callback and every expired credential returns 503 indefinitely. Preserve the prior refresh path on these supported CODEX_HOME layouts or provide a coordinator that works on them.
AGENTS.md reference: src/AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
1 participant
Summary
Maintainer rebase of #3112 onto current
dev— all four commits cherry-picked with author credit preserved, no conflicts.Serializes native-main refresh on the
CODEX_HOMEclaim so two OpenCodex instances sharing one Codex home cannot both publish a refreshed credential. Addresses the lock-scope half of #2999.Scope note, stated plainly: this does not close #2999. That issue describes two races. This is the serialization half; the publication/overwrite race remains carried by the existing refuse-rather-than-overwrite check. #2999 stays open with that scope recorded.
Review threads
The
CHANGES_REQUESTEDstate came from bot reviewers against commita6014b9688, which is four commits behind this head. The one substantive finding was resolved by a later commit on the same branch:src/codex/main-account.tsnow passes{ waitMs: 30_000, signal }into the claim wait, wheresignalisAbortSignal.any([dependencies.signal, refreshTimeout])— the caller's abort combined with the refresh timeout. That is exactly what the review asked for, delivered by "abort contended native-main refresh claims".Verification
Exact head
eaa8b8463:bun test ./tests/codex-main-account-refresh.test.ts— 5 pass, 0 failbun test ./tests/native-main-claim.test.ts— 8 pass, 0 failbun test ./tests/responses-native-main-refresh.test.ts— 8 pass, 0 failbun test ./tests/responses-compaction-routing.test.ts— 47 pass, 0 failOne pre-existing parallel flake, verified not ours. Running those files together with
tests/codex-auth-context.test.tsin one invocation failsCodex auth context > guardian and quota consumers back off and retry a busy credential refresh without reauthat ~4.9s — a file-lock contention timeout. The identical failure reproduces on cleanorigin/devwith the same file set and the same ~4.9s duration, and every file passes alone. It is a suite-parallelism artifact in a credential-refresh test that takes real locks, not fallout from this diff.Checklist
devdevrather than waived by assertion