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 {