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()