fix(fs): Windows-safe atomic replace everywhere — model switch now sticks

Root cause of 'model+effort switch works on Mac, not on Windows': the
switch APPLIES in-session (the dispatch/apply chain is platform-identical,
verified adversarially) but its persistence never sticks on Windows.
Every tmp+rename atomic write except auth/storage.rs committed with a
bare fs::rename, and Windows MoveFileExW(REPLACE_EXISTING) fails with a
sharing violation whenever AV/search-indexer/cloud-sync transiently holds
the destination open. Consequences: [models].default never persisted
(next launch = original model), the session summary's current model never
persisted (resume = original model), and the models cache went silently
stale (all its write errors were swallowed).

- New kigi_shell_base::util::fs::replace_file — THE commit step for
  tmp+rename: plain rename on Unix; on Windows delete-first + two short
  backoffs (the pattern auth/storage.rs shipped first), tmp cleaned on
  failure, error always returned. Windows branch type-checked against
  x86_64-pc-windows-msvc.
- Adopted at every replace site: config.toml (save_config /
  atomic_write_string / mcp saves), models cache (plus unique tmp
  suffixes and tracing::warn on failure — writes were fully silent),
  session storage (summary/current-model, jsonl, plan/signals/
  announcement/goal/graph state), auth.json, active-sessions registry,
  prompt history, claude/kimi import, campaigns state, goal artifacts.
  Directory-move renames (worktree pool, corrupt-file backups) keep
  plain rename — their destinations don't pre-exist.

Verified: kigi-shell + kigi-shell-base 5318 tests green, clippy clean,
msvc-target check of the new cfg(windows) code clean.
This commit is contained in:
2026-07-22 19:42:13 -04:00
parent 815bd99356
commit 27d009cb6e
18 changed files with 191 additions and 62 deletions
@@ -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");
}
}
@@ -1,4 +1,5 @@
pub mod event_id;
pub mod fs;
pub mod kigi_home;
pub mod secure_file;
pub mod tips;
@@ -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 {
+39 -11
View File
@@ -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");
}
}
}
@@ -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.
@@ -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,
@@ -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(())
}
+1 -4
View File
@@ -528,10 +528,7 @@ pub fn apply_at(plan: &KimiImportPlan, kigi_home: &Path) -> anyhow::Result<KimiA
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)?;
info!(
path = %config_path.display(),
added = applied.total_added(),
@@ -1147,7 +1147,7 @@ fn persist_chat_history_jsonl_sync(session_info: &SessionInfo, conversation: &[C
buf.push(b'\n');
}
std::fs::File::create(&tmp_path)?.write_all(&buf)?;
std::fs::rename(&tmp_path, &final_path)?;
crate::util::fs::replace_file(&tmp_path, &final_path)?;
Ok(())
})();
if let Err(e) = result {
@@ -594,10 +594,10 @@ async fn write_patch_file_atomic(path: &Path, body: &str) -> 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(())
}
@@ -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());
}
@@ -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
@@ -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(())
}
@@ -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<T: serde::de::DeserializeOwned>(&self, path: PathBuf) -> io::Result<Vec<T>> {
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<Summary> {
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<PersistedData> {
let summary = self.read_summary_sync(info)?;
@@ -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)]
@@ -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
@@ -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() {
@@ -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