diff --git a/docs/administration/server-configuration.mdx b/docs/administration/server-configuration.mdx index 30fbe39e9..0ce3714c6 100644 --- a/docs/administration/server-configuration.mdx +++ b/docs/administration/server-configuration.mdx @@ -10,7 +10,7 @@ description: "Server-owned settings.toml sections, CLI overrides, and environmen On a same-machine setup, the CLI and server share one `settings.toml`. On a remote deployment, the server machine has its own `settings.toml`, and the client machine keeps a separate local `settings.toml` for CLI-only values such as `[cli.target]`. -Legacy `server.toml`, `user.toml`, and `cli.toml` are ignored with a warning. Rename them to `settings.toml`. +Fabro only reads `settings.toml`. Older `server.toml`, `user.toml`, and `cli.toml` filenames are no longer part of the supported config surface. ### Which sections are server-owned diff --git a/docs/administration/troubleshooting.mdx b/docs/administration/troubleshooting.mdx index 89142fc64..8e818eebb 100644 --- a/docs/administration/troubleshooting.mdx +++ b/docs/administration/troubleshooting.mdx @@ -14,7 +14,7 @@ fabro doctor --server https://fabro.example.com/api/v1 ``` It checks: -- Local user config and legacy `~/.fabro/.env` warnings +- Local user config and storage directory health - Server-reported LLM provider connectivity - GitHub App, sandbox, and Brave Search credentials - Server authentication and crypto configuration diff --git a/docs/reference/user-configuration.mdx b/docs/reference/user-configuration.mdx index c9c29f9bf..3c426ba56 100644 --- a/docs/reference/user-configuration.mdx +++ b/docs/reference/user-configuration.mdx @@ -8,7 +8,7 @@ Fabro loads machine defaults from `~/.fabro/settings.toml`. The file is optional On a same-machine setup, the CLI and server both read this file. On a remote setup, each machine has its own `settings.toml` and reads the sections relevant to that process. -Legacy `cli.toml`, `user.toml`, and `server.toml` are ignored with a warning. Rename them to `settings.toml`. +Fabro only reads `settings.toml`. Older `cli.toml`, `user.toml`, and `server.toml` filenames are no longer part of the supported config surface. ## File location diff --git a/lib/crates/fabro-cli/src/commands/config/mod.rs b/lib/crates/fabro-cli/src/commands/config/mod.rs index 90b0a8586..83b0a29a2 100644 --- a/lib/crates/fabro-cli/src/commands/config/mod.rs +++ b/lib/crates/fabro-cli/src/commands/config/mod.rs @@ -21,10 +21,8 @@ fn config_layers( Some(path) => workflow_and_project_layers(path, cwd)?, None => (SettingsLayer::default(), load_settings_project(cwd)?), }; - let user_layer = user_config::settings_layer_with_config_and_storage_dir( - Some(ctx.base_config_path()), - None, - )?; + let user_layer = + user_config::load_settings_with_config_and_storage_dir(Some(ctx.base_config_path()), None)?; Ok(EffectiveSettingsLayers::new( SettingsLayer::default(), workflow_layer, diff --git a/lib/crates/fabro-cli/src/commands/doctor.rs b/lib/crates/fabro-cli/src/commands/doctor.rs index 894dc4da3..58eeb83ae 100644 --- a/lib/crates/fabro-cli/src/commands/doctor.rs +++ b/lib/crates/fabro-cli/src/commands/doctor.rs @@ -2,11 +2,7 @@ use std::path::{Path, PathBuf}; use anyhow::Result; use fabro_api::types as api_types; -use fabro_config::legacy_env; -use fabro_config::user::{ - active_settings_path, legacy_old_user_config_path, legacy_server_config_path, - legacy_user_config_path, -}; +use fabro_config::user::active_settings_path; use fabro_types::settings::CliSettings; use fabro_types::settings::cli::{CliLayer, OutputFormat}; pub(crate) use fabro_util::check_report::{ @@ -22,12 +18,9 @@ use crate::command_context::CommandContext; use crate::shared::print_json_pretty; use crate::user_config; -pub(crate) fn check_config( - settings_path: Option, - legacy_paths: &[PathBuf], -) -> CheckResult { - match (settings_path, legacy_paths.is_empty()) { - (Some(path), true) => { +pub(crate) fn check_config(settings_path: Option) -> CheckResult { + match settings_path { + Some(path) => { let display = contract_tilde(&path); CheckResult { name: "Configuration".to_string(), @@ -40,44 +33,7 @@ pub(crate) fn check_config( remediation: None, } } - (Some(path), false) => { - let display = contract_tilde(&path); - CheckResult { - name: "Configuration".to_string(), - status: CheckStatus::Warning, - summary: display.display().to_string(), - details: std::iter::once(CheckDetail::new(format!( - "Loaded from {}", - display.display() - ))) - .chain(legacy_paths.iter().map(|legacy| { - let legacy_display = contract_tilde(legacy); - CheckDetail::new(format!( - "Ignoring legacy config file {}", - legacy_display.display() - )) - })) - .collect(), - remediation: Some("Delete or rename legacy config files".to_string()), - } - } - (None, false) => CheckResult { - name: "Configuration".to_string(), - status: CheckStatus::Warning, - summary: "legacy config files ignored".to_string(), - details: legacy_paths - .iter() - .map(|legacy| { - CheckDetail::new(format!("Found legacy config file {}", legacy.display())) - }) - .chain(std::iter::once(CheckDetail::new( - "Rename one to ~/.fabro/settings.toml or create a new settings.toml" - .to_string(), - ))) - .collect(), - remediation: Some("Create ~/.fabro/settings.toml".to_string()), - }, - (None, true) => CheckResult { + None => CheckResult { name: "Configuration".to_string(), status: CheckStatus::Warning, summary: "no settings config file found".to_string(), @@ -89,22 +45,6 @@ pub(crate) fn check_config( } } -fn check_legacy_env(path: Option) -> Option { - path.map(|path| { - let display = contract_tilde(&path); - CheckResult { - name: "Legacy .env".to_string(), - status: CheckStatus::Warning, - summary: "legacy secrets file detected".to_string(), - details: vec![CheckDetail::new(format!( - "{} is no longer read by fabro", - display.display() - ))], - remediation: Some("Re-enter credentials with `fabro provider login`.".to_string()), - } - }) -} - #[derive(Debug, Clone, PartialEq, Eq)] struct StorageDirStatus { path: PathBuf, @@ -284,37 +224,20 @@ pub(crate) async fn run_doctor( }; let settings_config_path = active_settings_path(None); - let legacy_config_paths = [ - legacy_user_config_path(), - legacy_old_user_config_path(), - legacy_server_config_path(), - ] - .into_iter() - .flatten() - .filter(|path| path.exists()) - .collect::>(); - let legacy_env_path = { - let p = legacy_env::legacy_env_file_path(); - p.exists().then_some(p) - }; let settings = user_config::load_settings().unwrap_or_default(); - let storage_dir_path = user_config::storage_dir(&settings) - .unwrap_or_else(|_| fabro_util::Home::from_env().storage_dir()); + let storage_dir_path = + user_config::storage_dir(&settings).unwrap_or_else(|_| user_config::default_storage_dir()); let storage_dir = probe_storage_dir(&storage_dir_path); - let mut local_checks = vec![ + let local_checks = vec![ check_config( settings_config_path .exists() .then_some(settings_config_path), - &legacy_config_paths, ), check_storage_dir(&storage_dir), ]; - if let Some(legacy_env_check) = check_legacy_env(legacy_env_path) { - local_checks.push(legacy_env_check); - } let mut report = CheckReport { title: "Fabro Doctor".to_string(), @@ -470,45 +393,18 @@ mod tests { #[test] fn check_config_pass_with_path() { - let result = check_config(Some(PathBuf::from("/home/user/.fabro/settings.toml")), &[]); + let result = check_config(Some(PathBuf::from("/home/user/.fabro/settings.toml"))); assert_eq!(result.status, CheckStatus::Pass); assert!(result.summary.contains(".fabro/settings.toml")); } #[test] fn check_config_warning_without_path() { - let result = check_config(None, &[]); + let result = check_config(None); assert_eq!(result.status, CheckStatus::Warning); assert!(result.remediation.is_some()); } - #[test] - fn check_config_warning_for_legacy_only_path() { - let result = check_config(None, &[PathBuf::from("/home/user/.fabro/cli.toml")]); - assert_eq!(result.status, CheckStatus::Warning); - assert!(result.summary.contains("legacy")); - } - - #[test] - fn check_legacy_env_warning_when_present() { - let result = check_legacy_env(Some(PathBuf::from("/home/user/.fabro/.env"))); - assert_eq!( - result.as_ref().map(|check| check.status), - Some(CheckStatus::Warning) - ); - assert!( - result - .as_ref() - .is_some_and(|check| check.summary.contains("legacy secrets file")) - ); - } - - #[test] - fn check_legacy_env_is_omitted_when_absent() { - let result = check_legacy_env(None); - assert!(result.is_none()); - } - // -- check_storage_dir -- #[test] diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 5a6b44d19..babf6b928 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -16,8 +16,8 @@ use dialoguer::theme::ColorfulTheme; use dialoguer::{MultiSelect, Select}; use fabro_api::types::{CreateSecretRequest, SecretType as ApiSecretType}; use fabro_auth::{AuthCredential, AuthMethod, codex_oauth_config, credential_id_for}; -use fabro_config::user::{SETTINGS_CONFIG_FILENAME, legacy_default_storage_root}; -use fabro_config::{ResolveError, Storage, envfile, legacy_env}; +use fabro_config::user::{SETTINGS_CONFIG_FILENAME, default_storage_dir}; +use fabro_config::{ResolveError, Storage, envfile}; use fabro_install::{ InstallListenConfig, generate_jwt_keypair, merge_server_settings as merge_server_settings_impl, write_github_app_settings, write_token_settings, @@ -1166,7 +1166,9 @@ async fn persist_install_outputs( settings_write, server_was_running, |path| Box::pin(server_client::connect_api_client(path)), - |path, timeout| Box::pin(stop::stop_server(path, timeout)), + |path, timeout| { + Box::pin(async move { stop::stop_server(path, timeout).await.unwrap_or(false) }) + }, ) .await } @@ -1355,7 +1357,9 @@ async fn restart_server_after_install( restart_server_after_install_with( storage_dir, config_path, - |path, timeout| Box::pin(stop::stop_server(path, timeout)), + |path, timeout| { + Box::pin(async move { stop::stop_server(path, timeout).await.unwrap_or(false) }) + }, |storage_dir, config_path| { Box::pin(start::ensure_server_running_for_storage( storage_dir, @@ -1478,9 +1482,9 @@ async fn run_install_github_inner( let storage_dir = user_config::storage_dir(&parsed_settings).unwrap_or_else(|_| { args.storage_dir .clone_path() - .unwrap_or_else(|| legacy_default_storage_root().join("storage")) + .unwrap_or_else(default_storage_dir) }); - let server_was_running = record::active_server_record(&storage_dir).is_some(); + let server_was_running = record::active_server_record(&storage_dir)?.is_some(); let mut doc: toml::Value = toml::from_str(&existing_config_contents) .context("failed to parse existing settings.toml")?; @@ -1623,7 +1627,7 @@ async fn run_install_inner( let emoji = console::Emoji("⚒️ ", ""); let cli_settings = user_config::load_settings_with_storage_dir(args.storage_dir.as_deref())?; let storage_dir = user_config::storage_dir(&cli_settings)?; - let server_was_running = record::active_server_record(&storage_dir).is_some(); + let server_was_running = record::active_server_record(&storage_dir)?.is_some(); let fabro_dir = fabro_util::Home::from_env().root().to_path_buf(); let config_path = fabro_dir.join(SETTINGS_CONFIG_FILENAME); let existing_config_contents = std::fs::read_to_string(&config_path).ok(); @@ -1656,18 +1660,6 @@ async fn run_install_inner( std::fs::create_dir_all(&fabro_dir) .with_context(|| format!("creating fabro home directory {}", fabro_dir.display()))?; - { - let env_path = legacy_env::legacy_env_file_path(); - if env_path.exists() { - fabro_util::printerr!( - printer, - " Warning: {} is no longer read by fabro server. This install will persist runtime secrets in server.env and workflow-visible credentials in the vault instead.", - env_path.display() - ); - fabro_util::printerr!(printer, ""); - } - } - let facts = InstallFacts { codex_detected: detect_binary_on_path("codex").await, }; diff --git a/lib/crates/fabro-cli/src/commands/provider/login.rs b/lib/crates/fabro-cli/src/commands/provider/login.rs index 9654145a4..9445e6bac 100644 --- a/lib/crates/fabro-cli/src/commands/provider/login.rs +++ b/lib/crates/fabro-cli/src/commands/provider/login.rs @@ -1,7 +1,6 @@ use anyhow::Result; use fabro_api::types; use fabro_auth::credential_id_for; -use fabro_config::legacy_env; use fabro_types::settings::CliSettings; use fabro_types::settings::cli::CliLayer; use fabro_util::printer::Printer; @@ -36,17 +35,6 @@ pub(super) async fn login_command( let credential_id = credential_id_for(&credential).map_err(anyhow::Error::msg)?; let value = serde_json::to_string(&credential)?; - { - let path = legacy_env::legacy_env_file_path(); - if path.exists() { - fabro_util::printerr!( - printer, - " Warning: {} is no longer read by fabro server. Re-enter credentials with `fabro provider login`.", - path.display() - ); - } - } - server .api() .create_secret() diff --git a/lib/crates/fabro-cli/src/commands/run/command.rs b/lib/crates/fabro-cli/src/commands/run/command.rs index 81add8adf..9d6d2e912 100644 --- a/lib/crates/fabro-cli/src/commands/run/command.rs +++ b/lib/crates/fabro-cli/src/commands/run/command.rs @@ -7,7 +7,7 @@ use fabro_util::terminal::Styles; use crate::args::RunArgs; use crate::command_context::CommandContext; use crate::shared::print_json_pretty; -use crate::user_config::settings_layer_with_storage_dir; +use crate::user_config::load_settings_with_storage_dir; pub(crate) async fn execute( mut args: RunArgs, @@ -17,7 +17,7 @@ pub(crate) async fn execute( ) -> Result<()> { let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); let ctx = CommandContext::for_target(&args.target, printer, cli.clone(), cli_layer)?; - let cli_defaults = settings_layer_with_storage_dir(None)?; + let cli_defaults = load_settings_with_storage_dir(None)?; args.verbose = args.verbose || cli.output.verbosity == OutputVerbosity::Verbose; let quiet = args.detach; diff --git a/lib/crates/fabro-cli/src/commands/run/mod.rs b/lib/crates/fabro-cli/src/commands/run/mod.rs index 66a83c82c..da1515b2b 100644 --- a/lib/crates/fabro-cli/src/commands/run/mod.rs +++ b/lib/crates/fabro-cli/src/commands/run/mod.rs @@ -8,7 +8,7 @@ use crate::args::{AttachArgs, RunCommands, RunWorkerArgs, StartArgs}; use crate::command_context::CommandContext; use crate::server_runs::ServerSummaryLookup; use crate::shared::print_json_pretty; -use crate::user_config::settings_layer_with_storage_dir; +use crate::user_config::load_settings_with_storage_dir; pub(crate) mod attach; pub(crate) mod command; @@ -39,7 +39,7 @@ pub(crate) async fn dispatch( RunCommands::Run(args) => Box::pin(command::execute(args, cli, cli_layer, printer)).await, RunCommands::Create(args) => { let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); - let cli_defaults = settings_layer_with_storage_dir(None)?; + let cli_defaults = load_settings_with_storage_dir(None)?; let ctx = CommandContext::for_target(&args.target, printer, cli.clone(), cli_layer)?; let created_run = Box::pin(create::create_run( &ctx, diff --git a/lib/crates/fabro-cli/src/commands/server/foreground.rs b/lib/crates/fabro-cli/src/commands/server/foreground.rs index 390dc4a38..f2b536281 100644 --- a/lib/crates/fabro-cli/src/commands/server/foreground.rs +++ b/lib/crates/fabro-cli/src/commands/server/foreground.rs @@ -15,7 +15,7 @@ pub(crate) async fn execute( record_path: PathBuf, mut serve_args: ServeArgs, bind: BindRequest, - storage_dir: Option, + storage_dir: PathBuf, styles: &'static Styles, printer: Printer, ) -> Result<()> { @@ -35,22 +35,14 @@ pub(crate) async fn execute( None }; - let log_path = storage_dir.as_ref().map_or_else( - || { - record_path.parent().map_or_else( - || PathBuf::from("server.log"), - |parent| parent.join("server.log"), - ) - }, - |dir| Storage::new(dir).server_state().log_path(), - ); + let log_path = Storage::new(&storage_dir).server_state().log_path(); let dev_token_path = std::env::var_os("FABRO_DEV_TOKEN_PATH").map(PathBuf::from); let pid = std::process::id(); Box::pin(serve::serve_command( serve_args, styles, - storage_dir, + Some(storage_dir), move |resolved_bind| { record::write_server_record(&record_path, &record::ServerRecord { pid, diff --git a/lib/crates/fabro-cli/src/commands/server/mod.rs b/lib/crates/fabro-cli/src/commands/server/mod.rs index 77addbaae..eb8cfe6fb 100644 --- a/lib/crates/fabro-cli/src/commands/server/mod.rs +++ b/lib/crates/fabro-cli/src/commands/server/mod.rs @@ -9,7 +9,7 @@ use std::time::Duration; use anyhow::Result; use base64::Engine as _; use base64::engine::general_purpose::URL_SAFE_NO_PAD; -use fabro_config::user::{FABRO_CONFIG_ENV, active_settings_path, legacy_default_storage_root}; +use fabro_config::user::{FABRO_CONFIG_ENV, active_settings_path, default_storage_dir}; use fabro_server::bind::{self, Bind, BindRequest}; use fabro_server::install::{self, InstallAppState}; use fabro_server::serve::{self, ServeArgs}; @@ -27,6 +27,7 @@ use crate::user_config; pub(crate) async fn dispatch( command: ServerCommand, _globals: &GlobalArgs, + foreground_log_bootstrap: Option, printer: Printer, ) -> Result<()> { match command { @@ -62,6 +63,7 @@ pub(crate) async fn dispatch( foreground, serve_args, storage_dir, + foreground_log_bootstrap, styles, printer, )) @@ -73,8 +75,7 @@ pub(crate) async fn dispatch( }) => { let settings = user_config::load_settings_with_storage_dir(storage_dir.as_deref())?; let storage_dir = user_config::storage_dir(&settings)?; - stop::execute(&storage_dir, Duration::from_secs(timeout), printer).await; - Ok(()) + stop::execute(&storage_dir, Duration::from_secs(timeout), printer).await } ServerCommand::Restart(ServerRestartArgs { storage_dir, @@ -87,7 +88,7 @@ pub(crate) async fn dispatch( storage_dir.as_deref(), &serve_args, )? { - stop::stop_server(&bootstrap.storage_dir, Duration::from_secs(timeout)).await; + stop::stop_server(&bootstrap.storage_dir, Duration::from_secs(timeout)).await?; if serve_args.no_web { fabro_util::printerr!( printer, @@ -102,7 +103,7 @@ pub(crate) async fn dispatch( storage_dir.as_deref(), )?; let storage_dir = user_config::storage_dir(&settings)?; - stop::stop_server(&storage_dir, Duration::from_secs(timeout)).await; + stop::stop_server(&storage_dir, Duration::from_secs(timeout)).await?; let bind_addr = serve::resolve_bind_request_from_settings(&settings, serve_args.bind.as_deref())?; let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); @@ -111,6 +112,7 @@ pub(crate) async fn dispatch( foreground, serve_args, storage_dir, + foreground_log_bootstrap, styles, printer, )) @@ -136,6 +138,7 @@ pub(crate) async fn dispatch( .clone() .unwrap_or_else(|| user_config::active_settings_path(None)), ); + let storage_dir = user_config::storage_dir(&settings)?; let bind_addr = serve::resolve_bind_request_from_settings(&settings, serve_args.bind.as_deref())?; let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); @@ -146,7 +149,7 @@ pub(crate) async fn dispatch( ..serve_args }, bind_addr, - storage_dir.clone_path(), + storage_dir, styles, printer, )) @@ -181,10 +184,7 @@ fn maybe_install_bootstrap( None => default_install_bind_request(), }; - let storage_dir = storage_dir.map_or_else( - || legacy_default_storage_root().join("storage"), - std::path::Path::to_path_buf, - ); + let storage_dir = storage_dir.map_or_else(default_storage_dir, std::path::Path::to_path_buf); Ok(Some(InstallBootstrap { bind_request, diff --git a/lib/crates/fabro-cli/src/commands/server/record.rs b/lib/crates/fabro-cli/src/commands/server/record.rs index 560f9d9f7..1a6b15b5a 100644 --- a/lib/crates/fabro-cli/src/commands/server/record.rs +++ b/lib/crates/fabro-cli/src/commands/server/record.rs @@ -1,10 +1,10 @@ use std::path::{Path, PathBuf}; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail}; use chrono::{DateTime, Utc}; use fabro_config::Storage; -use fabro_config::user::legacy_default_storage_root; use fabro_server::bind::Bind; +use fabro_util::Home; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, Serialize, Deserialize)] @@ -50,9 +50,8 @@ fn server_record_path(storage_dir: &Path) -> PathBuf { } fn legacy_record_path(storage_dir: &Path) -> Option { - let default_storage_dir = legacy_default_storage_root().join("storage"); - if storage_dir == default_storage_dir { - Some(server_record_path(&legacy_default_storage_root())) + if storage_dir == fabro_config::user::default_storage_dir() { + Some(Home::from_env().root().join("server.json")) } else { None } @@ -71,14 +70,29 @@ fn active_server_record_at_path(path: PathBuf) -> Option { } } -pub(crate) fn active_server_record_details(storage_dir: &Path) -> Option { +pub(crate) fn active_server_record_details( + storage_dir: &Path, +) -> Result> { let primary_path = server_record_path(storage_dir); - active_server_record_at_path(primary_path) - .or_else(|| legacy_record_path(storage_dir).and_then(active_server_record_at_path)) + if let Some(active) = active_server_record_at_path(primary_path.clone()) { + return Ok(Some(active)); + } + + if let Some(legacy_path) = legacy_record_path(storage_dir) { + if active_server_record_at_path(legacy_path.clone()).is_some() { + bail!( + "Legacy server record {} is still active while current storage record {} is missing.\nStop the old daemon with a legacy Fabro CLI or manually clear the stale daemon before retrying.", + legacy_path.display(), + primary_path.display() + ); + } + } + + Ok(None) } -pub(crate) fn active_server_record(storage_dir: &Path) -> Option { - active_server_record_details(storage_dir).map(|active| active.record) +pub(crate) fn active_server_record(storage_dir: &Path) -> Result> { + Ok(active_server_record_details(storage_dir)?.map(|active| active.record)) } #[cfg(unix)] @@ -111,7 +125,7 @@ mod tests { ServerRecord { pid: std::process::id(), bind, - log_path: PathBuf::from("/tmp/server.log"), + log_path: PathBuf::from("/tmp/storage/logs/server.log"), dev_token_path: None, started_at: Utc::now(), } @@ -132,7 +146,7 @@ mod tests { #[test] fn active_server_record_returns_none_when_no_file() { let dir = tempfile::tempdir().unwrap(); - assert!(active_server_record(dir.path()).is_none()); + assert!(active_server_record(dir.path()).unwrap().is_none()); } #[test] @@ -143,7 +157,7 @@ mod tests { record.pid = u32::MAX; // definitely not alive write_server_record(&path, &record).unwrap(); - assert!(active_server_record(dir.path()).is_none()); - assert!(!path.exists()); // lazy cleanup removed file + assert!(active_server_record(dir.path()).unwrap().is_none()); + assert!(!path.exists()); } } diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index e7235e94d..1efecee7e 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -17,11 +17,17 @@ use tokio::time; use super::record; +pub(crate) struct ForegroundServerLogBootstrap { + #[expect(dead_code, reason = "held for its Drop to release the server lock")] + lock_file: std::fs::File, +} + pub(crate) async fn execute( bind: BindRequest, foreground: bool, mut serve_args: ServeArgs, storage_dir: PathBuf, + foreground_log_bootstrap: Option, styles: &'static Styles, printer: Printer, ) -> Result<()> { @@ -32,6 +38,8 @@ pub(crate) async fn execute( bind, serve_args, storage_dir, + foreground_log_bootstrap + .context("internal error: missing foreground server log bootstrap")?, styles, printer, )) @@ -49,6 +57,29 @@ pub(crate) async fn execute( } } +pub(crate) async fn prepare_foreground_server_log( + storage_dir: &Path, +) -> Result { + let lock_file = acquire_lock(storage_dir).await?; + if let Some(existing) = record::active_server_record(storage_dir)? { + bail!( + "Server already running (pid {}) on {}", + existing.pid, + existing.bind + ); + } + + let log_path = Storage::new(storage_dir).server_state().log_path(); + if let Some(parent) = log_path.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("creating log directory {}", parent.display()))?; + } + std::fs::File::create(&log_path) + .with_context(|| format!("creating server log file {}", log_path.display()))?; + + Ok(ForegroundServerLogBootstrap { lock_file }) +} + pub(crate) async fn ensure_server_running_for_storage( storage_dir: &Path, config_path: &Path, @@ -80,7 +111,7 @@ async fn ensure_server_running_with_bind( config_path: &Path, storage_dir: &Path, ) -> Result { - if let Some(existing) = record::active_server_record(storage_dir) { + if let Some(existing) = record::active_server_record(storage_dir)? { if bind_request .as_ref() .is_none_or(|requested| bind_matches_request(&existing.bind, requested)) @@ -124,7 +155,7 @@ async fn ensure_server_running_with_bind( ) .await { - Ok(()) => record::active_server_record(storage_dir) + Ok(()) => record::active_server_record(storage_dir)? .map(|server| server.bind) .ok_or_else(|| { anyhow!( @@ -133,7 +164,7 @@ async fn ensure_server_running_with_bind( ) }), Err(err) => { - if let Some(existing) = record::active_server_record(storage_dir) { + if let Some(existing) = record::active_server_record(storage_dir)? { Ok(existing.bind) } else { Err(err) @@ -258,6 +289,7 @@ async fn execute_foreground( bind: BindRequest, serve_args: ServeArgs, storage_dir: PathBuf, + _log_bootstrap: ForegroundServerLogBootstrap, styles: &'static Styles, printer: Printer, ) -> Result<()> { @@ -282,17 +314,6 @@ async fn execute_foreground( }, ); - let lock_file = acquire_lock(&storage_dir).await?; - let _lock_file = lock_file; // keep alive for the duration - - if let Some(existing) = record::active_server_record(&storage_dir) { - bail!( - "Server already running (pid {}) on {}", - existing.pid, - existing.bind - ); - } - let server_state = Storage::new(&storage_dir).server_state(); let record_path = server_state.record_path(); let log_path = server_state.log_path(); @@ -342,9 +363,9 @@ async fn execute_daemon( printer: Printer, ) -> Result<()> { let lock_file = acquire_lock(storage_dir).await?; - let _lock_file = lock_file; // keep alive until function returns + let _lock_file = lock_file; - if let Some(existing) = record::active_server_record(storage_dir) { + if let Some(existing) = record::active_server_record(storage_dir)? { if announce { bail!( "Server already running (pid {}) on {}", diff --git a/lib/crates/fabro-cli/src/commands/server/status.rs b/lib/crates/fabro-cli/src/commands/server/status.rs index 298328786..5c8536b3b 100644 --- a/lib/crates/fabro-cli/src/commands/server/status.rs +++ b/lib/crates/fabro-cli/src/commands/server/status.rs @@ -7,7 +7,7 @@ use fabro_util::printer::Printer; use super::record; pub(crate) fn execute(storage_dir: &Path, json: bool, printer: Printer) -> Result<()> { - let Some(record) = record::active_server_record(storage_dir) else { + let Some(record) = record::active_server_record(storage_dir)? else { if json { fabro_util::printout!(printer, r#"{{"status":"stopped"}}"#); } else { diff --git a/lib/crates/fabro-cli/src/commands/server/stop.rs b/lib/crates/fabro-cli/src/commands/server/stop.rs index b0460db03..dc83f55d1 100644 --- a/lib/crates/fabro-cli/src/commands/server/stop.rs +++ b/lib/crates/fabro-cli/src/commands/server/stop.rs @@ -1,15 +1,16 @@ use std::path::Path; use std::time::Duration; +use anyhow::Result; use fabro_server::bind::Bind; use fabro_util::printer::Printer; use tokio::time; use super::record; -pub(crate) async fn stop_server(storage_dir: &Path, timeout: Duration) -> bool { - let Some(active) = record::active_server_record_details(storage_dir) else { - return false; +pub(crate) async fn stop_server(storage_dir: &Path, timeout: Duration) -> Result { + let Some(active) = record::active_server_record_details(storage_dir)? else { + return Ok(false); }; let record = active.record; @@ -36,14 +37,15 @@ pub(crate) async fn stop_server(storage_dir: &Path, timeout: Duration) -> bool { let _ = std::fs::remove_file(path); } - true + Ok(true) } -pub(crate) async fn execute(storage_dir: &Path, timeout: Duration, printer: Printer) { - if !stop_server(storage_dir, timeout).await { +pub(crate) async fn execute(storage_dir: &Path, timeout: Duration, printer: Printer) -> Result<()> { + if !stop_server(storage_dir, timeout).await? { fabro_util::printerr!(printer, "Server is not running"); std::process::exit(1); } fabro_util::printerr!(printer, "Server stopped"); + Ok(()) } diff --git a/lib/crates/fabro-cli/src/commands/uninstall.rs b/lib/crates/fabro-cli/src/commands/uninstall.rs index 58ada15d6..500dc5d98 100644 --- a/lib/crates/fabro-cli/src/commands/uninstall.rs +++ b/lib/crates/fabro-cli/src/commands/uninstall.rs @@ -48,11 +48,14 @@ pub(crate) async fn run_uninstall( } let storage_dir = user_config::load_settings().map_or_else( - |_| home.storage_dir(), - |settings| user_config::storage_dir(&settings).unwrap_or_else(|_| home.storage_dir()), + |_| user_config::default_storage_dir(), + |settings| { + user_config::storage_dir(&settings) + .unwrap_or_else(|_| user_config::default_storage_dir()) + }, ); - let inventory = build_inventory(&home_root, &storage_dir); + let inventory = build_inventory(&home_root, &storage_dir)?; if !args.yes { if json { @@ -73,13 +76,13 @@ pub(crate) async fn run_uninstall( execute_uninstall(&inventory, json, printer).await } -fn build_inventory(home_root: &Path, storage_dir: &Path) -> Inventory { +fn build_inventory(home_root: &Path, storage_dir: &Path) -> Result { let home_size = dir_size(home_root); - let server_running = server::record::active_server_record_details(storage_dir).is_some(); + let server_running = server::record::active_server_record_details(storage_dir)?.is_some(); let shell_configs = find_shell_configs_with_sentinel(); let (binary_path, binary_is_managed) = resolve_binary(home_root); - Inventory { + Ok(Inventory { home_root: home_root.to_path_buf(), storage_dir: storage_dir.to_path_buf(), home_exists: true, @@ -88,7 +91,7 @@ fn build_inventory(home_root: &Path, storage_dir: &Path) -> Inventory { shell_configs, binary_path, binary_is_managed, - } + }) } fn dir_size(path: &Path) -> u64 { @@ -252,7 +255,7 @@ async fn execute_uninstall(inventory: &Inventory, json: bool, printer: Printer) // Unit 3a: Server stop if inventory.server_running { - server::stop::execute(&inventory.storage_dir, Duration::from_secs(5), printer).await; + server::stop::execute(&inventory.storage_dir, Duration::from_secs(5), printer).await?; result.server_stopped = true; } @@ -730,7 +733,7 @@ mod tests { create_fabro_home(&home_root); let storage_dir = home_root.join("storage"); - let inv = build_inventory(&home_root, &storage_dir); + let inv = build_inventory(&home_root, &storage_dir).unwrap(); assert!(inv.home_exists); assert!(inv.home_size > 0); @@ -745,7 +748,7 @@ mod tests { let home_root = tmp.path().join("fake-fabro-home"); create_fabro_home(&home_root); - let inv = build_inventory(&home_root, &home_root.join("storage")); + let inv = build_inventory(&home_root, &home_root.join("storage")).unwrap(); // shell_configs depends on the actual user's shell config files, // but we verify the field is populated (even if empty in CI/test) assert!(inv.shell_configs.is_empty() || !inv.shell_configs.is_empty()); diff --git a/lib/crates/fabro-cli/src/logging.rs b/lib/crates/fabro-cli/src/logging.rs index e787b6b13..984720677 100644 --- a/lib/crates/fabro-cli/src/logging.rs +++ b/lib/crates/fabro-cli/src/logging.rs @@ -1,18 +1,26 @@ +use std::fs::{File, OpenOptions}; use std::path::Path; use anyhow::{Context, Result}; use fabro_util::run_log; use tracing_appender::rolling; +use tracing_subscriber::fmt::writer::MakeWriter; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; use tracing_subscriber::{EnvFilter, fmt}; const LOG_RETENTION_DAYS: u32 = 7; +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum InternalLogSink { + Cli, + Server { path: std::path::PathBuf }, +} + pub(crate) fn init_tracing( debug: bool, config_log_level: Option<&str>, - log_prefix: &str, + sink: &InternalLogSink, ) -> Result<()> { let default_level = if debug { "debug" @@ -22,37 +30,28 @@ pub(crate) fn init_tracing( let filter = EnvFilter::try_from_env("FABRO_LOG").unwrap_or_else(|_| EnvFilter::new(default_level)); - let log_dir = fabro_util::Home::from_env().logs_dir(); + match sink { + InternalLogSink::Cli => { + let log_dir = fabro_util::Home::from_env().logs_dir(); - std::fs::create_dir_all(&log_dir) - .with_context(|| format!("Failed to create log directory: {}", log_dir.display()))?; + std::fs::create_dir_all(&log_dir).with_context(|| { + format!("Failed to create log directory: {}", log_dir.display()) + })?; - let file_appender = rolling::RollingFileAppender::builder() - .rotation(rolling::Rotation::DAILY) - .filename_prefix(log_prefix) - .filename_suffix("log") - .build(&log_dir) - .with_context(|| "Failed to create log file appender")?; + let file_appender = rolling::RollingFileAppender::builder() + .rotation(rolling::Rotation::DAILY) + .filename_prefix("cli") + .filename_suffix("log") + .build(&log_dir) + .with_context(|| "Failed to create log file appender")?; - cleanup_old_logs(&log_dir, log_prefix, LOG_RETENTION_DAYS); - - let run_log_writer = run_log::init(); - - tracing_subscriber::registry() - .with(filter) - .with( - fmt::layer() - .with_writer(file_appender) - .with_target(true) - .with_ansi(false), - ) - .with( - fmt::layer() - .with_writer(run_log_writer) - .with_target(true) - .with_ansi(false), - ) - .init(); + cleanup_old_logs(&log_dir, "cli", LOG_RETENTION_DAYS); + init_subscriber(filter, file_appender); + } + InternalLogSink::Server { path } => { + init_subscriber(filter, FixedFileAppender::open(path)?); + } + } Ok(()) } @@ -88,3 +87,50 @@ fn cleanup_old_logs(log_dir: &Path, prefix: &str, max_age_days: u32) { } } } + +fn init_subscriber(filter: EnvFilter, file_writer: W) +where + W: for<'writer> MakeWriter<'writer> + Send + Sync + 'static, +{ + let run_log_writer = run_log::init(); + + tracing_subscriber::registry() + .with(filter) + .with( + fmt::layer() + .with_writer(file_writer) + .with_target(true) + .with_ansi(false), + ) + .with( + fmt::layer() + .with_writer(run_log_writer) + .with_target(true) + .with_ansi(false), + ) + .init(); +} + +struct FixedFileAppender { + file: File, +} + +impl FixedFileAppender { + fn open(path: &Path) -> Result { + let file = OpenOptions::new() + .append(true) + .open(path) + .with_context(|| format!("Failed to open server log file: {}", path.display()))?; + Ok(Self { file }) + } +} + +impl<'writer> MakeWriter<'writer> for FixedFileAppender { + type Writer = File; + + fn make_writer(&'writer self) -> Self::Writer { + self.file + .try_clone() + .expect("fixed log file handle should be cloneable") + } +} diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 9c2903ce7..d1cf06709 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -25,7 +25,6 @@ use args::{ }; use clap::{CommandFactory, Parser}; use fabro_config::merge::combine_files; -use fabro_config::user::load_settings_config; use fabro_telemetry::{git, panic as tel_panic, sanitize, sender}; use fabro_types::settings::SettingsLayer; use fabro_types::settings::cli::OutputVerbosity; @@ -144,6 +143,10 @@ async fn main_inner() -> (String, Result<()>) { let cli_layer = global_args_cli_layer(&globals); let process_local_json = globals.json; let command_name = command.name().to_string(); + let pre_tracing_bootstrap = match pre_tracing_bootstrap(command.as_ref()).await { + Ok(bootstrap) => bootstrap, + Err(err) => return (command_name, Err(err)), + }; let user_settings = match user_config::load_settings() { Ok(settings) => settings, @@ -159,35 +162,15 @@ async fn main_inner() -> (String, Result<()>) { }; let printer = printer_from_verbosity(cli_settings.output.verbosity); - let config_log_level = if let Commands::Server(ServerNamespace { - command: - ServerCommand::Start(args::ServerStartArgs { - serve_args: args, .. - }) - | ServerCommand::Serve(args::ServerServeArgs { - serve_args: args, .. - }), - }) = command.as_ref() - { - match load_settings_config(args.config.as_deref()) { - Ok(layer) => layer - .server - .as_ref() - .and_then(|server| server.logging.as_ref()) - .and_then(|logging| logging.level.clone()), - Err(err) => return (command_name, Err(err.into())), - } - } else { - cli_settings.logging.level.clone() + let config_log_level = match &pre_tracing_bootstrap.sink { + logging::InternalLogSink::Cli => cli_settings.logging.level.clone(), + logging::InternalLogSink::Server { .. } => pre_tracing_bootstrap.config_log_level.clone(), }; - - let log_prefix = if command_name == "server start" || command_name == "server __serve" { - "server" - } else { - "cli" - }; - if let Err(err) = logging::init_tracing(globals.debug, config_log_level.as_deref(), log_prefix) - { + if let Err(err) = logging::init_tracing( + globals.debug, + config_log_level.as_deref(), + &pre_tracing_bootstrap.sink, + ) { fabro_util::printerr!( bootstrap_printer, "Warning: failed to initialize logging: {err:#}" @@ -195,6 +178,7 @@ async fn main_inner() -> (String, Result<()>) { } debug!(command = %command_name, "CLI command started"); + let foreground_server_log_bootstrap = pre_tracing_bootstrap.foreground_server_log_bootstrap; let upgrade_handle = if matches!( command.as_ref(), @@ -254,7 +238,13 @@ async fn main_inner() -> (String, Result<()>) { commands::model::execute(command, &cli_settings, &cli_layer, printer).await?; } Commands::Server(ns) => { - Box::pin(commands::server::dispatch(ns.command, &globals, printer)).await?; + Box::pin(commands::server::dispatch( + ns.command, + &globals, + foreground_server_log_bootstrap, + printer, + )) + .await?; } Commands::Doctor(args) => { let verbose = @@ -409,6 +399,90 @@ async fn main_inner() -> (String, Result<()>) { (command_name, result) } +struct PreTracingBootstrap { + sink: logging::InternalLogSink, + config_log_level: Option, + foreground_server_log_bootstrap: Option, +} + +impl PreTracingBootstrap { + fn cli() -> Self { + Self { + sink: logging::InternalLogSink::Cli, + config_log_level: None, + foreground_server_log_bootstrap: None, + } + } +} + +async fn pre_tracing_bootstrap(command: &Commands) -> Result { + match command { + Commands::Server(ServerNamespace { + command: ServerCommand::Start(args), + }) if args.foreground => { + prepare_server_bootstrap( + args.serve_args.config.as_deref(), + args.storage_dir.as_deref(), + true, + ) + .await + } + Commands::Server(ServerNamespace { + command: ServerCommand::Restart(args), + }) if args.foreground => { + prepare_server_bootstrap( + args.serve_args.config.as_deref(), + args.storage_dir.as_deref(), + true, + ) + .await + } + Commands::Server(ServerNamespace { + command: ServerCommand::Serve(args), + }) => { + prepare_server_bootstrap( + args.serve_args.config.as_deref(), + args.storage_dir.as_deref(), + false, + ) + .await + } + _ => Ok(PreTracingBootstrap::cli()), + } +} + +async fn prepare_server_bootstrap( + config_path: Option<&std::path::Path>, + storage_dir: Option<&std::path::Path>, + foreground: bool, +) -> Result { + let settings = + user_config::load_settings_with_config_and_storage_dir(config_path, storage_dir)?; + let storage_dir = user_config::storage_dir(&settings)?; + let storage = fabro_config::Storage::new(&storage_dir); + let foreground_server_log_bootstrap = if foreground { + Some(commands::server::start::prepare_foreground_server_log(&storage_dir).await?) + } else { + None + }; + + Ok(PreTracingBootstrap { + sink: logging::InternalLogSink::Server { + path: storage.server_state().log_path(), + }, + config_log_level: server_config_log_level(&settings), + foreground_server_log_bootstrap, + }) +} + +fn server_config_log_level(settings: &SettingsLayer) -> Option { + settings + .server + .as_ref() + .and_then(|server| server.logging.as_ref()) + .and_then(|logging| logging.level.clone()) +} + #[cfg(test)] mod tests { use args::{ @@ -418,6 +492,37 @@ mod tests { use super::*; + fn runtime() -> tokio::runtime::Runtime { + tokio::runtime::Runtime::new().expect("runtime should build") + } + + fn write_test_settings(path: &std::path::Path) { + std::fs::write( + path, + r#" +_version = 1 + +[server.logging] +level = "warn" +"#, + ) + .unwrap(); + } + + #[test] + fn pre_tracing_bootstrap_uses_cli_sink_for_normal_cli_command() { + let cli = Cli::try_parse_from(["fabro", "uninstall"]).expect("should parse"); + let command = cli.command.as_deref().unwrap(); + + let bootstrap = runtime() + .block_on(pre_tracing_bootstrap(command)) + .expect("bootstrap should resolve"); + + assert_eq!(bootstrap.sink, logging::InternalLogSink::Cli); + assert!(bootstrap.config_log_level.is_none()); + assert!(bootstrap.foreground_server_log_bootstrap.is_none()); + } + #[test] fn parse_provider_login_openai() { let cli = Cli::try_parse_from(["fabro", "provider", "login", "--provider", "openai"]) @@ -523,6 +628,157 @@ mod tests { } } + #[test] + fn pre_tracing_bootstrap_uses_server_sink_for_server_start_foreground() { + let storage_dir = tempfile::tempdir().unwrap(); + let config_dir = tempfile::tempdir().unwrap(); + let config_path = config_dir.path().join("settings.toml"); + write_test_settings(&config_path); + + let cli = Cli::try_parse_from([ + "fabro", + "server", + "start", + "--foreground", + "--storage-dir", + storage_dir.path().to_str().unwrap(), + "--config", + config_path.to_str().unwrap(), + ]) + .expect("should parse"); + let command = cli.command.as_deref().unwrap(); + + let bootstrap = runtime() + .block_on(pre_tracing_bootstrap(command)) + .expect("bootstrap should resolve"); + + assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { + path: storage_dir.path().join("logs").join("server.log"), + }); + assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); + assert!(bootstrap.foreground_server_log_bootstrap.is_some()); + } + + #[test] + fn pre_tracing_bootstrap_uses_server_sink_for_server_restart_foreground() { + let storage_dir = tempfile::tempdir().unwrap(); + let config_dir = tempfile::tempdir().unwrap(); + let config_path = config_dir.path().join("settings.toml"); + write_test_settings(&config_path); + + let cli = Cli::try_parse_from([ + "fabro", + "server", + "restart", + "--foreground", + "--storage-dir", + storage_dir.path().to_str().unwrap(), + "--config", + config_path.to_str().unwrap(), + ]) + .expect("should parse"); + let command = cli.command.as_deref().unwrap(); + + let bootstrap = runtime() + .block_on(pre_tracing_bootstrap(command)) + .expect("bootstrap should resolve"); + + assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { + path: storage_dir.path().join("logs").join("server.log"), + }); + assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); + assert!(bootstrap.foreground_server_log_bootstrap.is_some()); + } + + #[test] + fn pre_tracing_bootstrap_uses_server_sink_for_server_serve() { + let storage_dir = tempfile::tempdir().unwrap(); + let config_dir = tempfile::tempdir().unwrap(); + let config_path = config_dir.path().join("settings.toml"); + write_test_settings(&config_path); + let record_path = storage_dir.path().join("server.json"); + + let cli = Cli::try_parse_from([ + "fabro", + "server", + "__serve", + "--storage-dir", + storage_dir.path().to_str().unwrap(), + "--record-path", + record_path.to_str().unwrap(), + "--config", + config_path.to_str().unwrap(), + ]) + .expect("should parse"); + let command = cli.command.as_deref().unwrap(); + + let bootstrap = runtime() + .block_on(pre_tracing_bootstrap(command)) + .expect("bootstrap should resolve"); + + assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { + path: storage_dir.path().join("logs").join("server.log"), + }); + assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); + assert!(bootstrap.foreground_server_log_bootstrap.is_none()); + } + + #[test] + fn pre_tracing_bootstrap_uses_cli_sink_for_server_start_daemon_wrapper() { + let storage_dir = tempfile::tempdir().unwrap(); + let config_dir = tempfile::tempdir().unwrap(); + let config_path = config_dir.path().join("settings.toml"); + write_test_settings(&config_path); + + let cli = Cli::try_parse_from([ + "fabro", + "server", + "start", + "--storage-dir", + storage_dir.path().to_str().unwrap(), + "--config", + config_path.to_str().unwrap(), + ]) + .expect("should parse"); + let command = cli.command.as_deref().unwrap(); + + let bootstrap = runtime() + .block_on(pre_tracing_bootstrap(command)) + .expect("bootstrap should resolve"); + + assert_eq!(bootstrap.sink, logging::InternalLogSink::Cli); + assert!(bootstrap.config_log_level.is_none()); + assert!(bootstrap.foreground_server_log_bootstrap.is_none()); + } + + #[test] + fn pre_tracing_bootstrap_uses_cli_sink_for_server_restart_daemon_wrapper() { + let storage_dir = tempfile::tempdir().unwrap(); + let config_dir = tempfile::tempdir().unwrap(); + let config_path = config_dir.path().join("settings.toml"); + write_test_settings(&config_path); + + let cli = Cli::try_parse_from([ + "fabro", + "server", + "restart", + "--storage-dir", + storage_dir.path().to_str().unwrap(), + "--config", + config_path.to_str().unwrap(), + ]) + .expect("should parse"); + let command = cli.command.as_deref().unwrap(); + + let bootstrap = runtime() + .block_on(pre_tracing_bootstrap(command)) + .expect("bootstrap should resolve"); + + assert_eq!(bootstrap.sink, logging::InternalLogSink::Cli); + assert!(bootstrap.config_log_level.is_none()); + assert!(bootstrap.foreground_server_log_bootstrap.is_none()); + } + #[test] fn parse_provider_login_missing_provider_flag() { let result = Cli::try_parse_from(["fabro", "provider", "login"]); diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index da033e83e..6e1ba9ba0 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -231,7 +231,7 @@ fn remote_dev_token_auth_for_target<'a>( } fn remote_url_matches_active_local_tcp_server(api_url: &str, storage_dir: &Path) -> bool { - let Some(record) = record::active_server_record(storage_dir) else { + let Ok(Some(record)) = record::active_server_record(storage_dir) else { return false; }; let Bind::Tcp(bind_addr) = record.bind else { @@ -1190,7 +1190,9 @@ mod tests { record::write_server_record(&record_path, &record::ServerRecord { pid: std::process::id(), bind: Bind::Unix(temp_home.path().join("fabro.sock")), - log_path: storage.path().join("server.log"), + log_path: fabro_config::Storage::new(storage.path()) + .server_state() + .log_path(), dev_token_path: Some(token_path), started_at: chrono::Utc::now(), }) diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index a64cf5c46..21a1bfe9e 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -22,31 +22,18 @@ pub(crate) fn load_settings() -> anyhow::Result { load_settings_with_config_and_storage_dir(None, None) } -pub(crate) fn settings_layer_with_config_and_storage_dir( - config_path: Option<&Path>, - storage_dir: Option<&Path>, -) -> anyhow::Result { - let layer = load_settings_config(config_path)?; - Ok(apply_storage_dir_override(layer, storage_dir)) -} - -pub(crate) fn settings_layer_with_storage_dir( - storage_dir: Option<&Path>, -) -> anyhow::Result { - settings_layer_with_config_and_storage_dir(None, storage_dir) -} - pub(crate) fn load_settings_with_storage_dir( storage_dir: Option<&Path>, ) -> anyhow::Result { - settings_layer_with_storage_dir(storage_dir) + load_settings_with_config_and_storage_dir(None, storage_dir) } pub(crate) fn load_settings_with_config_and_storage_dir( config_path: Option<&Path>, storage_dir: Option<&Path>, ) -> anyhow::Result { - settings_layer_with_config_and_storage_dir(config_path, storage_dir) + let layer = load_settings_config(config_path)?; + Ok(apply_storage_dir_override(layer, storage_dir)) } fn render_resolve_errors(errors: Vec) -> anyhow::Error { diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index 105228a24..1a67a34ac 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -641,7 +641,7 @@ fn settings_missing_run_config_errors() { } #[test] -fn settings_legacy_cli_config_warns_and_ignores_it() { +fn settings_legacy_cli_config_is_silently_ignored() { let context = test_context!(); let project = tempfile::tempdir().unwrap(); @@ -663,9 +663,91 @@ name = "legacy-model" .arg("--local") .current_dir(project.path()) .assert() - .success() - .stderr(predicate::str::contains("ignoring legacy config file")) - .stderr(predicate::str::contains("Rename it to")); + .success(); + + assert!( + assert.get_output().stderr.is_empty(), + "settings should not warn about legacy config files: {}", + String::from_utf8_lossy(&assert.get_output().stderr) + ); + + let cfg = parse_settings(&assert.get_output().stdout); + assert_eq!(cfg["cli"]["output"]["verbosity"].as_str(), Some("normal")); + assert!( + cfg["run"]["model"].get("name").is_none(), + "resolved dense settings should omit an unset run.model.name" + ); +} + +#[test] +fn settings_legacy_user_config_is_silently_ignored() { + let context = test_context!(); + let project = tempfile::tempdir().unwrap(); + + context.write_home( + ".fabro/user.toml", + r#" +_version = 1 + +[cli.output] +verbosity = "verbose" + +[run.model] +name = "legacy-model" +"#, + ); + + let assert = context + .settings() + .arg("--local") + .current_dir(project.path()) + .assert() + .success(); + + assert!( + assert.get_output().stderr.is_empty(), + "settings should not warn about legacy config files: {}", + String::from_utf8_lossy(&assert.get_output().stderr) + ); + + let cfg = parse_settings(&assert.get_output().stdout); + assert_eq!(cfg["cli"]["output"]["verbosity"].as_str(), Some("normal")); + assert!( + cfg["run"]["model"].get("name").is_none(), + "resolved dense settings should omit an unset run.model.name" + ); +} + +#[test] +fn settings_legacy_server_config_is_silently_ignored() { + let context = test_context!(); + let project = tempfile::tempdir().unwrap(); + + context.write_home( + ".fabro/server.toml", + r#" +_version = 1 + +[cli.output] +verbosity = "verbose" + +[run.model] +name = "legacy-model" +"#, + ); + + let assert = context + .settings() + .arg("--local") + .current_dir(project.path()) + .assert() + .success(); + + assert!( + assert.get_output().stderr.is_empty(), + "settings should not warn about legacy config files: {}", + String::from_utf8_lossy(&assert.get_output().stderr) + ); let cfg = parse_settings(&assert.get_output().stdout); assert_eq!(cfg["cli"]["output"]["verbosity"].as_str(), Some("normal")); @@ -697,8 +779,13 @@ shared = "legacy" .arg("--local") .current_dir(project.path()) .assert() - .success() - .stderr(predicate::str::contains("ignoring legacy config file")); + .success(); + + assert!( + assert.get_output().stderr.is_empty(), + "settings should not warn about legacy config files: {}", + String::from_utf8_lossy(&assert.get_output().stderr) + ); let cfg = parse_settings(&assert.get_output().stdout); assert_eq!(run_model_name(&cfg), Some("project-model")); diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index 40e0196ca..2415c28a2 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -2,13 +2,10 @@ use std::process::Stdio; use std::sync::{Arc, Barrier}; use std::time::{Duration, Instant}; -use fabro_test::{apply_test_isolation, fabro_snapshot, test_context}; - -fn isolated_storage_dir() -> tempfile::TempDir { - let root = tempfile::tempdir_in("/tmp").unwrap(); - std::fs::create_dir_all(root.path().join("storage")).unwrap(); - root -} +use fabro_test::{ + apply_test_isolation, fabro_snapshot, isolated_storage_dir, server_log_files, stop_pid, + test_context, wait_for_log_line, wait_for_path, +}; #[test] fn help() { @@ -151,6 +148,377 @@ fn start_without_default_settings_enters_install_mode_in_foreground() { ); } +#[test] +#[expect( + clippy::disallowed_methods, + reason = "This sync integration test spawns the real foreground server process to verify log ownership." +)] +fn foreground_start_writes_tracing_to_storage_server_log() { + let home_dir = tempfile::tempdir_in("/tmp").unwrap(); + let storage_root = isolated_storage_dir(); + let storage_dir = storage_root.path().join("storage"); + let socket_path = storage_root.path().join("foreground.sock"); + let config_dir = tempfile::tempdir_in("/tmp").unwrap(); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write(&config_path, "_version = 1\n").unwrap(); + let storage_log_path = storage_dir.join("logs").join("server.log"); + std::fs::create_dir_all(storage_log_path.parent().unwrap()).unwrap(); + std::fs::write(&storage_log_path, "stale pre-start log entry\n").unwrap(); + + let mut cmd = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + apply_test_isolation(&mut cmd, home_dir.path()); + cmd.args(["server", "start", "--foreground"]) + .arg("--storage-dir") + .arg(&storage_dir) + .arg("--bind") + .arg(&socket_path) + .arg("--config") + .arg(&config_path) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::piped()); + + let mut child = cmd.spawn().expect("server start should spawn"); + let record_path = storage_dir.join("server.json"); + let deadline = Instant::now() + Duration::from_secs(5); + + while Instant::now() < deadline { + if record_path.exists() { + break; + } + if let Some(status) = child.try_wait().expect("server start should poll") { + let output = child + .wait_with_output() + .expect("server start output should be readable"); + panic!( + "foreground server exited before writing server.json with status {status}:\nstderr:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + } + std::thread::sleep(Duration::from_millis(50)); + } + + assert!( + record_path.exists(), + "expected foreground start to create server.json" + ); + + wait_for_log_line(&storage_log_path, "API server started"); + + let stop_output = { + let mut stop = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + apply_test_isolation(&mut stop, home_dir.path()); + stop.args(["server", "stop"]) + .arg("--storage-dir") + .arg(&storage_dir) + .output() + .expect("server stop should run") + }; + assert!( + stop_output.status.success(), + "server stop should succeed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&stop_output.stdout), + String::from_utf8_lossy(&stop_output.stderr) + ); + + let output = child + .wait_with_output() + .expect("server start output should be readable"); + wait_for_log_line( + &storage_log_path, + "Shutdown signal received, stopping server", + ); + let storage_log = std::fs::read_to_string(&storage_log_path).unwrap_or_default(); + assert!( + storage_log.contains("API server started"), + "expected {} to contain server tracing, got:\n{}\nforeground stderr:\n{}", + storage_log_path.display(), + storage_log, + String::from_utf8_lossy(&output.stderr) + ); + assert!( + storage_log.contains("Shutdown signal received, stopping server"), + "expected {} to contain shutdown tracing, got:\n{}", + storage_log_path.display(), + storage_log + ); + assert!( + !storage_log.contains("stale pre-start log entry"), + "expected startup to truncate stale log contents, got:\n{}", + storage_log + ); + assert!( + storage_log.find("API server started") + < storage_log.find("Shutdown signal received, stopping server"), + "expected shutdown trace to append after startup trace, got:\n{}", + storage_log + ); + + let home_server_logs = server_log_files(&home_dir.path().join(".fabro").join("logs")); + assert!( + home_server_logs.is_empty(), + "expected foreground server start to avoid home server logs, found: {home_server_logs:?}" + ); +} + +#[test] +fn daemon_start_writes_tracing_to_storage_server_log() { + let context = test_context!(); + let storage_root = isolated_storage_dir(); + let storage_dir = storage_root.path().join("storage"); + let socket_path = storage_root.path().join("daemon.sock"); + let config_dir = tempfile::tempdir_in("/tmp").unwrap(); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write(&config_path, "_version = 1\n").unwrap(); + let storage_log_path = storage_dir.join("logs").join("server.log"); + std::fs::create_dir_all(storage_log_path.parent().unwrap()).unwrap(); + std::fs::write(&storage_log_path, "stale pre-start log entry\n").unwrap(); + + context + .command() + .args(["server", "start"]) + .arg("--storage-dir") + .arg(&storage_dir) + .arg("--bind") + .arg(&socket_path) + .arg("--config") + .arg(&config_path) + .assert() + .success(); + + wait_for_log_line(&storage_log_path, "API server started"); + + context + .command() + .args(["server", "stop"]) + .arg("--storage-dir") + .arg(&storage_dir) + .assert() + .success(); + + wait_for_log_line( + &storage_log_path, + "Shutdown signal received, stopping server", + ); + + let storage_log = std::fs::read_to_string(&storage_log_path).unwrap_or_default(); + assert!( + storage_log.contains("API server started"), + "expected {} to contain startup tracing, got:\n{}", + storage_log_path.display(), + storage_log + ); + assert!( + storage_log.contains("Shutdown signal received, stopping server"), + "expected {} to contain shutdown tracing, got:\n{}", + storage_log_path.display(), + storage_log + ); + assert!( + !storage_log.contains("stale pre-start log entry"), + "expected startup to truncate stale log contents, got:\n{}", + storage_log + ); + assert!( + storage_log.find("API server started") + < storage_log.find("Shutdown signal received, stopping server"), + "expected shutdown trace to append after startup trace, got:\n{}", + storage_log + ); + + let home_server_logs = server_log_files(&context.home_dir.join(".fabro").join("logs")); + assert!( + home_server_logs.is_empty(), + "expected daemonized server start to avoid home server logs, found: {home_server_logs:?}" + ); +} + +#[test] +#[expect( + clippy::disallowed_methods, + reason = "This integration test moves a live daemon record on disk to simulate an unsupported legacy daemon upgrade." +)] +fn start_errors_when_only_a_legacy_running_server_record_exists() { + let home_dir = tempfile::tempdir_in("/tmp").unwrap(); + let fabro_home = home_dir.path().join(".fabro"); + let storage_dir = fabro_home.join("storage"); + let socket_path = home_dir.path().join("legacy.sock"); + let config_dir = tempfile::tempdir_in("/tmp").unwrap(); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write(&config_path, "_version = 1\n").unwrap(); + + let start_output = { + let mut start = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + apply_test_isolation(&mut start, home_dir.path()); + start + .args(["server", "start", "--bind"]) + .arg(&socket_path) + .arg("--config") + .arg(&config_path) + .output() + .expect("server start should run") + }; + assert!( + start_output.status.success(), + "server start should succeed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&start_output.stdout), + String::from_utf8_lossy(&start_output.stderr) + ); + + let current_record = storage_dir.join("server.json"); + wait_for_path(¤t_record); + let legacy_record = fabro_home.join("server.json"); + std::fs::rename(¤t_record, &legacy_record).unwrap(); + + let retry_output = { + let mut retry = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + apply_test_isolation(&mut retry, home_dir.path()); + retry + .args(["server", "start", "--bind"]) + .arg(home_dir.path().join("new.sock")) + .arg("--config") + .arg(&config_path) + .output() + .expect("server start retry should run") + }; + + let pid = serde_json::from_str::( + &std::fs::read_to_string(&legacy_record).unwrap(), + ) + .unwrap()["pid"] + .as_u64() + .unwrap() as u32; + stop_pid(pid); + let _ = std::fs::remove_file(&legacy_record); + let _ = std::fs::remove_file(&socket_path); + + assert!( + !retry_output.status.success(), + "server start should fail when only the legacy record exists" + ); + let stderr = String::from_utf8_lossy(&retry_output.stderr); + assert!( + stderr.contains(&legacy_record.display().to_string()), + "expected stderr to mention the legacy record path, got:\n{stderr}" + ); + assert!( + stderr.contains(¤t_record.display().to_string()), + "expected stderr to mention the current record path, got:\n{stderr}" + ); + assert!( + stderr.contains("legacy Fabro CLI"), + "expected stderr to instruct manual cleanup, got:\n{stderr}" + ); +} + +#[test] +#[expect( + clippy::disallowed_methods, + reason = "This sync integration test starts two real foreground server processes to verify lock ownership protects log truncation." +)] +fn concurrent_foreground_start_does_not_retruncate_storage_server_log() { + let home_dir = tempfile::tempdir_in("/tmp").unwrap(); + let storage_root = isolated_storage_dir(); + let storage_dir = storage_root.path().join("storage"); + let first_socket_path = storage_root.path().join("foreground-first.sock"); + let second_socket_path = storage_root.path().join("foreground-second.sock"); + let config_dir = tempfile::tempdir_in("/tmp").unwrap(); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write(&config_path, "_version = 1\n").unwrap(); + + let storage_log_path = storage_dir.join("logs").join("server.log"); + std::fs::create_dir_all(storage_log_path.parent().unwrap()).unwrap(); + std::fs::write(&storage_log_path, "stale pre-start log entry\n").unwrap(); + + let mut first = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + apply_test_isolation(&mut first, home_dir.path()); + first + .args(["server", "start", "--foreground"]) + .arg("--storage-dir") + .arg(&storage_dir) + .arg("--bind") + .arg(&first_socket_path) + .arg("--config") + .arg(&config_path) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::piped()); + + let first_child = first.spawn().expect("first foreground start should spawn"); + wait_for_path(&storage_dir.join("server.json")); + wait_for_log_line(&storage_log_path, "API server started"); + + let marker = "marker-after-first-start\n"; + { + use std::io::Write as _; + + let mut file = std::fs::OpenOptions::new() + .append(true) + .open(&storage_log_path) + .unwrap(); + file.write_all(marker.as_bytes()).unwrap(); + } + + let second_output = { + let mut second = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + apply_test_isolation(&mut second, home_dir.path()); + second + .args(["server", "start", "--foreground"]) + .arg("--storage-dir") + .arg(&storage_dir) + .arg("--bind") + .arg(&second_socket_path) + .arg("--config") + .arg(&config_path) + .output() + .expect("second foreground start should run") + }; + + assert!( + !second_output.status.success(), + "second foreground start should fail:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&second_output.stdout), + String::from_utf8_lossy(&second_output.stderr) + ); + let second_stderr = String::from_utf8_lossy(&second_output.stderr); + assert!( + second_stderr.contains("timed out waiting for server lock"), + "expected lock timeout failure, got:\n{second_stderr}" + ); + + let storage_log = std::fs::read_to_string(&storage_log_path).unwrap_or_default(); + assert!( + storage_log.contains(marker.trim_end()), + "expected second start to avoid retruncating the log, got:\n{}", + storage_log + ); + assert!( + !storage_log.contains("stale pre-start log entry"), + "expected the first start to truncate stale log contents, got:\n{}", + storage_log + ); + + let stop_output = { + let mut stop = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + apply_test_isolation(&mut stop, home_dir.path()); + stop.args(["server", "stop", "--timeout", "0"]) + .arg("--storage-dir") + .arg(&storage_dir) + .output() + .expect("server stop should run") + }; + assert!( + stop_output.status.success(), + "server stop should succeed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&stop_output.stdout), + String::from_utf8_lossy(&stop_output.stderr) + ); + + let _ = first_child + .wait_with_output() + .expect("first child should exit"); +} + #[test] #[expect( clippy::disallowed_methods, diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_status.rs b/lib/crates/fabro-cli/tests/it/cmd/server_status.rs index 2ba18ad2c..a29f63d68 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_status.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_status.rs @@ -1,10 +1,4 @@ -use fabro_test::{fabro_snapshot, test_context}; - -fn isolated_storage_dir() -> tempfile::TempDir { - let root = tempfile::tempdir_in("/tmp").unwrap(); - std::fs::create_dir_all(root.path().join("storage")).unwrap(); - root -} +use fabro_test::{fabro_snapshot, isolated_storage_dir, stop_pid, test_context, wait_for_path}; #[test] fn help() { @@ -47,3 +41,78 @@ fn status_when_not_running() { Server is not running "); } + +#[test] +#[expect( + clippy::disallowed_methods, + reason = "This integration test moves a live daemon record on disk to simulate an unsupported legacy daemon upgrade." +)] +fn status_errors_when_only_a_legacy_running_server_record_exists() { + let home_dir = tempfile::tempdir_in("/tmp").unwrap(); + let fabro_home = home_dir.path().join(".fabro"); + let storage_dir = fabro_home.join("storage"); + let socket_path = home_dir.path().join("legacy.sock"); + let config_dir = tempfile::tempdir_in("/tmp").unwrap(); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write(&config_path, "_version = 1\n").unwrap(); + + let start_output = { + let mut start = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut start, home_dir.path()); + start + .args(["server", "start", "--bind"]) + .arg(&socket_path) + .arg("--config") + .arg(&config_path) + .output() + .expect("server start should run") + }; + assert!( + start_output.status.success(), + "server start should succeed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&start_output.stdout), + String::from_utf8_lossy(&start_output.stderr) + ); + + let current_record = storage_dir.join("server.json"); + wait_for_path(¤t_record); + let legacy_record = fabro_home.join("server.json"); + std::fs::rename(¤t_record, &legacy_record).unwrap(); + + let status_output = { + let mut status = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut status, home_dir.path()); + status + .args(["server", "status"]) + .output() + .expect("server status should run") + }; + + let pid = serde_json::from_str::( + &std::fs::read_to_string(&legacy_record).unwrap(), + ) + .unwrap()["pid"] + .as_u64() + .unwrap() as u32; + stop_pid(pid); + let _ = std::fs::remove_file(&legacy_record); + let _ = std::fs::remove_file(&socket_path); + + assert!( + !status_output.status.success(), + "server status should fail when only the legacy record exists" + ); + let stderr = String::from_utf8_lossy(&status_output.stderr); + assert!( + stderr.contains(&legacy_record.display().to_string()), + "expected stderr to mention the legacy record path, got:\n{stderr}" + ); + assert!( + stderr.contains(¤t_record.display().to_string()), + "expected stderr to mention the current record path, got:\n{stderr}" + ); + assert!( + stderr.contains("legacy Fabro CLI"), + "expected stderr to instruct manual cleanup, got:\n{stderr}" + ); +} diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_stop.rs b/lib/crates/fabro-cli/tests/it/cmd/server_stop.rs index 19ff9f313..a7f0664db 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_stop.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_stop.rs @@ -1,10 +1,4 @@ -use fabro_test::{fabro_snapshot, test_context}; - -fn isolated_storage_dir() -> tempfile::TempDir { - let root = tempfile::tempdir_in("/tmp").unwrap(); - std::fs::create_dir_all(root.path().join("storage")).unwrap(); - root -} +use fabro_test::{fabro_snapshot, isolated_storage_dir, test_context}; #[test] fn help() { diff --git a/lib/crates/fabro-cli/tests/it/cmd/uninstall.rs b/lib/crates/fabro-cli/tests/it/cmd/uninstall.rs index 0817fa928..daccce700 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/uninstall.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/uninstall.rs @@ -1,6 +1,6 @@ use std::fs; -use fabro_test::{fabro_snapshot, test_context}; +use fabro_test::{fabro_snapshot, stop_pid, test_context, wait_for_path}; use serde_json::Value; #[test] @@ -164,3 +164,84 @@ fn not_installed_json() { assert_eq!(value["status"].as_str(), Some("not_installed")); } + +#[test] +#[expect( + clippy::disallowed_methods, + reason = "This integration test moves a live daemon record on disk to simulate an unsupported legacy daemon upgrade." +)] +fn uninstall_yes_fails_when_only_a_legacy_running_server_record_exists() { + let home_dir = tempfile::tempdir_in("/tmp").unwrap(); + let fabro_home = home_dir.path().join(".fabro"); + let storage_dir = fabro_home.join("storage"); + let socket_path = home_dir.path().join("legacy.sock"); + let config_dir = tempfile::tempdir_in("/tmp").unwrap(); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write(&config_path, "_version = 1\n").unwrap(); + std::fs::create_dir_all(&fabro_home).unwrap(); + std::fs::write(fabro_home.join("settings.toml"), "_version = 1\n").unwrap(); + + let start_output = { + let mut start = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut start, home_dir.path()); + start + .args(["server", "start", "--bind"]) + .arg(&socket_path) + .arg("--config") + .arg(&config_path) + .output() + .expect("server start should run") + }; + assert!( + start_output.status.success(), + "server start should succeed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&start_output.stdout), + String::from_utf8_lossy(&start_output.stderr) + ); + + let current_record = storage_dir.join("server.json"); + wait_for_path(¤t_record); + let legacy_record = fabro_home.join("server.json"); + std::fs::rename(¤t_record, &legacy_record).unwrap(); + let pid = serde_json::from_str::( + &std::fs::read_to_string(&legacy_record).unwrap(), + ) + .unwrap()["pid"] + .as_u64() + .unwrap() as u32; + + let uninstall_output = { + let mut uninstall = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + fabro_test::apply_test_isolation(&mut uninstall, home_dir.path()); + uninstall + .args(["uninstall", "--yes"]) + .output() + .expect("uninstall should run") + }; + + stop_pid(pid); + let _ = std::fs::remove_file(&legacy_record); + let _ = std::fs::remove_file(&socket_path); + + assert!( + !uninstall_output.status.success(), + "uninstall --yes should fail when only the legacy record exists" + ); + let stderr = String::from_utf8_lossy(&uninstall_output.stderr); + assert!( + stderr.contains(&legacy_record.display().to_string()), + "expected stderr to mention the legacy record path, got:\n{stderr}" + ); + assert!( + stderr.contains(¤t_record.display().to_string()), + "expected stderr to mention the current record path, got:\n{stderr}" + ); + assert!( + stderr.contains("legacy Fabro CLI"), + "expected stderr to instruct manual cleanup, got:\n{stderr}" + ); + assert!( + fabro_home.exists(), + "uninstall should not remove ~/.fabro when the legacy daemon detector fires" + ); +} diff --git a/lib/crates/fabro-config/src/legacy_env.rs b/lib/crates/fabro-config/src/legacy_env.rs deleted file mode 100644 index 6286f124b..000000000 --- a/lib/crates/fabro-config/src/legacy_env.rs +++ /dev/null @@ -1,12 +0,0 @@ -//! Helpers for the deprecated `~/.fabro/.env` file. -//! -//! Fabro no longer reads `.env` automatically. The only remaining use for this -//! module is detecting the old file so CLI commands can print migration -//! warnings. - -use std::path::PathBuf; - -/// Return the path to the legacy `~/.fabro/.env` file. -pub fn legacy_env_file_path() -> PathBuf { - crate::Home::from_env().root().join(".env") -} diff --git a/lib/crates/fabro-config/src/lib.rs b/lib/crates/fabro-config/src/lib.rs index 0929f62d8..6d4396d19 100644 --- a/lib/crates/fabro-config/src/lib.rs +++ b/lib/crates/fabro-config/src/lib.rs @@ -6,7 +6,6 @@ pub mod effective_settings; pub mod envfile; pub mod error; pub mod home; -pub mod legacy_env; pub mod load; pub mod merge; pub mod parse; diff --git a/lib/crates/fabro-config/src/resolve/server.rs b/lib/crates/fabro-config/src/resolve/server.rs index 6716440fc..befb71ea7 100644 --- a/lib/crates/fabro-config/src/resolve/server.rs +++ b/lib/crates/fabro-config/src/resolve/server.rs @@ -14,6 +14,7 @@ use fabro_types::settings::server::{ use fabro_util::Home; use super::{ResolveError, default_interp, parse_socket_addr, require_interp}; +use crate::user::default_storage_dir; pub fn resolve_server(layer: &ServerLayer, errors: &mut Vec) -> ServerSettings { let storage = resolve_storage(layer.storage.as_ref()); @@ -57,7 +58,7 @@ fn resolve_storage(layer: Option<&ServerStorageLayer>) -> ServerStorageSettings ServerStorageSettings { root: layer .and_then(|storage| storage.root.clone()) - .unwrap_or_else(|| default_interp(Home::from_env().storage_dir())), + .unwrap_or_else(|| default_interp(default_storage_dir())), } } diff --git a/lib/crates/fabro-config/src/storage.rs b/lib/crates/fabro-config/src/storage.rs index dc0a4f2a5..2522096f8 100644 --- a/lib/crates/fabro-config/src/storage.rs +++ b/lib/crates/fabro-config/src/storage.rs @@ -39,6 +39,11 @@ impl Storage { self.root.join("cache") } + #[must_use] + pub fn slatedb_cache_dir(&self) -> PathBuf { + self.cache_dir().join("slatedb") + } + #[must_use] pub fn secrets_path(&self) -> PathBuf { self.root @@ -173,6 +178,10 @@ mod tests { storage.cache_dir(), std::path::Path::new("/tmp/fabro-data/cache") ); + assert_eq!( + storage.slatedb_cache_dir(), + std::path::Path::new("/tmp/fabro-data/cache/slatedb") + ); assert_eq!( storage.secrets_path(), std::path::Path::new("/tmp/fabro-data/vaults/default/secrets.json") diff --git a/lib/crates/fabro-config/src/user.rs b/lib/crates/fabro-config/src/user.rs index a4a390a4a..951ae75aa 100644 --- a/lib/crates/fabro-config/src/user.rs +++ b/lib/crates/fabro-config/src/user.rs @@ -4,9 +4,7 @@ //! `~/.fabro/settings.toml` file. Runtime types that used to be //! re-exported from here live in `fabro_types::settings::user` now. -use std::collections::HashSet; use std::path::{Path, PathBuf}; -use std::sync::{Mutex, OnceLock}; use fabro_types::settings::SettingsLayer; @@ -15,23 +13,18 @@ use crate::home::Home; use crate::load::load_settings_path; pub const SETTINGS_CONFIG_FILENAME: &str = "settings.toml"; -pub const LEGACY_USER_CONFIG_FILENAME: &str = "cli.toml"; -pub const LEGACY_OLD_USER_CONFIG_FILENAME: &str = "user.toml"; -pub const LEGACY_SERVER_CONFIG_FILENAME: &str = "server.toml"; pub const FABRO_CONFIG_ENV: &str = "FABRO_CONFIG"; -static WARNED_LEGACY_USER_CONFIGS: OnceLock>> = OnceLock::new(); - pub fn default_settings_path() -> PathBuf { Home::from_env().user_config() } -pub fn default_socket_path() -> PathBuf { - Home::from_env().root().join("fabro.sock") +pub fn default_storage_dir() -> PathBuf { + Home::from_env().root().join("storage") } -pub fn legacy_default_storage_root() -> PathBuf { - Home::from_env().root().to_path_buf() +pub fn default_socket_path() -> PathBuf { + Home::from_env().root().join("fabro.sock") } pub fn active_settings_path(path: Option<&Path>) -> PathBuf { @@ -47,37 +40,9 @@ fn active_settings_path_with_lookup( .unwrap_or_else(default_settings_path) } -pub fn legacy_user_config_path() -> Option { - Some(Home::from_env().root().join(LEGACY_USER_CONFIG_FILENAME)) -} - -pub fn legacy_old_user_config_path() -> Option { - Some( - Home::from_env() - .root() - .join(LEGACY_OLD_USER_CONFIG_FILENAME), - ) -} - -pub fn legacy_server_config_path() -> Option { - Some(Home::from_env().root().join(LEGACY_SERVER_CONFIG_FILENAME)) -} - -fn warned_legacy_user_configs() -> &'static Mutex> { - WARNED_LEGACY_USER_CONFIGS.get_or_init(|| Mutex::new(HashSet::new())) -} - -fn should_warn_about_legacy_user_config(path: &Path) -> bool { - warned_legacy_user_configs() - .lock() - .expect("legacy user config warning lock poisoned") - .insert(path.to_path_buf()) -} - /// Load settings config from an explicit path or `~/.fabro/settings.toml`, /// returning defaults if the default file doesn't exist. An explicit path that /// doesn't exist is an error. -#[allow(clippy::print_stderr)] pub fn load_settings_config(path: Option<&Path>) -> Result { if let Some(explicit) = path .map(Path::to_path_buf) @@ -86,25 +51,7 @@ pub fn load_settings_config(path: Option<&Path>) -> Result { return load_v2_layer_from_path(&explicit); } - for legacy_path in [ - legacy_user_config_path(), - legacy_old_user_config_path(), - legacy_server_config_path(), - ] - .into_iter() - .flatten() - { - if legacy_path.is_file() && should_warn_about_legacy_user_config(&legacy_path) { - let target = default_settings_path(); - eprintln!( - "Warning: ignoring legacy config file {}. Rename it to {}.", - legacy_path.display(), - target.display() - ); - } - } - - let default = Home::from_env().root().join(SETTINGS_CONFIG_FILENAME); + let default = default_settings_path(); if default.is_file() { load_v2_layer_from_path(&default) } else { @@ -119,23 +66,10 @@ fn load_v2_layer_from_path(path: &Path) -> Result { #[cfg(test)] mod tests { use super::{ - LEGACY_OLD_USER_CONFIG_FILENAME, LEGACY_SERVER_CONFIG_FILENAME, - LEGACY_USER_CONFIG_FILENAME, SETTINGS_CONFIG_FILENAME, active_settings_path_with_lookup, - default_settings_path, default_socket_path, legacy_old_user_config_path, - legacy_server_config_path, legacy_user_config_path, should_warn_about_legacy_user_config, + SETTINGS_CONFIG_FILENAME, active_settings_path_with_lookup, default_settings_path, + default_socket_path, default_storage_dir, }; - #[test] - fn should_warn_about_legacy_user_config_once_per_path() { - let dir = tempfile::tempdir().unwrap(); - let first = dir.path().join("cli.toml"); - let second = dir.path().join("other-cli.toml"); - - assert!(should_warn_about_legacy_user_config(&first)); - assert!(!should_warn_about_legacy_user_config(&first)); - assert!(should_warn_about_legacy_user_config(&second)); - } - #[test] fn settings_paths_use_expected_filenames() { let home = dirs::home_dir().unwrap(); @@ -144,33 +78,8 @@ mod tests { default_settings_path(), home.join(".fabro").join(SETTINGS_CONFIG_FILENAME) ); + assert_eq!(default_storage_dir(), home.join(".fabro/storage")); assert_eq!(default_socket_path(), home.join(".fabro/fabro.sock")); - assert_eq!( - legacy_user_config_path(), - Some(home.join(".fabro").join(LEGACY_USER_CONFIG_FILENAME)) - ); - assert_eq!( - legacy_old_user_config_path(), - Some(home.join(".fabro").join(LEGACY_OLD_USER_CONFIG_FILENAME)) - ); - assert_eq!( - legacy_server_config_path(), - Some(home.join(".fabro").join(LEGACY_SERVER_CONFIG_FILENAME)) - ); - } - - #[test] - fn should_warn_once_per_legacy_path_even_with_multiple_filenames() { - let dir = tempfile::tempdir().unwrap(); - let user = dir.path().join("user.toml"); - let server = dir.path().join("server.toml"); - let cli = dir.path().join("cli.toml"); - - assert!(should_warn_about_legacy_user_config(&user)); - assert!(!should_warn_about_legacy_user_config(&user)); - assert!(should_warn_about_legacy_user_config(&server)); - assert!(!should_warn_about_legacy_user_config(&server)); - assert!(should_warn_about_legacy_user_config(&cli)); } #[test] diff --git a/lib/crates/fabro-config/tests/resolve_server.rs b/lib/crates/fabro-config/tests/resolve_server.rs index 811d8bb63..b5d55abac 100644 --- a/lib/crates/fabro-config/tests/resolve_server.rs +++ b/lib/crates/fabro-config/tests/resolve_server.rs @@ -1,4 +1,5 @@ use fabro_config::parse_settings_layer; +use fabro_config::user::default_storage_dir; use fabro_types::settings::server::{ GithubIntegrationStrategy, IpAllowEntry, ObjectStoreSettings, ServerListenSettings, }; @@ -16,7 +17,7 @@ fn resolves_server_defaults_from_empty_settings() { assert_eq!( settings.storage.root.as_source(), - Home::from_env().storage_dir().to_string_lossy() + default_storage_dir().to_string_lossy() ); assert!(settings.web.enabled); assert_eq!(settings.web.url.as_source(), "http://localhost:3000"); @@ -36,8 +37,7 @@ fn resolves_server_defaults_from_empty_settings() { ObjectStoreSettings::Local { root } => { assert_eq!( root.as_source(), - Home::from_env() - .storage_dir() + default_storage_dir() .join("objects") .join("artifacts") .to_string_lossy() @@ -51,8 +51,7 @@ fn resolves_server_defaults_from_empty_settings() { ObjectStoreSettings::Local { root } => { assert_eq!( root.as_source(), - Home::from_env() - .storage_dir() + default_storage_dir() .join("objects") .join("slatedb") .to_string_lossy() diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 9c1c2aab2..b2a783090 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -354,7 +354,7 @@ where let (object_store, slatedb_prefix, flush_interval, disk_cache) = build_slatedb_store(&resolved_server_settings)?; let cache_path = if disk_cache { - Some(data_dir.join("cache").join("slatedb")) + Some(storage.slatedb_cache_dir()) } else { None }; diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index fa5af894b..4eee2c974 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -150,6 +150,75 @@ fn apply_test_isolation_with_lookup( cmd.env(TEST_IN_MEMORY_STORE_ENV, "1"); } +/// Create a fresh tempdir containing an empty `storage/` subdirectory, for +/// isolating server lifecycle tests from the shared nextest session storage. +#[must_use] +pub fn isolated_storage_dir() -> tempfile::TempDir { + let root = tempfile::tempdir_in("/tmp").expect("tempdir under /tmp"); + std::fs::create_dir_all(root.path().join("storage")).expect("create storage dir"); + root +} + +/// Poll up to 5s for a path to appear; panic on timeout. +pub fn wait_for_path(path: &Path) { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while std::time::Instant::now() < deadline { + if path.exists() { + return; + } + std::thread::sleep(std::time::Duration::from_millis(50)); + } + panic!("timed out waiting for {}", path.display()); +} + +/// Poll up to 5s for `needle` to appear in the contents of `path`; panic on +/// timeout. +pub fn wait_for_log_line(path: &Path, needle: &str) { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while std::time::Instant::now() < deadline { + if std::fs::read_to_string(path) + .ok() + .is_some_and(|contents| contents.contains(needle)) + { + return; + } + std::thread::sleep(std::time::Duration::from_millis(50)); + } + panic!("timed out waiting for {needle:?} in {}", path.display()); +} + +/// SIGTERM the pid, wait up to 5s for it to exit, then SIGKILL if still alive. +pub fn stop_pid(pid: u32) { + fabro_proc::sigterm(pid); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while std::time::Instant::now() < deadline { + if !fabro_proc::process_alive(pid) { + return; + } + std::thread::sleep(std::time::Duration::from_millis(50)); + } + fabro_proc::sigkill(pid); +} + +/// List any `server.*.log` files under `logs_dir`. Used by server-lifecycle +/// tests to assert that no server logs leak into the home logs directory. +#[must_use] +pub fn server_log_files(logs_dir: &Path) -> Vec { + let Ok(entries) = std::fs::read_dir(logs_dir) else { + return Vec::new(); + }; + + entries + .flatten() + .map(|entry| entry.path()) + .filter(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("server.") && name.ends_with(".log")) + }) + .collect() +} + /// A test context for running fabro CLI commands. /// /// Each context gets isolated home/temp directories. The storage directory is diff --git a/lib/crates/fabro-util/src/home.rs b/lib/crates/fabro-util/src/home.rs index 9f24976e5..f8d77cfb8 100644 --- a/lib/crates/fabro-util/src/home.rs +++ b/lib/crates/fabro-util/src/home.rs @@ -44,26 +44,11 @@ impl Home { self.root.join("settings.toml") } - #[must_use] - pub fn server_config(&self) -> PathBuf { - self.root.join("settings.toml") - } - - #[must_use] - pub fn certs_dir(&self) -> PathBuf { - self.root.join("certs") - } - #[must_use] pub fn skills_dir(&self) -> PathBuf { self.root.join("skills") } - #[must_use] - pub fn storage_dir(&self) -> PathBuf { - self.root.join("storage") - } - #[must_use] pub fn socket_path(&self) -> PathBuf { self.root.join("fabro.sock") @@ -103,22 +88,10 @@ mod tests { home.user_config(), std::path::Path::new("/tmp/fabro-home/settings.toml") ); - assert_eq!( - home.server_config(), - std::path::Path::new("/tmp/fabro-home/settings.toml") - ); - assert_eq!( - home.certs_dir(), - std::path::Path::new("/tmp/fabro-home/certs") - ); assert_eq!( home.skills_dir(), std::path::Path::new("/tmp/fabro-home/skills") ); - assert_eq!( - home.storage_dir(), - std::path::Path::new("/tmp/fabro-home/storage") - ); assert_eq!( home.socket_path(), std::path::Path::new("/tmp/fabro-home/fabro.sock") diff --git a/lib/crates/fabro-workflow/src/run_lookup.rs b/lib/crates/fabro-workflow/src/run_lookup.rs index eff585e3a..0e3a09318 100644 --- a/lib/crates/fabro-workflow/src/run_lookup.rs +++ b/lib/crates/fabro-workflow/src/run_lookup.rs @@ -140,7 +140,7 @@ pub fn scratch_base(storage_dir: &Path) -> PathBuf { } pub fn default_scratch_base() -> PathBuf { - scratch_base(&fabro_util::Home::from_env().storage_dir()) + scratch_base(&fabro_config::user::default_storage_dir()) } fn scan_orphan_runs(base: &Path) -> Result> {