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 <[email protected]>

* test: fix test result finisher tests

Signed-off-by: joseph-sentry <[email protected]>

tasks/test_results_finisher.py

@@ -34,6 +34,7 @@
     Replacement(["\r"], "", EscapeEnum.REPLACE),
     Replacement(["\n"], "<br>", EscapeEnum.REPLACE),
 ]
+QUEUE_NOTIFY_KEY = "queue_notify"
 
 
 class TestResultsFinisherTask(BaseCodecovTask, name=test_results_finisher_task_name):
@@ -73,14 +74,26 @@ 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,
                     commit_yaml=commit_yaml,
                     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(

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