From 0294138ed21d9933bfd1c73096da0089b32ad4bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 28 Jul 2026 17:25:05 +0200 Subject: [PATCH] docs: explain why the post-force-kill reboot wait must stay unbounded Bounding this wait and giving up on the stuck thread looks like the obvious fix for #2553 (one stuck thread wedging every reboot forever), and #2573 tried exactly that. It's unsafe: php_main() calls tsrm_shutdown() on every reboot cycle, which requires every PHP thread to have already exited. Giving up on a thread whose OS thread is still alive violates that and crashes the process (confirmed by CI on #2573). Leave a comment so the same fix isn't attempted again without this context. --- phpmainthread.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/phpmainthread.go b/phpmainthread.go index 175460ac1e..64c782319b 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -176,6 +176,24 @@ func (mainThread *phpMainThread) rebootAllThreads() bool { slog.String("timeout", rebootGracePeriod.String()), ) thread.sendKillSignal() + // This wait is intentionally unbounded. force_kill_thread only + // interrupts the Zend VM at the next opcode boundary or wakes a + // real blocking syscall via EINTR (see frankenphp_force_kill_thread); + // a thread parked in a blocking Go/cgo call (e.g. go_ub_write to a + // stalled client) can't be reached by either and may never yield. + // It's tempting to bound this wait and give up on the thread + // instead of blocking the whole reboot forever, but that's unsafe: + // php_main() in frankenphp.c runs this same teardown on every + // reboot, not just final shutdown, and calls tsrm_shutdown() as + // soon as mainThread.state becomes Rebooting. tsrm_shutdown() + // requires every PHP thread to have already exited - giving up on + // one while its OS thread is still alive and registered in TSRM + // tears down global engine state out from under a thread that's + // still using it, a use-after-free confirmed by CI when this was + // tried (see the discussion on php/frankenphp#2573). The safe + // mitigations are reducing how often a thread ever gets stuck like + // this in the first place (see WithResponseWriteTimeout and the + // opcache_reset throttle), not recovering from it after the fact. thread.state.WaitFor(state.YieldingForReboot) }) }