Merge branch 'ajk-better-wh-failure-cache' into 'master'

Improve web-hook failure state cache

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111097



Merged-by: Andy Soiron <asoiron@gitlab.com>
Approved-by: Tianwen Chen <tchen@gitlab.com>
Approved-by: Andy Soiron <asoiron@gitlab.com>
Reviewed-by: Andy Soiron <asoiron@gitlab.com>
Reviewed-by: Bojan Marjanovic <bmarjanovic@gitlab.com>
Reviewed-by: Tianwen Chen <tchen@gitlab.com>
Co-authored-by: bmarjanovic <bmarjanovic@gitlab.com>
Co-authored-by: Alex Kalderimis <alex.kalderimis@gmail.com>

app/helpers/web_hooks/web_hooks_helper.rb

@@ -2,8 +2,6 @@
 module WebHooks
   module WebHooksHelper
-    EXPIRY_TTL = 1.hour
-
     def show_project_hook_failed_callout?(project:)
       return false if project_hook_page?
       return false unless current_user
@@ -12,17 +10,11 @@ def show_project_hook_failed_callout?(project:)
       # Assumes include of Users::CalloutsHelper
       return false if web_hook_disabled_dismissed?(project)
-      any_project_hook_failed?(project) # Most expensive query last
+      project.fetch_web_hook_failure
     end
     private
-    def any_project_hook_failed?(project)
-      Rails.cache.fetch("any_web_hook_failed:#{project.id}", expires_in: EXPIRY_TTL) do
-        ProjectHook.for_projects(project).disabled.exists?
-      end
-    end
-
     def project_hook_page?
       current_controller?('projects/hooks') || current_controller?('projects/hook_logs')
     end

app/models/concerns/web_hooks/has_web_hooks.rb

+# frozen_string_literal: true
+
+module WebHooks
+  module HasWebHooks
+    extend ActiveSupport::Concern
+
+    WEB_HOOK_CACHE_EXPIRY = 1.hour
+
+    def any_hook_failed?
+      hooks.disabled.exists?
+    end
+
+    def web_hook_failure_redis_key
+      "any_web_hook_failed:#{id}"
+    end
+
+    def last_failure_redis_key
+      "web_hooks:last_failure:project-#{id}"
+    end
+
+    def get_web_hook_failure
+      Gitlab::Redis::SharedState.with do |redis|
+        current = redis.get(web_hook_failure_redis_key)
+
+        Gitlab::Utils.to_boolean(current) if current
+      end
+    end
+
+    def fetch_web_hook_failure
+      Gitlab::Redis::SharedState.with do |_redis|
+        current = get_web_hook_failure
+        next current unless current.nil?
+
+        cache_web_hook_failure
+      end
+    end
+
+    def cache_web_hook_failure(state = any_hook_failed?)
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.set(web_hook_failure_redis_key, state.to_s, ex: WEB_HOOK_CACHE_EXPIRY)
+
+        state
+      end
+    end
+  end
+end

app/models/hooks/project_hook.rb

@@ -46,14 +46,18 @@ def parent
   override :update_last_failure
   def update_last_failure
-    return if executable?
-    key = "web_hooks:last_failure:project-#{project_id}"
-    time = Time.current.utc.iso8601
-    Gitlab::Redis::SharedState.with do |redis|
-      prev = redis.get(key)
-      redis.set(key, time) if !prev || prev < time
+    if executable?
+      project.cache_web_hook_failure if project.get_web_hook_failure # may need update
+    else
+      project.cache_web_hook_failure(true) # definitely failing, no need to check
+      Gitlab::Redis::SharedState.with do |redis|
+        last_failure_key = project.last_failure_redis_key
+        time = Time.current.utc.iso8601
+        prev = redis.get(last_failure_key)
+        redis.set(last_failure_key, time) if !prev || prev < time
+      end
     end
   end
 end

app/models/project.rb

@@ -41,6 +41,7 @@ class Project < ApplicationRecord
   include BlocksUnsafeSerialization
   include Subquery
   include IssueParent
+  include WebHooks::HasWebHooks
   extend Gitlab::Cache::RequestCache
   extend Gitlab::Utils::Override

spec/helpers/web_hooks/web_hooks_helper_spec.rb

@@ -2,7 +2,7 @@
 require 'spec_helper'
