compact: add all-packs reclaim gate and tiny-pack merging

[mr-raj12]

Problem

1. Any compaction rewrites the whole chunk index and invalidates every client's cached copy (borg2: compact always invalidates the whole index cache of clients #9817). The old policy decided per pack ("rewrite when unused bytes reach the threshold"), with no floor on whether the space reclaimed across the whole repository justified that cost. A single small wasteful pack could trigger a full index rewrite for very little gain.

2. Incremental backups leave one tiny, fully-used pack per run behind. Those packs have no unused bytes, so the per-pack policy never touched them and repositories accumulated thousands of small packs with no way to consolidate them.

Solution

All-packs reclaim gate

Drop or rewrite packs only when the space they would free reaches --threshold divided by 5 percent (2% at the default 10% threshold) of the total pack bytes. Below that floor the repository and the chunk index are left untouched, so client caches stay valid when the savings do not justify the cost. --threshold 0 disables the gate.

Tiny-pack merging

Collect fully-used packs smaller than the tiny limit, which is min(MIN_PACK_SIZE, pack_max_size // 2) (MIN_PACK_SIZE is 1 MB). Merge them only once their combined size reaches one full pack (pack_max_size), so every merge produces at least one full-size pack that can never be a merge candidate again. Capping the limit at half the max pack size keeps that property even when BORG_PACK_MAX_SIZE is set below 2 MB.

The merge copies whole pack files through store.defrag (content-addressed sha256 names), so bytes that no index entry covers (a chunk copy superseded by a later put, or objects from a backup that crashed before writing its index) are carried into the merged pack rather than dropped. Before writing anything, each source pack's index entries are checked for overlap or for claiming bytes past the pack's actual end; either means a corrupt index, and the merge raises IntegrityError and leaves the store untouched.

Packs are merged one batch at a time. Each batch's source packs are deleted only after its merged pack is stored and indexed, so a crash or Ctrl-C between batches never destroys the only stored copy of an object, and the store holds at most one batch of extra packs at a time. The merge reports progress per batch and stops cleanly at a batch boundary on Ctrl-C; the packs not yet merged are merged on the next run.

Index rewrite skip

Skip the full write_chunkindex_to_repo when the store did not change, so an unconditional index rewrite cannot defeat the all-packs gate.

Refuse up front on a restricted repository

borg compact now checks that the repository permissions allow write and delete on packs/ and index/ before doing any work, and raises a clear error instead of silently reaching a no-op or failing partway. A dry run only reads and reports, so it is still allowed on a read-only repository.

Testing

New and updated tests:

• test_compact_packs_merges_tiny_packs: tiny packs merge into fewer, larger packs, every object still reads back, and a second borg compact finds nothing to merge (guards against re-merging a pack it just produced).
• test_compact_packs_below_merge_size_gate_leaves_tiny_packs: merging does not fire on pack count alone, only once the combined size could fill a full pack.
• test_compact_packs_below_all_packs_gate_changes_nothing: a tiny unused pack beside a large used one is left untouched when the reclaim is below the gate.
• test_merge_packs_combines_whole_files, test_merge_packs_carries_unindexed_bytes_forward, test_merge_packs_detects_past_eof, test_merge_packs_skips_pack_already_gone: cover the merge path, the unindexed-bytes carry-forward, corrupt-index refusal, and a stale index entry pointing at a pack file already gone.
• test_assert_writable: covers the write and delete permission checks on packs/ and index/.
• The restricted-permissions tests assert borg compact refuses up front under no-delete, read-only, and write-only, and that a dry run is still allowed where reads are permitted.

Closes #9816

[codecov]

Codecov Report

❌ Patch coverage is 86.55462% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.24%. Comparing base (bc3f14d) to head (52d7ca0).
⚠️ Report is 10 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/borg/repository.py	86.90%	6 Missing and 5 partials ⚠️
src/borg/archiver/compact_cmd.py	85.29%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9892      +/-   ##
==========================================
- Coverage   85.32%   85.24%   -0.09%     
==========================================
  Files          93       93              
  Lines       15671    15898     +227     
  Branches     2387     2429      +42     
==========================================
+ Hits        13372    13552     +180     
- Misses       1598     1638      +40     
- Partials      701      708       +7     

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

[ThomasWaldmann]

Guess the test failure is because compact decides internally not to compact anything, because it is below the new threshold.

[mr-raj12]

will look into this , once #9877 is done and merged will need to rebase, as the complete flag and other related changes exist in

[ThomasWaldmann]

Review by Claude Fable 5

Overview

Three changes to borg compact, all aimed at not invalidating every client's cached chunk index (#9817) for marginal gains:

1. All-packs gate — skip drop/rewrite entirely when total reclaimable bytes are below threshold/5 percent (2% at default) of all pack bytes.
2. Tiny-pack merging — new Repository.merge_packs() combines fully-used packs smaller than MIN_PACK_SIZE (1 MB) into full-size packs via store.defrag, gated on their combined size reaching pack_max_size (fixes the one-tiny-pack-per-incremental-backup pileup, borg2: compact: better policy #9816).
3. Index-write skip — save_chunk_index() only runs when the store actually changed, plus an up-front assert_writable() so a restricted repo fails early instead of no-opping silently.

The overall design is sound and the crash-safety story is right: chunk indexes are deleted before the first store change, new packs are stored and indexed before sources are deleted, single-pack batches that defrag to the same content-hash name are correctly excluded from deletion, and whole-file copying preserves unindexed bytes (consistent with the #9868 policy). The index-write skip is safe: build_chunkindex_from_repo(write_immediately=True) already persists a rebuilt index at load time (index-loss case), fragment consolidation happens via repack_chunkindex() on the read path, and flags/sizes are never serialized anyway.

Issues, most important first

1. assert_writable raises the wrong exception type — user gets a traceback.
It raises borgstore's PermissionDenied (a BackendError), but the top-level handler in archiver/__init__.py only pretty-prints borg's Error subclasses. Everywhere else, borgstore errors are translated at the call site (e.g. except StoreBackendError as e: raise Error(str(e)) in Repository.__init__). As written, borg compact on a no-delete repo produces a traceback and the wrong exit code instead of a clean error message. Raise borg's Error (or a dedicated Error subclass on Repository).

2. The "merged packs are never tiny again" invariant breaks when BORG_PACK_MAX_SIZE < MIN_PACK_SIZE.
MIN_PACK_SIZE is a fixed 1 MB constant, but the merge gate and batch cap use the configurable repository.pack_max_size. If a user sets BORG_PACK_MAX_SIZE below 1 MB, every full-size pack stays a merge candidate forever: tiny_total >= pack_max_size is true on every run, so every compact re-defrags packs, deletes the chunk index, and rewrites it — the exact churn this PR exists to prevent. (This PR's own tests run in this regime with BORG_PACK_MAX_SIZE=300, which is why the second test needs the artificial 100000 value.) Fix: derive the tiny threshold from the actual cap, e.g. min(MIN_PACK_SIZE, pack_max_size // 2), so a merged output pack can never re-qualify.

3. merge_packs refreshes the lock only once, at entry.
compact_pack is called once per pack, so its entry _lock_refresh() fires throughout a long compaction. merge_packs handles all batches in a single call and uses self.store.info()/self.store.defrag() directly (not the store_* wrappers that refresh). Merging thousands of tiny packs copies gigabytes; without refreshes the exclusive lock can go stale mid-merge and another client may break it (the same failure family as #9883/#9894). Add self._lock_refresh() inside the batch loop — it's rate-limited internally, so per-batch is fine.

4. defrag errors are not translated in merge_packs.
Commit 7925caa deliberately wraps store.defrag's ReadRangeError into IntegrityError in compact_pack. The new defrag call in merge_packs has no such wrapper, so a pack that shrank between info() and defrag (concurrent modification, backend corruption) surfaces as a raw borgstore exception/traceback. Wrap it the same way.

5. Merge is uninterruptible and reports as one progress step.
The drop/rewrite loops check sig_int per pack; merge_packs is guarded only by one check before the call. A multi-gigabyte merge can't be stopped between batches (crash-safe either way, but Ctrl-C responsiveness regresses), and the progress bar counts the entire merge as a single step. Consider checking sig_int between batches and counting batches in the progress total.

Smaller points

• Docs not updated. The --threshold help text ("rewrite a pack when at least PERCENT of its bytes are unused") and the compact epilog don't mention the new all-packs gate (threshold/5 is a non-obvious magic ratio), the tiny-pack merging, or that --threshold 0 disables the gate. Given this changes when compact does nothing at all, users need this documented.
• Brittle test assertion. test_compact_packs_merges_tiny_packs asserts packs_after.isdisjoint(packs_before). If greedy batching ever produces a single-pack final batch (depends on exact object sizes vs the 300-byte cap), that pack defrags to its identical content-hash name, "survives", and the assertion fails. It passes today by size coincidence; a future change to the object header size would break it silently. Either tolerate a surviving singleton or pin the batch arithmetic.
• assert_writable has no test — neither the early-refusal path (no-delete/read-only repo) nor that --dry-run still works on a read-only repo (the code correctly skips the check for dry-run).
• Batching order is nondeterministic — for pid in pack_ids: iterates a set of hash ids, so batch composition varies run to run. Harmless for correctness, but sorting would make merges reproducible (and the store dedupe-skip more effective across retries).
• PR description is stale: it describes a MIN_PACK_COUNT (10) count-based trigger and "packs 15 → 1" test, but the code (correctly, per review feedback baked into the second test's comment) uses a combined-size gate. Worth updating before merge so the rationale in git history matches the code.

Verdict

Good direction and careful crash-safety work; the repository-level tests (unindexed bytes carried forward, past-EOF detection, already-gone pack) mirror compact_pack's conventions nicely. I'd want items 1–4 fixed before merge: 1 and 4 are small mechanical fixes, 3 is a few lines, and 2 needs a small design tweak (derive the tiny threshold from pack_max_size) plus the docs update.

🤖 Generated with Claude Code

[mr-raj12]

all addressed:

• compact now raises a CompactionPermissionDenied(Error) up front,
• tiny limit is min(MIN_PACK_SIZE, pack_max_size // 2),
• per-batch _lock_refresh(),
• defrag's ReadRangeError wrapped as IntegrityError,
• per-batch sig_int check and progress; plus docs, sorted deterministic batching, relaxed the isdisjoint assertion, added assert_writable/dry-run tests, and refreshed the PR description.

[ThomasWaldmann]

Follow-up review by Claude Fable 5

Re-reviewed at head 1aa33c0. All five issues from the previous review are properly fixed, and the fixes are correct. Verification of each:

1. Exception type — the new Repository.CompactionPermissionDenied(Error) with exit_mcode = 24 follows borg's Error convention (docstring template filled with the namespace arg), and 24 is unused (repository.py uses 10–23, with 20 retired). The restricted-permissions CLI tests were updated to expect it. One further suggestion on this below.

2. Tiny limit vs small BORG_PACK_MAX_SIZE — tiny_limit = min(MIN_PACK_SIZE, pack_max_size // 2) makes the invariant genuinely hold: any closed batch is larger than max_size − tiny_limit ≥ max_size/2 ≥ tiny_limit, so a merged pack can never re-qualify. Only the leftover final batch can stay tiny, and it alone cannot retrigger the tiny_total ≥ pack_max_size gate. The test now runs a second compact_packs() and asserts store_changed is False — exactly the regression guard needed.

3. Lock refresh — _lock_refresh() now runs per batch inside the merge loop.

4. ReadRangeError — wrapped into IntegrityError at the defrag call, matching compact_pack.

5. Interruptibility/progress — per-batch sig_int check and a dedicated ProgressIndicatorPercent(total=len(batches)). The per-batch source deletion added along the way is a crash-safety redesign, so it got a careful look: each batch's new pack is stored and its objects repointed before that batch's sources are deleted, the repo chunk index is already invalidated before the first store change, and the in-memory index stays consistent with the store at every batch boundary. A Ctrl-C or crash between batches leaves leftover candidates to be merged on the next run. This is sound — and actually better than the original all-then-delete design, since the store holds at most one batch of extra packs at a time. The produced-set skip for one-pack batches that reproduce their own content-hash name remains correct.

The smaller points were also addressed: docs (compact epilog section on both gates, updated --threshold help including the --threshold 0 escape hatch), the brittle isdisjoint assertion replaced with packs_after - packs_before plus the idempotence check, a direct assert_writable unit test covering all permission combinations, sorted/reproducible batching, --dry-run verified to work on read-only repos, and the PR description rewritten to match the size-based design.

CI: all test jobs pass (a few VM jobs still pending), codecov/patch passes; only codecov/project fails, which is the overall project-coverage target rather than something this PR introduced.

Remaining nits, none blocking: the outer compact progress bar counts the whole merge as one step while merge_packs runs its own indicator (cosmetic), and the produced set could be scoped per batch for clarity (behavior is correct either way — a multi-pack output can never collide with a different source pack's name).

Suggestion: generic PermissionDenied instead of CompactionPermissionDenied

A compaction-specific exception class puts operation details into Repository's generic vocabulary. Prefer a generic class that any future permission-gated operation (check --repair, repo-delete, …) can reuse, with the operation-specific context in the message. Since borg's Error formats its docstring with the constructor args:

class PermissionDenied(Error):
    """Repository permission denied: {}"""

    exit_mcode = 24

as a nested class on Repository (reading Repository.PermissionDenied at call sites, like the existing Repository.PathPermissionDenied). Then assert_writable supplies the specifics — including which namespace failed and what is actually granted, which the current message doesn't tell the user:

def assert_writable(self):
    """Raise PermissionDenied if the repo permissions forbid compaction. ..."""
    if self.permissions is None:
        return
    for namespace in ("packs", "index"):
        granted = set(self.permissions.get(namespace, self.permissions.get("", "")))
        if not (granted & set("wW")) or "D" not in granted:
            raise self.PermissionDenied(
                f"compaction needs write (w/W) and delete (D) permissions on {namespace}/, "
                f"but only {''.join(sorted(granted))!r} is granted (BORG_REPO_PERMISSIONS)."
            )

So instead of the fixed (need write and delete on packs/) the user sees e.g.:

Repository permission denied: compaction needs write (w/W) and delete (D) permissions on packs/, but only 'lrw' is granted (BORG_REPO_PERMISSIONS).

Knock-on changes: the three pytest.raises(Repository.CompactionPermissionDenied) in restricted_permissions_test.py and the cases in test_assert_writable become Repository.PermissionDenied. Keeping exit_mcode 24 on the generic class is right — one mcode for "repo permissions deny the operation" regardless of which operation, with the message carrying the detail.

Related naming thought, not essential now: assert_writable undersells what it checks (delete too, and specifically for compaction). With the exception going generic, renaming it to e.g. assert_compactable — or parametrizing it as assert_permissions(ops, operation="compaction") once a second caller shows up — would keep compaction specifics out of Repository's generic API.

🤖 Generated with Claude Code