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 or --archives-only.

Partial checks. Progress comes from the record timestamps: a partial check verifies the least-recently-checked packs first, so repeated --max-duration runs cover the whole repository whether or not --max-age is set. Bare --max-duration keeps working as documented today, so there is no breaking change and no sign-off to make. --max-age is a pure optimization on top: with --max-duration=3600 --max-age=1w, a daily one hour check skips packs whose result is 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; a partial check verifying the least-recently-checked packs first while skipping packs younger than --max-age; the summary naming the corrupt ids; and the argument rules for --max-age (rejected with --repair and --archives-only) and --max-duration (requires --repository-only). 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

❌ Patch coverage is 89.47368% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.27%. Comparing base (d5eb3f7) to head (2ec4e87).
⚠️ Report is 45 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/borg/repository.py	86.36%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9925      +/-   ##
==========================================
+ Coverage   86.14%   86.27%   +0.12%     
==========================================
  Files          96       96              
  Lines       17326    17468     +142     
  Branches     2649     2675      +26     
==========================================
+ Hits        14925    15070     +145     
+ Misses       1663     1660       -3     
  Partials      738      738              

☔ 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)

[ThomasWaldmann]

review by claude opus 5

Reviewed at ff6bb8b9d (rebased onto 8dd7241b0, three new commits since the last pass). d46523109 and 8ec9224bf are clean responses to the previous review. ff6bb8b9d ("calendar-aware --max-age, symmetric clock-skew window") undoes a fix that was already correct and opens a new hole. The headline finding from the last review is untouched.

Tests: 107 passed / 2 skipped, black and ruff clean.

1. Starvation: still open

d46523109 and 8ec9224bf addressed the two secondary points from the last review — the skew cap and the corrupt-pack documentation. The starvation finding, which was the headline there, has no commit and no reply. There is still no ordering of pack_infos; the loop walks store_list("packs") order exactly as before.

Re-confirmed on this head. One unit of work per run, --max-age=1w, a repo needing 13 units for a full pass, 30 consecutive daily runs, tail pack corrupt:

[starvation] check() results over 30 daily runs: {True}
[starvation] corrupt tail pack ever verified: False

One note on the evidence: the 8-pack repro from the last review now passes on this head, but only by accident. The widened window below adds two hours of slack, which breaks the exact-day boundary tie in that day-granularity simulation and lets the scan advance one slot. Move the repo one unit further past the edge and the cycle re-establishes itself, as above. The 2h of slack shifts where the cliff sits; it does not remove it.

The epilog still promises "until every pack has a result younger than --max-age", which is the property that fails.

The suggested fix is unchanged: sort pack_infos by recorded timestamp (unrecorded first) before the loop, so each run works on the least-recently-verified packs and coverage round-robins regardless of how max_age compares to the pass duration.

2. ff6bb8b9d re-breaks the skew window that 8ec9224bf had just fixed

8ec9224bf implemented exactly the right thing:

if -min(MAX_CLOCK_SKEW, max_age) <= age < max_age:

ff6bb8b9d then changed it to:

skew = min(MAX_CLOCK_SKEW, max_age)
if -skew <= age <= max_age + skew:

The symmetry is the bug, not the fix. The two directions carry opposite risk. A timestamp in the future has no innocent explanation other than a skewed writer clock, so tolerating it only avoids pointless re-hashing. A timestamp older than the window is precisely what --max-age exists to catch, and "genuinely stale" cannot be distinguished from "recent but the writer's clock was behind" — so the safe resolution is to re-verify, where the cost is some hashing, rather than to skip, where the cost is data that goes unverified.

Because the tolerance is min(MAX_CLOCK_SKEW, max_age), the overshoot is proportionally worst exactly where the user asked for tight freshness. Verified on this head:

[window] --max-age=3600s, record age 7190s, re-verified: False

--max-age=1H reuses a result nearly two hours old — double the requested window. --max-age=1d accepts 26h. Only at 4w does the 2h become noise. test_check_max_age_skips_stale_within_skew locks this in as intended behaviour.

Reverting that one condition to the 8ec9224bf form fixes it, and its test should be inverted.

3. Switching to relative_time_marker_validator reopens the --max-duration guard

relative_time_marker_validator returns the raw marker string, not a timedelta, so args.max_age is truthy for any well-formed marker including zero:

'0d'  -> arg='0d'  truthy=True  max_age=0s
'0S'  -> arg='0S'  truthy=True  max_age=0s

So borg check --repository-only --max-duration=3600 --max-age=0d now passes the --max-duration requires --max-age guard, computes max_age=0, disables all reuse, and produces a partial check that restarts from pack zero on every run and never completes — the exact failure the guard was added to prevent. With the previous interval type this was caught, because timedelta(0) is falsy. --repair --max-age=0d likewise no longer errors.

The fix is to compute max_age before the argument guards and test the resolved seconds rather than the marker string.

