address comments

app_dart/lib/src/model/firestore/ci_staging.dart

@@ -88,7 +88,8 @@ class CiStaging extends Document {
     required String checkRun,
     required String conclusion,
   }) async {
-    final logCrumb = 'markConclusion(${slug.owner}_${slug.name}_${sha}_$stage, $checkRun, $conclusion)';
+    final changeCrumb = '${slug.owner}_${slug.name}_$sha';
+    final logCrumb = 'markConclusion(${changeCrumb}_$stage, $checkRun, $conclusion)';
 
     // Marking needs to happen while in a transaction to ensure `remaining` is
     // updated correctly. For that to happen correctly; we need to perform a
@@ -110,8 +111,10 @@ class CiStaging extends Document {
 
     var remaining = -1;
     var failed = -1;
+    var total = -1;
     bool valid = false;
     String? checkRunGuard;
+    String? recordedConclusion;
 
     late final Document doc;
 
@@ -146,9 +149,15 @@ class CiStaging extends Document {
       }
       failed = maybeFailed;
 
+      final maybeTotal = int.tryParse(fields[kTotalField]?.integerValue ?? '');
+      if (maybeTotal == null) {
+        throw '$logCrumb: missing field "$kTotalField" for $transaction / ${doc.fields}';
+      }
+      total = maybeTotal;
+
       // We will have check_runs scheduled after the engine was built successfully, so missing the checkRun field
       // is an OK response to have. All fields should have been written at creation time.
-      final recordedConclusion = fields[checkRun]?.stringValue;
+      recordedConclusion = fields[checkRun]?.stringValue;
       if (recordedConclusion == null) {
         log.info('$logCrumb: $checkRun not present in doc for $transaction / $doc');
         await docRes.rollback(RollbackRequest(transaction: transaction), kDatabase);
@@ -157,6 +166,8 @@ class CiStaging extends Document {
           remaining: remaining,
           checkRunGuard: null,
           failed: failed,
+          summary: 'Check run "$checkRun" not present in $stage CI stage',
+          details: 'Change $changeCrumb',
         );
       }
 
@@ -207,7 +218,7 @@ class CiStaging extends Document {
       fields[checkRun] = Value(stringValue: conclusion);
       fields[kRemainingField] = Value(integerValue: '$remaining');
       fields[kFailedField] = Value(integerValue: '$failed');
-    } on DetailedApiRequestError catch (e) {
+    } on DetailedApiRequestError catch (e, stack) {
       if (e.status == 404) {
         // An attempt to read a document not in firestore should not be retried.
         log.info('$logCrumb: staging document not found for $transaction');
@@ -217,6 +228,15 @@ class CiStaging extends Document {
           remaining: -1,
           checkRunGuard: null,
           failed: failed,
+          summary: 'Internal server error',
+          details: '''
+Staging document not found for CI stage "$stage" for $changeCrumb. Got 404 from
+Firestore.
+
+Error:
+${e.toString()}
+$stack
+''',
         );
       }
       // All other errors should bubble up and be retried.
@@ -239,6 +259,15 @@ class CiStaging extends Document {
       remaining: remaining,
       checkRunGuard: checkRunGuard,
       failed: failed,
+      summary: valid ? 'All tests passed' : 'Not a valid state transition for $checkRun',
+      details: valid
+          ? '''
+For CI stage $stage:
+  Total check runs scheduled: $total
+  Pending: $remaining
+  Failed: $failed
+'''
+          : 'Attempted to transition the state of check run $checkRun from "$recordedConclusion" to "$conclusion".',
     );
   }
 
@@ -343,12 +372,16 @@ class StagingConclusion {
   final int remaining;
   final String? checkRunGuard;
   final int failed;
+  final String summary;
+  final String details;
 
   const StagingConclusion({
     required this.result,
     required this.remaining,
     required this.checkRunGuard,
     required this.failed,
+    required this.summary,
+    required this.details,
   });
 
   bool get isOk => result == StagingConclusionResult.ok;
@@ -366,8 +399,21 @@ class StagingConclusion {
           other.result == result &&
           other.remaining == remaining &&
           other.checkRunGuard == checkRunGuard &&
-          other.failed == failed);
+          other.failed == failed &&
+          other.summary == summary &&
+          other.details == details);
+
+  @override
+  int get hashCode => Object.hashAll([
+        result,
+        remaining,
+        checkRunGuard,
+        failed,
+        summary,
+        details,
+      ]);
 
   @override
-  int get hashCode => Object.hashAll([result, remaining, checkRunGuard, failed]);
+  String toString() =>
+      'StagingConclusion("$result", "$remaining", "$checkRunGuard", "$failed", "$summary", "$details")';
 }

app_dart/lib/src/service/scheduler.dart

@@ -464,7 +464,9 @@ class Scheduler {
     // Update validate ci.yaml check
     await closeCiYamlCheckRun('PR ${pullRequest.number}', exception, slug, ciValidationCheckRun);
 
-    // The 'lock' will be unlocked later in processCheckRunCompletion after all engine builds are processed.
+    // Normally the lock stays pending until the PR is ready to be enqueued, but
+    // there are situations (see code above) when it needs to be unlocked
+    // immediately.
     if (unlockMergeGroup) {
       await unlockMergeQueueGuard(slug, pullRequest.head!.sha!, lock);
     }
@@ -721,14 +723,25 @@ class Scheduler {
   ///
   /// This removes the merge group from the merge queue without landing it. The
   /// corresponding pull request will have to be fixed and re-enqueued again.
-  Future<void> failGuardForMergeGroup(RepositorySlug slug, String headSha, CheckRun lock) async {
+  Future<void> failGuardForMergeGroup(
+    RepositorySlug slug,
+    String headSha,
+    String summary,
+    String details,
+    CheckRun lock,
+  ) async {
     log.info('Failing merge group guard for merge group $headSha in $slug');
     await githubChecksService.githubChecksUtil.updateCheckRun(
       config,
       slug,
       lock,
       status: CheckRunStatus.completed,
       conclusion: CheckRunConclusion.failure,
+      output: CheckRunOutput(
+        title: Config.kCiYamlCheckName,
+        summary: summary,
+        text: details,
+      ),
     );
   }
 
@@ -931,7 +944,13 @@ class Scheduler {
       // will hold up all other PRs that are trying to land.
       if (isMergeGroup) {
         final guard = checkRunFromString(stagingConclusion.checkRunGuard!);
-        await failGuardForMergeGroup(slug, sha, guard);
+        await failGuardForMergeGroup(
+          slug,
+          sha,
+          stagingConclusion.summary,
+          stagingConclusion.details,
+          guard,
+        );
       }
       return false;
     }
@@ -951,7 +970,13 @@ class Scheduler {
       //   let the author sort it out.
       if (isMergeGroup) {
         final guard = checkRunFromString(stagingConclusion.checkRunGuard!);
-        await failGuardForMergeGroup(slug, sha, guard);
+        await failGuardForMergeGroup(
+          slug,
+          sha,
+          stagingConclusion.summary,
+          stagingConclusion.details,
+          guard,
+        );
       }
       return true;
     }
@@ -987,7 +1012,10 @@ class Scheduler {
   /// Whether the [checkRunEvent] is for a merge group (rather than a pull request).
   bool detectMergeGroup(cocoon_checks.CheckRun checkRun) {
     final headBranch = checkRun.checkSuite?.headBranch;
-    return headBranch?.startsWith('gh-readonly-queue/') ?? false;
+    if (headBranch == null) {
+      return false;
+    }
+    return tryParseGitHubMergeQueueBranch(headBranch).parsed;
   }
 
   Future<void> _closeSuccessfulEngineBuildStage({
@@ -1094,7 +1122,6 @@ class Scheduler {
       throw 'No PR found matching this check_run($id, $name)';
     }
 
-    Object? exception;
     try {
       // Both the author and label should be checked to make sure that no one is
       // attempting to get a pull request without check through.
@@ -1127,18 +1154,14 @@ class Scheduler {
         error,
         backtrace,
       );
-      exception = error;
+      rethrow;
     } catch (error, backtrace) {
       log.warning(
         '$logCrumb: Exception encountered when scheduling presubmit targets for ${pullRequest.number}',
         error,
         backtrace,
       );
-      exception = error;
-    }
-
-    if (exception != null) {
-      throw exception;
+      rethrow;
     }
   }