Remote PRs: cancel previous PR head commit CI

3 files changed, 59 insertions, 14 deletions

diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx
index 6372ef0..6ab93d7 100644
--- a/mod/mod-ci-github-gh.cxx
+++ b/mod/mod-ci-github-gh.cxx
@@ -513,7 +513,7 @@ namespace brep
   {
     p.next_expect (event::begin_object);
 
-    bool ac (false), pr (false), rp (false), in (false);
+    bool ac (false), pr (false), bf (false), rp (false), in (false);
 
     // Skip unknown/uninteresting members.
     //
@@ -526,6 +526,7 @@ namespace brep
 
       if      (c (ac, "action"))       action = p.next_expect_string ();
       else if (c (pr, "pull_request")) pull_request = gh_pull_request (p);
+      else if (c (bf, "before"))       before = p.next_expect_string ();
       else if (c (rp, "repository"))   repository = gh_repository (p);
       else if (c (in, "installation")) installation = gh_installation (p);
       else p.next_expect_value_skip ();
@@ -542,6 +543,7 @@ namespace brep
   {
     os << "action: " << pr.action;
     os << ", pull_request { " << pr.pull_request << " }";
+    os << ", before: " << (pr.before ? *pr.before : "null");
     os << ", repository { "  << pr.repository << " }";
     os << ", installation { " << pr.installation << " }";
 
diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx
index b29904b..6ede697 100644
--- a/mod/mod-ci-github-gh.hxx
+++ b/mod/mod-ci-github-gh.hxx
@@ -183,6 +183,12 @@ namespace brep
     string action;
 
     gh_pull_request pull_request;
+
+    // The SHA of the previous commit on the head branch before the current
+    // one. Only present if action is "synchronize".
+    //
+    optional<string> before;
+
     gh_repository repository;
     gh_installation installation;
 
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index ba80ed6..119968d 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -406,8 +406,7 @@ namespace brep
         // synchronize
         //   A pull request's head branch was updated from the base branch or
         //   new commits were pushed to the head branch. (Note that there is
-        //   no equivalent event for the base branch. That case gets handled
-        //   in handle_check_suite_request() instead. @@ Not anymore.)
+        //   no equivalent event for the base branch.)
         //
         // Note that both cases are handled the same: we start a new CI
         // request which will be reported on the new commit id.
@@ -507,11 +506,13 @@ namespace brep
     //
 
     // While it would have been nice to cancel CIs of PRs with this branch as
-    // base not to waste resources, there are complications: Firsty, we can
-    // only do this for remote PRs (since local PRs may share the result with
-    // branch push). Secondly, we try to do our best even if the branch
-    // protection rule for head behind is not enabled. In this case, it would
-    // be good to complete the CI. So maybe/later.
+    // base not to waste resources, there are complications: Firstly, we can
+    // only do this for remote PRs (since local PRs will most likely share the
+    // result with branch push). Secondly, we try to do our best even if the
+    // branch protection rule for head behind is not enabled. In this case, it
+    // would be good to complete the CI. So maybe/later. See also the head
+    // case in handle_pull_request(), where we do cancel remote PRs that are
+    // not shared.
 
     // Service id that uniquely identifies the CI tenant.
     //
@@ -1480,12 +1481,6 @@ namespace brep
 
     l3 ([&]{trace << "installation_access_token { " << *iat << " }";});
 
-    // Note that similar to the branch push case above, while it would have
-    // been nice to cancel the previous CI job once the PR head moves (the
-    // "synchronize" event), due to the head sharing problem the previous CI
-    // job might actually still be relevant (in both local and remote PR
-    // cases).
-
     // Distinguish between local and remote PRs by comparing the head and base
     // repositories' paths.
     //
@@ -1494,6 +1489,48 @@ namespace brep
       ? service_data::local
       : service_data::remote);
 
+    // Note that similar to the branch push case above, while it would have
+    // been nice to cancel the previous CI job once the PR head moves (the
+    // "synchronize" event), due to the head sharing problem the previous CI
+    // job might actually still be relevant (in both local and remote PR
+    // cases). So we only do it for the remote PRs and only if the head is not
+    // shared (via tenant reference counting).
+    //
+    if (kind == service_data::remote && pr.action == "synchronize")
+    {
+      if (pr.before)
+      {
+        // Service id that will uniquely identify the CI tenant.
+        //
+        string sid (pr.repository.node_id + ':' + *pr.before);
+
+        if (optional<tenant_service> ts = cancel (error, warn,
+                                                  verb_ ? &trace : nullptr,
+                                                  *build_db_, retry_,
+                                                  "ci-github", sid,
+                                                  true /* ref_count */))
+        {
+          l3 ([&]{trace << "pull request " << pr.pull_request.node_id
+                        << ": attempted to cancel CI of previous head commit"
+                        << " (ref_count: " << ts->ref_count << ')';});
+        }
+        else
+        {
+          // It's possible that there was no CI for the previous commit for
+          // various reasons (e.g., CI was not enabled).
+          //
+          l3 ([&]{trace << "pull request " << pr.pull_request.node_id
+                        << ": failed to cancel CI of previous head commit "
+                        << "with tenant_service id " << sid;});
+        }
+      }
+      else
+      {
+        error << "pull request " << pr.pull_request.node_id
+              << ": before commit is missing in synchronize event";
+      }
+    }
+
     // Note: for remote PRs the check_sha will be set later, in
     // build_unloaded_pre_check(), to test merge commit id.
     //