Merge branch '357373-fix-package-files-cleaner' into 'master'

Fix package file cleanup worker with duplicated PyPI files

See merge request gitlab-org/gitlab!84073

app/models/packages/package_file.rb

@@ -34,7 +34,7 @@ class Packages::PackageFile < ApplicationRecord
   validates :file, presence: true
   validates :file_name, presence: true
 
-  validates :file_name, uniqueness: { scope: :package }, if: -> { package&.pypi? }
+  validates :file_name, uniqueness: { scope: :package }, if: -> { !pending_destruction? && package&.pypi? }
 
   scope :recent, -> { order(id: :desc) }
   scope :limit_recent, ->(limit) { recent.limit(limit) }

app/workers/concerns/packages/cleanup_artifact_worker.rb

@@ -14,7 +14,7 @@ module Packages
 
         artifact.destroy!
       rescue StandardError
-        artifact&.error!
+        artifact&.update_column(:status, :error)
       end
 
       after_destroy
@@ -48,7 +48,7 @@ module Packages
           to_delete = next_item
 
           if to_delete
-            to_delete.processing!
+            to_delete.update_column(:status, :processing)
             log_cleanup_item(to_delete)
           end
 

spec/models/packages/package_file_spec.rb

@@ -23,6 +23,28 @@ RSpec.describe Packages::PackageFile, type: :model do
 
   describe 'validations' do
     it { is_expected.to validate_presence_of(:package) }
+
+    context 'with pypi package' do
+      let_it_be(:package) { create(:pypi_package) }
+
+      let(:package_file) { package.package_files.first }
+      let(:status) { :default }
+      let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
+
+      subject { package.package_files.create!(file: file, file_name: package_file.file_name, status: status) }
+
+      it 'can not save a duplicated file' do
+        expect { subject }.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: File name has already been taken")
+      end
+
+      context 'with a pending destruction package duplicated file' do
+        let(:status) { :pending_destruction }
+
+        it 'can save it' do
+          expect { subject }.to change { package.package_files.count }.from(1).to(2)
+        end
+      end
+    end
   end
 
   context 'with package filenames' do

spec/workers/packages/cleanup_package_file_worker_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Packages::CleanupPackageFileWorker do
-  let_it_be(:package) { create(:package) }
+  let_it_be_with_reload(:package) { create(:package) }
 
   let(:worker) { described_class.new }
 
@@ -23,7 +23,23 @@ RSpec.describe Packages::CleanupPackageFileWorker do
         expect(worker).to receive(:log_extra_metadata_on_done).twice
 
         expect { subject }.to change { Packages::PackageFile.count }.by(-1)
-          .and not_change { Packages::Package.count }
+                                .and not_change { Packages::Package.count }
+        expect { package_file2.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      end
+
+      context 'with a duplicated PyPI package file' do
+        let_it_be_with_reload(:duplicated_package_file) { create(:package_file, package: package) }
+
+        before do
+          package.update!(package_type: :pypi, version: '1.2.3')
+          duplicated_package_file.update_column(:file_name, package_file2.file_name)
+        end
+
+        it 'deletes one of the duplicates' do
+          expect { subject }.to change { Packages::PackageFile.count }.by(-1)
+                                  .and not_change { Packages::Package.count }
+          expect { package_file2.reload }.to raise_error(ActiveRecord::RecordNotFound)
+        end
       end
     end