Skip to content

fix(lockfile): add atomic writes and cache invalidation #7927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jdx merged 1 commit into from
Jan 31, 2026
Merged

fix(lockfile): add atomic writes and cache invalidation #7927

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 29 additions & 9 deletions src/lockfile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::toolset::{ToolSource, ToolVersion, ToolVersionList, Toolset};
use eyre::{Report, Result, bail};
use itertools::Itertools;
use serde_derive::{Deserialize, Serialize};
use std::fs;
use std::path::{Path, PathBuf};
use std::sync::LazyLock as Lazy;
use std::sync::Mutex;
Expand All @@ -17,6 +18,22 @@ use std::{
use toml_edit::DocumentMut;
use xx::regex;

/// Global caches for lockfile data - declared here so invalidation can access them
static ALL_LOCKFILES_CACHE: Lazy<Mutex<HashMap<Vec<PathBuf>, Arc<Lockfile>>>> =
Lazy::new(Default::default);
static SINGLE_LOCKFILE_CACHE: Lazy<Mutex<HashMap<PathBuf, Arc<Lockfile>>>> =
Lazy::new(Default::default);

/// Invalidate all lockfile caches. Call this after modifying a lockfile.
pub fn invalidate_caches() {
if let Ok(mut cache) = ALL_LOCKFILES_CACHE.lock() {
cache.clear();
}
if let Ok(mut cache) = SINGLE_LOCKFILE_CACHE.lock() {
cache.clear();
}
Comment on lines +29 to +34

@gemini-code-assist gemini-code-assist Bot Jan 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation of invalidate_caches silently ignores poisoned mutexes. If a thread panics while holding a cache lock, the cache may not be invalidated for the lifetime of the process, leading to stale data. It's more robust to recover from the poison and clear the cache anyway, as the clear() operation is safe even with potentially inconsistent data.

    ALL_LOCKFILES_CACHE.lock().unwrap_or_else(|e| e.into_inner()).clear();
    SINGLE_LOCKFILE_CACHE.lock().unwrap_or_else(|e| e.into_inner()).clear();

}

#[derive(Debug, Clone, Default, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct Lockfile {
Expand Down Expand Up @@ -213,8 +230,10 @@ impl Lockfile {
}

fn save<P: AsRef<Path>>(&self, path: P) -> Result<()> {
let path = path.as_ref();
if self.is_empty() {
let _ = file::remove_file(path);
invalidate_caches();
} else {
let mut lockfile = toml::Table::new();

Expand Down Expand Up @@ -252,7 +271,14 @@ impl Lockfile {

let content = toml::to_string_pretty(&toml::Value::Table(lockfile))?;
let content = format(content.parse()?);
file::write(path, content)?;

// Use atomic write: write to temp file, then rename
// This prevents partial writes from corrupting the lockfile
let temp_path = path.with_extension("lock.tmp");

Copilot AI Jan 31, 2026

Copy link

Choose a reason for hiding this comment

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

The temp file extension 'lock.tmp' will replace the existing extension rather than appending to it. For a file like 'config.lock', this creates 'config.tmp' instead of 'config.lock.tmp'. Consider using a pattern like path.with_file_name(format!(\"{}.tmp\", path.file_name().unwrap().to_str().unwrap())) or a similar approach to preserve the original filename structure.

Suggested change
let temp_path = path.with_extension("lock.tmp");
let temp_path = match path.file_name().and_then(|name| name.to_str()) {
Some(name) => path.with_file_name(format!("{name}.tmp")),
None => path.with_extension("lock.tmp"),
};

Copilot uses AI. Check for mistakes.
file::write(&temp_path, &content)?;
fs::rename(&temp_path, path)?;

Copilot AI Jan 31, 2026

Copy link

Choose a reason for hiding this comment

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

If fs::rename fails after successfully writing the temp file, the temporary file will be left behind without cleanup. Consider wrapping this operation in a guard or adding error handling to remove the temp file on failure.

Suggested change
fs::rename(&temp_path, path)?;
if let Err(err) = fs::rename(&temp_path, path) {
// Best-effort cleanup of temporary file if rename fails
let _ = fs::remove_file(&temp_path);
return Err(err.into());
}

Copilot uses AI. Check for mistakes.

invalidate_caches();
}
Ok(())
}
Expand Down Expand Up @@ -646,13 +672,10 @@ fn merge_tool_entries_with_env(
}

fn read_all_lockfiles(config: &Config) -> Arc<Lockfile> {
// Cache by sorted config paths to avoid recomputing on every call
static CACHE: Lazy<Mutex<HashMap<Vec<PathBuf>, Arc<Lockfile>>>> = Lazy::new(Default::default);

// Create a cache key from the config file paths
let cache_key: Vec<PathBuf> = config.config_files.keys().cloned().collect();

let mut cache = CACHE.lock().unwrap();
let mut cache = ALL_LOCKFILES_CACHE.lock().unwrap();
if let Some(cached) = cache.get(&cache_key) {
return Arc::clone(cached);
}
Expand Down Expand Up @@ -704,10 +727,7 @@ fn read_all_lockfiles(config: &Config) -> Arc<Lockfile> {
}

fn read_lockfile_for(path: &Path) -> Arc<Lockfile> {
// Cache by config path to avoid recomputing lockfile_path_for_config on every call
static CACHE: Lazy<Mutex<HashMap<PathBuf, Arc<Lockfile>>>> = Lazy::new(Default::default);

let mut cache = CACHE.lock().unwrap();
let mut cache = SINGLE_LOCKFILE_CACHE.lock().unwrap();
if let Some(cached) = cache.get(path) {
return Arc::clone(cached);
}
Expand Down
Loading