[keelerm84]

TestDataV2 gains use_preconfigured_flag, and FlagBuilderV2 gains
migration_settings, sampling_ratio, exclude_from_summaries and a nested
FlagMigrationSettingsBuilder. The v1 TestData has all of these, and v9
removes v1 TestData with the FDv1 data system, so TestDataV2 must cover
them first.

A preconfigured flag keeps its own version, as in v1 TestData, because
tests use specific versions to check the evaluation and event output. A
flag that replaces an existing flag gets the next version. A
preconfigured flag also discards any stored builder for its key, so a
later flag call starts from a new builder.

The shared spec config now gives the client an FDv2 data system with no
sources, in place of the FDv1 null data source. Specs that need data use
TestDataV2 as a synchronizer, and specs that need a persistent store use
a hash-backed MockFeatureStore through the data system config.

The end-to-end and environment ID specs poll the FDv2 endpoint with an
FDv2 payload. On a 401, FDv2 turns the data source off at once and does
not retry, so those specs now expect OFF, a single request and no full
start wait, in place of the FDv1 retry behavior.

The tracker specs use the V2 flag builder. The FDv2 store specs gain the
flag change cases that only the FDv1 data source specs covered: flags
that a full data set adds or removes, a deleted prerequisite, and a
removed dependency.

The nil SDK key and use_ldd fork specs still use FDv1 options, because
they test FDv1 logic in LDClient. They change with that logic when FDv1
is removed.

---

Note

Overview
TestDataV2 now supports preconfigured flags the same way v1 did: use_preconfigured_flag normalizes JSON/hashes/models, validates via deserialization, applies version rules (keep higher version or bump), clears any stored builder for that key, and broadcasts under an update mutex so deliveries stay ordered. Segments share the same normalization path; subscribe replaces add_instance and hands synchronizers a snapshot in one step so sync only queues changes after that baseline.

TestDataSourceV2 uses a shared fetch_result for initial loads; fetch snapshots current data; sync subscribes inside fetch_result so pre-sync updates are not replayed. FlagBuilderV2 gains migration_settings, sampling_ratio, exclude_from_summaries, and a small FlagMigrationSettingsBuilder; rules now copy clause value arrays when building.

Specs and mocks move from FDv1 data_source to data_system_config (TestDataV2, fdv2_poll_payload, polling/streaming builders, SilentSynchronizer, MockFeatureStore). End-to-end and 401 cases expect FDv2 behavior (e.g. OFF, single request, no long init wait). Store specs add dependency/delete cases; test_data_v2 and migration specs cover the new APIs and ordering.

^{Reviewed by Cursor Bugbot for commit c638040. Bugbot is set up for automated code reviews on this repo. Configure here.}

[kinyoklion]

Note

Agent-authored comment. This finding comes from an automated multi-agent review of this PR (general / security / adversarial / spec-conformance agents, graded against sdk-specs @ a75576c), posted with the account owner's review and approval. Each claim below was verified by executing the PR head, not just reading code. Please push back freely if any of it misreads the intent.

use_preconfigured_flag drops v1's boundary validation — silent failures where v1 was loud

The new use_preconfigured_flag (lib/ldclient-rb/integrations/test_data_v2.rb:191-214) stores the caller's hash raw. v1's use_preconfigured_item ran Impl::Model.deserialize at the API boundary, so structurally bad input raised in the caller's thread. V2 defers parsing to the client's data-system thread, which turns several plausible input mistakes into silent or confusing failures:

(a) One malformed flag drops the whole initial basis — and the client reports success.
use_preconfigured_flag({ key: 'bad', version: 1, on: true, variations: [true], prerequisites: ['oops'] }) is accepted without error. When a client fetches the basis, FeatureFlag.new raises inside decode_collection (impl/data_store/in_memory_feature_store.rb:111-123), which rescues and returns nil, so set_basis fails for the entire payload — a well-formed sibling flag in the same payload is lost too. Meanwhile impl/data_system/fdv2.rb:290-297 discards @store.apply's return value, and with a defined selector the data system sets status VALID and releases readiness. Net effect: the constructor returns "success", data source status is VALID, every variation serves the default, and the only signal is one error log line. Proof spec asserting the desired behavior fails today with the well-formed sibling serving "DEFAULT" instead of its configured value.

