diff --git a/caddy/app.go b/caddy/app.go index f10bf965bf..e4593acc49 100644 --- a/caddy/app.go +++ b/caddy/app.go @@ -126,19 +126,49 @@ func (f *FrankenPHPApp) addModuleWorkers(workers ...workerConfig) ([]workerConfi 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) + } - if w.Name == "" { - w.Name = f.generateUniqueModuleWorkerName(w.FileName) - } else if !strings.HasPrefix(w.Name, "m#") { - w.Name = "m#" + w.Name + for i := range workers { + if i < len(registered) { + workers[i].Name = registered[i].Name + continue } - f.Workers = append(f.Workers, *w) + 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() diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 3bab22d456..3d77031d27 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -858,6 +858,69 @@ func TestWorkerMetrics(t *testing.T) { )) } +// #2477: verify one pool per worker, no "_0" duplicates. +func TestPhpServerWorkerMatchPoolCount(t *testing.T) { + tester := caddytest.NewTester(t) + initServer(t, tester, ` + { + skip_install_trust + admin localhost:2999 + http_port `+testPort+` + https_port 9443 + metrics + } + + localhost:`+testPort+` { + php_server { + root ../testdata + worker { + file ../testdata/dedup-match-worker.php + num 1 + match /match/* + } + worker { + file ../testdata/dedup-plain-worker.php + num 1 + } + } + } + `, "caddyfile") + + matchedWorker, _ := fastabs.FastAbs("../testdata/dedup-match-worker.php") + plainWorker, _ := fastabs.FastAbs("../testdata/dedup-plain-worker.php") + + // the matched (non-.php) path must still be served by its worker + tester.AssertGetResponse("http://localhost:"+testPort+"/match/anything", http.StatusOK, "dedup-match-worker") + + resp, err := http.Get("http://localhost:2999/metrics") + require.NoError(t, err, "failed to fetch metrics") + t.Cleanup(func() { + require.NoError(t, resp.Body.Close()) + }) + + metrics := new(bytes.Buffer) + _, err = metrics.ReadFrom(resp.Body) + require.NoError(t, err, "failed to read metrics") + + var pools []string + for _, line := range strings.Split(metrics.String(), "\n") { + if !strings.HasPrefix(line, "frankenphp_total_workers{worker=") { + continue + } + if !strings.Contains(line, "dedup-match-worker.php") && !strings.Contains(line, "dedup-plain-worker.php") { + continue + } + pools = append(pools, line) + } + + require.Len(t, pools, 2, "expected exactly one pool per distinct worker, got: %v", pools) + joined := strings.Join(pools, "\n") + require.NotContains(t, joined, escapeMetricLabel(matchedWorker)+`_0`, "matched worker must not be registered twice: %v", pools) + require.NotContains(t, joined, escapeMetricLabel(plainWorker)+`_0`, "plain worker must not be registered twice: %v", pools) + require.Contains(t, joined, escapeMetricLabel(matchedWorker), "matched worker pool must be present: %v", pools) + require.Contains(t, joined, escapeMetricLabel(plainWorker), "plain worker pool must be present: %v", pools) +} + func TestNamedWorkerMetrics(t *testing.T) { var wg sync.WaitGroup tester := caddytest.NewTester(t) diff --git a/caddy/config_test.go b/caddy/config_test.go index a26d065a80..c0da09c565 100644 --- a/caddy/config_test.go +++ b/caddy/config_test.go @@ -1,12 +1,104 @@ 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 := ` diff --git a/caddy/module.go b/caddy/module.go index fe14818105..bf57c0efa0 100644 --- a/caddy/module.go +++ b/caddy/module.go @@ -45,6 +45,8 @@ type FrankenPHPModule struct { Env map[string]string `json:"env,omitempty"` // Workers configures the worker scripts to start. Workers []workerConfig `json:"workers,omitempty"` + // RouteGroup is set automatically to pair the route embeds of one php_server directive (#2477). Do not set it manually. + RouteGroup string `json:"route_group,omitempty"` resolvedDocumentRoot string preparedEnv frankenphp.PreparedEnv @@ -92,6 +94,7 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { } wc.requestOptions = append(wc.requestOptions, loggerOpt) + wc.routeGroup = f.RouteGroup f.Workers[i] = wc } @@ -339,6 +342,8 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) return m, err } +const routeGroupStateKey = "frankenphp.worker_route_group_seq" + // parsePhpServer parses the php_server directive, which has a similar syntax // to the php_fastcgi directive. A line such as this: // @@ -371,6 +376,12 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) return nil, h.ArgErr() } + // per-adaptation counter: identical for both embeds of this directive, distinct for every other + // (including separate snippet imports), and stable across re-adaptation since State resets each time + seq, _ := h.State[routeGroupStateKey].(int) + h.State[routeGroupStateKey] = seq + 1 + routeGroup := strconv.Itoa(seq) + // set up FrankenPHP phpsrv := FrankenPHPModule{} @@ -481,6 +492,8 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) } } + phpsrv.RouteGroup = routeGroup + // set up a route list that we'll append to routes := caddyhttp.RouteList{} diff --git a/caddy/workerconfig.go b/caddy/workerconfig.go index c50f0d0688..0ade09fee3 100644 --- a/caddy/workerconfig.go +++ b/caddy/workerconfig.go @@ -46,6 +46,7 @@ type workerConfig struct { requestOptions []frankenphp.RequestOption absFileName string matchRelPath string // pre-computed relative URL path for fast matching + routeGroup string // identifies the php_server directive whose route embeds share this pool } func unmarshalWorker(d *caddyfile.Dispenser) (workerConfig, error) { diff --git a/testdata/dedup-match-worker.php b/testdata/dedup-match-worker.php new file mode 100644 index 0000000000..a3eae39262 --- /dev/null +++ b/testdata/dedup-match-worker.php @@ -0,0 +1,8 @@ +