From 6d141a2618ec561851e40d8d7a52fa633adf12e1 Mon Sep 17 00:00:00 2001
From: Alex Kalderimis <alex.kalderimis@gmail.com>
Date: Fri, 3 Feb 2023 21:06:38 -0500
Subject: [PATCH 1/2] Improve web-hook failure state cache

This makes the following changes:

- uses the Redis shared state rather than the rails cache, to allow
  touching the cache from more places safely
- intelligently sets the cached value of any-hook-failed when a hook
  succeeds or fails. If it succeeds, then if the state was previously
  failing, we query sibling hooks to possibly clear it. If it is now
  failing, we don't have to do that query, and can safely mark it as
  failed.

Why do we do this?

- These changes ensure that if a hook fails, the cache is invalidated
  immediately, and if a hook succeeds, the staet is is cleared so that
  users do not see irrelevant notifications
- Additionally, this reduces the time between a hook failing and any
  notiication being shown to users.
---
 app/helpers/web_hooks/web_hooks_helper.rb     |  10 +-
 .../concerns/web_hooks/has_web_hooks.rb       |  38 +++++++
 app/models/hooks/project_hook.rb              |  20 +++-
 app/models/project.rb                         |   7 ++
 lib/gitlab/database/gitlab_schema.rb          |   2 +
 .../web_hooks/web_hooks_helper_spec.rb        |  18 +--
 spec/models/hooks/project_hook_spec.rb        | 107 ++++++++++++++++--
 spec/models/hooks/web_hook_spec.rb            |   2 +-
 spec/models/project_spec.rb                   |   8 ++
 .../has_web_hooks_shared_examples.rb          | 107 ++++++++++++++++++
 10 files changed, 278 insertions(+), 41 deletions(-)
 create mode 100644 app/models/concerns/web_hooks/has_web_hooks.rb
 create mode 100644 spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb

diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb
index bda9bf58fb7917..f4d713483e62cb 100644
--- a/app/helpers/web_hooks/web_hooks_helper.rb
+++ b/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 # Most expensive query last
     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
diff --git a/app/models/concerns/web_hooks/has_web_hooks.rb b/app/models/concerns/web_hooks/has_web_hooks.rb
new file mode 100644
index 00000000000000..7e054bfd2ef395
--- /dev/null
+++ b/app/models/concerns/web_hooks/has_web_hooks.rb
@@ -0,0 +1,38 @@
+# 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 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
diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb
index 81122c3ea10a44..f021182790c920 100644
--- a/app/models/hooks/project_hook.rb
+++ b/app/models/hooks/project_hook.rb
@@ -46,16 +46,24 @@ def parent
 
   override :update_last_failure
   def update_last_failure
-    return if executable?
+    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
 
-    key = "web_hooks:last_failure:project-#{project_id}"
-    time = Time.current.utc.iso8601
+      Gitlab::Redis::SharedState.with do |redis|
+        last_failure_key = "web_hooks:last_failure:project-#{project_id}"
+        time = Time.current.utc.iso8601
+        prev = redis.get(last_failure_key)
 
-    Gitlab::Redis::SharedState.with do |redis|
-      prev = redis.get(key)
-      redis.set(key, time) if !prev || prev < time
+        redis.set(last_failure_key, time) if !prev || prev < time
+      end
     end
   end
+
+  def any_failed_redis_key
+    project.web_hook_failure_redis_key
+  end
 end
 
 ProjectHook.prepend_mod_with('ProjectHook')
diff --git a/app/models/project.rb b/app/models/project.rb
index 27be84d787ee68..77ed076f0b5cc1 100644
--- a/app/models/project.rb
+++ b/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
@@ -1735,6 +1736,12 @@ def execute_hooks(data, hooks_scope = :push_hooks)
   end
   # rubocop: enable CodeReuse/ServiceClass
 
+  def web_hook_failure_redis_key
+    "any_web_hook_failed:#{id}"
+  end
+
+  # end HasWebHooks: These methods will all be moved to HasWebHooks
+
   def triggered_hooks(hooks_scope, data)
     triggered = ::Projects::TriggeredHooks.new(hooks_scope, data)
     triggered.add_hooks(hooks)
diff --git a/lib/gitlab/database/gitlab_schema.rb b/lib/gitlab/database/gitlab_schema.rb
index 38558512b6a110..1904035f1d366c 100644
--- a/lib/gitlab/database/gitlab_schema.rb
+++ b/lib/gitlab/database/gitlab_schema.rb
@@ -121,6 +121,8 @@ def self.schema_names
 
           key_name = data['table_name'] || data['view_name']
 
+          raise "No gitlab_schema for #{file_path}" unless data['gitlab_schema']
+
           dic[key_name] = data['gitlab_schema'].to_sym
         end
       end
