Repository navigation
feat: Add the override store, overlay, and data system wiring - #451
kinyoklion wants to merge 10 commits into
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
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.
156dc66 to
d677d6c
Compare
8023cbc to
65c2ed1
Compare
…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.
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
Adds experimental flag overrides: optional per-key flag/segment definitions that win at the store read boundary via
DataSystem::ConfigBuilder#overrides, without affecting init or data-source status when no source is configured.FDv2 builds and starts an
OverrideSource(skipped offline; start failures log and run without overrides), applies snapshots throughLayer/Overlay/Sink, and exposesoverride_items.LDClientserves overridden flags before the not-ready path, adjustsall_flags_statewhen LD data is unavailable, and propagatesoverride_affectedon errors; evaluation failures inall_flags_stateno longer crash when prerequisites are missing.Impl::DataStore::Storelinks the override layer for cross-graph dependency fan-out on flag-change notifications and fixes dependency tracking to use string keys so dependent flags are notified correctly.Reviewed by Cursor Bugbot for commit 0a4c8b7. 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