Skip to content

fix: renew the reservation while the action runs - #56

Merged
AlexeyShalaev merged 1 commit into
masterfrom
fix/52-renew-reservation
Oct 16, 2026
Merged

AlexeyShalaev merged 1 commit into
masterfrom
fix/52-renew-reservation

Conversation

@AlexeyShalaev

Copy link
Copy Markdown
Member

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_at a lease ahead (replace(), the method that already completes a reservation; created_at is kept). It is an async with self._renewing(claim) around await action(...), so every exit goes through it.

  • Stop, don't cancel. On completion, failure and cancellation the task is told to stop (an Event) and awaited before the result is written or the reservation deleted. A renewal SET cancelled 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_it pins this with a renewal that is "already on the wire" (shielded, lands 0.3 s later): with renewal.cancel() in place of stop-and-wait it fails with ('pending', None) == ('completed', 'done').
  • Renewal failure. Counted as storage_renew_error and 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.
  • Shutdown. When 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".
  • Expiry now means the owner is gone: the process died, or it could not run its event loop or reach the store for a whole lease. Renewal is on the event loop, so an action that blocks the loop for a lease still loses the key; the docs' lease example became that mistake, with asyncio.to_thread as the fix.

Cost: one extra SET per 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_seconds keeps its name and default and only loses the "must outlive the action" requirement.

Rejected

  • Waiter deadline from the record's expiry (second bullet of the issue). With renewal that makes the wait unbounded behind an action that hangs while its owner is alive. A waiter keeps its bound, one lease from arrival, then 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.
  • Conditional renewal (compare-and-set via Lua or Redis 8's 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:

master:
second returned {'charge_id': 'second'} effects [('first', 0.0), ('second', 1.05)]

this branch:
second raised IdempotencyInProgressError effects [('first', 0.0)]
first returned {'charge_id': 'first'} effects [('first', 0.0)]

New integration tests (tests/integration/test_reservation_renewal.py, real Redis via testcontainers, 1 s lease):

  • the reporter's scenario: one charge, the duplicate refused, the result replayed afterwards;
  • callers at 1.5 s and 2.5 s into a 3 s action are refused, and the stored record ends with the result's TTL, not a lease;
  • an owner process killed with SIGKILL mid-action: refused while it lives, the key free a lease after the kill;
  • an owner process whose asyncio.run() returns mid-action: exit code 0, no task warnings on stderr, the key released and still absent a renewal interval later;
  • the action returns / raises / is cancelled after several renewals: no renewal lands afterwards (completed record intact, or key absent).

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 with cancel() 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_error in 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.py and the agents page; whichever merges second gets rebased.

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

codecov Bot commented Oct 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@AlexeyShalaev
AlexeyShalaev merged commit e86c51f into master Oct 16, 2026
8 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/52-renew-reservation branch October 16, 2026 14:58
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.

A reservation is not renewed while its action runs, so after the lease a waiting duplicate runs the action too

1 participant