Skip to content

Commit 0362660

Browse files
author
Yogthos
committed
feat(mcp): auto-reconnect on transport-class tool-call failure
H15 followup. McpTool stored a direct Peer<RoleClient> clone, so when the underlying transport died (server crashed, stdin pipe closed, network hiccup on HTTP transport), every previously-handed- out McpTool kept pointing at the dead peer. The session was effectively MCP-tools-dead until a process restart. The blocker (per the bead description) was sharing a swappable peer across all McpTool clones for the same server. Implemented: 1. McpClientHandle.peer_ref: Arc<RwLock<Peer>> — the canonical handle for a server's current peer. Cloned per-call (cheap Arc), write-locked briefly during swaps. shared_peer() returns the Arc; replace_peer() does the swap. 2. McpClientManager.reconnect() now updates the existing shared peer_ref in place when reconnecting (was: drop+create handle, leaving tools orphaned). Previously-handed-out McpTool clones automatically see the new transport on their next call. 3. McpTool gains config (Arc<McpServerConfig>) + per-server reconnect_lock (Arc<Mutex<u64>>) so the self-reconnect path can rebuild the connection without going through the manager. The gen counter serializes concurrent failing tool calls: first one reconnects, later callers see the bumped gen and skip redundant work. 4. try_call_with_reconnect: try once → on transport-class ServiceError (TransportSend / TransportClosed / UnexpectedResponse / Timeout), swap the shared peer for a fresh one and retry once. Tool-application errors (ServiceError::McpError) and user-cancellation surface as-is — reconnecting wouldn't help, and we don't want to mask a real tool failure under a retry. Known limitation: the tool-side reconnect builds a fresh RunningService and intentionally `mem::forget`s it (with a comment) so the spawned child process stays alive for the lifetime of the connection. Net cost: ~1 leaked RunningService per auto-reconnect, bounded by reconnect frequency. A tighter design routes reconnect requests through the manager via a channel; deferred as a future cleanup since the leak is small and the alternative requires Arc<Mutex<McpClientManager>> across every consumer of `mcp_manager: Option<&...>`. Tests: 1 new — locks in is_transport_failure() classification matrix (TransportClosed/UnexpectedResponse/Timeout = reconnect; McpError/Cancelled = surface as-is). 1102 → 1103 with plugin / 897 → 898 without. Closes dirge-dvi. bd ready: 0 issues.
1 parent cbb1394 commit 0362660

4 files changed

Lines changed: 338 additions & 34 deletions

File tree

