From 2690bd3d9a42ab36bec70fd5766fd736472eb57d Mon Sep 17 00:00:00 2001 From: SunnyYYLin Date: Sun, 26 Jul 2026 04:24:15 +0800 Subject: [PATCH 1/3] fix(pool): prevent Windows panic on idle TTL subtraction Replace Instant::now() - ttl with saturating_duration_since to avoid overflow panic when the monotonic clock age is smaller than the TTL (e.g. fresh boot with 48h session_ttl_hours). Add regression test for the edge case. --- crates/openab-core/src/acp/pool.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/crates/openab-core/src/acp/pool.rs b/crates/openab-core/src/acp/pool.rs index 394d9f260..bd3442edc 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())); } } @@ -856,23 +862,20 @@ 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)); + assert!(classify_idle(last_active, true, now, std::time::Duration::from_secs(60))); } #[test] fn classify_idle_marks_stale_by_death() { 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, std::time::Duration::from_secs(60))); } #[test] fn classify_idle_keeps_fresh_alive_sessions() { let now = Instant::now(); - let cutoff = now - std::time::Duration::from_secs(60); - assert!(!classify_idle(now, true, cutoff)); + assert!(!classify_idle(now, true, now, std::time::Duration::from_secs(60))); } #[test] From e739f810cff753149492a5d7947bd75f338cad3f Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Mon, 10 Aug 2026 13:40:09 -0400 Subject: [PATCH 2/3] test(pool): production-path underflow regression + rustfmt the changed tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two round-3 review findings. F1 — deterministic production-path regression. The helper tests pass under both the old and new `classify_idle`, because the panic happened in `cleanup_idle` *before* `classify_idle` was ever called: `Instant::now() - ttl` overflows whenever the TTL exceeds the monotonic clock's age. `cleanup_idle_survives_ttl_larger_than_monotonic_clock` calls `cleanup_idle(u64::MAX)` on an empty pool, which reproduces that unconditionally on every platform — the subtraction ran before the pool was touched, so no sessions are needed. Verified to fail on the pre-fix arithmetic with "overflow when subtracting duration from instant", and to pass as written. Two helper cases added alongside it: - `classify_idle_keeps_sessions_at_the_exact_ttl_boundary` pins that `elapsed > ttl` kept the strict boundary of `last_active < now - ttl`. - `classify_idle_keeps_sessions_touched_after_now` covers `last_active > now`, which is reachable in production: `now` is sampled once at the top of `cleanup_idle`, so a turn can finish and touch a connection before that connection's `try_lock` is reached. `saturating_duration_since` yields ZERO there and the session stays fresh. F2 — rustfmt. Binding `ttl` to a local keeps every call on one line, which is rustfmt-clean and reads better than the six-line wrapped form rustfmt would otherwise produce. `cargo fmt -p openab-core -- --check` now reports zero diffs for `pool.rs`; the rest of the crate is left untouched (it has pre-existing formatting drift and no CI `cargo fmt` gate). Also imports `SessionPool` into the test module, which the new async test needs — `cargo clippy` does not compile test targets, so this only surfaces under `cargo test`. --- crates/openab-core/src/acp/pool.rs | 59 ++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 4 deletions(-) diff --git a/crates/openab-core/src/acp/pool.rs b/crates/openab-core/src/acp/pool.rs index bd3442edc..78c3f40d0 100644 --- a/crates/openab-core/src/acp/pool.rs +++ b/crates/openab-core/src/acp/pool.rs @@ -816,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; @@ -861,21 +861,72 @@ mod tests { #[test] fn classify_idle_marks_stale_by_time() { + let ttl = std::time::Duration::from_secs(60); let now = Instant::now(); let last_active = now - std::time::Duration::from_secs(120); - assert!(classify_idle(last_active, true, now, 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(); - assert!(classify_idle(now, false, now, std::time::Duration::from_secs(60))); + 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, std::time::Duration::from_secs(60))); + 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 now = Instant::now(); + assert!(!classify_idle(now - ttl, 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(); + 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] From 803f7ac7397d50e25e6a56a21ad1d6a2d6702ebc Mon Sep 17 00:00:00 2001 From: chaodu-agent Date: Mon, 10 Aug 2026 14:24:54 -0400 Subject: [PATCH 3/3] test(pool): build idle fixtures forward so they cannot underflow either Round-4 F1. The two stale/boundary fixtures constructed their timestamps as `Instant::now() - duration`, which is the exact panic this PR removes from production: on a host whose monotonic clock is younger than the offset, the test panics before `classify_idle` is ever called. The boundary case I added last commit made that worse by introducing a new instance of it. Both now build forward from `last_active`: - exact boundary: `now = last_active + ttl` (elapsed == ttl, still fresh) - stale-by-time: `now = last_active + ttl + 60s` (elapsed > ttl, stale) Assertions and intent are unchanged; the fixtures no longer depend on host uptime. Scope: the `better_candidate_*` and `purge_session_entries` fixtures in this module have the same latent pattern, but they are outside this diff and round 3 explicitly deferred them to a separate test-hardening follow-up, so they are left alone here. --- crates/openab-core/src/acp/pool.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/openab-core/src/acp/pool.rs b/crates/openab-core/src/acp/pool.rs index 78c3f40d0..62175f78e 100644 --- a/crates/openab-core/src/acp/pool.rs +++ b/crates/openab-core/src/acp/pool.rs @@ -862,8 +862,12 @@ mod tests { #[test] fn classify_idle_marks_stale_by_time() { let ttl = std::time::Duration::from_secs(60); - let now = Instant::now(); - let last_active = now - std::time::Duration::from_secs(120); + // 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)); } @@ -887,8 +891,9 @@ mod tests { #[test] fn classify_idle_keeps_sessions_at_the_exact_ttl_boundary() { let ttl = std::time::Duration::from_secs(60); - let now = Instant::now(); - assert!(!classify_idle(now - ttl, true, now, ttl)); + 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