Merge branch 'fix-feature-flag-race-condition' into 'master'

Fix feature flag race condition with stale reads [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57819

config/feature_flags/development/feature_flags_cache_stale_read_fix.yml

+---
+name: feature_flags_cache_stale_read_fix
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57819
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/325452
+milestone: '13.11'
+type: development
+group:
+default_enabled: false

lib/feature.rb

@@ -18,10 +18,6 @@ class Feature
     superclass.table_name = 'feature_gates'
   end
 
-  class ActiveSupportCacheStoreAdapter < Flipper::Adapters::ActiveSupportCacheStore
-    # overrides methods in EE
-  end
-
   InvalidFeatureFlagError = Class.new(Exception) # rubocop:disable Lint/InheritException
 
   class << self

lib/feature/active_support_cache_store_adapter.rb

+# frozen_string_literal: true
+
+# rubocop:disable Gitlab/NamespacedClass
+# This class was already nested this way before moving to a separate file
+class Feature
+  class ActiveSupportCacheStoreAdapter < Flipper::Adapters::ActiveSupportCacheStore
+    def enable(feature, gate, thing)
+      return super unless Feature.enabled?(:feature_flags_cache_stale_read_fix, default_enabled: :yaml)
+
+      result = @adapter.enable(feature, gate, thing)
+      @cache.write(key_for(feature.key), @adapter.get(feature), @write_options)
+      result
+    end
+
+    def disable(feature, gate, thing)
+      return super unless Feature.enabled?(:feature_flags_cache_stale_read_fix, default_enabled: :yaml)
+
+      result = @adapter.disable(feature, gate, thing)
+      @cache.write(key_for(feature.key), @adapter.get(feature), @write_options)
+      result
+    end
+
+    def remove(feature)
+      return super unless Feature.enabled?(:feature_flags_cache_stale_read_fix, default_enabled: :yaml)
+
+      result = @adapter.remove(feature)
+      @cache.delete(FeaturesKey)
+      @cache.write(key_for(feature.key), {}, @write_options)
+      result
+    end
+  end
+end
+# rubocop:disable Gitlab/NamespacedClass

spec/lib/feature_spec.rb

@@ -487,6 +487,109 @@ RSpec.describe Feature, stub_feature_flags: false do
     end
   end
 
+  context 'caching with stale reads from the database', :use_clean_rails_redis_caching, :request_store, :aggregate_failures do
+    let(:actor) { stub_feature_flag_gate('CustomActor:5') }
+    let(:another_actor) { stub_feature_flag_gate('CustomActor:10') }
+
+    # This is a bit unpleasant. For these tests we want to simulate stale reads
+    # from the database (due to database load balancing). A simple way to do
+    # that is to stub the response on the adapter Flipper uses for reading from
+    # the database. However, there isn't a convenient API for this. We know that
+    # the ActiveRecord adapter is always at the 'bottom' of the chain, so we can
+    # find it that way.
+    let(:active_record_adapter) do
+      adapter = described_class.flipper
+
+      loop do
+        break adapter unless adapter.instance_variable_get(:@adapter)
+
+        adapter = adapter.instance_variable_get(:@adapter)
+      end
+    end
+
+    [true, false].each do |enabled|
+      context "with feature_flags_cache_stale_read_fix #{enabled ? 'enabled' : 'disabled'}" do # rubocop:disable RSpec/EmptyExampleGroup
+        # Without this environment variable enabled we want the specs to fail.
+        method = enabled ? :it : :pending
+
+        before do
+          stub_feature_flags(feature_flags_cache_stale_read_fix: enabled)
+        end
+
+        send(method, 'gives the correct value when enabling for an additional actor') do
+          described_class.enable(:enabled_feature_flag, actor)
+          initial_gate_values = active_record_adapter.get(described_class.get(:enabled_feature_flag))
+
+          # This should only be enabled for `actor`
+          expect(described_class.enabled?(:enabled_feature_flag, actor)).to be(true)
+          expect(described_class.enabled?(:enabled_feature_flag, another_actor)).to be(false)
+          expect(described_class.enabled?(:enabled_feature_flag)).to be(false)
+
+          # Enable for `another_actor` and simulate a stale read
+          described_class.enable(:enabled_feature_flag, another_actor)
+          allow(active_record_adapter).to receive(:get).once.and_return(initial_gate_values)
+
+          # Should read from the cache and be enabled for both of these actors
+          expect(described_class.enabled?(:enabled_feature_flag, actor)).to be(true)
+          expect(described_class.enabled?(:enabled_feature_flag, another_actor)).to be(true)
+          expect(described_class.enabled?(:enabled_feature_flag)).to be(false)
+        end
+
+        send(method, 'gives the correct value when enabling for percentage of time') do
+          described_class.enable_percentage_of_time(:enabled_feature_flag, 10)
+          initial_gate_values = active_record_adapter.get(described_class.get(:enabled_feature_flag))
+
+          # Test against `gate_values` directly as otherwise it would be non-determistic
+          expect(described_class.get(:enabled_feature_flag).gate_values.percentage_of_time).to eq(10)
+
+          # Enable 50% of time and simulate a stale read
+          described_class.enable_percentage_of_time(:enabled_feature_flag, 50)
+          allow(active_record_adapter).to receive(:get).once.and_return(initial_gate_values)
+
+          # Should read from the cache and be enabled 50% of the time
+          expect(described_class.get(:enabled_feature_flag).gate_values.percentage_of_time).to eq(50)
+        end
+
+        send(method, 'gives the correct value when disabling the flag') do
+          described_class.enable(:enabled_feature_flag, actor)
+          described_class.enable(:enabled_feature_flag, another_actor)
+          initial_gate_values = active_record_adapter.get(described_class.get(:enabled_feature_flag))
+
+          # This be enabled for `actor` and `another_actor`
+          expect(described_class.enabled?(:enabled_feature_flag, actor)).to be(true)
+          expect(described_class.enabled?(:enabled_feature_flag, another_actor)).to be(true)
+          expect(described_class.enabled?(:enabled_feature_flag)).to be(false)
+
+          # Disable for `another_actor` and simulate a stale read
+          described_class.disable(:enabled_feature_flag, another_actor)
+          allow(active_record_adapter).to receive(:get).once.and_return(initial_gate_values)
+
+          # Should read from the cache and be enabled only for `actor`
+          expect(described_class.enabled?(:enabled_feature_flag, actor)).to be(true)
+          expect(described_class.enabled?(:enabled_feature_flag, another_actor)).to be(false)
+          expect(described_class.enabled?(:enabled_feature_flag)).to be(false)
+        end
+
+        send(method, 'gives the correct value when deleting the flag') do
+          described_class.enable(:enabled_feature_flag, actor)
+          initial_gate_values = active_record_adapter.get(described_class.get(:enabled_feature_flag))
+
+          # This should only be enabled for `actor`
+          expect(described_class.enabled?(:enabled_feature_flag, actor)).to be(true)
+          expect(described_class.enabled?(:enabled_feature_flag)).to be(false)
+
+          # Remove and simulate a stale read
+          described_class.remove(:enabled_feature_flag)
+          allow(active_record_adapter).to receive(:get).once.and_return(initial_gate_values)
+
+          # Should read from the cache and be disabled everywhere
+          expect(described_class.enabled?(:enabled_feature_flag, actor)).to be(false)
+          expect(described_class.enabled?(:enabled_feature_flag)).to be(false)
+        end
+      end
+    end
+  end
+
   describe Feature::Target do
     describe '#targets' do
       let(:project) { create(:project) }