Skip to content

fix: the defects the agents page turned up - #22

Merged
AlexeyShalaev merged 10 commits into
masterfrom
fix/agents-page-findings
Sep 6, 2026
Merged

AlexeyShalaev merged 10 commits into
masterfrom
fix/agents-page-findings

Conversation

@AlexeyShalaev

@AlexeyShalaev AlexeyShalaev commented Sep 6, 2026

Copy link
Copy Markdown
Member

Writing docs/agents.md against the source turned up eight things. All eight reproduced;
all eight are fixed here. One of them changes public defaults and is marked breaking.

Every code fix has a test that fails on origin/master and passes here — I checked each one
by restoring the old file under the new test.

1. The decorator's key and coordinator lookup

kwargs.get(key_param) was the only place the key was read, so a key passed positionally was
invisible: the function ran unprotected and nothing said so. Failing to find a coordinator did
the same — no exception, no log.

Reproduction: a decorated function called as act("key-1", coordinator) never reached
coordinate(); a decorated function with a key and no coordinator emitted zero log records.

The decorator now resolves the key from the positional arguments too, when the parameter can be
passed that way (the index is computed once, at decoration time, from the signature), and logs a
WARNING from idempotency_kit.core.decorators.aio.idempotent before running unprotected.

The lookup itself is unchanged, including the vars() scan of every positional argument. It is
loose, and I left it alone rather than tightening it in the same pass: narrowing it would turn a
working infra_param-less wiring into a silent miss for someone. infra_param= still takes the
value at its word, so a duck-typed coordinator keeps working.

Tests: test__decorator__positional_key__calls_coordinator,
test__decorator__keyword_only_key__is_not_read_from_positional_arguments,
test__decorator__no_coordinator__warns.

2. PydanticResultAdapter stored a record it could never decode

encode returned None for a falsy value and decode raised on a falsy payload.

Reproduction: an action returning None under PydanticResultAdapter, called three times
through a real coordinator and repository, ran three times and left
{"result": null, ...} in Redis — every replay was a decode failure, a re-execution, a
collision on save and a second failed decode. Separately, a model that dumps to {} or []
(an acknowledgement with no fields, a RootModel[list[int]] that is empty) encoded fine and
then failed to decode.

encode now refuses None with IdempotencyValidationError, which the coordinator already
handles: record_validation_error, one log line, result returned, operation uncached. decode
now rejects only a null payload, so an empty model round-trips.

Tests: the pydantic-empty-model case in the round-trip parametrize, and
test__coordinator__pydantic_adapter_on_a_none_result__stores_nothing_and_reports_it.

3. The Redis ImportError named a package and an extra that do not exist

It asked for redis-client-kit and offered idempotency-kit[redis-aio]; the requirement is
redis plus orjson and the declared extra is redis. docs/architecture.md repeated the
[redis-aio] spelling.

The test is mechanical rather than a string match: it pulls the extra out of the message and
asserts the distribution declares it (Provides-Extra from the installed metadata), so the
message cannot drift from pyproject.toml again.

Test: test_repository_import_error_names_the_declared_extra.

4. The API reference described the wrong client

docs/api_reference.md called the repository's redis argument an instance of a
redis-client-kit client. It is any redis.asyncio.Redis, subclasses included — which is why
fakeredis works in the tests and an instrumented client works in production. Documentation
fix only; the code was right.

5. Double-counted metrics

The repository and the coordinator each recorded hit, miss, collision and latency for the same
call, and IdempotencyProvider hands both the same APP-scoped collector.

Reproduction: one coordinator-driven miss produced two record_miss calls and two
method="get" latency observations; one hit produced two record_hit calls.

Each metric now has one owner. The coordinator keeps hit, miss, collision and the latency of
get and save — its hit and miss are the truthful ones, since a record that fails to decode
is a miss for the caller and a hit for storage. The repository keeps what the coordinator cannot
produce: its error counters, the bulk hit and miss counts of get_many, and the latency of
delete and get_many.

This changes what a directly-driven repository emits: repo.get() and repo.save() on their
own no longer count a hit, a miss, a collision or their latency. Dashboards built on the shipped
wiring get correct numbers instead of doubled ones; a dashboard built on a bare repository loses
those series. I could not find a fix that keeps both without one of them being wrong.

