Always apply obsoletion check in check_karma_thresholds

The "obsolete on unstable_karma" branch being `elif`ed with the
"disable autokarma on any negative karma" branch means that, if
we reach the unstable_karma threshold when autokarma is enabled,
we don't obsolete the update. That's clearly wrong. The most
likely way to hit this is for unstable_karma to be -1, so the
first negative karma causes both conditions to be met at once.
But it's also possible if e.g. unstable_karma is -2, the update
receives one negative karma, the maintainer re-enables autokarma,
then the update receives another negative karma.

We should *always* obsolete an update when it reaches
unstable_karma, regardless of anything else we do. If this means
we both obsolete it and disable autokarma at the same time, I
don't see any problem with that, it seems correct.

Signed-off-by: Adam Williamson <[email protected]>

bodhi-server/bodhi/server/models.py

@@ -3810,6 +3810,12 @@ def check_karma_thresholds(self, db, agent):
         # Also return if the status of the update is not in testing and the release is frozen
         if self.status != UpdateStatus.testing and self.release.state == ReleaseState.frozen:
             return
+        # Obsolete if unstable karma threshold reached
+        if self.unstable_karma and self.karma <= self.unstable_karma:
+            log.info("Automatically obsoleting %s (reached unstable karma threshold)", self.alias)
+            self.obsolete(db)
+            notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
+                dict(update=self, status='unstable')))
         # If an update receives negative karma disable autopush
         # exclude rawhide updates see #4566
         if (self.autokarma or self.autotime) and self._composite_karma[1] != 0 and \
@@ -3820,7 +3826,8 @@ def check_karma_thresholds(self, db, agent):
             self.autotime = False
             text = config.get('disable_automatic_push_to_stable')
             self.comment(db, text, author='bodhi')
-        elif self.stable_karma and self.karma >= self.stable_karma \
+        # If update with autopush reaches threshold, set request stable
+        if self.stable_karma and self.karma >= self.stable_karma \
                 and self.release.composed_by_bodhi and self.autokarma:
             # Updates for releases not "composed by Bodhi" (Rawhide,
             # ELN...) are pushed stable only by approve_testing.py
@@ -3834,11 +3841,6 @@ def check_karma_thresholds(self, db, agent):
             self.set_request(db, UpdateRequest.stable, agent)
             notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
                 dict(update=self, status='stable')))
-        elif self.unstable_karma and self.karma <= self.unstable_karma:
-            log.info("Automatically obsoleting %s (reached unstable karma threshold)", self.alias)
-            self.obsolete(db)
-            notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
-                dict(update=self, status='unstable')))
 
     @property
     def builds_json(self):

bodhi-server/tests/services/test_updates.py

@@ -4720,23 +4720,32 @@ def test_obsolete_if_unstable(self, autokarma, composed_by_bodhi, req, status):
         # caused by the obsoletion to have taken effect when we construct the
         # expected message
         up.comment(self.db, "foo", -1, 'bowlofeggs')
-        with fml_testing.mock_sends(
-            update_schemas.UpdateKarmaThresholdV1.from_dict(
+        if req:
+            # it's difficult to construct the expected messsage in this
+            # case because posting this comment obsoletes the update,
+            # publishes the UKT message, *then also* disables autopush,
+            # so we never quite have access to the state of the update
+            # right at the time the UKT message is published
+            expukt = update_schemas.UpdateKarmaThresholdV1
+        else:
+            # in this case we *only* obsolete the update, we don't also
+            # disable autopush, so we can construct the expected message -
+            # the current update state is the same as the state when the
+            # message was published
+            expukt = update_schemas.UpdateKarmaThresholdV1.from_dict(
                 {'update': up, 'status': 'unstable'}
-            ),
-            update_schemas.UpdateCommentV1
-        ):
+            )
+        with fml_testing.mock_sends(expukt, update_schemas.UpdateCommentV1):
             # doing a db commit causes the message to be published
             self.db.commit()
         assert up.status is UpdateStatus.obsolete
         assert up.request is None
-        # the obsolete path should not disable autokarma, but we can't assert
-        # this unconditionally because we might have hit the earlier disable-
-        # autokarma path
-        if (
-            req is UpdateRequest.stable
-            or not composed_by_bodhi
-        ):
+        # the obsolete path itself should not disable autokarma, but if
+        # the release is composed by bodhi, we *should* also have hit
+        # the 'disable autokarma for any negative karma' path
+        if composed_by_bodhi:
+            assert not up.autokarma
+        else:
             assert up.autokarma is autokarma
         assert up.karma == -2