Skip to content

create: do not wrap repository writes in backup_io("read") (silent data loss on ENOSPC) - #9853

Merged
ThomasWaldmann merged 1 commit into
borgbackup:masterfrom
ThomasWaldmann:fix-create-enospc-atomicity
Jul 3, 2026
Merged

create: do not wrap repository writes in backup_io("read") (silent data loss on ENOSPC)#9853
ThomasWaldmann merged 1 commit into
borgbackup:masterfrom
ThomasWaldmann:fix-create-enospc-atomicity

Conversation

@ThomasWaldmann

Copy link
Copy Markdown
Member

What

borg create on an out-of-space repository could silently commit a corrupt, unrestorable archive and exit 0 with a normal success summary.

Symptom

On a repository whose backend runs out of space during borg create:

  • create prints a normal summary — Added files: N, Error files: 0 — and exits 0.
  • The committed archive references chunks that were never durably stored. Afterwards:
    • borg checkMissing file chunk detected (exit 1)
    • borg compactRepository has N missing objects! (exit 2)

So a backup reports success but is not restorable.

Root cause

process_file() ran process_file_chunks() inside with backup_io("read"):

with backup_io("read"):
    self.process_file_chunks(item, cache, ..., backup_io_iter(self.chunker.chunkify(None, fd)))

That block is meant to guard reading the source file, but the source reads are already guarded individually by backup_io_iter(). The outer wrapper additionally caught add_chunk()'s repository writes. So a critical repository IO failure (e.g. ENOSPC during a pack flush) was wrapped into a per-file BackupOSError, tagged "read" — indistinguishable from "this source file couldn't be read". Borg then only warned, skipped the file, continued the walk, and create_inner() committed the archive via archive.save(). Because pack flushing is deferred, chunks of earlier, already-emitted items were lost too, so the whole archive ends up referencing missing chunks.

This directly contradicts the BackupOSError docstring:

These are non-critical and are only reported (warnings). Any unwrapped IO error is critical and aborts execution (for example repository IO failure).

Fix

Drop the outer backup_io("read") wrapper. Source reads stay per-file warnings (backup_io_iter is unchanged); repository OSErrors are now left unwrapped and therefore critical, aborting create before archive.save() runs — as the docstring prescribes.

Audited all with backup_io blocks across src/borg: this regular-file path was the only one wrapping a repository operation. The stdin/pipe and import-tar process_file_chunks call sites were already unwrapped (correct).

Testing

  • archive_test.py (40) and create_cmd_test.py (61) pass.
  • Reproduced on a space-limited macOS ramdisk (source data > free space), across a create/delete/compact churn matrix (24/32/64/96 MB, varied data):
    • Before: over-full creates exited 0 with corrupt archives; compact then reported Repository has N missing objects! and check reported missing chunks in every run.
    • After: over-full creates fail (exit 2) and commit nothing; the repository stays consistent (check: no problems found) in every run. Normal backups and unreadable-source-file handling (per-file warning) are unchanged.

Notes / follow-ups (not in this PR)

  • The abort currently surfaces as a traceback (Error: OSError: [Errno 28] …) plus a secondary FileNotFoundError during unwind, rather than a clean Error: No space left on device. Worth polishing (catch the repo OSError → clean Error, make PackWriter/close() teardown ENOSPC-clean).

🤖 Generated with Claude Code

process_file() ran process_file_chunks() inside `with backup_io("read")`.

That block was meant to guard reading the *source* file, but the source
reads are already guarded individually by backup_io_iter(). The outer
wrapper additionally caught add_chunk()'s *repository* writes, so a critical
repository IO failure -- e.g. the repo running out of space during a pack
flush -- was wrapped into a per-file BackupOSError. Borg then only warned,
skipped the file, and continued, and create_inner() still committed the
archive via archive.save().

The result: `borg create` on an out-of-space repo printed a normal success
summary ("Error files: 0"), exited 0, and committed an archive that
references chunks which were never durably stored. `borg check` afterwards
reports "Missing file chunk detected" and `borg compact` reports "Repository
has N missing objects!" -- silent, unrestorable-backup data loss.

Drop the outer backup_io("read") wrapper. Source reads stay per-file
warnings (backup_io_iter is unchanged); repository OSErrors are now left
unwrapped and therefore critical, aborting create before archive.save()
runs, exactly as the BackupOSError docstring prescribes.

Reproduced on a space-limited macOS ramdisk (source > free space): before,
create exited 0 with a corrupt archive; after, create fails and commits
nothing, and the repository stays consistent across a create/delete/compact
churn matrix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ThomasWaldmann
ThomasWaldmann force-pushed the fix-create-enospc-atomicity branch from eb82083 to 5aaa2dd Compare July 3, 2026 17:50
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.11%. Comparing base (a8c2c7b) to head (5aaa2dd).
⚠️ Report is 14 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9853      +/-   ##
==========================================
- Coverage   85.11%   85.11%   -0.01%     
==========================================
  Files          93       93              
  Lines       15409    15408       -1     
  Branches     2326     2326              
==========================================
- Hits        13115    13114       -1     
  Misses       1596     1596              
  Partials      698      698              

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

@ThomasWaldmann
ThomasWaldmann merged commit 985cbc4 into borgbackup:master Jul 3, 2026
18 of 19 checks passed
@ThomasWaldmann
ThomasWaldmann deleted the fix-create-enospc-atomicity branch July 3, 2026 18:35
ThomasWaldmann added a commit that referenced this pull request Aug 2, 2026
PackWriter now hands a full pack to a background store-thread (at most one
in flight): the pack bytes are joined, sha256-hashed (the pack_id) and stored
in the store-thread, while the caller goes on assembling the next pack.
hashlib and the store I/O release the GIL, so there is real overlap even on
CPython.  Throughput becomes max(assembly, store) instead of assembly + store.

The ChunkIndex is only ever touched by the calling thread: the store-thread's
results (or error) are applied when it is joined, at the next pack boundary or
flush().  Consequences:

- put()/add() return the *previous* pack's results while the current pack's
  store is in flight; update_pack_info() keys by chunk_id, so callers do not
  care which pack the results belong to.
- flush() is a barrier: it joins an in-flight store and writes the current
  buffer synchronously, so afterwards nothing is F_PENDING anymore (needed by
  the periodic chunk index persist (#9900) and by close()).
- a store error (e.g. ENOSPC) surfaces one pack later, from whichever
  add()/flush() call joins the store-thread - still before anything gets
  finalized, since the final flush is a barrier (no #9853-style regression).
  the failed pack's index entries are dropped and the buffered pieces die
  with the aborting command, so the close()-time index persist stays clean.
- get()/get_many() of a chunk whose pack store is still in flight join the
  store-thread first (read barrier), then read normally.
- close() joins a still-in-flight store (normally a no-op, flush ran before):
  a stored pack gets recorded, a failed one rolled back (not raising, to not
  mask the error being unwound).

Sharing the Store between the store-thread and the main thread (lock refresh,
reads of already stored packs) requires borgstore >= 0.6.0, which serializes
all Store operations internally (borgstore #206 / #207).  The borgstore
dependency is bumped accordingly and now also pulls the blake3 extra, so
borgstore's hash/defrag blake3 support is available server-side too.

BORG_PACK_ASYNC=no disables the store-thread (debugging aid).

The pre-existing synchronous-contract unit tests run with async_store=False;
new tests cover deferred results, the combined flush barrier, deferred error
surfacing with rollback, and the get() read barrier.
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