Avoid race condition when creating sidetag in Koji

packit_service/models.py

@@ -13,6 +13,7 @@
 from os import getenv
 from typing import (
     Dict,
+    Generator,
     Iterable,
     List,
     Optional,
@@ -3773,8 +3774,19 @@ class SidetagModel(Base):
     sidetag_group = relationship("SidetagGroupModel", back_populates="sidetags")
 
     @classmethod
-    def get_or_create(cls, group: "SidetagGroupModel", target: str) -> "SidetagModel":
-        with sa_session_transaction(commit=True) as session:
+    @contextmanager
+    def get_or_create_for_updating(
+        cls, group: "SidetagGroupModel", target: str
+    ) -> Generator["SidetagModel", None, None]:
+        """
+        Context manager that gets or creates a sidetag upon entering the context,
+        provides a corresponding instance of `SidetagModel` to be updated within the context
+        and commits the changes upon exiting the context, all within a single transaction.
+        """
+        session = singleton_session or Session()
+        session.begin()
+
+        try:
             sidetag = (
                 session.query(cls)
                 .filter_by(sidetag_group_id=group.id, target=target)
@@ -3787,22 +3799,30 @@ def get_or_create(cls, group: "SidetagGroupModel", target: str) -> "SidetagModel
 
                 group.sidetags.append(sidetag)
                 session.add(group)
-            return sidetag
+        except Exception as ex:
+            logger.warning(f"Exception while working with database: {ex!r}")
+            session.rollback()
+            raise
+
+        try:
+            yield sidetag
+        except Exception:
+            session.rollback()
+            raise
+
+        try:
+            session.add(sidetag)
+            session.commit()
+        except Exception as ex:
+            logger.warning(f"Exception while working with database: {ex!r}")
+            session.rollback()
+            raise
 
     @classmethod
     def get_by_koji_name(cls, koji_name: str) -> Optional["SidetagModel"]:
         with sa_session_transaction() as session:
             return session.query(SidetagModel).filter_by(koji_name=koji_name).first()
 
-    def set_koji_name(self, koji_name: str) -> None:
-        with sa_session_transaction(commit=True) as session:
-            self.koji_name = koji_name
-            session.add(self)
-
-    def delete(self) -> None:
-        with sa_session_transaction(commit=True) as session:
-            session.delete(self)
-
 
 @cached(cache=TTLCache(maxsize=2048, ttl=(60 * 60 * 24)))
 def get_usage_data(datetime_from=None, datetime_to=None, top=10) -> dict:

packit_service/worker/handlers/distgit.py

@@ -7,9 +7,11 @@
 import abc
 import logging
 import shutil
+from collections import defaultdict
 from datetime import datetime
+from functools import partial
 from os import getenv
-from typing import Optional, Tuple, Type, List, ClassVar
+from typing import Dict, Optional, Set, Tuple, Type, List, ClassVar
 
 from celery import Task
 
@@ -1045,19 +1047,22 @@ class TagIntoSidetagHandler(
     def get_checkers() -> Tuple[Type[Checker], ...]:
         return (PermissionOnDistgit,)
 
-    def run_for_branch(self, job_config: JobConfig, branch: str) -> None:
+    def run_for_branch(self, package: str, sidetag_group: str, branch: str) -> None:
         # we need Kerberos ticket to tag a build into sidetag
         # and to create a new sidetag (if needed)
         self.packit_api.init_kerberos_ticket()
-        sidetag = SidetagHelper.get_or_create_sidetag(job_config.sidetag_group, branch)
-        sidetag.tag_latest_stable_build(job_config.downstream_package_name)
+        sidetag = SidetagHelper.get_or_create_sidetag(sidetag_group, branch)
+        sidetag.tag_latest_stable_build(package)
 
     def run(self) -> TaskResults:
         comment = self.data.event_dict.get("comment")
         commands = get_packit_commands_from_comment(
             comment, self.service_config.comment_command_prefix
         )
         args = commands[1:] if len(commands) > 1 else ""
+        packages_to_tag: Dict[str, Dict[str, Set[str]]] = defaultdict(
+            partial(defaultdict, set)
+        )
         for job in self.package_config.get_job_views():
             if (
                 job.type == JobType.koji_build
@@ -1073,10 +1078,15 @@ def run(self) -> TaskResults:
                     continue
                 else:
                     branches = {self.pull_request.target_branch}
+                packages_to_tag[job.downstream_package_name][
+                    job.sidetag_group
+                ] |= branches
+        for package, sidetag_groups in packages_to_tag.items():
+            for sidetag_group, branches in sidetag_groups.items():
                 logger.debug(
-                    "Running downstream Koji build tagging "
-                    f"of {job.downstream_package_name} for {branches}"
+                    f"Running downstream Koji build tagging of {package} "
+                    f"for {branches} in {sidetag_group}"
                 )
                 for branch in branches:
-                    self.run_for_branch(job, branch)
+                    self.run_for_branch(package, sidetag_group, branch)
         return TaskResults(success=True, details={})

packit_service/worker/helpers/sidetag.py

@@ -2,7 +2,7 @@
 # SPDX-License-Identifier: MIT
 
 import logging
-from typing import Optional, Set
+from typing import Any, Optional, Set
 
 from packit.exceptions import PackitException
 from packit.utils.koji_helper import KojiHelper
@@ -70,7 +70,7 @@ def tag_latest_stable_build(self, package: str) -> None:
 
 
 class SidetagHelperMeta(type):
-    def __init__(cls, *args, **kwargs):
+    def __init__(cls, *args: Any, **kwargs: Any) -> None:
         cls._koji_helper: Optional[KojiHelper] = None
 
     @property
@@ -147,10 +147,14 @@ def get_or_create_sidetag(cls, sidetag_group: str, dist_git_branch: str) -> Side
             PackitException if the sidetag failed to be created in Koji.
         """
         group = SidetagGroupModel.get_or_create(sidetag_group)
-        sidetag = SidetagModel.get_or_create(group, dist_git_branch)
-        if not sidetag.koji_name or not cls.koji_helper.get_tag_info(sidetag.koji_name):
-            tag_info = cls.koji_helper.create_sidetag(dist_git_branch)
-            if not tag_info:
-                raise PackitException(f"Failed to create sidetag for {dist_git_branch}")
-            sidetag.set_koji_name(tag_info["name"])
+        with SidetagModel.get_or_create_for_updating(group, dist_git_branch) as sidetag:
+            if not sidetag.koji_name or not cls.koji_helper.get_tag_info(
+                sidetag.koji_name
+            ):
+                tag_info = cls.koji_helper.create_sidetag(dist_git_branch)
+                if not tag_info:
+                    raise PackitException(
+                        f"Failed to create sidetag for {dist_git_branch}"
+                    )
+                sidetag.koji_name = tag_info["name"]
         return Sidetag(sidetag, cls.koji_helper)

tests/integration/test_dg_commit.py

@@ -264,13 +264,9 @@ def test_downstream_koji_build(sidetag_group):
         koji_target = "f40-build-side-12345"
         sidetag = flexmock(koji_name=None)
 
-        def set_koji_name(name):
-            sidetag.koji_name = name
-
-        sidetag.should_receive("set_koji_name").with_args(koji_target).replace_with(
-            set_koji_name
+        flexmock(SidetagModel).should_receive("get_or_create_for_updating").and_return(
+            sidetag
         )
-        flexmock(SidetagModel).should_receive("get_or_create").and_return(sidetag)
         flexmock(SidetagGroupModel).should_receive("get_or_create").and_return(
             flexmock()
         )

tests/integration/test_pr_comment.py

@@ -3050,10 +3050,10 @@ def test_koji_build_tag_via_dist_git_pr_comment(pagure_pr_comment_added, all_bra
     flexmock(SidetagGroupModel).should_receive("get_or_create").with_args(
         "test"
     ).and_return(sidetag_group)
-    flexmock(SidetagModel).should_receive("get_or_create").with_args(
+    flexmock(SidetagModel).should_receive("get_or_create_for_updating").with_args(
         sidetag_group, "f39"
     ).and_return(flexmock(koji_name="f39-build-side-12345", target="f39"))
-    flexmock(SidetagModel).should_receive("get_or_create").with_args(
+    flexmock(SidetagModel).should_receive("get_or_create_for_updating").with_args(
         sidetag_group, "f40"
     ).and_return(flexmock(koji_name="f40-build-side-12345", target="f40"))
     flexmock(KojiHelper).should_receive("get_tag_info").with_args(