Make duplicate push builds yield to PR builds

In a GitHub double-event, Buildbot can queue the push and pull_request tarball-docker requests together, but the push build may get a worker first and complete while the corresponding PR request is still waiting in the queue. By the time the PR build finally runs, it is too late to prevent the duplicate tarball work and any downstream triggers that already happened.

Flip the direction so the push side yields when it sees the matching PR request. That makes the first running duplicate decide whether it should continue, instead of depending on the PR side to arrive soon enough after the fact.

The branch guard keeps this limited to disposable normal push branches: refs/* are ignored and branches covered by SAVED_PACKAGE_BRANCHES are not allowed to self-cancel.

Author: RazvanLiviuVarzaru

utils.py

@@ -16,7 +16,7 @@
 from buildbot.plugins import steps, util, worker
 from buildbot.process.builder import Builder
 from buildbot.process.buildstep import BuildStep
-from buildbot.process.results import FAILURE, SUCCESS
+from buildbot.process.results import CANCELLED, FAILURE, SUCCESS
 from buildbot.process.workerforbuilder import AbstractWorkerForBuilder
 from buildbot.worker import AbstractWorker
 from constants import (
@@ -682,11 +682,11 @@ def mtrEnv(props: IProperties) -> dict:
 
 
 class CancelDuplicateBuildRequests(BuildStep):
-    """BuildStep to cancel duplicate buildrequests for the same commit on the same builder
-    if the current build is for a pull request event. It checks for other pending buildrequests
-    with the same builder and if they have a sourcestamp with the same revision as
-    the current build, it cancels them. It only cancels normal branch builds, not other
-    pull request refs, and avoids important branches like main, release or merge branches.
+    """Stop duplicate push builds when a matching pull request build exists.
+
+    When GitHub sends push and pull_request events for the same commit, both can
+    queue tarball-docker requests. A normal branch push should yield to a matching
+    pull request buildrequest on the same builder.
     """
 
     name = "cancel duplicate buildrequests"
@@ -717,17 +717,15 @@ def _fmt_ss(ss):
 
     @staticmethod
     def _branch_is_cancelable(branch):
-        """Only cancel duplicate builds originating from normal branch pushes."""
+        """Only let disposable normal branch pushes yield to pull request builds."""
         if not branch:
             return False
 
         branch_lc = branch.lower()
         return (
             len(branch) > 5
             and not branch_lc.startswith("refs/")
-            and "release" not in branch_lc
-            and "merge" not in branch_lc
-            and "preview" not in branch_lc
+            and not fnmatch_any(branch, SAVED_PACKAGE_BRANCHES)
         )
 
     @defer.inlineCallbacks
@@ -744,8 +742,8 @@ def run(self):
             self.addCompleteLog("duplicate-buildrequests", "\n".join(lines) + "\n")
             return SUCCESS
 
-        if event != "pull_request":
-            lines.append("Event is not 'pull_request'; nothing to do.")
+        if event != "push":
+            lines.append("Event is not 'push'; nothing to do.")
             self.addCompleteLog("duplicate-buildrequests", "\n".join(lines) + "\n")
             return SUCCESS
 
@@ -768,15 +766,17 @@ def run(self):
             self.addCompleteLog("duplicate-buildrequests", "\n".join(lines) + "\n")
             return SUCCESS
 
-        current_revisions = {
-            ss.get("revision")
+        current_targets = {
+            (ss.get("revision"), ss.get("repository"), ss.get("codebase", ""))
             for ss in current_sourcestamps
             if ss.get("revision") is not None
+            and self._branch_is_cancelable(ss.get("branch"))
         }
 
-        if not current_revisions:
+        if not current_targets:
             lines.append(
-                "Current buildset has no revision in sourcestamps; nothing to do."
+                "Current buildset has no cancelable push sourcestamp with a "
+                "revision; nothing to do."
             )
             self.addCompleteLog("duplicate-buildrequests", "\n".join(lines) + "\n")
             return SUCCESS
@@ -787,20 +787,12 @@ def run(self):
             if ss.get("branch") is not None
         }
 
-        if not any(branch.startswith("refs/pull/") for branch in current_branches):
-            lines.append(
-                "Current build is a pull_request event, but no pull request ref "
-                "was found in sourcestamps; nothing to do."
-            )
-            self.addCompleteLog("duplicate-buildrequests", "\n".join(lines) + "\n")
-            return SUCCESS
-
         lines.append("Current:")
         lines.append(f"  buildid={current_buildid}")
         lines.append(f"  buildrequestid={current_buildrequestid}")
         lines.append(f"  builderid={current_builderid}")
         lines.append(f"  buildsetid={current_buildsetid}")
-        lines.append(f"  revisions={sorted(current_revisions)!r}")
+        lines.append(f"  targets={sorted(current_targets)!r}")
         lines.append(f"  branches={sorted(current_branches)!r}")
         current_url = self._buildrequest_url(current_buildrequestid)
         if current_url:
@@ -829,8 +821,6 @@ def run(self):
 
         matches = []
         actions = []
-        cancel_errors = []
-
         for br in buildrequests:
             brid = br["buildrequestid"]
 
@@ -848,16 +838,19 @@ def run(self):
             for other_ss in other_sourcestamps:
                 other_revision = other_ss.get("revision")
                 other_branch = other_ss.get("branch")
+                other_target = (
+                    other_revision,
+                    other_ss.get("repository"),
+                    other_ss.get("codebase", ""),
+                )
 
-                if other_revision not in current_revisions:
-                    continue
-
-                if not self._branch_is_cancelable(other_branch):
+                if not other_branch or not other_branch.startswith("refs/pull/"):
                     continue
 
-                matched_revision = other_revision
-                matched_branch = other_branch
-                break
+                if other_target in current_targets:
+                    matched_revision = other_revision
+                    matched_branch = other_branch
+                    break
 
             if matched_revision is None:
                 continue
@@ -876,47 +869,24 @@ def run(self):
 
             if self.dry_run:
                 msg = (
-                    f"[DRY-RUN] would cancel buildrequest {brid} "
+                    f"[DRY-RUN] would stop current build "
+                    f"because pull request buildrequest {brid} exists "
                     f"(claimed={br.get('claimed')}, "
                     f"revision={matched_revision!r}, "
                     f"branch={matched_branch!r})"
                 )
-                if info["url"]:
-                    msg += f" url={info['url']}"
-                actions.append(msg)
             else:
-                try:
-                    yield self.master.data.control(
-                        "cancel",
-                        {"reason": "Duplicate build for same commit on same builder"},
-                        ("buildrequests", brid),
-                    )
-                except Exception as e:
-                    msg = (
-                        f"Failed to request cancel for buildrequest {brid} "
-                        f"(claimed={br.get('claimed')}, "
-                        f"revision={matched_revision!r}, "
-                        f"branch={matched_branch!r}, "
-                        f"error={e!r})"
-                    )
-                    if info["url"]:
-                        msg += f" url={info['url']}"
-                    actions.append(msg)
-                    cancel_errors.append(msg)
-                    log.err(e, f"Failed to request cancel for buildrequest {brid}")
-                else:
-                    msg = (
-                        f"Requested cancel for buildrequest {brid} "
-                        f"(claimed={br.get('claimed')}, "
-                        f"revision={matched_revision!r}, "
-                        f"branch={matched_branch!r})"
-                    )
-                    if info["url"]:
-                        msg += f" url={info['url']}"
-                    actions.append(msg)
+                msg = (
+                    f"Stopping current build because pull request buildrequest {brid} "
+                    f"exists (claimed={br.get('claimed')}, "
+                    f"revision={matched_revision!r}, "
+                    f"branch={matched_branch!r})"
+                )
+            if info["url"]:
+                msg += f" url={info['url']}"
+            actions.append(msg)
 
-        lines.append(f"Matched duplicate buildrequests: {len(matches)}")
-        lines.append(f"Cancel request failures: {len(cancel_errors)}")
+        lines.append(f"Matched pull request buildrequests: {len(matches)}")
         lines.append("")
 
         if matches:
@@ -934,9 +904,14 @@ def run(self):
             lines.append("Actions:")
             lines.extend(f"  {a}" for a in actions)
         else:
-            lines.append("No matching cancelable buildrequests found on this builder.")
+            lines.append(
+                "No matching pull request buildrequests found on this builder."
+            )
 
         self.addCompleteLog("duplicate-buildrequests", "\n".join(lines) + "\n")
+        if matches and not self.dry_run:
+            self.build.stopBuild("Superseded by pull request build for same commit")
+            return CANCELLED
         return SUCCESS