From 11f7f02e8f262e7a8274f7079c29f5726e5c25b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 28 Jul 2026 10:19:02 +0200 Subject: [PATCH 1/4] fix: bound the reboot wait so an unkillable thread can't wedge the pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit frankenphp_force_kill_thread() only interrupts the Zend VM at the next opcode boundary. A thread parked in a blocking write to a stalled client (e.g. an unresponsive HTTP/2 peer) has already left PHP execution and can't be reached by it. rebootAllThreads()'s post-force-kill wait was unbounded (thread.state.WaitFor(state.YieldingForReboot) with no timeout), so a single such thread would hang the whole reboot forever: scalingMu stays locked, isRebooting never clears, and every other thread's reboot blocks on it too. The same unbounded wait exists one level up in RequestSafeStateChange(), so Shutdown() then hangs the same way on that thread. Bound the wait with another rebootGracePeriod after the kill signal. If the thread still hasn't yielded, give up on it: introduce a terminal Abandoned state so the thread permanently drops out of Ready/Inactive (dispatch and shutdown then skip it cleanly instead of panicking or hanging) rather than risk reusing a slot whose OS thread might still be alive. This is very likely the actual mechanism behind the escalating, un-recovering slowdown in https://github.com/php/frankenphp/issues/2553 (see the go_read_post/go_ub_write stall discussion on that issue and https://github.com/php/frankenphp/pull/2570#issuecomment-5099392587): any full-pool reboot — whether triggered by opcache_reset(), the watcher, or RestartWorkers() — that catches even one thread mid-write to a stalled client would wedge permanently, independent of how often reboots are triggered or by what. Reproduced with TestRebootDoesNotHangOnUnkillableThread, using a custom http.ResponseWriter whose Write() blocks forever to deterministically simulate a stalled client: hangs indefinitely on the unpatched code, resolves within ~12s (two grace periods) with the fix, and the pool remains usable afterwards. --- internal/state/state.go | 14 +++++- phpmainthread.go | 36 +++++++++++++- reboot_stuck_thread_test.go | 95 +++++++++++++++++++++++++++++++++++++ threadinactive.go | 2 +- threadregular.go | 5 +- threadworker.go | 2 +- 6 files changed, 147 insertions(+), 7 deletions(-) create mode 100644 reboot_stuck_thread_test.go diff --git a/internal/state/state.go b/internal/state/state.go index 37e4c2e696..aad733cedb 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -34,6 +34,13 @@ const ( RebootReady // all threads are yielding for main thread reboot YieldingForReboot + + // thread failed to yield even after a force-kill (e.g. stuck in a + // blocking write to a stalled client, unreachable by force-kill, which + // only interrupts the Zend VM at the next opcode boundary). Terminal: + // the underlying OS thread may still be alive, so its slot is abandoned + // rather than reused, permanently excluding it from dispatch. + Abandoned ) func (s State) String() string { @@ -66,6 +73,8 @@ func (s State) String() string { return "reboot ready" case YieldingForReboot: return "yielding for reboot" + case Abandoned: + return "abandoned" default: return "unknown" } @@ -201,8 +210,9 @@ func (ts *ThreadState) WaitForStateWithTimeout(timeout time.Duration, states ... func (ts *ThreadState) RequestSafeStateChange(nextState State) bool { ts.mu.Lock() switch ts.currentState { - // disallow state changes when already shutting down or done - case Reserved, ShuttingDown, Done: + // disallow state changes when already shutting down, done, or abandoned + // (abandoned is terminal: the thread never reaches a stable state again) + case Reserved, ShuttingDown, Done, Abandoned: ts.mu.Unlock() return false diff --git a/phpmainthread.go b/phpmainthread.go index 175460ac1e..dc03675810 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -159,6 +159,12 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { } } + // force_kill_thread only interrupts the Zend VM at the next opcode + // boundary (see frankenphp_force_kill_thread): a thread parked in a + // blocking write to a stalled client (e.g. an unresponsive HTTP/2 peer) + // has already left PHP execution and can't be reached by it. Give up on + // such threads instead of waiting on them forever, so one stuck thread + // can't wedge the reboot of every other thread. for _, thread := range rebootingThreads { rebootWg.Go(func() { close(thread.drainChan) @@ -176,7 +182,24 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { slog.String("timeout", rebootGracePeriod.String()), ) thread.sendKillSignal() - thread.state.WaitFor(state.YieldingForReboot) + if thread.state.WaitForStateWithTimeout(rebootGracePeriod, state.YieldingForReboot) { + return + } + + // still hasn't yielded: the underlying OS thread may still be alive + // (e.g. stuck in a blocking write), so its slot can't be reused + // without risking two OS threads sharing the same TSRM slot. + // Abandon it permanently rather than block every other thread's + // reboot on it; it stays out of Ready/Inactive forever, so + // dispatch naturally stops routing requests to it. + globalLogger.LogAttrs( + globalCtx, + slog.LevelError, + "thread unresponsive after force-kill, abandoning it permanently", + slog.String("name", thread.name()), + slog.String("state", thread.state.Name()), + ) + thread.state.Set(state.Abandoned) }) } @@ -189,11 +212,19 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { } mainThread.state.WaitFor(state.Ready) + rebootedThreads := 0 + abandonedThreads := 0 for _, thread := range rebootingThreads { + if thread.state.Is(state.Abandoned) { + abandonedThreads++ + continue + } + thread.drainChan = make(chan struct{}) if thread.state.CompareAndSwap(state.YieldingForReboot, state.RebootReady) { // wait for any of the stable states thread.state.WaitFor(state.Ready, state.Inactive, state.Reserved) + rebootedThreads++ } else { panic("unexpected state on reboot: " + thread.state.Name() + " for thread " + thread.name()) } @@ -204,7 +235,8 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { slog.LevelInfo, "thread reboot finished", slog.String("duration", time.Since(rebootStart).String()), - slog.Int("num_threads", len(rebootingThreads)), + slog.Int("num_threads", rebootedThreads), + slog.Int("abandoned_threads", abandonedThreads), ) return true diff --git a/reboot_stuck_thread_test.go b/reboot_stuck_thread_test.go new file mode 100644 index 0000000000..ec17ed8de8 --- /dev/null +++ b/reboot_stuck_thread_test.go @@ -0,0 +1,95 @@ +package frankenphp_test + +import ( + "fmt" + "net/http" + "net/http/httptest" + "os" + "sync" + "testing" + "time" + + "github.com/dunglas/frankenphp" + "github.com/stretchr/testify/require" +) + +// blockingResponseWriter simulates a client whose connection is stalled: any +// Write() call after headers are sent blocks forever. This reproduces a PHP +// thread stuck inside go_ub_write with no way for FrankenPHP's force-kill +// (which only interrupts the Zend VM at the next opcode boundary, see +// frankenphp_force_kill_thread) to unstick it, since the thread has already +// left PHP execution and is parked in a Go-level network write. +type blockingResponseWriter struct { + header http.Header + started chan struct{} + once sync.Once +} + +func newBlockingResponseWriter() *blockingResponseWriter { + return &blockingResponseWriter{header: make(http.Header), started: make(chan struct{})} +} + +func (w *blockingResponseWriter) Header() http.Header { return w.header } + +func (w *blockingResponseWriter) WriteHeader(int) {} + +func (w *blockingResponseWriter) Write(p []byte) (int, error) { + w.once.Do(func() { close(w.started) }) + select {} // block forever, like a write to a dead/stalled TCP connection +} + +// TestRebootDoesNotHangOnUnkillableThread reproduces the hang described on +// https://github.com/php/frankenphp/pull/2570#issuecomment-5099392587 and +// https://github.com/php/frankenphp/issues/2553: a thread genuinely stuck in +// a blocking write (e.g. to a stalled HTTP/2 client) can't be interrupted by +// force-kill, so rebootAllThreads()'s unbounded post-force-kill wait blocks +// forever, wedging the whole pool (scalingMu never released, isRebooting +// never cleared) rather than just losing the one stuck thread. +func TestRebootDoesNotHangOnUnkillableThread(t *testing.T) { + require.NoError(t, frankenphp.Init(frankenphp.WithNumThreads(2), frankenphp.WithMaxThreads(2))) + defer frankenphp.Shutdown() + + cwd, _ := os.Getwd() + + w := newBlockingResponseWriter() + go func() { + req := httptest.NewRequest("GET", "http://example.com/index.php", nil) + fr, err := frankenphp.NewRequestWithContext(req, frankenphp.WithRequestDocumentRoot(cwd+"/testdata/", false)) + if err != nil { + t.Errorf("NewRequestWithContext: %v", err) + return + } + // echo.php-style output: any script that writes output triggers go_ub_write + _ = frankenphp.ServeHTTP(w, fr) + }() + + select { + case <-w.started: + case <-time.After(5 * time.Second): + t.Fatal("the stuck request never started writing") + } + + // give go_ub_write a moment to actually be blocked in Write() + time.Sleep(100 * time.Millisecond) + + done := make(chan struct{}) + go func() { + frankenphp.RestartWorkers() + close(done) + }() + + select { + case <-done: + case <-time.After(20 * time.Second): + t.Fatal("RestartWorkers() hung forever on a thread stuck in a blocking write (force-kill can't interrupt it)") + } + + // the pool must still be usable after giving up on the stuck thread: a + // fresh request on a *new* writer must complete normally. + body, _ := testGet(fmt.Sprintf("http://example.com/index.php?i=%d", 1), func(rw http.ResponseWriter, r *http.Request) { + fr, err := frankenphp.NewRequestWithContext(r, frankenphp.WithRequestDocumentRoot(cwd+"/testdata/", false)) + require.NoError(t, err) + require.NoError(t, frankenphp.ServeHTTP(rw, fr)) + }, t) + require.Contains(t, body, "I am by birth a Genevese") +} diff --git a/threadinactive.go b/threadinactive.go index d0ee74390b..1ef1e0b87e 100644 --- a/threadinactive.go +++ b/threadinactive.go @@ -34,7 +34,7 @@ func (handler *inactiveThread) beforeScriptExecution() string { thread.state.MarkAsWaiting(false) return handler.beforeScriptExecution() - case state.Rebooting, state.ForceRebooting: + case state.Rebooting, state.ForceRebooting, state.Abandoned: return "" case state.RebootReady: thread.state.Set(state.Inactive) diff --git a/threadregular.go b/threadregular.go index 283627334f..0474970ca8 100644 --- a/threadregular.go +++ b/threadregular.go @@ -53,7 +53,10 @@ func (handler *regularThread) beforeScriptExecution() string { return handler.waitForRequest() case state.Ready: return handler.waitForRequest() - case state.Rebooting, state.ForceRebooting: + case state.Rebooting, state.ForceRebooting, state.Abandoned: + // Abandoned: this thread failed to yield during a reboot and was + // given up on (see rebootAllThreads); if the blocking call it was + // stuck in ever does return, just let the underlying thread exit. return "" case state.RebootReady: handler.requestCount = 0 diff --git a/threadworker.go b/threadworker.go index 843e1dd331..045959ea32 100644 --- a/threadworker.go +++ b/threadworker.go @@ -56,7 +56,7 @@ func (handler *workerThread) beforeScriptExecution() string { setupWorkerScript(handler, handler.worker) return handler.worker.fileName - case state.Rebooting, state.ForceRebooting: + case state.Rebooting, state.ForceRebooting, state.Abandoned: return "" case state.RebootReady: handler.requestCount = 0 From 6afe7618a1902f4b2196fb3fa133890577a4ad67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 28 Jul 2026 11:12:56 +0200 Subject: [PATCH 2/4] test: verify Shutdown() doesn't hang on an abandoned thread TestShutdownDoesNotHangWithAbandonedThread reboots into an Abandoned thread (same setup as TestRebootDoesNotHangOnUnkillableThread) and then calls Shutdown() while it's still stuck. Confirmed by temporarily reverting Abandoned's addition to RequestSafeStateChange's terminal-state bucket: the test then hangs, since RequestSafeStateChange recurses forever waiting for Ready/Inactive/Reserved on a thread that will never reach any of them. --- reboot_stuck_thread_test.go | 56 +++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/reboot_stuck_thread_test.go b/reboot_stuck_thread_test.go index ec17ed8de8..72e7b0c74e 100644 --- a/reboot_stuck_thread_test.go +++ b/reboot_stuck_thread_test.go @@ -93,3 +93,59 @@ func TestRebootDoesNotHangOnUnkillableThread(t *testing.T) { }, t) require.Contains(t, body, "I am by birth a Genevese") } + +// TestShutdownDoesNotHangWithAbandonedThread proves that Shutdown() itself +// bounds its wait once a thread has been abandoned: before Abandoned was a +// real terminal state, RequestSafeStateChange's unbounded +// WaitFor(Ready, Inactive, Reserved) fallback (and thread.shutdown()'s own +// unbounded WaitFor(Done) after a failed force-kill) would recurse/block +// forever on the same stuck thread that rebootAllThreads() gave up on. +func TestShutdownDoesNotHangWithAbandonedThread(t *testing.T) { + require.NoError(t, frankenphp.Init(frankenphp.WithNumThreads(2), frankenphp.WithMaxThreads(2))) + + cwd, _ := os.Getwd() + + w := newBlockingResponseWriter() + go func() { + req := httptest.NewRequest("GET", "http://example.com/index.php", nil) + fr, err := frankenphp.NewRequestWithContext(req, frankenphp.WithRequestDocumentRoot(cwd+"/testdata/", false)) + if err != nil { + t.Errorf("NewRequestWithContext: %v", err) + return + } + _ = frankenphp.ServeHTTP(w, fr) + }() + + select { + case <-w.started: + case <-time.After(5 * time.Second): + t.Fatal("the stuck request never started writing") + } + time.Sleep(100 * time.Millisecond) + + // Reboot first: this is what actually abandons the stuck thread. + rebootDone := make(chan struct{}) + go func() { + frankenphp.RestartWorkers() + close(rebootDone) + }() + + select { + case <-rebootDone: + case <-time.After(20 * time.Second): + t.Fatal("RestartWorkers() hung") + } + + // Shutdown() must not wait forever on the now-abandoned thread. + shutdownDone := make(chan struct{}) + go func() { + frankenphp.Shutdown() + close(shutdownDone) + }() + + select { + case <-shutdownDone: + case <-time.After(10 * time.Second): + t.Fatal("Shutdown() hung forever on the abandoned thread") + } +} From f01a379e7afb16416246d4ef64defa34584317d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 28 Jul 2026 15:16:07 +0200 Subject: [PATCH 3/4] fix: don't escalate to a raw kill signal against an already-unresponsive thread CI caught a segfault: TestRebootDoesNotHangOnUnkillableThread crashed the whole process on every Linux job (PHP 8.2-8.5), reproducing consistently. Not reproducible on macOS or on a from-source Linux/arm64 debug ZTS build, which points at the pthread_kill(SIGRTMIN+3) call itself, only sent on Linux/FreeBSD. A thread that ignores a full rebootGracePeriod has almost certainly already left PHP execution and is parked inside Go's own runtime (e.g. go_ub_write's netpoller wait for a stalled client) rather than a PHP-recognized C syscall. force_kill_thread's signal can't reach that at all (it's not a syscall Go's scheduler is blocked in), so sending it does nothing useful for the one scenario this code exists to handle, and delivering an async signal into an OS thread that's inside Go's runtime internals - not a syscall PHP knows about - risks corrupting shared process state instead. Replace the sendKillSignal() escalation with a second, signal-free grace period before giving up. This predates this branch (sendKillSignal was already called here); this branch's new test is just the first thing that exercised it against a permanently Go-blocked thread all the way through. thread.shutdown()'s own sendKillSignal() call (phpthread.go) has the same theoretical exposure but is unchanged here - it's pre-existing, untested against this scenario, and out of scope for this PR. --- phpmainthread.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/phpmainthread.go b/phpmainthread.go index dc03675810..38902eb551 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -165,6 +165,16 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { // has already left PHP execution and can't be reached by it. Give up on // such threads instead of waiting on them forever, so one stuck thread // can't wedge the reboot of every other thread. + // + // Do NOT escalate to sendKillSignal() here: on Linux/FreeBSD that's a + // real pthread_kill, which is only safe against a thread blocked in a + // PHP-recognized C syscall. A thread that ignored a full grace period + // has almost certainly already left PHP execution entirely and is + // parked inside Go's own runtime (e.g. go_ub_write's netpoller wait) - + // delivering an async signal there doesn't unstick it (Go's scheduler + // isn't a syscall the signal can interrupt) and, confirmed on Linux/ + // amd64 CI, can corrupt shared process state and crash the whole + // server. That's strictly worse than losing one thread's capacity. for _, thread := range rebootingThreads { rebootWg.Go(func() { close(thread.drainChan) @@ -172,16 +182,14 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { return } - // thread has not shut down in time, force kill the thread on the PHP side globalLogger.LogAttrs( globalCtx, slog.LevelWarn, - "force-killing thread on reboot timeout", + "thread did not yield within the grace period, waiting once more before giving up", slog.String("name", thread.name()), slog.String("state", thread.state.Name()), slog.String("timeout", rebootGracePeriod.String()), ) - thread.sendKillSignal() if thread.state.WaitForStateWithTimeout(rebootGracePeriod, state.YieldingForReboot) { return } @@ -195,7 +203,7 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { globalLogger.LogAttrs( globalCtx, slog.LevelError, - "thread unresponsive after force-kill, abandoning it permanently", + "thread still unresponsive after grace periods, abandoning it permanently", slog.String("name", thread.name()), slog.String("state", thread.state.Name()), ) From 1e3b84536644f583f06e3cffda272a86d11ba0fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 28 Jul 2026 15:31:19 +0200 Subject: [PATCH 4/4] Revert "fix: don't escalate to a raw kill signal against an already-unresponsive thread" This reverts commit f01a379e7afb16416246d4ef64defa34584317d7. --- phpmainthread.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/phpmainthread.go b/phpmainthread.go index 38902eb551..dc03675810 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -165,16 +165,6 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { // has already left PHP execution and can't be reached by it. Give up on // such threads instead of waiting on them forever, so one stuck thread // can't wedge the reboot of every other thread. - // - // Do NOT escalate to sendKillSignal() here: on Linux/FreeBSD that's a - // real pthread_kill, which is only safe against a thread blocked in a - // PHP-recognized C syscall. A thread that ignored a full grace period - // has almost certainly already left PHP execution entirely and is - // parked inside Go's own runtime (e.g. go_ub_write's netpoller wait) - - // delivering an async signal there doesn't unstick it (Go's scheduler - // isn't a syscall the signal can interrupt) and, confirmed on Linux/ - // amd64 CI, can corrupt shared process state and crash the whole - // server. That's strictly worse than losing one thread's capacity. for _, thread := range rebootingThreads { rebootWg.Go(func() { close(thread.drainChan) @@ -182,14 +172,16 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { return } + // thread has not shut down in time, force kill the thread on the PHP side globalLogger.LogAttrs( globalCtx, slog.LevelWarn, - "thread did not yield within the grace period, waiting once more before giving up", + "force-killing thread on reboot timeout", slog.String("name", thread.name()), slog.String("state", thread.state.Name()), slog.String("timeout", rebootGracePeriod.String()), ) + thread.sendKillSignal() if thread.state.WaitForStateWithTimeout(rebootGracePeriod, state.YieldingForReboot) { return } @@ -203,7 +195,7 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { globalLogger.LogAttrs( globalCtx, slog.LevelError, - "thread still unresponsive after grace periods, abandoning it permanently", + "thread unresponsive after force-kill, abandoning it permanently", slog.String("name", thread.name()), slog.String("state", thread.state.Name()), )