Read embedded ASIN/ISBN tags during scan (fixes Rescan Metadata for tagged files)

[dny238]

Fixes #780

Problem

When ListenArr scans an existing audio file that has an ASIN embedded in its tags (the freeform iTunes atom ----:com.apple.iTunes:ASIN that Audible rips and most audiobook taggers write), the ASIN is never extracted. The imported audiobook gets no identifier, and Rescan Metadata then fails immediately with "No ASIN or ISBN identifiers are available for metadata rescan" — even though the value is sitting in the file.

Root cause: FfprobeTagMetadataMapper.Apply() maps title/artist/album/track/disc/year but never reads the ASIN (or ISBN) tag.

Fix

• FfprobeTagMetadataMapper.Apply() now reads the ASIN and ISBN tags into AudioMetadata. AudioMetadata already had Asin/Isbn properties, and GetTag already matches case-insensitively (so the ----:com.apple.iTunes:ASIN atom, which ffprobe surfaces as ASIN, is picked up).
• AudiobookFileService.EnsureAudiobookFileAsync() — the shared file-registration path the scan job uses — now adopts those identifiers onto the audiobook when it has none, persists via the audiobook repository, and records a history entry. Existing identifiers are never overwritten. Since AudiobookIdentifierMapper.GetEffectiveIdentifiers backfills from the legacy Asin/Isbn fields, setting them is sufficient for Rescan Metadata to succeed.

Scope / notes

• Adopts only when the audiobook is missing the identifier — no clobbering of user- or metadata-supplied values.
• Follows the issue's suggested approach (steps 1–3). ISBN is included alongside ASIN so nothing the mapper reads goes unused.

Tests

• New FfprobeTagMetadataMapperTests: ASIN + ISBN extraction, case-insensitive ASIN match, absent-tag leaves null, and existing ASIN not overwritten.
• dotnet build clean (0 warnings). Related suites (Files / Scanning / Ffmpeg / Metadata) — 174/174 pass.

🤖 Generated with Claude Code

[m4bard]

On #780 I said the MP4 key ought to be checked before anyone leaned on it, so I checked it rather than leave the hedge sitting there. Your assumption holds. Tagged files in each dialect, read with the command Listenarr runs (ffprobe -v quiet -print_format json -show_format -show_streams), tag appearing under format.tags:

dialect	written as	ffprobe exposes
MP4 / M4B	----:com.apple.iTunes:ASIN	ASIN
ID3v2.3	TXXX:ASIN	ASIN
ID3v2.4	TXXX:ASIN	ASIN (and AUDIBLE_ASIN)
Vorbis / FLAC	asin	asin

The freeform atom does come through unprefixed, so GetTag(tags, "ASIN", "asin") covers all four. The lowercase spelling is doing real work for FLAC. Nothing to change there.

There is one thing I'd think about before this lands, because it runs into a bug that's already live.

AdoptFileIdentifiersAsync takes the ASIN off a linked file and writes it onto the record, which is only as reliable as file-to-book attribution is. On canary that attribution is wrong in a way I measured on #784: scanning Ghost Stories of an Antiquary by M. R. James also linked Henry James' The Turn of the Screw and James M. Barrie' Peter Pan in Kensington Gardens, because the matcher will accept "this file sits somewhere on this author's shelf" as grounds for attribution.

The adopt only fires when the audiobook has no ASIN, which is the library-import case this PR is aimed at, so the two conditions overlap rather than excluding each other. A wrongly linked file donates its ASIN, TryPopulateMissingMetadataAsync resolves that ASIN upstream and fills the empty fields, and it all gets persisted with a history entry. The book now claims to be a different book, and that's harder to notice afterwards than a stray file in a list.

A cheap guard would be to require the linked files to agree: if the files linked to one audiobook carry more than one distinct ASIN, that disagreement is itself a signal the attribution is off, so adopt nothing instead of picking one. When attribution is right it changes nothing. Waiting until attribution is tightened would also do it, since #717 rewrites this area and wants a title match rather than accepting the author on its own.

Two smaller things:

• The ISBN path doesn't have the guard the ASIN path has. ASIN is adopted only when audiobook.Asin is blank, but ISBN appends any new value found in any scanned file regardless of what the book already holds. The doc comment says existing identifiers are never overwritten, which is true, but it doesn't mention the accumulation, and that's the route a wrongly linked file takes.
• The new if (exists) branch re-extracts metadata for every already-registered file whenever the book still has no ASIN. For a book that genuinely has no embedded ASIN, that's an ffprobe per file on every scan, with nothing remembering the negative result.

