mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
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 <scratch>/runtime/server.log stays a file regardless --
it is read back by the run UI.
A CLI-side ServerLogSink::{File(PathBuf),Stdout} replaces Option<PathBuf>
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) <noreply@anthropic.com>
This commit is contained in:
parent
3a6ef48e56
commit
7abc11adc9
5 changed files with 113 additions and 18 deletions
|
|
@ -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<PathBuf>,
|
||||
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(())
|
||||
|
|
|
|||
|
|
@ -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<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 = 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();
|
||||
|
|
|
|||
|
|
@ -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::<LogDestination>().ok())
|
||||
.unwrap_or(state.server_settings().server.logging.destination)
|
||||
}
|
||||
|
||||
fn api_question_type(question_type: InterviewQuestionType) -> ApiQuestionType {
|
||||
match question_type {
|
||||
InterviewQuestionType::YesNo => ApiQuestionType::YesNo,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
];
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue