From a7507fb765b2542137db4546ade269182b9379d9 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 10:35:50 +0100 Subject: [PATCH 01/19] Simplified variable assignment --- Duplicati/Library/Main/Operation/Restore/FileProcessor.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 3aa584dd6..b9249c327 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -98,9 +98,7 @@ namespace Duplicati.Library.Main.Operation.Restore if (missing_blocks.Count > 0 && options.UseLocalBlocks && missing_blocks.Count > 0) { // Verify the local blocks at the original restore path that may be used to restore the file. - var (bw, new_missing_blocks) = await VerifyLocalBlocks(file, missing_blocks, blocks.Length, filehasher, blockhasher, options, results, block_request); - bytes_written = bw; - missing_blocks = new_missing_blocks; + (bytes_written, missing_blocks) = await VerifyLocalBlocks(file, missing_blocks, blocks.Length, filehasher, blockhasher, options, results, block_request); } if (file.BlocksetID != LocalDatabase.SYMLINK_BLOCKSET_ID && (blocks.Length == 0 || (blocks.Length == 1 && blocks[0].BlockSize == 0))) From a14d29da9316b3e8612d071e2c88d5eaa068d13d Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 10:40:14 +0100 Subject: [PATCH 02/19] Modified VerifyTargetBlocks to also return the blocks that were verified. --- .../Main/Operation/Restore/FileProcessor.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index b9249c327..801f6e7b7 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -93,9 +93,13 @@ namespace Duplicati.Library.Main.Operation.Restore sw_work?.Start(); // Verify the target file blocks that may already exist. - var (bytes_written, missing_blocks) = await VerifyTargetBlocks(file, blocks, filehasher, blockhasher, options, results, block_request); - - if (missing_blocks.Count > 0 && options.UseLocalBlocks && missing_blocks.Count > 0) + var (bytes_written, missing_blocks, verified_blocks) = await VerifyTargetBlocks(file, blocks, filehasher, blockhasher, options, results, block_request); + if (blocks.Length != missing_blocks.Count + verified_blocks.Count) + { + Logging.Log.WriteErrorMessage(LOGTAG, "BlockCountMismatch", null, $"Block count mismatch for {file.TargetPath} - expected: {blocks.Length}, actual: {missing_blocks.Count + verified_blocks.Count}"); + sw_work?.Stop(); + continue; + } { // Verify the local blocks at the original restore path that may be used to restore the file. (bytes_written, missing_blocks) = await VerifyLocalBlocks(file, missing_blocks, blocks.Length, filehasher, blockhasher, options, results, block_request); @@ -343,10 +347,11 @@ namespace Duplicati.Library.Main.Operation.Restore /// The restoration results. /// The channel to request blocks from the block manager. Used to inform the block manager which blocks are already present. /// An awaitable `Task`, which returns a collection of data blocks that are missing. - private static async Task<(long,List)> VerifyTargetBlocks(FileRequest file, BlockRequest[] blocks, System.Security.Cryptography.HashAlgorithm filehasher, System.Security.Cryptography.HashAlgorithm blockhasher, Options options, RestoreResults results, IChannel block_request) + private static async Task<(long,List,List)> VerifyTargetBlocks(FileRequest file, BlockRequest[] blocks, System.Security.Cryptography.HashAlgorithm filehasher, System.Security.Cryptography.HashAlgorithm blockhasher, Options options, RestoreResults results, IChannel block_request) { long bytes_read = 0; List missing_blocks = []; + List verified_blocks = []; // Check if the file exists if (File.Exists(file.TargetPath)) @@ -370,6 +375,7 @@ namespace Duplicati.Library.Main.Operation.Restore bytes_read += read; blocks[i].CacheDecrEvict = true; await block_request.WriteAsync(blocks[i]); + verified_blocks.Add(blocks[i]); } else { @@ -430,7 +436,7 @@ namespace Duplicati.Library.Main.Operation.Restore missing_blocks.AddRange(blocks); } - return (bytes_read, missing_blocks); + return (bytes_read, missing_blocks, verified_blocks); } /// From be99f03bacd3ebe452733dbfaf61837dd3326b66 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 10:41:33 +0100 Subject: [PATCH 03/19] VerifyLocalBlocks wasn't respecting dry run properly --- .../Main/Operation/Restore/FileProcessor.cs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 801f6e7b7..83fa6c115 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -462,7 +462,11 @@ namespace Duplicati.Library.Main.Operation.Restore // Open both files, as the target file is still being read to produce the overall file hash, if all the blocks are present across both the target and original files. using var f_original = SystemIO.IO_OS.FileOpenRead(file.OriginalPath); - using var f_target = SystemIO.IO_OS.FileOpenWrite(file.TargetPath); + using var f_target = options.Dryrun ? + (SystemIO.IO_OS.FileExists(file.TargetPath) ? + SystemIO.IO_OS.FileOpenRead(file.TargetPath) : + null) : + SystemIO.IO_OS.FileOpenWrite(file.TargetPath); var buffer = new byte[options.Blocksize]; long bytes_read = 0; long bytes_written = 0; @@ -496,6 +500,8 @@ namespace Duplicati.Library.Main.Operation.Restore missing_blocks.Add(blocks[j]); } else + { + if (!options.Dryrun) { try { @@ -509,6 +515,7 @@ namespace Duplicati.Library.Main.Operation.Restore results.BrokenLocalFiles.Add(file.TargetPath); } throw; + } } bytes_read += read; bytes_written += read; @@ -533,8 +540,8 @@ namespace Duplicati.Library.Main.Operation.Restore // The current block is not a missing block - read from the target file. try { - f_target.Seek(i * options.Blocksize, SeekOrigin.Begin); - read = await f_target.ReadAsync(buffer, 0, options.Blocksize); + f_target?.Seek(i * options.Blocksize, SeekOrigin.Begin); + read = await f_target?.ReadAsync(buffer, 0, options.Blocksize); } catch (Exception) { @@ -555,7 +562,7 @@ namespace Duplicati.Library.Main.Operation.Restore if (Convert.ToBase64String(filehasher.Hash) == file.Hash) { // Truncate the file if it is larger than the expected size. - if (file.Length < f_target.Length) + if (file.Length < f_target?.Length) { if (options.Dryrun) { @@ -588,6 +595,11 @@ namespace Duplicati.Library.Main.Operation.Restore } } + if (options.Dryrun) + { + Logging.Log.WriteDryrunMessage(LOGTAG, "DryrunRestore", @$"Would have restored {verified_blocks.Count} blocks ({bytes_written} bytes) from ""{file.OriginalPath}"" to ""{file.TargetPath}"""); + } + return (bytes_written, missing_blocks); } else From 8e92754efded2ebe38bee673d49744e383a48061 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 10:43:01 +0100 Subject: [PATCH 04/19] VerifyLocalBlocks now keeps track of internally verified blocks for logging --- Duplicati/Library/Main/Operation/Restore/FileProcessor.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 83fa6c115..68e55d25d 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -454,6 +454,7 @@ namespace Duplicati.Library.Main.Operation.Restore private static async Task<(long, List)> VerifyLocalBlocks(FileRequest file, List blocks, long total_blocks, System.Security.Cryptography.HashAlgorithm filehasher, System.Security.Cryptography.HashAlgorithm blockhasher, Options options, RestoreResults results, IChannel block_request) { List missing_blocks = []; + List verified_blocks = []; // Check if the file exists if (File.Exists(file.OriginalPath)) @@ -521,6 +522,7 @@ namespace Duplicati.Library.Main.Operation.Restore bytes_written += read; blocks[j].CacheDecrEvict = true; await block_request.WriteAsync(blocks[j]); + verified_blocks.Add(blocks[j]); } } else From f76bf90e686956fd437cc3b3cf46c7d36ea86279 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 10:43:23 +0100 Subject: [PATCH 05/19] Added additional dry run logging --- Duplicati/Library/Main/Operation/Restore/FileProcessor.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 68e55d25d..03ab0d186 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -273,6 +273,8 @@ namespace Duplicati.Library.Main.Operation.Restore results.SizeOfRestoredFiles += bytes_written; } sw_work?.Stop(); + + Logging.Log.WriteVerboseMessage(LOGTAG, "RestoredFile", "Restored file {0}", file.TargetPath); } } catch (RetiredException) From eba477c271fd1b2419d8f4348ed051e0f6f98fd3 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 10:44:08 +0100 Subject: [PATCH 06/19] Added handling of the 'overwrite' option in the new restore flow --- .../Main/Operation/Restore/FileProcessor.cs | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 03ab0d186..11d25991f 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -100,6 +100,28 @@ namespace Duplicati.Library.Main.Operation.Restore sw_work?.Stop(); continue; } + + // Check if the target file needs to be retargeted + if (missing_blocks.Count > 0 && !options.Overwrite) + { + var new_name = GenerateNewName(file, db, filehasher); + Logging.Log.WriteVerboseMessage(LOGTAG, "RetargetingFile", "Retargeting file {0} to {1}", file.TargetPath, new_name); + var new_file = new FileRequest(file.ID, file.OriginalPath, new_name, file.Hash, file.Length, file.BlocksetID); + if (options.UseLocalBlocks) + { + if (options.Dryrun) + { + Logging.Log.WriteDryrunMessage(LOGTAG, "DryrunRestore", @$"Would have copied {verified_blocks.Count} blocks ({verified_blocks.Count * options.Blocksize} bytes) from ""{file.TargetPath}"" to ""{new_file.TargetPath}"""); + } + else + { + CopyOldTargetBlocksToNewTarget(file, new_file, verified_blocks); + } + } + file = new_file; + } + + if (missing_blocks.Count > 0 && options.UseLocalBlocks) { // Verify the local blocks at the original restore path that may be used to restore the file. (bytes_written, missing_blocks) = await VerifyLocalBlocks(file, missing_blocks, blocks.Length, filehasher, blockhasher, options, results, block_request); @@ -299,6 +321,63 @@ namespace Duplicati.Library.Main.Operation.Restore }); } + private static void CopyOldTargetBlocksToNewTarget(FileRequest file, FileRequest new_file, List verified_blocks) + { + using var fs_old = SystemIO.IO_OS.FileOpenRead(file.TargetPath); + using var fs_new = SystemIO.IO_OS.FileOpenWrite(new_file.TargetPath); + + foreach (var block in verified_blocks) + { + fs_old.Seek(block.BlockOffset * block.BlockSize, SeekOrigin.Begin); + fs_new.Seek(block.BlockOffset * block.BlockSize, SeekOrigin.Begin); + + var buffer = new byte[block.BlockSize]; + fs_old.Read(buffer, 0, buffer.Length); + fs_new.Write(buffer, 0, buffer.Length); + } + } + + private static string GenerateNewName(FileRequest request, LocalRestoreDatabase database, System.Security.Cryptography.HashAlgorithm filehasher) + { + var ext = SystemIO.IO_OS.PathGetExtension(request.TargetPath) ?? ""; + if (!string.IsNullOrEmpty(ext) && !ext.StartsWith(".", StringComparison.Ordinal)) + ext = "." + ext; + + // First we try with a simple date append, assuming that there are not many conflicts there + var newname = SystemIO.IO_OS.PathChangeExtension(request.TargetPath, null) + "." + database.RestoreTime.ToLocalTime().ToString("yyyy-MM-dd", System.Globalization.CultureInfo.InvariantCulture); + var tr = newname + ext; + var c = 0; + while (SystemIO.IO_OS.FileExists(tr) && c < 1000) + { + try + { + // If we have a file with the correct name, + // it is most likely the file we want + filehasher.Initialize(); + + string key; + using (var file = SystemIO.IO_OS.FileOpenRead(tr)) + key = Convert.ToBase64String(filehasher.ComputeHash(file)); + + if (key == request.Hash) + { + //TODO: Also needs metadata check to make correct decision. + // We stick to the policy to restore metadata in place, if data ok. So, metadata block may be restored. + break; + } + } + catch (Exception ex) + { + Logging.Log.WriteWarningMessage(LOGTAG, "FailedToReadRestoreTarget", ex, "Failed to read candidate restore target {0}", tr); + } + tr = newname + " (" + (c++).ToString() + ")" + ext; + } + + newname = tr; + + return newname; + } + /// /// Restores the metadata for a file. /// From 637bfcefc8fc6fc1d9ef401fda4f5b97c1a107fd Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 11:25:19 +0100 Subject: [PATCH 07/19] Forgot to check whether the file exists --- Duplicati/Library/Main/Operation/Restore/FileProcessor.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 11d25991f..fea56ecc7 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -102,7 +102,7 @@ namespace Duplicati.Library.Main.Operation.Restore } // Check if the target file needs to be retargeted - if (missing_blocks.Count > 0 && !options.Overwrite) + if (missing_blocks.Count > 0 && !options.Overwrite && SystemIO.IO_OS.FileExists(file.TargetPath)) { var new_name = GenerateNewName(file, db, filehasher); Logging.Log.WriteVerboseMessage(LOGTAG, "RetargetingFile", "Retargeting file {0} to {1}", file.TargetPath, new_name); From eb64320a4902d87bda67ab3c24ee215397a612e5 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 11:51:52 +0100 Subject: [PATCH 08/19] Changed the reporting of restored files to match the legacy restore's reporting. --- .../Main/Operation/Restore/FileProcessor.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index fea56ecc7..6c002e6c5 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -93,7 +93,8 @@ namespace Duplicati.Library.Main.Operation.Restore sw_work?.Start(); // Verify the target file blocks that may already exist. - var (bytes_written, missing_blocks, verified_blocks) = await VerifyTargetBlocks(file, blocks, filehasher, blockhasher, options, results, block_request); + var (bytes_verified, missing_blocks, verified_blocks) = await VerifyTargetBlocks(file, blocks, filehasher, blockhasher, options, results, block_request); + long bytes_written = 0; if (blocks.Length != missing_blocks.Count + verified_blocks.Count) { Logging.Log.WriteErrorMessage(LOGTAG, "BlockCountMismatch", null, $"Block count mismatch for {file.TargetPath} - expected: {blocks.Length}, actual: {missing_blocks.Count + verified_blocks.Count}"); @@ -288,11 +289,15 @@ namespace Duplicati.Library.Main.Operation.Restore await RestoreMetadata(db, file, block_request, block_response, options, sw_meta, sw_work, sw_req, sw_resp); } - // Keep track of the restored files and their sizes - lock (results) + // TODO legacy restore doesn't count metadata restore as a restored file. + if (bytes_written > 0) { - results.RestoredFiles++; - results.SizeOfRestoredFiles += bytes_written; + // Keep track of the restored files and their sizes + lock (results) + { + results.RestoredFiles++; + results.SizeOfRestoredFiles += bytes_written; + } } sw_work?.Stop(); From 23aefd51c94a8289b732cde5ad6cad8877943ed0 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 11:52:12 +0100 Subject: [PATCH 09/19] Add unit test for Issue #5825 Restore behavior with overwrite option --- Duplicati/UnitTest/IssueTests.cs | 110 +++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/Duplicati/UnitTest/IssueTests.cs b/Duplicati/UnitTest/IssueTests.cs index 5683fddcc..72f156571 100644 --- a/Duplicati/UnitTest/IssueTests.cs +++ b/Duplicati/UnitTest/IssueTests.cs @@ -118,6 +118,116 @@ namespace Duplicati.UnitTest } + [Test] + [Category("Restore"), Category("Bug")] + public void Issue5825RestoreNoOverwrite([Values] bool legacy, [Values] bool local_blocks) + { + // Reproduction of Issue #5825 + // The logic in the previous version was to create a timestamped version of the file being restored to, if it already exists. + // It appears the new restore flow is not correctly doing the same. + // See forum thread: https://forum.duplicati.com/t/save-different-versions-with-timestamp-in-file-name-broken/19805 + + var testopts = new Dictionary(TestOptions) + { + ["restore-legacy"] = legacy.ToString().ToLower(), + ["restore-with-local-blocks"] = local_blocks.ToString().ToLower() + }; + + var original_dir = Path.Combine(DATAFOLDER, "some_original_dir"); + Directory.CreateDirectory(original_dir); + string f0 = Path.Combine(original_dir, "some_file"); + TestUtils.WriteTestFile(f0, 1024 * 20); + + // Backup the files + using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) + { + IBackupResults backupResults = c.Backup([DATAFOLDER]); + TestUtils.AssertResults(backupResults); + } + + // Attempt to restore the file + testopts["restore-path"] = RESTOREFOLDER; + using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) + { + var restoreResults = c.Restore([f0]); + Assert.That(restoreResults.RestoredFiles, Is.EqualTo(1), "File should have been restored"); + } + + // Verify that the files are equal + string f1 = Path.Combine(RESTOREFOLDER, "some_file"); + Assert.That(File.ReadAllBytes(f0), Is.EqualTo(File.ReadAllBytes(f1)), "Restored file should be equal to original file"); + + // Modify the restored file + TestUtils.WriteTestFile(f1, 1024 * 20); + + // Restore the file again, with overwrite. + testopts["overwrite"] = "true"; + using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) + { + var restoreResults = c.Restore([f0]); + Assert.That(restoreResults.RestoredFiles, Is.EqualTo(1), "File should have been restored"); + } + + // Verify that the files are equal + Assert.That(File.ReadAllBytes(f0), Is.EqualTo(File.ReadAllBytes(f1)), "Restored file should be equal to original file"); + + // Save the timestamp of the file + var timestamp = File.GetLastWriteTime(f1); + + // Touch the file + File.SetLastWriteTime(f1, DateTime.Now); + + // Check that the timestamp has changed, but the file is still equal + Assert.That(File.GetLastWriteTime(f1), Is.Not.EqualTo(timestamp), "Timestamp should have changed"); + Assert.That(File.ReadAllBytes(f0), Is.EqualTo(File.ReadAllBytes(f1)), "Restored file should be equal to original file"); + + // Restore the file again, with overwrite, should restore the timestamp of the file + using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) + { + var restoreResults = c.Restore([f0]); + Assert.That(restoreResults.RestoredFiles, Is.EqualTo(0), "File should not have been restored, only the metadata."); + } + + // Verify that the files are equal and the timestamp is restored + Assert.That(File.ReadAllBytes(f0), Is.EqualTo(File.ReadAllBytes(f1)), "Restored file should be equal to original file"); + Assert.That(File.GetLastWriteTime(f1), Is.EqualTo(timestamp), "Timestamp should be restored"); + + // Modify the restored file + TestUtils.WriteTestFile(f1, 1024 * 20); + + // Restore the file again, without overwrite. + testopts["overwrite"] = "false"; + using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) + { + var restoreResults = c.Restore([f0]); + Assert.That(restoreResults.RestoredFiles, Is.EqualTo(1), "File should have been restored"); + } + + // Verify that there exists a new file with a timestamp + var files = Directory.GetFiles(RESTOREFOLDER, "*", SearchOption.TopDirectoryOnly); + Assert.That(files.Length, Is.EqualTo(2), "There should be two files in the folder"); + var f2 = files.FirstOrDefault(v => v != f1); + + // Modify the new restored file as well + TestUtils.WriteTestFile(f2, 1024 * 20); + + // Restore the file again, without overwrite. + using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) + { + var restoreResults = c.Restore([f0]); + Assert.That(restoreResults.RestoredFiles, Is.EqualTo(1), "File should have been restored"); + } + + // Verify that there exists a new file with a timestamp + files = Directory.GetFiles(RESTOREFOLDER, "*", SearchOption.TopDirectoryOnly); + Assert.That(files.Length, Is.EqualTo(3), "There should be three files in the folder"); + + // Verify that the files are equal + var f3 = files.FirstOrDefault(v => v != f1 && v != f2); + Assert.That(File.ReadAllBytes(f0), Is.EqualTo(File.ReadAllBytes(f3)), "Restored file should be equal to original file"); + } + + [Test] [Category("Disruption"), Category("Bug")] public void TestSystematicErrors5023() From b759b4564d358f4664faf7eb28da60f7e8eb265c Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 12:09:04 +0100 Subject: [PATCH 10/19] Added docstrings to new functions --- .../Main/Operation/Restore/FileProcessor.cs | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 6c002e6c5..9bf30020a 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -326,9 +326,15 @@ namespace Duplicati.Library.Main.Operation.Restore }); } - private static void CopyOldTargetBlocksToNewTarget(FileRequest file, FileRequest new_file, List verified_blocks) + /// + /// Copies the blocks that were verified in the old target to the new target file. + /// + /// The old target file. + /// The new target file. + /// The blocks in the old file that were verified. + private static void CopyOldTargetBlocksToNewTarget(FileRequest old_file, FileRequest new_file, List verified_blocks) { - using var fs_old = SystemIO.IO_OS.FileOpenRead(file.TargetPath); + using var fs_old = SystemIO.IO_OS.FileOpenRead(old_file.TargetPath); using var fs_new = SystemIO.IO_OS.FileOpenWrite(new_file.TargetPath); foreach (var block in verified_blocks) @@ -342,6 +348,18 @@ namespace Duplicati.Library.Main.Operation.Restore } } + /// + /// Generates a new name for the target file. It starts by appending + /// the restore time to the file name. If that name is also already + /// taken, it appends a number to the name. It keeps incrementing the + /// number until it finds a name that is not taken. If it encounters a + /// file with the same name and the correct hash, it will retarget the + /// filename to that file, assuming that the file is the correct one. + /// + /// The original target file. + /// The restore database. + /// The file hasher used to verify whether any of the + /// The new filename. If the file already exists, its file hash matches the target hash. private static string GenerateNewName(FileRequest request, LocalRestoreDatabase database, System.Security.Cryptography.HashAlgorithm filehasher) { var ext = SystemIO.IO_OS.PathGetExtension(request.TargetPath) ?? ""; From 4cd565d592c5fd6afc1edff461effcb8994b260b Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 12:09:38 +0100 Subject: [PATCH 11/19] Added handling of already restored timestamp-retargeted files --- .../Main/Operation/Restore/FileProcessor.cs | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 9bf30020a..60f951a08 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -106,20 +106,29 @@ namespace Duplicati.Library.Main.Operation.Restore if (missing_blocks.Count > 0 && !options.Overwrite && SystemIO.IO_OS.FileExists(file.TargetPath)) { var new_name = GenerateNewName(file, db, filehasher); - Logging.Log.WriteVerboseMessage(LOGTAG, "RetargetingFile", "Retargeting file {0} to {1}", file.TargetPath, new_name); - var new_file = new FileRequest(file.ID, file.OriginalPath, new_name, file.Hash, file.Length, file.BlocksetID); - if (options.UseLocalBlocks) + if (SystemIO.IO_OS.FileExists(new_name)) { - if (options.Dryrun) - { - Logging.Log.WriteDryrunMessage(LOGTAG, "DryrunRestore", @$"Would have copied {verified_blocks.Count} blocks ({verified_blocks.Count * options.Blocksize} bytes) from ""{file.TargetPath}"" to ""{new_file.TargetPath}"""); - } - else - { - CopyOldTargetBlocksToNewTarget(file, new_file, verified_blocks); - } + // The file already exists, which it only does when it matches the target hash. So we can skip the file. + Logging.Log.WriteInformationMessage(LOGTAG, "FileAlreadyExists", null, $"File {file.TargetPath} already exists and matches the target hash as a copy: {new_name}"); + missing_blocks.Clear(); + } + else + { + Logging.Log.WriteVerboseMessage(LOGTAG, "RetargetingFile", "Retargeting file {0} to {1}", file.TargetPath, new_name); + var new_file = new FileRequest(file.ID, file.OriginalPath, new_name, file.Hash, file.Length, file.BlocksetID); + if (options.UseLocalBlocks) + { + if (options.Dryrun) + { + Logging.Log.WriteDryrunMessage(LOGTAG, "DryrunRestore", @$"Would have copied {verified_blocks.Count} blocks ({verified_blocks.Count * options.Blocksize} bytes) from ""{file.TargetPath}"" to ""{new_file.TargetPath}"""); + } + else + { + CopyOldTargetBlocksToNewTarget(file, new_file, verified_blocks); + } + } + file = new_file; } - file = new_file; } if (missing_blocks.Count > 0 && options.UseLocalBlocks) From 193b5a8e64e2fa8c310dea6a738bffa491a02a41 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 12:15:43 +0100 Subject: [PATCH 12/19] Extended the 5825 test to verify finding an already restored but timestamp retargeted file. --- Duplicati/UnitTest/IssueTests.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Duplicati/UnitTest/IssueTests.cs b/Duplicati/UnitTest/IssueTests.cs index 72f156571..6204c8c7f 100644 --- a/Duplicati/UnitTest/IssueTests.cs +++ b/Duplicati/UnitTest/IssueTests.cs @@ -1,4 +1,4 @@ -using Duplicati.Library.DynamicLoader; +using Duplicati.Library.DynamicLoader; using Duplicati.Library.Interface; using Duplicati.Library.Main; using NUnit.Framework; @@ -225,6 +225,16 @@ namespace Duplicati.UnitTest // Verify that the files are equal var f3 = files.FirstOrDefault(v => v != f1 && v != f2); Assert.That(File.ReadAllBytes(f0), Is.EqualTo(File.ReadAllBytes(f3)), "Restored file should be equal to original file"); + + // Modify the second file to match the original file - should not restore any files + File.WriteAllBytes(f2, File.ReadAllBytes(f0)); + + // Restore the file again, without overwrite. + using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) + { + var restoreResults = c.Restore([f0]); + Assert.That(restoreResults.RestoredFiles, Is.EqualTo(0), "File should not have been restored"); + } } From 5b7314621ccef885adee921a5c164cfb6769cf21 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 12:17:16 +0100 Subject: [PATCH 13/19] Reduced test file size to speed up the test. File size shouldn't matter too much here. --- Duplicati/UnitTest/IssueTests.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Duplicati/UnitTest/IssueTests.cs b/Duplicati/UnitTest/IssueTests.cs index 6204c8c7f..b4e09583c 100644 --- a/Duplicati/UnitTest/IssueTests.cs +++ b/Duplicati/UnitTest/IssueTests.cs @@ -1,4 +1,4 @@ -using Duplicati.Library.DynamicLoader; +using Duplicati.Library.DynamicLoader; using Duplicati.Library.Interface; using Duplicati.Library.Main; using NUnit.Framework; @@ -132,11 +132,12 @@ namespace Duplicati.UnitTest ["restore-legacy"] = legacy.ToString().ToLower(), ["restore-with-local-blocks"] = local_blocks.ToString().ToLower() }; + int test_filesize = 1024; var original_dir = Path.Combine(DATAFOLDER, "some_original_dir"); Directory.CreateDirectory(original_dir); string f0 = Path.Combine(original_dir, "some_file"); - TestUtils.WriteTestFile(f0, 1024 * 20); + TestUtils.WriteTestFile(f0, test_filesize); // Backup the files using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) @@ -158,7 +159,7 @@ namespace Duplicati.UnitTest Assert.That(File.ReadAllBytes(f0), Is.EqualTo(File.ReadAllBytes(f1)), "Restored file should be equal to original file"); // Modify the restored file - TestUtils.WriteTestFile(f1, 1024 * 20); + TestUtils.WriteTestFile(f1, test_filesize); // Restore the file again, with overwrite. testopts["overwrite"] = "true"; @@ -193,7 +194,7 @@ namespace Duplicati.UnitTest Assert.That(File.GetLastWriteTime(f1), Is.EqualTo(timestamp), "Timestamp should be restored"); // Modify the restored file - TestUtils.WriteTestFile(f1, 1024 * 20); + TestUtils.WriteTestFile(f1, test_filesize); // Restore the file again, without overwrite. testopts["overwrite"] = "false"; @@ -209,7 +210,7 @@ namespace Duplicati.UnitTest var f2 = files.FirstOrDefault(v => v != f1); // Modify the new restored file as well - TestUtils.WriteTestFile(f2, 1024 * 20); + TestUtils.WriteTestFile(f2, test_filesize); // Restore the file again, without overwrite. using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) From e88b16dd868f45ebf59a5cdca3ad4132f8860edb Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 12:28:08 +0100 Subject: [PATCH 14/19] Add assertions to verify untouched files during restore operations --- Duplicati/UnitTest/IssueTests.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Duplicati/UnitTest/IssueTests.cs b/Duplicati/UnitTest/IssueTests.cs index b4e09583c..425744d75 100644 --- a/Duplicati/UnitTest/IssueTests.cs +++ b/Duplicati/UnitTest/IssueTests.cs @@ -134,6 +134,8 @@ namespace Duplicati.UnitTest }; int test_filesize = 1024; + // TODO tjek om de gamle filer forbliver urørte. + var original_dir = Path.Combine(DATAFOLDER, "some_original_dir"); Directory.CreateDirectory(original_dir); string f0 = Path.Combine(original_dir, "some_file"); @@ -195,6 +197,7 @@ namespace Duplicati.UnitTest // Modify the restored file TestUtils.WriteTestFile(f1, test_filesize); + var f1_original = File.ReadAllBytes(f1); // Restore the file again, without overwrite. testopts["overwrite"] = "false"; @@ -204,6 +207,9 @@ namespace Duplicati.UnitTest Assert.That(restoreResults.RestoredFiles, Is.EqualTo(1), "File should have been restored"); } + // Verify that f1 is still the same + Assert.That(File.ReadAllBytes(f1), Is.EqualTo(f1_original), "The first restored file should remain untouched"); + // Verify that there exists a new file with a timestamp var files = Directory.GetFiles(RESTOREFOLDER, "*", SearchOption.TopDirectoryOnly); Assert.That(files.Length, Is.EqualTo(2), "There should be two files in the folder"); @@ -211,6 +217,7 @@ namespace Duplicati.UnitTest // Modify the new restored file as well TestUtils.WriteTestFile(f2, test_filesize); + var f2_original = File.ReadAllBytes(f2); // Restore the file again, without overwrite. using (var c = new Library.Main.Controller("file://" + TARGETFOLDER, testopts, null)) @@ -219,6 +226,10 @@ namespace Duplicati.UnitTest Assert.That(restoreResults.RestoredFiles, Is.EqualTo(1), "File should have been restored"); } + // Verify that f1 and f2 are still the same + Assert.That(File.ReadAllBytes(f1), Is.EqualTo(f1_original), "The first restored file should remain untouched"); + Assert.That(File.ReadAllBytes(f2), Is.EqualTo(f2_original), "The second restored file should remain untouched"); + // Verify that there exists a new file with a timestamp files = Directory.GetFiles(RESTOREFOLDER, "*", SearchOption.TopDirectoryOnly); Assert.That(files.Length, Is.EqualTo(3), "There should be three files in the folder"); @@ -236,6 +247,9 @@ namespace Duplicati.UnitTest var restoreResults = c.Restore([f0]); Assert.That(restoreResults.RestoredFiles, Is.EqualTo(0), "File should not have been restored"); } + + // Verify that f1 is still untouched + Assert.That(File.ReadAllBytes(f1), Is.EqualTo(f1_original), "The first restored file should remain untouched"); } From 1bea0f36966262e2ce054fa3edc684dc4228c181 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Sun, 5 Jan 2025 12:46:08 +0100 Subject: [PATCH 15/19] Restored empty files should count towards the restoration file count. --- Duplicati/Library/Main/Operation/Restore/FileProcessor.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 60f951a08..a79d97d38 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -137,8 +137,10 @@ namespace Duplicati.Library.Main.Operation.Restore (bytes_written, missing_blocks) = await VerifyLocalBlocks(file, missing_blocks, blocks.Length, filehasher, blockhasher, options, results, block_request); } + bool empty_file = false; if (file.BlocksetID != LocalDatabase.SYMLINK_BLOCKSET_ID && (blocks.Length == 0 || (blocks.Length == 1 && blocks[0].BlockSize == 0))) { + empty_file = true; if (options.Dryrun) { Logging.Log.WriteDryrunMessage(LOGTAG, "DryrunRestore", @$"Would have created empty file ""{file.TargetPath}"""); @@ -299,7 +301,7 @@ namespace Duplicati.Library.Main.Operation.Restore } // TODO legacy restore doesn't count metadata restore as a restored file. - if (bytes_written > 0) + if (empty_file || bytes_written > 0) { // Keep track of the restored files and their sizes lock (results) From 88846251b1165c9e03218797c53b7bd0ca014c21 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Mon, 6 Jan 2025 08:14:32 +0100 Subject: [PATCH 16/19] Restoring a symlink that already exists triggers an overwrite of the file, which should count towards number of files restored. --- .../Library/Main/Operation/Restore/FileProcessor.cs | 12 ++++++------ Duplicati/Library/Main/Operation/RestoreHandler.cs | 10 +++++++--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index a79d97d38..a1d5e2aee 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -137,10 +137,10 @@ namespace Duplicati.Library.Main.Operation.Restore (bytes_written, missing_blocks) = await VerifyLocalBlocks(file, missing_blocks, blocks.Length, filehasher, blockhasher, options, results, block_request); } - bool empty_file = false; + bool empty_file_or_symlink = false; if (file.BlocksetID != LocalDatabase.SYMLINK_BLOCKSET_ID && (blocks.Length == 0 || (blocks.Length == 1 && blocks[0].BlockSize == 0))) { - empty_file = true; + empty_file_or_symlink = true; if (options.Dryrun) { Logging.Log.WriteDryrunMessage(LOGTAG, "DryrunRestore", @$"Would have created empty file ""{file.TargetPath}"""); @@ -297,11 +297,11 @@ namespace Duplicati.Library.Main.Operation.Restore } if (!options.SkipMetadata) { - await RestoreMetadata(db, file, block_request, block_response, options, sw_meta, sw_work, sw_req, sw_resp); + empty_file_or_symlink |= await RestoreMetadata(db, file, block_request, block_response, options, sw_meta, sw_work, sw_req, sw_resp); } // TODO legacy restore doesn't count metadata restore as a restored file. - if (empty_file || bytes_written > 0) + if (empty_file_or_symlink || bytes_written > 0) { // Keep track of the restored files and their sizes lock (results) @@ -424,7 +424,7 @@ namespace Duplicati.Library.Main.Operation.Restore /// The stopwatch for internal profiling of the general processing. /// The stopwatch for internal profiling of the block requests. /// The stopwatch for internal profiling of the block responses. - private static async Task RestoreMetadata(LocalRestoreDatabase db, FileRequest file, IChannel block_request, IChannel block_response, Options options, Stopwatch sw_meta, Stopwatch sw_work, Stopwatch sw_req, Stopwatch sw_resp) + private static async Task RestoreMetadata(LocalRestoreDatabase db, FileRequest file, IChannel block_request, IChannel block_response, Options options, Stopwatch sw_meta, Stopwatch sw_work, Stopwatch sw_req, Stopwatch sw_resp) { sw_work?.Stop(); sw_meta?.Start(); @@ -448,7 +448,7 @@ namespace Duplicati.Library.Main.Operation.Restore } ms.Seek(0, SeekOrigin.Begin); - RestoreHandler.ApplyMetadata(file.TargetPath, ms, options.RestorePermissions, options.RestoreSymlinkMetadata, options.Dryrun); + return RestoreHandler.ApplyMetadata(file.TargetPath, ms, options.RestorePermissions, options.RestoreSymlinkMetadata, options.Dryrun); } /// diff --git a/Duplicati/Library/Main/Operation/RestoreHandler.cs b/Duplicati/Library/Main/Operation/RestoreHandler.cs index a3cd7d3db..766b4e1a1 100644 --- a/Duplicati/Library/Main/Operation/RestoreHandler.cs +++ b/Duplicati/Library/Main/Operation/RestoreHandler.cs @@ -607,8 +607,9 @@ namespace Duplicati.Library.Main.Operation m_result.EndTime = DateTime.UtcNow; } - public static void ApplyMetadata(string path, System.IO.Stream stream, bool restorePermissions, bool restoreSymlinkMetadata, bool dryrun) + public static bool ApplyMetadata(string path, System.IO.Stream stream, bool restorePermissions, bool restoreSymlinkMetadata, bool dryrun) { + // TODO This has been modified to return a bool indicating if anything was written to properly report the number of files restored. The legacy restore doesn't check this, which produces an error in the CI where it reports that no files have been restored, even though one have. It's in Duplicati/UnitTests/SymLinkTests.cs the test SymLinkTests.SymLinkExists() that fails on the very last assert that there are 0 warnings. using (var tr = new System.IO.StreamReader(stream)) using (var jr = new Newtonsoft.Json.JsonTextReader(tr)) { @@ -616,10 +617,11 @@ namespace Duplicati.Library.Main.Operation string k; long t; System.IO.FileAttributes fa; + var wrote_something = false; // If this is dry-run, we stop after having deserialized the metadata if (dryrun) - return; + return wrote_something; var isDirTarget = path.EndsWith(DIRSEP, StringComparison.Ordinal); var targetpath = isDirTarget ? path.Substring(0, path.Length - 1) : path; @@ -637,6 +639,7 @@ namespace Duplicati.Library.Main.Operation SystemIO.IO_OS.DirectoryDelete(targetpath, false); } SystemIO.IO_OS.CreateSymlink(targetpath, k, isDirTarget); + wrote_something = true; } // If the target is a folder, make sure we create it first else if (isDirTarget && !SystemIO.IO_OS.DirectoryExists(targetpath)) @@ -646,7 +649,7 @@ namespace Duplicati.Library.Main.Operation if (!restoreSymlinkMetadata && Snapshots.SnapshotUtility.IsSymlink(SystemIO.IO_OS, targetpath)) { Logging.Log.WriteVerboseMessage(LOGTAG, "no-symlink-metadata-restored", "Not applying metadata to symlink: {0}", targetpath); - return; + return wrote_something; } if (metadata.TryGetValue("CoreLastWritetime", out k) && long.TryParse(k, out t)) @@ -669,6 +672,7 @@ namespace Duplicati.Library.Main.Operation SystemIO.IO_OS.SetFileAttributes(targetpath, fa); SystemIO.IO_OS.SetMetadata(path, metadata, restorePermissions); + return wrote_something; } } From 6f263ad874dc308b90c9a2842ea23511b1b8226f Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Mon, 6 Jan 2025 16:00:11 +0100 Subject: [PATCH 17/19] Added a note about potential memory consumption of restores FileProcessor --- Duplicati/Library/Main/Operation/Restore/FileProcessor.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 8a557a095..292b535f8 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -87,6 +87,7 @@ namespace Duplicati.Library.Main.Operation.Restore sw_file?.Stop(); // Get information about the blocks for the file + // TODO rather than keeping all of the blocks in memory, we could do a single pass over the blocks using a cursor, only keeping the relevant block requests in memory. Maybe even only a single block request at a time. sw_block?.Start(); var blocks = db.GetBlocksFromFile(file.BlocksetID).ToArray(); sw_block?.Stop(); From 46b05b94efbcc06a1e0453746fd3bf520d94d448 Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Mon, 6 Jan 2025 16:01:53 +0100 Subject: [PATCH 18/19] Limit file name conflict attempts to 100 and log an error if exceeded --- .../Library/Main/Operation/Restore/FileProcessor.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs index 292b535f8..fcb9f0266 100644 --- a/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs +++ b/Duplicati/Library/Main/Operation/Restore/FileProcessor.cs @@ -389,7 +389,8 @@ namespace Duplicati.Library.Main.Operation.Restore var newname = SystemIO.IO_OS.PathChangeExtension(request.TargetPath, null) + "." + database.RestoreTime.ToLocalTime().ToString("yyyy-MM-dd", System.Globalization.CultureInfo.InvariantCulture); var tr = newname + ext; var c = 0; - while (SystemIO.IO_OS.FileExists(tr) && c < 1000) + var max_tries = 100; + while (SystemIO.IO_OS.FileExists(tr) && c < max_tries) { try { @@ -415,6 +416,12 @@ namespace Duplicati.Library.Main.Operation.Restore tr = newname + " (" + (c++).ToString() + ")" + ext; } + if (c >= max_tries) + { + Logging.Log.WriteErrorMessage(LOGTAG, "TooManyConflicts", null, "Too many conflicts when trying to find a new name for the target file"); + throw new Exception("Too many conflicts when trying to find a new name for the target file"); + } + newname = tr; return newname; From b9a20037dbfdfc9c8aafc2bf1aa8e4a36ddd6a0e Mon Sep 17 00:00:00 2001 From: Carl Johnsen Date: Mon, 6 Jan 2025 18:58:28 +0100 Subject: [PATCH 19/19] Empty commit