Handle exceptions while writing a movie file, and use a temporary file until writing has completed. Addresses #4258 and prevents loss of data in the event of an exception mid-write.

Author: SuuperW

src/BizHawk.Client.Common/Api/Classes/MovieApi.cs

@@ -53,6 +53,7 @@ public string GetInputAsMnemonic(int frame)
 			return Bk2LogEntryGenerator.GenerateLogEntry(_movieSession.Movie.GetInputState(frame));
 		}
 
+		// TODO: Change return type to indicate success/failure? This method is part of BizHawk's API for external tools/Lua.
 		public void Save(string filename)
 		{
 			if (_movieSession.Movie.NotActive())
@@ -70,7 +71,9 @@ public void Save(string filename)
 				}
 				_movieSession.Movie.Filename = filename;
 			}
-			_movieSession.Movie.Save();
+			FileWriteResult result = _movieSession.Movie.Save();
+			// unitl API is decided
+			if (result.Exception != null) throw result.Exception;
 		}
 
 		public IReadOnlyDictionary<string, string> GetHeader()

src/BizHawk.Client.Common/FileStreamWithTemp.cs

@@ -0,0 +1,104 @@
+using System.Diagnostics;
+using System.IO;
+
+using BizHawk.Common.StringExtensions;
+
+namespace BizHawk.Client.Common
+{
+	internal class FileStreamWithTemp : IDisposable
+	{
+		public readonly FileStream Stream;
+		public string FinalPath;
+		public string TempPath;
+
+		public bool UsingTempFile => TempPath != FinalPath;
+
+		private bool _finished = false;
+
+		private FileStreamWithTemp(string final, string temp)
+		{
+			FinalPath = final;
+			TempPath = temp;
+			Stream = new(temp, FileMode.Create, FileAccess.Write);
+		}
+
+		// There is no public constructor. This is the only way to create an instance.
+		public static FileWriteResult<FileStreamWithTemp> Create(string path)
+		{
+			string writePath = path;
+			// If the file already exists, we will write to a temporary location first and preserve the old one until we're done.
+			if (File.Exists(path))
+			{
+				writePath = path.InsertBeforeLast('.', ".saving", out bool inserted);
+				if (!inserted) writePath = $"{path}.saving";
+
+				if (File.Exists(writePath))
+				{
+					// The user should probably have dealt with this on the previously failed save.
+					// But maybe we should support plain old "try again", so let's delete it.
+					try
+					{
+						File.Delete(writePath);
+					}
+					catch (IOException ex)
+					{
+						return new(FileWriteEnum.FailedToOpen, writePath, ex);
+					}
+				}
+			}
+			try
+			{
+				return new(new FileStreamWithTemp(path, writePath), writePath);
+			}
+			catch (IOException ex)
+			{
+				return new(FileWriteEnum.FailedToOpen, writePath, ex);
+			}
+		}
+
+		/// <summary>
+		/// This method must be called after writing has finished. An exception will be thrown if it isn't.
+		/// </summary>
+		public FileWriteResult EndWrite()
+		{
+			if (_finished) throw new InvalidOperationException("Cannot finalize twice.");
+
+			_finished = true;
+			Dispose();
+
+			if (!UsingTempFile) return new(FileWriteEnum.Success, FinalPath, null);
+
+			_finished = false; // In case we fail, allow the caller to try again.
+			if (File.Exists(FinalPath))
+			{
+				try
+				{
+					File.Delete(FinalPath);
+				}
+				catch (IOException ex)
+				{
+					return new(FileWriteEnum.FailedToDeleteOldFile, TempPath, ex);
+				}
+			}
+			try
+			{
+				File.Move(TempPath, FinalPath);
+			}
+			catch (IOException ex)
+			{
+				return new(FileWriteEnum.FailedToRename, TempPath, ex);
+			}
+
+			_finished = true;
+			return new(FileWriteEnum.Success, FinalPath, null);
+		}
+
+		public void Dispose()
+		{
+			Stream.Dispose();
+
+			// The caller should call EndWrite and handle potential failure.
+			Debug.Assert(!_finished, $"{nameof(FileWriteResult)} should not be disposed before calling {nameof(EndWrite)}");
+		}
+	}
+}

src/BizHawk.Client.Common/FileWriteResult.cs

