fix: Do not propagate persistent-store errors from the sync FDv2 warm-start check - #506
Open
jsonbailey wants to merge 3 commits into
Open
fix: Do not propagate persistent-store errors from the sync FDv2 warm-start check#506jsonbailey wants to merge 3 commits into
jsonbailey wants to merge 3 commits into
Conversation
The persistent-store error catch moves out of the client and into FDv2.data_availability, which now degrades to DEFAULTS itself instead of letting a store I/O error propagate. This makes the availability gate total for every caller: is_initialized() reads the gate directly and so was not covered by the previous client-side guard.
… them Store.close() previously returned the close error as Optional[Exception], which the only caller (FDv2.stop) discarded, so a failed close was silently lost. It now logs a warning and returns None. Closing happens at shutdown, where there is no caller left to react to the error.
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.
The bug
Sync FDv2, when configured with a persistent store (e.g. Redis), could throw a store I/O error out of
variation()/variation_detail()/all_flags_state()during the warm-start window (before a data source initializes), instead of degrading toCLIENT_NOT_READY/ the default. Avariation()must never throw.The path:
LDClient._evaluate_internal/all_flags_statereadself._data_system.data_availabilitybefore their own try/except. For FDv2,data_availabilitycallsself._store.is_initialized()while no basis has arrived, which reaches the persistent store'sinitialized_internal()— a blocking query (e.g. redisexists()) that raises on a store error and was not caught.__evaluate_with_hooksdoes not catch exceptions fromblock(), so the error propagated out ofvariation(). (The store'sis_available()is wrapped safe, butinitialized_internal()was not — that asymmetry is the root.)The fix
Make the availability gate total:
FDv2.data_availabilitynow wraps the persistent-store read in atry/exceptand degrades toDataAvailability.DEFAULTSon error, logging it. A store I/O error therefore never leaves the gate —variation()returns the default withCLIENT_NOT_READYandall_flags_state()returns an invalid state.The catch lives in the gate rather than at the client call sites so it covers every caller. In particular,
is_initialized()readsdata_availabilitydirectly, so a client-side-only guard would have leftis_initialized()able to throw. This mirrors the async gate in #486, keeping the two implementations symmetric.The catch is scoped to the store read only; the
REFRESHEDandCACHEDchecks around it are untouched, so a real logic error there is not masked.Also: log persistent-store close errors
Store.close()previously returned the close error asOptional[Exception], which its only caller (FDv2.stop) discarded — so a failed close was silently lost. It now logs a warning and returnsNone. Closing happens at shutdown, where there is no caller left to react to the error. (Pre-existing since 9.16.0, not introduced by recent refactors; the async counterpart is in #486.)Also corrects a stale docstring on
FDv2._consume_synchronizer_results(described a tuple return; returns a singleConditionDirective).Validation
test_variation_does_not_throw_when_persistent_store_errors_during_warm_start: a persistent store whoseinitializedraises, with a synchronizer configured but no basis yet. Asserts the gate now returnsDEFAULTS(rather than raising), and that through the clientvariation()/variation_detail()return the default withCLIENT_NOT_READYandall_flags_state()is invalid — no exception.test_persistent_store_close_logs_and_swallows_error: a persistent store whoseclose()raises;Store.close()logs a warning and does not raise.mypy,isort,pycodestyleclean.Scope
Sync FDv2 only. Sync FDv1 reads
data_availabilitythe same way; out of scope here.Note
Overview
Sync FDv2 no longer lets persistent-store I/O failures escape
variation(),variation_detail(), orall_flags_state()during warm-start (before a data source has supplied a basis).FDv2.data_availabilitynow catches errors from the persistent-storeinitializedcheck, logs them, and reportsDataAvailability.DEFAULTSso the client returns defaults withCLIENT_NOT_READY(or an invalidall_flags_state) instead of raising.Store.close()logs a warning when closing the persistent store fails instead of returning a discarded exception.LDClientevaluation paths cachedata_availabilityin a local variable when branching on readiness (behavior unchanged).Adds regression tests for throwing
initializedduring warm-start and for close-error logging; fixes a stale docstring on_consume_synchronizer_results.Reviewed by Cursor Bugbot for commit 6a5dd9e. Bugbot is set up for automated code reviews on this repo. Configure here.