From 9461a808a6e9721437b38a3704c1cf5143a96790 Mon Sep 17 00:00:00 2001 From: lr90 Date: Sat, 10 Oct 2026 12:26:12 +0800 Subject: [PATCH 1/2] fix(sdk/python): wrap all transport errors and stop replaying writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two coupled transport defects. Leaked exceptions (#269): both the sync and async paths caught (ConnectError, TimeoutException, NetworkError). httpx.RemoteProtocolError — "server disconnected without sending a response", a routine production event — descends from ProtocolError, a *sibling* of NetworkError, so it escaped the SDK exception hierarchy and applications handling MemoriaError/MemoriaConnectionError missed the failure entirely. The same held for LocalProtocolError and ProxyError. Catch the httpx.TransportError base class instead: every subclass means no response was produced, which is exactly what MemoriaConnectionError describes, and new httpx subclasses can no longer slip through. Unsafe write replay (#265): POST/PATCH were retried on 502/503/504. No status code proves the upstream skipped the write — 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 — so retrying without an idempotency key risks committing one logical write twice. Non-idempotent retries are now opt-in via retry_unsafe_writes=True; the same rule applies to the newly-caught transport errors, so wrapping them does not introduce replay. ConnectError is still retried for every method because the request never reached the server, and idempotent methods are unchanged. Retry semantics are now documented as a per-method table. Fixes #265 Fixes #269 Co-Authored-By: Claude Opus 5 --- sdk/python/CHANGELOG.md | 14 +++ sdk/python/docs/python_sdk_design.md | 32 ++++-- sdk/python/src/memoria/_http.py | 83 +++++++++----- sdk/python/src/memoria/client.py | 18 ++- sdk/python/tests/unit/test_errors.py | 49 +++++++-- .../tests/unit/test_transport_errors.py | 104 ++++++++++++++++++ 6 files changed, 256 insertions(+), 44 deletions(-) create mode 100644 sdk/python/tests/unit/test_transport_errors.py diff --git a/sdk/python/CHANGELOG.md b/sdk/python/CHANGELOG.md index 9a0fcc7e..c6062255 100644 --- a/sdk/python/CHANGELOG.md +++ b/sdk/python/CHANGELOG.md @@ -2,6 +2,20 @@ ## [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. + ### 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. diff --git a/sdk/python/docs/python_sdk_design.md b/sdk/python/docs/python_sdk_design.md index a0ac87db..622e1248 100644 --- a/sdk/python/docs/python_sdk_design.md +++ b/sdk/python/docs/python_sdk_design.md @@ -586,13 +586,31 @@ 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. + +| Condition | Idempotent (GET/PUT/DELETE/…) | Non-idempotent (POST/PATCH) | +|-----------|-------------------------------|------------------------------| +| 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. --- diff --git a/sdk/python/src/memoria/_http.py b/sdk/python/src/memoria/_http.py index 5cb590af..6505988c 100644 --- a/sdk/python/src/memoria/_http.py +++ b/sdk/python/src/memoria/_http.py @@ -37,34 +37,45 @@ _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: +def _should_retry(method: str, status_code: int, retry_unsafe_writes: bool = False) -> bool: if method.upper() in _IDEMPOTENT_METHODS: 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) -> bool: +def _should_retry_network_error( + method: str, exc: Exception, retry_unsafe_writes: bool = False +) -> 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 methods: a read timeout, a dropped + connection or a disconnect-before-response on a POST 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 + if method.upper() in _IDEMPOTENT_METHODS: + return isinstance(exc, (httpx.TimeoutException, httpx.NetworkError, httpx.ProtocolError)) + return retry_unsafe_writes and isinstance( + exc, (httpx.TimeoutException, httpx.NetworkError, httpx.ProtocolError) + ) def _build_headers(api_key: str) -> dict[str, str]: @@ -127,11 +138,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: @@ -152,11 +165,12 @@ def _request( """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 +183,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 + ): time.sleep(_backoff(attempt)) continue raise MemoriaConnectionError(str(exc)) from exc @@ -187,7 +208,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) + and attempt < self._max_retries + ): time.sleep(_backoff(attempt)) continue @@ -227,9 +251,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 + ): await asyncio.sleep(_backoff(attempt)) continue raise MemoriaConnectionError(str(exc)) from exc @@ -245,7 +273,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) + and attempt < self._max_retries + ): await asyncio.sleep(_backoff(attempt)) continue diff --git a/sdk/python/src/memoria/client.py b/sdk/python/src/memoria/client.py index 64654827..6bd41558 100644 --- a/sdk/python/src/memoria/client.py +++ b/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), diff --git a/sdk/python/tests/unit/test_errors.py b/sdk/python/tests/unit/test_errors.py index 54cf03f8..5a2dbb64 100644 --- a/sdk/python/tests/unit/test_errors.py +++ b/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( diff --git a/sdk/python/tests/unit/test_transport_errors.py b/sdk/python/tests/unit/test_transport_errors.py new file mode 100644 index 00000000..23e1b76a --- /dev/null +++ b/sdk/python/tests/unit/test_transport_errors.py @@ -0,0 +1,104 @@ +"""Unit tests for transport-error mapping and write-retry safety. + +Every httpx failure that produced no response must reach callers as a +MemoriaConnectionError. RemoteProtocolError ("server disconnected without +sending a response") and ProxyError are siblings of NetworkError rather than +subclasses, so catching hand-picked subclasses let them escape the SDK's +exception hierarchy. +""" + +from __future__ import annotations + +import httpx +import pytest +from pytest_httpx import HTTPXMock + +from memoria import AsyncMemoriaClient, MemoriaClient, MemoriaConnectionError +from tests.conftest import API_KEY, BASE_URL, MEMORY_STUB + +# Every httpx.TransportError branch: no response was produced in any of them. +TRANSPORT_ERRORS = [ + httpx.ConnectError, + httpx.ConnectTimeout, + httpx.ReadTimeout, + httpx.ReadError, + httpx.WriteError, + httpx.RemoteProtocolError, + httpx.LocalProtocolError, + httpx.ProxyError, +] + + +@pytest.fixture +def client() -> MemoriaClient: + return MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=0) + + +@pytest.mark.parametrize("error", TRANSPORT_ERRORS, ids=lambda e: e.__name__) +def test_transport_error_is_wrapped_on_get( + httpx_mock: HTTPXMock, client: MemoriaClient, error: type[httpx.TransportError] +) -> None: + httpx_mock.add_exception(error("boom")) + with pytest.raises(MemoriaConnectionError) as raised: + client.memories.list() + assert isinstance(raised.value.__cause__, error) + + +@pytest.mark.parametrize("error", TRANSPORT_ERRORS, ids=lambda e: e.__name__) +def test_transport_error_is_wrapped_on_post( + httpx_mock: HTTPXMock, client: MemoriaClient, error: type[httpx.TransportError] +) -> None: + httpx_mock.add_exception(error("boom")) + with pytest.raises(MemoriaConnectionError) as raised: + client.memories.store(content="x") + assert isinstance(raised.value.__cause__, error) + + +@pytest.mark.parametrize("error", TRANSPORT_ERRORS, ids=lambda e: e.__name__) +@pytest.mark.asyncio +async def test_transport_error_is_wrapped_async( + httpx_mock: HTTPXMock, error: type[httpx.TransportError] +) -> None: + httpx_mock.add_exception(error("boom")) + async with AsyncMemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=0) as client: + with pytest.raises(MemoriaConnectionError) as raised: + await client.memories.store(content="x") + assert isinstance(raised.value.__cause__, error) + + +def test_disconnect_on_post_is_not_replayed_by_default(httpx_mock: HTTPXMock) -> None: + # The server may have committed before dropping the connection, so a POST + # must not be replayed just because the response was lost. + httpx_mock.add_exception(httpx.RemoteProtocolError("server disconnected")) + client = MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) + with pytest.raises(MemoriaConnectionError): + client.memories.store(content="x") + assert len(httpx_mock.get_requests()) == 1 + + +def test_disconnect_on_post_is_replayed_when_opted_in(httpx_mock: HTTPXMock) -> None: + httpx_mock.add_exception(httpx.RemoteProtocolError("server disconnected")) + httpx_mock.add_response(json=MEMORY_STUB) + client = MemoriaClient( + base_url=BASE_URL, api_key=API_KEY, max_retries=3, retry_unsafe_writes=True + ) + assert client.memories.store(content="x").memory_id == "mem_abc123" + assert len(httpx_mock.get_requests()) == 2 + + +def test_disconnect_on_get_is_retried(httpx_mock: HTTPXMock) -> None: + # Repeating an idempotent request cannot create anything, so recovery is safe. + httpx_mock.add_exception(httpx.RemoteProtocolError("server disconnected")) + httpx_mock.add_response(json={"items": [], "next_cursor": None}) + client = MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) + assert client.memories.list().items == [] + assert len(httpx_mock.get_requests()) == 2 + + +def test_connect_error_on_post_is_retried(httpx_mock: HTTPXMock) -> None: + # The request never reached the server, so it cannot have committed. + httpx_mock.add_exception(httpx.ConnectError("refused")) + httpx_mock.add_response(json=MEMORY_STUB) + client = MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) + assert client.memories.store(content="x").memory_id == "mem_abc123" + assert len(httpx_mock.get_requests()) == 2 From 579ad90a8975e383e6fa61a9cb3ad72728f65063 Mon Sep 17 00:00:00 2001 From: lr90 Date: Sat, 10 Oct 2026 13:25:29 +0800 Subject: [PATCH 2/2] fix(sdk): treat memories.correct as non-idempotent despite its PUT verb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to review on this PR; reproduced before fixing. Retry eligibility was decided purely by HTTP verb, and PUT is in the idempotent set, so retry_unsafe_writes=False did not protect memories.correct() — which sends PUT /v1/memories/{id}/correct. The server looks the memory up, mints a new UUID-backed replacement and supersedes the original, so a replay either 404s on the already-superseded memory or creates a second replacement. Worse, the ProtocolError retry branch added in this PR *expanded* the exposure: a disconnect on that PUT previously escaped without replay and would now have been reissued automatically. Eligibility is now per operation: _request/_arequest take an optional `idempotent` override that defaults to the method-based classification, and the two correct() call sites pass idempotent=False. Verified one attempt per logical correction for 504 and RemoteProtocolError, sync and async; retry_unsafe_writes=True still replays; DELETE and other genuinely idempotent methods keep their behavior. Co-Authored-By: Claude Opus 5 --- sdk/python/CHANGELOG.md | 3 + sdk/python/docs/python_sdk_design.md | 8 ++- sdk/python/src/memoria/_http.py | 51 ++++++++++---- sdk/python/src/memoria/resources/memories.py | 11 +++- .../tests/unit/test_transport_errors.py | 66 ++++++++++++++++++- 5 files changed, 122 insertions(+), 17 deletions(-) diff --git a/sdk/python/CHANGELOG.md b/sdk/python/CHANGELOG.md index c6062255..da16335b 100644 --- a/sdk/python/CHANGELOG.md +++ b/sdk/python/CHANGELOG.md @@ -15,6 +15,9 @@ 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, diff --git a/sdk/python/docs/python_sdk_design.md b/sdk/python/docs/python_sdk_design.md index 622e1248..f85955a7 100644 --- a/sdk/python/docs/python_sdk_design.md +++ b/sdk/python/docs/python_sdk_design.md @@ -594,7 +594,13 @@ 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. -| Condition | Idempotent (GET/PUT/DELETE/…) | Non-idempotent (POST/PATCH) | +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` | diff --git a/sdk/python/src/memoria/_http.py b/sdk/python/src/memoria/_http.py index 6505988c..596e7799 100644 --- a/sdk/python/src/memoria/_http.py +++ b/sdk/python/src/memoria/_http.py @@ -54,28 +54,51 @@ _IDEMPOTENT_METHODS = {"GET", "HEAD", "PUT", "DELETE", "OPTIONS", "TRACE"} -def _should_retry(method: str, status_code: int, retry_unsafe_writes: bool = False) -> 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 retry_unsafe_writes and status_code in _RETRY_STATUS_UNSAFE def _should_retry_network_error( - method: str, exc: Exception, retry_unsafe_writes: bool = False + method: str, + exc: Exception, + retry_unsafe_writes: bool = False, + idempotent: bool | None = None, ) -> bool: - """ConnectError is safe to retry for any method (server never received the request). + """ConnectError is safe to retry for anything (the server never received it). - Everything else is only safe for idempotent methods: a read timeout, a dropped - connection or a disconnect-before-response on a POST may all mean the server + 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 method.upper() in _IDEMPOTENT_METHODS: - return isinstance(exc, (httpx.TimeoutException, httpx.NetworkError, httpx.ProtocolError)) - return retry_unsafe_writes and isinstance( + 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]: @@ -161,6 +184,7 @@ def _request( *, params: dict[str, Any] | None = None, json: Any = None, + idempotent: bool | None = None, ) -> Any: """Execute a synchronous HTTP request with retry logic. @@ -191,7 +215,7 @@ def _request( except httpx.TransportError as exc: last_exc = exc if attempt < self._max_retries and _should_retry_network_error( - method, exc, self._retry_unsafe_writes + method, exc, self._retry_unsafe_writes, idempotent ): time.sleep(_backoff(attempt)) continue @@ -209,7 +233,7 @@ def _request( return resp.text if ( - _should_retry(method, resp.status_code, self._retry_unsafe_writes) + _should_retry(method, resp.status_code, self._retry_unsafe_writes, idempotent) and attempt < self._max_retries ): time.sleep(_backoff(attempt)) @@ -233,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. @@ -256,7 +281,7 @@ async def _arequest( except httpx.TransportError as exc: last_exc = exc if attempt < self._max_retries and _should_retry_network_error( - method, exc, self._retry_unsafe_writes + method, exc, self._retry_unsafe_writes, idempotent ): await asyncio.sleep(_backoff(attempt)) continue @@ -274,7 +299,7 @@ async def _arequest( return resp.text if ( - _should_retry(method, resp.status_code, self._retry_unsafe_writes) + _should_retry(method, resp.status_code, self._retry_unsafe_writes, idempotent) and attempt < self._max_retries ): await asyncio.sleep(_backoff(attempt)) diff --git a/sdk/python/src/memoria/resources/memories.py b/sdk/python/src/memoria/resources/memories.py index 0c1052b5..1cfb6b91 100644 --- a/sdk/python/src/memoria/resources/memories.py +++ b/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( diff --git a/sdk/python/tests/unit/test_transport_errors.py b/sdk/python/tests/unit/test_transport_errors.py index 23e1b76a..20987d6c 100644 --- a/sdk/python/tests/unit/test_transport_errors.py +++ b/sdk/python/tests/unit/test_transport_errors.py @@ -13,7 +13,12 @@ import pytest from pytest_httpx import HTTPXMock -from memoria import AsyncMemoriaClient, MemoriaClient, MemoriaConnectionError +from memoria import ( + AsyncMemoriaClient, + MemoriaClient, + MemoriaConnectionError, + MemoriaServerError, +) from tests.conftest import API_KEY, BASE_URL, MEMORY_STUB # Every httpx.TransportError branch: no response was produced in any of them. @@ -102,3 +107,62 @@ def test_connect_error_on_post_is_retried(httpx_mock: HTTPXMock) -> None: client = MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) assert client.memories.store(content="x").memory_id == "mem_abc123" assert len(httpx_mock.get_requests()) == 2 + + +# ── PUT /v1/memories/{id}/correct is non-idempotent despite the verb ───────── +# 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. + + +@pytest.mark.parametrize("status", [502, 503, 504]) +def test_correct_is_not_retried_on_gateway_error(httpx_mock: HTTPXMock, status: int) -> None: + httpx_mock.add_response(status_code=status, json={"detail": "gateway"}) + client = MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) + with pytest.raises(MemoriaServerError): + client.memories.correct("m1", new_content="new") + assert len(httpx_mock.get_requests()) == 1 + + +def test_correct_is_not_retried_on_disconnect(httpx_mock: HTTPXMock) -> None: + httpx_mock.add_exception(httpx.RemoteProtocolError("server disconnected")) + client = MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) + with pytest.raises(MemoriaConnectionError): + client.memories.correct("m1", new_content="new") + assert len(httpx_mock.get_requests()) == 1 + + +@pytest.mark.asyncio +async def test_correct_is_not_retried_async(httpx_mock: HTTPXMock) -> None: + httpx_mock.add_exception(httpx.RemoteProtocolError("server disconnected")) + async with AsyncMemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) as client: + with pytest.raises(MemoriaConnectionError): + await client.memories.correct("m1", new_content="new") + assert len(httpx_mock.get_requests()) == 1 + + +@pytest.mark.asyncio +async def test_correct_gateway_error_not_retried_async(httpx_mock: HTTPXMock) -> None: + httpx_mock.add_response(status_code=504, json={"detail": "gateway"}) + async with AsyncMemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) as client: + with pytest.raises(MemoriaServerError): + await client.memories.correct("m1", new_content="new") + assert len(httpx_mock.get_requests()) == 1 + + +def test_correct_is_retried_when_opted_in(httpx_mock: HTTPXMock) -> None: + httpx_mock.add_exception(httpx.RemoteProtocolError("server disconnected")) + httpx_mock.add_response(json=MEMORY_STUB) + client = MemoriaClient( + base_url=BASE_URL, api_key=API_KEY, max_retries=3, retry_unsafe_writes=True + ) + client.memories.correct("m1", new_content="new") + assert len(httpx_mock.get_requests()) == 2 + + +def test_genuinely_idempotent_delete_is_still_retried(httpx_mock: HTTPXMock) -> None: + # The override is per-operation: DELETE keeps its method-based default. + httpx_mock.add_exception(httpx.RemoteProtocolError("server disconnected")) + httpx_mock.add_response(status_code=204) + client = MemoriaClient(base_url=BASE_URL, api_key=API_KEY, max_retries=3) + client.memories.delete("m1") + assert len(httpx_mock.get_requests()) == 2