On the tests: they assert against hand-built JSON, so they'd pass unchanged even if ffprobe surfaced the tag under some other key, which is the one thing this PR depends on. The table above is that assumption actually checked. I have a corpus of tagged public-domain files covering every dialect if a fixture test would help, and I'm happy to hand over the files or the generator that makes them.

[dny238]

Thanks for actually tagging files in each dialect and checking format.tags — that's the assumption the whole PR rests on, and having it verified rather than asserted is exactly what the tests couldn't give you. Appreciate the offer of the corpus; a fixture built from real tagged files would be a strict improvement over the hand-built JSON, and I'd take you up on the generator.

Pushed 4def14c addressing the three points:

• Attribution guard. Adoption now requires agreement: ResolveUnanimousFileAsinAsync collects the ASINs across every file linked to the book and adopts only when they're unanimous — on any disagreement it adopts nothing rather than picking one, so a mis-attributed file can't donate its identifier for the auto-refresh to then act on. This is defense-in-depth; the real fix is attribution itself, which is why Match scanned files when the on-disk name drops a leading article #784 now drops its author arm and Harden path identity, durable moves, scans, and root relocation #717 tightens the matcher.
• ISBN. No longer accumulates — it's adopted only when the book has none, same rule as ASIN. That closes the route you flagged where a stray file appends onto an existing set.
• ffprobe cost. The already-registered re-read now reuses the per-file/mtime cache the create path populates, so a book with no embedded ASIN doesn't re-run ffprobe on every file each scan.

One honest limitation I'd rather name than paper over: the agreement check sees the files linked at the moment adoption fires, and adoption fires on the first file that carries an ASIN, so a conflicting file linked later in the same scan can still slip through the window. Tightening that fully means moving adoption to a post-scan step once the file set is settled — happy to do that if you'd prefer it over the incremental guard, but it felt like more surface than this PR should carry given #717/#784 are removing the mis-attribution at the source.

[m4bard]

All three land, and the unanimity rule is a better shape than what I suggested. Refusing on disagreement rather than trying to pick a winner is the part that makes it safe.

Readarr's answer to this is neither of the two options we were choosing between. It never adopts an identifier onto a book at all: it groups the files, identifies the group against candidate editions, and treats the embedded ASIN as one weighted input to that choice.

IdentificationService.cs#L50-L55 groups first, then identifies each group:

releases = _trackGroupingService.GroupTracks(localTracks);
foreach (var localRelease in releases) { ... IdentifyRelease(localRelease, ...); }

DistanceCalculator.cs#L89-L98 reads the ASIN across that group and scores it:

var asin = localTracks.MostCommon(x => x.FileTrackInfo.Asin);
if (asin.IsNotNullOrWhiteSpace() && edition.Asin.IsNotNullOrWhiteSpace())
    dist.AddBool("asin", asin != edition.Asin);
else if (asin.IsNullOrWhiteSpace() != edition.Asin.IsNullOrWhiteSpace())
    dist.AddBool("asin_missing", true);

with the weights in Distance.cs#L16-L19.

Grouping before identification has been there since 45d49117 in July 2020, so this is a settled design rather than a recent experiment.

MostCommon means plurality decides across the group, so where your check refuses on disagreement, Readarr takes the majority. Both are defensible and I do not think this is my call: refusing is the safer default, taking the majority matches the prior art, and there is a middle version where you take the majority but log the disagreement so it is visible rather than silent. Which of those do you want here?

The weights are worth a look either way. 847a9eae, "Improve match by adding small penalty for missing isbn/asin", November 2021, puts a contradicting identifier at 10.0 and a missing one at 0.1. Absence of evidence costs almost nothing, conflicting evidence is close to fatal. That is the same instinct as your unanimity rule, expressed as a score rather than a veto.

On the window you asked about. It is narrower than "later in the same scan", and it cuts in a direction worth naming. Adoption runs from EnsureAudiobookFileAsync, so once per file, and only while audiobook.Asin is blank. It fires on the first file carrying an ASIN and never runs again for that book, so whatever happened to be registered before that file is the entire set the check gets to see.

That makes the two paths behave differently. Re-scanning a book that already has its files linked, your new if (exists) path, hands it the settled set, and the agreement is real. A first scan reaches the first tagged file with almost nothing linked yet, often only that file, so the set is unanimous by having one member and the check passes without telling you anything. The first scan is also when misattribution happens, so the guard is weakest on the case that motivated it.

