fix(backup): decide full-zip publish target once and pin all-failed keep

_write_full_zip_backup_locked chose clean/salvage/discard in _publish_path
and then re-derived the same choice with an inverse test after the with
block. If only one copy changed later, .stat() could hit a path that was
never published and raise out of a "never raises" helper. _publish_path now
records the destination and the stat/return reuse it.

The `destination is None` discard branch in _atomic_output_path had no
teeth: publishing the empty all-failed archive over out_path kept every
test green. The serialization test now asserts an all-failed automatic run
leaves the previous good archive's members unchanged. Also refresh a stale
comment that still described a renamed salvage archive.
This commit is contained in:
kshitijk4poor
2026-09-27 13:59:30 +05:30
committed by kshitij
parent 25a0b3e3d2
commit 9a6417cd6b
2 changed files with 19 additions and 7 deletions

View File

@@ -1161,8 +1161,8 @@ def _create_prefixed_full_backup(
return None return None
out_path = backup_dir / f"{prefix}{datetime.now().strftime('%Y-%m-%d-%H%M%S')}.zip" out_path = backup_dir / f"{prefix}{datetime.now().strftime('%Y-%m-%d-%H%M%S')}.zip"
if _write_full_zip_backup(out_path, hermes_root) is None: if _write_full_zip_backup(out_path, hermes_root) is None:
# Incomplete runs leave a renamed salvage archive; cap those at one without letting # Incomplete runs publish straight to ``*.incomplete.zip`` (all-failed runs are discarded);
# them rotate complete backups out. # cap those salvages at one without letting them rotate complete backups out.
_prune_incomplete_zips(backup_dir, prefix, prune_what) _prune_incomplete_zips(backup_dir, prefix, prune_what)
return None return None
_prune_prefixed_zips(backup_dir, prefix, keep, prune_what) _prune_prefixed_zips(backup_dir, prefix, keep, prune_what)
@@ -2049,10 +2049,16 @@ def _write_full_zip_backup_locked(out_path: Path, hermes_root: Path) -> Optional
# every entry failed salvages nothing, so its empty archive is discarded rather than kept. # every entry failed salvages nothing, so its empty archive is discarded rather than kept.
salvage_path = out_path.with_name(out_path.stem + _INCOMPLETE_ZIP_SUFFIX) salvage_path = out_path.with_name(out_path.stem + _INCOMPLETE_ZIP_SUFFIX)
published: Optional[Path] = None
def _publish_path() -> Optional[Path]: def _publish_path() -> Optional[Path]:
# Decide clean/salvage/discard once; the post-publish stat and return reuse it.
nonlocal published
if not errors: if not errors:
return out_path published = out_path
return salvage_path if len(errors) < len(files_to_add) else None elif len(errors) < len(files_to_add):
published = salvage_path
return published
archive_started = time.monotonic() archive_started = time.monotonic()
try: try:
@@ -2068,11 +2074,11 @@ def _write_full_zip_backup_locked(out_path: Path, hermes_root: Path) -> Optional
logger.warning("Full-zip backup: zip write failed: %s", exc) logger.warning("Full-zip backup: zip write failed: %s", exc)
return None return None
if errors and len(errors) >= len(files_to_add): if published is None:
logger.warning("Full-zip backup: every entry failed, nothing salvaged: %s", _capped_errors()) logger.warning("Full-zip backup: every entry failed, nothing salvaged: %s", _capped_errors())
return None return None
zip_size = (salvage_path if errors else out_path).stat().st_size zip_size = published.stat().st_size
if errors: if published != out_path:
logger.warning( logger.warning(
"automatic backup phase=archive status=incomplete duration_ms=%.1f files=%d errors=%d " "automatic backup phase=archive status=incomplete duration_ms=%.1f files=%d errors=%d "
"bytes=%d salvage=%s skipped=%s", "bytes=%d salvage=%s skipped=%s",

View File

@@ -45,6 +45,12 @@ def test_zip_captures_live_wal_and_cleans_failed_staging(tmp_path, monkeypatch,
def refuse_write(self, filename, arcname=None, **kwargs): def refuse_write(self, filename, arcname=None, **kwargs):
raise OSError("archive device full") raise OSError("archive device full")
with zipfile.ZipFile(archive) as zipped:
previous = {info.filename: info.CRC for info in zipped.infolist()}
monkeypatch.setattr(zipfile.ZipFile, "write", refuse_write) monkeypatch.setattr(zipfile.ZipFile, "write", refuse_write)
run() run()
assert sorted(output.iterdir()) == [archive], "a failed write must not leak a private database snapshot" assert sorted(output.iterdir()) == [archive], "a failed write must not leak a private database snapshot"
if automatic:
with zipfile.ZipFile(archive) as zipped:
assert {info.filename: info.CRC for info in zipped.infolist()} == previous, \
"an all-failed run must not overwrite the previous good archive"