Tighten small-mesh node layout to prevent overlap - #11
Closed
i386 wants to merge 1 commit into
Closed
Conversation
Collaborator
|
Merged to main via cherry-pick in 8d5ab1b |
michaelneale
added a commit
that referenced
this pull request
May 21, 2026
…tion, dedup race, dead code) (#612) * docs(README): mark `model: \"mesh\"` MoA as experimental The MoA gateway is new and still being tuned (routing heuristics, error shapes, tuning knobs). Flag it explicitly in the README so readers do not assume the surface is stable. * fix(moa): close panic surface in worker output normalization PR #566 review (Copilot): the MoA crate had several latent panics around untrusted parsed responses. This commit closes them in one pass and adds regression tests for each. Changes ------- normalize.rs * Single sanitize pass in `normalize_worker_output` runs on the result of *every* parse strategy (JSON, KV, heuristic), not just the heuristic path. The previous shape returned early on JSON/KV success and let non-finite confidences leak into the arbiter where `partial_cmp/total_cmp` could panic on `.unwrap()`. The new helper `sanitize_worker_output` clamps NaN/Inf confidence to 0.5 and collapses non-object `tool_arguments` (Null, primitives, arrays) to `Some({})`. * New `extract_tool_arguments` helper replaces two dead `obj.get("arguments").cloned().or_else(\u2026)` chains in `try_json_parse`. The `or_else` branch was unreachable because `.cloned()` on `Some(Value::String(\u2026))` is already `Some`, so string-encoded JSON arguments leaked through unparsed. `extract_tool_arguments` now explicitly branches on String vs Object vs Null and parses the inner JSON when needed. backend.rs * `parse_retry_after` switched from `to_lowercase()` (Unicode-aware, can change UTF-8 byte length) to `to_ascii_lowercase()` (1:1 byte mapping). The earlier shape sliced the original string using the offset from the lowercased one, which could land mid-codepoint and panic for non-ASCII inputs before the marker. * `extract_text_from_response` switched from direct-indexing `resp["choices"][0]["message"]` to `.pointer()` chaining, returning a structured `Err("malformed response: \u2026")` on missing fields instead of silently producing empty content or panicking downstream. lib.rs * `best_answer` now uses `total_cmp` instead of `partial_cmp(\u2026).unwrap()`. `total_cmp` is total over all f32 (NaN/ Inf included), so even if a future caller bypasses `normalize_worker_output`, this site is panic-free. * `tool_call_response` now explicitly handles every input shape callers can construct: object \u2192 serialize, Null \u2192 `"{}"`, primitive / array \u2192 `"{}"`, validated JSON string \u2192 pass through, invalid string \u2192 `"{}"`. Previously `Value::Null` serialized to the literal four-char string "null", which downstream OpenAI tool-call consumers reject. Tests ----- 13 new tests (now 83/83 pass for the crate): * normalize.rs: kv_path_clamps_nan_confidence, kv_path_clamps_inf_confidence, json_string_encoded_arguments_are_parsed_to_object, null_tool_arguments_become_none, primitive_tool_arguments_collapse_to_empty_object. * backend.rs: parse_retry_after_handles_non_ascii_prefix_without_panic, extract_text_returns_err_on_missing_choices, extract_text_returns_err_on_empty_choices. * lib.rs (new `response_builder_tests` mod): best_answer_does_not_panic_on_nan_confidence, tool_call_response_emits_object_args_for_null, tool_call_response_emits_object_args_for_primitive, tool_call_response_passes_through_string_form_when_valid, tool_call_response_rejects_invalid_string_form. Validation ---------- * cargo test -p mesh-mixture-of-agents --lib: 83/83 pass * cargo check -p mesh-llm: clean * cargo clippy -p mesh-mixture-of-agents --all-targets -- -D warnings: clean * cargo fmt --all -- --check: clean * fix(moa): propagate failure signals to HTTP status, SSE, and tool-result reducer PR #566 review (Copilot) \u2014 three related fixes that all touch how MoA failures reach the caller. 1. HTTP status follows the body's failure signal, not TurnKind ------------------------------------------------------------- `write_moa_response` previously only used HTTP 502 when `TurnKind == Failed`. The tool-result reducer path (`handle_tool_result`) can return `error_response(\u2026)` with `TurnKind::ToolResult` when every reducer candidate fails \u2014 that returned HTTP 200 with an in-band error body, so dumb clients that only check the status code saw a "success". New helper `is_moa_failure_body(body)` recognises the two canonical failure signals MoA emits: a top-level `error` field, and / or `choices[0].finish_reason == "error"`. Status decision now uses this helper, so *every* error-shaped MoA response surfaces as 502 regardless of which sub-flow produced it. 2. SSE adapter propagates the original finish_reason ---------------------------------------------------- `send_moa_as_sse` used to hard-code `finish_reason: "stop"` (or `"tool_calls"` if any tool_calls were present). SSE clients keyed on `finish_reason` (Goose, OpenAI SDKs) therefore saw MoA failures as successful completions. Now the adapter reads `choices[0].finish_reason` from the response body and propagates it ("error" wins over the tool_calls heuristic), and when `is_moa_failure_body` is true it emits an explicit error chunk (`{ object: chat.completion.chunk, choices: [], error: \u2026 }`) before the final finish_reason chunk so SSE clients that scan deltas for an `error` field see the failure too. 3. Tool-result reducer emits tool_calls whenever tool_name is set ----------------------------------------------------------------- Both the tool-result path (`handle_tool_result`) and the fanout/arbiter path (`resolve_decision`) had the same bug: a `ToolProposal` from the reducer only became a real `tool_calls` reply when *both* `tool_name` AND `tool_arguments` were present. With `tool_arguments` missing, both paths silently fell back to a `chat_response` carrying the reducer's prose \u2014 which agent harnesses (Goose, OpenCode) ignore because they only act on `tool_calls`. Now both paths emit `tool_calls` whenever `tool_name` is set, and `tool_call_response` already collapses missing / non-object arguments to `"{}"`. Behaviour is consistent across paths and agent harnesses no longer lose tool calls to reducer prose. Tests ----- Four new `is_moa_failure_body` unit tests in moa_gateway.rs: top-level error, finish_reason=error, success body, tool_calls body. Validation ---------- * cargo test -p mesh-mixture-of-agents --lib: 83/83 pass * cargo test -p mesh-llm-host-runtime --lib: 1446/1446 pass * cargo clippy -p mesh-mixture-of-agents -p mesh-llm-host-runtime --all-targets -- -D warnings: clean * cargo fmt --all -- --check: clean * fix(moa): group aliases by canonical base before resolving backend PR #566 review (Copilot, item #10): the worker pool builder committed to a single alias per canonical model *before* trying to resolve a backend. Two real failure modes: 1. Stale-peer drop. The shortest alias is advertised only by a peer that drops between gossip refresh and orchestration. The peer is gone, `hosts_for_model(alias)` returns empty, the model is silently removed from the worker pool, and longer-form aliases for the same canonical model from still-reachable peers are rejected as duplicates. 2. Forced QUIC hop. The local node advertises a longer convention (e.g. `unsloth/Qwen3-8B-GGUF:Q4_K_M`) while a peer advertises a shorter variant (e.g. `Qwen3-8B-Q4_K_M`). The shortest-name rule picks the peer alias, `add_worker_backend` looks for a local port under that specific string in `targets`, finds nothing, and forces a QUIC tunnel even though the model is locally served. Fix: group all advertised aliases by canonical base first, then within each group sort so the most likely optimization wins first try (locally-served alias before remote, then shortest first as a tiebreaker). The resolver walks the group in order and takes the first alias that produces a backend, so an unreachable preferred alias falls back to a reachable longer one instead of dropping the model. Refactor extracts a small `resolve_one_worker_from_aliases` helper to keep `build_moa_config` under the cognitive-complexity limit. Tests (4 new) in moa_gateway.rs: * group_aliases_keeps_all_aliases_per_canonical_base * group_aliases_prefers_locally_served_alias_even_when_longer * group_aliases_falls_back_to_shortest_when_no_local * group_aliases_distinct_models_stay_in_separate_groups Validation ---------- * cargo test -p mesh-llm-host-runtime --lib moa_gateway: 13/13 pass * cargo test -p mesh-llm-host-runtime --lib: 1450/1450 pass * cargo clippy -p mesh-llm-host-runtime --all-targets -- -D warnings: clean * cargo fmt --all -- --check: clean * fix(moa): strip dead Session continuation/running-summary machinery PR #566 review (Copilot, item #11): `Session::classify_turn` used a `self.turns` counter to decide Fresh vs Continuation, but the gateway constructs a fresh `Session` per inbound request and never invokes `record_assistant_response` / `record_turn_outcome` in production. As a result `turns` was always 1, `Continuation` never fired, and the entire deterministic running-summary feature (`accepted_facts`, `AcceptedFact`, `record_turn_outcome`, `rebuild_summary`, `running_summary` accessor) silently never executed. `pack_fast` had a `turn_count() > 1` branch that appended the summary to the worker's system prompt; that branch was dead too. This commit reflects the gateway's actual design: MoA is request-scoped, and the caller (Goose, OpenCode, an SDK) owns the multi-turn loop and sends the full history each request. Continuation context comes from `session.messages()`, not from a gateway-owned summary. Changes ------- session.rs * Drop `TurnType::Continuation` (only `Fresh` and `ToolResult` remain). The header comment documents the request-scoped lifetime. * Drop fields: `turns`, `last_was_tool_call`, `accepted_facts`, `running_summary`. * Drop methods: `record_assistant_response`, `record_turn_outcome`, `rebuild_summary`, `running_summary`, `accepted_facts`, `turn_count`, `has_unprocessed_tool_results`. * Drop struct: `AcceptedFact`. * Rewrite `ingest` to rebuild `pending_tools` from the caller-provided history each call (delta tracking only worked when Sessions were persisted across requests). Both assistant-emitted tool_calls and tool-result messages are picked up; agent harnesses unchanged. * Simplify `classify_turn`: walk history backwards, return `ToolResult` if a `role: "tool"` message appears before any `role: "assistant"`, else `Fresh`. lib.rs * Drop the `Continuation` match arm. context.rs * Drop the `turn_count() > 1` running-summary injection from `pack_fast`. Context comes from `session.messages()`. Tests ----- * tool_result_turn test rewritten to not rely on the removed `record_assistant_response`; verifies the same behaviour with caller-provided history. * No other test changes needed. Net: 132 LoC of dead machinery removed. Validation ---------- * cargo test -p mesh-mixture-of-agents --lib: 83/83 pass * cargo test -p mesh-mixture-of-agents (incl. integration): all pass * cargo test -p mesh-llm-host-runtime --lib: 1450/1450 pass * cargo check -p mesh-llm: clean * cargo clippy -p mesh-mixture-of-agents --all-targets -- -D warnings: clean * cargo fmt --all -- --check: clean * fix(moa): tighten error code, linearize strip_thinking, sanitize header names, drop dead branch PR #566 review (Copilot) \u2014 batch of MEDIUM cleanups. mesh-mixture-of-agents ---------------------- * `error_response` now takes a `code: &str` parameter and the two call sites pass distinct constants: - `MOA_ERR_ALL_WORKERS_FAILED` for the fanout/arbiter failure path. - `MOA_ERR_ALL_REDUCERS_FAILED` for the tool-result reducer path. Previously both paths emitted `code = "all_workers_failed"`, which was misleading for clients branching on `error.code`. * `strip_thinking` rewritten as a single linear pass over the input. The previous shape rebuilt the entire string on every think block (`format!` + `replace` in a loop), which is O(n*k) for long worker outputs with many `<think>` blocks. Three new tests cover the new shape: orphan close-tag handling, fifty-block linear behaviour, and UTF-8 preservation through multibyte content. mesh-llm-host-runtime --------------------- * `network::openai::transport`: header NAMES are now validated against the RFC 7230 tchar grammar via a new `is_valid_header_name` helper. Names that fail the grammar are dropped with a tracing warning rather than written verbatim. CR/LF in header values is still stripped.\n Both `send_json_ok_with_headers` and\n `send_json_with_status_and_headers` route through a shared\n `append_safe_header(\u2026)` so the validation can't be bypassed by a\n future caller. Four new tests cover the validator and the safe\n header writer.\n\n* `network::openai::moa_gateway::send_moa_as_sse` now reuses\n `append_safe_header` so the SSE adapter gets the same name validation\n as the JSON writers.\n\n* `network::openai::ingress::try_intercept_moa`: dropped the\n unreachable `if let Some(_unused_stream) = \u2026 { tracing::error!(\u2026) }`\n branch. `try_handle_moa` self-gates on the model name and the outer\n gate guarantees it matches, so the inner call always returns\n `None` here. Replaced with `let _ = \u2026.await;` and a comment\n explaining the invariant.\n\nValidation\n----------\n* cargo test -p mesh-mixture-of-agents --lib: 86/86 pass (3 new)\n* cargo test -p mesh-llm-host-runtime --lib: 1454/1454 pass (4 new)\n* cargo clippy -p mesh-mixture-of-agents -p mesh-llm-host-runtime\n --all-targets -- -D warnings: clean\n* cargo fmt --all -- --check: clean * docs(moa): align stale comments and doc claims with current code PR #566 review (Copilot) \u2014 batch of LOW comment/doc corrections so future readers don't trust outdated narrative. * mesh-llm-host-runtime/src/network/openai/moa_gateway.rs: `try_handle_moa` doc said "returns `true` if handled" but the signature is `Option<TcpStream>`. Rewrote the doc to describe the actual contract: `Some(stream)` means "not MoA, fall through", `None` means "MoA consumed the stream and responded; do not respond again". * model-ref/src/lib.rs: comment claimed the function "emits a lowercase selector", but the implementation slices from the\n original (case-preserving) stem and lowercasing is only used for\n marker-position lookup. Reworded so the comment matches the code.\n\n* skippy-cache/src/resident/prefix.rs: regression-test comment\n referenced a "4*min_tokens floor" \u2014 that derivation was replaced by\n a hard `MIN_CTX_FOR_CELL_CAP = 8192` threshold in a follow-up\n commit. Updated to describe the floor that actually ships.\n\n* mesh-llm-host-runtime/src/inference/skippy/family_policy.rs:\n inline comment said the 16-entry cap "does not fully eliminate"\n unified-KV starvation and that the "proper fix is out of scope".\n The proper fix (token-based budget via `max_resident_tokens` in\n `ResidentCacheConfig::from_stage`) shipped in PR #566. Updated to\n describe entry-count as the coarse lever and `max_resident_tokens`\n as the complementary fine-grained budget.\n\n* docs/design/MOA_GATEWAY.md:\n - Role assignment section claimed "the largest of the big tier gets\n Strong". The impl only partitions small vs big, no\n within-tier sort by parameter count. Reworded to describe the\n actual heuristic and call out the future option.\n - Test plan said "29 unit tests" with a stale per-area breakdown.\n Updated to 86 with current coverage areas, and added a note to\n bump the number when adding tests.\n\nValidation\n----------\n* cargo test -p mesh-mixture-of-agents --lib: 86/86 pass\n* cargo test -p mesh-llm-host-runtime --lib: 1454/1454 pass\n* cargo test -p skippy-cache --lib: 13/13 pass\n* cargo test -p model-ref --lib: 10/10 pass\n* cargo clippy -p mesh-mixture-of-agents -p mesh-llm-host-runtime\n -p skippy-cache -p model-ref --all-targets -- -D warnings: clean\n* cargo fmt --all -- --check: clean * fix(moa): drop malformed tool_calls instead of inserting empty-id placeholders PR #612 review (Copilot) \u2014 two follow-ups. 1. session.rs: malformed tool_calls in caller history ----------------------------------------------------- The earlier shape defaulted missing/empty `id` and `function.name` to `""` and still pushed a `PendingToolCall`. With adversarial or buggy caller history this had two real consequences: * Two malformed calls share `call_id == ""` and become indistinguishable, so any later `role: "tool"` whose `tool_call_id` is also missing matches the first one rather than the intended call. * The `tool_call_response` wire-shape invariant (non-empty function name) is violated downstream. Now both `assistant`-side ingestion and `tool`-side matching skip entries with missing or empty `id`/`tool_call_id` (and `assistant` ingestion also skips entries with empty `function.name`), emitting a `tracing::warn!` so the issue is visible in logs. Well-formed history is unaffected. 2. normalize.rs: doc/code mismatch on tool_arguments ---------------------------------------------------- The `extract_tool_arguments` doc claimed missing/null `arguments` collapses to `Some({})`. The code returns `None` and relies on the downstream `tool_call_response` to substitute `"{}"` when serializing the wire shape. The wire output is correct either way, but the doc misled future maintainers about the invariant. Rewrote the comment to describe the actual invariant ("`None` or an object; `None` means emit `{}` at wire time") so a future refactor doesn't reintroduce the literal-"null" bug. Regression test --------------- `malformed_tool_calls_are_dropped_not_collapsed_to_empty_id` covers four malformed shapes in one history (missing id, missing function.name, empty id, plus an orphaned tool result) and asserts only the one well-formed call survives and the orphaned result doesn't attach. Validation ---------- * cargo test -p mesh-mixture-of-agents --lib: 87/87 pass (+1 new) * cargo test -p mesh-llm-host-runtime --lib: 1454/1454 pass * cargo clippy -p mesh-mixture-of-agents -p mesh-llm-host-runtime --all-targets -- -D warnings: clean * cargo fmt --all -- --check: clean Live end-to-end validation -------------------------- Built locally and deployed to the 2-node private mesh (M4 Qwen3-8B + Studio MiniMax). All four agent harness shapes from PR #566/#612 exercised the changed code paths: * mini-agent.py model=auto \u2014 2 turns, correct. * mini-agent.py model=mesh \u2014 2 turns, correct. * mini-agent2.py model=mesh \u2014 4 turns multi-file, correct. * Goose model=mesh \u2014 1 tool call, correct. No regressions in MoA fan-out / tool-result reducer routing. * fix(moa): route streaming failures to HTTP 502 (drop in-band SSE error) PR #612 review feedback (Nick) \u2014 two related findings on the streaming failure path. 1. Streaming MoA failures returned HTTP 200 ------------------------------------------- `write_moa_response` previously routed all streaming responses through `send_moa_as_sse`, which always emits `HTTP/1.1 200 OK` because SSE clients expect 200 before they start parsing the event stream. That meant `stream: true` MoA failures (`all_workers_failed`, `all_reducers_failed`, reducer hedge exhaustion) arrived as a 200 SSE that *happened* to carry `finish_reason: "error"` \u2014 dumb HTTP clients saw a "successful" stream and didn't realise inference had failed. The body is fully available before we decide how to write it, so we can collapse failure-shaped streaming responses to a non-streaming HTTP 502 JSON response with the structured error body. This matches the OpenAI API shape (failures on streaming endpoints come back as a single non-streaming JSON error response with the right HTTP status) and means streaming and non-streaming MoA failures are now consistent at the HTTP layer. 2. Error-only SSE chunk could break OpenAI-shape clients -------------------------------------------------------- The previous error chunk emitted on failure was `{ object: chat.completion.chunk, choices: [], error: \u2026 }`. Many OpenAI client SDKs assume every `chat.completion.chunk` has `choices[0]` and index blindly into it, so an empty `choices: []` array crashes those clients with an index error. With finding #1's routing change, failure-shaped bodies never reach `send_moa_as_sse` anymore, so the error-chunk emission is dead code. Drop it. `send_moa_as_sse` now does one clear thing: emit a single delta chunk + a `finish_reason: "stop" | "tool_calls"` stop chunk, both with a real `choices[0]`. A `debug_assert!` pins the invariant in tests. Tests ----- Four new unit tests covering the four corners of the routing decision: * `streaming_success_routes_to_sse` * `streaming_failure_routes_to_json_502_not_sse` * `non_streaming_success_routes_to_json_200` * `non_streaming_failure_routes_to_json_502` Validation ---------- * cargo test -p mesh-llm-host-runtime --lib: 1458/1458 pass (4 new) * cargo test -p mesh-mixture-of-agents --lib: 87/87 pass * cargo clippy ... --all-targets -- -D warnings: clean * cargo fmt --all -- --check: clean Live end-to-end validation on 2-node mesh (M4 + Studio MiniMax): * Non-streaming MoA happy path: HTTP 200 + structured response. * Streaming MoA happy path: HTTP 200 + `text/event-stream` + `[DONE]`. * mini-agent.py model=mesh: 2 turns, correct. * mini-agent2.py model=mesh: 4 turns multi-file, correct. * Goose model=mesh: 1 tool call, correct. The failure path is exercised by unit tests; production-triggering streaming failures would require fault injection beyond what the harness covers, but the routing logic and the SSE invariant are locked down at the code level.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before:
After:
