Skip to content

Commit b47f4c8

Browse files
sanchitrkclaude
sanchitrk
and
claude
committed
fix(harness): retry failed shared MCP instances and keep resource links reachable
- Restart an instance whose connect failed at the next exchange, so shared space/app instances recover for every session using them. - Carry earlier fixes onto the scoped service: tool-list refresh without the server lock, plain resource failures, and resource tools waiting for connecting servers. - Point resource links to read_mcp_resource only when that tool is declared. - Drop the record of a session with no MCP servers once its drain ends. - Hold a lease across the scope test's runs so a slow runner cannot change its pids. - Docs: app servers get no roots. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent a5ca5b6 commit b47f4c8

7 files changed

Lines changed: 112 additions & 42 deletions

File tree

‎packages/harness/src/mcp/mcp.ts‎

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -261,21 +261,22 @@ export const layer = (options: {
261261
);
262262
// Calls replace an expired session themselves; this covers a background request seeing it first.
263263
live.connection.onSessionExpired(current(retire(server, live)));
264+
// Listed without the server's lock, which calls and closing need: the listing can take a while.
264265
live.connection.onToolsChanged(
265-
current(
266-
live.connection.tools().pipe(
267-
Effect.tap((tools) =>
268-
Effect.sync(() => {
269-
server.tools = tools;
270-
}),
271-
),
272-
Effect.catch((error) =>
273-
Effect.sync(() => {
274-
server.error = `failed to refresh tools: ${error.message}`;
266+
Effect.sync(() => {
267+
fork(
268+
live.connection.tools().pipe(
269+
Effect.match({
270+
onSuccess: (tools) => {
271+
if (server.live === live) server.tools = tools;
272+
},
273+
onFailure: (error) => {
274+
if (server.live === live) server.error = `failed to refresh tools: ${error.message}`;
275+
},
275276
}),
276277
),
277-
),
278-
),
278+
);
279+
}),
279280
);
280281
live.connection.onLog((entry) => log.write(server.name, entry));
281282
};
@@ -453,7 +454,8 @@ export const layer = (options: {
453454
new ToolCallError({
454455
server: name,
455456
tool: "resources",
456-
message: `MCP server "${name}" failed: ${message}`,
457+
// Listings name the server beside each failure already.
458+
message,
457459
}),
458460
);
459461
return {
@@ -585,6 +587,11 @@ export const layer = (options: {
585587
};
586588
instances.set(key, server);
587589
start(server);
590+
} else if (server.state === "failed" && server.attempt === undefined) {
591+
// A server that could not connect declares no tools, so no call would retry it, and
592+
// shared instances would keep every later session on it. Retry in the background;
593+
// waiting would hold up every prompt while it is down.
594+
start(server);
588595
}
589596
const entry: Entry = { config, server };
590597
session.servers.set(name, entry);
@@ -635,6 +642,10 @@ export const layer = (options: {
635642
);
636643
}
637644
const output = { save: McpOutput.save };
645+
// Resource tools follow their servers' exposure: declared when one of them is `direct`.
646+
const resourceTools = [...session.servers.values()].some(
647+
(entry) => withResources(entry) && exposureOf(entry.config) === "direct",
648+
);
638649
for (const [name, entry] of session.servers) {
639650
const server = entry.server;
640651
if (server === undefined || server.tools.length === 0) continue;
@@ -650,23 +661,26 @@ export const layer = (options: {
650661
tool,
651662
name: names.get(tool.name)!,
652663
call: call(session, name, tool.name),
653-
content: { ...output, readable: withResources(entry) },
664+
// A resource link points to `read_mcp_resource` only when the model has it.
665+
content: { ...output, readable: resourceTools && withResources(entry) },
654666
}),
655667
);
656668
}
657669
}
658-
// Resource tools follow their servers' exposure: declared when one of them is `direct`.
659-
if (
660-
[...session.servers.values()].some(
661-
(entry) => withResources(entry) && exposureOf(entry.config) === "direct",
662-
)
663-
)
670+
if (resourceTools)
664671
tools.push(
665672
...McpResources.make({
666673
servers: () =>
667674
[...session.servers]
668675
.filter(([, entry]) => withResources(entry))
669676
.map(([name]) => resourceServer(session, name)),
677+
// A server still connecting may offer resources once it has.
678+
ready: () =>
679+
Effect.forEach(
680+
[...session.servers.values()],
681+
({ server }) => (server?.ready === undefined ? Effect.void : Deferred.await(server.ready)),
682+
{ discard: true },
683+
),
670684
content: output,
671685
}),
672686
);
@@ -685,6 +699,9 @@ export const layer = (options: {
685699
Effect.sync(() => {
686700
session.leases -= 1;
687701
for (const server of used(session)) hold(server, -1);
702+
// Every session drains, MCP or not; one with no servers has nothing worth keeping.
703+
if (session.leases === 0 && session.servers.size === 0 && sessions.get(session.id) === session)
704+
sessions.delete(session.id);
688705
}),
689706
).pipe(Effect.asVoid);
690707

‎packages/harness/src/mcp/value.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,8 @@ const command = Effect.fnUntraced(function* (line: string, what: string) {
9292
const cached = commands.get(line);
9393
if (cached !== undefined) return cached;
9494
const output = yield* run(line);
95-
if (output === undefined) return yield* new ValueError({ message: `Failed to resolve ${what} from shell command: ${line}` });
95+
if (output === undefined)
96+
return yield* new ValueError({ message: `Failed to resolve ${what} from shell command: ${line}` });
9697
commands.set(line, output);
9798
return output;
9899
});
@@ -117,7 +118,8 @@ const variables = Effect.fnUntraced(function* (value: string, what: string) {
117118
/** One value. `what` names it in the failure, for example `MCP server "docs" header "Authorization"`. */
118119
export const resolve = Effect.fnUntraced(function* (value: string, what: string) {
119120
const resolved = yield* value.startsWith("!") ? command(value.slice(1), what) : variables(value, what);
120-
if (UNSAFE.test(resolved)) return yield* new ValueError({ message: `${what} contains a line break or NUL character` });
121+
if (UNSAFE.test(resolved))
122+
return yield* new ValueError({ message: `${what} contains a line break or NUL character` });
121123
return resolved;
122124
});
123125

‎packages/harness/test/__artifacts__/mcp.session.json‎

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
"requests": 8,
33
"tools": [
44
"mcp__e2e__echo",
5+
"mcp__e2e__args",
56
"mcp__e2e__fail",
67
"mcp__e2e__pid",
78
"mcp__e2e__crash",
@@ -45,6 +46,10 @@
4546
"my-srv",
4647
false
4748
],
49+
[
50+
"badheader",
51+
true
52+
],
4853
[
4954
"later",
5055
true
@@ -63,6 +68,10 @@
6368
"my-srv",
6469
false
6570
],
71+
[
72+
"badheader",
73+
true
74+
],
6675
[
6776
"later",
6877
true
@@ -81,6 +90,10 @@
8190
"my-srv",
8291
false
8392
],
93+
[
94+
"badheader",
95+
true
96+
],
8497
[
8598
"later",
8699
true
@@ -115,6 +128,13 @@
115128
"{\n \"version\": 2\n}"
116129
]
117130
},
131+
"nulls": {
132+
"status": "completed",
133+
"isError": false,
134+
"content": [
135+
"{\"word\":\"abc\"}"
136+
]
137+
},
118138
"pid-before": {
119139
"status": "completed",
120140
"isError": false,
@@ -143,6 +163,13 @@
143163
"Invalid arguments for mcp__e2e__echo: Missing key\n at [\"text\"]"
144164
]
145165
},
166+
"pattern": {
167+
"status": "error",
168+
"isError": true,
169+
"content": [
170+
"Invalid arguments for mcp__e2e__args: Expected a string matching the RegExp ^[a-z]+$\n at [\"word\"]"
171+
]
172+
},
146173
"cwd": {
147174
"status": "completed",
148175
"isError": false,
@@ -178,6 +205,13 @@
178205
"{\"server\":\"e2e\",\"resources\":[{\"server\":\"e2e\",\"name\":\"data\",\"uri\":\"fixture://data.bin\",\"mimeType\":\"application/octet-stream\"}]}"
179206
]
180207
},
208+
"resources-blank": {
209+
"status": "completed",
210+
"isError": false,
211+
"content": [
212+
"{\"server\":\"e2e\",\"resources\":[{\"server\":\"e2e\",\"name\":\"notes\",\"uri\":\"fixture://notes.txt\",\"mimeType\":\"text/plain\"}],\"nextCursor\":\" page-2 \"}"
213+
]
214+
},
181215
"templates": {
182216
"status": "completed",
183217
"isError": false,
@@ -218,7 +252,7 @@
218252
{
219253
"server": "e2e",
220254
"state": "connected",
221-
"tools": 10
255+
"tools": 11
222256
},
223257
{
224258
"server": "off",
@@ -230,15 +264,22 @@
230264
"state": "disabled",
231265
"tools": 0
232266
},
267+
{
268+
"server": "badheader",
269+
"state": "failed",
270+
"error": "MCP server \"badheader\" header \"Authorization\" contains a line break or NUL character",
271+
"tools": 0
272+
},
233273
{
234274
"server": "later",
235275
"state": "connected",
236-
"tools": 10
276+
"tools": 11
237277
}
238278
],
239279
"configErrors": [
240280
"<tmp>/home/mcp.json: server \"my_srv\" conflicts with \"my-srv\"",
241281
"<tmp>/home/mcp.json: server \"broken\": url must be an http or https URL",
282+
"<tmp>/home/mcp.json: server \"signin\": \"oauth\" is not supported yet",
242283
"<tmp>/project/.codework/mcp.json: server \"ghost\" needs \"command\" or \"url\", or a user server to override"
243284
]
244285
}

‎packages/harness/test/__artifacts__/mcp.session.lifecycle.json‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
"declared": [
5959
[
6060
"mcp__e2e__echo",
61+
"mcp__e2e__args",
6162
"mcp__e2e__fail",
6263
"mcp__e2e__pid",
6364
"mcp__e2e__crash",
@@ -74,6 +75,7 @@
7475
],
7576
[
7677
"mcp__e2e__echo",
78+
"mcp__e2e__args",
7779
"mcp__e2e__fail",
7880
"mcp__e2e__pid",
7981
"mcp__e2e__crash",
@@ -90,6 +92,7 @@
9092
],
9193
[
9294
"mcp__e2e__echo",
95+
"mcp__e2e__args",
9396
"mcp__e2e__fail",
9497
"mcp__e2e__pid",
9598
"mcp__e2e__crash",
@@ -106,6 +109,7 @@
106109
],
107110
[
108111
"mcp__e2e__echo",
112+
"mcp__e2e__args",
109113
"mcp__e2e__fail",
110114
"mcp__e2e__pid",
111115
"mcp__e2e__crash",
@@ -121,6 +125,7 @@
121125
],
122126
[
123127
"mcp__e2e__echo",
128+
"mcp__e2e__args",
124129
"mcp__e2e__fail",
125130
"mcp__e2e__pid",
126131
"mcp__e2e__crash",
@@ -155,7 +160,7 @@
155160
{
156161
"server": "e2e",
157162
"state": "connected",
158-
"tools": 10
163+
"tools": 11
159164
},
160165
{
161166
"server": "remote",

‎packages/harness/test/__artifacts__/mcp.session.scopes.json‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,62 +29,62 @@
2929
{
3030
"server": "mine",
3131
"state": "disconnected",
32-
"tools": 10
32+
"tools": 11
3333
},
3434
{
3535
"server": "twin",
3636
"state": "disconnected",
37-
"tools": 10
37+
"tools": 11
3838
},
3939
{
4040
"server": "ours",
4141
"state": "disconnected",
42-
"tools": 10
42+
"tools": 11
4343
},
4444
{
4545
"server": "all",
4646
"state": "disconnected",
47-
"tools": 10
47+
"tools": 11
4848
},
4949
{
5050
"server": "mine",
5151
"state": "disconnected",
52-
"tools": 10
52+
"tools": 11
5353
},
5454
{
5555
"server": "twin",
5656
"state": "disconnected",
57-
"tools": 10
57+
"tools": 11
5858
},
5959
{
6060
"server": "ours",
6161
"state": "disconnected",
62-
"tools": 10
62+
"tools": 11
6363
},
6464
{
6565
"server": "all",
6666
"state": "disconnected",
67-
"tools": 10
67+
"tools": 11
6868
},
6969
{
7070
"server": "mine",
7171
"state": "disconnected",
72-
"tools": 10
72+
"tools": 11
7373
},
7474
{
7575
"server": "twin",
7676
"state": "disconnected",
77-
"tools": 10
77+
"tools": 11
7878
},
7979
{
8080
"server": "ours",
8181
"state": "disconnected",
82-
"tools": 10
82+
"tools": 11
8383
},
8484
{
8585
"server": "all",
8686
"state": "disconnected",
87-
"tools": 10
87+
"tools": 11
8888
}
8989
]
9090
}

‎packages/harness/test/mcp.session.test.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -402,9 +402,14 @@ it("mcp: scopes decide who shares a server, and idle servers close", async () =>
402402
const a = yield* Session.create({ directory: shared, model });
403403
const b = yield* Session.create({ directory: shared, model });
404404
const c = yield* Session.create({ directory: other, model });
405-
yield* a.run("a");
406-
yield* b.run("b");
407-
yield* c.run("c");
405+
// A lease on A's servers spans the three runs, so a slow runner cannot idle out the shared
406+
// instances between them; the short timeout only matters once it is released.
407+
yield* Effect.gen(function* () {
408+
yield* Mcp.Service.use((mcp) => mcp.lease(a.id));
409+
yield* a.run("a");
410+
yield* b.run("b");
411+
yield* c.run("c");
412+
}).pipe(Effect.scoped);
408413
// No drain holds any server now, so every instance closes once the idle timeout passes.
409414
const status = Mcp.Service.use((mcp) =>
410415
Effect.forEach([a, b, c], (session) => mcp.status(session.id)).pipe(Effect.map((all) => all.flat())),
@@ -426,7 +431,7 @@ it("mcp: scopes decide who shares a server, and idle servers close", async () =>
426431
database: ":memory:",
427432
plugins: [defaultPromptPlugin],
428433
llm,
429-
// Long enough for the three exchanges to share instances, short enough to watch them close.
434+
// Short, to watch idle instances close.
430435
mcpIdleTimeout: "1500 millis",
431436
}),
432437
),

0 commit comments

Comments
 (0)