Refactor CICD label management to use per-PR sequential queue#5035
Open
eyebrowsoffire wants to merge 4 commits into
Open
Refactor CICD label management to use per-PR sequential queue#5035eyebrowsoffire wants to merge 4 commits into
eyebrowsoffire wants to merge 4 commits into
Conversation
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.
jtmcdole
requested changes
Apr 30, 2026
Member
jtmcdole
left a comment
jtmcdole
left a comment
There was a problem hiding this comment.
I am still working through this, but wanted to post some early comments.
jtmcdole
requested changes
May 27, 2026
Member
jtmcdole
left a comment
jtmcdole
left a comment
There was a problem hiding this comment.
Really just one change, the UUID get/set in one step. LGTM otherwise
| set isPrivileged(bool? value) { | ||
| if (value != null) { | ||
| fields[kIsPrivilegedField] = value.toValue(); | ||
| } |
Member
There was a problem hiding this comment.
I don't know when we'd use it, but would you want:
} else {
fields.remove(kIsPrivilegedField);
}We could do that for kLatestShaField and kScheduledShaField as well - again, not sure when we'd use it, but maybe worth having?
| final lockValue = Uint8List.fromList('l'.codeUnits); | ||
|
|
||
| // Attempt to acquire lock | ||
| final existingLock = await cache.getOrCreate( |
Member
There was a problem hiding this comment.
I had to go read getOrCreateWithLocking() to realize it is only "locked" for the local instance, and not across multiple instances.
The dashboard relies on uuid; so you could:
// uuid: ^4.5.3
final uuid = Uuid();
final value = Uuid.parseAsByteList((uuid.v4()));
final existing = await cache.getOrCreate('pr_locks', lockKey, createFn: () => value, ttl: const Duration(minutes: 5));
/// I DO NOT KNOW IF THIS IS TRUE: set() doesn't have good dartdoc and I don't know if the value that is set is returned.
if (existingLock != value) {
throw const ServiceUnavailable('PR is locked by another instance');
}
// no need to call "set()"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change overhauls the management of the
CICDlabel in Cocoon to address race conditions and non-idempotent operations that caused issues in the previous attempt.Key changes:
PullRequestManagerto handle events for a single PR sequentially via an operation queue.webhook_subscription.darttoPullRequestManager, thinning out the webhook handler.openedandsynchronizeevents to handle privileged users and draft PRs correctly._scheduleIfMergeableidempotent by trackingscheduledShain Firestore to prevent duplicate triggering of presubmits for the same SHA.PullRequestManagercreation to be synchronous in the cache, queueing the asynchronous initialization to avoid creation race conditions.cicd_flowchart.mdfrom a previous commit to document the flow.