Be safe with regard to nil keys

This ensures that we don't make redis calls to release leases that do
not exist. Cancellation is still safe, and will not raise errors.

lib/gitlab/exclusive_lease.rb

@@ -12,6 +12,9 @@ module Gitlab
   # ExclusiveLease.
   #
   class ExclusiveLease
+    PREFIX = 'gitlab:exclusive_lease'
+    NoKey = Class.new(ArgumentError)
+
     LUA_CANCEL_SCRIPT = <<~EOS.freeze
       local key, uuid = KEYS[1], ARGV[1]
       if redis.call("get", key) == uuid then
@@ -34,14 +37,21 @@ module Gitlab
     end
 
     def self.cancel(key, uuid)
-      key = key&.start_with?('gitlab:exclusive_lease:') ? key : redis_shared_state_key(key)
+      return unless key.present?
+
       Gitlab::Redis::SharedState.with do |redis|
-        redis.eval(LUA_CANCEL_SCRIPT, keys: [key], argv: [uuid])
+        redis.eval(LUA_CANCEL_SCRIPT, keys: [ensure_prefixed_key(key)], argv: [uuid])
       end
     end
 
     def self.redis_shared_state_key(key)
-      "gitlab:exclusive_lease:#{key}"
+      "#{PREFIX}:#{key}"
+    end
+
+    def self.ensure_prefixed_key(key)
+      raise NoKey unless key.present?
+
+      key.start_with?(PREFIX) ? key : redis_shared_state_key(key)
     end
 
     # Removes any existing exclusive_lease from redis

spec/lib/gitlab/exclusive_lease_spec.rb

@@ -21,6 +21,27 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do
     end
   end
 
+  describe '.redis_shared_state_key' do
+    it 'provides a namespaced key' do
+      expect(described_class.redis_shared_state_key(unique_key))
+        .to start_with(described_class::PREFIX)
+        .and include(unique_key)
+    end
+  end
+
+  describe '.ensure_prefixed_key' do
+    it 'does not double prefix a key' do
+      prefixed = described_class.redis_shared_state_key(unique_key)
+
+      expect(described_class.ensure_prefixed_key(unique_key))
+        .to eq(described_class.ensure_prefixed_key(prefixed))
+    end
+
+    it 'raises errors when there is no key' do
+      expect { described_class.ensure_prefixed_key(nil) }.to raise_error(described_class::NoKey)
+    end
+  end
+
   describe '#renew' do
     it 'returns true when we have the existing lease' do
       lease = described_class.new(unique_key, timeout: 3600)
@@ -61,43 +82,61 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do
     end
   end
 
-  describe '.cancel' do
-    it 'can cancel a lease' do
-      uuid = new_lease(unique_key)
-      expect(uuid).to be_present
-      expect(new_lease(unique_key)).to eq(false)
-
-      described_class.cancel(unique_key, uuid)
-      expect(new_lease(unique_key)).to be_present
-    end
-
+  describe 'cancellation' do
     def new_lease(key)
-      described_class.new(key, timeout: 3600).try_obtain
+      described_class.new(key, timeout: 3600)
     end
-  end
 
-  describe '#cancel' do
-    it 'can cancel a lease' do
-      lease = new_lease(unique_key)
-      uuid = lease.try_obtain
-      expect(uuid).to be_present
-      expect(new_lease(unique_key).try_obtain).to eq(false)
+    shared_examples 'cancelling a lease' do
+      let(:lease) { new_lease(unique_key) }
 
-      lease.cancel
+      it 'releases the held lease' do
+        uuid = lease.try_obtain
+        expect(uuid).to be_present
+        expect(new_lease(unique_key).try_obtain).to eq(false)
 
-      expect(new_lease(unique_key).try_obtain).to be_present
+        cancel_lease(uuid)
+
+        expect(new_lease(unique_key).try_obtain).to be_present
+      end
     end
 
-    it 'is safe to call even if the lease was never obtained' do
-      lease = new_lease(unique_key)
+    describe '.cancel' do
+      def cancel_lease(uuid)
+        described_class.cancel(release_key, uuid)
+      end
+
+      context 'when called with the unprefixed key' do
+        it_behaves_like 'cancelling a lease' do
+          let(:release_key) { unique_key }
+        end
+      end
 
-      lease.cancel
+      context 'when called with the prefixed key' do
+        it_behaves_like 'cancelling a lease' do
+          let(:release_key) { described_class.redis_shared_state_key(unique_key) }
+        end
+      end
 
-      expect(new_lease(unique_key).try_obtain).to be_present
+      it 'does not raise errors when given a nil key' do
+        expect { described_class.cancel(nil, nil) }.not_to raise_error
+      end
     end
 
-    def new_lease(key)
-      described_class.new(key, timeout: 3600)
+    describe '#cancel' do
+      def cancel_lease(_uuid)
+        lease.cancel
+      end
+
+      it_behaves_like 'cancelling a lease'
+
+      it 'is safe to call even if the lease was never obtained' do
+        lease = new_lease(unique_key)
+
+        lease.cancel
+
+        expect(new_lease(unique_key).try_obtain).to be_present
+      end
     end
   end