Repository navigation
fix: renew the reservation while the action runs - #56
Merged
Merged
Conversation
A pending record expired a lease after its owner reserved the key, however
alive the owner was, so a duplicate that arrived meanwhile ("wait" mode
included) found the key free and ran the action a second time. The
coordinator now renews its reservation while the action runs: a task beside
it rewrites the pending record every third of a lease with its expiry a
lease ahead. The task is stopped and awaited, not cancelled, before the
result is written or the reservation released, on every exit including
cancellation, so a renewal still on its way to the store cannot land after
either. A failed renewal is counted as storage_renew_error and logged, and
the next one tries again; a store down for a whole lease loses the
reservation, as before, and the action carries on.
Expiry now means the owner is gone, and the lease no longer has to outlive
the action. A waiter still waits at most a lease from its arrival and then
raises IdempotencyInProgressError.
Closes #52
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes #52
Problem
The pending record expired at its owner's start plus the lease, however alive the owner was. A waiter's deadline is its own arrival plus the lease, which is later, so in
"wait"mode a duplicate that arrived while the first call was running saw the marker disappear, reserved the key and ran the action again. Any caller after the lease did the same. The docs answered with "the lease has to outlive the action", which pushes towards long leases and slow recovery from a crashed owner.Fix
While the action runs under our reservation, a task beside it rewrites the pending record every lease/3 with
expires_ata lease ahead (replace(), the method that already completes a reservation;created_atis kept). It is anasync with self._renewing(claim)aroundawait action(...), so every exit goes through it.Event) and awaited before the result is written or the reservation deleted. A renewalSETcancelled in flight can still reach the store after the completed record, put a pending marker back over it, and a lease later the next caller runs the action again.test__coordinator__renewal_in_flight_when_the_action_ends__lands_before_the_result_not_over_itpins this with a renewal that is "already on the wire" (shielded, lands 0.3 s later): withrenewal.cancel()in place of stop-and-wait it fails with('pending', None) == ('completed', 'done').storage_renew_errorand logged; the next attempt comes a third of a lease later. One or two misses in a row are absorbed. A store down for a whole lease loses the reservation, which is the pre-renewal behaviour, and the action carries on: cutting a running payment short is worse than the duplicate risk the outage already created.asyncio.run()cancels everything with an action in flight, the coordinator's cleanup awaits the (already cancelled) renewal task and releases the key; no "Task was destroyed" or "exception never retrieved".asyncio.to_threadas the fix.Cost: one extra
SETper in-flight action every lease/3 (every 10 s at the default 30 s lease), nothing for actions shorter than that. No public API change;in_flight_lease_secondskeeps its name and default and only loses the "must outlive the action" requirement.Rejected
IdempotencyInProgressError; since the marker no longer disappears under a live owner, that bound ends in a 409 instead of a second run. In the reporter's script the second call now raises after ~1 s and there is one charge.SET IFEQ). It would stop a renewal from overwriting a record that is no longer ours, but that only happens after the reservation was already lost to a long outage, when the duplicate has already happened. It needs an owner token on the record and a new repository method every custom repository would have to implement; the Two concurrent callers with the same key both run the action: an in-flight reservation is needed #26 plan rejected a fencing token for the same reason.Verification
The reporter's script, Redis 7 in Docker:
New integration tests (
tests/integration/test_reservation_renewal.py, real Redis via testcontainers, 1 s lease):asyncio.run()returns mid-action: exit code 0, no task warnings on stderr, the key released and still absent a renewal interval later;New unit tests (fakeredis, so they run on 3.11 to 3.14 in CI): renewal keeps the key past the lease; one failed renewal is counted and absorbed; all renewals failing loses the reservation while the owner still completes; the in-flight-renewal ordering test above.
Fails before: with master's
coordinator.py, 6 fail (the three reporter/latecomer/kill integration tests and the three renewal unit tests). The "no renewal lands afterwards" tests pass on master, since nothing renews there; with the stop-and-wait removed (renewal left running) all three fail, and the ordering unit test fails withcancel()as shown above.make check: ruff, format and mypy clean.uv run pytest --cov=idempotency_kit: 195 passed, 96% coverage. The renewal tests also pass on Python 3.11 and 3.14, and five repeated runs of them were green (margins are 0.3 s or more around every expiry).Docs: agents page (scope note, mental model, rule 2, the lease mistake replaced by the blocked-loop one,
storage_renew_errorin the metrics list), user guide (the lease paragraph, the coordinator snippet, constants table, best practice 3), API reference, architecture (how renewal stops, what expiry now means), and the constants/domain-service comments.Overlaps #54 and #55 in
coordinator.pyand the agents page; whichever merges second gets rebased.