Improve OPFS sync copy error messages#3824
Open
akirk wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improves diagnosability of OPFS→MEMFS initial sync failures by surfacing the first real underlying copy error (with path context) instead of the generic Promise.any() message, and adds a regression test for failed OPFS reads.
Changes:
- Wrap OPFS-to-MEMFS copy errors with the failing MEMFS path and preserve the original error via
cause. - Switch wait behavior from
Promise.any()toPromise.race()to surface the first rejection. - Add a Vitest regression test covering a failed OPFS file read during initial sync.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/php-wasm/web/src/lib/directory-handle-mount.ts | Improves error wrapping and changes promise waiting strategy to surface real copy failures. |
| packages/php-wasm/web/src/lib/directory-handle-mount.spec.ts | Adds an in-memory OPFS test harness and a regression test for initial sync error reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
193
to
195
| while (stack.length === 0 && ops.length > 0) { | ||
| await Promise.any(ops); | ||
| await Promise.race(ops); | ||
| } |
Comment on lines
+177
to
+181
| throw new Error( | ||
| `Failed to copy OPFS entry "${memfsEntryPath}" ` + | ||
| `to MEMFS: ${getErrorMessage(error)}`, | ||
| { cause: error } | ||
| ); |
2286407 to
4d4867a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation for the change, related issues
Persistent Playground sites can fail while copying an OPFS/local-fs mount into
MEMFS during boot. The sync loop previously waited with
Promise.any(), sowhen all pending copy operations rejected, Firefox surfaced the generic message:
That message hides the entry that failed and the underlying read/write error,
making submitted crash reports hard to diagnose.
Implementation details
error message.
cause.Promise.race()while waiting for pending copy operations so the firstreal rejection is surfaced instead of a generic
Promise.any()aggregate.Example new message:
Testing Instructions
Both pass locally.
Note: the test run still prints an existing unrelated Vitest warning from
tcp-over-fetch-websocket.spec.tsabout an unawaited assertion.