Add the deduplication hash to the job payload

This adds the job-deduplication hash to the payload of the job.

When the hash is part of the job, Sidekiq-server does not need to
recalculate it for the deleting it from redis. This allows us to
change the way hash is calculated without breaking deduplication on
the server-side.

It also fixes a bug that led us to investigating this, when the
serialisation of the job-arguments is different on the server-side,
the key would not correctly be deleted from Redis. This in turn would
cause the client to not enqueue new jobs for the same arguments for
the TTL of the key, even though they should be.

Changelog: fixed

lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job.rb

@@ -51,6 +51,8 @@ module Gitlab
             end
           end
 
+          job['idempotency_key'] = idempotency_key
+
           self.existing_jid = read_jid.value
         end
 
@@ -117,7 +119,7 @@ module Gitlab
         end
 
         def idempotency_key
-          @idempotency_key ||= "#{namespace}:#{idempotency_hash}"
+          @idempotency_key ||= job['idempotency_key'] || "#{namespace}:#{idempotency_hash}"
         end
 
         def idempotency_hash
@@ -129,6 +131,10 @@ module Gitlab
         end
 
         def idempotency_string
+          # TODO: dump the argument's JSON using `Sidekiq.dump_json` instead
+          # this should be done in the next release so all jobs are written
+          # with their idempotency key.
+          # see https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/1090
           "#{worker_class_name}:#{arguments.join('-')}"
         end
       end

spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb

@@ -51,6 +51,10 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi
                 .from([nil, -2])
                 .to(['123', be_within(1).of(described_class::DUPLICATE_KEY_TTL)])
       end
+
+      it "adds the idempotency key to the jobs payload" do
+        expect { duplicate_job.check! }.to change { job['idempotency_key'] }.from(nil).to(idempotency_key)
+      end
     end
 
     context 'when there was already a job with same arguments in the same queue' do
@@ -81,14 +85,39 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi
 
     context 'when the key exists in redis' do
       before do
-        set_idempotency_key(idempotency_key, 'existing-key')
+        set_idempotency_key(idempotency_key, 'existing-jid')
       end
 
-      it 'removes the key from redis' do
-        expect { duplicate_job.delete! }
-          .to change { read_idempotency_key_with_ttl(idempotency_key) }
-                .from(['existing-key', -1])
-                .to([nil, -2])
+      shared_examples 'deleting the duplicate job' do
+        it 'removes the key from redis' do
+          expect { duplicate_job.delete! }
+            .to change { read_idempotency_key_with_ttl(idempotency_key) }
+                  .from(['existing-jid', -1])
+                  .to([nil, -2])
+        end
+      end
+
+      context 'when the idempotency key is not part of the job' do
+        it_behaves_like 'deleting the duplicate job'
+
+        it 'recalculates the idempotency hash' do
+          expect(duplicate_job).to receive(:idempotency_hash).and_call_original
+
+          duplicate_job.delete!
+        end
+      end
+
+      context 'when the idempotency key is part of the job' do
+        let(:idempotency_key) { 'not the same as what we calculate' }
+        let(:job) { super().merge('idempotency_key' => idempotency_key) }
+
+        it_behaves_like 'deleting the duplicate job'
+
+        it 'does not recalculate the idempotency hash' do
+          expect(duplicate_job).not_to receive(:idempotency_hash)
+
+          duplicate_job.delete!
+        end
       end
     end
   end