feat(buzz-agent): add DatabricksAuthCoordinator single-flight OAuth

[wpfleger96]

What

Consolidates all Databricks OAuth acquisition behind one coordinator on PkceOAuthTokenSource. Every entry point — the four TokenSource methods (bearer, bearer_no_browser, refresh_now, interactive_login) plus the public acquire_with_intent — routes through a single acquire()/acquire_locked() core that owns browser and cooldown policy.

Before this, acquisition logic was scattered across those methods with no coordination: concurrent callers (Desktop discovery, the saved-agent model picker, managed-runtime inference) could each pop their own browser, and a just-denied attempt would immediately re-prompt on the next passive read.

How

• Intent policy. AuthIntent::{Auto, UserInitiated, Headless} decides whether a caller may open a browser and whether it honors the cooldown. Headless never browses; Auto browses but honors an unexpired cooldown; UserInitiated browses and bypasses+clears the cooldown.
• Two-layer single-flight per cache key. An in-process registry (INFLIGHT) coalesces same-key, same-intent callers onto one leader's attempt before the file lock. The slot key is (lock_path, AuthIntent), so a UserInitiated sign-in never inherits an Auto leader's result. A joined result is revalidated against the waiter's own contract. Across processes, callers serialize on a flock-based advisory lock and share success through the on-disk cache. RAII Drop releases both lock and leader slot.
• Joiner credential-state reconciliation. SlotPublish carries the full CachedToken on success. Each joiner reconciles its own independent state cell under state.lock().await before returning: adopt when absent, expired, or matching the joiner's rejected credential; preserve any distinct newer usable credential. On a matching shared failure, neutralize the joiner's in-memory rejected entry under lock via expire_rejected_memory — durable disk mutation is reserved for acquire_locked under the cross-process file lock. Without this, a joining source's state remains stale or empty and subsequent plain bearer() calls resurface the rejected or absent credential.
• Validate-before-persist boundary. finish() is the candidate-token persistence boundary for refresh and browser results. Before a token is written to cache or the cooldown is cleared, a bearer equal to the caller's rejected bytes yields a typed failure.
• Token neutralization. When acquire_locked enters with rejected = Some(bytes), it calls expire_rejected() under the state lock before any cache check.
• Cross-process failure single-flight. An AttemptRecord sidecar records a monotonically-increasing generation, intent, result code, and SHA-256 digest of the completing caller's rejected token. Adoption is temporal (pre-queue snapshot predates current generation) and digest-matched.
• Typed outcomes. AuthError with stable code()/from_code() replaces display-text matching.
• Durable cooldown sidecar. Every failed browser attempt is recorded next to the cache key. 5-minute expiry.
• Windows disk persistence disabled. On non-Unix platforms, persist() is a no-op. Lock, cooldown, and attempt sidecars are active on all platforms. Tests that seed or assert on the on-disk token cache are #[cfg(unix)]-gated.
• Injected browser opener invoked while the localhost callback listener is live.

Tests

• crates/buzz-agent/tests/databricks_auth_coordinator.rs: browser/cooldown/classification acceptance matrix with a scripted BrowserOpener and stub OIDC provider. P1 regressions exercise the full finish() → acquire_locked() → acquire_leader() → LeaderGuard::complete() → joiner wiring:
  • test_inprocess_joiner_reconciles_stale_state_after_shared_success (Unix): two real sources both loaded locally-fresh-but-rejected X; after shared success Y, subsequent plain bearer() on both returns Y, not X.
  • test_inprocess_joiner_neutralizes_rejected_on_matching_shared_failure (Unix): B's matching rejected X is force-expired in memory after shared RefreshRejected; subsequent read cannot return X.
  • test_inprocess_joiner_populates_empty_state_no_second_acquisition (non-Unix): empty A/B join a browser success; B's subsequent headless read returns Y without a second browser (no disk fallback on non-Unix exposes the regression).
  • test_crossprocess_userinitiated_waiter_adopts_predecessor_denial (snapshot-marker barrier replacing an earlier sleep for deterministic generation ordering).
