Last fixes

README.md

@@ -4,7 +4,7 @@
 
 # tgstation-server
 
-![CI Pipeline](https://github.com/tgstation/tgstation-server/workflows/CI%20Pipeline/badge.svg) [![codecov](https://codecov.io/gh/tgstation/tgstation-server/branch/master/graph/badge.svg)](https://codecov.io/gh/tgstation/tgstation-server)
+[![CI Pipeline](https://github.com/tgstation/tgstation-server/actions/workflows/ci-pipeline.yml/badge.svg)](https://github.com/tgstation/tgstation-server/actions/workflows/ci-pipeline.yml) [![codecov](https://codecov.io/gh/tgstation/tgstation-server/branch/master/graph/badge.svg)](https://codecov.io/gh/tgstation/tgstation-server)
 
 [![GitHub license](https://img.shields.io/github/license/tgstation/tgstation-server.svg)](LICENSE) [![Average time to resolve an issue](http://isitmaintained.com/badge/resolution/tgstation/tgstation-server.svg)](http://isitmaintained.com/project/tgstation/tgstation-server "Average time to resolve an issue") [![NuGet version](https://img.shields.io/nuget/v/Tgstation.Server.Api.svg)](https://www.nuget.org/packages/Tgstation.Server.Api) [![NuGet version](https://img.shields.io/nuget/v/Tgstation.Server.Client.svg)](https://www.nuget.org/packages/Tgstation.Server.Client)
 

src/Tgstation.Server.Host/Components/Engine/ByondInstallerBase.cs

@@ -146,7 +146,7 @@ await IOManager.DeleteDirectory(
 		}
 
 		/// <inheritdoc />
-		public override async ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter? progressReporter, CancellationToken cancellationToken)
+		public override async ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter progressReporter, CancellationToken cancellationToken)
 		{
 			CheckVersionValidity(version);
 

src/Tgstation.Server.Host/Components/Engine/DelegatingEngineInstaller.cs

@@ -37,7 +37,7 @@ public IEngineInstallation CreateInstallation(EngineVersion version, string path
 			=> DelegateCall(version, installer => installer.CreateInstallation(version, path, installationTask));
 
 		/// <inheritdoc />
-		public ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter? jobProgressReporter, CancellationToken cancellationToken)
+		public ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter jobProgressReporter, CancellationToken cancellationToken)
 			=> DelegateCall(version, installer => installer.DownloadVersion(version, jobProgressReporter, cancellationToken));
 
 		/// <inheritdoc />

src/Tgstation.Server.Host/Components/Engine/EngineInstallerBase.cs

@@ -52,7 +52,7 @@ protected EngineInstallerBase(IIOManager ioManager, ILogger<EngineInstallerBase>
 		public abstract ValueTask UpgradeInstallation(EngineVersion version, string path, CancellationToken cancellationToken);
 
 		/// <inheritdoc />
-		public abstract ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter? jobProgressReporter, CancellationToken cancellationToken);
+		public abstract ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter jobProgressReporter, CancellationToken cancellationToken);
 
 		/// <inheritdoc />
 		public abstract ValueTask TrustDmbPath(EngineVersion version, string fullDmbPath, CancellationToken cancellationToken);

src/Tgstation.Server.Host/Components/Engine/EngineManager.cs

@@ -118,7 +118,7 @@ public EngineManager(IIOManager ioManager, IEngineInstaller engineInstaller, IEv
 
 		/// <inheritdoc />
 		public async ValueTask ChangeVersion(
-			JobProgressReporter? progressReporter,
+			JobProgressReporter progressReporter,
 			EngineVersion version,
 			Stream? customVersionStream,
 			bool allowInstallation,
@@ -166,8 +166,11 @@ public async ValueTask<IEngineExecutableLock> UseExecutables(EngineVersion? requ
 				"Acquiring lock on BYOND version {version}...",
 				requiredVersion?.ToString() ?? $"{ActiveVersion} (active)");
 			var versionToUse = requiredVersion ?? ActiveVersion ?? throw new JobException(ErrorCode.EngineNoVersionsInstalled);
+
+			using var progressReporter = new JobProgressReporter();
+
 			var installLock = await AssertAndLockVersion(
-				null,
+				progressReporter,
 				versionToUse,
 				null,
 				requiredVersion != null,
@@ -388,15 +391,15 @@ await ValueTaskExtensions.WhenAll(
 		/// <summary>
 		/// Ensures a BYOND <paramref name="version"/> is installed if it isn't already.
 		/// </summary>
-		/// <param name="progressReporter">The optional <see cref="JobProgressReporter"/> for the operation.</param>
+		/// <param name="progressReporter">The <see cref="JobProgressReporter"/> for the operation.</param>
 		/// <param name="version">The <see cref="EngineVersion"/> to install.</param>
 		/// <param name="customVersionStream">Optional custom zip file <see cref="Stream"/> to use. Will cause a <see cref="Version.Build"/> number to be added.</param>
 		/// <param name="neededForLock">If this BYOND version is required as part of a locking operation.</param>
 		/// <param name="allowInstallation">If an installation should be performed if the <paramref name="version"/> is not installed. If <see langword="false"/> and an installation is required an <see cref="InvalidOperationException"/> will be thrown.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
 		/// <returns>A <see cref="ValueTask{TResult}"/> resulting in the <see cref="EngineExecutableLock"/>.</returns>
 		async ValueTask<EngineExecutableLock> AssertAndLockVersion(
-			JobProgressReporter? progressReporter,
+			JobProgressReporter progressReporter,
 			EngineVersion version,
 			Stream? customVersionStream,
 			bool neededForLock,
@@ -443,8 +446,7 @@ async ValueTask<EngineExecutableLock> AssertAndLockVersion(
 			{
 				if (installedOrInstalling)
 				{
-					if (progressReporter != null)
-						progressReporter.StageName = "Waiting for existing installation job...";
+					progressReporter.StageName = "Waiting for existing installation job...";
 
 					if (neededForLock && !installation.InstallationTask.IsCompleted)
 						logger.LogWarning("The required engine version ({version}) is not readily available! We will have to wait for it to install.", version);
@@ -468,8 +470,7 @@ async ValueTask<EngineExecutableLock> AssertAndLockVersion(
 					else
 						logger.LogInformation("Requested engine version {version} not currently installed. Doing so now...", version);
 
-					if (progressReporter != null)
-						progressReporter.StageName = "Running event";
+					progressReporter.StageName = "Running event";
 
 					var versionString = version.ToString();
 					await eventConsumer.HandleEvent(EventType.EngineInstallStart, new List<string> { versionString }, deploymentPipelineProcesses, cancellationToken);
@@ -504,14 +505,14 @@ async ValueTask<EngineExecutableLock> AssertAndLockVersion(
 		/// <summary>
 		/// Installs the files for a given BYOND <paramref name="version"/>.
 		/// </summary>
-		/// <param name="progressReporter">The optional <see cref="JobProgressReporter"/> for the operation.</param>
+		/// <param name="progressReporter">The <see cref="JobProgressReporter"/> for the operation.</param>
 		/// <param name="version">The <see cref="EngineVersion"/> being installed with the <see cref="Version.Build"/> number set if appropriate.</param>
 		/// <param name="customVersionStream">Custom zip file <see cref="Stream"/> to use. Will cause a <see cref="Version.Build"/> number to be added.</param>
 		/// <param name="deploymentPipelineProcesses">If processes should be launched as part of the deployment pipeline.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
 		/// <returns>A <see cref="ValueTask"/> representing the running operation.</returns>
 		async ValueTask InstallVersionFiles(
-			JobProgressReporter? progressReporter,
+			JobProgressReporter progressReporter,
 			EngineVersion version,
 			Stream? customVersionStream,
 			bool deploymentPipelineProcesses,
@@ -528,14 +529,12 @@ async ValueTask DirectoryCleanup()
 			try
 			{
 				IEngineInstallationData engineInstallationData;
+				var remainingProgress = 1.0;
 				if (customVersionStream == null)
 				{
-					if (progressReporter != null)
-						progressReporter.StageName = "Downloading version";
-
-					engineInstallationData = await engineInstaller.DownloadVersion(version, progressReporter, cancellationToken);
-
-					progressReporter?.ReportProgress(null);
+					using var subReporter = progressReporter.CreateSection("Downloading Version", 0.5);
+					remainingProgress -= 0.5;
+					engineInstallationData = await engineInstaller.DownloadVersion(version, subReporter, cancellationToken);
 				}
 				else
 #pragma warning disable CA2000 // Dispose objects before losing scope, false positive
@@ -544,33 +543,45 @@ async ValueTask DirectoryCleanup()
 						customVersionStream);
 #pragma warning restore CA2000 // Dispose objects before losing scope
 
-				await using (engineInstallationData)
+				JobProgressReporter remainingReporter;
+				try
+				{
+					remainingReporter = progressReporter.CreateSection(null, remainingProgress);
+				}
+				catch
 				{
-					if (progressReporter != null)
-						progressReporter.StageName = "Cleaning target directory";
+					await engineInstallationData.DisposeAsync();
+					throw;
+				}
 
-					await directoryCleanupTask;
+				using (remainingReporter)
+				{
+					await using (engineInstallationData)
+					{
+						remainingReporter.StageName = "Cleaning target directory";
 
-					if (progressReporter != null)
-						progressReporter.StageName = "Extracting data";
+						await directoryCleanupTask;
+						remainingReporter.ReportProgress(0.1);
+						remainingReporter.StageName = "Extracting data";
 
-					logger.LogTrace("Extracting engine to {extractPath}...", installFullPath);
-					await engineInstallationData.ExtractToPath(installFullPath, cancellationToken);
-				}
+						logger.LogTrace("Extracting engine to {extractPath}...", installFullPath);
+						await engineInstallationData.ExtractToPath(installFullPath, cancellationToken);
+						remainingReporter.ReportProgress(0.3);
+					}
 
-				if (progressReporter != null)
-					progressReporter.StageName = "Running installation actions";
+					remainingReporter.StageName = "Running installation actions";
 
-				await engineInstaller.Install(version, installFullPath, deploymentPipelineProcesses, cancellationToken);
+					await engineInstaller.Install(version, installFullPath, deploymentPipelineProcesses, cancellationToken);
 
-				if (progressReporter != null)
-					progressReporter.StageName = "Writing version file";
+					remainingReporter.ReportProgress(0.9);
+					remainingReporter.StageName = "Writing version file";
 
-				// make sure to do this last because this is what tells us we have a valid version in the future
-				await ioManager.WriteAllBytes(
-					ioManager.ConcatPath(installFullPath, VersionFileName),
-					Encoding.UTF8.GetBytes(version.ToString()),
-					cancellationToken);
+					// make sure to do this last because this is what tells us we have a valid version in the future
+					await ioManager.WriteAllBytes(
+						ioManager.ConcatPath(installFullPath, VersionFileName),
+						Encoding.UTF8.GetBytes(version.ToString()),
+						cancellationToken);
+				}
 			}
 			catch (HttpRequestException ex)
 			{

src/Tgstation.Server.Host/Components/Engine/IEngineInstaller.cs

@@ -24,10 +24,10 @@ interface IEngineInstaller
 		/// Download a given engine <paramref name="version"/>.
 		/// </summary>
 		/// <param name="version">The <see cref="EngineVersion"/> of the engine to download.</param>
-		/// <param name="jobProgressReporter">The optional <see cref="JobProgressReporter"/> for the operation.</param>
+		/// <param name="jobProgressReporter">The <see cref="JobProgressReporter"/> for the operation.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
 		/// <returns>A <see cref="ValueTask{TResult}"/> resulting in the <see cref="IEngineInstallationData"/> for the download.</returns>
-		ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter? jobProgressReporter, CancellationToken cancellationToken);
+		ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter jobProgressReporter, CancellationToken cancellationToken);
 
 		/// <summary>
 		/// Does actions necessary to get an extracted installation working.

src/Tgstation.Server.Host/Components/Engine/IEngineManager.cs

@@ -28,14 +28,14 @@ public interface IEngineManager : IComponentService, IDisposable
 		/// <summary>
 		/// Change the active <see cref="EngineVersion"/>.
 		/// </summary>
-		/// <param name="progressReporter">The optional <see cref="JobProgressReporter"/> for the operation.</param>
+		/// <param name="progressReporter">The <see cref="JobProgressReporter"/> for the operation.</param>
 		/// <param name="version">The new <see cref="EngineVersion"/>.</param>
 		/// <param name="customVersionStream">Optional <see cref="Stream"/> of a custom BYOND version zip file.</param>
 		/// <param name="allowInstallation">If an installation should be performed if the <paramref name="version"/> is not installed. If <see langword="false"/> and an installation is required an <see cref="InvalidOperationException"/> will be thrown.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
 		/// <returns>A <see cref="ValueTask"/> representing the running operation.</returns>
 		ValueTask ChangeVersion(
-			JobProgressReporter? progressReporter,
+			JobProgressReporter progressReporter,
 			EngineVersion version,
 			Stream? customVersionStream,
 			bool allowInstallation,

src/Tgstation.Server.Host/Components/Engine/OpenDreamInstaller.cs

@@ -133,23 +133,32 @@ public override IEngineInstallation CreateInstallation(EngineVersion version, st
 		}
 
 		/// <inheritdoc />
-		public override async ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter? jobProgressReporter, CancellationToken cancellationToken)
+		public override async ValueTask<IEngineInstallationData> DownloadVersion(EngineVersion version, JobProgressReporter jobProgressReporter, CancellationToken cancellationToken)
 		{
 			CheckVersionValidity(version);
+			ArgumentNullException.ThrowIfNull(jobProgressReporter);
 
 			// get a lock on a system wide OD repo
 			Logger.LogTrace("Cloning OD repo...");
 
-			var progressSection1 = jobProgressReporter?.CreateSection("Updating OpenDream git repository", 0.5f);
-
-			var repo = await repositoryManager.CloneRepository(
-				GeneralConfiguration.OpenDreamGitUrl,
-				null,
-				null,
-				null,
-				progressSection1,
-				true,
-				cancellationToken);
+			var progressSection1 = jobProgressReporter.CreateSection("Updating OpenDream git repository", 0.5f);
+			IRepository? repo;
+			try
+			{
+				repo = await repositoryManager.CloneRepository(
+					GeneralConfiguration.OpenDreamGitUrl,
+					null,
+					null,
+					null,
+					progressSection1,
+					true,
+					cancellationToken);
+			}
+			catch
+			{
+				progressSection1.Dispose();
+				throw;
+			}
 
 			try
 			{
@@ -168,19 +177,23 @@ public override async ValueTask<IEngineInstallationData> DownloadVersion(EngineV
 						cancellationToken);
 				}
 
-				var progressSection2 = jobProgressReporter?.CreateSection("Checking out OpenDream version", 0.5f);
+				progressSection1.Dispose();
+				progressSection1 = null;
 
-				var committish = version.SourceSHA
-					?? $"{GeneralConfiguration.OpenDreamGitTagPrefix}{version.Version!.Semver()}";
+				using (var progressSection2 = jobProgressReporter.CreateSection("Checking out OpenDream version", 0.5f))
+				{
+					var committish = version.SourceSHA
+						?? $"{GeneralConfiguration.OpenDreamGitTagPrefix}{version.Version!.Semver()}";
 
-				await repo.CheckoutObject(
-					committish,
-					null,
-					null,
-					true,
-					false,
-					progressSection2,
-					cancellationToken);
+					await repo.CheckoutObject(
+						committish,
+						null,
+						null,
+						true,
+						false,
+						progressSection2,
+						cancellationToken);
+				}
 
 				if (!await repo.CommittishIsParent("tgs-min-compat", cancellationToken))
 					throw new JobException(ErrorCode.OpenDreamTooOld);
@@ -192,6 +205,10 @@ await repo.CheckoutObject(
 				repo?.Dispose();
 				throw;
 			}
+			finally
+			{
+				progressSection1?.Dispose();
+			}
 		}
 
 		/// <inheritdoc />

src/Tgstation.Server.Host/Components/Repository/IRepository.cs

@@ -48,7 +48,7 @@ public interface IRepository : IGitRemoteAdditionalInformation, IDisposable
 		/// <param name="password">The optional password used for fetching from submodule repositories.</param>
 		/// <param name="updateSubmodules">If a submodule update should be attempted after the merge.</param>
 		/// <param name="moveCurrentReference">If a hard reset to the target committish should be performed instead of a checkout.</param>
-		/// <param name="progressReporter">The optional <see cref="JobProgressReporter"/> to report progress of the operation.</param>
+		/// <param name="progressReporter">The <see cref="JobProgressReporter"/> to report progress of the operation.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
 		/// <returns>A <see cref="ValueTask"/> representing the running operation.</returns>
 		ValueTask CheckoutObject(
@@ -57,7 +57,7 @@ ValueTask CheckoutObject(
 			string? password,
 			bool updateSubmodules,
 			bool moveCurrentReference,
-			JobProgressReporter? progressReporter,
+			JobProgressReporter progressReporter,
 			CancellationToken cancellationToken);
 
 		/// <summary>
@@ -85,14 +85,14 @@ ValueTask<TestMergeResult> AddTestMerge(
 		/// <summary>
 		/// Fetch commits from the origin repository.
 		/// </summary>
-		/// <param name="progressReporter">The optional <see cref="JobProgressReporter"/> to report progress of the operation.</param>
+		/// <param name="progressReporter">The <see cref="JobProgressReporter"/> to report progress of the operation.</param>
 		/// <param name="username">The optional username to fetch from the origin repository.</param>
 		/// <param name="password">The optional password to fetch from the origin repository.</param>
 		/// <param name="deploymentPipeline">If any events created should be marked as part of the deployment pipeline.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
 		/// <returns>A <see cref="ValueTask"/> representing the running operation.</returns>
 		ValueTask FetchOrigin(
-			JobProgressReporter? progressReporter,
+			JobProgressReporter progressReporter,
 			string? username,
 			string? password,
 			bool deploymentPipeline,

src/Tgstation.Server.Host/Components/Repository/IRepositoryManager.cs

@@ -35,7 +35,7 @@ public interface IRepositoryManager : IDisposable
 		/// <param name="initialBranch">The optional branch to clone.</param>
 		/// <param name="username">The optional username to clone from <paramref name="url"/>.</param>
 		/// <param name="password">The optional password to clone from <paramref name="url"/>.</param>
-		/// <param name="progressReporter">The optional <see cref="JobProgressReporter"/> for progress of the clone.</param>
+		/// <param name="progressReporter">The <see cref="JobProgressReporter"/> for progress of the clone.</param>
 		/// <param name="recurseSubmodules">If submodules should be recusively cloned and initialized.</param>
 		/// <param name="cancellationToken">The <see cref="CancellationToken"/> for the operation.</param>
 		/// <returns>A <see cref="ValueTask{TResult}"/> resulting i the newly cloned <see cref="IRepository"/>, <see langword="null"/> if one already exists.</returns>
@@ -44,7 +44,7 @@ public interface IRepositoryManager : IDisposable
 			string? initialBranch,
 			string? username,
 			string? password,
-			JobProgressReporter? progressReporter,
+			JobProgressReporter progressReporter,
 			bool recurseSubmodules,
 			CancellationToken cancellationToken);