From b404926c7553edbaa3d229ef620c8773aac19eda Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 03:16:50 -0400 Subject: [PATCH 1/6] fix(#760): pin the health clock in test fixtures so a handler's HTTP status cannot depend on machine load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `require_live_session` refuses a write command when `snapshot_age_ms >= 5000`, and `snapshot_age_ms` was `NetHealth::last_tick.elapsed()` — wall time between the fixture's construction and the handler being served. On a loaded box that crossed 5s and `combat::tests::cast_empty_gem_is_409_and_queues_nothing` got a 503 instead of its 409, from the first line of `post_cast`, before the gem was ever looked at. Re-derived before fixing: a probe that slept 5.1s between `empty_state()` and the same request returned `503 ... has not ticked in 5100ms`. The issue's attribution is correct. Introduce `eqoxide_ipc::HealthClock` — a newtype over a PRIVATE `Option` whose only non-wall constructor, `frozen_at`, is behind `#[cfg(any(test, feature = "test-fixtures"))]`. `NetHealth` carries one (`WALL` by default) and `HttpState::health()` derives every age from it. A release build therefore cannot represent a frozen health clock, which matters in the other direction too: a pinned clock would report a dead net thread as live forever (#343). `testkit::empty_state()` now pins its clock at its own stamps, so its ages are structurally 0 however long a test takes — the bad state is unrepresentable rather than merely unlikely. The seven `observe` tests whose subject IS the clock opt back in via a documented `testkit::empty_state_wall_clock()`. Mutation-checked: reverting either half turns three tests red, including a deterministic end-to-end repro of the reported 503. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-http/Cargo.toml | 4 +- crates/eqoxide-http/src/combat.rs | 84 +++++++++++++++ crates/eqoxide-http/src/lib.rs | 83 ++++++++++++--- crates/eqoxide-http/src/observe.rs | 19 ++-- crates/eqoxide-http/src/testkit.rs | 32 +++++- crates/eqoxide-ipc/src/lib.rs | 157 +++++++++++++++++++++++++++++ 6 files changed, 354 insertions(+), 25 deletions(-) diff --git a/crates/eqoxide-http/Cargo.toml b/crates/eqoxide-http/Cargo.toml index 513036e4..eeb78962 100644 --- a/crates/eqoxide-http/Cargo.toml +++ b/crates/eqoxide-http/Cargo.toml @@ -15,7 +15,9 @@ path = "src/lib.rs" # tests, which need `eq_net::packet_handler` — an app-crate module BELOW which http sits) can build # an `HttpState` and hit the observe router. Off in normal builds; the release binary never has it. # Forwards to eqoxide-nav so `testkit::empty_state` can build a real `ZoneAssetState::Ready`. -test-fixtures = ["eqoxide-nav/test-fixtures"] +# Forwards to eqoxide-ipc so `testkit::empty_state` can build a `NetHealth::frozen_at` — the pinned +# health clock that stops a downstream handler test's HTTP status depending on machine load (#760). +test-fixtures = ["eqoxide-nav/test-fixtures", "eqoxide-ipc/test-fixtures"] [dependencies] # eqoxide-http is the agent-facing REST API (axum). It sits at the TOP of the extracted-crate stack: diff --git a/crates/eqoxide-http/src/combat.rs b/crates/eqoxide-http/src/combat.rs index 80cb65ea..203328fa 100644 --- a/crates/eqoxide-http/src/combat.rs +++ b/crates/eqoxide-http/src/combat.rs @@ -558,6 +558,90 @@ mod tests { assert!(command.take_cast().is_none(), "an empty gem must not be queued as a cast"); } + /// #760: the request above must answer 409 for reasons that have nothing to do with how long the + /// test took to get there. `empty_state()` used to stamp its net-health clocks with + /// `Instant::now()` and `HttpState::health()` used to read them against `Instant::now()` again, + /// so `snapshot_age_ms` was "however long this test has been running" — and on a loaded box that + /// crossed `SESSION_STALE_TICK_MS` (5s) and `require_live_session` answered `503` from the very + /// first line of `post_cast`, before any gem was ever looked at. + /// + /// MEASURED, on this code, before the fix: the same request with a 5.1s sleep in front of it + /// returned `503 … the network thread has not ticked in 5100ms`. So the reported mechanism is + /// the real one and the 503 really does come from this guard on this route. + /// + /// This asserts the FIXTURE property that removes it, not a timing: real wall time passes here + /// (the sleep is deliberate and observable via `Instant::elapsed`) and the fixture's projected + /// ages stay exactly 0, because they are measured against a clock pinned at the fixture's own + /// construction instant. A 50ms sleep and a 5s one are the same assertion — `now - stamp` where + /// `now == stamp` is 0 for every duration — so this does not need to cost the suite 5 seconds + /// to prove the 5-second case. + #[tokio::test] + async fn handler_fixture_ages_do_not_track_the_wall_clock() { + let state = empty_state(); + assert!(state.net_health.lock().unwrap().clock.is_frozen(), + "the handler fixture must pin its health clock, or every gated handler test races \ + SESSION_STALE_TICK_MS (#760)"); + let started = std::time::Instant::now(); + std::thread::sleep(std::time::Duration::from_millis(50)); + assert!(started.elapsed() >= std::time::Duration::from_millis(50), + "precondition: real wall time really did pass"); + let h = state.health(); + assert_eq!(h.snapshot_age_ms, 0, + "the fixture's tick age must be pinned at 0, not the {}ms of wall clock that just \ + elapsed — this is the age require_live_session 503s on (#760)", started.elapsed().as_millis()); + assert_eq!(h.link_age_ms, 0, "the fixture's link age must be pinned too — it feeds `connected`"); + assert_eq!(h.last_packet_age_ms, 0, "the fixture's packet age must be pinned too"); + assert!(crate::require_live_session(&state).is_ok(), + "and the verdict the pinned ages produce is a live session"); + } + + /// #760, end to end and DETERMINISTIC: the exact request + /// `cast_empty_gem_is_409_and_queues_nothing` drives, served after more than + /// `SESSION_STALE_TICK_MS` (5s) of real wall time has passed since the fixture was built. + /// + /// This is the load condition the flake needs, forced rather than waited for — and forced in the + /// only direction that matters: a busy machine makes the sleep LONGER, never shorter, so this + /// test cannot become less severe under load. On the pre-fix code the measured answer here was + /// `503 … the network thread has not ticked in 5100ms`; the whole 409 body was never reached. + /// + /// It costs the suite ~5s. That is the price of turning "fails sometimes, on someone else's PR" + /// into a check that fails every time if the fix is undone. + #[tokio::test] + async fn cast_empty_gem_is_still_409_after_the_stale_session_bound_has_elapsed() { + let state = empty_state(); + set_gs(&state, |gs| { + gs.mem_spells = [eqoxide_core::game_state::EMPTY_GEM; 9]; + gs.mem_spells[0] = 202; + }); + let net_health = state.net_health.clone(); + let command = state.command.clone(); + let app = router().with_state(state); + // > SESSION_STALE_TICK_MS. `std::thread::sleep`, not `tokio::time::sleep`: the clock under + // test is `std::time::Instant`, which tokio's virtual time does not move. + std::thread::sleep(std::time::Duration::from_millis(5_100)); + let req = Request::post("/cast") + .header("content-type", "application/json") + .body(Body::from(r#"{"gem":7}"#)).unwrap(); + let resp = app.oneshot(req).await.unwrap(); + let status = resp.status(); + let text = body_text(resp).await; + assert_eq!(status, StatusCode::CONFLICT, + "5.1s of wall time must not change this request's answer — it is an empty gem either \ + way (#760). Body was: {text}"); + assert!(text.contains("empty"), "and for the real reason, not a stale-session one: {text}"); + // ONE lock, both reads. Two `net_health.lock()` calls in a single `assert_eq!` self-deadlock: + // the left guard is a temporary that lives to the end of the statement, and `std::sync::Mutex` + // is not reentrant — the second lock blocks forever. Measured: this test hung a whole + // `eqoxide-http` test binary until it was written this way. + let (clock_now, tick_stamp) = { + let h = net_health.lock().unwrap(); + (h.clock.now(), h.last_tick) + }; + assert_eq!(clock_now, tick_stamp, + "the fixture's clock is still pinned to its own tick stamp after 5.1s"); + assert!(command.take_cast().is_none(), "and still nothing was queued"); + } + // ── A3 Migration 3 (#448): POST /v1/combat/cast reports the TRUE outcome, not a queued 200 ── use eqoxide_command::{CastEnd, CommandResult}; diff --git a/crates/eqoxide-http/src/lib.rs b/crates/eqoxide-http/src/lib.rs index 7268167b..93df01b7 100644 --- a/crates/eqoxide-http/src/lib.rs +++ b/crates/eqoxide-http/src/lib.rs @@ -785,15 +785,22 @@ impl HttpState { /// renderer, not the network thread — has to be alive for the answer to be honest. pub(crate) fn health(&self) -> Health { let h = *self.net_health.lock().unwrap(); - let link_age = h.last_datagram.elapsed(); - let last_packet_ago = h.last_packet.elapsed(); + // #760: EVERY age below is measured back from `h.clock`, never from a bare `Instant::now()`. + // In production that clock IS `Instant::now()` (`HealthClock::WALL`, the only clock a + // non-test build can construct), so this is the same read-time measurement #343 requires. + // In a TEST it is pinned, which is what makes a fixture's liveness verdict a pure function + // of the fixture instead of a function of how long the test took to reach this line. If you + // add a field here, derive its age from `h.clock` too — a stray `.elapsed()` silently opts + // that one field back into wall time and re-arms #760's flake for it. + let link_age = h.clock.age_of(h.last_datagram); + let last_packet_ago = h.clock.age_of(h.last_packet); // #371: the active-probe verdict, all measured at read time (like every other health field, // per #343 — never cached, so no live publisher has to run for the answer to stay honest). // NOTE: the timeout check is measured against `first_unanswered_probe_sent`, NOT // `last_probe_sent` — the latter is bumped by every 30s resend and would let a permanently // wedged zone re-earn the 10s in-flight grace window forever (the #371-followup bug). - let probe_sent_ago = h.first_unanswered_probe_sent.map(|t| t.elapsed()); - let probe_reply_ago = h.last_probe_reply.map(|t| t.elapsed()); + let probe_sent_ago = h.first_unanswered_probe_sent.map(|t| h.clock.age_of(t)); + let probe_reply_ago = h.last_probe_reply.map(|t| h.clock.age_of(t)); // #470: `world_responsive` must know the LINK is dead. When a failed world-reconnect kills the // net thread the prober dies too, so no probe is ever outstanding — without this, the "no // probe" branch reported a zombie session alive forever. `connected` here is the SAME value @@ -817,12 +824,12 @@ impl HttpState { // #656: the io-starvation alert. Measured HERE, at read time (#343's rule for every age in // this payload) — `last_send_pressure_at` is a timestamp, never a pre-computed bool, so the // alert cannot go stale between the net thread's last write and this HTTP read. - let send_pressure_ago = h.last_send_pressure_at.map(|t| t.elapsed()); + let send_pressure_ago = h.last_send_pressure_at.map(|t| h.clock.age_of(t)); let send_starved = eqoxide_ipc::send_starved(h.send_pressure_streak, send_pressure_ago); Health { link_age_ms: link_age.as_millis() as u64, last_packet_age_ms: last_packet_ago.as_millis() as u64, - snapshot_age_ms: h.last_tick.elapsed().as_millis() as u64, + snapshot_age_ms: h.clock.age_of(h.last_tick).as_millis() as u64, // Link liveness, NOT world activity — see `NetHealth`. An idle session goes 40+s with // no application packet while the session layer keeps ACKing; calling that "disconnected" // would be just as much a lie as #343's frozen `connected: true`. @@ -838,7 +845,7 @@ impl HttpState { send_starved, send_failures_unretried: h.send_failures_unretried, last_send_error: h.last_send_error_kind, - last_send_error_age_ms: h.last_send_error_at.map(|t| t.elapsed().as_millis() as u64), + last_send_error_age_ms: h.last_send_error_at.map(|t| h.clock.age_of(t).as_millis() as u64), reliable_abandoned: h.reliable_abandoned, session_drop: h.session_drop, } @@ -1116,7 +1123,7 @@ mod player_view_datum_tests { #[cfg(test)] mod live_session_guard_tests { use super::*; - use crate::testkit::{ago, empty_state}; + use crate::testkit::empty_state; use axum::body::Body; use axum::http::{Request, StatusCode}; use tower::ServiceExt; @@ -1124,13 +1131,17 @@ mod live_session_guard_tests { /// Overwrite the three net-health clocks (ages in seconds) to model a given liveness state; the /// probe clocks are left at their `Default` (`None`), which is what the guard reads through /// `health()` but never keys its verdict on. + /// + /// #760: the clock is PINNED at the same instant the stamps are taken from, so the ages + /// `health()` reads back are exactly the numbers passed in. Before, the stamps were wall-clock + /// and the read happened later, so the real age was `arg + (however long the test then took)`. + /// By construction that put `tick_just_under_bound_passes` (1s, bound 5s) on a 4s margin that + /// machine load could eat — the same mechanism as #760, one test over. Not observed failing; + /// stated as a derivation from the old code, not as a measurement. fn set_clocks(s: &HttpState, datagram_ago: u64, tick_ago: u64, packet_ago: u64) { - *s.net_health.lock().unwrap() = NetHealth { - last_datagram: ago(datagram_ago), - last_packet: ago(packet_ago), - last_tick: ago(tick_ago), - ..NetHealth::default() - }; + *s.net_health.lock().unwrap() = NetHealth::frozen_at( + std::time::Instant::now(), datagram_ago, tick_ago, packet_ago, + ); } /// A freshly-built state (all clocks = now) is LIVE — the guard must pass. This is why every @@ -1140,6 +1151,50 @@ mod live_session_guard_tests { assert!(require_live_session(&empty_state()).is_ok()); } + /// #760: `health()` measures every age against `NetHealth::clock`, not against a fresh + /// `Instant::now()`. Proved by pinning the clock 7s AHEAD of the stamps and reading the ages + /// back exactly — no sleeping, no tolerance window. Swap any `h.clock.age_of(x)` in `health()` + /// back to `x.elapsed()` and the corresponding assertion here reads ~0 instead of 7000. + /// + /// This is the half that makes the frozen fixture mean anything: a fixture can pin its clock all + /// it likes, but only if the projection actually reads that clock does the wall clock stop + /// deciding a test's HTTP status. + #[test] + fn health_ages_are_measured_against_the_injected_clock() { + let s = empty_state(); + let stamped = std::time::Instant::now(); + *s.net_health.lock().unwrap() = NetHealth { + // Clock pinned 7s after the stamps → every age must read exactly 7000ms. + clock: HealthClock::frozen_at(stamped + std::time::Duration::from_secs(7)), + last_datagram: stamped, + last_packet: stamped, + last_tick: stamped, + ..NetHealth::default() + }; + let h = s.health(); + assert_eq!(h.snapshot_age_ms, 7_000, "snapshot_age_ms must come from the injected clock"); + assert_eq!(h.link_age_ms, 7_000, "link_age_ms must come from the injected clock"); + assert_eq!(h.last_packet_age_ms, 7_000, "last_packet_age_ms must come from the injected clock"); + // And the guard's verdict follows that injected reading, not the wall: 7s > 5s bound. + let (code, msg) = require_live_session(&s).unwrap_err(); + assert_eq!(code, StatusCode::SERVICE_UNAVAILABLE); + assert!(msg.contains("has not ticked in 7000ms"), "message: {msg}"); + } + + /// #760: the shared handler fixture pins its clock, so no handler test's status can depend on + /// how loaded the machine was. Delete the `frozen_at` in `testkit::empty_state` (back to + /// `NetHealth::default()`) and this goes RED immediately — `is_frozen()` is false — which is the + /// point: the property is structural, not a margin. + #[test] + fn handler_fixture_pins_its_health_clock() { + let s = empty_state(); + assert!(s.net_health.lock().unwrap().clock.is_frozen(), + "empty_state() must pin the health clock so gated handler tests cannot age into a 503"); + let h = s.health(); + assert_eq!((h.snapshot_age_ms, h.link_age_ms, h.last_packet_age_ms), (0, 0, 0), + "a pinned fixture's ages are exactly zero, not merely small"); + } + /// A healthy but IDLE session — connected + ticking, yet the WORLD has been quiet for a full /// minute (`last_packet` 60s stale) — must NOT be rejected. This is the #343 over-rejection guard: /// keying the verdict on `last_packet` (world quiet) instead of `connected`/`last_tick` (thread diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index 0727d37b..3e534336 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -87,7 +87,8 @@ fn zone_assets_not_ready(s: &HttpState) -> Option { /// frozen map and no marker of any kind. /// /// **This is the SAME clock as `/debug`'s `snapshot_age_ms`, not a second one** — both are -/// `HttpState::health().snapshot_age_ms`, i.e. `NetHealth::last_tick.elapsed()` measured fresh on +/// `HttpState::health().snapshot_age_ms`, i.e. `NetHealth::last_tick`'s age against the health clock +/// (`Instant::now()` in every non-test build — see `HealthClock`, #760) measured fresh on /// every request (#343: an age is only true at the instant it's read, so nothing here is cached). /// `last_tick` is bumped, unconditionally, once per gameplay tick by the SAME `eq-net` thread loop /// iteration that publishes `GameState` and drains `ActionLoop::tick` — the single writer of every @@ -1816,7 +1817,7 @@ async fn get_zone_exits(State(s): State) -> Response { #[cfg(test)] mod tests { use super::*; - use crate::testkit::{ago, empty_state, set_gs}; + use crate::testkit::{ago, empty_state, empty_state_wall_clock, set_gs}; use axum::body::Body; use axum::http::Request; use tower::ServiceExt; @@ -2638,7 +2639,7 @@ mod tests { /// publisher has to be alive for the answer to be honest. #[tokio::test] async fn debug_reports_disconnected_when_the_world_froze_and_nothing_republished() { - let state = empty_state(); + let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock // The world as it was when the link was still up — a sitting character, full HP. set_gs(&state, |gs| { gs.player_name = "Gmkblr".into(); @@ -2730,7 +2731,7 @@ mod tests { /// same number across consecutive polls whenever the render loop slept. #[tokio::test] async fn last_packet_age_advances_between_reads_with_no_publisher_running() { - let state = empty_state(); + let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock state.net_health.lock().unwrap().last_packet = ago(5); let first = debug_json(state.clone()).await["player"]["last_packet_age_ms"].as_u64().unwrap(); // Nothing renders, nothing publishes, no packet arrives — just time passing. @@ -2744,7 +2745,7 @@ mod tests { /// SERVER went quiet is not the same failure as a client whose own network thread wedged. #[tokio::test] async fn server_silence_and_publisher_stall_are_distinguishable() { - let state = empty_state(); + let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock // The link is dead (no datagrams at all), but our network thread is fine and still ticking. { let mut h = state.net_health.lock().unwrap(); @@ -2766,7 +2767,7 @@ mod tests { /// the LINK clock, and `last_packet_age_ms` is left free to say "the world is quiet". #[tokio::test] async fn a_quiet_world_on_a_live_link_is_still_connected() { - let state = empty_state(); + let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock { let mut h = state.net_health.lock().unwrap(); h.last_packet = ago(45); // the world has nothing to say... @@ -2814,7 +2815,7 @@ mod tests { /// NOT reset it). #[tokio::test] async fn debug_reports_idle_but_answered_world_as_responsive() { - let state = empty_state(); + let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock { let mut h = state.net_health.lock().unwrap(); h.last_datagram = std::time::Instant::now(); @@ -3644,7 +3645,7 @@ mod tests { /// one. #[tokio::test] async fn snapshot_age_ms_climbs_across_reads_of_a_frozen_source() { - let state = empty_state(); + let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock state.net_health.lock().unwrap().last_tick = ago(5); let first = body_json(get(state.clone(), "/messages").await).await["snapshot_age_ms"].as_u64().unwrap(); // Nothing ticks, nothing republishes — just time passing, exactly like a wedged net thread. @@ -3658,7 +3659,7 @@ mod tests { /// Same climb, over the header channel this time (`/doors`, a bare-array endpoint). #[tokio::test] async fn snapshot_age_header_climbs_across_reads_of_a_frozen_source() { - let state = empty_state(); + let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock state.net_health.lock().unwrap().last_tick = ago(5); let first = header_age_ms(&get(state.clone(), "/doors").await); tokio::time::sleep(std::time::Duration::from_millis(50)).await; diff --git a/crates/eqoxide-http/src/testkit.rs b/crates/eqoxide-http/src/testkit.rs index 3f1471d8..ad3dfa06 100644 --- a/crates/eqoxide-http/src/testkit.rs +++ b/crates/eqoxide-http/src/testkit.rs @@ -31,6 +31,27 @@ pub fn set_gs(state: &HttpState, f: impl FnOnce(&mut eqoxide_core::game_state::G state.game_state.store(Arc::new(gs)); } +/// As [`empty_state`], but with the health clock left on the REAL WALL CLOCK (#760). +/// +/// For the small, deliberate set of tests whose SUBJECT is the clock: the #343 read-time-derivation +/// guards, which stamp a stale source and then assert the projected age is genuinely `>= N` seconds, +/// or that it CLIMBS between two reads with no publisher running. Those properties are meaningless +/// against a pinned clock — an age that cannot move cannot be shown to move — so they opt out here, +/// explicitly and visibly, rather than the whole fixture staying wall-driven for their sake. +/// +/// **Everything else must use [`empty_state`].** A handler test on a wall clock is racing +/// `SESSION_STALE_TICK_MS`: if the machine puts 5s between this call and the request, the handler +/// answers `503 stale session` instead of whatever the test asserted (#760). There are exactly seven +/// call sites of this function, all in `observe`'s clock tests; if you are adding an eighth, check +/// that the age moving is really the property under test. +pub fn empty_state_wall_clock() -> HttpState { + let state = empty_state(); + // `NetHealth::default()` is the production constructor: `HealthClock::WALL`, all three stamps at + // `Instant::now()`. This is exactly what `empty_state` used to hand every test. + *state.net_health.lock().unwrap() = crate::NetHealth::default(); + state +} + /// As [`empty_state`], but wired to a CALLER-OWNED `NetHealthShared` — the same `Arc` the network /// thread's `EqStream` stamps. Exposed for #612's cross-crate test: eqoxide-net drives a REAL /// `EqStream` into a REAL send failure and then asserts the failure is visible in THIS crate's @@ -121,7 +142,16 @@ pub fn empty_state() -> HttpState { chat: Default::default(), spells: std::sync::Arc::new(eqoxide_core::spells::SpellDb::default()), game_state: Arc::new(arc_swap::ArcSwap::from_pointee(eqoxide_core::game_state::GameState::new())), - net_health: Arc::new(Mutex::new(crate::NetHealth::default())), + // #760: a FROZEN health clock, stamped at the same instant it is pinned to, so every age + // `HttpState::health()` projects for this fixture is exactly 0 — permanently, and whatever + // the machine is doing. With `NetHealth::default()` (the real wall clock) the ages were + // "however long this test has been running", so a handler test that took >5s to reach its + // request got `503 stale session` from `require_live_session` instead of the status it + // asserted. That is #760, and it fired on a diff that had nothing to do with it. + // Tests that WANT a stale session build their own `NetHealth::frozen_at(now, …)`. + net_health: Arc::new(Mutex::new(crate::NetHealth::frozen_at( + std::time::Instant::now(), 0, 0, 0, + ))), frame_profile: Arc::new(Mutex::new(eqoxide_ipc::FrameProfile::default())), quest: Default::default(), group_slots: Default::default(), diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index 7a96b72c..4df7bf6a 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -355,6 +355,58 @@ impl SessionDropCause { } } +/// Where a health projection reads **"now"** from when it turns [`NetHealth`]'s stamps into ages. +/// +/// #760. Every age in the projection is `now - stamp`. In production `now` is the real monotonic +/// clock, which is correct and must stay that way — silence *is* the signal (#343). But a TEST +/// fixture stamps its clocks at construction and is then read some unbounded time later, so its +/// ages are a function of **how long the test took**, i.e. of machine load. That made +/// `combat::tests::cast_empty_gem_is_409_and_queues_nothing` answer `503 stale session` instead of +/// the `409` it asserts whenever the box was busy enough to put >5s (`SESSION_STALE_TICK_MS`) +/// between `empty_state()` and the request being served — measured, not theorised: sleeping 5.1s +/// between the fixture and that request reproduces the 503 exactly. +/// +/// Freezing the clock removes the wall clock from the test's answer entirely, rather than moving +/// the threshold it races against. A fixture built with [`NetHealth::frozen_at`] has +/// `now == stamp`, so every age it projects is **exactly** zero however long the test takes. +/// +/// **A release build cannot construct a frozen clock.** The inner field is private and the only +/// constructor that can set it, [`HealthClock::frozen_at`], is behind the `test-fixtures` feature — +/// so the frozen variant is *unrepresentable* outside a test build. That matters for the honesty +/// invariant in the other direction: a frozen health clock would pin `snapshot_age_ms` at 0 and +/// report a dead net thread as live forever, which is exactly the #343 lie this projection exists +/// to prevent. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct HealthClock(Option); + +impl HealthClock { + /// The real monotonic clock — the only clock a non-test build can have, and the `Default`. + pub const WALL: HealthClock = HealthClock(None); + + /// A clock PINNED at `t`. Test fixtures only (see the type doc for why this is gated). + #[cfg(any(test, feature = "test-fixtures"))] + pub fn frozen_at(t: std::time::Instant) -> Self { + HealthClock(Some(t)) + } + + /// The instant every age in the projection is measured back from. + pub fn now(self) -> std::time::Instant { + self.0.unwrap_or_else(std::time::Instant::now) + } + + /// True for a pinned clock. Lets a test assert its fixture really is load-independent instead + /// of inferring it from a reading that happened to be 0. + pub fn is_frozen(self) -> bool { + self.0.is_some() + } + + /// The age of `stamp` against this clock, saturating at zero for a stamp in the future (which a + /// pinned clock makes reachable: a test may re-stamp a field after freezing). + pub fn age_of(self, stamp: std::time::Instant) -> std::time::Duration { + self.now().saturating_duration_since(stamp) + } +} + /// The three clocks that answer "can I trust anything else in this payload?", owned and stamped by /// the network thread and turned into `Health` **at HTTP read time** (`HttpState::health`), never /// cached (#8, #343). They are deliberately separate signals, because they fail independently: @@ -388,6 +440,9 @@ impl SessionDropCause { /// still-ACKing reader thread; this server does not work that way — do not reason from that model.) #[derive(Debug, Clone, Copy)] pub struct NetHealth { + /// Where the health projection reads "now" from (#760). `HealthClock::WALL` in every build that + /// is not a test; see [`HealthClock`]. + pub clock: HealthClock, /// Last inbound datagram of ANY kind, session-layer ACKs/keepalives included → link liveness. pub last_datagram: std::time::Instant, /// Last inbound APPLICATION packet (a decoded opcode that mutated `GameState`) → world activity. @@ -639,6 +694,7 @@ impl Default for NetHealth { fn default() -> Self { let now = std::time::Instant::now(); NetHealth { + clock: HealthClock::WALL, last_datagram: now, last_packet: now, last_tick: now, last_probe_sent: None, last_probe_reply: None, first_unanswered_probe_sent: None, @@ -653,6 +709,37 @@ impl Default for NetHealth { } impl NetHealth { + /// #760: a fixture whose clock is PINNED at `now`, with `last_datagram`/`last_packet`/`last_tick` + /// stamped at that same instant *minus* the ages the caller asks for (in seconds). Every age the + /// health projection derives from it is then **exactly** the number passed in — not "that number + /// plus however long the test has been running so far" — so a test's liveness verdict cannot + /// depend on machine load. `(0, 0, 0)` is a perfectly live session, permanently. + /// + /// Test fixtures only; see [`HealthClock`] for why a release build cannot reach this. + #[cfg(any(test, feature = "test-fixtures"))] + pub fn frozen_at( + now: std::time::Instant, + datagram_ago_secs: u64, + tick_ago_secs: u64, + packet_ago_secs: u64, + ) -> Self { + // `checked_sub` + `expect` (the same shape as `eqoxide_http::testkit::ago`): on a host whose + // monotonic epoch is younger than the age asked for there is no instant to name, and a loud + // panic naming the cause beats silently clamping to the epoch and reporting a smaller age + // than the fixture asked for. + let back = |secs: u64| { + now.checked_sub(std::time::Duration::from_secs(secs)) + .expect("monotonic clock younger than the requested fixture age") + }; + NetHealth { + clock: HealthClock::frozen_at(now), + last_datagram: back(datagram_ago_secs), + last_tick: back(tick_ago_secs), + last_packet: back(packet_ago_secs), + ..NetHealth::default() + } + } + /// #656: stamp a `send_wouldblock_rescued`/`send_deferred` event and update the consecutive- /// burst streak that `send_starved` reads. Call this from BOTH increment sites (never bump /// `send_pressure_streak`/`last_send_pressure_at` any other way, or the two would drift from the @@ -2099,6 +2186,76 @@ mod world_responsive_tests { } } +/// #760: `HealthClock` — where a health projection reads "now" when it turns [`NetHealth`]'s +/// stamps into ages. These pin the two properties the fix rests on: the production/`Default` clock +/// is the real wall clock (anything else would freeze every published age at process start — the +/// #343 lie), and a pinned clock yields ages that are a pure function of the stamps, so a fixture +/// built from it cannot drift with machine load. +#[cfg(test)] +mod health_clock_tests { + use super::{HealthClock, NetHealth}; + + // ── `HealthClock` — where a health projection reads "now" (#760) ──────────────────────────── + + /// The production clock is the wall clock, and `Default` is the production clock. If this ever + /// flipped, every age the client publishes would freeze at process start and `connected` would + /// latch true forever — the exact #343 lie. + #[test] + fn the_default_health_clock_is_the_wall_clock() { + assert!(!HealthClock::default().is_frozen(), "the default clock must be the real wall clock"); + assert!(!HealthClock::WALL.is_frozen()); + assert!(!NetHealth::default().clock.is_frozen(), + "a NetHealth built the production way must carry the wall clock"); + let before = std::time::Instant::now(); + let a = HealthClock::WALL.now(); + assert!(a >= before, "the wall clock must actually read the wall clock"); + } + + /// A pinned clock reads back the SAME instant every time, so an age against it is a pure + /// function of the two values and cannot move with machine load. This is the whole of #760. + #[test] + fn a_frozen_clock_does_not_advance_and_ages_exactly() { + let t = std::time::Instant::now(); + let c = HealthClock::frozen_at(t); + assert!(c.is_frozen()); + assert_eq!(c.now(), t); + std::thread::sleep(std::time::Duration::from_millis(30)); + assert_eq!(c.now(), t, "a pinned clock must not advance with wall time"); + assert_eq!(c.age_of(t), std::time::Duration::ZERO, + "a stamp taken at the pin reads exactly zero, however long ago that was"); + assert_eq!( + c.age_of(t - std::time::Duration::from_secs(4)), + std::time::Duration::from_secs(4), + "and an older stamp reads exactly its own age, with no wall-clock drift added"); + } + + /// A stamp AHEAD of the clock saturates to zero rather than panicking — reachable once the clock + /// is pinnable, because a fixture may re-stamp a field after freezing. + #[test] + fn a_stamp_ahead_of_the_clock_saturates_to_zero() { + let t = std::time::Instant::now(); + let c = HealthClock::frozen_at(t); + assert_eq!(c.age_of(t + std::time::Duration::from_secs(9)), std::time::Duration::ZERO); + } + + /// `NetHealth::frozen_at` stamps the three liveness clocks at exactly the ages asked for, + /// measured against its own pin — so a fixture's `(0,0,0)` is a permanently live session and its + /// `(20,0,20)` is a permanently disconnected one, with no margin for load to eat. + #[test] + fn net_health_frozen_at_yields_exact_ages() { + let now = std::time::Instant::now(); + let h = NetHealth::frozen_at(now, 20, 6, 41); + std::thread::sleep(std::time::Duration::from_millis(30)); + assert_eq!(h.clock.age_of(h.last_datagram), std::time::Duration::from_secs(20)); + assert_eq!(h.clock.age_of(h.last_tick), std::time::Duration::from_secs(6)); + assert_eq!(h.clock.age_of(h.last_packet), std::time::Duration::from_secs(41)); + let live = NetHealth::frozen_at(std::time::Instant::now(), 0, 0, 0); + std::thread::sleep(std::time::Duration::from_millis(30)); + assert_eq!(live.clock.age_of(live.last_tick), std::time::Duration::ZERO, + "a (0,0,0) fixture stays exactly fresh no matter how long the test runs"); + } +} + /// #656: the io-driver-starvation alert, tested as a pure function (`send_starved`) plus the /// `NetHealth::record_send_pressure` state machine that feeds it. These pin the fire/clear /// boundary the issue asked for: a signal that is TRUE while starvation is actually happening and From 54f813102e81b2b45d64a4f171e512bace12020c Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 04:23:07 -0400 Subject: [PATCH 2/6] fix(#760, review B1): derive every past-dated net-health stamp from the clock that reads it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the fix re-armed #760's failure mode one level down: `debug_reports_world_unresponsive_when_a_probe_goes_unanswered_while_the_link_acks` stayed on the now-frozen `empty_state()` while stamping `first_unanswered_probe_sent = Some(ago(15))` — a wall-clock stamp read back against a clock pinned at fixture construction. Effective age `15s − (time since empty_state())` against a 10s bound: a 5s margin machine load eats, in the same direction as the bug being fixed. Before this PR the age GREW, so it was unconditionally safe; the PR made a safe test fragile. Structural fix rather than moving that one test back to the wall clock: add `HealthClock::ago`, the inverse of `age_of`, so a stamp is always derived from the clock that will read it. It is correct for BOTH clocks — on a wall clock it is exactly `Instant::now() - N` — so the flagged form has no case where it is the right one. All nine net-health stamp sites in `observe.rs` move to it; the seven wall-clock tests keep `empty_state_wall_clock()` because their subject really is that an age moves. The prose rule did not prevent this (it named only "an age that must MOVE", not "an age that must EXCEED a bound"), so the rule is now executable: a source-scanning guard test bans a bare `ago(` on any net-health stamp assignment across observe/lib/combat/testkit. The doc rule is widened to name both shapes and to point at the guard. Also: narrow the `HealthClock` unrepresentability claim to what is actually true — the field is private to the crate, so the guarantee is against every OTHER crate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-http/src/observe.rs | 98 ++++++++++++++++++++++++------ crates/eqoxide-http/src/testkit.rs | 26 +++++++- crates/eqoxide-ipc/src/lib.rs | 30 ++++++++- 3 files changed, 130 insertions(+), 24 deletions(-) diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index 3e534336..b5ae7423 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -2651,9 +2651,10 @@ mod tests { // genuinely gone), and no publish of any kind. { let mut h = state.net_health.lock().unwrap(); - h.last_datagram = ago(60); - h.last_packet = ago(60); - h.last_tick = ago(60); + let c = h.clock; + h.last_datagram = c.ago(60); + h.last_packet = c.ago(60); + h.last_tick = c.ago(60); } let v = debug_json(state).await; @@ -2732,7 +2733,7 @@ mod tests { #[tokio::test] async fn last_packet_age_advances_between_reads_with_no_publisher_running() { let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock - state.net_health.lock().unwrap().last_packet = ago(5); + { let mut h = state.net_health.lock().unwrap(); let c = h.clock; h.last_packet = c.ago(5); } let first = debug_json(state.clone()).await["player"]["last_packet_age_ms"].as_u64().unwrap(); // Nothing renders, nothing publishes, no packet arrives — just time passing. tokio::time::sleep(std::time::Duration::from_millis(50)).await; @@ -2749,8 +2750,9 @@ mod tests { // The link is dead (no datagrams at all), but our network thread is fine and still ticking. { let mut h = state.net_health.lock().unwrap(); - h.last_datagram = ago(30); - h.last_packet = ago(30); + let c = h.clock; + h.last_datagram = c.ago(30); + h.last_packet = c.ago(30); } let p = debug_json(state).await["player"].clone(); assert_eq!(p["connected"], serde_json::json!(false)); @@ -2770,8 +2772,9 @@ mod tests { let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock { let mut h = state.net_health.lock().unwrap(); - h.last_packet = ago(45); // the world has nothing to say... - h.last_datagram = std::time::Instant::now(); // ...but the link is demonstrably alive. + let c = h.clock; + h.last_packet = c.ago(45); // the world has nothing to say... + h.last_datagram = c.now(); // ...but the link is demonstrably alive. } let p = debug_json(state).await["player"].clone(); assert_eq!(p["connected"], serde_json::json!(true), @@ -2792,15 +2795,22 @@ mod tests { let state = empty_state(); { let mut h = state.net_health.lock().unwrap(); - h.last_datagram = std::time::Instant::now(); // link is demonstrably alive (ACKing)... - h.last_packet = ago(30); // ...but the world has produced nothing... - h.last_probe_sent = Some(ago(15)); // ...and our probe (15s ago) went... + // #760/B1: every stamp below is derived from the fixture's OWN health clock (`c`), which + // is what `health()` will read them back against. Written as `ago(15)` — the wall clock — + // this test's probe age became `15s − (time since empty_state())` and the assertion below + // needed it to clear a 10s bound: a 5s margin that machine load ate. Measured, with a + // 5.1s sleep injected after `empty_state()`: `ago(15)` → world_responsive `true` + // (assertion FAILS), `c.ago(15)` → `false` (passes). Same sleep, opposite outcome. + let c = h.clock; + h.last_datagram = c.now(); // link is demonstrably alive (ACKing)... + h.last_packet = c.ago(30); // ...but the world has produced nothing... + h.last_probe_sent = Some(c.ago(15)); // ...and our probe (15s ago) went... h.last_probe_reply = None; // ...unanswered, past the 10s bound. // #371 wedge-flicker fix: `health()` reads the wedge-timeout clock off // `first_unanswered_probe_sent`, not `last_probe_sent` — this is the first (and, in this // scenario, only) unanswered send of the streak, so in production `record_probe_sent` // would have stamped both together. Mirror that here. - h.first_unanswered_probe_sent = Some(ago(15)); + h.first_unanswered_probe_sent = Some(c.ago(15)); } let p = debug_json(state).await["player"].clone(); assert_eq!(p["connected"], serde_json::json!(true), @@ -2809,6 +2819,55 @@ mod tests { "an unanswered probe on a live link is a WEDGED world — the #371 signal must fire"); } + /// #760/B1 — the rule about past-dated net-health stamps, as a TEST rather than as a doc comment. + /// + /// A stamp written `ago(N)` comes from the wall clock; `health()` reads it back against + /// `NetHealth::clock`. On a frozen fixture those are different clocks, so the age is + /// `N − (time since the fixture was built)` — it SHRINKS with machine load, silently, and any + /// assertion needing it to stay above a bound has a margin that load eats. That is #760's own + /// failure mode one level down, and it is what review found in + /// `debug_reports_world_unresponsive_when_a_probe_goes_unanswered_while_the_link_acks`. + /// + /// A prose rule did not prevent it (the rule named only "an age that must move", not "an age + /// that must exceed a bound"), so this scans the source instead. `HealthClock::ago` is correct + /// for BOTH clocks — on a wall clock it is exactly `Instant::now() - N` — so there is no case + /// where the flagged form is the right one, which is why this is a blanket ban and not a + /// judgement call. Bare `Instant::now()` is deliberately NOT flagged: it dates a stamp to the + /// present, which saturates to age 0 against either clock. + #[test] + fn no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it() { + /// Exactly the `NetHealth` fields `HttpState::health()` turns into an age. Adding a stamp + /// field to that projection without adding it here leaves the new field unguarded. + const STAMP_FIELDS: [&str; 8] = [ + "last_datagram", "last_packet", "last_tick", + "last_probe_sent", "last_probe_reply", "first_unanswered_probe_sent", + "last_send_pressure_at", "last_send_error_at", + ]; + let sources = [ + ("observe.rs", include_str!("observe.rs")), + ("lib.rs", include_str!("lib.rs")), + ("combat.rs", include_str!("combat.rs")), + ("testkit.rs", include_str!("testkit.rs")), + ]; + let mut offenders = Vec::new(); + for (file, src) in sources { + for (i, line) in src.lines().enumerate() { + let code = line.trim_start(); + if code.starts_with("//") { continue; } // prose, including this doc block + let Some(eq) = code.find('=') else { continue }; + let (lhs, rhs) = code.split_at(eq); + if !STAMP_FIELDS.iter().any(|f| lhs.contains(f)) { continue; } + if !rhs.contains("ago(") { continue; } + if rhs.contains(".ago(") { continue; } // clock-relative: the correct form + offenders.push(format!("{file}:{}: {}", i + 1, code.trim_end())); + } + } + assert!(offenders.is_empty(), + "these past-date a net-health stamp from the wall clock while `health()` will read it \ + back against `NetHealth::clock` — use `let c = h.clock; … = c.ago(N)` instead (#760):\n{}", + offenders.join("\n")); + } + /// #371, the false-alarm we must NOT raise (the #343 trap in reverse): a legitimately idle world /// — 45s with no spontaneous packet — whose probe IS answered stays `world_responsive: true`, /// while `last_packet_age_ms` still honestly reports the 45s of app-silence (the probe reply does @@ -2818,10 +2877,11 @@ mod tests { let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock { let mut h = state.net_health.lock().unwrap(); - h.last_datagram = std::time::Instant::now(); - h.last_packet = ago(45); // no spontaneous world output for 45s (normal idle) - h.last_probe_sent = Some(ago(20)); - h.last_probe_reply = Some(ago(2)); // ...but the probe was answered 2s ago → alive + let c = h.clock; + h.last_datagram = c.now(); + h.last_packet = c.ago(45); // no spontaneous world output for 45s (normal idle) + h.last_probe_sent = Some(c.ago(20)); + h.last_probe_reply = Some(c.ago(2)); // ...but the probe was answered 2s ago → alive // `first_unanswered_probe_sent` deliberately left `None` (the `empty_state()` default): in // production `record_probe_reply` clears it the instant a genuine reply lands, so an // ANSWERED probe's real state has no outstanding streak at all — this is what makes @@ -2844,7 +2904,7 @@ mod tests { #[tokio::test] async fn debug_defaults_world_responsive_true_before_the_first_probe() { let state = empty_state(); - state.net_health.lock().unwrap().last_packet = ago(20); + { let mut h = state.net_health.lock().unwrap(); let c = h.clock; h.last_packet = c.ago(20); } let p = debug_json(state).await["player"].clone(); assert_eq!(p["world_responsive"], serde_json::json!(true), "no probe sent yet → no verdict → true (read connected/last_packet_age_ms instead)"); @@ -3646,7 +3706,7 @@ mod tests { #[tokio::test] async fn snapshot_age_ms_climbs_across_reads_of_a_frozen_source() { let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock - state.net_health.lock().unwrap().last_tick = ago(5); + { let mut h = state.net_health.lock().unwrap(); let c = h.clock; h.last_tick = c.ago(5); } let first = body_json(get(state.clone(), "/messages").await).await["snapshot_age_ms"].as_u64().unwrap(); // Nothing ticks, nothing republishes — just time passing, exactly like a wedged net thread. tokio::time::sleep(std::time::Duration::from_millis(50)).await; @@ -3660,7 +3720,7 @@ mod tests { #[tokio::test] async fn snapshot_age_header_climbs_across_reads_of_a_frozen_source() { let state = empty_state_wall_clock(); // #760: this test's subject IS the wall clock - state.net_health.lock().unwrap().last_tick = ago(5); + { let mut h = state.net_health.lock().unwrap(); let c = h.clock; h.last_tick = c.ago(5); } let first = header_age_ms(&get(state.clone(), "/doors").await); tokio::time::sleep(std::time::Duration::from_millis(50)).await; let second = header_age_ms(&get(state, "/doors").await); diff --git a/crates/eqoxide-http/src/testkit.rs b/crates/eqoxide-http/src/testkit.rs index ad3dfa06..1efb29c9 100644 --- a/crates/eqoxide-http/src/testkit.rs +++ b/crates/eqoxide-http/src/testkit.rs @@ -15,7 +15,13 @@ use std::sync::{Arc, Mutex}; use crate::HttpState; -/// An `Instant` `secs` in the past (saturating — a just-booted host can't go below its epoch). +/// An `Instant` `secs` in the past, on the WALL clock. +/// +/// Fine for stamps that are aged against `Instant::now()` at read time — the asset-sync publisher's +/// `tick_stamped`, for instance. **Not** for a `NetHealth` stamp: `health()` ages those against +/// `NetHealth::clock`, which a fixture pins, so a wall-clock stamp read back against it drifts by +/// however long the test took (#760). Use `h.clock.ago(secs)` there; the ban is enforced by +/// `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it`. pub fn ago(secs: u64) -> std::time::Instant { std::time::Instant::now() .checked_sub(std::time::Duration::from_secs(secs)) @@ -42,8 +48,22 @@ pub fn set_gs(state: &HttpState, f: impl FnOnce(&mut eqoxide_core::game_state::G /// **Everything else must use [`empty_state`].** A handler test on a wall clock is racing /// `SESSION_STALE_TICK_MS`: if the machine puts 5s between this call and the request, the handler /// answers `503 stale session` instead of whatever the test asserted (#760). There are exactly seven -/// call sites of this function, all in `observe`'s clock tests; if you are adding an eighth, check -/// that the age moving is really the property under test. +/// call sites of this function, all in `observe`'s clock tests. +/// +/// If you are adding an eighth, there are **two** shapes to tell apart, and the earlier version of +/// this rule named only the first — which is precisely how a load-fragile test got past review: +/// +/// 1. **An age that must MOVE** (`assert!(second > first)` across two reads). Genuinely needs the +/// wall clock: an age that cannot move cannot be shown to move. This function is for these. +/// 2. **An age that must EXCEED A BOUND** (`assert!(age >= N)`, or a stamp placed past a timeout so +/// a verdict fires). These do **not** need the wall clock, and must not use it — stay on +/// [`empty_state`] and derive the stamp from the fixture's own clock (`let c = h.clock; +/// h.last_probe_sent = Some(c.ago(15));`). A wall-clock `ago(15)` read back against a pinned +/// clock ages to `15s − (time since the fixture was built)`, so the margin over the bound is +/// eaten by machine load — #760's failure mode, one level down. +/// +/// Shape 2 is enforced, not merely documented: see +/// `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it`. pub fn empty_state_wall_clock() -> HttpState { let state = empty_state(); // `NetHealth::default()` is the production constructor: `HealthClock::WALL`, all three stamps at diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index 4df7bf6a..f466ecef 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -370,8 +370,13 @@ impl SessionDropCause { /// the threshold it races against. A fixture built with [`NetHealth::frozen_at`] has /// `now == stamp`, so every age it projects is **exactly** zero however long the test takes. /// -/// **A release build cannot construct a frozen clock.** The inner field is private and the only -/// constructor that can set it, [`HealthClock::frozen_at`], is behind the `test-fixtures` feature — +/// **A release build cannot construct a frozen clock.** The inner field is private *to this crate* +/// — production code inside `eqoxide-ipc`'s own `src/` could still write `HealthClock(Some(t))` +/// directly, which is the normal Rust module boundary; the guarantee is against every OTHER crate, +/// and it is measured, not reasoned (a `compile_error!` probe on the `test-fixtures` feature does +/// not fire in `cargo build --release --bin eqoxide`, and does fire under `cargo test`). The only +/// constructor that can set it from outside, [`HealthClock::frozen_at`], is behind the +/// `test-fixtures` feature — /// so the frozen variant is *unrepresentable* outside a test build. That matters for the honesty /// invariant in the other direction: a frozen health clock would pin `snapshot_age_ms` at 0 and /// report a dead net thread as live forever, which is exactly the #343 lie this projection exists @@ -405,6 +410,27 @@ impl HealthClock { pub fn age_of(self, stamp: std::time::Instant) -> std::time::Duration { self.now().saturating_duration_since(stamp) } + + /// The stamp that THIS clock will read back as exactly `secs` old — the inverse of [`age_of`]. + /// + /// Every past-dated net-health stamp in a test must come from here rather than from a bare + /// `Instant::now() - secs`. On a wall clock the two are the same expression; on a PINNED clock + /// they are not, and the difference is a silent, drifting error: + /// `now() - secs` read back against a clock pinned at fixture construction ages to + /// `secs − (time since construction)`, so any assertion that needs the stamp to stay ABOVE a + /// bound has a margin that machine load eats. That is #760's own failure mode, re-armed one + /// level down — it happened, in review, to `debug_reports_world_unresponsive_when_a_probe_goes_ + /// unanswered_while_the_link_acks` (a 15s stamp that had to clear a 10s bound: a 5s margin). + /// Deriving the stamp from the clock that will read it makes the age exact for both kinds of + /// clock, so the correct call has no wrong variant to choose between. + /// + /// [`age_of`]: HealthClock::age_of + #[cfg(any(test, feature = "test-fixtures"))] + pub fn ago(self, secs: u64) -> std::time::Instant { + self.now() + .checked_sub(std::time::Duration::from_secs(secs)) + .expect("monotonic clock younger than the requested stamp age") + } } /// The three clocks that answer "can I trust anything else in this payload?", owned and stamped by From c326e74d5e5e336b8d223f572d27d91bdfd344c7 Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 04:36:33 -0400 Subject: [PATCH 3/6] test(#760, review B1): pin `HealthClock::ago` as the inverse of `age_of`, and the cross-clock drift it removes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asserts the property the B1 fix rests on in both directions: on one clock `ago` and `age_of` are exact inverses (pinned and wall alike, so a test never has to choose a spelling), and a wall-clock stamp read back against a pinned clock ages SHORT — the drift that ate a threshold test's margin. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-ipc/src/lib.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index 4ff8f119..eb238447 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -2312,6 +2312,29 @@ mod health_clock_tests { "and an older stamp reads exactly its own age, with no wall-clock drift added"); } + /// `ago` is the exact inverse of `age_of` **on the same clock**, for a pinned clock and for the + /// wall clock alike — that is the whole reason a test never has to choose between two spellings. + /// The second half is the one that matters: a stamp taken from the WALL clock and read back + /// against a PINNED one drifts by the gap between them, which is #760's failure mode one level + /// down (review finding B1). Asserted here as a strict inequality, not a tolerance window. + #[test] + fn ago_is_the_inverse_of_age_of_on_the_same_clock_and_drifts_across_clocks() { + let pin = std::time::Instant::now(); + let frozen = HealthClock::frozen_at(pin); + assert_eq!(frozen.age_of(frozen.ago(15)), std::time::Duration::from_secs(15), + "on a pinned clock, ago(15) must read back as exactly 15s"); + assert_eq!(HealthClock::WALL.age_of(HealthClock::WALL.ago(0)), std::time::Duration::ZERO, + "and the wall clock is the same operation, not a special case"); + + // The cross-clock error the guard test exists to ban: stamp from the wall clock AFTER the + // pin, read back against the pin. The gap is real elapsed time, so the age comes back SHORT. + std::thread::sleep(std::time::Duration::from_millis(40)); + let wall_stamp = HealthClock::WALL.ago(15); + assert!(frozen.age_of(wall_stamp) < std::time::Duration::from_secs(15), + "a wall-clock stamp read against a pinned clock must age SHORT — this is the drift that \ + eats a threshold test's margin (#760/B1)"); + } + /// A stamp AHEAD of the clock saturates to zero rather than panicking — reachable once the clock /// is pinnable, because a fixture may re-stamp a field after freezing. #[test] From c83e8b896c1535b6bbfc069d335dab7acd99ea02 Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 05:15:04 -0400 Subject: [PATCH 4/6] fix(#760): state the wall-clock half of the ago/age_of law as the inequality that is true MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first draft asserted `age_of(ago(0)) == ZERO` on the wall clock; it failed at `left: 100ns` because `now` advances between the two calls. That is this PR's own defect class — a wall-clock-dependent assertion — and the suite caught it. Monotonicity gives the claim that needs no tolerance window: the age can only read LONGER than asked, never shorter. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-ipc/src/lib.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index eb238447..caee6a1c 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -2323,8 +2323,14 @@ mod health_clock_tests { let frozen = HealthClock::frozen_at(pin); assert_eq!(frozen.age_of(frozen.ago(15)), std::time::Duration::from_secs(15), "on a pinned clock, ago(15) must read back as exactly 15s"); - assert_eq!(HealthClock::WALL.age_of(HealthClock::WALL.ago(0)), std::time::Duration::ZERO, - "and the wall clock is the same operation, not a special case"); + // The wall clock's version of the same law, stated as the inequality that is actually true: + // `now` advances between the two calls, so the age can only read LONGER than asked, never + // shorter. (Writing this as `assert_eq!(…, ZERO)` is what a first draft said; it failed at + // `left: 100ns`, which is this PR's own defect class — a wall-clock-dependent assertion — + // caught by the suite. Monotonicity gives a bound that needs no tolerance window.) + assert!(HealthClock::WALL.age_of(HealthClock::WALL.ago(15)) >= std::time::Duration::from_secs(15), + "on the wall clock, ago(15) must read back as AT LEAST 15s — a monotonic clock cannot \ + make a stamp younger than it was minted"); // The cross-clock error the guard test exists to ban: stamp from the wall clock AFTER the // pin, read back against the pin. The gap is real elapsed time, so the age comes back SHORT. From 7be3195e2b9bf860d2a6d357345eca7ea9d4af1c Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 06:41:19 -0400 Subject: [PATCH 5/6] =?UTF-8?q?fix(#760):=20round=203=20=E2=80=94=20widen?= =?UTF-8?q?=20the=20guard=20to=20statements,=20make=20the=20exemption=20re?= =?UTF-8?q?ceiver-aware,=20and=20stop=20the=20docs=20overstating=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 (B2/B3), both findings about tracked prose rather than code. B2 — the guard matched ONE physical line: first `=` splits it, lhs must name a stamp field, rhs must contain `ago(` and not `.ago(`. Review injected seven shapes and it caught 1 of 6 violations. The misses included the shape rustfmt itself writes (an assignment wrapped so the `=` and the `ago(` land on different lines), which already occurs on NetHealth stamps in eqoxide-net today, and `wall.ago(30)` — exempt because the `.ago(` exemption never looked at the receiver, though a WALL receiver stamped into a pinned fixture IS #760. The unit of matching is now a comment-stripped STATEMENT (`/* */` and `//` stripped, then cut at `;`, `{`, `}`), and the exemption is receiver-aware: only `c.ago(` and `….clock.ago(` are exempt, so a bare `ago(` and `WALL.ago(` are both flagged. Also flags `now() -` and `checked_sub`, the past-dating forms review's widened sweep found that a bare `ago(` grep cannot see. The remaining blind spot — aliasing through a local — is named in the test's own doc, because a text scan cannot close it. testkit.rs no longer says the ban "is enforced by" the guard; it says partially guarded and points at the measured blind spots. B3 — the r2 commit corrected the test to `>=` but left "exactly `secs` old" / "exact for both kinds of clock" / "exact inverse … alike" in the docs two lines above it. On the wall clock it is not exact, which is why the assertion is an inequality. Exactness claims deleted rather than restated. Non-blocking: eqoxide-ipc's `test-fixtures` doc now mentions the health-clock injection points it exposes, not just `Roster::insert_for_test`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-http/src/observe.rs | 97 ++++++++++++++++++++++++++---- crates/eqoxide-http/src/testkit.rs | 13 ++-- crates/eqoxide-ipc/Cargo.toml | 4 ++ crates/eqoxide-ipc/src/lib.rs | 19 ++++-- 4 files changed, 112 insertions(+), 21 deletions(-) diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index 2801d597..5b0c4fb9 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -2834,6 +2834,29 @@ mod tests { /// where the flagged form is the right one, which is why this is a blanket ban and not a /// judgement call. Bare `Instant::now()` is deliberately NOT flagged: it dates a stamp to the /// present, which saturates to age 0 against either clock. + /// + /// # What this actually matches, and what it therefore cannot see + /// + /// The unit of matching is a **comment-stripped statement**, not a physical line: the source is + /// stripped of `//` and `/* */` comments and then cut at `;`, `{` and `}`. A statement offends + /// when it mentions one of the eight [`STAMP_FIELDS`] *and* past-dates via a form that is not + /// clock-relative. This unit was chosen after review measured the physical-line version catching + /// **1 of 6** injected violation shapes; the misses included the shape **rustfmt itself writes** + /// (an assignment wrapped so the `=` and the `ago(` land on different lines), which already + /// occurs on `NetHealth` stamps in `eqoxide-net` today — benign there only because those + /// fixtures read on the wall clock. + /// + /// Past-dating forms flagged: any `ago(` whose receiver is not the reading clock (see + /// `receiver_is_the_reading_clock` — a bare `ago(` and `WALL.ago(` are both flagged; exempting + /// *any* `.ago(` regardless of receiver was review's sharpest miss), plus `now() -` / `now()-` + /// and `checked_sub`. Those last two are here because review's own sweep found past-dated sites + /// a bare `ago(` grep cannot see: **grep the concept, not the spelling.** + /// + /// Known blind spot, stated because it is real and not closable by a text scan: **aliasing + /// through a local.** `let s = ago(15); h.last_probe_sent = Some(s);` is two statements, neither + /// of which contains both halves, so it lands. Closing it needs dataflow, not text. Two lesser + /// ones: files outside the four scanned, and a ninth `Instant` field added to `NetHealth` and to + /// `health()` but not to [`STAMP_FIELDS`] — there is no cross-check against the struct. #[test] fn no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it() { /// Exactly the `NetHealth` fields `HttpState::health()` turns into an age. Adding a stamp @@ -2843,6 +2866,56 @@ mod tests { "last_probe_sent", "last_probe_reply", "first_unanswered_probe_sent", "last_send_pressure_at", "last_send_error_at", ]; + + /// Comment-stripped statements, each kept as its (1-based line, text) fragments so an + /// offender can still be reported at the exact line that names the field. + fn statements(src: &str) -> Vec> { + let mut out: Vec> = Vec::new(); + let mut cur: Vec<(usize, String)> = Vec::new(); + let mut in_block = false; + for (i, raw) in src.lines().enumerate() { + // Strip `/* … */` (possibly spanning lines), then a trailing `//`. + let (mut code, mut rest) = (String::new(), raw); + while !rest.is_empty() { + if in_block { + match rest.find("*/") { + Some(k) => { in_block = false; rest = &rest[k + 2..]; } + None => break, + } + } else { + match rest.find("/*") { + Some(k) => { code.push_str(&rest[..k]); in_block = true; rest = &rest[k + 2..]; } + None => { code.push_str(rest); break; } + } + } + } + if let Some(k) = code.find("//") { code.truncate(k); } + // Cut at statement and block boundaries so unrelated code never merges into one + // statement (which would invent co-occurrences and fire falsely). + for (n, piece) in code.split([';', '{', '}']).enumerate() { + if n > 0 && !cur.is_empty() { out.push(std::mem::take(&mut cur)); } + if !piece.trim().is_empty() { cur.push((i + 1, piece.to_string())); } + } + } + if !cur.is_empty() { out.push(cur); } + out + } + + /// Is this `ago(` call's RECEIVER the health clock that will read the stamp back? + /// + /// Exempt spellings are `c.ago(` (the convention every call site uses, `let c = h.clock;`) + /// and `….clock.ago(`. Everything else is flagged — a free `ago(`, and equally + /// `HealthClock::WALL.ago(` or any other receiver. The old rule exempted *any* `.ago(`, + /// which review showed lets `wall.ago(30)` into a pinned fixture: that is #760 exactly. + /// + /// This is a SPELLING rule, so it is deliberately conservative — a correct stamp taken from + /// a binding named something other than `c` is flagged too. The fix is to rename it to `c`. + fn receiver_is_the_reading_clock(prefix: &str) -> bool { + if prefix.ends_with(".clock.") { return true; } + let Some(rest) = prefix.strip_suffix("c.") else { return false }; + !rest.chars().next_back().is_some_and(|ch| ch.is_alphanumeric() || ch == '_') + } + let sources = [ ("observe.rs", include_str!("observe.rs")), ("lib.rs", include_str!("lib.rs")), @@ -2851,20 +2924,22 @@ mod tests { ]; let mut offenders = Vec::new(); for (file, src) in sources { - for (i, line) in src.lines().enumerate() { - let code = line.trim_start(); - if code.starts_with("//") { continue; } // prose, including this doc block - let Some(eq) = code.find('=') else { continue }; - let (lhs, rhs) = code.split_at(eq); - if !STAMP_FIELDS.iter().any(|f| lhs.contains(f)) { continue; } - if !rhs.contains("ago(") { continue; } - if rhs.contains(".ago(") { continue; } // clock-relative: the correct form - offenders.push(format!("{file}:{}: {}", i + 1, code.trim_end())); + for stmt in statements(src) { + let joined: String = stmt.iter().map(|(_, t)| t.as_str()).collect(); + let Some((line, _)) = stmt.iter().find(|(_, t)| STAMP_FIELDS.iter().any(|f| t.contains(f))) + else { continue }; + let past_dated = joined.match_indices("ago(") + .any(|(k, _)| !receiver_is_the_reading_clock(&joined[..k])) + || joined.contains("now() -") + || joined.contains("now()-") + || joined.contains("checked_sub"); + if !past_dated { continue; } + offenders.push(format!("{file}:{line}: {}", joined.trim())); } } assert!(offenders.is_empty(), - "these past-date a net-health stamp from the wall clock while `health()` will read it \ - back against `NetHealth::clock` — use `let c = h.clock; … = c.ago(N)` instead (#760):\n{}", + "these past-date a net-health stamp from a clock other than the one `health()` will read \ + it back against — use `let c = h.clock; … = c.ago(N)` instead (#760):\n{}", offenders.join("\n")); } diff --git a/crates/eqoxide-http/src/testkit.rs b/crates/eqoxide-http/src/testkit.rs index 1efb29c9..474b58fe 100644 --- a/crates/eqoxide-http/src/testkit.rs +++ b/crates/eqoxide-http/src/testkit.rs @@ -20,8 +20,12 @@ use crate::HttpState; /// Fine for stamps that are aged against `Instant::now()` at read time — the asset-sync publisher's /// `tick_stamped`, for instance. **Not** for a `NetHealth` stamp: `health()` ages those against /// `NetHealth::clock`, which a fixture pins, so a wall-clock stamp read back against it drifts by -/// however long the test took (#760). Use `h.clock.ago(secs)` there; the ban is enforced by -/// `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it`. +/// however long the test took (#760). Use `h.clock.ago(secs)` there. That ban is **partially** +/// guarded by `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_ +/// one_that_reads_it` — a source scan over four files that reads one statement at a time, so it does +/// catch the rustfmt-wrapped and struct-literal spellings but cannot see a stamp aliased through a +/// local. Read that test's doc for the measured blind spots before relying on it: a wrong spelling +/// it misses still compiles and still lands. pub fn ago(secs: u64) -> std::time::Instant { std::time::Instant::now() .checked_sub(std::time::Duration::from_secs(secs)) @@ -62,8 +66,9 @@ pub fn set_gs(state: &HttpState, f: impl FnOnce(&mut eqoxide_core::game_state::G /// clock ages to `15s − (time since the fixture was built)`, so the margin over the bound is /// eaten by machine load — #760's failure mode, one level down. /// -/// Shape 2 is enforced, not merely documented: see -/// `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it`. +/// Shape 2 is documented here and **partially** guarded by +/// `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it`, +/// which names its own measured blind spots. Do not read it as making the mistake unlandable. pub fn empty_state_wall_clock() -> HttpState { let state = empty_state(); // `NetHealth::default()` is the production constructor: `HealthClock::WALL`, all three stamps at diff --git a/crates/eqoxide-ipc/Cargo.toml b/crates/eqoxide-ipc/Cargo.toml index f06d495d..02ccd89a 100644 --- a/crates/eqoxide-ipc/Cargo.toml +++ b/crates/eqoxide-ipc/Cargo.toml @@ -12,6 +12,10 @@ path = "src/lib.rs" # deliberately-mismatched entity roster. Enable it as a DEV-dependency feature only — it is what # keeps a roster writable at all, and production must have exactly one writer (#643). See the # `Roster` doc comment. +# +# Also exposes the health-clock injection points `HealthClock::frozen_at`, `HealthClock::ago` and +# `NetHealth::frozen_at` (#760) — which is why `eqoxide-http` forwards to this feature: a handler +# test must be able to pin the clock `HttpState::health()` ages every stamp against. test-fixtures = [] [dependencies] diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index caee6a1c..4f5eef65 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -411,7 +411,12 @@ impl HealthClock { self.now().saturating_duration_since(stamp) } - /// The stamp that THIS clock will read back as exactly `secs` old — the inverse of [`age_of`]. + /// A stamp `secs` behind this clock's own reading of now — the inverse of [`age_of`]. + /// + /// On a **pinned** clock the round trip is exact: `age_of(ago(n)) == n`. On the **wall** clock it + /// is not, and cannot be — `now` advances between minting and reading, so `age_of(ago(n)) >= n` + /// is the strongest true statement. Both are asserted in + /// `ago_is_the_inverse_of_age_of_on_the_same_clock_and_drifts_across_clocks`. /// /// Every past-dated net-health stamp in a test must come from here rather than from a bare /// `Instant::now() - secs`. On a wall clock the two are the same expression; on a PINNED clock @@ -421,8 +426,8 @@ impl HealthClock { /// bound has a margin that machine load eats. That is #760's own failure mode, re-armed one /// level down — it happened, in review, to `debug_reports_world_unresponsive_when_a_probe_goes_ /// unanswered_while_the_link_acks` (a 15s stamp that had to clear a 10s bound: a 5s margin). - /// Deriving the stamp from the clock that will read it makes the age exact for both kinds of - /// clock, so the correct call has no wrong variant to choose between. + /// Deriving the stamp from the clock that will read it removes that drift, so the correct call + /// has no wrong variant to choose between. /// /// [`age_of`]: HealthClock::age_of #[cfg(any(test, feature = "test-fixtures"))] @@ -2312,9 +2317,11 @@ mod health_clock_tests { "and an older stamp reads exactly its own age, with no wall-clock drift added"); } - /// `ago` is the exact inverse of `age_of` **on the same clock**, for a pinned clock and for the - /// wall clock alike — that is the whole reason a test never has to choose between two spellings. - /// The second half is the one that matters: a stamp taken from the WALL clock and read back + /// `ago` inverts `age_of` **on the same clock**: exactly, on a pinned clock; up to the elapsed + /// gap between the two calls, on the wall clock — where an exact round trip is impossible, which + /// is why the wall-clock arm below is an inequality and the pinned arm is not. Either way the + /// call is the right one, so a test never has to choose between two spellings. + /// The cross-clock half is the one that matters: a stamp taken from the WALL clock and read back /// against a PINNED one drifts by the gap between them, which is #760's failure mode one level /// down (review finding B1). Asserted here as a strict inequality, not a tolerance window. #[test] From 5d9166bf18c0637c45fb7c8475e12474bbed9d93 Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 10:33:43 -0400 Subject: [PATCH 6/6] fix(#760/C1,C2): make the guard's own scanner reach the code it grades, and stop it flagging the correct clock Review round 3 found the source-scanning guard added in round 3 was blind to 87% of its own corpus: `statements()` stripped block comments with a bare `find("/*")` BEFORE truncating at `//`, so a `/*` inside a line or doc comment latched the block state permanently. Route globs in `//!` docs on line 1 of two of the four scanned files had it armed already. Round 3's twelve-cell shape table was therefore self-graded -- every probe happened to sit in the one window that was still visible. C1: replace the line-at-a-time stripper with a character-level `strip_to_code` (line comments, nesting block comments, strings with escapes, raw strings with `#` counts, char-literal vs lifetime), and add two PERMANENT controls rather than ad-hoc probes: * `assert!(ended_clean, ...)` -- a real source file never ends inside a comment or a string, so a latched scanner now fails loudly instead of returning a short clean list. * `GUARD_REACH_SENTINEL_*` at the end of each scanned file, asserted visible in the stripped output. The test binds all four by value, so deleting one is a compile error rather than a silent weakening. C2: `now() -` / `.checked_sub` were matched by a bare `contains()` with no receiver test, so `c.now().checked_sub(d)` -- the CORRECT clock -- was flagged, as was the body of `HealthClock::ago` itself. Route `now()` through the same `receiver_is_the_reading_clock` test as `ago(`. Delete the falsified sentence "there is no case where the flagged form is the right one" rather than reword it; the docs now say a flagged line is not automatically a wrong line. Also record, in the guard's own doc so the next person inherits it, the rule that produced this finding: a probe of a source scanner must be placed at several depths in EVERY scanned file, with a positive control proving the shape is detectable and a reach control proving the scanner arrives. N1: narrow the `testkit.rs` sentence that read as exhaustive, and document the brace/`match` cut misses -- measured 0/12 both ways against a 12/12 positive control, not reasoned. N2: the widening justification named "three" rustfmt-wrapped sites in eqoxide-net. There are seven, none reachable by this guard, and all would be false positives if they were. Restated. N3: `empty_state_with_net_health` silently un-pins the health clock; say so. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-http/src/combat.rs | 10 ++ crates/eqoxide-http/src/lib.rs | 10 ++ crates/eqoxide-http/src/observe.rs | 280 ++++++++++++++++++++++------- crates/eqoxide-http/src/testkit.rs | 30 +++- 4 files changed, 257 insertions(+), 73 deletions(-) diff --git a/crates/eqoxide-http/src/combat.rs b/crates/eqoxide-http/src/combat.rs index 203328fa..33e15772 100644 --- a/crates/eqoxide-http/src/combat.rs +++ b/crates/eqoxide-http/src/combat.rs @@ -851,3 +851,13 @@ mod tests { "message should name the real cause, not the unrelated \"provide gem/spell_id\" default-validation text: {text}"); } } + +/// Reach control for `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it` (#760/C1). +/// +/// That guard scans this file's source text. A scanner that silently stops early — as it did, when +/// a `/*` inside a route glob in a doc comment latched its block-comment state — reports +/// a clean scan of a corpus it never read, which is a confident falsehood. The guard asserts it can +/// SEE this constant; because it is the last item in the file, seeing it proves the scan arrived at +/// the end. **Keep it last.** +#[cfg(test)] +pub(crate) const GUARD_REACH_SENTINEL_COMBAT: u8 = 0; diff --git a/crates/eqoxide-http/src/lib.rs b/crates/eqoxide-http/src/lib.rs index 78f6bd20..97575a45 100644 --- a/crates/eqoxide-http/src/lib.rs +++ b/crates/eqoxide-http/src/lib.rs @@ -1343,3 +1343,13 @@ mod health_serde_tests { "a live session must serialize session_drop as an explicit null (#642)"); } } + +/// Reach control for `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it` (#760/C1). +/// +/// That guard scans this file's source text. A scanner that silently stops early — as it did, when +/// a `/*` inside a route glob in a doc comment latched its block-comment state — reports +/// a clean scan of a corpus it never read, which is a confident falsehood. The guard asserts it can +/// SEE this constant; because it is the last item in the file, seeing it proves the scan arrived at +/// the end. **Keep it last.** +#[cfg(test)] +pub(crate) const GUARD_REACH_SENTINEL_LIB: u8 = 0; diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index a68a7bb3..c7c1012e 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -3426,34 +3426,81 @@ mod tests { /// `debug_reports_world_unresponsive_when_a_probe_goes_unanswered_while_the_link_acks`. /// /// A prose rule did not prevent it (the rule named only "an age that must move", not "an age - /// that must exceed a bound"), so this scans the source instead. `HealthClock::ago` is correct - /// for BOTH clocks — on a wall clock it is exactly `Instant::now() - N` — so there is no case - /// where the flagged form is the right one, which is why this is a blanket ban and not a - /// judgement call. Bare `Instant::now()` is deliberately NOT flagged: it dates a stamp to the - /// present, which saturates to age 0 against either clock. + /// that must exceed a bound"), so this scans the source instead. /// - /// # What this actually matches, and what it therefore cannot see + /// # What this matches /// - /// The unit of matching is a **comment-stripped statement**, not a physical line: the source is - /// stripped of `//` and `/* */` comments and then cut at `;`, `{` and `}`. A statement offends - /// when it mentions one of the eight [`STAMP_FIELDS`] *and* past-dates via a form that is not - /// clock-relative. This unit was chosen after review measured the physical-line version catching - /// **1 of 6** injected violation shapes; the misses included the shape **rustfmt itself writes** - /// (an assignment wrapped so the `=` and the `ago(` land on different lines), which already - /// occurs on `NetHealth` stamps in `eqoxide-net` today — benign there only because those - /// fixtures read on the wall clock. + /// The unit is a **statement**, not a physical line: comments and literals are removed by a + /// character scanner, and what is left is cut at `;`, `{` and `}`. A statement offends when it + /// mentions one of the eight [`STAMP_FIELDS`] *and* past-dates through a receiver that is not + /// the clock reading it back (see `receiver_is_the_reading_clock`). Flagged forms: `ago(`, and + /// `now()` followed by `-` or `.checked_sub`. /// - /// Past-dating forms flagged: any `ago(` whose receiver is not the reading clock (see - /// `receiver_is_the_reading_clock` — a bare `ago(` and `WALL.ago(` are both flagged; exempting - /// *any* `.ago(` regardless of receiver was review's sharpest miss), plus `now() -` / `now()-` - /// and `checked_sub`. Those last two are here because review's own sweep found past-dated sites - /// a bare `ago(` grep cannot see: **grep the concept, not the spelling.** + /// # Why the `now()` spelling is matched, stated honestly /// - /// Known blind spot, stated because it is real and not closable by a text scan: **aliasing - /// through a local.** `let s = ago(15); h.last_probe_sent = Some(s);` is two statements, neither - /// of which contains both halves, so it lands. Closing it needs dataflow, not text. Two lesser - /// ones: files outside the four scanned, and a ninth `Instant` field added to `NetHealth` and to - /// `health()` but not to [`STAMP_FIELDS`] — there is no cross-check against the struct. + /// It was added after a sweep for the CONCEPT rather than the field name found past-dated + /// `NetHealth` stamps a bare `ago(` grep does not see. That sweep is worth keeping as a + /// methodological result — **grep the concept, not the spelling** — but its sites do NOT + /// justify the widening on their own terms, and an earlier version of this comment implied they + /// did. Two corrections: + /// + /// 1. There are **seven**, not the three previously published — re-enumerated by sweeping + /// `eqoxide-net` for all eight [`STAMP_FIELDS`] against all three past-dating spellings: + /// `crates/eqoxide-net/src/gameplay.rs:1569, 1831, 1835, 1839` and + /// `crates/eqoxide-net/src/transport.rs:1885, 1914, 2679`. + /// 2. Not one of them is reachable by this test — they are all in `eqoxide-net`, which is not in + /// the scanned set below and cannot be (`include_str!` reaches only this crate's own files). + /// And if the scan were extended to them they would all be **false positives**: each site's + /// `NetHealth` is built by `Default` (checked at every one of the seven), so its stamps are + /// past-dated from the wall clock and read back by the wall clock — the same clock — which is + /// exactly the property this test exists to require. + /// + /// What the widening actually buys is confined to the four files below: the spelling is now + /// caught *there* if it ever appears, where today it does not. + /// + /// # How to probe this guard, and why it is written down + /// + /// A source scanner has TWO failure modes, and only one of them is obvious. It can fail to + /// recognise a bad *shape* — and it can silently fail to *arrive* at the code in the first + /// place. Round 3 of #760 had a twelve-cell shape table that was entirely void, because every + /// probe was placed inside this test's own body, which happened to be the only region of + /// `observe.rs` the scanner still reached: a `/*` inside a route glob in a doc comment on line 1 + /// latched the block-comment state and blinded **87% of the corpus**, and the probes were sitting + /// in the window that this test's own `/* */`-containing doc had re-opened. The instrument was + /// measured only where it worked. + /// + /// So: **any probe of this guard must be placed at several depths in EVERY scanned file** — near + /// the top, mid-file, and near the end — and must be reported with two controls: + /// 1. a **positive control**, proving the injected shape is detectable at all, and + /// 2. a **reach control**, proving the scanner actually arrives at that location. + /// + /// Both controls are now permanent rather than ad-hoc: `scan_ended_clean` fails loudly if a file + /// ends mid-comment or mid-string (a real source file never does), and every scanned file ends + /// with a `GUARD_REACH_SENTINEL_*` const that this test asserts it can see. + /// + /// # What it still cannot see + /// + /// **Aliasing through a local.** `let s = ago(15); h.last_probe_sent = Some(s);` is two + /// statements, neither carrying both halves, so it lands unflagged. Closing that needs dataflow, + /// not text, and no amount of scanner care will do it. + /// + /// **A value produced inside a nested block or a `match` arm.** The cut at `{` and `}` puts + /// `h.last_tick =` and the `ago(30)` in `h.last_tick = { ago(30) };` into different statements, + /// so neither carries both halves. Same for a `match` that yields the stamp. Measured, with the + /// controls this doc demands: both spellings were injected at three depths in all four scanned + /// files (twelve locations) alongside a plain `now() -` positive control in the same function. + /// All twelve controls were flagged — so the scanner reached every location and the shape is + /// detectable there — and not one of the twenty-four brace/`match` forms was. + /// + /// Two lesser gaps: files outside the four named below, and a ninth `Instant` field added to + /// `NetHealth` and to `health()` but not to [`STAMP_FIELDS`] — that list is hand-maintained with + /// no cross-check against the struct. + /// + /// The rule is a SPELLING rule and errs toward flagging: it recognises the receivers `c.` and + /// `….clock.`, so a *correct* stamp taken from a binding named anything else is flagged too, and + /// so is a correct `self.now().checked_sub(…)` — which is the body of `HealthClock::ago` itself. + /// That method lives in `eqoxide-ipc` and is not scanned, but the point stands: a flagged line is + /// not automatically a wrong line. #[test] fn no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it() { /// Exactly the `NetHealth` fields `HttpState::health()` turns into an age. Adding a stamp @@ -3464,32 +3511,92 @@ mod tests { "last_send_pressure_at", "last_send_error_at", ]; - /// Comment-stripped statements, each kept as its (1-based line, text) fragments so an - /// offender can still be reported at the exact line that names the field. - fn statements(src: &str) -> Vec> { - let mut out: Vec> = Vec::new(); - let mut cur: Vec<(usize, String)> = Vec::new(); - let mut in_block = false; - for (i, raw) in src.lines().enumerate() { - // Strip `/* … */` (possibly spanning lines), then a trailing `//`. - let (mut code, mut rest) = (String::new(), raw); - while !rest.is_empty() { - if in_block { - match rest.find("*/") { - Some(k) => { in_block = false; rest = &rest[k + 2..]; } - None => break, - } - } else { - match rest.find("/*") { - Some(k) => { code.push_str(&rest[..k]); in_block = true; rest = &rest[k + 2..]; } - None => { code.push_str(rest); break; } + /// Code text per source line, with comments and literal contents removed, plus whether the + /// scan **ended clean** — i.e. not stranded inside a block comment or a string. + /// + /// The line-at-a-time version this replaces looked for `/*` before it truncated at `//`, so + /// a `/*` occurring *inside* a line or doc comment latched the block state permanently. That + /// is not a corner case: two of the four scanned files open with a `//!` doc line containing + /// a route glob (`/v1/observe/*`), which blinded the scanner from line 1. Literals are + /// skipped for the same family of reason — a `//` or `/*` inside a string is not a comment. + fn strip_to_code(src: &str) -> (Vec, bool) { + #[derive(PartialEq, Clone, Copy)] + enum S { Code, Line, Block(u32), Str, Raw(usize), Chr } + let b: Vec = src.chars().collect(); + let at = |k: usize| -> char { b.get(k).copied().unwrap_or('\0') }; + let ident = |ch: char| ch.is_alphanumeric() || ch == '_'; + let mut out = vec![String::new()]; + let (mut st, mut i) = (S::Code, 0usize); + while i < b.len() { + let c = b[i]; + if c == '\n' { + if st == S::Line { st = S::Code; } + out.push(String::new()); + i += 1; + continue; + } + match st { + S::Line => i += 1, + S::Block(d) => { + if c == '*' && at(i + 1) == '/' { + st = if d == 1 { S::Code } else { S::Block(d - 1) }; + i += 2; + } else if c == '/' && at(i + 1) == '*' { + st = S::Block(d + 1); // Rust block comments nest. + i += 2; + } else { i += 1; } + } + S::Str => { + if c == '\\' { i += 2; } else { if c == '"' { st = S::Code; } i += 1; } + } + S::Raw(h) => { + if c == '"' && (1..=h).all(|k| at(i + k) == '#') { st = S::Code; i += h + 1; } + else { i += 1; } + } + S::Chr => { + if c == '\\' { i += 2; } else { if c == '\'' { st = S::Code; } i += 1; } + } + S::Code => { + let prev_is_ident = i > 0 && ident(b[i - 1]); + // NB the branch ORDER here is not what fixes C1 — `//` and `/*` differ in + // their second character, so they can never both match. The fix is that + // `S::Line` skips to end-of-line, so a `/*` sitting inside a line or doc + // comment is never read as an opener at all. The version this replaced + // stripped block comments in a whole-line pass BEFORE truncating at `//`, + // which is exactly how a route glob in a `//!` line latched it forever. + if c == '/' && at(i + 1) == '/' { st = S::Line; i += 2; } + else if c == '/' && at(i + 1) == '*' { st = S::Block(1); i += 2; } + else if c == '"' { st = S::Str; i += 1; } + else if (c == 'r' || (c == 'b' && at(i + 1) == 'r')) && !prev_is_ident && { + let j = i + if c == 'b' { 2 } else { 1 }; + let mut h = 0; + while at(j + h) == '#' { h += 1; } + at(j + h) == '"' + } { + let j = i + if c == 'b' { 2 } else { 1 }; + let mut h = 0; + while at(j + h) == '#' { h += 1; } + st = S::Raw(h); + i = j + h + 1; } + // `'` is a char literal only if it closes; otherwise it is a lifetime. + else if c == '\'' && (at(i + 1) == '\\' || at(i + 2) == '\'') { st = S::Chr; i += 1; } + else { out.last_mut().unwrap().push(c); i += 1; } } } - if let Some(k) = code.find("//") { code.truncate(k); } - // Cut at statement and block boundaries so unrelated code never merges into one - // statement (which would invent co-occurrences and fire falsely). - for (n, piece) in code.split([';', '{', '}']).enumerate() { + } + (out, matches!(st, S::Code | S::Line)) + } + + /// Statements, each kept as its (1-based line, text) fragments so an offender is still + /// reported at the exact line that names the field. + fn statements(code: &[String]) -> Vec> { + let mut out: Vec> = Vec::new(); + let mut cur: Vec<(usize, String)> = Vec::new(); + for (i, line) in code.iter().enumerate() { + // Cut at statement AND block boundaries, so unrelated code never merges into one + // "statement" and invents a co-occurrence that fires falsely. + for (n, piece) in line.split([';', '{', '}']).enumerate() { if n > 0 && !cur.is_empty() { out.push(std::mem::take(&mut cur)); } if !piece.trim().is_empty() { cur.push((i + 1, piece.to_string())); } } @@ -3498,45 +3605,70 @@ mod tests { out } - /// Is this `ago(` call's RECEIVER the health clock that will read the stamp back? + /// Is this call's RECEIVER the health clock that will read the stamp back? /// - /// Exempt spellings are `c.ago(` (the convention every call site uses, `let c = h.clock;`) - /// and `….clock.ago(`. Everything else is flagged — a free `ago(`, and equally - /// `HealthClock::WALL.ago(` or any other receiver. The old rule exempted *any* `.ago(`, - /// which review showed lets `wall.ago(30)` into a pinned fixture: that is #760 exactly. - /// - /// This is a SPELLING rule, so it is deliberately conservative — a correct stamp taken from - /// a binding named something other than `c` is flagged too. The fix is to rename it to `c`. + /// Exempt spellings are `c.` (the convention every call site uses, `let c = h.clock;`) and + /// `….clock.`. Everything else is flagged — a free `ago(`, and equally `HealthClock::WALL` + /// or any other receiver. Exempting *any* `.ago(` regardless of receiver was review's + /// sharpest miss: `wall.ago(30)` stamped into a pinned fixture is #760 exactly. fn receiver_is_the_reading_clock(prefix: &str) -> bool { if prefix.ends_with(".clock.") { return true; } let Some(rest) = prefix.strip_suffix("c.") else { return false }; !rest.chars().next_back().is_some_and(|ch| ch.is_alphanumeric() || ch == '_') } + // Naming the sentinels as VALUES, not just as strings to grep for, is what makes deleting + // one a compile error rather than a silently weakened reach control. + let _sentinels_must_exist = ( + super::GUARD_REACH_SENTINEL_OBSERVE, + crate::GUARD_REACH_SENTINEL_LIB, + crate::combat::GUARD_REACH_SENTINEL_COMBAT, + crate::testkit::GUARD_REACH_SENTINEL_TESTKIT, + ); let sources = [ - ("observe.rs", include_str!("observe.rs")), - ("lib.rs", include_str!("lib.rs")), - ("combat.rs", include_str!("combat.rs")), - ("testkit.rs", include_str!("testkit.rs")), + ("observe.rs", include_str!("observe.rs"), "GUARD_REACH_SENTINEL_OBSERVE"), + ("lib.rs", include_str!("lib.rs"), "GUARD_REACH_SENTINEL_LIB"), + ("combat.rs", include_str!("combat.rs"), "GUARD_REACH_SENTINEL_COMBAT"), + ("testkit.rs", include_str!("testkit.rs"), "GUARD_REACH_SENTINEL_TESTKIT"), ]; let mut offenders = Vec::new(); - for (file, src) in sources { - for stmt in statements(src) { + for (file, src, sentinel) in sources { + let (code, ended_clean) = strip_to_code(src); + // REACH CONTROL 1 — loud failure instead of a silent early stop. A real source file + // never ends inside a block comment or a string, so this cannot false-positive. + assert!(ended_clean, + "{file}: the scanner ended stranded inside a comment or string literal, so it stopped \ + seeing code partway through and every 'clean' result below is worthless (#760/C1)"); + let stmts = statements(&code); + // REACH CONTROL 2 — the sentinel is the LAST item in each scanned file, so seeing it + // proves the scan arrived at the end rather than merely ending without complaint. + assert!(stmts.iter().flatten().any(|(_, t)| t.contains(sentinel)), + "{file}: the scanner never reached `{sentinel}` at the end of the file, so it covered \ + only a prefix of it — a clean scan of a corpus that was never read (#760/C1)"); + for stmt in stmts { let joined: String = stmt.iter().map(|(_, t)| t.as_str()).collect(); let Some((line, _)) = stmt.iter().find(|(_, t)| STAMP_FIELDS.iter().any(|f| t.contains(f))) else { continue }; - let past_dated = joined.match_indices("ago(") - .any(|(k, _)| !receiver_is_the_reading_clock(&joined[..k])) - || joined.contains("now() -") - || joined.contains("now()-") - || joined.contains("checked_sub"); - if !past_dated { continue; } + let bad_ago = joined.match_indices("ago(") + .any(|(k, _)| !receiver_is_the_reading_clock(&joined[..k])); + // `now()` is fine on its own — it dates a stamp to the present, which saturates to + // age 0 against either clock. It is past-dating, and so a #760 shape, only when it + // is followed by a subtraction. The receiver test applies here too: `c.now() - d` is + // the correct clock and must NOT be flagged (review finding C2). + let bad_now = joined.match_indices("now()").any(|(k, _)| { + if receiver_is_the_reading_clock(&joined[..k]) { return false; } + let tail = joined[k + "now()".len()..].trim_start(); + tail.starts_with('-') || tail.starts_with(".checked_sub") + }); + if !(bad_ago || bad_now) { continue; } offenders.push(format!("{file}:{line}: {}", joined.trim())); } } assert!(offenders.is_empty(), - "these past-date a net-health stamp from a clock other than the one `health()` will read \ - it back against — use `let c = h.clock; … = c.ago(N)` instead (#760):\n{}", + "these past-date a net-health stamp through a receiver that is not the clock `health()` \ + reads it back against — use `let c = h.clock; … = c.ago(N)` (#760). A flagged line is \ + not automatically wrong: the receiver test is a spelling rule, so a correct stamp under \ + a binding named other than `c` lands here too.\n{}", offenders.join("\n")); } @@ -4910,3 +5042,13 @@ mod zone_cross_observables_713 { "and it must say what the caller actually risks"); } } + +/// Reach control for `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it` (#760/C1). +/// +/// That guard scans this file's source text. A scanner that silently stops early — as it did, when +/// a `/*` inside a route glob in a doc comment latched its block-comment state on line 1 — reports +/// a clean scan of a corpus it never read, which is a confident falsehood. The guard asserts it can +/// SEE this constant; because it is the last item in the file, seeing it proves the scan arrived at +/// the end. **Keep it last.** +#[cfg(test)] +pub(crate) const GUARD_REACH_SENTINEL_OBSERVE: u8 = 0; diff --git a/crates/eqoxide-http/src/testkit.rs b/crates/eqoxide-http/src/testkit.rs index 474b58fe..d10af93a 100644 --- a/crates/eqoxide-http/src/testkit.rs +++ b/crates/eqoxide-http/src/testkit.rs @@ -22,10 +22,12 @@ use crate::HttpState; /// `NetHealth::clock`, which a fixture pins, so a wall-clock stamp read back against it drifts by /// however long the test took (#760). Use `h.clock.ago(secs)` there. That ban is **partially** /// guarded by `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_ -/// one_that_reads_it` — a source scan over four files that reads one statement at a time, so it does -/// catch the rustfmt-wrapped and struct-literal spellings but cannot see a stamp aliased through a -/// local. Read that test's doc for the measured blind spots before relying on it: a wrong spelling -/// it misses still compiles and still lands. +/// one_that_reads_it` — a source scan over four files, one statement at a time. It catches the +/// rustfmt-wrapped and struct-literal spellings. It does **not** catch a stamp aliased through a +/// local, nor one whose value is produced inside a nested block or a `match` arm (the scan cuts +/// statements at `{` and `}`, which separates the field from the call), nor anything in a file it +/// does not scan. Read that test's doc for the full measured list before relying on it: a wrong +/// spelling it misses still compiles and still lands. pub fn ago(secs: u64) -> std::time::Instant { std::time::Instant::now() .checked_sub(std::time::Duration::from_secs(secs)) @@ -82,6 +84,16 @@ pub fn empty_state_wall_clock() -> HttpState { /// `EqStream` into a REAL send failure and then asserts the failure is visible in THIS crate's /// `/v1/observe/debug` JSON, i.e. that the fact reaches something the agent can poll rather than /// merely being published into an internal struct. +/// +/// **This un-pins the health clock (#760/N3).** [`empty_state`] hands out a `NetHealth` frozen at +/// construction so a handler test cannot race `SESSION_STALE_TICK_MS`; the caller-owned `Arc` +/// replaces that wholesale, and callers build theirs with `NetHealth::default()`, i.e. the WALL +/// clock. That is correct for #612's purpose — the subject is a real `EqStream` stamping real +/// times — but it means a stamp written here is aged against the wall clock, so the two shapes in +/// [`empty_state_wall_clock`]'s doc apply to this function too, and the `observe` guard does not +/// scan the caller's crate. `crates/eqoxide-net/src/transport.rs:2679` past-dates +/// `last_send_pressure_at` through this path today; it is correct precisely because the clock is +/// the wall clock, which is the fact this paragraph exists to keep true. pub fn empty_state_with_net_health(net_health: eqoxide_ipc::NetHealthShared) -> HttpState { HttpState { net_health, ..empty_state() } } @@ -207,3 +219,13 @@ pub async fn observe_json(state: HttpState, path: &str) -> serde_json::Value { let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX).await.unwrap(); serde_json::from_slice(&bytes).unwrap() } + +/// Reach control for `observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it` (#760/C1). +/// +/// That guard scans this file's source text. A scanner that silently stops early — as it did, when +/// a `/*` inside a route glob in a doc comment latched its block-comment state — reports +/// a clean scan of a corpus it never read, which is a confident falsehood. The guard asserts it can +/// SEE this constant; because it is the last item in the file, seeing it proves the scan arrived at +/// the end. **Keep it last.** +#[cfg(test)] +pub(crate) const GUARD_REACH_SENTINEL_TESTKIT: u8 = 0;