feat: validate a configuration before it replaces the running one

[nicolas-grekas]

Start() calls Shutdown() before Init(), since the PHP runtime is a process singleton, so a declaration error takes the site down: Caddy rolls back the configuration, not the runtime that went with it, and the server answers 500 until the next reload. A missing worker file does it, so does a name two workers share, or a thread budget that does not add up.

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 instead of holding its own copy.

The Caddy app implements caddy.Validator on top of it, which Caddy calls while the previous configuration still serves:

POST /load with a worker file that does not exist
{"error":"loading config: ... frankenphp: invalid configuration: worker filename is invalid \"...\": no such file or directory"}

and the site that was running keeps serving, which TestRejectedReloadKeepsThePreviousSiteServing checks by counting the requests its worker served across the rejected reload.

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 no longer changes the names the next one gets.

Reported by @henderkes in #2617, where duplicate worker names added a trigger to a failure mode that predates it: #2617 (comment)

[henderkes]

Also, I did not read the comments during this review... too many comments, too many of them useless.

[Copilot]

Copilot review overview

🟡 Changes recommended

Validation misses per-server duplicate worker files, and worker initialization now has quadratic map-copying overhead.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2

Open (2)

• Validate misses per-server worker filename collisions · New
• Avoid quadratic allocations during worker initialization · New

What changed in this PR

Adds preflight configuration validation so invalid Caddy reloads do not stop the running PHP runtime.

Changes:

• Adds frankenphp.Validate() and shared worker validation.
• Implements Caddy’s configuration validator.
• Adds unit and reload regression tests.

File	Description
worker.go	Extracts worker file and declaration validation.
frankenphp.go	Adds Validate() and defers worker metrics.
frankenphp_test.go	Tests declaration validation errors.
docs/​library.md	Documents the validation API.
caddy/​app.go	Adds validation and refactors option collection.
caddy/​admin_test.go	Tests rejected reload continuity.
caddy/​config_test.go	Updates worker-name tests.
caddy/​serveridx_test.go	Updates server collection tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[nicolas-grekas]

Review addressed in a438dce, CI green.

You were right about both: configureHotReload adds its option while the modules provision and Start() collected the configuration over it, which was the red CI, and Validate() runs before those modules exist, so the workers a php_server declares were never part of what it checked. Provisioning-time options have their own slice now, and Start() runs the same rules over the options it collected before it shuts anything down, so a bad declaration leaves the running configuration alone whichever block it came from. TestRejectedReloadWithAModuleWorkerKeepsThePreviousSiteServing fails without that, with the 500 you would get today.

The declaration rules take lookups instead of copies of every name and path before them, per Copilot.

And the comments: fair, several restated the function under them. They are down from 49 lines to 35, the ones left say why.

[henderkes]

Please unsloppify the comments further. No AI is going to get that done.

[nicolas-grekas]

Please unsloppify the comments further. No AI is going to get that done.

I'd appreciate your guidance on this aspect, there aren't that many comment now.

[henderkes]

I'll look later.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 2 · 1

Open (4)

• len(options) is read outside the optionsMU lock, which introduces a data race if options can… · New
• strconv.Atoi will fail if the worker response includes a trailing newline or surrounding… · New
• The doc comment immediately preceding Validate currently starts with “Init…”, which will be… · New
• This comment references the “adapt endpoint”, but the test is calling POST /load. Update the… · New

Resolved since last review (2)

• Avoid quadratic allocations during worker initialization
• Validate misses per-server worker filename collisions

[nicolas-grekas]

Copilot review should be addressed + another issue I found with Validate() addressed also.