diff --git a/lib/ldclient-rb/impl/data_store/feature_store_client_wrapper.rb b/lib/ldclient-rb/impl/data_store/feature_store_client_wrapper.rb index fbbd1ea6..5c86147f 100644 --- a/lib/ldclient-rb/impl/data_store/feature_store_client_wrapper.rb +++ b/lib/ldclient-rb/impl/data_store/feature_store_client_wrapper.rb @@ -65,6 +65,18 @@ def initialized? @store.initialized? end + # + # Passthrough for the FDv2 store coordinator. If the wrapped store supports + # disabling its cache (e.g. {LaunchDarkly::Integrations::Util::CachingStoreWrapper}), + # forward the call; otherwise no-op. + # + # @return [void] + # + def disable_cache + return unless @store.respond_to?(:disable_cache) + wrapper { @store.disable_cache } + end + # (see LaunchDarkly::Interfaces::FeatureStore#stop) def stop poller_to_stop = nil diff --git a/lib/ldclient-rb/impl/data_store/store.rb b/lib/ldclient-rb/impl/data_store/store.rb index 93ac3989..f374b9d4 100644 --- a/lib/ldclient-rb/impl/data_store/store.rb +++ b/lib/ldclient-rb/impl/data_store/store.rb @@ -221,6 +221,12 @@ def get_data_store_status_provider # Switch to memory store as active @active_store = @memory_store + # In-memory store is now authoritative. Disable the persistent-store cache + # so we don't hold a duplicate copy of every flag. Done before the + # persistent_store.init below so the wrapper can skip its cache-population + # loop on this very call. + disable_persistent_cache + # Persist to persistent store if configured and writable @persistent_store.init(collections) if should_persist? @@ -271,6 +277,24 @@ def get_data_store_status_provider send_change_events(affected_items) unless affected_items.empty? end + # + # Disable the persistent store's in-memory cache, if it has one. The FDv2 in-memory + # store is now the source of truth, so the cache is dead weight and would just + # hold a duplicate copy of every flag. + # + # @return [void] + # + private def disable_persistent_cache + return if @persistent_store.nil? + return unless @persistent_store.respond_to?(:disable_cache) + + begin + @persistent_store.disable_cache + rescue => e + @logger.warn { "[LDClient] Failed to disable persistent store cache: #{e.message}" } + end + end + # # Returns whether data should be persisted to the persistent store. # diff --git a/lib/ldclient-rb/impl/integrations/redis_impl.rb b/lib/ldclient-rb/impl/integrations/redis_impl.rb index 0a0a3a2e..83fa7dd9 100644 --- a/lib/ldclient-rb/impl/integrations/redis_impl.rb +++ b/lib/ldclient-rb/impl/integrations/redis_impl.rb @@ -93,6 +93,10 @@ def initialized? def stop @wrapper.stop end + + def disable_cache + @wrapper.disable_cache + end end class RedisStoreImplBase diff --git a/lib/ldclient-rb/integrations/consul.rb b/lib/ldclient-rb/integrations/consul.rb index 1365baf9..e1ba4abd 100644 --- a/lib/ldclient-rb/integrations/consul.rb +++ b/lib/ldclient-rb/integrations/consul.rb @@ -32,8 +32,12 @@ def self.default_prefix # @option opts [String] :url shortcut for setting the `url` property of the Consul client configuration # @option opts [String] :prefix namespace prefix to add to all keys used by LaunchDarkly # @option opts [Logger] :logger a `Logger` instance; defaults to `Config.default_logger` - # @option opts [Integer] :expiration (15) expiration time for the in-memory cache, in seconds; 0 for no local caching - # @option opts [Integer] :capacity (1000) maximum number of items in the cache + # @option opts [Integer] :expiration (15) expiration time for the in-memory cache, in seconds; 0 for no local caching. + # When the SDK is configured to use FDv2, the persistent-store cache is automatically + # dropped once the in-memory store has been initialized, so this setting only affects + # the brief bootstrap window before the first set of flag data has been received. + # @option opts [Integer] :capacity (1000) maximum number of items in the cache. + # Same FDv2 caveat as `:expiration` applies. # @return [LaunchDarkly::Interfaces::FeatureStore] a feature store object # def self.new_feature_store(opts = {}) diff --git a/lib/ldclient-rb/integrations/dynamodb.rb b/lib/ldclient-rb/integrations/dynamodb.rb index 52e05cf3..7f4b4425 100644 --- a/lib/ldclient-rb/integrations/dynamodb.rb +++ b/lib/ldclient-rb/integrations/dynamodb.rb @@ -42,8 +42,12 @@ module DynamoDB # @option opts [Object] :existing_client an already-constructed DynamoDB client for the feature store to use # @option opts [String] :prefix namespace prefix to add to all keys used by LaunchDarkly # @option opts [Logger] :logger a `Logger` instance; defaults to `Config.default_logger` - # @option opts [Integer] :expiration (15) expiration time for the in-memory cache, in seconds; 0 for no local caching - # @option opts [Integer] :capacity (1000) maximum number of items in the cache + # @option opts [Integer] :expiration (15) expiration time for the in-memory cache, in seconds; 0 for no local caching. + # When the SDK is configured to use FDv2, the persistent-store cache is automatically + # dropped once the in-memory store has been initialized, so this setting only affects + # the brief bootstrap window before the first set of flag data has been received. + # @option opts [Integer] :capacity (1000) maximum number of items in the cache. + # Same FDv2 caveat as `:expiration` applies. # @return [LaunchDarkly::Interfaces::FeatureStore] a feature store object # def self.new_feature_store(table_name, opts = {}) diff --git a/lib/ldclient-rb/integrations/redis.rb b/lib/ldclient-rb/integrations/redis.rb index 0e5bf68c..e78b22ba 100644 --- a/lib/ldclient-rb/integrations/redis.rb +++ b/lib/ldclient-rb/integrations/redis.rb @@ -50,8 +50,12 @@ def self.default_prefix # @option opts [String] :prefix (default_prefix) namespace prefix to add to all hash keys used by LaunchDarkly # @option opts [Logger] :logger a `Logger` instance; defaults to `Config.default_logger` # @option opts [Integer] :max_connections size of the Redis connection pool - # @option opts [Integer] :expiration (15) expiration time for the in-memory cache, in seconds; 0 for no local caching - # @option opts [Integer] :capacity (1000) maximum number of items in the cache + # @option opts [Integer] :expiration (15) expiration time for the in-memory cache, in seconds; 0 for no local caching. + # When the SDK is configured to use FDv2, the persistent-store cache is automatically + # dropped once the in-memory store has been initialized, so this setting only affects + # the brief bootstrap window before the first set of flag data has been received. + # @option opts [Integer] :capacity (1000) maximum number of items in the cache. + # Same FDv2 caveat as `:expiration` applies. # @option opts [Object] :pool custom connection pool, if desired # @option opts [Boolean] :pool_shutdown_on_close whether calling `close` should shutdown the custom connection pool; # this is true by default, and should be set to false only if you are managing the pool yourself and want its diff --git a/lib/ldclient-rb/integrations/util/store_wrapper.rb b/lib/ldclient-rb/integrations/util/store_wrapper.rb index 03e3a91f..402e59ec 100644 --- a/lib/ldclient-rb/integrations/util/store_wrapper.rb +++ b/lib/ldclient-rb/integrations/util/store_wrapper.rb @@ -61,50 +61,52 @@ def init(all_data) @core.init_internal(all_data) @inited.make_true - unless @cache.nil? - @cache.clear + cache = @cache + unless cache.nil? + cache.clear all_data.each do |kind, items| - @cache[kind] = items_if_not_deleted(items) + cache[kind] = items_if_not_deleted(items) items.each do |key, item| - @cache[item_cache_key(kind, key)] = [item] + cache[item_cache_key(kind, key)] = [item] end end end end def get(kind, key) - unless @cache.nil? - cache_key = item_cache_key(kind, key) - cached = @cache[cache_key] # note, item entries in the cache are wrapped in an array so we can cache nil values + cache = @cache + cache_key = item_cache_key(kind, key) + unless cache.nil? + cached = cache[cache_key] # note, item entries in the cache are wrapped in an array so we can cache nil values return item_if_not_deleted(cached[0]) unless cached.nil? end item = @core.get_internal(kind, key) - unless @cache.nil? - @cache[cache_key] = [item] - end + cache[cache_key] = [item] unless cache.nil? item_if_not_deleted(item) end def all(kind) - unless @cache.nil? - items = @cache[all_cache_key(kind)] + cache = @cache + unless cache.nil? + items = cache[all_cache_key(kind)] return items unless items.nil? end items = items_if_not_deleted(@core.get_all_internal(kind)) - @cache[all_cache_key(kind)] = items unless @cache.nil? + cache[all_cache_key(kind)] = items unless cache.nil? items end def upsert(kind, item) new_state = @core.upsert_internal(kind, item) - unless @cache.nil? - @cache[item_cache_key(kind, item[:key])] = [new_state] - @cache.delete(all_cache_key(kind)) + cache = @cache + unless cache.nil? + cache[item_cache_key(kind, item[:key])] = [new_state] + cache.delete(all_cache_key(kind)) end end @@ -115,13 +117,14 @@ def delete(kind, key, version) def initialized? return true if @inited.value - if @cache.nil? + cache = @cache + if cache.nil? result = @core.initialized_internal? else - result = @cache[inited_cache_key] + result = cache[inited_cache_key] if result.nil? result = @core.initialized_internal? - @cache[inited_cache_key] = result + cache[inited_cache_key] = result end end @@ -129,6 +132,23 @@ def initialized? result end + # + # Disable the in-memory cache. Releases the cache reference so subsequent operations + # bypass it and go directly to the underlying core. Safe to call multiple times. + # + # Called by the FDv2 store coordinator once the in-memory store has become the + # source of truth and the persistent-store cache is no longer useful. Internal -- + # not part of the public API. + # + # @return [void] + # + def disable_cache + cache = @cache + return if cache.nil? + @cache = nil + cache.clear + end + def stop @core.stop end diff --git a/spec/impl/data_store/feature_store_client_wrapper_spec.rb b/spec/impl/data_store/feature_store_client_wrapper_spec.rb new file mode 100644 index 00000000..0a390922 --- /dev/null +++ b/spec/impl/data_store/feature_store_client_wrapper_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require "spec_helper" +require "ldclient-rb/impl/data_store/feature_store_client_wrapper" + +module LaunchDarkly + module Impl + module DataStore + describe FeatureStoreClientWrapperV2 do + let(:logger) { double.as_null_object } + let(:status_sink) { double(update_status: nil) } + + describe "#disable_cache" do + it "forwards to the underlying store when supported" do + inner = double("store", disable_cache: nil, init: nil, get: nil, all: nil, delete: nil, upsert: nil, initialized?: false, stop: nil) + expect(inner).to receive(:disable_cache).once + + wrapper = described_class.new(inner, status_sink, logger) + wrapper.disable_cache + end + + it "is a no-op when the underlying store does not respond to disable_cache" do + inner = Class.new do + def init(_); end + def get(_, _); end + def all(_); end + def delete(_, _, _); end + def upsert(_, _); end + def initialized?; false; end + def stop; end + end.new + + wrapper = described_class.new(inner, status_sink, logger) + expect { wrapper.disable_cache }.not_to raise_error + end + end + end + end + end +end diff --git a/spec/impl/data_store/store_spec.rb b/spec/impl/data_store/store_spec.rb index b3260ecb..c4aa6df2 100644 --- a/spec/impl/data_store/store_spec.rb +++ b/spec/impl/data_store/store_spec.rb @@ -41,7 +41,7 @@ module DataStore class StubPersistentStore include LaunchDarkly::Interfaces::FeatureStore - attr_reader :init_called_count, :upsert_calls, :data, :init_errors + attr_reader :init_called_count, :upsert_calls, :data, :init_errors, :disable_cache_called_count def initialize(should_fail: false) @data = { @@ -53,6 +53,11 @@ def initialize(should_fail: false) @upsert_calls = [] @should_fail = should_fail @init_errors = [] + @disable_cache_called_count = 0 + end + + def disable_cache + @disable_cache_called_count += 1 end def init(all_data) @@ -100,6 +105,39 @@ def reset_tracking @init_called_count = 0 @upsert_calls = [] @init_errors = [] + @disable_cache_called_count = 0 + end + end + + # Stub feature store that does NOT respond to disable_cache (custom user store). + class NonDisablingPersistentStore + include LaunchDarkly::Interfaces::FeatureStore + + attr_reader :init_called_count + + def initialize + @init_called_count = 0 + @data = {} + end + + def init(all_data) + @init_called_count += 1 + @data = all_data + end + + def get(_kind, _key); nil; end + def all(kind); @data[kind] || {}; end + def upsert(_kind, _item); end + def delete(_kind, _key, _version); end + def initialized?; @init_called_count > 0; end + def stop; end + end + + # Stub feature store whose disable_cache raises -- to verify Store keeps operating. + class RaisingPersistentStore < StubPersistentStore + def disable_cache + super + raise RuntimeError, "boom" end end @@ -578,6 +616,82 @@ def reset_tracking end end end + + describe "FDv2 cache lifecycle" do + let(:flag_change) do + LaunchDarkly::Interfaces::DataSystem::Change.new( + action: LaunchDarkly::Interfaces::DataSystem::ChangeType::PUT, + kind: LaunchDarkly::Interfaces::DataSystem::ObjectKind::FLAG, + key: flag_key, + version: 1, + object: flag_data + ) + end + + let(:full_change_set) do + LaunchDarkly::Interfaces::DataSystem::ChangeSet.new( + intent_code: LaunchDarkly::Interfaces::DataSystem::IntentCode::TRANSFER_FULL, + changes: [flag_change], + selector: LaunchDarkly::Interfaces::DataSystem::Selector.no_selector + ) + end + + it "disables the persistent store cache when the memory store receives its first full payload" do + persistent = StubPersistentStore.new + subject.with_persistence(persistent, true, nil) + + subject.apply(full_change_set, true) + + expect(persistent.disable_cache_called_count).to eq(1) + end + + it "disables the cache even when persist is false (read-only persistent store)" do + persistent = StubPersistentStore.new + subject.with_persistence(persistent, false, nil) + + subject.apply(full_change_set, false) + + expect(persistent.disable_cache_called_count).to eq(1) + end + + it "tolerates a persistent store that does not respond to disable_cache" do + persistent = NonDisablingPersistentStore.new + subject.with_persistence(persistent, true, nil) + + expect { subject.apply(full_change_set, true) }.not_to raise_error + expect(persistent.init_called_count).to eq(1) + end + + it "logs a warning if disable_cache raises but continues operating" do + persistent = RaisingPersistentStore.new + subject.with_persistence(persistent, true, nil) + expect(logger).to receive(:warn) + + expect { subject.apply(full_change_set, true) }.not_to raise_error + expect(persistent.init_called_count).to eq(1) + expect(persistent.disable_cache_called_count).to eq(1) + end + + it "keeps Store#commit working after the cache is disabled" do + persistent = StubPersistentStore.new + subject.with_persistence(persistent, true, nil) + subject.apply(full_change_set, true) + + persistent.reset_tracking + expect(subject.commit).to be_nil + expect(persistent.init_called_count).to eq(1) + end + + it "is called on every full transfer (idempotent under repeated set_basis)" do + persistent = StubPersistentStore.new + subject.with_persistence(persistent, true, nil) + + subject.apply(full_change_set, true) + subject.apply(full_change_set, true) + + expect(persistent.disable_cache_called_count).to eq(2) + end + end end end end diff --git a/spec/integrations/util/store_wrapper_spec.rb b/spec/integrations/util/store_wrapper_spec.rb index e20f8b21..a696745e 100644 --- a/spec/integrations/util/store_wrapper_spec.rb +++ b/spec/integrations/util/store_wrapper_spec.rb @@ -247,6 +247,68 @@ module Integrations end end + describe "#disable_cache" do + it "releases the cache so subsequent reads bypass it" do + core = MockCore.new + wrapper = subject.new(core, { expiration: 30 }) + item = { key: "flag", version: 1 } + core.force_set(THINGS, item) + + # prime the cache + expect(wrapper.get(THINGS, "flag")).to eq item + + # change underlying value; cached value should still be returned + itemv2 = { key: "flag", version: 2 } + core.force_set(THINGS, itemv2) + expect(wrapper.get(THINGS, "flag")).to eq item + + wrapper.disable_cache + + # after disable, reads see the underlying value + expect(wrapper.get(THINGS, "flag")).to eq itemv2 + end + + it "is a safe no-op when caching is disabled at config time" do + wrapper = subject.new(MockCore.new, { expiration: 0 }) + expect { wrapper.disable_cache }.not_to raise_error + end + + it "is idempotent" do + wrapper = subject.new(MockCore.new, { expiration: 30 }) + wrapper.disable_cache + expect { wrapper.disable_cache }.not_to raise_error + end + + it "does not repopulate the cache on upsert after disable" do + core = MockCore.new + wrapper = subject.new(core, { expiration: 30 }) + wrapper.init({ THINGS => {} }) + wrapper.disable_cache + + item = { key: "flag", version: 1 } + wrapper.upsert(THINGS, item) + + # mutate underlying value; if a read came from cache it would still see the upserted item + itemv2 = { key: "flag", version: 2 } + core.force_set(THINGS, itemv2) + expect(wrapper.get(THINGS, "flag")).to eq itemv2 + end + + it "does not repopulate the cache on get after disable" do + core = MockCore.new + wrapper = subject.new(core, { expiration: 30 }) + wrapper.disable_cache + + item = { key: "flag", version: 1 } + core.force_set(THINGS, item) + expect(wrapper.get(THINGS, "flag")).to eq item + + itemv2 = { key: "flag", version: 2 } + core.force_set(THINGS, itemv2) + expect(wrapper.get(THINGS, "flag")).to eq itemv2 + end + end + class MockCore def initialize @data = {}