From 9faf0d1c91b7cb7d1029fc66bd7619acf97d7c4d Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Mon, 2 Mar 2026 16:57:49 -0800 Subject: [PATCH 1/3] ensuring proper current_attempt tag is added when scheduling try builds --- analyze/pubspec.yaml | 1 + .../model/common/failed_presubmit_checks.dart | 20 ++++++--- .../lib/src/service/luci_build_service.dart | 45 +++++++++++++++++-- app_dart/lib/src/service/scheduler.dart | 18 +++++--- app_dart/pubspec.yaml | 2 +- .../firestore/unified_check_run_test.dart | 8 ++-- app_dart/test/service/scheduler_test.dart | 24 +++++----- cipd_packages/device_doctor/pubspec.yaml | 1 + dev/cocoon_code_health/pubspec.yaml | 2 +- licenses/pubspec.yaml | 1 + packages/cocoon_common/pubspec.yaml | 2 +- packages/cocoon_common_test/pubspec.yaml | 2 +- pubspec.yaml | 2 +- 13 files changed, 94 insertions(+), 34 deletions(-) diff --git a/analyze/pubspec.yaml b/analyze/pubspec.yaml index fa18f9888..ee12ab4e9 100644 --- a/analyze/pubspec.yaml +++ b/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 diff --git a/app_dart/lib/src/model/common/failed_presubmit_checks.dart b/app_dart/lib/src/model/common/failed_presubmit_checks.dart index 329467a22..e12aee144 100644 --- a/app_dart/lib/src/model/common/failed_presubmit_checks.dart +++ b/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 checkNames; + final Map 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")'; } diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index 86fa22f21..966cdd892 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/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> reScheduleTryBuilds({ + required Map 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> _scheduleTryBuilds({ + required Map targets, + required PullRequest pullRequest, + required EngineArtifacts engineArtifacts, + CheckRun? checkRunGuard, + CiStage? stage, }) async { if (targets.isEmpty) { - return targets; + return targets.keys.toList(); } final batchRequestList = []; @@ -263,7 +297,9 @@ class LuciBuildService { checkRuns.add(checkRunGuard); } - for (var target in targets) { + for (var entry in targets.entries) { + final target = entry.key; + final attemptNumber = entry.value; // If the unified check run flow is disabled create individual check runs // for each target. if (!isUnifiedCheckRunFlow || checkRunGuard == null) { @@ -324,7 +360,6 @@ class LuciBuildService { } final requestedDimensions = target.getDimensions(); - batchRequestList.add( bbv2.BatchRequest_Request( scheduleBuild: _createPresubmitScheduleBuild( @@ -340,6 +375,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 +415,7 @@ class LuciBuildService { ); } - return targets; + return targets.keys.toList(); } /// Cancels all the current builds on [pullRequest] with [reason]. diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index a58c4264a..3ee62b557 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -1791,10 +1791,16 @@ $stacktrace pullRequest, ); - final failedTargets = targets - .where((target) => failedChecks.checkNames.contains(target.name)) - .toList(); - if (failedTargets.length != failedChecks.checkNames.length) { + final failedTargets = []; + final checkRetries = {}; + for (final target in targets) { + if (failedChecks.checkRetries.containsKey(target.name)) { + failedTargets.add(target); + checkRetries[target] = failedChecks.checkRetries[target.name]!; + } + } + + if (failedTargets.length != failedChecks.checkRetries.length) { log.error( '$logCrumb: Failed to find all failed targets in presubmit targets', ); @@ -1803,8 +1809,8 @@ $stacktrace ); } - await _luciBuildService.scheduleTryBuilds( - targets: failedTargets, + await _luciBuildService.reScheduleTryBuilds( + targets: checkRetries, pullRequest: pullRequest, engineArtifacts: artifacts, checkRunGuard: failedChecks.checkRunGuard, diff --git a/app_dart/pubspec.yaml b/app_dart/pubspec.yaml index a75a91494..749f35d76 100644 --- a/app_dart/pubspec.yaml +++ b/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 diff --git a/app_dart/test/service/firestore/unified_check_run_test.dart b/app_dart/test/service/firestore/unified_check_run_test.dart index b303a8dd8..9937a1472 100644 --- a/app_dart/test/service/firestore/unified_check_run_test.dart +++ b/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( diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 6c2887ec5..352903392 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/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((t) => t.name == 'Linux A')), + predicate>((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((t) => t.name == 'Linux A')), + predicate>((map) { + return map.keys.any((t) => t.name == 'Linux A'); + }), named: 'targets', ), pullRequest: pullRequest, diff --git a/cipd_packages/device_doctor/pubspec.yaml b/cipd_packages/device_doctor/pubspec.yaml index 76443e2e7..52f903048 100644 --- a/cipd_packages/device_doctor/pubspec.yaml +++ b/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 diff --git a/dev/cocoon_code_health/pubspec.yaml b/dev/cocoon_code_health/pubspec.yaml index 006e29377..36cdc72e8 100644 --- a/dev/cocoon_code_health/pubspec.yaml +++ b/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 diff --git a/licenses/pubspec.yaml b/licenses/pubspec.yaml index f54ad9322..7fb9adee4 100644 --- a/licenses/pubspec.yaml +++ b/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 diff --git a/packages/cocoon_common/pubspec.yaml b/packages/cocoon_common/pubspec.yaml index 04ee71459..421132905 100644 --- a/packages/cocoon_common/pubspec.yaml +++ b/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 diff --git a/packages/cocoon_common_test/pubspec.yaml b/packages/cocoon_common_test/pubspec.yaml index 42e4ce1b0..1760ed7f8 100644 --- a/packages/cocoon_common_test/pubspec.yaml +++ b/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 diff --git a/pubspec.yaml b/pubspec.yaml index aa93cd6c0..c617f0a7a 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -25,7 +25,7 @@ workspace: dev_dependencies: - dart_flutter_team_lints: 3.5.2 + dart_flutter_team_lints: ^3.5.2 melos: ^7.0.0-dev.7 melos: From ed1b53327e6d390a4c90f83f8091425ccb075e2b Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Tue, 3 Mar 2026 13:54:25 -0800 Subject: [PATCH 2/3] PR review changes --- app_dart/lib/src/service/luci_build_service.dart | 4 +--- app_dart/lib/src/service/scheduler.dart | 8 +++----- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/app_dart/lib/src/service/luci_build_service.dart b/app_dart/lib/src/service/luci_build_service.dart index 966cdd892..b230d8006 100644 --- a/app_dart/lib/src/service/luci_build_service.dart +++ b/app_dart/lib/src/service/luci_build_service.dart @@ -297,9 +297,7 @@ class LuciBuildService { checkRuns.add(checkRunGuard); } - for (var entry in targets.entries) { - final target = entry.key; - final attemptNumber = entry.value; + 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) { diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 3ee62b557..8487a4a0b 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/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,16 +1791,14 @@ $stacktrace pullRequest, ); - final failedTargets = []; final checkRetries = {}; for (final target in targets) { if (failedChecks.checkRetries.containsKey(target.name)) { - failedTargets.add(target); checkRetries[target] = failedChecks.checkRetries[target.name]!; } } - if (failedTargets.length != failedChecks.checkRetries.length) { + if (checkRetries.length != failedChecks.checkRetries.length) { log.error( '$logCrumb: Failed to find all failed targets in presubmit targets', ); @@ -1818,7 +1816,7 @@ $stacktrace ); log.info( - '$logCrumb: Successfully rescheduled ${failedTargets.length} targets', + '$logCrumb: Successfully rescheduled ${checkRetries.length} targets', ); return const ProcessCheckRunResult.success(); } From 156cccb501042cf15694e0a845dd78899ae3b657 Mon Sep 17 00:00:00 2001 From: Dmitry Grand Date: Wed, 4 Mar 2026 10:18:12 -0800 Subject: [PATCH 3/3] resolve merge issues --- .../rerun_all_failed_jobs.dart | 2 +- .../service/firestore/unified_check_run.dart | 6 +- .../lib/src/utilities/mocks.mocks.dart | 76 +++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart b/app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart index d3ff2da32..326224b89 100644 --- a/app_dart/lib/src/request_handlers/rerun_all_failed_jobs.dart +++ b/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( diff --git a/app_dart/lib/src/service/firestore/unified_check_run.dart b/app_dart/lib/src/service/firestore/unified_check_run.dart index 3dc0087a7..a0ddf3e74 100644 --- a/app_dart/lib/src/service/firestore/unified_check_run.dart +++ b/app_dart/lib/src/service/firestore/unified_check_run.dart @@ -134,6 +134,7 @@ final class UnifiedCheckRun { } latestGuard.builds = builds; final checks = []; + final checkRetries = {}; for (final buildName in failedBuildNames) { final latestCheck = await getLatestPresubmitCheck( firestoreService: firestoreService, @@ -141,6 +142,7 @@ final class UnifiedCheckRun { 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) { diff --git a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart index 11ec661c0..41bdedaed 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.mocks.dart @@ -4101,6 +4101,26 @@ class MockLuciBuildService extends _i1.Mock implements _i17.LuciBuildService { ) as _i13.Future>); + @override + _i13.Future> reScheduleTryBuilds({ + required Map<_i27.Target, int>? targets, + required _i7.PullRequest? pullRequest, + required _i28.EngineArtifacts? engineArtifacts, + required _i7.CheckRun? checkRunGuard, + required _i17.CiStage? stage, + }) => + (super.noSuchMethod( + Invocation.method(#reScheduleTryBuilds, [], { + #targets: targets, + #pullRequest: pullRequest, + #engineArtifacts: engineArtifacts, + #checkRunGuard: checkRunGuard, + #stage: stage, + }), + returnValue: _i13.Future>.value(<_i27.Target>[]), + ) + as _i13.Future>); + @override _i13.Future cancelBuilds({ required _i7.PullRequest? pullRequest, @@ -5779,6 +5799,23 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { ) as _i13.Future<_i39.ProcessCheckRunResult>); + @override + _i13.Future<_i39.ProcessCheckRunResult> reRunTargets( + _i7.RepositorySlug? slug, + _i7.PullRequest? pullRequest, + List? names, + ) => + (super.noSuchMethod( + Invocation.method(#reRunTargets, [slug, pullRequest, names]), + returnValue: _i13.Future<_i39.ProcessCheckRunResult>.value( + _i20.dummyValue<_i39.ProcessCheckRunResult>( + this, + Invocation.method(#reRunTargets, [slug, pullRequest, names]), + ), + ), + ) + as _i13.Future<_i39.ProcessCheckRunResult>); + @override _i13.Future<_i7.PullRequest?> findPullRequestCached( int? checkRunId, @@ -5799,6 +5836,45 @@ class MockScheduler extends _i1.Mock implements _i17.Scheduler { ) as _i13.Future<_i7.PullRequest?>); + @override + _i13.Future<_i7.PullRequest?> findPullRequestCachedForPullRequestNum( + _i7.RepositorySlug? slug, + int? pullRequestNum, + ) => + (super.noSuchMethod( + Invocation.method(#findPullRequestCachedForPullRequestNum, [ + slug, + pullRequestNum, + ]), + returnValue: _i13.Future<_i7.PullRequest?>.value(), + ) + as _i13.Future<_i7.PullRequest?>); + + @override + _i13.Future<(List<_i27.Target>, _i28.EngineArtifacts)> + getAllTargetsForPullRequest( + _i7.RepositorySlug? slug, + _i7.PullRequest? pullRequest, + ) => + (super.noSuchMethod( + Invocation.method(#getAllTargetsForPullRequest, [ + slug, + pullRequest, + ]), + returnValue: + _i13.Future<(List<_i27.Target>, _i28.EngineArtifacts)>.value(( + <_i27.Target>[], + _i20.dummyValue<_i28.EngineArtifacts>( + this, + Invocation.method(#getAllTargetsForPullRequest, [ + slug, + pullRequest, + ]), + ), + )), + ) + as _i13.Future<(List<_i27.Target>, _i28.EngineArtifacts)>); + @override _i7.CheckRun checkRunFromString(String? input) => (super.noSuchMethod(