Repository navigation
feat: Add the override store, overlay, and data system wiring - #527
Conversation
34322d8 to
ee313c1
Compare
3fcdc80 to
19f88eb
Compare
ee313c1 to
2c70075
Compare
19f88eb to
7e7b3f4
Compare
2c70075 to
5f738a6
Compare
7e7b3f4 to
aa28264
Compare
5f738a6 to
0013c0e
Compare
aa28264 to
1c1bd5c
Compare
|
bugbot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
0013c0e to
85a71df
Compare
1c1bd5c to
3cd92be
Compare
85a71df to
f3db8c6
Compare
3cd92be to
cbbe302
Compare
|
bugbot review |
|
bugbot review |
1 similar comment
|
bugbot review |
|
bugbot review |
|
bugbot review |
f3db8c6 to
d9973c8
Compare
8fdc8b5 to
0656fef
Compare
02a32ab to
6393a98
Compare
0656fef to
b8f3c91
Compare
6393a98 to
29b98ac
Compare
03595d1 to
c571029
Compare
29b98ac to
1debdc2
Compare
Adds the override layer described by the OVERRIDE specification: an override store that holds marked flag and segment definitions, an overlay at the store read boundary that returns the override entry for a key in preference to LaunchDarkly data and enumerates the union of both, and a sink that applies each snapshot as a single replacement and notifies flag change listeners of every flag whose evaluation may have changed, including flags that depend on a changed prerequisite or segment. The override source is an option of the FDv2 data system configuration: DataSystemConfig.override_source and ConfigBuilder.overrides(). The source is built with the data system, so invalid configuration raises from the client constructor. It starts before the data system's own threads, so its initial load completes before the constructor returns, and it is closed with the client. OverrideSource and OverrideSink are public protocols so a custom source can be supplied. The client consults the override store before its not-initialized short-circuit. An overridden flag is served before the client has LaunchDarkly data, and a flag absent from the override store still returns the client-not-ready default. The all-flags state reads through the overlay and, while the client has no LaunchDarkly data, contains only the overridden flags. The async client and data system receive the same changes, with override change notifications delivered on the event loop. The OVERRIDE specification test vectors run as a unit test through the full client stack.
…arkly data Before initialization the clients consult the override layer directly. A flag the layer does not hold gets the not-ready handling it had without an override source, for an invalid context, a store outage, and an uninitialized persistent store alike. The all-flags state is unavailable when the layer is empty. The sync client stops the components it started when the data system fails to start.
… changes The override layer keeps a dependency graph of its own entries, rebuilt on each replacement. 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.
…iled override source from stopping the client all_flags_state returns a state of overrides alone only when no LaunchDarkly data is available and the override layer holds flags, and it reads those flags from the layer itself. Before, any layer entry was enough: a layer of segments alone over a store that held LaunchDarkly data without an init marker produced a valid state of that data, and with override flags present the same data entered the state. The overlay no longer serves the layer's 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 override source's start no longer fails client construction or start. The data system logs it, closes the source, clears what the source supplied, and the client runs without overrides. The SDK's own file-based source does not raise from start; a custom source could. Stopping the data system closes the override sink before it closes the source, so a snapshot supplied during shutdown is ignored. When the client is offline and an override source is configured, the SDK logs a warning that the 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, closes the source, removes its snapshot, and the client runs without overrides.
…nnot list its flags When the store's all-items read fails, the client logs the failure as before and, if the override layer holds flags, returns a state of those flags marked invalid instead of an empty one. Overrides only add to the state, and a client-side SDK bootstrapped from it uses its flags whatever $valid says, so the overridden values still reach it. With no override flags the state is the same invalid, empty state as without a source.
…flag An evaluation that fails after reading a definition from the override layer is still marked (OVERRIDE 1.5.4). The single-flag and all-flags error paths of both clients now set overrideAffected on the EXCEPTION reason from the flag's own marker, read through is_override_definition so that the error path cannot fail on an unexpected store item.
c571029 to
2fc0ad0
Compare
keelerm84
left a comment
keelerm84
left a comment
There was a problem hiding this comment.
Two things I'd like to see in the description, so they don't look like oversights later:
- The async client computes its change fan-out over the in-memory store (
async_fdv2.py:340-345). While a persistent store is the active store, a LaunchDarkly flag that depends on an overridden flag is not notified when the override changes. The directly overridden keys still are. The code comment covers this, but users of the async client with a persistent store should hear about it from the docs as well. all_flags_statestill keepstrackEvents,trackReasonanddebugEventsUntilDatefor override-affected flags (1.4.1.1). That lands with the events step, so a line saying it's deferred would help anyone reviewing this PR on its own.
| replacement: LayerContents = { | ||
| FEATURES: {key: FEATURES.decode(item).with_override_marker() for key, item in flags.items()}, | ||
| SEGMENTS: {key: SEGMENTS.decode(item).with_override_marker() for key, item in segments.items()}, | ||
| } |
There was a problem hiding this comment.
set_all accepts a definition with "deleted": true. The overlay's get and the not-initialized path in client.py:508 then serve it, without the deleted filter that the base store applies (store.py:72). FeatureFlag.__init__ returns early for a deleted item, so the slotted fields are never set, and evaluation raises. The except block at client.py:547 (async_client.py:560) then calls new_default_event(flag, ...), which reads flag.track_events and raises AttributeError outside any try. That exception reaches the application from variation() and variation_detail().
The SDK's own file source can produce this: {"flags": {"f": {"deleted": true}}} gets through _definition on the file-source branch. Someone who builds an override file from an exported payload could hit it.
Could set_all drop deleted items? The file data source behaves that way today, because the store filters them, so a deleted entry means "no override for this key". It would also be good to have a test for both clients.
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
We went with rejecting a deleted entry rather than dropping it, and made that the rule in the spec and in all six SDKs.
- A deleted item is a tombstone, which exists only for storage in a data store. An override entry defines a flag or segment, and an override is removed by removing its entry.
- Dropping the entry would silently ignore what the operator wrote. For example, someone who writes
"deleted": trueduring an incident to switch a flag off would see the LaunchDarkly value unchanged, with no error.
The spec change is launchdarkly/sdk-specs#288 (Requirements 1.1.1.1, 1.2.8.1, 1.2.8.2 and 1.3.6.2):
- A snapshot that contains a flag or segment marked as deleted is rejected whole, and the overrides in effect stay. The SDK logs the rejection, and
set_overridesraisesValueErrorto the source. - An override file with such an entry fails the reload, the same way as a file that cannot be parsed, and the log names the file and the key.
"deleted": false, as in entries copied from LaunchDarkly data, still loads.
For Python this is #574, stacked on top of this stack. It covers both clients: variation() and variation_detail() no longer raise, with a deleted entry from a custom source or from the file source. The contract tests are in launchdarkly/sdk-test-harness#599, and they pass against #574 with both the sync and async test services.
| replacement: LayerContents = { | ||
| FEATURES: {key: FEATURES.decode(item).with_override_marker() for key, item in flags.items()}, | ||
| SEGMENTS: {key: SEGMENTS.decode(item).with_override_marker() for key, item in segments.items()}, | ||
| } |
There was a problem hiding this comment.
The layer keys each entry by its mapping key, but all_flags_state reports flag['key'] (client.py:636, :656), and events use the definition's key too. If a custom source supplies {'a': <definition with key 'b'>}, variation('a') serves the override with events keyed b. Meanwhile all_flags_state drops LaunchDarkly flag a and shows LaunchDarkly's b in place of the override. The file source can't produce this because it forces the key, but OverrideSource is public, and nothing in the set_overrides docstring rules this case out. A ValueError on a mismatch here, which set_overrides already documents for invalid definitions, would make the error visible to the source instead of corrupting the report.
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in 0489514. OverrideLayer.set_all raises ValueError when a definition's key differs from its key in the mapping, and the layer is left unchanged. The set_overrides docs now say that each definition's key must be its key in the mapping.
Tests:
test_overrides.pycovers a flag and a segment, as a model object and as a dict.- In
test_ldclient_overrides.py, a source supplies{'a': <definition with key 'b'>}. The snapshot is rejected,akeeps its previous override,bkeeps its LaunchDarkly value, and the all-flags state shows both.
| def _close_components(self): | ||
| """ | ||
| Stops the SDK components after a failed start-up. The event processor is last because | ||
| it may still be sending events the other components generated. | ||
| """ | ||
| self._data_system.stop() | ||
| self.__big_segment_store_manager.stop() | ||
| self._event_processor.stop() |
There was a problem hiding this comment.
If _data_system.stop() raises here, that exception replaces the one from start. big_segment_store_manager.stop() and _event_processor.stop() are then skipped, so the event processor's threads leak, which this teardown was meant to prevent. Could each stop() go in its own try/except that logs, so every component is stopped and the caller still gets the original start failure?
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in 0489514. _close_components stops the data system, the big segment store manager, and the event processor each in its own try/except. A failure to stop is logged as a warning, so every component is stopped and the constructor raises the original start failure. A new test makes the data system's stop raise, and checks that the other two components still stop and that the start failure reaches the caller.
The async client's _close_components has the same pattern. It came from main (#486) and this PR does not change it, so I've left it for a follow-up.
…nent after a failed start The override layer rejects a snapshot whose definition has a key other than its key in the mapping, because evaluation reads the mapping key while the all-flags report and events use the definition key. After a failed start, each component is stopped in turn and a failure to stop is logged, so the start-up failure reaches the caller and the event processor still stops. The async override docs say that, with a persistent store, a change to an override does not notify LaunchDarkly flags that depend on it.
|
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion. @keelerm84, both points from your review are now in the description:
|
|
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 0489514. Configure here.
Summary
This is the third step of the flag overrides port described by the OVERRIDE specification. It is based on the model marker branch because the phases are stacked; retarget to
feat/overridesonce that branch merges.The override layer (
ldclient/impl/overrides).OverrideLayeris the override store: it holds marked shallow copies of the flag and segment definitions a source supplies, keyed by key, and is replaced wholesale on each update, so the layer's contents are exactly one snapshot at any instant and an empty snapshot clears it. Dictionaries are decoded with the model constructors, and an invalid definition raises rather than being defaulted. A definition whose key differs from its key in the snapshot raises too, and the layer is left unchanged: evaluation reads an entry by its snapshot key, while the all-flags state and events use the definition's key.OverrideStoreViewis the overlay at the store read boundary: a read of a flag or segment returns the override entry when one exists and the LaunchDarkly entry otherwise, whatever the state of the base store, and an enumeration is the union of both with the override entry winning, including over a deleted item. A failure of the base enumeration propagates, as it does without the overlay. When that read fails,all_flags_statelogs the failure as before and, if the override layer holds flags, returns a state of those flags marked invalid rather than an empty one: overrides only add to the state, and a client-side SDK bootstrapped from it uses its flags whatever$validsays, so the overridden values still reach it. With no override flags the state is the same invalid, empty state as without a source. The sink stops accepting snapshots when the client is closed, and when the source fails to start it also clears what the source supplied.OverrideSinkImplapplies each snapshot as one serialized operation and notifies flag change listeners of every flag whose merged-view evaluation may have changed: entries added, removed, or changed, plus every flag that depends on them through prerequisites or segment references. An identical snapshot notifies nothing.LaunchDarkly changes reach override dependents. The layer keeps a dependency graph of its entries, rebuilt on each replacement, and the store (sync and async) expands 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. Without an override source the store's behavior is unchanged.
Configuration and wiring.
OverrideSourceandOverrideSinkare public protocols inldclient.interfaces, so a custom source can be supplied.OverrideSourceBuilderandDataSystemConfig.override_source(also onAsyncDataSystemConfig) plusConfigBuilder.overrides(...)make the source an option of the FDv2 data system. The source is built with the data system, so invalid configuration raises from the client constructor like any other invalid component configuration. It starts before the data system's own threads and its initial load completes before the constructor returns; it is closed when the client is closed, after the sink stops accepting snapshots, and an exception from itscloseis logged. An exception from itsstartdoes not fail the constructor orstart: the data system logs it, closes the source, clears any snapshot the source supplied before failing, and the client runs without overrides. The SDK's own file-based source does not raise fromstart; a custom source could. The source is not built or started when the client is offline, and the SDK logs a warning that it is not used. The data system'sstorebecomes the overlay when a source is configured, so evaluation, prerequisite and segment resolution, and the all-flags read all see override precedence. The override source has no effect on initialization status, data availability, or data source status. The legacy FDv1 data system reports no override source. When the data system fails to start, the client constructor stops each component in turn and logs any failure to stop, so the event processor still stops and the caller receives the start failure.The client facade. Both
LDClientandAsyncLDClientconsult the override layer before the not-initialized short-circuit: an overridden flag is served before the client has LaunchDarkly data, and a flag the layer does not hold takes exactly the path it took before this feature, so a configured but empty source changes nothing with the same warning and event as before. The all-flags state reads through the overlay. While the client has no LaunchDarkly data it contains only the flags the layer holds, read from the layer rather than the overlay so that LaunchDarkly data in a store without an init marker stays out (with a once-per-client warning), and it is invalid when the layer holds no flags. 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. An evaluation that fails with an unexpected exception reports theEXCEPTIONreason withoverrideAffectedwhen the flag came from the override layer, in a single evaluation and in the all-flags state (OVERRIDE 1.5.4); the marker is read throughis_override_definition, so the error path cannot fail on an unexpected store item. On the async client, override change notifications are marshalled onto the event loop, like every other change notification.The async client with a persistent store. The async client works out which flags an override change affects from the in-memory store, the only store it can read synchronously (
async_fdv2.py). While a persistent store is the active store, a change to an override notifies the overridden flags, but not LaunchDarkly flags that depend on them through a prerequisite or a segment. The documentation ofAsyncDataSystemConfig.override_sourcesays so.Left to later PRs in the stack.
all_flags_statestill keepstrackEvents,trackReasonanddebugEventsUntilDatefor override-affected flags (OVERRIDE 1.4.1.1). The events step, feat: Mark override-affected evaluations in analytics events #528, clears them.Tests. Unit tests for the layer, overlay (sync and async), and sink; client tests for the gate, source lifecycle, construction errors, offline, precedence, prerequisite and segment overrides, all-flags state, and flag change and flag value change listeners, for both clients; and a runner for the OVERRIDE specification test vectors (
ldclient/testing/testdata/override-vectors/vectors.json) through the full client stack. The vector runner's per-evaluation summary marker assertion is added with the events step.SDK-3250
Note
Overview
Adds experimental flag/segment overrides that can take precedence over LaunchDarkly data at evaluation time, wired through FDv2 (sync and async).
Configuration: Optional
override_sourceon the data system (ConfigBuilder.overrides(...),OverrideSourceBuilder, publicOverrideSource/OverrideSinkprotocols). Offline clients skip building/starting the source.Runtime: An
OverrideLayerholds snapshot replacements;OverrideStoreViewoverlays the feature store so per-key reads prefer overrides;OverrideSinkImplapplies updates and fans out flag change notifications via merged-view dependency diffing. The store can register the layer as external dependencies so LaunchDarkly prerequisite/segment updates also notify affected override flags.Clients:
LDClient/AsyncLDClientread the override layer before the not-initialized default path and adjustall_flags_stateto return override-only bootstrapping when LD data is unavailable (with once-per-client warnings). Evaluation errors setoverrideAffectedwhen the definition came from overrides. Sync client rolls back components if data system start fails.Tests: Layer/overlay/sink units, broad sync+async client scenarios, store cross-graph notification, and OVERRIDE spec vectors (
override-vectors/vectors.json).Reviewed by Cursor Bugbot for commit 0489514. Bugbot is set up for automated code reviews on this repo. Configure here.
Opened from termag