From 0a33b9d653c4b63d066c6e99e48fa1e28b25b8b3 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Sat, 26 Sep 2026 16:12:06 -0400 Subject: [PATCH] fix(download): try the next acceptable copy when a transfer fails A failed transfer failed the whole download. On Soulseek the usual failure is one peer being offline or refusing, and a popular album has several other peers offering the same folder; the ranked list that names them was already held in m.results and nothing walked it. grab now loops: when a candidate's transfer fails, or delivers too little of the album to import, the next candidate is tried in its place, up to three in all. Three rules keep that honest: - Only a candidate auto-pick would itself have accepted is offered, so a second choice clears the same match, quality and guardrail gates as the first. - On Soulseek the failure is the peer's, so every folder that peer offered is skipped with it; elsewhere only the failed release is. - A candidate the user picked by hand does not fall back. They chose that copy, and quietly substituting another is a decision they did not make. The same change fixes auto-pick grabbing the wrong candidate. AutoPickVeto judges the best candidate inside the user's guardrails, but Start and Attempt then grabbed ranked[0] -- so when the overall best was over the size ceiling, the veto passed on the strength of the second and the first was downloaded anyway: the one copy the user had said not to take unattended. autoPick returns the candidate the veto actually judged. Closes #263 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT --- backend/download/concurrency_test.go | 2 +- backend/download/fallback_test.go | 261 ++++++++++++++++++++++++++ backend/download/manager.go | 265 +++++++++++++++++++++------ backend/download/rank.go | 39 ++++ 4 files changed, 507 insertions(+), 60 deletions(-) create mode 100644 backend/download/fallback_test.go diff --git a/backend/download/concurrency_test.go b/backend/download/concurrency_test.go index 308e733..729c07f 100644 --- a/backend/download/concurrency_test.go +++ b/backend/download/concurrency_test.go @@ -47,7 +47,7 @@ func grabAll( go func() { defer wg.Done() - f.manager.grab(ctx, dl, candidate, nil) + f.manager.grab(ctx, dl, candidate, nil, false) }() } diff --git a/backend/download/fallback_test.go b/backend/download/fallback_test.go new file mode 100644 index 0000000..48565d5 --- /dev/null +++ b/backend/download/fallback_test.go @@ -0,0 +1,261 @@ +package download + +import ( + "context" + "errors" + "os" + "testing" +) + +// A transfer that fails on one copy of an album is not a failed +// download while another acceptable copy exists. On Soulseek the usual +// failure is one peer being offline, with several others offering the +// same folder. + +var errPeerOffline = errors.New("peer went offline") + +func TestManagerFallsBackToTheNextCandidate(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + + // The failing source ranks first on priority, so the fallback is + // what reaches the one that works. + bad := fakeWithAlbum(1, "offline-peer", ".flac") + bad.GrabErr = errPeerOffline + good := fakeWithAlbum(2, "online-peer", ".flac") + + f.manager.installProvider(Config{ID: 1, Priority: 90}, bad) + f.manager.installProvider(Config{ID: 2, Priority: 10}, good) + + dl := fourTrackDownload() + + if _, err := f.manager.Start(context.Background(), dl); err != nil { + t.Fatalf("Start: %v", err) + } + + waitForDownloadState(t, f.store, dl.ID, StateComplete) + + if bad.GrabCalls != 1 || good.GrabCalls != 1 { + t.Errorf( + "grabs: failing=%d working=%d, want 1 and 1", + bad.GrabCalls, good.GrabCalls, + ) + } + + // The abandoned attempt's staging goes with it; only a request that + // fails outright keeps its staging for inspection. + waitFor(t, func() bool { + entries, err := os.ReadDir(f.staging.Root()) + + return err == nil && len(entries) == 0 + }, "the failed attempt's staging was never released") +} + +// Falling back must not lower the bar. A second choice outside the +// user's guardrails is not a choice auto-pick may make, first or second. +func TestManagerFallbackRespectsTheGuardrails(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + f.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 50}) + + bad := fakeWithAlbum(1, "offline-peer", ".flac") + bad.GrabErr = errPeerOffline + bad.Candidates[0].TotalSize = 40 << 20 + + huge := fakeWithAlbum(2, "oversized", ".flac") + huge.Candidates[0].TotalSize = 900 << 20 + + f.manager.installProvider(Config{ID: 1, Priority: 90}, bad) + f.manager.installProvider(Config{ID: 2, Priority: 10}, huge) + + dl := fourTrackDownload() + + if _, err := f.manager.Start(context.Background(), dl); err != nil { + t.Fatalf("Start: %v", err) + } + + waitForDownloadState(t, f.store, dl.ID, StateFailed) + + if huge.GrabCalls != 0 { + t.Errorf("fell back to a candidate over the size ceiling") + } +} + +// A copy the user picked by hand is the copy they asked for. Quietly +// substituting another is a decision they did not make. +func TestManagerPickDoesNotFallBack(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + + bad := fakeWithAlbum(1, "offline-peer", ".flac") + bad.GrabErr = errPeerOffline + good := fakeWithAlbum(2, "online-peer", ".flac") + + f.manager.installProvider(Config{ID: 1, Priority: 90}, bad) + f.manager.installProvider(Config{ID: 2, Priority: 10}, good) + + // A ceiling below both copies parks the result set for the user. + f.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 1}) + + bad.Candidates[0].TotalSize = 30 << 20 + good.Candidates[0].TotalSize = 30 << 20 + + dl := fourTrackDownload() + + if _, err := f.manager.Start(context.Background(), dl); err != nil { + t.Fatalf("Start: %v", err) + } + + if err := f.manager.Pick( + context.Background(), dl.ID, "offline-peer-cand", + ); err != nil { + t.Fatalf("Pick: %v", err) + } + + waitForDownloadState(t, f.store, dl.ID, StateFailed) + + if good.GrabCalls != 0 { + t.Errorf("a hand-picked grab fell back to another candidate") + } +} + +// Fallback is for surviving an offline peer or two, not for walking a +// forty-peer list for six hours. +func TestManagerFallbackIsBounded(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + + var providers []*FakeProvider + + for i := int64(1); i <= maxGrabAttempts+2; i++ { + p := fakeWithAlbum(i, "peer-"+itoa(int(i)), ".flac") + p.GrabErr = errPeerOffline + + f.manager.installProvider(Config{ID: i, Priority: 50}, p) + providers = append(providers, p) + } + + dl := fourTrackDownload() + + if _, err := f.manager.Start(context.Background(), dl); err != nil { + t.Fatalf("Start: %v", err) + } + + waitForDownloadState(t, f.store, dl.ID, StateFailed) + + grabs := 0 + for _, p := range providers { + grabs += p.GrabCalls + } + + if grabs != maxGrabAttempts { + t.Errorf("grabs = %d, want %d", grabs, maxGrabAttempts) + } +} + +// The veto judges the best candidate *inside* the guardrails, so the +// grab has to take that one — not the overall best, which may be the +// very copy the user said not to take unattended. +func TestManagerAutoPickTakesTheBestEligibleCandidate(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + f.manager.SetPreferences(AutoDownloadPrefs{MaxSizeMB: 50}) + + huge := fakeWithAlbum(1, "oversized", ".flac") + huge.Candidates[0].TotalSize = 900 << 20 + + fits := fakeWithAlbum(2, "fits", ".flac") + fits.Candidates[0].TotalSize = 40 << 20 + + f.manager.installProvider(Config{ID: 1, Priority: 90}, huge) + f.manager.installProvider(Config{ID: 2, Priority: 10}, fits) + + dl := fourTrackDownload() + + ranked, err := f.manager.Start(context.Background(), dl) + if err != nil { + t.Fatalf("Start: %v", err) + } + + if ranked[0].ID != "oversized-cand" { + t.Fatalf("fixture: best overall is %s, want the oversized copy", ranked[0].ID) + } + + waitForDownloadState(t, f.store, dl.ID, StateComplete) + + if huge.GrabCalls != 0 || fits.GrabCalls != 1 { + t.Errorf( + "grabs: oversized=%d fits=%d, want 0 and 1", + huge.GrabCalls, fits.GrabCalls, + ) + } +} + +// On Soulseek a failure is the peer's, so every folder that peer offered +// goes with it. Elsewhere a failure is the release's, and one indexer's +// other releases are still worth trying. +func TestRuledOutBy(t *testing.T) { + t.Parallel() + + failed := []Candidate{ + {ID: "slskd:alice:Album", Kind: KindSlskd, ProviderID: 1, Origin: "alice"}, + {ID: "tracker-1", Kind: KindProwlarr, ProviderID: 2, Origin: "indexer"}, + } + + cases := []struct { + name string + c Candidate + want bool + }{ + { + name: "the same candidate", + c: Candidate{ID: "tracker-1", Kind: KindProwlarr, ProviderID: 2, Origin: "indexer"}, + want: true, + }, + { + name: "another folder from a failed peer", + c: Candidate{ + ID: "slskd:alice:Album (2)", + Kind: KindSlskd, + ProviderID: 1, + Origin: "alice", + }, + want: true, + }, + { + name: "another peer", + c: Candidate{ID: "slskd:bob:Album", Kind: KindSlskd, ProviderID: 1, Origin: "bob"}, + want: false, + }, + { + name: "another release from the same indexer", + c: Candidate{ID: "tracker-2", Kind: KindProwlarr, ProviderID: 2, Origin: "indexer"}, + want: false, + }, + { + name: "a peer of the same name on a different daemon", + c: Candidate{ + ID: "slskd:alice:Album", + Kind: KindSlskd, + ProviderID: 3, + Origin: "alice", + }, + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + if got := ruledOutBy(tc.c, failed); got != tc.want { + t.Errorf("ruledOutBy = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/backend/download/manager.go b/backend/download/manager.go index 2b284b5..2af26c4 100644 --- a/backend/download/manager.go +++ b/backend/download/manager.go @@ -577,12 +577,12 @@ func (m *Manager) Start( )) } - if m.AutoPickable(dl, ranked) { + if pick, ok := autoPick(dl, ranked, m.preferences()); ok { if job != nil { job.Logf(jobs.LevelInfo, "Auto-selected best candidate") } - go m.grab(context.WithoutCancel(ctx), dl, ranked[0], job) + go m.grab(context.WithoutCancel(ctx), dl, pick, job, true) return ranked, nil } @@ -622,6 +622,13 @@ func (m *Manager) Attempt( return false, veto, nil } + pick, ok := autoPick(dl, ranked, m.preferences()) + if !ok { + // Unreachable while autoPick and AutoPickVeto agree; kept so a + // future divergence refuses rather than grabbing blind. + return false, "no candidate clears the auto-download bar", nil + } + if err := m.store.CreateDownload(ctx, dl); err != nil { return false, "", err } @@ -643,7 +650,7 @@ func (m *Manager) Attempt( )) } - go m.grab(context.WithoutCancel(ctx), dl, ranked[0], job) + go m.grab(context.WithoutCancel(ctx), dl, pick, job, true) return true, "", nil } @@ -678,7 +685,7 @@ func (m *Manager) Pick( job := m.startJob(dl) - go m.grab(context.WithoutCancel(ctx), dl, *chosen, job) + go m.grab(context.WithoutCancel(ctx), dl, *chosen, job, false) return nil } @@ -702,13 +709,20 @@ func (m *Manager) Cancel(ctx context.Context, downloadID string) error { return nil } -// grab drives one candidate all the way to the library. It runs on its +// grab drives one request all the way to the library. It runs on its // own goroutine and owns the job from here on. +// +// When fallback is set and a candidate's transfer fails, the next +// candidate that auto-pick would itself have accepted is tried in its +// place (see nextCandidate). It is set for the two unattended routes +// and not for a candidate the user picked by hand: they chose that copy, +// and quietly substituting another is a decision they did not make. func (m *Manager) grab( ctx context.Context, dl Download, c Candidate, job *jobs.Handle, + fallback bool, ) { ctx, cancel := context.WithTimeout(ctx, grabTimeout) defer cancel() @@ -723,6 +737,82 @@ func (m *Manager) grab( m.actMu.Unlock() }() + var failed []Candidate + + for { + out := m.attemptGrab(ctx, dl, c, job) + if out.err == nil { + m.finishGrab(ctx, dl, out.item, out.imported, job) + + return + } + + failed = append(failed, c) + + next, ok := m.nextCandidate(ctx, dl, failed, out, fallback) + if !ok { + m.failDownload(ctx, job, dl.ID, out.err) + + return + } + + m.logger.Info( + "download candidate failed; trying the next", + "download", dl.ID, + "failed", c.ID, + "next", next.ID, + "error", out.err, + ) + + if job != nil { + job.Logf(jobs.LevelWarn, fmt.Sprintf( + "%s failed (%v); trying %s instead", + describeCandidate(c), out.err, describeCandidate(next), + )) + } + + // The failed attempt's staging holds at most a partial folder + // nobody is going to import, and the next attempt reserves its + // own. Only the final failure keeps its staging for inspection. + if out.item.StagingDir != "" { + if err := m.staging.Release(out.item.StagingDir); err != nil { + m.logger.Warn("could not release staging dir", "error", err) + } + } + + c = next + } +} + +// maxGrabAttempts bounds how many candidates one request will try. A +// popular album can have dozens of peers; the point of falling back is +// to survive the ordinary one or two that are offline, not to walk the +// whole list for six hours. +const maxGrabAttempts = 3 + +// grabOutcome is how one candidate's attempt ended. +type grabOutcome struct { + item DownloadItem + imported ImportResult + err error + + // retryable reports whether another candidate might succeed where + // this one failed: the transfer failed, or delivered too little of + // the album. Anything else — no staging space, no library root, a + // tag write failing — would fail the next candidate identically. + retryable bool +} + +// attemptGrab takes one candidate through transfer and import. It +// records the item's own failure, but not the download's: whether the +// download has failed is the caller's decision, since another candidate +// may yet succeed. +func (m *Manager) attemptGrab( + ctx context.Context, + dl Download, + c Candidate, + job *jobs.Handle, +) 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 // global one. A delegate takes no slot at all: the transfer is @@ -731,9 +821,7 @@ func (m *Manager) grab( // work against our budget. plan, err := m.planTransfer(dl, c) if err != nil { - m.failDownload(ctx, job, dl.ID, err) - - return + return grabOutcome{err: err} } if !plan.delegated() { @@ -743,9 +831,7 @@ func (m *Manager) grab( case provSem <- struct{}{}: defer func() { <-provSem }() case <-ctx.Done(): - m.failDownload(ctx, job, dl.ID, ctx.Err()) - - return + return grabOutcome{err: ctx.Err()} } globalSem := m.globalSem() @@ -754,9 +840,7 @@ func (m *Manager) grab( case globalSem <- struct{}{}: defer func() { <-globalSem }() case <-ctx.Done(): - m.failDownload(ctx, job, dl.ID, ctx.Err()) - - return + return grabOutcome{err: ctx.Err()} } } @@ -771,24 +855,30 @@ func (m *Manager) grab( dir, err := m.staging.Reserve(item.ID) if err != nil { - m.failDownload(ctx, job, dl.ID, err) - - return + return grabOutcome{err: err} } item.StagingDir = dir if err := m.store.CreateItem(ctx, item); err != nil { - m.failDownload(ctx, job, dl.ID, err) + return grabOutcome{item: item, err: err} + } - return + fail := func(err error, retryable bool) grabOutcome { + if serr := m.store.SetItemState( + ctx, item.ID, StateFailed, err.Error(), + ); serr != nil { + m.logger.Warn("could not record item failure", "error", serr) + } + + return grabOutcome{item: item, err: err, retryable: retryable} } result, err := m.transfer(ctx, dl, item, plan, job) if err != nil { - m.failItem(ctx, job, item, dl.ID, err) - - return + // A delegate's failure is the external manager's verdict on the + // whole request, not on one copy of it. + return fail(err, !plan.delegated()) } m.setStates(ctx, dl.ID, item.ID, StateImporting) @@ -798,42 +888,116 @@ func (m *Manager) grab( job.SetStages(importStages(2)) } - var imported ImportResult - if result.Delegated { // The external manager already placed and tagged these files in // its own library. Moving them out from under a system that is // still managing them would be worse than useless, so the files // are recorded where they are and the library scan picks them // up in place. - imported = ImportResult{Paths: result.Files} - if job != nil { job.Logf(jobs.LevelInfo, fmt.Sprintf( "External manager imported %d files; recording them in place", len(result.Files), )) } - } else { - opts := m.importOptions() - opts.WriteTags = true - opts.LibraryRoot, err = m.library.LibraryPath(dl.LibraryID) - if err != nil { - m.failItem(ctx, job, item, dl.ID, - fmt.Errorf("resolve library root: %w", err)) - - return - } - - imported, err = m.importer.Import(ctx, dl, result, opts) - if err != nil { - m.failItem(ctx, job, item, dl.ID, err) - - return + return grabOutcome{ + item: item, + imported: ImportResult{Paths: result.Files}, } } + opts := m.importOptions() + opts.WriteTags = true + + opts.LibraryRoot, err = m.library.LibraryPath(dl.LibraryID) + if err != nil { + return fail(fmt.Errorf("resolve library root: %w", err), false) + } + + imported, err := m.importer.Import(ctx, dl, result, opts) + if err != nil { + return fail(err, errors.Is(err, ErrTooIncomplete)) + } + + return grabOutcome{item: item, imported: imported} +} + +// nextCandidate picks the candidate to try after the ones in failed. +// +// It only ever offers a candidate auto-pick would have taken on its own +// (autoAcceptable), so falling back cannot lower the bar an unattended +// download is held to: the second choice has to clear the same gates +// the first did. +// +// On Soulseek a failure belongs to the *peer* — offline, refusing, or +// holding us in a queue — so every folder that peer offered is skipped +// with it. Elsewhere a failure belongs to the release, and only that +// candidate is. +func (m *Manager) nextCandidate( + ctx context.Context, + dl Download, + failed []Candidate, + out grabOutcome, + fallback bool, +) (Candidate, bool) { + if !fallback || !out.retryable || ctx.Err() != nil || + len(failed) >= maxGrabAttempts { + return Candidate{}, false + } + + m.resMu.RLock() + ranked := m.results[dl.ID] + m.resMu.RUnlock() + + prefs := m.preferences() + + for _, c := range ranked { + if ruledOutBy(c, failed) || !autoAcceptable(dl, c, prefs) { + continue + } + + return c, true + } + + return Candidate{}, false +} + +// ruledOutBy reports whether a failure among failed also rules out c. +func ruledOutBy(c Candidate, failed []Candidate) bool { + for _, f := range failed { + if c.ID == f.ID && c.ProviderID == f.ProviderID { + return true + } + + if c.Kind == KindSlskd && f.Kind == KindSlskd && + c.ProviderID == f.ProviderID && c.Origin != "" && + c.Origin == f.Origin { + return true + } + } + + return false +} + +// describeCandidate names a candidate for the job log. +func describeCandidate(c Candidate) string { + if c.Origin != "" { + return fmt.Sprintf("%q from %s", c.Title, c.Origin) + } + + return fmt.Sprintf("%q", c.Title) +} + +// finishGrab records a successful import and retires what the request +// was holding. +func (m *Manager) finishGrab( + ctx context.Context, + dl Download, + item DownloadItem, + imported ImportResult, + job *jobs.Handle, +) { if err := m.store.SetItemImported( ctx, item.ID, imported.Paths, ); err != nil { @@ -1177,23 +1341,6 @@ func (m *Manager) failDownload( } } -// failItem records an item-level failure and fails its download. -func (m *Manager) failItem( - ctx context.Context, - job *jobs.Handle, - item DownloadItem, - downloadID string, - err error, -) { - if serr := m.store.SetItemState( - ctx, item.ID, StateFailed, err.Error(), - ); serr != nil { - m.logger.Warn("could not record item failure", "error", serr) - } - - m.failDownload(ctx, job, downloadID, err) -} - // startJob registers the request in the background jobs panel. func (m *Manager) startJob(dl Download) *jobs.Handle { if m.jobsReg == nil { diff --git a/backend/download/rank.go b/backend/download/rank.go index ecaa53c..731f9a2 100644 --- a/backend/download/rank.go +++ b/backend/download/rank.go @@ -735,6 +735,45 @@ func AutoPickVeto( return "" } +// autoAcceptable reports whether auto-pick may take this one candidate +// without asking: the request is anchored to a tracklist, and the +// candidate is inside the user's guardrails and clears the match and +// quality bars. It is AutoPickVeto's test applied to a single +// candidate, which is what falling back to a second choice needs. +func autoAcceptable(dl Download, c Candidate, prefs AutoDownloadPrefs) bool { + return dl.Anchored() && + len(dl.Expected) > 0 && + prefs.eligible(c, dl.runtimeMillis()) && + c.Match.Overall >= minMatch && + c.Quality.Overall >= minQuality +} + +// autoPick returns the candidate auto-pick takes: the best-ranked one +// it may take at all. +// +// That is not `ranked[0]`. AutoPickVeto judges the best candidate +// *inside* the guardrails, so when the overall best is outside them — +// over the size ceiling, say — the veto passes on the strength of the +// second, and grabbing the first would download exactly the copy the +// user said not to take unattended. +func autoPick( + dl Download, + ranked []Candidate, + prefs AutoDownloadPrefs, +) (Candidate, bool) { + if AutoPickVeto(dl, ranked, prefs) != "" { + return Candidate{}, false + } + + for _, c := range ranked { + if autoAcceptable(dl, c, prefs) { + return c, true + } + } + + return Candidate{}, false +} + // mergeMatched copies MatchedTo assignments from the audio-only slice // back onto the full file list. func mergeMatched(all, matched []CandidateFile) []CandidateFile {