From f38e98012df41b1fee6dfd63cb27d09e28364827 Mon Sep 17 00:00:00 2001 From: tpateeq Date: Thu, 16 Jul 2026 23:52:48 +0530 Subject: [PATCH] fix(io): write graph.json and manifest.json atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit graph.json (the clustered `to_json` write and the `--no-cluster`/merge raw dumps) and manifest.json were written with a direct `open()`/`write_text`, so a crash, kill, or disk-full mid-write left a truncated, unparseable file that the next load or `detect_incremental` then failed on. Add `write_text_atomic`/`write_json_atomic` in graphify.paths (temp file in the same directory + `os.replace`; JSON is streamed into the temp, not materialized as one string) and route the graph.json writers (export.to_json, cli._prune_graph_json_sources, the merge driver) plus detect.save_manifest through them. The helper preserves the destination's mode (an atomic replace never tightens 0644 to mkstemp's 0600), writes through a symlinked destination (shared-output setups), and falls back to copy-then-delete on a Windows os.replace lock — matching graphify.cache's existing atomic writer. On failure the previous file is left intact and the temp removed. Not a power-loss durability guarantee (no fsync, consistent with the rest of the codebase). --- graphify/cli.py | 6 ++- graphify/detect.py | 6 ++- graphify/export.py | 5 +- graphify/paths.py | 69 ++++++++++++++++++++++++ tests/test_atomic_writes.py | 101 ++++++++++++++++++++++++++++++++++++ 5 files changed, 181 insertions(+), 6 deletions(-) create mode 100644 tests/test_atomic_writes.py diff --git a/graphify/cli.py b/graphify/cli.py index e499ba809..dd1263ae6 100644 --- a/graphify/cli.py +++ b/graphify/cli.py @@ -246,7 +246,8 @@ def _prune_graph_json_sources(graph_path: Path, stale_sources: list[str]) -> int data["hyperedges"] = kept_hyper from graphify.export import backup_if_protected as _backup _backup(graph_path.parent) - graph_path.write_text(json.dumps(data, indent=2), encoding="utf-8") + from graphify.paths import write_json_atomic + write_json_atomic(graph_path, data, indent=2) return n_removed @@ -1615,7 +1616,8 @@ def dispatch_command(cmd: str) -> None: out_data = _jg.node_link_data(merged, edges="links") except TypeError: out_data = _jg.node_link_data(merged) - Path(_current_path).write_text(json.dumps(out_data, indent=2), encoding="utf-8") + from graphify.paths import write_json_atomic + write_json_atomic(_current_path, out_data, indent=2) sys.exit(0) elif cmd == "merge-graphs": diff --git a/graphify/detect.py b/graphify/detect.py index ce1aad856..766472410 100644 --- a/graphify/detect.py +++ b/graphify/detect.py @@ -1597,8 +1597,10 @@ def save_manifest( # their absolute form so the manifest round-trips on the saving # machine even when not every entry can be portably encoded. manifest = {_to_relative_for_storage(k, root): v for k, v in manifest.items()} - Path(manifest_path).parent.mkdir(parents=True, exist_ok=True) - Path(manifest_path).write_text(json.dumps(manifest, indent=2), encoding="utf-8") + from graphify.paths import write_json_atomic + # Atomic write: a crash mid-write must not leave a truncated manifest that + # detect_incremental then fails to parse. + write_json_atomic(manifest_path, manifest, indent=2) def detect_incremental( diff --git a/graphify/export.py b/graphify/export.py index d6e982565..e1f2caa99 100644 --- a/graphify/export.py +++ b/graphify/export.py @@ -315,8 +315,9 @@ def to_json(G: nx.Graph, communities: dict[int, list[str]], output_path: str, *, commit = built_at_commit if built_at_commit is not None else _git_head() if commit: data["built_at_commit"] = commit - with open(output_path, "w", encoding="utf-8") as f: # nosec - json.dump(data, f, indent=2) + from graphify.paths import write_json_atomic + # Atomic write: a crash/ENOSPC mid-write must not truncate a good graph.json. + write_json_atomic(output_path, data, indent=2) return True diff --git a/graphify/paths.py b/graphify/paths.py index d2bfdd9f5..4e42c1406 100644 --- a/graphify/paths.py +++ b/graphify/paths.py @@ -16,12 +16,81 @@ flow) and every reader honours it. from __future__ import annotations +import json import os import re +import stat +import tempfile from pathlib import Path, PurePosixPath GRAPHIFY_OUT = os.environ.get("GRAPHIFY_OUT", "graphify-out") + +def _atomic_replace(path: "str | Path", write_fn) -> None: + """Atomically replace ``path`` with content written by ``write_fn(f)``. + + Writes a temp file in the SAME directory, then ``os.replace``s it into place + (an atomic rename on one filesystem). A process kill (SIGKILL/Ctrl-C), OOM, or + ENOSPC mid-write leaves the previous file intact — the destination is + untouched until the rename. This is NOT a power-loss durability guarantee: + there is no fsync (matching the rest of the codebase), so an OS/hardware crash + right after the rename can still expose unflushed bytes on some filesystems. + The temp file is removed if the write fails. + + A symlinked destination is resolved first so the write goes THROUGH the link + to its target (rather than replacing the link with a regular file), keeping + the shared-output/worktree symlink setups this module documents working. + """ + # Resolve symlinks so the temp lands on the target's filesystem (same-fs + # atomic rename) and the replace writes through the link, not over it. + real = Path(os.path.realpath(str(path))) + real.parent.mkdir(parents=True, exist_ok=True) + fd, tmp = tempfile.mkstemp(dir=str(real.parent), prefix=f".{real.name}.", suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + write_fn(f) + # mkstemp creates the temp file 0600; match the destination's existing + # mode (or the umask default for a new file) so an atomic replace never + # silently tightens a previously group/world-readable output to + # owner-only. Best-effort — a chmod failure must not fail the write. + try: + mode = stat.S_IMODE(os.stat(real).st_mode) + except OSError: + umask = os.umask(0) + os.umask(umask) + mode = 0o666 & ~umask + try: + os.chmod(tmp, mode) + except OSError: + pass + try: + os.replace(tmp, str(real)) + except PermissionError: + # Windows: os.replace fails (WinError 5/32) when the destination is + # briefly locked by another handle (antivirus, an open reader). Fall + # back to copy-then-delete, matching graphify.cache's atomic writer. + import shutil + shutil.copy2(tmp, str(real)) + os.unlink(tmp) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise + + +def write_text_atomic(path: "str | Path", text: str) -> None: + """Atomically write ``text`` (UTF-8) to ``path``. See :func:`_atomic_replace`.""" + _atomic_replace(path, lambda f: f.write(text)) + + +def write_json_atomic(path: "str | Path", obj, *, indent: "int | None" = None) -> None: + """Atomically write ``obj`` as JSON to ``path``, streaming the encode into the + temp file rather than materializing the whole string first (matters for very + large graphs). See :func:`_atomic_replace`.""" + _atomic_replace(path, lambda f: json.dump(obj, f, indent=indent)) + # Directory segments that, when they appear as a whole path component, mark the # whole path as a test location. Matched against path *segments* (not raw # substrings) so "src/contest.py" / "latest/x.py" / "src/greatest/x.py" do NOT diff --git a/tests/test_atomic_writes.py b/tests/test_atomic_writes.py new file mode 100644 index 000000000..feb035783 --- /dev/null +++ b/tests/test_atomic_writes.py @@ -0,0 +1,101 @@ +"""Tests for atomic JSON writes (graph.json / manifest.json). + +A crash, kill, or disk-full mid-write must not leave a truncated/corrupt file +that a later load chokes on. `write_text_atomic` writes a temp file in the same +directory then `os.replace`s it into place; on failure the original is untouched. +""" +import json +import os + +import pytest + +from graphify.paths import write_text_atomic + + +def test_write_text_atomic_writes_and_leaves_no_tmp(tmp_path): + p = tmp_path / "out" / "graph.json" # parent doesn't exist yet + write_text_atomic(p, '{"a": 1}') + assert json.loads(p.read_text()) == {"a": 1} + # No leftover temp file in the target directory. + assert [x.name for x in p.parent.iterdir()] == ["graph.json"] + + +def test_write_text_atomic_preserves_existing_on_failure(tmp_path, monkeypatch): + p = tmp_path / "graph.json" + p.write_text("original", encoding="utf-8") + + def boom(src, dst): + raise OSError("simulated disk full") + + monkeypatch.setattr(os, "replace", boom) + with pytest.raises(OSError): + write_text_atomic(p, "content-that-must-not-land") + + # The original file is intact and the temp file was cleaned up. + assert p.read_text() == "original" + assert sorted(x.name for x in tmp_path.iterdir()) == ["graph.json"] + + +def test_write_text_atomic_preserves_existing_mode(tmp_path): + # An atomic replace must not tighten a 0644 file to mkstemp's 0600 default. + p = tmp_path / "graph.json" + p.write_text("{}", encoding="utf-8") + os.chmod(p, 0o644) + write_text_atomic(p, '{"x": 1}') + assert (os.stat(p).st_mode & 0o777) == 0o644 + + +def test_write_text_atomic_new_file_respects_umask(tmp_path): + # A brand-new file must land at the umask default (e.g. 0644), NOT mkstemp's + # 0600 — otherwise every fresh graph.json would be owner-only. + p = tmp_path / "new.json" + write_text_atomic(p, "{}") + umask = os.umask(0) + os.umask(umask) + assert (os.stat(p).st_mode & 0o777) == (0o666 & ~umask) + + +def test_write_text_atomic_writes_through_symlink(tmp_path): + # Shared-output setups symlink graph.json to shared storage; the atomic write + # must update the target and keep the link, not replace it with a real file. + target = tmp_path / "real.json" + target.write_text("old", encoding="utf-8") + link = tmp_path / "link.json" + link.symlink_to(target) + write_text_atomic(link, "new") + assert link.is_symlink() + assert target.read_text() == "new" + + +def test_write_json_atomic_roundtrip(tmp_path): + from graphify.paths import write_json_atomic + + p = tmp_path / "g.json" + write_json_atomic(p, {"nodes": [1, 2], "x": "é"}, indent=2) + assert json.loads(p.read_text()) == {"nodes": [1, 2], "x": "é"} + assert not any(name.name.endswith(".tmp") for name in tmp_path.iterdir()) + + +def test_to_json_writes_atomically_no_tmp_leftover(tmp_path): + import networkx as nx + from graphify.export import to_json + + G = nx.Graph() + G.add_node("a", label="a", file_type="code") + G.add_node("b", label="b", file_type="code") + G.add_edge("a", "b") + out = tmp_path / "graph.json" + assert to_json(G, {}, str(out), force=True) is True + json.loads(out.read_text()) # valid JSON + assert not any(x.name.endswith(".tmp") for x in tmp_path.iterdir()) + + +def test_save_manifest_writes_atomically(tmp_path): + from graphify.detect import save_manifest + + (tmp_path / "a.py").write_text("x = 1\n", encoding="utf-8") + mpath = tmp_path / "graphify-out" / "manifest.json" + save_manifest({"code": [str(tmp_path / "a.py")]}, manifest_path=str(mpath), + kind="both", root=tmp_path) + assert json.loads(mpath.read_text()) # non-empty, valid JSON + assert not any(x.name.endswith(".tmp") for x in mpath.parent.iterdir())