From 57b5baed7f99c9d1d7139f92e7ba28eba64bbf9e Mon Sep 17 00:00:00 2001 From: YueGuobin Date: Tue, 25 Aug 2026 21:32:09 +0800 Subject: [PATCH] fix: keep submission order and unify status in MCP batch handlers Batch node/link creation collected results with as_completed, so the response order followed completion rather than the submitted array and callers could not correlate entries. Collect in submission order via pool.map, and report batch deletes as status=success like every other batch action (the message still says what was deleted). --- .../gns3_copilot/gns3_client/api_handlers.py | 22 ++++----- tests/agent/mcp/test_handlers.py | 47 +++++++++++++++++++ 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/gns3server/agent/gns3_copilot/gns3_client/api_handlers.py b/gns3server/agent/gns3_copilot/gns3_client/api_handlers.py index 9c31d07d0..8c5a794e6 100644 --- a/gns3server/agent/gns3_copilot/gns3_client/api_handlers.py +++ b/gns3server/agent/gns3_copilot/gns3_client/api_handlers.py @@ -48,7 +48,7 @@ pulls the request-scoped user JWT from the context variables. """ from typing import Any -from concurrent.futures import ThreadPoolExecutor, as_completed +from concurrent.futures import ThreadPoolExecutor import hashlib import logging @@ -315,7 +315,6 @@ def create_node_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dic if not isinstance(nodes, list) or not nodes: return {"error": "nodes must be a non-empty array"} default_tid = params.get("template_id") - results = [] conn = _get_connector(gns3_ctx) def _create_one(node_data): tid = node_data.get("template_id", default_tid) @@ -336,10 +335,9 @@ def create_node_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dic except Exception as e: return {"template_id": tid, "status": "error", "error": str(e)} with ThreadPoolExecutor(max_workers=min(len(nodes), BATCH_MAX_WORKERS)) as pool: - futures = {pool.submit(_create_one, n): n for n in nodes} - for future in as_completed(futures): - results.append(future.result()) - return results + # pool.map keeps the submission order, so callers can correlate + # results with the nodes they sent regardless of completion order + return list(pool.map(_create_one, nodes)) # Single mode template_id = params.get("template_id") @@ -371,7 +369,7 @@ def delete_node_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dic def _del(nid): try: conn.http_call("delete", f"{conn.base_url}/projects/{project_id}/nodes/{nid}") - return {"node_id": nid, "status": "deleted"} + return {"node_id": nid, "status": "success", "message": f"Node {nid} deleted"} except Exception as e: return {"node_id": nid, "status": "error", "error": str(e)} with ThreadPoolExecutor(max_workers=min(len(node_ids), BATCH_MAX_WORKERS)) as pool: @@ -656,7 +654,6 @@ def create_link_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dic if links is not None: if not isinstance(links, list) or not links: return {"error": "links must be a non-empty array"} - results = [] conn = _get_connector(gns3_ctx) def _create_one(link_data): raw_nodes = link_data.get("nodes") @@ -676,10 +673,9 @@ def create_link_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dic except Exception as e: return {"status": "error", "error": str(e)} with ThreadPoolExecutor(max_workers=min(len(links), BATCH_MAX_WORKERS)) as pool: - futures = {pool.submit(_create_one, link): link for link in links} - for future in as_completed(futures): - results.append(future.result()) - return results + # pool.map keeps the submission order, so callers can correlate + # results with the links they sent regardless of completion order + return list(pool.map(_create_one, links)) # Single mode nodes = params.get("nodes") @@ -710,7 +706,7 @@ def delete_link_handler(params: dict[str, Any], gns3_ctx: dict[str, Any]) -> dic def _del(lid): try: conn.http_call("delete", f"{conn.base_url}/projects/{project_id}/links/{lid}") - return {"link_id": lid, "status": "deleted"} + return {"link_id": lid, "status": "success", "message": f"Link {lid} deleted"} except Exception as e: return {"link_id": lid, "status": "error", "error": str(e)} with ThreadPoolExecutor(max_workers=min(len(link_ids), BATCH_MAX_WORKERS)) as pool: diff --git a/tests/agent/mcp/test_handlers.py b/tests/agent/mcp/test_handlers.py index 0a85e0256..68ca430f9 100644 --- a/tests/agent/mcp/test_handlers.py +++ b/tests/agent/mcp/test_handlers.py @@ -182,6 +182,25 @@ class TestNode: }, ctx) assert result[0]["status"] == "success" + def test_create_batch_preserves_submission_order(self, ctx): + import time + from gns3server.agent.gns3_copilot.gns3_client.api_handlers import create_node_handler + with patch(f"{AH}._get_connector") as m: + conn = _mock_conn() + def _http_call(method, url, json_data=None, **kwargs): + # first submissions sleep longest so completion order is reversed + time.sleep({"slow": 0.25, "mid": 0.1}.get(json_data.get("name"), 0.0)) + resp = MagicMock() + resp.json.return_value = {"node_id": "n1", "name": json_data["name"]} + return resp + conn.http_call.side_effect = _http_call + m.return_value = conn + result = create_node_handler({ + "project_id": "p1", "template_id": "t1", + "nodes": [{"name": "slow"}, {"name": "mid"}, {"name": "fast"}], + }, ctx) + assert [r["node"]["name"] for r in result] == ["slow", "mid", "fast"] + def test_create_missing_project_id(self, ctx): from gns3server.agent.gns3_copilot.gns3_client.api_handlers import create_node_handler assert create_node_handler({}, ctx) == {"error": "project_id is required"} @@ -192,6 +211,8 @@ class TestNode: m.return_value = _mock_conn({}) result = delete_node_handler({"project_id": "p1", "node_ids": ["n1", "n2"]}, ctx) assert len(result) == 2 + # same status vocabulary as create/start/stop batches + assert all(r["status"] == "success" for r in result) def test_start_batch(self, ctx): from gns3server.agent.gns3_copilot.gns3_client.api_handlers import start_node_handler @@ -291,6 +312,32 @@ class TestLink: m.return_value = _mock_conn({}) result = delete_link_handler({"project_id": "p1", "link_ids": ["l1", "l2"]}, ctx) assert len(result) == 2 + # same status vocabulary as create batches + assert all(r["status"] == "success" for r in result) + + def test_create_batch_preserves_submission_order(self, ctx): + import time + from gns3server.agent.gns3_copilot.gns3_client.api_handlers import create_link_handler + with patch(f"{AH}._get_connector") as m: + conn = _mock_conn() + def _http_call(method, url, json_data=None, **kwargs): + # first submission sleeps longest so completion order is reversed + first_node = json_data["nodes"][0]["node_id"] + time.sleep(0.25 if first_node == "n1" else 0.0) + resp = MagicMock() + resp.json.return_value = {"link_id": f"link-{first_node}"} + return resp + conn.http_call.side_effect = _http_call + m.return_value = conn + result = create_link_handler({ + "project_id": "p1", + "links": [ + {"nodes": ["n1", 0, 0, "n2", 0, 0]}, + {"nodes": ["n3", 0, 0, "n4", 0, 0]}, + ], + "fields": ["link_id"], + }, ctx) + assert [r["link"]["link_id"] for r in result] == ["link-n1", "link-n3"] def test_update(self, ctx): from gns3server.agent.gns3_copilot.gns3_client.api_handlers import update_link_handler