From 4d15f0b1792577a4c8989ce2d4526f23c65d2100 Mon Sep 17 00:00:00 2001
From: Philipp Thun <philipp.thun@sap.com>
Date: Tue, 26 Sep 2023 13:06:34 +0200
Subject: [PATCH] Set operation to 'delete in progress' at beginning of action

---
 .../service_credential_binding_delete.rb      | 12 +++---
 app/actions/service_route_binding_delete.rb   |  9 ++---
 app/actions/v3/service_binding_delete.rb      | 37 +++++++++++++------
 app/actions/v3/service_instance_delete.rb     | 34 ++++++++---------
 .../legacy_jobs/service_instance_deletion.rb  | 31 ----------------
 app/jobs/v3/delete_binding_job.rb             |  8 ++--
 .../v3/delete_service_binding_job_factory.rb  |  4 +-
 app/jobs/v3/delete_service_instance_job.rb    | 10 ++---
 .../delete_service_route_binding_job_actor.rb |  3 --
 lib/cloud_controller/jobs.rb                  |  1 -
 .../service_credential_binding_delete_spec.rb | 20 +++++++---
 .../service_route_binding_delete_spec.rb      | 13 +++++++
 .../v3/service_instance_delete_spec.rb        | 27 ++++++++++++--
 13 files changed, 112 insertions(+), 97 deletions(-)
 delete mode 100644 app/jobs/v2/services/legacy_jobs/service_instance_deletion.rb

diff --git a/app/actions/service_credential_binding_delete.rb b/app/actions/service_credential_binding_delete.rb
index d57bb9222ec..e4ed94e7966 100644
--- a/app/actions/service_credential_binding_delete.rb
+++ b/app/actions/service_credential_binding_delete.rb
@@ -17,19 +17,19 @@ def initialize(type, user_audit_info)
 
       private
 
-      def perform_delete_actions(binding)
-        binding.destroy
+      def event_repository
+        @event_repository ||= Repositories::ServiceGenericBindingEventRepository.new(@event_repository_type)
+      end
 
+      def perform_delete_actions(binding)
         event_repository.record_delete(binding, @user_audit_info)
+
+        binding.destroy
       end
 
       def perform_start_delete_actions(binding)
         event_repository.record_start_delete(binding, @user_audit_info)
       end
-
-      def event_repository
-        @event_repository ||= Repositories::ServiceGenericBindingEventRepository.new(@event_repository_type)
-      end
     end
   end
 end
diff --git a/app/actions/service_route_binding_delete.rb b/app/actions/service_route_binding_delete.rb
index f6cc307c361..6f89dc12bc3 100644
--- a/app/actions/service_route_binding_delete.rb
+++ b/app/actions/service_route_binding_delete.rb
@@ -7,20 +7,17 @@ class ServiceRouteBindingDelete < V3::ServiceBindingDelete
       def initialize(user_audit_info)
         super()
         @user_audit_info = user_audit_info
+        @event_repository_type = Repositories::ServiceGenericBindingEventRepository::SERVICE_ROUTE_BINDING
       end
 
       private
 
       def event_repository
-        @event_repository ||= Repositories::ServiceGenericBindingEventRepository.new(
-          Repositories::ServiceGenericBindingEventRepository::SERVICE_ROUTE_BINDING)
+        @event_repository ||= Repositories::ServiceGenericBindingEventRepository.new(@event_repository_type)
       end
 
       def perform_delete_actions(binding)
-        event_repository.record_delete(
-          binding,
-          @user_audit_info
-        )
+        event_repository.record_delete(binding, @user_audit_info)
 
         binding.destroy
         binding.notify_diego
diff --git a/app/actions/v3/service_binding_delete.rb b/app/actions/v3/service_binding_delete.rb
index 037a22184a3..bda1f569d99 100644
--- a/app/actions/v3/service_binding_delete.rb
+++ b/app/actions/v3/service_binding_delete.rb
@@ -25,18 +25,20 @@ def blocking_operation_in_progress?(binding)
       def delete(binding)
         operation_in_progress! if blocking_operation_in_progress?(binding)
 
+        update_last_operation(binding) unless binding.operation_in_progress?
+
         result = send_unbind_to_client(binding)
         if result[:finished]
           perform_delete_actions(binding)
         else
           perform_start_delete_actions(binding)
