feat: unify php_server logic

caddy/admin_test.go

@@ -40,9 +40,6 @@ func TestRestartWorkerViaAdminApi(t *testing.T) {
 		}
 		`, "caddyfile")
 
-	// make sure workers are not still running from any previous tests
-	assertAdminResponse(t, tester, "POST", "workers/restart", http.StatusOK, "workers restarted successfully\n")
-
 	tester.AssertGetResponse("http://localhost:"+testPort+"/", http.StatusOK, "requests:1")
 	tester.AssertGetResponse("http://localhost:"+testPort+"/", http.StatusOK, "requests:2")
 
@@ -348,3 +345,49 @@ func TestAddModuleWorkerViaAdminApi(t *testing.T) {
 	// Make a request to the worker to verify it's working
 	tester.AssertGetResponse("http://localhost:"+testPort+"/worker-with-counter.php", http.StatusOK, "requests:1")
 }
+
+func TestRegisteredModuleWorkerPoolsMustBeCorrect(t *testing.T) {
+	tester := caddytest.NewTester(t)
+	initServer(t, tester, `
+		{
+			skip_install_trust
+			admin localhost:2999
+
+			frankenphp {
+				num_threads 4
+				worker ../testdata/worker-with-env.php 1
+			}
+		}
+
+		http://localhost:`+testPort+` {
+			route {
+				php {
+					root ../testdata
+					worker worker-with-counter.php 1 {
+						match /matched*
+					}
+				}
+				php {
+					root ../testdata
+					worker worker.php 1
+				}
+			}
+		}
+		`, "caddyfile")
+
+	debugState := getDebugState(t, tester)
+
+	worker1Path, _ := fastabs.FastAbs("../testdata/worker-with-env.php")
+	worker2Path, _ := fastabs.FastAbs("../testdata/worker-with-counter.php")
+	worker3Path, _ := fastabs.FastAbs("../testdata/worker.php")
+	receivedThreadNames := make([]string, 0)
+	for _, thread := range debugState.ThreadDebugStates {
+		receivedThreadNames = append(receivedThreadNames, thread.Name)
+	}
+
+	assert.Len(t, receivedThreadNames, 4, "expected 4 threads to be present")
+	assert.Contains(t, receivedThreadNames, "Regular PHP Thread", "expected a regular thread to be present")
+	assert.Contains(t, receivedThreadNames, "Worker PHP Thread - "+worker1Path, "expected global worker to be present")
+	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")
+}

caddy/app.go

@@ -17,7 +17,6 @@ import (
 	"github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile"
 	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
 	"github.com/dunglas/frankenphp"
-	"github.com/dunglas/frankenphp/internal/fastabs"
 )
 
 var (
@@ -64,6 +63,7 @@ type FrankenPHPApp struct {
 	metrics frankenphp.Metrics
 	ctx     context.Context
 	logger  *slog.Logger
+	modules map[int]*FrankenPHPModule
 }
 
 var errIni = errors.New(`"php_ini" must be in the format: php_ini "<key>" "<value>"`)
@@ -101,74 +101,6 @@ func (f *FrankenPHPApp) Provision(ctx caddy.Context) error {
 	return nil
 }
 
-func (f *FrankenPHPApp) generateUniqueModuleWorkerName(filepath string) string {
-	var i uint
-	filepath, _ = fastabs.FastAbs(filepath)
-	name := "m#" + filepath
-
-retry:
-	for _, wc := range f.Workers {
-		if wc.Name == name {
-			name = fmt.Sprintf("m#%s_%d", filepath, i)
-			i++
-
-			goto retry
-		}
-	}
-
-	return name
-}
-
-func (f *FrankenPHPApp) addModuleWorkers(workers ...workerConfig) ([]workerConfig, error) {
-	for i := range workers {
-		w := &workers[i]
-
-		if frankenphp.EmbeddedAppPath != "" && filepath.IsLocal(w.FileName) {
-			w.FileName = filepath.Join(frankenphp.EmbeddedAppPath, w.FileName)
-		}
-	}
-
-	// A php_server directive is provisioned once per route it's embedded in. Only the first embed
-	// registers its pools; later embeds reuse them by position, never touching other directives (#2477).
-	var registered []workerConfig
-	if len(workers) > 0 && workers[0].routeGroup != "" {
-		registered = f.moduleWorkersInRouteGroup(workers[0].routeGroup)
-	}
-
-	for i := range workers {
-		if i < len(registered) {
-			workers[i].Name = registered[i].Name
-			continue
-		}
-
-		f.registerModuleWorker(&workers[i])
-	}
-
-	return workers, nil
-}
-
-func (f *FrankenPHPApp) registerModuleWorker(w *workerConfig) {
-	if w.Name == "" {
-		w.Name = f.generateUniqueModuleWorkerName(w.FileName)
-	} else if !strings.HasPrefix(w.Name, "m#") {
-		w.Name = "m#" + w.Name
-	}
-
-	f.Workers = append(f.Workers, *w)
-}
-
-// moduleWorkersInRouteGroup returns the registered workers of one directive, in registration order.
-func (f *FrankenPHPApp) moduleWorkersInRouteGroup(routeGroup string) []workerConfig {
-	var group []workerConfig
-	for _, w := range f.Workers {
-		if w.routeGroup == routeGroup {
-			group = append(group, w)
-		}
-	}
-
-	return group
-}
-
 func (f *FrankenPHPApp) Start() error {
 	repl := caddy.NewReplacer()
 
@@ -188,23 +120,33 @@ func (f *FrankenPHPApp) Start() error {
 		frankenphp.WithMaxRequests(f.MaxRequests),
 	)
 
+	// register global workers
 	for _, w := range f.Workers {
-		w.options = append(w.options,
-			frankenphp.WithWorkerEnv(w.Env),
-			frankenphp.WithWorkerWatchMode(w.Watch),
-			frankenphp.WithWorkerMaxFailures(w.MaxConsecutiveFailures),
-			frankenphp.WithWorkerMaxThreads(w.MaxThreads),
-			frankenphp.WithWorkerRequestOptions(w.requestOptions...),
-		)
-
-		f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, repl.ReplaceKnown(w.FileName, ""), w.Num, w.options...))
+		w.FileName = repl.ReplaceKnown(w.FileName, "")
+		f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, w.toWorkerOptions()...))
+	}
+
+	// register module workers that are scoped to a php_server or php block
+	for _, module := range f.modules {
+		for _, w := range module.Workers {
+			w.FileName = repl.ReplaceKnown(w.FileName, "")
+			if module.server == nil {
+				return fmt.Errorf("module %d has no server", module.ServerIdx)
+			}
+			workerOptions := append(w.toWorkerOptions(), frankenphp.WithWorkerServerScope(module.server))
+			f.opts = append(f.opts, frankenphp.WithWorkers(w.Name, w.FileName, w.Num, workerOptions...))
+		}
 	}
 
 	frankenphp.Shutdown()
 	if err := frankenphp.Init(f.opts...); err != nil {
 		return err
 	}
 
+	// after startup, reset all configuration on the app instance
+	// this must happen here since the instance is re-used across reloads and tests
+	f.reset()
+
 	return nil
 }
 
@@ -220,18 +162,40 @@ func (f *FrankenPHPApp) Stop() error {
 		frankenphp.Shutdown()
 	}
 
-	// reset the configuration so it doesn't bleed into later tests
+	return nil
+}
+
+func (f *FrankenPHPApp) reset() {
 	f.Workers = nil
 	f.NumThreads = 0
 	f.MaxWaitTime = 0
 	f.MaxIdleTime = 0
 	f.MaxRequests = 0
-
+	f.PhpIni = nil
+	f.modules = nil
+	f.opts = nil
+	f.ctx = nil
+	f.metrics = nil
 	optionsMU.Lock()
 	options = nil
 	optionsMU.Unlock()
+}
 
-	return nil
+// register the php_server or php block to the app instance
+func (f *FrankenPHPApp) registerServer(m *FrankenPHPModule) {
+	if f.modules == nil {
+		f.modules = make(map[int]*FrankenPHPModule)
+	}
+
+	server, serverOpt := frankenphp.WithServer(m.ServerIdx, m.resolvedDocumentRoot, m.SplitPath, m.resolvedEnv)
+	m.server = server
+
+	// only register the server module if it's not already registered to avoid duplicate modules
+	// this can happen if multiple "php" directives are used in the same caddy route block (like with worker matches)
+	if _, ok := f.modules[m.ServerIdx]; !ok {
+		f.modules[m.ServerIdx] = m
+		f.opts = append(f.opts, serverOpt)
+	}
 }
 
 // UnmarshalCaddyfile implements caddyfile.Unmarshaler.

caddy/config_test.go

@@ -1,104 +1,12 @@
 package caddy
 
 import (
-	"strings"
 	"testing"
 
 	"github.com/caddyserver/caddy/v2/caddyconfig/caddyfile"
 	"github.com/stretchr/testify/require"
 )
 
-func TestPhpServerWorkerMatchNoDuplicatePools(t *testing.T) {
-	const config = `
-	{
-		php {
-			worker {
-				file ../testdata/worker-with-env.php
-				num 2
-				match /index.php/*
-			}
-			worker {
-				file ../testdata/worker-with-counter.php
-				num 1
-			}
-		}
-	}`
-
-	// a fresh copy per call mirrors the two module instances Caddy provisions for one directive
-	parseWorkers := func(routeGroup string) []workerConfig {
-		d := caddyfile.NewTestDispenser(config)
-		module := &FrankenPHPModule{}
-		require.NoError(t, module.UnmarshalCaddyfile(d))
-		require.Len(t, module.Workers, 2, "block should parse to two workers")
-		for i := range module.Workers {
-			module.Workers[i].routeGroup = routeGroup
-		}
-
-		return module.Workers
-	}
-
-	app := &FrankenPHPApp{}
-
-	first, err := app.addModuleWorkers(parseWorkers("g1")...)
-	require.NoError(t, err)
-	second, err := app.addModuleWorkers(parseWorkers("g1")...)
-	require.NoError(t, err)
-
-	require.Len(t, app.Workers, 2, "each worker must be registered exactly once")
-
-	for _, w := range app.Workers {
-		require.False(t, strings.HasSuffix(w.Name, "_0"),
-			"no _0-suffixed duplicate pool may exist, got %q", w.Name)
-	}
-
-	// both embeds must resolve to the same pools for serve-time matching
-	require.Len(t, first, 2)
-	require.Len(t, second, 2)
-	for i := range first {
-		require.Equal(t, first[i].Name, second[i].Name,
-			"both routes must reference the same pool name for worker %d", i)
-	}
-}
-
-func TestPhpServerSeparateDirectivesKeepDistinctPools(t *testing.T) {
-	worker := func(routeGroup string) workerConfig {
-		return workerConfig{FileName: "../testdata/worker-with-env.php", Num: 2, routeGroup: routeGroup}
-	}
-
-	app := &FrankenPHPApp{}
-	site1, err := app.addModuleWorkers(worker("g1"))
-	require.NoError(t, err)
-	site2, err := app.addModuleWorkers(worker("g2"))
-	require.NoError(t, err)
-
-	require.Len(t, app.Workers, 2, "identical workers from separate directives must stay separate pools")
-	require.NotEqual(t, site1[0].Name, site2[0].Name, "separate directives must not share a pool name")
-	require.True(t, strings.HasSuffix(site2[0].Name, "_0"),
-		"the second directive's pool must take a unique _0 name, got %q", site2[0].Name)
-}
-
-func TestPhpServerEmbedReuseIsPositional(t *testing.T) {
-	embed := func() []workerConfig {
-		return []workerConfig{
-			{FileName: "../testdata/worker-with-env.php", Num: 1, MatchPath: []string{"/x/*"}, routeGroup: "g1"},
-			{FileName: "../testdata/worker-with-env.php", Num: 1, MatchPath: []string{"/x/*"}, routeGroup: "g1"},
-		}
-	}
-
-	app := &FrankenPHPApp{}
-	first, err := app.addModuleWorkers(embed()...)
-	require.NoError(t, err)
-	second, err := app.addModuleWorkers(embed()...)
-	require.NoError(t, err)
-
-	require.Len(t, app.Workers, 2, "two identical workers in one block stay two pools, just as without the duplicate embed")
-	require.NotEqual(t, first[0].Name, first[1].Name, "the second identical worker must take its own _0 pool")
-	require.True(t, strings.HasSuffix(first[1].Name, "_0"), "got %q", first[1].Name)
-	for i := range first {
-		require.Equal(t, first[i].Name, second[i].Name, "the duplicate embed must reuse pools by position for worker %d", i)
-	}
-}
-
 func TestModuleWorkerDuplicateFilenamesFail(t *testing.T) {
 	// Create a test configuration with duplicate worker filenames
 	configWithDuplicateFilenames := `
@@ -168,7 +76,6 @@ func TestModuleWorkersDifferentNamesSucceed(t *testing.T) {
 
 	// Parse the first configuration
 	d1 := caddyfile.NewTestDispenser(configWithWorkerName1)
-	app := &FrankenPHPApp{}
 	module1 := &FrankenPHPModule{}
 
 	// Unmarshal the first configuration
@@ -196,16 +103,6 @@ func TestModuleWorkersDifferentNamesSucceed(t *testing.T) {
 
 	// Verify that no error was returned
 	require.NoError(t, err, "Expected no error when two workers have different names")
-
-	_, err = app.addModuleWorkers(module1.Workers...)
-	require.NoError(t, err, "Expected no error when adding the first module workers")
-	_, err = app.addModuleWorkers(module2.Workers...)
-	require.NoError(t, err, "Expected no error when adding the second module workers")
-
-	// Verify that both workers were added
-	require.Len(t, app.Workers, 2, "Expected two workers in the app")
-	require.Equal(t, "m#test-worker-1", app.Workers[0].Name, "First worker should have the correct name")
-	require.Equal(t, "m#test-worker-2", app.Workers[1].Name, "Second worker should have the correct name")
 }
 
 func TestModuleWorkerWithEnvironmentVariables(t *testing.T) {
@@ -294,7 +191,6 @@ func TestModuleWorkerWithCustomName(t *testing.T) {
 	// Parse the configuration
 	d := caddyfile.NewTestDispenser(configWithCustomName)
 	module := &FrankenPHPModule{}
-	app := &FrankenPHPApp{}
 
 	// Unmarshal the configuration
 	err := module.UnmarshalCaddyfile(d)
@@ -305,10 +201,4 @@ func TestModuleWorkerWithCustomName(t *testing.T) {
 	// Verify that the worker was added to the module
 	require.Len(t, module.Workers, 1, "Expected one worker to be added to the module")
 	require.Equal(t, "../testdata/worker-with-env.php", module.Workers[0].FileName, "Worker should have the correct filename")
-
-	// Verify that the worker was added to app.Workers with the m# prefix
-	module.Workers, err = app.addModuleWorkers(module.Workers...)
-	require.NoError(t, err, "Expected no error when adding the worker to the app")
-	require.Equal(t, "m#custom-worker-name", module.Workers[0].Name, "Worker should have the custom name, prefixed with m#")
-	require.Equal(t, "m#custom-worker-name", app.Workers[0].Name, "Worker should have the custom name, prefixed with m#")
 }