diff --git a/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/spec/helpers/web_hooks/web_hooks_helper_spec.rb
index bcd9d2df1dc266..7930ee18fb6bf4 100644
--- a/spec/helpers/web_hooks/web_hooks_helper_spec.rb
+++ b/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 'caches the DB calls until the TTL' 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
 
diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb
index 9a78d7b1719135..57410643e1342c 100644
--- a/spec/models/hooks/project_hook_spec.rb
+++ b/spec/models/hooks/project_hook_spec.rb
@@ -79,17 +79,81 @@ 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("web_hooks:last_failure:project-#{hook.project.id}")
+      end
+    end
+
+    def any_failed?
+      Gitlab::Redis::SharedState.with do |redis|
+        Gitlab::Utils.to_boolean(redis.get("any_web_hook_failed:#{hook.project.id}"))
+      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)
+      let(:redis_key) { "any_web_hook_failed:#{hook.project.id}" }
+
+      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
+        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 }
+        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
 
-        hook.update_last_failure
+        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
+          expect(hook.project).not_to receive(:hooks)
+
+          hook.update_last_failure
+        end
       end
     end
 
@@ -98,19 +162,17 @@ 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
 
@@ -122,6 +184,27 @@ def last_failure
         end
       end
 
+      context 'when the project was already in a failing state' do
+        it 'does not change it' 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 'when the project was previously not in a failing state' do
+        it 'changes it' do
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set("any_web_hook_failed:#{hook.project.id}", false)
+          end
+
+          expect { hook.update_last_failure }.to change { any_failed? }.to(true)
+        end
+      end
+
       context 'there is a prior value, from after now' do
         it 'does not update the state' do
           the_past = 1.minute.ago
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index 4bfa33280378dd..72958a54e108ef 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/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
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index c0af71a7e2705b..23dae7d0374534 100644
--- a/spec/models/project_spec.rb
+++ b/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)
diff --git a/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb b/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb
new file mode 100644
index 00000000000000..cd6eb8c77faf1c
--- /dev/null
+++ b/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb
@@ -0,0 +1,107 @@
+# 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
-- 
GitLab


From b98e6087cb1d81495600195a7415061739a25a4b Mon Sep 17 00:00:00 2001
From: bmarjanovic <bmarjanovic@gitlab.com>
Date: Mon, 13 Feb 2023 10:30:41 +0100
Subject: [PATCH 2/2] Apply reviewer's feedback

---
 app/helpers/web_hooks/web_hooks_helper.rb     |  2 +-
 .../concerns/web_hooks/has_web_hooks.rb       | 10 +++++-
 app/models/hooks/project_hook.rb              |  6 +---
 app/models/project.rb                         |  6 ----
 lib/gitlab/database/gitlab_schema.rb          |  2 --
 .../web_hooks/web_hooks_helper_spec.rb        |  2 +-
 spec/models/hooks/project_hook_spec.rb        | 35 +++++++++----------
 7 files changed, 28 insertions(+), 35 deletions(-)

diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb
index f4d713483e62cb..514db6ba8a2ff2 100644
--- a/app/helpers/web_hooks/web_hooks_helper.rb
+++ b/app/helpers/web_hooks/web_hooks_helper.rb
@@ -10,7 +10,7 @@ def show_project_hook_failed_callout?(project:)
       # Assumes include of Users::CalloutsHelper
       return false if web_hook_disabled_dismissed?(project)
 
-      project.fetch_web_hook_failure # Most expensive query last
+      project.fetch_web_hook_failure
     end
 
     private
diff --git a/app/models/concerns/web_hooks/has_web_hooks.rb b/app/models/concerns/web_hooks/has_web_hooks.rb
index 7e054bfd2ef395..161ce106b9b7ce 100644
--- a/app/models/concerns/web_hooks/has_web_hooks.rb
+++ b/app/models/concerns/web_hooks/has_web_hooks.rb
@@ -10,6 +10,14 @@ 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)
@@ -19,7 +27,7 @@ def get_web_hook_failure
     end
 
     def fetch_web_hook_failure
-      Gitlab::Redis::SharedState.with do |redis|
+      Gitlab::Redis::SharedState.with do |_redis|
         current = get_web_hook_failure
         next current unless current.nil?
 
diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb
index f021182790c920..8e9a74a68d031b 100644
--- a/app/models/hooks/project_hook.rb
+++ b/app/models/hooks/project_hook.rb
@@ -52,7 +52,7 @@ def update_last_failure
       project.cache_web_hook_failure(true) # definitely failing, no need to check
 
       Gitlab::Redis::SharedState.with do |redis|