On the calendar change itself: it is unrequested scope creep in an already large PR, but defensible — the unit set is identical (y m w d H M S), so nothing that parsed before stops parsing, and consistency with --older/--newer is a real argument. The semantic shift to note is that m now means calendar months rather than 31 days, so --max-age=12m and --max-age=1y coincide. Worth an explicit decision on whether it belongs in this PR or a separate one.

Smaller

The new docstring says a corrupt record "clears when ... compact removes the pack". It clears at the next check after compact removed it — pruning only runs inside check(). As written, a user may expect running compact to clear a failing check by itself.

Summary

The core design — unconditional retention, prune by listing, --max-age controlling reuse only — is right and has been since the second push. The concern is the last commit: it reverted a correct fix in favour of a symmetry that reads well but inverts the risk, and swapped an option's type in a way that silently reopened a guard, while the finding flagged as severe has now gone unanswered across two rounds. My suggestion is to address the starvation issue and revert or split ff6bb8b9d before adding more on top.

[ThomasWaldmann]

review by claude opus 5

Reviewed at 7171b688b (two new commits, no force-push). All three findings from the last pass are fixed, and each was verified empirically. 108 passed / 2 skipped, black and ruff clean.

No new defects this pass. There is one simplification the fix makes possible, which is the most interesting thing here.

The fixes hold up

23e2a210c is clean and complete. The skew window is back to -min(MAX_CLOCK_SKEW, max_age) <= age < max_age, and test_check_max_age_skips_stale_within_skew was correctly inverted into ..._reverifies_stale_within_skew rather than deleted. max_age is now resolved before the argument guards, with the "not allowed" guards testing args.max_age is not None and the "required" guard testing the resolved max_age — exactly the right split, and both --repair --max-age=0d and --max-duration --max-age=0d now get tests.

7171b688b fixes starvation with the sort. Re-running the repro that failed last time — 13 packs, one unit of work per run, --max-age=1w, tail pack corrupt:

[starvation] corrupt tail pack first verified on run: 12
[starvation] check() results, runs 0-14: [True x 12, False, False, False]
[coverage] 10/10 packs verified within 10 runs
[window] --max-age=3600s, record age 7190s, re-verified: True

Run 12 of a 13-pack repo is exactly one full pass — no wasted work, and the corruption is caught and then reported on every run after. The coverage probe shows strict round-robin, one distinct pack per run with no repeats.

Scoping the sort to partial is the right call: a non-partial run walks the whole listing anyway, so ordering cannot affect coverage there, and this keeps scan order unchanged for the common case.

The --max-duration requires --max-age guard is now unnecessary

Worth raising because it is the PR's only backward incompatibility — the one the description singles out as "the one point in this branch worth your explicit sign-off".

That guard was correct when progress depended on the age window: without --max-age, nothing was skipped, so every partial run restarted from pack zero. But the sort is a progress mechanism on its own. Verified packs get a fresh timestamp and sort to the back; unverified ones have no record and sort to the front. So a partial check advances round-robin whether or not a window is set:

[no-max-age] packs verified per run: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
[no-max-age] distinct packs covered in 10 runs: 10/10

That is max_age=0 — no window at all — giving perfect coverage. And it is better than the behaviour on master: instead of a cycle that completes and resets, coverage rolls continuously.

So the guard can be dropped, bare --max-duration keeps working exactly as documented today, and --max-age becomes what it should be — a pure optimization ("do not bother re-hashing what was checked recently") rather than a prerequisite. That removes the breaking change and the sign-off question with it. The epilog would need its "--max-duration requires --max-age" sentence rewritten accordingly.

My suggestion is to take this. Keeping the guard as a nudge toward the intended usage is defensible too, but then it is a deliberate UX choice rather than a technical necessity, and the PR description should stop presenting it as the latter.

Docs gap

The oldest-first ordering is the guarantee that makes partial checks work, and it appears nowhere in the user-facing docs. docs/internals/data-structures.rst still credits the window:

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.

Resumption is now the sort's doing, not --max-age's. Both that paragraph and the epilog should say that partial checks verify least-recently-checked packs first.

Minor

test_check_partial_verifies_least_recently_checked_first stores both packs with content that does not hash to their names, so it asserts ordering using two corrupt packs. The assertion is valid, but corrupt packs are never skipped, so the test cannot catch a regression where ordering and the skip window interact. Intact packs would make it a stronger test.

Non-issue, but worth recording as considered: sorting by timestamp means packs are hashed in an order unrelated to the store's directory layout, which could cost locality on a large repo. It is self-correcting — after the first pass the timestamp order is the order they were verified, which was name order, so subsequent passes preserve it. Only the first partial pass on a repo with pre-existing records is affected, and correctness outweighs it.

Where it stands

The PR is in good shape. The design has been right since the second push, all four rounds of findings are resolved, and the last two commits were accurate responses rather than approximations. What is left is the guard question above (a simplification, not a defect) and two doc sentences.