check: keep pack check results, add --max-age to reuse them

[mr-raj12]

Refs #9696.

cache/checked-packs maps pack id -> (timestamp, result) and holds the results of the repository pack check. Before this branch its contents were throwaway: a completed check called tracker.clear(), which empties the table and deletes the store object, and the next check cleared it again before starting. The records existed only to let a --max-duration check resume where the previous one stopped.

That made two things impossible. A cheap cron check could not hand its findings to a later repair, so corrupt pack ids only ever appeared as log lines in whichever run happened to find them. And a check could not reuse a result that was still perfectly good, which matters on remote or cloud storage where hashing every pack takes hours.

This branch keeps the records instead and lets the user decide, at check time, how old a result may be.

Retention. Records are kept in all cases. tracker.clear() at the end of a check becomes tracker.prune(pack_ids): drop the records whose pack id is no longer listed in packs/, then store the rest. Pruning runs after the pack listing has been scanned, on both the completed and the interrupted path.

Reuse. The new borg check --max-age INTERVAL skips a pack when its recorded result is intact and younger than INTERVAL. Without --max-age (the default) every pack is verified, and the results are still recorded. Records of corrupt packs are always re-verified regardless of age, so a repair never acts on a stale failure. --max-age cannot be combined with --repair.

Partial checks. --max-duration no longer has a private progress mechanism of its own: progress now comes from the record timestamps, so it requires --max-age and errors out without it. This is a small breaking change for anyone passing bare --max-duration, and it is the one point in this branch worth your explicit sign-off. In exchange the two options compose into the behaviour you described: with --max-duration=3600 --max-age=1w, a daily one hour check verifies packs that have no result younger than a week, until the whole repository has been covered once per week.

The summary names the corrupt packs, guarded on index_errors == 0 so the ids only appear when the pack check actually ran.

No format change and no version bump. Old blobs load unchanged, and an older borg reading a newer blob sees result=0 entries it re-verifies anyway. A single store object was preferred over a separate cache/corrupt-packs, because result already encodes the distinction and two files can drift out of sync where one cannot.

docs/internals/data-structures.rst describes the retention rule and how --max-age and --max-duration interact.

Tests in repository_test.py and check_cmd_test.py cover: intact and corrupt records surviving a completed check; a plain check recording results that a later --max-age check reuses; a check without --max-age verifying every pack while keeping the records; a carried-over corrupt record being re-verified and replaced once the pack is intact; selective pruning of records whose pack is no longer listed; corrupt records surviving across consecutive partial checks; the summary naming the corrupt ids; and the --repair/--max-age and --max-duration/--max-age argument rules. black and ruff are clean.

Not in here: machine-readable output, --repair consuming the recorded list, clearing records after a successful repair. Those depend on how borg check --repair and a separate repair command end up being split, which is still open.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.77%. Comparing base (a50eb6f) to head (16fac1b).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9925      +/-   ##
==========================================
+ Coverage   85.70%   85.77%   +0.07%     
==========================================
  Files          95       95              
  Lines       16815    16842      +27     
  Branches     2577     2586       +9     
==========================================
+ Hits        14411    14447      +36     
+ Misses       1668     1662       -6     
+ Partials      736      733       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[ThomasWaldmann]

I think we could keep all entries except the ones for packs that do not exist any more.

See also #9898.

The condition for skipping a pack would then change from "it is in checked-packs AND it was OK" to "it is in checked-packs AND it was OK AND the result is recent enough".

[mr-raj12]

I think we could keep all entries except the ones for packs that do not exist any more.

See also #9898.

The condition for skipping a pack would then change from "it is in checked-packs AND it was OK" to "it is in checked-packs AND it was OK AND the result is recent enough".

Implemented as a new --max-age INTERVAL option (e.g. 4w). Skip condition is now: in checked-packs AND it was OK AND the record is younger than max_age. Intact records are kept across cycles, only records of packs no longer listed in packs/ are pruned at cycle end. Corrupt records are always re-verified.
Default is 0 (current behaviour, full check verifies every pack) - can change it to a non-zero default if you prefer.

[ThomasWaldmann]

CI: /home/runner/work/borg/borg/docs/misc/asciinema/README.rst: WARNING: document isn't included in any toctree [toc.not_included]

I'll fix that, was a collateral damage of my asciinema updates.

Update: #9938 merged, please rebase on current master.

[ThomasWaldmann]

review by claude opus 5:

Reviewed at e7676d18d (correctly rebased on current master e846a85df). I built the branch in a throwaway worktree and ran repository_test.py (77 passed) and check_cmd_test.py (22 passed, 2 skipped) — clean. Both findings below were confirmed by running probe tests against the branch.

The design is sound and matches what was asked for in the earlier comment: retention is unconditional, the skip condition became "recorded AND ok AND recent enough", pruning is by "no longer listed in packs/". The prune() placement after the for/else is correct on both paths, because pack_infos is the complete listing taken up front — an interrupted partial check still prunes safely.

Two real defects, then smaller stuff.

---

