Merge pull request #3745 from cloudfoundry/update-task-stopped-events

Only emit TASK_STOPPED usage events when necessary

Fixes #3757

Author: tcdowney

app/models/runtime/task_model.rb

@@ -35,7 +35,7 @@ def after_update
       if running_state?
         create_start_event
       elsif terminal_state?
-        create_stop_event
+        create_stop_event_if_needed
       end
     end
 
@@ -104,6 +104,16 @@ def create_start_event
       Repositories::AppUsageEventRepository.new.create_from_task(self, 'TASK_STARTED')
     end
 
+    def create_stop_event_if_needed
+      app_usage_repo = Repositories::AppUsageEventRepository.new
+
+      start_event = app_usage_repo.find_by_task_and_state(task: self, state: 'TASK_STARTED')
+      existing_stop_event = app_usage_repo.find_by_task_and_state(task: self, state: 'TASK_STOPPED')
+      return if start_event.nil? || existing_stop_event.present?
+
+      create_stop_event
+    end
+
     def create_stop_event
       Repositories::AppUsageEventRepository.new.create_from_task(self, 'TASK_STOPPED')
     end

app/repositories/app_usage_event_repository.rb

@@ -8,6 +8,10 @@ def find(guid)
         AppUsageEvent.find(guid:)
       end
 
+      def find_by_task_and_state(task:, state:)
+        AppUsageEvent.find(task_guid: task.guid, state: state)
+      end
+
       def create_from_process(process, state_name=nil)
         AppUsageEvent.create(
           state: state_name || process.state,

spec/unit/lib/cloud_controller/diego/task_completion_handler_spec.rb

@@ -4,7 +4,7 @@
 module VCAP::CloudController
   module Diego
     RSpec.describe TaskCompletionHandler do
-      let!(:task) { TaskModel.make }
+      let!(:task) { TaskModel.make(:pending) }
       let(:handler) { TaskCompletionHandler.new }
       let(:logger) { instance_double(Steno::Logger, info: nil, error: nil, warn: nil) }
 
@@ -13,6 +13,10 @@ module Diego
       end
 
       describe '#complete_task' do
+        before do
+          task.update(state: TaskModel::RUNNING_STATE)
+        end
+
         context 'when the task succeeds' do
           let(:response) do
             {

spec/unit/lib/cloud_controller/diego/tasks_sync_spec.rb

@@ -42,6 +42,8 @@ module Diego
         context 'when a running CC task is missing from BBS' do
           let!(:running_task) { TaskModel.make(:running, created_at: 1.minute.ago) }
           let!(:canceling_task) { TaskModel.make(:canceling, created_at: 1.minute.ago) }
+          let!(:start_event_for_running_task) { AppUsageEvent.make(task_guid: running_task.guid, state: 'TASK_STARTED') }
+          let!(:start_event_for_canceling_task) { AppUsageEvent.make(task_guid: canceling_task.guid, state: 'TASK_STARTED') }
           let(:bbs_tasks) { [] }
 
           it 'marks the tasks as failed' do

spec/unit/models/runtime/task_model_spec.rb

@@ -16,7 +16,10 @@ module VCAP::CloudController
     describe 'after update' do
       let(:task) { TaskModel.make(app: parent_app, state: TaskModel::PENDING_STATE) }
 
-      context 'when the task is moving to SUCCEEDED_STATE' do
+      context 'when the task is moving to the SUCCEEDED_STATE' do
+        let(:task) { TaskModel.make(app: parent_app, state: TaskModel::RUNNING_STATE) }
+        let!(:start_event) { AppUsageEvent.make(task_guid: task.guid, state: 'TASK_STARTED') }
+
         it 'creates a TASK_STOPPED event' do
           task.update(state: TaskModel::SUCCEEDED_STATE)
 
@@ -27,14 +30,73 @@ module VCAP::CloudController
         end
       end
 
-      context 'when the task is moving to FAILED_STATE' do
-        it 'creates a TASK_STOPPED event' do
-          task.update(state: TaskModel::FAILED_STATE)
+      context 'when the task is moving to the FAILED_STATE' do
+        context 'when the task is moving from the RUNNING state' do
+          let!(:start_event) { AppUsageEvent.make(task_guid: task.guid, state: 'TASK_STARTED') }
+          let(:task) { TaskModel.make(app: parent_app, state: TaskModel::RUNNING_STATE) }
 
-          event = AppUsageEvent.find(task_guid: task.guid, state: 'TASK_STOPPED')
-          expect(event).not_to be_nil
-          expect(event.task_guid).to eq(task.guid)
-          expect(event.parent_app_guid).to eq(task.app.guid)
+          it 'creates a TASK_STOPPED event' do
+            task.update(state: TaskModel::FAILED_STATE)
+
+            event = AppUsageEvent.find(task_guid: task.guid, state: 'TASK_STOPPED')
+            expect(event).not_to be_nil
+            expect(event.task_guid).to eq(task.guid)
+            expect(event.parent_app_guid).to eq(task.app.guid)
+          end
+        end
+
+        context 'when the task is moving from the PENDING state' do
+          let(:task) { TaskModel.make(app: parent_app, state: TaskModel::PENDING_STATE) }
+
+          it 'does not create a TASK_STOPPED event' do
+            task.update(state: TaskModel::FAILED_STATE)
+
+            event = AppUsageEvent.find(task_guid: task.guid, state: 'TASK_STOPPED')
+            expect(event).to be_nil
+          end
+        end
+
+        context 'when the task is moving from the CANCELING state' do
+          let(:task) { TaskModel.make(app: parent_app, state: TaskModel::CANCELING_STATE) }
+
+          context 'when the task has a TASK_STARTED event' do
+            let!(:start_event) { AppUsageEvent.make(task_guid: task.guid, state: 'TASK_STARTED') }
+
+            it 'creates a TASK_STOPPED event' do
+              task.update(state: TaskModel::FAILED_STATE)
+
+              event = AppUsageEvent.find(task_guid: task.guid, state: 'TASK_STOPPED')
+              expect(event).not_to be_nil
+              expect(event.task_guid).to eq(task.guid)
+              expect(event.parent_app_guid).to eq(task.app.guid)
+            end
+          end
+
+          context 'when the task does not have a TASK_STARTED event' do
+            let!(:start_event) { nil }
+
+            it 'does not create a TASK_STOPPED event' do
+              task.update(state: TaskModel::FAILED_STATE)
+
+              event = AppUsageEvent.find(task_guid: task.guid, state: 'TASK_STOPPED')
+              expect(event).to be_nil
+            end
+          end
+        end
+
+        context 'when the task already has a TASK_STOPPED event' do
+          let!(:start_event) { AppUsageEvent.make(task_guid: task.guid, state: 'TASK_STARTED') }
+
+          before do
+            AppUsageEvent.make(task_guid: task.guid, state: 'TASK_STOPPED')
+          end
+
+          it 'does not create additional TASK_STOPPED event' do
+            task.update(state: TaskModel::FAILED_STATE)
+
+            events = AppUsageEvent.where(task_guid: task.guid, state: 'TASK_STOPPED')
+            expect(events.count).to equal(1)
+          end
         end
       end
 
@@ -57,6 +119,7 @@ module VCAP::CloudController
       end
 
       context 'when the state is not changing' do
+        let!(:start_event) { AppUsageEvent.make(task_guid: task.guid, state: 'TASK_STARTED') }
         let(:task) { TaskModel.make(app: parent_app, state: TaskModel::FAILED_STATE) }
 
         it 'does not create a TASK_STOPPED event' do