refactor(core): add fs::write_atomic and use it for the atomic file writes

Six writers repeated the same publication sequence: create a temporary file
in the target directory, write it, sync_all, optionally run a cancellation
check, then NamedTempFile::persist over the target. `core::fs::write_atomic`
runs exactly that order and optionally fsyncs the directory afterwards
(best effort, as extension storage did).

Each site keeps its temporary-file prefix (`.spotiflac-` for extension
storage, `.spotiflac-m3u-` for M3U export, tempfile's `.tmp` default,
exposed as `DEFAULT_TEMP_PREFIX`, for the registry cache, lyrics cache and
both network cover writers), its check placement between fsync and rename,
and its error text: `WriteAtomicError` displays a persist failure with
tempfile's own message, and converts to the underlying io::Error where the
site previously used `PersistError::error`.

Not converted, because their publication differs: playlist staging and
collection snapshots use persist_noclobber (collections also skips fsync),
the backup restore stages into a temporary directory, and the extension
package download maps each step to its own error context.

spotiflac-core now depends on the workspace `tempfile` already used by the
other crates; the lockfile only gains that edge.
This commit is contained in:
zarzet committed 2026-10-19 00:36:29 +07:00
1 parent f7a15cbd43
commit 190801c398
8 files changed
+268 -75

No files matched your search

+1
View File
@@ -2479,6 +2479,7 @@ dependencies = [
"rustix",
"serde",
"serde_json",
"tempfile",
"tz-rs",
"unicode-general-category",
"unicode-normalization",
+1
View File
@@ -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
+179 -1
View File
@@ -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<io::Error> for WriteAtomicError {
fn from(error: io::Error) -> Self {
Self::Io(error)
}
}
impl From<WriteAtomicError> 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<String> {
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"]);
}
}
@@ -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(())
})();
}
+10 -11
View File
@@ -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<Map<String, Value>, 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(())
}
@@ -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()),
+29 -28
View File
@@ -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}))
}
@@ -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<String, Entry>) -> 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(())
}