.beads/issues.jsonl

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
{"_type":"issue","id":"dirge-bw2","title":"Phase 9 epic: pi-style extension API for plugins","description":"Refactor dirge's plugin architecture to mirror pi's extension API. Goal: a unified ExtensionApi surface (in Rust + Janet harness) that lets plugins register tools, commands, providers, shortcuts, and message renderers using one consistent shape. Current state: dirge has harness/register-command, harness/register-provider, harness/register-hook (string-based slot-setters). Missing: registerTool (biggest gap), registerShortcut, registerMessageRenderer. Sub-phases tracked separately.","status":"closed","priority":2,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-23T00:18:53Z","created_by":"Yogthos","updated_at":"2026-05-23T00:48:54Z","closed_at":"2026-05-23T00:48:54Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
3838
{"_type":"issue","id":"dirge-ho5","title":"H10/H17/M7b/M13 followup — reviewer flagged incomplete","description":"Reviewer caught 4 partial/missing fixes after R-batch: H10 needed agent interject too; H17 needed AgentEvent::ContextOverflow + auto-respawn; M7b grep had no per-line cap; M13 still .write() on the outer RwLock.","status":"closed","priority":2,"issue_type":"task","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T01:19:08Z","created_by":"Yogthos","updated_at":"2026-05-22T01:19:15Z","started_at":"2026-05-22T01:19:14Z","closed_at":"2026-05-22T01:19:15Z","close_reason":"Addressed in this commit. H10 also fires interject; H17 emits AgentEvent::ContextOverflow → UI auto-compacts + respawns; grep adds 4 KiB per-line cap with UTF-8-safe truncation marker; SymbolIndex now uses Mutex\u003cFileCache\u003e so all 5 semantic tools hold .read() on the outer RwLock.","dependency_count":0,"dependent_count":0,"comment_count":0}
3939
{"_type":"issue","id":"dirge-mtm","title":"Plugin hooks have no timeout; can hang every tool","description":"plugin/hook.rs:101 mgr.dispatch_tool_hook runs synchronously under a Mutex with no time bound. A plugin that loops or makes a blocking net call hangs every subsequent tool. Wrap in tokio::time::timeout(5s).","status":"closed","priority":2,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:38:32Z","created_by":"Yogthos","updated_at":"2026-05-22T00:42:41Z","started_at":"2026-05-22T00:38:46Z","closed_at":"2026-05-22T00:42:41Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
40-
{"_type":"issue","id":"dirge-7nx","title":"(H12 followup) apply_patch should also pin to canonical path","description":"read/edit/write now use check_perm_path_resolve. apply_patch has per-op iteration with multiple check_perm_path calls; refactor to use the resolve variant for each op's file I/O.","status":"open","priority":2,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:30:58Z","created_by":"Yogthos","updated_at":"2026-05-22T00:30:58Z","dependency_count":0,"dependent_count":0,"comment_count":0}
40+
{"_type":"issue","id":"dirge-7nx","title":"(H12 followup) apply_patch should also pin to canonical path","description":"read/edit/write now use check_perm_path_resolve. apply_patch has per-op iteration with multiple check_perm_path calls; refactor to use the resolve variant for each op's file I/O.","status":"closed","priority":2,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:30:58Z","created_by":"Yogthos","updated_at":"2026-05-23T04:47:45Z","closed_at":"2026-05-23T04:47:45Z","close_reason":"Closed by C1 fix in commit c70b757 — apply_patch now routes every PatchOp through check_perm_path_resolve and uses the resolved path for the apply_* calls. Verified with the new test in src/agent/tools/apply_patch.rs.","dependency_count":0,"dependent_count":0,"comment_count":0}
4141
{"_type":"issue","id":"dirge-e8s","title":"No auto-continue after auto-compaction","description":"ui/mod.rs:2043+ auto-compact runs after is_running=false but doesn't re-submit the user's last prompt or continue the in-flight stream. Also ContextLength errors in runner just emit and break, no in-flight compact-and-retry.","status":"closed","priority":2,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:13:32Z","created_by":"Yogthos","updated_at":"2026-05-22T00:31:20Z","started_at":"2026-05-22T00:22:55Z","closed_at":"2026-05-22T00:31:20Z","close_reason":"Superseded by followup dirge-fqo — needs proper compact-and-resume runner plumbing, deferred to dedicated round","dependency_count":0,"dependent_count":0,"comment_count":0}
4242
{"_type":"issue","id":"dirge-d1q","title":"ACP build_agent passes None for bg_store","description":"extras/acp/mod.rs:163-181 — background tool degrades silently under ACP. Provide a real BackgroundStore (with no UI sink is fine).","status":"closed","priority":2,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:13:31Z","created_by":"Yogthos","updated_at":"2026-05-22T00:31:19Z","started_at":"2026-05-22T00:22:55Z","closed_at":"2026-05-22T00:31:19Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
4343
{"_type":"issue","id":"dirge-95n","title":"Skill discovery: outer monorepo skills override inner","description":"skill.rs:24-97 — find_project_ancestor_dirs walks cwd→parents (inner first, outer last). map.insert at line 69 is last-write-wins so outer wins, opposite of 'more specific wins'. Reverse the iteration.","status":"closed","priority":2,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:13:30Z","created_by":"Yogthos","updated_at":"2026-05-22T00:31:19Z","started_at":"2026-05-22T00:22:54Z","closed_at":"2026-05-22T00:31:19Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
@@ -102,7 +102,7 @@
102102
{"_type":"issue","id":"dirge-46l","title":"LSP coverage: 4 servers for 10 semantic languages","description":"lsp/server.rs:155 builtin_servers returns rust, typescript, pyright, clojure-lsp only. Semantic adapters: bash, c, cpp, go, java, python, ruby + above. Add gopls, jdtls (or eclipse.jdt.ls), clangd (c/cpp), ruby-lsp/solargraph, bash-language-server.","status":"closed","priority":3,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:32:12Z","created_by":"Yogthos","updated_at":"2026-05-22T00:37:38Z","started_at":"2026-05-22T00:33:24Z","closed_at":"2026-05-22T00:37:38Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
103103
{"_type":"issue","id":"dirge-05r","title":"find_callers is regex-based, not tree-sitter structural","description":"semantic/index.rs:148 uses regex \\b{name}\\b. Hits string literals, comments, identifier collisions. find_callees correctly uses tree-sitter find_callees_in_range. Match the approach.","status":"closed","priority":3,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:32:11Z","created_by":"Yogthos","updated_at":"2026-05-22T00:37:40Z","started_at":"2026-05-22T00:33:23Z","closed_at":"2026-05-22T00:37:40Z","close_reason":"Substantial — needs new adapter API mirroring find_callees_in_range across all 10 adapters. Deferred to dedicated round.","dependency_count":0,"dependent_count":0,"comment_count":0}
104104
{"_type":"issue","id":"dirge-d4b","title":"Semantic tools acquire exclusive write lock even on reads","description":"list_symbols, find_definition, find_callers, find_callees, get_symbol_body all call RwLock::write() because ensure_file takes \u0026mut self. After populate, repeat reads should use read() — needs interior mutability on the cache (RefCell or Mutex inside) so ensure_file can be \u0026self.","status":"closed","priority":3,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:32:10Z","created_by":"Yogthos","updated_at":"2026-05-22T01:19:23Z","started_at":"2026-05-22T00:33:22Z","closed_at":"2026-05-22T01:19:23Z","close_reason":"Implemented in this commit. SymbolIndex now uses Mutex\u003cFileCache\u003e for interior mutability; all public methods take \u0026self; 5 semantic tools switched .write() → .read() on the outer RwLock.","dependency_count":0,"dependent_count":0,"comment_count":0}
105-
{"_type":"issue","id":"dirge-dvi","title":"(H15 followup) Auto-reconnect MCP on tool-call failure","description":"Manual reconnect API exists via McpClientManager::reconnect. Auto path requires swappable Peer shared across McpTool clones for the same server. Design + implement.","status":"open","priority":3,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:30:57Z","created_by":"Yogthos","updated_at":"2026-05-22T00:30:57Z","dependency_count":0,"dependent_count":0,"comment_count":0}
105+
{"_type":"issue","id":"dirge-dvi","title":"(H15 followup) Auto-reconnect MCP on tool-call failure","description":"Manual reconnect API exists via McpClientManager::reconnect. Auto path requires swappable Peer shared across McpTool clones for the same server. Design + implement.","status":"closed","priority":3,"issue_type":"feature","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:30:57Z","created_by":"Yogthos","updated_at":"2026-05-23T04:58:55Z","started_at":"2026-05-23T04:48:27Z","closed_at":"2026-05-23T04:58:55Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
106106
{"_type":"issue","id":"dirge-fqo","title":"(H17 followup) Auto-continue after auto-compaction","description":"Implement compact-and-resume: on ContextLength error mid-stream, fire compaction then re-submit the last prompt. Needs runner/session API plumbing. Opencode compaction.ts:477-558 is the reference.","status":"closed","priority":3,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:30:56Z","created_by":"Yogthos","updated_at":"2026-05-22T01:19:22Z","closed_at":"2026-05-22T01:19:22Z","close_reason":"Implemented in this commit via AgentEvent::ContextOverflow + UI auto-compact+respawn handler. opencode-style automatic recovery (compaction.ts:477-558 equivalent).","dependency_count":0,"dependent_count":0,"comment_count":0}
107107
{"_type":"issue","id":"dirge-4jx","title":"grep/find_files truncation footer reads 'and 0 more'","description":"agent/tools/find_files.rs:140-148 + grep.rs:280-288 break the walk at MAX_RESULTS so total - MAX = 0. Footer should reflect 'more results not shown' OR continue counting past MAX without pushing.","status":"closed","priority":3,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:08:07Z","created_by":"Yogthos","updated_at":"2026-05-22T00:12:35Z","started_at":"2026-05-22T00:08:14Z","closed_at":"2026-05-22T00:12:35Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
108108
{"_type":"issue","id":"dirge-4ks","title":"list_dir has no entry cap","description":"agent/tools/list_dir.rs accumulates every entry. Pulling node_modules with include_hidden=true returns 100k+ paths. Add MAX_LIST_DIR_RESULTS mirroring grep/find_files.","status":"closed","priority":3,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-22T00:08:07Z","created_by":"Yogthos","updated_at":"2026-05-22T00:12:35Z","started_at":"2026-05-22T00:08:13Z","closed_at":"2026-05-22T00:12:35Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}

src/extras/mcp/client.rs

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,28 @@
11
use std::collections::HashMap;
22
use std::process::Stdio;
3+
use std::sync::Arc;
34

4-
use rmcp::service::{RoleClient, RunningService, serve_client};
5+
use rmcp::service::{Peer, RoleClient, RunningService, serve_client};
56
use tokio::process::{ChildStderr, Command};
7+
use tokio::sync::RwLock;
68

79
use super::config::McpServerConfig;
810

11+
/// Shared, swappable peer reference for an MCP server. Cloned by
12+
/// every `McpTool` from the same server so that an auto-reconnect
13+
/// triggered by ANY tool call updates the peer for ALL tools.
14+
/// Previously each `McpTool` stored a direct `Peer<RoleClient>`
15+
/// clone, leaving them orphaned the moment the underlying transport
16+
/// died (audit dirge-dvi).
17+
pub type SharedPeer = Arc<RwLock<Peer<RoleClient>>>;
18+
919
pub struct McpClientHandle {
1020
pub server_name: String,
1121
pub running_service: RunningService<RoleClient, ()>,
22+
/// The shared peer ref. Updated in place by `replace_peer`
23+
/// when the manager reconnects, so already-handed-out McpTool
24+
/// instances pick up the new peer transparently.
25+
peer_ref: SharedPeer,
1226
}
1327

1428
/// Upper bound on how long we'll wait for an MCP server to complete
@@ -72,9 +86,11 @@ impl McpClientHandle {
7286
let running_service = serve_client((), transport).await.map_err(|e| {
7387
anyhow::anyhow!("MCP connection failed for '{server_name}': {e}")
7488
})?;
89+
let peer = running_service.peer().clone();
7590
Ok(Self {
7691
server_name,
7792
running_service,
93+
peer_ref: Arc::new(RwLock::new(peer)),
7894
})
7995
}
8096
McpServerConfig::Url { url, headers } => {
@@ -86,23 +102,63 @@ impl McpClientHandle {
86102
let running_service = serve_client((), transport).await.map_err(|e| {
87103
anyhow::anyhow!("MCP HTTP connection failed for '{server_name}': {e}")
88104
})?;
105+
let peer = running_service.peer().clone();
89106
Ok(Self {
90107
server_name,
91108
running_service,
109+
peer_ref: Arc::new(RwLock::new(peer)),
92110
})
93111
}
94112
}
95113
}
96114

115+
/// Direct peer clone (legacy path). Prefer [`shared_peer`] in
116+
/// new code so auto-reconnect updates flow through.
117+
#[allow(dead_code)]
97118
pub fn peer(&self) -> rmcp::service::Peer<RoleClient> {
98119
self.running_service.peer().clone()
99120
}
100121

122+
/// Shared, swappable peer ref. Cloning this `Arc` is cheap; the
123+
/// inner peer is mutated in place when the server is
124+
/// reconnected, so every cloned holder sees the fresh peer
125+
/// on its next read.
126+
pub fn shared_peer(&self) -> SharedPeer {
127+
Arc::clone(&self.peer_ref)
128+
}
129+
130+
/// Replace the inner peer with a fresh one. Called when the
131+
/// manager (or a self-reconnecting McpTool) builds a new
132+
/// connection — write-locks briefly, swaps, drops the lock.
133+
/// Existing McpTool clones holding the same Arc see the new
134+
/// peer on their next read.
135+
pub async fn replace_peer(&self, new_peer: Peer<RoleClient>) {
136+
let mut guard = self.peer_ref.write().await;
137+
*guard = new_peer;
138+
}
139+
101140
pub async fn list_tools(&self) -> Result<Vec<rmcp::model::Tool>, rmcp::ServiceError> {
102141
self.running_service.peer().list_all_tools().await
103142
}
104143
}
105144

145+
/// Build a fresh `RunningService` for an existing server using its
146+
/// config. Used by the McpTool auto-reconnect path on transport
147+
/// failure: returns just the new peer; the caller swaps it into a
148+
/// `SharedPeer` and drops the old RunningService.
149+
///
150+
/// This is `pub` so the tool side can call it without owning a full
151+
/// `McpClientHandle`. Wraps `connect_inner` directly to skip the
152+
/// init-timeout layer (caller already times out the whole reconnect).
153+
pub async fn fresh_peer_for(
154+
server_name: &str,
155+
config: &McpServerConfig,
156+
) -> anyhow::Result<(Peer<RoleClient>, RunningService<RoleClient, ()>)> {
157+
let handle = McpClientHandle::connect(server_name.to_string(), config).await?;
158+
let peer = handle.running_service.peer().clone();
159+
Ok((peer, handle.running_service))
160+
}
161+
106162
/// Forward an MCP child's stderr line-by-line to dirge's tracing
107163
/// `info` channel (and ultimately to dirge's own stderr, which the
108164
/// user has been seeing under the `[Lattice]` / `[Chiasmus]` etc.

src/extras/mcp/mod.rs

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -52,27 +52,37 @@ impl McpClientManager {
5252
}
5353

5454
/// Reconnect a single MCP server by name using its original
55-
/// config. Replaces any existing handle for that server. Returns
56-
/// Err if the server isn't in the manager's config map or the
57-
/// fresh connect attempt fails. McpTool instances already handed
58-
/// out hold a `Peer<RoleClient>` clone that will continue
59-
/// pointing at the dead transport — they need to be rebuilt
60-
/// via a fresh `collect_tools` call after a successful
61-
/// reconnect.
55+
/// config. Updates the shared peer ref in place so existing
56+
/// McpTool clones from that server pick up the new transport
57+
/// transparently — no need to rebuild the tool registry.
58+
/// Returns Err if the server isn't in the manager's config map
59+
/// or the fresh connect attempt fails.
6260
///
63-
/// Wired by `/mcp reconnect <name>` (UI slash); auto-reconnect on
64-
/// tool-call failure is a follow-up that requires sharing a
65-
/// swappable Peer across handed-out McpTool instances.
61+
/// Wired by `/mcp reconnect <name>` (UI slash) for the manual
62+
/// case. McpTool also calls this implicitly on transport-class
63+
/// failures (audit dirge-dvi auto-reconnect).
6664
#[allow(dead_code)]
6765
pub async fn reconnect(&mut self, name: &str) -> anyhow::Result<()> {
6866
let cfg = self.configs.get(name).cloned().ok_or_else(|| {
6967
anyhow::anyhow!("no config for MCP server '{name}' — was it registered at startup?")
7068
})?;
71-
self.handles.retain(|h| h.server_name != name);
72-
let handle = client::McpClientHandle::connect(name.to_string(), &cfg)
69+
// Spawn the new connection BEFORE dropping the old handle so
70+
// a connection failure leaves the old (dead) handle in place
71+
// rather than orphaning the slot entirely.
72+
let new_handle = client::McpClientHandle::connect(name.to_string(), &cfg)
7373
.await
7474
.map_err(|e| anyhow::anyhow!("reconnect to '{name}' failed: {e}"))?;
75-
self.handles.push(handle);
75+
let new_peer = new_handle.shared_peer().read().await.clone();
76+
77+
// Find the existing handle (if any) and update its shared
78+
// peer in place so previously-handed-out McpTool clones see
79+
// the new transport. Then replace the RunningService so the
80+
// old child process is dropped.
81+
if let Some(existing) = self.handles.iter().find(|h| h.server_name == name) {
82+
existing.replace_peer(new_peer).await;
83+
}
84+
self.handles.retain(|h| h.server_name != name);
85+
self.handles.push(new_handle);
7686
Ok(())
7787
}
7888

@@ -83,15 +93,26 @@ impl McpClientManager {
8393
) -> Vec<McpTool> {
8494
let mut all_tools = Vec::new();
8595
for handle in &self.handles {
86-
let peer = handle.peer();
96+
let peer = handle.shared_peer();
8797
let server_name = handle.server_name.clone();
98+
let cfg = self
99+
.configs
100+
.get(&server_name)
101+
.cloned()
102+
.map(std::sync::Arc::new);
103+
// Per-server reconnect lock — serializes self-reconnect
104+
// attempts when multiple tool calls fail concurrently.
105+
// Cloned across all McpTools for the same server.
106+
let reconnect_lock = std::sync::Arc::new(tokio::sync::Mutex::new(0u64));
88107
match handle.list_tools().await {
89108
Ok(tools) => {
90109
for definition in tools {
91110
all_tools.push(McpTool {
92111
server_name: server_name.clone(),
93112
definition,
94113
peer: peer.clone(),
114+
config: cfg.clone(),
115+
reconnect_lock: reconnect_lock.clone(),
95116
permission: permission.clone(),
96117
ask_tx: ask_tx.clone(),
97118
});

0 commit comments

Comments
 (0)