refactor(verify): one honest return type for the compose live-state probe (str | None)
The helper returned container names OR a synthetic pseudo-name so the caller would refuse; on a hung daemon that rendered as "already has running container(s) (<docker compose ps timed out ...>)". It now returns the refusal reason or None, and the caller prints the reason as given.
This commit is contained in:
+16
-18
@@ -182,15 +182,13 @@ def _run_start_phase(
|
||||
return ReadinessResult(url, ready, status, time.monotonic() - started, error, _tail(output))
|
||||
|
||||
|
||||
def _running_compose_containers(root: Path) -> list[str] | None:
|
||||
"""Names of currently-running containers for the compose project at *root*,
|
||||
via ``docker compose ps`` (read-only; never mutates anything).
|
||||
def _compose_live_state_reason(root: Path) -> str | None:
|
||||
"""Why ``docker compose build``/``up`` must not run at *root*, or ``None`` to proceed.
|
||||
|
||||
``None`` only when docker itself is absent -- the build phase would fail the
|
||||
same way, so there is nothing to protect. A hung daemon or a non-zero probe
|
||||
is reported as a refusal: containers may be live and unobservable, which is
|
||||
exactly the #103567 loss window, and ``docker compose build`` would not have
|
||||
fared better against the same daemon.
|
||||
Read-only ``docker compose ps`` probe. Only a missing docker binary proceeds -- the
|
||||
build phase would fail the same way, so there is nothing to protect. A hung daemon
|
||||
or a non-zero probe refuses: containers may be live and unobservable, which is
|
||||
exactly the #103567 loss window.
|
||||
"""
|
||||
try:
|
||||
result = subprocess.run(
|
||||
@@ -200,11 +198,12 @@ def _running_compose_containers(root: Path) -> list[str] | None:
|
||||
except FileNotFoundError:
|
||||
return None
|
||||
except subprocess.TimeoutExpired:
|
||||
return ["<docker compose ps timed out after 15s; live containers cannot be ruled out>"]
|
||||
return "docker compose ps timed out after 15s; live containers cannot be ruled out"
|
||||
if result.returncode != 0:
|
||||
detail = (result.stderr or result.stdout or "").strip().splitlines()
|
||||
return [f"<docker compose ps failed (exit {result.returncode}): {detail[-1] if detail else 'no output'}>"]
|
||||
return [line for line in result.stdout.splitlines() if line.strip()]
|
||||
return f"docker compose ps failed (exit {result.returncode}): {detail[-1] if detail else 'no output'}"
|
||||
names = [line for line in result.stdout.splitlines() if line.strip()]
|
||||
return f"this compose project already has running container(s): {', '.join(names)}" if names else None
|
||||
|
||||
|
||||
def run_verify(
|
||||
@@ -229,16 +228,15 @@ def run_verify(
|
||||
|
||||
mutating = ("build" in selected) or ("start" in selected and not skip_start)
|
||||
if recipe.kind == "compose" and mutating:
|
||||
running = _running_compose_containers(root)
|
||||
if running:
|
||||
reason = _compose_live_state_reason(root)
|
||||
if reason:
|
||||
result.phases.append(PhaseResult(
|
||||
phase="build", command=recipe.build[0] if recipe.build else "docker compose build",
|
||||
exit_code=1, duration=0.0, output_tail=(
|
||||
"Refusing to run: this compose project already has running "
|
||||
f"container(s) ({', '.join(running)}). `docker compose build` + "
|
||||
"`up` would replace them on an image-hash change, destroying any "
|
||||
"container-local state they carry. If you intend to rebuild this "
|
||||
"live deployment, run `docker compose build`/`up` yourself."
|
||||
f"Refusing to run: {reason}. `docker compose build` + `up` would replace "
|
||||
"live containers on an image-hash change, destroying any container-local "
|
||||
"state they carry. If you intend to rebuild this live deployment, run "
|
||||
"`docker compose build`/`up` yourself."
|
||||
),
|
||||
))
|
||||
return result
|
||||
|
||||
@@ -116,11 +116,7 @@ class TestRunner:
|
||||
|
||||
|
||||
class TestComposeGuard:
|
||||
"""Regression for issue #103567: a compose recipe must refuse to run
|
||||
against a project directory that's already a live compose deployment
|
||||
with running containers -- ``docker compose build`` + ``up`` replaces
|
||||
them on an image-hash change, destroying container-local state (a real
|
||||
incident: a kanban worker's state DB was lost this way)."""
|
||||
"""#103567: a compose recipe must not build/up over a live deployment."""
|
||||
|
||||
def _compose_recipe(self):
|
||||
return Recipe(
|
||||
@@ -153,10 +149,7 @@ class TestComposeGuard:
|
||||
result = run_verify(tmp_path, self._compose_recipe(), skip_start=False)
|
||||
|
||||
assert not result.ok
|
||||
assert "Refusing to run" in result.phases[0].output_tail
|
||||
if isinstance(probe, MagicMock) and probe.returncode == 0:
|
||||
assert "myproject-db-1" in result.phases[0].output_tail
|
||||
assert "myproject-web-1" in result.phases[0].output_tail
|
||||
assert result.phases[0].exit_code == 1
|
||||
# Only the read-only probe ran -- never build or up.
|
||||
assert len(calls) == 1
|
||||
assert calls[0][:3] == ["docker", "compose", "ps"]
|
||||
@@ -182,7 +175,7 @@ class TestComposeGuard:
|
||||
|
||||
def test_guard_skipped_when_no_mutating_phase_selected(self, tmp_path, monkeypatch):
|
||||
probe = MagicMock()
|
||||
monkeypatch.setattr("agent.verify.runner._running_compose_containers", probe)
|
||||
monkeypatch.setattr("agent.verify.runner._compose_live_state_reason", probe)
|
||||
|
||||
with patch("agent.verify.runner._run_phase_command") as mock_phase:
|
||||
mock_phase.return_value = MagicMock(ok=True, phase="test")
|
||||
|
||||
Reference in New Issue
Block a user