From 7abc11adc9beb1c10ea5965d7ad9062785ebd9f6 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 26 Apr 2026 17:16:03 -0400 Subject: [PATCH 1/3] feat(server): route worker logs to the same destination as the parent Workers are an internal implementation detail; operators should not need to know about them. When the server runs in stdout mode (FABRO_LOG_DESTINATION=stdout, e.g. inside containers), workers now also stream their tracing to stdout so all server-level logs land on the same destination. The parent propagates its resolved destination to each worker via FABRO_LOG_DESTINATION and inherits the worker's stdout when the parent is in stdout mode (so worker stdout flows through to docker logs). The per-run log at /runtime/server.log stays a file regardless -- it is read back by the run UI. A CLI-side ServerLogSink::{File(PathBuf),Stdout} replaces Option so the file/stdout intent is explicit at the type level for both the Server and Worker sinks. LogDestination gains strum::IntoStaticStr so the parent can stringify it for the worker env without a hand-written map. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/src/logging.rs | 30 +++++++-- lib/crates/fabro-cli/src/main.rs | 65 ++++++++++++++++--- lib/crates/fabro-server/src/server.rs | 22 ++++++- lib/crates/fabro-server/src/spawn_env.rs | 1 + lib/crates/fabro-types/src/settings/server.rs | 13 +++- 5 files changed, 113 insertions(+), 18 deletions(-) diff --git a/lib/crates/fabro-cli/src/logging.rs b/lib/crates/fabro-cli/src/logging.rs index 661802eaf..f69b0fa9a 100644 --- a/lib/crates/fabro-cli/src/logging.rs +++ b/lib/crates/fabro-cli/src/logging.rs @@ -16,16 +16,20 @@ use tracing_subscriber::{EnvFilter, fmt}; const LOG_RETENTION_DAYS: u32 = 7; +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum ServerLogSink { + File(PathBuf), + Stdout, +} + #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum InternalLogSink { Cli, - /// `Some(path)` writes the server log to a file at `path`; `None` writes to - /// stdout. Server { - log_path: Option, + log: ServerLogSink, }, Worker { - server_log_path: PathBuf, + server_log: ServerLogSink, per_run_log_path: PathBuf, }, } @@ -62,15 +66,17 @@ pub(crate) fn init_tracing( init_subscriber(filter, file_appender); } InternalLogSink::Server { - log_path: Some(path), + log: ServerLogSink::File(path), } => { init_subscriber(filter, open_buffered_appender(path)?); } - InternalLogSink::Server { log_path: None } => { + InternalLogSink::Server { + log: ServerLogSink::Stdout, + } => { init_subscriber(filter, std::io::stdout); } InternalLogSink::Worker { - server_log_path, + server_log: ServerLogSink::File(server_log_path), per_run_log_path, } => { init_worker_subscriber( @@ -79,6 +85,16 @@ pub(crate) fn init_tracing( open_buffered_appender(per_run_log_path)?, ); } + InternalLogSink::Worker { + server_log: ServerLogSink::Stdout, + per_run_log_path, + } => { + init_worker_subscriber( + filter, + std::io::stdout, + open_buffered_appender(per_run_log_path)?, + ); + } } Ok(()) diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 74fa3f6a3..e1e21b95c 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -481,10 +481,10 @@ async fn prepare_server_bootstrap( } else { None }; - let log_path = (log_destination == LogDestination::File).then(|| runtime_directory.log_path()); + let log = server_log_sink(log_destination, &runtime_directory); Ok(PreTracingBootstrap { - sink: logging::InternalLogSink::Server { log_path }, + sink: logging::InternalLogSink::Server { log }, config_log_level: local_config.config_log_level().map(str::to_owned), foreground_server_log_bootstrap, }) @@ -496,10 +496,14 @@ fn prepare_run_worker_bootstrap( ) -> Result { let local_config = local_server::LocalServerConfig::load_with_storage_dir(storage_dir)?; let runtime_directory = fabro_config::RuntimeDirectory::new(local_config.storage_dir()); + let log_destination = logging::resolve_log_destination( + local_config.config_log_destination().unwrap_or_default(), + )?; + let server_log = server_log_sink(log_destination, &runtime_directory); Ok(PreTracingBootstrap { sink: logging::InternalLogSink::Worker { - server_log_path: runtime_directory.log_path(), + server_log, per_run_log_path: run_dir.join("runtime").join("server.log"), }, config_log_level: None, @@ -507,6 +511,16 @@ fn prepare_run_worker_bootstrap( }) } +fn server_log_sink( + destination: LogDestination, + runtime_directory: &fabro_config::RuntimeDirectory, +) -> logging::ServerLogSink { + match destination { + LogDestination::File => logging::ServerLogSink::File(runtime_directory.log_path()), + LogDestination::Stdout => logging::ServerLogSink::Stdout, + } +} + #[cfg(test)] #[expect( clippy::disallowed_methods, @@ -711,7 +725,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { - log_path: None, + log: logging::ServerLogSink::Stdout, }); assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); assert!(bootstrap.foreground_server_log_bootstrap.is_some()); @@ -742,7 +756,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { - log_path: None, + log: logging::ServerLogSink::Stdout, }); assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); assert!(bootstrap.foreground_server_log_bootstrap.is_some()); @@ -771,7 +785,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { - log_path: None, + log: logging::ServerLogSink::Stdout, }); assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); assert!(bootstrap.foreground_server_log_bootstrap.is_none()); @@ -803,7 +817,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { - log_path: None, + log: logging::ServerLogSink::Stdout, }); }); } @@ -916,13 +930,48 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Worker { - server_log_path: storage_dir.path().join("logs").join("server.log"), + server_log: logging::ServerLogSink::File( + storage_dir.path().join("logs").join("server.log"), + ), per_run_log_path: run_dir.path().join("runtime").join("server.log"), }); assert!(bootstrap.config_log_level.is_none()); assert!(bootstrap.foreground_server_log_bootstrap.is_none()); } + #[test] + fn pre_tracing_bootstrap_worker_uses_stdout_when_env_overrides() { + let storage_dir = tempfile::tempdir().unwrap(); + let run_dir = tempfile::tempdir().unwrap(); + let cli = Cli::try_parse_from([ + "fabro", + "__run-worker", + "--server", + "/tmp/fabro.sock", + "--storage-dir", + storage_dir.path().to_str().unwrap(), + "--run-dir", + run_dir.path().to_str().unwrap(), + "--run-id", + "01ARZ3NDEKTSV4RRFFQ69G5FAV", + "--mode", + "start", + ]) + .expect("should parse"); + let command = cli.command.as_deref().unwrap(); + + with_var(EnvVars::FABRO_LOG_DESTINATION, Some("stdout"), || { + let bootstrap = runtime() + .block_on(pre_tracing_bootstrap(command)) + .expect("bootstrap should resolve"); + + assert_eq!(bootstrap.sink, logging::InternalLogSink::Worker { + server_log: logging::ServerLogSink::Stdout, + per_run_log_path: run_dir.path().join("runtime").join("server.log"), + }); + }); + } + #[test] fn pre_tracing_bootstrap_uses_cli_sink_for_server_start_daemon_wrapper() { let storage_dir = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index dc9662e41..f02bdddf4 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -71,7 +71,9 @@ use fabro_store::{ #[cfg(test)] use fabro_types::BlockedReason; use fabro_types::settings::run::RunMode; -use fabro_types::settings::server::{GithubIntegrationSettings, GithubIntegrationStrategy}; +use fabro_types::settings::server::{ + GithubIntegrationSettings, GithubIntegrationStrategy, LogDestination, +}; use fabro_types::settings::{InterpString, RunNamespace}; use fabro_types::{ ActorRef, EventBody, InterviewQuestionRecord, InterviewQuestionType, PullRequestRecord, @@ -3915,6 +3917,11 @@ fn worker_command( let server_target = daemon.bind.to_target(); let worker_token = issue_worker_token(state.worker_token_keys(), &run_id) .map_err(|_| anyhow::anyhow!("failed to sign worker token"))?; + let server_destination = resolved_log_destination(state); + let worker_stdout = match server_destination { + LogDestination::Stdout => Stdio::inherit(), + LogDestination::File => Stdio::null(), + }; let mut cmd = Command::new(exe); cmd.arg("__run-worker") .arg("--server") @@ -3928,7 +3935,7 @@ fn worker_command( .arg("--mode") .arg(worker_mode_arg(mode)) .stdin(Stdio::piped()) - .stdout(Stdio::null()) + .stdout(worker_stdout) .stderr(Stdio::piped()); apply_worker_env(&mut cmd); @@ -3937,6 +3944,10 @@ fn worker_command( cmd.env(EnvVars::FABRO_LOG, level); } } + if (state.env_lookup)(EnvVars::FABRO_LOG_DESTINATION).is_none() { + let value: &'static str = server_destination.into(); + cmd.env(EnvVars::FABRO_LOG_DESTINATION, value); + } cmd.env_remove(EnvVars::FABRO_WORKER_TOKEN); cmd.env(EnvVars::FABRO_WORKER_TOKEN, worker_token); @@ -3946,6 +3957,13 @@ fn worker_command( Ok(cmd) } +fn resolved_log_destination(state: &AppState) -> LogDestination { + (state.env_lookup)(EnvVars::FABRO_LOG_DESTINATION) + .as_deref() + .and_then(|raw| raw.parse::().ok()) + .unwrap_or(state.server_settings().server.logging.destination) +} + fn api_question_type(question_type: InterviewQuestionType) -> ApiQuestionType { match question_type { InterviewQuestionType::YesNo => ApiQuestionType::YesNo, diff --git a/lib/crates/fabro-server/src/spawn_env.rs b/lib/crates/fabro-server/src/spawn_env.rs index c60e0f719..f4ab6ae49 100644 --- a/lib/crates/fabro-server/src/spawn_env.rs +++ b/lib/crates/fabro-server/src/spawn_env.rs @@ -11,6 +11,7 @@ const WORKER_ENV_ALLOWLIST: &[&str] = &[ EnvVars::RUST_LOG, EnvVars::RUST_BACKTRACE, EnvVars::FABRO_LOG, + EnvVars::FABRO_LOG_DESTINATION, EnvVars::FABRO_HOME, EnvVars::FABRO_STORAGE_ROOT, ]; diff --git a/lib/crates/fabro-types/src/settings/server.rs b/lib/crates/fabro-types/src/settings/server.rs index fa7ed1626..2aec85d8e 100644 --- a/lib/crates/fabro-types/src/settings/server.rs +++ b/lib/crates/fabro-types/src/settings/server.rs @@ -230,7 +230,18 @@ pub struct ServerSchedulerSettings { pub max_concurrent_runs: usize, } -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize, strum::EnumString)] +#[derive( + Debug, + Clone, + Copy, + Default, + PartialEq, + Eq, + Serialize, + Deserialize, + strum::EnumString, + strum::IntoStaticStr, +)] #[serde(rename_all = "lowercase")] #[strum(serialize_all = "lowercase")] pub enum LogDestination { From e6ca9eba875be723c0cd39b03734875730c815b0 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 26 Apr 2026 17:36:03 -0400 Subject: [PATCH 2/3] refactor(cli): rename ServerLogSink to LogSink The Server prefix is redundant -- the type is used by both Server and Worker variants of InternalLogSink, and the helper that builds it from a runtime directory is renamed to log_sink to match. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/src/logging.rs | 14 +++++++------- lib/crates/fabro-cli/src/main.rs | 24 ++++++++++++------------ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/lib/crates/fabro-cli/src/logging.rs b/lib/crates/fabro-cli/src/logging.rs index f69b0fa9a..03fabe7b5 100644 --- a/lib/crates/fabro-cli/src/logging.rs +++ b/lib/crates/fabro-cli/src/logging.rs @@ -17,7 +17,7 @@ use tracing_subscriber::{EnvFilter, fmt}; const LOG_RETENTION_DAYS: u32 = 7; #[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) enum ServerLogSink { +pub(crate) enum LogSink { File(PathBuf), Stdout, } @@ -26,10 +26,10 @@ pub(crate) enum ServerLogSink { pub(crate) enum InternalLogSink { Cli, Server { - log: ServerLogSink, + log: LogSink, }, Worker { - server_log: ServerLogSink, + server_log: LogSink, per_run_log_path: PathBuf, }, } @@ -66,17 +66,17 @@ pub(crate) fn init_tracing( init_subscriber(filter, file_appender); } InternalLogSink::Server { - log: ServerLogSink::File(path), + log: LogSink::File(path), } => { init_subscriber(filter, open_buffered_appender(path)?); } InternalLogSink::Server { - log: ServerLogSink::Stdout, + log: LogSink::Stdout, } => { init_subscriber(filter, std::io::stdout); } InternalLogSink::Worker { - server_log: ServerLogSink::File(server_log_path), + server_log: LogSink::File(server_log_path), per_run_log_path, } => { init_worker_subscriber( @@ -86,7 +86,7 @@ pub(crate) fn init_tracing( ); } InternalLogSink::Worker { - server_log: ServerLogSink::Stdout, + server_log: LogSink::Stdout, per_run_log_path, } => { init_worker_subscriber( diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index e1e21b95c..1b843c7c2 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -481,7 +481,7 @@ async fn prepare_server_bootstrap( } else { None }; - let log = server_log_sink(log_destination, &runtime_directory); + let log = log_sink(log_destination, &runtime_directory); Ok(PreTracingBootstrap { sink: logging::InternalLogSink::Server { log }, @@ -499,7 +499,7 @@ fn prepare_run_worker_bootstrap( let log_destination = logging::resolve_log_destination( local_config.config_log_destination().unwrap_or_default(), )?; - let server_log = server_log_sink(log_destination, &runtime_directory); + let server_log = log_sink(log_destination, &runtime_directory); Ok(PreTracingBootstrap { sink: logging::InternalLogSink::Worker { @@ -511,13 +511,13 @@ fn prepare_run_worker_bootstrap( }) } -fn server_log_sink( +fn log_sink( destination: LogDestination, runtime_directory: &fabro_config::RuntimeDirectory, -) -> logging::ServerLogSink { +) -> logging::LogSink { match destination { - LogDestination::File => logging::ServerLogSink::File(runtime_directory.log_path()), - LogDestination::Stdout => logging::ServerLogSink::Stdout, + LogDestination::File => logging::LogSink::File(runtime_directory.log_path()), + LogDestination::Stdout => logging::LogSink::Stdout, } } @@ -725,7 +725,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { - log: logging::ServerLogSink::Stdout, + log: logging::LogSink::Stdout, }); assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); assert!(bootstrap.foreground_server_log_bootstrap.is_some()); @@ -756,7 +756,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { - log: logging::ServerLogSink::Stdout, + log: logging::LogSink::Stdout, }); assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); assert!(bootstrap.foreground_server_log_bootstrap.is_some()); @@ -785,7 +785,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { - log: logging::ServerLogSink::Stdout, + log: logging::LogSink::Stdout, }); assert_eq!(bootstrap.config_log_level.as_deref(), Some("warn")); assert!(bootstrap.foreground_server_log_bootstrap.is_none()); @@ -817,7 +817,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Server { - log: logging::ServerLogSink::Stdout, + log: logging::LogSink::Stdout, }); }); } @@ -930,7 +930,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Worker { - server_log: logging::ServerLogSink::File( + server_log: logging::LogSink::File( storage_dir.path().join("logs").join("server.log"), ), per_run_log_path: run_dir.path().join("runtime").join("server.log"), @@ -966,7 +966,7 @@ destination = "{destination}" .expect("bootstrap should resolve"); assert_eq!(bootstrap.sink, logging::InternalLogSink::Worker { - server_log: logging::ServerLogSink::Stdout, + server_log: logging::LogSink::Stdout, per_run_log_path: run_dir.path().join("runtime").join("server.log"), }); }); From 583fa1e9d19fef0118b16f47f63a9341639597d3 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 26 Apr 2026 17:47:22 -0400 Subject: [PATCH 3/3] refactor(config): share log destination resolution Move FABRO_LOG_DESTINATION parsing into fabro-config so CLI and server worker startup use the same validation behavior. Worker startup now exports one canonical resolved destination instead of relying on a generic env allowlist path. --- .../fabro-cli/src/commands/server/start.rs | 4 +- lib/crates/fabro-cli/src/logging.rs | 23 ---- lib/crates/fabro-cli/src/main.rs | 4 +- lib/crates/fabro-config/src/lib.rs | 2 + lib/crates/fabro-config/src/logging.rs | 49 +++++++ lib/crates/fabro-server/src/server.rs | 126 ++++++++++++++++-- lib/crates/fabro-server/src/spawn_env.rs | 3 +- 7 files changed, 172 insertions(+), 39 deletions(-) create mode 100644 lib/crates/fabro-config/src/logging.rs diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index 4547ffb89..cb87cdcf5 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -23,7 +23,7 @@ use tokio::process::Command as TokioCommand; use tokio::task::spawn_blocking; use tokio::time; -use crate::{local_server, logging}; +use crate::local_server; pub(crate) struct ForegroundServerLogBootstrap { #[expect(dead_code, reason = "held for its Drop to release the server lock")] @@ -268,7 +268,7 @@ async fn execute_daemon( } let resolved_settings = resolve_runtime_server_settings_for_start(serve_args, storage_dir)?; - let destination = logging::resolve_log_destination(resolved_settings.logging.destination)?; + let destination = fabro_config::resolve_log_destination(resolved_settings.logging.destination)?; if matches!(destination, LogDestination::Stdout) { bail!( "[server.logging].destination = \"stdout\" is incompatible with daemon mode; use `fabro server start --foreground`" diff --git a/lib/crates/fabro-cli/src/logging.rs b/lib/crates/fabro-cli/src/logging.rs index 03fabe7b5..1aa0d9a78 100644 --- a/lib/crates/fabro-cli/src/logging.rs +++ b/lib/crates/fabro-cli/src/logging.rs @@ -6,7 +6,6 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use fabro_static::EnvVars; -use fabro_types::settings::server::LogDestination; use fabro_util::run_log::BufferedFileAppender; use tracing_appender::rolling; use tracing_subscriber::fmt::writer::MakeWriter; @@ -100,28 +99,6 @@ pub(crate) fn init_tracing( Ok(()) } -pub(crate) fn resolve_log_destination( - config_destination: LogDestination, -) -> Result { - let env_value = std::env::var(EnvVars::FABRO_LOG_DESTINATION).ok(); - resolve_log_destination_with_env(config_destination, env_value.as_deref()) -} - -pub(crate) fn resolve_log_destination_with_env( - config_destination: LogDestination, - env_value: Option<&str>, -) -> Result { - match env_value { - Some(value) => value.parse::().with_context(|| { - format!( - "invalid {} value `{value}`; expected `file` or `stdout`", - EnvVars::FABRO_LOG_DESTINATION - ) - }), - None => Ok(config_destination), - } -} - fn cleanup_old_logs(log_dir: &Path, prefix: &str, max_age_days: u32) { let cutoff = chrono::Utc::now().date_naive() - chrono::Duration::days(i64::from(max_age_days)); let Ok(entries) = std::fs::read_dir(log_dir) else { diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 1b843c7c2..2604f4617 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -467,7 +467,7 @@ async fn prepare_server_bootstrap( let local_config = local_server::LocalServerConfig::load(config_path, storage_dir)?; let storage_dir = local_config.storage_dir().to_path_buf(); let runtime_directory = fabro_config::RuntimeDirectory::new(storage_dir.clone()); - let log_destination = logging::resolve_log_destination( + let log_destination = fabro_config::resolve_log_destination( local_config.config_log_destination().unwrap_or_default(), )?; let foreground_server_log_bootstrap = if foreground { @@ -496,7 +496,7 @@ fn prepare_run_worker_bootstrap( ) -> Result { let local_config = local_server::LocalServerConfig::load_with_storage_dir(storage_dir)?; let runtime_directory = fabro_config::RuntimeDirectory::new(local_config.storage_dir()); - let log_destination = logging::resolve_log_destination( + let log_destination = fabro_config::resolve_log_destination( local_config.config_log_destination().unwrap_or_default(), )?; let server_log = log_sink(log_destination, &runtime_directory); diff --git a/lib/crates/fabro-config/src/lib.rs b/lib/crates/fabro-config/src/lib.rs index a5edd51a2..c0d3121d6 100644 --- a/lib/crates/fabro-config/src/lib.rs +++ b/lib/crates/fabro-config/src/lib.rs @@ -16,6 +16,7 @@ pub mod envfile; pub mod error; pub mod home; mod load; +pub mod logging; pub mod parse; pub mod project; pub mod resolve; @@ -51,6 +52,7 @@ pub use layers::{ TeamsIntegrationLayer, WorkflowLayer, }; pub(crate) use layers::{Combine, SettingsLayer}; +pub use logging::{resolve_log_destination, resolve_log_destination_with_env}; pub use parse::ParseError; pub use resolve::{ ResolveError, resolve_cli, resolve_features, resolve_project, resolve_run, resolve_server, diff --git a/lib/crates/fabro-config/src/logging.rs b/lib/crates/fabro-config/src/logging.rs new file mode 100644 index 000000000..0457a5326 --- /dev/null +++ b/lib/crates/fabro-config/src/logging.rs @@ -0,0 +1,49 @@ +use anyhow::{Context, Result}; +use fabro_static::EnvVars; +use fabro_types::settings::server::LogDestination; + +pub fn resolve_log_destination(config_destination: LogDestination) -> Result { + let env_value = std::env::var(EnvVars::FABRO_LOG_DESTINATION).ok(); + resolve_log_destination_with_env(config_destination, env_value.as_deref()) +} + +pub fn resolve_log_destination_with_env( + config_destination: LogDestination, + env_value: Option<&str>, +) -> Result { + match env_value { + Some(value) => value.parse::().with_context(|| { + format!( + "invalid {} value `{value}`; expected `file` or `stdout`", + EnvVars::FABRO_LOG_DESTINATION + ) + }), + None => Ok(config_destination), + } +} + +#[cfg(test)] +mod tests { + use fabro_static::EnvVars; + use fabro_types::settings::server::LogDestination; + + use super::resolve_log_destination_with_env; + + #[test] + fn env_destination_overrides_config_destination() { + let destination = + resolve_log_destination_with_env(LogDestination::File, Some("stdout")).unwrap(); + + assert_eq!(destination, LogDestination::Stdout); + } + + #[test] + fn invalid_env_destination_is_reported() { + let err = resolve_log_destination_with_env(LogDestination::File, Some("stdot")) + .expect_err("invalid destination should fail"); + + let message = err.to_string(); + assert!(message.contains(EnvVars::FABRO_LOG_DESTINATION)); + assert!(message.contains("stdot")); + } +} diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index f02bdddf4..ae1c60d33 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -3917,7 +3917,7 @@ fn worker_command( let server_target = daemon.bind.to_target(); let worker_token = issue_worker_token(state.worker_token_keys(), &run_id) .map_err(|_| anyhow::anyhow!("failed to sign worker token"))?; - let server_destination = resolved_log_destination(state); + let server_destination = resolved_log_destination(state)?; let worker_stdout = match server_destination { LogDestination::Stdout => Stdio::inherit(), LogDestination::File => Stdio::null(), @@ -3944,10 +3944,8 @@ fn worker_command( cmd.env(EnvVars::FABRO_LOG, level); } } - if (state.env_lookup)(EnvVars::FABRO_LOG_DESTINATION).is_none() { - let value: &'static str = server_destination.into(); - cmd.env(EnvVars::FABRO_LOG_DESTINATION, value); - } + let value: &'static str = server_destination.into(); + cmd.env(EnvVars::FABRO_LOG_DESTINATION, value); cmd.env_remove(EnvVars::FABRO_WORKER_TOKEN); cmd.env(EnvVars::FABRO_WORKER_TOKEN, worker_token); @@ -3957,11 +3955,12 @@ fn worker_command( Ok(cmd) } -fn resolved_log_destination(state: &AppState) -> LogDestination { - (state.env_lookup)(EnvVars::FABRO_LOG_DESTINATION) - .as_deref() - .and_then(|raw| raw.parse::().ok()) - .unwrap_or(state.server_settings().server.logging.destination) +fn resolved_log_destination(state: &AppState) -> anyhow::Result { + let env_value = (state.env_lookup)(EnvVars::FABRO_LOG_DESTINATION); + fabro_config::resolve_log_destination_with_env( + state.server_settings().server.logging.destination, + env_value.as_deref(), + ) } fn api_question_type(question_type: InterviewQuestionType) -> ApiQuestionType { @@ -9173,6 +9172,95 @@ level = "debug" ); } + #[cfg(unix)] + #[test] + fn worker_command_sets_fabro_log_destination_from_server_logging_config() { + let storage_dir = tempfile::tempdir().unwrap(); + let state = worker_command_test_state_with_extra_config( + storage_dir.path(), + &["dev-token"], + Some(TEST_DEV_TOKEN), + r#" +[server.logging] +destination = "stdout" +"#, + ); + let run_id = RunId::new(); + + let cmd = worker_command( + state.as_ref(), + run_id, + RunExecutionMode::Start, + storage_dir.path(), + ) + .unwrap(); + + assert_eq!( + command_env_value(&cmd, EnvVars::FABRO_LOG_DESTINATION), + EnvOverride::Set("stdout".to_string()) + ); + } + + #[cfg(unix)] + #[test] + fn worker_command_env_log_destination_overrides_server_logging_config() { + let storage_dir = tempfile::tempdir().unwrap(); + let state = worker_command_test_state_with_extra_config_and_env_lookup( + storage_dir.path(), + &["dev-token"], + Some(TEST_DEV_TOKEN), + r#" +[server.logging] +destination = "file" +"#, + |name| (name == EnvVars::FABRO_LOG_DESTINATION).then(|| "stdout".to_string()), + ); + let run_id = RunId::new(); + + let cmd = worker_command( + state.as_ref(), + run_id, + RunExecutionMode::Start, + storage_dir.path(), + ) + .unwrap(); + + assert_eq!( + command_env_value(&cmd, EnvVars::FABRO_LOG_DESTINATION), + EnvOverride::Set("stdout".to_string()) + ); + } + + #[cfg(unix)] + #[test] + fn worker_command_rejects_invalid_env_log_destination() { + let storage_dir = tempfile::tempdir().unwrap(); + let state = worker_command_test_state_with_extra_config_and_env_lookup( + storage_dir.path(), + &["dev-token"], + Some(TEST_DEV_TOKEN), + r#" +[server.logging] +destination = "file" +"#, + |name| (name == EnvVars::FABRO_LOG_DESTINATION).then(|| "stdot".to_string()), + ); + let run_id = RunId::new(); + + let Err(err) = worker_command( + state.as_ref(), + run_id, + RunExecutionMode::Start, + storage_dir.path(), + ) else { + panic!("invalid env destination should fail"); + }; + + let message = err.to_string(); + assert!(message.contains(EnvVars::FABRO_LOG_DESTINATION)); + assert!(message.contains("stdot")); + } + #[test] fn build_app_state_requires_session_secret_for_worker_tokens() { let server_settings = server_settings_from_toml( @@ -9224,6 +9312,22 @@ methods = ["dev-token"] methods: &[&str], dev_token: Option<&str>, extra_config: &str, + ) -> Arc { + worker_command_test_state_with_extra_config_and_env_lookup( + storage_dir, + methods, + dev_token, + extra_config, + |_| None, + ) + } + + fn worker_command_test_state_with_extra_config_and_env_lookup( + storage_dir: &Path, + methods: &[&str], + dev_token: Option<&str>, + extra_config: &str, + env_lookup: impl Fn(&str) -> Option + Send + Sync + 'static, ) -> Arc { let dev_token = dev_token.map(str::to_owned); std::fs::create_dir_all(storage_dir).unwrap(); @@ -9264,7 +9368,7 @@ allowed_usernames = ["octocat"] server_settings_from_toml(&source), manifest_run_defaults_from_toml(&source), 5, - |_| None, + env_lookup, &server_secret_env, ) } diff --git a/lib/crates/fabro-server/src/spawn_env.rs b/lib/crates/fabro-server/src/spawn_env.rs index f4ab6ae49..934729af8 100644 --- a/lib/crates/fabro-server/src/spawn_env.rs +++ b/lib/crates/fabro-server/src/spawn_env.rs @@ -11,7 +11,6 @@ const WORKER_ENV_ALLOWLIST: &[&str] = &[ EnvVars::RUST_LOG, EnvVars::RUST_BACKTRACE, EnvVars::FABRO_LOG, - EnvVars::FABRO_LOG_DESTINATION, EnvVars::FABRO_HOME, EnvVars::FABRO_STORAGE_ROOT, ]; @@ -78,6 +77,7 @@ mod tests { ("USER".to_string(), "alice".to_string()), ("RUST_LOG".to_string(), "debug".to_string()), ("FABRO_LOG".to_string(), "debug".to_string()), + ("FABRO_LOG_DESTINATION".to_string(), "stdout".to_string()), ("FABRO_HOME".to_string(), "/tmp/fabro-home".to_string()), ( "FABRO_STORAGE_ROOT".to_string(), @@ -106,6 +106,7 @@ mod tests { assert_eq!(actual.get("PATH").map(String::as_str), Some("/bin")); assert_eq!(actual.get("HOME").map(String::as_str), Some("/tmp/home")); assert_eq!(actual.get("FABRO_LOG").map(String::as_str), Some("debug")); + assert!(!actual.contains_key("FABRO_LOG_DESTINATION")); assert_eq!( actual.get("FABRO_DEV_TOKEN").map(String::as_str), Some("fabro_dev_abababababababababababababababababababababababababababababababab")