Expire persisted cache names after a feature is enabled/disabledsh-expire-persisted-names-on-feature-change

If we've changed the state of the feature flags, we should just
expire the local cache to ensure the latest changes show up.

This makes it easier to investigate the bug in
https://gitlab.com/gitlab-org/gitlab-ce/issues/64468.

2 files changed, 29 insertions, 3 deletions

diff --git a/lib/feature.rb b/lib/feature.rb
index e28333aa58e..3ac2766cab4 100644
--- a/lib/feature.rb
+++ b/lib/feature.rb
@@ -21,6 +21,8 @@ class Feature
   class << self
     delegate :group, to: :flipper
 
+    PERSISTED_NAMES_CACHE_KEY = 'flipper:persisted_names'
+
     def all
       flipper.features.to_a
     end
@@ -34,12 +36,17 @@ class Feature
         begin
           # We saw on GitLab.com, this database request was called 2300
           # times/s. Let's cache it for a minute to avoid that load.
-          Gitlab::ThreadMemoryCache.cache_backend.fetch('flipper:persisted_names', expires_in: 1.minute) do
+          l1_cache_backend.fetch(PERSISTED_NAMES_CACHE_KEY, expires_in: 1.minute) do
             FlipperFeature.feature_names
           end
         end
     end
 
+    def expire_persisted_names
+      l1_cache_backend.delete(PERSISTED_NAMES_CACHE_KEY)
+      Gitlab::SafeRequestStore[:flipper_persisted_names] = nil
+    end
+
     def persisted?(feature)
       # Flipper creates on-memory features when asked for a not-yet-created one.
       # If we want to check if a feature has been actually set, we look for it
@@ -65,11 +72,15 @@ class Feature
     end
 
     def enable(key, thing = true)
-      get(key).enable(thing)
+      get(key).enable(thing).tap do
+        expire_persisted_names
+      end
     end
 
     def disable(key, thing = false)
-      get(key).disable(thing)
+      get(key).disable(thing).tap do
+        expire_persisted_names
+      end
     end
 
     def enable_group(key, group)
diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb
index 127463a57e8..bf92a03b898 100644
--- a/spec/lib/feature_spec.rb
+++ b/spec/lib/feature_spec.rb
@@ -52,6 +52,17 @@ describe Feature do
     end
   end
 
+  describe '.expire_persisted_names' do
+    it 'expires names after a new name is persisted' do
+      expect(described_class.persisted_names).to be_empty
+      expect(described_class).to receive(:expire_persisted_names).and_call_original
+
+      described_class.enable(:test_key)
+
+      expect(described_class.persisted_names).to eq(['test_key'])
+    end
+  end
+
   describe '.persisted?' do
     context 'when the feature is persisted' do
       it 'returns true when feature name is a string' do
@@ -133,12 +144,16 @@ describe Feature do
     end
 
     it 'returns false for existing disabled feature in the database' do
+      expect(described_class).to receive(:expire_persisted_names).and_call_original
+
       described_class.disable(:disabled_feature_flag)
 
       expect(described_class.enabled?(:disabled_feature_flag)).to be_falsey
     end
 
     it 'returns true for existing enabled feature in the database' do
+      expect(described_class).to receive(:expire_persisted_names).and_call_original
+
       described_class.enable(:enabled_feature_flag)
 
       expect(described_class.enabled?(:enabled_feature_flag)).to be_truthy