Skip to content

fix: stop repeating tasks without waiting out the current interval - #435

Closed
beekld wants to merge 1 commit into
mainfrom
devin/1788896862-fix-flaky-fdv2-spec
Closed

fix: stop repeating tasks without waiting out the current interval#435
beekld wants to merge 1 commit into
mainfrom
devin/1788896862-fix-flaky-fdv2-spec

Conversation

@beekld

@beekld beekld commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Fixes the flakiness in spec/impl/data_system/fdv2_datasystem_spec.rb, whose root cause is a real stall in RepeatingTask#stop.

  • RepeatingTask#stop no longer blocks for up to a full interval (10s for the FDv2 synchronizer condition timer, poll_interval for polling); it now signals an event the worker waits on instead of relying on Thread#run
  • FDv2 records the environment ID from a synchronizer update before setting the ready event, so environment_id is populated once initialization completes
  • FDv2#stop also joins synchronizer threads started while it was shutting down
  • One spec waits for the flag keys it asserts on instead of a change count

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Flaky failures seen on #434 CI (environment ID is retained from a valid synchronizer update, FDv1 fallback with initializer ...). Both reproduce on main.

Implementation details

Root cause 1 — RepeatingTask#stop waits out the sleep. stop called @worker.run to wake the worker out of sleep, then joined it. Thread#run only helps if the worker is already sleeping; when stop runs before the worker reaches its sleep, the wakeup is lost and join blocks for the whole delay. Measured on main, stopping a task with a 10s start delay immediately after start blocks 10.0s every time.

FDv2 creates such a timer (interval 10, start_delay 10) for every synchronizer attempt and stops it in the ensure of consume_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 for LD/PollingDataSource (stop could block a poll interval) and the store availability checkers.

The worker now waits on a Concurrent::Event for both the start delay and the inter-run delay; stop sets it and joins.

Root cause 2 — ready event set before the environment ID is recorded. consume_synchronizer_results set @ready_event and then called record_environment_id, so a caller unblocked by initialization could observe environment_id == nil. That is the expected: "env-abc" got: nil failure on #434, and the same race is visible to SDK users after wait_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 (initialflag put, initialflag delete, fdv1replacementflag put). 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#stop iterated @threads without 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::OutsideOfExampleError on stderr).

Verification (Ruby 3.2, bundle exec rspec spec/impl/data_system/fdv2_datasystem_spec.rb, 25 consecutive runs):

failures
main 28 failures across 25 runs, 6 distinct examples
this branch 0

Full suite and rubocop clean. 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::Event for start delay and inter-run waits so stop can wake the worker immediately instead of relying on Thread#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_id before signaling ready on valid synchronizer updates, so callers unblocked by initialization see a populated ID. Thread bookkeeping is synchronized when appending to @threads, and stop loops 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.

Co-Authored-By: Bee Klimt <bklimt@launchdarkly.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration

Copy link
Copy Markdown
Contributor

@cursor review

@beekld
beekld marked this pull request as ready for review September 8, 2026 20:28
@beekld
beekld requested a review from a team as a code owner September 8, 2026 20:28
@jsonbailey

Copy link
Copy Markdown
Contributor

I'm working on fixing the flaky tests and this looks to be a dupe of #432 though not all of it.

@beekld

beekld commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

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!

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Agreed — the RepeatingTask#stop lost wake-up is the same root cause and #432 covers it (with better spec coverage), so I've closed this out in favor of yours.

The non-overlapping parts are split into #437: environment_id recorded before @ready_event.set, @threads registration/join race in FDv2#stop (the FDv2-synchronizers thread can be registered after stop snapshots the list, which is where the ExpiredTestDoubleError output comes from), and the FDv1-fallback spec waiting on "2 change events" instead of the two flag keys it asserts. Verified green only with #432 applied, so #437 is a follow-on to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants