Skip to content

Repository: don't mask the original exception when unwinding with buffered chunks - #10013

Open
ThomasWaldmann wants to merge 2 commits into
borgbackup:masterfrom
ThomasWaldmann:packwriter-drop-on-unwind
Open

Repository: don't mask the original exception when unwinding with buffered chunks#10013
ThomasWaldmann wants to merge 2 commits into
borgbackup:masterfrom
ThomasWaldmann:packwriter-drop-on-unwind

Conversation

@ThomasWaldmann

@ThomasWaldmann ThomasWaldmann commented Aug 2, 2026

Copy link
Copy Markdown
Member

Problem

Repository.close() asserts that the PackWriter has no unflushed chunks. When a command aborts with chunks still buffered, the with repository: unwind calls close() and that assertion raises AssertionError, masking the original exception. Buffered chunks also leave F_PENDING entries in the chunk index, which the close()-time index persist asserts on (chunk ... has no pack location yet).

Reproduce: open a Repository, put() one small chunk (below the pack size limit), raise inside the with-block — the AssertionError replaces the real error.

This affects the paths that put chunks without a Cache:

  • ArchiveChecker.add_reference()borg check --repair
  • borg debug put-obj

Commands that use a Cache are not affected: Cache is the inner with, so Cache.close() unwinds first and flushes the pack writer via _maybe_write_chunks_index(force=True). ArchiveChecker.finish() flushes as well since #10072, but only on the success path, so an abort before that still reaches close() with a non-empty buffer.

Fix

On exception unwind, Repository.__exit__ calls PackWriter.discard() — the abort-side counterpart to flush():

  • it joins a still in-flight pack store first, so a pack that was already stored gets recorded in the index. Joining first also means dropping buffered entries cannot break update_pack_info() for a chunk id that sits in the in-flight pack and in the buffer (dropping first would delete the shared index entry, update_pack_info() would then KeyError mid-pack and leave F_PENDING leftovers for the persist to trip over — masking the original error again through a different door).
  • then it drops the buffered pieces and their still-pending index entries: those chunks were never stored and die with the aborted operation. Store errors from the join are logged, not raised.

close()'s assertion is unchanged: on a clean close it still catches a genuinely forgotten flush(). __exit__ is the only place that knows whether we are unwinding, so the discard decision lives there.

All abort-time index cleanup goes through one helper, PackWriter._drop_index_entries():

  • it never builds the chunk index from the repo — that I/O can fail and mask the error being unwound. With no in-memory index there is nothing to delete: add() installs a chunk's entry before buffering its piece, so pending entries never outlive a dropped index.
  • it only deletes entries that are still pending — a resolved entry means the chunk is in a stored pack; only the aborted duplicate piece dies.
  • _apply_outcome() gets the same no-index guard, so joining a pack store while aborting cannot trigger an index rebuild either.

close() itself could still mask the original error a few lines further down: the close-time chunk index persist and the lock release both do store I/O, which fails again exactly when the abort was a store failure. Both are now logged instead of raised (the persisted index is only a cache and gets rebuilt; an unreleasable lock goes stale eventually). The lock release and store close now run in a finally block, so a close-time error — including the unflushed-chunks assertion — cannot leak the exclusive lock anymore.

Also: dropped buffer pieces are logged (debug level), the abort semantics are documented in docs/internals/packs.rst, and ChunkIndex.is_pending/F_PENDING were added to the .pyi stub.

Tests

  • test_exception_unwind_drops_buffered_chunks — the base repro: asserts the original ValueError (not AssertionError) propagates, and that after reopening the buffered chunk is neither in the chunk index nor readable.
  • test_exception_unwind_records_inflight_pack_drops_buffer — unwind with one pack in flight and more chunks buffered, including a chunk id that is in both: the stored pack's chunks stay recorded and readable, only the never-stored chunk dies. Fails without the join-before-drop ordering.
  • test_exception_unwind_survives_failing_index_persist — the store dies mid-operation: the close-time index persist fails, is logged, the original exception propagates, and the lock is still released. Fails without the guarded persist.
  • test_exception_unwind_does_not_rebuild_dropped_chunk_index — buffered chunks plus a dropped index: asserts no chunk-index rebuild happens during unwind and the original exception survives.
  • test_close_with_unflushed_chunks_asserts — documents that a clean exit with buffered chunks still trips the assertion, and that even that failing close releases the lock and closes the store.

Rebased onto current master (b6b8ec062); the only conflict was context in hashindex.pyi against the prefix-iteritems change. Full test suite passes, ruff clean (one pre-existing ruff format divergence at repository.py:1173 is unrelated — newer local ruff).

