Skip to content

Fix/reorganize retag split - #1186

Draft
nick2000713 wants to merge 4 commits into
Nezreka:devfrom
nick2000713:fix/reorganize-retag-split
Draft

Fix/reorganize retag split#1186
nick2000713 wants to merge 4 commits into
Nezreka:devfrom
nick2000713:fix/reorganize-retag-split

Conversation

@nick2000713

@nick2000713 nick2000713 commented Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

reorganize was doing three jobs,

i found this on my Library V2 branch ,i had spent an
evening fixing album and track titles by hand in the library, then ran
Reorganize to move the files onto the new template, and it renamed them back..
not to the names i had fixed, to the names a metadata provider had.

so i looked at what Reorganize actually does, and it is not renaming files. it
copies every file into a staging folder and runs it through
_post_process_matched_download, which is the download pipeline. the same code
that decides whether a file some stranger sent over soulseek is acceptable,
pointed at files i already own, that are already in my library, that i had just
edited by hand. it re-tagged them from a provider and it never asked whether
any of those values were mine.

once i saw that, alot of old bug reports stopped looking like separate bugs.

the pattern

every time Reorganize hurt somebody, the fix was to switch off one more piece
of the download pipeline. reading the file top to bottom:

nine patches, and every one of them says the same sentence in different words..
where the provider and the user's own library disagreed, the library was right.
nobody wrote that sentence down anywhere, so it got rediscovered nine times.

and then it stopped being patches:

at that point Reorganize had two planners, two executors, a mode dropdown, a
source dropdown, nine opt out flags, and it still quarantined my own files.

the diagnosis is in none of those nine. it is that Reorganize was wired to the
wrong pipeline. post processing knows how to pick a destination and write tags,
so reusing it looked free.. it is not free, it also decides whether to accept
a file, and that question makes no sense about a file the user already owns.
each fix cut one more wire instead of unplugging it.

what it is now

two jobs, two names. re-tag writes tags, reorganize writes paths. that is the
whole PR.

reorganize

_plan_from_catalogue reads the album's own rows and nothing else, so the
filename is the title the Library page shows. no provider call, which means:

  • an album with no stored source id is reorganizable. the old planner
    refused it outright, for an operation that needs no provider at all
  • the preview is offline. no multi second wait, and no Invalid base62 id 400s
    from candidate ids that were never spotify's in the first place
  • total_discs is the layout the catalogue knows, not one a live tracklist
    decides differently on every call

reorganize_album_rename_only is the only executor now. #875's "rename only" is
just the behaviour. reorganize_album with its staging, watchdog, per track
post processing and 3 thread pool is gone.

and all nine opt outs go with it. not because i removed them one by one, but
because there is no acceptance check left to opt out of and no provider left to
disagree with. _keep_user_casing, _keep_user_year and the disc cap all
become true by construction once the plan reads the catalogue.

core/library_reorganize.py goes from 2504 to 981 lines.

the destination is still built by
core.imports.paths.build_final_path_for_track through the same context shape
post processing uses, so a reorganize destination and a fresh download's
destination still cannot drift apart. that was the one good reason for the
original design and it is kept.

sidecars travel now. the full mode deleted a track's .lrc/.nfo/.cue at
the source, which was only safe because post processing re-created them at the
destination from a provider. a move has no second half, so they get carried,
and never over a file that is already at the destination. album cover art moves
too once all audio has left the folder, so an emptied folder can finally be
pruned.

re-tag

same underlying problem one layer down.. the thing that decided was not the
thing that wrote.

