Conversation
Caddy stops apps without dependency ordering. Shutting down PHP from the FrankenPHP app Stop method can break requests while HTTP remains open. Register one global OnExit callback after all apps have drained instead, while preserving the existing reload shutdown path. Add a process-level regression for service during shutdown_delay, in-flight request draining, PHP teardown, and successful process exit. Run it in the existing Linux PHP matrix using the already-built CLI. Validation on current main with Linux ARM64, PHP 8.5.9 and Go 1.27.1: patched regression and existing reload integration pass; unpatched main fails the regression. Go formatting, Bash syntax, ShellCheck and actionlint pass. Full PHP-version matrix and worker-mode shutdown not run locally.
Contributor
|
Makes sense 👍 . Could you maybe also test that a worker script is still able to do graceful shutdown, eg: # worker.php
while(frankenphp_handle_request($handler)){
}
# Make sure this code here still runs in a test, for example by creating a temp file
shutdownlogic(); |
Exercise a real worker request before SIGTERM and require cleanup code after the frankenphp_handle_request loop to finish before process exit. Check the marker is absent before shutdown and contains the expected value afterward, with a bounded wait and successful server exit. Validated the full shutdown script against the patched Linux ARM64 runtime with PHP 8.5.9: all five request-draining cycles and worker cleanup pass. Omitting the fixture cleanup makes the new assertion fail. Bash syntax, ShellCheck and git diff checks pass.
nesl247
marked this pull request as ready for review
September 14, 2026 14:31
AlliBalliBaba
approved these changes
Sep 16, 2026
AlliBalliBaba
left a comment
AlliBalliBaba
left a comment
Contributor
There was a problem hiding this comment.
Probably also makes senso to move shutdown_test.sh into the tests folder
Author
I kept it where the reset test is. I can move them both here, but I wouldn't do one and not the other IMO. |
Contributor
|
Yeah I'd move them both tbh |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2644.
Caddy stops its apps without dependency ordering. Calling
frankenphp.Shutdown()fromFrankenPHPApp.Stop()can therefore stop PHP while the HTTP app is still accepting requests duringshutdown_delay, producing HTTP 500 responses.Register a single process-global
caddy.OnExitcallback so PHP stays available until all Caddy apps have stopped, including the HTTP shutdown delay and in-flight request draining. Keep the existing reload shutdown path unchanged.Add a process-level regression to the existing Linux test workflow, using the CLI it already builds. Five fresh processes exercise the unordered stop path. The test verifies PHP responses during the shutdown delay, an in-flight response spanning listener closure, actual PHP teardown, and clean process exit. A bounded release handshake avoids waiting for a fixed long-running request.
Validated against current
main(daead870980d2aba0c2df76cbea880b5a608ff0b) on Linux ARM64 with PHP 8.5.9 and Go 1.27.1:mainafter SIGTERM.reload_test.shintegration check passes.The full PHP-version matrix and worker-mode shutdown were not run locally. The regression is wired into the existing PHP 8.2–8.5 Linux jobs. Feedback on the process-exit lifecycle hook is welcome.