diff --git a/hive-agent/src/login.rs b/hive-agent/src/login.rs index 717e0206..2e63ad6e 100644 --- a/hive-agent/src/login.rs +++ b/hive-agent/src/login.rs @@ -9,6 +9,7 @@ //! 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}; @@ -155,6 +156,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 probe = Duration::from_millis(poll_ms.max(2000)); loop { tokio::time::sleep(probe).await; @@ -162,7 +168,7 @@ pub async fn wait_for_login( // (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) { + 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"); @@ -171,43 +177,80 @@ 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() +} + /// 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 { +/// 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 { let Ok(entries) = std::fs::read_dir(dir) else { return false; }; - let mut any_file = false; - let mut any_readable_mtime = false; for entry in entries.flatten() { if !is_cred_file(&entry) { continue; } - any_file = true; let Ok(meta) = entry.metadata() else { continue }; - let Ok(mtime) = meta.modified() else { continue }; - any_readable_mtime = true; - if mtime > since { - return true; + 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; + } + } } } - any_file && !any_readable_mtime + 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()) + } + #[test] fn has_session_only_counts_credential_files() { let dir = tempfile::tempdir().unwrap(); @@ -226,7 +269,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); fs::write(dir.path().join("history.jsonl"), b"{}").unwrap(); assert!( - !has_fresh_credentials(dir.path(), NO_PRIOR_FAILURE), + !fresh(dir.path(), NO_PRIOR_FAILURE), "history files must not count as a session" ); } @@ -240,7 +283,7 @@ mod tests { .unwrap() .path() .join("never-created-subdir"); - assert!(!has_fresh_credentials(&missing, NO_PRIOR_FAILURE)); + assert!(!fresh(&missing, NO_PRIOR_FAILURE)); } #[test] @@ -249,9 +292,9 @@ mod tests { // 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)); + assert!(!fresh(dir.path(), NO_PRIOR_FAILURE)); fs::write(dir.path().join(".credentials.json"), b"{}").unwrap(); - assert!(has_fresh_credentials(dir.path(), NO_PRIOR_FAILURE)); + assert!(fresh(dir.path(), NO_PRIOR_FAILURE)); } #[test] @@ -262,7 +305,7 @@ mod tests { let dir = tempfile::tempdir().unwrap(); fs::write(dir.path().join(".credentials.json"), b"{}").unwrap(); let since = SystemTime::now() + Duration::from_secs(1); - assert!(!has_fresh_credentials(dir.path(), since)); + assert!(!fresh(dir.path(), since)); } #[test] @@ -275,7 +318,7 @@ mod tests { 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)); + assert!(fresh(dir.path(), since)); } #[test] @@ -287,7 +330,7 @@ mod tests { let since = SystemTime::now(); std::thread::sleep(Duration::from_millis(20)); fs::write(dir.path().join(".credentials.json"), b"{\"v\":2}").unwrap(); - assert!(has_fresh_credentials(dir.path(), since)); + assert!(fresh(dir.path(), since)); } #[test] @@ -298,13 +341,21 @@ mod tests { let dir = tempfile::tempdir().unwrap(); fs::write(dir.path().join(".credentials.json"), b"{}").unwrap(); let since = SystemTime::now() + Duration::from_hours(1); - assert!(!has_fresh_credentials(dir.path(), since)); + assert!(!fresh(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. +// 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.