From bd6a1faaa84d4af7bd5ce355eef1f570b69e1130 Mon Sep 17 00:00:00 2001 From: jsonbailey Date: Thu, 27 Aug 2026 10:15:11 -0500 Subject: [PATCH 1/3] fix: Do not propagate persistent-store errors from the sync FDv2 warm-start check --- ldclient/client.py | 26 ++++++-- ldclient/impl/datasystem/fdv2.py | 2 +- .../impl/datasystem/test_fdv2_persistence.py | 66 +++++++++++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/ldclient/client.py b/ldclient/client.py index a26e1ada..f923a46d 100644 --- a/ldclient/client.py +++ b/ldclient/client.py @@ -402,14 +402,31 @@ def evaluate(): hook_result = self.__evaluate_with_hooks(key=key, context=context, default_value=default_stage.value, method="migration_variation", block=evaluate) return hook_result.results['default_stage'], hook_result.results['tracker'] + def _data_availability(self) -> DataAvailability: + """Reads the current data availability, degrading to ``DEFAULTS`` on error. + + During the warm-start window (before a data source initializes) the + availability gate may query a persistent store, such as Redis. A store + I/O error must not propagate out of an evaluation, so treat it as "no + data available"; the caller then returns the default value with + ``CLIENT_NOT_READY`` instead of raising. + """ + try: + return self._data_system.data_availability + except Exception as e: + log.error("Error checking data availability; treating data as unavailable: %s" % repr(e)) + log.debug(traceback.format_exc()) + return DataAvailability.DEFAULTS + def _evaluate_internal(self, key: str, context: Context, default: Any, event_factory) -> Tuple[EvaluationDetail, Optional[FeatureFlag]]: default = self._config.get_default(key, default) if self._config.offline: return EvaluationDetail(default, None, error_reason('CLIENT_NOT_READY')), None - if self._data_system.data_availability != DataAvailability.REFRESHED: - if self._data_system.data_availability == DataAvailability.CACHED: + availability = self._data_availability() + if availability != DataAvailability.REFRESHED: + if availability == DataAvailability.CACHED: log.warning("Feature Flag evaluation attempted before client has initialized - using last known values from feature store for feature key: " + key) else: log.warning("Feature Flag evaluation attempted before client has initialized! Feature store unavailable - returning default: " + str(default) + " for feature key: " + key) @@ -479,8 +496,9 @@ def all_flags_state(self, context: Context, **kwargs) -> FeatureFlagsState: log.warning("all_flags_state() called, but client is in offline mode. Returning empty state") return FeatureFlagsState(False) - if self._data_system.data_availability != DataAvailability.REFRESHED: - if self._data_system.data_availability == DataAvailability.CACHED: + availability = self._data_availability() + if availability != DataAvailability.REFRESHED: + if availability == DataAvailability.CACHED: log.warning("all_flags_state() called before client has finished initializing! Using last known values from feature store") else: log.warning("all_flags_state() called before client has finished initializing! Feature store unavailable - returning empty state") diff --git a/ldclient/impl/datasystem/fdv2.py b/ldclient/impl/datasystem/fdv2.py index 8939312c..6357ec61 100644 --- a/ldclient/impl/datasystem/fdv2.py +++ b/ldclient/impl/datasystem/fdv2.py @@ -392,7 +392,7 @@ def _consume_synchronizer_results( """ Consume results from a synchronizer until a condition is met or it fails. - :return: Tuple of (should_remove_sync, fallback_to_fdv1, directive) + :return: the ConditionDirective describing how to proceed """ action_queue: Queue = Queue() timer = RepeatingTask( diff --git a/ldclient/testing/impl/datasystem/test_fdv2_persistence.py b/ldclient/testing/impl/datasystem/test_fdv2_persistence.py index 1f1069e5..2d20f2c6 100644 --- a/ldclient/testing/impl/datasystem/test_fdv2_persistence.py +++ b/ldclient/testing/impl/datasystem/test_fdv2_persistence.py @@ -3,11 +3,15 @@ from threading import Event from typing import Any, Callable, Dict, List, Mapping, Optional +import pytest + +from ldclient.client import Context from ldclient.config import Config, DataSystemConfig from ldclient.impl.datasystem import DataAvailability from ldclient.impl.datasystem.fdv2 import FDv2 from ldclient.integrations.test_datav2 import TestDataV2 from ldclient.interfaces import DataStoreMode, FeatureStore, FlagChange +from ldclient.testing.test_ldclient import make_client from ldclient.versioned_data_kind import FEATURES, SEGMENTS, VersionedDataKind @@ -782,3 +786,65 @@ def init(self, all_data): assert err is not None, "Commit should return error from persistent store" assert isinstance(err, RuntimeError) assert str(err) == "Simulated persistent store failure" + + +class ThrowingInitializedStore(StubFeatureStore): + """A persistent store whose ``initialized`` check raises, to simulate a + store I/O error (for example a Redis connection failure) during the + warm-start availability gate.""" + + @property + def initialized(self) -> bool: + raise RuntimeError("persistent store I/O error") + + +def test_variation_does_not_throw_when_persistent_store_errors_during_warm_start(caplog): + """A persistent-store error at the warm-start availability gate must not + propagate out of the client. + + While a synchronizer is configured but has not yet supplied a basis, the + availability gate consults the persistent store's initialized state. If that + query raises, evaluation must degrade to the default value with + ``CLIENT_NOT_READY`` and ``all_flags_state()`` must return an invalid state, + rather than raising. This is the sync counterpart to the async fix in #486. + """ + persistent_store = ThrowingInitializedStore() + + # A synchronizer is configured but the data system is never started, so no + # basis arrives. This is the warm-start window in which the gate reads the + # persistent store's initialized state. + data_system_config = DataSystemConfig( + data_store_mode=DataStoreMode.READ_ONLY, + data_store=persistent_store, + initializers=None, + synchronizers=[TestDataV2.data_source().builder], + ) + fdv2 = FDv2(Config(sdk_key="dummy"), data_system_config) + + # The gate itself raises: this is the unguarded root the fix addresses. + with pytest.raises(RuntimeError): + _ = fdv2.data_availability + + # Drive the same failing gate through the client and confirm it degrades + # instead of propagating the error. + client = make_client() + try: + client._data_system = fdv2 + context = Context.from_dict({"key": "user", "kind": "user"}) + + assert client.variation("flag-key", context, default="default-value") == "default-value" + + detail = client.variation_detail("flag-key", context, default="default-value") + assert detail.value == "default-value" + assert detail.reason == {"kind": "ERROR", "errorKind": "CLIENT_NOT_READY"} + assert detail.is_default_value() is True + + assert client.all_flags_state(context).valid is False + finally: + client.close() + + assert any( + "Error checking data availability" in record.message + for record in caplog.records + if record.levelname == "ERROR" + ) From 064ae3797262925b68628d3a9420733b978326c2 Mon Sep 17 00:00:00 2001 From: jsonbailey Date: Thu, 27 Aug 2026 12:06:40 -0500 Subject: [PATCH 2/3] fix: Move the warm-start guard into the FDv2 gate so no caller can throw 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. --- ldclient/client.py | 20 ++----------------- ldclient/impl/datasystem/fdv2.py | 11 +++++++++- .../impl/datasystem/test_fdv2_persistence.py | 11 ++++------ 3 files changed, 16 insertions(+), 26 deletions(-) diff --git a/ldclient/client.py b/ldclient/client.py index f923a46d..2329defa 100644 --- a/ldclient/client.py +++ b/ldclient/client.py @@ -402,29 +402,13 @@ def evaluate(): hook_result = self.__evaluate_with_hooks(key=key, context=context, default_value=default_stage.value, method="migration_variation", block=evaluate) return hook_result.results['default_stage'], hook_result.results['tracker'] - def _data_availability(self) -> DataAvailability: - """Reads the current data availability, degrading to ``DEFAULTS`` on error. - - During the warm-start window (before a data source initializes) the - availability gate may query a persistent store, such as Redis. A store - I/O error must not propagate out of an evaluation, so treat it as "no - data available"; the caller then returns the default value with - ``CLIENT_NOT_READY`` instead of raising. - """ - try: - return self._data_system.data_availability - except Exception as e: - log.error("Error checking data availability; treating data as unavailable: %s" % repr(e)) - log.debug(traceback.format_exc()) - return DataAvailability.DEFAULTS - def _evaluate_internal(self, key: str, context: Context, default: Any, event_factory) -> Tuple[EvaluationDetail, Optional[FeatureFlag]]: default = self._config.get_default(key, default) if self._config.offline: return EvaluationDetail(default, None, error_reason('CLIENT_NOT_READY')), None - availability = self._data_availability() + availability = self._data_system.data_availability if availability != DataAvailability.REFRESHED: if availability == DataAvailability.CACHED: log.warning("Feature Flag evaluation attempted before client has initialized - using last known values from feature store for feature key: " + key) @@ -496,7 +480,7 @@ def all_flags_state(self, context: Context, **kwargs) -> FeatureFlagsState: log.warning("all_flags_state() called, but client is in offline mode. Returning empty state") return FeatureFlagsState(False) - availability = self._data_availability() + availability = self._data_system.data_availability if availability != DataAvailability.REFRESHED: if availability == DataAvailability.CACHED: log.warning("all_flags_state() called before client has finished initializing! Using last known values from feature store") diff --git a/ldclient/impl/datasystem/fdv2.py b/ldclient/impl/datasystem/fdv2.py index 6357ec61..9c98a0d9 100644 --- a/ldclient/impl/datasystem/fdv2.py +++ b/ldclient/impl/datasystem/fdv2.py @@ -565,7 +565,16 @@ def data_availability(self) -> DataAvailability: if self._store.selector().is_defined(): return DataAvailability.REFRESHED - if not self._configured_with_data_sources or self._store.is_initialized(): + if not self._configured_with_data_sources: + return DataAvailability.CACHED + + try: + store_initialized = self._store.is_initialized() + except Exception as e: + log.error("Error checking persistent store readiness; treating data as unavailable: %s", e) + return DataAvailability.DEFAULTS + + if store_initialized: return DataAvailability.CACHED return DataAvailability.DEFAULTS diff --git a/ldclient/testing/impl/datasystem/test_fdv2_persistence.py b/ldclient/testing/impl/datasystem/test_fdv2_persistence.py index 2d20f2c6..05bc2b3d 100644 --- a/ldclient/testing/impl/datasystem/test_fdv2_persistence.py +++ b/ldclient/testing/impl/datasystem/test_fdv2_persistence.py @@ -3,8 +3,6 @@ from threading import Event from typing import Any, Callable, Dict, List, Mapping, Optional -import pytest - from ldclient.client import Context from ldclient.config import Config, DataSystemConfig from ldclient.impl.datasystem import DataAvailability @@ -821,11 +819,10 @@ def test_variation_does_not_throw_when_persistent_store_errors_during_warm_start ) fdv2 = FDv2(Config(sdk_key="dummy"), data_system_config) - # The gate itself raises: this is the unguarded root the fix addresses. - with pytest.raises(RuntimeError): - _ = fdv2.data_availability + # The gate itself must not raise: it degrades to DEFAULTS instead. + assert fdv2.data_availability == DataAvailability.DEFAULTS - # Drive the same failing gate through the client and confirm it degrades + # Drive the same gate through the client and confirm it degrades # instead of propagating the error. client = make_client() try: @@ -844,7 +841,7 @@ def test_variation_does_not_throw_when_persistent_store_errors_during_warm_start client.close() assert any( - "Error checking data availability" in record.message + "Error checking persistent store readiness" in record.message for record in caplog.records if record.levelname == "ERROR" ) From 6a5dd9e91da1dae2f7b5200a6786e5ecd08bb51d Mon Sep 17 00:00:00 2001 From: jsonbailey Date: Thu, 27 Aug 2026 16:06:13 -0500 Subject: [PATCH 3/3] refactor: Log sync persistent-store close errors instead of returning 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. --- ldclient/impl/datasystem/store.py | 5 ++--- .../impl/datasystem/test_fdv2_persistence.py | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/ldclient/impl/datasystem/store.py b/ldclient/impl/datasystem/store.py index 5cb78f79..b6532605 100644 --- a/ldclient/impl/datasystem/store.py +++ b/ldclient/impl/datasystem/store.py @@ -521,7 +521,7 @@ def __mapping(data: Dict[str, ModelEntity]) -> Dict[str, Dict[str, Any]]: return e return None - def close(self) -> Optional[Exception]: + def close(self) -> None: """Close the store and any persistent store if configured.""" with self._lock: if self._persistent_store is not None: @@ -532,8 +532,7 @@ def close(self) -> Optional[Exception]: if callable(close): close() except Exception as e: - return e - return None + log.warning("Error closing the persistent store: %s", e) def get_data_store_status_provider(self) -> Optional[DataStoreStatusProvider]: """Get the data store status provider for the persistent store, if configured.""" diff --git a/ldclient/testing/impl/datasystem/test_fdv2_persistence.py b/ldclient/testing/impl/datasystem/test_fdv2_persistence.py index 05bc2b3d..d9f68bd5 100644 --- a/ldclient/testing/impl/datasystem/test_fdv2_persistence.py +++ b/ldclient/testing/impl/datasystem/test_fdv2_persistence.py @@ -845,3 +845,25 @@ def test_variation_does_not_throw_when_persistent_store_errors_during_warm_start for record in caplog.records if record.levelname == "ERROR" ) + + +def test_persistent_store_close_logs_and_swallows_error(caplog): + """A persistent-store close error is logged as a warning, not raised.""" + from ldclient.impl.datasystem.store import Store + from ldclient.impl.listeners import Listeners + + class ClosingFailsStore(StubFeatureStore): + def close(self): + raise RuntimeError("close boom") + + store = Store(Listeners(), Listeners()) + store.with_persistence(ClosingFailsStore(), True, None) + + # close() must log the error rather than raise it. + store.close() + + assert any( + "Error closing the persistent store" in record.message + for record in caplog.records + if record.levelname == "WARNING" + )