From dbd77b9b228a568ccd6a75b8fd7f1103ac2f6bfa Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Tue, 26 May 2026 03:20:00 +0530 Subject: [PATCH 1/4] fix(keyring): cache availability probe with OnceLock to prevent repeated 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 so macOS only shows the permission dialog once per app launch. --- .../BootCheckGate/BootCheckGate.tsx | 11 +- .../components/oauth/oauthAuthReadiness.ts | 15 +- src/openhuman/app_state/ops.rs | 12 +- src/openhuman/keyring/mod.rs | 3 + src/openhuman/keyring/ops.rs | 23 +++ src/openhuman/keyring/tests.rs | 154 ++++++++++++++++++ 6 files changed, 207 insertions(+), 11 deletions(-) diff --git a/app/src/components/BootCheckGate/BootCheckGate.tsx b/app/src/components/BootCheckGate/BootCheckGate.tsx index 7de91386d9..57f011e7d7 100644 --- a/app/src/components/BootCheckGate/BootCheckGate.tsx +++ b/app/src/components/BootCheckGate/BootCheckGate.tsx @@ -620,15 +620,8 @@ export default function BootCheckGate({ children }: BootCheckGateProps) { try { const checkResult = await runBootCheck(mode, transport); log('[boot-check] gate — check result=%s', checkResult.kind); - - if (checkResult.kind === 'match') { - // Gate resolves — render children. - setPhase('result'); - setResult(checkResult); - } else { - setPhase('result'); - setResult(checkResult); - } + setPhase('result'); + setResult(checkResult); } catch (err) { logError('[boot-check] gate — unexpected error: %o', err); setPhase('result'); diff --git a/app/src/components/oauth/oauthAuthReadiness.ts b/app/src/components/oauth/oauthAuthReadiness.ts index 154295fb03..f0b16b0396 100644 --- a/app/src/components/oauth/oauthAuthReadiness.ts +++ b/app/src/components/oauth/oauthAuthReadiness.ts @@ -4,7 +4,11 @@ import { getCoreStateSnapshot } from '../../lib/coreState/store'; import { bootCheckTransport } from '../../services/bootCheckService'; import { getCoreRpcUrl, testCoreRpcConnection } from '../../services/coreRpcClient'; import { isTauri } from '../../services/webviewAccountService'; -import { getStoredCoreMode, getStoredCoreToken } from '../../utils/configPersistence'; +import { + getStoredCoreMode, + getStoredCoreToken, + storeCoreMode, +} from '../../utils/configPersistence'; const logPrefix = '[oauth-auth-readiness]'; const log = debug('oauth:auth-readiness'); @@ -83,6 +87,15 @@ export async function waitForOAuthAuthReadiness( sawCoreMode = true; break; } + // In the Tauri desktop app the core is always embedded locally. If the + // picker hasn't run yet (e.g. first launch before BootCheckGate finishes, + // or core mode was just cleared), default to 'local' so OAuth can proceed + // without forcing the user to navigate back through the runtime picker. + if (isTauri()) { + storeCoreMode('local'); + sawCoreMode = true; + break; + } await delay(POLL_MS); } diff --git a/src/openhuman/app_state/ops.rs b/src/openhuman/app_state/ops.rs index 7f94bdc663..9dd41f2867 100644 --- a/src/openhuman/app_state/ops.rs +++ b/src/openhuman/app_state/ops.rs @@ -535,7 +535,17 @@ pub async fn snapshot() -> Result, String> { // snapshot. On Windows this doubled the surface area for the // "Timed out waiting for auth profile lock" failure reported in // Sentry against `openhuman.app_state_snapshot`. - let session_profile = load_app_session_profile(&config)?; + // + // `load_app_session_profile` calls `acquire_lock()`, which busy-waits + // with `thread::sleep` for up to ~35s when the lock is contended. Calling + // it directly on a tokio worker thread blocks that thread for the entire + // wait, exhausting the thread pool under concurrent snapshot calls and + // triggering `ERR_CONNECTION_TIMED_OUT` on all RPC connections. + let config_for_profile = config.clone(); + let session_profile = + tokio::task::spawn_blocking(move || load_app_session_profile(&config_for_profile)) + .await + .unwrap_or_else(|e| Err(format!("[app_state] auth profile load task panicked: {e}")))?; let mut auth = session_state_from_profile(session_profile.as_ref()); let session_token = session_token_from_profile(session_profile.as_ref()); let stored_user = sanitize_snapshot_user(auth.user.clone()); diff --git a/src/openhuman/keyring/mod.rs b/src/openhuman/keyring/mod.rs index 98821b1e41..b09ba4bb2b 100644 --- a/src/openhuman/keyring/mod.rs +++ b/src/openhuman/keyring/mod.rs @@ -47,3 +47,6 @@ pub use store::init_workspace; #[cfg(test)] #[allow(unused_imports)] pub(crate) use ops::force_backend_for_test; + +#[cfg(test)] +mod tests; diff --git a/src/openhuman/keyring/ops.rs b/src/openhuman/keyring/ops.rs index ff5ed6c39a..ccb21c4422 100644 --- a/src/openhuman/keyring/ops.rs +++ b/src/openhuman/keyring/ops.rs @@ -3,12 +3,21 @@ //! All public functions delegate to the active backend selected by [`crate::openhuman::keyring::store`]. use std::path::Path; +use std::sync::OnceLock; use chacha20poly1305::aead::{rand_core::RngCore, OsRng}; use crate::openhuman::keyring::error::KeyringError; use crate::openhuman::keyring::store::backend; +// Cached result of the one-time keychain probe. Running the probe on every +// `is_available()` call means 4 OS-keychain round-trips (delete / set / get / +// delete) per call, which triggers repeated macOS access-permission dialogs +// and starves callers that poll frequently (e.g. snapshot, wallet guards). +// The backend selection is already frozen in a OnceLock, so the probe result +// is stable for the lifetime of the process — caching it here is safe. +static AVAILABILITY_CACHE: OnceLock = OnceLock::new(); + // ── Outcome type ───────────────────────────────────────────────────────────── /// Outcome of a file-to-keychain migration attempt. @@ -82,7 +91,16 @@ pub fn delete(user_id: &str, key: &str) -> Result<(), KeyringError> { /// For the `file` and `mock` backends this always returns `true`. For the /// `os` backend on Linux headless systems (no Secret Service daemon) this /// returns `false`; callers should fall back to file-based storage. +/// +/// The result is cached after the first call — the probe runs exactly once +/// per process lifetime. This prevents repeated OS-keychain round-trips (and +/// the macOS access-permission dialogs they trigger) when polled by +/// wallet guards or snapshot loops. pub fn is_available() -> bool { + *AVAILABILITY_CACHE.get_or_init(probe_availability) +} + +fn probe_availability() -> bool { const PROBE_USER: &str = "__probe__"; const PROBE_KEY: &str = "__openhuman_keyring_probe__"; const PROBE_VALUE: &str = "__probe_value__"; @@ -100,6 +118,11 @@ pub fn is_available() -> bool { } let result = (|| -> Result { + // Delete any leftover probe key from a previous launch before writing. + // Without this, `set` fails with "item already exists" (-25299) on + // every launch after the first, causing `is_available` to incorrectly + // return false even when the keychain is fully functional. + let _ = delete(PROBE_USER, PROBE_KEY); set(PROBE_USER, PROBE_KEY, PROBE_VALUE)?; let readback = get(PROBE_USER, PROBE_KEY)?; delete(PROBE_USER, PROBE_KEY)?; diff --git a/src/openhuman/keyring/tests.rs b/src/openhuman/keyring/tests.rs index 9fefdfe482..2b31a9b1b1 100644 --- a/src/openhuman/keyring/tests.rs +++ b/src/openhuman/keyring/tests.rs @@ -380,6 +380,160 @@ fn get_or_create_random_idempotent_file_backend() { assert_eq!(first, second, "idempotent read-back"); } +// ── is_available probe: idempotency regression tests ───────────────────────── +// +// The probe in `is_available()` must return `true` on every launch, not just the +// first one. Before the fix, `set()` was called blindly: on macOS the Keychain +// returns `-25299 "item already exists"` when the probe key survives from a +// prior launch, flipping `is_available` to `false` even though the keychain is +// fully functional. The fix deletes the probe key first so the write always +// lands clean. +// +// We verify this with a `StrictSetBackend` — a `FileBackend` wrapper that +// rejects `set()` on an already-existing key, exactly matching the Keychain +// semantics that exposed the bug. This lets the regression run in CI without +// the `OPENHUMAN_TEST_OS_KEYCHAIN=1` gate. + +/// Wraps `FileBackend` but fails `set` with `KeyringError::Backend` when the +/// key already exists, mirroring macOS Keychain's `-25299` behaviour. +struct StrictSetBackend { + inner: FileBackend, +} + +impl StrictSetBackend { + fn new(dir: &std::path::Path) -> Self { + Self { + inner: FileBackend::new(dir), + } + } +} + +impl KeyringBackend for StrictSetBackend { + fn get(&self, key: &str) -> Result, KeyringError> { + self.inner.get(key) + } + fn set(&self, key: &str, value: &str) -> Result<(), KeyringError> { + if self.inner.get(key)?.is_some() { + return Err(KeyringError::Backend( + "Platform secure storage failure: The specified item already exists in the keychain. (OSStatus -25299)".into(), + )); + } + self.inner.set(key, value) + } + fn delete(&self, key: &str) -> Result<(), KeyringError> { + self.inner.delete(key) + } + fn name(&self) -> &'static str { + "strict-set-mock" + } +} + +/// Run the OLD (broken) probe logic against any backend. +/// Returns `true` only if the round-trip succeeded. +fn old_probe(b: &B) -> bool { + const KEY: &str = "__probe__:__openhuman_keyring_probe__"; + const VAL: &str = "__probe_value__"; + if b.set(KEY, VAL).is_err() { + return false; + } + let ok = b.get(KEY).ok().flatten().as_deref() == Some(VAL); + let _ = b.delete(KEY); + ok +} + +/// Run the NEW (fixed) probe logic against any backend. +/// Deletes any residue before writing, making the probe idempotent. +fn new_probe(b: &B) -> bool { + const KEY: &str = "__probe__:__openhuman_keyring_probe__"; + const VAL: &str = "__probe_value__"; + let _ = b.delete(KEY); + if b.set(KEY, VAL).is_err() { + return false; + } + let ok = b.get(KEY).ok().flatten().as_deref() == Some(VAL); + let _ = b.delete(KEY); + ok +} + +#[test] +fn probe_old_logic_fails_when_key_preexists() { + // Demonstrates the bug: old probe returns false the second time because + // `set` on an already-existing key fails with "item already exists". + let dir = TempDir::new().expect("tempdir"); + let b = StrictSetBackend::new(dir.path()); + + // First probe: key absent → succeeds. + assert!(old_probe(&b), "first probe should succeed"); + + // Key was deleted by the probe — re-seed it to simulate a leftover from a + // previous launch (e.g. the app was force-quit after writing but before + // the delete could run, or a previous `is_available` call left it behind). + b.inner + .set("__probe__:__openhuman_keyring_probe__", "stale") + .unwrap(); + + // Second probe with residue: `set` hits "already exists" → returns false. + assert!( + !old_probe(&b), + "old probe must fail when probe key pre-exists (regression target)" + ); +} + +#[test] +fn probe_new_logic_succeeds_even_when_key_preexists() { + // Verifies the fix: new probe deletes any residue first, so it always + // succeeds regardless of leftover state. + let dir = TempDir::new().expect("tempdir"); + let b = StrictSetBackend::new(dir.path()); + + assert!(new_probe(&b), "first probe should succeed"); + + // Re-seed to simulate a leftover key (same scenario as the bug test). + b.inner + .set("__probe__:__openhuman_keyring_probe__", "stale") + .unwrap(); + + assert!( + new_probe(&b), + "new probe must succeed even when probe key pre-exists" + ); +} + +#[test] +fn probe_new_logic_is_idempotent_across_multiple_runs() { + // New probe must return true on every consecutive call. + let dir = TempDir::new().expect("tempdir"); + let b = StrictSetBackend::new(dir.path()); + + for i in 0..5 { + assert!(new_probe(&b), "probe run {i} should succeed"); + } +} + +#[test] +fn is_available_returns_true_on_repeated_calls_os_backend() { + // End-to-end: the real `is_available()` must return true on consecutive + // calls even when the OS backend retains the probe key between runs. + // Guarded by OPENHUMAN_TEST_OS_KEYCHAIN=1 to avoid blocking CI on a + // Keychain permission dialog. + if !os_keychain_available() { + eprintln!("skip: set OPENHUMAN_TEST_OS_KEYCHAIN=1 to run OS keychain tests"); + return; + } + // Use the OsBackend directly to simulate the cross-launch residue. + let b = backend::OsBackend; + let probe_key = "__probe__:__openhuman_keyring_probe__"; + // Pre-seed to mimic a leftover from a previous app launch. + let _ = b.set(probe_key, "__probe_value__"); + // `is_available()` must still return true. + assert!( + super::ops::is_available(), + "is_available must return true even with pre-existing probe key in OS keychain" + ); + // Cleanup. + let _ = b.delete(probe_key); +} + // ── migrate_from_file (via module functions, requires OS keychain or file backend) ── #[test] From 91f90194492b5d026e0912e2c261baa29de2ac95 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Tue, 26 May 2026 03:35:16 +0530 Subject: [PATCH 2/4] test(keyring): exercise cached path with second is_available() call CodeRabbit flagged that the repeated-calls test only invoked is_available() once, leaving the OnceLock cache path untested. Add a second assertion. --- src/openhuman/keyring/tests.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/openhuman/keyring/tests.rs b/src/openhuman/keyring/tests.rs index 2b31a9b1b1..0342ba1609 100644 --- a/src/openhuman/keyring/tests.rs +++ b/src/openhuman/keyring/tests.rs @@ -530,6 +530,11 @@ fn is_available_returns_true_on_repeated_calls_os_backend() { super::ops::is_available(), "is_available must return true even with pre-existing probe key in OS keychain" ); + // Repeated call should also stay true (exercises the OnceLock cached path). + assert!( + super::ops::is_available(), + "is_available must remain true on repeated calls (cached result)" + ); // Cleanup. let _ = b.delete(probe_key); } From 3c39f31b5d2bcde63bc7c60fbeaef0e3dac7c836 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Tue, 26 May 2026 05:15:40 +0530 Subject: [PATCH 3/4] test(oauth): add storeCoreMode to configPersistence mock and fix core_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. --- app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts b/app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts index 53ac959823..a0ba8a83e0 100644 --- a/app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts +++ b/app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts @@ -25,6 +25,7 @@ vi.mock('../../../services/bootCheckService', () => ({ vi.mock('../../../utils/configPersistence', () => ({ getStoredCoreMode: vi.fn(), getStoredCoreToken: vi.fn().mockReturnValue(null), + storeCoreMode: vi.fn(), })); vi.mock('../../../services/webviewAccountService', () => ({ @@ -59,6 +60,9 @@ describe('oauthAuthReadiness', () => { it('returns core_mode_unset when BootCheckGate has not committed a mode', async () => { vi.mocked(getStoredCoreMode).mockReturnValue(null); + // Must be web (non-Tauri) — in Tauri the code defaults to 'local' and never + // returns core_mode_unset. + vi.mocked(isTauri).mockReturnValue(false); const result = await waitForOAuthAuthReadiness(500); From 88a3ef3a9d2b2b65661513c83718dd9a89fe8078 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Tue, 26 May 2026 10:30:52 +0530 Subject: [PATCH 4/4] test(oauth): cover Tauri default-to-local path in waitForOAuthAuthReadiness 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. --- .../oauth/__tests__/oauthAuthReadiness.test.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts b/app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts index a0ba8a83e0..6b9048b969 100644 --- a/app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts +++ b/app/src/components/oauth/__tests__/oauthAuthReadiness.test.ts @@ -4,7 +4,11 @@ import { getCoreStateSnapshot } from '../../../lib/coreState/store'; import { bootCheckTransport } from '../../../services/bootCheckService'; import { testCoreRpcConnection } from '../../../services/coreRpcClient'; import { isTauri } from '../../../services/webviewAccountService'; -import { getStoredCoreMode, getStoredCoreToken } from '../../../utils/configPersistence'; +import { + getStoredCoreMode, + getStoredCoreToken, + storeCoreMode, +} from '../../../utils/configPersistence'; import { oauthAuthReadinessUserMessage, prepareOAuthLoginLaunch, @@ -58,6 +62,16 @@ describe('oauthAuthReadiness', () => { vi.mocked(isTauri).mockReturnValue(true); }); + it('defaults to local mode in Tauri when no core mode is stored', async () => { + vi.mocked(getStoredCoreMode).mockReturnValue(null); + // isTauri is true from beforeEach — should auto-set 'local' and proceed + + const result = await waitForOAuthAuthReadiness(2_000); + + expect(vi.mocked(storeCoreMode)).toHaveBeenCalledWith('local'); + expect(result).toEqual({ ready: true }); + }); + it('returns core_mode_unset when BootCheckGate has not committed a mode', async () => { vi.mocked(getStoredCoreMode).mockReturnValue(null); // Must be web (non-Tauri) — in Tauri the code defaults to 'local' and never