From 70dc595e6c7788a5b9b8305f88b62dbbd066a04f Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Sat, 22 Aug 2026 09:04:20 +0200 Subject: [PATCH 1/2] Revert "fix: removes opcache_hook again (#2564)" This reverts 3f43199a411d0ffc62b7f1072ae49503a79db5e3 and adds a regression test. Without the hook, an opcache restart is performed while worker threads are still executing code that lives in opcache shared memory, and the process dies with SIGSEGV. opcache defers a restart until accel_is_inactive(), which probes for a conflicting lock using fcntl F_GETLK on its own lock file. POSIX fcntl locks belong to the process and never conflict with locks that process already holds, so the probe reports "inactive" no matter how many threads of this process are inside a request. That check works for prefork SAPIs, where the accessors are other processes, and kill_all_lockers() makes the assumption explicit by signalling the locking pid. It does nothing for a single-process threaded server. Once the restart is carried out, accel_interned_strings_restore_state() memsets every string interned past the startup watermark and zend_shared_alloc_restore_state() rewinds each segment position, so the op_array a worker is executing is handed back out to the next compile. Rebooting threads when the restart is scheduled is what gets them out of shared memory before the rewind happens. The hook fires at most once per restart cycle, because zend_accel_schedule_restart() returns early while restart_pending is already set. Measured on dunglas/frankenphp:builder-php8.5 (PHP 8.5.9 ZTS): with the hook the new test passes and logs a thread reboot, without it the test binary dies with "signal: segmentation fault (core dumped)". --- frankenphp.c | 14 ++++++- opcache_test.go | 75 ++++++++++++++++++++++++++++++++++++ testdata/opcache/trigger.php | 36 +++++++++++++++++ testdata/opcache/worker.php | 30 +++++++++++++++ 4 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 opcache_test.go create mode 100644 testdata/opcache/trigger.php create mode 100644 testdata/opcache/worker.php diff --git a/frankenphp.c b/frankenphp.c index b15507f69d..aa37f5d9bc 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -1017,9 +1017,14 @@ PHP_FUNCTION(frankenphp_log) { } } +static void frankenphp_opcache_restart_hook(int reason) { + (void)reason; + go_schedule_opcache_reset(frankenphp_thread_index()); +} + /* {{{ thread-safe opcache reset */ PHP_FUNCTION(frankenphp_opcache_reset) { - go_schedule_opcache_reset(frankenphp_thread_index()); + frankenphp_opcache_restart_hook(0); RETVAL_TRUE; } /* }}} */ @@ -1705,6 +1710,13 @@ static void *php_main(void *arg) { frankenphp_sapi_module.startup(&frankenphp_sapi_module); +#if defined(ZTS) && PHP_VERSION_ID >= 80400 + /* Also restart everything on opcache memory overflow or similar events, the + * hook is triggered right before an opcache reset is scheduled + */ + zend_accel_schedule_restart_hook = frankenphp_opcache_restart_hook; +#endif + /* check if a default filter is set in php.ini and only filter if * it is, this is deprecated and will be removed in PHP 9 */ char *default_filter; diff --git a/opcache_test.go b/opcache_test.go new file mode 100644 index 0000000000..fe706e1bd4 --- /dev/null +++ b/opcache_test.go @@ -0,0 +1,75 @@ +package frankenphp_test + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestOpcacheRestartKeepsWorkerThreadsAlive guards the opcache restart hook. +// +// A worker holds references into opcache shared memory for its whole life. An +// opcache restart rewinds that memory to a startup watermark while the worker +// is still executing, because opcache defers a restart until +// accel_is_inactive(), which is an fcntl probe that cannot see threads of the +// calling process. The hook reboots every thread before that happens. +// +// Without the hook this test does not fail, it takes the process down with +// SIGSEGV, which is the regression being guarded. +func TestOpcacheRestartKeepsWorkerThreadsAlive(t *testing.T) { + t.Cleanup(func() { + _ = os.RemoveAll(filepath.Join(os.TempDir(), "frankenphp-opcache-restart-test")) + }) + + runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, _ int) { + get := func(path string) string { + w := httptest.NewRecorder() + handler(w, httptest.NewRequest("GET", "http://example.com"+path, nil)) + + return strings.TrimSpace(w.Body.String()) + } + + if body := get("/opcache/trigger.php?b=0"); body == "NOOPCACHE" { + t.Skip("opcache is not available in this build") + } + + require.Equal(t, "OK", get("/opcache/worker.php"), "worker must be healthy before the restart") + + // Compile until opcache performs a restart. The counters only move + // once a restart has actually been carried out, not merely scheduled. + restarted := false + for b := 1; b <= 20 && !restarted; b++ { + body := get("/opcache/trigger.php?b=" + strconv.Itoa(b)) + if n, err := strconv.Atoi(body); err == nil && n > 0 { + restarted = true + } + } + + if !restarted { + t.Skip("could not force an opcache restart in this environment") + } + + assert.Equal(t, "OK", get("/opcache/worker.php"), "worker must survive an opcache restart") + }, &testOptions{ + workerScript: "opcache/worker.php", + nbWorkers: 1, + nbParallelRequests: 1, + phpIni: map[string]string{ + "opcache.enable": "1", + "opcache.enable_cli": "1", + "opcache.memory_consumption": "8", + "opcache.interned_strings_buffer": "1", + "opcache.max_accelerated_files": "200", + "opcache.file_update_protection": "0", + "opcache.validate_timestamps": "1", + "opcache.revalidate_freq": "0", + }, + }) +} diff --git a/testdata/opcache/trigger.php b/testdata/opcache/trigger.php new file mode 100644 index 0000000000..b621e69ee6 --- /dev/null +++ b/testdata/opcache/trigger.php @@ -0,0 +1,36 @@ + 1, 'b' => [2, 3], 'c' => 'literal']; + +$str = 'opcache_canary_interned_string'; +$arr = OPCACHE_CANARY; + +// Fingerprints are plain integers on the thread heap, so a rewind cannot +// touch them. +$expLen = strlen($str); +$expCrc = crc32($str); +$expArr = crc32(serialize($arr)); + +$handler = static function () use (&$str, &$arr, $expLen, $expCrc, $expArr) { + if (strlen($str) !== $expLen || crc32($str) !== $expCrc || crc32(serialize($arr)) !== $expArr) { + echo 'CORRUPT'; + + return; + } + + echo 'OK'; +}; + +for ($running = true; $running;) { + $running = frankenphp_handle_request($handler); +} From ed1898fa97e3e2127d29568a1cd952ef009ebd2b Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Sat, 22 Aug 2026 09:28:56 +0200 Subject: [PATCH 2/2] fix: report repeated automatic opcache restarts Addresses the concern that motivated #2564: a runtime that invalidates heavily could restart opcache over and over, and rebooting the threads every time would turn that into a reboot loop. Reboots are not throttled. opcache rewinds its shared memory whether or not the threads are ready, so declining a reboot brings back the segfault the hook exists to prevent. Skipping is not a safe trade. What the loop actually means is that the compiled code does not fit in opcache.memory_consumption, so opcache can never settle: it fills, it overflows, it restarts, it fills again. Today that same configuration already thrashes, just silently. This makes it visible with an error naming the two ini settings that fix it, logged once per window rather than on every restart. go_schedule_opcache_reset() now knows whether the restart came from opcache or from a userland opcache_reset(). Only the automatic path is counted, so an operator calling opcache_reset() in a loop does not trigger the message. Reboot coalescing already exists: rebootAllThreads() returns early when a reboot is in flight. frankenphp_opcache_restart_hook() moves behind the same guard as the assignment in php_main(), which is now its only caller. Without that, builds for PHP 8.2 and 8.3 fail with -Werror=unused-function. --- frankenphp.c | 11 +++++++-- frankenphp.go | 53 +++++++++++++++++++++++++++++++++++++++--- opcacherestart_test.go | 46 ++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 5 deletions(-) create mode 100644 opcacherestart_test.go diff --git a/frankenphp.c b/frankenphp.c index aa37f5d9bc..f9927354b1 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -1017,14 +1017,21 @@ PHP_FUNCTION(frankenphp_log) { } } +/* Scheduled by opcache itself, on shared memory exhaustion or hash overflow. + * The reason is ignored: opcache_reset() is already intercepted below, so only + * the automatic reasons reach this hook. Guarded like the assignment in + * php_main(), which is its only caller, so builds without the hook do not trip + * -Werror=unused-function. */ +#if defined(ZTS) && PHP_VERSION_ID >= 80400 static void frankenphp_opcache_restart_hook(int reason) { (void)reason; - go_schedule_opcache_reset(frankenphp_thread_index()); + go_schedule_opcache_reset(frankenphp_thread_index(), true); } +#endif /* {{{ thread-safe opcache reset */ PHP_FUNCTION(frankenphp_opcache_reset) { - frankenphp_opcache_restart_hook(0); + go_schedule_opcache_reset(frankenphp_thread_index(), false); RETVAL_TRUE; } /* }}} */ diff --git a/frankenphp.go b/frankenphp.go index 79b135b808..92b4b07a72 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -773,11 +773,58 @@ func go_is_context_done(threadIndex C.uintptr_t) C.bool { return C.bool(phpThreads[threadIndex].handler.frankenPHPContext().isDone) } +const ( + // Bounds on how often opcache is expected to restart by itself. Beyond + // this the compiled code does not fit in the configured shared memory and + // opcache is thrashing, which only the operator can fix. + automaticOpcacheRestartWindow = time.Minute + automaticOpcacheRestartThreshold = 5 +) + +var automaticOpcacheRestarts struct { + sync.Mutex + windowStart time.Time + count int +} + +// recordAutomaticOpcacheRestart counts the restarts opcache schedules on its +// own and reports true the first time the threshold is crossed in a window, +// so the caller logs once per window instead of on every restart. +func recordAutomaticOpcacheRestart(now time.Time) bool { + automaticOpcacheRestarts.Lock() + defer automaticOpcacheRestarts.Unlock() + + if now.Sub(automaticOpcacheRestarts.windowStart) > automaticOpcacheRestartWindow { + automaticOpcacheRestarts.windowStart = now + automaticOpcacheRestarts.count = 0 + } + + automaticOpcacheRestarts.count++ + + return automaticOpcacheRestarts.count == automaticOpcacheRestartThreshold +} + //export go_schedule_opcache_reset -func go_schedule_opcache_reset(threadIndex C.uintptr_t) { - if mainThread != nil { - go mainThread.rebootAllThreads() +func go_schedule_opcache_reset(threadIndex C.uintptr_t, automatic C.bool) { + if mainThread == nil { + return } + + // A reboot is never skipped, not even when they pile up: opcache rewinds + // its shared memory whether or not the threads are ready for it, so + // declining to reboot brings back the crash the hook exists to prevent. + // Repeated automatic restarts are reported instead, since the fix is to + // give opcache more room. rebootAllThreads() already ignores a call made + // while a reboot is in flight. + if bool(automatic) && recordAutomaticOpcacheRestart(time.Now()) && globalLogger.Enabled(globalCtx, slog.LevelError) { + globalLogger.LogAttrs(globalCtx, slog.LevelError, + "opcache keeps restarting and PHP threads are rebooting each time, raise opcache.memory_consumption and opcache.max_accelerated_files", + slog.Int("restarts", automaticOpcacheRestartThreshold), + slog.Duration("window", automaticOpcacheRestartWindow), + ) + } + + go mainThread.rebootAllThreads() } func convertArgs(args []string) (C.int, []*C.char) { diff --git a/opcacherestart_test.go b/opcacherestart_test.go new file mode 100644 index 0000000000..4701399dd1 --- /dev/null +++ b/opcacherestart_test.go @@ -0,0 +1,46 @@ +package frankenphp + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestAutomaticOpcacheRestartsAreReportedOncePerWindow(t *testing.T) { + reset := func() { + automaticOpcacheRestarts.Lock() + automaticOpcacheRestarts.windowStart = time.Time{} + automaticOpcacheRestarts.count = 0 + automaticOpcacheRestarts.Unlock() + } + + t.Run("stays quiet below the threshold", func(t *testing.T) { + reset() + now := time.Now() + for i := 1; i < automaticOpcacheRestartThreshold; i++ { + assert.False(t, recordAutomaticOpcacheRestart(now), "restart %d must not report", i) + } + }) + + t.Run("reports exactly once when the threshold is crossed", func(t *testing.T) { + reset() + now := time.Now() + reported := 0 + for i := 0; i < automaticOpcacheRestartThreshold*3; i++ { + if recordAutomaticOpcacheRestart(now) { + reported++ + } + } + assert.Equal(t, 1, reported, "a burst must be reported once, not on every restart") + }) + + t.Run("restarts spread out never reach the threshold", func(t *testing.T) { + reset() + now := time.Now() + for i := 0; i < automaticOpcacheRestartThreshold*3; i++ { + now = now.Add(automaticOpcacheRestartWindow + time.Second) + assert.False(t, recordAutomaticOpcacheRestart(now), "occasional restarts are normal") + } + }) +}