Skip to content

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Oct 1, 2025

Motivation for the change, related issues

Fixes a Playground CLI crash that happened when rotating a PHP instance while using a local mount for /wordpress.

Implementation details

Imagine you're mounting a local directory, e.g. /home/adam/my-site at /wordpress in Playground's VFS. By default, /wordpress does not exist so createNodeFsMountHandler() will create it. When it's time to unmount, createNodeFsMountHandler() remembers it created /wordpress just for mounting and will attempt to clean it up. However, since /wordpress is also a CWD, the filesystem will refuse to delete it by throwing error 10 EBUSY.

This PR changes the unmount handler default behavior. It no longer cleans up the mounted directories, unless the cleanupNodesOnUmount is explicitly set to true. This means that mounting something at /wordpress/wp-content/plugins/my-plugin will leave an empty directory in there after the unmount. This should not affect git status as git does not version empty directories.

If we really wanted to keep cleaning up the mount-related directories, we'd need to follow the following runtime rotation procedure:

  1. Change the CWD to /
  2. Unmount from old FS
  3. Create new runtime and new FS
  4. Mount to new FS
  5. Try to restore the previous CWD
  6. Only throw on failure at this point, not at the temporary unmount

So, effectively, move the failure if the path structure is off after the rotation, not during the rotation.

Testing Instructions (or ideally a Blueprint)

CI unit tests.

For manual testing, do this:

$ cli.ts server --mount-before-install=./test-site/:/wordpress

Then:

  1. Let it do the setup.
  2. Modify wp-content/themes/twentytwentyfour/functions.php to call non_existent_function(). This will nudge the PHP class to rotate the runtime and call unmount() for all the mounts,
  3. Run the following command:
$ cli.ts server --mount-before-install=./test-site/:/wordpress  --skip-wordpress-setup

Finally, visit the local site. You should see an error screen. Refresh it three times, confirm you still see the same WordPress error screen.

method: req.method as any,
body: await bufferRequestBody(req),
});
const isodate = new Date().toISOString();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, let's move it to another pr

return PHPResponse.forHttpCode(404);
}

if (!fsPath.endsWith('.php')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just code reformatting, I'll move this to another pr

@adamziel
Copy link
Collaborator Author

adamziel commented Oct 1, 2025

Let's ditch this approach and move forward with #2714 so we don't lose the "cleanup after ourselves" semantics of nodefs mounts.

@adamziel adamziel closed this Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant