diff --git a/docs/turn-loop/README.md b/docs/turn-loop/README.md index ee382050..6c8f3aa7 100644 --- a/docs/turn-loop/README.md +++ b/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 diff --git a/hive-agent/src/login.rs b/hive-agent/src/login.rs index 2e63ad6e..b6f028a8 100644 --- a/hive-agent/src/login.rs +++ b/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>, 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 { - 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, } -/// 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, -) -> 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. diff --git a/hive-agent/src/main.rs b/hive-agent/src/main.rs index cca08786..bc16baf4 100644 --- a/hive-agent/src/main.rs +++ b/hive-agent/src/main.rs @@ -612,14 +612,7 @@ async fn serve_main(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( 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; }