feat: v0.3.0 — review welle 2 (M-4, M-5, M-6)
Three resilience and honesty fixes from the v0.2.8 review. Minor version bump because redeploy_project and system_prune return different strings. M-4: trigger_build_stream now converts every non-ReadTimeout httpx.HTTPError (ConnectError, ConnectTimeout, WriteError, RemoteProtocolError, ...) into a SynologyError with a clear message. Previously only ReadTimeout was handled; everything else propagated as a raw httpx exception. redeploy_project now tracks whether stop was actually issued and, when build_stream fails after a successful stop, tells the user the project is in STOPPED state and recommends start_project / retry rather than the misleading "use stop + start separately" workaround. M-5: _wait_for_project_running exits early on BUILD_FAILED / ERROR (new _TERMINAL_FAILURE_STATUSES frozenset). DSM signals these statuses within seconds of a failed image pull; the old polling loop kept waiting up to 5 minutes for RUNNING. redeploy_project now surfaces the terminal status with a BUILD_FAILED-specific hint to update_image_tag. M-6: system_prune preview now enumerates user-created networks that have no containers attached (excluding the three built-in networks bridge/host/none, which Docker never prunes). Previously the preview noted "Unused networks: (not counted)" even though SYNO.Docker.Utils/prune does delete them — users could lose networks they had not been warned about. Tests: - 2 new dsm_client tests: ConnectError and RemoteProtocolError both raise SynologyError, not raw httpx exceptions. - 2 new project tests: recovery hint after stop+build_stream failure (RUNNING case); old workaround retained for the STOPPED case where no stop was issued. - 3 new polling tests: BUILD_FAILED and ERROR each trigger early exit; redeploy_project surfaces BUILD_FAILED with update_image_tag hint. - 2 new system_prune preview tests: counts unused networks correctly, excludes built-ins; network-fetch failure is non-fatal. 245 tests pass. ruff check + ruff format clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -563,6 +563,46 @@ async def test_build_stream_read_timeout_swallowed() -> None:
|
||||
assert result is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_build_stream_connect_error_raises_synology_error() -> None:
|
||||
"""M-4: ConnectError (DSM unreachable) must surface as SynologyError, not raw httpx."""
|
||||
async with DsmClient(base_url="https://nas.local:443") as client:
|
||||
mark_initialized(client)
|
||||
client._http = AsyncMock()
|
||||
|
||||
ctx = MagicMock()
|
||||
ctx.__aenter__ = AsyncMock(side_effect=httpx.ConnectError("nas offline"))
|
||||
ctx.__aexit__ = AsyncMock(return_value=None)
|
||||
client._http.stream = MagicMock(return_value=ctx)
|
||||
|
||||
with pytest.raises(SynologyError) as exc_info:
|
||||
await client.trigger_build_stream("proj-1")
|
||||
|
||||
msg = str(exc_info.value)
|
||||
assert "transport error" in msg
|
||||
assert "ConnectError" in msg
|
||||
# Cause is suppressed via `from None` to keep error message clean.
|
||||
assert exc_info.value.__cause__ is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_build_stream_remote_protocol_error_raises_synology_error() -> None:
|
||||
"""M-4: RemoteProtocolError (broken response framing) is also converted."""
|
||||
async with DsmClient(base_url="https://nas.local:443") as client:
|
||||
mark_initialized(client)
|
||||
client._http = AsyncMock()
|
||||
|
||||
ctx = MagicMock()
|
||||
ctx.__aenter__ = AsyncMock(side_effect=httpx.RemoteProtocolError("server disconnected"))
|
||||
ctx.__aexit__ = AsyncMock(return_value=None)
|
||||
client._http.stream = MagicMock(return_value=ctx)
|
||||
|
||||
with pytest.raises(SynologyError) as exc_info:
|
||||
await client.trigger_build_stream("proj-1")
|
||||
|
||||
assert "RemoteProtocolError" in str(exc_info.value)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_build_stream_http_500_scrubs_sid() -> None:
|
||||
async with DsmClient(base_url="https://nas.local:443") as client:
|
||||
|
||||
@@ -381,3 +381,139 @@ async def test_redeploy_unknown_status_returns_error():
|
||||
|
||||
assert "UPDATING" in result
|
||||
assert "Workaround" in result or "stop_project" in result
|
||||
|
||||
|
||||
# ──────────────────────────────────────────────────────────────────────
|
||||
# M-4: clear recovery hint when build_stream fails after stop succeeded
|
||||
# ──────────────────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_redeploy_build_stream_transport_error_shows_stopped_recovery_hint():
|
||||
"""M-4: build_stream transport error after RUNNING-stop must tell the user the
|
||||
project is now STOPPED and recommend start_project / retry."""
|
||||
from mcp_synology_container.dsm_client import SynologyError
|
||||
|
||||
client, calls = make_stateful_redeploy_mock(
|
||||
"RUNNING",
|
||||
build_stream_raises=SynologyError(
|
||||
"build_stream transport error: ConnectError: nas offline", code=0
|
||||
),
|
||||
)
|
||||
tools = make_projects_tools(client)
|
||||
|
||||
with patch("mcp_synology_container.modules.projects.asyncio.sleep"):
|
||||
result = await tools["redeploy_project"]("myapp", confirmed=True)
|
||||
|
||||
# No raw stack trace — clean message
|
||||
assert "transport error" in result
|
||||
assert "ConnectError" in result
|
||||
# The recovery hint must point at the actual situation
|
||||
assert "STOPPED" in result
|
||||
assert "start_project" in result
|
||||
# Old misleading workaround text must NOT appear
|
||||
assert "stop_project + start_project separately" not in result
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_redeploy_build_stream_error_on_stopped_project_keeps_old_workaround():
|
||||
"""If the project was STOPPED to begin with, no stop was issued, so the
|
||||
'STOPPED recovery' hint is NOT appropriate — keep the original workaround."""
|
||||
from mcp_synology_container.dsm_client import SynologyError
|
||||
|
||||
client, calls = make_stateful_redeploy_mock(
|
||||
"STOPPED",
|
||||
build_stream_raises=SynologyError("build failed", code=114),
|
||||
)
|
||||
tools = make_projects_tools(client)
|
||||
|
||||
with patch("mcp_synology_container.modules.projects.asyncio.sleep"):
|
||||
result = await tools["redeploy_project"]("myapp", confirmed=True)
|
||||
|
||||
assert "build failed" in result or "Error during redeploy" in result
|
||||
# Stop was never issued; new recovery hint should not appear
|
||||
assert "was stopped before this error" not in result
|
||||
|
||||
|
||||
# ──────────────────────────────────────────────────────────────────────
|
||||
# M-5: polling exits early on BUILD_FAILED / ERROR
|
||||
# ──────────────────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_wait_for_project_running_returns_early_on_build_failed():
|
||||
"""_wait_for_project_running must exit as soon as DSM reports BUILD_FAILED,
|
||||
not wait the full timeout."""
|
||||
from mcp_synology_container.modules.projects import _wait_for_project_running
|
||||
|
||||
client = AsyncMock()
|
||||
|
||||
async def mock_request(api, method, **kwargs):
|
||||
if method == "list":
|
||||
return project_list("BUILD_FAILED")
|
||||
return {}
|
||||
|
||||
client.request.side_effect = mock_request
|
||||
|
||||
with patch("mcp_synology_container.modules.projects.asyncio.sleep"):
|
||||
# 100s timeout, 2s interval — if the early-exit isn't there the test
|
||||
# would still terminate quickly because sleep is mocked, but the call
|
||||
# count assertion below catches a non-exiting loop.
|
||||
result = await _wait_for_project_running(client, "myapp", timeout=100, interval=2)
|
||||
|
||||
assert result == "BUILD_FAILED"
|
||||
# Only a few list() calls — exit was on the first poll iteration.
|
||||
list_calls = [c for c in client.request.call_args_list if c.args[1] == "list"]
|
||||
assert len(list_calls) <= 2
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_wait_for_project_running_returns_early_on_error():
|
||||
from mcp_synology_container.modules.projects import _wait_for_project_running
|
||||
|
||||
client = AsyncMock()
|
||||
|
||||
async def mock_request(api, method, **kwargs):
|
||||
if method == "list":
|
||||
return project_list("ERROR")
|
||||
return {}
|
||||
|
||||
client.request.side_effect = mock_request
|
||||
|
||||
with patch("mcp_synology_container.modules.projects.asyncio.sleep"):
|
||||
result = await _wait_for_project_running(client, "myapp", timeout=100, interval=2)
|
||||
|
||||
assert result == "ERROR"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_redeploy_surfaces_build_failed_with_hint():
|
||||
"""When polling reports BUILD_FAILED, redeploy_project must include a clear
|
||||
hint to inspect the image tag and retry."""
|
||||
client = AsyncMock()
|
||||
build_done = False
|
||||
|
||||
async def mock_request(api, method, **kwargs):
|
||||
if method == "list":
|
||||
return project_list("BUILD_FAILED") if build_done else project_list("RUNNING")
|
||||
return {}
|
||||
|
||||
async def mock_build_stream(project_id):
|
||||
nonlocal build_done
|
||||
build_done = True
|
||||
|
||||
client.request.side_effect = mock_request
|
||||
client.trigger_build_stream = AsyncMock(side_effect=mock_build_stream)
|
||||
tools = make_projects_tools(client)
|
||||
|
||||
with patch("mcp_synology_container.modules.projects.asyncio.sleep"):
|
||||
result = await tools["redeploy_project"]("myapp", confirmed=True)
|
||||
|
||||
assert "Redeploy failed" in result
|
||||
assert "BUILD_FAILED" in result
|
||||
assert "update_image_tag" in result
|
||||
assert "redeployed successfully" not in result
|
||||
# Polling must have exited early, not run to the full timeout.
|
||||
list_calls = [c for c in client.request.call_args_list if c.args[1] == "list"]
|
||||
# Generous upper bound — early exit means handful of polls, not hundreds.
|
||||
assert len(list_calls) <= 5
|
||||
|
||||
@@ -305,3 +305,92 @@ async def test_system_prune_api_error():
|
||||
|
||||
result = await tools["system_prune"](confirmed=True)
|
||||
assert "Error" in result
|
||||
|
||||
|
||||
# ──────────────────────────────────────────────────────────────────────
|
||||
# M-6: system_prune preview now counts unused networks
|
||||
# ──────────────────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
SAMPLE_NETWORKS_FOR_PRUNE = {
|
||||
"network": [
|
||||
# User-created, no containers attached → will be pruned
|
||||
{"name": "orphan_net", "driver": "bridge", "containers": []},
|
||||
# User-created, in use → must NOT be counted
|
||||
{
|
||||
"name": "myapp_default",
|
||||
"driver": "bridge",
|
||||
"containers": ["web", "db"],
|
||||
},
|
||||
# Built-in networks: Docker never prunes these even if empty
|
||||
{"name": "bridge", "driver": "bridge", "containers": []},
|
||||
{"name": "host", "driver": "host", "containers": []},
|
||||
{"name": "none", "driver": "null", "containers": []},
|
||||
# Another user-created empty network
|
||||
{"name": "legacy_net", "driver": "bridge", "containers": []},
|
||||
]
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_system_prune_preview_counts_unused_networks() -> None:
|
||||
"""M-6: preview must enumerate user-created networks with no containers,
|
||||
skipping the three built-in networks (bridge/host/none)."""
|
||||
from mcp_synology_container.modules.system import register_system
|
||||
|
||||
client = AsyncMock()
|
||||
|
||||
async def mock_request(api, method, **kwargs):
|
||||
if api == "SYNO.Docker.Image":
|
||||
return SAMPLE_IMAGES
|
||||
if api == "SYNO.Docker.Container":
|
||||
return SAMPLE_CONTAINERS
|
||||
if api == "SYNO.Docker.Network":
|
||||
return SAMPLE_NETWORKS_FOR_PRUNE
|
||||
return {}
|
||||
|
||||
client.request.side_effect = mock_request
|
||||
mcp, tools = make_mock_mcp()
|
||||
register_system(mcp, make_config(), client)
|
||||
|
||||
result = await tools["system_prune"]()
|
||||
|
||||
# Two unused user-created networks; the three built-ins must not appear.
|
||||
assert "Unused networks: 2" in result
|
||||
assert "orphan_net" in result
|
||||
assert "legacy_net" in result
|
||||
# Built-in network names must not appear in the prune preview.
|
||||
assert " - bridge " not in result
|
||||
assert " - host " not in result
|
||||
assert " - none " not in result
|
||||
# Network with containers must not be listed.
|
||||
assert "myapp_default" not in result
|
||||
# Old "not counted" placeholder must be gone.
|
||||
assert "not counted" not in result
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_system_prune_preview_network_fetch_failure_is_nonfatal() -> None:
|
||||
"""If the network list fetch fails, the preview still works (0 networks)."""
|
||||
from mcp_synology_container.dsm_client import SynologyError
|
||||
from mcp_synology_container.modules.system import register_system
|
||||
|
||||
client = AsyncMock()
|
||||
|
||||
async def mock_request(api, method, **kwargs):
|
||||
if api == "SYNO.Docker.Image":
|
||||
return SAMPLE_IMAGES
|
||||
if api == "SYNO.Docker.Container":
|
||||
return SAMPLE_CONTAINERS
|
||||
if api == "SYNO.Docker.Network":
|
||||
raise SynologyError("network list failed", code=100)
|
||||
return {}
|
||||
|
||||
client.request.side_effect = mock_request
|
||||
mcp, tools = make_mock_mcp()
|
||||
register_system(mcp, make_config(), client)
|
||||
|
||||
result = await tools["system_prune"]()
|
||||
# Preview still renders; networks count falls back to 0.
|
||||
assert "preview" in result.lower()
|
||||
assert "Unused networks: 0" in result
|
||||
|
||||
Reference in New Issue
Block a user