From d7a1f8f0d0239aeee0b2d4902283910d82052ca0 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 18 May 2020 15:45:57 -0400 Subject: [PATCH 1/7] Fix GET /DreamDaemon possible not returning completed jobs --- .../Components/Deployment/DmbFactory.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/Tgstation.Server.Host/Components/Deployment/DmbFactory.cs b/src/Tgstation.Server.Host/Components/Deployment/DmbFactory.cs index cea2a3f7c7..9c6b2190de 100644 --- a/src/Tgstation.Server.Host/Components/Deployment/DmbFactory.cs +++ b/src/Tgstation.Server.Host/Components/Deployment/DmbFactory.cs @@ -202,6 +202,15 @@ namespace Tgstation.Server.Host.Components.Deployment .Include(x => x.RevisionInformation).ThenInclude(x => x.ActiveTestMerges).ThenInclude(x => x.TestMerge).ThenInclude(x => x.MergedBy) .FirstAsync(cancellationToken).ConfigureAwait(false)).ConfigureAwait(false); // can't wait to see that query + if (!compileJob.Job.StoppedAt.HasValue) + { + // This happens if we're told to load the compile job that is currently finished up + // It can constitute an API violation if it's returned by the DreamDaemonController so just set it here + // Bit of a hack, but it should work out to be the same value + logger.LogTrace("Setting missing StoppedAt for CompileJob job..."); + compileJob.Job.StoppedAt = DateTimeOffset.Now; + } + logger.LogTrace("Loading compile job {0}...", compileJob.Id); var providerSubmitted = false; var newProvider = new DmbProvider(compileJob, ioManager, () => From a1e6c323ebdf24a4990a33fd2db29a7df12ba3b4 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 18 May 2020 15:46:23 -0400 Subject: [PATCH 2/7] Fix session controller reattach issue --- .../Components/Session/SessionController.cs | 84 ++++++++++--------- 1 file changed, 45 insertions(+), 39 deletions(-) diff --git a/src/Tgstation.Server.Host/Components/Session/SessionController.cs b/src/Tgstation.Server.Host/Components/Session/SessionController.cs index 19b5b7b06c..471cbad030 100644 --- a/src/Tgstation.Server.Host/Components/Session/SessionController.cs +++ b/src/Tgstation.Server.Host/Components/Session/SessionController.cs @@ -105,6 +105,11 @@ namespace Tgstation.Server.Host.Components.Session /// readonly ReattachInformation reattachInformation; + /// + /// A used for the topic send operation made on reattaching. + /// + readonly CancellationTokenSource reattachTopicCts; + /// /// The for the /// @@ -230,23 +235,20 @@ namespace Tgstation.Server.Host.Components.Session rebootTcs = new TaskCompletionSource(); primeTcs = new TaskCompletionSource(); - + reattachTopicCts = new CancellationTokenSource(); synchronizationLock = new object(); - CancellationTokenSource cts = null; _ = process.Lifetime.ContinueWith( x => { - cts?.Cancel(); + if (!disposed) + reattachTopicCts.Cancel(); chatTrackingContext.Active = false; }, TaskScheduler.Current); LaunchResult = GetLaunchResult( assemblyInformationProvider, -#pragma warning disable CA2000 // Dispose objects before losing scope - cts = new CancellationTokenSource(), -#pragma warning restore CA2000 // Dispose objects before losing scope startupTimeout, reattached); @@ -290,6 +292,7 @@ namespace Tgstation.Server.Host.Components.Session bridgeRegistration.Dispose(); Dmb?.Dispose(); // will be null when released chatTrackingContext.Dispose(); + reattachTopicCts.Dispose(); disposed = true; } else @@ -305,49 +308,52 @@ namespace Tgstation.Server.Host.Components.Session } } + /// + /// The for . + /// + /// The . + /// The, optional, startup timeout in seconds. + /// If DreamDaemon was reattached. + /// A resulting in the for the operation. async Task GetLaunchResult( IAssemblyInformationProvider assemblyInformationProvider, - CancellationTokenSource cancellationTokenSource, uint? startupTimeout, bool reattached) { - using (cancellationTokenSource) + var startTime = DateTimeOffset.Now; + Task toAwait = process.Startup; + + if (startupTimeout.HasValue) + toAwait = Task.WhenAny(process.Startup, Task.Delay(startTime.AddSeconds(startupTimeout.Value) - startTime)); + + await toAwait.ConfigureAwait(false); + + var result = new LaunchResult { - var startTime = DateTimeOffset.Now; - Task toAwait = process.Startup; + ExitCode = process.Lifetime.IsCompleted ? (int?)await process.Lifetime.ConfigureAwait(false) : null, + StartupTime = process.Startup.IsCompleted ? (TimeSpan?)(DateTimeOffset.Now - startTime) : null + }; - if (startupTimeout.HasValue) - toAwait = Task.WhenAny(process.Startup, Task.Delay(startTime.AddSeconds(startupTimeout.Value) - startTime)); + logger.LogTrace("Launch result: {0}", result); - await toAwait.ConfigureAwait(false); + if (!result.ExitCode.HasValue && reattached && !disposed) + { + var reattachResponse = await SendCommand( + new TopicParameters( + assemblyInformationProvider.Version, + reattachInformation.RuntimeInformation.ServerPort), + reattachTopicCts.Token) + .ConfigureAwait(false); - var result = new LaunchResult - { - ExitCode = process.Lifetime.IsCompleted ? (int?)await process.Lifetime.ConfigureAwait(false) : null, - StartupTime = process.Startup.IsCompleted ? (TimeSpan?)(DateTimeOffset.Now - startTime) : null - }; - - logger.LogTrace("Launch result: {0}", result); - - if (!result.ExitCode.HasValue && reattached) - { - var reattachResponse = await SendCommand( - new TopicParameters( - assemblyInformationProvider.Version, - reattachInformation.RuntimeInformation.ServerPort), - cancellationTokenSource.Token) - .ConfigureAwait(false); - - if (reattachResponse.InteropResponse?.CustomCommands != null) - chatTrackingContext.CustomCommands = reattachResponse.InteropResponse.CustomCommands; - else if (reattachResponse.InteropResponse != null) - logger.LogWarning( - "DMAPI v{0} isn't returning the TGS custom commands list. Functionality added in v5.2.0.", - Dmb.CompileJob.DMApiVersion.Semver()); - } - - return result; + if (reattachResponse.InteropResponse?.CustomCommands != null) + chatTrackingContext.CustomCommands = reattachResponse.InteropResponse.CustomCommands; + else if (reattachResponse.InteropResponse != null) + logger.LogWarning( + "DMAPI v{0} isn't returning the TGS custom commands list. Functionality added in v5.2.0.", + Dmb.CompileJob.DMApiVersion.Semver()); } + + return result; } /// From 352fee06361fb55bc665d116f3a765ef9e8fe0cb Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 18 May 2020 15:47:30 -0400 Subject: [PATCH 3/7] Minor code cleanup --- src/Tgstation.Server.Host/Database/DatabaseSeeder.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Tgstation.Server.Host/Database/DatabaseSeeder.cs b/src/Tgstation.Server.Host/Database/DatabaseSeeder.cs index 694f5e4da9..ec15fb3e3c 100644 --- a/src/Tgstation.Server.Host/Database/DatabaseSeeder.cs +++ b/src/Tgstation.Server.Host/Database/DatabaseSeeder.cs @@ -69,8 +69,8 @@ namespace Tgstation.Server.Host.Database // Fix the issue with ulong enums // https://github.com/tgstation/tgstation-server/commit/db341d43b3dab74fe3681f5172ca9bfeaafa6b6d#diff-09f06ec4584665cf89bb77b97f5ccfb9R36-R39 // https://github.com/JamesNK/Newtonsoft.Json/issues/2301 - admin.AdministrationRights = admin.AdministrationRights & RightsHelper.AllRights(); - admin.InstanceManagerRights = admin.InstanceManagerRights & RightsHelper.AllRights(); + admin.AdministrationRights &= RightsHelper.AllRights(); + admin.InstanceManagerRights &= RightsHelper.AllRights(); } if (platformIdentifier.IsWindows) From a3706efbfdd56feb7662997583b79f8e4ebb87f2 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 18 May 2020 15:55:32 -0400 Subject: [PATCH 4/7] More code cleanup --- .../Controllers/RepositoryController.cs | 716 +++++++++--------- 1 file changed, 353 insertions(+), 363 deletions(-) diff --git a/src/Tgstation.Server.Host/Controllers/RepositoryController.cs b/src/Tgstation.Server.Host/Controllers/RepositoryController.cs index 43271c6d4e..9dc34a435e 100644 --- a/src/Tgstation.Server.Host/Controllers/RepositoryController.cs +++ b/src/Tgstation.Server.Host/Controllers/RepositoryController.cs @@ -100,7 +100,7 @@ namespace Tgstation.Server.Host.Controllers databaseContext.RevisionInformations.Add(revisionInfo); } - revisionInfo.OriginCommitSha = revisionInfo.OriginCommitSha ?? lastOriginCommitSha; + revisionInfo.OriginCommitSha ??= lastOriginCommitSha; if (revisionInfo.OriginCommitSha == null) { revisionInfo.OriginCommitSha = repoSha; @@ -178,43 +178,39 @@ namespace Tgstation.Server.Host.Controllers if(repoManager.InUse) return Conflict(new ErrorMessage(ErrorCode.RepoBusy)); - using (var repo = await repoManager.LoadRepository(cancellationToken).ConfigureAwait(false)) + using var repo = await repoManager.LoadRepository(cancellationToken).ConfigureAwait(false); + // clone conflict + if (repo != null) + return Conflict(new ErrorMessage(ErrorCode.RepoExists)); + + var job = new Models.Job { - // clone conflict - if (repo != null) - return Conflict(new ErrorMessage(ErrorCode.RepoExists)); - - var job = new Models.Job + Description = String.Format(CultureInfo.InvariantCulture, "Clone branch {1} of repository {0}", origin, cloneBranch ?? "master"), + StartedBy = AuthenticationContext.User, + CancelRightsType = RightsType.Repository, + CancelRight = (ulong)RepositoryRights.CancelClone, + Instance = Instance + }; + var api = currentModel.ToApi(); + await jobManager.RegisterOperation(job, async (paramJob, databaseContext, progressReporter, ct) => + { + using var repos = await repoManager.CloneRepository(new Uri(origin), cloneBranch, currentModel.AccessUser, currentModel.AccessToken, progressReporter, ct).ConfigureAwait(false); + if (repos == null) + throw new JobException(ErrorCode.RepoExists); + var instance = new Models.Instance { - Description = String.Format(CultureInfo.InvariantCulture, "Clone branch {1} of repository {0}", origin, cloneBranch ?? "master"), - StartedBy = AuthenticationContext.User, - CancelRightsType = RightsType.Repository, - CancelRight = (ulong)RepositoryRights.CancelClone, - Instance = Instance + Id = Instance.Id }; - var api = currentModel.ToApi(); - await jobManager.RegisterOperation(job, async (paramJob, databaseContext, progressReporter, ct) => - { - using (var repos = await repoManager.CloneRepository(new Uri(origin), cloneBranch, currentModel.AccessUser, currentModel.AccessToken, progressReporter, ct).ConfigureAwait(false)) - { - if (repos == null) - throw new JobException(ErrorCode.RepoExists); - var instance = new Models.Instance - { - Id = Instance.Id - }; - databaseContext.Instances.Attach(instance); - if (await PopulateApi(api, repos, databaseContext, instance, ct).ConfigureAwait(false)) - await databaseContext.Save(ct).ConfigureAwait(false); - } - }, cancellationToken).ConfigureAwait(false); + databaseContext.Instances.Attach(instance); + if (await PopulateApi(api, repos, databaseContext, instance, ct).ConfigureAwait(false)) + await databaseContext.Save(ct).ConfigureAwait(false); + }, cancellationToken).ConfigureAwait(false); - api.Origin = model.Origin; - api.Reference = model.Reference; - api.ActiveJob = job.ToApi(); + api.Origin = model.Origin; + api.Reference = model.Reference; + api.ActiveJob = job.ToApi(); - return StatusCode((int)HttpStatusCode.Created, api); - } + return StatusCode((int)HttpStatusCode.Created, api); } /// @@ -283,17 +279,15 @@ namespace Tgstation.Server.Host.Controllers if (repoManager.InUse) return Conflict(new ErrorMessage(ErrorCode.RepoBusy)); - using (var repo = await repoManager.LoadRepository(cancellationToken).ConfigureAwait(false)) + using var repo = await repoManager.LoadRepository(cancellationToken).ConfigureAwait(false); + if (repo != null && await PopulateApi(api, repo, DatabaseContext, Instance, cancellationToken).ConfigureAwait(false)) { - if (repo != null && await PopulateApi(api, repo, DatabaseContext, Instance, cancellationToken).ConfigureAwait(false)) - { - // user may have fucked with the repo manually, do what we can - await DatabaseContext.Save(cancellationToken).ConfigureAwait(false); - return StatusCode((int)HttpStatusCode.Created, api); - } - - return Json(api); + // user may have fucked with the repo manually, do what we can + await DatabaseContext.Save(cancellationToken).ConfigureAwait(false); + return StatusCode((int)HttpStatusCode.Created, api); } + + return Json(api); } /// @@ -396,15 +390,13 @@ namespace Tgstation.Server.Host.Controllers if (repoManager.InUse) return Conflict(new ErrorMessage(ErrorCode.RepoBusy)); - using (var repo = await repoManager.LoadRepository(cancellationToken).ConfigureAwait(false)) - { - if (repo == null) - return Conflict(new ErrorMessage(ErrorCode.RepoMissing)); - await PopulateApi(api, repo, DatabaseContext, Instance, cancellationToken).ConfigureAwait(false); + using var repo = await repoManager.LoadRepository(cancellationToken).ConfigureAwait(false); + if (repo == null) + return Conflict(new ErrorMessage(ErrorCode.RepoMissing)); + await PopulateApi(api, repo, DatabaseContext, Instance, cancellationToken).ConfigureAwait(false); - if (model.Origin != null && model.Origin != repo.Origin) - return BadRequest(new ErrorMessage(ErrorCode.RepoCantChangeOrigin)); - } + if (model.Origin != null && model.Origin != repo.Origin) + return BadRequest(new ErrorMessage(ErrorCode.RepoCantChangeOrigin)); } // this is just db stuf so stow it away @@ -444,358 +436,356 @@ namespace Tgstation.Server.Host.Controllers await jobManager.RegisterOperation(job, async (paramJob, databaseContext, progressReporter, ct) => { - using (var repo = await repoManager.LoadRepository(ct).ConfigureAwait(false)) + using var repo = await repoManager.LoadRepository(ct).ConfigureAwait(false); + if (repo == null) + throw new JobException(ErrorCode.RepoMissing); + + var modelHasShaOrReference = model.CheckoutSha != null || model.Reference != null; + + var startReference = repo.Reference; + var startSha = repo.Head; + string postUpdateSha = null; + + if (newTestMerges && !repo.IsGitHubRepository) + throw new JobException(ErrorCode.RepoUnsupportedTestMergeRemote); + + var committerName = currentModel.ShowTestMergeCommitters.Value + ? AuthenticationContext.User.Name + : currentModel.CommitterName; + + var hardResettingToOriginReference = model.UpdateFromOrigin == true && model.Reference != null; + + var numSteps = (model.NewTestMerges?.Count ?? 0) + (model.UpdateFromOrigin == true ? 1 : 0) + (!modelHasShaOrReference ? 2 : (hardResettingToOriginReference ? 3 : 1)); + var doneSteps = 0; + + Action NextProgressReporter() { - if (repo == null) - throw new JobException(ErrorCode.RepoMissing); + var tmpDoneSteps = doneSteps; + ++doneSteps; + return progress => progressReporter((progress + (100 * tmpDoneSteps)) / numSteps); + } - var modelHasShaOrReference = model.CheckoutSha != null || model.Reference != null; + progressReporter(0); - var startReference = repo.Reference; - var startSha = repo.Head; - string postUpdateSha = null; + // get a base line for where we are + Models.RevisionInformation lastRevisionInfo = null; - if (newTestMerges && !repo.IsGitHubRepository) - throw new JobException(ErrorCode.RepoUnsupportedTestMergeRemote); + var attachedInstance = new Models.Instance + { + Id = Instance.Id + }; + databaseContext.Instances.Attach(attachedInstance); - var committerName = currentModel.ShowTestMergeCommitters.Value - ? AuthenticationContext.User.Name - : currentModel.CommitterName; + await LoadRevisionInformation(repo, databaseContext, attachedInstance, null, x => lastRevisionInfo = x, ct).ConfigureAwait(false); - var hardResettingToOriginReference = model.UpdateFromOrigin == true && model.Reference != null; + // apply new rev info, tracking applied test merges + async Task UpdateRevInfo() + { + var last = lastRevisionInfo; + await LoadRevisionInformation(repo, databaseContext, attachedInstance, last.OriginCommitSha, x => lastRevisionInfo = x, ct).ConfigureAwait(false); + lastRevisionInfo.ActiveTestMerges.AddRange(last.ActiveTestMerges); + } - var numSteps = (model.NewTestMerges?.Count ?? 0) + (model.UpdateFromOrigin == true ? 1 : 0) + (!modelHasShaOrReference ? 2 : (hardResettingToOriginReference ? 3 : 1)); - var doneSteps = 0; - - Action NextProgressReporter() + try + { + // fetch/pull + if (model.UpdateFromOrigin == true) { - var tmpDoneSteps = doneSteps; - ++doneSteps; - return progress => progressReporter((progress + (100 * tmpDoneSteps)) / numSteps); - } - - progressReporter(0); - - // get a base line for where we are - Models.RevisionInformation lastRevisionInfo = null; - - var attachedInstance = new Models.Instance - { - Id = Instance.Id - }; - databaseContext.Instances.Attach(attachedInstance); - - await LoadRevisionInformation(repo, databaseContext, attachedInstance, null, x => lastRevisionInfo = x, ct).ConfigureAwait(false); - - // apply new rev info, tracking applied test merges - async Task UpdateRevInfo() - { - var last = lastRevisionInfo; - await LoadRevisionInformation(repo, databaseContext, attachedInstance, last.OriginCommitSha, x => lastRevisionInfo = x, ct).ConfigureAwait(false); - lastRevisionInfo.ActiveTestMerges.AddRange(last.ActiveTestMerges); - } - - try - { - // fetch/pull - if (model.UpdateFromOrigin == true) + if (!repo.Tracking) + throw new JobException(ErrorCode.RepoReferenceRequired); + await repo.FetchOrigin(currentModel.AccessUser, currentModel.AccessToken, NextProgressReporter(), ct).ConfigureAwait(false); + doneSteps = 1; + if (!modelHasShaOrReference) { - if (!repo.Tracking) - throw new JobException(ErrorCode.RepoReferenceRequired); - await repo.FetchOrigin(currentModel.AccessUser, currentModel.AccessToken, NextProgressReporter(), ct).ConfigureAwait(false); - doneSteps = 1; - if (!modelHasShaOrReference) + var fastForward = await repo.MergeOrigin(committerName, currentModel.CommitterEmail, NextProgressReporter(), ct).ConfigureAwait(false); + if (!fastForward.HasValue) + throw new JobException(ErrorCode.RepoMergeConflict); + await UpdateRevInfo().ConfigureAwait(false); + if (fastForward.Value) { - var fastForward = await repo.MergeOrigin(committerName, currentModel.CommitterEmail, NextProgressReporter(), ct).ConfigureAwait(false); - if (!fastForward.HasValue) - throw new JobException(ErrorCode.RepoMergeConflict); - await UpdateRevInfo().ConfigureAwait(false); - if (fastForward.Value) - { - lastRevisionInfo.OriginCommitSha = repo.Head; - await repo.Sychronize(currentModel.AccessUser, currentModel.AccessToken, currentModel.CommitterName, currentModel.CommitterEmail, NextProgressReporter(), true, ct).ConfigureAwait(false); - postUpdateSha = repo.Head; - } - else - NextProgressReporter()(100); - } - } - - // checkout/hard reset - if (modelHasShaOrReference) - { - var validCheckoutSha = - model.CheckoutSha != null - && !repo.Head.StartsWith(model.CheckoutSha, StringComparison.OrdinalIgnoreCase); - var validCheckoutReference = - model.Reference != null - && !repo.Reference.Equals(model.Reference, StringComparison.OrdinalIgnoreCase); - if (validCheckoutSha || validCheckoutReference) - { - var committish = model.CheckoutSha ?? model.Reference; - var isSha = await repo.IsSha(committish, cancellationToken).ConfigureAwait(false); - - if ((isSha && model.Reference != null) || (!isSha && model.CheckoutSha != null)) - throw new JobException(ErrorCode.RepoSwappedShaOrReference); - - await repo.CheckoutObject(committish, NextProgressReporter(), ct).ConfigureAwait(false); - await LoadRevisionInformation(repo, databaseContext, attachedInstance, null, x => lastRevisionInfo = x, ct).ConfigureAwait(false); // we've either seen origin before or what we're checking out is on origin + lastRevisionInfo.OriginCommitSha = repo.Head; + await repo.Sychronize(currentModel.AccessUser, currentModel.AccessToken, currentModel.CommitterName, currentModel.CommitterEmail, NextProgressReporter(), true, ct).ConfigureAwait(false); + postUpdateSha = repo.Head; } else NextProgressReporter()(100); - - if (hardResettingToOriginReference) - { - if (!repo.Tracking) - throw new JobException(ErrorCode.RepoReferenceNotTracking); - await repo.ResetToOrigin(NextProgressReporter(), ct).ConfigureAwait(false); - await repo.Sychronize(currentModel.AccessUser, currentModel.AccessToken, currentModel.CommitterName, currentModel.CommitterEmail, NextProgressReporter(), true, ct).ConfigureAwait(false); - await LoadRevisionInformation(repo, databaseContext, attachedInstance, null, x => lastRevisionInfo = x, ct).ConfigureAwait(false); - - // repo head is on origin so force this - // will update the db if necessary - lastRevisionInfo.OriginCommitSha = repo.Head; - } } + } - // test merging - Dictionary prMap = null; - if (newTestMerges) + // checkout/hard reset + if (modelHasShaOrReference) + { + var validCheckoutSha = + model.CheckoutSha != null + && !repo.Head.StartsWith(model.CheckoutSha, StringComparison.OrdinalIgnoreCase); + var validCheckoutReference = + model.Reference != null + && !repo.Reference.Equals(model.Reference, StringComparison.OrdinalIgnoreCase); + if (validCheckoutSha || validCheckoutReference) { - // bit of sanitization - foreach (var I in model.NewTestMerges.Where(x => String.IsNullOrWhiteSpace(x.PullRequestRevision))) - I.PullRequestRevision = null; + var committish = model.CheckoutSha ?? model.Reference; + var isSha = await repo.IsSha(committish, cancellationToken).ConfigureAwait(false); - var gitHubClient = currentModel.AccessToken != null - ? gitHubClientFactory.CreateClient(currentModel.AccessToken) - : (String.IsNullOrEmpty(generalConfiguration.GitHubAccessToken) - ? gitHubClientFactory.CreateClient() - : gitHubClientFactory.CreateClient(generalConfiguration.GitHubAccessToken)); + if ((isSha && model.Reference != null) || (!isSha && model.CheckoutSha != null)) + throw new JobException(ErrorCode.RepoSwappedShaOrReference); - var repoOwner = repo.GitHubOwner; - var repoName = repo.GitHubRepoName; + await repo.CheckoutObject(committish, NextProgressReporter(), ct).ConfigureAwait(false); + await LoadRevisionInformation(repo, databaseContext, attachedInstance, null, x => lastRevisionInfo = x, ct).ConfigureAwait(false); // we've either seen origin before or what we're checking out is on origin + } + else + NextProgressReporter()(100); - // optimization: if we've already merged these exact same commits in this fashion before, just find the rev info for it and check it out - Models.RevisionInformation revInfoWereLookingFor = null; - bool needToApplyRemainingPrs = true; - if (lastRevisionInfo.OriginCommitSha == lastRevisionInfo.CommitSha) + if (hardResettingToOriginReference) + { + if (!repo.Tracking) + throw new JobException(ErrorCode.RepoReferenceNotTracking); + await repo.ResetToOrigin(NextProgressReporter(), ct).ConfigureAwait(false); + await repo.Sychronize(currentModel.AccessUser, currentModel.AccessToken, currentModel.CommitterName, currentModel.CommitterEmail, NextProgressReporter(), true, ct).ConfigureAwait(false); + await LoadRevisionInformation(repo, databaseContext, attachedInstance, null, x => lastRevisionInfo = x, ct).ConfigureAwait(false); + + // repo head is on origin so force this + // will update the db if necessary + lastRevisionInfo.OriginCommitSha = repo.Head; + } + } + + // test merging + Dictionary prMap = null; + if (newTestMerges) + { + // bit of sanitization + foreach (var I in model.NewTestMerges.Where(x => String.IsNullOrWhiteSpace(x.PullRequestRevision))) + I.PullRequestRevision = null; + + var gitHubClient = currentModel.AccessToken != null + ? gitHubClientFactory.CreateClient(currentModel.AccessToken) + : (String.IsNullOrEmpty(generalConfiguration.GitHubAccessToken) + ? gitHubClientFactory.CreateClient() + : gitHubClientFactory.CreateClient(generalConfiguration.GitHubAccessToken)); + + var repoOwner = repo.GitHubOwner; + var repoName = repo.GitHubRepoName; + + // optimization: if we've already merged these exact same commits in this fashion before, just find the rev info for it and check it out + Models.RevisionInformation revInfoWereLookingFor = null; + bool needToApplyRemainingPrs = true; + if (lastRevisionInfo.OriginCommitSha == lastRevisionInfo.CommitSha) + { + // In order for this to work though we need the shas of all the commits + if (model.NewTestMerges.Any(x => x.PullRequestRevision == null)) + prMap = new Dictionary(); + + bool cantSearch = false; + foreach (var I in model.NewTestMerges) { - // In order for this to work though we need the shas of all the commits - if (model.NewTestMerges.Any(x => x.PullRequestRevision == null)) - prMap = new Dictionary(); - - bool cantSearch = false; - foreach (var I in model.NewTestMerges) - { - if (I.PullRequestRevision != null) + if (I.PullRequestRevision != null) #pragma warning disable CA1308 // Normalize strings to uppercase - I.PullRequestRevision = I.PullRequestRevision?.ToLowerInvariant(); // ala libgit2 + I.PullRequestRevision = I.PullRequestRevision?.ToLowerInvariant(); // ala libgit2 #pragma warning restore CA1308 // Normalize strings to uppercase - else - try - { - // retrieve the latest sha - var pr = await gitHubClient.PullRequest.Get(repoOwner, repoName, I.Number).ConfigureAwait(false); - prMap.Add(I.Number, pr); - I.PullRequestRevision = pr.Head.Sha; - } - catch - { - cantSearch = true; - break; - } - } - - if (!cantSearch) - { - var dbPull = await databaseContext.RevisionInformations - .Where(x => x.Instance.Id == Instance.Id - && x.OriginCommitSha == lastRevisionInfo.OriginCommitSha - && x.ActiveTestMerges.Count <= model.NewTestMerges.Count - && x.ActiveTestMerges.Count > 0) - .Include(x => x.ActiveTestMerges) - .ThenInclude(x => x.TestMerge) - .ToListAsync(cancellationToken).ConfigureAwait(false); - - // split here cause this bit has to be done locally - revInfoWereLookingFor = dbPull - .Where(x => x.ActiveTestMerges.Count == model.NewTestMerges.Count - && x.ActiveTestMerges.Select(y => y.TestMerge) - .All(y => model.NewTestMerges.Any(z => - y.Number == z.Number - && y.PullRequestRevision.StartsWith(z.PullRequestRevision, StringComparison.Ordinal) - && (y.Comment?.Trim().ToUpperInvariant() == z.Comment?.Trim().ToUpperInvariant() || z.Comment == null)))) - .FirstOrDefault(); - - if (revInfoWereLookingFor == null && model.NewTestMerges.Count > 1) - { - // okay try to add at least SOME prs we've seen before - var search = model.NewTestMerges.ToList(); - - var appliedTestMergeIds = new List(); - - Models.RevisionInformation lastGoodRevInfo = null; - do - { - foreach (var I in search) - { - revInfoWereLookingFor = dbPull - .Where(x => model.NewTestMerges.Any(z => - x.PrimaryTestMerge.Number == z.Number - && x.PrimaryTestMerge.PullRequestRevision.StartsWith(z.PullRequestRevision, StringComparison.Ordinal) - && (x.PrimaryTestMerge.Comment?.Trim().ToUpperInvariant() == z.Comment?.Trim().ToUpperInvariant() || z.Comment == null)) - && x.ActiveTestMerges.Select(y => y.TestMerge).All(y => appliedTestMergeIds.Contains(y.Id))) - .FirstOrDefault(); - - if (revInfoWereLookingFor != null) - { - lastGoodRevInfo = revInfoWereLookingFor; - appliedTestMergeIds.Add(revInfoWereLookingFor.PrimaryTestMerge.Id); - search.Remove(I); - break; - } - } - } - while (revInfoWereLookingFor != null && search.Count > 0); - - revInfoWereLookingFor = lastGoodRevInfo; - needToApplyRemainingPrs = search.Count != 0; - if (needToApplyRemainingPrs) - model.NewTestMerges = search; - } - else if (revInfoWereLookingFor != null) - needToApplyRemainingPrs = false; - } - } - - if (revInfoWereLookingFor != null) - { - // goteem - await repo.ResetToSha(revInfoWereLookingFor.CommitSha, NextProgressReporter(), cancellationToken).ConfigureAwait(false); - lastRevisionInfo = revInfoWereLookingFor; - } - - if (needToApplyRemainingPrs) - { - // an invocation of LoadRevisionInformation could have already loaded this user - var contextUser = databaseContext.Users.Local.Where(x => x.Id == AuthenticationContext.User.Id).FirstOrDefault(); - if (contextUser == default) - { - // No reason to call the DB, just attach it - contextUser = new Models.User - { - Id = AuthenticationContext.User.Id - }; - databaseContext.Users.Attach(contextUser); - } else - Logger.LogTrace("Skipping attaching the user to the database context as it is already loaded!"); - - foreach (var I in model.NewTestMerges) - { - Octokit.PullRequest pr = null; - string errorMessage = null; - - if (lastRevisionInfo.ActiveTestMerges.Any(x => x.TestMerge.Number == I.Number)) - throw new JobException(ErrorCode.RepoDuplicateTestMerge); - - Exception exception = null; try { - // load from cache if possible - if (prMap == null || !prMap.TryGetValue(I.Number, out pr)) - pr = await gitHubClient.PullRequest.Get(repoOwner, repoName, I.Number).ConfigureAwait(false); - } - catch (Octokit.RateLimitExceededException ex) - { - // you look at your anonymous access and sigh - errorMessage = "REMOTE API ERROR: RATE LIMITED"; - exception = ex; - } - catch (Octokit.AuthorizationException ex) - { - errorMessage = "REMOTE API ERROR: BAD CREDENTIALS"; - exception = ex; - } - catch (Octokit.NotFoundException ex) - { - // you look at your shithub and sigh - errorMessage = "REMOTE API ERROR: PULL REQUEST NOT FOUND"; - exception = ex; - } - - if (exception != null) - Logger.LogWarning("Error retrieving pull request metadata: {0}", exception); - - // we want to take the earliest truth possible to prevent RCEs, if this fails AddTestMerge will set it - if (I.PullRequestRevision == null && pr != null) + // retrieve the latest sha + var pr = await gitHubClient.PullRequest.Get(repoOwner, repoName, I.Number).ConfigureAwait(false); + prMap.Add(I.Number, pr); I.PullRequestRevision = pr.Head.Sha; - - var mergeResult = await repo.AddTestMerge( - I, - committerName, - currentModel.CommitterEmail, - currentModel.AccessUser, - currentModel.AccessToken, - NextProgressReporter(), - ct).ConfigureAwait(false); - - if (!mergeResult.HasValue) - throw new JobException( - ErrorCode.RepoTestMergeConflict, - new JobException( - $"Merge of PR #{I.Number} at {I.PullRequestRevision.Substring(0, 7)} conflicted!")); - - ++doneSteps; - - var revInfoUpdateTask = UpdateRevInfo(); - - var tm = new Models.TestMerge + } + catch { - Author = pr?.User.Login ?? errorMessage, - BodyAtMerge = pr?.Body ?? errorMessage ?? String.Empty, - MergedAt = DateTimeOffset.Now, - TitleAtMerge = pr?.Title ?? errorMessage ?? String.Empty, - Comment = I.Comment, - Number = I.Number, - MergedBy = contextUser, - PullRequestRevision = I.PullRequestRevision, - Url = pr?.HtmlUrl ?? errorMessage - }; + cantSearch = true; + break; + } + } - await revInfoUpdateTask.ConfigureAwait(false); + if (!cantSearch) + { + var dbPull = await databaseContext.RevisionInformations + .Where(x => x.Instance.Id == Instance.Id + && x.OriginCommitSha == lastRevisionInfo.OriginCommitSha + && x.ActiveTestMerges.Count <= model.NewTestMerges.Count + && x.ActiveTestMerges.Count > 0) + .Include(x => x.ActiveTestMerges) + .ThenInclude(x => x.TestMerge) + .ToListAsync(cancellationToken).ConfigureAwait(false); - lastRevisionInfo.PrimaryTestMerge = tm; - lastRevisionInfo.ActiveTestMerges.Add(new RevInfoTestMerge + // split here cause this bit has to be done locally + revInfoWereLookingFor = dbPull + .Where(x => x.ActiveTestMerges.Count == model.NewTestMerges.Count + && x.ActiveTestMerges.Select(y => y.TestMerge) + .All(y => model.NewTestMerges.Any(z => + y.Number == z.Number + && y.PullRequestRevision.StartsWith(z.PullRequestRevision, StringComparison.Ordinal) + && (y.Comment?.Trim().ToUpperInvariant() == z.Comment?.Trim().ToUpperInvariant() || z.Comment == null)))) + .FirstOrDefault(); + + if (revInfoWereLookingFor == null && model.NewTestMerges.Count > 1) + { + // okay try to add at least SOME prs we've seen before + var search = model.NewTestMerges.ToList(); + + var appliedTestMergeIds = new List(); + + Models.RevisionInformation lastGoodRevInfo = null; + do { - TestMerge = tm - }); + foreach (var I in search) + { + revInfoWereLookingFor = dbPull + .Where(x => model.NewTestMerges.Any(z => + x.PrimaryTestMerge.Number == z.Number + && x.PrimaryTestMerge.PullRequestRevision.StartsWith(z.PullRequestRevision, StringComparison.Ordinal) + && (x.PrimaryTestMerge.Comment?.Trim().ToUpperInvariant() == z.Comment?.Trim().ToUpperInvariant() || z.Comment == null)) + && x.ActiveTestMerges.Select(y => y.TestMerge).All(y => appliedTestMergeIds.Contains(y.Id))) + .FirstOrDefault(); + + if (revInfoWereLookingFor != null) + { + lastGoodRevInfo = revInfoWereLookingFor; + appliedTestMergeIds.Add(revInfoWereLookingFor.PrimaryTestMerge.Id); + search.Remove(I); + break; + } + } + } + while (revInfoWereLookingFor != null && search.Count > 0); + + revInfoWereLookingFor = lastGoodRevInfo; + needToApplyRemainingPrs = search.Count != 0; + if (needToApplyRemainingPrs) + model.NewTestMerges = search; } + else if (revInfoWereLookingFor != null) + needToApplyRemainingPrs = false; } } - var currentHead = repo.Head; - if (startSha != currentHead || (postUpdateSha != null && postUpdateSha != currentHead)) + if (revInfoWereLookingFor != null) { - await repo.Sychronize(currentModel.AccessUser, currentModel.AccessToken, currentModel.CommitterName, currentModel.CommitterEmail, NextProgressReporter(), false, ct).ConfigureAwait(false); - await UpdateRevInfo().ConfigureAwait(false); + // goteem + await repo.ResetToSha(revInfoWereLookingFor.CommitSha, NextProgressReporter(), cancellationToken).ConfigureAwait(false); + lastRevisionInfo = revInfoWereLookingFor; } - await databaseContext.Save(ct).ConfigureAwait(false); - } - catch - { - doneSteps = 0; - numSteps = 2; + if (needToApplyRemainingPrs) + { + // an invocation of LoadRevisionInformation could have already loaded this user + var contextUser = databaseContext.Users.Local.Where(x => x.Id == AuthenticationContext.User.Id).FirstOrDefault(); + if (contextUser == default) + { + // No reason to call the DB, just attach it + contextUser = new Models.User + { + Id = AuthenticationContext.User.Id + }; + databaseContext.Users.Attach(contextUser); + } + else + Logger.LogTrace("Skipping attaching the user to the database context as it is already loaded!"); - // the stuff didn't make it into the db, forget what we've done and abort - await repo.CheckoutObject(startReference ?? startSha, NextProgressReporter(), default).ConfigureAwait(false); - if (startReference != null && repo.Head != startSha) - await repo.ResetToSha(startSha, NextProgressReporter(), default).ConfigureAwait(false); - else - progressReporter(100); - throw; + foreach (var I in model.NewTestMerges) + { + Octokit.PullRequest pr = null; + string errorMessage = null; + + if (lastRevisionInfo.ActiveTestMerges.Any(x => x.TestMerge.Number == I.Number)) + throw new JobException(ErrorCode.RepoDuplicateTestMerge); + + Exception exception = null; + try + { + // load from cache if possible + if (prMap == null || !prMap.TryGetValue(I.Number, out pr)) + pr = await gitHubClient.PullRequest.Get(repoOwner, repoName, I.Number).ConfigureAwait(false); + } + catch (Octokit.RateLimitExceededException ex) + { + // you look at your anonymous access and sigh + errorMessage = "REMOTE API ERROR: RATE LIMITED"; + exception = ex; + } + catch (Octokit.AuthorizationException ex) + { + errorMessage = "REMOTE API ERROR: BAD CREDENTIALS"; + exception = ex; + } + catch (Octokit.NotFoundException ex) + { + // you look at your shithub and sigh + errorMessage = "REMOTE API ERROR: PULL REQUEST NOT FOUND"; + exception = ex; + } + + if (exception != null) + Logger.LogWarning("Error retrieving pull request metadata: {0}", exception); + + // we want to take the earliest truth possible to prevent RCEs, if this fails AddTestMerge will set it + if (I.PullRequestRevision == null && pr != null) + I.PullRequestRevision = pr.Head.Sha; + + var mergeResult = await repo.AddTestMerge( + I, + committerName, + currentModel.CommitterEmail, + currentModel.AccessUser, + currentModel.AccessToken, + NextProgressReporter(), + ct).ConfigureAwait(false); + + if (!mergeResult.HasValue) + throw new JobException( + ErrorCode.RepoTestMergeConflict, + new JobException( + $"Merge of PR #{I.Number} at {I.PullRequestRevision.Substring(0, 7)} conflicted!")); + + ++doneSteps; + + var revInfoUpdateTask = UpdateRevInfo(); + + var tm = new Models.TestMerge + { + Author = pr?.User.Login ?? errorMessage, + BodyAtMerge = pr?.Body ?? errorMessage ?? String.Empty, + MergedAt = DateTimeOffset.Now, + TitleAtMerge = pr?.Title ?? errorMessage ?? String.Empty, + Comment = I.Comment, + Number = I.Number, + MergedBy = contextUser, + PullRequestRevision = I.PullRequestRevision, + Url = pr?.HtmlUrl ?? errorMessage + }; + + await revInfoUpdateTask.ConfigureAwait(false); + + lastRevisionInfo.PrimaryTestMerge = tm; + lastRevisionInfo.ActiveTestMerges.Add(new RevInfoTestMerge + { + TestMerge = tm + }); + } + } } + + var currentHead = repo.Head; + if (startSha != currentHead || (postUpdateSha != null && postUpdateSha != currentHead)) + { + await repo.Sychronize(currentModel.AccessUser, currentModel.AccessToken, currentModel.CommitterName, currentModel.CommitterEmail, NextProgressReporter(), false, ct).ConfigureAwait(false); + await UpdateRevInfo().ConfigureAwait(false); + } + + await databaseContext.Save(ct).ConfigureAwait(false); + } + catch + { + doneSteps = 0; + numSteps = 2; + + // the stuff didn't make it into the db, forget what we've done and abort + await repo.CheckoutObject(startReference ?? startSha, NextProgressReporter(), default).ConfigureAwait(false); + if (startReference != null && repo.Head != startSha) + await repo.ResetToSha(startSha, NextProgressReporter(), default).ConfigureAwait(false); + else + progressReporter(100); + throw; } }, cancellationToken).ConfigureAwait(false); From e180dcb279c2046d9a955eafba29c58d546f0279 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 18 May 2020 16:07:33 -0400 Subject: [PATCH 5/7] Fix warning --- src/Tgstation.Server.Host/Controllers/RepositoryController.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Tgstation.Server.Host/Controllers/RepositoryController.cs b/src/Tgstation.Server.Host/Controllers/RepositoryController.cs index 9dc34a435e..9e70de265a 100644 --- a/src/Tgstation.Server.Host/Controllers/RepositoryController.cs +++ b/src/Tgstation.Server.Host/Controllers/RepositoryController.cs @@ -179,6 +179,7 @@ namespace Tgstation.Server.Host.Controllers return Conflict(new ErrorMessage(ErrorCode.RepoBusy)); using var repo = await repoManager.LoadRepository(cancellationToken).ConfigureAwait(false); + // clone conflict if (repo != null) return Conflict(new ErrorMessage(ErrorCode.RepoExists)); From bc434d1df6012d65c33a4bada9d2889f97137b33 Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 18 May 2020 16:35:39 -0400 Subject: [PATCH 6/7] Patch the watchdog shutdown crash --- .../Components/Watchdog/WatchdogBase.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs b/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs index ecfd994cd4..893591612c 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs @@ -147,6 +147,11 @@ namespace Tgstation.Server.Host.Components.Watchdog /// bool running; + /// + /// If the has been d. + /// + bool disposed; + /// /// Initializes a new instance of the . /// @@ -216,6 +221,7 @@ namespace Tgstation.Server.Host.Components.Watchdog restartRegistration.Dispose(); DisposeAndNullControllers(); monitorCts?.Dispose(); + disposed = true; } /// @@ -672,7 +678,9 @@ namespace Tgstation.Server.Host.Components.Watchdog false, cancellationToken); - if (monitorState.NextAction != MonitorAction.Exit) + if (disposed) + monitorState.NextAction = MonitorAction.Exit; + else if (monitorState.NextAction != MonitorAction.Exit) monitorState = await MonitorRestart(cancellationToken).ConfigureAwait(false); await chatTask.ConfigureAwait(false); From 3ecc78fe267d7a0b290239601070f4b71c96ab5e Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Mon, 18 May 2020 16:55:41 -0400 Subject: [PATCH 7/7] Fix a message's formatting --- src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs b/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs index 893591612c..b694624a7d 100644 --- a/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs +++ b/src/Tgstation.Server.Host/Components/Watchdog/WatchdogBase.cs @@ -674,7 +674,7 @@ namespace Tgstation.Server.Host.Components.Watchdog ? "Restarting" : "Shutting down"; var chatTask = Chat.SendWatchdogMessage( - $"Monitor crashed, this should NEVER happen! Please report this, full details in logs!{nextActionMessage}. Error: {e.Message}", + $"Monitor crashed, this should NEVER happen! Please report this, full details in logs! {nextActionMessage}. Error: {e.Message}", false, cancellationToken);