From 760021ea5aebf483dad26144e7c9828ce0f0e3e1 Mon Sep 17 00:00:00 2001 From: Logan Date: Tue, 18 Aug 2026 11:26:35 -0400 Subject: [PATCH] fix(downloads): stop searching a list there is nothing to search with Every pass attempted every request, each came back "no download clients are enabled", and RecordAttempt wrote that down as an attempt and put a retry on the clock -- so a wanted list built deliberately without a client accrued failures and announced "next check in 6 hours" about a check that cannot happen. Wanting something with no way to fetch it is supported. Being told it is being looked for is a lie, and the row says what is true instead. Everything above the attempt still runs: an artist subscription still expands, and a request satisfied by some other route -- ripped, bought, copied in -- is still retired. Neither needs a provider. TestReconcileRespectsBatchSize now installs a client that finds nothing, because a batch size is about how many requests one pass searches for and that only means something when there is something to search with. Refs #37 --- backend/download/reconcile.go | 32 +++++-- backend/download/reconcile_test.go | 78 ++++++++++++++++ .../downloads-view/downloads-view.ts | 16 +++- .../components/downloads-no-client.test.ts | 90 +++++++++++++++++++ 4 files changed, 208 insertions(+), 8 deletions(-) create mode 100644 frontend/test/components/downloads-no-client.test.ts diff --git a/backend/download/reconcile.go b/backend/download/reconcile.go index 1e209c3..1984171 100644 --- a/backend/download/reconcile.go +++ b/backend/download/reconcile.go @@ -310,15 +310,35 @@ func (r *Reconciler) run(ctx context.Context, force bool) (Summary, error) { summary.Synced = r.syncExternalLists(ctx) - attempted, started, err := r.attemptDue(ctx, force) - if err != nil { - return summary, err + // Nothing is searched for when there is nothing to search with, and + // the point is what that *does not* do to the list. + // + // Attempting anyway is not merely wasted work: every request comes + // back "no download clients are enabled", which RecordAttempt writes + // down as an attempt and schedules a retry for -- so a user who has + // deliberately built a wanted list with no client watched their + // requests accrue failures and announce "next check in 6 hours" + // about a check that cannot happen. Wanting something without a way + // to fetch it is a supported thing to do; being told it is being + // looked for is a lie. + // + // Everything above this line still runs: an artist subscription + // still expands, and a request the user satisfied by some other + // route -- ripped, bought, copied in -- is still retired, because + // neither needs a provider. + summary.NoProviders = len(r.manager.enabledProviders()) == 0 + + if !summary.NoProviders { + attempted, started, err := r.attemptDue(ctx, force) + if err != nil { + return summary, err + } + + summary.Attempted = attempted + summary.Started = started } - summary.Attempted = attempted - summary.Started = started summary.Waiting = r.countWaiting(ctx) - summary.NoProviders = len(r.manager.enabledProviders()) == 0 r.logger.Info( "reconciled request list", diff --git a/backend/download/reconcile_test.go b/backend/download/reconcile_test.go index 23eef8c..114ff4b 100644 --- a/backend/download/reconcile_test.go +++ b/backend/download/reconcile_test.go @@ -453,6 +453,15 @@ func TestReconcileRespectsBatchSize(t *testing.T) { f := newReconcileFixture(t) ctx := context.Background() + // A client that searches and finds nothing. The batch size is about + // how many requests one pass *searches for*, which only means + // anything when there is something to search with -- a pass with no + // provider now attempts nothing at all, deliberately. + f.manager.installProvider( + Config{ID: 1, Priority: 50}, + NewFakeProvider(1, "finds-nothing", Caps{CanSearch: true}), + ) + f.reconciler.SetBatch(2) for _, mbid := range []string{"rg-1", "rg-2", "rg-3", "rg-4"} { @@ -593,3 +602,72 @@ func TestSummaryReportsNoProviders(t *testing.T) { t.Error("summary did not report that no download client is enabled") } } + +// ...and it does not search, which is the part the user sees. +// +// Attempting with no provider fails every request with "no download +// clients are enabled", and RecordAttempt writes that down as an +// attempt and schedules a retry -- so a wanted list built deliberately +// without a client accrued failures and announced "next check in 6 +// hours" about a check that cannot happen. Wanting something with no +// way to fetch it is supported; being told it is being looked for is +// a lie. +func TestNoProvidersMeansNoAttempt(t *testing.T) { + t.Parallel() + + f := newReconcileFixture(t) + ctx := context.Background() + + id, err := f.store.AddRequest(ctx, Request{ + MBID: "rg-1", + Entity: EntityReleaseGroup, + LibraryID: 1, + Title: "OK Computer", + }) + if err != nil { + t.Fatalf("AddRequest: %v", err) + } + + f.catalog.tracklists["rg-1"] = fourTrackDownload().Expected + + summary, err := f.reconciler.RunNow(ctx) + if err != nil { + t.Fatalf("RunNow: %v", err) + } + + if summary.Attempted != 0 { + t.Errorf("attempted %d requests with no client to search with, want 0", + summary.Attempted) + } + + // The list still knows what is on it: "nothing happened" has to be + // reportable as "nothing was searched for, of the one thing you + // want" rather than as silence. + if summary.Waiting != 1 { + t.Errorf("summary reported %d waiting, want 1", summary.Waiting) + } + + req, err := f.store.GetRequest(ctx, id) + if err != nil { + t.Fatalf("GetRequest: %v", err) + } + + if req.Attempts != 0 { + t.Errorf("attempts = %d, want 0: a pass that could not search did not", + req.Attempts) + } + + if req.LastError != "" { + t.Errorf("lastError = %q, want empty: the request did not fail, it "+ + "was never tried", req.LastError) + } + + // A new request is due immediately (next_try_at is set to now on + // insert), so the fault is not the presence of a time -- it is a + // time pushed into the future by a failed attempt, which is what the + // UI renders as "next check in 6 hours". + if req.NextTryAt.After(time.Now().Add(time.Minute)) { + t.Errorf("next try scheduled for %v: a check that cannot happen was "+ + "put on the clock", req.NextTryAt) + } +} diff --git a/frontend/src/components/downloads-view/downloads-view.ts b/frontend/src/components/downloads-view/downloads-view.ts index 8047e34..43441eb 100644 --- a/frontend/src/components/downloads-view/downloads-view.ts +++ b/frontend/src/components/downloads-view/downloads-view.ts @@ -512,7 +512,9 @@ export class DownloadsView extends ViewLifecycleMixin(LitElement) { ${request.artist ? `${request.artist} — ` : ''}${request.title || request.mbid} -
${requestDetail(request, this.nowMs)}
+
+ ${requestDetail(request, this.nowMs, this.canDownload)} +
${request.state === 'satisfied' @@ -706,10 +708,20 @@ export class DownloadsView extends ViewLifecycleMixin(LitElement) { * looked for rather than as an error, because that is what it is — the * retry is already scheduled and there is nothing for the user to do. */ -function requestDetail(request: Request, nowMs: number): string { +function requestDetail( + request: Request, + nowMs: number, + canDownload: boolean, +): string { if (request.state === 'satisfied') return 'In your library'; if (request.state === 'paused') return 'Paused — not being looked for'; + // With no client there is no search and no retry clock — the + // backend stopped scheduling one — so a row must not imply either. + // "Queued" and "next check in 6 hours" are both promises nothing is + // in a position to keep. + if (!canDownload) return 'On your list — no download client to search with'; + if (request.attempts === 0) return 'Queued — not searched for yet'; const tries = `Searched ${request.attempts} time${request.attempts === 1 ? '' : 's'}`; diff --git a/frontend/test/components/downloads-no-client.test.ts b/frontend/test/components/downloads-no-client.test.ts new file mode 100644 index 0000000..5f88275 --- /dev/null +++ b/frontend/test/components/downloads-no-client.test.ts @@ -0,0 +1,90 @@ +/** + * What a wanted list says when there is nothing to search with. + * + * Wanting something without a download client is a supported thing to + * do — the list is kept, and it starts moving when a client is added. + * What was not supported was the app *claiming to be looking*: every + * pass attempted each request, failed it with "no download clients are + * enabled", recorded that as an attempt and scheduled a retry, so a row + * read "Searched 3 times, no download clients are enabled · next check + * in 6 hours" about a check that could not happen. + * + * The backend half is `TestNoProvidersMeansNoAttempt`. This is the row. + */ +import { describe, expect, it, beforeEach } from 'vitest'; +import type { LitElement } from 'lit'; + +import '@components/downloads-view/downloads-view'; +import { stub, emit, flush, resetHarness } from '@test/support/harness'; +import { Events } from '../../src/events'; +import { fixture, shadowAll } from '@test/support/render'; + +/** A request that has been tried and is waiting on a retry — the shape + * a list with a client in it produces. */ +const WAITING = { + id: 1, + mbid: 'rg-1', + entity: 'release-group', + libraryId: 1, + artist: 'Aurora Fields', + title: 'Glass Harbour', + state: 'wanted', + attempts: 3, + lastError: 'no source has it yet', + nextTryAt: new Date(Date.now() + 6 * 3600_000).toISOString(), +}; + +const PROVIDER = { + id: 1, + kind: 'slskd', + name: 'Sound', + enabled: true, + priority: 50, +}; + +/** + * The download store is a singleton whose `init()` runs once per + * session, so a second mount does not re-read the provider list. The + * event is how the app itself learns a client was added, and is what + * makes this test independent of which case ran first. + */ +async function view(providers: unknown[]): Promise { + stub('download.Service.ListProviders', providers); + + const el = await fixture('downloads-view'); + + emit(Events.DownloadProvidersChanged); + await flush(); + await el.updateComplete; + + return el; +} + +const details = (el: LitElement) => + shadowAll(el, '.detail').map((d) => d.textContent!.trim()); + +describe('a request row with no download client', () => { + beforeEach(() => { + resetHarness(); + stub('download.Service.ProviderKinds', []); + stub('download.Service.ListProviders', []); + stub('download.Service.ListDownloads', []); + stub('download.Service.ListRequests', [WAITING]); + }); + + it('does not promise a check that cannot happen', async () => { + const el = await view([]); + + expect(details(el)).toHaveLength(1); + expect(details(el)[0]).toBe( + 'On your list — no download client to search with', + ); + expect(details(el)[0]).not.toMatch(/next check/); + }); + + it('reports the retry schedule again once a client exists', async () => { + const el = await view([PROVIDER]); + + expect(details(el)[0]).toMatch(/next check/); + }); +});