Review

6 files changed, 69 insertions, 100 deletions

diff --git a/mod/mod-ci-github-gh.cxx b/mod/mod-ci-github-gh.cxx
index 55cd663..fc5cf82 100644
--- a/mod/mod-ci-github-gh.cxx
+++ b/mod/mod-ci-github-gh.cxx
@@ -19,7 +19,7 @@ namespace brep
   // Return the GitHub check run status corresponding to a build_state.
   //
   string
-  gh_to_status (build_state st) noexcept
+  gh_to_status (build_state st)
   {
     // Just return by value (small string optimization).
     //
@@ -593,6 +593,14 @@ namespace brep
                       "invalid IAT expires_at value '" + v +
                         "': " + e.what ());
         }
+        catch (const system_error& e)
+        {
+          // Translate for simplicity.
+          //
+          throw_json (p,
+                      "unable to convert IAT expires_at value '" + v +
+                      "': " + e.what ());
+        }
       }
       else p.next_expect_value_skip ();
     }
@@ -619,37 +627,17 @@ namespace brep
   string
   gh_to_iso8601 (timestamp t)
   {
-    try
-    {
-      return butl::to_string (t,
-                              "%Y-%m-%dT%TZ",
-                              false /* special */,
-                              false /* local */);
-    }
-    catch (const system_error& e)
-    {
-      throw runtime_error (
-        string ("failed to convert timestamp to ISO 8601 string: ") +
-        e.what ());
-    }
+    return butl::to_string (t,
+                            "%Y-%m-%dT%TZ",
+                            false /* special */,
+                            false /* local */);
   }
 
   timestamp
   gh_from_iso8601 (const string& s)
   {
-    try
-    {
-      // @@ TMP butl::from_string()'s comment says it also throws
-      //    invalid_argument but that seems to be false.
-      //
-      return butl::from_string (s.c_str (),
-                                "%Y-%m-%dT%TZ",
-                                false /* local */);
-    }
-    catch (const system_error& e)
-    {
-      throw invalid_argument ("invalid ISO 8601 timestamp value '" + s +
-                              "': " + e.what ());
-    }
+    return butl::from_string (s.c_str (),
+                              "%Y-%m-%dT%TZ",
+                              false /* local */);
   }
 }
diff --git a/mod/mod-ci-github-gh.hxx b/mod/mod-ci-github-gh.hxx
index 5fff2bc..eeda871 100644
--- a/mod/mod-ci-github-gh.hxx
+++ b/mod/mod-ci-github-gh.hxx
@@ -107,7 +107,7 @@ namespace brep
   // Return the GitHub check run status corresponding to a build_state.
   //
   string
-  gh_to_status (build_state st) noexcept;
+  gh_to_status (build_state);
 
   // Return the build_state corresponding to a GitHub check run status
   // string. Throw invalid_argument if the passed status was invalid.
@@ -214,12 +214,14 @@ namespace brep
     gh_installation_access_token () = default;
   };
 
-  // Throw runtime_error if the conversion fails.
+  // Throw system_error if the conversion fails due to underlying operating
+  // system errors.
   //
   string
   gh_to_iso8601 (timestamp);
 
-  // Throw invalid_argument if the conversion fails.
+  // Throw invalid_argument if the conversion fails due to the invalid
+  // argument and system_error if due to underlying operating system errors.
   //
   timestamp
   gh_from_iso8601 (const string&);
diff --git a/mod/mod-ci-github-gq.cxx b/mod/mod-ci-github-gq.cxx
index a0a0d6b..7b7e464 100644
--- a/mod/mod-ci-github-gq.cxx
+++ b/mod/mod-ci-github-gq.cxx
@@ -521,9 +521,11 @@ namespace brep
         os                                                        << '\n';
         os << "  startedAt: " << gq_str (gh_to_iso8601 (*sa));
       }
