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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-22 23:47:00 -04:00
parent 28a9f036d8
commit a6e755c200
No known key found for this signature in database
5 changed files with 64 additions and 61 deletions

View file

@ -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<String> {

View file

@ -63,6 +63,26 @@ fn load_v2_layer_from_path(path: &Path) -> Result<SettingsLayer> {
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::{

View file

@ -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<serde_json::Value> = 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()
}
}

View file

@ -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<SettingsLayer> {
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]

View file

@ -1316,7 +1316,11 @@ async fn get_system_info(
_auth: AuthenticatedService,
State(state): State<Arc<AppState>>,
) -> 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<Arc<AppState>>,
Json(req): Json<RunManifest>,
) -> 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<Arc<AppState>>,
Json(req): Json<RenderWorkflowGraphRequest>,
) -> 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(),