From f6f1b37250b00f48e8fccb5db65b039f8a992fa7 Mon Sep 17 00:00:00 2001
From: Martin Edenhofer <me@zammad.com>
Date: Tue, 5 Jan 2021 12:35:56 +0100
Subject: [PATCH] Fixes issue #3336 - High system load, "StatusCode: 500 -
 execution expired", ActiveRecord::Deadlocked issues, and scheduler problems
 prevent Zammad from working well/quickly.

---
 app/models/application_model/can_lookup.rb    | 40 ++++---------------
 app/models/application_model/has_cache.rb     |  3 +-
 .../application_model/can_lookup_examples.rb  |  2 +-
 3 files changed, 11 insertions(+), 34 deletions(-)

diff --git a/app/models/application_model/can_lookup.rb b/app/models/application_model/can_lookup.rb
index b703dc87f..ac16a216b 100644
--- a/app/models/application_model/can_lookup.rb
+++ b/app/models/application_model/can_lookup.rb
@@ -46,41 +46,17 @@ returns
 
     private
 
-    def find_and_save_to_cache_by(args)
-      attribute    = args.keys.first
-      lookup_value = args.values.first.to_s
+    def find_and_save_to_cache_by(attr)
+      record = find_by(attr)
+      return nil if string_key?(attr.keys.first) && (record&.send(attr.keys.first) != attr.values.first.to_s) # enforce case-sensitivity on MySQL
+      return record if ActiveRecord::Base.connection.transaction_open? # rollbacks can invalidate cache entries
 
-      # rollbacks can invalidate cache entry
-      # therefore we don't write it
-      if ActiveRecord::Base.connection.transaction_open?
-        result = find_by(attribute => lookup_value)
-        # enforce case-sensitivity on MySQL
-        result = nil if !key_sensitive_match?(result, attribute, lookup_value)
-      else
-        # get the record via an `FOR UPDATE` DB lock inside of
-        # a transaction to ensure that we don't write obsolete
-        # data into the cache
-        transaction do
-          result = lock.find_by(attribute => lookup_value)
-          # enforce case-sensitivity on MySQL
-          if key_sensitive_match?(result, attribute, lookup_value)
-            # cache only if we got a key-sensitive match
-            cache_set(lookup_value, result)
-          else
-            # no key-sensitive match - no result
-            result = nil
-          end
-        end
-      end
-
-      result
+      cache_set(attr.values.first, record)
+      record
     end
 
-    def key_sensitive_match?(record, attribute, lookup_value)
-      return false if record.blank?
-      return true if type_for_attribute(attribute.to_s).type != :string
-
-      record[attribute] == lookup_value
+    def string_key?(key)
+      type_for_attribute(key.to_s).type == :string
     end
 
   end
diff --git a/app/models/application_model/has_cache.rb b/app/models/application_model/has_cache.rb
index 6d2b444f6..de9ba6ce6 100644
--- a/app/models/application_model/has_cache.rb
+++ b/app/models/application_model/has_cache.rb
@@ -41,7 +41,8 @@ module ApplicationModel::HasCache
 
     def cache_set(data_id, data)
       key = "#{self}::#{data_id}"
-      Cache.write(key, data)
+      # cache for 4 hours max to lower impact of race conditions
+      Cache.write(key, data, expires_in: 4.hours)
     end
 
     def cache_get(data_id)
diff --git a/spec/models/application_model/can_lookup_examples.rb b/spec/models/application_model/can_lookup_examples.rb
index 47544887f..3fb99a17b 100644
--- a/spec/models/application_model/can_lookup_examples.rb
+++ b/spec/models/application_model/can_lookup_examples.rb
@@ -55,7 +55,7 @@ RSpec.shared_examples 'ApplicationModel::CanLookup' do
             it 'saves the value to the cache' do
               expect(Rails.cache)
                 .to receive(:write)
-                .with("#{described_class}::#{instance.send(attribute)}", instance, { expires_in: 7.days })
+                .with("#{described_class}::#{instance.send(attribute)}", instance, { expires_in: 4.hours })
                 .and_call_original
 
               expect { described_class.lookup(attribute => instance.send(attribute)) }