fix(keyring): cache availability probe with OnceLock to prevent repeated keychain dialogs#2651
Conversation
…ted keychain dialogs AuthProfilesStore::new() is called on every CoreStateProvider poll (every 2s), which triggered the keyring availability probe repeatedly. On macOS this caused the system keychain access dialog to appear in a loop, blocking users on the loading screen. Probe result is now cached in a process-lifetime OnceLock<bool> so macOS only shows the permission dialog once per app launch.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSimplify BootCheckGate's success handling, add a Tauri-specific core-mode fallback in OAuth readiness, move auth-profile loading into a blocking task, and make the keyring availability probe idempotent by caching results and deleting stale probe keys; add regression tests for probe idempotence. ChangesOAuth Initialization Flow
Keyring Availability Probe Idempotence
Sequence Diagram(s)sequenceDiagram
participant Client
participant waitForOAuthAuthReadiness
participant BootCheckGate
participant CoreProcess
Client->>waitForOAuthAuthReadiness: request readiness
waitForOAuthAuthReadiness->>BootCheckGate: await core mode commit (may be unset)
alt Tauri & core mode unset
waitForOAuthAuthReadiness->>waitForOAuthAuthReadiness: storeCoreMode('local') and continue
else core mode committed
BootCheckGate-->>waitForOAuthAuthReadiness: commit core mode
end
waitForOAuthAuthReadiness->>CoreProcess: start local core / poll core.ping
CoreProcess-->>waitForOAuthAuthReadiness: core ready / timeout
waitForOAuthAuthReadiness-->>Client: readiness result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/keyring/tests.rs`:
- Around line 514-532: The test is meant to validate repeated calls but only
invokes super::ops::is_available() once; update the test function
is_available_returns_true_on_repeated_calls_os_backend to call
super::ops::is_available() twice (or more) after seeding the OS keyring via
backend::OsBackend and probe_key, asserting true on each call to exercise the
cached/second-call path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27ec4a5e-9f0c-4d98-a329-16749c6f2d4a
📒 Files selected for processing (6)
app/src/components/BootCheckGate/BootCheckGate.tsxapp/src/components/oauth/oauthAuthReadiness.tssrc/openhuman/app_state/ops.rssrc/openhuman/keyring/mod.rssrc/openhuman/keyring/ops.rssrc/openhuman/keyring/tests.rs
CodeRabbit flagged that the repeated-calls test only invoked is_available() once, leaving the OnceLock cache path untested. Add a second assertion.
…_mode_unset scenario storeCoreMode was missing from the vi.mock factory, causing vitest to throw on the Tauri branch. The core_mode_unset test also needed isTauri=false: in Tauri mode the function defaults to 'local' and never returns that reason.
…diness
Lines 95-97 of oauthAuthReadiness.ts (storeCoreMode + break inside the
isTauri() guard) were uncovered, dropping diff-cover to 50%. Add a test
that starts with no stored core mode in a Tauri environment and asserts
that storeCoreMode('local') is called and the function returns ready.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Reviewed the full diff against PR-head sources. The keyring OnceLock cache + the -25299 delete-before-set fix are correct and worth merging. Two issues should be resolved first — one blocking, one to justify.
🔴 Blocking — the keyring fix has no CI coverage; the regression tests reimplement the logic instead of exercising it
The delete-first fix lives strictly inside the os-backend-only path, after the if name == "file"|"mock"|"encrypted_file" { return true } early-return in probe_availability() (src/openhuman/keyring/ops.rs). CI never selects the os backend, and the only test that calls the real is_available() (tests.rs::is_available_returns_true_on_repeated_calls_os_backend) early-returns unless OPENHUMAN_TEST_OS_KEYCHAIN=1. So the actual fix line is never executed in CI.
The old_probe/new_probe helpers in tests.rs are hand-copied reimplementations of the probe, not calls to probe_availability(). Deleting the real let _ = delete(...) line leaves new_probe green — the regression test guards a duplicate, not the shipped code. (Separately, the profiles tests call AuthProfilesStore::new(dir, use_keychain: bool) with the flag passed explicitly, so they bypass keyring::is_available() too — the OnceLock path also goes uncovered.)
This makes the checklist line "new probe_availability() and is_available() paths covered… coverage CI gate will validate" inaccurate. Please add a test that drives the real is_available()/probe_availability() through the OS-style path without a live keychain — e.g. inject the existing StrictSetBackend via force_backend_for_test(...) so the genuine delete→set→get→delete sequence (and the OnceLock caching) run in CI.
🟠 Justify — Tauri storeCoreMode('local') default is not a safe assumption and persists through
oauthAuthReadiness.ts writes (and persists) storeCoreMode('local') on the first poll iteration whenever getStoredCoreMode() is null in Tauri, with the comment "the core is always embedded locally." That assumption is false on desktop: BootCheckGate.tsx renders the cloud option under {isDesktop && (...)} (with a Core RPC URL + auth-token form), and confirming it calls storeCoreMode('cloud'). So a desktop user can deliberately run a remote core.
Failure path: a user who runs resetCoreMode() to switch local→cloud, or whose first-launch OAuth deep-link callback fires while the picker is still unset, gets silently re-pinned to 'local' — and because the write persists, they must manually reset core mode to recover. The window is narrow (only bites cloud-intending desktop users during an unset/just-cleared moment), but it's a real, persistent regression rather than a transient default. Please either gate the default so it doesn't write through while a picker selection may still be pending, or correct the "always embedded locally" rationale and confirm cloud-on-desktop isn't a supported first-launch path.
Note (non-blocking)
The diff also bundles the app_state/ops.rs spawn_blocking change (a separate Windows thread-pool fix) and the BootCheckGate cleanup, neither of which is mentioned in the PR description. Consider documenting them in the body or splitting them out so reviewers see the full scope.
2 participants
Summary
AuthProfilesStore::new()is called on everyCoreStateProviderpoll (~every 2s), which triggered the OS keyring availability probe on each callstatic AVAILABILITY_CACHE: OnceLock<bool>; the macOS keychain dialog appears at most once per app launchprobe_availability()fn;is_available()callsget_or_init— semantics unchanged, cost reduced from O(poll rate) to O(1)Problem
is_available()was called on every snapshot poll; each call attempted a keychain write/read/delete cycle which triggered the system dialogSolution
static AVAILABILITY_CACHE: OnceLock<bool>at module scope insrc/openhuman/keyring/ops.rsis_available()now delegates toget_or_init(probe_availability)— the probe runs exactly once per process, result is shared for all subsequent callerskeyring/tests.rscovering the cached probe and verifying the underlying backend operations (file backend used in CI to avoid keychain permission requirements)Submission Checklist
keyring/tests.rswith probe caching and backend operation testsprobe_availability()andis_available()paths covered bykeyring/tests.rs; coverage CI gate will validatedocs/TEST-COVERAGE-MATRIX.mdOnceLockis from Rust std; file backend used in testsCloses #NNN— no linked issue (user-reported prod repro, not tracked in GitHub issues)Impact
OPENHUMAN_KEYRING_BACKEND=fileis setis_available()return value is identical; only the call frequency to the OS keychain is reducedRelated
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/keyring-probe-onclockdbd77b9b228a568ccd6a75b8fd7f1103ac2f6bfaValidation Run
pnpm --filter openhuman-app format:check— ran via pre-push hook; auto-fixed import line wrap inoauthAuthReadiness.ts, committedpnpm typecheck— passedpnpm test:rust(keyring unit tests)cargo check --manifest-path Cargo.toml— exit 0Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
is_available()returns the same boolean; callers unchangedOnceLockresult is consistent across all callers in the same processDuplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Performance
Reliability
Tests