hive-agent: guard the unreadable-mtime login fallback against stale re-triggers

docs/turn-loop/README.md

@@ -52,10 +52,16 @@ agents) runs:
 - **Login detection** — both boot (`login::has_session`, Online vs
   NeedsLogin) and `wait_for_login`'s resume check key off the credential
   files in `login::CRED_FILE_NAMES` (the set `/logout` deletes).
-  `wait_for_login` resumes only when that set changes (a new file or a
-  newer mtime), so stale credentials on disk at the 401 don't trigger an
-  instant false-resume, and leftover session-history files don't read as a
-  live session after a logout + container recreate.
+  `wait_for_login` takes a `since: SystemTime` baseline (the instant of
+  the 401 that parked it, or `login::NO_PRIOR_FAILURE` at cold boot) and
+  resumes only once a credential file's mtime postdates it — so stale
+  credentials already on disk at the 401 don't trigger an instant
+  false-resume, and a login that lands *before* `wait_for_login` even
+  starts polling still resumes correctly (baselining on a fixed instant
+  rather than an entry-time directory snapshot is what closes that race).
+  Leftover session-history files still don't read as a live session after
+  a logout + container recreate (`has_session`/`is_cred_file` scope to
+  `CRED_FILE_NAMES` either way).
 
 ## Harness binary shape
 

hive-agent/src/login.rs

@@ -9,9 +9,10 @@
 //! so a "contains any regular file" check would wrongly report `Online` after
 //! a logout + container recreate and burn a turn 401-ing before it reroutes.
 
+use std::collections::HashSet;
 use std::path::{Path, PathBuf};
 use std::sync::{Arc, Mutex};
-use std::time::Duration;
+use std::time::{Duration, SystemTime};
 
 use crate::events::Bus;
 
@@ -110,14 +111,27 @@ impl LoginState {
     }
 }
 
-/// Block until the bound `~/.claude/` dir contains a session that
-/// post-dates this call, polling on a `poll_ms` interval (min 2s).
-/// Flips `state` to `Online` when login lands; caller resumes its
-/// serve loop. Snapshots the dir at entry and only resumes when the
-/// snapshot advances (mtime OR file-count change), avoiding the
-/// infinite-401 loop a bare-existence check would produce when stale
-/// credentials are already on disk. Mtime-snapshot resumption rationale
-/// and `DirSnapshot` two-axis design: see
+/// Baseline for [`has_fresh_credentials`] when the caller has no specific
+/// prior-failure instant to compare against (the cold-boot call site, where
+/// `has_session` already established no credential file exists yet — so
+/// there's nothing that could be mistaken for stale). Any real file's mtime
+/// postdates the Unix epoch, so this baseline behaves as "resume the first
+/// time a credential file with a readable mtime shows up."
+pub const NO_PRIOR_FAILURE: SystemTime = SystemTime::UNIX_EPOCH;
+
+/// Block until the bound `~/.claude/` dir holds a credential file whose
+/// mtime postdates `since`, polling on a `poll_ms` interval (min 2s). Flips
+/// `state` to `Online` when login lands; caller resumes its serve loop.
+///
+/// `since` is normally the instant the caller detected the failure that
+/// parked it here (an API 401) — see [`NO_PRIOR_FAILURE`] for the cold-boot
+/// case, which has no such instant and doesn't need one. Comparing against
+/// a fixed instant rather than an entry-time directory snapshot is what
+/// avoids the infinite-401 loop a bare-existence check would produce when
+/// stale credentials are already on disk: a stale file's mtime predates
+/// `since` (keep waiting), a fresh login's mtime postdates it (resume) —
+/// including a login that lands *before* this call even starts polling,
+/// which an entry-snapshot comparison could miss entirely. Full rationale:
 /// [`docs/turn-loop/::The loop`](../../docs/turn-loop/README.md).
 ///
 /// # Panics
