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..2d057763 100644 --- a/crates/eqoxide-http/src/observe.rs +++ b/crates/eqoxide-http/src/observe.rs @@ -914,6 +914,26 @@ 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, + // 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 — 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 + // 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 + // 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 @@ -2943,6 +2963,114 @@ 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. (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. + /// + /// 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"); + } + + /// **#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 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 + /// 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 + /// 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 → 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] + 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 \ + 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. \ + 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 9444860f..31e00357 100644 --- a/crates/eqoxide-ipc/src/lib.rs +++ b/crates/eqoxide-ipc/src/lib.rs @@ -1360,6 +1360,64 @@ 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, + /// **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 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 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. + /// + /// **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 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` → + /// `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. + /// **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 + /// 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. + /// + /// **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, } /// A named obstruction with a position — the agent-facing form of `traversability::Blockage` @@ -1426,6 +1484,62 @@ 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: 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 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 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 + /// 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, 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). @@ -1438,10 +1552,25 @@ 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 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, + // KEPT, deliberately — and this is the field the E0027 net was built for. A dead fine + // 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(); *reason = why.map(str::to_string); @@ -1449,6 +1578,7 @@ impl NavStatus { *blocked_goal = None; *blocked_frontier = None; *tier = None; + *local = None; } } @@ -1456,7 +1586,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 } } } @@ -1464,7 +1594,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 e832faa9..9b750bd7 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, @@ -407,6 +418,23 @@ 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 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. + // + // 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 // requested" — and this is the line that runs on a SUCCESSFUL `/v1/move/zone_cross`, so the @@ -417,6 +445,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 +470,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 @@ -471,11 +510,83 @@ 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. + /// + /// **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 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. 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 `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 + /// 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 + /// `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 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. + 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). @@ -1420,6 +1531,12 @@ impl Walker { state: "planner_dead".into(), reason: "local_planner_dead".into(), stuck_ticks: 0, plan_us: 0, })); + // #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. + self.latch_local_planner_liveness(); } // LOS clamp (#685): shorten the carrot at a convex corner so the walker rounds it instead @@ -2015,6 +2132,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, ]; } @@ -2644,6 +2765,334 @@ 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()); + + // 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. + 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: 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 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, 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 }` → 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))); + 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"); + + // 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 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 + /// 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 + /// `nav_state` comes from the COARSE planner, a different object). + /// + /// 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 + /// `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 + /// 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)); + 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 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 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. + 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 liveness field is an addition, \ + not a hole in #766's guarantee"); + assert!(s.local_planner_dead, + "#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.** 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 + /// 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 — + /// `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..bf11c467 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). | @@ -840,12 +840,26 @@ 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 > 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. + +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 @@ -864,6 +878,53 @@ 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. It +stays `true` for the life of that worker, because the thread does not come back — recovering it needs +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 — 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 +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 +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 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. + +**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