From 62bb40fc4d94c4615802610acc387415cbe5f52e Mon Sep 17 00:00:00 2001 From: Logan Date: Tue, 11 Aug 2026 01:15:23 -0400 Subject: [PATCH] fix(download): make "check now" actually check now, and say what it did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The button ran a normal reconcile pass, which honours each request's retry backoff — so a request searched an hour ago was not due, nothing was searched, and the button looked broken. The backoff is a promise to the providers, not to the user: a person pressing "check now" *is* the schedule, so a user-initiated pass ignores it and the loop still does not. "Nothing happened" also needed a reason. Summary now carries how many requests are still being looked for and whether any download client is enabled at all, which is the one cause of silence the user can fix — and the requests tab says so above the list rather than leaving an inert list to be interpreted. The rest is the retry schedule finally being admitted to: rows show when the next check falls due, "Looking for" explains that a request sitting there is waiting rather than failing, and the page header says how often the list is worked. --- backend/database/sql/queries/download.sql | 10 ++ backend/database/sql/sqlcgen/download.sql.go | 52 ++++++ backend/download/reconcile.go | 68 +++++++- backend/download/reconcile_test.go | 98 ++++++++++++ backend/download/requeststore.go | 17 ++ backend/download/service.go | 2 +- .../downloads-view/downloads-view.ts | 149 ++++++++++++++++-- frontend/wailsjs/go/models.ts | 4 + 8 files changed, 377 insertions(+), 23 deletions(-) diff --git a/backend/database/sql/queries/download.sql b/backend/database/sql/queries/download.sql index 22df883..2cb4405 100644 --- a/backend/database/sql/queries/download.sql +++ b/backend/database/sql/queries/download.sql @@ -176,6 +176,16 @@ WHERE state = 'wanted' ORDER BY attempts, created_at LIMIT ?; +-- name: ListWantedDownloadRequests :many +-- The same set ignoring the backoff, for a pass the user asked for by +-- hand: "check now" that respected a six-hour retry schedule looked +-- like a button that did nothing. +SELECT * FROM download_requests +WHERE state = 'wanted' + AND entity <> 'artist' +ORDER BY attempts, created_at +LIMIT ?; + -- name: ListChildDownloadRequests :many SELECT * FROM download_requests WHERE parent_id = ? ORDER BY id; diff --git a/backend/database/sql/sqlcgen/download.sql.go b/backend/database/sql/sqlcgen/download.sql.go index 2601074..bb9da3a 100644 --- a/backend/database/sql/sqlcgen/download.sql.go +++ b/backend/database/sql/sqlcgen/download.sql.go @@ -739,6 +739,58 @@ func (q *Queries) ListLiveDownloads(ctx context.Context) ([]DownloadDownload, er return items, nil } +const listWantedDownloadRequests = `-- name: ListWantedDownloadRequests :many +SELECT id, mbid, entity, library_id, artist, title, scope, secondary, state, parent_id, attempts, last_error, last_tried_at, next_try_at, external_ids, created_at, updated_at FROM download_requests +WHERE state = 'wanted' + AND entity <> 'artist' +ORDER BY attempts, created_at +LIMIT ? +` + +// The same set ignoring the backoff, for a pass the user asked for by +// hand: "check now" that respected a six-hour retry schedule looked +// like a button that did nothing. +func (q *Queries) ListWantedDownloadRequests(ctx context.Context, limit int64) ([]DownloadRequest, error) { + rows, err := q.db.QueryContext(ctx, listWantedDownloadRequests, limit) + if err != nil { + return nil, err + } + defer rows.Close() + var items []DownloadRequest + for rows.Next() { + var i DownloadRequest + if err := rows.Scan( + &i.ID, + &i.Mbid, + &i.Entity, + &i.LibraryID, + &i.Artist, + &i.Title, + &i.Scope, + &i.Secondary, + &i.State, + &i.ParentID, + &i.Attempts, + &i.LastError, + &i.LastTriedAt, + &i.NextTryAt, + &i.ExternalIds, + &i.CreatedAt, + &i.UpdatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const recordDownloadRequestAttempt = `-- name: RecordDownloadRequestAttempt :exec UPDATE download_requests SET attempts = attempts + 1, diff --git a/backend/download/reconcile.go b/backend/download/reconcile.go index 79c2b9a..1e209c3 100644 --- a/backend/download/reconcile.go +++ b/backend/download/reconcile.go @@ -251,6 +251,16 @@ type Summary struct { // Synced is how many requests were pushed to an external list. Synced int `json:"synced"` + + // Waiting is how many requests are on the list and still being + // looked for. A pass that did nothing is the normal case, and the + // UI can only say so honestly if it knows the list was not empty. + Waiting int `json:"waiting"` + + // NoProviders reports that nothing could be searched because no + // download client is enabled — the one "nothing happened" the user + // can actually fix. + NoProviders bool `json:"noProviders"` } // changed reports whether the pass altered anything worth refreshing @@ -259,9 +269,22 @@ func (s Summary) changed() bool { return s.Expanded > 0 || s.Satisfied > 0 || s.Started > 0 } -// RunOnce works the request list once. It is safe to call directly, and -// the "search now" button does. +// RunOnce works the request list once, honouring each request's +// backoff. This is what the loop calls. func (r *Reconciler) RunOnce(ctx context.Context) (Summary, error) { + return r.run(ctx, false) +} + +// RunNow works the request list ignoring backoff. This is what the +// "check now" button calls: a scheduled retry is a promise to the +// provider, not to the user, and a person who presses a button expects +// their list to actually be searched rather than to be told it is not +// due yet. +func (r *Reconciler) RunNow(ctx context.Context) (Summary, error) { + return r.run(ctx, true) +} + +func (r *Reconciler) run(ctx context.Context, force bool) (Summary, error) { r.runMu.Lock() defer r.runMu.Unlock() @@ -287,13 +310,15 @@ func (r *Reconciler) RunOnce(ctx context.Context) (Summary, error) { summary.Synced = r.syncExternalLists(ctx) - attempted, started, err := r.attemptDue(ctx) + attempted, started, err := r.attemptDue(ctx, force) if err != nil { return summary, err } summary.Attempted = attempted summary.Started = started + summary.Waiting = r.countWaiting(ctx) + summary.NoProviders = len(r.manager.enabledProviders()) == 0 r.logger.Info( "reconciled request list", @@ -506,10 +531,19 @@ func (r *Reconciler) retireOwned(ctx context.Context) (int, error) { // Attempting downloads // --------------------------------------------------------------------------- -// attemptDue searches for a bounded batch of due requests and grabs the -// ones with a clear winner. -func (r *Reconciler) attemptDue(ctx context.Context) (attempted, started int, err error) { - due, err := r.store.ListDueRequests(ctx, r.batch) +// attemptDue searches for a bounded batch of requests and grabs the +// ones with a clear winner. force takes requests whose backoff has not +// elapsed as well. +func (r *Reconciler) attemptDue( + ctx context.Context, + force bool, +) (attempted, started int, err error) { + list := r.store.ListDueRequests + if force { + list = r.store.ListWantedRequests + } + + due, err := list(ctx, r.batch) if err != nil { return 0, 0, err } @@ -615,6 +649,26 @@ func (r *Reconciler) attempt(ctx context.Context, req Request) (bool, string) { return started, reason } +// countWaiting reports how many non-artist requests are still being +// looked for, so "nothing happened" can be reported as "nothing new +// for the twelve things on your list" rather than as silence. +func (r *Reconciler) countWaiting(ctx context.Context) int { + requests, err := r.store.ListRequests(ctx) + if err != nil { + return 0 + } + + waiting := 0 + + for _, req := range requests { + if req.State == RequestStateWanted && !req.Entity.Expands() { + waiting++ + } + } + + return waiting +} + // --------------------------------------------------------------------------- // External list sync // --------------------------------------------------------------------------- diff --git a/backend/download/reconcile_test.go b/backend/download/reconcile_test.go index e87c22a..23eef8c 100644 --- a/backend/download/reconcile_test.go +++ b/backend/download/reconcile_test.go @@ -495,3 +495,101 @@ func waitFor(t *testing.T, cond func() bool, msg string) { t.Fatal(msg) } + +// "Check now" is the user overriding the retry schedule, so it must +// search a request whose backoff has not elapsed. The scheduled pass +// must not: the backoff exists to keep a fruitless search off the +// providers, and a loop that ignored it would hammer them. +func TestRunNowIgnoresBackoffAndRunOnceDoesNot(t *testing.T) { + t.Parallel() + + f := newReconcileFixture(t) + ctx := context.Background() + + provider := NewFakeProvider(1, "weak", Caps{CanSearch: true, CanTransport: true}) + provider.Candidates = []Candidate{candidateFor( + "weak-1", []string{"Something Else Entirely"}, ".mp3", 3_000_000, + )} + + f.manager.installProvider(Config{ID: 1, Priority: 50}, provider) + + id, err := f.store.AddRequest(ctx, Request{ + MBID: "rg-1", + Entity: EntityReleaseGroup, + LibraryID: 1, + Artist: "Radiohead", + Title: "OK Computer", + }) + if err != nil { + t.Fatalf("AddRequest: %v", err) + } + + f.catalog.tracklists["rg-1"] = fourTrackDownload().Expected + + // First pass: attempted, found nothing, backoff armed. + if _, err := f.reconciler.RunOnce(ctx); err != nil { + t.Fatalf("RunOnce: %v", err) + } + + scheduled, err := f.reconciler.RunOnce(ctx) + if err != nil { + t.Fatalf("RunOnce (second): %v", err) + } + + if scheduled.Attempted != 0 { + t.Errorf("scheduled pass attempted %d, want 0 while backed off", + scheduled.Attempted) + } + + forced, err := f.reconciler.RunNow(ctx) + if err != nil { + t.Fatalf("RunNow: %v", err) + } + + if forced.Attempted != 1 { + t.Errorf("forced pass attempted %d, want 1", forced.Attempted) + } + + if forced.Waiting != 1 { + t.Errorf("summary reported %d waiting, want 1 so the UI can say "+ + "what was searched", forced.Waiting) + } + + req, err := f.store.GetRequest(ctx, id) + if err != nil { + t.Fatalf("GetRequest: %v", err) + } + + if req.Attempts != 2 { + t.Errorf("attempts = %d, want 2 after a forced re-check", req.Attempts) + } +} + +// A pass with no providers says so, because "nothing happened" with no +// reason is the one outcome the user cannot act on. +func TestSummaryReportsNoProviders(t *testing.T) { + t.Parallel() + + f := newReconcileFixture(t) + ctx := context.Background() + + if _, err := f.store.AddRequest(ctx, Request{ + MBID: "rg-1", + Entity: EntityReleaseGroup, + LibraryID: 1, + Title: "OK Computer", + }); 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.NoProviders { + t.Error("summary did not report that no download client is enabled") + } +} diff --git a/backend/download/requeststore.go b/backend/download/requeststore.go index a61ad93..6763e92 100644 --- a/backend/download/requeststore.go +++ b/backend/download/requeststore.go @@ -152,6 +152,23 @@ func (s *Store) ListDueRequests(ctx context.Context, limit int) ([]Request, erro return requestRowsToRequests(rows), nil } +// ListWantedRequests returns downloadable requests regardless of their +// backoff, least-attempted first. Only a user-initiated pass uses +// this: the loop honours the schedule, a person pressing "check now" +// is the schedule. +func (s *Store) ListWantedRequests(ctx context.Context, limit int) ([]Request, error) { + if limit <= 0 { + limit = defaultDueBatch + } + + rows, err := s.db.ReadQueries.ListWantedDownloadRequests(ctx, int64(limit)) + if err != nil { + return nil, fmt.Errorf("list wanted download requests: %w", err) + } + + return requestRowsToRequests(rows), nil +} + // ListChildRequests returns the requests an artist subscription // produced. func (s *Store) ListChildRequests( diff --git a/backend/download/service.go b/backend/download/service.go index db2e04a..a350be8 100644 --- a/backend/download/service.go +++ b/backend/download/service.go @@ -609,7 +609,7 @@ func (s *Service) ReconcileRequests() (Summary, error) { ) } - summary, err := s.reconciler.RunOnce(context.Background()) + summary, err := s.reconciler.RunNow(context.Background()) if err != nil { return summary, err } diff --git a/frontend/src/components/downloads-view/downloads-view.ts b/frontend/src/components/downloads-view/downloads-view.ts index 971392d..7cfd315 100644 --- a/frontend/src/components/downloads-view/downloads-view.ts +++ b/frontend/src/components/downloads-view/downloads-view.ts @@ -33,6 +33,14 @@ export class DownloadsView extends LitElement { @state() private lastSummary: RequestSummary | null = null; + /** True when at least one download client is enabled. */ + @state() private canDownload = false; + + /** Ticks so "next check in …" ages while the page is open. */ + @state() private nowMs = Date.now(); + + private clockTimer?: ReturnType; + private unsubscribe: (() => void) | null = null; static override styles = [ @@ -167,6 +175,24 @@ export class DownloadsView extends LitElement { color: var(--yj-text-secondary, #b3b3b3); margin: 8px 0 0; } + + .notice { + display: flex; + align-items: center; + gap: 8px; + padding: 10px 12px; + margin-bottom: 12px; + border-radius: 6px; + font-size: 12px; + background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06)); + color: var(--yj-text-secondary, #b3b3b3); + } + + .section-hint { + margin: 0 0 8px; + font-size: 12px; + color: var(--yj-text-tertiary, #888); + } `, ]; @@ -176,12 +202,20 @@ export class DownloadsView extends LitElement { this.unsubscribe = downloadStore.subscribe(() => { this.requests = downloadStore.requests; this.downloads = downloadStore.downloads; + this.canDownload = downloadStore.available; }); void downloadStore.init().then(() => { this.requests = downloadStore.requests; this.downloads = downloadStore.downloads; + this.canDownload = downloadStore.available; }); + + // A "next check" that never moves reads as a stuck page, so the + // relative times re-render on their own. + this.clockTimer = setInterval(() => { + this.nowMs = Date.now(); + }, 30_000); } override disconnectedCallback(): void { @@ -189,6 +223,7 @@ export class DownloadsView extends LitElement { this.unsubscribe?.(); this.unsubscribe = null; + clearInterval(this.clockTimer); } override render() { @@ -201,10 +236,11 @@ export class DownloadsView extends LitElement { size="small" appearance="outlined" ?disabled=${this.checking} + title="Search every download client for everything on this list right now, instead of waiting for the next scheduled check" @click=${() => void this.checkNow()} > - ${this.checking ? 'Checking…' : 'Check now'} + ${this.checking ? 'Searching…' : 'Check now'} ` : nothing} @@ -213,7 +249,10 @@ export class DownloadsView extends LitElement {

