Skip to content

Conversation

@jsonbailey
Copy link
Contributor

@jsonbailey jsonbailey commented Dec 2, 2025

Note

Introduces FDv1 data system wiring to existing v1 processors, adds NullUpdateProcessor, updates the DataSystem API, integrates with LDClient, and adds comprehensive specs.

  • Data System (FDv1):
    • Adds LaunchDarkly::Impl::DataSystem::FDv1 that wires v1 data source/store behind the DataSystem interface.
    • Supports custom data source factories/instances, passes diagnostic_accumulator to streaming, and manages broadcasters, status providers, and flag tracking.
    • Computes data_availability and target_availability based on config, processor, and store state.
  • Null Data Source:
    • Adds LaunchDarkly::Impl::DataSource::NullUpdateProcessor that instantly marks initialized and no-ops on stop.
  • DataSystem API:
    • Changes start to take no args and return a Concurrent::Event.
    • Adds set_diagnostic_accumulator hook.
    • Expands DataAvailability (REFRESHED, ALL, .at_least?).
    • Adds diagnostic mixins (DiagnosticAccumulator, DiagnosticSource) and protocol mixins (Initializer, Synchronizer), plus Update value object.
  • LDClient Integration:
    • Uses namespaced NullUpdateProcessor; removes internal NullUpdateProcessor class.
    • Keeps default data source creation logic; passes diagnostics to stream processor.
  • Tests:
    • Adds spec/impl/datasystem/fdv1_spec.rb covering processor selection, diagnostics, availability, and lifecycle.
    • Updates spec/impl/datasystem_spec.rb for new API and mixins; enhances spec/mock_components.rb with MockUpdateProcessor and namespaced null processor.

Written by Cursor Bugbot for commit 66b8876. This will update automatically on new commits. Configure here.

@jsonbailey jsonbailey requested a review from a team as a code owner December 2, 2025 22:23
# @return [Concurrent::Event] Event that will be set when initialization is complete
#
def start(ready_event)
def start
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a little torn on having the ready_event passed in vs returning it. The existing data_source public interface for its start method returns a Concurrent::Event. This means we would either need an interface layer to sync the two events between the data system and the data source, or we would need a breaking change to have the data source interface accept the event vs return it.


if @update_processor && @update_processor.initialized?
return DataAvailability::REFRESHED
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: LDD mode incorrectly reports data availability as refreshed

In LDD (LaunchDarkly Daemon) mode, data_availability returns REFRESHED even when the data store is empty. After start() is called, the NullUpdateProcessor.initialized? check always passes (since it unconditionally returns true), causing REFRESHED to be returned without verifying the store has any data. In LDD mode, data comes from an external daemon populating the store, so availability should be determined by the store's initialization state, not the null processor's state. This could mislead consumers into thinking fresh data is available when the store is actually empty.

Additional Locations (1)

Fix in Cursor Fix in Web

#
def set_flag_value_eval_fn(eval_fn)
@flag_tracker_impl = LaunchDarkly::Impl::FlagTracker.new(@flag_change_broadcaster, eval_fn)
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Flag tracker replacement loses references to original instance

The set_flag_value_eval_fn method replaces @flag_tracker_impl with an entirely new FlagTracker instance. If any code calls flag_tracker before this method runs and caches the returned reference, that cached reference will continue pointing to the old tracker with the dummy evaluation function (lambda { |_key, _context| nil }). Flag value change listeners added via the stale reference would use the wrong eval function, always computing flag values as nil and failing to detect actual value changes. A better approach would be to update the eval function on the existing tracker instance rather than replacing it.

Additional Locations (1)

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants