From 3d0aff00261598178a5ea26e505a3673fbfcfa79 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Sun, 2 Aug 2026 16:43:45 +0200 Subject: [PATCH] Repository.close(): don't mask the original exception when unwinding with buffered chunks When a borg command aborts with chunks still buffered in the PackWriter (e.g. an exception mid-create between pack flushes), the "with repository:" unwind called close(), whose "PackWriter has unflushed chunks" assertion raised AssertionError and masked the original exception. Also, buffered chunks left F_PENDING entries in the chunk index, which the close()-time index persist would trip over. Fix: on exception unwind, Repository.__exit__ now drops the buffered pieces and their still-pending index entries via the new PackWriter._drop_buffered() (also reused in flush()'s store-failure path), so the original exception propagates unmasked and no F_PENDING entries are persisted. The never-stored chunks die with the aborted operation. On a clean close, the assertion still catches a forgotten flush(). Co-Authored-By: Claude Fable 5 --- src/borg/repository.py | 32 ++++++++++++++++++++------- src/borg/testsuite/repository_test.py | 31 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/src/borg/repository.py b/src/borg/repository.py index 254c094589..e4109ec65b 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -196,21 +196,31 @@ def flush(self): offset += obj_size key = "packs/" + bin_to_hex(pack_id) - pending_ids = [chunk_id for chunk_id, _ in self._pieces] try: self.store.store(key, pack_data) except Exception: - # the pack was not stored: drop the index entries for its chunks. - for chunk_id in pending_ids: - if chunk_id in self.chunks: # a chunk_id may appear more than once in this pack - del self.chunks[chunk_id] + # the pack was not stored: drop the pieces and the index entries for its chunks. + self._drop_buffered() raise - finally: - self._pieces = [] # cleared on success and on failure - self._size = 0 + self._pieces = [] + self._size = 0 self.chunks.update_pack_info(results) # set the real location and clear F_PENDING return results + def _drop_buffered(self): + """Drop the buffered pieces and their (still pending) index entries. + + Called when a pack store failed or the caller is aborting: chunks not yet handed + to the store die with it. Dropping their entries keeps the index free of F_PENDING + leftovers, so the close()-time index persist works. + """ + pieces = self._pieces + self._pieces = [] + self._size = 0 + for chunk_id, _ in pieces: + if chunk_id in self.chunks: # a chunk_id may appear more than once in the buffer + del self.chunks[chunk_id] + class PackReader: """Reads pack files, the read-side counterpart to PackWriter. @@ -594,6 +604,12 @@ def __enter__(self): return self def __exit__(self, exc_type, exc_val, exc_tb): + if exc_type is not None and self._pack_writer is not None: + # unwinding an exception: chunks still buffered in the pack writer were never + # stored, so they die with the aborted operation. drop them (and their + # F_PENDING index entries) so close() neither trips its flush assertion -- + # which would mask the original exception -- nor persists pending entries. + self._pack_writer._drop_buffered() self.close() @property diff --git a/src/borg/testsuite/repository_test.py b/src/borg/testsuite/repository_test.py index 1e6d7cfd1e..79874eda68 100644 --- a/src/borg/testsuite/repository_test.py +++ b/src/borg/testsuite/repository_test.py @@ -153,6 +153,37 @@ def test_chunk_index_persisted_on_close(tmp_path): assert pdchunk(repository.get(H(x))) == b"DATA" +def test_exception_unwind_drops_buffered_chunks(tmp_path): + # An exception inside "with repository:" unwinds with chunks still buffered in the + # PackWriter (put() buffers until a pack fills or flush() is called). __exit__ must + # drop the buffered chunks so that close() neither replaces the original exception + # with its "call flush() before close()" assertion nor persists F_PENDING index + # entries for chunks that were never stored. + location = os.fspath(tmp_path / "repo") + with pytest.raises(ValueError, match="original error"): + with Repository(location, exclusive=True, create=True) as repository: + repository.put(H(0), fchunk(b"DATA")) + assert repository._pack_writer._pieces # small chunk: still buffered, no pack written + raise ValueError("original error") + with Repository(location, exclusive=True) as repository: + # the buffered chunk died with the aborted operation: not in the index, not readable + assert H(0) not in repository.chunks + with pytest.raises(Repository.ObjectNotFound): + repository.get(H(0)) + + +def test_close_with_unflushed_chunks_asserts(tmp_path): + # On a clean (non-exception) path, closing with buffered chunks is a caller bug: + # the assertion in close() still catches a forgotten flush(). + location = os.fspath(tmp_path / "repo") + with pytest.raises(AssertionError, match="unflushed"): + with Repository(location, exclusive=True, create=True) as repository: + repository.put(H(0), fchunk(b"DATA")) + # clean up the deliberately broken close: drop the buffered chunk, then close for real + repository._pack_writer._drop_buffered() + repository.close() + + def test_read_data(repo_fixtures, request): with get_repository_from_fixture(repo_fixtures, request) as repository: meta, data = b"meta", b"data"