openab-agent: multi-vendor OAuth ADR + cross-process auth.json locking (codex + MCP)

[brettchien]

What problem does this solve?

openab-agent's auth.json is a shared, multi-writer credential file with an unlocked read-modify-write, and openab-agent runs one process per Discord thread. So ordinary concurrent usage = concurrent processes refreshing the same OAuth token → refresh-token-rotation reuse → worst case OAuth 2.1 §10.4 token-family revocation = fleet-wide logout. API-key users never hit this; OAuth adoption (PR #1187, Anthropic) is what activates it.

This PR lands the ADR that sets the multi-vendor OAuth pattern (so every future provider stops hand-rolling its own flow) and the concurrency fix for the release-blocker-class storage race — for both the codex tenant and the MCP CredentialStore.

Closes #

Discord Discussion URL: https://discord.com/channels/1490643539682529300/1519372543377539173

At a Glance

auth.json  (multi-tenant: codex / anthropic-oauth / mcp:<server>×N, one process per Discord thread)

 (a) file integrity — ONE global lock          (b) refresh-token rotation — ONE lock per tenant
 ┌─ with_auth_locked(path, f) ──────────┐       ┌─ lock_tenant_refresh(path, tenant) ───────────┐
 │ flock EX  auth.json.global.lock       │       │ try-lock  auth.json.refresh.<tenant>.lock     │
 │ re-read → f(&mut map) → gc_pending →   │       │  + async backoff + 10s timeout (never wedge)  │
 │ atomic tmp+rename                      │       │ held ACROSS the network refresh               │
 └───────────────────────────────────────┘       └───────────────────────────────────────────────┘
   ▲ save_tokens (codex)                            ▲ get_valid_token / force_refresh (codex)
   ▲ McpCredentialStore::save / clear (MCP)         ▲ resolve_oauth_dial → get_access_token (MCP)
   = writers MERGE, never lost-update                = exactly ONE network refresh per tenant; the
                                                        lock-loser re-load()s the winner's token and
                                                        skips its own refresh (no RT_old reuse)

Prior Art & Industry Research

OpenClaw: API keys + subscription OAuth; stores per-profile {access,refresh,expires,accountId} and treats the profile file as a token sink refreshed under a file lock — direct corroboration for the locked-RMW decision.

Hermes Agent (NousResearch): PROVIDER_REGISTRY dataclasses declare each provider's auth type + URLs + env vars behind one resolve_runtime_provider() entry point; auth.json guarded with fcntl/msvcrt file locks — corroborates both the registry shape (OAuthVendor) and the file-lock decision.

Other references:

• Pi (earendil-works/pi) — ported flow for feat(native-agent): Anthropic OAuth (Claude Pro/Max) login for openab-agent #1187; CLAUDE_CODE_OAUTH_TOKEN (#3591); Kiro/Cursor/xAI provider extensions.
• rclone — rcloneEncryptedClientSecret + runtime obscure.MustReveal(): precedent for the ADR §9 Q2 encode-at-rest of a non-confidential bundled OAuth secret (scanner-evasion, explicitly not encryption).
• Antigravity (agy) OAuth ecosystem — GitHub survey 2026-06-24 (≥326 repos hardcode the same client_id/secret; 99/107 sampled commit it plaintext) grounding the agy go/no-go + secret-storage decisions.

Proposed Solution

ADR (docs/adr/openab-agent-oauth.md): a two-axis model — auth (OAuthVendor descriptor + a shared driver on the official oauth2 crate) kept orthogonal to inference (one provider per wire format) — plus a 14-variant vendor feasibility matrix, the CLAUDE_CODE_OAUTH_TOKEN env route, the /auth two-step UX (per-user-keyed PKCE pending state), and the storage invariant below.

Implementation (this PR):

• with_auth_locked — global flock(2) on an auth.json.global.lock sidecar across re-read → mutate → atomic write. All writers funnel through it (codex save_tokens + MCP McpCredentialStore::save/clear), so they merge onto the latest on-disk state instead of lost-updating.
• lock_tenant_refresh — per-tenant flock (non-blocking try + async backoff + 10s timeout) held across the network refresh, with a double-checked re-read, so concurrent processes do exactly one real refresh per tenant. Codex uses it in get_valid_token/force_refresh; MCP uses it in resolve_oauth_dial around get_access_token().
• flock(2) on sidecar files (kernel auto-releases on fd close / process death — no stale lock); libc::flock under cfg(unix), non-unix no-op.
• ADR §7 pending-entry GC: created_at + a 15-min sweep inside with_auth_locked.

Why this approach?

• Two locks, two hazards. The global lock fixes file lost-updates; the per-tenant lock fixes RTR revocation. A single global lock held across the network refresh would serialize one tenant's slow refresh in front of every other tenant (head-of-line blocking MCP servers / Codex), so refresh I/O is held under the per-tenant lock only.
• flock(2) over a sentinel lockfile — the kernel releases it on process death, so a crashed holder never wedges the file (no orphan cleanup / manual timeout).
• MCP single-flight without an rmcp fork. rmcp's CredentialStore exposes no pre-refresh hook, but openab drives the MCP refresh explicitly in resolve_oauth_dial; rmcp's get_access_token re-load()s auth.json and skips the network refresh when the token is already fresh, so the per-tenant lock + rmcp's own fresh-load gives cross-process single-flight with no extra double-check code.
• Known limitation: rustix is not in-tree (the ADR text was optimistic), so this uses libc::flock directly.

Alternatives Considered

