From 2b7703ad8395df78bf76eaaf2c1e29995af58e4b Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Sun, 20 Sep 2026 08:38:53 +0200 Subject: [PATCH 1/6] feat: validate a configuration before it replaces the running one Start() calls Shutdown() before Init(), since the PHP runtime is a process singleton, so a declaration error takes the site down and Caddy rolls back the configuration, not the runtime that went with it: a missing worker file, a name two workers share, a thread budget that does not add up, any of them leaves the server answering 500 until the next reload. frankenphp.Validate() takes the options Init() takes and reports what it would refuse, touching nothing: the thread budget, the worker files, and the names and scopes workers may take. The rules are the ones Init() runs, newWorker() now shares them rather than holding its own copy. The Caddy app implements caddy.Validator on it, which Caddy calls while the previous configuration still serves, so a rejected reload keeps the site up. Collecting the options moved out of Start() for that, and the names workers take are uniquified per collection rather than per app, so validating a configuration does not change the names the next one gets. --- caddy/admin_test.go | 84 +++++++++++++++++++++++++++ caddy/app.go | 124 +++++++++++++++++++++++++--------------- caddy/config_test.go | 16 +++--- caddy/serveridx_test.go | 11 ++-- docs/library.md | 2 +- frankenphp.go | 46 ++++++++++++++- frankenphp_test.go | 27 +++++++++ worker.go | 60 +++++++++++++++---- 8 files changed, 298 insertions(+), 72 deletions(-) diff --git a/caddy/admin_test.go b/caddy/admin_test.go index b5fe37daae..bfad8fa206 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "strconv" "strings" "sync" "testing" @@ -409,3 +410,86 @@ func TestRegisteredModuleWorkerPoolsMustBeCorrect(t *testing.T) { assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker2Path, "expected module worker with \"match\" directive to be present") assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker3Path, "expected module worker without \"match\" directive to be present") } + +// a configuration Caddy rolls back must leave the running one serving: +// Validate() rejects it before Start() has a chance to stop the runtime +func TestRejectedReloadKeepsThePreviousSiteServing(t *testing.T) { + tester := caddytest.NewTester(t) + initServer(t, tester, ` + { + skip_install_trust + admin localhost:2999 + http_port `+testPort+` + + frankenphp { + worker ../testdata/worker-with-counter.php 1 + } + } + + localhost:`+testPort+` { + route { + root ../testdata + rewrite worker-with-counter.php + php + } + } + `, "caddyfile") + + workerURL := "http://localhost:" + testPort + "/worker-with-counter.php" + servedBefore := countedRequests(t, workerURL) + + // the worker file does not exist, which Init() only reports once the + // running configuration is gone + rejected := ` + { + skip_install_trust + admin localhost:2999 + http_port ` + testPort + ` + + frankenphp { + worker ../testdata/not-a-worker.php 1 + } + } + + localhost:` + testPort + ` { + route { + root ../testdata + rewrite worker-with-counter.php + php + } + } + ` + + r, err := http.NewRequest("POST", "http://localhost:2999/load", bytes.NewBufferString(rejected)) + require.NoError(t, err) + r.Header.Set("Content-Type", "text/caddyfile") + resp, err := http.DefaultClient.Do(r) + require.NoError(t, err) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + // the adapt endpoint answers 200 with the format warnings, then the error + require.Contains(t, string(body), "invalid configuration: worker filename is invalid") + + // the runtime that served before the rejected reload still serves, and + // it is the same one: its worker kept counting + require.Equal(t, servedBefore+1, countedRequests(t, workerURL)) +} + +// the number of requests testdata/worker-with-counter.php has served +func countedRequests(t *testing.T, workerURL string) int { + t.Helper() + + resp, err := http.Get(workerURL) + require.NoError(t, err) + defer func() { require.NoError(t, resp.Body.Close()) }() + require.Equal(t, http.StatusOK, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + count, err := strconv.Atoi(strings.TrimPrefix(string(body), "requests:")) + require.NoError(t, err, "unexpected worker response %q", body) + + return count +} diff --git a/caddy/app.go b/caddy/app.go index fcee129180..711405d30c 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -65,7 +65,6 @@ type FrankenPHPApp struct { ctx context.Context logger *slog.Logger modules []*FrankenPHPModule - usedWorkerNames map[string]bool httpApp *caddyhttp.App hasStarted atomic.Bool started chan any @@ -107,18 +106,33 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error { return nil } -func (f *FrankenPHPApp) Start() error { - defer func() { - close(f.started) - }() +// Validate implements caddy.Validator. Caddy calls it before the running +// configuration is replaced, so a declaration error is reported while that +// configuration still serves: Start() shuts the runtime down before Init() +// can report anything, and Caddy rolls back the configuration, not the PHP +// runtime that went with it. +func (f *FrankenPHPApp) Validate() error { + opts, err := f.collectOptions(caddy.NewReplacer(), false) + if err != nil { + return err + } + + return frankenphp.Validate(opts...) +} - repl := caddy.NewReplacer() +// collectOptions turns the configuration into the options Init() takes. +// Validate() collects them for a configuration that may never start, Start() +// for the one starting, where the servers it creates are the ones the +// modules serve from, hence keep. +func (f *FrankenPHPApp) collectOptions(repl *caddy.Replacer, keep bool) ([]frankenphp.Option, error) { + // We have at least 9 hardcoded options + opts := make([]frankenphp.Option, 0, 9+len(options)) optionsMU.RLock() - f.opts = append(f.opts, options...) + opts = append(opts, options...) optionsMU.RUnlock() - f.opts = append(f.opts, + opts = append(opts, frankenphp.WithContext(f.ctx), frankenphp.WithLogger(f.logger), frankenphp.WithNumThreads(f.NumThreads), @@ -130,20 +144,37 @@ func (f *FrankenPHPApp) Start() error { frankenphp.WithMaxRequests(f.MaxRequests), ) + usedWorkerNames := make(map[string]bool, len(f.Workers)) + // register global workers for _, w := range f.Workers { w.FileName = repl.ReplaceKnown(w.FileName, "") - w.Name = f.createUniqueWorkerName(w, "") - opts, err := w.toWorkerOptions() + w.Name = createUniqueWorkerName(usedWorkerNames, w, "") + workerOptions, err := w.toWorkerOptions() if err != nil { - return err + return nil, err } - f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, opts...)) + opts = append(opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...)) } - if err := f.registerModules(repl); err != nil { + moduleOpts, err := f.collectModuleOptions(repl, usedWorkerNames, keep) + if err != nil { + return nil, err + } + + return append(opts, moduleOpts...), nil +} + +func (f *FrankenPHPApp) Start() error { + defer func() { + close(f.started) + }() + + opts, err := f.collectOptions(caddy.NewReplacer(), true) + if err != nil { return err } + f.opts = opts // if FrankenPHP is currently running, shut it down first // this will happen in admin API reloads and caddy tests @@ -178,35 +209,43 @@ func (f *FrankenPHPApp) Stop() error { } // register workers and servers for "php" and "php_server" modules -func (f *FrankenPHPApp) registerModules(repl *caddy.Replacer) error { - modulesByIndex := make(map[int]*FrankenPHPModule, len(f.modules)) - for _, module := range f.modules { - if module.ServerIndex == 0 { - if err := f.registerModule(repl, module); err != nil { - return err - } - continue - } +func (f *FrankenPHPApp) collectModuleOptions(repl *caddy.Replacer, usedWorkerNames map[string]bool, keep bool) ([]frankenphp.Option, error) { + opts := make([]frankenphp.Option, 0, len(f.modules)) + serversByIndex := make(map[int]*frankenphp.Server, len(f.modules)) + for _, module := range f.modules { // modules with the same server_idx should share the same server instance // example: the worker { match * } rule adds 2 "php" subroutes to the caddy handler // the 2 handlers belong to the same "php_server" and must therefore share workers - if existingModule, ok := modulesByIndex[module.ServerIndex]; ok { - module.server = existingModule.server - continue + if module.ServerIndex != 0 { + if server, ok := serversByIndex[module.ServerIndex]; ok { + if keep { + module.server = server + } + + continue + } } - modulesByIndex[module.ServerIndex] = module - if err := f.registerModule(repl, module); err != nil { - return err + server, moduleOpts, err := f.collectModule(repl, module, usedWorkerNames) + if err != nil { + return nil, err + } + + if keep { + module.server = server + } + if module.ServerIndex != 0 { + serversByIndex[module.ServerIndex] = server } + opts = append(opts, moduleOpts...) } - return nil + return opts, nil } -// register a server instance and its workers for a single Caddy module -func (f *FrankenPHPApp) registerModule(repl *caddy.Replacer, module *FrankenPHPModule) error { +// collect the server instance and the worker options of a single Caddy module +func (f *FrankenPHPApp) collectModule(repl *caddy.Replacer, module *FrankenPHPModule, usedWorkerNames map[string]bool) (*frankenphp.Server, []frankenphp.Option, error) { serverName := f.resolveServerName(module) server, err := frankenphp.NewServer( module.resolvedDocumentRoot, @@ -216,34 +255,29 @@ func (f *FrankenPHPApp) registerModule(repl *caddy.Replacer, module *FrankenPHPM frankenphp.WithServerLogger(module.logger), ) if err != nil { - return err + return nil, nil, err } - module.server = server - f.opts = append(f.opts, frankenphp.WithServer(server)) + opts := []frankenphp.Option{frankenphp.WithServer(server)} for _, w := range module.Workers { w.FileName = repl.ReplaceKnown(w.FileName, "") - w.Name = f.createUniqueWorkerName(w, serverName) + w.Name = createUniqueWorkerName(usedWorkerNames, w, serverName) workerOptions, err := w.toWorkerOptions() if err != nil { - return err + return nil, nil, err } workerOptions = append(workerOptions, frankenphp.WithWorkerServerScope(server)) - f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...)) + opts = append(opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...)) } - return nil + return server, opts, nil } // avoid name collisions for workers // on collision, a name is first qualified with the server name // (":") before falling back to a numeric postfix -func (f *FrankenPHPApp) createUniqueWorkerName(wc workerConfig, serverName string) string { - if f.usedWorkerNames == nil { - f.usedWorkerNames = make(map[string]bool) - } - +func createUniqueWorkerName(usedWorkerNames map[string]bool, wc workerConfig, serverName string) string { if wc.Name == "" { wc.Name, _ = fastabs.FastAbs(wc.FileName) } @@ -251,8 +285,8 @@ func (f *FrankenPHPApp) createUniqueWorkerName(wc workerConfig, serverName strin name := wc.Name suffix := 0 for { - if _, ok := f.usedWorkerNames[name]; !ok { - f.usedWorkerNames[name] = true + if _, ok := usedWorkerNames[name]; !ok { + usedWorkerNames[name] = true break } if serverName != "" { diff --git a/caddy/config_test.go b/caddy/config_test.go index 607051cbd8..0598b42530 100644 --- a/caddy/config_test.go +++ b/caddy/config_test.go @@ -250,16 +250,16 @@ func TestModuleWorkerWithCustomName(t *testing.T) { } func TestCreateUniqueWorkerNames(t *testing.T) { - app := &FrankenPHPApp{} + usedWorkerNames := map[string]bool{} filename := "../testdata/worker-with-env.php" absFileName, _ := filepath.Abs(filename) names := make([]string, 6) for i := range 3 { - names[i] = app.createUniqueWorkerName(workerConfig{ + names[i] = createUniqueWorkerName(usedWorkerNames, workerConfig{ FileName: filename, Name: "custom-worker-name", }, "") - names[i+3] = app.createUniqueWorkerName(workerConfig{ + names[i+3] = createUniqueWorkerName(usedWorkerNames, workerConfig{ FileName: filename, }, "") } @@ -273,14 +273,14 @@ func TestCreateUniqueWorkerNames(t *testing.T) { } func TestCreateUniqueWorkerNamesQualifiedByServer(t *testing.T) { - app := &FrankenPHPApp{} + usedWorkerNames := map[string]bool{} wc := workerConfig{FileName: "../testdata/worker-with-env.php", Name: "queue"} - require.Equal(t, "queue", app.createUniqueWorkerName(wc, "one.example.com")) + require.Equal(t, "queue", createUniqueWorkerName(usedWorkerNames, wc, "one.example.com")) // on collision, the name is qualified with the server name - require.Equal(t, "two.example.com:queue", app.createUniqueWorkerName(wc, "two.example.com")) + require.Equal(t, "two.example.com:queue", createUniqueWorkerName(usedWorkerNames, wc, "two.example.com")) // when the qualified name is also taken, fall back to the numeric postfix - require.Equal(t, "queue_1", app.createUniqueWorkerName(wc, "two.example.com")) + require.Equal(t, "queue_1", createUniqueWorkerName(usedWorkerNames, wc, "two.example.com")) // workers without a server keep the numeric postfix behavior - require.Equal(t, "queue_2", app.createUniqueWorkerName(wc, "")) + require.Equal(t, "queue_2", createUniqueWorkerName(usedWorkerNames, wc, "")) } diff --git a/caddy/serveridx_test.go b/caddy/serveridx_test.go index 7414c07169..1fd401429d 100644 --- a/caddy/serveridx_test.go +++ b/caddy/serveridx_test.go @@ -26,7 +26,8 @@ func TestRegisterModulesWithSameServerIndexShareOneServer(t *testing.T) { shared2 := &FrankenPHPModule{ServerIndex: 1, resolvedDocumentRoot: "../testdata"} app.modules = []*FrankenPHPModule{shared1, shared2} - require.NoError(t, app.registerModules(caddy.NewReplacer())) + _, err := app.collectModuleOptions(caddy.NewReplacer(), map[string]bool{}, true) + require.NoError(t, err) require.NotNil(t, shared1.server) require.Same(t, shared1.server, shared2.server, "modules with the same server_idx must share one server instance") @@ -39,7 +40,8 @@ func TestRegisterModulesWithoutServerIndexGetOwnServers(t *testing.T) { indexed := &FrankenPHPModule{ServerIndex: 1, resolvedDocumentRoot: "../testdata"} app.modules = []*FrankenPHPModule{auto1, indexed, auto2} - require.NoError(t, app.registerModules(caddy.NewReplacer())) + _, err := app.collectModuleOptions(caddy.NewReplacer(), map[string]bool{}, true) + require.NoError(t, err) require.NotNil(t, auto1.server) require.NotNil(t, auto2.server) @@ -59,8 +61,9 @@ func TestRegisterModulesFirstModuleWinsPerIdx(t *testing.T) { second := &FrankenPHPModule{ServerIndex: 2, resolvedDocumentRoot: "../testdata/env"} app.modules = []*FrankenPHPModule{first, second} - require.NoError(t, app.registerModules(caddy.NewReplacer())) + opts, err := app.collectModuleOptions(caddy.NewReplacer(), map[string]bool{}, true) + require.NoError(t, err) require.Same(t, first.server, second.server) - require.Len(t, app.opts, 1, "only one server must be registered for a shared index") + require.Len(t, opts, 1, "only one server must be registered for a shared index") } diff --git a/docs/library.md b/docs/library.md index 7782f03182..968d2bd971 100644 --- a/docs/library.md +++ b/docs/library.md @@ -17,7 +17,7 @@ For a minimal example see [https://pkg.go.dev](https://pkg.go.dev/github.com/dun `NewServer()` takes a human-readable name used to attribute workers, metrics and logs to the server (defaults to `server_` at registration when empty), the document root, the split path suffixes (defaults to `[".php"]`), environment variables made available to every request, and a `*slog.Logger` (defaults to the global logger). -`Init()` starts the PHP runtime and must be called exactly once before serving requests; `Shutdown()` stops it. Calling `Server.ServeHTTP()` before `Init()` or after `Shutdown()` returns `ErrNotRunning`. The same `*Server` may be passed to `Init()` again after a `Shutdown()`, for instance to reload the configuration. +`Init()` starts the PHP runtime and must be called exactly once before serving requests; `Shutdown()` stops it. `Validate()` takes the same options and reports whether `Init()` would accept them, without starting anything: a host replacing a running configuration should call it before stopping the one in place, since `Init()` only reports a declaration error once the previous runtime is gone. Calling `Server.ServeHTTP()` before `Init()` or after `Shutdown()` returns `ErrNotRunning`. The same `*Server` may be passed to `Init()` again after a `Shutdown()`, for instance to reload the configuration. ## Multiple servers diff --git a/frankenphp.go b/frankenphp.go index d8a8c2cf61..763fd11722 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -225,8 +225,6 @@ func calculateMaxThreads(opt *opt) (numWorkers int, _ error) { // https://github.com/php/frankenphp/issues/126 opt.workers[i].num = maxProcs } - metrics.TotalWorkers(w.name, w.num) - numWorkers += opt.workers[i].num if w.maxThreads > 0 { @@ -299,6 +297,47 @@ func calculateMaxThreads(opt *opt) (numWorkers int, _ error) { } // Init starts the PHP runtime and the configured workers. +// Validate reports whether a configuration would be accepted by Init(), +// without starting anything or touching the configuration already running. +// It runs the rules a declaration must follow: the thread budget, the +// worker files, and the names and scopes workers may take. +// +// A host that replaces a running configuration should call it before +// stopping the one in place, since Init() only reports these errors once +// the previous runtime is gone. +func Validate(options ...Option) error { + opt := &opt{} + for _, o := range options { + if err := o(opt); err != nil { + return err + } + } + + if _, err := calculateMaxThreads(opt); err != nil { + return err + } + + takenNames := make(map[string]bool, len(opt.workers)) + takenGlobalPaths := make(map[string]bool, len(opt.workers)) + for _, w := range opt.workers { + w, err := resolveWorkerFile(w) + if err != nil { + return err + } + + if err := checkWorkerDeclaration(w, takenNames, takenGlobalPaths); err != nil { + return err + } + + takenNames[w.name] = true + if w.server == nil { + takenGlobalPaths[w.fileName] = true + } + } + + return nil +} + func Init(options ...Option) error { if !isRunning.CompareAndSwap(false, true) { return ErrAlreadyStarted @@ -351,6 +390,9 @@ func Init(options ...Option) error { } metrics.TotalThreads(opt.numThreads) + for _, w := range opt.workers { + metrics.TotalWorkers(w.name, w.num) + } config := Config() diff --git a/frankenphp_test.go b/frankenphp_test.go index 6e1ae40cbd..d51ede55d4 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1504,3 +1504,30 @@ func testOpcachePreload(t *testing.T, opts *testOptions) { assert.Equal(t, "I am preloaded", body) }, opts) } + +// Validate reports what Init() would refuse, without starting anything +func TestValidateReportsDeclarationErrors(t *testing.T) { + assert.NoError(t, frankenphp.Validate( + frankenphp.WithNumThreads(2), + frankenphp.WithWorkers("worker", "testdata/worker.php", 1), + )) + + assert.ErrorContains(t, frankenphp.Validate( + frankenphp.WithWorkers("worker", "testdata/not-a-worker.php", 1), + ), "worker filename is invalid") + + assert.ErrorContains(t, frankenphp.Validate( + frankenphp.WithWorkers("same", "testdata/worker.php", 1), + frankenphp.WithWorkers("same", "testdata/index.php", 1), + ), "two workers cannot have the same name") + + assert.ErrorContains(t, frankenphp.Validate( + frankenphp.WithWorkers("one", "testdata/worker.php", 1), + frankenphp.WithWorkers("two", "testdata/worker.php", 1), + ), "two global workers cannot have the same filename") + + assert.ErrorContains(t, frankenphp.Validate( + frankenphp.WithNumThreads(2), + frankenphp.WithMaxThreads(1), + ), "max_threads") +} diff --git a/worker.go b/worker.go index 388dfbd031..f4bd3adaa7 100644 --- a/worker.go +++ b/worker.go @@ -101,42 +101,78 @@ func initWorkers(opts []workerOpt) error { return nil } -func newWorker(o workerOpt) (*worker, error) { - // Order is important! - // This order ensures that FrankenPHP started from inside a symlinked directory will properly resolve any paths. - // If it is started from outside a symlinked directory, it is resolved to the same path that we use in the Caddy module. +// resolveWorkerFile turns a declared filename into the path a worker runs, +// and names the worker after it when the declaration left the name out. +// +// Order is important! +// This order ensures that FrankenPHP started from inside a symlinked directory will properly resolve any paths. +// If it is started from outside a symlinked directory, it is resolved to the same path that we use in the Caddy module. +func resolveWorkerFile(o workerOpt) (workerOpt, error) { absFileName, err := filepath.EvalSymlinks(filepath.FromSlash(o.fileName)) if err != nil { - return nil, fmt.Errorf("worker filename is invalid %q: %w", o.fileName, err) + return o, fmt.Errorf("worker filename is invalid %q: %w", o.fileName, err) } absFileName, err = fastabs.FastAbs(absFileName) if err != nil { - return nil, fmt.Errorf("worker filename is invalid %q: %w", o.fileName, err) + return o, fmt.Errorf("worker filename is invalid %q: %w", o.fileName, err) } if _, err := os.Stat(absFileName); err != nil { - return nil, fmt.Errorf("worker file not found %q: %w", absFileName, err) + return o, fmt.Errorf("worker file not found %q: %w", absFileName, err) } + o.fileName = absFileName if o.name == "" { o.name = absFileName } + return o, nil +} + +// checkWorkerDeclaration holds the rules a set of workers must follow, +// against the names and paths the ones before it took. Validate() runs them +// over a configuration that may never start, newWorker() over the one +// starting, so both answer the same way. +func checkWorkerDeclaration(o workerOpt, takenNames map[string]bool, takenGlobalPaths map[string]bool) error { if o.server == nil { - if globalWorkersByPath[absFileName] != nil { - return nil, fmt.Errorf("two global workers cannot have the same filename: %q", absFileName) + if takenGlobalPaths[o.fileName] { + return fmt.Errorf("two global workers cannot have the same filename: %q", o.fileName) } // no server means no set of requests to match against, the matcher would never run if o.matchRequest != nil { - return nil, fmt.Errorf("worker %q has a request matcher but no server scope, use WithWorkerServerScope()", o.name) + return fmt.Errorf("worker %q has a request matcher but no server scope, use WithWorkerServerScope()", o.name) } } - if workersByName[o.name] != nil { - return nil, fmt.Errorf("two workers cannot have the same name: %q", o.name) + if takenNames[o.name] { + return fmt.Errorf("two workers cannot have the same name: %q", o.name) + } + + return nil +} + +func newWorker(o workerOpt) (*worker, error) { + o, err := resolveWorkerFile(o) + if err != nil { + return nil, err + } + + takenNames := make(map[string]bool, len(workersByName)) + for name := range workersByName { + takenNames[name] = true } + takenGlobalPaths := make(map[string]bool, len(globalWorkersByPath)) + for path := range globalWorkersByPath { + takenGlobalPaths[path] = true + } + + if err := checkWorkerDeclaration(o, takenNames, takenGlobalPaths); err != nil { + return nil, err + } + + absFileName := o.fileName // env should always contain FRANKENPHP_WORKER and the parent php_server env if o.env == nil { From 8eb579fa34bde99cb6e06c1b8cd83d2e94bce67a Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 21 Sep 2026 15:16:14 +0200 Subject: [PATCH 2/6] fix: check the workers a php_server declares before the runtime goes Caddy calls Validate() while it provisions the app, before the modules that carry php_server blocks provision themselves, so the workers they declare were not part of what it checked: a bad one passed validation and failed inside Init(), once Shutdown() had already taken the runtime that was serving. The site then answered "server is not registered" until the next reload, which is the failure this PR is about. Start() now runs the same rules over the options it collected, before it shuts anything down, so a rejected configuration leaves the running one alone whichever block declared the worker. Also fixes two things the review found: the options a module adds while it provisions, the hot reload among them, were dropped by Start() collecting the configuration over them, and they now have a slice of their own; and the declaration rules take lookups rather than copies of every name and path taken before, which made a declaration cost the ones before it. --- caddy/admin_test.go | 60 +++++++++++++++++++++++++++++++++++++++++++++ caddy/app.go | 14 ++++++++++- caddy/hotreload.go | 2 +- frankenphp.go | 3 ++- worker.go | 18 +++++--------- 5 files changed, 82 insertions(+), 15 deletions(-) diff --git a/caddy/admin_test.go b/caddy/admin_test.go index bfad8fa206..7789cf0c55 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -477,6 +477,66 @@ func TestRejectedReloadKeepsThePreviousSiteServing(t *testing.T) { } // the number of requests testdata/worker-with-counter.php has served +// TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing covers the +// workers a php_server block declares: Caddy calls Validate() before the +// modules provision, so those reach Start() alone, which checks them before +// it takes the runtime in place down. +func TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing(t *testing.T) { + tester := caddytest.NewTester(t) + initServer(t, tester, ` + { + skip_install_trust + admin localhost:2999 + http_port `+testPort+` + } + + localhost:`+testPort+` { + route { + root ../testdata + rewrite worker-with-counter.php + php { + worker ../testdata/worker-with-counter.php 1 + } + } + } + `, "caddyfile") + + workerURL := "http://localhost:" + testPort + "/worker-with-counter.php" + servedBefore := countedRequests(t, workerURL) + + rejected := ` + { + skip_install_trust + admin localhost:2999 + http_port ` + testPort + ` + } + + localhost:` + testPort + ` { + route { + root ../testdata + rewrite worker-with-counter.php + php { + worker ../testdata/not-a-worker.php 1 + } + } + } + ` + + r, err := http.NewRequest("POST", "http://localhost:2999/load", bytes.NewBufferString(rejected)) + require.NoError(t, err) + r.Header.Set("Content-Type", "text/caddyfile") + resp, err := http.DefaultClient.Do(r) + require.NoError(t, err) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Contains(t, string(body), "worker filename is invalid") + + // the runtime that served before the rejected reload still serves, and + // it is the same one: its worker kept counting + require.Equal(t, servedBefore+1, countedRequests(t, workerURL)) +} + func countedRequests(t *testing.T, workerURL string) int { t.Helper() diff --git a/caddy/app.go b/caddy/app.go index 711405d30c..34e8ab5792 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -61,6 +61,9 @@ type FrankenPHPApp struct { MaxRequests int `json:"max_requests,omitempty"` opts []frankenphp.Option + // options the modules add while they provision, before Start() collects + // the rest: they belong to the configuration but nothing else knows them + provisionOpts []frankenphp.Option metrics frankenphp.Metrics ctx context.Context logger *slog.Logger @@ -126,12 +129,14 @@ func (f *FrankenPHPApp) Validate() error { // modules serve from, hence keep. func (f *FrankenPHPApp) collectOptions(repl *caddy.Replacer, keep bool) ([]frankenphp.Option, error) { // We have at least 9 hardcoded options - opts := make([]frankenphp.Option, 0, 9+len(options)) + opts := make([]frankenphp.Option, 0, 9+len(options)+len(f.provisionOpts)) optionsMU.RLock() opts = append(opts, options...) optionsMU.RUnlock() + opts = append(opts, f.provisionOpts...) + opts = append(opts, frankenphp.WithContext(f.ctx), frankenphp.WithLogger(f.logger), @@ -176,6 +181,13 @@ func (f *FrankenPHPApp) Start() error { } f.opts = opts + // Validate() ran before the modules provisioned, so it saw the workers + // of the global block alone; the ones a php_server declares are checked + // here, while the configuration in place still serves + if err := frankenphp.Validate(f.opts...); err != nil { + return err + } + // if FrankenPHP is currently running, shut it down first // this will happen in admin API reloads and caddy tests frankenphp.Shutdown() diff --git a/caddy/hotreload.go b/caddy/hotreload.go index bec8b16fbd..e882c8f147 100644 --- a/caddy/hotreload.go +++ b/caddy/hotreload.go @@ -48,7 +48,7 @@ func (f *FrankenPHPModule) configureHotReload(app *FrankenPHPApp) error { f.HotReload.Topic = "https://frankenphp.dev/hot-reload/" + uid } - app.opts = append(app.opts, frankenphp.WithHotReload(f.HotReload.Topic, f.mercureHub, f.HotReload.Watch)) + app.provisionOpts = append(app.provisionOpts, frankenphp.WithHotReload(f.HotReload.Topic, f.mercureHub, f.HotReload.Watch)) // add the hot reload to the env variables if f.Env == nil { diff --git a/frankenphp.go b/frankenphp.go index 763fd11722..f3700b7d41 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -325,7 +325,8 @@ func Validate(options ...Option) error { return err } - if err := checkWorkerDeclaration(w, takenNames, takenGlobalPaths); err != nil { + if err := checkWorkerDeclaration(w, func(name string) bool { return takenNames[name] }, + func(path string) bool { return takenGlobalPaths[path] }); err != nil { return err } diff --git a/worker.go b/worker.go index f4bd3adaa7..d7325aecf2 100644 --- a/worker.go +++ b/worker.go @@ -134,9 +134,9 @@ func resolveWorkerFile(o workerOpt) (workerOpt, error) { // against the names and paths the ones before it took. Validate() runs them // over a configuration that may never start, newWorker() over the one // starting, so both answer the same way. -func checkWorkerDeclaration(o workerOpt, takenNames map[string]bool, takenGlobalPaths map[string]bool) error { +func checkWorkerDeclaration(o workerOpt, nameTaken func(string) bool, globalPathTaken func(string) bool) error { if o.server == nil { - if takenGlobalPaths[o.fileName] { + if globalPathTaken(o.fileName) { return fmt.Errorf("two global workers cannot have the same filename: %q", o.fileName) } @@ -146,7 +146,7 @@ func checkWorkerDeclaration(o workerOpt, takenNames map[string]bool, takenGlobal } } - if takenNames[o.name] { + if nameTaken(o.name) { return fmt.Errorf("two workers cannot have the same name: %q", o.name) } @@ -159,16 +159,10 @@ func newWorker(o workerOpt) (*worker, error) { return nil, err } - takenNames := make(map[string]bool, len(workersByName)) - for name := range workersByName { - takenNames[name] = true - } - takenGlobalPaths := make(map[string]bool, len(globalWorkersByPath)) - for path := range globalWorkersByPath { - takenGlobalPaths[path] = true - } + nameTaken := func(name string) bool { _, taken := workersByName[name]; return taken } + globalPathTaken := func(path string) bool { _, taken := globalWorkersByPath[path]; return taken } - if err := checkWorkerDeclaration(o, takenNames, takenGlobalPaths); err != nil { + if err := checkWorkerDeclaration(o, nameTaken, globalPathTaken); err != nil { return nil, err } From 40d7dcede2645d65b41a2211428487dcbd7479e8 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 21 Sep 2026 15:23:08 +0200 Subject: [PATCH 3/6] chore: fewer comments, the ones left say why Marc read none of them, which is fair: several restated the name of the function under them. The ones that carry a reason stay, shorter. --- caddy/admin_test.go | 11 +++-------- caddy/app.go | 21 ++++++++------------- frankenphp.go | 13 +++++-------- worker.go | 6 ++---- 4 files changed, 18 insertions(+), 33 deletions(-) diff --git a/caddy/admin_test.go b/caddy/admin_test.go index 7789cf0c55..10f1152afc 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -476,11 +476,8 @@ func TestRejectedReloadKeepsThePreviousSiteServing(t *testing.T) { require.Equal(t, servedBefore+1, countedRequests(t, workerURL)) } -// the number of requests testdata/worker-with-counter.php has served -// TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing covers the -// workers a php_server block declares: Caddy calls Validate() before the -// modules provision, so those reach Start() alone, which checks them before -// it takes the runtime in place down. +// the workers a php_server declares reach Start() alone, since Caddy calls +// Validate() before the modules provision func TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing(t *testing.T) { tester := caddytest.NewTester(t) initServer(t, tester, ` @@ -531,12 +528,10 @@ func TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing(t *testing.T require.NoError(t, err) require.NoError(t, resp.Body.Close()) require.Contains(t, string(body), "worker filename is invalid") - - // the runtime that served before the rejected reload still serves, and - // it is the same one: its worker kept counting require.Equal(t, servedBefore+1, countedRequests(t, workerURL)) } +// 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 34e8ab5792..515425893b 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -61,8 +61,7 @@ type FrankenPHPApp struct { MaxRequests int `json:"max_requests,omitempty"` opts []frankenphp.Option - // options the modules add while they provision, before Start() collects - // the rest: they belong to the configuration but nothing else knows them + // added by the modules as they provision, before Start() collects the rest provisionOpts []frankenphp.Option metrics frankenphp.Metrics ctx context.Context @@ -109,11 +108,9 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error { return nil } -// Validate implements caddy.Validator. Caddy calls it before the running -// configuration is replaced, so a declaration error is reported while that +// Validate implements caddy.Validator, which Caddy calls while the running // configuration still serves: Start() shuts the runtime down before Init() -// can report anything, and Caddy rolls back the configuration, not the PHP -// runtime that went with it. +// can report anything. func (f *FrankenPHPApp) Validate() error { opts, err := f.collectOptions(caddy.NewReplacer(), false) if err != nil { @@ -124,9 +121,8 @@ func (f *FrankenPHPApp) Validate() error { } // collectOptions turns the configuration into the options Init() takes. -// Validate() collects them for a configuration that may never start, Start() -// for the one starting, where the servers it creates are the ones the -// modules serve from, hence keep. +// keep is for the configuration that starts, whose servers the modules +// serve from; Validate() collects those of one that may never start. func (f *FrankenPHPApp) collectOptions(repl *caddy.Replacer, keep bool) ([]frankenphp.Option, error) { // We have at least 9 hardcoded options opts := make([]frankenphp.Option, 0, 9+len(options)+len(f.provisionOpts)) @@ -181,9 +177,9 @@ func (f *FrankenPHPApp) Start() error { } f.opts = opts - // Validate() ran before the modules provisioned, so it saw the workers - // of the global block alone; the ones a php_server declares are checked - // here, while the configuration in place still serves + // Validate() ran before the modules provisioned, so it saw the global + // block alone: what a php_server declares is checked here, in time for + // the configuration in place to survive a refusal if err := frankenphp.Validate(f.opts...); err != nil { return err } @@ -256,7 +252,6 @@ func (f *FrankenPHPApp) collectModuleOptions(repl *caddy.Replacer, usedWorkerNam return opts, nil } -// collect the server instance and the worker options of a single Caddy module func (f *FrankenPHPApp) collectModule(repl *caddy.Replacer, module *FrankenPHPModule, usedWorkerNames map[string]bool) (*frankenphp.Server, []frankenphp.Option, error) { serverName := f.resolveServerName(module) server, err := frankenphp.NewServer( diff --git a/frankenphp.go b/frankenphp.go index f3700b7d41..ad114ae4fc 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -297,14 +297,11 @@ func calculateMaxThreads(opt *opt) (numWorkers int, _ error) { } // Init starts the PHP runtime and the configured workers. -// Validate reports whether a configuration would be accepted by Init(), -// without starting anything or touching the configuration already running. -// It runs the rules a declaration must follow: the thread budget, the -// worker files, and the names and scopes workers may take. -// -// A host that replaces a running configuration should call it before -// stopping the one in place, since Init() only reports these errors once -// the previous runtime is gone. +// Validate reports whether Init() would accept a configuration, without +// starting anything: the thread budget, the worker files, and the names and +// scopes workers may take. A host replacing a running configuration should +// call it before stopping the one in place, since Init() only reports these +// errors once the previous runtime is gone. func Validate(options ...Option) error { opt := &opt{} for _, o := range options { diff --git a/worker.go b/worker.go index d7325aecf2..dae2f6dd5a 100644 --- a/worker.go +++ b/worker.go @@ -130,10 +130,8 @@ func resolveWorkerFile(o workerOpt) (workerOpt, error) { return o, nil } -// checkWorkerDeclaration holds the rules a set of workers must follow, -// against the names and paths the ones before it took. Validate() runs them -// over a configuration that may never start, newWorker() over the one -// starting, so both answer the same way. +// checkWorkerDeclaration holds the rules a set of workers must follow, so +// Validate() and newWorker() answer the same way. func checkWorkerDeclaration(o workerOpt, nameTaken func(string) bool, globalPathTaken func(string) bool) error { if o.server == nil { if globalPathTaken(o.fileName) { From e2727738c922a5e2aefee57ea99b9a46581a7069 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 21 Sep 2026 16:50:43 +0200 Subject: [PATCH 4/6] chore: reword the comments about Validate() --- caddy/admin_test.go | 7 +++---- caddy/app.go | 21 +++++++++------------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/caddy/admin_test.go b/caddy/admin_test.go index 10f1152afc..97eda64107 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -411,8 +411,7 @@ func TestRegisteredModuleWorkerPoolsMustBeCorrect(t *testing.T) { assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker3Path, "expected module worker without \"match\" directive to be present") } -// a configuration Caddy rolls back must leave the running one serving: -// Validate() rejects it before Start() has a chance to stop the runtime +// Validate() must reject incorrect configs before Start() calls frankenphp.Shutdown() func TestRejectedReloadKeepsThePreviousSiteServing(t *testing.T) { tester := caddytest.NewTester(t) initServer(t, tester, ` @@ -476,8 +475,8 @@ func TestRejectedReloadKeepsThePreviousSiteServing(t *testing.T) { require.Equal(t, servedBefore+1, countedRequests(t, workerURL)) } -// the workers a php_server declares reach Start() alone, since Caddy calls -// Validate() before the modules provision +// Validate() cannot see a worker declared in a php_server block: Caddy calls it +// before the modules provision, so Start() must reject those before shutting down func TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing(t *testing.T) { tester := caddytest.NewTester(t) initServer(t, tester, ` diff --git a/caddy/app.go b/caddy/app.go index 515425893b..eb3ba7ddc7 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -60,16 +60,15 @@ type FrankenPHPApp struct { // EXPERIMENTAL: MaxRequests sets the maximum number of requests a PHP thread handles before restarting (0 = unlimited) MaxRequests int `json:"max_requests,omitempty"` - opts []frankenphp.Option - // added by the modules as they provision, before Start() collects the rest + opts []frankenphp.Option provisionOpts []frankenphp.Option - metrics frankenphp.Metrics - ctx context.Context - logger *slog.Logger - modules []*FrankenPHPModule - httpApp *caddyhttp.App - hasStarted atomic.Bool - started chan any + metrics frankenphp.Metrics + ctx context.Context + logger *slog.Logger + modules []*FrankenPHPModule + httpApp *caddyhttp.App + hasStarted atomic.Bool + started chan any } var errIni = errors.New(`"php_ini" must be in the format: php_ini "" ""`) @@ -108,9 +107,7 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error { return nil } -// Validate implements caddy.Validator, which Caddy calls while the running -// configuration still serves: Start() shuts the runtime down before Init() -// can report anything. +// Validate is called by Caddy before Start() to validate before shutting down the currently running config func (f *FrankenPHPApp) Validate() error { opts, err := f.collectOptions(caddy.NewReplacer(), false) if err != nil { From 7f23d57c8f61ad145d54872949d68220f6e611ab Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 21 Sep 2026 18:47:42 +0200 Subject: [PATCH 5/6] chore: read len(options) under its lock, and put the doc comment of Init() back above it --- caddy/admin_test.go | 3 ++- caddy/app.go | 3 +-- frankenphp.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/caddy/admin_test.go b/caddy/admin_test.go index 97eda64107..e929c9ae69 100644 --- a/caddy/admin_test.go +++ b/caddy/admin_test.go @@ -467,7 +467,8 @@ func TestRejectedReloadKeepsThePreviousSiteServing(t *testing.T) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) require.NoError(t, resp.Body.Close()) - // the adapt endpoint answers 200 with the format warnings, then the error + // /load writes the adaptation warnings before loading, so the status is + // already 200 when the error follows them in the body require.Contains(t, string(body), "invalid configuration: worker filename is invalid") // the runtime that served before the rejected reload still serves, and diff --git a/caddy/app.go b/caddy/app.go index eb3ba7ddc7..74ea6961dd 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -121,10 +121,9 @@ func (f *FrankenPHPApp) Validate() error { // keep is for the configuration that starts, whose servers the modules // serve from; Validate() collects those of one that may never start. func (f *FrankenPHPApp) collectOptions(repl *caddy.Replacer, keep bool) ([]frankenphp.Option, error) { + optionsMU.RLock() // We have at least 9 hardcoded options opts := make([]frankenphp.Option, 0, 9+len(options)+len(f.provisionOpts)) - - optionsMU.RLock() opts = append(opts, options...) optionsMU.RUnlock() diff --git a/frankenphp.go b/frankenphp.go index ad114ae4fc..8baa67eb3d 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -296,7 +296,6 @@ func calculateMaxThreads(opt *opt) (numWorkers int, _ error) { return numWorkers, nil } -// Init starts the PHP runtime and the configured workers. // Validate reports whether Init() would accept a configuration, without // starting anything: the thread budget, the worker files, and the names and // scopes workers may take. A host replacing a running configuration should @@ -336,6 +335,7 @@ func Validate(options ...Option) error { return nil } +// Init starts the PHP runtime and the configured workers. func Init(options ...Option) error { if !isRunning.CompareAndSwap(false, true) { return ErrAlreadyStarted From d0a770c24dacb678effb93adb29b5e560bf391ad Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Mon, 21 Sep 2026 18:47:42 +0200 Subject: [PATCH 6/6] fix: Validate() refuses two workers of a server sharing a file, as Init() does --- frankenphp.go | 14 +++++++++----- frankenphp_test.go | 16 ++++++++++++++++ worker.go | 22 ++++++++++++++++------ 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/frankenphp.go b/frankenphp.go index 8baa67eb3d..a0a13647e5 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -314,21 +314,25 @@ func Validate(options ...Option) error { } takenNames := make(map[string]bool, len(opt.workers)) - takenGlobalPaths := make(map[string]bool, len(opt.workers)) + takenPaths := make(map[*Server]map[string]bool, 1) + nameTaken := func(name string) bool { return takenNames[name] } + pathTaken := func(server *Server, path string) bool { return takenPaths[server][path] } for _, w := range opt.workers { w, err := resolveWorkerFile(w) if err != nil { return err } - if err := checkWorkerDeclaration(w, func(name string) bool { return takenNames[name] }, - func(path string) bool { return takenGlobalPaths[path] }); err != nil { + if err := checkWorkerDeclaration(w, nameTaken, pathTaken); err != nil { return err } takenNames[w.name] = true - if w.server == nil { - takenGlobalPaths[w.fileName] = true + if w.matchRequest == nil { + if takenPaths[w.server] == nil { + takenPaths[w.server] = make(map[string]bool) + } + takenPaths[w.server][w.fileName] = true } } diff --git a/frankenphp_test.go b/frankenphp_test.go index d51ede55d4..520c9c1066 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -1526,6 +1526,22 @@ func TestValidateReportsDeclarationErrors(t *testing.T) { frankenphp.WithWorkers("two", "testdata/worker.php", 1), ), "two global workers cannot have the same filename") + server, err := frankenphp.NewServer(testDataDir) + require.NoError(t, err) + matchAll := frankenphp.WithWorkerMatcher(func(*http.Request) bool { return true }) + + assert.ErrorContains(t, frankenphp.Validate( + frankenphp.WithServer(server), + frankenphp.WithWorkers("one", "testdata/worker.php", 1, frankenphp.WithWorkerServerScope(server)), + frankenphp.WithWorkers("two", "testdata/worker.php", 1, frankenphp.WithWorkerServerScope(server)), + ), "two workers in a server cannot have the same filename") + + assert.NoError(t, frankenphp.Validate( + frankenphp.WithServer(server), + frankenphp.WithWorkers("one", "testdata/worker.php", 1, frankenphp.WithWorkerServerScope(server)), + frankenphp.WithWorkers("two", "testdata/worker.php", 1, frankenphp.WithWorkerServerScope(server), matchAll), + )) + assert.ErrorContains(t, frankenphp.Validate( frankenphp.WithNumThreads(2), frankenphp.WithMaxThreads(1), diff --git a/worker.go b/worker.go index dae2f6dd5a..4a596c2fb6 100644 --- a/worker.go +++ b/worker.go @@ -131,10 +131,11 @@ func resolveWorkerFile(o workerOpt) (workerOpt, error) { } // checkWorkerDeclaration holds the rules a set of workers must follow, so -// Validate() and newWorker() answer the same way. -func checkWorkerDeclaration(o workerOpt, nameTaken func(string) bool, globalPathTaken func(string) bool) error { +// Validate() and newWorker() answer the same way; pathTaken is asked for the +// global workers with a nil server. +func checkWorkerDeclaration(o workerOpt, nameTaken func(string) bool, pathTaken func(*Server, string) bool) error { if o.server == nil { - if globalPathTaken(o.fileName) { + if pathTaken(nil, o.fileName) { return fmt.Errorf("two global workers cannot have the same filename: %q", o.fileName) } @@ -142,6 +143,9 @@ func checkWorkerDeclaration(o workerOpt, nameTaken func(string) bool, globalPath if o.matchRequest != nil { return fmt.Errorf("worker %q has a request matcher but no server scope, use WithWorkerServerScope()", o.name) } + } else if o.matchRequest == nil && pathTaken(o.server, o.fileName) { + // a matcher tells the workers of a server sharing a file apart + return fmt.Errorf("two workers in a server cannot have the same filename: %q", o.fileName) } if nameTaken(o.name) { @@ -157,10 +161,16 @@ func newWorker(o workerOpt) (*worker, error) { return nil, err } - nameTaken := func(name string) bool { _, taken := workersByName[name]; return taken } - globalPathTaken := func(path string) bool { _, taken := globalWorkersByPath[path]; return taken } + nameTaken := func(name string) bool { return workersByName[name] != nil } + pathTaken := func(server *Server, path string) bool { + if server == nil { + return globalWorkersByPath[path] != nil + } + + return server.workersByPath[path] != nil + } - if err := checkWorkerDeclaration(o, nameTaken, globalPathTaken); err != nil { + if err := checkWorkerDeclaration(o, nameTaken, pathTaken); err != nil { return nil, err }