From 44c98540e19d0b3e5438c80df4709f55de4a0f11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Wed, 1 Oct 2025 14:37:48 +0200 Subject: [PATCH] [Node] Stop deleting NODEFS mount directories on unmount --- .../php-wasm/node/src/lib/node-fs-mount.ts | 20 +- packages/php-wasm/node/src/test/mount.spec.ts | 197 ++++++++++++++++-- .../universal/src/lib/php-request-handler.ts | 62 +++--- packages/php-wasm/universal/src/lib/php.ts | 2 +- packages/playground/cli/src/run-cli.ts | 7 +- packages/playground/cli/src/start-server.ts | 5 + 6 files changed, 244 insertions(+), 49 deletions(-) diff --git a/packages/php-wasm/node/src/lib/node-fs-mount.ts b/packages/php-wasm/node/src/lib/node-fs-mount.ts index b562edd3ec..00bdfbcfcf 100644 --- a/packages/php-wasm/node/src/lib/node-fs-mount.ts +++ b/packages/php-wasm/node/src/lib/node-fs-mount.ts @@ -3,10 +3,20 @@ import { FSHelpers, type MountHandler, } from '@php-wasm/universal'; +import { isParentOf } from '@php-wasm/util'; import { lstatSync } from 'fs'; import { dirname } from 'path'; -export function createNodeFsMountHandler(localPath: string): MountHandler { +export type NodeFsMountHandlerOptions = { + cleanupNodesOnUnmount?: boolean; +}; + +export function createNodeFsMountHandler( + localPath: string, + options: NodeFsMountHandlerOptions = { + cleanupNodesOnUnmount: false, + } +): MountHandler { return function (php, FS, vfsMountPoint) { /** * When Emscripten attempt to mount a local path into VFS, it looks up the path @@ -51,8 +61,14 @@ export function createNodeFsMountHandler(localPath: string): MountHandler { FS.mount(FS.filesystems['NODEFS'], { root: localPath }, vfsMountPoint); return () => { FS!.unmount(vfsMountPoint); - if (removeVfsNode) { + if (removeVfsNode && options.cleanupNodesOnUnmount) { if (FS.isDir(lookup.node.mode)) { + if (isParentOf(vfsMountPoint, FS.cwd())) { + throw new Error( + `Cannot remove the VFS directory "${vfsMountPoint}" on umount cleanup – it is a parent of the CWD "${FS.cwd()}". Change CWD before ` + + `unmounting or explicitly disable post-unmount node cleanup with createNodeFsMountHandler(path, {cleanupNodesOnUnmount: false}).` + ); + } FS.rmdir(vfsMountPoint); } else { FS.unlink(vfsMountPoint); diff --git a/packages/php-wasm/node/src/test/mount.spec.ts b/packages/php-wasm/node/src/test/mount.spec.ts index 51a6a5579c..67da43c9f3 100644 --- a/packages/php-wasm/node/src/test/mount.spec.ts +++ b/packages/php-wasm/node/src/test/mount.spec.ts @@ -154,15 +154,17 @@ describe('Mounting', () => { expect(php.isDir(fileMountPoint)).toBe(false); }); - it('Should unmount mounted file and remove created node from VFS', async () => { + it('Should unmount mounted file and remove created node from VFS (with cleanupNodesOnUnmount=true)', async () => { const unmount = await php.mount( fileMountPoint, - createNodeFsMountHandler(filePath) + createNodeFsMountHandler(filePath, { + cleanupNodesOnUnmount: true, + }) ); expect(php.isFile(fileMountPoint)).toBe(true); - unmount(); + await unmount(); expect(php.isFile(fileMountPoint)).toBe(false); }); @@ -172,7 +174,7 @@ describe('Mounting', () => { createNodeFsMountHandler(filePath) ); - unmount(); + await unmount(); await php.mount( fileMountPoint, createNodeFsMountHandler(filePath) @@ -196,7 +198,7 @@ describe('Mounting', () => { expect(php.isFile(mountPoint)).toBe(true); - unmount(); + await unmount(); expect(php.isDir(dirname(mountPoint))).toBe(true); }); }); @@ -498,38 +500,44 @@ describe('Mounting', () => { } }); - it('Should unmount mounted directory and remove created node from VFS', async () => { + it('Should unmount mounted directory and remove created node from VFS (with cleanupNodesOnUnmount=true)', async () => { const unmount = await php.mount( directoryMountPoint, - createNodeFsMountHandler(directoryPath) + createNodeFsMountHandler(directoryPath, { + cleanupNodesOnUnmount: true, + }) ); expect(php.isDir(directoryMountPoint)).toBe(true); - unmount(); + await unmount(); expect(php.isDir(directoryMountPoint)).toBe(false); }); - it('Should unmount mounted directory, but not remove the parent directory from VFS if it was created manually', async () => { + it('Should unmount mounted directory, but not remove the parent directory from VFS if it was created manually (with cleanupNodesOnUnmount=true)', async () => { await php.mkdir(directoryMountPoint); const unmount = await php.mount( directoryMountPoint, - createNodeFsMountHandler(directoryPath) + createNodeFsMountHandler(directoryPath, { + cleanupNodesOnUnmount: true, + }) ); expect(php.isDir(directoryMountPoint)).toBe(true); - unmount(); + await unmount(); expect(php.isDir(directoryMountPoint)).toBe(true); }); - it('Should remount mounted directory after unmounting', async () => { + it('Should remount mounted directory after unmounting (with cleanupNodesOnUnmount=true)', async () => { const unmount = await php.mount( directoryMountPoint, - createNodeFsMountHandler(directoryPath) + createNodeFsMountHandler(directoryPath, { + cleanupNodesOnUnmount: true, + }) ); - unmount(); + await unmount(); await php.mount( directoryMountPoint, createNodeFsMountHandler(directoryPath) @@ -628,4 +636,165 @@ describe('Mounting', () => { fs.rmSync(tempBase, { recursive: true, force: true }); } }); + + describe('Should respect the cleanupNodesOnUnmount option', () => { + let tempBase = '', + testFile = '', + testDir = ''; + beforeAll(() => { + tempBase = fs.mkdtempSync( + path.join(os.tmpdir(), 'playground-cleanup-') + ); + testFile = path.join(tempBase, 'test.txt'); + testDir = path.join(tempBase, 'testdir'); + + fs.writeFileSync(testFile, 'test content'); + fs.mkdirSync(testDir); + fs.writeFileSync( + path.join(testDir, 'nested.txt'), + 'nested content' + ); + }); + + afterAll(() => { + fs.rmSync(tempBase, { recursive: true, force: true }); + }); + + describe('cleanupNodesOnUnmount=false', () => { + it('Should not remove the VFS directory if it did not exist before the mount', async () => { + const unmount = await php.mount( + '/mount-target', + createNodeFsMountHandler(testDir, { + cleanupNodesOnUnmount: false, + }) + ); + + expect(php.isDir('/mount-target')).toBe(true); + await unmount(); + expect(php.isDir('/mount-target')).toBe(true); + expect(php.listFiles('/mount-target')).toEqual([]); + }); + + it('Should not remove the VFS file if it did not exist before the mount', async () => { + const unmount = await php.mount( + '/mount-target', + createNodeFsMountHandler(testFile, { + cleanupNodesOnUnmount: false, + }) + ); + + expect(php.isFile('/mount-target')).toBe(true); + expect(php.readFileAsText('/mount-target')).toBe( + 'test content' + ); + await unmount(); + expect(php.isFile('/mount-target')).toBe(true); + expect(php.readFileAsText('/mount-target')).toBe(''); + }); + }); + + describe('cleanupNodesOnUnmount=true', () => { + it('Should remove the VFS directory if it did **not** exist before the mount', async () => { + const unmount = await php.mount( + '/mount-target', + createNodeFsMountHandler(testDir, { + cleanupNodesOnUnmount: true, + }) + ); + + expect(php.isDir('/mount-target')).toBe(true); + await unmount(); + expect(php.fileExists('/mount-target')).toBe(false); + }); + + it('Should remove the VFS file if it did **not** exist before the mount', async () => { + const unmount = await php.mount( + '/mount-target', + createNodeFsMountHandler(testFile, { + cleanupNodesOnUnmount: true, + }) + ); + + expect(php.isFile('/mount-target')).toBe(true); + await unmount(); + expect(php.fileExists('/mount-target')).toBe(false); + }); + + it('Should not remove the VFS directory if it did exist before the mount', async () => { + php.mkdir('/mount-target'); + const unmount = await php.mount( + '/mount-target', + createNodeFsMountHandler(testDir, { + cleanupNodesOnUnmount: true, + }) + ); + + expect(php.isDir('/mount-target')).toBe(true); + await unmount(); + expect(php.isDir('/mount-target')).toBe(true); + }); + + it('Should not remove the VFS file if it did exist before the mount', async () => { + php.writeFile('/mount-target', 'Hello, world!'); + const unmount = await php.mount( + '/mount-target', + createNodeFsMountHandler(testFile, { + cleanupNodesOnUnmount: true, + }) + ); + + expect(php.isFile('/mount-target')).toBe(true); + await unmount(); + expect(php.isFile('/mount-target')).toBe(true); + expect(php.readFileAsText('/mount-target')).toBe( + 'Hello, world!' + ); + }); + + it('Should refuse to remove the VFS directory if it is the current CWD', async () => { + const unmount = await php.mount( + '/mount-target', + createNodeFsMountHandler(testDir, { + cleanupNodesOnUnmount: true, + }) + ); + + expect(php.isDir('/mount-target')).toBe(true); + php.chdir('/mount-target'); + expect(unmount()).rejects.toThrow( + /Cannot remove the VFS directory/ + ); + }); + + it('Should refuse to remove the VFS directory if it is a parent of the current CWD', async () => { + const unmount = await php.mount( + '/mount-target', + createNodeFsMountHandler(testDir, { + cleanupNodesOnUnmount: true, + }) + ); + + expect(php.isDir('/mount-target')).toBe(true); + php.mkdir('/mount-target/subdirectory'); + php.chdir('/mount-target/subdirectory'); + expect(unmount()).rejects.toThrow( + /Cannot remove the VFS directory/ + ); + }); + + it('Should remove the VFS directory if it is a child of the current CWD', async () => { + const unmount = await php.mount( + '/mount-target/subdirectory', + createNodeFsMountHandler(testDir, { + cleanupNodesOnUnmount: true, + }) + ); + + expect(php.isDir('/mount-target/subdirectory')).toBe(true); + php.chdir('/mount-target'); + await unmount(); + expect(php.isDir('/mount-target')).toBe(true); + }); + }); + }); }); diff --git a/packages/php-wasm/universal/src/lib/php-request-handler.ts b/packages/php-wasm/universal/src/lib/php-request-handler.ts index 4a921ce09b..471c9c7c57 100644 --- a/packages/php-wasm/universal/src/lib/php-request-handler.ts +++ b/packages/php-wasm/universal/src/lib/php-request-handler.ts @@ -445,39 +445,39 @@ export class PHPRequestHandler implements AsyncDisposable { // We need to confirm that the current target file exists because // file-not-found fallback actions may redirect to non-existent files. - if (primaryPhp.isFile(fsPath)) { - if (fsPath.endsWith('.php')) { - const effectiveRequest: PHPRequest = { - ...request, - // Pass along URL with the #fragment filtered out - url: requestedUrl.toString(), - }; - const response = await this.#spawnPHPAndDispatchRequest( - effectiveRequest, - fsPath - ); - - /** - * If the response is but the exit code is non-zero, let's rewrite the - * HTTP status code as 500. We're acting as a HTTP server here and - * this behavior is in line with what Nginx and Apache do. - */ - if (response.ok() && response.exitCode !== 0) { - return new PHPResponse( - 500, - response.headers, - response.bytes, - response.errors, - response.exitCode - ); - } - return response; - } else { - return this.#serveStaticFile(primaryPhp, fsPath); - } - } else { + if (!primaryPhp.isFile(fsPath)) { return PHPResponse.forHttpCode(404); } + + if (!fsPath.endsWith('.php')) { + return this.#serveStaticFile(primaryPhp, fsPath); + } + + const effectiveRequest: PHPRequest = { + ...request, + // Pass along URL with the #fragment filtered out + url: requestedUrl.toString(), + }; + const response = await this.#spawnPHPAndDispatchRequest( + effectiveRequest, + fsPath + ); + + /** + * If the response is but the exit code is non-zero, let's rewrite the + * HTTP status code as 500. We're acting as a HTTP server here and + * this behavior is in line with what Nginx and Apache do. + */ + if (response.ok() && response.exitCode !== 0) { + return new PHPResponse( + 500, + response.headers, + response.bytes, + response.errors, + response.exitCode + ); + } + return response; } /** diff --git a/packages/php-wasm/universal/src/lib/php.ts b/packages/php-wasm/universal/src/lib/php.ts index 977eab626b..fc9eea4321 100644 --- a/packages/php-wasm/universal/src/lib/php.ts +++ b/packages/php-wasm/universal/src/lib/php.ts @@ -1433,7 +1433,7 @@ export class PHP implements Disposable { }; this.#mounts[virtualFSPath] = mountObject; return () => { - mountObject.unmount(); + return mountObject.unmount(); }; } diff --git a/packages/playground/cli/src/run-cli.ts b/packages/playground/cli/src/run-cli.ts index 5d9f904b61..7b61e9c8c8 100644 --- a/packages/playground/cli/src/run-cli.ts +++ b/packages/playground/cli/src/run-cli.ts @@ -808,7 +808,12 @@ export async function runCLI(args: RunCLIArgs): Promise { } return new PHPResponse(302, headers, new Uint8Array()); } - return await loadBalancer.handleRequest(request); + try { + return await loadBalancer.handleRequest(request); + } catch (e) { + logger.error(e); + return PHPResponse.forHttpCode(500); + } }, }); } diff --git a/packages/playground/cli/src/start-server.ts b/packages/playground/cli/src/start-server.ts index b9493b4960..ba9ef1ad40 100644 --- a/packages/playground/cli/src/start-server.ts +++ b/packages/playground/cli/src/start-server.ts @@ -4,6 +4,7 @@ import express from 'express'; import type { IncomingMessage, Server, ServerResponse } from 'http'; import type { AddressInfo } from 'net'; import type { RunCLIServer } from './run-cli'; +import { logger } from '@php-wasm/logger'; export interface ServerOptions { port: number; @@ -36,6 +37,10 @@ export async function startServer( method: req.method as any, body: await bufferRequestBody(req), }); + const isodate = new Date().toISOString(); + logger.info( + `[${isodate}] ${phpResponse.httpStatusCode} ${req.method} ${req.url}` + ); res.statusCode = phpResponse.httpStatusCode; for (const key in phpResponse.headers) {