Fail on undocumented HTTP statuses and tighten input validation

README.md

@@ -59,7 +59,7 @@ with ClusterClient(["node1.example.com", "node2.example.com"], timeout=5.0) as c
 
 ## How it works
 
-The whole design rests on three words for what a node holds for a given group: `PRESENT`, `ABSENT` and `UNKNOWN`. `UNKNOWN` means the client could not prove either state: GET failed three times in a row, or answered with something that is not in the contract.
+The whole design rests on three words for what a node holds for a given group: `PRESENT`, `ABSENT` and `UNKNOWN`. `UNKNOWN` means the client could not prove either state: GET could not determine it within the retry budget, three attempts by default, or answered with something that is not in the contract.
 
 **Read before write.** The client asks every node for the group's state first. If any node is `UNKNOWN`, it stops there and changes nothing. You cannot safely undo later what you could not see now.
 
@@ -71,7 +71,7 @@ The whole design rests on three words for what a node holds for a given group: `
 
 **Ambiguous mutation outcomes are reconciled via GET.** A timeout, a connection error or a 5xx after POST or DELETE does not say whether the node applied the change. The client reads the node and lets the observed state decide. If the state is already the requested one, the write happened and the answer was lost. A `400` from POST and a `404` from DELETE are handled the same way, because the brief says a `400` may mean the group already exists and does not say what a `404` on DELETE means.
 
-**A mutation is repeated only with permission.** POST or DELETE is sent again only when all three hold: the previous answer was ambiguous, GET shows the node still in its old state, and attempts remain. Three attempts in total, with 0.1 s and then 0.2 s between them. A `400`, a `404` or any status outside the contract never leads to a second attempt: after the GET it is either already done or a hard failure.
+**A mutation is repeated only with permission.** POST or DELETE is sent again only when all three hold: the previous answer was ambiguous, GET shows the node still in its old state, and attempts remain. Three attempts in total, with 0.1 s and then 0.2 s between them. A `400` or a `404` never leads to a second attempt: after the GET it is either already done or a hard failure. Any status outside the contract is a hard failure.
 
 **Rollback is best-effort compensating rollback.** On failure the client walks the nodes it touched in reverse order and drives each one back to the state observed before the operation, using the same rules as the forward path. It never deletes a group that existed before a create, never recreates a group that was absent before a delete, and never touches a node it did not reach. A node whose last verified state is already its old state needs no call at all.
 
@@ -83,7 +83,7 @@ Where the brief is silent, this is what I chose.
 
 - A host is a bare hostname, optionally with a port, or a root `http://` / `https://` URL. Bare hostnames get `http://`. A trailing slash is tolerated; a path, query or fragment is rejected. An empty host list and duplicate hosts are rejected before any call.
 - A group id is any string with at least one non-whitespace character. It is sent unchanged in request bodies and percent-encoded in the GET path. Further naming rules belong to the server.
-- Only the documented success codes count as success. Any other status, including other 2xx codes, is checked with GET and never retried.
+- Only the documented success codes count as success. Any other status, including other 2xx codes, is a failure: the client reads the node once so the rollback knows its state, but never counts it as success and never retries.
 - A GET answering `200` with a missing or different `groupId` is a protocol failure: `UNKNOWN`, without retry. A node that answers wrongly does not get better by being asked again.
 - Every request has an explicit timeout, default 5 seconds. Backoff is fixed exponential without jitter.
 - Nothing else changes the same group while an operation runs. The API offers no concurrency control, so the client cannot detect a concurrent writer.
