Handle replaced tenants and clean up code/comments

7 files changed, 109 insertions, 105 deletions

diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx
index fc5cf82..f0de991 100644
--- a/mod/mod-ci-github-gh.cxx
+++ b/mod/mod-ci-github-gh.cxx
@@ -356,7 +356,7 @@ namespace brep
   {
     p.next_expect (event::begin_object);
 
-    bool ni (false), nm (false), fn (false), cu (false);
+    bool ni (false), fn (false), cu (false);
 
     // Skip unknown/uninteresting members.
     //
@@ -368,14 +368,12 @@ namespace brep
       };
 
       if      (c (ni, "node_id"))        node_id = p.next_expect_string ();
-      else if (c (nm, "name"))           name = p.next_expect_string ();
       else if (c (fn, "full_name"))      path = p.next_expect_string ();
       else if (c (cu, "clone_url"))      clone_url = p.next_expect_string ();
       else p.next_expect_value_skip ();
     }
 
     if (!ni) missing_member (p, "gh_repository", "node_id");
-    if (!nm) missing_member (p, "gh_repository", "name");
     if (!fn) missing_member (p, "gh_repository", "full_name");
     if (!cu) missing_member (p, "gh_repository", "clone_url");
   }
@@ -384,7 +382,6 @@ namespace brep
   operator<< (ostream& os, const gh_repository& rep)
   {
     os << "node_id: " << rep.node_id
-       << ", name: " << rep.name
        << ", path: " << rep.path
        << ", clone_url: " << rep.clone_url;
 
diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx
index eeda871..392c0e8 100644
--- a/mod/mod-ci-github-gh.hxx
+++ b/mod/mod-ci-github-gh.hxx
@@ -21,8 +21,6 @@ namespace butl
 
 namespace brep
 {
-   // @@@ Check if any data members are unused (once the dust settles).
-
   using build_queued_hints = tenant_service_build_queued::build_queued_hints;
 
   // GitHub request/response types (all start with gh_).
@@ -87,15 +85,12 @@ namespace brep
     string node_id;
     unsigned int number;
 
-    // @@ TMP The unused base/head members may be useful for trace output when
-    //    we receive the pull_request webhook.
-
     string base_path; // Repository path (<org>/<repo>) under github.com.
-    string base_ref;  // @@ TODO Remove if remains unused.
-    string base_sha;  // @@ TODO Remove if remains unused.
+    string base_ref;
+    string base_sha;
 
     string head_path;
-    string head_ref; // @@ TODO Remove if remains unused.
+    string head_ref;
     string head_sha;
 
     explicit
@@ -133,7 +128,6 @@ namespace brep
   struct gh_repository
   {
     string node_id;
-    string name;
     string path; // Repository path (<org>/<repo>) under github.com.
     string clone_url;
 
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index 7b7e464..774eeed 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -17,9 +17,11 @@ namespace brep
   // bottom).
   //
   static const string& gq_name (const string&);
+  static string        gq_name (string&&);
   static string        gq_str (const string&);
   static string        gq_bool (bool);
   static const string& gq_enum (const string&);
+  static string        gq_enum (string&&);
 
   [[noreturn]] static void
   throw_json (json::parser& p, const string& m)
@@ -273,11 +275,6 @@ namespace brep
             // Note that GitHub won't allow us to change a built check run to
             // any other state (but all other transitions are allowed).
             //
-            // @@ Are we handling the case where the resulting state (built)
-            //    differs from what we expect?
-            //
-            // @@@ Does built-to-built transition updates status?
-            //
             if (rst != st && rst != build_state::built)
             {
               error << "unexpected check_run status: received '" << rcr.status
@@ -830,8 +827,6 @@ namespace brep
   //
   // Return the name or throw invalid_argument if it is invalid.
   //
-  // @@ TODO: dangerous API.
-  //
   static const string&
   gq_name (const string& v)
   {
@@ -850,6 +845,13 @@ namespace brep
     return v;
   }
 
+  static string
+  gq_name (string&& v)
+  {
+    gq_name (v);
+    return move (v);
+  }
+
   // Serialize a string to GraphQL.
   //
   // Return the serialized string or throw invalid_argument if the string is
@@ -904,8 +906,6 @@ namespace brep
   //
   // Return the enum value or throw invalid_argument if it is invalid.
   //
-  // @@ TODO: dangerous API.
-  //
   static const string&
   gq_enum (const string& v)
   {
@@ -914,4 +914,12 @@ namespace brep
 
     return gq_name (v);
   }
+
+  static string
+  gq_enum (string&& v)
+  {
+    gq_enum (v);
+    return move (v);
+  }
+
 }
diff --git a/mod/mod-ci-github-service-data.hxx b/mod/mod-ci-github-service-data.hxx
index 776ec8d..0f4c760 100644
--- a/mod/mod-ci-github-service-data.hxx
+++ b/mod/mod-ci-github-service-data.hxx
@@ -11,8 +11,6 @@
 
 namespace brep
 {
-  // @@@ Check if any data members are unused (once the dust settles).
-
   // 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
@@ -99,7 +97,7 @@ namespace brep
 
     // The following two are only used for pull requests.
     //
-    // @@ TODO/LATER: maybe put them in a struct?
+    // @@ TODO/LATER: maybe put them in a struct, if more members?
     //
     optional<string>   pr_node_id;
     optional<uint32_t> pr_number;
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 113da2e..394638a 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -19,26 +19,6 @@
 
 #include <stdexcept>
 
-// @@ Remaining TODOs
-//
-//    - Rerequested checks
-//
-//      - check_suite (action: rerequested): received when user re-runs all
-//        checks.
-//
-//      - check_run (action: rerequested): received when user re-runs a
-//        specific check or all failed checks.
-//
-//        @@ TMP I have confirmed that the above is accurate.
-//
-//      Will need to extract a few more fields from check_runs, but the layout
-//      is very similar to that of check_suite.
-//
-//    - Choose strong webhook secret (when deploying).
-//
-//    - Check that delivery UUID has not been received before (replay attack).
-//
-
 // Resources:
 //
 //    Creating an App:
@@ -280,9 +260,6 @@ namespace brep
     // is that we want be "notified" of new actions at which point we can
     // decide whether to ignore them or to handle.
     //
-    // @@ There is also check_run even (re-requested by user, either
-    //    individual check run or all the failed check runs).
-    //
     if (event == "check_suite")
     {
       gh_check_suite_event cs;
@@ -316,13 +293,10 @@ namespace brep
       else if (cs.action == "completed")
       {
         // GitHub thinks that "all the check runs in this check suite have
-        // completed and a conclusion is available". Looks like this one we
-        // ignore?
+        // completed and a conclusion is available". Check with our own
+        // bookkeeping and log an error if there is a mismatch.
         //
-        // What if our bookkeeping says otherwise? But then we can't even
-        // access the service data easily here. @@ TODO: maybe/later.
-        //
-        return true;
+        return handle_check_suite_completed (move (cs), warning_success);
       }
       else
       {
@@ -558,10 +532,6 @@ namespace brep
 
     l3 ([&]{trace << "installation_access_token { " << *iat << " }";});
 
-    // @@ What happens if we call this functions with an already existing
-    //    node_id (e.g., replay attack). See the UUID header above.
-    //
-
     // While it would have been nice to cancel CIs of PRs with this branch as
     // 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
@@ -690,6 +660,23 @@ namespace brep
     return true;
   }
 
+  bool ci_github::
+  handle_check_suite_completed (gh_check_suite_event cs, bool warning_success)
+  {
+    // The plans is as follows:
+    //
+    // 1. Load the service data.
+    //
+    // 2. Verify it is completed.
+    //
+    // 3. Verify (like in build_built()) that all the check runs are
+    //    completed.
+    //
+    // 4. Verify the result matches what GitHub thinks it is (if easy).
+
+    return true;
+  }
+
   // Create a gq_built_result.
   //
   // Throw invalid_argument in case of invalid result_status.
@@ -795,6 +782,7 @@ namespace brep
     // archived.
     //
     service_data sd;
+    string tenant_id;
     {
       // Service id that uniquely identifies the CI tenant.
       //
@@ -868,6 +856,8 @@ namespace brep
         {
           fail << "failed to parse service data: " << e;
         }
+
+        tenant_id = d->tenant_id;
       }
       else
       {
@@ -1015,7 +1005,8 @@ namespace brep
     // only if the build is actually restarted).
     //
     auto update_sd = [&error, &new_iat, &race,
-                      &cr, &bcr, &ccr] (const string& /*tenant_id*/,
+                      tenant_id = move (tenant_id),
+                      &cr, &bcr, &ccr] (const string& ti,
                                         const tenant_service& ts,
                                         build_state) -> optional<string>
     {
@@ -1024,6 +1015,16 @@ namespace brep
 
       race = false; // Reset.
 
+      if (tenant_id != ti)
+      {
+        // The tenant got replaced since we loaded it but we managed to
+        // trigger a rebuild in the new tenant. Who knows whose check runs are
+        // visible, so let's fail ours similar to the cases below.
+        //
+        race = true;
+        return nullopt;
+      }
+
       service_data sd;
       try
       {
@@ -1320,9 +1321,8 @@ namespace brep
     return true;
   }
 
-  function<optional<string> (const string& tenant_id,
-                             const tenant_service&)> ci_github::
-  build_unloaded (const string& /*tenant_id*/,
+  function<optional<string> (const string&, const tenant_service&)> ci_github::
+  build_unloaded (const string& ti,
                   tenant_service&& ts,
                   const diag_epilogue& log_writer) const noexcept
   {
@@ -1342,12 +1342,11 @@ namespace brep
     }
 
     return sd.pre_check
-             ? build_unloaded_pre_check (move (ts), move (sd), log_writer)
-             : build_unloaded_load (move (ts), move (sd), log_writer);
+      ? build_unloaded_pre_check (move (ts), move (sd), log_writer)
+      : build_unloaded_load (ti, move (ts), move (sd), log_writer);
   }
 
-  function<optional<string> (const string& tenant_id,
-                             const tenant_service&)> ci_github::
+  function<optional<string> (const string&, const tenant_service&)> ci_github::
   build_unloaded_pre_check (tenant_service&& ts,
                             service_data&& sd,
                             const diag_epilogue& log_writer) const noexcept
@@ -1547,9 +1546,9 @@ namespace brep
     return nullptr;
   }
 
-  function<optional<string> (const string& tenant_id,
-                             const tenant_service&)> ci_github::
-  build_unloaded_load (tenant_service&& ts,
+  function<optional<string> (const string&, const tenant_service&)> ci_github::
+  build_unloaded_load (const string& tenant_id,
+                       tenant_service&& ts,
                        service_data&& sd,
                        const diag_epilogue& log_writer) const noexcept
   try
@@ -1751,15 +1750,19 @@ namespace brep
       return nullptr; // Nothing to save (but potentially retry on next call).
 
     return [&error,
+            tenant_id,
             iat = move (new_iat),
             cni = move (conclusion_node_id)]
-      (const string& /*tenant_id*/,
+      (const string& ti,
        const tenant_service& ts) -> optional<string>
     {
       // NOTE: this lambda may be called repeatedly (e.g., due to
       // transaction being aborted) and so should not move out of its
       // captures.
 
+      if (tenant_id != ti)
+        return nullopt; // Do nothing if the tenant has been replaced.
+
       service_data sd;
       try
       {
@@ -1812,9 +1815,9 @@ namespace brep
   //    them when notifying GitHub. The first is not important (we expect the
   //    state to go back to building shortly). The second should normally not
   //    happen and would mean that a completed check suite may go back on its
-  //    conclusion (which would be pretty confusing for the user). @@@ This
-  //    can/will happen on check run rebuild. Distinguish between internal
-  //    and external rebuilds?
+  //    conclusion (which would be pretty confusing for the user). Note that
+  //    the ->queued state transition of a check run rebuild triggered by
+  //    us is handled directly in handle_check_run_rerequest().
   //
   //    So, for GitHub notifications, we only have the following linear
   //    transition sequence:
@@ -1891,9 +1894,8 @@ namespace brep
   //    if we have node_id, then we update, otherwise, we create (potentially
   //    overriding the check run created previously).
   //
-  function<optional<string> (const string& tenant_id,
-                             const tenant_service&)> ci_github::
-  build_queued (const string& /*tenant_id*/,
+  function<optional<string> (const string&, const tenant_service&)> ci_github::
+  build_queued (const string& tenant_id,
                 const tenant_service& ts,
                 const vector<build>& builds,
                 optional<build_state> istate,
@@ -2019,16 +2021,20 @@ namespace brep
       }
     }
 
-    return [bs = move (bs),
+    return [tenant_id,
+            bs = move (bs),
             iat = move (new_iat),
             crs = move (crs),
             error = move (error),
-            warn = move (warn)] (const string& /*tenant_id*/,
+            warn = move (warn)] (const string& ti,
                                  const tenant_service& ts) -> optional<string>
     {
       // NOTE: this lambda may be called repeatedly (e.g., due to transaction
       // being aborted) and so should not move out of its captures.
 
+      if (tenant_id != ti)
+        return nullopt; // Do nothing if the tenant has been replaced.
+
       service_data sd;
       try
       {
@@ -2077,9 +2083,8 @@ namespace brep
     return nullptr;
   }
 
-  function<optional<string> (const string& tenant_id,
-                             const tenant_service&)> ci_github::
-  build_building (const string& /*tenant_id*/,
+  function<optional<string> (const string&, const tenant_service&)> ci_github::
+  build_building (const string& tenant_id,
                   const tenant_service& ts,
                   const build& b,
                   const diag_epilogue& log_writer) const noexcept
@@ -2187,15 +2192,19 @@ namespace brep
       }
     }
 
-    return [iat = move (new_iat),
+    return [tenant_id,
+            iat = move (new_iat),
             cr = move (*cr),
             error = move (error),
-            warn = move (warn)] (const string& /*tenant_id*/,
+            warn = move (warn)] (const string& ti,
                                  const tenant_service& ts) -> optional<string>
     {
       // NOTE: this lambda may be called repeatedly (e.g., due to transaction
       // being aborted) and so should not move out of its captures.
 
+      if (tenant_id != ti)
+        return nullopt; // Do nothing if the tenant has been replaced.
+
       service_data sd;
       try
       {
@@ -2241,9 +2250,8 @@ namespace brep
     return nullptr;
   }
 
-  function<optional<string> (const string& tenant_id,
-                             const tenant_service&)> ci_github::
-  build_built (const string& /*tenant_id*/,
+  function<optional<string> (const string&, const tenant_service&)> ci_github::
+  build_built (const string& tenant_id,
                const tenant_service& ts,
                const build& b,
                const diag_epilogue& log_writer) const noexcept
@@ -2253,9 +2261,8 @@ namespace brep
 
     NOTIFICATION_DIAG (log_writer);
 
-    // @@ TODO Include service_data::event_node_id and perhaps ts.id in
-    //    diagnostics? E.g. when failing to update check runs we print the
-    //    build ID only.
+    // @@ TODO Include ts.id in diagnostics? Check run build ids alone seem
+    //    kind of meaningless. Log lines get pretty long this way however.
 
     service_data sd;
     try
@@ -2569,16 +2576,20 @@ namespace brep
       }
     }
 
-    return [iat = move (new_iat),
+    return [tenant_id,
+            iat = move (new_iat),
             cr = move (cr),
             completed = completed,
             error = move (error),
-            warn = move (warn)] (const string& /*tenant_id*/,
+            warn = move (warn)] (const string& ti,
                                  const tenant_service& ts) -> optional<string>
     {
       // NOTE: this lambda may be called repeatedly (e.g., due to transaction
       // being aborted) and so should not move out of its captures.
 
+      if (tenant_id != ti)
+        return nullopt; // Do nothing if the tenant has been replaced.
+
       service_data sd;
       try
       {
diff --git a/mod/mod-ci-github.hxx b/mod/mod-ci-github.hxx
index 9b9814e..104f889 100644
--- a/mod/mod-ci-github.hxx
+++ b/mod/mod-ci-github.hxx
@@ -42,26 +42,23 @@ namespace brep
     virtual const cli::options&
     cli_options () const {return options::ci_github::description ();}
 
-    virtual function<optional<string> (const string& tenant_id,
-                                       const tenant_service&)>
+    virtual function<optional<string> (const string&, const tenant_service&)>
     build_unloaded (const string& tenant_id,
                     tenant_service&&,
                     const diag_epilogue& log_writer) const noexcept override;
 
-    function<optional<string> (const string& tenant_id,
-                               const tenant_service&)>
+    function<optional<string> (const string&, const tenant_service&)>
     build_unloaded_pre_check (tenant_service&&,
                               service_data&&,
                               const diag_epilogue&) const noexcept;
 
-    function<optional<string> (const string& tenant_id,
-                               const tenant_service&)>
-    build_unloaded_load (tenant_service&&,
+    function<optional<string> (const string&, const tenant_service&)>
+    build_unloaded_load (const string& tenant_id,
+                         tenant_service&&,
                          service_data&&,
                          const diag_epilogue&) const noexcept;
 
-    virtual function<optional<string> (const string& tenant_id,
-                                       const tenant_service&)>
+    virtual function<optional<string> (const string&, const tenant_service&)>
     build_queued (const string& tenant_id,
                   const tenant_service&,
                   const vector<build>&,
@@ -69,15 +66,13 @@ namespace brep
                   const build_queued_hints&,
                   const diag_epilogue& log_writer) const noexcept override;
 
-    virtual function<optional<string> (const string& tenant_id,
-                                       const tenant_service&)>
+    virtual function<optional<string> (const string&, const tenant_service&)>
     build_building (const string& tenant_id,
                     const tenant_service&,
                     const build&,
                     const diag_epilogue& log_writer) const noexcept override;
 
-    virtual function<optional<string> (const string& tenant_id,
-                                       const tenant_service&)>
+    virtual function<optional<string> (const string&, const tenant_service&)>
     build_built (const string& tenant_id,
                  const tenant_service&,
                  const build&,
diff --git a/mod/module.cli b/mod/module.cli
index ca750e9..274ecd4 100644
--- a/mod/module.cli
+++ b/mod/module.cli
@@ -860,7 +860,8 @@ namespace brep
       {
         "<secret>",
         "The GitHub App's configured webhook secret. If not set, then the
-         GitHub CI service is disabled."
+         GitHub CI service is disabled. Note: make sure to choose a strong
+         (random) secret."
       }
 
       path ci-github-app-private-key