Repository navigation
feat(redis): lease renewal for in-flight stream entries - #299
sebastianwessel wants to merge 19 commits into
Conversation
Redis resets a PEL entry's idle time only when it is (re)delivered, not while a handler works on it. With retry_on_idle_ms, a handler that runs longer than the threshold (multi-minute LLM calls) had its entry reclaimed and redelivered through the retry stream while it was still running, so the message was processed twice in parallel. The only workaround was a long retry_on_idle_ms, which delays crash recovery by the same amount. subscribe(..., lease_renewal=True) renews every in-flight entry each lease_renewal_interval_ms (default retry_on_idle_ms // 3) with an ownership-checked XCLAIM <stream> <group> <consumer> 0 <id> JUSTID in one Lua script: same owner, same delivery count, idle time reset. A crashed consumer stops renewing and is reclaimed as before. - Main and .retry stream, each with its own group and consumer; batch handlers; entries read into the PEL but still queued in the process (max_records > 1, max_workers > 1). Entries older than the oldest run in flight (an earlier process with the same consumer name) are left for the reclaimer. lease_renewal defaults max_records to 1. - Lost lease (entry left this consumer's PEL): cancel the handler and raise LeaseLostError, or log and continue with cancel_on_lease_lost=False. An entry still owned but trimmed from the stream can't be redelivered, so its handler finishes. - Each renewal is bounded by half the interval; failures are logged with stream / group / ids and never stop the consumer. Renewal starts before the consumers and stops after broker.stop(). - max_processing_ms: handler deadline, cancel + ProcessingTimeoutError, NACK for the normal retry, so a hung handler can't hold a lease forever. - Validated in Agent.subscribe and RedisTransport.subscribe: requires retry_on_idle_ms, interval < retry_on_idle_ms, knobs require lease_renewal=True, rejected with no_ack and AckPolicy.MANUAL. - delete_on_ack and the lease wrapper share one call_handler() helper. Opt-in; subscriptions without the options are unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
QualOps Code Quality AnalysisStatus: Summary
🟠 High Issues (5)
🟡 Medium Issues (6)
...and 3 more medium issues 📊 Full ReportPowered by QualOps |
- Handler wrapper: tell the wrapper's own cancellation (agent stop) from an SDK-initiated one without Task.cancelling(), which Python 3.10 lacks. The handler task is only awaited through asyncio.wait, so a stop that lands while a timed-out handler unwinds propagates instead of becoming ProcessingTimeoutError. A handler that cancels itself still propagates CancelledError. - Log when a leased message has no stream ids (it runs unleased) instead of skipping the lease silently; read message_ids defensively. - LeaseManager.add returns whether the keeper is new and refuses a second registration with different settings instead of replacing a keeper whose task keeps running the old one. discard() also cancels the task. - Subscribe rolls back exactly the keepers it (and its retry subscribe) created, instead of re-deriving the retry consumer name, and also when broker.subscriber() raises. Keepers added after connect() start right away. connect() stops the lease manager it started if the broker fails to start. - Renewal runs at a fixed rate; the prefetch scan reads XPENDING in pages of 1000 (one round trip in practice); finished / lost-queued ids are pruned against the state after the round's awaits. - is_async_callable: unwraps functools.partial, uses inspect (asyncio's variant is deprecated in 3.14) and still recognises AsyncMock on 3.10/3.11. - Option validation rejects bools for the ms options and a non-bool cancel_on_lease_lost. Co-Authored-By: Claude Opus 5.5 <[email protected]>
valdis
left a comment
There was a problem hiding this comment.
maybe we should use a verb form for the flags name, like auto_renew_lease the same style as we have with delete_on_ack ?
pyproject.toml allowed faststream>=0.6, but CI only ever installed the locked 0.7.x, so the bottom of the range was never tested. It didn't work: - 0.6.0-0.6.2: StreamSub has no min_idle_time (added in 0.6.3) and the SDK always passes it, so every Redis stream subscribe raised TypeError. - 0.6.3: a min_idle_time (XAUTOCLAIM) subscription never claimed pending entries (test_redis_no_ack_on_error); FastStream reworked that read path in 0.6.4. Raising the floor to 0.6.4 breaks no setup that works today; the failure moves from runtime to install time. The lock only changes its content-hash (faststream stays 0.7.5). The two backoff tests that failed on 0.6.x were a test-helper issue, not an SDK one: BinaryMessageFormatV1.encode() is sync in 0.6 and async in 0.7, and _make_envelope always awaited it. The SDK itself only uses parse(), which is sync in both. The helper now awaits only an awaitable. New CI job test-min-faststream runs the whole suite on Python 3.10 with faststream 0.6.4 (installed over the lock) and is part of the all-checks-passed gate. Co-Authored-By: Claude Opus 5.5 <[email protected]>
FastStream 0.7.6 changed the stream subscriber's NOGROUP handling: instead
of raising StreamGroupNotFoundError (the task supervisor restarted the read,
so it resumed once the group existed again) it now calls
_stop_on_terminal_error, which stops the subscriber for good ("restart the
application"). The group monitor still recreated the group, but nothing
consumed from it, so test_nogroup_recovery_via_monitor and
test_partial_group_loss_redelivers_existing_messages failed on 0.7.7 (the
newest version faststream<0.8 allows) while CI, installing the lock's
0.7.5, stayed green.
connect() now remembers the consumer-group stream subscribers it started,
and each monitor pass, right after ensuring the groups, restarts those that
are stopped and whose stop() has finished. The groups exist by then, so
FastStream's own XGROUP CREATE in start() is a BUSYGROUP no-op and a
partial loss keeps the id="0" start (existing entries are redelivered).
A failed restart is logged and retried next pass; nothing restarts once
disconnect() has begun. No-op on FastStream <= 0.7.5.
The lock moves to faststream 0.7.7 (fast-depends 3.0.9) so CI runs the
integration tests against the new behaviour.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
- Batch mode: a lost entry still cancels the whole batch (it is one unit; none is acked, the entries still owned stay pending for retry), but LeaseLostError now carries lost_ids next to message_ids and names the lost entries. Documented. - Renewal loop: a round slower than the interval still gets a short pause (10% of the interval) instead of back-to-back rounds, and the overrun is logged once per streak. - Wrapper: if the keeper ever cancels an invocation before its handler task is attached, the task is cancelled right away. Nothing awaits in that window today (the reported race can't happen); this keeps it closed. - Subscribe: the assert guarding group / consumer / interval (stripped by python -O) is replaced by an explicit ValueError for a missing group and a non-assert type narrowing. - anyio, imported directly since sync handlers run in a worker thread, is a declared dependency with FastStream 0.6.4's own range (>=4.0,<5), so it adds no constraint. Lock: content-hash only; requirements.txt lists it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The SDK's own boolean behaviour flags are verb phrases (delete_on_ack, retry_on_error, cancel_on_lease_lost) without an auto_ prefix; the noun lease_renewal was the odd one out (review feedback). The other lease options already follow the existing conventions and keep their names: lease_renewal_interval_ms matches <object>_<action>_interval (group_monitor_interval_s, retry_reclaim_interval_s) and stays in ms like retry_backoff_max_ms, being bounded by and defaulting to a third of retry_on_idle_ms; max_processing_ms matches max_*. Not released yet (only under [Unreleased]), so no alias. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…rval_ms Shares the flag's prefix, like the retry options do (retry_on_idle_ms, retry_reclaim_interval_s, retry_backoff_*), so the lease options group together: renew_lease, renew_lease_interval_ms (verb + object + interval, like retry_reclaim_interval_s). The docs table now also states that the interval and cancel_on_lease_lost require renew_lease=True, as the docstring and the validation already do. Not released yet, so no alias. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@valdis good call, done:
I skipped the Final set: |
…se-renewal Keep this PR's CHANGELOG entries under [Unreleased]; main released 0.6.0 in the meantime. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…adline Cancelling the task that awaits anyio.to_thread.run_sync returns at once while the thread keeps running. Past max_processing_ms the entry was then NACKed and no longer renewed, so the reclaimer redelivered it while the first thread was still working: the parallel duplicate the lease exists to prevent. Sync handlers are now never cancelled: the wrapper waits for the thread (still leased past the deadline), then discards the result and raises ProcessingTimeoutError / LeaseLostError. Docs and docstrings now match. Co-Authored-By: Claude Opus 5.5 <[email protected]>
With max_workers > 1 (or explicit max_records > 1), every id that finished beside a long-running handler was kept until that handler ended. The prefetch scan now drops finished ids it no longer finds in the PEL (acked or reclaimed); NACKed ones, and ids that finished while the scan ran, are kept so they are still never renewed. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A prefetched entry seen by the scan that started, finished and was acked while the renewal script was in flight came back 'not pending' and was logged as 'Lease lost for queued entry ... before its handler started', leaving a stale id in the lost-queued set. Ids whose run began during the round are now skipped there. Co-Authored-By: Claude Opus 5.5 <[email protected]>
renew_lease needs SCRIPT LOAD / EVALSHA. A server that refuses them (ACL without @scripting, managed-Redis policy) only showed up as a renewal warning every interval. LeaseManager.start() now loads the script up front and logs a clear error if the server refuses it; it never raises, so the transport still starts. Co-Authored-By: Claude Opus 5.5 <[email protected]>
It shared poetry-<os>-py3.10-<lock hash> with the regular Python 3.10 test job, so a venv with FastStream 0.6.4 installed over the lock could be cached and restored there. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The group monitor restarts any stream subscriber FastStream stopped, but logged every restart as caused by a lost consumer group. The cause isn't known there; the message now says what was restarted and gives NOGROUP only as an example. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Compare the retry count through int(), so the lease tests pass whether the reclaimer writes _retry_count as a string (today) or as a number (#293). Co-Authored-By: Claude Opus 5.5 <[email protected]>
The max_records default applies unless a number is given: an explicit max_records=None is treated like an unset one (as in #304), which 'unless set' did not say. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- LeaseKeeper.in_flight returns a read-only view. - _stream_group() no longer raises on a stream_sub without a name. - renew_lease with retry_on_idle_ms < 2 is rejected: the default interval (retry_on_idle_ms // 3, at least 1) would not be below it, which the explicit-interval check already refuses. - LeaseLostError and ProcessingTimeoutError are in the Redis transport API reference. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The Redis transport section is about 30% shorter with every caveat kept (sync handlers, blocked event loop, trimmed vs lost entries, batches, prefetch, scripting, at-least-once). The README paragraph is cut to a summary plus a link, and the CHANGELOG entry to a few bullets. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Went through the QualOps findings (plus an internal review) and pushed fixes in eb65982..b28d656. Fixed
Not changed
Other fixes in the same push:
203 passed / 12 skipped, also on FastStream 0.6.4. |
Description
Problem. With Redis Streams consumer groups and SDK-managed retries (
retry_on_idle_ms), the reclaimer treats any PEL entry idle for longer thanretry_on_idle_msas abandoned. Redis resets an entry's idle time only when it is (re)delivered, not while a handler is working on it. So a handler that runs longer thanretry_on_idle_ms(multi-minute LLM calls, in our case) has its entry reclaimed and redelivered through the.retrystream while the first run is still going: the same message gets processed twice, in parallel. The only workaround today is a very longretry_on_idle_ms, which delays crash recovery by the same amount.Fix. An opt-in in-flight lease:
subscribe(..., renew_lease=True)keeps renewing the entries a subscription is working on, so the reclaimer skips them. A crashed consumer stops renewing, and its entries are still reclaimedretry_on_idle_msafter the last renewal.Also in this PR (see FastStream compatibility): consuming resumes after a lost consumer group on FastStream ≥ 0.7.6, the minimum FastStream is raised to 0.6.4 (older 0.6 releases never worked), and CI now tests the lowest supported FastStream.
Type of Change
Design
renew_lease_interval_ms, one Lua script runs per chunk of up to 100 ids. For each entry it checks that the entry is still pending for this consumer (XPENDING <id> <id> 1) and still in the stream (XRANGE), then runsXCLAIM <stream> <group> <same consumer> 0 <id> JUSTID. The owner stays the same, the delivery count doesn't move (JUSTID), and the idle time resets. The reclaimer's ownXCLAIMpassesmin-idle-time = retry_on_idle_ms, so a renewed entry is skipped even if it was already on the reclaimer's candidate list. The ownership check sits in the same script because a bareXCLAIM ... 0would take the entry back from the reclaimer. Per-id result: renewed / lost / trimmed.interval / 2.{channel}.{handler}.retry, read by its own group (<handler>-retry) and a per-process consumer. The recursive retry subscribe gets its own keeper, with that group and consumer.StreamSubscriber._get_msgsrunsXREADGROUP COUNT=max_recordsand then callsconsume_onefor each message in turn. Withmax_records=NonenoCOUNTis sent, so the whole backlog is read at once. Every entry read is already in this consumer's PEL, and the queued ones go idle behind a slow handler. This PR handles that in two ways:XPENDINGpage of 1000 per round in practice). This coversmax_records > 1andmax_workers > 1. Entries whose handler already finished (NACKed failures) are excluded, so retries still happen. Entries older than the oldest run in flight (left by an earlier process with the same consumer name, e.g. a restarted container with the same host and pid) are left for the reclaimer. Forbatch=True, every id in the batch is renewed while the batch handler runs.renew_lease=Truedefaultsmax_recordsto 1 (unless it is set explicitly orbatch=True). A leased entry stays with the worker that read it, so with long handlers you want an idle worker to take the next entry rather than a busy one holding it.retry_on_idle_ms, e.g. a Redis outage): by default the handler (running in a child task) is cancelled andeggai.transport.LeaseLostErroris raised in place of its result, so it can't overlap its redelivery. The entry is not acked. A batch is one unit: if any of its entries is lost the batch handler is cancelled and none is acked (the entries still owned stay pending and are retried);LeaseLostError.lost_idsnames the lost entries,message_idsthe whole batch. A prefetched entry found lost before its handler started raises without running the handler. Withcancel_on_lease_lost=Falsethe SDK logs an error and the handler finishes.MAXLEN/XTRIM/XDEL): not a lost lease. Nothing can redeliver it, so the handler finishes, a warning is logged, and renewal stops for that id. Only an entry this consumer still owns counts as trimmed. An entry that left the PEL counts as lost even if it is gone from the stream, because withdelete_on_ackthe reclaimerXDELs the original when it moves it to.retry.asyncio.wait, which never raises the child's cancellation, so the wrapper tells its own cancellation (agent stop, forwarded to the handler) from an SDK-initiated one (lost lease, deadline) withoutTask.cancelling(), which Python 3.10 doesn't have. A handler that cancels itself still propagatesCancelledError.NOGROUP) are logged with stream, group, consumer and ids, and retried at the next interval; they never reach the consumer loop. A message without stream ids runs unleased, with a warning. Renewal starts inconnect()before the consumers start (and right away for a lease subscription added afterconnect()), and stops indisconnect()afterbroker.stop(), so handlers finishing during a graceful shutdown keep their leases. If the broker fails to start, the renewal tasks are stopped again. A subscribe that fails rolls back exactly the keepers it (and its retry subscribe) created; a second registration of the same stream/group/consumer with different lease settings is refused instead of silently replacing a keeper that is in use.max_processing_ms. The SDK had no handler deadline, and a lease makes one necessary: a hung handler would otherwise hold its lease forever. A handler still running at the deadline is cancelled,ProcessingTimeoutError(aTimeoutError) is raised, and the entry is NACKed. It then goes through the normal retry path and counts towardsmax_retries. It also works withoutrenew_lease.delete_on_ack( lease( tracing( filters( handler ) ) ) ), so renewal stops before the entry is acked or deleted. The sync-handler path (call_handler: worker thread, awaitable results awaited) is now shared withdelete_on_ackinstead of being duplicated;is_async_callablealso recognisesfunctools.partialof async callables.Options
Naming follows the SDK's existing conventions (review feedback): boolean behaviour flags are verb phrases like
delete_on_ack/retry_on_error, hencerenew_leaseandcancel_on_lease_lost; a feature's options share its prefix likeretry_on_idle_ms/retry_reclaim_interval_s/retry_backoff_*, hencerenew_lease_interval_ms; every SDK-defined duration carries its unit, in ms when it is measured againstretry_on_idle_ms(likeretry_backoff_max_ms): the interval is bounded by and defaults to a third of it; limits aremax_*.renew_leaseFalseretry_on_idle_msand a consumer name. Rejected withno_ack=TrueandAckPolicy.MANUAL(an entry acked mid-run would look like a lost lease).renew_lease_interval_msretry_on_idle_ms // 3< retry_on_idle_ms. The default tolerates two failed renewals in a row. Requiresrenew_lease=True.cancel_on_lease_lostTruerenew_lease=True.max_processing_msNoneretry_on_idle_ms.These are validated in
Agent.subscribe(at decoration time, same checks, via a sharedresolve_lease_options) and inRedisTransport.subscribe, before anything is registered on the broker.Trueis not accepted as a millisecond value.LeaseLostErrorandProcessingTimeoutErrorare exported fromeggai.transport.FastStream compatibility
Testing across the allowed
faststream>=0.6,<0.8range (CI only ever installed the locked version) turned up two problems at either end, fixed here:NOGROUPhandling: it used to raise, and the task supervisor restarted the read, so reading resumed once the SDK's group monitor recreated the group. Now it stops the subscriber for good ("restart the application"). The monitor still recreated the group, but nothing read from it (test_nogroup_recovery_via_monitorandtest_partial_group_loss_redelivers_existing_messagesfailed on 0.7.7, onmaintoo).connect()now remembers the consumer-group stream subscribers it started, and each monitor pass, right after ensuring the groups, restarts those that are stopped and whosestop()has finished. The group exists by then, so FastStream's ownXGROUP CREATEis aBUSYGROUPno-op, and a partial loss still redelivers from id0. A failed restart is logged and retried; nothing restarts oncedisconnect()has begun. No-op on FastStream ≤ 0.7.5. The lock moves to FastStream 0.7.7 (fast-depends 3.0.9, nothing else) so CI covers this path;requirements.txtfollows.>=0.6.4,<0.8, was>=0.6). The SDK never worked on the older 0.6 releases: on 0.6.0–0.6.2 every Redis stream subscribe raisedTypeError(StreamSubhas nomin_idle_timebefore 0.6.3, and the SDK always passes it), and on 0.6.3 amin_idle_timesubscription never claimed pending entries. No setup that works today breaks; the failure moves from runtime to install time. The two backoff tests that failed on 0.6.x were a test-helper issue (BinaryMessageFormatV1.encode()is sync in 0.6, async in 0.7); the SDK only usesparse(), which is sync in both.anyiois now a declared dependency (>=4.0,<5, the range FastStream 0.6.4 already requires, so no new constraint): the SDK imports it directly to run sync handlers in a worker thread (redis.pyalready did onmain).test-min-faststreamruns the whole suite on Python 3.10 with FastStream 0.6.4 (installed over the lock) and is part of theall-checks-passedgate.Compatibility
Lease renewal is fully opt-in. Without these options, subscriptions are unchanged: no lease client, no renewal task, no extra wrapper (a test asserts
transport._lease_manager is None). No change to the reclaimer, stream keys or message shape. Requirements: Lua scripting (EVALSHA) on the server;XPENDING/XCLAIM JUSTIDare Redis ≥ 5. Tested against Redis 7 (OSS).The FastStream changes don't alter behaviour on versions where the SDK already worked (≤ 0.7.5); the subscriber restart only fires for subscribers FastStream stopped.
Limits (documented): a sync handler runs in a worker thread and can't be interrupted, so a lost lease or a missed deadline takes effect when the thread returns. A handler that blocks the event loop also blocks renewal; that includes a sync handler combined with
data_type/filter_by_message, which the (pre-existing) filter wrapper calls on the event loop. Delivery stays at-least-once: the lease removes the parallel duplicate, not the redelivery after a crash.Review findings (QualOps)
All findings from both QualOps runs were checked against the code.
First run:
connect()never started (007), unbounded sequentialXPENDINGpaging (008), pruning against a stale lower bound (001), empty message ids silently unleased (002),is_async_callablemissingfunctools.partial(003), a rollback that re-derived the retry consumer name (wrong with a customconsumer=),add()replacing a keeper whose task kept running the old one, fragileraw_message["message_ids"]access, bool/int option validation gaps.resolve_lease_optionsmutates kwargs" (read-only; now documented), "min()can raise ValueError" (no await between the empty check andmin()), "retry subscribe reuses the main stream's coordinates" (only the options are passed), "Redis URL stored in plaintext" (same as the rest of the transport).Second run (after the first round of fixes):
assertguarding group / consumer / interval (stripped bypython -O) is now an explicitValueError; a renewal round slower than the interval no longer triggers back-to-back rounds;anyiois declared as a dependency;LeaseLostErrornames the lost entries of a batch (lost_ids).begin()and attaching the handler task" (high). Nothing awaits in that window, so the keeper can't run there; the wrapper now cancels the task right away if the invocation was already cancelled, so the window stays closed if anawaitis ever added.lost_ids.CodeQL: no alerts ("No new alerts in code changed by this pull request").
Testing
All CI jobs reproduced locally in containers, each against its own throwaway
redis:7-alpine:lint:ruff check,ruff format --check,mypyclean (FastStream 0.7.7).teston Python 3.10, 3.11, 3.12, 3.13 with the lock (FastStream 0.7.7): 193 passed, 12 skipped each (Kafka, not available).test-min-faststream, Python 3.10 with FastStream 0.6.4: 193 passed, 12 skipped.build: wheel and sdist passtwine check; a clean install importseggai, the transports and the new exceptions.delete_on_acksuites on Python 3.13 with FastStream 0.7.7.sdk/tests/test_redis_lease_renewal.py(61 tests), against real Redis:retry_on_idle_ms=500runs exactly once with the lease. While it runs, the PEL entry keeps its owner,times_delivered == 1, and idle stays below 500 ms. Without the lease the same handler runs twice, in parallel (the contrast test reproduces the bug)..retry. Stopping renewal mid-run lets the entry be reclaimed. A stale entry left under the same consumer name by an earlier process is not renewed.LeaseLostErroris logged. Withcancel_on_lease_lost=Falsethe handler finishes. AnXDEL'd entry lets the handler finish, with a warning.max_records=5andmax_workers=2each have a with/without contrast (redelivered without the lease, exactly once with it). The defaultmax_records=1is checked. Batch: every id in the batch is renewed.max_processing_mscancels a hung handler, and the entry is retried via.retry.agent.stop()cancels a running handler, stops the renewal tasks and closes the client. A lease subscription added afterconnect()gets its renewal task immediately.consumer=) and keeps a keeper an earlier subscribe registered.lost_ids; a run cancelled before its task exists never starts; the prefetch scan pages through a large PEL; pruning uses the state after the round; a lost status cancels only its own invocation; finished (NACKed) entries are never renewed; a queued entry lost before start raises without running; results, errors, outer cancellation (also while a timed-out handler unwinds) and a self-cancelling handler pass through the wrapper; missing message ids run unleased with a warning;is_async_callablewith partials, async__call__andAsyncMock;ProcessingTimeoutError; keeper reuse / conflict / discard; option validation on bothAgent.subscribeandRedisTransport.subscribe.sdk/tests/test_redis.py: new unit test for the group monitor's subscriber restart (restarts stopped subscribers, leaves running and still-stopping ones alone, retries a failed restart, never restarts afterdisconnect()began).mkdocslocally for the Redis transport page; the new section and API docstring render.Changelog
sdk/CHANGELOG.mdunder[Unreleased]section (Added: lease renewal,max_processing_ms; Changed: minimum FastStream 0.6.4,anyiodeclared; Fixed:NOGROUPrecovery on FastStream ≥ 0.7.6)Checklist
make lintpasses)make formatapplied)docs/docs/sdk/redis-transport.md, SDK README, and theRedisTransport.subscribedocstring (the SDK's typed option reference)Additional Notes
There is no retry example in this repository (examples live in eggai-examples), so the docs section carries the usage example.
🤖 Generated with Claude Code