handle_pull_request(): Create pre-check CI request

4 files changed, 81 insertions, 68 deletions

diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index c5bf6a4..3812bf7 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -102,14 +102,15 @@ namespace brep
                 timestamp iat_ea,
                 uint64_t iid,
                 string rid,
-                string rs,
+                string hs,
                 bool rr)
       : kind (local), pre_check (false), re_request (rr),
         warning_success (ws),
         installation_access (move (iat_tok), iat_ea),
         installation_id (iid),
         repository_node_id (move (rid)),
-        report_sha (move (rs))
+        check_sha (hs),
+        report_sha (move (hs))
   {
   }
 
diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index 462e7f7..a6cb742 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -11,6 +11,8 @@
 
 namespace brep
 {
+  // @@@ Check is any data members are unused.
+
   // Service data associated with the tenant (corresponds to GH check suite).
   //
   // It is always a top-level JSON object and the first member is always the
@@ -44,8 +46,7 @@ namespace brep
   };
 
   // We have two kinds of service data that correspond to the following two
-  // scenarios (those are the only possible ones, until/unless we add support
-  // for merge queues):
+  // typical scenarios (until/unless we add support for merge queues):
   //
   // 1. Branch push (via check_suite) plus zero or more local PRs (via
   //    pull_request) that share the same head commit id.
@@ -58,6 +59,13 @@ namespace brep
   // can be created and is not behind base. We do all this before we actually
   // create the CI tenant.
   //
