Actually fix race condition when creating sidetags in Koji

Signed-off-by: Nikola Forró <[email protected]>
packit · Nov 11, 2024 · 3de1e30 · 3de1e30
1 parent bea3f49
commit 3de1e30
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 25 deletions.
diff --git a/packit_service/models.py b/packit_service/models.py
@@ -3974,18 +3974,24 @@ class SidetagModel(Base):
     @contextmanager
     def get_or_create_for_updating(
         cls,
-        group: "SidetagGroupModel",
+        group_name: str,
         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()
+        with sa_session_transaction(commit=True) as session:
+            # lock the sidetag group in the DB by using SELECT ... FOR UPDATE
+            # all other workers accessing this group will block here until the context is exited
+            group = (
+                session.query(SidetagGroupModel)
+                .filter_by(name=group_name)
+                .with_for_update()
+                .first()
+            )
 
-        try:
             sidetag = session.query(cls).filter_by(sidetag_group_id=group.id, target=target).first()
             if not sidetag:
                 sidetag = cls()
@@ -3994,24 +4000,9 @@ def get_or_create_for_updating(
 
                 group.sidetags.append(sidetag)
                 session.add(group)
-        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"]:

diff --git a/packit_service/worker/helpers/sidetag.py b/packit_service/worker/helpers/sidetag.py
@@ -150,7 +150,7 @@ 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)
-        with SidetagModel.get_or_create_for_updating(group, dist_git_branch) as sidetag:
+        with SidetagModel.get_or_create_for_updating(group.name, dist_git_branch) as sidetag:
             if not sidetag.koji_name or not cls.koji_helper.get_tag_info(
                 sidetag.koji_name,
             ):

diff --git a/tests/integration/test_dg_commit.py b/tests/integration/test_dg_commit.py
@@ -268,7 +268,7 @@ def test_downstream_koji_build(sidetag_group):
             flexmock(koji_name=sidetag),
         )
         flexmock(SidetagGroupModel).should_receive("get_or_create").and_return(
-            flexmock(),
+            flexmock(name="test"),
         )
         flexmock(KojiHelper).should_receive("get_tag_info").and_return(None)
         flexmock(PackitAPI).should_receive("init_kerberos_ticket").and_return(None)
@@ -299,7 +299,7 @@ def test_downstream_koji_build(sidetag_group):
             flexmock(koji_name=sidetag),
         )
         flexmock(SidetagGroupModel).should_receive("get_or_create").and_return(
-            flexmock(),
+            flexmock(name="test"),
         )
         flexmock(KojiHelper).should_receive("get_tag_info").and_return(None)
         flexmock(PackitAPI).should_receive("init_kerberos_ticket").and_return(None)

diff --git a/tests/integration/test_pr_comment.py b/tests/integration/test_pr_comment.py
@@ -3054,16 +3054,16 @@ def test_koji_build_tag_via_dist_git_pr_comment(pagure_pr_comment_added, all_bra
         default_dg_branch="rawhide",
     ).and_return({"f39", "f40"})
 
-    sidetag_group = flexmock()
+    sidetag_group = flexmock(name="test")
     flexmock(SidetagGroupModel).should_receive("get_or_create").with_args(
         "test",
     ).and_return(sidetag_group)
     flexmock(SidetagModel).should_receive("get_or_create_for_updating").with_args(
-        sidetag_group,
+        sidetag_group.name,
         "f39",
     ).and_return(flexmock(koji_name="f39-build-side-12345", target="f39"))
     flexmock(SidetagModel).should_receive("get_or_create_for_updating").with_args(
-        sidetag_group,
+        sidetag_group.name,
         "f40",
     ).and_return(flexmock(koji_name="f40-build-side-12345", target="f40"))
     flexmock(KojiHelper).should_receive("get_tag_info").with_args(