diff --git a/features/steps/tdd_server_connect_atomic_writes_steps.py b/features/steps/tdd_server_connect_atomic_writes_steps.py new file mode 100644 index 000000000..672153acf --- /dev/null +++ b/features/steps/tdd_server_connect_atomic_writes_steps.py @@ -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." + ) diff --git a/features/tdd_server_connect_atomic_writes.feature b/features/tdd_server_connect_atomic_writes.feature new file mode 100644 index 000000000..9572779a2 --- /dev/null +++ b/features/tdd_server_connect_atomic_writes.feature @@ -0,0 +1,44 @@ +@tdd_expected_fail @tdd_bug @tdd_bug_993 +Feature: TDD Bug #993 — server_connect writes three config values non-atomically + As a developer + I want to verify that server_connect writes all three config values atomically + So that a partial failure does not leave the configuration in a half-written state + + # 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 try/except, no transaction, and no rollback. If the second call + # fails (e.g. disk full, permissions error), server.url is already persisted + # but server.namespace and server.tls-verify retain their old values. The + # config is left in a half-written state. + # + # Expected behavior: all three config values are written atomically — either + # all succeed or all fail (rollback to original state). + # + # This test uses the @tdd_expected_fail tag until the fix in #993 is merged. + # The tag inverts the result so CI passes while the bug is still unfixed. + # See CONTRIBUTING.md > Bug Fix Workflow > TDD Bug Test Tags. + + Scenario: Config remains unchanged when second set_value fails during server_connect + Given a fresh config directory for atomic write test + And the config has pre-existing server values + When I invoke server_connect but set_value fails on the second call + Then an error should have been raised during server_connect + And the config should have rolled back server.url to its original value + And the config should still have the original server.namespace + And the config should still have the original server.tls-verify + + Scenario: Config remains unchanged when third set_value fails during server_connect + Given a fresh config directory for atomic write test + And the config has pre-existing server values + When I invoke server_connect but set_value fails on the third call + Then an error should have been raised during server_connect + And the config should have rolled back server.url to its original value + And the config should have rolled back server.namespace to its original value + And the config should still have the original server.tls-verify + + Scenario: No partial server.url persisted when namespace write raises + Given a fresh config directory for atomic write test + And the config has no server values + When I invoke server_connect but set_value fails on the second call + Then an error should have been raised during server_connect + And the config should not contain a server.url value diff --git a/robot/container_resolve_crash.robot b/robot/container_resolve_crash.robot index 3dcd1c508..e9d4f057d 100644 --- a/robot/container_resolve_crash.robot +++ b/robot/container_resolve_crash.robot @@ -24,7 +24,7 @@ ${HELPER} ${CURDIR}/helper_container_resolve_crash.py Plan Tree Regression Guard For Container.resolve() [Documentation] Regression guard: plan tree should run without resolve() crash [Tags] tdd_bug tdd_bug_647 - ${result}= Run Process ${PYTHON} ${HELPER} plan-tree-crash cwd=${WORKSPACE} timeout=30s + ${result}= Run Process ${PYTHON} ${HELPER} plan-tree-crash cwd=${WORKSPACE} timeout=120s on_timeout=kill Log ${result.stdout} Log ${result.stderr} Run Keyword If ${result.rc} == 2 Fail Unexpected non-AttributeError failure: ${result.stderr} @@ -35,7 +35,7 @@ Plan Tree Regression Guard For Container.resolve() Plan Explain Regression Guard For Container.resolve() [Documentation] Regression guard: plan explain should run without resolve() crash [Tags] tdd_bug tdd_bug_647 - ${result}= Run Process ${PYTHON} ${HELPER} plan-explain-crash cwd=${WORKSPACE} timeout=30s + ${result}= Run Process ${PYTHON} ${HELPER} plan-explain-crash cwd=${WORKSPACE} timeout=120s on_timeout=kill Log ${result.stdout} Log ${result.stderr} Run Keyword If ${result.rc} == 2 Fail Unexpected non-AttributeError failure: ${result.stderr} @@ -46,7 +46,7 @@ Plan Explain Regression Guard For Container.resolve() Plan Correct Regression Guard For Container.resolve() [Documentation] Regression guard: plan correct should run without resolve() crash [Tags] tdd_bug tdd_bug_647 - ${result}= Run Process ${PYTHON} ${HELPER} plan-correct-crash cwd=${WORKSPACE} timeout=30s + ${result}= Run Process ${PYTHON} ${HELPER} plan-correct-crash cwd=${WORKSPACE} timeout=120s on_timeout=kill Log ${result.stdout} Log ${result.stderr} Run Keyword If ${result.rc} == 2 Fail Unexpected non-AttributeError failure: ${result.stderr}