+  // Note that the above two cases are typical but not the only possible
+  // scenarios. Specifically, it is possible to have a mixture of all three
+  // kinds (branch push, local PR, and remote PR) since the same head commit
+  // id can be present in both local and remote branches. There is no way to
+  // handle this case perfectly and we do the best we can (see
+  // build_unloaded_pre_check() for details).
+  //
   struct service_data
   {
     // The data schema version. Note: must be first member in the object.
@@ -66,6 +74,8 @@ namespace brep
 
     // Kind and phase.
     //
+    // @@ TODO Serialize these fields.
+    //
     enum {local, remote /*, queue */} kind;
     bool pre_check;
     bool re_request; // Re-requested (rebuild).
@@ -129,12 +139,15 @@ namespace brep
 
     // The check_suite constructor.
     //
+    // Note that check_sha and report_sha are both the SHA of the
+    // check_suite's head commit.
+    //
     service_data (bool warning_success,
                   string iat_token,
                   timestamp iat_expires_at,
                   uint64_t installation_id,
                   string repository_node_id,
-                  string report_sha,
+                  string head_sha,
                   bool re_request);
 
     // The pull_request constructor.
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 84633fd..6289d8b 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -473,6 +473,8 @@ namespace brep
     //
     string sid (cs.repository.node_id + ":" + cs.check_suite.head_sha);
 
+    // @@ Let's pass the "state" arguments explicitly in both constructors.
+    //
     service_data sd (warning_success,
                      iat->token,
                      iat->expires_at,
@@ -493,8 +495,15 @@ namespace brep
 
     // Create an unloaded CI request.
     //
+    // Note: use no delay since we need to (re)create the synthetic conclusion
+    // check run as soon as possible.
+    //
     // Note that we use the create() API instead of start() since duplicate
-    // management is no available in start().
+    // management is not available in start().
+    //
+    // After this call we will start getting the build_unloaded()
+    // notifications until (1) we load the request, (2) we cancel it, or (3)
+    // it gets archived after some timeout.
     //
     auto pr (create (error,
                      warn,
@@ -508,7 +517,7 @@ namespace brep
     if (!pr)
     {
       fail << "check suite " << cs.check_suite.node_id
-           << ": unable to create unloaded CI request";
+           << ": unable to create unloaded CI tenant";
     }
 
     if (dtm == duplicate_tenant_mode::replace &&
@@ -524,6 +533,8 @@ namespace brep
 
   // High-level description of pull request (PR) handling
   //
+  // @@ TODO Update these comments.
+  //
   // - Some GitHub pull request terminology:
   //
   //   - Fork and pull model: Pull requests are created in a forked
@@ -681,30 +692,61 @@ 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).
+
+    // @@ TMP We can already determine whether the PR is local or remote so we
+    //    could pass `kind` to the service_data constructor. Let's do.
+
+    // @@ TODO: what happens when the entire PR build is re-requested?
+
+    // @@ TODO Serialize the new service_data fields!
+    //
+    // Note that check_sha will be set later, in build_unloaded_pre_check().
+    //
     service_data sd (warning_success,
                      move (iat->token),
                      iat->expires_at,
                      pr.installation.id,
                      move (pr.repository.node_id),
-                     pr.pull_request.head_sha,
-                     pr.repository.clone_url,
+                     move (pr.pull_request.head_sha) /* report_sha */,
+                     move (pr.repository.clone_url),
                      pr.pull_request.number);
 
-    // Create unloaded CI request. Cancel the existing CI request first if the
-    // head branch has been updated (action is `synchronize`).
+    assert (sd.pre_check); // @@ Get rid (once ctor changed).
+
+    // Create an unloaded CI request for the pre-check phase (during which we
+    // wait for the PR's merge commit and behindness to become available).
     //
-    // After this call we will start getting the build_unloaded()
-    // notifications until (1) we load the request, (2) we cancel it, or (3)
-    // it gets archived after some timeout.
+    // Create with an empty service id so that the generated tenant id is used
+    // instead during the pre-check phase (so as not to clash with a proper
+    // service id for this head commit, potentially created in
+    // handle_check_suite() or as another PR).
     //
-    bool cancel_first (pr.action == "synchronize");
+    tenant_service ts ("", "ci-github", sd.json ());
 
-    if (!create_pull_request_ci (error, warn, trace,
-                                 sd, pr.pull_request.node_id,
-                                 cancel_first))
+    // Note: use no delay since we need to start the actual CI (which in turn
+    // (re)creates the synthetic conclusion check run) as soon as possible.
+    //
+    // After this call we will start getting the build_unloaded()
+    // notifications -- which will be routed to build_unloaded_pre_check() --
+    // until we cancel the request or it gets archived after some
+    // timeout. (Note that we never actually load this request, we always
+    // cancel it; see build_unloaded_pre_check() for details.)
+    //
+    if (!create (error,
+                 warn,
+                 &trace,
+                 *build_db_,
+                 move (ts),
+                 chrono::seconds (30) /* interval */,
+                 chrono::seconds (0) /* delay */))
     {
       fail << "pull request " << pr.pull_request.node_id
-           << ": unable to create unloaded CI request";
+           << ": unable to create unloaded pre-check tenant";
     }
 
     return true;
@@ -749,6 +791,12 @@ namespace brep
     // - Otherwise, create unloaded CI tenant (with proper duplicate mode
     //   based on re_request) and cancel itself.
 
+    // Note that in case of a mixed local/remote case, whether we CI the head
+    // commit or test merge commit will be racy and there is nothing we can do
+    // about (the purely local case can get "upgraded" to mixed after we have
+    // started the CI job).
+    //
+
     return nullptr;
   }
 
@@ -1881,36 +1929,6 @@ namespace brep
     };
   }
 
-  bool ci_github::
-  create_pull_request_ci (const basic_mark& error,
-                          const basic_mark& warn,
-                          const basic_mark& trace,
-                          const service_data& sd,
-                          const string& nid,
-                          bool cf) const
-  {
-    // Cancel the existing CI request if asked to do so. Ignore failure
-    // because the request may already have been cancelled for other reasons.
-    //
-    if (cf)
-    {
-      if (!cancel (error, warn, &trace, *build_db_, "ci-github", nid))
-        l3 ([&] {trace << "unable to cancel CI for pull request " << nid;});
-    }
-
-    // Create a new unloaded CI request.
-    //
-    tenant_service ts (nid, "ci-github", sd.json ());
-
-    // Note: use no delay since we need to (re)create the synthetic merge
-    // check run as soon as possible.
-    //
-    return create (error, warn, &trace,
-                   *build_db_, move (ts),
-                   chrono::seconds (30) /* interval */,
-                   chrono::seconds (0)  /* delay */).has_value ();
-  }
-
   string ci_github::
   details_url (const build& b) const
   {
diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx
index e919065..8284f7f 100644
--- a/mod/mod-ci-github.hxx
+++ b/mod/mod-ci-github.hxx
@@ -91,25 +91,6 @@ namespace brep
     bool
     handle_pull_request (gh_pull_request_event, bool warning_success);
 
-    // Create an unloaded CI request for a pull request. If `cancel_first` is
-    // true, cancel its existing CI request first.
-    //
-    // Return true if an unloaded CI request was created. Ignore failure to
-    // cancel because the CI request may already have been cancelled for other
-    // reasons.
-    //
-    // After this call we will start getting the build_unloaded()
-    // notifications until (1) we load the request, (2) we cancel it, or (3)
-    // it gets archived after some timeout.
-    //
-    bool
-    create_pull_request_ci (const basic_mark& error,
-                            const basic_mark& warn,
-                            const basic_mark& trace,
-                            const service_data&,
-                            const string& pull_request_node_id,
-                            bool cancel_first) const;
-
     // Build a check run details_url for a build.
     //
     string