Make GCWorker.Start idempotent via sync.Once (x9)

Calling Start a second time used to spawn a duplicate goroutine,
leaking the first one and double-counting tick events. sync.Once
matches the existing lifecycle (Stop is also one-shot — the worker
is not designed to be restarted after Stop), so the simplest fix is
a startOnce.Do guard. Added a unit test that calls Start twice and
asserts only one goroutine runs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

2 files changed, 83 insertions, 23 deletions

diff --git a/player-server/internal/service/gc.go b/player-server/internal/service/gc.go
index ab7f646..9a7752e 100644
--- a/player-server/internal/service/gc.go
+++ b/player-server/internal/service/gc.go
@@ -24,6 +24,7 @@ type GCWorker struct {
 	tickCh    <-chan time.Time
 	runDoneCh chan struct{}
 	stopCh    chan struct{}
+	startOnce sync.Once
 	stopOnce  sync.Once
 	wg        sync.WaitGroup
 	mediaRoot string
@@ -56,32 +57,38 @@ func (w *GCWorker) WithInterval(interval time.Duration) *GCWorker {
 	return w
 }
 
-// Start launches the GC goroutine.
+// Start launches the GC goroutine. Idempotent: subsequent calls after the
+// first are no-ops. Paired with Stop (also one-shot via sync.Once); the
+// worker is not designed to be restarted after Stop, so a sync.Once guard
+// matches the existing lifetime semantics and prevents goroutine leaks
+// from accidental double-Start.
 func (w *GCWorker) Start() {
-	w.ctx, w.cancel = context.WithCancel(context.Background())
-	tickCh := w.tickCh
-	if tickCh == nil {
-		w.ticker = time.NewTicker(w.interval)
-		tickCh = w.ticker.C
-	}
-	w.wg.Add(1)
-	go func() {
-		defer w.wg.Done()
-		for {
-			select {
-			case <-tickCh:
-				func() {
-					defer func() {
-						RecoverWorker(w.logger, "gc", recover())
+	w.startOnce.Do(func() {
+		w.ctx, w.cancel = context.WithCancel(context.Background())
+		tickCh := w.tickCh
+		if tickCh == nil {
+			w.ticker = time.NewTicker(w.interval)
+			tickCh = w.ticker.C
+		}
+		w.wg.Add(1)
+		go func() {
+			defer w.wg.Done()
+			for {
+				select {
+				case <-tickCh:
+					func() {
+						defer func() {
+							RecoverWorker(w.logger, "gc", recover())
+						}()
+						w.run(w.ctx)
 					}()
-					w.run(w.ctx)
-				}()
-				w.notifyRunDone()
-			case <-w.stopCh:
-				return
+					w.notifyRunDone()
+				case <-w.stopCh:
+					return
+				}
 			}
-		}
-	}()
+		}()
+	})
 }
 
 // Stop stops the GC goroutine and waits for it to finish.
diff --git a/player-server/internal/service/gc_test.go b/player-server/internal/service/gc_test.go
index 07c8924..18c8a93 100644
--- a/player-server/internal/service/gc_test.go
+++ b/player-server/internal/service/gc_test.go
@@ -6,6 +6,7 @@ import (
 	"log/slog"
 	"os"
 	"path/filepath"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -265,6 +266,58 @@ func TestGCWorker_DoubleStop(t *testing.T) {
 	w.Stop() // must not panic
 }
 
+// TestGCWorker_DoubleStartIdempotent verifies that calling Start more than
+// once does not spawn additional goroutines. Each spawned worker increments
+// wg by 1; we drain a single tick and assert only one run happens, then Stop
+// must return promptly (would deadlock on wg.Wait if extra goroutines existed
+// without their own stop signal handling).
+func TestGCWorker_DoubleStartIdempotent(t *testing.T) {
+	now := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
+	var runCount int32
+	store := &repository.MockStore{
+		MediaRepo: repository.MockMediaRepo{
+			ListDeletedMediaFunc: func(ctx context.Context) ([]model.Media, error) {
+				atomic.AddInt32(&runCount, 1)
+				return nil, nil
+			},
+		},
+	}
+
+	logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError}))
+	w := NewGCWorker(store, &clock.MockClock{T: now}, "/tmp", time.Minute, logger)
+	tickCh := make(chan time.Time, 4)
+	runDoneCh := make(chan struct{}, 4)
+	w.tickCh = tickCh
+	w.runDoneCh = runDoneCh
+
+	// Call Start multiple times: only the first should spawn a goroutine.
+	w.Start()
+	w.Start()
+	w.Start()
+
+	// A single tick must produce exactly one run; if extra goroutines were
+	// spawned they would also pick up ticks from the same channel.
+	tickCh <- now
+	select {
+	case <-runDoneCh:
+	case <-time.After(time.Second):
+		t.Fatal("timed out waiting for gc run")
+	}
+
+	// No additional runs should be pending.
+	select {
+	case <-runDoneCh:
+		t.Fatal("unexpected extra run from duplicate Start")
+	case <-time.After(50 * time.Millisecond):
+	}
+
+	w.Stop()
+
+	if got := atomic.LoadInt32(&runCount); got != 1 {
+		t.Fatalf("expected exactly 1 run, got %d", got)
+	}
+}
+
 func TestGCWorker_RelPathFallback(t *testing.T) {
 	now := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
 	tmpDir := t.TempDir()