diff --git a/crates/openab-core/src/acp/pool.rs b/crates/openab-core/src/acp/pool.rs index 394d9f260..62175f78e 100644 --- a/crates/openab-core/src/acp/pool.rs +++ b/crates/openab-core/src/acp/pool.rs @@ -81,8 +81,13 @@ fn get_or_insert_gate(map: &mut HashMap>>, key: &str) -> A } /// Returns true when a session should be treated as stale during idle cleanup. -fn classify_idle(last_active: Instant, alive: bool, cutoff: Instant) -> bool { - last_active < cutoff || !alive +fn classify_idle( + last_active: Instant, + alive: bool, + now: Instant, + ttl: std::time::Duration, +) -> bool { + now.saturating_duration_since(last_active) > ttl || !alive } /// Returns true when a locked, in-flight session has exceeded the hung threshold. @@ -654,7 +659,8 @@ impl SessionPool { } pub async fn cleanup_idle(&self, ttl_secs: u64) { - let cutoff = Instant::now() - std::time::Duration::from_secs(ttl_secs); + let now = Instant::now(); + let ttl = std::time::Duration::from_secs(ttl_secs); let hung_threshold = std::time::Duration::from_secs(self.hung_threshold_secs); let (snapshot, activity_map, cancel_map, pgid_map) = { @@ -736,7 +742,7 @@ impl SessionPool { activity.touch(); } } - if classify_idle(conn.last_active, conn.alive(), cutoff) { + if classify_idle(conn.last_active, conn.alive(), now, ttl) { stale.push((key, conn_handle, conn.acp_session_id.clone())); } } @@ -810,7 +816,7 @@ impl SessionPool { mod tests { use super::{ better_candidate, classify_hung, classify_idle, get_or_insert_gate, purge_session_entries, - remove_if_same_handle, PoolState, + remove_if_same_handle, PoolState, SessionPool, }; use crate::acp::connection::SessionActivity; use std::collections::HashMap; @@ -855,24 +861,77 @@ mod tests { #[test] fn classify_idle_marks_stale_by_time() { - let now = Instant::now(); - let cutoff = now - std::time::Duration::from_secs(60); - let last_active = now - std::time::Duration::from_secs(120); - assert!(classify_idle(last_active, true, cutoff)); + let ttl = std::time::Duration::from_secs(60); + // Build fixtures forward from `last_active`, never backward from `now`: + // `Instant::now() - d` carries the very underflow panic this PR removes + // from production, and would fire on a host whose monotonic clock is + // younger than `d`. + let last_active = Instant::now(); + let now = last_active + ttl + std::time::Duration::from_secs(60); + assert!(classify_idle(last_active, true, now, ttl)); } #[test] fn classify_idle_marks_stale_by_death() { + let ttl = std::time::Duration::from_secs(60); let now = Instant::now(); - let cutoff = now - std::time::Duration::from_secs(60); - assert!(classify_idle(now, false, cutoff)); + assert!(classify_idle(now, false, now, ttl)); } #[test] fn classify_idle_keeps_fresh_alive_sessions() { + let ttl = std::time::Duration::from_secs(60); + let now = Instant::now(); + assert!(!classify_idle(now, true, now, ttl)); + } + + /// The expiry boundary is strict: elapsed == ttl is still fresh. Pins that + /// the switch from `last_active < now - ttl` to `elapsed > ttl` did not + /// shift the comparison by one tick. + #[test] + fn classify_idle_keeps_sessions_at_the_exact_ttl_boundary() { + let ttl = std::time::Duration::from_secs(60); + let last_active = Instant::now(); + let now = last_active + ttl; + assert!(!classify_idle(last_active, true, now, ttl)); + } + + /// `now` is sampled once at the top of `cleanup_idle`, but a turn can finish + /// and touch `last_active` before this connection's `try_lock` is reached — + /// so `last_active > now` is reachable in production. `saturating_duration_since` + /// yields ZERO there, which must classify the session as fresh rather than + /// underflowing. + #[test] + fn classify_idle_keeps_sessions_touched_after_now() { + let ttl = std::time::Duration::from_secs(60); let now = Instant::now(); - let cutoff = now - std::time::Duration::from_secs(60); - assert!(!classify_idle(now, true, cutoff)); + assert!(!classify_idle(now + ttl, true, now, ttl)); + } + + /// Regression for the panic this PR fixes: `cleanup_idle` used to compute + /// `Instant::now() - ttl` before touching the pool, which panics whenever the + /// TTL exceeds the monotonic clock's age (uptime). An oversized TTL reproduces + /// that unconditionally on every platform — this test panics on the old + /// implementation and completes on the current one. The helper tests above + /// cannot catch it: the panic happened before `classify_idle` was called. + #[tokio::test] + async fn cleanup_idle_survives_ttl_larger_than_monotonic_clock() { + let pool = SessionPool::new( + crate::config::AgentConfig { + command: "/bin/true".into(), + args: vec![], + working_dir: "/tmp".into(), + env: HashMap::new(), + inherit_env: vec![], + command_explicit: true, + }, + 1, + crate::config::default_prompt_hard_timeout_secs(), + HashMap::new(), + ); + + // No sessions are needed: the panicking subtraction ran unconditionally. + pool.cleanup_idle(u64::MAX).await; } #[test]