fix(retry,errors): mark a 5xx-refused write ambiguous; stop eating the validation message (#116, #117) (#124)

Two halves of contracts shipped today that are unreachable in practice.

#116 -- `.ambiguous_write` was wired only to the timeout and transport-error
handlers. The 5xx branch got `False` from `should_retry`, then fell through to
a bare `raise error_from_response(...)`: no flag, no note. A 502/503 from a
gateway that already committed at the origin is the MORE common ambiguous
outcome in production than a client-side socket timeout, and it was the one
case the caller was never told about. The write was correctly sent once, so
there is no duplicate -- but a caller doing the documented

    except OilPriceAPIError as e:
        if getattr(e, "ambiguous_write", False):
            reconcile()

skipped reconciliation for exactly the case the fix was written for.

Fixed at all three request helpers -- `client.request`,
`client.request_with_headers`, `async_client.request` -- with a byte-identical
block, so sync and async do not diverge. 4xx and 429 stay unmarked: both
refused the request outright, so nothing landed.

#117 -- `ValidationError.__str__` replaced the message with
"Validation error for '<field>'" whenever `field` was set. `_url._reject()`
always sets `field="path"`, so the four sentences of remediation added in #113
were unreachable through `str(e)`, `print(e)`, `logging.exception` and the
traceback's last line; only `e.message` held them. For a non-string path
(`value is None`) even the reason vanished, leaving bare
"Validation error for 'path'".

The message is now kept and the field/value detail appended. The historical
form is unchanged when no message was supplied, so callers and tests relying
on the field prefix still see it. This was never only a #113 problem --
alerts.py, diesel.py and every other caller passing both `message=` and
`field=` were losing their text the same way.

Tests: tests/unit/test_ambiguous_write_signal.py (12) and
tests/unit/test_validation_error_message.py (8). Both proven red against
`origin/main` source (13 failed / 7 passed) before the fix.

Suite: 112 failed / 650 passed / 63 skipped, against a measured baseline of
112 failed / 630 passed / 63 skipped on clean main (the 112 are pre-existing,
all missing pytest-asyncio and respx in the environment).


Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: karlwaldman

oilpriceapi/async_client.py

@@ -285,10 +285,21 @@ async def request(
                         )
                         await asyncio.sleep(wait_time)
                         continue
-                raise error_from_response(
+                error = error_from_response(
                     response,
                     commodity=params.get("commodity") if params else None,
                 )
+                # A 5xx is ambiguous, not a refusal: a gateway can return 502
+                # after the origin already committed. The write was correctly
+                # sent once and not replayed -- tell the caller the outcome is
+                # unknown so they reconcile instead of assuming it failed
+                # (#116). 4xx and 429 refused the request outright, so nothing
+                # landed and there is nothing to reconcile.
+                if response.status_code >= 500 and not self._retry_strategy.is_replay_safe(
+                    method, idempotent
+                ):
+                    raise mark_ambiguous_write(error, method)
+                raise error
 
             except httpx.TimeoutException as error:
                 last_exception = error_from_exception(

oilpriceapi/client.py

@@ -328,10 +328,21 @@ def request(
                         )
                         time.sleep(wait_time)
                         continue
-                raise error_from_response(
+                error = error_from_response(
                     response,
                     commodity=params.get("commodity") if params else None,
                 )
+                # A 5xx is ambiguous, not a refusal: a gateway can return 502
+                # after the origin already committed. The write was correctly
+                # sent once and not replayed -- tell the caller the outcome is
+                # unknown so they reconcile instead of assuming it failed
+                # (#116). 4xx and 429 refused the request outright, so nothing
+                # landed and there is nothing to reconcile.
+                if response.status_code >= 500 and not self._retry_strategy.is_replay_safe(
+                    method, idempotent
+                ):
+                    raise mark_ambiguous_write(error, method)
+                raise error
 
             except httpx.TimeoutException as error:
                 last_exception = error_from_exception(
@@ -461,10 +472,21 @@ def request_with_headers(
                         )
                         time.sleep(wait_time)
                         continue
-                raise error_from_response(
+                error = error_from_response(
                     response,
                     commodity=params.get("commodity") if params else None,
                 )
+                # A 5xx is ambiguous, not a refusal: a gateway can return 502
+                # after the origin already committed. The write was correctly
+                # sent once and not replayed -- tell the caller the outcome is
+                # unknown so they reconcile instead of assuming it failed
+                # (#116). 4xx and 429 refused the request outright, so nothing
+                # landed and there is nothing to reconcile.
+                if response.status_code >= 500 and not self._retry_strategy.is_replay_safe(
+                    method, idempotent
+                ):
+                    raise mark_ambiguous_write(error, method)
+                raise error
 
             except httpx.TimeoutException as error:
                 last_exception = error_from_exception(

oilpriceapi/exceptions.py

@@ -333,13 +333,31 @@ def __init__(
         self.field = field
         self.value = value
 
+    #: Rendered when the raiser supplied no message of its own.
+    DEFAULT_MESSAGE = "Validation error"
+
     def __str__(self) -> str:
-        message = super().__str__()
-        if self.field:
-            message = f"Validation error for '{self.field}'"
+        if not self.field:
+            return super().__str__()
+
+        # A supplied message is the only place the raiser's guidance lives --
+        # the origin guard in _url.py composes four sentences of remediation --
+        # and this method used to discard it whenever `field` was set, so no
+        # ordinary print, log or traceback could ever show it (#117). Keep the
+        # message and APPEND the field/value detail that other callers, and
+        # their tests, rely on.
+        #
+        # Historically the field-set form carried no "[422]" prefix; keep that.
+        if self.message in ("", self.DEFAULT_MESSAGE):
+            detail = f"Validation error for '{self.field}'"
             if self.value is not None:
-                message += f": invalid value '{self.value}'"
-        return message
+                detail += f": invalid value '{self.value}'"
+            return detail
+
+        detail = f"{self.message} (field '{self.field}'"
+        if self.value is not None:
+            detail += f", invalid value '{self.value}'"
+        return detail + ")"
 
 
 class ServerError(OilPriceAPIError):

tests/unit/test_ambiguous_write_signal.py

@@ -0,0 +1,217 @@
+"""A write whose outcome is unknown must SAY so, on every ambiguous path (#116).
+
+#115 stopped replaying non-idempotent writes and promised the caller two
+things: the write is sent once, AND the resulting error carries
+``.ambiguous_write = True`` plus a note telling them to check whether it
+landed. Only the first half was wired to the 5xx branch.
+
+A 502/503 from a gateway that already committed at the origin is the most
+common ambiguous outcome in production -- more common than the client-side
+socket timeout that was the only marked case. A caller implementing the
+documented contract skipped reconciliation for exactly the case the fix
+existed for.
+
+Every assertion here is made on the exception the public method raises and on
+what reached the transport, never on an internal flag.
+"""
+
+import asyncio
+from unittest.mock import patch
+
+import httpx
+import pytest
+
+from oilpriceapi import AsyncOilPriceAPI, OilPriceAPI
+from oilpriceapi.exceptions import OilPriceAPIError
+
+# Not a credential: a fixture string, every request here hits a mock transport.
+FIXTURE_KEY = "-".join(["fixture", "not", "a", "real", "key"])
+
+
+class _Counter:
+    def __init__(self, outcome):
+        self.methods = []
+        self._outcome = outcome
+
+    def __call__(self, request):
+        self.methods.append(request.method)
+        return self._outcome(request)
+
+
+def _status(code, headers=None):
+    def handler(request):
+        return httpx.Response(code, headers=headers or {}, json={"error": "nope"})
+
+    return handler
+
+
+def _sync_client(counter, **kwargs):
+    client = OilPriceAPI(api_key=FIXTURE_KEY, **kwargs)
+    client._client = httpx.Client(
+        base_url=client.base_url,
+        headers=client.headers,
+        transport=httpx.MockTransport(counter),
+    )
+    return client
+
+
+def _async_client(counter, **kwargs):
+    client = AsyncOilPriceAPI(api_key=FIXTURE_KEY, **kwargs)
+    client._client = httpx.AsyncClient(
+        base_url=client.base_url,
+        headers=client.headers,
+        transport=httpx.MockTransport(counter),
+    )
+    return client
+
+
+def _assert_ambiguous(error, method):
+    assert getattr(error, "ambiguous_write", False) is True, (
+        "the caller was not told the write outcome is unknown"
+    )
+    assert "NOT retried" in str(error)
+    assert method.upper() in str(error)
+
+
+# ---------------------------------------------------------------------------
+# (a) A 5xx on a non-idempotent write is ambiguous and must be marked
+
+
+@pytest.mark.parametrize("code", [500, 502, 503, 504])
+def test_sync_post_5xx_is_marked_ambiguous(code):
+    counter = _Counter(_status(code))
+    client = _sync_client(counter)
+
+    with patch("time.sleep"):
+        with pytest.raises(OilPriceAPIError) as excinfo:
+            client.request("POST", "/v1/subscriptions", json_data={"plan": "x"})
+
+    assert counter.methods == ["POST"], "the write must still be sent exactly once"
+    _assert_ambiguous(excinfo.value, "POST")
+
+
+def test_sync_patch_5xx_is_marked_ambiguous():
+    counter = _Counter(_status(503))
+    client = _sync_client(counter)
+
+    with patch("time.sleep"):
+        with pytest.raises(OilPriceAPIError) as excinfo:
+            client.request("PATCH", "/v1/alerts/1", json_data={"threshold": 1})
+
+    assert counter.methods == ["PATCH"]
+    _assert_ambiguous(excinfo.value, "PATCH")
+
+
+def test_sync_request_with_headers_post_5xx_is_marked_ambiguous():
+    counter = _Counter(_status(503))
+    client = _sync_client(counter)
+
+    with patch("time.sleep"):
+        with pytest.raises(OilPriceAPIError) as excinfo:
+            client.request_with_headers("POST", "/v1/subscriptions", json_data={"plan": "x"})
+
+    assert counter.methods == ["POST"]
+    _assert_ambiguous(excinfo.value, "POST")
+
+
+def test_async_post_5xx_is_marked_ambiguous():
+    counter = _Counter(_status(503))
+
+    async def scenario():
+        client = _async_client(counter)
+        with pytest.raises(OilPriceAPIError) as excinfo:
+            await client.request("POST", "/v1/subscriptions", json_data={"plan": "x"})
+        await client._client.aclose()
+        return excinfo.value
+
+    async def fake_sleep(seconds):
+        return None
+
+    with patch("asyncio.sleep", fake_sleep):
+        error = asyncio.run(scenario())
+
+    assert counter.methods == ["POST"]
+    _assert_ambiguous(error, "POST")
+
+
+def test_sync_and_async_produce_the_same_signal():
+    """The two clients must not diverge on this contract."""
+    sync_counter = _Counter(_status(503))
+    with patch("time.sleep"):
+        with pytest.raises(OilPriceAPIError) as sync_info:
+            _sync_client(sync_counter).request("POST", "/v1/subscriptions", json_data={})
+
+    async_counter = _Counter(_status(503))
+
+    async def scenario():
+        client = _async_client(async_counter)
+        with pytest.raises(OilPriceAPIError) as info:
+            await client.request("POST", "/v1/subscriptions", json_data={})
+        await client._client.aclose()
+        return info.value
+
+    async def fake_sleep(seconds):
+        return None
+
+    with patch("asyncio.sleep", fake_sleep):
+        async_error = asyncio.run(scenario())
+
+    assert type(sync_info.value) is type(async_error)
+    assert str(sync_info.value) == str(async_error)
+    assert sync_info.value.ambiguous_write == async_error.ambiguous_write is True
+
+
+# ---------------------------------------------------------------------------
+# (b) Everything that is NOT ambiguous must stay unmarked
+
+
+def test_sync_post_4xx_is_not_ambiguous():
+    """A 400 refused the write outright; nothing landed, nothing to reconcile."""
+    counter = _Counter(_status(400))
+    client = _sync_client(counter)
+
+    with patch("time.sleep"):
+        with pytest.raises(OilPriceAPIError) as excinfo:
+            client.request("POST", "/v1/subscriptions", json_data={"plan": "x"})
+
+    assert getattr(excinfo.value, "ambiguous_write", False) is False
+    assert "NOT retried" not in str(excinfo.value)
+
+
+def test_sync_post_429_is_not_ambiguous():
+    """429 is an outright refusal, so the write definitively did not happen."""
+    counter = _Counter(_status(429))
+    client = _sync_client(counter)
+
+    with patch("time.sleep"):
+        with pytest.raises(OilPriceAPIError) as excinfo:
+            client.request("POST", "/v1/subscriptions", json_data={"plan": "x"})
+
+    assert counter.methods == ["POST", "POST", "POST"], "429 is safe to replay"
+    assert getattr(excinfo.value, "ambiguous_write", False) is False
+
+
+def test_sync_get_5xx_is_not_ambiguous():
+    counter = _Counter(_status(503))
+    client = _sync_client(counter)
+
+    with patch("time.sleep"):
+        with pytest.raises(OilPriceAPIError) as excinfo:
+            client.request("GET", "/v1/prices/latest")
+
+    assert counter.methods == ["GET", "GET", "GET"]
+    assert getattr(excinfo.value, "ambiguous_write", False) is False
+
+
+def test_sync_post_5xx_with_idempotent_true_is_retried_and_not_ambiguous():
+    counter = _Counter(_status(503))
+    client = _sync_client(counter)
+
+    with patch("time.sleep"):
+        with pytest.raises(OilPriceAPIError) as excinfo:
+            client.request(
+                "POST", "/v1/diesel-prices/stations", json_data={}, idempotent=True
+            )
+
+    assert counter.methods == ["POST", "POST", "POST"]
+    assert getattr(excinfo.value, "ambiguous_write", False) is False