From 4f220d9532ff86164d59e249364f85322e3e3c73 Mon Sep 17 00:00:00 2001 From: Taloth Saldono Date: Thu, 11 Jun 2020 23:54:28 +0200 Subject: [PATCH] Fixed: Removed hardlink-based transactional file transfer logic (instead relying on explicit copy+delete for cifs) --- .../DiskTests/DiskTransferServiceFixture.cs | 416 +++--------------- .../Disk/DiskTransferService.cs | 267 ++--------- .../MusicTests/MoveArtistServiceFixture.cs | 11 +- .../DeleteDirectoryFixture.cs | 2 +- .../DeleteFileFixture.cs | 8 +- .../UpdateTests/UpdateServiceFixture.cs | 2 +- .../Extras/Files/ExtraFileManager.cs | 2 +- .../Update/InstallUpdateService.cs | 2 +- 8 files changed, 93 insertions(+), 617 deletions(-) diff --git a/src/NzbDrone.Common.Test/DiskTests/DiskTransferServiceFixture.cs b/src/NzbDrone.Common.Test/DiskTests/DiskTransferServiceFixture.cs index d9cc1ae20..7995a1dbb 100644 --- a/src/NzbDrone.Common.Test/DiskTests/DiskTransferServiceFixture.cs +++ b/src/NzbDrone.Common.Test/DiskTests/DiskTransferServiceFixture.cs @@ -17,8 +17,6 @@ namespace NzbDrone.Common.Test.DiskTests { private readonly string _sourcePath = @"C:\source\my.video.mkv".AsOsAgnostic(); private readonly string _targetPath = @"C:\target\my.video.mkv".AsOsAgnostic(); - private readonly string _backupPath = @"C:\source\my.video.mkv.backup~".AsOsAgnostic(); - private readonly string _tempTargetPath = @"C:\target\my.video.mkv.partial~".AsOsAgnostic(); private readonly string _nfsFile = ".nfs01231232"; private MockMount _sourceMount; @@ -46,65 +44,17 @@ namespace NzbDrone.Common.Test.DiskTests Mocker.GetMock() .Setup(v => v.GetMount(It.Is(p => p.StartsWith(_sourceMount.RootDirectory)))) - .Returns(_sourceMount); + .Returns(s => _sourceMount); Mocker.GetMock() .Setup(v => v.GetMount(It.Is(p => p.StartsWith(_targetMount.RootDirectory)))) - .Returns(_targetMount); + .Returns(s => _targetMount); WithEmulatedDiskProvider(); WithExistingFile(_sourcePath); } - [Test] - public void should_use_verified_transfer_on_mono() - { - PosixOnly(); - - Subject.VerificationMode.Should().Be(DiskTransferVerificationMode.TryTransactional); - } - - [Test] - public void should_not_use_verified_transfer_on_windows() - { - WindowsOnly(); - - Subject.VerificationMode.Should().Be(DiskTransferVerificationMode.VerifyOnly); - - var result = Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); - - Mocker.GetMock() - .Verify(v => v.TryCreateHardLink(_sourcePath, _backupPath), Times.Never()); - - Mocker.GetMock() - .Verify(v => v.MoveFile(_sourcePath, _targetPath, false), Times.Once()); - } - - [TestCase("fuse.mergerfs", "")] - [TestCase("fuse.rclone", "")] - [TestCase("mergerfs", "")] - [TestCase("rclone", "")] - [TestCase("", "fuse.mergerfs")] - [TestCase("", "fuse.rclone")] - [TestCase("", "mergerfs")] - [TestCase("", "rclone")] - public void should_not_use_verified_transfer_on_specific_filesystems(string fsSource, string fsTarget) - { - MonoOnly(); - - _sourceMount.DriveFormat = fsSource; - _targetMount.DriveFormat = fsTarget; - - var result = Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); - - Mocker.GetMock() - .Verify(v => v.TryCreateHardLink(_sourcePath, _backupPath), Times.Never()); - - Mocker.GetMock() - .Verify(v => v.MoveFile(_sourcePath, _targetPath, false), Times.Once()); - } - [Test] public void should_throw_if_path_is_the_same() { @@ -125,72 +75,75 @@ namespace NzbDrone.Common.Test.DiskTests [Test] public void should_rename_via_temp_if_different_casing() { + var backupPath = _sourcePath + ".backup~"; var targetPath = Path.Combine(Path.GetDirectoryName(_sourcePath), Path.GetFileName(_sourcePath).ToUpper()); Mocker.GetMock() - .Setup(v => v.MoveFile(_sourcePath, _backupPath, true)) + .Setup(v => v.MoveFile(_sourcePath, backupPath, true)) .Callback(() => { - WithExistingFile(_backupPath, true); + WithExistingFile(backupPath, true); WithExistingFile(_sourcePath, false); }); Mocker.GetMock() - .Setup(v => v.MoveFile(_backupPath, targetPath, false)) + .Setup(v => v.MoveFile(backupPath, targetPath, false)) .Callback(() => { WithExistingFile(targetPath, true); - WithExistingFile(_backupPath, false); + WithExistingFile(backupPath, false); }); var result = Subject.TransferFile(_sourcePath, targetPath, TransferMode.Move); Mocker.GetMock() - .Verify(v => v.MoveFile(_backupPath, targetPath, false), Times.Once()); + .Verify(v => v.MoveFile(backupPath, targetPath, false), Times.Once()); } [Test] public void should_rollback_rename_via_temp_on_exception() { + var backupPath = _sourcePath + ".backup~"; var targetPath = Path.Combine(Path.GetDirectoryName(_sourcePath), Path.GetFileName(_sourcePath).ToUpper()); Mocker.GetMock() - .Setup(v => v.MoveFile(_sourcePath, _backupPath, true)) + .Setup(v => v.MoveFile(_sourcePath, backupPath, true)) .Callback(() => { - WithExistingFile(_backupPath, true); + WithExistingFile(backupPath, true); WithExistingFile(_sourcePath, false); }); Mocker.GetMock() - .Setup(v => v.MoveFile(_backupPath, targetPath, false)) + .Setup(v => v.MoveFile(backupPath, targetPath, false)) .Throws(new IOException("Access Violation")); Assert.Throws(() => Subject.TransferFile(_sourcePath, targetPath, TransferMode.Move)); Mocker.GetMock() - .Verify(v => v.MoveFile(_backupPath, _sourcePath, false), Times.Once()); + .Verify(v => v.MoveFile(backupPath, _sourcePath, false), Times.Once()); } [Test] public void should_log_error_if_rollback_move_fails() { + var backupPath = _sourcePath + ".backup~"; var targetPath = Path.Combine(Path.GetDirectoryName(_sourcePath), Path.GetFileName(_sourcePath).ToUpper()); Mocker.GetMock() - .Setup(v => v.MoveFile(_sourcePath, _backupPath, true)) + .Setup(v => v.MoveFile(_sourcePath, backupPath, true)) .Callback(() => { - WithExistingFile(_backupPath, true); + WithExistingFile(backupPath, true); WithExistingFile(_sourcePath, false); }); Mocker.GetMock() - .Setup(v => v.MoveFile(_backupPath, targetPath, false)) + .Setup(v => v.MoveFile(backupPath, targetPath, false)) .Throws(new IOException("Access Violation")); Mocker.GetMock() - .Setup(v => v.MoveFile(_backupPath, _sourcePath, false)) + .Setup(v => v.MoveFile(backupPath, _sourcePath, false)) .Throws(new IOException("Access Violation")); Assert.Throws(() => Subject.TransferFile(_sourcePath, targetPath, TransferMode.Move)); @@ -225,28 +178,44 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void should_fallback_to_copy_if_hardlink_failed() + public void should_use_copy_delete_on_cifs() { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithFailedHardlink(); + _sourceMount.DriveFormat = "ext4"; + _targetMount.DriveFormat = "cifs"; var result = Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); Mocker.GetMock() - .Verify(v => v.CopyFile(_sourcePath, _tempTargetPath, false), Times.Once()); + .Verify(v => v.MoveFile(_sourcePath, _targetPath, false), Times.Never()); Mocker.GetMock() - .Verify(v => v.MoveFile(_tempTargetPath, _targetPath, false), Times.Once()); + .Verify(v => v.CopyFile(_sourcePath, _targetPath, false), Times.Once()); - VerifyDeletedFile(_sourcePath); + Mocker.GetMock() + .Verify(v => v.DeleteFile(_sourcePath), Times.Once()); } [Test] - public void mode_none_should_not_verify_copy() + public void should_use_move_on_cifs_if_same_mount() { - Subject.VerificationMode = DiskTransferVerificationMode.None; + _sourceMount.DriveFormat = "cifs"; + _targetMount = _sourceMount; + + var result = Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); + + Mocker.GetMock() + .Verify(v => v.MoveFile(_sourcePath, _targetPath, false), Times.Once()); + Mocker.GetMock() + .Verify(v => v.CopyFile(_sourcePath, _targetPath, false), Times.Never()); + + Mocker.GetMock() + .Verify(v => v.DeleteFile(_sourcePath), Times.Never()); + } + + [Test] + public void should_not_verify_copy() + { Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Copy); Mocker.GetMock() @@ -254,10 +223,8 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_none_should_not_verify_move() + public void should_not_verify_move() { - Subject.VerificationMode = DiskTransferVerificationMode.None; - Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); Mocker.GetMock() @@ -265,10 +232,8 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_none_should_delete_existing_target_when_overwriting() + public void should_delete_existing_target_when_overwriting() { - Subject.VerificationMode = DiskTransferVerificationMode.None; - WithExistingFile(_targetPath); Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move, true); @@ -281,10 +246,8 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_none_should_throw_if_existing_target_when_not_overwriting() + public void should_throw_if_existing_target_when_not_overwriting() { - Subject.VerificationMode = DiskTransferVerificationMode.None; - WithExistingFile(_targetPath); Assert.Throws(() => Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move, false)); @@ -297,10 +260,8 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_verifyonly_should_verify_copy() + public void should_verify_copy() { - Subject.VerificationMode = DiskTransferVerificationMode.VerifyOnly; - Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Copy); Mocker.GetMock() @@ -311,10 +272,8 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_verifyonly_should_rollback_copy_on_partial_and_throw() + public void should_rollback_copy_on_partial_and_throw() { - Subject.VerificationMode = DiskTransferVerificationMode.VerifyOnly; - Mocker.GetMock() .Setup(v => v.CopyFile(_sourcePath, _targetPath, false)) .Callback(() => @@ -331,8 +290,6 @@ namespace NzbDrone.Common.Test.DiskTests [Test] public void should_log_error_if_rollback_copy_fails() { - Subject.VerificationMode = DiskTransferVerificationMode.VerifyOnly; - Mocker.GetMock() .Setup(v => v.CopyFile(_sourcePath, _targetPath, false)) .Callback(() => @@ -350,10 +307,8 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_verifyonly_should_verify_move() + public void should_verify_move() { - Subject.VerificationMode = DiskTransferVerificationMode.VerifyOnly; - Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); Mocker.GetMock() @@ -364,10 +319,8 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_verifyonly_should_not_rollback_move_on_partial_and_throw() + public void should_not_rollback_move_on_partial_and_throw_if_source_gone() { - Subject.VerificationMode = DiskTransferVerificationMode.VerifyOnly; - Mocker.GetMock() .Setup(v => v.MoveFile(_sourcePath, _targetPath, false)) .Callback(() => @@ -385,10 +338,8 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_verifyonly_should_rollback_move_on_partial_if_source_remains() + public void should_rollback_move_on_partial_if_source_remains() { - Subject.VerificationMode = DiskTransferVerificationMode.VerifyOnly; - Mocker.GetMock() .Setup(v => v.MoveFile(_sourcePath, _targetPath, false)) .Callback(() => @@ -405,8 +356,6 @@ namespace NzbDrone.Common.Test.DiskTests [Test] public void should_log_error_if_rollback_partialmove_fails() { - Subject.VerificationMode = DiskTransferVerificationMode.VerifyOnly; - Mocker.GetMock() .Setup(v => v.MoveFile(_sourcePath, _targetPath, false)) .Callback(() => @@ -424,251 +373,7 @@ namespace NzbDrone.Common.Test.DiskTests } [Test] - public void mode_transactional_should_move_and_verify_if_same_folder() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - var targetPath = _sourcePath + ".test"; - - var result = Subject.TransferFile(_sourcePath, targetPath, TransferMode.Move); - - Mocker.GetMock() - .Verify(v => v.TryCreateHardLink(_sourcePath, _backupPath), Times.Never()); - - Mocker.GetMock() - .Verify(v => v.CopyFile(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never()); - - Mocker.GetMock() - .Verify(v => v.MoveFile(_sourcePath, targetPath, false), Times.Once()); - } - - [Test] - public void mode_trytransactional_should_revert_to_verifyonly_if_hardlink_fails() - { - Subject.VerificationMode = DiskTransferVerificationMode.TryTransactional; - - WithFailedHardlink(); - - Mocker.GetMock() - .Setup(v => v.MoveFile(_sourcePath, _targetPath, false)) - .Callback(() => - { - WithExistingFile(_sourcePath, false); - WithExistingFile(_targetPath, true); - }); - - var result = Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); - - Mocker.GetMock() - .Verify(v => v.TryCreateHardLink(_sourcePath, _backupPath), Times.Once()); - - Mocker.GetMock() - .Verify(v => v.CopyFile(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never()); - - Mocker.GetMock() - .Verify(v => v.MoveFile(_sourcePath, _targetPath, false), Times.Once()); - } - - [Test] - public void mode_transactional_should_delete_old_backup_on_move() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithExistingFile(_backupPath); - - WithSuccessfulHardlink(_sourcePath, _backupPath); - - Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); - - Mocker.GetMock() - .Verify(v => v.DeleteFile(_backupPath), Times.Once()); - } - - [Test] - public void mode_transactional_should_delete_old_partial_on_move() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithExistingFile(_tempTargetPath); - - WithSuccessfulHardlink(_sourcePath, _backupPath); - - Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); - - Mocker.GetMock() - .Verify(v => v.DeleteFile(_tempTargetPath), Times.Once()); - } - - [Test] - public void mode_transactional_should_delete_old_partial_on_copy() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithExistingFile(_tempTargetPath); - - Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Copy); - - Mocker.GetMock() - .Verify(v => v.DeleteFile(_tempTargetPath), Times.Once()); - } - - [Test] - public void mode_transactional_should_hardlink_before_move() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithSuccessfulHardlink(_sourcePath, _backupPath); - - var result = Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); - - Mocker.GetMock() - .Verify(v => v.TryCreateHardLink(_sourcePath, _backupPath), Times.Once()); - } - - [Test] - public void mode_transactional_should_retry_if_partial_copy() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - var retry = 0; - Mocker.GetMock() - .Setup(v => v.CopyFile(_sourcePath, _tempTargetPath, false)) - .Callback(() => - { - WithExistingFile(_tempTargetPath, true, 900); - if (retry++ == 1) - { - WithExistingFile(_tempTargetPath, true, 1000); - } - }); - - var result = Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Copy); - - ExceptionVerification.ExpectedWarns(1); - } - - [Test] - public void mode_transactional_should_retry_twice_if_partial_copy() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - var retry = 0; - Mocker.GetMock() - .Setup(v => v.CopyFile(_sourcePath, _tempTargetPath, false)) - .Callback(() => - { - WithExistingFile(_tempTargetPath, true, 900); - if (retry++ == 3) - { - throw new Exception("Test Failed, retried too many times."); - } - }); - - Assert.Throws(() => Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Copy)); - - ExceptionVerification.ExpectedWarns(2); - ExceptionVerification.ExpectedErrors(1); - } - - [Test] - public void mode_transactional_should_remove_source_after_move() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithSuccessfulHardlink(_sourcePath, _backupPath); - - Mocker.GetMock() - .Setup(v => v.MoveFile(_backupPath, _tempTargetPath, false)) - .Callback(() => WithExistingFile(_tempTargetPath, true)); - - var result = Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); - - VerifyDeletedFile(_sourcePath); - } - - [Test] - public void mode_transactional_should_not_remove_source_if_partial_still_exists() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - var targetPath = Path.Combine(Path.GetDirectoryName(_targetPath), Path.GetFileName(_targetPath).ToUpper()); - var tempTargetPath = targetPath + ".partial~"; - - WithSuccessfulHardlink(_sourcePath, _backupPath); - - WithExistingFile(_targetPath); - - Mocker.GetMock() - .Setup(v => v.MoveFile(_backupPath, tempTargetPath, false)) - .Callback(() => WithExistingFile(tempTargetPath, true)); - - Mocker.GetMock() - .Setup(v => v.MoveFile(tempTargetPath, targetPath, false)) - .Callback(() => { }); - - Assert.Throws(() => Subject.TransferFile(_sourcePath, targetPath, TransferMode.Move)); - - Mocker.GetMock() - .Verify(v => v.DeleteFile(_sourcePath), Times.Never()); - } - - [Test] - public void mode_transactional_should_remove_partial_if_copy_fails() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithSuccessfulHardlink(_sourcePath, _backupPath); - - Mocker.GetMock() - .Setup(v => v.CopyFile(_sourcePath, _tempTargetPath, false)) - .Callback(() => - { - WithExistingFile(_tempTargetPath, true, 900); - }) - .Throws(new IOException("Blackbox IO error")); - - Assert.Throws(() => Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Copy)); - - VerifyDeletedFile(_tempTargetPath); - } - - [Test] - public void mode_transactional_should_remove_backup_if_move_throws() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithSuccessfulHardlink(_sourcePath, _backupPath); - - Mocker.GetMock() - .Setup(v => v.MoveFile(_backupPath, _tempTargetPath, false)) - .Throws(new IOException("Blackbox IO error")); - - Assert.Throws(() => Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move)); - - VerifyDeletedFile(_backupPath); - } - - [Test] - public void mode_transactional_should_remove_partial_if_move_fails() - { - Subject.VerificationMode = DiskTransferVerificationMode.Transactional; - - WithSuccessfulHardlink(_sourcePath, _backupPath); - - Mocker.GetMock() - .Setup(v => v.MoveFile(_backupPath, _tempTargetPath, false)) - .Callback(() => - { - WithExistingFile(_backupPath, false); - WithExistingFile(_tempTargetPath, true, 900); - }); - - Subject.TransferFile(_sourcePath, _targetPath, TransferMode.Move); - - VerifyDeletedFile(_tempTargetPath); - } - - [Test] + [Retry(5)] public void CopyFolder_should_copy_folder() { WithRealDiskProvider(); @@ -884,23 +589,6 @@ namespace NzbDrone.Common.Test.DiskTests .Verify(v => v.MoveFolder(src, dst), Times.Never()); } - [Test] - public void TransferFolder_should_not_use_movefolder_if_on_same_mount_but_transactional() - { - WithEmulatedDiskProvider(); - - var src = @"C:\Base1\TestDir1".AsOsAgnostic(); - var dst = @"C:\Base1\TestDir2".AsOsAgnostic(); - - WithMockMount(@"C:\Base1".AsOsAgnostic()); - WithExistingFile(@"C:\Base1\TestDir1\test.file.txt".AsOsAgnostic()); - - Subject.TransferFolder(src, dst, TransferMode.Move, DiskTransferVerificationMode.Transactional); - - Mocker.GetMock() - .Verify(v => v.MoveFolder(src, dst), Times.Never()); - } - [Test] public void TransferFolder_should_not_use_movefolder_if_on_different_mount() { diff --git a/src/NzbDrone.Common/Disk/DiskTransferService.cs b/src/NzbDrone.Common/Disk/DiskTransferService.cs index 0db138f63..01281da4d 100644 --- a/src/NzbDrone.Common/Disk/DiskTransferService.cs +++ b/src/NzbDrone.Common/Disk/DiskTransferService.cs @@ -12,64 +12,38 @@ namespace NzbDrone.Common.Disk { public interface IDiskTransferService { - TransferMode TransferFolder(string sourcePath, string targetPath, TransferMode mode, bool verified = true); - TransferMode TransferFile(string sourcePath, string targetPath, TransferMode mode, bool overwrite = false, bool verified = true); + TransferMode TransferFolder(string sourcePath, string targetPath, TransferMode mode); + TransferMode TransferFile(string sourcePath, string targetPath, TransferMode mode, bool overwrite = false); int MirrorFolder(string sourcePath, string targetPath); } - public enum DiskTransferVerificationMode - { - None, - VerifyOnly, - TryTransactional, - Transactional - } - public class DiskTransferService : IDiskTransferService { - private const int RetryCount = 2; - private readonly IDiskProvider _diskProvider; private readonly Logger _logger; - public DiskTransferVerificationMode VerificationMode { get; set; } - public DiskTransferService(IDiskProvider diskProvider, Logger logger) { _diskProvider = diskProvider; _logger = logger; - - // TODO: Atm we haven't seen partial transfers on windows so we disable verified transfer. - // (If enabled in the future, be sure to check specifically for ReFS, which doesn't support hardlinks.) - VerificationMode = OsInfo.IsWindows ? DiskTransferVerificationMode.VerifyOnly : DiskTransferVerificationMode.TryTransactional; } - public TransferMode TransferFolder(string sourcePath, string targetPath, TransferMode mode, bool verified = true) - { - var verificationMode = verified ? VerificationMode : DiskTransferVerificationMode.VerifyOnly; - - return TransferFolder(sourcePath, targetPath, mode, verificationMode); - } - - public TransferMode TransferFolder(string sourcePath, string targetPath, TransferMode mode, DiskTransferVerificationMode verificationMode) + public TransferMode TransferFolder(string sourcePath, string targetPath, TransferMode mode) { Ensure.That(sourcePath, () => sourcePath).IsValidPath(); Ensure.That(targetPath, () => targetPath).IsValidPath(); if (mode == TransferMode.Move && !_diskProvider.FolderExists(targetPath)) { - if (verificationMode == DiskTransferVerificationMode.TryTransactional || verificationMode == DiskTransferVerificationMode.VerifyOnly) - { - var sourceMount = _diskProvider.GetMount(sourcePath); - var targetMount = _diskProvider.GetMount(targetPath); + var sourceMount = _diskProvider.GetMount(sourcePath); + var targetMount = _diskProvider.GetMount(targetPath); - // If we're on the same mount, do a simple folder move. - if (sourceMount != null && targetMount != null && sourceMount.RootDirectory == targetMount.RootDirectory) - { - _logger.Debug("Move Directory [{0}] > [{1}]", sourcePath, targetPath); - _diskProvider.MoveFolder(sourcePath, targetPath); - return mode; - } + // If we're on the same mount, do a simple folder move. + if (sourceMount != null && targetMount != null && sourceMount.RootDirectory == targetMount.RootDirectory) + { + _logger.Debug("Move Directory [{0}] > [{1}]", sourcePath, targetPath); + _diskProvider.MoveFolder(sourcePath, targetPath); + return mode; } } @@ -89,7 +63,7 @@ namespace NzbDrone.Common.Disk continue; } - result &= TransferFolder(subDir.FullName, Path.Combine(targetPath, subDir.Name), mode, verificationMode); + result &= TransferFolder(subDir.FullName, Path.Combine(targetPath, subDir.Name), mode); } foreach (var sourceFile in _diskProvider.GetFileInfos(sourcePath)) @@ -101,7 +75,7 @@ namespace NzbDrone.Common.Disk var destFile = Path.Combine(targetPath, sourceFile.Name); - result &= TransferFile(sourceFile.FullName, destFile, mode, true, verificationMode); + result &= TransferFile(sourceFile.FullName, destFile, mode, true); } if (mode.HasFlag(TransferMode.Move)) @@ -176,7 +150,7 @@ namespace NzbDrone.Common.Disk continue; } - TransferFile(sourceFile.FullName, targetFile, TransferMode.Copy, true, true); + TransferFile(sourceFile.FullName, targetFile, TransferMode.Copy, true); filesCopied++; } @@ -226,14 +200,7 @@ namespace NzbDrone.Common.Disk } } - public TransferMode TransferFile(string sourcePath, string targetPath, TransferMode mode, bool overwrite = false, bool verified = true) - { - var verificationMode = verified ? VerificationMode : DiskTransferVerificationMode.None; - - return TransferFile(sourcePath, targetPath, mode, overwrite, verificationMode); - } - - public TransferMode TransferFile(string sourcePath, string targetPath, TransferMode mode, bool overwrite, DiskTransferVerificationMode verificationMode) + public TransferMode TransferFile(string sourcePath, string targetPath, TransferMode mode, bool overwrite = false) { Ensure.That(sourcePath, () => sourcePath).IsValidPath(); Ensure.That(targetPath, () => targetPath).IsValidPath(); @@ -308,78 +275,33 @@ namespace NzbDrone.Common.Disk } // Adjust the transfer mode depending on the filesystems - if (verificationMode == DiskTransferVerificationMode.TryTransactional) - { - var sourceMount = _diskProvider.GetMount(sourcePath); - var targetMount = _diskProvider.GetMount(targetPath); + var sourceMount = _diskProvider.GetMount(sourcePath); + var targetMount = _diskProvider.GetMount(targetPath); - var isSameMount = sourceMount != null && targetMount != null && sourceMount.RootDirectory == targetMount.RootDirectory; + var isSameMount = sourceMount != null && targetMount != null && sourceMount.RootDirectory == targetMount.RootDirectory; - var sourceDriveFormat = sourceMount?.DriveFormat ?? string.Empty; - var targetDriveFormat = targetMount?.DriveFormat ?? string.Empty; + var sourceDriveFormat = sourceMount?.DriveFormat ?? string.Empty; + var targetDriveFormat = targetMount?.DriveFormat ?? string.Empty; - if (isSameMount) - { - // No transaction needed for operations on same mount, force VerifyOnly - verificationMode = DiskTransferVerificationMode.VerifyOnly; - } - else if (sourceDriveFormat.Contains("mergerfs") || sourceDriveFormat.Contains("rclone") || - targetDriveFormat.Contains("mergerfs") || targetDriveFormat.Contains("rclone")) - { - // Cloud storage filesystems don't need any Transactional stuff and it hurts performance, force VerifyOnly - verificationMode = DiskTransferVerificationMode.VerifyOnly; - } - else if ((sourceDriveFormat == "cifs" || targetDriveFormat == "cifs") && OsInfo.IsNotWindows) - { - // Force Transactional on a cifs mount due to the likeliness of move failures on certain scenario's on mono - verificationMode = DiskTransferVerificationMode.Transactional; - } - } + var isCifs = targetDriveFormat == "cifs"; if (mode.HasFlag(TransferMode.Copy)) { - if (verificationMode == DiskTransferVerificationMode.Transactional || verificationMode == DiskTransferVerificationMode.TryTransactional) - { - if (TryCopyFileTransactional(sourcePath, targetPath, originalSize)) - { - return TransferMode.Copy; - } - - throw new IOException(string.Format("Failed to completely transfer [{0}] to [{1}], aborting.", sourcePath, targetPath)); - } - else if (verificationMode == DiskTransferVerificationMode.VerifyOnly) - { - TryCopyFileVerified(sourcePath, targetPath, originalSize); - return TransferMode.Copy; - } - else - { - _diskProvider.CopyFile(sourcePath, targetPath); - return TransferMode.Copy; - } + TryCopyFileVerified(sourcePath, targetPath, originalSize); + return TransferMode.Copy; } if (mode.HasFlag(TransferMode.Move)) { - if (verificationMode == DiskTransferVerificationMode.Transactional || verificationMode == DiskTransferVerificationMode.TryTransactional) - { - if (TryMoveFileTransactional(sourcePath, targetPath, originalSize, verificationMode)) - { - return TransferMode.Move; - } - - throw new IOException(string.Format("Failed to completely transfer [{0}] to [{1}], aborting.", sourcePath, targetPath)); - } - else if (verificationMode == DiskTransferVerificationMode.VerifyOnly) - { - TryMoveFileVerified(sourcePath, targetPath, originalSize); - return TransferMode.Move; - } - else + if (isCifs && !isSameMount) { - _diskProvider.MoveFile(sourcePath, targetPath); + TryCopyFileVerified(sourcePath, targetPath, originalSize); + _diskProvider.DeleteFile(sourcePath); return TransferMode.Move; } + + TryMoveFileVerified(sourcePath, targetPath, originalSize); + return TransferMode.Move; } return TransferMode.None; @@ -464,137 +386,6 @@ namespace NzbDrone.Common.Disk Thread.Sleep(3000); } - private bool TryCopyFileTransactional(string sourcePath, string targetPath, long originalSize) - { - var tempTargetPath = targetPath + ".partial~"; - - if (_diskProvider.FileExists(tempTargetPath)) - { - _logger.Trace("Removing old partial."); - _diskProvider.DeleteFile(tempTargetPath); - } - - try - { - for (var i = 0; i <= RetryCount; i++) - { - _diskProvider.CopyFile(sourcePath, tempTargetPath); - - if (_diskProvider.FileExists(tempTargetPath)) - { - var targetSize = _diskProvider.GetFileSize(tempTargetPath); - if (targetSize == originalSize) - { - _diskProvider.MoveFile(tempTargetPath, targetPath); - return true; - } - } - - WaitForIO(); - - _diskProvider.DeleteFile(tempTargetPath); - - if (i == RetryCount) - { - _logger.Error("Failed to completely transfer [{0}] to [{1}], aborting.", sourcePath, targetPath); - } - else - { - _logger.Warn("Failed to completely transfer [{0}] to [{1}], retrying [{2}/{3}].", sourcePath, targetPath, i + 1, RetryCount); - } - } - } - catch - { - WaitForIO(); - - if (_diskProvider.FileExists(tempTargetPath)) - { - _diskProvider.DeleteFile(tempTargetPath); - } - - throw; - } - - return false; - } - - private bool TryMoveFileTransactional(string sourcePath, string targetPath, long originalSize, DiskTransferVerificationMode verificationMode) - { - var backupPath = sourcePath + ".backup~"; - var tempTargetPath = targetPath + ".partial~"; - - if (_diskProvider.FileExists(backupPath)) - { - _logger.Trace("Removing old backup."); - _diskProvider.DeleteFile(backupPath); - } - - if (_diskProvider.FileExists(tempTargetPath)) - { - _logger.Trace("Removing old partial."); - _diskProvider.DeleteFile(tempTargetPath); - } - - try - { - _logger.Trace("Attempting to move hardlinked backup."); - if (_diskProvider.TryCreateHardLink(sourcePath, backupPath)) - { - _diskProvider.MoveFile(backupPath, tempTargetPath); - - if (_diskProvider.FileExists(tempTargetPath)) - { - var targetSize = _diskProvider.GetFileSize(tempTargetPath); - - if (targetSize == originalSize) - { - _diskProvider.MoveFile(tempTargetPath, targetPath); - if (_diskProvider.FileExists(tempTargetPath)) - { - throw new IOException(string.Format("Temporary file '{0}' still exists, aborting.", tempTargetPath)); - } - - _logger.Trace("Hardlink move succeeded, deleting source."); - _diskProvider.DeleteFile(sourcePath); - return true; - } - } - - Thread.Sleep(5000); - - _diskProvider.DeleteFile(tempTargetPath); - } - } - finally - { - if (_diskProvider.FileExists(backupPath)) - { - _diskProvider.DeleteFile(backupPath); - } - } - - if (verificationMode == DiskTransferVerificationMode.Transactional) - { - _logger.Trace("Hardlink move failed, reverting to copy."); - if (TryCopyFileTransactional(sourcePath, targetPath, originalSize)) - { - _logger.Trace("Copy succeeded, deleting source."); - _diskProvider.DeleteFile(sourcePath); - return true; - } - } - else - { - _logger.Trace("Hardlink move failed, reverting to move."); - TryMoveFileVerified(sourcePath, targetPath, originalSize); - return true; - } - - _logger.Trace("Move failed."); - return false; - } - private void TryCopyFileVerified(string sourcePath, string targetPath, long originalSize) { try diff --git a/src/NzbDrone.Core.Test/MusicTests/MoveArtistServiceFixture.cs b/src/NzbDrone.Core.Test/MusicTests/MoveArtistServiceFixture.cs index 58a3e7df0..8688f53fb 100644 --- a/src/NzbDrone.Core.Test/MusicTests/MoveArtistServiceFixture.cs +++ b/src/NzbDrone.Core.Test/MusicTests/MoveArtistServiceFixture.cs @@ -63,7 +63,7 @@ namespace NzbDrone.Core.Test.MusicTests private void GivenFailedMove() { Mocker.GetMock() - .Setup(s => s.TransferFolder(It.IsAny(), It.IsAny(), TransferMode.Move, true)) + .Setup(s => s.TransferFolder(It.IsAny(), It.IsAny(), TransferMode.Move)) .Throws(); } @@ -99,8 +99,7 @@ namespace NzbDrone.Core.Test.MusicTests .Verify( v => v.TransferFolder(_command.SourcePath, _command.DestinationPath, - TransferMode.Move, - It.IsAny()), + TransferMode.Move), Times.Once()); Mocker.GetMock() @@ -123,8 +122,7 @@ namespace NzbDrone.Core.Test.MusicTests .Verify( v => v.TransferFolder(_bulkCommand.Artist.First().SourcePath, expectedPath, - TransferMode.Move, - It.IsAny()), + TransferMode.Move), Times.Once()); } @@ -141,8 +139,7 @@ namespace NzbDrone.Core.Test.MusicTests .Verify( v => v.TransferFolder(_command.SourcePath, _command.DestinationPath, - TransferMode.Move, - It.IsAny()), Times.Never()); + TransferMode.Move), Times.Never()); Mocker.GetMock() .Verify(v => v.GetArtistFolder(It.IsAny(), null), Times.Never()); diff --git a/src/NzbDrone.Core.Test/ProviderTests/RecycleBinProviderTests/DeleteDirectoryFixture.cs b/src/NzbDrone.Core.Test/ProviderTests/RecycleBinProviderTests/DeleteDirectoryFixture.cs index 5ff398d62..433e6f3a6 100644 --- a/src/NzbDrone.Core.Test/ProviderTests/RecycleBinProviderTests/DeleteDirectoryFixture.cs +++ b/src/NzbDrone.Core.Test/ProviderTests/RecycleBinProviderTests/DeleteDirectoryFixture.cs @@ -46,7 +46,7 @@ namespace NzbDrone.Core.Test.ProviderTests.RecycleBinProviderTests Mocker.Resolve().DeleteFolder(path); Mocker.GetMock() - .Verify(v => v.TransferFolder(path, @"C:\Test\Recycle Bin\30 Rock".AsOsAgnostic(), TransferMode.Move, true), Times.Once()); + .Verify(v => v.TransferFolder(path, @"C:\Test\Recycle Bin\30 Rock".AsOsAgnostic(), TransferMode.Move), Times.Once()); } [Test] diff --git a/src/NzbDrone.Core.Test/ProviderTests/RecycleBinProviderTests/DeleteFileFixture.cs b/src/NzbDrone.Core.Test/ProviderTests/RecycleBinProviderTests/DeleteFileFixture.cs index 09404ae0d..b01338f7b 100644 --- a/src/NzbDrone.Core.Test/ProviderTests/RecycleBinProviderTests/DeleteFileFixture.cs +++ b/src/NzbDrone.Core.Test/ProviderTests/RecycleBinProviderTests/DeleteFileFixture.cs @@ -1,4 +1,4 @@ -using System; +using System; using Moq; using NUnit.Framework; using NzbDrone.Common.Disk; @@ -44,7 +44,7 @@ namespace NzbDrone.Core.Test.ProviderTests.RecycleBinProviderTests Mocker.Resolve().DeleteFile(path); - Mocker.GetMock().Verify(v => v.TransferFile(path, @"C:\Test\Recycle Bin\S01E01.avi".AsOsAgnostic(), TransferMode.Move, false, true), Times.Once()); + Mocker.GetMock().Verify(v => v.TransferFile(path, @"C:\Test\Recycle Bin\S01E01.avi".AsOsAgnostic(), TransferMode.Move, false), Times.Once()); } [Test] @@ -60,7 +60,7 @@ namespace NzbDrone.Core.Test.ProviderTests.RecycleBinProviderTests Mocker.Resolve().DeleteFile(path); - Mocker.GetMock().Verify(v => v.TransferFile(path, @"C:\Test\Recycle Bin\S01E01_2.avi".AsOsAgnostic(), TransferMode.Move, false, true), Times.Once()); + Mocker.GetMock().Verify(v => v.TransferFile(path, @"C:\Test\Recycle Bin\S01E01_2.avi".AsOsAgnostic(), TransferMode.Move, false), Times.Once()); } [Test] @@ -84,7 +84,7 @@ namespace NzbDrone.Core.Test.ProviderTests.RecycleBinProviderTests Mocker.Resolve().DeleteFile(path, "30 Rock"); - Mocker.GetMock().Verify(v => v.TransferFile(path, @"C:\Test\Recycle Bin\30 Rock\S01E01.avi".AsOsAgnostic(), TransferMode.Move, false, true), Times.Once()); + Mocker.GetMock().Verify(v => v.TransferFile(path, @"C:\Test\Recycle Bin\30 Rock\S01E01.avi".AsOsAgnostic(), TransferMode.Move, false), Times.Once()); } } } diff --git a/src/NzbDrone.Core.Test/UpdateTests/UpdateServiceFixture.cs b/src/NzbDrone.Core.Test/UpdateTests/UpdateServiceFixture.cs index 1d5c41577..3470710b2 100644 --- a/src/NzbDrone.Core.Test/UpdateTests/UpdateServiceFixture.cs +++ b/src/NzbDrone.Core.Test/UpdateTests/UpdateServiceFixture.cs @@ -145,7 +145,7 @@ namespace NzbDrone.Core.Test.UpdateTests Subject.Execute(new ApplicationUpdateCommand()); Mocker.GetMock() - .Verify(c => c.TransferFolder(updateClientFolder, _sandboxFolder, TransferMode.Move, false)); + .Verify(c => c.TransferFolder(updateClientFolder, _sandboxFolder, TransferMode.Move)); } [Test] diff --git a/src/NzbDrone.Core/Extras/Files/ExtraFileManager.cs b/src/NzbDrone.Core/Extras/Files/ExtraFileManager.cs index 5036a226b..d3ed88c7a 100644 --- a/src/NzbDrone.Core/Extras/Files/ExtraFileManager.cs +++ b/src/NzbDrone.Core/Extras/Files/ExtraFileManager.cs @@ -69,7 +69,7 @@ namespace NzbDrone.Core.Extras.Files transferMode = _configService.CopyUsingHardlinks ? TransferMode.HardLinkOrCopy : TransferMode.Copy; } - _diskTransferService.TransferFile(path, newFileName, transferMode, true, false); + _diskTransferService.TransferFile(path, newFileName, transferMode, true); return new TExtraFile { diff --git a/src/NzbDrone.Core/Update/InstallUpdateService.cs b/src/NzbDrone.Core/Update/InstallUpdateService.cs index 0540db052..5ca8f80d1 100644 --- a/src/NzbDrone.Core/Update/InstallUpdateService.cs +++ b/src/NzbDrone.Core/Update/InstallUpdateService.cs @@ -144,7 +144,7 @@ namespace NzbDrone.Core.Update } _logger.Info("Preparing client"); - _diskTransferService.TransferFolder(_appFolderInfo.GetUpdateClientFolder(), updateSandboxFolder, TransferMode.Move, false); + _diskTransferService.TransferFolder(_appFolderInfo.GetUpdateClientFolder(), updateSandboxFolder, TransferMode.Move); // Set executable flag on update app if (OsInfo.IsOsx || (OsInfo.IsLinux && PlatformInfo.IsNetCore))