From 1e67f5624993551b22a54efd5a43a92041ced6d7 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 2 Sep 2026 13:47:26 +0200 Subject: [PATCH] hive-sh4re: one saturating_age for every loose-end producer `age_seconds` is documented on the LooseEnd enum as saturating to zero on any clock anomaly, but the derivation was in three places: hive-c0re had a named `saturating_age` helper with tests, and the in-agent socket server hand-rolled the same two lines twice, untested. Move the helper to hive-sh4re::inbox, beside the enum whose contract it implements and inside the one crate both producers already depend on. Its three tests move with it (not dropped) and gain two arms: the whole-i64 range, where the saturating_sub is what stops the subtraction overflowing, and a far-past control so those zeros are the clamp firing rather than the function bottoming out on large inputs. The two clamps are not redundant, which is what `to_loose_end`'s doc got wrong: it credited "saturating" for the zero, but saturating_sub bottoms out at i64::MIN, still negative. The try_from is what yields 0. Also cover the two projections themselves, which is the part the shared helper cannot: that a reminder ages from created_at rather than due_at, and a todo from updated_at, with a future timestamp reading 0 through both and a past-timestamp control on each. --- hive-agent/src/todo_server.rs | 135 ++++++++++++++++++++++++++++++++-- hive-c0re/src/loose_ends.rs | 31 +------- hive-sh4re/src/inbox.rs | 63 ++++++++++++++++ 3 files changed, 193 insertions(+), 36 deletions(-) diff --git a/hive-agent/src/todo_server.rs b/hive-agent/src/todo_server.rs index c7fa7828..17fc71fd 100644 --- a/hive-agent/src/todo_server.rs +++ b/hive-agent/src/todo_server.rs @@ -26,7 +26,7 @@ use std::sync::Arc; use anyhow::{Context, Result}; use hive_agent_sock::{Request, Response}; -use hive_sh4re::inbox::LooseEnd; +use hive_sh4re::inbox::{LooseEnd, saturating_age}; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::net::{UnixListener, UnixStream}; use tokio::sync::Notify; @@ -430,13 +430,12 @@ fn no_reminders_store() -> Response { /// it's due). fn reminder_to_loose_end(r: Reminder) -> LooseEnd { let now = chrono::Utc::now().timestamp(); - let age = u64::try_from(now.saturating_sub(r.created_at.timestamp())).unwrap_or(0); LooseEnd::Reminder { id: r.id, owner: crate::identity::label(), message: r.message, due_at: r.due_at, - age_seconds: age, + age_seconds: saturating_age(now, r.created_at.timestamp()), } } @@ -448,16 +447,140 @@ fn err(e: &anyhow::Error) -> Response { } /// Map a stored [`Todo`] to a [`LooseEnd::Todo`], deriving `age_seconds` -/// from `updated_at` (saturating so a backwards clock step reads 0). +/// from `updated_at` via [`saturating_age`]. fn to_loose_end(t: Todo) -> LooseEnd { let now = chrono::Utc::now().timestamp(); - let age = u64::try_from(now.saturating_sub(t.updated_at.timestamp())).unwrap_or(0); LooseEnd::Todo { id: t.id, subsystem: t.subsystem, subsystem_key: t.subsystem_key, summary: t.summary, source: t.source, - age_seconds: age, + age_seconds: saturating_age(now, t.updated_at.timestamp()), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::{DateTime, Duration, Utc}; + + /// `saturating_age`'s own arms are exact (`hive-sh4re::inbox`); what + /// is only checkable here is *which* stored timestamp each projection + /// feeds it, and both read the clock internally — so these assert a + /// range wide enough that a slow runner can't fail them. + const SLACK: u64 = 600; + + /// Not asserted anywhere below: `LooseEnd::Reminder.owner`. It comes + /// from `identity::label()`, which reads a process-global env var + /// that `identity`'s own tests set and restore — in the same test + /// binary, in parallel. Any value assertion here would be a race, not + /// a check. + fn reminder(created_at: DateTime, due_at: DateTime) -> Reminder { + Reminder { + id: 7, + message: "check the gate".to_owned(), + file_path: None, + due_at, + created_at, + } + } + + fn todo(updated_at: DateTime) -> Todo { + Todo { + id: 11, + subsystem: "matrix".to_owned(), + subsystem_key: Some("!room:x".to_owned()), + summary: "3 unread".to_owned(), + source: Some("#ops".to_owned()), + updated_at, + } + } + + #[test] + fn a_reminder_ages_from_created_at_not_from_when_it_is_due() { + // Scheduled 600s ago, due an hour out. Reading the age off + // `due_at` would clamp to 0, so the two fields are separable. + let r = reminder( + Utc::now() - Duration::seconds(600), + Utc::now() + Duration::seconds(3600), + ); + let due = r.due_at; + match reminder_to_loose_end(r) { + LooseEnd::Reminder { + id, + message, + due_at, + age_seconds, + .. + } => { + assert_eq!(id, 7); + assert_eq!(message, "check the gate"); + assert_eq!(due_at, due, "due_at passes through untouched"); + assert!( + (600..600 + SLACK).contains(&age_seconds), + "age {age_seconds} should track created_at, not due_at" + ); + } + other => panic!("expected a Reminder loose end, got {other:?}"), + } + } + + #[test] + fn a_todo_ages_from_updated_at_and_carries_its_dedup_key() { + let t = todo(Utc::now() - Duration::seconds(600)); + match to_loose_end(t) { + LooseEnd::Todo { + id, + subsystem, + subsystem_key, + summary, + source, + age_seconds, + } => { + assert_eq!(id, 11); + assert_eq!(subsystem, "matrix"); + // Distinct values: the two `Option` fields are + // adjacent and same-typed, so a swap has to be visible. + assert_eq!(subsystem_key.as_deref(), Some("!room:x")); + assert_eq!(source.as_deref(), Some("#ops")); + assert_eq!(summary, "3 unread"); + assert!((600..600 + SLACK).contains(&age_seconds)); + } + other => panic!("expected a Todo loose end, got {other:?}"), + } + } + + #[test] + fn a_timestamp_in_the_future_reads_as_zero_through_both_projections() { + // Not a hypothetical: these stores are written by in-container + // producers, and `updated_at` crossing `now` is exactly what a + // clock sync during a turn produces. + let ahead = Utc::now() + Duration::seconds(3600); + let behind = Utc::now() - Duration::seconds(600); + + let (future_r, past_r) = (age_of_reminder(ahead), age_of_reminder(behind)); + let (future_t, past_t) = (age_of_todo(ahead), age_of_todo(behind)); + + assert_eq!(future_r, 0); + assert_eq!(future_t, 0); + // Controls: same call, past timestamp — so the zeros above are + // the clamp firing, not the projections always emitting 0. + assert!(past_r >= 600, "reminder control read {past_r}"); + assert!(past_t >= 600, "todo control read {past_t}"); + } + + fn age_of_reminder(created_at: DateTime) -> u64 { + match reminder_to_loose_end(reminder(created_at, Utc::now())) { + LooseEnd::Reminder { age_seconds, .. } => age_seconds, + other => panic!("expected a Reminder loose end, got {other:?}"), + } + } + + fn age_of_todo(updated_at: DateTime) -> u64 { + match to_loose_end(todo(updated_at)) { + LooseEnd::Todo { age_seconds, .. } => age_seconds, + other => panic!("expected a Todo loose end, got {other:?}"), + } } } diff --git a/hive-c0re/src/loose_ends.rs b/hive-c0re/src/loose_ends.rs index feb67481..408ee926 100644 --- a/hive-c0re/src/loose_ends.rs +++ b/hive-c0re/src/loose_ends.rs @@ -20,7 +20,7 @@ use anyhow::Result; use chrono::Utc; -use hive_sh4re::inbox::LooseEnd; +use hive_sh4re::inbox::{LooseEnd, saturating_age}; use crate::coordinator::Coordinator; @@ -93,32 +93,3 @@ pub fn hive_wide(coord: &Coordinator) -> Result> { } Ok(out) } - -fn saturating_age(now: i64, then: i64) -> u64 { - let delta = now.saturating_sub(then); - u64::try_from(delta).unwrap_or(0) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn saturating_age_handles_clock_back_step() { - // `now` < `then`: caller's clock went backwards between rows. - // We saturate to 0 rather than returning a negative or - // wrapping around to ~u64::MAX (which would render as "27 - // billion years ago" in the wake prompt). - assert_eq!(saturating_age(100, 200), 0); - } - - #[test] - fn saturating_age_normal_case() { - assert_eq!(saturating_age(1_000_000, 999_990), 10); - } - - #[test] - fn saturating_age_zero_when_equal() { - assert_eq!(saturating_age(42, 42), 0); - } -} diff --git a/hive-sh4re/src/inbox.rs b/hive-sh4re/src/inbox.rs index 7b8d8db3..05d12eb0 100644 --- a/hive-sh4re/src/inbox.rs +++ b/hive-sh4re/src/inbox.rs @@ -188,6 +188,23 @@ pub enum LooseEnd { }, } +/// Age in seconds between two unix timestamps, clamped to 0 when `then` +/// is in the future. This is what backs the `age_seconds` promise on +/// every [`LooseEnd`] variant above, so it lives beside the enum rather +/// than in either producer: hive-c0re derives approval ages here, and +/// hive-agent's in-agent socket server derives reminder + todo ages. +/// +/// Both clamps are load-bearing and neither substitutes for the other: +/// `saturating_sub` keeps the subtraction itself from overflowing on +/// absurd inputs, and the `try_from` is what turns a negative delta into +/// 0 — on its own `saturating_sub` saturates towards `i64::MIN`, which is +/// still negative and would wrap to ~`u64::MAX` in an `as` cast. +#[must_use] +pub fn saturating_age(now: i64, then: i64) -> u64 { + let delta = now.saturating_sub(then); + u64::try_from(delta).unwrap_or(0) +} + /// Kind discriminator for `CancelLooseEnd`. Per-kind store + /// authorisation rules live in /// `docs/process/conventions.md::Loose-ends wire shape`. @@ -198,3 +215,49 @@ pub enum CancelLooseEndKind { /// Withdraw a pending approval (manager surface only). Approval, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn saturating_age_normal_case() { + assert_eq!(saturating_age(1_000_000, 999_990), 10); + } + + #[test] + fn saturating_age_zero_when_equal() { + assert_eq!(saturating_age(42, 42), 0); + } + + #[test] + fn saturating_age_handles_clock_back_step() { + // `now` < `then`: the clock went backwards between rows. 0 beats + // both a negative and the ~u64::MAX an `as` cast would produce, + // which renders as "27 billion years ago" in the wake prompt. + assert_eq!(saturating_age(100, 200), 0); + } + + #[test] + fn a_far_future_timestamp_stays_zero_rather_than_wrapping() { + // The back-step case above is one second of skew; this is the + // whole i64 range, where the subtraction itself saturates. Both + // must land on 0, and the pair is what pins the two clamps + // together — either one alone passes one of these and not both. + assert_eq!(saturating_age(i64::MIN, i64::MAX), 0); + assert_eq!(saturating_age(0, i64::MAX), 0); + } + + #[test] + fn a_far_past_timestamp_is_a_real_age_not_a_clamp() { + // Control for the two arms above: the maximum representable + // delta still comes back as itself, so their 0 is the clamp + // firing and not this function bottoming out on large inputs. + let age = saturating_age(i64::MAX, i64::MIN); + assert_eq!(age, u64::try_from(i64::MAX).unwrap()); + assert_eq!( + saturating_age(i64::MAX, 0), + u64::try_from(i64::MAX).unwrap() + ); + } +}