From 79553e0b8bb0ca570f63e43cb57f54af0c58773a Mon Sep 17 00:00:00 2001 From: Zane <75694352+zanesq@users.noreply.github.com> Date: Thu, 26 Jun 2025 11:08:57 -0700 Subject: [PATCH] Improve config file editing and recovery fallback mechanisms (#3082) --- crates/goose-server/src/openapi.rs | 2 + .../src/routes/config_management.rs | 127 ++-- crates/goose/src/config/base.rs | 627 ++++++++++++++---- crates/goose/src/session/storage.rs | 3 +- ui/desktop/openapi.json | 46 ++ ui/desktop/src/App.tsx | 42 +- ui/desktop/src/api/sdk.gen.ts | 16 +- ui/desktop/src/api/types.gen.ts | 46 ++ 8 files changed, 722 insertions(+), 187 deletions(-) diff --git a/crates/goose-server/src/openapi.rs b/crates/goose-server/src/openapi.rs index 317376f4..ae77398f 100644 --- a/crates/goose-server/src/openapi.rs +++ b/crates/goose-server/src/openapi.rs @@ -23,6 +23,8 @@ use utoipa::OpenApi; #[openapi( paths( super::routes::config_management::backup_config, + super::routes::config_management::recover_config, + super::routes::config_management::validate_config, super::routes::config_management::init_config, super::routes::config_management::upsert_config, super::routes::config_management::remove_config, diff --git a/crates/goose-server/src/routes/config_management.rs b/crates/goose-server/src/routes/config_management.rs index e2e919b9..b1b609ee 100644 --- a/crates/goose-server/src/routes/config_management.rs +++ b/crates/goose-server/src/routes/config_management.rs @@ -457,44 +457,15 @@ pub async fn init_config( return Ok(Json("Config already exists".to_string())); } - let workspace_root = match std::env::current_exe() { - Ok(mut exe_path) => { - while let Some(parent) = exe_path.parent() { - let cargo_toml = parent.join("Cargo.toml"); - if cargo_toml.exists() { - if let Ok(content) = std::fs::read_to_string(&cargo_toml) { - if content.contains("[workspace]") { - exe_path = parent.to_path_buf(); - break; - } - } - } - exe_path = parent.to_path_buf(); - } - exe_path - } - Err(_) => return Err(StatusCode::INTERNAL_SERVER_ERROR), - }; - - let init_config_path = workspace_root.join("init-config.yaml"); - if !init_config_path.exists() { - return Ok(Json( + // Use the shared function to load init-config.yaml + match goose::config::base::load_init_config_from_workspace() { + Ok(init_values) => match config.save_values(init_values) { + Ok(_) => Ok(Json("Config initialized successfully".to_string())), + Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR), + }, + Err(_) => Ok(Json( "No init-config.yaml found, using default configuration".to_string(), - )); - } - - let init_content = match std::fs::read_to_string(&init_config_path) { - Ok(content) => content, - Err(_) => return Err(StatusCode::INTERNAL_SERVER_ERROR), - }; - let init_values: HashMap = match serde_yaml::from_str(&init_content) { - Ok(values) => values, - Err(_) => return Err(StatusCode::INTERNAL_SERVER_ERROR), - }; - - match config.save_values(init_values) { - Ok(_) => Ok(Json("Config initialized successfully".to_string())), - Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR), + )), } } @@ -555,8 +526,8 @@ pub async fn backup_config( backup_name.push(".bak"); let backup = config_path.with_file_name(backup_name); - match std::fs::rename(&config_path, &backup) { - Ok(_) => Ok(Json(format!("Moved {:?} to {:?}", config_path, backup))), + match std::fs::copy(&config_path, &backup) { + Ok(_) => Ok(Json(format!("Copied {:?} to {:?}", config_path, backup))), Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR), } } else { @@ -564,6 +535,82 @@ pub async fn backup_config( } } +#[utoipa::path( + post, + path = "/config/recover", + responses( + (status = 200, description = "Config recovery attempted", body = String), + (status = 500, description = "Internal server error") + ) +)] +pub async fn recover_config( + State(state): State>, + headers: HeaderMap, +) -> Result, StatusCode> { + verify_secret_key(&headers, &state)?; + + let config = Config::global(); + + // Force a reload which will trigger recovery if needed + match config.load_values() { + Ok(values) => { + let recovered_keys: Vec = values.keys().cloned().collect(); + if recovered_keys.is_empty() { + Ok(Json("Config recovery completed, but no data was recoverable. Starting with empty configuration.".to_string())) + } else { + Ok(Json(format!( + "Config recovery completed. Recovered {} keys: {}", + recovered_keys.len(), + recovered_keys.join(", ") + ))) + } + } + Err(e) => { + tracing::error!("Config recovery failed: {}", e); + Err(StatusCode::INTERNAL_SERVER_ERROR) + } + } +} + +#[utoipa::path( + get, + path = "/config/validate", + responses( + (status = 200, description = "Config validation result", body = String), + (status = 422, description = "Config file is corrupted") + ) +)] +pub async fn validate_config( + State(state): State>, + headers: HeaderMap, +) -> Result, StatusCode> { + verify_secret_key(&headers, &state)?; + + let config_dir = choose_app_strategy(APP_STRATEGY.clone()) + .expect("goose requires a home dir") + .config_dir(); + + let config_path = config_dir.join("config.yaml"); + + if !config_path.exists() { + return Ok(Json("Config file does not exist".to_string())); + } + + match std::fs::read_to_string(&config_path) { + Ok(content) => match serde_yaml::from_str::(&content) { + Ok(_) => Ok(Json("Config file is valid".to_string())), + Err(e) => { + tracing::warn!("Config validation failed: {}", e); + Err(StatusCode::UNPROCESSABLE_ENTITY) + } + }, + Err(e) => { + tracing::error!("Failed to read config file: {}", e); + Err(StatusCode::INTERNAL_SERVER_ERROR) + } + } +} + #[utoipa::path( get, path = "/config/current-model", @@ -597,6 +644,8 @@ pub fn routes(state: Arc) -> Router { .route("/config/pricing", post(get_pricing)) .route("/config/init", post(init_config)) .route("/config/backup", post(backup_config)) + .route("/config/recover", post(recover_config)) + .route("/config/validate", get(validate_config)) .route("/config/permissions", post(upsert_permissions)) .route("/config/current-model", get(get_current_model)) .with_state(state) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 68ceff41..eabdaf4d 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -7,7 +7,7 @@ use serde_json::Value; use std::collections::HashMap; use std::env; use std::fs::OpenOptions; -use std::io::{Read, Seek, SeekFrom, Write}; +use std::io::Write; use std::path::{Path, PathBuf}; use thiserror::Error; @@ -200,22 +200,179 @@ impl Config { // Load current values from the config file pub fn load_values(&self) -> Result, ConfigError> { if self.config_path.exists() { - let file_content = std::fs::read_to_string(&self.config_path)?; - // Parse YAML into JSON Value for consistent internal representation - let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?; - let json_value: Value = serde_json::to_value(yaml_value)?; - - match json_value { - Value::Object(map) => Ok(map.into_iter().collect()), - _ => Ok(HashMap::new()), - } + self.load_values_with_recovery() } else { - Ok(HashMap::new()) + // Config file doesn't exist, try to recover from backup first + tracing::info!("Config file doesn't exist, attempting recovery from backup"); + + if let Ok(backup_values) = self.try_restore_from_backup() { + tracing::info!("Successfully restored config from backup"); + return Ok(backup_values); + } + + // No backup available, create a default config + tracing::info!("No backup found, creating default configuration"); + + // Try to load from init-config.yaml if it exists, otherwise use empty config + let default_config = self + .load_init_config_if_exists() + .unwrap_or_else(|_| HashMap::new()); + + self.create_and_save_default_config(default_config) } } + // Helper method to create and save default config with consistent logging + fn create_and_save_default_config( + &self, + default_config: HashMap, + ) -> Result, ConfigError> { + // Try to write the default config to disk + match self.save_values(default_config.clone()) { + Ok(_) => { + if default_config.is_empty() { + tracing::info!("Created fresh empty config file"); + } else { + tracing::info!( + "Created fresh config file from init-config.yaml with {} keys", + default_config.len() + ); + } + Ok(default_config) + } + Err(write_error) => { + tracing::error!("Failed to write default config file: {}", write_error); + // Even if we can't write to disk, return config so app can still run + Ok(default_config) + } + } + } + + // Load values with automatic recovery from corruption + fn load_values_with_recovery(&self) -> Result, ConfigError> { + let file_content = std::fs::read_to_string(&self.config_path)?; + + // First attempt: try to parse the current config + match self.parse_yaml_content(&file_content) { + Ok(values) => Ok(values), + Err(parse_error) => { + tracing::warn!( + "Config file appears corrupted, attempting recovery: {}", + parse_error + ); + + // Try to recover from backup + if let Ok(backup_values) = self.try_restore_from_backup() { + tracing::info!("Successfully restored config from backup"); + return Ok(backup_values); + } + + // Last resort: create a fresh default config file + tracing::error!("Could not recover config file, creating fresh default configuration. Original error: {}", parse_error); + + // Try to load from init-config.yaml if it exists, otherwise use empty config + let default_config = self + .load_init_config_if_exists() + .unwrap_or_else(|_| HashMap::new()); + + self.create_and_save_default_config(default_config) + } + } + } + + // Parse YAML content into HashMap + fn parse_yaml_content(&self, content: &str) -> Result, ConfigError> { + if content.trim().is_empty() { + return Ok(HashMap::new()); + } + + let yaml_value: serde_yaml::Value = serde_yaml::from_str(content)?; + let json_value: Value = serde_json::to_value(yaml_value)?; + + match json_value { + Value::Object(map) => Ok(map.into_iter().collect()), + _ => Ok(HashMap::new()), + } + } + + // Try to restore from backup file + fn try_restore_from_backup(&self) -> Result, ConfigError> { + let backup_paths = self.get_backup_paths(); + + for backup_path in backup_paths { + if backup_path.exists() { + match std::fs::read_to_string(&backup_path) { + Ok(backup_content) => { + match self.parse_yaml_content(&backup_content) { + Ok(values) => { + // Successfully parsed backup, restore it as the main config + if let Err(e) = self.save_values(values.clone()) { + tracing::warn!( + "Failed to restore backup as main config: {}", + e + ); + } else { + tracing::info!( + "Restored config from backup: {:?}", + backup_path + ); + } + return Ok(values); + } + Err(e) => { + tracing::warn!( + "Backup file {:?} is also corrupted: {}", + backup_path, + e + ); + continue; + } + } + } + Err(e) => { + tracing::warn!("Could not read backup file {:?}: {}", backup_path, e); + continue; + } + } + } + } + + Err(ConfigError::NotFound("No valid backup found".to_string())) + } + + // Get list of backup file paths in order of preference + fn get_backup_paths(&self) -> Vec { + let mut paths = Vec::new(); + + // Primary backup (created by backup_config endpoint) + if let Some(file_name) = self.config_path.file_name() { + let mut backup_name = file_name.to_os_string(); + backup_name.push(".bak"); + paths.push(self.config_path.with_file_name(backup_name)); + } + + // Timestamped backups + for i in 1..=5 { + if let Some(file_name) = self.config_path.file_name() { + let mut backup_name = file_name.to_os_string(); + backup_name.push(format!(".bak.{}", i)); + paths.push(self.config_path.with_file_name(backup_name)); + } + } + + paths + } + + // Try to load init-config.yaml from workspace root if it exists + fn load_init_config_if_exists(&self) -> Result, ConfigError> { + load_init_config_from_workspace() + } + // Save current values to the config file pub fn save_values(&self, values: HashMap) -> Result<(), ConfigError> { + // Create backup before writing new config + self.create_backup_if_needed()?; + // Convert to YAML for storage let yaml_value = serde_yaml::to_string(&values)?; @@ -225,22 +382,97 @@ impl Config { .map_err(|e| ConfigError::DirectoryError(e.to_string()))?; } - // Open the file with write permissions, create if it doesn't exist - let mut file = OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .open(&self.config_path)?; + // Write to a temporary file first for atomic operation + let temp_path = self.config_path.with_extension("tmp"); - // Acquire an exclusive lock - file.lock_exclusive() - .map_err(|e| ConfigError::LockError(e.to_string()))?; + { + let mut file = OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .open(&temp_path)?; - // Write the contents using the same file handle - file.write_all(yaml_value.as_bytes())?; - file.sync_all()?; + // Acquire an exclusive lock + file.lock_exclusive() + .map_err(|e| ConfigError::LockError(e.to_string()))?; + + // Write the contents using the same file handle + file.write_all(yaml_value.as_bytes())?; + file.sync_all()?; + + // Unlock is handled automatically when file is dropped + } + + // Atomically replace the original file + std::fs::rename(&temp_path, &self.config_path)?; + + Ok(()) + } + + // Create backup of current config file if it exists and is valid + fn create_backup_if_needed(&self) -> Result<(), ConfigError> { + if !self.config_path.exists() { + return Ok(()); + } + + // Check if current config is valid before backing it up + let current_content = std::fs::read_to_string(&self.config_path)?; + if self.parse_yaml_content(¤t_content).is_err() { + // Don't back up corrupted files + return Ok(()); + } + + // Rotate existing backups + self.rotate_backups()?; + + // Create new backup + if let Some(file_name) = self.config_path.file_name() { + let mut backup_name = file_name.to_os_string(); + backup_name.push(".bak"); + let backup_path = self.config_path.with_file_name(backup_name); + + if let Err(e) = std::fs::copy(&self.config_path, &backup_path) { + tracing::warn!("Failed to create config backup: {}", e); + // Don't fail the entire operation if backup fails + } else { + tracing::debug!("Created config backup: {:?}", backup_path); + } + } + + Ok(()) + } + + // Rotate backup files to keep the most recent ones + fn rotate_backups(&self) -> Result<(), ConfigError> { + if let Some(file_name) = self.config_path.file_name() { + // Move .bak.4 to .bak.5, .bak.3 to .bak.4, etc. + for i in (1..5).rev() { + let mut current_backup = file_name.to_os_string(); + current_backup.push(format!(".bak.{}", i)); + let current_path = self.config_path.with_file_name(¤t_backup); + + let mut next_backup = file_name.to_os_string(); + next_backup.push(format!(".bak.{}", i + 1)); + let next_path = self.config_path.with_file_name(&next_backup); + + if current_path.exists() { + let _ = std::fs::rename(¤t_path, &next_path); + } + } + + // Move .bak to .bak.1 + let mut backup_name = file_name.to_os_string(); + backup_name.push(".bak"); + let backup_path = self.config_path.with_file_name(&backup_name); + + if backup_path.exists() { + let mut backup_1_name = file_name.to_os_string(); + backup_1_name.push(".bak.1"); + let backup_1_path = self.config_path.with_file_name(&backup_1_name); + let _ = std::fs::rename(&backup_path, &backup_1_path); + } + } - // Unlock is handled automatically when file is dropped Ok(()) } @@ -342,56 +574,14 @@ impl Config { /// - There is an error reading or writing the config file /// - There is an error serializing the value pub fn set_param(&self, key: &str, value: Value) -> Result<(), ConfigError> { - // Ensure the directory exists - if let Some(parent) = self.config_path.parent() { - std::fs::create_dir_all(parent) - .map_err(|e| ConfigError::DirectoryError(e.to_string()))?; - } - - // Open the file with read+write permissions, create if it doesn't exist - let mut file = OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) - .open(&self.config_path)?; - - // Acquire an exclusive lock for the entire operation - file.lock_exclusive() - .map_err(|e| ConfigError::LockError(e.to_string()))?; - - // Load current values while holding the lock - read through the file handle - let mut values = { - let mut file_content = String::new(); - file.seek(SeekFrom::Start(0))?; - file.read_to_string(&mut file_content)?; - - if file_content.trim().is_empty() { - HashMap::new() - } else { - let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?; - let json_value: Value = serde_json::to_value(yaml_value)?; - match json_value { - Value::Object(map) => map.into_iter().collect(), - _ => HashMap::new(), - } - } - }; + // Load current values with recovery if needed + let mut values = self.load_values()?; // Modify values values.insert(key.to_string(), value); - // Convert to YAML for storage - let yaml_value = serde_yaml::to_string(&values)?; - - // Write the contents using the same file handle - file.seek(SeekFrom::Start(0))?; // Seek to beginning before writing - file.set_len(0)?; // Clear the file - file.write_all(yaml_value.as_bytes())?; - file.sync_all()?; - - // Unlock is handled automatically when file is dropped - Ok(()) + // Save all values using the atomic write approach + self.save_values(values) } /// Delete a configuration value in the config file. @@ -507,6 +697,60 @@ impl Config { } } +/// Load init-config.yaml from workspace root if it exists. +/// This function is shared between the config recovery and the init_config endpoint. +pub fn load_init_config_from_workspace() -> Result, ConfigError> { + let workspace_root = match std::env::current_exe() { + Ok(mut exe_path) => { + while let Some(parent) = exe_path.parent() { + let cargo_toml = parent.join("Cargo.toml"); + if cargo_toml.exists() { + if let Ok(content) = std::fs::read_to_string(&cargo_toml) { + if content.contains("[workspace]") { + exe_path = parent.to_path_buf(); + break; + } + } + } + exe_path = parent.to_path_buf(); + } + exe_path + } + Err(_) => { + return Err(ConfigError::FileError(std::io::Error::new( + std::io::ErrorKind::NotFound, + "Could not determine executable path", + ))) + } + }; + + let init_config_path = workspace_root.join("init-config.yaml"); + if !init_config_path.exists() { + return Err(ConfigError::NotFound( + "init-config.yaml not found".to_string(), + )); + } + + let init_content = std::fs::read_to_string(&init_config_path)?; + let init_values: HashMap = + match serde_yaml::from_str::(&init_content) { + Ok(yaml_value) => { + let json_value: Value = serde_json::to_value(yaml_value)?; + match json_value { + Value::Object(map) => map.into_iter().collect(), + _ => HashMap::new(), + } + } + Err(e) => { + tracing::warn!("Failed to parse init-config.yaml: {}", e); + return Err(ConfigError::DeserializeError(e.to_string())); + } + }; + + tracing::info!("Loaded init-config.yaml with {} keys", init_values.len()); + Ok(init_values) +} + #[cfg(test)] mod tests { use super::*; @@ -760,94 +1004,191 @@ mod tests { } #[test] - fn test_concurrent_extension_writes() -> Result<(), ConfigError> { - use std::sync::{Arc, Barrier}; - use std::thread; - use std::time::Duration; - + fn test_config_recovery_from_backup() -> Result<(), ConfigError> { let temp_file = NamedTempFile::new().unwrap(); - let config = Arc::new(Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?); - let barrier = Arc::new(Barrier::new(3)); // For 3 concurrent threads - let mut handles = vec![]; + let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; - // Initialize with empty values - config.save_values(HashMap::new())?; + // Create a valid config first + config.set_param("key1", Value::String("value1".to_string()))?; - // Spawn 3 threads that will try to write extension configs simultaneously - for i in 0..3 { - let config = Arc::clone(&config); - let barrier = Arc::clone(&barrier); - let handle = thread::spawn(move || -> Result<(), ConfigError> { - // Wait for all threads to reach this point - barrier.wait(); - - // Add a small random delay to increase chance of concurrent access - thread::sleep(Duration::from_millis(i * 10)); - - let extension_key = format!("extension_{}", i); - - // Use set_param which handles concurrent access properly - config.set_param( - &extension_key, - serde_json::json!({ - "name": format!("test_extension_{}", i), - "version": format!("1.0.{}", i), - "enabled": true, - "settings": { - "option1": format!("value{}", i), - "option2": i - } - }), - )?; - Ok(()) - }); - handles.push(handle); + // Verify the backup was created by the first write + let backup_paths = config.get_backup_paths(); + println!("Backup paths: {:?}", backup_paths); + for (i, path) in backup_paths.iter().enumerate() { + println!("Backup {} exists: {}", i, path.exists()); } - // Wait for all threads to complete - for handle in handles { - handle.join().unwrap()?; + // Make another write to ensure backup is created + config.set_param("key2", Value::Number(42.into()))?; + + // Check again + for (i, path) in backup_paths.iter().enumerate() { + println!( + "After second write - Backup {} exists: {}", + i, + path.exists() + ); } - // Verify all extension configs were written correctly - let final_values = config.load_values()?; + // Corrupt the main config file + std::fs::write(temp_file.path(), "invalid: yaml: content: [unclosed")?; - // Print the final values for debugging - println!("Final values: {:?}", final_values); + // Try to load values - should recover from backup + let recovered_values = config.load_values()?; + println!("Recovered values: {:?}", recovered_values); - assert_eq!( - final_values.len(), - 3, - "Expected 3 extension configs, got {}", - final_values.len() + // Should have recovered the data + assert!( + recovered_values.len() >= 1, + "Should have recovered at least one key" ); - for i in 0..3 { - let extension_key = format!("extension_{}", i); + Ok(()) + } - let config = final_values - .get(&extension_key) - .expect(&format!("Missing extension config for {}", extension_key)); + #[test] + fn test_config_recovery_creates_fresh_file() -> Result<(), ConfigError> { + let temp_file = NamedTempFile::new().unwrap(); + let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; - // Verify the structure matches what we wrote - let config_obj = config.as_object().unwrap(); - assert_eq!( - config_obj.get("name").unwrap().as_str().unwrap(), - format!("test_extension_{}", i) - ); - assert_eq!( - config_obj.get("version").unwrap().as_str().unwrap(), - format!("1.0.{}", i) - ); - assert!(config_obj.get("enabled").unwrap().as_bool().unwrap()); + // Create a corrupted config file with no backup + std::fs::write(temp_file.path(), "invalid: yaml: content: [unclosed")?; - let settings = config_obj.get("settings").unwrap().as_object().unwrap(); - assert_eq!( - settings.get("option1").unwrap().as_str().unwrap(), - format!("value{}", i) - ); - assert_eq!(settings.get("option2").unwrap().as_i64().unwrap() as i32, i); + // Try to load values - should create a fresh default config + let recovered_values = config.load_values()?; + + // Should return empty config + assert_eq!(recovered_values.len(), 0); + + // Verify that a clean config file was written to disk + let file_content = std::fs::read_to_string(temp_file.path())?; + + // Should be valid YAML (empty object) + let parsed: serde_yaml::Value = serde_yaml::from_str(&file_content)?; + assert!(parsed.is_mapping()); + + // Should be able to load it again without issues + let reloaded_values = config.load_values()?; + assert_eq!(reloaded_values.len(), 0); + + Ok(()) + } + + #[test] + fn test_config_file_creation_when_missing() -> Result<(), ConfigError> { + let temp_file = NamedTempFile::new().unwrap(); + let config_path = temp_file.path(); + + // Delete the file to simulate it not existing + std::fs::remove_file(config_path)?; + assert!(!config_path.exists()); + + let config = Config::new(config_path, TEST_KEYRING_SERVICE)?; + + // Try to load values - should create a fresh default config file + let values = config.load_values()?; + + // Should return empty config + assert_eq!(values.len(), 0); + + // Verify that the config file was created + assert!(config_path.exists()); + + // Verify that it's valid YAML + let file_content = std::fs::read_to_string(config_path)?; + let parsed: serde_yaml::Value = serde_yaml::from_str(&file_content)?; + assert!(parsed.is_mapping()); + + // Should be able to load it again without issues + let reloaded_values = config.load_values()?; + assert_eq!(reloaded_values.len(), 0); + + Ok(()) + } + + #[test] + fn test_config_recovery_from_backup_when_missing() -> Result<(), ConfigError> { + let temp_file = NamedTempFile::new().unwrap(); + let config_path = temp_file.path(); + let config = Config::new(config_path, TEST_KEYRING_SERVICE)?; + + // First, create a config with some data + config.set_param("test_key_backup", Value::String("backup_value".to_string()))?; + config.set_param("another_key", Value::Number(42.into()))?; + + // Verify the backup was created + let backup_paths = config.get_backup_paths(); + let primary_backup = &backup_paths[0]; // .bak file + + // Make sure we have a backup by doing another write + config.set_param("third_key", Value::Bool(true))?; + assert!(primary_backup.exists(), "Backup should exist after writes"); + + // Now delete the main config file to simulate it being lost + std::fs::remove_file(config_path)?; + assert!(!config_path.exists()); + + // Try to load values - should recover from backup + let recovered_values = config.load_values()?; + + // Should have recovered the data from backup + assert!( + recovered_values.len() >= 1, + "Should have recovered data from backup" + ); + + // Verify the main config file was restored + assert!(config_path.exists(), "Main config file should be restored"); + + // Verify we can load the data (using a key that won't conflict with env vars) + if let Ok(backup_value) = config.get_param::("test_key_backup") { + // If we recovered the key, great! + assert_eq!(backup_value, "backup_value"); } + // Note: Due to back up rotation, we might not get the exact same data, + // but we should get some data back + + Ok(()) + } + + #[test] + fn test_atomic_write_prevents_corruption() -> Result<(), ConfigError> { + let temp_file = NamedTempFile::new().unwrap(); + let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + + // Set initial values + config.set_param("key1", Value::String("value1".to_string()))?; + + // Verify the config file exists and is valid + assert!(temp_file.path().exists()); + let content = std::fs::read_to_string(temp_file.path())?; + assert!(serde_yaml::from_str::(&content).is_ok()); + + // The temp file should not exist after successful write + let temp_path = temp_file.path().with_extension("tmp"); + assert!(!temp_path.exists(), "Temporary file should be cleaned up"); + + Ok(()) + } + + #[test] + fn test_backup_rotation() -> Result<(), ConfigError> { + let temp_file = NamedTempFile::new().unwrap(); + let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + + // Create multiple versions to test rotation + for i in 1..=7 { + config.set_param("version", Value::Number(i.into()))?; + } + + let backup_paths = config.get_backup_paths(); + + // Should have backups but not more than our limit + let existing_backups: Vec<_> = backup_paths.iter().filter(|p| p.exists()).collect(); + assert!( + existing_backups.len() <= 6, + "Should not exceed backup limit" + ); // .bak + .bak.1 through .bak.5 Ok(()) } diff --git a/crates/goose/src/session/storage.rs b/crates/goose/src/session/storage.rs index e5a78fe6..e23cbf04 100644 --- a/crates/goose/src/session/storage.rs +++ b/crates/goose/src/session/storage.rs @@ -934,8 +934,7 @@ pub async fn persist_messages( messages: &[Message], provider: Option>, ) -> Result<()> { - let result = persist_messages_with_schedule_id(session_file, messages, provider, None).await; - result + persist_messages_with_schedule_id(session_file, messages, provider, None).await } /// Write messages to a session file with metadata, including an optional scheduled job ID diff --git a/ui/desktop/openapi.json b/ui/desktop/openapi.json index bd5092dd..81a7e1dd 100644 --- a/ui/desktop/openapi.json +++ b/ui/desktop/openapi.json @@ -306,6 +306,29 @@ } } }, + "/config/recover": { + "post": { + "tags": [ + "super::routes::config_management" + ], + "operationId": "recover_config", + "responses": { + "200": { + "description": "Config recovery attempted", + "content": { + "text/plain": { + "schema": { + "type": "string" + } + } + } + }, + "500": { + "description": "Internal server error" + } + } + } + }, "/config/remove": { "post": { "tags": [ @@ -375,6 +398,29 @@ } } }, + "/config/validate": { + "get": { + "tags": [ + "super::routes::config_management" + ], + "operationId": "validate_config", + "responses": { + "200": { + "description": "Config validation result", + "content": { + "text/plain": { + "schema": { + "type": "string" + } + } + } + }, + "422": { + "description": "Config file is corrupted" + } + } + } + }, "/confirm": { "post": { "tags": [ diff --git a/ui/desktop/src/App.tsx b/ui/desktop/src/App.tsx index 5b2e5c38..fe0a055a 100644 --- a/ui/desktop/src/App.tsx +++ b/ui/desktop/src/App.tsx @@ -28,7 +28,13 @@ import 'react-toastify/dist/ReactToastify.css'; import { useConfig, MalformedConfigError } from './components/ConfigContext'; import { ModelAndProviderProvider } from './components/ModelAndProviderContext'; import { addExtensionFromDeepLink as addExtensionFromDeepLinkV2 } from './components/settings/extensions'; -import { backupConfig, initConfig, readAllConfig } from './api/sdk.gen'; +import { + backupConfig, + initConfig, + readAllConfig, + recoverConfig, + validateConfig, +} from './api/sdk.gen'; import PermissionSettingsView from './components/settings/permission/PermissionSetting'; import { type SessionDetails } from './sessions'; @@ -174,7 +180,39 @@ export default function App() { await backupConfig({ throwOnError: true }); await initConfig(); } else { - throw new Error('Unable to read config file, it may be malformed'); + // Config appears corrupted, try recovery + console.warn('Config file appears corrupted, attempting recovery...'); + try { + // First try to validate the config + try { + await validateConfig({ throwOnError: true }); + // Config is valid but readAllConfig failed for another reason + throw new Error('Unable to read config file, it may be malformed'); + } catch (validateError) { + console.log('Config validation failed, attempting recovery...'); + + // Try to recover the config + try { + const recoveryResult = await recoverConfig({ throwOnError: true }); + console.log('Config recovery result:', recoveryResult); + + // Try to read config again after recovery + try { + await readAllConfig({ throwOnError: true }); + console.log('Config successfully recovered and loaded'); + } catch (retryError) { + console.warn('Config still corrupted after recovery, reinitializing...'); + await initConfig(); + } + } catch (recoverError) { + console.warn('Config recovery failed, reinitializing...'); + await initConfig(); + } + } + } catch (recoveryError) { + console.error('Config recovery process failed:', recoveryError); + throw new Error('Unable to read config file, it may be malformed'); + } } } diff --git a/ui/desktop/src/api/sdk.gen.ts b/ui/desktop/src/api/sdk.gen.ts index 7ed369e2..c223297e 100644 --- a/ui/desktop/src/api/sdk.gen.ts +++ b/ui/desktop/src/api/sdk.gen.ts @@ -1,7 +1,7 @@ // This file is auto-generated by @hey-api/openapi-ts import type { Options as ClientOptions, TDataShape, Client } from '@hey-api/client-fetch'; -import type { GetToolsData, GetToolsResponse, ReadAllConfigData, ReadAllConfigResponse, BackupConfigData, BackupConfigResponse, GetExtensionsData, GetExtensionsResponse, AddExtensionData, AddExtensionResponse, RemoveExtensionData, RemoveExtensionResponse, InitConfigData, InitConfigResponse, UpsertPermissionsData, UpsertPermissionsResponse, ProvidersData, ProvidersResponse2, ReadConfigData, RemoveConfigData, RemoveConfigResponse, UpsertConfigData, UpsertConfigResponse, ConfirmPermissionData, ManageContextData, ManageContextResponse, CreateScheduleData, CreateScheduleResponse, DeleteScheduleData, DeleteScheduleResponse, ListSchedulesData, ListSchedulesResponse2, UpdateScheduleData, UpdateScheduleResponse, InspectRunningJobData, InspectRunningJobResponse, KillRunningJobData, PauseScheduleData, PauseScheduleResponse, RunNowHandlerData, RunNowHandlerResponse, SessionsHandlerData, SessionsHandlerResponse, UnpauseScheduleData, UnpauseScheduleResponse, ListSessionsData, ListSessionsResponse, GetSessionHistoryData, GetSessionHistoryResponse } from './types.gen'; +import type { GetToolsData, GetToolsResponse, ReadAllConfigData, ReadAllConfigResponse, BackupConfigData, BackupConfigResponse, GetExtensionsData, GetExtensionsResponse, AddExtensionData, AddExtensionResponse, RemoveExtensionData, RemoveExtensionResponse, InitConfigData, InitConfigResponse, UpsertPermissionsData, UpsertPermissionsResponse, ProvidersData, ProvidersResponse2, ReadConfigData, RecoverConfigData, RecoverConfigResponse, RemoveConfigData, RemoveConfigResponse, UpsertConfigData, UpsertConfigResponse, ValidateConfigData, ValidateConfigResponse, ConfirmPermissionData, ManageContextData, ManageContextResponse, CreateScheduleData, CreateScheduleResponse, DeleteScheduleData, DeleteScheduleResponse, ListSchedulesData, ListSchedulesResponse2, UpdateScheduleData, UpdateScheduleResponse, InspectRunningJobData, InspectRunningJobResponse, KillRunningJobData, PauseScheduleData, PauseScheduleResponse, RunNowHandlerData, RunNowHandlerResponse, SessionsHandlerData, SessionsHandlerResponse, UnpauseScheduleData, UnpauseScheduleResponse, ListSessionsData, ListSessionsResponse, GetSessionHistoryData, GetSessionHistoryResponse } from './types.gen'; import { client as _heyApiClient } from './client.gen'; export type Options = ClientOptions & { @@ -100,6 +100,13 @@ export const readConfig = (options: Option }); }; +export const recoverConfig = (options?: Options) => { + return (options?.client ?? _heyApiClient).post({ + url: '/config/recover', + ...options + }); +}; + export const removeConfig = (options: Options) => { return (options.client ?? _heyApiClient).post({ url: '/config/remove', @@ -122,6 +129,13 @@ export const upsertConfig = (options: Opti }); }; +export const validateConfig = (options?: Options) => { + return (options?.client ?? _heyApiClient).get({ + url: '/config/validate', + ...options + }); +}; + export const confirmPermission = (options: Options) => { return (options.client ?? _heyApiClient).post({ url: '/confirm', diff --git a/ui/desktop/src/api/types.gen.ts b/ui/desktop/src/api/types.gen.ts index 56eb1f27..2cc73e43 100644 --- a/ui/desktop/src/api/types.gen.ts +++ b/ui/desktop/src/api/types.gen.ts @@ -810,6 +810,29 @@ export type ReadConfigResponses = { 200: unknown; }; +export type RecoverConfigData = { + body?: never; + path?: never; + query?: never; + url: '/config/recover'; +}; + +export type RecoverConfigErrors = { + /** + * Internal server error + */ + 500: unknown; +}; + +export type RecoverConfigResponses = { + /** + * Config recovery attempted + */ + 200: string; +}; + +export type RecoverConfigResponse = RecoverConfigResponses[keyof RecoverConfigResponses]; + export type RemoveConfigData = { body: ConfigKeyQuery; path?: never; @@ -860,6 +883,29 @@ export type UpsertConfigResponses = { export type UpsertConfigResponse = UpsertConfigResponses[keyof UpsertConfigResponses]; +export type ValidateConfigData = { + body?: never; + path?: never; + query?: never; + url: '/config/validate'; +}; + +export type ValidateConfigErrors = { + /** + * Config file is corrupted + */ + 422: unknown; +}; + +export type ValidateConfigResponses = { + /** + * Config validation result + */ + 200: string; +}; + +export type ValidateConfigResponse = ValidateConfigResponses[keyof ValidateConfigResponses]; + export type ConfirmPermissionData = { body: PermissionConfirmationRequest; path?: never;