[autorevert] fix handling for insufficient successes (#7235)

Previously the code was trying to group branches for restarts resulting
from "infra check" and from "insufficient events", and this was a
mistake, resulting in delayed restarts.

Specifically, in this situation:
<img width="999" height="747" alt="image"
src="https://github.com/user-attachments/assets/9cd0051e-8d87-4fe2-af90-88a776847c4d"
/>
a restart on the success side is expected, but the system waits for
pending job on the failure side.


This PR decouples and simplifies the logic. Now, all restarts are
scheduled independently (relying on set deduplication) and all final
checks are performed afterwards.

Added a unit test to specifically verify the case above.

Author: izaitsevfb

aws/lambda/pytorch-auto-revert/pytorch_auto_revert/signal.py

@@ -51,6 +51,9 @@ class IneligibleReason(Enum):
     NO_PARTITION = "no_partition"  # insufficient commit history to form partitions
     INFRA_NOT_CONFIRMED = "infra_not_confirmed"  # infra check not confirmed
     INSUFFICIENT_FAILURES = "insufficient_failures"  # not enough failures to make call
+    INSUFFICIENT_SUCCESSES = (
+        "insufficient_successes"  # not enough successes to make call
+    )
     PENDING_GAP = "pending_gap"  # unknown/pending commits present
 
 
@@ -366,41 +369,39 @@ def process_valid_autorevert_pattern(
             if not c.events:
                 restart_commits.add(c.head_sha)
 
+        # infra check section
+
         infra_check_result = partition.confirm_not_an_infra_issue()
-        # note re: event_count < 3:
-        # this is a confidence heuristic to detect flakiness, can adjust as needed
         if (
             infra_check_result == InfraCheckResult.RESTART_FAILURE
-            or partition.failure_events_count() < 3
+            and not partition.failed[-1].has_pending
         ):
-            if not partition.failed[-1].has_pending:
-                # restarting oldest failed
-                restart_commits.add(partition.failed[-1].head_sha)
-            else:
-                if infra_check_result == InfraCheckResult.RESTART_FAILURE:
-                    return Ineligible(
-                        IneligibleReason.INFRA_NOT_CONFIRMED,
-                        f"waiting on pending events on suspected failure side: {partition.failed[-1].head_sha}",
-                    )
-                else:
-                    return Ineligible(
-                        IneligibleReason.INSUFFICIENT_FAILURES,
-                        f"insufficient failures to make call, "
-                        f"pending events on suspected failure side: {partition.failed[-1].head_sha}",
-                    )
+            # restarting oldest failed
+            restart_commits.add(partition.failed[-1].head_sha)
 
         if (
             infra_check_result == InfraCheckResult.RESTART_SUCCESS
-            or partition.success_events_count() < 2
+            and not partition.successful[0].has_pending
         ):
-            if not partition.successful[0].has_pending:
-                # restarting newest successful
-                restart_commits.add(partition.successful[0].head_sha)
-            else:
-                return Ineligible(
-                    IneligibleReason.INFRA_NOT_CONFIRMED,
-                    f"waiting on pending events on suspected success side: {partition.successful[0].head_sha}",
-                )
+            # restarting newest successful
+            restart_commits.add(partition.successful[0].head_sha)
+
+        # additional heuristics to reduce uncertainty / flakiness
+        REQUIRE_FAILED_EVENTS = 3
+        REQUIRE_SUCCESS_EVENTS = 2
+
+        # this is a confidence heuristic to detect flakiness, can adjust the number of events as needed
+        if (
+            partition.failure_events_count() < REQUIRE_FAILED_EVENTS
+            and not partition.failed[-1].has_pending
+        ):
+            restart_commits.add(partition.failed[-1].head_sha)
+
+        if (
+            partition.success_events_count() < REQUIRE_SUCCESS_EVENTS
+            and not partition.successful[0].has_pending
+        ):
+            restart_commits.add(partition.successful[0].head_sha)
 
         if restart_commits:
             return RestartCommits(commit_shas=restart_commits)
@@ -411,6 +412,18 @@ def process_valid_autorevert_pattern(
                 f"infra check result: {infra_check_result.value}",
             )
 
+        if partition.failure_events_count() < REQUIRE_FAILED_EVENTS:
+            return Ineligible(
+                IneligibleReason.INSUFFICIENT_FAILURES,
+                f"not enough failures to make call: {partition.failure_events_count()}",
+            )
+
+        if partition.success_events_count() < REQUIRE_SUCCESS_EVENTS:
+            return Ineligible(
+                IneligibleReason.INSUFFICIENT_SUCCESSES,
+                f"not enough successes to make call: {partition.success_events_count()}",
+            )
+
         if partition.unknown:
             # there are still pending/missing commits in the unknown partition
             unknown_shas = ", ".join(c.head_sha for c in partition.unknown)

aws/lambda/pytorch-auto-revert/pytorch_auto_revert/tests/test_signal.py

@@ -271,7 +271,6 @@ def test_insufficient_failures_returns_insufficient_failures_when_pending_on_fai
         res = s.process_valid_autorevert_pattern()
         self.assertIsInstance(res, Ineligible)
         self.assertEqual(res.reason, IneligibleReason.INSUFFICIENT_FAILURES)
-        self.assertIn("sha_failed_pend", res.message)
 
     def test_insufficient_successes_returns_restart_newest_success_when_no_pending(
         self,
@@ -307,8 +306,10 @@ def test_insufficient_successes_returns_restart_newest_success_when_no_pending(
         self.assertTrue(hasattr(res, "commit_shas"))
         self.assertIn("sha_success", res.commit_shas)
 
-    def test_insufficient_successes_infra_not_confirmed_when_pending_on_success(self):
-        # Three failures present, but newest successful has pending → ineligible awaiting confirmation
+    def test_insufficient_successes_returns_specific_reason_when_pending_on_success(
+        self,
+    ):
+        # Three failures present, but newest successful has pending → ineligible due to insufficient successes
         c_fail_newest = SignalCommit(
             head_sha="sha_fail_newest",
             events=[self._ev("job", SignalStatus.FAILURE, 11)],
@@ -335,8 +336,9 @@ def test_insufficient_successes_infra_not_confirmed_when_pending_on_success(self
         )
         res = s.process_valid_autorevert_pattern()
         self.assertIsInstance(res, Ineligible)
+        # Should be ineligible due to insufficient successes, but infra check takes precedence and
+        # it currently requires two successes to confirm not infra
         self.assertEqual(res.reason, IneligibleReason.INFRA_NOT_CONFIRMED)
-        self.assertIn("sha_success_pend", res.message)
 
     def test_both_sides_restart_accumulate_when_below_thresholds(self):
         # One failure total (<3) and one success total (<2), neither pending.
@@ -360,6 +362,43 @@ def test_both_sides_restart_accumulate_when_below_thresholds(self):
         self.assertIn("sha_failed", res.commit_shas)
         self.assertIn("sha_success", res.commit_shas)
 
+    def test_success_restart_even_when_failed_side_pending_and_insufficient_failures(
+        self,
+    ):
+        # Scenario:
+        # - Only one failed event on the failed side, and that failed commit also has a pending event
+        # - Success side has successes that are earlier than failure (so infra check yields RESTART_SUCCESS)
+        # Expected: restart is still proposed on the success side (due to infra check),
+        # even though failures < 3 and the failed commit is pending.
+
+        # Failed (newer): has PENDING and then FAILURE
+        c_failed_pending = SignalCommit(
+            head_sha="sha_fail_pend",
+            events=[
+                self._ev("job", SignalStatus.PENDING, 5),
+                self._ev("job", SignalStatus.FAILURE, 6),
+            ],
+        )
+        # Successful (older): two successes earlier than any failure/pending, not pending
+        c_success_ok = SignalCommit(
+            head_sha="sha_success_ok",
+            events=[
+                self._ev("job", SignalStatus.SUCCESS, 2),
+                self._ev("job", SignalStatus.SUCCESS, 4),
+            ],
+        )
+        s = Signal(
+            key="job",
+            workflow_name="wf",
+            commits=[c_failed_pending, c_success_ok],  # newest -> older
+        )
+        res = s.process_valid_autorevert_pattern()
+        # Should be a RestartCommits proposing restart on the success side only
+        self.assertNotIsInstance(res, AutorevertPattern)
+        self.assertTrue(hasattr(res, "commit_shas"))
+        self.assertIn("sha_success_ok", res.commit_shas)
+        self.assertNotIn("sha_fail_pend", res.commit_shas)
+
 
 if __name__ == "__main__":
     unittest.main()