Tests: test_metrics_comprehensive (rewritten) and
test_metrics_shared_with_coordinator_count_each_operation_once.

6. BaseIdempotencySettings.enabled was read by nothing

grep for readers of the field in the package returns nothing: setting it to False changed
no behaviour anywhere.

Rather than drop a field people already have in their config, I gave it the meaning it claims:
AsyncIdempotencyCoordinator takes enabled=True and coordinate() becomes a pass-through
when it is False — no read, no write, no metric — and the shipped coordinator provider passes
settings.enabled through. The provider reads it with getattr(settings, "enabled", True) so a
settings object written against IdempotencySettingsProtocol before the field was part of it
keeps building a container; the protocol now declares it.

Tests: test__coordinator__disabled__runs_the_action_without_touching_storage,
test__shipped_providers__settings_disabled__coordinator_is_a_pass_through.

7. Two sets of TTL defaults (breaking)

IdempotencyDomainService defaulted to 30 minutes / 60 s / 24 h; BaseIdempotencySettings
shipped 60 minutes / 1 s / 30 days. The effective bounds depended on which one built the
service.

core/constants.py is now the single source and the settings model takes its field defaults
from it. I took the wider pair, deliberately: narrowing the ceiling to 24 hours would start
rejecting TTLs that work today, and IdempotencyInvalidTTLError is caught by the coordinator —
those operations would have gone quietly uncached, which is the failure mode this whole round is
about. Nothing that works today stops working.

