From a60c643a48029dd41029173a466573ef2b6284c8 Mon Sep 17 00:00:00 2001 From: shulaoda <165626830+shulaoda@users.noreply.github.com> Date: Sun, 26 Jul 2026 02:15:39 +0800 Subject: [PATCH] fix(js-runtime): stop deleting a concurrently-installed runtime --- crates/vite_js_runtime/src/download.rs | 83 ++++++++++++++++++++++++-- crates/vite_js_runtime/src/runtime.rs | 10 +--- 2 files changed, 79 insertions(+), 14 deletions(-) diff --git a/crates/vite_js_runtime/src/download.rs b/crates/vite_js_runtime/src/download.rs index 5f28bf6f45..8f1815f0cf 100644 --- a/crates/vite_js_runtime/src/download.rs +++ b/crates/vite_js_runtime/src/download.rs @@ -327,6 +327,7 @@ fn extract_zip(archive_path: &AbsolutePath, target_dir: &AbsolutePath) -> Result pub async fn move_to_cache( source: &AbsolutePath, target: &AbsolutePathBuf, + binary_path: &AbsolutePath, version: &str, ) -> Result<(), Error> { // Create parent directory @@ -355,13 +356,18 @@ pub async fn move_to_cache( .await??; tracing::debug!("Lock acquired: {lock_path:?}"); - // Check again after acquiring the lock, in case another process completed - // the installation while we were downloading - if fs::try_exists(target.as_path()).await.unwrap_or(false) { - tracing::debug!("Target already exists after lock acquisition, skipping move: {target:?}"); - // Lock is released when lock_file is dropped at end of scope + // Under the lock, decide by the binary (not just the directory) and clean up + // here, so it can't delete a peer's install mid-race: + // - binary present -> a peer's complete install, keep it and skip + // - dir without binary -> a stale/partial install, remove then move + if fs::try_exists(binary_path.as_path()).await.unwrap_or(false) { + tracing::debug!("Complete install already present at {target:?}, skipping move"); return Ok(()); } + if fs::try_exists(target.as_path()).await.unwrap_or(false) { + tracing::debug!("Removing incomplete install at {target:?} before move"); + fs::remove_dir_all(target.as_path()).await?; + } // Atomic rename (lock is still held) fs::rename(source.as_path(), target.as_path()).await?; @@ -372,6 +378,8 @@ pub async fn move_to_cache( #[cfg(test)] mod tests { + use tempfile::TempDir; + use super::*; #[test] @@ -383,4 +391,69 @@ mod tests { assert_eq!(parse_max_age(""), None); assert_eq!(parse_max_age("max-age=invalid"), None); } + + fn cache_root(dir: &TempDir) -> AbsolutePathBuf { + AbsolutePathBuf::new(dir.path().to_path_buf()).unwrap() + } + + /// A fresh extracted download containing the binary. + async fn make_source(cache: &AbsolutePathBuf, contents: &[u8]) -> AbsolutePathBuf { + let source = cache.join("extract"); + tokio::fs::create_dir_all(source.join("bin").as_path()).await.unwrap(); + tokio::fs::write(source.join("bin").join("node").as_path(), contents).await.unwrap(); + source + } + + #[tokio::test] + async fn move_to_cache_moves_into_absent_target() { + let root = TempDir::new().unwrap(); + let cache = cache_root(&root); + let source = make_source(&cache, b"new").await; + + let target = cache.join("node").join("1.0.0"); + let binary = target.join("bin").join("node"); + + move_to_cache(&source, &target, &binary, "1.0.0").await.unwrap(); + + assert_eq!(tokio::fs::read(binary.as_path()).await.unwrap(), b"new"); + } + + #[tokio::test] + async fn move_to_cache_replaces_stale_incomplete_target() { + let root = TempDir::new().unwrap(); + let cache = cache_root(&root); + let source = make_source(&cache, b"new").await; + + // Stale install: directory exists but the binary is missing. + let target = cache.join("node").join("1.0.0"); + tokio::fs::create_dir_all(target.join("leftover").as_path()).await.unwrap(); + let binary = target.join("bin").join("node"); + + move_to_cache(&source, &target, &binary, "1.0.0").await.unwrap(); + + assert_eq!(tokio::fs::read(binary.as_path()).await.unwrap(), b"new"); + assert!(!tokio::fs::try_exists(target.join("leftover").as_path()).await.unwrap()); + } + + // Regression for the concurrent-install TOCTOU: a peer's complete install + // (dir + binary) must be kept, not clobbered by our own fresh copy. + #[tokio::test] + async fn move_to_cache_keeps_a_complete_target_intact() { + let root = TempDir::new().unwrap(); + let cache = cache_root(&root); + let source = make_source(&cache, b"ours").await; + + // A peer's complete install already at the target. + let target = cache.join("node").join("1.0.0"); + let binary = target.join("bin").join("node"); + tokio::fs::create_dir_all(target.join("bin").as_path()).await.unwrap(); + tokio::fs::write(binary.as_path(), b"peer").await.unwrap(); + + move_to_cache(&source, &target, &binary, "1.0.0").await.unwrap(); + + // The peer's binary is preserved, not clobbered. + assert_eq!(tokio::fs::read(binary.as_path()).await.unwrap(), b"peer"); + // Our source download is left untouched (not moved over the peer's install). + assert!(tokio::fs::try_exists(source.as_path()).await.unwrap()); + } } diff --git a/crates/vite_js_runtime/src/runtime.rs b/crates/vite_js_runtime/src/runtime.rs index 8be4c61cb1..d03dbd6ee5 100644 --- a/crates/vite_js_runtime/src/runtime.rs +++ b/crates/vite_js_runtime/src/runtime.rs @@ -205,14 +205,6 @@ pub async fn download_runtime_with_provider( }); } - // If install_dir exists but binary doesn't, it's an incomplete installation - clean it up - if tokio::fs::try_exists(&install_dir).await.unwrap_or(false) { - tracing::warn!( - "Incomplete installation detected at {install_dir:?}, removing before re-download" - ); - tokio::fs::remove_dir_all(&install_dir).await?; - } - let download_message = format!("Downloading {} v{version}...", provider.name()); tracing::info!("{download_message}"); @@ -272,7 +264,7 @@ pub async fn download_runtime_with_provider( .await?; // Move extracted directory to cache location - move_to_cache(&extracted_path, &install_dir, version).await?; + move_to_cache(&extracted_path, &install_dir, &binary_path, version).await?; tracing::info!("{} {version} installed at {install_dir:?}", provider.name());