From c3eea27b4fb362f95917bcc5065aa228e8eba323 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 24 Jul 2026 17:27:40 -0400 Subject: [PATCH] tune(artwork): 5s backoff base + 12h give-up, drop the cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Retry backoff now starts at 5s (was 15s) so a transient failure recovers on essentially the next drain, and jitter widens to ±40% so a wave of correlated failures doesn't re-clump into one poll. Add a 12h give-up budget measured from enqueued_at: once the next backoff would land past it, the worker stops retrying instead of grinding at a cap forever. A bare failure settles absent (handed to the 24h stale-absent sweep, and still recoverable on a page view); a found-stale keeps its already-served art. The budget bounds the tail, so the separate 48h backoffCap is removed. --- core/artwork/worker.go | 35 +++++++++++++++++--------- core/artwork/worker_test.go | 49 ++++++++++++++++++++++++++++--------- 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/core/artwork/worker.go b/core/artwork/worker.go index 93efc51f8..237cab8c3 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -22,10 +22,12 @@ import ( const ( workerPollInterval = 5 * time.Second - backoffBase = 15 * time.Second - backoffCap = 48 * time.Hour - breakerThreshold = 5 - breakerProbeAfter = time.Minute + backoffBase = 5 * time.Second + // giveUpAfter bounds the retry budget from enqueue: past it the worker stops retrying and + // hands the item to the periodic stale-absent recheck (settling absent on a bare failure). + giveUpAfter = 12 * time.Hour + breakerThreshold = 5 + breakerProbeAfter = time.Minute ) var errBreakerOpen = errors.New("artwork: external circuit breaker open") @@ -215,11 +217,22 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) outco log.Warn(ctx, "artwork: could not delete processed queue item", "kind", item.ItemKind, "id", item.ItemID, err) } case outcomeFoundStale, outcomeFailed: - // MarkFailedIfUnchanged, not MarkFailed: a scan that re-enqueued this row mid-flight reset - // retry_at, so stale backoff must not stomp its fresh, immediate eligibility. retryAt := time.Now().Add(backoff(item.Attempts)) - if err := queue.MarkFailedIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt, retryAt); err != nil { - log.Warn(ctx, "artwork: could not reschedule failed queue item", "kind", item.ItemKind, "id", item.ItemID, err) + if retryAt.Before(item.EnqueuedAt.Add(giveUpAfter)) { + // MarkFailedIfUnchanged, not MarkFailed: a scan that re-enqueued this row mid-flight reset + // retry_at, so stale backoff must not stomp its fresh, immediate eligibility. + if err := queue.MarkFailedIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt, retryAt); err != nil { + log.Warn(ctx, "artwork: could not reschedule failed queue item", "kind", item.ItemKind, "id", item.ItemID, err) + } + break + } + // Retry budget exhausted: stop retrying. A bare failure settles absent so the stale-absent + // sweep (and a page view) can still recover it; a stale-found keeps its already-served art. + if out == outcomeFailed { + writeAbsent(ctx, w.deps.ds.Artwork(ctx), item) + } + if err := queue.DeleteIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt); err != nil { + log.Warn(ctx, "artwork: could not remove exhausted queue item", "kind", item.ItemKind, "id", item.ItemID, err) } } return out @@ -314,14 +327,14 @@ func (w *Worker) gateFor(name string) *extGate { return g } -// backoffFor returns min(5m×4^n, 48h) scaled by (1+jitter), with jitter in [-0.2, 0.2]. +// backoffFor returns min(5s×4^n, giveUpAfter) scaled by (1+jitter), with jitter in [-0.4, 0.4]. func backoffFor(attempts int, jitter float64) time.Duration { - d := math.Min(float64(backoffBase)*math.Pow(4, float64(attempts)), float64(backoffCap)) + d := math.Min(float64(backoffBase)*math.Pow(4, float64(attempts)), float64(giveUpAfter)) return time.Duration(d * (1 + jitter)) } func backoff(attempts int) time.Duration { - return backoffFor(attempts, rand.Float64()*0.4-0.2) //nolint:gosec // retry jitter, not security-sensitive + return backoffFor(attempts, rand.Float64()*0.8-0.4) //nolint:gosec // retry jitter, not security-sensitive } // breaker opens after breakerThreshold consecutive external errors and admits a diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index aab5f7e56..cf1969c73 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -311,6 +311,31 @@ var _ = Describe("Worker", func() { Expect(it.RetryAt).To(BeTemporally("==", dequeued.Add(time.Minute))) }) + It("gives up and settles absent once the retry budget is exhausted", func() { + conf.Server.CoverArtPriority = "external" + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "al9", Name: "Album"}}) + imageAgents(&fakeImageAgent{name: "failAgent", err: errors.New("agent timed out")}) + w = NewWorker(ds, store, ag, ffm, broker, imgCache) + Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ItemKind: "al", ItemID: "al9"})).To(Succeed()) + // Age the row past the budget so the next retry would land beyond enqueued_at+giveUpAfter. + for k, v := range queueRepo.Data { + if v.ItemID == "al9" { + v.EnqueuedAt = time.Now().Add(-(giveUpAfter + time.Hour)) + queueRepo.Data[k] = v + } + } + + n, err := w.drain(ctx, 1) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(1)) + + // Row removed (stops retrying) and the failure settles absent for the periodic sweep. + Expect(findQueued(queueRepo, "al", "al9")).To(BeNil()) + ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al9", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(ia.Hash).To(BeEmpty()) + }) + It("resolves a private playlist under an admin context instead of failing forever", func() { ds.MockedUser = adminUserRepo() vds := &visibilityPlaylistDS{ @@ -565,15 +590,15 @@ var _ = Describe("backoff", func() { attempts int want time.Duration }{ - {0, 15 * time.Second}, - {1, 60 * time.Second}, - {2, 4 * time.Minute}, - {3, 16 * time.Minute}, - {4, 64 * time.Minute}, - {5, 256 * time.Minute}, - {6, 1024 * time.Minute}, - {7, 48 * time.Hour}, - {8, 48 * time.Hour}, + {0, 5 * time.Second}, + {1, 20 * time.Second}, + {2, 80 * time.Second}, + {3, 320 * time.Second}, + {4, 1280 * time.Second}, + {5, 5120 * time.Second}, + {6, 20480 * time.Second}, + {7, 12 * time.Hour}, + {8, 12 * time.Hour}, } { Expect(backoffFor(c.attempts, 0)).To(Equal(c.want), "attempt %d", c.attempts) } @@ -585,9 +610,9 @@ var _ = Describe("backoff", func() { Expect(backoffFor(2, -0.2)).To(Equal(time.Duration(float64(base) * 0.8))) }) - It("keeps random jitter within +/-20%", func() { - lo := time.Duration(float64(16*time.Minute) * 0.8) - hi := time.Duration(float64(16*time.Minute) * 1.2) + It("keeps random jitter within +/-40%", func() { + lo := time.Duration(float64(320*time.Second) * 0.6) + hi := time.Duration(float64(320*time.Second) * 1.4) for range 200 { d := backoff(3) Expect(d).To(BeNumerically(">=", lo))