check: use one chunk index for the checker and the repository, refs #10364

[mr-raj12]

Description

borg check holds the chunk index in memory twice. Refs #10364, same change as #10359 does for borg compact.

Where the second index comes from:

• full check: Repository.check() loads the index from the index/ fragments and keeps it as repository.chunks. ArchiveChecker.check() then builds its own index while that one is still in memory.
• --archives-only: ArchiveChecker.check() builds its index, then the first repository.get() builds a second one through the Repository.chunks property.
• --repair with changed packs: finish() rebuilds the index from the packs while the checker's index is still in memory.

Changes in ArchiveChecker:

• check(): calls repository.flush() first. Rebuilding the index drops the entries of chunks still buffered in the pack writer, and a reused index can not resolve them in get().
• check() without --repair: uses the index Repository.check() loaded, if there is one. That index is loaded from the same index/ fragments the checker would load.
• check() otherwise: calls repository.invalidate_chunk_index() before building the index, then sets repository.chunks = self.chunks. get(), put() and delete() of the repository use this index from then on.
• check() after its own build: calls self.chunks.clear_new(). A rebuild from the packs sets F_NEW (entry not stored in the index/ fragments yet) on every entry, and Repository.close() stores all F_NEW entries as a new fragment. So a check without --repair never stores the index, also when the fragments are missing or corrupt, and a --repair that stops before finish() does not store it either.
• add_callback(): compresses and encrypts a re-chunked item metadata chunk only when --repair stores it.
• add_reference(): with --repair, only calls repository.put(), which adds the entry to the index and sets its pack location. Calling self.chunks.add() before put() on the same index fails the assert v.size == 0 or v.size == size in ChunkIndex.add(). Without --repair, it does not change the index: entries without a pack location (F_PENDING) in the repository's index make Repository.close() fail with AssertionError: chunk ... has no pack location yet.
• verify_data() with --repair: removes del self.chunks[defect_chunk]. repository.delete() already removes the entry from the index, so the del raises KeyError.
• verify_data() with --repair: marks the index invalid before the first repository.delete(). delete() rewrites the pack, and the index/ fragments point its other chunks at the old pack until finish() stores the new index. If the check stops before finish(), the next use rebuilds the index from the packs.
• check(): no longer sets repository.chunkindex_validate / chunkindex_drop_corrupt_tail. The lazy Repository.chunks build is not reached during a check.
• finish() with changed packs: calls repository.invalidate_chunk_index() and sets self.chunks = None before rebuilding the index from the packs.
• finish(): clears the invalid marker after storing the index.

Repository: the chunks setter comment lists check next to compact as a caller that installs the index it built, and the invalidate_chunk_index() docstring describes freeing the index before a rebuild.

Tests:

• test_check_holds_a_single_chunk_index, for --archives-only, full check and --repair, on a repo with a missing item metadata chunk. It asserts which index builds ArchiveChecker does, that repository.chunks is not loaded at each of them and that no index the checker built before is still alive (weakref), that repository.chunks is the checker's index in rebuild_archives(), and that the Repository.chunks property builds no index. All three cases fail on master.
• test_check_without_repair_leaves_the_chunk_index_alone: a check without --repair on the same kind of repo does not change the index/ fragments or the chunk ids in the index, with and without index/ fragments before the check.
• test_check_with_buffered_chunks: ArchiveChecker.check() after a put(), with and without --repair.
• test_check_repair_verify_data_aborted_marks_the_index_invalid: a --repair --verify-data that stops after the delete leaves the marker set, and every other chunk stays readable.
• test_check_repair_clears_the_invalid_marker: --repair on a repo with the marker set and no index/ fragments clears the marker.
• With only repository.chunks = self.chunks added to master, 28 tests in check_cmd_test.py fail, most of them in Repository.close() with the F_PENDING AssertionError above, among them test_check_format on an intact repo and the existing test_verify_data, test_missing_file_chunk and test_missing_archive_item_chunk.

Full test suite: 3082 passed, 1142 skipped.

Checklist

• PR is against master (or maintenance branch if only applicable there)
• New code has tests and docs where appropriate
• Tests pass (run tox or the relevant test subset)
• Commit messages are clean and reference related issues

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.17%. Comparing base (21caafd) to head (4d9af76).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10368      +/-   ##
==========================================
+ Coverage   88.15%   88.17%   +0.02%     
==========================================
  Files         103      103              
  Lines       18879    18890      +11     
  Branches     2917     2919       +2     
==========================================
+ Hits        16642    16656      +14     
+ Misses       1554     1551       -3     
  Partials      683      683              

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

[ThomasWaldmann]

Reviewed on the PR base 96085d51 vs head 4b6d54ca. The change does what it says, the reasoning in the description holds up under testing, and the two things that could have gone wrong — a non-repair check writing to the index, and the repair paths corrupting the now-shared index — are both handled and both covered by tests. Comments below are non-blocking.

What I verified

The memory claim, measured. Build counts alone can't show this (they're mostly the same as master), so I counted non-empty ChunkIndex objects alive after every build:

run	master: builds / alive at once	this PR: builds / alive at once
check --archives-only	2 / 2	1 / 1
check	2 / 2	2 / 1
check --verify-data	2 / 2	2 / 1
check --repair, healthy repo	2 / 2	2 / 1
check --repair, item chunk missing (packs change)	3 / 3	3 / 1

Peak is one index everywhere, including the repair-with-changed-packs case that master had at three.

"The one-liner alone is not enough". With only self.repository.chunks = self.chunks applied to the base, 28 tests in check_cmd_test.py fail (the description says 10 — an undercount, the direction is right). 24 of them die in close() with AssertionError: chunk … has no pack location yet: without repair, the old add_reference left F_PENDING entries for the re-chunked item stream in what is now the repository's index. That hits even test_check_format on a healthy repo, so the add_reference change is load-bearing.

Tests. The two new tests pass locally (local + remote kinds); check_cmd_test.py + repository/cache/compact tests: 298 passed. test_check_holds_a_single_chunk_index is well designed — repository_builds == 0 catches any accidental lazy build in the window between invalidate_chunk_index() and the assignment.

Reasoning I checked and agree with. Nothing in the checker reads entry.size (only item.size), so put() recording size 0 instead of the plaintext size changes nothing; finish() either rebuilds or zeroes sizes on write anyway. The removed del self.chunks[defect_chunk] would indeed KeyError, since delete() now removes it from the same object. And the sharing is a small correctness gain for --repair: reads now resolve through the pack-walked index rather than a fragment-built one, so a chunk that exists in a pack but not in stale fragments is readable during repair.

Comments

1. The clear_new() comment gives the wrong reason (archive.py#L2248). It says clearing F_NEW stops close() re-storing the entries "as an extra fragment" — but on the normal path finish() invalidates the index before close() runs, so close() stores nothing regardless. Where it matters is an exception before finish(): then close() would persist the entire pack-walked index as an "incremental" fragment next to the old ones. Worth saying that's the case being guarded.

2. A full check now discards an index it just loaded. Repository.check() merges the fragments and installs the result with the comment "so later reads reuse it", and the very next thing ArchiveChecker.check() does is invalidate it and merge the same fragments again (that's the 2 / 1 in the table). For a non-repair check with repository.is_chunk_index_loaded, the checker could take that index instead of building — identical content, same F_NEW state — saving one fragment merge per full check and making that comment true again. Optional; peak memory is one either way.

3. Is the slow rebuild in finish() still needed? (archive.py#L2761) Its old justification ("delete repoints that pack's other objects in the repository's index, so our offsets are stale") is gone by construction — there is only one index now and compact_pack()/put() maintain it in place. What remains is a re-validation of every object header after repair. That may well be worth keeping as belt-and-braces, but it's now a choice rather than a necessity, and it's the last double pack walk in --repair. A follow-up question, not for this PR.

4. add_reference(id_, size, cdata) — size is now unused (archive.py#L2535). It has to stay (it's the callback signature archive_put_items uses), but a one-word comment would stop the next reader from wondering.

5. Merge interaction with compact: build the chunk index once, not three times #10359. The follow-up there rewrites the Repository.chunks setter comment to list compact as an installing caller; this PR adds the checker as another one and doesn't touch that comment. Whichever lands second should mention both.

6. Description nit: "10 tests" → 28, per above.

Out of scope, noted for later

check --repair on a repo whose repository check finds a corrupt fragment still does two full pack walks in sequence (Repository.check() rebuilds and persists, then the checker rebuilds again). Pre-existing, unchanged here, peak stays one.

🤖 Review drafted with Claude Code

[ThomasWaldmann]

ping @mr-raj12

[mr-raj12]

Rebased and done: 1, 4, 6 (28 tests); 2: a check without --repair uses the index Repository.check() loaded; 5: setter comment lists check next to compact, since #10359 is merged.
3 is item 2 of #10318 (under #8466), not changed here. The double pack walk in check --repair with a corrupt index is noted on #10318.

[ThomasWaldmann]

Second review, on head 4920b1cb (base d7095f90). The review-feedback commit does what it says: a non-repair full check now takes the index Repository.check() loaded, --repair still builds from the packs, and the comments are accurate for the paths they describe. I found no correctness regression; the notes below are pre-existing gaps the PR now owns, one new API contract, and cleanups. None of them block merging.

What I verified

• check_cmd_test.py, compact_cmd_test.py, repository_test.py, cache_test.py: 308 passed, 62 skipped locally; black and ruff clean.
• The reuse branch is equivalent to the old build: on the CLI path the only producer of a loaded index before ArchiveChecker.check() is the fragments-only load in Repository.check(), which is the same fragment merge plus clear_new() the checker's slow_rebuild=False build would have done; validate/on_drop/drop_corrupt_tail only matter on the slow path, which that load can never have taken. Every case where Repository.check() does not install (invalid marker, corrupt/vanished fragment, index errors without --repair, --archives-only, Ctrl-C) falls back to the checker's build.
• Every mutation of the shared index goes to the one object (put() via PackWriter.chunks, delete()/compact_pack() defaulting to self.chunks, flush()), and nothing between finish()'s last invalidate_chunk_index() and close() touches the index (Manifest.write() is a store op, close() guards on is_chunk_index_loaded).
• Peak memory: no moment with two full indexes alive; the only overlaps are the bounded per-fragment/per-batch temporaries that predate the PR.

Findings

1. Aborted --repair --verify-data after a delete leaves the stored index stale (archive.py#L2447). delete(update_index=False) rewrites pack P to P' and repoints P's other objects only in memory; those entries are not F_NEW (after clear_new()), so if rebuild_archives raises before finish() (e.g. the "Unknown archive metadata version" raise, or a store error; Ctrl-C does reach finish()), close() persists nothing and the fragments keep pointing every kept object of P at a pack that no longer exists. Measured: 11/11 sibling entries stale, extract fails with PackNotFound, check --repository-only reports a missing pack, until another check --repair. Same on master, so pre-existing, but the new comment "finish() rebuilds the index from the rewritten packs anyway" relies on the assumption this window breaks. compact covers the same window by marking the index invalid before its first store change; check could do the same before the first delete().

2. A non-repair check still writes a pack-walked index at close() (archive.py#L2307). clear_new() runs only under if repair:. When the fragments are missing, invalid or corrupt, the non-repair build falls through to the slow pack walk, every entry is F_NEW, the index is installed, and close() stores it as a new incremental fragment (no delete_other, so a corrupt fragment survives and the next load rebuilds again). Measured on base and PR: delete index/, run check --archives-only: rc 0 and one new fragment afterwards. Not a regression, but test_check_without_repair_leaves_the_chunk_index_alone only covers intact fragments, and its docstring claims the general case. Either clear F_NEW unconditionally after the checker's own build (then a check never persists), or narrow the docstring.

3. New API precondition: no buffered puts before check() (archive.py#L2290). invalidate_chunk_index() plus rebuild drops the F_PENDING entry of a chunk still buffered in the pack writer; the next flush() raises KeyError from update_pack_info. Reproduced with r.put(k, cdata) followed by ArchiveChecker().check(r, repair=True). The reuse branch has the mirror case (verify_data iterates onto a pending entry, get() raises PackLocationUnknown, which the loop does not catch). do_check never puts before the archive check, so only direct callers can hit it; a self.repository.flush() before the invalidate, or an assertion that nothing is pending, would state the contract.

4. chunkindex_validate / chunkindex_drop_corrupt_tail are now dead during a check (archive.py#L2312). The lazy Repository.chunks build is never reached (the new test asserts repository_builds == 0, and nothing after finish()'s invalidate touches the index), so the comment describes "a rebuild of repository.chunks after invalidate_chunk_index()" that does not happen; same for the comment at repository.py#L907. Drop the assignments (their only consumer is the lazy getter) or reword as a defensive fallback.

5. Stale comment at archive.py#L2268: "self.chunks, built just below" is no longer true in the reuse branch. The invalidate_chunk_index() docstring (repository.py#L1098) also only describes the "on-disk index deleted" use, not the free-before-rebuild use this PR adds.

6. The test cannot see a second live index in finish() (archive.py#L2821). loaded_at_build records is_chunk_index_loaded, which is False after invalidate_chunk_index() whether or not the checker still holds the old index, so deleting self.chunks = None keeps the test green while the rebuild runs with two full indexes alive. A weakref to the index returned by the first patched build, asserted dead (after gc.collect(), for PyPy) when the second build is called, would pin the property the PR is about.

7. finish() branches leave self.chunks in different states (archive.py#L2837). The modified branch nulls it; the else branch writes it and then only invalidates the repository's reference, so the checker keeps a detached full index until it is collected, and invalidate_chunk_index() runs twice on the modified path. One invalidate hoisted above the if and self.chunks = None in both branches makes the invariant uniform.

8. The finish() pack walk is now a choice, not a necessity (archive.py#L2817). Deferred to check --repair: share one object header parser, stop re-validating unchanged packs #10318 as agreed; just noting that the new comment's "validating every object header" justification is thin (the first walk in check() already validated every header, and an on_drop here only sets error_found after all analysis is done), so the comment should say it is a deliberate re-validation.

9. Small things: add_callback runs repo_objs.format() (compress + encrypt) on every re-chunked item stream even without --repair, only for add_reference to discard it (pre-existing; moving the guard into the callback avoids it). The new test could use list_chunkindex_hashes() and open_repository(archiver) instead of hand-rolling both, and its setup is the same three lines as test_missing_archive_item_chunk. The setter comment "an index compact (it needs the usage flags) or check built itself" reads as two alternatives; "an index that compact (...) or check built itself" is what is meant.

🤖 Review drafted with Claude Code

[mr-raj12]

Fixed here: 1 (index marked invalid before the first delete, cleared in finish()), 2 (clear_new() after every checker build), 3 (flush() before the index is reused or rebuilt), 4, 5, 6 (weakref), 9.
7 and 8 go into the #8466 PR, which rewrites finish().

[ThomasWaldmann]

Round 3 reviewed on head 27e5985d. The three fixes do what they say, and I verified each one against the base: the unflushed-put KeyError is gone, a non-repair check on a repo without index/ fragments no longer stores one, and the aborted --repair --verify-data case that on the base left every following command dying with PackNotFound now recovers on the next use through the marker. Lint clean; check_cmd_test.py, compact_cmd_test.py, repository_test.py, cache_test.py, archive_test.py: 385 passed, 64 skipped. Revert probes (stubbing each round-3 line and re-running) show every fix has a test that fails without it, except the one noted in 4.

One new problem, then smaller things.

1. An abort inside finish()'s rebuild now loses the repaired chunks, with no marker (archive.py#L2830). finish() drops the only in-memory index before the slow rebuild, and the marker is written only on verify_data's delete path. So if the rebuild itself fails (store error, second Ctrl-C, OOM) after rebuild_archives has already stored new item/archive-metadata chunks and rewritten the archives directory, nothing records those chunks: the old fragments stay, no marker, and every repaired archive is unreadable until another check --repair. On the base the repository's index was still loaded during the rebuild and close() persisted the F_NEW put entries, so the same abort was harmless. Measured: check --repair on a repo with a missing item chunk, build_chunkindex_from_repo patched to raise when write_immediately=True: afterwards both archives' new metadata ids are absent from the index, marker False, plain check rc 1 with "Archive metadata block ... is missing!" for both. Cheapest fix: write_chunkindex_invalid(self.repository) right before the invalidate in the chunks_modified branch (idempotent if verify_data already set it; finish() clears it after the write anyway).

2. The marker write arguably belongs in Repository.delete() (repository.py#L1590). The fragments go stale inside delete(), the moment compact_pack deletes the old pack, and delete() has a second caller, borg debug delete-obj (update_index=True), with the identical window: pack rewritten, old pack deleted, then the full index write, whose own guard only covers deleting the superseded fragments after the new ones exist. Writing the marker in delete() before compact_pack (clearing it after its own full write when update_index=True; finish() keeps clearing it for update_index=False) covers both callers and removes the index_marked_invalid once-flag. Your call whether that belongs in this PR.

3. A non-repair check on a marker-set repo now deletes the index and stores nothing (archive.py#L2311). With the marker set (e.g. after an aborted repair), build_chunkindex_from_repo deletes fragments and marker (pre-existing), walks every pack, and the unconditional clear_new() discards the result, so the repository is left with no index/ at all and the next command repeats the full walk. Measured: marker + 3 fragments, borg check: rc 0, then fragments == [] and no marker; the following repo-list logs "rebuilding the chunk index ... the slow way". Base and round 2 persisted that rebuild. Correct either way, but a cost in exactly the slow-repo corner, it makes the repository check's "will be rebuilt on next use" warning misleading (the archives phase is the next use), and the marker case is not in test_check_without_repair_leaves_the_chunk_index_alone (only no-index is). Accept and say so in the comment, or add the parametrization.

4. test_check_repair_verify_data_aborted_marks_the_index_invalid: the final assertion is vacuous (check_cmd_test.py#L603). The repository.get(chunk_id) loop triggers the lazy rebuild, which clears the marker and persists a fresh index at close(), so the later check --repair never sees the marker and assert not chunkindex_is_invalid cannot fail; stubbing out finish()'s delete_chunkindex_invalid keeps this test green (test_check_repair_clears_the_invalid_marker is the one that catches it). The state the marker exists for, marker set and stale fragments present, is never fed to check --repair end to end. Reorder: assert the marker in a block with no get(), run check --repair, then assert the marker is gone and every chunk reads.

5. Docs describe the marker as guarding fragment deletion only. docs/internals/data-structures.rst (91-95), docs/internals/packs.rst (319-324), constants.py (140-144) and the write_chunkindex_invalid docstring all say it "is written before deleting index fragments and removed after the last stale fragment is gone". Round 3 adds a second meaning: fragments intact but pointing at a rewritten pack. One clause in each place, wherever the write ends up after 2.

6. chunkindex_validate / chunkindex_drop_corrupt_tail are now dead (repository.py#L910). No setter is left anywhere; the lazy getter always passes None/False. Removing the two attributes, the comment and the getter kwargs is smaller than a comment saying nobody sets them, and the borg2: avoid doing an unnecessary rebuild of the index #8476 reason for them (the checker's separate lazy index needed the same resync) is structurally satisfied by this PR.

🤖 Review drafted with Claude Code

[mr-raj12]

Round 4 is in a1151bc: items 1, 2, 4, 5 done as discussed; item 3 took your parametrization option, so test_check_without_repair_leaves_the_chunk_index_alone is now test_check_without_repair_stores_no_chunk_index (index / no-index / marker).
Item 6 is not done here, it belongs to #10378, and repository.py now has a TODO pointing at it.
Merge order: #10378 conflicts with this branch in archive.py and repository.py (it deletes the comment block round 4 reworded), so I'd prefer #10368 first, then I rebase #10378.

[mr-raj12]

Amended the round 4 commit message only (no code change), head is now 9c05196.

[ThomasWaldmann]

Round 4 reviewed on head 9c051968 (rebased onto cb39cfca; the range-diff shows context changes only). Items 1, 2, 4 and 5 are done as discussed, and I verified the important one: with an abort inside finish()'s rebuild the marker is now set, and the next check, repo-list, extract --dry-run and create all recover on their own; with the new marker write stubbed out, the round-3 failure comes back. The reordered aborted-repair test now feeds "marker set + stale fragments" into check --repair end to end. Revert probes: stubbing the marker write in delete() fails three tests, the marker clear in delete() fails the no-stored-index case, the marker write in finish() fails the new index-rebuild abort test. Lint clean; check_cmd_test.py, compact_cmd_test.py, debug_cmds_test.py, repository_test.py, cache_test.py, archive_test.py, hashindex_test.py: 430 passed, 80 skipped. The interplay with #10013 (ChunkIndex.add() keeping a resolved location, drop-on-unwind, the new close()) is fine: add_reference never re-adds a resolved id, and the unwinding path gives the same results as the tests' direct calls.

Moving the marker into delete() introduced one new problem, then smaller things.

1. A refused delete() leaves the marker set although nothing changed (repository.py#L1704). The marker is written before compact_pack(), which can raise before any store change: check_pack_objects IntegrityError, ReadRangeError from defrag, PermissionDenied on packs/ under no-delete / write-only (both allow writing cache/), or an F_PENDING id. Nothing clears the marker then, so the next command deletes every (valid) index/ fragment and rebuilds from the packs, and if that rebuild cannot succeed the repository stops working where it worked before. Measured twice independently: one pack truncated by a few bytes, repo-list and create rc 0; borg debug delete-obj <id in that pack> is refused with IntegrityError, the only store op is the marker write; the next repo-list / create / extract deletes fragments and marker, then fails with CorruptPack, and so does every later command. Plain borg check does not help, only check --repair recovers. With the marker write stubbed, repo-list and three create runs stay rc 0. Same shape with BORG_REPO_PERMISSIONS=write-only: refused delete-obj, then the next write-only create deletes all fragments and fails with PermissionDenied: 'r' required for packs/... until a full-permission client rebuilds. Only debug delete-obj can hit it (verify_data's index comes from a drop_corrupt_tail pack walk), but that is the command people are told to run on corrupt chunks.
   The fragments only go stale when the old pack is deleted, so the marker belongs right before that. transform_pack already has the idiom: a before_change callback, "called once, just before the first store modification", which repo-compress uses to invalidate the stored index. compact_pack could take the same parameter and delete() pass lambda: write_chunkindex_invalid(self). Please add the missing test with it: compact_pack raising before the store changes must leave no marker and the fragments untouched (none of the three new delete() tests covers that state).

2. A hard kill between Archives.create and finish() is still unguarded (archive.py#L2814). The fragments lack the repaired chunks from the moment Archives.create flushes the pack and switches archives/ to the new id, not only from finish(). An exception exit is safe (close() persists the F_NEW entries incrementally, measured), os._exit at the second Archives.create is not: no marker, repo-list raises DoesNotExist, check rc 1 "Archive metadata block ... is missing!", until check --repair. Same on the base, so not from this PR and no data is lost; writing the marker where add_reference first sets chunks_modified would close it. Optional here.

3. The marker docs list two writers; finish() is a third. The write_chunkindex_invalid docstring, constants.py (140-145), data-structures.rst and packs.rst name fragment deletion and Repository.delete(), but finish() writes it for another reason (the fragments lack the chunks the repair stored; no fragment deleted, no pack rewritten). Untouched comments in cache.py (926-927 "Finish the interrupted deletion", 652-653) still call the marker an interrupted deletion, and delete_chunkindex_invalid's new precondition "The index/ fragments must be complete" holds only vacuously for delete_chunkindex_from_repo, which calls it with no fragments left ("if any" fixes it). Stating the reason generically, "fragments missing entries or pointing at deleted packs", covers every writer and survives moving the write again for 1.

4. The TODO points at remove the drop_corrupt_tail mechanism, refs #10369 #10378, which keeps chunkindex_validate (repository.py#L959). As it stands, remove the drop_corrupt_tail mechanism, refs #10369 #10378 removes only chunkindex_drop_corrupt_tail and keeps chunkindex_validate plus the checker's assignment to it, so "remove them (remove the drop_corrupt_tail mechanism, refs #10369 #10378)" is only true if the rebase of remove the drop_corrupt_tail mechanism, refs #10369 #10378 drops both. Fine with me to merge this one first as you propose; then please make remove the drop_corrupt_tail mechanism, refs #10369 #10378 remove both, and use the house style for the reference (refs #NNNN / TODO(#NNNN ...)).

5. Small: test_delete_marks_the_chunk_index_invalid repeats the delete() call and the marker assert in its stored / not stored branches; build_one_pack (repository_test.py:677) plus one reopen block that optionally calls delete_chunkindex_from_repo does the same in half the lines. check_cmd_test.py lines 15-16 are two from ...cache import lines that fit on one. packs.rst 328-329 was not re-wrapped after the edit.

Item 3 of the last round (non-repair check on a marker-set repo stores nothing) is fine as pinned by the new marker parametrization.

🤖 Review drafted with Claude Code

[mr-raj12]

Round 5 (c331fa2): compact_pack takes before_change, called just before the old pack is deleted, since defrag checks permissions and reads everything before writing; new test covers overlap, truncated pack and no-delete. Marker docs are generic now, the TODO reads TODO(#10378), and the #10378 rebase will drop both attributes.
The hard kill between Archives.create and finish() also happens on master, so I'll leave that for a follow-up.

[ThomasWaldmann]

Round 5 reviewed on head c331fa20. The round-4 regression is fixed and I found nothing that blocks merging; what is left is minor and listed below.

Verified: a borg debug delete-obj that is refused on a truncated pack now performs no store operation at all, the marker stays unset, and repo-list / create keep working afterwards. The normal path orders its operations correctly (new pack stored, marker written, old pack deleted, index stored, marker cleared), and both aborted-repair scenarios still converge. All three cases of test_delete_refused_leaves_the_chunk_index_valid fail under the round-4 ordering, and with before_change ignored the four other marker tests fail, so everything is load-bearing. Your claim about defrag holds for borgstore 0.6.2: it checks permissions and reads every span before its single write. Lint clean; check_cmd_test.py, compact_cmd_test.py, debug_cmds_test.py, repo_compress_cmd_test.py, repository_test.py, cache_test.py, archive_test.py, hashindex_test.py: 442 passed, 80 skipped. Deferring the hard-kill window between Archives.create and finish() to a follow-up is fine, master has the same gap.

1. compact_pack drops the index entry before before_change can fail (repository.py#L1801). del chunks[drop_id] runs before the callback, so if the marker write raises, the in-memory index has lost an object that still exists in the untouched old pack (probe: a following get() raises ObjectNotFound in-process; after reopen everything reads fine, and on disk all is consistent: no marker, fragments unchanged, old pack intact, one orphan new pack). No current caller continues after the exception, so it is only an ordering nit: move the before_change block above the del loop.

2. The no-delete test case passes only because of borgstore's defrag permission check (repository_test.py#L477). PosixFS.defrag demands W on packs/, which no-delete lacks; nothing in compact_pack guarantees the early refusal. With the base-class defrag (only store()'s wW check), the same delete() stores the new pack, writes the marker and is then denied at store_delete: marker set plus an orphan pack. That self-heals through a rebuild, but it is the round-4 class again on any backend that allows puts and denies deletes. self.assert_writable() at the top of delete() would refuse before any store change regardless of the backend (the test then expects Repository.PermissionDenied); otherwise name the case for what it tests (defrag-denied) and note the dependency.

3. Stale test docstring (repository_test.py#L450): "delete() marks the chunk index invalid before it rewrites the pack". Since this round the marker is written after the new pack is stored, just before the old pack is deleted.

4. The rule in the write_chunkindex_invalid docstring does not fit finish() (cache.py#L624). "Store it before a store change that leaves the index/ fragments missing entries or pointing at deleted packs": finish() stores it after the put()s that left the fragments missing entries, and read literally the rule would apply to every borg create put(), which correctly writes no marker. delete_chunkindex_invalid's "must list every chunk in the packs" is also stricter than reality (gap objects and orphans from crashed backups are in packs but never in fragments). Something like "before deleting index/ fragments whose entries are stored nowhere else, or a pack the fragments point at; check --repair also stores it before replacing the index after it changed packs", and "the fragments, if any, must hold the complete current index and point only at existing packs".

5. before_change now means different timing in two sibling methods (repository.py#L1725). transform_pack documents it as "called once, just before the first store modification"; in compact_pack it runs after the new pack was stored. Passing delete()'s marker lambda to transform_pack would recreate the round-4 bug (marker written, then a failed store of the new pack leaves it set). A different name for compact_pack's parameter (before_old_pack_delete or similar) removes the trap; alternatively a follow-up moves transform_pack's call to just before its store_delete and both share one docstring.

6. Docs nits: in packs.rst the inline literal borg check --repair is split across two lines, so the rendered literal contains a line break (docutils does not warn; reflow it onto one line). In data-structures.rst (94-96), "before deleting a single object (which rewrites its pack and deletes the old one)" is slightly off since this round: the marker is written between those two steps, and not at all when the delete is refused. "before a single-object delete removes the old pack" says it exactly.

🤖 Review drafted with Claude Code

[mr-raj12]

Round 6 (4d9af76): delete() calls assert_writable() first, so it refuses before any store change on any backend; the no-delete case now expects Repository.PermissionDenied.
compact_pack's hook is renamed before_old_pack_delete and runs before the index entries are dropped. Marker docstrings and docs use your wording, and the other text fixes are in.