183144 Ensuring proper current_attempt tag is added when rescheduling try builds

analyze/pubspec.yaml

@@ -13,5 +13,6 @@ dependencies:
   platform: 3.1.6
 
 dev_dependencies:
+  dart_flutter_team_lints: ^3.5.2
   mockito: ^5.4.6
   test_api: ^0.7.7

app_dart/lib/src/model/common/failed_presubmit_checks.dart

@@ -5,6 +5,8 @@
 /// @docImport 'failed_presubmit_checks.dart';
 library;
 
+import 'package:collection/collection.dart';
+
 import 'package:github/github.dart';
 
 import '../firestore/base.dart';
@@ -15,12 +17,12 @@ import '../firestore/base.dart';
 class FailedChecksForRerun {
   final CheckRun checkRunGuard;
   final CiStage stage;
-  final List<String> checkNames;
+  final Map<String, int> checkRetries;
 
   const FailedChecksForRerun({
     required this.checkRunGuard,
     required this.stage,
-    required this.checkNames,
+    required this.checkRetries,
   });
 
   @override
@@ -29,12 +31,20 @@ class FailedChecksForRerun {
       (other is FailedChecksForRerun &&
           other.checkRunGuard == checkRunGuard &&
           other.stage == stage &&
-          other.checkNames == checkNames);
+          const DeepCollectionEquality().equals(
+            other.checkRetries,
+            checkRetries,
+          ));
 
   @override
-  int get hashCode => Object.hashAll([checkRunGuard, stage, checkNames]);
+  int get hashCode => Object.hashAll([
+    checkRunGuard,
+    stage,
+    ...checkRetries.keys,
+    ...checkRetries.values,
+  ]);
 
   @override
   String toString() =>
-      'FailedChecksForRerun("$checkRunGuard", "$stage", "$checkNames")';
+      'FailedChecksForRerun("$checkRunGuard", "$stage", "$checkRetries")';
 }

app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart

@@ -84,7 +84,7 @@ final class RerunAllFailedJobs extends ApiRequestHandler {
     );
 
     final failedTargets = targets
-        .where((target) => failedChecks.checkNames.contains(target.name))
+        .where((target) => failedChecks.checkRetries.containsKey(target.name))
         .toList();
 
     await _luciBuildService.scheduleTryBuilds(

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

@@ -134,13 +134,15 @@ final class UnifiedCheckRun {
       }
       latestGuard.builds = builds;
       final checks = <PresubmitCheck>[];
+      final checkRetries = <String, int>{};
       for (final buildName in failedBuildNames) {
         final latestCheck = await getLatestPresubmitCheck(
           firestoreService: firestoreService,
           checkRunId: guardCheckRunId,
           buildName: buildName,
           transaction: transaction,
         );
+        checkRetries[buildName] = (latestCheck?.attemptNumber ?? 0) + 1;
         checks.add(
           PresubmitCheck.init(
             buildName: buildName,
@@ -160,7 +162,7 @@ final class UnifiedCheckRun {
         );
         return FailedChecksForRerun(
           checkRunGuard: latestGuard.checkRun,
-          checkNames: failedBuildNames,
+          checkRetries: checkRetries,
           stage: latestGuard.stage,
         );
       } catch (e) {
@@ -234,7 +236,7 @@ final class UnifiedCheckRun {
       );
       return FailedChecksForRerun(
         checkRunGuard: guard.checkRun,
-        checkNames: [buildName],
+        checkRetries: {buildName: (latestCheck?.attemptNumber ?? 0) + 1},
         stage: guard.stage,
       );
     } catch (e) {

app_dart/lib/src/service/luci_build_service.dart

@@ -229,9 +229,43 @@ class LuciBuildService {
     required EngineArtifacts engineArtifacts,
     CheckRun? checkRunGuard,
     CiStage? stage,
+  }) async {
+    return _scheduleTryBuilds(
+      targets: {for (final target in targets) target: 1},
+      pullRequest: pullRequest,
+      engineArtifacts: engineArtifacts,
+      checkRunGuard: checkRunGuard,
+      stage: stage,
+    );
+  }
+
+  /// Reschedules presubmit [targets] on BuildBucket for [pullRequest] with attempt numbers.
+  Future<List<Target>> reScheduleTryBuilds({
+    required Map<Target, int> targets,
+    required PullRequest pullRequest,
+    required EngineArtifacts engineArtifacts,
+    required CheckRun checkRunGuard,
+    required CiStage stage,
+  }) async {
+    return _scheduleTryBuilds(
+      targets: targets,
+      pullRequest: pullRequest,
+      engineArtifacts: engineArtifacts,
+      checkRunGuard: checkRunGuard,
+      stage: stage,
+    );
+  }
+
+  /// Schedules presubmit [targets] on BuildBucket for [pullRequest].
+  Future<List<Target>> _scheduleTryBuilds({
+    required Map<Target, int> targets,
+    required PullRequest pullRequest,
+    required EngineArtifacts engineArtifacts,
+    CheckRun? checkRunGuard,
+    CiStage? stage,
   }) async {
     if (targets.isEmpty) {
-      return targets;
+      return targets.keys.toList();
     }
 
     final batchRequestList = <bbv2.BatchRequest_Request>[];
@@ -263,7 +297,7 @@ class LuciBuildService {
       checkRuns.add(checkRunGuard);
     }
 
-    for (var target in targets) {
+    for (final MapEntry(key: target, value: attemptNumber) in targets.entries) {
       // If the unified check run flow is disabled create individual check runs
       // for each target.
       if (!isUnifiedCheckRunFlow || checkRunGuard == null) {
@@ -324,7 +358,6 @@ class LuciBuildService {
       }
 
       final requestedDimensions = target.getDimensions();
-
       batchRequestList.add(
         bbv2.BatchRequest_Request(
           scheduleBuild: _createPresubmitScheduleBuild(
@@ -340,6 +373,8 @@ class LuciBuildService {
             tags: isUnifiedCheckRunFlow && checkRunGuard != null
                 ? BuildTags([
                     GuardCheckRunIdBuildTag(guardCheckRunId: checkRunGuard.id!),
+                    if (attemptNumber > 1)
+                      CurrentAttemptBuildTag(attemptNumber: attemptNumber),
                   ])
                 : BuildTags([
                     GitHubCheckRunIdBuildTag(checkRunId: userData.checkRunId!),
@@ -378,7 +413,7 @@ class LuciBuildService {
       );
     }
 
-    return targets;
+    return targets.keys.toList();
   }
 
   /// Cancels all the current builds on [pullRequest] with [reason].

app_dart/lib/src/service/scheduler.dart

@@ -1779,7 +1779,7 @@ $stacktrace
       guardCheckRunId: checkRunEvent.checkRun!.id!,
     );
 
-    if (failedChecks == null) {
+    if (failedChecks == null || failedChecks.checkRetries.isEmpty) {
       log.error('$logCrumb: No failed targets found');
       return const ProcessCheckRunResult.missingEntity(
         'No failed targets found',
@@ -1791,10 +1791,14 @@ $stacktrace
       pullRequest,
     );
 
-    final failedTargets = targets
-        .where((target) => failedChecks.checkNames.contains(target.name))
-        .toList();
-    if (failedTargets.length != failedChecks.checkNames.length) {
+    final checkRetries = <Target, int>{};
+    for (final target in targets) {
+      if (failedChecks.checkRetries.containsKey(target.name)) {
+        checkRetries[target] = failedChecks.checkRetries[target.name]!;
+      }
+    }
+
+    if (checkRetries.length != failedChecks.checkRetries.length) {
       log.error(
         '$logCrumb: Failed to find all failed targets in presubmit targets',
       );
@@ -1803,16 +1807,16 @@ $stacktrace
       );
     }
 
-    await _luciBuildService.scheduleTryBuilds(
-      targets: failedTargets,
+    await _luciBuildService.reScheduleTryBuilds(
+      targets: checkRetries,
       pullRequest: pullRequest,
       engineArtifacts: artifacts,
       checkRunGuard: failedChecks.checkRunGuard,
       stage: failedChecks.stage,
     );
 
     log.info(
-      '$logCrumb: Successfully rescheduled ${failedTargets.length} targets',
+      '$logCrumb: Successfully rescheduled ${checkRetries.length} targets',
     );
     return const ProcessCheckRunResult.success();
   }

app_dart/pubspec.yaml

@@ -63,7 +63,7 @@ dev_dependencies:
     path: ../packages/cocoon_integration_test
   cocoon_server_test:
     path: ../packages/cocoon_server_test
-  dart_flutter_team_lints: 3.5.2
+  dart_flutter_team_lints: ^3.5.2
   fake_async: ^1.3.3
   json_serializable: ^6.9.4
   mockito: ^5.4.6

app_dart/test/service/firestore/unified_check_run_test.dart

@@ -344,8 +344,8 @@ void main() {
           guardCheckRunId: 123,
         );
         expect(result, isNotNull);
-        expect(result!.checkNames, contains('linux'));
-        expect(result.checkNames, isNot(contains('mac')));
+        expect(result!.checkRetries, containsPair('linux', 2));
+        expect(result.checkRetries.keys, isNot(contains('mac')));
         expect(result.stage, CiStage.fusionTests);
 
         final guardDoc = await firestoreService.getDocument(
@@ -424,8 +424,8 @@ void main() {
           guardCheckRunId: 123,
         );
         expect(result, isNotNull);
-        expect(result!.checkNames, contains('linux'));
-        expect(result.checkNames, isNot(contains('mac')));
+        expect(result!.checkRetries, containsPair('linux', 2));
+        expect(result.checkRetries.keys, isNot(contains('mac')));
         expect(result.stage, CiStage.fusionTests);
 
         final guardDoc = await firestoreService.getDocument(

app_dart/test/service/scheduler_test.dart

@@ -1249,11 +1249,11 @@ targets:
           'installation': {'id': 123},
         });
 
-        // Ensure scheduleTryBuilds returns something to avoid null errors?
-        // MockLuciBuildService.scheduleTryBuilds is mocked using Mockito?
+        // Ensure reScheduleTryBuilds returns something to avoid null errors?
+        // MockLuciBuildService.reScheduleTryBuilds is mocked using Mockito?
         // In this file `MockLuciBuildService` extends `Mock` implements `LuciBuildService`.
         when(
-          mockLuciBuildService.scheduleTryBuilds(
+          mockLuciBuildService.reScheduleTryBuilds(
             targets: anyNamed('targets'),
             pullRequest: anyNamed('pullRequest'),
             engineArtifacts: anyNamed('engineArtifacts'),
@@ -1268,9 +1268,11 @@ targets:
         );
 
         verify(
-          mockLuciBuildService.scheduleTryBuilds(
+          mockLuciBuildService.reScheduleTryBuilds(
             targets: argThat(
-              contains(predicate<Target>((t) => t.name == 'Linux A')),
+              predicate<Map<Target, int>>((map) {
+                return map.keys.any((t) => t.name == 'Linux A');
+              }),
               named: 'targets',
             ),
             pullRequest: pullRequest,
@@ -1391,11 +1393,11 @@ targets:
           'installation': {'id': 123},
         });
 
-        // Ensure scheduleTryBuilds returns something to avoid null errors?
-        // MockLuciBuildService.scheduleTryBuilds is mocked using Mockito?
+        // Ensure reScheduleTryBuilds returns something to avoid null errors?
+        // MockLuciBuildService.reScheduleTryBuilds is mocked using Mockito?
         // In this file `MockLuciBuildService` extends `Mock` implements `LuciBuildService`.
         when(
-          mockLuciBuildService.scheduleTryBuilds(
+          mockLuciBuildService.reScheduleTryBuilds(
             targets: anyNamed('targets'),
             pullRequest: anyNamed('pullRequest'),
             engineArtifacts: anyNamed('engineArtifacts'),
@@ -1410,9 +1412,11 @@ targets:
         );
 
         verify(
-          mockLuciBuildService.scheduleTryBuilds(
+          mockLuciBuildService.reScheduleTryBuilds(
             targets: argThat(
-              contains(predicate<Target>((t) => t.name == 'Linux A')),
+              predicate<Map<Target, int>>((map) {
+                return map.keys.any((t) => t.name == 'Linux A');
+              }),
               named: 'targets',
             ),
             pullRequest: pullRequest,

cipd_packages/device_doctor/pubspec.yaml

@@ -18,6 +18,7 @@ dependencies:
 
 dev_dependencies:
   build_runner: ^2.4.13
+  dart_flutter_team_lints: ^3.5.2
   fake_async: 1.3.3
   mockito: ^5.4.4
   test: ^1.26.3

dev/cocoon_code_health/pubspec.yaml

@@ -20,5 +20,5 @@ dependencies:
   path: ^1.9.1
 
 dev_dependencies:
-  dart_flutter_team_lints:
+  dart_flutter_team_lints: ^3.5.2
   test: ^1.25.15

licenses/pubspec.yaml

@@ -11,5 +11,6 @@ dependencies:
   platform: 3.1.6
 
 dev_dependencies:
+  dart_flutter_team_lints: ^3.5.2
   mockito: ^5.4.6
   test_api: ^0.7.7

packages/cocoon_common/pubspec.yaml

@@ -17,6 +17,6 @@ dev_dependencies:
   build_runner: ^2.4.15
   cocoon_common_test:
     path: ../cocoon_common_test
-  dart_flutter_team_lints:
+  dart_flutter_team_lints: ^3.5.2
   json_serializable: ^6.9.4
   test: ^1.25.15

packages/cocoon_common_test/pubspec.yaml

@@ -13,5 +13,5 @@ dependencies:
   matcher: ^0.12.17
 
 dev_dependencies:
-  dart_flutter_team_lints:
+  dart_flutter_team_lints: ^3.5.2
   test: ^1.25.15