@@ -101,7 +101,7 @@ ruff check .
 ruff format --check .
 ```
 
-The tests use `httpx.MockTransport`, so nothing touches the network, and the retry policy takes an injected sleep function, so nothing waits. Every failure-path test asserts the exact sequence of HTTP calls, which is how "no blind retry" and "rollback touches only my nodes" are proven rather than assumed.
+The tests use `httpx.MockTransport`, so nothing touches the network, and the retry policy takes an injected sleep function, so nothing waits. Key failure-path tests assert the exact sequence of HTTP calls, which is how "no blind retry" and "rollback touches only my nodes" are proven rather than assumed.
 
 ## What I would add for production
 

src/api_consumer/client.py

@@ -6,7 +6,7 @@
 
 import httpx
 
-from api_consumer.node_api import NodeAPI, NodeState
+from api_consumer.node_api import NodeAPI, NodeState, normalize_host
 from api_consumer.retry import RetryPolicy, drive_node_to, observe
 
 
@@ -66,15 +66,15 @@ def __init__(
         policy: RetryPolicy | None = None,
         client: httpx.Client | None = None,
     ) -> None:
+        normalized = [normalize_host(host) for host in hosts]
+        if not normalized:
+            raise ValueError("at least one host is required")
+        if len(set(normalized)) != len(normalized):
+            raise ValueError(f"duplicate hosts: {normalized}")
         self._owns_client = client is None
         self._client = client or httpx.Client(timeout=timeout)
         self._policy = policy or RetryPolicy()
-        self._nodes = [NodeAPI(host, self._client) for host in hosts]
-        if not self._nodes:
-            raise ValueError("at least one host is required")
-        seen = [node.host for node in self._nodes]
-        if len(set(seen)) != len(seen):
-            raise ValueError(f"duplicate hosts: {seen}")
+        self._nodes = [NodeAPI(host, self._client) for host in normalized]
 
     def close(self) -> None:
         """Close the HTTP client, if this instance created it."""
@@ -96,8 +96,8 @@ def delete_group(self, group_id: str) -> OperationResult:
         return self._apply("delete", group_id, NodeState.ABSENT)
 
     def _apply(self, operation: str, group_id: str, desired: NodeState) -> OperationResult:
-        if not group_id.strip():
-            raise ValueError("group_id must contain at least one non-whitespace character")
+        if not isinstance(group_id, str) or not group_id.strip():
+            raise ValueError("group_id must be a string with at least one non-whitespace character")
         before = {node.host: observe(node, group_id, self._policy) for node in self._nodes}
         for host, state in before.items():
             if state is NodeState.UNKNOWN:

src/api_consumer/retry.py

@@ -45,9 +45,11 @@ def drive_node_to(
 ) -> DriveResult:
     """Send the mutation for `desired` and settle its outcome by the contract rules.
 
-    A documented success is trusted. Every other answer is followed by GET, and the
-    observed state decides. The mutation is sent again only after an ambiguous answer,
-    when GET shows the node still in its old state, and while attempts remain.
+    A documented success is trusted. Every other answer is followed by GET. After an
+    ambiguous answer or a clarifying 400/404 the observed state decides. A status outside
+    the contract is a failure whatever GET shows; the GET only records the node's state
+    for the rollback. The mutation is sent again only after an ambiguous answer, when GET
+    shows the node still in its old state, and while attempts remain.
     """
     if desired is NodeState.UNKNOWN:
         raise ValueError("desired state must be PRESENT or ABSENT")
@@ -64,6 +66,8 @@ def drive_node_to(
             f"{verb} was {outcome.value} (attempt {attempt} of {policy.max_attempts}), "
             f"GET shows {state.value}"
         )
+        if outcome is Outcome.UNEXPECTED:
+            return DriveResult(False, state, why)
         if state is desired:
             return DriveResult(True, desired, why)
         if outcome is Outcome.AMBIGUOUS and state is old and attempt < policy.max_attempts:

tests/test_client_create.py

@@ -109,8 +109,8 @@ def test_empty_or_duplicate_hosts_are_rejected(hosts):
         ClusterClient(hosts, client=FakeCluster().client)
 
 
-@pytest.mark.parametrize("group_id", ["", "   "])
-def test_blank_group_id_is_rejected_before_any_call(group_id):
+@pytest.mark.parametrize("group_id", ["", "   ", 123, None])
+def test_invalid_group_id_is_rejected_before_any_call(group_id):
     fake, client = cluster_with({})
     with pytest.raises(ValueError):
         client.create_group(group_id)

tests/test_drive.py

@@ -107,3 +107,15 @@ def test_a_policy_without_attempts_is_rejected():
     node = FakeCluster().node("node1")
     with pytest.raises(ValueError):
         drive_node_to(node, "g1", PRESENT, RetryPolicy(max_attempts=0, sleep=lambda _: None))
+
+
+def test_undocumented_status_fails_even_when_get_shows_the_desired_state():
+    result, calls, slept = drive(PRESENT, mutations=[200], gets=[200])
+    assert (result.reached, result.state) == (False, PRESENT)
+    assert calls == ["POST", "GET"] and slept == []
+
+
+def test_delete_undocumented_status_fails_even_when_get_shows_absent():
+    result, calls, _ = drive(ABSENT, mutations=[403], gets=[404])
+    assert (result.reached, result.state) == (False, ABSENT)
+    assert calls == ["DELETE", "GET"]