diff --git a/docs/turn-loop/README.md b/docs/turn-loop/README.md index 6c8f3aa7..ee382050 100644 --- a/docs/turn-loop/README.md +++ b/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 diff --git a/hive-agent/src/login.rs b/hive-agent/src/login.rs index b6f028a8..717e0206 100644 --- a/hive-agent/src/login.rs +++ b/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 -/// 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 +141,7 @@ pub async fn wait_for_login( state: Arc>, 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 -/// 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, -} - -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 credential +/// files exist but *none* of their mtimes are readable at all (exotic fs, +/// NFS quirks where `metadata()`/`modified()` errors on every file). That +/// fallback is the same "don't block forever on a signal we can't read" +/// protection an earlier entry-snapshot-diffing design covered with a raw +/// file-count comparison; reframed here as "no readable mtime, but +/// something is there" since there's no entry snapshot to diff against +/// under a fixed-baseline comparison. +fn has_fresh_credentials(dir: &Path, since: SystemTime) -> bool { 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) { - snap.newest_mtime = Some(mtime); + any_readable_mtime = true; + if mtime > since { + 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, - } + any_file && !any_readable_mtime } #[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_eq!( - snap.file_count, 0, - "history files must not count as session" + assert!( + !has_fresh_credentials(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 +240,71 @@ mod tests { .unwrap() .path() .join("never-created-subdir"); - let snap = snapshot_dir(&missing); - assert_eq!(snap, DirSnapshot::default()); + assert!(!has_fresh_credentials(&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!(!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 - // 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!(has_fresh_credentials(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!(!has_fresh_credentials(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!(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() { - // 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!(!has_fresh_credentials(dir.path(), since)); } } + +// 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. diff --git a/hive-agent/src/main.rs b/hive-agent/src/main.rs index bc16baf4..cca08786 100644 --- a/hive-agent/src/main.rs +++ b/hive-agent/src/main.rs @@ -612,7 +612,14 @@ 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).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( 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; }