From 2aaf8a8a9c053fbb399d845f12812ba80652e63d Mon Sep 17 00:00:00 2001
From: Andrew Voynov <andrewvoynov.b@gmail.com>
Date: Sat, 1 Mar 2025 00:45:36 +0300
Subject: [PATCH] Fix parallel package installation

Also removes the possibility of a broken package cache state.
---
 crates/typst-kit/src/package.rs | 66 ++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs
index 172d8740a..6383f2d2d 100644
--- a/crates/typst-kit/src/package.rs
+++ b/crates/typst-kit/src/package.rs
@@ -20,6 +20,9 @@ pub const DEFAULT_NAMESPACE: &str = "preview";
 /// The default packages sub directory within the package and package cache paths.
 pub const DEFAULT_PACKAGES_SUBDIR: &str = "typst/packages";
 
+/// The default download directory where all packages are downloaded to.
+pub const DEFAULT_DOWNLOAD_DIR: &str = "typst/downloads";
+
 /// Holds information about where packages should be stored and downloads them
 /// on demand, if possible.
 #[derive(Debug)]
@@ -84,6 +87,7 @@ impl PackageStorage {
         progress: &mut dyn Progress,
     ) -> PackageResult<PathBuf> {
         let subdir = format!("{}/{}/{}", spec.namespace, spec.name, spec.version);
+        let download_dir = dirs::cache_dir().map(|dir| dir.join(DEFAULT_DOWNLOAD_DIR));
 
         if let Some(packages_dir) = &self.package_path {
             let dir = packages_dir.join(&subdir);
@@ -92,7 +96,9 @@ impl PackageStorage {
             }
         }
 
-        if let Some(cache_dir) = &self.package_cache_path {
+        if let (Some(cache_dir), Some(download_dir)) =
+            (&self.package_cache_path, download_dir)
+        {
             let dir = cache_dir.join(&subdir);
             if dir.exists() {
                 return Ok(dir);
@@ -100,7 +106,7 @@ impl PackageStorage {
 
             // Download from network if it doesn't exist yet.
             if spec.namespace == DEFAULT_NAMESPACE {
-                self.download_package(spec, &dir, progress)?;
+                self.download_package(&download_dir, spec, &dir, progress)?;
                 if dir.exists() {
                     return Ok(dir);
                 }
@@ -166,6 +172,7 @@ impl PackageStorage {
     /// Panics if the package spec namespace isn't `DEFAULT_NAMESPACE`.
     pub fn download_package(
         &self,
+        download_dir: &Path,
         spec: &PackageSpec,
         package_dir: &Path,
         progress: &mut dyn Progress,
@@ -191,11 +198,58 @@ impl PackageStorage {
             }
         };
 
+        // Temporary place where the package will be downloaded before being moved
+        // to the target directory;
+        let package_download_dir = download_dir.join(format!(
+            "{}-{}-{}",
+            spec.name,
+            spec.version,
+            std::process::id() // Make directory name unique.
+        ));
+
+        let create_dir = |dir, dir_name| {
+            fs::create_dir_all(dir).map_err(|err| {
+                PackageError::Other(Some(eco_format!(
+                    "failed to create a {dir_name} directory ({err})"
+                )))
+            })
+        };
+
+        create_dir(package_download_dir.as_path(), "download")?;
+        create_dir(package_dir, "package")?;
+
         let decompressed = flate2::read::GzDecoder::new(data.as_slice());
-        tar::Archive::new(decompressed).unpack(package_dir).map_err(|err| {
-            fs::remove_dir_all(package_dir).ok();
-            PackageError::MalformedArchive(Some(eco_format!("{err}")))
-        })
+        tar::Archive::new(decompressed)
+            .unpack(&package_download_dir)
+            .map_err(|err| {
+                fs::remove_dir_all(package_dir).ok();
+                PackageError::MalformedArchive(Some(eco_format!("{err}")))
+            })?;
+
+        let removed_download_dir = || {
+            fs::remove_dir_all(&package_download_dir).map_err(|err| {
+                PackageError::Other(Some(eco_format!(
+                    "failed to delete temporary package directory: {err}"
+                )))
+            })
+        };
+
+        // - Assuming both paths are from the same mount point.
+        // - DirectoryNotEmpty error is assumed to happen only when another instance
+        //   already moved the package to the destination directory, therefore it is
+        //   safe to ignore it.
+        match fs::rename(&package_download_dir, package_dir) {
+            Ok(()) => Ok(()),
+            Err(err) if err.kind() == std::io::ErrorKind::DirectoryNotEmpty => {
+                removed_download_dir()
+            }
+            Err(err) => {
+                removed_download_dir()?;
+                Err(PackageError::Other(Some(eco_format!(
+                    "failed to move the downloaded package directory: {err}"
+                ))))
+            }
+        }
     }
 }