Codecov Report

❌ Patch coverage is 84.21053% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.34%. Comparing base (7da670c) to head (a1b88e2).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##           master    #9909      +/-   ##
==========================================
+ Coverage   85.13%   85.34%   +0.21%     
==========================================
  Files          93       93              
  Lines       15899    15932      +33     
  Branches     2428     2429       +1     
==========================================
+ Hits        13535    13597      +62     
+ Misses       1654     1631      -23     
+ Partials      710      704       -6     

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

Also: does it handle Ctrl-C correctly?

Yes, store.store() writes atomically and appends a sha256 that's verified on load, so an interrupt can't corrupt the set or mark a pack checked when it wasn't. The worst case you redo a last one minute of work, capped at 1 min now that checkpoints run every 60s.

Docs not updated — data-structures.rst:34 still documents last-pack-checked ("key of last pack checked as text"). It should describe checked-packs instead.

Checkpoint cost grows with repo size (note, not a blocker). The old checkpoint wrote a ~70-byte marker; the new one rewrites the whole table (41 bytes/pack) every 5 minutes — ~4 MB per checkpoint at 100k packs. Fine in practice, just no longer O(1).

Guess we should reduce checkpoint frequency: every 30 mins.

Re-reviewed the updated branch. All 62 repository_test.py tests pass locally, black --check is clean, and I verified the resume semantics directly (see test suggestions below): a pack recorded intact is genuinely skipped on resume, and a full check ignores the recorded set, re-verifies everything, and drops the set.

The update resolves what I'd consider required: the docs are fixed (and better than asked — the old entry was misdocumented under config/, now correctly a cache/ section), and the PackTracker refactor is a clean improvement: state and serialization in one place, check() reads much better, and the class placement and Entry/EntryFormatT pattern match existing conventions (ChunkIndexEntryFormatT in hashindex.pyx).

Remaining minor points, none blocking:

clear() is a mild footgun. It deletes the stored blob but leaves the in-memory table populated. Harmless in today's two call sites (tracker fresh or immediately discarded), but a future caller doing tracker.clear(); tracker.is_intact(...) would get stale answers. Either add self.table.clear() or rename to something like drop_saved() so the name says what it does.

Old marker cleanup. A repo that ran partial checks under older borg keeps its cache/last-pack-checked object forever; nothing deletes it now. A one-line store.delete("cache/last-pack-checked") (ignoring StoreObjectNotFound) in PackTracker.clear() would retire it.

Schema guard on load. If a future borg changes Entry's fields, a valid old blob still loads fine and entry.result then raises AttributeError mid-check — the sha256 protects against rot, not schema evolution. Cheap guard after HashTableNT.read: discard the table unless key_size == 32 and value_type._fields == Entry._fields.

Test gap: the skip itself is untested. The four tests prove the re-check cases, but none proves that an intact-recorded pack is skipped (the point of the mechanism), nor that a full check ignores the set. Both pass on your branch; feel free to take these:

def _spy_hash(repository, monkeypatch):
    hashed_keys = []
    orig_hash = repository.store.hash

    def spy(key):
        hashed_keys.append(key)
        return orig_hash(key)

    monkeypatch.setattr(repository.store, "hash", spy)
    return hashed_keys


def test_check_partial_skips_pack_recorded_intact(tmp_path, monkeypatch):
    # a pack recorded intact this cycle is skipped on resume, not re-verified.
    intact = fchunk(b"INTACT", chunk_id=H(1))
    intact_id = sha256(intact).digest()
    with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository:
        pack_key = "packs/" + bin_to_hex(intact_id)
        repository.store_store(pack_key, intact)

        tracker = PackTracker(repository.store)
        tracker.record(intact_id, ok=True)
        tracker.save()

        hashed_keys = _spy_hash(repository, monkeypatch)
        assert repository.check(repair=False, max_duration=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 re-verifies everything even if a partial cycle recorded the pack intact,
    # and drops the set once the cycle is complete.
    intact = fchunk(b"INTACT", chunk_id=H(1))
    intact_id = sha256(intact).digest()
    with Repository(str(tmp_path / "repo"), exclusive=True, create=True) as repository:
        pack_key = "packs/" + bin_to_hex(intact_id)
        repository.store_store(pack_key, intact)

        tracker = PackTracker(repository.store)
        tracker.record(intact_id, ok=True)
        tracker.save()

        hashed_keys = _spy_hash(repository, monkeypatch)
        assert repository.check(repair=False) is True
        assert pack_key in hashed_keys  # full check verified it
        after = PackTracker(repository.store)
        after.load()
        assert len(after) == 0  # set dropped after complete cycle

Tiny wording nits: the docs say "pack key -> timestamp, result" but it's the 32-byte binary pack id ("key" elsewhere in that doc means the store key like packs/<hex>); the PR body says the old checkpoint interval was "every minute" (it was 5 minutes); and the last commit message says "reduce checkpoint interval" where it increases the interval (reduces the frequency).