hive-agent: fix login-detection race with a fixed-baseline check (#3057)

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
+  `wait_for_login` takes a `since: SystemTime` baseline (the instant of
-  newer mtime), so stale credentials on disk at the 401 don't trigger an
+  the 401 that parked it, or `login::NO_PRIOR_FAILURE` at cold boot) and
-  instant false-resume, and leftover session-history files don't read as a
+  resumes only once a credential file's mtime postdates it — so stale
-  live session after a logout + container recreate.
+  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

@@ -11,7 +11,7 @@
 
 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 +110,27 @@ impl LoginState {
     }
 }
 
-/// Block until the bound `~/.claude/` dir contains a session that
+/// Baseline for [`has_fresh_credentials`] when the caller has no specific
-/// post-dates this call, polling on a `poll_ms` interval (min 2s).
+/// prior-failure instant to compare against (the cold-boot call site, where
-/// Flips `state` to `Online` when login lands; caller resumes its
+/// `has_session` already established no credential file exists yet — so
-/// serve loop. Snapshots the dir at entry and only resumes when the
+/// there's nothing that could be mistaken for stale). Any real file's mtime
-/// snapshot advances (mtime OR file-count change), avoiding the
+/// postdates the Unix epoch, so this baseline behaves as "resume the first
-/// infinite-401 loop a bare-existence check would produce when stale
+/// time a credential file with a readable mtime shows up."
-/// credentials are already on disk. Mtime-snapshot resumption rationale
+pub const NO_PRIOR_FAILURE: SystemTime = SystemTime::UNIX_EPOCH;
-/// and `DirSnapshot` two-axis design: see
+
+/// 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 +141,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 +155,14 @@ 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);
     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) {
             tracing::info!("claude session refreshed — entering turn loop");
             *state.lock().unwrap() = LoginState::Online;
             bus.emit_status("online");
@@ -154,59 +171,34 @@ pub async fn wait_for_login(
     }
 }
 
-/// Snapshot of the credential files (see [`CRED_FILE_NAMES`]) in the dir at a
+/// True if `dir` holds a credential file (see [`CRED_FILE_NAMES`]) whose
-/// point in time: how many are present + newest `mtime` across them. The two
+/// mtime is strictly newer than `since`, or — fallback — if credential
-/// axes are both load-bearing for `wait_for_login`'s refresh check
+/// files exist but *none* of their mtimes are readable at all (exotic fs,
-/// (`session_refreshed`): mtime catches the common case (re-login overwrites
+/// NFS quirks where `metadata()`/`modified()` errors on every file). That
-/// an existing credentials file in-place), `file_count` catches the
+/// fallback is the same "don't block forever on a signal we can't read"
-/// pathological case where `meta.modified()` errors on every file (exotic fs,
+/// protection an earlier entry-snapshot-diffing design covered with a raw
-/// NFS quirks) so the mtime axis stays `None` forever but a new credential
+/// file-count comparison; reframed here as "no readable mtime, but
-/// file still triggers a resume. Defaults to `{0, None}` on `read_dir` failure
+/// something is there" since there's no entry snapshot to diff against
-/// (missing or unreadable dir) — `wait_for_login` then resumes when a
+/// under a fixed-baseline comparison.
-/// credential file first appears.
+fn has_fresh_credentials(dir: &Path, since: SystemTime) -> bool {
-#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
-struct DirSnapshot {
-    file_count: usize,
-    newest_mtime: Option<std::time::SystemTime>,
-}
-
-fn snapshot_dir(dir: &Path) -> DirSnapshot {
     let Ok(entries) = std::fs::read_dir(dir) else {
-        return DirSnapshot::default();
+        return false;
     };
-    let mut snap = DirSnapshot::default();
+    let mut any_file = false;
+    let mut any_readable_mtime = false;
     for entry in entries.flatten() {
         if !is_cred_file(&entry) {
             continue;
         }
-        snap.file_count += 1;
+        any_file = true;
         let Ok(meta) = entry.metadata() else { continue };
         let Ok(mtime) = meta.modified() else { continue };
-        if snap.newest_mtime.is_none_or(|cur| mtime > cur) {
+        any_readable_mtime = true;
-            snap.newest_mtime = Some(mtime);
+        if mtime > since {
+            return true;
         }
     }
-    snap
+    any_file && !any_readable_mtime
-}
-
-/// 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,
-    }
 }
 
 #[cfg(test)]
