docs/turn-loop/README.md

@@ -52,16 +52,10 @@ 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` 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).
+  `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.
 
 ## Harness binary shape
 

hive-agent/src/login.rs

@@ -9,10 +9,9 @@
 //! 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, SystemTime};
+use std::time::Duration;
 
 use crate::events::Bus;
 
@@ -111,27 +110,14 @@ impl LoginState {
     }
 }
 
-/// 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:
+/// 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
 /// [`docs/turn-loop/::The loop`](../../docs/turn-loop/README.md).
 ///
 /// # Panics
@@ -142,7 +128,6 @@ pub async fn wait_for_login(
     state: Arc<Mutex<LoginState>>,
     bus: &Bus,
     poll_ms: u64,
-    since: SystemTime,
 ) {
     tracing::warn!(
         claude_dir = %claude_dir.display(),
@@ -156,19 +141,11 @@ 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");
-    // 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 snapshot = snapshot_dir(claude_dir);
     let probe = Duration::from_millis(poll_ms.max(2000));
     loop {
         tokio::time::sleep(probe).await;
-        // 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) {
+        if session_refreshed(snapshot, snapshot_dir(claude_dir)) {
             tracing::info!("claude session refreshed — entering turn loop");
             *state.lock().unwrap() = LoginState::Online;
             bus.emit_status("online");
@@ -177,79 +154,67 @@ pub async fn wait_for_login(
     }
 }
 
-/// 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()
+/// 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>,
 }
 
-/// 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 {
+fn snapshot_dir(dir: &Path) -> DirSnapshot {
     let Ok(entries) = std::fs::read_dir(dir) else {
-        return false;
+        return DirSnapshot::default();
     };
+    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 };
-        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;
-                }
-            }
+        let Ok(mtime) = meta.modified() else { continue };
+        if snap.newest_mtime.is_none_or(|cur| mtime > cur) {
+            snap.newest_mtime = Some(mtime);
         }
     }
-    false
+    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,
+    }
 }
 
 #[cfg(test)]
 mod tests {
-    use std::collections::HashSet;
     use std::fs;
     use std::time::{Duration, SystemTime};
 
-    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())
-    }
+    use super::{DirSnapshot, has_session, session_refreshed, snapshot_dir};
 
     #[test]
     fn has_session_only_counts_credential_files() {
@@ -265,17 +230,26 @@ mod tests {
     }
 
     #[test]
-    fn has_fresh_credentials_ignores_non_credential_files() {
+    fn snapshot_dir_ignores_non_credential_files() {
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join("history.jsonl"), b"{}").unwrap();
-        assert!(
-            !fresh(dir.path(), NO_PRIOR_FAILURE),
-            "history files must not count as a session"
+        let snap = snapshot_dir(dir.path());
+        assert_eq!(
+            snap.file_count, 0,
+            "history files must not count as session"
         );
     }
 
     #[test]
-    fn has_fresh_credentials_missing_dir_is_false() {
+    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() {
         // Defensive: a nonexistent dir must NOT panic. Bind mounts that
         // disappear mid-poll (host purge during operator intervention)
         // would otherwise crash the harness.
@@ -283,79 +257,92 @@ mod tests {
             .unwrap()
             .path()
             .join("never-created-subdir");
-        assert!(!fresh(&missing, NO_PRIOR_FAILURE));
+        let snap = snapshot_dir(&missing);
+        assert_eq!(snap, DirSnapshot::default());
     }
 
     #[test]
-    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.
+    fn snapshot_dir_picks_latest_mtime_and_counts_files() {
         let dir = tempfile::tempdir().unwrap();
-        assert!(!fresh(dir.path(), NO_PRIOR_FAILURE));
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        assert!(fresh(dir.path(), NO_PRIOR_FAILURE));
+        // 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));
     }
 
     #[test]
-    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
+    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
         // another 401-failing turn.
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        let since = SystemTime::now() + Duration::from_secs(1);
-        assert!(!fresh(dir.path(), since));
+        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())));
     }
 
     #[test]
-    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.
+    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.
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        let since = SystemTime::now();
+        let snapshot = snapshot_dir(dir.path());
         std::thread::sleep(Duration::from_millis(20));
         fs::write(dir.path().join(".credentials.json"), b"{\"v\":2}").unwrap();
-        assert!(fresh(dir.path(), since));
+        assert!(session_refreshed(snapshot, snapshot_dir(dir.path())));
     }
 
     #[test]
-    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.
+    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.
         let dir = tempfile::tempdir().unwrap();
         fs::write(dir.path().join(".credentials.json"), b"{}").unwrap();
-        let since = SystemTime::now() + Duration::from_hours(1);
-        assert!(!fresh(dir.path(), since));
+        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())));
     }
 }
-
-// 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,14 +612,7 @@ 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,
-            login::NO_PRIOR_FAILURE,
-        )
-        .await;
+        login::wait_for_login(&claude_dir, login_state.clone(), &bus, poll_ms).await;
     } else {
         // Clear any stale `hyperhive-needs-login` sentinel left over
         // from a prior boot — `online` status writes the sentinel
@@ -799,16 +792,11 @@ 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;
         }