core/tag_writer.py owns the tag diff and the tag write as a pair.
build_tag_diff and write_tags_to_file apply the same guards on purpose,
there is even a comment saying so ("applies the SAME check … so this preview
always matches the actual write outcome"). the re-tag job did not use it. it
carried its own comparison in retag_planner.plan_track, which knew none of
those guards, and read the file with a different reader than the writer
consults.

so it promised changes the writer then refused. and because _create_finding
refreshes a pending row in place, a finding like that never goes away.. you
apply it, you get told it worked, and it is back on the next scan. two ways in,
both reproduced with running code:

file genre 'Rock, Pop, Indie'  vs  source ['Rock','Pop']
  planner:  genre changes                              -> finding
  writer:   genre_write_value_is_subset_of_existing    -> keeps the file's value

file album artist 'Real Band'  vs  a compilation's 'Various Artists'
  planner:  artist changes                             -> finding
  writer:   guard_placeholder_overwrite (#800)         -> keeps the file's value

the second one also promised the user the exact opposite of what they want.

and one it could not see at all: _current_value('artist') compared against
album_artist, so a file with a correct album artist and a wrong ARTIST tag
looked perfectly tagged. build_tag_diff has carried both fields all along.

plan_track hands a full payload to build_tag_diff now and keeps only the
keys whose field really changed, so applying still touches nothing the finding
did not show. fields the writer holds back come back as protected and the
finding says so, instead of listing a change that will not happen. the reader
is read_file_tags, the same one the guards use.. soulsync_client._read_tags
takes mutagen's easy view and only the first value of each frame, so a multi
genre file was read back as one genre and the two halves disagreed about the
same file.

three more while i was in there:

  • the default was flagging the whole library. cover_art: 'replace' was the
    default and a cover action on its own is enough to create a finding, so every
    matched album got one on every scan, tags perfect or not, forever. it is
    fill_missing now. fill missing also asked "has this album got art?" of the
    raw db path, so on a path mapped setup the folder does not exist, the answer
    was always no, and it behaved exactly like replace
  • cover.jpg went to the directory of the last track written, so a
    Disc 1/ + Disc 2/ album got exactly one
  • three different definitions of "which albums are matched to a source" had
    drifted. reorganize accepted six, this job four, so a discogs or hydrabase
    matched album was reorganizable but not re-taggable and fell out of a
    continue without a word. one ALBUM_SOURCE_ID_COLUMNS now

the one i want you to decide, not me

the third commit (a302429e4) deletes both old planners, which means it removes
#592 "Embedded tags" as a feature.

my argument is that the catalogue planner is what #592 was reaching for. zero
api calls, trusts what the user already has, but read from the library instead
of from the files, so a title you corrected in the ui is the name that lands on
disk. for a locally scanned library the catalogue is the file tags anyway
(SoulSyncTrack.title = tags['title']), so tag mode computes the same answer
through a second implementation and a disk read per file. where they do
diverge, the catalogue is the one that knows about your corrections.

but it is your feature and your call, so it is its own commit. drop
a302429e4 and you keep the first two, the re-tag fix and reorganize only
moves, with both old planners still in place. nothing in the first two depends
on it.

tests

python is 14943 passed, 1 failed. that one failure
(watchlist/test_batch_add_reporting) is red on plain dev too, this branch does
not touch it. frontend artist-detail is 878 passed, oxfmt and oxlint clean
on everything changed.

seven test files went with the code they tested.. provider resolution, edition
matching, feat matching, disc layout capping, tag mode, the unknown artist hint
and download/reorganize path agreement.
test_library_reorganize_orchestrator.py tested the deleted executor, what
survives of it is test_reorganize_preview.py, ported to the catalogue. new
ones are test_reorganize_from_catalogue.py, including a provider stub that
raises if anything reaches for one, plus the sidecar cases and runner cases
pinning that every item routes to the mover.

where this came from and where it goes

this PR is the clean up. on library-overhaul i took it further, because
Library V2 has somewhere to put the thing dev cannot express.. that the user
said so.

there it is three roles instead of two:

Manual Match  ->  identity only, the provider id
Re-tag        ->  provider -> catalogue -> file tags, both halves reviewable
Reorganize    ->  catalogue -> path, offline and idempotent

lib2 keeps a per field user override layer (lib2_metadata_overrides) and every
read path projects it, so the value the Library page shows is the value that
gets written. re-tag read the base row instead, which is exactly the bug i
opened this PR describing.. a title i had corrected by hand, overwritten in the
file with the value the page no longer showed. fixing that turned into a rule:
hand beats provider, but the user decides per field.

what that buys, none of which dev can express today:

  • the diff row carries three values instead of two. what is in the file, what
    will be written (your override), and what the catalogue wanted:
    Vogel Im Kafig / Vogel im Käfig / Vogel im Käfig (OST)
  • a conflicting row expands and offers "keep mine / take theirs" per field, and
    only for rows that actually conflict, not a field matrix over 500 tracks
  • the write api takes an explicit release list,
    write_tags(..., overwrite_manual=[(track_id, field), …]), instead of one
    global switch
  • findings carry which fields are hand set and the bulk prompt says so.. "23
    findings, 4 with fields you set by hand, [Keep My Edits (19)] [Overwrite My
    Edits Too (23)]"
  • the third role has its own module, core/library2/catalogue_refresh.py, which
    is provider -> catalogue. position matching on disc+track with title
    similarity as the fallback, every source row consumed once, and a track that
    is not found comes back matched: False instead of quietly vanishing.
    accepting a suggestion deletes the override rather than writing the base
    row, otherwise the override just keeps winning on every read path afterwards
  • the job is scoped, so "run this for one artist" cannot produce library wide
    findings that are one Fix All away from touching everything else

same shape as this PR, one layer sharper. happy to walk through any of it if it
is useful for where dev is heading.

dev added 4 commits August 25, 2026 22:47
dev
fix(retag): the job decides with the same engine that writes
a7e8008 
`core/tag_writer` owns the tag diff and the tag write as a pair: `build_tag_diff`
and `write_tags_to_file` apply the same guards on purpose, with a comment saying
so. The re-tag job did not use it. It carried its own comparison in
`retag_planner.plan_track`, which knew none of them, and read the file with a
different reader than the one the writer's guards consult.

So the job promised changes the writer then refused. And because
`_create_finding` refreshes a pending row in place rather than inserting a new
one, such a finding never goes away: apply it, get told it succeeded, see it
again on the next scan. Two ways in, both reproduced:

* file genre `Rock, Pop, Indie` against a source's `Rock, Pop` — the planner
  called it a change, `genre_write_value_is_subset_of_existing` kept the file's
  richer value.
* a compilation's `Various Artists` over a real name — the planner called it a
  change, the Nezreka#800 placeholder guard kept the file's value. That one also
  promised the user the opposite of what they want.

And one it could not see at all: `_current_value('artist')` compared against
`album_artist`, so a file with a correct album artist and a wrong ARTIST tag
looked perfectly tagged. `build_tag_diff` has carried both fields all along.

`plan_track` now shapes the source's values into a full payload, hands it to
`build_tag_diff`, and keeps only the keys whose field actually changed — so an
apply still touches nothing the finding did not show. Fields the writer holds
back come back as `protected` and reach the finding, which says so instead of
listing a change that will not happen. The reader is `read_file_tags`, the same
one the guards use; `_read_tags` took mutagen's easy view and only the FIRST
value of each frame, so a multi-genre file was read back as one genre.

An unreadable file is now skipped rather than planned. `{}` for "could not
read" is indistinguishable from "has no tags", which turned a file nobody could
open into a finding claiming every field was wrong.

The year is deliberately passed as a year and not as `release_date`: with a
year-only value build_tag_diff PRESERVES a more specific date already in the
file (Nezreka#824), instead of flattening every dated file in the library on the first
scan.

**Cover art no longer flags the whole library.** The default was
`cover_art: 'replace'`, and a cover action alone is enough to create a finding —
so every matched album got one, tags perfect or not, on every scan. The default
is `fill_missing` now; `replace` stays as the deliberate "re-pull all my art"
run. Fill-missing also asked its question of the RAW db path, so on a
path-mapped setup the folder did not exist, the answer was always "no art", and
it behaved exactly like replace. It resolves the path first now — once, for the
whole album, instead of per track.

**One list of matched sources.** Three definitions had drifted: reorganize
accepted six sources, this job four. A Discogs- or Hydrabase-matched album was
therefore reorganizable but not re-taggable, and it fell out of a `continue`
without a word. `ALBUM_SOURCE_ID_COLUMNS` in `core/metadata/registry.py` is now
the one definition, shared by both. (`track_number_repair` keeps its own on
purpose — adding MusicBrainz there changes a job this change is not about.)

**cover.jpg reaches every folder the album occupies.** It went to the directory
of the LAST track written, so a Disc 1/ + Disc 2/ album got exactly one.

Tests: 4 new planner cases for the guards, 3 for the job's reader and held-back
fields, 2 for the cover default and the resolved path, 2 for eligibility and the
multi-disc sidecar. Four existing fixtures gained an `artist` tag — they modelled
a file that had none, which is precisely what the old planner could not see.
dev
refactor(reorganize): a reorganize moves files, and that is all it does
573f9cb 
A reorganize applies the current file-organization template to files the user
ALREADY OWNS. It was doing considerably more than that.

**It ran an acceptance check on the library.** Every file was copied into a
staging folder and pushed through `_post_process_matched_download` — the
DOWNLOAD pipeline, which exists to decide whether a file of unknown origin may
be kept. The library kept failing it, and four opt-outs accumulated in the
context builder, one per report:

* `is_local_import` (Nezreka#804) — the integrity leg quarantined a copy over a
  duration the re-resolved provider tracklist disagreed with ('Through Glass',
  283s vs Discogs' 241s).
* `_skip_quarantine_check: 'acoustid'` (Nezreka#1182) — the identity leg quarantined a
  file over its OWN fingerprint. Sawano Hiroyuki fingerprints as 澤野弘之, so
  moving a track you own ended in `status=failed, moved=0` and a ~40MB
  quarantined copy, and the run only worked on a second attempt with "Rename
  only" ticked.
* `_no_album_folder_reuse` (Nezreka#829) — Nezreka#829's existing-folder reuse resolved the
  folder the album was being moved OUT of, so every already-together album
  previewed as `unchanged` and a template change silently no-opped.
* plus `_keep_user_casing` twice and `_keep_user_year` (Nezreka#1078, Nezreka#1080), each
  added after a report, each saying: where the catalogue and the provider
  disagreed, the catalogue was right.

It also re-tagged — work the Library Re-tag job already does, from a source it
can show you first — and copied ~800MB for a 20-track FLAC album.

**And it needed a provider to decide where a file goes.** The tracklist came
from a live call, so an album with no stored source id could not be reorganized
at all (`no_source_id`, "run enrichment first"), a preview took seconds, and
candidate ids that were never Spotify's produced `Invalid base62 id` 400s.

So both halves are replaced by the thing that was underneath all along:

* `_plan_from_catalogue` reads the album's own rows. Offline, no source to
  resolve, and the filename is the title the Library page shows. The three
  `_keep_user_*` patches become true by construction; `total_discs` is the
  layout the catalogue knows rather than one a live tracklist decides
  differently on each call.
* `reorganize_album_rename_only` is the only executor. Nezreka#875 asked for a mode
  that only moves; it is the whole behaviour now. `reorganize_album` and its
  staging, watchdog, per-track post-processing and concurrency pool are gone
  (−584 lines), and the two acceptance-check opt-outs go with them: there is no
  check left to opt out of.

The destination is still built by `core.imports.paths.build_final_path_for_track`
through the same context shape post-processing uses, so a reorganize
destination and a fresh download's destination still cannot drift apart.

**Sidecars travel.** The full mode DELETED a track's .lrc/.nfo/.cue at the
source because post-processing re-created them at the destination. A move has
no second half, so `_move_track_sidecars` carries them — never over one already
at the destination. `_delete_album_sidecars` is deleted rather than called for
the same reason: it swept an emptied folder's cover art, which nothing would
re-create now. (Consequence, stated rather than hidden: a folder left holding
only a cover.jpg is not pruned. No data is lost.)

**Nothing left to configure.** The mode picker (Nezreka#592 'tags' vs 'api'), the
source picker, and the "Full reorganize / Rename only" action select are gone
from both modals; the endpoints take no body; the queue item no longer carries
`metadata_source` or `rename_only`. The Tools job's Nezreka#862 api→tags fallback goes
too — it existed because media-server albums have no source ids, which is no
longer a question anyone asks.

The provider and tag planners remain reachable by explicit argument and keep
their tests; nothing in the product asks for them. Removing them means removing
Nezreka#592 as a feature, which is a product decision, not a cleanup.

Tests: `test_library_reorganize_orchestrator.py` tested the deleted executor —
35 of its 45 tests were staging, post-processing, concurrency and provider
resolution. What survives is `test_reorganize_preview.py`, ported to the
catalogue. New: `test_reorganize_from_catalogue.py` (the planner, incl. a
provider stub that raises), three sidecar cases, and two runner cases pinning
that every item routes to the mover.
dev
refactor(reorganize): one planner, and it reads the library
a302429 
Follow-up to the previous commit, which made the catalogue planner the default
and left the provider and tag planners reachable by explicit argument. Nothing
in the product asked for them, so they were a second answer to a question that
now has one — and a second answer no reviewer could tell was dead.

Removed with them: `_resolve_source` and the source fallback chain, alternate-
edition resolution and scoring, `_find_api_track` / `_prenormalize_api_tracks`
and the feat-credit matcher, `_keep_user_casing` / `_keep_user_year`, the
single-disc cap heuristic, `available_sources_for_album` / `authed_sources` and
the two `/reorganize/sources` endpoints, and `core/library/reorganize_tag_source.py`.
`core/library_reorganize.py` is 1904 -> 981 lines.

**This removes Nezreka#592 ("Embedded tags" mode) as a user-facing feature.** It was a
second way to answer "where does this file go" that existed because the first
way needed a provider. The catalogue planner is what Nezreka#592 was reaching for —
zero API calls, trusts what the user has — but read from the library rather
than from the files, so a hand-corrected title in the UI is the name on disk.
Stated plainly because it is a product decision, not a cleanup.

**A source is no longer part of a reorganize.** `QueueItem` loses `source` and
`result_source`; `enqueue`/`enqueue_many` no longer take one; the Tools job
stops putting one on the items it enqueues. Nothing asks a provider, so there
is nothing about "which source" left to carry, log, or display.

**Album sidecars travel too.** `_move_album_sidecars` moves cover art and
album-level sidecars once ALL audio has left the source folder — never over an
existing destination file, and unrecognised real content (a PDF booklet) and OS
junk stay put. That closes the leftover-folder gap the previous commit named:
an emptied folder can now actually be pruned.

**`_extract_source_ids` moved rather than died.** `core.metadata.canonical_resolver`
imports it at runtime, and canonical resolution has nothing to do with
reorganizing. It lives beside the column map it reads, as
`core.metadata.registry.extract_album_source_ids`.

The `album_needs_enrichment` branch is gone from the Tools job: the catalogue
planner never returns `no_source_id`, so the finding could only ever be a
dead-end telling the user to enrich an album that needs no enrichment. The
finding TYPE stays registered so historical rows still render.

Tests: seven files went with the code they tested (provider resolution, edition
matching, feat matching, disc-layout capping, tag mode, the unknown-artist
hint, and download/reorganize path agreement). `test_reorganize_queue.py`,
`test_reorganize_runner.py` and `test_library_reorganize.py` drop their
per-item source. `test_case_folding_integration.py` no longer stubs two
reorganize settings that no longer exist. Four new cases pin the album-sidecar
move. Suite: 14943 passed, 1 pre-existing failure
(`watchlist/test_batch_add_reporting`, red on clean dev too).
dev
fix(reorganize,retag): four things the review caught
cb0e9a2 
**A wrong album artist took the track artists with it.** `_WRITE_KEYS` mapped
the `album_artist` diff row to `artist_name` alone, but `write_tags_to_file`
writes the ARTIST tag from `track_artist or artist_name`. So when only the
album artist changed, the payload carried no `track_artist` and the album
artist landed in the track's ARTIST tag too:

    file:   artist='Guest Band'  albumartist='WRONG'
    source: track artist 'Guest Band', album artist 'DJ Alpha'
    plan:   {'album_artist': {'old': 'WRONG', 'new': 'DJ Alpha'}}
    writer: ARTIST <- 'DJ Alpha'

On a compilation or a DJ mix, a finding that said nothing about the track
artists would have replaced every one of them. The row writes both keys now.

**A half-downloaded multi-disc album lost its disc folders.** The catalogue
only knows the discs whose tracks have been imported, so a 2-disc album with
disc 1 in the library reads as single-disc — and `total_discs_declared` then
stops the path builder asking anyone else. The download that filed those tracks
DID know (it asked a provider), so it wrote `Album/Disc 1/…` and the reorganize
plan proposed `Album/…`: out of the disc folder now, back into it when disc 2
lands. That flip-flop is the thing this branch exists to stop.

The folder the files are already in is evidence the imported rows do not carry,
so it gets a say: a track sitting in a `Disc N` directory means the album is
filed by disc, and the plan keeps it that way. Offline — it reads the stored
path string, not the disk.

**A sibling-format file could overwrite one at the destination.**
`_move_sibling_to_destination` called `shutil.move` with no existence check,
which on one filesystem is `os.rename` and clobbers. Two frames up, the
canonical move refuses the same situation outright rather than destroy a file
nobody asked about. It refuses now too.

**And siblings were carried ahead of the audio.** They moved before
`os.rename(current_abs, new_abs)`, so a rename that then failed left the `.opus`
at the new path while the `.flac` and the catalogue row still named the old one
— from a track the summary counted as failed. They follow the audio now, the
same order `_move_track_sidecars` already used and states the reason for.

Four tests, one per finding. Suite: 14948 passed, 1 pre-existing failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nick2000713