fix(schedular): a dead calendar workbench resets instead of failing forever

FIX_REPORT.md

@@ -0,0 +1,192 @@
+# Fix: PlannerAgent's MCP workbench never recovered from a transport blip
+
+## Incident
+
+The `google-calendar-mcp` container crash-looped from 09:13 (a zero-byte
+`tokens.json`). The Slack bot started at 11:24 while it was down. Every "Add
+to calendar" press then failed with `MCP Actor not running, call
+initialize() first`, including "Try again", and it stayed broken even after
+the server came back — only a bot restart fixed it.
+
+## Root cause
+
+`PlannerAgent._ensure_workbench` cached one `McpWorkbench` for the process
+lifetime with no health check and no reset. AutoGen's
+`McpWorkbench.call_tool` only calls `start()` when `self._actor` is falsy;
+once an actor exists but its session has died, every later call raises `MCP
+Actor not running` forever, and nothing in `PlannerAgent` ever discarded that
+dead actor. `McpCalendarClient` (timeboxing) had already solved exactly this
+with `_RECOVERABLE_ERROR_MARKERS` / `_is_recoverable_transport_error` /
+`_reset_workbench` / a retry-once loop — `PlannerAgent` simply never got the
+same treatment.
+
+## What changed
+
+1. **`src/fateforger/core/mcp_transport.py` (new).** Pulls the recoverable-
+   transport-error markers and `is_recoverable_transport_error(exc)` out of
+   `McpCalendarClient` into one shared, pure, offline-testable module so the
+   two clients cannot drift apart on what counts as recoverable. No I/O, no
+   network, no model.
+
+2. **`src/fateforger/agents/timeboxing/mcp_clients.py`.** `McpCalendarClient`
+   now imports and delegates to `is_recoverable_transport_error` instead of
+   keeping its own private `_RECOVERABLE_ERROR_MARKERS` tuple and matching
+   logic. `_is_recoverable_transport_error` is kept as a thin classmethod
+   wrapper so nothing else in the class had to change. Behaviour is
+   unchanged — same markers, same matching, same call sites.
+
+3. **`src/fateforger/agents/schedular/agent.py`.** `PlannerAgent` gained:
+   - `_reset_workbench()`: discards the cached (possibly dead) workbench so
+     the next `_ensure_workbench()` call builds a fresh one. Best-effort
+     calls `.stop()` on the old one if present, swallowing any error from an
+     already-broken actor.
+   - `_call_tool_with_retry(tool_name, arguments, *, retry: bool)`: calls a
+     tool on the current workbench; on a non-recoverable error it re-raises
+     immediately (no reset, no retry — the server was reachable and said
+     no); on a recoverable transport error it always resets the workbench,
+     then either retries once (`retry=True`) or re-raises without resending
+     (`retry=False`).
+
+   All six `await workbench.call_tool(...)` sites inside `PlannerAgent` (the
+   original lines 478, 532, 548, 600, 632, 669) now go through this method.
+   Line 898 (`call_create_event_with_retry`, a module-level function taking
+   a `workbench` argument for the separate `CalendarEventWorkerAgent`) was
+   left untouched — see "What I deliberately did not touch" below.
+
+## Write-safety decision
+
+Reads (`list-events`, `get-event`) are retried (`retry=True`): resending a
+read cannot create a duplicate side effect, so this is the case the incident
+was actually about — a "Try again" click doing a `get-event`/`list-events`
+should just work once the workbench resets.
+
+`delete-event` (cleaning up an already-cancelled event before recreating it)
+is also retried (`retry=True`): deleting is idempotent — resending it either
+succeeds again or 404s on an already-deleted event, and that 404 path is
+already handled by the surrounding `try/except` that logs a warning and
+falls through to the create path. It can never produce a duplicate.
+
+`create-event` / `update-event` (the main upsert call, and its
+already-exists fallback update) are **not** retried (`retry=False`). The
+reasoning:
+
+- `MCP Actor not running` is raised locally, in the client, *before any
+  request reaches the server* — so when that specific error fires, the
+  first attempt is known not to have happened server-side, and a retry
+  would be safe on that basis alone.
+- But `is_recoverable_transport_error` also matches other markers — a
+  response timeout (`"timed out while waiting for response to
+  ClientRequest"`), a dropped connection (`"server disconnected"`) — and
+  those *can* fire after the request already left the client. For those,
+  whether the create reached the server is genuinely unknown.
+- The two markers live behind one boolean and one call site can't tell them
+  apart without adding a second axis of exception classification that
+  doesn't exist yet. Given that ambiguity, and that a wrong call here writes
+  a duplicate event to Hugo's real calendar, I chose the conservative
+  reading: never auto-resend a mutating calendar call.
+- Resetting the workbench still happens on every recoverable error
+  regardless of `retry`, so the production bug — the workbench staying dead
+  forever, so even "Try again" fails — is still fixed for creates: the
+  *next* press (a fresh `handle_upsert_calendar_event` call) gets a working
+  workbench and calendar. This was the actual failure mode in the incident;
+  it does not require retrying the mutating call itself, only recovering the
+  connection for the next attempt.
+
+This decision is locked by
+`test_retry_false_resets_the_workbench_but_never_resends_the_call` and
+`test_upsert_calendar_event_does_not_blindly_retry_a_create` — both fail if
+a create is ever wired through with `retry=True`.
+
+## What I deliberately did not touch
+
+`call_create_event_with_retry` (module-level function, ~line 898) belongs to
+the separate `CalendarEventWorkerAgent`, not `PlannerAgent`. It takes its own
+`workbench` argument, retries based on `result.is_error` (tenacity,
+3 attempts) rather than on a caught exception, and isn't part of the actor-
+caching bug this incident was about (`CalendarEventWorkerAgent` builds its
+own `McpWorkbench` in `__init__` and never routes through
+`PlannerAgent._ensure_workbench`). The task allowed leaving it alone unless
+it could be folded in cleanly without changing its signature; folding it in
+would mean either changing its signature (it has no access to
+`is_recoverable_transport_error`/`_reset_workbench`, which are instance
+methods on a different class) or duplicating the reset logic on a bare
+function, neither of which is clean. Left as is.
+
+## Tests
+
+New files:
+- `tests/unit/test_mcp_transport.py` — the shared classifier, in isolation.
+- `tests/unit/test_planner_agent_workbench_retry.py` — `PlannerAgent`'s
+  retry helper and its wiring into the real call sites. Covers, each
+  confirmed to fail before the corresponding implementation existed:
+  - a recoverable error causes exactly one reset and one retry, and the
+    second attempt's result is returned;
+  - a non-recoverable error propagates immediately with no reset and no
+    retry;
+  - the retry budget is one: two consecutive recoverable errors raise
+    rather than looping (asserted via exact call/reset counts);
+  - `retry=False` still resets the workbench but never resends the call
+    (the write-safety lock);
+  - an end-to-end check that `handle_suggest_next_slot` actually recovers
+    through the real `list-events` call site;
+  - an end-to-end check that `handle_upsert_calendar_event` does not
+    blindly retry a `create-event` after a recoverable failure, and that
+    the dead workbench still gets discarded.
+
+All six were run and observed failing (`AttributeError:
+'PlannerAgent' object has no attribute '_call_tool_with_retry'`, or the real
+exception surfacing unhandled) before the implementation was added, per the
+task's requirement.
+
+## Test command and output
+
+```
+PYTHONPATH=src /Users/hugoevers/VScode-projects/admonish-1/.venv/bin/python -m pytest tests/ -k "planner or calendar or mcp or schedular" -q -m "not slow"
+```
+
+```
+============================= test session starts ==============================
+platform darwin -- Python 3.11.9, pytest-8.4.2, pluggy-1.6.0
+rootdir: /Users/hugoevers/VScode-projects/admonish-1/.claude/worktrees/planner-mcp-retry
+configfile: pyproject.toml
+plugins: langsmith-0.12.1, mock-3.15.1, cov-7.1.0, httpx-0.30.0, anyio-4.15.0, asyncio-0.21.2
+asyncio: mode=Mode.AUTO
+collected 3501 items / 2911 deselected / 590 selected
+
+... (584 passed, 6 skipped, unrelated pre-existing skips) ...
+
+=============== 584 passed, 6 skipped, 2911 deselected in 14.32s ===============
+```
+
+Also ran in isolation:
+
+```
+PYTHONPATH=src /Users/hugoevers/VScode-projects/admonish-1/.venv/bin/python -m pytest tests/unit/test_planner_agent_workbench_retry.py tests/unit/test_mcp_transport.py -q
+============================== 18 passed in 0.09s ==============================
+```
+
+## Concerns / follow-ups
+
+- `_reset_workbench`'s best-effort `.stop()` call swallows all exceptions
+  from the dying actor. That mirrors `McpCalendarClient`'s existing
+  `_reset_workbench`, which doesn't even attempt `.stop()` because
+  `McpWorkbench` has no `close` attribute (it only skips closing, it never
+  calls `.stop()` either). I chose to attempt `.stop()` best-effort in
+  `PlannerAgent` since `McpWorkbench` does expose it, but if that stop
+  itself resource-leaks under a truly dead actor, that would need separate
+  investigation — out of scope here.
+- The ambiguity between "actor never started, definitely pre-request" and
+  "timeout/disconnect, possibly mid-request" inside a single
+  `is_recoverable_transport_error` boolean is a real limitation: a more
+  precise fix would split the classifier into "definitely pre-request" vs
+  "ambiguous", and only auto-retry creates on the former. I did not do that
+  here because it goes beyond what either client currently distinguishes.
+  I checked: `McpCalendarClient._call_tool_payload` (the retry-once wrapper
+  this whole design is modelled on) is, in production, only ever invoked by
+  `list_day_snapshot` against `list-events` — a read. It has never actually
+  exercised its retry-once path against a write. So `PlannerAgent` refusing
+  to auto-retry `create-event`/`update-event` isn't a deviation from prior
+  art; it's the first time this pattern is applied to a write at all, and I
+  erred conservative rather than assume the existing retry logic was ever
+  validated against that case. Splitting the classifier remains a genuine
+  follow-up, not a decision I'm confident closes the topic.

