diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs index 8d125b8ea..e0fba2b7e 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/MockGitHubEventClient.cs @@ -115,6 +115,25 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Tests return Task.FromResult(numUpdates); } + /// + /// The mock ProcessPendingScheduledUpdates just returns the number of updates + /// + /// integer,the number of pending updates that would be processed + public override Task ProcessPendingScheduledUpdates() + { + int numUpdates = 0; + Console.WriteLine($"ProcessPendingScheduledUpdates::ProcessPendingUpdates, number of pending comments = {_gitHubComments.Count}"); + numUpdates += _gitHubComments.Count; + + Console.WriteLine($"ProcessPendingScheduledUpdates::ProcessPendingUpdates, number of pending IssueUpdates = {_gitHubIssuesToUpdate.Count}"); + numUpdates += _gitHubIssuesToUpdate.Count; + + Console.WriteLine($"ProcessPendingScheduledUpdates::ProcessPendingUpdates, number of issues to Lock = {_gitHubIssuesToLock.Count}"); + numUpdates += _gitHubIssuesToLock.Count; + + return Task.FromResult(numUpdates); + } + /// /// IsUserCollaborator override. Returns IsCollaboratorReturn value /// diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/Static/ScheduledEventProcessingTests.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/Static/ScheduledEventProcessingTests.cs index c6ce83a1d..3aa8c4f99 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/Static/ScheduledEventProcessingTests.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor.Tests/Static/ScheduledEventProcessingTests.cs @@ -43,7 +43,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Tests.Static mockGitHubEventClient.CreateSearchIssuesResult(expectedUpdates, scheduledEventPayload.Repository, ItemState.Open); await ScheduledEventProcessing.CloseAddressedIssues(mockGitHubEventClient, scheduledEventPayload); - var totalUpdates = await mockGitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); + var totalUpdates = await mockGitHubEventClient.ProcessPendingScheduledUpdates(); // Verify the RuleCheck Assert.AreEqual(ruleState == RuleState.On, mockGitHubEventClient.RulesConfiguration.RuleEnabled(rule), $"Rule '{rule}' enabled should have been {ruleState == RuleState.On} but RuleEnabled returned {ruleState != RuleState.On}.'"); if (RuleState.On == ruleState) @@ -86,7 +86,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Tests.Static mockGitHubEventClient.CreateSearchIssuesResult(expectedUpdates, scheduledEventPayload.Repository, ItemState.Open); await ScheduledEventProcessing.CloseStaleIssues(mockGitHubEventClient, scheduledEventPayload); - var totalUpdates = await mockGitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); + var totalUpdates = await mockGitHubEventClient.ProcessPendingScheduledUpdates(); // Verify the RuleCheck Assert.AreEqual(ruleState == RuleState.On, mockGitHubEventClient.RulesConfiguration.RuleEnabled(rule), $"Rule '{rule}' enabled should have been {ruleState == RuleState.On} but RuleEnabled returned {ruleState != RuleState.On}.'"); if (RuleState.On == ruleState) @@ -128,7 +128,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Tests.Static mockGitHubEventClient.CreateSearchIssuesResult(expectedUpdates, scheduledEventPayload.Repository, ItemState.Open); await ScheduledEventProcessing.CloseStalePullRequests(mockGitHubEventClient, scheduledEventPayload); - var totalUpdates = await mockGitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); + var totalUpdates = await mockGitHubEventClient.ProcessPendingScheduledUpdates(); // Verify the RuleCheck Assert.AreEqual(ruleState == RuleState.On, mockGitHubEventClient.RulesConfiguration.RuleEnabled(rule), $"Rule '{rule}' enabled should have been {ruleState == RuleState.On} but RuleEnabled returned {ruleState != RuleState.On}.'"); if (RuleState.On == ruleState) @@ -173,7 +173,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Tests.Static mockGitHubEventClient.CreateSearchIssuesResult(expectedUpdates, scheduledEventPayload.Repository, ItemState.Open); await ScheduledEventProcessing.IdentifyStalePullRequests(mockGitHubEventClient, scheduledEventPayload); - var totalUpdates = await mockGitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); + var totalUpdates = await mockGitHubEventClient.ProcessPendingScheduledUpdates(); // Verify the RuleCheck Assert.AreEqual(ruleState == RuleState.On, mockGitHubEventClient.RulesConfiguration.RuleEnabled(rule), $"Rule '{rule}' enabled should have been {ruleState == RuleState.On} but RuleEnabled returned {ruleState != RuleState.On}.'"); if (RuleState.On == ruleState) @@ -218,7 +218,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Tests.Static mockGitHubEventClient.CreateSearchIssuesResult(expectedUpdates, scheduledEventPayload.Repository, ItemState.Open); await ScheduledEventProcessing.IdentifyStaleIssues(mockGitHubEventClient, scheduledEventPayload); - var totalUpdates = await mockGitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); + var totalUpdates = await mockGitHubEventClient.ProcessPendingScheduledUpdates(); // Verify the RuleCheck Assert.AreEqual(ruleState == RuleState.On, mockGitHubEventClient.RulesConfiguration.RuleEnabled(rule), $"Rule '{rule}' enabled should have been {ruleState == RuleState.On} but RuleEnabled returned {ruleState != RuleState.On}.'"); if (RuleState.On == ruleState) @@ -261,7 +261,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Tests.Static mockGitHubEventClient.CreateSearchIssuesResult(expectedUpdates, scheduledEventPayload.Repository, ItemState.Open); await ScheduledEventProcessing.LockClosedIssues(mockGitHubEventClient, scheduledEventPayload); - var totalUpdates = await mockGitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); + var totalUpdates = await mockGitHubEventClient.ProcessPendingScheduledUpdates(); // Verify the RuleCheck Assert.AreEqual(ruleState == RuleState.On, mockGitHubEventClient.RulesConfiguration.RuleEnabled(rule), $"Rule '{rule}' enabled should have been {ruleState == RuleState.On} but RuleEnabled returned {ruleState != RuleState.On}.'"); if (RuleState.On == ruleState) @@ -303,7 +303,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Tests.Static mockGitHubEventClient.CreateSearchIssuesResult(expectedUpdates, scheduledEventPayload.Repository, ItemState.Open); await ScheduledEventProcessing.EnforceMaxLifeOfIssues(mockGitHubEventClient, scheduledEventPayload); - var totalUpdates = await mockGitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); + var totalUpdates = await mockGitHubEventClient.ProcessPendingScheduledUpdates(); // Verify the RuleCheck Assert.AreEqual(ruleState == RuleState.On, mockGitHubEventClient.RulesConfiguration.RuleEnabled(rule), $"Rule '{rule}' enabled should have been {ruleState == RuleState.On} but RuleEnabled returned {ruleState != RuleState.On}.'"); if (RuleState.On == ruleState) diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs index e524e7bf4..f68435bbf 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/Constants/RateLimitConstants.cs @@ -11,5 +11,12 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.Constants // https://docs.github.com/en/rest/search?apiVersion=2022-11-28#about-search // The SearchIssues API has a rate limit of 1000 results which resets every 60 seconds public const int SearchIssuesRateLimit = 1000; + // https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28#about-primary-rate-limits + // There's a 500/hour limit on content creation. In theory, Closing an issue, Locking an issue and + // creating a comment are all considered content creation. + public const int ContentCreationRateLimit = 300; + // The actual rate limit per minute for content creation is 80 but to ensure that scheduled tasks + // don't interfere with Actions processing or people doing things in the GitHub UI. + public const int ScheduledUpdatesPerMinuteRateLimit = 50; } } diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/EventProcessing/ScheduledEventProcessing.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/EventProcessing/ScheduledEventProcessing.cs index 50105043d..ff0290f9d 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/EventProcessing/ScheduledEventProcessing.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/EventProcessing/ScheduledEventProcessing.cs @@ -79,7 +79,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.EventProcessing { // The second argument is IssueOrPullRequestNumber which isn't applicable to scheduled events (cron tasks) // since they're not going to be changing a single IssueUpdate like rules processing does. - await gitHubEventClient.ProcessPendingUpdates(scheduledEventPayload.Repository.Id); + await gitHubEventClient.ProcessPendingScheduledUpdates(); } } @@ -130,6 +130,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.EventProcessing ) { Issue issue = result.Items[iCounter++]; + // This rule only sets the state IssueUpdate issueUpdate = gitHubEventClient.GetIssueUpdate(issue, false); issueUpdate.State = ItemState.Closed; issueUpdate.StateReason = ItemStateReason.Completed; @@ -211,6 +212,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.EventProcessing ) { Issue issue = result.Items[iCounter++]; + // This rule only sets the state IssueUpdate issueUpdate = gitHubEventClient.GetIssueUpdate(issue, false); issueUpdate.State = ItemState.Closed; issueUpdate.StateReason = ItemStateReason.NotPlanned; @@ -285,6 +287,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.EventProcessing ) { Issue issue = result.Items[iCounter++]; + // This rule only sets the state IssueUpdate issueUpdate = gitHubEventClient.GetIssueUpdate(issue, false); issueUpdate.State = ItemState.Closed; issueUpdate.StateReason = ItemStateReason.NotPlanned; @@ -366,7 +369,8 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.EventProcessing ) { Issue issue = result.Items[iCounter++]; - IssueUpdate issueUpdate = gitHubEventClient.GetIssueUpdate(issue, false); + // This rule needs to the full IssueUpdate as it's adding a label + IssueUpdate issueUpdate = gitHubEventClient.GetIssueUpdate(issue, false, false); issueUpdate.AddLabel(TriageLabelConstants.NoRecentActivity); gitHubEventClient.AddToIssueUpdateList(scheduledEventPayload.Repository.Id, issue.Number, @@ -451,7 +455,8 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.EventProcessing ) { Issue issue = result.Items[iCounter++]; - IssueUpdate issueUpdate = gitHubEventClient.GetIssueUpdate(issue, false); + // This rule needs to the full IssueUpdate as it's adding a label + IssueUpdate issueUpdate = gitHubEventClient.GetIssueUpdate(issue, false, false); issueUpdate.AddLabel(TriageLabelConstants.NoRecentActivity); gitHubEventClient.AddToIssueUpdateList(scheduledEventPayload.Repository.Id, issue.Number, @@ -597,6 +602,7 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor.EventProcessing ) { Issue issue = result.Items[iCounter++]; + // This rule only sets the state IssueUpdate issueUpdate = gitHubEventClient.GetIssueUpdate(issue, false); // Close the issue issueUpdate.State = ItemState.Closed; diff --git a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs index c72231cf7..571c91bc7 100644 --- a/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs +++ b/tools/github-event-processor/Azure.Sdk.Tools.GitHubEventProcessor/GitHubEventClient.cs @@ -1,9 +1,11 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net; using System.Net.Http; using System.Net.Http.Json; using System.Threading.Tasks; +using System.Xml.Linq; using Azure.Sdk.Tools.CodeownersUtils.Constants; using Azure.Sdk.Tools.GitHubEventProcessor.Constants; using Azure.Sdk.Tools.GitHubEventProcessor.GitHubPayload; @@ -12,12 +14,13 @@ using Octokit; namespace Azure.Sdk.Tools.GitHubEventProcessor { + // JRS /// /// GitHubEventClient is a singleton. It holds the GitHubClient and Rules instances as well /// as any updates queued during event processing. After all the relevant rules have been processed, - /// a call to ProcessPendingUpdates will process all of the pending updates. This ensures that the - /// individual rules don't need to deal with calls to GitHub and the respective error processing, - /// within the rules, themselves. + /// a call to ProcessPendingUpdates or ProcessPendingScheduledUpdates, in the case of scheduled rules + /// will process all of the pending updates. This ensures that the individual rules don't need to deal + /// with calls to GitHub and the respective error processing, within the rules, themselves. /// public class GitHubEventClient { @@ -27,6 +30,13 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor private static readonly int MaxIssueAssignees = 10; + // Used when updating a large number of items that could cause a SecondaryRateLimit exception if + // too many are done within a minute. + private static readonly int OneMinuteInMilliseconds = 60000; + + // This is the maximum number of times certain GitHub API calls will be attempted + private static readonly int MaxNumberOfTries = 5; + /// /// Class to store the information needed to create a GitHub Comment on an Issue or PullRequest. /// @@ -176,6 +186,8 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor /// 1. IssueUpdate /// 2. Added Comments /// 3. Removed Dismissals + /// 4. Adding Assignees + /// 5. Add/Remove Labels /// /// The Id of the repository /// The Issue or PullRequest number if not processing a scheduled task. @@ -298,6 +310,268 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor return numUpdates; } + /// + /// Scheduled Updates are different from Actions updates. Scheduled jobs can cause updates to several hundred issues + /// through closing, comment creation and locking or any combination thereof. The reason why this needs to be a + /// separate function is because of the per-minute secondary rate limit. There's a cap of 80 content creation updates + /// per minute but this per-repository and affects not only Scheduled events but also actions and contentent creation + /// through the UI. + /// + /// Integer, the number of update calls made + public virtual async Task ProcessPendingScheduledUpdates() + { + Console.WriteLine("Processing pending scheduled updates..."); + int numUpdates = 0; + int numExpectedUpdates = ComputeNumberOfExpectedUpdates(); + + // The order of processing for pending updates is Comment->Close->Lock + // If any update fails, don't process further updates. + HashSet itemsToSkip = new HashSet(); + try + { + // Process any comments + for (int iCounter = 0;iCounter < _gitHubComments.Count;iCounter++) + { + numUpdates++; + if (numUpdates % RateLimitConstants.ScheduledUpdatesPerMinuteRateLimit == 0) + { + await Delay("ProcessPendingScheduledUpdates:ScheduledUpdatesPerMinuteRateLimit", OneMinuteInMilliseconds); + } + if (!await CreateGitHubComment(_gitHubComments[iCounter])) + { + if (!itemsToSkip.Contains(_gitHubComments[iCounter].IssueOrPullRequestNumber)) + { + itemsToSkip.Add(_gitHubComments[iCounter].IssueOrPullRequestNumber); + } + } + } + + // Process any Scheduled task IssueUpdates + for (int iCounter = 0; iCounter < _gitHubIssuesToUpdate.Count;iCounter++) + { + // If the previous update failed, skip this one. + if (itemsToSkip.Contains(_gitHubIssuesToUpdate[iCounter].IssueOrPRNumber)) + { + continue; + } + numUpdates++; + if (numUpdates % RateLimitConstants.ScheduledUpdatesPerMinuteRateLimit == 0) + { + await Delay("ProcessPendingScheduledUpdates:ScheduledUpdatesPerMinuteRateLimit", OneMinuteInMilliseconds); + } + if (!await UpdateGitHubIssue(_gitHubIssuesToUpdate[iCounter])) + { + if (!itemsToSkip.Contains(_gitHubIssuesToUpdate[iCounter].IssueOrPRNumber)) + { + itemsToSkip.Add(_gitHubIssuesToUpdate[iCounter].IssueOrPRNumber); + } + } + } + + // Process any issue locks last in case the issue is being updated or having a comment added + // prior to being locked + for (int iCounter = 0; iCounter < _gitHubIssuesToLock.Count; iCounter++) + { + // If the previous update failed, skip this one. + if (itemsToSkip.Contains(_gitHubIssuesToLock[iCounter].IssueNumber)) + { + continue; + } + + numUpdates++; + if (numUpdates % RateLimitConstants.ScheduledUpdatesPerMinuteRateLimit == 0) + { + await Delay("ProcessPendingScheduledUpdates:ScheduledUpdatesPerMinuteRateLimit", OneMinuteInMilliseconds); + } + + // In theory, locking should be the last operation and there should be no dupes in the lock list + // but it's better to be safe than sorry. + if (!await LockGitHubIssue(_gitHubIssuesToLock[iCounter])) + { + if (!itemsToSkip.Contains(_gitHubIssuesToLock[iCounter].IssueNumber)) + { + itemsToSkip.Add(_gitHubIssuesToLock[iCounter].IssueNumber); + } + } + } + + Console.WriteLine("Finished processing pending scheduled updates."); + } + // For the moment, nothing special is being done when rate limit exceptions are + // thrown but keep them separate in case that changes. + catch (RateLimitExceededException rateLimitEx) + { + string message = $"RateLimitExceededException was thrown processing pending updates. Total expected updates={numExpectedUpdates}, number of updates made={numUpdates}."; + Console.WriteLine(message); + Console.WriteLine(rateLimitEx); + } + catch (Exception ex) + { + string message = $"Exception was thrown processing pending updates. Total expected updates={numExpectedUpdates}, number of updates made={numUpdates}."; + Console.WriteLine(message); + Console.WriteLine(ex); + } + + return numUpdates; + } + + /// + /// Common "sleep equivalent" function. + /// + /// string, the reason why delay is being called. + /// milliseconds to delay + /// + private async Task Delay(string reasonForDelay, int millisecondsDelay) + { + Console.WriteLine($"delaying for {millisecondsDelay} milliseconds due to: {reasonForDelay}"); + await Task.Delay(millisecondsDelay); + } + + /// + /// Wrapper around Issue.Update with retries. + /// + /// GitHubIssueToUpdate instance for the issue to update + /// True if updated, false otherwise. + private async Task UpdateGitHubIssue(GitHubIssueToUpdate issueToUpdate) + { + for (int iAttempt = 1; iAttempt <= MaxNumberOfTries; iAttempt++) + { + try + { + await _gitHubClient.Issue.Update(issueToUpdate.RepositoryId, + issueToUpdate.IssueOrPRNumber, + issueToUpdate.IssueUpdate); + return true; + + } + catch (SecondaryRateLimitExceededException secondaryRateLimitEx) + { + // These are the status codes that would require a sleep. If this wasn't the last try + // then sleep for 1 minute + if ((secondaryRateLimitEx.HttpResponse.StatusCode == HttpStatusCode.Forbidden || + secondaryRateLimitEx.HttpResponse.StatusCode == HttpStatusCode.TooManyRequests) && + iAttempt < MaxNumberOfTries) + { + await Delay($"UpdateGitHubIssue:SecondaryRateLimitExceededException, HttpStatusCode={secondaryRateLimitEx.HttpResponse.StatusCode}", OneMinuteInMilliseconds); + } + else + { + string message = $"UpdateGitHubIssue:SecondaryRateLimitExceededException was thrown and there are no more retries. Issue/PR affected={issueToUpdate.IssueOrPRNumber}."; + Console.WriteLine(message); + Console.WriteLine(secondaryRateLimitEx); + } + } + catch (ApiValidationException apiValidationEx) + { + Console.WriteLine($"UpdateGitHubIssue:ApiValidationException processing IssueUpdate on {issueToUpdate.IssueOrPRNumber}. ApiValidationException={apiValidationEx}"); + break; + } + catch (Exception ex) + { + Console.WriteLine($"UpdateGitHubIssue:Exception processing IssueUpdate on {issueToUpdate.IssueOrPRNumber}. Ex={ex}"); + break; + } + + } + return false; + } + + /// + /// Wrapper around Issue.CommentCreate with retries. + /// + /// GitHubComment instance with the comment, IssueOrPullRequestNumber and repositoryId + /// True if updated, false otherwise. + private async Task CreateGitHubComment(GitHubComment comment) + { + for (int iAttempt=1;iAttempt <= MaxNumberOfTries;iAttempt++) + { + try + { + await _gitHubClient.Issue.Comment.Create(comment.RepositoryId, + comment.IssueOrPullRequestNumber, + comment.Comment); + return true; + + } + catch (SecondaryRateLimitExceededException secondaryRateLimitEx) + { + // These are the status codes that would require a sleep. If this wasn't the last try + // then sleep for 1 minute + if ((secondaryRateLimitEx.HttpResponse.StatusCode == HttpStatusCode.Forbidden || + secondaryRateLimitEx.HttpResponse.StatusCode == HttpStatusCode.TooManyRequests) && + iAttempt < MaxNumberOfTries) + { + await Delay($"CreateGitHubComment:SecondaryRateLimitExceededException, HttpStatusCode={secondaryRateLimitEx.HttpResponse.StatusCode}", OneMinuteInMilliseconds); + } + else + { + string message = $"CreateGitHubComment:SecondaryRateLimitExceededException was thrown and there are no more retries. Issue/PR affected={comment.IssueOrPullRequestNumber}."; + Console.WriteLine(message); + Console.WriteLine(secondaryRateLimitEx); + } + } + catch (ApiValidationException apiValidationEx) + { + Console.WriteLine($"CreateGitHubComment:ApiValidationException processing comment on {comment.IssueOrPullRequestNumber}. ApiValidationException={apiValidationEx}"); + break; + } + catch (Exception ex) + { + Console.WriteLine($"CreateGitHubComment:Exception processing comment on {comment.IssueOrPullRequestNumber}. Ex={ex}"); + break; + } + } + return false; + } + + /// + /// Wrapper around Issue.LockUnlock.Lock with retries + /// + /// GitHubIssueToLock instance which contains the repositoryId, issue number and lock reason. + /// True if updated, false otherwise. + private async Task LockGitHubIssue(GitHubIssueToLock issueToLock) + { + for (int iAttempt = 1; iAttempt <= MaxNumberOfTries; iAttempt++) + { + try + { + await _gitHubClient.Issue.LockUnlock.Lock(issueToLock.RepositoryId, + issueToLock.IssueNumber, + issueToLock.LockReason); + return true; + + } + catch (SecondaryRateLimitExceededException secondaryRateLimitEx) + { + // These are the status codes that would require a sleep. If this wasn't the last try + // then sleep for 1 minute + if ((secondaryRateLimitEx.HttpResponse.StatusCode == HttpStatusCode.Forbidden || + secondaryRateLimitEx.HttpResponse.StatusCode == HttpStatusCode.TooManyRequests) && + iAttempt < MaxNumberOfTries) + { + await Delay($"LockGitHubIssue:SecondaryRateLimitExceededException, HttpStatusCode={secondaryRateLimitEx.HttpResponse.StatusCode}", OneMinuteInMilliseconds); + } + else + { + string message = $"LockGitHubIssue:SecondaryRateLimitExceededException was thrown and there are no more retries. Issue affected={issueToLock.IssueNumber}."; + Console.WriteLine(message); + Console.WriteLine(secondaryRateLimitEx); + } + } + catch (ApiValidationException apiValidationEx) + { + Console.WriteLine($"LockGitHubIssue:ApiValidationException processing Lock on {issueToLock.IssueNumber}. ApiValidationException={apiValidationEx}"); + break; + } + catch (Exception ex) + { + Console.WriteLine($"LockGitHubIssue:Exception processing Lock on {issueToLock.IssueNumber}. Ex={ex}"); + break; + } + } + return false; + } + /// /// Compute and output the number of expected updates. /// @@ -357,13 +631,12 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor /// Optional message to prepend to the rate limit message. public async Task WriteRateLimits(string prependMessage = null) { - int maxTries = 5; // 200 ms. If the rate limits cannot be fetched in 1 second, there's a problem with GitHub. // Unlike scheduled events which have a longer back off period, normal event processing cannot // delay that long before retrying. int sleepDuration = 200; - for (int tryNumber = 1; tryNumber <= maxTries; tryNumber++) + for (int tryNumber = 1; tryNumber <= MaxNumberOfTries; tryNumber++) { try { @@ -382,14 +655,14 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor } catch (Exception ex) { - if (tryNumber == maxTries) + if (tryNumber == MaxNumberOfTries) { - Console.WriteLine($"Exception trying to get RateLimit from GitHub. Number of attempts, {maxTries}, exhausted. Rethrowing."); + Console.WriteLine($"Exception trying to get RateLimit from GitHub. Number of attempts, {MaxNumberOfTries}, exhausted. Rethrowing."); throw; } else { - Console.WriteLine($"Exception trying to get RateLimit from GitHub, attempt number: {tryNumber} of {maxTries}. Waiting {sleepDuration}ms before trying again."); + Console.WriteLine($"Exception trying to get RateLimit from GitHub, attempt number: {tryNumber} of {MaxNumberOfTries}. Waiting {sleepDuration}ms before trying again."); Console.WriteLine($"Exception: {ex}"); await Task.Delay(sleepDuration); } @@ -401,7 +674,11 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor /// Return the number of updates a scheduled task can make. The Core Rate Limit that GitHub Actions can make is 15000/hour /// for enterprise and 1000/hour for non-enterprise. The max number of results that can be retried from SearchIssues is 1000. /// The CoreRateLimit is set when WriteRateLimits is called and this is done at the start of processing in Main. If the core - /// rate limit is 15000, return 1000, otherwise return 100 which is 1/10th of the hourly limit for non-enterprise repository. + /// rate limit is 15000, return 300, otherwise return 100 which is 1/10th of the hourly limit for non-enterprise repository. + /// The reason for the 300 limit is that there's a cap of 500 content-generating requests per hour. 300 leaves 200 open for + /// Actions processing and other Scheduled events. Most Scheduled events are just playing catch up on items that meet their + /// criteria since they last time they ran and typically only have handful of updates. It's new Scheduled events that will + /// probably need this. /// /// The number updates a scheduled task can make. public virtual async Task ComputeScheduledTaskUpdateLimit() @@ -415,9 +692,9 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor CoreRateLimit = miscRateLimit.Resources.Core.Limit; } updateLimit = CoreRateLimit / 10; - if (updateLimit > RateLimitConstants.SearchIssuesRateLimit) + if (updateLimit > RateLimitConstants.ContentCreationRateLimit) { - updateLimit = RateLimitConstants.SearchIssuesRateLimit; + updateLimit = RateLimitConstants.ContentCreationRateLimit; } Console.WriteLine($"Setting the scheduled task update limit to: {updateLimit}"); return updateLimit; @@ -501,73 +778,81 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor /// Overloaded convenience function that'll return the IssueUpdate. Actions all make changes to /// the same, shared, IssueUpdate because they're processing on the same event. For scheduled /// event processing, there will be multiple, unique IssueUpdates and there won't be a shared one. + /// If an issue is only being used for a state change, clear out everything. Anything null, or 0 in + /// the case of the Milestone, won't get updated IIssuesClient.Update is called, only the state which + /// will be set by the rule. The reason this is necessary is that even though everything on the Issue + /// or PR being processed comes from GitHub and the IssueUpdate is created from this, we've seen a + /// couple of issues where passing the IssueUpdate back to GitHub causes an ApiValidationException + /// when GitHub is trying to parse the IssueUpdate. This odd considering GitHub is where the information + /// came from to begin with. Clearing things out was already something we do for Actions that just need + /// to change the state and, now, for scheduled events that only change the state. /// /// Octokit.Issue from the event payload /// Whether or not actions are being processed. Default is true. + /// Whether or not this IssueUpdate is only being used to change the issue state (closing/opening). Default is true. /// Octokit.IssueUpdate - public IssueUpdate GetIssueUpdate(Issue issue, bool isProcessingAction = true) + public IssueUpdate GetIssueUpdate(Issue issue, bool isProcessingAction = true, bool isOnlyStateChange = true) { + IssueUpdate tempIssueUpdate = null; + if (isOnlyStateChange) + { + tempIssueUpdate = new IssueUpdate + { + Milestone = issue.Milestone == null + ? new int?() + : issue.Milestone.Number, + State = null, + Body = null, + Title = null + }; + } + else + { + tempIssueUpdate = issue.ToUpdate(); + } + if (isProcessingAction) { if (null == _issueUpdate) { - // For Actions, the IssueUpdate should only be used to set the state. - // Everything else should be null so it doesn't touch those other fields - // except for the Milestone which, if null, would clear it out if one was - // set. That's the only field to pull from the payload. - _issueUpdate = new IssueUpdate - { - Milestone = issue.Milestone == null - ? new int?() - : issue.Milestone.Number, - State = null, - Body = null, - Title = null - }; + _issueUpdate = tempIssueUpdate; } return _issueUpdate; } else { - return issue.ToUpdate(); + return tempIssueUpdate; } } /// - /// Overloaded convenience function that'll return the IssueUpdate. Actions all make changes to - /// the same, shared, IssueUpdate because they're processing on the same event. For scheduled - /// event processing, there will be multiple, unique IssueUpdates and there won't be shared one. + /// Overloaded convenience function that'll return the IssueUpdate for a PR. Whether or + /// not an Action is being processed is not necessary for this overload because results + /// coming back from Search queries are always returned as Issues meaning that this overload + /// is always going to be called in Actions processing. /// /// Octokit.PullRequest from the event payload - /// Whether or not actions are being processed. Default is true. /// Octokit.IssueUpdate - public IssueUpdate GetIssueUpdate(PullRequest pullRequest, bool isProcessingAction = true) + public IssueUpdate GetIssueUpdate(PullRequest pullRequest) { - if (isProcessingAction) + if (null == _issueUpdate) { - if (null == _issueUpdate) + // For Actions, the IssueUpdate should only be used to set the state. + // Everything else should be null so it doesn't touch those other fields + // except for the Milestone which, if null, would clear it out if one was + // set. That's the only field to pull from the payload. + _issueUpdate = new IssueUpdate { - // For Actions, the IssueUpdate should only be used to set the state. - // Everything else should be null so it doesn't touch those other fields - // except for the Milestone which, if null, would clear it out if one was - // set. That's the only field to pull from the payload. - _issueUpdate = new IssueUpdate - { - Milestone = pullRequest.Milestone == null - ? new int?() - : pullRequest.Milestone.Number, - State = null, - Body = null, - Title = null - }; + Milestone = pullRequest.Milestone == null + ? new int?() + : pullRequest.Milestone.Number, + State = null, + Body = null, + Title = null + }; - } - return _issueUpdate; - } - else - { - return CreateIssueUpdateForPR(pullRequest); } + return _issueUpdate; } /// @@ -984,7 +1269,8 @@ namespace Azure.Sdk.Tools.GitHubEventProcessor Console.WriteLine($"QueryIssues, sleeping for {sleepDuration/61} seconds before retrying."); // Task.Delay over Sleep will push the wait into the IO completion state and unblocks the thread // from the threadpool whereas sleep blocks the thread in the threadpool. - await Task.Delay(tryNumber * sleepDuration); + await Delay($"QueryIssues, sleeping for {tryNumber * sleepDuration / 61} seconds before retrying.", + tryNumber * sleepDuration); } } }