Revert "fix: removes opcache_hook again (#2564)"

frankenphp.c

@@ -1017,9 +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(), true);
+}
+#endif
+
 /* {{{ thread-safe opcache reset */
 PHP_FUNCTION(frankenphp_opcache_reset) {
-  go_schedule_opcache_reset(frankenphp_thread_index());
+  go_schedule_opcache_reset(frankenphp_thread_index(), false);
 
   RETVAL_TRUE;
 } /* }}} */
@@ -1705,6 +1717,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;

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) {

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",
+		},
+	})
+}

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")
+		}
+	})
+}

testdata/opcache/trigger.php

@@ -0,0 +1,36 @@
+<?php
+
+// Runs on a regular thread. Compiles fresh scripts until the opcache hash
+// overflows, which schedules a restart, then reports how many restarts have
+// been performed so the caller can stop as soon as one lands.
+if (!function_exists('opcache_get_status')) {
+    echo 'NOOPCACHE';
+
+    return;
+}
+
+$dir = sys_get_temp_dir() . '/frankenphp-opcache-restart-test';
+@mkdir($dir, 0777, true);
+
+$batch = (int) ($_GET['b'] ?? 0);
+for ($i = 0; $i < 120; $i++) {
+    $file = $dir . '/g' . $batch . '_' . $i . '.php';
+    file_put_contents($file, "<?php\nfunction fn_{$batch}_{$i}() { return {$i}; }\n" . str_repeat("// pad\n", 40));
+    @include_once $file;
+}
+
+// Discarding scripts grows wasted_shared_memory, which is what lets the
+// hash-full path schedule a restart.
+foreach (glob($dir . '/*.php') as $file) {
+    @opcache_invalidate($file, true);
+}
+
+$status = @opcache_get_status(false);
+if (!is_array($status) || !isset($status['opcache_statistics'])) {
+    echo 'NOOPCACHE';
+
+    return;
+}
+
+$s = $status['opcache_statistics'];
+echo (int) ($s['oom_restarts'] + $s['hash_restarts'] + $s['manual_restarts']);

testdata/opcache/worker.php

@@ -0,0 +1,30 @@
+<?php
+
+// Keeps references into opcache shared memory alive across requests: the
+// literal is an interned string and the const array is IS_ARRAY_IMMUTABLE.
+// Both live in memory that opcache rewinds on a restart, so if a restart is
+// performed while this worker runs, the process dies.
+const OPCACHE_CANARY = ['a' => 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);
+}