• In-process Mutex / tokio single-flight — useless across the per-thread processes.
• Sentinel lockfile (create→delete) — deadlocks if a holder dies; flock(2) auto-releases.
• Refresh outside the lock, re-read on commit (an earlier ADR draft) — rejected: every process has already sent the refresh with the same RT_old before it commits, so RTR still fires.
• Forcing everything through rmcp CredentialStore — lossy; the shared layer sits below both stores (file RMW).

Validation

Rust changes:

• cargo check passes
• cargo test passes (192 passed, 0 failed, 11 ignored) — incl. new with_auth_locked_merges_concurrent_tenants_no_lost_update and with_auth_locked_gcs_stale_pending_but_keeps_fresh_and_tokens
• cargo clippy -- -D warnings clean
• cargo fmt --check clean

All PRs:

• Manual testing — ran the full gate from openab-agent/ (the crate is built standalone, matching ci-openab-agent.yml). Reviewed end-to-end with a second agent across two ADR-review rounds and two code-review rounds; all findings (RTR refresh race, per-user PKCE keying, pending GC, portable WouldBlock, MCP cross-process refresh) addressed and re-verified.

[brettchien]

All review findings addressed and re-verified — CI fully green (build-builder, check, 16 smoke-test variants), chaodu-agent LGTM x2. No open items on my side; handing off for maintainer review.

[brettchien]

All review feedback addressed across three rounds — chaodu-agent's 8 findings (ADR/code drift, CI exit-code, fail-open→fail-closed) and the follow-up fail-closed lock-timeout bound. Second-agent review (Mira) confirms all points. CI fully green. Handing off for maintainer review.

[chaodu-agent]

LGTM ✅ — Well-engineered cross-process locking for auth.json that fixes a real release-blocker-class OAuth RTR race, paired with a thorough ADR.

What This PR Does

Fixes a latent concurrency bug where multiple openab-agent processes (one per Discord thread) could concurrently refresh the same OAuth token, triggering OAuth 2.1 §10.4 token-family revocation (fleet-wide logout). Lands both the ADR documenting the multi-vendor OAuth pattern and the implementation of the two-lock file-locking scheme.

How It Works

Two complementary flock(2) locks on sidecar files:

1. Global lock (auth.json.global.lock) — held across the re-read → mutate → atomic-write cycle in with_auth_locked, so concurrent writers (codex save_tokens + MCP CredentialStore) merge instead of lost-updating.
2. Per-tenant lock (auth.json.refresh.<tenant>.lock) — held across the network refresh (lock_tenant_refresh), with non-blocking try + async backoff + bounded timeout. Fail-closed on timeout (surface retryable error rather than risk double-RT presentation).

CI workflow improvements: workspace isolation fix, separated build step, robust ACP smoke test with proper exit-code propagation.

Findings

#	Severity	Finding	Location
1	🟢	Correct two-lock separation avoids both lost-update and RTR hazards without head-of-line blocking	auth.rs:227-485
2	🟢	Fail-closed timeout design with bounded refresh is properly reasoned — a timeout genuinely signals an abnormal state	auth.rs:380-420
3	🟢	MCP integration correctly uses OauthDialError::Transient to avoid tripping the circuit breaker or forcing re-login on lock contention	mcp/runtime.rs:646-680
4	🟢	Pending-entry GC (ADR §7) is a good defensive measure against stale verifiers accumulating	auth.rs:310-320
5	🟢	CI smoke test hardening: exit-code capture + proper error propagation fixes silent timeout/crash failures	ci-openab-agent.yml

What's Good (🟢)

• Two-lock design is the right call. A single global lock held across network I/O would serialize all tenants — the per-tenant lock neatly avoids that head-of-line blocking while still preventing RTR.
• flock(2) on sidecars — kernel auto-release on fd close / process death eliminates stale lock concerns. Sidecar placement is correct since atomic rename swaps the inode of auth.json itself.
• Double-check pattern in get_valid_token — re-reads after acquiring the tenant lock so the lock-loser adopts the winner's token. MCP path gets this free from rmcp's initialize_from_store() reload.
• Fail-closed with bounded timeout — REFRESH_LOCK_TIMEOUT is derived from REFRESH_HTTP_TIMEOUT × MAX_REFRESH_ROUND_TRIPS + margin, making the invariant (lock-hold always shorter than waiter's deadline) provable and drift-resistant.
• Non-unix no-op with one-time warn — pragmatic given the de-facto unix-only deployment.
• Test coverage — with_auth_locked_merges_concurrent_tenants_no_lost_update, GC test, and the fail-closed contention test directly exercise the core invariants.
• ADR is comprehensive — the vendor feasibility matrix, prior art survey (OpenClaw, Hermes, rclone), and §9 decisions give future contributors clear context for incremental vendor additions.
• CI improvements — the workspace isolation (printf '\\n[workspace]\\n' >> Cargo.toml) matches the Dockerfile.unified pattern, and the reworked smoke test no longer silently swallows timeout/crash exits.

Baseline Check

• PR opened: 2026-06-24
• Main already has: auth.json with atomic tmp+rename in write_auth_file, but no cross-process locking — save_tokens and McpCredentialStore::save both do unlocked read-modify-write
• Net-new value: (1) Global file-integrity lock preventing lost-updates between codex and MCP writers; (2) Per-tenant refresh serialisation preventing RTR family revocation; (3) ADR establishing the multi-vendor OAuth pattern; (4) CI robustness improvements

[chaodu-agent]

LGTM ✅ — Cross-process auth.json locking is well-engineered and correctly addresses the RTR race condition. ADR is thorough. Approving.