From d22be7857595456aa3efe93cbaa94a8ff227861c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 13 Apr 2026 23:54:08 -0400 Subject: [PATCH] fix(server): honor server.listen when bind is omitted --- docs/administration/server-configuration.mdx | 2 +- docs/reference/cli.mdx | 6 +- .../fabro-cli/src/commands/server/mod.rs | 23 ++-- .../fabro-cli/src/commands/server/start.rs | 61 ++++++--- .../fabro-cli/tests/it/cmd/server_start.rs | 65 ++++++++++ lib/crates/fabro-server/src/serve.rs | 117 ++++++++++++++++-- 6 files changed, 227 insertions(+), 47 deletions(-) diff --git a/docs/administration/server-configuration.mdx b/docs/administration/server-configuration.mdx index a5f20cb0b..13d578741 100644 --- a/docs/administration/server-configuration.mdx +++ b/docs/administration/server-configuration.mdx @@ -102,7 +102,7 @@ Several `settings.toml` settings can be overridden via `fabro server start` flag | Flag | Default | Description | |---|---|---| -| `--bind` | `~/.fabro/fabro.sock` | Address to bind: `IP` or `IP:port` for TCP, or a path for Unix socket | +| `--bind` | Resolved `[server.listen]`, falling back to `~/.fabro/fabro.sock` when `[server.listen]` is absent | Address to bind: `IP` or `IP:port` for TCP, or a path for Unix socket | | `--web` | enabled | Enable the embedded web UI, browser auth routes, and web-only helper endpoints | | `--no-web` | disabled | Disable the embedded web UI, browser auth routes, and web-only helper endpoints | | `--foreground` | — | Run in the foreground instead of daemonizing | diff --git a/docs/reference/cli.mdx b/docs/reference/cli.mdx index b785df962..1c7bffe38 100644 --- a/docs/reference/cli.mdx +++ b/docs/reference/cli.mdx @@ -329,10 +329,10 @@ fabro model test -m claude-sonnet-4-5 ## `fabro server start` -Start the Fabro server daemon. By default, the server launches as a background process listening on a Unix socket. Use `--foreground` for the previous blocking behavior. +Start the Fabro server daemon. By default, the server launches as a background process using the resolved `[server.listen]` setting, and falls back to a Unix socket when `[server.listen]` is absent. Use `--foreground` for the previous blocking behavior. ```bash -fabro server start # background daemon on Unix socket +fabro server start # background daemon using server.listen or the default Unix socket fabro server start --bind 127.0.0.1 # TCP on 32276, or random port if 32276 is busy fabro server start --bind 127.0.0.1:8080 # TCP on a specific port fabro server start --no-web # API and /health only @@ -342,7 +342,7 @@ fabro server start --sandbox daytona --max-concurrent-runs 4 | Flag | Description | Default | |---|---|---| -| `--bind ` | Address to bind: `IP` or `IP:port` for TCP, or a path for Unix socket | `~/.fabro/fabro.sock` | +| `--bind ` | Address to bind: `IP` or `IP:port` for TCP, or a path for Unix socket | Resolved `[server.listen]`, falling back to `~/.fabro/fabro.sock` when `[server.listen]` is absent | | `--web` | Enable the embedded web UI, browser auth routes, and web-only helper endpoints | Enabled | | `--no-web` | Disable the embedded web UI, browser auth routes, and web-only helper endpoints | Disabled | | `--foreground` | Run in the foreground instead of daemonizing | — | diff --git a/lib/crates/fabro-cli/src/commands/server/mod.rs b/lib/crates/fabro-cli/src/commands/server/mod.rs index 2e16117de..7cece95ea 100644 --- a/lib/crates/fabro-cli/src/commands/server/mod.rs +++ b/lib/crates/fabro-cli/src/commands/server/mod.rs @@ -7,9 +7,7 @@ pub(crate) mod stop; use std::time::Duration; use anyhow::Result; -use fabro_server::bind; -use fabro_server::bind::BindRequest; -use fabro_server::serve::ServeArgs; +use fabro_server::serve::{self, ServeArgs}; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; @@ -34,10 +32,8 @@ pub(crate) async fn dispatch( storage_dir.as_deref(), )?; let storage_dir = user_config::storage_dir(&settings)?; - let bind_addr = match serve_args.bind.as_deref() { - Some(s) => bind::parse_bind(s)?, - None => BindRequest::Unix(user_config::default_socket_path()), - }; + 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())); Box::pin(start::execute( bind_addr, @@ -68,19 +64,18 @@ pub(crate) async fn dispatch( record_path, serve_args, }) => { + let settings = user_config::load_settings_with_config_and_storage_dir( + serve_args.config.as_deref(), + storage_dir.as_deref(), + )?; let active_config_path = Some( serve_args .config .clone() .unwrap_or_else(|| user_config::active_settings_path(None)), ); - let bind_addr = if let Some(s) = serve_args.bind.as_deref() { - bind::parse_bind(s)? - } else { - // __serve should always receive an explicit --bind from the parent, - // but fall back to the storage dir default if missing. - BindRequest::Unix(user_config::default_socket_path()) - }; + 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())); Box::pin(foreground::execute( record_path, diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index faf12708d..a22ae21c3 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -1,9 +1,9 @@ use std::path::{Path, PathBuf}; use std::time::Duration; -use anyhow::{Result, bail}; +use anyhow::{Result, anyhow, bail}; use chrono::Utc; -use fabro_config::user::default_socket_path; +use fabro_config::user::load_settings_config; use fabro_config::{Storage, envfile}; use fabro_server::bind::{Bind, BindRequest}; use fabro_server::serve; @@ -44,12 +44,7 @@ pub(crate) async fn ensure_server_running_for_storage( storage_dir: &Path, config_path: &Path, ) -> Result { - if let Some(existing) = record::active_server_record(storage_dir) { - return Ok(existing.bind); - } - - let bind = Bind::Unix(default_socket_path()); - ensure_server_running_with_bind(bind, config_path, storage_dir).await + ensure_server_running_with_bind(None, config_path, storage_dir).await } pub(crate) async fn ensure_server_running_on_socket( @@ -57,18 +52,25 @@ pub(crate) async fn ensure_server_running_on_socket( config_path: &Path, storage_dir: &Path, ) -> Result<()> { - let bind = Bind::Unix(socket_path.to_path_buf()); - let _ = ensure_server_running_with_bind(bind, config_path, storage_dir).await?; + let _ = ensure_server_running_with_bind( + Some(BindRequest::Unix(socket_path.to_path_buf())), + config_path, + storage_dir, + ) + .await?; Ok(()) } async fn ensure_server_running_with_bind( - bind: Bind, + bind_request: Option, config_path: &Path, storage_dir: &Path, ) -> Result { if let Some(existing) = record::active_server_record(storage_dir) { - if existing.bind == bind { + if bind_request + .as_ref() + .is_none_or(|requested| bind_matches_request(&existing.bind, requested)) + { return Ok(existing.bind); } bail!( @@ -79,7 +81,7 @@ async fn ensure_server_running_with_bind( } let serve_args = ServeArgs { - bind: None, + bind: bind_request.as_ref().map(ToString::to_string), web: false, no_web: false, model: None, @@ -90,9 +92,12 @@ async fn ensure_server_running_with_bind( config: Some(config_path.to_path_buf()), }; - let bind_request = match &bind { - Bind::Unix(path) => BindRequest::Unix(path.clone()), - Bind::Tcp(addr) => BindRequest::Tcp(*addr), + let bind_request = match bind_request { + Some(bind_request) => bind_request, + None => { + let settings = load_settings_config(Some(config_path))?; + serve::resolve_bind_request_from_settings(&settings, None)? + } }; match execute_daemon( @@ -104,7 +109,14 @@ async fn ensure_server_running_with_bind( ) .await { - Ok(()) => Ok(bind), + Ok(()) => record::active_server_record(storage_dir) + .map(|server| server.bind) + .ok_or_else(|| { + anyhow!( + "Server started but no active record was found for {}", + storage_dir.display() + ) + }), Err(err) => { if let Some(existing) = record::active_server_record(storage_dir) { Ok(existing.bind) @@ -115,6 +127,21 @@ async fn ensure_server_running_with_bind( } } +fn bind_matches_request(existing: &Bind, requested: &BindRequest) -> bool { + match (existing, requested) { + (Bind::Unix(existing_path), BindRequest::Unix(requested_path)) => { + existing_path == requested_path + } + (Bind::Tcp(existing_addr), BindRequest::Tcp(requested_addr)) => { + existing_addr == requested_addr + } + (Bind::Tcp(existing_addr), BindRequest::TcpHost(requested_host)) => { + existing_addr.ip() == *requested_host + } + _ => false, + } +} + fn server_max_concurrent_runs_override() -> Option { std::env::var("FABRO_SERVER_MAX_CONCURRENT_RUNS") .ok() 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 d67dc9a46..83c9b4352 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -138,6 +138,71 @@ fn start_without_bind_uses_home_socket_instead_of_storage_socket() { .success(); } +#[test] +fn start_without_bind_uses_configured_tcp_listen_address() { + let context = test_context!(); + let storage_root = isolated_storage_dir(); + let storage_dir = storage_root.path().join("storage"); + let config_dir = tempfile::tempdir_in("/tmp").unwrap(); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write( + &config_path, + r#" +_version = 1 + +[server.listen] +type = "tcp" +address = "127.0.0.1:0" +"#, + ) + .unwrap(); + + let mut cmd = context.command(); + cmd.env("FABRO_STORAGE_DIR", &storage_dir); + cmd.args([ + "server", + "start", + "--dry-run", + "--config", + config_path.to_str().unwrap(), + ]); + let output = cmd.output().expect("server start command should run"); + assert!( + output.status.success(), + "server start should succeed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + let bind_regex = regex::Regex::new(r"127\.0\.0\.1:\d+").unwrap(); + assert!( + bind_regex.is_match(&stderr), + "expected configured tcp bind in stderr, got {stderr}" + ); + + let output = context + .command() + .env("FABRO_STORAGE_DIR", &storage_dir) + .args(["server", "status", "--json"]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let json: serde_json::Value = serde_json::from_slice(&output).unwrap(); + let bind = json["bind"].as_str().expect("bind should be a string"); + assert!( + bind.starts_with("127.0.0.1:"), + "expected configured tcp bind, got {bind}" + ); + + context + .command() + .env("FABRO_STORAGE_DIR", &storage_dir) + .args(["server", "stop"]) + .assert() + .success(); +} + #[test] fn start_with_tcp_host_only_bind_resolves_to_host_and_port() { let context = test_context!(); diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 421ffa711..2fcc5b349 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -4,10 +4,11 @@ use std::time::Duration; use anyhow::Context; use clap::Args; +use fabro_config::merge::combine_files; use fabro_config::user::load_settings_config; use fabro_config::{Storage, resolve_server_from_file}; use fabro_sandbox::SandboxProvider; -use fabro_types::settings::server::GithubIntegrationStrategy; +use fabro_types::settings::server::{GithubIntegrationStrategy, ServerLayer, ServerListenLayer}; use fabro_types::settings::{ InterpString, ObjectStoreSettings, ServerListenSettings, ServerSettings as ResolvedServerSettings, SettingsLayer, @@ -187,6 +188,51 @@ fn resolve_server_settings(file: &SettingsLayer) -> anyhow::Result, +) -> anyhow::Result { + let effective_settings = match explicit_bind.map(bind::parse_bind).transpose()? { + Some(BindRequest::TcpHost(host)) => return Ok(BindRequest::TcpHost(host)), + Some(bind) => combine_files(settings.clone(), bind_override_layer(bind)), + None => settings.clone(), + }; + let resolved = resolve_server_settings(&effective_settings)?; + resolved_bind_request(&resolved) +} + +fn bind_override_layer(bind: BindRequest) -> SettingsLayer { + let listen = match bind { + BindRequest::Unix(path) => ServerListenLayer::Unix { + path: Some(InterpString::parse(&path.display().to_string())), + }, + BindRequest::Tcp(address) => ServerListenLayer::Tcp { + address: Some(InterpString::parse(&address.to_string())), + tls: None, + }, + BindRequest::TcpHost(_) => { + unreachable!("host-only bind requests are handled before building a settings override") + } + }; + + SettingsLayer { + server: Some(ServerLayer { + listen: Some(listen), + ..ServerLayer::default() + }), + ..SettingsLayer::default() + } +} + +fn resolved_bind_request( + resolved_server_settings: &ResolvedServerSettings, +) -> anyhow::Result { + match &resolved_server_settings.listen { + ServerListenSettings::Unix { path } => Ok(BindRequest::Unix(resolve_interp_path(path)?)), + ServerListenSettings::Tcp { address, .. } => Ok(BindRequest::Tcp(*address)), + } +} + fn resolve_interp(value: &InterpString) -> anyhow::Result { value .resolve(|name| std::env::var(name).ok()) @@ -294,6 +340,8 @@ where // Shared config for live reloading let effective_settings = apply_runtime_settings(&disk_settings, &args, dry_run_mode, &data_dir); let resolved_server_settings = resolve_server_settings(&effective_settings)?; + let bind_request = + resolve_bind_request_from_settings(&effective_settings, args.bind.as_deref())?; let shared_settings = Arc::new(RwLock::new(effective_settings)); std::fs::create_dir_all(&data_dir)?; let (auth_mode, max_concurrent_runs) = { @@ -337,14 +385,6 @@ where let router = build_router_with_options(Arc::clone(&state), auth_mode, RouterOptions { web_enabled }); - let bind_request = match args.bind { - Some(ref s) => bind::parse_bind(s)?, - None => match &resolved_server_settings.listen { - ServerListenSettings::Unix { path } => BindRequest::Unix(resolve_interp_path(path)?), - ServerListenSettings::Tcp { address, .. } => BindRequest::Tcp(*address), - }, - }; - // Optionally start webhook listener let webhook_manager = match resolved_server_settings.integrations.github.strategy { GithubIntegrationStrategy::Token => None, @@ -663,13 +703,14 @@ mod tests { use fabro_config::parse_settings_layer; use fabro_types::settings::SettingsLayer; + use fabro_util::Home; use super::{ ServeArgs, ServerTitlePhase, apply_runtime_settings, bind_tcp_host_with_fallback, - build_object_store_with_preference, resolve_server_settings, router_web_enabled, - server_bind_title, server_title, + build_object_store_with_preference, resolve_bind_request_from_settings, + resolve_server_settings, router_web_enabled, server_bind_title, server_title, }; - use crate::bind::Bind; + use crate::bind::{Bind, BindRequest}; fn parse_settings(source: &str) -> SettingsLayer { parse_settings_layer(source).expect("v2 fixture should parse") @@ -763,6 +804,58 @@ enabled = false ); } + #[test] + fn resolve_bind_request_from_settings_defaults_to_socket_when_listen_is_absent() { + let bind = + resolve_bind_request_from_settings(&SettingsLayer::default(), None).expect("bind"); + + assert_eq!(bind, BindRequest::Unix(Home::from_env().socket_path())); + } + + #[test] + fn resolve_bind_request_from_settings_uses_configured_tcp_when_no_explicit_bind_is_given() { + let settings = parse_settings( + r#" +_version = 1 + +[server.listen] +type = "tcp" +address = "127.0.0.1:0" +"#, + ); + + let bind = resolve_bind_request_from_settings(&settings, None).expect("bind"); + + assert_eq!(bind, BindRequest::Tcp("127.0.0.1:0".parse().unwrap())); + } + + #[test] + fn resolve_bind_request_from_settings_prefers_explicit_bind_over_config() { + let settings = parse_settings( + r#" +_version = 1 + +[server.listen] +type = "tcp" +address = "127.0.0.1:32276" +"#, + ); + + let bind = + resolve_bind_request_from_settings(&settings, Some("/tmp/fabro.sock")).expect("bind"); + + assert_eq!(bind, BindRequest::Unix(PathBuf::from("/tmp/fabro.sock"))); + } + + #[test] + fn resolve_bind_request_from_settings_preserves_host_only_cli_bind() { + let settings = SettingsLayer::default(); + + let bind = resolve_bind_request_from_settings(&settings, Some("127.0.0.1")).expect("bind"); + + assert_eq!(bind, BindRequest::TcpHost("127.0.0.1".parse().unwrap())); + } + #[test] fn web_enabled_stays_enabled_without_github_app_mode() { let base = parse_settings(