diff --git a/crates/codegen/kigi-shell-base/src/util/fs.rs b/crates/codegen/kigi-shell-base/src/util/fs.rs new file mode 100644 index 0000000..06bf47e --- /dev/null +++ b/crates/codegen/kigi-shell-base/src/util/fs.rs @@ -0,0 +1,87 @@ +//! Filesystem primitives shared across the shell. + +use std::io; +use std::path::Path; + +/// Replace `dest` with `tmp` — the commit step of every tmp+rename atomic +/// write in the product. This is the ONE place that knows how to make that +/// commit stick on Windows; call sites must never inline a bare +/// `fs::rename` replace again. +/// +/// Unix `rename(2)` replaces atomically and needs no help. Windows +/// `MoveFileExW(REPLACE_EXISTING)` fails with a sharing violation while +/// ANOTHER process (antivirus scanner, search indexer, cloud sync) holds +/// `dest` open — the classic "persists on macOS, silently doesn't on +/// Windows" failure (a model switch that never sticks, a stale models +/// cache). On Windows a failed rename therefore deletes the destination +/// first (the pattern `auth/storage.rs` shipped first) and retries with two +/// short back-offs for scanners that hold the file for a few milliseconds. +/// +/// On final failure the tmp file is removed (no litter) and the error is +/// returned — callers decide severity, but MUST at least log it (errors +/// never pass silently). +pub fn replace_file(tmp: &Path, dest: &Path) -> io::Result<()> { + let result = replace_file_inner(tmp, dest); + if result.is_err() { + let _ = std::fs::remove_file(tmp); + } + result +} + +#[cfg(not(windows))] +fn replace_file_inner(tmp: &Path, dest: &Path) -> io::Result<()> { + std::fs::rename(tmp, dest) +} + +#[cfg(windows)] +fn replace_file_inner(tmp: &Path, dest: &Path) -> io::Result<()> { + let mut last = match std::fs::rename(tmp, dest) { + Ok(()) => return Ok(()), + Err(e) => e, + }; + for backoff_ms in [0u64, 10, 50] { + if backoff_ms > 0 { + std::thread::sleep(std::time::Duration::from_millis(backoff_ms)); + } + // Delete-first: marks an open-with-delete-sharing file for deletion + // and clears the way for a plain rename; harmless when absent. + let _ = std::fs::remove_file(dest); + match std::fs::rename(tmp, dest) { + Ok(()) => return Ok(()), + Err(e) => last = e, + } + } + Err(last) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The common contract on every platform: replace over an existing + /// destination, create a missing one, and error (cleaning the tmp) + /// when the tmp itself is missing. + #[test] + fn replace_file_commits_and_cleans_up() { + let dir = tempfile::tempdir().expect("tempdir"); + let dest = dir.path().join("target.json"); + let tmp = dir.path().join("target.json.tmp"); + + // Create-missing. + std::fs::write(&tmp, b"v1").unwrap(); + replace_file(&tmp, &dest).expect("create"); + assert_eq!(std::fs::read(&dest).unwrap(), b"v1"); + assert!(!tmp.exists(), "tmp must be consumed"); + + // Replace-existing. + std::fs::write(&tmp, b"v2").unwrap(); + replace_file(&tmp, &dest).expect("replace"); + assert_eq!(std::fs::read(&dest).unwrap(), b"v2"); + assert!(!tmp.exists()); + + // Missing tmp → error, dest untouched. + let err = replace_file(&tmp, &dest).expect_err("missing tmp must fail"); + assert_eq!(err.kind(), io::ErrorKind::NotFound); + assert_eq!(std::fs::read(&dest).unwrap(), b"v2"); + } +} diff --git a/crates/codegen/kigi-shell-base/src/util/mod.rs b/crates/codegen/kigi-shell-base/src/util/mod.rs index 681115e..72b4ae0 100644 --- a/crates/codegen/kigi-shell-base/src/util/mod.rs +++ b/crates/codegen/kigi-shell-base/src/util/mod.rs @@ -1,4 +1,5 @@ pub mod event_id; +pub mod fs; pub mod kigi_home; pub mod secure_file; pub mod tips; diff --git a/crates/codegen/kigi-shell/src/active_sessions.rs b/crates/codegen/kigi-shell/src/active_sessions.rs index 064b21a..abb88c5 100644 --- a/crates/codegen/kigi-shell/src/active_sessions.rs +++ b/crates/codegen/kigi-shell/src/active_sessions.rs @@ -170,9 +170,7 @@ fn write_data_file_atomic( let json = serde_json::to_string_pretty(sessions) .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; fs::write(tmp_path, json.as_bytes())?; - fs::rename(tmp_path, data_path).inspect_err(|_| { - let _ = fs::remove_file(tmp_path); - }) + crate::util::fs::replace_file(tmp_path, data_path) } fn is_pid_alive(pid: u32) -> bool { diff --git a/crates/codegen/kigi-shell/src/agent/models.rs b/crates/codegen/kigi-shell/src/agent/models.rs index e4c0810..ad23723 100644 --- a/crates/codegen/kigi-shell/src/agent/models.rs +++ b/crates/codegen/kigi-shell/src/agent/models.rs @@ -1637,16 +1637,31 @@ impl ModelsCacheManager { } } - /// Sync; see `load_fresh` note. + /// Unique tmp suffix (PID + nanos) so concurrent writers never share an + /// inode (mirrors `util::config::persist`). + fn tmp_path(&self) -> std::path::PathBuf { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + self.path + .with_extension(format!("json.tmp.{}.{}", std::process::id(), nanos)) + } + + /// Sync; see `load_fresh` note. Best-effort, but NEVER silent: a failed + /// cache write leaves a stale catalog on disk, which on Windows (sharing + /// violations) previously diverged picker behavior with zero trace. fn atomic_write(&self, cache: &ModelsCache) { if let Some(parent) = self.path.parent() { let _ = std::fs::create_dir_all(parent); } - let tmp = self.path.with_extension("json.tmp"); - if let Ok(json) = serde_json::to_vec_pretty(cache) - && std::fs::write(&tmp, &json).is_ok() - { - let _ = std::fs::rename(&tmp, &self.path); + let tmp = self.tmp_path(); + let result = serde_json::to_vec_pretty(cache) + .map_err(std::io::Error::other) + .and_then(|json| std::fs::write(&tmp, &json)) + .and_then(|()| crate::util::fs::replace_file(&tmp, &self.path)); + if let Err(e) = result { + tracing::warn!(error = %e, path = %self.path.display(), "models cache write failed"); } } @@ -1654,12 +1669,25 @@ impl ModelsCacheManager { if let Some(parent) = self.path.parent() { let _ = tokio::fs::create_dir_all(parent).await; } - let tmp = self.path.with_extension("json.tmp"); - let Ok(json) = serde_json::to_vec_pretty(cache) else { - return; + let tmp = self.tmp_path(); + let json = match serde_json::to_vec_pretty(cache) { + Ok(json) => json, + Err(e) => { + tracing::warn!(error = %e, "models cache serialize failed"); + return; + } }; - if tokio::fs::write(&tmp, &json).await.is_ok() { - let _ = tokio::fs::rename(&tmp, &self.path).await; + let result = match tokio::fs::write(&tmp, &json).await { + Ok(()) => { + let dest = self.path.clone(); + tokio::task::spawn_blocking(move || crate::util::fs::replace_file(&tmp, &dest)) + .await + .unwrap_or_else(|e| Err(std::io::Error::other(e))) + } + Err(e) => Err(e), + }; + if let Err(e) = result { + tracing::warn!(error = %e, path = %self.path.display(), "models cache write failed"); } } } diff --git a/crates/codegen/kigi-shell/src/auth/storage.rs b/crates/codegen/kigi-shell/src/auth/storage.rs index 81eb3ae..6ee508a 100644 --- a/crates/codegen/kigi-shell/src/auth/storage.rs +++ b/crates/codegen/kigi-shell/src/auth/storage.rs @@ -430,17 +430,12 @@ fn write_store_to(path: &Path, auth_store: &AuthStore) -> std::io::Result<()> { Ok(()) } -/// Atomic write: tmp + rename. Unix `rename(2)` replaces atomically; -/// Windows `rename` requires removing the target first. +/// Atomic write: tmp + Windows-safe replace (see `util::fs::replace_file`, +/// which this site's inline delete-first pattern graduated into). fn write_auth_json_atomic(auth_file: &Path, auth_store: &AuthStore) -> std::io::Result<()> { let tmp = auth_file.with_extension(format!("json.{}.tmp", std::process::id())); write_store_to(&tmp, auth_store)?; - #[cfg(windows)] - { - let _ = std::fs::remove_file(auth_file); - } - std::fs::rename(&tmp, auth_file)?; - Ok(()) + crate::util::fs::replace_file(&tmp, auth_file) } /// Non-atomic fallback: truncate and rewrite `auth.json` in place. diff --git a/crates/codegen/kigi-shell/src/claude_import.rs b/crates/codegen/kigi-shell/src/claude_import.rs index 1a7050d..fe63023 100644 --- a/crates/codegen/kigi-shell/src/claude_import.rs +++ b/crates/codegen/kigi-shell/src/claude_import.rs @@ -670,10 +670,7 @@ fn write_import_marker(config_path: &Path) -> anyhow::Result<()> { let _ = std::fs::remove_file(&tmp); return Err(e.into()); } - if let Err(e) = std::fs::rename(&tmp, config_path) { - let _ = std::fs::remove_file(&tmp); - return Err(e.into()); - } + crate::util::fs::replace_file(&tmp, config_path)?; Ok(()) } @@ -854,7 +851,7 @@ fn apply_items_to_config(config_path: &Path, items: &[ImportableItem]) -> anyhow std::fs::create_dir_all(parent)?; } std::fs::write(&tmp, &toml_str)?; - std::fs::rename(&tmp, config_path)?; + crate::util::fs::replace_file(&tmp, config_path)?; info!( path = %config_path.display(), count, @@ -1204,7 +1201,7 @@ fn apply_hooks_to_dir(hooks_dir: &Path, items: &[ImportableItem]) -> anyhow::Res let json_str = serde_json::to_string_pretty(&root)?; let tmp = target.with_extension("json.tmp"); std::fs::write(&tmp, &json_str)?; - std::fs::rename(&tmp, &target)?; + crate::util::fs::replace_file(&tmp, &target)?; info!( path = %target.display(), count, diff --git a/crates/codegen/kigi-shell/src/claude_import_state.rs b/crates/codegen/kigi-shell/src/claude_import_state.rs index 87aeedf..38e60e8 100644 --- a/crates/codegen/kigi-shell/src/claude_import_state.rs +++ b/crates/codegen/kigi-shell/src/claude_import_state.rs @@ -90,7 +90,7 @@ pub fn save_import_state(state: &ImportState) -> std::io::Result<()> { // `claude_import_state.json.tmp` (the last extension is replaced). let tmp = path.with_extension("json.tmp"); std::fs::write(&tmp, &json)?; - std::fs::rename(&tmp, &path)?; + crate::util::fs::replace_file(&tmp, &path)?; Ok(()) } diff --git a/crates/codegen/kigi-shell/src/kimi_import.rs b/crates/codegen/kigi-shell/src/kimi_import.rs index 4ed374d..900b4e8 100644 --- a/crates/codegen/kigi-shell/src/kimi_import.rs +++ b/crates/codegen/kigi-shell/src/kimi_import.rs @@ -528,10 +528,7 @@ pub fn apply_at(plan: &KimiImportPlan, kigi_home: &Path) -> anyhow::Result std::io::Result<()> .unwrap_or("goal-classifier.patch"); let tmp = dir.join(format!(".{file_name}.{}.tmp", uuid::Uuid::now_v7())); tokio::fs::write(&tmp, body).await?; - if let Err(err) = tokio::fs::rename(&tmp, path).await { - let _ = tokio::fs::remove_file(&tmp).await; - return Err(err); - } + let dest = path.to_path_buf(); + tokio::task::spawn_blocking(move || crate::util::fs::replace_file(&tmp, &dest)) + .await + .map_err(std::io::Error::other)??; Ok(()) } diff --git a/crates/codegen/kigi-shell/src/session/goal_tracker.rs b/crates/codegen/kigi-shell/src/session/goal_tracker.rs index fe367b7..3433e12 100644 --- a/crates/codegen/kigi-shell/src/session/goal_tracker.rs +++ b/crates/codegen/kigi-shell/src/session/goal_tracker.rs @@ -895,7 +895,8 @@ impl GoalTracker { }; let dest = goal_dir.join(name); let _ = std::fs::create_dir_all(&goal_dir); - if std::fs::rename(&src, &dest).is_ok() || copy_no_follow(&src, &dest).is_ok() { + if crate::util::fs::replace_file(&src, &dest).is_ok() || copy_no_follow(&src, &dest).is_ok() + { append_skeptic_reports(&scratch_root, &dest); o.last_classifier_details_path = Some(dest.to_string_lossy().into_owned()); } diff --git a/crates/codegen/kigi-shell/src/session/graph_project.rs b/crates/codegen/kigi-shell/src/session/graph_project.rs index 991e8d5..4e2aa4f 100644 --- a/crates/codegen/kigi-shell/src/session/graph_project.rs +++ b/crates/codegen/kigi-shell/src/session/graph_project.rs @@ -120,7 +120,7 @@ pub fn project(dir: &Path, state: &GraphOrchestration) -> std::io::Result<()> { f.write_all(&buf)?; f.sync_all()?; } - std::fs::rename(&tmp, &target) + crate::util::fs::replace_file(&tmp, &target) } /// Load the projected graph, `Ok(None)` when absent. Malformed content diff --git a/crates/codegen/kigi-shell/src/session/prompt_history.rs b/crates/codegen/kigi-shell/src/session/prompt_history.rs index a5c5004..f35086f 100644 --- a/crates/codegen/kigi-shell/src/session/prompt_history.rs +++ b/crates/codegen/kigi-shell/src/session/prompt_history.rs @@ -98,7 +98,7 @@ pub fn truncate_if_needed(cwd: &str) -> io::Result<()> { } } - std::fs::rename(temp_path, path)?; + crate::util::fs::replace_file(&temp_path, &path)?; Ok(()) } diff --git a/crates/codegen/kigi-shell/src/session/storage/jsonl/mod.rs b/crates/codegen/kigi-shell/src/session/storage/jsonl/mod.rs index 4580276..1e88858 100644 --- a/crates/codegen/kigi-shell/src/session/storage/jsonl/mod.rs +++ b/crates/codegen/kigi-shell/src/session/storage/jsonl/mod.rs @@ -13,6 +13,16 @@ use std::fs::OpenOptions; use std::io::{self, Read}; use std::path::{Path, PathBuf}; use tokio::io::AsyncWriteExt; + +/// Commit `tmp` over `target` with the shared Windows-safe replace +/// (`util::fs::replace_file`): a bare async rename silently lost session +/// state — including the switched model — on Windows whenever AV/indexer +/// held the destination open. +async fn replace_file_async(tmp: PathBuf, target: PathBuf) -> io::Result<()> { + tokio::task::spawn_blocking(move || crate::util::fs::replace_file(&tmp, &target)) + .await + .unwrap_or_else(|e| Err(io::Error::other(e))) +} /// How the adapter resolves the session directory on disk. /// /// - `FromRoot` (default): computes `{root}/sessions/{urlencoded(cwd)}/{session_id}/` @@ -289,7 +299,7 @@ impl JsonlStorageAdapter { } let tmp = path.with_extension("jsonl.tmp"); tokio::fs::write(&tmp, &content).await?; - tokio::fs::rename(&tmp, &path).await + replace_file_async(tmp, path).await } fn read_jsonl(&self, path: PathBuf) -> io::Result> { if !path.exists() { @@ -378,7 +388,7 @@ impl JsonlStorageAdapter { .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; let tmp = summary_path.with_extension("json.tmp"); std::fs::write(&tmp, &bytes)?; - std::fs::rename(&tmp, &summary_path) + crate::util::fs::replace_file(&tmp, &summary_path) } fn read_summary_sync(&self, info: &Info) -> io::Result { let path = self.summary_file(info); @@ -1057,7 +1067,7 @@ impl StorageAdapter for JsonlStorageAdapter { let target = self.plan_mode_state_file(info); let tmp = target.with_extension("json.tmp"); tokio::fs::write(&tmp, json).await?; - tokio::fs::rename(&tmp, &target).await + replace_file_async(tmp, target).await } async fn write_signals( &self, @@ -1069,7 +1079,7 @@ impl StorageAdapter for JsonlStorageAdapter { let target = self.signals_file(info); let tmp = target.with_extension("json.tmp"); tokio::fs::write(&tmp, signals_json).await?; - tokio::fs::rename(&tmp, &target).await + replace_file_async(tmp, target).await } async fn write_announcement_state( &self, @@ -1081,7 +1091,7 @@ impl StorageAdapter for JsonlStorageAdapter { let target = self.announcement_state_file(info); let tmp = target.with_extension("json.tmp"); tokio::fs::write(&tmp, json).await?; - tokio::fs::rename(&tmp, &target).await + replace_file_async(tmp, target).await } async fn write_goal_mode_state( &self, @@ -1096,7 +1106,7 @@ impl StorageAdapter for JsonlStorageAdapter { } let tmp = target.with_extension("json.tmp"); tokio::fs::write(&tmp, json).await?; - tokio::fs::rename(&tmp, &target).await + replace_file_async(tmp, target).await } async fn write_graph_mode_state( &self, @@ -1119,7 +1129,7 @@ impl StorageAdapter for JsonlStorageAdapter { } let tmp = target.with_extension("json.tmp"); tokio::fs::write(&tmp, json).await?; - tokio::fs::rename(&tmp, &target).await + replace_file_async(tmp, target).await } async fn load_session(&self, info: &Info) -> io::Result { let summary = self.read_summary_sync(info)?; diff --git a/crates/codegen/kigi-shell/src/session/storage/summary_write.rs b/crates/codegen/kigi-shell/src/session/storage/summary_write.rs index bf558ad..8295dfd 100644 --- a/crates/codegen/kigi-shell/src/session/storage/summary_write.rs +++ b/crates/codegen/kigi-shell/src/session/storage/summary_write.rs @@ -230,7 +230,10 @@ fn write_summary_atomic(summary_path: &Path, summary: &Summary) -> io::Result<() .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; let tmp = summary_path.with_extension("json.tmp"); std::fs::write(&tmp, &bytes)?; - std::fs::rename(&tmp, summary_path) + // Windows-safe replace: this is the write that persists a session's + // CURRENT MODEL — a bare rename made a switched model silently revert + // on resume whenever AV/indexer held summary.json open on Windows. + crate::util::fs::replace_file(&tmp, summary_path) } #[cfg(test)] diff --git a/crates/codegen/kigi-shell/src/util/config/campaigns.rs b/crates/codegen/kigi-shell/src/util/config/campaigns.rs index a6e382b..10ce445 100644 --- a/crates/codegen/kigi-shell/src/util/config/campaigns.rs +++ b/crates/codegen/kigi-shell/src/util/config/campaigns.rs @@ -114,9 +114,7 @@ fn dismiss_campaign_ids_at( let nonce = DISMISS_TMP_NONCE.fetch_add(1, Ordering::Relaxed); let tmp = path.with_extension(format!("json.{}.{}.tmp", std::process::id(), nonce)); std::fs::write(&tmp, &json)?; - std::fs::rename(&tmp, &path).inspect_err(|_| { - let _ = std::fs::remove_file(&tmp); - }) + crate::util::fs::replace_file(&tmp, &path) } /// `KIGI_CAMPAIGNS_OVERRIDE` JSON array replaces all sources (`[]` = none; beats diff --git a/crates/codegen/kigi-shell/src/util/config/mcp.rs b/crates/codegen/kigi-shell/src/util/config/mcp.rs index 52b4f6a..a4a312d 100644 --- a/crates/codegen/kigi-shell/src/util/config/mcp.rs +++ b/crates/codegen/kigi-shell/src/util/config/mcp.rs @@ -367,7 +367,9 @@ pub async fn save_mcp_disabled_tools(server_name: &str, disabled_tools: &[String let _ = tokio::fs::create_dir_all(parent).await; } tokio::fs::write(&tmp, &toml_str).await?; - tokio::fs::rename(&tmp, &path).await?; + tokio::task::spawn_blocking(move || crate::util::fs::replace_file(&tmp, &path)) + .await + .map_err(std::io::Error::other)??; Ok(()) } @@ -419,7 +421,9 @@ pub async fn save_mcp_server_enabled(server_name: &str, enabled: bool) -> Result let _ = tokio::fs::create_dir_all(parent).await; } tokio::fs::write(&tmp, &toml_str).await?; - tokio::fs::rename(&tmp, &path).await?; + tokio::task::spawn_blocking(move || crate::util::fs::replace_file(&tmp, &path)) + .await + .map_err(std::io::Error::other)??; Ok(()) } @@ -476,7 +480,10 @@ pub async fn save_mcp_server_config_at( let _ = tokio::fs::create_dir_all(parent).await; } tokio::fs::write(&tmp, &toml_str).await?; - tokio::fs::rename(&tmp, &path).await?; + let dest = path.to_path_buf(); + tokio::task::spawn_blocking(move || crate::util::fs::replace_file(&tmp, &dest)) + .await + .map_err(std::io::Error::other)??; Ok(()) } @@ -553,7 +560,12 @@ pub async fn delete_mcp_server_config_at( let _ = tokio::fs::create_dir_all(parent).await; } tokio::fs::write(&tmp, &toml_str).await?; - tokio::fs::rename(&tmp, &path).await?; + { + let dest = path.to_path_buf(); + tokio::task::spawn_blocking(move || crate::util::fs::replace_file(&tmp, &dest)) + .await + .map_err(std::io::Error::other)??; + } // Clean up OAuth credentials for the deleted server. if let Ok(mut cred_store) = kigi_mcp::credentials::McpCredentialStore::load_default() { diff --git a/crates/codegen/kigi-shell/src/util/config/persist.rs b/crates/codegen/kigi-shell/src/util/config/persist.rs index f7cd61b..89bf3cb 100644 --- a/crates/codegen/kigi-shell/src/util/config/persist.rs +++ b/crates/codegen/kigi-shell/src/util/config/persist.rs @@ -88,7 +88,12 @@ pub async fn save_config(config: &Config) -> Result<()> { } let _ = prior_mode; - tokio::fs::rename(&tmp, &path).await?; + // Windows-safe replace (delete-first + retry on sharing violations) — + // a bare rename made `/model` persistence silently fail on Windows + // whenever AV/indexer/cloud-sync held config.toml open. + tokio::task::spawn_blocking(move || crate::util::fs::replace_file(&tmp, &path)) + .await + .map_err(|e| anyhow::anyhow!("config replace task: {e}"))??; Ok(()) } @@ -138,11 +143,8 @@ pub(crate) fn atomic_write_string(path: &std::path::Path, content: &str) -> std: } let _ = prior_mode; - if let Err(e) = std::fs::rename(&tmp, path) { - let _ = std::fs::remove_file(&tmp); - return Err(e); - } - Ok(()) + // Windows-safe replace; cleans up the tmp file on failure itself. + crate::util::fs::replace_file(&tmp, path) } /// Merge `[toolset.ask_user_question]` into the root table. `[toolset]` is