@@ -0,0 +1,89 @@
+#nullable enable
+
+using System.Diagnostics;
+
+namespace BizHawk.Client.Common
+{
+	public enum FileWriteEnum
+	{
+		Success,
+		FailedToOpen,
+		FailedDuringWrite,
+		FailedToDeleteOldFile,
+		FailedToRename,
+	}
+
+	/// <summary>
+	/// Provides information about the success or failure of an attempt to write to a file.
+	/// </summary>
+	public class FileWriteResult
+	{
+		public readonly FileWriteEnum Error = FileWriteEnum.Success;
+		public readonly Exception? Exception;
+		public readonly string WritePath;
+
+		public bool IsError => Error != FileWriteEnum.Success;
+
+		public FileWriteResult(FileWriteEnum error, string path, Exception? exception)
+		{
+			Error = error;
+			Exception = exception;
+			WritePath = path;
+		}
+
+		/// <summary>
+		/// Converts this instance to a different generic type.
+		/// The new instance will take the value given only if this instance has no error.
+		/// </summary>
+		/// <param name="value">The value of the new instance. Ignored if this instance has an error.</param>
+		public FileWriteResult<T> Convert<T>(T value)
+		{
+			if (Error == FileWriteEnum.Success) return new(value, WritePath);
+			else return new(this);
+		}
+
+		public FileWriteResult(FileWriteResult other) : this(other.Error, other.WritePath, other.Exception) { }
+
+		public string UserFriendlyErrorMessage()
+		{
+			switch (Error)
+			{
+				case FileWriteEnum.Success:
+					return $"The file {WritePath} was written successfully.";
+				case FileWriteEnum.FailedToOpen:
+					return $"The file {WritePath} could not be created.";
+				case FileWriteEnum.FailedDuringWrite:
+					return $"The file {WritePath} was created, but an error occurred while writing to it.";
+				case FileWriteEnum.FailedToDeleteOldFile:
+					return $"The file {WritePath} was created successfully, but the old file could not be deleted.";
+				case FileWriteEnum.FailedToRename:
+					return $"The file {WritePath} was created successfully, but could not be renamed.";
+				default:
+					return "unreachable";
+			}
+		}
+	}
+
+	/// <summary>
+	/// Provides information about the success or failure of an attempt to write to a file.
+	/// If successful, also provides a related object instance.
+	/// </summary>
+	public class FileWriteResult<T> : FileWriteResult
+	{
+		/// <summary>
+		/// Value will not be null if <see cref="FileWriteResult.Error"/> is <see cref="FileWriteEnum.Success"/>.
+		/// Otherwise, Value will be null.
+		/// </summary>
+		public readonly T? Value = default;
+
+		public FileWriteResult(FileWriteEnum error, string path, Exception? exception) : base(error, path, exception) { }
+
+		public FileWriteResult(T value, string path) : base(FileWriteEnum.Success, path, null)
+		{
+			Debug.Assert(value != null, "Should not give a null value on success. Use the non-generic type if there is no value.");
+			Value = value;
+		}
+
+		public FileWriteResult(FileWriteResult other) : base(other.Error, other.WritePath, other.Exception) { }
+	}
+}

src/BizHawk.Client.Common/FrameworkZipWriter.cs

@@ -1,3 +1,5 @@
+#nullable enable
+
 using System.IO;
 using System.IO.Compression;
 
@@ -7,18 +9,18 @@ namespace BizHawk.Client.Common
 {
 	public class FrameworkZipWriter : IZipWriter
 	{
-		private ZipArchive _archive;
+		private ZipArchive? _archive;
 
-		private FileStream _fs;
+		private FileStreamWithTemp? _fs;
 
-		private Zstd _zstd;
+		private Zstd? _zstd;
 		private readonly CompressionLevel _level;
 		private readonly int _zstdCompressionLevel;
 
-		public FrameworkZipWriter(string path, int compressionLevel)
+		private bool _disposed;
+
+		private FrameworkZipWriter(int compressionLevel)
 		{
-			_fs = new(path, FileMode.Create, FileAccess.Write);
-			_archive = new(_fs, ZipArchiveMode.Create, leaveOpen: true);
 			if (compressionLevel == 0)
 				_level = CompressionLevel.NoCompression;
 			else if (compressionLevel < 5)
@@ -32,8 +34,28 @@ public FrameworkZipWriter(string path, int compressionLevel)
 			_zstdCompressionLevel = compressionLevel * 2 + 1;
 		}
 
+		public static FileWriteResult<FrameworkZipWriter> Create(string path, int compressionLevel)
+		{
+			FileWriteResult<FileStreamWithTemp> fs = FileStreamWithTemp.Create(path);
+			if (fs.IsError) return new(fs);
+
+			FrameworkZipWriter ret = new(compressionLevel);
+			ret._fs = fs.Value!;
+			ret._archive = new(ret._fs.Stream, ZipArchiveMode.Create, leaveOpen: true);
+
+			return fs.Convert(ret);
+		}
+
+		public FileWriteResult EndWrite()
+		{
+			if (_fs == null) throw new ObjectDisposedException("Cannot use disposed ZipWriter.");
+			return _fs.EndWrite();
+		}
+
 		public void WriteItem(string name, Action<Stream> callback, bool zstdCompress)
 		{
+			if (_archive == null || _zstd == null) throw new ObjectDisposedException("Cannot use disposed ZipWriter.");
+
 			// don't compress with deflate if we're already compressing with zstd
 			// this won't produce meaningful compression, and would just be a timesink
 			using var stream = _archive.CreateEntry(name, zstdCompress ? CompressionLevel.NoCompression : _level).Open();
@@ -51,19 +73,17 @@ public void WriteItem(string name, Action<Stream> callback, bool zstdCompress)
 
 		public void Dispose()
 		{
-			if (_archive != null)
-			{
-				_archive.Dispose();
-				_archive = null;
-			}
-			_fs?.Flush(flushToDisk: true);
-			_fs?.Dispose();
+			if (_disposed) return;
+			_disposed = true;
+
+			_archive!.Dispose();
+			_archive = null;
+			_zstd!.Dispose();
+			_zstd = null;
+
+			_fs!.Stream.Flush(flushToDisk: true);
+			_fs.Dispose();
 			_fs = null;
-			if (_zstd != null)
-			{
-				_zstd.Dispose();
-				_zstd = null;
-			}
 		}
 	}
 }

src/BizHawk.Client.Common/IZipWriter.cs

@@ -5,5 +5,10 @@ namespace BizHawk.Client.Common
 	public interface IZipWriter : IDisposable
 	{
 		void WriteItem(string name, Action<Stream> callback, bool zstdCompress);
+
+		/// <summary>
+		/// This method must be called after writing has finished.
+		/// </summary>
+		FileWriteResult EndWrite();
 	}
 }

