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/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..f6c10f731e 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,10 +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, - so pack_id == chunk_id and the naming scheme is unchanged from before. - 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): @@ -179,17 +169,9 @@ 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 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. results = [] @@ -675,10 +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) 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; 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/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..e5e368c7e1 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,7 @@ 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 obj_offset == 0 assert obj_size == len(cdata) @@ -344,11 +341,7 @@ 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.size == 0 # uncompressed size filled in by cache layer @@ -372,8 +365,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"