Resructure mixin

Author: johha

app/jobs/mixins/recursive_delete_root_job_mixin.rb

@@ -1,6 +1,7 @@
 require 'errors/sub_resource_error'
 require 'cloud_controller/errors/api_error'
 require 'cloud_controller/errors/compound_error'
+require 'jobs/v3/sub_resource_failures'
 
 module VCAP::CloudController
   module Jobs
@@ -27,14 +28,15 @@ def perform_with_root_job_handling
 
         yield
       rescue SubResourceError => e
+        failures = sub_resource_failures
         if e.any_in_progress?
-          log_immediate_failures(e.failures) # log failures that surfaced this run; async ones still polling defer us and are retried on the next run
+          failures.log_immediate(e.failures) # async still polling: defer; the surfaced sync failures are retried next run
           return
         end
 
-        raise log_recursive_delete_failure(compound_error_for(e.failures)) # sync error occurred & no async job pending -> log and fail
+        raise failures.log_and_return(failures.compound_error(e.failures)) # sync failure, nothing async pending
       rescue CloudController::Errors::CompoundError => e
-        raise log_recursive_delete_failure(e) # async sub job failed -> log and fail
+        raise sub_resource_failures.log_and_return(e) # an async sub-job failed terminally
       rescue CloudController::Errors::ApiError
         raise
       rescue StandardError => e
@@ -96,24 +98,15 @@ def sub_jobs_in_flight?
       def raise_if_sub_jobs_failed
         return unless sub_jobs.any? { |s| s.state == PollableJobModel::FAILED_STATE }
 
-        raise CloudController::Errors::CompoundError.new(all_failure_errors)
+        raise sub_resource_failures.compound_error
       end
 
-      # Logs failures that surfaced during this run (a binding's unbind failed immediately rather than going
-      # async-in-progress). Makes them visible to operators even though the run defers on the async ones,
-      # which are retried on the next run.
-      def log_immediate_failures(failures)
-        failures.each do |error|
-          logger.warn("#{display_name} #{resource_guid} (job #{root_job_guid}) sub-resource deletion failed: #{error.message}")
-        end
-      end
-
-      # Logs each underlying failure and returns the error so callers can `raise log_recursive_delete_failure(error)`.
-      def log_recursive_delete_failure(error)
-        error.underlying_errors.each do |underlying|
-          logger.warn("#{display_name} #{resource_guid} (job #{root_job_guid}) sub-resource deletion failed: #{underlying.message}")
-        end
-        error
+      # Rebuilt per call, never memoised: the job YAML-serialises itself on reschedule.
+      def sub_resource_failures
+        VCAP::CloudController::V3::SubResourceFailures.new(
+          sub_jobs: sub_jobs, sub_resource_errors: sub_resource_errors, logger: logger,
+          display_name: display_name, resource_guid: resource_guid, root_job_guid: root_job_guid
+        )
       end
 
       def add_in_progress_warning(job)
@@ -128,39 +121,10 @@ def in_progress_warning_detail
         'This operation is still in progress: it is waiting for one or more dependent operations to finish.'
       end
 
-      def compound_error_for(raised_failures)
-        errors = all_failure_errors
-        errors = raised_failures.map { |e| CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', e.message) } if errors.empty?
-        CloudController::Errors::CompoundError.new(errors)
-      end
-
-      def all_failure_errors
-        by_guid = {}
-        sub_resource_errors.each { |guid, err| by_guid[guid] = err }
-        sub_job_errors.each { |guid, err| by_guid[guid] ||= err }
-        by_guid.values
-      end
-
-      def sub_job_errors
-        sub_jobs.select { |s| s.state == PollableJobModel::FAILED_STATE }.map do |sub_job|
-          [sub_job.resource_guid, CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', sub_job_error_detail(sub_job))]
-        end
-      end
-
+      # Host hook: [guid, ApiError] pairs for child resources that failed synchronously with no async sub-job.
       def sub_resource_errors
         []
       end
-
-      def sub_job_error_detail(sub_job)
-        identity = "#{sub_job.resource_type} #{sub_job.resource_guid}"
-        return identity if sub_job.cf_api_error.nil?
-
-        parsed = Psych.safe_load(sub_job.cf_api_error, strict_integer: true)
-        detail = parsed && parsed['errors']&.first&.fetch('detail', nil)
-        detail.present? ? "#{identity}: #{detail}" : identity
-      rescue Psych::Exception
-        identity
-      end
     end
   end
 end

app/jobs/v3/sub_resource_failures.rb

@@ -0,0 +1,67 @@
+require 'cloud_controller/errors/api_error'
+require 'cloud_controller/errors/compound_error'
+
+module VCAP::CloudController
+  module V3
+    # Merges failed sub-jobs (async child rows) and failed sub-resources (sync unbinds, no sub-job row) into one error.
+    # Never memoised onto the job: the job YAML-serialises itself on reschedule.
+    class SubResourceFailures
+      # sub_resource_errors: [guid, ApiError] pairs from the host job's sync-failure hook (empty for jobs with none).
+      def initialize(sub_jobs:, sub_resource_errors:, logger:, display_name:, resource_guid:, root_job_guid:)
+        @sub_jobs = sub_jobs
+        @sub_resource_errors = sub_resource_errors
+        @logger = logger
+        @display_name = display_name
+        @resource_guid = resource_guid
+        @root_job_guid = root_job_guid
+      end
+
+      # Falls back to the raised sync failures when nothing durable was recorded.
+      def compound_error(raised_failures=[])
+        errors = merged_errors
+        errors = raised_failures.map { |e| CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', e.message) } if errors.empty?
+        CloudController::Errors::CompoundError.new(errors)
+      end
+
+      # Surfaces failures that failed immediately this run, so they are visible even though we defer on the async ones.
+      def log_immediate(failures)
+        failures.each { |error| log_failure(error.message) }
+      end
+
+      def log_and_return(error)
+        error.underlying_errors.each { |underlying| log_failure(underlying.message) }
+        error
+      end
+
+      private
+
+      def log_failure(message)
+        @logger.warn("#{@display_name} #{@resource_guid} (job #{@root_job_guid}) sub-resource deletion failed: #{message}")
+      end
+
+      def merged_errors
+        by_guid = {}
+        @sub_resource_errors.each { |guid, err| by_guid[guid] = err }
+        sub_job_errors.each { |guid, err| by_guid[guid] ||= err }
+        by_guid.values
+      end
+
+      def sub_job_errors
+        @sub_jobs.select { |s| s.state == PollableJobModel::FAILED_STATE }.map do |sub_job|
+          [sub_job.resource_guid, CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', sub_job_error_detail(sub_job))]
+        end
+      end
+
+      def sub_job_error_detail(sub_job)
+        identity = "#{sub_job.resource_type} #{sub_job.resource_guid}"
+        return identity if sub_job.cf_api_error.nil?
+
+        parsed = Psych.safe_load(sub_job.cf_api_error, strict_integer: true)
+        detail = parsed && parsed['errors']&.first&.fetch('detail', nil)
+        detail.present? ? "#{identity}: #{detail}" : identity
+      rescue Psych::Exception
+        identity
+      end
+    end
+  end
+end

spec/unit/jobs/mixins/recursive_delete_root_job_mixin_spec.rb

@@ -318,95 +318,42 @@ def in_progress_warning_detail
           expect { job.send(:raise_if_sub_jobs_failed) }.not_to raise_error
         end
 
-        context 'when a settled sub-job has failed' do
-          before do
-            make_sub_job(state: PollableJobModel::FAILED_STATE,
-                         resource_type: 'service_credential_binding', resource_guid: 'binding-1',
-                         cf_api_error: YAML.dump({ 'errors' => [{ 'title' => 'CF-UnableToPerform', 'code' => 10_009,
-                                                                  'detail' => 'unbind could not be completed: broker exploded' }] }))
-            make_sub_job(state: PollableJobModel::COMPLETE_STATE)
-          end
-
-          it 'raises a CompoundError of UnprocessableEntity carrying the failed sub-job detail' do
-            job.send(:fetch_root_context)
-            expect { job.send(:raise_if_sub_jobs_failed) }.to raise_error(CloudController::Errors::CompoundError) do |err|
-              expect(err.underlying_errors.map(&:name)).to eq(%w[UnprocessableEntity])
-              expect(err.underlying_errors.first.message).to include('unbind could not be completed: broker exploded')
-            end
-          end
-
-          context 'when the failed sub-job has no stored error detail' do
-            before do
-              make_sub_job(state: PollableJobModel::FAILED_STATE,
-                           resource_type: 'service_credential_binding', resource_guid: 'binding-2')
-            end
-
-            it 'falls back to a resource reference for that entry' do
-              job.send(:fetch_root_context)
-              expect { job.send(:raise_if_sub_jobs_failed) }.to raise_error(CloudController::Errors::CompoundError) do |err|
-                expect(err.underlying_errors.map(&:message)).to include(a_string_including('service_credential_binding binding-2'))
-              end
-            end
-          end
+        # The shape of the raised error (merge, dedup, detail parsing) is covered in sub_resource_failures_spec.
+        it 'raises a CompoundError when a sub-job has settled failed' do
+          make_sub_job(state: PollableJobModel::FAILED_STATE, resource_type: 'service_credential_binding', resource_guid: 'binding-1')
+          job.send(:fetch_root_context)
+          expect { job.send(:raise_if_sub_jobs_failed) }.to raise_error(CloudController::Errors::CompoundError)
         end
       end
 
       describe 'sub_resource_errors (durable sync-failure hook)' do
         let!(:root_pollable_job) { make_root }
 
+        let(:job_with_sync_failure) do
+          klass = Class.new(test_job_class) do
+            def sub_resource_errors
+              [['sync-binding', CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', 'sync unbind failed')]]
+            end
+          end
+          klass.new('resource-guid-1')
+        end
+
         it 'defaults to none so jobs without sub-resources are unaffected' do
           make_sub_job(state: PollableJobModel::COMPLETE_STATE)
           job.send(:fetch_root_context)
           expect { job.send(:raise_if_sub_jobs_failed) }.not_to raise_error
         end
 
-        context 'when a subclass reports a failed sub-resource with no matching sub-job' do
-          let(:job_with_sync_failure) do
-            klass = Class.new(test_job_class) do
-              def sub_resource_errors
-                [['sync-binding', CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', 'sync unbind failed')]]
-              end
-            end
-            klass.new('resource-guid-1')
-          end
-
-          it 'does NOT halt when there is no failed sub-job, so the action can re-run and retry it' do
-            job_with_sync_failure.send(:fetch_root_context)
-            expect { job_with_sync_failure.send(:raise_if_sub_jobs_failed) }.not_to raise_error
-          end
-
-          it 'is reported once a sub-job has terminally failed (action is then skipped), merged with that sub-job' do
-            make_sub_job(state: PollableJobModel::FAILED_STATE,
-                         resource_type: 'service_credential_binding', resource_guid: 'async-binding',
-                         cf_api_error: YAML.dump({ 'errors' => [{ 'detail' => 'async unbind failed' }] }))
-            job_with_sync_failure.send(:fetch_root_context)
-
-            expect { job_with_sync_failure.send(:raise_if_sub_jobs_failed) }.to raise_error(CloudController::Errors::CompoundError) do |err|
-              expect(err.underlying_errors.map(&:message)).to include(a_string_including('sync unbind failed'), a_string_including('async unbind failed'))
-            end
-          end
+        # Self-heal contract: a sync failure alone must not halt, so the action re-runs and retries it.
+        it 'does NOT halt when a sub-resource failed but no sub-job has failed' do
+          job_with_sync_failure.send(:fetch_root_context)
+          expect { job_with_sync_failure.send(:raise_if_sub_jobs_failed) }.not_to raise_error
         end
 
-        context 'when a failed sub-resource shares its guid with a failed sub-job' do
-          let(:job_with_dup) do
-            klass = Class.new(test_job_class) do
-              def sub_resource_errors
-                [['shared-guid', CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', 'unbind failed once')]]
-              end
-            end
-            klass.new('resource-guid-1')
-          end
-
-          it 'reports the resource only once' do
-            make_sub_job(state: PollableJobModel::FAILED_STATE,
-                         resource_type: 'service_credential_binding', resource_guid: 'shared-guid',
-                         cf_api_error: YAML.dump({ 'errors' => [{ 'detail' => 'unbind failed once' }] }))
-            job_with_dup.send(:fetch_root_context)
-
-            expect { job_with_dup.send(:raise_if_sub_jobs_failed) }.to raise_error(CloudController::Errors::CompoundError) do |err|
-              expect(err.underlying_errors.size).to eq(1)
-            end
-          end
+        it 'halts once a sub-job has terminally failed, feeding the hook into the raised error' do
+          make_sub_job(state: PollableJobModel::FAILED_STATE, resource_type: 'service_credential_binding', resource_guid: 'async-binding')
+          job_with_sync_failure.send(:fetch_root_context)
+          expect { job_with_sync_failure.send(:raise_if_sub_jobs_failed) }.to raise_error(CloudController::Errors::CompoundError)
         end
       end
 
@@ -503,32 +450,13 @@ def sub_resource_errors
           expect(logger).not_to have_received(:warn).with(a_string_including('sub-resource deletion failed'))
         end
 
-        it 'translates SubResourceError with real failures to a CompoundError of UnprocessableEntity' do
+        # Routing only; the CompoundError's contents are covered in sub_resource_failures_spec.
+        it 'translates a SubResourceError with real failures to a CompoundError' do
           expect do
             job.send(:perform_with_root_job_handling) do
               raise SubResourceError.new([StandardError.new('one broke'), StandardError.new('two broke')])
             end
-          end.to raise_error(CloudController::Errors::CompoundError) do |err|
-            expect(err.underlying_errors).to all(be_a(CloudController::Errors::ApiError))
-            expect(err.underlying_errors.map(&:name)).to eq(%w[UnprocessableEntity UnprocessableEntity])
-            expect(err.underlying_errors.map(&:message)).to include(match(/one broke/), match(/two broke/))
-          end
-        end
-
-        it 'merges current-tick sync failures with settled failed sub-jobs into one CompoundError' do
-          make_sub_job(state: PollableJobModel::FAILED_STATE,
-                       resource_type: 'service_credential_binding', resource_guid: 'async-binding',
-                       cf_api_error: YAML.dump({ 'errors' => [{ 'title' => 'CF-UnableToPerform', 'code' => 10_009,
-                                                                'detail' => 'async unbind failed' }] }))
-
-          expect do
-            job.send(:perform_with_root_job_handling) do
-              raise SubResourceError.new([StandardError.new('sync unbind failed')])
-            end
-          end.to raise_error(CloudController::Errors::CompoundError) do |err|
-            expect(err.underlying_errors.map(&:name)).to all(eq('UnprocessableEntity'))
-            expect(err.underlying_errors.map(&:message)).to include(match(/sync unbind failed/), match(/async unbind failed/))
-          end
+          end.to raise_error(CloudController::Errors::CompoundError)
         end
 
         it 'passes ApiErrors through unchanged' do