fix: Drive repeating tasks from a delay source - #521
Merged
Merged
Conversation
jsonbailey
marked this pull request as ready for review
September 14, 2026 16:35
joker23
approved these changes
Sep 15, 2026
tanderson-ld
self-requested a review
September 15, 2026 15:11
tanderson-ld
approved these changes
Sep 15, 2026
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.
BEGIN_COMMIT_OVERRIDE
feat: Drive repeating tasks from a delay source
fix: Repeating task wait duration is no longer reduced by a slow callback duration.
END_COMMIT_OVERRIDE
Summary
Repeating tasks now take their wait from a delay source rather than a fixed interval, and the wait starts when the callback returns rather than when it began.
Groundwork for RETRY-spec conformance, which needs polling to vary its interval after a failure. Nothing varies it yet — every caller here uses the fixed-interval path — so this is a refactor plus one behavior change, reviewable on its own.
Behavior change
The wait now starts when the callback returns. Previously the interval was measured from the start of each invocation and the callback's own duration was subtracted, so a slow callback shortened the following wait. A flush configured every 5 seconds that takes 200ms used to run every 5s; it now runs 5.2s apart.
Bounded by the callback's duration, and
poll_intervalis already documented as a minimum rather than an exact rate. The reason for changing it: with a variable delay, subtracting the attempt's duration means a slow failure gets less backoff than a fast one, which is backwards — a slow failure more likely means a struggling service.What changed
ldclient/impl/delay.py(new) — aDelaySourceprotocol with a single read-only property, andFixedDelayfor the constant case. It imports nothing buttyping, so both schedulers can depend on it and neither owns it.impl/repeating_task.pyandimpl/aio/concurrency.py— both take aDelaySource, with anat_intervalfactory for the fixed case. Ten call sites change by one word.start()methods are now idempotent, logging and returning instead of raising.AsyncRepeatingTaskpreviously raisedRuntimeError; sync inheritedThread.start(), which also raises. The motivation is thatAsyncLDClient.start()is documented as an idempotent no-op, so a raise from an internal primitive could surface out of a call the caller was told is safe to repeat.Testing
make test: 1536 passed.make lint: clean across 226 files.test_async_polling.py::test_second_start_call_raisesbecame..._is_a_no_op— it asserted the behavior this change removes.Gap semantics was verified by measurement rather than by reading: a 0.2s callback on a 0.2s interval now produces 0.41s between invocation starts, where the old arithmetic produced back-to-back runs. No existing test asserted an upper bound on a period, which is the shape that would have broken; the one period assertion is a lower bound, which gap semantics can only satisfy more easily.
Note
Overview
Introduces a shared
DelaySource(FixedDelayfor constant waits) and refactors syncRepeatingTaskandAsyncRepeatingTaskto read **next_delayafter each callback instead of taking a bare interval. Call sites move toat_interval(...); nothing uses variable delays yet, but the shape supports future retry/backoff behavior.Timing change: the wait between runs now starts when the callback returns, not when it begins—slow work no longer shortens the following sleep (e.g. a 5s flush plus 200ms work is ~5.2s between starts, not ~5s).
Lifecycle change:
start()is idempotent on both paths (log and ignore duplicate starts instead of raising), matching callers like idempotent clientstart(). Async repeating tasks also log unhandled background failures via a done callback.Tests add coverage for delay reads, gap semantics, idempotent start, and start-after-stop not resuming.
Reviewed by Cursor Bugbot for commit 8d7515e. Bugbot is set up for automated code reviews on this repo. Configure here.