From d159624a8863e7b599656498d1b2ec8faa8270f1 Mon Sep 17 00:00:00 2001 From: jehrr <79864894+jehrr@users.noreply.github.com> Date: Thu, 8 Oct 2026 14:12:25 +0000 Subject: [PATCH] Write outputs atomically; test Python 3.13 in CI Two findings of the 2026-10-08 audit that held up against the code. Co-Authored-By: Claude Sonnet 5.5 --- .github/workflows/tests.yml | 2 +- CHANGELOG.md | 11 +++++++++ output_writer.py | 49 ++++++++++++++++++++++++++++++++++--- smoke_test.py | 41 +++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 4 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 85e3084..b22b510 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -17,7 +17,7 @@ jobs: matrix: # 3.9 is the floor the README claims. Claiming it without testing it is # how a walrus operator or a `X | None` annotation ships and breaks it. - python-version: ["3.9", "3.12"] + python-version: ["3.9", "3.13"] steps: - uses: actions/checkout@v4 diff --git a/CHANGELOG.md b/CHANGELOG.md index fd5ee29..8c7b1b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,6 +73,17 @@ user would notice, the release notes say so first. `page_flow.attempts` in every entry point; a check reads the source of each one and calls the validator, so a new engine declaring the flag with a plain int fails the suite. +- **Output files are written atomically.** `write_json`, `write_csv` and the + `.meta.json` sidecar opened their target with mode `w`, which truncates + before a byte is written, so a crash, a kill or a full disk halfway through + left a shorter file where a complete one had been, and a truncated sidecar + beside good rows. They now write a temp file in the target's own directory, + `fsync`, and `os.replace` it; an existing file keeps its mode and a new one + gets what `open()` would have given it. The offline suite checks a failed + write leaves the previous file byte-identical and no temp file behind. +- **CI now tests the upper end of the Python range the README claims.** The + offline matrix ran 3.9 and 3.12 while the README said "both ends are tested" + for 3.9 through 3.13; it now runs 3.9 and 3.13. - **The Scraper API path (`scraper_api_client.py`) failed whenever a wait flag was given, and never saw the target's status.** Measured 2026-09-23 against diff --git a/output_writer.py b/output_writer.py index b19066f..8531c92 100644 --- a/output_writer.py +++ b/output_writer.py @@ -20,8 +20,11 @@ gets the right header for the mode that produced it. """ +import contextlib import csv import json +import os +import tempfile from dataclasses import dataclass, asdict, field, fields from datetime import datetime, timezone from typing import Optional, List, Set, Sequence, Any, Type @@ -348,8 +351,48 @@ def _csv_escape(v: Any) -> Any: return v, False +@contextlib.contextmanager +def _atomic(path: str, newline: Optional[str] = None): + """Write to a temporary file beside `path`, then rename over it. + + A kill, a full disk or a crash halfway through `json.dump` would leave a + TRUNCATED file where last night's good one was (`save` refuses to + overwrite good data with an empty result for the same reason), and the + sidecar beside it would still describe the old run. `os.replace` is + atomic on POSIX and Windows, but only within one filesystem, hence the + temp file in the target's OWN directory; `fsync` first, so the rename + cannot reach the disk before the bytes do. + + The temp file is created 0600, so the mode is set explicitly: an existing + file keeps its own, a new one gets what `open()` would have given it. + """ + directory = os.path.dirname(os.path.abspath(path)) or "." + handle = tempfile.NamedTemporaryFile( + mode="w", encoding="utf-8", newline=newline, dir=directory, + prefix=os.path.basename(path) + ".", suffix=".tmp", delete=False) + try: + with handle: + yield handle + handle.flush() + os.fsync(handle.fileno()) + try: + mode = os.stat(path).st_mode & 0o7777 + except OSError: + umask = os.umask(0) + os.umask(umask) + mode = 0o666 & ~umask + os.chmod(handle.name, mode) + os.replace(handle.name, path) + except BaseException: + try: + os.unlink(handle.name) + except OSError: + pass + raise + + def write_json(rows: Sequence[Any], path: str) -> None: - with open(path, "w", encoding="utf-8") as f: + with _atomic(path) as f: json.dump([asdict(r) for r in rows], f, ensure_ascii=False, indent=2) @@ -365,7 +408,7 @@ def write_csv(rows: Sequence[Any], path: str, row_cls: Type = Product) -> int: # Returns how many cells were formula-escaped (see _csv_escape). fieldnames = [f.name for f in fields(row_cls)] escaped = 0 - with open(path, "w", encoding="utf-8", newline="") as f: + with _atomic(path, newline="") as f: writer = csv.DictWriter(f, fieldnames=fieldnames) writer.writeheader() for r in rows: @@ -461,7 +504,7 @@ def write_run_meta(out_prefix: str, meta: dict) -> str: both complete, and between runs of different `mode`. """ path = f"{out_prefix}.meta.json" - with open(path, "w", encoding="utf-8") as f: + with _atomic(path) as f: json.dump(meta, f, ensure_ascii=False, indent=2) print(f"[+] Wrote run metadata -> {path} (status={meta.get('status')})") return path diff --git a/smoke_test.py b/smoke_test.py index 5e89590..e15e9e7 100644 --- a/smoke_test.py +++ b/smoke_test.py @@ -3472,6 +3472,46 @@ def test_engine_fetch_paths(skips): return ok +def test_writes_are_atomic(): + """A failed write must leave the previous good file whole, and a + successful one must not leave a 0600 file or a stray temp file.""" + import tempfile + import output_writer as ow + ok = True + with tempfile.TemporaryDirectory() as d: + path = os.path.join(d, "out.json") + ow.write_json([Product(sku="a", url="u", title="t")], path) + good = open(path, encoding="utf-8").read() + os.chmod(path, 0o640) + + class Boom: # asdict() raises halfway through building the payload + pass + try: + ow.write_json([Boom()], path) + raised = False + except Exception: + raised = True + ok &= check("a write that fails raises", raised) + ok &= check("a failed write leaves the previous file byte-identical", + open(path, encoding="utf-8").read() == good) + ok &= check("a failed write leaves no temp file behind", + os.listdir(d) == ["out.json"]) + + ow.write_json([Product(sku="b", url="u", title="t")], path) + ok &= check("an overwrite keeps the existing file's mode", + os.stat(path).st_mode & 0o777 == 0o640) + fresh = os.path.join(d, "new.csv") + ow.write_csv([], fresh) + umask = os.umask(0) + os.umask(umask) + ok &= check("a new file gets the mode open() would have given it", + os.stat(fresh).st_mode & 0o777 == 0o666 & ~umask) + src = inspect.getsource(ow) + ok &= check("no writer opens its target for writing in place", + 'open(path, "w"' not in src) + return ok + + def main() -> int: ok = True # Checks that could not run because an optional engine library is absent. @@ -3493,6 +3533,7 @@ def main() -> int: ok &= test_page_flow() ok &= test_output_contract() ok &= test_writers() + ok &= test_writes_are_atomic() ok &= test_finish_run() ok &= test_diff() ok &= test_captcha()