Repository navigation
fix(core): wait for MCP servers only before a session's first request - #58374
JoshuaMart wants to merge 5 commits into
Conversation
MCP servers connect asynchronously, and McpTool.flush only waited for the initial registration, which reads the servers already connected. A session step made while a server was still starting went out without that server's tools; they arrived only with the debounced refresh, on a later step. With `codemode: false` this hides the tools entirely on the first step, so an agent that relies on them without `execute` sees nothing to call. Mcp.settle waits for servers still pending, and flush refreshes the tool registry when it had to wait. Startup stays bounded by each server's startup timeout. Co-Authored-By: Claude Opus 5.5 <[email protected]>
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works. I reproduced the bug with a stdio MCP server that waits 10s before answering initialize (codemode: false), submitting a prompt right away through the TUI with a simulated model. On dev, the first model request went out after ~0.2s without demo_echo. With this PR, it waited ~8.8s and included demo_echo. The new test fails on dev, passes with the PR and was stable over 10 reruns. The rest of test/mcp.test.ts passes except one MCP-descendants test, which fails on dev too. tsc -b tsconfig.tests.json in packages/core passes.
The public API is unchanged: Mcp.Interface.settle is internal to core.
A maintainer should make a design call here. On #51812 a team member said that not blocking on starting servers was intentional. With this change, every step waits silently for any server still in pending, for up to its startup timeout (30s by default). This applies to all servers, not only codemode: false ones, and the TUI shows nothing while it waits. #51812 also suggested limiting the wait to the first request and capping it at a few seconds. One more gap is noted inline.
Address review: the wait no longer runs on every step. - Only a session's first model request (no assistant message yet) waits, through the new McpTool.ready. Later steps keep McpTool.flush and never wait. - Each starting server is waited for at most its `timeout.wait`, which defaults to 3 s and can be set under `mcp.timeout` or per server. - After the wait, the registry is compared with the tools of every connected server and refreshed when they differ. This also covers a server that is connected but whose debounced ToolsChanged refresh has not run yet. Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
The new commit addresses both earlier points. Only a session's first request now waits, capped by timeout.wait (3s by default). ready now compares the connected tools with the registry instead of relying on server status, so a server that has just connected isn't missed.
I re-ran the end-to-end check through the TUI with a simulated model and a slow stdio server (codemode: false):
- 2s server:
devsends the first request withoutdemo_echo; this PR waits about 0.7s and includes it. - 10s server, default wait: the first request goes out after about 3.2s without it, as intended.
- 10s server with
wait: 15000: the first request includes it.
test/mcp.test.ts and test/session-generate.test.ts pass, except the MCP-descendants test, which also fails on dev. tsc -b tsconfig.tests.json in packages/core passes.
Public API: adds an optional timeout.wait (milliseconds, default 3000) under mcp.timeout and mcp.servers.<name>.timeout, with the generated client types and docs updated. It's backwards compatible.
No new concerns.
JoshuaMart
changed the title
Co-Authored-By: Claude Opus 5.5 <[email protected]>
There was a problem hiding this comment.
The new commit only merges upstream dev. The change itself is the same as the version I reviewed last time, apart from the regenerated packages/protocol/openapi.json, which now lists the optional MCP timeout.wait key.
I re-ran the end-to-end check through the TUI with a simulated model:
- A server that takes 2s to start gets its tool into the first request.
- A server that takes 10s is skipped after the default 3s wait.
test/mcp.test.ts and test/session-generate.test.ts pass, except the MCP-descendants test, which also fails on dev. tsc -b tsconfig.tests.json in packages/core passes.
Public API: adds an optional timeout.wait (milliseconds, default 3000) under mcp.timeout and mcp.servers.<name>.timeout, with the OpenAPI spec, generated client types and docs updated. It's backwards compatible.
No new concerns.
|
A case this leaves out: sessions that already have assistant messages. We run
For now we work around it by waiting 1 s before the prompt whenever we saw the server connect, or just replaced it. It would help if the registry check in |
Address review: two cases still missed connected tools. - A session resumed in a fresh process already has assistant messages, so it skipped the startup wait. The wait now runs on a session's first request in this Location instead. - After a server connects or is replaced (PUT /api/experimental/mcp), the next request could still run before the debounced ToolsChanged reload. Every request now compares the registry with the connected servers' tools and reloads when they differ. It never waits for servers that are still starting. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thanks @aguilaair both cases were fixed in c63c0da: Resuming in a fresh process: the startup wait now runs on a session’s first request in the process, not only on its very first model request. A resumed session waits for servers still starting (within timeout.wait) on its first prompt after a restart. Later requests and replaced servers: every request now compares the tool registry with the connected servers’ tools and reloads it when they differ, without waiting. That closes the ~100 ms gap after a server connects and after PUT /api/experimental/mcp/:server. A server that has been connected for a while costs nothing. Checked end to end with opencode serve: after restarting it, run -c on an existing session gets the MCP tool on its first request, and so does a prompt right after replacing the server with the PUT. Before this change, both were missing it. Your 1 s workaround shouldn’t be needed with this. |
There was a problem hiding this comment.
The new commit looks sound. The startup wait now applies to a session's first request in this process, so resumed sessions get it too. Every later request picks up tools from servers that have connected since, without waiting for ones still starting.
I re-ran the end-to-end check through the TUI with a simulated model on the PR:
- A stdio server that takes 2s to start gets
demo_echointo the first request. - A server that takes 10s is skipped after the default 3s wait.
I didn't re-run dev this time; it sent the first request without demo_echo in my earlier runs. I didn't test the resumed-session and mid-session paths end to end; the new unit tests cover them.
test/mcp.test.ts and test/session-generate.test.ts pass, except the MCP-descendants test, which also failed on dev earlier. tsc -b tsconfig.tests.json in packages/core passes.
Public API: unchanged from before. It adds an optional timeout.wait (milliseconds, default 3000) under mcp.timeout and mcp.servers.<name>.timeout, with the OpenAPI spec, generated client types and docs updated. It's backwards compatible.
No blocking concerns. McpTool.flush is no longer called outside tests and could be removed.
Issue for this PR
Fixes #51812
Type of change
What does this PR do?
MCP servers connect in the background, and
SessionContext.selectonly waited for the initial tool registration, which sees only the servers already connected. So a session started while a server is still starting sends its first request without that server's tools. With Code Mode, later steps catch up. Withcodemode: falsethe tools are just missing, and an agent meant to call<server>_<tool>directly usually stops.Startup stays non-blocking. Only a session's first model request (no assistant message yet) calls the new
McpTool.ready:pending, each for at most its newtimeout.wait. The default is 3 s, and it can be set globally undermcp.timeoutor per server.connectedbut whose debouncedToolsChangedrefresh hasn't run yet (the ~100 ms gap reported on First prompt after start has no MCP tools: tool list does not wait for pending MCP connections #51812).Later steps keep using
flushand never wait. A server that connects or fails earlier ends the wait early, so the 3 s cap only applies to a server that is still starting.How did you verify your code works?
packages/core/test/mcp.test.ts:flushdoesn't wait;wait.packages/core/test/session-generate.test.ts: the firstselectusesready, andflushonce an assistant message exists. All new tests pass with--rerun-each 10.bun run script/test.tsinpackages/corepasses, except 7 shell tests that also fail on unmodifieddevon my machine.bun run checkpasses.run --standalone, a stdio server that delaysinitialize,codemode: false, and a fake OpenAI-compatible provider that logs the tools it receives. Times are from CLI launch to the first request.demo_echo, and the model calls it withoutexecute. 2.5 s.demo_echoand leaves at 3.4 s, compared with 0.6 s before this change; it is 0.95 s with"wait": 500.Screenshots / recordings
N/A
Checklist
Added by the opencode bot (AI-generated).
Fixes #51812
Related: #51089
Public API: adds optional MCP
timeout.waitconfig key (default 3000 ms; backwards compatible)