[magrhino]

Summary

Hardcover edition lookups return audio_seconds, but conversion discarded it, leaving some wanted audiobooks with durationSeconds: 0 when search began. This change fills a missing book runtime from the chosen audio edition, so the opt-in size-per-minute scoring in #2750 can use the available book metadata. Closes #2755. Closes #2756. Refs #2750.

• Carry the positive edition runtime through hydration without replacing an existing book duration or mixing runtimes across known ASINs.
• Rank audio evidence ahead of runtime when choosing the edition; runtime only breaks ties between equally scored editions, so it cannot displace better language and cover metadata.
• Never promote a known print format to audio on runtime alone, including when a stored Unknown format would hide the fetched print format.
• Respect a locked language (Book.CanWrite), including one deliberately cleared to empty, on both the duration-only and full write paths.
• Use a guarded duration-only database update keyed on the book identity (foreign_id, metadata_provider, media_type, asin) and an unknown stored duration; each clause is pinned by its own test.
• Normalize whitespace-only values for all nine edition text columns during metadata upsert, including legacy rows and Unicode whitespace; meaningful stored values keep their spacing.
• Add focused repository and provider-to-book tests, a user-guide note, and a changelog fragment.

Scope

• One reviewable objective: preserve available Hardcover audio runtime before search, without weakening edition selection.
• Split per review: concurrent-edit protection for the full hydration write (UpdateHydratedMetadata, the updated_at precondition, ReloadHydratedBook, the SetLanguage bump, and edition-write rebind atomicity) moved to fix(metadata): preserve concurrent edits during hydration (#2758) #2794, which closes Hardcover edition hydration can overwrite concurrent book edits during metadata promotion #2758. This PR still persists non-duration changes through Books.Update, as main does today.
• Follow-up work: release scoring stays in feat(indexer): normalise the audiobook size bonus by runtime and cap grabs (#2740) #2750; candidate-specific runtime selection and file upgrades are separate.

Checklist

• Commits signed off under the DCO.
• Tests added or updated.
• User-facing behavior documented in docs/User-Guide-Wiki.md.
• Added changelog.d/2755-hardcover-audiobook-duration.md.
• N/A — docs/DEPLOYMENT.md and template.env: no deployment or configuration change.
• N/A — CHANGELOG.md is assembled from fragments at release time.

Test plan

• GOTOOLCHAIN=go1.26.6 go test -count=1 ./cmd/... ./internal/... — all packages pass.
• GOTOOLCHAIN=go1.26.6 go vet ./internal/...
• golangci-lint run ./internal/db/... ./internal/bookhydrate/... — 0 issues (local v2.13.2, not the pinned v2.11.4).
• Mutation checks: dropping any of the four identity clauses from either the guarded UPDATE or its fallback SELECT in FillMissingAudiobookDuration fails TestBookRepoFillMissingAudiobookDurationRequiresMatchingIdentity (8/8); reverting the asin upsert expression to COALESCE(NULLIF(...)) fails the legacy whitespace-ASIN hydration case.
• Not run locally — -race; CI's race matrix covers it.
• N/A — Docker/Compose validation: no container behavior changed.

[codecov]

Codecov Report

❌ Patch coverage is 95.61404% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/db/books.go	86.20%	2 Missing and 2 partials ⚠️
internal/bookhydrate/hardcover.go	98.14%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[vavallee]

Thanks for this, and for the four issues behind it. The runtime gap is real and structural: models.Edition had nowhere to put audio_seconds before this PR, so nothing could have carried it. I reproduced #2756 on clean main too, stored ASIN = " ", want "B012345678".

Four things need changing before this can land. Three of them are about the widened audio classifier, which reaches further than the runtime fix needs it to.

1. The chosen edition now supplies language and cover, and the choice got worse.

isLikelyAudioEdition accepts any edition with DurationSeconds > 0 && !IsEbook, and preferredAudioEdition prefers a runtime bearing edition over a higher scoring one when no ASIN is in play. deriveAudiobookMetadataFromEdition then takes Language and ImageURL from that same edition. With two editions and no ASINs:

{Format:"Audiobook", Language:"eng", ImageURL:"good"}          // audioEditionScore 11
{Format:"Unknown", Language:"ger", ImageURL:"bad", Dur:36000}  // audioEditionScore 1
→ stored language="ger" imageURL="bad"

On main the second edition is not audio at all and the book gets eng. Your own TestHydrateHardcoverEditionsPrefersRuntimeWhenEditionsHaveNoASIN codifies the new behaviour as intended. I do not think it should be: take the duration from the runtime bearing edition and everything else from the best scoring one, or restrict the runtime tiebreak to editions of equal audioEditionScore. Worth noting we are merging #2754 on the thesis that edition language is weak evidence, so trading language accuracy for a runtime cuts against that.

2. A print edition carrying a runtime gets promoted to an audiobook.

Same widening, worse consequence. One edition, Format:"Hardcover", ASIN:"B00PRINT01", Language:"por", DurationSeconds:36000, book MediaType:"":

mediaType="audiobook" asin="B00PRINT01" lang="por" ASINPromoted=true

!edition.IsEbook is true for every physical format, so any print edition with audio_seconds flips MediaType and takes over the book's ASIN. Whether Hardcover ever populates audio_seconds on a print edition I could not check offline, so that part is unverified, but #1732 exists because widening MediaType on audio evidence caused bad grabs and this widening has no guard.

3. Half your write guard is not pinned by anything.

I replaced

before.Language == book.Language && before.ImageURL == book.ImageURL {

with true { and the whole ./internal/bookhydrate suite stayed green. With that condition gone, a hydration that derived a language or cover takes the duration only write, persists the duration, silently drops the derived values, and reports MetadataDerived = false. The duration fill itself is well pinned; disabling it fails seven named tests. This one needs a test with a derived language plus a filled duration asserting the full update path and the persisted language.

4. #2756 is fixed for asin only.

internal/db/editions.go still has COALESCE(NULLIF(editions.title, ''), excluded.title) and the same for isbn_13, isbn_10, publisher, format, language, image_url and edition_info. A whitespace only stored ISBN-13 survives a valid incoming one exactly as the ASIN did, and the incoming side trim you added is ASIN only too, so a whitespace only incoming ISBN still gets stored and creates a fresh instance. Your asin CASE with the Unicode whitespace literal is correct but it is now a one off idiom sitting beside eight copies of the broken one. A shared expression across all nine would close it properly.

Minor, no action needed: the in memory workaround at the top of HydrateHardcoverEditions ("UpsertMetadata can retain a whitespace only ASIN") is made unreachable by your own normalisation, except for legacy rows which the new CASE also fixes.

Heads up on rebasing. Main has moved under you. #2760 merged (locked language), then #2773 which generalised it: there is now a Book.CanWrite(field) helper on the model and the !book.IsFieldLocked(...) spelling is gone from these packages. #2774 also landed. deriveAudiobookMetadataFromEdition is where those overlap with yours.

One thing to carry into the rebase, because it is easy to get wrong: your before.Language == book.Language comparison and the lock check are answering different questions. Yours is about write atomicity, the lock is about write authority. With yours alone a locked empty language still gets filled, the comparison then routes to the full update, and the lock is clobbered. Neither guard substitutes for the other and right now nothing tests the seam between them.

Genuinely good catches in here. Happy to look again as soon as you have pushed.

[magrhino]

@vavallee Thank you for the careful review.

The fixes address all four requested changes, plus the locked-language interaction highlighted in the review:

• Edition selection: audio evidence now ranks ahead of runtime. Runtime only breaks ties between equally scored editions, so a weaker edition cannot displace better language and cover metadata.
• Print-format protection: runtime alone no longer promotes a known print format to audio, including when a previously stored Unknown format would otherwise hide the fetched print format.
• Combined metadata writes: regression tests now verify that duration plus language or cover are persisted together. Removing the language/cover guard makes these tests fail.
• Whitespace handling: all nine edition text fields now normalize incoming whitespace and replace whitespace-only stored values, including Unicode whitespace. Meaningful stored values retain their original spacing.
• Language locks: hydration preserves locked languages, including deliberately empty values, with tests covering both duration-only and combined metadata updates.

Further review uncovered additional concurrency and caller-state issues, which are also fixed:

• Concurrent edits could be overwritten. Full hydration writes now update only hydration-owned fields and require the original updated_at value. Edits made during fetching or enrichment win; discarded changes are reloaded and are not reported as successfully applied. Imported EPUB language updates now advance updated_at so they receive the same protection.
• Search could use stale in-memory state. Duration-only writes now refresh the caller’s complete book state, keeping ASIN, duration, monitoring, and status consistent. Refresh also runs after empty, unchanged, or failed edition fetches, while preserving the original fetch error.
• Reloads could lose ingestion context or retain context from the wrong work. Provider series links and other transient ingestion metadata survive reloads only when the book identity is unchanged.
• Fetched editions could attach after a rebind. Edition inserts and updates now atomically verify the parent identity captured before fetching. Late writes for the previous identity are skipped.

The changes are split into three commits for review: text normalization, audio selection/language locks, and concurrency/caller-state protection. Regression tests accompany each part; user documentation and the existing changelog fragment are updated.

Affected package tests, focused API and race tests, vet, and pinned lint passed. Both intermediate commit snapshots also passed hydration and database tests. Full-suite validation still has the unrelated limitations tracked in #2752 and #2778; the full database race run timed out, while focused race checks passed.

The separate policy for editions already persisted before a later rebind remains tracked in #2781.

[vavallee]

All four of my findings are genuinely fixed, and well pinned. I re-ran each of my original probes against this head and they behave correctly now, and I mutation checked the guards rather than trusting the claims:

• Moving the runtime tiebreak back ahead of audioEditionScore fails ...RanksAudioEvidenceBeforeRuntime. You took the option that drops the runtime when the best edition lacks one, which is the right tradeoff and is a cost of the option rather than a defect.
• The print edition case now yields mediaType="ebook" asin="" MetadataDerived=false, pinned twice.
• Replacing the whole before.Language == ... && before.ImageURL == ... condition with true now fails five tests. That was finding 3 and it is properly closed.
• The nine column whitespace work is thorough. I neutered fillMissingText for each column one at a time and every one fails.

That is good work. Three things before it can land, and I think it should be split.

1. The updated_at precondition is not sound, and it fails silently.

UpdateHydratedMetadata compares WHERE id=? AND updated_at=? using timeValueArg, which formats to RFC3339Nano. That is a string comparison against a reformatted time.Time.

books.updated_at is DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, and nothing normalises it. The codebase already knows this: timeParseLayouts in internal/db/books.go carries nine layouts precisely because the column holds several historical shapes, including SQLite's CURRENT_TIMESTAMP output and Go's time.String() output, which its own comment calls "the #914 corruption".

So a row is read through a nine layout parser and then compared as one canonical string. Any row not already stored in RFC3339Nano can never match:

stored "2026-09-01 10:00:00"                     -> updated=false, err=nil   (CURRENT_TIMESTAMP shape)
stored "2026-09-01 10:00:00.123456789 +0000 UTC" -> updated=false, err=nil   (#914 shape)
stored "2026-09-01T10:00:00.123456789Z"          -> updated=true             (control)

No concurrent edit, no error, nothing above debug level. The row is reloaded, every result flag is cleared, and everything hydration derived is discarded. Permanently, on every refresh, for those rows.

It is reachable: the dedup arm at internal/api/authors.go:2503 sets hydrateExistingFromMatchedHardcover with no preceding write, and existing comes straight out of ListByAuthorIncludingExcluded. On main that path writes through Update and succeeds. The other four hydration call sites all Create or Update immediately beforehand, which rewrites updated_at in the canonical shape, so they are safe and that is why this did not show up.

It fails closed, so nothing is corrupted. But it is a silent permanent regression for legacy rows.

Do not compare a reformatted timestamp. Carry the raw column text on the snapshot and compare that, or add a monotonic row version integer. julianday() on both sides would rescue the CURRENT_TIMESTAMP shape but not the #914 one. Whatever you pick, the three cases above want to be the test.

Smaller, same area: an unparseable updated_at leaves b.UpdatedAt zero, which makes this return a hard "invalid book snapshot" error where main silently wrote.

2. FillMissingAudiobookDuration's identity clause is entirely unpinned. Removing AND foreign_id=? AND metadata_provider=? AND media_type=? AND asin=? leaves ./internal/db and ./internal/bookhydrate green, and so does neutering each of the four individually. Only duration_seconds <= 0 is pinned. The "changed ASIN must reload the current identity" step passes before the fix too, because it sets DurationSeconds = 42000 in the same write so the duration clause blocks it anyway. It needs a case that changes only the ASIN.

3. The dead ASIN block is still there, with a comment that is now false. internal/bookhydrate/hardcover.go:138-141 says "UpsertMetadata can retain a whitespace-only ASIN". It cannot, because this PR fixed that. Deleting the block leaves both suites green. I flagged it last round as minor with no action needed; keeping it is fine, keeping the claim is not.

Please split it. The seam is exact. At 116b3de2 the PR closes #2755 and #2756 and fixes all four findings including the finding 3 guard and its test. I checked that commit out and ran the three packages green, and its persist block still calls Books.Update, so #2758 is untouched there.

572ac7be is a different objective: UpdateHydratedMetadata, the precondition, ReloadHydratedBook, the reload semantics, the SetLanguage bump, the interface changes and the rebind atomicity. It closes a separately filed issue and it carries the defect above.

Merge the first three commits as this PR once 2 and 3 are done. Take 572ac7be out as its own PR against #2758, where the timestamp comparison gets the review it needs.

Two more for that second PR when it exists. The unconditional ReloadHydratedBook on the no-change path runs a GetByID, which is bookCTE with two grouped subqueries and three LEFT JOINs, on every hydration that derived nothing, which is the common case, inside the per created book loop that prefetchHardcoverEditions exists to keep cheap and under the per author write lock. If hydration wrote nothing the caller's snapshot is no staler than before it ran, so I would push back on that. And no call site reads Result at all, so the "not reported as successfully applied" property is real but currently carried entirely by the reload rather than by the flags.

Bookkeeping. This closes #2758 and the body still says it is merely tracked, and the Scope box still says "One reviewable objective", which is no longer true. #2375 is untouched, which is fine, but it is the same defect at refreshBookStatus (internal/db/books.go:1049 still ends return r.Update(ctx, b) on a snapshot read at :997), so the second PR is the natural home for it.

On the race timeout: #2752 is merged as 2e282b2c and is in this branch's own base, and neither it nor #2778 explains a race timeout in ./internal/db. CI's race matrix is green on this head, all six shards, db at 19m and non-api at 27m. It looks like a local machine artefact and the test plan should say so rather than citing two unrelated issues.

The changelog fragment is one ninety word sentence covering eight behaviours; it wants one line per user visible change. And the User Guide paragraph about skipping edition writes after a rebind is internal mechanics a reader cannot act on.

[magrhino]

Thanks for going back over this so carefully, and for breaking the guards on purpose rather than taking the claims on trust. That found two gaps I should have caught myself. All three points are addressed and the PR is split along the line you suggested.

1. Split. Done as requested. The concurrency commit is now #2794.

2. Identity guards in FillMissingAudiobookDuration. You were right that the "changed ASIN" step was only passing because of the duration condition. The new TestBookRepoFillMissingAudiobookDurationRequiresMatchingIdentity changes one stored column at a time (foreign_id, metadata_provider, media_type as audiobook→both, asin) and runs each case twice:

stored duration 0     -> pins the guarded UPDATE
stored duration 42000 -> pins the fallback SELECT (must return 0, not another identity's runtime)

I then removed each of the four conditions from the UPDATE and from the SELECT in turn. All 8 changes fail the test.

3. Dead ASIN block. Removed, comment included. The test case that was supposed to exercise it never did: it seeded through UpsertMetadata, which already stores NULL. It now seeds through EditionRepo.Upsert, which writes the ASIN verbatim, so the row really is legacy " \u00a0". With the asin expression reverted to COALESCE(NULLIF(editions.asin, ''), excluded.asin), it fails with stored ASIN="" duration=0.

Bookkeeping. The description now says #2758 lives in #2794 and fixes the Scope box. On the race timeout, you were right: that was my machine, not #2752 or #2778. That line is gone, and the test plan now lists what I actually ran: the full go test ./cmd/... ./internal/... suite passes locally on this head.

[vavallee]

The split was the right call, and the new guard is better than what I asked for. Dropping `updated_at` entirely for a compare and set on the identity columns plus `duration_seconds <= 0` sidesteps the nine timestamp shapes problem instead of fighting it, and writing one column means a concurrent edit to anything else can't be lost.

Checked, merged onto current main: merges clean, builds, vets, and `./internal/db`, `./internal/bookhydrate`, `./internal/metadata/hardcover` and `./internal/models` pass. Dropping `AND asin = ?` fails `TestBookRepoFillMissingAudiobookDurationRequiresMatchingIdentity`. I also made sure the equality guards can't silently miss on a NULL: `asin` and `duration_seconds` are both `NOT NULL` with defaults since `005_audiobook.sql`.