fix(pool): prevent panic on Instant subtraction underflow

[SunnyYYLin]

What problem does this solve?

Instant::now() - ttl panics when the monotonic clock has not yet elapsed ttl worth of time. This is a logic error in the original code, not a platform-specific bug.

Closes #1459

Discord Discussion URL: https://discordapp.com/channels/1491295327620169908/1530676804996825088

Review Contract

Goal

Fix the panic caused by Instant::now() - ttl when the elapsed time since the monotonic clock epoch is less than ttl (e.g. session_ttl_hours=48 on a machine with < 48h uptime).

Non-goals

No behavioral change. No change to TTL semantics.

Accepted Residual Risks

None - saturating_duration_since returns Duration::ZERO on underflow, which correctly classifies the session as fresh (not idle).

Acceptance Criteria

• cargo test -p openab-core classify_idle --lib passes (3 tests)
• cargo clippy --workspace --features unified -- -D warnings clean
• No behavioral change when now > last_active + ttl (normal case)

Follow-ups

No deferred work - the fix is complete and self-contained.

Why was this never reported?

Instant - Duration in Rust calls checked_sub().expect() - it panics on underflow. The original code computes Instant::now() - ttl to get a cutoff point, then compares last_active < cutoff.

This panics whenever the monotonic clock has not elapsed ttl since its epoch. On all platforms the monotonic clock starts at boot, so the condition is uptime < TTL. With the default session_ttl_hours=48, this means any machine that has been up for less than 48 hours will panic on the first reaper tick.

It went unreported because:

1. Linux servers typically have uptimes of weeks/months, far exceeding 48h
2. The panic only triggers on the reaper tick (every ~60s), not at startup
3. Windows machines reboot frequently but were not commonly used as openab hosts until unified mode

The bug is platform-agnostic. A freshly booted Linux machine with session_ttl_hours=48 would panic too.

Proposed Solution

Replace let cutoff = Instant::now() - ttl; last_active < cutoff with now.saturating_duration_since(last_active) > ttl. last_active is always <= now, so saturating_duration_since never underflows.

At a Glance

Before (panics when uptime < TTL):
  cutoff = Instant::now() - ttl        ← underflow
  last_active < cutoff

After (always safe):
  now.saturating_duration_since(last_active) > ttl

Prior Art & Industry Research

Not applicable - trivial arithmetic safety fix.

Alternatives Considered

• checked_sub + unwrap_or: more verbose, same effect.
• Instant::now().checked_duration_since(last_active): returns Option, requires extra branching.

Validation

• cargo test -p openab-core classify_idle --lib - 3 passed
• cargo clippy --workspace --features unified -- -D warnings - clean

[chaodu-obk]

LGTM ✅ - The change removes the idle-cleanup panic without changing TTL semantics.

Note

No blocking correctness, safety, or integration issues were found in the exact-SHA review.

What This PR Does

It fixes a panic in idle-session cleanup when the configured TTL is longer than the elapsed monotonic-clock time. The implementation replaces panic-prone Instant subtraction with elapsed-duration comparison while preserving the existing stale-session policy.

How It Works

cleanup_idle captures one now value and converts ttl_secs to a Duration. classify_idle then evaluates now.saturating_duration_since(last_active) > ttl, or treats a dead connection as stale. The strict > comparison preserves the prior last_active < cutoff boundary behavior, while saturation removes the underflow panic.

Findings

#	Severity	Finding	Location
1	🟢	The fix removes the panic-prone subtraction and preserves normal TTL and dead-session behavior.	crates/openab-core/src/acp/pool.rs:84-90, 659-745

Finding Details

🟢 F1: Safe elapsed-time classification

The new calculation is equivalent to the old cutoff comparison whenever last_active is not later than now, which is how the connection timestamp is maintained. If a future timestamp ever appears, saturation conservatively keeps the session fresh instead of panicking or evicting it prematurely.

Baseline Check

• PR opened: 2026-07-25T20:24:46Z
• Declared base: main
• Merge-base: 685973307bf79d6848c21b33a5bd55b6aea1c433
• Main already has: the idle cleanup path and the Instant cutoff comparison.
• Net-new value: removes the Instant::now() - ttl underflow panic while retaining the existing cleanup semantics.

Addressing External Reviewer Feedback

No external review comments or threads were present for this round.

Independent Review Summary

• Reviewer A (correctness): no additional findings; the arithmetic and boundary behavior are equivalent in the normal case.
• Reviewer B (architecture): no API, concurrency, or integration regression identified.
• Reviewer C (testing): local execution was unavailable because the environment has no Rust toolchain; GitHub validation for this SHA was successful, and the changed unit tests cover stale, dead, and fresh paths.

Validation

• GitHub validate check: successful on 2690bd3d9a42ab36bec70fd5766fd736472eb57d.
• GitHub check check: successful on 2690bd3d9a42ab36bec70fd5766fd736472eb57d.
• GitHub unified smoke-test matrix: completed successfully for the PR head.
• Local targeted command cargo test -p openab-core classify_idle --lib: not run because cargo, rustc, and rustup are unavailable in the review environment.

5. Three Reasons We Might Not Need This PR

1. The failure may appear only on recently booted hosts - Long-running hosts can hide the defect, reducing the chance it is noticed without targeted testing.
2. The patch is narrowly scoped - It does not add a dedicated regression test for the exact clock-epoch underflow scenario, so future changes still need to preserve this invariant.
3. Saturation can mask a future timestamp invariant violation - A timestamp later than now is treated as fresh; this is the safe direction, but telemetry could be useful if such a violation becomes possible.

These are decision considerations, not blocking findings; the current patch is minimal and correct.

[chaodu-obk]

LGTM ✅ - No blocking findings; F1 is praise only.

Consolidated review: #1457 (comment)