src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs

@@ -71,7 +71,7 @@ public static IMovie ToBk2(this IMovie old)
 			return bk2;
 		}
 
-		public static ITasMovie ConvertToSavestateAnchoredMovie(this ITasMovie old, int frame, byte[] savestate)
+		public static FileWriteResult<ITasMovie> ConvertToSavestateAnchoredMovie(this ITasMovie old, int frame, byte[] savestate)
 		{
 			string newFilename = ConvertFileNameToTasMovie(old.Filename);
 
@@ -115,11 +115,11 @@ public static ITasMovie ConvertToSavestateAnchoredMovie(this ITasMovie old, int
 				}
 			}
 
-			tas.Save();
-			return tas;
+			FileWriteResult saveResult = tas.Save();
+			return saveResult.Convert(tas);
 		}
 
-		public static ITasMovie ConvertToSaveRamAnchoredMovie(this ITasMovie old, byte[] saveRam)
+		public static FileWriteResult<ITasMovie> ConvertToSaveRamAnchoredMovie(this ITasMovie old, byte[] saveRam)
 		{
 			string newFilename = ConvertFileNameToTasMovie(old.Filename);
 
@@ -146,8 +146,8 @@ public static ITasMovie ConvertToSaveRamAnchoredMovie(this ITasMovie old, byte[]
 				tas.Subtitles.Add(sub);
 			}
 
-			tas.Save();
-			return tas;
+			FileWriteResult saveResult = tas.Save();
+			return saveResult.Convert(tas);
 		}
 
 #pragma warning disable RCS1224 // private but for unit test

src/BizHawk.Client.Common/movie/MovieSession.cs

@@ -244,7 +244,7 @@ public void RunQueuedMovie(bool recordMode, IEmulator emulator)
 		public void AbortQueuedMovie()
 			=> _queuedMovie = null;
 
-		public void StopMovie(bool saveChanges = true)
+		public FileWriteResult StopMovie(bool saveChanges = true)
 		{
 			if (Movie.IsActive())
 			{
@@ -262,8 +262,17 @@ public void StopMovie(bool saveChanges = true)
 
 				if (saveChanges && Movie.Changes)
 				{
-					Movie.Save();
-					Output($"{Path.GetFileName(Movie.Filename)} written to disk.");
+					FileWriteResult result = Movie.Save();
+					if (result.IsError)
+					{
+						Output($"Failed to write {Path.GetFileName(Movie.Filename)} to disk.");
+						Output(result.UserFriendlyErrorMessage());
+						return result;
+					}
+					else
+					{
+						Output($"{Path.GetFileName(Movie.Filename)} written to disk.");
+					}
 				}
 				Movie.Stop();
 
@@ -279,6 +288,8 @@ public void StopMovie(bool saveChanges = true)
 			}
 
 			Movie = null;
+
+			return new(FileWriteEnum.Success, "", null);
 		}
 
 		public IMovie Get(string path, bool loadMovie)