fix(mcp,core): resolve critical TODOs — MCP Await Discipline violation + stale comments#3537
Merged
fix(mcp,core): resolve critical TODOs — MCP Await Discipline violation + stale comments#3537
Conversation
…n, stale test import comments - Rewrite `commit_added_server` in zeph-mcp to satisfy Await Discipline §4: acquire and release `clients` write guard with no `.await` while held, then write `server_trust` and `server_tools` independently. Previous code held `clients` guard across two async awaits creating a deadlock hazard. - Add `commit_added_server_rejects_duplicate` test covering sequential duplicate rejection and winner-state preservation for trust level and tools. - Add `McpClient::new_disconnected_for_test` (#[cfg(test)] pub(crate)) stub. - Document the narrow concurrent add/remove race window introduced by the fix with a TODO referencing #3536 for tracking. - Remove three stale `TODO(critic): #3497 follow-up` comments from test aggregator mod.rs files where the path migration was already complete. - Add deferred migration marker in tool_execution/native.rs for the ~30 remaining `super::super::*` occurrences (follow-up to #3497).
github-actions
Bot
added
rust
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
1 participant
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.
Summary
commit_added_serverAwait Discipline §4 violation (zeph-mcp): the previous code held theclientswrite guard across two.awaitpoints (server_trust.write().await,server_tools.write().await), creating a potential deadlock if another task heldserver_trustwrite while waiting forclients. Fixed by releasing theclientsguard before all subsequent awaits; TOCTOU protection preserved.TODO(critic)comments inzeph-coretest aggregator files where thesuper::super::*→crate::migration was already complete.tool_execution/native.rsfor ~30 remainingsuper::super::*occurrences (separate follow-up, not part of original TODOs).New issues filed
commit_added_server+remove_serverrace (documented with TODO in code; separate fix tracked there)Test plan
cargo +nightly fmt --check— cleancargo clippy --workspace -- -D warnings— 0 warningscargo nextest run -p zeph-mcp --lib— 429/429 passedcargo nextest run -p zeph-core --lib— 1302/1302 passedcommit_added_server_rejects_duplicatecovers sequential duplicate rejection and winner-state preservationCloses #3534 (partial — the Await Discipline TODO is now resolved)