None of which is a reason to hold the PR. Deciding once the file set has settled is what actually closes the window, but it is a different shape of change, and you are right that #784 and #717 remove the cause rather than the symptom. The one thing I would add now is a sentence on ResolveUnanimousFileAsinAsync recording that the set is whatever was linked when the first tagged file arrived. Otherwise the limitation lives only in this thread, and the next person will read a guard that looks airtight and wonder why it did not fire.

I can measure it instead of leaving it as a reading. The corpus has a deliberate wrong-ASIN tag state and the attribution harness can build a library where a scan links a file belonging to another book, so pointing those at each other would show whether a conflicting ASIN reaches adoption before the check sees it. Say the word and I will run it against 4def14c9.

On fixtures, yes, take whatever is useful. tools/generate_library.py --scenario tag-dialects writes the same ASIN into MP4 atoms, ID3v2.3, ID3v2.4 and Vorbis comments, so you get one real file per dialect instead of four hand-written JSON blobs. Public domain, verified ASINs, one second of silence each, so the whole set is kilobytes. I can cut a fixture directory and open it as a PR against your branch if that is easier than wiring in a generator.

[therobbiedavis]

#717 changes AudiobookFile registration into a database-enforced filesystem identity/ownership contract and serializes path-bearing mutations through the audiobook operation boundary. The ASIN/ISBN tag extraction in this PR remains distinct, but please rebase on #717 and make identifier adoption run through the new registration/operation-lock flow so it cannot race file ownership or concurrent audiobook updates.

[dny238]

@therobbiedavis sounds good — I'll rebase this on #717 once it lands and route the identifier adoption through the new registration/operation-lock flow so it can't race file ownership or concurrent audiobook updates. Happy to hold until #717 merges so I'm building against its final shape rather than a moving target.

