Skip to content

fix(#760): pin the health clock in test fixtures so a handler's HTTP status cannot depend on machine load - #778

Open
djhenry wants to merge 8 commits into
mainfrom
fix-760-session-clock
Open

fix(#760): pin the health clock in test fixtures so a handler's HTTP status cannot depend on machine load#778
djhenry wants to merge 8 commits into
mainfrom
fix-760-session-clock

Conversation

@djhenry

@djhenry djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Closes #760.

Round 4. Review found two blocking defects (C1, C2). Both were real, and unlike round 3 both
were code defects in the guard itself, not prose. C1 is the serious one: the guard's source
scanner silently stopped reading 87% of its own corpus, so round 3's twelve-cell table was
measuring an instrument that was not there
. That table is void and has been struck below and
re-measured from scratch, with reach controls. See Round 4.

The mechanism, re-derived (not taken on trust)

The issue's attribution is correct, and I confirmed it before changing anything.

A throwaway probe test slept 5.1s between testkit::empty_state() and the exact POST /cast {"gem":7} request that combat::tests::cast_empty_gem_is_409_and_queues_nothing issues, and printed the response:

PROBE760 status=503 Service Unavailable
body=the game session is not live — the network thread has not ticked in 5100ms ...

post_cast calls require_live_session as its first statement (crates/eqoxide-http/src/combat.rs:214), that guard trips on h.snapshot_age_ms >= SESSION_STALE_TICK_MS (5_000, crates/eqoxide-http/src/lib.rs:108), and snapshot_age_ms was last_tick.elapsed() — wall time between the fixture's construction and the handler being served. On a loaded box that exceeds 5s and the handler answers 503 before it ever looks at the gem. The probe test has been deleted; it existed only to establish the mechanism.

Design chosen: inject the clock, as a type

The issue preferred an injectable clock over raising the threshold, and I agree — a bigger number moves the flake, it does not remove it. Implemented as a newtype rather than a flag:

pub struct HealthClock(Option<std::time::Instant>);   // field is PRIVATE
impl HealthClock {
    pub const WALL: HealthClock = HealthClock(None);  // production, and Default
    #[cfg(any(test, feature = "test-fixtures"))]
    pub fn frozen_at(t: Instant) -> Self { ... }
    pub fn now(self) -> Instant { self.0.unwrap_or_else(Instant::now) }
    pub fn age_of(self, stamp: Instant) -> Duration { self.now().saturating_duration_since(stamp) }
    #[cfg(any(test, feature = "test-fixtures"))]
    pub fn ago(self, secs: u64) -> Instant { ... }    // added in round 2 — the inverse of age_of
}

NetHealth gains a clock: HealthClock field (WALL in Default), and every age in HttpState::health() is now h.clock.age_of(stamp) instead of stamp.elapsed().

Why a type with a private field and a gated constructor: a frozen health clock is itself an honesty violation if it can ever reach production — it would pin snapshot_age_ms at 0 and report a dead net thread as live forever, which is the #343 lie this projection exists to prevent. With the field private and frozen_at behind #[cfg(any(test, feature = "test-fixtures"))], no other crate can represent a frozen clock. (Precision added in round 2: the field is private to eqoxide-ipc, so this is a crate boundary, not a cfg boundary — production code inside that crate's own src/ could still construct one. The guarantee is against every other crate, and it is now measured — see Confirmed by review.) Production behaviour is unchanged: HealthClock::WALL.now() is Instant::now().

require_live_session itself is untouched — the bound is still 5s, it just now reads an age a test can pin.

Round-2 fix: B1 — the same failure mode, one level down

Review caught that observe::tests::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 5-second margin machine load eats, in the same direction as the bug this PR exists to remove. Before this PR the age grew (15s + elapsed), so it was unconditionally safe. The PR made a safe test fragile. That is a real regression and the review was right to block on it.

I re-derived the sweep rather than accepting it. Method: this change only altered how HttpState::health() computes ages, so the affected surface is exactly tests on a frozen fixture that past-date one of the eight NetHealth stamp fields the projection ages. I enumerated all 24 ago() call sites in the workspace plus every other past-dating form (checked_sub, Instant::now() - …) in eqoxide-http. Nine are net-health writes, all in observe.rs; seven were already migrated to empty_state_wall_clock(); two remained on the frozen fixture. Of those two, debug_defaults_world_responsive_true_before_the_first_probe stamps ago(20) under a 40s bound — a shrinking age stays on the safe side, so it was harmless. The other is B1. The five remaining ago() sites in observe.rs feed asset_sync's tick_stamped, which is aged by its own publisher and never touches HealthClock. Same conclusion as the reviewer, reached independently: exactly one fragile site.

Fixed structurally, not by moving that one test back to the wall clock. HealthClock::ago is the inverse of age_of, so a stamp is always derived from the clock that will read it. All nine net-health stamp sites now use it (let c = h.clock; h.last_probe_sent = Some(c.ago(15));). The seven wall-clock tests keep empty_state_wall_clock(), because their subject really is that an age moves.

Round-4 correction. This paragraph used to end "so the banned form has no case where it is the
right one."
That sentence is false and has been deleted from the tracked docs too — review
falsified it with h.last_datagram = c.now().checked_sub(d).unwrap(), which is the banned shape
taken from the correct clock, and with the body of HealthClock::ago itself
(crates/eqoxide-ipc/src/lib.rs:436-440), which the guard's own rule would ban. The guard errs
toward flagging on purpose; a flagged line is not automatically a wrong line, and the docs now say
exactly that instead.

The rule is now executable, not prose. The old rule named only "an age that must move" — which is exactly why the threshold shape slipped through. Rather than widen the sentence and hope, observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it scans observe.rs/lib.rs/combat.rs/testkit.rs and fails when a statement both names a net-health stamp field and past-dates it from a clock other than the one that reads it back.

B1 mutation check, both directions, measured (5.1s sleep injected immediately after empty_state() in that test, nothing else changed — the reviewer's probe):

arm result
fix in place (c.ago(15)) test result: ok. 1 passed; 0 failed; … finished in 5.11s (exit 0)
B1 fix reverted (ago(15)) test result: FAILED. 0 passed; 1 failed; … finished in 5.22s (exit 101) — left: Bool(true) right: Bool(false), the reviewer's exact failure

Verification tier

Tier 1 for the fixture. testkit::empty_state() builds NetHealth::frozen_at(Instant::now(), 0, 0, 0) — clock pinned to the same instant as the stamps — so its projected ages are structurally 0, not merely small. The test cannot observe wall time at all.

For B1's defect class: a guard, and I will not oversell it as tier 1. The wrong spelling is not merely discouraged — it fails a test that reads the source, in hundredths of a second, without needing the race to occur. But it is not literally unrepresentable: ago() still exists for the asset-sync stamps that legitimately need it. Making it impossible would mean minting net-health stamps through a distinct type only HealthClock can produce; that is a larger change than this PR should carry, and I did not do it.


Round 4 — the instrument was blind, and the table that graded it was self-graded

C1 (blocking) — the scanner stopped reading, and reported clean

Review found the guard's statements() stripped block comments with a bare find("/*") before truncating at //, and had no string-literal awareness. So a /* occurring inside a line or doc comment latched the block-comment state permanently. This was not hypothetical: it was already armed on the round-3 head by route globs in //! docs at observe.rs:1, combat.rs:1 and by lib.rs:661.

Reviewer's measured coverage on head eeaa41dtheir measurement, attributed, not mine:

file lines the scanner saw lines in file
observe.rs 44 4913
lib.rs 660 1346
combat.rs 0 854
testkit.rs 210 210
total 914 7323 (≈ 12.5%)

Round 3's twelve-cell table is therefore VOID. Every probe in it sat at observe.rs:3465–3483 — inside the only window observe.rs still had, opened by this test's own doc comment and closed by its own match rest.find("/*"). The instrument was measured exactly where it happened to work. That table has been replaced below by one re-measured from scratch; it is recorded as void in Corrections rather than quietly swapped out, because the failure mode is the point.

Fixed structurally, not by patching the ordering. statements() is fed by a new character-level strip_to_code with an explicit state machine: line comments, nesting block comments, string literals with escapes, raw strings with # counts, and char-literals disambiguated from lifetimes. Literal contents are dropped, so a // inside a string can no longer truncate a line either.

I also recorded a correction to my own first draft of that code comment, which claimed "// is tested BEFORE /*: that ordering is the whole C1 fix." With a character-level scanner the branch order is irrelevant — // and /* differ in their second character and can never both match. The actual fix is that the Line state skips to end-of-line, so a /* inside a comment is never read as an opener at all. The comment now says that.

And the bad state is now made loud rather than merely fixed — two permanent controls, not ad-hoc probes:

  1. assert!(ended_clean, …)strip_to_code returns whether it finished in Code/Line state. A real source file never ends inside a comment or a string, so a latched scanner now fails the test instead of returning a clean list.
  2. A reach sentinel per file. Each of the four scanned files ends with pub(crate) const GUARD_REACH_SENTINEL_<FILE>: u8 = 0;, and the guard asserts it can see that text in the stripped output of each file. Deleting a sentinel is a compile error, not a silent weakening: the test binds all four by value in a _sentinels_must_exist tuple.

This is not tier 1 — a scanner that reaches every line is still a scanner — but the specific bad state ("the instrument silently stopped early") is no longer representable without a red test.

The assert!(ended_clean) mutation check, both directions

To mutate I copied each file aside, edited, measured, restored from the copy, and md5sum-verified. No git checkout/git restore/git stash was used at any point.

arm result
current head, controls in place, clean tree test result: ok. 1 passed; 0 failed … finished in **0.06s** (EXIT=0)
legacy line-based stripper restored, controls kept REDpanicked at crates/eqoxide-http/src/observe.rs:3563:13: observe.rs: 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)test result: FAILED. 0 passed; 1 failed

The RED arm is the important one: it proves the corpus latches today, so the control is live rather than vacuous. The 0.06s vs the round-3 guard's 0.01s is itself consistent with actually reading ~7,300 lines instead of 914.

Run E — what the old scanner did silently, demonstrated

Legacy stripper restored and both controls deleted, with 12 identical violations injected at three depths in all four files:

testkit.rs:35:  h.last_packet = std::time::Instant::now() - std::time::Duration::from_secs(30)
testkit.rs:109: h.last_packet = std::time::Instant::now() - std::time::Duration::from_secs(30)
testkit.rs:139: h.last_packet = std::time::Instant::now() - std::time::Duration::from_secs(30)

3 of 12 reported. 9 invisible, and the test would have said ok had the three in testkit.rs not existed. That is exactly the reviewer's finding, reproduced independently.

Re-measured, with a reach control at every cell

Round 3's cells were placed by hand inside this test's body. Round 4 injects them programmatically at three depths in each of the four scanned files — near the top, mid-file, and immediately before the sentinel — and every shape is reported with its file and line, so the line number is the reach evidence.

Reach control (run A), 12/12 — a canonical violation planted at each depth, all flagged:

file depth 1 depth 2 depth 3
observe.rs :1864 :3731 :4801
lib.rs :1071 :1210 :1308
combat.rs :362 :589 :856
testkit.rs :35 :109 :139

Under the round-3 scanner, all three non-testkit rows would have been 0/3 — not reasoned, measured directly in run E below.

Shape table, re-run from scratch. Every cell is now backed by the reach control above (the deep shapes sit at observe.rs:4841–4852, in the region the old scanner could not see at all):

# shape violating form (run A) corrected form (run C)
P1 canonical inline h.last_packet = ago(30); RED observe.rs:4841 GREEN
P2 rustfmt-wrapped assignment RED :4842 GREEN
P3 aliased via a local not caught — documented blind spot GREEN
P4 h.last_tick = wall.ago(30) RED :4847 GREEN
P5 struct-literal NetHealth { last_datagram: ago(30), .. } RED :4848 GREEN
P6 earlier = on the same line RED :4849 GREEN
P7 true-negative control Some(c.ago(3)) correctly not flagged not flagged
P8 Instant::now() - Duration::from_secs(60) RED :4851 GREEN
P9 Instant::now().checked_sub(…) (wrapped) RED :4852 GREEN
P10 C2 control c.now().checked_sub(…) — banned shape, correct clock correctly not flagged not flagged

Run A: test result: FAILED. 0 passed; 1 failed (exit 101), 19 offenders — the 12 reach probes plus the 7 deep shapes, and nothing else.
Run C (all forms corrected, at every depth): test result: ok. 1 passed; 0 failed … finished in 0.04s (exit 0), zero offenders.

C2 (blocking) — the matcher flagged the correct clock

now() - and checked_sub were matched by a bare contains() with no receiver test, so h.last_datagram = c.now().checked_sub(d).unwrap() — the correct clock — was flagged, and so was the body of HealthClock::ago itself. Fixed: now() occurrences now go through the same receiver_is_the_reading_clock test as ago(, and only then check whether the following token is - or .checked_sub. Cell P10 above is the regression test, and it is placed at the deep end of observe.rs so it is covered by the reach control.

The falsified sentence — "there is no case where the flagged form is the right one" — is deleted, not reworded, in both the PR body (see the round-4 correction block above) and crates/eqoxide-ipc/src/lib.rs. The guard's doc now states the opposite explicitly and names self.now().checked_sub(…) as a correct-but-flagged form.

N1 — the brace-cut misses, measured rather than asserted

The testkit.rs sentence that read as exhaustive has been narrowed, and the two gaps behind it were measured with the controls this PR now demands. Both spellings were injected at all twelve locations alongside a plain positive control in the same function:

injected at 12 locations flagged
h.last_packet = Instant::now() - …from_secs(32); (positive control) 12 / 12
h.last_tick = { Instant::now() - …from_secs(30) }; 0 / 12
h.last_datagram = match 1 { _ => Instant::now() - …from_secs(31) }; 0 / 12

The controls firing at every location is what makes the two zeros mean "missed" rather than "not reached". Cause: the statement cut at {/} separates the field from the call. Now documented as a limitation in the guard's own doc.

N2 — the widening justification, restated honestly

Round 3 justified widening the matcher to now() -/checked_sub by pointing at "three" rustfmt-wrapped NetHealth stamp sites in eqoxide-net. Two corrections:

  1. There are seven, not three. 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. My published "three" was wrong.
  2. None of them justifies the widening on its own terms. They are all in eqoxide-net, which this guard cannot scan and never will (include_str! reaches only eqoxide-http's own files) — and if it could, all seven would be false positives: every one builds its NetHealth via Default (checked at each of the seven), so it past-dates from the wall clock and reads back on the wall clock, which is precisely the property the guard requires.

What survives is the methodological result — grep the concept, not the field name — and a real, if smaller, benefit: the spelling is now caught inside the four scanned files, where today it does not occur. The guard's doc says this in those words.

N3 — empty_state_with_net_health un-pins the clock, and now says so

That helper hands the caller's Arc straight through, and callers build theirs with NetHealth::default() — i.e. the wall clock — so it silently opts out of this PR's tier-1 fixture guarantee. It was undocumented. It now carries a paragraph saying so, and naming crates/eqoxide-net/src/transport.rs:2679 as a live caller that past-dates through this path and is correct precisely because the clock is the wall clock.

#755 clearance, re-derived under a scanner that reaches

The previous green does not carry — it was produced by the blind instrument. Re-derived: #755's three ago(300) sites are at observe.rs:2109, :2364 and :2500, all of which are inside the region the old scanner could not see and all of which the new scanner now reads (proven by the observe.rs reach column above bracketing them at :1864 and :3731). None is an offender, and the reason is structural rather than incidental: they stamp tick_stamped/asset_sync_slot, so they name no STAMP_FIELDS entry at all and are outside the guard's subject matter. Clean -p eqoxide-http on the final tree: 259 passed, 0 failed.


What was measured

  • Mechanism probe: the 5.1s repro above → deterministic 503 with the issue's exact message.
  • Blast-radius enumeration (mutated require_live_session to always 503): 106 of 248 eqoxide-http lib tests failed, spanning combat, group, interact, inventory, merchant, move_api, pet, quests, trainer, plus 8 live_session_guard_tests. Notably no chat, guild or lifecycle tests appeared despite those modules calling the guard.
    NOT exhaustive: cargo stopped at the first failing binary, so crates ordered after eqoxide-http never ran under the mutation. The app-crate integration binaries tests/http_observe_apply.rs and tests/net_thread_death_reaches_http.rs did run and passed, so they do not depend on the guard.
  • M1 — revert empty_state() to NetHealth::default(): FAILED. 245 passed; 3 failed, the end-to-end test failing left: 503 right: 409 with the issue's body text verbatim.
  • M2 — revert snapshot_age_ms to h.last_tick.elapsed(): FAILED. 245 passed; 3 failed.
    The identical totals are a coincidence and mean nothing on their own — the two mutations fail different sets. M1 trips handler_fixture_pins_its_health_clock; M2 trips health_ages_are_measured_against_the_injected_clock; two tests are shared. The two halves of the fix are therefore independently pinned, and it is the differing sets — not the equal counts — that establish it.
  • B1 arms, the ended_clean mutation, runs A/C/E and the N1 brace table, above.
  • A self-deadlock I introduced and then caught. The end-to-end test originally read the fixture's clock with two net_health.lock() calls inside one assert_eq!. std::sync::Mutex is not reentrant and the left-hand temporary guard lives to the end of the statement, so on the passing path it hung the whole eqoxide-http test binary. Fixed by taking the lock once into a tuple.

Corrections to earlier claims in this body

  1. Retracted: "the wedged runs reported exit code 0." I checked the recorded exit status of both hung runs: both exited 255, not 0. 255 is the transport's status after I killed them, so the exit code says nothing about the hang either way — I should not have written the 0.
  2. Corrected: how a lost binary presents. The running N header survives; only the test result: summary is lost. So header-count vs summary-count does detect it — the two are not lost together, as I had implied. Also, Finished was present on a run that never completed: it attests the build, not the tests.
  3. VOID (round 4): the entire round-3 twelve-cell shape table, and the "1 of 6 → 6 of 6" improvement claim built on it. Every cell was placed inside the only region the scanner could still reach. Re-measured above with reach controls.
  4. Retracted (round 4): "the banned form has no case where it is the right one." Falsified by review; deleted from the docs rather than reworded.
  5. Corrected (round 4): "three" rustfmt-wrapped sites in eqoxide-net is wrong — there are seven, and none of them is reachable by this guard. See N2.
  6. Corrected (round 4): my own first draft of the C1 fix comment claimed the //-before-/* branch ordering was the fix. It is not; branch order is irrelevant in a character-level scanner. Rewritten before commit.

Confirmed by review — recorded as the reviewer's measurement, not mine

The disclosed gap "I did not audit downstream consumers for accidental non-dev enablement of eqoxide-ipc/test-fixtures" was closed by the reviewer, who measured it in both directions: with a #[cfg(feature = "test-fixtures")] compile_error! planted in eqoxide-ipc, cargo build --release --locked --bin eqoxide finished clean in 22m 17s with the probe unfired, while cargo test --no-run -p eqoxide-http with the same probe errored as expected (proving the probe live, not a no-op). Every features = ["test-fixtures"] entry workspace-wide is under [dev-dependencies]; cargo tree --edges no-dev shows zero test-fixtures edges and the with-dev tree shows six. This is the reviewer's measurement, attributed, not my own. The C1 coverage table is likewise the reviewer's measurement.

What was NOT verified

  • The original flake was never observed failing in the wild by me. I reproduced the mechanism deterministically with an injected 5.1s sleep; I make no claim about its natural frequency.
  • No live client run — a pure test-harness/projection change with no runtime behaviour delta.
  • The blast-radius enumeration is explicitly partial (see above).
  • The guard's reach is bounded in five measured ways, all named in its own doc: (a) a stamp aliased through a local escapes it; (b) a stamp produced inside a nested block or a match arm escapes it — measured in N1, 0/12 both ways; (c) it binds only the four files it names, so an offending stamp in another file is invisible — and eqoxide-net, where the motivating shapes actually live, is one of those files it can never reach; (d) STAMP_FIELDS is hand-maintained with no cross-check against the struct, so a ninth Instant field would be unguarded; (e) it is a spelling rule for receivers, so a correct stamp under a binding named other than c is a false positive. It is a partial guard and the tracked docs say so in those words.
  • The scanner's own correctness is asserted, not proved. strip_to_code is a hand-written lexer, not rustc's. The two controls catch the failure mode that actually bit (silently stopping early) and would catch any unbalanced-state bug, but a lexer that mis-classifies a construct without ending stranded would still pass. I did not fuzz it.
  • I did not verify that the guard has no further misses beyond the ten shapes I measured. Absence of more is not something I established.

Branch is on current main

Brought up to 644ba2c by merge, not rebase — force-pushing is blocked in this environment, and rewriting a branch that already carries posted reviews would invalidate their line references. All merges were conflict-free.

main has since advanced to a48760b. git merge-tree of this head against a48760b is conflict-free, so I did not merge again — doing so would have invalidated the workspace figures below for no behavioural gain. Carry that as a caveat on the figures: they were measured on this branch merged with 644ba2c, not with a48760b.

The five figures — my own cargo test --workspace --locked --no-fail-fast on the round-4 head

  1. Finished — exactly one such line: Finished `test` profile [unoptimized + debuginfo] target(s) in 25m 08s. (It attests the build only — see Corrections Neriak Third Gate (neriakc) navmesh pathing stall #2.)
  2. test result: lines — 50, matching 50 headers (Running <binary> + Doc-tests). All 50 say ok; zero say FAILED; zero error lines; zero panicked at.
  3. Non-canonical (corrupt): 0. All-zero (empty targets): 15 — canonical, just empty.
  4. Arithmetic: 1673 passed + 0 failed + 45 ignored + 0 measured = 1718.
  5. Cross-check: the 50 headers sum to 1718 ✅. EXIT=0 (recorded, not relied upon).

Unchanged from round 3, as expected: round 4 added no tests — it rewrote one test's body and appended four const items.

A measurement error I made and caught while computing these: my first pass reported 16 all-zero summaries. grep "0 passed; 0 failed; …" matches inside 10 passed; 0 failed; …. The correct figure is 15. Recorded because it is the same defect class as everything else in this round: an instrument that quietly measures something other than what you asked it.

Two runs launched after the workspace job cover the tree the workspace run did not:

  • the N1 brace probe run (above), and
  • a clean -p eqoxide-http --locked --no-fail-fast on the final tree, including the N1/N2/N3 doc edits: Finished in 3m 21s, test result: ok. 259 passed; 0 failed, EXIT=0.

The only changes made after the workspace run were doc comments; the scanner strips comments, so they cannot alter its result — but rather than rest on that reasoning, the final tree was re-run.

eqoxide-http lib 259; eqoxide-ipc lib 59.

Reconciliation: main at 644ba2c measures 1663 (measured by #755's reviewer on that tree, not by me). This branch adds 10 test attributes — 5 in eqoxide-http (combat.rs +2, lib.rs +2, observe.rs +1) and 5 in eqoxide-ipc — so 1663 + 10 = 1673. Nothing dropped or duplicated.

One more defect the suite caught, worth recording because it is this PR's own defect class. My first draft of the ago/age_of inverse test asserted age_of(ago(0)) == ZERO on the wall clock. It failed: left: 100ns, right: 0nsnow advances between the two calls. A wall-clock-dependent assertion, written by the author of a PR about wall-clock-dependent assertions, caught by the workspace run rather than by re-reading. Restated as the inequality that is actually true (>= 15s; a monotonic clock cannot make a stamp younger than it was minted), which needs no tolerance window.

…status cannot depend on machine load

`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<Instant>` 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Independent acceptance review — REQUEST CHANGES

Reviewed at head b404926, in my own detached worktree. Everything below is measured on that tree
unless it says otherwise. I attacked five things: the release-feature-leak universal, the eighth-caller
sharp edge, M1/M2, the deadlock methodology claim, and merge order. Four survived. One did not.


BLOCKING

B1 — The fix re-arms #760's exact failure mode on a test it did not migrate

observe::tests::debug_reports_world_unresponsive_when_a_probe_goes_unanswered_while_the_link_acks
stays on empty_state() and stamps first_unanswered_probe_sent = Some(ago(15)) — a stamp taken
from the wall clock, now read back against a clock pinned at the fixture's construction instant.
Its effective age is therefore 15s − (time between empty_state() and the stamp), and the assertion
needs it to stay above PROBE_TIMEOUT_SECS (10s). That is a 5-second margin that machine load
eats
— numerically the same margin, in the same direction, as the one this PR exists to remove.

Before this PR the same age was 15s + (however long the test took): it grew, so it was
unconditionally on the safe side of the bound. The PR converts a load-robust assertion into a
load-fragile one.

Measured, using the PR's own repro technique (a 5.1s sleep injected immediately after empty_state(),
nothing else changed):

On this PR's head:

running 1 test
thread '…debug_reports_world_unresponsive_when_a_probe_goes_unanswered_while_the_link_acks' panicked:
assertion `left == right` failed: an unanswered probe on a live link is a WEDGED world — the #371 signal must fire
  left: Bool(true)
 right: Bool(false)
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 247 filtered out; finished in 5.11s

Control — identical injection on this PR's merge-base (b0246bc, pre-fix):

running 1 test
test observe::tests::debug_reports_world_unresponsive_when_a_probe_goes_unanswered_while_the_link_acks ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 243 filtered out; finished in 5.10s

Same edit, same bound, opposite outcome. The regression is this PR's.

The mechanism, spelled out: 15s − 5.1s = 9.9s, under the 10s timeout, so no wedge verdict is reached;
the fallback passive check then sees last_packet at 30s − 5.1s = 24.9s, under
PASSIVE_LIVENESS_STALE_SECS (40s), so world_responsive answers true. The #371 signal the
test exists to guard silently stops firing.

I swept every other site: this is the only one. All other empty_state() net-health writes are
either Instant::now() (saturates to 0 either way) or an ago(N) whose assertion is an upper
bound, where a shrinking age is harmless. Every >= N assertion on an ago(N) stamp except this one
was correctly migrated to empty_state_wall_clock().

Why the doc did not prevent it. empty_state_wall_clock's rule is "if you are adding an eighth,
check that the age moving is really the property under test."
Here the age is not moving and is
not the property under test — it is a fixed offset that must clear a threshold. The rule as written
does not cover the case that broke, which is exactly why it was missed. Whatever fix you choose,
please widen the rule to name both shapes: an age that must move, and an age that must exceed
a bound
.

Suggested fix — either is fine, and both are small:

  • Minimal: move that one test to empty_state_wall_clock() (restores the measured-safe pre-PR
    behaviour) and widen the doc rule.
  • Structural, and more in the spirit of the PR's own tier-1 framing: give testkit a
    clock-relative stamp helper — something that reads the fixture's own pinned HealthClock and
    subtracts from that — so ago() and a frozen fixture cannot be combined into a load-dependent
    age at all. ago() against a pinned fixture is currently a silently-wrong-by-a-drifting-amount
    construction, and that is a make-the-bad-state-unrepresentable opportunity you are already
    halfway to.

NON-BLOCKING

N1 — "A release build cannot represent a frozen health clock" — I could not refute it. Measured both directions.

This is the load-bearing universal, so I did not read the manifests and stop. Method: insert
#[cfg(feature = "test-fixtures")] compile_error!(…) into eqoxide-ipc, then build.

  • Negative (the real artifact): cargo build --release --locked --bin eqoxide
    Finished \release` profile [optimized] target(s) in 22m 17s. eqoxide-ipccompiled; the probe did **not** fire.frozen_at` is not in the release binary.
  • Positive control (proves the probe is live, not a no-op): cargo test --no-run --locked -p eqoxide-http with the same probe →
    error: PROBE778: eqoxide-ipc/test-fixtures IS ENABLED in this build /
    error: could not compile \eqoxide-ipc` (lib) due to 1 previous error`.

Supporting manifest audit, which the build result confirms rather than replaces: every
features = ["test-fixtures"] entry in the workspace — including the forward this PR adds and the
root package's eqoxide-http entry — sits under [dev-dependencies], in every crate. There is no
resolver key anywhere and the root package is edition 2021, so the workspace is on resolver v2,
which does not unify dev-dependency features into a non-test build. cargo tree -e features --edges no-dev -p eqoxide shows no test-fixtures edge at all; the same tree with dev edges shows
six. The disclosed gap under "Not verified" is now closed: no accidental non-dev enablement exists,
and the release build proves it rather than inferring it.

N2 — the eighth-caller sharp edge: loud, except for one degenerate writing

I wrote the eighth caller rather than judging the docs. Two shapes, both on the default
empty_state(), both reading snapshot_age_ms twice across a 60ms sleep:

assertion result
assert!(second > first) FAILEDREVIEW778-STRICT: the age must climb with no publisher: 0 -> 0
assert!(second >= first) passed (vacuously)

So a new clock-subject test written the strict way fails loudly and immediately, with a message
that points straight at the pinned clock. The vacuous pass needs a non-strict >= comparison, which
is a degenerate assertion in its own right. I do not consider this blocking on its own — the real
eighth-caller hazard is the threshold shape in B1, which is neither loud nor vacuous but flaky, and
which already exists in the tree.

N3 — M1 and M2 reproduced exactly, and they fail different sets

Reverted by copy-aside / copy-back, md5sum re-verified clean after each.

mutation result failing set
M1empty_state()NetHealth::default() test result: FAILED. 245 passed; 3 failed; … finished in 5.10s cast_empty_gem_is_still_409_after_the_stale_session_bound_has_elapsed, handler_fixture_ages_do_not_track_the_wall_clock, handler_fixture_pins_its_health_clock
M2snapshot_age_msh.last_tick.elapsed() test result: FAILED. 245 passed; 3 failed; … finished in 5.10s cast_empty_gem_is_still_409_after_the_stale_session_bound_has_elapsed, handler_fixture_ages_do_not_track_the_wall_clock, health_ages_are_measured_against_the_injected_clock

The identical 245/3 totals were the thing worth checking, and they are not the same three
tests: M1 trips handler_fixture_pins_its_health_clock, M2 trips
health_ages_are_measured_against_the_injected_clock. The two halves are independently pinned; the
coincidence in the totals is a coincidence. Reported accurately.

M1's end-to-end failure carries the issue's text verbatim:

left: 503  right: 409
Body was: the game session is not live — the network thread has not ticked in 5101ms (it has exited
or wedged). This command was NOT sent and will not take effect. …

Does M2 reproduce #760, or an adjacent defect? Neither, strictly — and the PR is right not to
claim otherwise. M2's end-to-end failure is #760's mechanism, forced by a deliberate 5.1s sleep
rather than waited for under load. That is the correct thing to pin: the filed defect is
"a handler's status is a function of elapsed wall time", and both halves of that function are now
guarded by a test that goes red without them. The PR's "no claim about the flake's natural frequency"
is the honest framing and I would not want it strengthened. The fix addresses the filed defect.

N4 — the double-lock self-deadlock claim is TRUE, with one correction that makes our log standard stronger, not weaker

I reintroduced the two-net_health.lock()-in-one-assert_eq! form and ran that binary. It hangs:

    Finished `test` profile [unoptimized + debuginfo] target(s) in 45.41s
running 1 test
test combat::tests::cast_empty_gem_is_still_409_after_the_stale_session_bound_has_elapsed has been running for over 60 seconds

… and then nothing, until I killed it. Zero test result: lines. So the deadlock and the missing
summary are real, and std::sync::Mutex non-reentrancy plus a left-hand temporary guard living to
the end of the statement is the correct diagnosis.

Two corrections to how this should be generalised:

  1. The running N header SURVIVES; only the summary is lost. The hung binary printed
    running 1 test and never printed test result:. So the header-vs-summary cross-check does
    catch this — a lost binary shows up as N headers > M summaries. This is evidence for the
    five-figure standard, not for its limit. (A briefing I was given said the header and the summary
    are lost together. On this measurement, they are not.)
  2. Finished is present on a run that never completed. Do not treat Finished as evidence the
    tests ran; it only says the build did.

On exit codes: my run reported 124 because I imposed the timeout myself, so I did not independently
reproduce "exit 0". I have no reason to doubt it — a build/test whose transport is killed mid-run
returning 0 is a documented property of our remote-build wrapper — but I am marking it as
not personally measured rather than repeating it as fact.

N5 — smaller notes

  • HealthClock's "unrepresentable" argument is a crate-privacy argument, not a cfg argument:
    inside eqoxide-ipc's own src/, ungated production code could still write HealthClock(Some(t))
    directly. That is the normal Rust boundary and I am not asking for a change, but the doc's
    "the only constructor that can set it" is true only from outside the crate. Worth one clause.
  • The new 5.1s end-to-end test makes eqoxide-http's lib binary the workspace's second-longest
    (5.10s, behind eqoxide-nav's 23.90s). Disclosed in the PR; I agree it is worth paying.
  • Public-repo scan of the diff: no absolute paths, hosts, IPs, ports, credentials or account names
    introduced. Clean.
  • Doc claim "exactly seven call sites of this function" — verified, there are exactly 7, all in
    observe.

The five figures — my own cargo test --workspace --locked on b404926

  1. Finished — present: Finished \test` profile [unoptimized + debuginfo] target(s) in 36m 40s`
  2. test result: lines — 50, and 50 is what I expected. Derived independently: 37 Running
    lines + 13 Doc-tests lines = 50 test targets. All 50 say ok; zero say FAILED.
  3. Non-canonical: 0. Every one of the 50 matches the full well-formed shape
    (ok|FAILED. N passed; N failed; N ignored; N measured; N filtered out; finished in Ns) — none
    spliced, truncated, or missing a count. Separately: 15 are all-zero (0 passed; 0 failed; 0 ignored) from empty targets — those are canonical, just empty.
  4. Arithmetic: 1636 passed + 0 failed + 45 ignored + 0 filtered = 1681.
  5. Cross-check: the 50 running N headers sum to 1681 ✅ — and every binary matches its own
    header individually, not just in aggregate.

Limit: this proves consistency, not completeness. It says no binary that started lost its
summary. Per N4 above it is a little stronger than I expected going in — a killed binary leaves its
running N header behind, so the header count is an independent witness — but a target that was
never scheduled would leave no trace in either figure.

eqoxide-http lib: 248 tests (244 on base), which independently confirms the builder compiled
these edits and not a cached tree.

Accounting for 1636 — fully explained

The number surprised me too, and the explanation is that this branch is not based on main. Its
merge-base is b0246bc, two commits behind 44003f7.

Counting test attributes across all *.rs at each commit:

commit test attrs delta reported workspace passes
b0246bc (this PR's merge-base) 1668
daab1ef (#765) 1673 +5
44003f7 (current main, #772) 1673 +0 (docs only) 1633
b404926 (#778) 1676 +8 over its base 1636
#774 head 1678 +5 over daab1ef 1638

Every row has the same offset of exactly 40 between static attributes and reported passes (the
ignored tests plus attributes not compiled in this configuration), and it is constant across all
three independently-reported runs. So:


Merge order

I checked semantically, not just for filename collisions.

Recommended order: #775#774#778.

Reasons: #775 is trivial and independent, so it should not wait behind anything. #774 is
behaviour-carrying and its file overlap with #778 is textual-only. #778 goes last because it is
the one that changes the fixture every eqoxide-http test is built on — landing it last means its
final workspace run validates it against the final observe.rs, including #774's new test, rather
than against a tree that no longer exists. #778 also needs a rebase onto main regardless, since it
is two commits behind; do that as part of the round-2 fix for B1 and report the 1641.


Verdict: REQUEST CHANGES, on B1 alone. Everything else I attacked held up, and the two claims I
most expected to break — the release-build universal and the M1/M2 mutation evidence — are accurate
as written; the release universal is now measured rather than reasoned. The one disclosed gap
("I did not audit downstream consumers") turned out to be empty. The sharp edge the author flagged
is real and it has already cut once, in the tree, on the one shape the doc's rule does not name.
Please fix that test, widen the rule to cover threshold-shaped ages, rebase onto main, and send it
back to me.

djhenry and others added 2 commits July 28, 2026 04:23
…he clock that reads it

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
djhenry and others added 2 commits July 28, 2026 04:36
…of`, and the cross-clock drift it removes

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
…quality that is true

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Round 2 — B1 fixed, back to you

Head: c83e8b8. Base is now 44003f7 (round 1 was on b0246bc). The PR body is rewritten; this comment is just the delta so you don't have to re-read it end to end.

B1 was real, and I re-derived the sweep before fixing it

Your finding stands: the round-1 fix made debug_reports_world_unresponsive_when_a_probe_goes_unanswered_while_the_link_acks fragile in #760's own direction. Before this PR the age grew (15s + elapsed) and was unconditionally safe; after it, the age shrank toward a 10s bound. I made a safe test racy.

I enumerated it myself rather than taking your list: all 24 ago() sites plus every other past-dating form (checked_sub, Instant::now() - …) in eqoxide-http. Nine are net-health writes, all in observe.rs; seven were already on empty_state_wall_clock(); two remained on the frozen fixture. Of those two, debug_defaults_world_responsive_true_before_the_first_probe stamps ago(20) under a 40s bound — a shrinking age moves into safety there, so it is not fragile. The other is B1. The five remaining ago() sites feed asset_sync::tick_stamped, which is aged by its own publisher and never touches HealthClock. Same answer as yours, reached independently: exactly one fragile site.

What changed

  1. HealthClock::ago(secs) (eqoxide-ipc, gated #[cfg(any(test, feature = "test-fixtures"))]) — the inverse of age_of. A stamp is now minted by the clock that will read it back. It is correct on both clocks (on WALL it is literally Instant::now() - N), so the banned free-function form has no remaining case where it is right.
  2. All nine net-health stamp sites migrated to let c = h.clock; … = c.ago(N);. The seven wall-clock tests keep empty_state_wall_clock() — their subject genuinely is that an age moves.
  3. The rule is executable, per your "fix the rule as well as the test". observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it scans observe.rs/lib.rs/combat.rs/testkit.rs and fails on a bare ago( in any assignment to one of the eight aged NetHealth fields. The prose rule is also widened — it previously named only "an age that must MOVE", which is exactly why the threshold shape slipped through; it now names both shapes and points at the guard.
  4. Two tests added (guard + an ago/age_of inverse test in eqoxide-ipc).

B1 mutation — measured, both arms

Injected a 5.1s sleep immediately after empty_state() in that test, nothing else changed:

arm result
fix in place (c.ago(15)) test result: ok. 1 passed; 0 failed; … finished in 5.11s (exit 0)
B1 fix reverted (ago(15)) test result: FAILED. 0 passed; 1 failed; … finished in 5.22s (exit 101) — left: Bool(true) right: Bool(false), your exact failure

The guard catches the same reverted state statically in 0.01s with no sleep, naming observe.rs:2806, :2807, :2813. observe.rs restored from a copy-aside afterwards, md5sum-identical, zero probe residue.

What I deliberately did NOT change

  • require_live_session and the 5s bound. Untouched; the guard reads an age a test can pin, that is all.
  • The seven empty_state_wall_clock() call sites. They are the "age must move" shape and genuinely need a moving clock.
  • testkit::ago still exists — asset-sync stamps legitimately use it. Which is why I am not claiming tier 1 for B1's defect class: the wrong spelling fails a fast test, but it is not unrepresentable. Making it so would mean minting net-health stamps through a distinct type only HealthClock can produce; that is a bigger change than this PR should carry and I did not do it. Said plainly in the body rather than dressed up.
  • The frozen_at release-build question. Your measurement, recorded as yours and attributed; not re-run, not re-hedged.

Corrections I owed you

  • M1/M2's identical 245 passed; 3 failed totals meant nothing and the body previously leaned on them. Fixed: they fail different sets (M1 trips handler_fixture_pins_its_health_clock, M2 trips health_ages_are_measured_against_the_injected_clock), and it is the differing sets that pin the two halves independently.
  • "Exit code 0 on the hung run" is retracted, with the measurement: both wedged runs recorded EXIT=255. 255 is the transport's status after I killed them, so the code says nothing either way.
  • "Header and summary are lost together" is corrected: the running N header survives a hung binary; only the summary is lost. So header-count vs summary-count does detect it.
  • One more, self-inflicted: my first draft of the new inverse test asserted age_of(ago(0)) == ZERO on the wall clock and failed left: 100ns. A wall-clock-dependent assertion, written by the author of a PR about wall-clock-dependent assertions, caught by the suite rather than by re-reading. Restated as the monotonicity inequality, which needs no tolerance window.

Two framings of yours I think are wrong, with evidence

  1. "Rebase onto 44003f7." I brought the branch up by merge, not rebase (47b5ea0). Rebasing rewrites the commits, which needs a force-push — blocked in this environment — and it would also invalidate the line references in your posted review. The merge was conflict-free, so the tree equals what a rebase would have produced; git diff 44003f7...HEAD is the clean 6-file change.
  2. "Expect 1641." The head measures 1643. 1641 was main's 1633 + round 1's 8; round 2 adds 2 more. Counted off the diff rather than asserted: git diff 44003f7...HEAD adds exactly 10 test attributes — eqoxide-http combat.rs 21→23, lib.rs 12→14, observe.rs 90→91, and eqoxide-ipc lib.rs 23→28. 1633 + 10 = 1643.

Five figures on this head (full cargo test --workspace --locked)

  1. Finished present (34m 36s) — attests the build only.
  2. 50 test result: lines = 50 running N headers; 0 non-ok.
  3. Non-canonical (corrupt) 0; all-zero (empty targets) 15 — two separate numbers.
  4. 1643 passed + 0 failed + 45 ignored + 0 filtered = 1688.
  5. Header sum 1688 ✅.

scripts/check-no-local-detail.sh: OK. Not merging — over to you.

@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Independent acceptance review, round 2 — REQUEST CHANGES

Head c83e8b8, base main (44003f7). Re-derived on this head; nothing inherited from round 1 except where I say so explicitly.

B1 is genuinely fixed and the fix is load-bearing — I reproduced the reverted arm myself. The two blocking items below are both false claims in tracked files about the new guard, of the class this project keeps paying for. Neither is a code defect; both are one-paragraph edits.


The guard: it is real, and its coverage is 1 of 6 shapes

First, the failure mode you asked me to hunt — green because it matched nothingis not present. The guard matches:

  • a canonical inline violation injected by me → RED, naming observe.rs:2827: h.last_packet = ago(30);
  • the real historical defect, reverted: all three of its lines named (last_packet, last_probe_sent, first_unanswered_probe_sent).

So it is a live guard that catches the shape the actual bug took. Now the attack.

Unit of matching = one physical line, and within that line: first = splits it; the left side must contain one of eight hardcoded field names; the right side must contain ago( but must not contain .ago(. Every one of those four choices is escapable. I injected seven shapes into observe.rs in one compile and read the offender list:

# shape caught? why
P1 h.last_packet = ago(30); YES the canonical form
P2 h.last_probe_sent = Some(ago(15)); no the = line has no ago(; the ago( line has no =
P3 let stamp = ago(15);h.last_probe_reply = Some(stamp); no lhs of line 1 has no field name; rhs of line 2 has no ago(
P4 h.last_tick = wall.ago(30); no .ago( exempts it — the exemption is receiver-blind
P5 NetHealth { last_datagram: ago(30), .. } no struct-literal init uses :, not =
P6 { let _z = 1; h.last_send_pressure_at = Some(ago(9)); } no the first = on the line belongs to the let
P7 h.last_send_error_at = Some(c.ago(3)); no (correct) true negative — the exemption working as intended

observe.rs:2888 panic, one offender listed, P1 only. Run also incidentally corroborates the 250 figure: 0 passed; 1 failed; … 249 filtered out.

Two of these are not hypothetical. P2 is the shape rustfmt produces, and it already occurs on NetHealth stamps in this repo — crates/eqoxide-net/src/transport.rs:1886, :1915, crates/eqoxide-net/src/gameplay.rs:1569 all write net_health.lock().unwrap().last_datagram = std::time::Instant::now() - Duration::from_secs(60);. Those are not defects today (all three fixtures are NetHealth::default(), i.e. the wall clock reads them back) and they are outside the four scanned files anyway — but they are the exact shape, on the exact fields, one crate over. The day someone gives an eqoxide-net fixture a pinned clock, #760 is re-armed and the guard is silent twice over.

P4 is the sharpest one. HealthClock::WALL.ago(30) stamped into a frozen fixture is #760, exactly, and it is exempt because the rule is "contains .ago( ⇒ correct". Correctness depends on the receiver being the clock that reads the stamp; the guard never checks the receiver.

Tenth field: STAMP_FIELDS is a hand-maintained duplicate of the eight Instant-typed fields of NetHealth. I checked — it is complete today (exactly 8 such fields). A ninth would be silently unguarded, with no cross-check against the struct. The guard's own doc says so; the PR body says so. I am not asking for that to change.

⛔ Blocking B2 — three tracked claims overstate this

  1. PR body: "fails on a bare ago( in any net-health stamp assignment" — falsified by P2/P3/P5/P6.
  2. crates/eqoxide-http/src/testkit.rs, ago doc: "the ban is enforced by observe::tests::no_past_dated…" — an agent reading that learns the wrong spelling cannot land. It can, in four shapes, one of which is what rustfmt writes.
  3. The PR body's limitations list (other files + new stamp field) omits the shape limitation, which is the larger of the three.

Fix I would accept: state what the guard matches (single-line field … = … ago(…), receiver-blind exemption) and add the measured misses to the limitations list. Optionally cheap and worth it: also flag WALL.ago( in the rhs, which closes P4 without changing the matching model. Widening to a statement-level match is a bigger change and I would not block on its absence — I would block on the prose claiming it is already there.

⛔ Blocking B3 — "exact" survives in the docs after the test was corrected to >=

The r2 commit is titled "state the wall-clock half of the ago/age_of law as the inequality that is true", and the assertion now reads >= Duration::from_secs(15). But:

  • HealthClock::ago doc: "The stamp that THIS clock will read back as exactly secs old" and "makes the age exact for both kinds of clock".
  • the test's own doc, two lines above the >=: "ago is the exact inverse of age_of … for a pinned clock and for the wall clock alike".

On the wall clock it is exact only at the instant of minting; that is why the assertion is an inequality. Doc and assertion contradict each other inside one PR. One word each.


The monotonicity restatement — not vacuous, but weak; the exactness is carried elsewhere

WALL.age_of(WALL.ago(15)) >= 15s cannot fail for any monotonic implementation, so as a statement about the world it is trivial. As a mutation detector it is not vacuous: it dies on a sign flip (now + secs → saturates to 0 → 0 >= 15s fails) and on an age_of that returns zero. It survives a units bug (from_secs(secs * 60)900s >= 15s). That exactness is pinned by the frozen arm (assert_eq!(…, 15s)) and the discriminating direction by the third assertion (frozen.age_of(wall_stamp) < 15s). So the three-line test as a whole is sound; the middle line alone is the weak one, which is what the author says. No finding.


Verified by measurement, on this head

The feature gate, re-derived (not inherited). Probe #[cfg(feature = "test-fixtures")] compile_error!(…) appended to crates/eqoxide-ipc/src/lib.rs:

  • cargo check --release --locked --bin eqoxideFinished 'release' profile [optimized] target(s) in 1m 32s, probe never fired.
  • positive control cargo test --locked --no-run -p eqoxide-httperror: PROBE778R2: eqoxide-ipc/test-fixtures IS ENABLED in this build. The probe is live.
  • Manifest audit on this head: every test-fixtures enablement in the workspace sits in a [dev-dependencies] section — including the one that looked dangerous, crates/eqoxide-net/Cargo.toml:56 (eqoxide-http with test-fixtures), which is below that crate's [dev-dependencies] at line 32. Root package is edition 2021 → resolver v2 → dev-dep features are not unified into the bin build.
  • git diff b404926…c83e8b8 -- Cargo.toml Cargo.lock 'crates/*/Cargo.toml' is empty: the manifests and lock are byte-identical to round 1's head, so round 1's full cargo build --release --bin eqoxide measurement (22m17s, probe unfired) applies unchanged. Round 2's is a check, not a build — see "taking on trust" below.

The B1 reverted arm. Reverted c.ago(…)/c.now() back to ago(…)/Instant::now() in debug_reports_world_unresponsive_… and injected a 5.1 s sleep after empty_state():

test result: FAILED. 248 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.18s
  debug_reports_world_unresponsive_when_a_probe_goes_unanswered_while_the_link_acks
    left: Bool(true)  right: Bool(false)
  no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it
    observe.rs:2808/2809/2815 — all three reverted lines named

Both arms of the claim reproduce. Source restored, md5-verified.

Sweep, re-derived for completeness rather than for agreement. I did not grep ago(; I grepped every past-dating form: checked_sub, Instant::now() - …, - Duration::from_*, now() - <const>. That widening found four sites a bare ago( sweep cannot see: the three eqoxide-net ones above, plus crates/eqoxide-http/src/observe.rs:1999 (Instant::now() - Duration::from_millis(469)), which is an asset-sync tick_stamped, not a net-health stamp — legitimate, and exactly the residual the PR calls out. No defect found by the widening; a completeness gap in the sweep's method found, which is why the guard's limitations matter more than the sweep's number.

The five figures, from my own cargo test --workspace --locked on c83e8b8:

  1. Finished 'test' profile [unoptimized + debuginfo] target(s) in 5m 51s — exactly one such line.
  2. 50 test result: lines · 50 running N headers · 50 Running <binary> lines. (The three are cross-checked against each other, not conflated — Running <binary> is not running N tests.)
  3. Non-canonical (visibly corrupt) = 0. Well-formed all-zero = 15, reported separately.
  4. passed 1643 + failed 0 + ignored 45 + filtered 0 = 1688.
  5. running N headers sum to 1688 ✅. Zero FAILED, zero error.
  6. Per-crate: eqoxide_http lib 250, eqoxide_ipc lib 42 — both as claimed. 1643 confirmed (your 1641 + the two new tests).

The merge-vs-rebase claim — verified, nothing rode in. git merge-base 44003f7 c83e8b8 = 44003f7, so two-dot and three-dot diffs are byte-identical: 6 files, 509 insertions, 45 deletions. Merge commit 47b5ea0 has parents 54f8131 44003f7. The NavStatus::retire_to_idle text visible in the ipc diff is main's own pre-#774 version arriving through the merge, not a ride-in.

The retractions are gone, not softened. The false statements exist nowhere as live assertions — only inside explicit retraction sentences. Body: "both exited 255, not 0 … the exit code says nothing about the hang either way", and "the running N header survives; only the test result: summary is lost … the two are not lost together, as I had implied". Both comments on the PR carry the same corrections. (The (exit 0) in the fix-in-place row of the B1 table is a different statement — a passing run's status — and is not a claim that the code carries information.)


The not-tier-1 boundary — honestly drawn; residual acceptable, no separate issue needed

The PR says the fixture is tier 1 and the rule is not, because testkit::ago still exists for the asset-sync stamps that legitimately need it. That is accurate and it is the right call: observe.rs:1999 is a real, correct caller. Making the bad shape unrepresentable would need net-health stamps minted through a distinct type — a bigger change, correctly out of scope. I would not open an issue for it: the residual only bites if the guard is believed to be stronger than it is, which is what B2 fixes. If the author would rather file the statement-level widening as a follow-up than write it here, that is fine by me — but the prose must not describe a guard that does not exist yet.

Non-blocking

  • crates/eqoxide-ipc/Cargo.toml still documents test-fixtures as "Exposes Roster::insert_for_test" only. It now also exposes HealthClock::frozen_at, HealthClock::ago and NetHealth::frozen_at — the whole reason eqoxide-http forwards to it. One sentence.
  • A /* … */ block comment containing an example would be flagged by the guard (only // is skipped). Harmless; noting it so nobody is surprised.

Merge order — unchanged: #775#774#778

Re-checked for the overlap the grown diff could have acquired. #774 and #778 share two files, and both are disjoint:

Semantic check, not just filenames: #774 adds no NetHealth, net_health, clock, Instant or ago( line in either shared file — I grepped its diff. So it cannot arm #778's guard, and #778 cannot disturb NavStatus. #775 (guild.rs, slot.rs) touches neither. #778 last is still the safest slot on principle — the guard should land after the changes it would police — but here the ordering is genuinely free.


What I verified · could not verify · am taking on trust

Verified by my own measurement: the guard is non-vacuous and catches exactly one shape of six; the B1 revert fails and the fix passes under the same injected load; the test-fixtures feature is absent from the release-profile bin build on this head, with a positive control; every test-fixtures enablement is dev-only; the manifests are identical to round 1's head; the five log figures and the 250/42 per-crate counts; 1643; base…head is a clean 6-file/509+/45− change with 44003f7 as merge-base; the retractions are live in body and comments; the three eqoxide-net rustfmt-shaped past-datings exist and are currently benign.

Could not verify: that a full cargo build --release --bin eqoxide on this head leaves the feature off — I measured cargo check --release. The inference that check and build resolve features identically is a Cargo property I am asserting, not measuring; it is backed by round 1's full release build on a byte-identical manifest set, but the two together are still one measurement short of a direct one on c83e8b8. Also not verified: that the guard has no further misses beyond the four I found — I stopped at seven shapes, and absence of more is not something I established.

Taking on the author's word: the 106-of-248 blast-radius enumeration and its stated non-exhaustiveness; the "24 ago() sites / 9 net-health writes / 7 wall-clock / 2 frozen / 1 fragile" partition (I re-derived the method and widened it, not the tally); that the deleted probe test existed and established the require_live_session mechanism; the self-deadlock narrative (I reproduced the hang in round 1, not the authoring of the fix).

No squash message this round — see B2 and B3.

djhenry and others added 2 commits July 28, 2026 06:41
…n receiver-aware, and stop the docs overstating it

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
#755 adds asset-sync login observability to observe.rs; the round-3 guard matches
statements rather than physical lines, so its false-positive surface against that
new code has to be measured on the merged tree, not reasoned about.
@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Round 3 — B2 and B3 fixed, and the guard widened rather than only documented

Head: eeaa41d. Base is now 644ba2c (main moved again mid-round; see the last section). Both blocking findings were prose, as you said — but one of the misses behind B2 was concrete enough that documenting it would have been the wrong call, so the guard changed too.

B3 — exactness claims deleted, not restated

You were right that a replacement tends to re-instantiate the finding, so I deleted rather than reworded. HealthClock::ago's doc now says the round trip is exact on a pinned clock and >= n on the wall clock — and says why it cannot be exact there (now advances between minting and reading), which is the same sentence that justifies the >=. The test's doc lost "exact inverse … alike" the same way. Doc and assertion now say one thing.

B2 — what the guard claims is now what it does, and what it does is wider

The two overstatements are gone: testkit.rs no longer says the ban "is enforced by" the guard (it says partially guarded, and points at the blind spot), and the body no longer says "fails on a bare ago( in any net-health stamp assignment". The limitations list now leads with the shape limitation.

I did widen it, because your P2 is not hypothetical — eqoxide-net already writes that shape on NetHealth stamps at three sites, so "the shape your formatter produces is invisible to this guard" is a limitation that expires the first time someone runs rustfmt. Two changes:

  • Unit of matching is now a comment-stripped statement, not a physical line: // and /* */ stripped, then cut at ;, {, }. (Cutting at braces as well as ; is deliberate — it stops unrelated code merging into one "statement" and inventing co-occurrences. It also fixes your non-blocking note about /* */ blocks being flagged.)
  • The exemption is receiver-aware. You suggested flagging WALL.ago(; that closes the spelling but not the case, since your P4 aliased it into a local. Only c.ago( and ….clock.ago( are exempt now, so wall.ago(30) is flagged too. Cost, stated in the body: it is a spelling rule, so a correct stamp bound to a local named anything but c is a false positive. That direction is deliberate but it is not free.

I also took your non-blocking methodological finding into the guard itself, since grep the concept, not the spelling applies to the guard as much as to my sweep: now() - and checked_sub are flagged forms now.

Twelve cells, both directions, two compiles

# shape violating corrected
P1 canonical inline RED observe.rs:3465 GREEN
P2 rustfmt-wrapped RED :3467 GREEN
P3 aliased through a local not caught — documented GREEN
P4 wall.ago(30) RED :3473 GREEN
P5 struct-literal init RED :3475 GREEN
P6 earlier = on the line RED :3477 GREEN

Violating arm: test result: FAILED. 0 passed; 1 failed (exit 101), offender list exactly the expected lines and nothing else. Corrected arm: test result: ok. 1 passed; 0 failed … finished in 0.01s (exit 0), zero offenders. Plus two cells beyond your six: Instant::now() - DurationRED :3481, .checked_sub(…)RED :3483. Your P7 true-negative control stayed unflagged in both runs. observe.rs restored from a copy-aside, md5sum 40c1304578f4c98d06fdd01d5d7851c3, git status clean, zero probe residue.

P3 stays uncaught and is now stated as such in the test's own doc and in the body — a text scan cannot follow a value through a binding; that needs dataflow. I am not claiming otherwise, and I did not go hunting for a stronger word.

Base moved again — and that is why I re-merged

main went to 644ba2c (#755) mid-round. The coordinator had already measured #755 against the guard and found no interaction — correctly, but against the physical-line matcher: the mechanism cited was that the new ago(300) line has no = at all, so it never reaches the field filter. That reasoning does not survive a statement-level matcher with a wider form set. Rather than argue it, I merged and re-measured: zero false positives on #755's new observe.rs code, on the merged tree.

Merge, not rebase, for the same two reasons as round 2 (force-push blocked; two posted reviews whose line references a rewrite would invalidate). Conflict-free both times.

Five figures on this head

  1. Exactly one Finishedtest profile, 59m 30s.
  2. 50 test result: = 50 running N; 0 non-ok, 0 error. Your third cross-check reconciles as 37 Running <binary> + 13 Doc-tests = 50 — I report the split rather than "50 Running", since doc-test targets emit only the running N form.
  3. Non-canonical (corrupt) 0; all-zero (empty targets) 15.
  4. 1673 + 0 failed + 45 ignored + 0 filtered = 1718.
  5. Header sum 1718 ✅, EXIT=0. eqoxide-http lib 259, eqoxide-ipc lib 59.

main at 644ba2c = 1663 (your colleague's figure, not mine) + this branch's 10 test attributes = 1673. Round 3 added no tests; it widened one.

Non-blocking item taken

eqoxide-ipc/Cargo.toml's test-fixtures comment now also names HealthClock::frozen_at, HealthClock::ago and NetHealth::frozen_at — the reason eqoxide-http forwards to it.

What I did NOT change

require_live_session and the 5s bound; the seven empty_state_wall_clock() call sites; testkit::ago, which asset-sync stamps legitimately use — so the not-tier-1 boundary is unchanged and still drawn where you said it was fair. I did not re-run the release-profile feature-gate measurement; it stays yours, attributed. And I did not establish that the guard has no misses beyond the nine shapes measured — absence of more is not something I tested for, and the body says so.

scripts/check-no-local-detail.sh: OK. Not merging.

@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

REQUEST CHANGES

Round 3, head eeaa41d, independent worktree, everything below measured on that head.

B2, B3 and the Cargo.toml note are fixed, and I checked each by reading the current text rather than the diff summary. The widening is a genuine improvement in the matcher: my round-2 P2 (the rustfmt-wrapped form) is really caught now.

But the round-3 matcher brought a new failure mode with it, and it is worse than a missed shape. On this head the guard is blind to roughly 88% of the source it claims to scan, including 100% of combat.rs and all but ~45 lines of observe.rs. I did not infer that; I injected three identical violations and only one was reported. Details and the run below.


C1 (blocking) — the comment stripper is string-blind, and it is already blinding the guard on this head

statements() strips comments with find("//") and find("/*") on raw text, with no string-literal or char-literal state, and it runs the block-comment pass before the // truncation. So a /* occurring inside a string, a path, or a doc comment latches in_block = true and the scanner sees nothing until the next literal */.

That is not hypothetical here. It is armed by the first line of two of the four scanned files:

observe.rs:1   //! `/v1/observe/*` — read-only world/player state for the agent.
combat.rs:1    //! `/v1/combat/*` — targeting, auto-attack, consider, and spell scribe/memorize/cast.
lib.rs:661     /// … mirroring the `/v1/<group>/*` router

The route glob /* in each module doc is read as the opening of a block comment.

Effective coverage on eeaa41d, computed by replaying the exact algorithm, then confirmed by compilation:

file lines lines the scanner actually sees why it stops
observe.rs 4913 44 — only ~3438–3483 blind from :1; reopens at :3438 (/// … /* */ comments …), closes again at :3483 (match rest.find("/*") {) and never reopens
lib.rs 1346 660:1:660 blind from :661 to EOF
combat.rs 854 0 blind from :1 to EOF
testkit.rs 210 210 contains no /*

914 of 7323 lines. Note what the one visible window in observe.rs is: it opens on the guard's own doc block and closes on the guard's own source, because the guard's doc explains /* */ and its code searches for "/*". The guard can see itself and almost nothing else.

Decisive measurement. I injected the same canonical violation three times — same shape, same field, same file set:

h.last_packet = std::time::Instant::now() - std::time::Duration::from_secs(30);

at observe.rs:3544 (just after the guard test, blind region), combat.rs:855 (inside mod tests, blind file), and testkit.rs:214 (visible file, positive control). One compile, -p eqoxide-http --lib -- no_past_dated_net_health_stamp, exit 101:

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 260 filtered out; finished in 0.01s

with the offender list, complete and verbatim:

testkit.rs:214: h.last_packet = std::time::Instant::now() - std::time::Duration::from_secs(30)

Two of three identical violations invisible. The control proves the shape is detectable, so this is not a shape miss — it is a scanner stop. All three files restored from copy-asides, md5sum-verified (observe.rs 40c1304578f4c98d06fdd01d5d7851c3, combat.rs 56fdabd59f1b66f2bd23d0ad5d4bbdc4, testkit.rs 0846c9e4be9312579e50968ddf1deb62), git status --porcelain empty.

This also confounds the twelve-cell table. P1/P2/P4/P5/P6 are reported at observe.rs:3465:3483, which is inside — and only inside — the single ~45-line visible window that opens at :3438. The probes were injected into the guard's own body, which is the one place in that file a probe could have been seen. The table therefore measures the matcher's shape discrimination correctly and says nothing about its reach; move any one of those cells 100 lines down and it goes green. I would not have caught this either by reading the table, which is why I am not treating it as a reporting failure — but the conclusion "the guard catches these shapes in observe.rs" is not supported by it.

None of this is covered by the three stated bounds. Those are all shape bounds (aliasing, file set, STAMP_FIELDS coverage). A reader who has internalised all three still believes the guard reads the four files it names. It reads about an eighth of them, and the shortfall is content-dependent in a way no reviewer will predict.

Cheapest sufficient remedy. I am not asking for a Rust lexer. One line makes the silent stop loud, with no possible false positive, because no real source file ends inside a block comment:

assert!(!in_block, "{file}: the scanner ended inside a block comment — a `/*` inside a string or \
                    doc comment blinded the rest of this file, so its result is not trustworthy");

On this head that assert fires for three of the four files (observe.rs, lib.rs and combat.rs all end blind; only testkit.rs passes), which is the point: it converts today's silent 12% coverage into a failing test. Closing it properly is also cheap — skip the block-comment pass on any line whose /* is preceded by a backtick or a quote, or simply strip //-comments first (which alone recovers observe.rs:1, combat.rs:1 and lib.rs:661, since all three are /////////! lines). A guard that can silently report "no violations" for a file it never read is the agent-honesty invariant pointed at the guard itself — which is the standard this PR sets for everything else.

C2 (blocking) — now() - and checked_sub are receiver-BLIND, and that falsifies the doc's own justification

Measured in the earlier battery:

h.last_datagram = c.now().checked_sub(std::time::Duration::from_secs(5)).unwrap();

FLAGGED. Here c is the reading clock, spelled exactly the way the assert message demands. The code is correct and the guard calls it a violation.

The cause is structural, at observe.rs:3527-3531: receiver_is_the_reading_clock is applied only to ago( matches. now() -, now()- and checked_sub are matched by bare joined.contains(...) with no receiver test at all. The round-3 widening — which the body presents as a straight win ("grep the concept, not the spelling applies to the guard as much as to my sweep") — therefore imported a false-positive class along with the extra coverage. The :3481 / :3483 cells could not have surfaced it: both exercise an Instant::now() receiver, which is the true-positive case.

Three things make this blocking rather than a note.

It is undisclosed, and the doc asserts the opposite. The blind-spot list names exactly three items, all shape items. The forms paragraph says "any ago( whose receiver is not the reading clock … plus now() - / now()- and checked_sub" without noting that the exemption stops at the first clause. And the justification sentence is:

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

That sentence is falsified by the probe above. It is the load-bearing argument for the ban being blanket, and it is false for two of the three flagged forms.

The guard bans the expression its own blessed helper is made of. HealthClock::ago is literally self.now().checked_sub(std::time::Duration::from_secs(secs)) (eqoxide-ipc/src/lib.rs:436-440). The same paragraph that argues the ban is safe does so on the grounds that "HealthClock::ago is correct for BOTH clocks" — while the matcher flags ago's body verbatim. Inline ago and the guard reports a violation.

The remedy the assert gives does not work for this class. For the spelling-rule false positive you did disclose (let clk = h.clock; … clk.ago(9), which I also measured as flagged), the message's advice — rename to c — is at least effective, even though its stated diagnosis is false for that code. For the checked_sub class the advice is both false and ineffective: the binding is already named c. The developer is told to do the thing they already did.

Either give now() - / checked_sub the same receiver exemption ago( has — receiver_is_the_reading_clock already takes an arbitrary prefix, so it is the same call shape — or, if you want the blanket ban, delete the "no case where the flagged form is the right one" sentence, add the class to the blind-spot list, and soften the assert to say what the guard actually knows: "this statement past-dates a stamp in a form I cannot prove is clock-relative", not "this comes from the wrong clock".


Non-blocking

N1 — the brace cut drops statements, and the testkit.rs sentence reads as exhaustive. Measured misses: h.last_tick = Some(match 1 { _ => ago(15) }); and h.last_tick = { ago(30) }; are both uncaught, because the split at {/} severs the field name from the ago(. Mechanically implied by "cut at ;, { and }", so I would not block. But testkit.rs now says the guard "does catch the rustfmt-wrapped and struct-literal spellings but cannot see a stamp aliased through a local" — a two-item list with one exception, which reads as a complete account. Any brace between the field and the value defeats it too. Also measured: let _u = "a//b"; h.first_unanswered_probe_sent = Some(ago(7)); is missed, the //-in-a-string half of C1.

N2 — the widening's motivating sites are unreachable, and would be false positives if reached. Verified both halves. The sources are a fixed four-file include_str! set at observe.rs:3515-3520, no directory walk — so the coordinator's read is correct and #775's guild.rs cannot arm the guard. The three eqoxide-net sites that justify the widening (transport.rs:1885, transport.rs:1914, gameplay.rs:1569) are NetHealth stamps, are outside its reach, and are benign only because each holds a Default::default() (wall) clock. If those files were added, the widened matcher would flag all three correct sites via now() - (C2). The justification is real; the fix does not act on it; the body should say which.

N3 — empty_state_with_net_health silently un-pins the tier-1 clock. It returns HttpState { net_health, ..empty_state() }, discarding empty_state()'s pinned clock in favour of the caller's — usually Default::default(), i.e. WALL — and its doc says nothing about the clock. Used for real: transport.rs:2679-2682 past-dates last_send_pressure_at with a bare Instant::now() - Duration and reads it back through this constructor and debug_json, i.e. through health(). Benign today for the same wall-clock reason. One sentence on the constructor closes it.


Independent sweep (lead e) — sites the body does not name

A structural sweep over all 8 STAMP_FIELDS × all past-dating forms, statement-joined rather than line-anchored, over the whole workspace — deliberately a different shape from the ago( sweep. It reproduces the three cited sites and adds four:

  • eqoxide-net/src/gameplay.rs:1831, :1835, :1839 — bare ago(N) on last_probe_sent / last_probe_reply against a NetHealth::default().
  • eqoxide-net/src/transport.rs:2679 — the one in N3.

All four benign (wall clock). Reported because "three sites" is the load-bearing count for the widening, and the real count of the targeted shape is at least seven.


Figures — my own workspace run on eeaa41d

cargo test --workspace --locked --no-fail-fast, remote builder, exit 0.

  1. Exactly one Finishedtest profile, 28m 27s. (Different machine load from your 59m 30s; the figure that matters is the count, and it is 1.)
  2. 50 test result: lines = 50 running N headers. 0 error lines, 0 FAILED. Third cross-check: 37 Running <binary> + 13 Doc-tests = 50 ✅ — and on my log the line-anchored Running grep gave 37 with no splices, so the reconciliation holds by both counts.
  3. Non-canonical test result: lines 0; all-zero (empty target) results 15 — reported separately.
  4. 1673 passed + 0 failed + 45 ignored + 0 filtered out = 1718.
  5. running N header sum 1718 ✅ — matches (4) exactly.

Per-target spot checks match yours: eqoxide-http lib 259, eqoxide-ipc lib 59.

Attribute arithmetic re-derived independently: 644ba2c = 1698, a48760b = 1699, eeaa41d = 1708 → branch delta exactly 10 ✅, consistent with 1663 → 1673 at a constant offset of 35 between raw #[test]/#[tokio::test] attribute counts and executed totals. The base caveat is published, and the 1663 figure is explicitly attributed to #755's reviewer rather than claimed — that is the right way to carry a borrowed number.

The #755 interaction, re-measured rather than reasoned

Not taken on either of your words. On the merged head, #755's ago(300) code is present in observe.rs at three sites (:2109, :2364, :2500) and the guard test is green in the workspace run above (observe::tests::no_past_dated_net_health_stamp_is_taken_from_a_clock_other_than_the_one_that_reads_it ... ok). So the reported outcome — zero false positives from #755 — is correct.

The stated mechanism is now a third one, and it matters: it is not the old find('=') filter (gone), and it is not only that those statements name no STAMP_FIELDS (true, and it would be sufficient). All three sites are at lines above :3438, i.e. inside observe.rs's blind region — the scanner never reaches them at all. Two independent reasons for the same green, one of which is C1. That is exactly why "green" was not evidence of anything here, and why I would not accept a green run on this file as clearance for any future base move either, until C1 is fixed.


Three lists, kept separate

Verified myself, under compilation or by reading the current text:

  • The source list is a fixed four-file include_str! set with no directory walk; docs(#770): delete slot.rs's false backstop claim, name the canonical spelling in guild.rs's assert #775's guild.rs cannot arm the guard (observe.rs:3515-3520).
  • B3 fixed — the exactness claims are deleted, not reworded; ago's doc distinguishes pinned (== n) from wall (>= n) and says why.
  • B2 fixed — testkit.rs says "partially guarded" and points at a blind spot; the body no longer says "any net-health stamp assignment"; the limitations list leads with the shape limitation.
  • Round-2 N3 fixed — eqoxide-ipc/Cargo.toml names the three injection points.
  • Both stated costs appear under "What was NOT verified", including "I did not verify that the guard has no further misses beyond the nine shapes I measured". That disclosure is what made C1 and C2 findable without contradicting you, and it is the right shape for it.
  • Eleven probe shapes across three compiles, offender lists read from the assert, all files restored from copy-asides and md5sum-verified, worktree git status --porcelain empty.
  • The three cited eqoxide-net sites exist, are NetHealth stamps, and are outside the guard's reach.
  • The test-attribute delta of 10, and the five figures above.

Could not verify:

  • That the guard has no further misses beyond the shapes now measured between us. After C1 I would treat "no more" as unlikely rather than merely unproven.
  • Your 59m 30s wall time and per-target splits on your machine; I have my own run instead.

Taken on your word:

Nothing here asks for a lexer, a dataflow pass, or a bigger guard. C1 wants one assert!(!in_block, …) and, ideally, moving the // strip before the /* */ strip. C2 wants either the same receiver exemption on two more matchers or an honest assert message. The underlying change — pinning the clock, ago on the reading clock, the fixture work — I have no objection to and did not find a defect in this round. The defect is in the checker's account of itself.

…s, 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
@djhenry

djhenry commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Round 4 — C1 and C2 fixed. Round 3's twelve-cell table is void.

New head 5d9166b (base: this branch merged with 644ba2c; main has since advanced to a48760b and git merge-tree against it is conflict-free, so I did not merge again — carry that as the caveat on the figures).

Both blocking findings were real, and both were code defects in the guard, not prose. The C1 finding is the one that matters: I published a table grading an instrument that was not reading the code it graded. I have not tried to salvage any of it.


C1 — the scanner latched, and reported clean

Confirmed. statements() stripped block comments with a bare find("/*") before truncating at //, so a /* inside a line or doc comment latched the state forever — armed on eeaa41d by the route globs in the //! docs. Your coverage table (44/4913, 660/1346, 0/854, 210/210 → 914/7323 ≈ 12.5%) is recorded in the PR body as your measurement, attributed.

Fixed structurally, not by reordering the two branches: strip_to_code is now a character-level state machine (line comments, nesting block comments, strings with escapes, raw strings with # counts, char-literal vs lifetime), and literal contents are dropped so a // inside a string cannot truncate a line either.

I also corrected my own first draft of that code comment, which claimed "// is tested BEFORE /*: that ordering is the whole C1 fix." It is not — with a character-level scanner the branch order is irrelevant, since // and /* differ in their second character. The fix is that the Line state skips to end-of-line. Same defect class as the round: a mechanism claim reasoned rather than measured.

Beyond what was asked — the two controls are permanent, not ad-hoc:

  1. assert!(ended_clean, …)strip_to_code reports whether it finished in Code/Line. A real source file never ends inside a comment or a string.
  2. A GUARD_REACH_SENTINEL_<FILE> const at the end of each scanned file, asserted visible in that file's stripped output. The test binds all four by value in a _sentinels_must_exist tuple, so deleting a sentinel is a compile error, not a silent weakening.

Not tier 1 — a scanner that reaches every line is still a scanner — but the specific bad state, "the instrument silently stopped early", is no longer representable without a red test.

assert!(ended_clean) mutation, both directions

Copy-aside → mutate → measure → restore from the copy → md5sum-verify. No git checkout/restore/stash at any point.

arm result
head 5d9166b, controls in place, clean tree test result: ok. 1 passed; 0 failed … finished in **0.06s**, EXIT=0
legacy line stripper restored, controls kept REDpanicked at crates/eqoxide-http/src/observe.rs:3563:13:
observe.rs: 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)
test result: FAILED. 0 passed; 1 failed

The RED arm is the load-bearing one: it proves the corpus latches today, so the control is live rather than vacuous. (0.06s vs the round-3 guard's 0.01s is itself consistent with reading ~7,300 lines instead of 914.)

Run E — the silent blindness, demonstrated

Legacy stripper and both controls removed, with 12 identical violations injected at three depths in all four files:

testkit.rs:35:  h.last_packet = std::time::Instant::now() - std::time::Duration::from_secs(30)
testkit.rs:109: h.last_packet = std::time::Instant::now() - std::time::Duration::from_secs(30)
testkit.rs:139: h.last_packet = std::time::Instant::now() - std::time::Duration::from_secs(30)

3 of 12. Nine invisible — and had the three testkit.rs cells not existed, the test would have said ok.


Re-measured from scratch, with a reach control at every cell

Probes are now injected programmatically at three depths in each of the four scanned files (top / mid / immediately before the sentinel), and every hit is reported with file and line, so the line number is the reach evidence.

Reach control (run A) — 12/12:

file depth 1 depth 2 depth 3
observe.rs :1864 :3731 :4801
lib.rs :1071 :1210 :1308
combat.rs :362 :589 :856
testkit.rs :35 :109 :139

All three non-testkit rows would have been 0/3 under the round-3 scanner — measured in run E, not reasoned.

Shape table (deep shapes at observe.rs:4841–4852, i.e. in the region the old scanner could not see at all):

# shape violating (run A) corrected (run C)
P1 canonical inline h.last_packet = ago(30); RED :4841 GREEN
P2 rustfmt-wrapped assignment RED :4842 GREEN
P3 aliased via a local not caught — documented blind spot GREEN
P4 h.last_tick = wall.ago(30) RED :4847 GREEN
P5 struct-literal NetHealth { last_datagram: ago(30), .. } RED :4848 GREEN
P6 earlier = on the same line RED :4849 GREEN
P7 true-negative control Some(c.ago(3)) correctly not flagged not flagged
P8 Instant::now() - Duration::from_secs(60) RED :4851 GREEN
P9 Instant::now().checked_sub(…) wrapped RED :4852 GREEN
P10 C2 control c.now().checked_sub(…) correctly not flagged not flagged

Run A: FAILED. 0 passed; 1 failed (exit 101), 19 offenders — the 12 reach probes plus exactly 7 deep shapes, nothing else.
Run C (every form corrected, at every depth): ok. 1 passed; 0 failed … 0.04s (exit 0), zero offenders.


C2 — the matcher flagged the correct clock

Confirmed, including that it banned the body of HealthClock::ago (crates/eqoxide-ipc/src/lib.rs:436-440). now() now goes through the same receiver_is_the_reading_clock test as ago( before the -/.checked_sub lookahead. P10 is the regression cell, placed at the deep end of observe.rs so the reach control covers it.

The falsified sentence — "there is no case where the flagged form is the right one" — is deleted, not reworded, from the guard doc, from eqoxide-ipc, and from the PR body. The doc now states the opposite and names self.now().checked_sub(…) as a correct-but-flagged form.

The probe rule is now in the guard's doc

Written into no_past_dated_net_health_stamp_…'s doc under "How to probe this guard, and why it is written down", including the record that round 3's table was void and why — so the next person inherits the failure mode, not just the rule.


Non-blocking, all taken

N1 — measured, not asserted. The exhaustive-sounding testkit.rs sentence is narrowed. Both brace shapes injected at all twelve locations alongside a positive control in the same function:

injected at 12 locations flagged
h.last_packet = Instant::now() - …from_secs(32); (positive control) 12 / 12
h.last_tick = { Instant::now() - …from_secs(30) }; 0 / 12
h.last_datagram = match 1 { _ => Instant::now() - …from_secs(31) }; 0 / 12

The control firing at every location is what makes the zeros mean missed rather than not reached. The //-in-a-string miss is closed by the new lexer (literal contents are dropped).

N2 — restated, and my count was wrong. Round 3 justified the now() widening by pointing at "three" rustfmt-wrapped NetHealth sites in eqoxide-net. Re-enumerated by sweeping that crate for all eight stamp fields against all three past-dating spellings: there are sevengameplay.rs:1569, 1831, 1835, 1839 and transport.rs:1885, 1914, 2679. And none of them justifies the widening on its own terms: they are all in eqoxide-net, which this guard cannot scan and never will (include_str! reaches only eqoxide-http's own files), and if it could, all seven would be false positives — each builds its NetHealth via Default (checked at every one of the seven), so it past-dates from the wall clock and reads back on the wall clock, which is exactly the property the guard requires. What survives is the methodological result — grep the concept, not the field name — plus a smaller, real benefit: the spelling is now caught inside the four scanned files, where today it does not occur. The guard's doc says this in those words.

N3 — documented. empty_state_with_net_health hands the caller's Arc through and callers build theirs with NetHealth::default(), silently opting out of the tier-1 fixture guarantee. It now says so, and names crates/eqoxide-net/src/transport.rs:2679 as a live caller that is correct precisely because the clock is the wall clock.

#755 clearance, re-derived under a scanner that reaches

Agreed that the previous green did not carry. Re-derived: the ago(300) sites at observe.rs:2109, :2364, :2500 are all inside the old blind region and all now read (bracketed by the observe.rs reach column at :1864 and :3731). None is an offender, and structurally so rather than incidentally: they stamp tick_stamped/asset_sync_slot and name no STAMP_FIELDS entry, so they are outside the guard's subject matter entirely.


Five figures — cargo test --workspace --locked --no-fail-fast

  1. one Finished line: `test` profile … in 25m 08s (attests the build only)
  2. 50 test result: lines matching 50 headers; all ok; zero FAILED, zero error, zero panicked at
  3. non-canonical 0; all-zero (empty targets) 15
  4. 1673 passed + 0 failed + 45 ignored + 0 measured = 1718
  5. headers sum to 1718 ✅; EXIT=0 (recorded, not relied upon)

Unchanged from round 3, as expected — round 4 added no tests. Caveat carried: measured on this branch merged with 644ba2c, not a48760b.

A measurement error I made and caught here: my first pass reported 16 all-zero summaries, because grep "0 passed; …" matches inside 10 passed; …. Correct figure 15. Same defect class as everything else in this round.

The workspace run predates the N1/N2/N3 doc edits. Those are comments, which the scanner strips — but rather than rest on that argument, the final tree was re-run: -p eqoxide-http --locked --no-fail-fast, Finished in 3m 21s, test result: ok. **259 passed; 0 failed**, EXIT=0.

Files restored from copy-aside after every mutation, md5sum-verified, zero probe residue; worktree and the shared checkout both clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cast_empty_gem_is_409_and_queues_nothing flakes to 503 under load — require_live_session guards on wall-clock age

1 participant