@@ -214,7 +206,7 @@ mod tests {
     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]
     fn has_session_only_counts_credential_files() {
@@ -230,26 +222,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!(
-        assert_eq!(
+            !has_fresh_credentials(dir.path(), NO_PRIOR_FAILURE),
-            snap.file_count, 0,
+            "history files must not count as a session"
-            "history files must not count as session"
         );
     }
 
     #[test]
-    fn snapshot_dir_empty_dir_is_default() {
+    fn has_fresh_credentials_missing_dir_is_false() {
-        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() {
         // 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 +240,71 @@ mod tests {
             .unwrap()
             .path()
             .join("never-created-subdir");
-        let snap = snapshot_dir(&missing);
+        assert!(!has_fresh_credentials(&missing, NO_PRIOR_FAILURE));
-        assert_eq!(snap, DirSnapshot::default());
     }
 
     #[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!(!has_fresh_credentials(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
+        assert!(has_fresh_credentials(dir.path(), NO_PRIOR_FAILURE));
-        // 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));
     }
 
     #[test]
-    fn session_refreshed_first_login_flips_on_cred_file() {
+    fn has_fresh_credentials_stale_creds_dont_resume() {
-        // Empty-dir snapshot → a credential file appearing means a fresh
+        // Stale credentials.json already predates the failure baseline;
-        // login landed. First-time login semantics.
+        // wait_for_login must NOT resume — it would loop straight into
-        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
         // another 401-failing turn.
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        let snapshot = snapshot_dir(dir.path());
+        let since = SystemTime::now() + Duration::from_secs(1);
-        assert_eq!(snapshot.file_count, 1);
+        assert!(!has_fresh_credentials(dir.path(), since));
-        // No change to the file → loop must NOT exit.
-        assert!(!session_refreshed(snapshot, snapshot_dir(dir.path())));
     }
 
     #[test]
-    fn session_refreshed_after_creds_rewrite_flips() {
+    fn has_fresh_credentials_login_already_landed_before_baseline_check_resumes() {
-        // After the stale-creds snapshot, the operator's `/login/code`
+        // The exact race this design closes: a fresh login's mtime
-        // flow lands a refreshed credentials file — its mtime bumps
+        // postdates `since` even though it was written before this
-        // strictly past the snapshot and wait_for_login resumes.
+        // 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!(has_fresh_credentials(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!(has_fresh_credentials(dir.path(), since));
     }
 
     #[test]
-    fn session_refreshed_snapshot_with_future_mtime_doesnt_flip() {
+    fn has_fresh_credentials_future_baseline_doesnt_resume() {
-        // Defensive: a snapshot set to a future timestamp (e.g. clock
+        // Defensive: a baseline set to a future timestamp (e.g. clock
-        // skew between snapshot and probe) must keep waiting until a
+        // skew) must keep waiting until a file's mtime actually exceeds
-        // file's mtime actually exceeds it, not return on first poll.
+        // 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 {
+        let since = SystemTime::now() + Duration::from_hours(1);
-            file_count: 1,
+        assert!(!has_fresh_credentials(dir.path(), since));
-            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())));
     }
 }
+
+// The "credential file exists but its mtime is unreadable at all" fallback
+// (see `has_fresh_credentials`'s doc comment) isn'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. The branch exists for
+// a real production failure mode (NFS quirks), not a hypothetical.

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;
         }