Summary
When REMOTE_PATH is a directory and LOCAL_DESTINATION does not already exist, modal volume get treats the destination as a single file rather than creating it as a directory.
For a directory containing multiple files, the downloader can assign every remote entry the same local output path. Because downloads run concurrently, several workers may then create or overwrite that path at the same time. The command can therefore leave one regular file containing nondeterministic/partial data instead of the requested directory tree.
The remote Volume data is unaffected, but the local download is invalid.
Environment
- Modal client:
1.5.3
- Python:
3.12
- OS: Linux
- Volume: v2
The relevant logic is also still present on main:
https://github.com/modal-labs/modal-client/blob/main/py/modal/cli/_download.py#L47-L67
Minimal reproduction
Using any test Volume as <volume>:
mkdir -p /tmp/modal-volume-get-repro/source
printf 'alpha\n' > /tmp/modal-volume-get-repro/source/a.txt
printf 'beta\n' > /tmp/modal-volume-get-repro/source/b.txt
modal volume put --force <volume> /tmp/modal-volume-get-repro/source /directory-repro
# Ensure the requested local destination does not exist.
rm -rf /tmp/modal-volume-get-repro/download
modal volume get <volume> directory-repro /tmp/modal-volume-get-repro/download
Expected behavior
/tmp/modal-volume-get-repro/download should be created as a directory containing the recursively downloaded contents:
/tmp/modal-volume-get-repro/download/
├── a.txt
└── b.txt
This seems consistent with the CLI documentation, which says that when REMOTE_PATH is a folder, its contents are downloaded recursively:
https://modal.com/docs/cli/latest/volume#modal-volume-get
Actual behavior
The nonexistent destination is initially treated as a file path. Depending on timing and the entries returned, the result may be a regular file, an exception, or an otherwise incomplete/malformed local tree. With a large directory, I observed download become a regular file while many remote files were concurrently written to that same path.
Creating the destination directory before running the command avoids that overwrite race, but also changes the layout. For example:
mkdir -p /tmp/modal-volume-get-repro/download
modal volume get <volume> directory-repro /tmp/modal-volume-get-repro/download
produces an extra remote-basename level:
/tmp/modal-volume-get-repro/download/directory-repro/a.txt
/tmp/modal-volume-get-repro/download/directory-repro/b.txt
Source-level cause
_volume_download() currently selects the output path using the local path's current filesystem type:
if local_destination.is_dir():
output_path = local_destination / rel_path
else:
output_path = local_destinationFor a destination that does not exist, Path.is_dir() is false. Each entry produced by the recursive remote listing can therefore receive the identical local_destination output path. The producer can enqueue those entries before any consumer has created the path, after which concurrent consumers open the same path with "wb".
The branch appears to have been introduced while adding support for downloading a single remote file under a different local filename:
7071501
The tests added in that change cover:
- A single file downloaded to a new filename.
- A directory downloaded into an already-existing temporary directory.
They do not appear to cover a remote directory downloaded to a nonexistent local destination.
Suggested fix
Determine whether the requested remote path represents a file or directory before choosing destination semantics, rather than inferring it from whether the local destination already exists.
For a remote directory, create the local destination directory and map all entries relative to the requested remote directory. For a single remote file, retain the current ability to use a nonexistent destination as the new filename.
A regression test with a remote directory containing at least two files and a nonexistent local destination should expose the concurrent same-path assignment. It may also be useful to specify and test whether an existing destination directory should contain the remote directory itself or only its contents.
Version
1.5.3
App ID
No response
Summary
When
REMOTE_PATHis a directory andLOCAL_DESTINATIONdoes not already exist,modal volume gettreats the destination as a single file rather than creating it as a directory.For a directory containing multiple files, the downloader can assign every remote entry the same local output path. Because downloads run concurrently, several workers may then create or overwrite that path at the same time. The command can therefore leave one regular file containing nondeterministic/partial data instead of the requested directory tree.
The remote Volume data is unaffected, but the local download is invalid.
Environment
1.5.33.12The relevant logic is also still present on
main:https://github.com/modal-labs/modal-client/blob/main/py/modal/cli/_download.py#L47-L67
Minimal reproduction
Using any test Volume as
<volume>:Expected behavior
/tmp/modal-volume-get-repro/downloadshould be created as a directory containing the recursively downloaded contents:This seems consistent with the CLI documentation, which says that when
REMOTE_PATHis a folder, its contents are downloaded recursively:https://modal.com/docs/cli/latest/volume#modal-volume-get
Actual behavior
The nonexistent destination is initially treated as a file path. Depending on timing and the entries returned, the result may be a regular file, an exception, or an otherwise incomplete/malformed local tree. With a large directory, I observed
downloadbecome a regular file while many remote files were concurrently written to that same path.Creating the destination directory before running the command avoids that overwrite race, but also changes the layout. For example:
produces an extra remote-basename level:
Source-level cause
_volume_download()currently selects the output path using the local path's current filesystem type:For a destination that does not exist,
Path.is_dir()is false. Each entry produced by the recursive remote listing can therefore receive the identicallocal_destinationoutput path. The producer can enqueue those entries before any consumer has created the path, after which concurrent consumers open the same path with"wb".The branch appears to have been introduced while adding support for downloading a single remote file under a different local filename:
7071501
The tests added in that change cover:
They do not appear to cover a remote directory downloaded to a nonexistent local destination.
Suggested fix
Determine whether the requested remote path represents a file or directory before choosing destination semantics, rather than inferring it from whether the local destination already exists.
For a remote directory, create the local destination directory and map all entries relative to the requested remote directory. For a single remote file, retain the current ability to use a nonexistent destination as the new filename.
A regression test with a remote directory containing at least two files and a nonexistent local destination should expose the concurrent same-path assignment. It may also be useful to specify and test whether an existing destination directory should contain the remote directory itself or only its contents.
Version
1.5.3
App ID
No response