Merge branch 'remove-feature-flag-fix-flag' into 'master'

Remove feature_flags_cache_stale_read_fix flag [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!59398

changelogs/unreleased/remove-feature-flag-fix-flag.yml

+---
+title: Fix rare race condition in GitLab-internal feature flags with database load
+  balancing enabled
+merge_request:
+author:
+type: fixed

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/active_support_cache_store_adapter.rb

@@ -5,24 +5,18 @@
 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)

spec/lib/feature_spec.rb

@@ -507,86 +507,75 @@ RSpec.describe Feature, stub_feature_flags: false do
       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
+    it '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))
 
-        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)
+      # 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)
+      # 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
+      # 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))
+    it '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)
+      # 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)
+      # 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
+      # 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))
+    it '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)
+      # 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)
+      # 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
+      # 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))
+    it '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)
+      # 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)
+      # 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
+      # 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