From 74dfb9f652d9cbc45f492907e105d456d68bf503 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 14 Apr 2026 13:08:07 -0400 Subject: [PATCH] Restructure local object store layout --- lib/crates/fabro-cli/src/commands/install.rs | 74 +++++++++++- lib/crates/fabro-config/src/defaults.toml | 2 +- lib/crates/fabro-config/src/resolve/server.rs | 10 +- lib/crates/fabro-config/src/storage.rs | 25 ++-- .../fabro-config/tests/resolve_server.rs | 22 +++- lib/crates/fabro-server/src/serve.rs | 113 +++++++++++------- lib/crates/fabro-store/src/artifact_store.rs | 33 +++++ lib/crates/fabro-store/src/slate/mod.rs | 4 +- lib/crates/fabro-types/src/settings/server.rs | 2 +- 9 files changed, 226 insertions(+), 59 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 9a3ba2ef1..83507c363 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -16,12 +16,15 @@ use dialoguer::{MultiSelect, Select}; use fabro_api::types::{CreateSecretRequest, SecretType as ApiSecretType}; use fabro_auth::{AuthCredential, AuthMethod, codex_oauth_config, credential_id_for}; use fabro_config::user::SETTINGS_CONFIG_FILENAME; -use fabro_config::{Storage, envfile, legacy_env}; +use fabro_config::{ResolveError, Storage, envfile, legacy_env}; use fabro_model::Provider; -use fabro_types::settings::CliSettings; +use fabro_server::serve; +use fabro_store::ArtifactStore; +use fabro_types::settings::{CliSettings, SettingsLayer}; use fabro_types::settings::cli::{CliLayer, OutputFormat}; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; +use fabro_util::version::FABRO_VERSION; use fabro_util::{dev_token, session_secret}; use futures::future::BoxFuture; use rand::Rng; @@ -1190,6 +1193,29 @@ async fn persist_install_outputs( .await } +fn render_server_resolve_errors(errors: Vec) -> anyhow::Error { + anyhow::anyhow!( + "failed to resolve server settings:\n{}", + errors + .into_iter() + .map(|error| error.to_string()) + .collect::>() + .join("\n") + ) +} + +async fn write_artifact_store_metadata( + settings: &SettingsLayer, + fabro_version: &str, +) -> Result<()> { + let resolved = + fabro_config::resolve_server_from_file(settings).map_err(render_server_resolve_errors)?; + let (object_store, prefix) = serve::build_artifact_object_store(&resolved)?; + let artifact_store = ArtifactStore::new(object_store, prefix); + artifact_store.write_metadata(fabro_version).await?; + Ok(()) +} + async fn persist_install_outputs_with_settings( storage_dir: &Path, server_env_secrets: &[(String, String)], @@ -1505,6 +1531,18 @@ async fn run_install_inner( server_was_running, ) .await?; + let install_settings = user_config::apply_storage_dir_override( + fabro_config::parse_settings_layer(&settings_toml) + .context("failed to parse generated settings.toml")?, + args.storage_dir.as_deref(), + ); + if let Err(err) = write_artifact_store_metadata(&install_settings, FABRO_VERSION).await { + fabro_util::printerr!( + printer, + " {} failed to write artifact store metadata: {err}", + s.yellow.apply_to("Warning:") + ); + } fabro_util::printerr!( printer, " {} Saved {} runtime secrets to {}", @@ -2118,6 +2156,38 @@ client_id = "client-id" ); } + #[tokio::test] + async fn write_artifact_store_metadata_creates_marker_in_resolved_store() { + let dir = tempfile::tempdir().unwrap(); + let settings = fabro_config::parse_settings_layer(&format!( + r#" +_version = 1 + +[server.storage] +root = "{}" +"#, + dir.path().display() + )) + .unwrap(); + + write_artifact_store_metadata(&settings, "test-version") + .await + .unwrap(); + + let value: serde_json::Value = serde_json::from_str( + &std::fs::read_to_string( + dir.path() + .join("objects") + .join("artifacts") + .join("store-metadata.json"), + ) + .unwrap(), + ) + .unwrap(); + assert_eq!(value["fabro_version"], "test-version"); + assert!(value["created_at"].as_str().is_some()); + } + #[test] fn non_interactive_source_rejects_missing_scripted_inputs() { let args = install_args(true, InstallNonInteractiveArgs::default()); diff --git a/lib/crates/fabro-config/src/defaults.toml b/lib/crates/fabro-config/src/defaults.toml index 60c3c7941..6eb4c77af 100644 --- a/lib/crates/fabro-config/src/defaults.toml +++ b/lib/crates/fabro-config/src/defaults.toml @@ -46,7 +46,7 @@ max_concurrent_runs = 5 [server.artifacts] provider = "local" -prefix = "artifacts" +prefix = "" [server.slatedb] provider = "local" diff --git a/lib/crates/fabro-config/src/resolve/server.rs b/lib/crates/fabro-config/src/resolve/server.rs index ae6d9f47e..f50cd658c 100644 --- a/lib/crates/fabro-config/src/resolve/server.rs +++ b/lib/crates/fabro-config/src/resolve/server.rs @@ -166,7 +166,7 @@ fn resolve_artifacts( provider, layer.and_then(|artifacts| artifacts.local.as_ref()), layer.and_then(|artifacts| artifacts.s3.as_ref()), - storage_root, + &object_store_default_root(storage_root, "artifacts"), "server.artifacts", errors, ), @@ -190,7 +190,7 @@ fn resolve_slatedb( provider, layer.and_then(|slatedb| slatedb.local.as_ref()), layer.and_then(|slatedb| slatedb.s3.as_ref()), - storage_root, + &object_store_default_root(storage_root, "slatedb"), "server.slatedb", errors, ), @@ -236,6 +236,12 @@ fn resolve_object_store( } } +fn object_store_default_root(storage_root: &InterpString, domain: &str) -> InterpString { + let root = storage_root.as_source(); + let root = root.trim_end_matches('/'); + InterpString::parse(&format!("{root}/objects/{domain}")) +} + fn resolve_integrations(layer: Option<&ServerIntegrationsLayer>) -> ServerIntegrationsSettings { ServerIntegrationsSettings { github: layer diff --git a/lib/crates/fabro-config/src/storage.rs b/lib/crates/fabro-config/src/storage.rs index 2bf960763..0aad99eab 100644 --- a/lib/crates/fabro-config/src/storage.rs +++ b/lib/crates/fabro-config/src/storage.rs @@ -58,13 +58,18 @@ impl Storage { } #[must_use] - pub fn store_dir(&self) -> PathBuf { - self.root.join("store") + pub fn objects_dir(&self) -> PathBuf { + self.root.join("objects") } #[must_use] - pub fn artifact_store_dir(&self) -> PathBuf { - self.root.join("artifacts") + pub fn slatedb_dir(&self) -> PathBuf { + self.objects_dir().join("slatedb") + } + + #[must_use] + pub fn artifacts_dir(&self) -> PathBuf { + self.objects_dir().join("artifacts") } } @@ -164,12 +169,16 @@ mod tests { std::path::Path::new("/tmp/fabro-data/vaults/default/secrets.json") ); assert_eq!( - storage.store_dir(), - std::path::Path::new("/tmp/fabro-data/store") + storage.objects_dir(), + std::path::Path::new("/tmp/fabro-data/objects") ); assert_eq!( - storage.artifact_store_dir(), - std::path::Path::new("/tmp/fabro-data/artifacts") + storage.slatedb_dir(), + std::path::Path::new("/tmp/fabro-data/objects/slatedb") + ); + assert_eq!( + storage.artifacts_dir(), + std::path::Path::new("/tmp/fabro-data/objects/artifacts") ); assert_eq!( storage.server_state().record_path(), diff --git a/lib/crates/fabro-config/tests/resolve_server.rs b/lib/crates/fabro-config/tests/resolve_server.rs index f4563387e..45d96a7f1 100644 --- a/lib/crates/fabro-config/tests/resolve_server.rs +++ b/lib/crates/fabro-config/tests/resolve_server.rs @@ -36,12 +36,30 @@ fn resolves_server_defaults_from_empty_settings() { ObjectStoreSettings::Local { root } => { assert_eq!( root.as_source(), - Home::from_env().storage_dir().to_string_lossy() + Home::from_env() + .storage_dir() + .join("objects") + .join("artifacts") + .to_string_lossy() ); } ObjectStoreSettings::S3 { .. } => panic!("expected local artifact store by default"), } - assert_eq!(settings.artifacts.prefix.as_source(), "artifacts"); + assert_eq!(settings.artifacts.prefix.as_source(), ""); + + match settings.slatedb.store { + ObjectStoreSettings::Local { root } => { + assert_eq!( + root.as_source(), + Home::from_env() + .storage_dir() + .join("objects") + .join("slatedb") + .to_string_lossy() + ); + } + ObjectStoreSettings::S3 { .. } => panic!("expected local slatedb store by default"), + } } #[test] diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index a358ecf3f..8a1f3a944 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -143,7 +143,7 @@ fn use_in_memory_store() -> bool { ) } -fn build_object_store_with_preference( +fn build_local_object_store_with_preference( store_path: &Path, use_in_memory: bool, ) -> anyhow::Result> { @@ -155,8 +155,33 @@ fn build_object_store_with_preference( Ok(Arc::new(LocalFileSystem::new_with_prefix(store_path)?)) } -fn build_object_store(store_path: &Path) -> anyhow::Result> { - build_object_store_with_preference(store_path, use_in_memory_store()) +fn build_object_store_from_settings( + settings: &ObjectStoreSettings, +) -> anyhow::Result> { + if use_in_memory_store() { + return Ok(Arc::new(InMemory::new())); + } + + match settings { + ObjectStoreSettings::Local { root } => { + build_local_object_store_with_preference(&resolve_interp_path(root)?, false) + } + ObjectStoreSettings::S3 { + bucket, + region, + endpoint, + path_style, + } => { + let mut builder = AmazonS3Builder::from_env() + .with_bucket_name(resolve_interp(bucket)?) + .with_region(resolve_interp(region)?) + .with_virtual_hosted_style_request(!*path_style); + if let Some(endpoint) = endpoint.as_ref() { + builder = builder.with_endpoint(resolve_interp(endpoint)?); + } + Ok(Arc::new(builder.build()?)) + } + } } fn resolve_server_settings(file: &SettingsLayer) -> anyhow::Result { @@ -228,39 +253,20 @@ fn resolve_interp_path(value: &InterpString) -> anyhow::Result { Ok(PathBuf::from(resolve_interp(value)?)) } -fn build_artifact_object_store( +pub fn build_artifact_object_store( settings: &ResolvedServerSettings, ) -> anyhow::Result<(Arc, String)> { let prefix = resolve_interp(&settings.artifacts.prefix)?; + let object_store = build_object_store_from_settings(&settings.artifacts.store)?; + Ok((object_store, prefix)) +} - if use_in_memory_store() { - return Ok((Arc::new(InMemory::new()), prefix)); - } - - match &settings.artifacts.store { - ObjectStoreSettings::Local { root } => { - let root = resolve_interp_path(root)?; - std::fs::create_dir_all(&root)?; - let object_store = Arc::new(LocalFileSystem::new_with_prefix(&root)?); - Ok((object_store, prefix)) - } - ObjectStoreSettings::S3 { - bucket, - region, - endpoint, - path_style, - } => { - let mut builder = AmazonS3Builder::from_env() - .with_bucket_name(resolve_interp(bucket)?) - .with_region(resolve_interp(region)?) - .with_virtual_hosted_style_request(!*path_style); - if let Some(endpoint) = endpoint.as_ref() { - builder = builder.with_endpoint(resolve_interp(endpoint)?); - } - let object_store = Arc::new(builder.build()?); - Ok((object_store, prefix)) - } - } +fn build_slatedb_store( + settings: &ResolvedServerSettings, +) -> anyhow::Result<(Arc, String, Duration)> { + let prefix = resolve_interp(&settings.slatedb.prefix)?; + let object_store = build_object_store_from_settings(&settings.slatedb.store)?; + Ok((object_store, prefix, settings.slatedb.flush_interval)) } /// Start the HTTP API server. @@ -310,12 +316,12 @@ where }; let web_enabled = router_web_enabled(&resolved_server_settings); - let store_path = storage.store_dir(); - let object_store = build_object_store(&store_path)?; + let (object_store, slatedb_prefix, flush_interval) = + build_slatedb_store(&resolved_server_settings)?; let store = Arc::new(fabro_store::Database::new( - Arc::clone(&object_store), - "", - Duration::from_millis(1), + object_store, + slatedb_prefix, + flush_interval, )); let (artifact_object_store, artifact_prefix) = build_artifact_object_store(&resolved_server_settings)?; @@ -653,6 +659,7 @@ fn server_bind_title(bind: &Bind) -> String { #[cfg(test)] mod tests { use std::path::PathBuf; + use std::time::Duration; use fabro_config::parse_settings_layer; use fabro_types::settings::SettingsLayer; @@ -660,8 +667,9 @@ mod tests { use super::{ ServeArgs, ServerTitlePhase, apply_runtime_settings, bind_tcp_host_with_fallback, - build_object_store_with_preference, resolve_bind_request_from_settings, - resolve_server_settings, router_web_enabled, server_bind_title, server_title, + build_local_object_store_with_preference, build_slatedb_store, + resolve_bind_request_from_settings, resolve_server_settings, router_web_enabled, + server_bind_title, server_title, }; use crate::bind::{Bind, BindRequest}; @@ -851,7 +859,7 @@ strategy = "token" let temp = tempfile::tempdir().unwrap(); let store_path = temp.path().join("store"); - let disk_store = build_object_store_with_preference(&store_path, false) + let disk_store = build_local_object_store_with_preference(&store_path, false) .expect("disk-backed store should build"); assert!( store_path.exists(), @@ -860,7 +868,7 @@ strategy = "token" drop(disk_store); let mem_path = temp.path().join("memory-store"); - let mem_store = build_object_store_with_preference(&mem_path, true) + let mem_store = build_local_object_store_with_preference(&mem_path, true) .expect("memory-backed store should build"); assert!( !mem_path.exists(), @@ -869,6 +877,29 @@ strategy = "token" drop(mem_store); } + #[test] + fn build_slatedb_store_uses_configured_local_root() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("custom-slatedb"); + let settings = parse_settings(&format!( + r#" +_version = 1 + +[server.slatedb.local] +root = "{}" +"#, + root.display() + )); + + let resolved = resolve_server_settings(&settings).expect("settings should resolve"); + let (_object_store, prefix, flush_interval) = + build_slatedb_store(&resolved).expect("slatedb store should build"); + + assert!(root.exists(), "configured SlateDB root should be created"); + assert_eq!(prefix, ""); + assert_eq!(flush_interval, Duration::from_millis(1)); + } + #[tokio::test] async fn tcp_host_request_uses_preferred_port_when_available() { let preferred = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); diff --git a/lib/crates/fabro-store/src/artifact_store.rs b/lib/crates/fabro-store/src/artifact_store.rs index eb5df75fb..3f3e72afb 100644 --- a/lib/crates/fabro-store/src/artifact_store.rs +++ b/lib/crates/fabro-store/src/artifact_store.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use bytes::Bytes; +use chrono::Utc; use fabro_types::RunId; use futures::StreamExt; use object_store::ObjectStore; @@ -146,6 +147,19 @@ impl ArtifactStore { Ok(()) } + pub async fn write_metadata(&self, fabro_version: &str) -> Result<()> { + let path = parse_object_path(&self.prefixed_raw("store-metadata.json"))?; + let body = serde_json::to_vec(&serde_json::json!({ + "created_at": Utc::now().to_rfc3339(), + "fabro_version": fabro_version, + })) + .map_err(|err| Error::Other(format!("artifact metadata serialization failed: {err}")))?; + self.object_store + .put(&path, Bytes::from(body).into()) + .await?; + Ok(()) + } + fn run_prefix(&self, run_id: &RunId) -> Result { parse_object_path(&self.prefixed_raw(&run_id.to_string())) } @@ -286,6 +300,25 @@ mod tests { ArtifactStore::new(object_store, "artifacts") } + #[tokio::test] + async fn write_metadata_persists_store_marker() { + let object_store: Arc = Arc::new(InMemory::new()); + let store = ArtifactStore::new(object_store.clone(), "artifacts"); + + store.write_metadata("test-version").await.unwrap(); + + let bytes = object_store + .get(&ObjectPath::from("artifacts/store-metadata.json")) + .await + .unwrap() + .bytes() + .await + .unwrap(); + let value: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(value["fabro_version"], "test-version"); + assert!(value["created_at"].as_str().is_some()); + } + #[tokio::test] async fn round_trips_unicode_nodes_and_nested_filenames() { let store = test_store(); diff --git a/lib/crates/fabro-store/src/slate/mod.rs b/lib/crates/fabro-store/src/slate/mod.rs index bb4d95f57..699283c5c 100644 --- a/lib/crates/fabro-store/src/slate/mod.rs +++ b/lib/crates/fabro-store/src/slate/mod.rs @@ -48,7 +48,7 @@ impl Database { } fn shared_db_prefix(&self) -> String { - format!("{}slatedb", self.base_prefix) + self.base_prefix.clone() } async fn open_db(&self) -> Result { @@ -399,7 +399,7 @@ mod tests { let remaining = store.list_runs(&ListRunsQuery::default()).await.unwrap(); assert_eq!(remaining.len(), 1); assert_eq!(remaining[0].run_id, test_run_id("run-2")); - assert!(!list_paths(object_store, "runs/slatedb").await.is_empty()); + assert!(!list_paths(object_store, "runs/").await.is_empty()); } #[tokio::test] diff --git a/lib/crates/fabro-types/src/settings/server.rs b/lib/crates/fabro-types/src/settings/server.rs index 29ec55931..683a04d06 100644 --- a/lib/crates/fabro-types/src/settings/server.rs +++ b/lib/crates/fabro-types/src/settings/server.rs @@ -388,7 +388,7 @@ pub enum ObjectStoreProvider { #[serde(deny_unknown_fields)] pub struct ObjectStoreLocalLayer { /// Overrides the default root, which otherwise falls back to - /// `server.storage.root`. + /// `{server.storage.root}/objects/{domain}`. #[serde(default, skip_serializing_if = "Option::is_none")] pub root: Option, }