Merge pull request #1668 from tgstation/ParaFix [TGSDeploy]

v5.16.2: Fix issues with test merges being removed by auto-update

build/Version.props

@@ -3,7 +3,7 @@
   <!-- Integration tests will ensure they match across the board -->
   <Import Project="ControlPanelVersion.props" />
   <PropertyGroup>
-    <TgsCoreVersion>5.16.1</TgsCoreVersion>
+    <TgsCoreVersion>5.16.2</TgsCoreVersion>
     <TgsConfigVersion>4.7.1</TgsConfigVersion>
     <TgsApiVersion>9.12.0</TgsApiVersion>
     <TgsCommonLibraryVersion>6.0.1</TgsCommonLibraryVersion>

src/Tgstation.Server.Host/Components/Instance.cs

@@ -382,28 +382,28 @@ async Task UpdateRevInfo(string currentHead, bool onOrigin, IEnumerable<TestMerg
 					// build current commit data if it's missing
 					await UpdateRevInfo(repo.Head, false, null);
 
+					var preserveTestMerges = repositorySettings.AutoUpdatesKeepTestMerges.Value;
+					var remoteDeploymentManager = remoteDeploymentManagerFactory.CreateRemoteDeploymentManager(
+						metadata,
+						repo.RemoteGitProvider.Value);
+
+					var updatedTestMerges = await remoteDeploymentManager.RemoveMergedTestMerges(
+						repo,
+						repositorySettings,
+						currentRevInfo,
+						cancellationToken);
+
 					var result = await repo.MergeOrigin(
 						NextProgressReporter("Merge Origin"),
 						repositorySettings.CommitterName,
 						repositorySettings.CommitterEmail,
 						true,
 						cancellationToken);
 
-					var preserveTestMerges = repositorySettings.AutoUpdatesKeepTestMerges.Value;
-					var remoteDeploymentManager = remoteDeploymentManagerFactory.CreateRemoteDeploymentManager(
-						metadata,
-						repo.RemoteGitProvider.Value);
-
 					// take appropriate auto update actions
 					var shouldSyncTracked = false;
 					if (result.HasValue)
 					{
-						var updatedTestMerges = await remoteDeploymentManager.RemoveMergedTestMerges(
-							repo,
-							repositorySettings,
-							currentRevInfo,
-							cancellationToken);
-
 						if (updatedTestMerges.Count == 0)
 						{
 							logger.LogTrace("All test merges have been merged on remote");
@@ -517,7 +517,12 @@ await jobManager.RegisterOperation(
 							RepositoryAutoUpdateJob,
 							cancellationToken);
 
-						await jobManager.WaitForJobCompletion(repositoryUpdateJob, null, cancellationToken, cancellationToken);
+						var repoUpdateJobResult = await jobManager.WaitForJobCompletion(repositoryUpdateJob, null, cancellationToken, cancellationToken);
+						if (repoUpdateJobResult == false)
+						{
+							logger.LogWarning("Aborting auto-update due to repository update error!");
+							continue;
+						}
 
 						Job compileProcessJob;
 						using (var repo = await RepositoryManager.LoadRepository(cancellationToken))

src/Tgstation.Server.Host/Jobs/IJobManager.cs

@@ -33,8 +33,8 @@ public interface IJobManager
 		/// <param name="canceller">The <see cref="User"/> to cancel the <paramref name="job"/>. If <see langword="null"/> the TGS user will be used.</param>
 		/// <param name="jobCancellationToken">A <see cref="CancellationToken"/> that will cancel the <paramref name="job"/>.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
-		/// <returns>A <see cref="Task"/> representing the <see cref="Job"/>.</returns>
-		Task WaitForJobCompletion(Job job, User canceller, CancellationToken jobCancellationToken, CancellationToken cancellationToken);
+		/// <returns>A <see cref="Task{TResult}"/> representing the <see cref="Job"/>. Results in <see langword="true"/> if the <see cref="Job"/> completed without errors, <see langword="false"/> if errors occurred, or <see langword="null"/> if the job isn't registered.</returns>
+		Task<bool?> WaitForJobCompletion(Job job, User canceller, CancellationToken jobCancellationToken, CancellationToken cancellationToken);
 
 		/// <summary>
 		/// Cancels a give <paramref name="job"/>.

src/Tgstation.Server.Host/Jobs/JobHandler.cs

@@ -30,20 +30,20 @@ sealed class JobHandler : IDisposable
 		readonly CancellationTokenSource cancellationTokenSource;
 
 		/// <summary>
-		/// A <see cref="Func{T, TResult}"/> taking a <see cref="CancellationToken"/> and returning a <see cref="Task"/> that the <see cref="JobHandler"/> will wrap.
+		/// A <see cref="Func{T, TResult}"/> taking a <see cref="CancellationToken"/> and returning a <see cref="Task{TResult}"/> that the <see cref="JobHandler"/> will wrap.
 		/// </summary>
-		readonly Func<CancellationToken, Task> jobActivator;
+		readonly Func<CancellationToken, Task<bool>> jobActivator;
 
 		/// <summary>
 		/// The <see cref="Task"/> being run.
 		/// </summary>
-		Task task;
+		Task<bool> task;
 
 		/// <summary>
 		/// Initializes a new instance of the <see cref="JobHandler"/> class.
 		/// </summary>
 		/// <param name="jobActivator">The value of <see cref="jobActivator"/>.</param>
-		public JobHandler(Func<CancellationToken, Task> jobActivator)
+		public JobHandler(Func<CancellationToken, Task<bool>> jobActivator)
 		{
 			this.jobActivator = jobActivator ?? throw new ArgumentNullException(nameof(jobActivator));
 			cancellationTokenSource = new CancellationTokenSource();
@@ -56,8 +56,8 @@ public JobHandler(Func<CancellationToken, Task> jobActivator)
 		/// Wait for <see cref="task"/> to complete.
 		/// </summary>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
-		/// <returns>A <see cref="Task"/> representing the running operation.</returns>
-		public Task Wait(CancellationToken cancellationToken)
+		/// <returns>A <see cref="Task{TResult}"/> representing the job. Results in <see langword="true"/> if the job completed without errors, <see langword="false"/> otherwise.</returns>
+		public Task<bool> Wait(CancellationToken cancellationToken)
 		{
 			if (task == null)
 				throw new InvalidOperationException("Job not started!");

src/Tgstation.Server.Host/Jobs/JobService.cs

@@ -247,7 +247,7 @@ public void SetJobProgress(JobResponse apiResponse)
 		}
 
 		/// <inheritdoc />
-		public async Task WaitForJobCompletion(Job job, User canceller, CancellationToken jobCancellationToken, CancellationToken cancellationToken)
+		public async Task<bool?> WaitForJobCompletion(Job job, User canceller, CancellationToken jobCancellationToken, CancellationToken cancellationToken)
 		{
 			ArgumentNullException.ThrowIfNull(job);
 
@@ -259,7 +259,7 @@ public async Task WaitForJobCompletion(Job job, User canceller, CancellationToke
 			lock (synchronizationLock)
 			{
 				if (!jobs.TryGetValue(job.Id.Value, out handler))
-					return;
+					return null;
 
 				noMoreJobsShouldStart = this.noMoreJobsShouldStart;
 			}
@@ -268,11 +268,14 @@ public async Task WaitForJobCompletion(Job job, User canceller, CancellationToke
 				await Extensions.TaskExtensions.InfiniteTask.WaitAsync(cancellationToken);
 
 			Task cancelTask = null;
+			bool result;
 			using (jobCancellationToken.Register(() => cancelTask = CancelJob(job, canceller, true, cancellationToken)))
-				await handler.Wait(cancellationToken);
+				result = await handler.Wait(cancellationToken);
 
 			if (cancelTask != null)
 				await cancelTask;
+
+			return result;
 		}
 
 		/// <inheritdoc />
@@ -291,12 +294,14 @@ public void Activate(IInstanceCoreProvider instanceCoreProvider)
 		/// <param name="operation">The <see cref="JobEntrypoint"/> for the <paramref name="job"/>.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
 		/// <returns>A <see cref="Task"/> representing the running operation.</returns>
-		async Task RunJob(Job job, JobEntrypoint operation, CancellationToken cancellationToken)
+		async Task<bool> RunJob(Job job, JobEntrypoint operation, CancellationToken cancellationToken)
 		{
 			using (LogContext.PushProperty(SerilogContextHelper.JobIdContextProperty, job.Id))
 				try
 				{
 					void LogException(Exception ex) => logger.LogDebug(ex, "Job {jobId} exited with error!", job.Id);
+
+					var result = false;
 					try
 					{
 						var oldJob = job;
@@ -334,6 +339,7 @@ await operation(
 							cancellationToken);
 
 						logger.LogDebug("Job {jobId} completed!", job.Id);
+						result = true;
 					}
 					catch (OperationCanceledException ex)
 					{
@@ -368,6 +374,8 @@ await databaseContextFactory.UseContext(async databaseContext =>
 						// DCT: Cancellation token is for job, operation should always run
 						await databaseContext.Save(CancellationToken.None);
 					});
+
+					return result;
 				}
 				finally
 				{

src/Tgstation.Server.Host/Utils/GitHub/GitHubClientFactory.cs

@@ -80,6 +80,7 @@ GitHubClient GetOrCreateClient(string accessToken)
 		{
 			GitHubClient client;
 			bool cacheHit;
+			DateTimeOffset? lastUsed;
 			lock (clientCache)
 			{
 				string cacheKey;
@@ -105,11 +106,13 @@ GitHubClient GetOrCreateClient(string accessToken)
 						client.Credentials = new Credentials(accessToken);
 
 					clientCache.Add(cacheKey, (client, now));
+					lastUsed = null;
 				}
 				else
 				{
 					logger.LogTrace("Cache hit for GitHubClient");
 					client = tuple.Item1;
+					lastUsed = tuple.Item2;
 					tuple.Item2 = now;
 				}
 
@@ -144,13 +147,15 @@ GitHubClient GetOrCreateClient(string accessToken)
 						rateLimitInfo.Reset.ToString("o"));
 				else if (rateLimitInfo.Remaining < 25) // good luck hitting these lines on codecov
 					logger.LogWarning(
-						"Requested GitHub client has only {remainingRequests} requests remaining! Limit resets at {resetTime}",
+						"Requested GitHub client has only {remainingRequests} requests remaining after the usage at {lastUse}! Limit resets at {resetTime}",
 						rateLimitInfo.Remaining,
+						lastUsed,
 						rateLimitInfo.Reset.ToString("o"));
 				else
 					logger.LogDebug(
-						"Requested GitHub client has {remainingRequests} requests remaining. Limit resets at {resetTime}",
+						"Requested GitHub client has {remainingRequests} requests remaining after the usage {lastUse}. Limit resets at {resetTime}",
 						rateLimitInfo.Remaining,
+						lastUsed,
 						rateLimitInfo.Reset.ToString("o"));
 
 			return client;

tests/Tgstation.Server.Host.Tests/Components/Chat/Providers/TestDiscordProvider.cs

@@ -39,7 +39,7 @@ public static void Initialize(TestContext _)
 				.Returns(Task.CompletedTask);
 			mockSetup
 				.Setup(x => x.WaitForJobCompletion(It.IsNotNull<Job>(), It.IsAny<User>(), It.IsAny<CancellationToken>(), It.IsAny<CancellationToken>()))
-				.Returns(Task.CompletedTask);
+				.Returns(Task.FromResult<bool?>(true));
 			mockJobManager = mockSetup.Object;
 		}
 

tests/Tgstation.Server.Host.Tests/Components/Chat/Providers/TestIrcProvider.cs

@@ -76,7 +76,7 @@ public async Task TestConnectAndDisconnect()
 				.Returns(Task.CompletedTask);
 			mockSetup
 				.Setup(x => x.WaitForJobCompletion(It.IsNotNull<Job>(), It.IsAny<User>(), It.IsAny<CancellationToken>(), It.IsAny<CancellationToken>()))
-				.Returns(Task.CompletedTask);
+				.Returns(Task.FromResult<bool?>(true));
 			var mockJobManager = mockSetup.Object;
 			await using var provider = new IrcProvider(mockJobManager, new AsyncDelayer(), loggerFactory.CreateLogger<IrcProvider>(), Mock.Of<IAssemblyInformationProvider>(), new ChatBot
 			{

tests/Tgstation.Server.Host.Tests/Jobs/TestJobHandler.cs

@@ -11,10 +11,11 @@ public sealed class TestJobHandler
 		Task currentWaitTask;
 		bool cancelled;
 
-		async Task TestJob(CancellationToken cancellationToken)
+		async Task<bool> TestJob(CancellationToken cancellationToken)
 		{
 			await currentWaitTask;
 			cancelled = cancellationToken.IsCancellationRequested;
+			return true;
 		}
 
 		[TestMethod]