fix: review follow-ups on the php_server refactor (#2565)

Follow-up to #2499, targeting `refactor/phpserver`. Four commits, each
independently reviewable.

Findings from a close read of the branch. Nothing here changes the
direction of the refactor; the first two are the ones I would not want
to ship.

### 1. The per-worker Mercure hub is dropped

`assignMercureHub()` fills `workerConfig.options`, but the new
`toWorkerOptions()` builds a fresh slice and never appends it, where
`Start()` used to pass `w.options` directly. The field is now written
and never read.

`WithWorkerMercureHub()` appends `WithMercureHub()` to the worker
request options, which is what lets a worker publish from its startup
code, outside `frankenphp_handle_request()`. Request-scoped publishing
kept working because the module carries `WithMercureHub()` in its own
request options, which is why no test noticed. #2543 extends the same
function and inherits the bug.

### 2. `FrankenPHPModule.server` can be nil in `ServeHTTP()`

It is assigned by `FrankenPHPApp.Start()`, but caddy starts apps by
ranging over a map, so the http app can begin serving first. The handler
dereferenced it unconditionally, turning that window into a nil pointer
panic where `main` returned a clean `ErrNotRunning`.

Two neighbouring lifecycle issues are fixed in the same commit: modules
left in `app.modules` by a config that failed to provision are
registered again by the next reload (the app is a process singleton and
`reset()` only runs at the end of `Start()`), and `match` in a global
worker block was parsed then silently ignored, so it is now rejected at
parse time as `docs/config.md` already documents.

### 3. A `Server` could not be registered twice

`unregisterServers()` only flipped `isRegistered`, so the worker slices
and maps kept the previous run's entries and a second `Init()` with the
same `*Server` failed with `two workers in a server cannot have the same
filename`. Caddy dodges this by building a fresh `NewServer` per
`Start()`, but `docs/library.md` presents `NewServer` + `Init` as the
library pattern with no hint the instance is single-use.

Same commit: the `server_<idx>` default was written into `s.name`
permanently, servers were marked registered about a hundred lines before
`initWorkers()` and the thread setup (a request in that window found
neither a worker nor a regular thread, so `activateServers()` is split
out), and `WithWorkerMatcher()` without `WithWorkerServerScope()` was a
silent no-op that still got path-matched via `globalWorkersByPath`, the
opposite of what the matcher asked for.

### 4. Logger and prepared env

Worker startup contexts hardcoded `globalLogger` even though the
worker's server was at hand, so worker boot messages and worker stdout
bypassed the per-`php_server` logger this branch introduces. The module
used to append `WithRequestLogger()` to each worker's request options
and that line is gone, so nothing else covered it.

`go_register_server_variables()` merged the prepared env whenever the
server had one, but `registerPreparedEnv()` only runs from
`go_update_request_info()`, which returns early without a request. For a
server-scoped extension worker handling a message the merge copied
whatever the thread-local prepared env held from an earlier request.

### Tests

Added: `toWorkerOptions()` keeps provisioned options, a `Server`
re-registered after `Shutdown()` still serves its worker and keeps its
name, and a request matcher without a server scope is rejected.

Both modules pass `go vet` including test files. The runtime tests need
CI: on my box (WSL2) per-thread engine bootstrap of an embed ZTS build
is pathologically slow and linking needs dev libs I do not have, so
every `Init()`-based test is unrunnable locally, including on unmodified
base commits.

### Not included

`worker.mercureHub` is assigned by `configureMercure()` and never read
anywhere; removing the dead field touches the `nomercure` build-tag
pair, so I left it alone. Say the word and I will fold it in.

Author: nicolas-grekas

caddy/app.go

@@ -83,6 +83,10 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error {
 	f.ctx = ctx
 	f.logger = ctx.Slogger()
 
+	// drop the modules of a config that failed to provision, reset() only runs in Start()
+	f.modules = nil
+	f.usedWorkerNames = nil
+
 	// We have at least 7 hardcoded options
 	f.opts = make([]frankenphp.Option, 0, 7+len(options))
 
@@ -371,6 +375,10 @@ func (f *FrankenPHPApp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				if frankenphp.EmbeddedAppPath != "" && filepath.IsLocal(wc.FileName) {
 					wc.FileName = filepath.Join(frankenphp.EmbeddedAppPath, wc.FileName)
 				}
+				// a global worker has no php_server, so no set of requests to match against
+				if len(wc.MatchPath) != 0 {
+					return d.Errf(`"match" can only be used in a php_server worker block, not in a global one: %q`, wc.FileName)
+				}
 				// check for duplicate workers
 				for _, existingWorker := range f.Workers {
 					if existingWorker.FileName == wc.FileName {

caddy/config_test.go

@@ -7,9 +7,20 @@ import (
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
+	"github.com/dunglas/frankenphp"
 	"github.com/stretchr/testify/require"
 )
 
+// options collected while provisioning a module, like the Mercure hub, must reach frankenphp
+func TestWorkerOptionsKeepProvisionedOptions(t *testing.T) {
+	wc := workerConfig{FileName: "../testdata/worker-with-env.php"}
+	base := len(wc.toWorkerOptions())
+
+	wc.options = append(wc.options, frankenphp.WithWorkerMaxThreads(3))
+
+	require.Len(t, wc.toWorkerOptions(), base+1, "worker options set during provisioning must be forwarded")
+}
+
 func TestModuleRequestBodyTimeout(t *testing.T) {
 	d := caddyfile.NewTestDispenser(`
 	{

caddy/module.go

@@ -47,7 +47,7 @@ type FrankenPHPModule struct {
 	Env map[string]string `json:"env,omitempty"`
 	// Workers configures the worker scripts to start.
 	Workers []workerConfig `json:"workers,omitempty"`
-	// ServerIdx is the idx of the php_server this module belongs to
+	// ServerIdx is set automatically to pair the route embeds of one php_server directive. Do not set it manually: modules sharing an index share one server, defined by the first of them.
 	ServerIdx int `json:"server_idx,omitempty"`
 	// RequestBodyTimeout is an idle timeout on request body reads: a stalled (slow POST) client is cut off while a steady upload of any size succeeds. Defaults to 60s when omitted; set to 0 to disable.
 	RequestBodyTimeout *caddy.Duration `json:"request_body_timeout,omitempty"`
@@ -188,6 +188,11 @@ func needReplacement(s string) bool {
 
 // ServeHTTP implements caddyhttp.MiddlewareHandler.
 func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ caddyhttp.Handler) error {
+	// the server is assigned in FrankenPHPApp.Start(), which caddy may run after the http app started serving
+	if f.server == nil {
+		return caddyhttp.Error(http.StatusInternalServerError, frankenphp.ErrNotRunning)
+	}
+
 	ctx := r.Context()
 	repl := ctx.Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
 

caddy/workerconfig.go

@@ -165,6 +165,9 @@ func (wc *workerConfig) toWorkerOptions() []frankenphp.WorkerOption {
 		frankenphp.WithWorkerRequestOptions(wc.requestOptions...),
 	}
 
+	// options collected while provisioning the module, e.g. the Mercure hub
+	opts = append(opts, wc.options...)
+
 	// copy the caddy match logic and create a unique matcher function for this worker
 	// inject the matcher into frankenphp
 	if len(wc.MatchPath) > 0 {

cgi.go

@@ -171,11 +171,14 @@ func go_register_server_variables(threadIndex C.uintptr_t, trackVarsArray *C.zva
 	thread := phpThreads[threadIndex]
 	fc := thread.handler.frankenPHPContext()
 
-	if fc.request != nil {
-		addKnownVariablesToServer(fc, trackVarsArray)
-		addHeadersToServer(fc.ctx, fc.request, trackVarsArray)
+	if fc.request == nil {
+		// go_update_request_info() never ran, the thread-local prepared env still holds the previous request
+		return
 	}
 
+	addKnownVariablesToServer(fc, trackVarsArray)
+	addHeadersToServer(fc.ctx, fc.request, trackVarsArray)
+
 	// The Prepared Environment is registered last and can overwrite any previous values
 	if len(fc.env) != 0 || len(fc.server.env) != 0 {
 		C.frankenphp_merge_with_prepared_env(trackVarsArray)

context.go

@@ -122,14 +122,21 @@ func newWorkerDummyContext(w *worker) (*frankenPHPContext, error) {
 		return nil, err
 	}
 
+	server := w.server
+	if server == nil {
+		// global worker, not associated with a server
+		server = fallbackServer
+	}
+
 	fc := &frankenPHPContext{
 		done:      make(chan any),
 		ctx:       r.Context(),
-		server:    w.server,
+		server:    server,
 		request:   r,
 		startedAt: time.Now(),
-		logger:    globalLogger,
-		worker:    w,
+		// startup output of a scoped worker belongs to its server's logger
+		logger: server.logger.Load(),
+		worker: w,
 	}
 
 	for _, o := range w.requestOptions {
@@ -138,38 +145,32 @@ func newWorkerDummyContext(w *worker) (*frankenPHPContext, error) {
 		}
 	}
 
-	if fc.server == nil {
-		// global worker, not associated with a server
-		fc.server = fallbackServer
-	}
-
 	splitCgiPath(fc)
 
 	return fc, nil
 }
 
 // newContextFromMessage creates a context from a message (external workers)
 func newContextFromMessage(message any, rw http.ResponseWriter, ctx context.Context, w *worker) *frankenPHPContext {
-	fc := &frankenPHPContext{
+	server := w.server
+	if server == nil {
+		server = fallbackServer
+	}
+
+	if ctx == nil {
+		ctx = globalCtx
+	}
+
+	return &frankenPHPContext{
 		done:              make(chan any),
 		startedAt:         time.Now(),
-		server:            w.server,
+		server:            server,
 		worker:            w,
-		logger:            globalLogger,
+		logger:            server.logger.Load(),
 		responseWriter:    rw,
 		handlerParameters: message,
 		ctx:               ctx,
 	}
-
-	if fc.server == nil {
-		fc.server = fallbackServer
-	}
-
-	if fc.ctx == nil {
-		fc.ctx = globalCtx
-	}
-
-	return fc
 }
 
 // closeContext sends the response to the client

docs/library.md

@@ -46,7 +46,7 @@ func main() {
 
 `NewServer()` takes a human-readable name used to attribute workers, metrics and logs to the server (defaults to `server_<idx>` 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`.
+`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.
 
 ## Multiple servers
 
@@ -87,7 +87,7 @@ err := frankenphp.Init(
 )
 ```
 
-Workers declared without a server scope are global: they match by file path on any server.
+Workers declared without a server scope are global: they match by file path on any server. Since a global worker has no set of requests to match against, combining `WithWorkerMatcher()` with a global worker is a configuration error and `Init()` rejects it.
 
 ## Per-request options
 

frankenphp.go

@@ -342,6 +342,9 @@ func Init(options ...Option) error {
 
 	initAutoScaling(mainThread)
 
+	// only now that the workers and threads are up may requests reach a server
+	activateServers()
+
 	if globalLogger.Enabled(globalCtx, slog.LevelInfo) {
 		globalLogger.LogAttrs(globalCtx, slog.LevelInfo, "FrankenPHP started 🐘", slog.String("php_version", Version().Version), slog.Int("num_threads", mainThread.numThreads), slog.Int("max_threads", mainThread.maxThreads), slog.Int("max_requests", maxRequestsPerThread))
 
@@ -782,6 +785,8 @@ func resetGlobals() {
 	workers = nil
 	workersByName = nil
 	globalWorkersByPath = nil
+	servers = nil
+	fallbackServer.logger.Store(globalLogger)
 	watcherIsEnabled = false
 	maxIdleTime = defaultMaxIdleTime
 	maxRequestsPerThread = 0

server.go

@@ -13,15 +13,16 @@ import (
 // Server represents a preconfigured server block
 // requests and workers can be scoped to a Server
 type Server struct {
-	idx                       int
+	idx int
+	// name passed to NewServer(), kept so re-registering resolves the default anew
+	configuredName            string
 	name                      string
 	root                      string
 	splitPath                 []string
 	env                       PreparedEnv
 	workers                   []*worker
 	workersByPath             map[string]*worker
 	workersWithRequestMatcher []*worker
-	workerOpts                []workerOpt
 
 	// registered while FrankenPHP runs with this server; read by concurrent
 	// ServeHTTP calls while Init()/Shutdown() flip it, hence atomic
@@ -48,15 +49,29 @@ func newFallbackServer() *Server {
 	return s
 }
 
+// registerServers assigns the identity of every server and clears the workers of a previous run,
+// so the same *Server can be passed to Init() again after a Shutdown()
+// servers do not accept requests yet at this point, see activateServers()
 func registerServers(newServers []*Server) {
 	servers = newServers
 	fallbackServer.logger.Store(globalLogger)
-	fallbackServer.isRegistered.Store(true)
+	fallbackServer.resetWorkers()
+
 	for i, s := range servers {
 		s.idx = i
+		s.name = s.configuredName
 		if s.name == "" {
 			s.name = "server_" + strconv.Itoa(i)
 		}
+		s.resetWorkers()
+	}
+}
+
+// activateServers lets registered servers accept requests
+// it runs once workers and threads are up, so a request cannot reach a server before them
+func activateServers() {
+	fallbackServer.isRegistered.Store(true)
+	for _, s := range servers {
 		s.isRegistered.Store(true)
 	}
 }
@@ -66,6 +81,14 @@ func unregisterServers() {
 	for _, server := range servers {
 		server.isRegistered.Store(false)
 	}
+	servers = nil
+}
+
+// resetWorkers drops the workers of a previous run; initWorkers() adds them back
+func (s *Server) resetWorkers() {
+	s.workers = nil
+	s.workersByPath = make(map[string]*worker)
+	s.workersWithRequestMatcher = nil
 }
 
 // NewServer creates a Server that can be registered via WithServer().
@@ -83,12 +106,12 @@ func NewServer(name, root string, splitPath []string, env map[string]string, log
 	}
 
 	s := &Server{
-		name:          name,
-		root:          root,
-		splitPath:     splitPath,
-		env:           PrepareEnv(env),
-		workersByPath: make(map[string]*worker),
-		workerOpts:    make([]workerOpt, 0),
+		configuredName: name,
+		name:           name,
+		root:           root,
+		splitPath:      splitPath,
+		env:            PrepareEnv(env),
+		workersByPath:  make(map[string]*worker),
 	}
 
 	if logger == nil {

server_test.go

@@ -172,6 +172,42 @@ func TestServer(t *testing.T) {
 		assert.Contains(t, err.Error(), "two workers in a server cannot have the same filename")
 	})
 
+	t.Run("error_on_request_matcher_without_server_scope", func(t *testing.T) {
+		t.Cleanup(frankenphp.Shutdown)
+
+		err := frankenphp.Init(
+			frankenphp.WithWorkers("global", testDataDir+"worker-with-counter.php", 1,
+				frankenphp.WithWorkerMatcher(func(*http.Request) bool { return true }),
+			),
+		)
+
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "no server scope")
+	})
+
+	// re-registering a Server must not trip the duplicate filename check on its own workers
+	t.Run("reregistration_after_shutdown", func(t *testing.T) {
+		server, err := frankenphp.NewServer("", testDataDir, nil, nil, nil)
+		require.NoError(t, err)
+
+		opts := []frankenphp.Option{
+			frankenphp.WithServer(server),
+			frankenphp.WithWorkers("counter", testDataDir+"worker-with-counter.php", 1,
+				frankenphp.WithWorkerServerScope(server),
+			),
+		}
+
+		initServers(t, opts...)
+		assert.Equal(t, "requests:1", serverGet(t, server, "http://example.com/worker-with-counter.php"))
+		assert.Equal(t, "server_0", server.Name())
+
+		frankenphp.Shutdown()
+
+		initServers(t, opts...)
+		assert.Equal(t, "requests:1", serverGet(t, server, "http://example.com/worker-with-counter.php"))
+		assert.Equal(t, "server_0", server.Name())
+	})
+
 	t.Run("error_on_missing_registration", func(t *testing.T) {
 		server, _ := frankenphp.NewServer("", testDataDir, nil, nil, nil)