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" + ); +}