(b) Exported flag JSON silently mis-evaluates.
transform_keys(&:to_sym) is top-level only, so use_preconfigured_flag(JSON.parse(flag_json)) — the most natural way to obtain a "complete flag data model" — keeps string keys at depth. Verified end-to-end through a real client: fallthrough.variation decodes to nil → evaluation returns the default with ERROR(MALFORMED_FLAG); rules decode with zero clauses and match every context; migration.checkRatio is silently dropped. (v1 was loud for this input shape — nil key → NoMethodError at the call site — and additionally accepted JSON strings, which V2 rejects with NoMethodError on as_json.)

(c) Missing/odd input surfaces as a bare NoMethodError.
No :key → undefined method `to_sym' for nil (raised inside the write lock — the lock itself unwinds correctly, but nothing says what was missing); nil → undefined method `as_json' for nil; a non-string/symbol :key fails the same way.

(d) version is unvalidated.
version: "100" is stored verbatim, and the next use_preconfigured_flag on the same key raises TypeError: no implicit conversion of Integer into String inside the write lock.

Suggestion: validate and deep-normalize at the boundary — e.g. run Impl::Model.deserialize over a deep-symbolized copy of the input, raising ArgumentError naming the offending field. (A deep-dup in the same pass would also fix the shallow-dup aliasing: today, mutating the caller's nested structures after the call changes the stored test data with no version bump and no notification.) Worth a separate ticket beyond this PR: fdv2.rb ignoring the apply result is what makes a failed basis look like VALID + ready — that predates this change, but this API exposes it.

We have four failing proof specs (rspec, ruby 3.4) that encode the desired behavior for (a)–(c) and flip green with a boundary-validation fix — happy to push them to this branch or a follow-up if useful.

[kinyoklion]

Note

Agent-authored comment. From the same automated multi-agent review as the boundary-validation comment above, posted with the account owner's review and approval. Verified by executing the PR head.

Replacement version clamp discards a higher provided version (test_data_v2.rb:201)

updated_flag[:version] = old_flag.nil? ? (updated_flag[:version] || 1) : old_flag[:version] + 1

The replacement branch unconditionally stores old + 1, ignoring the caller's provided version:

td.use_preconfigured_flag({ key: "f", version: 5,   ... })  # new flag → stores 5   ✓
td.use_preconfigured_flag({ key: "f", version: 100, ... })  # replace → stores 6    ✗ (not 100)

This undercuts the PR's own rationale for keeping versions — "tests use specific versions to check the evaluation and event output" — exactly when a test replaces a flag with an explicit higher version: versions surface in all_flags_state and feature events, and nothing tells the author their version was rewritten.

Spec angle. TDS 2.6.3 mandates the bump only conditionally: "MUST automatically increment the version number of the flag if the provided flag's version is less than or equal to the current version." The ≤ case exists because version-checking stores reject non-monotonic writes, and the PR handles it correctly (test_data_v2_spec.rb:69-76 pins 100 then 5 → 101). The spec is silent on the > case, where the natural reading is that an already-higher version needs no intervention. So the clamp doesn't violate the letter of the MUST, but extends it past the spec in a way that destroys caller intent. (The method's doc comment does document the clamp — "a flag that replaces an existing flag gets a version one more than the version of the existing flag" — so if this is the intended semantic, the alternative resolution is to keep it and file the spec-ambiguity note below.)

Cross-SDK divergence. Go's TestDataV2 implements the conditional reading (testhelpers/ldtestdatav2/test_data_source.go:120-133): if f.Version < version { f.Version = version } — replace at current=5 with provided=100 keeps 100. Same spec text, opposite behavior; portable test suites would observe different event output across SDKs.

Same line, minor edge: (updated_flag[:version] || 1) keeps a provided version: 0 for a new flag (0 || 1 → 0 in Ruby); Go normalizes to 1.

Suggested fix (keep-higher with a monotonic floor — Go's semantics and the spec's intent):

floor = old_flag.nil? ? 0 : old_flag[:version]
provided = updated_flag[:version] || 0
updated_flag[:version] = [provided, floor + 1].max  # then map 0 → 1 for a versionless new flag

We have a failing proof spec for both cases (plus one pinning the mandated ≤ case, which passes today and must keep passing) — happy to push them to this branch or a follow-up. Fair disclosure: v1 TestData has the identical unconditional clamp (test_data.rb:176-182), so this is v1 parity faithfully ported; the observation is that the new v2 API had the spec's conditional and the Go reference available, and the > case is currently untested.

[kinyoklion]

Note

Agent-authored comment. Same automated multi-agent review as the two comments above, posted with the account owner's review and approval. Both halves of this were proven deterministically by executing the PR head (a blocking fake instance for the delivery ordering, and a direct store-level test for the missing version check).

