fix: update presubmit check handling and improve guard failure logic

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

@@ -233,8 +233,12 @@ final class PresubmitCheck extends AppDocument<PresubmitCheck> {
     fields[fieldEndTime] = endTime.toValue();
   }
 
-  set summary(String summary) {
-    fields[fieldSummary] = summary.toValue();
+  set summary(String? summary) {
+    if (summary == null) {
+      fields.remove(fieldSummary);
+    } else {
+      fields[fieldSummary] = summary.toValue();
+    }
   }
 
   void updateFromBuild(bbv2.Build build) {

app_dart/lib/src/service/firestore/unified_check_run.dart

@@ -76,66 +76,6 @@ final class UnifiedCheckRun {
     }
   }
 
-  /// Initializes a new document for the given [tasks] in Firestore so that stage-tracking can succeed.
-  ///
-  /// The list of tasks will be written as fields of a document with additional fields for tracking the creationTime
-  /// number of tasks, remaining count. It is required to include [commitSha] as a json encoded [CheckRun] as this
-  /// will be used to unlock any check runs blocking progress.
-  ///
-  /// Returns the created document or throws an error.
-  static Future<Document> initializePresubmitGuardDocument({
-    required FirestoreService firestoreService,
-
-    required RepositorySlug slug,
-    required int pullRequestId,
-    required CheckRun checkRun,
-    required CiStage stage,
-    required String commitSha,
-    required int creationTime,
-    required String author,
-    required List<String> tasks,
-  }) async {
-    final buildCount = tasks.length;
-    final logCrumb =
-        'initializeDocument(${slug.owner}_${slug.name}_${pullRequestId}_${checkRun.id}_$stage, $buildCount builds)';
-
-    final presubmitGuard = PresubmitGuard(
-      checkRun: checkRun,
-      commitSha: commitSha,
-      slug: slug,
-      pullRequestId: pullRequestId,
-      stage: stage,
-      creationTime: creationTime,
-      author: author,
-      remainingBuilds: buildCount,
-      failedBuilds: 0,
-      builds: {for (final task in tasks) task: TaskStatus.waitingForBackfill},
-    );
-
-    final presubmitGuardId = PresubmitGuard.documentIdFor(
-      slug: slug,
-      pullRequestId: pullRequestId,
-      checkRunId: checkRun.id!,
-      stage: stage,
-    );
-
-    try {
-      // Calling createDocument multiple times for the same documentId will return a 409 - ALREADY_EXISTS error;
-      // this is good because it means we don't have to do any transactions.
-      // curl -X POST -H "Content-Type: application/json" -H "Authorization: Bearer <TOKEN>" "https://firestore.googleapis.com/v1beta1/projects/flutter-dashboard/databases/cocoon/documents/presubmit_guard?documentId=foo_bar_baz" -d '{"fields": {"test": {"stringValue": "baz"}}}'
-      final newDoc = await firestoreService.createDocument(
-        presubmitGuard,
-        collectionId: presubmitGuard.runtimeMetadata.collectionId,
-        documentId: presubmitGuardId.documentId,
-      );
-      log.info('$logCrumb: document created');
-      return newDoc;
-    } catch (e) {
-      log.warn('$logCrumb: failed to create document', e);
-      rethrow;
-    }
-  }
-
   /// Returns _all_ checks running against the specified github [checkRunId].
   static Future<List<PresubmitCheck>> queryAllPresubmitChecksForGuard({
     required FirestoreService firestoreService,
@@ -248,9 +188,9 @@ final class UnifiedCheckRun {
         await firestoreService.rollback(transaction);
         return PresubmitGuardConclusion(
           result: PresubmitGuardConclusionResult.missing,
-          remaining: presubmitGuard.remainingBuilds ?? -1,
+          remaining: presubmitGuard.remainingBuilds!,
           checkRunGuard: presubmitGuard.checkRunJson,
-          failed: presubmitGuard.failedBuilds ?? -1,
+          failed: presubmitGuard.failedBuilds!,
           summary:
               'Check run "${state.buildName}" not present in ${guardId.stage} CI stage',
           details: 'Change $changeCrumb',
@@ -271,11 +211,24 @@ final class UnifiedCheckRun {
       remaining = presubmitGuard.remainingBuilds!;
       failed = presubmitGuard.failedBuilds!;
       final builds = presubmitGuard.builds;
+      var status = builds?[state.buildName]!;
 
-      // If build is in progress, we should only update appropriate checks with
-      // that [TaskStatus]
-      if (state.status.isBuildInProgress) {
+      // If build is waiting for backfill, that means its initiated by github
+      // or re-run. So no processing needed, we should only update appropriate
+      // checks with that [TaskStatus]
+      if (state.status == TaskStatus.waitingForBackfill) {
+        status = state.status;
+        valid = true;
+        // If build is in progress, we should update apropriate checks with start
+        // time and their status to that [TaskStatus] only if the build is not
+        // completed.
+      } else if (state.status == TaskStatus.inProgress) {
         presubmitCheck.startTime = state.startTime!;
+        // If the build is not completed, update the status.
+        if (!status!.isBuildCompleted) {
+          status = state.status;
+        }
+        valid = true;
       } else {
         // "remaining" should go down if buildSuccessed of any attempt
         // or buildFailed a first attempt.
@@ -284,35 +237,35 @@ final class UnifiedCheckRun {
         //   attemptNumber = 1 && buildFailed: up (+1)
         // So if the test existed and either remaining or failed_count is changed;
         // the response is valid.
-
-        if (state.status.isBuildSuccessed ||
-            state.attemptNumber == 1 && state.status.isBuildFailed) {
+        status = state.status;
+        if (status.isBuildSuccessed) {
           // Guard against going negative and log enough info so we can debug.
           if (remaining == 0) {
             throw '$logCrumb: field "${PresubmitGuard.fieldRemainingBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}';
           }
           remaining = remaining - 1;
-          valid = true;
-        }
 
-        // Only rollback the "failed" counter if this is a successful test run,
-        // i.e. the test failed, the user requested a rerun, and now it passes.
-        if (state.attemptNumber > 1 && state.status.isBuildSuccessed) {
-          log.info(
-            '$logCrumb: conclusion flipped to positive - assuming test was re-run',
-          );
-          if (failed == 0) {
-            throw '$logCrumb: field "${PresubmitGuard.fieldFailedBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}';
+          // Only rollback the "failed" counter if this is a successful test run,
+          // i.e. the test failed, the user requested a rerun, and now it passes.
+          if (state.attemptNumber > 1) {
+            log.info(
+              '$logCrumb: conclusion flipped to positive - assuming test was re-run',
+            );
+            if (failed == 0) {
+              throw '$logCrumb: field "${PresubmitGuard.fieldFailedBuilds}" is already zero for $transaction / ${presubmitGuardDocument.fields}';
+            }
+            failed = failed - 1;
           }
           valid = true;
-          failed = failed - 1;
         }
 
         // Only increment the "failed" counter if the conclusion failed for the first attempt.
-        if (state.attemptNumber == 1 && state.status.isBuildFailed) {
-          log.info('$logCrumb: test failed');
+        if (status.isBuildFailed) {
+          if (state.attemptNumber == 1) {
+            log.info('$logCrumb: test failed');
+            failed = failed + 1;
+          }
           valid = true;
-          failed = failed + 1;
         }
 
         // All checks pass. "valid" is only set to true if there was a change in either the remaining or failed count.
@@ -322,10 +275,11 @@ final class UnifiedCheckRun {
         presubmitGuard.remainingBuilds = remaining;
         presubmitGuard.failedBuilds = failed;
         presubmitCheck.endTime = state.endTime!;
+        presubmitCheck.summary = state.summary;
       }
-      builds![state.buildName] = state.status;
+      builds![state.buildName] = status;
       presubmitGuard.builds = builds;
-      presubmitCheck.status = state.status;
+      presubmitCheck.status = status;
     } on DetailedApiRequestError catch (e, stack) {
       if (e.status == 404) {
         // An attempt to read a document not in firestore should not be retried.

app_dart/lib/src/service/scheduler.dart

@@ -671,6 +671,7 @@ class Scheduler {
       // only required GitHub check.
       await failGuardForMergeGroup(
         slug,
+        lock,
         headSha,
         'Failed to schedule checks for merge group',
         '''
@@ -679,7 +680,7 @@ $logCrumb
 ERROR: $e
 $s
 ''',
-        lock,
+        null,
       );
     }
   }
@@ -854,10 +855,11 @@ $s
   /// corresponding pull request will have to be fixed and re-enqueued again.
   Future<void> failGuardForMergeGroup(
     RepositorySlug slug,
+    CheckRun lock,
     String headSha,
     String summary,
     String details,
-    CheckRun lock,
+    String? detailsUrl,
   ) async {
     log.info('Failing merge group guard for merge group $headSha in $slug');
     await _githubChecksService.githubChecksUtil.updateCheckRun(
@@ -871,6 +873,7 @@ $s
         summary: summary,
         text: details,
       ),
+      detailsUrl: detailsUrl,
     );
   }
 
@@ -1018,15 +1021,32 @@ $s
         final guard = checkRunFromString(stagingConclusion.checkRunGuard!);
         await failGuardForMergeGroup(
           check.slug,
+          guard,
           check.sha,
           stagingConclusion.summary,
           stagingConclusion.details,
-          guard,
+          null,
         );
       }
       return false;
     }
 
+    // If the number of failed checks is equal to the number of remaining checks, then all remaining checks have failed.
+    if (check.isUnifiedCheckRun &&
+        stagingConclusion.failed > 0 &&
+        stagingConclusion.failed == stagingConclusion.remaining) {
+      final guard = checkRunFromString(stagingConclusion.checkRunGuard!);
+      await failGuardForMergeGroup(
+        check.slug,
+        guard,
+        check.sha,
+        kMergeQueueLockDescription,
+        'For CI stage ${check.stage} ${stagingConclusion.failed} checks failed',
+        'https://flutter-dashboard.appspot.com/repo/${check.slug.name}/checkrun/${check.guardId}',
+      );
+      return true;
+    }
+
     // Are there tests remaining? Keep waiting.
     if (stagingConclusion.isPending) {
       log.info(
@@ -1047,10 +1067,11 @@ $s
         final guard = checkRunFromString(stagingConclusion.checkRunGuard!);
         await failGuardForMergeGroup(
           check.slug,
+          guard,
           check.sha,
           stagingConclusion.summary,
           stagingConclusion.details,
-          guard,
+          null,
         );
       }
       return true;

app_dart/test/service/firestore/unified_check_run_test.dart

@@ -132,31 +132,36 @@ void main() {
         );
 
         // Initialize documents
-        await UnifiedCheckRun.initializePresubmitGuardDocument(
-          firestoreService: firestoreService,
+        final guard = PresubmitGuard(
+          checkRun: checkRun,
+          commitSha: sha,
           slug: slug,
           pullRequestId: 1,
-          checkRun: checkRun,
           stage: CiStage.fusionEngineBuild,
-          commitSha: sha,
           creationTime: 1000,
           author: 'dash',
-          tasks: ['linux', 'mac'],
+          builds: {
+            for (final task in ['linux', 'mac'])
+              task: TaskStatus.waitingForBackfill,
+          },
+          remainingBuilds: 2,
+          failedBuilds: 0,
         );
 
         final check1 = PresubmitCheck.init(
           buildName: 'linux',
-          checkRunId: 123,
+          checkRunId: guardId.checkRunId,
           creationTime: 1000,
         );
         final check2 = PresubmitCheck.init(
           buildName: 'mac',
-          checkRunId: 123,
+          checkRunId: guardId.checkRunId,
           creationTime: 1000,
         );
 
         await firestoreService.writeViaTransaction(
           documentsToWrites([
+            Document(name: guard.name, fields: guard.fields),
             Document(name: check1.name, fields: check1.fields),
             Document(name: check2.name, fields: check2.fields),
           ], exists: false),
@@ -262,7 +267,7 @@ void main() {
         );
 
         expect(result.result, PresubmitGuardConclusionResult.ok);
-        expect(result.remaining, 1);
+        expect(result.remaining, 2);
         expect(result.failed, 1);
       });