From 2967a0d2c860d3af16164e106b547256b7ebf7c7 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Tue, 19 May 2026 14:59:27 +0300 Subject: Unify panic recovery into service.RecoverWorker cmd/player/main.go had recoverBackgroundWorkerPanic and internal/service/recover.go had handleWorkerPanic implementing the same log-and-stack panic recovery. Export the service variant as RecoverWorker (unchanged signature), use it from main.go, and drop the duplicate helper plus the now-unused runtime/debug import. Co-Authored-By: Claude Opus 4.7 --- player-server/cmd/player/main.go | 14 ++++++-------- player-server/internal/service/gc.go | 2 +- player-server/internal/service/podcast_checker.go | 2 +- player-server/internal/service/recover.go | 17 ++++++++++++++++- player-server/internal/service/scan.go | 2 +- 5 files changed, 25 insertions(+), 12 deletions(-) diff --git a/player-server/cmd/player/main.go b/player-server/cmd/player/main.go index 72bff60..8835a1a 100644 --- a/player-server/cmd/player/main.go +++ b/player-server/cmd/player/main.go @@ -8,7 +8,6 @@ import ( "net/http" "os" "os/signal" - "runtime/debug" "syscall" "time" @@ -82,12 +81,6 @@ func buildLogger(logLevel string) *slog.Logger { return slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: level})) } -func recoverBackgroundWorkerPanic(logger *slog.Logger, worker string) { - if r := recover(); r != nil && logger != nil { - logger.Error("background worker panic", "worker", worker, "panic", r, "stack", string(debug.Stack())) - } -} - // wireDeps constructs the core service layer dependencies. func wireDeps(cfg *internal.Config, store repository.Store, logger *slog.Logger, appCtx context.Context) *appDeps { clk := clock.RealClock{} @@ -144,7 +137,12 @@ func startBackgroundWorkers(deps *appDeps) { select { case <-ticker.C: func() { - defer recoverBackgroundWorkerPanic(deps.logger, "podcast checker") + // Use the unified service.RecoverWorker helper so this + // matches every other background-worker panic path in + // the codebase (gc, rescan, podcast feed check). + defer func() { + service.RecoverWorker(deps.logger, "podcast checker", recover()) + }() if err := deps.podcastSvc.CheckFeeds(context.Background()); err != nil { deps.logger.Error("podcast feed check failed", "err", err) } diff --git a/player-server/internal/service/gc.go b/player-server/internal/service/gc.go index 6eb485a..ab7f646 100644 --- a/player-server/internal/service/gc.go +++ b/player-server/internal/service/gc.go @@ -72,7 +72,7 @@ func (w *GCWorker) Start() { case <-tickCh: func() { defer func() { - handleWorkerPanic(w.logger, "gc", recover()) + RecoverWorker(w.logger, "gc", recover()) }() w.run(w.ctx) }() diff --git a/player-server/internal/service/podcast_checker.go b/player-server/internal/service/podcast_checker.go index 1de920c..362fd0e 100644 --- a/player-server/internal/service/podcast_checker.go +++ b/player-server/internal/service/podcast_checker.go @@ -42,7 +42,7 @@ func (s *podcastFeedChecker) CheckFeeds(ctx context.Context) error { go func(f model.PodcastFeed) { defer wg.Done() defer func() { - handleWorkerPanic(s.logger, "podcast feed check", recover()) + RecoverWorker(s.logger, "podcast feed check", recover()) }() if err := s.checkFeed(ctx, f); err != nil { s.logger.Warn("podcast feed check failed", "feed_id", f.ID, "feed_url", f.FeedURL, "err", err) diff --git a/player-server/internal/service/recover.go b/player-server/internal/service/recover.go index c89a78e..59a4716 100644 --- a/player-server/internal/service/recover.go +++ b/player-server/internal/service/recover.go @@ -6,7 +6,22 @@ import ( "runtime/debug" ) -func handleWorkerPanic(logger *slog.Logger, worker string, r any) error { +// RecoverWorker is the unified panic-recovery helper for background workers. +// Pass the value returned by recover() in r; when non-nil it logs the panic +// together with the stack trace and returns a formatted error so callers can +// propagate the failure (e.g. to scan progress) if they care. When r is nil +// the function is a no-op, which makes it safe to use unconditionally inside +// a deferred wrapper. +// +// Typical usage: +// +// defer func() { +// service.RecoverWorker(logger, "podcast checker", recover()) +// }() +// +// This consolidates what used to be duplicated as handleWorkerPanic (here) +// and recoverBackgroundWorkerPanic (in cmd/player) into one exported variant. +func RecoverWorker(logger *slog.Logger, worker string, r any) error { if r == nil { return nil } diff --git a/player-server/internal/service/scan.go b/player-server/internal/service/scan.go index da5c54f..fe9513b 100644 --- a/player-server/internal/service/scan.go +++ b/player-server/internal/service/scan.go @@ -61,7 +61,7 @@ func (s *scanService) TriggerRescan(ctx context.Context) error { defer cancel() defer s.notifyDone() defer func() { - if err := handleWorkerPanic(s.logger, "rescan", recover()); err != nil { + if err := RecoverWorker(s.logger, "rescan", recover()); err != nil { progress.Done(err) } }() -- cgit v1.2.3