-RSpec.describe WebHooks::WebHooksHelper do
+RSpec.describe WebHooks::WebHooksHelper, :clean_gitlab_redis_shared_state, feature_category: :integrations do
   let_it_be_with_reload(:project) { create(:project) }
   let(:current_user) { nil }
@@ -43,20 +43,12 @@
         expect(helper).to be_show_project_hook_failed_callout(project: project)
       end
-      it 'caches the DB calls until the TTL', :use_clean_rails_memory_store_caching, :request_store do
-        helper.show_project_hook_failed_callout?(project: project)
-
-        travel_to((described_class::EXPIRY_TTL - 1.second).from_now) do
-          expect do
-            helper.show_project_hook_failed_callout?(project: project)
-          end.not_to exceed_query_limit(0)
+      it 'stores a value' do
+        Gitlab::Redis::SharedState.with do |redis|
+          expect(redis).to receive(:set).with(anything, 'true', ex: 1.hour)
         end
-        travel_to((described_class::EXPIRY_TTL + 1.second).from_now) do
-          expect do
-            helper.show_project_hook_failed_callout?(project: project)
-          end.to exceed_query_limit(0)
-        end
+        helper.show_project_hook_failed_callout?(project: project)
       end
     end

spec/models/hooks/project_hook_spec.rb

@@ -79,17 +79,91 @@ def find_hooks
   end
   describe '#update_last_failure', :clean_gitlab_redis_shared_state do
-    let(:hook) { build(:project_hook) }
+    let_it_be(:hook) { create(:project_hook) }
+
+    def last_failure
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.get(hook.project.last_failure_redis_key)
+      end
+    end
+
+    def any_failed?
+      Gitlab::Redis::SharedState.with do |redis|
+        Gitlab::Utils.to_boolean(redis.get(hook.project.web_hook_failure_redis_key))
+      end
+    end
     it 'is a method of this class' do
       expect { hook.update_last_failure }.not_to raise_error
     end
     context 'when the hook is executable' do
-      it 'does not update the state' do
-        expect(Gitlab::Redis::SharedState).not_to receive(:with)
-        hook.update_last_failure
+      let(:redis_key) { hook.project.web_hook_failure_redis_key }
+
+      def redis_value
+        any_failed?
+      end
+
+      context 'when the state was previously failing' do
+        before do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set(redis_key, true)
+          end
+        end
+        it 'does update the state' do
+          expect { hook.update_last_failure }.to change { redis_value }.to(false)
+        end
+
+        context 'when there is another failing sibling hook' do
+          before do
+            create(:project_hook, :permanently_disabled, project: hook.project)
+          end
+
+          it 'does not update the state' do
+            expect { hook.update_last_failure }.not_to change { redis_value }.from(true)
+          end
+
+          it 'caches the current value' do
+            Gitlab::Redis::SharedState.with do |redis|
+              expect(redis).to receive(:set).with(redis_key, 'true', ex: 1.hour).and_call_original
+            end
+
+            hook.update_last_failure
+          end
+        end
+      end
+
+      context 'when the state was previously unknown' do
+        before do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.del(redis_key)
+          end
+        end
+
+        it 'does not update the state' do
+          expect { hook.update_last_failure }.not_to change { redis_value }.from(nil)
+        end
+      end
+
+      context 'when the state was previously not failing' do
+        before do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set(redis_key, false)
+          end
+        end
+
+        it 'does not update the state' do
+          expect { hook.update_last_failure }.not_to change { redis_value }.from(false)
+        end
+
+        it 'does not cache the current value' do
+          Gitlab::Redis::SharedState.with do |redis|
+            expect(redis).not_to receive(:set)
+          end
+
+          hook.update_last_failure
+        end
       end
     end
@@ -98,28 +172,34 @@ def find_hooks
         allow(hook).to receive(:executable?).and_return(false)
       end
-      def last_failure
-        Gitlab::Redis::SharedState.with do |redis|
-          redis.get("web_hooks:last_failure:project-#{hook.project.id}")
-        end
-      end
-
       context 'there is no prior value', :freeze_time do
-        it 'updates the state' do
+        it 'updates last_failure' do
           expect { hook.update_last_failure }.to change { last_failure }.to(Time.current)
         end
+
+        it 'updates any_failed?' do
+          expect { hook.update_last_failure }.to change { any_failed? }.to(true)
+        end
       end
