Fix race condition and wrong instance status when cancelling instance launch

Agent/Phantom.Agent.Minecraft/Instance/InstanceProcess.cs

@@ -4,19 +4,19 @@ using Phantom.Utils.Runtime;
 
 namespace Phantom.Agent.Minecraft.Instance; 
 
-public sealed class InstanceSession : IDisposable {
+public sealed class InstanceProcess : IDisposable {
 	public InstanceProperties InstanceProperties { get; }
 	public CancellableSemaphore BackupSemaphore { get; } = new (1, 1);
 	
 	private readonly RingBuffer<string> outputBuffer = new (10000);
 	private event EventHandler<string>? OutputEvent;
 
-	public event EventHandler? SessionEnded;
+	public event EventHandler? Ended;
 	public bool HasEnded { get; private set; }
 
 	private readonly Process process;
 
-	internal InstanceSession(InstanceProperties instanceProperties, Process process) {
+	internal InstanceProcess(InstanceProperties instanceProperties, Process process) {
 		this.InstanceProperties = instanceProperties;
 		this.process = process;
 		this.process.EnableRaisingEvents = true;
@@ -51,7 +51,7 @@ public sealed class InstanceSession : IDisposable {
 	private void ProcessOnExited(object? sender, EventArgs e) {
 		OutputEvent = null;
 		HasEnded = true;
-		SessionEnded?.Invoke(this, EventArgs.Empty);
+		Ended?.Invoke(this, EventArgs.Empty);
 	}
 
 	public void Kill() {
@@ -68,6 +68,6 @@ public sealed class InstanceSession : IDisposable {
 		process.Dispose();
 		BackupSemaphore.Dispose();
 		OutputEvent = null;
-		SessionEnded = null;
+		Ended = null;
 	}
 }

Agent/Phantom.Agent.Minecraft/Launcher/BaseLauncher.cs

@@ -51,7 +51,7 @@ public abstract class BaseLauncher {
 		processArguments.Add("nogui");
 
 		var process = new Process { StartInfo = startInfo };
-		var session = new InstanceSession(instanceProperties, process);
+		var instanceProcess = new InstanceProcess(instanceProperties, process);
 
 		try {
 			await AcceptEula(instanceProperties);
@@ -77,7 +77,7 @@ public abstract class BaseLauncher {
 			return new LaunchResult.CouldNotStartMinecraftServer();
 		}
 
-		return new LaunchResult.Success(session);
+		return new LaunchResult.Success(instanceProcess);
 	}
 
 	private protected virtual void CustomizeJvmArguments(JvmArgumentBuilder arguments) {}

Agent/Phantom.Agent.Minecraft/Launcher/LaunchResult.cs

@@ -5,7 +5,7 @@ namespace Phantom.Agent.Minecraft.Launcher;
 public abstract record LaunchResult {
 	private LaunchResult() {}
 
-	public sealed record Success(InstanceSession Session) : LaunchResult;
+	public sealed record Success(InstanceProcess Process) : LaunchResult;
 
 	public sealed record InvalidJavaRuntime : LaunchResult;
 	

Agent/Phantom.Agent.Services/Backups/BackupManager.cs

@@ -16,9 +16,9 @@ sealed partial class BackupManager {
 		this.temporaryBasePath = Path.Combine(agentFolders.TemporaryFolderPath, "backups");
 	}
 
-	public async Task<BackupCreationResult> CreateBackup(string loggerName, InstanceSession session, CancellationToken cancellationToken) {
+	public async Task<BackupCreationResult> CreateBackup(string loggerName, InstanceProcess process, CancellationToken cancellationToken) {
 		try {
-			if (!await session.BackupSemaphore.Wait(TimeSpan.FromSeconds(1), cancellationToken)) {
+			if (!await process.BackupSemaphore.Wait(TimeSpan.FromSeconds(1), cancellationToken)) {
 				return new BackupCreationResult(BackupCreationResultKind.BackupAlreadyRunning);
 			}
 		} catch (ObjectDisposedException) {
@@ -28,9 +28,9 @@ sealed partial class BackupManager {
 		}
 
 		try {
-			return await new BackupCreator(destinationBasePath, temporaryBasePath, loggerName, session, cancellationToken).CreateBackup();
+			return await new BackupCreator(destinationBasePath, temporaryBasePath, loggerName, process, cancellationToken).CreateBackup();
 		} finally {
-			session.BackupSemaphore.Release();
+			process.BackupSemaphore.Release();
 		}
 	}
 
@@ -39,23 +39,23 @@ sealed partial class BackupManager {
 		private readonly string temporaryBasePath;
 		private readonly string loggerName;
 		private readonly ILogger logger;
-		private readonly InstanceSession session;
+		private readonly InstanceProcess process;
 		private readonly BackupCommandListener listener;
 		private readonly CancellationToken cancellationToken;
 
-		public BackupCreator(string destinationBasePath, string temporaryBasePath, string loggerName, InstanceSession session, CancellationToken cancellationToken) {
+		public BackupCreator(string destinationBasePath, string temporaryBasePath, string loggerName, InstanceProcess process, CancellationToken cancellationToken) {
 			this.destinationBasePath = destinationBasePath;
 			this.temporaryBasePath = temporaryBasePath;
 			this.loggerName = loggerName;
 			this.logger = PhantomLogger.Create<BackupManager>(loggerName);
-			this.session = session;
+			this.process = process;
 			this.listener = new BackupCommandListener(logger);
 			this.cancellationToken = cancellationToken;
 		}
 
 		public async Task<BackupCreationResult> CreateBackup() {
 			logger.Information("Backup started.");
-			session.AddOutputListener(listener.OnOutput, maxLinesToReadFromHistory: 0);
+			process.AddOutputListener(listener.OnOutput, maxLinesToReadFromHistory: 0);
 			try {
 				var resultBuilder = new BackupCreationResult.Builder();
 				
@@ -77,7 +77,7 @@ sealed partial class BackupManager {
 				
 				return result;
 			} finally {
-				session.RemoveOutputListener(listener.OnOutput);
+				process.RemoveOutputListener(listener.OnOutput);
 			}
 		}
 		
@@ -85,7 +85,7 @@ sealed partial class BackupManager {
 			try {
 				await DisableAutomaticSaving();
 				await SaveAllChunks();
-				await new BackupArchiver(destinationBasePath, temporaryBasePath, loggerName, session.InstanceProperties, cancellationToken).ArchiveWorld(resultBuilder);
+				await new BackupArchiver(destinationBasePath, temporaryBasePath, loggerName, process.InstanceProperties, cancellationToken).ArchiveWorld(resultBuilder);
 			} catch (OperationCanceledException) {
 				resultBuilder.Kind = BackupCreationResultKind.BackupCancelled;
 				logger.Warning("Backup creation was cancelled.");
@@ -105,18 +105,18 @@ sealed partial class BackupManager {
 		}
 
 		private async Task DisableAutomaticSaving() {
-			await session.SendCommand(MinecraftCommand.SaveOff, cancellationToken);
+			await process.SendCommand(MinecraftCommand.SaveOff, cancellationToken);
 			await listener.AutomaticSavingDisabled.Task.WaitAsync(cancellationToken);
 		}
 
 		private async Task SaveAllChunks() {
 			// TODO Try if not flushing and waiting a few seconds before flushing reduces lag.
-			await session.SendCommand(MinecraftCommand.SaveAll(flush: true), cancellationToken);
+			await process.SendCommand(MinecraftCommand.SaveAll(flush: true), cancellationToken);
 			await listener.SavedTheGame.Task.WaitAsync(cancellationToken);
 		}
 
 		private async Task EnableAutomaticSaving() {
-			await session.SendCommand(MinecraftCommand.SaveOn, cancellationToken);
+			await process.SendCommand(MinecraftCommand.SaveOn, cancellationToken);
 			await listener.AutomaticSavingEnabled.Task.WaitAsync(cancellationToken);
 		}
 	}

Agent/Phantom.Agent.Services/Backups/BackupScheduler.cs

@@ -14,17 +14,17 @@ sealed class BackupScheduler : CancellableBackgroundTask {
 
 	private readonly string loggerName;
 	private readonly BackupManager backupManager;
-	private readonly InstanceSession session;
+	private readonly InstanceProcess process;
 	private readonly int serverPort;
 	private readonly ServerStatusProtocol serverStatusProtocol;
 	private readonly ManualResetEventSlim serverOutputWhileWaitingForOnlinePlayers = new ();
 	
 	public event EventHandler<BackupCreationResult>? BackupCompleted; 
 
-	public BackupScheduler(TaskManager taskManager, BackupManager backupManager, InstanceSession session, int serverPort, string loggerName) : base(PhantomLogger.Create<BackupScheduler>(loggerName), taskManager, "Backup scheduler for " + loggerName) {
+	public BackupScheduler(TaskManager taskManager, BackupManager backupManager, InstanceProcess process, int serverPort, string loggerName) : base(PhantomLogger.Create<BackupScheduler>(loggerName), taskManager, "Backup scheduler for " + loggerName) {
 		this.loggerName = loggerName;
 		this.backupManager = backupManager;
-		this.session = session;
+		this.process = process;
 		this.serverPort = serverPort;
 		this.serverStatusProtocol = new ServerStatusProtocol(loggerName);
 	}
@@ -50,13 +50,13 @@ sealed class BackupScheduler : CancellableBackgroundTask {
 	}
 
 	private async Task<BackupCreationResult> CreateBackup() {
-		return await backupManager.CreateBackup(loggerName, session, CancellationToken.None);
+		return await backupManager.CreateBackup(loggerName, process, CancellationToken.None);
 	}
 
 	private async Task WaitForOnlinePlayers() {
 		bool needsToLogOfflinePlayersMessage = true;
 		
-		session.AddOutputListener(ServerOutputListener, maxLinesToReadFromHistory: 0);
+		process.AddOutputListener(ServerOutputListener, maxLinesToReadFromHistory: 0);
 		try {
 			while (!CancellationToken.IsCancellationRequested) {
 				serverOutputWhileWaitingForOnlinePlayers.Reset();
@@ -83,7 +83,7 @@ sealed class BackupScheduler : CancellableBackgroundTask {
 				await serverOutputWhileWaitingForOnlinePlayers.WaitHandle.WaitOneAsync(CancellationToken);
 			}
 		} finally {
-			session.RemoveOutputListener(ServerOutputListener);
+			process.RemoveOutputListener(ServerOutputListener);
 		}
 	}
 

Agent/Phantom.Agent.Services/Instances/Sessions/InstanceSession.cs

@@ -0,0 +1,28 @@
+using Phantom.Agent.Minecraft.Instance;
+
+namespace Phantom.Agent.Services.Instances.Sessions; 
+
+sealed class InstanceSession : IDisposable {
+	private readonly InstanceProcess process;
+	private readonly InstanceContext context;
+	private readonly InstanceLogSender logSender;
+
+	public InstanceSession(InstanceProcess process, InstanceContext context) {
+		this.process = process;
+		this.context = context;
+		this.logSender = new InstanceLogSender(context.Services.TaskManager, context.Configuration.InstanceGuid, context.ShortName);
+		
+		this.process.AddOutputListener(SessionOutput);
+	}
+	
+	private void SessionOutput(object? sender, string line) {
+		context.Logger.Verbose("[Server] {Line}", line);
+		logSender.Enqueue(line);
+	}
+	
+	public void Dispose() {
+		logSender.Stop();
+		process.Dispose();
+		context.Services.PortManager.Release(context.Configuration);
+	}
+}

Agent/Phantom.Agent.Services/Instances/States/InstanceLaunchingState.cs

@@ -1,6 +1,7 @@
 using Phantom.Agent.Minecraft.Instance;
 using Phantom.Agent.Minecraft.Launcher;
 using Phantom.Agent.Minecraft.Server;
+using Phantom.Agent.Services.Instances.Sessions;
 using Phantom.Common.Data.Instance;
 using Phantom.Common.Data.Minecraft;
 using Phantom.Common.Data.Replies;
@@ -24,7 +25,7 @@ sealed class InstanceLaunchingState : IInstanceState, IDisposable {
 		launchTask.ContinueWith(OnLaunchFailure, CancellationToken.None, TaskContinuationOptions.NotOnRanToCompletion, TaskScheduler.Default);
 	}
 
-	private async Task<InstanceSession> DoLaunch() {
+	private async Task<InstanceProcess> DoLaunch() {
 		var cancellationToken = cancellationTokenSource.Token;
 		cancellationToken.ThrowIfCancellationRequested();
 
@@ -59,29 +60,35 @@ sealed class InstanceLaunchingState : IInstanceState, IDisposable {
 		}
 
 		context.SetStatus(InstanceStatus.Launching);
-		return launchSuccess.Session;
+		return launchSuccess.Process;
 	}
 
-	private void OnLaunchSuccess(Task<InstanceSession> task) {
+	private void OnLaunchSuccess(Task<InstanceProcess> task) {
 		context.TransitionState(() => {
+			context.ReportEvent(InstanceEvent.LaunchSucceded);
+			
+			var process = task.Result;
+			var session = new InstanceSession(process, context);
+			
 			if (cancellationTokenSource.IsCancellationRequested) {
-				context.Services.PortManager.Release(context.Configuration);
-				return (new InstanceNotRunningState(), InstanceStatus.NotRunning);
+				return (new InstanceStoppingState(context, process, session), InstanceStatus.Stopping);
 			}
 			else {
-				context.ReportEvent(InstanceEvent.LaunchSucceded);
-				return (new InstanceRunningState(context, task.Result), null);
+				return (new InstanceRunningState(context, process, session), null);
 			}
 		});
 	}
 
 	private void OnLaunchFailure(Task task) {
-		if (task.Exception is { InnerException: LaunchFailureException e }) {
-			context.Logger.Error(e.LogMessage);
-			context.SetLaunchFailedStatusAndReportEvent(e.Reason);
-		}
-		else {
-			context.SetLaunchFailedStatusAndReportEvent(InstanceLaunchFailReason.UnknownError);
+		if (task.IsFaulted) {
+			if (task.Exception is { InnerException: LaunchFailureException e }) {
+				context.Logger.Error(e.LogMessage);
+				context.SetLaunchFailedStatusAndReportEvent(e.Reason);
+			}
+			else {
+				context.Logger.Error(task.Exception, "Caught exception while launching instance.");
+				context.SetLaunchFailedStatusAndReportEvent(InstanceLaunchFailReason.UnknownError);
+			}
 		}
 
 		context.Services.PortManager.Release(context.Configuration);

Agent/Phantom.Agent.Services/Instances/States/InstanceRunningState.cs

@@ -1,6 +1,7 @@
 using Phantom.Agent.Minecraft.Command;
 using Phantom.Agent.Minecraft.Instance;
 using Phantom.Agent.Services.Backups;
+using Phantom.Agent.Services.Instances.Sessions;
 using Phantom.Common.Data.Backups;
 using Phantom.Common.Data.Instance;
 using Phantom.Common.Data.Minecraft;
@@ -10,30 +11,27 @@ namespace Phantom.Agent.Services.Instances.States;
 
 sealed class InstanceRunningState : IInstanceState {
 	private readonly InstanceContext context;
-	private readonly InstanceSession session;
-	private readonly InstanceLogSender logSender;
+	private readonly InstanceProcess process;
 	private readonly BackupScheduler backupScheduler;
-	private readonly SessionObjects sessionObjects;
+	private readonly RunningSessionDisposer runningSessionDisposer;
 	
 	private readonly CancellationTokenSource delayedStopCancellationTokenSource = new ();
 	private bool stateOwnsDelayedStopCancellationTokenSource = true;
 	private bool isStopping;
 
-	public InstanceRunningState(InstanceContext context, InstanceSession session) {
+	public InstanceRunningState(InstanceContext context, InstanceProcess process, InstanceSession session) {
 		this.context = context;
-		this.session = session;
-		this.logSender = new InstanceLogSender(context.Services.TaskManager, context.Configuration.InstanceGuid, context.ShortName);
-		this.backupScheduler = new BackupScheduler(context.Services.TaskManager, context.Services.BackupManager, session, context.Configuration.ServerPort, context.ShortName);
+		this.process = process;
+		this.backupScheduler = new BackupScheduler(context.Services.TaskManager, context.Services.BackupManager, process, context.Configuration.ServerPort, context.ShortName);
 		this.backupScheduler.BackupCompleted += OnScheduledBackupCompleted;
-		this.sessionObjects = new SessionObjects(this);
+		this.runningSessionDisposer = new RunningSessionDisposer(this, session);
 	}
 
 	public void Initialize() {
-		session.AddOutputListener(SessionOutput);
-		session.SessionEnded += SessionEnded;
+		process.Ended += ProcessEnded;
 		
-		if (session.HasEnded) {
-			if (sessionObjects.Dispose()) {
+		if (process.HasEnded) {
+			if (runningSessionDisposer.Dispose()) {
 				context.Logger.Warning("Session ended immediately after it was started.");
 				context.ReportEvent(InstanceEvent.Stopped);
 				context.Services.TaskManager.Run("Transition state of instance " + context.ShortName + " to not running", () => context.TransitionState(new InstanceNotRunningState(), InstanceStatus.Failed(InstanceLaunchFailReason.UnknownError)));
@@ -45,13 +43,8 @@ sealed class InstanceRunningState : IInstanceState {
 		}
 	}
 
-	private void SessionOutput(object? sender, string line) {
-		context.Logger.Verbose("[Server] {Line}", line);
-		logSender.Enqueue(line);
-	}
-
-	private void SessionEnded(object? sender, EventArgs e) {
-		if (!sessionObjects.Dispose()) {
+	private void ProcessEnded(object? sender, EventArgs e) {
+		if (!runningSessionDisposer.Dispose()) {
 			return;
 		}
 
@@ -88,9 +81,9 @@ sealed class InstanceRunningState : IInstanceState {
 	}
 
 	private IInstanceState PrepareStoppedState() {
-		session.SessionEnded -= SessionEnded;
+		process.Ended -= ProcessEnded;
 		backupScheduler.Stop();
-		return new InstanceStoppingState(context, session, sessionObjects);
+		return new InstanceStoppingState(context, process, runningSessionDisposer);
 	}
 
 	private void CancelDelayedStop() {
@@ -134,7 +127,7 @@ sealed class InstanceRunningState : IInstanceState {
 	public async Task<bool> SendCommand(string command, CancellationToken cancellationToken) {
 		try {
 			context.Logger.Information("Sending command: {Command}", command);
-			await session.SendCommand(command, cancellationToken);
+			await process.SendCommand(command, cancellationToken);
 			return true;
 		} catch (OperationCanceledException) {
 			return false;
@@ -148,12 +141,14 @@ sealed class InstanceRunningState : IInstanceState {
 		context.ReportEvent(new InstanceBackupCompletedEvent(e.Kind, e.Warnings));
 	}
 
-	public sealed class SessionObjects {
+	private sealed class RunningSessionDisposer : IDisposable {
 		private readonly InstanceRunningState state;
+		private readonly InstanceSession session;
 		private bool isDisposed;
 
-		public SessionObjects(InstanceRunningState state) {
+		public RunningSessionDisposer(InstanceRunningState state, InstanceSession session) {
 			this.state = state;
+			this.session = session;
 		}
 
 		public bool Dispose() {
@@ -172,10 +167,12 @@ sealed class InstanceRunningState : IInstanceState {
 				state.CancelDelayedStop();
 			}
 			
-			state.logSender.Stop();
-			state.session.Dispose();
-			state.context.Services.PortManager.Release(state.context.Configuration);
+			session.Dispose();
 			return true;
 		}
+
+		void IDisposable.Dispose() {
+			Dispose();
+		}
 	}
 }

Agent/Phantom.Agent.Services/Instances/States/InstanceStoppingState.cs

@@ -9,13 +9,13 @@ namespace Phantom.Agent.Services.Instances.States;
 
 sealed class InstanceStoppingState : IInstanceState, IDisposable {
 	private readonly InstanceContext context;
-	private readonly InstanceSession session;
-	private readonly InstanceRunningState.SessionObjects sessionObjects;
+	private readonly InstanceProcess process;
+	private readonly IDisposable sessionDisposer;
 
-	public InstanceStoppingState(InstanceContext context, InstanceSession session, InstanceRunningState.SessionObjects sessionObjects) {
-		this.sessionObjects = sessionObjects;
-		this.session = session;
+	public InstanceStoppingState(InstanceContext context, InstanceProcess process, IDisposable sessionDisposer) {
 		this.context = context;
+		this.process = process;
+		this.sessionDisposer = sessionDisposer;
 	}
 
 	public void Initialize() {
@@ -27,9 +27,9 @@ sealed class InstanceStoppingState : IInstanceState, IDisposable {
 	private async Task DoStop() {
 		try {
 			// Do not release the semaphore after this point.
-			if (!await session.BackupSemaphore.CancelAndWait(TimeSpan.FromSeconds(1))) {
+			if (!await process.BackupSemaphore.CancelAndWait(TimeSpan.FromSeconds(1))) {
 				context.Logger.Information("Waiting for backup to finish...");
-				await session.BackupSemaphore.CancelAndWait(Timeout.InfiniteTimeSpan);
+				await process.BackupSemaphore.CancelAndWait(Timeout.InfiniteTimeSpan);
 			}
 			
 			context.Logger.Information("Sending stop command...");
@@ -47,10 +47,10 @@ sealed class InstanceStoppingState : IInstanceState, IDisposable {
 	private async Task DoSendStopCommand() {
 		using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(5));
 		try {
-			await session.SendCommand(MinecraftCommand.Stop, timeout.Token);
+			await process.SendCommand(MinecraftCommand.Stop, timeout.Token);
 		} catch (OperationCanceledException) {
 			// ignore
-		} catch (ObjectDisposedException e) when (e.ObjectName == typeof(Process).FullName && session.HasEnded) {
+		} catch (ObjectDisposedException e) when (e.ObjectName == typeof(Process).FullName && process.HasEnded) {
 			// ignore
 		} catch (Exception e) {
 			context.Logger.Warning(e, "Caught exception while sending stop command.");
@@ -60,11 +60,11 @@ sealed class InstanceStoppingState : IInstanceState, IDisposable {
 	private async Task DoWaitForSessionToEnd() {
 		using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(55));
 		try {
-			await session.WaitForExit(timeout.Token);
+			await process.WaitForExit(timeout.Token);
 		} catch (OperationCanceledException) {
 			try {
 				context.Logger.Warning("Waiting timed out, killing session...");
-				session.Kill();
+				process.Kill();
 			} catch (Exception e) {
 				context.Logger.Error(e, "Caught exception while killing session.");
 			}
@@ -84,6 +84,6 @@ sealed class InstanceStoppingState : IInstanceState, IDisposable {
 	}
 
 	public void Dispose() {
-		sessionObjects.Dispose();
+		sessionDisposer.Dispose();
 	}
 }