chore: Separate status check from synchronizer functionality #373
Conversation
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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
keelerm84
force-pushed
the
mk/sdk-1579/improve-recovery
branch
from
cbcd3d6 to
7e0dbce
Compare
jsonbailey
approved these changes
Nov 17, 2025
| synchronizer.stop() | ||
| timer.stop() | ||
|
|
||
| sync_reader.join(0.5) |
Contributor
There was a problem hiding this comment.
What is the purpose of the half second timeout here? My understanding is that if it doesn't stop in .5 seconds, execution will move on and that thread will still continue to process.
Contributor
There was a problem hiding this comment.
Not a blocker, just curious.
Member
Author
There was a problem hiding this comment.
We are trying to shut down in an orderly fashion. The timeout gives us time to close out correctly, but eventually we do have to move on if it isn't halting as expected. The idea is that if close has been called, the python interpreter is going to exit shortly after anyway.
kinyoklion
approved these changes
Nov 17, 2025
keelerm84
force-pushed
the
mk/sdk-1574/outage-detector
branch
from
85f59ba to
8e81cdb
Compare
In the previous setup, we would only check the fallback or recovery conditions once the synchronizer returned an update. If the synchronizer was stuck, or nothing was changing in the environment, we would never check the conditions. This configuration also exposed an interesting behavior. If the synchronizer cannot connect, it will emit error updates. Each time we receive an error, we check if we have failed to initialize for the last 10 seconds. If so, we re-create the primary synchronizer. When it continues to fail, the first update will trigger the condition check. And since it has still failed for 10 seconds, it will immediately error out. With this change, we can be assured a synchronizer is given at least 10 seconds to try before the condition is evaluated.
keelerm84
force-pushed
the
mk/sdk-1579/improve-recovery
branch
from
7e0dbce to
38fb5f4
Compare
| synchronizer.stop() | ||
| timer.stop() | ||
|
|
||
| sync_reader.join(0.5) |
There was a problem hiding this comment.
Bug: Thread Termination Failure Leads to Instability
The sync_reader thread is given only a 0.5-second timeout to exit. If it hasn't finished within that time, execution continues while the thread is still running, potentially causing race conditions or resource leaks when a new synchronizer is subsequently created and another reader thread is started.
keelerm84
force-pushed
the
mk/sdk-1579/improve-recovery
branch
2 times, most recently
from
1799cee to
644cc13
Compare
keelerm84
force-pushed
the
mk/sdk-1579/improve-recovery
branch
2 times, most recently
from
842e8fa to
e578a2e
Compare
keelerm84
force-pushed
the
mk/sdk-1579/improve-recovery
branch
from
e578a2e to
a37fa56
Compare
ldclient/impl/datasystem/store.py
Outdated
|
|
||
| # Send change events | ||
| if affected_items: | ||
| print("@@@@@ affected items are (delta)", affected_items, "\n") |
There was a problem hiding this comment.
Bug: Debug print statements left in production code
Debug print() statement with marker @@@@@ left in production code at line 374. This should be removed before merging.
keelerm84
force-pushed
the
mk/sdk-1579/improve-recovery
branch
from
a37fa56 to
2ca4af0
Compare
keelerm84
added a commit
that referenced
this pull request
Nov 19, 2025
In the previous setup, we would only check the fallback or recovery conditions once the synchronizer returned an update. If the synchronizer was stuck, or nothing was changing in the environment, we would never check the conditions. This configuration also exposed an interesting behavior. If the synchronizer cannot connect, it will emit error updates. Each time we receive an error, we check if we have failed to initialize for the last 10 seconds. If so, we re-create the primary synchronizer. When it continues to fail, the first update will trigger the condition check. And since it has still failed for 10 seconds, it will immediately error out. With this change, we can be assured a synchronizer is given at least 10 seconds to try before the condition is evaluated.
keelerm84
added a commit
that referenced
this pull request
Nov 19, 2025
In the previous setup, we would only check the fallback or recovery conditions once the synchronizer returned an update. If the synchronizer was stuck, or nothing was changing in the environment, we would never check the conditions. This configuration also exposed an interesting behavior. If the synchronizer cannot connect, it will emit error updates. Each time we receive an error, we check if we have failed to initialize for the last 10 seconds. If so, we re-create the primary synchronizer. When it continues to fail, the first update will trigger the condition check. And since it has still failed for 10 seconds, it will immediately error out. With this change, we can be assured a synchronizer is given at least 10 seconds to try before the condition is evaluated.
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.
In the previous setup, we would only check the fallback or recovery
conditions once the synchronizer returned an update. If the synchronizer
was stuck, or nothing was changing in the environment, we would never
check the conditions.
This configuration also exposed an interesting behavior. If the
synchronizer cannot connect, it will emit error updates. Each time we
receive an error, we check if we have failed to initialize for the last
10 seconds. If so, we re-create the primary synchronizer.
When it continues to fail, the first update will trigger the condition
check. And since it has still failed for 10 seconds, it will immediately
error out. With this change, we can be assured a synchronizer is given
at least 10 seconds to try before the condition is evaluated.
Note
Separates FDv2 fallback/recovery checks from synchronizer emissions via a timer+reader thread, fixes initial status timestamp, removes streaming context manager hooks, and skips flaky file-source tests in CI.
_consume_synchronizer_resultsnow uses a queue, background reader thread, andRepeatingTasktimer to trigger regular status checks and act without waiting for sync updates.DataSourceStatus.sincewithtime.time()inDataSourceStatusProviderImpl.__enter__,__exit__) fromStreamingDataSource.test_two_phase_initto ensure version progression and add readiness/change events.LD_SKIP_FLAKY_TESTSis set.LD_SKIP_FLAKY_TESTS: truefor test steps on Linux and Windows jobs.Written by Cursor Bugbot for commit fa34244. This will update automatically on new commits. Configure here.