danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix: atomic file placement so Jellyfin can't index a half-written track (null-disc)

Browse source 
safe_move_file used shutil.move, which for a CROSS-filesystem move (downloads volume ->
library volume, common in Docker/NAS) copies the file to the FINAL path incrementally. A
media-server real-time watcher (Jellyfin) can catch that partial file mid-write and cache it
with null/incomplete metadata — tracks landing with no disc. Inconsistent because it only bites
cross-fs and races the scan tick; 're-add library' fixes it (rescan reads the now-complete file)
and the on-disk tags are fine — exactly the reported symptoms.

Fix: same-fs uses an atomic os.replace (also overwrites dst); cross-fs copies to a HIDDEN temp
sibling, fsyncs, then atomic os.replace into place (+ temp cleanup on failure). A watcher only
ever sees the COMPLETE file. EXDEV/EPERM/EACCES + the old string check route here, so detection
is strictly broader than before.

Tests: same-fs move, simulated EXDEV routes to the atomic path and leaves no partial temp, helper
completes+cleans, helper cleans temp + preserves source on failure. Existing replace-destination
test still green; 574 imports+relocate tests pass.
This commit is contained in:
BoulderBadgeDad 2026-06-23 23:17:50 -07:00
parent 03926bd6e2
commit 9d091207f6
2 changed files with 130 additions and 17 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

72
core/imports/file_ops.py
View file

@ -2,6 +2,7 @@
from __future__ import annotations
import errno
import logging
import os
import re
@ -102,6 +103,41 @@ def cleanup_slskd_dedup_siblings(source_path) -> List[str]:
return deleted
def _atomic_cross_device_move(src: Path, dst: Path) -> None:
"""Move ``src`` to ``dst`` across filesystems WITHOUT ever exposing a partial file at
the final path.
Copies into a hidden temp sibling of ``dst`` (same filesystem), fsyncs, then does an
atomic ``os.replace`` into place, then deletes ``src``. A media-server file watcher
(Jellyfin/Plex real-time monitoring) therefore only ever indexes the COMPLETE file —
an incremental in-place copy was what Jellyfin could catch mid-write and cache with
null/incomplete metadata (tracks landing with no disc). Cleans up the temp on failure.
"""
src, dst = Path(src), Path(dst)
tmp = dst.parent / f".{dst.name}.ssync-tmp"
try:
with open(src, "rb") as f_src, open(tmp, "wb") as f_dst:
shutil.copyfileobj(f_src, f_dst)
f_dst.flush()
os.fsync(f_dst.fileno())
try:
shutil.copystat(str(src), str(tmp)) # preserve mtime/permissions (copy2-like)
except OSError:
pass
os.replace(str(tmp), str(dst)) # atomic within dst's filesystem
except Exception:
try:
if tmp.exists():
tmp.unlink()
except OSError:
pass
raise
try:
src.unlink()
except OSError:
logger.info(f"Could not delete source after cross-device move (may be owned by another process): {src}")
def safe_move_file(src, dst):
"""Move a file safely across filesystems."""
src = Path(src)
@ -129,7 +165,13 @@ def safe_move_file(src, dst):
break
try:
shutil.move(str(src), str(dst))
# Same-filesystem move: an atomic rename that also overwrites dst. A media-server
# watcher (Jellyfin/Plex real-time monitoring) therefore never sees a partial file
# at the final name. Cross-filesystem raises EXDEV (some network mounts raise
# EPERM/EACCES) and we copy atomically below instead of letting the move write the
# destination incrementally — the partial-file-at-final-name is what caused tracks
# to land in Jellyfin with null/incomplete metadata (no disc).
os.replace(str(src), str(dst))
return
except FileNotFoundError:
if dst.exists():
@ -137,8 +179,6 @@ def safe_move_file(src, dst):
return
raise
except (OSError, PermissionError) as e:
error_msg = str(e).lower()
if dst.exists() and dst.stat().st_size > 0:
logger.warning(f"Move raised {type(e).__name__} but destination exists, treating as success: {e}")
try:
@ -147,23 +187,21 @@ def safe_move_file(src, dst):
logger.info(f"Could not delete source file (may be owned by another process): {src}")
return
if "cross-device" in error_msg or "operation not permitted" in error_msg or "permission denied" in error_msg:
logger.warning(f"Cross-device move detected, using fallback copy method: {e}")
error_msg = str(e).lower()
cross_device = (
getattr(e, "errno", None) in (errno.EXDEV, errno.EPERM, errno.EACCES)
or "cross-device" in error_msg
or "operation not permitted" in error_msg
or "permission denied" in error_msg
)
if cross_device:
logger.warning(f"Cross-device move, using atomic copy+rename: {e}")
try:
with open(src, "rb") as f_src:
with open(dst, "wb") as f_dst:
shutil.copyfileobj(f_src, f_dst)
f_dst.flush()
os.fsync(f_dst.fileno())
try:
src.unlink()
except PermissionError:
logger.info(f"Could not delete source file (may be owned by another process): {src}")
logger.info(f"Successfully moved file using fallback method: {src} -> {dst}")
_atomic_cross_device_move(src, dst)
logger.info(f"Successfully moved file atomically across filesystems: {src} -> {dst}")
return
except Exception as fallback_error:
logger.error(f"Fallback copy also failed: {fallback_error}")
logger.error(f"Atomic cross-device move failed: {fallback_error}")
raise
raise

