Repository navigation
fix: don't read PHP globals when restarting an unhealthy thread - #2696
Merged
dunglas merged 1 commit intoOct 10, 2026
Merged
Conversation
frankenphp_log_message() reads SG(server_context), whose TSRM storage ts_free_thread() has already released, so restarting an unhealthy thread segfaulted the whole process.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The targeted fix addresses both unsafe log calls, with regression coverage and no unresolved findings.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes #2688 by avoiding access to freed PHP globals when logging an unhealthy thread’s restart.
Changes:
- Passes the thread-local index directly to both restart log calls.
- Adds a regression test checking request recovery and restart logging after startup failure.
| File | Description |
|---|---|
| frankenphp.c | Makes restart logging safe after PHP thread teardown. |
| frankenphp_test.go | Tests unhealthy-thread replacement and subsequent request handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
|
Thanks! |
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
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.
When a thread is marked unhealthy,
php_thread()callsts_free_thread()and then logs "Restarting unhealthy thread" throughfrankenphp_log_message(), which readsSG(server_context). The thread no longer has TSRM storage at that point, so the read faults and the whole process segfaults instead of the thread being replaced. This started in v1.12.4, when #2438 routed that log call throughfrankenphp_thread_index().Both log calls on that path now pass the thread-local
thread_indextogo_log(), like thego_frankenphp_on_thread_shutdown()andfrankenphp_new_php_thread()calls next to them.TestUnhealthyThreadIsRestartedmakes request startup fail with a query string larger thanmemory_limit, then checks that the next request is served and the restart is logged. Without the fix, the test crashes the process.Fixes #2688