diff --git a/backend/download/fillin.go b/backend/download/fillin.go new file mode 100644 index 0000000..d645c6c --- /dev/null +++ b/backend/download/fillin.go @@ -0,0 +1,190 @@ +package download + +import ( + "cmp" + "context" + "fmt" + "strings" + + "yellowjacket/backend/jobs" +) + +// Filling in an almost-complete album (#276). +// +// A grab that delivers nine of twelve tracks clears the completeness +// floor and is imported, and before this the other three were never +// looked for. On Soulseek that is the commonest way an album ends up +// almost right: one peer's folder is missing a track, or one file +// failed. So after an import, each missing track is searched for on +// its own and fetched from somewhere else, into the same album. + +// maxFillInTracks bounds how many tracks are fetched one by one. An +// album missing more than a few is a different candidate's job, not a +// dozen single-track grabs. +const maxFillInTracks = 3 + +// fillIn fetches the tracks a successful import did not deliver. It +// never fails the download: the album is already imported, and a track +// it cannot find is logged and left. +func (m *Manager) fillIn( + ctx context.Context, + dl Download, + main Candidate, + imported ImportResult, + job *jobs.Handle, +) { + missing := missingTracks(dl, imported.Matched) + if len(missing) == 0 { + return + } + + for _, t := range missing { + if ctx.Err() != nil { + return + } + + paths, err := m.fillInTrack(ctx, dl, main, t, job) + if err != nil { + m.logger.Info( + "could not fill in a missing track", + "download", dl.ID, + "track", t.Title, + "error", err, + ) + + if job != nil { + job.Logf(jobs.LevelWarn, fmt.Sprintf( + "Could not find %q elsewhere: %v", t.Title, err, + )) + } + + continue + } + + if job != nil { + job.Logf(jobs.LevelInfo, fmt.Sprintf( + "Filled in %q from another source (%d file)", t.Title, len(paths), + )) + } + } +} + +// missingTracks is what an import left out, when filling it in is +// worth trying: an album with a tracklist, a few tracks short. +func missingTracks(dl Download, matched []ExpectedTrack) []ExpectedTrack { + // A recording request is one track; there is no album to complete. + if dl.RecordingMBID != "" || len(dl.Expected) < 2 { + return nil + } + + have := make(map[trackKey]bool, len(matched)) + for _, t := range matched { + have[keyOf(t)] = true + } + + var missing []ExpectedTrack + + for _, t := range dl.Expected { + if !have[keyOf(t)] { + missing = append(missing, t) + } + } + + // A half-empty album was a poor copy, not a nearly complete one. + if len(missing) > maxFillInTracks || 2*len(missing) >= len(dl.Expected) { + return nil + } + + return missing +} + +// fillInTrack searches for one track and grabs the first acceptable +// copy that is not from the source that already failed to supply it. +// One attempt: a fill-in that walks a candidate list per track would +// multiply a download's grabs by the number of gaps. +func (m *Manager) fillInTrack( + ctx context.Context, + dl Download, + main Candidate, + t ExpectedTrack, + job *jobs.Handle, +) ([]string, error) { + want := trackRequest(dl, t) + + ranked, err := m.Search(ctx, want) + if err != nil { + return nil, err + } + + prefs := m.preferences() + + for _, c := range ranked { + if ruledOutBy(c, []Candidate{main}) || !autoAcceptable(want, c, prefs) { + continue + } + + // The track is being put into an album this app placed; a + // delegate would put it in its own library instead. + if plan, err := m.planTransfer(want, c); err != nil || plan.delegated() { + continue + } + + narrowed, ok := narrowTo(c, t) + if !ok { + continue + } + + out := m.attemptGrab(ctx, dl, narrowed, job, []ExpectedTrack{t}) + + if out.item.StagingDir != "" { + if err := m.staging.Release(out.item.StagingDir); err != nil { + m.logger.Warn("could not release staging dir", "error", err) + } + } + + if out.err != nil { + return nil, out.err + } + + if err := m.store.SetItemImported( + ctx, out.item.ID, out.imported.Paths, + ); err != nil { + m.logger.Warn("could not record imported paths", "error", err) + } + + return out.imported.Paths, nil + } + + return nil, ErrNoCandidates +} + +// trackRequest is the search for one track of an album: the track's +// artist and title as the query, the album kept so a copy from that +// album outranks the same song off a compilation, and one expected +// track so a single file is a complete answer. +func trackRequest(dl Download, t ExpectedTrack) Download { + artist := cmp.Or(t.Artist, dl.Artist) + + want := dl + want.Query = strings.TrimSpace(artist + " " + t.Title) + want.Expected = []ExpectedTrack{t} + + return want +} + +// narrowTo trims a candidate to the one file that aligns to t, so the +// grab fetches a track rather than whatever else the folder offered. +func narrowTo(c Candidate, t ExpectedTrack) (Candidate, bool) { + aligned, _ := matchFiles(c.Files, []ExpectedTrack{t}) + + for _, f := range aligned { + if f.IsAudio && f.MatchedTo == t.Position { + c.Files = []CandidateFile{f} + c.TotalSize = f.Size + + return c, true + } + } + + return Candidate{}, false +} diff --git a/backend/download/fillin_test.go b/backend/download/fillin_test.go new file mode 100644 index 0000000..7b79d47 --- /dev/null +++ b/backend/download/fillin_test.go @@ -0,0 +1,168 @@ +package download + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" +) + +// Filling in the tracks an almost-complete album is missing (#276). + +func fiveTrackTitles() []string { + return append(allTitles(), "Let Down") +} + +func fiveTrackDownload() Download { + dl := fourTrackDownload() + dl.Expected = append(dl.Expected, ExpectedTrack{Position: 5, Title: "Let Down"}) + + return dl +} + +func TestMissingTracks(t *testing.T) { + t.Parallel() + + dl := fiveTrackDownload() + got := func(positions ...int) []ExpectedTrack { + out := make([]ExpectedTrack, 0, len(positions)) + for _, p := range positions { + out = append(out, dl.Expected[p-1]) + } + + return out + } + + cases := []struct { + name string + dl Download + matched []ExpectedTrack + want int + }{ + {"complete", dl, got(1, 2, 3, 4, 5), 0}, + {"one short", dl, got(1, 2, 3, 4), 1}, + {"two short", dl, got(1, 2, 3), 2}, + {"half gone is a poor copy", dl, got(1, 2), 0}, + {"a recording is not an album", func() Download { + d := dl + d.RecordingMBID = "rec" + + return d + }(), got(1, 2, 3, 4), 0}, + } + + for _, tc := range cases { + if n := len(missingTracks(tc.dl, tc.matched)); n != tc.want { + t.Errorf("%s: %d missing, want %d", tc.name, n, tc.want) + } + } + + big := fiveTrackDownload() + for i := 6; i <= 20; i++ { + big.Expected = append(big.Expected, ExpectedTrack{Position: i, Title: "T" + itoa(i)}) + } + + if n := len(missingTracks(big, big.Expected[:16])); n != 0 { + t.Errorf("four of twenty missing: %d filled in, want none past %d", n, maxFillInTracks) + } +} + +// A fill-in import takes only the file that is the missing track, and +// places it in the album with the album's tags; anything else the grab +// brought is left out. +func TestImportOnlyTakesTheMissingTrack(t *testing.T) { + t.Parallel() + + f := newImportFixture(t, + "05 - Let Down.flac", + "02 - Paranoid Android.flac", + ) + + dl := fiveTrackDownload() + + got, err := f.importer.Import( + context.Background(), dl, + Result{Dir: f.dir, Files: f.files}, + ImportOptions{LibraryRoot: f.root, WriteTags: true, Only: dl.Expected[4:]}, + ) + if err != nil { + t.Fatalf("Import: %v", err) + } + + want := filepath.Join(f.root, "Radiohead", "OK Computer", "05 Let Down.flac") + if len(got.Paths) != 1 || got.Paths[0] != want { + t.Errorf("imported %q, want only %s", got.Paths, want) + } + + // A file that is not the missing track is not imported at all. + g := newImportFixture(t, "02 - Paranoid Android.flac") + + if _, err := g.importer.Import( + context.Background(), dl, + Result{Dir: g.dir, Files: g.files}, + ImportOptions{LibraryRoot: g.root, WriteTags: true, Only: dl.Expected[4:]}, + ); !errors.Is(err, ErrTooIncomplete) { + t.Errorf("Import = %v, want nothing matched", err) + } +} + +// The album comes from one source missing its fifth track; the fifth is +// then found on its own at another and lands in the same album. +func TestManagerFillsInAMissingTrack(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + + titles := fiveTrackTitles() + + album := NewFakeProvider(1, "album", Caps{CanSearch: true, CanTransport: true}) + ac := candidateFor("album-cand", titles, ".flac", 30_000_000) + ac.ProviderID = 1 + album.Candidates = []Candidate{ac} + + for i, tt := range titles[:4] { + album.Written[trackToken(i+1)+" - "+tt+".flac"] = []byte("audio-data") + } + + single := NewFakeProvider(2, "single", Caps{CanSearch: true, CanTransport: true}) + sc := Candidate{ + ID: "single-cand", + Protocol: ProtocolDirect, + Title: "Radiohead - OK Computer", + Artist: "Radiohead", + Files: []CandidateFile{{ + Path: "Radiohead - OK Computer/05 - Let Down.flac", + Size: 30_000_000, + }}, + Health: 0.5, + ProviderID: 2, + } + single.Candidates = []Candidate{sc} + single.Written["05 - Let Down.flac"] = []byte("audio-data") + + f.manager.installProvider(Config{ID: 1, Priority: 90}, album) + f.manager.installProvider(Config{ID: 2, Priority: 10}, single) + + dl := fiveTrackDownload() + + if _, err := f.manager.Start(context.Background(), dl); err != nil { + t.Fatalf("Start: %v", err) + } + + waitForDownloadState(t, f.store, dl.ID, StateComplete) + + if album.GrabCallCount() != 1 || single.GrabCallCount() != 1 { + t.Errorf( + "grabs: album=%d single=%d, want 1 and 1", + album.GrabCallCount(), single.GrabCallCount(), + ) + } + + for i, tt := range titles { + p := filepath.Join(f.root, "Radiohead", "OK Computer", trackToken(i+1)+" "+tt+".flac") + if _, err := os.Stat(p); err != nil { + t.Errorf("track %d not in the library: %v", i+1, err) + } + } +} diff --git a/backend/download/importer.go b/backend/download/importer.go index 45020a5..c4c2022 100644 --- a/backend/download/importer.go +++ b/backend/download/importer.go @@ -79,6 +79,12 @@ type ImportOptions struct { // them. Off for delegate providers, which have already imported // and tagged the files themselves. WriteTags bool + + // Only, when set, imports just the files that align to these + // tracks of the download and skips the completeness check: it is a + // fill-in for tracks an earlier grab of the same album did not + // deliver (#276), tagged and placed as part of that album. + Only []ExpectedTrack } // DefaultPathTemplate is the layout used when none is configured. @@ -118,6 +124,10 @@ type ImportResult struct { // Skipped counts non-audio files left in staging (logs, cue sheets, // scene .nfo files) — deliberately not imported. Skipped int + + // Matched are the expected tracks an imported file was aligned to, + // which is how a caller learns what the grab did not deliver. + Matched []ExpectedTrack } // Import verifies, tags and moves a completed grab into the library. @@ -140,14 +150,25 @@ func (i *Importer) Import( return ImportResult{}, ErrNoAudio } - if err := checkCompleteness(len(audio), dl); err != nil { - return ImportResult{}, err + if len(opts.Only) == 0 { + if err := checkCompleteness(len(audio), dl); err != nil { + return ImportResult{}, err + } } // Align staged files to the expected tracklist so tags and // filenames reflect the release, not the uploader's naming. plan := i.planFiles(audio, dl) + if len(opts.Only) > 0 { + plan = onlyTracks(plan, opts.Only) + if len(plan) == 0 { + return ImportResult{}, fmt.Errorf( + "%w: no file matched the missing track", ErrTooIncomplete, + ) + } + } + out := ImportResult{ Paths: make([]string, 0, len(plan)), Skipped: skipped, @@ -184,11 +205,49 @@ func (i *Importer) Import( } out.Paths = append(out.Paths, dest) + + if p.Matched { + out.Matched = append(out.Matched, p.Track) + } } return out, nil } +// trackKey identifies an expected track within a release. +type trackKey struct{ disc, position int } + +func keyOf(t ExpectedTrack) trackKey { + return trackKey{disc: t.DiscNumber, position: t.Position} +} + +// onlyTracks keeps the planned files aligned to one of want. A fill-in +// grab can bring more than the one file it was after — a folder where +// the title also matched a live take — and anything else would land in +// the album as a duplicate or a stranger. +func onlyTracks(plan []plannedFile, want []ExpectedTrack) []plannedFile { + keys := make(map[trackKey]bool, len(want)) + for _, t := range want { + keys[keyOf(t)] = true + } + + out := make([]plannedFile, 0, len(want)) + seen := map[trackKey]bool{} + + for _, p := range plan { + k := keyOf(p.Track) + if !p.Matched || !keys[k] || seen[k] { + continue + } + + seen[k] = true + + out = append(out, p) + } + + return out +} + // plannedFile pairs a staged file with the expected track it matched. type plannedFile struct { Source string diff --git a/backend/download/manager.go b/backend/download/manager.go index 3b429de..d41bb28 100644 --- a/backend/download/manager.go +++ b/backend/download/manager.go @@ -747,8 +747,9 @@ func (m *Manager) grab( var failed []Candidate for { - out := m.attemptGrab(ctx, dl, c, job) + out := m.attemptGrab(ctx, dl, c, job, nil) if out.err == nil { + m.fillIn(ctx, dl, c, out.imported, job) m.finishGrab(ctx, dl, out.item, out.imported, job) return @@ -836,6 +837,7 @@ func (m *Manager) attemptGrab( dl Download, c Candidate, job *jobs.Handle, + only []ExpectedTrack, ) grabOutcome { // Who will move the bytes is decided before any slot is taken, so // the transfer waits in its own provider's queue rather than in a @@ -942,6 +944,7 @@ func (m *Manager) attemptGrab( opts := m.importOptions() opts.WriteTags = true + opts.Only = only opts.LibraryRoot, err = m.library.LibraryPath(dl.LibraryID) if err != nil { diff --git a/backend/download/provider_slskd.go b/backend/download/provider_slskd.go index a0fda64..bb62a5e 100644 --- a/backend/download/provider_slskd.go +++ b/backend/download/provider_slskd.go @@ -573,8 +573,10 @@ func isVariousArtists(artist string) bool { // usually matches one file per folder. The two-file floor that filters // out one-file noise for an album therefore filtered out every result // for a track, and a single-track request could never be served here. +// A request expecting one track — a recording, or the fill-in for one +// missing from an album (#276) — takes a one-file folder. func minFilesFor(dl Download) int { - if dl.RecordingMBID != "" { + if dl.RecordingMBID != "" || len(dl.Expected) == 1 { return 1 }