From b11b5ece2e68e4e5456fe1e3f364ff7564bdcb8b Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Tue, 21 Jul 2026 19:01:27 -0500 Subject: [PATCH] =?UTF-8?q?fix(tui):=20revision-aware=20reload.mcp=20?= =?UTF-8?q?=E2=80=94=20an=20ack=20now=20means=20the=20revision=20was=20LOA?= =?UTF-8?q?DED?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #20379, finding 1 (High). Two ways an MCP config revision could be silently acknowledged without ever being applied: Client: the poll advanced its accepted mcp_rev BEFORE calling reload.mcp, and quietRpc collapses failures to null — a reload that failed against a temporarily broken server left the revision recorded as applied, and no subsequent poll retried until an unrelated MCP edit. The handshake is now syncMcpReload(): send the observed rev with the request, advance `accepted` only when the server answers status=reloaded (to the server's loaded_rev, falling back to the requested rev on older gateways), and re-compare on EVERY poll tick — decoupled from mtime — so a transient failure heals on the next tick. An in-flight guard stops the 5s poll from stacking requests behind a slow reload. Server: generation-only coalescing let a follower triggered by revision B ack against revision A's registry when the config changed under a slow leader. The leader now re-hashes the MCP-relevant config after discovery and repeats until stable (bounded), records _mcp_reload_loaded_rev, and a follower coalesces only when the revision it was asked to load matches — otherwise it re-runs the full reload itself. Responses carry loaded_rev. Deterministic tests for the exact failure sequences: failed reload → no ack, no generation advance; A-then-B overlap → follower re-runs; matching rev → coalesces; failed leader → follower re-runs; legacy no-rev callers keep generation-only coalescing (thread ordering via an instrumented lock, no sleeps). Client: 6 vitest cases on the ack/retry/in-flight contract. --- tests/tui_gateway/test_mcp_reload_rev.py | 236 +++++++++++++++++++++ tui_gateway/server.py | 104 ++++++--- ui-tui/src/__tests__/useConfigSync.test.ts | 98 ++++++++- ui-tui/src/app/useConfigSync.ts | 83 ++++++-- ui-tui/src/gatewayTypes.ts | 4 + 5 files changed, 478 insertions(+), 47 deletions(-) create mode 100644 tests/tui_gateway/test_mcp_reload_rev.py diff --git a/tests/tui_gateway/test_mcp_reload_rev.py b/tests/tui_gateway/test_mcp_reload_rev.py new file mode 100644 index 000000000000..866473d1a42e --- /dev/null +++ b/tests/tui_gateway/test_mcp_reload_rev.py @@ -0,0 +1,236 @@ +"""reload.mcp revision-aware coalescing (review on #20379, finding 1). + +The TUI's config poll sends the ``mcp_rev`` it observed with each reload +request. The server must guarantee that a success response means THAT +revision (or a newer one) was actually loaded: + +- The leader re-hashes the MCP-relevant config after discovery and repeats + until the hash is stable, so a config edit racing a slow reload can't be + silently skipped. +- A follower that waited behind a leader coalesces only when the leader's + loaded revision matches the follower's requested revision; otherwise it + re-runs the full reload itself. +- A failed reload returns a JSON-RPC error and does not advance the + generation, so a follower behind a failed leader re-runs too. + +Each test file runs in its own subprocess (run_tests.sh isolation), but the +fixtures still restore the module globals they touch. +""" + +from __future__ import annotations + +import threading + +import pytest + +import tools.mcp_tool as mcp_tool +import tui_gateway.server as srv + + +@pytest.fixture() +def reload_env(monkeypatch): + """Neutralize side effects and expose call-counting fakes.""" + calls = {"discover": 0, "shutdown": 0} + rev_box = {"rev": "rev-a"} + + monkeypatch.setattr(mcp_tool, "shutdown_mcp_servers", lambda: calls.__setitem__("shutdown", calls["shutdown"] + 1)) + monkeypatch.setattr(mcp_tool, "discover_mcp_tools", lambda: calls.__setitem__("discover", calls["discover"] + 1)) + monkeypatch.setattr(srv, "_compute_mcp_rev", lambda: rev_box["rev"]) + + saved = (srv._mcp_reload_gen, srv._mcp_reload_loaded_rev) + srv._mcp_reload_gen = 0 + srv._mcp_reload_loaded_rev = "" + yield calls, rev_box + srv._mcp_reload_gen, srv._mcp_reload_loaded_rev = saved + + +def _reload(rev: str | None = None, rid: int = 1) -> dict: + params: dict = {"session_id": "no-such-session", "confirm": True} + if rev is not None: + params["rev"] = rev + return srv._methods["reload.mcp"](rid, params) + + +def test_success_reports_loaded_rev(reload_env): + calls, rev_box = reload_env + rev_box["rev"] = "rev-a" + + envelope = _reload(rev="rev-a") + + assert envelope["result"]["status"] == "reloaded" + assert envelope["result"]["loaded_rev"] == "rev-a" + assert calls["discover"] == 1 + assert srv._mcp_reload_gen == 1 + + +def test_failed_reload_is_an_error_and_no_generation_advance(reload_env, monkeypatch): + """The exact client-facing contract: a failure must NOT look like an ack. + quietRpc on the TUI side collapses this error to null and keeps the + revision un-accepted, so the next poll retries.""" + calls, _ = reload_env + + def _boom(): + raise RuntimeError("flapping server") + + monkeypatch.setattr(mcp_tool, "discover_mcp_tools", _boom) + + envelope = _reload(rev="rev-b") + + assert "error" in envelope + assert srv._mcp_reload_gen == 0 + assert srv._mcp_reload_loaded_rev == "" + + +def test_leader_rehashes_until_stable_when_config_changes_mid_reload(reload_env, monkeypatch): + """Revision A starts a reload; the config changes to revision B while + discovery is connecting servers. The leader must not mark A complete — + it re-hashes after discovery and reloads again until stable, so the + reported loaded_rev is what discovery actually read.""" + calls, _ = reload_env + + # Hash sequence: before pass 1 → rev-a; after pass 1 → rev-b (config + # changed mid-discovery); after pass 2 → rev-b (stable). + hashes = iter(["rev-a", "rev-b", "rev-b"]) + monkeypatch.setattr(srv, "_compute_mcp_rev", lambda: next(hashes)) + + envelope = _reload(rev="rev-a") + + assert envelope["result"]["loaded_rev"] == "rev-b" + assert calls["discover"] == 2 # pass 1 read stale config, pass 2 converged + assert srv._mcp_reload_gen == 1 + + +class _WaiterLock: + """Lock wrapper that signals when a FOLLOWER enters the blocking ``with`` + path. The handler snapshots ``gen_before`` on the line before ``with``, + so once ``waiting`` fires the snapshot is already taken and it is safe to + let the leader complete — no sleep-based ordering.""" + + def __init__(self): + self._lock = threading.Lock() + self.waiting = threading.Event() + + def acquire(self, blocking: bool = True) -> bool: + return self._lock.acquire(blocking) + + def release(self) -> None: + self._lock.release() + + def locked(self) -> bool: + return self._lock.locked() + + def __enter__(self): + self.waiting.set() + self._lock.acquire() + return self + + def __exit__(self, *exc): + self._lock.release() + return False + + +def _run_leader_follower(reload_env, monkeypatch, follower_rev): + """Drive the A-then-B overlap deterministically: the leader blocks inside + discovery until the follower is queued on the lock, then completes.""" + calls, _rev_box = reload_env + + lock = _WaiterLock() + monkeypatch.setattr(srv, "_mcp_reload_lock", lock) + + leader_in_discovery = threading.Event() + release_leader = threading.Event() + + def _slow_discover(): + calls["discover"] += 1 + if calls["discover"] == 1: + leader_in_discovery.set() + assert release_leader.wait(timeout=10) + + monkeypatch.setattr(mcp_tool, "discover_mcp_tools", _slow_discover) + + results: dict = {} + + lt = threading.Thread(target=lambda: results.__setitem__("leader", _reload(rev="rev-a", rid=1)), daemon=True) + lt.start() + assert leader_in_discovery.wait(timeout=10) + + ft = threading.Thread(target=lambda: results.__setitem__("follower", _reload(rev=follower_rev, rid=2)), daemon=True) + ft.start() + # The follower has snapshotted gen_before once it blocks on the lock. + assert lock.waiting.wait(timeout=10) + release_leader.set() + + lt.join(timeout=10) + ft.join(timeout=10) + assert not lt.is_alive() and not ft.is_alive() + + return results, calls + + +def test_follower_with_matching_rev_coalesces(reload_env, monkeypatch): + results, calls = _run_leader_follower(reload_env, monkeypatch, follower_rev="rev-a") + + assert results["leader"]["result"]["status"] == "reloaded" + assert results["follower"]["result"]["status"] == "reloaded" + assert results["follower"]["result"].get("coalesced") is True + # Only the leader ran discovery. + assert calls["discover"] == 1 + + +def test_follower_with_newer_rev_reruns_full_reload(reload_env, monkeypatch): + """The race from the review: the leader loaded revision A, but this + follower was triggered by revision B. Coalescing would ack B against A's + registry — instead the follower must re-run the full reload.""" + results, calls = _run_leader_follower(reload_env, monkeypatch, follower_rev="rev-b") + + assert results["follower"]["result"]["status"] == "reloaded" + assert results["follower"]["result"].get("coalesced") is None + # Leader discovery + follower's own re-run. + assert calls["discover"] == 2 + + +def test_follower_behind_failed_leader_reruns(reload_env, monkeypatch): + """A failed leader never advances the generation — the follower re-runs + the full reload instead of acking over an empty/partial registry.""" + calls, _ = reload_env + + lock = _WaiterLock() + monkeypatch.setattr(srv, "_mcp_reload_lock", lock) + + leader_in_discovery = threading.Event() + release_leader = threading.Event() + + def _discover(): + calls["discover"] += 1 + if calls["discover"] == 1: + leader_in_discovery.set() + assert release_leader.wait(timeout=10) + raise RuntimeError("flapping server") + + monkeypatch.setattr(mcp_tool, "discover_mcp_tools", _discover) + + results: dict = {} + lt = threading.Thread(target=lambda: results.__setitem__("leader", _reload(rev="rev-a", rid=1)), daemon=True) + lt.start() + assert leader_in_discovery.wait(timeout=10) + + ft = threading.Thread(target=lambda: results.__setitem__("follower", _reload(rev="rev-a", rid=2)), daemon=True) + ft.start() + assert lock.waiting.wait(timeout=10) + release_leader.set() + + lt.join(timeout=10) + ft.join(timeout=10) + + assert "error" in results["leader"] + assert results["follower"]["result"]["status"] == "reloaded" + assert calls["discover"] == 2 + + +def test_legacy_request_without_rev_still_coalesces_on_generation(reload_env, monkeypatch): + """Manual /reload-mcp and older clients send no rev — generation-only + coalescing (the pre-existing contract) still applies.""" + results, calls = _run_leader_follower(reload_env, monkeypatch, follower_rev=None) # type: ignore[arg-type] + + assert results["follower"]["result"].get("coalesced") is True + assert calls["discover"] == 1 diff --git a/tui_gateway/server.py b/tui_gateway/server.py index facc6f878253..e2d31d154825 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -12689,23 +12689,7 @@ def _(rid, params: dict) -> dict: # config-change poller uses it to reload MCP servers only when their # config actually changed — a /skin or /statusbar write bumps mtime # but must not cost a multi-second MCP reconnect. - try: - cfg = _load_cfg() - # mcp_servers holds the server DEFINITIONS the classic CLI watches - # for auto-reload (cli.py::_check_config_mcp_changes) — omitting it - # meant editing a server bumped mtime but not mcp_rev, so the TUI - # skipped reload.mcp and new servers never connected until a manual - # /reload-mcp. `mcp` (settings) and `tools` (enable/disable) round - # out the MCP-relevant surface. - rev_src = json.dumps( - {"mcp": cfg.get("mcp"), "mcp_servers": cfg.get("mcp_servers"), "tools": cfg.get("tools")}, - sort_keys=True, - default=str, - ) - mcp_rev = hashlib.sha1(rev_src.encode()).hexdigest()[:12] - except Exception: - mcp_rev = "" - return _ok(rid, {"mtime": mtime, "mcp_rev": mcp_rev}) + return _ok(rid, {"mtime": mtime, "mcp_rev": _compute_mcp_rev()}) return _err(rid, 4002, f"unknown config key: {key}") @@ -12882,14 +12866,46 @@ def _(rid, params: dict) -> dict: # reload.mcp runs on the RPC pool (see _LONG_HANDLERS) so a slow/flapping MCP # server can't freeze the reader thread. Serialize reloads: overlapping # shutdown+discover pairs from stacked config-change polls would interleave -# and leave the registry half-built. Piggyback rather than queue — a reload -# that arrives while one is running would just redo identical work. +# and leave the registry half-built. _mcp_reload_lock = threading.Lock() # Bumped once per SUCCESSFUL shutdown+discover. A follower that waited on the # lock only skips the redundant reload if this advanced while it waited — i.e. # the leader actually completed. If the leader threw (flapping server), the # follower sees no advance and re-runs the full reload itself. _mcp_reload_gen = 0 +# The mcp_rev hash that the last successful reload actually LOADED (config +# re-hashed after discovery, so it reflects what discover_mcp_tools read — +# not what the caller hoped for). A follower coalesces only when the +# revision it was asked to load matches this; otherwise the config changed +# under the leader (rev A loaded, rev B requested) and the follower must +# re-run the full reload itself instead of acking B against A's registry. +_mcp_reload_loaded_rev = "" +# Bounded convergence for a config edit racing a slow reload: the leader +# re-hashes after discovery and repeats until the hash is stable. +_MCP_RELOAD_MAX_PASSES = 3 + + +def _compute_mcp_rev() -> str: + """Hash of the MCP-relevant config sections (server definitions, + settings, toolset enables). ``config.get mtime`` ships it to the TUI so + cosmetic writes don't trigger reloads; ``reload.mcp`` uses it for + revision-aware coalescing. Empty string = unknown (fail open).""" + try: + cfg = _load_cfg() + # mcp_servers holds the server DEFINITIONS the classic CLI watches + # for auto-reload (cli.py::_check_config_mcp_changes) — omitting it + # meant editing a server bumped mtime but not mcp_rev, so the TUI + # skipped reload.mcp and new servers never connected until a manual + # /reload-mcp. `mcp` (settings) and `tools` (enable/disable) round + # out the MCP-relevant surface. + rev_src = json.dumps( + {"mcp": cfg.get("mcp"), "mcp_servers": cfg.get("mcp_servers"), "tools": cfg.get("tools")}, + sort_keys=True, + default=str, + ) + return hashlib.sha1(rev_src.encode()).hexdigest()[:12] + except Exception: + return "" def _finish_reload(rid, params: dict, *, coalesced: bool) -> dict: @@ -12903,7 +12919,7 @@ def _finish_reload(rid, params: dict, *, coalesced: bool) -> dict: except Exception as _exc: logger.warning("Failed to persist mcp_reload_confirm=false: %s", _exc) - payload = {"status": "reloaded"} + payload = {"status": "reloaded", "loaded_rev": _mcp_reload_loaded_rev} if coalesced: payload["coalesced"] = True @@ -12991,28 +13007,49 @@ def _(rid, params: dict) -> dict: ) _emit("session.info", params.get("session_id", ""), _session_info(agent, session)) - global _mcp_reload_gen + global _mcp_reload_gen, _mcp_reload_loaded_rev + + # The revision the CALLER is asking to load (the mcp_rev its poll + # observed). Empty on legacy clients and manual /reload-mcp — those + # coalesce on generation alone, as before. + req_rev = str(params.get("rev") or "") def _do_full_reload() -> None: """shutdown+discover+refresh under the lock, then mark a completed generation. The lock spans the refresh too: releasing after discover would let a second reload tear the registry down while - this one is still reading it to rebuild the session snapshot.""" - global _mcp_reload_gen + this one is still reading it to rebuild the session snapshot. + + Config can change WHILE discover is connecting servers (a slow + reload racing a config edit): re-hash after discovery and repeat + until the hash is stable, so the generation we mark completed + always reflects the config that was actually loaded.""" + global _mcp_reload_gen, _mcp_reload_loaded_rev + + loaded = _compute_mcp_rev() + for _ in range(_MCP_RELOAD_MAX_PASSES): + shutdown_mcp_servers() + discover_mcp_tools() + after = _compute_mcp_rev() + if after == loaded: + break + loaded = after - shutdown_mcp_servers() - discover_mcp_tools() _refresh_session_agent() + _mcp_reload_loaded_rev = loaded _mcp_reload_gen += 1 # Serialize reloads. The LEADER (won the non-blocking acquire) runs the # full reload. A FOLLOWER (lock busy) snapshots the generation, waits, - # then — still holding the lock — checks whether a reload actually - # COMPLETED while it waited: if so it just refreshes its own agent - # against the fresh registry (coalesced); if the leader threw (flapping - # server, no generation advance) it re-runs the full reload itself, so - # a failed leader can never leave a follower reporting a bogus success - # over an empty/partial registry. + # then — still holding the lock — checks whether a reload that + # actually COMPLETED while it waited satisfies ITS request: the + # generation must have advanced (leader didn't throw) AND the loaded + # revision must match the one this follower was asked to apply. Both + # true → just refresh its own agent against the fresh registry + # (coalesced). Leader threw, or leader loaded an older revision than + # this request observed → re-run the full reload, so a failed or + # stale leader can never leave a follower acking a revision that was + # never loaded. if _mcp_reload_lock.acquire(blocking=False): try: _do_full_reload() @@ -13024,7 +13061,10 @@ def _(rid, params: dict) -> dict: gen_before = _mcp_reload_gen with _mcp_reload_lock: - if _mcp_reload_gen > gen_before: + leader_completed = _mcp_reload_gen > gen_before + rev_satisfied = not req_rev or req_rev == _mcp_reload_loaded_rev + + if leader_completed and rev_satisfied: _refresh_session_agent() coalesced = True else: diff --git a/ui-tui/src/__tests__/useConfigSync.test.ts b/ui-tui/src/__tests__/useConfigSync.test.ts index e760e8d748a1..4338541c6235 100644 --- a/ui-tui/src/__tests__/useConfigSync.test.ts +++ b/ui-tui/src/__tests__/useConfigSync.test.ts @@ -7,7 +7,9 @@ import { normalizeBusyInputMode, normalizeIndicatorStyle, normalizeMouseTracking, - normalizeStatusBar + normalizeStatusBar, + syncMcpReload, + type McpRevState } from '../app/useConfigSync.js' describe('applyDisplay', () => { @@ -372,6 +374,100 @@ describe('applyDisplay → voice.record_key (#18994)', () => { }) }) +// Review on #20379 (finding 1): an MCP config revision must never be acked +// before the server confirms it was LOADED. The old poll advanced its +// accepted revision first and fired reload.mcp second — a reload that failed +// (quietRpc → null) left the revision recorded as applied, and no subsequent +// poll retried it until an unrelated MCP edit. +describe('syncMcpReload (revision-aware ack)', () => { + const gwOk = (payload: unknown) => ({ request: vi.fn(() => Promise.resolve(payload)), on: vi.fn(), off: vi.fn() }) as any + + const freshState = (accepted = 'rev-a'): McpRevState => ({ accepted, inFlight: false }) + + it('advances accepted only after the server confirms the reload', async () => { + const gw = gwOk({ status: 'reloaded', loaded_rev: 'rev-b' }) + const state = freshState() + const onReloaded = vi.fn() + + await syncMcpReload(gw, 's1', 'rev-b', state, onReloaded) + + expect(gw.request).toHaveBeenCalledWith('reload.mcp', { confirm: true, rev: 'rev-b', session_id: 's1' }) + expect(state.accepted).toBe('rev-b') + expect(onReloaded).toHaveBeenCalledTimes(1) + }) + + it('does NOT advance accepted when the reload RPC fails — next poll retries', async () => { + const gw = { request: vi.fn(() => Promise.reject(new Error('flapping server'))), on: vi.fn(), off: vi.fn() } as any + const state = freshState() + const onReloaded = vi.fn() + + await syncMcpReload(gw, 's1', 'rev-b', state, onReloaded) + + // The exact failure sequence from the review: reload fails, revision + // must remain un-acked so the next tick retries it. + expect(state.accepted).toBe('rev-a') + expect(state.inFlight).toBe(false) + expect(onReloaded).not.toHaveBeenCalled() + + // Next poll tick: the server recovered — the SAME revision goes through. + gw.request = vi.fn(() => Promise.resolve({ status: 'reloaded', loaded_rev: 'rev-b' })) + await syncMcpReload(gw, 's1', 'rev-b', state, onReloaded) + expect(state.accepted).toBe('rev-b') + expect(onReloaded).toHaveBeenCalledTimes(1) + }) + + it('does not advance on confirm_required (reload did not happen)', async () => { + const gw = gwOk({ message: 'confirm first', status: 'confirm_required' }) + const state = freshState() + + await syncMcpReload(gw, 's1', 'rev-b', state) + + expect(state.accepted).toBe('rev-a') + }) + + it('records the server-reported loaded_rev, not the requested rev', async () => { + // A config edit raced the reload: the server re-hashed after discovery + // and loaded rev-c. Recording rev-c (not rev-b) makes the next poll a + // no-op instead of an immediate redundant reload. + const gw = gwOk({ status: 'reloaded', loaded_rev: 'rev-c' }) + const state = freshState() + + await syncMcpReload(gw, 's1', 'rev-b', state) + + expect(state.accepted).toBe('rev-c') + }) + + it('is a no-op when the revision is already accepted or empty', async () => { + const gw = gwOk({ status: 'reloaded' }) + const state = freshState() + + await syncMcpReload(gw, 's1', 'rev-a', state) + await syncMcpReload(gw, 's1', '', state) + + expect(gw.request).not.toHaveBeenCalled() + }) + + it('does not stack requests while one is in flight', async () => { + let resolveFirst!: (v: unknown) => void + const gw = { + request: vi.fn(() => new Promise(res => (resolveFirst = res))), + on: vi.fn(), + off: vi.fn() + } as any + const state = freshState() + + const first = syncMcpReload(gw, 's1', 'rev-b', state) + + // Second tick while the first RPC is outstanding: swallowed. + await syncMcpReload(gw, 's1', 'rev-b', state) + expect(gw.request).toHaveBeenCalledTimes(1) + + resolveFirst({ status: 'reloaded', loaded_rev: 'rev-b' }) + await first + expect(state.accepted).toBe('rev-b') + }) +}) + // Round-12 Copilot review regression on #19835: the live mtime-reload // path was previously untested, so a regression in the polling/RPC // wiring to applyDisplay would only be visible at runtime. The fetch diff --git a/ui-tui/src/app/useConfigSync.ts b/ui-tui/src/app/useConfigSync.ts index f5f0a525f793..6a1e5032111c 100644 --- a/ui-tui/src/app/useConfigSync.ts +++ b/ui-tui/src/app/useConfigSync.ts @@ -129,6 +129,56 @@ const quietRpc = async = Record>( } } +// ── MCP revision handshake ─────────────────────────────────────────── +// +// The poll must not ack an MCP config revision until the server confirms it +// actually LOADED it. Advancing `accepted` before the reload succeeds loses +// revisions permanently: quietRpc collapses a failed reload to null, the +// next poll sees the same mcp_rev, and the new config never applies until +// an unrelated MCP edit. So `accepted` only moves on a confirmed reload — +// to the server's loaded_rev (what discovery actually read), falling back +// to the requested rev for older gateways. Retries are decoupled from +// mtime: every poll re-compares, so a transiently broken server heals on +// the next tick. + +export interface McpRevState { + /** Last revision the server CONFIRMED it loaded (or boot baseline). */ + accepted: string + /** A reload RPC is outstanding — don't stack another every poll tick. */ + inFlight: boolean +} + +export const syncMcpReload = async ( + gw: GatewayClient, + sid: string, + nextMcpRev: string, + state: McpRevState, + onReloaded?: () => void +): Promise => { + if (!nextMcpRev || nextMcpRev === state.accepted || state.inFlight) { + return + } + + state.inFlight = true + + try { + const r = await quietRpc(gw, 'reload.mcp', { + confirm: true, + rev: nextMcpRev, + session_id: sid + }) + + if (r?.status === 'reloaded') { + state.accepted = String(r.loaded_rev || nextMcpRev) + onReloaded?.() + } + // Failure (null) or confirm_required: leave `accepted` unchanged so the + // next poll tick retries the same revision. + } finally { + state.inFlight = false + } +} + const _voiceRecordKeyFromConfig = (cfg: ConfigFullResponse | null): ParsedVoiceRecordKey => { const raw = cfg?.config?.voice?.record_key @@ -245,7 +295,7 @@ export function useConfigSync({ sid }: UseConfigSyncOptions) { const mtimeRef = useRef(0) - const mcpRevRef = useRef('') + const mcpRevRef = useRef({ accepted: '', inFlight: false }) useEffect(() => { if (!sid) { @@ -261,9 +311,9 @@ export function useConfigSync({ mtimeRef.current = Number(r?.mtime ?? 0) // Seed the MCP revision baseline too: after a normal boot mtime is // already non-zero, so the poller's baseline branch never runs, and an - // unset mcpRevRef would make the FIRST cosmetic write (mtime bump, same + // unset baseline would make the FIRST cosmetic write (mtime bump, same // mcp_rev) look like an MCP change and fire a needless reload.mcp. - mcpRevRef.current = String(r?.mcp_rev ?? '') + mcpRevRef.current.accepted = String(r?.mcp_rev ?? '') }) void hydrateFullConfig(gw, setBellOnComplete, setVoiceRecordKey) }, [gw, setBellOnComplete, setVoiceEnabled, setVoiceRecordKey, sid]) @@ -281,28 +331,33 @@ export function useConfigSync({ if (!mtimeRef.current) { if (next) { mtimeRef.current = next - mcpRevRef.current = nextMcpRev + mcpRevRef.current.accepted = nextMcpRev } return } + // Reload MCP only when the MCP-relevant config actually changed. + // Cosmetic writes (/skin, /statusbar, /theme) bump mtime constantly; + // reconnecting every MCP server for those costs seconds and made + // skin switching feel glacial. The handshake runs on EVERY poll tick + // (not just mtime changes) so a failed reload retries until the + // server confirms the revision was loaded. + if (nextMcpRev) { + void syncMcpReload(gw, sid, nextMcpRev, mcpRevRef.current, () => + turnController.pushActivity('MCP reloaded after config change') + ) + } + if (!next || next === mtimeRef.current) { return } mtimeRef.current = next - // Reload MCP only when the MCP-relevant config actually changed. - // Cosmetic writes (/skin, /statusbar, /theme) bump mtime constantly; - // reconnecting every MCP server for those costs seconds and made - // skin switching feel glacial. Older gateways don't send mcp_rev — - // fall back to reload-on-any-change there. - const mcpChanged = !nextMcpRev || nextMcpRev !== mcpRevRef.current - - mcpRevRef.current = nextMcpRev - - if (mcpChanged) { + // Older gateways don't send mcp_rev — fall back to + // reload-on-any-change there (no ack tracking possible). + if (!nextMcpRev) { quietRpc(gw, 'reload.mcp', { session_id: sid, confirm: true }).then( r => r && turnController.pushActivity('MCP reloaded after config change') ) diff --git a/ui-tui/src/gatewayTypes.ts b/ui-tui/src/gatewayTypes.ts index a4bbe04a8971..649561728702 100644 --- a/ui-tui/src/gatewayTypes.ts +++ b/ui-tui/src/gatewayTypes.ts @@ -436,6 +436,10 @@ export interface ModelOptionsResponse { export interface ReloadMcpResponse { status?: string message?: string + /** The mcp_rev the server actually loaded (re-hashed after discovery). + * The client records THIS as its accepted revision, not the one it + * requested — a reload that raced a config edit reports the newer rev. */ + loaded_rev?: string } export interface ReloadEnvResponse {