mirror of
https://github.com/andrewyng/openworker.git
synced 2026-09-14 08:04:32 +00:00
MCP failure notice fires on state change, not every session
First session after a server breaks (or breaks differently) gets the transcript notice; unchanged-broken stays quiet (Connectors carries it). Personas declaring the server keep the every-session notice. Dedupe persists across restarts; recovery re-arms it.
This commit is contained in:
@@ -1193,6 +1193,9 @@ class SessionManager:
|
|||||||
try:
|
try:
|
||||||
conn = await self.mcp.ensure(server)
|
conn = await self.mcp.ensure(server)
|
||||||
self._mcp_errors.pop(server.name, None)
|
self._mcp_errors.pop(server.name, None)
|
||||||
|
# Recovery resets the notice dedupe: if this server breaks again
|
||||||
|
# later, the next session gets a fresh transcript notice.
|
||||||
|
self._clear_mcp_notified(server.name)
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
if mcp_oauth.is_auth_required(exc):
|
if mcp_oauth.is_auth_required(exc):
|
||||||
# Stored tokens no longer refresh (vendor rotated/expired
|
# Stored tokens no longer refresh (vendor rotated/expired
|
||||||
@@ -1218,9 +1221,19 @@ class SessionManager:
|
|||||||
logger.warning(
|
logger.warning(
|
||||||
"mcp %s failed to connect: %s", server.name, msg[:500]
|
"mcp %s failed to connect: %s", server.name, msg[:500]
|
||||||
)
|
)
|
||||||
self._mcp_session_failures.setdefault(session_id, []).append(
|
# Transcript notice on state CHANGE, not state (owner ruling
|
||||||
server.name
|
# 2026-08-21): a continuously-broken server stamps only the first
|
||||||
)
|
# session after it breaks (or breaks differently) — the Connectors
|
||||||
|
# page carries the standing error. Personas that DECLARE the server
|
||||||
|
# in their manifest keep the every-session notice: for them the
|
||||||
|
# missing tools are material every time (the 2026-08-20 drill case).
|
||||||
|
declared = persona_mcp is not None and server.name in persona_mcp
|
||||||
|
if declared or self._should_notify_mcp_failure(
|
||||||
|
server.name, self._mcp_errors.get(server.name, "")
|
||||||
|
):
|
||||||
|
self._mcp_session_failures.setdefault(session_id, []).append(
|
||||||
|
server.name
|
||||||
|
)
|
||||||
continue
|
continue
|
||||||
callables = build_callables(
|
callables = build_callables(
|
||||||
server,
|
server,
|
||||||
@@ -1239,6 +1252,21 @@ class SessionManager:
|
|||||||
out.extend(callables)
|
out.extend(callables)
|
||||||
return out
|
return out
|
||||||
|
|
||||||
|
def _should_notify_mcp_failure(self, name: str, error: str) -> bool:
|
||||||
|
"""True once per failure episode: the first session after `name` starts
|
||||||
|
failing (or its error text changes) notices; unchanged-broken stays quiet.
|
||||||
|
Persisted in prefs so an app relaunch doesn't re-stamp the same complaint."""
|
||||||
|
notified = self._prefs.setdefault("mcp_notified_errors", {})
|
||||||
|
if notified.get(name) == error:
|
||||||
|
return False
|
||||||
|
notified[name] = error
|
||||||
|
self._save_prefs()
|
||||||
|
return True
|
||||||
|
|
||||||
|
def _clear_mcp_notified(self, name: str) -> None:
|
||||||
|
if self._prefs.get("mcp_notified_errors", {}).pop(name, None) is not None:
|
||||||
|
self._save_prefs()
|
||||||
|
|
||||||
def pop_mcp_failures(self, session_id: str) -> list[tuple[str, Optional[str]]]:
|
def pop_mcp_failures(self, session_id: str) -> list[tuple[str, Optional[str]]]:
|
||||||
"""Drain (name, error) for servers that failed while preparing this session's
|
"""Drain (name, error) for servers that failed while preparing this session's
|
||||||
tools — consumed once by the WS handler to append a transcript notice."""
|
tools — consumed once by the WS handler to append a transcript notice."""
|
||||||
@@ -1337,6 +1365,7 @@ class SessionManager:
|
|||||||
# survive an app restart, so it lives in prefs, not memory.
|
# survive an app restart, so it lives in prefs, not memory.
|
||||||
self._prefs.setdefault("mcp_last_test", {})[name] = int(time.time())
|
self._prefs.setdefault("mcp_last_test", {})[name] = int(time.time())
|
||||||
self._save_prefs()
|
self._save_prefs()
|
||||||
|
self._clear_mcp_notified(name)
|
||||||
return {"ok": True, "tools": len(conn.tools)}
|
return {"ok": True, "tools": len(conn.tools)}
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
if (
|
if (
|
||||||
@@ -1425,6 +1454,7 @@ class SessionManager:
|
|||||||
self._mcp_auth_hints.discard(name)
|
self._mcp_auth_hints.discard(name)
|
||||||
if self._prefs.get("mcp_last_test", {}).pop(name, None) is not None:
|
if self._prefs.get("mcp_last_test", {}).pop(name, None) is not None:
|
||||||
self._save_prefs()
|
self._save_prefs()
|
||||||
|
self._clear_mcp_notified(name)
|
||||||
# Removing a server must not leave its connection running until the next
|
# Removing a server must not leave its connection running until the next
|
||||||
# restart, nor its OAuth tokens + DCR registration in the secret store —
|
# restart, nor its OAuth tokens + DCR registration in the secret store —
|
||||||
# "Remove" is the user saying this server is GONE (owner review 2026-08-21).
|
# "Remove" is the user saying this server is GONE (owner review 2026-08-21).
|
||||||
|
|||||||
@@ -496,3 +496,86 @@ def test_delete_mcp_shuts_down_connection_and_forgets_tokens(tmp_path):
|
|||||||
assert res["ok"]
|
assert res["ok"]
|
||||||
assert conn.shutdown.is_set()
|
assert conn.shutdown.is_set()
|
||||||
assert mgr.secrets.get(mcp_oauth.PROFILE_PREFIX + "gone-srv") is None
|
assert mgr.secrets.get(mcp_oauth.PROFILE_PREFIX + "gone-srv") is None
|
||||||
|
|
||||||
|
|
||||||
|
# -- notice dedupe: state CHANGE, not state (owner ruling 2026-08-21) -----------
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_session_notice_fires_once_per_failure_episode(tmp_path, monkeypatch):
|
||||||
|
"""A continuously-broken server stamps only the FIRST session after it breaks;
|
||||||
|
a changed error re-notices; recovery then re-breakage re-notices. The
|
||||||
|
Connectors page carries the standing error in between."""
|
||||||
|
from types import SimpleNamespace
|
||||||
|
|
||||||
|
from coworker.server import SessionManager
|
||||||
|
|
||||||
|
mgr = SessionManager(data_dir=tmp_path / "data")
|
||||||
|
server = SimpleNamespace(
|
||||||
|
name="flaky", transport="stdio", url=None, auth=None, enabled=True, include_tools=None, exclude_tools=None, requires_approval=True
|
||||||
|
)
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"coworker.server.manager.load_mcp_servers", lambda *a, **k: [server]
|
||||||
|
)
|
||||||
|
|
||||||
|
fail_with: list[str] = ["boom one"]
|
||||||
|
|
||||||
|
async def ensure(s, **kw):
|
||||||
|
if fail_with:
|
||||||
|
raise RuntimeError(fail_with[0])
|
||||||
|
return SimpleNamespace(tools=[])
|
||||||
|
|
||||||
|
monkeypatch.setattr(mgr.mcp, "ensure", ensure)
|
||||||
|
monkeypatch.setattr(mgr.mcp, "last_stderr", lambda n: None)
|
||||||
|
|
||||||
|
await mgr.prepare_mcp_tools("s1", workspace=str(tmp_path))
|
||||||
|
assert [n for n, _ in mgr.pop_mcp_failures("s1")] == ["flaky"]
|
||||||
|
|
||||||
|
# Same error, next session: quiet (the Connectors page still shows it).
|
||||||
|
await mgr.prepare_mcp_tools("s2", workspace=str(tmp_path))
|
||||||
|
assert mgr.pop_mcp_failures("s2") == []
|
||||||
|
assert mgr._mcp_errors.get("flaky") # standing error intact
|
||||||
|
|
||||||
|
# Different error: notice again.
|
||||||
|
fail_with[0] = "boom two"
|
||||||
|
await mgr.prepare_mcp_tools("s3", workspace=str(tmp_path))
|
||||||
|
assert [n for n, _ in mgr.pop_mcp_failures("s3")] == ["flaky"]
|
||||||
|
|
||||||
|
# Recovery clears the dedupe; the next breakage notices afresh.
|
||||||
|
fail_with.clear()
|
||||||
|
await mgr.prepare_mcp_tools("s4", workspace=str(tmp_path))
|
||||||
|
assert mgr.pop_mcp_failures("s4") == []
|
||||||
|
fail_with.append("boom three")
|
||||||
|
await mgr.prepare_mcp_tools("s5", workspace=str(tmp_path))
|
||||||
|
assert [n for n, _ in mgr.pop_mcp_failures("s5")] == ["flaky"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_notice_dedupe_survives_restart(tmp_path, monkeypatch):
|
||||||
|
"""The dedupe is persisted: relaunching the app must not re-stamp the same
|
||||||
|
unchanged complaint into the first session (the owner-hit annoyance)."""
|
||||||
|
from types import SimpleNamespace
|
||||||
|
|
||||||
|
server = SimpleNamespace(
|
||||||
|
name="flaky", transport="stdio", url=None, auth=None, enabled=True, include_tools=None, exclude_tools=None, requires_approval=True
|
||||||
|
)
|
||||||
|
monkeypatch.setattr(
|
||||||
|
"coworker.server.manager.load_mcp_servers", lambda *a, **k: [server]
|
||||||
|
)
|
||||||
|
|
||||||
|
async def ensure(s, **kw):
|
||||||
|
raise RuntimeError("same boom")
|
||||||
|
|
||||||
|
from coworker.server import SessionManager
|
||||||
|
|
||||||
|
mgr = SessionManager(data_dir=tmp_path / "data")
|
||||||
|
monkeypatch.setattr(mgr.mcp, "ensure", ensure)
|
||||||
|
monkeypatch.setattr(mgr.mcp, "last_stderr", lambda n: None)
|
||||||
|
await mgr.prepare_mcp_tools("s1", workspace=str(tmp_path))
|
||||||
|
assert [n for n, _ in mgr.pop_mcp_failures("s1")] == ["flaky"]
|
||||||
|
|
||||||
|
reborn = SessionManager(data_dir=tmp_path / "data")
|
||||||
|
monkeypatch.setattr(reborn.mcp, "ensure", ensure)
|
||||||
|
monkeypatch.setattr(reborn.mcp, "last_stderr", lambda n: None)
|
||||||
|
await reborn.prepare_mcp_tools("s2", workspace=str(tmp_path))
|
||||||
|
assert reborn.pop_mcp_failures("s2") == []
|
||||||
|
|||||||
Reference in New Issue
Block a user