From 8adcf93b6a3e188e0f853a8fbeb99bb084c886fe Mon Sep 17 00:00:00 2001 From: Marcus van Elst Date: Mon, 18 May 2026 11:38:36 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20v0.3.3=20=E2=80=94=20delete=5Fcontainer?= =?UTF-8?q?=20params=20(error=20114)=20+=20delete=5Fproject=20orphan=20gua?= =?UTF-8?q?rd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 1 — delete_container (DSM error 114): SYNO.Docker.Container/delete requires three parameters: name (JSON-encoded), force=false, and preserve_profile=false. Previously only a bare `name` string was sent, causing DSM to reject the call with error 114. Added the two missing fields and JSON-encode name to match the DSM convention. The connector-side running-container guard is unchanged; force stays hard-coded to false. Bug 2 — delete_project orphan containers: Production test revealed that DSM does NOT reject Project/delete on a running project — it silently removes the registration and leaves the containers running without any project context. The previous implementation tried to handle this via a caught SynologyError that never actually fires. Fix: check the project status from _find_project connector-side before issuing any DSM call; if RUNNING, return an error pointing at stop_project. The delete request is never sent for a running project. The corresponding unit test (test_delete_project_running_returns_stop_hint) was a false positive — it mocked a DSM rejection that real DSM never produces. Replaced with test_delete_project_running_blocked_connector_side which asserts that client.request("delete") is never called when the project is RUNNING. Co-Authored-By: Claude Opus 4.7 --- CHANGELOG.md | 21 ++++++++++++++- pyproject.toml | 2 +- .../modules/containers.py | 7 ++++- .../modules/projects.py | 17 ++++++------ tests/test_modules/test_containers.py | 26 +++++++++++++++++++ tests/test_modules/test_projects.py | 21 +++++++-------- uv.lock | 2 +- 7 files changed, 72 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1fea9e..b3c55a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,26 @@ All notable changes to this project will be documented in this file. -## [0.3.2] - 2026-05-18 +## [0.3.3] - 2026-05-18 + +### Fixed + +- `delete_container`: `SYNO.Docker.Container/delete` requires three + parameters — `name` (JSON-encoded), `force=false`, and + `preserve_profile=false`. Previously only `name` was sent (without + JSON-encoding), causing DSM to reject the call with error 114. +- `delete_project`: DSM does **not** reject `Project/delete` on a + running project — it silently removes the registration and leaves + the containers running as orphans. The connector now blocks the call + itself when the project status is `RUNNING`, before issuing any DSM + request, and tells the user to `stop_project` first. The previous + implementation relied on a DSM-level rejection that never occurs in + practice; the corresponding unit test was a false positive (mocked + an error that real DSM never returns) and has been replaced with a + test that asserts `Project/delete` is never called for a `RUNNING` + project. + + ### Added diff --git a/pyproject.toml b/pyproject.toml index 2fb91a8..3c3a9f5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "mcp-synology-container" -version = "0.3.2" +version = "0.3.3" description = "MCP server for Synology Container Manager" requires-python = ">=3.12" dependencies = [ diff --git a/src/mcp_synology_container/modules/containers.py b/src/mcp_synology_container/modules/containers.py index e30e382..bcb9fa3 100644 --- a/src/mcp_synology_container/modules/containers.py +++ b/src/mcp_synology_container/modules/containers.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import logging import re from typing import TYPE_CHECKING, Any @@ -319,7 +320,11 @@ def register_containers(mcp: FastMCP, config: AppConfig, client: DsmClient) -> N await client.request( "SYNO.Docker.Container", "delete", - params={"name": resolved_name}, + params={ + "name": json.dumps(resolved_name), + "force": "false", + "preserve_profile": "false", + }, ) return f"Deleted container '{display_name}'." except Exception as e: diff --git a/src/mcp_synology_container/modules/projects.py b/src/mcp_synology_container/modules/projects.py index e1549bd..1334ca9 100644 --- a/src/mcp_synology_container/modules/projects.py +++ b/src/mcp_synology_container/modules/projects.py @@ -403,6 +403,15 @@ def register_projects(mcp: FastMCP, config: AppConfig, client: DsmClient) -> Non f"Call this tool again with confirmed=True to proceed." ) + # Guard: DSM does NOT reject a delete call on a running project — it + # removes the registration silently and leaves the containers running + # as orphans. Reject here so we never put the NAS into that state. + if status == "RUNNING": + return ( + f"Error: project '{project_name}' is RUNNING. " + f"Stop it first with stop_project, then delete_project." + ) + try: await client.request( "SYNO.Docker.Project", @@ -411,14 +420,6 @@ def register_projects(mcp: FastMCP, config: AppConfig, client: DsmClient) -> Non params={"id": json.dumps(project_id)}, ) except SynologyError as e: - # DSM refuses to delete a running project. We deliberately do NOT - # auto-stop — that would be too destructive for a delete tool — - # but we tell the user how to proceed. - if status == "RUNNING": - return ( - f"Cannot delete project '{project_name}' while it is running ({e}).\n" - f"Stop the project first with stop_project." - ) return f"Error deleting project '{project_name}': {e}" return ( diff --git a/tests/test_modules/test_containers.py b/tests/test_modules/test_containers.py index 81b755d..62c7dd4 100644 --- a/tests/test_modules/test_containers.py +++ b/tests/test_modules/test_containers.py @@ -317,6 +317,32 @@ async def test_delete_container_stopped_confirmed(): assert "myapp_web" in result +@pytest.mark.asyncio +async def test_delete_container_sends_required_params(): + """SYNO.Docker.Container/delete must include name (json-encoded), force=false, + and preserve_profile=false — without them DSM returns error 114.""" + from mcp_synology_container.modules.containers import register_containers + + client = AsyncMock() + client.request.return_value = { + "details": {"State": {"Running": False, "Status": "exited"}}, + "profile": {"image": "nginx:latest"}, + } + + mcp, tools = make_mock_mcp() + register_containers(mcp, make_config(), client) + + await tools["delete_container"]("myapp_web", confirmed=True) + + # Find the delete call (second call — first is Container/get) + delete_calls = [c for c in client.request.call_args_list if c.args[1] == "delete"] + assert len(delete_calls) == 1 + params = delete_calls[0].kwargs.get("params") or {} + assert params["name"] == '"myapp_web"' + assert params["force"] == "false" + assert params["preserve_profile"] == "false" + + @pytest.mark.asyncio async def test_container_stats_cpu_calculation(): """CPU% is computed via the standard Docker formula.""" diff --git a/tests/test_modules/test_projects.py b/tests/test_modules/test_projects.py index 664649e..93b8592 100644 --- a/tests/test_modules/test_projects.py +++ b/tests/test_modules/test_projects.py @@ -898,20 +898,17 @@ async def test_delete_project_happy_path(): @pytest.mark.asyncio -async def test_delete_project_running_returns_stop_hint(): - """DSM refusing to delete a running project produces a clean 'stop_project' hint - rather than a raw error.""" - from mcp_synology_container.dsm_client import SynologyError - - client, calls = make_delete_project_client( - project=SAMPLE_PROJECT_RUNNING, - delete_raises=SynologyError("Project is running", code=2103), - ) +async def test_delete_project_running_blocked_connector_side(): + """Live test showed that DSM does NOT reject Project/delete on a running project — + it silently orphans the containers. The connector must therefore block the call + itself when the project is RUNNING, without ever calling client.request(delete).""" + client, calls = make_delete_project_client(project=SAMPLE_PROJECT_RUNNING) tools = make_projects_tools(client) result = await tools["delete_project"]("myapp", confirmed=True) + assert "RUNNING" in result assert "stop_project" in result - assert "running" in result.lower() - # No "deleted" success line - assert "deleted (registration removed)" not in result + # The delete endpoint must NOT have been called — no orphaned containers. + delete_calls = [m for _, m, _ in calls if m == "delete"] + assert delete_calls == [] diff --git a/uv.lock b/uv.lock index cf7b5ee..bf9e6e2 100644 --- a/uv.lock +++ b/uv.lock @@ -362,7 +362,7 @@ wheels = [ [[package]] name = "mcp-synology-container" -version = "0.3.2" +version = "0.3.3" source = { editable = "." } dependencies = [ { name = "click" },