-          update_last_operation(binding, operation: result[:operation])
+          update_last_operation_with_details(binding, operation: result[:operation])
         end
 
-        return result
+        result
       rescue => e
         unless e.is_a? ConcurrencyError
-          update_last_operation(binding, state: 'failed', description: e.message)
+          update_last_operation_with_failure(binding, e.message)
         end
 
         raise e
@@ -48,7 +50,7 @@ def poll(binding)
 
         case details[:last_operation][:state]
         when 'in progress'
-          update_last_operation(
+          update_last_operation_with_details(
             binding,
             description: details[:last_operation][:description],
             operation: binding.last_operation.broker_provided_operation)
@@ -57,13 +59,13 @@ def poll(binding)
           perform_delete_actions(binding)
           return PollingFinished
         when 'failed'
-          update_last_operation(binding, state: 'failed', description: details[:last_operation][:description])
+          update_last_operation_with_failure(binding, details[:last_operation][:description])
           raise LastOperationFailedState
         end
       rescue LastOperationFailedState => e
         raise e
       rescue => e
-        update_last_operation(binding, state: 'failed', description: e.message)
+        update_last_operation_with_failure(binding, e.message)
         raise e
       end
 
@@ -86,15 +88,28 @@ def send_unbind_to_client(binding)
         unprocessable!(binding, err)
       end
 
-      def update_last_operation(binding, description: nil, state: 'in progress', operation: nil)
+      def update_last_operation(binding)
+        binding.save_with_attributes_and_new_operation({}, { type: 'delete', state: 'in progress' })
+      end
+
+      def update_last_operation_with_details(binding, description: nil, operation: nil)
+        lo = { type: 'delete', state: 'in progress' }
+        description ||= binding.last_operation&.description
+        operation ||= binding.last_operation&.broker_provided_operation
+        lo.merge!({ description: description }) unless description.nil?
+        lo.merge!({ broker_provided_operation: operation }) unless operation.nil?
+        binding.save_with_attributes_and_new_operation({}, lo)
+      end
+
+      def update_last_operation_with_failure(binding, message)
         binding.save_with_attributes_and_new_operation(
           {},
           {
             type: 'delete',
-            state: state,
-            description: description,
-            broker_provided_operation: operation || binding.last_operation&.broker_provided_operation
-          })
+            state: 'failed',
+            description: message,
+          }
+        )
       end
 
       def unprocessable!(binding, err)
diff --git a/app/actions/v3/service_instance_delete.rb b/app/actions/v3/service_instance_delete.rb
index 3f8c7059fb4..e305146c5d7 100644
--- a/app/actions/v3/service_instance_delete.rb
+++ b/app/actions/v3/service_instance_delete.rb
@@ -37,6 +37,8 @@ def blocking_operation_in_progress?
       def delete
         operation_in_progress! if blocking_operation_in_progress?
 
+        update_last_operation unless service_instance.operation_in_progress?
+
         errors = remove_associations
         raise errors.first if errors.any?
 
@@ -44,13 +46,16 @@ def delete
         if result[:finished]
           perform_delete_actions
         else
-          update_last_operation_with_operation_id(result[:operation])
+          update_last_operation_with_details(operation: result[:operation])
           record_start_delete_event
         end
 
-        return result
+        result
       rescue => e
-        update_last_operation_with_failure(e.message) unless service_instance.operation_in_progress?
+        unless (e.is_a? CloudController::Errors::ApiError) && e.name == 'AsyncServiceInstanceOperationInProgress'
+          update_last_operation_with_failure(e.message)
+        end
+
         raise e
       end
 
@@ -61,7 +66,7 @@ def poll
         )
         case result[:last_operation][:state]
         when 'in progress'
-          update_last_operation_with_description(result[:last_operation][:description])
+          update_last_operation_with_details(description: result[:last_operation][:description])
           ContinuePolling.call(result[:retry_after])
         when 'succeeded'
           perform_delete_actions
@@ -152,21 +157,16 @@ def unshare_all_spaces
         end
       end
 
-      def update_last_operation_with_operation_id(operation_id)
-        service_instance.save_with_new_operation(
-          {},
-          {
-            type: 'delete',
-            state: 'in progress',
-            broker_provided_operation: operation_id
-          }
-        )
+      def update_last_operation
+        service_instance.save_with_new_operation({}, { type: 'delete', state: 'in progress' })
       end
 
