From 201082dc2a7910841aefdb0ded0a3920dce5888c Mon Sep 17 00:00:00 2001
From: Simon Westphahl <simon.westphahl@bmw.de>
Date: Thu, 11 Apr 2024 10:05:47 +0200
Subject: [PATCH] Cancel jobs of abandoned circular dep. change

When a change that is part of a circular dependency is abandoned we'd
set the item status to dequeued needing change. This will set all builds
as skipped, overwriting exiting builds.

This means that when the item was removed, we did not cancel any of the
builds. For normal builds this mainly waste resources, but if there are
paused builds, those will be leaked and continue running until the
executor is force-restarted.

The fix here is to cancel the jobs before setting it as dequeued needing
change.

Change-Id: If111fe1a21a1c944abcf460a6601293c255376d6
---
 .../git/org_project7/playbooks/run.yaml       |  7 +++++
 .../git/org_project7/zuul.yaml                | 18 +++++++++++++
 .../config/circular-dependencies/main.yaml    |  1 +
 tests/unit/test_circular_dependencies.py      | 27 +++++++++++++++++++
 zuul/manager/__init__.py                      |  3 +++
 5 files changed, 56 insertions(+)
 create mode 100644 tests/fixtures/config/circular-dependencies/git/org_project7/playbooks/run.yaml
 create mode 100644 tests/fixtures/config/circular-dependencies/git/org_project7/zuul.yaml

diff --git a/tests/fixtures/config/circular-dependencies/git/org_project7/playbooks/run.yaml b/tests/fixtures/config/circular-dependencies/git/org_project7/playbooks/run.yaml
new file mode 100644
index 0000000000..9d6f2dae80
--- /dev/null
+++ b/tests/fixtures/config/circular-dependencies/git/org_project7/playbooks/run.yaml
@@ -0,0 +1,7 @@
+- hosts: all
+  tasks:
+    - name: Pause and let child run
+      zuul_return:
+        data:
+          zuul:
+            pause: true
diff --git a/tests/fixtures/config/circular-dependencies/git/org_project7/zuul.yaml b/tests/fixtures/config/circular-dependencies/git/org_project7/zuul.yaml
new file mode 100644
index 0000000000..b74a589919
--- /dev/null
+++ b/tests/fixtures/config/circular-dependencies/git/org_project7/zuul.yaml
@@ -0,0 +1,18 @@
+- job:
+    name: project7-parent-job
+    run: playbooks/run.yaml
+    deduplicate: true
+
+- job:
+    name: project7-child-job
+    deduplicate: true
+
+- project:
+    name: org/project7
+    queue: integrated
+    check:
+      jobs:
+        - project7-parent-job
+        - project7-child-job:
+            dependencies:
+              - project7-parent-job
diff --git a/tests/fixtures/config/circular-dependencies/main.yaml b/tests/fixtures/config/circular-dependencies/main.yaml
index d03b087f9f..2aa16dfafe 100644
--- a/tests/fixtures/config/circular-dependencies/main.yaml
+++ b/tests/fixtures/config/circular-dependencies/main.yaml
@@ -12,6 +12,7 @@
           - org/project3
           - org/project5
           - org/project6
+          - org/project7
       github:
         untrusted-projects:
           - gh/project
diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py
index 6e4897a445..beb8c4387a 100644
--- a/tests/unit/test_circular_dependencies.py
+++ b/tests/unit/test_circular_dependencies.py
@@ -1175,6 +1175,33 @@ class TestGerritCircularDependencies(ZuulTestCase):
         self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified")
         self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "-1")
 
+    def test_abandon_cancel_jobs(self):
+        self.executor_server.hold_jobs_in_build = True
+        A = self.fake_gerrit.addFakeChange("org/project7", "master", "A")
+        B = self.fake_gerrit.addFakeChange("org/project7", "master", "B")
+
+        # A <-> B (via commit-depends)
+        A.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
+            A.subject, B.data["url"]
+        )
+        B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
+            B.subject, A.data["url"]
+        )
+
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+
+        self.fake_gerrit.addEvent(B.getChangeAbandonedEvent())
+        self.waitUntilSettled()
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        self.assertHistory([
+            dict(name="project7-parent-job", result="ABORTED", changes="2,1 1,1"),
+        ], ordered=False)
+
     def test_cycle_merge_conflict(self):
         self.hold_merge_jobs_in_queue = True
         A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py
index feaa94079a..cd98b6f5a1 100644
--- a/zuul/manager/__init__.py
+++ b/zuul/manager/__init__.py
@@ -478,6 +478,9 @@ class PipelineManager(metaclass=ABCMeta):
             for item_change in item.changes:
                 if item_change.equals(change):
                     if len(item.changes) > 1:
+                        # We need to cancel all jobs here as setting
+                        # dequeued needing change will skip all jobs.
+                        self.cancelJobs(item)
                         msg = ("Dependency cycle change "
                                f"{change.url} abandoned.")
                         item.setDequeuedNeedingChange(msg)