chore: Add support for persistent store contract tests.#502
chore: Add support for persistent store contract tests.#502kinyoklion wants to merge 14 commits intomainfrom
Conversation
Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
…s CI Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
…r Windows CI" This reverts commit d960e6f.
|
Devin, please update with the latest changes from main. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Removed enable_persistence_tests and use_redis from server.yml contract tests (which have no Redis service). Added dedicated contract-tests and contract-tests-curl jobs to server-redis.yml with a Redis service container and enable_persistence_tests: true. Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The default of 'false' was always passed as $4 to build.sh, which explicitly set build_redis='OFF' and overrode the auto-detection for Redis target names. An empty default means $4 is empty, so neither branch matches and auto-detection is preserved. Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…sts support The pinned SHA (contract-tests-v1.1.0) did not have the enable_persistence_tests input, causing persistence tests to be skipped. Updated to contract-tests-v1.3.0 which supports this input. Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
The Go test harness sends cache TTL as 'ttl' (in seconds), but the C++ data model expected 'ttlMs' (in milliseconds). This meant the TTL value was never deserialized, and the default 5-minute cache TTL was always used regardless of what the test requested. - Rename ConfigPersistentCache::ttlMs to ttl to match JSON field name - Change CacheRefresh from milliseconds to seconds to match harness units Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
| } else if (in.persistentDataStore->cache.mode == "ttl") { | ||
| if (in.persistentDataStore->cache.ttl) { | ||
| lazy_load.CacheRefresh(std::chrono::seconds( | ||
| *in.persistentDataStore->cache.ttl)); |
There was a problem hiding this comment.
TTL value likely treated as wrong time unit
High Severity
The CacheRefresh API accepts std::chrono::milliseconds, and the LaunchDarkly SDK contract test harness protocol sends the ttl value in milliseconds. However, the code wraps the raw ttl integer in std::chrono::seconds, which implicitly converts to milliseconds by multiplying by 1000. This makes the effective cache TTL 1000× larger than intended (e.g., a harness-sent value of 30000 ms becomes 30,000 seconds instead of 30 seconds). The data model comment also incorrectly states the unit is seconds. The value likely needs to be wrapped in std::chrono::milliseconds instead.
Additional Locations (1)
There was a problem hiding this comment.
ttl (number, optional): If the cache mode is ttl, this value will be the time-to-live for cache entries in seconds.
In lazy load / daemon mode, the SDK's Initialized() check was blocking all flag evaluations when the $inited key was not found in the persistent store. This is problematic because in daemon mode, an external process (like Relay Proxy) populates the store, and the $inited key may not always be present. The fix changes LazyLoad::Initialized() to always return true, allowing evaluations to proceed using available data. When the underlying source reports not initialized ($inited key not found), a warning is logged to alert operators that a Relay Proxy or other SDK should set this key. This aligns with the Go SDK behavior where daemon mode (ExternalUpdatesOnly) always considers the data source initialized. Updated unit tests to reflect the new behavior and added tests verifying the warning is logged appropriately. Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
Reworked approach based on review feedback: Initialized() should return false when $inited is not set (consistent with other SDK implementations), and the evaluation path should handle this case by warning and proceeding rather than blocking. Changes: - Added CanEvaluateWhenNotInitialized() virtual method to IDataSystem interface (defaults to false) - LazyLoad overrides to return true (can serve on demand) - PreEvaluationChecks warns and proceeds when data system can evaluate while not initialized, instead of returning CLIENT_NOT_READY - AllFlagsState similarly warns and proceeds instead of returning empty - Reverted LazyLoad::Initialized() to original behavior (truthfully reports whether $inited key exists) - Added unit test for CanEvaluateWhenNotInitialized() This matches the pattern used in the Erlang SDK where the evaluation path distinguishes between 'not initialized' (blocks) and 'store initialized' (warns but proceeds). Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
…amb/persistence-contract-tests
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| "initialization completed; using last known values " | ||
| "from data store. The $inited key was not found in " | ||
| "the store; typically a Relay Proxy or other SDK " | ||
| "should set this key."; |
There was a problem hiding this comment.
Warning logged on every evaluation in lazy load
Medium Severity
In PreEvaluationChecks, a verbose warning is logged on every single evaluation call when Initialized() returns false and CanEvaluateWhenNotInitialized() is true. In LazyLoad mode where $inited is never set (common without Relay Proxy), this produces a warning on every flag evaluation, potentially thousands per second. Same issue exists in AllFlagsState. This warning likely needs to be logged only once or rate-limited.
Additional Locations (1)
There was a problem hiding this comment.
This is consistent with other implementations.
https://github.com/launchdarkly/go-server-sdk/blob/ef9846a95f060076b19b405c26ad452dca574b1e/ldclient.go#L1389
@keelerm84 Relay does write $inited, correct?
Co-Authored-By: rlamb@launchdarkly.com <kingdewman@gmail.com>
There was a problem hiding this comment.
Currently this SDK doesn't support read-write mode. It supports a purely lazy system for use with daemon mode.
There was a problem hiding this comment.
With FDv2 support we will want to standardize this with the FDv2 behavior.
2 participants
Summary
Adds support for running persistent data store contract tests against the C++ server SDK's Redis integration.
Key changes:
.github/actions/ci/action.yml): Addeduse_redisinput, passed as$4tobuild.shto explicitly controlLD_BUILD_REDIS_SUPPORT.scripts/build.sh: Added$4parameter for explicit Redis support toggle (overrides automatic target-name detection when provided).server-redis.yml: Addeduse_redis: trueto all existing Redis workflow jobs. Added newcontract-testsandcontract-tests-curljobs with a Redis service container,enable_persistence_tests: true, and--skip-fromsuppression file.ConfigPersistentCache,ConfigPersistentStore, andConfigPersistentDataStorestructs; wiredpersistentDataStoreintoConfigParams. TTL field usesttl(seconds) to match the Go test harness.entity_manager.cpp): Implemented Redis persistent store creation and cache mode configuration (off,ttl,infinite) behindLD_REDIS_SUPPORT_ENABLEDcompile flag.main.cpp): Advertisespersistent-data-store-rediswhen Redis support is compiled in.launchdarkly::server_redis_sourceand definesLD_REDIS_SUPPORT_ENABLEDwhenLD_BUILD_REDIS_SUPPORTis ON.client_impl.cpp,idata_system.hpp,lazy_load_system.hpp): AddedIDataSystem::CanEvaluateWhenNotInitialized()virtual method. LazyLoad overrides it to returntrue, allowing flag evaluations in daemon mode when$initedis not set (logs a warning instead of returningCLIENT_NOT_READY). This matches Go/Erlang SDK behavior where a Relay Proxy sets the$initedkey but evaluations should still proceed without it.suppressions.txt): 12 read-write persistent store tests are suppressed because the C++ SDK does not yet support write-through from BackgroundSync to persistent store. These tests require the SDK to write received streaming data to Redis alongside the in-memory store.Updates since last revision:
suppressions.txtfor 12 read-write persistent store tests and wired it into both contract test jobs via--skip-from. The C++ SDK's BackgroundSync does not yet support writing to a persistent store (it only reads from memory). This is a known feature gap tracked separately.v1.1.0tov1.3.0(required forenable_persistence_testsandextra_paramsinputs to work).CanEvaluateWhenNotInitialized()fix from PR fix: allow lazy load evaluations when $inited key is not set #508 — daemon mode tests now pass (previously all returnedCLIENT_NOT_READY).enable_persistence_testsanduse_redisout of the general server contract tests inserver.yml(which have no Redis service) into dedicated contract test jobs inserver-redis.yml.mainwhich includes PR chore: update CI runners and dependencies for macOS and Windows #505 (CI runner + OpenSSL + Boost pin fixes) and PR fix: update foxy for Boost 1.90 compatibility and unpin macOS Boost #506 (foxy Boost 1.90 compatibility + Boost unpin).CI results: All 44 checks pass. Persistence tests: 8 daemon-mode + 2 prefix tests pass; 12 read-write tests skipped via suppressions.
Review & Testing Checklist for Human
CanEvaluateWhenNotInitialized()behavioral change: When$initedis missing in daemon mode,PreEvaluationChecksnow logs a warning on every evaluation call (no rate limiting). Consider whether this is acceptable log volume or if a once-per-session guard is needed.24 * 365 hoursTTL) matches contract test expectations. Confirm there isn't a dedicated "infinite" API onLazyLoadBuilder.CacheRefresh(std::chrono::seconds(0))) actually disables caching as intended — i.e., 0 seconds means "don't cache" rather than "always stale".suppressions.txt— these represent missing BackgroundSync write-through functionality. Confirm this is an acceptable gap for now or if a follow-up issue should be filed.Notes
Note
Medium Risk
Adds new CI jobs and contract-test harness logic for Redis-backed persistent stores and changes client evaluation behavior when not initialized for lazy-load mode; issues could surface as CI failures or altered early-evaluation semantics.
Overview
Adds Redis-backed persistent data store coverage to server SDK contract tests by introducing dedicated
server-redis.ymlcontract-test jobs that spin up a Redis service and enable persistence tests (with and without CURL).Extends the shared CI action and
scripts/build.shto accept an explicituse_redistoggle (controllingLD_BUILD_REDIS_SUPPORT), and wires the contract-test harness to parsepersistentDataStoreconfig, link Redis sources conditionally, advertise thepersistent-data-store-rediscapability, and build a lazy-load Redis data source with configurable cache modes (off/ttl/infinite).Updates server SDK evaluation paths to allow lazy-load data systems to evaluate before "initialized" (logging warnings instead of returning
CLIENT_NOT_READY) via a newIDataSystem::CanEvaluateWhenNotInitialized()override and accompanying unit test.Written by Cursor Bugbot for commit 6cf1a3c. This will update automatically on new commits. Configure here.