fix(config): atomic save + corruption-preserving load (A19)
AppConfig::save now writes to a same-dir temp file and atomically renames over the target (mirrors identity.rs/friends.rs), and surfaces errors via log_msg instead of swallowing them. AppConfig::load distinguishes a missing config (silent default, first run) from a present-but-corrupt one: the damaged file is moved aside to config.json.corrupt.<unix_secs> before falling back to defaults, so a later save can no longer clobber the user's real prefs. Path-injectable seams save_to/load_from + LoadOutcome with unit tests (round-trip, missing, corrupt-preserves-bytes, no leftover temp). No new deps, no schema/wire change. Codex-implemented (gpt-5.5 xhigh), senior-reviewed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+207
-14
@@ -1,9 +1,12 @@
|
||||
use crate::notify::Sound;
|
||||
use crate::theme::AppTheme;
|
||||
use anyhow::Context;
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::collections::{BTreeMap, HashMap};
|
||||
use std::fs;
|
||||
use std::path::PathBuf;
|
||||
use std::io::Write;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::time::{SystemTime, UNIX_EPOCH};
|
||||
|
||||
/// Relay/discovery posture, trading connectivity against how much the n0
|
||||
/// infrastructure learns about you. See the network module for details.
|
||||
@@ -345,6 +348,13 @@ pub struct AppConfig {
|
||||
pub window_y: Option<i32>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum LoadOutcome {
|
||||
Missing,
|
||||
Loaded,
|
||||
Recovered,
|
||||
}
|
||||
|
||||
impl Default for AppConfig {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
@@ -475,24 +485,115 @@ impl AppConfig {
|
||||
}
|
||||
|
||||
pub fn load() -> Self {
|
||||
if let Some(path) = Self::config_path()
|
||||
&& let Ok(contents) = fs::read_to_string(&path)
|
||||
&& let Ok(config) = serde_json::from_str(&contents) {
|
||||
return config;
|
||||
}
|
||||
Self::default()
|
||||
let Some(path) = Self::config_path() else {
|
||||
return Self::default();
|
||||
};
|
||||
let (config, _) = Self::load_from(&path);
|
||||
config
|
||||
}
|
||||
|
||||
pub fn save(&self) {
|
||||
if let Some(path) = Self::config_path() {
|
||||
if let Some(dir) = path.parent() {
|
||||
let _ = fs::create_dir_all(dir);
|
||||
if let Err(e) = self.save_to(&path) {
|
||||
crate::log_msg(&format!("config: save failed: {e:#}"));
|
||||
}
|
||||
if let Ok(json) = serde_json::to_string_pretty(self) {
|
||||
let _ = fs::write(path, json);
|
||||
} else {
|
||||
crate::log_msg("config: save failed: could not determine a config directory");
|
||||
}
|
||||
}
|
||||
|
||||
pub fn load_from(path: &Path) -> (Self, LoadOutcome) {
|
||||
match fs::read_to_string(path) {
|
||||
Ok(contents) => match serde_json::from_str(&contents) {
|
||||
Ok(config) => (config, LoadOutcome::Loaded),
|
||||
Err(e) => {
|
||||
let backup = recover_corrupt_config(path, &format!("failed to parse: {e}"));
|
||||
(Self::default(), backup)
|
||||
}
|
||||
},
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
|
||||
(Self::default(), LoadOutcome::Missing)
|
||||
}
|
||||
Err(e) => {
|
||||
let backup = recover_corrupt_config(path, &format!("failed to read: {e}"));
|
||||
(Self::default(), backup)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub fn save_to(&self, path: &Path) -> anyhow::Result<()> {
|
||||
let parent = path
|
||||
.parent()
|
||||
.context("config path has no parent directory")?;
|
||||
fs::create_dir_all(parent)
|
||||
.with_context(|| format!("failed to create {}", parent.display()))?;
|
||||
|
||||
let json = serde_json::to_string_pretty(self).context("failed to encode config")?;
|
||||
let tmp = config_tmp_path(path)?;
|
||||
let result = (|| -> anyhow::Result<()> {
|
||||
{
|
||||
let mut f = fs::File::create(&tmp)
|
||||
.with_context(|| format!("failed to create {}", tmp.display()))?;
|
||||
f.write_all(json.as_bytes())
|
||||
.with_context(|| format!("failed to write {}", tmp.display()))?;
|
||||
f.sync_all().ok();
|
||||
}
|
||||
fs::rename(&tmp, path).with_context(|| {
|
||||
format!("failed to rename {} -> {}", tmp.display(), path.display())
|
||||
})?;
|
||||
Ok(())
|
||||
})();
|
||||
|
||||
if result.is_err() {
|
||||
let _ = fs::remove_file(&tmp);
|
||||
}
|
||||
result
|
||||
}
|
||||
}
|
||||
|
||||
fn config_tmp_path(path: &Path) -> anyhow::Result<PathBuf> {
|
||||
let parent = path
|
||||
.parent()
|
||||
.context("config path has no parent directory")?;
|
||||
let mut name = path
|
||||
.file_name()
|
||||
.context("config path has no file name")?
|
||||
.to_os_string();
|
||||
name.push(format!(".tmp.{}", std::process::id()));
|
||||
Ok(parent.join(name))
|
||||
}
|
||||
|
||||
fn corrupt_backup_path(path: &Path) -> PathBuf {
|
||||
let secs = SystemTime::now()
|
||||
.duration_since(UNIX_EPOCH)
|
||||
.map(|d| d.as_secs())
|
||||
.unwrap_or(0);
|
||||
let parent = path.parent().unwrap_or_else(|| Path::new("."));
|
||||
let mut name = path
|
||||
.file_name()
|
||||
.map(|n| n.to_os_string())
|
||||
.unwrap_or_else(|| "config.json".into());
|
||||
name.push(format!(".corrupt.{secs}"));
|
||||
parent.join(name)
|
||||
}
|
||||
|
||||
fn recover_corrupt_config(path: &Path, reason: &str) -> LoadOutcome {
|
||||
let backup = corrupt_backup_path(path);
|
||||
match fs::rename(path, &backup) {
|
||||
Ok(()) => {
|
||||
crate::log_msg(&format!(
|
||||
"config: {reason}; moved damaged config to {}",
|
||||
backup.display()
|
||||
));
|
||||
}
|
||||
Err(e) => {
|
||||
crate::log_msg(&format!(
|
||||
"config: {reason}; failed to move damaged config to {}: {e}",
|
||||
backup.display()
|
||||
));
|
||||
}
|
||||
}
|
||||
LoadOutcome::Recovered
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -507,11 +608,103 @@ mod tests {
|
||||
assert_eq!(original, deserialized);
|
||||
}
|
||||
|
||||
fn temp_config_path(tag: &str) -> PathBuf {
|
||||
let mut p = std::env::temp_dir();
|
||||
p.push(format!(
|
||||
"peerspeak-configtest-{}-{}",
|
||||
std::process::id(),
|
||||
tag
|
||||
));
|
||||
p.push("config.json");
|
||||
p
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn save_to_then_load_from_round_trips() {
|
||||
let path = temp_config_path("roundtrip");
|
||||
let _ = fs::remove_dir_all(path.parent().unwrap());
|
||||
let cfg = AppConfig {
|
||||
username: "Ada".into(),
|
||||
input_device: "mic".into(),
|
||||
output_device: "speaker".into(),
|
||||
noise_gate_threshold: 0.42,
|
||||
..AppConfig::default()
|
||||
};
|
||||
|
||||
cfg.save_to(&path).unwrap();
|
||||
let (loaded, outcome) = AppConfig::load_from(&path);
|
||||
|
||||
assert_eq!(outcome, LoadOutcome::Loaded);
|
||||
assert_eq!(loaded, cfg);
|
||||
let _ = fs::remove_dir_all(path.parent().unwrap());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_from_missing_returns_default_without_corrupt_backup() {
|
||||
let path = temp_config_path("missing");
|
||||
let _ = fs::remove_dir_all(path.parent().unwrap());
|
||||
|
||||
let (loaded, outcome) = AppConfig::load_from(&path);
|
||||
|
||||
assert_eq!(outcome, LoadOutcome::Missing);
|
||||
assert_eq!(loaded, AppConfig::default());
|
||||
assert!(!path.parent().unwrap().exists());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_from_corrupt_file_preserves_original_bytes() {
|
||||
let path = temp_config_path("corrupt");
|
||||
let _ = fs::remove_dir_all(path.parent().unwrap());
|
||||
fs::create_dir_all(path.parent().unwrap()).unwrap();
|
||||
let corrupt = b"{ this is not json";
|
||||
fs::write(&path, corrupt).unwrap();
|
||||
|
||||
let (loaded, outcome) = AppConfig::load_from(&path);
|
||||
|
||||
assert_eq!(outcome, LoadOutcome::Recovered);
|
||||
assert_eq!(loaded, AppConfig::default());
|
||||
assert_ne!(fs::read(&path).ok().as_deref(), Some(corrupt.as_slice()));
|
||||
let backups: Vec<_> = fs::read_dir(path.parent().unwrap())
|
||||
.unwrap()
|
||||
.map(|entry| entry.unwrap().path())
|
||||
.filter(|entry| {
|
||||
entry
|
||||
.file_name()
|
||||
.and_then(|name| name.to_str())
|
||||
.is_some_and(|name| name.starts_with("config.json.corrupt."))
|
||||
})
|
||||
.collect();
|
||||
assert_eq!(backups.len(), 1, "expected one corrupt backup");
|
||||
assert_eq!(fs::read(&backups[0]).unwrap(), corrupt);
|
||||
let _ = fs::remove_dir_all(path.parent().unwrap());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn save_to_leaves_no_tmp_file_after_success() {
|
||||
let path = temp_config_path("atomic");
|
||||
let _ = fs::remove_dir_all(path.parent().unwrap());
|
||||
|
||||
AppConfig::default().save_to(&path).unwrap();
|
||||
|
||||
let tmp_files: Vec<_> = fs::read_dir(path.parent().unwrap())
|
||||
.unwrap()
|
||||
.map(|entry| entry.unwrap().path())
|
||||
.filter(|entry| {
|
||||
entry
|
||||
.file_name()
|
||||
.and_then(|name| name.to_str())
|
||||
.is_some_and(|name| name.contains(".tmp."))
|
||||
})
|
||||
.collect();
|
||||
assert!(tmp_files.is_empty(), "leftover temp files: {tmp_files:?}");
|
||||
let _ = fs::remove_dir_all(path.parent().unwrap());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_backward_compat_default_fill() {
|
||||
let minimal_json = r#"{"input_device":"","output_device":"","noise_gate_threshold":0.01}"#;
|
||||
let deserialized: AppConfig = serde_json::from_str(minimal_json).unwrap();
|
||||
|
||||
|
||||
assert_eq!(deserialized.network_mode, NetworkMode::RelayNoDiscovery);
|
||||
// Configs predating the presence posture load as friends-only (no beacon).
|
||||
assert_eq!(deserialized.presence_mode, crate::presence::PresenceMode::Normal);
|
||||
@@ -813,11 +1006,11 @@ mod tests {
|
||||
"unrecognized_field_xyz_123": "some_value"
|
||||
}"#;
|
||||
let deserialized_res: Result<AppConfig, _> = serde_json::from_str(json_with_extra);
|
||||
|
||||
|
||||
// Assert that deserialization succeeds even with unrecognized/unknown fields.
|
||||
// This confirms that serde does not reject unknown fields (i.e. default behavior).
|
||||
assert!(deserialized_res.is_ok(), "Config deserialization failed when an unknown field was present");
|
||||
|
||||
|
||||
let config = deserialized_res.unwrap();
|
||||
assert_eq!(config.input_device, "");
|
||||
assert_eq!(config.output_device, "");
|
||||
|
||||
Reference in New Issue
Block a user