Repository navigation
feat: Add the override store, overlay, and data system wiring - #451
Conversation
0f89736 to
4810ac9
Compare
e2df553 to
e4d05bb
Compare
4810ac9 to
20eea70
Compare
e4d05bb to
09915cc
Compare
20eea70 to
8072ee4
Compare
09915cc to
4f4c242
Compare
8072ee4 to
ed8fccf
Compare
4f4c242 to
54278dc
Compare
|
bugbot review |
54278dc to
0dee958
Compare
bb04f10 to
6ad2419
Compare
0dee958 to
18341e8
Compare
6ad2419 to
7579ddb
Compare
18341e8 to
45c6daa
Compare
|
bugbot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
bugbot review |
2 similar comments
|
bugbot review |
|
bugbot review |
|
bugbot review |
|
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 0a4c8b7. Configure here.
7579ddb to
fae4a5c
Compare
0a4c8b7 to
827c4e2
Compare
| # A failed evaluation has no evaluation state, so it reports no prerequisites. | ||
| prerequisites: eval_state&.prerequisites, | ||
| version: f[:version], | ||
| trackEvents: f[:trackEvents] || requires_experiment_data, |
There was a problem hiding this comment.
An override-affected flag keeps its trackEvents, trackReason, and debugEventsUntilDate here. A client-side SDK bootstrapped from this state then sends full feature events, and debug events, for every evaluation of an overridden flag. Section 1.7 rules those out, and 1.4.1.1 asks the report to turn them off for this reason. Go clears all three when the result is override-affected (ldclient.go, AllFlagsState).
The description defers the events work (1.7), but 1.4.1.1 sits in 1.4 and this PR is where the report starts carrying overrides. Can this gate the three fields on detail.reason.override_affected? If it's meant to land with the events PR, could the description say so?
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
It lands with the events PR: #452 turns off trackEvents, trackReason, and debugEventsUntilDate in all_flags_state when the evaluation is override-affected (lib/ldclient-rb/ldclient.rb around line 712 on that branch), as Go does, together with the rest of the event policy. This PR's description now says so.
| tracker = DependencyTracker.new | ||
| KINDS.each do |kind| | ||
| view[kind].each do |key, item| | ||
| tracker.update_dependencies_from(kind, key.to_s, item) |
There was a problem hiding this comment.
When a flag change listener is registered, an entry whose dependency fields can't be read makes set_overrides raise here. Two inputs that reach it: a deleted entry ({key: "f", version: 2, deleted: true}, whose model has nil prerequisites) and a clause with a non-array values. By then @layer.set_all has already swapped, so the snapshot is live, no listener hears about any entry in it, and the exception goes back to the source. Every later snapshot raises the same way while the entry is in the old or new view. If a listener is registered before start (a plugin, say), the cleanup in start_override_source hits it again and LDClient.new raises.
Layer#set_all already rescues this case for its own graph. Can tracker_for do the same per item? A deleted entry is also served today as a flag that evaluates to EXCEPTION and shadows the LaunchDarkly flag, where LD data and Go give FLAG_NOT_FOUND, so it may be simplest to reject deleted entries in index_items, before the swap.
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in a9befc7, both ways you suggested. index_items now rejects a deleted entry before the layer is replaced, so the previous snapshot stays in effect and the deleted entry never shadows the LaunchDarkly flag. tracker_for rescues per item, as Layer#set_all does, so an entry whose dependency fields cannot be read (a clause with a non-array values) contributes no edges and every changed entry is still notified. New sink specs cover both and fail without the change.
| @override_source = nil | ||
| begin | ||
| source.stop | ||
| rescue => e |
There was a problem hiding this comment.
The default start and stop in OverrideSource raise NotImplementedError, which is a ScriptError, so rescue => e here and in start_override_source doesn't catch it. A custom source that includes the module and skips stop because it holds nothing makes LDClient#close raise before the synchronizer, the store, and the event processor are stopped, and the FDv2 threads keep running. Skipping start makes LDClient.new raise. Both go against the interface docs, which say a source failure doesn't stop the client from starting or shutting down.
Can these rescue ScriptError as well, or can the rest of stop run in an ensure?
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in a9befc7. start_override_source and stop_override_source now rescue StandardError, ScriptError, which covers the NotImplementedError of the interface defaults, so a source that leaves out start starts the client without overrides and one that leaves out stop no longer keeps the synchronizer, the store, and the executor from stopping. New FDv2 specs use a source that includes the module and implements only one of the two; both fail without the change.
| # @param initial_modified_item [Hash] {kind:, key:} hash | ||
| # @return [void] | ||
| # | ||
| private def add_affected_items(items_out, initial_modified_item) |
There was a problem hiding this comment.
This runs for every FDv2 user, including those who never configure overrides. Results are the same as before, but each item reached starts a fresh traversal under the store lock, even when @external_dependencies is nil. A full transfer of a 1000-deep prerequisite chain goes from 0.017 s at the base to 2.3 s, and a 10k-flag data set with a shared segment from 0.30 s to 0.44 s.
Go calls the tracker directly when there is no external graph (internal/datasystem/store.go, addAffectedItems). Can this do the same, so users without overrides keep today's cost?
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in a9befc7. When no external graph is set, add_affected_items calls the store's own tracker once with the string key, which is what the store did before external graphs existed, so FDv2 users without overrides keep the previous cost. The fixpoint over both graphs only runs when an override layer is attached. The store specs, including the dependent-flag specs from #470, pass.
| # | ||
| # @example | ||
| # overrides = LaunchDarkly::Integrations::FileData.override_source(paths: ["/etc/launchdarkly/overrides.json"]) | ||
| # config = LaunchDarkly::Config.new(data_system: LaunchDarkly::DataSystem.default.overrides(overrides)) |
There was a problem hiding this comment.
The option is data_system_config:, so copying this example as written gives a client that silently ignores the override.
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in a9befc7: the example now passes data_system_config: LaunchDarkly::DataSystem.default.overrides(overrides).build. The module documentation in lib/ldclient-rb/data_system.rb on main has the same mistake in its three examples (data_system: and no .build); that predates this stack, so I left it for a separate change.
827c4e2 to
40612a6
Compare
fae4a5c to
16d5b70
Compare
40612a6 to
8023cbc
Compare
16d5b70 to
156dc66
Compare
Flag overrides, as defined by the OVERRIDE specification, need three pieces below the store: a marker on the flag and segment model that says a definition came from the override store, evaluator marking that follows every definition an evaluation reads, and an indicator on the evaluation reason that reports the marking to callers. The model classes carry the marker as an attribute that is never serialized. `as_override` returns a marked shallow copy and leaves the original unchanged. Only the SDK components that manage override entries set it. The evaluator marks an evaluation as override-affected when the evaluated flag, a prerequisite flag at any depth, or a segment read during clause matching carries the marker. A segment counts when it is read, so a segment that does not match, or that is read through a negated clause, still marks the evaluation. The marking propagates upward only: a prerequisite's own record reflects the definitions its own subtree read, and a plain prerequisite inside a marked evaluation is not marked. An error result is marked when a marked definition was read before the failure, including when the failure was raised while a prerequisite was being evaluated. An evaluation that reads no marked definition keeps the shared precomputed detail instances. `EvaluationReason#override_affected` carries the indicator. It is written to JSON as `overrideAffected` only when true, so the wire format of ordinary evaluations is unchanged. `with_override_affected` follows the `with_big_segments_status` precedent and returns the same instance when nothing changes. Flag overrides are currently experimental and subject to change.
156dc66 to
d677d6c
Compare
8023cbc to
65c2ed1
Compare
Ruby 3.1 through 3.3 report file modification times in whole seconds on Windows, so a truncation 20 ms after another one does not look like a new write. The reloader specs that need a rewrite to look new now stamp the file's modification time themselves. The listen gem polls on Windows CI and compares the lstat of a symbolic link, so it cannot see a link replaced within the second of its creation. The watcher spec that replaces a link waits for the next second first on a platform with whole-second file times.
Adds the override layer that the OVERRIDE specification describes: a runtime-mutable collection of flag and segment definitions that takes precedence over LaunchDarkly data at the store read boundary. - `LaunchDarkly::Interfaces::Overrides::OverrideSource` and `OverrideSink` define how a source supplies complete snapshots to the SDK. - `Impl::Overrides::Layer` holds marked entries in an immutable hash that is swapped on update, so the layer holds exactly one snapshot at any instant. - `Impl::Overrides::Overlay` sits at the store read boundary. A read for a key returns the override entry when one exists and the LaunchDarkly entry otherwise. An enumeration is the union with the override entry winning. Initialization status is the base store's alone. - `Impl::Overrides::Sink` applies snapshots and notifies flag change listeners of every flag whose merged-view evaluation may have changed, including flags that depend on an overridden prerequisite or segment. - `DataSystem::ConfigBuilder#overrides` accepts one override source builder. The FDv2 data system builds it at construction, so an invalid configuration raises from `LDClient.new`, starts it before its run loop so the initial load is in effect before the constructor returns, serves reads through the overlay, and stops it when the client closes. Offline clients build no source. - The client consults the override store before the not-initialized short-circuit: an overridden flag is served before LaunchDarkly data arrives, and a flag that is not overridden still returns the client-not-ready default. `all_flags_state` reads through the overlay and, before initialization, returns only the overridden flags. A wrong-type migration result keeps the marking of the evaluation it replaces. - `FileData.make_flag_with_value` gains an off form, which the override source uses so that a value-only override reports the OFF reason kind. The file data sources keep their existing form. The OVERRIDE specification's test vectors run as a spec through the full client stack. The per-evaluation summary marker in the vectors is asserted once the event processor carries it. Flag overrides are currently experimental and subject to change.
… changes The override layer keeps a dependency graph of its entries in each snapshot. The store expands every changed LaunchDarkly item through both its own graph and the layer's, to a fixpoint, so a change to a prerequisite flag or a segment that an override entry references notifies that entry's flag as well. This matches how dependent flags behave for LaunchDarkly data. The store's own dependency graph now keys items by string, the form in which the data model names prerequisites and segments. Changes keyed by symbol, which is what the change set carries, previously reached no dependents at all.
…e source from blocking the client Before initialization, all_flags_state returns a state of overrides alone only when the store is not initialized and the override store holds flags, and it reads those flags from the override store itself. An evaluation does the same for the flag it evaluates. Before, a configured source was enough: an empty override store over a store that held LaunchDarkly data without an init marker produced a valid state of that data, and an evaluation served it. The overlay no longer returns the override entries alone when the base store fails to list its items, so a client whose store fails reports an invalid state and logs the failure, as it does without a source. An exception from the source's start no longer prevents the client from starting: it is logged, the source is stopped, a snapshot it supplied is removed, and the client runs without overrides. Stopping the data system closes the sink before it stops the source, so a snapshot supplied during shutdown is ignored. In offline mode the SDK logs that the configured source is not started.
A source logs and retries a failed load, and raises a configuration error from its builder's build. If start raises anyway, the SDK logs it, stops the source, removes its snapshot, and the client runs without overrides.
When the evaluator raises for a flag that came from the override store, variation_detail and all_flags_state now report the EXCEPTION error reason with override_affected set, from the flag's own marker, as the events already do. all_flags_state also no longer raises NoMethodError when an evaluation raises: the failed flag has no evaluation state, so it reports no prerequisites. That crash predates overrides; it made the exception path of all_flags_state unreachable.
The FeatureStore interface documents its entities as hashes, and a custom store can return a plain hash. The evaluator raises on one, and the rescue then called override? on the hash, so variation_detail and all_flags_state raised NoMethodError instead of returning the EXCEPTION result they returned before overrides. A helper now reads the marker only from an object that has it. A spec also shows that a flag whose evaluation raises after a flag with prerequisites reports no prerequisites: block locals start fresh on each iteration.
…he store fails Overrides only add to the all flags state. When the store cannot list its flags, the state without overrides is invalid and empty, so with overrides it now holds the override store's flags and stays invalid, and the failure is logged as before. A client-side SDK bootstrapped from the state uses its flags whatever $valid says, so the overridden values still reach it.
…ields and unimplemented source methods, and skip the fan-out loop without overrides The sink now rejects a deleted entry before it replaces the layer; an override defines a flag or segment, and a deleted one shadowed the LaunchDarkly item with one that could not be evaluated. Building the dependency graph for change notifications no longer raises for an entry whose dependency fields cannot be read, as the layer's own graph already did, so the other entries are still notified. A source that does not implement start or stop, whose interface defaults raise NotImplementedError, no longer stops the client from starting or from stopping the rest of the data system. The store's dependency fan-out calls its own tracker once when no external graph is set, as before external graphs existed, so users without overrides keep the previous cost. The overrides example uses the data_system_config option and builds the configuration.
a9befc7 to
1005ed0
Compare
…ide-affected evaluations (#452) <!-- TERMAG_PR_BODY_BEGIN --> ## Summary This PR is stacked on #451 because it uses the override store and the marking that PR wires into the client. This change carries the override-affected marking from evaluation into analytics events, as the OVERRIDE specification requires. Event handling keys on the marking scalar that the client hands to the event processor, not on the evaluation reason. `EvalEvent` and `record_eval_event` gain an `override_affected` flag. The client sets it from the evaluation result for the evaluated flag, from each prerequisite record's own marking for prerequisite evaluations, and from the flag's own marker when an evaluation raises. An unknown flag is never marked. The event dispatcher produces no individual feature event and no debug event for a marked evaluation, whatever the flag's configuration requests. Marked evaluations appear in summary events only. The summarizer keys counters by version, variation, and the marker, so override-affected and other evaluations of the same flag, variation, and version accumulate into separate counters. The summary event writes `overrideAffected: true` on the marked counters only, following the existing `unknown` marker. Event ingestion must accept that property on summary counters before release. `all_flags_state` presents an override-affected flag with `trackEvents` false, `trackReason` false, and no `debugEventsUntilDate`, so a consumer bootstrapped from the state sends no individual events for it. The flag, its value, its version, and its reason stay in the state. When an evaluation raises, there is no evaluation result, so the flag's own override marker decides, as it does for the reason and for the event of a single evaluation. The specification vectors now also check the summary marker the client hands to the event processor for each evaluation. Verification: processor specs (no feature event, no debug event, marked counter, counter split, unmarked counters unchanged, marked prerequisite record), summarizer spec (split by marker), client specs (record arguments for direct, segment, prerequisite, unknown, and error cases; all-flags tracking fields including experiment tracking and details-only-for-tracked), and end-to-end payload specs through the real event processor (index plus marked summary only for an overridden flag, individual events for an unaffected prerequisite inside a marked evaluation, ordinary events when nothing is overridden). Full suite and RuboCop are clean. Each new spec was checked against a deliberate defect in the code it covers. The existing FDv1 and FDv2 file data sources keep their current behavior. This series does not change them; the override feature is additive. [SDK-3249](https://launchdarkly.atlassian.net/browse/SDK-3249) [SDK-3249]: https://launchdarkly.atlassian.net/browse/SDK-3249?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Analytics now threads an **`override_affected`** marker from evaluations into `EvalEvent` / `record_eval_event`. The client sets it from the evaluation result (and per-prerequisite marking for prereq records; flag-store marker on errors). > > **Event pipeline:** Override-affected evals still hit summary counters and index events, but **no feature or debug events** are emitted even when tracking or debug mode is on. The summarizer buckets counts by version, variation, and the marker; summary payloads add **`overrideAffected: true`** only on marked counters. > > **`all_flags_state`:** For override-affected flags, **`trackEvents`**, **`trackReason`**, and **`debugEventsUntilDate`** are cleared so bootstrapped clients do not send individual events; values/reasons stay present. > > Specs cover processor, summarizer, client wiring, OVERRIDE vectors (`summaryOverrideAffected`), and end-to-end payloads, plus **`RecordingEventProcessor`** for tests. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit e2e01d8. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- TERMAG_PR_BODY_END --> <!-- termag: box=i-0575d1c54853058d7 project=projects --> <sub>Opened from termag</sub>
Summary
This PR is stacked on #450 because it uses the override marker, the evaluator marking, and the reason indicator that PR adds.
This change adds the override layer that the OVERRIDE specification describes: a runtime-mutable collection of flag and segment definitions that takes precedence over LaunchDarkly data at the store read boundary, populated by an override source through a sink. With no override source configured the SDK behaves exactly as before. The file-based source, the events changes, and the contract test service follow in later PRs.
LaunchDarkly::Interfaces::Overrides::OverrideSourceandOverrideSinkdefine how a source supplies complete snapshots to the SDK. The sink accepts data model objects or hashes in the flag and segment data model. The SDK marks the entries itself.Impl::Overrides::Layerholds marked entries in an immutable hash that is swapped on update, so the layer holds exactly one snapshot at any instant and reads are lock-free.Impl::Overrides::Overlaysits at the store read boundary: a read for a key returns the override entry when one exists and the LaunchDarkly entry otherwise, an enumeration is the union with the override entry winning, and initialization status is the base store's alone.Impl::Overrides::Sinkapplies snapshots and notifies flag change listeners of every flag whose merged-view evaluation may have changed, including flags that depend on an overridden prerequisite or segment. It computes dependency edges over both the old and the new merged view, as the Go reference does.DataSystem::ConfigBuilder#overridesaccepts one override source builder, the samebuild(sdk_key, config)protocol as the data source builders. The FDv2 data system builds the source at construction, so an invalid configuration raises fromLDClient.newlike other invalid component configuration. It starts the source before its run loop, so the initial load is in effect before the constructor returns. An exception from the source'sstartdoes not prevent the client from starting: the SDK logs it, stops the source, removes any snapshot the source supplied, and the client runs without overrides (the SDK's own file source does not raise fromstart; a custom source could). It serves reads through the overlay. When the client closes, the sink stops accepting snapshots before the source is stopped, and an exception fromstopis logged. An offline client builds no source and logs a warning that the configured source is not started.The client consults the override store before the not-initialized short-circuit. An overridden flag is served before LaunchDarkly data arrives, read from the override store itself, and a flag that is not overridden still returns the client-not-ready default, even when a store without an init marker holds LaunchDarkly data for it.
all_flags_statereads through the overlay. Before initialization, when the store is not initialized and the override store holds flags, it returns a state of those flags alone, read from the override store, with a once-per-client warning; with no override flags the state is invalid, as it is without a source. Without a source this state is empty, because the SDK reports no flags before the store is initialized, so the override flags only add to what it reports; overrides never remove a flag from the state. A failure of the base store to list its flags propagates through the overlay, andall_flags_statelogs it as without a source. Without a source the state is then invalid and empty; with override flags it holds those flags and stays invalid, so the overrides only add to it. A client-side SDK bootstrapped from the state uses its flags whatever$validsays (js-client-sdk 3.x and the 4.x browser SDK only log a warning), so the overridden values still reach it. A wrong-type migration result keeps the marking of the evaluation it replaces. When the evaluator raises for a flag that came from the override store,variation_detailandall_flags_statereport theEXCEPTIONerror reason withoverride_affectedset, from the flag's own marker. This also fixes a pre-existing crash:all_flags_stateraisedNoMethodErrorwhen an evaluation raised, because the failed flag has no evaluation state; it now reports that flag with no prerequisites. The marker is read only from the SDK's model objects: a custom store can return a flag as a plain hash, as theFeatureStoreinterface documents, and such a flag never came from the override store, so its failed evaluation still returns the unmarkedEXCEPTIONresult instead of raising.A value-only override expands, through the file loading code from #449, into a flag that is on and serves its single value by fallthrough, which reports the FALLTHROUGH reason kind as the specification describes.
The OVERRIDE specification's test vectors (
spec/fixtures/override-vectors/vectors.json) run as a spec through the full client stack, checking value, variation index, and reason under the vectors' comparison rules. The vectors' per-evaluation summary marker is asserted once the event processor carries the marking, in the events PR.LaunchDarkly changes reach override dependents.
Layerkeeps a dependency graph of its entries in each snapshot, andStoreexpands every changed LaunchDarkly item through both its own graph and the layer's, to a fixpoint. A change to a prerequisite flag or a segment that an override entry references therefore notifies that entry's flag, the same way dependent flags are notified for LaunchDarkly data, and a chain may cross between LaunchDarkly data and overrides any number of times.The FDv2 store's defect that seeded its dependency fan-out with symbol keys, while
Impl::DependencyTrackerindexes dependents under string keys, is fixed onmainby #470, which the stack's base now includes. The fan-out helper here also compares keys as strings, which the fixpoint needs in any case, and the store spec keeps the #470 specs beside this PR's specs for the override layer.Verification: specs for the layer (marking, replacement, snapshot atomicity under concurrent writers), the overlay (precedence, union, deleted base items, base failure, initialization), the sink (added, removed, changed, and unchanged entries, dependency fan-out through prerequisites and nested segments, both merged views, no-listener path, base failure, serialized updates), the FDv2 wiring (construction, start order, overlay reads, stop, offline, availability unaffected), the client (not-ready gate, all-flags state, change notifications through the flag tracker, migration marker, lifecycle), and the specification vectors. Full suite and RuboCop are clean, and the full suite passes on JRuby 9.4 (the specs wait on queues without
Queue#pop's timeout, which Ruby 3.1 and JRuby 9.4 lack). Each new spec was checked against a deliberate defect in the code it covers.The existing FDv1 and FDv2 file data sources keep their current behavior. This series does not change them; the override feature is additive.
SDK-3249
Note
Overview
feat: Add the override store, overlay, and data system wiring
Adds experimental, runtime flag/segment overrides:
DataSystem::ConfigBuilder#overridesaccepts anOverrideSourcebuilder, FDv2 buildsLayer+Overlay+Sink, wiresStoreto cross-graph dependency notifications, and extends evaluator/reason plumbing so prerequisites, segments, and client APIs respect override markers while still documenting that the surface is experimental.Highlights
Impl::Overrides::{Layer,Overlay,Sink}) plusInterfaces::Overridescontracts mark model objects, merge overrides ahead of LD data, and fan-out flag-change listeners using merged dependency graphs.EvaluationReason, evaluator state, prerequisite records, migrations, andLDClientpaths (variation_detail,all_flags_state) now propagateoverrideAffected, handle uninitialized stores by consulting pure override snapshots, and keep JSON compatibility by emitting the bit only when true.start/stop), exposesoverride_items, and coordinates withStorevia external dependency graphs; specs/vectors cover layer semantics, notifier behavior, client flows, and file-data reloader timing fixes on coarse clocks.Reviewed by Cursor Bugbot for commit 1005ed0. Bugbot is set up for automated code reviews on this repo. Configure here.
The all-flags report keeps an override-affected flag's
trackEvents,trackReason, anddebugEventsUntilDatein this PR; #452 turns them off (OVERRIDE Requirement 1.4.1.1) together with the rest of the event policy for override-affected evaluations.Opened from termag