Skip to content

Commit

Permalink
Fix presubmit retry of missing builder (#3708)
Browse files Browse the repository at this point in the history
Currently the presubmit LUCI pubsub handler has a couple of un-ACK'd messages because the PR has a ci.yaml that references a builder that was recently removed. The current code to determine whether to retry the job is throwing an uncaught exception when validating the config, causing the handler to return a 500.

This catches that error, and forces the job not to retry, so that the message can be processed. Not retrying is the correct behavior, since it would fail again anway.
  • Loading branch information
stuartmorgan authored May 10, 2024
1 parent 7a8903e commit 8d621c6
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,16 @@ class PresubmitLuciSubscriptionV2 extends SubscriptionHandlerV2 {
sha: userData['commit_sha'] as String,
);
late CiYaml ciYaml;
if (commit.branch == Config.defaultBranch(commit.slug)) {
ciYaml = await scheduler.getCiYaml(commit, validate: true);
} else {
ciYaml = await scheduler.getCiYaml(commit);
try {
if (commit.branch == Config.defaultBranch(commit.slug)) {
ciYaml = await scheduler.getCiYaml(commit, validate: true);
} else {
ciYaml = await scheduler.getCiYaml(commit);
}
} on FormatException {
// If ci.yaml no longer passes validation (for example, because a builder
// has been removed), ensure no retries.
return 0;
}

// Do not block on the target not found.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,4 +300,58 @@ void main() {
),
).called(1);
});

test('Build not rescheduled if ci.yaml fails validation.', () async {
scheduler.failCiYamlValidation = true;
when(
mockGithubChecksService.updateCheckStatus(
build: anyNamed('build'),
userDataMap: anyNamed('userDataMap'),
luciBuildService: anyNamed('luciBuildService'),
slug: anyNamed('slug'),
rescheduled: false,
),
).thenAnswer((_) async => true);
when(mockGithubChecksService.taskFailed(any)).thenAnswer((_) => true);
when(mockGithubChecksService.currentAttempt(any)).thenAnswer((_) => 1);

final Map<String, dynamic> userDataMap = {
'repo_owner': 'flutter',
'commit_branch': Config.defaultBranch(Config.flutterSlug),
'commit_sha': 'abc',
'repo_name': 'flutter',
};

tester.message = createPushMessageV2(
Int64(1),
status: bbv2.Status.SUCCESS,
builder: 'Linux C',
userData: userDataMap,
);

final bbv2.BuildsV2PubSub buildsV2PubSub = createBuild(
Int64(1),
status: bbv2.Status.SUCCESS,
builder: 'Linux C',
);

await tester.post(handler);
verifyNever(
mockLuciBuildService.rescheduleBuild(
build: buildsV2PubSub.build,
builderName: 'Linux C',
userDataMap: userDataMap,
rescheduleAttempt: 1,
),
);
verify(
mockGithubChecksService.updateCheckStatus(
build: anyNamed('build'),
userDataMap: anyNamed('userDataMap'),
luciBuildService: anyNamed('luciBuildService'),
slug: anyNamed('slug'),
rescheduled: false,
),
).called(1);
});
}
12 changes: 10 additions & 2 deletions app_dart/test/src/service/fake_scheduler_v2.dart
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,22 @@ class FakeSchedulerV2 extends SchedulerV2 {
/// [CiYaml] value to be injected on [getCiYaml].
CiYaml? ciYaml;

/// If true, getCiYaml will throw a [FormatException] when validation is
/// enforced, simulating failing validation.
bool failCiYamlValidation = false;

@override
Future<CiYaml> getCiYaml(
Commit commit, {
CiYaml? totCiYaml,
RetryOptions? retryOptions,
bool validate = false,
}) async =>
ciYaml ?? _defaultConfig;
}) async {
if (validate && failCiYamlValidation) {
throw const FormatException('Failed validation!');
}
return ciYaml ?? _defaultConfig;
}

@override
Future<Commit> generateTotCommit({required String branch, required RepositorySlug slug}) async {
Expand Down

0 comments on commit 8d621c6

Please sign in to comment.