fix(mcp): show startup progress as each server connects - #5897
Conversation
Cargo publication of v0.9.12 stopped at codewhale-tui after 18 crates: its 16 include_str! paths reached outside the package root, so the verified tarball could not compile. Move the existing plugin into the TUI package and update every source, test, CI, documentation, and ignore reference. Keep one canonical bundle, with all 16 embedded runtime files byte-identical to the v0.9.12 release payload. Validation: - Original cargo publish --locked -p codewhale-tui: FAIL, 16 missing-file errors; Cargo refused the upload. - Corrected cargo package --locked --allow-dirty -p codewhale-tui: PASS, including compilation of the unpacked library and binary. - npm test in crates/tui/plugins/computer-use: 31 passed; 0 failed. - Embedded payload comparison: 16/16 original files identical in source and in the verified Cargo tarball. - cargo fmt --all -- --check: PASS. - Coordination npm test && npm run check:web gate: unavailable here; this Rust repository defines neither root script (both report Missing script). No broad-suite or hosted-CI pass is claimed. This is a local packaging repair based on dcd4c20. It does not move the published v0.9.12 tag or alter the already-published release binaries.
Addresses #5887. Return the owned concurrent connection tasks to callers and consume each completion immediately. Keep the eight-connection limit, short pool locks, catalog generation checks, required-server errors, and cancellation-on-drop. Validation: 157/157 focused nextest tests passed with repository-standard RUST_MIN_STACK=16777216; includes a real engine event-loop fixture holding one server while the ready server becomes visible. cargo check, Clippy -D warnings, and fmt passed. An earlier broad libtest filter hit the unrelated setup test stack overflow under the default stack; no full workspace gate claimed. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d0f84519-0fd0-49de-8b93-d0eea008f5e7) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…gress-0913-20260905
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e818c6b8-8f33-4b8e-91f9-f57adb9ba9ae) |
|
Claude finished @Hmbown's task in 2m 35s —— View job Review of #5897 — show MCP startup progress as each server connects
SummaryThe core change is sound. The new test Plugin relocation ( No provider/model-prefix routing code is touched by this PR — the EPIC #2608 invariant doesn't apply here. Minor / non-blocking notes
Nothing else stood out as a correctness, race-condition, or safety issue. I was not able to run |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes concurrency/timing in MCP boot and tool-pool visibility, which is correctness-sensitive and best validated with final human review despite good targeted regression coverage.
Pull request overview
Improves MCP session boot responsiveness by applying connection results as each server finishes handshaking (instead of buffering until the slowest finishes), so the TUI’s “connecting” count and available tool pool reflect real-time progress during startup.
Changes:
- Replaced the “connect and await all” helper with
spawn_pending_connects, returning a bounded-concurrencyJoinSetthat callers canjoin_next()incrementally. - Updated MCP session boot (and pool connect flow) to store each successful connection under a short lock and emit per-server progress updates immediately as each task completes.
- Added an engine integration test that stalls one stdio server during
initializeand asserts the fast server becomes visible (and connecting count drops) before the slow server is released.
File summaries
| File | Description |
|---|---|
| crates/tui/src/mcp.rs | Introduces spawn_pending_connects (returns JoinSet) and adjusts connect flow to consume results incrementally while preserving concurrency limits and error behavior. |
| crates/tui/src/core/engine.rs | Updates MCP session boot to join_next() each connection task, update the shared pool under a short lock, and emit per-completion progress events. |
| crates/tui/src/core/engine/tests.rs | Adds a regression test ensuring mid-boot snapshots show early-ready servers even if another server stalls. |
Review details
- Files reviewed: 8/32 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4530a19ceb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "fast": { "command": "node", "args": [server, "fast", release] }, | ||
| "slow": { "command": "node", "args": [server, "slow", release] } |
There was a problem hiding this comment.
On Rust-only or minimal build hosts where node is not on PATH, both fixture connections fail to spawn, so the progress loop consumes the finished event, waits for its 10-second timeout, and the subsequent finished loop waits another 10 seconds before the test fails. Node is not a required dependency for Codewhale, and the existing Node-backed test in crates/tui/src/mcp/tests.rs explicitly checks availability and skips when absent; add the same guard here or use a self-contained test server so the TUI library suite remains portable.
Useful? React with 👍 / 👎.
| { | ||
| let mut pool = pool_for_task.lock().await; | ||
| match result { | ||
| Ok(connection) => pool.store_ready_connection(name, connection), |
There was a problem hiding this comment.
If the MCP config is edited while a slow startup connection is pending, an already-ready server exposed by this change can be invoked and cause get_or_connect to reload the pool and drop connections under the old config; when the slow task later completes, this unconditional store re-inserts its stale connection and store_ready_connection stamps it with the new catalog generation. Because all_tools projects stored connections and a subsequent get_or_connect returns a ready connection without checking that its server is still configured or enabled, a removed, disabled, or authority-changed server can remain advertised and executable. Reject completions whose captured generation/config authority is stale, or reconcile the config before inserting each result.
AGENTS.md reference: AGENTS.md:L28-L29
Useful? React with 👍 / 👎.
| let _ = progress_tx.send(McpBootUpdate::Progress { | ||
| generation, |
There was a problem hiding this comment.
When one server completes between user turns while at least one slower server is still connecting, this progress path makes the newly ready server's tools appear in the next turn but does not set pending_prefix_change_reason; that reason is set only by the later Finished update. The next request therefore changes the session-pinned tools array as undeclared drift, which trips the C5 debug_assert! in debug builds and records an avoidable KV-cache miss in release builds. Either keep the MCP catalog frozen until boot finishes or stamp a declared prefix change whenever a progress update exposes new tools.
AGENTS.md reference: AGENTS.md:L136-L139
Useful? React with 👍 / 👎.
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
PR replaces bulk MCP connect collection with per-completion JoinSet processing so the engine emits progress and stores ready connections as each server finishes, and relocates the computer-use plugin bundle to crates/tui/plugins. The change is generally well-formed; the main concerns are panic bookkeeping and test coverage for tool availability.
Findings
- [WARNING] Panicked connect tasks leave the real server in the connecting list (
crates/tui/src/core/engine.rs:6497)
When a spawned connect task panics, join_next returns a JoinError and the code synthesizes the name "connection task".remaining.retain(|pending_name| pending_name != &name)does not remove the actual pending server name, so subsequent McpBootUpdate::Progress events can continue to report that server as connecting and connection_errors is keyed under the synthetic name. This was present before, but the new per-completion progress makes the stale UI state directly visible.
Suggestions
crates/tui/src/core/engine/tests.rs— Extend the new mcp_boot_reports_ready_server_before_stalled_server_finishes test to also assert that the fast server's tools are already available in the MCP pool before the slow server is released. The current snapshot assertion proves the progress list is fixed, but not the claimed tool-availability behavior.
Assessment
The core change correctly removes the slowest-server bottleneck and keeps the eight-connection limit intact. I would like the panicked-task bookkeeping fixed and the regression test extended to verify ready tools are admitted before slow servers finish; otherwise the implementation matches the stated goals.
Advisory review by Codewhale (codewhale review --pr 5897 --post, head d4e6c868188d67c31ddef090cee7889783a3ff1d). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| while let Some(joined) = connects.join_next().await { | ||
| let (name, result) = joined | ||
| .unwrap_or_else(|error| ("connection task".to_string(), Err(error.into()))); | ||
| remaining.retain(|pending_name| pending_name != &name); |
There was a problem hiding this comment.
[WARNING] Panicked connect tasks leave the real server in the connecting list
When a spawned connect task panics, join_next returns a JoinError and the code synthesizes the name "connection task". remaining.retain(|pending_name| pending_name != &name) does not remove the actual pending server name, so subsequent McpBootUpdate::Progress events can continue to report that server as connecting and connection_errors is keyed under the synthetic name. This was present before, but the new per-completion progress makes the stale UI state directly visible.
Revalidate config before each background completion, reject obsolete connection generations, and check plugin authority before insertion. Declare boot catalog changes at progress and tool snapshot boundaries; retain server names when handshakes panic. Validation: 160 focused nextest tests passed; cargo check and clippy -D warnings passed; cargo fmt and git diff --check passed. Node fixture verifies tools are usable before the stalled handshake completes and removed/disabled servers cannot be restored. Runtime root has no npm test/check:web scripts. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d3c1fa23-39a5-490f-989b-2dbfdd34d291) |
|
Claude finished @Hmbown's task in 3m 33s —— View job Review of #5897 — show MCP startup progress as each server connects
SummaryNote: The newest commit, 9d05c12 "reject stale boot connections and declare live catalog changes," directly answers every substantive finding raised by the earlier automated reviews on this PR:
I re-read these fixes against their call sites rather than trusting the commit message; the generation/authority checks line up correctly (no intervening Minor / non-blocking
No correctness, race, or panic issues found beyond the above. Provider/model routing (EPIC #2608 invariant) is untouched by this PR — confirmed via |
|
Review follow-up is pushed at 9d05c12. Background completion re-reads configuration, cancels obsolete batches, and refuses to re-stamp stale connection generations. Plugin authority is checked before insertion. Progress application, queue draining, and the in-flight tool snapshot all declare the catalog refresh. Handshake panics retain their server name, and Node fixtures skip cleanly when Node is unavailable. 160 focused tests pass, including ready tool availability before a stalled handshake completes, config removal/disable during that handshake, stale generation rejection, and prefix declarations. Cargo check, clippy with warnings denied, formatting, and diff checks pass. Hosted CI is running on this new head. |
CI correctly rejected the stderr diagnostic under clippy::print_stderr. Replace it with tracing. Validation: cargo clippy --workspace --all-targets --all-features --locked -- -D warnings passed; cargo fmt and diff checks passed. The preceding behavioral test run passed 160/160. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2f6d1501-7015-41e6-8320-0278981b398d) |
|
Claude finished @Hmbown's task in 1m 56s —— View job Review of #5897 — show MCP startup progress as each server connects
SummaryDiffed
I also independently re-verified (by reading the actual diffs, not just trusting the prior comments) that
Plugin relocation ( No provider/model-prefix routing code is touched ( Minor / still open, non-blocking
VerdictNothing new to flag. This round's only change ( |
There was a problem hiding this comment.
Codewhale review
PR #5897 replaces the batched MCP connection wait with a JoinSet consumed as tasks complete, so progress events and pool tools are emitted as each server finishes. It also tightens stale-generation handling in store_ready_connection and updates computer-use bundle paths. The core approach is sound and the added tests cover fast-before-slow visibility and stale pool insertion, but two concurrency/bookkeeping risks remain.
Findings
- [WARNING] Pool mutex is held across an await in the incremental boot loop (
crates/tui/src/core/engine.rs:6506)
The engine boot task locks pool_for_task and then awaits pool.reload_if_config_changed() while still holding the guard. This can block mcp_tools() snapshots, TUI progress reads, and other pool users for filesystem/latency, which contradicts the short-lock intent and can stall UI during config reload. - [WARNING] Explicit connect-all records stale config errors as connection failures (
crates/tui/src/mcp.rs:3214)
In McpPool::connect_all_servers, a result rejected by store_ready_connection because the catalog generation changed is passed to note_connect_failure and pushed into the returned errors. That pollutes per-server backoff/auth state for servers that may be valid under the newly reloaded config; stale successes should be ignored or remaining connects aborted, as the engine boot loop does.
Assessment
The implementation is close and the test coverage is good, but the async lock hold should be scoped down and the connect-all stale-result path should avoid recording false failures before merge.
Advisory review by Codewhale (codewhale review --pr 5897 --post, head c92ebf795953318162b77d68d155f132957bbde2). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| // A turn may have reloaded the pool while these handshakes | ||
| // were in flight. Never let their old authority or failures | ||
| // overwrite the newly installed configuration. | ||
| let reload = pool.reload_if_config_changed().await; |
There was a problem hiding this comment.
[WARNING] Pool mutex is held across an await in the incremental boot loop
The engine boot task locks pool_for_task and then awaits pool.reload_if_config_changed() while still holding the guard. This can block mcp_tools() snapshots, TUI progress reads, and other pool users for filesystem/latency, which contradicts the short-lock intent and can stall UI during config reload.
There was a problem hiding this comment.
Checked the callee: reload_if_config_changed is an async wrapper whose body only calls the synchronous reload_from_config_sources(false); it contains no await and performs no handshake/network request. Its future does not park while holding the guard. The mtime/config check is intentional at insertion so a stale handshake cannot restore revoked configuration. The existing reload implementation uses synchronous filesystem reads under the same pool authority; moving those reads off-thread would be a separate reload refactor, not a correction to the claimed suspended-await behavior. The delayed-server test verifies ready tools remain available while another handshake is stalled.
| let result = result | ||
| .and_then(|connection| self.store_ready_connection(name.clone(), connection)); | ||
| if let Err(error) = result { | ||
| self.note_connect_failure(&name, &error); |
There was a problem hiding this comment.
[WARNING] Explicit connect-all records stale config errors as connection failures
In McpPool::connect_all_servers, a result rejected by store_ready_connection because the catalog generation changed is passed to note_connect_failure and pushed into the returned errors. That pollutes per-server backoff/auth state for servers that may be valid under the newly reloaded config; stale successes should be ignored or remaining connects aborted, as the engine boot loop does.
There was a problem hiding this comment.
In connect_all, &mut self is held for the complete batch. The catalog-generation atomic belongs to this pool (it is not a cloned Arc), and its mutation methods require borrowing this same pool; another safe caller cannot mutate it concurrently with that exclusive borrow. The config reload occurs after the batch, and that existing path clears the old pass's errors before retrying. The background engine pass is the path that releases the pool between handshakes, and it now explicitly rejects/cancels obsolete batches before recording success or failure. I do not see a reachable stale-generation/backoff case in the exclusive connect_all path.
Startup buffered every MCP connection result until the slowest batch finished, so the TUI could keep showing “20 connecting” even after individual servers were ready. Ready tools were also unavailable in the pool during that wait.
Consume each owned connection task as it completes, update the engine-owned pool under a short lock, and emit per-server progress immediately. Both startup and explicit connect-all use the same bounded task set. Preserve the eight-connection limit, catalog generations, plugin-authority validation, required-server errors, and cancellation when the task set is dropped.
Fixes #5887. The deterministic fixture proves this startup batching defect; it does not reproduce every server in the founder’s private configuration.
Validation: 157/157 focused MCP and engine nextest tests passed with the documented 16 MiB test stack; cargo check, Clippy with -D warnings, and formatting pass. A real engine event-loop fixture holds one stdio server until a release file is written and verifies that the other server is already visible and the connecting count has dropped before that release. Boot completion, retry ownership, stale-generation handling, catalog and authority tests pass.
An initial broad libtest substring filter also selected an unrelated setup test and aborted on its default-stack overflow; the focused run uses the repository’s documented stack setting. Full workspace tests were not run locally.
Based on the bundle relocation in #5890. This PR is v0.9.13 work and does not retag or republish v0.9.12.
Note
Medium Risk
Changes concurrent MCP startup, pool storage, and config-reload cancellation in the engine path that feeds the tool catalog and TUI boot state.
Overview
Fixes MCP session boot so the TUI and tool catalog update as each server finishes handshaking, instead of waiting for the whole batch. The engine now
join_nexts on a boundedJoinSetfromspawn_pending_connects, stores ready connections under a short pool lock, and sendsMcpBootUpdate::Progressafter every completion so the connecting list and snapshot shrink incrementally.Stale-config safety is tightened:
store_ready_connectionreturnsResultand rejects connections whosecatalog_generationno longer matches the pool; mid-boot reloads abort remaining connect tasks and stop applying in-flight results. Boot progress and earlymcp_tools()during in-flight boot setpending_prefix_change_reasontomcp-session-bootso the KV prefix refreshes when tools appear before the final boot event.Also updates computer-use references from
plugins/computer-usetocrates/tui/plugins/computer-use(CI, gitignore, embedded bundle paths, docs)—aligned with the bundle relocation in #5890.New engine and pool tests cover fast-before-slow visibility, config invalidation during handshake, and prefix declaration on progress/catalog read.
Reviewed by Cursor Bugbot for commit c92ebf7. Bugbot is set up for automated code reviews on this repo. Configure here.