-      catch (const runtime_error& e)
+      catch (const system_error& e)
       {
-        throw invalid_argument ("invalid started_at value " +
+        // Translate for simplicity.
+        //
+        throw invalid_argument ("unable to convert started_at value " +
                                 to_string (system_clock::to_time_t (*sa)) +
                                 ": " + e.what ());
       }
diff --git a/mod/mod-ci-github-gq.hxx b/mod/mod-ci-github-gq.hxx
index 86ab859..50950d4 100644
--- a/mod/mod-ci-github-gq.hxx
+++ b/mod/mod-ci-github-gq.hxx
@@ -24,7 +24,8 @@ namespace brep
   // `check_runs` with the new data (node id and state_synced). Return false
   // and issue diagnostics if the request failed.
   //
-  // Throw invalid_argument.
+  // Throw invalid_argument if the passed data is invalid, missing, or
+  // inconsistent.
   //
   // Note that creating a check_run named `foo` will effectively replace any
   // existing check_runs with that name. They will still exist on the GitHub
@@ -42,7 +43,8 @@ namespace brep
   // data (node id, state, and state_synced). Return false and issue
   // diagnostics if the request failed.
   //
-  // Throw invalid_argument.
+  // Throw invalid_argument if the passed data is invalid, missing, or
+  // inconsistent.
   //
   // If the details_url is absent GitHub will use the app's homepage.
   //
@@ -70,7 +72,8 @@ namespace brep
   // Update a check run on GitHub. Update `cr` with the new data (state and
   // state_synced). Return false and issue diagnostics if the request failed.
   //
-  // Throw invalid_argument.
+  // Throw invalid_argument if the passed data is invalid, missing, or
+  // inconsistent.
   //
   // Note that GitHub allows any state transitions except from built (but
   // built to built is allowed). The latter case is signalled by setting the
diff --git a/mod/mod-ci-github-service-data.cxx b/mod/mod-ci-github-service-data.cxx
index d3071b2..31a556d 100644
--- a/mod/mod-ci-github-service-data.cxx
+++ b/mod/mod-ci-github-service-data.cxx
@@ -220,9 +220,11 @@ namespace brep
       {
         v = gh_to_iso8601 (installation_access.expires_at);
       }
-      catch (const runtime_error& e)
+      catch (const system_error& e)
       {
-        throw invalid_argument ("invalid IAT expires_at value " +
+        // Translate for simplicity.
+        //
+        throw invalid_argument ("unable to convert IAT expires_at value " +
                                 to_string (system_clock::to_time_t (
                                   installation_access.expires_at)));
       }
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index efe61e3..5d80ab6 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -693,8 +693,7 @@ namespace brep
 
   // Create a gq_built_result.
   //
-  // Throw invalid_argument in case of invalid result_status (highly
-  // unlikely).
+  // Throw invalid_argument in case of invalid result_status.
   //
   static gq_built_result
   make_built_result (result_status rs, bool warning_success, string message)
@@ -1354,12 +1353,11 @@ namespace brep
   {
     // NOTE: this function is noexcept and should not throw.
     //
-    //       In a few places where invalid_argument is highly unlikely to be
-    //       thrown and/or would indicate that things are seriously broken we
-    //       let it propagate to the function catch block where the pre-check
-    //       tenant will be canceled otherwise we could end up in an infinite
-    //       loop (because the problematic arguments won't be changing).
-    //
+    // In a few places where invalid_argument is unlikely to be thrown and/or
+    // would indicate that things are seriously broken we let it propagate to
+    // the function catch block where the pre-check tenant will be canceled
+    // (otherwise we could end up in an infinite loop, e.g., because the
+    // problematic arguments won't change).
 
     NOTIFICATION_DIAG (log_writer);
 
@@ -1533,26 +1531,9 @@ namespace brep
     try
     {
       if (cancel (error, warn, verb_ ? &trace : nullptr,
-                   *build_db_, retry_, ts.type, ts.id))
-        l3 ([&]{trace << "canceled pre-check tenant " << ts.id;});
-    }
-    catch (const runtime_error& e) // Database retries exhausted.
-    {
-      l3 ([&]{trace << "failed to cancel pre-check tenant " << ts.id << ": "
-                    << e.what ();});
-    }
-
-    return nullptr;
-  }
-  catch (...)
-  {
-    NOTIFICATION_DIAG (log_writer);
-    error << "pull request " << *sd.pr_node_id << ": unhandled exception";
-
-    try
-    {
-      if (cancel (error, warn, verb_ ? &trace : nullptr,
-                  *build_db_, retry_, ts.type, ts.id))
+                  *build_db_, retry_,
+                  ts.type,
+                  ts.id))
         l3 ([&]{trace << "canceled pre-check tenant " << ts.id;});
     }
     catch (const runtime_error& e) // Database retries exhausted.
@@ -1572,12 +1553,11 @@ namespace brep
   {
     // NOTE: this function is noexcept and should not throw.
     //
-    //       In a few places where invalid_argument is highly unlikely to be
-    //       thrown and/or would indicate that things are seriously broken we
-    //       let it propagate to the function catch block where the tenant
-    //       will be canceled otherwise we could end up in an infinite loop
-    //       (because the problematic arguments won't be changing).
-    //
+    // In a few places where invalid_argument is unlikely to be thrown and/or
+    // would indicate that things are seriously broken we let it propagate to
+    // the function catch block where the tenant will be canceled (otherwise
+    // we could end up in an infinite loop, e.g., because the problematic
+    // arguments won't change).
 
     NOTIFICATION_DIAG (log_writer);
 
@@ -1688,16 +1668,24 @@ namespace brep
     //
     string conclusion_node_id; // Conclusion check run node ID.
 
-    if (auto cr = create_synthetic_cr (conclusion_check_run_name))
+    if (!sd.conclusion_node_id)
     {
-      l3 ([&]{trace << "created check_run { " << *cr << " }";});
+      if (auto cr = create_synthetic_cr (conclusion_check_run_name))
+      {
+        l3 ([&]{trace << "created check_run { " << *cr << " }";});
 
-      conclusion_node_id = move (*cr->node_id);
+        conclusion_node_id = move (*cr->node_id);
+      }
     }
 
+    const string& effective_conclusion_node_id (
+      sd.conclusion_node_id
+      ? *sd.conclusion_node_id
+      : conclusion_node_id);
+
     // Load the CI tenant if the conclusion check run was created.
     //
-    if (!conclusion_node_id.empty ())
+    if (!effective_conclusion_node_id.empty ())
     {
       string ru; // Repository URL.
 
@@ -1723,13 +1711,13 @@ namespace brep
         optional<start_result> r (load (error, warn, verb_ ? &trace : nullptr,
                                         *build_db_, retry_,
                                         move (ts),
-                                        move (rl)));
+                                        move (rl));
 
         if (!r || r->status != 200)
         {
           // Let unlikely invalid_argument propagate (see above).
           //
-          if (auto cr = update_synthetic_cr (conclusion_node_id,
+          if (auto cr = update_synthetic_cr (effective_conclusion_node_id,
                                              conclusion_check_run_name,
                                              result_status::error,
                                              to_check_run_summary (r)))
@@ -1751,10 +1739,13 @@ namespace brep
       catch (const runtime_error& e) // Database retries exhausted.
       {
         error << "failed to load CI tenant " << ts.id << ": " << e.what ();
+
+        // Fall through to retry on next call.
       }
     }
-    else if (!new_iat)
-      return nullptr; // Nothing to save (but retry on next call).
+
+    if (!new_iat && conclusion_node_id.empty ())
+      return nullptr; // Nothing to save (but potentially retry on next call).
 
     return [&error,
             iat = move (new_iat),
@@ -1807,25 +1798,6 @@ namespace brep
 
     return nullptr;
   }
-  catch (...)
-  {
-    NOTIFICATION_DIAG (log_writer);
-    error << "CI tenant " << ts.id << ": unhandled exception";
-
-    try
-    {
-      if (cancel (error, warn, verb_ ? &trace : nullptr,
-                  *build_db_, retry_, ts.type, ts.id))
-        l3 ([&]{trace << "canceled CI tenant " << ts.id;});
-    }
-    catch (const runtime_error& e) // Database retries exhausted.
-    {
-      l3 ([&]{trace << "failed to cancel CI tenant " << ts.id << ": "
-                    << e.what ();});
-    }
-
-    return nullptr;
-  }
 
   // Build state change notifications (see tenant-services.hxx for
   // background). Mapping our state transitions to GitHub pose multiple
@@ -2394,7 +2366,7 @@ namespace brep
         // Note: let all serialization exceptions propagate. The XML
         // serialization code can throw bad_alloc or xml::serialization in
         // case of I/O failures, but we're serializing to a string stream so
-        // both exceptions are highly unlikely.
+        // both exceptions are unlikely.
         //
         ostringstream os;
         xml::serializer s (os, "check_run_summary");