From 41f6b290d81f7bb6bee9f5a719ef61a23df9926c Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Mon, 24 Aug 2026 18:34:34 +0100 Subject: [PATCH 1/3] feat: rebuild the moods from the library's measured tag distribution The first set of moods was guessed at, and a tag census of the real library shows how badly. Four tags in the dance mood -- edm, big room, hardstyle, electronic dance music -- carry nothing whatsoever, while techno, electro, big beat, rave and progressive house together carry over seven thousand plays and were all absent. The drum and bass mood missed four of the six spellings the taggers actually use, plus techstep, darkstep and hospital records. The eighties mood was the worst of it. It leaned on the bare `80s` tag, which is the eleventh most-played tag here and sits on Def Leppard, Bon Jovi and AC/DC rather than on anything with a synthesiser in it. Removing it and adding a hair-metal mood -- glam rock, hair metal, glam metal, arena rock, AOR, windowed to 1975-1994 -- puts roughly eight thousand plays somewhere sensible and stops the synth playlist being a stadium rock playlist. Whole clusters had no mood at all: metal at around twenty-five thousand plays across heavy, thrash, speed, power, death and NWOBHM, and pop punk and emo at a similar figure. Both now exist, along with bass, nu-metal and indie. Three kinds of tag are excluded on principle, with a test enforcing it, because each produces a playlist that looks reasonable and is not. Nationality describes a passport rather than a sound, and `american` alone spans 241 artists. `rock` and `electronic` span 340 and 275, which is most of the collection. And Last.fm's most popular tag for an artist is frequently the artist's own name, so selecting on `green day` yields a Green Day playlist wearing a genre's clothes. --- README.md | 41 +++++++++++++++---- music_curator.py | 82 +++++++++++++++++++++++++++++-------- tests/test_music_curator.py | 34 +++++++++++++++ 3 files changed, 131 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index c8382f6..7b8cde2 100644 --- a/README.md +++ b/README.md @@ -174,17 +174,40 @@ An artist neither key resolves is recorded as fetched with no tags, so the next pass does not spend a request on it again. A genuine failure — a rate limit, a bad key — is *not* recorded, so that one is retried. -The built-in moods are chosen for this library rather than as a general -taxonomy: +The built-in moods are built from the **tag distribution of this library**, +measured, rather than from a general taxonomy: | Mood | Selected on | -| ------------------ | ------------------------------------------------- | -| `80s-synths` | synthpop, new wave, synthwave — released 1975-1992 | -| `high-energy-rock` | hard rock, punk, pop punk, alternative | -| `screamo` | screamo, post-hardcore, metalcore, emo | -| `drum-and-bass` | drum and bass, liquid funk, neurofunk, jungle | -| `dance` | house, big room, hardstyle, trance, dubstep | -| `classic-rock` | classic rock, prog, psychedelic, blues rock | +| --------------- | -------------------------------------------------------- | +| `drum-and-bass` | drum and bass and its six spellings, liquid funk, neurofunk, jungle, techstep, hospital records | +| `bass` | dubstep, brostep, grime, trip-hop, big beat | +| `dance` | house and its variants, trance, techno, electro, rave | +| `pop-punk` | pop punk, punk, emo, emocore, easycore, power pop | +| `screamo` | screamo, post-hardcore, metalcore, melodic hardcore, trancecore | +| `heavy-metal` | heavy metal, thrash, speed, power, death, prog, NWOBHM | +| `hair-metal` | hair metal, glam metal, glam rock, arena rock, AOR — 1975-1994 | +| `nu-metal` | nu metal, alternative metal, rapcore, industrial | +| `classic-rock` | classic rock, prog, psychedelic, blues rock, 70s, 60s | +| `80s-synths` | synthpop, electropop, new wave, post-punk — 1975-1992 | +| `indie` | indie, indie rock, indie pop, britpop, singer-songwriter | + +Measuring first mattered. `synthwave`, `edm`, `big room` and `hardstyle` are +plausible tags that carry **nothing at all** here, while `techstep`, `easycore` +and `hospital records` carry real weight. Guessing produces the first list. + +Three kinds of tag are never used, and there is a test enforcing it: + +- **Nationality** — `american` alone spans 241 artists. A passport is not a + sound. +- **`rock` and `electronic`** — 340 and 275 artists, most of the library. A + mood that matches everything is not a mood. +- **Artist names** — Last.fm's most popular tag for an artist is frequently + their own name. `green day`, `paramore` and `queen` are single-artist + playlists waiting to happen. + +`80s-synths` deliberately excludes the bare `80s` tag, which in this library +sits on Def Leppard and Bon Jovi rather than on anything with a synthesiser in +it. Those belong to `hair-metal`, which is why that mood now exists. An artist qualifies when their tag weights inside a mood sum to at least 30 out of Last.fm's 0-100 scale. One low-weight tag is not a genre, it is somebody's diff --git a/music_curator.py b/music_curator.py index 03687f0..7c15463 100644 --- a/music_curator.py +++ b/music_curator.py @@ -1242,49 +1242,97 @@ PLAYLISTS = ( # `years` filters on the album's release date, which is what separates eighties # synth records from everything a synthpop tag would otherwise drag in. DEFAULT_VIBES = ( + # Built from the tag distribution of this library rather than from a general + # taxonomy, which is why some obvious-looking tags are absent and some + # unobvious ones are here. `synthwave`, `edm`, `big room` and `hardstyle` + # carry nothing at all; `techstep`, `easycore` and `hospital records` carry + # real weight. + # + # Three kinds of tag are deliberately never used. Nationality -- american, + # british, swedish -- describes a passport, not a sound, and `american` + # alone spans 241 artists. `rock` and `electronic` span 340 and 275, which + # is most of the library and therefore no mood at all. And Last.fm's most + # popular tag for an artist is frequently their own name, so `green day`, + # `paramore` and `queen` are single-artist playlists waiting to happen. { - "name": "80s-synths", + "name": "drum-and-bass", "tags": [ - "synthpop", "synth pop", "synth-pop", "new wave", "synthwave", - "new romantic", "electropop", "80s", "1980s", + "drum and bass", "dnb", "drum n bass", "drum'n'bass", "drum & bass", + "drum 'n' bass", "liquid funk", "neurofunk", "jungle", "techstep", + "darkstep", "drumstep", "breakbeat", "hospital records", ], - "years": [1975, 1992], }, { - "name": "high-energy-rock", + "name": "bass", + "tags": ["dubstep", "brostep", "grime", "trip-hop", "big beat"], + }, + { + "name": "dance", "tags": [ - "hard rock", "punk rock", "pop punk", "punk", "alternative rock", - "rock", "garage rock", "skate punk", + "house", "electro house", "progressive house", "tech house", "trance", + "techno", "electro", "rave", "dance", "minimal", + ], + }, + { + "name": "pop-punk", + "tags": [ + "pop punk", "pop-punk", "punk rock", "punk", "skate punk", "emo", + "emocore", "easycore", "powerpop", "power pop", "post-grunge", ], }, { "name": "screamo", "tags": [ - "screamo", "post-hardcore", "metalcore", "emo", "hardcore", - "melodic hardcore", "emocore", + "screamo", "post-hardcore", "metalcore", "melodic metalcore", + "melodic hardcore", "hardcore", "trancecore", "deathcore", ], }, { - "name": "drum-and-bass", + "name": "heavy-metal", "tags": [ - "drum and bass", "drum n bass", "dnb", "liquid funk", "neurofunk", - "jungle", "breakbeat", + "heavy metal", "metal", "thrash metal", "thrash", "speed metal", + "power metal", "death metal", "progressive metal", "nwobhm", + "classic metal", ], }, { - "name": "dance", + "name": "hair-metal", "tags": [ - "electro house", "house", "big room", "electronic dance music", - "edm", "hardstyle", "trance", "dubstep", "electro", + "hair metal", "glam metal", "glam rock", "arena rock", "aor", + "rock and roll", "rock n roll", + ], + "years": [1975, 1994], + }, + { + "name": "nu-metal", + "tags": [ + "nu metal", "nu-metal", "alternative metal", "rapcore", + "industrial metal", "industrial rock", "industrial", ], }, { "name": "classic-rock", "tags": [ - "classic rock", "progressive rock", "psychedelic rock", - "blues rock", "70s", "60s", + "classic rock", "progressive rock", "psychedelic rock", "psychedelic", + "blues rock", "blues", "southern rock", "art rock", "space rock", + "british invasion", "folk rock", "70s", "60s", ], }, + { + "name": "80s-synths", + # Without the bare `80s` tag, which in this library sits on Def Leppard + # and Bon Jovi rather than on anything with a synthesiser in it. The + # year window is kept as a second filter, not as the only one. + "tags": [ + "synthpop", "electropop", "new wave", "post-punk", + "post-punk revival", "new romantic", + ], + "years": [1975, 1992], + }, + { + "name": "indie", + "tags": ["indie", "indie rock", "indie pop", "britpop", "singer-songwriter"], + }, ) diff --git a/tests/test_music_curator.py b/tests/test_music_curator.py index d47d353..738e914 100644 --- a/tests/test_music_curator.py +++ b/tests/test_music_curator.py @@ -1198,3 +1198,37 @@ def test_a_real_failure_is_not_recorded_so_the_next_pass_retries(tmp_path): # One artist failed hard and must still be pending. assert store.scalar("SELECT COUNT(*) FROM artist_tag_fetched") == 1 + + +# Tags that carry real weight in a real library, against tags that describe a +# passport, span most of the collection, or are somebody's artist name. +USELESS_TAGS = { + "american", "british", "australian", "canadian", "swedish", "dutch", "german", + "scottish", "english", "uk", "usa", "canada", + "rock", "electronic", "pop", "alternative", "metal ", "all", "heavy", + "female vocalists", "male vocalists", "female vocalist", + "my top songs", "cover", "covers", "not emo", + "green day", "paramore", "queen", "bon jovi", "shinedown", "aerosmith", + "journey", "fleetwood mac", +} + + +def test_no_mood_selects_on_a_useless_tag(): + """Nationality is not a sound; `rock` and `electronic` span most of the + library; and Last.fm's top tag for an artist is often their own name.""" + for vibe in music_curator.DEFAULT_VIBES: + overlap = {tag.casefold() for tag in vibe["tags"]} & USELESS_TAGS + assert not overlap, f"{vibe['name']} selects on {overlap}" + + +def test_mood_names_are_unique(): + names = [vibe["name"] for vibe in music_curator.DEFAULT_VIBES] + assert len(names) == len(set(names)) + + +def test_the_eighties_mood_does_not_lean_on_the_bare_decade_tag(): + """In this library `80s` sits on Def Leppard and Bon Jovi, not on + synthesisers. The hair-metal mood is where those belong.""" + synths = next(v for v in music_curator.DEFAULT_VIBES if v["name"] == "80s-synths") + assert "80s" not in synths["tags"] + assert synths["years"] == [1975, 1992] -- 2.54.0 From 8c4da6e14e8e8291825a78d09f949466441cc43d Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Mon, 24 Aug 2026 18:39:59 +0100 Subject: [PATCH 2/3] feat: let a mood exclude tags, and widen the eighties one to the canon The eighties mood selected on synth-specific tags only, on the grounds that the bare `80s` tag drags in Def Leppard and Bon Jovi. Checking the canon against live Last.fm pages shows what that costs. Eurythmics is tagged 80s, new wave, pop, female vocalists, synth pop -- four of those five are tags no mood may use. Frankie Goes to Hollywood is tagged 80s, new wave, pop, british, dance, and survives only on new wave. An act tagged purely 80s, pop and dance, which is a great deal of eighties pop, was missed outright. Two spellings also matter more than the census suggested. Of Depeche Mode, Duran Duran, Eurythmics and Frankie Goes to Hollywood, three carry "synth pop" with a space and only one carries "synthpop" without. The census could not show this because it only sees artists already in the library; the spelling that was kept was the rarer one. Add an `exclude` list to a mood. An excluded tag drops the artist outright rather than docking their score, which is the only construction that expresses "the eighties, but not the stadium rock" -- weighting cannot do it, because the tag it would weight is the one both share. The synth acts do not carry hard rock or hair metal, and that is the whole of the difference. A mood may not both select on and exclude the same tag; that is rejected when the definitions are read rather than silently producing nothing. --- README.md | 30 ++++++++++++++--- music_curator.py | 48 +++++++++++++++++++++++---- tests/test_music_curator.py | 65 ++++++++++++++++++++++++++++++++++--- 3 files changed, 128 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 7b8cde2..2fb209e 100644 --- a/README.md +++ b/README.md @@ -188,7 +188,7 @@ measured, rather than from a general taxonomy: | `hair-metal` | hair metal, glam metal, glam rock, arena rock, AOR — 1975-1994 | | `nu-metal` | nu metal, alternative metal, rapcore, industrial | | `classic-rock` | classic rock, prog, psychedelic, blues rock, 70s, 60s | -| `80s-synths` | synthpop, electropop, new wave, post-punk — 1975-1992 | +| `80s-synths` | 80s, new wave, synth pop, electropop, post-punk — 1975-1992, rock excluded | | `indie` | indie, indie rock, indie pop, britpop, singer-songwriter | Measuring first mattered. `synthwave`, `edm`, `big room` and `hardstyle` are @@ -205,9 +205,31 @@ Three kinds of tag are never used, and there is a test enforcing it: their own name. `green day`, `paramore` and `queen` are single-artist playlists waiting to happen. -`80s-synths` deliberately excludes the bare `80s` tag, which in this library -sits on Def Leppard and Bon Jovi rather than on anything with a synthesiser in -it. Those belong to `hair-metal`, which is why that mood now exists. +### Exclusions + +A mood may also list `exclude`. An excluded tag drops the artist outright rather +than docking their score, and it exists because `80s-synths` cannot be written +any other way. + +`80s` is the eleventh most-played tag here, and it sits on Def Leppard and Bon +Jovi exactly as heavily as on Eurythmics. Weighting cannot separate them, +because the tag it would weight is the one they share. What does separate them +is that the stadium rock also carries `hard rock` and `hair metal`, and the +synth acts do not. + +Checked against live Last.fm pages, since the tag census only sees artists +already in the library: + +| Artist | Tags | +| --- | --- | +| Eurythmics | `80s`, `new wave`, `pop`, `female vocalists`, `synth pop` | +| Frankie Goes to Hollywood | `80s`, `new wave`, `pop`, `british`, `dance` | +| Depeche Mode | `electronic`, `synthpop`, `new wave`, `80s`, `synth pop` | +| Duran Duran | `new wave`, `80s`, `pop`, `synth pop`, `rock` | + +Four of Eurythmics' five tags are ones no mood may use. Three of the four +artists spell it **`synth pop`** with a space; only one spells it `synthpop`. +Guessing one spelling would have missed most of the canon. An artist qualifies when their tag weights inside a mood sum to at least 30 out of Last.fm's 0-100 scale. One low-weight tag is not a genre, it is somebody's diff --git a/music_curator.py b/music_curator.py index 7c15463..ec3c143 100644 --- a/music_curator.py +++ b/music_curator.py @@ -1320,12 +1320,23 @@ DEFAULT_VIBES = ( }, { "name": "80s-synths", - # Without the bare `80s` tag, which in this library sits on Def Leppard - # and Bon Jovi rather than on anything with a synthesiser in it. The - # year window is kept as a second filter, not as the only one. + # `80s` is included, which on its own would drag in Def Leppard and Bon + # Jovi -- they carry it as heavily as Eurythmics does. The exclusion is + # what separates them: the stadium rock is also tagged hard rock and + # hair metal, and the synth acts are not. Weighting cannot do this, + # because the tag it would weight is the one they share. + # + # Both spellings of synth pop are listed. Of Depeche Mode, Duran Duran, + # Eurythmics and Frankie Goes to Hollywood, three carry "synth pop" with + # a space and only one carries "synthpop" without. "tags": [ - "synthpop", "electropop", "new wave", "post-punk", - "post-punk revival", "new romantic", + "80s", "new wave", "synth pop", "synthpop", "synth-pop", "synthwave", + "electropop", "new romantic", "post-punk", "post-punk revival", + ], + "exclude": [ + "hard rock", "hair metal", "glam metal", "glam rock", "heavy metal", + "metal", "arena rock", "aor", "nwobhm", "thrash metal", + "classic rock", "southern rock", "blues rock", ], "years": [1975, 1992], }, @@ -1358,6 +1369,13 @@ def load_vibes(path): raise ValueError(f"{path}: {name!r} is not a usable vibe name (a-z, 0-9, -)") if not vibe.get("tags"): raise ValueError(f"{path}: vibe {name!r} lists no tags") + overlap = {str(tag).casefold() for tag in vibe["tags"]} & { + str(tag).casefold() for tag in vibe.get("exclude", []) + } + if overlap: + raise ValueError( + f"{path}: vibe {name!r} both selects on and excludes {sorted(overlap)}" + ) return tuple(loaded) @@ -1464,8 +1482,24 @@ def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now): for vibe in vibes: tags = [str(tag).strip().casefold() for tag in vibe["tags"]] placeholders = ",".join("?" * len(tags)) - years = vibe.get("years") parameters = [*tags, vibe.get("min_score", VIBE_MIN_SCORE)] + + # An exclusion drops the artist outright rather than docking their + # score. It is the only way to write "the eighties, but not the stadium + # rock": those artists carry `80s` as heavily as the synth acts do, so + # no amount of weighting separates them -- but they also carry `hard + # rock`, and the synth acts do not. + excluded = [str(tag).strip().casefold() for tag in vibe.get("exclude", [])] + exclude_clause = "" + if excluded: + exclude_clause = ( + " AND NOT EXISTS (SELECT 1 FROM artist_tag x" + " WHERE x.norm_artist = a.norm_name" + f" AND x.tag IN ({','.join('?' * len(excluded))}))" + ) + parameters += excluded + + years = vibe.get("years") year_clause = "" if years: year_clause = ( @@ -1488,7 +1522,7 @@ def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now): JOIN lidarr_artist a ON a.id = t.artist_id JOIN vibe v ON v.norm_artist = a.norm_name LEFT JOIN lidarr_album al ON al.id = t.album_id - WHERE {PLAYABLE}{year_clause} + WHERE {PLAYABLE}{exclude_clause}{year_clause} ORDER BY ((t.id * {SHUFFLE_MULTIPLIER}) + ?) % {SHUFFLE_MODULUS} LIMIT ? """ diff --git a/tests/test_music_curator.py b/tests/test_music_curator.py index 738e914..78e752f 100644 --- a/tests/test_music_curator.py +++ b/tests/test_music_curator.py @@ -1226,9 +1226,66 @@ def test_mood_names_are_unique(): assert len(names) == len(set(names)) -def test_the_eighties_mood_does_not_lean_on_the_bare_decade_tag(): - """In this library `80s` sits on Def Leppard and Bon Jovi, not on - synthesisers. The hair-metal mood is where those belong.""" +ROCK_TAGS = { + "classic rock", "hard rock", "blues rock", "southern rock", "arena rock", + "glam rock", "hair metal", "glam metal", "heavy metal", "metal", "art rock", + "psychedelic rock", "progressive rock", "rock and roll", "rock n roll", +} + + +def test_a_decade_tag_in_a_non_rock_mood_must_exclude_the_rock(): + """`80s` sits on Def Leppard and Bon Jovi as heavily as on Eurythmics, so a + mood that reaches for a decade without wanting rock has to say so. A mood + that does want it -- classic-rock reaching for 70s -- is exempt.""" + for vibe in music_curator.DEFAULT_VIBES: + tags = {tag.casefold() for tag in vibe["tags"]} + decades = {"60s", "70s", "80s", "90s"} & tags + if not decades or tags & ROCK_TAGS: + continue + excluded = {tag.casefold() for tag in vibe.get("exclude", [])} + missing = {"hard rock", "hair metal"} - excluded + assert not missing, f"{vibe['name']} selects on {decades} without excluding {missing}" + + +def test_the_eighties_mood_covers_the_canon(): + """Depeche Mode, Duran Duran, Eurythmics and Frankie Goes to Hollywood -- + checked against their live Last.fm tags. Three of the four carry "synth pop" + with a space; only one carries it without.""" synths = next(v for v in music_curator.DEFAULT_VIBES if v["name"] == "80s-synths") - assert "80s" not in synths["tags"] + tags = {t.casefold() for t in synths["tags"]} + for artist_tags in ( + {"80s", "new wave", "pop", "female vocalists", "synth pop"}, # Eurythmics + {"80s", "new wave", "pop", "british", "dance"}, # Frankie + {"electronic", "synthpop", "new wave", "80s", "synth pop"}, # Depeche Mode + {"new wave", "80s", "pop", "synth pop", "rock"}, # Duran Duran + ): + assert tags & artist_tags, artist_tags assert synths["years"] == [1975, 1992] + + +def test_an_excluded_tag_drops_the_artist(tmp_path): + """Weighting cannot separate the eighties synth acts from the eighties + stadium rock, because the tag they would be weighted on is the one they + share.""" + store, source, mirror = tagged_store( + tmp_path, + { + "Played Band": [{"name": "80s", "count": 100}, {"name": "hard rock", "count": 90}], + "Silent Band": [{"name": "80s", "count": 100}, {"name": "synth pop", "count": 90}], + }, + ) + vibes = [{"name": "eighties", "tags": ["80s", "synth pop"], "exclude": ["hard rock"]}] + + music_curator.build_vibe_playlists(store, vibes, mirror, str(source), 100, NOW) + + written = (mirror / "_playlists" / "eighties.m3u").read_text() + assert "Silent Band" in written + assert "Played Band" not in written + + +def test_a_vibe_cannot_both_select_and_exclude_a_tag(tmp_path): + path = tmp_path / "vibes.json" + path.write_text(json.dumps([{"name": "x", "tags": ["80s"], "exclude": ["80s"]}])) + + with pytest.raises(ValueError, match="selects on and excludes"): + music_curator.load_vibes(str(path)) -- 2.54.0 From a7d16ca0b2e2b419f75873a65a6cd7ca7aa2e26c Mon Sep 17 00:00:00 2001 From: Emma Thorpe Date: Mon, 24 Aug 2026 18:45:25 +0100 Subject: [PATCH 3/3] fix: give playlists the mirror's ownership rather than root's Playlists were being written owned by root. The image runs as root by default, deliberately, so that a bind-mounted dataset of any ownership stays writable -- but everything it writes then comes out root-owned, and a root-owned playlist inside a mirror owned by the apps account is unreadable to whatever serves it. The group-read bit does not help when the group is also root. Copy the ownership of the mirror the playlist is being written into. That needs no configuration, cannot drift from whatever the mirror actually is, and does nothing at all when the two already agree -- which is the case whenever the container is run with an explicit user. Applied to the temporary file before the rename, so a playlist is never briefly visible owned by the wrong account, and to the _playlists directory when this is the run that creates it. A chown that is refused is ignored rather than fatal: that only happens when not running as root, which is precisely the case where the ownership was already right. --- README.md | 7 +++++ music_curator.py | 47 +++++++++++++++++++++++++++++--- tests/test_music_curator.py | 53 +++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 2fb209e..3ae5d07 100644 --- a/README.md +++ b/README.md @@ -261,6 +261,13 @@ in step by hand. Override it if that guess is wrong. Entries are written **relative to the playlist file**, so one playlist works from the NAS, from a Mac over SMB, and from Linux, without rewriting. +Each playlist is given the **owner and group of the mirror** it is written +into. The image runs as root by default so that a bind mount of any ownership +stays writable, and the cost of that is output owned by root — which the account +serving the share cannot read, group bit or no group bit, because the group is +also root. Copying the mirror's own ownership avoids having to be told what it +should be, and does nothing when the two already agree. + A track is only listed once its mirror file has been confirmed to exist. Lidarr holding the FLAC says nothing about whether the MP3 has been encoded yet. If a large number are missing, the run says so — that is what a wrong `--library-root` diff --git a/music_curator.py b/music_curator.py index ec3c143..31e38c4 100644 --- a/music_curator.py +++ b/music_curator.py @@ -1534,7 +1534,7 @@ def build_vibe_playlists(store, vibes, mirror_root, library_root, limit, now): continue entries.append({**dict(row), "mirror": mirror}) - write_playlist(directory / f"{vibe['name']}.m3u", entries) + write_playlist(directory / f"{vibe['name']}.m3u", entries, Path(mirror_root)) total += len(entries) logger.info("playlist %-20s %4d tracks -- by tag", vibe["name"], len(entries)) @@ -1572,7 +1572,40 @@ def mirror_path_for(source, library_root, mirror_root): return (Path(mirror_root) / relative).with_suffix(MIRROR_SUFFIX) -def write_playlist(path, entries): +def set_ownership(path, uid, gid): + """Give a path an owner and group. Returns whether anything changed.""" + try: + current = path.stat() + if (current.st_uid, current.st_gid) == (uid, gid): + return False + os.chown(path, uid, gid) + except OSError: + # Not permitted unless running as root, which is the case where the + # ownership is already whatever the caller runs as. + return False + return True + + +def match_ownership(path, reference): + """Give a path the owner and group of the tree it is joining. + + The image runs as root by default, so that a bind mount of any ownership + stays writable. The cost is that everything it writes comes out root-owned, + and a root-owned playlist inside a mirror owned by the apps account is + unreadable to the thing that serves it -- the group bit does not help when + the group is root. + + Copying the mirror's own ownership avoids having to be told what it should + be, and is a no-op when the two already agree. + """ + try: + wanted = reference.stat() + except OSError: + return False + return set_ownership(path, wanted.st_uid, wanted.st_gid) + + +def write_playlist(path, entries, reference=None): """Write one extended M3U, atomically. Paths are relative to the playlist file, so the same playlist works from the @@ -1584,7 +1617,11 @@ def write_playlist(path, entries): lines.append(f"#EXTINF:{seconds},{entry['artist']} - {entry['title']}") lines.append(os.path.relpath(entry["mirror"], path.parent)) + fresh = not path.parent.exists() path.parent.mkdir(parents=True, exist_ok=True) + if fresh and reference is not None: + match_ownership(path.parent, reference) + handle, temporary = tempfile.mkstemp(dir=path.parent, suffix=".m3u.part") os.close(handle) temporary = Path(temporary) @@ -1595,6 +1632,10 @@ def write_playlist(path, entries): mode = temporary.stat().st_mode if not mode & GROUP_READ: temporary.chmod(mode | GROUP_READ) + # Before the rename, so the playlist is never briefly visible owned by + # the wrong account. + if reference is not None: + match_ownership(temporary, reference) os.replace(temporary, path) finally: temporary.unlink(missing_ok=True) @@ -1625,7 +1666,7 @@ def build_playlists(store, mirror_root, library_root, limit, now): missing += 1 continue entries.append({**dict(row), "mirror": mirror}) - write_playlist(directory / f"{name}.m3u", entries) + write_playlist(directory / f"{name}.m3u", entries, Path(mirror_root)) total += len(entries) logger.info("playlist %-20s %4d tracks -- %s", name, len(entries), description) diff --git a/tests/test_music_curator.py b/tests/test_music_curator.py index 78e752f..df74267 100644 --- a/tests/test_music_curator.py +++ b/tests/test_music_curator.py @@ -1,4 +1,5 @@ import json +import os import stat import urllib.error from pathlib import Path @@ -1289,3 +1290,55 @@ def test_a_vibe_cannot_both_select_and_exclude_a_tag(tmp_path): with pytest.raises(ValueError, match="selects on and excludes"): music_curator.load_vibes(str(path)) + + +def test_matching_ownership_is_a_no_op_when_it_already_agrees(tmp_path): + target = tmp_path / "file" + target.write_text("x") + + assert music_curator.match_ownership(target, tmp_path) is False + + +def test_ownership_failure_is_tolerated(tmp_path): + """Not permitted unless running as root -- which is exactly the case where + the ownership is already whatever the caller runs as.""" + target = tmp_path / "file" + target.write_text("x") + + # uid 0 from a non-root test process: refused, and must not raise. + assert music_curator.set_ownership(target, 0, 0) is False + + +def test_a_playlist_is_chowned_to_match_the_mirror(tmp_path, monkeypatch): + """The image runs as root, so its output is root-owned, and a root-owned + playlist in an apps-owned mirror is unreadable to whatever serves it.""" + api, source, mirror = playlist_library(tmp_path) + store = store_at(tmp_path) + music_curator.index_library( + music_curator.Lidarr("http://lidarr", "key", transport=api), store + ) + music_curator.match_library(store) + + attempted = [] + real_stat = music_curator.Path.stat + + def pretend_mirror_is_owned_by_568(self, *args, **kwargs): + info = real_stat(self, *args, **kwargs) + if self == mirror: + return os.stat_result( + (info.st_mode, info.st_ino, info.st_dev, info.st_nlink, 568, 568, + info.st_size, int(info.st_atime), int(info.st_mtime), int(info.st_ctime)) + ) + return info + + monkeypatch.setattr(music_curator.Path, "stat", pretend_mirror_is_owned_by_568) + monkeypatch.setattr( + music_curator.os, "chown", lambda p, u, g: attempted.append((str(p), u, g)) + ) + + music_curator.build_playlists(store, mirror, str(source), 100, NOW) + + assert attempted, "no ownership was applied" + assert all(tuple(owner) == (568, 568) for _, *owner in attempted) + # The temporary file, before the rename, never the finished playlist. + assert all(path.endswith(".part") or path.endswith("_playlists") for path, *_ in attempted) -- 2.54.0