scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-12 23:02:41 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

Merge remote-tracking branch 'origin/main'

Browse source
This commit is contained in:
Bryan Helmkamp 2026-04-26 18:00:53 -04:00
commit 9587b83cfe
No known key found for this signature in database
8 changed files with 273 additions and 45 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
lib/crates/fabro-cli/src/commands/server/start.rs
View file

@ -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`"

53
lib/crates/fabro-cli/src/logging.rs
View file

@ -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;
@ -16,16 +15,20 @@ use tracing_subscriber::{EnvFilter, fmt};
const LOG_RETENTION_DAYS: u32 = 7;
#[derive(Clone, Debug, PartialEq, Eq)]
pub(crate) enum LogSink {
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<PathBuf>,
log: LogSink,
},
Worker {
server_log_path: PathBuf,
server_log: LogSink,
per_run_log_path: PathBuf,
},
}
@ -62,15 +65,17 @@ pub(crate) fn init_tracing(
init_subscriber(filter, file_appender);
}
InternalLogSink::Server {
log_path: Some(path),
log: LogSink::File(path),
} => {
init_subscriber(filter, open_buffered_appender(path)?);
}
InternalLogSink::Server { log_path: None } => {
InternalLogSink::Server {
log: LogSink::Stdout,
} => {
init_subscriber(filter, std::io::stdout);
}
InternalLogSink::Worker {
server_log_path,
server_log: LogSink::File(server_log_path),
per_run_log_path,
} => {
init_worker_subscriber(
@ -79,33 +84,21 @@ pub(crate) fn init_tracing(
open_buffered_appender(per_run_log_path)?,
);
}
InternalLogSink::Worker {
server_log: LogSink::Stdout,
per_run_log_path,
} => {
init_worker_subscriber(
filter,
std::io::stdout,
open_buffered_appender(per_run_log_path)?,
);
}
}
Ok(())
}
pub(crate) fn resolve_log_destination(
config_destination: LogDestination,
) -> Result<LogDestination> {
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<LogDestination> {
match env_value {
Some(value) => value.parse::<LogDestination>().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 {

67
lib/crates/fabro-cli/src/main.rs
View file

@ -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 {
@ -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 = 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<PreTracingBootstrap> {
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 = fabro_config::resolve_log_destination(
local_config.config_log_destination().unwrap_or_default(),
)?;
let server_log = 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 log_sink(
destination: LogDestination,
runtime_directory: &fabro_config::RuntimeDirectory,
) -> logging::LogSink {
match destination {
LogDestination::File => logging::LogSink::File(runtime_directory.log_path()),
LogDestination::Stdout => logging::LogSink::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::LogSink::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::LogSink::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::LogSink::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::LogSink::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::LogSink::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::LogSink::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();

2
lib/crates/fabro-config/src/lib.rs
View file

@ -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,

49
lib/crates/fabro-config/src/logging.rs Normal file
View file

@ -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<LogDestination> {
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<LogDestination> {
match env_value {
Some(value) => value.parse::<LogDestination>().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"));
}
}

128
lib/crates/fabro-server/src/server.rs
View file

@ -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,8 @@ fn worker_command(
cmd.env(EnvVars::FABRO_LOG, level);
}
}
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 +3955,14 @@ fn worker_command(
Ok(cmd)
}
fn resolved_log_destination(state: &AppState) -> anyhow::Result<LogDestination> {
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 {
match question_type {
InterviewQuestionType::YesNo => ApiQuestionType::YesNo,
@ -9155,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(
@ -9206,6 +9312,22 @@ methods = ["dev-token"]
methods: &[&str],
dev_token: Option<&str>,
extra_config: &str,
) -> Arc<AppState> {
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<String> + Send + Sync + 'static,
) -> Arc<AppState> {
let dev_token = dev_token.map(str::to_owned);
std::fs::create_dir_all(storage_dir).unwrap();
@ -9246,7 +9368,7 @@ allowed_usernames = ["octocat"]
server_settings_from_toml(&source),
manifest_run_defaults_from_toml(&source),
5,
|_| None,
env_lookup,
&server_secret_env,
)
}

2
lib/crates/fabro-server/src/spawn_env.rs
View file

@ -77,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(),
@ -105,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")

13
lib/crates/fabro-types/src/settings/server.rs
View file

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