Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes stale saved-site OPFS ZIP exports after pooled PHP requests by ensuring all proxied writes are flushed and by adding a Web Locks–based durability boundary so exports wait for journal replay to settle.
Changes:
- Adds a pooled-PHP OPFS regression test and related assertions for durability lock propagation.
- Flushes all active OPFS mount journals when a secondary (pooled) PHP request ends.
- Introduces an origin-wide durability lock name, holding it exclusively during journal replay and shared during ZIP export traversal.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/playground/website/src/lib/state/redux/slice-clients.ts | Updates stored OPFS mount descriptor typing to align with remote MountDescriptor and new durability option. |
| packages/playground/website/src/lib/state/redux/persist-temporary-site.ts | Passes a per-site durability lock name into OPFS mount options. |
| packages/playground/website/src/lib/state/redux/persist-temporary-site.spec.ts | Verifies durabilityLockName is forwarded when mounting OPFS. |
| packages/playground/website/src/lib/state/redux/boot-site-client.ts | Passes durability lock name into OPFS mounts when booting a saved OPFS site. |
| packages/playground/website/src/lib/state/redux/boot-site-client.spec.ts | Updates mocks/assertions for durability lock; removes legacy OPFS metadata-path test. |
| packages/playground/website/src/lib/state/opfs/opfs-site-storage.ts | Wraps ZIP exports in a shared Web Lock to await earlier exclusive journal replays. |
| packages/playground/website/src/lib/state/opfs/opfs-site-storage.spec.ts | Adds unit test ensuring export traversal waits for the shared durability lock. |
| packages/playground/website/src/lib/state/opfs/opfs-site-path.ts | Introduces getOpfsSiteDurabilityLockName() to centralize lock naming. |
| packages/playground/website/playwright/e2e/opfs.spec.ts | Adds Chromium-only E2E regressions for pooled-request flush + export durability. |
| packages/playground/remote/src/lib/playground-worker-endpoint.ts | Flushes all OPFS mounts on secondary PHP request.end; plumbs through durabilityLockName. |
| packages/php-wasm/web/src/lib/directory-handle-mount.ts | Takes durabilityLockName and acquires an exclusive Web Lock while draining the OPFS journal. |
| packages/php-wasm/web/src/lib/directory-handle-mount.spec.ts | Adds unit test verifying exclusive lock is held during flush. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ashfame
changed the title
brandonpayton
left a comment
brandonpayton
left a comment
There was a problem hiding this comment.
@ashfame based on the PR description and the fact that we only have a single PHP-WASM Worker in Playground web, I wonder:
Is the deeper issue the fact that our export logic does not respect VFS ownership of OPFS?
This is what we'd do in a regular operating system. We'd ask the operating system for the data instead of directly reading bytes from a raw block device. We'd do that because the operating system is in charge of caching reads and writes and because skipping that layer would expose us to the possibility of incomplete or corrupted data.
We may still need some kind of locking for this, but I think everything could be simpler and more encapsulated if we:
- Change export to work through the PHP-WASM VFS abstraction.
- Make sure additional requests to the PHP-WASM cannot write to OPFS during a flush if flush is async. If the actual flush is not async, we may not need a lock at all, though we'd need to be very careful to prove this and to make it hard to be accidentally async in the future.
ashfame
changed the title
ashfame
changed the title
|
@brandonpayton Thanks — the previous PR description conflated two export APIs. I’ve corrected it and removed the unrelated The regular Download as .zip flow already follows your first suggestion: it calls Playground web uses one Web Worker per site, but that worker’s The actual gap is lifecycle propagation: writes from a secondary instance reach the primary VFS and are captured by its OPFS journal, but The remaining change triggers the active OPFS journal flushes when a secondary request ends. It does not modify export or add export locking. The existing flush implementation is single-flight and drains journal entries added during an in-progress flush in subsequent batches. The regression test forces a request onto the secondary instance, confirms its write is visible in the live VFS, and then verifies that it reaches OPFS. We also mutation-tested it: it fails without the implementation change and passes with it. Does that address your concern? |
brandonpayton
left a comment
brandonpayton
left a comment
There was a problem hiding this comment.
Thank you, @ashfame! This is much simpler than the previous iteration.
That said, I think we need to adjust the approach to more directly address our proxying and journaling relationships instead of adding special, secondary-only behavior in the worker.
I left some notes on the test as well.
| }); | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| const pooledRequest = playground.run({ | ||
| code: `<?php usleep(750000); file_put_contents(${JSON.stringify( |
There was a problem hiding this comment.
What is the purpose of this sleep?
| async ({ markerPath, markerContents }) => { | ||
| const playground = (window as any).playgroundSites.getClient(); | ||
| const primaryRequest = playground.run({ | ||
| code: '<?php usleep(500000);', |
There was a problem hiding this comment.
Let's comment on the purpose of this sleep.
| }, | ||
| { markerPath, markerContents } | ||
| ); | ||
| expect(liveMarkerContents).toBe(markerContents); |
There was a problem hiding this comment.
Let's say what we are proving with this assertion.
| ); | ||
| expect(liveMarkerContents).toBe(markerContents); | ||
|
|
||
| await expect |
There was a problem hiding this comment.
I'm finding myself confused by what we are proving here. Above, we try to make sure a second request is handled by a secondary PHP instance that writes a file to an OPFS mount.
Does that mean our check here is to ensure the expected changes are flushed to OPFS after the request completes?
I feel like this test is probably exercising all the right things, but can we really prove that? Can we prove we are using a secondary instance for the second request? Is it possible there are races within the time frames when the request promise is resolved, when we continue from awaiting the second request, and when we are doing the polling below?
Is there a way we could more explicitly prove that writes to a secondary PHP instance are 100% flushed to OPFS before any other PHP instance runs?
| /** | ||
| * Secondary PHP instances write through the primary PHP's filesystem. | ||
| * Their request-end event stays on the secondary instance, so trigger | ||
| * every active mount journal here after those proxied writes finish. | ||
| */ | ||
| php.addEventListener('request.end', () => { | ||
| for (const [mountpoint, mount] of Object.entries( | ||
| this.opfsMounts | ||
| )) { | ||
| void mount.flush().catch((error) => { | ||
| logger.error( | ||
| `OPFS flush failed after a pooled PHP request at "${mountpoint}"`, | ||
| error | ||
| ); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I think this is working at the wrong level of abstraction. It may solve the problem, but shouldn't the solution be in the code that configures journaling and only listens to request.end for a single PHP instance. I think we probably need to consider that abstraction and whether it could do one of the following:
- be aware of multiple PHP instances
- use a better trigger than request.end on a single PHP instance
The truth is that Playground web creates a single PHP instance to own the FS and expects secondary PHP instances to delegate to it.
Would it makes sense for this truth to be reflected in an event delegated from the secondary PHP instances to the primary PHP instance, like proxyfs.request.end? Maybe such an event from the primary PHP instance could be wired within proxyFileSystem().
If the primary PHP notified when a secondary PHP-instance-with-proxed-FS ended its request, then journalFSEventsToOpfs() could subscribe to that event as well.
We could complain that this exposes system internals, but at least it would be a solution that explicitly coupled to the internal reality of proxying FS. The alternative here is to split the ownership of OPFS flushes between multiple modules, with this module taking some responsibility from journalFSEventsToOpfs().
What do you think?
|
@brandonpayton Great feedback! Closing this PR, superseded by #4260 |
## What Persist filesystem changes made by pooled PHP instances to autosaved Playground sites after the request completes. This supersedes #4240 with the architecture Brandon proposed: PROXYFS propagates replica request completion to the filesystem owner, while the OPFS journal remains solely responsible for deciding when to flush. ## Root cause Playground web uses a pool of PHP instances. Secondary instances access shared paths through PROXYFS backed by the primary PHP instance. Their writes are captured by the primary instance's OPFS journal, but `request.end` is emitted only by the secondary instance. The primary journal therefore has no completion signal for the proxied request, and recent changes can remain queued in memory instead of reaching OPFS. ## Deterministic regression proof The Chromium test exercises the original saved-site symptom instead of asserting any event or flush machinery introduced by this PR: - It writes a unique sentinel outside the proxied mounts, so only the primary PHP runtime can see it. - The first request creates a file barrier and remains blocked; the second request then creates its own barrier and remains blocked. The test observes each barrier instead of sleeping to guess when either request started, then overwrites the same file to release that request. - The test releases and fully awaits the first request before allowing the second request to write the marker, eliminating a later primary `request.end` that could accidentally flush it. - The completed responses prove the requests ran on `primary` and `secondary`, respectively. - A public `readFileAsText()` call proves the marker reached the shared live VFS, then an independent read of the autosaved site's real OPFS directory checks the persisted bytes. At red SHA [`4a5116926`](4a51169), the PR contained only this test. The Chromium storage job reached the final OPFS assertion on all four attempts and consistently received `undefined` instead of the marker contents; all other 60 tests passed. See the [expected failed CI job](https://github.com/WordPress/wordpress-playground/actions/runs/31738099889/job/94574529357). The regression test at the fixed SHA is byte-for-byte identical to the red version. ## Implementation - `proxyFileSystem()` forwards a replica's `request.end` as `proxyfs.request.end` on the filesystem owner. - `journalFSEventsToOpfs()` handles that lifecycle event through its existing single-flight background flush, alongside direct `request.end` and `filesystem.write` triggers. - Journal teardown removes the new listener with its existing listeners. This keeps pool-specific behavior out of Playground's worker and keeps OPFS flush ownership inside the journaling abstraction. It does not change export code or introduce an export lock. ## Validation - Red CI: [Chromium storage job](https://github.com/WordPress/wordpress-playground/actions/runs/31738099889/job/94574529357) — the regression test alone fails at the persisted-bytes assertion on all four attempts. - Local green: universal tests 184 passed / 4 skipped; web tests 60 passed; website rerun 377 passed; affected typechecks and lints passed. - Green CI: [full CI run](https://github.com/WordPress/wordpress-playground/actions/runs/31740060392) and [Chromium storage job](https://github.com/WordPress/wordpress-playground/actions/runs/31740060392/job/94581003107) passed on fixed SHA [`b9013b91e`](b9013b9).
What
Ensure filesystem changes made by pooled PHP instances are flushed to OPFS for every saved Playground site. Without this trigger, recent WordPress changes could remain queued in memory instead of reaching the site's persisted files.
The regular Playground ZIP export already works through the active PHP-WASM VFS via
zipWpContent(). This PR fixes the persistence lifecycle exposed by that flow; it does not change export code or test the standaloneapi.htmlexportSavedSiteAsZip()API.Root cause
Playground web uses one Web Worker per site, but that worker's
PHPRequestHandlermaintains a pool of PHP-WASM instances and allows two concurrent instances by default. Secondary instances proxy shared filesystem paths into the primary instance through PROXYFS.The OPFS journal is mounted on the primary PHP instance and listens for its
request.endevent. A request handled by a secondary instance writes through the primary filesystem, so the journal captures those operations, but itsrequest.endevent remains on the secondary instance. The request could therefore finish while the captured operations remained queued in memory until a later lifecycle event happened to flush them.Fix
Listen for
request.endon secondary PHP instances and trigger a flush for every active OPFS mount after their proxied writes finish. The existing journal flush is single-flight and drains entries added while a flush is running in subsequent batches.Regression test
The new Chromium E2E test creates an autosaved site as its fixture, keeps the primary PHP instance busy so a concurrent request runs on a pooled instance, writes a marker file from that pooled request, confirms the marker exists in the live VFS, and then verifies that the same contents reach the site's underlying OPFS directory. Although the fixture is autosaved, the implementation covers every active OPFS mount and therefore all saved sites.
The test was explicitly run on both sides of the implementation change:
b4e0945a6), it failed at the OPFS assertion: the live marker contained the expected text, but OPFS returnedundefined.a5746e958), the same focused test passed.This fail-before/pass-after result confirms that the test covers the missing persistence trigger rather than merely restating the implementation.
Validation
npm exec -- nx build playground-websitenpm exec -- nx run-many -t typecheck lint -p playground-remote playground-website --parallel=3