From d0e3991d16d8ecc155272bb4242b9f2fe12d98d8 Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Wed, 15 Jul 2026 19:04:38 +0200 Subject: [PATCH] fix(chromium): serialize browser starts to prevent pinning proxy latch after a start timeout --- pkg/modules/chromium/browser.go | 21 +++++++++++++++ pkg/modules/chromium/browser_test.go | 39 ++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 pkg/modules/chromium/browser_test.go diff --git a/pkg/modules/chromium/browser.go b/pkg/modules/chromium/browser.go index 409941e0..3bd1bb85 100644 --- a/pkg/modules/chromium/browser.go +++ b/pkg/modules/chromium/browser.go @@ -58,6 +58,14 @@ type chromiumBrowser struct { userProfileDirPath string ctxMu sync.RWMutex isStarted atomic.Bool + // startMu serializes Start calls. The supervisor's runWithDeadline + // abandons a Start goroutine when the request deadline expires while + // Chromium's startup handshake is still hanging; the abandoned goroutine + // keeps running, holding the resources it acquired (the pinning proxy). + // Serializing here prevents a second, overlapping Start from colliding + // with the in-flight one on the shared pinning proxy. + // See https://github.com/gotenberg/gotenberg/issues/1599. + startMu sync.Mutex arguments browserArguments fs *gotenberg.FileSystem @@ -77,6 +85,19 @@ func newChromiumBrowser(arguments browserArguments) browser { } func (b *chromiumBrowser) Start(logger *slog.Logger) error { + // Refuse to run while a previous Start is still in flight. That previous + // Start may be a goroutine the supervisor abandoned after the request + // deadline expired while the Chromium startup handshake was hanging; it + // still holds the pinning proxy it started. An abandoned goroutine keeps + // holding startMu until it unwinds (bounded by --chromium-start-timeout), + // so no overlapping Start can collide with it on the shared pinning proxy + // and latch Chromium into a permanent "pinning proxy already started" + // state. See https://github.com/gotenberg/gotenberg/issues/1599. + if !b.startMu.TryLock() { + return errors.New("browser start already in progress") + } + defer b.startMu.Unlock() + if b.isStarted.Load() { return errors.New("browser is already started") } diff --git a/pkg/modules/chromium/browser_test.go b/pkg/modules/chromium/browser_test.go new file mode 100644 index 00000000..1067b888 --- /dev/null +++ b/pkg/modules/chromium/browser_test.go @@ -0,0 +1,39 @@ +package chromium + +import ( + "context" + "log/slog" + "strings" + "testing" +) + +// TestChromiumBrowser_Start_rejectsOverlappingStart guards against the latch +// reported in https://github.com/gotenberg/gotenberg/issues/1599. When the +// supervisor abandons a Start goroutine on request-deadline expiry, that +// goroutine keeps running and holds startMu (and the pinning proxy it started) +// until it unwinds. A second Start must be refused rather than proceeding to +// start the pinning proxy a second time. +func TestChromiumBrowser_Start_rejectsOverlappingStart(t *testing.T) { + b := &chromiumBrowser{initialCtx: context.Background()} + + // Simulate a Start still in flight. + b.startMu.Lock() + defer b.startMu.Unlock() + + err := b.Start(slog.New(slog.DiscardHandler)) + if err == nil { + t.Fatal("expected an error when a start is already in progress, got nil") + } + if !strings.Contains(err.Error(), "already in progress") { + t.Fatalf("expected an 'already in progress' error, got %q", err) + } + + // The guard must return before touching any startup resource, so no user + // profile directory is created and the browser stays not started. + if b.userProfileDirPath != "" { + t.Fatalf("expected no user profile directory to be created, got %q", b.userProfileDirPath) + } + if b.isStarted.Load() { + t.Fatal("expected the browser to stay not started") + } +}