feat!: reserve the key while the action runs - #28
Merged
Conversation
Two callers with the same key, the second arriving while the first one's action is still running, both ran the action: the check was a GET and the write a SET NX after the action, with nothing marking the key as taken in between. The coordinator now reserves the key first, with a pending record written under SET NX and the in-flight lease as its TTL, and writes the result over it afterwards with the new repository method replace(). What a second caller gets is the coordinator's in_flight mode: "wait" (the default) polls until the first caller's record lands, "raise" raises the new IdempotencyInProgressError at once, and "run" is the previous flow for actions that are safe to repeat. An action that raises or is cancelled deletes its reservation, as does a result that cannot be stored, so the retry runs again. A pending record past its lease counts as absent. BREAKING CHANGE: a second concurrent caller with the same key now waits for the first caller's result instead of running the action too, and coordinate() and the decorator can raise IdempotencyInProgressError. Pass in_flight="run" to the coordinator (or set it on the settings object) to keep the previous behaviour. AsyncIdempotencyRepository gained replace(record); a custom repository needs it, and the coordinator raises TypeError at construction without it unless in_flight="run". IdempotencyRecord gained status, which records written before this change read as "completed". Closes #26
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Sep 6, 2026
Two concurrent callers with the same key both run the action: an in-flight reservation is needed
#26
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two
coordinate()calls with the same key, the second starting while the first one's action was still running, both ran the action. The check was aGET, the write aSET NXafter the action, and nothing marked the key as taken in between — so the one window an idempotency key exists for, a client that timed out and retried, was the window in which the library charged the card twice. Yourmeasure.pyon master:The coordinator now reserves the key before the action runs. The reservation is the record in a pending state —
IdempotencyRecordgainedstatus: Literal["pending", "completed"] = "completed", so a record written before this change reads as completed and nothing is migrated — written underSET NXwith the lease as its TTL, under the very same storage key. After the action the completed record is written over it with the new repository methodreplace(record), which issavewithoutNX. Reserve-first keeps a miss at two round trips, the same as the oldGET+SET NX; a hit costs two instead of one, and hits are the rare path. When the reservation collides the coordinator reads the key: a completed record is a hit, a pending one is another caller in flight, and anything else (the holder gave up between our write and our read, a record this adapter cannot decode, a read that failed) means the action runs unreserved and the result is written withreplace, which also heals a stale-shape record instead of re-running until it expires.in_flighton the coordinator decides what the second caller gets."wait", the default, polls every 50 ms until the record lands and gives up withIdempotencyInProgressErrorafter a whole lease;"raise"raises it as soon as it sees the reservation, the 409 shape;"run"is the previous flow verbatim, for actions that are genuinely safe to repeat.in_flight_lease_secondsdefaults to 30. Both are onBaseIdempotencySettingsand go through the Dishka provider the wayenableddoes. An action that raises deletes its reservation before the exception propagates, cancellation included, so the retry runs again; so does a result that cannot be stored (an out-of-range TTL, aNoneunderPydanticResultAdapter) — that one I found the hard way, when the existing round-trip test for theNonecase sat waiting for a 30 s lease behind a marker with no result ever coming. A pending record past its lease counts as absent, so a crashed worker cannot wedge a key beyond it, and a waiter whose holder fails takes the key and runs the action itself rather than giving up.I went with
waitas the default, as you leaned in the issue. The retry-while-in-flight window is the reason to have an idempotency key at all, and a default that runs the effect twice there is the wrong default however well rule 2 documented it. The cost lands only on the duplicate caller, which waits for the winner's result instead of producing a second one;in_flight="run"is one argument away. It is a pre-1.0 minor underbump-minor-pre-major, like the TTL change in 0.2.0, and the PR title carries the!. The migration text is in theBREAKING CHANGE:footer of the commit, for the changelog if you squash with the body.Two things the upgrade note has to say, and does (user guide, agents page rules 21 and 22). A custom repository needs
replace; the coordinator raisesTypeErrorat construction when the repository lacks it and the mode is notrun, so that failure is at startup rather than a 30 s hang on every retry. And during a rolling upgrade an instance still on 0.2.x reads a pending record as a completed one with anullresult:PydanticResultAdapterturns that into a decode failure and runs the action, the old behaviour, butJsonResultAdapterandVoidResultAdapterwould replay theNone. Roll out within_flight="run"and switch once every instance is on the new version, or accept the window.Metrics: no new counter.
IdempotencyMetricsProtocolis implemented by hand in the user guide, so a new method breaks every custom collector, for an outcome that is already visible:record_collisionkeeps meaning two callers on one key at once and is what the second caller records on finding the reservation, a waiter then records a hit like any hit, and a refusal is the exception the application already counts as a 409. The reservation is timed undermethod="reserve"; a failed reservation or release isstorage_reserve_error/storage_release_error.Rejected: a separate lock key or a lock protocol — two keys per operation, two concepts, cluster hash slots to think about, where a pending record is one concept under one key. A fencing token checked with a Lua script on the final write and on the delete — it only matters when the lease is shorter than the action, a misconfiguration that already degrades to exactly the old behaviour, and it would cost a
lupadependency on the test suite; the lease is documented as having to outlive the action, with an example under Common mistakes.Docs: rule 2, rule 6, rule 8, rule 18 and the mutex example on the agents page follow the code, plus rules 21 and 22, the new error, the record's
status,replace,create_pending_record, the constants and the settings fields. The architecture page's request flows are redrawn around the reservation and the "at-most-once or exactly-once" sentence now says what the library gives and what it does not. The user guide has an "In-flight requests" section with the mode table, the 409 mapping, the lease, failures, storage trouble and the upgrade note; the README, index and quickstart no longer describe the second request as executing.CHANGELOG.mduntouched, release-please owns it.Tests:
tests/unit/core/test_in_flight.pyholds your scenario — twocoordinate()calls, the second starting 50 ms into the first one's 200 ms action, a provider that remembers its charges — asserting one charge and identical results inwaitmode,IdempotencyInProgressErrorfor the second caller inraisemode, and two charges inrunmode; then the release on a raising and on a cancelled action, the waiter taking over when the holder fails, an expired lease counting as absent, the bounded wait (withtime.monotonicpatched, so the test is instant), storage trouble on reserve and on release, an undecodable record being replaced, arun-mode coordinator meeting a marker from a reserving peer, constructor validation, and theTypeErrorfor a repository withoutreplace. The same scenario runs against real Redis in the integration suite. The mock-based coordinator tests describe theGET→SET NXchoreography exactly, so their fixture pinsin_flight="run"and they remain the spec of that mode;replacehas its own repository tests, and there is a decode test for the pre-change JSON shape (nostatus). As a negative control I ran your scenario, written with 0.2.0 names only, against master in a worktree:AssertionError: assert ['ch_1', 'ch_2'] == ['ch_1']. The new test module cannot even collect there (ImportError: cannot import name 'IdempotencyInProgressError').The gate, before touching anything and again at the end:
(Before: 120 passed, 96.80%; the one warning is the pre-existing
testcontainers.redisdeprecation.)uv.lockis untouched.Your script, unmodified, against this branch in a venv with
uv pip install -e ".[redis]" "testcontainers[redis]":The second block is #27, which follows on a branch based on this one.
Closes #26