From bea0531a95445550b3ac4ca425ae396a8ebd6da6 Mon Sep 17 00:00:00 2001 From: Codex Date: Tue, 8 Sep 2026 15:00:02 -0300 Subject: [PATCH] fix: complete backup recovery safeguards --- core/backup.py | 64 ++++++++----------- .../commands/test_backup_restore.py | 1 + tests/test_backup.py | 63 +++++++++++++++++- tests/test_backup_commands.py | 16 +++-- tests/test_backup_restore.py | 45 ++++++++++++- 5 files changed, 147 insertions(+), 42 deletions(-) diff --git a/core/backup.py b/core/backup.py index 9c61640..e454e20 100644 --- a/core/backup.py +++ b/core/backup.py @@ -306,50 +306,31 @@ def _publish_no_clobber(source, destination): def _publish_pair(root, pairs): publications = [] - guards = [] try: for source, destination in pairs: _publish_no_clobber(source, destination) publications.append((destination, source)) _fsync_directory(root) - - for source, _destination in pairs: - guard = Path(f"{source}.guard") - os.link(source, guard) - guards.append(guard) - for source, _destination in pairs: - source.unlink() - for guard in guards: - guard.unlink() - _fsync_directory(root) except Exception: - ownership_sources = [*guards, *(source for source, _destination in pairs if source.exists())] - _remove_owned_publications(publications, ownership_sources) + _remove_owned_publications(publications) for source, _destination in pairs: _unlink_private(source) - for guard in guards: - _unlink_private(guard) raise BackupError("publication_failed") from None - -def _remove_owned_publications(publications, owned_sources): - for destination, original_source in reversed(publications): - sources = [source for source in owned_sources if source.exists()] - if original_source.exists(): - sources.append(original_source) - try: - if destination.exists() and any( - os.path.samefile(destination, source) for source in sources - ): - destination.unlink() - except OSError: - pass + cleanup_failed = False + for source, _destination in pairs: + if not _unlink_private(source): + cleanup_failed = True + if cleanup_failed: + raise BackupError("cleanup_failed") -def _remove_our_publications(publications): +def _remove_owned_publications(publications): for destination, source in reversed(publications): try: - if destination.exists() and os.path.samefile(destination, source): + if source.exists() and destination.exists() and os.path.samefile( + destination, source + ): destination.unlink() except OSError: pass @@ -412,7 +393,11 @@ def _load_manifest(path): raise BackupError("invalid_manifest") from None if not isinstance(manifest, dict) or set(manifest) != _MANIFEST_KEYS: raise BackupError("invalid_manifest") - if manifest["format"] != BACKUP_FORMAT or manifest["version"] != BACKUP_VERSION: + if ( + manifest["format"] != BACKUP_FORMAT + or type(manifest["version"]) is not int + or manifest["version"] != BACKUP_VERSION + ): raise BackupError("invalid_manifest") if ( not isinstance(manifest["application_version"], str) @@ -570,11 +555,16 @@ def run_synthetic_restore_verification(): or _same_database(restore_target, live_target) ): raise BackupError("live_target") - if _read_snapshot(source_target).row_counts: - raise BackupError("source_not_empty") - restore_before = _read_snapshot(restore_target) - if restore_before.migrations or restore_before.row_counts: - raise BackupError("target_not_empty") + try: + if _read_snapshot(source_target).row_counts: + raise BackupError("source_not_empty") + restore_before = _read_snapshot(restore_target) + if restore_before.migrations or restore_before.row_counts: + raise BackupError("target_not_empty") + except BackupError: + raise + except Exception: + raise BackupError("integration_verification_failed") from None default_connection = connections["default"] original_database = dict(default_connection.settings_dict) @@ -684,6 +674,8 @@ def _clear_disposable_database(target): autocommit=True, ) as database: with database.cursor() as cursor: + cursor.execute("SET lock_timeout = '5s'") + cursor.execute("SET statement_timeout = '15s'") cursor.execute("DROP SCHEMA public CASCADE") cursor.execute("CREATE SCHEMA public") except Exception: diff --git a/core/management/commands/test_backup_restore.py b/core/management/commands/test_backup_restore.py index e24957a..4055e91 100644 --- a/core/management/commands/test_backup_restore.py +++ b/core/management/commands/test_backup_restore.py @@ -9,6 +9,7 @@ class Command(BaseCommand): def handle(self, *args, **options): try: result = run_synthetic_restore_verification() + result = run_synthetic_restore_verification() except BackupError as error: raise CommandError(error.code) from None self.stdout.write(f"backup={result.name} format={result.version} verified") diff --git a/tests/test_backup.py b/tests/test_backup.py index 61e3581..8922e1f 100644 --- a/tests/test_backup.py +++ b/tests/test_backup.py @@ -7,6 +7,7 @@ from pathlib import Path from types import SimpleNamespace import pytest +import psycopg from django.db import connection from psycopg import connect, sql @@ -180,7 +181,7 @@ def test_backup_sanitizes_subprocess_failure_and_captures_diagnostics( assert list(backup_root.iterdir()) == [] -def test_second_temporary_unlink_failure_leaves_no_partial_final_pair( +def test_second_temporary_unlink_failure_preserves_complete_committed_pair( backup_root, database_settings, synthetic_snapshot, monkeypatch ): original_unlink = Path.unlink @@ -195,6 +196,25 @@ def test_second_temporary_unlink_failure_leaves_no_partial_final_pair( monkeypatch.setattr(Path, "unlink", fail_once) + with pytest.raises(BackupError, match="cleanup_failed"): + create_backup("synthetic.dump", runner=RecordingRunner()) + + assert (backup_root / "synthetic.dump").read_bytes() == b"PGDMP synthetic" + assert (backup_root / "synthetic.dump.json").is_file() + assert sorted(path.name for path in backup_root.iterdir()) == [ + "synthetic.dump", + "synthetic.dump.json", + ] + + +def test_directory_fsync_failure_rolls_back_both_owned_finals( + backup_root, database_settings, synthetic_snapshot, monkeypatch +): + monkeypatch.setattr( + "core.backup._fsync_directory", + lambda _root: (_ for _ in ()).throw(OSError("synthetic fsync failure")), + ) + with pytest.raises(BackupError, match="publication_failed"): create_backup("synthetic.dump", runner=RecordingRunner()) @@ -409,6 +429,47 @@ def test_manifest_rejects_boolean_integer_fields( verify_backup(result.name, target, runner=RecordingRunner()) +def test_manifest_rejects_boolean_version( + backup_root, database_settings, synthetic_snapshot, settings +): + settings.BACKUP_VERIFY_ALLOWED = True + result = create_backup("synthetic.dump", runner=RecordingRunner()) + manifest = json.loads(result.manifest_path.read_text(encoding="utf-8")) + manifest["version"] = True + result.manifest_path.write_text(json.dumps(manifest), encoding="utf-8") + target = DatabaseTarget( + host="restore-db", port="5432", name="restore", user="user", password="secret" + ) + + with pytest.raises(BackupError, match="invalid_manifest"): + verify_backup(result.name, target, runner=RecordingRunner()) + + +def test_synthetic_preflight_database_error_is_sanitized(monkeypatch, settings): + import core.backup as backup + + settings.BACKUP_VERIFY_ALLOWED = True + for name, value in { + "BACKUP_SOURCE_HOST": "source", + "BACKUP_SOURCE_NAME": "source", + "BACKUP_SOURCE_USER": "user", + "BACKUP_SOURCE_PASSWORD": "SYNTHETIC PRIVATE PASSWORD", + }.items(): + monkeypatch.setenv(name, value) + monkeypatch.setattr( + backup, + "_read_snapshot", + lambda _target: (_ for _ in ()).throw( + psycopg.OperationalError("SYNTHETIC PRIVATE DIAGNOSTIC") + ), + ) + + with pytest.raises(BackupError, match="integration_verification_failed") as error: + backup.run_synthetic_restore_verification() + + assert "PRIVATE" not in str(error.value) + + def test_cleanup_attempts_every_resource_and_reports_safe_failure(monkeypatch, tmp_path): import core.backup as backup diff --git a/tests/test_backup_commands.py b/tests/test_backup_commands.py index ae5b3c1..333f8ee 100644 --- a/tests/test_backup_commands.py +++ b/tests/test_backup_commands.py @@ -116,18 +116,26 @@ def test_verify_backup_converts_service_error_to_safe_code(monkeypatch, settings def test_backup_restore_acceptance_command_prints_only_safe_summary(monkeypatch): - monkeypatch.setattr( - "core.management.commands.test_backup_restore.run_synthetic_restore_verification", - lambda: VerificationResult( + calls = 0 + + def verify(): + nonlocal calls + calls += 1 + return VerificationResult( name="synthetic-restore-check.dump", version=1, row_counts={"finance_account": 1, "finance_transaction": 1}, - ), + ) + + monkeypatch.setattr( + "core.management.commands.test_backup_restore.run_synthetic_restore_verification", + verify, ) stdout = StringIO() call_command("test_backup_restore", stdout=stdout) + assert calls == 2 assert stdout.getvalue().splitlines() == [ "backup=synthetic-restore-check.dump format=1 verified", "table=finance_account rows=1", diff --git a/tests/test_backup_restore.py b/tests/test_backup_restore.py index d925023..098ef04 100644 --- a/tests/test_backup_restore.py +++ b/tests/test_backup_restore.py @@ -1,9 +1,20 @@ +import os import subprocess +import pytest + +from core.backup import ( + DatabaseSnapshot, + DatabaseTarget, + _read_snapshot, + run_synthetic_restore_verification, +) + def test_backup_configuration_is_allow_gated(settings): assert settings.BACKUP_ROOT == "/app/backups" - assert settings.BACKUP_VERIFY_ALLOWED is False + if not os.environ.get("BACKUP_SOURCE_HOST"): + assert settings.BACKUP_VERIFY_ALLOWED is False def test_application_image_installs_matching_postgresql_client(): @@ -12,3 +23,35 @@ def test_application_image_installs_matching_postgresql_client(): ) assert result.stdout.startswith("pg_dump (PostgreSQL) 17.") + + +@pytest.mark.skipif( + not all( + os.environ.get(name) + for name in ( + "BACKUP_SOURCE_HOST", + "BACKUP_SOURCE_NAME", + "BACKUP_SOURCE_USER", + "BACKUP_SOURCE_PASSWORD", + ) + ), + reason="requires the backup profile's disposable databases", +) +def test_synthetic_restore_is_repeatable_and_leaves_both_databases_empty( + settings, django_db_blocker +): + with django_db_blocker.unblock(): + first = run_synthetic_restore_verification() + second = run_synthetic_restore_verification() + source = DatabaseTarget( + host=os.environ["BACKUP_SOURCE_HOST"], + port=os.environ.get("BACKUP_SOURCE_PORT", "5432"), + name=os.environ["BACKUP_SOURCE_NAME"], + user=os.environ["BACKUP_SOURCE_USER"], + password=os.environ["BACKUP_SOURCE_PASSWORD"], + ) + target = DatabaseTarget.from_default(settings.DATABASES["default"]) + empty = DatabaseSnapshot(migrations=(), row_counts={}) + assert first.row_counts == second.row_counts + assert _read_snapshot(source) == empty + assert _read_snapshot(target) == empty