@m4bard thanks for the careful read. You're right that the guard is weakest on a first scan, where the first tagged file is "unanimous" just by being the only member — I've pushed a doc comment on ResolveUnanimousFileAsinAsync spelling out exactly that limitation (it refuses once a second, disagreeing file appears, but can't retroactively un-adopt an ASIN from a lone early file). And yes please on the fixtures — real tagged files across tag dialects would let me replace the hand-built metadata in the tests with something closer to what actually lands on disk.

[m4bard]

Fixtures are ready. tools/make_tag_fixtures.py in https://github.com/m4bard/listenarr-testdata builds them:

python3 tools/make_tag_fixtures.py --out fixtures/tag-adoption

Six directories, each one book whose files are tagged individually:

case	files	what it is
agree-same-dialect	3 tagged	the easy adopt
agree-mixed-dialect	3 tagged	same ASIN, spelled three ways
disagree	2 tagged	two different real ASINs
partial-one-tagged	1 of 3 tagged	one tagged file among bare ones
partial-lone-file	1 file	a single tagged file, nothing else
untagged	0 tagged	no identifier in any spelling

agree-mixed-dialect is the one that will exercise your extraction rather than your comparison. The same ASIN comes back from ffprobe as ASIN on the m4b, as both ASIN and AUDIBLE_ASIN on the mp3, and as lowercase asin on the flac, so agreement has to be decided after extraction and cannot be done by comparing raw tag keys.

partial-lone-file is the case from your doc comment. One tagged file is unanimous by construction, so a guard that adopts on agreement adopts from it, and there is no second file to disagree later.

partial-one-tagged is the question next to it: one tagged file plus two bare ones is unanimous among the files that carry an identifier at all, which is a choice worth making explicitly rather than falling into.

Each set writes a manifest.json listing, per file, which ASIN was written and under which keys, so a test can assert against that rather than against hardcoded strings. Audio is one second of generated silence, so the whole set is small. Both ASINs are real catalogue values from the corpus and a test asserts they resolve there.

Two things I checked rather than assumed. Every file was read back through ffprobe to confirm the identifier actually surfaces, because fixtures that silently carry nothing would turn a passing test into a test of nothing. And rebuilding replaces the set rather than merging into it, which matters because a stale file surviving into untagged would make that case quietly wrong.

If the shape is not what you need, say what is missing and I will add cases. Adding one is a few lines.

[dny238]

Pushed the rework onto current canary (post-#717). Summary of how it now fits the new file-registration flow:

• FfprobeTagMetadataMapper reads ASIN / AUDIBLE_ASIN / ISBN tags into AudioMetadata, so a scanned file's embedded identifier is available during registration.
• Identifier adoption happens in a new AudiobookFileService.IdentifierAdoption partial, called from inside EnsureAudiobookFileCoreAsync — i.e. within the filesystemMutationCoordinator → audiobookOperationCoordinator locks, on the snapshot loaded there. So the identifier write can't race file ownership or a concurrent audiobook update. The unanimity guard still refuses to adopt when the book's linked files carry disagreeing ASINs.
• @therobbiedavis — per your note about not holding the lock: the Audible metadata refresh runs after the lock is released. The core signals adoptedIdentifier out via a StrongBox<bool>, and the outer EnsureAudiobookFileAsync does the network lookup post-lock (filling empty fields only). So the network call never holds the global filesystem lock.

Build + suite green: ffprobe-mapper tests (incl. the AUDIBLE_ASIN dialect + no-overwrite), FillMissingFields, and the coordination test updated for the new ctor dependency; 21 file-service/scan tests still pass.

@m4bard — I'd love to take you up on the tag-dialect fixtures for real integration coverage here (the agree-mixed-dialect and the two partial-* cases especially). Point me at the generator and I'll wire them into the adoption tests.

[m4bard]

The generator is tools/make_tag_fixtures.py in https://github.com/m4bard/listenarr-testdata:

python3 tools/make_tag_fixtures.py --out ./build/tag-adoption

It calls ffmpeg from PATH; --ffmpeg overrides that. All three cases exist under the names you used. agree-mixed-dialect is three files carrying one ASIN spelled three ways: an iTunes freeform atom on the m4b, TXXX:ASIN plus TXXX:AUDIBLE_ASIN on the ID3v2.4 mp3, and a lowercase asin Vorbis comment on the flac. partial-one-tagged is one tagged file among two bare ones. partial-lone-file is the single tagged file your doc comment describes. Each run also writes a manifest.json that records, per file, the ASIN written and the keys it went under, so a test can assert against the manifest rather than against strings typed into the test. The README section "One fixture set that is not a scenario" has the rest.

I read the diff at 08996b8 but have not run it. The guard and the post-lock refresh both look like your description: ResolveUnanimousFileAsinAsync collects the current file's ASIN plus each linked file's and returns null above one distinct value, and the StrongBox signal holds the metadata lookup until ExecuteExclusiveAsync has returned.

Two cases the tests do not seem to reach. GetTag skips whitespace-only values, so an ASIN tag that is present but empty falls through to the next spelling. That reads correct, but nothing pins it. And within one file, an ASIN and an AUDIBLE_ASIN that disagree resolve silently to whichever name comes first in the list, since the unanimity guard compares across files rather than within one. Is intra-file precedence settled for you, or is it worth a fixture? The generator does not produce either case today; both are a few lines to add if you want them.

Disclosure: this was produced with Claude Code at my direction. Every file and line cited was re-read against the stated commit, and anything described as verified was verified by running it. I reviewed the text before posting. I have not read every line of the diffs I am citing.

[dny238]

Thanks for the fixtures and the code read.

I've pinned both cases you raised with unit tests at the FfprobeTagMetadataMapper level (they live entirely in GetTag, so JSON tag inputs cover them without a binary fixture):

• Empty tag fall-through: a whitespace-only ASIN is skipped and the next spelling wins.
• Intra-file precedence: a single file with disagreeing ASIN vs AUDIBLE_ASIN resolves to the first name in the lookup order (ASIN before AUDIBLE_ASIN). So yes — intra-file precedence is settled and now pinned; the unanimity guard stays deliberately cross-file only, and there's no silent intra-file resolution left unasserted.

Your fixtures are still valuable for exercising the extraction end-to-end (the agree-mixed-dialect case in particular), so I'll keep them in mind for a broader integration pass — but for these two specific behaviors the mapper-level tests are the tightest place to pin them.

[m4bard]

Agreed, and I think you put them in the right place. Both behaviours are inside GetTag, which takes the parsed tag object rather than a file, so a binary fixture would only have wrapped ffprobe around an assertion that is not about ffprobe.

I read both tests at c6428cb6 rather than running them. The precedence one does what I was hoping for: it fails if the name list in FfprobeTagMetadataMapper.Apply is reordered, so ASIN winning over AUDIBLE_ASIN stops being true by accident. The whitespace one pins the fall-through rather than the next spelling happening to win.

So I am not adding either case to the generator, and the fixture set stays as it is. If the broader integration pass happens, agree-mixed-dialect is the one I would point at, since that is the case where one ASIN has to survive three different on-disk encodings before anything compares it. I am not asking for anything there.

Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting.