fix: safe opcache_reset by AlliBalliBaba · Pull Request #2073 · php/frankenphp

AlliBalliBaba · 2025-12-14T22:19:13Z

Idea to fix #1737

Just a WIP, to test in CI.

# Conflicts: # caddy/caddy_test.go

alexandre-daubois · 2026-03-12T08:23:58Z

Hi @AlliBalliBaba, any news on this side? 🙂
I think I stumbled across the very same issue with #2265

AlliBalliBaba · 2026-03-12T10:10:51Z

I experimented a lot with juggling thread states, but not sure it's possible to make a reset fully safe from our side alone.

There are 2 main race conditions:

A thread parses a script without having restarted after a opcache_reset has been triggered.
Multiple threads trigger an opcache_reset simultaneously.

The first race condition kind of gets mitigated by the changes in this PR. THe second one would need to somehow be fixed in php-src or requires a hook to reject opcache_resets.

withinboredom · 2026-03-12T10:18:34Z

Can we try doing what we do with the environment functions and simply override/replace the existing one:

take a global write lock (read lock taken before executing a php script)
do the real reset
release the lock

AlliBalliBaba · 2026-03-12T11:13:57Z

Yeah I tried that as well. But an opcache_reset can also happen for other reasons, like after calling opcache_invalidate repeatedly because memory is filling up.

But maybe overwriting the user function would still be a good first step 👍

henderkes · 2026-03-13T06:34:39Z

Can we try doing what we do with the environment functions and simply override/replace the existing one:

take a global write lock (read lock taken before executing a php script)

do the real reset

release the lock

I tried this and it kind of works, but workers are an issue. Only got it working by locking new requests, draining all workers, doing the reset, restarting all workers and then unlocking.

AlliBalliBaba · 2026-03-13T13:02:20Z

Oh I just realized that overwriting opcache and restarting workers was also exactly what this branch does. Do you hard-link against the opcache extension in your solution @henderkes? When hard-linking and calling the reset directly, this can probably be done much more cleanly.
Simply calling the original reset function will make some Docker image builds still fail with segfaults.

henderkes · 2026-03-13T13:04:38Z

Yes, I hard linked because opcache is always active in 8.5+ (which is where I test). I'll tidy up my changes and merge them on top of this branch when I get to it.

AlliBalliBaba · 2026-03-13T13:18:57Z

Maybe we can make it a requirement also for other PHP versions, not sure how many installations there are without it.
Solution would still be the same, all threads need to stop completely, but we would force a flush instead of letting PHP schedule one.

henderkes · 2026-03-13T13:36:19Z

You already had logic to check whether opcache exists in the branch so I only had to pick in a minor change here. Let's see if tests pass.

Edit: well, apparently not. I only tested with 8.5 locally and guess what's passing x)

Okay, I think I get the issue now. Hard linking to opcache won't help because the init chain for shared extensions is different than for static extensions, which is why it works for me locally, but not in CI in < 8.5. Need to wait with actually calling opcache reset until the extension is properly loaded.

henderkes · 2026-03-13T15:58:58Z

Alright, we're down to php 8.2 segfaulting. I'm not terribly keep to find out why.

henderkes · 2026-03-14T03:57:02Z

Down to only debian 8.2 failing...

henderkes · 2026-03-15T02:47:31Z

Ah sry @henderkes, thought you knew from this also happening in #2272. The opcache preload tests were always segfaulting in 8.2. The PR that added the tests (#2257) did not trigger the Docker Images build, so this went under the radar. Still haven't looked into why they are failing. Worst case scenario, we'll remove them again.

I looked at the last 3 runs on the main branch and they were green with the test passing, so I thought it had to be connected to this branch. Extremely annoying, but kind of my own fault for not remembering them.

I've removed the 8.2 specific workarounds here so it does the safe session reset for every php version. Only 9 months to go for 8.2 support anyway...

AlliBalliBaba · 2026-03-15T19:20:22Z

 func RestartWorkers() {
-	// disallow scaling threads while restarting workers
-	scalingMu.Lock()
-	defer scalingMu.Unlock()
-
-	threadsToRestart := drainWorkerThreads()
-
-	for _, thread := range threadsToRestart {
-		thread.drainChan = make(chan struct{})
-		thread.state.Set(state.Ready)
-	}
+	restartThreadsAndOpcacheReset(false)
 }


I wonder if restartWorkers should also restart regular threads, I suspect that might be what's causing #2265

I also want to try under which conditions opcache_reset is not strictly necessary with restart_workers, something for a different PR.

Signed-off-by: Marc <m@pyc.ac>

AlliBalliBaba · 2026-03-16T21:37:19Z

Final test for this branch would be the moodle installation from #1737 if you get around to try it. Otherwise I can try setting it up again.

henderkes · 2026-03-17T02:51:23Z

I won't have the time for that this week, would be great if you could pick it up!

henderkes · 2026-03-24T06:44:19Z

Final test for this branch would be the moodle installation from #1737 if you get around to try it. Otherwise I can try setting it up again.

It no longer crashes and the container keeps running, but with so many opcache resets scheduled it times some requests out.

henderkes

@dunglas @withinboredom @alexandre-daubois ready for review now.

alexandre-daubois · 2026-03-24T09:33:37Z

I have yet to review the code more deeply but I can confirm this fixed my issue!

AlliBalliBaba · 2026-03-24T20:10:31Z

It no longer crashes and the container keeps running, but with so many opcache resets scheduled it times some requests out.

Hmm requests timing out means there might be deadlock somewhere..

Maybe we can also wait for the logic from #2292 to completely reset all threads as an alternate approach.

henderkes · 2026-03-24T23:37:36Z

It no longer crashes and the container keeps running, but with so many opcache resets scheduled it times some requests out.

Hmm requests timing out means there might be deadlock somewhere..

Maybe we can also wait for the logic from #2292 to completely reset all threads as an alternate approach.

Oh no, I don't think so. I usually set timeout to 5s while testing. Not sure why the moodle installation takes so much time at the start, once it's installed nothing times out.

I'll see if increasing timeout from a fresh installation still runs into the timeouts, but I don't think so.

AlliBalliBaba · 2026-03-25T11:29:26Z

Hmm when increasing the number of requests to 1000 in the OpcacheTest I'm also getting the deadlock, I think you've discovered yet another race condition 😩. Not in our implementation, but probably opcache.

At this point I think skipping opcache and doing a clean thread reset might be the better approach.

henderkes · 2026-03-25T14:38:53Z

The deadlock just came down to our tests. When an assertion failed it never called wg.Done(). I've increased the timeout to accommodate for the opcache resets + sleeps and it works now.

PR php#2073 drains all threads for explicit opcache_reset() calls, but WordPress triggers implicit restarts via opcache_invalidate() filling opcache memory → zend_accel_schedule_restart() → restart_pending → the actual reset runs during the next php_request_startup(). That path bypasses the opcache_reset() override and races on shared memory. Add a pthread_rwlock around the request lifecycle: - Normal requests: read lock (concurrent, single atomic CAS) - When zend_accel_schedule_restart_hook fires: set an atomic flag - Next php_request_startup(): sees flag → write lock (exclusive), blocks until all current requests complete, resets safely - After startup: unlock, all threads resume Combined with PR php#2073, this covers both explicit opcache_reset() (thread drain) and implicit OOM/hash-overflow restarts (rwlock). Testing: without this patch, crashes every ~10 min on a WordPress multisite. With PR php#2073 alone, same crash frequency. With the rwlock from our PR php#2349 alone, crashes reduced to every 6-12 hours (worker threads not covered). Combined fix expected to eliminate both paths.

dunglas

So let's merge?

dunglas · 2026-04-17T07:08:19Z

+	numRequests := 1000
+	wg.Add(numRequests)
+	for i := 0; i < numRequests; i++ {
+


Suggested change

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR aims to make opcache_reset() safe under concurrency by funneling resets through a coordinated thread restart mechanism and ensuring only one actual opcache reset runs at a time.

Changes:

Centralize worker draining/restarting into shared drainThreads / restartThreadsAndOpcacheReset.
Introduce new state machine phases to coordinate opcache reset across threads.
Add a concurrency-focused integration test and PHP fixtures for triggering opcache_reset().

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
worker.go	Routes draining/restarting through new shared helpers.
threadworker.go	Moves opcache reset flow into a coordinated state sequence; removes direct reset call.
threadregular.go	Adds coordinated opcache reset flow for regular threads (and imports C).
internal/state/state.go	Adds new thread states for opcache reset coordination.
frankenphp.go	Implements global coordination: draining, restart, once-only reset, and C-export hook.
frankenphp.c	Overrides `opcache_reset()` to schedule a safe reset; changes reset implementation to call original handler.
testdata/require.php	Adds fixture for php `require` during opcache reset test.
testdata/opcache_reset.php	Adds fixture endpoint that calls `opcache_reset()`.
caddy/caddy_test.go	Adds concurrency test attempting to reproduce opcache reset segfault race.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+		opcacheResetWg.Go(func() {
+			thread.state.WaitFor(state.OpcacheResettingDone)
+		})



+/* Forward declaration */
+PHP_FUNCTION(frankenphp_opcache_reset);
+zif_handler orig_opcache_reset;


+static void frankenphp_override_opcache_reset(void) {
+  zend_function *func = zend_hash_str_find_ptr(
+      CG(function_table), "opcache_reset", sizeof("opcache_reset") - 1);
+  if (func != NULL && func->type == ZEND_INTERNAL_FUNCTION &&
+      ((zend_internal_function *)func)->handler !=
+          ZEND_FN(frankenphp_opcache_reset)) {
+    orig_opcache_reset = ((zend_internal_function *)func)->handler;
+    ((zend_internal_function *)func)->handler =
+        ZEND_FN(frankenphp_opcache_reset);
+  }
+}


+
+	threadsToRestart := drainThreads(withRegularThreads)
+
+	opcacheResetOnce = sync.Once{}


+	if threadsAreRestarting.CompareAndSwap(false, true) {
+		go restartThreadsAndOpcacheReset(true)
+	}


+		// introduce some random delay
+		if rand.IntN(10) > 8 {
+			time.Sleep(time.Millisecond * 10)
+		}
+
+		go func() {
+			defer wg.Done()
+			// randomly call opcache_reset
+			if rand.IntN(10) > 7 {


henderkes · 2026-05-13T11:22:12Z

So let's merge?

If you don't mind tackling the copilot flagged issues, please. I'm working on a different project atm.

AlliBalliBaba added 9 commits December 14, 2025 23:14

Tests.

e94e3bc

Merge branch 'main' into fix/opcache-safe-reset

48bf41f

Wait 1s for deadlocks.

973d722

Merge branch 'main' into fix/opcache-safe-reset

2a9a9a8

# Conflicts: # caddy/caddy_test.go

Adjusts waitgroup logic.

d1e28d5

Starts separate opcache_reset request flow once all threads are stopped.

8e87d00

Test with grace period (again)

acf2a1c

Force all threads to call opcache_reset().

d8c185c

test

3e7cdd0

henderkes force-pushed the fix/opcache-safe-reset branch 2 times, most recently from e54cd96 to 5c8e340 Compare March 13, 2026 13:52

Merge remote-tracking branch 'origin/main' into fix/opcache-safe-reset

1d75824

henderkes force-pushed the fix/opcache-safe-reset branch from 5c8e340 to 1d75824 Compare March 13, 2026 13:56

henderkes added 2 commits March 13, 2026 21:00

fix clang format

df82e81

call into cgo for reset directly, no fake dummy

0d87765

henderkes force-pushed the fix/opcache-safe-reset branch from e7bd25a to 0d87765 Compare March 13, 2026 15:44

clang fmt

0564eaf

override opcache reset handler for every php thread in php 8.2

49fc878

maybe after request startup?

22c6ba6

make it run on 8.2 again

45d49c1

henderkes marked this pull request as ready for review March 15, 2026 02:47

AlliBalliBaba commented Mar 15, 2026

View reviewed changes

henderkes reviewed Mar 16, 2026

View reviewed changes

Comment thread worker.go Outdated

Update worker.go

d189770

Signed-off-by: Marc <m@pyc.ac>

Merge remote-tracking branch 'origin/main' into fix/opcache-safe-reset

0b2521d

henderkes approved these changes Mar 24, 2026

View reviewed changes

fix tests timing out

eebec0b

cap sleep at 100ms

30407d7

henderkes requested a review from dunglas March 26, 2026 13:47

henderkes added 2 commits March 26, 2026 21:55

Merge remote-tracking branch 'origin/main' into fix/opcache-safe-reset

fc77310

Merge branch 'main' into fix/opcache-safe-reset

d5ed5da

henderkes mentioned this pull request Apr 2, 2026

How to reload FrankenPHP without restart to reresolve root directory #2266

Open

henderkes mentioned this pull request Apr 17, 2026

fix: drain PHP threads during opcache restart to prevent heap corruption #2349

Closed

superdav42 mentioned this pull request Apr 17, 2026

fix: add rwlock for implicit opcache restarts (OOM/hash overflow) #2352

Open

AlliBalliBaba mentioned this pull request Apr 22, 2026

fix: reboot threads on opcache reset and watcher #2364

Open

dunglas reviewed May 13, 2026

View reviewed changes

dunglas requested a review from Copilot May 13, 2026 10:41

Copilot AI reviewed May 13, 2026

View reviewed changes


		threadsToRestart := drainThreads(withRegularThreads)

		opcacheResetOnce = sync.Once{}

Conversation

AlliBalliBaba commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexandre-daubois commented Mar 12, 2026

Uh oh!

AlliBalliBaba commented Mar 12, 2026

Uh oh!

withinboredom commented Mar 12, 2026

Uh oh!

AlliBalliBaba commented Mar 12, 2026

Uh oh!

henderkes commented Mar 13, 2026

Uh oh!

AlliBalliBaba commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 13, 2026

Uh oh!

AlliBalliBaba commented Mar 13, 2026

Uh oh!

henderkes commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 13, 2026

Uh oh!

henderkes commented Mar 14, 2026

Uh oh!

henderkes commented Mar 15, 2026

Uh oh!

AlliBalliBaba Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

AlliBalliBaba Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlliBalliBaba commented Mar 16, 2026

Uh oh!

henderkes commented Mar 17, 2026

Uh oh!

henderkes commented Mar 24, 2026

Uh oh!

henderkes left a comment

Choose a reason for hiding this comment

Uh oh!

alexandre-daubois commented Mar 24, 2026

Uh oh!

AlliBalliBaba commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henderkes commented Mar 24, 2026

Uh oh!

AlliBalliBaba commented Mar 25, 2026

Uh oh!

henderkes commented Mar 25, 2026

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

dunglas Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

henderkes commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AlliBalliBaba commented Dec 14, 2025 •

edited

Loading

AlliBalliBaba commented Mar 13, 2026 •

edited

Loading

henderkes commented Mar 13, 2026 •

edited

Loading

AlliBalliBaba commented Mar 24, 2026 •

edited

Loading