Fix crash when syncing old subjects from patchwork -> Github

Summary:
We saw crashes in KPD like:
```
Traceback (most recent call last):
  File "kernel_patches_daemon/daemon.py", line 58, in run
    await self.run_once()
  File "kernel_patches_daemon/daemon.py", line 49, in run_once
    await self.github_sync_worker.sync_patches()
  File "kernel_patches_daemon/github_sync.py", line 287, in sync_patches
    await worker.checkout_and_patch(branch_name, latest_series)
  File "kernel_patches_daemon/branch_worker.py", line 986, in checkout_and_patch
    return await self.apply_push_comment(branch_name, series_to_apply)
  File "kernel_patches_daemon/branch_worker.py", line 911, in apply_push_comment
    raise NewPRWithNoChangeException(self.repo_pr_base_branch, branch_name)
```

This can happen during patchwork -> Github sync when a patch series is already merged upstream so no changes are necessary vs the target branch and thus no PR is generated. While some code accounts for this case, the "syncing old subjects" part did not and thus the exception would cause recurring crashes.

Reviewed By: chantra

Differential Revision: D63265309

fbshipit-source-id: 98f4fa960d4132a1a222f058c88d5102b6d51d40

Author: ryantimwilson

kernel_patches_daemon/branch_worker.py

@@ -979,6 +979,7 @@ async def checkout_and_patch(
         Patch in place and push.
         Returns true if whole series applied.
         Return None if at least one patch in series failed.
+        Raises NewPRWithNoChangeException if series would not result in any changes.
         If at least one patch in series failed nothing gets pushed.
         """
         if await self._pr_closed(branch_name, series_to_apply):

kernel_patches_daemon/github_sync.py

@@ -168,6 +168,24 @@ def close_existing_prs_with_same_base(
                 if pr_to_close.title in worker.prs:
                     del worker.prs[pr_to_close.title]
 
+    async def checkout_and_patch_safe(
+        self, worker, branch_name: str, series_to_apply: Series
+    ) -> Optional[PullRequest]:
+        try:
+            self.increment_counter("all_known_subjects")
+            pr = await worker.checkout_and_patch(branch_name, series_to_apply)
+            if pr is None:
+                logging.info(
+                    f"PR associated with branch {branch_name} for series {series_to_apply.id} is closed; ignoring"
+                )
+            return pr
+        except NewPRWithNoChangeException as e:
+            self.increment_counter("empty_pr")
+            logger.exception(
+                f"Could not create PR for series {series_to_apply.id} merging {e.base_branch} into {e.target_branch} as PR would introduce no changes"
+            )
+            return None
+
     async def sync_patches(self) -> None:
         """
         One subject = one branch
@@ -239,17 +257,8 @@ async def sync_patches(self) -> None:
                     else:
                         logging.info(msg + "no more next, staying.")
                 logging.info(f"Choosing branch {branch} to create/update PR.")
-                try:
-                    self.increment_counter("all_known_subjects")
-                    pr = await worker.checkout_and_patch(pr_branch_name, series)
-                    if pr is None:
-                        logging.info(
-                            "PR associated with branch {pr_branch_name} for series {series.id} is closed; ignoring"
-                        )
-                        continue
-                except NewPRWithNoChangeException:
-                    self.increment_counter("empty_pr")
-                    logger.exception("Could not create PR with no changes")
+                pr = await self.checkout_and_patch_safe(worker, pr_branch_name, series)
+                if pr is None:
                     continue
 
                 logging.info(
@@ -283,8 +292,11 @@ async def sync_patches(self) -> None:
                         pr.edit(title=subject.subject)
                     branch_name = f"{await subject.branch}{HEAD_BASE_SEPARATOR}{worker.repo_branch}"
                     latest_series = await subject.latest_series or series
-                    self.increment_counter("all_known_subjects")
-                    await worker.checkout_and_patch(branch_name, latest_series)
+                    pr = await self.checkout_and_patch_safe(
+                        worker, branch_name, latest_series
+                    )
+                    if pr is None:
+                        continue
                     await worker.sync_checks(pr, latest_series)
 
             await loop.run_in_executor(None, worker.expire_branches)

tests/test_github_sync.py

@@ -10,8 +10,9 @@
 import unittest
 from dataclasses import dataclass
 from typing import Any, Dict, Optional
-from unittest.mock import MagicMock, patch
+from unittest.mock import AsyncMock, MagicMock, patch
 
+from kernel_patches_daemon.branch_worker import NewPRWithNoChangeException
 from kernel_patches_daemon.config import KPDConfig
 from kernel_patches_daemon.github_sync import GithubSync
 
@@ -54,7 +55,7 @@ def __init__(
         super().__init__(*args, **presets)
 
 
-class TestGihubSync(unittest.TestCase):
+class TestGihubSync(unittest.IsolatedAsyncioTestCase):
     def setUp(self) -> None:
         patcher = patch("kernel_patches_daemon.github_connector.Github")
         self._gh_mock = patcher.start()
@@ -128,3 +129,37 @@ def test_close_existing_prs_with_same_base(self) -> None:
             self.assertTrue("matching" not in worker.prs)
 
         matching_pr_mock.edit.assert_called_with(state="close")
+
+    async def test_checkout_and_patch_safe(self) -> None:
+        pr_branch_name = "fake_pr_branch"
+        series = MagicMock()
+        pr = MagicMock()
+        branch_worker_mock = MagicMock()
+        branch_worker_mock.checkout_and_patch = AsyncMock()
+
+        # PR generated
+        branch_worker_mock.checkout_and_patch.return_value = pr
+        self.assertEqual(
+            await self._gh.checkout_and_patch_safe(
+                branch_worker_mock, pr_branch_name, series
+            ),
+            pr,
+        )
+
+        # One patch in series failed to apply
+        branch_worker_mock.checkout_and_patch.return_value = None
+        self.assertIsNone(
+            await self._gh.checkout_and_patch_safe(
+                branch_worker_mock, pr_branch_name, series
+            )
+        )
+
+        # Series generates no changes vs target branch, likely already merged
+        branch_worker_mock.checkout_and_patch.side_effect = NewPRWithNoChangeException(
+            pr_branch_name, "target"
+        )
+        self.assertIsNone(
+            await self._gh.checkout_and_patch_safe(
+                branch_worker_mock, pr_branch_name, series
+            )
+        )