@@ -128,6 +142,7 @@ pub async fn wait_for_login(
     state: Arc<Mutex<LoginState>>,
     bus: &Bus,
     poll_ms: u64,
+    since: SystemTime,
 ) {
     tracing::warn!(
         claude_dir = %claude_dir.display(),
@@ -141,11 +156,19 @@ pub async fn wait_for_login(
     // small empty file, so re-entering this function after a transient
     // operator action is a no-op for the on-disk state.
     bus.emit_status("needs_login_idle");
-    let snapshot = snapshot_dir(claude_dir);
+    // Baseline for the unreadable-mtime fallback in `has_fresh_credentials`
+    // — captured once, here, not recomputed per poll. See that fn's doc
+    // comment for why a *standing* unreadable-mtime file must not
+    // re-trigger the fallback on every iteration.
+    let baseline_unreadable = unreadable_mtime_names(claude_dir);
     let probe = Duration::from_millis(poll_ms.max(2000));
     loop {
         tokio::time::sleep(probe).await;
-        if session_refreshed(snapshot, snapshot_dir(claude_dir)) {
+        // A login that already landed by the time this first poll runs
+        // (the race this baseline design closes) is caught here on the
+        // very first iteration — nothing about the check depends on how
+        // long we've been polling.
+        if has_fresh_credentials(claude_dir, since, &baseline_unreadable) {
             tracing::info!("claude session refreshed — entering turn loop");
             *state.lock().unwrap() = LoginState::Online;
             bus.emit_status("online");
@@ -154,67 +177,79 @@ pub async fn wait_for_login(
     }
 }
 
-/// Snapshot of the credential files (see [`CRED_FILE_NAMES`]) in the dir at a
-/// point in time: how many are present + newest `mtime` across them. The two
-/// axes are both load-bearing for `wait_for_login`'s refresh check
-/// (`session_refreshed`): mtime catches the common case (re-login overwrites
-/// an existing credentials file in-place), `file_count` catches the
-/// pathological case where `meta.modified()` errors on every file (exotic fs,
-/// NFS quirks) so the mtime axis stays `None` forever but a new credential
-/// file still triggers a resume. Defaults to `{0, None}` on `read_dir` failure
-/// (missing or unreadable dir) — `wait_for_login` then resumes when a
-/// credential file first appears.
-#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
-struct DirSnapshot {
-    file_count: usize,
-    newest_mtime: Option<std::time::SystemTime>,
+/// Names of credential files (see [`CRED_FILE_NAMES`]) currently present in
+/// `dir` whose mtime is *not* readable (`metadata()`/`modified()` errors —
+/// exotic fs, NFS quirks). Used once, at [`wait_for_login`]'s entry, as the
+/// baseline its unreadable-mtime fallback compares against.
+fn unreadable_mtime_names(dir: &Path) -> HashSet<String> {
+    let Ok(entries) = std::fs::read_dir(dir) else {
+        return HashSet::new();
+    };
+    entries
+        .flatten()
+        .filter(is_cred_file)
+        .filter(|e| e.metadata().and_then(|m| m.modified()).is_err())
+        .filter_map(|e| e.file_name().to_str().map(str::to_owned))
+        .collect()
 }
 
-fn snapshot_dir(dir: &Path) -> DirSnapshot {
+/// True if `dir` holds a credential file (see [`CRED_FILE_NAMES`]) whose
+/// mtime is strictly newer than `since`, or — fallback — if a credential
+/// file's mtime is unreadable at all (exotic fs, NFS quirks) *and its name
+/// isn't in `baseline_unreadable`* — i.e. it's a new occurrence since
+/// [`wait_for_login`] started polling, not a standing condition.
+///
+/// The `baseline_unreadable` guard matters: without it, a *stale* file
+/// whose mtime happens to be permanently unreadable would satisfy the
+/// fallback on every single poll (it's always "present with no readable
+/// mtime"), resuming instantly and reintroducing the exact infinite-401
+/// loop this whole mechanism exists to prevent. Requiring the name to be
+/// new mirrors what an earlier entry-snapshot-diffing design covered with
+/// a raw file-count comparison — "something *changed*", not "something
+/// *is present*".
+fn has_fresh_credentials(
+    dir: &Path,
+    since: SystemTime,
+    baseline_unreadable: &HashSet<String>,
+) -> bool {
     let Ok(entries) = std::fs::read_dir(dir) else {
-        return DirSnapshot::default();
+        return false;
     };
-    let mut snap = DirSnapshot::default();
     for entry in entries.flatten() {
         if !is_cred_file(&entry) {
             continue;
         }
-        snap.file_count += 1;
         let Ok(meta) = entry.metadata() else { continue };
-        let Ok(mtime) = meta.modified() else { continue };
-        if snap.newest_mtime.is_none_or(|cur| mtime > cur) {
-            snap.newest_mtime = Some(mtime);
+        match meta.modified() {
+            Ok(mtime) if mtime > since => return true,
+            Ok(_) => {}
+            Err(_) => {
+                let is_new = entry
+                    .file_name()
+                    .to_str()
+                    .is_some_and(|name| !baseline_unreadable.contains(name));
+                if is_new {
+                    return true;
+                }
+            }
         }
     }
-    snap
-}
-
-/// Has the credentials dir been written since `prev`? Used as the
-/// exit condition for `wait_for_login`:
-///
-/// - `file_count` changed → something was added or removed, treat as
-///   refresh (covers the "all files have unreadable mtime" edge case).
-/// - `newest_mtime` advanced → existing file was rewritten in place
-///   (the common claude re-login path).
-/// - prev had no mtime (empty or all-unreadable) and now has one →
-///   first useful signal we've seen, treat as refresh.
-fn session_refreshed(prev: DirSnapshot, now: DirSnapshot) -> bool {
-    if now.file_count != prev.file_count {
-        return true;
-    }
-    match (prev.newest_mtime, now.newest_mtime) {
-        (None, Some(_)) => true,
-        (Some(p), Some(n)) => n > p,
-        _ => false,
-    }
+    false
 }
 
 #[cfg(test)]
 mod tests {
+    use std::collections::HashSet;
     use std::fs;
     use std::time::{Duration, SystemTime};
 
-    use super::{DirSnapshot, has_session, session_refreshed, snapshot_dir};
+    use super::{NO_PRIOR_FAILURE, has_fresh_credentials, has_session};
+
+    /// Test-only shorthand: most cases don't exercise the unreadable-mtime
+    /// fallback, so they don't care about its baseline.
+    fn fresh(dir: &std::path::Path, since: SystemTime) -> bool {
+        has_fresh_credentials(dir, since, &HashSet::new())
+    }
 
     #[test]
     fn has_session_only_counts_credential_files() {
@@ -230,26 +265,17 @@ mod tests {
     }
 
     #[test]
-    fn snapshot_dir_ignores_non_credential_files() {
+    fn has_fresh_credentials_ignores_non_credential_files() {
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join("history.jsonl"), b"{}").unwrap();
-        let snap = snapshot_dir(dir.path());
-        assert_eq!(
-            snap.file_count, 0,
-            "history files must not count as session"
+        assert!(
+            !fresh(dir.path(), NO_PRIOR_FAILURE),
+            "history files must not count as a session"
         );
     }
 
     #[test]
-    fn snapshot_dir_empty_dir_is_default() {
-        let dir = tempfile::tempdir().unwrap();
-        let snap = snapshot_dir(dir.path());
-        assert_eq!(snap.file_count, 0);
-        assert!(snap.newest_mtime.is_none());
-    }
-
-    #[test]
-    fn snapshot_dir_missing_dir_is_default() {
+    fn has_fresh_credentials_missing_dir_is_false() {
         // Defensive: a nonexistent dir must NOT panic. Bind mounts that
         // disappear mid-poll (host purge during operator intervention)
         // would otherwise crash the harness.
@@ -257,92 +283,79 @@ mod tests {
             .unwrap()
             .path()
             .join("never-created-subdir");
-        let snap = snapshot_dir(&missing);
-        assert_eq!(snap, DirSnapshot::default());
+        assert!(!fresh(&missing, NO_PRIOR_FAILURE));
     }
 
     #[test]
-    fn snapshot_dir_picks_latest_mtime_and_counts_files() {
+    fn has_fresh_credentials_first_login_resumes() {
+        // Cold-boot case: no prior failure instant, empty dir. A credential
+        // file appearing is a fresh login by construction — its mtime
+        // postdates the epoch baseline.
         let dir = tempfile::tempdir().unwrap();
+        assert!(!fresh(dir.path(), NO_PRIOR_FAILURE));
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        // Sleep so the second file's mtime is strictly greater than
-        // the first on filesystems with low timestamp resolution.
-        std::thread::sleep(Duration::from_millis(20));
-        let newer_path = dir.path().join("mcp-needs-auth-cache.json");
-        fs::write(&newer_path, b"{}").unwrap();
-        let snap = snapshot_dir(dir.path());
-        assert_eq!(snap.file_count, 2);
-        let newer_meta = fs::metadata(&newer_path).unwrap().modified().unwrap();
-        assert_eq!(snap.newest_mtime, Some(newer_meta));
+        assert!(fresh(dir.path(), NO_PRIOR_FAILURE));
     }
 
     #[test]
-    fn session_refreshed_first_login_flips_on_cred_file() {
-        // Empty-dir snapshot → a credential file appearing means a fresh
-        // login landed. First-time login semantics.
-        let dir = tempfile::tempdir().unwrap();
-        let snapshot = snapshot_dir(dir.path());
-        assert!(!session_refreshed(snapshot, snapshot_dir(dir.path())));
-        fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        assert!(session_refreshed(snapshot, snapshot_dir(dir.path())));
-    }
-
-    #[test]
-    fn session_refreshed_stale_creds_dont_flip_immediately() {
-        // Stale credentials.json already exists at entry; wait_for_login
-        // must NOT immediately return — it would loop straight into
+    fn has_fresh_credentials_stale_creds_dont_resume() {
+        // Stale credentials.json already predates the failure baseline;
+        // wait_for_login must NOT resume — it would loop straight into
         // another 401-failing turn.
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        let snapshot = snapshot_dir(dir.path());
-        assert_eq!(snapshot.file_count, 1);
-        // No change to the file → loop must NOT exit.
-        assert!(!session_refreshed(snapshot, snapshot_dir(dir.path())));
+        let since = SystemTime::now() + Duration::from_secs(1);
+        assert!(!fresh(dir.path(), since));
     }
 
     #[test]
-    fn session_refreshed_after_creds_rewrite_flips() {
-        // After the stale-creds snapshot, the operator's `/login/code`
-        // flow lands a refreshed credentials file — its mtime bumps
-        // strictly past the snapshot and wait_for_login resumes.
+    fn has_fresh_credentials_login_already_landed_before_baseline_check_resumes() {
+        // The exact race this design closes: a fresh login's mtime
+        // postdates `since` even though it was written before this
+        // function is ever called — no entry-time snapshot to miss it
+        // against.
+        let dir = tempfile::tempdir().unwrap();
+        let since = SystemTime::now();
+        std::thread::sleep(Duration::from_millis(20));
+        fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
+        assert!(fresh(dir.path(), since));
+    }
+
+    #[test]
+    fn has_fresh_credentials_rewrite_after_failure_resumes() {
+        // The operator's `/login/code` flow rewrites the stale file in
+        // place; its mtime bumps strictly past the failure baseline.
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        let snapshot = snapshot_dir(dir.path());
+        let since = SystemTime::now();
         std::thread::sleep(Duration::from_millis(20));
         fs::write(dir.path().join(".credentials.json"), b"{\"v\":2}").unwrap();
-        assert!(session_refreshed(snapshot, snapshot_dir(dir.path())));
+        assert!(fresh(dir.path(), since));
     }
 
     #[test]
-    fn session_refreshed_snapshot_with_future_mtime_doesnt_flip() {
-        // Defensive: a snapshot set to a future timestamp (e.g. clock
-        // skew between snapshot and probe) must keep waiting until a
-        // file's mtime actually exceeds it, not return on first poll.
+    fn has_fresh_credentials_future_baseline_doesnt_resume() {
+        // Defensive: a baseline set to a future timestamp (e.g. clock
+        // skew) must keep waiting until a file's mtime actually exceeds
+        // it, not resume on the strength of an existing file alone.
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        let snapshot = DirSnapshot {
-            file_count: 1,
-            newest_mtime: Some(SystemTime::now() + Duration::from_hours(1)),
-        };
-        assert!(!session_refreshed(snapshot, snapshot_dir(dir.path())));
-    }
-
-    #[test]
-    fn session_refreshed_count_change_flips_when_mtime_unreadable() {
-        // Defensive: if all files have unreadable `meta.modified()`
-        // (exotic fs / NFS), newest_mtime stays `None` forever — but
-        // file_count axis still catches new files appearing. Simulated
-        // here by forging a snapshot with file_count=1 + no mtime, then
-        // writing a second file.
-        let dir = tempfile::tempdir().unwrap();
-        fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        let forged = DirSnapshot {
-            file_count: 1,
-            newest_mtime: None,
-        };
-        fs::write(dir.path().join("mcp-needs-auth-cache.json"), b"{}").unwrap();
-        // Real snapshot has file_count=2, so refresh fires even
-        // though the mtime axis would be inconclusive.
-        assert!(session_refreshed(forged, snapshot_dir(dir.path())));
+        let since = SystemTime::now() + Duration::from_hours(1);
+        assert!(!fresh(dir.path(), since));
     }
 }
+
+// The unreadable-mtime fallback and its `baseline_unreadable` guard (see
+// `has_fresh_credentials`'s doc comment) aren't covered by a unit test here
+// — forging an unreadable `metadata()`/`modified()` call portably needs
+// platform-specific setup (a permission-denied file, an exotic fs) this
+// test module doesn't have infrastructure for, and a normal file's mtime is
+// always readable in a test tempdir, so there's no way to reach the `Err`
+// arm (or meaningfully exercise `baseline_unreadable`, which only matters
+// inside it) without faking that. The branch exists for a real production
+// failure mode (NFS quirks), not a hypothetical — a review pass on this
+// module caught that the original version of this fallback ignored `since`
+// entirely and could re-trigger the infinite-401 loop this file exists to
+// prevent; the current shape (only a *new* unreadable-mtime name resumes)
+// is reasoned about in the doc comment above since it can't be asserted
+// here.

hive-agent/src/main.rs

@@ -612,7 +612,14 @@ async fn serve_main<S: Surface>(socket: &Path, poll_ms: u64) -> Result<()> {
     let (todo_wake, todos_store) =
         spawn_todo_socket(reminder_store.clone(), question_store.clone(), &bus);
     if matches!(initial, LoginState::NeedsLogin) {
-        login::wait_for_login(&claude_dir, login_state.clone(), &bus, poll_ms).await;
+        login::wait_for_login(
+            &claude_dir,
+            login_state.clone(),
+            &bus,
+            poll_ms,
+            login::NO_PRIOR_FAILURE,
+        )
+        .await;
     } else {
         // Clear any stale `hyperhive-needs-login` sentinel left over
         // from a prior boot — `online` status writes the sentinel
@@ -792,11 +799,16 @@ async fn serve_loop<S: Surface>(
         apply_todo_wake_checked(ctrl.todo_wake_checked, &mut todo_miss_streak, &bus);
         if ctrl.auth_failed {
             *login_state.lock().unwrap() = LoginState::NeedsLogin;
+            // Baseline the resume check on *this instant*, not on a
+            // directory snapshot taken after `wait_for_login` starts
+            // polling — closes the race where a login lands between the
+            // 401 and the first poll. See `wait_for_login`'s doc comment.
             login::wait_for_login(
                 &claude_dir,
                 login_state.clone(),
                 &bus,
                 u64::try_from(interval.as_millis()).unwrap_or(2000),
+                std::time::SystemTime::now(),
             )
             .await;
         }