chore: Add support for persistent store contract tests.#502
chore: Add support for persistent store contract tests.#502kinyoklion wants to merge 13 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)
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)
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 andenable_persistence_tests: true— these are the only jobs that run persistence contract tests.ConfigPersistentCache,ConfigPersistentStore, andConfigPersistentDataStorestructs; wiredpersistentDataStoreintoConfigParams.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.Updates since last revision:
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(which run a Redis service container). The general server contract tests inserver.ymlare unchanged frommain.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). All 46 CI checks pass.Review & Testing Checklist for Human
24 * 365 hoursTTL) matches what the contract test harness expects — 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".build.sh$4parameter logic doesn't break any other callers ofbuild.shthat may pass positional arguments differently. When$4is not provided, the variable is unset and the existing auto-detection logic is preserved.server-redis.ymlon Linux; Mac/Windows Redis tests are still TODO (no Redis service available on those runners).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.