From 1f79c2b6babd9611db503ab2885f28f1c46183c8 Mon Sep 17 00:00:00 2001 From: Alexey Shalaev <75322386+AlexeyShalaev@users.noreply.github.com> Date: Fri, 16 Oct 2026 18:34:55 +0400 Subject: [PATCH] fix!: raise on an invalid idempotency key instead of running the action unprotected coordinate() let a key the store cannot hold through to the reservation, where the repository rejected it, and the coordinator treated that like a storage outage: the action ran unreserved and nothing was stored, so every retry with the key repeated the effect. An empty key skipped the coordinator altogether. coordinate() now validates the operation and any non-None key with IdempotencyIdentifiers before anything is read, reserved or run, and raises IdempotencyValidationError, in every in_flight mode. The decorator hands an empty key to the coordinator instead of skipping it. None still means no key; enabled=False is still a pass-through without the check. BREAKING CHANGE: coordinate() and @async_idempotent raise IdempotencyValidationError for an empty key, which used to mean "no idempotency", and for a key or operation that is whitespace-only, contains ":" or is too long, which used to run the action unprotected. Pass None, not "", when a request carries no key, and map IdempotencyValidationError to HTTP 400. Closes #51 --- docs/agents.md | 23 ++++--- docs/api_reference.md | 4 +- docs/user_guide.md | 8 ++- .../core/decorators/aio/idempotent.py | 2 +- .../core/services/aio/coordinator.py | 18 +++++- tests/unit/core/test_coordinator.py | 62 +++++++++++++++++++ tests/unit/core/test_idempotent.py | 21 +++++++ 7 files changed, 123 insertions(+), 15 deletions(-) diff --git a/docs/agents.md b/docs/agents.md index fb9a3fe..030c016 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -179,7 +179,7 @@ result = await coordinator.coordinate( | Method | Returns | Notes | |---|---|---| -| `AsyncIdempotencyCoordinator.coordinate(operation, idempotency_key, ttl_seconds, adapter, action, /, *args, idempotency_fingerprint=None, **kwargs)` | `T` | never raises for storage or decode trouble; raises `IdempotencyInProgressError` when the key is in flight and `in_flight` says so, `IdempotencyKeyReuseError` when the record's fingerprint differs from `idempotency_fingerprint` | +| `AsyncIdempotencyCoordinator.coordinate(operation, idempotency_key, ttl_seconds, adapter, action, /, *args, idempotency_fingerprint=None, **kwargs)` | `T` | never raises for storage or decode trouble; raises `IdempotencyValidationError` when the operation or a non-`None` key is invalid, `IdempotencyInProgressError` when the key is in flight and `in_flight` says so, `IdempotencyKeyReuseError` when the record's fingerprint differs from `idempotency_fingerprint` | | `IdempotencyDomainService.create_record(operation, idempotency_key, result, *, ttl_minutes=None, fingerprint=None)` | `IdempotencyRecord` | raises `IdempotencyInvalidTTLError`, `IdempotencyValidationError` | | `IdempotencyDomainService.create_pending_record(operation, idempotency_key, *, lease_seconds, fingerprint=None)` | `IdempotencyRecord` | the reservation; not held to the TTL bounds; raises `IdempotencyValidationError` | | `IdempotencyDomainService.validate_record(record)` | `None` | raises `IdempotencyRecordExpiredError`; the coordinator calls it on every record it reads | @@ -312,8 +312,12 @@ fields existed is read as enabled, `"wait"` and 30 seconds. keyword arguments, and from the positional arguments when the parameter can be passed that way. Keyword-only is still the shape to write: nothing can land in it by position, and the call site has to name it. -4. **A falsy key means no idempotency.** `None` and `""` both short-circuit straight to the - action, in the decorator and in `coordinate()`. +4. **Only `None` means no idempotency.** `None` short-circuits straight to the action, in + the decorator and in `coordinate()`. Any other key, `""` included, is checked together + with the operation before anything is read or run (rule 12); an invalid one raises + `IdempotencyValidationError`, 400 in HTTP terms, and the action does not run. Pass `None`, + not `""`, when a request carries no key. `enabled=False` skips the check along with + everything else. 5. **A coordinator the decorator cannot find means no idempotency.** It looks for `infra_param` by name in `kwargs` then as an attribute of the first positional argument, then for an `AsyncIdempotencyCoordinator` by type in `kwargs`, in `args`, and in the @@ -348,8 +352,10 @@ fields existed is read as enabled, `"wait"` and 30 seconds. `IdempotencyInvalidTTLError`, which the coordinator catches: the operation is simply not cached, and the caller gets its result anyway. 12. **Neither identifier may contain a colon**, both are stripped of surrounding whitespace, - and the lengths are 100 for `operation` and 255 for `idempotency_key`. The Redis - repository re-validates on every call, so an over-long key raises there too. + and the lengths are 100 for `operation` and 255 for `idempotency_key`; an empty or + whitespace-only one is invalid. `coordinate()` and the decorator raise + `IdempotencyValidationError` for either before the action runs, and the Redis repository + re-validates on every call. 13. **`PydanticResultAdapter` cannot represent an absent result.** `encode` raises `IdempotencyValidationError` when handed `None` rather than storing a `null` it could never decode again; the coordinator counts that as `record_validation_error`, logs it, @@ -532,11 +538,12 @@ All derive from `IdempotencyError`, which is exported alongside them. | `IdempotencyInProgressError` | `(operation, key)` | another call with the same key is still running its action. `coordinate()` and the decorator raise it at once with `in_flight="raise"`, and after a whole lease of waiting with `"wait"`. Carries `.operation` and `.key` | | `IdempotencyKeyReuseError` | `(operation, key, stored_fingerprint, fingerprint)` | the record under the key was made for a different request. `coordinate()` and the decorator raise it before the action runs, from a completed record and from a pending one. Carries all four | | `IdempotencyStorageError` | `(message, operation=None, original_error=None)` | the backend failed. Carries `.operation` and `.original_error` | -| `IdempotencyValidationError` | `(message, errors=None)` | an identifier or a result failed validation. `.errors` holds the Pydantic error list when there is one | +| `IdempotencyValidationError` | `(message, errors=None)` | an identifier or a result failed validation. `coordinate()` and the decorator raise it for an invalid operation or key before the action runs. `.errors` holds the Pydantic error list when there is one | | `IdempotencyInvalidTTLError` | `(ttl_seconds, min_ttl, max_ttl)` | the TTL is outside the domain service's range. Carries all three | -Through `coordinate()` and the decorator only `IdempotencyInProgressError` and -`IdempotencyKeyReuseError` reach the caller — both are about the request, not about storage: +Through `coordinate()` and the decorator only `IdempotencyValidationError` for the +identifiers, `IdempotencyInProgressError` and `IdempotencyKeyReuseError` reach the caller — +all three are about the request, not about storage: the collision is resolved into a wait or the winner's result, and every other one is logged, counted and swallowed. The rest are the contract of the repository and the domain service, which is where you meet them if you drive those directly. diff --git a/docs/api_reference.md b/docs/api_reference.md index 5a82cc3..c3ed141 100644 --- a/docs/api_reference.md +++ b/docs/api_reference.md @@ -56,7 +56,7 @@ The flow: reserve the key, run the action, store the result; replay a stored res - `in_flight_lease_seconds` (int, default: 30): How long a reservation is held before it counts as abandoned, and the longest a waiting caller waits. Must be at least 1 and longer than the action can take. - *Note*: The constructor raises `TypeError` when the repository has no `replace` and `in_flight` is not `"run"`. - **Methods**: - - `coordinate(operation, idempotency_key, ttl_seconds, adapter, action, /, *args, idempotency_fingerprint=None, **kwargs)`: Runs the flow and returns the action's result type. Everything after the five positional-only arguments goes to the action except `idempotency_fingerprint`, the fingerprint of the request, which is stored with the record and compared on a hit. Never raises for storage or decode trouble; raises `IdempotencyInProgressError` when the key is in flight and `in_flight` says so, and `IdempotencyKeyReuseError` when the record's fingerprint differs from the caller's. + - `coordinate(operation, idempotency_key, ttl_seconds, adapter, action, /, *args, idempotency_fingerprint=None, **kwargs)`: Runs the flow and returns the action's result type. Everything after the five positional-only arguments goes to the action except `idempotency_fingerprint`, the fingerprint of the request, which is stored with the record and compared on a hit. A `None` key runs the action and nothing else. Never raises for storage or decode trouble; raises `IdempotencyValidationError` when the operation or a non-`None` key (the empty string included) is invalid, before the action runs, `IdempotencyInProgressError` when the key is in flight and `in_flight` says so, and `IdempotencyKeyReuseError` when the record's fingerprint differs from the caller's. ### async_idempotent The same flow as a decorator: it finds the key in the call's arguments and the coordinator in the call's arguments or on `self`, then delegates to `coordinate()`. @@ -119,6 +119,6 @@ Redis implementation of the repository protocol. - **`IdempotencyInProgressError(operation, key)`**: Raised by `coordinate()` and the decorator when another call with the same key is still running its action. - **`IdempotencyKeyReuseError(operation, key, stored_fingerprint, fingerprint)`**: Raised by `coordinate()` and the decorator when the record under the key was made for a request with a different fingerprint. - **`IdempotencyStorageError(message, operation, original_error)`**: Raised on storage failure. -- **`IdempotencyValidationError(message, errors=None)`**: Raised for invalid input (e.g. empty key, too long string). +- **`IdempotencyValidationError(message, errors=None)`**: Raised for invalid input (e.g. empty key, too long string), by `coordinate()` and the decorator before the action runs as well. - `errors` (list, optional): Detailed Pydantic validation errors. - **`IdempotencyInvalidTTLError(ttl_seconds, min_ttl, max_ttl)`**: Raised for invalid TTL. diff --git a/docs/user_guide.md b/docs/user_guide.md index 473cfc9..c592f3b 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -456,6 +456,12 @@ The same `idempotency_key` can be used for different operations (e.g., `user.cre - Both are stripped of leading/trailing whitespace. - Max lengths: operation (100), key (255). +`coordinate()` and the decorator check both before the action runs. An invalid key — empty, +whitespace-only, with a colon or too long — raises `IdempotencyValidationError` instead of +running the action unprotected, so a malformed `Idempotency-Key` header becomes a 400 rather +than a request whose retries are silently not deduplicated. Only `None` means "no key": +pass `None`, not `""`, when the request carries none. + ### Error Handling The library defines several exceptions to handle various idempotency scenarios: @@ -465,7 +471,7 @@ The library defines several exceptions to handle various idempotency scenarios: - **`IdempotencyKeyReuseError`**: Raised by `coordinator.coordinate()` and the decorator when the record under the key was made for a request with a different fingerprint. Carries both fingerprints. Map it to HTTP 422. - **`IdempotencyRecordExpiredError`**: Raised by `service.validate_record()` if the record exists but its TTL has passed. - **`IdempotencyInvalidTTLError`**: Raised by `service.create_record()` if the requested TTL is outside the allowed range (configured in `IdempotencyDomainService`). -- **`IdempotencyValidationError`**: Raised by `service.create_record()` if validation of `operation` or `idempotency_key` fails (e.g., empty string or too long). +- **`IdempotencyValidationError`**: Raised by `coordinator.coordinate()` and the decorator before the action runs, and by `service.create_record()`, if validation of `operation` or `idempotency_key` fails (e.g., empty string or too long). Map it to HTTP 400. - **`IdempotencyStorageError`**: Raised for critical infrastructure failures like Redis connection errors. This allows you to distinguish between a cache miss and a storage backend failure. - **`IdempotencyError`**: Base exception for all library errors. Also raised for data corruption during deserialization. diff --git a/idempotency_kit/core/decorators/aio/idempotent.py b/idempotency_kit/core/decorators/aio/idempotent.py index a8ab18f..14c0642 100644 --- a/idempotency_kit/core/decorators/aio/idempotent.py +++ b/idempotency_kit/core/decorators/aio/idempotent.py @@ -91,7 +91,7 @@ def decorator(func: Callable[..., Awaitable[T]]) -> Callable[..., Awaitable[T]]: async def wrapper(*args: Any, **kwargs: Any) -> T: # 1. Resolve idempotency key idempotency_key = _resolve_key(args, kwargs) - if not idempotency_key: + if idempotency_key is None: return await func(*args, **kwargs) # 2. Resolve coordinator diff --git a/idempotency_kit/core/services/aio/coordinator.py b/idempotency_kit/core/services/aio/coordinator.py index 29d4ccb..79b3e74 100644 --- a/idempotency_kit/core/services/aio/coordinator.py +++ b/idempotency_kit/core/services/aio/coordinator.py @@ -6,6 +6,8 @@ from enum import Enum, auto from typing import Any, Generic, TypeVar +from pydantic import ValidationError + from idempotency_kit.core.constants import ( DEFAULT_IN_FLIGHT_LEASE_SECONDS, DEFAULT_IN_FLIGHT_MODE, @@ -20,7 +22,7 @@ IdempotencyRecordExpiredError, IdempotencyValidationError, ) -from idempotency_kit.core.models.entities import IdempotencyRecord +from idempotency_kit.core.models.entities import IdempotencyIdentifiers, IdempotencyRecord from idempotency_kit.core.protocols.adapter import ResultAdapter from idempotency_kit.core.protocols.aio.repository import AsyncIdempotencyRepository from idempotency_kit.core.protocols.metrics import IdempotencyMetricsProtocol, NoOpIdempotencyMetrics @@ -122,7 +124,12 @@ async def coordinate( the record, and a record under the same key with a different fingerprint is a key reuse. ``None`` on either side means no comparison, which is the key-only behaviour. - With ``enabled=False`` the action is simply run: nothing is read, nothing is + A ``None`` key means no idempotency: the action is simply run. Any other key, + the empty string included, has to be a valid identifier, and so does the operation; + an invalid one raises ``IdempotencyValidationError`` before the action runs, since + running it unprotected would leave the client believing its retries are safe. + + With ``enabled=False`` the action is simply run: nothing is validated, read or written, and no metric is recorded. Storage and decode trouble never raise: the coordinator degrades to running the @@ -131,9 +138,14 @@ async def coordinate( ``"wait"`` and a whole lease has passed; and ``IdempotencyKeyReuseError``, when the record under the key was made for a different request. """ - if not self._enabled or not idempotency_key: + if not self._enabled or idempotency_key is None: return await action(*args, **kwargs) + try: + IdempotencyIdentifiers(operation=operation, idempotency_key=idempotency_key) + except ValidationError as e: + raise IdempotencyValidationError(str(e), errors=e.errors()) from e + if self._in_flight == "run": return await self._coordinate_unreserved( operation, idempotency_key, ttl_seconds, adapter, action, idempotency_fingerprint, *args, **kwargs diff --git a/tests/unit/core/test_coordinator.py b/tests/unit/core/test_coordinator.py index 078e616..f3cc541 100644 --- a/tests/unit/core/test_coordinator.py +++ b/tests/unit/core/test_coordinator.py @@ -4,11 +4,15 @@ from unittest.mock import AsyncMock, MagicMock import pytest +from fakeredis import FakeAsyncRedis as AsyncRedisClient +from idempotency_kit.core.adapters import JsonResultAdapter +from idempotency_kit.core.constants import InFlightMode from idempotency_kit.core.exceptions import IdempotencyKeyCollisionError, IdempotencyValidationError from idempotency_kit.core.models.entities import IdempotencyRecord from idempotency_kit.core.services.aio.coordinator import AsyncIdempotencyCoordinator from idempotency_kit.core.services.domain import IdempotencyDomainService +from idempotency_kit.infra.storage.redis.aio import RedisAsyncIdempotencyRepository @pytest.mark.asyncio @@ -358,3 +362,61 @@ async def test__coordinator__disabled__runs_the_action_without_touching_storage( mock_repo.get.assert_not_called() mock_repo.save.assert_not_called() metrics.record_miss.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("in_flight", ["wait", "raise", "run"]) +@pytest.mark.parametrize( + "idempotency_key", ["", "order:42", "x" * 256, " "], ids=["empty", "colon", "too_long", "whitespace"] +) +async def test__coordinator__invalid_key__raises_validation_error_on_every_retry_and_never_runs_the_action( + redis_repository: RedisAsyncIdempotencyRepository, + fake_redis: AsyncRedisClient, + idempotency_key: str, + in_flight: InFlightMode, +) -> None: + """Issue #51: a key the store cannot hold used to run the action unprotected, so every retry repeated it.""" + # Arrange + coordinator = AsyncIdempotencyCoordinator( + repository=redis_repository, domain_service=IdempotencyDomainService(), in_flight=in_flight + ) + action = AsyncMock(return_value={"charge_id": "ch_1"}) + + # Act & Assert + for _ in range(2): + with pytest.raises(IdempotencyValidationError, match="idempotency_key"): + await coordinator.coordinate("payment.charge", idempotency_key, 3600, JsonResultAdapter(), action) + action.assert_not_called() + assert await fake_redis.keys("*") == [] + + +@pytest.mark.asyncio +async def test__coordinator__invalid_operation__raises_validation_error_before_the_action( + redis_repository: RedisAsyncIdempotencyRepository, +) -> None: + """The operation is half of the storage key and is held to the same rules.""" + # Arrange + coordinator = AsyncIdempotencyCoordinator(repository=redis_repository, domain_service=IdempotencyDomainService()) + action = AsyncMock() + + # Act & Assert + with pytest.raises(IdempotencyValidationError, match="operation"): + await coordinator.coordinate("payment:charge", "order-42", 3600, JsonResultAdapter(), action) + action.assert_not_called() + + +@pytest.mark.asyncio +async def test__coordinator__disabled_with_an_invalid_key__runs_the_action_without_validating( + mock_repo: AsyncMock, mock_domain_service: MagicMock, mock_adapter: MagicMock +) -> None: + """enabled=False stays a pass-through: the switch does not start refusing requests.""" + # Arrange + coordinator = AsyncIdempotencyCoordinator(repository=mock_repo, domain_service=mock_domain_service, enabled=False) + action = AsyncMock(return_value="ran") + + # Act + result = await coordinator.coordinate("op", "", 60, mock_adapter, action) + + # Assert + assert result == "ran" + action.assert_awaited_once() diff --git a/tests/unit/core/test_idempotent.py b/tests/unit/core/test_idempotent.py index 240d7a1..5cf39d2 100644 --- a/tests/unit/core/test_idempotent.py +++ b/tests/unit/core/test_idempotent.py @@ -112,6 +112,27 @@ async def my_func(idempotency_key: str | None) -> str: mock_coordinator.coordinate.assert_not_called() +@pytest.mark.asyncio +async def test__decorator__empty_key__goes_to_the_coordinator( + mock_coordinator: MagicMock, mock_adapter: MagicMock +) -> None: + """Only ``None`` means no key; an empty one is the coordinator's to refuse.""" + # Arrange + mock_coordinator.coordinate.return_value = "coordinated" + + @async_idempotent(operation="test.op", adapter=mock_adapter) + async def my_func(idempotency_key: str | None, coord: AsyncIdempotencyCoordinator) -> str: + return "direct" + + # Act + result = await my_func(idempotency_key="", coord=mock_coordinator) + + # Assert + assert result == "coordinated" + mock_coordinator.coordinate.assert_awaited_once() + assert mock_coordinator.coordinate.call_args.args[1] == "" + + @pytest.mark.asyncio async def test__decorator__no_coordinator__executes_directly(mock_adapter: MagicMock) -> None: """Test that missing coordinator causes direct function execution."""