-        last_failure_key = "web_hooks:last_failure:project-#{project_id}"
+        last_failure_key = project.last_failure_redis_key
         time = Time.current.utc.iso8601
         prev = redis.get(last_failure_key)
 
@@ -60,10 +60,6 @@ def update_last_failure
       end
     end
   end
-
-  def any_failed_redis_key
-    project.web_hook_failure_redis_key
-  end
 end
 
 ProjectHook.prepend_mod_with('ProjectHook')
diff --git a/app/models/project.rb b/app/models/project.rb
index 77ed076f0b5cc1..63ea19202ceaee 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1736,12 +1736,6 @@ def execute_hooks(data, hooks_scope = :push_hooks)
   end
   # rubocop: enable CodeReuse/ServiceClass
 
-  def web_hook_failure_redis_key
-    "any_web_hook_failed:#{id}"
-  end
-
-  # end HasWebHooks: These methods will all be moved to HasWebHooks
-
   def triggered_hooks(hooks_scope, data)
     triggered = ::Projects::TriggeredHooks.new(hooks_scope, data)
     triggered.add_hooks(hooks)
diff --git a/lib/gitlab/database/gitlab_schema.rb b/lib/gitlab/database/gitlab_schema.rb
index 1904035f1d366c..38558512b6a110 100644
--- a/lib/gitlab/database/gitlab_schema.rb
+++ b/lib/gitlab/database/gitlab_schema.rb
@@ -121,8 +121,6 @@ def self.schema_names
 
           key_name = data['table_name'] || data['view_name']
 
-          raise "No gitlab_schema for #{file_path}" unless data['gitlab_schema']
-
           dic[key_name] = data['gitlab_schema'].to_sym
         end
       end
diff --git a/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/spec/helpers/web_hooks/web_hooks_helper_spec.rb
index 7930ee18fb6bf4..fdd0be8095b2e8 100644
--- a/spec/helpers/web_hooks/web_hooks_helper_spec.rb
+++ b/spec/helpers/web_hooks/web_hooks_helper_spec.rb
@@ -43,7 +43,7 @@
         expect(helper).to be_show_project_hook_failed_callout(project: project)
       end
 
-      it 'caches the DB calls until the TTL' do
+      it 'stores a value' do
         Gitlab::Redis::SharedState.with do |redis|
           expect(redis).to receive(:set).with(anything, 'true', ex: 1.hour)
         end
diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb
index 57410643e1342c..c3484c4a42c97c 100644
--- a/spec/models/hooks/project_hook_spec.rb
+++ b/spec/models/hooks/project_hook_spec.rb
@@ -83,13 +83,13 @@ def find_hooks
 
     def last_failure
       Gitlab::Redis::SharedState.with do |redis|
-        redis.get("web_hooks:last_failure:project-#{hook.project.id}")
+        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("any_web_hook_failed:#{hook.project.id}"))
+        Gitlab::Utils.to_boolean(redis.get(hook.project.web_hook_failure_redis_key))
       end
     end
 
@@ -98,7 +98,7 @@ def any_failed?
     end
 
     context 'when the hook is executable' do
-      let(:redis_key) { "any_web_hook_failed:#{hook.project.id}" }
+      let(:redis_key) { hook.project.web_hook_failure_redis_key }
 
       def redis_value
         any_failed?
@@ -123,6 +123,14 @@ def redis_value
           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
 
@@ -134,7 +142,7 @@ def redis_value
         end
 
         it 'does not update the state' do
-          expect { hook.update_last_failure }.not_to change { redis_value }
+          expect { hook.update_last_failure }.not_to change { redis_value }.from(nil)
         end
       end
 
@@ -150,7 +158,9 @@ def redis_value
         end
 
         it 'does not cache the current value' do
-          expect(hook.project).not_to receive(:hooks)
+          Gitlab::Redis::SharedState.with do |redis|
+            expect(redis).not_to receive(:set)
+          end
 
           hook.update_last_failure
         end
@@ -175,17 +185,14 @@ def redis_value
       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
-      end
 
-      context 'when the project was already in a failing state' do
-        it 'does not change it' do
+        it 'does not change the failing state' do
           the_future = 1.minute.from_now
           hook.update_last_failure
 
@@ -195,16 +202,6 @@ def redis_value
         end
       end
 
-      context 'when the project was previously not in a failing state' do
-        it 'changes it' do
-          Gitlab::Redis::SharedState.with do |redis|
-            redis.set("any_web_hook_failed:#{hook.project.id}", false)
-          end
-
-          expect { hook.update_last_failure }.to change { any_failed? }.to(true)
-        end
-      end
-
       context 'there is a prior value, from after now' do
         it 'does not update the state' do
           the_past = 1.minute.ago
-- 
GitLab