From a6e755c200755405ac71de1ffd89f61fb57b6e11 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 22 Apr 2026 23:47:00 -0400 Subject: [PATCH] simplify: dedupe storage-root override + cache demo settings - Promote `apply_storage_dir_override` to `fabro_config::user` so the serve startup path stops carrying its own copy of the storage-root mutation that already lived in `fabro-cli/user_config.rs`. - Inline the `load_settings` and `router_web_enabled` one-liner wrappers in `fabro-server/src/serve.rs` and drop the dead `let _ = CliLayer::default()` marker. - Cache the demo `server_settings()` JSON in a `OnceLock` so the demo mode stops re-parsing TOML, re-resolving, and re-serializing the same static fixture on every `GET /api/v1/settings` request. - Standardize the four `state.settings.read().unwrap()` callsites in `fabro-server/src/server.rs` on `.expect("settings lock poisoned")` to match the existing convention. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/src/user_config.rs | 17 ------------ lib/crates/fabro-config/src/user.rs | 20 ++++++++++++++ lib/crates/fabro-server/src/demo/mod.rs | 25 +++++++++++------- lib/crates/fabro-server/src/serve.rs | 35 ++++++------------------- lib/crates/fabro-server/src/server.rs | 28 ++++++++++++++------ 5 files changed, 64 insertions(+), 61 deletions(-) diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 03429dbb4..774d3c8a5 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -30,23 +30,6 @@ pub(crate) fn load_settings_with_config_and_storage_dir( Ok(apply_storage_dir_override(layer, storage_dir)) } -pub(crate) fn apply_storage_dir_override( - mut layer: SettingsLayer, - storage_dir: Option<&Path>, -) -> SettingsLayer { - use fabro_types::settings::interp::InterpString; - use fabro_types::settings::server::{ServerLayer, ServerStorageLayer}; - if let Some(dir) = storage_dir { - let server = layer.server.get_or_insert_with(ServerLayer::default); - let storage = server - .storage - .get_or_insert_with(ServerStorageLayer::default); - storage.root = Some(InterpString::parse(&dir.display().to_string())); - } - - layer -} - /// Pull the resolved CLI target configuration out of `[cli.target]`. /// Returns either an http(s) URL or a unix socket path. fn cli_target_from_settings(settings: &CliNamespace) -> Option { diff --git a/lib/crates/fabro-config/src/user.rs b/lib/crates/fabro-config/src/user.rs index 951ae75aa..b561c361b 100644 --- a/lib/crates/fabro-config/src/user.rs +++ b/lib/crates/fabro-config/src/user.rs @@ -63,6 +63,26 @@ fn load_v2_layer_from_path(path: &Path) -> Result { load_settings_path(path) } +/// Override the resolved storage root in a settings layer with a runtime path. +/// +/// Used at server startup and by CLI commands that accept `--storage-dir`. +pub fn apply_storage_dir_override( + mut layer: SettingsLayer, + storage_dir: Option<&Path>, +) -> SettingsLayer { + use fabro_types::settings::interp::InterpString; + use fabro_types::settings::server::{ServerLayer, ServerStorageLayer}; + if let Some(dir) = storage_dir { + let server = layer.server.get_or_insert_with(ServerLayer::default); + let storage = server + .storage + .get_or_insert_with(ServerStorageLayer::default); + storage.root = Some(InterpString::parse(&dir.display().to_string())); + } + + layer +} + #[cfg(test)] mod tests { use super::{ diff --git a/lib/crates/fabro-server/src/demo/mod.rs b/lib/crates/fabro-server/src/demo/mod.rs index d52044c2b..3709bd62d 100644 --- a/lib/crates/fabro-server/src/demo/mod.rs +++ b/lib/crates/fabro-server/src/demo/mod.rs @@ -1554,9 +1554,14 @@ mod insights { } mod settings { + use std::sync::OnceLock; + pub(super) fn server_settings() -> serde_json::Value { - let settings = fabro_config::parse_settings_layer( - r#" + static CACHED: OnceLock = OnceLock::new(); + CACHED + .get_or_init(|| { + let settings = fabro_config::parse_settings_layer( + r#" _version = 1 [server.listen] @@ -1592,13 +1597,15 @@ slug = "fabro-dev" [features] session_sandboxes = false "#, - ) - .expect("demo settings fixture should parse"); + ) + .expect("demo settings fixture should parse"); - serde_json::to_value( - fabro_config::ServerSettings::from_layer(&settings) - .expect("demo settings fixture should resolve"), - ) - .expect("demo settings should serialize") + serde_json::to_value( + fabro_config::ServerSettings::from_layer(&settings) + .expect("demo settings fixture should resolve"), + ) + .expect("demo settings should serialize") + }) + .clone() } } diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index d35b1a46f..3705d5437 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -7,7 +7,7 @@ use anyhow::Context; use clap::Args; use fabro_config::bind::{self, Bind, BindRequest}; use fabro_config::merge::combine_files; -use fabro_config::user::load_settings_config; +use fabro_config::user::{apply_storage_dir_override, load_settings_config}; use fabro_config::{ServerSettings, Storage}; use fabro_install::{OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_SECRET_ACCESS_KEY_ENV}; use fabro_sandbox::SandboxProvider; @@ -159,12 +159,7 @@ pub struct ServeArgs { pub watch_web: bool, } -fn load_settings(path: Option<&Path>) -> anyhow::Result { - Ok(load_settings_config(path)?) -} - fn apply_serve_overrides(base: &SettingsLayer, args: &ServeArgs) -> SettingsLayer { - use fabro_types::settings::cli::CliLayer; use fabro_types::settings::interp::InterpString; use fabro_types::settings::run::{RunLayer, RunModelLayer, RunSandboxLayer}; use fabro_types::settings::server::{ServerLayer, ServerWebLayer}; @@ -189,8 +184,6 @@ fn apply_serve_overrides(base: &SettingsLayer, args: &ServeArgs) -> SettingsLaye let sandbox_layer = run.sandbox.get_or_insert_with(RunSandboxLayer::default); sandbox_layer.provider = Some(sandbox.to_string()); } - // CliLayer is namespaced; nothing to populate from flag overrides today. - let _ = CliLayer::default(); settings } @@ -199,19 +192,7 @@ fn apply_runtime_settings( args: &ServeArgs, data_dir: &Path, ) -> SettingsLayer { - use fabro_types::settings::interp::InterpString; - use fabro_types::settings::server::{ServerLayer, ServerStorageLayer}; - let mut settings = apply_serve_overrides(base, args); - let server = settings.server.get_or_insert_with(ServerLayer::default); - let storage = server - .storage - .get_or_insert_with(ServerStorageLayer::default); - storage.root = Some(InterpString::parse(&data_dir.to_string_lossy())); - settings -} - -fn router_web_enabled(settings: &ServerNamespace) -> bool { - settings.web.enabled + apply_storage_dir_override(apply_serve_overrides(base, args), Some(data_dir)) } async fn resolve_github_webhook_ip_allowlist( @@ -625,7 +606,7 @@ where #[cfg(debug_assertions)] let watch_web = args.watch_web; let config_path = args.config.clone(); - let disk_settings = load_settings(config_path.as_deref())?; + let disk_settings = load_settings_config(config_path.as_deref())?; let disk_server_settings = resolve_server_settings(&disk_settings)?; let data_dir = match storage_dir_override { Some(path) => path, @@ -652,7 +633,7 @@ where let max_concurrent_runs = resolved_server_settings.scheduler.max_concurrent_runs; (auth_mode, max_concurrent_runs) }; - let web_enabled = router_web_enabled(&resolved_server_settings); + let web_enabled = resolved_server_settings.web.enabled; let github_meta_resolver = GitHubMetaResolver::from_cache_dir(&storage.cache_dir())?; let (object_store, slatedb_prefix, flush_interval, disk_cache) = @@ -759,7 +740,7 @@ where interval.tick().await; // skip first immediate tick loop { interval.tick().await; - match load_settings(config_path_for_poll.as_deref()) { + match load_settings_config(config_path_for_poll.as_deref()) { Ok(new_disk_settings) => { let effective = apply_runtime_settings( &new_disk_settings, @@ -1073,8 +1054,8 @@ mod tests { bind_tcp_host_with_fallback, build_local_object_store_with_preference, build_object_store_from_settings_with_lookup, build_slatedb_store, resolve_bind_request_from_settings, resolve_github_webhook_ip_allowlist, - resolve_server_settings, resolve_startup_github_webhook_ip_allowlist, router_web_enabled, - server_bind_title, server_title, + resolve_server_settings, resolve_startup_github_webhook_ip_allowlist, server_bind_title, + server_title, }; use crate::server::create_app_state_with_options; @@ -1278,7 +1259,7 @@ strategy = "token" let resolved = resolve_server_settings(&base).expect("settings should resolve"); - assert!(router_web_enabled(&resolved)); + assert!(resolved.web.enabled); } #[test] diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 7dec3ca1b..fc3ee36f3 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -1316,7 +1316,11 @@ async fn get_system_info( _auth: AuthenticatedService, State(state): State>, ) -> Response { - let settings = state.settings.read().unwrap().clone(); + let settings = state + .settings + .read() + .expect("settings lock poisoned") + .clone(); let (total_runs, active_runs) = { let runs = state.runs.lock().expect("runs lock poisoned"); let active = runs @@ -3996,7 +4000,10 @@ async fn create_run( Ok(req) => req, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; - let prepared = match run_manifest::prepare_manifest(&state.settings.read().unwrap(), &req) { + let prepared = match run_manifest::prepare_manifest( + &state.settings.read().expect("settings lock poisoned"), + &req, + ) { Ok(prepared) => prepared, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; @@ -4098,7 +4105,10 @@ async fn run_preflight( State(state): State>, Json(req): Json, ) -> Response { - let prepared = match run_manifest::prepare_manifest(&state.settings.read().unwrap(), &req) { + let prepared = match run_manifest::prepare_manifest( + &state.settings.read().expect("settings lock poisoned"), + &req, + ) { Ok(prepared) => prepared, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; @@ -4124,11 +4134,13 @@ async fn render_graph_from_manifest( State(state): State>, Json(req): Json, ) -> Response { - let prepared = - match run_manifest::prepare_manifest(&state.settings.read().unwrap(), &req.manifest) { - Ok(prepared) => prepared, - Err(err) => return ApiError::bad_request(err.to_string()).into_response(), - }; + let prepared = match run_manifest::prepare_manifest( + &state.settings.read().expect("settings lock poisoned"), + &req.manifest, + ) { + Ok(prepared) => prepared, + Err(err) => return ApiError::bad_request(err.to_string()).into_response(), + }; let validated = match run_manifest::validate_prepared_manifest(&prepared) { Ok(validated) => validated, Err(err) => return ApiError::bad_request(err.to_string()).into_response(),