From 83f19265cdaf1fd18e0989468cc96e40bd0d4056 Mon Sep 17 00:00:00 2001 From: Kristoffer Carlsson Date: Mon, 17 Aug 2026 14:55:04 +0200 Subject: [PATCH] Fix concurrent add dropping downloaded version --- devdocs/locking_and_update_flows.md | 13 +- src/command_add.rs | 202 ++++++++++++++++++++++++++-- 2 files changed, 201 insertions(+), 14 deletions(-) diff --git a/devdocs/locking_and_update_flows.md b/devdocs/locking_and_update_flows.md index 19216406..0b9f8c82 100644 --- a/devdocs/locking_and_update_flows.md +++ b/devdocs/locking_and_update_flows.md @@ -148,13 +148,20 @@ flowchart TD inst -->|yes| ret["print + return"] inst -->|no| dl["download_version_to_temp
NO lock held"] dl --> wl["load_mut_config_db
exclusive lock"] - wl --> recheck{"installed by
another process?"} - recheck -->|yes| ret2["print + return"] - recheck -->|no| commit["commit_version_install
(rename temp → final)"] + wl --> recheck{"concurrent channel
with same name?"} + recheck -->|"same system version
or explicit non-system channel"| ret2["discard download
print + return"] + recheck -->|"different system version
or no channel"| commit["commit_version_install
(rename temp → final)"] commit --> cfg["insert channel + save_config_db"] cfg --> done(["release"]) ``` +The commit re-check is version-aware. A concurrent system channel at the same +version makes the download redundant, while a system channel at a different +version is moved to the version resolved by this `add` and the download is +registered for reuse. Concurrent linked, direct-download, and alias channels +are explicit claims on the channel name, so `add` preserves them and discards +its database-version download. + For nightly/PR channels (`add_non_db`), `install_non_db_version` → `install_from_url` downloads into a temp dir and atomically renames into place with **no lock held**; the exclusive lock is taken afterwards only to write the diff --git a/src/command_add.rs b/src/command_add.rs index 071d47ad..b474e51f 100644 --- a/src/command_add.rs +++ b/src/command_add.rs @@ -1,5 +1,5 @@ use crate::config_file::{ - load_config_db, load_mut_config_db, save_config_db, JuliaupConfigChannel, + load_config_db, load_mut_config_db, save_config_db, JuliaupConfig, JuliaupConfigChannel, }; use crate::global_paths::GlobalPaths; #[cfg(not(windows))] @@ -12,6 +12,45 @@ use crate::utils::{print_juliaup_style, JuliaupMessageType}; use crate::versions_file::load_versions_db; use anyhow::{anyhow, Context, Result}; use regex::Regex; +use tempfile::TempDir; + +#[derive(Debug, PartialEq)] +enum AddChannelOutcome { + Installed, + AlreadyInstalled, +} + +/// Commits a downloaded database version after re-checking the channel under +/// the exclusive configuration lock. +/// +/// A concurrent system-channel install only wins when it selected the same +/// version. If it selected a different version, keep the completed download +/// and move the channel to the version this `add` resolved before downloading. +/// Non-system channels are explicit name claims and are never overwritten. +fn commit_downloaded_channel( + channel: &str, + required_version: &str, + downloaded: TempDir, + config_data: &mut JuliaupConfig, + paths: &GlobalPaths, +) -> Result { + match config_data.installed_channels.get(channel) { + Some(JuliaupConfigChannel::SystemChannel { version }) if version != required_version => {} + Some(_) => return Ok(AddChannelOutcome::AlreadyInstalled), + None => {} + } + + commit_version_install(downloaded, required_version, config_data, paths)?; + + config_data.installed_channels.insert( + channel.to_string(), + JuliaupConfigChannel::SystemChannel { + version: required_version.to_string(), + }, + ); + + Ok(AddChannelOutcome::Installed) +} pub fn run_command_add(channel: &str, paths: &GlobalPaths) -> Result<()> { // This regex is dynamically compiled, but its runtime is negligible compared to downloading Julia @@ -58,20 +97,18 @@ pub fn run_command_add(channel: &str, paths: &GlobalPaths) -> Result<()> { let mut config_file = load_mut_config_db(paths) .with_context(|| "`add` command failed to load configuration data.")?; - if config_file.data.installed_channels.contains_key(channel) { + if commit_downloaded_channel( + channel, + required_version, + downloaded, + &mut config_file.data, + paths, + )? == AddChannelOutcome::AlreadyInstalled + { eprintln!("'{}' is already installed.", channel); return Ok(()); } - commit_version_install(downloaded, required_version, &mut config_file.data, paths)?; - - config_file.data.installed_channels.insert( - channel.to_string(), - JuliaupConfigChannel::SystemChannel { - version: required_version.clone(), - }, - ); - if config_file.data.default.is_none() { config_file.data.default = Some(channel.to_string()); } @@ -174,3 +211,146 @@ fn add_non_db(channel: &str, paths: &GlobalPaths) -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::config_file::JuliaupConfigVersion; + use std::path::Path; + use tempfile::Builder; + + fn test_paths(dir: &Path) -> GlobalPaths { + GlobalPaths { + juliauphome: dir.to_path_buf(), + juliaupconfig: dir.join("juliaup.json"), + lockfile: dir.join(".juliaup-lock"), + versiondb: dir.join("versiondb-test.json"), + #[cfg(feature = "selfupdate")] + juliaupselfhome: dir.to_path_buf(), + #[cfg(feature = "selfupdate")] + juliaupselfconfig: dir.join("juliaupself.json"), + #[cfg(feature = "selfupdate")] + juliaupselfbin: dir.to_path_buf(), + } + } + + fn downloaded_install(dir: &Path, marker: &str) -> Result { + let downloaded = Builder::new().prefix("julia-temp-").tempdir_in(dir)?; + std::fs::create_dir_all(downloaded.path().join("bin"))?; + std::fs::write(downloaded.path().join("bin/julia"), marker)?; + Ok(downloaded) + } + + fn installed_version(path: &str) -> JuliaupConfigVersion { + JuliaupConfigVersion { + path: path.to_string(), + binary_path: None, + } + } + + #[test] + fn concurrent_different_system_version_commits_download() -> Result<()> { + let dir = tempfile::tempdir()?; + let paths = test_paths(dir.path()); + let downloaded = downloaded_install(dir.path(), "downloaded")?; + let mut config = JuliaupConfig::default(); + + config.installed_versions.insert( + "1.10.11+0.test".to_string(), + installed_version("./julia-1.10.11+0.test"), + ); + config.installed_channels.insert( + "1.10".to_string(), + JuliaupConfigChannel::SystemChannel { + version: "1.10.11+0.test".to_string(), + }, + ); + + let outcome = + commit_downloaded_channel("1.10", "1.10.12+0.test", downloaded, &mut config, &paths)?; + + assert_eq!(outcome, AddChannelOutcome::Installed); + assert!(config.installed_versions.contains_key("1.10.12+0.test")); + assert!(matches!( + config.installed_channels.get("1.10"), + Some(JuliaupConfigChannel::SystemChannel { version }) + if version == "1.10.12+0.test" + )); + assert_eq!( + std::fs::read_to_string(paths.juliauphome.join("julia-1.10.12+0.test/bin/julia"))?, + "downloaded" + ); + Ok(()) + } + + #[test] + fn concurrent_same_system_version_discards_download() -> Result<()> { + let dir = tempfile::tempdir()?; + let paths = test_paths(dir.path()); + let downloaded = downloaded_install(dir.path(), "duplicate")?; + let downloaded_path = downloaded.path().to_path_buf(); + let target = paths.juliauphome.join("julia-1.10.12+0.test/bin"); + std::fs::create_dir_all(&target)?; + std::fs::write(target.join("julia"), "existing")?; + + let mut config = JuliaupConfig::default(); + config.installed_versions.insert( + "1.10.12+0.test".to_string(), + installed_version("./julia-1.10.12+0.test"), + ); + config.installed_channels.insert( + "1.10".to_string(), + JuliaupConfigChannel::SystemChannel { + version: "1.10.12+0.test".to_string(), + }, + ); + + let outcome = + commit_downloaded_channel("1.10", "1.10.12+0.test", downloaded, &mut config, &paths)?; + + assert_eq!(outcome, AddChannelOutcome::AlreadyInstalled); + assert!(!downloaded_path.exists()); + assert_eq!(std::fs::read_to_string(target.join("julia"))?, "existing"); + Ok(()) + } + + fn assert_concurrent_explicit_channel_is_preserved( + explicit_channel: JuliaupConfigChannel, + ) -> Result<()> { + let dir = tempfile::tempdir()?; + let paths = test_paths(dir.path()); + let mut config = JuliaupConfig::default(); + config + .installed_channels + .insert("1.10".to_string(), explicit_channel.clone()); + let downloaded = downloaded_install(dir.path(), "downloaded")?; + let downloaded_path = downloaded.path().to_path_buf(); + + let outcome = + commit_downloaded_channel("1.10", "1.10.12+0.test", downloaded, &mut config, &paths)?; + + assert_eq!(outcome, AddChannelOutcome::AlreadyInstalled); + assert!(!downloaded_path.exists()); + assert!(config.installed_channels.get("1.10") == Some(&explicit_channel)); + assert!(!config.installed_versions.contains_key("1.10.12+0.test")); + Ok(()) + } + + #[test] + fn concurrent_explicit_channels_preserve_name_claim() -> Result<()> { + assert_concurrent_explicit_channel_is_preserved(JuliaupConfigChannel::LinkedChannel { + command: "/custom/julia".to_string(), + args: None, + })?; + assert_concurrent_explicit_channel_is_preserved( + JuliaupConfigChannel::DirectDownloadChannel { + path: "./julia-1.10".to_string(), + url: "https://example.com/julia.tar.gz".to_string(), + local_etag: "etag".to_string(), + server_etag: "etag".to_string(), + version: "1.10.99-DEV".to_string(), + binary_path: None, + }, + ) + } +}