From f4e8dd87746596d0a9121621263d58966d8d8d45 Mon Sep 17 00:00:00 2001 From: jehrr <79864894+jehrr@users.noreply.github.com> Date: Fri, 9 Oct 2026 10:03:06 +0000 Subject: [PATCH] Write JSON, CSV and the sidecar atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit's second finding, and real. open(path, "w") truncates before a single byte is written, so a crash, a kill or a full disk partway through json.dump leaves a SHORTER file where a complete one was. Reproduced here: a 204,292-byte out.json came back 28 bytes and invalid. That breaks the same promise save keeps when it refuses to overwrite good output with an empty result, by a different route — the previous good run destroyed by the ATTEMPT to replace it rather than by its outcome. The sidecar is the half the audit does not mention and the one that matters most: .meta.json is the file a consumer branches on, so a truncated one beside good rows reads as a broken run over data that is fine. All three writers go through one _atomic: temp file in the target's OWN directory (os.replace is atomic only within one filesystem), flush + fsync before the rename, and the temp file unlinked on any exception including KeyboardInterrupt. newline="" is threaded through for the csv module. Lifted from a sibling rather than written here, because the part that is easy to get wrong was got wrong across the family: NamedTemporaryFile creates its file 0600 and a rename keeps that. Measured 2026-10-09 by CALLING each sibling's writer and stat-ing the result — 8 of 11 leave the output 0600, one hardcodes 0644, two derive it from the umask. This takes the third: a new file gets what open() would have given it, and a target somebody tightened keeps its own mode. Five faults planted, suite red on a named check for each. The suite also carries its own control inline: it asserts a truncating writer really does destroy the file before asserting this one does not. One control stayed green first time and is reported rather than smoothed over: dropping newline="" changes nothing on Linux, so that behavioural check cannot fail on CI. The byte assertion is kept and labelled as a sanity check; the guard that can fail is the wiring one beside it. Not included, with the reasoning in the PR: a run identifier for the JSON/CSV/sidecar set (a failed run already writes no sidecar at all), and CSV formula neutralisation (measured 0 of 3,620 string cells on a real run, and a separate decision). Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 36 +++++++++++++ output_writer.py | 70 +++++++++++++++++++++++-- smoke_test.py | 132 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 235 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9562bd1..9f229e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,42 @@ from a row count, it leads the section in a blockquote. ### Fixed +- **An interrupted write destroyed the previous good run's output.** The + audit's second finding, and real. `open(path, "w")` truncates before a + single byte is written, so a crash, a kill or a full disk partway through + `json.dump` left a SHORTER file where a complete one had been. Measured + here before the fix: a 204,292-byte `out.json` came back **28 bytes and + invalid JSON**. + + That breaks the same promise `save` keeps when it refuses to overwrite + good output with an empty result — by a different route, with the previous + run destroyed by the ATTEMPT to replace it rather than by its outcome. The + sidecar is the part that matters most and the part the audit did not + mention: `.meta.json` is the file a consumer branches on, so a + truncated one beside good rows reads as a broken run over data that is + fine. + + All three writers now go through one `_atomic` helper — a temporary file + in the TARGET's own directory (`os.replace` is atomic only within one + filesystem), `flush` + `fsync` before the rename, and the temp file + unlinked on any exception including `KeyboardInterrupt`. + + Lifted from a sibling rather than written here, because the one part that + is easy to get wrong was got wrong across the family: `NamedTemporaryFile` + creates its file `0600` and a rename keeps that, so an output nobody else + can read is the default. Measured 2026-10-09 by CALLING each sibling's + writer and stat-ing the file it produced — **8 of 11 leave their output + 0600**, one hardcodes `0644`, and two derive the mode from the umask. This + takes the third kind: a new file gets what `open()` would have given it, + and a target somebody tightened deliberately keeps its own mode. + + `newline=""` is threaded through for the CSV, or the csv module doubles + the carriage return on every row. + + Checked with its own control: the suite asserts that a truncating writer + really does destroy the file before asserting that this one does not, so + "the file survived" cannot be a property of the test. + - **`--mode events` stopped at the first event page with no markets and reported the run as `complete`.** Found by a third-party audit of 8 October and real. Reproduced against `main` end to end: one empty page at diff --git a/output_writer.py b/output_writer.py index 96b1ea3..6ec0a21 100644 --- a/output_writer.py +++ b/output_writer.py @@ -64,8 +64,12 @@ README too, because a reader who joins on `sku` across a re-slug gets two rows and no warning. """ +import contextlib import csv import json +import os +import stat +import tempfile from dataclasses import dataclass, asdict, field, fields from datetime import datetime, timezone from typing import Optional, List, Set, Sequence, Any, Type @@ -330,8 +334,68 @@ def _csv_value(v: Any) -> Any: return v +@contextlib.contextmanager +def _atomic(path: str, newline: Optional[str] = None): + """Write to a temporary file beside `path`, then rename over it. + + LIFTED from a sibling repo rather than written here. This is an + un-propagated family fix, not a design question, and the one part of it + that is easy to get wrong was got wrong in eight sibling repos — see the + mode note below. Measured across the family on 2026-10-09 by CALLING + each sibling's writer and stat-ing the file it produced: 8 of 11 atomic + implementations leave their output 0600, one hardcodes 0644, and two + derive it from the umask. This is one of the two. + + Plain truncating `open` empties the file before a single byte is + written, so a crash, a kill or a full disk halfway through `json.dump` + leaves a SHORTER file where a complete one was. That breaks the promise + `save` keeps when it refuses to overwrite good output with an empty + result, by a different route: the previous good run is destroyed by the + ATTEMPT to replace it rather than by its outcome. + + The sidecar matters most, and the audit that raised this did not mention + it: `.meta.json` is the file a consumer branches on, so a truncated + one beside good rows reads as a broken run over data that is fine. + + The temporary file goes in the SAME directory on purpose: `os.replace` + is atomic only within one filesystem, and a temp file under /tmp can be + on another one, where the rename degrades to an interruptible copy. + `fsync` before the rename makes the content durable, not merely visible. + + `NamedTemporaryFile` creates the file 0600 and a rename keeps that, so + the mode is set explicitly: an existing target keeps its own (someone + may have tightened it deliberately), and a new one gets what `open()` + would have given it. A flat 0644 would be wrong in both directions. + """ + 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 = stat.S_IMODE(os.stat(path).st_mode) + except OSError: + umask = os.umask(0) + os.umask(umask) + mode = 0o666 & ~umask + os.chmod(handle.name, mode) + os.replace(handle.name, path) + except BaseException: + # Including KeyboardInterrupt and SystemExit: an interrupted run must + # not leave a stray .tmp beside the output it did not replace. + 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) @@ -344,7 +408,7 @@ def write_csv(rows: Sequence[Any], path: str, row_cls: Type = Product) -> None: # The header comes from `row_cls`, not from the first row, so an empty # run still writes the columns of the mode that produced it. fieldnames = [f.name for f in fields(row_cls)] - 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: @@ -433,7 +497,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 32ec456..6fab859 100644 --- a/smoke_test.py +++ b/smoke_test.py @@ -1135,6 +1135,137 @@ def test_one_page_is_complete_not_partial(): return ok +def test_writes_are_atomic(): + group("A write that dies partway leaves the previous good file alone") + import contextlib + import json as _json + import output_writer + import stat as _stat + import tempfile + ok = True + + # The defect, stated as what it destroys: `open(path, "w")` truncates + # before a single byte is written, so a crash, a kill or a full disk + # halfway through `json.dump` leaves a SHORTER file where a complete one + # was. `save` already refuses to overwrite good output with an EMPTY + # result; this is the same promise broken by a different route — the + # previous good run destroyed by the attempt to replace it rather than + # by its outcome. + # + # Measured on this repo before the fix: a 204,292-byte good `out.json` + # came back 28 bytes and invalid. + rows = [Market(sku="%012x" % i, url="https://polymarket.com/event/e", + title="market %d" % i) for i in range(200)] + + class _Boom(Exception): + pass + + def _die_partway(obj, fp, *a, **k): + # Signature matches json.dump(obj, fp, ...): the FILE is second. + fp.write('[\n {"sku": "000000000000",\n') + raise _Boom("interrupted mid-write") + + @contextlib.contextmanager + def _truncating(path, newline=None): + with open(path, "w", encoding="utf-8", newline=newline) as fh: + yield fh + + def _survives(writer): + """Good file size, size after an interrupted write, stray files.""" + directory = tempfile.mkdtemp() + target = os.path.join(directory, "out.json") + real_atomic, real_dump = output_writer._atomic, _json.dump + output_writer._atomic = writer + try: + output_writer.write_json(rows, target) + good = os.path.getsize(target) + _json.dump = _die_partway + try: + output_writer.write_json(rows, target) + except _Boom: + pass + finally: + _json.dump = real_dump + after = os.path.getsize(target) + try: + _json.load(open(target, encoding="utf-8")) + valid = True + except Exception: # noqa: BLE001 — "did it parse" is the question + valid = False + strays = [n for n in os.listdir(directory) if n != "out.json"] + return good, after, valid, strays + finally: + output_writer._atomic = real_atomic + + # The CONTROL first: without it, "the file survived" could be a property + # of the test rather than of the writer. + good, after, valid, _ = _survives(_truncating) + ok &= check("control: a truncating writer really does destroy it " + "(%d bytes -> %d, parses: %s)" % (good, after, valid), + after < good and not valid) + + good, after, valid, strays = _survives(output_writer._atomic) + ok &= check("the previous good file survives an interrupted write", + after == good and valid) + ok &= check("...and no half-written temp file is left beside it", + strays == []) + + # The half eight sibling repos get wrong. NamedTemporaryFile creates its + # file 0600 and a rename keeps that, so an output nobody else can read + # is the default unless the mode is set back deliberately. + directory = tempfile.mkdtemp() + target = os.path.join(directory, "mode.json") + previous = os.umask(0o022) + try: + output_writer.write_json(rows, target) + mode = _stat.S_IMODE(os.stat(target).st_mode) + ok &= check("a new output is readable, not 0600 (got %s under umask " + "022)" % oct(mode), mode == 0o644) + os.chmod(target, 0o600) + output_writer.write_json(rows, target) + kept = _stat.S_IMODE(os.stat(target).st_mode) + ok &= check("...and a target somebody tightened keeps its own mode", + kept == 0o600) + finally: + os.umask(previous) + + # `newline=""` has to survive the move into the temp file, or the csv + # module's own "\r\n" is translated again and every row gains a doubled + # carriage return. + # + # The BEHAVIOUR cannot be tested here, and that is worth stating rather + # than leaving as a check that looks load-bearing: the translation only + # happens where os.linesep is "\r\n", so on Linux — and therefore on CI + # — dropping `newline=""` changes the bytes not at all. Planting that + # fault left the suite green, which is a hole in the suite and not a + # pass. So the row count below is a sanity check, and the assertion that + # can actually fail is the WIRING one after it. + csv_path = os.path.join(directory, "out.csv") + output_writer.write_csv(rows[:3], csv_path, row_cls=Market) + raw = open(csv_path, "rb").read() + ok &= check("the CSV has one line terminator per row plus the header", + raw.count(b"\r\n") == 4 and raw.count(b"\r\r\n") == 0) + + # And the wiring: a writer that quietly goes back to `open` would pass + # every check above that does not call it. + source = open(os.path.join(REPO_ROOT, "output_writer.py"), + encoding="utf-8").read() + tree = ast.parse(source) + for node in ast.walk(tree): + if not isinstance(node, ast.FunctionDef): + continue + if node.name not in ("write_json", "write_csv", "write_run_meta"): + continue + body = ast.get_source_segment(source, node) or "" + ok &= check("%s() writes through _atomic" % node.name, + "_atomic(" in body and 'open(' not in body) + if node.name == "write_csv": + # The one that a Linux run cannot catch by behaviour. + ok &= check("...and hands it newline='' for the csv module", + 'newline=""' in body) + return ok + + def test_output_contract(): group("The output contract (§9)") ok = True @@ -2857,6 +2988,7 @@ def main() -> int: ok &= test_page_flow_policy() ok &= test_scroll_loop() ok &= test_one_page_is_complete_not_partial() + ok &= test_writes_are_atomic() ok &= test_output_contract() ok &= test_writers_and_finish_run() ok &= test_diff()