fix(#827): take zone_cross's verdict and its grid from one call, not two slots - #840
Conversation
…two slots `ActionLoop::resolve_zone_cross` took PERMISSION from the zone-asset state (`zone_assets::usability`) and the GRID from `self.collision`, a separate shared slot, under a second lock. Nothing coupled them, and `begin_zone_load` empties the collision slot BEFORE it publishes `Pending`, so a zone change landing between the two reads paired a usable verdict with an emptied slot. The region lookup was then `None` for want of a grid and the `(None, None)` arm published `no_path` / `zone_line_not_in_map` — a reason documented as "the locally loaded zone geometry has no matching WLD zone-line (DRNTP) trigger region", i.e. a confident claim about the contents of a map that was never opened. #829's reviewer constructed that end state directly and got exactly that string. It now calls `zone_assets::usable_collision` — the single accessor #803/#821 introduced for `/v1/observe/zone_exits` — whose `Ok` arm carries the `Arc<Collision>` the `Ready` state owns. The `Arc` is cloned out so the zone-asset lock is not held across the lookup, and the resolution below operates on a non-optional `Collision`: there is no second slot left to straddle and no `Option` left to be `None` for want of a grid, so reaching the `(None, None)` arm now means the region map really was read. That is a shape change, not a guard: the bad pairing is no longer spellable in this function. It does NOT make the two-slot shape unrepresentable in general (#833 asks for that) — the accessor is still a convention any caller can decline to use. Regression test `zone_cross_answers_from_the_grid_its_own_gate_blessed_827` covers both halves of the acceptance bar: an emptied slot under a usable verdict must not answer `zone_line_not_in_map` (it walks to the line the blessed grid carries, or reports #815's `region_data_*`), AND a genuinely loaded region map that lacks the region must still answer exactly that — including with the slot emptied, where the reason is now truthful because it came from the grid that was read. `docs/http-api.md`: the "Still not covered, on `zone_cross` only" paragraph and the `zone_line_not_in_map` reason row both documented this gap as open; leaving them would be a false claim in a tracked file. Both rewritten, with the residual-caveat cross-reference in the `region_data_*` row dropped. NOT measured, and not claimed: whether a real zone change actually interleaves that way. It was never measured before this change either — the construction (here and in #829's review) sets the end state directly. Closes #827 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
Independent review — PR #840 (fix #827)Reviewed in my own worktree at What I verified at source (not taken from the PR body)The three load-bearing structural claims all hold, and hold for the reasons stated:
MeasurementsStreams captured separately, Baseline (unmutated branch). Compile sentinel: exactly one M1 spot-check, applied at the CALL SITE (verdict still from MUST-FIX (blocking)B1 —
|
Review round 1 on #840, both blocking findings, both doc-comment only. No executable code changes. B1 - action_loop.rs, the #827 test's rustdoc called row 4 "the sharpest of the four ... because it now comes from a map that was read". Measured false in review: with the M1 mutation applied at the call site and rows 1-3 removed, row 4 alone is GREEN (1 passed; 0 failed; 382 filtered out). Pre-fix, an emptied slot left `located` and `region_absent` both None and the (None, None) arm published the same string from no map at all, so row 4 cannot attribute its outcome to the grid it was answered from. Rewritten to name rows 1-2 as the measured #827 discriminators and to say what rows 3-4 actually pin (they assert the reason is still produced, so a fix that suppressed it rather than re-sourcing it fails there - stated as what the rows assert; no suppression mutation was run). B2 - observe.rs, this PR falsified that file's enumeration of usability()'s non-test consumers: `action_loop.rs` no longer calls it directly. Worse, a re-verifying `git grep 'usability('` still matches that file, because the only remaining hit is #827's own test asserting its premise - a scanner whose reach no longer matches the claim resting on it, failing silently toward "still true". The list is now three direct consumers plus `resolve_zone_cross` one call indirect through `usable_collision`, whose first statement calls `usability` and returns its verdict as Err - so the "stop zone-crossing" consequence still holds, and now says by which path. The count history is spelled out rather than swapped: it read three, was corrected to four when `move_api.rs` was found missing, and is three direct again for an unrelated reason - not because that omission returned. N4 - the same bullet cited `observe.rs:1554` for a call now at `:1574`. Pre-existing and non-blocking, fixed in passing since the bullet is being rewritten anyway. Verified: test -p eqoxide-net -p eqoxide-http --locked --no-fail-fast, streams captured separately. 4 `running N tests?` headers vs 4 `test result:` lines (equal); 0 non-canonical result lines; 0 targets on the full `0 passed; 0 failed; 0 ignored` triple; 651 passed + 0 failed + 3 ignored + 0 filtered = 654 = header total 654. eqoxide-net lib 383 and eqoxide-http lib 268, both unchanged from round 1, as a doc-only diff should be. 0 FAILED, 0 panicked at, 0 stderr error/warning lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV
Round 2 — both blocking findings addressed, doc/comment only (
|
Closes #827.
What was wrong
ActionLoop::resolve_zone_crosstook permission from one place and the grid from another:zone_assets::usability; if usable, it fell through;self.collision— a separate shared slot.Nothing coupled the two reads, and
zone_assets::begin_zone_loadwrites*collision_slot = Nonebefore it publishes
Pending. A zone change landing between (1) and (2) therefore paired ausable verdict with an emptied slot; the lookup was
Nonefor want of a grid; and the(None, None)arm published
no_path/zone_line_not_in_map— a reasondocs/http-api.mddefines as "thelocally loaded zone geometry has no matching WLD zone-line (DRNTP) trigger region". That is a
confident claim about the contents of a map the client never opened, which an agent has no second
channel to check. #829's reviewer constructed that end state directly and got exactly that string
(#827 comment 1).
What changed
resolve_zone_crossnow callszone_assets::usable_collision— the single accessor #803/#821introduced for
/v1/observe/zone_exits— whoseOkarm carries theArc<Collision>that theReadystate owns. TheArcis cloned out so the zone-asset lock is not held across the lookupand publish, and the whole resolution below operates on that one value:
self.collisionis no longer read anywhere in this function.Why the single-accessor shape is stronger than a guard
A guard says "check before you read"; it can be satisfied and then bypassed, and the compiler is
indifferent. Here the check is the read:
Option.Ready'scollisionfield is not optional, sousable_collision'sOkarm cannot hand back "usable, but no grid". Inside the resolution thereis no
Option<Arc<Collision>>left to beNone— so(None, None)can only be reached with aregion map in hand, and the reason it publishes is once again a statement about data that was read.
of one mutex. A zone change cannot land "between" them, because there is no between.
read. The failure was invisible at the call site. The new shape makes the wrong pairing something
you would have to go out of your way to write — reintroducing it means deliberately fetching a
grid the function was already handed (which is exactly mutation M1 below, and it now REDs).
What this does not do: it does not make the two-slot pattern unrepresentable in the language, or
anywhere else in the codebase. It removes it from
resolve_zone_cross. See the #833 section.docs/http-api.md— the paragraph #829 left behind#829 deliberately documented this bug as still open. Leaving that text after fixing it would itself
be a false claim in a tracked file, which is this repo's most common defect.
BEFORE (lines 615-626):
AFTER:
Two more doc edits in the same region, both for the same reason:
zone_line_not_in_mapreason row carried a**One caveat on "loaded" (#827):**clausesaying the reason "is also what you get when the client held no collision grid at all" and that
it "usually — not always" means the map was read. That is now false; replaced with
**"loaded" is now literal (#827):**and the reason's actual meaning.region_data_*row referred to the previous row as "(previous row, with its own residualcaveat)". The residual caveat is the one just removed, so the parenthetical is now dangling;
trimmed to
(previous row).Source comments updated for the same reason: the
(None, None)arm's block explaining that case (b)was "NOT fixed here", the
#600gate paragraph, the#815one-guard paragraph, and thezone_assetsfield doc onActionLoop.Test
zone_cross_answers_from_the_grid_its_own_gate_blessed_827(ineqoxide-net). It publishes bothslots exactly as
finish_zone_loaddoes in production, then — for the #827 rows — empties onlythe collision slot, which is the state
begin_zone_loadleaves behind between its two writes. Thezone-asset verdict is untouched and genuinely usable (asserted as a premise, so a row that refuses is
refusing at the region lookup and not at the #600 gate). Honour the premise, break the conclusion.
emptied_slot_region_presentzone_line_boxfor the requested indexno_pathemptied_slot_region_data_failedErr(Missing)no_path/region_data_missingloaded_lacks_region_both_slotsno_path/zone_line_not_in_mapemptied_slot_loaded_lacks_regionno_path/zone_line_not_in_mapRows 3 and 4 are the acceptance bar's second half — the one that is usually dropped. A "fix" that
simply stopped emitting
zone_line_not_in_mapwould pass rows 1-2 and fail these. Row 4 is thesharpest of the four: the slot is emptied and the answer is still
zone_line_not_in_map, becauseit now comes from a map that really was read. The fix removes the falsehood, not the reason.
Mutation table
All production mutations are at the call site in
resolve_zone_cross, never inside a callee'sbody (a body-wrap cannot distinguish "this branch is dead" from "the predicate is false"). Each was
applied to a
cp -pcopy of the file and reverted from that copy with anmd5sumcheck. Counts arethe
eqoxide-netlib target fromrbuild … test -p eqoxide-net --locked --no-fail-fast.usable_collision, but the lookup re-readsself.collision(`let guard = self.collision.read().unwrap(); … guard.as_ref().and_then(left: Some("zone_line_not_in_map"),right: Some("region_data_missing"). That is the acceptance sentence, measured.zone_line_box(…)toflat_below(-1000.0)(no zone-line region)(None, Some(absent))arm publishesSome("zone_line_not_in_map")instead ofSome(absent.as_str())(row 1 deleted so the loop reaches row 2)zone_cross_reports_an_unread_region_map_as_such_never_as_a_map_data_gap. Row 2 is not passing vacuously.usable_collisioncall; grid from a second, independentusable_collisioncall under a fresh lock of the same slotErrmid-zone-change), but it cannot reproduce #827, which is what this test constrains.M1 is the row that matters: the exact shape #833 measured as undetected at 382/0 is now RED.
Does this close #833's hole?
Yes, for the hole as filed — and it is the option #833 asked for. #833 is about
resolve_zone_crosssamplingregion_data_absent()and the zone-line lookup from one read guardover
self.collision, a deliberate choice that the reviewer's M6 mutation (split it back into two.read()s) removed with the target staying green at 382/0.That guard no longer exists.
resolve_zone_crossdoes not lockself.collisionat all; both factsare method calls on one owned
Arc<Collision>. There is no guard left to split, and no second.read()to add — which is #833's option 1 ("return both facts from a single accessor that owns thelock"), reached by taking the grid from the accessor that already owns the verdict. And where #829's
M6 survived, my M1 — the nearest writable equivalent, sourcing the grid from
self.collisionagain— now REDs.
Two honest limits. (a) This is not a language-level ban: nothing stops a future edit from adding
a
self.collision.read()back into this function. What has changed is that doing so is nowgratuitous (the grid is already in scope) and caught. (b) It says nothing about the other readers
of the shared collision slot elsewhere in
action_loop.rs. If #833 is read as the general property,it is not closed; if it is read as filed — the guard in
resolve_zone_cross— it is. I have notclosed the issue; that is the reviewer's/owner's call.
Five-figure log standard
rbuild <worktree> test --workspace --locked --no-fail-fast, stdout and stderr captured toseparate files, run against the exact committed tree.
Finished `test` profile [unoptimized + debuginfo] target(s) in 41m 32s— exactly one occurrence. Run completion is establishedseparately: the process exited, and the final line of stdout is a complete
test result:line.55lines matching^running [0-9]+ tests?$(singular
testincluded) vs55lines matching^test result:. Equal.0— every one of the 55 matchestest result: (ok|FAILED)\. N passed; N failed; N ignored; N measured; N filtered out; finished in.0 passed; 0 failed; 0 ignored:14.passed 1832 + failed 0 + ignored 47 + filtered 0 = 1879, and therunning Nheaders sum to 1879. Equal.Result: 1832 passed, 0 failed, 47 ignored, 0 filtered.
0occurrences ofFAILED,0ofpanicked at,0lines startingerrorand0startingwarningin stderr.Count delta, reconciled by name. One test was added and none removed.
eqoxide-net's lib targetreads
383 passed; 0 failed; 0 ignoredin this run, andtest action_loop::tests::zone_cross_answers_from_the_grid_its_own_gate_blessed_827 ... okappears inthis run's stdout — the name is confirmed
ok, not inferred from the total. The pre-change figurefor that target is 382, which I did not measure on
origin/maindirectly; I measured it as the382 passed / 1 failedof mutation runs M1/M3 (383 tests, mine failing) on this branch, and itmatches the
382/0#833 records for the same target. The workspace baseline of 1831 is thereforearithmetic, not a measured figure — I did not run
origin/main's suite.scripts/check-no-local-detail.sh: exit 0 ("no forbidden patterns in tracked files").NOT verified, and not claimed
between two writes inside the net thread; a live client cannot be steered into it on demand, and a
live run that didn't hit it would be an existence proof over one trajectory, not evidence about
a race. (Per the verification hierarchy, live validates premises, never a "cannot" claim.)
change and after it. Both fix(#815): stop reporting a map-data GAP for a region map the client never read #829's reviewer and this test reach the end state by construction rather
than by racing
begin_zone_load. The fix makes the question moot forzone_cross; it does notanswer it. Do not upgrade this into "observed in the field".
function's resolution path, because there is no optional grid and no second lock there.
zone_line_not_in_mapis only ever said about a mapthat was read") is discharged by the shape — the grid is non-optional at the emit site — rather
than by a property over inputs. If a reviewer thinks the shape argument is weaker than I read it,
that is the finding to push on.
zone_assets.rswas deliberately not touched (PR fix(#826): make collision() and usability() fail to compile together on a new state variant #837 is in review against it). Oneobservation for whoever owns that file, non-blocking: the
usabilitydoc lists the four consumersthat "go through it (verified by grep)".
drain_zone_crossstill does —usable_collisiondelegates its verdict to
usability— but the grep that verifies the list would now have toinclude
usable_collisionto find this caller. The claim is still true; the stated verificationmethod for it is one call-site indirect.
self.collisionreaders inaction_loop.rsare unchanged — the standing auto-cross,combat line-of-sight and swim probing all still read the slot directly. They drive physical
movement rather than publishing an observable route claim (the reasoning
usability's own docgives), and I did not re-audit them.
Ruling:
Closes #827, not merely "addresses"The issue's acceptance is "a
zone_crosswhose collision slot is empty while the zone-asset verdictis usable must not report
zone_line_not_in_map, paired with a still-green test that agenuinely-loaded region map lacking the region still does." Both halves are met and both are
mutation-discriminating (M1/M5/M3 for the first, M4 and rows 3-4 for the second), and the construction
the issue names is no longer expressible in the function.
Closes #827is in the commit message.🤖 Generated with Claude Code
https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV