From fd1775356170552b48107efcab569e8a49197271 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 18 Jun 2026 18:50:46 +0530 Subject: [PATCH 1/2] repository: always name packs by sha256(pack), refs #8572 Drop the single-chunk shortcut that reused the chunk_id as the pack_id. A pack is now always named by sha256 of its bytes, even when it holds a single chunk, so no code can depend on pack_id == chunk_id. --- src/borg/conftest.py | 3 -- src/borg/repository.py | 33 +++++-------------- src/borg/testsuite/archiver/check_cmd_test.py | 2 +- src/borg/testsuite/repository_test.py | 22 +++++-------- 4 files changed, 17 insertions(+), 43 deletions(-) diff --git a/src/borg/conftest.py b/src/borg/conftest.py index fb80b5bff2..80cd87cb28 100644 --- a/src/borg/conftest.py +++ b/src/borg/conftest.py @@ -80,9 +80,6 @@ def pytest_sessionfinish(session, exitstatus): @pytest.fixture(autouse=True) def clean_env(tmpdir_factory, monkeypatch): # also avoid to use anything from the outside environment: - # note: BORG_TESTONLY_SHA256_PACK_ID is intentionally NOT exempted here. The repository - # module captures it at import time (repository.FORCE_SHA256_PACK_ID), before this fixture - # runs, so wiping it per test is harmless and the env stays fully isolated. keys = [key for key in os.environ if key.startswith("BORG_") and key not in ("BORG_FUSE_IMPL",)] for key in keys: monkeypatch.delenv(key, raising=False) diff --git a/src/borg/repository.py b/src/borg/repository.py index b07a2af2aa..f44c5fd37b 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -102,14 +102,6 @@ def build_rest_backend(location): return REST(base_url="http://stdio-backend", command=rest_serve_command(location)) -# Test-only switch: force sha256 pack_ids even at N=1, to expose code that still assumes -# pack_id == chunk_id. Read once here, at import time, on purpose: -# - it happens before the per-test clean_env fixture wipes BORG_* vars, so the tox env that -# sets this var does not need a clean_env exemption (the captured value survives the wipe); -# - flush() then checks a plain bool instead of touching os.environ on every write. -FORCE_SHA256_PACK_ID = os.environ.get("BORG_TESTONLY_SHA256_PACK_ID") == "1" - - class PackWriter: """Buffers chunks into a pack file and writes to the store when full. @@ -123,8 +115,7 @@ class PackWriter: uses that repository's single, authoritative index (see the chunks property), so there is never a second copy to keep in sync. Unit tests pass an explicit index. - At max_count=1 (N=1 phase) each put() maps exactly one chunk to one pack, - so pack_id == chunk_id and the naming scheme is unchanged from before. + At max_count=1 (N=1 phase) each put() maps exactly one chunk to one pack. Raising max_count later (N>1 phase) enables real packing without touching this class's interface. """ @@ -179,17 +170,10 @@ def flush(self): # that incremental string concatenation would cause in Python). pack_data = b"".join(cdata for _, cdata in self._pieces) - # Determine pack_id. - # N=1: the pack contains exactly one chunk, so we keep pack_id == chunk_id - # (backward-compatible file naming: packs/{chunk_id_hex}). - # N>1: the pack contains multiple chunks; use SHA256(pack_bytes) so the - # file is content-addressed and borgstore can verify/cache it. - # BORG_TESTONLY_SHA256_PACK_ID (see FORCE_SHA256_PACK_ID): always use sha256 even at - # N=1, exposing code that still assumes pack_id == chunk_id. - if self.max_count == 1 and not FORCE_SHA256_PACK_ID: - pack_id = self._pieces[0][0] # N=1: pack_id == chunk_id - else: - pack_id = sha256(pack_data).digest() + # Name the pack by the hash of its bytes (content-addressing), independent of how many + # chunks it holds or what their ids are. This is why a single-chunk pack's name is not its + # chunk_id: the pack and the chunk are different objects with different identities. + pack_id = sha256(pack_data).digest() # Record (chunk_id, pack_id, obj_offset, obj_size) for every piece. results = [] @@ -675,10 +659,9 @@ def check_pack(pack): # add all existing objects to the index. # borg check: the index may have corrupted objects (we did not delete them) # borg check --repair: the index will only have non-corrupted objects. - # the pack file name is the pack_id (sha256(pack) at N>1 or with the - # BORG_TESTONLY_SHA256_PACK_ID switch), which is not the chunk_id, so recover - # each object's real (chunk_id, offset, size) from its on-disk header rather - # than assuming pack file name == chunk_id. + # the pack file name is the pack_id (sha256(pack_bytes)), which is not the + # chunk_id, so recover each object's real (chunk_id, offset, size) from its + # on-disk header rather than assuming pack file name == chunk_id. pack_id = hex_to_bin(info.name) for chunk_id, obj_offset, obj_size in RepoObj.iter_object_headers(pack): chunks[chunk_id] = ChunkIndexEntry( diff --git a/src/borg/testsuite/archiver/check_cmd_test.py b/src/borg/testsuite/archiver/check_cmd_test.py index a1c0ecb8cd..a9412e59ac 100644 --- a/src/borg/testsuite/archiver/check_cmd_test.py +++ b/src/borg/testsuite/archiver/check_cmd_test.py @@ -449,7 +449,7 @@ def test_empty_repository(archivers, request): with Repository(archiver.repository_location, exclusive=True) as repository: # empty the repo by dropping every pack file directly via the store. We iterate the actual # packs/ listing (the file names are the pack_ids), so this does not depend on what list() - # yields or on pack_id == chunk_id. + # yields. for info in repository.store_list("packs"): repository.store_delete("packs/" + info.name) cmd(archiver, "check", exit_code=1) diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 12a90ea329..3f7a683b3a 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -5,7 +5,7 @@ import pytest from ..helpers import IntegrityError, Location, bin_to_hex from ..hashindex import ChunkIndex -from ..repository import Repository, MAX_DATA_SIZE, rest_serve_command, PackWriter, FORCE_SHA256_PACK_ID +from ..repository import Repository, MAX_DATA_SIZE, rest_serve_command, PackWriter from ..repoobj import RepoObj, OBJ_MAGIC, OBJ_VERSION from .hashindex_test import H @@ -55,8 +55,8 @@ def reopen(repository, exclusive: bool | None = True, create=False): def fchunk(data, meta=b"", chunk_id=b"\x00" * 32): # Build a raw chunk with a valid RepoObj layout but no encryption or compression. Pass a unique - # chunk_id when objects must not share a pack: identical bytes hash to the same sha256 pack id, - # so under BORG_TESTONLY_SHA256_PACK_ID they would otherwise collapse into one pack. + # chunk_id when objects must not share a pack: identical bytes hash to the same sha256 pack id + # and would otherwise collapse into one pack. hdr = RepoObj.obj_header.pack(OBJ_MAGIC, OBJ_VERSION, chunk_id, len(meta), len(data)) assert isinstance(data, bytes) chunk = hdr + meta + data @@ -222,10 +222,8 @@ def test_pack_writer_n1_flush(): assert len(results) == 1 stored_id, pack_id, obj_offset, obj_size = results[0] assert stored_id == chunk_id - if FORCE_SHA256_PACK_ID: - assert pack_id == sha256(cdata).digest() # sha256 switch: pack is named by its content - else: - assert pack_id == chunk_id # N=1: pack_id == chunk_id + assert pack_id == sha256(cdata).digest() + assert pack_id != chunk_id assert obj_offset == 0 assert obj_size == len(cdata) @@ -344,11 +342,8 @@ def test_put_marks_id_in_chunk_index(tmp_path): repository.put(id1, fchunk(b"ZEROS")) entry = repository._chunks.get(id1) assert entry is not None - if FORCE_SHA256_PACK_ID: - # sha256 switch: the pack is named by its content, not by the chunk_id. - assert entry.pack_id == sha256(fchunk(b"ZEROS")).digest() - else: - assert entry.pack_id == id1 # N=1: pack_id == chunk_id, set by update_pack_info in put() + assert entry.pack_id == sha256(fchunk(b"ZEROS")).digest() + assert entry.pack_id != id1 assert entry.size == 0 # uncompressed size filled in by cache layer @@ -372,8 +367,7 @@ def test_check_detects_corruption_in_later_object(tmp_path): def test_pack_writer_final_partial_pack_uses_sha256(): - # When max_count > 1, a final flush with only 1 piece must still use SHA256, - # not the N=1 pack_id == chunk_id hack. + # A final flush with fewer pieces than max_count must still use SHA256(pack_bytes). store = MockStore() chunk_id = b"d" * 32 cdata = b"solo" From 0ece418991ea0bb2b6394ebfbd28a9bf85e2ca11 Mon Sep 17 00:00:00 2001 From: Mrityunjay Raj Date: Thu, 18 Jun 2026 19:32:59 +0530 Subject: [PATCH 2/2] cleanup: drop leftover pack_id==chunk_id references after sha256 naming, refs #8572 Remove the dead BORG_TESTONLY_SHA256_PACK_ID tox env and CI job, fix the packs.rst pack-id docs, and reword comments/tests to describe sha256 pack naming instead of the removed shortcut. --- .github/workflows/ci.yml | 54 --------------------------- docs/internals/packs.rst | 9 +++-- pyproject.toml | 5 --- src/borg/repository.py | 15 +++----- src/borg/testsuite/repository_test.py | 2 - 5 files changed, 11 insertions(+), 74 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4c2a5fde95..c0f36aa368 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -703,57 +703,3 @@ jobs: report_type: coverage env_vars: OS,python files: coverage.xml - - sha256_pack_id_tests: - name: sha256 pack-id (informational) - needs: [lint] - runs-on: ubuntu-24.04 - timeout-minutes: 90 - continue-on-error: true - concurrency: - group: sha256-pack-id-${{ github.head_ref || github.ref }} - cancel-in-progress: false - - steps: - - uses: actions/checkout@v6 - with: - fetch-depth: 0 - fetch-tags: true - - - name: Set up Python 3.12 - uses: actions/setup-python@v6 - with: - python-version: "3.12" - - - name: Cache pip - uses: actions/cache@v5 - with: - path: ~/.cache/pip - key: ${{ runner.os }}-${{ runner.arch }}-pip-sha256-pack-id-${{ hashFiles('requirements.d/development.lock.txt') }} - restore-keys: | - ${{ runner.os }}-${{ runner.arch }}-pip- - - - name: Cache tox environments - uses: actions/cache@v5 - with: - path: .tox - key: ${{ runner.os }}-${{ runner.arch }}-tox-sha256-pack-id-${{ hashFiles('requirements.d/development.lock.txt', 'pyproject.toml') }} - restore-keys: | - ${{ runner.os }}-${{ runner.arch }}-tox-sha256-pack-id- - - - name: Install Linux packages - run: | - sudo apt-get update - sudo apt-get install -y pkg-config build-essential - sudo apt-get install -y libssl-dev libacl1-dev liblz4-dev - - - name: Install Python requirements - run: | - python -m pip install --upgrade pip setuptools wheel - pip install -r requirements.d/development.lock.txt - - - name: Install borgbackup - run: pip install -ve ".[cockpit,s3,sftp,rclone]" - - - name: Run tests with sha256 pack-ids - run: tox -e sha256-pack-id diff --git a/docs/internals/packs.rst b/docs/internals/packs.rst index f5f555a3b4..211e1cd93a 100644 --- a/docs/internals/packs.rst +++ b/docs/internals/packs.rst @@ -71,12 +71,13 @@ Blobs follow one another contiguously with no padding:: Pack ID ~~~~~~~ -The pack ID equals the ``chunk_id`` of the blob it contains:: +The pack ID is the SHA-256 of the pack file's bytes:: - pack_id = chunk_id + pack_id = sha256(pack_bytes) -Since ``chunk_id`` is the ID hash of the plaintext, the filename commits to the -content. ``borg check`` can detect silent corruption without decrypting any blob. +Content-addressing the file by its own bytes makes the name commit to the +content, so borgstore can verify and cache it and ``borg check`` can detect +silent corruption of the stored file. Namespace ~~~~~~~~~ diff --git a/pyproject.toml b/pyproject.toml index c9e0ec8cc2..683a463726 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -257,11 +257,6 @@ extras = ["pyfuse3", "sftp", "s3", "rclone"] set_env = {BORG_FUSE_IMPL = "mfusepy"} extras = ["mfusepy", "sftp", "s3", "rclone"] -# Informational env: forces sha256 pack_ids even with max_count=1 to expose -# code that still assumes pack_id == chunk_id. Run: tox -e sha256-pack-id -[tool.tox.env.sha256-pack-id] -set_env = {BORG_TESTONLY_SHA256_PACK_ID = "1"} - [tool.tox.env.ruff] skip_install = true deps = ["ruff"] diff --git a/src/borg/repository.py b/src/borg/repository.py index f44c5fd37b..f6c10f731e 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -115,9 +115,8 @@ class PackWriter: uses that repository's single, authoritative index (see the chunks property), so there is never a second copy to keep in sync. Unit tests pass an explicit index. - At max_count=1 (N=1 phase) each put() maps exactly one chunk to one pack. - Raising max_count later (N>1 phase) enables real packing without touching - this class's interface. + max_count bounds how many chunks a pack accumulates before flush() writes it. + Raising it produces larger packs without changing this class's interface. """ def __init__(self, store, *, max_count=1, chunks=None, repository=None): @@ -170,9 +169,8 @@ def flush(self): # that incremental string concatenation would cause in Python). pack_data = b"".join(cdata for _, cdata in self._pieces) - # Name the pack by the hash of its bytes (content-addressing), independent of how many - # chunks it holds or what their ids are. This is why a single-chunk pack's name is not its - # chunk_id: the pack and the chunk are different objects with different identities. + # Name the pack by the SHA-256 of its bytes: the name commits to the stored content, + # so borgstore can verify and cache the file. pack_id = sha256(pack_data).digest() # Record (chunk_id, pack_id, obj_offset, obj_size) for every piece. @@ -659,9 +657,8 @@ def check_pack(pack): # add all existing objects to the index. # borg check: the index may have corrupted objects (we did not delete them) # borg check --repair: the index will only have non-corrupted objects. - # the pack file name is the pack_id (sha256(pack_bytes)), which is not the - # chunk_id, so recover each object's real (chunk_id, offset, size) from its - # on-disk header rather than assuming pack file name == chunk_id. + # the pack file name is the pack_id; each object's chunk_id, offset and size + # come from its on-disk header, so scan the headers to rebuild the index. pack_id = hex_to_bin(info.name) for chunk_id, obj_offset, obj_size in RepoObj.iter_object_headers(pack): chunks[chunk_id] = ChunkIndexEntry( diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 3f7a683b3a..e5e368c7e1 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -223,7 +223,6 @@ def test_pack_writer_n1_flush(): stored_id, pack_id, obj_offset, obj_size = results[0] assert stored_id == chunk_id assert pack_id == sha256(cdata).digest() - assert pack_id != chunk_id assert obj_offset == 0 assert obj_size == len(cdata) @@ -343,7 +342,6 @@ def test_put_marks_id_in_chunk_index(tmp_path): entry = repository._chunks.get(id1) assert entry is not None assert entry.pack_id == sha256(fchunk(b"ZEROS")).digest() - assert entry.pack_id != id1 assert entry.size == 0 # uncompressed size filled in by cache layer