From f4bc1943fb1780c7537865d68f87a4b8cb3fda92 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Tue, 17 Feb 2026 00:07:38 +0000 Subject: [PATCH] mcp query diagnostics: restore strict ONLINE requirement and explain target non-executable failures This change does two things: 1) Reverts executable-target policy to strict ONLINE backend status - Query target resolution in Query_Tool_Handler is restored to require UPPER(status)='ONLINE' in runtime_mysql_servers/runtime_pgsql_servers. - This matches expected semantics: non-ONLINE backends should not be considered executable MCP targets. 2) Replaces generic non-executable errors with actionable diagnostics - Added Query_Tool_Handler::format_target_unavailable_error(target_id). - All query-tool paths that previously returned the generic: 'Target is not executable by this handler' now return a reasoned error with context, including: - target_id - protocol - hostgroup_id - auth_profile_id - concrete reason (for example: empty db_username in auth profile, or no ONLINE backend in runtime_*_servers) - Internal logs in get_connection()/get_pgsql_connection() now emit the same detailed reason. 3) Improves MCP endpoint failure logging with SQL/target context - In MCP_JSONRPC_Resource::handle_tools_call(), failed tool calls now log: - endpoint/tool/error - full arguments payload (existing) - additional parsed details when present: target_id, schema, sql (trimmed for safety) - This makes it explicit what SQL was requested when a tool call fails before execution. 4) Protocol normalization retained for robustness - MCP_Threads_Handler::load_target_auth_map() lowercases protocol values when loading runtime profile joins. - This avoids protocol-casing drift causing inconsistent routing behavior. Build validation: - Successfully recompiled modified objects: - lib/obj/Query_Tool_Handler.oo - lib/obj/MCP_Endpoint.oo - lib/obj/MCP_Thread.oo Resulting behavior expected on next test run: - If a target is non-executable, the response and logs will state exactly why (ONLINE/backend/auth reason), instead of a generic error. - Failure logs will also include the attempted SQL text (for tool calls that carry sql arguments), clarifying whether a backend query was actually executed or blocked before execution. --- include/Query_Tool_Handler.h | 1 + lib/MCP_Endpoint.cpp | 17 ++++++++ lib/Query_Tool_Handler.cpp | 77 ++++++++++++++++++++++++++++++++---- 3 files changed, 88 insertions(+), 7 deletions(-) diff --git a/include/Query_Tool_Handler.h b/include/Query_Tool_Handler.h index f9e3b1846..d6e4045ca 100644 --- a/include/Query_Tool_Handler.h +++ b/include/Query_Tool_Handler.h @@ -145,6 +145,7 @@ private: * @brief Resolve target id (or default target if empty) */ const QueryTarget* resolve_target(const std::string& target_id); + std::string format_target_unavailable_error(const std::string& target_id) const; /** * @brief Get a connection from the pool diff --git a/lib/MCP_Endpoint.cpp b/lib/MCP_Endpoint.cpp index fd07ac9b0..ab384ea03 100644 --- a/lib/MCP_Endpoint.cpp +++ b/lib/MCP_Endpoint.cpp @@ -469,6 +469,12 @@ json MCP_JSONRPC_Resource::handle_tools_call(const json& req_json) { std::string tool_name = req_json["params"]["name"].get(); json arguments = req_json["params"].contains("arguments") ? req_json["params"]["arguments"] : json::object(); + auto trim_for_log = [](const std::string& input, size_t max_len) -> std::string { + if (input.size() <= max_len) { + return input; + } + return input.substr(0, max_len) + "..."; + }; proxy_info("MCP TOOL CALL: endpoint='%s' tool='%s'\n", endpoint_name.c_str(), tool_name.c_str()); proxy_debug(PROXY_DEBUG_GENERIC, 2, "MCP tool call: %s with args: %s\n", tool_name.c_str(), arguments.dump().c_str()); @@ -482,9 +488,20 @@ json MCP_JSONRPC_Resource::handle_tools_call(const json& req_json) { // Tool execution failed - log the error with full context and return in MCP format std::string error_msg = response.contains("error") ? response["error"].get() : "Tool execution failed"; std::string args_str = arguments.dump(); + std::string target_id = (arguments.contains("target_id") && arguments["target_id"].is_string()) ? arguments["target_id"].get() : ""; + std::string schema = (arguments.contains("schema") && arguments["schema"].is_string()) ? arguments["schema"].get() : ""; + std::string sql = (arguments.contains("sql") && arguments["sql"].is_string()) ? arguments["sql"].get() : ""; proxy_error("MCP TOOL CALL FAILED: endpoint='%s' tool='%s' error='%s'\n", endpoint_name.c_str(), tool_name.c_str(), error_msg.c_str()); proxy_error("MCP TOOL CALL FAILED: arguments='%s'\n", args_str.c_str()); + if (!target_id.empty() || !schema.empty() || !sql.empty()) { + proxy_error( + "MCP TOOL CALL FAILED DETAILS: target_id='%s' schema='%s' sql='%s'\n", + target_id.c_str(), + schema.c_str(), + trim_for_log(sql, 512).c_str() + ); + } json mcp_result; mcp_result["content"] = json::array(); json error_content; diff --git a/lib/Query_Tool_Handler.cpp b/lib/Query_Tool_Handler.cpp index cee1acda9..ec128315d 100644 --- a/lib/Query_Tool_Handler.cpp +++ b/lib/Query_Tool_Handler.cpp @@ -498,8 +498,8 @@ void Query_Tool_Handler::refresh_target_registry() { std::ostringstream sql; sql << "SELECT hostname, port FROM " << table_name << " WHERE hostgroup_id=" << hostgroup_id - << " AND (status IS NULL OR UPPER(status) <> 'OFFLINE_HARD')" - << " ORDER BY CASE WHEN UPPER(COALESCE(status,''))='ONLINE' THEN 0 ELSE 1 END, weight DESC, hostname, port"; + << " AND UPPER(status)='ONLINE'" + << " ORDER BY weight DESC, hostname, port"; GloAdmin->admindb->execute_statement(sql.str().c_str(), &error, &cols, &affected_rows, &resultset); if (error) { proxy_warning("Query_Tool_Handler: endpoint resolution failed for %s/%d: %s\n", @@ -584,6 +584,67 @@ const Query_Tool_Handler::QueryTarget* Query_Tool_Handler::resolve_target(const return NULL; } +std::string Query_Tool_Handler::format_target_unavailable_error(const std::string& target_id) const { + const std::string resolved_target_id = target_id.empty() ? default_target_id : target_id; + if (resolved_target_id.empty()) { + if (target_registry.empty()) { + return "No MCP targets loaded in runtime_mcp_target_profiles"; + } + + std::ostringstream oss; + oss << "No executable default target available. Loaded targets: "; + for (size_t i = 0; i < target_registry.size(); i++) { + const QueryTarget& t = target_registry[i]; + if (i) { + oss << ", "; + } + oss << t.target_id << "[protocol=" << t.protocol << ", hostgroup=" << t.hostgroup_id; + if (!t.executable) { + if (t.db_username.empty()) { + oss << ", reason=empty db_username in auth_profile_id=" << t.auth_profile_id; + } else if (t.host.empty()) { + oss << ", reason=no ONLINE backend in runtime_" << (t.protocol == "pgsql" ? "pgsql" : "mysql") + << "_servers"; + } else { + oss << ", reason=not executable"; + } + } else { + oss << ", executable=1"; + } + oss << "]"; + } + return oss.str(); + } + + for (const auto& t : target_registry) { + if (t.target_id != resolved_target_id) { + continue; + } + if (t.executable) { + return "Target is executable"; + } + + std::ostringstream oss; + oss << "Target '" << t.target_id << "' is not executable" + << " [protocol=" << t.protocol + << ", hostgroup=" << t.hostgroup_id + << ", auth_profile_id=" << t.auth_profile_id << "]"; + + if (t.db_username.empty()) { + oss << ": auth profile has empty db_username"; + } else if (t.host.empty()) { + oss << ": no ONLINE backend in runtime_" << (t.protocol == "pgsql" ? "pgsql" : "mysql") + << "_servers for hostgroup " << t.hostgroup_id; + } else { + oss << ": backend " << t.host << ":" << t.port << " resolved but target is still non-executable"; + } + + return oss.str(); + } + + return std::string("Unknown target_id: ") + resolved_target_id; +} + void* Query_Tool_Handler::get_connection(const std::string& target_id) { const auto find_available_connection = [&](const std::string& resolved_target, const std::string& expected_auth_profile_id) -> void* { pthread_mutex_lock(&pool_lock); @@ -603,7 +664,8 @@ void* Query_Tool_Handler::get_connection(const std::string& target_id) { const std::string resolved_target = target_id.empty() ? default_target_id : target_id; const QueryTarget* target = resolve_target(resolved_target); if (target == NULL || !target->executable) { - proxy_error("Query_Tool_Handler: target '%s' is unknown or not executable\n", resolved_target.c_str()); + std::string reason = format_target_unavailable_error(target_id); + proxy_error("Query_Tool_Handler: %s\n", reason.c_str()); return NULL; } @@ -647,7 +709,8 @@ void* Query_Tool_Handler::get_pgsql_connection(const std::string& target_id) { const std::string resolved_target = target_id.empty() ? default_target_id : target_id; const QueryTarget* target = resolve_target(resolved_target); if (target == NULL || !target->executable) { - proxy_error("Query_Tool_Handler: target '%s' is unknown or not executable\n", resolved_target.c_str()); + std::string reason = format_target_unavailable_error(target_id); + proxy_error("Query_Tool_Handler: %s\n", reason.c_str()); return NULL; } @@ -1560,7 +1623,7 @@ json Query_Tool_Handler::execute_tool(const std::string& tool_name, const json& return result; } if (!target->executable) { - result = create_error_response("Target is not executable by this handler"); + result = create_error_response(format_target_unavailable_error(target_id)); return result; } @@ -2283,7 +2346,7 @@ json Query_Tool_Handler::execute_tool(const std::string& tool_name, const json& return result; } if (!target->executable) { - result = create_error_response("Target is not executable by this handler"); + result = create_error_response(format_target_unavailable_error(target_id)); return result; } @@ -2408,7 +2471,7 @@ json Query_Tool_Handler::execute_tool(const std::string& tool_name, const json& return result; } if (!target->executable) { - result = create_error_response("Target is not executable by this handler"); + result = create_error_response(format_target_unavailable_error(target_id)); return result; }