Repository navigation
ci: Run the persistence contract tests - #391
Merged
Merged
Conversation
The contract test service had no persistent store support: the data system store configuration was commented out, and the FDv1 configuration was not handled at all, so CI ran the harness with persistence tests disabled. - The service configures a Redis, DynamoDB, or Consul store from the FDv2 dataSystem.store configuration (harness v3) and from the FDv1 persistentDataStore configuration (harness v2). An FDv1 configuration with a store and no data source runs in daemon mode, as the harness expects. The store clients fail fast, as the store recovery tests require. - The service references the three store projects in this repository. They reference the published SDK package unless UseLocalServerSdk is set, which the service sets, so the service always runs against the local SDK. - The service advertises the persistent-data-store-redis, -dynamodb, -consul and -recovery capabilities. - CI starts the stores on the Linux runner and enables the persistence tests there, in the pull request workflow and in the nightly. The service listens on port 9000, its first argument, since DynamoDB uses port 8000. Pre-existing store format defects that the tests found are suppressed: a deleted flag is written without a "key" property (Redis, Consul) or as the placeholder "null" (DynamoDB), and the Consul store fails to read an empty collection.
Member
Author
|
bugbot review |
…r projects From the review of this change: - The contract-tests action defaulted its new port input to 9000, which the client SDK CI also uses, while the client test service always listens on 8000. The default is 8000 again, and the server CI and nightly pass 9000. - Switching the store projects to the local SDK with a property broke solution builds: a solution restore resolves the published SDK package for them, and the build then compiled them against both SDKs. The store projects are unchanged now. A small project under contract-tests/Stores builds each store from its sources against the local SDK; one project per store, because the stores share internal type names. - The action waits until the stores accept connections and Consul has a leader before the tests start. - The service treats a configuration with service endpoint URIs as having a data source when it decides on daemon mode, treats an empty prefix as no prefix, and rejects a store configuration without a DSN.
Member
Author
|
bugbot review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 649f89f. Configure here.
kinyoklion
marked this pull request as ready for review
November 3, 2026 23:25
…persistence-main Co-Authored-By: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> # Conflicts: # pkgs/sdk/server/contract-tests/Representations.cs # pkgs/sdk/server/contract-tests/SdkClientEntity.cs # pkgs/sdk/server/contract-tests/TestService.cs
tanderson-ld
approved these changes
Nov 4, 2026
joker23
approved these changes
Nov 4, 2026
kinyoklion
added a commit
that referenced
this pull request
Nov 6, 2026
<!-- TERMAG_PR_BODY_BEGIN --> ## Summary The server SDK's contract test service had no persistent store support: the data system store configuration in `SdkClientEntity.cs` was commented out with a TODO about the store packages, and the FDv1 `persistentDataStore` configuration was not handled. CI therefore ran both harness versions with `enable_persistence_tests: 'false'`. This change makes the service drive real Redis, DynamoDB, and Consul stores and runs the persistence suites in CI. It is separate from the persistence fix in #390 (and #389 on `main`), which these tests do not cover yet; the harness gains tests for that behavior in a separate change. ## Test service - The FDv2 `dataSystem.store` and `storeMode` configuration (harness v3) and the FDv1 top-level `persistentDataStore` configuration (harness v2) build the store with `Components.PersistentDataStore`, with the requested prefix and cache mode. - An FDv1 configuration with a store and no streaming, polling, or service endpoint configuration runs in daemon mode (`Components.ExternalUpdatesOnly`), which the v2 suite expects. Without this, the client tried to reach LaunchDarkly and failed to initialize. - An empty prefix means the store's default prefix, and a store configuration without a DSN is rejected. - The Redis and DynamoDB clients fail fast (500 ms timeouts, no DynamoDB retries), as the store recovery tests require. The Consul client does not retry. - The service advertises `persistent-data-store-redis`, `persistent-data-store-dynamodb`, `persistent-data-store-consul`, and `persistent-data-store-recovery`. - The service already takes its port as an optional first argument; CI now passes one. Its default stays 8000. ### Building against the local SDK The store projects in `pkgs/` reference the published `LaunchDarkly.ServerSdk` package, so a reference to them would bring a second copy of the SDK into the service. Instead, a small project per store under `contract-tests/Stores/` compiles that store's sources against the SDK project, with the same client library and version range as the store's own project. There is one project per store because the Redis and DynamoDB stores share internal type names. The store projects themselves are not changed, and neither are their packages or their builds. On this branch the SDK is 9.0.0-alpha, outside the stores' published range (`[8.0.0, 9.0.0)`); the stores build and pass against it from their sources. ## CI - `.github/actions/contract-tests` gains `enable_persistence_tests` (default `false`) and `test_service_port` (default `8000`). With persistence enabled, it starts the stores with `launchdarkly/gh-actions/actions/persistent-stores`, waits until they accept connections and Consul has a leader, and enables the tests in both harness runs. - The server SDK workflow enables the persistence tests on the Linux runner only, where the stores start in Docker, and passes port 9000 on every runner: the harness expects DynamoDB on port 8000, the service's default. The client SDK workflow uses the same action with the default port, because its test service always listens on 8000. ## Suppressed tests The persistence suites found three pre-existing defects in the store integrations. They are suppressed with a comment in each suppression file. The harness reads every non-blank line as a test ID, so a line that starts with `#` matches no test. 1. A deleted flag is stored without a `"key"` property: `{"version":N,"deleted":true}`, from the `ItemDescriptor` serializer in `DataStoreTypes.cs` (Redis and Consul store that string), or as the placeholder `"null"` in the item attribute (`DynamoDBDataStoreImpl.MarshalItem`). The harness expects a JSON tombstone with a `key` property, the item key or `$deleted`. This fails `read-write/data source deletions respect versioning` for each store (both harness versions) and, in v3, the five store recovery tests that check a deleted flag after the write back. With a one-line serializer change that writes `"key": "$deleted"`, the Redis and Consul tests pass, and the recovery write back itself works; DynamoDB needs its own fix. 2. `ConsulDataStoreImpl.GetAllAsync` iterates `result.Response`, which is null when no key has the collection's prefix, so reading an empty collection throws. This fails `collection cardinality/collection holds no flags` for Consul. These need SDK and store changes, which this PR does not make. For the keyless tombstone, the v3 harness's own tombstone suite lists `{"version":N,"deleted":true}` as a shape that .NET, Java, and Node write and that readers must accept, so for Redis and Consul the fix may belong in the harness's read-write matcher instead. DynamoDB's `"null"` placeholder is not one of the accepted shapes. Until one of these lands, CI does not check the store write back after recovery or the store side of deletions for any store; the only recovery test that runs checks evaluations from memory during the outage. ## Testing With Redis, DynamoDB local, and Consul running in Docker on their default ports: - Harness v2 (2.41.1), full suite with persistence enabled and the suppression file: all tests passed (4989 ran). - Harness v3 (3.2.0-alpha.15, from `v3` at 58f4122), full suite with persistence enabled and the FDv2 suppression file: all tests passed (5174 ran). Without the suppressions, the persistence suites fail exactly the tests listed above (4 in v2, 19 in v3). After the review changes (the store projects, the port default, the readiness wait), both persistence suites pass again with the suppressions (v3: 141 ran; v2: 111 ran), and `LaunchDarkly.ServerSdk.sln` builds with no errors. This is the `dotnet-server-sdk-v9` version of #391. <!-- TERMAG_PR_BODY_END --> <!-- termag: box=i-0575d1c54853058d7 project=Overrides --> <sub>Opened from termag</sub> [SDK-3493](https://launchdarkly.atlassian.net/browse/SDK-3493) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Enables the SDK test harness’s persistent-store suites in CI** by wiring the contract test service to real Redis, DynamoDB, and Consul integrations and extending the shared contract-test GitHub Action. > > The contract test service now maps harness **FDv1 `persistentDataStore`** and **FDv2 `dataSystem.store`** into `Components.PersistentDataStore` (Redis, DynamoDB, Consul) with prefix/cache options, **500 ms fail-fast** client settings for recovery tests, and **daemon mode** (`ExternalUpdatesOnly`) when a store is configured without streaming/polling. It advertises the new persistence capabilities. **Wrapper projects under `Stores/`** compile each integration’s sources against the in-repo SDK so CI does not pull a second published SDK copy. > > **CI:** `enable_persistence_tests` and `test_service_port` are plumbed through the composite action; when enabled it starts stores, waits on ports **6379/8000/8500** and Consul leadership, and launches the service on a non-8000 port (DynamoDB local uses 8000). **Server SDK workflow** turns persistence on **Linux only** and uses port **9000**. > > **Known integration gaps** are temporarily **skipped** via suppression entries (deletion tombstone shape without `"key"`, DynamoDB `"null"` deletes, Consul empty collection reads)—CI still runs the rest of the persistence suites. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 8f77f3f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> [SDK-3493]: https://launchdarkly.atlassian.net/browse/SDK-3493?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
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.
Summary
The server SDK's contract test service had no persistent store support: the data system store configuration in
SdkClientEntity.cswas commented out with a TODO about the store packages, and the FDv1persistentDataStoreconfiguration was not handled. CI therefore ran both harness versions withenable_persistence_tests: 'false'. This change makes the service drive real Redis, DynamoDB, and Consul stores and runs the persistence suites in CI.It is separate from the persistence fix in #389 (and #390 on
dotnet-server-sdk-v9), which these tests do not cover yet; the harness gains tests for that behavior in a separate change.Test service
dataSystem.storeandstoreModeconfiguration (harness v3) and the FDv1 top-levelpersistentDataStoreconfiguration (harness v2) build the store withComponents.PersistentDataStore, with the requested prefix and cache mode.Components.ExternalUpdatesOnly), which the v2 suite expects. Without this, the client tried to reach LaunchDarkly and failed to initialize.persistent-data-store-redis,persistent-data-store-dynamodb,persistent-data-store-consul, andpersistent-data-store-recovery.dotnet-server-sdk-v9. Its default stays 8000.Building against the local SDK
The store projects in
pkgs/reference the publishedLaunchDarkly.ServerSdkpackage, so a reference to them would bring a second copy of the SDK into the service. Instead, a small project per store undercontract-tests/Stores/compiles that store's sources against the SDK project, with the same client library and version range as the store's own project. There is one project per store because the Redis and DynamoDB stores share internal type names. The store projects themselves are not changed, and neither are their packages or their builds.CI
.github/actions/contract-testsgainsenable_persistence_tests(defaultfalse) andtest_service_port(default8000). With persistence enabled, it starts the stores withlaunchdarkly/gh-actions/actions/persistent-stores, waits until they accept connections and Consul has a leader, and enables the tests in both harness runs.Suppressed tests
The persistence suites found three pre-existing defects in the store integrations. They are suppressed with a comment in each suppression file. The harness reads every non-blank line as a test ID, so a line that starts with
#matches no test."key"property:{"version":N,"deleted":true}, from theItemDescriptorserializer inDataStoreTypes.cs(Redis and Consul store that string), or as the placeholder"null"in the item attribute (DynamoDBDataStoreImpl.MarshalItem). The harness expects a JSON tombstone with akeyproperty, the item key or$deleted. This failsread-write/data source deletions respect versioningfor each store (both harness versions) and, in v3, the five store recovery tests that check a deleted flag after the write back. With a one-line serializer change that writes"key": "$deleted", the Redis and Consul tests pass, and the recovery write back itself works; DynamoDB needs its own fix.ConsulDataStoreImpl.GetAllAsynciteratesresult.Response, which is null when no key has the collection's prefix, so reading an empty collection throws. This failscollection cardinality/collection holds no flagsfor Consul.These need SDK and store changes, which this PR does not make. For the keyless tombstone, the v3 harness's own tombstone suite lists
{"version":N,"deleted":true}as a shape that .NET, Java, and Node write and that readers must accept, so for Redis and Consul the fix may belong in the harness's read-write matcher instead. DynamoDB's"null"placeholder is not one of the accepted shapes. Until one of these lands, CI does not check the store write back after recovery or the store side of deletions for any store; the only recovery test that runs checks evaluations from memory during the outage.Testing
With Redis, DynamoDB local, and Consul running in Docker on their default ports:
v3at 58f4122), full suite with persistence enabled and the FDv2 suppression file: all tests passed (5095 ran).Without the suppressions, the persistence suites fail exactly the tests listed above (4 in v2, 19 in v3).
After the review changes (the store projects, the port default, the readiness wait), both persistence suites pass again with the suppressions (v3: 141 ran; v2: 111 ran), and
LaunchDarkly.ServerSdk.slnbuilds with no errors.#392 makes the same change on
dotnet-server-sdk-v9.Opened from termag
SDK-3493
Note
Overview
Enables persistent store contract tests in CI by wiring the server SDK contract test service to real Redis, DynamoDB, and Consul, and turning those suites on in GitHub Actions.
The contract test service now builds Redis, DynamoDB, and Consul integrations from in-repo sources (via
Stores/*wrapper projects) against the local SDK, implementsCreatePersistentDataStorefor harness FDv1persistentDataStoreand FDv2dataSystem.store, supports daemon mode when a store is configured without streaming/polling, advertises the new store capabilities, and accepts an optional listen port (9000 in CI so DynamoDB can use 8000). Store clients use short timeouts for recovery tests.The contract-tests composite action gains
enable_persistence_testsandtest_service_port, starts stores and waits for readiness when enabled, and forwards those flags to both harness runs. sdk-server-ci runs persistence tests on Linux only; nightly always enables them with port 9000.Known failures from pre-existing tombstone / empty-collection bugs in the store integrations are listed in
test-supressions*.txtso CI stays green until separate fixes land.Reviewed by Cursor Bugbot for commit fd9c0d6. Bugbot is set up for automated code reviews on this repo. Configure here.