From e73a0e1b4e6e2a1486b2244d3b22b7c95dbe80c2 Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 02:33:03 -0400 Subject: [PATCH 1/7] =?UTF-8?q?fix(#766):=20retire=20nav=5Flocal=20with=20?= =?UTF-8?q?the=20goal=20=E2=80=94=20a=20zone=20change=20no=20longer=20publ?= =?UTF-8?q?ishes=20the=20previous=20zone's=20fine-tier=20verdict?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Walker::reset_for_zone_change` did not clear `NavStatus.local`, so between the reset and the next walker tick that reached `resolve_goal` with no goal, a reader of `GET /v1/observe/debug` could get `nav_local: {"state":"no_way_through", ...}` beside `nav_state: idle` / `nav_reason: zoned` — a verdict about threading a corridor in the zone just left, computed against a collision grid that no longer exists. Fixed at the single writer #732 introduced: `NavStatus::retire_to_idle` now retires `local` along with `goal`, `blocked_goal`, `blocked_frontier` and `tier`, so all six routes to `idle` are uniform and the exhaustive destructure keeps forcing the next field to be decided. The explicit `s.local = None;` in `CommandState::stamp_new_goal` is deleted as redundant. Non-`idle` states are untouched, so #382's deliberate keep-the-verdict-on-`blocked`/`no_path` design is preserved (`stop_nav_blocked` never publishes `idle`). Also adds the missing `self.local_planner.cancel()` to `reset_for_zone_change`, beside the coarse `planner.cancel()` that was already there. Concluded an oversight rather than a considered asymmetry from git archaeology, and scoped honestly in the code comment: no production route was found on which a stale fine reply is APPLIED in the new zone, but the armed `pending` slot does make `post_if_idle` a no-op, silently refusing the new zone's first fine plans until the stale reply drains. Five regression tests, four of them mutation-checked; see the PR body for the assert file:line and left/right values. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-command/src/nav.rs | 80 +++++++++++++++- crates/eqoxide-http/src/observe.rs | 43 +++++++++ crates/eqoxide-ipc/src/lib.rs | 28 +++++- crates/eqoxide-nav/src/walker.rs | 130 +++++++++++++++++++++++++- crates/eqoxide-net/src/action_loop.rs | 56 +++++++++++ docs/http-api.md | 9 ++ 6 files changed, 336 insertions(+), 10 deletions(-) diff --git a/crates/eqoxide-command/src/nav.rs b/crates/eqoxide-command/src/nav.rs index 3056c7e9..e5cdce44 100644 --- a/crates/eqoxide-command/src/nav.rs +++ b/crates/eqoxide-command/src/nav.rs @@ -161,11 +161,12 @@ impl CommandState { // throwaway `probe_route_len` field: E0027 in `eqoxide-ipc`, `eqoxide-command` compiled // clean, and `request_stop` published the field beside `state: "idle"`. // - // Behaviour-identical: `retire_to_idle` writes a strict subset of what this list writes for - // `idle` (it deliberately keeps `local`, which is cleared just below), and both `idle` call - // sites already pass `goal: None`. + // Behaviour-identical: `retire_to_idle` writes exactly what this list writes for `idle`, and + // both `idle` call sites already pass `goal: None`. (#766 removed the last difference — the + // explicit `s.local = None;` that used to sit here because `retire_to_idle` kept `local`. + // `retire_to_idle` now retires `local` itself, so keeping the line would only mean two + // writers for one field and a comment that has to explain the overlap.) if new_state == "idle" { - s.local = None; s.retire_to_idle(reason); return s.goal_id; } @@ -687,4 +688,75 @@ mod tests { "the newer goal's coordinates must not be cleared by the older request's backstop"); assert_eq!(s.reason, None); } + + /// **#766 (agent-honesty): the FINE tier's verdict is a per-goal fact, so every command-side + /// route to `idle` must retire it too.** + /// + /// #766 was reported against the zone change, but as with #732 the defect was one writer up: + /// `NavStatus::retire_to_idle` deliberately KEPT `local`, so of the six documented routes to + /// `idle` the two that reached it without a `Walker::clear_local_plan` on the same tick leaked + /// it — the zone change (pinned in `eqoxide-nav`/`eqoxide-net`) and this crate's + /// `zone_cross_dropped_unhandled` backstop. `stopped` and `goto_cancelled` did NOT leak it, + /// because `stamp_new_goal` carried its own explicit `s.local = None;` line; this PR deleted + /// that line as redundant once `retire_to_idle` owns the field, so those two routes now depend + /// on the shared writer and are pinned here for that reason. + /// + /// `no_way_through` is the fixture verdict because `eqoxide_http::observe` filters `threaded` + /// out — an unhealthy verdict is the only kind that reaches an agent at all. + /// + /// **Mutation check:** delete `*local = None;` from `NavStatus::retire_to_idle` + /// (`crates/eqoxide-ipc/src/lib.rs`) → all three cases go RED. Restore the deleted + /// `s.local = None;` in `stamp_new_goal` on top of that mutation and the first two go green + /// again while `zone_cross_unhandled` stays RED — which is the split this test exists to show. + #[test] + fn every_command_side_retirement_retires_the_fine_tiers_verdict_766() { + let stale = || eqoxide_ipc::NavLocal { + state: "no_way_through".into(), reason: "search_closed".into(), + stuck_ticks: 7, plan_us: 1234, + }; + // The two `stamp_new_goal` routes. NOTE the ordering: `request_goto` itself goes through + // `stamp_new_goal`'s NON-idle branch, which clears `local` — so the verdict is planted + // AFTER it, or the post-condition would be met before the code under test ever runs. + for (label, act) in [ + ("stop", Box::new(|cs: &CommandState| { cs.request_stop(); }) as Box), + ("cancel_goto", Box::new(|cs: &CommandState| { cs.request_cancel_goto(); }) as Box), + ] { + let cs = CommandState::default(); + cs.request_goto((2216.0, 579.0, -113.0)); + cs.nav.nav_state.lock().unwrap().local = Some(stale()); + assert_eq!(cs.nav.nav_state.lock().unwrap().local, Some(stale()), + "{label}: PREMISE — the fine tier's verdict really is loaded, so the assertion \ + below cannot pass on `NavStatus::default()`"); + + act(&cs); + + let s = cs.nav.nav_state.lock().unwrap(); + assert_eq!(s.state, "idle", "{label}: the route ends at idle"); + assert_eq!(s.local, None, + "{label}: #766 — the fine tier's last word is its verdict on threading toward the \ + goal that just ended. Beside `idle` it is a live-looking answer about work that is \ + over, and after a zone change it describes a corridor in a world we have left."); + } + + // The #725 backstop: a one-shot cross drained and never answered. Same ordering hazard — + // `request_zone_cross` also stamps through the non-idle branch — so the verdict is planted + // while the ticket is HELD, which is also exactly when the fine tier would have produced + // one (the walker keeps ticking while the drain runs). Planting it does not touch + // `state`/`reason`/`goal_id`, so the ticket's `moved` guard still sees an unanswered request. + let cs = CommandState::default(); + cs.request_zone_cross(30); + let ticket = cs.take_zone_cross().expect("the request was queued"); + cs.nav.nav_state.lock().unwrap().local = Some(stale()); + assert_eq!(cs.nav.nav_state.lock().unwrap().local, Some(stale()), + "zone_cross_unhandled: PREMISE — the verdict is loaded before the backstop fires"); + drop(ticket); + + let s = cs.nav.nav_state.lock().unwrap(); + assert_eq!(s.state, "idle"); + assert_eq!(s.reason.as_deref(), Some(NAV_REASON_ZONE_CROSS_UNHANDLED), + "PREMISE: the backstop really did fire — otherwise the `local` assertion below would be \ + about a row nothing retired"); + assert_eq!(s.local, None, + "#766: the sixth route to `idle` retires the fine verdict like the other five"); + } } diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index 776120cb..07e4f27f 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -2943,6 +2943,49 @@ mod tests { `idle` to the goal it asked for; only the per-goal FACTS are retired"); } + /// **#766 (agent-honesty) — the OBSERVER half of the fine tier's retirement.** + /// + /// The sibling of the test above, for the field #732 left standing. `nav_local` is read off the + /// same cloned `NavStatus` and passed through exactly one filter — `.filter(|l| l.state != + /// "threaded")` — so an UNHEALTHY verdict reaches the response body verbatim, and an unhealthy + /// verdict is the only kind an agent can ever see. That makes `no_way_through` the right fixture + /// and makes the first half of this test a real premise: it asserts that the planted verdict + /// genuinely publishes, so the `null` below is the retirement's doing and not the filter's. + /// + /// Mutation check: delete `*local = None;` from `NavStatus::retire_to_idle` + /// (`crates/eqoxide-ipc/src/lib.rs`) → the `nav_local` assertion goes RED with the previous + /// zone's `no_way_through` object in the diff. + #[tokio::test] + async fn debug_publishes_no_nav_local_once_the_goal_is_retired_to_idle_766() { + let state = empty_state(); + { + let mut s = state.nav.nav_state.lock().unwrap(); + s.goal_id = 4; + s.state = "navigating".into(); + s.local = Some(eqoxide_ipc::NavLocal { + state: "no_way_through".into(), reason: "search_closed".into(), + stuck_ticks: 7, plan_us: 1234, + }); + } + let v = debug_json(state.clone()).await; + assert_eq!(v["nav_local"]["state"], serde_json::json!("no_way_through"), + "PREMISE: an un-retired UNHEALTHY fine verdict is published — the harm is reachable by a \ + reader of GET /v1/observe/debug, not merely resident in memory"); + assert_eq!(v["nav_local"]["stuck_ticks"], serde_json::json!(7), + "PREMISE: and the whole object comes through, not just a state string"); + + // The zone-change retirement, which is where #766 was reported. + state.nav.nav_state.lock().unwrap().retire_to_idle(Some("zoned")); + + let v = debug_json(state).await; + assert_eq!(v["player"]["nav_state"], serde_json::json!("idle")); + assert_eq!(v["player"]["nav_reason"], serde_json::json!("zoned")); + assert_eq!(v["nav_local"], serde_json::json!(null), + "#766: `no_way_through` beside `idle`/`zoned` describes a corridor in the zone the \ + reader has LEFT, computed against a collision grid that no longer exists — the fine \ + tier's verdict is about threading toward a goal, so it retires with the goal"); + } + /// #471 (agent-honesty): the server placed two Mobs (consecutive spawn_ids, e.g. 526/527) at a /// byte-identical position; the wire disambiguates their names with a numeric suffix /// ("Geeda"/"Geeda00"), so in the name-keyed roster they survive as TWO entries. The observe diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index 9444860f..d5ca77d2 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -1426,6 +1426,23 @@ impl NavStatus { /// the same construction `AssetSyncState::slots()` uses in `eqoxide-ipc::asset_sync`. Note the /// weaker precondition: `NavStatus`'s fields are `pub` and read directly by several crates, so /// this pins the *retirement path* only. It does not stop other code writing the fields. + /// + /// **#766 moved `local` from KEPT to retired.** #732 left it standing with a `_keep_local` + /// binding on the grounds that the FINE tier is "a different tier" whose clearing + /// `Walker::clear_local_plan` owns. That reasoning holds for the tier's *machinery* and it still + /// does — this function does not touch `LocalPlanner`, and every NON-`idle` state keeps `local` + /// exactly as before, which is what preserves #382's deliberate keep-the-fine-verdict-on-`blocked` + /// design (`Walker::stop_nav_blocked` publishes `blocked`/`no_path`, never `idle`, so it does not + /// come through here). It does not hold for the published FIELD on an `idle` row: `NavLocal` is + /// the fine planner's verdict on threading toward *the goal that just ended*, so it is a per-goal + /// fact by the same argument as `tier`. Two of the six routes left it standing (`zoned` — #766's + /// report — and `zone_cross_dropped_unhandled`); the other four already cleared it — `goal_dropped` + /// and `respawned` because `Walker::resolve_goal`'s no-goto branch calls `clear_local_plan()` on + /// the same tick before it retires, `stopped` and `goto_superseded` through an explicit + /// `s.local = None;` in `CommandState::stamp_new_goal`, now deleted as redundant. Both halves of + /// that four are RUN, not read: `eqoxide-nav`'s + /// `the_goal_dropped_route_already_cleared_the_fine_verdict_before_766` and `eqoxide-command`'s + /// `every_command_side_retirement_retires_the_fine_tiers_verdict_766`. pub fn retire_to_idle(&mut self, why: Option<&str>) { // The same writer-level guard as `Walker::set_nav_state_because` and // `CommandState::stamp_new_goal`: on `idle`, `nav_reason: null` is reserved for boot (#725). @@ -1438,10 +1455,12 @@ impl NavStatus { // Zeroing or bumping it here would break that correlation. goal_id: _keep_goal_id, goal, blocked_goal, blocked_frontier, tier, - // KEPT, deliberately — `local` is the FINE tier's own last word, an independent fact - // about a different tier (#382), and `Walker::clear_local_plan` owns clearing it. Wiring - // it in here would silently take that ownership away from #382's design. - local: _keep_local, + // #766: RETIRED, not kept. The fine tier's verdict is about threading toward the goal + // that is now over — a `no_way_through` published beside `idle`/`zoned` asserts something + // about a corridor in a zone we have left, computed against a collision grid that no + // longer exists. See the "#766 moved `local`" paragraph above for why this does not + // undo #382's ownership: only the `idle` row is touched, and only the published field. + local, } = self; *state = "idle".to_string(); *reason = why.map(str::to_string); @@ -1449,6 +1468,7 @@ impl NavStatus { *blocked_goal = None; *blocked_frontier = None; *tier = None; + *local = None; } } diff --git a/crates/eqoxide-nav/src/walker.rs b/crates/eqoxide-nav/src/walker.rs index e832faa9..05528f08 100644 --- a/crates/eqoxide-nav/src/walker.rs +++ b/crates/eqoxide-nav/src/walker.rs @@ -407,6 +407,22 @@ impl Walker { // coordinate space. Abandon it — applying it here would drive the character at a route // through a zone it is no longer in. self.planner.cancel(); + // #766: the identical sentence is true of the FINE plan in flight, and this line was + // MISSING. Not a considered asymmetry — archaeology: the coarse `cancel()` above predates + // the fine worker (it is in the zone reset at f2dce47^, before #382 existed), and #382's own + // diff added `local_planner.cancel()` beside the fine-plan drop at every OTHER site it + // touched — `clear_local_plan`, `stop_nav_blocked`, `resolve_goal`, `drive_teleport_detect` + // — while this reset, which already cleared `local_path`/`local_i`/`local_stuck_ticks` + // three lines up, was never revisited. No comment anywhere records a reason for the gap. + // + // HONEST SCOPE: I could not construct a production route on which a previous zone's fine + // reply is actually APPLIED in the new zone — every path from here to the next + // `local_planner.poll()` (which is gated on a non-empty `self.path`, cleared above) passes + // through a `clear_local_plan()` first. So this is defence in depth restoring #382's own + // pattern, not a leak I measured. What it does fix outright is the ARMED `pending` slot: + // `post_if_idle` is a no-op while one is in flight, so until the stale reply is drained the + // new zone's first fine plans are silently refused. + self.local_planner.cancel(); self.awaiting_first_plan = false; // SAY WHY (#725 review B1). A bare `idle` here is indistinguishable from "nothing was ever // requested" — and this is the line that runs on a SUCCESSFUL `/v1/move/zone_cross`, so the @@ -417,6 +433,11 @@ impl Walker { // (`nav_goal: [2216.87, 579.17, -113.25]` read in lfaydark, from the zone before it). // `NavStatus::retire_to_idle` also clears `tier` (no route committed → no per-route tier), // which is why the explicit clear that used to sit on the next line is gone: one owner. + // #766: and `local` — the fine tier's last verdict was about threading a corridor in the + // zone we just left, against a collision grid that no longer exists. Before this it was left + // standing until some LATER tick reached `resolve_goal` with no goal and called + // `clear_local_plan`; in between, `nav_local: {"state":"no_way_through", ...}` published + // beside `nav_state: idle` / `nav_reason: zoned`. self.set_nav_state_because("idle", Some(NAV_REASON_ZONED)); // Publish the cleared snapshot so no consumer keeps drawing the previous zone's state. // Position: None — the old zone's coordinates would be a confident wrong answer in the @@ -437,8 +458,14 @@ impl Walker { /// `reason` is the machine-readable WHY behind a terminal state. pub fn set_nav_state(&self, state: &str) { self.set_nav_state_because(state, None); } - /// Set the walker's state + reason. **Deliberately does NOT touch `local`** — the fine tier's - /// last word is an independent fact about a different tier (#382). + /// Set the walker's state + reason. **On any NON-`idle` state this deliberately does not touch + /// `local`** — the fine tier's last word is an independent fact about a different tier (#382), + /// and it is the evidence behind a terminal `blocked`/`no_path`. + /// + /// `idle` is the exception, and #766 made it one: an `idle` goes through + /// `NavStatus::retire_to_idle`, which retires `local` along with the rest of the finished goal's + /// facts. `idle` means the goal is over, so the fine tier's verdict about threading toward it is + /// over too. Nothing here touches the fine PLANNER — see `Walker::clear_local_plan`. pub fn set_nav_state_because(&self, state: &str, reason: Option<&str>) { // #725 review round 3, B1: enforce the `idle` row's universal at the WRITER, not per call // site. `nav_reason: null` on `idle` means exactly one thing — no nav request has been made @@ -2644,6 +2671,105 @@ mod tests { be reported with it, or success and 'your request was thrown away' are the same read"); } + /// **#766: a zone change must retire the FINE tier too — the published verdict AND the plan in + /// flight.** `reset_for_zone_change` cleared the coarse tier's every trace and the fine tier's + /// *walker-side* state (`local_path`, `local_i`, `local_stuck_ticks`) but left two things + /// standing: the published `NavStatus.local` (the field `/v1/observe/debug` serves as + /// `nav_local`) and `LocalPlanner`'s `pending` slot. + /// + /// **What this test measures, precisely.** It reads the row IMMEDIATELY after + /// `reset_for_zone_change` returns, with no intervening tick — which is the whole point. The + /// pre-fix code did eventually clear `local`, on some LATER tick that reached `resolve_goal` + /// with no goal and called `clear_local_plan`; the defect was the window in between, during + /// which a reader got `nav_local: {"state":"no_way_through"}` beside `nav_state: idle` / + /// `nav_reason: zoned`. So "the field is already retired at the instant the reset returns" is + /// the property, and an immediate read is what states it. This test does **not** measure how + /// WIDE that window was in wall-clock terms on a live client — see the PR body. + /// + /// Mutation check: delete `*local = None;` from `NavStatus::retire_to_idle` → the `local` + /// assertion goes RED. Delete `self.local_planner.cancel();` from `reset_for_zone_change` → the + /// `is_planning` assertion goes RED. They are separate lines in separate crates and each has its + /// own assertion here, so neither can ride on the other. + #[test] + fn a_zone_change_retires_the_fine_tiers_verdict_and_its_in_flight_plan_766() { + let col = open_plane(400.0); + let (mut w, nav, _intent, _view) = walker_with(col.clone()); + + // The previous zone's fine tier reached a verdict, and it is the UNHEALTHY kind — the only + // kind `observe.rs` publishes at all (it filters `threaded` out), so this is the shape a + // reader actually sees. + w.set_nav_local(Some(eqoxide_ipc::NavLocal { + state: "no_way_through".into(), reason: "search_closed".into(), + stuck_ticks: 7, plan_us: 1234, + })); + assert_eq!(nav.nav_state.lock().unwrap().local.as_ref().map(|l| l.state.clone()), + Some("no_way_through".to_string()), + "PREMISE: the observable field is genuinely loaded before the reset — otherwise the \ + post-condition below would be satisfied by the default row and prove nothing"); + + // …and a fine plan is genuinely in flight, posted the way `drive_walk` posts one. + let c = col.read().unwrap().as_ref().cloned().expect("PREMISE: the fixture has collision"); + assert!(w.local_planner.post_if_idle(crate::planner::LocalRequest { + gen: 0, // assigned by the planner + start: [0.0, 0.0, 0.0], goal: [20.0, 0.0, 0.0], + cell: 2.0, bound: 40.0, carrot_tol: 4.0, collision: c, + }), "PREMISE: the post succeeded, so there IS a plan to abandon"); + assert!(w.local_planner.is_planning(), + "PREMISE: the fine planner's `pending` slot is armed — without this the cancel \ + assertion below would pass on a planner that was never busy"); + + w.reset_for_zone_change(); + + // Read the row RIGHT HERE — no tick between the reset and this read. + let after_reset = nav.nav_state.lock().unwrap().clone(); + assert_eq!(after_reset.state, "idle"); + assert_eq!(after_reset.reason.as_deref(), Some(NAV_REASON_ZONED)); + assert_eq!(after_reset.local, None, + "#766: the fine tier's verdict is about threading a corridor in the zone we just LEFT, \ + computed against a collision grid that no longer exists — publishing it beside \ + `idle`/`zoned` tells the agent something false about the zone it is standing in"); + assert!(!w.local_planner.is_planning(), + "#766: and the fine plan in flight must be abandoned like the coarse one — while \ + `pending` is armed, `post_if_idle` is a no-op, so the NEW zone's first fine plans are \ + silently refused until the stale reply drains"); + } + + /// **#766, the OTHER walker route to `idle` — and it is here to MEASURE a claim, not to guard a + /// line.** The fix's rationale (in `NavStatus::retire_to_idle`'s doc comment and this PR's body) + /// says four of the six routes to `idle` already cleared `local` before #766, two of them — + /// `goal_dropped` and `respawned` — because `resolve_goal`'s no-goto branch calls + /// `clear_local_plan()` on the same tick, *before* it retires. That was read off the source, and + /// a mechanism claim read off the source is exactly the kind this project keeps getting wrong, + /// so it is run here instead. + /// + /// **Deliberately NOT mutation-pinned.** Deleting `*local = None;` from `retire_to_idle` leaves + /// this GREEN, because `clear_local_plan()` gets there first — that is the whole finding. Read + /// it as a measurement of the pre-existing behaviour and a regression guard on this route, not + /// as evidence for the fix. + #[test] + fn the_goal_dropped_route_already_cleared_the_fine_verdict_before_766() { + let (mut w, nav, _intent, _view) = walker_with(Arc::new(std::sync::RwLock::new(None))); + let gs = eqoxide_core::game_state::GameState::new(); + *nav.goto_target.lock().unwrap() = Some((10.0, 20.0, 3.0)); + w.set_nav_state_because("following", None); + w.set_nav_local(Some(eqoxide_ipc::NavLocal { + state: "no_way_through".into(), reason: "search_closed".into(), + stuck_ticks: 7, plan_us: 1234, + })); + assert!(nav.nav_state.lock().unwrap().local.is_some(), + "PREMISE: the verdict is loaded, and `following` is non-terminal so the tick below \ + genuinely reaches the retirement branch"); + + *nav.goto_target.lock().unwrap() = None; // the leader despawned + assert!(w.resolve_goal(&gs).is_none()); + + let s = nav.nav_state.lock().unwrap(); + assert_eq!(s.reason.as_deref(), Some(NAV_REASON_GOAL_DROPPED)); + assert_eq!(s.local, None, + "#766: this route was never the leaky one — `clear_local_plan()` runs on the same tick. \ + Measured here so the PR's four-of-six claim is not just read off the source."); + } + /// **#725 review round 3: the writer-level guard itself is pinned.** The test above is a /// PER-CALL-SITE assertion, and the round-3 review measured what that class of test cannot do: /// a brand-new reasonless `idle` on a path nobody wrote an assertion for (`perform_cross`'s diff --git a/crates/eqoxide-net/src/action_loop.rs b/crates/eqoxide-net/src/action_loop.rs index 77a38ba3..7e4d8372 100644 --- a/crates/eqoxide-net/src/action_loop.rs +++ b/crates/eqoxide-net/src/action_loop.rs @@ -5237,6 +5237,62 @@ mod tests { match this `idle` to the goal it issued. Only the per-goal facts are retired."); } + /// **#766 (agent-honesty), the sibling of the test above: `nav_local` must not survive a zone + /// change either.** #732 retired `nav_goal` and `nav_tier`; the fine tier's verdict (#382) was + /// left standing, so the same crossing that produced #732's measured `idle` + stale `nav_goal` + /// pair could also publish `nav_local: {"state":"no_way_through", …}` beside `nav_state: idle` / + /// `nav_reason: zoned` — an assertion about a corridor in the zone we have left, computed + /// against a collision grid that no longer exists. + /// + /// Driven through the REAL production hook for the same reason: `sync_zone_points` observing a + /// new `gs.world.zone_name` is what the packet path calls, and it is the only thing that reaches + /// `Walker::reset_for_zone_change` in production. `eqoxide-nav`'s + /// `a_zone_change_retires_the_fine_tiers_verdict_and_its_in_flight_plan_766` pins the walker + /// call directly (and the `LocalPlanner` cancel, which is not observable from here); + /// `eqoxide-http`'s `debug_publishes_no_nav_local_once_the_goal_is_retired_to_idle_766` pins the + /// JSON end. This one pins that the production path in between actually runs it. + /// + /// Mutation check: delete `*local = None;` from `NavStatus::retire_to_idle` + /// (`crates/eqoxide-ipc/src/lib.rs`) → the post-zone `local` assertion goes RED. + #[tokio::test] + async fn a_zone_change_retires_the_previous_zones_nav_local_766() { + let (mut al, nav, command, _collision, _za) = shared_nav_action_loop(); + let mut gs = GameState::new(); + gs.world.zone_name = "gfaydark".into(); + al.sync_zone_points(&gs); // settle `current_zone` so the next call is a real CHANGE + + let _goal_id = command.request_goto((2216.0, 579.0, -113.0)); + al.walker.set_nav_state_because("navigating", None); + // The fine tier's last word in the OLD zone. Planted after the transition for the same + // reason #732's `tier` is: otherwise the post-condition is satisfied by the default row. + // `no_way_through` specifically, because `observe.rs` filters `threaded` out — the + // unhealthy verdict is the only kind that reaches an agent at all. + nav.nav_state.lock().unwrap().local = Some(eqoxide_ipc::NavLocal { + state: "no_way_through".into(), reason: "search_closed".into(), + stuck_ticks: 7, plan_us: 1234, + }); + { + let s = nav.nav_state.lock().unwrap(); + assert_eq!(s.local.as_ref().map(|l| l.state.clone()), Some("no_way_through".to_string()), + "PREMISE: the field an observer reads really is loaded with the OLD zone's verdict"); + assert_eq!(s.state, "navigating", + "PREMISE: a NON-terminal, non-idle state — so the retirement under test is what \ + clears it, not some earlier transition"); + } + + gs.begin_zone_in(); + gs.world.zone_name = "lfaydark".into(); + al.sync_zone_points(&gs); + + let s = nav.nav_state.lock().unwrap().clone(); + assert_eq!(s.state, "idle", "a zone change ends navigation (#248)"); + assert_eq!(s.reason.as_deref(), Some(eqoxide_nav::walker::NAV_REASON_ZONED)); + assert_eq!(s.local, None, + "#766: the fine tier's verdict is a per-goal fact by the same argument as `nav_goal` \ + and `nav_tier` — when the goal is retired at the zone line, the verdict about \ + threading toward it goes with it"); + } + /// REAPER: a zone change while a buy is parked fires `Unconfirmed` for the stranded Sender and /// clears `pending_buy`, so a shop echo in the NEW zone can't mis-correlate it. Driven through the /// real `sync_zone_points` zone-change hook. diff --git a/docs/http-api.md b/docs/http-api.md index e8191727..0d4b5c9f 100644 --- a/docs/http-api.md +++ b/docs/http-api.md @@ -846,6 +846,15 @@ said. It is **`null` while the tier is healthy** (a complete fine route to its c > *goal* unreachable, so a tight doorway must never be able to tell an agent its destination does not > exist. Only the coarse planner, which closes the whole zone's frontier, may say `no_path`. +**`nav_local` is `null` on every `idle`, whichever `nav_reason` got you there** (#766) — `zoned`, +`goal_dropped`, `respawned`, `stopped`, `goto_superseded`, `zone_cross_dropped_unhandled`. The fine +tier's verdict is about threading toward *a goal*, so when the goal is retired the verdict goes with +it, exactly like `nav_goal` and `nav_tier` (#732). The `zoned` case is the sharp one: a `no_way_through` +left standing across a crossing describes a corridor in the zone you just left, computed against a +collision grid that no longer exists — and it is the only kind that publishes, because a healthy +`threaded` verdict is filtered out anyway. It **does** survive a terminal `blocked` / `no_path`, on +purpose: there it is the *evidence* behind the failure you are being told about. + **Why this field exists.** The fine tier is bounded *spatially* (a 40 u window) plus a deterministic node cap (#394 removed its old 150 ms wall clock, so its answer no longer depends on machine load), and until #382 it ran **inline on the network thread**, every nav tick — the last A* left on that thread, a From ef99227c42f436bede8c90996e99d948097cebee Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 03:53:17 -0400 Subject: [PATCH 2/7] fix(#766) review round 1: retract a false claim, and close the doc's universal for real RETRACTION. The previous commit on this branch (e73a0e1) says in its body, in the source comment it added to `Walker::reset_for_zone_change`, and in one test assertion message, that the added `local_planner.cancel()` "does fix outright" the armed `pending` slot -- that `post_if_idle` is a no-op while a stale reply is in flight, so the new zone's first fine plans are silently refused -- and it attributes that to mutation M2. THAT IS FALSE. `post_if_idle` has one production call site, in `drive_walk`, behind `!self.path.is_empty()`. `self.path` is made non-empty at exactly two sites (`apply_plan`'s `Route` and `Exhausted { progress }` arms; every other write to it is a `.clear()`), and each calls `clear_local_plan()` -- hence `local_planner.cancel()` -- within three lines. `drive_walk`'s empty-path `else` arm cancels as well. A cancel always intervenes first, so no production `post_if_idle` can be refused. M2 shows only that the line is load-bearing for its own assertion; it never covered the claim. The sentence is deleted, not reworded, from the source comment, the assertion message and the PR body. It cannot be removed from e73a0e1's body without rewriting pushed history, so it is retracted here instead -- the squash message must not carry it into main. B2. `docs/http-api.md` states "`nav_local` is `null` on every `idle`" as a universal over the row, and `retire_to_idle` only made it true at the transition: `local` is published from the net thread while `POST /v1/move/stop` retires the row from the HTTP thread, each taking the lock separately, so a verdict computed while the goal was live can land after it is gone. No call site is at fault, so this is a runtime race and not something an assert can catch -- and `debug_assert!` is compiled out of `--release`, which would leave the documented universal false in the shipped binary. `Walker::set_nav_local` therefore coerces a verdict aimed at an already-`idle` row to `None`, making the universal true in every profile. Mutation-checked both ways: deleting the coercion turns the post-retirement assertion RED while the mid-goal one stays GREEN, so the guard fires, and fires only where it should. N2. Deleted the four-site count from the #382 archaeology comment; it overcounted the sites that called `local_planner.cancel()` directly. Describes the mechanism instead of counting it. N3. "only the published field is touched" understated the change: `Walker::local_says_no_way_through` reads `local` back as a steering input, so clearing it on `idle` clears that input too. Stated as an effect, with why it is the correct one. N1. `stop_nav_blocked` never publishing `idle` is true by convention, not by construction -- its `state` is a `&str`. Every call site in the tree passes a literal, so the design holds; recorded as a known limit in `retire_to_idle`'s doc comment rather than fixed, since this PR does not otherwise touch that function. Workspace: 50 test result lines vs 50 running headers, 0 non-canonical, 15 all-zero (empty targets), 1639 passed / 0 failed / 45 ignored / 0 filtered = 1684. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-ipc/src/lib.rs | 40 ++++++++++--- crates/eqoxide-nav/src/walker.rs | 98 +++++++++++++++++++++++++++----- docs/http-api.md | 5 ++ 3 files changed, 120 insertions(+), 23 deletions(-) diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index d5ca77d2..f8881045 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -1435,14 +1435,32 @@ impl NavStatus { /// design (`Walker::stop_nav_blocked` publishes `blocked`/`no_path`, never `idle`, so it does not /// come through here). It does not hold for the published FIELD on an `idle` row: `NavLocal` is /// the fine planner's verdict on threading toward *the goal that just ended*, so it is a per-goal - /// fact by the same argument as `tier`. Two of the six routes left it standing (`zoned` — #766's - /// report — and `zone_cross_dropped_unhandled`); the other four already cleared it — `goal_dropped` - /// and `respawned` because `Walker::resolve_goal`'s no-goto branch calls `clear_local_plan()` on - /// the same tick before it retires, `stopped` and `goto_superseded` through an explicit - /// `s.local = None;` in `CommandState::stamp_new_goal`, now deleted as redundant. Both halves of - /// that four are RUN, not read: `eqoxide-nav`'s - /// `the_goal_dropped_route_already_cleared_the_fine_verdict_before_766` and `eqoxide-command`'s - /// `every_command_side_retirement_retires_the_fine_tiers_verdict_766`. + /// fact by the same argument as `tier`. + /// + /// Before #766 the routes did not agree with each other. `zoned` — the reported one — and + /// `zone_cross_dropped_unhandled` left the verdict standing. The rest already cleared it, by two + /// different mechanisms that are RUN here rather than read off the source: `goal_dropped` (and + /// `respawned`, which shares its branch) because `Walker::resolve_goal`'s no-goto branch calls + /// `clear_local_plan()` on the same tick before it retires — `eqoxide-nav`'s + /// `the_goal_dropped_route_already_cleared_the_fine_verdict_before_766`; and the command-side + /// ones through an explicit `s.local = None;` in `CommandState::stamp_new_goal`, now deleted as + /// redundant — `eqoxide-command`'s + /// `every_command_side_retirement_retires_the_fine_tiers_verdict_766`. Routing them all through + /// here replaces that agreement-by-coincidence with one writer. (`respawned` is covered by + /// reading the shared branch, not by its own test.) + /// + /// **This covers the transition only.** `docs/http-api.md` states `nav_local: null` as a + /// universal over every `idle` row, and a retirement writer cannot deliver that on its own — the + /// fine tier publishes from another thread and can land a verdict after the row went `idle`. + /// The other half of the guarantee is the coercion in `Walker::set_nav_local`; see its doc + /// comment for why that one is a coercion and not an assert. + /// + /// **The `stop_nav_blocked` half of the #382 argument is true by convention, not by + /// construction.** Its `state` is a `&str`, so nothing stops a future caller passing `"idle"` + /// and routing a terminal `blocked` through this retirement after all. Every call site in the + /// tree today passes a literal — `blocked`, `no_path`, `search_exhausted` — so the design holds + /// now; a `debug_assert!(state != "idle", …)` there would make it structural, and is worth + /// doing outside this issue's scope. Recorded as a known limit rather than left implied. pub fn retire_to_idle(&mut self, why: Option<&str>) { // The same writer-level guard as `Walker::set_nav_state_because` and // `CommandState::stamp_new_goal`: on `idle`, `nav_reason: null` is reserved for boot (#725). @@ -1459,7 +1477,11 @@ impl NavStatus { // that is now over — a `no_way_through` published beside `idle`/`zoned` asserts something // about a corridor in a zone we have left, computed against a collision grid that no // longer exists. See the "#766 moved `local`" paragraph above for why this does not - // undo #382's ownership: only the `idle` row is touched, and only the published field. + // undo #382's ownership: only the `idle` row is affected, and this function does not + // touch `LocalPlanner` — but note the effect is not merely cosmetic, because + // `Walker::local_says_no_way_through` reads this same field back as a steering input. + // Clearing it on `idle` clears that input too, which is what we want: on an `idle` row + // there is no goal, so there is no corridor for it to be an opinion about. local, } = self; *state = "idle".to_string(); diff --git a/crates/eqoxide-nav/src/walker.rs b/crates/eqoxide-nav/src/walker.rs index 05528f08..37174cfc 100644 --- a/crates/eqoxide-nav/src/walker.rs +++ b/crates/eqoxide-nav/src/walker.rs @@ -410,18 +410,19 @@ impl Walker { // #766: the identical sentence is true of the FINE plan in flight, and this line was // MISSING. Not a considered asymmetry — archaeology: the coarse `cancel()` above predates // the fine worker (it is in the zone reset at f2dce47^, before #382 existed), and #382's own - // diff added `local_planner.cancel()` beside the fine-plan drop at every OTHER site it - // touched — `clear_local_plan`, `stop_nav_blocked`, `resolve_goal`, `drive_teleport_detect` - // — while this reset, which already cleared `local_path`/`local_i`/`local_stuck_ticks` - // three lines up, was never revisited. No comment anywhere records a reason for the gap. + // diff dropped the fine plan — directly or through `clear_local_plan` — at the sites it + // touched, while this reset, which already cleared `local_path`/`local_i`/ + // `local_stuck_ticks` three lines up, was never revisited. No comment anywhere records a + // reason for the gap. // - // HONEST SCOPE: I could not construct a production route on which a previous zone's fine - // reply is actually APPLIED in the new zone — every path from here to the next - // `local_planner.poll()` (which is gated on a non-empty `self.path`, cleared above) passes - // through a `clear_local_plan()` first. So this is defence in depth restoring #382's own - // pattern, not a leak I measured. What it does fix outright is the ARMED `pending` slot: - // `post_if_idle` is a no-op while one is in flight, so until the stale reply is drained the - // new zone's first fine plans are silently refused. + // SCOPE: this line is defence in depth restoring #382's own pattern. No production route + // can reach a `post_if_idle` that the stale `pending` slot would refuse. Its only + // production caller is in `drive_walk`, behind `!self.path.is_empty()`; `self.path` is made + // non-empty at exactly two sites (`apply_plan`'s `Route` and `Exhausted { progress }` + // arms — every other write to it is a `.clear()`), and each of those calls + // `clear_local_plan()`, hence `local_planner.cancel()`, within three lines; `drive_walk`'s + // empty-path `else` arm cancels as well. So a cancel always intervenes first, and this line + // is not backed by any measured failure of the new zone's planning. self.local_planner.cancel(); self.awaiting_first_plan = false; // SAY WHY (#725 review B1). A bare `idle` here is indistinguishable from "nothing was ever @@ -498,8 +499,25 @@ impl Walker { } /// Publish the FINE tier's last honest outcome (#382). Never touches `state`/`reason`. + /// + /// **A verdict is never stored on an `idle` row (#766).** `NavLocal` is the fine planner's + /// verdict on threading toward *a goal*, and `idle` is the state that means there is no goal — + /// so `Some` beside `idle` is precisely the stale-fact class this issue is about. + /// `NavStatus::retire_to_idle` clears the field at the TRANSITION; this is the other half, the + /// writer-level guard that keeps it clear for the row's whole lifetime. Without it + /// `docs/http-api.md`'s "`nav_local` is `null` on every `idle`" is only true at the instant of + /// retirement, which is not what a polling agent reads. + /// + /// **It is a coercion, not a `debug_assert!`, and that is deliberate.** The gap is a runtime + /// race, not a programming error: `local` is published from the net thread, and `POST /v1/move/stop` + /// can retire the row from the HTTP thread in between, so a verdict computed while the goal was + /// live can arrive after it is gone — no call site is wrong. An assert cannot prevent that, and + /// it is compiled out of `--release` besides, so it would leave the documented universal false + /// in the shipped binary. Dropping the value instead makes it true in every profile. Nothing is + /// lost: the verdict describes a goal that no longer exists. pub fn set_nav_local(&self, local: Option) { let mut s = self.nav.nav_state.lock().unwrap(); + let local = if s.state == "idle" { None } else { local }; if s.local != local { s.local = local; } } @@ -2695,6 +2713,12 @@ mod tests { let col = open_plane(400.0); let (mut w, nav, _intent, _view) = walker_with(col.clone()); + // We are mid-goal in the previous zone. This is not scene-setting: `set_nav_local` refuses + // to store a verdict on an `idle` row (#766, see its doc comment), and the fixture row + // starts `idle`, so a plant without this line would be swallowed and the PREMISE below + // would catch it. + w.set_nav_state_because("navigating", None); + // The previous zone's fine tier reached a verdict, and it is the UNHEALTHY kind — the only // kind `observe.rs` publishes at all (it filters `threaded` out), so this is the shape a // reader actually sees. @@ -2729,9 +2753,55 @@ mod tests { computed against a collision grid that no longer exists — publishing it beside \ `idle`/`zoned` tells the agent something false about the zone it is standing in"); assert!(!w.local_planner.is_planning(), - "#766: and the fine plan in flight must be abandoned like the coarse one — while \ - `pending` is armed, `post_if_idle` is a no-op, so the NEW zone's first fine plans are \ - silently refused until the stale reply drains"); + "#766: the fine plan in flight is abandoned like the coarse one — it was computed \ + against the previous zone's collision grid. Defence in depth: no production route \ + reaches a `post_if_idle` this stale `pending` would refuse (see the SCOPE note in \ + `reset_for_zone_change`), so this pins the line, not a measured failure"); + } + + /// **#766 review B2: the documented universal holds for the whole `idle` row, not just its + /// first instant.** `docs/http-api.md` says "`nav_local` is `null` on every `idle`". Retiring + /// the field in `retire_to_idle` only makes that true at the TRANSITION — and the two writers + /// race, because `set_nav_local` is called from the net thread while `POST /v1/move/stop` retires the + /// row from the HTTP thread, each taking the `nav_state` lock separately. A verdict computed + /// while the goal was live can therefore land after it is gone, with no call site at fault. + /// `set_nav_local` coerces it away; this measures both directions of that coercion, because a + /// guard that swallowed everything would satisfy the negative half on its own. + /// + /// Mutation check: delete `let local = if s.state == "idle" { None } else { local };` from + /// `Walker::set_nav_local` → the post-retirement assertion goes RED and the mid-goal one stays + /// GREEN. The line can fire, and it fires only where it should. + #[test] + fn a_verdict_arriving_after_the_goal_is_retired_is_not_published_766() { + let (w, nav, _intent, _view) = walker_with(Arc::new(std::sync::RwLock::new(None))); + let verdict = || Some(eqoxide_ipc::NavLocal { + state: "no_way_through".into(), reason: "search_closed".into(), + stuck_ticks: 7, plan_us: 1234, + }); + + // Direction 1 — mid-goal, the verdict publishes exactly as #382 intended. Without this the + // test would pass on a `set_nav_local` that had been gutted to a no-op. + w.set_nav_state_because("navigating", None); + w.set_nav_local(verdict()); + assert_eq!(nav.nav_state.lock().unwrap().local, verdict(), + "PREMISE: the guard does not disturb the tier's normal publication — a verdict on a \ + live goal is still what a reader gets"); + + // Direction 2 — the goal is retired, and only THEN does the fine tier's reply come back. + // This is the interleaving, not a hypothetical: the reply was in flight across the + // retirement. Which of the six reasons retired the row is immaterial — they all land in + // `retire_to_idle` — so this uses the one whose constant lives in this crate. + w.set_nav_state_because("idle", Some(NAV_REASON_ZONED)); + assert_eq!(nav.nav_state.lock().unwrap().local, None, + "PREMISE: retirement cleared the field, so the assertion below is about the LATE \ + write and cannot be satisfied by leftover state"); + + w.set_nav_local(verdict()); + assert_eq!(nav.nav_state.lock().unwrap().local, None, + "#766 B2: `idle` means there is no goal, and `NavLocal` is a verdict about threading \ + toward one — publishing it here would tell the agent the fine planner is stuck on a \ + goal it no longer has. `docs/http-api.md` states this as a universal over the row, \ + so the row must hold it for its whole lifetime, not just at the transition"); } /// **#766, the OTHER walker route to `idle` — and it is here to MEASURE a claim, not to guard a diff --git a/docs/http-api.md b/docs/http-api.md index 0d4b5c9f..860c5142 100644 --- a/docs/http-api.md +++ b/docs/http-api.md @@ -855,6 +855,11 @@ collision grid that no longer exists — and it is the only kind that publishes, `threaded` verdict is filtered out anyway. It **does** survive a terminal `blocked` / `no_path`, on purpose: there it is the *evidence* behind the failure you are being told about. +This holds for the whole `idle` row, not just the moment it becomes `idle`, and it takes two writers to +say so. `NavStatus::retire_to_idle` clears the field on the transition; `Walker::set_nav_local` then +refuses to store a verdict onto a row that is already `idle`, which is what covers a fine-tier reply +that was still in flight on another thread when the goal was retired. + **Why this field exists.** The fine tier is bounded *spatially* (a 40 u window) plus a deterministic node cap (#394 removed its old 150 ms wall clock, so its answer no longer depends on machine load), and until #382 it ran **inline on the network thread**, every nav tick — the last A* left on that thread, a From 1204a0e35fa8205a35743763ec42eeabd3ac814c Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 06:42:12 -0400 Subject: [PATCH 3/7] =?UTF-8?q?fix(#766=20review=20r2):=20a=20dead=20fine?= =?UTF-8?q?=20planner=20is=20a=20SESSION=20fault=20=E2=80=94=20stop=20reti?= =?UTF-8?q?ring=20it=20with=20the=20goal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review was REQUEST CHANGES on three findings. B3 was a real agent-honesty hole this PR had opened, so it is fixed as a behaviour change, not a narrowed sentence. B4 and B5 were a false claim and a hole in my own mutation evidence. B3 — the behaviour fix. `planner_dead` is one of `nav_local`'s three publishable states, but unlike `no_way_through` / `exhausted` it is not a verdict about a goal: it is a latched, session-scoped client fault meaning steering has permanently degraded to the coarse 8u route, and `nav_local` was its only publication surface in the tree (the `no_path`/`planner_dead` pair on `nav_state` comes from the COARSE planner). Retiring `nav_local` on every `idle` therefore hid it from an agent BETWEEN goals — exactly when an agent polls to decide what to do next — while `docs/http-api.md` went on asserting both "dead for the rest of the session" and "`nav_local` is `null` on every `idle`". Fixed with a new session-scoped `NavStatus::local_planner_dead`, published as a top-level `nav_local_planner_dead` on GET /v1/observe/debug, always present in both states (a health check needs a readable "alive", not the mere absence of "dead"). `retire_to_idle` KEEPS it — it is the field the E0027 net was built for. Deliberately a separate field and NOT a carve-out in `retire_to_idle`: a carve-out would re-open the clear-on-every-`idle` uniformity #766 exists to create. `Walker::latch_local_planner_liveness` mirrors the fact at the point of DISCOVERY, in `drive_walk`'s fine-planner `is_dead()` branch. An interim draft put that latch at the top of the walk tick and claimed the placement was "unconditional". My own test went RED and was right: three early returns sit above it, and `ActionLoop::tick` does not call `drive_walk` at all once `resolve_goal` returns `None`. The doc now records the real mechanism and the residual limit (a worker that dies and is never posted to again is undetectable by any reader). B4 — `retire_to_idle`'s known-limit paragraph said a `debug_assert!` "would make it structural". False, and my own B2 reasoning is the counter-argument: `debug_assert!` compiles out under `--release`, so it is a test-time instrument. The structural remedy is a typed `state`, which is workspace-wide and out of scope. Corrected in place. B5 — the reviewer replaced the `set_nav_local` guard with the wrong predicate `if s.reason.is_some()` and the whole workspace stayed green: my two test directions varied two things at once. Added a third direction — a terminal `blocked` row, non-`idle` AND carrying a reason — asserting the verdict SURVIVES. That also pins #382's keep-the-verdict-as-evidence design, which had no test anywhere in the tree. Mutations run, all reverted by md5-verified copy-back: * wrong predicate `s.reason.is_some()` → eqoxide-nav 214 passed; 1 failed (walker.rs:2868, the new `blocked` assertion) * delete `latch_local_planner_liveness()` → eqoxide-nav 214 passed; 1 failed (walker.rs:2929, the discovery assertion) * clear `local_planner_dead` in `retire_to_idle` → eqoxide-nav 214 passed; 1 failed (walker.rs:2946) AND eqoxide-http 246 passed; 1 failed (observe.rs:3050) RETRACTION, carried forward: commit e73a0e1's body claims the missing `local_planner.cancel()` fixes an armed `pending` slot that silently refuses the new zone's first fine plans. That is FALSE — a `clear_local_plan()`, hence a `cancel()`, always intervenes first, so no production `post_if_idle` can be refused. The cancel line is defence in depth restoring #382's own pattern, nothing more. Force-push is not done in this repo, so the false sentence stays in history and is retracted here instead. The squash message must not carry it into main. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-http/src/observe.rs | 70 ++++++++++++ crates/eqoxide-ipc/src/lib.rs | 62 +++++++++-- crates/eqoxide-nav/src/walker.rs | 164 +++++++++++++++++++++++++++-- docs/http-api.md | 27 ++++- 4 files changed, 307 insertions(+), 16 deletions(-) diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index 07e4f27f..c2fa7c9d 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -914,6 +914,18 @@ async fn get_debug(State(s): State) -> Json { // clock" with no way to ask which, and `nav_state` said a confident `navigating` throughout. // The clock is gone; the ambiguity went with it. "nav_local": nav_local, + // SESSION-scoped fine-planner liveness (#766 review B3). `nav_local` above is a PER-GOAL + // verdict and #766 retires it with the goal — correct for `no_way_through` / `exhausted`, and + // wrong for `planner_dead`, which is a latched client fault, not a statement about a goal. + // Carried here it survives every retirement, so an agent BETWEEN goals — which is when it + // polls this endpoint to decide what to do next — can still see that its steering has + // permanently degraded to the coarse 8u route. `planner_dead` still appears in `nav_local` + // while a route is committed; this is the channel that does not vanish when the route does. + // + // Always present, in BOTH states, unlike the `null`-when-healthy fields around it: checking + // your own health needs a readable "alive", not merely the absence of "dead" — which is + // indistinguishable from an older client that never had the field. + "nav_local_planner_dead": nav.local_planner_dead, // The agent-honesty payload behind a terminal `no_path` (#378 Phase 2). `null` when there is // nothing to report. `goal` (if present) is the DEFINITIVE "your goal itself cannot be stood // at"; `frontier` is the hazard at the search's CLOSEST APPROACH — one blocking fact, named @@ -2986,6 +2998,64 @@ mod tests { tier's verdict is about threading toward a goal, so it retires with the goal"); } + /// **#766 review B3 — the session-scoped fault must NOT retire with the goal.** + /// + /// The counterweight to the test above, and the reason this PR did not simply narrow a sentence. + /// `planner_dead` is the third publishable `nav_local.state`, and it is not a verdict about a + /// goal: it is a latched client fault meaning steering has permanently degraded to the coarse + /// 8 u route. Retiring `nav_local` on every `idle` — which is #766's whole point — therefore hid + /// it from an agent BETWEEN goals, which is precisely when an agent polls this endpoint. So the + /// fault moved to its own session-scoped field and this measures the split at the JSON surface, + /// where an agent actually reads it: after the same retirement, the per-goal verdict is `null` + /// and the session fault is still `true`. + /// + /// It also pins the always-present shape. A `null`-when-healthy field would make "alive" + /// indistinguishable from "this client is too old to have the field", and a health check you + /// cannot distinguish from a missing feature is not a health check. + /// + /// Mutation check, RUN: clear `local_planner_dead` in `NavStatus::retire_to_idle` instead of + /// keeping it → this crate reports `246 passed; 1 failed`, the failure being the final assertion + /// here (and `eqoxide-nav` goes red on its own row-level assertion, separately). The + /// always-present shape is pinned by the first assertion but NOT mutation-checked — omitting a + /// key from a `json!` literal is a shape change, not a behaviour one, and I did not run it. + #[tokio::test] + async fn debug_keeps_publishing_a_dead_fine_planner_after_the_goal_is_retired_766() { + let state = empty_state(); + let v = debug_json(state.clone()).await; + assert_eq!(v["nav_local_planner_dead"], serde_json::json!(false), + "PREMISE: liveness is published in BOTH states — a healthy client says so out loud, so \ + the `true` below is a change this test caused and not a key that only ever appears \ + when something is wrong"); + + { + let mut s = state.nav.nav_state.lock().unwrap(); + s.goal_id = 4; + s.state = "navigating".into(); + s.local = Some(eqoxide_ipc::NavLocal { + state: "planner_dead".into(), reason: "local_planner_dead".into(), + stuck_ticks: 0, plan_us: 0, + }); + s.local_planner_dead = true; // what `Walker::latch_local_planner_liveness` writes + } + let v = debug_json(state.clone()).await; + assert_eq!(v["nav_local"]["state"], serde_json::json!("planner_dead"), + "PREMISE: while a route is committed the fault is visible in BOTH channels, so the \ + assertions below measure which one survives rather than which one exists"); + assert_eq!(v["nav_local_planner_dead"], serde_json::json!(true)); + + state.nav.nav_state.lock().unwrap().retire_to_idle(Some("zoned")); + + let v = debug_json(state).await; + assert_eq!(v["nav_local"], serde_json::json!(null), + "#766 is unchanged by B3: the per-goal channel still retires with the goal. The session \ + field is an addition, not a hole in that guarantee"); + assert_eq!(v["nav_local_planner_dead"], serde_json::json!(true), + "#766 B3: the worker thread does not come back — recovering it needs a client restart — \ + so an agent between goals must still be able to read that its steering has degraded. \ + Clearing this on retirement would report a recovery that never happened, which is the \ + agent-honesty defect class #766 exists to close, not one it may create"); + } + /// #471 (agent-honesty): the server placed two Mobs (consecutive spawn_ids, e.g. 526/527) at a /// byte-identical position; the wire disambiguates their names with a numeric suffix /// ("Geeda"/"Geeda00"), so in the name-keyed roster they survive as TWO entries. The observe diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index f8881045..f1a9be43 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -1360,6 +1360,29 @@ pub struct NavStatus { /// `nav_state: navigating` needs to know, in the same snapshot, whether the tier that is actually /// steering it can see a way through the next 40 u. pub local: Option, + /// **SESSION-scoped, latched: the fine worker thread has died** (#766 review B3). `true` once + /// `LocalPlanner::is_dead()` has been observed; never cleared, because the thread never comes + /// back — recovering it needs a client restart. Published as the top-level `nav_local_planner_dead` + /// on GET /v1/observe/debug, always, in both states: an agent checking its own health needs to be + /// able to read "alive", not merely fail to read "dead". + /// + /// This field exists because `local` could not carry the fact honestly. `NavLocal` is a PER-GOAL + /// verdict and #766 retires it with the goal, but `planner_dead` was riding in it as one of its + /// three publishable `state` values while being a session fact about the *client*, not a + /// statement about any goal — and `local` was its only publication surface in the tree (the + /// `no_path`/`planner_dead` pair on `state`/`reason` comes from the COARSE planner). So + /// retirement destroyed it, and the review found the consequence: an agent between goals — exactly + /// when it polls `/v1/observe/debug` to decide what to do next — could not learn that its fine + /// planner was dead. Splitting the session fact out of the per-goal row fixes that without a + /// carve-out in [`NavStatus::retire_to_idle`], which would have re-opened the very + /// clear-`local`-on-every-`idle` uniformity #766 exists to create. + /// + /// **Known limit, stated rather than implied.** `LocalPlanner`'s death is only *discoverable* + /// through a failed send or a disconnected receive, both of which happen on a tick that has a + /// committed route. A worker that dies and is never posted to again is not detectable by any + /// reader, this field included; what the latch guarantees is that once the death HAS been seen it + /// stays visible for the rest of the session instead of dying with the next goal. + pub local_planner_dead: bool, } /// A named obstruction with a position — the agent-facing form of `traversability::Blockage` @@ -1433,9 +1456,16 @@ impl NavStatus { /// does — this function does not touch `LocalPlanner`, and every NON-`idle` state keeps `local` /// exactly as before, which is what preserves #382's deliberate keep-the-fine-verdict-on-`blocked` /// design (`Walker::stop_nav_blocked` publishes `blocked`/`no_path`, never `idle`, so it does not - /// come through here). It does not hold for the published FIELD on an `idle` row: `NavLocal` is - /// the fine planner's verdict on threading toward *the goal that just ended*, so it is a per-goal - /// fact by the same argument as `tier`. + /// come through here). It does not hold for the published FIELD on an `idle` row: a `NavLocal` + /// carrying `no_way_through` or `exhausted` is the fine planner's verdict on threading toward *the + /// goal that just ended*, so it is a per-goal fact by the same argument as `tier`. + /// + /// **That argument covers two of the three publishable states, not all three** (review B3). + /// `planner_dead` was never a verdict about a goal — it is a latched, session-scoped client fault + /// that happened to be riding in this field as one of its three publishable `state` values, and + /// retiring it with the + /// goal would hide a dead fine worker from an agent between goals. It is not carved out here; + /// it now has its own session-scoped field, `local_planner_dead`, which this function KEEPS. /// /// Before #766 the routes did not agree with each other. `zoned` — the reported one — and /// `zone_cross_dropped_unhandled` left the verdict standing. The rest already cleared it, by two @@ -1459,8 +1489,20 @@ impl NavStatus { /// construction.** Its `state` is a `&str`, so nothing stops a future caller passing `"idle"` /// and routing a terminal `blocked` through this retirement after all. Every call site in the /// tree today passes a literal — `blocked`, `no_path`, `search_exhausted` — so the design holds - /// now; a `debug_assert!(state != "idle", …)` there would make it structural, and is worth - /// doing outside this issue's scope. Recorded as a known limit rather than left implied. + /// now, but it is grep-checkable, not enforced. + /// + /// The structural remedy is a typed `state` — an enum whose `idle` variant `stop_nav_blocked` + /// cannot name — and that is a workspace-wide change, out of this issue's scope. A + /// `debug_assert!(state != "idle", …)` would NOT be that remedy, and the earlier draft of this + /// paragraph was wrong to call it structural (review B4): `debug_assert!` compiles out under + /// `--release`, so it is a test-time instrument. That is not a new opinion: it is the same + /// argument `Walker::set_nav_local`'s doc makes for taking a coercion instead of an assert, and + /// the repo already says it out loud about the #725 writer guard — the doc on + /// `a_reasonless_idle_is_refused_by_the_writer_not_just_by_a_per_call_site_test_725` in + /// `eqoxide-nav` calls that `debug_assert!` "a TEST-TIME instrument, not a runtime one". It would + /// raise + /// the odds of catching a bad call site in CI; it would not make the invariant hold in the shipped + /// binary. Recorded as a known limit rather than left implied. pub fn retire_to_idle(&mut self, why: Option<&str>) { // The same writer-level guard as `Walker::set_nav_state_because` and // `CommandState::stamp_new_goal`: on `idle`, `nav_reason: null` is reserved for boot (#725). @@ -1483,6 +1525,12 @@ impl NavStatus { // Clearing it on `idle` clears that input too, which is what we want: on an `idle` row // there is no goal, so there is no corridor for it to be an opinion about. local, + // KEPT, deliberately — and this is the field the E0027 net was built for. A dead fine + // worker is a SESSION fact about the client, not a fact about the goal that just ended, + // and it is latched because the thread does not come back. Clearing it here would tell an + // agent between goals that its degraded steering had healed. See the field's own doc for + // why it is a separate field rather than a carve-out in the `local` arm above. + local_planner_dead: _keep_local_planner_dead, } = self; *state = "idle".to_string(); *reason = why.map(str::to_string); @@ -1498,7 +1546,7 @@ impl Default for NavStatus { fn default() -> Self { NavStatus { state: "idle".into(), reason: None, local: None, blocked_goal: None, blocked_frontier: None, tier: None, - goal_id: 0, goal: None } + goal_id: 0, goal: None, local_planner_dead: false } } } @@ -1506,7 +1554,7 @@ impl From<&str> for NavStatus { fn from(state: &str) -> Self { NavStatus { state: state.to_string(), reason: None, local: None, blocked_goal: None, blocked_frontier: None, tier: None, - goal_id: 0, goal: None } + goal_id: 0, goal: None, local_planner_dead: false } } } diff --git a/crates/eqoxide-nav/src/walker.rs b/crates/eqoxide-nav/src/walker.rs index 37174cfc..c5ce0d50 100644 --- a/crates/eqoxide-nav/src/walker.rs +++ b/crates/eqoxide-nav/src/walker.rs @@ -513,14 +513,51 @@ impl Walker { /// can retire the row from the HTTP thread in between, so a verdict computed while the goal was /// live can arrive after it is gone — no call site is wrong. An assert cannot prevent that, and /// it is compiled out of `--release` besides, so it would leave the documented universal false - /// in the shipped binary. Dropping the value instead makes it true in every profile. Nothing is - /// lost: the verdict describes a goal that no longer exists. + /// in the shipped binary. Dropping the value instead makes it true in every profile. + /// + /// **What is lost by dropping it, precisely** (review B3). For `no_way_through` and `exhausted`, + /// nothing: they are verdicts about threading toward a goal that no longer exists. The earlier + /// draft of this doc said that of all of them, and it was false for the third publishable state. + /// `planner_dead` is a latched session fault about the *client*, not about any goal, and dropping + /// it here would have hidden a dead fine worker from an agent between goals — which is when an + /// agent polls `/v1/observe/debug` to decide what to do next. That fact is no longer carried in + /// this field alone: [`Walker::latch_local_planner_liveness`] mirrors it into the session-scoped + /// `NavStatus::local_planner_dead`, which retirement keeps and this coercion does not touch. So + /// the sentence is now true as scoped, because the code makes it true, not because it was + /// narrowed until it stopped saying anything. pub fn set_nav_local(&self, local: Option) { let mut s = self.nav.nav_state.lock().unwrap(); let local = if s.state == "idle" { None } else { local }; if s.local != local { s.local = local; } } + /// Mirror the fine worker's death into the SESSION-scoped `NavStatus::local_planner_dead` (#766 + /// review B3). Latched: `LocalPlanner::is_dead()` is itself latched, and this only ever sets. + /// + /// Called from the `is_dead()` branch in [`Walker::drive_walk`], beside the per-goal + /// `nav_local` publication it backs up. **That is the DISCOVERY site, and discovery is the + /// constraint** — an earlier draft of this method put the call at the top of the walk tick and + /// claimed the placement was "unconditional"; the test below failed and was right to. Two things + /// falsify that: `drive_walk` returns early in three places above it (`halt_no_world`, the coarse + /// `planner.is_dead()` stop, `awaiting_first_plan`), and `ActionLoop::tick` does not call + /// `drive_walk` at all once `resolve_goal` returns `None`. There is no "every tick" to hook. + /// + /// So the latch is placed where the fact first becomes knowable. `LocalPlanner`'s death is only + /// discoverable through a failed send or a disconnected receive — `post_if_idle` and `poll`, both + /// inside the `have_path` branch, both above this check on the same tick. Latching here therefore + /// records it on the very tick it is discovered, and because the record is on the shared row + /// rather than in the per-goal verdict, the later retirement cannot take it away. What changes is + /// not WHEN the fault is seen but how long it stays visible: for the rest of the session, instead + /// of until the next goal ends. + /// + /// The residual limit is stated on the field: a worker that dies and is never posted to again is + /// not discoverable by any reader, this one included. + pub fn latch_local_planner_liveness(&self) { + if !self.local_planner.is_dead() { return; } + let mut s = self.nav.nav_state.lock().unwrap(); + if !s.local_planner_dead { s.local_planner_dead = true; } + } + /// The player's position for the snapshot — **`None` until the server has told us where we /// are** (#615 review F1: a fresh login published a confident `[0,0,0]`, 985 units from the /// character; "unknown" must be representable, never a fabricated origin). @@ -1465,6 +1502,11 @@ impl Walker { state: "planner_dead".into(), reason: "local_planner_dead".into(), stuck_ticks: 0, plan_us: 0, })); + // #766 B3: and mirror it into the SESSION-scoped row, which retirement keeps. The + // `set_nav_local` above is a PER-GOAL channel that #766 now retires on every route + // to `idle` — correct for the two verdict states, wrong for this one. See the + // method's doc for why this call sits here and not somewhere more general. + self.latch_local_planner_liveness(); } // LOS clamp (#685): shorten the carrot at a convex corner so the walker rounds it instead @@ -2765,12 +2807,22 @@ mod tests { /// race, because `set_nav_local` is called from the net thread while `POST /v1/move/stop` retires the /// row from the HTTP thread, each taking the `nav_state` lock separately. A verdict computed /// while the goal was live can therefore land after it is gone, with no call site at fault. - /// `set_nav_local` coerces it away; this measures both directions of that coercion, because a - /// guard that swallowed everything would satisfy the negative half on its own. - /// - /// Mutation check: delete `let local = if s.state == "idle" { None } else { local };` from - /// `Walker::set_nav_local` → the post-retirement assertion goes RED and the mid-goal one stays - /// GREEN. The line can fire, and it fires only where it should. + /// `set_nav_local` coerces it away; this measures three directions of that coercion, because a + /// guard that swallowed everything would satisfy the negative half on its own — and, per review + /// B5, because a guard keyed on the WRONG thing would satisfy the first two. + /// + /// **Why three and not two.** The first two directions are `navigating`+`reason: None` and + /// `idle`+`reason: Some`, which vary two things at once, so any predicate separating those rows + /// passes: the reviewer replaced the guard with `if s.reason.is_some()` and the whole workspace + /// stayed green. The third direction is `blocked`+`reason: Some` — non-`idle` *and* carrying a + /// reason — which is exactly the row that tells a state-keyed guard apart from a reason-keyed one. + /// + /// Mutation checks, both RUN on this branch, not reasoned. (a) Delete + /// `let local = if s.state == "idle" { None } else { local };` from `Walker::set_nav_local` → + /// the post-retirement assertion goes RED, the other directions stay GREEN. (b) Replace it with + /// `if s.reason.is_some() { None } else { local }` → `eqoxide-nav` reports + /// `FAILED. 214 passed; 1 failed; 16 ignored`, the one failure being the `blocked` assertion in + /// this test. So the line can fire, fires only where it should, and keys on the right field. #[test] fn a_verdict_arriving_after_the_goal_is_retired_is_not_published_766() { let (w, nav, _intent, _view) = walker_with(Arc::new(std::sync::RwLock::new(None))); @@ -2802,6 +2854,102 @@ mod tests { toward one — publishing it here would tell the agent the fine planner is stuck on a \ goal it no longer has. `docs/http-api.md` states this as a universal over the row, \ so the row must hold it for its whole lifetime, not just at the transition"); + + // Direction 3 — a TERMINAL `blocked` row, which carries a reason. This is the direction that + // makes the test able to see a WRONG predicate (review B5). Directions 1 and 2 vary two + // things at once (`navigating`+no reason vs `idle`+reason), so any predicate separating those + // two rows passes them both — the reviewer drove `if s.reason.is_some()` through the entire + // workspace green. `blocked` is non-`idle` AND carries a reason, so it separates the two. + // + // It also pins #382's keep-the-verdict-as-EVIDENCE design, which had no test anywhere in the + // tree despite being this field's most load-bearing documented behaviour on a non-`idle` row: + // on a terminal failure the fine tier's verdict is the evidence BEHIND the failure the agent + // is being told about, so suppressing it would delete the explanation and keep the complaint. + w.set_nav_state_because("blocked", Some("local_no_way_through")); + w.set_nav_local(verdict()); + assert_eq!(nav.nav_state.lock().unwrap().local, verdict(), + "#766 B5 / #382: a terminal `blocked` row MUST keep the fine tier's verdict — it is the \ + evidence behind the failure being reported, and `Walker::local_says_no_way_through` \ + reads it back as a steering input. The guard keys on `state == \"idle\"` and nothing \ + else; a predicate that keyed on `reason` instead would pass every other assertion in \ + this test and silently break this design"); + } + + /// **#766 review B3 — a dead fine worker is a SESSION fault and must outlive the goal.** + /// + /// `planner_dead` is one of the three publishable `nav_local.state` values, but unlike + /// `no_way_through` / `exhausted` it is not a verdict about a goal: it is a latched client fault + /// meaning steering has permanently degraded to the coarse 8 u route. #766 retires `nav_local` on + /// every route to `idle`, and the review found the consequence — an agent BETWEEN goals, which is + /// when it polls to decide what to do next, could no longer see that its fine planner was dead. + /// `nav_local` is its only publication surface in the tree (the `no_path`/`planner_dead` pair on + /// `nav_state` comes from the COARSE planner, a different object). + /// + /// The fix is a separate session-scoped field, not a carve-out in `retire_to_idle` — that would + /// have re-opened the clear-on-every-`idle` uniformity #766 exists to create. + /// + /// **This test is driven end-to-end by production `drive_walk`, and an earlier draft that was not + /// is why.** That draft called `drive_walk` with an EMPTY path, on the theory that a latch placed + /// inside the `have_path` branch would be unobservable between goals. It went RED, and it was + /// right: three early returns sit above that point (`halt_no_world`, the COARSE `planner.is_dead()` + /// stop, `awaiting_first_plan`), and `ActionLoop::tick` does not call `drive_walk` at all once + /// `resolve_goal` returns `None` — so there is no "between goals" tick to latch on. The fact is + /// only ever knowable inside `have_path`, because that is where the channel is touched. So the + /// latch goes at the point of DISCOVERY and the row carries it forward from there. Below, the test + /// kills the worker and then hands the whole job to `drive_walk`: it does not call + /// `is_dead()` in a loop to force the state, and it does not call the latch itself. + /// + /// Mutation checks, both RUN. Delete the `latch_local_planner_liveness()` call from `drive_walk` + /// → `eqoxide-nav` reports `FAILED. 214 passed; 1 failed; 16 ignored`, the failure being this + /// test's discovery assertion. Clear `local_planner_dead` in `retire_to_idle` instead of keeping + /// it → `eqoxide-nav` `214 passed; 1 failed` (this test's post-retirement assertion) **and** + /// `eqoxide-http` `246 passed; 1 failed` (the endpoint test). Two lines in two crates, an + /// assertion each, and the second is visible from the published API as well as from the row. + #[test] + fn a_dead_fine_planner_stays_visible_after_the_goal_is_retired_766() { + let (mut w, nav, _intent, _view) = walker_with(open_plane(400.0)); + let mut gs = eqoxide_core::game_state::GameState::new(); + gs.world.zone_name = TEST_ZONE.into(); // #600: loaded-zone match, or drive_walk halts early + gs.player_x = 0.0; gs.player_y = 0.0; gs.player_z = 0.0; gs.player_pos_known = true; + + // A committed route the walker is following — the ONLY situation in which the fine tier is + // touched at all, and therefore the only one in which its death is discoverable. + let goal = (60.0, 0.0, 0.0); + *nav.goto_target.lock().unwrap() = Some(goal); + w.set_nav_state("navigating"); + w.path = vec![[0.0, 0.0, 0.0], [20.0, 0.0, 0.0], [40.0, 0.0, 0.0], [60.0, 0.0, 0.0]]; + w.path_i = 0; + w.path_goal = Some(goal); // same goal → no replan, no `awaiting_first_plan` + + // Kill the fine worker the way a panic does: its reply `Sender` drops. Nothing has NOTICED + // yet — noticing is a failed send or a disconnected receive, and both live in `drive_walk`. + w.local_planner.kill_worker_for_test(); + assert!(!nav.nav_state.lock().unwrap().local_planner_dead, + "PREMISE: nothing has published the fault yet, so the assertion below measures the \ + latch and not a value that was already there"); + + w.drive_walk(&mut gs, goal); + + assert!(nav.nav_state.lock().unwrap().local_planner_dead, + "#766 B3: production `drive_walk` discovered the fine worker's death on this tick and \ + must record it on the SESSION-scoped row, not only in the per-goal verdict — steering \ + has permanently degraded to the coarse 8u route and the thread does not come back"); + assert_eq!(nav.nav_state.lock().unwrap().local.as_ref().map(|l| l.state.clone()), + Some("planner_dead".into()), + "PREMISE: the pre-existing per-goal publication still happens — the session field is an \ + addition beside it, not a replacement for it"); + + // …and now the retirement that #766 made uniform. This is the exact interleaving the review + // found: `nav_local` goes `null` and the agent is between goals, which is when it polls. + w.reset_for_zone_change(); + let s = nav.nav_state.lock().unwrap(); + assert_eq!(s.state, "idle"); + assert_eq!(s.local, None, + "PREMISE: the per-goal verdict is still retired — the session field is an addition, not \ + a hole in #766's guarantee"); + assert!(s.local_planner_dead, + "#766 B3: the thread does not come back, so the fault does not heal. Clearing it here \ + would tell an agent its degraded steering had recovered when nothing recovered it"); } /// **#766, the OTHER walker route to `idle` — and it is here to MEASURE a claim, not to guard a diff --git a/docs/http-api.md b/docs/http-api.md index 860c5142..6303c0bb 100644 --- a/docs/http-api.md +++ b/docs/http-api.md @@ -840,7 +840,7 @@ said. It is **`null` while the tier is healthy** (a complete fine route to its c |---------|---------| | `no_way_through` | The fine planner **closed its entire 40 u window** and found no way along the committed coarse route from here. A falsifiable **local** "no" — the coarse route is being re-planned around it. It says **nothing** about whether your goal is reachable. | | `exhausted` | The fine search was **cut short** (node cap) before closing its window: **"I don't know"**, not "no". The walker is steering on the best partial it has. | -| `planner_dead` | The fine worker thread has **died**. Steering has degraded to the coarse 8 u route for the rest of the session — the character **keeps walking**, but handles thin ramps and narrow openings worse. A client fault; restart to recover it. | +| `planner_dead` | The fine worker thread has **died**. Steering has degraded to the coarse 8 u route for the rest of the session — the character **keeps walking**, but handles thin ramps and narrow openings worse. A client fault; restart to recover it. **Do not poll for this here** — it is a session fault, not a per-goal verdict, so it disappears from `nav_local` when the goal is retired. Read `nav_local_planner_dead` instead. | > **`nav_local.state` is never `no_path`, and structurally cannot be.** A 40 u window can never prove a > *goal* unreachable, so a tight doorway must never be able to tell an agent its destination does not @@ -878,6 +878,31 @@ corridor is not threadable" from "the steering planner hasn't caught up." `nav_l > reported as "no route"**, and an unreachable goal is now reported before the character takes a > single step. +### `nav_local_planner_dead` — fine-planner liveness, session-scoped + +```json +"nav_local_planner_dead": false +``` + +**Always present, in both states.** `true` once the fine worker thread has been observed dead; never +returns to `false`, because the thread does not come back — recovering it needs a client restart. + +This field exists because the `nav_local`-is-`null`-on-`idle` rule above would otherwise hide a client +fault. Two of `nav_local`'s three states — `no_way_through`, `exhausted` — are verdicts about +threading toward *a goal*, so retiring them with the goal is right. `planner_dead` is not: it is a +latched fault about the client itself that happened to be riding in the same field. Left there, it +would vanish on every retirement, so an agent **between goals** — exactly when you poll +`/v1/observe/debug` to decide what to do next — could not learn that its steering had permanently +degraded to the coarse 8 u route. It would reappear only after a new goal had committed a route. + +So check liveness here, not in `nav_local`. `nav_local` still reports `planner_dead` while a route is +committed, which is useful in context; this is the channel that does not vanish when the route does. + +**One honest limit.** The worker's death is only *discoverable* through a failed send or a +disconnected receive, and both happen on a tick that has a committed route. A worker that dies and is +never posted to again is not detectable by any reader, this field included. What is guaranteed is that +once the death has been seen, it stays visible for the rest of the session. + --- ## Connection health From 2fd3da90fb7c72b8256d9e4ec74e3232cb9c0f2f Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 08:05:31 -0400 Subject: [PATCH 4/7] docs(#766 review r3): make the new field discoverable, fix a false completeness claim, drop drifted locators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Doc/comment-only. No production or test code changed; the three crates touched re-run identical (eqoxide-nav 215, eqoxide-http 247, eqoxide-ipc 37, 0 failed), so round 3's workspace figure stands. B6 — `nav_local_planner_dead` was undiscoverable from the endpoint's own field index. `docs/http-api.md`'s GET /v1/observe/debug row is the only place the tree enumerates that endpoint's top-level fields, and the new one was absent; the row also never linked to "The fine steering tier" at all, so the new section was reachable only by someone who already knew the name. That defeats the point of B3, whose whole argument is that an agent can find the field. The row now lists it and links to both sections. B7 — "drive_walk returns early in three places above it" is false; there are five (the zone-usability halt, the mid-tick collision-grid-vanished halt, a coarse reply apply_plan says terminated the goal, the COARSE planner.is_dead() stop, and awaiting_first_plan). More important than the count: the recorded MECHANISM was the shallower one. The decisive defect in an earlier latch placement is ORDERING — LocalPlanner.dead is written at exactly two sites, both called only from inside drive_walk's have_path block and both above the is_dead() check on the same tick, so an earlier latch is one tick late by construction and never fires if the goal retires in that gap. Reachability is additional. Both are now stated, ordering first, in `latch_local_planner_liveness`'s doc and in the B3 test's doc. B8 — the four file:line mutation locators had drifted (correct when measured, then pushed down by a doc-comment expansion in the same commit), and two of them ended up naming assertions the mutation cannot break, which reads as fabricated evidence to anyone reproducing them. Every published COUNT was right. Removed rather than re-measured: a line number is right only until the next edit above it, and a freshly re-checked one is trusted more than it deserves. Mutations are now named by the assertion they turn RED. The four older locators from rounds 1-2 are gone too, for consistency. Non-blocking, added on the reviewer's note: `local_planner_dead`'s doc now records that "session" means PROCESS today — LocalPlanner::spawn is reached only via Walker::new -> ActionLoop::new, whose one production call site is in run_login_flow, which returns when the gameplay phase ends. If relogin ever became in-process, a new healthy worker would inherit this row still reading `true`. Flagged rather than pre-emptively cleared: there is no such route today and a clear on an unreachable route is untestable. Not carried forward from earlier commit bodies on this branch, which are immutable: e73a0e1's claim that the armed `pending` slot silently refuses the new zone's first fine plans (false, retracted twice) and its "five regression tests, four mutation-checked" (now eight/seven); ef99227's 1684 figures, which are not the merged tree's. ef99227's retraction paragraph SHOULD be carried into any squash — after squashing it is the only surviving record of the B1 correction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-http/src/observe.rs | 5 +- crates/eqoxide-ipc/src/lib.rs | 12 +++++ crates/eqoxide-nav/src/walker.rs | 81 ++++++++++++++++++------------ docs/http-api.md | 2 +- 4 files changed, 64 insertions(+), 36 deletions(-) diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index c2fa7c9d..4539225a 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -3014,8 +3014,9 @@ mod tests { /// cannot distinguish from a missing feature is not a health check. /// /// Mutation check, RUN: clear `local_planner_dead` in `NavStatus::retire_to_idle` instead of - /// keeping it → this crate reports `246 passed; 1 failed`, the failure being the final assertion - /// here (and `eqoxide-nav` goes red on its own row-level assertion, separately). The + /// keeping it → the final assertion here goes RED (`246 passed; 1 failed` in this crate), and + /// `eqoxide-nav` goes red separately on its own row-level assertion. Named by assertion, not by + /// line number, deliberately (review B8): a line locator drifts on the next edit above it. The /// always-present shape is pinned by the first assertion but NOT mutation-checked — omitting a /// key from a `json!` literal is a shape change, not a behaviour one, and I did not run it. #[tokio::test] diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index f1a9be43..b6ef3013 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -1382,6 +1382,18 @@ pub struct NavStatus { /// committed route. A worker that dies and is never posted to again is not detectable by any /// reader, this field included; what the latch guarantees is that once the death HAS been seen it /// stays visible for the rest of the session instead of dying with the next goal. + /// + /// **"Session" means PROCESS today, and that is load-bearing** (round-3 review, non-blocking). + /// `LocalPlanner::spawn` is reached only through `Walker::new` → `ActionLoop::new`, and the one + /// production call site of `ActionLoop::new` is in `run_login_flow`, which returns as soon as the + /// gameplay phase ends — so exactly one fine worker exists per process and "latched forever" and + /// "latched for this worker" are the same statement. If a future change ever re-entered gameplay + /// in-process (relogin without restart), a NEW `Walker` with a NEW, healthy `LocalPlanner` would + /// be handed this same shared row still reading `true`, and the client would report a dead fine + /// planner it had just replaced — the honest-signal failure in the opposite direction. Whoever + /// makes relogin in-process owns clearing this at the same point they construct the new worker; + /// it is flagged here rather than pre-emptively cleared, because there is no such route today and + /// a clear on a route that cannot happen is untestable. pub local_planner_dead: bool, } diff --git a/crates/eqoxide-nav/src/walker.rs b/crates/eqoxide-nav/src/walker.rs index c5ce0d50..9d49ff06 100644 --- a/crates/eqoxide-nav/src/walker.rs +++ b/crates/eqoxide-nav/src/walker.rs @@ -536,19 +536,29 @@ impl Walker { /// /// Called from the `is_dead()` branch in [`Walker::drive_walk`], beside the per-goal /// `nav_local` publication it backs up. **That is the DISCOVERY site, and discovery is the - /// constraint** — an earlier draft of this method put the call at the top of the walk tick and - /// claimed the placement was "unconditional"; the test below failed and was right to. Two things - /// falsify that: `drive_walk` returns early in three places above it (`halt_no_world`, the coarse - /// `planner.is_dead()` stop, `awaiting_first_plan`), and `ActionLoop::tick` does not call - /// `drive_walk` at all once `resolve_goal` returns `None`. There is no "every tick" to hook. - /// - /// So the latch is placed where the fact first becomes knowable. `LocalPlanner`'s death is only - /// discoverable through a failed send or a disconnected receive — `post_if_idle` and `poll`, both - /// inside the `have_path` branch, both above this check on the same tick. Latching here therefore - /// records it on the very tick it is discovered, and because the record is on the shared row - /// rather than in the per-goal verdict, the later retirement cannot take it away. What changes is - /// not WHEN the fault is seen but how long it stays visible: for the rest of the session, instead - /// of until the next goal ends. + /// constraint.** An earlier draft put the call earlier in the walk tick, on the theory that + /// "unconditional" beat "inside `have_path`". Two independent things kill that, and review B7 is + /// right that the ORDERING one is decisive while the reachability one is merely additional: + /// + /// 1. **Ordering — structural, and by itself sufficient.** `LocalPlanner.dead` is written at + /// exactly two places, the failed send in `post_if_idle` and the disconnected receive in + /// `poll`. Both are called only from inside `drive_walk`'s `have_path` block, and both sit + /// ABOVE this check on the same tick. So at any point earlier in the tick `is_dead()` can only + /// report a death some EARLIER tick already discovered — an earlier latch is always one tick + /// late, and if the goal retires in that gap it never fires at all. That is the same + /// between-goals hole B3 exists to close, just moved one tick over. + /// 2. **Reachability — additional, and the reason a "just put it higher" placement is not even + /// reliably one tick late.** `drive_walk` returns early at **five** points above + /// `let have_path`, not the three an earlier draft of this doc claimed (review B7): the + /// zone-usability halt; the mid-tick collision-grid-vanished halt; a coarse reply that + /// `apply_plan` says terminated the goal; the COARSE `planner.is_dead()` stop; and + /// `awaiting_first_plan`. Separately, `ActionLoop::tick` does not call `drive_walk` at all once + /// `resolve_goal` returns `None`, so there is no between-goals tick to hook either. + /// + /// Latching at the discovery site therefore records the fault on the very tick it becomes + /// knowable, and because the record is on the shared row rather than in the per-goal verdict, the + /// later retirement cannot take it away. What changes is not WHEN the fault is seen but how long + /// it stays visible: for the rest of the session, instead of until the next goal ends. /// /// The residual limit is stated on the field: a worker that dies and is never posted to again is /// not discoverable by any reader, this one included. @@ -2817,12 +2827,15 @@ mod tests { /// stayed green. The third direction is `blocked`+`reason: Some` — non-`idle` *and* carrying a /// reason — which is exactly the row that tells a state-keyed guard apart from a reason-keyed one. /// - /// Mutation checks, both RUN on this branch, not reasoned. (a) Delete + /// Mutation checks, both RUN on this branch, not reasoned, and reported by ASSERTION rather than + /// by line number (review B8: a line locator drifts on the next edit above it, and a freshly + /// re-measured one is trusted more than it deserves). (a) Delete /// `let local = if s.state == "idle" { None } else { local };` from `Walker::set_nav_local` → /// the post-retirement assertion goes RED, the other directions stay GREEN. (b) Replace it with - /// `if s.reason.is_some() { None } else { local }` → `eqoxide-nav` reports - /// `FAILED. 214 passed; 1 failed; 16 ignored`, the one failure being the `blocked` assertion in - /// this test. So the line can fire, fires only where it should, and keys on the right field. + /// `if s.reason.is_some() { None } else { local }` → the `blocked` assertion below is the ONLY + /// thing that goes RED anywhere (`eqoxide-nav` `FAILED. 214 passed; 1 failed; 16 ignored`; before + /// that assertion existed the same mutation left the whole workspace green). So the line can + /// fire, fires only where it should, and keys on the right field. #[test] fn a_verdict_arriving_after_the_goal_is_retired_is_not_published_766() { let (w, nav, _intent, _view) = walker_with(Arc::new(std::sync::RwLock::new(None))); @@ -2889,22 +2902,24 @@ mod tests { /// have re-opened the clear-on-every-`idle` uniformity #766 exists to create. /// /// **This test is driven end-to-end by production `drive_walk`, and an earlier draft that was not - /// is why.** That draft called `drive_walk` with an EMPTY path, on the theory that a latch placed - /// inside the `have_path` branch would be unobservable between goals. It went RED, and it was - /// right: three early returns sit above that point (`halt_no_world`, the COARSE `planner.is_dead()` - /// stop, `awaiting_first_plan`), and `ActionLoop::tick` does not call `drive_walk` at all once - /// `resolve_goal` returns `None` — so there is no "between goals" tick to latch on. The fact is - /// only ever knowable inside `have_path`, because that is where the channel is touched. So the - /// latch goes at the point of DISCOVERY and the row carries it forward from there. Below, the test - /// kills the worker and then hands the whole job to `drive_walk`: it does not call - /// `is_dead()` in a loop to force the state, and it does not call the latch itself. - /// - /// Mutation checks, both RUN. Delete the `latch_local_planner_liveness()` call from `drive_walk` - /// → `eqoxide-nav` reports `FAILED. 214 passed; 1 failed; 16 ignored`, the failure being this - /// test's discovery assertion. Clear `local_planner_dead` in `retire_to_idle` instead of keeping - /// it → `eqoxide-nav` `214 passed; 1 failed` (this test's post-retirement assertion) **and** - /// `eqoxide-http` `246 passed; 1 failed` (the endpoint test). Two lines in two crates, an - /// assertion each, and the second is visible from the published API as well as from the row. + /// is why.** That draft pre-forced `is_dead()` in a loop, called `drive_walk` with an EMPTY path, + /// and latched from a point above `let have_path`. It went RED, and it was right to: that tick + /// took one of the FIVE early returns above `have_path` (it had no committed route, so it posted + /// a first plan and returned at `awaiting_first_plan`) and never reached the latch. See + /// [`Walker::latch_local_planner_liveness`] for why the deeper defect in that placement is + /// ORDERING rather than reachability — `dead` is only ever set inside `have_path`, so any earlier + /// latch is a tick late by construction even when it is reached. Below, the test does neither: no + /// forcing loop, no direct latch call, and a committed route so production discovers the death + /// itself. + /// + /// Mutation checks, both RUN, reported by ASSERTION rather than by line number — a re-measured + /// line number is correct only until the next edit above it, and reads more trustworthy than it + /// is (review B8). Delete the `latch_local_planner_liveness()` call from `drive_walk` → the + /// discovery assertion here goes RED, `eqoxide-nav` `214 passed; 1 failed; 16 ignored`. Clear + /// `local_planner_dead` in `retire_to_idle` instead of keeping it → the post-retirement assertion + /// here goes RED (`eqoxide-nav` `214 passed; 1 failed`) **and** so does the endpoint test in + /// `eqoxide-http` (`246 passed; 1 failed`). Two lines in two crates, one assertion each, and the + /// second is visible from the published API as well as from the row. #[test] fn a_dead_fine_planner_stays_visible_after_the_goal_is_retired_766() { let (mut w, nav, _intent, _view) = walker_with(open_plane(400.0)); diff --git a/docs/http-api.md b/docs/http-api.md index 6303c0bb..10e2dc3e 100644 --- a/docs/http-api.md +++ b/docs/http-api.md @@ -33,7 +33,7 @@ working. The implementation lives in `src/http/.rs`, each exposing a `rou | Route | Description | |-------|-------------| -| `GET /v1/observe/debug` | Player (zone, race, class, level, pos `[east,north,up]`, heading ccw/cw, `currency`, server_corrections, vitals `hp_pct`/`hp`/`hp_max`/`mana_pct`/`xp_pct`, `levitating` (three-valued `true`/`false`/`null` — see [`levitating`](#levitating--three-valued-levitate-buff-state-not-a-gravity-reading-598)), target `target_id`/`target_name`/`target_hp_pct`/`target_con`/`target_attitude`/`target_level`) + **navigation — SPLIT ACROSS TWO NESTING LEVELS; this grouping is by topic, not by where the field lives** (under `player`: `nav_state`, `nav_reason`, `position_provisional`, `crossing_pending_ms`. Top-level, siblings of `player`, NOT under it — same convention as `last_consider`: `nav_goal_id`, `nav_goal`, `nav_blocked_by`, `nav_tier`, `nav_declined_pads`, `nav_local`, `nav_support`, `nav_tight`; they sit outside `player` because that object is already at serde_json's macro recursion limit — see [Navigation state](#navigation-state) and [`nav_declined_pads`](#nav_declined_pads--the-teleport-pads-nav-refused-offered-back-to-you-543--266)) + **connection health** (`connected`, `link_age_ms`, `last_packet_age_ms`, `snapshot_age_ms`, `world_responsive`, `last_world_response_ms`, `send_failures`, `send_wouldblock_rescued`, `send_deferred`, `send_starved`, `send_failures_unretried`, `last_send_error`, `last_send_error_age_ms`, `reliable_abandoned` — see [Connection health](#connection-health)) + **`net_thread_dead`** (`null` while the network thread is alive; a reason string once it has died and the whole payload is a frozen final snapshot — see [net_thread_dead](#net_thread_dead--the-frozen-worlds-terminality-634)) + **`zone_cross_best_effort`** and **`zone_cross_stopped`** (top-level, `null` while there is nothing to disclose — see [Zone-cross degradations you can detect](#zone-cross-degradations-you-can-detect-713)) + **`last_consider`** (spawn-scoped result of the most recent consider of ANY spawn, target or not — see [Consider results](#consider-results)) + camera state. | +| `GET /v1/observe/debug` | Player (zone, race, class, level, pos `[east,north,up]`, heading ccw/cw, `currency`, server_corrections, vitals `hp_pct`/`hp`/`hp_max`/`mana_pct`/`xp_pct`, `levitating` (three-valued `true`/`false`/`null` — see [`levitating`](#levitating--three-valued-levitate-buff-state-not-a-gravity-reading-598)), target `target_id`/`target_name`/`target_hp_pct`/`target_con`/`target_attitude`/`target_level`) + **navigation — SPLIT ACROSS TWO NESTING LEVELS; this grouping is by topic, not by where the field lives** (under `player`: `nav_state`, `nav_reason`, `position_provisional`, `crossing_pending_ms`. Top-level, siblings of `player`, NOT under it — same convention as `last_consider`: `nav_goal_id`, `nav_goal`, `nav_blocked_by`, `nav_tier`, `nav_declined_pads`, `nav_local`, `nav_local_planner_dead`, `nav_support`, `nav_tight`; they sit outside `player` because that object is already at serde_json's macro recursion limit — see [Navigation state](#navigation-state), [The fine steering tier](#the-fine-steering-tier-nav_local--382) for `nav_local` and [`nav_local_planner_dead`](#nav_local_planner_dead--fine-planner-liveness-session-scoped) — the **session-scoped** fine-planner liveness flag, the one nav field that is always present rather than `null` when healthy, and the one to poll for a dead fine planner because `nav_local` retires with the goal — and [`nav_declined_pads`](#nav_declined_pads--the-teleport-pads-nav-refused-offered-back-to-you-543--266)) + **connection health** (`connected`, `link_age_ms`, `last_packet_age_ms`, `snapshot_age_ms`, `world_responsive`, `last_world_response_ms`, `send_failures`, `send_wouldblock_rescued`, `send_deferred`, `send_starved`, `send_failures_unretried`, `last_send_error`, `last_send_error_age_ms`, `reliable_abandoned` — see [Connection health](#connection-health)) + **`net_thread_dead`** (`null` while the network thread is alive; a reason string once it has died and the whole payload is a frozen final snapshot — see [net_thread_dead](#net_thread_dead--the-frozen-worlds-terminality-634)) + **`zone_cross_best_effort`** and **`zone_cross_stopped`** (top-level, `null` while there is nothing to disclose — see [Zone-cross degradations you can detect](#zone-cross-degradations-you-can-detect-713)) + **`last_consider`** (spawn-scoped result of the most recent consider of ANY spawn, target or not — see [Consider results](#consider-results)) + camera state. | | `GET /v1/observe/frame` | Current rendered frame as a PNG (`Content-Type: image/png`). **503 while the zone's assets are still loading** — see [`zone_assets`](#zone_assets--is-the-world-this-response-describes-actually-loaded-579); `?allow_pending=1` opts past it. Optional `preset`/`pitch`/`yaw`/`distance` params request a one-off diagnostic camera angle for just this capture — see [Camera override for `/frame`](#camera-override-for-observeframe-422). | | `GET /v1/observe/entities[?labeled=1]` | Default: `{ "": [x,y,z], ... }` for all known entities, with same-base-name + byte-identical-position duplicates collapsed (#471 — suspected server-side `spawn2` duplication; the model is untouched so each instance is still targetable by its full name). `?labeled=1` returns the richer `{count, entities:{"":[x,y,z]}, deduped, duplicate_groups:[{position,names,kept}], note, poses, snapshot_age_ms}` exposing which duplicates were collapsed, plus **`poses`** (#643): `{"": {pose, gait}}`, keyed **exactly** like `entities` — the two are projected under one lock, so indexing `poses` by any name in `entities` is safe. `pose` is the server-published body state — `standing`/`freeze`/`looting`/`sitting`/`crouching`/`lying`, or **`unknown()`** when the server sent a code this client does not recognise (reported verbatim, never guessed at). `gait` is the signed locomotion-speed code from the entity's last position update (~12 at walk, 28 at full run, negative when backing up); **`null` means "no position update yet", NOT "standing still"**. The default bare-map shape carries the same freshness value in the `X-Snapshot-Age-Ms` header instead — see [Per-endpoint freshness](#per-endpoint-freshness--snapshot_age_ms-646). | | `GET /v1/observe/inventory` | `{count, items:[{slot,item_id,name,charges,icon,idfile}], currency, coin_verified, snapshot_age_ms}`. Slots are Titanium **wire** ids (DB general slots 23-30 → wire 22-29). | From 76fcddabd4c6e975345536d759e56c66a0bd224b Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 09:27:50 -0400 Subject: [PATCH 5/7] =?UTF-8?q?fix(#766=20review=20r4):=20scope=20the=20fi?= =?UTF-8?q?ne-planner=20latch=20to=20the=20WORKER=20=E2=80=94=20clear=20it?= =?UTF-8?q?=20in=20Walker::new?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 was a doc-accuracy round that added a false claim to a tracked file. `local_planner_dead`'s field doc declined a fix on the ground that "a clear on a route that cannot happen is untestable". That is false, and it was the only reason given. What the missing in-process-relogin route makes untestable is the end-to-end SCENARIO; the CLEAR is a different thing and it has a home — `Walker::new`, the same constructor that calls `LocalPlanner::spawn()`, which takes caller-owned NavSlots and which the eqoxide-nav suite already calls directly. B9 (blocking): `Walker::new` now clears `local_planner_dead` as it spawns the worker, so the latch's lifetime is the WORKER's rather than the shared row's. A second Walker over the same row — the shape an in-process relogin would take — would otherwise inherit `true` and report a planner it had just replaced as dead forever: #343's shape, a value outliving the thing it describes, and a lie in the honesty-critical direction. New test `a_new_walker_does_not_inherit_a_previous_workers_death_766` constructs over a deliberately dirty row. The untestability sentence is DELETED, not reworded. This is a no-op on today's single-Walker process and is not claimed as anything more: `Walker::new` runs once per process via `ActionLoop::new` from `run_login_flow`. It is a structural guarantee about the flag's lifetime. B10: the B3 test's doc stated an uncommitted draft's internals as flat fact. Round 4 established the draft is in no commit, ref or reflog, so no account of it — mine included — is reproducible. Both that doc and `latch_local_planner_liveness`'s now mark it as recollection, using this branch's existing un-run convention, and rest their argument on the present tree instead. B11: the PR body and the source told different stories about that same run. Reconciled to one hedged account. The reviewer withdrew its round-3 ordering attribution as an inference about a tree it never saw; I am not re-asserting mine as history either. The ordering mechanism and the reachability facts are stated as properties of the tree you can read today, which is what they are. Also: added the new citation to walker.rs's `_cited` guard array — the steering.rs citation scan caught that I had cited a test in a doc comment without naming it in a guard. Found by the guard, not by me. Measured, remote builder, four crates (eqoxide-nav/-http/-ipc/-net; -net because action_loop.rs include_str!s docs/http-api.md, which round 4 edited without re-running it): nav 216 / http 247 / ipc 37 / net 380, 0 failed, 8 `test result:` lines vs 8 `running N` headers, 0 non-canonical, 1 all-zero, 882 + 0 + 19 + 0 = 901 = the header sum. Tree md5-verified identical before and after the run. M8 (mutation, run): delete the clear from `Walker::new` → exactly one assertion RED, the new B9 one, nav 215 passed; 1 failed; 16 ignored, with 247/37/380 unmoved in the other three crates. Reverted from an md5-verified copy-aside. eqoxide-command was NOT re-run and that is inference, not measurement: the -ipc edit is doc-comment-only and -command's source-scanning guards read its own src/. Squash note: this commit's claims are current. Do NOT carry e73a0e1's armed- `pending` claim or its "five regression tests, four mutation-checked" count, and do not present ef99227's 1684 figures as the merged tree's — but DO carry ef99227's retraction paragraph, which post-squash is the only record of the B1 correction. The round-3 workspace figure 1686 sits on base daab1ef and is not comparable with main's 1663. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-ipc/src/lib.rs | 25 ++++---- crates/eqoxide-nav/src/walker.rs | 99 ++++++++++++++++++++++++++++---- docs/http-api.md | 8 ++- 3 files changed, 108 insertions(+), 24 deletions(-) diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index b6ef3013..b7016b31 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -1383,17 +1383,20 @@ pub struct NavStatus { /// reader, this field included; what the latch guarantees is that once the death HAS been seen it /// stays visible for the rest of the session instead of dying with the next goal. /// - /// **"Session" means PROCESS today, and that is load-bearing** (round-3 review, non-blocking). - /// `LocalPlanner::spawn` is reached only through `Walker::new` → `ActionLoop::new`, and the one - /// production call site of `ActionLoop::new` is in `run_login_flow`, which returns as soon as the - /// gameplay phase ends — so exactly one fine worker exists per process and "latched forever" and - /// "latched for this worker" are the same statement. If a future change ever re-entered gameplay - /// in-process (relogin without restart), a NEW `Walker` with a NEW, healthy `LocalPlanner` would - /// be handed this same shared row still reading `true`, and the client would report a dead fine - /// planner it had just replaced — the honest-signal failure in the opposite direction. Whoever - /// makes relogin in-process owns clearing this at the same point they construct the new worker; - /// it is flagged here rather than pre-emptively cleared, because there is no such route today and - /// a clear on a route that cannot happen is untestable. + /// **Latched for the life of a WORKER, and cleared where one is spawned** (rounds 3–5 review). + /// "Session" means PROCESS today: `LocalPlanner::spawn` is reached only through `Walker::new` → + /// `ActionLoop::new`, and the one production call site of `ActionLoop::new` is in + /// `run_login_flow`, which returns as soon as the gameplay phase ends — so exactly one fine + /// worker exists per process and "latched forever" and "latched for this worker" coincide. They + /// stop coinciding the moment anything builds a second `Walker` over this row (the shape an + /// in-process relogin would take): a NEW, healthy `LocalPlanner` would inherit `true` and the + /// client would report a fault it had just repaired, permanently — #343's shape, and a lie in the + /// honesty-critical direction. So `Walker::new` clears this flag as it spawns the worker, tying + /// the latch's lifetime to the worker's rather than to the row's. That is a no-op on today's + /// single-`Walker` process and is guarded by + /// `a_new_walker_does_not_inherit_a_previous_workers_death_766` in `eqoxide-nav`, which + /// constructs over a dirty row directly — the relogin *scenario* has no route to test through, + /// but the *clear* does, and the clear is what carries the guarantee. pub local_planner_dead: bool, } diff --git a/crates/eqoxide-nav/src/walker.rs b/crates/eqoxide-nav/src/walker.rs index 9d49ff06..f03ea7c5 100644 --- a/crates/eqoxide-nav/src/walker.rs +++ b/crates/eqoxide-nav/src/walker.rs @@ -311,6 +311,17 @@ impl Walker { // reason about a different state than the loader writes. zone_assets: crate::zone_assets::ZoneAssetStateShared, ) -> Self { + // #766 review B9: a FRESH fine worker starts alive, so the row it will be published on must + // say so. `local_planner_dead` is latched for the life of a worker (see its field doc), and + // the row outlives any one `Walker` — it is a shared `Arc` the HTTP surface also holds. Today + // exactly one `Walker` is built per process, so this clear is a no-op in production; it is + // here so that the flag's lifetime is tied to the WORKER's, structurally, at the one place + // that spawns one. Without it, a second `Walker` over the same row — the shape an in-process + // relogin would create — would inherit `true` and report a planner it had just replaced as + // dead forever — #343's shape, a value that outlives the thing it describes (there, + // `connected: true` published by a loop that had stopped running; here, `dead` published for + // a thread that had been replaced). + nav.nav_state.lock().unwrap().local_planner_dead = false; Walker { nav, world, collision, nav_intent, nav_debug, zone_assets, debug_seq: 0, @@ -536,9 +547,13 @@ impl Walker { /// /// Called from the `is_dead()` branch in [`Walker::drive_walk`], beside the per-goal /// `nav_local` publication it backs up. **That is the DISCOVERY site, and discovery is the - /// constraint.** An earlier draft put the call earlier in the walk tick, on the theory that - /// "unconditional" beat "inside `have_path`". Two independent things kill that, and review B7 is - /// right that the ORDERING one is decisive while the reachability one is merely additional: + /// constraint.** The tempting alternative is to call this earlier in the walk tick, + /// "unconditionally", so that it cannot be missed. (An uncommitted draft of mine did; that draft + /// is in no commit, ref or reflog, so treat any account of it — including + /// [`a_dead_fine_planner_stays_visible_after_the_goal_is_retired_766`]'s — as recollection rather + /// than history, review B10. The argument below does not rest on it.) Two independent things kill + /// that placement, and review B7 is right that the ORDERING one is decisive while the + /// reachability one is merely additional: /// /// 1. **Ordering — structural, and by itself sufficient.** `LocalPlanner.dead` is written at /// exactly two places, the failed send in `post_if_idle` and the disconnected receive in @@ -2112,6 +2127,10 @@ mod tests { // added in round 6 by `steering`'s mechanical citation scan: cited in a doc comment in // this file and named in no guard list. cancelling_the_goto_while_loading_returns_to_idle, + // #766 round 5: cited by `Walker::latch_local_planner_liveness`'s rustdoc, which points + // at this test for the B10 hedge on the uncommitted draft. Caught by `steering`'s scan, + // not by me — the citation was added and the guard was not. + a_dead_fine_planner_stays_visible_after_the_goal_is_retired_766, ]; } @@ -2902,14 +2921,21 @@ mod tests { /// have re-opened the clear-on-every-`idle` uniformity #766 exists to create. /// /// **This test is driven end-to-end by production `drive_walk`, and an earlier draft that was not - /// is why.** That draft pre-forced `is_dead()` in a loop, called `drive_walk` with an EMPTY path, - /// and latched from a point above `let have_path`. It went RED, and it was right to: that tick - /// took one of the FIVE early returns above `have_path` (it had no committed route, so it posted - /// a first plan and returned at `awaiting_first_plan`) and never reached the latch. See - /// [`Walker::latch_local_planner_liveness`] for why the deeper defect in that placement is - /// ORDERING rather than reachability — `dead` is only ever set inside `have_path`, so any earlier - /// latch is a tick late by construction even when it is reached. Below, the test does neither: no - /// forcing loop, no direct latch call, and a committed route so production discovers the death + /// is why.** My recollection of that draft — pre-forcing `is_dead()` in a loop, calling + /// `drive_walk` with an EMPTY path, latching from above `let have_path` — is **recollection, not + /// history**: the review established that the draft survives in no commit, ref or reflog entry, + /// so nobody can reproduce it and no account of its internals, mine included, is checkable + /// (round-5 review B10). Treat it the way this branch treats any un-run claim. + /// + /// What IS checkable is on the tree in front of you, and it is the part that matters. An empty + /// path returns at `awaiting_first_plan`, one of the FIVE early returns above `let have_path`, so + /// no latch placed after `advance_cursor` can fire in that fixture at all — reachability. And + /// independently of any draft, `dead` is only ever written inside `have_path`, so an earlier latch + /// is a tick late by construction even where it IS reached — ordering, which is the decisive + /// defect and is argued in full on [`Walker::latch_local_planner_liveness`]. The two are separate + /// failures with separate evidence; earlier rounds on both sides collapsed them into one story + /// and got the attribution wrong in both directions. Below, the test avoids the whole question: + /// no forcing loop, no direct latch call, and a committed route so production discovers the death /// itself. /// /// Mutation checks, both RUN, reported by ASSERTION rather than by line number — a re-measured @@ -2967,6 +2993,57 @@ mod tests { would tell an agent its degraded steering had recovered when nothing recovered it"); } + /// **#766 review B9 — the latch is scoped to the WORKER, and construction is where that is + /// enforced.** `local_planner_dead` never clears once set, which is right for a thread that does + /// not come back — but "never clears" and "outlives the thread it describes" are different + /// claims, and only the first one is wanted. The row is a shared `Arc` the HTTP surface holds; a + /// `Walker` is not. So a second `Walker` over the same row would publish a *fresh, healthy* + /// worker as permanently dead: #343's shape, and a lie in the honesty-critical direction (the + /// client asserting a fault it has just fixed). + /// + /// Production cannot reach that today — `Walker::new` runs once per process, through + /// `ActionLoop::new` from `run_login_flow`, which returns when the gameplay phase ends. Round 4 + /// declined the clear on that ground and the round-5 review was right to block it: what the + /// missing route makes untestable is the end-to-end relogin *scenario*, not the *clear*, and the + /// clear is what carries the guarantee. `Walker::new` takes caller-owned slots and this suite + /// already calls it directly, so the property is testable at construction with no relogin route + /// in sight — which is what this does. Tier: the flag's lifetime is now pinned to the + /// constructor that spawns the worker, so the bad state is created-and-cleared in one place + /// rather than argued about in a comment. + /// + /// Mutation check, RUN, named by assertion rather than by line (review B8): delete the + /// `local_planner_dead = false` clear from [`Walker::new`] → this test's B9 assertion goes RED, + /// alone. Measured across `eqoxide-nav` / `-http` / `-ipc` / `-net` (this branch's blast radius, + /// the last of them because `action_loop.rs` `include_str!`s `docs/http-api.md`): `eqoxide-nav` + /// `215 passed; 1 failed; 16 ignored`, and 247 / 37 / 380 unmoved in the other three. Not a + /// workspace run — say "these four crates", not "nothing anywhere". That the blast radius is one + /// assertion is the honest measure of the fix: on today's single-`Walker` process it is a + /// structural guarantee, not a behaviour change to any live path. + #[test] + fn a_new_walker_does_not_inherit_a_previous_workers_death_766() { + // A row that already carries a dead worker's latch — the state an in-process relogin would + // hand the next `Walker`, and the only way to reach it from here (nothing in the process + // retires a `Walker` today, which is exactly why the scenario is not the thing under test). + let nav: eqoxide_ipc::NavSlots = Default::default(); + nav.nav_state.lock().unwrap().local_planner_dead = true; + assert!(nav.nav_state.lock().unwrap().local_planner_dead, + "PREMISE: the row starts dirty, so the assertion below measures the constructor and not \ + a field that was `false` all along"); + + let world: eqoxide_ipc::WorldSlots = Default::default(); + let intent: eqoxide_ipc::NavIntent = Default::default(); + let view: crate::diagnostics::NavDebugView = Default::default(); + let collision = open_plane(400.0); + let za = zone_assets_for(&collision); + let _w = Walker::new(nav.clone(), world, collision, intent, view, za); + + assert!(!nav.nav_state.lock().unwrap().local_planner_dead, + "#766 B9: `Walker::new` has just spawned a NEW `LocalPlanner`, which is alive. Leaving \ + the previous worker's latch standing would publish `nav_local_planner_dead: true` for a \ + thread that is running fine, and it would never clear — the client asserting a \ + permanent fault it had itself just repaired"); + } + /// **#766, the OTHER walker route to `idle` — and it is here to MEASURE a claim, not to guard a /// line.** The fix's rationale (in `NavStatus::retire_to_idle`'s doc comment and this PR's body) /// says four of the six routes to `idle` already cleared `local` before #766, two of them — diff --git a/docs/http-api.md b/docs/http-api.md index 10e2dc3e..dd648530 100644 --- a/docs/http-api.md +++ b/docs/http-api.md @@ -884,8 +884,12 @@ corridor is not threadable" from "the steering planner hasn't caught up." `nav_l "nav_local_planner_dead": false ``` -**Always present, in both states.** `true` once the fine worker thread has been observed dead; never -returns to `false`, because the thread does not come back — recovering it needs a client restart. +**Always present, in both states.** `true` once the fine worker thread has been observed dead. It +stays `true` for the life of that worker, because the thread does not come back — recovering it needs +a client restart. The one thing that clears it is the construction of a *new* fine worker, which today +happens exactly once per process, at startup: the flag is scoped to the worker it describes, not to +the process, so it can never outlive the thread it is reporting on. In practice, over a running +client, that means it is one-way. This field exists because the `nav_local`-is-`null`-on-`idle` rule above would otherwise hide a client fault. Two of `nav_local`'s three states — `no_way_through`, `exhausted` — are verdicts about From d56f06919431c6b752b3cd728dba6f3b6ccd423b Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 10:20:38 -0400 Subject: [PATCH 6/7] fix(#766 review r5): sweep the worker-vs-session concept; delete the false "can never outlive its thread" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 was REQUEST CHANGES with the CODE confirmed right. Both blocking findings were introduced by round 5 itself, in the prose it wrote to fix round 4's false claim. B12 — the fix contradicted the field's own defining paragraph. Round 5 made `Walker::new` clear `local_planner_dead` and documented that in the TAIL of a doc comment whose LEAD still read "SESSION-scoped, latched ... never cleared, because the thread never comes back". One comment, two lifetimes, and the one a reader meets first was the wrong one. The remedy is a sweep of the CONCEPT, not a list of the sites I could name. Grepped `never clear|never cleared|never clears|does not come back|never comes back|client restart|session-scoped|SESSION fact|session fault|one-way|outliv| rest of the session` over crates/, docs/ and src/: 101 matching lines, all read. 24 carried the old story. My own enumeration would have named 3. Eight of the 24 are assertion messages and test-doc headings, which a grep of the field NAME cannot reach because they name the concept instead. The rule applied, so it is checkable rather than a taste call: - agent-facing surfaces (docs/http-api.md, the JSON state glosses) keep "session-scoped" -- accurate from outside, since exactly one fine worker is built per process -- and now say WHY the two spans coincide; - client-internal surfaces (rustdoc, comments, test docs, assert messages) say "worker-scoped", which is the rule the code enforces; - every surface using the session framing points at the other one. Consequence statements ("degraded to the coarse 8u route for the rest of the session", "recovering it needs a client restart") are left standing: true of today's process, and not claims about the field's lifetime. That is a decision, not an oversight. B13 — "so it can never outlive the thread it is reporting on" is FALSE, and round 5 put it in docs/http-api.md as the justification for the clear. There is no Drop for Walker or LocalPlanner anywhere in the workspace, and run_net_thread writes a terminal reason on all four of its exit arms; on every one of those the worker is gone while the row -- which the HTTP surface holds its own Arc to -- goes on publishing whatever it last held, `false` included. That is #343's shape in the healthy direction. Both premises verified here before writing the correction. It is disclosed rather than hidden (`net_thread_dead` is non-null on precisely those paths, and the endpoint marks the whole payload a frozen final snapshot), so it is a doc overreach and not a live lie. Per the review, NO teardown writer was added -- that would be a new untested route to fix something an existing signal already reports. The sentence is deleted and replaced by two separately-true claims: the field is one-way over a running client (a property of the PROCESS, true before the clear existed -- the causal "so" is dropped), and a `false` reading is only meaningful while `net_thread_dead` is null. Doc, comment and assert-message only; no behaviour change. Re-run on the standing four crates (eqoxide-nav / -http / -ipc / -net, the last because action_loop.rs include_str!s docs/http-api.md): 8 result lines vs 8 headers, 0 non-canonical, 1 all-zero, 882 + 0 + 19 + 0 = 901 = header sum, 0 FAILED. Per crate 216 / 247 / 37 / 380 -- identical to round 5 in every figure. Measurement rule added: count all-zero targets with a field-anchored match on all three fields, never with a substring. A grep for "0 passed" returns 4 on this log where the anchored form returns 1 (it also matches "380 passed" and two lines with a nonzero `ignored`). The reviewer measured the same effect independently, 5 vs 2, on its own log. SQUASH GUIDANCE -- this repo squash-merges and the default body concatenates EVERY commit body, so anything not excluded lands in main's history. Write the squash body from the FINAL state of the tree, not by assembling commit bodies. Do NOT carry: - "a clear on a route that cannot happen is untestable" (2fd3da9) -- the newest false claim on the branch; - the `pending`/`post_if_idle` claim, which is in THREE bodies: e73a0e1, ef99227, 1204a0e; - "drive_walk returns early in three places" (2fd3da9) -- corrected to five; - "SESSION-scoped ... never cleared" and "it can never outlive the thread it is reporting on" -- this round's two; - any per-round test total as the merged tree's: this branch is based on daab1ef, before #755. DO carry ef99227's retraction paragraph. Closes #766. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-http/src/observe.rs | 21 ++++++---- crates/eqoxide-ipc/src/lib.rs | 55 ++++++++++++++++++-------- crates/eqoxide-nav/src/walker.rs | 62 ++++++++++++++++++------------ docs/http-api.md | 24 ++++++++++-- 4 files changed, 110 insertions(+), 52 deletions(-) diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index 4539225a..1ea49006 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -914,7 +914,11 @@ async fn get_debug(State(s): State) -> Json { // clock" with no way to ask which, and `nav_state` said a confident `navigating` throughout. // The clock is gone; the ambiguity went with it. "nav_local": nav_local, - // SESSION-scoped fine-planner liveness (#766 review B3). `nav_local` above is a PER-GOAL + // WORKER-scoped fine-planner liveness (#766 review B3; scope corrected from "session" by + // round-6 review B12 — the latch is cleared by `Walker::new` as it spawns a replacement, and + // it reads as session-scoped from outside only because exactly one fine worker is built per + // process. `docs/http-api.md` keeps the agent-facing "session-scoped" name and says why). + // `nav_local` above is a PER-GOAL // verdict and #766 retires it with the goal — correct for `no_way_through` / `exhausted`, and // wrong for `planner_dead`, which is a latched client fault, not a statement about a goal. // Carried here it survives every retirement, so an agent BETWEEN goals — which is when it @@ -2998,16 +3002,19 @@ mod tests { tier's verdict is about threading toward a goal, so it retires with the goal"); } - /// **#766 review B3 — the session-scoped fault must NOT retire with the goal.** + /// **#766 review B3 — the worker-scoped fault must NOT retire with the goal.** /// /// The counterweight to the test above, and the reason this PR did not simply narrow a sentence. /// `planner_dead` is the third publishable `nav_local.state`, and it is not a verdict about a /// goal: it is a latched client fault meaning steering has permanently degraded to the coarse /// 8 u route. Retiring `nav_local` on every `idle` — which is #766's whole point — therefore hid /// it from an agent BETWEEN goals, which is precisely when an agent polls this endpoint. So the - /// fault moved to its own session-scoped field and this measures the split at the JSON surface, - /// where an agent actually reads it: after the same retirement, the per-goal verdict is `null` - /// and the session fault is still `true`. + /// fault moved to its own field and this measures the split at the JSON surface, where an agent + /// actually reads it: after the same retirement, the per-goal verdict is `null` and the + /// worker-scoped fault is still `true`. (The field's lifetime is the fine WORKER's, not the + /// session's — round-6 review B12. Nothing in this test turns on the difference; it retires a + /// goal, and retiring a goal does not replace a worker. What it would catch is a clear placed on + /// a retirement route, which is the defect it was written for.) /// /// It also pins the always-present shape. A `null`-when-healthy field would make "alive" /// indistinguishable from "this client is too old to have the field", and a health check you @@ -3048,8 +3055,8 @@ mod tests { let v = debug_json(state).await; assert_eq!(v["nav_local"], serde_json::json!(null), - "#766 is unchanged by B3: the per-goal channel still retires with the goal. The session \ - field is an addition, not a hole in that guarantee"); + "#766 is unchanged by B3: the per-goal channel still retires with the goal. The \ + liveness field is an addition, not a hole in that guarantee"); assert_eq!(v["nav_local_planner_dead"], serde_json::json!(true), "#766 B3: the worker thread does not come back — recovering it needs a client restart — \ so an agent between goals must still be able to read that its steering has degraded. \ diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index b7016b31..fdd7d885 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -1360,20 +1360,25 @@ pub struct NavStatus { /// `nav_state: navigating` needs to know, in the same snapshot, whether the tier that is actually /// steering it can see a way through the next 40 u. pub local: Option, - /// **SESSION-scoped, latched: the fine worker thread has died** (#766 review B3). `true` once - /// `LocalPlanner::is_dead()` has been observed; never cleared, because the thread never comes - /// back — recovering it needs a client restart. Published as the top-level `nav_local_planner_dead` - /// on GET /v1/observe/debug, always, in both states: an agent checking its own health needs to be - /// able to read "alive", not merely fail to read "dead". + /// **The fine worker thread has died — latched, and scoped to that WORKER** (#766 review B3/B9). + /// Set `true` the instant `LocalPlanner::is_dead()` is observed, and cleared by **nothing on any + /// nav route**: no goal, no zone change, no retirement touches it, because the thread does not + /// come back and recovering it needs a client restart. The one writer that clears it is + /// `Walker::new` (`eqoxide-nav`), which does so as it spawns a REPLACEMENT worker — so what the + /// latch describes is the worker, not the process. Those coincide today, exactly one `Walker` + /// being built per process, which is why the agent-facing docs call the field session-scoped; the + /// last paragraph here says why the distinction is worth keeping anyway. Published as the + /// top-level `nav_local_planner_dead` on GET /v1/observe/debug, always, in both states: an agent + /// checking its own health needs to be able to read "alive", not merely fail to read "dead". /// /// This field exists because `local` could not carry the fact honestly. `NavLocal` is a PER-GOAL /// verdict and #766 retires it with the goal, but `planner_dead` was riding in it as one of its - /// three publishable `state` values while being a session fact about the *client*, not a + /// three publishable `state` values while being a fact about the *client's fine worker*, not a /// statement about any goal — and `local` was its only publication surface in the tree (the /// `no_path`/`planner_dead` pair on `state`/`reason` comes from the COARSE planner). So /// retirement destroyed it, and the review found the consequence: an agent between goals — exactly /// when it polls `/v1/observe/debug` to decide what to do next — could not learn that its fine - /// planner was dead. Splitting the session fact out of the per-goal row fixes that without a + /// planner was dead. Splitting the worker fact out of the per-goal row fixes that without a /// carve-out in [`NavStatus::retire_to_idle`], which would have re-opened the very /// clear-`local`-on-every-`idle` uniformity #766 exists to create. /// @@ -1381,7 +1386,8 @@ pub struct NavStatus { /// through a failed send or a disconnected receive, both of which happen on a tick that has a /// committed route. A worker that dies and is never posted to again is not detectable by any /// reader, this field included; what the latch guarantees is that once the death HAS been seen it - /// stays visible for the rest of the session instead of dying with the next goal. + /// stays visible for the rest of that worker's life — which, on today's one-`Walker` process, is + /// the rest of the session — instead of dying with the next goal. /// /// **Latched for the life of a WORKER, and cleared where one is spawned** (rounds 3–5 review). /// "Session" means PROCESS today: `LocalPlanner::spawn` is reached only through `Walker::new` → @@ -1397,6 +1403,18 @@ pub struct NavStatus { /// `a_new_walker_does_not_inherit_a_previous_workers_death_766` in `eqoxide-nav`, which /// constructs over a dirty row directly — the relogin *scenario* has no route to test through, /// but the *clear* does, and the clear is what carries the guarantee. + /// + /// **That covers the BIRTH end of a worker's life and nothing covers the death end** (#766 review + /// B13). There is no `Drop` for `Walker` or for `LocalPlanner` anywhere, so when the net thread + /// ends — `run_net_thread` in `src/model.rs` writes a terminal reason on all four of its exit + /// arms — the worker is gone while this row, which the HTTP surface holds its own `Arc` to, goes + /// on publishing whatever it last held, `false` included. So do NOT read this field as "the flag + /// can never outlive the thread it reports on": a stale `false` after teardown is exactly that. + /// It is *disclosed* rather than hidden — `net_thread_dead` is non-null on precisely those paths + /// and the endpoint marks the whole payload a frozen final snapshot — which is why the review + /// asked for the sentence to be corrected and explicitly did **not** ask for a teardown writer: + /// adding one would be a new, untested route to fix something an existing signal already tells + /// the agent. pub local_planner_dead: bool, } @@ -1476,11 +1494,13 @@ impl NavStatus { /// goal that just ended*, so it is a per-goal fact by the same argument as `tier`. /// /// **That argument covers two of the three publishable states, not all three** (review B3). - /// `planner_dead` was never a verdict about a goal — it is a latched, session-scoped client fault - /// that happened to be riding in this field as one of its three publishable `state` values, and - /// retiring it with the + /// `planner_dead` was never a verdict about a goal — it is a latched client fault, scoped to the + /// fine WORKER rather than to any goal (round-6 review B12; the session framing this paragraph + /// used to carry is the agent-facing one, and `local_planner_dead`'s own doc says why the two + /// coincide today), that happened to be riding in this field as one of its three publishable + /// `state` values, and retiring it with the /// goal would hide a dead fine worker from an agent between goals. It is not carved out here; - /// it now has its own session-scoped field, `local_planner_dead`, which this function KEEPS. + /// it now has its own field, `local_planner_dead`, which this function KEEPS. /// /// Before #766 the routes did not agree with each other. `zoned` — the reported one — and /// `zone_cross_dropped_unhandled` left the verdict standing. The rest already cleared it, by two @@ -1541,10 +1561,13 @@ impl NavStatus { // there is no goal, so there is no corridor for it to be an opinion about. local, // KEPT, deliberately — and this is the field the E0027 net was built for. A dead fine - // worker is a SESSION fact about the client, not a fact about the goal that just ended, - // and it is latched because the thread does not come back. Clearing it here would tell an - // agent between goals that its degraded steering had healed. See the field's own doc for - // why it is a separate field rather than a carve-out in the `local` arm above. + // worker is a fact about the WORKER, not about the goal that just ended, and it is + // latched because that thread does not come back. Retiring a goal is not replacing a + // worker, so nothing here has repaired anything; clearing it would tell an agent between + // goals that its degraded steering had healed. (Scoped to the worker, not to the session + // — round-6 review B12. `Walker::new` is the one writer that clears it. See the field's + // own doc, both for that and for why it is a separate field rather than a carve-out in + // the `local` arm above.) local_planner_dead: _keep_local_planner_dead, } = self; *state = "idle".to_string(); diff --git a/crates/eqoxide-nav/src/walker.rs b/crates/eqoxide-nav/src/walker.rs index f03ea7c5..aa558bc5 100644 --- a/crates/eqoxide-nav/src/walker.rs +++ b/crates/eqoxide-nav/src/walker.rs @@ -529,10 +529,10 @@ impl Walker { /// **What is lost by dropping it, precisely** (review B3). For `no_way_through` and `exhausted`, /// nothing: they are verdicts about threading toward a goal that no longer exists. The earlier /// draft of this doc said that of all of them, and it was false for the third publishable state. - /// `planner_dead` is a latched session fault about the *client*, not about any goal, and dropping - /// it here would have hidden a dead fine worker from an agent between goals — which is when an - /// agent polls `/v1/observe/debug` to decide what to do next. That fact is no longer carried in - /// this field alone: [`Walker::latch_local_planner_liveness`] mirrors it into the session-scoped + /// `planner_dead` is a latched fault about the *client's fine worker*, not about any goal, and + /// dropping it here would have hidden a dead fine worker from an agent between goals — which is + /// when an agent polls `/v1/observe/debug` to decide what to do next. That fact is no longer + /// carried in this field alone: [`Walker::latch_local_planner_liveness`] mirrors it into /// `NavStatus::local_planner_dead`, which retirement keeps and this coercion does not touch. So /// the sentence is now true as scoped, because the code makes it true, not because it was /// narrowed until it stopped saying anything. @@ -542,8 +542,11 @@ impl Walker { if s.local != local { s.local = local; } } - /// Mirror the fine worker's death into the SESSION-scoped `NavStatus::local_planner_dead` (#766 - /// review B3). Latched: `LocalPlanner::is_dead()` is itself latched, and this only ever sets. + /// Mirror the fine worker's death into `NavStatus::local_planner_dead` (#766 review B3). Latched + /// for the life of the WORKER — `LocalPlanner::is_dead()` is itself latched, and this only ever + /// sets; the single clear lives in [`Walker::new`], which runs when a REPLACEMENT worker is + /// spawned (round-6 review B12: this line used to call the field SESSION-scoped, which is the + /// agent-facing name for the same span on today's one-`Walker` process, not the internal rule). /// /// Called from the `is_dead()` branch in [`Walker::drive_walk`], beside the per-goal /// `nav_local` publication it backs up. **That is the DISCOVERY site, and discovery is the @@ -573,7 +576,8 @@ impl Walker { /// Latching at the discovery site therefore records the fault on the very tick it becomes /// knowable, and because the record is on the shared row rather than in the per-goal verdict, the /// later retirement cannot take it away. What changes is not WHEN the fault is seen but how long - /// it stays visible: for the rest of the session, instead of until the next goal ends. + /// it stays visible: for the rest of the worker's life — which, one `Walker` being built per + /// process today, is the rest of the session — instead of until the next goal ends. /// /// The residual limit is stated on the field: a worker that dies and is never posted to again is /// not discoverable by any reader, this one included. @@ -1527,7 +1531,8 @@ impl Walker { state: "planner_dead".into(), reason: "local_planner_dead".into(), stuck_ticks: 0, plan_us: 0, })); - // #766 B3: and mirror it into the SESSION-scoped row, which retirement keeps. The + // #766 B3: and mirror it into the shared row, which retirement keeps (the latch is + // scoped to the WORKER, not to the session — round-6 review B12). The // `set_nav_local` above is a PER-GOAL channel that #766 now retires on every route // to `idle` — correct for the two verdict states, wrong for this one. See the // method's doc for why this call sits here and not somewhere more general. @@ -2907,7 +2912,7 @@ mod tests { this test and silently break this design"); } - /// **#766 review B3 — a dead fine worker is a SESSION fault and must outlive the goal.** + /// **#766 review B3 — a dead fine worker is a WORKER fault and must outlive the goal.** /// /// `planner_dead` is one of the three publishable `nav_local.state` values, but unlike /// `no_way_through` / `exhausted` it is not a verdict about a goal: it is a latched client fault @@ -2917,8 +2922,11 @@ mod tests { /// `nav_local` is its only publication surface in the tree (the `no_path`/`planner_dead` pair on /// `nav_state` comes from the COARSE planner, a different object). /// - /// The fix is a separate session-scoped field, not a carve-out in `retire_to_idle` — that would - /// have re-opened the clear-on-every-`idle` uniformity #766 exists to create. + /// The fix is a separate field outliving the goal, not a carve-out in `retire_to_idle` — that + /// would have re-opened the clear-on-every-`idle` uniformity #766 exists to create. (Its + /// lifetime is the fine WORKER's; round-6 review B12 corrected an earlier "session-scoped" here. + /// Nothing this test does turns on the difference — it retires a goal, and retiring a goal does + /// not replace a worker.) /// /// **This test is driven end-to-end by production `drive_walk`, and an earlier draft that was not /// is why.** My recollection of that draft — pre-forcing `is_dead()` in a loop, calling @@ -2973,12 +2981,13 @@ mod tests { assert!(nav.nav_state.lock().unwrap().local_planner_dead, "#766 B3: production `drive_walk` discovered the fine worker's death on this tick and \ - must record it on the SESSION-scoped row, not only in the per-goal verdict — steering \ - has permanently degraded to the coarse 8u route and the thread does not come back"); + must record it on the shared row that outlives the goal, not only in the per-goal \ + verdict — steering has degraded to the coarse 8u route and THIS thread does not come \ + back"); assert_eq!(nav.nav_state.lock().unwrap().local.as_ref().map(|l| l.state.clone()), Some("planner_dead".into()), - "PREMISE: the pre-existing per-goal publication still happens — the session field is an \ - addition beside it, not a replacement for it"); + "PREMISE: the pre-existing per-goal publication still happens — the liveness field is \ + an addition beside it, not a replacement for it"); // …and now the retirement that #766 made uniform. This is the exact interleaving the review // found: `nav_local` goes `null` and the agent is between goals, which is when it polls. @@ -2986,20 +2995,23 @@ mod tests { let s = nav.nav_state.lock().unwrap(); assert_eq!(s.state, "idle"); assert_eq!(s.local, None, - "PREMISE: the per-goal verdict is still retired — the session field is an addition, not \ - a hole in #766's guarantee"); + "PREMISE: the per-goal verdict is still retired — the liveness field is an addition, \ + not a hole in #766's guarantee"); assert!(s.local_planner_dead, - "#766 B3: the thread does not come back, so the fault does not heal. Clearing it here \ - would tell an agent its degraded steering had recovered when nothing recovered it"); + "#766 B3: a zone change does not replace the fine worker, so the dead one is still the \ + live one and the fault has not healed. Clearing it here would tell an agent its \ + degraded steering had recovered when nothing recovered it"); } /// **#766 review B9 — the latch is scoped to the WORKER, and construction is where that is - /// enforced.** `local_planner_dead` never clears once set, which is right for a thread that does - /// not come back — but "never clears" and "outlives the thread it describes" are different - /// claims, and only the first one is wanted. The row is a shared `Arc` the HTTP surface holds; a - /// `Walker` is not. So a second `Walker` over the same row would publish a *fresh, healthy* - /// worker as permanently dead: #343's shape, and a lie in the honesty-critical direction (the - /// client asserting a fault it has just fixed). + /// enforced.** Once set, `local_planner_dead` is cleared by nothing on any nav route — no goal, + /// no zone change, no retirement — which is right for a thread that does not come back. But + /// "no nav route clears it" and "it outlives the thread it describes" are different claims, and + /// only the first one is wanted. The row is a shared `Arc` the HTTP surface holds; a `Walker` is + /// not. So a second `Walker` over the same row would publish a *fresh, healthy* worker as + /// permanently dead: #343's shape, and a lie in the honesty-critical direction (the client + /// asserting a fault it has just fixed). `Walker::new` is therefore the one writer that clears + /// it, and this test is what pins that — so do not read the opening clause as "never clears". /// /// Production cannot reach that today — `Walker::new` runs once per process, through /// `ActionLoop::new` from `run_login_flow`, which returns when the gameplay phase ends. Round 4 diff --git a/docs/http-api.md b/docs/http-api.md index dd648530..2abfb4a0 100644 --- a/docs/http-api.md +++ b/docs/http-api.md @@ -886,10 +886,26 @@ corridor is not threadable" from "the steering planner hasn't caught up." `nav_l **Always present, in both states.** `true` once the fine worker thread has been observed dead. It stays `true` for the life of that worker, because the thread does not come back — recovering it needs -a client restart. The one thing that clears it is the construction of a *new* fine worker, which today -happens exactly once per process, at startup: the flag is scoped to the worker it describes, not to -the process, so it can never outlive the thread it is reporting on. In practice, over a running -client, that means it is one-way. +a client restart. + +*Session-scoped* is the agent-facing name for that lifetime, and it is accurate: the latch the client +keeps internally is scoped to the fine **worker**, and exactly one fine worker is built per client +process, so from out here the two are the same span. The distinction only matters to the client's own +code, which is where it is written down. + +**Over a running client this field is one-way.** Nothing in the process constructs a second fine +worker, so nothing clears it. That is a property of the *process*, not of the field, and it was +equally true before the clear described next existed. + +**A `false` reading is only meaningful while `net_thread_dead` is `null`.** The client clears this +flag in one place — when it constructs a new fine worker — and nothing writes it when a worker *ends* +without a replacement. Once the network thread has exited, the fine worker is gone and this row, like +the rest of the payload, is frozen at whatever it last held; `net_thread_dead` is the field that tells +you so. Check it before you trust a `false` here. + +> The clear-on-construction exists so that if anything ever *did* build a second fine worker +> in-process, the new one would not inherit the old one's latch and report a fault the client had just +> repaired. On today's single-worker process it is a no-op. This field exists because the `nav_local`-is-`null`-on-`idle` rule above would otherwise hide a client fault. Two of `nav_local`'s three states — `no_way_through`, `exhausted` — are verdicts about From ad3c5a0e5a335414a42b6da6abc05cd5b358bc5b Mon Sep 17 00:00:00 2001 From: dhenry Date: Tue, 28 Jul 2026 11:00:08 -0400 Subject: [PATCH 7/7] fix(#766 review r6): replace "permanently degraded" with a claim about writers; unwrap a doc span Round 6 was REQUEST CHANGES on two findings, both again in prose this PR wrote. The reviewer re-verified the code has not moved. B14 -- my 12-term concept sweep did not contain `permanent*`, and that one missing term yields four survivors, three on lines round 6 ADDED: - docs/http-api.md contradicted itself inside one section: line 888 "recovering it needs a client restart", line 915 "permanently degraded". A restart recovers it, so it is not permanent -- and this is the surface where "session-scoped" was defended as accurate. "Permanently" is not "session-scoped"; it is strictly stronger. - observe.rs was B12's shape at six lines' range, inside one comment block round 6 added: "the lifetime is the fine WORKER's, not the process" and six lines later "permanently degraded". - two test docs asserted the same permanence as fact. Three OTHER uses of the word stay: they name the lie a second worker would tell, which is why this is a finding and not a quibble -- the PR was saying in the same four files both that the degradation is permanent and that "permanent" is what it would be lying about. Round 6's own diff deleted this word from one walker assert message, so the concept was recognised that round and fixed in one of five factual uses. All four now use the reviewer's suggested shape: "degraded to the coarse 8u route, and nothing on any nav route recovers it" -- a claim about WRITERS, checkable against retire_to_idle and Walker::new, where "permanently" was checkable against nothing. THE METHOD CHANGED, NOT THE LIST. Adding permanent* to a 13-term list would reproduce the method that just failed. The sweep was keyed on the PREDICATE -- the open-ended set of English phrasings for "this outlives the worker" -- which cannot be enumerated. Two measured failures on this PR: a field-NAME grep missed 21 of 24; a 12-term PREDICATE list missed 4 more. Round 7 sweeps by SUBJECT, which is a closed set: local_planner_dead, nav_local_planner_dead, LocalPlanner, fine planner, fine-planner, fine worker, fine tier, FINE 2u, planner_dead, coarse 8 ?u -- over crates/, docs/, src/. 224 anchor lines, +/-6-line window, 1096 prose lines. Validated against a corpus I did not choose: the reviewer's 16 terms plus my original 12 give 431 tree-wide hits, of which 36 fall inside the subject window and 395 outside. All 36 read; every one is either about a different field (the COARSE planner, net_thread_dead, transport lifetime) or is one of this round's corrections. The control's vocabulary found nothing inside the subject the subject sweep did not already contain. No "found N, fixed N" figure is restated. B15 -- two unbalanced doc spans, in the one touched file no guard reaches. Measured against the TRUE merge base daab1ef (diffing against main mixes in other PRs' observe.rs edits): ipc/lib.rs 12->12, walker.rs 0->0, observe.rs 4->6. The pair was one code span, `.filter(|l| l.state != "threaded")`, broken across a /// wrap -- cargo doc renders the break inside the span and #773 records the fragment as un-greppable. Now on its own line, with a comment saying why. observe.rs is back to the merge base's 4; the remaining pairs (1418/1419, 3820/3821) are pre-existing and out of scope. Reach proved by RUNNING the guard, not by reading it: steering.rs copied aside (md5 dac4b3b0...), observe.rs added to cited_in, real guard run -> exactly four span hits at the two pre-existing pairs, test RED, `0 passed; 1 failed; 0 ignored; 0 measured; 231 filtered out`; restored by copy-back with the md5 re-verified identical. No git stash, no git checkout. N1 filed as #787 (agent-honesty, severity:low): "exactly one fine worker per process" is load-bearing for four sentences here and pinned by nothing. True today, verified independently; the B9 test cannot be the pin because building a second Walker is its method. The three doc sites now cite #787. Doc and comment only; zero non-comment lines changed. Four crates re-run: 0 `error` lines, 8 result lines vs 8 headers, 0 non-canonical, 1 all-zero anchored (4 naive -- fourth confirmation of the anchored rule), 882 + 0 + 19 + 0 = 901 = header sum, 0 FAILED. Per crate 216/247/37/380, identical to rounds 5 and 6. SQUASH GUIDANCE -- write the squash body from the FINAL state of the tree, not by assembling commit bodies. That rule is the mechanism; the list below is a cross-check on it, and the list has needed extending three rounds running while the rule has not. Do NOT carry: - "a clear on a route that cannot happen is untestable" (2fd3da9); - the `pending`/`post_if_idle` claim: e73a0e1, ef99227, 1204a0e; - "drive_walk returns early in three places" (2fd3da9) -- it is five; - "SESSION-scoped ... never cleared" and "it can never outlive the thread it is reporting on" (d56f069's parents); - 1204a0e's "a latched, session-scoped client fault meaning steering has permanently degraded" and "a new session-scoped NavStatus:: local_planner_dead" -- both corrected scope words plus B14's, one body; - "24 found, 24 fixed" as a completeness claim, and any per-round test total as the merged tree's: this branch is based on daab1ef, before #755. DO carry ef99227's retraction paragraph. Closes #766. Refs #787. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HQVEpaaKeXsZcW9VT2roeV --- crates/eqoxide-http/src/observe.rs | 21 ++++++++++++++------- crates/eqoxide-ipc/src/lib.rs | 4 +++- crates/eqoxide-nav/src/walker.rs | 3 ++- docs/http-api.md | 8 +++++--- 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/crates/eqoxide-http/src/observe.rs b/crates/eqoxide-http/src/observe.rs index 1ea49006..2d057763 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -917,13 +917,17 @@ async fn get_debug(State(s): State) -> Json { // WORKER-scoped fine-planner liveness (#766 review B3; scope corrected from "session" by // round-6 review B12 — the latch is cleared by `Walker::new` as it spawns a replacement, and // it reads as session-scoped from outside only because exactly one fine worker is built per - // process. `docs/http-api.md` keeps the agent-facing "session-scoped" name and says why). + // process — a premise nothing in the tree pins, #787. `docs/http-api.md` keeps the + // agent-facing "session-scoped" name and says why). // `nav_local` above is a PER-GOAL // verdict and #766 retires it with the goal — correct for `no_way_through` / `exhausted`, and // wrong for `planner_dead`, which is a latched client fault, not a statement about a goal. // Carried here it survives every retirement, so an agent BETWEEN goals — which is when it // polls this endpoint to decide what to do next — can still see that its steering has - // permanently degraded to the coarse 8u route. `planner_dead` still appears in `nav_local` + // degraded to the coarse 8u route and that nothing on any nav route recovers it (a claim + // about WRITERS, which is what the tree guarantees; "permanently" was strictly stronger and + // false in the same breath as this block's own "the lifetime is the WORKER's" — round-6 + // review B14). `planner_dead` still appears in `nav_local` // while a route is committed; this is the channel that does not vanish when the route does. // // Always present, in BOTH states, unlike the `null`-when-healthy fields around it: checking @@ -2962,9 +2966,12 @@ mod tests { /// **#766 (agent-honesty) — the OBSERVER half of the fine tier's retirement.** /// /// The sibling of the test above, for the field #732 left standing. `nav_local` is read off the - /// same cloned `NavStatus` and passed through exactly one filter — `.filter(|l| l.state != - /// "threaded")` — so an UNHEALTHY verdict reaches the response body verbatim, and an unhealthy - /// verdict is the only kind an agent can ever see. That makes `no_way_through` the right fixture + /// same cloned `NavStatus` and passed through exactly one filter — + /// `.filter(|l| l.state != "threaded")` — so an UNHEALTHY verdict reaches the response body + /// verbatim, and an unhealthy verdict is the only kind an agent can ever see. (Kept whole on one + /// line: a code span broken across a `///` wrap renders the break inside the span and is + /// un-greppable — #773, round-6 review B15.) + /// That makes `no_way_through` the right fixture /// and makes the first half of this test a real premise: it asserts that the planted verdict /// genuinely publishes, so the `null` below is the retirement's doing and not the filter's. /// @@ -3006,8 +3013,8 @@ mod tests { /// /// The counterweight to the test above, and the reason this PR did not simply narrow a sentence. /// `planner_dead` is the third publishable `nav_local.state`, and it is not a verdict about a - /// goal: it is a latched client fault meaning steering has permanently degraded to the coarse - /// 8 u route. Retiring `nav_local` on every `idle` — which is #766's whole point — therefore hid + /// goal: it is a latched client fault meaning steering has degraded to the coarse 8 u route + /// with nothing on any nav route to recover it. Retiring `nav_local` on every `idle` — which is #766's whole point — therefore hid /// it from an agent BETWEEN goals, which is precisely when an agent polls this endpoint. So the /// fault moved to its own field and this measures the split at the JSON surface, where an agent /// actually reads it: after the same retirement, the per-goal verdict is `null` and the diff --git a/crates/eqoxide-ipc/src/lib.rs b/crates/eqoxide-ipc/src/lib.rs index fdd7d885..31e00357 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -1393,7 +1393,9 @@ pub struct NavStatus { /// "Session" means PROCESS today: `LocalPlanner::spawn` is reached only through `Walker::new` → /// `ActionLoop::new`, and the one production call site of `ActionLoop::new` is in /// `run_login_flow`, which returns as soon as the gameplay phase ends — so exactly one fine - /// worker exists per process and "latched forever" and "latched for this worker" coincide. They + /// worker exists per process and "latched forever" and "latched for this worker" coincide. + /// **That premise is pinned by nothing — no guard, no test — and #787 tracks it**; the B9 test + /// below cannot be the pin, because building a second `Walker` is its method. They /// stop coinciding the moment anything builds a second `Walker` over this row (the shape an /// in-process relogin would take): a NEW, healthy `LocalPlanner` would inherit `true` and the /// client would report a fault it had just repaired, permanently — #343's shape, and a lie in the diff --git a/crates/eqoxide-nav/src/walker.rs b/crates/eqoxide-nav/src/walker.rs index aa558bc5..9b750bd7 100644 --- a/crates/eqoxide-nav/src/walker.rs +++ b/crates/eqoxide-nav/src/walker.rs @@ -2916,7 +2916,8 @@ mod tests { /// /// `planner_dead` is one of the three publishable `nav_local.state` values, but unlike /// `no_way_through` / `exhausted` it is not a verdict about a goal: it is a latched client fault - /// meaning steering has permanently degraded to the coarse 8 u route. #766 retires `nav_local` on + /// meaning steering has degraded to the coarse 8 u route with nothing on any nav route to + /// recover it. #766 retires `nav_local` on /// every route to `idle`, and the review found the consequence — an agent BETWEEN goals, which is /// when it polls to decide what to do next, could no longer see that its fine planner was dead. /// `nav_local` is its only publication surface in the tree (the `no_path`/`planner_dead` pair on diff --git a/docs/http-api.md b/docs/http-api.md index 2abfb4a0..bf11c467 100644 --- a/docs/http-api.md +++ b/docs/http-api.md @@ -891,7 +891,8 @@ a client restart. *Session-scoped* is the agent-facing name for that lifetime, and it is accurate: the latch the client keeps internally is scoped to the fine **worker**, and exactly one fine worker is built per client process, so from out here the two are the same span. The distinction only matters to the client's own -code, which is where it is written down. +code, which is where it is written down — as is the fact that nothing currently pins the one-worker +premise this name rests on (#787). **Over a running client this field is one-way.** Nothing in the process constructs a second fine worker, so nothing clears it. That is a property of the *process*, not of the field, and it was @@ -912,8 +913,9 @@ fault. Two of `nav_local`'s three states — `no_way_through`, `exhausted` — a threading toward *a goal*, so retiring them with the goal is right. `planner_dead` is not: it is a latched fault about the client itself that happened to be riding in the same field. Left there, it would vanish on every retirement, so an agent **between goals** — exactly when you poll -`/v1/observe/debug` to decide what to do next — could not learn that its steering had permanently -degraded to the coarse 8 u route. It would reappear only after a new goal had committed a route. +`/v1/observe/debug` to decide what to do next — could not learn that its steering had degraded to +the coarse 8 u route with nothing on any nav route to recover it. It would reappear only after a new +goal had committed a route. So check liveness here, not in `nav_local`. `nav_local` still reports `planner_dead` while a route is committed, which is useful in context; this is the channel that does not vanish when the route does.