T02: unique temp file per save, parse errors name the field

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"

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"))'] }

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<Settings> {
             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)
+    let count = SAVES.fetch_add(1, Ordering::Relaxed);
-        .with_context(|| format!("settings: cannot create {}", temp.display()))?;
+    let temp = path.with_extension(format!("json.{}-{count}.tmp", std::process::id()));
-    file.write_all(text.as_bytes())
+    let written = fs::OpenOptions::new()
-        .and_then(|()| file.sync_all())
+        .write(true)
-        .with_context(|| format!("settings: cannot write {}", temp.display()))?;
+        .create_new(true)
-    fs::rename(&temp, path).with_context(|| {
+        .open(&temp)
-        format!(
+        .and_then(|mut file| {
-            "settings: cannot move {} to {}",
+            file.write_all(text.as_bytes())?;
-            temp.display(),
+            file.sync_all()
-            path.display()
+        })
-        )
+        .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");
+        let named = |json: &str, field: &str| {
-        assert!(format!("{:#}", load(&path).expect_err("unknown style")).contains("beam"));
+            fs::write(&path, json).expect("write");
-        fs::write(&path, r#"{ "font_sise": 12 }"#).expect("write");
+            let error = format!("{:#}", load(&path).expect_err(json));
-        assert!(format!("{:#}", load(&path).expect_err("unknown field")).contains("font_sise"));
+            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");
     }