From a8b5e85c9b8711c7eb79434e65a2688572ea8d30 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Sat, 7 Nov 2026 01:07:09 +0100 Subject: [PATCH] All folders get their original modification time back on a restore The backup records the time of the folders with something in them, packed in the meta record, so an older version still reads the snapshot. A restore through Nextcloud leaves a folder that was already there alone. The round trip checks what Nextcloud shows, not only the disk. --- CHANGELOG.md | 3 + README.md | 5 +- lib/Job/BackupRun.php | 31 +++++- lib/Job/TreeWalker.php | 18 ++- lib/Repository/Repository.php | 32 ++++++ lib/Service/RestoreService.php | 18 +++ tests/Unit/Repository/FolderTimesTest.php | 127 ++++++++++++++++++++++ tests/integration/roundtrip.sh | 21 +++- 8 files changed, 244 insertions(+), 11 deletions(-) create mode 100644 tests/Unit/Repository/FolderTimesTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index d69f736..122fbfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ shown under 1.0.0. The git tag of a release candidate goes after the date. - A schedule per location: off, once a day, or every N hours from a given time, on the page under Backups or with `occ backup:target:schedule`. After the update every location keeps the time that held for all of them; a new location starts with the schedule of the first one. +- All folders get their original modification time back on a restore, not only the empty ones, from + the first backup made with this version. An older version of NG Backup can still read new restore + points. A restore through Nextcloud into a folder that is already there leaves its time alone. ### Changed - Locations are backed up one after the other: a backup that has to wait shows as queued. A restore diff --git a/README.md b/README.md index b8b136c..f13749e 100644 --- a/README.md +++ b/README.md @@ -293,8 +293,9 @@ On the backlog; know them before you rely on NG Backup: - **Empty folders only come back from backups made with 1.0.3 or newer.** Older restore points do not contain them. -- **Folders with files in them show the time of the restore**, not their original modification time. - The files keep theirs. On the roadmap. +- **Folders with files in them only get their original modification time back from backups made with + 1.1.0 or newer.** From older restore points they show the time of the restore; the files keep theirs. + A restore through Nextcloud into a folder that is already there leaves that folder's time alone. - **Only the data directory is backed up.** Object storage as primary storage (`objectstore` in `config.php`) is not supported: a backup refuses to start there. Folders mounted through the External storage app are not in the data directory and are **not included**, without a warning. diff --git a/lib/Job/BackupRun.php b/lib/Job/BackupRun.php index ac1ac90..fc4921a 100644 --- a/lib/Job/BackupRun.php +++ b/lib/Job/BackupRun.php @@ -52,14 +52,30 @@ final class BackupRun { $stats = $state['stats']; $walker = new TreeWalker($state['roots'], $state['exclude'], function (string $logical) use (&$state, &$stats): void { self::recordUnreadable($state, $stats, $logical); - }); + }, true); $parent = new ParentCursor($repo, $state['parent'], $state['parentPos']); $lines = []; $emptyDirs = []; + $folders = []; $finishedWalk = true; foreach ($walker->walk($state['after']) as $logical => $path) { - if (!mb_check_encoding($logical, 'UTF-8') || !Repository::isSafePath($logical)) { + $safe = mb_check_encoding($logical, 'UTF-8') && Repository::isSafePath($logical); + if ($walker->atFolderWithContent()) { + // Only its time: what is in it is reported by itself, and a bad name in the path of + // each of those is what gets reported. The cursor stays for such a name, see below. + $mtime = $safe ? @filemtime($path) : false; + if ($safe && $mtime !== false) { + $folders[] = json_encode([$logical, $mtime], JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR); + $state['after'] = $logical; + if (microtime(true) >= $deadline) { + $finishedWalk = false; + break; + } + } + continue; + } + if (!$safe) { // A name a snapshot cannot hold (a backslash, a control character, not UTF-8). One // such path must not make a backup fail, or the whole snapshot unreadable later. // The cursor stays where it is: the state is JSON, which cannot hold such a name. A @@ -164,6 +180,11 @@ final class BackupRun { $writer->write(implode("\n", $emptyDirs) . "\n"); $state['dirSegments'][] = $writer->finish(); } + if ($folders !== []) { + $writer = $repo->blobWriter($stats); + $writer->write(implode("\n", $folders) . "\n"); + $state['folderSegments'][] = $writer->finish(); + } $repo->flushPacks(); $state['parentPos'] = $parent->position(); $state['stats'] = $stats; @@ -186,10 +207,14 @@ final class BackupRun { $emptyDirs[] = json_decode($line, true, 8, JSON_THROW_ON_ERROR); } } + // The times of the other folders too, packed: every look at a snapshot's meta record reads + // this, and a string of a few hundred KiB costs that far less than tens of thousands of arrays. + $folders = isset($state['folderSegments']) ? implode("\n", iterator_to_array($repo->readLines(array_values(array_merge(...$state['folderSegments']))), false)) : ''; $repo->putSnapshot($state['snapshot'], ['time' => $state['started'], 'parent' => $state['parent'], 'label' => $state['label'], 'roots' => array_keys($state['roots']), 'stats' => $state['stats'], - 'warnings' => self::warnings($state)] + ($emptyDirs === [] ? [] : ['emptyDirs' => $emptyDirs]) + $state['meta'], + 'warnings' => self::warnings($state)] + ($emptyDirs === [] ? [] : ['emptyDirs' => $emptyDirs]) + + ($folders === '' ? [] : ['folderTimes' => base64_encode((string)gzdeflate($folders))]) + $state['meta'], $repo->readLines(array_values(array_merge(...$state['segments'])))); $state['phase'] = 'commit'; } diff --git a/lib/Job/TreeWalker.php b/lib/Job/TreeWalker.php index 25366dc..bf35c88 100644 --- a/lib/Job/TreeWalker.php +++ b/lib/Job/TreeWalker.php @@ -17,6 +17,8 @@ namespace OCA\NgBackup\Job; * and be merge-joined with the previous snapshot without holding any list in memory. */ final class TreeWalker { + private bool $atFolder = false; + /** * @param array $roots name => absolute directory, walked in this order * @param list $exclude path prefixes ("/") that are skipped entirely; @@ -27,9 +29,16 @@ final class TreeWalker { private array $exclude = [], /** Called with the logical path of a directory that cannot be listed. */ private ?\Closure $onUnreadable = null, + /** Also yield directories with something in them, right before the first thing in them. */ + private bool $withFolders = false, ) { } + /** Whether what was just yielded is a directory with something in it (see $withFolders). */ + public function atFolderWithContent(): bool { + return $this->atFolder; + } + /** @return \Generator logical path => absolute path, strictly after $after */ public function walk(?string $after = null): \Generator { foreach ($this->roots as $name => $dir) { @@ -75,7 +84,14 @@ final class TreeWalker { if (is_link($childAbs)) { continue; // never follow symlinks out of the data directory } - if (is_dir($childAbs)) { + $childIsDir = is_dir($childAbs); + if (($childIsDir || is_file($childAbs)) && $empty && !$this->isExcluded($childLogical, $childIsDir) + && $this->withFolders && $logical !== '' && ($after === null || self::compare($logical, $after) > 0)) { + $this->atFolder = true; + yield $logical => $abs; + $this->atFolder = false; + } + if ($childIsDir) { $empty = $empty && $this->isExcluded($childLogical, true); // Skip a directory entirely if everything in it sorts before the cursor. if ($after !== null && self::compare($childLogical, $after) < 0 && !self::isPrefix($childLogical, $after)) { diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 32a51be..e68b347 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -435,9 +435,41 @@ final class Repository { self::restoreEmptyDir($target, $dest, $path, $mtime); } } + // Very last, nothing is written in them any more. Also on a run that goes on after a stop, and + // the deepest first. The time is not worth stopping for: all the files are there by now. + foreach (array_reverse($this->folderTimes($snapshotId)) as [$path, $mtime]) { + if (!self::underPrefix($path, $prefix) || ($stripPrefix && $path === rtrim($prefix, '/'))) { + continue; // the target itself is not ours to change + } + if ($heartbeat !== null) { + $heartbeat(); + } + $dest = self::destination($target, $path, $prefix, $stripPrefix); + @touch(self::directoryInTarget($target, $dest, $path), $mtime); + } return $count; } + /** + * The folders with something in them that a snapshot recorded, parents before children (none + * before 1.1.0). Damage here is no reason to give up on the files of a restore. + * + * @return list path, modification time + */ + public function folderTimes(string $snapshotId): array { + $meta = $this->lastMeta[$snapshotId] ?? $this->snapshotMeta($snapshotId); + $packed = is_string($meta['folderTimes'] ?? null) ? base64_decode($meta['folderTimes'], true) : false; + $plain = $packed === false ? false : @gzinflate($packed); + $folders = []; + foreach ($plain === false ? [] : explode("\n", $plain) as $line) { + $folder = json_decode($line, true); + if (is_array($folder) && is_string($folder[0] ?? null) && self::isSafePath($folder[0]) && is_int($folder[1] ?? null)) { + $folders[] = [$folder[0], $folder[1]]; + } + } + return $folders; + } + /** * The folders without anything in them that a snapshot recorded (none before 1.0.3). * diff --git a/lib/Service/RestoreService.php b/lib/Service/RestoreService.php index 83d3153..e330887 100644 --- a/lib/Service/RestoreService.php +++ b/lib/Service/RestoreService.php @@ -155,6 +155,17 @@ final class RestoreService { $dest = $this->ensureFolder($userFolder, $parentRel); $names = $single ? [basename($rel) => $entries['']] : $entries; + // The folders with something in them that this restore creates get their time back; one that + // was already there keeps its own, as its content changes now. Noted before anything is written. + /** @var array $newFolders a folder called "2026" is an integer key */ + $newFolders = []; + foreach ($single ? [] : $repo->folderTimes($snapshotId) as [$dir, $mtime]) { + $relative = $dir === $source ? '' : (str_starts_with($dir, $source . '/') ? substr($dir, strlen($source) + 1) : null); + if ($relative !== null && ($relative === '' ? !$destExisted : !$dest->nodeExists($relative))) { + $newFolders[$relative] = $mtime; + } + } + $this->cleaned = []; $restored = 0; foreach ($names as $relPath => $entry) { @@ -190,6 +201,13 @@ final class RestoreService { if ($mode === self::MODE_REPLACE && !$single) { $trashed = $this->trashExtras($dest, '', array_flip(array_keys($names)) + $emptyDirs); } + // Last and deepest first: a change in a folder also touches the folders above it. + foreach (array_reverse($newFolders, true) as $dir => $mtime) { + $this->ensureFolder($dest, (string)$dir)->touch($mtime); + if ($progress !== null) { + $progress($restored, count($names), (string)$dir); // keeps the lease alive too + } + } return ['restored' => $restored, 'trashed' => $trashed, 'target' => $uid . ':' . $userFolder->getRelativePath($dest->getPath())]; } diff --git a/tests/Unit/Repository/FolderTimesTest.php b/tests/Unit/Repository/FolderTimesTest.php new file mode 100644 index 0000000..923ab3c --- /dev/null +++ b/tests/Unit/Repository/FolderTimesTest.php @@ -0,0 +1,127 @@ +backend = new LocalBackend($this->tempDir()); + $this->keys = KeyRing::generate(); + Repository::initWithKey($this->backend, $this->keys, $this->keys->wrap('pw')); + $this->src = $this->tempDir(); + mkdir("$this->src/data/alice/files/Old/Deep", 0700, true); + mkdir("$this->src/data/alice/files/2026", 0700, true); + file_put_contents("$this->src/data/alice/files/Old/Deep/a.txt", 'a'); + file_put_contents("$this->src/data/alice/files/Old/b.txt", 'b'); + file_put_contents("$this->src/data/alice/files/2026/c.txt", 'c'); + touch("$this->src/data/alice/files/Old/Deep", self::OLDER); + touch("$this->src/data/alice/files/Old", self::OLD); + touch("$this->src/data/alice/files/2026", self::OLD); + } + + private function repo(): Repository { + return Repository::openWithKey($this->backend, $this->keys); + } + + private function backup(float $seconds = 30.0): string { + $state = BackupRun::start(['data' => "$this->src/data"]); + for ($i = 0; $i < 500 && $state['phase'] !== 'commit'; $i++) { + $state = json_decode(json_encode(BackupRun::step($this->repo(), $state, microtime(true) + $seconds)), true); + } + $this->assertSame('commit', $state['phase']); + $this->repo()->recordSnapshot($state['snapshot']); + return $state['snapshot']; + } + + private function time(string $path): int { + clearstatcache(); + return (int)filemtime($path); + } + + public function testEachFolderIsRecordedOnceAlsoInSmallSteps(): void { + $expected = ['data', 'data/alice', 'data/alice/files', 'data/alice/files/2026', 'data/alice/files/Old', 'data/alice/files/Old/Deep']; + $this->assertSame($expected, array_column($this->repo()->folderTimes($this->backup()), 0)); + $this->assertSame($expected, array_column($this->repo()->folderTimes($this->backup(0.0)), 0), 'a step per entry'); + } + + public function testNoEmptyFolderIsAmongThem(): void { + mkdir("$this->src/data/alice/files/Hollow"); + $snapshot = $this->backup(); + $this->assertNotContains('data/alice/files/Hollow', array_column($this->repo()->folderTimes($snapshot), 0)); + $this->assertContains('data/alice/files/Hollow', array_column($this->repo()->emptyDirs($snapshot), 0)); + } + + public function testARestoreBringsThemBack(): void { + $snapshot = $this->backup(); + $target = $this->tempDir(); + $this->repo()->restore($snapshot, $target); + $this->assertSame(self::OLD, $this->time("$target/data/alice/files/Old")); + $this->assertSame(self::OLDER, $this->time("$target/data/alice/files/Old/Deep")); + $this->assertSame(self::OLD, $this->time("$target/data/alice/files/2026")); + } + + public function testARestoreThatGoesOnAfterAStopToo(): void { + $snapshot = $this->backup(); + $target = $this->tempDir(); + $this->repo()->restore($snapshot, $target); + touch("$target/data/alice/files/Old", 1800000000); + $this->repo()->restore($snapshot, $target, keepUnchanged: true); + $this->assertSame(self::OLD, $this->time("$target/data/alice/files/Old")); + } + + public function testAPrefixLeavesTheTargetAndOthersAlone(): void { + $snapshot = $this->backup(); + $target = $this->tempDir(); + $this->repo()->restore($snapshot, $target, 'data/alice/files/Old/', true); + $this->assertSame(self::OLDER, $this->time("$target/Deep")); + $this->assertNotSame(self::OLD, $this->time($target), 'the directory that was chosen is not ours to change'); + $this->assertDirectoryDoesNotExist("$target/2026"); + } + + public function testALinkOutOfTheTargetIsRefused(): void { + $snapshot = $this->backup(); + $target = $this->tempDir(); + $outside = $this->tempDir(); + touch($outside, 1800000000); + mkdir("$target/data/alice/files", 0700, true); + symlink($outside, "$target/data/alice/files/Old"); + $this->expectException(\OCA\NgBackup\Repository\RestoreTargetException::class); + $this->repo()->restore($snapshot, $target); + } + + public function testAFolderWithABadNameIsLeftOutAndWhatIsInItReported(): void { + mkdir("$this->src/data/odd\\folder"); + file_put_contents("$this->src/data/odd\\folder/x", 'x'); + $state = BackupRun::start(['data' => "$this->src/data"]); + for ($i = 0; $i < 50 && $state['phase'] !== 'commit'; $i++) { + $state = json_decode(json_encode(BackupRun::step($this->repo(), $state, 0.0), JSON_THROW_ON_ERROR), true, 512, JSON_THROW_ON_ERROR); + } + $this->assertSame('commit', $state['phase']); + $this->assertSame(1, $state['stats']['badNames']); + $this->repo()->recordSnapshot($state['snapshot']); + $this->assertNotContains("data/odd\\folder", array_column($this->repo()->folderTimes($state['snapshot']), 0)); + } + + public function testASnapshotWithoutThemHasNone(): void { + $snapshot = $this->repo()->backupDirectory($this->src)['snapshot']; + $this->assertSame([], $this->repo()->folderTimes($snapshot)); + } +} diff --git a/tests/integration/roundtrip.sh b/tests/integration/roundtrip.sh index 20d40c4..d1daecd 100755 --- a/tests/integration/roundtrip.sh +++ b/tests/integration/roundtrip.sh @@ -23,12 +23,15 @@ ok "key, kit code and confirmation" occ backup:target:add ci $TARGET_ARGS --path=repo >/dev/null || fail "target add" ok "location added" -# Test data: a user with files. +# Test data: a user with files. No skeleton files on the first login, below, in the middle of the checks. +occ config:system:set skeletondirectory --value='' >/dev/null +occ config:system:set templatedirectory --value='' >/dev/null $X sh -c "OC_PASS='Roundtrip-pw-2026' php occ user:add --password-from-env $U >/dev/null" # large.bin (40 MB) fills more than one pack: a restore to a directory reads ahead over several ranges $X sh -c "mkdir -p $DATA/$U/files/Docs/Sub $DATA/$U/files/Docs/Empty/Nested $DATA/$U/files/Hollow && touch -d @1700000000 $DATA/$U/files/Hollow && for i in 1 2 3 4 5; do head -c 300000 /dev/urandom > $DATA/$U/files/Docs/f\$i.bin; done; echo hello > $DATA/$U/files/Docs/Sub/note.txt; head -c 6000000 /dev/urandom > $DATA/$U/files/big.bin; head -c 40000000 /dev/urandom > $DATA/$U/files/large.bin" +$X sh -c "touch -d @1600000000 $DATA/$U/files/Docs/Sub $DATA/$U/files/Docs" occ files:scan "$U" >/dev/null -SUM_BEFORE=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") +SUM_BEFORE=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum") occ backup:run ci -l first >/dev/null || fail "first backup" SECOND=$(occ backup:run ci -l second) @@ -44,6 +47,14 @@ occ files:scan "$U" >/dev/null occ backup:restore ci "$SNAP" "data/$U/files/Docs" -m new-folder >/dev/null || fail "restore new-folder" $X sh -c "ls $DATA/$U/files | grep -q '^Restored '" || fail "no Restored folder" $X sh -c "test -d $DATA/$U/files/Restored\ */Docs/Empty/Nested" || fail "the empty folder is not in the Restored folder" +# Folders with files: on disk and in what Nextcloud shows, the parent as well as the child. +RDIR=$($X sh -c "cd $DATA/$U/files && ls -d Restored\ *") +[ "$($X sh -c "stat -c %Y $DATA/$U/files/Restored\ */Docs/Sub")" = 1600000000 ] || fail "a folder with files did not get its time back" +[ "$($X sh -c "stat -c %Y $DATA/$U/files/Restored\ */Docs")" = 1600000000 ] || fail "the parent folder did not get its time back" +DAV=$($X curl -sf -u "$U:Roundtrip-pw-2026" -X PROPFIND -H 'Depth: 1' "http://localhost/remote.php/dav/files/$U/$(php -r 'echo rawurlencode($argv[1]);' "$RDIR")/Docs/") || fail "PROPFIND" +[ "$(echo "$DAV" | grep -o 'Sun, 13 Sep 2020 12:26:40 GMT' | wc -l)" = 2 ] || fail "Nextcloud does not show the time of the folders: $(echo "$DAV" | grep -o '[^<]*' | sort | uniq -c)" +occ backup:restore ci "$SNAP" "data/$U/files/Docs/Sub" -m merge >/dev/null || fail "restore into a folder that is there" +[ "$($X stat -c %Y "$DATA/$U/files/Docs/Sub")" != 1600000000 ] || fail "a folder that was already there got the old time" occ backup:browse ci "$SNAP" "data/$U/files" | grep Hollow >/dev/null || fail "an empty folder is not shown when browsing the snapshot" occ backup:restore ci "$SNAP" "data/$U/files/Hollow" -m merge >/dev/null || fail "restore of a folder that was empty" $X sh -c "test -d $DATA/$U/files/Hollow" || fail "the empty folder did not come back by itself" @@ -52,8 +63,8 @@ occ backup:restore ci "$SNAP" "data/$U/files/Docs" -m replace >/dev/null || fail $X sh -c "test -d $DATA/$U/files/Docs/Empty/Nested" || fail "replace did not bring back the empty folder, or moved it to the trash" ok "empty folders: restored in each mode, by themselves and shown when browsing" $X sh -c "rm -rf '$DATA/$U/files/Restored '*" ; occ files:scan "$U" >/dev/null -SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") -[ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after restore" +SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum") +[ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after restore: $(diff <(echo "$SUM_BEFORE") <(echo "$SUM_AFTER") | cut -c1-120)" $X sh -c "ls $DATA/$U/files_trashbin/files/ | grep -q '^extra.txt'" || fail "extra file not in trash" ok "restore new-folder and replace: files identical, extra file in the trash" @@ -67,7 +78,7 @@ ok "restore of one file to a directory; the temporary of a stopped restore remov # All of the user's files to a directory: over S3 and SFTP the packs are read ahead occ backup:restore ci "$SNAP" "data/$U/files" -d /tmp/ngb-rt-dir >/dev/null || fail "restore of a folder to a directory" -SUM_DIR=$($X sh -c "cd /tmp/ngb-rt-dir/data/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum") +SUM_DIR=$($X sh -c "cd /tmp/ngb-rt-dir/data/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum") [ "$SUM_BEFORE" = "$SUM_DIR" ] || fail "the files restored to a directory differ" $X sh -c "ls -A /tmp/ngb-rt-dir | grep -q ngb-restore-spool" && fail "the read-ahead spool was left in the directory" $X sh -c "test -d /tmp/ngb-rt-dir/data/$U/files/Docs/Empty/Nested" || fail "the empty folder is not in the directory"