docs: library comments say what the scanner and resolver do, not how they came to be (#6277)
release / govulncheck (push) Successful in 20s
release / web (push) Successful in 1m20s
release / go (push) Successful in 1m37s
release / integration (push) Successful in 5m15s
release / android (push) Successful in 5m26s
release / Build signed APK (releases and dev) (push) Successful in 5m45s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Successful in 16s
release / Verify release artifacts (tag releases only) (push) Skipped
release / govulncheck (push) Successful in 20s
release / web (push) Successful in 1m20s
release / go (push) Successful in 1m37s
release / integration (push) Successful in 5m15s
release / android (push) Successful in 5m26s
release / Build signed APK (releases and dev) (push) Successful in 5m45s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Successful in 16s
release / Verify release artifacts (tag releases only) (push) Skipped
Rewrites the internal/library comment blocks to rule 32. Task, milestone, decision and rule ids are gone; the incidents behind them are restated as the constraint they left (why a file is deleted before its row, why the missing mark is a mark, why duplicates block on duration). Counts and album names from one library are dropped from production comments; test comments keep the names their fixtures use. The tagReadVersion log keeps each version's before and after, without the measurements. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
+34
-33
@@ -9,48 +9,46 @@ import (
|
||||
syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync"
|
||||
)
|
||||
|
||||
// Move detection (#2528).
|
||||
// Move detection.
|
||||
//
|
||||
// Track identity is file_path: UpsertTrack conflicts on it, and the reconcile
|
||||
// pass in reconcile.go clears a missing mark when the walk sees that same path
|
||||
// again. So a file that comes back exactly where it was restores cleanly, but a
|
||||
// file that comes back RENAMED or in a different directory looked, to the
|
||||
// file that comes back RENAMED or in a different directory looks, to the
|
||||
// scanner, like a deletion plus an unrelated new track:
|
||||
//
|
||||
// - the old row stayed marked missing, holding the like and every play_event
|
||||
// - a fresh row appeared with no history
|
||||
// - nothing connected them
|
||||
// - the old row stays marked missing, holding the like and every play_event
|
||||
// - a fresh row appears with no history
|
||||
// - nothing connects them
|
||||
//
|
||||
// A liked song read as unliked after a retag, its play count reset to zero, and
|
||||
// Rediscover could offer it as a discovery. All silently. Renumbering an album
|
||||
// was enough to do it — which is exactly what happened on the operator's copy of
|
||||
// Minutes to Midnight.
|
||||
// A liked song would read as unliked after a retag, its play count reset to
|
||||
// zero, and Rediscover could offer it as a discovery. Renumbering an album is
|
||||
// enough to do it.
|
||||
//
|
||||
// The fix adopts the existing row rather than inserting: re-point its file_path
|
||||
// at the new location and clear the mark. The caller's normal UpsertTrack then
|
||||
// conflicts on file_path and updates THAT row, so the track id survives and
|
||||
// likes, plays and playlist memberships travel with it. Clients see an update
|
||||
// rather than a delete-and-create, so no cache churn either.
|
||||
// So the scan adopts the existing row rather than inserting: re-point its
|
||||
// file_path at the new location and clear the mark. The caller's normal
|
||||
// UpsertTrack then conflicts on file_path and updates THAT row, so the track id
|
||||
// survives and likes, plays and playlist memberships travel with it. Clients
|
||||
// see an update rather than a delete-and-create, so no cache churn either.
|
||||
//
|
||||
// Only rows already marked missing are eligible. A row whose file is present
|
||||
// elsewhere is a duplicate, not a move, and re-pointing it would corrupt the
|
||||
// copy that still exists. That constraint is what makes this safe, and the
|
||||
// marking added in #2523 is what makes it expressible.
|
||||
// missing mark is what makes it expressible.
|
||||
|
||||
// trackAdopter is the slice of dbq.Queries move detection needs, narrowed so the
|
||||
// match/ambiguity logic can be tested against a fake.
|
||||
// trackAdopter is the slice of dbq.Queries move detection needs, narrowed so
|
||||
// the match/ambiguity logic can be tested against a fake.
|
||||
type trackAdopter interface {
|
||||
FindMissingTrackByMbid(ctx context.Context, mbid string) ([]dbq.FindMissingTrackByMbidRow, error)
|
||||
FindMissingTrackByAudioHash(ctx context.Context, audioStreamSha256 []byte) ([]dbq.FindMissingTrackByAudioHashRow, error)
|
||||
AdoptTrackPath(ctx context.Context, arg dbq.AdoptTrackPathParams) (int64, error)
|
||||
}
|
||||
|
||||
// adoptMovedTrack looks for a missing row that is the same recording as the file
|
||||
// at newPath and re-points it there. Reports whether a row was adopted.
|
||||
// adoptMovedTrack looks for a missing row that is the same recording as the
|
||||
// file at newPath and re-points it there. Reports whether a row was adopted.
|
||||
//
|
||||
// Never returns an error: failing to detect a move is a missed optimisation, not
|
||||
// a broken scan. The caller carries on and inserts a fresh row, which is the
|
||||
// pre-#2528 behaviour.
|
||||
// Never returns an error: failing to detect a move is a missed optimisation,
|
||||
// not a broken scan. The caller carries on and inserts a fresh row.
|
||||
func (s *Scanner) adoptMovedTrack(
|
||||
ctx context.Context, q trackAdopter, newPath string,
|
||||
audioHash []byte, recordingMBID string,
|
||||
@@ -67,10 +65,10 @@ func (s *Scanner) adoptMovedTrack(
|
||||
}
|
||||
}
|
||||
|
||||
// Audio-hash fallback for untagged files (#3914): the encoded audio itself,
|
||||
// which a rename, a move or a retag leaves unchanged. Absent when the hash
|
||||
// could not be taken, and then nothing is matched — an empty hash must never
|
||||
// be looked up, or every unhashable file would pair with every other.
|
||||
// Audio-hash fallback for untagged files: the encoded audio itself, which a
|
||||
// rename, a move or a retag leaves unchanged. Absent when the hash could
|
||||
// not be taken, and then nothing is matched — an empty hash must never be
|
||||
// looked up, or every unhashable file would pair with every other.
|
||||
if len(audioHash) > 0 {
|
||||
rows, err := q.FindMissingTrackByAudioHash(ctx, audioHash)
|
||||
if err != nil {
|
||||
@@ -84,7 +82,8 @@ func (s *Scanner) adoptMovedTrack(
|
||||
return false
|
||||
}
|
||||
|
||||
// candidate is the shared shape of both lookups, so uniqueMatch is written once.
|
||||
// candidate is the shared shape of both lookups, so uniqueMatch is written
|
||||
// once.
|
||||
type candidate struct {
|
||||
id pgtype.UUID
|
||||
filePath string
|
||||
@@ -107,9 +106,10 @@ func rowsFromAudioHash(rows []dbq.FindMissingTrackByAudioHashRow) []candidate {
|
||||
}
|
||||
|
||||
// uniqueMatch requires exactly one candidate. Adopting an arbitrary row out of
|
||||
// several would attach this file's future history to a coin flip, which is worse
|
||||
// than starting a fresh row — a fork is recoverable later, a wrong merge isn't.
|
||||
// Libraries with genuine duplicates hit this, so it's logged rather than silent.
|
||||
// several would attach this file's future history to a coin flip, which is
|
||||
// worse than starting a fresh row — a fork is recoverable later, a wrong merge
|
||||
// isn't. Libraries with genuine duplicates hit this, so it's logged rather than
|
||||
// silent.
|
||||
func (s *Scanner) uniqueMatch(
|
||||
cands []candidate, newPath, via string,
|
||||
) (candidate, bool) {
|
||||
@@ -130,8 +130,9 @@ func (s *Scanner) adopt(
|
||||
) bool {
|
||||
n, err := q.AdoptTrackPath(ctx, dbq.AdoptTrackPathParams{ID: c.id, FilePath: newPath})
|
||||
if err != nil {
|
||||
// A unique violation on file_path means something else claimed this path
|
||||
// first. Fall through to a normal insert rather than failing the file.
|
||||
// A unique violation on file_path means something else claimed this
|
||||
// path first. Fall through to a normal insert rather than failing the
|
||||
// file.
|
||||
s.logger.Warn("library scan: adopting moved track failed",
|
||||
"path", newPath, "via", via, "err", err)
|
||||
return false
|
||||
@@ -141,7 +142,7 @@ func (s *Scanner) adopt(
|
||||
// update, so its mark was already cleared.
|
||||
return false
|
||||
}
|
||||
// Logged with both paths: this is the operator's only window onto a
|
||||
// Logged with both paths: this is an admin's only window onto a
|
||||
// reorganisation being understood as a move rather than a new track.
|
||||
s.logger.Info("library scan: track moved, history preserved",
|
||||
"from", c.filePath, "to", newPath, "via", via, "track_id", syncpkg.FormatUUID(c.id))
|
||||
|
||||
Reference in New Issue
Block a user