Skip to content

Conversation

@brandonpayton
Copy link
Member

Motivation for the change, related issues

Today, if we nest a NODEFS mount inside another NODEFS mount, hot-swapping the PHP runtime fails when we attempt to unmount a nested mount from a parent that has already been unmounted. This PR fixes that issue.

cc @adamziel who mentioned an issue that looked like this one.

Implementation details

This PR changes PHP runtime hot-swapping to unmount mounts in reverse order, and the attempted unmount no longer fails.

Testing Instructions (or ideally a Blueprint)

  • CI

@brandonpayton
Copy link
Member Author

I'd like to add a test for this before we merge. It would be good to catch other issues with hot-swapping runtimes.

I thought we already had such tests but haven't been able to find any.

@brandonpayton
Copy link
Member Author

@brandonpayton
Copy link
Member Author

I'll add a test for nested NODEFS mounts and confirm it detects this issue prior to the fix.

@brandonpayton
Copy link
Member Author

OK, I've added a runtime rotation test with nested mounts. When I locally revert this fix, the test begins to fail due to the bug. This should be good to go. Let's confirm the tests pass.

@adamziel
Copy link
Collaborator

adamziel commented Nov 3, 2025

Thank you! I also wonder if there'a a meta problem in that PHP even crashes in the first place on a failed require call. I wonder what's happening in native php-fpm for example. But, regardless, we this fix in place we can live with either behavior just fine. Thank you!

@adamziel adamziel merged commit d160270 into trunk Nov 3, 2025
29 checks passed
@adamziel adamziel deleted the php-wasm/fix-runtime-hot-swap-unmount-crash branch November 3, 2025 23:23
@brandonpayton
Copy link
Member Author

I also wonder if there'a a meta problem in that PHP even crashes in the first place on a failed require call. I wonder what's happening in native php-fpm for example.

That's a great point! I'll make a note to poke around a bit, even just for better understanding on our part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants