From e93b1a3561441101894a4b064492ee2cc7579002 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Fri, 5 Jun 2026 10:48:17 -0700 Subject: [PATCH 1/2] Write VRT index atomically via temp-then-rename (#2965) write_vrt wrote the final .vrt in place. On the tiled path the tile directory is promoted with an atomic rename before the index is written, so the index was the last non-atomic step and an interrupted write could leave a partial .vrt pointing at a complete tile set. Route the index write through the existing _write_bytes helper, which writes to a temp file in the destination directory and os.replace's it into place. Add tests covering temp-file cleanup, no partial file on write failure, and preservation of an existing index on a failed rewrite. --- xrspatial/geotiff/_vrt.py | 10 ++- xrspatial/geotiff/tests/write/test_basic.py | 72 +++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/xrspatial/geotiff/_vrt.py b/xrspatial/geotiff/_vrt.py index e3bb67bb..b67576ba 100644 --- a/xrspatial/geotiff/_vrt.py +++ b/xrspatial/geotiff/_vrt.py @@ -2242,7 +2242,13 @@ def _pixel_size_mismatch(a: float, b: float) -> bool: lines.append('') xml = '\n'.join(lines) + '\n' - with open(vrt_path, 'w') as f: - f.write(xml) + # Write the index atomically: a temp file in the destination + # directory followed by ``os.replace``. The tiled writer promotes the + # tile directory before this call, so a direct in-place write here is + # the last non-atomic step and an interrupted write would leave a + # partial ``.vrt`` pointing at a complete tile set. ``_write_bytes`` + # already implements the temp-then-rename pattern for local paths. + from ._writer import _write_bytes + _write_bytes(xml.encode('utf-8'), vrt_path) return vrt_path diff --git a/xrspatial/geotiff/tests/write/test_basic.py b/xrspatial/geotiff/tests/write/test_basic.py index 1c747c2d..2bb79d82 100644 --- a/xrspatial/geotiff/tests/write/test_basic.py +++ b/xrspatial/geotiff/tests/write/test_basic.py @@ -1526,6 +1526,78 @@ def test_compatible_sources_succeed(tmp_path): assert os.path.exists(vrt) +def test_write_vrt_leaves_no_temp_file(tmp_path): + """A successful write_vrt leaves the final .vrt and no stray temp + files in the destination directory (issue #2965).""" + d = _unique_dir(tmp_path, "atomic_clean") + a = os.path.join(d, "a.tif") + b = os.path.join(d, "b.tif") + _write_tif(a, h=4, w=4, dtype=np.float32) + _write_tif(b, h=4, w=4, dtype=np.float32, origin_x=4.0) + vrt = os.path.join(d, "out.vrt") + _priv_write_vrt(vrt, [a, b]) + assert os.path.exists(vrt) + # No leftover temp artifacts from the temp-then-rename write. + leftovers = glob.glob(os.path.join(d, "*.tmp")) + glob.glob( + os.path.join(d, "*.tmp*")) + assert leftovers == [], f"temp files not cleaned up: {leftovers}" + + +def test_write_vrt_no_partial_file_on_write_failure(tmp_path, monkeypatch): + """If the index write fails mid-flight, no partial .vrt is left at + the final path (issue #2965). The write goes to a temp file in the + destination directory and is os.replace'd into place, so a failure + before the rename never publishes a truncated index.""" + d = _unique_dir(tmp_path, "atomic_fail") + a = os.path.join(d, "a.tif") + b = os.path.join(d, "b.tif") + _write_tif(a, h=4, w=4, dtype=np.float32) + _write_tif(b, h=4, w=4, dtype=np.float32, origin_x=4.0) + vrt = os.path.join(d, "out.vrt") + + # Force the atomic helper to blow up while writing the temp file, + # before any rename onto the final path could happen. + def boom(file_bytes, path): + raise OSError("simulated disk-full during VRT index write") + + # write_vrt imports _write_bytes locally from _writer at call time, + # so patching the source module is what intercepts the call. + monkeypatch.setattr(writer_mod, "_write_bytes", boom) + + with pytest.raises(OSError, match="simulated disk-full"): + _priv_write_vrt(vrt, [a, b]) + + # The final path must not exist as a partial file. + assert not os.path.exists(vrt) + + +def test_write_vrt_failure_preserves_existing_file(tmp_path, monkeypatch): + """A failed re-write leaves any pre-existing .vrt at the final path + untouched rather than truncating it (issue #2965).""" + d = _unique_dir(tmp_path, "atomic_preserve") + a = os.path.join(d, "a.tif") + b = os.path.join(d, "b.tif") + _write_tif(a, h=4, w=4, dtype=np.float32) + _write_tif(b, h=4, w=4, dtype=np.float32, origin_x=4.0) + vrt = os.path.join(d, "out.vrt") + + # First write succeeds and produces a valid index. + _priv_write_vrt(vrt, [a, b]) + original = open(vrt, "rb").read() + assert original + + def boom(file_bytes, path): + raise OSError("simulated failure during VRT rewrite") + + monkeypatch.setattr(writer_mod, "_write_bytes", boom) + with pytest.raises(OSError, match="simulated failure"): + _priv_write_vrt(vrt, [a, b]) + + # The original index is intact (the temp-then-rename never published + # a partial file over it). + assert open(vrt, "rb").read() == original + + def test_pixel_size_within_tolerance_accepted(tmp_path): d = _unique_dir(tmp_path, "tol") a = os.path.join(d, "a.tif") From 8eebe7e243a97db75e64affcf1d1647587894ee5 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Fri, 5 Jun 2026 10:50:54 -0700 Subject: [PATCH 2/2] Address review nit: use with-blocks for file reads in atomic test (#2965) --- xrspatial/geotiff/tests/write/test_basic.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/xrspatial/geotiff/tests/write/test_basic.py b/xrspatial/geotiff/tests/write/test_basic.py index 2bb79d82..48dd6b13 100644 --- a/xrspatial/geotiff/tests/write/test_basic.py +++ b/xrspatial/geotiff/tests/write/test_basic.py @@ -1583,7 +1583,8 @@ def test_write_vrt_failure_preserves_existing_file(tmp_path, monkeypatch): # First write succeeds and produces a valid index. _priv_write_vrt(vrt, [a, b]) - original = open(vrt, "rb").read() + with open(vrt, "rb") as f: + original = f.read() assert original def boom(file_bytes, path): @@ -1595,7 +1596,8 @@ def boom(file_bytes, path): # The original index is intact (the temp-then-rename never published # a partial file over it). - assert open(vrt, "rb").read() == original + with open(vrt, "rb") as f: + assert f.read() == original def test_pixel_size_within_tolerance_accepted(tmp_path):