test(credentials): cover rename-stage retry + best-effort tmp cleanup (#3364 follow-up) (#3398)

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<AtomicUsize>,
+    force_transient_failures_write: Arc<AtomicUsize>,
+    /// `#[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<AtomicUsize>,
 }
 
 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<Option<String>> {
@@ -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<AuthProfileKind> {
     match value {
         "oauth" => Ok(AuthProfileKind::OAuth),

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:?}"
+    );
+}