min_ttl_seconds moves the other way, from 1 to 60, and no shipped path can observe it: the
coordinator floors every TTL at max(1, ttl_seconds // 60) minutes, so it never asks for less
than 60 seconds, and create_record takes whole minutes.

BREAKING CHANGE: DEFAULT_TTL_MINUTES is 60 (was 30) and MAX_TTL_SECONDS is 2592000 (was
86400), so IdempotencyDomainService() with no arguments keeps records for an hour and accepts
up to 30 days. Pass the arguments explicitly to keep the old numbers.

One existing test hard-coded 2000 minutes as "above the maximum"; it now derives the value from
MAX_TTL_SECONDS.

Test: test__domain_service_and_settings__agree_on_the_ttl_defaults.

8. validate_record and IdempotencyRecordExpiredError had no caller

True, and it pointed at a real hole rather than dead code: the coordinator trusted the
repository never to hand back an expired record. The shipped Redis repository does check, but
expiry is the domain's rule and a backend without native expiry cannot enforce it.

Reproduction: a repository returning a record whose expires_at was an hour in the past made
the coordinator replay it forever — the action never ran.

The coordinator now calls validate_record on every record it reads and counts an expired one
as a miss. No change for the Redis backend, which already filtered them.

Test: test__coordinator__expired_record_from_repository__is_a_miss.

Docs

docs/agents.md stated four of these defects as rules — the silent coordinator lookup, the
invisible positional key, the poison null, and both layers counting — so those lines went with
the code, along with the TTL numbers, the coordinator signature, the settings field list and the
validate_record note. api_reference.md, user_guide.md, quickstart.md and
architecture.md carry the same changes.

Verification

make check
uv run ruff check .          All checks passed!
uv run ruff format --check . 50 files already formatted
uv run mypy idempotency_kit  Success: no issues found in 31 source files

make test-unit               115 passed, 5 deselected
make test-integration        5 passed, 115 deselected   (Docker, testcontainers Redis 7)
make test                    120 passed, coverage 96.80% (threshold 90%)

uv sync --frozen was used throughout and uv.lock is unchanged.

Alex Shalaev added 8 commits September 6, 2026 21:14
…inator is found

The decorator only ever looked in **kwargs for the key, so a key passed
positionally was invisible and the operation ran unprotected. It now resolves the
key from the positional arguments too, when the parameter can be passed that way.

Failing to find a coordinator still degrades to running the function, but it is
now logged as a warning instead of happening in silence.
…er decode

encode() returned null for a falsy value and decode() rejected a falsy payload, so
an action returning None stored a record that failed to decode for the life of that
record: every replay re-ran the action, collided on save, and failed to decode the
winner. A model that dumps to an empty mapping or list hit the same decode guard.

encode() now refuses None outright -- reported as record_validation_error and
swallowed by the coordinator, so the operation goes uncached rather than
poison-cached -- and decode() only rejects a null payload.
The ImportError told the reader to install redis-client-kit and offered a
[redis-aio] extra; the declared extra is [redis] and the requirement is redis
plus orjson. The architecture page named the same missing extra, and the API
reference described the repository's redis argument as a redis-client-kit
client when it is any redis.asyncio.Redis.
The repository and the coordinator both recorded hit, miss, collision and latency
for the same call, and the shipped providers give them one APP-scoped collector, so
every coordinator-driven get produced two misses and two method="get" observations.

The coordinator now owns those four -- its hit and miss are the truthful ones, since
a record that fails to decode is a miss for the caller. The repository keeps what the
coordinator cannot produce: its error counters, the bulk hit and miss counts of
get_many, and the latency of delete and get_many.
IdempotencyDomainService.validate_record and IdempotencyRecordExpiredError were
public with no caller in the library, and the coordinator trusted the repository to
never hand back an expired record. The shipped Redis repository does check, but the
rule belongs to the domain and a backend without native expiry cannot enforce it, so
a third-party repository replayed stale results forever. The coordinator now
validates every record it reads and counts an expired one as a miss.
BaseIdempotencySettings.enabled was read by nothing: setting it to False changed no
behaviour anywhere in the library. AsyncIdempotencyCoordinator now takes enabled
(default True), and the shipped coordinator provider passes the settings flag
through, so False makes every call a pass-through to the action.

The provider reads the flag with getattr so a settings object written against the
protocol before enabled was part of it keeps working.
IdempotencyDomainService defaulted to 30 minutes, a floor of 60 seconds and a
ceiling of 24 hours, while BaseIdempotencySettings shipped 60 minutes, a floor of
1 second and a ceiling of 30 days -- so the effective bounds depended on whether the
service was built by hand or from the settings object. core/constants.py is now the
single source and the settings model takes its field defaults from it.

The wider pair won on both bounds, because narrowing them would have started
rejecting TTLs that work today, and the coordinator swallows an out-of-range TTL:
the operation would have gone quietly uncached.

BREAKING CHANGE: DEFAULT_TTL_MINUTES is 60 (was 30) and MAX_TTL_SECONDS is 2592000
(was 86400), so IdempotencyDomainService() built without arguments now keeps records
for an hour and accepts a TTL of up to 30 days. BaseIdempotencySettings.min_ttl_seconds
defaults to 60 (was 1), which no shipped path can observe: the coordinator already
floors every TTL at one minute. Pass the arguments explicitly to keep the old values.
agents.md stated four of the defects as rules -- the silent coordinator lookup, the
positional key the decorator could not see, the poison null from
PydanticResultAdapter, and both layers counting the same get -- so those rules had to
go with the code. The TTL defaults, the coordinator signature and the settings field
list follow the new canonical numbers, and the reference, guide, quickstart and
architecture pages carry the same changes.
@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

The decode guard for a null payload, the infra_param-on-self lookup and the fallback
for a callable inspect cannot describe.
@AlexeyShalaev

Copy link
Copy Markdown
Member Author

One thing I ran into and did not touch, since it is CI configuration rather than one of the findings.

codecov/patch fails on this PR and will fail on any PR that changes code outside the Redis repository. .github/workflows/ci.yml runs coverage only in the integration job — uv run pytest -m integration --cov=idempotency_kit — and that is the only report uploaded, so codecov's view of the project is integration-only. The unit job runs pytest -m unit with no coverage at all. Every line this PR adds to the coordinator, the decorator, the adapters, the settings model and the Dishka providers is covered by unit tests and by nothing the integration suite runs, so codecov reads it as uncovered.

Locally the full suite is 120 passed at 97% total, and make test (unit + integration, 90% floor) passes. If it is worth aligning, the smallest change is to run coverage in the unit matrix too and upload both reports as separate flags.

@AlexeyShalaev
AlexeyShalaev merged commit 1573db4 into master Sep 6, 2026
7 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/agents-page-findings branch September 6, 2026 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant