fix(sdk): wrap all transport errors and stop replaying writes

sdk/python/CHANGELOG.md

@@ -2,6 +2,23 @@
 
 ## [Unreleased]
 
+### Fixed
+- Every `httpx.TransportError` now surfaces as `MemoriaConnectionError` with the original
+  exception preserved as `__cause__`. `RemoteProtocolError` ("server disconnected without
+  sending a response"), `LocalProtocolError` and `ProxyError` are siblings of `NetworkError`
+  rather than subclasses, so they previously escaped the SDK exception hierarchy entirely.
+
+### Changed
+- **Breaking (behavioral):** non-idempotent requests (POST/PATCH) are no longer retried on
+  502/503/504 or on transport errors other than `ConnectError`. None of those prove the server
+  skipped the write — a gateway can return 504 while the upstream commits — so retrying without
+  an idempotency key risked duplicate writes. Pass `retry_unsafe_writes=True` to either client
+  to restore the previous behavior. Idempotent methods are unchanged, and `ConnectError` (where
+  the request never reached the server) is still retried for every method.
+  Retry eligibility is per operation rather than purely per HTTP verb: `memories.correct()`
+  uses PUT but is treated as non-idempotent, because the server mints a replacement record and
+  supersedes the original, so a replay either 404s or creates a second replacement.
+
 ### Added
 - Sync and async `memories.query()` for exact structured filtering through the REST API,
   including scalar `extra_metadata`, subject, type, session, trust tier, branch, and pagination.

sdk/python/docs/python_sdk_design.md

@@ -586,13 +586,37 @@ Personal mode users won't see this. SDK exposes `MemoriaForbiddenError` for grou
 
 ### Retry Policy
 
-| Condition | Retry? |
-|-----------|--------|
-| 5xx / network errors | Yes — exponential backoff (`max_retries=3`) |
-| 401 | No |
-| 403 | No |
-| 422 | No (caller bug) |
-| Other 4xx | No |
+Retry eligibility depends on whether repeating the request can create something.
+Idempotent methods (GET/HEAD/PUT/DELETE/OPTIONS/TRACE) are safe to repeat;
+non-idempotent ones (POST/PATCH) are not, and no status code or transport error
+proves the server did *not* already commit the write — a gateway can return 504
+while the upstream keeps going and commits, and a connection can drop after the
+write but before the response. Without an idempotency key there is no way to
+retry such a write safely, so by default the SDK does not.
+
+Eligibility is per *operation*, not purely per HTTP verb: `memories.correct()`
+uses `PUT /v1/memories/{id}/correct` but is **not** idempotent — the server
+mints a replacement record and supersedes the original, so a replay either 404s
+on the already-superseded memory or creates a second replacement. It is
+classified with the non-idempotent column below.
+
+| Condition | Idempotent (GET/PUT/DELETE/…) | Non-idempotent (POST/PATCH, `memories.correct`) |
+|-----------|-------------------------------|------------------------------|
+| 500 | Yes — exponential backoff (`max_retries=3`) | No |
+| 502 / 503 / 504 | Yes | No — opt in with `retry_unsafe_writes=True` |
+| `ConnectError` (never reached the server) | Yes | Yes |
+| Other transport errors (timeout, dropped connection, disconnect before response, proxy failure) | Yes | No — opt in with `retry_unsafe_writes=True` |
+| 401 / 403 / 422 / other 4xx | No | No |
+
+Every `httpx.TransportError` — including `RemoteProtocolError` ("server
+disconnected without sending a response"), which is a sibling of `NetworkError`
+rather than a subclass — is surfaced as `MemoriaConnectionError`, with the
+original exception preserved as `__cause__`.
+
+`retry_unsafe_writes=True` trades at-most-once for resilience: a write whose
+response was lost may be committed twice. Note that `memories.store` is
+additionally protected server-side by near-duplicate detection, which supersedes
+rather than duplicates identical content; other write endpoints are not.
 
 ---
 

sdk/python/src/memoria/_http.py

@@ -37,34 +37,68 @@
 _DEFAULT_TIMEOUT = 30.0
 _DEFAULT_MAX_RETRIES = 3
 
-# 502/503/504 are gateway-level errors: the upstream server almost certainly
-# did not process the request, so retrying is safe for any HTTP method.
-# 500 is ambiguous for non-idempotent methods: the server may have processed
-# the request and written data before returning the error. Retrying a POST/PATCH
-# on 500 could produce duplicate writes (e.g. duplicate memories), so we exclude
-# it for non-safe methods.
+# For idempotent methods, repeating the request cannot create anything, so every
+# 5xx that suggests a transient fault is retried.
+#
+# For non-idempotent methods there is no status code that proves the server did
+# *not* process the request: a gateway can return 504 while the upstream keeps
+# going and commits, and 502/503 can follow a commit if the upstream dies right
+# after writing. Without an idempotency key there is no way to retry such a
+# write safely, so by default we do not — the caller sees the error and decides.
+# Callers who would rather risk a duplicate than surface a transient failure can
+# opt in with ``retry_unsafe_writes=True``.
 _RETRY_STATUS_SAFE = {500, 502, 503, 504}       # idempotent methods: GET/HEAD/PUT/DELETE
-_RETRY_STATUS_UNSAFE = {502, 503, 504}           # non-idempotent methods: POST/PATCH
+_RETRY_STATUS_UNSAFE = {502, 503, 504}           # non-idempotent methods, opt-in only
 
 # HTTP methods where repeating the request is guaranteed not to cause side-effects.
 _IDEMPOTENT_METHODS = {"GET", "HEAD", "PUT", "DELETE", "OPTIONS", "TRACE"}
 
 
-def _should_retry(method: str, status_code: int) -> bool:
-    if method.upper() in _IDEMPOTENT_METHODS:
+def _is_idempotent(method: str, idempotent: bool | None) -> bool:
+    """Whether repeating this request is safe.
+
+    The HTTP verb is only a default: an endpoint can be non-idempotent despite
+    using an idempotent-looking method, so callers may override. PUT
+    /v1/memories/{id}/correct is the case in point — the server mints a new
+    record and supersedes the old one, so a replay either 404s on the
+    already-superseded memory or creates a second replacement.
+    """
+    if idempotent is not None:
+        return idempotent
+    return method.upper() in _IDEMPOTENT_METHODS
+
+
+def _should_retry(
+    method: str,
+    status_code: int,
+    retry_unsafe_writes: bool = False,
+    idempotent: bool | None = None,
+) -> bool:
+    if _is_idempotent(method, idempotent):
         return status_code in _RETRY_STATUS_SAFE
-    return status_code in _RETRY_STATUS_UNSAFE
+    return retry_unsafe_writes and status_code in _RETRY_STATUS_UNSAFE
+
 
+def _should_retry_network_error(
+    method: str,
+    exc: Exception,
+    retry_unsafe_writes: bool = False,
+    idempotent: bool | None = None,
+) -> bool:
+    """ConnectError is safe to retry for anything (the server never received it).
 
-def _should_retry_network_error(method: str, exc: Exception) -> bool:
-    """ConnectError is safe to retry for any method (server never received the request).
-    TimeoutException is only safe for idempotent methods — a read timeout on POST may
-    mean the server processed the request but the response was lost in transit."""
+    Everything else is only safe for idempotent operations: a read timeout, a
+    dropped connection or a disconnect-before-response may all mean the server
+    processed the request and only the response was lost.
+    """
     if isinstance(exc, httpx.ConnectError):
         return True
-    if isinstance(exc, (httpx.TimeoutException, httpx.NetworkError)):
-        return method.upper() in _IDEMPOTENT_METHODS
-    return False
+    retryable = isinstance(
+        exc, (httpx.TimeoutException, httpx.NetworkError, httpx.ProtocolError)
+    )
+    if _is_idempotent(method, idempotent):
+        return retryable
+    return retry_unsafe_writes and retryable
 
 
 def _build_headers(api_key: str) -> dict[str, str]:
@@ -127,11 +161,13 @@ def __init__(
         api_key: str,
         timeout: float = _DEFAULT_TIMEOUT,
         max_retries: int = _DEFAULT_MAX_RETRIES,
+        retry_unsafe_writes: bool = False,
     ) -> None:
         self._base_url = base_url.rstrip("/")
         self._api_key = api_key
         self._timeout = timeout
         self._max_retries = max_retries
+        self._retry_unsafe_writes = retry_unsafe_writes
         self._headers = _build_headers(api_key)
 
     def _url(self, path: str) -> str:
@@ -148,15 +184,17 @@ def _request(
         *,
         params: dict[str, Any] | None = None,
         json: Any = None,
+        idempotent: bool | None = None,
     ) -> Any:
         """Execute a synchronous HTTP request with retry logic.
 
         Retry policy:
-          - 502/503/504: always retried (gateway errors; server did not process request)
-          - 500: only retried for idempotent methods (GET/HEAD/PUT/DELETE) to avoid
-            duplicate writes on non-idempotent endpoints like POST memories.store
-          - ConnectError: always retried (connection never reached server)
-          - TimeoutException/NetworkError: only retried for idempotent methods
+          - idempotent methods (GET/HEAD/PUT/DELETE): 500/502/503/504 and any
+            transport error are retried
+          - non-idempotent methods (POST/PATCH): not retried unless the client was
+            built with ``retry_unsafe_writes=True``, because no status code or
+            transport error proves the server did not already commit the write
+          - ConnectError: always retried; the request never reached the server
         """
         url = self._url(path)
         last_exc: Exception | None = None
@@ -169,9 +207,16 @@ def _request(
                     json=json,
                     timeout=self._timeout,
                 )
-            except (httpx.ConnectError, httpx.TimeoutException, httpx.NetworkError) as exc:
+            # httpx.TransportError covers every failure that produced no response —
+            # including ProtocolError/RemoteProtocolError ("server disconnected
+            # without sending a response") and ProxyError, which are siblings of
+            # NetworkError rather than subclasses. Catching the base class keeps
+            # new httpx subclasses from escaping the SDK hierarchy.
+            except httpx.TransportError as exc:
                 last_exc = exc
-                if attempt < self._max_retries and _should_retry_network_error(method, exc):
+                if attempt < self._max_retries and _should_retry_network_error(
+                    method, exc, self._retry_unsafe_writes, idempotent
+                ):
                     time.sleep(_backoff(attempt))
                     continue
                 raise MemoriaConnectionError(str(exc)) from exc
@@ -187,7 +232,10 @@ def _request(
                 except Exception:
                     return resp.text
 
-            if _should_retry(method, resp.status_code) and attempt < self._max_retries:
+            if (
+                _should_retry(method, resp.status_code, self._retry_unsafe_writes, idempotent)
+                and attempt < self._max_retries
+            ):
                 time.sleep(_backoff(attempt))
                 continue
 
@@ -209,6 +257,7 @@ async def _arequest(
         *,
         params: dict[str, Any] | None = None,
         json: Any = None,
+        idempotent: bool | None = None,
     ) -> Any:
         """Execute an asynchronous HTTP request with retry logic.
 
@@ -227,9 +276,13 @@ async def _arequest(
                     json=json,
                     timeout=self._timeout,
                 )
-            except (httpx.ConnectError, httpx.TimeoutException, httpx.NetworkError) as exc:
+            # See the sync path: catch the TransportError base class so protocol and
+            # proxy failures cannot escape as raw httpx exceptions.
+            except httpx.TransportError as exc:
                 last_exc = exc
-                if attempt < self._max_retries and _should_retry_network_error(method, exc):
+                if attempt < self._max_retries and _should_retry_network_error(
+                    method, exc, self._retry_unsafe_writes, idempotent
+                ):
                     await asyncio.sleep(_backoff(attempt))
                     continue
                 raise MemoriaConnectionError(str(exc)) from exc
@@ -245,7 +298,10 @@ async def _arequest(
                 except Exception:
                     return resp.text
 
-            if _should_retry(method, resp.status_code) and attempt < self._max_retries:
+            if (
+                _should_retry(method, resp.status_code, self._retry_unsafe_writes, idempotent)
+                and attempt < self._max_retries
+            ):
                 await asyncio.sleep(_backoff(attempt))
                 continue
 

sdk/python/src/memoria/client.py

@@ -56,8 +56,15 @@ def __init__(
         *,
         timeout: float = _DEFAULT_TIMEOUT,
         max_retries: int = _DEFAULT_MAX_RETRIES,
+        retry_unsafe_writes: bool = False,
     ) -> None:
-        super().__init__(base_url, api_key, timeout=timeout, max_retries=max_retries)
+        super().__init__(
+            base_url,
+            api_key,
+            timeout=timeout,
+            max_retries=max_retries,
+            retry_unsafe_writes=retry_unsafe_writes,
+        )
         self._http = httpx.Client(
             headers=_build_headers(api_key),
             timeout=httpx.Timeout(timeout),
@@ -135,8 +142,15 @@ def __init__(
         *,
         timeout: float = _DEFAULT_TIMEOUT,
         max_retries: int = _DEFAULT_MAX_RETRIES,
+        retry_unsafe_writes: bool = False,
     ) -> None:
-        super().__init__(base_url, api_key, timeout=timeout, max_retries=max_retries)
+        super().__init__(
+            base_url,
+            api_key,
+            timeout=timeout,
+            max_retries=max_retries,
+            retry_unsafe_writes=retry_unsafe_writes,
+        )
         self._ahttp = httpx.AsyncClient(
             headers=_build_headers(api_key),
             timeout=httpx.Timeout(timeout),

sdk/python/src/memoria/resources/memories.py

@@ -492,7 +492,11 @@ def correct(
         branch: str | None = None,
     ) -> Memory:
         body = _strip_none({"new_content": new_content, "reason": reason, "branch": branch})
-        data = self._client._request("PUT", f"/v1/memories/{id}/correct", json=body)
+        # PUT, but not idempotent: the server mints a replacement record and
+        # supersedes the original, so a replay 404s or creates a second one.
+        data = self._client._request(
+            "PUT", f"/v1/memories/{id}/correct", json=body, idempotent=False
+        )
         return Memory.from_dict(data)
 
     def correct_by_query(
@@ -787,7 +791,10 @@ async def correct(
         branch: str | None = None,
     ) -> Memory:
         body = _strip_none({"new_content": new_content, "reason": reason, "branch": branch})
-        data = await self._client._arequest("PUT", f"/v1/memories/{id}/correct", json=body)
+        # PUT, but not idempotent — see the sync path.
+        data = await self._client._arequest(
+            "PUT", f"/v1/memories/{id}/correct", json=body, idempotent=False
+        )
         return Memory.from_dict(data)
 
     async def correct_by_query(

sdk/python/tests/unit/test_errors.py

@@ -20,6 +20,13 @@ def client_with_retry() -> MemoriaClient:
     return MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=2)
 
 
+@pytest.fixture
+def client_retrying_writes() -> MemoriaClient:
+    return MemoriaClient(
+        base_url=BASE_URL, api_key=API_KEY, max_retries=2, retry_unsafe_writes=True
+    )
+
+
 def test_500_raises_server_error(httpx_mock: HTTPXMock, client_no_retry: MemoriaClient) -> None:
     httpx_mock.add_response(status_code=500, json={"detail": "internal error"})
     with pytest.raises(MemoriaServerError) as exc:
@@ -38,24 +45,48 @@ def test_500_on_post_not_retried(
     assert len(httpx_mock.get_requests()) == 1
 
 
-def test_502_on_post_retried_then_succeeds(
-    httpx_mock: HTTPXMock, client_with_retry: MemoriaClient
+@pytest.mark.parametrize("status", [502, 503, 504])
+def test_gateway_error_on_post_not_retried_by_default(
+    httpx_mock: HTTPXMock, client_with_retry: MemoriaClient, status: int
 ) -> None:
-    # 502/503/504 are gateway errors; POST is safe to retry (server did not process).
-    httpx_mock.add_response(status_code=502, json={"detail": "bad gateway"})
-    httpx_mock.add_response(status_code=502, json={"detail": "bad gateway"})
+    # A gateway error does not prove the upstream skipped the write: it can fire
+    # after a commit (notably 504). Without an idempotency key the only safe
+    # default for a non-idempotent method is to surface the error.
+    httpx_mock.add_response(status_code=status, json={"detail": "gateway"})
+    with pytest.raises(MemoriaServerError):
+        client_with_retry.memories.store(content="x")
+    assert len(httpx_mock.get_requests()) == 1
+
+
+@pytest.mark.parametrize("status", [502, 503, 504])
+def test_gateway_error_on_post_retried_when_opted_in(
+    httpx_mock: HTTPXMock, client_retrying_writes: MemoriaClient, status: int
+) -> None:
+    httpx_mock.add_response(status_code=status, json={"detail": "gateway"})
     httpx_mock.add_response(json=MEMORY_STUB)
-    mem = client_with_retry.memories.store(content="x")
+    mem = client_retrying_writes.memories.store(content="x")
     assert mem.memory_id == "mem_abc123"
+    assert len(httpx_mock.get_requests()) == 2
 
 
-def test_502_exhausted_retries_raises(
-    httpx_mock: HTTPXMock, client_with_retry: MemoriaClient
+@pytest.mark.parametrize("status", [500, 502, 503, 504])
+def test_gateway_error_on_get_still_retried(
+    httpx_mock: HTTPXMock, client_with_retry: MemoriaClient, status: int
+) -> None:
+    # Idempotent methods are unaffected: repeating a GET cannot create anything.
+    httpx_mock.add_response(status_code=status, json={"detail": "gateway"})
+    httpx_mock.add_response(json={"items": [], "next_cursor": None})
+    assert client_with_retry.memories.list().items == []
+    assert len(httpx_mock.get_requests()) == 2
+
+
+def test_gateway_error_exhausted_retries_raises(
+    httpx_mock: HTTPXMock, client_retrying_writes: MemoriaClient
 ) -> None:
     for _ in range(3):  # max_retries=2 → 3 total attempts
         httpx_mock.add_response(status_code=502, json={"detail": "bad gateway"})
     with pytest.raises(MemoriaServerError):
-        client_with_retry.memories.store(content="x")
+        client_retrying_writes.memories.store(content="x")
 
 
 def test_unknown_4xx_raises_api_error(