From 9779b8fff132ae327aabe3073afa684967229ca0 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Tue, 21 Jul 2026 19:48:26 +0530 Subject: [PATCH 1/3] check: keep the records of corrupt packs across check cycles Only the intact-pack records are cycle progress; the corrupt ones are kept for repair until the pack verifies intact or is gone from packs/, and their ids are now reported in the check summary. Refs #9696. --- docs/internals/data-structures.rst | 8 ++- src/borg/repository.py | 57 +++++++++++++++---- src/borg/testsuite/repository_test.py | 79 ++++++++++++++++++++++++++- 3 files changed, 128 insertions(+), 16 deletions(-) diff --git a/docs/internals/data-structures.rst b/docs/internals/data-structures.rst index 03fe44d71b..83f1cdb22f 100644 --- a/docs/internals/data-structures.rst +++ b/docs/internals/data-structures.rst @@ -36,9 +36,11 @@ config/ cache/ checked-packs - repository check progress (partial checks, full checks' checkpointing), - the set of packs checked so far this cycle (pack id -> timestamp, result), - as a hashtable with an appended integrity hash + repository check results (pack id -> timestamp, result), as a hashtable with an + appended integrity hash. Records of intact packs hold the check progress (partial + checks, full checks' checkpointing) and are dropped when a check cycle completes. + Records of corrupt packs are kept for repair until the pack verifies intact or is + no longer listed in packs/. There is a list of pointers to archive objects in this directory: diff --git a/src/borg/repository.py b/src/borg/repository.py index d8b21d6c42..03b5987b69 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -237,11 +237,14 @@ def iter_headers(self): class PackTracker: - """Packs verified in the current check cycle, mapping pack_id -> (timestamp, result). + """Pack verification results, mapping pack_id -> (timestamp, result). A cycle is one full pass over packs/; --max-duration may spread it over several partial checks. + Intact records (result=1) hold the cycle progress and are dropped when the cycle completes. + Corrupt records (result=0) are kept across cycles until the pack verifies intact or is no longer + listed in packs/. Stored at cache/checked-packs as the serialized table with a sha256 over it appended. - new() starts a cycle, load() resumes the stored one. + new() starts an empty tracker, load() reads the stored one. """ NAME = "cache/checked-packs" @@ -301,6 +304,31 @@ def get(self, pack_id): def record(self, pack_id, ok): self.table[pack_id] = self.Entry(timestamp=int(time.time()), result=int(ok)) + def corrupt_ids(self): + """Return the ids of the packs recorded corrupt, sorted.""" + return sorted(pack_id for pack_id, entry in self.table.items() if not entry.result) + + def drop_ok(self): + """Remove the intact-pack records, keeping the corrupt ones.""" + # the keys are collected first because the table must not be mutated while iterating it. + for pack_id in [pack_id for pack_id, entry in self.table.items() if entry.result]: + del self.table[pack_id] + + def finish_cycle(self, pack_ids): + """End a completed cycle: drop the intact records and the corrupt records of packs that are + gone, then store the remaining records (or delete the stored object if none remain). + + pack_ids is the set of pack ids listed in packs/ during this cycle; a record for an id not in + it refers to a pack that was deleted or compacted away. + """ + self.drop_ok() + for pack_id in [pack_id for pack_id, _ in self.table.items() if pack_id not in pack_ids]: + del self.table[pack_id] + if len(self.table): + self.save() + else: + self.clear() + def save(self): with io.BytesIO() as f: self.table.write(f) @@ -737,7 +765,8 @@ def check(self, repair=False, max_duration=0): rebuild re-reads every pack anyway - so a read-only check just stops and reports it instead of continuing. The index is never rebuilt here in any case: reading every pack to do so would be far too slow and expensive for a routine (e.g. cron) check. Salvaging good objects out of - corrupt packs and dropping those packs is left to repair, refs #8572. + corrupt packs and dropping those packs is left to repair, refs #8572. The ids of the packs + found corrupt are kept in cache/checked-packs for repair, refs #9696. """ def verify(namespace, name): @@ -761,15 +790,15 @@ def store_list(namespace): assert not (repair and partial) mode = "partial" if partial else "full" logger.info(f"Starting {mode} repository check") - if partial: - tracker = PackTracker.load(self.store) - else: - tracker = PackTracker.new(self.store) - tracker.clear() # a full check verifies every pack, so discard the stored cycle - if len(tracker): + tracker = PackTracker.load(self.store) + if not partial: + tracker.drop_ok() # a full check verifies every pack, so it starts a new cycle + if not len(tracker): + logger.info("Starting from beginning.") + elif partial: logger.info(f"Continuing check cycle, {len(tracker)} packs already checked.") else: - logger.info("Starting from beginning.") + logger.info(f"Starting from beginning, re-verifying {len(tracker)} packs recorded corrupt.") t_start = time.monotonic() t_last_checkpoint = t_start index_files = index_errors = 0 @@ -821,11 +850,11 @@ def store_list(namespace): tracker.save() break else: - # scanned all packs without hitting the time limit: the cycle is done, drop the set. + # scanned all packs without hitting the time limit: the cycle is done, drop its progress. if pack_infos: pack_pi.show(current=len(pack_infos)) # finish at 100% logger.info("Finished checking packs.") - tracker.clear() + tracker.finish_cycle({hex_to_bin(info.name) for info in pack_infos}) pack_pi.finish() else: # TODO: --repair will rebuild the index from the packs here instead of stopping (refs #8572). @@ -834,6 +863,10 @@ def store_list(namespace): logger.info( f"Checked {index_files} index files ({index_errors} errors) and {pack_files} packs ({pack_errors} errors)." ) + if index_errors == 0: # the packs were checked, so the corrupt records are from this check + corrupt_ids = tracker.corrupt_ids() + if corrupt_ids: + logger.error("Corrupt packs: " + ", ".join(bin_to_hex(pack_id) for pack_id in corrupt_ids)) if objs_errors == 0: logger.info(f"Finished {mode} repository check, no problems found.") elif repair: diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index ee8b0ef249..3425183fde 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -1,4 +1,5 @@ import io +import logging import os import sys from collections import namedtuple @@ -1116,7 +1117,83 @@ def test_check_full_ignores_recorded_set(tmp_path, monkeypatch): assert pack_key in hashed_keys # verified after = PackTracker.load(repository.store) - assert len(after) == 0 # cycle complete, set dropped + assert len(after) == 0 # cycle complete, intact records dropped + + +def _store_corrupt_pack(repository, pack_id): + # the stored content does not hash to pack_id, so verifying it fails. + repository.store_store("packs/" + bin_to_hex(pack_id), b"CORRUPT-does-not-match-name") + return pack_id + + +def test_check_full_keeps_corrupt_record_after_cycle(tmp_path): + # a completed cycle keeps the records of corrupt packs and drops the records of intact ones. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + intact_id, _ = _store_intact_pack(repository) + corrupt_id = _store_corrupt_pack(repository, H(2)) + + assert repository.check(repair=False) is False + + after = PackTracker.load(repository.store) + assert after.corrupt_ids() == [corrupt_id] + assert intact_id not in after.table + + +def test_check_full_reports_corrupt_pack_ids(tmp_path, caplog): + # the summary names the corrupt packs. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + corrupt_id = _store_corrupt_pack(repository, H(1)) + + with caplog.at_level(logging.ERROR, logger="borg.repository"): + assert repository.check(repair=False) is False + + assert f"Corrupt packs: {bin_to_hex(corrupt_id)}" in caplog.text + + +def test_check_full_reverifies_carried_over_corrupt_record(tmp_path, monkeypatch): + # a corrupt record from an earlier cycle is re-verified and dropped once the pack is intact again. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + intact_id, pack_key = _store_intact_pack(repository) + + tracker = PackTracker.new(repository.store) + tracker.record(intact_id, ok=False) # recorded corrupt in an earlier cycle + tracker.save() + + hashed_keys = _spy_hash(repository, monkeypatch) + + assert repository.check(repair=False) is True + assert pack_key in hashed_keys # re-verified + + after = PackTracker.load(repository.store) + assert len(after) == 0 # verified intact, so the corrupt record is dropped + + +def test_check_full_prunes_corrupt_record_of_vanished_pack(tmp_path): + # a corrupt record for a pack that is no longer listed is dropped at cycle end. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + _store_intact_pack(repository) + + tracker = PackTracker.new(repository.store) + tracker.record(H(9), ok=False) # no such pack in packs/ + tracker.save() + + assert repository.check(repair=False) is True + + after = PackTracker.load(repository.store) + assert len(after) == 0 + + +def test_check_partial_keeps_corrupt_record_across_runs(tmp_path): + # corrupt records survive a partial check that completes its cycle, too. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + corrupt_id = _store_corrupt_pack(repository, H(1)) + + assert repository.check(repair=False, max_duration=3600) is False + assert PackTracker.load(repository.store).corrupt_ids() == [corrupt_id] + + # a second run re-verifies it and keeps reporting it. + assert repository.check(repair=False, max_duration=3600) is False + assert PackTracker.load(repository.store).corrupt_ids() == [corrupt_id] def test_check_checked_packs_ignores_foreign_entry_layout(tmp_path): From 57f2394c2ddb80dae195b583241449a73179ff09 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 23 Jul 2026 02:14:27 +0530 Subject: [PATCH 2/3] check: add --max-age to reuse intact-pack check results across cycles --- docs/internals/data-structures.rst | 6 +- src/borg/archiver/check_cmd.py | 22 ++++- src/borg/repository.py | 35 ++++--- src/borg/testsuite/archiver/check_cmd_test.py | 19 ++++ src/borg/testsuite/repository_test.py | 94 +++++++++++++++++++ 5 files changed, 158 insertions(+), 18 deletions(-) diff --git a/docs/internals/data-structures.rst b/docs/internals/data-structures.rst index 83f1cdb22f..5b9763c373 100644 --- a/docs/internals/data-structures.rst +++ b/docs/internals/data-structures.rst @@ -39,8 +39,10 @@ cache/ repository check results (pack id -> timestamp, result), as a hashtable with an appended integrity hash. Records of intact packs hold the check progress (partial checks, full checks' checkpointing) and are dropped when a check cycle completes. - Records of corrupt packs are kept for repair until the pack verifies intact or is - no longer listed in packs/. + With ``check --max-age`` they are kept across cycles instead and reused while + younger than the given age. Records of corrupt packs are kept for repair until + the pack verifies intact or is no longer listed in packs/. Records of packs no + longer listed in packs/ are pruned when a cycle completes. There is a list of pointers to archive objects in this directory: diff --git a/src/borg/archiver/check_cmd.py b/src/borg/archiver/check_cmd.py index 1b7cf3d420..806ccf53ce 100644 --- a/src/borg/archiver/check_cmd.py +++ b/src/borg/archiver/check_cmd.py @@ -4,7 +4,7 @@ from ..archive import ArchiveChecker from ..constants import * # NOQA from ..helpers import set_ec, EXIT_WARNING, CancelledByUser, CommandError, IntegrityError -from ..helpers import yes, ArchiveFormatter +from ..helpers import interval, yes, ArchiveFormatter from ..helpers.argparsing import ArgumentParser from ..logger import create_logger @@ -44,6 +44,8 @@ def do_check(self, args, repository): raise CommandError("--repository-only contradicts the --find-lost-archives option.") if args.repair and args.max_duration: raise CommandError("--repair does not allow --max-duration argument.") + if args.repair and args.max_age: + raise CommandError("--repair does not allow the --max-age option.") if args.max_duration and not args.repo_only: # when doing a partial repo check, we can only do a low-level check of the repository files. # archives check requires that a full repo check was done before and has built/cached a ChunkIndex. @@ -64,7 +66,8 @@ def do_check(self, args, repository): # the repository check has finished, which can take hours. ArchiveFormatter.validate_format(format) if not args.archives_only: - if not repository.check(repair=args.repair, max_duration=args.max_duration): + max_age = int(args.max_age.total_seconds()) if args.max_age else 0 + if not repository.check(repair=args.repair, max_duration=args.max_duration, max_age=max_age): set_ec(EXIT_WARNING) if not args.repo_only and not archive_checker.check( repository, @@ -131,6 +134,12 @@ def build_parser_check(self, subparsers, common_parser, mid_common_parser): ``--max-duration`` you must also pass ``--repository-only``, and must not pass ``--archives-only``, nor ``--repair``. + The ``--max-age`` option keeps the results of previous repository checks and + skips packs whose intact result is younger than the given interval (e.g. + ``--max-age=4w``), spreading the verification cost over repeated checks. + Packs recorded corrupt are always re-verified. ``--max-age`` cannot be + combined with ``--repair``. + **Warning:** Please note that partial repository checks (i.e., running with ``--max-duration``) can only perform non-cryptographic checksum checks on the repository files. Enabling partial repository checks excludes archive checks @@ -219,6 +228,15 @@ def build_parser_check(self, subparsers, common_parser, mid_common_parser): subparser.add_argument( "--find-lost-archives", dest="find_lost_archives", action="store_true", help="attempt to find lost archives" ) + subparser.add_argument( + "--max-age", + metavar="INTERVAL", + dest="max_age", + type=interval, + default=None, + action=Highlander, + help="reuse intact-pack check results younger than INTERVAL (e.g. 4w)", + ) subparser.add_argument( "--max-duration", metavar="SECONDS", diff --git a/src/borg/repository.py b/src/borg/repository.py index 03b5987b69..7070f8982b 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -240,9 +240,9 @@ class PackTracker: """Pack verification results, mapping pack_id -> (timestamp, result). A cycle is one full pass over packs/; --max-duration may spread it over several partial checks. - Intact records (result=1) hold the cycle progress and are dropped when the cycle completes. - Corrupt records (result=0) are kept across cycles until the pack verifies intact or is no longer - listed in packs/. + Intact records (result=1) hold the cycle progress and are dropped when the cycle completes, + or kept across cycles when the cycle is finished with keep_ok. Corrupt records (result=0) + are kept across cycles until the pack verifies intact or is no longer listed in packs/. Stored at cache/checked-packs as the serialized table with a sha256 over it appended. new() starts an empty tracker, load() reads the stored one. """ @@ -314,14 +314,15 @@ def drop_ok(self): for pack_id in [pack_id for pack_id, entry in self.table.items() if entry.result]: del self.table[pack_id] - def finish_cycle(self, pack_ids): - """End a completed cycle: drop the intact records and the corrupt records of packs that are - gone, then store the remaining records (or delete the stored object if none remain). + def finish_cycle(self, pack_ids, keep_ok=False): + """End a completed cycle: drop the intact records (unless keep_ok) and the records of packs + that are gone, then store the remaining records (or delete the stored object if none remain). pack_ids is the set of pack ids listed in packs/ during this cycle; a record for an id not in it refers to a pack that was deleted or compacted away. """ - self.drop_ok() + if not keep_ok: + self.drop_ok() for pack_id in [pack_id for pack_id, _ in self.table.items() if pack_id not in pack_ids]: del self.table[pack_id] if len(self.table): @@ -753,7 +754,7 @@ def info(self): info = dict(id=self.id, version=self.version) return info - def check(self, repair=False, max_duration=0): + def check(self, repair=False, max_duration=0, max_age=0): """Check repository consistency. packs/ and index/ objects are named by the sha256 of their content, so a pack or index file @@ -767,6 +768,9 @@ def check(self, repair=False, max_duration=0): far too slow and expensive for a routine (e.g. cron) check. Salvaging good objects out of corrupt packs and dropping those packs is left to repair, refs #8572. The ids of the packs found corrupt are kept in cache/checked-packs for repair, refs #9696. + + max_age (seconds, 0 = off): keep the intact-pack records across cycles and skip packs whose + intact record is younger than max_age. """ def verify(namespace, name): @@ -791,12 +795,14 @@ def store_list(namespace): mode = "partial" if partial else "full" logger.info(f"Starting {mode} repository check") tracker = PackTracker.load(self.store) - if not partial: - tracker.drop_ok() # a full check verifies every pack, so it starts a new cycle + if not partial and not max_age: + tracker.drop_ok() # without max_age, a full check verifies every pack: start a new cycle if not len(tracker): logger.info("Starting from beginning.") elif partial: - logger.info(f"Continuing check cycle, {len(tracker)} packs already checked.") + logger.info(f"Continuing check cycle, {len(tracker)} pack check results on record.") + elif max_age: + logger.info(f"{len(tracker)} pack check results on record, reusing those younger than max_age.") else: logger.info(f"Starting from beginning, re-verifying {len(tracker)} packs recorded corrupt.") t_start = time.monotonic() @@ -832,8 +838,9 @@ def store_list(namespace): pack_pi.show(increase=1) # advance for skipped packs too, so the bar tracks packs/, not work done pack_id = hex_to_bin(info.name) entry = tracker.get(pack_id) - if entry is not None and entry.result: # intact in this cycle; a corrupt one is verified again - continue + if entry is not None and entry.result: # recorded intact; a corrupt one is verified again + if not max_age or time.time() - entry.timestamp < max_age: + continue pack_files += 1 ok = verify("packs", info.name) if not ok: @@ -854,7 +861,7 @@ def store_list(namespace): if pack_infos: pack_pi.show(current=len(pack_infos)) # finish at 100% logger.info("Finished checking packs.") - tracker.finish_cycle({hex_to_bin(info.name) for info in pack_infos}) + tracker.finish_cycle({hex_to_bin(info.name) for info in pack_infos}, keep_ok=bool(max_age)) pack_pi.finish() else: # TODO: --repair will rebuild the index from the packs here instead of stopping (refs #8572). diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 1e91ea84ba..3e2e4d2970 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -65,6 +65,25 @@ def test_check_usage(archivers, request): assert "archive2" in output +def test_check_max_age(archivers, request): + archiver = request.getfixturevalue(archivers) + check_cmd_setup(archiver) + + # --repair does not allow --max-age. + if archiver.FORK_DEFAULT: + cmd(archiver, "check", "--repair", "--max-age=1d", exit_code=CommandError().exit_code) + else: + with pytest.raises(CommandError): + cmd(archiver, "check", "--repair", "--max-age=1d") + + # a first check with --max-age records the results, a second one reuses them. + output = cmd(archiver, "check", "-v", "--repository-only", "--max-age=4w", exit_code=0) + assert "Starting full repository check" in output + output = cmd(archiver, "check", "-v", "--repository-only", "--max-age=4w", exit_code=0) + assert "reusing those younger than max_age" in output + assert "no problems found" in output + + def test_date_matching(archivers, request): archiver = request.getfixturevalue(archivers) check_cmd_setup(archiver) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 3425183fde..cf3191b639 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -2,6 +2,7 @@ import logging import os import sys +import time from collections import namedtuple from hashlib import sha256 @@ -1196,6 +1197,99 @@ def test_check_partial_keeps_corrupt_record_across_runs(tmp_path): assert PackTracker.load(repository.store).corrupt_ids() == [corrupt_id] +def test_check_max_age_skips_fresh_ok(tmp_path, monkeypatch): + # with max_age, a pack recorded intact recently is not re-verified, and its record is kept. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + intact_id, pack_key = _store_intact_pack(repository) + + tracker = PackTracker.new(repository.store) + tracker.record(intact_id, ok=True) # fresh timestamp + tracker.save() + + hashed_keys = _spy_hash(repository, monkeypatch) + + assert repository.check(repair=False, max_age=3600) is True + assert pack_key not in hashed_keys # skipped, its record is fresh + + after = PackTracker.load(repository.store) + assert after.table[intact_id].result == 1 # kept across the cycle + + +def test_check_max_age_reverifies_stale_ok(tmp_path, monkeypatch): + # with max_age, a pack whose intact record is older than max_age is re-verified. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + intact_id, pack_key = _store_intact_pack(repository) + + tracker = PackTracker.new(repository.store) + old_ts = int(time.time()) - 100 + tracker.table[intact_id] = PackTracker.Entry(timestamp=old_ts, result=1) + tracker.save() + + hashed_keys = _spy_hash(repository, monkeypatch) + + assert repository.check(repair=False, max_age=50) is True + assert pack_key in hashed_keys # stale, re-verified + + after = PackTracker.load(repository.store) + assert after.table[intact_id].timestamp > old_ts # record refreshed + + +def test_check_max_age_reverifies_corrupt_even_when_fresh(tmp_path, monkeypatch): + # the age window only applies to intact records: a fresh corrupt record is still re-verified. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + intact_id, pack_key = _store_intact_pack(repository) + + tracker = PackTracker.new(repository.store) + tracker.record(intact_id, ok=False) # fresh, but corrupt + tracker.save() + + hashed_keys = _spy_hash(repository, monkeypatch) + + assert repository.check(repair=False, max_age=3600) is True + assert pack_key in hashed_keys # re-verified despite the fresh record + + +def test_check_max_age_prunes_vanished_ok_record(tmp_path): + # with max_age, an intact record for a pack no longer listed is pruned at cycle end. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + intact_id, _ = _store_intact_pack(repository) + + tracker = PackTracker.new(repository.store) + tracker.record(intact_id, ok=True) + tracker.record(H(9), ok=True) # no such pack in packs/ + tracker.save() + + assert repository.check(repair=False, max_age=3600) is True + + after = PackTracker.load(repository.store) + assert intact_id in after.table + assert H(9) not in after.table + + +def test_check_max_age_partial_progress(tmp_path, monkeypatch): + # a partial check with max_age skips packs with a fresh intact record and verifies the rest. + pack_a = fchunk(b"A", chunk_id=H(1)) + pack_a_id = sha256(pack_a).digest() + pack_b = fchunk(b"BB", chunk_id=H(2)) + pack_b_id = sha256(pack_b).digest() + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + repository.store_store("packs/" + bin_to_hex(pack_a_id), pack_a) + repository.store_store("packs/" + bin_to_hex(pack_b_id), pack_b) + + tracker = PackTracker.new(repository.store) + tracker.record(pack_a_id, ok=True) # fresh + tracker.save() + + hashed_keys = _spy_hash(repository, monkeypatch) + + assert repository.check(repair=False, max_duration=3600, max_age=3600) is True + assert "packs/" + bin_to_hex(pack_a_id) not in hashed_keys + assert "packs/" + bin_to_hex(pack_b_id) in hashed_keys + + after = PackTracker.load(repository.store) + assert pack_a_id in after.table and pack_b_id in after.table + + def test_check_checked_packs_ignores_foreign_entry_layout(tmp_path): # load() drops a set whose entries have a different layout than Entry, even though its sha256 matches. OtherEntry = namedtuple("OtherEntry", "timestamp result extra") From e7676d18de2f4725d2ab41f308d2f75b9ff28b5b Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Fri, 24 Jul 2026 04:12:44 +0530 Subject: [PATCH 3/3] check: keep pack check results unconditionally, --max-age only controls their reuse Records are pruned only for packs no longer listed in packs/, so --max-duration now requires --max-age to make progress. --- docs/internals/data-structures.rst | 12 ++-- src/borg/archiver/check_cmd.py | 37 ++++++----- src/borg/repository.py | 43 ++++--------- src/borg/testsuite/archiver/check_cmd_test.py | 9 ++- src/borg/testsuite/repository_test.py | 62 ++++++++++++------- 5 files changed, 84 insertions(+), 79 deletions(-) diff --git a/docs/internals/data-structures.rst b/docs/internals/data-structures.rst index 5b9763c373..e4b8f5cc9d 100644 --- a/docs/internals/data-structures.rst +++ b/docs/internals/data-structures.rst @@ -37,12 +37,12 @@ config/ cache/ checked-packs repository check results (pack id -> timestamp, result), as a hashtable with an - appended integrity hash. Records of intact packs hold the check progress (partial - checks, full checks' checkpointing) and are dropped when a check cycle completes. - With ``check --max-age`` they are kept across cycles instead and reused while - younger than the given age. Records of corrupt packs are kept for repair until - the pack verifies intact or is no longer listed in packs/. Records of packs no - longer listed in packs/ are pruned when a cycle completes. + appended integrity hash. Records are kept across checks: ``check --max-age`` + skips packs whose intact record is younger than the given age, which also lets + partial checks (``--max-duration``) continue where a previous one stopped. + Records of corrupt packs are kept for repair and always re-verified. Records of + packs no longer listed in packs/ are pruned when a check finishes scanning + packs/. There is a list of pointers to archive objects in this directory: diff --git a/src/borg/archiver/check_cmd.py b/src/borg/archiver/check_cmd.py index 806ccf53ce..215d7a2575 100644 --- a/src/borg/archiver/check_cmd.py +++ b/src/borg/archiver/check_cmd.py @@ -46,6 +46,9 @@ def do_check(self, args, repository): raise CommandError("--repair does not allow --max-duration argument.") if args.repair and args.max_age: raise CommandError("--repair does not allow the --max-age option.") + if args.max_duration and not args.max_age: + # partial checks progress by skipping packs whose record is younger than max_age. + raise CommandError("--max-duration requires the --max-age option.") if args.max_duration and not args.repo_only: # when doing a partial repo check, we can only do a low-level check of the repository files. # archives check requires that a full repo check was done before and has built/cached a ChunkIndex. @@ -122,23 +125,25 @@ def build_parser_check(self, subparsers, common_parser, mid_common_parser): repository checks only, or pass ``--archives-only`` to run the archive checks only. - The ``--max-duration`` option can be used to split a long-running repository - check into multiple partial checks. After the given number of seconds, the check - is interrupted. The next partial check will continue where the previous one - stopped, until the full repository has been checked. Assuming a complete check - would take 7 hours, then running a daily check with ``--max-duration=3600`` - (1 hour) would result in one full repository check per week. Doing a full - repository check aborts any previous partial check; the next partial check will - restart from the beginning. With partial repository checks you can run neither - archive checks, nor enable repair mode. Consequently, if you want to use - ``--max-duration`` you must also pass ``--repository-only``, and must not pass - ``--archives-only``, nor ``--repair``. + The ``--max-age`` option makes the check reuse the results of previous + repository checks: packs whose intact result is younger than the given + interval (e.g. ``--max-age=4w``) are skipped, spreading the verification + cost over repeated checks. Check results are recorded in any case; + ``--max-age`` only controls their reuse. Packs recorded corrupt are always + re-verified. ``--max-age`` cannot be combined with ``--repair``. - The ``--max-age`` option keeps the results of previous repository checks and - skips packs whose intact result is younger than the given interval (e.g. - ``--max-age=4w``), spreading the verification cost over repeated checks. - Packs recorded corrupt are always re-verified. ``--max-age`` cannot be - combined with ``--repair``. + The ``--max-duration`` option can be used to split a long-running repository + check into multiple partial checks. After the given number of seconds, the + check is interrupted. Because a verified pack's result is recorded and + reused, ``--max-duration`` requires ``--max-age``: the next partial check + skips the recently verified packs and continues with the rest, until every + pack has a result younger than ``--max-age``. Assuming a complete check + would take 7 hours, then running a daily check with ``--max-duration=3600 + --max-age=1w`` (1 hour) would result in one full repository verification + per week. With partial repository checks you can run neither archive + checks, nor enable repair mode. Consequently, if you want to use + ``--max-duration`` you must also pass ``--repository-only``, and must not + pass ``--archives-only``, nor ``--repair``. **Warning:** Please note that partial repository checks (i.e., running with ``--max-duration``) can only perform non-cryptographic checksum checks on the diff --git a/src/borg/repository.py b/src/borg/repository.py index 7070f8982b..3829d73528 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -239,10 +239,9 @@ def iter_headers(self): class PackTracker: """Pack verification results, mapping pack_id -> (timestamp, result). - A cycle is one full pass over packs/; --max-duration may spread it over several partial checks. - Intact records (result=1) hold the cycle progress and are dropped when the cycle completes, - or kept across cycles when the cycle is finished with keep_ok. Corrupt records (result=0) - are kept across cycles until the pack verifies intact or is no longer listed in packs/. + Records are kept across checks: intact records (result=1) are reused by checks run with + max_age, corrupt records (result=0) are kept for repair and always re-verified. Records of + packs no longer listed in packs/ are pruned when a check finishes scanning packs/. Stored at cache/checked-packs as the serialized table with a sha256 over it appended. new() starts an empty tracker, load() reads the stored one. """ @@ -308,21 +307,11 @@ def corrupt_ids(self): """Return the ids of the packs recorded corrupt, sorted.""" return sorted(pack_id for pack_id, entry in self.table.items() if not entry.result) - def drop_ok(self): - """Remove the intact-pack records, keeping the corrupt ones.""" - # the keys are collected first because the table must not be mutated while iterating it. - for pack_id in [pack_id for pack_id, entry in self.table.items() if entry.result]: - del self.table[pack_id] - - def finish_cycle(self, pack_ids, keep_ok=False): - """End a completed cycle: drop the intact records (unless keep_ok) and the records of packs - that are gone, then store the remaining records (or delete the stored object if none remain). - - pack_ids is the set of pack ids listed in packs/ during this cycle; a record for an id not in - it refers to a pack that was deleted or compacted away. + def prune(self, pack_ids): + """Drop the records whose pack id is not in pack_ids (the set of pack ids listed in packs/), + then store the remaining records (or delete the stored object if none remain). """ - if not keep_ok: - self.drop_ok() + # the keys are collected first because the table must not be mutated while iterating it. for pack_id in [pack_id for pack_id, _ in self.table.items() if pack_id not in pack_ids]: del self.table[pack_id] if len(self.table): @@ -769,8 +758,8 @@ def check(self, repair=False, max_duration=0, max_age=0): corrupt packs and dropping those packs is left to repair, refs #8572. The ids of the packs found corrupt are kept in cache/checked-packs for repair, refs #9696. - max_age (seconds, 0 = off): keep the intact-pack records across cycles and skip packs whose - intact record is younger than max_age. + max_age (seconds, 0 = verify every pack): skip packs whose intact record is younger than + max_age. Check results are always recorded and kept. """ def verify(namespace, name): @@ -795,16 +784,12 @@ def store_list(namespace): mode = "partial" if partial else "full" logger.info(f"Starting {mode} repository check") tracker = PackTracker.load(self.store) - if not partial and not max_age: - tracker.drop_ok() # without max_age, a full check verifies every pack: start a new cycle if not len(tracker): logger.info("Starting from beginning.") - elif partial: - logger.info(f"Continuing check cycle, {len(tracker)} pack check results on record.") elif max_age: logger.info(f"{len(tracker)} pack check results on record, reusing those younger than max_age.") else: - logger.info(f"Starting from beginning, re-verifying {len(tracker)} packs recorded corrupt.") + logger.info(f"{len(tracker)} pack check results on record, verifying every pack.") t_start = time.monotonic() t_last_checkpoint = t_start index_files = index_errors = 0 @@ -839,7 +824,7 @@ def store_list(namespace): pack_id = hex_to_bin(info.name) entry = tracker.get(pack_id) if entry is not None and entry.result: # recorded intact; a corrupt one is verified again - if not max_age or time.time() - entry.timestamp < max_age: + if max_age and time.time() - entry.timestamp < max_age: continue pack_files += 1 ok = verify("packs", info.name) @@ -853,15 +838,13 @@ def store_list(namespace): logger.info(f"Checkpointing at pack {info.name}.") tracker.save() if partial and now > t_start + max_duration: - logger.info(f"Finished partial repository check, {len(tracker)} packs checked so far.") - tracker.save() + logger.info(f"Finished partial repository check, {len(tracker)} pack check results on record.") break else: - # scanned all packs without hitting the time limit: the cycle is done, drop its progress. if pack_infos: pack_pi.show(current=len(pack_infos)) # finish at 100% logger.info("Finished checking packs.") - tracker.finish_cycle({hex_to_bin(info.name) for info in pack_infos}, keep_ok=bool(max_age)) + tracker.prune({hex_to_bin(info.name) for info in pack_infos}) pack_pi.finish() else: # TODO: --repair will rebuild the index from the packs here instead of stopping (refs #8572). diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index 3e2e4d2970..d3fc8500fe 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -69,15 +69,18 @@ def test_check_max_age(archivers, request): archiver = request.getfixturevalue(archivers) check_cmd_setup(archiver) - # --repair does not allow --max-age. + # --repair does not allow --max-age, and --max-duration requires --max-age. if archiver.FORK_DEFAULT: cmd(archiver, "check", "--repair", "--max-age=1d", exit_code=CommandError().exit_code) + cmd(archiver, "check", "--repository-only", "--max-duration=3600", exit_code=CommandError().exit_code) else: with pytest.raises(CommandError): cmd(archiver, "check", "--repair", "--max-age=1d") + with pytest.raises(CommandError): + cmd(archiver, "check", "--repository-only", "--max-duration=3600") - # a first check with --max-age records the results, a second one reuses them. - output = cmd(archiver, "check", "-v", "--repository-only", "--max-age=4w", exit_code=0) + # a check records its results, a later one with --max-age reuses them. + output = cmd(archiver, "check", "-v", "--repository-only", exit_code=0) assert "Starting full repository check" in output output = cmd(archiver, "check", "-v", "--repository-only", "--max-age=4w", exit_code=0) assert "reusing those younger than max_age" in output diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index cf3191b639..ec803b08ae 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -1026,7 +1026,7 @@ def test_check_partial_rechecks_pack_sorting_before_checked_one(tmp_path): with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: repository.store_store("packs/" + bin_to_hex(intact_id), intact) - # mark the intact pack as already checked in this cycle. + # mark the intact pack as recently checked. tracker = PackTracker.new(repository.store) tracker.record(intact_id, ok=True) tracker.save() @@ -1036,11 +1036,11 @@ def test_check_partial_rechecks_pack_sorting_before_checked_one(tmp_path): assert bin_to_hex(early_id) < bin_to_hex(intact_id) repository.store_store("packs/" + bin_to_hex(early_id), b"CORRUPT-does-not-match-name") - assert repository.check(repair=False, max_duration=3600) is False + assert repository.check(repair=False, max_duration=3600, max_age=3600) is False def test_check_partial_rechecks_pack_recorded_corrupt(tmp_path): - # a pack recorded corrupt earlier in the cycle is re-verified, so the corruption keeps being reported. + # a pack recorded corrupt earlier is re-verified, so the corruption keeps being reported. with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: corrupt_id = H(1) # stored content does not hash to this name repository.store_store("packs/" + bin_to_hex(corrupt_id), b"CORRUPT-does-not-match-name") @@ -1049,7 +1049,7 @@ def test_check_partial_rechecks_pack_recorded_corrupt(tmp_path): tracker.record(corrupt_id, ok=False) tracker.save() - assert repository.check(repair=False, max_duration=3600) is False + assert repository.check(repair=False, max_duration=3600, max_age=3600) is False def _spy_hash(repository, monkeypatch): @@ -1084,12 +1084,12 @@ def test_check_partial_clears_recorded_corruption_when_intact(tmp_path, monkeypa hashed_keys = _spy_hash(repository, monkeypatch) - assert repository.check(repair=False, max_duration=3600) is True + assert repository.check(repair=False, max_duration=3600, max_age=3600) is True assert pack_key in hashed_keys # re-verified def test_check_partial_skips_pack_recorded_intact(tmp_path, monkeypatch): - # a pack recorded intact in this cycle is skipped when a partial check resumes. + # a pack recorded intact recently is skipped when a partial check resumes. with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: intact_id, pack_key = _store_intact_pack(repository) @@ -1099,12 +1099,12 @@ def test_check_partial_skips_pack_recorded_intact(tmp_path, monkeypatch): hashed_keys = _spy_hash(repository, monkeypatch) - assert repository.check(repair=False, max_duration=3600) is True + assert repository.check(repair=False, max_duration=3600, max_age=3600) is True assert pack_key not in hashed_keys # skipped, not re-verified def test_check_full_ignores_recorded_set(tmp_path, monkeypatch): - # a full check verifies every pack regardless of the recorded set, then drops the set. + # without max_age, a check verifies every pack regardless of the recorded set, but keeps the records. with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: intact_id, pack_key = _store_intact_pack(repository) @@ -1115,10 +1115,10 @@ def test_check_full_ignores_recorded_set(tmp_path, monkeypatch): hashed_keys = _spy_hash(repository, monkeypatch) assert repository.check(repair=False) is True - assert pack_key in hashed_keys # verified + assert pack_key in hashed_keys # verified despite the fresh intact record after = PackTracker.load(repository.store) - assert len(after) == 0 # cycle complete, intact records dropped + assert after.table[intact_id].result == 1 # record kept def _store_corrupt_pack(repository, pack_id): @@ -1127,8 +1127,8 @@ def _store_corrupt_pack(repository, pack_id): return pack_id -def test_check_full_keeps_corrupt_record_after_cycle(tmp_path): - # a completed cycle keeps the records of corrupt packs and drops the records of intact ones. +def test_check_full_keeps_records_after_check(tmp_path): + # a completed check keeps the records of both corrupt and intact packs. with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: intact_id, _ = _store_intact_pack(repository) corrupt_id = _store_corrupt_pack(repository, H(2)) @@ -1137,7 +1137,7 @@ def test_check_full_keeps_corrupt_record_after_cycle(tmp_path): after = PackTracker.load(repository.store) assert after.corrupt_ids() == [corrupt_id] - assert intact_id not in after.table + assert after.table[intact_id].result == 1 def test_check_full_reports_corrupt_pack_ids(tmp_path, caplog): @@ -1152,12 +1152,12 @@ def test_check_full_reports_corrupt_pack_ids(tmp_path, caplog): def test_check_full_reverifies_carried_over_corrupt_record(tmp_path, monkeypatch): - # a corrupt record from an earlier cycle is re-verified and dropped once the pack is intact again. + # a corrupt record from an earlier check is re-verified; an intact result replaces it. with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: intact_id, pack_key = _store_intact_pack(repository) tracker = PackTracker.new(repository.store) - tracker.record(intact_id, ok=False) # recorded corrupt in an earlier cycle + tracker.record(intact_id, ok=False) # recorded corrupt in an earlier check tracker.save() hashed_keys = _spy_hash(repository, monkeypatch) @@ -1166,13 +1166,13 @@ def test_check_full_reverifies_carried_over_corrupt_record(tmp_path, monkeypatch assert pack_key in hashed_keys # re-verified after = PackTracker.load(repository.store) - assert len(after) == 0 # verified intact, so the corrupt record is dropped + assert after.table[intact_id].result == 1 # verified intact, corrupt record replaced def test_check_full_prunes_corrupt_record_of_vanished_pack(tmp_path): - # a corrupt record for a pack that is no longer listed is dropped at cycle end. + # a corrupt record for a pack that is no longer listed is dropped when the check finishes. with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: - _store_intact_pack(repository) + intact_id, _ = _store_intact_pack(repository) tracker = PackTracker.new(repository.store) tracker.record(H(9), ok=False) # no such pack in packs/ @@ -1181,19 +1181,20 @@ def test_check_full_prunes_corrupt_record_of_vanished_pack(tmp_path): assert repository.check(repair=False) is True after = PackTracker.load(repository.store) - assert len(after) == 0 + assert H(9) not in after.table + assert intact_id in after.table def test_check_partial_keeps_corrupt_record_across_runs(tmp_path): - # corrupt records survive a partial check that completes its cycle, too. + # corrupt records survive completed partial checks, too. with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: corrupt_id = _store_corrupt_pack(repository, H(1)) - assert repository.check(repair=False, max_duration=3600) is False + assert repository.check(repair=False, max_duration=3600, max_age=3600) is False assert PackTracker.load(repository.store).corrupt_ids() == [corrupt_id] # a second run re-verifies it and keeps reporting it. - assert repository.check(repair=False, max_duration=3600) is False + assert repository.check(repair=False, max_duration=3600, max_age=3600) is False assert PackTracker.load(repository.store).corrupt_ids() == [corrupt_id] @@ -1212,7 +1213,7 @@ def test_check_max_age_skips_fresh_ok(tmp_path, monkeypatch): assert pack_key not in hashed_keys # skipped, its record is fresh after = PackTracker.load(repository.store) - assert after.table[intact_id].result == 1 # kept across the cycle + assert after.table[intact_id].result == 1 # record kept def test_check_max_age_reverifies_stale_ok(tmp_path, monkeypatch): @@ -1250,7 +1251,7 @@ def test_check_max_age_reverifies_corrupt_even_when_fresh(tmp_path, monkeypatch) def test_check_max_age_prunes_vanished_ok_record(tmp_path): - # with max_age, an intact record for a pack no longer listed is pruned at cycle end. + # an intact record for a pack no longer listed is pruned when the check finishes. with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: intact_id, _ = _store_intact_pack(repository) @@ -1290,6 +1291,19 @@ def test_check_max_age_partial_progress(tmp_path, monkeypatch): assert pack_a_id in after.table and pack_b_id in after.table +def test_check_max_age_reuses_records_of_plain_check(tmp_path, monkeypatch): + # a check without max_age records its results, a later check with max_age reuses them. + with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository: + intact_id, pack_key = _store_intact_pack(repository) + + assert repository.check(repair=False) is True + + hashed_keys = _spy_hash(repository, monkeypatch) + + assert repository.check(repair=False, max_age=3600) is True + assert pack_key not in hashed_keys # skipped, reusing the plain check's record + + def test_check_checked_packs_ignores_foreign_entry_layout(tmp_path): # load() drops a set whose entries have a different layout than Entry, even though its sha256 matches. OtherEntry = namedtuple("OtherEntry", "timestamp result extra")