From ed7b7fa7df0bf8f7f6a5fc0d496459e880bd9516 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 14 May 2025 14:29:03 -0700 Subject: [PATCH 1/3] Conditionally lock MQG on flutter/flutter master only. --- app_dart/lib/src/service/scheduler.dart | 49 +++++++++++++++-------- app_dart/test/service/scheduler_test.dart | 22 +--------- 2 files changed, 34 insertions(+), 37 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index f372027b5..674ae1e3e 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -325,27 +325,40 @@ class Scheduler { await cancelPreSubmitTargets(pullRequest: pullRequest, reason: reason); final slug = pullRequest.base!.repo!.slug(); + final isFusion = slug == Config.flutterSlug; // The MQ only waits for "required status checks" before deciding whether to // merge the PR into the target branch. This required check added to both // the PR and to the merge group, and so it must be completed in both cases. - final lock = await lockMergeGroupChecks(slug, pullRequest.head!.sha!); + final CheckRun? lock; + { + if (!isFusion) { + log.debug('Skipping merge queue guard: $slug is not flutter/flutter'); + lock = null; + } else if (Config.defaultBranch(slug) != pullRequest.head!.ref) { + log.debug( + 'Skipping merge queue guard: ${pullRequest.head!.ref} is not the ' + 'default flutter/flutter branch', + ); + lock = null; + } else { + lock = await _lockMergeGroupChecks(slug, pullRequest.head!.sha!); + } + } - // Track if we should unlock the merge group lock in case of non-fusion or - // revert bots. + // Track if we should unlock the merge group lock in case of rever bots. + // This is different than simply not having a lock - because flutter/flutter + // master requires to see the lock existing and have been passed (versus + // non-existence). var unlockMergeGroup = false; final ciValidationCheckRun = await _createCiYamlCheckRun(pullRequest, slug); log.info('Creating presubmit targets for ${pullRequest.number}'); Object? exception; - final isFusion = slug == Config.flutterSlug; do { try { final sha = pullRequest.head!.sha!; - if (!isFusion) { - unlockMergeGroup = true; - } // Both the author and label should be checked to make sure that no one is // attempting to get a pull request without check through. @@ -381,7 +394,7 @@ class Scheduler { await _runCiTestingStage( pullRequest: pullRequest, - checkRunGuard: '$lock', + checkRunGuard: lock?.toString() ?? '', logCrumb: logCrumb, // The if-branch already skips the engine build phase. @@ -470,8 +483,8 @@ class Scheduler { // 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); + if (lock != null && unlockMergeGroup) { + await _unlockMergeQueueGuard(slug, pullRequest.head!.sha!, lock); } log.info( 'Finished triggering builds for: pr ${pullRequest.number}, commit ${pullRequest.base!.sha}, branch ${pullRequest.head!.ref} and slug $slug}', @@ -561,12 +574,12 @@ class Scheduler { } log.info('$logCrumb: scheduling merge group checks'); - final lock = await lockMergeGroupChecks(slug, headSha); + final lock = await _lockMergeGroupChecks(slug, headSha); // If the repo is not fusion, it doesn't run anything in the MQ, so just // close the merge group guard. if (!isFusion) { - await unlockMergeQueueGuard(slug, headSha, lock); + await _unlockMergeQueueGuard(slug, headSha, lock); return; } @@ -730,7 +743,7 @@ $s /// While this check is still in progress, the merge queue will not merge the /// respective PR onto the target branch (e.g. main or master), because this /// check is "required". - Future lockMergeGroupChecks( + Future _lockMergeGroupChecks( RepositorySlug slug, String headSha, ) async { @@ -754,7 +767,7 @@ $s /// /// If the guard is guarding a pull request, this immediately makes the pull /// request eligible for enqueuing into the merge queue. - Future unlockMergeQueueGuard( + Future _unlockMergeQueueGuard( RepositorySlug slug, String headSha, CheckRun lock, @@ -1113,7 +1126,11 @@ $s required String logCrumb, }) async { log.info('$logCrumb: Stage completed: ${CiStage.fusionTests}'); - await unlockMergeQueueGuard(slug, sha, checkRunFromString(mergeQueueGuard)); + await _unlockMergeQueueGuard( + slug, + sha, + checkRunFromString(mergeQueueGuard), + ); } /// Returns the presubmit targets for the fusion repo [pullRequest] that should run for the given [stage]. @@ -1146,7 +1163,7 @@ $s // Unlock the guarding check_run. final checkRunGuard = checkRunFromString(mergeQueueGuard); - await unlockMergeQueueGuard(slug, sha, checkRunGuard); + await _unlockMergeQueueGuard(slug, sha, checkRunGuard); } /// Schedules post-engine build tests (i.e. engine tests, and framework tests). diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 5abc60636..62e28ef47 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -2392,11 +2392,6 @@ targets: ), ).captured, [ - Config.kMergeQueueLockName, - const CheckRunOutput( - title: Config.kMergeQueueLockName, - summary: Scheduler.kMergeQueueLockDescription, - ), Config.kCiYamlCheckName, const CheckRunOutput( title: Config.kCiYamlCheckName, @@ -2578,11 +2573,6 @@ targets: ), ).captured, [ - Config.kMergeQueueLockName, - const CheckRunOutput( - title: Config.kMergeQueueLockName, - summary: Scheduler.kMergeQueueLockDescription, - ), Config.kCiYamlCheckName, const CheckRunOutput( title: Config.kCiYamlCheckName, @@ -2619,11 +2609,6 @@ targets: ), ).captured, [ - Config.kMergeQueueLockName, - const CheckRunOutput( - title: Config.kMergeQueueLockName, - summary: Scheduler.kMergeQueueLockDescription, - ), Config.kCiYamlCheckName, const CheckRunOutput( title: Config.kCiYamlCheckName, @@ -2654,12 +2639,7 @@ targets: output: anyNamed('output'), ), ).captured, - [ - CheckRunStatus.completed, - CheckRunConclusion.success, - CheckRunStatus.completed, - CheckRunConclusion.success, - ], + [CheckRunStatus.completed, CheckRunConclusion.success], ); }); From a473e6e496f617303a9c17b450124ee51277ea53 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 15 May 2025 11:17:07 -0700 Subject: [PATCH 2/3] Feedback. --- app_dart/lib/src/service/scheduler.dart | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 674ae1e3e..217a2eb5b 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -332,17 +332,11 @@ class Scheduler { // the PR and to the merge group, and so it must be completed in both cases. final CheckRun? lock; { - if (!isFusion) { - log.debug('Skipping merge queue guard: $slug is not flutter/flutter'); - lock = null; - } else if (Config.defaultBranch(slug) != pullRequest.head!.ref) { - log.debug( - 'Skipping merge queue guard: ${pullRequest.head!.ref} is not the ' - 'default flutter/flutter branch', - ); - lock = null; - } else { + final prBranch = pullRequest.head!.ref; + if (isFusion && Config.defaultBranch(slug) == prBranch) { lock = await _lockMergeGroupChecks(slug, pullRequest.head!.sha!); + } else { + log.debug('Skipping merge queue guard: $slug/$prBranch'); } } From 3ad13f7d7b145ef9ed27cf29e832db5926ae3797 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 15 May 2025 11:18:21 -0700 Subject: [PATCH 3/3] ++ --- app_dart/lib/src/service/scheduler.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 217a2eb5b..b953ad620 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -336,6 +336,7 @@ class Scheduler { if (isFusion && Config.defaultBranch(slug) == prBranch) { lock = await _lockMergeGroupChecks(slug, pullRequest.head!.sha!); } else { + lock = null; log.debug('Skipping merge queue guard: $slug/$prBranch'); } }