-      context 'there is a prior value, from before now' do
+      context 'when there is a prior last_failure, from before now' do
         it 'updates the state' do
           the_future = 1.minute.from_now
-
           hook.update_last_failure
           travel_to(the_future) do
             expect { hook.update_last_failure }.to change { last_failure }.to(the_future.iso8601)
           end
         end
+
+        it 'does not change the failing state' do
+          the_future = 1.minute.from_now
+          hook.update_last_failure
+
+          travel_to(the_future) do
+            expect { hook.update_last_failure }.not_to change { any_failed? }.from(true)
+          end
+        end
       end
       context 'there is a prior value, from after now' do

spec/models/hooks/web_hook_spec.rb

@@ -443,7 +443,7 @@
   describe '#update_last_failure' do
     it 'is a method of this class' do
-      expect { described_class.new.update_last_failure }.not_to raise_error
+      expect { described_class.new(project: project).update_last_failure }.not_to raise_error
     end
   end

spec/models/project_spec.rb

@@ -8826,6 +8826,14 @@ def has_external_wiki
     end
   end
+  it_behaves_like 'something that has web-hooks' do
+    let_it_be_with_reload(:object) { create(:project) }
+
+    def create_hook
+      create(:project_hook, project: object)
+    end
+  end
+
   private
   def finish_job(export_job)

spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'something that has web-hooks' do
+  describe '#any_hook_failed?', :clean_gitlab_redis_shared_state do
+    subject { object.any_hook_failed? }
+
+    context 'when there are no hooks' do
+      it { is_expected.to eq(false) }
+    end
+
+    context 'when there are hooks' do
+      before do
+        create_hook
+        create_hook
+      end
+
+      it { is_expected.to eq(false) }
+
+      context 'when there is a failed hook' do
+        before do
+          hook = create_hook
+          hook.update!(recent_failures: WebHook::FAILURE_THRESHOLD + 1)
+        end
+
+        it { is_expected.to eq(true) }
+      end
+    end
+  end
+
+  describe '#cache_web_hook_failure', :clean_gitlab_redis_shared_state do
+    context 'when no value is passed' do
+      it 'stores the return value of #any_hook_failed? and passes it back' do
+        allow(object).to receive(:any_hook_failed?).and_return(true)
+
+        Gitlab::Redis::SharedState.with do |r|
+          expect(r).to receive(:set)
+            .with(object.web_hook_failure_redis_key, 'true', ex: 1.hour)
+            .and_call_original
+        end
+
+        expect(object.cache_web_hook_failure).to eq(true)
+      end
+    end
+
+    context 'when a value is passed' do
+      it 'stores the value and passes it back' do
+        expect(object).not_to receive(:any_hook_failed?)
+
+        Gitlab::Redis::SharedState.with do |r|
+          expect(r).to receive(:set)
+            .with(object.web_hook_failure_redis_key, 'foo', ex: 1.hour)
+            .and_call_original
+        end
+
+        expect(object.cache_web_hook_failure(:foo)).to eq(:foo)
+      end
+    end
+  end
+
+  describe '#get_web_hook_failure', :clean_gitlab_redis_shared_state do
+    subject { object.get_web_hook_failure }
+
+    context 'when no value is stored' do
+      it { is_expected.to be_nil }
+    end
+
+    context 'when stored as true' do
+      before do
+        object.cache_web_hook_failure(true)
+      end
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'when stored as false' do
+      before do
+        object.cache_web_hook_failure(false)
+      end
+
+      it { is_expected.to eq(false) }
+    end
+  end
+
+  describe '#fetch_web_hook_failure', :clean_gitlab_redis_shared_state do
+    context 'when a value has not been stored' do
+      it 'does not call #any_hook_failed?' do
+        expect(object.get_web_hook_failure).to be_nil
+        expect(object).to receive(:any_hook_failed?).and_return(true)
+
+        expect(object.fetch_web_hook_failure).to eq(true)
+        expect(object.get_web_hook_failure).to eq(true)
+      end
+    end
+
+    context 'when a value has been stored' do
+      before do
+        object.cache_web_hook_failure(true)
+      end
+
+      it 'does not call #any_hook_failed?' do
+        expect(object).not_to receive(:any_hook_failed?)
+
+        expect(object.fetch_web_hook_failure).to eq(true)
+      end
+    end
+  end
+end