Refs #38856 - simplify auto-publish to always use events

ianballou · ianballou · commit 48386bd6070d · 2025-11-17T19:28:52.000Z
* All composite publishes now go through event system (not just :running case)
* Event handler performs coordination and chaining
* Pass calling_task_id through event metadata for sibling exclusion
diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb
@@ -374,16 +374,12 @@ def auto_publish_composites!(component_task_id)
           when :scheduled
             Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled, skipping duplicate")
             next
-          when :running
-            # A composite publish is currently running - we need to schedule another one
-            # after it finishes to include this new component version
-            Rails.logger.info("Composite CV #{composite_cv.name} publish running, scheduling event for retry")
-            schedule_auto_publish_event(composite_cv, description)
+          when :running, nil
+            # Either composite is running or no composite activity detected
+            # Schedule event to trigger composite publish with proper coordination
+            Rails.logger.info("Composite CV #{composite_cv.name} scheduling auto-publish event")
+            schedule_auto_publish_event(composite_cv, description, component_task_id)
             next
-          when nil
-            # No composite publish running or scheduled - trigger one now
-            sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id)
-            trigger_composite_publish(composite_cv, sibling_task_ids, description)
           end
         end
       end
@@ -411,9 +407,9 @@ def composite_publish_status(composite_cv)
     end
 
     # Schedule an event to retry composite publish after current one finishes
-    def schedule_auto_publish_event(composite_cv, description)
+    def schedule_auto_publish_event(composite_cv, description, component_task_id)
       ::Katello::EventQueue.push_event(::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, composite_cv.id) do |attrs|
-        attrs[:metadata] = { description: description, version_id: self.id }
+        attrs[:metadata] = { description: description, version_id: self.id, calling_task_id: component_task_id }
       end
     end
 
@@ -429,65 +425,55 @@ def scheduled_task_for_composite?(task, composite_cv)
       false
     end
 
-    # Trigger a composite publish, either immediately or chained to sibling tasks
-    def trigger_composite_publish(composite_cv, sibling_task_ids, description)
-      if sibling_task_ids.any?
-        trigger_chained_composite_publish(composite_cv, sibling_task_ids, description)
-      else
-        trigger_immediate_composite_publish(composite_cv, description)
-      end
+    # Trigger a composite publish with coordination for sibling tasks.
+    # Checks for running component CV publishes and chains if necessary.
+    def self.trigger_composite_publish_with_coordination(composite_cv, description, triggered_by_version_id, calling_task_id: nil)
+      # Find currently running component CV publish tasks
+      component_cv_ids = composite_cv.components.pluck(:content_view_id)
+      running_tasks = ForemanTasks::Task::DynflowTask
+        .for_action(::Actions::Katello::ContentView::Publish)
+        .where(state: ['planning', 'planned', 'running'])
+        .select do |task|
+          task_input = task.input
+          task_input && component_cv_ids.include?(task_input.dig('content_view', 'id'))
+        end
+
+      sibling_task_ids = running_tasks.map(&:external_id)
+      # Exclude current task if provided (when called from auto_publish_composites!)
+      sibling_task_ids.reject! { |id| id == calling_task_id } if calling_task_id
+
+      trigger_publish_with_sibling_tasks(composite_cv, sibling_task_ids, description, triggered_by_version_id)
     rescue ForemanTasks::Lock::LockConflict => e
       Rails.logger.info("Composite CV #{composite_cv.name} publish lock conflict: #{e.class} - #{e.message}")
       ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv)
+      raise
     rescue StandardError => e
       Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.class} - #{e.message}")
       Rails.logger.debug(e.backtrace.join("\n")) if e.backtrace
       ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv)
       raise
     end
 
-    # Trigger a chained composite publish that waits for sibling tasks
-    def trigger_chained_composite_publish(composite_cv, sibling_task_ids, description)
-      ForemanTasks.dynflow.world.chain(
-        sibling_task_ids,
-        ::Actions::Katello::ContentView::Publish,
-        composite_cv,
-        description,
-        triggered_by_id: self.id
-      )
-    end
-
-    # Trigger an immediate composite publish
-    def trigger_immediate_composite_publish(composite_cv, description)
-      ForemanTasks.async_task(
-        ::Actions::Katello::ContentView::Publish,
-        composite_cv,
-        description,
-        triggered_by_id: self.id
-      )
+    # Trigger a composite publish, chaining to sibling tasks if any exist
+    def self.trigger_publish_with_sibling_tasks(composite_cv, sibling_task_ids, description, triggered_by_version_id)
+      if sibling_task_ids.any?
+        ForemanTasks.dynflow.world.chain(
+          sibling_task_ids,
+          ::Actions::Katello::ContentView::Publish,
+          composite_cv,
+          description,
+          triggered_by_id: triggered_by_version_id
+        )
+      else
+        ForemanTasks.async_task(
+          ::Actions::Katello::ContentView::Publish,
+          composite_cv,
+          description,
+          triggered_by_id: triggered_by_version_id
+        )
+      end
     end
 
