feat: validate a configuration before it replaces the running one - #2661
nicolas-grekas wants to merge 1 commit into
Conversation
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.
| close(f.started) | ||
| }() | ||
|
|
||
| opts, err := f.collectOptions(caddy.NewReplacer(), true) |
There was a problem hiding this comment.
configureHotReload calls WithHotReload during provisioning so this is going to discard that, I think that's why CI is red
| // 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) |
There was a problem hiding this comment.
if this ends up called before provisioning (see last comment) this is going to ignore php_server block works, and if one is incorrectly configured, it'll pass validation but still fail actual reload
| 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) | ||
| } |
There was a problem hiding this comment.
this only checks worker name duplicates for global workers (o.server == nil)... so what about php_server module workers? I assume two will pass check but fail init?
|
Also, I did not read the comments during this review... too many comments, too many of them useless. |
There was a problem hiding this comment.
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)
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鈥檚 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/鈥媗ibrary.md |
Documents the validation API. |
caddy/鈥媋pp.go |
Adds validation and refactors option collection. |
caddy/鈥媋dmin_test.go |
Tests rejected reload continuity. |
caddy/鈥媍onfig_test.go |
Updates worker-name tests. |
caddy/鈥媠erveridx_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.
| 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 | ||
| } |
Start()callsShutdown()beforeInit(), 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 optionsInit()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 onesInit()runs,newWorker()now shares them instead of holding its own copy.The Caddy app implements
caddy.Validatoron top of it, which Caddy calls while the previous configuration still serves:and the site that was running keeps serving, which
TestRejectedReloadKeepsThePreviousSiteServingchecks 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)