chore: Add support for persistent store contract tests. - #502
chore: Add support for persistent store contract tests.#502kinyoklion wants to merge 18 commits into
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.
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.
## Summary In lazy load (daemon) mode the SDK gated all evaluations on the persistent store's `$inited` key, so a store populated by the Relay Proxy or another SDK that hadn't set `$inited` caused every evaluation to return `CLIENT_NOT_READY`. This makes `LazyLoad` always report initialized and evaluate on demand. - `Initialized()` now always returns `true` — the lazy load system can fetch data on demand from the store, so it is always ready to evaluate. - `Initialize()` sets `kValid` immediately, and logs a one-time warning if the store's `$inited` key is absent (evaluations still proceed). - Removed the now-unused `RefreshInitState()`, `initialized_` member, and `Keys::kInitialized`. - Tests updated to cover always-true `Initialized()`, immediate `kValid` status, and the warning-on-missing-`$inited` behavior. No changes to `client_impl.cpp`: since `Initialized()` returns `true`, `PreEvaluationChecks` and `AllFlagsState` proceed with their existing logic. ## Design note `LazyLoad` is the persistent-store (daemon) data system and never runs alongside a streaming/polling data source — streaming uses the in-memory Background Sync / FDv2 systems. There is therefore no configuration where `$inited` needs to gate evaluation, so returning `true` unconditionally is safe. This matches the server-side contract tests (the "no data source" persistence tests populate the store's `features` without `$inited` and still expect real evaluations) and the Go/Java/.NET daemon-mode pattern, where a `NullDataSource` reports initialized immediately. ## Behavioral change When the store is completely unreachable, evaluations now proceed and return the default with `FLAG_NOT_FOUND` rather than `CLIENT_NOT_READY`, consistent with treating the system as always initialized. ## Testing Not built locally (CMake toolchain unavailable); relying on CI for the first compile. Follow-up: run the daemon-mode persistent-store contract tests against Redis end-to-end after this and #502 land (this PR fixes the initialization gating; #502 has the TTL field-name fix). [Link to Devin session](https://app.devin.ai/sessions/20776de6453c4e8baa2e4486dfe9aaba) | Requested by: @kinyoklion <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes core readiness gating for daemon/persistent-store evaluations; unreachable stores now surface flag-not-found defaults instead of CLIENT_NOT_READY, which is intentional but affects operational failure modes. > > **Overview** > **Lazy load (daemon) mode no longer blocks evaluations on the persistent store’s `$inited` key.** Previously missing `$inited` could leave the client not ready and return `CLIENT_NOT_READY`; evaluations can proceed using on-demand reads from the store. > > `LazyLoad::Initialized()` **always returns `true`**, and `Initialize()` **always moves the data source to `kValid`** after a one-time check of the reader. If the store lacks `$inited`, the SDK **logs a warning** but still treats the system as ready—aligned with Go/Java/.NET daemon behavior. > > The **TTL-based init refresh path is removed** (`RefreshInitState`, `initialized_`, and the `initialized` expiration key), and tests now assert always-true initialization, immediate `kValid`, and warning/no-warning logging. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5da2bd3. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
| - cron: '0 8 * * *' | ||
|
|
||
| jobs: | ||
| contract-tests: |
There was a problem hiding this comment.
- Is this redundant redundant with the existing contract test CI runner?
- There is currently one workflow for
v2tests, and one forv3, which includes fdv2. But I think these new workflows only cover thev2tests without fdv2. If we add redis/dynamodb as a second dimension, should we set up a test matrix with all the combinations, or...?
There was a problem hiding this comment.
Both good catches — addressed in 8b4f028.
-
It was partly redundant: the store jobs also re-ran the whole non-persistence v2 suite that
libs/server-sdkalready runs. They now pass-run=persistent.data.store, so each job only runs the persistence suite (the store-specific dimension). I also dropped the rediscontract-tests-curljob — the HTTP backend has no bearing on store behavior andlibs/server-sdkalready covers curl. -
Each store workflow is now a matrix over harness
v2/v3, which required a code fix: the v3 harness nests the store config underdataSystem.store.persistentDataStore(plusstoreMode), so previously every v3 persistence test failed. The service now reads either location. Current per-store results:
| harness | run | suppressed |
|---|---|---|
| v2 | 14 | 12 |
| v3 | 15 | 19 |
The v3 suppressions (persistence-suppressions-fdv2.txt) are the read-write tests plus all with data source cases — the data system method is a variant (Lazy Load or Background Sync or FDv2), so a store can't be combined with a data source, and the store integrations are read-only sources. Both are SDK gaps rather than harness/test-service gaps.
One caveat with -run: if the harness ever renames the persistent data store group, these jobs would pass while running zero tests.
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
|
Devin, I want you to verify that the builds pass the contract tests when you enable CURL networking for the SDK. This is to verify there isn't a conflict with internal use of CURL from other packages. Don't add it to the CI matrix at this time. |
Verified locally with Build: clean, no symbol or link conflicts — both resolve to the system Persistence suite against DynamoDB Local (
Full suites against the same curl-enabled service: v2 Not covered: Redis + curl, since redis-plus-plus/hiredis doesn't resolve in this sandbox (noted earlier in the PR). Redis links no curl of its own, so the AWS SDK is the only third-party curl consumer in play. |
|
Follow-up: Redis + curl is now covered too. My earlier local hiredis failure was not a fetch/network issue — the sandbox had CMake 3.22, and
So both stores pass the persistence suite with curl networking enabled. Still no CI matrix change. |
3 participants
Runs the harness persistent data store suite against the C++ server SDK's Lazy Load data system, for Redis and DynamoDB, under both harness versions.
persistentDataStore(v2) ordataSystem.store.persistentDataStore(v3) — and builds aLazyLoadBuilderfrom it, picking a source bystore.type.LD_REDIS_SUPPORT_ENABLED/LD_DYNAMODB_SUPPORT_ENABLED) and the matchingpersistent-data-store-*capability is advertised only when present.v2/v3, filtered to-run=persistent.data.storeso it doesn't duplicate the suiteslibs/server-sdkalready runs.with data source— both SDK gaps, not test-service gaps.Implementation details
Cache modes
off→CacheRefresh(0s),ttl→CacheRefresh(ttl seconds),infinite→ a very large TTL (there is no dedicated "forever" API).DynamoDB
Follows the convention the other SDKs' test services use: table
sdk-contract-tests, the DSN is the endpoint, regionus-east-1, dummy static credentials (ignored by DynamoDB Local).Build/CI
scripts/build.shtakes a$5DynamoDB toggle mirroring the existing$4Redis toggle; the shared CI action exposesuse_dynamodb. Positional args are quoted in the action so an unsetuse_redisdoesn't shift the DynamoDB argument into its slot.server-dynamodb.ymlgains a contract-test job againstamazon/dynamodb-local; it needsinstall_curl: truebecause the AWS C++ SDK links libcurl.contract-tests-curljob was dropped: the HTTP backend has no bearing on store behavior, andlibs/server-sdkalready covers curl.Why tests remain suppressed
ISerializedDataReader); nothing writes received data back to the store.with data source(both store modes):DataSystemBuilder's method is a variant of Lazy Load / Background Sync / FDv2, so a store cannot be combined with a data source.$initedhandling comes from fix: allow lazy load evaluations when $inited key is not set #508 (already on main), so this branch no longer carries its ownCanEvaluateWhenNotInitialized()change;ignores database initialization flagpasses.Testing
Locally against DynamoDB Local: v2
14 ran, v315 ran(withpersistence-suppressions-fdv2.txt), all passing. Redis is covered by CI; it doesn't build in this sandbox due to a local hiredis/redis++ fetch issue unrelated to this change.No UI surface, so no screenshots or staging link apply.
Link to Devin session: https://app.devin.ai/sessions/115b0bb6b38349f7bd3e69e35d62d559
Requested by: @kinyoklion