diff --git a/CHANGELOG.md b/CHANGELOG.md index ff1a8053..c2e151c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -202,7 +202,15 @@ Earlier stable lines (`stable/1.6`, `stable/1.5`) are frozen. stranding a live session and published catalog with the config already gone), and `reconcile_sync` retries a removal that timed out instead of marking it done — previously a DB-driven delete of a busy server could be a - silent, permanent no-op until process restart. + silent, permanent no-op until process restart. A refresh outcome now + threads consistently to every operator surface off one source of truth + (the per-server `last_refresh_outcome`): a busy-skip and a genuine failure + are each reported distinctly from a real "no changes" — `/mcp refresh` + prints "skipped" or "failed" rather than a false "no changes", the + node-internal refresh endpoint returns `202 skipped` instead of a + misleading `200 ok` for a refresh that never ran, and the admin console's + refresh pill paints a benign "skipped" in a neutral tint rather than + failure-red. - **OpenAI Responses streaming: truncated and refused responses no longer vanish.** A response that hit `max_output_tokens` terminates the stream diff --git a/tests/test_mcp_admin_api.py b/tests/test_mcp_admin_api.py index 78418ee8..20577dfb 100644 --- a/tests/test_mcp_admin_api.py +++ b/tests/test_mcp_admin_api.py @@ -2578,6 +2578,35 @@ class TestInternalMcpRefreshOneEndpoint: assert "url" not in data["server"] assert data["server"]["circuit_open"] is True + def test_refresh_one_skipped_returns_202(self, node_app_factory) -> None: + # A busy-lock skip never ran the refresh — it must NOT be reported + # as 200 "ok" (the caller would believe the catalog is current). + # 202 Accepted + status "skipped": the health-tick retry will run it. + mgr = MagicMock() + mgr.refresh_sync.return_value = {"srv": None} + # The endpoint reads the outcome from the manager accessor, not the + # stripped status (the public projection whitelists it out). + mgr.last_refresh_outcome.return_value = "skipped" + mgr.get_server_status.return_value = { + "connected": True, + "tools": 3, + "resources": 0, + "prompts": 1, + "error": "", + "transport": "stdio", + "command": "secret", + "url": "", + "circuit_open": False, + "consecutive_failures": 0, + } + c = node_app_factory(mgr) + r = c.post("/v1/api/_internal/mcp-refresh/srv") + assert r.status_code == 202 + data = r.json() + assert data["status"] == "skipped" + assert "command" not in data["server"] # stripped + mgr.last_refresh_outcome.assert_called_with("srv") + def test_refresh_one_invalid_name_returns_400(self, node_app_factory) -> None: # sec-4: name validation symmetric with console side. mgr = MagicMock() diff --git a/tests/test_mcp_client.py b/tests/test_mcp_client.py index 849e5bab..e78507ce 100644 --- a/tests/test_mcp_client.py +++ b/tests/test_mcp_client.py @@ -1499,6 +1499,36 @@ class TestSessionRefresh: session.ui.on_error.assert_called_once() assert "MCP refresh failed" in session.ui.on_error.call_args[0][0] + def test_mcp_refresh_renders_skip_distinctly(self, tmp_db): + # A None result (busy-skip) with a "skipped" outcome must NOT render + # as "no changes" — the operator would think the server is current. + mock_mcp = MagicMock() + mock_mcp.get_tools.return_value = [_fake_openai_tool()] + mock_mcp.server_names = ["srv"] + mock_mcp.refresh_sync.return_value = {"srv": None} + mock_mcp.last_refresh_outcome.return_value = "skipped" + session = self._make_session(mcp_client=mock_mcp) + + session.handle_command("/mcp refresh") + rendered = session.ui.on_info.call_args[0][0] + assert "skipped" in rendered + assert "no changes" not in rendered + + def test_mcp_refresh_renders_failure_distinctly(self, tmp_db): + # A None result whose outcome is an error must render as a failure, + # NOT as "no changes" (the pre-#839 lie the None sentinel closes). + mock_mcp = MagicMock() + mock_mcp.get_tools.return_value = [_fake_openai_tool()] + mock_mcp.server_names = ["srv"] + mock_mcp.refresh_sync.return_value = {"srv": None} + mock_mcp.last_refresh_outcome.return_value = "error:ConnectionError" + session = self._make_session(mcp_client=mock_mcp) + + session.handle_command("/mcp refresh") + rendered = session.ui.on_info.call_args[0][0] + assert "failed" in rendered + assert "no changes" not in rendered + # --------------------------------------------------------------------------- # MCP Resources @@ -2593,12 +2623,17 @@ class TestCircuitBreaker: mock_session.list_prompts = AsyncMock(side_effect=dead) _seed_static_state(mgr, "test", session=mock_session) - await mgr._refresh_all("test") + results = await mgr._refresh_all("test") # Dead session evicted → next refresh tick / dispatch reconnects. assert mgr._static_servers["test"].session is None ts, outcome = mgr._last_refresh["test"] assert outcome == "error:ClosedResourceError" + # A FAILURE reports None, NOT ([], []) — rendering a failed + # refresh as "no changes" is the operator lie the sentinel + # closes; the outcome disambiguates None (error vs skipped). + assert results["test"] is None + assert mgr.last_refresh_outcome("test") == "error:ClosedResourceError" asyncio.run(_run()) @@ -3358,6 +3393,32 @@ class TestStaticNotificationRefresh: "a same-window different-kind notification must spawn its own refresh" ) + def test_scheduling_failure_rolls_back_stamp_and_marker(self, running_loop_mgr) -> None: + """If _spawn_background raises (loop tearing down), BOTH the coalesce + marker and the debounce stamp we just wrote must be rolled back — + else a same-kind push landing in the window afterward is debounced + against a stamp for a refresh that never spawned, and (on the pool + path, no on_debounce_drop) never recovers.""" + mgr, loop, _thread = running_loop_mgr + handler = mgr._make_static_notification_handler("srv") + + def _boom(coro: Any, _label: str) -> None: + coro.close() # avoid "coroutine was never awaited" — prod is loop-teardown + raise RuntimeError("loop closing") + + async def _fire() -> None: + mgr._static_connect_lock_for("srv") + _seed_static_state(mgr, "srv", session=MagicMock()) + note = mcp_types.ServerNotification( + mcp_types.ToolListChangedNotification(method="notifications/tools/list_changed") + ) + with patch.object(mgr, "_spawn_background", side_effect=_boom): + await handler(note) # must swallow the scheduling failure + + _run_on_loop(loop, _fire()) + assert ("srv", "tools") not in mgr._last_notification_refresh, "stamp must roll back" + assert ("srv", "tools") not in mgr._static_refresh_pending, "marker must roll back" + def test_same_kind_debounce_drop_arms_retry(self, running_loop_mgr) -> None: """A SAME-kind push landing in the debounce window AFTER the previous runner completed (no runner queued) is genuinely lost to diff --git a/turnstone/console/static/admin.js b/turnstone/console/static/admin.js index 60fe1427..7512851a 100644 --- a/turnstone/console/static/admin.js +++ b/turnstone/console/static/admin.js @@ -4811,8 +4811,13 @@ function _renderMcpServers(items) { ageShort = Math.floor(ageSeconds / 3600) + "h"; else ageShort = Math.floor(ageSeconds / 86400) + "d"; const outcomeText = newestRefreshOutcome || "unknown"; - const pillCls = - outcomeText === "ok" ? "mcp-refresh-pill-ok" : "mcp-refresh-pill-err"; + // "skipped" is benign (lock busy, retry scheduled) — a neutral info + // pill, not the error-red any-non-ok used to paint; only genuine + // "error:*" outcomes are failures. + let pillCls; + if (outcomeText === "ok") pillCls = "mcp-refresh-pill-ok"; + else if (outcomeText === "skipped") pillCls = "mcp-refresh-pill-skip"; + else pillCls = "mcp-refresh-pill-err"; const pillTitle = "Last refresh " + new Date(newestRefreshAt * 1000).toISOString() + diff --git a/turnstone/console/static/style.css b/turnstone/console/static/style.css index 0be5166c..986d3371 100644 --- a/turnstone/console/static/style.css +++ b/turnstone/console/static/style.css @@ -2823,6 +2823,13 @@ h3.skill-spec-heading { background: color-mix(in srgb, var(--warn) 15%, transparent); color: var(--warn); } +/* "skipped" is benign and transient (the server's lock was busy; a retry + is scheduled) — an info tint, NOT the warn red the error pill uses, so a + healthy busy server is not painted as failed. */ +.mcp-refresh-pill-skip { + background: color-mix(in srgb, var(--accent) 15%, transparent); + color: var(--accent); +} .mcp-detail-modal::before { background: linear-gradient( diff --git a/turnstone/core/mcp_client.py b/turnstone/core/mcp_client.py index 4a583ce1..81a4ed32 100644 --- a/turnstone/core/mcp_client.py +++ b/turnstone/core/mcp_client.py @@ -1674,12 +1674,16 @@ class MCPClientManager: f"{label_prefix} {kind} refresh for '{label_target}'", ) except Exception as exc: - # Scheduling failed (loop shutting down) — release the - # coalesce marker or this key+kind never refreshes again. - # Structured fields only — ``exc_info=True`` would serialize - # the chained ``httpx.Request`` whose headers carry the - # bearer (configured for auth_type=static, minted for pool). + # Scheduling failed (loop shutting down) — release BOTH the + # coalesce marker and the debounce stamp we just wrote, or a + # same-kind push landing inside the window afterward is + # debounced against a stamp for a refresh that never spawned + # (the pool path has no on_debounce_drop recovery). Structured + # fields only — ``exc_info=True`` would serialize the chained + # ``httpx.Request`` whose headers carry the bearer (configured + # for auth_type=static, minted for pool). pending.discard(marker) + stamps.pop(marker, None) log.warning( "list_changed refresh scheduling failed for %s exc=%s", origin, @@ -4292,6 +4296,19 @@ class MCPClientManager: self._set_error(name, f"Refresh failed: {type(exc).__name__}: {exc}") self._arm_refresh_retry(name) + def last_refresh_outcome(self, name: str) -> str | None: + """Authoritative last-refresh outcome for static server *name*. + + One of ``"ok"``, ``"skipped"``, ``"error:"``, or ``None`` + (no refresh since start). The single source of truth every + operator surface renders from — the ``refresh_sync`` diff tuple + says only WHAT changed, not whether the pass ran, was skipped, or + failed. Static-only (reads ``_last_refresh`` directly); no + oauth_user status routing. + """ + row = self._last_refresh.get(name) + return row[1] if row is not None else None + def _arm_refresh_retry(self, name: str) -> None: """Arm the health-tick refresh retry for *name* — the ONE gate copy. @@ -4480,14 +4497,23 @@ class MCPClientManager: """Refresh tools, resources, and prompts for one or all servers. For disconnected servers (in config but not connected), attempts - reconnect. Returns ``{server: (added, removed)}`` per server — - or ``None`` for a server whose refresh was SKIPPED (another - operation held its lock, or it was removed/evicted mid-pass). - ``None`` is a distinct shape on purpose: rendering a skip as - ``([], [])`` told the operator "refreshed, no changes" for a - server that was never refreshed at all. Skips on live servers - also stamp a ``skipped`` ``_last_refresh`` row so the admin pill - tells the same story. + reconnect. Per-server value: + + * ``(added, removed)`` — the refresh RAN to completion; the tuple + is the tool diff (``([], [])`` = ran, no changes). Outcome + ``ok``. + * ``None`` — the refresh produced NO catalog, either SKIPPED + (another operation held the lock, or removed/evicted mid-pass; + outcome ``skipped``) or FAILED (it ran and errored; outcome + ``error:``). Consumers disambiguate via the + ``last_refresh_outcome`` status field. + + ``None`` is a distinct shape on purpose: rendering either a skip + or a failure as ``([], [])`` told the operator "refreshed, no + changes" for a server that was never refreshed / whose refresh + failed. The ``_last_refresh`` status row carries the authoritative + outcome for every operator surface (CLI, HTTP endpoint, admin + pill) so they render consistently off ONE source of truth. """ results: dict[str, tuple[list[str], list[str]] | None] = {} targets = [server_name] if server_name else list(self._server_configs.keys()) @@ -4564,7 +4590,13 @@ class MCPClientManager: # abort the whole refresh pass. Redaction + retry-arm live # in the shared helper. self._record_refresh_failure(name, exc, context="Refresh") - results[name] = ([], []) + # None, NOT ``([], [])``: a failure produced no catalog, and + # ``([], [])`` is reserved for "ran, no changes" — rendering + # a failure as "no changes" is the operator lie the sentinel + # exists to prevent. Consumers disambiguate None (skipped vs + # failed) via ``last_refresh_outcome``, which the + # ``error:`` write at the end of this block sets. + results[name] = None # A dead transport leaves a non-None but unusable session, so # the reconnect branch at the top of this loop (gated on # ``session is None``) would never fire and every later pass @@ -4605,13 +4637,13 @@ class MCPClientManager: ) -> dict[str, tuple[list[str], list[str]] | None]: """Refresh tools synchronously (blocks the calling thread). - Returns ``{server: (added_names, removed_names)}`` per server, or - ``{server: None}`` when that server's refresh was SKIPPED (its - lock was held by a concurrent reconnect/refresh, or it was - removed mid-pass). Callers MUST distinguish ``None`` from - ``([], [])``: the latter is a refresh that ran and found no - changes; ``None`` is a refresh that never ran, deferred to the - health-tick retry. + Returns ``{server: (added_names, removed_names)}`` for a refresh + that RAN (``([], [])`` = no changes), or ``{server: None}`` when + it produced no catalog — SKIPPED (lock held by a concurrent + reconnect/refresh, or removed mid-pass) or FAILED. Callers MUST + NOT render ``None`` as "no changes"; consult + :meth:`last_refresh_outcome` (or the status ``last_refresh_outcome`` + field) to distinguish ``skipped`` from ``error:``. """ assert self._loop is not None future = asyncio.run_coroutine_threadsafe(self._refresh_all(server_name), self._loop) diff --git a/turnstone/core/session.py b/turnstone/core/session.py index add7b520..364450f5 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -3059,12 +3059,17 @@ class ChatSession: lines: list[str] = [] for srv, diff in sorted(results.items()): if diff is None: - # Skipped, NOT refreshed: another operation held the - # server's lock (a reconnect / a push refresh), or it was - # removed mid-pass. Reported distinctly from "no changes" - # so the operator isn't told a stale server is current; - # the health-tick retry runs the real refresh shortly. - lines.append(f" {srv}: {dim('skipped (busy — retry scheduled)')}") + # No catalog produced — the refresh either was SKIPPED + # (lock held by a concurrent reconnect/push refresh, or + # removed mid-pass) or FAILED. Disambiguate via the + # authoritative outcome so the operator is never told a + # stale/broken server is current. Both are distinct from + # "no changes". + outcome = self._mcp_client.last_refresh_outcome(srv) or "" + if outcome.startswith("error"): + lines.append(f" {srv}: {RED}refresh failed{RESET} ({dim(outcome)})") + else: + lines.append(f" {srv}: {dim('skipped (busy — retry scheduled)')}") continue added, removed = diff if added or removed: diff --git a/turnstone/server.py b/turnstone/server.py index 149ced66..8b37e444 100644 --- a/turnstone/server.py +++ b/turnstone/server.py @@ -3459,9 +3459,19 @@ def internal_mcp_refresh_one(request: Request) -> JSONResponse: return JSONResponse({"status": "error", "error": "refresh failed"}, status_code=500) # _refresh_all swallows per-server errors into _last_error rather than - # raising, so a 200-OK from refresh_sync isn't enough — re-check status - # and surface 500 if the refresh actually failed for this server. + # raising, so a 200-OK from refresh_sync isn't enough — re-check the + # authoritative outcome. A refresh can (a) run and fail → 500, (b) be + # SKIPPED because the server's connect lock was busy (a reconnect / a + # push refresh already running) → 202 with status "skipped", the + # health-tick retry will run it, or (c) run cleanly → 200 ok. Reporting + # a skip as 200 "ok" told the caller the catalog is current when + # nothing ran. The outcome is read from the manager directly — the + # public status projection whitelists ``last_refresh_outcome`` out (it + # encodes the error class, which read-scope deliberately coarsens to + # ``has_error``), so it cannot be recovered from the stripped dict. status = _public_server_status(mcp_mgr, name) + if mcp_mgr.last_refresh_outcome(name) == "skipped": + return JSONResponse({"status": "skipped", "server": status}, status_code=202) if status.get("error"): log.warning( "internal_mcp_refresh_one: refresh reported error for %s: %s", name, status["error"]