Stop using Sidekiq for updating Key#last_used_atremove-use-key-worker

This makes things simpler as no scheduling is involved. Further we
remove the need for running a SELECT + UPDATE just to get the key and
update it, whereas we only need an UPDATE when setting last_used_at
directly in a request.

The added service class takes care of updating Key#last_used_at without
using Sidekiq. Further it makes sure we only try to obtain a Redis lease
if we're confident that we actually need to do so, instead of always
obtaining it. We also make sure to _only_ update last_used_at instead of
also updating updated_at.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/36663

8 files changed, 116 insertions, 65 deletions

diff --git a/app/models/key.rb b/app/models/key.rb
index 4fa6cac2fd0..0c41e34d969 100644
--- a/app/models/key.rb
+++ b/app/models/key.rb
@@ -4,8 +4,6 @@ class Key < ActiveRecord::Base
   include Gitlab::CurrentSettings
   include Sortable
 
-  LAST_USED_AT_REFRESH_TIME = 1.day.to_i
-
   belongs_to :user
 
   before_validation :generate_fingerprint
@@ -54,10 +52,7 @@ class Key < ActiveRecord::Base
   end
 
   def update_last_used_at
-    lease = Gitlab::ExclusiveLease.new("key_update_last_used_at:#{id}", timeout: LAST_USED_AT_REFRESH_TIME)
-    return unless lease.try_obtain
-
-    UseKeyWorker.perform_async(id)
+    Keys::LastUsedService.new(self).execute
   end
 
   def add_to_shell
diff --git a/app/services/keys/last_used_service.rb b/app/services/keys/last_used_service.rb
new file mode 100644
index 00000000000..066f3246158
--- /dev/null
+++ b/app/services/keys/last_used_service.rb
@@ -0,0 +1,33 @@
+module Keys
+  class LastUsedService
+    TIMEOUT = 1.day.to_i
+
+    attr_reader :key
+
+    # key - The Key for which to update the last used timestamp.
+    def initialize(key)
+      @key = key
+    end
+
+    def execute
+      # We _only_ want to update last_used_at and not also updated_at (which
+      # would be updated when using #touch).
+      key.update_column(:last_used_at, Time.zone.now) if update?
+    end
+
+    def update?
+      last_used = key.last_used_at
+
+      return false if last_used && (Time.zone.now - last_used) <= TIMEOUT
+
+      !!redis_lease.try_obtain
+    end
+
+    private
+
+    def redis_lease
+      Gitlab::ExclusiveLease
+        .new("key_update_last_used_at:#{key.id}", timeout: TIMEOUT)
+    end
+  end
+end
diff --git a/app/workers/use_key_worker.rb b/app/workers/use_key_worker.rb
deleted file mode 100644
index c9d382cc5d6..00000000000
--- a/app/workers/use_key_worker.rb
+++ /dev/null
@@ -1,13 +0,0 @@
-class UseKeyWorker
-  include Sidekiq::Worker
-  include DedicatedSidekiqQueue
-
-  def perform(key_id)
-    key = Key.find(key_id)
-    key.touch(:last_used_at)
-  rescue ActiveRecord::RecordNotFound
-    Rails.logger.error("UseKeyWorker: couldn't find key with ID=#{key_id}, skipping job")
-
-    false
-  end
-end
diff --git a/changelogs/unreleased/remove-use-key-worker.yml b/changelogs/unreleased/remove-use-key-worker.yml
new file mode 100644
index 00000000000..a39bcae66bc
--- /dev/null
+++ b/changelogs/unreleased/remove-use-key-worker.yml
@@ -0,0 +1,5 @@
+---
+title: Stop using Sidekiq for updating Key#last_used_at
+merge_request:
+author:
+type: changed
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 24c001362c6..d169c38a693 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -38,7 +38,6 @@
   - [invalid_gpg_signature_update, 2]
   - [create_gpg_signature, 2]
   - [upload_checksum, 1]
-  - [use_key, 1]
   - [repository_fork, 1]
   - [repository_import, 1]
   - [project_service, 1]
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb
index dbc4aba8547..8eabc4ca72f 100644
--- a/spec/models/key_spec.rb
+++ b/spec/models/key_spec.rb
@@ -37,30 +37,17 @@ describe Key, :mailer do
     end
 
     describe "#update_last_used_at" do
