diff --git a/CHANGELOG.md b/CHANGELOG.md index 66a8ffc..a1c0b09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Automatic retry no longer repeats non-idempotent requests. The transport + retried 429/503/504 and network errors for every method, so a lost response to + a POST/PATCH/DELETE that the server had already applied would duplicate a + create or re-run a bound action. Some bound actions in this API take no + arguments and mutate on every call, so a repeat is never harmless. Retries now + require either a read-only method or an explicit `Idempotency-Key`; otherwise + the error surfaces so the caller can decide. **This is a behaviour change**: a + transient 503 on a write that previously succeeded after a silent retry will + now raise. Pass `idempotency_key=` to opt back in. + +- The sdist no longer sweeps in nested checkouts. `[tool.hatch.build.targets.sdist]` + listed patterns like `docs/` and `src/` without a leading slash, so they matched at + any depth — and `.claude/worktrees/agent-*/` holds full copies of the repo. The + 0.7.0 sdist picked up 1350 extra files that way (2 MiB instead of 434 KiB), + including older copies of `docs/configuration.md` and `docs/multi-company.md` from + before example identifiers were replaced with placeholders. The wheel was never + affected, since it builds from `packages`, and `git ls-files` was clean — only + inspecting the built artifact showed it. Patterns are now anchored to the project + root. + - `build_url` now validates `entity_set_name` and `record_id` as single URL path components, rejecting raw `/`, `\`, `?`, `#` and the `.`/`..` segments. Both values are spliced directly into the request path, so a record key containing diff --git a/pyproject.toml b/pyproject.toml index f633fcd..ecd0b38 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -108,17 +108,25 @@ packages = ["src/bcli", "src/bcli_cli", "src/bcli_mcp"] "packs" = "bcli/packs/_builtin" [tool.hatch.build.targets.sdist] +# Leading slashes anchor these to the project root. Without them the patterns +# match at any depth, so a nested checkout — .claude/worktrees/agent-*/ holds +# full copies of the repo — had its own docs/, src/ and tests/ swept into the +# sdist: 1350 extra files, including pre-sanitisation docs still carrying real +# tenant identifiers. The wheel was unaffected (it builds from `packages`), and +# `git ls-files` was clean, so only inspecting the built artifact caught it. +# If you add a pattern here, anchor it, then verify with: +# uv build && tar -tzf dist/bc_cli-*.tar.gz | awk -F/ 'NF>1{print $2}' | sort -u include = [ - "src/", - "packs/", - "tests/", - "examples/", - "docs/", - "README.md", - "LICENSE", - "NOTICE", - "CHANGELOG.md", - "pyproject.toml", + "/src", + "/packs", + "/tests", + "/examples", + "/docs", + "/README.md", + "/LICENSE", + "/NOTICE", + "/CHANGELOG.md", + "/pyproject.toml", ] [tool.pytest.ini_options] diff --git a/src/bcli/client/_transport.py b/src/bcli/client/_transport.py index d0e3576..0d51920 100644 --- a/src/bcli/client/_transport.py +++ b/src/bcli/client/_transport.py @@ -41,6 +41,13 @@ # Retryable status codes _RETRYABLE = {429, 503, 504} +#: Methods that can be repeated without applying an effect twice. Deliberately +#: excludes DELETE and PUT: both are idempotent by HTTP semantics, but a repeat +#: here surfaces as a 404 or overwrites a concurrent change, and neither is what +#: an automatic retry should decide on the caller's behalf. Pass an +#: ``idempotency_key`` to opt a mutation back into retrying. +_IDEMPOTENT_METHODS = frozenset({"GET", "HEAD"}) + DEFAULT_TIMEOUT = 60 DEFAULT_MAX_RETRIES = 3 INITIAL_BACKOFF = 1.0 # seconds @@ -144,6 +151,21 @@ async def _request( backoff = INITIAL_BACKOFF t0 = time.monotonic() + # Repeating a request is only safe when it cannot apply an effect twice. + # A read can always be repeated. A POST / PATCH / DELETE cannot: the + # server may already have applied it when the response was lost, so a + # retry duplicates a create or re-runs a business action. Some bound + # actions in this API take no arguments and mutate on every invocation, + # so there is no such thing as a harmless repeat — one 503 from a gateway + # could recalculate twice with nothing in the log to say so. + # + # An Idempotency-Key re-enables retry, because that is what lets a + # gateway (or a future server-side implementation) collapse the + # duplicate. Without one, the error surfaces instead: a visible + # transient failure the caller can retry deliberately beats an invisible + # double-write. + retry_safe = method.upper() in _IDEMPOTENT_METHODS or idempotency_key is not None + for attempt in range(self._max_retries + 1): try: auth_headers = await self._inject_auth() @@ -194,8 +216,8 @@ async def _request( bc_message, correlation_id = _parse_bc_error(response) status = response.status_code - # Retry on retryable errors - if status in _RETRYABLE and attempt < self._max_retries: + # Retry on retryable errors — but only if repeating is safe. + if status in _RETRYABLE and attempt < self._max_retries and retry_safe: retry_after = _get_retry_after(response) wait = retry_after if retry_after else backoff logger.warning( @@ -238,7 +260,10 @@ async def _request( httpx.RemoteProtocolError, ) as e: last_error = e - if attempt < self._max_retries: + # The most dangerous case for a mutation: the request may have + # reached the server and been applied before the connection + # dropped, so there is no way to know a retry is safe. + if attempt < self._max_retries and retry_safe: logger.warning( "Network error on %s %s: %s, retrying in %.1fs", method, url, e, backoff, diff --git a/tests/test_client/test_retry_idempotency.py b/tests/test_client/test_retry_idempotency.py new file mode 100644 index 0000000..6b16c56 --- /dev/null +++ b/tests/test_client/test_retry_idempotency.py @@ -0,0 +1,124 @@ +"""Automatic retry must not silently re-run a non-idempotent request. + +The transport retried 429/503/504 and network errors for *every* method. For a +GET that is free. For a POST, PATCH or DELETE it is not: the server may already +have applied the request when the response was lost, so a retry duplicates a +create or re-runs a business action. + +That is not theoretical for this SDK. Bound actions like the LLP-utilisation +recalculation take no arguments and mutate on every invocation — there is no +such thing as a harmless repeat. A single 503 from a gateway could recalculate +twice, and nothing in the log would say so. + +So retries are now allowed only when repeating the request is safe: + +* the method is read-only (GET / HEAD), or +* the caller supplied an ``Idempotency-Key``, which is what lets a gateway (or a + future server-side implementation) collapse the duplicate. + +Otherwise the error surfaces. A visible transient failure the caller can retry +deliberately is strictly better than an invisible double-write. +""" + +from __future__ import annotations + +import httpx +import pytest + +from bcli.client._transport import BCTransport +from bcli.errors import ServerError + + +class _StubAuth: + async def get_access_token(self) -> str: + return "token" + + def clear_cache(self) -> None: + return None + + +def _transport(handler: httpx.MockTransport) -> BCTransport: + t = BCTransport(_StubAuth(), max_retries=2) + t._client = httpx.AsyncClient(transport=handler) + return t + + +def _counting_handler(status: int): + """Always answers `status`; records how many times it was called.""" + calls: list[str] = [] + + def handle(request: httpx.Request) -> httpx.Response: + calls.append(request.method) + return httpx.Response(status, json={"error": {"message": "nope"}}) + + return httpx.MockTransport(handle), calls + + +class TestReadsStillRetry: + @pytest.mark.parametrize("status", [429, 503, 504]) + async def test_get_retries_as_before(self, status): + handler, calls = _counting_handler(status) + t = _transport(handler) + with pytest.raises(Exception): + await t._request("GET", "https://example.test/x") + assert len(calls) == 3, "GET should still use all attempts" + + +class TestMutationsDoNotRetryWithoutAKey: + @pytest.mark.parametrize("method", ["POST", "PATCH", "DELETE"]) + @pytest.mark.parametrize("status", [429, 503, 504]) + async def test_single_attempt_only(self, method, status): + handler, calls = _counting_handler(status) + t = _transport(handler) + with pytest.raises(Exception): + await t._request(method, "https://example.test/x") + assert len(calls) == 1, ( + f"{method} was retried without an idempotency key; a lost response " + f"would duplicate the write" + ) + + @pytest.mark.parametrize("method", ["POST", "PATCH", "DELETE"]) + async def test_network_error_is_not_retried_either(self, method): + """The dangerous case: the request may have been applied before the + connection dropped, so there is no way to know a retry is safe.""" + calls: list[str] = [] + + def handle(request: httpx.Request) -> httpx.Response: + calls.append(request.method) + raise httpx.ReadTimeout("boom", request=request) + + t = _transport(httpx.MockTransport(handle)) + with pytest.raises(ServerError): + await t._request(method, "https://example.test/x") + assert len(calls) == 1 + + +class TestAnIdempotencyKeyReEnablesRetry: + @pytest.mark.parametrize("method", ["POST", "PATCH", "DELETE"]) + async def test_key_allows_retry(self, method): + handler, calls = _counting_handler(503) + t = _transport(handler) + with pytest.raises(Exception): + await t._request(method, "https://example.test/x", idempotency_key="k-1") + assert len(calls) == 3 + + async def test_the_key_is_actually_sent(self): + seen: list[str | None] = [] + + def handle(request: httpx.Request) -> httpx.Response: + seen.append(request.headers.get("Idempotency-Key")) + return httpx.Response(200, json={}) + + t = _transport(httpx.MockTransport(handle)) + await t._request("POST", "https://example.test/x", idempotency_key="k-2") + assert seen == ["k-2"] + + +class TestSuccessPathUnaffected: + @pytest.mark.parametrize("method", ["GET", "POST", "PATCH", "DELETE"]) + async def test_a_successful_mutation_still_works(self, method): + def handle(request: httpx.Request) -> httpx.Response: + return httpx.Response(200, json={"ok": True}) + + t = _transport(httpx.MockTransport(handle)) + assert await t._request(method, "https://example.test/x") == {"ok": True}