Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 28 additions & 8 deletions src/backend/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,18 +97,36 @@ impl HttpBackend {
let extracted_path = self.get_cached_extracted_path(cache_key);
let metadata_path = self.get_cache_metadata_path(cache_key);

// Create cache directory
file::create_dir_all(&cache_path)?;
// Ensure parent directory exists (we'll atomically rename into it)
if let Some(parent) = cache_path.parent() {
file::create_dir_all(parent)?;
}

// Remove existing extracted contents if they exist
// Create a unique temporary directory for atomic extraction
let pid = std::process::id();
let now_ms = SystemTime::now().duration_since(UNIX_EPOCH)?.as_millis();
let tmp_dir_name = format!("{}.tmp-{}-{}", cache_key, pid, now_ms);
Comment on lines +106 to +108
Copy link

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temporary directory naming scheme uses process ID and timestamp but doesn't handle potential collisions if the same process creates multiple temp dirs within the same millisecond. Consider using a random component or atomic counter to ensure uniqueness.

Copilot uses AI. Check for mistakes.
let tmp_extract_path = match cache_path.parent() {
Some(parent) => parent.join(tmp_dir_name),
None => cache_path.with_extension(format!("tmp-{}-{}", pid, now_ms)),
};

// Ensure any previous temp dir is removed
if tmp_extract_path.exists() {
let _ = file::remove_all(&tmp_extract_path);
Copy link

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error from file::remove_all is silently ignored with let _. If removal fails, the subsequent extraction may fail or behave unexpectedly. Consider logging the error or handling it appropriately.

Suggested change
let _ = file::remove_all(&tmp_extract_path);
if let Err(e) = file::remove_all(&tmp_extract_path) {
eprintln!("Warning: Failed to remove temp directory {:?}: {}", tmp_extract_path, e);
}

Copilot uses AI. Check for mistakes.
}

// Perform extraction into the temp directory
self.extract_artifact_to_cache(file_path, &tmp_extract_path, tv, opts, pr)?;

// Replace any existing extracted cache atomically
if extracted_path.exists() {
file::remove_all(&extracted_path)?;
}
// Rename temp directory to the final cache path
std::fs::rename(&tmp_extract_path, &extracted_path)?;

// Extract tarball to cache
self.extract_artifact_to_cache(file_path, &extracted_path, tv, opts, pr)?;

// Create metadata
// Only write metadata after the extracted directory is in place
let metadata = CacheMetadata {
url: url.to_string(),
checksum: lookup_platform_key(opts, "checksum")
Expand All @@ -118,7 +136,6 @@ impl HttpBackend {
platform: self.get_platform_key(),
};

// Write metadata
let metadata_json = serde_json::to_string_pretty(&metadata)?;
file::write(&metadata_path, metadata_json)?;

Expand Down Expand Up @@ -344,6 +361,9 @@ impl Backend for HttpBackend {
let cache_key = self.get_file_based_cache_key(&file_path, &opts)?;
let cached_extracted_path = self.get_cached_extracted_path(&cache_key);

// Acquire a cache-level lock to serialize extraction for this cache key
let _cache_lock = crate::lock_file::get(&cached_extracted_path, ctx.force)?;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Cache Directory Creation Race Condition

The cache-level lock is acquired on cached_extracted_path before its parent directory is ensured to exist. On a cold cache, lock_file::get may fail when trying to create the lock file in a non-existent directory (e.g., dirs::CACHE/http-tarballs).

Fix in Cursor Fix in Web

// Check if tarball is already cached
if self.is_tarball_cached(&cache_key) {
ctx.pr.set_message("using cached tarball".into());
Expand Down
Loading