src/fateforger/agents/schedular/agent.py

@@ -23,6 +23,7 @@
 from pydantic import TypeAdapter, ValidationError
 
 from fateforger.core.config import settings
+from fateforger.core.mcp_transport import is_recoverable_transport_error
 from fateforger.debug.diag import with_timeout
 from fateforger.haunt.mixins import HauntAwareAgentMixin
 from fateforger.haunt.models import FollowUpPlan, HauntTone
@@ -140,6 +141,69 @@ def _ensure_workbench(self) -> McpWorkbench:
         self._workbench = McpWorkbench(params)
         return self._workbench
 
+    async def _reset_workbench(self) -> None:
+        """Discard the cached workbench so the next call builds a fresh one.
+
+        AutoGen's `McpWorkbench.call_tool` only calls `start()` when its actor
+        is falsy; once an actor exists but its session has died, every later
+        call raises "MCP Actor not running" forever. Dropping the cached
+        instance here is what lets one bad call recover on the very next
+        press instead of requiring a process restart -- which is exactly what
+        stayed broken in production on 2026-09-09.
+        """
+        current = self._workbench
+        self._workbench = None
+        stop = getattr(current, "stop", None)
+        if callable(stop):
+            try:
+                maybe = stop()
+                if hasattr(maybe, "__await__"):
+                    await maybe
+            except Exception:
+                # Best-effort cleanup only -- a dead actor may already be
+                # unstoppable, and that must never block the reset.
+                pass
+
+    async def _call_tool_with_retry(
+        self,
+        tool_name: str,
+        arguments: dict,
+        *,
+        retry: bool,
+    ) -> object:
+        """Call an MCP tool, resetting the workbench once on a recoverable
+        transport failure.
+
+        `retry=True` additionally resends the call once after the reset --
+        safe only for calls whose failure mode cannot leave a duplicate
+        side effect behind (reads, and idempotent writes like delete). Any
+        non-recoverable error propagates immediately, untouched: it means
+        the server was reachable and said no, which a reset cannot fix.
+
+        `retry=False` still resets the workbench on a recoverable error (so
+        the *next* call -- a fresh "Try again" press -- gets a working
+        workbench), but re-raises without resending this call. Use this for
+        anything that mutates the calendar: "MCP Actor not running" is
+        raised locally, before any request reaches the server, so the first
+        attempt is known not to have happened server-side -- but a retry
+        would still be a *second* request, and other recoverable markers
+        (a response timeout, a dropped connection) cover cases where the
+        first request may already have reached the server. Resending a
+        create in that state risks writing a duplicate event to a real
+        calendar, which is worse than the bug this exists to fix.
+        """
+        workbench = self._ensure_workbench()
+        try:
+            return await workbench.call_tool(tool_name, arguments=arguments)
+        except Exception as exc:
+            if not is_recoverable_transport_error(exc):
+                raise
+            await self._reset_workbench()
+            if not retry:
+                raise
+            workbench = self._ensure_workbench()
+            return await workbench.call_tool(tool_name, arguments=arguments)
+
     @staticmethod
     def _extract_tool_payload(
         result: object,
@@ -415,7 +479,6 @@ def _event_blocks(
     async def handle_suggest_next_slot(
         self, message: SuggestNextSlot, ctx: MessageContext
     ) -> SuggestedSlot:
-        workbench = self._ensure_workbench()
         try:
             tz = ZoneInfo(message.time_zone)
         except Exception:
@@ -475,7 +538,7 @@ def _first_gap(
             if window_end <= window_start:
                 continue
 
-            result = await workbench.call_tool(
+            result = await self._call_tool_with_retry(
                 "list-events",
                 arguments={
                     "calendarId": message.calendar_id,
@@ -484,6 +547,7 @@ def _first_gap(
                     "singleEvents": True,
                     "orderBy": "startTime",
                 },
+                retry=True,  # read-only: safe to resend after a dead-actor reset
             )
             payload = self._extract_tool_payload(result)
             tool_error = self._extract_tool_error(payload)
@@ -522,19 +586,19 @@ async def handle_upsert_calendar_event(
             message.event_id,
             message.summary,
         )
-        workbench = self._ensure_workbench()
         tz = ZoneInfo(message.time_zone or "UTC")
         preexisting_cancelled = False
 
         # Prefer deterministic upsert (get → update|create) over LLM tool-routing.
         exists = False
         try:
-            fetched = await workbench.call_tool(
+            fetched = await self._call_tool_with_retry(
                 "get-event",
                 arguments={
                     "calendarId": message.calendar_id,
                     "eventId": message.event_id,
                 },
+                retry=True,  # read-only: safe to resend after a dead-actor reset
             )
             event = self._normalize_event(self._extract_tool_payload(fetched))
             event_status = self._event_status(event)
@@ -545,12 +609,16 @@ async def handle_upsert_calendar_event(
                     message.event_id,
                 )
                 try:
-                    await workbench.call_tool(
+                    await self._call_tool_with_retry(
                         "delete-event",
                         arguments={
                             "calendarId": message.calendar_id,
                             "eventId": message.event_id,
                         },
+                        # Deleting is idempotent -- retrying a delete cannot
+                        # produce a duplicate, only a harmless not-found on
+                        # an already-deleted event.
+                        retry=True,
                     )
                 except Exception as exc:
                     logger.warning(
@@ -597,9 +665,19 @@ async def handle_upsert_calendar_event(
             upsert_action = "update-event" if exists else "create-event"
             if upsert_action == "create-event" and preexisting_cancelled:
                 upsert_arguments["allowDuplicates"] = True
-            upsert_result = await workbench.call_tool(
+            upsert_result = await self._call_tool_with_retry(
                 upsert_action,
                 arguments=upsert_arguments,
+                # create-event/update-event write to a real calendar. "MCP
+                # Actor not running" is raised locally before any request
+                # reaches the server, so it is safe to know the first
+                # attempt never happened -- but the other recoverable
+                # markers (a response timeout, a dropped connection) can
+                # fire *after* the request left the client, when a retry
+                # would be a genuine second write. Never resend a mutating
+                # call automatically; the reset below still happens so the
+                # next user press gets a working workbench.
+                retry=False,
             )
         except Exception as e:
             logger.error("Failed to upsert calendar event: %s", e, exc_info=True)
@@ -629,9 +707,12 @@ async def handle_upsert_calendar_event(
                 target_event_id,
             )
             try:
-                fallback_result = await workbench.call_tool(
+                fallback_result = await self._call_tool_with_retry(
                     "update-event",
                     arguments=upsert_arguments,
+                    # Same write-safety reasoning as the primary upsert call
+                    # above: never auto-resend a mutating call.
+                    retry=False,
                 )
             except Exception as exc:
                 return UpsertCalendarEventResult(
@@ -666,12 +747,13 @@ async def handle_upsert_calendar_event(
             )
 
         try:
-            fetched = await workbench.call_tool(
+            fetched = await self._call_tool_with_retry(
                 "get-event",
                 arguments={
                     "calendarId": message.calendar_id,
                     "eventId": target_event_id,
                 },
+                retry=True,  # read-only: safe to resend after a dead-actor reset
             )
             event = self._normalize_event(self._extract_tool_payload(fetched)) or {}
         except Exception as exc: