Review

Summary

Verified the fix locally: reverted mcp-client-manager.ts to main, the new regression test fails with expected 2 to be 1 at the disconnectCallsBeforeResolve assertion. After applying the patch, all 8 manager tests pass. The race fix lands as advertised.

Approach

Single-flight via Map<string, Promise<void>> keyed by serverName is the right pattern here — granularity allows cross-server work to remain parallel, and the pointer-equality guard in the finally block (get(...) === discoveryPromise) avoids accidentally deleting a successor's promise. The public/private split is clean.

Issues

1. (medium) Undocumented addition of stopHealthCheck(serverName) at the start of discoverMcpToolsForServerInternal

mcp-client-manager.ts:185 (post-patch) adds this.stopHealthCheck(serverName); before the existing-client cleanup. This is a separate, useful bug fix — previously, if rediscovery failed during connect(), the old health timer would keep ticking against a deleted client until the next failure pushed it into reconnectServer. After this change, the timer is stopped immediately.

Worth either calling it out in the PR description or splitting into its own commit; otherwise future git blame on this line won't tell the full story.

2. (minor) Promise.resolve().then(...) wrapper is unnecessary

const discoveryPromise = Promise.resolve().then(() =>
  this.discoverMcpToolsForServerInternal(serverName, cliConfig),
);
this.serverDiscoveryPromises.set(serverName, discoveryPromise);

discoverMcpToolsForServerInternal is async — calling it directly returns a Promise; the synchronous portion of its body (which contains no recursion back into the public method) runs before the first await. So this is equivalent without the extra microtask hop:

const discoveryPromise = this.discoverMcpToolsForServerInternal(serverName, cliConfig);
this.serverDiscoveryPromises.set(serverName, discoveryPromise);

The current form also makes the "unknown server" no-op path require an extra microtask to settle — a small but unnecessary regression.

3. (minor) stop() does not clear serverDiscoveryPromises

If stop() runs while a discovery is in flight, this.clients.clear() happens, but the in-flight code path will later call this.clients.set(serverName, client), leaking a connected client into a stopped manager. This race exists on main too (so not introduced here), but since this PR is already managing in-flight state, it would be natural to either clear the map in stop() or await outstanding promises before clearing clients. Follow-up is fine, not blocking.

4. (minor) Test mock condition is overly complex

mcp-client-manager.test.ts:248-253:

if (
  replacementClients.length === 0 &&
  vi.mocked(McpClient).mock.calls.length === 1
) {
  return firstClient as unknown as McpClient;
}

mock.calls.length === 1 already uniquely identifies the first construction; the replacementClients.length === 0 clause is redundant. The existing test on line 199 uses the more idiomatic mockReturnValueOnce(first).mockReturnValueOnce(second) chain — would read more cleanly here too.

5. (note, not a bug) Coalesced caller's cliConfig is silently dropped

The second caller's cliConfig argument never reaches client.discover() — the in-flight call uses the originator's config. Today this is fine because cliConfig rarely differs between rediscovery calls, but worth a one-line comment on the internal method to flag the assumption for future readers.

Risk

• Behavior change is scoped to the "concurrent rediscovery for the same server" window. discoverAllMcpTools, single-server reconnect (reconnectServer), and cross-server discovery are unaffected.
• New test covers the core coalescing scenario plus map cleanup (third call must do real work). Existing 6 tests still pass.
• Not covered: rejection path of the in-flight promise (currently internal swallows connect/discover errors, so it doesn't reject in practice) and the stop()-during-discovery race noted in #3.

Recommendation

Direction and implementation are right. Suggest addressing #1 and #2 before merge (mention stopHealthCheck somewhere; drop the Promise.resolve().then wrapper). #3/#4/#5 can be follow-ups.

No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review

Verified locally on dd73312f7: deepseek-v4-pro's #2 (startHealthCheck in finally) and #3 (serverDiscoveryPromises cleanup in stop() / disconnectServer()) are both addressed, with regression coverage for each. Break-fix loop in the comment above demonstrates the new tests genuinely catch the bugs they were written for, and the full core suite (6604 tests) is green.

Approving.