Add FileSystemHandle.move method

[a-sully]

Migrated from WICG/file-system-access#317 (see #2)

Addresses WICG/file-system-access#64 and WICG/file-system-access#65

• At least two implementers are interested (and none opposed):
  • Chrome
  • …
• Tests are written and can be reviewed and commented upon at:
  • https://github.com/web-platform-tests/wpt/blob/master/file-system-access/script-tests/FileSystemDirectoryHandle-move.js
• Implementation bugs are filed:
  • Chrome: https://crbug.com/1140805
  • Firefox: …
  • Safari: …

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[annevk]

I'd suggest we don't include the Move.md file here.

[Explosion-Scratch]

Any updates on this? Thanks @a-sully for working on this though. When this gets merged what exactly happens? Does this feature get added to all JavaScript engines supporting the whatwg standard?

[jesup]

Open questions:

• What happens if there are existing SyncAccessHandles to a file being moved?
• What if there are FileHandles to a file being moved?
• What if there are DirectoryHandles to a file being moved?
• What happens if a different process/worker/etc tries to grab a FileHandle/DirectoryHandle/SyncAccess handle on a file or directory being moved while it is still in the process of being moved (promise has not resolved yet). If this does not succeed, what error is thrown?
• Is it possible that a move() could end up in a state other than completely moved or completely not-moved? I.e. if there's a disk or permission error during moving a directory. This is unlikely in OPFS, but may be possible in the more general usage.

[jesup]

Open questions:

* What happens if there are existing SyncAccessHandles to a file being moved?

* What if there are FileHandles to a file being moved?

* What if there are DirectoryHandles to a file being moved?

This comment in move.md from the patch partly answers these questions (I presume what's meant here is "All handles to entries"):

• All entries contained within a directory which is moved no longer point to an
  existing file/directory. We currently have no plans to explicitly update or
  invalidate these handles, which would otherwise be an expensive recursive
  operation.

That doesn't answer about FileHandles or SyncAccessHandles to a file being moved/renamed.

* What happens if a different process/worker/etc tries to grab a FileHandle/DirectoryHandle/SyncAccess handle on a file or directory being moved while it is still in the process of being moved (promise has not resolved yet).   If this does not succeed, what error is thrown?

* Is it possible that a move() could end up in a state other than completely moved or completely not-moved?   I.e. if there's a disk or permission error during moving a directory.   This is unlikely in OPFS, but may be possible in the more general usage.

This is discussed in move.md, but there's no resolution of the discussion.

[jesup]

Right now the justification for the answer about moving directories is based on move.md, which isn't here.
There's also no description in the spec as to what happens to any of these cases; these need to be speced. One can infer that if we move foo to bar, that the directory now holds bar instead of foo. But there may be race conditions between a worker and mainthread, or worker and worker...
It also means that one needs to reverse-engineer the current implementations.

[jesup]

Note that the WPT tests for move() encode several things not in this spec related to what are valid names:

testing that you can't rename a file to "Lorem." (no trailing periods - this isn't specified)
testing that you can't rename a file "#$23423@352^*3243" (no invalid characters - this isn't specified - none of those are separators)

The spec says: "A valid file name is a string that is not an empty string, is not equal to "." or "..", and does not contain '/' or any other character used as path separator on the underlying platform."
and
"Note: This means that '' is not allowed in names on Windows, but might be allowed on other operating systems. Additionally underlying file systems might have further restrictions on what names are or aren’t allowed, so a string merely being a valid file name is not a guarantee that creating a file or directory with that name will succeed." "We should consider having further normative restrictions on file names that will never be allowed using this API, rather than leaving it entirely up to underlying file systems."

[jesup]

The tests also test that move() can overwrite an existing file. This is mentioned nowhere in the spec, and the spec says "Attempt to move entry to destination in the underlying file system." which does not to me indicate it should overwrite. (It also checks that having a WritableFileStream open on the destination blocks move())

[jesup]

Also file.move(dir, "") should fail, per the spec; the wpt tests assume it's the same as file.move(dir)

[a-sully]

Thanks for bringing up these questions! Apologies for the confusion, Move.md is a bit outdated since it was written before the locking mechanisms for SyncAccessHandles were created. I'm planning to update this PR once #21 lands to incorporate those locking mechanisms. For now, to respond to your questions...

Open questions:

• What happens if there are existing SyncAccessHandles to a file being moved?