-      let(:key) { create(:key) }
-
-      context 'when key was not updated during the last day' do
-        before do
-          allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain)
-            .and_return('000000')
-        end
-
-        it 'enqueues a UseKeyWorker job' do
-          expect(UseKeyWorker).to receive(:perform_async).with(key.id)
-          key.update_last_used_at
-        end
-      end
+      it 'updates the last used timestamp' do
+        key = build(:key)
+        service = double(:service)
+
+        expect(Keys::LastUsedService).to receive(:new)
+          .with(key)
+          .and_return(service)
 
-      context 'when key was updated during the last day' do
-        before do
-          allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain)
-            .and_return(false)
-        end
+        expect(service).to receive(:execute)
 
-        it 'does not enqueue a UseKeyWorker job' do
-          expect(UseKeyWorker).not_to receive(:perform_async)
-          key.update_last_used_at
-        end
+        key.update_last_used_at
       end
     end
   end
diff --git a/spec/services/keys/last_used_service_spec.rb b/spec/services/keys/last_used_service_spec.rb
new file mode 100644
index 00000000000..edaace293ae
--- /dev/null
+++ b/spec/services/keys/last_used_service_spec.rb
@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+describe Keys::LastUsedService do
+  describe '#execute', :clean_gitlab_redis_shared_state do
+    it 'updates the key when it has not been used recently' do
+      key = create(:key, last_used_at: 1.year.ago)
+      time = Time.zone.now
+
+      Timecop.freeze(time) { described_class.new(key).execute }
+
+      expect(key.last_used_at).to eq(time)
+    end
+
+    it 'does not update the key when it has been used recently' do
+      time = 1.minute.ago
+      key = create(:key, last_used_at: time)
+
+      described_class.new(key).execute
+
+      expect(key.last_used_at).to eq(time)
+    end
+
+    it 'does not update the updated_at field' do
+      # Since a lot of these updates could happen in parallel for different keys
+      # we want these updates to be as lightweight as possible, hence we want to
+      # make sure we _only_ update last_used_at and not always updated_at.
+      key = create(:key, last_used_at: 1.year.ago)
+
+      recorder = ActiveRecord::QueryRecorder.new do
+        described_class.new(key).execute
+      end
+
+      expect(recorder.count).to eq(1)
+      expect(recorder.log[0]).not_to include('updated_at')
+    end
+  end
+
+  describe '#update?', :clean_gitlab_redis_shared_state do
+    it 'returns true when no last used timestamp is present' do
+      key = build(:key, last_used_at: nil)
+      service = described_class.new(key)
+
+      expect(service.update?).to eq(true)
+    end
+
+    it 'returns true when the key needs to be updated' do
+      key = build(:key, last_used_at: 1.year.ago)
+      service = described_class.new(key)
+
+      expect(service.update?).to eq(true)
+    end
+
+    it 'returns false when a lease has already been obtained' do
+      key = build(:key, last_used_at: 1.year.ago)
+      service = described_class.new(key)
+
+      expect(service.update?).to eq(true)
+      expect(service.update?).to eq(false)
+    end
+
+    it 'returns false when the key does not yet need to be updated' do
+      key = build(:key, last_used_at: 1.minute.ago)
+      service = described_class.new(key)
+
+      expect(service.update?).to eq(false)
+    end
+  end
+end
diff --git a/spec/workers/use_key_worker_spec.rb b/spec/workers/use_key_worker_spec.rb
deleted file mode 100644
index e50c788b82a..00000000000
--- a/spec/workers/use_key_worker_spec.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-require 'spec_helper'
-
-describe UseKeyWorker do
-  describe "#perform" do
-    it "updates the key's last_used_at attribute to the current time when it exists" do
-      worker = described_class.new
-      key = create(:key)
-      current_time = Time.zone.now
-
-      Timecop.freeze(current_time) do
-        expect { worker.perform(key.id) }
-          .to change { key.reload.last_used_at }.from(nil).to be_like_time(current_time)
-      end
-    end
-
-    it "returns false and skips the job when the key doesn't exist" do
-      worker = described_class.new
-      key = create(:key)
-
-      expect(worker.perform(key.id + 1)).to eq false
-    end
-  end
-end