Music you have requested, and the download attempts that have run for it. A request that cannot be found today stays - on the list and is looked for again later. + on the list and is looked for again later — roughly every + six hours at first, then less often the longer it goes + unfound. “Check now” skips that wait and searches + everything on the list immediately.

@@ -248,6 +287,7 @@ export class DownloadsView extends LitElement { const satisfied = this.requests.filter((r) => r.state === 'satisfied'); return html` + ${this.renderProviderNotice()} ${this.renderSummary()} ${satisfied.length > 0 ? html` @@ -269,7 +309,18 @@ export class DownloadsView extends LitElement { subscriptions, (r) => this.renderSubscription(r), )} - ${this.renderRequestSection('Looking for', wanted, (r) => this.renderRequest(r))} + ${wanted.length > 0 + ? html` +

Looking for

+

+ Requested, not found yet. Nothing is wrong — each + of these is searched again on the schedule below, + and moves to “Found” the moment it lands in your + library, however it got there. +

+ ${wanted.map((r) => this.renderRequest(r))} + ` + : nothing} ${this.renderRequestSection('Paused', paused, (r) => this.renderRequest(r))} ${this.renderRequestSection('Found', satisfied, (r) => this.renderRequest(r))} `; @@ -284,6 +335,26 @@ export class DownloadsView extends LitElement { `; } + /** + * A request list with no download client behind it is a list that + * can never move, and that is the single most likely reason “check + * now” appears to do nothing. Say so where the button is. + */ + private renderProviderNotice() { + if (this.canDownload) return nothing; + + return html` +
+ + + No download client is enabled, so nothing on this list + can be searched for. Requests are still kept — add a + client under Settings → Downloads and they start moving. + +
+ `; + } + private renderSummary() { if (!this.lastSummary) return nothing; @@ -293,14 +364,23 @@ export class DownloadsView extends LitElement { s.expanded > 0 ? `${s.expanded} new album${s.expanded === 1 ? '' : 's'} found` : '', s.satisfied > 0 ? `${s.satisfied} already owned` : '', s.started > 0 ? `${s.started} downloading` : '', - s.attempted > 0 ? `${s.attempted} searched for` : '', + s.attempted > 0 + ? `${s.attempted} searched, no clear match yet` + : '', ].filter(Boolean); - return html` -

- ${parts.length > 0 ? parts.join(' · ') : 'Nothing new this time.'} -

- `; + if (parts.length > 0) { + return html`

${parts.join(' · ')}

`; + } + + // "Nothing happened" needs a reason, or the button looks broken. + const idle = s.noProviders + ? 'Nothing was searched: no download client is enabled.' + : s.waiting > 0 + ? `Searched all ${s.waiting} request${s.waiting === 1 ? '' : 's'} — no source has anything new yet.` + : 'Nothing on the list to search for.'; + + return html`

${idle}

`; } private renderRequestSection( @@ -361,7 +441,7 @@ export class DownloadsView extends LitElement { ${request.artist ? `${request.artist} — ` : ''}${request.title || request.mbid}
-
${requestDetail(request)}
+
${requestDetail(request, this.nowMs)}
${request.state === 'satisfied' @@ -495,15 +575,54 @@ export class DownloadsView extends 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): string { +function requestDetail(request: Request, nowMs: number): string { if (request.state === 'satisfied') return 'In your library'; - if (request.state === 'paused') return 'Paused'; + if (request.state === 'paused') return 'Paused — not being looked for'; - if (request.attempts === 0) return 'Not looked for yet'; + if (request.attempts === 0) return 'Queued — not searched for yet'; - const reason = request.lastError ? ` — ${request.lastError}` : ''; + const tries = `Searched ${request.attempts} time${request.attempts === 1 ? '' : 's'}`; + const reason = request.lastError ? `, ${request.lastError}` : ''; + // Wails types a Go time.Time as an opaque class; over the wire it + // is the RFC 3339 string JSON marshalled it as. + const next = nextCheckPhrase( + request.nextTryAt as unknown as string | undefined, + nowMs, + ); - return `Looked for ${request.attempts} time${request.attempts === 1 ? '' : 's'}${reason}`; + return `${tries}${reason}${next}`; +} + +/** + * "Next check" as a phrase, because the retry schedule is the part of + * this feature nothing in the UI used to admit existed — a row that + * says only "searched 3 times" gives the user no way to tell a waiting + * request from an abandoned one. + */ +function nextCheckPhrase(nextTryAt: string | undefined, nowMs: number): string { + if (!nextTryAt) return ''; + + const due = new Date(nextTryAt).getTime(); + if (Number.isNaN(due)) return ''; + + const deltaMs = due - nowMs; + if (deltaMs <= 0) return ' · due for another search'; + + return ` · next check ${relativeFuture(deltaMs)}`; +} + +/** Coarse "in 3 hours" phrasing; minutes are noise on a 6-hour cycle. */ +function relativeFuture(ms: number): string { + const minutes = Math.round(ms / 60_000); + + if (minutes < 60) return `in ${Math.max(1, minutes)} min`; + + const hours = Math.round(minutes / 60); + if (hours < 48) return `in ${hours} hour${hours === 1 ? '' : 's'}`; + + const days = Math.round(hours / 24); + + return `in ${days} day${days === 1 ? '' : 's'}`; } declare global { diff --git a/frontend/wailsjs/go/models.ts b/frontend/wailsjs/go/models.ts index 9d35b18..163dd59 100755 --- a/frontend/wailsjs/go/models.ts +++ b/frontend/wailsjs/go/models.ts @@ -868,6 +868,8 @@ export namespace download { attempted: number; started: number; synced: number; + waiting: number; + noProviders: boolean; static createFrom(source: any = {}) { return new Summary(source); @@ -880,6 +882,8 @@ export namespace download { this.attempted = source["attempted"]; this.started = source["started"]; this.synced = source["synced"]; + this.waiting = source["waiting"]; + this.noProviders = source["noProviders"]; } }