From 7538e739b9f79b80f4b78edad01443824f129958 Mon Sep 17 00:00:00 2001 From: Codex Date: Tue, 8 Sep 2026 05:44:20 -0300 Subject: [PATCH] fix: publish exports without clobbering --- finance/exports.py | 34 +++-------- tests/finance/test_exports.py | 104 ++++++++++++++++++++++++++++++++-- tests/test_export_command.py | 61 +++++++++++++++++++- 3 files changed, 164 insertions(+), 35 deletions(-) diff --git a/finance/exports.py b/finance/exports.py index 23816f1..8ede348 100644 --- a/finance/exports.py +++ b/finance/exports.py @@ -194,28 +194,12 @@ def write_export(name: str | None = None) -> ExportResult: destination = root / name if destination.is_symlink(): raise ExportError("invalid_name") + if destination.exists(): + raise ExportError("existing_destination") - reservation_fd = None - owns_reservation = False temporary_path = None - published = False - phase = "reservation" + phase = "snapshot" try: - try: - reservation_fd = os.open( - destination, - os.O_WRONLY | os.O_CREAT | os.O_EXCL, - 0o600, - ) - owns_reservation = True - except FileExistsError: - raise ExportError("target_exists") from None - finally: - if reservation_fd is not None: - os.close(reservation_fd) - reservation_fd = None - - phase = "snapshot" artifact = build_export() phase = "publication" temporary_fd, temporary_name = tempfile.mkstemp( @@ -226,9 +210,12 @@ def write_export(name: str | None = None) -> ExportResult: stream.write(artifact.content) stream.flush() os.fsync(stream.fileno()) - os.replace(temporary_path, destination) + try: + os.link(temporary_path, destination) + except FileExistsError: + raise ExportError("existing_destination") from None + temporary_path.unlink() temporary_path = None - published = True return ExportResult( name=name, path=destination, @@ -246,11 +233,6 @@ def write_export(name: str | None = None) -> ExportResult: temporary_path.unlink(missing_ok=True) except OSError: pass - if owns_reservation and not published: - try: - destination.unlink(missing_ok=True) - except OSError: - pass def _csv_content(model, fields): diff --git a/tests/finance/test_exports.py b/tests/finance/test_exports.py index 517bf8a..0949c61 100644 --- a/tests/finance/test_exports.py +++ b/tests/finance/test_exports.py @@ -1,5 +1,6 @@ import csv import json +import threading from datetime import date, datetime, timezone as datetime_timezone from decimal import Decimal from hashlib import sha256 @@ -8,7 +9,9 @@ from uuid import UUID from zipfile import ZipFile import pytest +from django.db import connections, transaction +import finance.exports as exports from finance.exports import build_export from finance.models import ( Account, @@ -35,11 +38,26 @@ EXPECTED_FILES = [ "transfers.csv", "source_records.csv", ] -EXPECTED_TRANSACTION_HEADER = ( - "id,account_id,amount,posting_date,occurrence_date,description,kind,payee_id," - "category_id,statement_id,refund_of_id,installment_plan_id,installment_number," - "created_at,updated_at" -) +EXPECTED_HEADERS = { + "accounts.csv": "id,name,kind,currency,archived,created_at,updated_at", + "payees.csv": "id,name,archived,created_at,updated_at", + "categories.csv": "id,name,kind,archived,created_at,updated_at", + "statements.csv": ( + "id,account_id,period_start,period_end,due_date,statement_balance,created_at,updated_at" + ), + "installment_plans.csv": ( + "id,account_id,description,total_installments,created_at,updated_at" + ), + "transactions.csv": ( + "id,account_id,amount,posting_date,occurrence_date,description,kind,payee_id," + "category_id,statement_id,refund_of_id,installment_plan_id,installment_number," + "created_at,updated_at" + ), + "transfers.csv": "id,outgoing_id,incoming_id,created_at,updated_at", + "source_records.csv": ( + "id,transaction_id,document_hash,source_format,parser_version,item_locator,created_at" + ), +} def rows(package, name): @@ -156,9 +174,9 @@ def test_export_has_versioned_manifest_and_stable_csv_contract(): assert manifest["version"] == 1 assert manifest["application_version"] == "0.1.0" assert list(manifest["files"]) == EXPECTED_FILES - assert package.read("transactions.csv").decode("utf-8").splitlines()[0] == EXPECTED_TRANSACTION_HEADER for name in EXPECTED_FILES: content = package.read(name) + assert content.decode("utf-8").splitlines()[0] == EXPECTED_HEADERS[name] assert manifest["files"][name]["sha256"] == sha256(content).hexdigest() assert manifest["files"][name]["rows"] == len(rows(package, name)) @@ -218,3 +236,77 @@ def test_export_is_byte_stable_for_the_same_snapshot(monkeypatch): assert first.filename == second.filename == "gizmoduck-export-20260907T123000Z.zip" assert first.content == second.content + + +def test_export_uses_one_repeatable_read_snapshot_across_related_files(monkeypatch): + seed_complete_export() + accounts_read = threading.Event() + writer_finished = threading.Event() + writer_errors = [] + new_account_id = UUID("10000000-0000-4000-8000-000000000003") + new_payee_id = UUID("20000000-0000-4000-8000-000000000002") + new_transaction_id = UUID("60000000-0000-4000-8000-000000000005") + new_source_id = UUID("80000000-0000-4000-8000-000000000002") + original_csv_content = exports._csv_content + + def pause_after_accounts(model, fields): + content = original_csv_content(model, fields) + if model is Account: + accounts_read.set() + assert writer_finished.wait(10), "separate writer did not finish" + return content + + def commit_related_rows(): + try: + assert accounts_read.wait(10), "export did not read accounts" + with transaction.atomic(): + account = Account.objects.create( + id=new_account_id, + name="Synthetic Concurrent Account", + kind=Account.Kind.BANK, + currency="BRL", + ) + payee = Payee.objects.create( + id=new_payee_id, + name="Synthetic Concurrent Payee", + ) + row = Transaction.objects.create( + id=new_transaction_id, + account=account, + amount=Decimal("-7.25"), + posting_date=date(2026, 3, 1), + description="Synthetic concurrent transaction", + kind=Transaction.Kind.EXPENSE, + payee=payee, + ) + SourceRecord.objects.create( + id=new_source_id, + transaction=row, + document_hash="b" * 64, + source_format=SourceRecord.Format.BANK_CSV, + parser_version="synthetic-v2", + item_locator="line:3", + ) + except BaseException as exc: + writer_errors.append(exc) + finally: + connections.close_all() + writer_finished.set() + + monkeypatch.setattr(exports, "_csv_content", pause_after_accounts) + writer = threading.Thread(target=commit_related_rows) + writer.start() + artifact = build_export() + writer.join(10) + + assert not writer.is_alive() + assert writer_errors == [] + with ZipFile(BytesIO(artifact.content)) as package: + assert str(new_account_id) not in {row["id"] for row in rows(package, "accounts.csv")} + assert str(new_payee_id) not in {row["id"] for row in rows(package, "payees.csv")} + assert str(new_transaction_id) not in { + row["id"] for row in rows(package, "transactions.csv") + } + assert str(new_source_id) not in { + row["id"] for row in rows(package, "source_records.csv") + } diff --git a/tests/test_export_command.py b/tests/test_export_command.py index d718e2f..1762f7a 100644 --- a/tests/test_export_command.py +++ b/tests/test_export_command.py @@ -1,4 +1,5 @@ import json +import threading from datetime import datetime, timezone as datetime_timezone from io import StringIO @@ -67,12 +68,66 @@ def test_write_export_rejects_existing_target_before_snapshot(export_root, monke monkeypatch.setattr("finance.exports.build_export", unexpected_snapshot) - with pytest.raises(ExportError, match="target_exists"): + with pytest.raises(ExportError, match="existing_destination"): write_export(existing.name) assert existing.read_bytes() == b"original" +def test_write_export_does_not_expose_final_name_while_snapshot_is_built( + export_root, monkeypatch +): + destination = export_root / "private-until-complete.zip" + + def inspect_during_snapshot(): + assert not destination.exists() + return synthetic_artifact() + + monkeypatch.setattr("finance.exports.build_export", inspect_during_snapshot) + + write_export(destination.name) + + assert destination.read_bytes() == b"synthetic zip bytes" + + +def test_write_export_preserves_final_file_created_concurrently( + export_root, monkeypatch +): + destination = export_root / "concurrent.zip" + snapshot_started = threading.Event() + competing_file_created = threading.Event() + writer_errors = [] + + def concurrent_snapshot(): + snapshot_started.set() + assert competing_file_created.wait(5), "competing writer did not finish" + return synthetic_artifact() + + def publish_competing_file(): + try: + assert snapshot_started.wait(5), "export did not start its snapshot" + destination.write_bytes(b"concurrent writer") + except BaseException as exc: + writer_errors.append(exc) + finally: + competing_file_created.set() + + monkeypatch.setattr("finance.exports.build_export", concurrent_snapshot) + writer = threading.Thread(target=publish_competing_file) + writer.start() + + try: + with pytest.raises(ExportError, match="existing_destination"): + write_export(destination.name) + finally: + writer.join(5) + + assert not writer.is_alive() + assert writer_errors == [] + assert destination.read_bytes() == b"concurrent writer" + assert list(export_root.iterdir()) == [destination] + + def test_write_export_rejects_destination_symlink(export_root, monkeypatch): outside = export_root.parent / "outside-export.zip" outside.write_bytes(b"outside original") @@ -108,10 +163,10 @@ def test_write_export_removes_reservation_and_temporary_file_on_snapshot_failure def test_write_export_cleans_up_if_atomic_publication_fails(export_root, monkeypatch): monkeypatch.setattr("finance.exports.build_export", synthetic_artifact) - def failed_replace(_source, _destination): + def failed_link(_source, _destination): raise OSError("SYNTHETIC PRIVATE FILESYSTEM DETAIL") - monkeypatch.setattr("finance.exports.os.replace", failed_replace) + monkeypatch.setattr("finance.exports.os.link", failed_link) with pytest.raises(ExportError, match="publication_failed") as caught: write_export("failed.zip")