From 86e277e9f067c3ccc7a766d8f72ccee238dd7809 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 00:11:40 +0200 Subject: [PATCH] refactor(services): one retry policy for wifi and networkd, with non-tautological shipped-policy tests Closes #665. Refs #646 (part 1 of two; the 2s-reconnect-loop half is left). Part 1 (#646): `wifi`'s `RetryPolicy`/`ProbeStep` (#634) and `networkd`'s `RefreshRetry`/`RefreshStep` (#645) were near-identical -- same three fields, same shipped constants (None / 500ms / 8s), and a `backoff()` body that was character-for-character the same `checked_shl`/`saturating_mul`/`min` sequence. Only a pair of hand-written cross-referencing doc comments kept them in sync, and #665 is what happened when that failed. Both now use `crate::retry::{Policy, Step}` -- a new private module in `hytte-services`, next to the existing `mod eds_retry`. The surviving shape is `networkd`'s generic `step(&Result, attempt)`, whose `Proceed` carries no payload; `wifi`'s `probe_until_conclusive` now reads the verdict off the probe's own `Result` instead of out of the step, so the inconclusive-vs- answered weighing is explicit in the caller. That is strictly stronger against #613: with no payload on `Proceed`, no code path can manufacture a `BackendChoice` out of a `ProbeError` at all. Named `retry::Policy` rather than `RetryPolicy` because `hytte_bus::RetryPolicy` already exists and is a different thing (per-call Never/Once/Backoff) -- it is referenced in `PROBE_RETRY`'s own docs, now as an explicit link. Pure refactor: no log line, delay, or control-flow branch changed. Both shipped constants keep `max_attempts: None`, `initial: 500ms`, `max_backoff: 8s`. Deliberately NOT folded in, per #646: `hytte_bus`'s `Backoff` (stateful cursor, no budget, no give-up) and `wifi/watcher.rs:131`'s `sleep(2s)` (a one-shot debounce before `return`, not a retry loop). Both are named in the new module's docs so nobody re-opens them as oversights. #646's second half -- the three fixed-2s reconnect loops in `networkd::listen`, `mpris` and `bluetooth` -- is left alone. Capping those changes delays and log wording, and the interesting part is a design question this change has no answer for: those loops have no clean success signal, so "when does the attempt counter reset?" needs deciding before a budget means anything there. Part 2 (#665): the shipped-policy tests were tautological on the delay. They asserted `step(&failed(), n) == Retry { after: policy.backoff(n) }` -- the very expression `step` computes internally -- so they pinned that `step` calls `backoff`, not that `backoff` returns a sane number. Verified the falsification on 61967dd: setting `STARTUP_REFRESH_RETRY.initial` to `Duration::ZERO` left the whole suite green (681 passed, 0 failed), while the behaviour became `sleep(Duration::ZERO)` on every retry -- a tight `error!` flood, exactly what that constant's own docs argue is impossible. The same edit to `wifi::PROBE_RETRY` on 61967dd DOES go red, confirming #665's one-sided-drift claim: #634 compared against the literal `max_backoff`, #645 replaced that with a re-computation. `retry.rs` now carries a `SHIPPED` table of every policy the crate ships, swept by two tests, so a third policy cannot be added without these assertions coming along: - the first delay is strictly greater than zero (the floor neither side had); - the ceiling is actually reached, compared against the literal `max_backoff` (matching #634's assertion, which #645 had dropped); - first retry <= 1s, `max_backoff` within [initial, 30s]. Every comparison is against a literal, never a re-computation. The per-site `the_shipped_policy_never_goes_inert` tests keep only the site-specific claim (`max_attempts: None`, argued on #613/#621) and now use `matches!`. Also corrects #665 item 2: `networkd`'s comment claimed the 2s loop only covers a `listen` that had been established and dropped, but `listen` opens with its own initial `refresh`, so it also covers one that was never established. The comment now says so, including that a post-seed recovery surfaces as that loop's `warn!` going quiet rather than as a `startup refresh RECOVERED` line. Co-Authored-By: Claude Opus 5 (1M context) --- crates/hytte-services/src/lib.rs | 1 + crates/hytte-services/src/networkd.rs | 187 +++++++---------- crates/hytte-services/src/retry.rs | 286 ++++++++++++++++++++++++++ crates/hytte-services/src/wifi/mod.rs | 243 ++++++++-------------- 4 files changed, 443 insertions(+), 274 deletions(-) create mode 100644 crates/hytte-services/src/retry.rs diff --git a/crates/hytte-services/src/lib.rs b/crates/hytte-services/src/lib.rs index 9d60106..64e5fef 100644 --- a/crates/hytte-services/src/lib.rs +++ b/crates/hytte-services/src/lib.rs @@ -33,6 +33,7 @@ pub mod places; pub mod power_profiles; pub mod recorder; pub mod resolved; +mod retry; pub mod screensaver; pub mod sensors; pub mod systemd; diff --git a/crates/hytte-services/src/networkd.rs b/crates/hytte-services/src/networkd.rs index dbc685a..e78e06f 100644 --- a/crates/hytte-services/src/networkd.rs +++ b/crates/hytte-services/src/networkd.rs @@ -44,6 +44,7 @@ use hytte_reactive::{Service, registry, spawn_supervised}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::time::Duration; +use crate::retry; use crate::wifi_backend::BackendChoice; const NETWORKD_NAME: &str = "org.freedesktop.network1"; @@ -307,41 +308,17 @@ async fn probe_link_backend() -> LinkBackend { } } -/// What to do after one startup [`refresh`] attempt in the -/// [`LinkBackend::Networkd`] arm. -/// -/// The whole retry decision lives in [`RefreshRetry::step`] — a pure function — -/// so it is unit-testable without a bus. Deliberately the same shape as -/// `wifi`'s `ProbeStep`/`RetryPolicy` (#634): the two paths retry the same class -/// of transient system-bus failure, and someone debugging one while reading the -/// journal of the other must not find two different behaviours. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -enum RefreshStep { - /// The refresh landed. Stop retrying and enter the `listen` loop. - Proceed, - /// It failed and attempts remain: wait, then refresh again. - Retry { - /// How long to wait before the next attempt. - after: Duration, - }, - /// Still failing and the attempt budget is spent: log loudly and stay inert. - /// - /// **Not reachable under the shipped [`STARTUP_REFRESH_RETRY`]**, which is - /// unbounded. Kept expressible (and tested) so a bounded policy stays one - /// field away. - GiveUp, -} - -/// Retry schedule for a failed startup [`refresh`]. -#[derive(Clone, Copy, Debug)] -struct RefreshRetry { - /// Attempt budget, counting the first refresh. `None` means "retry forever". - max_attempts: Option, - /// Delay before the first retry; doubles with each further attempt. - initial: Duration, - /// Ceiling the doubling delay is clamped to. - max_backoff: Duration, -} +// ── Startup refresh retry (issue #621) ─────────────────────────────────────── +// +// The retry *mechanism* — schedule, attempt budget, the pure `Proceed`/`Retry`/ +// `GiveUp` decision — lives in `crate::retry`, shared with `wifi`'s backend +// probe since #646. The two paths retry the same class of transient system-bus +// failure, and someone debugging one while reading the journal of the other must +// not find two different behaviours; before #646 that was kept true by a pair of +// doc comments pointing at each other by hand. +// +// What stays here is the *judgement*: why this policy is unbounded, and what +// each attempt logs. /// The policy the [`LinkBackend::Networkd`] arm runs: **unbounded** retry with /// capped backoff. @@ -375,46 +352,27 @@ struct RefreshRetry { /// **The policy is one field to change.** A bounded policy is /// `max_attempts: Some(n)` and nothing else; `the_shipped_policy_never_goes_inert` /// asserts the shipped default is not. -const STARTUP_REFRESH_RETRY: RefreshRetry = RefreshRetry { +/// +/// Unlike `wifi`'s probe this path has no verdict to weigh before consulting the +/// policy: a `refresh` either read the links or could not, and "could not" is +/// never a fact about the host here — the backend was already elected. +/// +/// The schedule's own invariants — a nonzero first delay, a ceiling that is +/// actually reached — are asserted over *every* shipped policy in `retry`'s +/// tests (#665), not here. +pub(crate) const STARTUP_REFRESH_RETRY: retry::Policy = retry::Policy { max_attempts: None, initial: Duration::from_millis(500), max_backoff: Duration::from_secs(8), }; -impl RefreshRetry { - /// Delay before the retry that follows `attempt` (1-based): [`Self::initial`] - /// doubled once per elapsed attempt, clamped to [`Self::max_backoff`]. - fn backoff(self, attempt: u32) -> Duration { - let factor = 1_u32 - .checked_shl(attempt.saturating_sub(1)) - .unwrap_or(u32::MAX); - self.initial.saturating_mul(factor).min(self.max_backoff) - } - - /// The pure retry/proceed decision. `attempt` is 1-based and counts the - /// refresh that produced `outcome`. - /// - /// Unlike `wifi`'s probe there is no verdict to weigh: a `refresh` either - /// read the links or could not, and "could not" is never a fact about the - /// host here — the backend was already elected. - fn step(self, outcome: &Result, attempt: u32) -> RefreshStep { - match outcome { - Ok(_) => RefreshStep::Proceed, - Err(_) if self.max_attempts.is_some_and(|max| attempt >= max) => RefreshStep::GiveUp, - Err(_) => RefreshStep::Retry { - after: self.backoff(attempt), - }, - } - } -} - /// Seed the link list from networkd, retrying while [`refresh`] fails. /// /// Returns `true` once a refresh has landed — i.e. once it is worth entering the /// `listen` loop. `false` means the policy stopped asking first, which the /// shipped unbounded [`STARTUP_REFRESH_RETRY`] never does. async fn seed_links( - policy: RefreshRetry, + policy: retry::Policy, links_out: &Mutable>, primary_out: &Mutable>, source_out: &Mutable, @@ -430,7 +388,7 @@ async fn seed_links( .map_or_else(String::new, ToString::to_string); match policy.step(&outcome, attempt) { - RefreshStep::Proceed => { + retry::Step::Proceed => { // Only announce a recovery if there was something to recover // from; the ordinary first-try success stays quiet. if attempt > 1 { @@ -443,7 +401,7 @@ async fn seed_links( } return true; } - RefreshStep::Retry { after } => { + retry::Step::Retry { after } => { // Logged on *every* attempt, not every Nth: the backoff caps at // 8s, so an unreachable networkd stays loud here rather than // degrading into a silent poll. See `STARTUP_REFRESH_RETRY`. @@ -460,7 +418,7 @@ async fn seed_links( tokio::time::sleep(after).await; attempt += 1; } - RefreshStep::GiveUp => { + retry::Step::GiveUp => { tracing::error!( attempts = attempt, error = %reason, @@ -531,9 +489,23 @@ impl Service for NetworkdService { { return; } - // The 2s retry below is untouched: it covers a `listen` that - // had been established and dropped, which already retried and - // is not what #621 is about. + // The 2s retry below is untouched, but not for the reason + // this comment used to give (#665): it does *not* only + // cover a `listen` that had been established and dropped. + // `listen` opens with its own initial `refresh`, so this + // loop also covers a `listen` that was never established + // — i.e. a networkd that died between the seed above and + // the subscription below. + // + // That is still a milder failure than #621: the seed has + // already published `LinkSource::Networkd`, so a later + // death leaves the link list *stale*, not indeterminate, + // and it self-heals when networkd returns. The + // observability gap is real though — a post-seed recovery + // shows up as this loop's `warn!` going quiet, **not** as + // a `startup refresh RECOVERED` line, so grepping for + // that line will not evidence a real self-heal. The + // uncapped 2s cadence here is #646's second half. loop { match listen(&links_writer, &primary_writer, &source_writer).await { Ok(()) => tracing::warn!("networkd stream ended, retrying in 2s"), @@ -884,12 +856,19 @@ mod tests { } // --- startup refresh retry (#621) --- + // + // Since #646 the retry *mechanism* — the doubling schedule, the attempt + // budget, the give-up boundary — is tested once, in `crate::retry`, over the + // shared type; so are the shipped constants' delay invariants (#665), over + // *every* shipped policy rather than just this one. What is asserted here is + // this seed's own claim: a failed refresh retries rather than ending the + // task. /// A *bounded* policy with tiny delays, so the give-up path stays reachable /// and named. Deliberately not [`STARTUP_REFRESH_RETRY`] — these tests - /// assert the mechanism; the shipped shape is asserted separately below. - fn bounded() -> RefreshRetry { - RefreshRetry { + /// assert the decision; the shipped shape is asserted separately below. + fn bounded() -> retry::Policy { + retry::Policy { max_attempts: Some(3), initial: Duration::from_millis(10), max_backoff: Duration::from_millis(40), @@ -908,10 +887,10 @@ mod tests { // returns `Ok` trips `clippy::unnecessary_wraps`, and the wrapper is the // whole point here — `step` decides on the `Result`, not on a bool. let landed: Result<()> = Ok(()); - assert_eq!(bounded().step(&landed, 1), RefreshStep::Proceed); + assert_eq!(bounded().step(&landed, 1), retry::Step::Proceed); // Success ends the retrying whenever it arrives, not just first time. - assert_eq!(bounded().step(&landed, 3), RefreshStep::Proceed); - assert_eq!(bounded().step(&landed, u32::MAX), RefreshStep::Proceed); + assert_eq!(bounded().step(&landed, 3), retry::Step::Proceed); + assert_eq!(bounded().step(&landed, u32::MAX), retry::Step::Proceed); } #[test] @@ -920,37 +899,27 @@ mod tests { // for the process lifetime. A failure must schedule another attempt. assert_eq!( bounded().step(&failed(), 1), - RefreshStep::Retry { + retry::Step::Retry { after: Duration::from_millis(10) }, "the first failed seed must retry, not go inert (#621)" ); assert_eq!( bounded().step(&failed(), 2), - RefreshStep::Retry { + retry::Step::Retry { after: Duration::from_millis(20) } ); } - #[test] - fn a_bounded_policy_gives_up_once_its_budget_is_spent() { - assert_eq!(bounded().step(&failed(), 3), RefreshStep::GiveUp); - assert_eq!(bounded().step(&failed(), 4), RefreshStep::GiveUp); - assert_eq!(bounded().step(&failed(), 99), RefreshStep::GiveUp); - } - - #[test] - fn backoff_doubles_and_clamps_to_the_ceiling() { - let policy = bounded(); - assert_eq!(policy.backoff(1), Duration::from_millis(10)); - assert_eq!(policy.backoff(2), Duration::from_millis(20)); - assert_eq!(policy.backoff(3), Duration::from_millis(40)); - // Clamped, and the shift can't overflow at absurd attempt counts. - assert_eq!(policy.backoff(4), Duration::from_millis(40)); - assert_eq!(policy.backoff(u32::MAX), Duration::from_millis(40)); - } - + /// The delays are deliberately **not** asserted here, and deliberately never + /// against `STARTUP_REFRESH_RETRY.backoff(attempt)` — that is the expression + /// `step` computes internally, so comparing against it pins only that `step` + /// calls `backoff`, not that `backoff` produces a sane number. That + /// tautology is what #665 filed: with it, `initial: Duration::ZERO` kept the + /// whole suite green while turning the retry into a tight `error!` flood. + /// `crate::retry`'s shipped-policy tests carry the real assertions, for this + /// constant and `wifi::PROBE_RETRY` alike. #[test] fn the_shipped_policy_never_goes_inert() { // The decision on #621: unbounded. A bound would put the user back in @@ -962,32 +931,16 @@ mod tests { STARTUP_REFRESH_RETRY's docs and #621" ); for attempt in [1_u32, 2, 8, 1_000, u32::MAX] { - assert_eq!( - STARTUP_REFRESH_RETRY.step(&failed(), attempt), - RefreshStep::Retry { - after: STARTUP_REFRESH_RETRY.backoff(attempt) - }, + assert!( + matches!( + STARTUP_REFRESH_RETRY.step(&failed(), attempt), + retry::Step::Retry { .. } + ), "attempt {attempt}: a failed seed ended the task instead of retrying" ); } } - #[test] - fn the_shipped_policy_starts_promptly_and_stays_audible() { - // Prompt: the common case is a boot race of well under a second, so the - // first retry must not sit behind the ceiling. - assert!(STARTUP_REFRESH_RETRY.backoff(1) <= Duration::from_secs(1)); - assert!(STARTUP_REFRESH_RETRY.max_backoff >= STARTUP_REFRESH_RETRY.initial); - // Audible: every attempt logs at `error!`, which is only defensible - // because the delay is capped — an uncapped backoff would decay into a - // silent poll, the failure mode the observability requirement rules out. - assert!( - STARTUP_REFRESH_RETRY.max_backoff <= Duration::from_secs(30), - "the retry logs every attempt; an unbounded delay would make a dead \ - networkd silent in the journal" - ); - } - // --- parse_describe --- const SAMPLE_DESCRIBE: &str = r#"{ diff --git a/crates/hytte-services/src/retry.rs b/crates/hytte-services/src/retry.rs new file mode 100644 index 0000000..fda4f5b --- /dev/null +++ b/crates/hytte-services/src/retry.rs @@ -0,0 +1,286 @@ +//! One retry policy for the crate's "the daemon was not there yet" loops — +//! issue #646. +//! +//! Two services grew the same policy independently: `wifi`'s backend probe +//! (#613) and `networkd`'s startup link seed (#621). They retry the same class +//! of transient system-bus failure, ship the same three numbers, and their +//! `backoff()` bodies were character-for-character identical. What kept them in +//! sync was a pair of doc comments cross-referencing each other **by hand** — +//! and that hand-sync had already failed once: #665 found that `networkd` +//! mirrored the policy but dropped the assertion pinning its ceiling. +//! +//! What lives here is the *mechanism*: the schedule, the pure decision, and the +//! invariants every shipped policy must hold. What stays at the call sites is +//! the *judgement* — which outcomes are worth retrying, why the shipped budget +//! is unbounded, and what each attempt logs. +//! +//! [`Policy::step`] only ever sees a plain `Result`, never the value inside it. +//! Deciding what counts as a retryable failure is therefore the caller's job, +//! which is exactly what lets one type serve both sites: `wifi` weighs +//! *inconclusive* ("I could not ask", retry) against *answered* ("the bus +//! replied", commit — including the answer "neither daemon is present") before +//! handing the `Result` over, while for `networkd` a refresh simply either read +//! the links or did not. +//! +//! **Deliberately not folded in** — so nobody re-opens these as oversights: +//! +//! - `hytte_bus`'s `Backoff` (`hytte-bus/src/connection.rs`) is a stateful +//! cursor with `reset()`/`next()`, no attempt budget and no give-up. Different +//! shape, different job; #646 rules it out by name. +//! - `crate::eds_retry` is the EDS worker threads' resilience kit — blocking, +//! `std::sync::mpsc`-shaped, and it carries a failure-streak detector rather +//! than an attempt budget. +//! - `wifi/watcher.rs`'s 2s sleep is a one-shot debounce before a `return`, not +//! a retry loop. #646 names it explicitly so nobody "fixes" it into one. +//! +//! Everything here is pure — no bus, no clock, no tokio — so the tests are +//! hermetic and run in the default `cargo test` bucket. + +use std::time::Duration; + +/// A retry schedule: an attempt budget plus an exponential, capped delay. +#[derive(Clone, Copy, Debug)] +pub(crate) struct Policy { + /// Attempt budget, counting the first try. `None` means "retry forever". + pub(crate) max_attempts: Option, + /// Delay before the first retry; doubles with each further attempt. + pub(crate) initial: Duration, + /// Ceiling the doubling delay is clamped to. + pub(crate) max_backoff: Duration, +} + +/// What to do after one attempt. +/// +/// The whole decision lives in [`Policy::step`] — a pure function — so it is +/// unit-testable without a bus. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum Step { + /// The attempt succeeded. Stop retrying. + /// + /// Carries no payload on purpose. What the success *was* is the caller's to + /// read off its own `Result`, and that is what lets one policy serve a + /// `Result<(), E>` seed and a `Result` probe alike (#646). + /// It also makes it impossible for a caller to invent a verdict out of an + /// error, which is the shape #613 regressed on. + Proceed, + /// It failed and attempts remain: wait, then try again. + Retry { + /// How long to wait before the next attempt. + after: Duration, + }, + /// Still failing and the attempt budget is spent: the caller should log + /// loudly and stop. + /// + /// **Not reachable under either shipped policy** — both are unbounded (see + /// `wifi::PROBE_RETRY` and `networkd::STARTUP_REFRESH_RETRY` for why, argued + /// on #613 and #621 respectively). Kept expressible, and tested, so a + /// bounded policy stays one field away. + GiveUp, +} + +impl Policy { + /// Delay before the retry that follows `attempt` (1-based): [`Self::initial`] + /// doubled once per elapsed attempt, clamped to [`Self::max_backoff`]. + /// + /// Saturating throughout, so an absurd `attempt` clamps to the ceiling + /// rather than overflowing: `checked_shl` returns `None` once the shift + /// reaches 32, and the multiply saturates before the `min`. + fn backoff(self, attempt: u32) -> Duration { + let factor = 1_u32 + .checked_shl(attempt.saturating_sub(1)) + .unwrap_or(u32::MAX); + self.initial.saturating_mul(factor).min(self.max_backoff) + } + + /// The pure retry/proceed decision. `attempt` is 1-based and counts the try + /// that produced `outcome`. + /// + /// Generic over the `Result` because the policy weighs only *whether* the + /// attempt succeeded, never what it returned. A caller for which some `Ok` + /// values are still worth retrying — or some `Err` values are not — must + /// resolve that before calling; see `wifi::probe_until_conclusive`, where + /// `Ok(BackendChoice::None)` is a real finding and must commit rather than + /// be asked again. + pub(crate) fn step(self, outcome: &Result, attempt: u32) -> Step { + match outcome { + Ok(_) => Step::Proceed, + Err(_) if self.max_attempts.is_some_and(|max| attempt >= max) => Step::GiveUp, + Err(_) => Step::Retry { + after: self.backoff(attempt), + }, + } + } +} + +/// Every retry policy this crate **ships**, so the `every_shipped_policy_*` +/// tests below see all of them. +/// +/// **A new shipped policy belongs in this list.** These are the only tests that +/// look at real numbers rather than a test-local schedule, and #665 exists +/// precisely because the second policy shipped without them: its own test +/// compared `step`'s delay against `policy.backoff(attempt)` — the very +/// expression `step` computes internally — so `initial: Duration::ZERO` kept the +/// whole suite green while turning the retry into a tight `error!` flood. +#[cfg(test)] +const SHIPPED: &[(&str, Policy)] = &[ + ("wifi::PROBE_RETRY", crate::wifi::PROBE_RETRY), + ( + "networkd::STARTUP_REFRESH_RETRY", + crate::networkd::STARTUP_REFRESH_RETRY, + ), +]; + +#[cfg(test)] +mod tests { + use super::{Policy, SHIPPED, Step}; + use std::time::Duration; + + /// A *bounded* policy with tiny delays, so the give-up path stays reachable + /// and named. Deliberately not one of the shipped constants — these tests + /// assert the mechanism, so tuning a shipped policy must not redden them. + /// The shipped numbers are asserted separately, at the bottom of this file. + fn bounded() -> Policy { + Policy { + max_attempts: Some(3), + initial: Duration::from_millis(10), + max_backoff: Duration::from_millis(40), + } + } + + /// A failed attempt. The error type is irrelevant to the policy — that is + /// the whole point of the generic `step`. + fn failed() -> Result<(), &'static str> { + Err("the daemon did not answer") + } + + #[test] + fn a_successful_attempt_proceeds_whenever_it_arrives() { + // A local binding rather than a helper fn: a function that always + // returns `Ok` trips `clippy::unnecessary_wraps`, and the wrapper is the + // whole point here — `step` decides on the `Result`, not on a bool. + let landed: Result<(), &'static str> = Ok(()); + assert_eq!(bounded().step(&landed, 1), Step::Proceed); + // Success ends the retrying whenever it arrives, not just first time. + assert_eq!(bounded().step(&landed, 3), Step::Proceed); + assert_eq!(bounded().step(&landed, u32::MAX), Step::Proceed); + } + + #[test] + fn a_failed_attempt_retries_while_the_budget_lasts() { + assert_eq!( + bounded().step(&failed(), 1), + Step::Retry { + after: Duration::from_millis(10) + }, + "the first failure must schedule another attempt, not end the loop (#613, #621)" + ); + assert_eq!( + bounded().step(&failed(), 2), + Step::Retry { + after: Duration::from_millis(20) + } + ); + } + + #[test] + fn a_bounded_policy_gives_up_once_its_budget_is_spent() { + assert_eq!(bounded().step(&failed(), 3), Step::GiveUp); + assert_eq!(bounded().step(&failed(), 4), Step::GiveUp); + assert_eq!(bounded().step(&failed(), 99), Step::GiveUp); + } + + #[test] + fn an_unbounded_policy_never_gives_up() { + let forever = Policy { + max_attempts: None, + ..bounded() + }; + for attempt in [1_u32, 3, 100, u32::MAX] { + assert!( + matches!(forever.step(&failed(), attempt), Step::Retry { .. }), + "attempt {attempt}: an unbounded policy stopped asking" + ); + } + } + + #[test] + fn backoff_doubles_and_clamps_to_the_ceiling() { + let policy = bounded(); + assert_eq!(policy.backoff(1), Duration::from_millis(10)); + assert_eq!(policy.backoff(2), Duration::from_millis(20)); + assert_eq!(policy.backoff(3), Duration::from_millis(40)); + // Clamped, and the shift can't overflow at absurd attempt counts. + assert_eq!(policy.backoff(4), Duration::from_millis(40)); + assert_eq!(policy.backoff(u32::MAX), Duration::from_millis(40)); + } + + // ── The shipped constants (#665) ───────────────────────────────────────── + // + // Every assertion below compares against a *literal* — a field of the policy + // under test, or `Duration::ZERO` — and never against `policy.backoff(..)`, + // which is the expression `step` computes internally and so pins nothing + // about the number it produces. That re-computation is the tautology #665 + // filed. + + /// The falsification #665 documented: set `initial: Duration::ZERO` on + /// either shipped policy and everything else stays green, because expected + /// and computed both become zero. The resulting behaviour is + /// `sleep(Duration::ZERO)` on every retry — and since *both* loops log every + /// attempt at `error!`, that is not a fast retry, it is a tight journal + /// flood as fast as the bus can fail. Both policies' own docs argue that + /// cannot happen "because the backoff caps"; these two lines are what make + /// the argument true. + #[test] + fn every_shipped_policy_sleeps_a_nonzero_time_and_reaches_its_ceiling() { + for (name, policy) in SHIPPED { + assert!( + policy.backoff(1) > Duration::ZERO, + "{name}: the first retry delay is zero — every attempt logs at `error!`, so this \ + is a tight log flood rather than a backoff (#665)" + ); + assert_eq!( + policy.backoff(u32::MAX), + policy.max_backoff, + "{name}: the doubling never actually reaches `max_backoff`, so the documented \ + ceiling is not the real one (#665)" + ); + // …and `step` is wired to that ceiling, not merely to some delay. + // Unbounded policies only: a bounded one gives up long before + // `u32::MAX` attempts, which `a_bounded_policy_gives_up_…` covers. + if policy.max_attempts.is_none() { + assert_eq!( + policy.step(&failed(), u32::MAX), + Step::Retry { + after: policy.max_backoff + }, + "{name}: `step` does not settle at the literal `max_backoff` (#665)" + ); + } + } + } + + /// Prompt, because the case both policies exist for is a boot race of well + /// under a second — the first retry must not sit behind the ceiling. + /// Audible, because every attempt logs at `error!`, which is only defensible + /// while the delay stays capped: an uncapped backoff would decay into a + /// silent poll, the failure mode the observability requirement rules out. + #[test] + fn every_shipped_policy_starts_promptly_and_stays_audible() { + for (name, policy) in SHIPPED { + assert!( + policy.backoff(1) <= Duration::from_secs(1), + "{name}: the first retry is too slow to catch a daemon that is only milliseconds \ + behind the shell" + ); + assert!( + policy.max_backoff >= policy.initial, + "{name}: the ceiling is below the floor" + ); + assert!( + policy.max_backoff <= Duration::from_secs(30), + "{name}: the retry logs every attempt; a longer delay would let a dead daemon go \ + quiet in the journal" + ); + } + } +} diff --git a/crates/hytte-services/src/wifi/mod.rs b/crates/hytte-services/src/wifi/mod.rs index 6d2dd2f..7c2017b 100644 --- a/crates/hytte-services/src/wifi/mod.rs +++ b/crates/hytte-services/src/wifi/mod.rs @@ -44,7 +44,10 @@ use std::sync::{Arc, OnceLock}; use std::time::Duration; use tokio::sync::{Mutex as AsyncMutex, RwLock}; -use crate::wifi_backend::{BackendChoice, ProbeError}; +use crate::retry; +// `ProbeError` is no longer named outside the tests: since #646 the policy is +// generic over the `Result`, so only `probe_backend`'s own signature mentions it. +use crate::wifi_backend::BackendChoice; // ── Public re-exports ───────────────────────────────────────────────────────── @@ -186,38 +189,11 @@ const ANCHOR_NAME: &str = "mov.vibec0re.trollshell.iwd-agent"; // Picking up a daemon that appears *after* a conclusive verdict, or switching // backends at runtime, is out of scope here: it needs a cancellation primitive // that does not exist yet (#633). - -/// What to do after one probe attempt. -/// -/// The whole retry decision lives in [`RetryPolicy::step`] — a pure function — -/// so it is unit-testable without a bus. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -enum ProbeStep { - /// The probe answered. Commit to this backend and start it. - Commit(BackendChoice), - /// The probe was inconclusive and attempts remain: wait, then ask again. - Retry { - /// How long to wait before the next attempt. - after: Duration, - }, - /// Still inconclusive and the attempt budget is spent: log loudly and stay - /// inert. - /// - /// **Not reachable under the shipped [`PROBE_RETRY`]**, which is unbounded. - /// Kept expressible (and tested) so a bounded policy stays one field away. - GiveUp, -} - -/// Retry schedule for an *inconclusive* backend probe. -#[derive(Clone, Copy, Debug)] -struct RetryPolicy { - /// Attempt budget, counting the first probe. `None` means "retry forever". - max_attempts: Option, - /// Delay before the first retry; doubles with each further attempt. - initial: Duration, - /// Ceiling the doubling delay is clamped to. - max_backoff: Duration, -} +// +// The retry *mechanism* — schedule, budget, the pure decision — lives in +// `crate::retry`, shared with `networkd`'s startup seed since #646. What stays +// here is the *judgement*: which probe outcomes are worth asking again about, +// and why the shipped budget is unbounded. /// The policy this service runs: **unbounded** retry with capped backoff. /// @@ -227,7 +203,8 @@ struct RetryPolicy { /// healthy bus a probe is sub-millisecond, but the failure mode being retried /// is exactly the one where it is slow — [`crate::wifi_backend::probe_backend`] /// makes two sequential `hytte_bus::call`s, each with a 25s timeout and -/// `RetryPolicy::Once`, so a down socket costs roughly 10s per attempt (fast +/// [`hytte_bus::RetryPolicy::Once`] (a different, per-call policy — not this +/// one), so a down socket costs roughly 10s per attempt (fast /// fail plus a 5s reconnect wait, twice) and a wedged peer up to ~50s (two full /// timeouts, which `is_transient_zbus_error` does not classify as transient, so /// there is no retry). @@ -250,43 +227,19 @@ struct RetryPolicy { /// silently polled. /// /// **The policy is one field to change.** A bounded policy is -/// `max_attempts: Some(n)` and nothing else; the same question is open for -/// `networkd`'s startup refresh on #621, and the two subsystems should end up -/// consistent. -const PROBE_RETRY: RetryPolicy = RetryPolicy { +/// `max_attempts: Some(n)` and nothing else; `networkd`'s startup refresh +/// answered the same question the same way on #621, and since #646 the two +/// subsystems share [`retry::Policy`] rather than a hand-kept cross-reference. +/// +/// The schedule's invariants — a nonzero first delay, a ceiling that is +/// actually reached — are asserted over *every* shipped policy in +/// `retry`'s tests (#665), not here. +pub(crate) const PROBE_RETRY: retry::Policy = retry::Policy { max_attempts: None, initial: Duration::from_millis(500), max_backoff: Duration::from_secs(8), }; -impl RetryPolicy { - /// Delay before the retry that follows `attempt` (1-based): [`Self::initial`] - /// doubled once per elapsed attempt, clamped to [`Self::max_backoff`]. - fn backoff(self, attempt: u32) -> Duration { - let factor = 1_u32 - .checked_shl(attempt.saturating_sub(1)) - .unwrap_or(u32::MAX); - self.initial.saturating_mul(factor).min(self.max_backoff) - } - - /// The pure retry/commit decision. `attempt` is 1-based and counts the - /// probe that produced `outcome`. - /// - /// `Ok(_)` always commits — **including `Ok(BackendChoice::None)`**, which - /// is a real answer ("the bus replied and neither daemon is present"). - /// Retrying *that* would be the bug; only "I could not ask" - /// ([`ProbeError`]) is retried. - fn step(self, outcome: &Result, attempt: u32) -> ProbeStep { - match outcome { - Ok(choice) => ProbeStep::Commit(*choice), - Err(_) if self.max_attempts.is_some_and(|max| attempt >= max) => ProbeStep::GiveUp, - Err(_) => ProbeStep::Retry { - after: self.backoff(attempt), - }, - } - } -} - /// Probe for the Wi-Fi backend, retrying only while the probe is /// *inconclusive*, and return the verdict to commit to. /// @@ -299,19 +252,35 @@ impl RetryPolicy { /// a `RECOVERED` line naming the attempt count: #609's diagnostic exists to /// *measure* how often this fires, and a retry that healed quietly would trade /// a visible permanent bug for an invisible intermittent one. -async fn probe_until_conclusive(policy: RetryPolicy) -> Option { +async fn probe_until_conclusive(policy: retry::Policy) -> Option { let mut attempt: u32 = 1; loop { let outcome = crate::wifi_backend::probe_backend().await; // Rendered up front so the log arms below can name the failure. Empty - // on `Ok`, which only ever reaches the `Commit` arm. + // on `Ok`, which only ever reaches the `Proceed` arm. let reason = outcome .as_ref() .err() .map_or_else(String::new, ToString::to_string); - match policy.step(&outcome, attempt) { - ProbeStep::Commit(choice) => { + // The verdict is weighed *here*, not inside the policy (#646). All + // [`retry::Policy::step`] sees is the `Ok`/`Err` split — and for this + // probe that split already is the distinction that matters: any answer + // is conclusive and commits, **including `Ok(BackendChoice::None)`** + // ("the bus replied and neither daemon is present"), while `ProbeError` + // ("I could not ask") is the only retryable outcome. Retrying a real + // `None` would be a bug of its own — an endless poll on a host that + // simply has no Wi-Fi daemon — and committing an `Err` as `None` was + // #613 itself. + let step = policy.step(&outcome, attempt); + match step { + retry::Step::Proceed => { + // `Proceed` is returned for `Ok` and nothing else, so the + // verdict is right here in the probe's own `Result`. Reading it + // off `outcome` rather than out of the step is what keeps the + // policy generic — and means no code path can conjure a + // `BackendChoice` out of an error. + let choice = outcome.expect("retry::Step::Proceed is returned only for Ok"); // The recovery wording is gated on the *verdict*, not just on // having retried: a host that genuinely runs no Wi-Fi daemon // recovers into `None`, and telling that user "Wi-Fi is coming @@ -338,7 +307,7 @@ async fn probe_until_conclusive(policy: RetryPolicy) -> Option { } return Some(choice); } - ProbeStep::Retry { after } => { + retry::Step::Retry { after } => { // Logged on *every* attempt, not every Nth: the backoff caps at // 8s, so a permanently dead bus stays loud here rather than // degrading into a silent poll. See `PROBE_RETRY`. @@ -355,7 +324,7 @@ async fn probe_until_conclusive(policy: RetryPolicy) -> Option { tokio::time::sleep(after).await; attempt += 1; } - ProbeStep::GiveUp => { + retry::Step::GiveUp => { tracing::error!( attempts = attempt, error = %reason, @@ -1073,14 +1042,21 @@ async fn do_known_network_call(path: &str, method: &str) -> Result<(), hytte_bus #[cfg(test)] mod tests { - use super::{BackendChoice, Duration, ProbeError, ProbeStep, RetryPolicy}; + use super::{BackendChoice, Duration, retry}; + use crate::wifi_backend::ProbeError; + + // Since #646 the retry *mechanism* — the doubling schedule, the attempt + // budget, the give-up boundary — is tested once, in `crate::retry`, over the + // shared type; so are the shipped constants' delay invariants (#665), over + // *every* shipped policy rather than this one. What is asserted here is the + // part that is specific to this probe: which outcomes count as an answer. /// Test policy: a small, explicit budget so the give-up boundary is easy to /// name. Deliberately not [`super::PROBE_RETRY`] — these tests assert the /// *decision*, not the shipped numbers, so tuning the policy does not /// redden them. - fn bounded() -> RetryPolicy { - RetryPolicy { + fn bounded() -> retry::Policy { + retry::Policy { max_attempts: Some(3), initial: Duration::from_millis(10), max_backoff: Duration::from_millis(40), @@ -1100,12 +1076,12 @@ mod tests { #[test] fn ok_commits_on_the_first_attempt() { assert_eq!( - bounded().step(&Ok(BackendChoice::NetworkManager), 1), - ProbeStep::Commit(BackendChoice::NetworkManager) + bounded().step(&Ok::<_, ProbeError>(BackendChoice::NetworkManager), 1), + retry::Step::Proceed ); assert_eq!( - bounded().step(&Ok(BackendChoice::Iwd), 1), - ProbeStep::Commit(BackendChoice::Iwd) + bounded().step(&Ok::<_, ProbeError>(BackendChoice::Iwd), 1), + retry::Step::Proceed ); } @@ -1116,42 +1092,20 @@ mod tests { #[test] fn ok_none_commits_and_does_not_retry() { assert_eq!( - bounded().step(&Ok(BackendChoice::None), 1), - ProbeStep::Commit(BackendChoice::None) - ); - } - - // ── An inconclusive probe retries, then gives up at the bound ───────────── - - #[test] - fn err_retries_while_attempts_remain() { - let policy = bounded(); - assert_eq!( - policy.step(&inconclusive(), 1), - ProbeStep::Retry { - after: Duration::from_millis(10) - } - ); - assert_eq!( - policy.step(&inconclusive(), 2), - ProbeStep::Retry { - after: Duration::from_millis(20) - } + bounded().step(&Ok::<_, ProbeError>(BackendChoice::None), 1), + retry::Step::Proceed ); } - #[test] - fn err_past_the_bound_gives_up() { - let policy = bounded(); - // The budget is 3 attempts: the third inconclusive answer is the last. - assert_eq!(policy.step(&inconclusive(), 3), ProbeStep::GiveUp); - assert_eq!(policy.step(&inconclusive(), 4), ProbeStep::GiveUp); - assert_eq!(policy.step(&inconclusive(), 99), ProbeStep::GiveUp); - } - /// The #613 regression itself: an inconclusive probe must never commit to /// "no backend". That collapse is what latched Wi-Fi off for a whole /// session and made a shell restart the only cure (#607). + /// + /// Since #646 the type system carries half of this: `retry::Step::Proceed` + /// has no payload, so `probe_until_conclusive` can only ever return a + /// `BackendChoice` it read out of an `Ok` — there is no longer a code path + /// that could manufacture `None` from a `ProbeError`. What is left to assert + /// is that an inconclusive probe does not reach that arm at all. #[test] fn inconclusive_probe_never_commits_to_no_backend() { let policy = bounded(); @@ -1159,11 +1113,11 @@ mod tests { let step = policy.step(&inconclusive(), attempt); assert_ne!( step, - ProbeStep::Commit(BackendChoice::None), - "attempt {attempt}: an inconclusive probe was committed as 'no backend'" + retry::Step::Proceed, + "attempt {attempt}: an inconclusive probe was treated as an answer" ); assert!( - matches!(step, ProbeStep::Retry { .. } | ProbeStep::GiveUp), + matches!(step, retry::Step::Retry { .. } | retry::Step::GiveUp), "attempt {attempt}: expected retry-or-give-up, got {step:?}" ); } @@ -1171,55 +1125,30 @@ mod tests { // ── Policy shape ───────────────────────────────────────────────────────── + /// The policy this service actually ships must be the unbounded one: a + /// bounded give-up would re-create #607 (Wi-Fi gone, restart the only cure) + /// whenever the bus takes longer than the bound. A bounded policy stays + /// expressible, and `crate::retry`'s tests keep its give-up path covered. + /// + /// The delays are asserted in `crate::retry` over every shipped policy — + /// deliberately not here, and deliberately never against + /// `PROBE_RETRY.backoff(attempt)`, which is the expression `step` computes + /// internally and therefore pins nothing (#665). #[test] - fn backoff_doubles_and_clamps_to_the_ceiling() { - let policy = bounded(); - assert_eq!(policy.backoff(1), Duration::from_millis(10)); - assert_eq!(policy.backoff(2), Duration::from_millis(20)); - assert_eq!(policy.backoff(3), Duration::from_millis(40)); - // Clamped, and no overflow panic at absurd attempt counts. - assert_eq!(policy.backoff(4), Duration::from_millis(40)); - assert_eq!(policy.backoff(u32::MAX), Duration::from_millis(40)); - } - - /// The shipped shape: `max_attempts: None` never gives up, at any attempt - /// count. This is the property the fix rests on — a bounded give-up would - /// re-create #607 (Wi-Fi gone, restart the only cure) whenever the bus - /// takes longer than the bound. - #[test] - fn unbounded_policy_never_gives_up() { - let forever = RetryPolicy { - max_attempts: None, - ..bounded() - }; - for attempt in [1_u32, 3, 100, u32::MAX] { - assert_eq!( - forever.step(&inconclusive(), attempt), - ProbeStep::Retry { - after: forever.backoff(attempt) - } - ); - } - } - - /// The policy this service actually ships must be the unbounded one, and - /// its first retry prompt enough to catch a bus that is only milliseconds - /// behind the shell. A bounded policy stays expressible (and tested above), - /// but shipping one would put the #613 latch back on a longer fuse. - #[test] - fn shipped_policy_is_unbounded_and_starts_promptly() { + fn the_shipped_policy_never_goes_inert() { assert_eq!( super::PROBE_RETRY.max_attempts, None, - "the shipped probe policy must never give up; see PROBE_RETRY's docs and #621" + "the shipped probe policy must never give up; see PROBE_RETRY's docs and #613" ); - assert_eq!( - super::PROBE_RETRY.step(&inconclusive(), u32::MAX), - ProbeStep::Retry { - after: super::PROBE_RETRY.max_backoff - } - ); - assert!(super::PROBE_RETRY.backoff(1) <= Duration::from_secs(1)); - assert!(super::PROBE_RETRY.max_backoff >= super::PROBE_RETRY.initial); + for attempt in [1_u32, 3, 100, u32::MAX] { + assert!( + matches!( + super::PROBE_RETRY.step(&inconclusive(), attempt), + retry::Step::Retry { .. } + ), + "attempt {attempt}: an inconclusive probe ended the task instead of retrying" + ); + } } }