-    # Find sibling component publish tasks that should be waited for
-    def find_sibling_component_publish_tasks(composite_cv, current_task_id)
-      # Get all component CV IDs for this composite
-      component_cv_ids = composite_cv.components.pluck(:content_view_id)
-
-      # Find all currently running publish tasks for these component CVs
-      running_tasks = ForemanTasks::Task::DynflowTask
-        .for_action(::Actions::Katello::ContentView::Publish)
-        .where(state: ['planning', 'planned', 'running'])
-        .select do |task|
-          # Check if task is publishing one of the component CVs
-          task_input = task.input
-          task_input && component_cv_ids.include?(task_input.dig('content_view', 'id'))
-        end
-
-      task_ids = running_tasks.map(&:external_id)
-
-      # Exclude the current task - if we're running this code, the current task
-      # has already reached its Finalize step and doesn't need to be waited for
-      task_ids.reject { |id| id == current_task_id }
-    end
 
     # Find active (planning/planned/running) composite publish tasks (does NOT check scheduled tasks)
     def find_active_composite_publish_tasks(composite_cv)
diff --git a/app/models/katello/events/auto_publish_composite_view.rb b/app/models/katello/events/auto_publish_composite_view.rb
@@ -25,10 +25,14 @@ def run
         return unless composite_view
 
         begin
-          ForemanTasks.async_task(::Actions::Katello::ContentView::Publish,
-                              composite_view,
-                              metadata[:description],
-                              triggered_by: metadata[:version_id])
+          # Use the same coordination logic as auto_publish_composites! to check for
+          # running component tasks and chain if necessary
+          ::Katello::ContentViewVersion.trigger_composite_publish_with_coordination(
+            composite_view,
+            metadata[:description],
+            metadata[:version_id],
+            calling_task_id: metadata[:calling_task_id]
+          )
         rescue => e
           self.retry = true if e.is_a?(ForemanTasks::Lock::LockConflict)
           deliver_failure_notification
diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb
@@ -39,45 +39,37 @@ def setup
                         :latest => true)
     end
 
-    def test_auto_publish_with_no_sibling_tasks_triggers_immediately
+    def test_auto_publish_schedules_event_when_no_composite_activity
       task_id = SecureRandom.uuid
 
-      # Stub to return no scheduled, no running composite, no sibling tasks
+      # Stub to return no scheduled, no running composite
       ForemanTasks::Task::DynflowTask.stubs(:for_action)
         .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks
         .then.returns(stub(where: stub(select: [])))  # Running composite check: none
-        .then.returns(stub(where: stub(select: [])))  # Sibling check: none
 
-      ForemanTasks.expects(:async_task).with(
-        ::Actions::Katello::ContentView::Publish,
-        @composite_cv,
-        anything,
-        triggered_by_id: @component1_version.id
-      ).returns(stub(id: SecureRandom.uuid))
+      ::Katello::EventQueue.expects(:push_event).with(
+        ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE,
+        @composite_cv.id
+      )
 
       @component1_version.auto_publish_composites!(task_id)
     end
 
-    def test_auto_publish_with_sibling_tasks_uses_chaining
-      task_id1 = SecureRandom.uuid
-      task_id2 = SecureRandom.uuid
-
-      sibling_task = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } })
+    def test_auto_publish_schedules_event_when_composite_running
+      task_id = SecureRandom.uuid
+      running_task = stub(input: { 'content_view' => { 'id' => @composite_cv.id } })
 
+      # Stub to return no scheduled but a running composite
       ForemanTasks::Task::DynflowTask.stubs(:for_action)
         .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks
