feat(#663): activate MCP-recover for low-cadence agents (R1 enable-gate + R2 checkable) - #665
Merged
Merged
Conversation
…te + R2 checkable) Two refinements that let `mcp_recover` be enabled on an agent whose progress/transition watchdog is OFF (barsik's shape) — the activation blockers found in the #664 post-merge review. R1 — watchdog enable-gate (session_watchdog._evaluate): `mcp_recover` is its own opt-in, independent of the master `cfg.enabled` switch. The MCP-bind branch now runs before the `enabled` gate (return early only when BOTH `enabled` and `mcp_recover` are off); the progress/transition watchdog stays gated on `enabled`. During a BOOTING/RECONNECTING transition the branch is a cheap no-op (connected is False), so the transition branch still handles that wedge. R2 — checkable signal (api._watchdog_mcp_bind_status): `checkable` was `heartbeat_interval > 0`, which wrongly excludes wake-driven sidekicks that carry interval==0 yet heartbeat in practice (barsik; Murzik Finding #2). Now also true when the agent emitted a recent AGENT-ORIGIN heartbeat (get_latest_agent_heartbeat — excludes synthetic server_presence rows, so a wedged agent can't look checkable-and-bound). Extracted as the pure, unit-tested helper `compute_mcp_checkable`. Still flag-gated (mcp_recover default OFF). Tests: R1 enable-gate routing + progress-gated-off, R2 checkable policy matrix. 52 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Proves the R1 _evaluate reorder doesn't steal lifecycle transitions: with enabled=True + mcp_recover=True and a RECONNECTING (connected=False) snap, the MCP-bind branch no-ops and the transition branch still owns the wedge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Follow-up to #664 (merged,
83d054f). #664 shipped MCP-orphan auto-recovery flag-gated default-OFF so it could soak on one agent (barsik) before fleet rollout. The post-merge review surfaced two blockers that make flippingmcp_recoveron barsik a no-op. This PR fixes both so the flag is actually actionable for a low-cadence agent.barsik's live shape (the soak target):
watchdog_config={"enabled": false},heartbeat_interval=0,wake_interval=3600.R1 — watchdog enable-gate (
session_watchdog._evaluate)mcp_recoveris its own opt-in, independent of the mastercfg.enabledswitch. Previouslyif not cfg.enabled: returnat the top of_evaluateskipped the MCP-bind branch entirely for any agent with the progress watchdog off — so barsik (enabled=false) could never MCP-recover.Now: return early only when both
enabledandmcp_recoverare off. The MCP-bind branch runs before theenabledgate; the progress/transition watchdog stays gated onenabled. (Reordering note: during a BOOTING/RECONNECTING transitionsnap.connectedis False, so the MCP-bind branch is a cheap no-op there and the transition branch below still handles that wedge class.)R2 — checkable signal (
api._watchdog_mcp_bind_status)checkablewasheartbeat_interval > 0. barsik carriesheartbeat_interval == 0but heartbeats in practice (wake-driven), so it was wrongly excluded — Murzik Finding #2. Nowcheckableis also true when the agent emitted a recent agent-origin heartbeat (get_latest_agent_heartbeat, which excludes the scheduler's syntheticserver_presencerows — so a wedged agent can't look checkable-and-bound). Extracted as the pure, unit-tested helpercompute_mcp_checkable(window:DEFAULT_MCP_CHECKABLE_HEARTBEAT_MAX_AGE = 1800s).Safety
mcp_recoverdefault OFF. No agent's behavior changes until the flag is set.hb > 0still → checkable).Tests
tests/test_session_watchdog.py— 52 pass, ruff clean.enabled=False, mcp_recover=True; fully-off agent is a true no-op (no status lookup, no state entry); progress/transition watchdog stays gated off when disabled.compute_mcp_checkablepolicy matrix (no agent / no epoch / cadence agent / recent beat / stale beat / no history / window boundary).The honest tension I want a second pair of eyes on before we flip the flag on barsik:
A healthy-but-idle barsik and a wedged barsik both present as
bound=Falseright after a gateway rebuild. The only difference is the healthy one eventually rebinds (on its next MCP call) while the wedged one never does. With R2, a recently-active barsik ischeckablefor 30 min, so during the 240s sustained-unbound window after a rebuild there's a false-positive risk: if a healthy barsik doesn't make any MCP round-trip for 240s post-resume, the watchdog would force-fresh restart it.Mitigations I'm relying on:
boundgoes sticky-true on the first MCP call of the epoch and stays true → the risk window is only the brief post-rebuild rebind gap, not steady-state.Questions: (a) Is passive heartbeat-recency
checkablesoak-safe enough for barsik, or should barsik enablement wait for the phase-2 activemcp_probeelicitation (which removes the ambiguity by forcing an MCP round-trip at launch)? (b) Is1800sa sane window, or would you tie it to a multiple of the unbound deadline instead? (c) Any objection to the_evaluatereorder (MCP-bind now before the transition branch)?Closes the activation blockers for #663. Does not include phase-2 (active probe / launch-time directive) — tracked separately.
🤖 Generated with Claude Code