https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/208827
>From 9ab295a249cb39f4ce98a609b6333d1cdcf255ed Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <[email protected]> Date: Wed, 8 Jul 2026 14:47:47 -0700 Subject: [PATCH 1/2] [lldb-mcp] Multiplex across all discovered LLDB instances Connect to every LLDB MCP server advertised under ~/.lldb rather than a single one, and present them to the client as one server. A stale registry entry from a crashed instance simply fails to connect and is skipped. Each instance is identified by the pid of its lldb process, now recorded in the ServerInfo registry file. Tools and resources are addressed with instance-qualified URIs, e.g. lldb-mcp://instance/{pid}/debugger/{id} and lldb://instance/{pid}/debugger/{id}/target/{idx}. Listing requests (sessions_list, resources/list) fan out to every backend and aggregate; targeted requests (command, resources/read) are routed by the pid parsed from the URI. Backends only know their local lldb-mcp://debugger/{id} form, so URIs are rewritten in both directions. Add Binder::FailPendingRequests (and Client::CancelPendingRequests) so that when the client disconnects with a request still in flight to a backend, the abandoned reply is satisfied with an error instead of being destroyed unanswered, which would trip the binder's "must reply" assert. The multiplexer cancels its backends on shutdown before unwinding. Assisted-by: Claude --- lldb/include/lldb/Host/JSONTransport.h | 13 + lldb/include/lldb/Protocol/MCP/Client.h | 5 + lldb/include/lldb/Protocol/MCP/Server.h | 5 + lldb/source/Protocol/MCP/Client.cpp | 4 + lldb/source/Protocol/MCP/Server.cpp | 14 +- lldb/tools/lldb-mcp/Multiplexer.cpp | 265 ++++++++-- lldb/tools/lldb-mcp/Multiplexer.h | 66 ++- lldb/tools/lldb-mcp/lldb-mcp.cpp | 58 ++- lldb/unittests/Host/JSONTransportTest.cpp | 20 + lldb/unittests/Protocol/MCPServerInfoTest.cpp | 2 + lldb/unittests/tools/CMakeLists.txt | 2 + lldb/unittests/tools/lldb-mcp/CMakeLists.txt | 15 + .../tools/lldb-mcp/MultiplexerTest.cpp | 467 ++++++++++++++++++ 13 files changed, 866 insertions(+), 70 deletions(-) create mode 100644 lldb/unittests/tools/lldb-mcp/CMakeLists.txt create mode 100644 lldb/unittests/tools/lldb-mcp/MultiplexerTest.cpp diff --git a/lldb/include/lldb/Host/JSONTransport.h b/lldb/include/lldb/Host/JSONTransport.h index 0f58697e9f34d..87adbbc04eeda 100644 --- a/lldb/include/lldb/Host/JSONTransport.h +++ b/lldb/include/lldb/Host/JSONTransport.h @@ -592,6 +592,19 @@ class Binder : public JSONTransport<Proto>::MessageHandler { m_disconnect_handler(); } + /// Fails every in-flight outgoing request, invoking its reply with an error. + /// Call when the connection is going away, so pending replies are satisfied + /// rather than destroyed unanswered. + void FailPendingRequests(llvm::StringRef reason) { + std::scoped_lock<std::recursive_mutex> guard(m_mutex); + std::map<Id, Callback<void(const Resp &)>> pending; + std::swap(pending, m_pending_responses); + for (auto &entry : pending) { + Req req = Proto::Make(entry.first, /*method=*/"", std::nullopt); + entry.second(Proto::Make(req, llvm::createStringError(reason))); + } + } + private: template <typename T> llvm::Expected<T> static Parse(const llvm::json::Value &raw, diff --git a/lldb/include/lldb/Protocol/MCP/Client.h b/lldb/include/lldb/Protocol/MCP/Client.h index cefbd136cda2a..71e179c669eda 100644 --- a/lldb/include/lldb/Protocol/MCP/Client.h +++ b/lldb/include/lldb/Protocol/MCP/Client.h @@ -57,6 +57,11 @@ class Client { /// MCP notifications. void NotifyInitialized(); + /// Fails every request still awaiting a response, invoking its reply with an + /// error. Use before teardown so abandoned replies are satisfied rather than + /// destroyed unanswered. + void CancelPendingRequests(llvm::StringRef reason); + private: void Log(llvm::StringRef message); diff --git a/lldb/include/lldb/Protocol/MCP/Server.h b/lldb/include/lldb/Protocol/MCP/Server.h index 498c54bed780e..951df733f6b1e 100644 --- a/lldb/include/lldb/Protocol/MCP/Server.h +++ b/lldb/include/lldb/Protocol/MCP/Server.h @@ -14,6 +14,8 @@ #include "lldb/Protocol/MCP/Resource.h" #include "lldb/Protocol/MCP/Tool.h" #include "lldb/Protocol/MCP/Transport.h" +#include "lldb/lldb-defines.h" +#include "lldb/lldb-types.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" @@ -86,6 +88,9 @@ class ServerInfoHandle; /// coordinate connecting an lldb-mcp client. struct ServerInfo { std::string connection_uri; + /// Process id of the lldb instance hosting this server, used as its stable + /// identity. + lldb::pid_t pid = LLDB_INVALID_PROCESS_ID; /// Writes the server info into a unique file in `~/.lldb`. static llvm::Expected<ServerInfoHandle> Write(const ServerInfo &); diff --git a/lldb/source/Protocol/MCP/Client.cpp b/lldb/source/Protocol/MCP/Client.cpp index edf651920ada2..8611a08086948 100644 --- a/lldb/source/Protocol/MCP/Client.cpp +++ b/lldb/source/Protocol/MCP/Client.cpp @@ -81,6 +81,10 @@ void Client::ResourcesRead(const ReadResourceParams ¶ms, void Client::NotifyInitialized() { m_notify_initialized(); } +void Client::CancelPendingRequests(llvm::StringRef reason) { + m_binder->FailPendingRequests(reason); +} + void Client::Log(llvm::StringRef message) { if (m_log_callback) m_log_callback(message); diff --git a/lldb/source/Protocol/MCP/Server.cpp b/lldb/source/Protocol/MCP/Server.cpp index ecbea4b9022c0..c0ea6785ddb2a 100644 --- a/lldb/source/Protocol/MCP/Server.cpp +++ b/lldb/source/Protocol/MCP/Server.cpp @@ -50,17 +50,23 @@ void ServerInfoHandle::Remove() { } json::Value lldb_protocol::mcp::toJSON(const ServerInfo &SM) { - return json::Object{{"connection_uri", SM.connection_uri}}; + return json::Object{{"connection_uri", SM.connection_uri}, {"pid", SM.pid}}; } bool lldb_protocol::mcp::fromJSON(const json::Value &V, ServerInfo &SM, json::Path P) { json::ObjectMapper O(V, P); - return O && O.map("connection_uri", SM.connection_uri); + return O && O.map("connection_uri", SM.connection_uri) && + O.mapOptional("pid", SM.pid); } Expected<ServerInfoHandle> ServerInfo::Write(const ServerInfo &info) { - std::string buf = formatv("{0}", toJSON(info)).str(); + // The file is written by the process that hosts the server, so stamp its pid + // to keep it consistent with the filename below. + ServerInfo stamped = info; + stamped.pid = getpid(); + + std::string buf = formatv("{0}", toJSON(stamped)).str(); size_t num_bytes = buf.size(); FileSpec user_lldb_dir = HostInfo::GetUserLLDBDir(); @@ -70,7 +76,7 @@ Expected<ServerInfoHandle> ServerInfo::Write(const ServerInfo &info) { return error.takeError(); FileSpec mcp_registry_entry_path = user_lldb_dir.CopyByAppendingPathComponent( - formatv("lldb-mcp-{0}.json", getpid()).str()); + formatv("lldb-mcp-{0}.json", stamped.pid).str()); const File::OpenOptions flags = File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate | diff --git a/lldb/tools/lldb-mcp/Multiplexer.cpp b/lldb/tools/lldb-mcp/Multiplexer.cpp index 02bb40477c30a..bfd7140031ebc 100644 --- a/lldb/tools/lldb-mcp/Multiplexer.cpp +++ b/lldb/tools/lldb-mcp/Multiplexer.cpp @@ -7,10 +7,12 @@ //===----------------------------------------------------------------------===// #include "Multiplexer.h" -#include "llvm/ADT/StringSwitch.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include "llvm/Support/FormatVariadic.h" #include "llvm/Support/JSON.h" +#include <map> +#include <memory> using namespace llvm; using namespace lldb_protocol::mcp; @@ -33,8 +35,69 @@ constexpr llvm::StringLiteral kBackendToolCommand = "command"; constexpr llvm::StringLiteral kBackendToolDebuggerList = "debugger_list"; /// @} +/// Backend-local URI prefixes. +/// @{ +constexpr llvm::StringLiteral kDebuggerLocalPrefix = "lldb-mcp://debugger/"; +constexpr llvm::StringLiteral kResourceLocalPrefix = "lldb://debugger/"; +/// @} + +std::string replaceAll(StringRef text, StringRef from, StringRef to) { + std::string result; + size_t pos = 0; + while (true) { + size_t next = text.find(from, pos); + if (next == StringRef::npos) { + result += text.substr(pos).str(); + break; + } + result += text.substr(pos, next - pos).str(); + result += to.str(); + pos = next + from.size(); + } + return result; +} + +CallToolResult makeTextResult(std::string text) { + CallToolResult result; + result.content.emplace_back(TextContent{{std::move(text)}}); + return result; +} + } // namespace +std::string lldb_mcp::RewriteURIsToGlobal(StringRef text, lldb::pid_t pid) { + std::string debugger_global = + formatv("lldb-mcp://instance/{0}/debugger/", pid).str(); + std::string resource_global = + formatv("lldb://instance/{0}/debugger/", pid).str(); + std::string step = replaceAll(text, kDebuggerLocalPrefix, debugger_global); + return replaceAll(step, kResourceLocalPrefix, resource_global); +} + +std::optional<RoutedURI> lldb_mcp::ParseGlobalURI(StringRef uri) { + size_t scheme_end = uri.find("://"); + if (scheme_end == StringRef::npos) + return std::nullopt; + + StringRef scheme = uri.take_front(scheme_end); + StringRef rest = uri.drop_front(scheme_end + 3); + if (!rest.consume_front("instance/")) + return std::nullopt; + + size_t slash = rest.find('/'); + StringRef pid_str = slash == StringRef::npos ? rest : rest.take_front(slash); + lldb::pid_t pid; + if (pid_str.getAsInteger(10, pid)) + return std::nullopt; + + StringRef tail = + slash == StringRef::npos ? StringRef() : rest.drop_front(slash + 1); + RoutedURI routed; + routed.pid = pid; + routed.local = formatv("{0}://{1}", scheme, tail).str(); + return routed; +} + Multiplexer::Multiplexer(std::unique_ptr<MCPTransport> client_transport, LogCallback log_callback) : m_client_transport(std::move(client_transport)), @@ -55,8 +118,18 @@ Multiplexer::Multiplexer(std::unique_ptr<MCPTransport> client_transport, }); } -void Multiplexer::AddBackend(std::unique_ptr<Client> backend) { - m_backends.push_back(std::move(backend)); +void Multiplexer::AddBackend(lldb::pid_t pid, std::unique_ptr<Client> backend) { + // A backend that disconnects or errors mid-request will never answer its + // forwarded requests. Fail them so the client sees an error instead of + // waiting forever, since there is no request timeout. + Client *client = backend.get(); + client->SetDisconnectHandler( + [client] { client->CancelPendingRequests("backend disconnected"); }); + client->SetErrorHandler([client](llvm::Error error) { + consumeError(std::move(error)); + client->CancelPendingRequests("backend transport error"); + }); + m_backends.push_back(Backend{pid, std::move(backend)}); } llvm::Error Multiplexer::Run() { @@ -77,10 +150,20 @@ void Multiplexer::SetDisconnectHandler(llvm::unique_function<void()> handler) { m_disconnect_handler = std::move(handler); } -Client *Multiplexer::Route() { - if (m_backends.empty()) - return nullptr; - return m_backends.front().get(); +void Multiplexer::Shutdown() { + for (Backend &backend : m_backends) + backend.client->CancelPendingRequests("lldb-mcp is shutting down"); +} + +Client *Multiplexer::RouteToPid(lldb::pid_t pid) { + for (Backend &backend : m_backends) + if (backend.pid == pid) + return backend.client.get(); + return nullptr; +} + +Client *Multiplexer::First() { + return m_backends.empty() ? nullptr : m_backends.front().client.get(); } Expected<InitializeResult> @@ -108,10 +191,12 @@ Expected<ListToolsResult> Multiplexer::HandleToolsList() { json::Object{{"type", "string"}, {"description", "The LLDB command to run."}}}, {"debugger", - json::Object{{"type", "string"}, - {"description", - "The debugger id or URI selecting the debug " - "session. Defaults to the first."}}}, + json::Object{ + {"type", "string"}, + {"description", + "The debugger URI selecting the debug session, as reported by " + "sessions_list, e.g. lldb-mcp://instance/{pid}/debugger/{id}. " + "Defaults to the first session."}}}, }}, {"required", json::Array{"command"}}, }; @@ -119,7 +204,8 @@ Expected<ListToolsResult> Multiplexer::HandleToolsList() { ToolDefinition sessions_list; sessions_list.name = kToolSessionsList; - sessions_list.description = "List the active debug sessions."; + sessions_list.description = + "List the active debug sessions across all lldb instances."; sessions_list.inputSchema = json::Object{{"type", "object"}}; result.tools.push_back(std::move(sessions_list)); @@ -128,38 +214,157 @@ Expected<ListToolsResult> Multiplexer::HandleToolsList() { void Multiplexer::HandleToolsCall(const CallToolParams ¶ms, Reply<CallToolResult> reply) { - Client *backend = Route(); + if (params.name == kToolCommand) + return HandleCommand(params, std::move(reply)); + if (params.name == kToolSessionsList) + return HandleSessionsList(std::move(reply)); + reply(createStringError(formatv("no tool \"{0}\"", params.name))); +} + +void Multiplexer::HandleCommand(const CallToolParams ¶ms, + Reply<CallToolResult> reply) { + json::Object args; + if (params.arguments) + if (const json::Object *object = params.arguments->getAsObject()) + args = *object; + + std::string debugger_arg; + if (std::optional<StringRef> debugger = args.getString("debugger")) + debugger_arg = debugger->str(); + + Client *backend = nullptr; + if (debugger_arg.empty()) { + // Default to the first session, letting the backend pick its debugger. + backend = First(); + args.erase("debugger"); + } else { + std::optional<RoutedURI> routed = ParseGlobalURI(debugger_arg); + if (!routed) + return reply(createStringError( + formatv("malformed debugger uri \"{0}\"", debugger_arg))); + backend = RouteToPid(routed->pid); + args["debugger"] = routed->local; + } + if (!backend) return reply(createStringError("no debug session available")); - std::optional<StringRef> backend_tool = - StringSwitch<std::optional<StringRef>>(params.name) - .Case(kToolCommand, kBackendToolCommand) - .Case(kToolSessionsList, kBackendToolDebuggerList) - .Default(std::nullopt); - if (!backend_tool) - return reply(createStringError(formatv("no tool \"{0}\"", params.name))); - CallToolParams backend_params; - backend_params.arguments = params.arguments; - backend_params.name = *backend_tool; - + backend_params.name = kBackendToolCommand; + backend_params.arguments = json::Value(std::move(args)); backend->ToolsCall(backend_params, std::move(reply)); } +void Multiplexer::HandleSessionsList(Reply<CallToolResult> reply) { + if (m_backends.empty()) + return reply(makeTextResult("")); + + struct State { + size_t remaining; + // Keyed by pid so the aggregated output is deterministic. + std::map<lldb::pid_t, std::string> texts; + std::string error; + Reply<CallToolResult> reply; + }; + auto state = std::make_shared<State>(); + state->remaining = m_backends.size(); + state->reply = std::move(reply); + + for (Backend &backend : m_backends) { + lldb::pid_t pid = backend.pid; + CallToolParams params; + params.name = kBackendToolDebuggerList; + backend.client->ToolsCall( + params, [state, pid](Expected<CallToolResult> result) { + if (result) { + std::string text; + for (const TextContent &content : result->content) + text += RewriteURIsToGlobal(content.text, pid); + state->texts[pid] = std::move(text); + } else if (state->error.empty()) { + state->error = toString(result.takeError()); + } else { + consumeError(result.takeError()); + } + + if (--state->remaining != 0) + return; + if (!state->error.empty()) + return state->reply(createStringError(state->error)); + std::string combined; + for (const auto &entry : state->texts) + combined += entry.second; + state->reply(makeTextResult(combined)); + }); + } +} + void Multiplexer::HandleResourcesList(Reply<ListResourcesResult> reply) { - Client *backend = Route(); - if (!backend) + if (m_backends.empty()) return reply(ListResourcesResult{}); - backend->ResourcesList(std::move(reply)); + + struct State { + size_t remaining; + std::map<lldb::pid_t, std::vector<Resource>> resources; + std::string error; + Reply<ListResourcesResult> reply; + }; + auto state = std::make_shared<State>(); + state->remaining = m_backends.size(); + state->reply = std::move(reply); + + for (Backend &backend : m_backends) { + lldb::pid_t pid = backend.pid; + backend.client->ResourcesList( + [state, pid](Expected<ListResourcesResult> result) { + if (result) { + std::vector<Resource> rewritten; + for (Resource resource : result->resources) { + resource.uri = RewriteURIsToGlobal(resource.uri, pid); + rewritten.push_back(std::move(resource)); + } + state->resources[pid] = std::move(rewritten); + } else if (state->error.empty()) { + state->error = toString(result.takeError()); + } else { + consumeError(result.takeError()); + } + + if (--state->remaining != 0) + return; + if (!state->error.empty()) + return state->reply(createStringError(state->error)); + ListResourcesResult combined; + for (auto &entry : state->resources) + for (Resource &resource : entry.second) + combined.resources.push_back(std::move(resource)); + state->reply(std::move(combined)); + }); + } } void Multiplexer::HandleResourcesRead(const ReadResourceParams ¶ms, Reply<ReadResourceResult> reply) { - Client *backend = Route(); + std::optional<RoutedURI> routed = ParseGlobalURI(params.uri); + if (!routed) + return reply(createStringError( + formatv("malformed resource uri \"{0}\"", params.uri))); + + Client *backend = RouteToPid(routed->pid); if (!backend) - return reply(createStringError("no debug session available")); - backend->ResourcesRead(params, std::move(reply)); + return reply(createStringError(formatv("no instance {0}", routed->pid))); + + ReadResourceParams backend_params; + backend_params.uri = routed->local; + lldb::pid_t pid = routed->pid; + backend->ResourcesRead( + backend_params, [reply = std::move(reply), + pid](Expected<ReadResourceResult> result) mutable { + if (result) + for (TextResourceContents &content : result->contents) + content.uri = RewriteURIsToGlobal(content.uri, pid); + reply(std::move(result)); + }); } void Multiplexer::HandleDisconnect() { diff --git a/lldb/tools/lldb-mcp/Multiplexer.h b/lldb/tools/lldb-mcp/Multiplexer.h index c6a09684cfdb5..7e2df9f437074 100644 --- a/lldb/tools/lldb-mcp/Multiplexer.h +++ b/lldb/tools/lldb-mcp/Multiplexer.h @@ -12,21 +12,47 @@ #include "lldb/Protocol/MCP/Client.h" #include "lldb/Protocol/MCP/Protocol.h" #include "lldb/Protocol/MCP/Transport.h" +#include "lldb/lldb-types.h" #include "llvm/ADT/FunctionExtras.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include <memory> +#include <optional> +#include <string> #include <vector> namespace lldb_mcp { +/// Rewrites the backend-local debugger and resource URIs found in \p text to +/// their instance-qualified global form for instance \p pid, e.g. +/// `lldb-mcp://debugger/1` becomes `lldb-mcp://instance/{pid}/debugger/1`. +std::string RewriteURIsToGlobal(llvm::StringRef text, lldb::pid_t pid); + +/// An instance-qualified URI decomposed for routing. +struct RoutedURI { + lldb::pid_t pid; + /// The backend-local URI, with the `instance/{pid}` segment stripped. + std::string local; +}; + +/// Parses an instance-qualified global URI (`<scheme>://instance/{pid}/...`) +/// into its instance pid and backend-local URI. Returns std::nullopt if \p uri +/// is not instance-qualified. +std::optional<RoutedURI> ParseGlobalURI(llvm::StringRef uri); + /// The multiplexer is the single MCP server a client talks to. It presents a /// unified tool and resource surface and fans out to one or more backend LLDB -/// MCP servers, each reached through an mcp::Client. +/// MCP servers, each reached through an mcp::Client and identified by the pid +/// of its lldb instance. /// -/// Requests that can be answered locally (initialize, tools/list) are handled -/// directly. Requests that target a backend (tools/call, resources/list, -/// resources/read) are forwarded asynchronously, and the backend's answer is -/// relayed back to the client. +/// Requests answered locally (initialize, tools/list) are handled directly. +/// Requests that list across instances (sessions_list, resources/list) fan out +/// to every backend and aggregate. Requests that target one instance (command, +/// resources/read) are routed by an instance-qualified URI of the form +/// `lldb-mcp://instance/{pid}/debugger/{id}` (tools) or +/// `lldb://instance/{pid}/debugger/{id}/target/{idx}` (resources). Backends +/// only know their local `lldb-mcp://debugger/{id}` form, so URIs are rewritten +/// in both directions. class Multiplexer { public: template <typename T> using Reply = lldb_private::transport::Reply<T>; @@ -35,17 +61,27 @@ class Multiplexer { std::unique_ptr<lldb_protocol::mcp::MCPTransport> client_transport, lldb_protocol::mcp::LogCallback log_callback = {}); - /// Adds a backend. The client must already be running (see Client::Run). - /// Takes ownership. - void AddBackend(std::unique_ptr<lldb_protocol::mcp::Client> backend); + /// Adds a backend identified by the pid of its lldb instance. The client must + /// already be running (see Client::Run). Takes ownership. + void AddBackend(lldb::pid_t pid, + std::unique_ptr<lldb_protocol::mcp::Client> backend); /// Registers the client-facing handlers. Backends should be added first. llvm::Error Run(); + /// Fails any requests still in flight to a backend. Call after the client has + /// disconnected, before teardown. + void Shutdown(); + /// Sets a handler invoked when the client disconnects (EOF). void SetDisconnectHandler(llvm::unique_function<void()> handler); private: + struct Backend { + lldb::pid_t pid; + std::unique_ptr<lldb_protocol::mcp::Client> client; + }; + /// Locally-answered requests. /// @{ llvm::Expected<lldb_protocol::mcp::InitializeResult> @@ -53,10 +89,13 @@ class Multiplexer { llvm::Expected<lldb_protocol::mcp::ListToolsResult> HandleToolsList(); /// @} - /// Requests forwarded to a backend. + /// Requests routed to or aggregated across backends. /// @{ void HandleToolsCall(const lldb_protocol::mcp::CallToolParams ¶ms, Reply<lldb_protocol::mcp::CallToolResult> reply); + void HandleCommand(const lldb_protocol::mcp::CallToolParams ¶ms, + Reply<lldb_protocol::mcp::CallToolResult> reply); + void HandleSessionsList(Reply<lldb_protocol::mcp::CallToolResult> reply); void HandleResourcesList(Reply<lldb_protocol::mcp::ListResourcesResult> reply); void HandleResourcesRead(const lldb_protocol::mcp::ReadResourceParams ¶ms, @@ -68,15 +107,16 @@ class Multiplexer { void Log(llvm::StringRef message); - /// Returns the backend a request routes to, or null if none is available. - /// All requests currently route to the single backend. - lldb_protocol::mcp::Client *Route(); + /// Returns the backend for \p pid, or nullptr if there is none. + lldb_protocol::mcp::Client *RouteToPid(lldb::pid_t pid); + /// Returns the first backend, or nullptr if there are none. + lldb_protocol::mcp::Client *First(); std::unique_ptr<lldb_protocol::mcp::MCPTransport> m_client_transport; lldb_protocol::mcp::MCPBinderUP m_client_binder; lldb_protocol::mcp::LogCallback m_log_callback; - std::vector<std::unique_ptr<lldb_protocol::mcp::Client>> m_backends; + std::vector<Backend> m_backends; llvm::unique_function<void()> m_disconnect_handler; }; diff --git a/lldb/tools/lldb-mcp/lldb-mcp.cpp b/lldb/tools/lldb-mcp/lldb-mcp.cpp index 7a21cca743de8..a51935d1e3da9 100644 --- a/lldb/tools/lldb-mcp/lldb-mcp.cpp +++ b/lldb/tools/lldb-mcp/lldb-mcp.cpp @@ -93,7 +93,7 @@ llvm::Error launch() { return Host::LaunchProcess(info).takeError(); } -Expected<ServerInfo> loadOrStart( +Expected<std::vector<ServerInfo>> loadOrStart( // FIXME: This should become a CLI arg. lldb_private::Timeout<std::micro> timeout = std::chrono::seconds(30)) { using namespace std::chrono; @@ -117,12 +117,7 @@ Expected<ServerInfo> loadOrStart( continue; } - // FIXME: Support selecting / multiplexing a specific lldb instance. - if (servers->size() > 1) - return createStringError("too many MCP servers running, picking a " - "specific one is not yet implemented"); - - return servers->front(); + return std::move(*servers); } return createStringError("timed out waiting for MCP server to start"); @@ -206,9 +201,9 @@ int main(int argc, char *argv[]) { IOObjectSP output_sp = std::make_shared<NativeFile>( fileno(stdout), File::eOpenOptionWriteOnly, NativeFile::Unowned); - Expected<ServerInfo> server_info = loadOrStart(); - if (!server_info) - exitWithError(server_info.takeError()); + Expected<std::vector<ServerInfo>> servers = loadOrStart(); + if (!servers) + exitWithError(servers.takeError()); static MainLoop loop; sys::SetInterruptFunction([]() { @@ -216,24 +211,38 @@ int main(int argc, char *argv[]) { [](MainLoopBase &loop) { loop.RequestTermination(); }); }); - // Connect to the backend LLDB MCP server and drive it as an MCP client. - Expected<IOObjectSP> backend_io = connectToServer(*server_info); - if (!backend_io) - exitWithError(backend_io.takeError()); - - auto backend_transport = std::make_unique<lldb_protocol::mcp::Transport>( - loop, *backend_io, *backend_io, makeLogCallback()); - auto backend = std::make_unique<lldb_protocol::mcp::Client>( - std::move(backend_transport), makeLogCallback()); - if (llvm::Error error = backend->Run()) - exitWithError(std::move(error)); - // Present a unified MCP server to the client over stdio. auto client_transport = std::make_unique<lldb_protocol::mcp::Transport>( loop, input_sp, output_sp, makeLogCallback()); lldb_mcp::Multiplexer multiplexer(std::move(client_transport), makeLogCallback()); - multiplexer.AddBackend(std::move(backend)); + + // A stale registry entry left by a crashed instance fails to connect; skip it + // rather than aborting. + size_t connected = 0; + for (const ServerInfo &info : *servers) { + Expected<IOObjectSP> backend_io = connectToServer(info); + if (!backend_io) { + consumeError(backend_io.takeError()); + continue; + } + + auto backend_transport = std::make_unique<lldb_protocol::mcp::Transport>( + loop, *backend_io, *backend_io, makeLogCallback()); + auto backend = std::make_unique<lldb_protocol::mcp::Client>( + std::move(backend_transport), makeLogCallback()); + if (llvm::Error error = backend->Run()) { + consumeError(std::move(error)); + continue; + } + + multiplexer.AddBackend(info.pid, std::move(backend)); + ++connected; + } + + if (connected == 0) + exitWithError(createStringError("failed to connect to any MCP server")); + multiplexer.SetDisconnectHandler([]() { loop.RequestTermination(); }); if (llvm::Error error = multiplexer.Run()) exitWithError(std::move(error)); @@ -241,5 +250,8 @@ int main(int argc, char *argv[]) { if (llvm::Error error = loop.Run().takeError()) exitWithError(std::move(error)); + // The client is gone; fail any requests still in flight to a backend so their + // replies are satisfied rather than destroyed unanswered during teardown. + multiplexer.Shutdown(); return EXIT_SUCCESS; } diff --git a/lldb/unittests/Host/JSONTransportTest.cpp b/lldb/unittests/Host/JSONTransportTest.cpp index afa59b81ae315..c0adbd855cec8 100644 --- a/lldb/unittests/Host/JSONTransportTest.cpp +++ b/lldb/unittests/Host/JSONTransportTest.cpp @@ -831,6 +831,26 @@ TEST_F(TransportBinderTest, InBoundAsyncRequestsError) { Run(); } +TEST_F(TransportBinderTest, FailPendingRequests) { + OutgoingRequest<MyFnResult, MyFnParams> addFn = + binder->Bind<MyFnResult, MyFnParams>("add"); + bool replied = false; + std::string message; + addFn(MyFnParams{1, 2}, [&](Expected<MyFnResult> result) { + replied = true; + message = toString(result.takeError()); + }); + + // The request is now awaiting a response. FailPendingRequests fires its reply + // with an error, synchronously. + binder->FailPendingRequests("connection closed"); + EXPECT_TRUE(replied); + EXPECT_THAT(message, HasSubstr("connection closed")); + + // A second call is a no-op: nothing is left pending. + binder->FailPendingRequests("again"); +} + // Out-bound binding event handler. TEST_F(TransportBinderTest, OutBoundEvents) { OutgoingEvent<MyFnParams> emitEvent = binder->Bind<MyFnParams>("evt"); diff --git a/lldb/unittests/Protocol/MCPServerInfoTest.cpp b/lldb/unittests/Protocol/MCPServerInfoTest.cpp index d78c158e3ca7d..737220e45305f 100644 --- a/lldb/unittests/Protocol/MCPServerInfoTest.cpp +++ b/lldb/unittests/Protocol/MCPServerInfoTest.cpp @@ -24,10 +24,12 @@ using namespace lldb_protocol::mcp; TEST(MCPServerInfoTest, JSONRoundtrip) { ServerInfo info; info.connection_uri = "unix:///tmp/test.sock"; + info.pid = 4321; Expected<ServerInfo> deserialized = roundtripJSON(info); ASSERT_THAT_EXPECTED(deserialized, Succeeded()); EXPECT_EQ(deserialized->connection_uri, info.connection_uri); + EXPECT_EQ(deserialized->pid, info.pid); } TEST(MCPServerInfoTest, EmptyHandleRemoveIsNoOp) { diff --git a/lldb/unittests/tools/CMakeLists.txt b/lldb/unittests/tools/CMakeLists.txt index 42b0c25dd1fc9..e3e605fd663d4 100644 --- a/lldb/unittests/tools/CMakeLists.txt +++ b/lldb/unittests/tools/CMakeLists.txt @@ -1,3 +1,5 @@ +add_subdirectory(lldb-mcp) + if(LLDB_TOOL_LLDB_SERVER_BUILD) if (NOT LLVM_USE_SANITIZER MATCHES ".*Address.*") add_subdirectory(lldb-server) diff --git a/lldb/unittests/tools/lldb-mcp/CMakeLists.txt b/lldb/unittests/tools/lldb-mcp/CMakeLists.txt new file mode 100644 index 0000000000000..c2f6c2de3cbd7 --- /dev/null +++ b/lldb/unittests/tools/lldb-mcp/CMakeLists.txt @@ -0,0 +1,15 @@ +add_lldb_unittest(LLDBMCPTests + MultiplexerTest.cpp + ${LLDB_SOURCE_DIR}/tools/lldb-mcp/Multiplexer.cpp + + LINK_COMPONENTS + Support + LINK_LIBS + lldbHost + lldbProtocolMCP + lldbUtility + LLVMTestingSupport + ) + +target_include_directories(LLDBMCPTests PRIVATE + ${LLDB_SOURCE_DIR}/tools/lldb-mcp) diff --git a/lldb/unittests/tools/lldb-mcp/MultiplexerTest.cpp b/lldb/unittests/tools/lldb-mcp/MultiplexerTest.cpp new file mode 100644 index 0000000000000..b8c74a9c90d50 --- /dev/null +++ b/lldb/unittests/tools/lldb-mcp/MultiplexerTest.cpp @@ -0,0 +1,467 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "Multiplexer.h" +#include "TestingSupport/Host/JSONTransportTestUtilities.h" +#include "TestingSupport/SubsystemRAII.h" +#include "lldb/Host/FileSystem.h" +#include "lldb/Host/HostInfo.h" +#include "lldb/Host/MainLoop.h" +#include "lldb/Host/MainLoopBase.h" +#include "lldb/Host/Socket.h" +#include "lldb/Protocol/MCP/Client.h" +#include "lldb/Protocol/MCP/MCPError.h" +#include "lldb/Protocol/MCP/Protocol.h" +#include "lldb/Protocol/MCP/Resource.h" +#include "lldb/Protocol/MCP/Server.h" +#include "lldb/Protocol/MCP/Tool.h" +#include "lldb/Protocol/MCP/Transport.h" +#include "llvm/Support/Error.h" +#include "llvm/Support/JSON.h" +#include "llvm/Testing/Support/Error.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include <chrono> +#include <memory> +#include <optional> + +using namespace llvm; +using namespace lldb; +using namespace lldb_private; +using namespace lldb_protocol::mcp; +using namespace lldb_mcp; + +#ifndef _WIN32 + +namespace { + +/// A tool that echoes a label and the debugger argument it received, so tests +/// can verify both routing and request-path URI rewriting. +class FakeCommandTool : public Tool { +public: + FakeCommandTool(std::string label) + : Tool("command", "fake command"), m_label(std::move(label)) {} + + llvm::Expected<CallToolResult> Call(const ToolArguments &args) override { + std::string debugger; + if (std::holds_alternative<json::Value>(args)) + if (const json::Object *object = + std::get<json::Value>(args).getAsObject()) + if (std::optional<StringRef> value = object->getString("debugger")) + debugger = value->str(); + + CallToolResult result; + result.content.emplace_back( + TextContent{{m_label + " debugger=" + debugger}}); + return result; + } + +private: + std::string m_label; +}; + +/// A tool that mimics the backend debugger_list output. +class FakeDebuggerListTool : public Tool { +public: + FakeDebuggerListTool() : Tool("debugger_list", "fake debugger_list") {} + + llvm::Expected<CallToolResult> Call(const ToolArguments &) override { + CallToolResult result; + result.content.emplace_back(TextContent{{"- lldb-mcp://debugger/1\n"}}); + return result; + } +}; + +/// A tool that always fails, to exercise the cross-backend error paths. +class FakeFailingTool : public Tool { +public: + explicit FakeFailingTool(std::string name) + : Tool(std::move(name), "always fails") {} + + llvm::Expected<CallToolResult> Call(const ToolArguments &) override { + return llvm::createStringError("backend failure"); + } +}; + +class FakeResourceProvider : public ResourceProvider { +public: + using ResourceProvider::ResourceProvider; + + std::vector<Resource> GetResources() const override { + Resource resource; + resource.uri = "lldb://debugger/1"; + resource.name = "debugger_1"; + return {resource}; + } + + llvm::Expected<ReadResourceResult> + ReadResource(llvm::StringRef uri) const override { + if (uri != "lldb://debugger/1") + return llvm::make_error<UnsupportedURI>(uri.str()); + TextResourceContents contents; + contents.uri = "lldb://debugger/1"; + contents.text = "resource contents"; + ReadResourceResult result; + result.contents.push_back(contents); + return result; + } +}; + +class MultiplexerTest : public testing::Test { +public: + /// A peer handler that receives forwarded requests but never replies, so + /// they stay pending on the backend. + using SilentHandler = MockMessageHandler<ProtocolDescriptor>; + + SubsystemRAII<FileSystem, HostInfo, Socket> subsystems; + + MainLoop loop; + std::unique_ptr<Multiplexer> mux; + std::unique_ptr<Client> client; + std::vector<std::unique_ptr<Server>> servers; + std::vector<std::unique_ptr<TestTransport<ProtocolDescriptor>>> silent_peers; + std::vector<std::unique_ptr<SilentHandler>> silent_handlers; + + void SetUp() override { + auto pair = TestTransport<ProtocolDescriptor>::createConnectedPair(loop); + mux = std::make_unique<Multiplexer>(std::move(pair.second)); + client = std::make_unique<Client>(std::move(pair.first)); + EXPECT_THAT_ERROR(client->Run(), Succeeded()); + } + + /// Adds a fake backend with a command tool labelled \p label. + void AddBackend(lldb::pid_t pid, std::string label) { + auto pair = TestTransport<ProtocolDescriptor>::createConnectedPair(loop); + + auto server = std::make_unique<Server>("fake", "0.1.0"); + server->AddTool(std::make_unique<FakeCommandTool>(std::move(label))); + server->AddTool(std::make_unique<FakeDebuggerListTool>()); + server->AddResourceProvider(std::make_unique<FakeResourceProvider>()); + EXPECT_THAT_ERROR(server->Accept(std::move(pair.second)), Succeeded()); + + auto backend = std::make_unique<Client>(std::move(pair.first)); + EXPECT_THAT_ERROR(backend->Run(), Succeeded()); + mux->AddBackend(pid, std::move(backend)); + servers.push_back(std::move(server)); + } + + /// Adds a backend whose debugger_list tool always errors, to exercise the + /// cross-backend aggregation error path. + void AddFailingBackend(lldb::pid_t pid) { + auto pair = TestTransport<ProtocolDescriptor>::createConnectedPair(loop); + + auto server = std::make_unique<Server>("fake", "0.1.0"); + server->AddTool(std::make_unique<FakeFailingTool>("debugger_list")); + EXPECT_THAT_ERROR(server->Accept(std::move(pair.second)), Succeeded()); + + auto backend = std::make_unique<Client>(std::move(pair.first)); + EXPECT_THAT_ERROR(backend->Run(), Succeeded()); + mux->AddBackend(pid, std::move(backend)); + servers.push_back(std::move(server)); + } + + /// Adds a backend whose peer receives forwarded requests but never replies, + /// so a forwarded request is left pending. Returns the backend's transport so + /// a test can simulate a disconnect; the peer and its handler are kept alive + /// by the fixture. + TestTransport<ProtocolDescriptor> *AddSilentBackend(lldb::pid_t pid) { + auto pair = TestTransport<ProtocolDescriptor>::createConnectedPair(loop); + auto handler = std::make_unique<SilentHandler>(); + // The peer receives the forwarded request but never replies. + EXPECT_CALL(*handler, + Received(testing::A<const ProtocolDescriptor::Req &>())) + .Times(testing::AnyNumber()); + EXPECT_THAT_ERROR(pair.second->RegisterMessageHandler(*handler), + Succeeded()); + TestTransport<ProtocolDescriptor> *transport = pair.first.get(); + auto backend = std::make_unique<Client>(std::move(pair.first)); + EXPECT_THAT_ERROR(backend->Run(), Succeeded()); + mux->AddBackend(pid, std::move(backend)); + silent_peers.push_back(std::move(pair.second)); + silent_handlers.push_back(std::move(handler)); + return transport; + } + + /// Registers the multiplexer's handlers. Backends must be added first. + void Start() { EXPECT_THAT_ERROR(mux->Run(), Succeeded()); } + + /// Issues a request and runs the loop until its reply arrives. + template <typename Result> + llvm::Expected<Result> + Capture(llvm::unique_function<void(Client::Reply<Result>)> invoke) { + std::optional<llvm::Expected<Result>> captured; + invoke([&](llvm::Expected<Result> result) { + captured = std::move(result); + loop.RequestTermination(); + }); + bool registered = loop.AddCallback( + [](MainLoopBase &loop) { + loop.RequestTermination(); + FAIL() << "timed out waiting for reply"; + }, + std::chrono::seconds(5)); + EXPECT_TRUE(registered); + EXPECT_THAT_ERROR(loop.Run().takeError(), Succeeded()); + if (!captured) + return createStringError("no reply"); + return std::move(*captured); + } +}; + +std::string commandText(const CallToolResult &result) { + std::string text; + for (const TextContent &content : result.content) + text += content.text; + return text; +} + +} // namespace + +TEST(MultiplexerURITest, ParseGlobalURI) { + std::optional<RoutedURI> routed = + ParseGlobalURI("lldb-mcp://instance/42/debugger/3"); + ASSERT_TRUE(routed.has_value()); + EXPECT_EQ(routed->pid, 42u); + EXPECT_EQ(routed->local, "lldb-mcp://debugger/3"); + + routed = ParseGlobalURI("lldb://instance/7/debugger/1/target/0"); + ASSERT_TRUE(routed.has_value()); + EXPECT_EQ(routed->pid, 7u); + EXPECT_EQ(routed->local, "lldb://debugger/1/target/0"); + + // Not instance-qualified. + EXPECT_FALSE(ParseGlobalURI("lldb-mcp://debugger/1").has_value()); + EXPECT_FALSE(ParseGlobalURI("not a uri").has_value()); + // Non-numeric instance. + EXPECT_FALSE(ParseGlobalURI("lldb-mcp://instance/x/debugger/1").has_value()); +} + +TEST(MultiplexerURITest, RewriteURIsToGlobal) { + EXPECT_EQ(RewriteURIsToGlobal("- lldb-mcp://debugger/1\n", 42), + "- lldb-mcp://instance/42/debugger/1\n"); + EXPECT_EQ(RewriteURIsToGlobal("lldb://debugger/1/target/0", 7), + "lldb://instance/7/debugger/1/target/0"); + // Text without a known prefix is unchanged. + EXPECT_EQ(RewriteURIsToGlobal("no uris here", 1), "no uris here"); +} + +TEST_F(MultiplexerTest, Initialize) { + AddBackend(100, "A"); + Start(); + llvm::Expected<InitializeResult> result = + Capture<InitializeResult>([&](Client::Reply<InitializeResult> reply) { + client->Initialize(InitializeParams{"2024-11-05", {}, {"test", "0.1"}}, + std::move(reply)); + }); + ASSERT_THAT_EXPECTED(result, Succeeded()); + EXPECT_EQ(result->serverInfo.name, "lldb-mcp"); +} + +TEST_F(MultiplexerTest, ToolsListIsUnifiedSurface) { + AddBackend(100, "A"); + Start(); + llvm::Expected<ListToolsResult> result = + Capture<ListToolsResult>([&](Client::Reply<ListToolsResult> reply) { + client->ToolsList(std::move(reply)); + }); + ASSERT_THAT_EXPECTED(result, Succeeded()); + std::vector<std::string> names; + for (const ToolDefinition &tool : result->tools) + names.push_back(tool.name); + EXPECT_THAT(names, testing::UnorderedElementsAre("command", "sessions_list")); +} + +TEST_F(MultiplexerTest, SessionsListAggregatesAcrossBackends) { + AddBackend(100, "A"); + AddBackend(200, "B"); + Start(); + llvm::Expected<CallToolResult> result = + Capture<CallToolResult>([&](Client::Reply<CallToolResult> reply) { + client->ToolsCall(CallToolParams{"sessions_list", {}}, + std::move(reply)); + }); + ASSERT_THAT_EXPECTED(result, Succeeded()); + std::string text = commandText(*result); + EXPECT_THAT(text, testing::HasSubstr("lldb-mcp://instance/100/debugger/1")); + EXPECT_THAT(text, testing::HasSubstr("lldb-mcp://instance/200/debugger/1")); +} + +TEST_F(MultiplexerTest, CommandRoutesByInstance) { + AddBackend(100, "A"); + AddBackend(200, "B"); + Start(); + + auto run = [&](lldb::pid_t pid) { + return Capture<CallToolResult>([&](Client::Reply<CallToolResult> reply) { + json::Object args{ + {"command", "whatever"}, + {"debugger", + formatv("lldb-mcp://instance/{0}/debugger/1", pid).str()}}; + client->ToolsCall(CallToolParams{"command", json::Value(std::move(args))}, + std::move(reply)); + }); + }; + + llvm::Expected<CallToolResult> a = run(100); + ASSERT_THAT_EXPECTED(a, Succeeded()); + // Routed to backend A, and the debugger URI was rewritten to backend-local. + EXPECT_EQ(commandText(*a), "A debugger=lldb-mcp://debugger/1"); + + llvm::Expected<CallToolResult> b = run(200); + ASSERT_THAT_EXPECTED(b, Succeeded()); + EXPECT_EQ(commandText(*b), "B debugger=lldb-mcp://debugger/1"); +} + +TEST_F(MultiplexerTest, CommandUnknownInstanceErrors) { + AddBackend(100, "A"); + Start(); + llvm::Expected<CallToolResult> result = + Capture<CallToolResult>([&](Client::Reply<CallToolResult> reply) { + json::Object args{{"command", "x"}, + {"debugger", "lldb-mcp://instance/999/debugger/1"}}; + client->ToolsCall( + CallToolParams{"command", json::Value(std::move(args))}, + std::move(reply)); + }); + EXPECT_THAT_EXPECTED(result, Failed()); +} + +TEST_F(MultiplexerTest, ResourcesListAggregatesAndRewrites) { + AddBackend(100, "A"); + AddBackend(200, "B"); + Start(); + llvm::Expected<ListResourcesResult> result = Capture<ListResourcesResult>( + [&](Client::Reply<ListResourcesResult> reply) { + client->ResourcesList(std::move(reply)); + }); + ASSERT_THAT_EXPECTED(result, Succeeded()); + std::vector<std::string> uris; + for (const Resource &resource : result->resources) + uris.push_back(resource.uri); + EXPECT_THAT(uris, + testing::UnorderedElementsAre("lldb://instance/100/debugger/1", + "lldb://instance/200/debugger/1")); +} + +TEST_F(MultiplexerTest, ResourcesReadRoutesAndRewrites) { + AddBackend(100, "A"); + AddBackend(200, "B"); + Start(); + llvm::Expected<ReadResourceResult> result = + Capture<ReadResourceResult>([&](Client::Reply<ReadResourceResult> reply) { + client->ResourcesRead( + ReadResourceParams{"lldb://instance/200/debugger/1"}, + std::move(reply)); + }); + ASSERT_THAT_EXPECTED(result, Succeeded()); + ASSERT_THAT(result->contents, testing::SizeIs(1)); + // The response URI is rewritten back to the global form. + EXPECT_EQ(result->contents.front().uri, "lldb://instance/200/debugger/1"); + EXPECT_EQ(result->contents.front().text, "resource contents"); +} + +TEST_F(MultiplexerTest, SessionsListSurfacesBackendError) { + AddBackend(100, "A"); + AddFailingBackend(200); + Start(); + llvm::Expected<CallToolResult> result = + Capture<CallToolResult>([&](Client::Reply<CallToolResult> reply) { + client->ToolsCall(CallToolParams{"sessions_list", {}}, + std::move(reply)); + }); + // One backend erroring fails the whole aggregation rather than silently + // dropping that instance's sessions. + EXPECT_THAT_EXPECTED(result, Failed()); +} + +TEST_F(MultiplexerTest, ResourcesReadMalformedURIErrors) { + AddBackend(100, "A"); + Start(); + llvm::Expected<ReadResourceResult> result = + Capture<ReadResourceResult>([&](Client::Reply<ReadResourceResult> reply) { + client->ResourcesRead(ReadResourceParams{"lldb://debugger/1"}, + std::move(reply)); + }); + EXPECT_THAT_EXPECTED(result, Failed()); +} + +TEST_F(MultiplexerTest, ResourcesReadUnknownInstanceErrors) { + AddBackend(100, "A"); + Start(); + llvm::Expected<ReadResourceResult> result = + Capture<ReadResourceResult>([&](Client::Reply<ReadResourceResult> reply) { + client->ResourcesRead( + ReadResourceParams{"lldb://instance/999/debugger/1"}, + std::move(reply)); + }); + EXPECT_THAT_EXPECTED(result, Failed()); +} + +TEST_F(MultiplexerTest, ShutdownFailsPendingBackendRequest) { + AddSilentBackend(100); + Start(); + + std::optional<llvm::Expected<CallToolResult>> captured; + json::Object args{{"command", "x"}, + {"debugger", "lldb-mcp://instance/100/debugger/1"}}; + client->ToolsCall(CallToolParams{"command", json::Value(std::move(args))}, + [&](llvm::Expected<CallToolResult> result) { + captured = std::move(result); + loop.RequestTermination(); + }); + + // The request is forwarded to the silent backend and left pending; shutting + // down must fail it so the client gets an error instead of hanging. The + // delay lets the forward complete first (sends run as undelayed callbacks). + bool scheduled = loop.AddCallback([&](MainLoopBase &) { mux->Shutdown(); }, + std::chrono::milliseconds(100)); + EXPECT_TRUE(scheduled); + loop.AddCallback( + [](MainLoopBase &l) { + l.RequestTermination(); + FAIL() << "timed out waiting for reply"; + }, + std::chrono::seconds(5)); + + EXPECT_THAT_ERROR(loop.Run().takeError(), Succeeded()); + ASSERT_TRUE(captured.has_value()); + EXPECT_THAT_EXPECTED(*captured, Failed()); +} + +TEST_F(MultiplexerTest, BackendDisconnectFailsPendingRequest) { + TestTransport<ProtocolDescriptor> *backend = AddSilentBackend(100); + Start(); + + std::optional<llvm::Expected<CallToolResult>> captured; + json::Object args{{"command", "x"}, + {"debugger", "lldb-mcp://instance/100/debugger/1"}}; + client->ToolsCall(CallToolParams{"command", json::Value(std::move(args))}, + [&](llvm::Expected<CallToolResult> result) { + captured = std::move(result); + loop.RequestTermination(); + }); + + // Once the request is pending on the backend, a backend disconnect must fail + // it rather than leave the client hanging. + bool scheduled = + loop.AddCallback([&](MainLoopBase &) { backend->SimulateClosed(); }, + std::chrono::milliseconds(100)); + EXPECT_TRUE(scheduled); + loop.AddCallback( + [](MainLoopBase &l) { + l.RequestTermination(); + FAIL() << "timed out waiting for reply"; + }, + std::chrono::seconds(5)); + + EXPECT_THAT_ERROR(loop.Run().takeError(), Succeeded()); + ASSERT_TRUE(captured.has_value()); + EXPECT_THAT_EXPECTED(*captured, Failed()); +} + +#endif // ifndef _WIN32 >From 3a3dcecee19629cf889d46dce82e742f4388ef21 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <[email protected]> Date: Sat, 11 Jul 2026 16:22:17 -0700 Subject: [PATCH 2/2] Address John's feedback --- lldb/tools/lldb-mcp/Multiplexer.cpp | 4 ++++ lldb/tools/lldb-mcp/lldb-mcp.cpp | 12 ++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lldb/tools/lldb-mcp/Multiplexer.cpp b/lldb/tools/lldb-mcp/Multiplexer.cpp index bfd7140031ebc..0162c27b2d9ae 100644 --- a/lldb/tools/lldb-mcp/Multiplexer.cpp +++ b/lldb/tools/lldb-mcp/Multiplexer.cpp @@ -259,6 +259,8 @@ void Multiplexer::HandleSessionsList(Reply<CallToolResult> reply) { if (m_backends.empty()) return reply(makeTextResult("")); + // All backends share the multiplexer's MainLoop, so these reply callbacks run + // serially on one thread and the aggregation state below needs no locking. struct State { size_t remaining; // Keyed by pid so the aggregated output is deterministic. @@ -303,6 +305,8 @@ void Multiplexer::HandleResourcesList(Reply<ListResourcesResult> reply) { if (m_backends.empty()) return reply(ListResourcesResult{}); + // These reply callbacks run serially on the shared MainLoop thread, so this + // state needs no locking. struct State { size_t remaining; std::map<lldb::pid_t, std::vector<Resource>> resources; diff --git a/lldb/tools/lldb-mcp/lldb-mcp.cpp b/lldb/tools/lldb-mcp/lldb-mcp.cpp index a51935d1e3da9..17f93af780ea1 100644 --- a/lldb/tools/lldb-mcp/lldb-mcp.cpp +++ b/lldb/tools/lldb-mcp/lldb-mcp.cpp @@ -219,11 +219,16 @@ int main(int argc, char *argv[]) { // A stale registry entry left by a crashed instance fails to connect; skip it // rather than aborting. + LogCallback log = makeLogCallback(); size_t connected = 0; for (const ServerInfo &info : *servers) { Expected<IOObjectSP> backend_io = connectToServer(info); if (!backend_io) { - consumeError(backend_io.takeError()); + std::string reason = toString(backend_io.takeError()); + if (log) + log(formatv("skipping unreachable MCP server (pid {0}): {1}", info.pid, + reason) + .str()); continue; } @@ -232,7 +237,10 @@ int main(int argc, char *argv[]) { auto backend = std::make_unique<lldb_protocol::mcp::Client>( std::move(backend_transport), makeLogCallback()); if (llvm::Error error = backend->Run()) { - consumeError(std::move(error)); + std::string reason = toString(std::move(error)); + if (log) + log(formatv("skipping MCP server (pid {0}): {1}", info.pid, reason) + .str()); continue; } _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
