From 5ea28337ec39278887508ae982adcecb2a96638f Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Fri, 23 Oct 2026 04:48:30 +0530 Subject: [PATCH 1/2] fix(fs): atomic writes through one sink; file_write takes the path lock --- .../src/filesystem/apply_patch/mod.rs | 5 +- .../src/filesystem/csv_export/mod.rs | 2 +- .../tinytools-std/src/filesystem/file_sink.rs | 56 ++++++++++++++++++- .../src/filesystem/file_sink_tests.rs | 44 +++++++++++++++ .../src/filesystem/file_write/mod.rs | 4 ++ .../src/filesystem/update_memory_md/mod.rs | 49 ++-------------- .../filesystem/update_memory_md/mod_tests.rs | 2 +- 7 files changed, 114 insertions(+), 48 deletions(-) create mode 100644 crates/tinytools-std/src/filesystem/file_sink_tests.rs diff --git a/crates/tinytools-std/src/filesystem/apply_patch/mod.rs b/crates/tinytools-std/src/filesystem/apply_patch/mod.rs index 26b252f..b05a3b6 100644 --- a/crates/tinytools-std/src/filesystem/apply_patch/mod.rs +++ b/crates/tinytools-std/src/filesystem/apply_patch/mod.rs @@ -14,6 +14,7 @@ //! containing `x`). An empty `old_string` against a file that *does* exist is //! still an error — "replace nothing" is ambiguous, not a create. +use super::file_sink::atomic_write; use super::gate::{FsGate, gate_for_context}; use crate::file_state; use async_trait::async_trait; @@ -390,7 +391,7 @@ impl ApplyPatchTool { let mut written: Vec<&FileBuffer> = Vec::new(); for buf in buffers.values() { let path = &buf.path; - if let Err(e) = tokio::fs::write(&buf.resolved, &buf.contents).await { + if let Err(e) = atomic_write(&buf.resolved, buf.contents.as_bytes()).await { let restore_errors = restore_originals(&written).await; let suffix = if restore_errors.is_empty() { "; previously-written files restored from snapshot".to_string() @@ -428,7 +429,7 @@ async fn restore_originals(written: &[&FileBuffer]) -> Vec { let mut errors = Vec::new(); for buf in written { let result = match &buf.original { - Some(original) => tokio::fs::write(&buf.resolved, original).await, + Some(original) => atomic_write(&buf.resolved, original.as_bytes()).await, // The batch created this file, so "restore" means remove it — // otherwise a failed multi-file patch leaves a half-written new // file behind and the caller cannot tell it apart from a success. diff --git a/crates/tinytools-std/src/filesystem/csv_export/mod.rs b/crates/tinytools-std/src/filesystem/csv_export/mod.rs index 354c6ce..7c55155 100644 --- a/crates/tinytools-std/src/filesystem/csv_export/mod.rs +++ b/crates/tinytools-std/src/filesystem/csv_export/mod.rs @@ -241,7 +241,7 @@ impl CsvExportTool { } // Write the CSV file - match tokio::fs::write(&resolved_target, &csv_content).await { + match super::file_sink::atomic_write(&resolved_target, csv_content.as_bytes()).await { Ok(()) => { let size_display = if csv_bytes >= 1024 * 1024 { format!("{:.1} MB", csv_bytes as f64 / (1024.0 * 1024.0)) diff --git a/crates/tinytools-std/src/filesystem/file_sink.rs b/crates/tinytools-std/src/filesystem/file_sink.rs index a50dcf2..1ed9fd4 100644 --- a/crates/tinytools-std/src/filesystem/file_sink.rs +++ b/crates/tinytools-std/src/filesystem/file_sink.rs @@ -16,8 +16,10 @@ use std::path::Path; use std::sync::Arc; +use std::sync::atomic::{AtomicU64, Ordering}; use async_trait::async_trait; +use tokio::io::AsyncWriteExt; /// Where a tool's file writes land. #[async_trait] @@ -32,10 +34,58 @@ pub(super) struct OsFileSink; #[async_trait] impl FileSink for OsFileSink { async fn write(&self, path: &Path, contents: &[u8]) -> std::io::Result<()> { - tokio::fs::write(path, contents).await + atomic_write(path, contents).await } } +/// Monotonic counter for temp-file names within a process. +static TEMP_FILE_SEQ: AtomicU64 = AtomicU64::new(0); + +/// Atomically replace `path`'s contents with `contents`. +/// +/// Writes a fresh (`create_new`) sibling temp file in the same directory, so +/// the rename stays on one filesystem, gives it the target's permissions, and +/// `rename`s it over the target. A crash mid-write leaves the old file or the +/// complete new one, never a truncation, and a symlink at `path` is replaced +/// rather than followed. The rename installs a new inode: other hard links +/// keep the old contents, and the file is owned by the writing user. +pub(super) async fn atomic_write(path: &Path, contents: &[u8]) -> std::io::Result<()> { + let dir = path.parent().ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!("{} has no parent directory", path.display()), + ) + })?; + let name = path.file_name().unwrap_or_default().to_string_lossy(); + let seq = TEMP_FILE_SEQ.fetch_add(1, Ordering::Relaxed); + let tmp = dir.join(format!(".{name}.{}.{seq}.tmp", std::process::id())); + let mut file = tokio::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&tmp) + .await + .map_err(|e| std::io::Error::new(e.kind(), format!("stage temp file: {e}")))?; + let mut result = stage(&mut file, path, contents).await; + drop(file); + if result.is_ok() { + result = tokio::fs::rename(&tmp, path).await; + } + if result.is_err() { + let _ = tokio::fs::remove_file(&tmp).await; + } + result +} + +/// Fill the temp file, copy the target's permissions onto it (or `+x` is +/// lost on the rename), and flush it to disk before the rename. +async fn stage(file: &mut tokio::fs::File, target: &Path, contents: &[u8]) -> std::io::Result<()> { + file.write_all(contents).await?; + if let Ok(meta) = tokio::fs::metadata(target).await { + file.set_permissions(meta.permissions()).await?; + } + file.sync_all().await +} + /// The sink a tool uses unless a caller installs another one. pub(super) fn os_sink() -> Arc { Arc::new(OsFileSink) @@ -57,3 +107,7 @@ impl FileSink for RefusingSink { )) } } + +#[cfg(test)] +#[path = "file_sink_tests.rs"] +mod tests; diff --git a/crates/tinytools-std/src/filesystem/file_sink_tests.rs b/crates/tinytools-std/src/filesystem/file_sink_tests.rs new file mode 100644 index 0000000..7542726 --- /dev/null +++ b/crates/tinytools-std/src/filesystem/file_sink_tests.rs @@ -0,0 +1,44 @@ +//! The OS sink replaces a file atomically: through a temp file and a rename. + +#![allow(clippy::unwrap_used)] + +use super::*; + +// A symlink swapped in after a tool's symlink check must not redirect the +// write: the rename replaces the link itself, and its target is untouched. +#[cfg(unix)] +#[tokio::test] +async fn a_write_replaces_a_symlink_instead_of_following_it() { + let dir = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let secret = outside.path().join("secret.txt"); + std::fs::write(&secret, "secret").unwrap(); + let link = dir.path().join("a.txt"); + std::os::unix::fs::symlink(&secret, &link).unwrap(); + + OsFileSink.write(&link, b"new").await.unwrap(); + + assert_eq!(std::fs::read_to_string(&secret).unwrap(), "secret"); + assert!(!std::fs::symlink_metadata(&link).unwrap().is_symlink()); + assert_eq!(std::fs::read_to_string(&link).unwrap(), "new"); +} + +// The rename installs a new inode, so the target's mode bits are copied to +// it first: an executable script stays executable after an edit. +#[cfg(unix)] +#[tokio::test] +async fn a_write_keeps_the_targets_permissions() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let script = dir.path().join("run.sh"); + std::fs::write(&script, "old").unwrap(); + std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o750)).unwrap(); + + OsFileSink.write(&script, b"new").await.unwrap(); + + let mode = std::fs::metadata(&script).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o750); + assert_eq!(std::fs::read_to_string(&script).unwrap(), "new"); + let leftovers = std::fs::read_dir(dir.path()).unwrap().count(); + assert_eq!(leftovers, 1, "a temp file was left behind"); +} diff --git a/crates/tinytools-std/src/filesystem/file_write/mod.rs b/crates/tinytools-std/src/filesystem/file_write/mod.rs index 0efcd66..f0cad90 100644 --- a/crates/tinytools-std/src/filesystem/file_write/mod.rs +++ b/crates/tinytools-std/src/filesystem/file_write/mod.rs @@ -194,6 +194,10 @@ impl FileWriteTool { )); } + // Per-path lock, as edit_file and apply_patch take: the stale check and + // the write must not interleave with another tool's edit of the file. + let _path_guard = file_state::acquire_path_lock(&resolved_target).await; + // File-state guard: reject writes based on stale or partial reads. if let Some(agent_id) = file_state::current_file_state_agent_id() { if let Some(msg) = file_state::check_stale_read(&agent_id, &resolved_target) { diff --git a/crates/tinytools-std/src/filesystem/update_memory_md/mod.rs b/crates/tinytools-std/src/filesystem/update_memory_md/mod.rs index 8797b20..072cda4 100644 --- a/crates/tinytools-std/src/filesystem/update_memory_md/mod.rs +++ b/crates/tinytools-std/src/filesystem/update_memory_md/mod.rs @@ -4,7 +4,6 @@ use async_trait::async_trait; use serde_json::json; use std::collections::HashMap; use std::path::{Path, PathBuf}; -use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, LazyLock, Mutex}; use tinytools::ToolRunContext; use tinytools::{PermissionLevel, Tool, ToolCallOptions, ToolResult}; @@ -25,9 +24,6 @@ const ALLOWED_FILES: &[&str] = &["MEMORY.md", "SKILL.md"]; static WORKSPACE_WRITE_LOCKS: LazyLock>>>> = LazyLock::new(|| Mutex::new(HashMap::new())); -/// Monotonic counter for temp-file uniqueness within a process. -static TEMP_FILE_SEQ: AtomicU64 = AtomicU64::new(0); - /// Return (creating if needed) the shared async write lock for `workspace_dir`. /// /// The lock key is the canonicalized workspace path when it resolves (so two @@ -93,46 +89,13 @@ async fn acquire_cross_process_write_lock(workspace_dir: &Path) -> anyhow::Resul .map_err(|e| anyhow::anyhow!("workspace lock task join failed: {e}"))? } -/// Atomically replace `path`'s contents with `content`. -/// -/// Writes to a sibling temp file in the same directory (so the rename stays on -/// one filesystem and is atomic) and `rename`s it over the target. A crash -/// mid-write leaves either the old file or the complete new file — never a -/// half-written truncation. Callers MUST hold the per-workspace write lock so -/// the read-modify-write is serialized end-to-end. +/// [`super::file_sink::atomic_write`], with the error naming `file`. +/// Callers MUST hold the per-workspace write lock so the read-modify-write is +/// serialized end-to-end. async fn atomic_write(path: &Path, file: &str, content: &str) -> anyhow::Result<()> { - let dir = path - .parent() - .ok_or_else(|| anyhow::anyhow!("target path has no parent directory"))?; - let seq = TEMP_FILE_SEQ.fetch_add(1, Ordering::Relaxed); - let tmp_name = format!(".{file}.{}.{seq}.tmp", std::process::id()); - let tmp_path = dir.join(tmp_name); - - tracing::debug!( - tmp = %tmp_path.display(), - target = %path.display(), - bytes = content.len(), - "[update_memory_md] atomic write: staging temp file" - ); - - if let Err(e) = tokio::fs::write(&tmp_path, content).await { - // Clean up a partially-written temp file so a failed stage doesn't - // litter the workspace (CodeRabbit: temp not cleaned on initial write). - let _ = tokio::fs::remove_file(&tmp_path).await; - return Err(anyhow::anyhow!("Failed to stage temp file for {file}: {e}")); - } - - if let Err(e) = tokio::fs::rename(&tmp_path, path).await { - // Best-effort cleanup so a failed rename doesn't litter the workspace. - let _ = tokio::fs::remove_file(&tmp_path).await; - return Err(anyhow::anyhow!("Failed to atomically write {file}: {e}")); - } - - tracing::debug!( - target = %path.display(), - "[update_memory_md] atomic write: rename committed" - ); - Ok(()) + super::file_sink::atomic_write(path, content.as_bytes()) + .await + .map_err(|e| anyhow::anyhow!("Failed to atomically write {file}: {e}")) } /// Appends or replaces a named section in MEMORY.md or SKILL.md. diff --git a/crates/tinytools-std/src/filesystem/update_memory_md/mod_tests.rs b/crates/tinytools-std/src/filesystem/update_memory_md/mod_tests.rs index ca2cfb1..509e690 100644 --- a/crates/tinytools-std/src/filesystem/update_memory_md/mod_tests.rs +++ b/crates/tinytools-std/src/filesystem/update_memory_md/mod_tests.rs @@ -265,7 +265,7 @@ async fn atomic_write_reports_staging_and_rename_failures() { let err = atomic_write(&missing_dir_target, "MEMORY.md", "x") .await .unwrap_err(); - assert!(err.to_string().contains("Failed to stage temp file")); + assert!(err.to_string().contains("stage temp file"), "{err}"); // Renaming a file over a directory fails after the temp file was staged. let dir_target = dir.path().join("MEMORY.md"); From 4d42cbac95222ae3493bbcbdb13e7e132f74fe9e Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Fri, 23 Oct 2026 04:58:30 +0530 Subject: [PATCH 2/2] test(file_sink): cover the failure paths of atomic_write --- .../src/filesystem/file_sink_tests.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crates/tinytools-std/src/filesystem/file_sink_tests.rs b/crates/tinytools-std/src/filesystem/file_sink_tests.rs index 7542726..f4678b3 100644 --- a/crates/tinytools-std/src/filesystem/file_sink_tests.rs +++ b/crates/tinytools-std/src/filesystem/file_sink_tests.rs @@ -42,3 +42,21 @@ async fn a_write_keeps_the_targets_permissions() { let leftovers = std::fs::read_dir(dir.path()).unwrap().count(); assert_eq!(leftovers, 1, "a temp file was left behind"); } + +#[tokio::test] +async fn failed_writes_report_why_and_leave_no_temp_file() { + let no_parent = atomic_write(Path::new("/"), b"x").await.unwrap_err(); + assert_eq!(no_parent.kind(), std::io::ErrorKind::InvalidInput); + + let dir = tempfile::tempdir().unwrap(); + let missing_dir = atomic_write(&dir.path().join("nope").join("a.txt"), b"x") + .await + .unwrap_err(); + assert!(missing_dir.to_string().contains("stage temp file")); + + // Renaming a file over a directory fails after the temp file was staged. + let target = dir.path().join("a.txt"); + std::fs::create_dir(&target).unwrap(); + assert!(atomic_write(&target, b"x").await.is_err()); + assert_eq!(std::fs::read_dir(dir.path()).unwrap().count(), 1); +}