fix: stop repeating tasks without waiting out the current interval - #435
fix: stop repeating tasks without waiting out the current interval#435beekld wants to merge 1 commit into
Conversation
Co-Authored-By: Bee Klimt <bklimt@launchdarkly.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@cursor review |
|
I'm working on fixing the flaky tests and this looks to be a dupe of #432 though not all of it. |
I'm happy to defer to your fix. Thanks! |
|
Agreed — the The non-overlapping parts are split into #437: |
Fixes the flakiness in
spec/impl/data_system/fdv2_datasystem_spec.rb, whose root cause is a real stall inRepeatingTask#stop.RepeatingTask#stopno longer blocks for up to a full interval (10s for the FDv2 synchronizer condition timer,poll_intervalfor polling); it now signals an event the worker waits on instead of relying onThread#runenvironment_idis populated once initialization completesFDv2#stopalso joins synchronizer threads started while it was shutting downRequirements
Related issues
Flaky failures seen on #434 CI (
environment ID is retained from a valid synchronizer update,FDv1 fallback with initializer ...). Both reproduce onmain.Implementation details
Root cause 1 —
RepeatingTask#stopwaits out the sleep.stopcalled@worker.runto wake the worker out ofsleep, then joined it.Thread#runonly helps if the worker is already sleeping; whenstopruns before the worker reaches itssleep, the wakeup is lost andjoinblocks for the whole delay. Measured onmain, stopping a task with a 10s start delay immediately afterstartblocks 10.0s every time.FDv2 creates such a timer (
interval 10, start_delay 10) for every synchronizer attempt and stops it in theensureofconsume_synchronizer_results, so every synchronizer transition — fallback to the secondary, FDv1 fallback, removal of a failed synchronizer, shutdown — could be delayed by up to 10 seconds. The specs wait 2-5s for those transitions, hence the flakes. Same hazard in production forLD/PollingDataSource(stopcould block a poll interval) and the store availability checkers.The worker now waits on a
Concurrent::Eventfor both the start delay and the inter-run delay;stopsets it and joins.Root cause 2 — ready event set before the environment ID is recorded.
consume_synchronizer_resultsset@ready_eventand then calledrecord_environment_id, so a caller unblocked by initialization could observeenvironment_id == nil. That is theexpected: "env-abc" got: nilfailure on #434, and the same race is visible to SDK users afterwait_for_initialization. Order is now reversed.Root cause 3 — count-based wait in the FDv1-fallback-with-initializer spec. Falling back replaces the store basis, producing three flag change events (
initialflagput,initialflagdelete,fdv1replacementflagput). The spec unblocked after 2 events and then asserted both keys were present, so it failed whenever the delete landed before the FDv1 put (expected ["initialflag", "initialflag"] to include "fdv1replacementflag"). It now waits for both keys.Also fixed:
FDv2#stopiterated@threadswithout synchronization, so a synchronizer thread appended by the main loop during shutdown was never joined. In the specs this left threads calling mock doubles after the example ended (RSpec::Mocks::OutsideOfExampleErroron stderr).Verification (Ruby 3.2,
bundle exec rspec spec/impl/data_system/fdv2_datasystem_spec.rb, 25 consecutive runs):mainFull suite and
rubocopclean. Not run under JRuby locally; CI covers it.Link to Devin session: https://app.devin.ai/sessions/2f339ab061a440e1ba42bb7ab0675b35
Open in Devin Desktop: https://app.devin.ai/desktop/session/2f339ab061a440e1ba42bb7ab0675b35?variant=devin
Requested by: @beekld
Note
Overview
RepeatingTask now uses a
Concurrent::Eventfor start delay and inter-run waits sostopcan wake the worker immediately instead of relying onThread#run(which could miss the sleep and block for a full interval). That affects FDv2’s synchronizer condition timer, polling, and store availability checkers.FDv2 records
environment_idbefore signaling ready on valid synchronizer updates, so callers unblocked by initialization see a populated ID. Thread bookkeeping is synchronized when appending to@threads, andstoploops until all tracked threads are joined, including ones started during shutdown.The FDv1 fallback with initializer spec waits until both expected flag keys appear rather than unblocking after a fixed change count.
Reviewed by Cursor Bugbot for commit 1e3bd93. Bugbot is set up for automated code reviews on this repo. Configure here.