75
tests/imports/test_import_file_ops.py
View file

@ -169,3 +169,78 @@ def test_read_staging_file_metadata_uses_filename_fallbacks_when_tags_are_invali
"track_number": 2,
"disc_number": 1,
}
# ── atomic cross-filesystem move (Jellyfin null-disc mitigation) ──────────────
import errno # noqa: E402
import os # noqa: E402
import pytest # noqa: E402
from core.imports import file_ops as _fo # noqa: E402
from core.imports.file_ops import _atomic_cross_device_move # noqa: E402
def test_same_fs_move_moves_and_removes_source(tmp_path):
src = tmp_path / "s.flac"
src.write_text("hello")
dst = tmp_path / "lib" / "t.flac" # parent created by safe_move_file
safe_move_file(src, dst)
assert dst.read_text() == "hello"
assert not src.exists()
def test_cross_device_move_routes_to_atomic_and_completes(tmp_path, monkeypatch):
# Simulate a cross-filesystem move: the same-fs os.replace raises EXDEV, and the
# atomic helper's temp->dst replace (same fs) succeeds. The file must complete and
# no partial temp file may be left at the final name's directory.
src = tmp_path / "s.flac"
src.write_text("payload")
dstdir = tmp_path / "lib"
dstdir.mkdir()
dst = dstdir / "t.flac"
real_replace = os.replace
def fake_replace(a, b):
if str(a) == str(src): # the cross-fs move attempt
raise OSError(errno.EXDEV, "Invalid cross-device link")
return real_replace(a, b) # the temp -> dst rename (same fs)
monkeypatch.setattr(_fo.os, "replace", fake_replace)
safe_move_file(src, dst)
assert dst.read_text() == "payload"
assert not src.exists()
assert not list(dstdir.glob(".*ssync-tmp")) # complete file only, no leftover temp
def test_atomic_helper_completes_and_cleans_temp(tmp_path):
src = tmp_path / "s.flac"
src.write_text("payload")
dstdir = tmp_path / "d"
dstdir.mkdir()
dst = dstdir / "t.flac"
_atomic_cross_device_move(src, dst)
assert dst.read_text() == "payload"
assert not src.exists()
assert not list(dstdir.glob(".*ssync-tmp"))
def test_atomic_helper_cleans_temp_and_keeps_source_on_failure(tmp_path, monkeypatch):
src = tmp_path / "s.flac"
src.write_text("payload")
dstdir = tmp_path / "d"
dstdir.mkdir()
dst = dstdir / "t.flac"
def boom(_a, _b):
raise OSError("replace failed")
monkeypatch.setattr(_fo.os, "replace", boom)
with pytest.raises(OSError):
_atomic_cross_device_move(src, dst)
assert src.exists() # source preserved on failure
assert not dst.exists() # no partial final file
assert not list(dstdir.glob(".*ssync-tmp")) # temp cleaned up