From 83ec4b1c64246e474dc14d055d466aeb31529716 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Wed, 14 Oct 2026 11:35:50 -0400 Subject: [PATCH] T02: unique temp file per save, parse errors name the field --- Cargo.lock | 12 ++++++ Cargo.toml | 1 + src/settings/store.rs | 97 +++++++++++++++++++++++++++++++++++-------- 3 files changed, 92 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cc0a089..010b8ed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1757,6 +1757,7 @@ dependencies = [ "log", "serde", "serde_json", + "serde_path_to_error", ] [[package]] @@ -5926,6 +5927,17 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_path_to_error" +version = "0.1.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "10a9ff822e371bb5403e391ecd83e182e0e77ba7f6fe0160b795797109d1b457" +dependencies = [ + "itoa", + "serde", + "serde_core", +] + [[package]] name = "serde_repr" version = "0.1.21" diff --git a/Cargo.toml b/Cargo.toml index b5e3194..e10ce15 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ gpui_platform = { package = "gpui-pre-platform", version = "=0.3.8", features = log = "0.4" serde = { version = "1", features = ["derive"] } serde_json = "1" +serde_path_to_error = "0.1.20" [lints.rust] unexpected_cfgs = { level = "warn", check-cfg = ['cfg(feature, values("cargo-clippy"))'] } diff --git a/src/settings/store.rs b/src/settings/store.rs index 5b31528..26391fe 100644 --- a/src/settings/store.rs +++ b/src/settings/store.rs @@ -2,6 +2,7 @@ use std::{ fs, io::{ErrorKind, Write}, path::{Path, PathBuf}, + sync::atomic::{AtomicU64, Ordering}, }; use anyhow::Context as _; @@ -58,7 +59,17 @@ pub fn load(path: &Path) -> anyhow::Result { return Err(error).with_context(|| format!("settings: cannot read {}", path.display())); } }; - let settings: Settings = serde_json::from_str(&text) + let mut reader = serde_json::Deserializer::from_str(&text); + let settings: Settings = serde_path_to_error::deserialize(&mut reader).map_err(|error| { + anyhow::anyhow!( + "settings: {} does not parse at `{}`: {}", + path.display(), + error.path(), + error.inner() + ) + })?; + reader + .end() .with_context(|| format!("settings: {} does not parse", path.display()))?; settings .validate() @@ -74,25 +85,40 @@ pub fn save(path: &Path, settings: &Settings) -> anyhow::Result<()> { .with_context(|| format!("settings: {} has no folder", path.display()))?; fs::create_dir_all(folder) .with_context(|| format!("settings: cannot create {}", folder.display()))?; - let temp = path.with_extension("json.tmp"); let mut text = serde_json::to_string_pretty(settings).context("settings: cannot encode")?; text.push('\n'); - let mut file = fs::File::create(&temp) - .with_context(|| format!("settings: cannot create {}", temp.display()))?; - file.write_all(text.as_bytes()) - .and_then(|()| file.sync_all()) - .with_context(|| format!("settings: cannot write {}", temp.display()))?; - fs::rename(&temp, path).with_context(|| { - format!( - "settings: cannot move {} to {}", - temp.display(), - path.display() - ) - })?; + let count = SAVES.fetch_add(1, Ordering::Relaxed); + let temp = path.with_extension(format!("json.{}-{count}.tmp", std::process::id())); + let written = fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&temp) + .and_then(|mut file| { + file.write_all(text.as_bytes())?; + file.sync_all() + }) + .and_then(|()| fs::rename(&temp, path)); + if let Err(error) = written { + if temp.exists() + && let Err(left) = fs::remove_file(&temp) + { + log::error!("settings: {} stays behind: {left}", temp.display()); + } + return Err(error).with_context(|| { + format!( + "settings: cannot write {} through {}", + path.display(), + temp.display() + ) + }); + } log::info!("settings: saved {}", path.display()); Ok(()) } +/// Saves made by this process, so each temp file has its own name. +static SAVES: AtomicU64 = AtomicU64::new(0); + #[cfg(test)] mod tests { use super::*; @@ -129,6 +155,34 @@ mod tests { fs::remove_dir_all(folder).expect("cleanup"); } + #[test] + fn overlapping_saves_leave_one_whole_file() { + let folder = folder("overlap"); + let path = folder.join("settings.json"); + let writers: Vec<_> = (0..4) + .map(|writer| { + let path = path.clone(); + std::thread::spawn(move || { + for round in 0..50 { + let settings = Settings { + scrollback: writer * 1000 + round, + ..Settings::default() + }; + save(&path, &settings).expect("each save lands"); + } + }) + }) + .collect(); + for writer in writers { + writer.join().expect("a writer finishes"); + } + let settings = load(&path).expect("the file is whole"); + assert_eq!(settings.scrollback % 1000, 49); + let left: Vec<_> = fs::read_dir(&folder).expect("folder").collect(); + assert_eq!(left.len(), 1, "only settings.json remains"); + fs::remove_dir_all(folder).expect("cleanup"); + } + #[test] fn a_missing_field_takes_its_default() { let folder = folder("partial"); @@ -150,10 +204,17 @@ mod tests { let error = load(&path).expect_err("does not parse"); assert!(format!("{error:#}").contains("does not parse")); assert!(error.to_string().contains("settings.json")); - fs::write(&path, r#"{ "cursor_style": "beam" }"#).expect("write"); - assert!(format!("{:#}", load(&path).expect_err("unknown style")).contains("beam")); - fs::write(&path, r#"{ "font_sise": 12 }"#).expect("write"); - assert!(format!("{:#}", load(&path).expect_err("unknown field")).contains("font_sise")); + let named = |json: &str, field: &str| { + fs::write(&path, json).expect("write"); + let error = format!("{:#}", load(&path).expect_err(json)); + assert!(error.contains("settings.json"), "{error}"); + assert!(error.contains(field), "{error}"); + }; + named(r#"{ "cursor_style": "beam" }"#, "`cursor_style`"); + named(r#"{ "scrollback": -1 }"#, "`scrollback`"); + named(r#"{ "font_size": "large" }"#, "`font_size`"); + named(r#"{ "font_sise": 12 }"#, "font_sise"); + named(r#"{ "font_size": 12 } x"#, "trailing characters"); fs::remove_dir_all(folder).expect("cleanup"); }