diff --git a/src/openhuman/credentials/profiles.rs b/src/openhuman/credentials/profiles.rs index eb130e861..c802fd2b2 100644 --- a/src/openhuman/credentials/profiles.rs +++ b/src/openhuman/credentials/profiles.rs @@ -59,10 +59,13 @@ const LOCK_TIMEOUT_MS: u64 = STALE_LOCK_AGE_MS + 5_000; /// Retry budget for the JSON write + rename in `write_persisted_locked`. /// Same shape as the lock-create call at the bottom of `acquire_lock` (which -/// is what closed Sentry OPENHUMAN-TAURI-H1 / H8 in #1641 / #2085). 6 attempts -/// at base 100ms doubles up to ~6.3s worst-case before surfacing. Sized to -/// stay well inside `LOCK_TIMEOUT_MS` so concurrent acquire_lock callers -/// never time out behind a single retry-loop owner. +/// is what closed Sentry OPENHUMAN-TAURI-H1 / H8 in #1641 / #2085). With +/// `attempts = 6`, `retry_with_backoff` issues at most 6 calls and sleeps +/// 5 times between them (last failure breaks without sleeping): +/// `100+200+400+800+1600 ≈ 3.1s per stage`, so the write and rename stages +/// together sit at `≈6.2s` worst case. Sized to stay well inside +/// `LOCK_TIMEOUT_MS = 35_000` so concurrent acquire_lock callers never time +/// out behind a single retry-loop owner. const PERSIST_RETRY_ATTEMPTS: u32 = 6; const PERSIST_RETRY_BASE_MS: u64 = 100; @@ -214,13 +217,22 @@ pub struct AuthProfilesStore { /// Whether the OS keychain is available on this machine. /// Cached at construction time to avoid repeated probes. use_keychain: bool, - /// `#[cfg(test)]` failure injection — when non-zero, the next retryable - /// FS call inside `write_persisted_locked` consumes one count and returns - /// a `__TEST_TRANSIENT__` error so the `is_transient_fs_error` classifier - /// treats it as retryable (`src/openhuman/util.rs:618`). Production - /// binaries never see this field. + /// `#[cfg(test)]` failure injection for the **write** stage of + /// `write_persisted_locked`. When non-zero, the next call inside the + /// `fs::write(tmp)` retry loop consumes one count and returns a + /// `__TEST_TRANSIENT__` error so `is_transient_fs_error` treats it as + /// retryable (`src/openhuman/util.rs:618`). Production binaries never + /// see this field. #[cfg(test)] - force_transient_failures: Arc, + force_transient_failures_write: Arc, + /// `#[cfg(test)]` failure injection for the **rename** stage of + /// `write_persisted_locked`. Separate counter from the write stage so a + /// test can exercise the rename retry loop without first having to drain + /// failures through the write stage (see PR #3364 review feedback — + /// the headline retry path was line-covered but not behaviour-covered + /// before this split). + #[cfg(test)] + force_transient_failures_rename: Arc, } impl AuthProfilesStore { @@ -263,7 +275,9 @@ impl AuthProfilesStore { user_id, use_keychain, #[cfg(test)] - force_transient_failures: Arc::new(AtomicUsize::new(0)), + force_transient_failures_write: Arc::new(AtomicUsize::new(0)), + #[cfg(test)] + force_transient_failures_rename: Arc::new(AtomicUsize::new(0)), } } @@ -966,7 +980,7 @@ impl AuthProfilesStore { PERSIST_RETRY_ATTEMPTS, PERSIST_RETRY_BASE_MS, || { - self.consume_test_transient_failure()?; + self.consume_test_transient_failure_write()?; fs::write(&tmp_path, &json).context("write auth profile tmp") }, ) @@ -977,12 +991,12 @@ impl AuthProfilesStore { ) })?; - retry_with_backoff( + let rename_result = retry_with_backoff( "replace auth profile store", PERSIST_RETRY_ATTEMPTS, PERSIST_RETRY_BASE_MS, || { - self.consume_test_transient_failure()?; + self.consume_test_transient_failure_rename()?; fs::rename(&tmp_path, &self.path).context("rename auth profile tmp -> store") }, ) @@ -991,45 +1005,79 @@ impl AuthProfilesStore { "Failed to replace auth profile store at {}", self.path.display() ) - })?; + }); - Ok(()) + if rename_result.is_err() { + // Best-effort orphan cleanup: `tmp_path` is `…tmp.{pid}.{nanos}` + // — unique per call — so a permanently-failing rename otherwise + // leaks one tmp file per `app_state_snapshot` poll (~2s cadence) + // under sustained Windows AV / Search-Indexer holds. Cleaning + // here keeps the directory tidy; the cleanup itself can fail + // (the same AV that blocked the rename may block the unlink), + // which is why we deliberately drop the result. + let _ = fs::remove_file(&tmp_path); + } + + rename_result } - /// Consume one test-injected transient FS failure if any are queued. - /// No-op in production builds. + /// Consume one test-injected transient FS failure for the **write** + /// stage if any are queued. No-op in production builds. #[cfg(test)] - fn consume_test_transient_failure(&self) -> Result<()> { - let remaining = self.force_transient_failures.load(Ordering::SeqCst); - if remaining == 0 { - return Ok(()); - } - self.force_transient_failures.fetch_sub(1, Ordering::SeqCst); - Err(anyhow::anyhow!( - "__TEST_TRANSIENT__ injected transient FS failure" - )) + fn consume_test_transient_failure_write(&self) -> Result<()> { + consume_one(&self.force_transient_failures_write) + } + + /// Consume one test-injected transient FS failure for the **rename** + /// stage if any are queued. No-op in production builds. + #[cfg(test)] + fn consume_test_transient_failure_rename(&self) -> Result<()> { + consume_one(&self.force_transient_failures_rename) } #[cfg(not(test))] #[inline(always)] - fn consume_test_transient_failure(&self) -> Result<()> { + fn consume_test_transient_failure_write(&self) -> Result<()> { Ok(()) } - /// Queue `n` test-only forced transient FS failures. The next `n` - /// retryable calls inside `write_persisted_locked` return a - /// `__TEST_TRANSIENT__` error before the underlying FS op runs; the - /// retry helper treats them as retryable. - #[cfg(test)] - pub(super) fn force_next_transient_failures(&self, n: usize) { - self.force_transient_failures.store(n, Ordering::SeqCst); + #[cfg(not(test))] + #[inline(always)] + fn consume_test_transient_failure_rename(&self) -> Result<()> { + Ok(()) } - /// Test introspection: how many forced transient failures are still - /// queued. Lets tests verify the retry helper drained the queue. + /// Queue `n` test-only forced transient FS failures for the write + /// stage. The next `n` calls inside the `fs::write(tmp)` retry loop + /// return a `__TEST_TRANSIENT__` error before the underlying FS op + /// runs; the retry helper treats them as retryable. #[cfg(test)] - pub(super) fn remaining_forced_failures(&self) -> usize { - self.force_transient_failures.load(Ordering::SeqCst) + pub(super) fn force_next_write_failures(&self, n: usize) { + self.force_transient_failures_write + .store(n, Ordering::SeqCst); + } + + /// Queue `n` test-only forced transient FS failures for the rename + /// stage. Separate from the write counter so tests can exercise the + /// rename retry loop in isolation (PR #3364 review feedback). + #[cfg(test)] + pub(super) fn force_next_rename_failures(&self, n: usize) { + self.force_transient_failures_rename + .store(n, Ordering::SeqCst); + } + + /// Test introspection: how many forced write-stage failures are still + /// queued. + #[cfg(test)] + pub(super) fn remaining_forced_write_failures(&self) -> usize { + self.force_transient_failures_write.load(Ordering::SeqCst) + } + + /// Test introspection: how many forced rename-stage failures are still + /// queued. + #[cfg(test)] + pub(super) fn remaining_forced_rename_failures(&self) -> usize { + self.force_transient_failures_rename.load(Ordering::SeqCst) } fn encrypt_optional(&self, value: Option<&str>) -> Result> { @@ -1409,6 +1457,21 @@ fn default_now_rfc3339() -> String { Utc::now().to_rfc3339() } +/// Decrement an `AtomicUsize` failure-injection counter by one if it is +/// non-zero, returning a `__TEST_TRANSIENT__` error so `is_transient_fs_error` +/// classifies the failure as retryable. Used by both per-stage consumers in +/// `write_persisted_locked` (test-only). +#[cfg(test)] +fn consume_one(counter: &AtomicUsize) -> Result<()> { + if counter.load(Ordering::SeqCst) == 0 { + return Ok(()); + } + counter.fetch_sub(1, Ordering::SeqCst); + Err(anyhow::anyhow!( + "__TEST_TRANSIENT__ injected transient FS failure" + )) +} + fn parse_profile_kind(value: &str) -> Result { match value { "oauth" => Ok(AuthProfileKind::OAuth), diff --git a/src/openhuman/credentials/profiles_tests.rs b/src/openhuman/credentials/profiles_tests.rs index fb04f8e05..1b7a3519b 100644 --- a/src/openhuman/credentials/profiles_tests.rs +++ b/src/openhuman/credentials/profiles_tests.rs @@ -690,93 +690,89 @@ fn auth_profile_kind_serde_roundtrip() { assert_eq!(json, "\"token\""); } -// ── Regression coverage for Sentry TAURI-RUST-92J / #3355 ───────────────── +// ── Regression coverage for Sentry TAURI-RUST-92J / #3355 / #3364 ───────── // // `write_persisted_locked` retries transient Windows FS errors // (`is_transient_fs_error` family — `ERROR_SHARING_VIOLATION` (32), // `ERROR_ACCESS_DENIED` (5), `ERROR_DELETE_PENDING` (303), etc.) via -// `retry_with_backoff`. Matches the sibling `.lock`-create retry that -// already closed OPENHUMAN-TAURI-H1 / H8 — the JSON `fs::write` + -// `fs::rename` path was the missing partial. +// `retry_with_backoff` on BOTH the `fs::write(tmp)` and the +// `fs::rename(tmp -> auth-profiles.json)` stages. Matches the sibling +// `.lock`-create retry that already closed OPENHUMAN-TAURI-H1 / H8 — the +// JSON `fs::write` + `fs::rename` path was the missing partial. // -// `force_next_transient_failures` is the `#[cfg(test)]`-only injection point -// — it consumes one queued failure per retry attempt and returns an error -// whose chain contains `__TEST_TRANSIENT__`, which `is_transient_fs_error` -// recognises as retryable on every platform (see `src/openhuman/util.rs`). +// Failure injection is now split per stage (`force_next_write_failures` and +// `force_next_rename_failures`) so each retry loop can be exercised in +// isolation. Originally a single shared counter, addressed in #3364 review +// where the rename retry path was line-covered but not behaviour-covered +// because the write stage drained every queued failure first. +// +// Each `#[cfg(test)]` consumer returns an error whose chain contains +// `__TEST_TRANSIENT__`, which `is_transient_fs_error` recognises as +// retryable on every platform (see `src/openhuman/util.rs`). #[test] -fn write_persisted_locked_retries_one_shot_transient() { +fn write_stage_retries_one_shot_transient() { let tmp = TempDir::new().unwrap(); let store = AuthProfilesStore::new(tmp.path(), false); - // Queue one forced transient FS failure — the first retry attempt - // returns `__TEST_TRANSIENT__`, the second runs the real `fs::write` and - // succeeds. `upsert_profile` therefore returns Ok and the queue drains. - store.force_next_transient_failures(1); + // First write call returns the test sentinel; second runs the real + // `fs::write` and succeeds. Rename stage is untouched. + store.force_next_write_failures(1); - let profile = AuthProfile::new_token("anthropic", "default", "tok-1".into()); + let profile = AuthProfile::new_token("anthropic", "default", "tok-w1".into()); store .upsert_profile(profile.clone(), true) - .expect("retry should absorb the single transient failure"); + .expect("retry should absorb the single write-stage transient"); - assert_eq!( - store.remaining_forced_failures(), - 0, - "retry helper must have consumed the queued forced failure" - ); + assert_eq!(store.remaining_forced_write_failures(), 0); + assert_eq!(store.remaining_forced_rename_failures(), 0); - // Round-trip the profile to prove the store wrote real bytes after the retry. let data = store.load().unwrap(); assert!(data.profiles.contains_key(&profile.id)); } #[test] -fn write_persisted_locked_absorbs_burst_of_transients() { +fn write_stage_absorbs_burst_of_transients() { let tmp = TempDir::new().unwrap(); let store = AuthProfilesStore::new(tmp.path(), false); - // Queue 5 forced transient failures — fewer than the retry budget - // (PERSIST_RETRY_ATTEMPTS = 6) so the 6th attempt succeeds. Covers the - // common "AV holds destination for a few hundred ms" case which was the - // root cause of TAURI-RUST-92J — the file genuinely lands on disk after - // the helper waits out the transient. - store.force_next_transient_failures(5); + // 5 forced write failures — fewer than the retry budget + // (PERSIST_RETRY_ATTEMPTS = 6), so the 6th attempt runs the real write + // and succeeds. Covers the common "AV holds destination for a few + // hundred ms" case which was the root cause of TAURI-RUST-92J. + store.force_next_write_failures(5); - let profile = AuthProfile::new_token("anthropic", "default", "tok-burst".into()); + let profile = AuthProfile::new_token("anthropic", "default", "tok-w-burst".into()); store .upsert_profile(profile.clone(), true) - .expect("retry must absorb a burst of transient failures within budget"); + .expect("retry must absorb a burst of write-stage transients within budget"); - assert_eq!( - store.remaining_forced_failures(), - 0, - "retry helper must drain every queued failure before succeeding" - ); + assert_eq!(store.remaining_forced_write_failures(), 0); + assert_eq!(store.remaining_forced_rename_failures(), 0); let data = store.load().unwrap(); let loaded = data .profiles .get(&profile.id) .expect("profile must round-trip after retry"); - assert_eq!(loaded.token.as_deref(), Some("tok-burst")); + assert_eq!(loaded.token.as_deref(), Some("tok-w-burst")); } #[test] -fn write_persisted_locked_exhausts_retries_on_persistent_transient() { +fn write_stage_exhausts_retries_on_persistent_transient() { let tmp = TempDir::new().unwrap(); let store = AuthProfilesStore::new(tmp.path(), false); - // Queue more forced failures than the retry budget for the write stage - // (PERSIST_RETRY_ATTEMPTS = 6) — every retry returns the test sentinel, - // so `retry_with_backoff` ultimately surfaces the failed-after-N-attempts - // error. Genuinely unrecoverable failures still surface to Sentry as - // honest signal; this is not a noise-suppression layer. - store.force_next_transient_failures(6); + // 6 forced failures — the full retry budget — so every attempt returns + // the sentinel and `retry_with_backoff` ultimately surfaces the + // failed-after-N-attempts error. Genuinely unrecoverable failures still + // reach Sentry as honest signal; not a noise-suppression layer. + store.force_next_write_failures(6); - let profile = AuthProfile::new_token("anthropic", "default", "tok-2".into()); + let profile = AuthProfile::new_token("anthropic", "default", "tok-w2".into()); let err = store .upsert_profile(profile, true) - .expect_err("persistent transient must exhaust retries and surface as Err"); + .expect_err("persistent write-stage transient must exhaust retries and surface as Err"); let chain = format!("{err:?}"); assert!( @@ -788,3 +784,94 @@ fn write_persisted_locked_exhausts_retries_on_persistent_transient() { "retry helper must annotate the exhausted attempts count: {chain}" ); } + +#[test] +fn rename_stage_retries_one_shot_transient() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + // No write-stage injection — write runs clean on the first attempt. + // The first rename attempt returns the sentinel; the second succeeds. + // This is the path the headline of PR #3364 was about: previously the + // shared-counter design left this loop with line coverage but no + // behaviour coverage. + store.force_next_rename_failures(1); + + let profile = AuthProfile::new_token("anthropic", "default", "tok-r1".into()); + store + .upsert_profile(profile.clone(), true) + .expect("retry should absorb the single rename-stage transient"); + + assert_eq!(store.remaining_forced_write_failures(), 0); + assert_eq!(store.remaining_forced_rename_failures(), 0); + + let data = store.load().unwrap(); + assert!(data.profiles.contains_key(&profile.id)); + + // Successful rename consumes the tmp; directory should hold only the + // final `auth-profiles.json` (plus the `.lock`, if still present from + // the operation). No orphaned tmp files even after retry. + let parent = store.path().parent().unwrap(); + let leaked: Vec<_> = std::fs::read_dir(parent) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| { + e.file_name() + .to_string_lossy() + .contains("auth-profiles.json.tmp.") + }) + .collect(); + assert!( + leaked.is_empty(), + "successful rename must consume the tmp, not orphan it: {leaked:?}" + ); +} + +#[test] +fn rename_stage_exhausts_retries_and_cleans_up_tmp() { + let tmp = TempDir::new().unwrap(); + let store = AuthProfilesStore::new(tmp.path(), false); + + // Full retry budget on the rename stage — every attempt returns the + // sentinel, so `retry_with_backoff` surfaces failed-after-N-attempts. + // This is the test the shared-counter design could not express — the + // write stage previously drained the queue before the rename closure + // ever ran, so the rename's outer `with_context` ("Failed to replace + // auth profile store") was unreachable from a green test. + store.force_next_rename_failures(6); + + let profile = AuthProfile::new_token("anthropic", "default", "tok-r2".into()); + let err = store + .upsert_profile(profile, true) + .expect_err("persistent rename-stage transient must exhaust retries and surface as Err"); + + let chain = format!("{err:?}"); + assert!( + chain.contains("Failed to replace auth profile store"), + "rename-stage outer with_context must be preserved for Sentry fingerprint stability: {chain}" + ); + assert!( + chain.contains("replace auth profile store failed after"), + "retry helper must annotate the exhausted attempts count for the rename stage: {chain}" + ); + + // Best-effort tmp cleanup: the rename retry exhausted, but the + // best-effort `fs::remove_file(&tmp_path)` in `write_persisted_locked` + // should have removed the orphaned `auth-profiles.json.tmp.{pid}.{nanos}`. + // (Pre-#3364-followup this test would fail because the tmp was leaked + // on every sustained-failure poll.) + let parent = store.path().parent().unwrap(); + let leaked: Vec<_> = std::fs::read_dir(parent) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| { + e.file_name() + .to_string_lossy() + .contains("auth-profiles.json.tmp.") + }) + .collect(); + assert!( + leaked.is_empty(), + "rename exhaustion must trigger best-effort tmp cleanup; leaked: {leaked:?}" + ); +}