Avoid leaving a push event empty if payload cannot be createdsh-validate-path-project-import

If the payload cannot be created for some reason, we could be left with a nil
push event payload, which causes Error 500s when viewing the dashboard. Guard
against this error and log when it happens.

Avoids problems seen in #38823

3 files changed, 25 insertions, 10 deletions

diff --git a/changelogs/unreleased/sh-validate-path-project-import.yml b/changelogs/unreleased/sh-validate-path-project-import.yml
new file mode 100644
index 00000000000..acad66c0ab2
--- /dev/null
+++ b/changelogs/unreleased/sh-validate-path-project-import.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid leaving a push event empty if payload cannot be created
+merge_request:
+author:
+type: fixed
diff --git a/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb b/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb
index 84ac00f1a5c..7088aa0860a 100644
--- a/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb
+++ b/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb
@@ -128,8 +128,14 @@ module Gitlab
       end
 
       def process_event(event)
-        replicate_event(event)
-        create_push_event_payload(event) if event.push_event?
+        ActiveRecord::Base.transaction do
+          replicate_event(event)
+          create_push_event_payload(event) if event.push_event?
+        end
+      rescue ActiveRecord::InvalidForeignKey => e
+        # A foreign key error means the associated event was removed. In this
+        # case we'll just skip migrating the event.
+        Rails.logger.error("Unable to migrate event #{event.id}: #{e}")
       end
 
       def replicate_event(event)
@@ -137,9 +143,6 @@ module Gitlab
           .with_indifferent_access.except(:title, :data)
 
         EventForMigration.create!(new_attributes)
-      rescue ActiveRecord::InvalidForeignKey
-        # A foreign key error means the associated event was removed. In this
-        # case we'll just skip migrating the event.
       end
 
       def create_push_event_payload(event)
@@ -156,9 +159,6 @@ module Gitlab
           ref: event.trimmed_ref_name,
           commit_title: event.commit_title
         )
-      rescue ActiveRecord::InvalidForeignKey
-        # A foreign key error means the associated event was removed. In this
-        # case we'll just skip migrating the event.
       end
 
       def find_events(start_id, end_id)
diff --git a/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb b/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb
index 7351d45336a..5432d270555 100644
--- a/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb
+++ b/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb
@@ -281,6 +281,17 @@ describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migrati
 
       migration.process_event(event)
     end
+
+    it 'handles an error gracefully' do
+      event1 = create_push_event(project, author, { commits: [] })
+
+      expect(migration).to receive(:replicate_event).and_call_original
+      expect(migration).to receive(:create_push_event_payload).and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key')
+
+      migration.process_event(event1)
+
+      expect(described_class::EventForMigration.all.count).to eq(0)
+    end
   end
 
   describe '#replicate_event' do
@@ -335,9 +346,8 @@ describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migrati
     it 'does not create push event payloads for removed events' do
       allow(event).to receive(:id).and_return(-1)
 
-      payload = migration.create_push_event_payload(event)
+      expect { migration.create_push_event_payload(event) }.to raise_error(ActiveRecord::InvalidForeignKey)
 
-      expect(payload).to be_nil
       expect(PushEventPayload.count).to eq(0)
     end