Minor tweaks to state transition code

2 files changed, 33 insertions, 27 deletions

diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index 7da694b..06ea7b2 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -1370,7 +1370,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).
+  //    conclusion (which would be pretty confusing for the user). @@@ This
+  //    can/will happen on check run rebuild. Distinguish between internal
+  //    and external rebuilds?
   //
   //    So, for GitHub notifications, we only have the following linear
   //    transition sequence:
@@ -1476,7 +1478,7 @@ namespace brep
     //
     for (const build& b: builds)
     {
-      string bid (gh_check_run_name (b)); // Full build ID.
+      string bid (gh_check_run_name (b)); // Full build id.
 
       if (const check_run* scr = sd.find_check_run (bid))
       {
@@ -1498,6 +1500,8 @@ namespace brep
         else
         {
           // Ignore interrupted.
+          //
+          assert (*istate == build_state::building);
         }
       }
       else
@@ -1542,7 +1546,7 @@ namespace brep
     //
     if (iat != nullptr)
     {
-      // Create a check_run for each build.
+      // Create a check_run for each build as a single request.
       //
       if (gq_create_check_runs (error,
                                 crs,
@@ -1552,6 +1556,8 @@ namespace brep
       {
         for (const check_run& cr: crs)
         {
+          // We can only create a check run in the queued state.
+          //
           assert (cr.state == build_state::queued);
           l3 ([&]{trace << "created check_run { " << cr << " }";});
         }
@@ -1626,13 +1632,12 @@ namespace brep
     }
 
     optional<check_run> cr; // Updated check run.
-    string bid (gh_check_run_name (b)); // Full Build ID.
+    string bid (gh_check_run_name (b)); // Full build id.
 
     if (check_run* scr = sd.find_check_run (bid)) // Stored check run.
     {
       // Update the check run if it exists on GitHub and the queued
-      // notification succeeded and updated the service data, otherwise do
-      // nothing.
+      // notification updated the service data, otherwise do nothing.
       //
       if (scr->state == build_state::queued)
       {
@@ -1697,12 +1702,10 @@ namespace brep
         if (cr->state == build_state::built)
         {
           warn << "check run " << bid << ": already in built state on GitHub";
-
           return nullptr;
         }
 
         assert (cr->state == build_state::building);
-
         l3 ([&]{trace << "updated check_run { " << *cr << " }";});
       }
     }
@@ -1772,13 +1775,16 @@ namespace brep
       return nullptr;
     }
 
-    // Absent if have any unbuilt check runs.
+    // Here we need to update the state of this check run and, if there are no
+    // more unbuilt ones, update the synthetic conclusion check run.
+    //
+    // Absent means we still have unbuilt check runs.
     //
     optional<result_status> conclusion (*b.status);
 
     check_run cr; // Updated check run.
     {
-      string bid (gh_check_run_name (b)); // Full Build ID.
+      string bid (gh_check_run_name (b)); // Full build id.
 
       optional<check_run> scr;
       for (check_run& cr: sd.check_runs)
@@ -1792,11 +1798,10 @@ namespace brep
         {
           if (cr.state == build_state::built)
           {
+            assert (cr.status);
+
             if (conclusion)
-            {
-              assert (cr.status);
               *conclusion |= *cr.status;
-            }
           }
           else
             conclusion = nullopt;
@@ -1826,6 +1831,9 @@ namespace brep
         warn << "check run " << bid << ": out of order built notification; "
              << "no check run state in service data";
 
+        // Note that we have no hints here and so have to use the full build
+        // id for name.
+        //
         cr.build_id = move (bid);
         cr.name = cr.build_id;
       }
@@ -1955,10 +1963,10 @@ namespace brep
         sm = os.str ();
       }
 
-      gq_built_result br (gh_to_conclusion (*b.status, sd.warning_success),
-                          circle (*b.status) + ' ' +
-                            ucase (to_string (*b.status)),
-                          move (sm));
+      gq_built_result br (
+        gh_to_conclusion (*b.status, sd.warning_success),
+        circle (*b.status) + ' ' + ucase (to_string (*b.status)),
+        move (sm));
 
       if (cr.node_id)
       {
@@ -1974,7 +1982,6 @@ namespace brep
                                  move (br)))
         {
           assert (cr.state == build_state::built);
-
           l3 ([&]{trace << "updated check_run { " << cr << " }";});
         }
       }
@@ -1983,7 +1990,7 @@ namespace brep
         // Create new check run.
         //
         // Note that we don't have build hints so will be creating this check
-        // run with the full build ID as name. In the unlikely event that an
+        // run with the full build id as name. In the unlikely event that an
         // out of order build_queued() were to run before we've saved this
         // check run to the service data it will create another check run with
         // the shortened name which will never get to the built state.
@@ -1998,7 +2005,6 @@ namespace brep
                                  move (br)))
         {
           assert (cr.state == build_state::built);
-
           l3 ([&]{trace << "created check_run { " << cr << " }";});
         }
       }
@@ -2042,8 +2048,6 @@ namespace brep
         {
           assert (sd.conclusion_node_id);
 
-          // Update the conclusion check run with success.
-          //
           result_status rs (*conclusion);
 
           optional<gq_built_result> br (
@@ -2068,8 +2072,7 @@ namespace brep
                                    move (br)))
           {
             assert (cr.state == build_state::built);
-
-            l3 ([&]{trace << "updated check_run { " << cr << " }";});
+            l3 ([&]{trace << "updated conclusion check_run { " << cr << " }";});
           }
           else
           {
diff --git a/mod/tenant-service.hxx b/mod/tenant-service.hxx
index 6a982ea..8ba199a 100644
--- a/mod/tenant-service.hxx
+++ b/mod/tenant-service.hxx
@@ -50,7 +50,9 @@ namespace brep
   // sense for the implementation to protect against overwriting later states
   // with earlier. For example, if it's possible to place a condition on a
   // notification, it makes sense to only set the state to queued if none of
-  // the later states (e.g., building) are already in effect.
+  // the later states (e.g., building) are already in effect. See also
+  // ci_start::rebuild() for additional details on the build->queued
+  // transition.
   //
   // Note also that it's possible for the build to get deleted at any stage
   // without any further notifications. This can happen, for example, due to
@@ -131,8 +133,9 @@ namespace brep
   // returned callback should be NULL).
   //
   // Note: make sure the implementation of this notification does not take
-  // too long (currently 40 seconds) to avoid nested notifications. Note
-  // also that the first notification is delayed (currently 10 seconds).
+  // longer than the notification_interval argument of ci_start::create() to
+  // avoid nested notifications. The first notification can be delayed with
+  // the notify_delay argument.
   //
   class tenant_service_build_unloaded: public virtual tenant_service_base
   {