|
|
|
@@ -0,0 +1,315 @@
|
|
|
|
|
"""Step definitions for TDD Bug #993 — server_connect non-atomic writes.
|
|
|
|
|
|
|
|
|
|
This test captures bug #993: ``server_connect`` in ``server.py`` makes three
|
|
|
|
|
sequential ``set_value()`` calls (``server.url``, ``server.namespace``,
|
|
|
|
|
``server.tls-verify``) with no transaction, no try/except, and no rollback.
|
|
|
|
|
If a middle call fails (e.g. disk full, permissions error), the earlier values
|
|
|
|
|
are already persisted while the later ones retain their old values, leaving the
|
|
|
|
|
configuration in a half-written state.
|
|
|
|
|
|
|
|
|
|
The test uses the ``@tdd_expected_fail`` tag until the fix in #993 is merged.
|
|
|
|
|
See CONTRIBUTING.md > Bug Fix Workflow > TDD Bug Test Tags.
|
|
|
|
|
"""
|
|
|
|
|
|
|
|
|
|
from __future__ import annotations
|
|
|
|
|
|
|
|
|
|
import os
|
|
|
|
|
import shutil
|
|
|
|
|
import tempfile
|
|
|
|
|
import tomllib
|
|
|
|
|
from pathlib import Path
|
|
|
|
|
from typing import Any
|
|
|
|
|
from unittest.mock import patch
|
|
|
|
|
|
|
|
|
|
from behave import given, then, when
|
|
|
|
|
from behave.runner import Context
|
|
|
|
|
|
|
|
|
|
from cleveragents.application.services.config_service import ConfigService
|
|
|
|
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
# Helpers
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _read_flat_config(config_path: Path) -> dict[str, Any]:
|
|
|
|
|
"""Read the TOML config and return a flat dict of top-level keys."""
|
|
|
|
|
if not config_path.exists():
|
|
|
|
|
return {}
|
|
|
|
|
with open(config_path, "rb") as fh:
|
|
|
|
|
return tomllib.load(fh)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
class _FailingConfigService(ConfigService):
|
|
|
|
|
"""A ConfigService subclass that raises on a specified set_value call.
|
|
|
|
|
|
|
|
|
|
Parameters
|
|
|
|
|
----------
|
|
|
|
|
fail_on_call_number:
|
|
|
|
|
The 1-based call number of ``set_value`` on which to raise.
|
|
|
|
|
For example, ``fail_on_call_number=2`` raises on the second call.
|
|
|
|
|
"""
|
|
|
|
|
|
|
|
|
|
def __init__(
|
|
|
|
|
self,
|
|
|
|
|
*,
|
|
|
|
|
config_dir: Path,
|
|
|
|
|
config_path: Path,
|
|
|
|
|
fail_on_call_number: int,
|
|
|
|
|
event_bus: Any = None,
|
|
|
|
|
) -> None:
|
|
|
|
|
super().__init__(
|
|
|
|
|
config_dir=config_dir,
|
|
|
|
|
config_path=config_path,
|
|
|
|
|
event_bus=event_bus,
|
|
|
|
|
)
|
|
|
|
|
self._fail_on_call = fail_on_call_number
|
|
|
|
|
self._call_count = 0
|
|
|
|
|
|
|
|
|
|
def set_value(self, key: str, value: Any) -> None:
|
|
|
|
|
"""Override set_value to fail on a specific call number."""
|
|
|
|
|
self._call_count += 1
|
|
|
|
|
if self._call_count == self._fail_on_call:
|
|
|
|
|
raise OSError(
|
|
|
|
|
f"Simulated disk failure on set_value call #{self._call_count} "
|
|
|
|
|
f"(key={key!r})"
|
|
|
|
|
)
|
|
|
|
|
super().set_value(key, value)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
# Given steps
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
|
|
|
|
_ORIGINAL_URL: str = "https://original.example.com"
|
|
|
|
|
_ORIGINAL_NAMESPACE: str = "original-ns"
|
|
|
|
|
_ORIGINAL_TLS_VERIFY: bool = False
|
|
|
|
|
|
|
|
|
|
_NEW_URL: str = "https://new-server.example.com"
|
|
|
|
|
_NEW_NAMESPACE: str = "new-ns"
|
|
|
|
|
_NEW_TLS_VERIFY: bool = True
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@given("a fresh config directory for atomic write test")
|
|
|
|
|
def step_fresh_config_dir(context: Context) -> None:
|
|
|
|
|
"""Create a temporary HOME and config directory for isolated testing."""
|
|
|
|
|
context.atomic_test_home = tempfile.mkdtemp(prefix="tdd993_")
|
|
|
|
|
context.atomic_config_dir = Path(context.atomic_test_home) / ".cleveragents"
|
|
|
|
|
context.atomic_config_dir.mkdir(parents=True, exist_ok=True)
|
|
|
|
|
context.atomic_config_path = context.atomic_config_dir / "config.toml"
|
|
|
|
|
context.atomic_old_home = os.environ.get("HOME")
|
|
|
|
|
os.environ["HOME"] = context.atomic_test_home
|
|
|
|
|
|
|
|
|
|
# Register cleanup
|
|
|
|
|
def _cleanup() -> None:
|
|
|
|
|
old_home = getattr(context, "atomic_old_home", None)
|
|
|
|
|
if old_home is not None:
|
|
|
|
|
os.environ["HOME"] = old_home
|
|
|
|
|
else:
|
|
|
|
|
os.environ.pop("HOME", None)
|
|
|
|
|
test_home = getattr(context, "atomic_test_home", None)
|
|
|
|
|
if test_home is not None:
|
|
|
|
|
shutil.rmtree(test_home, ignore_errors=True)
|
|
|
|
|
|
|
|
|
|
if not hasattr(context, "_cleanup_handlers"):
|
|
|
|
|
context._cleanup_handlers = []
|
|
|
|
|
context._cleanup_handlers.append(_cleanup)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@given("the config has pre-existing server values")
|
|
|
|
|
def step_pre_existing_config(context: Context) -> None:
|
|
|
|
|
"""Write pre-existing server config values to the TOML file."""
|
|
|
|
|
svc = ConfigService(
|
|
|
|
|
config_dir=context.atomic_config_dir,
|
|
|
|
|
config_path=context.atomic_config_path,
|
|
|
|
|
)
|
|
|
|
|
svc.set_value("server.url", _ORIGINAL_URL)
|
|
|
|
|
svc.set_value("server.namespace", _ORIGINAL_NAMESPACE)
|
|
|
|
|
svc.set_value("server.tls-verify", _ORIGINAL_TLS_VERIFY)
|
|
|
|
|
|
|
|
|
|
# Verify values were written
|
|
|
|
|
data = _read_flat_config(context.atomic_config_path)
|
|
|
|
|
assert data.get("server.url") == _ORIGINAL_URL
|
|
|
|
|
assert data.get("server.namespace") == _ORIGINAL_NAMESPACE
|
|
|
|
|
assert data.get("server.tls-verify") == _ORIGINAL_TLS_VERIFY
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@given("the config has no server values")
|
|
|
|
|
def step_no_server_values(context: Context) -> None:
|
|
|
|
|
"""Ensure the config file has no server values (start clean)."""
|
|
|
|
|
data = _read_flat_config(context.atomic_config_path)
|
|
|
|
|
assert "server.url" not in data
|
|
|
|
|
assert "server.namespace" not in data
|
|
|
|
|
assert "server.tls-verify" not in data
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
# When steps
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@when("I invoke server_connect but set_value fails on the second call")
|
|
|
|
|
def step_invoke_with_second_call_failure(context: Context) -> None:
|
|
|
|
|
"""Call server_connect with a ConfigService that fails on set_value #2.
|
|
|
|
|
|
|
|
|
|
The three set_value calls in server_connect are:
|
|
|
|
|
1. svc.set_value("server.url", ...) — succeeds
|
|
|
|
|
2. svc.set_value("server.namespace", ...) — FAILS (OSError)
|
|
|
|
|
3. svc.set_value("server.tls-verify", ...) — never reached
|
|
|
|
|
|
|
|
|
|
This simulates a disk-full or permissions error on the second write.
|
|
|
|
|
"""
|
|
|
|
|
failing_svc = _FailingConfigService(
|
|
|
|
|
config_dir=context.atomic_config_dir,
|
|
|
|
|
config_path=context.atomic_config_path,
|
|
|
|
|
fail_on_call_number=2,
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
with patch(
|
|
|
|
|
"cleveragents.cli.commands.server._get_config_service",
|
|
|
|
|
return_value=failing_svc,
|
|
|
|
|
):
|
|
|
|
|
context.atomic_error = None
|
|
|
|
|
try:
|
|
|
|
|
from cleveragents.cli.commands.server import server_connect
|
|
|
|
|
|
|
|
|
|
# Call the function directly (bypassing Typer CLI) to isolate
|
|
|
|
|
# the config-writing logic from CLI framework error handling.
|
|
|
|
|
server_connect(
|
|
|
|
|
server_url=_NEW_URL,
|
|
|
|
|
namespace=_NEW_NAMESPACE,
|
|
|
|
|
tls_verify=_NEW_TLS_VERIFY,
|
|
|
|
|
fmt="json",
|
|
|
|
|
)
|
|
|
|
|
except (OSError, SystemExit) as exc:
|
|
|
|
|
context.atomic_error = exc
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@when("I invoke server_connect but set_value fails on the third call")
|
|
|
|
|
def step_invoke_with_third_call_failure(context: Context) -> None:
|
|
|
|
|
"""Call server_connect with a ConfigService that fails on set_value #3.
|
|
|
|
|
|
|
|
|
|
The three set_value calls in server_connect are:
|
|
|
|
|
1. svc.set_value("server.url", ...) — succeeds
|
|
|
|
|
2. svc.set_value("server.namespace", ...) — succeeds
|
|
|
|
|
3. svc.set_value("server.tls-verify", ...) — FAILS (OSError)
|
|
|
|
|
"""
|
|
|
|
|
failing_svc = _FailingConfigService(
|
|
|
|
|
config_dir=context.atomic_config_dir,
|
|
|
|
|
config_path=context.atomic_config_path,
|
|
|
|
|
fail_on_call_number=3,
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
with patch(
|
|
|
|
|
"cleveragents.cli.commands.server._get_config_service",
|
|
|
|
|
return_value=failing_svc,
|
|
|
|
|
):
|
|
|
|
|
context.atomic_error = None
|
|
|
|
|
try:
|
|
|
|
|
from cleveragents.cli.commands.server import server_connect
|
|
|
|
|
|
|
|
|
|
server_connect(
|
|
|
|
|
server_url=_NEW_URL,
|
|
|
|
|
namespace=_NEW_NAMESPACE,
|
|
|
|
|
tls_verify=_NEW_TLS_VERIFY,
|
|
|
|
|
fmt="json",
|
|
|
|
|
)
|
|
|
|
|
except (OSError, SystemExit) as exc:
|
|
|
|
|
context.atomic_error = exc
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
# Then steps
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@then("an error should have been raised during server_connect")
|
|
|
|
|
def step_check_error_raised(context: Context) -> None:
|
|
|
|
|
"""Assert that the simulated OSError was actually raised.
|
|
|
|
|
|
|
|
|
|
If server_connect silently swallows the OSError (e.g. via a blanket
|
|
|
|
|
try/except), the When step would succeed without raising, and the
|
|
|
|
|
subsequent config-checking Then steps could pass for the wrong reason.
|
|
|
|
|
This step guards against that false-pass scenario.
|
|
|
|
|
"""
|
|
|
|
|
assert context.atomic_error is not None, (
|
|
|
|
|
"Expected an OSError or SystemExit to be raised during "
|
|
|
|
|
"server_connect, but no error was captured. The simulated disk "
|
|
|
|
|
"failure may have been silently swallowed."
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@then("the config should have rolled back server.url to its original value")
|
|
|
|
|
def step_check_url_rolled_back(context: Context) -> None:
|
|
|
|
|
"""Assert server.url was rolled back to the original pre-existing value.
|
|
|
|
|
|
|
|
|
|
Bug #993: server.url is written by the FIRST set_value() call, which
|
|
|
|
|
succeeds before the second call fails. Without rollback, server.url
|
|
|
|
|
retains the NEW value — proving the config is in a half-written state.
|
|
|
|
|
This assertion FAILS while the bug exists (server.url == NEW_URL).
|
|
|
|
|
"""
|
|
|
|
|
data = _read_flat_config(context.atomic_config_path)
|
|
|
|
|
actual_url = data.get("server.url")
|
|
|
|
|
assert actual_url == _ORIGINAL_URL, (
|
|
|
|
|
f"Bug #993: server.url was NOT rolled back after a partial failure. "
|
|
|
|
|
f"Expected original value {_ORIGINAL_URL!r}, but got {actual_url!r}. "
|
|
|
|
|
f"The config is in a half-written state because set_value() calls "
|
|
|
|
|
f"are not atomic."
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@then("the config should still have the original server.namespace")
|
|
|
|
|
def step_check_namespace_unchanged(context: Context) -> None:
|
|
|
|
|
"""Assert server.namespace still has the original value."""
|
|
|
|
|
data = _read_flat_config(context.atomic_config_path)
|
|
|
|
|
actual_ns = data.get("server.namespace")
|
|
|
|
|
assert actual_ns == _ORIGINAL_NAMESPACE, (
|
|
|
|
|
f"Expected original namespace {_ORIGINAL_NAMESPACE!r}, got {actual_ns!r}."
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@then("the config should still have the original server.tls-verify")
|
|
|
|
|
def step_check_tls_unchanged(context: Context) -> None:
|
|
|
|
|
"""Assert server.tls-verify still has the original value."""
|
|
|
|
|
data = _read_flat_config(context.atomic_config_path)
|
|
|
|
|
actual_tls = data.get("server.tls-verify")
|
|
|
|
|
assert actual_tls == _ORIGINAL_TLS_VERIFY, (
|
|
|
|
|
f"Expected original tls-verify {_ORIGINAL_TLS_VERIFY!r}, got {actual_tls!r}."
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@then("the config should have rolled back server.namespace to its original value")
|
|
|
|
|
def step_check_namespace_rolled_back(context: Context) -> None:
|
|
|
|
|
"""Assert server.namespace was rolled back after third-call failure.
|
|
|
|
|
|
|
|
|
|
When the third set_value fails, both server.url and server.namespace
|
|
|
|
|
have already been persisted with new values. Without rollback,
|
|
|
|
|
server.namespace retains the NEW value — proving non-atomicity.
|
|
|
|
|
This assertion FAILS while the bug exists.
|
|
|
|
|
"""
|
|
|
|
|
data = _read_flat_config(context.atomic_config_path)
|
|
|
|
|
actual_ns = data.get("server.namespace")
|
|
|
|
|
assert actual_ns == _ORIGINAL_NAMESPACE, (
|
|
|
|
|
f"Bug #993: server.namespace was NOT rolled back after a partial "
|
|
|
|
|
f"failure on the third set_value() call. "
|
|
|
|
|
f"Expected original value {_ORIGINAL_NAMESPACE!r}, but got "
|
|
|
|
|
f"{actual_ns!r}. The config is in a half-written state."
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@then("the config should not contain a server.url value")
|
|
|
|
|
def step_check_no_url(context: Context) -> None:
|
|
|
|
|
"""Assert server.url was not persisted when starting from empty config.
|
|
|
|
|
|
|
|
|
|
Bug #993: When starting from a clean config and the second set_value
|
|
|
|
|
fails, server.url has already been written. Without rollback, the
|
|
|
|
|
config now contains a server.url with no matching namespace or
|
|
|
|
|
tls-verify — a half-written state. This assertion FAILS while the
|
|
|
|
|
bug exists.
|
|
|
|
|
"""
|
|
|
|
|
data = _read_flat_config(context.atomic_config_path)
|
|
|
|
|
assert "server.url" not in data, (
|
|
|
|
|
f"Bug #993: server.url was persisted despite a failure in a "
|
|
|
|
|
f"subsequent set_value() call. Found server.url={data['server.url']!r}. "
|
|
|
|
|
f"The config should be empty (all-or-nothing atomicity) but a "
|
|
|
|
|
f"partial write occurred."
|
|
|
|
|
)
|