navidrome/cmd/utils.go

Ignoring revisions in .git-blame-ignore-revs. Click here to bypass and see the normal blame view.

74 lines
1.9 KiB
Go
Raw Normal View History

package cmd
import (
"context"
"errors"
"fmt"
feat(cli): add an artwork command group for diagnosing and re-driving artwork (#5957) * feat(artwork): add a resolution chain trace collector * feat(artwork): trace the local priority chain * fix(artwork): record priority candidates the chain never evaluated * refactor(artwork): report never-evaluated candidates as skipped * feat(artwork): trace external agents at the gate seam * feat(artwork): add repository queries to enqueue by current source * feat(artwork): expose a tracing resolver for the CLI * feat(artwork): read a single queue row by item The explain CLI must report whether an item is queued, at what priority and when it retries; the queue repository could only be drained in eligibility batches, which cannot see a row that is still backing off. * feat(cli): add artwork explain Prints why an item has the artwork it has: the stored state, its queue row, the governing config, the resolver's priority-chain walk and the verdict. Offline by default so a diagnostic run cannot add load to an external provider; --live asks the agents for real. Playlists and radios do not walk a priority chain, so they report that instead of an empty chain table. * fix(artwork): trace an external tier that never reaches an agent A configured 'external' token vanished from the chain when no enabled agent provided images for that entity type, and for synthetic artists, leaving the trace unable to say whether the tier was even considered. * fix(cli): never state an artwork outcome the walk did not observe A transient external failure traced as 'error' fell through to 'not resolved', which is the most common state behind a missing-artwork report. It is now indeterminate, and an offline win that a skipped higher-priority external candidate could have taken says so instead of naming a winner the live chain might not pick. * feat(cli): add artwork refresh * feat(cli): add artwork reprocess Bulk re-enqueues artwork by kind and/or by the source an item currently resolves from, previewing the matched count and confirming before queueing. The preview counts with CountBySource (rows matched) and reports separately what EnqueueBySource inserted: its DO NOTHING conflict policy leaves an already-queued row untouched, so the two numbers differ and the output must not claim the skipped rows were re-queued. An unknown --source is rejected against the sources present in item_artwork, rather than silently matching nothing and printing a reassuring 0. * fix(cli): cover the reprocess selection rule and validate sources table-wide The reconciliation that makes --source alone target every kind was only exercised through runReprocess, which no test calls: mutating it to `all := reprocessAll` left the suite green. It is now reprocessSelectsAll, covered for all three selectors. Scoping source validation to the selected kinds made the same well-formed filter valid or invalid depending on which other kinds were selected, and its error read the same for a typo as for a source that simply does not apply to the chosen kind. Validation is now table-wide: a typo still aborts, while a valid-but-inapplicable source falls through to "Nothing matches". Also: the prompt now counts only the kinds that reach an external agent as external cost, and --dry-run on an empty selection reports a dry run. * fix(cli): cover the reprocess --yes guard and preview the external cost Mutating the --yes check to `if true` left the suite green, so the one bypass of the confirmation was unverified. The choice is now reprocessConfirm(yes, in), covered in both directions. The external estimate only reached the operator through the prompt, which --dry-run skips — hiding the number in the one mode that exists to show it before committing. The preview now carries it, and the prompt drops the clause when no lookup will be made. An empty selection says so again under --dry-run. * feat(artwork): add read-only queue and absent counters Both are needed by the artwork status CLI: a queue breakdown by kind and priority, and the absent totals split against the recheck cutoff. * feat(cli): add artwork status Reports the queue, where artwork currently resolves from, absent counts against the 24h recheck window, and the stored config fingerprint versus the current one — the line that turns 'why is my server re-resolving everything?' into one command. fingerprint() and staleAbsentAge are exported so the CLI reports the values backfill itself compares, instead of a second copy of the formula that can silently drift. * fix(cli): lead the artwork status backfill line with the queued backlog By the time anyone runs a diagnostic, backfill has usually already stored the new fingerprint, so 'up to date' was printed while thousands of items churned through external providers. The backlog is the finding; the fingerprint is context. Also echoes the config inputs the fingerprint covers, so a change can be traced to the setting that caused it, and pins the rendered rows: the Absent values, the queue TOTAL and a queue-scoped kind/priority pair were all unasserted, so kindName and priorityName were effectively untested. FingerprintInputs is now the single listing ConfigFingerprint hashes; a pinned hash proves the value did not change. * refactor(artwork): export the trace outcome vocabulary The CLI hardcoded the outcome literals and the "external:" prefix, so renaming a constant's value in core/artwork left cmd compiling and the suite green while `artwork explain` silently degraded its verdict. Renaming a value now fails the golden vocabulary test in core/artwork and the explainResult tests in cmd. * fix(cli): keep the re-enqueue warning when a backfill is already running A stale stored fingerprint with items already queued is the worst state the system can be in: a second full re-enqueue is pending on top of the one running. The line carried the weakest wording of the three, and was untested. * refactor(artwork): drop the unreachable breaker branch from the tracing gate --live wires the tracing gate straight to passthroughGate, so errBreakerOpen can never reach it; the test only passed by injecting a fake gate. * refactor(artwork): delete the never-emitted not-reached outcome Candidates after the winner are lower priority and say nothing about why a source won; the ones that matter sit above it and are already recorded. * refactor(artwork): make the trace nil-safe in one place only add already handles a nil trace, so record's own guard was dead; Steps was the odd one out and would panic where every other method tolerates nil. * refactor(artwork): export the trace types directly ChainTrace and TraceStep were unexported types re-exported through aliases, which existed only so the CLI had a name to refer to them by. The types are public API — Resolver.Steps returns []TraceStep and the CLI constructs a ChainTrace — so name them that way and drop the indirection. Encapsulation is unchanged: add, mu and steps stay unexported, so only this package can write a step. * refactor(cli): simplify parseArtworkKind with slices.Contains Replaces a nested loop and a manual append with slices.Contains and the repo's slice.Map helper. Same behaviour, same error message. * fix(cli): print the absent artwork source under the name --source accepts `artwork explain` rendered the stored empty source as "(absent)", while `artwork reprocess --source` only accepts "absent", so pasting what explain printed straight back into reprocess was rejected as an unknown source. * refactor(artwork): own the kind list and the chain predicate in the package Export RecheckKinds and add WalksPriorityChain so the CLI stops keeping its own copies of both, and unexport externalCandidate, which nothing outside the package consumes. * refactor(cli): drop the artwork command's duplicated state and formatting Reuse artwork.RecheckKinds and artwork.WalksPriorityChain, extract newTabWriter and externalEstimate, fold reprocessSelectsAll into selectedKinds, and derive the queue total and the walks-chain flag instead of carrying them in the report structs. * test(persistence): drop two artwork-queue specs that cannot fail One seeded hash and source together and then asserted the two counts agree, so its setup guaranteed the result; the other repeated the count-does-not- enqueue property already covered by the CountBySource spec. * refactor(artwork): rename Resolver to TracingResolver for clarity * fix(cli): count playlists in the artwork reprocess external estimate The estimate used WalksPriorityChain, which is true only for artist and album, so a playlist-only reprocess reported "External lookups: none" and the confirmation prompt dropped the external-cost warning. Playlists do reach the network: through the m3u ExternalImageURL fetch when EnableM3UExternalAlbumArt is on, and — verified by test — through the generated grid, whose tiles resolve album art via the full album priority chain. Adds artwork.MayFetchExternal, a config-aware predicate for "can this kind's resolver reach the network", and uses it for the estimate. WalksPriorityChain keeps its separate job of deciding whether explain prints a chain block. * fix(cli): estimate artwork reprocess external lookups per agent, not per item The reprocess prompt billed one external lookup per externally-capable item. fetchArtistImage/fetchAlbumImage try every enabled image agent and stop early only on a hit, and resolvePlaylist can fetch the m3u image and then resolve up to four sampled albums for the grid, each walking the album agents again. The number the operator confirmed could understate real provider traffic several fold, in the prompt whose whole job is to stop a provider flood. ExternalLookupsPerItem now multiplies by the visible image-agent count and adds the playlist grid factor. It stays a floor: the CLI never calls Manager.Start(), so the plugin registry is empty and plugin-provided agents are dropped by getEnabledAgentNames. On an install with 5 agents of which 3 are plugins the count is well under the truth, so the wording is now "at least N" rather than "up to N" — a zero visible count still bills one lookup for the same reason. Fixing the plugin visibility is out of scope: Manager.Start() needs a Subsonic router and writes to the DB via syncPlugins, breaking this command group's read-only guarantee. * fix(cli): state the artwork reprocess estimate as an estimate, not a bound Neither bound is true. A ceiling is false because plugin agents are invisible to a CLI that never starts the plugin manager, and a floor is false because a local hit ends the walk before any agent is asked and a hit on the first agent skips the rest. "at least N" traded one wrong claim for another. The line now names its blind spots instead: External lookups: ~340 estimated (plugin agents not counted; local hits may need fewer). The same line is reused in the confirmation prompt, and the zero case still reads "External lookups: none." with the prompt dropping the clause entirely. The count itself is unchanged. * fix(cli): account for every configured agent in artwork explain The Agents: line printed the raw config while the Chain only showed the agents the CLI could construct, with nothing explaining the gap: plugin agents are never registered in a CLI that does not start the plugin manager, and a built-in without credentials returns nil. Three of five agents could vanish, including ones ranked above the one shown. Also treat a live external error before the winning hit like the already-handled would-try case: the resolver serves such a hit provisionally and retries later, so the verdict is indeterminate. The Result line is still not qualified when an unavailable agent might have won; that needs agent ranking, and is left to the follow-up that makes the CLI load plugin agents for real. * fix(cli): do not call an external artwork win indeterminate explainResult qualified the verdict whenever an external OutcomeError appeared before the winning hit. When a later external agent returns an image, fetchArtistImage/fetchAlbumImage discard the earlier error, so extError is false: the worker settles the item and schedules no retry. Telling the operator it may resolve differently on a retry was wrong. The warning is only correct when a lower-priority local source won while an external error was recorded, which is the case that carries extError. * fix(cli): accept --source absent when nothing is currently absent validateSources checks the requested sources against the ones item_artwork actually uses, to catch a typo. The reserved empty source (spelled 'absent' on the CLI) is a valid filter even when it matches nothing, so a scheduled 'artwork reprocess --source absent --yes' stopped working the moment the library finished resolving. Treat it as intrinsically valid and let the existing zero-match path report it. * feat(artwork): explain disc and media file artwork from the CLI `artwork explain` rejected `dc` and `mf` because it validated against RecheckKinds, the list of kinds the backfill revisits. Those are different questions: a kind with no recheck path still has artwork someone can report as wrong. Disc artwork now walks DiscArtPriority under a trace, so explain reports which entry won and why the others lost, including entries that map to no source at all (external is unsupported, a disc with no subtitle, an album folder with no images). Media file artwork traces its single embedded candidate, separating "EnableMediaFileCoverArt is off" from "the track has no embedded art" — stored state cannot tell those apart. Each command now validates against the kinds it can actually serve: explain takes all six, refresh takes artwork.RefreshableKinds (which nativeapi now shares instead of keeping its own copy), reprocess still takes RecheckKinds. Disc artwork stays out of refresh: the worker cannot resolve it, so the queue row would be rejected on every drain. WalksPriorityChain becomes Explainable, and ResolveArtist/ResolveAlbum collapse into Resolve(kind, id). * refactor(artwork): one disc-artwork walk for serving and explain resolveDisc duplicated the loop selectImageReader already ran: try each source in priority order, take the first that yields an image. The serving path and the CLI diverged on two details as a result — only selectImageReader checked ctx between candidates and logged each attempt. Both now call discArtworkReader.selectImage, which takes the chainState the CLI already uses for the other kinds. The serving path passes an untraced one, whose nil trace makes recording a no-op. selectImageReader had no other caller and is gone. The disc tests move from fromDiscArtPriority to discCandidates, so they assert the skip reason for an entry that maps to no source rather than that it silently vanished, and cancellation mid-walk is now covered. * fix(artwork): reject a nil reader in the resize cache instead of panicking resizedItem.Reader closes what open() hands back, so an open() that reports "no image" as (nil, nil) rather than an error takes the request down with a nil-pointer panic. Every caller returns an error today, and no test covered it: the resolution e2e harness stubs the resize reader out entirely, so no e2e path reaches this code at all. Guard it and cover Reader directly. * refactor(artwork): move the keeps-state fact into core, drop a redundant guard keepsArtworkState lived in package cmd and re-derived by hand what RefreshableKinds already encodes: the same five-of-six kinds. It is now artwork.KeepsState, beside the list, with a test pinning the two together — nothing else stopped them drifting, and a drift would have explain report stored state for a kind that keeps none. serveDisc's closure also hand-rolled a nil-reader error that both consumers of open() now produce themselves: serveSource for a full-size request, resizedItem.Reader for a resized one. * fix(artwork): route disc candidates through the shared resolvers openCandidate ran its own source loop and threw the error away, so a disc track that exists but cannot be parsed traced as "miss" — indistinguishable from a track with no embedded art. fromTag and fromFFmpegTag already report that case as errSourceUnreadable; only this loop was discarding it. Telling those two apart is what the trace is for. Candidates now carry a resolve func instead of raw sources: embedded goes to resolveEmbedded, and the folder-backed entries to resolveFolderSource, extracted from resolveFolderFile so both callers classify an unopenable file the same way. openCandidate and its absolute-path special case go away with it. Disc's own fromExternalFile and fromDiscSubtitle still swallow open errors, so folder candidates cannot report unreadable yet; that is a change to their error contracts. * fix(artwork): report an unreadable local candidate as indeterminate processor.acquire treats resolution.localError exactly as it treats extError: a fault is not a definitive "no image", so it retries instead of settling absent. explainResult qualified only the external case, so a chain that ended on an unreadable local candidate printed "not resolved" — the one verdict that says the walk was conclusive. The qualification belongs only to the unresolved branch. chainState.try stamps extErr onto a hit and deliberately drops localErr, so an unreadable step followed by a hit is settled as found and must not carry a warning; a test pins that. Found by Codex on 5f65d7cfa.
2026-08-14 21:07:56 -04:00
"io"
feat(cli): add 'doctor' and 'search rebuild' commands to recover from FTS5 corruption (#6069) * feat(db): add repair command to rebuild a corrupted FTS5 search index A corrupted media_file_fts index made every scan fail with 'database disk image is malformed', and sqlite3's built-in 'rebuild' command cannot repair contentless FTS5 tables, leaving users to hand-drop tables and triggers. Add 'navidrome db repair': it runs PRAGMA integrity_check, and when the reported corruption is confined to the FTS5 search tables, drops and recreates the three tables and their nine triggers and repopulates them from the base tables (which hold all the data, so nothing is lost). The result is verified with the FTS5-native 'integrity-check' command, which reads only the rebuilt indexes instead of re-scanning the whole database (on a 761MB production copy: ~9s full check, ~1s rebuild, sub-second verify). A --rebuild flag forces the rebuild even when the check passes, for silently desynced indexes. The rebuild refuses to run while migrations are pending, and a schema-comparison test guards the duplicated DDL against drifting from the migration. The DbPath existence check and the YES confirmation prompt, previously copy-pasted across the backup commands, are extracted into shared cmd helpers used by both backup and repair. Part of #6067 * fix(db): type the FTS migration version as int64 for 32-bit builds The untyped constant defaults to int, which overflows on arm/v7 and 386. * feat(db): split repair into 'db doctor' and 'search rebuild' commands A single 'db repair' command promised more than it delivered: the only thing it could actually repair was the search index, and its diagnosis and its fix were welded together, so a forced rebuild paid the full integrity check twice. Split it: 'navidrome db doctor' is strictly read-only, runs both PRAGMA integrity_check and PRAGMA foreign_key_check, and routes the user (to 'search rebuild' when corruption is FTS-only, to backup/.recover otherwise). 'navidrome search rebuild' just rebuilds and verifies the FTS index, which takes ~2s on a prod-size library instead of ~19s. * refactor(cmd): extract a testable doctor function and bound foreign key output Extract the doctor routing (check, classify, advise) into a function that takes an io.Writer, so the advice paths are unit-tested and the process exit happens in the cobra wrapper after the DB is closed (os.Exit was skipping the deferred close, leaving WAL/SHM files behind on the unhealthy paths). Aggregate foreign_key_check by (table, parent): the raw pragma emits one row per orphan, which is unbounded output on a large corrupted library. Also make confirmYES take an io.Reader, drop the unused return from the renamed requireExistingDB, share the FTS table list with the tests, and stop the schema-guard specs from paying for a seeded database they never use. * docs(cmd): promise 'never alters your data' instead of 'never modifies the database' Closing the doctor's connection can checkpoint a stale WAL into the main file (as any SQLite tool does), so the byte-level claim was too strong. The checks themselves are read-only and no logical content ever changes. * fix(cmd): make 'db doctor' advice honest when checks are inconclusive PRAGMA integrity_check stops at 100 errors and emits no marker row, so a saturated result was being read as the whole picture. IntegrityCheck now sets the limit itself and reports saturation as a truncated list, and doctor no longer claims corruption is limited to the search index in that case. Foreign key violations now print a next step instead of only flipping the exit code: migrations run with foreign_keys off, so orphan rows are a realistic leftover on a database that is not corrupt. Also corrects the 'search rebuild' help, which promised that 'db doctor' detects when a rebuild is needed -- integrity_check cannot see an index that is merely out of sync; gives the never-migrated case its intended message instead of a raw 'no such table: goose_db_version'; and extracts rebuildSearchIndex so the database is closed before log.Fatal exits. * refactor(cmd): promote 'db doctor' to a top-level 'doctor' command The 'db' group held a single subcommand, and the checks planned for it reach past the database: config, music folder permissions, external tools. None of those belong under 'db'. Promoting it also evens out the shape of the pair. The command that finds the problem is now top-level alongside 'search rebuild', the command that fixes it, matching the 'brew doctor' convention users already expect. 'db doctor' has never been released, so no alias or deprecation is needed. * refactor(db): tighten the doctor and search rebuild internals Follow-up cleanup with no behaviour change except where noted. integrity_check now asks the pragma for one row beyond the reported limit and treats that extra row as the proof it truncated, instead of inferring truncation from a saturated count. That distinguishes a list of exactly 100 issues from one that was cut short -- the old test could not, and 100 was SQLite's own default, so passing it was a no-op. ForeignKeyCheck returns []FKViolation instead of pre-formatted English, moving the prose to the layer that already owns the CLI vocabulary. The goose table probe shared with isSchemaEmpty becomes hasGooseTable, so 'has this database ever been migrated' has one spelling. Also folds ftsMigrationApplied into requireFTSMigration, lifts printFindings out of a closure that captured nothing, names the FTS trigger suffixes once, and corrects the ftsSchemaDDL comment: the drift test compares against the full migration chain, not the single frozen migration it claimed. * fix(db): verify the rebuilt search index before committing it RebuildFTS committed its transaction and only then ran the FTS5 integrity check, from the caller. A rebuild that produced a bad index was therefore already persisted by the time anyone noticed, leaving the user worse off than before they ran the command. The check now runs inside the transaction, so a rebuild that does not verify rolls back and leaves the original index in place. VerifyFTS keeps its *sql.DB signature for callers outside a transaction; the shared body takes the small execer interface that both *sql.DB and *sql.Tx satisfy. Adds a spec for the rollback: it removes a column the repopulating SELECT reads, so the transaction fails after the drops, and asserts the old index still answers queries. * refactor(cmd): drop the unused io.Reader parameter from confirmYES The reader was added as a test seam that no test ever used: all three callers pass os.Stdin. Back to fmt.Scanln, which drops the parameter and the now-unused os import from backup.go and search.go. * fix(cmd): stop promising a scan clears every foreign key violation doctor told the user to run 'navidrome scan -f' for any foreign key violation. SQLStore.GC only purges albums, artists, folders, annotations, bookmarks, tags and playlist tracks, so orphans elsewhere survive it and the next doctor run still reports them. player.user_id references user(id) and no scan phase touches that table at all. The advice now says a scan clears some of them and the rest have to be removed by hand, which keeps the next step the earlier round asked for without claiming a cleanup that does not happen. * docs(db): trim over-long comments on the doctor and rebuild paths Six comments ran past two lines or repeated something already stated nearby. The RebuildFTS doc claimed the rebuild rolls back on a column mismatch, which the new 'verifies before committing' sentence already implies, and a spec comment restated that same rationale a second time. * docs: drop em dashes from the comments added in this branch
2026-09-11 22:26:28 -04:00
"os"
"strings"
feat(cli): add an artwork command group for diagnosing and re-driving artwork (#5957) * feat(artwork): add a resolution chain trace collector * feat(artwork): trace the local priority chain * fix(artwork): record priority candidates the chain never evaluated * refactor(artwork): report never-evaluated candidates as skipped * feat(artwork): trace external agents at the gate seam * feat(artwork): add repository queries to enqueue by current source * feat(artwork): expose a tracing resolver for the CLI * feat(artwork): read a single queue row by item The explain CLI must report whether an item is queued, at what priority and when it retries; the queue repository could only be drained in eligibility batches, which cannot see a row that is still backing off. * feat(cli): add artwork explain Prints why an item has the artwork it has: the stored state, its queue row, the governing config, the resolver's priority-chain walk and the verdict. Offline by default so a diagnostic run cannot add load to an external provider; --live asks the agents for real. Playlists and radios do not walk a priority chain, so they report that instead of an empty chain table. * fix(artwork): trace an external tier that never reaches an agent A configured 'external' token vanished from the chain when no enabled agent provided images for that entity type, and for synthetic artists, leaving the trace unable to say whether the tier was even considered. * fix(cli): never state an artwork outcome the walk did not observe A transient external failure traced as 'error' fell through to 'not resolved', which is the most common state behind a missing-artwork report. It is now indeterminate, and an offline win that a skipped higher-priority external candidate could have taken says so instead of naming a winner the live chain might not pick. * feat(cli): add artwork refresh * feat(cli): add artwork reprocess Bulk re-enqueues artwork by kind and/or by the source an item currently resolves from, previewing the matched count and confirming before queueing. The preview counts with CountBySource (rows matched) and reports separately what EnqueueBySource inserted: its DO NOTHING conflict policy leaves an already-queued row untouched, so the two numbers differ and the output must not claim the skipped rows were re-queued. An unknown --source is rejected against the sources present in item_artwork, rather than silently matching nothing and printing a reassuring 0. * fix(cli): cover the reprocess selection rule and validate sources table-wide The reconciliation that makes --source alone target every kind was only exercised through runReprocess, which no test calls: mutating it to `all := reprocessAll` left the suite green. It is now reprocessSelectsAll, covered for all three selectors. Scoping source validation to the selected kinds made the same well-formed filter valid or invalid depending on which other kinds were selected, and its error read the same for a typo as for a source that simply does not apply to the chosen kind. Validation is now table-wide: a typo still aborts, while a valid-but-inapplicable source falls through to "Nothing matches". Also: the prompt now counts only the kinds that reach an external agent as external cost, and --dry-run on an empty selection reports a dry run. * fix(cli): cover the reprocess --yes guard and preview the external cost Mutating the --yes check to `if true` left the suite green, so the one bypass of the confirmation was unverified. The choice is now reprocessConfirm(yes, in), covered in both directions. The external estimate only reached the operator through the prompt, which --dry-run skips — hiding the number in the one mode that exists to show it before committing. The preview now carries it, and the prompt drops the clause when no lookup will be made. An empty selection says so again under --dry-run. * feat(artwork): add read-only queue and absent counters Both are needed by the artwork status CLI: a queue breakdown by kind and priority, and the absent totals split against the recheck cutoff. * feat(cli): add artwork status Reports the queue, where artwork currently resolves from, absent counts against the 24h recheck window, and the stored config fingerprint versus the current one — the line that turns 'why is my server re-resolving everything?' into one command. fingerprint() and staleAbsentAge are exported so the CLI reports the values backfill itself compares, instead of a second copy of the formula that can silently drift. * fix(cli): lead the artwork status backfill line with the queued backlog By the time anyone runs a diagnostic, backfill has usually already stored the new fingerprint, so 'up to date' was printed while thousands of items churned through external providers. The backlog is the finding; the fingerprint is context. Also echoes the config inputs the fingerprint covers, so a change can be traced to the setting that caused it, and pins the rendered rows: the Absent values, the queue TOTAL and a queue-scoped kind/priority pair were all unasserted, so kindName and priorityName were effectively untested. FingerprintInputs is now the single listing ConfigFingerprint hashes; a pinned hash proves the value did not change. * refactor(artwork): export the trace outcome vocabulary The CLI hardcoded the outcome literals and the "external:" prefix, so renaming a constant's value in core/artwork left cmd compiling and the suite green while `artwork explain` silently degraded its verdict. Renaming a value now fails the golden vocabulary test in core/artwork and the explainResult tests in cmd. * fix(cli): keep the re-enqueue warning when a backfill is already running A stale stored fingerprint with items already queued is the worst state the system can be in: a second full re-enqueue is pending on top of the one running. The line carried the weakest wording of the three, and was untested. * refactor(artwork): drop the unreachable breaker branch from the tracing gate --live wires the tracing gate straight to passthroughGate, so errBreakerOpen can never reach it; the test only passed by injecting a fake gate. * refactor(artwork): delete the never-emitted not-reached outcome Candidates after the winner are lower priority and say nothing about why a source won; the ones that matter sit above it and are already recorded. * refactor(artwork): make the trace nil-safe in one place only add already handles a nil trace, so record's own guard was dead; Steps was the odd one out and would panic where every other method tolerates nil. * refactor(artwork): export the trace types directly ChainTrace and TraceStep were unexported types re-exported through aliases, which existed only so the CLI had a name to refer to them by. The types are public API — Resolver.Steps returns []TraceStep and the CLI constructs a ChainTrace — so name them that way and drop the indirection. Encapsulation is unchanged: add, mu and steps stay unexported, so only this package can write a step. * refactor(cli): simplify parseArtworkKind with slices.Contains Replaces a nested loop and a manual append with slices.Contains and the repo's slice.Map helper. Same behaviour, same error message. * fix(cli): print the absent artwork source under the name --source accepts `artwork explain` rendered the stored empty source as "(absent)", while `artwork reprocess --source` only accepts "absent", so pasting what explain printed straight back into reprocess was rejected as an unknown source. * refactor(artwork): own the kind list and the chain predicate in the package Export RecheckKinds and add WalksPriorityChain so the CLI stops keeping its own copies of both, and unexport externalCandidate, which nothing outside the package consumes. * refactor(cli): drop the artwork command's duplicated state and formatting Reuse artwork.RecheckKinds and artwork.WalksPriorityChain, extract newTabWriter and externalEstimate, fold reprocessSelectsAll into selectedKinds, and derive the queue total and the walks-chain flag instead of carrying them in the report structs. * test(persistence): drop two artwork-queue specs that cannot fail One seeded hash and source together and then asserted the two counts agree, so its setup guaranteed the result; the other repeated the count-does-not- enqueue property already covered by the CountBySource spec. * refactor(artwork): rename Resolver to TracingResolver for clarity * fix(cli): count playlists in the artwork reprocess external estimate The estimate used WalksPriorityChain, which is true only for artist and album, so a playlist-only reprocess reported "External lookups: none" and the confirmation prompt dropped the external-cost warning. Playlists do reach the network: through the m3u ExternalImageURL fetch when EnableM3UExternalAlbumArt is on, and — verified by test — through the generated grid, whose tiles resolve album art via the full album priority chain. Adds artwork.MayFetchExternal, a config-aware predicate for "can this kind's resolver reach the network", and uses it for the estimate. WalksPriorityChain keeps its separate job of deciding whether explain prints a chain block. * fix(cli): estimate artwork reprocess external lookups per agent, not per item The reprocess prompt billed one external lookup per externally-capable item. fetchArtistImage/fetchAlbumImage try every enabled image agent and stop early only on a hit, and resolvePlaylist can fetch the m3u image and then resolve up to four sampled albums for the grid, each walking the album agents again. The number the operator confirmed could understate real provider traffic several fold, in the prompt whose whole job is to stop a provider flood. ExternalLookupsPerItem now multiplies by the visible image-agent count and adds the playlist grid factor. It stays a floor: the CLI never calls Manager.Start(), so the plugin registry is empty and plugin-provided agents are dropped by getEnabledAgentNames. On an install with 5 agents of which 3 are plugins the count is well under the truth, so the wording is now "at least N" rather than "up to N" — a zero visible count still bills one lookup for the same reason. Fixing the plugin visibility is out of scope: Manager.Start() needs a Subsonic router and writes to the DB via syncPlugins, breaking this command group's read-only guarantee. * fix(cli): state the artwork reprocess estimate as an estimate, not a bound Neither bound is true. A ceiling is false because plugin agents are invisible to a CLI that never starts the plugin manager, and a floor is false because a local hit ends the walk before any agent is asked and a hit on the first agent skips the rest. "at least N" traded one wrong claim for another. The line now names its blind spots instead: External lookups: ~340 estimated (plugin agents not counted; local hits may need fewer). The same line is reused in the confirmation prompt, and the zero case still reads "External lookups: none." with the prompt dropping the clause entirely. The count itself is unchanged. * fix(cli): account for every configured agent in artwork explain The Agents: line printed the raw config while the Chain only showed the agents the CLI could construct, with nothing explaining the gap: plugin agents are never registered in a CLI that does not start the plugin manager, and a built-in without credentials returns nil. Three of five agents could vanish, including ones ranked above the one shown. Also treat a live external error before the winning hit like the already-handled would-try case: the resolver serves such a hit provisionally and retries later, so the verdict is indeterminate. The Result line is still not qualified when an unavailable agent might have won; that needs agent ranking, and is left to the follow-up that makes the CLI load plugin agents for real. * fix(cli): do not call an external artwork win indeterminate explainResult qualified the verdict whenever an external OutcomeError appeared before the winning hit. When a later external agent returns an image, fetchArtistImage/fetchAlbumImage discard the earlier error, so extError is false: the worker settles the item and schedules no retry. Telling the operator it may resolve differently on a retry was wrong. The warning is only correct when a lower-priority local source won while an external error was recorded, which is the case that carries extError. * fix(cli): accept --source absent when nothing is currently absent validateSources checks the requested sources against the ones item_artwork actually uses, to catch a typo. The reserved empty source (spelled 'absent' on the CLI) is a valid filter even when it matches nothing, so a scheduled 'artwork reprocess --source absent --yes' stopped working the moment the library finished resolving. Treat it as intrinsically valid and let the existing zero-match path report it. * feat(artwork): explain disc and media file artwork from the CLI `artwork explain` rejected `dc` and `mf` because it validated against RecheckKinds, the list of kinds the backfill revisits. Those are different questions: a kind with no recheck path still has artwork someone can report as wrong. Disc artwork now walks DiscArtPriority under a trace, so explain reports which entry won and why the others lost, including entries that map to no source at all (external is unsupported, a disc with no subtitle, an album folder with no images). Media file artwork traces its single embedded candidate, separating "EnableMediaFileCoverArt is off" from "the track has no embedded art" — stored state cannot tell those apart. Each command now validates against the kinds it can actually serve: explain takes all six, refresh takes artwork.RefreshableKinds (which nativeapi now shares instead of keeping its own copy), reprocess still takes RecheckKinds. Disc artwork stays out of refresh: the worker cannot resolve it, so the queue row would be rejected on every drain. WalksPriorityChain becomes Explainable, and ResolveArtist/ResolveAlbum collapse into Resolve(kind, id). * refactor(artwork): one disc-artwork walk for serving and explain resolveDisc duplicated the loop selectImageReader already ran: try each source in priority order, take the first that yields an image. The serving path and the CLI diverged on two details as a result — only selectImageReader checked ctx between candidates and logged each attempt. Both now call discArtworkReader.selectImage, which takes the chainState the CLI already uses for the other kinds. The serving path passes an untraced one, whose nil trace makes recording a no-op. selectImageReader had no other caller and is gone. The disc tests move from fromDiscArtPriority to discCandidates, so they assert the skip reason for an entry that maps to no source rather than that it silently vanished, and cancellation mid-walk is now covered. * fix(artwork): reject a nil reader in the resize cache instead of panicking resizedItem.Reader closes what open() hands back, so an open() that reports "no image" as (nil, nil) rather than an error takes the request down with a nil-pointer panic. Every caller returns an error today, and no test covered it: the resolution e2e harness stubs the resize reader out entirely, so no e2e path reaches this code at all. Guard it and cover Reader directly. * refactor(artwork): move the keeps-state fact into core, drop a redundant guard keepsArtworkState lived in package cmd and re-derived by hand what RefreshableKinds already encodes: the same five-of-six kinds. It is now artwork.KeepsState, beside the list, with a test pinning the two together — nothing else stopped them drifting, and a drift would have explain report stored state for a kind that keeps none. serveDisc's closure also hand-rolled a nil-reader error that both consumers of open() now produce themselves: serveSource for a full-size request, resizedItem.Reader for a resized one. * fix(artwork): route disc candidates through the shared resolvers openCandidate ran its own source loop and threw the error away, so a disc track that exists but cannot be parsed traced as "miss" — indistinguishable from a track with no embedded art. fromTag and fromFFmpegTag already report that case as errSourceUnreadable; only this loop was discarding it. Telling those two apart is what the trace is for. Candidates now carry a resolve func instead of raw sources: embedded goes to resolveEmbedded, and the folder-backed entries to resolveFolderSource, extracted from resolveFolderFile so both callers classify an unopenable file the same way. openCandidate and its absolute-path special case go away with it. Disc's own fromExternalFile and fromDiscSubtitle still swallow open errors, so folder candidates cannot report unreadable yet; that is a change to their error contracts. * fix(artwork): report an unreadable local candidate as indeterminate processor.acquire treats resolution.localError exactly as it treats extError: a fault is not a definitive "no image", so it retries instead of settling absent. explainResult qualified only the external case, so a chain that ended on an unreadable local candidate printed "not resolved" — the one verdict that says the walk was conclusive. The qualification belongs only to the unresolved branch. chainState.try stamps extErr onto a hit and deliberately drops localErr, so an unreadable step followed by a hit is settled as found and must not carry a warning; a test pins that. Found by Codex on 5f65d7cfa.
2026-08-14 21:07:56 -04:00
"text/tabwriter"
feat(cli): add 'doctor' and 'search rebuild' commands to recover from FTS5 corruption (#6069) * feat(db): add repair command to rebuild a corrupted FTS5 search index A corrupted media_file_fts index made every scan fail with 'database disk image is malformed', and sqlite3's built-in 'rebuild' command cannot repair contentless FTS5 tables, leaving users to hand-drop tables and triggers. Add 'navidrome db repair': it runs PRAGMA integrity_check, and when the reported corruption is confined to the FTS5 search tables, drops and recreates the three tables and their nine triggers and repopulates them from the base tables (which hold all the data, so nothing is lost). The result is verified with the FTS5-native 'integrity-check' command, which reads only the rebuilt indexes instead of re-scanning the whole database (on a 761MB production copy: ~9s full check, ~1s rebuild, sub-second verify). A --rebuild flag forces the rebuild even when the check passes, for silently desynced indexes. The rebuild refuses to run while migrations are pending, and a schema-comparison test guards the duplicated DDL against drifting from the migration. The DbPath existence check and the YES confirmation prompt, previously copy-pasted across the backup commands, are extracted into shared cmd helpers used by both backup and repair. Part of #6067 * fix(db): type the FTS migration version as int64 for 32-bit builds The untyped constant defaults to int, which overflows on arm/v7 and 386. * feat(db): split repair into 'db doctor' and 'search rebuild' commands A single 'db repair' command promised more than it delivered: the only thing it could actually repair was the search index, and its diagnosis and its fix were welded together, so a forced rebuild paid the full integrity check twice. Split it: 'navidrome db doctor' is strictly read-only, runs both PRAGMA integrity_check and PRAGMA foreign_key_check, and routes the user (to 'search rebuild' when corruption is FTS-only, to backup/.recover otherwise). 'navidrome search rebuild' just rebuilds and verifies the FTS index, which takes ~2s on a prod-size library instead of ~19s. * refactor(cmd): extract a testable doctor function and bound foreign key output Extract the doctor routing (check, classify, advise) into a function that takes an io.Writer, so the advice paths are unit-tested and the process exit happens in the cobra wrapper after the DB is closed (os.Exit was skipping the deferred close, leaving WAL/SHM files behind on the unhealthy paths). Aggregate foreign_key_check by (table, parent): the raw pragma emits one row per orphan, which is unbounded output on a large corrupted library. Also make confirmYES take an io.Reader, drop the unused return from the renamed requireExistingDB, share the FTS table list with the tests, and stop the schema-guard specs from paying for a seeded database they never use. * docs(cmd): promise 'never alters your data' instead of 'never modifies the database' Closing the doctor's connection can checkpoint a stale WAL into the main file (as any SQLite tool does), so the byte-level claim was too strong. The checks themselves are read-only and no logical content ever changes. * fix(cmd): make 'db doctor' advice honest when checks are inconclusive PRAGMA integrity_check stops at 100 errors and emits no marker row, so a saturated result was being read as the whole picture. IntegrityCheck now sets the limit itself and reports saturation as a truncated list, and doctor no longer claims corruption is limited to the search index in that case. Foreign key violations now print a next step instead of only flipping the exit code: migrations run with foreign_keys off, so orphan rows are a realistic leftover on a database that is not corrupt. Also corrects the 'search rebuild' help, which promised that 'db doctor' detects when a rebuild is needed -- integrity_check cannot see an index that is merely out of sync; gives the never-migrated case its intended message instead of a raw 'no such table: goose_db_version'; and extracts rebuildSearchIndex so the database is closed before log.Fatal exits. * refactor(cmd): promote 'db doctor' to a top-level 'doctor' command The 'db' group held a single subcommand, and the checks planned for it reach past the database: config, music folder permissions, external tools. None of those belong under 'db'. Promoting it also evens out the shape of the pair. The command that finds the problem is now top-level alongside 'search rebuild', the command that fixes it, matching the 'brew doctor' convention users already expect. 'db doctor' has never been released, so no alias or deprecation is needed. * refactor(db): tighten the doctor and search rebuild internals Follow-up cleanup with no behaviour change except where noted. integrity_check now asks the pragma for one row beyond the reported limit and treats that extra row as the proof it truncated, instead of inferring truncation from a saturated count. That distinguishes a list of exactly 100 issues from one that was cut short -- the old test could not, and 100 was SQLite's own default, so passing it was a no-op. ForeignKeyCheck returns []FKViolation instead of pre-formatted English, moving the prose to the layer that already owns the CLI vocabulary. The goose table probe shared with isSchemaEmpty becomes hasGooseTable, so 'has this database ever been migrated' has one spelling. Also folds ftsMigrationApplied into requireFTSMigration, lifts printFindings out of a closure that captured nothing, names the FTS trigger suffixes once, and corrects the ftsSchemaDDL comment: the drift test compares against the full migration chain, not the single frozen migration it claimed. * fix(db): verify the rebuilt search index before committing it RebuildFTS committed its transaction and only then ran the FTS5 integrity check, from the caller. A rebuild that produced a bad index was therefore already persisted by the time anyone noticed, leaving the user worse off than before they ran the command. The check now runs inside the transaction, so a rebuild that does not verify rolls back and leaves the original index in place. VerifyFTS keeps its *sql.DB signature for callers outside a transaction; the shared body takes the small execer interface that both *sql.DB and *sql.Tx satisfy. Adds a spec for the rollback: it removes a column the repopulating SELECT reads, so the transaction fails after the drops, and asserts the old index still answers queries. * refactor(cmd): drop the unused io.Reader parameter from confirmYES The reader was added as a test seam that no test ever used: all three callers pass os.Stdin. Back to fmt.Scanln, which drops the parameter and the now-unused os import from backup.go and search.go. * fix(cmd): stop promising a scan clears every foreign key violation doctor told the user to run 'navidrome scan -f' for any foreign key violation. SQLStore.GC only purges albums, artists, folders, annotations, bookmarks, tags and playlist tracks, so orphans elsewhere survive it and the next doctor run still reports them. player.user_id references user(id) and no scan phase touches that table at all. The advice now says a scan clears some of them and the rest have to be removed by hand, which keeps the next step the earlier round asked for without claiming a cleanup that does not happen. * docs(db): trim over-long comments on the doctor and rebuild paths Six comments ran past two lines or repeated something already stated nearby. The RebuildFTS doc claimed the rebuild rolls back on a column mismatch, which the new 'verifies before committing' sentence already implies, and a spec comment restated that same rationale a second time. * docs: drop em dashes from the comments added in this branch
2026-09-11 22:26:28 -04:00
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/core/auth"
"github.com/navidrome/navidrome/db"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
"github.com/navidrome/navidrome/persistence"
)
feat(scanner): per-library PID configuration (#6252) * feat(model): add per-library PID config columns * refactor(metadata): pass PID config to ToMediaFile and add spec validation * feat(scanner): rescan only libraries whose PID config changed * feat(server): validate library PID config and rescan on change * feat(ui): edit per-library PID config * fix(ui): label the PID mode selects * fix: tighten per-library PID rescan edge cases An interrupted PID rescan no longer upgrades every library to a full scan, a save that loses the race for the scanner logs at debug, the confirm dialog only shows when the effective PID spec changes, and it now gets translation keys. * refactor(metadata): pass the library to ToMediaFile ToMediaFile and core.Inspect took the library ID and its PID config as separate arguments, so a caller could mix values from two libraries. They now take the model.Library and resolve the effective PID config from it. * chore: tidy per-library PID comments, PropTypes and migration Trim comments that restated the code, add PropTypes to the new UI components, and recreate the migration with make migration-sql. * fix(ui): show the PID spec help under its input * feat(cmd): make inspect use the file's library PID config inspect always used the global PID config, so it showed different IDs than the scanner for files in a library with an override. It now finds the file's library in the DB and uses its effective config, falling back to the global config when there is no DB or the file is outside every library. It never creates a DB. The library path matcher moves from core/playlists to model so both can use it. * refactor: simplify per-library PID code Share the DB-file check between CLI commands, move ErrAlreadyScanning to model so core no longer imports scanner, read the libraries once for insights, and let ValidatePIDSpec accept an empty spec and look tags up directly. In the scanner, use FullScanInProgress instead of a second flag, and skip recomputing album IDs when the album spec did not change. In the UI, share the PID inputs between Create and Edit, and use docsUrl. * feat(ui): add section titles to Library Create and pre-fill Custom PID specs Custom now starts from the global spec, so admins edit a working spec instead of typing one from scratch. * fix(inspect): map files with the library-relative path the scanner uses Inspect gave metadata the file's directory as typed, so folder-based PIDs never matched the DB. It now uses the path relative to the library root, through the scanner's helper, which moves to model. * fix(scanner): say when a PID rescan only covers target folders * fix: reject tag aliases in album PID specs and match root libraries Tags are stored under canonical names, so an alias in a spec always reads as empty. In an album spec that gives every album the same ID, so album specs now require the tag name. Track specs keep accepting aliases, since the default one uses them. LibraryMatcher now matches paths under a library at the filesystem root. * refactor(model): move the tag alias lookup to tag_mappings.go * test: run the library matcher and inspect tests on Windows Build test paths with filepath instead of Unix literals, so they use the OS separator like filepath.Abs output, and drop the Windows skips. * feat(ui): add pt-BR translations for per-library PID settings
2026-10-02 05:05:32 -04:00
// existingDBFile returns the database file (DbPath minus DSN params), and whether it exists.
func existingDBFile() (string, bool) {
feat(cli): add 'doctor' and 'search rebuild' commands to recover from FTS5 corruption (#6069) * feat(db): add repair command to rebuild a corrupted FTS5 search index A corrupted media_file_fts index made every scan fail with 'database disk image is malformed', and sqlite3's built-in 'rebuild' command cannot repair contentless FTS5 tables, leaving users to hand-drop tables and triggers. Add 'navidrome db repair': it runs PRAGMA integrity_check, and when the reported corruption is confined to the FTS5 search tables, drops and recreates the three tables and their nine triggers and repopulates them from the base tables (which hold all the data, so nothing is lost). The result is verified with the FTS5-native 'integrity-check' command, which reads only the rebuilt indexes instead of re-scanning the whole database (on a 761MB production copy: ~9s full check, ~1s rebuild, sub-second verify). A --rebuild flag forces the rebuild even when the check passes, for silently desynced indexes. The rebuild refuses to run while migrations are pending, and a schema-comparison test guards the duplicated DDL against drifting from the migration. The DbPath existence check and the YES confirmation prompt, previously copy-pasted across the backup commands, are extracted into shared cmd helpers used by both backup and repair. Part of #6067 * fix(db): type the FTS migration version as int64 for 32-bit builds The untyped constant defaults to int, which overflows on arm/v7 and 386. * feat(db): split repair into 'db doctor' and 'search rebuild' commands A single 'db repair' command promised more than it delivered: the only thing it could actually repair was the search index, and its diagnosis and its fix were welded together, so a forced rebuild paid the full integrity check twice. Split it: 'navidrome db doctor' is strictly read-only, runs both PRAGMA integrity_check and PRAGMA foreign_key_check, and routes the user (to 'search rebuild' when corruption is FTS-only, to backup/.recover otherwise). 'navidrome search rebuild' just rebuilds and verifies the FTS index, which takes ~2s on a prod-size library instead of ~19s. * refactor(cmd): extract a testable doctor function and bound foreign key output Extract the doctor routing (check, classify, advise) into a function that takes an io.Writer, so the advice paths are unit-tested and the process exit happens in the cobra wrapper after the DB is closed (os.Exit was skipping the deferred close, leaving WAL/SHM files behind on the unhealthy paths). Aggregate foreign_key_check by (table, parent): the raw pragma emits one row per orphan, which is unbounded output on a large corrupted library. Also make confirmYES take an io.Reader, drop the unused return from the renamed requireExistingDB, share the FTS table list with the tests, and stop the schema-guard specs from paying for a seeded database they never use. * docs(cmd): promise 'never alters your data' instead of 'never modifies the database' Closing the doctor's connection can checkpoint a stale WAL into the main file (as any SQLite tool does), so the byte-level claim was too strong. The checks themselves are read-only and no logical content ever changes. * fix(cmd): make 'db doctor' advice honest when checks are inconclusive PRAGMA integrity_check stops at 100 errors and emits no marker row, so a saturated result was being read as the whole picture. IntegrityCheck now sets the limit itself and reports saturation as a truncated list, and doctor no longer claims corruption is limited to the search index in that case. Foreign key violations now print a next step instead of only flipping the exit code: migrations run with foreign_keys off, so orphan rows are a realistic leftover on a database that is not corrupt. Also corrects the 'search rebuild' help, which promised that 'db doctor' detects when a rebuild is needed -- integrity_check cannot see an index that is merely out of sync; gives the never-migrated case its intended message instead of a raw 'no such table: goose_db_version'; and extracts rebuildSearchIndex so the database is closed before log.Fatal exits. * refactor(cmd): promote 'db doctor' to a top-level 'doctor' command The 'db' group held a single subcommand, and the checks planned for it reach past the database: config, music folder permissions, external tools. None of those belong under 'db'. Promoting it also evens out the shape of the pair. The command that finds the problem is now top-level alongside 'search rebuild', the command that fixes it, matching the 'brew doctor' convention users already expect. 'db doctor' has never been released, so no alias or deprecation is needed. * refactor(db): tighten the doctor and search rebuild internals Follow-up cleanup with no behaviour change except where noted. integrity_check now asks the pragma for one row beyond the reported limit and treats that extra row as the proof it truncated, instead of inferring truncation from a saturated count. That distinguishes a list of exactly 100 issues from one that was cut short -- the old test could not, and 100 was SQLite's own default, so passing it was a no-op. ForeignKeyCheck returns []FKViolation instead of pre-formatted English, moving the prose to the layer that already owns the CLI vocabulary. The goose table probe shared with isSchemaEmpty becomes hasGooseTable, so 'has this database ever been migrated' has one spelling. Also folds ftsMigrationApplied into requireFTSMigration, lifts printFindings out of a closure that captured nothing, names the FTS trigger suffixes once, and corrects the ftsSchemaDDL comment: the drift test compares against the full migration chain, not the single frozen migration it claimed. * fix(db): verify the rebuilt search index before committing it RebuildFTS committed its transaction and only then ran the FTS5 integrity check, from the caller. A rebuild that produced a bad index was therefore already persisted by the time anyone noticed, leaving the user worse off than before they ran the command. The check now runs inside the transaction, so a rebuild that does not verify rolls back and leaves the original index in place. VerifyFTS keeps its *sql.DB signature for callers outside a transaction; the shared body takes the small execer interface that both *sql.DB and *sql.Tx satisfy. Adds a spec for the rollback: it removes a column the repopulating SELECT reads, so the transaction fails after the drops, and asserts the old index still answers queries. * refactor(cmd): drop the unused io.Reader parameter from confirmYES The reader was added as a test seam that no test ever used: all three callers pass os.Stdin. Back to fmt.Scanln, which drops the parameter and the now-unused os import from backup.go and search.go. * fix(cmd): stop promising a scan clears every foreign key violation doctor told the user to run 'navidrome scan -f' for any foreign key violation. SQLStore.GC only purges albums, artists, folders, annotations, bookmarks, tags and playlist tracks, so orphans elsewhere survive it and the next doctor run still reports them. player.user_id references user(id) and no scan phase touches that table at all. The advice now says a scan clears some of them and the rest have to be removed by hand, which keeps the next step the earlier round asked for without claiming a cleanup that does not happen. * docs(db): trim over-long comments on the doctor and rebuild paths Six comments ran past two lines or repeated something already stated nearby. The RebuildFTS doc claimed the rebuild rolls back on a column mismatch, which the new 'verifies before committing' sentence already implies, and a spec comment restated that same rationale a second time. * docs: drop em dashes from the comments added in this branch
2026-09-11 22:26:28 -04:00
path, _, _ := strings.Cut(conf.Server.DbPath, "?")
feat(scanner): per-library PID configuration (#6252) * feat(model): add per-library PID config columns * refactor(metadata): pass PID config to ToMediaFile and add spec validation * feat(scanner): rescan only libraries whose PID config changed * feat(server): validate library PID config and rescan on change * feat(ui): edit per-library PID config * fix(ui): label the PID mode selects * fix: tighten per-library PID rescan edge cases An interrupted PID rescan no longer upgrades every library to a full scan, a save that loses the race for the scanner logs at debug, the confirm dialog only shows when the effective PID spec changes, and it now gets translation keys. * refactor(metadata): pass the library to ToMediaFile ToMediaFile and core.Inspect took the library ID and its PID config as separate arguments, so a caller could mix values from two libraries. They now take the model.Library and resolve the effective PID config from it. * chore: tidy per-library PID comments, PropTypes and migration Trim comments that restated the code, add PropTypes to the new UI components, and recreate the migration with make migration-sql. * fix(ui): show the PID spec help under its input * feat(cmd): make inspect use the file's library PID config inspect always used the global PID config, so it showed different IDs than the scanner for files in a library with an override. It now finds the file's library in the DB and uses its effective config, falling back to the global config when there is no DB or the file is outside every library. It never creates a DB. The library path matcher moves from core/playlists to model so both can use it. * refactor: simplify per-library PID code Share the DB-file check between CLI commands, move ErrAlreadyScanning to model so core no longer imports scanner, read the libraries once for insights, and let ValidatePIDSpec accept an empty spec and look tags up directly. In the scanner, use FullScanInProgress instead of a second flag, and skip recomputing album IDs when the album spec did not change. In the UI, share the PID inputs between Create and Edit, and use docsUrl. * feat(ui): add section titles to Library Create and pre-fill Custom PID specs Custom now starts from the global spec, so admins edit a working spec instead of typing one from scratch. * fix(inspect): map files with the library-relative path the scanner uses Inspect gave metadata the file's directory as typed, so folder-based PIDs never matched the DB. It now uses the path relative to the library root, through the scanner's helper, which moves to model. * fix(scanner): say when a PID rescan only covers target folders * fix: reject tag aliases in album PID specs and match root libraries Tags are stored under canonical names, so an alias in a spec always reads as empty. In an album spec that gives every album the same ID, so album specs now require the tag name. Track specs keep accepting aliases, since the default one uses them. LibraryMatcher now matches paths under a library at the filesystem root. * refactor(model): move the tag alias lookup to tag_mappings.go * test: run the library matcher and inspect tests on Windows Build test paths with filepath instead of Unix literals, so they use the OS separator like filepath.Abs output, and drop the Windows skips. * feat(ui): add pt-BR translations for per-library PID settings
2026-10-02 05:05:32 -04:00
_, err := os.Stat(path)
return path, err == nil
}
// requireExistingDB aborts the command when the database file does not exist.
func requireExistingDB() {
if path, ok := existingDBFile(); !ok {
feat(cli): add 'doctor' and 'search rebuild' commands to recover from FTS5 corruption (#6069) * feat(db): add repair command to rebuild a corrupted FTS5 search index A corrupted media_file_fts index made every scan fail with 'database disk image is malformed', and sqlite3's built-in 'rebuild' command cannot repair contentless FTS5 tables, leaving users to hand-drop tables and triggers. Add 'navidrome db repair': it runs PRAGMA integrity_check, and when the reported corruption is confined to the FTS5 search tables, drops and recreates the three tables and their nine triggers and repopulates them from the base tables (which hold all the data, so nothing is lost). The result is verified with the FTS5-native 'integrity-check' command, which reads only the rebuilt indexes instead of re-scanning the whole database (on a 761MB production copy: ~9s full check, ~1s rebuild, sub-second verify). A --rebuild flag forces the rebuild even when the check passes, for silently desynced indexes. The rebuild refuses to run while migrations are pending, and a schema-comparison test guards the duplicated DDL against drifting from the migration. The DbPath existence check and the YES confirmation prompt, previously copy-pasted across the backup commands, are extracted into shared cmd helpers used by both backup and repair. Part of #6067 * fix(db): type the FTS migration version as int64 for 32-bit builds The untyped constant defaults to int, which overflows on arm/v7 and 386. * feat(db): split repair into 'db doctor' and 'search rebuild' commands A single 'db repair' command promised more than it delivered: the only thing it could actually repair was the search index, and its diagnosis and its fix were welded together, so a forced rebuild paid the full integrity check twice. Split it: 'navidrome db doctor' is strictly read-only, runs both PRAGMA integrity_check and PRAGMA foreign_key_check, and routes the user (to 'search rebuild' when corruption is FTS-only, to backup/.recover otherwise). 'navidrome search rebuild' just rebuilds and verifies the FTS index, which takes ~2s on a prod-size library instead of ~19s. * refactor(cmd): extract a testable doctor function and bound foreign key output Extract the doctor routing (check, classify, advise) into a function that takes an io.Writer, so the advice paths are unit-tested and the process exit happens in the cobra wrapper after the DB is closed (os.Exit was skipping the deferred close, leaving WAL/SHM files behind on the unhealthy paths). Aggregate foreign_key_check by (table, parent): the raw pragma emits one row per orphan, which is unbounded output on a large corrupted library. Also make confirmYES take an io.Reader, drop the unused return from the renamed requireExistingDB, share the FTS table list with the tests, and stop the schema-guard specs from paying for a seeded database they never use. * docs(cmd): promise 'never alters your data' instead of 'never modifies the database' Closing the doctor's connection can checkpoint a stale WAL into the main file (as any SQLite tool does), so the byte-level claim was too strong. The checks themselves are read-only and no logical content ever changes. * fix(cmd): make 'db doctor' advice honest when checks are inconclusive PRAGMA integrity_check stops at 100 errors and emits no marker row, so a saturated result was being read as the whole picture. IntegrityCheck now sets the limit itself and reports saturation as a truncated list, and doctor no longer claims corruption is limited to the search index in that case. Foreign key violations now print a next step instead of only flipping the exit code: migrations run with foreign_keys off, so orphan rows are a realistic leftover on a database that is not corrupt. Also corrects the 'search rebuild' help, which promised that 'db doctor' detects when a rebuild is needed -- integrity_check cannot see an index that is merely out of sync; gives the never-migrated case its intended message instead of a raw 'no such table: goose_db_version'; and extracts rebuildSearchIndex so the database is closed before log.Fatal exits. * refactor(cmd): promote 'db doctor' to a top-level 'doctor' command The 'db' group held a single subcommand, and the checks planned for it reach past the database: config, music folder permissions, external tools. None of those belong under 'db'. Promoting it also evens out the shape of the pair. The command that finds the problem is now top-level alongside 'search rebuild', the command that fixes it, matching the 'brew doctor' convention users already expect. 'db doctor' has never been released, so no alias or deprecation is needed. * refactor(db): tighten the doctor and search rebuild internals Follow-up cleanup with no behaviour change except where noted. integrity_check now asks the pragma for one row beyond the reported limit and treats that extra row as the proof it truncated, instead of inferring truncation from a saturated count. That distinguishes a list of exactly 100 issues from one that was cut short -- the old test could not, and 100 was SQLite's own default, so passing it was a no-op. ForeignKeyCheck returns []FKViolation instead of pre-formatted English, moving the prose to the layer that already owns the CLI vocabulary. The goose table probe shared with isSchemaEmpty becomes hasGooseTable, so 'has this database ever been migrated' has one spelling. Also folds ftsMigrationApplied into requireFTSMigration, lifts printFindings out of a closure that captured nothing, names the FTS trigger suffixes once, and corrects the ftsSchemaDDL comment: the drift test compares against the full migration chain, not the single frozen migration it claimed. * fix(db): verify the rebuilt search index before committing it RebuildFTS committed its transaction and only then ran the FTS5 integrity check, from the caller. A rebuild that produced a bad index was therefore already persisted by the time anyone noticed, leaving the user worse off than before they ran the command. The check now runs inside the transaction, so a rebuild that does not verify rolls back and leaves the original index in place. VerifyFTS keeps its *sql.DB signature for callers outside a transaction; the shared body takes the small execer interface that both *sql.DB and *sql.Tx satisfy. Adds a spec for the rollback: it removes a column the repopulating SELECT reads, so the transaction fails after the drops, and asserts the old index still answers queries. * refactor(cmd): drop the unused io.Reader parameter from confirmYES The reader was added as a test seam that no test ever used: all three callers pass os.Stdin. Back to fmt.Scanln, which drops the parameter and the now-unused os import from backup.go and search.go. * fix(cmd): stop promising a scan clears every foreign key violation doctor told the user to run 'navidrome scan -f' for any foreign key violation. SQLStore.GC only purges albums, artists, folders, annotations, bookmarks, tags and playlist tracks, so orphans elsewhere survive it and the next doctor run still reports them. player.user_id references user(id) and no scan phase touches that table at all. The advice now says a scan clears some of them and the rest have to be removed by hand, which keeps the next step the earlier round asked for without claiming a cleanup that does not happen. * docs(db): trim over-long comments on the doctor and rebuild paths Six comments ran past two lines or repeated something already stated nearby. The RebuildFTS doc claimed the rebuild rolls back on a column mismatch, which the new 'verifies before committing' sentence already implies, and a spec comment restated that same rationale a second time. * docs: drop em dashes from the comments added in this branch
2026-09-11 22:26:28 -04:00
log.Fatal("No existing database", "path", path)
}
}
func confirmYES(warning string) bool {
fmt.Println(warning)
fmt.Printf("Please enter YES (all caps) to continue: ")
var input string
_, err := fmt.Scanln(&input)
return input == "YES" && err == nil
}
feat(cli): add an artwork command group for diagnosing and re-driving artwork (#5957) * feat(artwork): add a resolution chain trace collector * feat(artwork): trace the local priority chain * fix(artwork): record priority candidates the chain never evaluated * refactor(artwork): report never-evaluated candidates as skipped * feat(artwork): trace external agents at the gate seam * feat(artwork): add repository queries to enqueue by current source * feat(artwork): expose a tracing resolver for the CLI * feat(artwork): read a single queue row by item The explain CLI must report whether an item is queued, at what priority and when it retries; the queue repository could only be drained in eligibility batches, which cannot see a row that is still backing off. * feat(cli): add artwork explain Prints why an item has the artwork it has: the stored state, its queue row, the governing config, the resolver's priority-chain walk and the verdict. Offline by default so a diagnostic run cannot add load to an external provider; --live asks the agents for real. Playlists and radios do not walk a priority chain, so they report that instead of an empty chain table. * fix(artwork): trace an external tier that never reaches an agent A configured 'external' token vanished from the chain when no enabled agent provided images for that entity type, and for synthetic artists, leaving the trace unable to say whether the tier was even considered. * fix(cli): never state an artwork outcome the walk did not observe A transient external failure traced as 'error' fell through to 'not resolved', which is the most common state behind a missing-artwork report. It is now indeterminate, and an offline win that a skipped higher-priority external candidate could have taken says so instead of naming a winner the live chain might not pick. * feat(cli): add artwork refresh * feat(cli): add artwork reprocess Bulk re-enqueues artwork by kind and/or by the source an item currently resolves from, previewing the matched count and confirming before queueing. The preview counts with CountBySource (rows matched) and reports separately what EnqueueBySource inserted: its DO NOTHING conflict policy leaves an already-queued row untouched, so the two numbers differ and the output must not claim the skipped rows were re-queued. An unknown --source is rejected against the sources present in item_artwork, rather than silently matching nothing and printing a reassuring 0. * fix(cli): cover the reprocess selection rule and validate sources table-wide The reconciliation that makes --source alone target every kind was only exercised through runReprocess, which no test calls: mutating it to `all := reprocessAll` left the suite green. It is now reprocessSelectsAll, covered for all three selectors. Scoping source validation to the selected kinds made the same well-formed filter valid or invalid depending on which other kinds were selected, and its error read the same for a typo as for a source that simply does not apply to the chosen kind. Validation is now table-wide: a typo still aborts, while a valid-but-inapplicable source falls through to "Nothing matches". Also: the prompt now counts only the kinds that reach an external agent as external cost, and --dry-run on an empty selection reports a dry run. * fix(cli): cover the reprocess --yes guard and preview the external cost Mutating the --yes check to `if true` left the suite green, so the one bypass of the confirmation was unverified. The choice is now reprocessConfirm(yes, in), covered in both directions. The external estimate only reached the operator through the prompt, which --dry-run skips — hiding the number in the one mode that exists to show it before committing. The preview now carries it, and the prompt drops the clause when no lookup will be made. An empty selection says so again under --dry-run. * feat(artwork): add read-only queue and absent counters Both are needed by the artwork status CLI: a queue breakdown by kind and priority, and the absent totals split against the recheck cutoff. * feat(cli): add artwork status Reports the queue, where artwork currently resolves from, absent counts against the 24h recheck window, and the stored config fingerprint versus the current one — the line that turns 'why is my server re-resolving everything?' into one command. fingerprint() and staleAbsentAge are exported so the CLI reports the values backfill itself compares, instead of a second copy of the formula that can silently drift. * fix(cli): lead the artwork status backfill line with the queued backlog By the time anyone runs a diagnostic, backfill has usually already stored the new fingerprint, so 'up to date' was printed while thousands of items churned through external providers. The backlog is the finding; the fingerprint is context. Also echoes the config inputs the fingerprint covers, so a change can be traced to the setting that caused it, and pins the rendered rows: the Absent values, the queue TOTAL and a queue-scoped kind/priority pair were all unasserted, so kindName and priorityName were effectively untested. FingerprintInputs is now the single listing ConfigFingerprint hashes; a pinned hash proves the value did not change. * refactor(artwork): export the trace outcome vocabulary The CLI hardcoded the outcome literals and the "external:" prefix, so renaming a constant's value in core/artwork left cmd compiling and the suite green while `artwork explain` silently degraded its verdict. Renaming a value now fails the golden vocabulary test in core/artwork and the explainResult tests in cmd. * fix(cli): keep the re-enqueue warning when a backfill is already running A stale stored fingerprint with items already queued is the worst state the system can be in: a second full re-enqueue is pending on top of the one running. The line carried the weakest wording of the three, and was untested. * refactor(artwork): drop the unreachable breaker branch from the tracing gate --live wires the tracing gate straight to passthroughGate, so errBreakerOpen can never reach it; the test only passed by injecting a fake gate. * refactor(artwork): delete the never-emitted not-reached outcome Candidates after the winner are lower priority and say nothing about why a source won; the ones that matter sit above it and are already recorded. * refactor(artwork): make the trace nil-safe in one place only add already handles a nil trace, so record's own guard was dead; Steps was the odd one out and would panic where every other method tolerates nil. * refactor(artwork): export the trace types directly ChainTrace and TraceStep were unexported types re-exported through aliases, which existed only so the CLI had a name to refer to them by. The types are public API — Resolver.Steps returns []TraceStep and the CLI constructs a ChainTrace — so name them that way and drop the indirection. Encapsulation is unchanged: add, mu and steps stay unexported, so only this package can write a step. * refactor(cli): simplify parseArtworkKind with slices.Contains Replaces a nested loop and a manual append with slices.Contains and the repo's slice.Map helper. Same behaviour, same error message. * fix(cli): print the absent artwork source under the name --source accepts `artwork explain` rendered the stored empty source as "(absent)", while `artwork reprocess --source` only accepts "absent", so pasting what explain printed straight back into reprocess was rejected as an unknown source. * refactor(artwork): own the kind list and the chain predicate in the package Export RecheckKinds and add WalksPriorityChain so the CLI stops keeping its own copies of both, and unexport externalCandidate, which nothing outside the package consumes. * refactor(cli): drop the artwork command's duplicated state and formatting Reuse artwork.RecheckKinds and artwork.WalksPriorityChain, extract newTabWriter and externalEstimate, fold reprocessSelectsAll into selectedKinds, and derive the queue total and the walks-chain flag instead of carrying them in the report structs. * test(persistence): drop two artwork-queue specs that cannot fail One seeded hash and source together and then asserted the two counts agree, so its setup guaranteed the result; the other repeated the count-does-not- enqueue property already covered by the CountBySource spec. * refactor(artwork): rename Resolver to TracingResolver for clarity * fix(cli): count playlists in the artwork reprocess external estimate The estimate used WalksPriorityChain, which is true only for artist and album, so a playlist-only reprocess reported "External lookups: none" and the confirmation prompt dropped the external-cost warning. Playlists do reach the network: through the m3u ExternalImageURL fetch when EnableM3UExternalAlbumArt is on, and — verified by test — through the generated grid, whose tiles resolve album art via the full album priority chain. Adds artwork.MayFetchExternal, a config-aware predicate for "can this kind's resolver reach the network", and uses it for the estimate. WalksPriorityChain keeps its separate job of deciding whether explain prints a chain block. * fix(cli): estimate artwork reprocess external lookups per agent, not per item The reprocess prompt billed one external lookup per externally-capable item. fetchArtistImage/fetchAlbumImage try every enabled image agent and stop early only on a hit, and resolvePlaylist can fetch the m3u image and then resolve up to four sampled albums for the grid, each walking the album agents again. The number the operator confirmed could understate real provider traffic several fold, in the prompt whose whole job is to stop a provider flood. ExternalLookupsPerItem now multiplies by the visible image-agent count and adds the playlist grid factor. It stays a floor: the CLI never calls Manager.Start(), so the plugin registry is empty and plugin-provided agents are dropped by getEnabledAgentNames. On an install with 5 agents of which 3 are plugins the count is well under the truth, so the wording is now "at least N" rather than "up to N" — a zero visible count still bills one lookup for the same reason. Fixing the plugin visibility is out of scope: Manager.Start() needs a Subsonic router and writes to the DB via syncPlugins, breaking this command group's read-only guarantee. * fix(cli): state the artwork reprocess estimate as an estimate, not a bound Neither bound is true. A ceiling is false because plugin agents are invisible to a CLI that never starts the plugin manager, and a floor is false because a local hit ends the walk before any agent is asked and a hit on the first agent skips the rest. "at least N" traded one wrong claim for another. The line now names its blind spots instead: External lookups: ~340 estimated (plugin agents not counted; local hits may need fewer). The same line is reused in the confirmation prompt, and the zero case still reads "External lookups: none." with the prompt dropping the clause entirely. The count itself is unchanged. * fix(cli): account for every configured agent in artwork explain The Agents: line printed the raw config while the Chain only showed the agents the CLI could construct, with nothing explaining the gap: plugin agents are never registered in a CLI that does not start the plugin manager, and a built-in without credentials returns nil. Three of five agents could vanish, including ones ranked above the one shown. Also treat a live external error before the winning hit like the already-handled would-try case: the resolver serves such a hit provisionally and retries later, so the verdict is indeterminate. The Result line is still not qualified when an unavailable agent might have won; that needs agent ranking, and is left to the follow-up that makes the CLI load plugin agents for real. * fix(cli): do not call an external artwork win indeterminate explainResult qualified the verdict whenever an external OutcomeError appeared before the winning hit. When a later external agent returns an image, fetchArtistImage/fetchAlbumImage discard the earlier error, so extError is false: the worker settles the item and schedules no retry. Telling the operator it may resolve differently on a retry was wrong. The warning is only correct when a lower-priority local source won while an external error was recorded, which is the case that carries extError. * fix(cli): accept --source absent when nothing is currently absent validateSources checks the requested sources against the ones item_artwork actually uses, to catch a typo. The reserved empty source (spelled 'absent' on the CLI) is a valid filter even when it matches nothing, so a scheduled 'artwork reprocess --source absent --yes' stopped working the moment the library finished resolving. Treat it as intrinsically valid and let the existing zero-match path report it. * feat(artwork): explain disc and media file artwork from the CLI `artwork explain` rejected `dc` and `mf` because it validated against RecheckKinds, the list of kinds the backfill revisits. Those are different questions: a kind with no recheck path still has artwork someone can report as wrong. Disc artwork now walks DiscArtPriority under a trace, so explain reports which entry won and why the others lost, including entries that map to no source at all (external is unsupported, a disc with no subtitle, an album folder with no images). Media file artwork traces its single embedded candidate, separating "EnableMediaFileCoverArt is off" from "the track has no embedded art" — stored state cannot tell those apart. Each command now validates against the kinds it can actually serve: explain takes all six, refresh takes artwork.RefreshableKinds (which nativeapi now shares instead of keeping its own copy), reprocess still takes RecheckKinds. Disc artwork stays out of refresh: the worker cannot resolve it, so the queue row would be rejected on every drain. WalksPriorityChain becomes Explainable, and ResolveArtist/ResolveAlbum collapse into Resolve(kind, id). * refactor(artwork): one disc-artwork walk for serving and explain resolveDisc duplicated the loop selectImageReader already ran: try each source in priority order, take the first that yields an image. The serving path and the CLI diverged on two details as a result — only selectImageReader checked ctx between candidates and logged each attempt. Both now call discArtworkReader.selectImage, which takes the chainState the CLI already uses for the other kinds. The serving path passes an untraced one, whose nil trace makes recording a no-op. selectImageReader had no other caller and is gone. The disc tests move from fromDiscArtPriority to discCandidates, so they assert the skip reason for an entry that maps to no source rather than that it silently vanished, and cancellation mid-walk is now covered. * fix(artwork): reject a nil reader in the resize cache instead of panicking resizedItem.Reader closes what open() hands back, so an open() that reports "no image" as (nil, nil) rather than an error takes the request down with a nil-pointer panic. Every caller returns an error today, and no test covered it: the resolution e2e harness stubs the resize reader out entirely, so no e2e path reaches this code at all. Guard it and cover Reader directly. * refactor(artwork): move the keeps-state fact into core, drop a redundant guard keepsArtworkState lived in package cmd and re-derived by hand what RefreshableKinds already encodes: the same five-of-six kinds. It is now artwork.KeepsState, beside the list, with a test pinning the two together — nothing else stopped them drifting, and a drift would have explain report stored state for a kind that keeps none. serveDisc's closure also hand-rolled a nil-reader error that both consumers of open() now produce themselves: serveSource for a full-size request, resizedItem.Reader for a resized one. * fix(artwork): route disc candidates through the shared resolvers openCandidate ran its own source loop and threw the error away, so a disc track that exists but cannot be parsed traced as "miss" — indistinguishable from a track with no embedded art. fromTag and fromFFmpegTag already report that case as errSourceUnreadable; only this loop was discarding it. Telling those two apart is what the trace is for. Candidates now carry a resolve func instead of raw sources: embedded goes to resolveEmbedded, and the folder-backed entries to resolveFolderSource, extracted from resolveFolderFile so both callers classify an unopenable file the same way. openCandidate and its absolute-path special case go away with it. Disc's own fromExternalFile and fromDiscSubtitle still swallow open errors, so folder candidates cannot report unreadable yet; that is a change to their error contracts. * fix(artwork): report an unreadable local candidate as indeterminate processor.acquire treats resolution.localError exactly as it treats extError: a fault is not a definitive "no image", so it retries instead of settling absent. explainResult qualified only the external case, so a chain that ended on an unreadable local candidate printed "not resolved" — the one verdict that says the walk was conclusive. The qualification belongs only to the unresolved branch. chainState.try stamps extErr onto a hit and deliberately drops localErr, so an unreadable step followed by a hit is settled as found and must not carry a warning; a test pins that. Found by Codex on 5f65d7cfa.
2026-08-14 21:07:56 -04:00
// newTabWriter keeps every CLI table on the same column settings.
func newTabWriter(out io.Writer) *tabwriter.Writer {
return tabwriter.NewWriter(out, 0, 4, 2, ' ', 0)
}
func getAdminContext(ctx context.Context) (model.DataStore, context.Context) {
sqlDB := db.Db()
ds := persistence.New(sqlDB)
ctx = auth.WithAdminUser(ctx, ds)
u, _ := request.UserFrom(ctx)
if !u.IsAdmin {
log.Fatal(ctx, "There must be at least one admin user to run this command.")
}
return ds, ctx
}
func getUser(ctx context.Context, id string, ds model.DataStore) (*model.User, error) {
refactor(persistence): stateless repositories with per-call context (#6149) * refactor(persistence): adopt generic deluan/rest repository API Pin deluan/rest to the refactor branch. REST-facing repository methods take a context and return typed values. Drop DataStore.Resource and ResourceRepository; the native API names typed repositories directly through a per-request adapter that later commits remove. * refactor(persistence): base repository helpers take a context * refactor(persistence): LibraryRepository takes a context per call * refactor(persistence): PropertyRepository takes a context per call * refactor(persistence): UserPropsRepository takes a context per call * refactor(persistence): TranscodingRepository takes a context per call * refactor(persistence): ShareRepository takes a context per call * refactor(persistence): PlayerRepository takes a context per call * refactor(persistence): RadioRepository takes a context per call * refactor(persistence): PlayQueueRepository takes a context per call * refactor(persistence): Tag and Genre repositories take a context per call * refactor(persistence): PluginRepository takes a context per call * refactor(persistence): Scrobble repositories take a context per call * refactor(persistence): FolderRepository takes a context per call * refactor(persistence): Artwork repositories take a context per call * refactor(persistence): UserRepository takes a context per call * refactor(persistence): ArtistRepository takes a context per call ReadAll no longer rewrites the shared sort mappings for the role filter; it works on a per-call copy. * test(persistence): assert artist role sort sanitization in ReadAll * refactor(persistence): AlbumRepository takes a context per call * test(persistence): pass the test context to album repository helpers * refactor(persistence): MediaFileRepository takes a context per call * refactor(persistence): Playlist repositories take a context per call * refactor(persistence): build all repositories once per store * refactor(core): REST repository wrappers are built once * refactor(persistence): repositories are stateless Remove the context field from the base repository and the per-request REST adapter. Enable the containedctx linter so no repository can hold a request context again. * chore(lint): skip containedctx in test files * refactor: share simplifications from the stateless repositories sweep Add deleteOwnedAll on sqlRepository and use it in player/share Delete to remove the duplicated bulk-delete loop; have Share.Repository() return model.ShareRepository so subsonic sharing.go drops its repeated type assertions. * chore(core): assert REST wrappers implement Persistable * chore: reformat imports * perf(persistence): build repositories on first use Each transaction store used to construct all 21 repositories up front, paying for filter and sort mapping setup the block never touched. Fields are now sync.OnceValue thunks, so a store only builds what it uses. * fix(persistence): clean plugin references per deleted user A bulk user delete that fails on a later id had already removed the earlier rows but skipped their plugin cleanup. Cleanup now runs right after each successful delete. * fix(core): unload disabled plugins even when a user delete fails A bulk delete can fail on a later id after earlier users were removed and their plugins auto-disabled. The wrapper returned before unloading, leaving those plugins running until the next successful delete or a restart. * chore(deps): pin deluan/rest to v1.0.1 Replaces the pseudo-version of the refactor branch with the tagged release. REST error messages now name the bare type (Artist, not model.Artist). * test: use the spec context instead of context.Background() Replace the context.Background()/context.TODO() calls this branch added to tests with the spec's ctx, GinkgoT().Context(), or t/b.Context(), so repository calls are bound to the running spec's lifetime. * test: declare the spec context once per Describe Set ctx from GinkgoT().Context() first in each top-level BeforeEach and reuse it, building user contexts on top of it instead of repeating inline calls.
2026-09-25 18:06:10 -04:00
user, err := ds.User().FindByUsername(ctx, id)
if err != nil && !errors.Is(err, model.ErrNotFound) {
return nil, fmt.Errorf("finding user by name: %w", err)
}
if errors.Is(err, model.ErrNotFound) {
refactor(persistence): stateless repositories with per-call context (#6149) * refactor(persistence): adopt generic deluan/rest repository API Pin deluan/rest to the refactor branch. REST-facing repository methods take a context and return typed values. Drop DataStore.Resource and ResourceRepository; the native API names typed repositories directly through a per-request adapter that later commits remove. * refactor(persistence): base repository helpers take a context * refactor(persistence): LibraryRepository takes a context per call * refactor(persistence): PropertyRepository takes a context per call * refactor(persistence): UserPropsRepository takes a context per call * refactor(persistence): TranscodingRepository takes a context per call * refactor(persistence): ShareRepository takes a context per call * refactor(persistence): PlayerRepository takes a context per call * refactor(persistence): RadioRepository takes a context per call * refactor(persistence): PlayQueueRepository takes a context per call * refactor(persistence): Tag and Genre repositories take a context per call * refactor(persistence): PluginRepository takes a context per call * refactor(persistence): Scrobble repositories take a context per call * refactor(persistence): FolderRepository takes a context per call * refactor(persistence): Artwork repositories take a context per call * refactor(persistence): UserRepository takes a context per call * refactor(persistence): ArtistRepository takes a context per call ReadAll no longer rewrites the shared sort mappings for the role filter; it works on a per-call copy. * test(persistence): assert artist role sort sanitization in ReadAll * refactor(persistence): AlbumRepository takes a context per call * test(persistence): pass the test context to album repository helpers * refactor(persistence): MediaFileRepository takes a context per call * refactor(persistence): Playlist repositories take a context per call * refactor(persistence): build all repositories once per store * refactor(core): REST repository wrappers are built once * refactor(persistence): repositories are stateless Remove the context field from the base repository and the per-request REST adapter. Enable the containedctx linter so no repository can hold a request context again. * chore(lint): skip containedctx in test files * refactor: share simplifications from the stateless repositories sweep Add deleteOwnedAll on sqlRepository and use it in player/share Delete to remove the duplicated bulk-delete loop; have Share.Repository() return model.ShareRepository so subsonic sharing.go drops its repeated type assertions. * chore(core): assert REST wrappers implement Persistable * chore: reformat imports * perf(persistence): build repositories on first use Each transaction store used to construct all 21 repositories up front, paying for filter and sort mapping setup the block never touched. Fields are now sync.OnceValue thunks, so a store only builds what it uses. * fix(persistence): clean plugin references per deleted user A bulk user delete that fails on a later id had already removed the earlier rows but skipped their plugin cleanup. Cleanup now runs right after each successful delete. * fix(core): unload disabled plugins even when a user delete fails A bulk delete can fail on a later id after earlier users were removed and their plugins auto-disabled. The wrapper returned before unloading, leaving those plugins running until the next successful delete or a restart. * chore(deps): pin deluan/rest to v1.0.1 Replaces the pseudo-version of the refactor branch with the tagged release. REST error messages now name the bare type (Artist, not model.Artist). * test: use the spec context instead of context.Background() Replace the context.Background()/context.TODO() calls this branch added to tests with the spec's ctx, GinkgoT().Context(), or t/b.Context(), so repository calls are bound to the running spec's lifetime. * test: declare the spec context once per Describe Set ctx from GinkgoT().Context() first in each top-level BeforeEach and reuse it, building user contexts on top of it instead of repeating inline calls.
2026-09-25 18:06:10 -04:00
user, err = ds.User().Get(ctx, id)
if err != nil {
return nil, fmt.Errorf("finding user by id: %w", err)
}
}
return user, nil
}