Handle RecordInvalid in concurrent asset upload scenario

app/controllers/api/scratch/assets_controller.rb

@@ -64,7 +64,9 @@ def create_asset(reject_conflicting_content: false, **attributes)
         if scratch_asset.new_record?
           begin
             scratch_asset.save!
-          rescue ActiveRecord::RecordNotUnique
+          rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e
+            raise unless duplicate_filename_error?(e)
+
             logger.info("Scratch asset already created during concurrent upload: #{attributes.fetch(:filename)}")
             scratch_asset = ScratchAsset.find_by!(attributes)
           end
@@ -79,6 +81,12 @@ def create_asset(reject_conflicting_content: false, **attributes)
         render json: { status: 'ok', 'content-name': params[:id] }, status: :created
       end
 
+      def duplicate_filename_error?(error)
+        return true if error.is_a?(ActiveRecord::RecordNotUnique)
+
+        error.record.errors.of_kind?(:filename, :taken)
+      end
+
       def attach_file_with_conflict_check(scratch_asset, filename)
         scratch_asset.with_lock do
           if scratch_asset.file.attached?
@@ -93,7 +101,9 @@ def attach_file_with_conflict_check(scratch_asset, filename)
       end
 
       def attach_file_unless_present(scratch_asset, filename)
-        scratch_asset.file.attach(io: request.body, filename:) unless scratch_asset.file.attached?
+        scratch_asset.with_lock do
+          scratch_asset.file.attach(io: request.body, filename:) unless scratch_asset.file.attached?
+        end
       end
 
       def file_matches?(scratch_asset)

spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb

@@ -352,30 +352,61 @@ def make_request
       end
 
       it 'remains idempotent when another request creates the same project asset first' do
-        existing_asset = create_uploaded_scratch_asset(filename:, project:, body: 'winner-body')
-        racing_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: teacher.id)
-
-        allow(ScratchAsset).to receive(:find_or_initialize_by).and_wrap_original do |original, *args|
-          attributes = args.first
-          if attributes[:filename] == filename &&
-             attributes[:project]&.id == project.id &&
-             attributes[:uploaded_user_id] == teacher.id
-            racing_asset
-          else
-            original.call(*args)
-          end
-        end
-        allow(racing_asset).to receive(:save!).and_raise(ActiveRecord::RecordNotUnique)
+        first_asset = create_uploaded_scratch_asset(filename:, project:, body: 'winner-body')
+        second_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: teacher.id)
+        stub_find_or_initialize_scratch_asset(second_asset, filename:, project:, uploaded_user_id: teacher.id)
+        allow(second_asset).to receive(:save!).and_raise(ActiveRecord::RecordNotUnique)
 
         blob_count = ActiveStorage::Blob.count
 
         expect { make_request }.not_to change(ScratchAsset, :count)
 
         expect(response).to have_http_status(:created)
-        expect(existing_asset.reload.file.download).to eq('winner-body')
+        expect(first_asset.reload.file.download).to eq('winner-body')
+        expect(ActiveStorage::Blob.count).to eq(blob_count)
+      end
+
+      it 'remains idempotent when another request creates the same project asset first - validated after other commit' do
+        first_asset = create_uploaded_scratch_asset(filename:, project:, body: 'winner-body')
+        second_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: teacher.id)
+        stub_find_or_initialize_scratch_asset(second_asset, filename:, project:, uploaded_user_id: teacher.id)
+
+        blob_count = ActiveStorage::Blob.count
+
+        expect { make_request }.not_to change(ScratchAsset, :count)
+
+        expect(response).to have_http_status(:created)
+        expect(first_asset.reload.file.download).to eq('winner-body')
         expect(ActiveStorage::Blob.count).to eq(blob_count)
       end
 
+      it 'does not attach a second file when another request attaches one first' do
+        existing_asset = ScratchAsset.create!(filename:, project:, uploaded_user_id: teacher.id)
+        second_asset = ScratchAsset.find(existing_asset.id)
+        second_asset.file.attached?
+        stub_find_or_initialize_scratch_asset(second_asset, filename:, project:, uploaded_user_id: teacher.id)
+        ScratchAsset.find(existing_asset.id).file.attach(
+          io: StringIO.new('winner-body'), filename:, content_type: 'image/png'
+        )
+
+        expect { make_request }.not_to change(ActiveStorage::Blob, :count)
+
+        expect(response).to have_http_status(:created)
+        expect(ActiveStorage::Attachment.where(record: existing_asset, name: 'file').count).to eq(1)
+        expect(existing_asset.reload.file.download).to eq('winner-body')
+      end
+
+      it 'raises any other errors not related to the race condition' do
+        invalid_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: nil)
+        stub_find_or_initialize_scratch_asset(invalid_asset, filename:, project:, uploaded_user_id: teacher.id)
+        allow(Sentry).to receive(:capture_exception)
+
+        expect { make_request }.not_to change(ScratchAsset, :count)
+
+        expect(response).to have_http_status(:internal_server_error)
+        expect(Sentry).to have_received(:capture_exception).with(an_instance_of(ActiveRecord::RecordInvalid))
+      end
+
       context 'when the current project can be viewed but not updated' do
         let(:student) { create(:student, school:) }
         let(:teacher_project) do
@@ -688,4 +719,17 @@ def create_uploaded_scratch_asset(filename:, project:, body:, uploaded_user_id:
       asset.file.attach(io: StringIO.new(body), filename:, content_type:)
     end
   end
+
+  def stub_find_or_initialize_scratch_asset(replacement, filename:, project:, uploaded_user_id:)
+    allow(ScratchAsset).to receive(:find_or_initialize_by).and_wrap_original do |original, *args|
+      attributes = args.first
+      if attributes[:filename] == filename &&
+         attributes[:project]&.id == project&.id &&
+         attributes[:uploaded_user_id] == uploaded_user_id
+        replacement
+      else
+        original.call(*args)
+      end
+    end
+  end
 end