From 7c8e71b85d6475576bc1d22472ee399de7b62247 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Wed, 20 May 2026 07:06:04 +0300 Subject: Make GCWorker.Start idempotent via sync.Once (x9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- player-server/internal/service/gc.go | 53 +++++++++++++++++-------------- player-server/internal/service/gc_test.go | 53 +++++++++++++++++++++++++++++++ 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() -- cgit v1.2.3