Skip to content

tool_list_locked entries orphaned on server removal / probe-block in connect path #6139

Description

@bug-ops

Description

While fixing #6118 (OAuth-transport MCP servers silently exempted from the lock_tool_list hardening), the adversarial critic and code reviewer independently identified two pre-existing, non-blocking gaps in tool_list_locked cleanup that are now reachable for OAuth server IDs too (previously only reachable via non-OAuth paths, where they already existed unnoticed).

Both are fail-closed and benign (a stale entry only rejects refresh notifications for an id with no live connection — a bounded memory leak across many add/remove cycles of distinct ids, not a security regression), so they were not blocking for #6118 and are filed here as a separate follow-up.

F1: remove_server / shutdown_all_shared never clean tool_list_locked

crates/zeph-mcp/src/manager/server.rs:319-324 (remove_server) and :399-402 (shutdown_all_shared) clear server_tools, server_trust, server_fingerprints, last_refresh, and connected_server_ids — but not tool_list_locked. A server connected under lock_tool_list = true and later removed leaves an orphaned entry in the map.

F2: probe-block cleanup asymmetry in handle_connect_result

The add_server path removes the lock on probe-block (server.rs:210, probe_or_cleanup), but the connect_all/connect_oauth_deferred path's handle_connect_result (crates/zeph-mcp/src/manager/connect.rs:627-630) returns fail() on probe-block without removing the lock — asymmetric with the add_server probe path.

Expected Behavior

tool_list_locked entries are released on server removal, manager shutdown, and probe-block regardless of which connection path (add_server vs. connect_all/OAuth-deferred) established them — centralizing cleanup so all four sites (remove_server, shutdown_all_shared, handle_connect_result probe-block, add_server probe-block) stay in sync.

Actual Behavior

tool_list_locked entries can be orphaned (F1) or left stale after a probe-block on the connect_all/OAuth-deferred path (F2), while the add_server path already cleans up correctly in the equivalent case.

Environment

Logs / Evidence

Metadata

Metadata

Assignees

Labels

P3Research — medium-high complexitybugSomething isn't workingtech-debtTechnical debt

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions