From 71b08b61a190f8b54d1bdcd5a7c78a8f0a4c6729 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 25 Sep 2026 11:56:59 -0400 Subject: [PATCH] Preserve custom local artifact roots when overriding storage --- .../administration/server-configuration.mdx | 4 ++ .../fabro-cli/tests/it/scenario/artifacts.rs | 23 +++++--- lib/apps/fabro-server/src/install.rs | 11 ++-- lib/apps/fabro-server/src/serve.rs | 52 ++++++++++++++++++- lib/foundation/fabro-types/src/dense.rs | 24 +++------ 5 files changed, 85 insertions(+), 29 deletions(-) diff --git a/docs/public/administration/server-configuration.mdx b/docs/public/administration/server-configuration.mdx index 48315c0e6..d6850d3d2 100644 --- a/docs/public/administration/server-configuration.mdx +++ b/docs/public/administration/server-configuration.mdx @@ -247,6 +247,10 @@ prefix = "artifacts" root = "/var/lib/fabro/objects" ``` +A custom local artifact root is preserved at startup and when `--storage-dir` +changes the server's storage directory. When `local.root` is omitted, the +default `/objects/artifacts` location follows that override. + The wizard only covers AWS S3 bucket/region plus one of: - runtime credentials already supplied by the deployment environment diff --git a/lib/apps/fabro-cli/tests/it/scenario/artifacts.rs b/lib/apps/fabro-cli/tests/it/scenario/artifacts.rs index 81f327746..952a99621 100644 --- a/lib/apps/fabro-cli/tests/it/scenario/artifacts.rs +++ b/lib/apps/fabro-cli/tests/it/scenario/artifacts.rs @@ -16,14 +16,14 @@ async fn artifact_worker_captures_large_files_in_the_configured_local_store() { return; } let context = test_context!(); - let server = RunningServer::start_with( - "\n[server.artifacts]\nprovider = \"local\"\nprefix = \"selected-prefix\"\n", - &[], - ) - .await; - // RunningServer explicitly selects --storage-dir, which also selects the - // local artifact root. Inspect that resolved backend, outside the sandbox. - let objects = server.storage_dir.join("objects/artifacts"); + let objects = context.temp_dir.join("selected-artifact-root"); + let settings = format!( + "\n[server.artifacts]\nprovider = \"local\"\nprefix = \"selected-prefix\"\n[server.artifacts.local]\nroot = {:?}\n", + objects.to_str().unwrap() + ); + // RunningServer also passes --storage-dir. The explicit artifact directory + // must still receive the captures, independently of the database root. + let server = RunningServer::start_with(&settings, &[]).await; let workspace = artifact_workspace(&context); tokio::fs::write(workspace.join("workflow.fabro"), r#"digraph Capture { graph [goal="Capture binary files", default_max_retries=0] @@ -58,6 +58,13 @@ async fn artifact_worker_captures_large_files_in_the_configured_local_store() { serde_json::to_value(rebuilt.unwrap().artifacts).unwrap(), projection["artifacts"] ); + assert!( + !server + .storage_dir + .join(format!("objects/artifacts/selected-prefix/{run_id}")) + .exists(), + "captures must not be redirected to the default artifact directory" + ); for (path, size) in [ ("medium.bin", 3 * 1024 * 1024), diff --git a/lib/apps/fabro-server/src/install.rs b/lib/apps/fabro-server/src/install.rs index 3d3ee4547..49a3416a6 100644 --- a/lib/apps/fabro-server/src/install.rs +++ b/lib/apps/fabro-server/src/install.rs @@ -2262,8 +2262,7 @@ async fn write_artifact_store_metadata( settings: &ServerSettings, storage_dir: &Path, ) -> anyhow::Result<()> { - let mut settings = settings.clone(); - settings.server.storage.root = storage_dir.display().to_string(); + let settings = settings.clone().with_storage_override(storage_dir); let (object_store, prefix) = serve::build_artifact_object_store(&settings.server)?; let artifact_store = ArtifactStore::new(object_store, prefix); artifact_store.write_metadata(FABRO_VERSION).await?; @@ -2596,8 +2595,12 @@ methods = ["dev-token"] .await .unwrap(); - let mut overridden = settings.clone(); - overridden.server.storage.root = dir.path().display().to_string(); + assert!( + dir.path() + .join("objects/artifacts/store-metadata.json") + .is_file() + ); + let overridden = settings.clone().with_storage_override(dir.path()); let (object_store, prefix) = crate::serve::build_artifact_object_store(&overridden.server).unwrap(); let marker = if prefix.is_empty() { diff --git a/lib/apps/fabro-server/src/serve.rs b/lib/apps/fabro-server/src/serve.rs index 3b969de84..57f2c15b2 100644 --- a/lib/apps/fabro-server/src/serve.rs +++ b/lib/apps/fabro-server/src/serve.rs @@ -1151,7 +1151,7 @@ fn server_bind_title(bind: &Bind) -> String { )] mod tests { use std::io; - use std::path::PathBuf; + use std::path::{Path, PathBuf}; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; use std::task::Poll; @@ -1347,6 +1347,56 @@ mod tests { assert_eq!(root, "/srv/fabro-storage/objects/artifacts"); } + #[test] + fn runtime_server_settings_preserve_custom_artifact_root() { + let settings = server_settings( + r#" +_version = 1 +[server.storage] +root = "/srv/from-disk" +[server.artifacts] +prefix = "selected-prefix" +[server.artifacts.local] +root = "/mnt/artifact-files" +"#, + ); + for storage_root in ["/srv/from-disk", "/srv/from-runtime"] { + let resolved = settings + .clone() + .with_storage_override(Path::new(storage_root)); + assert_eq!(resolved.server.storage.root, storage_root); + assert_eq!(resolved.server.artifacts, settings.server.artifacts); + // Startup and config reload can apply the same override again. + assert_eq!( + resolved + .clone() + .with_storage_override(Path::new(storage_root)), + resolved + ); + } + } + + #[test] + fn runtime_server_settings_preserve_s3_artifact_configuration() { + let settings = server_settings( + r#" +_version = 1 +[server.artifacts] +provider = "s3" +prefix = "selected-prefix" +[server.artifacts.s3] +bucket = "artifact-bucket" +region = "us-east-1" +endpoint = "https://objects.example.test" +path_style = true +"#, + ); + let resolved = settings + .clone() + .with_storage_override(Path::new("/srv/from-runtime")); + assert_eq!(resolved.server.artifacts, settings.server.artifacts); + } + #[test] fn runtime_server_settings_keep_disk_defaults_out_of_manifest_defaults() { let mut resolved = resolved_runtime_settings( diff --git a/lib/foundation/fabro-types/src/dense.rs b/lib/foundation/fabro-types/src/dense.rs index c3355b3b4..4be1bc1c6 100644 --- a/lib/foundation/fabro-types/src/dense.rs +++ b/lib/foundation/fabro-types/src/dense.rs @@ -16,27 +16,19 @@ pub struct ServerSettings { impl ServerSettings { #[must_use] pub fn with_storage_override(mut self, path: &Path) -> Self { + // Only the derived default follows the storage directory. A custom + // artifact location is independent of the database and runtime root. + let default_artifact_root = Path::new(&self.server.storage.root).join("objects/artifacts"); + if let ObjectStoreSettings::Local { root } = &mut self.server.artifacts.store { + if Path::new(root) == default_artifact_root { + *root = path.join("objects/artifacts").display().to_string(); + } + } self.server.storage.root = path.display().to_string(); - override_local_object_store_root(&mut self.server.artifacts.store, path, "artifacts"); self } } -fn override_local_object_store_root( - store: &mut ObjectStoreSettings, - storage_root: &Path, - domain: &str, -) { - let ObjectStoreSettings::Local { root } = store else { - return; - }; - *root = storage_root - .join("objects") - .join(domain) - .display() - .to_string(); -} - #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] pub struct UserSettings { pub cli: CliNamespace,