-      def update_last_operation_with_description(description)
-        lo = service_instance.last_operation.to_hash
-        lo[:broker_provided_operation] = service_instance.last_operation.broker_provided_operation
-        lo[:description] = description
+      def update_last_operation_with_details(description: nil, operation: nil)
+        lo = { type: 'delete', state: 'in progress' }
+        description ||= service_instance.last_operation&.description
+        operation ||= service_instance.last_operation&.broker_provided_operation
+        lo.merge!({ description: description }) unless description.nil?
+        lo.merge!({ broker_provided_operation: operation }) unless operation.nil?
         service_instance.save_with_new_operation({}, lo)
       end
 
diff --git a/app/jobs/v2/services/legacy_jobs/service_instance_deletion.rb b/app/jobs/v2/services/legacy_jobs/service_instance_deletion.rb
deleted file mode 100644
index 03065abf1fa..00000000000
--- a/app/jobs/v2/services/legacy_jobs/service_instance_deletion.rb
+++ /dev/null
@@ -1,31 +0,0 @@
-module VCAP::CloudController
-  module Jobs
-    module Services
-      ### THIS CLASS IS DEPRECATED
-      #
-      # We have to leave this job definition for backwards compatibility with any old deployments that still
-      # run this job.
-
-      class ServiceInstanceDeletion < VCAP::CloudController::Jobs::CCJob
-        attr_accessor :guid
-
-        def initialize(guid)
-          @guid = guid
-        end
-
-        def perform
-          delegate_job = DeleteActionJob.new(ServiceInstance, @guid, ServiceInstanceDelete.new)
-          delegate_job.perform
-        end
-
-        def job_name_in_configuration
-          :model_deletion
-        end
-
-        def max_attempts
-          1
-        end
-      end
-    end
-  end
-end
diff --git a/app/jobs/v3/delete_binding_job.rb b/app/jobs/v3/delete_binding_job.rb
index 9577ee06abe..5419f37f900 100644
--- a/app/jobs/v3/delete_binding_job.rb
+++ b/app/jobs/v3/delete_binding_job.rb
@@ -14,11 +14,11 @@ def initialize(type, binding_guid, user_audit_info:)
       end
 
       def actor
-        DeleteServiceBindingFactory.for(@type)
+        @actor ||= DeleteServiceBindingFactory.for(@type)
       end
 
       def action
-        DeleteServiceBindingFactory.action(@type, @user_audit_info)
+        @action ||= DeleteServiceBindingFactory.action(@type, @user_audit_info)
       end
 
       def operation
@@ -79,11 +79,11 @@ def handle_timeout
       private
 
       def binding
-        actor.get_resource(resource_guid)
+        @binding ||= actor.get_resource(resource_guid)
       end
 
       def delete_in_progress?
-        binding.last_operation&.type == 'delete' &&
+        binding.reload.last_operation&.type == 'delete' &&
           binding.last_operation&.state == 'in progress'
       end
 
diff --git a/app/jobs/v3/delete_service_binding_job_factory.rb b/app/jobs/v3/delete_service_binding_job_factory.rb
index 34644654f81..d007f09e24a 100644
--- a/app/jobs/v3/delete_service_binding_job_factory.rb
+++ b/app/jobs/v3/delete_service_binding_job_factory.rb
@@ -38,10 +38,10 @@ def self.type_of(binding)
         case binding
         when RouteBinding
           :route
-        when ServiceKey
-          :key
         when ServiceBinding
           :credential
+        when ServiceKey
+          :key
         else
           raise InvalidType
         end
diff --git a/app/jobs/v3/delete_service_instance_job.rb b/app/jobs/v3/delete_service_instance_job.rb
index 1e84e15b4cf..70d0c201463 100644
--- a/app/jobs/v3/delete_service_instance_job.rb
+++ b/app/jobs/v3/delete_service_instance_job.rb
@@ -5,7 +5,7 @@
 module VCAP::CloudController
   module V3
     class DeleteServiceInstanceJob < VCAP::CloudController::Jobs::ReoccurringJob
-      attr_reader :resource_guid
+      attr_reader :resource_guid, :user_audit_info
 
       def initialize(guid, user_audit_info)
         super()
@@ -55,19 +55,17 @@ def display_name
 
       private
 
-      attr_reader :user_audit_info
-
       def service_instance
-        ManagedServiceInstance.first(guid: resource_guid)
+        @service_instance ||= ManagedServiceInstance.first(guid: resource_guid)
       end
 
       def delete_in_progress?
-        service_instance.last_operation&.type == 'delete' &&
+        service_instance.reload.last_operation&.type == 'delete' &&
           service_instance.last_operation&.state == 'in progress'
       end
 
       def action
-        ServiceInstanceDelete.new(service_instance, Repositories::ServiceEventRepository.new(user_audit_info))
+        @action ||= ServiceInstanceDelete.new(service_instance, Repositories::ServiceEventRepository.new(user_audit_info))
       end
     end
   end
diff --git a/app/jobs/v3/delete_service_route_binding_job_actor.rb b/app/jobs/v3/delete_service_route_binding_job_actor.rb
index a71d8955984..35d3ddfc905 100644
--- a/app/jobs/v3/delete_service_route_binding_job_actor.rb
+++ b/app/jobs/v3/delete_service_route_binding_job_actor.rb
@@ -1,6 +1,3 @@
-require 'jobs/reoccurring_job'
-require 'cloud_controller/errors/api_error'
-
 module VCAP::CloudController
   module V3
     class DeleteServiceRouteBindingJobActor
diff --git a/lib/cloud_controller/jobs.rb b/lib/cloud_controller/jobs.rb
index a78d3e1b651..e69447132f6 100644
--- a/lib/cloud_controller/jobs.rb
+++ b/lib/cloud_controller/jobs.rb
@@ -37,7 +37,6 @@
 require 'jobs/runtime/prune_completed_builds'
 require 'jobs/runtime/prune_excess_app_revisions'
 
-require 'jobs/v2/services/legacy_jobs/service_instance_deletion'
 require 'jobs/v2/services/service_usage_events_cleanup'
 
 require 'jobs/v2/upload_droplet_from_user'
diff --git a/spec/unit/actions/service_credential_binding_delete_spec.rb b/spec/unit/actions/service_credential_binding_delete_spec.rb
index 81cad876755..2d5f9e142e2 100644
--- a/spec/unit/actions/service_credential_binding_delete_spec.rb
+++ b/spec/unit/actions/service_credential_binding_delete_spec.rb
@@ -17,11 +17,19 @@ module V3
         allow(binding_event_repo).to receive(:record_start_delete)
       end
 
-      RSpec.shared_examples 'successful credential binding delete' do |klass|
+      RSpec.shared_examples 'successful credential binding delete' do |klass, klass_operation|
         it 'deletes the binding' do
           action.delete(binding)
 
           expect(klass.all).to be_empty
+          expect(klass_operation.all).to be_empty
+        end
+
+        it "sets the operation to 'delete in progress'" do
+          expect_any_instance_of(klass).to receive(:save_with_attributes_and_new_operation).with({}, { type: 'delete', state: 'in progress' }).and_call_original
+          expect_any_instance_of(klass).to receive(:save_with_attributes_and_new_operation).and_call_original
+
+          action.delete(binding)
         end
 
         it 'creates an audit event' do
@@ -40,7 +48,7 @@ module V3
         end
       end
 
-      RSpec.shared_examples 'managed service instance binding delete' do |klass|
+      RSpec.shared_examples 'managed service instance binding delete' do |klass, klass_operation|
         context 'managed service instance' do
           let(:service_instance) { ManagedServiceInstance.make(space: space) }
 
@@ -54,7 +62,7 @@ module V3
               allow(VCAP::Services::ServiceBrokers::V2::Client).to receive(:new).and_return(broker_client)
             end
 
-            it_behaves_like 'successful credential binding delete', klass
+            it_behaves_like 'successful credential binding delete', klass, klass_operation
           end
 
           context 'async unbinding' do
@@ -212,12 +220,12 @@ module V3
         end
 
         describe '#delete' do
-          it_behaves_like 'managed service instance binding delete', ServiceBinding
+          it_behaves_like 'managed service instance binding delete', ServiceBinding, ServiceBindingOperation
 
           context 'user-provided service instance' do
             let(:service_instance) { UserProvidedServiceInstance.make(space: space) }
 
-            it_behaves_like 'successful credential binding delete', ServiceBinding
+            it_behaves_like 'successful credential binding delete', ServiceBinding, ServiceBindingOperation
           end
         end
 
