From feaee08d6b730251ef090a886fab5c70ebdd4cb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Sat, 10 Oct 2026 07:59:26 +0200 Subject: [PATCH] fix: serve requests reaching the previous configuration during reloads Caddy starts the new configuration before it stops the previous one. Since each php_server got its own frankenphp.Server, the new app's Start() unregisters the servers the previous HTTP server still routes to, so every request it handles until it stops fails with ErrNotRunning, answered with a 500. Such requests now wait for the new configuration to start and are handed to its module serving the same server name. Requests that cannot be forwarded get a 503 instead of a 500. Fixes https://github.com/php/frankenphp/issues/2698 --- caddy/admin_test.go | 83 +++++++++++++++++++++++++++++++++++ caddy/app.go | 10 +++++ caddy/module.go | 57 ++++++++++++++++++++---- testdata/worker-slow-boot.php | 9 ++++ 4 files changed, 151 insertions(+), 8 deletions(-) create mode 100644 testdata/worker-slow-boot.php diff --git a/caddy/admin_test.go b/caddy/admin_test.go index bbac1188c2..7979025667 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -540,6 +540,89 @@ func TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing(t *testing.T require.Equal(t, servedBefore+1, countedRequests(t, workerURL)) } +// requests still routed by the previous configuration must reach the new workers once booted +func TestReloadKeepsServingWhileWorkersBoot(t *testing.T) { + config := ` + { + skip_install_trust + admin localhost:2999 + http_port ` + testPort + ` + } + + localhost:` + testPort + ` { + php_server { + root ../testdata + worker { + file worker-slow-boot.php + num 1 + } + } + } + ` + + tester := caddytest.NewTester(t) + initServer(t, tester, config, "caddyfile") + + workerURL := "http://localhost:" + testPort + "/worker-slow-boot.php" + client := &http.Client{Timeout: 3 * time.Second} + done := make(chan struct{}) + var ( + wg sync.WaitGroup + mu sync.Mutex + statuses = map[int]int{} + ) + stop := sync.OnceFunc(func() { + close(done) + wg.Wait() + }) + t.Cleanup(stop) + + // one request per tick: a request stuck on a draining worker must not stop the probing + wg.Go(func() { + ticker := time.NewTicker(5 * time.Millisecond) + defer ticker.Stop() + + for { + select { + case <-done: + return + case <-ticker.C: + } + + wg.Go(func() { + resp, err := client.Get(workerURL) + if err != nil { + return + } + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + + mu.Lock() + statuses[resp.StatusCode]++ + mu.Unlock() + }) + } + }) + + for range 3 { + r, err := http.NewRequest("POST", "http://localhost:2999/load", strings.NewReader(config)) + require.NoError(t, err) + r.Header.Set("Content-Type", "text/caddyfile") + // reload even though the configuration did not change + r.Header.Set("Cache-Control", "must-revalidate") + resp, err := http.DefaultClient.Do(r) + require.NoError(t, err) + _, _ = io.Copy(io.Discard, resp.Body) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusOK, resp.StatusCode) + } + + stop() + + require.NotZero(t, statuses[http.StatusOK]) + assert.Equal(t, map[int]int{http.StatusOK: statuses[http.StatusOK]}, statuses) +} + // the number of requests testdata/worker-with-counter.php has served func countedRequests(t *testing.T, workerURL string) int { t.Helper() diff --git a/caddy/app.go b/caddy/app.go index 8dd40490d6..4560494ae3 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -23,6 +23,9 @@ import ( var ( options []frankenphp.Option optionsMU sync.RWMutex + + // the app whose servers are registered, or about to be + activeApp atomic.Pointer[FrankenPHPApp] ) // EXPERIMENTAL: RegisterWorkers provides a way for extensions to register frankenphp.Workers @@ -180,10 +183,15 @@ func (f *FrankenPHPApp) Start() error { return err } + // requests still reaching the previous configuration are forwarded to this one + activeApp.Store(f) + // if FrankenPHP is currently running, shut it down first // this will happen in admin API reloads and caddy tests frankenphp.Shutdown() if err := frankenphp.Init(f.opts...); err != nil { + activeApp.CompareAndSwap(f, nil) + return err } @@ -193,6 +201,8 @@ func (f *FrankenPHPApp) Start() error { } func (f *FrankenPHPApp) Stop() error { + activeApp.CompareAndSwap(f, nil) + if f.logger.Enabled(f.ctx, slog.LevelInfo) { f.logger.LogAttrs(f.ctx, slog.LevelInfo, "FrankenPHP stopped 🐘") } diff --git a/caddy/module.go b/caddy/module.go index 20dcec9ee2..767d09e61f 100644 --- a/caddy/module.go +++ b/caddy/module.go @@ -185,14 +185,9 @@ func needReplacement(s string) bool { } // ServeHTTP implements caddyhttp.MiddlewareHandler. -func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ caddyhttp.Handler) error { - if !f.app.hasStarted.Load() { - // stall any incoming request if FrankenPHP has not started yet, blocking for up to 10 seconds - select { - case <-f.app.started: - case <-time.After(10 * time.Second): - return caddyhttp.Error(http.StatusServiceUnavailable, frankenphp.ErrNotRunning) - } +func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error { + if !waitForStart(f.app) { + return caddyhttp.Error(http.StatusServiceUnavailable, frankenphp.ErrNotRunning) } ctx := r.Context() @@ -226,6 +221,9 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c } err := f.server.ServeHTTP(w, r, opts...) + if errors.Is(err, frankenphp.ErrNotRunning) { + return f.serveWithReplacement(w, r, next) + } if _, rejected := errors.AsType[frankenphp.ErrRejected](err); err != nil && !rejected { return caddyhttp.Error(http.StatusInternalServerError, err) @@ -234,6 +232,49 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c return nil } +// waitForStart stalls the request for up to 10 seconds if FrankenPHP has not started yet +func waitForStart(app *FrankenPHPApp) bool { + if app.hasStarted.Load() { + return true + } + + select { + case <-app.started: + return true + case <-time.After(10 * time.Second): + return false + } +} + +// serveWithReplacement hands the request to the configuration Caddy started before this one stopped routing +func (f *FrankenPHPModule) serveWithReplacement(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error { + notRunning := caddyhttp.Error(http.StatusServiceUnavailable, frankenphp.ErrNotRunning) + + app := activeApp.Load() + if app == nil || app == f.app || !waitForStart(app) { + return notRunning + } + + var target *FrankenPHPModule + for _, m := range app.modules { + if m.server == nil || m.server.Name() != f.server.Name() { + continue + } + + // modules sharing a server are interchangeable, distinct servers with the same name are not + if target != nil && target.server != m.server { + return notRunning + } + target = m + } + + if target == nil { + return notRunning + } + + return target.ServeHTTP(w, r, next) +} + // UnmarshalCaddyfile implements caddyfile.Unmarshaler. func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { for d.Next() { diff --git a/testdata/worker-slow-boot.php b/testdata/worker-slow-boot.php new file mode 100644 index 0000000000..be217b495b --- /dev/null +++ b/testdata/worker-slow-boot.php @@ -0,0 +1,9 @@ +