move() takes an exclusive lock on the handle. Therefore a file cannot be moved while there is an open SyncAccessHandle (a reminder to me that we really need to clear up #18)

• What if there are FileHandles to a file being moved?
• What if there are DirectoryHandles to a file being moved?

#30 is intended to address these. Since a FileSystemHandle maps to a path, these handles will dangle once the underlying file/dir is moved.

• What happens if a different process/worker/etc tries to grab a FileHandle/DirectoryHandle/SyncAccess handle on a file or directory being moved while it is still in the process of being moved (promise has not resolved yet). If this does not succeed, what error is thrown?

If a SyncAccessHandle is being created, presumably whichever operation is able to grab their respective lock will win and the other will reject.

As for getFileHandle() and getDirectoryHandle(), if the path has been moved (and create: true has not been specified) then I don't think this deserves any special treatment? Do you see a problem with rejecting with NotFoundError as is currently specified when the requested path does not exist?

• Is it possible that a move() could end up in a state other than completely moved or completely not-moved? I.e. if there's a disk or permission error during moving a directory. This is unlikely in OPFS, but may be possible in the more general usage.

Yes. A couple examples:

• Power loss during a cross-file system move
• Moving a directory from OPFS -> non-OPFS will trigger Safe Browsing checks on all containing files. If any of these checks fail, the unsafe files will not be moved (while the safe files will be moved)

In these cases, the promise will be rejected with an AbortError.

The tests also test that move() can overwrite an existing file. This is mentioned nowhere in the spec, and the spec says "Attempt to move entry to destination in the underlying file system." which does not to me indicate it should overwrite. (It also checks that having a WritableFileStream open on the destination blocks move())

You are correct. And I'm wondering whether this is the right behavior in the first place... I think there's a strong argument to be made that we should reject a move to a path that already exists and allow the site to delete the destination handle and try again if they truly want it overwritten.

[a-sully]

Also file.move(dir, "") should fail, per the spec; the wpt tests assume it's the same as file.move(dir)

Good catch. We should probably update the WPTs and not the spec? Ideally the "is this a safe file" code can be shared between move(), getFileHandle(), and getDirectoryHandle()

[jesup]

Also file.move(dir, "") should fail, per the spec; the wpt tests assume it's the same as file.move(dir)

Good catch. We should probably update the WPTs and not the spec? Ideally the "is this a safe file" code can be shared between move(), getFileHandle(), and getDirectoryHandle()

yes, the WPTs should be updated

[jesup]

Attempts to move a directory to within itself should fail.
Attempts to move the root directory should fail... at least in OPFS, but that would be handled by the above restriction anyways. No idea about non-OPFS.
Note that for non-OPFS if you move a directory out of OPFS you have to check all files in the directory for safe-browsing

[jesup]

The wpt tests check that moving a file to the same name succeeds (I.e. succeeds in doing nothing). (They don't check if moving a directory to the same name succeeds, note.) This behavior does not appear to be anywhere in the spec here; it basically says "attempts to move it in the underlying filesystem" which could fail or succeed.
The operation here should be well-specified, probably by saying that it should always succeed (for both directories and files)

[jesup]

I wonder if the fact that move-that-doesn't-move succeeds in chrome is related to the fact that it currently silently allows a move() to overwrite a file. At this moment, we fail that WPT test because we check if the destination file exists and if so, fail.

[a-sully]

Just another area that shows it's better to get agreement on APIs first, before any field lockin occurs... like with SyncAccessHandle synchronous-ness

Agreed!

On that note, we're planning to ship move() of files outside of the OPFS on Chromium browsers soon. Please feel free to leave feedback on the explainer (a-sully#2) or TAG review (w3ctag/design-reviews#805)

Q: Should we scope down this PR to only consider file moves? Ideally this PR would have merged months (when we decided to support move() for files within the OPFS) but I worry it will be tricky to land this PR while it contains directory moves while #59 is unresolved. Thoughts?

[a-sully]

Hi all, I've updated this PR to make use of the various infrastructural changes that have merged recently (primarily #95 and #96). Please take a look at the updated PR. Thanks!

@jesup I've tried to answer all your questions. Apologies for not being able to answer them months ago! (We really needed the above infrastructure changes to appropriately answer many of them.) Please let me know if there's anything I've missed.

Open questions:

• What happens if there are existing SyncAccessHandles to a file being moved?

move() takes a lock on both the source and destination file. If there is an open SyncAccessHandle, neither the file itself nor any of its parents (as per #139 (review)) are able to be moved

• What if there are FileHandles to a file being moved?

Only the handle which move() is called on is updated. I've added a note about this in the PR

• What if there are DirectoryHandles to a file being moved?

Ditto

• What happens if a different process/worker/etc tries to grab a FileHandle/DirectoryHandle/SyncAccess handle on a file or directory being moved while it is still in the process of being moved (promise has not resolved yet). If this does not succeed, what error is thrown?

We now have a file system queue!

• Is it possible that a move() could end up in a state other than completely moved or completely not-moved? I.e. if there's a disk or permission error during moving a directory. This is unlikely in OPFS, but may be possible in the more general usage.

Yes. I've added a note about this in the PR, though since no browser has yet implemented directory moves outside of the BucketFS, that part is admittedly a bit vague for now

• I think at least some non-normative mention of WebLocks to avoid overwriting would be helpful here
• Attempts to move a directory to within itself should fail.
• Attempts to move the root directory should fail... at least in OPFS

Added!

Note that for non-OPFS if you move a directory out of OPFS you have to check all files in the directory for safe-browsing

As per this, I've specified that moving across the OPFS boundary is not allowed (for now). I welcome thoughts on #114, as well as thoughts on supporting cross-BucketFS moves

• The wpt tests check that moving a file to the same name succeeds (I.e. succeeds in doing nothing).

I agree that we shouldn't need to actually call into e.g. mv, but I don't think we should do nothing. We should definitely run permission checks, for example, and it seems useful to still e.g. get a NoModificationAllowedError if the file is locked. For now I've specified it such that we'll run most all the steps right up until mv, but exit early before actually attempting to move on disk.

[a-sully]

A few things to note:

• The steps for requesting access to the destination are still subject to change depending on what we decide in Add a serializable "permission root locator" to a FileSystemHandle #131
• Locking a directory entry is not yet supported, but we're working on it: Support locking of directory entries #139
• No browsers yet support directory moves outside of the BucketFS, so there are likely more restrictions we'll want to add - that may require some acrobatics with the https://github.com/wicg/file-system-access spec. For example, we should not allow well-known directories to be moved

[JasonGrass]

The behavior of renaming files is inconsistent with the native API.

When using the FileSystemHandle.move method to rename files, it modifies the "Modified time" of the file. However, this issue does not occur when manually operating or using the native API, as renaming usually does not modify a file's "Modified time".
Is this behavior expected?

Should I report this issue here or somewhere else?

PS My test environment: Win10, Chrome120.0.6099.129

[miketaylr]

Is this behavior expected?

AFAICT, this PR doesn't say anything about setting a file's lastModified (or to not do so). @a-sully?