@@ -245,7 +253,7 @@ module V3
         end
 
         describe '#delete' do
-          it_behaves_like 'managed service instance binding delete', ServiceKey
+          it_behaves_like 'managed service instance binding delete', ServiceKey, ServiceKeyOperation
         end
 
         describe '#poll' do
diff --git a/spec/unit/actions/service_route_binding_delete_spec.rb b/spec/unit/actions/service_route_binding_delete_spec.rb
index 89e067df907..c94d77e96d0 100644
--- a/spec/unit/actions/service_route_binding_delete_spec.rb
+++ b/spec/unit/actions/service_route_binding_delete_spec.rb
@@ -10,6 +10,7 @@ module V3
         perform_action
 
         expect(RouteBinding.all).to be_empty
+        expect(RouteBindingOperation.all).to be_empty
       end
 
       it 'creates an audit event' do
@@ -99,6 +100,12 @@ module V3
             end
 
             it_behaves_like 'successful route binding delete'
+
+            it "sets the operation to 'delete in progress'" do
+              expect_any_instance_of(RouteBinding).to receive(:save_with_attributes_and_new_operation).with({}, { type: 'delete', state: 'in progress' }).and_call_original
+
+              perform_action
+            end
           end
 
           context 'async unbinding' do
@@ -126,6 +133,12 @@ module V3
           let(:perform_action) { action.delete(binding) }
 
           it_behaves_like 'successful route binding delete'
+
+          it "sets the operation to 'delete in progress'" do
+            expect_any_instance_of(RouteBinding).to receive(:save_with_attributes_and_new_operation).with({}, { type: 'delete', state: 'in progress' }).and_call_original
+
+            perform_action
+          end
         end
       end
 
diff --git a/spec/unit/actions/v3/service_instance_delete_spec.rb b/spec/unit/actions/v3/service_instance_delete_spec.rb
index db1d83c3500..46faff04f6e 100644
--- a/spec/unit/actions/v3/service_instance_delete_spec.rb
+++ b/spec/unit/actions/v3/service_instance_delete_spec.rb
@@ -127,13 +127,20 @@ module V3
           end
 
           it 'deletes it from the database' do
-            subject.delete
+            action.delete
 
             expect(ServiceInstance.all).to be_empty
+            expect(ServiceInstanceOperation.all).to be_empty
+          end
+
+          it "sets the operation to 'delete in progress'" do
+            expect_any_instance_of(ServiceInstance).to receive(:save_with_new_operation).with({}, { type: 'delete', state: 'in progress' }).and_call_original
+
+            action.delete
           end
 
           it 'creates an audit event' do
-            subject.delete
+            action.delete
 
             expect(event_repository).to have_received(:record_user_provided_service_instance_event).with(
               :delete,
@@ -256,13 +263,20 @@ module V3
 
           context 'when the client succeeds synchronously' do
             it 'deletes it from the database' do
-              subject.delete
+              action.delete
 
               expect(ServiceInstance.all).to be_empty
+              expect(ServiceInstanceOperation.all).to be_empty
+            end
+
+            it "sets the operation to 'delete in progress'" do
+              expect_any_instance_of(ServiceInstance).to receive(:save_with_new_operation).with({}, { type: 'delete', state: 'in progress' }).and_call_original
+
+              action.delete
             end
 
             it 'creates an audit event' do
-              subject.delete
+              action.delete
 
               expect(event_repository).to have_received(:record_service_instance_event).with(
                 :delete,
@@ -289,6 +303,9 @@ module V3
             end
 
             it 'updates the last operation' do
+              expect_any_instance_of(ServiceInstance).to receive(:save_with_new_operation).with({}, { type: 'delete', state: 'in progress' }).and_call_original
+              expect_any_instance_of(ServiceInstance).to receive(:save_with_new_operation).and_call_original
+
               action.delete
 
               expect(ServiceInstance.first.last_operation.type).to eq('delete')
@@ -335,6 +352,7 @@ module V3
                 action.delete
 
                 expect(ServiceInstance.all).to be_empty
+                expect(ServiceInstanceOperation.all).to be_empty
               end
             end
 
@@ -654,6 +672,7 @@ module V3
             action.poll
 
             expect(ServiceInstance.all).to be_empty
+            expect(ServiceInstanceOperation.all).to be_empty
           end
 
           it 'creates an audit event' do