From 34e265489bbfc711df149e3df795342637cac2c7 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Tue, 28 Apr 2026 16:49:27 -0700 Subject: [PATCH 1/4] Refactor CICD label management to use per-PR sequential queue This change overhauls the management of the `CICD` label in Cocoon to address race conditions and non-idempotent operations that caused issues in the previous attempt. Key changes: - Introduced `PullRequestManager` to handle events for a single PR sequentially via an operation queue. - Moved PR-specific logic from `webhook_subscription.dart` to `PullRequestManager`, thinning out the webhook handler. - Implemented new state machine logic for `opened` and `synchronize` events to handle privileged users and draft PRs correctly. - Made `_scheduleIfMergeable` idempotent by tracking `scheduledSha` in Firestore to prevent duplicate triggering of presubmits for the same SHA. - Refactored `PullRequestManager` creation to be synchronous in the cache, queueing the asynchronous initialization to avoid creation race conditions. - Restored `cicd_flowchart.md` from a previous commit to document the flow. - Fixed all static analysis issues and ensured all tests pass. --- app_dart/docs/cicd_flowchart.md | 33 + app_dart/lib/cocoon_service.dart | 1 + .../model/firestore/pull_request_state.dart | 85 ++ .../github/webhook_subscription.dart | 770 +----------- .../lib/src/service/pull_request_manager.dart | 1029 +++++++++++++++++ .../github/webhook_subscription_test.dart | 78 +- .../lib/src/utilities/mocks.dart | 2 +- 7 files changed, 1235 insertions(+), 763 deletions(-) create mode 100644 app_dart/docs/cicd_flowchart.md create mode 100644 app_dart/lib/src/model/firestore/pull_request_state.dart create mode 100644 app_dart/lib/src/service/pull_request_manager.dart diff --git a/app_dart/docs/cicd_flowchart.md b/app_dart/docs/cicd_flowchart.md new file mode 100644 index 000000000..bdd1eb6f0 --- /dev/null +++ b/app_dart/docs/cicd_flowchart.md @@ -0,0 +1,33 @@ +# CICD Label Flowchart + +This flowchart describes the logic for managing the `CICD` label in Cocoon for running presubmit CI. + +```mermaid +flowchart TD + PR_Opened([Event: PR Opened]) --> Is_Draft{Is Draft?} + + Is_Draft -- No --> Is_Privileged_Open{Is Privileged?} + Is_Draft -- Yes --> Create_Awaiting[Create Awaiting Check Run] + + Is_Privileged_Open -- Yes --> Add_Label[Add CICD Label] + Add_Label --> Start_Pre[Start Presubmits] + + Is_Privileged_Open -- No --> Create_Awaiting + + Create_Awaiting --> State_Awaiting((State: Awaiting)) + Start_Pre --> State_Running((State: Running)) + + State_Awaiting --> Label_Added([Event: CICD Label Added]) + Label_Added --> Resolve_Awaiting[Resolve Awaiting Check Run] + Resolve_Awaiting --> Start_Pre + + State_Running --> Changes_Pushed([Event: Changes Pushed]) + Changes_Pushed --> Is_Privileged_Push{Is Privileged?} + Remove_Label --> Create_Awaiting + + Is_Privileged_Push -- No --> Remove_Label[Remove CICD Label] + + Is_Privileged_Push -- Yes --> Label_Present{Has CICD Label?} + Label_Present -- Yes --> Start_Pre + Label_Present -- No --> Create_Awaiting +``` diff --git a/app_dart/lib/cocoon_service.dart b/app_dart/lib/cocoon_service.dart index 9cd66123a..6c6e325f0 100644 --- a/app_dart/lib/cocoon_service.dart +++ b/app_dart/lib/cocoon_service.dart @@ -60,4 +60,5 @@ export 'src/service/flags/dynamic_config.dart'; export 'src/service/gerrit_service.dart'; export 'src/service/github_checks_service.dart'; export 'src/service/luci_build_service.dart'; +export 'src/service/pull_request_manager.dart'; export 'src/service/scheduler.dart'; diff --git a/app_dart/lib/src/model/firestore/pull_request_state.dart b/app_dart/lib/src/model/firestore/pull_request_state.dart new file mode 100644 index 000000000..075a793d4 --- /dev/null +++ b/app_dart/lib/src/model/firestore/pull_request_state.dart @@ -0,0 +1,85 @@ +// Copyright 2026 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:convert'; + +import 'package:github/github.dart'; +import 'package:googleapis/firestore/v1.dart'; + +import '../../service/firestore.dart'; +import 'base.dart'; + +/// Represents the state of a Pull Request that we want to persist across events. +final class PullRequestState extends AppDocument { + static const kCollectionId = 'pullRequestStates'; + static const kIsPrivilegedField = 'is_privileged'; + static const kLatestShaField = 'latest_sha'; + static const kScheduledShaField = 'scheduled_sha'; + static const kSlugField = 'slug'; + static const kNumberField = 'number'; + + @override + AppDocumentMetadata get runtimeMetadata => metadata; + + /// Description of the document in Firestore. + static final metadata = AppDocumentMetadata( + collectionId: kCollectionId, + fromDocument: PullRequestState.fromDocument, + ); + + /// Create [PullRequestState] from a Document. + static PullRequestState fromDocument(Document doc) { + return PullRequestState() + ..fields = doc.fields! + ..name = doc.name!; + } + + /// Whether the PR author is a privileged user (roller or flutter-hacker). + bool? get isPrivileged => fields[kIsPrivilegedField]?.booleanValue; + + set isPrivileged(bool? value) { + if (value != null) { + fields[kIsPrivilegedField] = Value(booleanValue: value); + } + } + + /// The latest SHA that we have processed for this PR. + String? get latestSha => fields[kLatestShaField]?.stringValue; + + set latestSha(String? value) { + if (value != null) { + fields[kLatestShaField] = Value(stringValue: value); + } + } + + /// The SHA for which we have scheduled presubmits. + String? get scheduledSha => fields[kScheduledShaField]?.stringValue; + + set scheduledSha(String? value) { + if (value != null) { + fields[kScheduledShaField] = Value(stringValue: value); + } + } + + /// The repository slug associated with the pull request. + RepositorySlug get slug => RepositorySlug.fromJson( + json.decode(fields[kSlugField]!.stringValue!) as Map, + ); + + set slug(RepositorySlug value) { + fields[kSlugField] = json.encode(value.toJson()).toValue(); + } + + /// The PR number. + int get number => int.parse(fields[kNumberField]!.integerValue!); + + set number(int value) { + fields[kNumberField] = Value(integerValue: '$value'); + } + + /// Generates the document ID for a PR. + static String getDocumentId(RepositorySlug slug, int number) { + return '${slug.owner}_${slug.name}_$number'; + } +} diff --git a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart index 2ab49a5d4..e22cf5b94 100644 --- a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart +++ b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart @@ -9,7 +9,6 @@ import 'package:cocoon_server/logging.dart'; import 'package:github/github.dart'; import 'package:github/github.dart' as github; import 'package:github/hooks.dart'; -import 'package:googleapis/firestore/v1.dart' as g; import 'package:meta/meta.dart'; import '../../../cocoon_service.dart'; @@ -20,36 +19,8 @@ import '../../model/github/workflow_job.dart' as workflow_job; import '../../request_handling/exceptions.dart'; import '../../request_handling/subscription_handler.dart'; import '../../service/commit_service.dart'; -import '../../service/github_service.dart'; import '../../service/scheduler/process_check_run_result.dart'; -// Filenames which are not actually tests. -const List kNotActuallyATest = [ - 'packages/flutter/lib/src/gestures/hit_test.dart', -]; - -/// List of repos that require check for tests. -Set kNeedsTests = { - Config.flutterSlug, - Config.packagesSlug, -}; - -// Extentions for files that use // for single line comments. -// See [_allChangesAreCodeComments] for more. -@visibleForTesting -const Set knownCommentCodeExtensions = { - 'cc', - 'cpp', - 'dart', - 'gradle', - 'groovy', - 'java', - 'kt', - 'm', - 'mm', - 'swift', -}; - /// Subscription for processing GitHub webhooks. /// /// The PubSub subscription is set up here: @@ -65,7 +36,7 @@ final class GithubWebhookSubscription extends SubscriptionHandler { static const _estimatedGitOnBorgMaximumSyncDuration = Duration(minutes: 15); /// Creates a subscription for processing GitHub webhooks. - const GithubWebhookSubscription({ + GithubWebhookSubscription({ required super.cache, required super.config, required this.scheduler, @@ -77,9 +48,18 @@ final class GithubWebhookSubscription extends SubscriptionHandler { @visibleForTesting DateTime Function() now = DateTime.now, // Gets the initial github events from this sub after the webhook uploads them. }) : _now = now, + prManagerCache = PullRequestManagerCache( + capacity: 100, + firestore: firestore, + config: config, + scheduler: scheduler, + gerritService: gerritService, + pullRequestLabelProcessorProvider: pullRequestLabelProcessorProvider, + ), super(subscriptionName: 'github-webhooks-sub'); final DateTime Function() _now; + final PullRequestManagerCache prManagerCache; /// Cocoon scheduler to trigger tasks against changes from GitHub. final Scheduler scheduler; @@ -199,50 +179,33 @@ final class GithubWebhookSubscription extends SubscriptionHandler { final result = await _processPullRequestClosed(pullRequestEvent); return result.toResponse(); case 'edited': - if (pullRequestEvent.changes != null && - pullRequestEvent.changes!.base != null) { - await _addCICDForRollersAndMembers(pullRequestEvent); - } - await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!); - await _checkForTests(pullRequestEvent); + final manager = prManagerCache.getOrCreate(pullRequestEvent); + await manager.handleEdited(pullRequestEvent); break; case 'opened': - await _addCICDForRollersAndMembers(pullRequestEvent); - await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!); - await _checkForTests(pullRequestEvent); - await _tryReleaseApproval(pullRequestEvent); + final manager = prManagerCache.getOrCreate(pullRequestEvent); + await manager.handleOpened(pullRequestEvent); break; case 'reopened': - // TODO(matanlurey): Handle more elegantly than just failing. - // https://github.com/flutter/flutter/issues/162656 - await _warnThatANewCommitIsNeeded(pullRequestEvent); + final manager = prManagerCache.getOrCreate(pullRequestEvent); + await manager.handleReopened(pullRequestEvent); + break; case 'labeled': log.info( '$crumb: PR labels = [${pr.labels?.map((label) => '"${label.name}"').join(', ')}]', ); - final labelEvent = _getLabeledEvent(rawRequest); - if (labelEvent?.label case final label?) { - if (Config.kCicdLabelIds.contains(label.id) && - label.name == Config.kCicdLabel) { - log.info('new CICD label added - scheduling tests'); - await scheduler.resolveAwaitingCicdLabelCheckRun( - slug, - pr.head!.sha!, - ); - await _scheduleIfMergeable(pullRequestEvent); - } - } - - await _processLabels(pr); - await _updatePullRequest(pr); - + final labelName = labelEvent?.label.name; + final labelId = labelEvent?.label.id; + final manager = prManagerCache.getOrCreate(pullRequestEvent); + await manager.handleLabeled(pullRequestEvent, labelName, labelId); break; case 'dequeued': await _respondToPullRequestDequeued(pullRequestEvent); break; case 'synchronize': - await _addCICDForRollersAndMembers(pullRequestEvent); + final manager = prManagerCache.getOrCreate(pullRequestEvent); + await manager.handleSynchronize(pullRequestEvent); break; // Ignore the rest of the events. case 'ready_for_review': @@ -258,19 +221,6 @@ final class GithubWebhookSubscription extends SubscriptionHandler { return Response.emptyOk; } - Future _processLabels(PullRequest pullRequest) async { - final slug = pullRequest.base!.repo!.slug(); - final githubService = await config.createGithubService(slug); - - final labelProcessor = pullRequestLabelProcessorProvider( - config: config, - githubService: githubService, - pullRequest: pullRequest, - ); - - return labelProcessor.processLabels(); - } - /// Handles a GitHub webhook with the event type "merge_group". /// /// A merge group contains commits from multiple pull requests. Each pull @@ -413,109 +363,6 @@ final class GithubWebhookSubscription extends SubscriptionHandler { } } - /// This method assumes that jobs should be cancelled if they are already - /// runnning. - Future _scheduleIfMergeable(PullRequestEvent pullRequestEvent) async { - final pr = pullRequestEvent.pullRequest!; - final slug = pullRequestEvent.repository!.slug(); - - log.info( - 'Scheduling tasks if mergeable(${pr.mergeable}): owner=${slug.owner} repo=${slug.name} and pr=${pr.number}', - ); - - // The mergeable flag may be null. False indicates there's a merge conflict, - // null indicates unknown. Err on the side of allowing the job to run. - if (pr.mergeable == false) { - final slug = pullRequestEvent.repository!.slug(); - final gitHubClient = await config.createGitHubClient(pullRequest: pr); - final body = config.mergeConflictPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - return; - } - - try { - await scheduler.triggerPresubmitTargets(pullRequest: pr); - - // When presubmit targets are scheduled the PR acquires a new Merge Queue - // Guard. This can happen when the PR is just created, a new commit is - // pushed, reopened, etc. In all cases the guard may need to be unlocked if, - // for example, the "emergency" label is present. - await _processLabels(pr); - } on g.DetailedApiRequestError catch (e) { - if (e.status != HttpStatus.conflict) { - rethrow; - } - await _warnThatANewCommitIsNeeded(pullRequestEvent); - } - } - - /// Update the PR stored in [PrCheckRuns] so that subsequent checks are fresh. - Future _updatePullRequest(PullRequest pr) async { - final sha = pr.head!.sha!; - final didUpdate = await PrCheckRuns.updatePullRequestForSha( - firestore, - sha, - pr, - ); - if (!didUpdate) { - log.debug('No PR found for SHA: $sha, did not update'); - } else { - log.debug('Updated PR for SHA: $sha'); - } - } - - /// Warns that [pr] is out of date, and a new commit is needed. - /// - /// See . - Future _warnThatANewCommitIsNeeded( - PullRequestEvent pullRequestEvent, - ) async { - final pr = pullRequestEvent.pullRequest!; - final slug = pullRequestEvent.repository!.slug(); - final sha = pr.head!.sha!; - - final gitHubClient = config.createGitHubClientWithToken( - await config.githubOAuthToken, - ); - await gitHubClient.issues.createComment( - slug, - pr.number!, - Config.newCommitIsNeeded(sha: sha), - ); - } - - /// Release tooling generates cherrypick pull requests that should be granted an approval. - Future _tryReleaseApproval(PullRequestEvent pullRequestEvent) async { - final pr = pullRequestEvent.pullRequest!; - final slug = pullRequestEvent.repository!.slug(); - - final defaultBranch = Config.defaultBranch(slug); - final branch = pr.base?.ref; - if (branch == null || branch.contains(defaultBranch)) { - // This isn't a release branch PR - return; - } - - final releaseAccounts = await config.releaseAccounts; - if (releaseAccounts.contains(pr.user?.login) == false) { - // The author isn't in the list of release accounts, do nothing - return; - } - - final gitHubClient = config.createGitHubClientWithToken( - await config.githubOAuthToken, - ); - final review = CreatePullRequestReview( - slug.owner, - slug.name, - pr.number!, - 'APPROVE', - ); - await gitHubClient.pullRequests.createReview(slug, review); - } - @useResult Future _processPullRequestClosed( PullRequestEvent pullRequestEvent, @@ -580,418 +427,6 @@ final class GithubWebhookSubscription extends SubscriptionHandler { ); } - Future _addCICDForRollersAndMembers( - PullRequestEvent pullRequestEvent, - ) async { - final pr = pullRequestEvent.pullRequest!; - final slug = pr.base!.repo!.slug(); - final author = pr.user!.login!; - final githubService = await config.createGithubService(slug); - - final isRoller = config.rollerAccounts.contains(pr.user!.login); - final isFlutterHacker = await githubService.isTeamMember( - 'flutter-hackers', - author, - slug.owner, - ); - log.debug('isRoller=$isRoller, isFlutterHacker=$isFlutterHacker'); - - if (config.supportedRepos.contains(slug) && (isRoller || isFlutterHacker)) { - final gitHubClient = await config.createGitHubClient(pullRequest: pr); - await gitHubClient.issues.addLabelsToIssue(slug, pr.number!, ['CICD']); - log.debug('Added CICD label to PR $pr.number'); - } - } - - Future _checkForTests(PullRequestEvent pullRequestEvent) async { - final pr = pullRequestEvent.pullRequest!; - // We do not need to add test labels if this is an auto roller author. - if (config.rollerAccounts.contains(pr.user!.login)) { - return; - } - final eventAction = pullRequestEvent.action; - final slug = pr.base!.repo!.slug(); - final isTipOfTree = pr.base!.ref == Config.defaultBranch(slug); - final gitHubClient = await config.createGitHubClient(pullRequest: pr); - await _validateRefs(gitHubClient, pr); - if (kNeedsTests.contains(slug) && isTipOfTree) { - log.info( - 'Applying framework repo labels for: owner=${slug.owner} repo=${slug.name} and pr=${pr.number}', - ); - switch (slug.name) { - case 'flutter': - final isFusion = slug == Config.flutterSlug; - final files = await gitHubClient.pullRequests - .listFiles(slug, pr.number!) - .toList(); - await _applyFrameworkRepoLabels( - gitHubClient, - eventAction, - pr, - files, - slug, - ); - if (isFusion) { - await _applyEngineRepoLabels( - gitHubClient, - eventAction, - pr, - files, - slug, - ); - } - case 'packages': - return _applyPackageTestChecks(gitHubClient, eventAction, pr); - } - } - } - - Future _applyFrameworkRepoLabels( - GitHub gitHubClient, - String? eventAction, - PullRequest pr, - List files, - RepositorySlug slug, - ) async { - if (pr.user!.login == 'engine-flutter-autoroll') { - return; - } - - final labels = {}; - var hasTests = false; - var needsTests = false; - - var frameworkFiles = 0; - - for (var file in files) { - final filename = file.filename!; - - if (!_isFusionEnginePath(filename)) { - frameworkFiles++; - } - - if (_fileContainsAddedCode(file) && - !_isTestExempt(filename) && - !filename.startsWith('dev/bots/') && - !filename.endsWith('.gitignore')) { - needsTests = !_allChangesAreCodeComments(file); - } - - // Check to see if tests were submitted with this PR. - if (_isAFrameworkTest(filename)) { - hasTests = true; - } - } - - if (frameworkFiles == 0) { - // a fusion / engine only change. - return; - } - - if (pr.user!.login == 'fluttergithubbot') { - needsTests = false; - labels.addAll(['c: tech-debt', 'c: flake']); - } - - if (labels.isNotEmpty) { - await gitHubClient.issues.addLabelsToIssue( - slug, - pr.number!, - labels.toList(), - ); - } - - if (!hasTests && - needsTests && - !pr.draft! && - !_isPrUpdatingReleaseBranch(pr)) { - final body = config.missingTestsPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - } - } - - bool _isAFrameworkTest(String filename) { - if (kNotActuallyATest.any(filename.endsWith)) { - return false; - } - // Check for Objective-C tests which end in either "Tests.m" or "Test.m" - // in the "dev" directory. - final objectiveCTestRegex = RegExp(r'.*dev\/.*Test[s]?\.m$'); - return filename.endsWith('_test.dart') || - filename.endsWith('.expect') || - filename.contains('test_fixes') || - // Include updates to test utilities or test data - filename.contains('packages/flutter_tools/test/') || - // Kotlin source tests, used in the Flutter Gradle Plugin. - filename.startsWith('packages/flutter_tools/gradle/src/test/') || - filename.startsWith('dev/bots/analyze.dart') || - filename.startsWith('dev/bots/test.dart') || - filename.startsWith('dev/devicelab/bin/tasks') || - filename.startsWith('dev/devicelab/lib/tasks') || - filename.startsWith('dev/benchmarks') || - objectiveCTestRegex.hasMatch(filename); - } - - /// Returns true if changes to [filename] are exempt from the testing - /// requirement, across repositories. - bool _isTestExempt(String filename, {String engineBasePath = ''}) { - final isBuildPythonScript = - filename.startsWith('${engineBasePath}sky/tools') && - filename.endsWith('.py'); - return filename.contains('.ci.yaml') || - filename.endsWith('analysis_options.yaml') || - filename.endsWith('AUTHORS') || - filename.endsWith('CODEOWNERS') || - filename.endsWith('TESTOWNERS') || - filename.endsWith('pubspec.yaml') || - filename.endsWith('pubspec.yaml.tmpl') || - // Exempt categories. - filename.contains('.gemini/') || - filename.contains('.github/') || - filename.endsWith('.md') || - // Exempt paths. - filename.startsWith('dev/customer_testing/tests.version') || - filename.startsWith('dev/devicelab/lib/versions/gallery.dart') || - filename.startsWith('dev/integration_tests/') || - filename.startsWith('docs/') || - filename.startsWith('${engineBasePath}docs/') || - filename.endsWith('test/flutter_test_config.dart') || - // ↓↓↓ Begin engine specific paths ↓↓↓ - filename == 'DEPS' || // note: in monorepo; DEPS is still at the root. - isBuildPythonScript || - filename.endsWith('.gni') || - filename.endsWith('.gn') || - filename.startsWith('${engineBasePath}impeller/fixtures/') || - filename.startsWith('${engineBasePath}impeller/golden_tests/') || - filename.startsWith('${engineBasePath}impeller/playground/') || - filename.startsWith( - '${engineBasePath}shell/platform/embedder/tests/', - ) || - filename.startsWith( - '${engineBasePath}shell/platform/embedder/fixtures/', - ) || - filename.startsWith('${engineBasePath}testing/') || - filename.startsWith('${engineBasePath}tools/clangd_check/'); - } - - bool _isFusionEnginePath(String path) => - path.startsWith('engine/') || path == 'DEPS'; - - Future _applyEngineRepoLabels( - GitHub gitHubClient, - String? eventAction, - PullRequest pr, - List files, - RepositorySlug slug, - ) async { - // Do not apply the test labels for the autoroller accounts. - if (pr.user!.login == 'skia-flutter-autoroll') { - return; - } - - var hasTests = false; - var needsTests = false; - - // If engine labels are being applied to the flutterSlug - we're in a fusion repo. - final isFusion = slug == Config.flutterSlug; - final engineBasePath = isFusion ? 'engine/src/flutter/' : ''; - - var engineFiles = 0; - - for (var file in files) { - final path = file.filename!; - if (isFusion && _isFusionEnginePath(path)) { - engineFiles++; - } - - if (_fileContainsAddedCode(file) && - !_isTestExempt(path, engineBasePath: engineBasePath) && - // License goldens are auto-generated. - !path.startsWith('${engineBasePath}ci/licenses_golden/') && - // Build configuration files tell CI what to run. - !path.startsWith('${engineBasePath}ci/builders/')) { - needsTests = !_allChangesAreCodeComments(file); - } - - if (_isAnEngineTest(path)) { - hasTests = true; - } - } - - if (isFusion && engineFiles == 0) { - // framework only change - return; - } - - if (!hasTests && - needsTests && - !pr.draft! && - !_isPrUpdatingReleaseBranch(pr)) { - final body = config.missingTestsPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - } - } - - bool _isAnEngineTest(String filename) { - final engineTestRegExp = RegExp( - r'(tests?|benchmarks?)\.(dart|java|mm|m|cc|sh|py|swift)$', - ); - return filename.contains('IosBenchmarks') || - filename.contains('IosUnitTests') || - filename.contains('scenario_app') || - engineTestRegExp.hasMatch(filename.toLowerCase()); - } - - bool _fileContainsAddedCode(PullRequestFile file) { - // When null, do not assume 0 lines have been added. - final linesAdded = file.additionsCount ?? 1; - final linesDeleted = file.deletionsCount ?? 0; - final linesTotal = file.changesCount ?? linesDeleted + linesAdded; - return linesAdded > 0 || linesDeleted != linesTotal; - } - - // Runs automated test checks for both flutter/packages. - Future _applyPackageTestChecks( - GitHub gitHubClient, - String? eventAction, - PullRequest pr, - ) async { - final slug = pr.base!.repo!.slug(); - final files = gitHubClient.pullRequests.listFiles(slug, pr.number!); - var hasTests = false; - var needsTests = false; - - await for (PullRequestFile file in files) { - final filename = file.filename!; - - if (_fileContainsAddedCode(file) && - !_isTestExempt(filename) && - !filename.contains('.ci/') && - // Custom package-specific test runners. These do not count as tests - // for the purposes of testing a change that otherwise needs tests, - // but since they are the driver for tests they don't need test - // coverage. - !filename.endsWith('tool/run_tests.dart') && - !filename.endsWith('run_tests.sh')) { - needsTests = !_allChangesAreCodeComments(file); - } - // See https://github.com/flutter/flutter/blob/master/docs/ecosystem/testing/Plugin-Tests.md for discussion - // of various plugin test types and locations. - if (filename.endsWith('_test.dart') || - // Native iOS/macOS tests. - filename.contains('RunnerTests/') || - filename.contains('RunnerUITests/') || - filename.contains('darwin/Tests/') || - // Native Android tests. - filename.contains('android/src/test/') || - filename.contains('androidTest/') || - // Native Linux tests. - filename.endsWith('_test.cc') || - // Native Windows tests. - filename.endsWith('_test.cpp') || - // Pigeon native tests. - filename.contains('/platform_tests/') || - // Test files in package-specific test folders. - filename.contains('go_router/test_fixes/') || - filename.contains('go_router_builder/test_inputs/')) { - hasTests = true; - } - } - - if (!hasTests && - needsTests && - !pr.draft! && - !_isPrUpdatingReleaseBranch(pr)) { - final body = config.missingTestsPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - } - } - - /// Validate the base and head refs of the PR. - Future _validateRefs(GitHub gitHubClient, PullRequest pr) async { - final slug = pr.base!.repo!.slug(); - String body; - const releaseChannels = ['stable', 'beta', 'dev']; - // Close PRs that use a release branch as a source. - if (releaseChannels.contains(pr.head!.ref)) { - body = config.wrongHeadBranchPullRequestMessage(pr.head!.ref!); - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.pullRequests.edit(slug, pr.number!, state: 'closed'); - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - return; - } - final defaultBranchName = Config.defaultBranch(pr.base!.repo!.slug()); - final baseName = pr.base!.ref!; - if (baseName == defaultBranchName) { - return; - } - if (_isPrUpdatingReleaseBranch(pr)) { - body = config.releaseBranchPullRequestMessage; - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - return; - } - - // For repos migrated to main, close PRs opened against master. - final isMaster = pr.base?.ref == 'master'; - final isMigrated = defaultBranchName == 'main'; - // PRs should never be open to "beta" or "stable." - final isReleaseChannelBranch = releaseChannels.contains(pr.base?.ref); - if ((isMaster && isMigrated) || isReleaseChannelBranch) { - body = _getWrongBaseComment( - base: baseName, - defaultBranch: defaultBranchName, - ); - if (!await _alreadyCommented(gitHubClient, pr, body)) { - await gitHubClient.pullRequests.edit( - slug, - pr.number!, - base: Config.defaultBranch(slug), - ); - await gitHubClient.issues.createComment(slug, pr.number!, body); - } - } - } - - static bool _isPrUpdatingReleaseBranch(PullRequest pr) { - return isReleaseCandidateBranch(branchName: pr.base!.ref!); - } - - Future _alreadyCommented( - GitHub gitHubClient, - PullRequest pr, - String message, - ) async { - final comments = gitHubClient.issues.listCommentsByIssue( - pr.base!.repo!.slug(), - pr.number!, - ); - await for (IssueComment comment in comments) { - if (comment.body != null && comment.body!.contains(message)) { - return true; - } - } - return false; - } - - String _getWrongBaseComment({ - required String base, - required String defaultBranch, - }) { - final messageTemplate = config.wrongBaseBranchPullRequestMessage; - return messageTemplate - .replaceAll('{{target_branch}}', base) - .replaceAll('{{default_branch}}', defaultBranch); - } - PullRequestEvent? _getPullRequestEvent(String request) { try { return PullRequestEvent.fromJson( @@ -1013,163 +448,4 @@ final class GithubWebhookSubscription extends SubscriptionHandler { return null; } } - - /// Returns true if the changes to [file] are all code comments. - /// - /// If that cannot be determined with confidence, returns false. False - /// negatives (e.g., for /* */-style multi-line comments) should be expected. - bool _allChangesAreCodeComments(PullRequestFile file) { - final linesAdded = file.additionsCount; - final linesDeleted = file.deletionsCount; - final patch = file.patch; - // If information is missing, err or the side of assuming it's a non-comment - // change. - if (linesAdded == null || linesDeleted == null || patch == null) { - return false; - } - - final filename = file.filename!; - final extension = filename.contains('.') - ? filename.split('.').last.toLowerCase() - : null; - if (extension == null || !knownCommentCodeExtensions.contains(extension)) { - return false; - } - - // Only handles single-line comments; identifying multi-line comments - // would require the full file and non-trivial parsing. Also doesn't handle - // end-of-line comments (e.g., "int x = 0; // Info about x"). - final commentRegex = RegExp(r'^[+-]\s*//'); - final onlyWhitespaceRegex = RegExp(r'^[+-]\s*$'); - for (var line in patch.split('\n')) { - if (!line.startsWith('+') && !line.startsWith('-')) { - continue; - } - - if (onlyWhitespaceRegex.hasMatch(line)) { - // whitespace only changes don't require tests - continue; - } - - if (!commentRegex.hasMatch(line)) { - return false; - } - } - return true; - } -} - -typedef PullRequestLabelProcessorProvider = - PullRequestLabelProcessor Function({ - required Config config, - required GithubService githubService, - required PullRequest pullRequest, - }); - -class PullRequestLabelProcessor { - PullRequestLabelProcessor({ - required this.config, - required this.githubService, - required this.pullRequest, - }) : slug = pullRequest.base!.repo!.slug(), - prNumber = pullRequest.number!; - - static const kEmergencyLabelEducation = ''' -Detected the `emergency` label. - -If you add the `autosubmit` label, the bot will wait until all presubmits pass but ignore the tree status, allowing fixes for tree breakages while still validating that they don't break any existing presubmits. - -The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". -'''; - - final Config config; - final GithubService githubService; - final PullRequest pullRequest; - final RepositorySlug slug; - final int prNumber; - late final String logCrumb = - '$PullRequestLabelProcessor($slug/pull/$prNumber)'; - - void logInfo(Object? message) { - log.info('$logCrumb: $message'); - } - - void logSevere(Object? message, {Object? error, StackTrace? stackTrace}) { - log.error('$logCrumb: $message', error, stackTrace); - } - - Future processLabels() async { - final hasEmergencyLabel = - pullRequest.labels?.any( - (label) => label.name == Config.kEmergencyLabel, - ) ?? - false; - if (hasEmergencyLabel) { - // The merge guard and Flutter Presubmits check can be unlocked without approval checks because: - // - // * For manual merges the GitHub repo settings already require minimum - // approvals before the PR can be submitted. - // * For `autosubmit` label Cocoon has the [Approval] validation that - // checks approvals before attempting to merge the PR. - await _unlockCheckrunsForEmergency(); - } else { - logInfo('no emergency label; moving on.'); - } - } - - Future _unlockCheckrunsForEmergency() async { - await _unlockCheckrun(Config.kMergeQueueLockName); - await _unlockCheckrun(Config.kFlutterPresubmitsName); - - // Let the developer know what is happening with the MQ when this label is found the first time. - try { - if (!await githubService.commentExists( - slug, - prNumber, - PullRequestLabelProcessor.kEmergencyLabelEducation, - )) { - await githubService.createComment( - slug, - issueNumber: prNumber, - body: PullRequestLabelProcessor.kEmergencyLabelEducation, - ); - } - } catch (e, s) { - logSevere( - 'failed to leave educational comment for emergency label.', - error: e, - stackTrace: s, - ); - } - } - - Future _unlockCheckrun(String checkName) async { - logInfo('attempting to unlock the $checkName for emergency'); - - final guard = (await githubService.getCheckRunsFiltered( - slug: slug, - ref: pullRequest.head!.sha!, - checkName: checkName, - )).singleOrNull; - - if (guard == null) { - logSevere( - 'failed to process the emergency label. "$checkName" check run is missing.', - ); - return; - } - - await githubService.updateCheckRun( - slug: slug, - checkRun: guard, - status: CheckRunStatus.completed, - conclusion: CheckRunConclusion.success, - output: CheckRunOutput( - title: checkName, - summary: 'Emergency label applied.', - ), - ); - - logInfo('unlocked "$checkName", allowing it to land as an emergency.'); - } } diff --git a/app_dart/lib/src/service/pull_request_manager.dart b/app_dart/lib/src/service/pull_request_manager.dart new file mode 100644 index 000000000..df71d10cc --- /dev/null +++ b/app_dart/lib/src/service/pull_request_manager.dart @@ -0,0 +1,1029 @@ +// Copyright 2026 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; +import 'dart:collection'; +import 'dart:convert'; +import 'dart:io'; + +import 'package:cocoon_common/is_release_branch.dart'; +import 'package:cocoon_server/logging.dart'; +import 'package:github/github.dart'; +import 'package:github/hooks.dart'; +import 'package:googleapis/firestore/v1.dart' hide Value; + +import '../model/firestore/pr_check_runs.dart'; +import '../model/firestore/pull_request_state.dart'; +import 'config.dart'; +import 'firestore.dart'; +import 'gerrit_service.dart'; +import 'github_service.dart'; +import 'scheduler.dart'; + +// Filenames which are not actually tests. +const List kNotActuallyATest = [ + 'packages/flutter/lib/src/gestures/hit_test.dart', +]; + +/// List of repos that require check for tests. +Set kNeedsTests = { + Config.flutterSlug, + Config.packagesSlug, +}; + +// Extentions for files that use // for single line comments. +const Set knownCommentCodeExtensions = { + 'cc', + 'cpp', + 'dart', + 'gradle', + 'groovy', + 'java', + 'kt', + 'm', + 'mm', + 'swift', +}; + +/// Manages the state and event queue for a single Pull Request. +class PullRequestManager { + final RepositorySlug slug; + final int prNumber; + final FirestoreService firestore; + final Config config; + final Scheduler scheduler; + final GerritService gerritService; + final PullRequestLabelProcessorProvider pullRequestLabelProcessorProvider; + + final PullRequestOperationQueue _queue = PullRequestOperationQueue(); + + late final bool _isPrivileged; + late String _latestSha; + String? _scheduledSha; + + PullRequestManager._({ + required this.slug, + required this.prNumber, + required this.firestore, + required this.config, + required this.scheduler, + required this.gerritService, + required this.pullRequestLabelProcessorProvider, + }); + + /// Kicks off asynchronous initialization in the queue. + void _initialize(PullRequestEvent event) { + _addEvent(() async { + final filterMap = { + 'slug =': json.encode(slug.toJson()), + 'number =': prNumber, + }; + final docs = await firestore.query( + PullRequestState.kCollectionId, + filterMap, + ); + + if (docs.isNotEmpty) { + final state = PullRequestState.fromDocument(docs.first); + _isPrivileged = state.isPrivileged ?? false; + _latestSha = state.latestSha ?? event.pullRequest!.head!.sha!; + _scheduledSha = state.scheduledSha; + log.info( + 'Hydrated PullRequestManager for $slug/$prNumber: isPrivileged=$_isPrivileged, latestSha=$_latestSha, scheduledSha=$_scheduledSha', + ); + } else { + final pr = event.pullRequest!; + final author = pr.user!.login!; + final githubService = await config.createGithubService(slug); + final isRoller = config.rollerAccounts.contains(author); + final isFlutterHacker = await githubService.isTeamMember( + 'flutter-hackers', + author, + slug.owner, + ); + _isPrivileged = isRoller || isFlutterHacker; + _latestSha = pr.head!.sha!; + + // Persist initial state + final state = PullRequestState() + ..slug = slug + ..number = prNumber + ..isPrivileged = _isPrivileged + ..latestSha = _latestSha; + final document = Document(fields: state.fields); + await firestore.createDocument( + document, + collectionId: PullRequestState.kCollectionId, + ); + log.info('Created initial PullRequestState for $slug/$prNumber'); + } + }); + } + + bool get isQueueEmpty => _queue.isEmpty; + + /// Adds an event to the queue to be processed sequentially. + Future _addEvent(Future Function() operation) { + return _queue.add(operation); + } + + /// Persists the current state to Firestore. + Future persist() async { + final filterMap = { + 'slug =': json.encode(slug.toJson()), + 'number =': prNumber, + }; + final docs = await firestore.query( + PullRequestState.kCollectionId, + filterMap, + ); + + final state = PullRequestState() + ..slug = slug + ..number = prNumber + ..isPrivileged = _isPrivileged + ..latestSha = latestSha + ..scheduledSha = _scheduledSha; + + if (docs.isEmpty) { + // Create + final document = Document(fields: state.fields); + await firestore.createDocument( + document, + collectionId: PullRequestState.kCollectionId, + ); + log.info('Created PullRequestState for $slug/$prNumber'); + } else { + // Update + final existing = PullRequestState.fromDocument(docs.first); + existing.isPrivileged = _isPrivileged; + existing.latestSha = latestSha; + existing.scheduledSha = _scheduledSha; + + final tx = await firestore.beginTransaction(); + await firestore.commit(tx, documentsToWrites([existing])); + log.info('Updated PullRequestState for $slug/$prNumber'); + } + } + + /// The latest SHA that we have processed for this PR. + String get latestSha => _latestSha; + set latestSha(String value) { + if (_latestSha != value) { + _latestSha = value; + _addEvent(persist); + } + } + + /// The SHA for which we have scheduled presubmits. + String? get scheduledSha => _scheduledSha; + set scheduledSha(String? value) { + if (_scheduledSha != value) { + _scheduledSha = value; + _addEvent(persist); + } + } + + /// Handles PR opened event. + Future handleOpened(PullRequestEvent event) async { + await _addEvent(() async { + final pr = event.pullRequest!; + final sha = pr.head!.sha!; + latestSha = sha; // Update latest SHA. + + if (!pr.draft! && _isPrivileged) { + final gitHubClient = await config.createGitHubClient(pullRequest: pr); + await gitHubClient.issues.addLabelsToIssue(slug, pr.number!, ['CICD']); + log.debug('Added CICD label to PR $pr.number'); + await _scheduleIfMergeable(event); + } else { + await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); + } + + await checkForTests(event); + await _tryReleaseApproval(event); + }); + } + + /// Handles PR synchronize event. + Future handleSynchronize(PullRequestEvent event) async { + await _addEvent(() async { + final pr = event.pullRequest!; + final sha = pr.head!.sha!; + latestSha = sha; // Update latest SHA. + + final hasLabel = + pr.labels?.any((IssueLabel l) => l.name == Config.kCicdLabel) ?? + false; + final isPrivilegedUser = _isPrivileged; + + if (isPrivilegedUser) { + if (hasLabel) { + await _scheduleIfMergeable(event); + } else { + await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); + } + } else { + if (hasLabel) { + final githubService = await config.createGithubService(slug); + await githubService.removeLabel(slug, pr.number!, Config.kCicdLabel); + log.debug('Removed CICD label from PR ${pr.number}'); + } + await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); + } + }); + } + + /// Handles PR edited event. + Future handleEdited(PullRequestEvent event) async { + await _addEvent(() async { + await checkForTests(event); + }); + } + + /// Handles PR reopened event. + Future handleReopened(PullRequestEvent event) async { + await _addEvent(() async { + final pr = event.pullRequest!; + await _warnThatANewCommitIsNeeded(event); + await _processLabels(pr); + await _updatePullRequest(pr); + }); + } + + /// Handles PR labeled event. + Future handleLabeled( + PullRequestEvent event, + String? labelName, + int? labelId, + ) async { + await _addEvent(() async { + final pr = event.pullRequest!; + final sha = pr.head!.sha!; + + if (labelName == Config.kCicdLabel && + Config.kCicdLabelIds.contains(labelId)) { + log.info('new CICD label added - scheduling tests'); + await scheduler.resolveAwaitingCicdLabelCheckRun(slug, sha); + await _scheduleIfMergeable(event); + } + + await _processLabels(pr); + await _updatePullRequest(pr); + }); + } + + Future checkForTests(PullRequestEvent pullRequestEvent) async { + final pr = pullRequestEvent.pullRequest!; + // We do not need to add test labels if this is an auto roller author. + if (config.rollerAccounts.contains(pr.user!.login)) { + return; + } + final eventAction = pullRequestEvent.action; + final isTipOfTree = pr.base!.ref == Config.defaultBranch(slug); + final gitHubClient = await config.createGitHubClient(pullRequest: pr); + await _validateRefs(gitHubClient, pr); + if (kNeedsTests.contains(slug) && isTipOfTree) { + log.info( + 'Applying framework repo labels for: owner=${slug.owner} repo=${slug.name} and pr=${pr.number}', + ); + switch (slug.name) { + case 'flutter': + final isFusion = slug == Config.flutterSlug; + final files = await gitHubClient.pullRequests + .listFiles(slug, pr.number!) + .toList(); + await _applyFrameworkRepoLabels( + gitHubClient, + eventAction, + pr, + files, + slug, + ); + if (isFusion) { + await _applyEngineRepoLabels( + gitHubClient, + eventAction, + pr, + files, + slug, + ); + } + case 'packages': + return _applyPackageTestChecks(gitHubClient, eventAction, pr); + } + } + } + + Future _applyFrameworkRepoLabels( + GitHub gitHubClient, + String? eventAction, + PullRequest pr, + List files, + RepositorySlug slug, + ) async { + if (pr.user!.login == 'engine-flutter-autoroll') { + return; + } + + final labels = {}; + var hasTests = false; + var needsTests = false; + + var frameworkFiles = 0; + + for (var file in files) { + final filename = file.filename!; + + if (!_isFusionEnginePath(filename)) { + frameworkFiles++; + } + + if (_fileContainsAddedCode(file) && + !_isTestExempt(filename) && + !filename.startsWith('dev/bots/') && + !filename.endsWith('.gitignore')) { + needsTests = !_allChangesAreCodeComments(file); + } + + // Check to see if tests were submitted with this PR. + if (_isAFrameworkTest(filename)) { + hasTests = true; + } + } + + if (frameworkFiles == 0) { + // a fusion / engine only change. + return; + } + + if (pr.user!.login == 'fluttergithubbot') { + needsTests = false; + labels.addAll(['c: tech-debt', 'c: flake']); + } + + if (labels.isNotEmpty) { + await gitHubClient.issues.addLabelsToIssue( + slug, + pr.number!, + labels.toList(), + ); + } + + if (!hasTests && + needsTests && + !pr.draft! && + !_isPrUpdatingReleaseBranch(pr)) { + final body = config.missingTestsPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + } + } + + bool _isAFrameworkTest(String filename) { + if (kNotActuallyATest.any(filename.endsWith)) { + return false; + } + // Check for Objective-C tests which end in either "Tests.m" or "Test.m" + // in the "dev" directory. + final objectiveCTestRegex = RegExp(r'.*dev\/.*Test[s]?\.m$'); + return filename.endsWith('_test.dart') || + filename.endsWith('.expect') || + filename.contains('test_fixes') || + // Include updates to test utilities or test data + filename.contains('packages/flutter_tools/test/') || + // Kotlin source tests, used in the Flutter Gradle Plugin. + filename.startsWith('packages/flutter_tools/gradle/src/test/') || + filename.startsWith('dev/bots/analyze.dart') || + filename.startsWith('dev/bots/test.dart') || + filename.startsWith('dev/devicelab/bin/tasks') || + filename.startsWith('dev/devicelab/lib/tasks') || + filename.startsWith('dev/benchmarks') || + objectiveCTestRegex.hasMatch(filename); + } + + /// Returns true if changes to [filename] are exempt from the testing + /// requirement, across repositories. + bool _isTestExempt(String filename, {String engineBasePath = ''}) { + final isBuildPythonScript = + filename.startsWith('${engineBasePath}sky/tools') && + filename.endsWith('.py'); + return filename.contains('.ci.yaml') || + filename.endsWith('analysis_options.yaml') || + filename.endsWith('AUTHORS') || + filename.endsWith('CODEOWNERS') || + filename.endsWith('TESTOWNERS') || + filename.endsWith('pubspec.yaml') || + filename.endsWith('pubspec.yaml.tmpl') || + // Exempt categories. + filename.contains('.gemini/') || + filename.contains('.github/') || + filename.endsWith('.md') || + // Exempt paths. + filename.startsWith('dev/customer_testing/tests.version') || + filename.startsWith('dev/devicelab/lib/versions/gallery.dart') || + filename.startsWith('dev/integration_tests/') || + filename.startsWith('docs/') || + filename.startsWith('${engineBasePath}docs/') || + filename.endsWith('test/flutter_test_config.dart') || + // ↓↓↓ Begin engine specific paths ↓↓↓ + filename == 'DEPS' || // note: in monorepo; DEPS is still at the root. + isBuildPythonScript || + filename.endsWith('.gni') || + filename.endsWith('.gn') || + filename.startsWith('${engineBasePath}impeller/fixtures/') || + filename.startsWith('${engineBasePath}impeller/golden_tests/') || + filename.startsWith('${engineBasePath}impeller/playground/') || + filename.startsWith( + '${engineBasePath}shell/platform/embedder/tests/', + ) || + filename.startsWith( + '${engineBasePath}shell/platform/embedder/fixtures/', + ) || + filename.startsWith('${engineBasePath}testing/') || + filename.startsWith('${engineBasePath}tools/clangd_check/'); + } + + bool _isFusionEnginePath(String path) => + path.startsWith('engine/') || path == 'DEPS'; + + Future _applyEngineRepoLabels( + GitHub gitHubClient, + String? eventAction, + PullRequest pr, + List files, + RepositorySlug slug, + ) async { + // Do not apply the test labels for the autoroller accounts. + if (pr.user!.login == 'skia-flutter-autoroll') { + return; + } + + var hasTests = false; + var needsTests = false; + + // If engine labels are being applied to the flutterSlug - we're in a fusion repo. + final isFusion = slug == Config.flutterSlug; + final engineBasePath = isFusion ? 'engine/src/flutter/' : ''; + + var engineFiles = 0; + + for (var file in files) { + final path = file.filename!; + if (isFusion && _isFusionEnginePath(path)) { + engineFiles++; + } + + if (_fileContainsAddedCode(file) && + !_isTestExempt(path, engineBasePath: engineBasePath) && + // License goldens are auto-generated. + !path.startsWith('${engineBasePath}ci/licenses_golden/') && + // Build configuration files tell CI what to run. + !path.startsWith('${engineBasePath}ci/builders/')) { + needsTests = !_allChangesAreCodeComments(file); + } + + if (_isAnEngineTest(path)) { + hasTests = true; + } + } + + if (isFusion && engineFiles == 0) { + // framework only change + return; + } + + if (!hasTests && + needsTests && + !pr.draft! && + !_isPrUpdatingReleaseBranch(pr)) { + final body = config.missingTestsPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + } + } + + bool _isAnEngineTest(String filename) { + final engineTestRegExp = RegExp( + r'(tests?|benchmarks?)\.(dart|java|mm|m|cc|sh|py|swift)$', + ); + return filename.contains('IosBenchmarks') || + filename.contains('IosUnitTests') || + filename.contains('scenario_app') || + engineTestRegExp.hasMatch(filename.toLowerCase()); + } + + bool _fileContainsAddedCode(PullRequestFile file) { + // When null, do not assume 0 lines have been added. + final linesAdded = file.additionsCount ?? 1; + final linesDeleted = file.deletionsCount ?? 0; + final linesTotal = file.changesCount ?? linesDeleted + linesAdded; + return linesAdded > 0 || linesDeleted != linesTotal; + } + + // Runs automated test checks for both flutter/packages. + Future _applyPackageTestChecks( + GitHub gitHubClient, + String? eventAction, + PullRequest pr, + ) async { + final slug = pr.base!.repo!.slug(); + final files = gitHubClient.pullRequests.listFiles(slug, pr.number!); + var hasTests = false; + var needsTests = false; + + await for (PullRequestFile file in files) { + final filename = file.filename!; + + if (_fileContainsAddedCode(file) && + !_isTestExempt(filename) && + !filename.contains('.ci/') && + // Custom package-specific test runners. These do not count as tests + // for the purposes of testing a change that otherwise needs tests, + // but since they are the driver for tests they don't need test + // coverage. + !filename.endsWith('tool/run_tests.dart') && + !filename.endsWith('run_tests.sh')) { + needsTests = !_allChangesAreCodeComments(file); + } + // See https://github.com/flutter/flutter/blob/master/docs/ecosystem/testing/Plugin-Tests.md for discussion + // of various plugin test types and locations. + if (filename.endsWith('_test.dart') || + // Native iOS/macOS tests. + filename.contains('RunnerTests/') || + filename.contains('RunnerUITests/') || + filename.contains('darwin/Tests/') || + // Native Android tests. + filename.contains('android/src/test/') || + filename.contains('androidTest/') || + // Native Linux tests. + filename.endsWith('_test.cc') || + // Native Windows tests. + filename.endsWith('_test.cpp') || + // Pigeon native tests. + filename.contains('/platform_tests/') || + // Test files in package-specific test folders. + filename.contains('go_router/test_fixes/') || + filename.contains('go_router_builder/test_inputs/')) { + hasTests = true; + } + } + + if (!hasTests && + needsTests && + !pr.draft! && + !_isPrUpdatingReleaseBranch(pr)) { + final body = config.missingTestsPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + } + } + + /// Validate the base and head refs of the PR. + Future _validateRefs(GitHub gitHubClient, PullRequest pr) async { + final slug = pr.base!.repo!.slug(); + String body; + const releaseChannels = ['stable', 'beta', 'dev']; + // Close PRs that use a release branch as a source. + if (releaseChannels.contains(pr.head!.ref)) { + body = config.wrongHeadBranchPullRequestMessage(pr.head!.ref!); + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.pullRequests.edit(slug, pr.number!, state: 'closed'); + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + return; + } + final defaultBranchName = Config.defaultBranch(pr.base!.repo!.slug()); + final baseName = pr.base!.ref!; + if (baseName == defaultBranchName) { + return; + } + if (_isPrUpdatingReleaseBranch(pr)) { + body = config.releaseBranchPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + return; + } + + // For repos migrated to main, close PRs opened against master. + final isMaster = pr.base?.ref == 'master'; + final isMigrated = defaultBranchName == 'main'; + // PRs should never be open to "beta" or "stable." + final isReleaseChannelBranch = releaseChannels.contains(pr.base?.ref); + if ((isMaster && isMigrated) || isReleaseChannelBranch) { + body = _getWrongBaseComment( + base: baseName, + defaultBranch: defaultBranchName, + ); + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.pullRequests.edit( + slug, + pr.number!, + base: Config.defaultBranch(slug), + ); + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + } + } + + static bool _isPrUpdatingReleaseBranch(PullRequest pr) { + return isReleaseCandidateBranch(branchName: pr.base!.ref!); + } + + Future _alreadyCommented( + GitHub gitHubClient, + PullRequest pr, + String message, + ) async { + final comments = gitHubClient.issues.listCommentsByIssue( + pr.base!.repo!.slug(), + pr.number!, + ); + await for (IssueComment comment in comments) { + if (comment.body != null && comment.body!.contains(message)) { + return true; + } + } + return false; + } + + String _getWrongBaseComment({ + required String base, + required String defaultBranch, + }) { + final messageTemplate = config.wrongBaseBranchPullRequestMessage; + return messageTemplate + .replaceAll('{{target_branch}}', base) + .replaceAll('{{default_branch}}', defaultBranch); + } + + /// Returns true if the changes to [file] are all code comments. + /// + /// If that cannot be determined with confidence, returns false. False + /// negatives (e.g., for /* */-style multi-line comments) should be expected. + bool _allChangesAreCodeComments(PullRequestFile file) { + final linesAdded = file.additionsCount; + final linesDeleted = file.deletionsCount; + final patch = file.patch; + // If information is missing, err or the side of assuming it's a non-comment + // change. + if (linesAdded == null || linesDeleted == null || patch == null) { + return false; + } + + final filename = file.filename!; + final extension = filename.contains('.') + ? filename.split('.').last.toLowerCase() + : null; + if (extension == null || !knownCommentCodeExtensions.contains(extension)) { + return false; + } + + // Only handles single-line comments; identifying multi-line comments + // would require the full file and non-trivial parsing. Also doesn't handle + // end-of-line comments (e.g., "int x = 0; // Info about x"). + final commentRegex = RegExp(r'^[+-]\s*//'); + final onlyWhitespaceRegex = RegExp(r'^[+-]\s*$'); + for (var line in patch.split('\n')) { + if (!line.startsWith('+') && !line.startsWith('-')) { + continue; + } + + if (onlyWhitespaceRegex.hasMatch(line)) { + // whitespace only changes don't require tests + continue; + } + + if (!commentRegex.hasMatch(line)) { + return false; + } + } + return true; + } + + /// Release tooling generates cherrypick pull requests that should be granted an approval. + Future _tryReleaseApproval(PullRequestEvent pullRequestEvent) async { + final pr = pullRequestEvent.pullRequest!; + final slug = pullRequestEvent.repository!.slug(); + + final defaultBranch = Config.defaultBranch(slug); + final branch = pr.base?.ref; + if (branch == null || branch.contains(defaultBranch)) { + // This isn't a release branch PR + return; + } + + final releaseAccounts = await config.releaseAccounts; + if (releaseAccounts.contains(pr.user?.login) == false) { + // The author isn't in the list of release accounts, do nothing + return; + } + + final gitHubClient = config.createGitHubClientWithToken( + await config.githubOAuthToken, + ); + final review = CreatePullRequestReview( + slug.owner, + slug.name, + pr.number!, + 'APPROVE', + ); + await gitHubClient.pullRequests.createReview(slug, review); + } + + Future _processLabels(PullRequest pullRequest) async { + final slug = pullRequest.base!.repo!.slug(); + final githubService = await config.createGithubService(slug); + + final labelProcessor = pullRequestLabelProcessorProvider( + config: config, + githubService: githubService, + pullRequest: pullRequest, + ); + + return labelProcessor.processLabels(); + } + + Future _scheduleIfMergeable(PullRequestEvent pullRequestEvent) async { + final pr = pullRequestEvent.pullRequest!; + final slug = pullRequestEvent.repository!.slug(); + final sha = pr.head!.sha!; + + if (scheduledSha == sha) { + log.info('Presubmits already scheduled for SHA $sha. Skipping.'); + return; + } + + log.info( + 'Scheduling tasks if mergeable(${pr.mergeable}): owner=${slug.owner} repo=${slug.name} and pr=${pr.number}', + ); + + if (pr.mergeable == false) { + final slug = pullRequestEvent.repository!.slug(); + final gitHubClient = await config.createGitHubClient(pullRequest: pr); + final body = config.mergeConflictPullRequestMessage; + if (!await _alreadyCommented(gitHubClient, pr, body)) { + await gitHubClient.issues.createComment(slug, pr.number!, body); + } + return; + } + + try { + await scheduler.triggerPresubmitTargets(pullRequest: pr); + scheduledSha = sha; // Update scheduled SHA! + await _processLabels(pr); + } on DetailedApiRequestError catch (e) { + if (e.status != HttpStatus.conflict) { + rethrow; + } + await _warnThatANewCommitIsNeeded(pullRequestEvent); + } + } + + Future _warnThatANewCommitIsNeeded( + PullRequestEvent pullRequestEvent, + ) async { + final pr = pullRequestEvent.pullRequest!; + final slug = pullRequestEvent.repository!.slug(); + final sha = pr.head!.sha!; + + final gitHubClient = config.createGitHubClientWithToken( + await config.githubOAuthToken, + ); + await gitHubClient.issues.createComment( + slug, + pr.number!, + Config.newCommitIsNeeded(sha: sha), + ); + } + + /// Update the PR stored in [PrCheckRuns] so that subsequent checks are fresh. + Future _updatePullRequest(PullRequest pr) async { + final sha = pr.head!.sha!; + final didUpdate = await PrCheckRuns.updatePullRequestForSha( + firestore, + sha, + pr, + ); + if (!didUpdate) { + log.debug('No PR found for SHA: $sha, did not update'); + } else { + log.debug('Updated PR for SHA: $sha'); + } + } +} + +/// A helper to ensure sequential execution of asynchronous tasks. +class PullRequestOperationQueue { + Future _currentOperation = Future.value(); + int _pendingOperations = 0; + + bool get isEmpty => _pendingOperations == 0; + + Future add(Future Function() operation) { + final completer = Completer(); + _pendingOperations++; + _currentOperation = _currentOperation.then((_) async { + try { + await operation(); + completer.complete(); + } catch (e, s) { + log.warn('Error in PullRequestOperationQueue', e, s); + completer.completeError(e, s); + } finally { + _pendingOperations--; + } + }); + return completer.future; + } +} + +/// LRU Cache for [PullRequestManager] instances. +class PullRequestManagerCache { + final int capacity; + final FirestoreService firestore; + final Config config; + final Scheduler scheduler; + final GerritService gerritService; + final PullRequestLabelProcessorProvider pullRequestLabelProcessorProvider; + final Map _cache = {}; + + PullRequestManagerCache({ + required this.capacity, + required this.firestore, + required this.config, + required this.scheduler, + required this.gerritService, + required this.pullRequestLabelProcessorProvider, + }); + + PullRequestManager getOrCreate(PullRequestEvent event) { + final pr = event.pullRequest!; + final slug = event.repository!.slug(); + final prNumber = pr.number!; + + final key = '${slug.owner}_${slug.name}_$prNumber'; + + // 1. Check cache + final manager = _cache.remove(key); + if (manager != null) { + _cache[key] = manager; // Move to end + return manager; + } + + // 2. Create new synchronously + final newManager = PullRequestManager._( + slug: slug, + prNumber: prNumber, + firestore: firestore, + config: config, + scheduler: scheduler, + gerritService: gerritService, + pullRequestLabelProcessorProvider: pullRequestLabelProcessorProvider, + ); + + newManager._initialize(event); + + // Eviction logic + if (_cache.length >= capacity) { + // Try to evict the least recently used + String? keyToEvict; + for (final k in _cache.keys) { + final m = _cache[k]!; + if (m.isQueueEmpty) { + keyToEvict = k; + break; + } + } + if (keyToEvict != null) { + _cache.remove(keyToEvict); + log.info('Evicted PullRequestManager for $keyToEvict'); + } else { + log.warn('Cache full, but all managers are busy. Not evicting.'); + } + } + + _cache[key] = newManager; + return newManager; + } +} + +typedef PullRequestLabelProcessorProvider = + PullRequestLabelProcessor Function({ + required Config config, + required GithubService githubService, + required PullRequest pullRequest, + }); + +class PullRequestLabelProcessor { + PullRequestLabelProcessor({ + required this.config, + required this.githubService, + required this.pullRequest, + }) : slug = pullRequest.base!.repo!.slug(), + prNumber = pullRequest.number!; + + static const kEmergencyLabelEducation = ''' +Detected the `emergency` label. + +If you add the `autosubmit` label, the bot will wait until all presubmits pass but ignore the tree status, allowing fixes for tree breakages while still validating that they don't break any existing presubmits. + +The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". +'''; + + final Config config; + final GithubService githubService; + final PullRequest pullRequest; + final RepositorySlug slug; + final int prNumber; + late final String logCrumb = + '$PullRequestLabelProcessor($slug/pull/$prNumber)'; + + void logInfo(Object? message) { + log.info('$logCrumb: $message'); + } + + void logSevere(Object? message, {Object? error, StackTrace? stackTrace}) { + log.error('$logCrumb: $message', error, stackTrace); + } + + Future processLabels() async { + final hasEmergencyLabel = + pullRequest.labels?.any( + (label) => label.name == Config.kEmergencyLabel, + ) ?? + false; + if (hasEmergencyLabel) { + // The merge guard and Flutter Presubmits check can be unlocked without approval checks because: + // + // * For manual merges the GitHub repo settings already require minimum + // approvals before the PR can be submitted. + // * For `autosubmit` label Cocoon has the [Approval] validation that + // checks approvals before attempting to merge the PR. + await _unlockCheckrunsForEmergency(); + } else { + logInfo('no emergency label; moving on.'); + } + } + + Future _unlockCheckrunsForEmergency() async { + await _unlockCheckrun(Config.kMergeQueueLockName); + await _unlockCheckrun(Config.kFlutterPresubmitsName); + + // Let the developer know what is happening with the MQ when this label is found the first time. + try { + if (!await githubService.commentExists( + slug, + prNumber, + PullRequestLabelProcessor.kEmergencyLabelEducation, + )) { + await githubService.createComment( + slug, + issueNumber: prNumber, + body: PullRequestLabelProcessor.kEmergencyLabelEducation, + ); + } + } catch (e, s) { + logSevere( + 'failed to leave educational comment for emergency label.', + error: e, + stackTrace: s, + ); + } + } + + Future _unlockCheckrun(String checkName) async { + logInfo('attempting to unlock the $checkName for emergency'); + + final guard = (await githubService.getCheckRunsFiltered( + slug: slug, + ref: pullRequest.head!.sha!, + checkName: checkName, + )).singleOrNull; + + if (guard == null) { + logSevere( + 'failed to process the emergency label. "$checkName" check run is missing.', + ); + return; + } + + await githubService.updateCheckRun( + slug: slug, + checkRun: guard, + status: CheckRunStatus.completed, + conclusion: CheckRunConclusion.success, + output: CheckRunOutput( + title: checkName, + summary: 'Emergency label applied.', + ), + ); + + logInfo('unlocked "$checkName", allowing it to land as an emergency.'); + } +} diff --git a/app_dart/test/request_handlers/github/webhook_subscription_test.dart b/app_dart/test/request_handlers/github/webhook_subscription_test.dart index fc09e7ff5..460f911df 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -23,6 +23,7 @@ import 'package:cocoon_service/src/service/big_query.dart'; import 'package:cocoon_service/src/service/cache_service.dart'; import 'package:cocoon_service/src/service/config.dart'; import 'package:cocoon_service/src/service/github_service.dart'; +import 'package:cocoon_service/src/service/pull_request_manager.dart'; import 'package:cocoon_service/src/service/scheduler.dart'; import 'package:fixnum/fixnum.dart'; import 'package:github/github.dart' hide Branch; @@ -3949,50 +3950,96 @@ void foo() { ); test( - 'synchronize event with CICD label does not schedules tests', + 'synchronize event by non-privileged user removes CICD label and creates awaiting checkrun', () async { + githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( action: 'synchronize', withCicdLabel: true, ); await tester.post(webhook); + + expect(githubService.removedLabels, [ + (Config.flutterSlug, 123, Config.kCicdLabel), + ]); + verify( + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + 'Awaiting CICD label', + output: anyNamed('output'), + ), + ).called(1); expect(scheduler.triggerPresubmitTargetsCnt, 0); }, ); test( - 'synchronize event adds CICD label if author is member of flutter-hackers', + 'synchronize event by non-privileged user without label creates awaiting checkrun', () async { + githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( action: 'synchronize', - login: 'test-flutter-hacker', + withCicdLabel: false, ); await tester.post(webhook); + verify( - issuesService.addLabelsToIssue(Config.flutterSlug, 123, ['CICD']), - ); + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + 'Awaiting CICD label', + output: anyNamed('output'), + ), + ).called(1); + expect(scheduler.triggerPresubmitTargetsCnt, 0); }, ); + test( - 'synchronize event does not add CICD label if author is not member of flutter-hackers', + 'synchronize event by privileged user with label schedules tests', () async { - tester.message = generateGithubWebhookMessage(action: 'synchronize'); + // Setup: Open PR as privileged user to populate state + tester.message = generateGithubWebhookMessage( + action: 'opened', + login: 'test-flutter-hacker', + ); + await tester.post(webhook); + expect(scheduler.triggerPresubmitTargetsCnt, 1); + + // Act: Synchronize with label + tester.message = generateGithubWebhookMessage( + action: 'synchronize', + login: 'test-flutter-hacker', + withCicdLabel: true, + ); await tester.post(webhook); + + expect(scheduler.triggerPresubmitTargetsCnt, 1); verifyNever( - issuesService.addLabelsToIssue(Config.flutterSlug, 123, any), + mockGithubChecksUtil.createCheckRun( + any, + any, + any, + 'Awaiting CICD label', + output: anyNamed('output'), + ), ); }, ); test( - 'opened PR without CICD label creates "Awaiting CICD label" checkrun', + 'synchronize event by privileged user without label creates awaiting checkrun', () async { githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( - action: 'opened', + action: 'synchronize', + login: 'test-flutter-hacker', withCicdLabel: false, ); @@ -4007,16 +4054,17 @@ void foo() { output: anyNamed('output'), ), ).called(1); + expect(scheduler.triggerPresubmitTargetsCnt, 0); }, ); test( - 'opened PR with CICD label STILL creates "Awaiting CICD label" checkrun if not exists', + 'opened PR without CICD label creates "Awaiting CICD label" checkrun', () async { githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( action: 'opened', - withCicdLabel: true, + withCicdLabel: false, ); await tester.post(webhook); @@ -4034,12 +4082,12 @@ void foo() { ); test( - 'edited PR without CICD label creates "Awaiting CICD label" checkrun if not exists', + 'opened PR with CICD label STILL creates "Awaiting CICD label" checkrun if not exists', () async { githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}'; tester.message = generateGithubWebhookMessage( - action: 'edited', - withCicdLabel: false, + action: 'opened', + withCicdLabel: true, ); await tester.post(webhook); diff --git a/packages/cocoon_integration_test/lib/src/utilities/mocks.dart b/packages/cocoon_integration_test/lib/src/utilities/mocks.dart index a5ccfc38a..67878c7d3 100644 --- a/packages/cocoon_integration_test/lib/src/utilities/mocks.dart +++ b/packages/cocoon_integration_test/lib/src/utilities/mocks.dart @@ -6,7 +6,6 @@ import 'dart:io'; import 'dart:typed_data'; import 'package:cocoon_service/src/foundation/github_checks_util.dart'; -import 'package:cocoon_service/src/request_handlers/github/webhook_subscription.dart'; import 'package:cocoon_service/src/service/access_token_provider.dart'; import 'package:cocoon_service/src/service/big_query.dart'; import 'package:cocoon_service/src/service/branch_service.dart'; @@ -17,6 +16,7 @@ import 'package:cocoon_service/src/service/discord_service.dart'; import 'package:cocoon_service/src/service/github_checks_service.dart'; import 'package:cocoon_service/src/service/github_service.dart'; import 'package:cocoon_service/src/service/luci_build_service.dart'; +import 'package:cocoon_service/src/service/pull_request_manager.dart'; import 'package:cocoon_service/src/service/scheduler.dart'; import 'package:github/github.dart'; import 'package:googleapis/bigquery/v2.dart'; From a927ff336c622117ee5c581ffd4f2aa2cd19bfde Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Tue, 28 Apr 2026 17:13:34 -0700 Subject: [PATCH 2/4] Look up by document ID instead of a filter query. --- .../lib/src/service/pull_request_manager.dart | 62 +++++++------------ 1 file changed, 22 insertions(+), 40 deletions(-) diff --git a/app_dart/lib/src/service/pull_request_manager.dart b/app_dart/lib/src/service/pull_request_manager.dart index df71d10cc..dd5f3a9f9 100644 --- a/app_dart/lib/src/service/pull_request_manager.dart +++ b/app_dart/lib/src/service/pull_request_manager.dart @@ -4,7 +4,6 @@ import 'dart:async'; import 'dart:collection'; -import 'dart:convert'; import 'dart:io'; import 'package:cocoon_common/is_release_branch.dart'; @@ -75,24 +74,25 @@ class PullRequestManager { /// Kicks off asynchronous initialization in the queue. void _initialize(PullRequestEvent event) { _addEvent(() async { - final filterMap = { - 'slug =': json.encode(slug.toJson()), - 'number =': prNumber, - }; - final docs = await firestore.query( - PullRequestState.kCollectionId, - filterMap, - ); + final documentId = PullRequestState.getDocumentId(slug, prNumber); + final name = + '$kDocumentParent/${PullRequestState.kCollectionId}/$documentId'; - if (docs.isNotEmpty) { - final state = PullRequestState.fromDocument(docs.first); + try { + final doc = await firestore.getDocument(name); + final state = PullRequestState.fromDocument(doc); _isPrivileged = state.isPrivileged ?? false; _latestSha = state.latestSha ?? event.pullRequest!.head!.sha!; _scheduledSha = state.scheduledSha; log.info( 'Hydrated PullRequestManager for $slug/$prNumber: isPrivileged=$_isPrivileged, latestSha=$_latestSha, scheduledSha=$_scheduledSha', ); - } else { + } on DetailedApiRequestError catch (e) { + if (e.status != HttpStatus.notFound) { + rethrow; + } + + // Document not found, initialize as new final pr = event.pullRequest!; final author = pr.user!.login!; final githubService = await config.createGithubService(slug); @@ -105,16 +105,18 @@ class PullRequestManager { _isPrivileged = isRoller || isFlutterHacker; _latestSha = pr.head!.sha!; - // Persist initial state + // Persist initial state using the specific document ID! final state = PullRequestState() ..slug = slug ..number = prNumber ..isPrivileged = _isPrivileged ..latestSha = _latestSha; + final document = Document(fields: state.fields); await firestore.createDocument( document, collectionId: PullRequestState.kCollectionId, + documentId: documentId, ); log.info('Created initial PullRequestState for $slug/$prNumber'); } @@ -130,14 +132,9 @@ class PullRequestManager { /// Persists the current state to Firestore. Future persist() async { - final filterMap = { - 'slug =': json.encode(slug.toJson()), - 'number =': prNumber, - }; - final docs = await firestore.query( - PullRequestState.kCollectionId, - filterMap, - ); + final documentId = PullRequestState.getDocumentId(slug, prNumber); + final name = + '$kDocumentParent/${PullRequestState.kCollectionId}/$documentId'; final state = PullRequestState() ..slug = slug @@ -146,25 +143,10 @@ class PullRequestManager { ..latestSha = latestSha ..scheduledSha = _scheduledSha; - if (docs.isEmpty) { - // Create - final document = Document(fields: state.fields); - await firestore.createDocument( - document, - collectionId: PullRequestState.kCollectionId, - ); - log.info('Created PullRequestState for $slug/$prNumber'); - } else { - // Update - final existing = PullRequestState.fromDocument(docs.first); - existing.isPrivileged = _isPrivileged; - existing.latestSha = latestSha; - existing.scheduledSha = _scheduledSha; - - final tx = await firestore.beginTransaction(); - await firestore.commit(tx, documentsToWrites([existing])); - log.info('Updated PullRequestState for $slug/$prNumber'); - } + final document = Document(name: name, fields: state.fields); + + await firestore.writeViaTransaction(documentsToWrites([document])); + log.info('Persisted PullRequestState for $slug/$prNumber'); } /// The latest SHA that we have processed for this PR. From 5c6df6a458e6f343048a9d1d64b60d712736eb6b Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Fri, 1 May 2026 10:25:16 -0700 Subject: [PATCH 3/4] Use redis to ensure serialized handling of PRs across instances. --- .../github/webhook_subscription.dart | 45 +- .../lib/src/service/pull_request_manager.dart | 527 +++++++++++------- .../github/webhook_subscription_test.dart | 30 +- 3 files changed, 377 insertions(+), 225 deletions(-) diff --git a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart index e22cf5b94..5509741df 100644 --- a/app_dart/lib/src/request_handlers/github/webhook_subscription.dart +++ b/app_dart/lib/src/request_handlers/github/webhook_subscription.dart @@ -48,18 +48,9 @@ final class GithubWebhookSubscription extends SubscriptionHandler { @visibleForTesting DateTime Function() now = DateTime.now, // Gets the initial github events from this sub after the webhook uploads them. }) : _now = now, - prManagerCache = PullRequestManagerCache( - capacity: 100, - firestore: firestore, - config: config, - scheduler: scheduler, - gerritService: gerritService, - pullRequestLabelProcessorProvider: pullRequestLabelProcessorProvider, - ), super(subscriptionName: 'github-webhooks-sub'); final DateTime Function() _now; - final PullRequestManagerCache prManagerCache; /// Cocoon scheduler to trigger tasks against changes from GitHub. final Scheduler scheduler; @@ -170,25 +161,34 @@ final class GithubWebhookSubscription extends SubscriptionHandler { throw const InternalServerError('Unsupported repository'); } + final context = PullRequestEventContext( + cache: cache, + firestore: firestore, + config: config, + scheduler: scheduler, + gerritService: gerritService, + pullRequestLabelProcessorProvider: pullRequestLabelProcessorProvider, + ); + // See the API reference: // https://developer.github.com/v3/activity/events/types/#pullrequestevent // which unfortunately is a bit light on explanations. log.info('$crumb: processing $eventAction for ${pr.htmlUrl}'); switch (eventAction) { case 'closed': - final result = await _processPullRequestClosed(pullRequestEvent); - return result.toResponse(); + return await PullRequestManager.handleClosed( + pullRequestEvent, + context, + _processPullRequestClosed, + ); case 'edited': - final manager = prManagerCache.getOrCreate(pullRequestEvent); - await manager.handleEdited(pullRequestEvent); + await PullRequestManager.handleEdited(pullRequestEvent, context); break; case 'opened': - final manager = prManagerCache.getOrCreate(pullRequestEvent); - await manager.handleOpened(pullRequestEvent); + await PullRequestManager.handleOpened(pullRequestEvent, context); break; case 'reopened': - final manager = prManagerCache.getOrCreate(pullRequestEvent); - await manager.handleReopened(pullRequestEvent); + await PullRequestManager.handleReopened(pullRequestEvent, context); break; case 'labeled': log.info( @@ -197,15 +197,18 @@ final class GithubWebhookSubscription extends SubscriptionHandler { final labelEvent = _getLabeledEvent(rawRequest); final labelName = labelEvent?.label.name; final labelId = labelEvent?.label.id; - final manager = prManagerCache.getOrCreate(pullRequestEvent); - await manager.handleLabeled(pullRequestEvent, labelName, labelId); + await PullRequestManager.handleLabeled( + pullRequestEvent, + context, + labelName, + labelId, + ); break; case 'dequeued': await _respondToPullRequestDequeued(pullRequestEvent); break; case 'synchronize': - final manager = prManagerCache.getOrCreate(pullRequestEvent); - await manager.handleSynchronize(pullRequestEvent); + await PullRequestManager.handleSynchronize(pullRequestEvent, context); break; // Ignore the rest of the events. case 'ready_for_review': diff --git a/app_dart/lib/src/service/pull_request_manager.dart b/app_dart/lib/src/service/pull_request_manager.dart index dd5f3a9f9..9d4b01955 100644 --- a/app_dart/lib/src/service/pull_request_manager.dart +++ b/app_dart/lib/src/service/pull_request_manager.dart @@ -5,6 +5,7 @@ import 'dart:async'; import 'dart:collection'; import 'dart:io'; +import 'dart:typed_data'; import 'package:cocoon_common/is_release_branch.dart'; import 'package:cocoon_server/logging.dart'; @@ -14,11 +15,15 @@ import 'package:googleapis/firestore/v1.dart' hide Value; import '../model/firestore/pr_check_runs.dart'; import '../model/firestore/pull_request_state.dart'; +import '../request_handling/exceptions.dart'; +import '../request_handling/response.dart'; +import 'cache_service.dart'; import 'config.dart'; import 'firestore.dart'; import 'gerrit_service.dart'; import 'github_service.dart'; import 'scheduler.dart'; +import 'scheduler/process_check_run_result.dart'; // Filenames which are not actually tests. const List kNotActuallyATest = [ @@ -55,11 +60,10 @@ class PullRequestManager { final GerritService gerritService; final PullRequestLabelProcessorProvider pullRequestLabelProcessorProvider; - final PullRequestOperationQueue _queue = PullRequestOperationQueue(); - - late final bool _isPrivileged; - late String _latestSha; + final bool _isPrivileged; + String _latestSha; String? _scheduledSha; + bool _isDirty = false; PullRequestManager._({ required this.slug, @@ -69,69 +73,273 @@ class PullRequestManager { required this.scheduler, required this.gerritService, required this.pullRequestLabelProcessorProvider, - }); + required bool isPrivileged, + required String latestSha, + String? scheduledSha, + }) : _isPrivileged = isPrivileged, + _latestSha = latestSha, + _scheduledSha = scheduledSha; + + /// Runs an action with a distributed lock on the pull request. + static Future _runWithLock({ + required PullRequestEvent event, + required CacheService cache, + required Future Function() action, + }) async { + final pr = event.pullRequest!; + final slug = event.repository!.slug(); + final prNumber = pr.number!; + final lockKey = 'pr_lock_${slug.owner}_${slug.name}_$prNumber'; + final lockValue = Uint8List.fromList('l'.codeUnits); + + // Attempt to acquire lock + final existingLock = await cache.getOrCreate( + 'pr_locks', + lockKey, + createFn: null, + ); + if (existingLock != null) { + throw const ServiceUnavailable('PR is locked by another instance'); + } + await cache.set( + 'pr_locks', + lockKey, + lockValue, + ttl: const Duration(minutes: 5), + ); + + try { + return await action(); + } finally { + await cache.purge('pr_locks', lockKey); + } + } - /// Kicks off asynchronous initialization in the queue. - void _initialize(PullRequestEvent event) { - _addEvent(() async { - final documentId = PullRequestState.getDocumentId(slug, prNumber); - final name = - '$kDocumentParent/${PullRequestState.kCollectionId}/$documentId'; - - try { - final doc = await firestore.getDocument(name); - final state = PullRequestState.fromDocument(doc); - _isPrivileged = state.isPrivileged ?? false; - _latestSha = state.latestSha ?? event.pullRequest!.head!.sha!; - _scheduledSha = state.scheduledSha; - log.info( - 'Hydrated PullRequestManager for $slug/$prNumber: isPrivileged=$_isPrivileged, latestSha=$_latestSha, scheduledSha=$_scheduledSha', + static Future handleOpened( + PullRequestEvent event, + PullRequestEventContext context, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, ); - } on DetailedApiRequestError catch (e) { - if (e.status != HttpStatus.notFound) { - rethrow; - } - - // Document not found, initialize as new - final pr = event.pullRequest!; - final author = pr.user!.login!; - final githubService = await config.createGithubService(slug); - final isRoller = config.rollerAccounts.contains(author); - final isFlutterHacker = await githubService.isTeamMember( - 'flutter-hackers', - author, - slug.owner, + await manager._handleOpened(event); + await manager.persist(); + }, + ); + } + + static Future handleSynchronize( + PullRequestEvent event, + PullRequestEventContext context, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, ); - _isPrivileged = isRoller || isFlutterHacker; - _latestSha = pr.head!.sha!; - - // Persist initial state using the specific document ID! - final state = PullRequestState() - ..slug = slug - ..number = prNumber - ..isPrivileged = _isPrivileged - ..latestSha = _latestSha; - - final document = Document(fields: state.fields); - await firestore.createDocument( - document, - collectionId: PullRequestState.kCollectionId, - documentId: documentId, + await manager._handleSynchronize(event); + await manager.persist(); + }, + ); + } + + static Future handleEdited( + PullRequestEvent event, + PullRequestEventContext context, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, ); - log.info('Created initial PullRequestState for $slug/$prNumber'); - } - }); + await manager._handleEdited(event); + await manager.persist(); + }, + ); } - bool get isQueueEmpty => _queue.isEmpty; + static Future handleReopened( + PullRequestEvent event, + PullRequestEventContext context, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, + ); + await manager._handleReopened(event); + await manager.persist(); + }, + ); + } - /// Adds an event to the queue to be processed sequentially. - Future _addEvent(Future Function() operation) { - return _queue.add(operation); + static Future handleLabeled( + PullRequestEvent event, + PullRequestEventContext context, + String? labelName, + int? labelId, + ) async { + await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final manager = await PullRequestManager._create( + slug: event.repository!.slug(), + prNumber: event.pullRequest!.number!, + firestore: context.firestore, + config: context.config, + scheduler: context.scheduler, + gerritService: context.gerritService, + pullRequestLabelProcessorProvider: + context.pullRequestLabelProcessorProvider, + event: event, + ); + await manager._handleLabeled(event, labelName, labelId); + await manager.persist(); + }, + ); + } + + static Future handleClosed( + PullRequestEvent event, + PullRequestEventContext context, + Future Function(PullRequestEvent) processClosedFn, + ) async { + return await _runWithLock( + event: event, + cache: context.cache, + action: () async { + final result = await processClosedFn(event); + return result.toResponse(); + }, + ); } - /// Persists the current state to Firestore. + /// Creates and hydrates a [PullRequestManager] instance. + static Future _create({ + required RepositorySlug slug, + required int prNumber, + required FirestoreService firestore, + required Config config, + required Scheduler scheduler, + required GerritService gerritService, + required PullRequestLabelProcessorProvider + pullRequestLabelProcessorProvider, + required PullRequestEvent event, + }) async { + final documentId = PullRequestState.getDocumentId(slug, prNumber); + final name = + '$kDocumentParent/${PullRequestState.kCollectionId}/$documentId'; + + bool isPrivileged; + String latestSha; + String? scheduledSha; + + try { + final doc = await firestore.getDocument(name); + final state = PullRequestState.fromDocument(doc); + isPrivileged = state.isPrivileged ?? false; + latestSha = state.latestSha ?? event.pullRequest!.head!.sha!; + scheduledSha = state.scheduledSha; + log.info( + 'Hydrated PullRequestManager for $slug/$prNumber: isPrivileged=$isPrivileged, latestSha=$latestSha, scheduledSha=$scheduledSha', + ); + } on DetailedApiRequestError catch (e) { + if (e.status != HttpStatus.notFound) { + rethrow; + } + + // Document not found, initialize as new + final pr = event.pullRequest!; + final author = pr.user!.login!; + final githubService = await config.createGithubService(slug); + final isRoller = config.rollerAccounts.contains(author); + final isFlutterHacker = await githubService.isTeamMember( + 'flutter-hackers', + author, + slug.owner, + ); + isPrivileged = isRoller || isFlutterHacker; + latestSha = pr.head!.sha!; + + // Persist initial state using the specific document ID! + final state = PullRequestState() + ..slug = slug + ..number = prNumber + ..isPrivileged = isPrivileged + ..latestSha = latestSha; + + final document = Document(fields: state.fields); + await firestore.createDocument( + document, + collectionId: PullRequestState.kCollectionId, + documentId: documentId, + ); + log.info('Created initial PullRequestState for $slug/$prNumber'); + } + + return PullRequestManager._( + slug: slug, + prNumber: prNumber, + firestore: firestore, + config: config, + scheduler: scheduler, + gerritService: gerritService, + pullRequestLabelProcessorProvider: pullRequestLabelProcessorProvider, + isPrivileged: isPrivileged, + latestSha: latestSha, + scheduledSha: scheduledSha, + ); + } + + /// Persists the current state to Firestore if modified. Future persist() async { + if (!_isDirty) { + return; + } + final documentId = PullRequestState.getDocumentId(slug, prNumber); final name = '$kDocumentParent/${PullRequestState.kCollectionId}/$documentId'; @@ -147,6 +355,7 @@ class PullRequestManager { await firestore.writeViaTransaction(documentsToWrites([document])); log.info('Persisted PullRequestState for $slug/$prNumber'); + _isDirty = false; } /// The latest SHA that we have processed for this PR. @@ -154,7 +363,7 @@ class PullRequestManager { set latestSha(String value) { if (_latestSha != value) { _latestSha = value; - _addEvent(persist); + _isDirty = true; } } @@ -163,97 +372,86 @@ class PullRequestManager { set scheduledSha(String? value) { if (_scheduledSha != value) { _scheduledSha = value; - _addEvent(persist); + _isDirty = true; } } /// Handles PR opened event. - Future handleOpened(PullRequestEvent event) async { - await _addEvent(() async { - final pr = event.pullRequest!; - final sha = pr.head!.sha!; - latestSha = sha; // Update latest SHA. + Future _handleOpened(PullRequestEvent event) async { + final pr = event.pullRequest!; + final sha = pr.head!.sha!; + latestSha = sha; // Update latest SHA. - if (!pr.draft! && _isPrivileged) { - final gitHubClient = await config.createGitHubClient(pullRequest: pr); - await gitHubClient.issues.addLabelsToIssue(slug, pr.number!, ['CICD']); - log.debug('Added CICD label to PR $pr.number'); - await _scheduleIfMergeable(event); - } else { - await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); - } + if (!pr.draft! && _isPrivileged) { + final gitHubClient = await config.createGitHubClient(pullRequest: pr); + await gitHubClient.issues.addLabelsToIssue(slug, pr.number!, ['CICD']); + log.debug('Added CICD label to PR $pr.number'); + await _scheduleIfMergeable(event); + } else { + await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); + } - await checkForTests(event); - await _tryReleaseApproval(event); - }); + await checkForTests(event); + await _tryReleaseApproval(event); } /// Handles PR synchronize event. - Future handleSynchronize(PullRequestEvent event) async { - await _addEvent(() async { - final pr = event.pullRequest!; - final sha = pr.head!.sha!; - latestSha = sha; // Update latest SHA. - - final hasLabel = - pr.labels?.any((IssueLabel l) => l.name == Config.kCicdLabel) ?? - false; - final isPrivilegedUser = _isPrivileged; - - if (isPrivilegedUser) { - if (hasLabel) { - await _scheduleIfMergeable(event); - } else { - await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); - } + Future _handleSynchronize(PullRequestEvent event) async { + final pr = event.pullRequest!; + final sha = pr.head!.sha!; + latestSha = sha; // Update latest SHA. + + final hasLabel = + pr.labels?.any((IssueLabel l) => l.name == Config.kCicdLabel) ?? false; + final isPrivilegedUser = _isPrivileged; + + if (isPrivilegedUser) { + if (hasLabel) { + await _scheduleIfMergeable(event); } else { - if (hasLabel) { - final githubService = await config.createGithubService(slug); - await githubService.removeLabel(slug, pr.number!, Config.kCicdLabel); - log.debug('Removed CICD label from PR ${pr.number}'); - } await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); } - }); + } else { + if (hasLabel) { + final githubService = await config.createGithubService(slug); + await githubService.removeLabel(slug, pr.number!, Config.kCicdLabel); + log.debug('Removed CICD label from PR ${pr.number}'); + } + await scheduler.createAwaitingCicdLabelCheckRun(slug, sha); + } } /// Handles PR edited event. - Future handleEdited(PullRequestEvent event) async { - await _addEvent(() async { - await checkForTests(event); - }); + Future _handleEdited(PullRequestEvent event) async { + await checkForTests(event); } /// Handles PR reopened event. - Future handleReopened(PullRequestEvent event) async { - await _addEvent(() async { - final pr = event.pullRequest!; - await _warnThatANewCommitIsNeeded(event); - await _processLabels(pr); - await _updatePullRequest(pr); - }); + Future _handleReopened(PullRequestEvent event) async { + final pr = event.pullRequest!; + await _warnThatANewCommitIsNeeded(event); + await _processLabels(pr); + await _updatePullRequest(pr); } /// Handles PR labeled event. - Future handleLabeled( + Future _handleLabeled( PullRequestEvent event, String? labelName, int? labelId, ) async { - await _addEvent(() async { - final pr = event.pullRequest!; - final sha = pr.head!.sha!; + final pr = event.pullRequest!; + final sha = pr.head!.sha!; - if (labelName == Config.kCicdLabel && - Config.kCicdLabelIds.contains(labelId)) { - log.info('new CICD label added - scheduling tests'); - await scheduler.resolveAwaitingCicdLabelCheckRun(slug, sha); - await _scheduleIfMergeable(event); - } + if (labelName == Config.kCicdLabel && + Config.kCicdLabelIds.contains(labelId)) { + log.info('new CICD label added - scheduling tests'); + await scheduler.resolveAwaitingCicdLabelCheckRun(slug, sha); + await _scheduleIfMergeable(event); + } - await _processLabels(pr); - await _updatePullRequest(pr); - }); + await _processLabels(pr); + await _updatePullRequest(pr); } Future checkForTests(PullRequestEvent pullRequestEvent) async { @@ -800,99 +998,22 @@ class PullRequestManager { } } -/// A helper to ensure sequential execution of asynchronous tasks. -class PullRequestOperationQueue { - Future _currentOperation = Future.value(); - int _pendingOperations = 0; - - bool get isEmpty => _pendingOperations == 0; - - Future add(Future Function() operation) { - final completer = Completer(); - _pendingOperations++; - _currentOperation = _currentOperation.then((_) async { - try { - await operation(); - completer.complete(); - } catch (e, s) { - log.warn('Error in PullRequestOperationQueue', e, s); - completer.completeError(e, s); - } finally { - _pendingOperations--; - } - }); - return completer.future; - } -} - -/// LRU Cache for [PullRequestManager] instances. -class PullRequestManagerCache { - final int capacity; +class PullRequestEventContext { + final CacheService cache; final FirestoreService firestore; final Config config; final Scheduler scheduler; final GerritService gerritService; final PullRequestLabelProcessorProvider pullRequestLabelProcessorProvider; - final Map _cache = {}; - PullRequestManagerCache({ - required this.capacity, + PullRequestEventContext({ + required this.cache, required this.firestore, required this.config, required this.scheduler, required this.gerritService, required this.pullRequestLabelProcessorProvider, }); - - PullRequestManager getOrCreate(PullRequestEvent event) { - final pr = event.pullRequest!; - final slug = event.repository!.slug(); - final prNumber = pr.number!; - - final key = '${slug.owner}_${slug.name}_$prNumber'; - - // 1. Check cache - final manager = _cache.remove(key); - if (manager != null) { - _cache[key] = manager; // Move to end - return manager; - } - - // 2. Create new synchronously - final newManager = PullRequestManager._( - slug: slug, - prNumber: prNumber, - firestore: firestore, - config: config, - scheduler: scheduler, - gerritService: gerritService, - pullRequestLabelProcessorProvider: pullRequestLabelProcessorProvider, - ); - - newManager._initialize(event); - - // Eviction logic - if (_cache.length >= capacity) { - // Try to evict the least recently used - String? keyToEvict; - for (final k in _cache.keys) { - final m = _cache[k]!; - if (m.isQueueEmpty) { - keyToEvict = k; - break; - } - } - if (keyToEvict != null) { - _cache.remove(keyToEvict); - log.info('Evicted PullRequestManager for $keyToEvict'); - } else { - log.warn('Cache full, but all managers are busy. Not evicting.'); - } - } - - _cache[key] = newManager; - return newManager; - } } typedef PullRequestLabelProcessorProvider = diff --git a/app_dart/test/request_handlers/github/webhook_subscription_test.dart b/app_dart/test/request_handlers/github/webhook_subscription_test.dart index 460f911df..27b32ced9 100644 --- a/app_dart/test/request_handlers/github/webhook_subscription_test.dart +++ b/app_dart/test/request_handlers/github/webhook_subscription_test.dart @@ -4,6 +4,7 @@ import 'dart:async'; import 'dart:io'; +import 'dart:typed_data'; import 'package:buildbucket/buildbucket_pb.dart' as bbv2; import 'package:cocoon_common/core_extensions.dart'; @@ -47,6 +48,7 @@ void main() { useTestLoggerPerTest(); late GithubWebhookSubscription webhook; + late CacheService cache; late FakeBuildBucketClient fakeBuildBucketClient; late FakeConfig config; late FakeGithubService githubService; @@ -163,9 +165,10 @@ void main() { gerritService = FakeGerritService(); fakeNow = DateTime.now(); + cache = CacheService(inMemory: true); webhook = GithubWebhookSubscription( config: config, - cache: CacheService(inMemory: true), + cache: cache, gerritService: gerritService, scheduler: scheduler, commitService: commitService, @@ -4144,4 +4147,29 @@ void foo() { }, ); }); + + group('Redis locks', () { + test('fails with 503 when PR is locked', () async { + final slug = Config.flutterSlug; + const prNumber = 123; + final lockKey = 'pr_lock_${slug.owner}_${slug.name}_$prNumber'; + final lockValue = Uint8List.fromList('l'.codeUnits); + + // Pre-populate cache to simulate lock held by another instance + await cache.set('pr_locks', lockKey, lockValue); + + tester.message = generateGithubWebhookMessage( + action: 'opened', + number: prNumber, + ); + + try { + await tester.post(webhook); + fail('Should have thrown ServiceUnavailable'); + } on ServiceUnavailable catch (e) { + expect(e.statusCode, HttpStatus.serviceUnavailable); + expect(e.message, contains('PR is locked by another instance')); + } + }); + }); } From 51662065335c9c02d82f4c9f403772d9bc74bd3c Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Fri, 1 May 2026 10:29:54 -0700 Subject: [PATCH 4/4] Add alternative diagram and fix some other comments from codefu. --- app_dart/docs/cicd_flowchart.md | 29 +++++++++++++++++++ .../model/firestore/pull_request_state.dart | 10 +++---- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/app_dart/docs/cicd_flowchart.md b/app_dart/docs/cicd_flowchart.md index bdd1eb6f0..79a54c72c 100644 --- a/app_dart/docs/cicd_flowchart.md +++ b/app_dart/docs/cicd_flowchart.md @@ -31,3 +31,32 @@ flowchart TD Label_Present -- Yes --> Start_Pre Label_Present -- No --> Create_Awaiting ``` + +## Alternative State Machine Diagram (PlantUML) + +```plantuml +@startuml +state PR { + state Waiting + Waiting : entry / Create Awaiting Check Run + Waiting : exit / resolve(awaiting check) + + state Running + Running: entry/ Start Presubmits + + state if_priv2 <> + Running --> if_priv2: onPushed + if_priv2 --> Running: isPrivileged && labeled(CICD) + if_priv2 --> Waiting : default: remove(CICD) + + state if_draft <> + PR --> if_draft: openned + + if_draft --> Waiting : isDraft + if_draft --> Running: isPrivileged + if_draft --> Waiting : default + + Waiting --> Running: labeled(CICD) +} +@enduml +``` diff --git a/app_dart/lib/src/model/firestore/pull_request_state.dart b/app_dart/lib/src/model/firestore/pull_request_state.dart index 075a793d4..0e5e2c256 100644 --- a/app_dart/lib/src/model/firestore/pull_request_state.dart +++ b/app_dart/lib/src/model/firestore/pull_request_state.dart @@ -40,7 +40,7 @@ final class PullRequestState extends AppDocument { set isPrivileged(bool? value) { if (value != null) { - fields[kIsPrivilegedField] = Value(booleanValue: value); + fields[kIsPrivilegedField] = value.toValue(); } } @@ -49,7 +49,7 @@ final class PullRequestState extends AppDocument { set latestSha(String? value) { if (value != null) { - fields[kLatestShaField] = Value(stringValue: value); + fields[kLatestShaField] = value.toValue(); } } @@ -58,7 +58,7 @@ final class PullRequestState extends AppDocument { set scheduledSha(String? value) { if (value != null) { - fields[kScheduledShaField] = Value(stringValue: value); + fields[kScheduledShaField] = value.toValue(); } } @@ -75,11 +75,11 @@ final class PullRequestState extends AppDocument { int get number => int.parse(fields[kNumberField]!.integerValue!); set number(int value) { - fields[kNumberField] = Value(integerValue: '$value'); + fields[kNumberField] = value.toValue(); } /// Generates the document ID for a PR. static String getDocumentId(RepositorySlug slug, int number) { - return '${slug.owner}_${slug.name}_$number'; + return '${slug.owner}\u001F${slug.name}\u001F$number'; } }