fix: keep the mirror stable across Lidarr upgrades and renames
Build and publish container / build (pull_request) Canceled after 6m18s
Build and publish container / build (pull_request) Canceled after 6m18s
Three defects in how the mirror tracked its source, all of which show up on a library that something else reorganises. Pruning probed the source tree for a mirror file's original name, trying each known extension in turn. A source saved as .FLAC was never found, so its mirror file was deleted as an orphan and re-encoded on the next pass, for ever. Pruning now works from the set of paths the pass actually accounted for, which cannot disagree with the walk over letter case or extension coverage. Two sources could also claim one mirror path -- 01 Song.flac beside a leftover 01 Song.mp3, which is what an interrupted upgrade leaves behind. Both encoded to the same destination, whichever finished last won the race, and every later pass found the other one stale. The best-quality source now wins, ties break on path, and the loser is logged. The source mtime was read after encoding rather than before. A file still being written when the pass reached it would be stamped with its final mtime while holding truncated audio, and would never be revisited. Adds regression tests for all three, plus the format-upgrade, album-rename and whole-library-deletion cases, each verified to fail before the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
82c9da6d62
commit
7a57e03c7b
@@ -28,6 +28,29 @@ mtime, so a file is stale exactly when the two differ. There is no database to
|
|||||||
fall out of step with the library, which matters when something else — Lidarr,
|
fall out of step with the library, which matters when something else — Lidarr,
|
||||||
in this case — is the thing that owns and reorganises it.
|
in this case — is the thing that owns and reorganises it.
|
||||||
|
|
||||||
|
### What that means for Lidarr
|
||||||
|
|
||||||
|
| Lidarr does this | The mirror does this |
|
||||||
|
| --------------------------------------- | -------------------------------------------------------------------------------- |
|
||||||
|
| Replaces a file with a better rip | Re-encodes in place. Same path in, same path out, so no duplicate |
|
||||||
|
| Upgrades MP3 to FLAC | Both map to the same `.mp3` mirror path, so the old one is overwritten |
|
||||||
|
| Renames a track, album or artist folder | Old path pruned, new path encoded. Correct, but it re-encodes rather than moving |
|
||||||
|
| Deletes an album or artist | Every orphaned mirror file is deleted and the emptied directories go too |
|
||||||
|
|
||||||
|
Pruning is driven by what the pass actually found, not by guessing source
|
||||||
|
filenames from mirror ones: a `.FLAC` source would not be found by a search for
|
||||||
|
`.flac`, and the mirror file would be deleted and rebuilt on alternate passes
|
||||||
|
for ever.
|
||||||
|
|
||||||
|
If two sources want the same mirror path — a `01 Song.flac` next to a leftover
|
||||||
|
`01 Song.mp3`, which is what an interrupted upgrade leaves — the better format
|
||||||
|
wins, ties break on path, and the loser is logged. Without that rule both
|
||||||
|
encode to the same destination and every pass finds one of them stale.
|
||||||
|
|
||||||
|
The mtime is read _before_ encoding rather than after. A file still being
|
||||||
|
written when the pass reaches it would otherwise be stamped with its final
|
||||||
|
mtime while holding truncated audio, and never be revisited.
|
||||||
|
|
||||||
Encodes are written to a temporary file and renamed into place, so an
|
Encodes are written to a temporary file and renamed into place, so an
|
||||||
interrupted run cannot leave a truncated MP3 that the next run mistakes for
|
interrupted run cannot leave a truncated MP3 that the next run mistakes for
|
||||||
finished work. A lock file in the mirror root stops two passes overlapping.
|
finished work. A lock file in the mirror root stops two passes overlapping.
|
||||||
|
|||||||
+67
-12
@@ -50,6 +50,23 @@ SOURCE_EXTENSIONS = {
|
|||||||
# quality for nothing.
|
# quality for nothing.
|
||||||
COPY_EXTENSIONS = {".mp3"}
|
COPY_EXTENSIONS = {".mp3"}
|
||||||
|
|
||||||
|
# Best first. Used only to settle which source wins when two of them want the
|
||||||
|
# same mirror path; see plan().
|
||||||
|
SOURCE_PRIORITY = [
|
||||||
|
".flac",
|
||||||
|
".wav",
|
||||||
|
".aif",
|
||||||
|
".aiff",
|
||||||
|
".ape",
|
||||||
|
".wv",
|
||||||
|
".alac",
|
||||||
|
".m4a",
|
||||||
|
".ogg",
|
||||||
|
".opus",
|
||||||
|
".wma",
|
||||||
|
".mp3",
|
||||||
|
]
|
||||||
|
|
||||||
# Looked for in the source directory when a file has no embedded picture.
|
# Looked for in the source directory when a file has no embedded picture.
|
||||||
COVER_NAMES = ("cover.jpg", "folder.jpg", "front.jpg", "cover.png", "folder.png")
|
COVER_NAMES = ("cover.jpg", "folder.jpg", "front.jpg", "cover.png", "folder.png")
|
||||||
|
|
||||||
@@ -186,6 +203,12 @@ def encode(source, mirror, quality_args, dry_run):
|
|||||||
mirror.parent.mkdir(parents=True, exist_ok=True)
|
mirror.parent.mkdir(parents=True, exist_ok=True)
|
||||||
cover = None if has_embedded_picture(source) else find_cover(source.parent)
|
cover = None if has_embedded_picture(source) else find_cover(source.parent)
|
||||||
|
|
||||||
|
# Read the source's mtime before encoding, not after. If the file is still
|
||||||
|
# being written -- a Lidarr import landing mid-pass -- stamping the mirror
|
||||||
|
# with the later mtime would mark truncated output as current. Stamping the
|
||||||
|
# earlier one leaves the two mismatched, so the next pass re-encodes it.
|
||||||
|
stat = source.stat()
|
||||||
|
|
||||||
# Encode to a temporary file in the destination directory and rename it
|
# Encode to a temporary file in the destination directory and rename it
|
||||||
# into place, so an interrupted run cannot leave a truncated MP3 that the
|
# into place, so an interrupted run cannot leave a truncated MP3 that the
|
||||||
# next run would treat as complete.
|
# next run would treat as complete.
|
||||||
@@ -199,7 +222,6 @@ def encode(source, mirror, quality_args, dry_run):
|
|||||||
if completed.returncode != 0:
|
if completed.returncode != 0:
|
||||||
lines = completed.stderr.strip().splitlines()
|
lines = completed.stderr.strip().splitlines()
|
||||||
return Result("failed", source, lines[-1] if lines else "ffmpeg failed")
|
return Result("failed", source, lines[-1] if lines else "ffmpeg failed")
|
||||||
stat = source.stat()
|
|
||||||
os.utime(temporary, (stat.st_atime, stat.st_mtime))
|
os.utime(temporary, (stat.st_atime, stat.st_mtime))
|
||||||
os.replace(temporary, mirror)
|
os.replace(temporary, mirror)
|
||||||
except Exception as error: # noqa: BLE001 - reported per file, run continues
|
except Exception as error: # noqa: BLE001 - reported per file, run continues
|
||||||
@@ -227,9 +249,8 @@ def copy(source, mirror, dry_run):
|
|||||||
return Result("copied", mirror)
|
return Result("copied", mirror)
|
||||||
|
|
||||||
|
|
||||||
def process(source, source_root, mirror_root, quality_args, dry_run):
|
def process(source, mirror, quality_args, dry_run):
|
||||||
"""Bring one source file's mirror entry up to date."""
|
"""Bring one source file's mirror entry up to date."""
|
||||||
mirror = mirror_path_for(source, source_root, mirror_root)
|
|
||||||
if is_current(source, mirror):
|
if is_current(source, mirror):
|
||||||
return Result("skipped", mirror)
|
return Result("skipped", mirror)
|
||||||
if source.suffix.lower() in COPY_EXTENSIONS:
|
if source.suffix.lower() in COPY_EXTENSIONS:
|
||||||
@@ -245,15 +266,47 @@ def find_sources(root):
|
|||||||
yield path
|
yield path
|
||||||
|
|
||||||
|
|
||||||
def prune(source_root, mirror_root, dry_run):
|
def plan(scan_root, source_root, mirror_root):
|
||||||
"""Delete mirror files whose source is gone, and any dirs left empty."""
|
"""Map each mirror path to the one source that should produce it.
|
||||||
extensions = SOURCE_EXTENSIONS | COPY_EXTENSIONS
|
|
||||||
|
Two sources can want the same mirror path -- `01 Song.flac` alongside a
|
||||||
|
leftover `01 Song.mp3`, which is what an interrupted Lidarr upgrade leaves
|
||||||
|
behind. Without a decision here both would encode to the same destination,
|
||||||
|
each pass would find the loser stale, and the mirror would be rewritten
|
||||||
|
forever. Preferring the highest-quality source, ties broken by path, makes
|
||||||
|
the outcome stable and predictable instead.
|
||||||
|
"""
|
||||||
|
chosen = {}
|
||||||
|
for source in find_sources(scan_root):
|
||||||
|
mirror = mirror_path_for(source, source_root, mirror_root)
|
||||||
|
rival = chosen.get(mirror)
|
||||||
|
if rival is None:
|
||||||
|
chosen[mirror] = source
|
||||||
|
continue
|
||||||
|
winner, loser = sorted((source, rival), key=source_rank)
|
||||||
|
logger.warning("%s and %s both map to %s; using %s", rival, source, mirror, winner)
|
||||||
|
chosen[mirror] = winner
|
||||||
|
return chosen
|
||||||
|
|
||||||
|
|
||||||
|
def source_rank(source):
|
||||||
|
"""Sort key preferring better source formats, then a stable path order."""
|
||||||
|
suffix = source.suffix.lower()
|
||||||
|
position = SOURCE_PRIORITY.index(suffix) if suffix in SOURCE_PRIORITY else len(SOURCE_PRIORITY)
|
||||||
|
return (position, str(source))
|
||||||
|
|
||||||
|
|
||||||
|
def prune(mirror_root, expected, dry_run):
|
||||||
|
"""Delete mirror files this pass did not account for, and empty dirs.
|
||||||
|
|
||||||
|
Driven by the set of paths the pass expects to exist rather than by
|
||||||
|
probing the source tree for names, which would disagree with it over
|
||||||
|
letter case and over any extension the walker does not collect.
|
||||||
|
"""
|
||||||
removed = 0
|
removed = 0
|
||||||
|
|
||||||
for mirror in sorted(mirror_root.rglob(f"*{MIRROR_SUFFIX}")):
|
for mirror in sorted(mirror_root.rglob(f"*{MIRROR_SUFFIX}")):
|
||||||
relative = mirror.relative_to(mirror_root)
|
if mirror in expected:
|
||||||
stem = source_root / relative
|
|
||||||
if any((stem.with_suffix(extension)).exists() for extension in extensions):
|
|
||||||
continue
|
continue
|
||||||
removed += 1
|
removed += 1
|
||||||
if dry_run:
|
if dry_run:
|
||||||
@@ -281,10 +334,12 @@ def run_once(scan_root, source_root, mirror_root, quality_args, jobs, dry_run, d
|
|||||||
counts = {"encoded": 0, "copied": 0, "skipped": 0, "failed": 0}
|
counts = {"encoded": 0, "copied": 0, "skipped": 0, "failed": 0}
|
||||||
failures = []
|
failures = []
|
||||||
|
|
||||||
|
work = plan(scan_root, source_root, mirror_root)
|
||||||
|
|
||||||
with concurrent.futures.ThreadPoolExecutor(max_workers=jobs) as pool:
|
with concurrent.futures.ThreadPoolExecutor(max_workers=jobs) as pool:
|
||||||
futures = [
|
futures = [
|
||||||
pool.submit(process, source, source_root, mirror_root, quality_args, dry_run)
|
pool.submit(process, source, mirror, quality_args, dry_run)
|
||||||
for source in find_sources(scan_root)
|
for mirror, source in work.items()
|
||||||
]
|
]
|
||||||
for future in concurrent.futures.as_completed(futures):
|
for future in concurrent.futures.as_completed(futures):
|
||||||
result = future.result()
|
result = future.result()
|
||||||
@@ -292,7 +347,7 @@ def run_once(scan_root, source_root, mirror_root, quality_args, jobs, dry_run, d
|
|||||||
if result.action == "failed":
|
if result.action == "failed":
|
||||||
failures.append(result)
|
failures.append(result)
|
||||||
|
|
||||||
removed = prune(source_root, mirror_root, dry_run) if do_prune else 0
|
removed = prune(mirror_root, set(work), dry_run) if do_prune else 0
|
||||||
|
|
||||||
for failure in failures:
|
for failure in failures:
|
||||||
logger.error("failed: %s: %s", failure.path, failure.error)
|
logger.error("failed: %s: %s", failure.path, failure.error)
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import os
|
import os
|
||||||
|
import shutil
|
||||||
import subprocess
|
import subprocess
|
||||||
import time
|
import time
|
||||||
|
|
||||||
@@ -152,6 +153,100 @@ def test_existing_mp3_is_copied_not_re_encoded(tmp_path, make_flac):
|
|||||||
assert (mirror / "b.mp3").read_bytes() == (source / "b.mp3").read_bytes()
|
assert (mirror / "b.mp3").read_bytes() == (source / "b.mp3").read_bytes()
|
||||||
|
|
||||||
|
|
||||||
|
def test_format_upgrade_replaces_rather_than_duplicating(tmp_path, make_flac):
|
||||||
|
"""Lidarr replacing an MP3 with a FLAC must not leave two mirror files."""
|
||||||
|
source = tmp_path / "src"
|
||||||
|
mirror = tmp_path / "dst"
|
||||||
|
flac = make_flac(source / "Album" / "01 Song.flac")
|
||||||
|
subprocess.run(
|
||||||
|
["ffmpeg", "-loglevel", "error", "-y", "-i", str(flac), str(source / "Album" / "01 Song.mp3")],
|
||||||
|
check=True,
|
||||||
|
capture_output=True,
|
||||||
|
)
|
||||||
|
flac.unlink()
|
||||||
|
|
||||||
|
run(source, mirror)
|
||||||
|
assert sorted(p.name for p in (mirror / "Album").iterdir()) == ["01 Song.mp3"]
|
||||||
|
|
||||||
|
# The upgrade: the MP3 goes, a FLAC arrives at the same stem.
|
||||||
|
(source / "Album" / "01 Song.mp3").unlink()
|
||||||
|
make_flac(source / "Album" / "01 Song.flac", title="Upgraded")
|
||||||
|
run(source, mirror)
|
||||||
|
|
||||||
|
assert sorted(p.name for p in (mirror / "Album").iterdir()) == ["01 Song.mp3"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_renamed_album_leaves_nothing_behind(tmp_path, make_flac):
|
||||||
|
"""A Lidarr rename is a delete plus an add; the old tree must not linger."""
|
||||||
|
source = tmp_path / "src"
|
||||||
|
mirror = tmp_path / "dst"
|
||||||
|
make_flac(source / "Artist" / "Album (2019)" / "01 Song.flac")
|
||||||
|
|
||||||
|
run(source, mirror)
|
||||||
|
(source / "Artist" / "Album (2019)").rename(source / "Artist" / "Album (2020)")
|
||||||
|
run(source, mirror)
|
||||||
|
|
||||||
|
assert (mirror / "Artist" / "Album (2020)" / "01 Song.mp3").is_file()
|
||||||
|
assert not (mirror / "Artist" / "Album (2019)").exists()
|
||||||
|
|
||||||
|
|
||||||
|
def test_competing_sources_pick_the_lossless_one_and_stay_stable(tmp_path, make_flac, probe_tag):
|
||||||
|
"""Both a FLAC and an MP3 at one stem: the FLAC wins, and stays won."""
|
||||||
|
source = tmp_path / "src"
|
||||||
|
mirror = tmp_path / "dst"
|
||||||
|
make_flac(source / "a.flac", title="From FLAC")
|
||||||
|
other = make_flac(tmp_path / "scratch" / "other.flac", title="From MP3")
|
||||||
|
subprocess.run(
|
||||||
|
["ffmpeg", "-loglevel", "error", "-y", "-i", str(other), str(source / "a.mp3")],
|
||||||
|
check=True,
|
||||||
|
capture_output=True,
|
||||||
|
)
|
||||||
|
# Age the loser well beyond the mtime tolerance, so "is it current?" gives
|
||||||
|
# a definite answer for it rather than one that depends on the clock.
|
||||||
|
older = time.time() - 3600
|
||||||
|
os.utime(source / "a.mp3", (older, older))
|
||||||
|
|
||||||
|
run(source, mirror)
|
||||||
|
assert probe_tag(mirror / "a.mp3", "title") == "From FLAC"
|
||||||
|
|
||||||
|
# The loser must not make the mirror look stale on the next pass, or every
|
||||||
|
# run would re-encode for ever.
|
||||||
|
first = (mirror / "a.mp3").stat().st_mtime_ns
|
||||||
|
run(source, mirror)
|
||||||
|
assert (mirror / "a.mp3").stat().st_mtime_ns == first
|
||||||
|
|
||||||
|
|
||||||
|
def test_uppercase_extension_is_not_pruned_and_re_encoded(tmp_path, make_flac):
|
||||||
|
"""A .FLAC source must not be treated as an orphan on the next pass."""
|
||||||
|
source = tmp_path / "src"
|
||||||
|
mirror = tmp_path / "dst"
|
||||||
|
made = make_flac(source / "a.flac")
|
||||||
|
made.rename(source / "a.FLAC")
|
||||||
|
|
||||||
|
run(source, mirror)
|
||||||
|
first = (mirror / "a.mp3").stat().st_mtime_ns
|
||||||
|
|
||||||
|
run(source, mirror)
|
||||||
|
assert (mirror / "a.mp3").is_file()
|
||||||
|
assert (mirror / "a.mp3").stat().st_mtime_ns == first
|
||||||
|
|
||||||
|
|
||||||
|
def test_whole_library_deleted_empties_the_mirror(tmp_path, make_flac):
|
||||||
|
source = tmp_path / "src"
|
||||||
|
mirror = tmp_path / "dst"
|
||||||
|
make_flac(source / "A" / "one.flac")
|
||||||
|
make_flac(source / "B" / "two.flac")
|
||||||
|
|
||||||
|
run(source, mirror)
|
||||||
|
shutil.rmtree(source / "A")
|
||||||
|
shutil.rmtree(source / "B")
|
||||||
|
run(source, mirror)
|
||||||
|
|
||||||
|
assert list(mirror.rglob("*.mp3")) == []
|
||||||
|
assert not (mirror / "A").exists()
|
||||||
|
assert not (mirror / "B").exists()
|
||||||
|
|
||||||
|
|
||||||
def test_dry_run_writes_nothing(tmp_path, make_flac):
|
def test_dry_run_writes_nothing(tmp_path, make_flac):
|
||||||
source = tmp_path / "src"
|
source = tmp_path / "src"
|
||||||
mirror = tmp_path / "dst"
|
mirror = tmp_path / "dst"
|
||||||
|
|||||||
Reference in New Issue
Block a user