diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index caa8901ae..84023ceb0 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -193,40 +193,35 @@ impl PackageStorage { } }; - // Temporary place where the package will be extracted before being - // moved to the target directory. To avoid name clashing we use a PRNG - // to get a unique directory name. - let tempdir = Tempdir::create(cache_dir.join(format!( - "{}/{}/.tmp-{}-{}", - spec.namespace, - spec.name, + // The directory in which the package's version lives. + let base_dir = cache_dir.join(format!("{}/{}", spec.namespace, spec.name)); + + // The place at which the specific package version will live in the end. + let package_dir = base_dir.join(format!("{}", spec.version)); + + // To prevent multiple Typst instances from interferring, we download + // into a temporary directory first and then move this directory to + // its final destination. + // + // In the `rename` function's documentation it is stated: + // > This will not work if the new name is on a different mount point. + // + // By locating the temporary directory directly next to where the + // package directory will live, we are (trying our best) making sure + // that `tempdir` and `package_dir` are on the same mount point. + let tempdir = Tempdir::create(base_dir.join(format!( + ".tmp-{}-{}", spec.version, fastrand::u32(..), ))) .map_err(|err| error("failed to create temporary package directory", err))?; + // Decompress the archive into the temporary directory. let decompressed = flate2::read::GzDecoder::new(data.as_slice()); tar::Archive::new(decompressed) .unpack(&tempdir) .map_err(|err| PackageError::MalformedArchive(Some(eco_format!("{err}"))))?; - // The place into which we'll move the extracted package. - let package_dir = - cache_dir.join(format!("{}/{}/{}", spec.namespace, spec.name, spec.version)); - - // As to not overcomplicate the code to combat an already rare case - // where multiple instances try to download the same package version - // concurrently, we are abusing the behavior of the `rename` FS - // operation without the help of file locking. - // - // From the function's documentation it is stated that: - // - // > This will not work if the new name is on a different mount point. - // - // By extracting package into the same base directory where all packages - // live we are (trying our best) making sure that `extracted_package_dir` - // and `package_dir` are on the same mount point. - // // When trying to move (i.e., `rename`) the directory from one place to // another and the target/destination directory is empty, then the // operation will succeed (if it's atomic, or hardware doesn't fail, or @@ -234,11 +229,11 @@ impl PackageStorage { // empty, i.e., another instance already successfully moved the package, // then we can safely ignore the `DirectoryNotEmpty` error. // - // This means that we do not check the integrity of the existing moved - // package in a very rare case where it is broken, as it does not - // (currently) justify the additional code complexity. If such situation - // occur and it will be reported, then we probably would consider a - // more comprehensive solution (i.e., file locking or checksums). + // This means that we do not check the integrity of an existing moved + // package, just like we don't check the integrity if the package + // directory already existed in the first place. If situations with + // broken packages still occur even with the rename safeguard, we might + // consider more complex solutions like file locking or checksums. match fs::rename(&tempdir, &package_dir) { Ok(()) => Ok(()), Err(err) if err.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(()),