Skip to content

Conditionally lock MQG on flutter/flutter+master only. #4685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 28 additions & 16 deletions app_dart/lib/src/service/scheduler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -325,27 +325,35 @@ 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;
{
final prBranch = pullRequest.head!.ref;
if (isFusion && Config.defaultBranch(slug) == prBranch) {
lock = await _lockMergeGroupChecks(slug, pullRequest.head!.sha!);
} else {
lock = null;
log.debug('Skipping merge queue guard: $slug/$prBranch');
}
}

// 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.
Expand Down Expand Up @@ -381,7 +389,7 @@ class Scheduler {

await _runCiTestingStage(
pullRequest: pullRequest,
checkRunGuard: '$lock',
checkRunGuard: lock?.toString() ?? '',
logCrumb: logCrumb,

// The if-branch already skips the engine build phase.
Expand Down Expand Up @@ -470,8 +478,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}',
Expand Down Expand Up @@ -561,12 +569,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;
}

Expand Down Expand Up @@ -730,7 +738,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<CheckRun> lockMergeGroupChecks(
Future<CheckRun> _lockMergeGroupChecks(
RepositorySlug slug,
String headSha,
) async {
Expand All @@ -754,7 +762,7 @@ $s
///
/// If the guard is guarding a pull request, this immediately makes the pull
/// request eligible for enqueuing into the merge queue.
Future<void> unlockMergeQueueGuard(
Future<void> _unlockMergeQueueGuard(
RepositorySlug slug,
String headSha,
CheckRun lock,
Expand Down Expand Up @@ -1113,7 +1121,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].
Expand Down Expand Up @@ -1146,7 +1158,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).
Expand Down
22 changes: 1 addition & 21 deletions app_dart/test/service/scheduler_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2392,11 +2392,6 @@ targets:
),
).captured,
<Object?>[
Config.kMergeQueueLockName,
const CheckRunOutput(
title: Config.kMergeQueueLockName,
summary: Scheduler.kMergeQueueLockDescription,
),
Config.kCiYamlCheckName,
const CheckRunOutput(
title: Config.kCiYamlCheckName,
Expand Down Expand Up @@ -2578,11 +2573,6 @@ targets:
),
).captured,
<Object?>[
Config.kMergeQueueLockName,
const CheckRunOutput(
title: Config.kMergeQueueLockName,
summary: Scheduler.kMergeQueueLockDescription,
),
Config.kCiYamlCheckName,
const CheckRunOutput(
title: Config.kCiYamlCheckName,
Expand Down Expand Up @@ -2619,11 +2609,6 @@ targets:
),
).captured,
<Object?>[
Config.kMergeQueueLockName,
const CheckRunOutput(
title: Config.kMergeQueueLockName,
summary: Scheduler.kMergeQueueLockDescription,
),
Config.kCiYamlCheckName,
const CheckRunOutput(
title: Config.kCiYamlCheckName,
Expand Down Expand Up @@ -2654,12 +2639,7 @@ targets:
output: anyNamed('output'),
),
).captured,
<Object?>[
CheckRunStatus.completed,
CheckRunConclusion.success,
CheckRunStatus.completed,
CheckRunConclusion.success,
],
<Object?>[CheckRunStatus.completed, CheckRunConclusion.success],
);
});

Expand Down