8 Commits

Author	SHA1	Message	Date
Patrick Buckley	c6b4dc26be	fix(console): atomic coord-subsystem commit + offload startup teardown ... Address Copilot review feedback on PR #487: 1. **Atomic commit invariant**: ``_bootstrap_coord_subsystem`` previously stamped ``coord_mgr`` ~50 lines before the final ``coord_registry`` commit, and started threads + subscriptions in between. A concurrent dashboard request running through ``_require_coord_mgr`` during the runtime-bootstrap window could observe ``coord_mgr`` set with ``coord_registry`` still ``None`` and surface the misleading "Restart the console after adding a model definition" 503. Refactored to two phases: (a) build everything as locals, (b) start side-effects (StateWriter / observer / nudge watcher / child fan-out / cleanup thread), then atomic commit at the end with ``coord_mgr`` stamped LAST. The build-phase ``try/except`` rolls back any started side-effects from local handles before re-raising — no daemon thread or subscription leaks across retries, and ``app.state`` is never stamped on a partial failure. 2. **Class-attr cleanup symmetry**: ``_teardown_partial_coord_subsystem`` now also clears ``ConsoleCoordinatorUI._coord_mgr`` / ``_collector`` / ``_console_metrics`` to match the lifespan shutdown path (server.py ~line 4629). A failed bootstrap (or test teardown reuse) no longer leaks process-global pointers at a half-built subsystem. 3. **Lifespan startup offload**: the lifespan startup error path used to call ``_teardown_partial_coord_subsystem`` synchronously, which in turn calls ``StateWriter.shutdown(timeout=2.0)`` — a thread-join + sync DB writes that could block the event loop for up to 2s while the console is still coming up. Wrapped the whole load-and-bootstrap in ``asyncio.to_thread`` via the new ``_load_and_bootstrap_coord_subsystem`` synchronous helper, so all blocking work (including any rollback) runs on a worker thread. Mirrors the pattern the regular lifespan shutdown (line ~4620) and the runtime CRUD-triggered path already use. Tests: - ``test_bootstrap_atomic_commit_no_partial_visibility``: a polling thread in tight loop watches ``coord_mgr`` / ``coord_registry`` during a real bootstrap and asserts no observation has ``coord_mgr`` set with ``coord_registry`` still ``None``. - ``test_real_bootstrap_rolls_back_partial_state_on_side_effect_failure``: monkeypatches ``install_idle_nudge_watcher`` to raise mid-build, asserts ``app.state`` shows the clean fresh-install state and the builder-failure error string surfaces ``RuntimeError`` (not the stale "no models" boot-time message).	2026-05-06 23:29:08 -07:00
Patrick Buckley	3143965e00	fix(console): bootstrap coord subsystem on first model add ... A freshly-installed console with no model rows in the DB at boot caught the ``ValueError`` from ``load_model_registry()`` in the lifespan and skipped the entire coord subsystem build, leaving ``coord_mgr`` ``None``. ``_refresh_coord_registry`` then bailed out at ``existing is None`` rather than building the subsystem on first model add — operators had to restart the console after configuring their first model in the admin panel for the "Coordinator subsystem not initialized" banner to clear. Extract the lifespan's coord build into a reusable ``_bootstrap_coord_subsystem`` and add ``_maybe_bootstrap_coord_subsystem`` that runs as an ``asyncio.to_thread`` follow-on after every admin model-CRUD endpoint (create/update/delete/reload). The helper: - fast-paths to a no-op when ``coord_mgr`` is already set; - guards concurrent first-install attempts with ``_COORD_BOOTSTRAP_LOCK`` + double-checked re-test inside the lock; - pre-computes config-derived integers BEFORE any thread starts so ``int(config_store.get(...))`` failures don't strand a started ``StateWriter`` daemon; - stamps ``coord_state_writer`` to ``app.state`` immediately after ``.start()`` so the new ``_teardown_partial_coord_subsystem`` can shut it down on a partial failure (no thread leaks across retries); - atomically commits ``coord_registry`` + clears ``coord_registry_error`` as the final step so callers can rely on the invariant ``coord_registry`` is set iff ``coord_mgr`` is set; - replaces the stale boot-time "no model definitions" message with a builder-failure-specific diagnosis (carrying ``type(exc).__name__``) on construction failure so the dashboard's 503 banner reflects the actual cause. Both the lifespan path and the runtime-bootstrap path now route through the same helper and the same teardown on failure. Tests: 12 new tests covering the helper-level wiring (idempotent fast-path, missing-prereq parametrised over ``config_store`` / ``collector`` / ``console_metrics``, no-rows error recording, builder failure error replacement, partial-state teardown), the endpoint integration, the deterministic concurrent-call lock test (uses an instrumented lock wrapper that signals when a second acquirer arrives, so the test fails fast on slow CI rather than depending on a wall-clock sleep), and a real-builder end-to-end case constructing a working ``SessionManager`` against a real ``ConfigStore`` + real ``ClusterCollector``.	2026-05-06 23:29:08 -07:00
Patrick Buckley	32fd8f29c7	feat(providers): api_surface toggle + mistral medium reasoning fix (#469) ... * feat(providers): api_surface toggle + mistral medium reasoning fix Mistral medium open-weights served by vLLM expects reasoning_effort via the Responses API (`reasoning.effort`), not as a `chat_template_kwargs` entry on Chat Completions. The session was unconditionally injecting `{"reasoning_effort": ...}` into `chat_template_kwargs` for every openai-compatible request, which corrupted the prompt rendering for any backend whose chat template didn't consume that key (Mistral medium, Mistral cloud, Groq, OpenRouter). Changes: - Add `api_surface` ("chat" | "responses") to `ModelConfig.server_compat` and thread it through `create_provider` / `model_registry.get_provider`. `openai-compatible` defaults to Chat Completions; operators can flip individual aliases to Responses for endpoints that support it. - New `vllm-mistral-medium` profile that pre-fills api_surface=responses on Detect for known Mistral medium model ids. - Drop the unconditional `reasoning_effort` injection into `chat_template_kwargs`. Operators running gpt-oss-style local templates that consume `reasoning_effort` from the chat template now opt in via `server_compat.extra_body.chat_template_kwargs`. - New "API Surface" select in the Models admin tab; allowlist-validated server-side at create/update time; pre-filled by Detect via the profile suggestion. - Evict the cached provider singleton in `ModelRegistry.reload()` when api_surface changes (previously only cfg.provider triggered eviction). - Fix `_run_agent` fallback path to inherit the session's primary alias for capability and server_compat resolution; previously the fallback passed `alias=None`, which silently dropped per-model caps on the agent path. Tests: 5117 passed (-m "not live"); ruff + mypy clean. * fix(providers): don't auto-suggest Responses for Mistral medium vLLM's Responses API surface for Mistral medium open-weights doesn't wire up the Mistral tool-call parser as of vLLM 0.x — tool calls leak into the response as ``[TOOL_CALLS]<name>{...}`` text instead of structured tool_calls. Chat Completions on the same engine handles tools cleanly via ``--tool-call-parser mistral``, and reasoning can be turned on via the vLLM CLI ``--reasoning-parser`` flag. Drop the auto-suggest mapping so Detect falls back to the generic ``vllm`` profile. Keep the ``vllm-mistral-medium`` profile definition in place so an operator who specifically wants per-request effort and accepts the tool-calling limitation can still pick "Responses API" manually in the admin UI. * fix(providers): address Copilot review on PR #469 - providers/__init__.py: drop the redundant *_responses_provider / *_chat_provider names; have create_provider use _openai_provider and _openai_compat_provider directly so they're not flagged as unused globals. - console/server.py: tighten _validate_api_surface to a strict equality match against the canonical {"chat", "responses"} set. The previous strip().lower() membership check accepted ' Responses '/'CHAT' but stored the raw string verbatim, which then failed to round-trip through the admin <select>. - console/static/admin.js: gate the entire server_compat block (server type, api_surface, extra_body) on provider == "openai-compatible" at save time so toggling provider away can't leave a stale hidden surface selection in the persisted capabilities JSON. - tests/test_session.py: splat the bad kwarg via **dict so CodeQL no longer flags the call as a wrong-name keyword (the point of the test is the runtime contract, not the static type). - tests/test_admin_model_registry_refresh.py: add endpoint-level tests for the api_surface validation on both create and update — covers the bogus-value rejection, non-canonical-string rejection, and the happy path persisting through to the refreshed registry.	2026-05-03 13:37:50 -07:00
Patrick Buckley	8aef377a57	fix(coord): tighten coord_registry refresh logging + comments per round-2 review ... Three follow-ups from Copilot's round-2 review on #453. ValueError logging surfaced the wrong reason The catch-all ``except ValueError:`` logged ``reason=no_enabled_rows`` unconditionally, but ``ModelRegistry.__init__`` raises ValueError for five distinct config issues (empty models, default / fallback / agent / plan / task alias not present). Operator looking at logs for a config.toml typo would see the wrong cause. Switch to ``log.warning("...reason=%s", exc)`` so the actual error message threads through. Behavior unchanged — existing registry still preserved on every ValueError path. Misleading shutdown() comment The ``finally`` comment claimed shutdown() was closing clients the throwaway registry created during DB load. ``load_model_registry`` only constructs ModelConfigs and the bare ``ModelRegistry(...)``; ``ModelRegistry.__init__`` leaves ``_clients`` / ``_providers`` empty and they populate lazily on first resolve. Today shutdown() iterates empty dicts. Comment now says so explicitly while keeping the call (and its try/except) for forward-compat against an eager-init future. Stale "probe" wording in test docstring ``test_helper_preserves_registry_when_db_probe_fails`` → ``test_helper_preserves_registry_when_strict_load_fails``. The explicit probe was removed in commit 1ba17ed when the helper switched to ``load_model_registry(..., strict=True)``; the test name and docstring still talked about a probe. Updated wording reflects that the loader's strict-mode re-raise is what the helper catches now. 132 tests pass.	2026-04-29 18:25:40 -07:00
Patrick Buckley	e3f2237c36	refactor(coord): hygiene pass on coord_registry refresh — async + selective teardown + test cleanup ... Hygiene follow-ups from the multi-stage code review on #453. perf-1 — sync helper called from async route handlers ``_refresh_coord_registry`` runs two sync DB reads and a registry reload that takes ``_client_lock``; calling it directly from an async handler held the event loop for the duration. All four call sites now ``await asyncio.to_thread(_refresh_coord_registry, ...)``, matching the pattern from commit ``1f7d6ad`` (offloaded ``tenant_check``). perf-3 — ModelRegistry.reload() tore down all clients unconditionally The reload always closed every cached client and provider, even when the changed fields (``model``, ``temperature``, ``context_window``) didn't touch the connection target. Now selective: clients drop only when alias removed or ``(base_url, api_key, provider)`` differs; providers drop only when alias removed or ``provider`` string differs. Keeps connection pools warm across the common admin-edit case where only metadata changed. Two new ``test_model_registry`` cases lock the keep-warm vs drop-on-change behaviour, and the existing ``test_reload_clears_clients`` was updated (it asserted the old overly-aggressive contract) into ``test_reload_keeps_clients_when_connection_target_unchanged``. q-5 — helper rename ``_refresh_console_coord_registry`` → ``_refresh_coord_registry``. The ``console_`` prefix was redundant given the function lives in ``turnstone/console/server.py`` and sibling helpers there (``_notify_nodes_model_reload``, ``_publish_config_change``, ``_collect_model_status``) all omit it. q-1 — shared test middleware ``tests/test_admin_model_registry_refresh`` now imports the header-driven ``_AuthMiddleware`` from ``tests/_coord_test_helpers`` and sets default ``X-Test-User`` / ``X-Test-Perms`` headers on the ``TestClient``. The local hardcoded variant duplicated infrastructure the helper module exists to centralise. q-3 — multi-alias test registry ``_make_registry`` extracted a ``_make_config`` helper and gained an ``extras={alias: model}`` param so multi-alias scenarios stop hand-building ``ModelConfig`` literals. ``test_delete_endpoint_refreshes_registry`` now uses the helper. 310 tests pass across the related coordinator + model surfaces.	2026-04-29 18:25:40 -07:00
Patrick Buckley	70eb50ccb7	test(coord): lock the empty-body gate with a refresh-call spy ... bug-3 / q-2 from the multi-stage review on #453: the previous test ``test_update_endpoint_with_empty_body_does_not_blow_up`` asserted only that the registry's model name was unchanged after an empty PUT, which holds whether or not the refresh ran (DB row matches registry → refresh is idempotent). A regression that always called ``_refresh_console_coord_registry`` — exactly the gate this test was meant to lock — would have left the assertion green. Rename to ``test_update_endpoint_skips_refresh_on_empty_body`` and spy on the helper via ``monkeypatch.setattr``. Empty-body PUT must register zero calls; any future change that drops the ``if updates:`` gate now fails loudly.	2026-04-29 18:25:40 -07:00
Patrick Buckley	6fc2806315	fix(coord): tighten coord_registry refresh — DB probe + accurate boot-from-empty docstring ... Two follow-ups from Copilot review of #453: 1. ``load_model_registry`` swallows storage read errors internally (logs + continues with config.toml-only models). Without a strict probe in the helper, a transient DB outage on an admin CRUD would apply a truncated registry that drops every DB-sourced alias — silently, since the loader returns a non-empty registry built from ``[models.*]`` config.toml entries. Add an explicit ``storage.list_model_definitions(enabled_only=True)`` probe before the loader call so the failure is visible here and the existing registry is preserved on outage. 2. The previous docstring claimed ``admin_model_reload`` "has its own boot-from-empty story." It doesn't — it just calls this helper, which no-ops when ``coord_registry`` is None. When no model rows existed at boot, lifespan leaves the entire coord subsystem uninitialized (no ``coord_mgr``, no ``coord_adapter``, no ``session_factory``), and a console restart remains required after the operator adds the first row. Tighten the docstring to admit that limitation rather than overstating the helper's reach. New test ``test_helper_preserves_registry_when_db_probe_fails`` monkeypatches ``list_model_definitions`` to raise and asserts the existing registry stays intact.	2026-04-29 18:25:40 -07:00
Patrick Buckley	4c6a62933f	fix(coord): auto-refresh console coord_registry on model-definition changes ... The console builds ``app.state.coord_registry`` once at lifespan startup and the coordinator session factory closes over that exact instance. Until now, the model-definition admin endpoints (create/update/delete) wrote to the DB but never touched the in-process registry — and the explicit reload button only fanned out to nodes via HTTP, also leaving the console's own registry stale. Symptom: an operator who changed the underlying model name behind a local-LLM alias (same alias, same endpoint) saw the DB row update immediately, but coordinator sessions kept calling the prior model name until the console process was restarted. Fix: a new helper ``_refresh_console_coord_registry`` rebuilds a fresh ModelRegistry from DB and applies it to ``app.state.coord_registry`` via the existing thread-safe ``ModelRegistry.reload()`` — in-place mutation preserves object identity so the factory closure keeps working, and active coord sessions auto-pick up the swap on their next ``send()`` via ``ChatSession._refresh_model_from_registry``. Wired into four endpoints in ``console/server.py``: - ``admin_create_model_definition`` — after the DB write - ``admin_update_model_definition`` — after the DB write, gated on ``if updates:`` so a no-op PUT skips the rebuild - ``admin_delete_model_definition`` — after the DB write - ``admin_model_reload`` — between ``_publish_config_change`` and ``_notify_nodes_model_reload`` so the console mirrors what the reload broadcasts to nodes Failure isolation: a load or reload error leaves the existing registry intact (logged + swallowed). Coord stays usable while the operator investigates; the explicit reload remains the user-facing recovery path. No node fan-out on CRUD — the explicit reload button continues to gate cluster-wide HTTP propagation, preserving today's UX semantics on shared clusters. Tests in ``tests/test_admin_model_registry_refresh.py`` cover: - helper-level: rebuild from DB, identity preservation, no-op when registry is None, preservation on load failure / no-enabled-rows / reload validation error - endpoint-level: create / update / delete / explicit-reload all refresh the registry; an empty PUT skips the rebuild	2026-04-29 18:25:40 -07:00