fix(reborn): refresh OAuth runtime credentials on staging - #5053
Conversation
|
🚅 Deployed to the ironclaw-pr-5053 environment in ironclaw-ci-preview
|
There was a problem hiding this comment.
Code Review
This pull request removes the tracking of refreshed credential account IDs in ProductAuthRuntimeCredentialAccountRefresher. This change ensures that each OAuth staging triggers a fresh credential refresh rather than indefinitely reusing a previously staged access secret, since access-token expiry is not part of the credential account record. The corresponding unit tests have been updated to assert that subsequent staging requests trigger a refresh. No review comments were provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRemoves the ChangesRemove OAuth account refresh deduplication
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…5071) BLOCKER found in thermo-nuclear review: the inline conditional-refresh optimization (A2 — skip the token-endpoint round-trip when the access token is still within its freshness margin, the #5053 anti-hammer fix) was a silent no-op in production. Root cause: the OAuth provider client writes access-token `expires_at` to the real (filesystem) secret store, but `RebornProductAuthServices::new` defaulted its `secret_store` to a fresh empty `InMemorySecretStore` and the builder `with_secret_store` was never called on any production composition path. So the margin check read `metadata() == None` every time and always refreshed — defeating the whole point of making the refresh conditional. Fix: make `secret_store` a required parameter of the canonical ports->services bridge `RebornProductAuthServicePorts::into_services`, and thread the same `secret_store` that backs the provider client through `compose_product_auth_services` at both the production and local-dev call sites. The store is now structurally guaranteed to be the same one the provider writes to. `new()`/`from_shared` keep their in-memory default for the ~50 direct test constructors (which never write tokens), so blast radius stays at the 4 into_services call sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…5071) (#5087) * docs(reborn): plan proactive Google OAuth refresh (#5071) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(secrets): persist access-token expiry through SecretStore::put (#5071 WS1) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): margin-conditional inline OAuth refresh + invalid_grant classification (#5071 WS2) - Add DEFAULT_ACCESS_REFRESH_MARGIN (5 min) to skip refresh when token still fresh - Wire secret_store into ProductAuthRuntimeCredentialAccountRefresher via optional builder on RebornProductAuthServices - Parse OAuth error body in execute_token_request; classify invalid_grant as AuthProductError::InvalidGrant - Handle InvalidGrant in credential.rs by marking account Revoked (permanent, not transient) - Add InvalidGrant to non-exhaustive match in auth_interaction service (maps to StaleAuth) - Fix pre-existing 3-arg put() calls in tests (expires_at=None); fix crash-safe write-order test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): cross-process advisory lock around OAuth credential refresh (#5071 WS3) Blocking pg_advisory_lock serializes concurrent refreshers (second waits, then refreshes with the rotated token) rather than racing the token endpoint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): background Google OAuth credential keepalive worker (#5071 WS4) Implements WS4 of the proactive Google OAuth token refresh feature: - B1: cross-owner candidate enumeration on FilesystemAuthProductServices<F> via new `list_refresh_candidates` method; adds `root: Option<Arc<F>>` and `new_with_root` constructor to enable scanning outside the scoped mount. - B2/B3: new `credential_refresh_worker` module (mirrors trigger_poller.rs) with `CredentialRefreshCandidateSource` trait, `CredentialRefreshWorkerDeps`, `spawn_credential_refresh_worker`, and the tick loop (startup jitter, idle-threshold filter, per-tick cap, advisory-locked refresh). - B4: `CredentialRefreshSettings` in runtime_input.rs; added to `RebornRuntimeInput` with builder method. - Spawn wiring: `build_reborn_runtime` takes the candidate source from `RebornServices` and spawns the worker, storing the handle in `RebornRuntime`; `shutdown()` drains with a 5-second timeout. - All new code gated behind `#[cfg(any(feature = "libsql", feature = "postgres"))]` so no-db builds (reborn_cli default) compile warning-free. - Logging: `debug!` only — never `info!`/`warn!` (REPL/TUI invariant). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): leader-lock keepalive worker, enable in prod, review fixes + tests (#5071) - Replace per-account advisory lock with a deployment-wide leader-election lock used by the worker only (pg_try_advisory_lock, one fixed key, 1 conn); inline path reverts to the in-process refresh_locks guard. - Enable the worker for the Serve caller via the CLI with an IRONCLAW_CREDENTIAL_REFRESH_ENABLED kill-switch (struct default stays off). - Review fixes: worker debug!-only logging, accurate advisory-lock comment, cleanup log message, startup jitter default, cancellation between refreshes, silent-ok annotation, settings doc/Default consistency. - Simplifications: secret_store non-optional; inline refresh margin is config. - Tests: worker tick/candidate-filter/transient-error, spawn guard, A2 margin (fresh-skip/within-margin/absent-expiry), filesystem expires_at roundtrip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): address PR #5087 review — overflow guards, refresh-token rollback, margin wiring, annotations (#5071) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): keepalive covers agent/project roots, fail-closed leader lock, InMemory honors expiry, drop dead margin knob (#5087) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): cover keepalive candidate enumeration across agent/project scopes (#5087) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): warn at startup when keepalive worker is enabled but deps missing (#5087) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(reborn): cargo fmt --all (#5087) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix: thread expires_at through all remaining SecretStore::put call sites (#5087) WS1 signature change missed sites in ironclaw_host_runtime (lib + integration tests) and possibly others, breaking the all-features / E2E builds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): remove dead test helper failing_refresh_put (clippy -D warnings) (#5087) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): address PR #5087 review — cancellation-safe leader lock, dedup refresh arms, tests (#5071) Round of PR review fixes: - credential.rs: collapse the duplicated InvalidGrant / RefreshFailed terminal refresh-state handling into a single report_terminal_refresh_status helper (re-read, stale-check, status update, report) — only the target status differs. - product_auth_refresh_lock.rs: switch the leader lock from a session-level pg_try_advisory_lock to a TRANSACTION-level pg_try_advisory_xact_lock wrapped around the sweep. The xact lock auto-releases on commit, rollback, panic, or task cancellation, so an aborted sweep can no longer strand the lock on a pooled connection and block all future leader elections. - credential_refresh_worker.rs: race each per-account refresh against the CancellationToken with tokio::select! (biased) so shutdown no longer waits for an in-flight token-endpoint call; extract a pure, unit-tested select_idle_candidates() for the idle/cap policy. - credential_refresh_worker.rs tests: cover idle filtering + max_per_tick cap. - runtime/mod.rs: split the IRONCLAW_CREDENTIAL_REFRESH_ENABLED parse into a pure apply_credential_refresh_override() and add tests for Serve default-on, non-Serve default-off, kill-switch, force-on, blank, and invalid values. - product_auth_durable.rs / worker trait doc: soften the over-promising "never projects secret handles" comments to accurately state the records carry opaque secret handles that must not be logged/serialized. - product_auth_runtime_credentials/tests.rs: assert the expiry-fixture put() succeeds (.expect) so the within-margin/skip branches are actually exercised. - oauth_helpers_contract.rs: fix an erroneous earlier test edit that swapped the invalid scope to auth/drive (which is in the approved set) — restore the genuinely-unapproved gmail.insert so the rejection assertion holds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): wire the OAuth secret store into product-auth services (#5071) BLOCKER found in thermo-nuclear review: the inline conditional-refresh optimization (A2 — skip the token-endpoint round-trip when the access token is still within its freshness margin, the #5053 anti-hammer fix) was a silent no-op in production. Root cause: the OAuth provider client writes access-token `expires_at` to the real (filesystem) secret store, but `RebornProductAuthServices::new` defaulted its `secret_store` to a fresh empty `InMemorySecretStore` and the builder `with_secret_store` was never called on any production composition path. So the margin check read `metadata() == None` every time and always refreshed — defeating the whole point of making the refresh conditional. Fix: make `secret_store` a required parameter of the canonical ports->services bridge `RebornProductAuthServicePorts::into_services`, and thread the same `secret_store` that backs the provider client through `compose_product_auth_services` at both the production and local-dev call sites. The store is now structurally guaranteed to be the same one the provider writes to. `new()`/`from_shared` keep their in-memory default for the ~50 direct test constructors (which never write tokens), so blast radius stays at the 4 into_services call sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): bundle keepalive worker deps into CredentialRefreshWorkerReady; fmt (#5071) Make the illegal "enabled but deps half-wired" state unrepresentable instead of guarding it at runtime: - Replace the two `Option` fields (`credential_refresh_candidate_source`, `credential_refresh_leader_lock`) on RebornServices with a single `CredentialRefreshWorkerReady` enum that bundles candidate_source + leader_lock + refresh_port into one `Ready` variant (or `Absent`). The deps are only ever produced together on the durable production path, so the type now enforces all-or-nothing wiring. - runtime.rs spawn site collapses from a 3-tuple match with an "enabled-but-deps-missing → warn!" arm into a clean two-arm enum match. That warn arm was effectively dead (enabled only on the Serve path, which always produces Ready); the enum deletes it rather than relying on it never firing. The `enabled` policy flag still gates the actual spawn inside spawn_credential_refresh_worker. Also apply cargo fmt (the previous two commits were not formatted — the real CI failure was the fmt gate cascading into the dependent jobs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): thread expires_at through qa_trace fixture put() after merging main (#5071) main #5095 added recorded QA fixtures whose harness seeds the secret store via SecretStore::put; this branch changed that signature to take expires_at. The merge of the two collided (E0061) in the branch-merged-into-main CI ref while the branch alone compiled. Add the `None` expiry arg to both seed calls in tests/support/reborn/qa_trace.rs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): harden keepalive fairness + cover invalid_grant revocation (#5071) Address PR #5087 review (review 4539067623): - credential_refresh_worker: sort idle candidates oldest-first (by updated_at) before applying max_per_tick. The oldest accounts are closest to Google's 7-day refresh-token idle-death, so they must be served first — this stops a head-of-enumeration subset (e.g. accounts stuck in transient errors that keep their old updated_at) from starving the tail when more than max_per_tick accounts are idle. Unit test now asserts oldest-first selection. - auth contract: lock the stable sanitized-code mapping AuthProductError::InvalidGrant -> AuthErrorCode::RefreshFailed. - auth contract: add a caller-level test for the new invalid_grant branch — provider returns InvalidGrant, assert the persisted account is Revoked and the recovery projection is ReauthorizeRequired/AccountRevoked (distinct from the generic RefreshFailed path). Adds an invalid_grant_next_refresh_for_tests hook to the auth fakes, mirroring the existing fail_next_refresh_for_tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Tests