From 60991b5c54cf4af7a01a6e52571c333e20b6f856 Mon Sep 17 00:00:00 2001 From: joseph-sentry <136376984+joseph-sentry@users.noreply.github.com> Date: Tue, 9 Apr 2024 10:31:35 -0400 Subject: [PATCH] fix: apply notify task after lock is released (#265) * fix: apply notify task after lock is released there is the possibility of an issue when the notify task is queued up before the test result finisher releases the notification lock due to the fact that a notify will drop itself if it fails to lock, so we should make sure that the test result finisher is not holding the notification lock before it queues up the notify task Signed-off-by: joseph-sentry * test: fix test result finisher tests Signed-off-by: joseph-sentry --- tasks/test_results_finisher.py | 33 ++++++++++++++++--- .../tests/unit/test_test_results_finisher.py | 32 ++++++++++++++---- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/tasks/test_results_finisher.py b/tasks/test_results_finisher.py index 13e3bd27f..96b0f7c08 100644 --- a/tasks/test_results_finisher.py +++ b/tasks/test_results_finisher.py @@ -34,6 +34,7 @@ Replacement(["\r"], "", EscapeEnum.REPLACE), Replacement(["\n"], "
", EscapeEnum.REPLACE), ] +QUEUE_NOTIFY_KEY = "queue_notify" class TestResultsFinisherTask(BaseCodecovTask, name=test_results_finisher_task_name): @@ -73,7 +74,7 @@ def run_impl( LockType.NOTIFICATION, retry_num=self.request.retries, ): - return self.process_impl_within_lock( + finisher_result = self.process_impl_within_lock( db_session=db_session, repoid=repoid, commitid=commitid, @@ -81,6 +82,18 @@ def run_impl( previous_result=chord_result, **kwargs, ) + if finisher_result[QUEUE_NOTIFY_KEY]: + self.app.tasks[notify_task_name].apply_async( + args=None, + kwargs=dict( + repoid=repoid, + commitid=commitid, + current_yaml=commit_yaml.to_dict(), + ), + ) + + return finisher_result + except LockRetry as retry: self.retry(max_retries=5, countdown=retry.countdown) @@ -115,7 +128,11 @@ def process_impl_within_lock( "test_results.finisher", tags={"status": "failure", "reason": "no_successful_processing"}, ) - return {"notify_attempted": False, "notify_succeeded": False} + return { + "notify_attempted": False, + "notify_succeeded": False, + QUEUE_NOTIFY_KEY: False, + } commit_report = commit.commit_report(ReportType.TEST_RESULTS) with metrics.timing("test_results.finisher.fetch_latest_test_instances"): @@ -185,7 +202,11 @@ def process_impl_within_lock( repoid=repoid, commitid=commitid, current_yaml=commit_yaml.to_dict() ), ) - return {"notify_attempted": False, "notify_succeeded": False} + return { + "notify_attempted": False, + "notify_succeeded": False, + QUEUE_NOTIFY_KEY: True, + } metrics.incr( "test_results.finisher", @@ -218,7 +239,11 @@ def process_impl_within_lock( "test_results.finisher.test_result_notifier", tags={"status": success, "reason": reason}, ) - return {"notify_attempted": True, "notify_succeeded": success} + return { + "notify_attempted": True, + "notify_succeeded": success, + QUEUE_NOTIFY_KEY: False, + } def check_if_no_success(self, previous_result): return all( diff --git a/tasks/tests/unit/test_test_results_finisher.py b/tasks/tests/unit/test_test_results_finisher.py index 32e1f4e92..2ce4ffd58 100644 --- a/tasks/tests/unit/test_test_results_finisher.py +++ b/tasks/tests/unit/test_test_results_finisher.py @@ -11,7 +11,7 @@ from database.tests.factories import CommitFactory, PullFactory, UploadFactory from services.repository import EnrichedPull from services.test_results import generate_test_id -from tasks.test_results_finisher import TestResultsFinisherTask +from tasks.test_results_finisher import QUEUE_NOTIFY_KEY, TestResultsFinisherTask here = Path(__file__) @@ -220,7 +220,11 @@ def test_upload_finisher_task_call( commit_yaml={"codecov": {"max_report_age": False}}, ) - expected_result = {"notify_attempted": True, "notify_succeeded": True} + expected_result = { + "notify_attempted": True, + "notify_succeeded": True, + QUEUE_NOTIFY_KEY: False, + } assert expected_result == result mock_repo_provider_comments.post_comment.assert_called_with( @@ -278,7 +282,11 @@ def test_upload_finisher_task_call_no_failures( commit_yaml={"codecov": {"max_report_age": False}}, ) - expected_result = {"notify_attempted": False, "notify_succeeded": False} + expected_result = { + "notify_attempted": False, + "notify_succeeded": False, + QUEUE_NOTIFY_KEY: True, + } test_results_mock_app.tasks[ "app.tasks.notify.Notify" ].apply_async.assert_called_with( @@ -336,7 +344,11 @@ def test_upload_finisher_task_call_no_success( commit_yaml={"codecov": {"max_report_age": False}}, ) - expected_result = {"notify_attempted": False, "notify_succeeded": False} + expected_result = { + "notify_attempted": False, + "notify_succeeded": False, + QUEUE_NOTIFY_KEY: False, + } assert expected_result == result @@ -380,7 +392,11 @@ def test_upload_finisher_task_call_existing_comment( commit_yaml={"codecov": {"max_report_age": False}}, ) - expected_result = {"notify_attempted": True, "notify_succeeded": True} + expected_result = { + "notify_attempted": True, + "notify_succeeded": True, + QUEUE_NOTIFY_KEY: False, + } mock_repo_provider_comments.edit_comment.assert_called_with( pull.pullid, @@ -419,7 +435,11 @@ def test_upload_finisher_task_call_comment_fails( commit_yaml={"codecov": {"max_report_age": False}}, ) - expected_result = {"notify_attempted": True, "notify_succeeded": False} + expected_result = { + "notify_attempted": True, + "notify_succeeded": False, + QUEUE_NOTIFY_KEY: False, + } assert expected_result == result