1. Partial check reports "Corrupt packs" and "no problems found", exit code 0

src/borg/repository.py:856

if index_errors == 0:  # the packs were checked, so the corrupt records are from this check

That comment is false on the --max-duration break path. A partial check that stops before reaching a pack recorded corrupt by an earlier run still emits that pack's id, while objs_errors is 0. Confirmed output from a two-pack repo where the recorded-corrupt pack sorts second and max_duration expires after the first:

INFO  Checked 1 index files (0 errors) and 1 packs (0 errors).
ERROR Corrupt packs: 6442040b644f009de79ec6352c535f22fffb254a0a4efcfb0107cd39a1b2b1bd
INFO  Finished partial repository check, no problems found.

check() returned True, so no EXIT_WARNING. This is exactly the advertised workflow (daily --max-duration=3600 --max-age=1w cron): a corruption found on Monday gets re-announced at ERROR level on every subsequent day that doesn't reach it, each time with exit code 0 and "no problems found". A cron job keyed on the exit code sees success; a human reading the log sees a contradiction.

Two coherent ways out, and I'd take the second:

• Report only what this run verified — accumulate the corrupt ids in a local list next to pack_errors instead of asking the tracker.
• Keep reporting all recorded corrupt ids, but make the wording and the outcome agree: say which ones were re-verified this run vs. carried over, and let carried-over ones count toward the failure result. A partial check that knows about an unrepaired corrupt pack arguably should keep exiting non-zero until it is repaired — that is the point of persisting the record. It does mean a partial check fails forever until repair exists, which is a maintainer call.

Either way pack_errors == 0 and corrupt_ids should not produce "no problems found".

2. A future timestamp makes a pack unverifiable forever

src/borg/repository.py:827

if max_age and time.time() - entry.timestamp < max_age:

If entry.timestamp is in the future the difference is negative, which is < max_age for any max_age, so the pack is skipped — and skipped again on every later check until wall clock catches up. timestamp is stored as "Q", so a client with a badly wrong clock (or a repo checked from several hosts, one of them skewed) can write a record that suppresses verification of that pack for years. Confirmed with a record 10 years ahead and max_age=1: the pack was not hashed.

One-line fix, treating a future record as stale:

age = time.time() - entry.timestamp
if max_age and 0 <= age < max_age:
    continue

The failure here is silent rather than noisy, which is what makes it worse than the usual clock-skew annoyances.

---

Smaller points

--max-duration now requiring --max-age. Explicit sign-off was asked for, so: the hard error looks right to me, rather than a silent behaviour change. Old semantics genuinely cannot be reproduced — the old cycle reset is gone, so a bare --max-duration would restart from pack 0 every run and never finish. Two things would soften it: make the message actionable ("--max-duration requires --max-age, e.g. --max-age=4w" — the current one leaves the user to find the docs), and consider whether defaulting --max-age to something like 4w when --max-duration is passed would be better than breaking existing cron lines outright.

--archives-only --max-age=… is silently ignored (check_cmd.py:71 — the repo check is skipped entirely). --max-duration errors out in the same situation (check_cmd.py:52). Worth the matching CommandError for consistency.

The summary hides how much was skipped. With --max-age, Checked N index files (0 errors) and M packs (0 errors) reports M < total with no hint why, and then says "no problems found" — a user could reasonably read that as a full verification. Adding the reused count, e.g. … and M packs (0 errors, K results reused), makes the reduced coverage visible.

Unbounded corrupt-id log line (repository.py:859). A badly damaged repo joins every corrupt id into one line; 10k packs is a ~650 KB log record. Cap it (first ~100 plus a count) — the full list is on record in checked-packs anyway.

Nested condition reads oddly now (repository.py:826-828). Without max_age the outer if does nothing; it's one condition:

if entry is not None and entry.result and max_age and 0 <= time.time() - entry.timestamp < max_age:
    continue  # recorded intact recently; corrupt records are always re-verified

Stale test name. test_check_full_ignores_recorded_set no longer describes what it asserts — the comment was updated, the name wasn't. Something like test_check_without_max_age_verifies_all_but_keeps_records.

Doc wording nit, docs/internals/data-structures.rst: "pruned when a check finishes scanning packs/" — pruning also happens on the interrupted path, which is correct behaviour but not what the sentence says. "when a check finishes" is enough.

Not a PR issue: docs/usage/check.rst.inc doesn't list --max-age, but that is the periodic build_usage/build_man commit, not something to hand-edit here.

[ThomasWaldmann]

For 2. add and use borg.constants.MAX_CLOCK_SKEW = 7200 # [s] - everything that is more than that in the future is considered invalid and re-checked.

corrupt-id log line: 1 id per line?

[mr-raj12]

updated!, records are kept unconditionally now, nothing drops a good entry except the pack going away from packs/.
--max-age only decides reuse: default 0 re-verifies every pack but still keeps the results, and > 0 reuses the recent-enough good ones, so the user decides what to re-check

[ThomasWaldmann]

review by claude opus 5

