Skip to content

Port upstream 0.69.0: retry Codex auth.json publication races - #688

Open
Finesssee wants to merge 3 commits into
port/upstream-0.69.0from
port/micro-0.69.0-codex-auth-publication-retry
Open

Port upstream 0.69.0: retry Codex auth.json publication races#688
Finesssee wants to merge 3 commits into
port/upstream-0.69.0from
port/micro-0.69.0-codex-auth-publication-retry

Conversation

@Finesssee

@Finesssee Finesssee commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

CodexApi::load_credentials (rust/src/providers/codex/api.rs) now tolerates the Codex CLI publishing auth.json while we read it. A missing, unreadable, or partially published (parse failure) file is re-read up to twice, 50 ms apart, before the error is reported. The delay is an async tokio::time::sleep, so dropping the fetch future cancels it.

After the retries the error categories stay separate: missing -> NotInstalled, unreadable -> Other, malformed/incomplete -> Parse. A stale or gated credential (AuthRequired) is not retried. Nothing is written and the credential cache TTL/mtime semantics are unchanged. The single-read logic moved unchanged into load_credentials_once.

Upstream reference

Ported / Deferred

  • Ported: the auth.json read retry (half (a) of upstream item Fix setup-windows.ps1 for PowerShell 5.1 by adding UTF-8 BOM #6).
  • Deferred to the separate codex-plan-change-baseline micro PR: discarding the previous plan's quota baseline on a plan change (half (b)).
  • Not ported: upstream also rereads native credentials that are inside the 5-minute renewal window during fetch (needsRefresh on codexHome). Local behavior treats an in-window external credential as AuthRequired and this PR keeps "stale unchanged" per the audit spec. The upstream isAvailable path has no local counterpart. The other refactors in fix(codex): retry auth reads and reset quota baselines on plan changes steipete/CodexBar#4088 (result construction, backfill helpers, prepareCredentialsForUsage signature) are Swift-internal and not applicable.

Validation

Run with cargo +1.98.0, E-core pinned, slot-1 target dir.

  • cargo fmt --all: clean.
  • cargo clippy --workspace --all-targets -- -D warnings: pass.
  • cargo test -p codexbar credential_retry: 7 passed, 0 failed (published-after-60ms missing file, torn-then-replaced file, missing -> NotInstalled, unreadable -> Other, malformed -> Parse, stale not retried, cancellable delay).
  • cargo test -p codexbar providers::codex: 55 passed, 0 failed.
  • Full cargo test -p codexbar not run: no shared code touched (core, settings, cost_scanner, spend_contract).
  • Note: the shared port-audit scratch dir (IMPLEMENT.md, ecargo.sh) was wiped mid-run; cargo was run through an equivalent local wrapper (start /affinity FFFF0000 /belownormal).

Affected areas

  • Rust backend (Codex provider)
  • Tauri shell / React UI / tray / settings / float bar

UI proof

Not applicable.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 0a3d000c-a9d4-4f2c-a3c4-f63c0345c250

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Finesssee

Finesssee commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Reviewed by Codex gpt-6-luna (xhigh); verified and validated by Claude

Thermo-nuclear review of PR #688 (Codex auth.json publication retry): 1 finding.

  • Medium rust/src/providers/codex/api.rs: Path::exists() turns filesystem access errors into false, so an unreadable credentials path was reported as NotInstalled, and a file removed between the existence check and the read escaped that classification. Metadata and read errors now share one mapper that keeps NotFound as NotInstalled (including the custom-backend guidance) and reports other I/O failures as Other.

@Finesssee

Finesssee commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up: the finding is fixed at the new head. Nothing left open.

Commands run: cargo +1.98.0 fmt --all --check, cargo +1.98.0 clippy --all-targets -- -D warnings on rust/ and apps/desktop-tauri/src-tauri, cargo +1.98.0 test --lib providers::codex (55 passed).

@Finesssee
Address thermo review
9d70007
@Finesssee
Port upstream 0.69.0: reread stale Codex credentials during publication
4b93d3d 
Upstream loadCredentials(retryStale: true) also rereads a native credential
that needs renewal, because the Codex CLI may be publishing its renewal.
The usage path now repeats every failed read, including the gate's stale
AuthRequired, up to three reads 50 ms apart. The last error keeps its
category, so unchanged stale credentials still need the owner's renewal.

The retry loop is a separate function so the tests can count reads like
upstream's injected reader. The tests run on a paused clock and publish
between reads, so they no longer race a writer thread:
- an owner publication in progress (missing, partial, incomplete, expired,
  near expiry) is picked up on the second read and keeps its workspace;
- retries are bounded at three reads and keep the final error category;
- an unpolled or dropped load stops reading.

The external-OAuth gate now loads settings only when the opt-in decides
the outcome (no refresh provenance). Repeated reads no longer reload the
host settings each time.
@Finesssee

Finesssee commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Lane B review: fixes at 4b93d3d

Reviewed the full diff against its base port/upstream-0.69.0 and against upstream steipete#4088 in v0.69.0, specifically CodexOAuthFetchStrategy.loadCredentials in CodexProviderDescriptor.swift and the credential-publication tests.

Defect: a stale credential was never reread (fixed in 4b93d3d). Upstream's usage path calls loadCredentials(retryStale: true). It rereads when a read throws, and also when the codexHome credential needsRefresh, because the CLI may be publishing its renewal at that moment.

On Windows, that source is CODEX_HOME\auth.json with a refresh token. When its JWT is inside the renewal window, the gate reports AuthRequired. This PR retried only NotInstalled, Other and Parse, so the gate's AuthRequired returned at once. A test, stale_external_credentials_are_not_retried, locked that in.

Now every failed read is repeated, up to three reads 50 ms apart, which matches upstream's retriesRemaining = 2. After the last read the error keeps its category, so unchanged stale credentials still need the owner's renewal. Nothing is redeemed or written, and the credential cache is unchanged.

Tests rewritten to be deterministic. The retry loop is now reread_during_owner_publication, so tests can count reads the way upstream's injected reader does. They run on tokio's paused clock and publish the file between reads, so they no longer race a writer thread:

  • usage_read_retries_an_owner_publication_in_progress: missing, unreadable, partial, incomplete, expired and near-expiry files are each replaced by a fresh publication after the first read. The second read picks it up with its account id, and the file is not modified.
  • usage_read_retries_are_bounded_and_keep_the_final_error: exactly three reads, and the final error category is kept (NotInstalled, Other, Parse, AuthRequired).
  • load_credentials_rereads_before_reporting_the_final_error: for missing and expired files, the wait is two retry delays.
  • load_credentials_returns_usable_credentials_without_waiting: a usable credential returns after one read with no delay.
  • dropping_the_load_cancels_the_retry_delay: an unpolled load reads nothing, and a load dropped during its delay stops after one read. This mirrors upstream's Task.checkCancellation().

Gate loads settings lazily. enforce_external_oauth_gate reloaded host settings on every read. The opt-in only changes the outcome when the credential has no last_refresh. It is now loaded only in that case, so repeated reads do not reload settings.json each time. The outcome is identical.

Not ported

  • Upstream "OAuth availability retries a partial credential publication". Windows has no separate availability probe (isAvailable with retryStale: false), so fetch_usage is the only credential read.
  • The docs/codex.md and docs/codex-oauth.md text. Windows has no counterpart page.
  • Upstream's eight-day last_refresh staleness rule. It is absent on Windows before this PR too. The JWT expiry stays the authority.

Validation (Windows, toolchain 1.98.0)

  • cargo fmt --all --check: pass
  • cargo clippy --workspace --all-targets -- -D warnings: pass
  • cargo test -p codexbar --lib providers::codex::api: 40 passed
  • cargo test -p codexbar: 2165 passed, 0 failed, 1 ignored
  • cargo test -p codexbar-desktop-tauri: 461 passed, 1 failed. The failure is commands::tests::bootstrap_payload_exposes_every_provider_variant, which reads host settings where a deprecated provider is enabled. It fails the same way on main, and this PR does not touch it.

UI proof: not applicable. The change is to backend credential reads, with no visible surface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Finesssee