-        .then.returns(stub(where: stub(select: []))) # Running composite check: none
-        .then.returns(stub(where: stub(select: [sibling_task]))) # Sibling check: found sibling
-
-      ForemanTasks.dynflow.world.expects(:chain).with(
-        [task_id2],
-        ::Actions::Katello::ContentView::Publish,
-        @composite_cv,
-        anything,
-        triggered_by_id: @component1_version.id
-      ).returns(stub(id: SecureRandom.uuid))
-
-      @component1_version.auto_publish_composites!(task_id1)
+        .then.returns(stub(where: stub(select: [running_task]))) # Running composite check: found running
+
+      ::Katello::EventQueue.expects(:push_event).with(
+        ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE,
+        @composite_cv.id
+      )
+
+      @component1_version.auto_publish_composites!(task_id)
     end
 
     def test_auto_publish_skips_when_composite_already_scheduled
@@ -129,56 +121,5 @@ def test_auto_publish_schedules_event_when_composite_running
       @component1_version.auto_publish_composites!(task_id)
     end
 
-    def test_auto_publish_handles_lock_conflict_gracefully
-      task_id = SecureRandom.uuid
-
-      ForemanTasks::Task::DynflowTask.stubs(:for_action)
-        .returns(stub(where: stub(any?: false))) # Scheduled check: none
-        .then.returns(stub(where: stub(select: [])))  # Running composite check: none
-        .then.returns(stub(where: stub(select: [])))  # Sibling check: none
-
-      lock = stub('required_lock')
-      conflicting_task = stub(id: 123)
-      conflicting_lock = stub(task: conflicting_task)
-
-      ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(lock, [conflicting_lock]))
-      ::Katello::UINotifications::ContentView::AutoPublishFailure.expects(:deliver!).with(@composite_cv)
-
-      assert_nothing_raised do
-        @component1_version.auto_publish_composites!(task_id)
-      end
-    end
-
-    def test_find_sibling_component_publish_tasks_finds_running_tasks
-      task_id1 = SecureRandom.uuid
-      task_id2 = SecureRandom.uuid
-
-      # Create mock running tasks
-      task1 = stub(external_id: task_id1, input: { 'content_view' => { 'id' => @component_cv1.id } })
-      task2 = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } })
-
-      ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(stub(where: stub(select: [task1, task2])))
-
-      current_task_id = SecureRandom.uuid
-      result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, current_task_id)
-
-      # Should include both sibling tasks but exclude current task
-      assert_equal 2, result.length
-      assert_includes result, task_id1
-      assert_includes result, task_id2
-      assert_not_includes result, current_task_id
-    end
-
-    def test_find_sibling_tasks_excludes_non_component_tasks
-      task_id = SecureRandom.uuid
-
-      # The select block will filter out tasks for CVs that aren't components
-      ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(stub(where: stub(select: [])))
-
-      result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, task_id)
-
-      # Should exclude current task and other CV's task
-      assert_empty result
-    end
   end
 end
diff --git a/test/models/events/auto_publish_composite_view_test.rb b/test/models/events/auto_publish_composite_view_test.rb
@@ -7,13 +7,14 @@ class AutoPublishCompositeViewTest < ActiveSupport::TestCase
       let(:component_version) { katello_content_view_versions(:library_view_version_1) }
 
       def test_run_with_publish
-        metadata = { description: "Auto Publish - Test", version_id: component_version.id }
+        calling_task_id = SecureRandom.uuid
+        metadata = { description: "Auto Publish - Test", version_id: component_version.id, calling_task_id: calling_task_id }
 
-        ForemanTasks.expects(:async_task).with(
-          ::Actions::Katello::ContentView::Publish,
+        ::Katello::ContentViewVersion.expects(:trigger_composite_publish_with_coordination).with(
           composite_view,
           metadata[:description],
-          triggered_by: metadata[:version_id]
+          metadata[:version_id],
+          calling_task_id: calling_task_id
         )
 
         event = AutoPublishCompositeView.new(composite_view.id) do |instance|
@@ -31,9 +32,12 @@ def test_run_with_error
       end
 
       def test_run_with_lock_error
-        metadata = { description: "Auto Publish - Test", version_id: component_version.id }
+        calling_task_id = SecureRandom.uuid
+        metadata = { description: "Auto Publish - Test", version_id: component_version.id, calling_task_id: calling_task_id }
 
-        ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(mock, []))
+        ::Katello::ContentViewVersion.expects(:trigger_composite_publish_with_coordination).raises(
+          ForemanTasks::Lock::LockConflict.new(mock, [])
+        )
 
         instance = AutoPublishCompositeView.new(composite_view.id) do |event|
           event.metadata = metadata
