From 64c28dd45ee4e7dc9b1e297e44c5d99ef7971fb9 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Mon, 27 Apr 2026 19:05:05 +0200 Subject: [PATCH] fix(supervisor): retry first launch on failure (#1538) --- pkg/gotenberg/supervisor.go | 39 ++++++++++++++------------ pkg/gotenberg/supervisor_test.go | 47 ++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 17 deletions(-) diff --git a/pkg/gotenberg/supervisor.go b/pkg/gotenberg/supervisor.go index 73a7f8d7..d669a643 100644 --- a/pkg/gotenberg/supervisor.go +++ b/pkg/gotenberg/supervisor.go @@ -86,12 +86,13 @@ type processSupervisor struct { maxConcurrency int64 semaphore chan struct{} firstStart atomic.Bool - firstStartOnce sync.Once - // firstStartErr stores the error from the first Launch attempt executed - // via firstStartOnce. Subsequent callers that enter the !firstStart block - // need to observe this value after the Once has completed, without - // re-executing the closure. - firstStartErr error + // firstStartMu serializes lazy-launch attempts so concurrent callers do + // not all spawn Launch() simultaneously. Using a mutex (instead of + // sync.Once) lets a failed launch be retried by the next caller, since a + // transient failure (such as a cold-start timeout) must not poison the + // supervisor for the rest of the container's lifetime. See + // https://github.com/gotenberg/gotenberg/issues/1538. + firstStartMu sync.Mutex reqCounter atomic.Int64 reqQueueSize atomic.Int64 restartsCounter atomic.Int64 @@ -346,8 +347,6 @@ func (s *processSupervisor) maybeIdleShutdown() { // Reset state so ensureStarted() re-launches on next request. s.firstStart.Store(false) - s.firstStartOnce = sync.Once{} - s.firstStartErr = nil s.reqCounter.Store(0) s.logger.DebugContext(context.Background(), "process stopped due to idle timeout") @@ -375,21 +374,27 @@ func (s *processSupervisor) acquireSlot(ctx context.Context, logger *slog.Logger } } -// ensureStarted performs a one-time lazy launch of the process on its first -// use. Subsequent calls are no-ops. +// ensureStarted performs a lazy launch of the process on its first use. +// Concurrent callers serialize on firstStartMu; once the launch succeeds, +// subsequent calls short-circuit on the firstStart flag. A failed launch +// leaves firstStart unset, so the next caller retries the launch. func (s *processSupervisor) ensureStarted(ctx context.Context) error { if s.firstStart.Load() { return nil } - s.firstStartOnce.Do(func() { - s.firstStartErr = s.runWithDeadline(ctx, func() error { - return s.Launch() - }) - }) + s.firstStartMu.Lock() + defer s.firstStartMu.Unlock() - if s.firstStartErr != nil { - return fmt.Errorf("process first start: %w", s.firstStartErr) + if s.firstStart.Load() { + return nil + } + + err := s.runWithDeadline(ctx, func() error { + return s.Launch() + }) + if err != nil { + return fmt.Errorf("process first start: %w", err) } return nil diff --git a/pkg/gotenberg/supervisor_test.go b/pkg/gotenberg/supervisor_test.go index a5c0c57c..a13e6e99 100644 --- a/pkg/gotenberg/supervisor_test.go +++ b/pkg/gotenberg/supervisor_test.go @@ -898,6 +898,53 @@ func TestProcessSupervisor_IdleShutdown(t *testing.T) { } } +func TestProcessSupervisor_RetryAfterFailedFirstStart(t *testing.T) { + // Regression test for https://github.com/gotenberg/gotenberg/issues/1538: + // a failed first launch must not poison the supervisor; the next request + // must retry Launch() instead of returning the cached error forever. + logger := slog.New(slog.DiscardHandler) + + var startCalls atomic.Int64 + process := &ProcessMock{ + StartMock: func(logger *slog.Logger) error { + if startCalls.Add(1) == 1 { + return errors.New("first start failed") + } + return nil + }, + StopMock: func(logger *slog.Logger) error { + return nil + }, + HealthyMock: func(logger *slog.Logger) bool { + return true + }, + } + + ps := NewProcessSupervisor(logger, process, 0, 0, 1, 0).(*processSupervisor) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + err := ps.Run(ctx, logger, func() error { return nil }) + if err == nil { + t.Fatal("expected first Run to fail because Launch failed") + } + if ps.firstStart.Load() { + t.Fatal("firstStart must remain false after a failed Launch") + } + + err = ps.Run(ctx, logger, func() error { return nil }) + if err != nil { + t.Fatalf("expected second Run to succeed after the supervisor retries Launch, got: %v", err) + } + if !ps.firstStart.Load() { + t.Fatal("expected firstStart to be set after the second Launch succeeds") + } + if got := startCalls.Load(); got != 2 { + t.Fatalf("expected exactly 2 Start calls, got %d", got) + } +} + func TestProcessSupervisor_IdleShutdownSkippedWhenActive(t *testing.T) { logger := slog.New(slog.DiscardHandler)