Re-reviewed at 74fe29434 (rebased onto 470182b94, plus the two new commits responding to the earlier review). Both earlier findings are properly fixed, and I verified the fixes. But re-reading with the reuse logic in mind turned up a worse problem than either of them, which those fixes don't touch.

The headline: --max-duration --max-age can silently stop checking part of the repo, forever

The old cycle-set had a guarantee that is now gone. On master a partial run skips whatever is in the set regardless of age, so every run advances to new packs and a full pass always completes in ceil(total / max_duration) runs. With an age window instead, if records at the head of the listing go stale before the scan reaches the tail, each run spends its budget re-verifying the head and the tail is never reached. It is not a slow-convergence case — it is a stable cycle.

Scaled-down repro of the recipe the epilog recommends (7 units of work per pass, --max-age=1w, one unit of work per run) on a repo that grew by exactly one unit, with the 8th pack corrupt. 30 consecutive daily runs:

check() results over 30 daily runs: {True}
corrupt tail pack ever verified: False
records on file: 7 of 8 packs

Every run exits 0 and logs "no problems found". The corrupt pack is never hashed once. A 3-pack version shows the cycle bare — the packs verified per run are [[0], [1], [0], [1], [0], [1]].

This matters more than the two findings in the earlier review because it is silent: those were noisy contradictions in the log, this is a check that reports success while never looking at part of the repository. And the documented configuration sits exactly on the cliff edge — --max-duration=3600 --max-age=1w for a repo needing 7 hours converges with zero margin. Any repo growth, a slower run, or one skipped cron night pushes it over, and there is no signal when that happens. The epilog's claim "until every pack has a result younger than --max-age" is precisely the property that fails.

The clean fix is to stop relying on listing order and process the least-recently-verified packs first: sort pack_infos by recorded timestamp (unrecorded first) before the loop. That restores a starvation-free guarantee independent of how max_age compares to the pass duration — each run always works on the oldest results, so coverage round-robins. It also lets the scan stop early once it reaches the first pack fresher than max_age, instead of walking the whole listing to skip. Cost is one sort of an already-in-memory list; the only visible change is progress-bar ordering.

If reordering is unwanted, the fallback is to require a margin — document that max_age must comfortably exceed the expected full-pass duration, and warn at runtime when a partial run skipped nothing but also did not finish. That is weaker; I would reorder.

On the two new commits

9d89e6060 (clock skew, --archives-only, one id per line) and 74fe29434 (fail on carried-over corrupt records, reused count) are good work — the review was read accurately rather than the symptoms patched. black and ruff clean, 101 passed / 2 skipped across repository_test.py + check_cmd_test.py.

Points worth raising anyway:

The clock-skew fix has no test. -MAX_CLOCK_SKEW <= age < max_age is the whole fix and nothing exercises the future-dated branch. Easy to add next to test_check_max_age_reverifies_stale_ok.

MAX_CLOCK_SKEW = 7200 swallows small --max-age values. With --max-age=1H, a record two hours in the future counts as recent while one 61 minutes in the past is stale. Bounding the tolerance by the window — -min(MAX_CLOCK_SKEW, max_age) <= age < max_age — keeps the asymmetry from exceeding the user's own setting.

test_check_partial_break_reports_unreached_corrupt_record does not discriminate. It asserts check() is False and the id is reported, but both hold whether or not the scan actually broke before reaching the corrupt pack — so it would pass even if the break never happened. It needs a _spy_hash assertion that the corrupt pack was not hashed. Its comment is also slightly off: the injected clock jump fires during the index verify (which goes through the same store.hash), not "once the first pack has been hashed" — it happens to still break after one pack, but for a different reason than stated.

--archives-only --max-age rejection is untested and undocumented. The code rule is in; test_check_max_age covers only the --repair and --max-duration rules, and the epilog still says only that --max-age cannot be combined with --repair.

One thing to decide deliberately

74fe29434 makes check() return False whenever any pack is recorded corrupt, not only ones verified this run. That was the recommendation and I still think it is right, but the consequence should be conscious: once a single pack is recorded corrupt, every future check exits non-zero forever, and there is no way to clear it. Re-verification only clears a record if the pack becomes intact, pruning only fires if the pack leaves packs/, and --repair for this is unimplemented (refs #8572). A user whose cron check starts failing has no supported remedy short of deleting cache/checked-packs by hand. That is defensible — it is real corruption and hiding it is worse — but it is a new permanent-failure mode arriving before the repair path that is supposed to resolve it, so it is worth a deliberate decision rather than inheriting it.

Everything else from the earlier list — reused-count in the summary, one id per line, merged skip condition, error message with an example, renamed test_check_without_max_age_verifies_all_but_keeps_records, doc wording — landed correctly.

[mr-raj12]

fixed both, future-skew tolerance is now capped at min(MAX_CLOCK_SKEW, max_age), so a small window won't accept more future skew than past age (and added a test). The permanent corrupt-pack failure is intentional and now documented in the check() docstring: it clears on re-verify, compact, or repair (#8572)