From bd150c640cc167105af7db2c86d9e9660b8a8fa8 Mon Sep 17 00:00:00 2001 From: tupe12334 Date: Mon, 9 Nov 2026 04:17:50 +0900 Subject: [PATCH] fix(state): fsync state.json before atomic rename and clean up temp file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit save_state wrote a temp file and renamed it over state.json, but never synced the temp file first. On ext4 (delalloc) or APFS a crash or power loss after the rename can persist the rename before the data, leaving an empty or truncated state.json and a session that fails to load. A failed write or rename also left state.json.tmp. behind. Write through File + sync_all before the rename, and remove the temp file on failure. Adds tests for overwrite + no leftover temp, and for cleanup when the rename fails. Ratchets clippy::mem_forget to deny per the pre-push checklist. Opened by the moadim routine "Polyhook/steplock production-readiness audit → PR + Slack". Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +++ core/Cargo.toml | 1 + core/src/state.rs | 27 +++++++++++++++++++++------ core/src/state_tests.rs | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f34c81b..dcfacb5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Block messages read as a checklist step and show the `steplock step done` command instead of `sh .../ack.sh`. Claude Code auto mode flagged running `ack.sh` as the agent approving its own gate, so agents in auto mode could not finish any checklist - With `allow_preview_request = true`, the first block message lists every step instead of pointing at `preview.sh` +### Fixed +- `state.json` writes are now fsynced before the atomic rename, so a crash or power loss can no longer leave an empty or truncated session state. A failed write no longer leaves a `state.json.tmp.` file behind + ### Deprecated - `ack.sh` and `preview.sh` are still written to each session dir so old sessions keep working, but no message mentions them. They will be removed in a future release diff --git a/core/Cargo.toml b/core/Cargo.toml index fcb6712..039eaa1 100644 --- a/core/Cargo.toml +++ b/core/Cargo.toml @@ -91,6 +91,7 @@ needless_pass_by_value = "deny" trivially_copy_pass_by_ref = "deny" verbose_file_reads = "deny" unused_self = "deny" +mem_forget = "deny" [dependencies] cel-interpreter = "0.10" diff --git a/core/src/state.rs b/core/src/state.rs index 57915a1..b523c0b 100644 --- a/core/src/state.rs +++ b/core/src/state.rs @@ -1,6 +1,7 @@ //! Session state persistence and core domain types. use std::collections::HashMap; -use std::fs; +use std::fs::{self, File}; +use std::io::{self, Write}; use std::path::Path; use std::process; @@ -48,18 +49,32 @@ pub fn load_state(path: &Path) -> Result { Ok(serde_json::from_str(&content)?) } -/// Atomically write `state` to `path` via a PID-suffixed temp file and POSIX `mv`. +/// Atomically write `state` to `path` via a PID-suffixed temp file and rename. +/// +/// The temp file is fsynced before the rename, so a crash or power loss leaves +/// either the old or the new `state.json` — never an empty or truncated one. +/// The temp file is removed if the write or rename fails. /// /// # Errors /// -/// Returns an error if serialization fails or the write or rename fails. +/// Returns an error if serialization fails or the write, sync, or rename fails. pub fn save_state(path: &Path, state: &SessionState) -> Result<()> { let dir = path.parent().unwrap_or_else(|| Path::new(".")); let tmp = dir.join(format!("state.json.tmp.{}", process::id())); let content = serde_json::to_string_pretty(state)?; - fs::write(&tmp, content)?; - fs::rename(&tmp, path)?; - Ok(()) + let written = write_synced(&tmp, content.as_bytes()).and_then(|()| fs::rename(&tmp, path)); + if written.is_err() { + // ponytail: best-effort cleanup; the original error is what matters. + let _ignored: io::Result<()> = fs::remove_file(&tmp); + } + Ok(written?) +} + +/// Write `bytes` to a new file at `path` and flush it to disk. +fn write_synced(path: &Path, bytes: &[u8]) -> io::Result<()> { + let mut file = File::create(path)?; + file.write_all(bytes)?; + file.sync_all() } /// Initialize fresh state for the first (initial) state of a flow. diff --git a/core/src/state_tests.rs b/core/src/state_tests.rs index f371bbc..fd167c3 100644 --- a/core/src/state_tests.rs +++ b/core/src/state_tests.rs @@ -59,3 +59,40 @@ fn load_state_error_on_missing_file() { let path = tmp.path().join("nonexistent.json"); load_state(&path).unwrap_err(); } + +#[test] +fn save_state_overwrites_and_leaves_no_temp_file() { + let tmp = TempDir::new().unwrap(); + let path = tmp.path().join("state.json"); + save_state(&path, &init_state("gate", "first")).unwrap(); + save_state(&path, &init_state("gate", "second")).unwrap(); + assert_eq!( + load_state(&path).unwrap().current_state, + "second", + "second save must win" + ); + let names: Vec<_> = fs::read_dir(tmp.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); + assert_eq!(names, vec!["state.json"], "temp file must be renamed away"); +} + +#[test] +fn save_state_failure_removes_temp_file() { + let tmp = TempDir::new().unwrap(); + // Renaming a file over a non-empty directory fails on every platform. + let path = tmp.path().join("state.json"); + fs::create_dir(&path).unwrap(); + fs::write(path.join("keep"), "x").unwrap(); + save_state(&path, &init_state("gate", "first")).unwrap_err(); + let names: Vec<_> = fs::read_dir(tmp.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); + assert_eq!( + names, + vec!["state.json"], + "temp file must be cleaned up on failure" + ); +}