Merge pull request #1020 from Valyrian-Code/fix/dedup-public-dataloss

Avoid data loss in dedup_public.py when a link cannot be created

Author: Xpirix

scripts/dedup_public.py

@@ -179,16 +179,28 @@ def is_allowed(path: Path) -> bool:
             if args.dry_run:
                 print(f"DRY-RUN: replace {p} -> link to {rel_target}")
             else:
+                # Create the replacement link at a temporary path first, then
+                # atomically move it over the original. Unlinking the original
+                # before the link existed meant a failed os.symlink/os.link left
+                # the file deleted with no replacement, losing it permanently.
+                tmp = p.with_name(p.name + '.dedup-tmp')
                 try:
-                    p.unlink()
+                    if tmp.is_symlink() or tmp.exists():
+                        tmp.unlink()
                     if args.mode == 'symlink':
-                        os.symlink(rel_target, p)
+                        os.symlink(rel_target, tmp)
                     else:
-                        os.link(canonical, p)
+                        os.link(canonical, tmp)
+                    os.replace(tmp, p)
                     linked += 1
                 except Exception as e:
                     print(f"ERROR: Failed to link {p} -> {rel_target}: {e}", file=sys.stderr)
-                    # If linking fails, leave file as-is
+                    # Clean up the temporary link; the original file is intact.
+                    try:
+                        if tmp.is_symlink() or tmp.exists():
+                            tmp.unlink()
+                    except OSError:
+                        pass
             processed += 1
 
     print(f"Dedup complete. processed={processed} linked={linked} unique={len(canonical_by_hash)} base={base_root}")

test/test_dedup_public.py

@@ -0,0 +1,63 @@
+# -*- coding: utf-8 -*-
+"""Regression tests for scripts/dedup_public.py.
+
+The deduplicator replaces files that are byte-identical to a base-language file
+with a link to that canonical copy. It must never delete a file before the
+replacement link exists, otherwise a failed link leaves the published asset
+gone for good.
+"""
+import os
+import sys
+
+import dedup_public
+
+
+def _run(public_dir, mode="symlink"):
+    argv = [
+        "dedup_public.py",
+        "--public-dir", str(public_dir),
+        "--ext", "png",
+        "--mode", mode,
+    ]
+    old_argv = sys.argv
+    sys.argv = argv
+    try:
+        return dedup_public.main()
+    finally:
+        sys.argv = old_argv
+
+
+def _make_duplicate_tree(tmp_path):
+    """A canonical file at the root and an identical copy under a subdir."""
+    content = b"DUPLICATE-IMAGE-BYTES"
+    (tmp_path / "logo.png").write_bytes(content)
+    sub = tmp_path / "fr"
+    sub.mkdir()
+    dup = sub / "logo.png"
+    dup.write_bytes(content)
+    return dup, content
+
+
+def test_failed_link_does_not_delete_original(tmp_path, monkeypatch):
+    dup, content = _make_duplicate_tree(tmp_path)
+
+    def boom(*args, **kwargs):
+        raise OSError("simulated link failure")
+
+    monkeypatch.setattr(dedup_public.os, "symlink", boom)
+
+    assert _run(tmp_path) == 0
+    # The duplicate must survive intact when the link could not be created.
+    assert dup.is_file() and not dup.is_symlink()
+    assert dup.read_bytes() == content
+
+
+def test_duplicate_is_replaced_with_symlink_on_success(tmp_path):
+    dup, content = _make_duplicate_tree(tmp_path)
+
+    assert _run(tmp_path) == 0
+    # On success the duplicate becomes a symlink that still resolves to the
+    # original bytes, and no stale temp file is left behind.
+    assert dup.is_symlink()
+    assert dup.read_bytes() == content
+    assert not (dup.parent / (dup.name + ".dedup-tmp")).exists()