compact: stop cleanly at a pack boundary on Ctrl-C, keep the saved index valid

[mr-raj12]

Summary

borg compact invalidates the chunk index before deleting any packs, then rewrites it once at the end. If Ctrl-C hits mid-deletion, that rewrite never happens, so the next command finds no valid index and rebuilds it from scratch, scanning every pack. On a remote repository that can mean pulling the whole thing over the network.

Now a single Ctrl-C stops the pack-deletion and pack-rewrite loops at the next pack boundary. The index for whatever was already processed gets saved before the command exits with an error, so nothing downstream needs a rebuild. A second Ctrl-C still aborts immediately, same as before.

Test plan

• Added a test that interrupts compact right after the first pack delete and checks the saved index still matches the repository.
• Ran the compact test suite locally, all green.

Closes #9830

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.24%. Comparing base (6854dca) to head (2b7c127).
⚠️ Report is 4 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/borg/archiver/compact_cmd.py	85.71%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9890      +/-   ##
==========================================
- Coverage   85.24%   85.24%   -0.01%     
==========================================
  Files          93       93              
  Lines       15570    15576       +6     
  Branches     2357     2360       +3     
==========================================
+ Hits        13273    13277       +4     
- Misses       1598     1599       +1     
- Partials      699      700       +1     

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

[ThomasWaldmann]

Review by Claude Opus

The production change is correct and well-designed. I traced the consistency invariant carefully:

• Restructuring forget from a list to a per-pack {pid: set()} dict is the key enabler: each dropped pack's index entries are now removed in the same loop iteration as its store_delete, right after it succeeds. So self.chunks stays consistent with the store after every pack.
• Breaking out of the drop_packs loop leaves already-deleted packs' entries gone and not-yet-deleted packs' entries intact — consistent.
• Breaking out of the rewrite_packs loop leaves un-rewritten packs physically present with both their keep and drop entries still pointing at the valid old pack — consistent (the unused entries just get reconsidered on the next run).
• The final if sig_int: raise Error(...) after save_chunk_index() matches the existing convention exactly (create_cmd.py, prune_cmd.py) and correctly skips cleanup_files_cache().
• Even the edge case where Ctrl-C lands during the earlier archive-nuke phase is handled: nothing gets deleted, save_chunk_index() re-persists the untouched (valid) index. No consistency hole.

Style, comments, and imports are all in keeping with the surrounding code. 👍

Issue — the test does not exercise the fix (confirmed by running it)

test_compact_soft_interrupt_persists_valid_index passes, but for the wrong reason: zero packs are ever deleted, so the mid-pack-deletion path the PR fixes is never hit.

Root cause: the test triggers sig_int on the first repository.store_delete call (if len(calls) == 1). But the first store_delete is not a pack deletion. In report_and_delete(), which runs before compact_packs(), manifest.archives.nuke_by_id() calls store_delete("archives/…") to purge the soft-deleted archive. Then delete_chunkindex_from_repo deletes the index/… objects. Pack deletions come last.

I verified this empirically by replicating the test's exact trigger logic with the fix applied:

PROBE: total store_delete calls=3  first='archives/edfb98…'
PROBE: PACKS actually deleted=0

Observed store_delete order:

archives/70bf90…      ← nuke soft-deleted archive → this fires sig_int (calls==1)
index/c50717…         ← delete_chunkindex_from_repo (invalidation)
index/f7c384…
packs/…  (x6)         ← never reached: drop loop sees sig_int already True, breaks immediately

Consequences:

• The drop_packs and rewrite_packs loops both break on their first iteration, deleting 0 packs. The per-pack del self.chunks[id] consistency logic — the heart of the fix — is never executed by this test.
• assert 0 < len(calls) < len(pack_names_before) with its comment # stopped before deleting all packs is misleading: calls here is 3 (one archive + two index deletes), not a pack count. It would pass even if the pack-deletion code were completely broken.
• The final for … assert bin_to_hex(entry.pack_id) in pack_names_after can't catch a bug either — with no packs deleted, every entry trivially points at a surviving pack.

Suggested fix — gate the interrupt on actual pack deletions so it fires between packs:

def store_delete_then_interrupt(name, **kwargs):
    original_store_delete(name, **kwargs)
    if name.startswith("packs/"):        # only count/trigger on real pack deletes
        calls.append(name)
        if len(calls) == 1:              # one Ctrl-C after the first pack is deleted
            sig_int._sig_int_triggered = True

With this, the drop loop deletes exactly one pack (and removes that pack's index entries) before stopping — which is the scenario the PR is actually about. To make the assertions meaningful, also confirm the repo is genuinely in a partial state, e.g. assert 0 < len(repository.store_list("packs")) < len(pack_names_before) rather than counting store_delete calls.

Minor / non-blocking

• The logger.info(f"Deleting {deleted} unused objects…") line is printed before the loops, so on interrupt it overstates what was actually removed. Cosmetic.
• Reaching directly into sig_int._sig_int_triggered from the test couples to a private attribute, but there's precedent and no public setter, so acceptable.

Verdict

Ship the production change — it's correct and idiomatic. Please fix the test before merge: as written it green-lights without ever running the code it claims to cover, so it would not catch a regression in the pack-boundary stop logic.

_{🤖 Reviewed with Claude Code}

[mr-raj12]

Fixed the test to trigger on the first actual pack delete instead of the archive-nuke call, and assert on the real pack count. Also switched keep/drop/forget to defaultdict(set)