diff --git a/rust_backend/Cargo.lock b/rust_backend/Cargo.lock index 699d0e0e..2e7ce231 100644 --- a/rust_backend/Cargo.lock +++ b/rust_backend/Cargo.lock @@ -2479,6 +2479,7 @@ dependencies = [ "rustix", "serde", "serde_json", + "tempfile", "tz-rs", "unicode-general-category", "unicode-normalization", diff --git a/rust_backend/crates/core/Cargo.toml b/rust_backend/crates/core/Cargo.toml index 4968195a..bd6c080e 100644 --- a/rust_backend/crates/core/Cargo.toml +++ b/rust_backend/crates/core/Cargo.toml @@ -18,6 +18,7 @@ chrono.workspace = true rustix.workspace = true claxon.workspace = true roxmltree.workspace = true +tempfile.workspace = true [target.'cfg(not(any(target_os = "android", target_os = "ios")))'.dependencies] tz-rs.workspace = true diff --git a/rust_backend/crates/core/src/fs.rs b/rust_backend/crates/core/src/fs.rs index 05d9525f..44ca1deb 100644 --- a/rust_backend/crates/core/src/fs.rs +++ b/rust_backend/crates/core/src/fs.rs @@ -1,7 +1,82 @@ -//! Filesystem path helpers shared by the native and extension backends. +//! Filesystem helpers shared by the native and extension backends. +use std::fmt; +use std::fs::File; +use std::io; use std::path::{Component, Path, PathBuf}; +/// The prefix `tempfile::NamedTempFile::new_in` uses, for writers that kept it. +pub const DEFAULT_TEMP_PREFIX: &str = ".tmp"; + +/// A [`write_atomic`] failure. `Persist` keeps tempfile's own message +/// (`failed to persist temporary file: ...`); converting to [`io::Error`] +/// yields the underlying rename error instead, like `PersistError::error`. +#[derive(Debug)] +pub enum WriteAtomicError { + Io(io::Error), + Persist(tempfile::PersistError), +} + +impl fmt::Display for WriteAtomicError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Io(error) => error.fmt(formatter), + Self::Persist(error) => error.fmt(formatter), + } + } +} + +impl std::error::Error for WriteAtomicError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::Io(error) => error.source(), + Self::Persist(error) => error.source(), + } + } +} + +impl From for WriteAtomicError { + fn from(error: io::Error) -> Self { + Self::Io(error) + } +} + +impl From for io::Error { + fn from(error: WriteAtomicError) -> Self { + match error { + WriteAtomicError::Io(error) => error, + WriteAtomicError::Persist(error) => error.error, + } + } +} + +/// Replaces `path` atomically. In order: creates a `{prefix}XXXXXX` temporary +/// file in `directory`, passes it to `write`, `sync_all`s it, runs `check`, +/// then renames it over `path` (`NamedTempFile::persist`, which replaces an +/// existing file). With `sync_directory`, `directory` is then fsynced on a +/// best-effort basis. Any earlier failure removes the temporary file. +pub fn write_atomic( + directory: &Path, + path: &Path, + prefix: &str, + write: impl FnOnce(&mut File) -> io::Result<()>, + check: impl FnOnce() -> io::Result<()>, + sync_directory: bool, +) -> Result<(), WriteAtomicError> { + let mut file = tempfile::Builder::new() + .prefix(prefix) + .tempfile_in(directory)?; + write(file.as_file_mut())?; + file.as_file().sync_all()?; + check()?; + file.persist(path).map_err(WriteAtomicError::Persist)?; + // Directory fsync is unavailable on some filesystems; rename remains atomic. + if sync_directory && let Ok(directory) = File::open(directory) { + let _ = directory.sync_all(); + } + Ok(()) +} + /// Lexically cleans a path like Go's `filepath.Clean`: drops `.` components, /// folds `name/..`, keeps leading `..` on relative paths, drops `..` above the /// root, and returns `.` for an empty result. The filesystem is not consulted. @@ -82,4 +157,107 @@ mod tests { assert_eq!(clean(input), previous_isrc_clean(input), "{input:?}"); } } + + fn names(directory: &Path) -> Vec { + let mut names: Vec<_> = std::fs::read_dir(directory) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + names + } + + #[test] + fn default_temp_prefix_matches_named_temp_file() { + let directory = tempfile::tempdir().unwrap(); + let file = tempfile::NamedTempFile::new_in(directory.path()).unwrap(); + let name = file.path().file_name().unwrap().to_string_lossy(); + assert!(name.starts_with(DEFAULT_TEMP_PREFIX), "{name}"); + assert_eq!(name.len(), DEFAULT_TEMP_PREFIX.len() + 6); + } + + #[test] + fn write_atomic_stages_with_prefix_syncs_checks_then_replaces() { + use std::io::Write; + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("value.json"); + std::fs::write(&path, b"old").unwrap(); + let order = std::cell::RefCell::new(Vec::new()); + + write_atomic( + directory.path(), + &path, + ".spotiflac-", + |file| { + let staged = names(directory.path()); + assert_eq!(staged.len(), 2, "{staged:?}"); + assert!(staged[0].starts_with(".spotiflac-"), "{staged:?}"); + order.borrow_mut().push("write"); + file.write_all(b"new") + }, + || { + assert_eq!(std::fs::read(&path).unwrap(), b"old"); + order.borrow_mut().push("check"); + Ok(()) + }, + true, + ) + .unwrap(); + assert_eq!(*order.borrow(), ["write", "check"]); + assert_eq!(std::fs::read(&path).unwrap(), b"new"); + assert_eq!(names(directory.path()), ["value.json"]); + + for failing_check in [false, true] { + let error = write_atomic( + directory.path(), + &path, + DEFAULT_TEMP_PREFIX, + |file| { + file.write_all(b"partial")?; + if failing_check { + Ok(()) + } else { + Err(io::Error::other("write failed")) + } + }, + || Err(io::Error::other("cancelled")), + false, + ) + .unwrap_err(); + let expected = if failing_check { + "cancelled" + } else { + "write failed" + }; + assert_eq!(error.to_string(), expected); + assert_eq!(std::fs::read(&path).unwrap(), b"new"); + assert_eq!(names(directory.path()), ["value.json"]); + } + } + + #[test] + fn write_atomic_persist_errors_keep_tempfile_message_and_io_cause() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("occupied"); + std::fs::create_dir(&path).unwrap(); + std::fs::write(path.join("child"), b"").unwrap(); + let error = write_atomic( + directory.path(), + &path, + ".tmp", + |_| Ok(()), + || Ok(()), + false, + ) + .unwrap_err(); + let WriteAtomicError::Persist(persist) = &error else { + panic!("{error:?}"); + }; + let message = persist.to_string(); + assert_eq!(error.to_string(), message); + assert!(message.starts_with("failed to persist temporary file: ")); + let cause = persist.error.to_string(); + assert_eq!(io::Error::from(error).to_string(), cause); + assert_eq!(names(directory.path()), ["occupied"]); + } } diff --git a/rust_backend/crates/extensions/src/repository.rs b/rust_backend/crates/extensions/src/repository.rs index 1094b6c9..c6076cc4 100644 --- a/rust_backend/crates/extensions/src/repository.rs +++ b/rust_backend/crates/extensions/src/repository.rs @@ -9,6 +9,7 @@ pub use package::{destination_path, write_verified_package}; pub use urls::{RegistryLocation, registry_location, require_https}; use serde::{Deserialize, Serialize}; +use spotiflac_core::fs::{DEFAULT_TEMP_PREFIX, write_atomic}; use spotiflac_network::{HttpRequest, NetworkService, NetworkSession}; use std::collections::BTreeMap; use std::fs::{self, File, OpenOptions}; @@ -273,10 +274,14 @@ impl ExtensionRepository { if bytes.len() as u64 > MAX_CACHE_BYTES { return Ok(()); } - let mut file = tempfile::NamedTempFile::new_in(&self.cache_directory)?; - file.write_all(&bytes)?; - file.as_file().sync_all()?; - file.persist(self.cache_directory.join(CACHE_FILE))?; + write_atomic( + &self.cache_directory, + &self.cache_directory.join(CACHE_FILE), + DEFAULT_TEMP_PREFIX, + |file| file.write_all(&bytes), + || Ok(()), + false, + )?; Ok(()) })(); } diff --git a/rust_backend/crates/extensions/src/storage.rs b/rust_backend/crates/extensions/src/storage.rs index 4bfe48a5..d7ae119f 100644 --- a/rust_backend/crates/extensions/src/storage.rs +++ b/rust_backend/crates/extensions/src/storage.rs @@ -9,7 +9,7 @@ use hmac::{Hmac, Mac}; use serde_json::{Map, Value}; use sha2::{Digest, Sha256}; use std::collections::HashMap; -use std::fs::{self, File}; +use std::fs; use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex, OnceLock, RwLock, RwLockReadGuard, Weak}; @@ -433,16 +433,15 @@ fn parse_object(data: &[u8]) -> Result, StorageError> { pub(crate) fn atomic_write(path: &Path, data: &[u8]) -> Result<(), StorageError> { file_identity(path)?; let directory = path.parent().ok_or(StorageError::InvalidPath)?; - let mut file = tempfile::Builder::new() - .prefix(".spotiflac-") - .tempfile_in(directory)?; - file.write_all(data)?; - file.as_file().sync_all()?; - file.persist(path).map_err(|error| error.error)?; - // Directory fsync is unavailable on some filesystems; rename remains atomic. - if let Ok(directory) = File::open(directory) { - let _ = directory.sync_all(); - } + spotiflac_core::fs::write_atomic( + directory, + path, + ".spotiflac-", + |file| file.write_all(data), + || Ok(()), + true, + ) + .map_err(io::Error::from)?; Ok(()) } diff --git a/rust_backend/crates/mobile/src/native_playlists.rs b/rust_backend/crates/mobile/src/native_playlists.rs index 4558d519..5ad790de 100644 --- a/rust_backend/crates/mobile/src/native_playlists.rs +++ b/rust_backend/crates/mobile/src/native_playlists.rs @@ -2,9 +2,10 @@ //! CSV remains line-based for compatibility with existing exports/imports. use crate::native_library::path_keys::dart_trim as trim; use serde_json::{Value, json}; +use spotiflac_core::fs::write_atomic; use std::{ collections::HashMap, - io::{BufWriter, Write}, + io::{self, BufWriter, Write}, path::Path, }; @@ -39,32 +40,34 @@ pub(crate) fn execute( .parent() .filter(|parent| !parent.as_os_str().is_empty()) .unwrap_or(Path::new(".")); - let mut staging = tempfile::Builder::new() - .prefix(".spotiflac-m3u-") - .tempfile_in(parent) - .map_err(|e| e.to_string())?; - { - let mut file = BufWriter::with_capacity(64 * 1024, staging.as_file_mut()); - file.write_all(b"#EXTM3U\n").map_err(|e| e.to_string())?; - for entry in entries { - check()?; - let duration = entry["duration"].as_i64().filter(|n| *n > 0).unwrap_or(-1); - let artist = trim(entry["artist"].as_str().unwrap_or_default()); - let title = entry["name"].as_str().unwrap_or_default(); - let display = if artist.is_empty() { - title.to_owned() - } else { - format!("{artist} - {title}") - }; - let source = entry["path"].as_str().ok_or("Missing M3U entry path")?; - writeln!(file, "#EXTINF:{duration},{display}\n{source}") - .map_err(|e| e.to_string())?; - } - file.flush().map_err(|e| e.to_string())?; - } - staging.as_file().sync_all().map_err(|e| e.to_string())?; - check()?; - staging.persist(path).map_err(|e| e.to_string())?; + write_atomic( + parent, + Path::new(path), + ".spotiflac-m3u-", + |staging| { + let mut file = BufWriter::with_capacity(64 * 1024, staging); + file.write_all(b"#EXTM3U\n")?; + for entry in entries { + check().map_err(io::Error::other)?; + let duration = entry["duration"].as_i64().filter(|n| *n > 0).unwrap_or(-1); + let artist = trim(entry["artist"].as_str().unwrap_or_default()); + let title = entry["name"].as_str().unwrap_or_default(); + let display = if artist.is_empty() { + title.to_owned() + } else { + format!("{artist} - {title}") + }; + let source = entry["path"] + .as_str() + .ok_or_else(|| io::Error::other("Missing M3U entry path"))?; + writeln!(file, "#EXTINF:{duration},{display}\n{source}")?; + } + file.flush() + }, + || check().map_err(io::Error::other), + false, + ) + .map_err(|e| e.to_string())?; Ok(json!({"output_path": path, "committed": true})) } _ => Err("Unknown playlist operation".into()), diff --git a/rust_backend/crates/mobile/src/network_tags.rs b/rust_backend/crates/mobile/src/network_tags.rs index a2697a04..c5e5140c 100644 --- a/rust_backend/crates/mobile/src/network_tags.rs +++ b/rust_backend/crates/mobile/src/network_tags.rs @@ -3,6 +3,7 @@ use base64::Engine; use serde_json::{Value, json}; use sha2::{Digest, Sha256}; +use spotiflac_core::fs::{DEFAULT_TEMP_PREFIX, write_atomic}; use spotiflac_core::{lower_hex, tags}; use spotiflac_network::{ HttpRequest, NetworkService, NetworkSession, policy::NetworkPermissions, url::UrlParts, @@ -189,15 +190,15 @@ fn cache_cover( fs::create_dir_all(directory).map_err(|error| error.to_string())?; let digest = lower_hex(&Sha256::digest(bytes)); let target = directory.join(format!("network_cover_{digest}.image")); - let mut staged = - tempfile::NamedTempFile::new_in(directory).map_err(|error| error.to_string())?; - staged.write_all(bytes).map_err(|error| error.to_string())?; - staged - .as_file() - .sync_all() - .map_err(|error| error.to_string())?; - check()?; - staged.persist(&target).map_err(|error| error.to_string())?; + write_atomic( + directory, + &target, + DEFAULT_TEMP_PREFIX, + |file| file.write_all(bytes), + || check().map_err(io::Error::other), + false, + ) + .map_err(|error| error.to_string())?; let mut files = fs::read_dir(directory) .map_err(|error| error.to_string())? .filter_map(Result::ok) @@ -279,25 +280,25 @@ pub(crate) fn promote_cover( { return Err("Artwork input must be a regular file".into()); } - let mut staged = - tempfile::NamedTempFile::new_in(destination).map_err(|error| error.to_string())?; - let mut buffer = [0; 64 * 1024]; - loop { - check()?; - let count = input.read(&mut buffer).map_err(|error| error.to_string())?; - if count == 0 { - break; - } - staged - .write_all(&buffer[..count]) - .map_err(|error| error.to_string())?; - } - staged - .as_file() - .sync_all() - .map_err(|error| error.to_string())?; - check()?; - staged.persist(&target).map_err(|error| error.to_string())?; + write_atomic( + destination, + &target, + DEFAULT_TEMP_PREFIX, + |file| { + let mut buffer = [0; 64 * 1024]; + loop { + check().map_err(io::Error::other)?; + let count = input.read(&mut buffer)?; + if count == 0 { + return Ok(()); + } + file.write_all(&buffer[..count])?; + } + }, + || check().map_err(io::Error::other), + false, + ) + .map_err(|error| error.to_string())?; } Ok(json!({"cover_path": target.to_string_lossy(), "sha256": digest})) } diff --git a/rust_backend/crates/providers/src/lyrics/cache.rs b/rust_backend/crates/providers/src/lyrics/cache.rs index e529f3b0..09fc74b7 100644 --- a/rust_backend/crates/providers/src/lyrics/cache.rs +++ b/rust_backend/crates/providers/src/lyrics/cache.rs @@ -1,6 +1,7 @@ //! Go-compatible successful-lyrics cache with bounded, atomic persistence. use serde::{Deserialize, Serialize}; +use spotiflac_core::fs::{DEFAULT_TEMP_PREFIX, write_atomic}; use spotiflac_core::lyrics::LyricsResponse; use std::collections::BTreeMap; use std::fs::{self, OpenOptions}; @@ -298,10 +299,14 @@ fn write_snapshot(path: &Path, entries: &BTreeMap) -> std::io::Re "lyrics cache exceeds persistence size limit", )); } - let mut temporary = tempfile::NamedTempFile::new_in(parent)?; - temporary.write_all(&bytes)?; - temporary.as_file().sync_all()?; - temporary.persist(path).map_err(|error| error.error)?; + write_atomic( + parent, + path, + DEFAULT_TEMP_PREFIX, + |file| file.write_all(&bytes), + || Ok(()), + false, + )?; Ok(()) }