From c18bbb82fa92844486d4c0605899f7903e49292e Mon Sep 17 00:00:00 2001 From: Matias Palma Date: Mon, 20 Apr 2026 22:50:25 -0400 Subject: [PATCH 1/2] fix: write write_file and disabled_extensions.json atomically to survive mid-write crashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit write_file in lib.rs and the disabled_extensions.json write in extensions.rs both called fs::write directly. A crash or power loss midway through that write left the target truncated, which corrupted whatever the user had been saving (including the extensions registry itself — a crash during an enable/disable left the JSON in an invalid state and broke extension loading on next launch). Added a new `fs_atomic::write_atomic` helper that: - Writes the payload to a hidden sibling temp file (`....tmp`) so the eventual rename stays on the same volume (the atomicity guarantee is per-volume). - Calls `sync_data` before renaming, so the data has actually reached disk rather than merely the OS page cache. - Renames over the destination. On POSIX `rename` is atomic at the filesystem level; on Windows `std::fs::rename` maps to `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH`, which is atomic on NTFS for same-volume targets. - Cleans up the temp file on any error so we don't leak `.tmp` files if the rename itself fails (read-only target, cross-volume path, etc.). Both call sites now go through the helper. Added three unit tests covering new-file write, existing-file replacement, and the no-leftover `.tmp` invariant. Closes the non-atomic-write path without touching the disk format or any public command surface. --- apps/desktop/src-tauri/src/extensions.rs | 2 +- apps/desktop/src-tauri/src/fs_atomic.rs | 128 +++++++++++++++++++++++ apps/desktop/src-tauri/src/lib.rs | 3 +- 3 files changed, 131 insertions(+), 2 deletions(-) create mode 100644 apps/desktop/src-tauri/src/fs_atomic.rs diff --git a/apps/desktop/src-tauri/src/extensions.rs b/apps/desktop/src-tauri/src/extensions.rs index e8236cdb..7b5e9e3e 100644 --- a/apps/desktop/src-tauri/src/extensions.rs +++ b/apps/desktop/src-tauri/src/extensions.rs @@ -527,7 +527,7 @@ pub async fn toggle_extension_state( } let json = serde_json::to_string_pretty(&disabled).map_err(|e| e.to_string())?; - std::fs::write(disabled_file, json).map_err(|e| e.to_string())?; + crate::fs_atomic::write_atomic(&disabled_file, json.as_bytes()).map_err(|e| e.to_string())?; Ok(()) } diff --git a/apps/desktop/src-tauri/src/fs_atomic.rs b/apps/desktop/src-tauri/src/fs_atomic.rs new file mode 100644 index 00000000..6be16fd8 --- /dev/null +++ b/apps/desktop/src-tauri/src/fs_atomic.rs @@ -0,0 +1,128 @@ +use std::io::Write; +use std::path::Path; + +/// Write `bytes` to `path` atomically: stream to a sibling temp file, fsync +/// it, then rename over the destination. +/// +/// On POSIX `rename` is atomic at the filesystem level. On Windows +/// `std::fs::rename` maps to `MoveFileEx` with +/// `MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH`, which performs the +/// replacement atomically on NTFS for same-volume targets. Either way, a +/// crash or power loss mid-write leaves the **original** file on disk +/// rather than a half-written truncation. +/// +/// The temp file lives alongside the target so the rename stays on the +/// same volume (the atomicity guarantee is per-volume). A PID+timestamp +/// suffix keeps the temp name unique without pulling in a full tempfile +/// dependency, and the leading `.` keeps editors/file-watchers from +/// surfacing it to the user. +pub fn write_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { + let parent = path.parent().ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "atomic write target has no parent directory", + ) + })?; + + if !parent.as_os_str().is_empty() && !parent.exists() { + std::fs::create_dir_all(parent)?; + } + + let file_name = path + .file_name() + .ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "atomic write target has no file name", + ) + })? + .to_string_lossy() + .into_owned(); + + let pid = std::process::id(); + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + let tmp_path = parent.join(format!(".{}.{}.{}.tmp", file_name, pid, nanos)); + + // Write + fsync before rename so the data is actually on disk, not + // merely in the OS page cache, before we commit. + { + let mut tmp = std::fs::File::create(&tmp_path)?; + if let Err(e) = tmp.write_all(bytes).and_then(|_| tmp.sync_data()) { + let _ = std::fs::remove_file(&tmp_path); + return Err(e); + } + } + + match std::fs::rename(&tmp_path, path) { + Ok(()) => Ok(()), + Err(e) => { + // Best-effort cleanup so we don't leak `.tmp` files when the + // rename itself fails (read-only target, cross-volume, etc.). + let _ = std::fs::remove_file(&tmp_path); + Err(e) + } + } +} + +#[cfg(test)] +mod tests { + use super::write_atomic; + use std::fs; + + fn tmp_dir() -> std::path::PathBuf { + let base = std::env::temp_dir().join(format!( + "trixty-fs-atomic-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0) + )); + fs::create_dir_all(&base).unwrap(); + base + } + + #[test] + fn writes_new_file() { + let dir = tmp_dir(); + let target = dir.join("new.txt"); + write_atomic(&target, b"hello").unwrap(); + assert_eq!(fs::read_to_string(&target).unwrap(), "hello"); + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn replaces_existing_file_contents() { + let dir = tmp_dir(); + let target = dir.join("replace.txt"); + fs::write(&target, b"old payload").unwrap(); + write_atomic(&target, b"new payload").unwrap(); + assert_eq!(fs::read_to_string(&target).unwrap(), "new payload"); + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn leaves_no_temp_files_behind_after_success() { + let dir = tmp_dir(); + let target = dir.join("clean.txt"); + write_atomic(&target, b"payload").unwrap(); + let leftovers: Vec<_> = fs::read_dir(&dir) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| { + e.file_name() + .to_string_lossy() + .ends_with(".tmp") + }) + .collect(); + assert!( + leftovers.is_empty(), + "expected no .tmp leftovers, found: {:?}", + leftovers.iter().map(|e| e.file_name()).collect::>() + ); + fs::remove_dir_all(&dir).ok(); + } +} diff --git a/apps/desktop/src-tauri/src/lib.rs b/apps/desktop/src-tauri/src/lib.rs index f84992fe..db7d7f57 100644 --- a/apps/desktop/src-tauri/src/lib.rs +++ b/apps/desktop/src-tauri/src/lib.rs @@ -1,5 +1,6 @@ mod about; mod error; +mod fs_atomic; mod http; mod pty; @@ -200,7 +201,7 @@ fn read_file(path: String) -> Result { #[tauri::command] fn write_file(path: String, content: String) -> Result<(), String> { - fs::write(&path, content).map_err(|e| { + fs_atomic::write_atomic(std::path::Path::new(&path), content.as_bytes()).map_err(|e| { let err = format!("Failed to write file {}: {}", path, e); error!("{}", err); redact_user_paths(&err) From b191da3c2040ad2c93f15d62f112901342184bbb Mon Sep 17 00:00:00 2001 From: Matias Palma Date: Tue, 21 Apr 2026 00:09:26 -0400 Subject: [PATCH 2/2] fix: unique temp via create_new+retry, preserve permissions, thread-safe test dirs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses all three review threads on #170. - Temp uniqueness. The previous PID+nanos suffix relied on SystemTime resolution being finer than a save cycle, which isn't always true (15.6 ms on older Windows) and `.unwrap_or(0)` collapsed clock failures to the same value. A `File::create` on a duplicate suffix would also truncate an existing temp that another caller owned. Introduced `create_unique_temp` that opens the temp with `OpenOptions::create_new(true)` (fails fast with AlreadyExists), adds a process-wide `AtomicU64` counter alongside PID + nanos so two racing calls inside the same tick never clash, and retries up to 32 times before surfacing an error. - Permission preservation. When the destination file already exists, carry its mode bits to the temp via `set_permissions` before the rename. On Unix this preserves the executable / group / other bits that the review called out (previously the rename landed a default 0644/0666-style mask even if the original file was 0755). On Windows `Permissions` only exposes the read-only bit and ACLs inherit from the parent directory, so the carry-over is a no-op in practice — documented that in the doc-comment so a future editor doesn't try to plug a hole that doesn't exist. - Test-parallelism flake. `tmp_dir()` in the test module now incorporates the current thread id plus a process-wide atomic counter in addition to PID and nanos, so two Rust-test threads racing into the helper at the same clock tick always pick disjoint directories. - Added a Unix-only regression test (`preserves_unix_mode_bits_on_replace`) that chmod 0o755s the destination, overwrites, and asserts the mode survives. - Mentioned as polish: the `io_other_error` clippy lint pushed us to use `std::io::Error::other` instead of `Error::new(Other, …)` in the retry-exhausted branch. --- apps/desktop/src-tauri/src/fs_atomic.rs | 141 ++++++++++++++++++++---- 1 file changed, 118 insertions(+), 23 deletions(-) diff --git a/apps/desktop/src-tauri/src/fs_atomic.rs b/apps/desktop/src-tauri/src/fs_atomic.rs index 6be16fd8..6c2958a2 100644 --- a/apps/desktop/src-tauri/src/fs_atomic.rs +++ b/apps/desktop/src-tauri/src/fs_atomic.rs @@ -1,5 +1,7 @@ +use std::fs::OpenOptions; use std::io::Write; use std::path::Path; +use std::sync::atomic::{AtomicU64, Ordering}; /// Write `bytes` to `path` atomically: stream to a sibling temp file, fsync /// it, then rename over the destination. @@ -12,10 +14,19 @@ use std::path::Path; /// rather than a half-written truncation. /// /// The temp file lives alongside the target so the rename stays on the -/// same volume (the atomicity guarantee is per-volume). A PID+timestamp -/// suffix keeps the temp name unique without pulling in a full tempfile -/// dependency, and the leading `.` keeps editors/file-watchers from -/// surfacing it to the user. +/// same volume (the atomicity guarantee is per-volume). The temp name +/// combines a process-wide atomic counter with PID and a nanosecond +/// timestamp, and is opened with `create_new(true)` so two concurrent +/// callers racing on the same target can never share the same file +/// (the loser retries with a new suffix rather than sharing-and-truncating +/// an existing temp). +/// +/// When the destination already exists, its mode bits (on Unix) are copied +/// onto the temp file before the rename so an executable being atomically +/// replaced doesn't silently lose its `+x` bit. On Windows, ACLs inherited +/// from the destination's parent directory apply to the new file; that's +/// the same inheritance git/editors rely on for new files, so we don't +/// attempt manual ACL copy. pub fn write_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { let parent = path.parent().ok_or_else(|| { std::io::Error::new( @@ -39,21 +50,28 @@ pub fn write_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { .to_string_lossy() .into_owned(); - let pid = std::process::id(); - let nanos = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|d| d.as_nanos()) - .unwrap_or(0); - let tmp_path = parent.join(format!(".{}.{}.{}.tmp", file_name, pid, nanos)); + // Open a fresh temp file with create_new(true). On collision (another + // process / another thread inside the same process already minted the + // same suffix) the call errors with AlreadyExists and we loop with a + // new suffix rather than truncating a temp that someone else owns. + let (tmp_path, mut tmp) = create_unique_temp(parent, &file_name)?; // Write + fsync before rename so the data is actually on disk, not // merely in the OS page cache, before we commit. - { - let mut tmp = std::fs::File::create(&tmp_path)?; - if let Err(e) = tmp.write_all(bytes).and_then(|_| tmp.sync_data()) { - let _ = std::fs::remove_file(&tmp_path); - return Err(e); - } + if let Err(e) = tmp.write_all(bytes).and_then(|_| tmp.sync_data()) { + drop(tmp); + let _ = std::fs::remove_file(&tmp_path); + return Err(e); + } + drop(tmp); + + // If the destination exists, carry over its permission bits to the + // temp file so the rename doesn't silently change mode. On Windows + // `set_permissions` only flips the read-only bit and ACLs are + // inherited from the parent directory, so this is effectively a no-op + // there; on Unix it preserves chmod state including +x. + if let Ok(dest_meta) = std::fs::metadata(path) { + let _ = std::fs::set_permissions(&tmp_path, dest_meta.permissions()); } match std::fs::rename(&tmp_path, path) { @@ -67,19 +85,73 @@ pub fn write_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { } } +/// Generates unique temp paths even under SystemTime coarseness. Combines +/// PID, a nanosecond timestamp, and a process-wide monotonically +/// increasing counter so two callers that stat `SystemTime` inside the +/// same tick still get distinct suffixes. Retries with a fresh counter +/// value on `AlreadyExists` so a concurrent foreign file never wins the +/// race. +fn create_unique_temp( + parent: &Path, + file_name: &str, +) -> std::io::Result<(std::path::PathBuf, std::fs::File)> { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let pid = std::process::id(); + + // Bound the retries so a permanently unwriteable directory surfaces as + // an error instead of spinning forever. + for _ in 0..32 { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + let seq = COUNTER.fetch_add(1, Ordering::Relaxed); + let tmp_path = parent.join(format!(".{}.{}.{}.{}.tmp", file_name, pid, nanos, seq)); + + match OpenOptions::new() + .write(true) + .create_new(true) + .open(&tmp_path) + { + Ok(file) => return Ok((tmp_path, file)), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + } + } + + Err(std::io::Error::other( + "could not allocate a unique temp file after 32 attempts", + )) +} + #[cfg(test)] mod tests { use super::write_atomic; use std::fs; + use std::sync::atomic::{AtomicU64, Ordering}; fn tmp_dir() -> std::path::PathBuf { + // Tests run in parallel by default. `SystemTime::now()` resolution + // on some platforms (notably older Windows) is 15.6 ms, which is + // coarse enough that two threads racing into this function share a + // nanos value. Add a process-wide atomic counter plus the current + // thread id so distinct test threads always get distinct paths. + static COUNTER: AtomicU64 = AtomicU64::new(0); + let seq = COUNTER.fetch_add(1, Ordering::Relaxed); + let tid = format!("{:?}", std::thread::current().id()); + let tid_safe: String = tid + .chars() + .map(|c| if c.is_ascii_alphanumeric() { c } else { '_' }) + .collect(); let base = std::env::temp_dir().join(format!( - "trixty-fs-atomic-{}-{}", + "trixty-fs-atomic-{}-{}-{}-{}", std::process::id(), + tid_safe, std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_nanos()) - .unwrap_or(0) + .unwrap_or(0), + seq, )); fs::create_dir_all(&base).unwrap(); base @@ -112,11 +184,7 @@ mod tests { let leftovers: Vec<_> = fs::read_dir(&dir) .unwrap() .filter_map(|e| e.ok()) - .filter(|e| { - e.file_name() - .to_string_lossy() - .ends_with(".tmp") - }) + .filter(|e| e.file_name().to_string_lossy().ends_with(".tmp")) .collect(); assert!( leftovers.is_empty(), @@ -125,4 +193,31 @@ mod tests { ); fs::remove_dir_all(&dir).ok(); } + + // Unix-only: the "executable bit lost on overwrite" regression the + // review flagged is only observable through the Unix permission + // model. On Windows `std::fs::Permissions` exposes only the + // read-only bit and ACLs are inherited from the parent directory, + // so there is no meaningful assertion beyond "the file still + // exists", which the other tests already cover. + #[cfg(unix)] + #[test] + fn preserves_unix_mode_bits_on_replace() { + use std::os::unix::fs::PermissionsExt; + + let dir = tmp_dir(); + let target = dir.join("perm.txt"); + fs::write(&target, b"original").unwrap(); + + let mut perms = fs::metadata(&target).unwrap().permissions(); + perms.set_mode(0o755); + fs::set_permissions(&target, perms).unwrap(); + + write_atomic(&target, b"replaced").unwrap(); + + let new_perms = fs::metadata(&target).unwrap().permissions(); + assert_eq!(new_perms.mode() & 0o777, 0o755); + + fs::remove_dir_all(&dir).ok(); + } }