From 28da4ffb83d02efce0aed2f9c9848ccfa2afc75c Mon Sep 17 00:00:00 2001 From: Loong Date: Mon, 27 Jul 2026 16:58:07 +0100 Subject: [PATCH 1/4] fix: make file replacements atomic Stage single-file writes beside their destinations and preserve Unix creation permissions. Commit ETag-backed downloads before updating validators so failed transfers keep the previous cache usable. --- Cargo.lock | 10 + Cargo.toml | 1 + crates/maa-atomic-fs/Cargo.toml | 13 ++ crates/maa-atomic-fs/src/lib.rs | 256 ++++++++++++++++++++++ crates/maa-cli/Cargo.toml | 1 + crates/maa-cli/src/atomic_fs.rs | 126 ----------- crates/maa-cli/src/config/asst.rs | 2 +- crates/maa-cli/src/config/import.rs | 11 +- crates/maa-cli/src/config/mod.rs | 2 +- crates/maa-cli/src/main.rs | 1 - crates/maa-cli/src/run/preset/copilot.rs | 4 +- crates/maa-installer/Cargo.toml | 1 + crates/maa-installer/src/download/etag.rs | 192 +++++++++++++++- xtask/Cargo.toml | 1 + xtask/src/release/package.rs | 92 ++++++-- 15 files changed, 546 insertions(+), 167 deletions(-) create mode 100644 crates/maa-atomic-fs/Cargo.toml create mode 100644 crates/maa-atomic-fs/src/lib.rs delete mode 100644 crates/maa-cli/src/atomic_fs.rs diff --git a/Cargo.lock b/Cargo.lock index 80ac9673..e90af3f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1042,6 +1042,13 @@ version = "0.4.33" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0ceec5bc11778974d1bcb055b18002eba7f4b3518b6a0081b3af5f21666da9ad" +[[package]] +name = "maa-atomic-fs" +version = "0.1.0" +dependencies = [ + "tempfile", +] + [[package]] name = "maa-cli" version = "0.7.5" @@ -1057,6 +1064,7 @@ dependencies = [ "git2", "indicatif", "log", + "maa-atomic-fs", "maa-core", "maa-dirs", "maa-installer", @@ -1129,6 +1137,7 @@ dependencies = [ "flate2", "indicatif", "log", + "maa-atomic-fs", "semver", "sha2", "tar", @@ -2529,6 +2538,7 @@ dependencies = [ "clap", "digest-io", "flate2", + "maa-atomic-fs", "maa-dirs", "maa-version", "semver", diff --git a/Cargo.toml b/Cargo.toml index 0f3196cd..e8874fb4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ indexmap = "2.13.1" indicatif = "0.18" libloading = "0.9" log = "0.4.20" +maa-atomic-fs = { path = "crates/maa-atomic-fs", version = "0.1" } maa-core = { path = "crates/maa-core", version = "0.1", default-features = false } maa-dirs = { path = "crates/maa-dirs", version = "0.3" } maa-ffi-string = { path = "crates/maa-ffi-string", version = "0.1" } diff --git a/crates/maa-atomic-fs/Cargo.toml b/crates/maa-atomic-fs/Cargo.toml new file mode 100644 index 00000000..e7966a86 --- /dev/null +++ b/crates/maa-atomic-fs/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "maa-atomic-fs" +version = "0.1.0" +edition.workspace = true +description = "Atomic file replacement utilities" +repository.workspace = true +license.workspace = true + +[dependencies] +tempfile = { workspace = true } + +[lints] +workspace = true diff --git a/crates/maa-atomic-fs/src/lib.rs b/crates/maa-atomic-fs/src/lib.rs new file mode 100644 index 00000000..9dd20af8 --- /dev/null +++ b/crates/maa-atomic-fs/src/lib.rs @@ -0,0 +1,256 @@ +//! Utilities for atomically replacing individual files. +//! +//! Writes are staged in a temporary file beside the destination, synchronized, +//! and then atomically moved into place. A failure before the final replacement +//! leaves the destination unchanged. +//! +//! On Unix, write operations preserve the permission bits of an existing +//! destination, while a new destination uses normal file creation permissions +//! (`0o666` filtered by the process umask). Other file metadata is not +//! preserved. +//! +//! These operations do not provide multi-file or directory transactions, file +//! locking, or parent-directory synchronization for power-loss durability. + +use std::{ + fs::{self, File}, + io::{self, Read, Write}, + path::Path, +}; + +use tempfile::NamedTempFile; + +/// Atomically replaces `path` with `content`. +/// +/// On Unix, the replacement preserves an existing destination's permission +/// bits, while a new destination uses normal file creation permissions. +pub fn write(path: impl AsRef, content: impl AsRef<[u8]>) -> io::Result<()> { + write_with(path, |file| file.write_all(content.as_ref())) +} + +/// Atomically replaces `path` with all bytes read from `reader`. +/// +/// On Unix, the replacement preserves an existing destination's permission +/// bits, while a new destination uses normal file creation permissions. +pub fn write_from(path: impl AsRef, reader: &mut impl Read) -> io::Result { + write_with(path, |file| io::copy(reader, file)) +} + +/// Atomically replaces `to` with a copy of `from`. +/// +/// On Unix, the replacement inherits the source file's permission bits through +/// [`fs::copy`]. +pub fn copy(from: impl AsRef, to: impl AsRef) -> io::Result { + let from = from.as_ref(); + write_with_temp(to.as_ref(), |temp| fs::copy(from, temp.path())) +} + +/// Atomically replaces `path` after `fill` populates a staging file. +/// +/// `fill` may return any error type that can absorb [`io::Error`]. The +/// destination remains unchanged if filling, synchronizing, or replacing the +/// file fails. +/// +/// On Unix, the replacement preserves an existing destination's permission +/// bits, while a new destination uses normal file creation permissions. +pub fn write_with(path: P, fill: F) -> Result +where + P: AsRef, + F: FnOnce(&mut File) -> Result, + E: From, +{ + let path = path.as_ref(); + let (mut temp, final_permissions) = new_write_temp(path)?; + let result = fill(temp.as_file_mut())?; + if let Some(permissions) = final_permissions { + temp.as_file().set_permissions(permissions)?; + } + commit_temp(temp, path, result) +} + +fn write_with_temp( + path: &Path, + fill: impl FnOnce(&mut NamedTempFile) -> Result, +) -> Result +where + E: From, +{ + let mut temp = NamedTempFile::new_in(parent_dir(path)?)?; + let result = fill(&mut temp)?; + commit_temp(temp, path, result) +} + +fn commit_temp(mut temp: NamedTempFile, path: &Path, result: T) -> Result +where + E: From, +{ + temp.as_file_mut().sync_all()?; + temp.into_temp_path() + .persist(path) + .map_err(|error| error.error)?; + Ok(result) +} + +fn new_write_temp(path: &Path) -> io::Result<(NamedTempFile, Option)> { + let final_permissions = match fs::metadata(path) { + Ok(metadata) => Some(metadata.permissions()), + Err(error) if error.kind() == io::ErrorKind::NotFound => None, + Err(error) => return Err(error), + }; + let parent = parent_dir(path)?; + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + if final_permissions.is_none() { + let temp = tempfile::Builder::new() + .permissions(fs::Permissions::from_mode(0o666)) + .tempfile_in(parent)?; + let permissions = temp.as_file().metadata()?.permissions(); + temp.as_file() + .set_permissions(fs::Permissions::from_mode(0o600))?; + return Ok((temp, Some(permissions))); + } + } + + Ok((NamedTempFile::new_in(parent)?, final_permissions)) +} + +fn parent_dir(path: &Path) -> io::Result<&Path> { + match path.parent() { + None => Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("Path {} has no parent directory", path.display()), + )), + Some(parent) if parent.as_os_str().is_empty() => Ok(Path::new(".")), + Some(parent) => Ok(parent), + } +} + +#[cfg(test)] +#[cfg_attr(coverage_nightly, coverage(off))] +mod tests { + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; + use std::{fs, io::Cursor}; + + use tempfile::tempdir; + + use super::*; + + #[test] + fn write_replaces_existing_content() { + let temp_dir = tempdir().unwrap(); + let path = temp_dir.path().join("config.json"); + fs::write(&path, "old").unwrap(); + + write(&path, "new").unwrap(); + + assert_eq!(fs::read_to_string(&path).unwrap(), "new"); + } + + #[test] + fn write_from_replaces_existing_content() { + let temp_dir = tempdir().unwrap(); + let path = temp_dir.path().join("config.json"); + fs::write(&path, "old").unwrap(); + let mut reader = Cursor::new(br#"{"foo":"bar"}"#); + + write_from(&path, &mut reader).unwrap(); + + assert_eq!(fs::read(&path).unwrap(), br#"{"foo":"bar"}"#); + } + + #[test] + fn copy_replaces_existing_content() { + let temp_dir = tempdir().unwrap(); + let source = temp_dir.path().join("source.json"); + let target = temp_dir.path().join("target.json"); + fs::write(&source, "new").unwrap(); + fs::write(&target, "old").unwrap(); + + copy(&source, &target).unwrap(); + + assert_eq!(fs::read_to_string(&target).unwrap(), "new"); + } + + #[cfg(unix)] + #[test] + fn write_preserves_existing_permissions() { + let temp_dir = tempdir().unwrap(); + let path = temp_dir.path().join("config.json"); + fs::write(&path, "old").unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o640)).unwrap(); + + write(&path, "new").unwrap(); + + assert_eq!( + fs::metadata(path).unwrap().permissions().mode() & 0o777, + 0o640 + ); + } + + #[cfg(unix)] + #[test] + fn new_file_uses_regular_creation_permissions() { + let temp_dir = tempdir().unwrap(); + let regular = temp_dir.path().join("regular.json"); + let atomic = temp_dir.path().join("atomic.json"); + File::create(®ular).unwrap(); + + write(&atomic, "new").unwrap(); + + let regular_mode = fs::metadata(regular).unwrap().permissions().mode() & 0o777; + let atomic_mode = fs::metadata(atomic).unwrap().permissions().mode() & 0o777; + assert_eq!(atomic_mode, regular_mode); + } + + #[cfg(unix)] + #[test] + fn copy_uses_source_permissions() { + let temp_dir = tempdir().unwrap(); + let source = temp_dir.path().join("source.json"); + let target = temp_dir.path().join("target.json"); + fs::write(&source, "new").unwrap(); + fs::set_permissions(&source, fs::Permissions::from_mode(0o640)).unwrap(); + fs::write(&target, "old").unwrap(); + fs::set_permissions(&target, fs::Permissions::from_mode(0o600)).unwrap(); + + copy(&source, &target).unwrap(); + + assert_eq!( + fs::metadata(target).unwrap().permissions().mode() & 0o777, + 0o640 + ); + } + + #[test] + fn failed_fill_leaves_original_intact() { + let temp_dir = tempdir().unwrap(); + let path = temp_dir.path().join("config.json"); + fs::write(&path, "original").unwrap(); + + let result = write_with::<_, _, (), io::Error>(&path, |_file| { + Err(io::Error::other("simulated failure")) + }); + + assert!(result.is_err()); + assert_eq!(fs::read_to_string(&path).unwrap(), "original"); + } + + #[test] + fn bare_filename_uses_current_directory() { + assert_eq!( + parent_dir(Path::new("output.json")).unwrap(), + Path::new(".") + ); + } + + #[test] + fn rejects_root_path() { + let error = write("/", "new").unwrap_err(); + + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + } +} diff --git a/crates/maa-cli/Cargo.toml b/crates/maa-cli/Cargo.toml index 7ec6d5e1..0061e14e 100644 --- a/crates/maa-cli/Cargo.toml +++ b/crates/maa-cli/Cargo.toml @@ -22,6 +22,7 @@ env_logger = { workspace = true, features = ["auto-color"] } git2 = { workspace = true, optional = true } indicatif = { workspace = true } log = { workspace = true } +maa-atomic-fs = { workspace = true } maa-dirs = { workspace = true } maa-installer = { workspace = true } maa-str-ext = { workspace = true } diff --git a/crates/maa-cli/src/atomic_fs.rs b/crates/maa-cli/src/atomic_fs.rs deleted file mode 100644 index 73f575b0..00000000 --- a/crates/maa-cli/src/atomic_fs.rs +++ /dev/null @@ -1,126 +0,0 @@ -use std::{ - fs, - io::{self, Read, Write}, - path::Path, -}; - -use tempfile::NamedTempFile; - -pub fn write(path: impl AsRef, content: impl AsRef<[u8]>) -> io::Result<()> { - write_with(path, |temp| temp.write_all(content.as_ref())) -} - -pub fn write_from(path: impl AsRef, reader: &mut impl Read) -> io::Result { - write_with(path, |temp| io::copy(reader, temp)) -} - -pub fn copy(from: impl AsRef, to: impl AsRef) -> io::Result { - let from = from.as_ref(); - write_with(to, |temp| fs::copy(from, temp.path())) -} - -/// Atomically write to `path` by letting `fill` populate a staging temp file, -/// then fsync + rename. The closure can fail with any error type that -/// absorbs `io::Error`, so callers may return e.g. `serde_json::Error`. -pub fn write_with(path: P, fill: F) -> Result -where - P: AsRef, - F: FnOnce(&mut NamedTempFile) -> Result, - E: From, -{ - let path = path.as_ref(); - let mut temp = NamedTempFile::new_in(parent_dir(path)?)?; - let result = fill(&mut temp)?; - temp.as_file_mut().sync_all()?; - persist(temp.into_temp_path(), path)?; - Ok(result) -} - -fn persist(temp_path: tempfile::TempPath, path: &Path) -> io::Result<()> { - temp_path.persist(path).map_err(|e| e.error) -} - -fn parent_dir(path: &Path) -> io::Result<&Path> { - match path.parent() { - None => Err(io::Error::new( - io::ErrorKind::InvalidInput, - format!("Path {} has no parent directory", path.display()), - )), - Some(p) if p.as_os_str().is_empty() => Ok(Path::new(".")), - Some(p) => Ok(p), - } -} - -#[cfg(test)] -#[cfg_attr(coverage_nightly, coverage(off))] -mod tests { - use std::{fs, io::Cursor}; - - use tempfile::tempdir; - - use super::*; - - #[test] - fn write_replaces_existing_content() { - let temp_dir = tempdir().unwrap(); - let path = temp_dir.path().join("config.json"); - - fs::write(&path, "old").unwrap(); - - write(&path, "new").unwrap(); - assert_eq!(fs::read_to_string(&path).unwrap(), "new"); - } - - #[test] - fn write_from_writes_stream() { - let temp_dir = tempdir().unwrap(); - let path = temp_dir.path().join("config.json"); - let mut reader = Cursor::new(br#"{"foo":"bar"}"#); - - write_from(&path, &mut reader).unwrap(); - assert_eq!(fs::read(&path).unwrap(), br#"{"foo":"bar"}"#); - } - - #[test] - fn copy_replaces_existing_content() { - let temp_dir = tempdir().unwrap(); - let source = temp_dir.path().join("source.json"); - let target = temp_dir.path().join("target.json"); - - fs::write(&source, "new").unwrap(); - fs::write(&target, "old").unwrap(); - - copy(&source, &target).unwrap(); - assert_eq!(fs::read_to_string(&target).unwrap(), "new"); - } - - #[test] - fn bare_filename_uses_current_directory() { - assert_eq!( - parent_dir(Path::new("output.json")).unwrap(), - Path::new(".") - ); - } - - #[test] - fn rejects_root_path() { - let error = write("/", "new").unwrap_err(); - - assert_eq!(error.kind(), io::ErrorKind::InvalidInput); - } - - #[test] - fn failed_fill_leaves_original_intact() { - let temp_dir = tempdir().unwrap(); - let path = temp_dir.path().join("config.json"); - - fs::write(&path, "original").unwrap(); - - let result = write_with::<_, _, (), io::Error>(&path, |_temp| { - Err(io::Error::other("simulated failure")) - }); - - assert!(result.is_err()); - assert_eq!(fs::read_to_string(&path).unwrap(), "original"); - } -} diff --git a/crates/maa-cli/src/config/asst.rs b/crates/maa-cli/src/config/asst.rs index 5e9c5745..a44e4061 100644 --- a/crates/maa-cli/src/config/asst.rs +++ b/crates/maa-cli/src/config/asst.rs @@ -497,7 +497,7 @@ fn migrate_legacy_tasks_json(resource_dir: &Path, hot_update_root: &Path) -> Res bail!("Expected {} to be a directory", new_dir.display()); } - crate::atomic_fs::copy(&old, &new) + maa_atomic_fs::copy(&old, &new) .with_context(|| format!("Failed to copy {} to {}", old.display(), new.display()))?; Ok(()) diff --git a/crates/maa-cli/src/config/import.rs b/crates/maa-cli/src/config/import.rs index 05df7bab..6e69ffbe 100644 --- a/crates/maa-cli/src/config/import.rs +++ b/crates/maa-cli/src/config/import.rs @@ -7,7 +7,7 @@ use anyhow::{Context, Result, bail}; use maa_dirs::Ensure; use super::{Filetype, SUPPORTED_EXTENSION}; -use crate::{atomic_fs, state::AGENT}; +use crate::state::AGENT; /// Represents the source of a configuration file to import #[cfg_attr(test, derive(PartialEq))] @@ -65,11 +65,12 @@ impl<'a> ImportSource<'a> { match self { ImportSource::Remote(url) => { let response = AGENT.get(url).call()?; - atomic_fs::write_from(target, &mut response.into_body().as_reader()).with_context( - || format!("Failed to write imported file to {}", target.display()), - ) + maa_atomic_fs::write_from(target, &mut response.into_body().as_reader()) + .with_context(|| { + format!("Failed to write imported file to {}", target.display()) + }) } - ImportSource::Local(path) => atomic_fs::copy(path, target).with_context(|| { + ImportSource::Local(path) => maa_atomic_fs::copy(path, target).with_context(|| { format!( "Failed to copy file from {} to {}", path.display(), diff --git a/crates/maa-cli/src/config/mod.rs b/crates/maa-cli/src/config/mod.rs index 29c89c83..82941abd 100644 --- a/crates/maa-cli/src/config/mod.rs +++ b/crates/maa-cli/src/config/mod.rs @@ -66,7 +66,7 @@ impl Filetype { where T: serde::Serialize, { - crate::atomic_fs::write_with(path, |temp| self.write_to(temp, value)) + maa_atomic_fs::write_with(path, |file| self.write_to(file, value)) } fn write_to(&self, mut writer: W, value: &T) -> Result<()> diff --git a/crates/maa-cli/src/main.rs b/crates/maa-cli/src/main.rs index 89becacf..bcd1dba8 100644 --- a/crates/maa-cli/src/main.rs +++ b/crates/maa-cli/src/main.rs @@ -5,7 +5,6 @@ use maa_dirs as dirs; #[macro_use(join)] extern crate maa_dirs; -mod atomic_fs; mod log; mod state; diff --git a/crates/maa-cli/src/run/preset/copilot.rs b/crates/maa-cli/src/run/preset/copilot.rs index 8f513750..41cd5071 100644 --- a/crates/maa-cli/src/run/preset/copilot.rs +++ b/crates/maa-cli/src/run/preset/copilot.rs @@ -469,7 +469,7 @@ impl CopilotFile { // (os error 5) when the file is locked by another process. If another // thread/process already wrote the cache file we can safely move on. #[cfg(windows)] - if let Err(e) = crate::atomic_fs::write(&json_file, &content) { + if let Err(e) = maa_atomic_fs::write(&json_file, &content) { if e.kind() == std::io::ErrorKind::PermissionDenied && let Ok(m) = fs::metadata(&json_file) && m.is_file() @@ -487,7 +487,7 @@ impl CopilotFile { } #[cfg(not(windows))] - crate::atomic_fs::write(&json_file, &content).with_context(|| { + maa_atomic_fs::write(&json_file, &content).with_context(|| { format!( "Failed to write downloaded copilot cache file to {}", json_file.display() diff --git a/crates/maa-installer/Cargo.toml b/crates/maa-installer/Cargo.toml index d887b875..3e44aca0 100644 --- a/crates/maa-installer/Cargo.toml +++ b/crates/maa-installer/Cargo.toml @@ -12,6 +12,7 @@ digest = { workspace = true, optional = true } flate2 = { workspace = true, optional = true } indicatif = { workspace = true } log = { workspace = true } +maa-atomic-fs = { workspace = true } semver = { workspace = true } tar = { workspace = true, optional = true } thiserror = { workspace = true } diff --git a/crates/maa-installer/src/download/etag.rs b/crates/maa-installer/src/download/etag.rs index ccf1d6e4..3060c043 100644 --- a/crates/maa-installer/src/download/etag.rs +++ b/crates/maa-installer/src/download/etag.rs @@ -7,12 +7,21 @@ //! In rare concurrent write scenarios, some ETag updates may be lost, which is //! acceptable as the cache will be refreshed on the next check. -use std::{fs, path::Path, time}; +use std::{fs, io, path::Path, time}; use ureq::http::StatusCode; use crate::error::{Error, ErrorKind, Result, WithDesc}; +fn set_modified(path: &Path, modified: time::SystemTime) -> io::Result<()> { + #[cfg(windows)] + let file = fs::OpenOptions::new().write(true).open(path)?; + #[cfg(not(windows))] + let file = fs::File::open(path)?; + + file.set_modified(modified) +} + pub fn download_with_etag( agent: &ureq::Agent, url: &str, @@ -46,26 +55,195 @@ pub fn download_with_etag( match response.status() { StatusCode::OK => { log::trace!("Downloaded file {}", dest.display()); - let etag = response.headers().get("ETag").and_then(|v| v.to_str().ok()); + let etag = response + .headers() + .get("ETag") + .and_then(|v| v.to_str().ok()) + .map(str::to_owned); + + maa_atomic_fs::write_with(dest, |file| { + io::copy(&mut response.into_body().as_reader(), file) + }) + .then_with_desc(|| format!("Failed to update file at {}", dest.display()))?; + if let Some(etag) = etag { log::trace!("Updated ETag {}", etag_file.display()); - fs::write(&etag_file, etag).then_with_desc(|| { + maa_atomic_fs::write(&etag_file, etag).then_with_desc(|| { format!("Failed to update ETag at {}", etag_file.display()) })?; + } else if let Err(error) = fs::remove_file(&etag_file) + && error.kind() != io::ErrorKind::NotFound + { + return Err(error) + .then_with_desc(|| format!("Failed to remove {}", etag_file.display())); } - let mut file = fs::File::create(dest)?; - std::io::copy(&mut response.into_body().as_reader(), &mut file)?; Ok(()) } StatusCode::NOT_MODIFIED => { log::trace!("File {} is up to date", dest.display()); - if let Ok(file) = fs::File::open(&etag_file) { + if set_modified(&etag_file, time::SystemTime::now()).is_ok() { log::trace!("Touched {}", dest.display()); - let _ = file.set_modified(time::SystemTime::now()); } Ok(()) } s => Err(Error::new(ErrorKind::Network).with_desc(format!("unexpected status code {s}"))), } } + +#[cfg(test)] +#[cfg_attr(coverage_nightly, coverage(off))] +mod tests { + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; + use std::{ + fs, + io::{Read, Write}, + net::TcpListener, + thread, + }; + + use tempfile::tempdir; + + use super::*; + + fn serve_once(response: &'static [u8]) -> (String, thread::JoinHandle>) { + let listener = TcpListener::bind(("127.0.0.1", 0)).unwrap(); + let address = listener.local_addr().unwrap(); + let handle = thread::spawn(move || { + let (mut stream, _) = listener.accept().unwrap(); + let mut request = Vec::new(); + loop { + let mut chunk = [0; 1024]; + let count = stream.read(&mut chunk).unwrap(); + if count == 0 { + break; + } + request.extend_from_slice(&chunk[..count]); + if request.windows(4).any(|window| window == b"\r\n\r\n") { + break; + } + } + stream.write_all(response).unwrap(); + request + }); + (format!("http://{address}/manifest"), handle) + } + + #[test] + fn interrupted_download_preserves_cached_file_and_etag() { + let temp_dir = tempdir().unwrap(); + let dest = temp_dir.path().join("manifest.json"); + let etag_file = dest.with_added_extension("etag"); + fs::write(&dest, "old manifest").unwrap(); + fs::write(&etag_file, "\"old\"").unwrap(); + let (url, server) = serve_once( + b"HTTP/1.1 200 OK\r\nContent-Length: 20\r\nETag: \"new\"\r\nConnection: close\r\n\r\npartial", + ); + + let result = download_with_etag(&ureq::Agent::new_with_defaults(), &url, &dest, None); + drop(server.join().unwrap()); + + assert!(result.is_err()); + assert_eq!(fs::read_to_string(&dest).unwrap(), "old manifest"); + assert_eq!(fs::read_to_string(&etag_file).unwrap(), "\"old\""); + } + + #[test] + fn successful_download_replaces_cached_file_and_etag() { + let temp_dir = tempdir().unwrap(); + let dest = temp_dir.path().join("manifest.json"); + let etag_file = dest.with_added_extension("etag"); + fs::write(&dest, "old manifest").unwrap(); + fs::write(&etag_file, "\"old\"").unwrap(); + let (url, server) = serve_once( + b"HTTP/1.1 200 OK\r\nContent-Length: 12\r\nETag: \"new\"\r\nConnection: close\r\n\r\nnew manifest", + ); + + download_with_etag(&ureq::Agent::new_with_defaults(), &url, &dest, None).unwrap(); + drop(server.join().unwrap()); + + assert_eq!(fs::read_to_string(&dest).unwrap(), "new manifest"); + assert_eq!(fs::read_to_string(&etag_file).unwrap(), "\"new\""); + } + + #[test] + fn etag_failure_occurs_after_file_commit() { + let temp_dir = tempdir().unwrap(); + let dest = temp_dir.path().join("manifest.json"); + let etag_file = dest.with_added_extension("etag"); + fs::write(&dest, "old manifest").unwrap(); + fs::create_dir(&etag_file).unwrap(); + let (url, server) = serve_once( + b"HTTP/1.1 200 OK\r\nContent-Length: 12\r\nETag: \"new\"\r\nConnection: close\r\n\r\nnew manifest", + ); + + let result = download_with_etag(&ureq::Agent::new_with_defaults(), &url, &dest, None); + drop(server.join().unwrap()); + + assert!(result.is_err()); + assert_eq!(fs::read_to_string(&dest).unwrap(), "new manifest"); + } + + #[test] + fn response_without_etag_removes_stale_etag() { + let temp_dir = tempdir().unwrap(); + let dest = temp_dir.path().join("manifest.json"); + let etag_file = dest.with_added_extension("etag"); + fs::write(&dest, "old manifest").unwrap(); + fs::write(&etag_file, "\"old\"").unwrap(); + let (url, server) = serve_once( + b"HTTP/1.1 200 OK\r\nContent-Length: 12\r\nConnection: close\r\n\r\nnew manifest", + ); + + download_with_etag(&ureq::Agent::new_with_defaults(), &url, &dest, None).unwrap(); + drop(server.join().unwrap()); + + assert_eq!(fs::read_to_string(&dest).unwrap(), "new manifest"); + assert!(!etag_file.exists()); + } + + #[test] + fn not_modified_uses_etag_preserves_cache_and_refreshes_timestamp() { + let temp_dir = tempdir().unwrap(); + let dest = temp_dir.path().join("manifest.json"); + let etag_file = dest.with_added_extension("etag"); + fs::write(&dest, "cached manifest").unwrap(); + fs::write(&etag_file, "\"current\"").unwrap(); + let old_modified = time::SystemTime::UNIX_EPOCH; + set_modified(&etag_file, old_modified).unwrap(); + #[cfg(unix)] + fs::set_permissions(&etag_file, fs::Permissions::from_mode(0o444)).unwrap(); + let (url, server) = serve_once(b"HTTP/1.1 304 Not Modified\r\nConnection: close\r\n\r\n"); + + download_with_etag(&ureq::Agent::new_with_defaults(), &url, &dest, None).unwrap(); + let request = String::from_utf8(server.join().unwrap()) + .unwrap() + .to_ascii_lowercase(); + + assert!(request.contains("if-none-match: \"current\"\r\n")); + assert_eq!(fs::read_to_string(&dest).unwrap(), "cached manifest"); + assert_eq!(fs::read_to_string(&etag_file).unwrap(), "\"current\""); + assert!(fs::metadata(etag_file).unwrap().modified().unwrap() > old_modified); + } + + #[test] + fn check_interval_skips_request() { + let temp_dir = tempdir().unwrap(); + let dest = temp_dir.path().join("manifest.json"); + let etag_file = dest.with_added_extension("etag"); + fs::write(&dest, "cached manifest").unwrap(); + fs::write(&etag_file, "\"current\"").unwrap(); + + download_with_etag( + &ureq::Agent::new_with_defaults(), + "not a valid URL", + &dest, + Some(time::Duration::from_secs(24 * 60 * 60)), + ) + .unwrap(); + + assert_eq!(fs::read_to_string(&dest).unwrap(), "cached manifest"); + assert_eq!(fs::read_to_string(&etag_file).unwrap(), "\"current\""); + } +} diff --git a/xtask/Cargo.toml b/xtask/Cargo.toml index d162b00a..4577ad3b 100644 --- a/xtask/Cargo.toml +++ b/xtask/Cargo.toml @@ -13,6 +13,7 @@ base16ct = { workspace = true } clap = { workspace = true, features = ["derive"] } digest-io = { workspace = true } flate2 = { workspace = true } +maa-atomic-fs = { workspace = true } maa-dirs.workspace = true maa-version = { workspace = true } semver = { workspace = true } diff --git a/xtask/src/release/package.rs b/xtask/src/release/package.rs index 55fcae0a..74bed270 100644 --- a/xtask/src/release/package.rs +++ b/xtask/src/release/package.rs @@ -128,41 +128,38 @@ fn read_or_create_manifest(file: &str) -> Result> { } fn write_manifest(file: &str, manifest: &VersionManifest
) -> Result<()> { - let content = serde_json::to_string_pretty(manifest).context("Failed to serialize manifest")?; - - fs::write(file, content).with_context(|| format!("Failed to write {file}")) + maa_atomic_fs::write_with(file, |writer| -> Result<()> { + serde_json::to_writer_pretty(writer, manifest)?; + Ok(()) + }) + .with_context(|| format!("Failed to write {file}")) } fn write_shell_format(file: &str, manifest: &VersionManifest
) -> Result<()> { // Write a shell-friendly .txt format alongside the JSON let txt_path = PathBuf::from(file).with_extension("txt"); - let txt_path_str = txt_path.to_str().unwrap(); use std::io::Write; - let mut txt_file = std::fs::File::create(&txt_path) - .with_context(|| format!("Failed to create {txt_path_str}"))?; - - writeln!(txt_file, "VERSION={}", manifest.version)?; - writeln!(txt_file, "TAG={}", manifest.details.tag)?; - writeln!(txt_file, "COMMIT={}", manifest.details.commit)?; - writeln!(txt_file)?; - - // Write assets in a shell-friendly format - for (target, asset) in &manifest.details.assets { - let target_upper = target.to_uppercase().replace('-', "_"); - writeln!(txt_file, "# {target}")?; - writeln!(txt_file, "{target_upper}_NAME={}", asset.name)?; - writeln!(txt_file, "{target_upper}_SIZE={}", asset.size)?; - writeln!(txt_file, "{target_upper}_SHA256={}", asset.sha256sum)?; + maa_atomic_fs::write_with(&txt_path, |txt_file| -> Result<()> { + writeln!(txt_file, "VERSION={}", manifest.version)?; + writeln!(txt_file, "TAG={}", manifest.details.tag)?; + writeln!(txt_file, "COMMIT={}", manifest.details.commit)?; writeln!(txt_file)?; - } - txt_file - .sync_all() - .with_context(|| format!("Failed to sync {txt_path_str}"))?; + // Write assets in a shell-friendly format + for (target, asset) in &manifest.details.assets { + let target_upper = target.to_uppercase().replace('-', "_"); + writeln!(txt_file, "# {target}")?; + writeln!(txt_file, "{target_upper}_NAME={}", asset.name)?; + writeln!(txt_file, "{target_upper}_SIZE={}", asset.size)?; + writeln!(txt_file, "{target_upper}_SHA256={}", asset.sha256sum)?; + writeln!(txt_file)?; + } - Ok(()) + Ok(()) + }) + .with_context(|| format!("Failed to write {}", txt_path.display())) } fn create_archive(target: &str, version: &str, dir: &str) -> Result<(String, String)> { @@ -191,3 +188,50 @@ fn create_archive(target: &str, version: &str, dir: &str) -> Result<(String, Str Ok((archive_name, checksum_hash)) } + +#[cfg(test)] +mod tests { + use tempfile::tempdir; + + use super::*; + + #[test] + fn release_metadata_replaces_existing_files() { + let temp_dir = tempdir().unwrap(); + let json_path = temp_dir.path().join("stable.json"); + let txt_path = temp_dir.path().join("stable.txt"); + fs::write(&json_path, "stale json").unwrap(); + fs::write(&txt_path, "stale text").unwrap(); + + let manifest = VersionManifest { + version: Version::new(1, 2, 3), + details: Details { + tag: "v1.2.3".to_string(), + commit: "0123456789abcdef".to_string(), + assets: BTreeMap::from([("aarch64-apple-darwin".to_string(), Asset { + name: "maa_cli-v1.2.3-aarch64-apple-darwin.tar.gz".to_string(), + size: 42, + sha256sum: "deadbeef".to_string(), + })]), + }, + }; + + let json_path = json_path.to_string_lossy(); + write_manifest(&json_path, &manifest).unwrap(); + write_shell_format(&json_path, &manifest).unwrap(); + + let persisted: VersionManifest
= + serde_json::from_reader(fs::File::open(&*json_path).unwrap()).unwrap(); + assert_eq!(persisted.version, Version::new(1, 2, 3)); + assert_eq!(persisted.details.tag, "v1.2.3"); + assert_eq!(persisted.details.commit, "0123456789abcdef"); + + let shell = fs::read_to_string(txt_path).unwrap(); + assert!(shell.starts_with("VERSION=1.2.3\nTAG=v1.2.3\nCOMMIT=0123456789abcdef\n\n")); + assert!( + shell + .contains("AARCH64_APPLE_DARWIN_NAME=maa_cli-v1.2.3-aarch64-apple-darwin.tar.gz\n") + ); + assert!(!shell.contains("stale text")); + } +} From 01234feada8f8a377816a83918d9c1f6f0987209 Mon Sep 17 00:00:00 2001 From: Loong Date: Mon, 27 Jul 2026 18:07:21 +0100 Subject: [PATCH 2/4] fix: enable nightly coverage for atomic fs --- crates/maa-atomic-fs/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/maa-atomic-fs/src/lib.rs b/crates/maa-atomic-fs/src/lib.rs index 9dd20af8..522e2704 100644 --- a/crates/maa-atomic-fs/src/lib.rs +++ b/crates/maa-atomic-fs/src/lib.rs @@ -1,3 +1,5 @@ +#![cfg_attr(coverage_nightly, feature(coverage_attribute))] + //! Utilities for atomically replacing individual files. //! //! Writes are staged in a temporary file beside the destination, synchronized, From 97c7b2f850e78d8b10a78d82bba76f45304812f9 Mon Sep 17 00:00:00 2001 From: Loong Date: Wed, 19 Aug 2026 14:11:14 +0100 Subject: [PATCH 3/4] fix: correct ETag touch log path --- crates/maa-installer/src/download/etag.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/maa-installer/src/download/etag.rs b/crates/maa-installer/src/download/etag.rs index 3060c043..b842f66e 100644 --- a/crates/maa-installer/src/download/etag.rs +++ b/crates/maa-installer/src/download/etag.rs @@ -83,7 +83,7 @@ pub fn download_with_etag( StatusCode::NOT_MODIFIED => { log::trace!("File {} is up to date", dest.display()); if set_modified(&etag_file, time::SystemTime::now()).is_ok() { - log::trace!("Touched {}", dest.display()); + log::trace!("Touched {}", etag_file.display()); } Ok(()) } From 4c90d76299c8722e5939958e0362c728720aea7f Mon Sep 17 00:00:00 2001 From: Loong Date: Sun, 13 Sep 2026 15:21:47 +0100 Subject: [PATCH 4/4] docs: clarify concurrent ETag cache behavior --- crates/maa-installer/src/download/etag.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/maa-installer/src/download/etag.rs b/crates/maa-installer/src/download/etag.rs index b842f66e..b2c2bf6a 100644 --- a/crates/maa-installer/src/download/etag.rs +++ b/crates/maa-installer/src/download/etag.rs @@ -3,9 +3,10 @@ //! This module provides caching functionality to avoid re-downloading manifests //! when they haven't changed, using HTTP ETag headers. //! -//! Note: The cache does not use file locking for simplicity and performance. -//! In rare concurrent write scenarios, some ETag updates may be lost, which is -//! acceptable as the cache will be refreshed on the next check. +//! Note: The cache does not use file locking or multi-file transactions. Rare +//! concurrent successful downloads may therefore leave a complete body paired +//! with an ETag from another response. This is an accepted cache consistency +//! tradeoff; each individual file replacement remains atomic. use std::{fs, io, path::Path, time};