🤖 Generated with Claude Code

@ThomasWaldmann
ThomasWaldmann marked this pull request as draft August 2, 2026 14:48
@codecov

codecov Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.60870% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.76%. Comparing base (bf6a449) to head (c15f87f).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/borg/repository.py 82.60% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10013      +/-   ##
==========================================
- Coverage   86.77%   86.76%   -0.01%     
==========================================
  Files          98       98              
  Lines       17277    17299      +22     
  Branches     2622     2624       +2     
==========================================
+ Hits        14992    15010      +18     
- Misses       1587     1592       +5     
+ Partials      698      697       -1     

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

@ThomasWaldmann
ThomasWaldmann force-pushed the packwriter-drop-on-unwind branch 2 times, most recently from f3ca1d1 to 7bd4296 Compare August 7, 2026 18:32
@ThomasWaldmann ThomasWaldmann changed the title Repository.close(): don't mask the original exception when unwinding with buffered chunks Repository: don't mask the original exception when unwinding with buffered chunks Aug 7, 2026
@ThomasWaldmann
ThomasWaldmann force-pushed the packwriter-drop-on-unwind branch from 7bd4296 to f533d6c Compare August 11, 2026 08:43
@ThomasWaldmann
ThomasWaldmann marked this pull request as ready for review August 11, 2026 09:13
ThomasWaldmann and others added 2 commits August 12, 2026 15:58
…fered chunks

When a command aborts with chunks still buffered in the PackWriter, the
"with repository:" unwind called close(), whose "PackWriter has unflushed
chunks" assertion raised AssertionError and masked the original exception.
Buffered chunks also left F_PENDING entries in the chunk index, which the
close()-time index persist asserts on.

This affects the paths that put chunks without a Cache: ArchiveChecker
(borg check --repair) and borg debug put-obj. Commands that use a Cache are
unaffected, because Cache.close() unwinds first and flushes the pack writer.
ArchiveChecker.finish() flushes too, but only on the success path, so an
abort before that still reaches close() with a non-empty buffer.

Fix: on exception unwind, Repository.__exit__ drops the buffered pieces and
their still-pending index entries via PackWriter._drop_buffered(), 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().

_drop_buffered() only ever runs while aborting, so it must not build the
chunk index from the repo: that I/O can fail and mask the error being
unwound. It now empties the buffer before it touches the index and skips the
index cleanup when no index is loaded, where there is nothing to delete
anyway. invalidate_chunk_index() is what leaves that state behind; its
callers all flush first or never buffer, so this keeps the helper safe
either way.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…raise from close() teardown

Review follow-ups on the drop-on-unwind fix:

Repository.__exit__ now calls PackWriter.discard(), the abort-side
counterpart to flush(): it joins a still in-flight pack store first, so a
pack that was already stored gets recorded in the index - and dropping
buffered entries can no longer break update_pack_info() for a chunk id
sitting in that pack and in the buffer (dropping first deleted the shared
index entry, update_pack_info() then raised KeyError mid-pack and left
F_PENDING leftovers for the close()-time persist to assert on, masking the
original exception through a different door). Store errors from the join
are logged, not raised.

All abort-time index cleanup goes through PackWriter._drop_index_entries():
it never builds the chunk index from the repo (that I/O can fail and mask
the error being unwound; with no in-memory index there is nothing to
delete, since add() installs a chunk's entry before buffering its piece)
and it only deletes entries that are still pending - a resolved entry means
the chunk is in a stored pack, only the aborted duplicate piece dies.
_apply_outcome() gets the same no-index guard, so joining a pack store
while aborting cannot trigger a rebuild either.

close() could still mask the original error a few lines further down: the
close()-time chunk index persist and the lock release both do store I/O,
which fails again exactly when the abort was caused by a failing store.
Both are now logged instead of raised (the persisted index is only a
cache; an unreleasable lock goes stale eventually), and the lock release
and store close run in a finally block, so a close()-time error - e.g. the
unflushed-chunks assertion - cannot leak the exclusive lock anymore.

Also: log dropped buffer pieces (debug level), document the abort
semantics in docs/internals/packs.rst, add ChunkIndex.is_pending/F_PENDING
to the .pyi stub, new tests for the join-before-drop ordering and the
guarded persist (both fail without the fixes).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ThomasWaldmann
ThomasWaldmann force-pushed the packwriter-drop-on-unwind branch from 888bdc2 to c15f87f Compare August 12, 2026 14:09
@ThomasWaldmann ThomasWaldmann added this to the 2.0.0b23 milestone Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant