Improve error handling for already deleted resources (#4352)

* Improve error handling for already deleted resources

If a user triggers two DELETE requests for the same resource within a few milliseconds two deletion jobs are created.
If both jobs are be executed in parallel it might happen that one of them fails with a `Sequel::NoExistingObject` error.
This change prevents this by catching those errors but only if the error message originates from the current model/ table.
Therefore it is ensured that similar error messages when e.g. deleting sub resources still result in job failures.

The same applies if a user unbind routes but also deletes the route itself within a very short time period.
For this case the route model now also catches `Sequel::NoExistingObject` errors related to route bindings.

Additionally, the error handling in the DeleteActionJob was improved to catch errors when the actions itself don't handle them.

* Ensure `delete` returns empty error

Otherwise it might return a dataset.

* ensure array is not frozen

Author: johha

app/actions/app_delete.rb

@@ -52,6 +52,8 @@ def delete(apps, record_event: true)
         end
         logger.info("Deleted app: #{app.guid}")
       end
+
+      []
     end
 
     def delete_without_event(apps)

app/actions/organization_delete.rb

@@ -35,6 +35,8 @@ def delete(org_dataset)
           Repositories::OrganizationEventRepository.new.record_organization_delete_request(org, @user_audit_info, { recursive: true })
         end
       end
+
+      []
     end
 
     def timeout_error(dataset)

app/actions/v2/organization_delete.rb

@@ -21,6 +21,7 @@ def delete(org_dataset)
             org.destroy
           end
         end
+        []
       end
 
       def timeout_error(dataset)

app/jobs/delete_action_job.rb

@@ -17,13 +17,23 @@ def perform
         logger.info("Deleting model class '#{model_class}' with guid '#{resource_guid}'")
 
         dataset = model_class.where(guid: resource_guid)
-        if delete_action_can_return_warnings?
-          errors, warnings = delete_action.delete(dataset)
-        else
-          errors = delete_action.delete(dataset)
+        errors = []
+
+        begin
+          if delete_action_can_return_warnings?
+            errors, warnings = delete_action.delete(dataset)
+          else
+            errors = delete_action.delete(dataset)
+          end
+        rescue StandardError => e
+          errors << e
         end
 
-        raise errors.first unless errors&.empty?
+        # Ignore errors if the target resource has already been deleted (e.g., by a parallel job)
+        quoted_table_name = model_class.db.quote_identifier(model_class.table_name)
+        errors.reject! { |err| err.is_a?(Sequel::NoExistingObject) && err.message.include?("DELETE FROM #{quoted_table_name}") } unless errors.frozen?
+
+        raise errors.first unless errors.empty?
 
         warnings
       end

app/models/runtime/route.rb

@@ -263,6 +263,10 @@ def internal_route_vip_range
 
     def destroy_route_bindings
       errors = RouteBindingDelete.new.delete(route_binding_dataset)
+
+      quoted_table_name = RouteBinding.db.quote_identifier(RouteBinding.table_name)
+      errors.reject! { |e| e.is_a?(Sequel::NoExistingObject) && e.message.include?("DELETE FROM #{quoted_table_name}") }
+
       raise errors.first unless errors.empty?
     end
 

spec/unit/jobs/delete_action_job_spec.rb

@@ -2,6 +2,19 @@
 
 module VCAP::CloudController
   module Jobs
+    RSpec.shared_examples 'a delete action handling external deletion' do
+      before do
+        allow_any_instance_of(resource.class).to receive(:destroy).and_wrap_original do |original_method, *args|
+          Sequel::Model.db.run("DELETE FROM #{resource.class.table_name} WHERE id = #{resource.id}") # Simulate external deletion
+          original_method.call(*args)
+        end
+      end
+
+      it 'still attempts to delete the resource even if it was already deleted externally' do
+        expect { delete_job.perform }.not_to raise_error
+      end
+    end
+
     RSpec.describe DeleteActionJob, job_context: :worker do
       let(:user) { User.make(admin: true) }
       let(:delete_action) { instance_double(SpaceDelete, delete: []) }
@@ -127,6 +140,32 @@ module Jobs
           expect(job.resource_guid).to eq(space.guid)
         end
       end
+
+      context 'when the resource is deleted externally before destroy' do
+        it_behaves_like 'a delete action handling external deletion' do
+          let(:resource) { PackageModel.make }
+          let(:delete_action) { PackageDelete.new(nil) }
+          let(:delete_job) { DeleteActionJob.new(PackageModel, resource.guid, delete_action) }
+        end
+
+        it_behaves_like 'a delete action handling external deletion' do
+          let(:resource) { Space.make }
+          let(:delete_action) { SpaceDelete.new(nil, nil) }
+          let(:delete_job) { DeleteActionJob.new(Space, resource.guid, delete_action) }
+        end
+
+        it_behaves_like 'a delete action handling external deletion' do
+          let(:resource) { Route.make }
+          let(:delete_action) { RouteDeleteAction.new(nil) }
+          let(:delete_job) { DeleteActionJob.new(Route, resource.guid, delete_action) }
+        end
+
+        it_behaves_like 'a delete action handling external deletion' do
+          let(:resource) { User.make }
+          let(:delete_action) { UserDeleteAction.new }
+          let(:delete_job) { DeleteActionJob.new(User, resource.guid, delete_action) }
+        end
+      end
     end
   end
 end

spec/unit/models/runtime/route_spec.rb

@@ -1284,6 +1284,23 @@ module VCAP::CloudController
             expect(process.reload.routes[0]).to eq route
           end
         end
+
+        context 'when route_binding is deleted externally before destroy' do
+          before do
+            allow_any_instance_of(ServiceKeyDelete).to receive(:delete_service_binding).and_wrap_original do |original_method, *args|
+              service_binding = args.first
+              service_binding.destroy
+              original_method.call(*args)
+            end
+          end
+
+          it 'does not raise a Sequel::NoExistingObject error' do
+            route_binding = RouteBinding.make
+            route = route_binding.route
+            stub_unbind(route_binding)
+            expect { route.destroy }.not_to raise_error
+          end
+        end
       end
     end