Out-of-order upsert delivery + no stale-version rejection in the FDv2 store = clients can stick on a stale flag

Two cooperating layers:

Layer 1 — delivery is not serialized with commit. Both update (test_data_v2.rb:110-132) and the new use_preconfigured_flag (:191-214) compute the new flag under the write lock, copy @instances, then call instance.upsert_flag after releasing the lock. Two concurrent calls therefore serialize their @current_flags updates but not their propagation. Deterministic interleaving (thread A commits v2 and blocks inside a fake instance's upsert_flag; thread B commits and delivers v3; then A is released): the instance receives [1, 3, 2] — the newest write arrives before the stale one.

Layer 2 — the store accepts the regression. InMemoryFeatureStoreV2#apply_delta (impl/data_store/in_memory_feature_store.rb:86-104) does items_of_kind[key] = item with no version check. Feeding a v3 basis and then a v2 delta leaves store.get returning version 2 with the stale value. Contrast with the FDv1 store (in_memory_store.rb:72), which rejects old[:version] >= item[:version] — so the identical race in v1 TestData was self-healing per key, and in FDv2 it's sticky: the client holds the stale flag indefinitely (until the next update for that key), while make_init_data and late-joining clients see the fresh one. Flag-change listeners do still fire for the regressive write, so the symptom is a spurious notification carrying regressed data rather than a missed one.

Concrete trigger in a test suite: a "reset these flags" helper racing a test body that updates the same key — the test then reads back the wrong variation with no error anywhere.

Labeling, for fairness: the compute-inside/deliver-outside pattern is pre-existing in update (and use_preconfigured_segment); this PR adds use_preconfigured_flag as a second caller of the racy path. The missing store version-guard is pre-existing FDv2 infrastructure, untouched by this diff — but it's what makes the race observable, and it also undercuts TDS 2.6.3's stated rationale in the spec ("prevents silent rejection by the data store's Upsert method" — the FDv2 store performs no such rejection).

Suggested direction: cheapest correct fix for this PR is propagating upserts while holding the lock (the critical section is short — an enqueue per instance). The more robust fix, at FDv2-infra scope rather than this PR, is adding stale-version rejection to InMemoryFeatureStoreV2#apply_delta to match the FDv1 store's semantics; that also future-proofs every other synchronizer racing a TestData-style source.

Proof specs for both layers fail today ([1, 3, 2] delivery order; store keeps v2 after a v3 basis) and flip green with either fix — happy to push them to this branch or the follow-up.

[kinyoklion]

Feedback in comments

[keelerm84]

Fixed in f4887c4 and aadae60. use_preconfigured_flag and use_preconfigured_segment now deep-convert the input to symbol keys, store a copy, and raise ArgumentError at the call site for nil or another type, a missing or empty key, a non-integer version, malformed JSON, a value JSON cannot hold (such as a Time, NaN or Infinity), or a data model error. A Set becomes an array. They also accept a JSON string again, as v1 did. Specs cover each case, plus nested string keys from JSON.parse and later mutation of the caller's hash.

Agreed that fdv2.rb ignoring the @store.apply result is the deeper problem, since any synchronizer can hit it. That predates this PR, so I'll handle it separately.

[keelerm84]

Fixed in f4887c4, with Go's semantics: the stored version is max(provided, current + 1), and a new flag with version 0 or no version gets 1. Specs cover replace-with-higher (5 then 100 keeps 100), replace-with-lower (100 then 5 gives 101), and version 0.

[keelerm84]

Layer 1 is fixed in f4887c4. update, use_preconfigured_flag and use_preconfigured_segment now hold a mutex across the commit and the delivery to the instances. The existing read-write lock is not reentrant, and an instance's upsert_flag calls back into get_version, so delivering under that lock would deadlock; the separate mutex avoids that. A spec blocks the first delivery and checks that the second change waits and arrives second.

Layer 2, the missing stale-version rejection in InMemoryFeatureStoreV2#apply_delta, is FDv2 store behavior outside this PR. It goes in its own PR, along with the duplicate flag change events the same missing check causes.

[keelerm84]

Correction on layer 2: there won't be a separate PR. FDv2 guarantees upstream that payloads arrive in order, so apply_delta does not need a version check. Go has none either. FDv1 had no such guarantee, which is why its stores check versions.

TestDataV2 is a local source, so it has to keep that order itself. This PR fixes the order of changes between two updates. #491 fixes the other case, where a change made before a synchronizer starts was sent again after the initial data.