• auth.rs in-crate tests: lock-primitive edges, disk-recheck on shared failure (#[cfg(unix)]), and:
  • test_joiner_reconciliation_blocked_until_state_lock_released: deterministic direct-poll proof that awaited reconciliation requires state.lock().await. The test task holds B's state mutex and manually polls a pinned real acquire() future — Poll 2 (slot published, mutex still held) must return Pending because lock().await blocks; with try_lock instead, Poll 2 returns Ready, failing the assertion.
  • test_joiner_preserve_distinct_newer_credential: deterministic direct polling parks B at slot.wait(), then writes Z directly into B's state in the same task, then publishes Y and awaits completion. B must return Y but leave state == Z. Mutation check: unconditional adoption overwrites Z with Y, failing the state assertion.
  • test_joiner_shared_failure_recovers_disk_replacement (Unix): the matching-failure joiner enters the recovery branch — after expire_rejected_memory (in-memory, empty state no-op) it reads a sibling-written disk replacement via usable_from_disk and returns it. Mutation check: removing the usable_from_disk recovery branch returns Err(RefreshRejected).
  • test_joiner_failure_does_not_write_disk (Unix): byte-for-byte disk-invariance regression — a matching-failure joiner calls expire_rejected_memory and must not touch the on-disk cache. An independent process C may write a valid replacement between A's failure and B's reconciliation; this guard ensures B's unfenced in-memory neutralization cannot overwrite C's concurrent disk write. Mutation check: reverting to expire_rejected rewrites the file (expires_at = 0), changing the bytes and failing the assertion.
  • test_lock_timeout_leaves_cooldown_sidecar_byte_for_byte_untouched: a waiter past its deadline returns LockTimeout before entering acquire_locked; the cooldown sidecar bytes are unchanged. Documents a pre-lock limitation: LockTimeout callers do not neutralize state or sidecars.

Scope / follow-ups

• Runtime 401 handling is deferred. This PR owns acquisition single-flight and policy.
• Desktop wiring is Phase 2 (not in this PR's boundary). Confined to crates/buzz-agent/.
• Windows DACL is a follow-up once the windows-sys binding is available.

Stack

Built on #5534 (hayt/databricks-oauth-cache-hardening), now merged. Retargeted to main.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes required at e58204b88601af14460f17940d2bc3d008f78b49:

1. [P1] Preserve UserInitiated retry semantics across in-process coalescing. Auto and UserInitiated use the same (lock_path, may_open_browser = true) slot, and a joiner returns the leader’s result without applying its own intent (auth.rs lines 675–699). If an explicit user action joins an Auto leader that returns an active cooldown at lines 802–807, the user receives the prior Denied/TimedOut result instead of clearing the cooldown and opening sign-in as UserInitiated promises. Key the slot by the relevant intent policy, or make a user-initiated joiner retry when it inherited an automatic cooldown result. Add the mixed-intent race test; the current tests cover same-intent coalescing and sequential cooldown bypass only.

2. [P1] Do not accept an expired replacement after a 401. In cached_hit, rejected = Some(t) considers any token whose bytes differ from t usable (lines 603–623), without checking is_expired. An expired in-memory or on-disk token B can therefore be returned as the presumed sibling replacement for rejected token A, skipping refresh for refresh_now and every public rejected-token acquisition. A replacement must differ from the rejected token and still be unexpired.

3. [P1] Separate authorization-code rejection from exchange infrastructure failure. The code-exchange path maps every non-success status, including 429 and 5xx, plus malformed 2xx JSON/token payloads, to ExchangeFailed (lines 1544–1556). That variant is terminal LlmAuth and cooldown-worthy, so a transient provider outage after callback is reported as a rejected code and suppresses automatic auth for five minutes. Classify transport/429/5xx and malformed success responses as NetworkUnavailable; reserve ExchangeFailed for an OAuth response that establishes the authorization grant was rejected. Mirror the refresh classifier’s status/body coverage in exchange tests.

I reviewed the public entry-point × intent × cache/refresh/browser/cooldown matrix, same-process and cross-process coordination, cancellation/deadline behavior, OAuth classification, and platform paths using pinned GitHub source only. I did not execute PR code.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes required at 4513d2385fcd4e09109c99756f83c309ea3346b9:

1. [P1] Include rejected-token identity in same-process single-flight. cached_hit correctly makes the caller's rejected bearer part of cache validity, but InflightKey contains only (lock_path, AuthIntent) and a joiner returns the leader's result without applying its own rejected value (auth.rs lines 656–707 and 1061–1132). This breaks the public 401/403 retry contract when concurrent requests reject different bearer generations. For example, while refresh_now(A) is refreshing, refresh_now(B) joins the same Headless slot; if the leader publishes B, the second request receives the exact token it just reported rejected and retries its provider call with known-bad credentials. The inverse can also inherit a terminal refresh/network result even though that caller's cached replacement was already valid. Include a non-secret digest of rejected in the slot key, or revalidate a joined result against the waiter's rejected value and rerun its own cache/acquisition policy. Add a deterministic concurrent different-rejected-values test proving a waiter never receives its rejected bytes.

The three blockers from the previous head are fixed: full AuthIntent now separates mixed cooldown policy, rejected replacements must differ and be unexpired, and exchange infrastructure failures remain NetworkUnavailable. I reviewed public entry points, cache/refresh/browser/cooldown transitions, same/cross-process coordination, cancellation/deadlines, OAuth classification, platform cache paths, and the test matrix using pinned GitHub source only. Exact-head CI is otherwise green; I did not execute PR code.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

[P1] Do not persist a bearer after proving it equals the caller’s rejected token.

The new equality guard runs in acquire_leader only after acquire_locked returns (crates/buzz-agent/src/auth.rs:786-810). Both successful live-token paths call finish first (:845, :906), and finish atomically saves the token to disk and memory before returning it (:921-930). If a provider reissues the exact bearer just rejected with 401/403, this call correctly returns RefreshRejected or NetworkUnavailable, but leaves that known-bad bearer cached as fresh. The next ordinary bearer() calls acquire(Headless, None) and cached_hit accepts it because no rejected identity is supplied (:605-623, :943-946). The runtime can therefore restore and send credentials this flow already proved unusable; a fresh process does the same from disk.

Validate the candidate before committing it, or explicitly invalidate/remove the persisted candidate on equality without destroying a still-valid concurrent replacement. Add lifecycle regressions for both sticky refresh and sticky browser exchange: after the rejected-aware acquisition fails, a following bearer() and a newly constructed token source must not return the rejected bytes.

The prior different-rejected-joiner race itself is fixed: joiners revalidate the published result and perform a bounded rerun. This remaining blocker is the persistence boundary after that rerun.

Read-only exact-head source review; no PR code was checked out or executed.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested at exact head 5cdc56bae2ece14fdacce7f8c0de064eaab5ffa6:

1. P1: rejected-token reissuance still leaves the original known-bad cache entry live. The new finish guard correctly refuses to persist a newly issued token equal to rejected, but it does not invalidate the existing cached token. In the real lifecycle, bearer() returns cached unexpired A, the provider rejects A, and refresh_now(A) receives sticky A from refresh. finish returns RefreshRejected before saving, leaving the original unexpired A in memory and on disk. The next plain bearer(None), or a fresh process, accepts A again. The new poison tests seed expired-seed while passing a different rejected value (sticky-token / sticky-browser), so they cannot detect this path. Add regressions that seed the same unexpired A passed as rejected, then prove the failure cannot be followed by a cache hit for A; invalidate conditionally so a concurrently persisted distinct replacement is preserved.

2. P1: the file lock serializes cross-process failures but does not single-flight them for UserInitiated or Headless. After process A fails and releases the lock, an already-waiting process B enters acquire_locked. A UserInitiated waiter clears A's just-written cooldown and opens a second browser; a Headless 401 waiter repeats the failed refresh. The process-local INFLIGHT slot cannot publish A's failure across processes. The current denial test only uses an Auto waiter, whose cooldown policy masks the gap; success shares through the cache. Preserve an attempt generation/outcome so callers already queued behind that generation can adopt failures while later explicit user retries still bypass cooldown, and add real two-process denial and failed-refresh regressions.

3. P1 security: Windows persistent OAuth cache files are not owner-only. The non-Unix read path accepts any cache ACL unchanged, and the non-Unix temp-file path creates cache/cooldown files with inherited default ACLs. These files hold access and refresh tokens; the implementation comments explicitly defer the owner-only DACL. On a permissive parent or pre-existing broad ACL, other local principals can read persisted credentials. Create and validate/repair an owner-only Windows DACL, or disable persistent OAuth caching there until that guarantee exists. The Unix path's O_NOFOLLOW, fd-based mode repair, 0600 creation, and atomic rename do not cover Windows.

The previous exact-head blocker about persisting the newly returned rejected token before validation is partly fixed by moving equality validation ahead of save; item 1 is the remaining full-lifecycle gap. CI's normal build/test/security gates are green; the Codex security-review job was cancelled. Review was read-only; no PR code was executed.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested on exact head 8af6afb3c8f9164dfd6cc765d5416f70c9d174b6.

1. P1: Do not share rejected-token-relative failures across different rejected bearers. The in-process slot key contains only (lock_path, intent) (crates/buzz-agent/src/auth.rs:780-840). If Headless caller A rejects X and refresh reissues X, finish returns RefreshRejected; concurrent caller B rejecting Y inherits that error, although the successful refresh proved the grant remains live and X does not equal B’s rejected value. The cross-process attempt record has the same problem because it stores intent/result but not rejected identity (:958-963, 1253-1262). Existing tests cover the inverse shared-success collision where the leader returns the joiner’s rejected bytes, not a leader-relative failure. Key rejection-relative outcomes by a non-secret rejected-token digest, or rerun joiners when the shared result may not satisfy their rejected identity; cover both in-process and cross-process transitions.

2. P1: An adopter must not create a new attempt generation. When a queued process adopts a predecessor’s terminal failure, acquire_locked immediately calls write_attempt again (:958-963). That records work which never ran. A caller arriving after the real attempt can snapshot generation 1, queue behind the adopter that advances it to 2, and incorrectly adopt the old failure; overlapping arrivals can relay it indefinitely. Return the adopted error without advancing the sidecar, and add a three-process regression where C arrives after A’s failure but while B is adopting it.

3. P1: The non-Unix memory-only policy breaks the cross-process success contract and its newly required Windows CI lane. persist is a no-op and read_private_cache refuses/deletes disk tokens on non-Unix (:512-527, 1578-1595), so a Windows waiter serialized by LockFileEx cannot consume the winner’s bearer and performs another refresh/browser flow. Exact-head Windows CI proves this: test_crossprocess_two_coordinators_race_to_one_grant_and_cache gets browser-token-2 versus browser-token-1, and the added auth-coordinator step fails 20 of 33 tests. Either provide secure Windows success handoff, or explicitly scope the product contract and Windows tests to invariants that remain true without persistence. Do not leave a required platform lane structurally red.

4. P2: Rejected-token neutralization is not fail-closed for a readable stale file. expire_rejected rewrites best-effort and then removes best-effort (:544-570). If both fail while the original token file remains readable, such as an owner-writable cache file in a now read-only directory where temp creation and unlink fail, a fresh process still serves the locally-unexpired bearer already proven dead. The regression substitutes a directory at the cache path, which the read path rejects independently, so it does not cover this case. Add a fallback that durably expires/removes the readable regular file or make subsequent loads reject it, with the actual double-failure regression.

The OAuth status classification, Unix no-follow/0600/atomic cache path, bounded lock/browser lifetimes, public intent routing, and prior rejected-token persistence fixes otherwise look coherent. The unrelated Desktop smoke layout failure is not attributed to this PR. Review was read-only; PR code was not executed.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Changes requested: one P2 regression

Reviewed head 81216c80c55772bee34ccf5f848ee913bb6b7a82 against base 4a9de1a3a121285ef475d630b2b5764044c02cde. This is a convergence-focused re-review of acquisition coordination. The prior full-credential joiner-retention/neutralization finding and process-denial snapshot-barrier finding are addressed. The new failure cleanup introduces the following durable-state regression.

P2: Keep joiner failure cleanup from writing the cache outside the auth-file lock

Location: crates/buzz-agent/src/auth.rs:937-940.

The matching-failure joiner holds only its own state mutex, but calls expire_rejected, which also reads and rewrites the shared disk cache (lines 587-605). The shared auth-file lock has already been released by the leader; it does not fence this joiner. The equality check and subsequent atomic rename are not a compare-and-swap.

Source-derived Unix interleaving:

1. A and B are independent same-key sources in one process, both recovering rejected X. A leads, expires disk X under the file lock, then fails. B receives the matching shared failure.
2. B reads the expired disk X inside expire_rejected; it still matches X, so B prepares another write. A later explicit acquisition C, in another process, holds the auth-file lock and successfully persists Y with its new refresh token.
3. B's unfenced rename commits after C's rename, replacing Y with its expired-X snapshot and old refresh token. The in-place/remove fallbacks are unfenced as well.

Impact: a successful sign-in loses its durable credential. Fresh sources miss Y, retry the old refresh token, and can fail headlessly or require another interactive sign-in when that old token is invalid. B's disk recovery check also misses the replacement it just overwrote. This is introduced by the new joiner helper call, not an inherited concern or Windows persistence follow-up.

Smallest safe repair: separate conditional in-memory invalidation from durable invalidation. Keep the joiner's local-state reconciliation, but ensure every credential-content write/removal remains owned by the existing cross-process lock; do not reuse the combined disk-mutating helper from a lockless joiner. For normal completed attempts the leader already handles disk invalidation. Do not assume every shared error proves that step ran: lock-acquisition failures return before it. Add a deterministic witness for failed A/joining B versus successful C that asserts C's full persisted credential survives and B cannot serve rejected X.

Closed findings and nonblocking coverage note

• Full CachedToken publication, awaited conditional adoption, matching rejected-memory invalidation, and preservation of distinct usable Z address the previous P1. Headless, Auto, and UserInitiated remain separated; non-Unix same-process retention is covered without requiring disk persistence.
• The worker snapshot marker is emitted after capturing the attempt generation and observed before releasing the predecessor. The earlier process-denial timing issue is closed. Unix disk-test gates and the Windows CI crash-release description are corrected.
• Nonblocking: test_joiner_shared_failure_recovers_disk_replacement (auth.rs:2316-2364) now installs a usable disk replacement before acquisition without state contention. acquire returns it at its initial fast path, so the test passes without exercising shared-failure recovery. Arrange the replacement after the caller has actually joined, and make removal of that recovery branch fail the test. The new lock/nonoverwrite witnesses also rely on yield_now() as a scheduling barrier (auth.rs:2923-2939,3032-3052); use explicit polling to Pending or a boundary acknowledgment instead of claiming one yield guarantees the child reached the intended await.

Coverage/limits: checked the public acquisition entrypoints through cache/refresh/browser producers, leader publication, independent joiner state, subsequent reads, rejected-identity and intent boundaries, process generation adoption, persistence ownership, cancellation, and changed test/CI wiring. Read exact-base governing/product documents; integrated all assigned review lanes. Desktop wiring, runtime 401 integration, and Windows secure persistence/cross-process success handoff remain excluded by the agreed contract. Source-only: no checkout, build, tests, or PR code execution; the race above is established by source ordering, not a claimed runtime reproduction. CI success is not asserted.

[wesbillman]

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Review clear: prior blocker resolved

Reviewed exact head e1c89528e7526edabe8cf506fe7bf1cadd3ce35a against base 4a9de1a3a121285ef475d630b2b5764044c02cde. No actionable blocker remains in this corrective review of the agreed acquisition-only contract.

• Lockless cache overwrite repaired. The matching-failure joiner now awaits its local state and calls memory-only invalidation before a filtered disk read (auth.rs:978–985). That path cannot rewrite/truncate/remove credential contents. Combined durable invalidation remains in acquire_locked, reached under the auth-file guard (1029–1085); refresh/browser persistence retains the same ownership. The sole P2 blocker in my previous review is closed.
• Regression witnesses now reach production boundaries. The revised tests directly poll real acquire futures: disk recovery happens after joining, disk contents remain unchanged on shared failure, reconciliation cannot complete while state is locked, and distinct usable Z survives shared success Y (2364–2555, 3066–3263). The disk-invariance test is a no-content-write witness, not a three-process race execution. Independent source audit agreed; no mutation tests were executed.
• Established boundaries preserved. Headless/Auto/UserInitiated separation, full-credential sharing, rejected-identity checks, conditional joiner adoption, generation matching, and cooldown ownership remain intact. A pre-lock LockTimeout does not guarantee durable invalidation; the documented possibility of a later plain read re-adopting disk X is not certified away by this repair.

Scope: re-reviewed the auth.rs delta from the last published head 81216c80, including the subsequent documentation and four waker-borrow corrections, using prior whole-acquisition review evidence for unchanged paths. Read exact-base governing/product guidance and integrated the independent test lane. Desktop Phase 2 wiring, runtime 401 integration, and Windows secure persistence/cross-process success handoff remain deferred; non-Unix same-process credential retention remains in scope.

Validation is source-only: no checkout, build, test, live workflow, or PR-code execution. No current CI success is asserted. This is a clear COMMENTED review, not an approval.