diff --git a/apps/fabro-web/app/routes/settings-environments.tsx b/apps/fabro-web/app/routes/settings-environments.tsx index 509a8c0c5..e833c339b 100644 --- a/apps/fabro-web/app/routes/settings-environments.tsx +++ b/apps/fabro-web/app/routes/settings-environments.tsx @@ -20,10 +20,6 @@ import { import { ConfirmDialog } from "../components/ui"; import { useToast } from "../components/toast"; -// `default` is seeded and protected from deletion by the server; reflect that -// in the UI instead of letting the delete fail with a 409. -const PROTECTED_ID = "default"; - // `local` is a reserved, in-memory environment the server includes only when // the local sandbox provider is enabled. It has no configurable settings, so it // gets its own panel instead of a row in the managed environments list. @@ -237,7 +233,6 @@ function EnvironmentRow({ {environment.id} {environment.provider} - {environment.id === PROTECTED_ID ? protected : null}
{resourcesSummary(environment)} @@ -286,7 +281,6 @@ function RowMenu({ disabled: boolean; onDelete: () => void; }) { - const protectedFromDelete = environment.id === PROTECTED_ID; return ( - {protectedFromDelete ? "Protected" : "Delete"} + Delete diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index d956aa170..16b7f44cd 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -2012,6 +2012,22 @@ async fn run_install_inner(args: &InstallArgs, ctx: &CommandContext) -> Result<( dev_token_for_auth_store.as_deref(), ) .await?; + + // Seed the built-in environments next to the settings file. The server never + // seeds on startup, so install is the only place built-ins are written; + // existing files are preserved, so re-running install never clobbers edits. + let environment_dir = config_path + .parent() + .unwrap_or_else(|| Path::new(".")) + .join("environments"); + if let Err(err) = fabro_environment::seed_environments(&environment_dir) { + fabro_util::printerr!( + printer, + " {} Failed to seed built-in environments: {err}", + s.yellow.apply_to("Warning:") + ); + } + if let Some(token) = dev_token_for_auth_store { let user_settings = UserSettingsBuilder::from_toml(&settings_toml)?; let target = match user_config::resolve_nondefault_server_target( diff --git a/lib/crates/fabro-environment/src/error.rs b/lib/crates/fabro-environment/src/error.rs index 81b624f68..9adfbcecd 100644 --- a/lib/crates/fabro-environment/src/error.rs +++ b/lib/crates/fabro-environment/src/error.rs @@ -31,8 +31,6 @@ pub enum EnvironmentStoreError { expected: EnvironmentRevision, actual: EnvironmentRevision, }, - #[error("environment is protected and cannot be deleted: {id}")] - Protected { id: EnvironmentId }, #[error("environment is reserved and cannot be modified: {id}")] Reserved { id: EnvironmentId }, #[error("environment validation failed: {source}")] @@ -95,7 +93,6 @@ impl EnvironmentStoreError { Self::NotFound { .. } => "not_found", Self::AlreadyExists { .. } => "already_exists", Self::StaleRevision { .. } => "stale_revision", - Self::Protected { .. } => "protected", Self::Reserved { .. } => "reserved", Self::Validation { .. } => "validation", Self::InvalidFilename { .. } => "invalid_filename", diff --git a/lib/crates/fabro-environment/src/lib.rs b/lib/crates/fabro-environment/src/lib.rs index 9de628740..1846500bd 100644 --- a/lib/crates/fabro-environment/src/lib.rs +++ b/lib/crates/fabro-environment/src/lib.rs @@ -6,4 +6,4 @@ mod store; pub use error::{EnvironmentStoreError, EnvironmentValidationError}; pub use id::{EnvironmentId, EnvironmentRevision, EnvironmentRevisionParseError}; pub use model::{Environment, EnvironmentDraft}; -pub use store::{EnvironmentStore, seeded_catalog_layer}; +pub use store::{EnvironmentStore, seed_environments, seeded_catalog_layer}; diff --git a/lib/crates/fabro-environment/src/store.rs b/lib/crates/fabro-environment/src/store.rs index 6bc620c58..4486e6245 100644 --- a/lib/crates/fabro-environment/src/store.rs +++ b/lib/crates/fabro-environment/src/store.rs @@ -14,8 +14,11 @@ use crate::{ Environment, EnvironmentDraft, EnvironmentId, EnvironmentRevision, EnvironmentStoreError, }; -/// Environments written to disk on first startup. `local` is intentionally -/// absent: it is a reserved, in-memory environment (see [`RESERVED_LOCAL_ID`]). +/// Built-in environments written to disk by the installer (see +/// [`seed_environments`]). The server itself never seeds: a Fabro instance that +/// has not been installed has no managed environments, and a run that selects +/// an absent environment fails explicitly. `local` is intentionally absent: it +/// is a reserved, in-memory environment (see [`RESERVED_LOCAL_ID`]). const SEEDS: &[(&str, &str)] = &[ ("default", DEFAULT_ENVIRONMENT_TOML), ("docker", DOCKER_ENVIRONMENT_TOML), @@ -130,15 +133,19 @@ fn build_catalog_layer( } impl EnvironmentStore { - /// Synchronously seed missing built-in environment files and load all - /// persisted environments. The synchronous file access runs during server - /// startup before request handling begins. - pub fn load_or_seed( + /// Synchronously load all persisted environments. The synchronous file + /// access runs during server startup before request handling begins. + /// + /// The server never seeds built-in environments; seeding is an install-time + /// action (see [`seed_environments`]). An uninstalled instance therefore + /// has no managed environments on disk, and the reserved `local` + /// environment is the only entry present (when the local provider is + /// enabled). + pub fn load( dir: impl Into, local_enabled: bool, ) -> Result { let dir = dir.into(); - seed_missing_environments(&dir)?; let mut environments = load_environments(&dir)?; if local_enabled { let local = synthetic_local_environment()?; @@ -224,9 +231,9 @@ impl EnvironmentStore { id: &EnvironmentId, expected: &EnvironmentRevision, ) -> Result<(), EnvironmentStoreError> { - if id.as_str() == "default" { - return Err(EnvironmentStoreError::Protected { id: id.clone() }); - } + // `default` is an ordinary deletable environment. Deleting it removes the + // run fallback, which is intentional: a run that selects `default` after + // it is gone fails explicitly rather than silently using a built-in. if id.as_str() == RESERVED_LOCAL_ID { return Err(EnvironmentStoreError::Reserved { id: id.clone() }); } @@ -267,12 +274,16 @@ fn check_revision( Ok(()) } +/// Writes the built-in environment seeds (`default`, `docker`, `daytona`) into +/// `dir`, creating the directory if needed. Existing files are left untouched, +/// so this is idempotent and never clobbers operator edits. Called by the +/// installer; the running server does not seed. #[expect( clippy::disallowed_methods, clippy::disallowed_types, - reason = "Environment directory seeding runs synchronously during startup before request handling." + reason = "Install-time environment seeding runs synchronously from the installer before the server starts." )] -fn seed_missing_environments(dir: &Path) -> Result<(), EnvironmentStoreError> { +pub fn seed_environments(dir: &Path) -> Result<(), EnvironmentStoreError> { std::fs::create_dir_all(dir).map_err(|err| EnvironmentStoreError::io(dir, err))?; for (id, content) in SEEDS { let path = dir.join(format!("{id}.toml")); @@ -487,27 +498,68 @@ mod tests { } #[tokio::test] - async fn absent_directory_loads_and_seeds_built_ins() { + async fn load_does_not_seed_built_ins() { let dir = tempfile::tempdir().unwrap(); let environment_dir = dir.path().join("environments"); - let store = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap(); - let environments = store.list(); - - // `local` is present in memory (local provider enabled) but the other - // three are the on-disk seeds. + // The server loads without seeding: an uninstalled instance has only the + // reserved in-memory `local` environment, and nothing is written to disk. + let store = EnvironmentStore::load(&environment_dir, true).unwrap(); assert_eq!( - environments + store + .list() .iter() .map(|environment| environment.id.as_str()) .collect::>(), - vec!["daytona", "default", "docker", "local"] + vec!["local"] ); + for id in ["default", "docker", "daytona"] { + assert!(!environment_dir.join(format!("{id}.toml")).exists()); + } + } + + #[tokio::test] + async fn seed_environments_writes_built_ins_and_load_picks_them_up() { + let dir = tempfile::tempdir().unwrap(); + let environment_dir = dir.path().join("environments"); + + super::seed_environments(&environment_dir).unwrap(); for id in ["default", "docker", "daytona"] { assert!(environment_dir.join(format!("{id}.toml")).exists()); } // `local` is reserved and in-memory; it is never written to disk. assert!(!environment_dir.join("local.toml").exists()); + + let store = EnvironmentStore::load(&environment_dir, true).unwrap(); + assert_eq!( + store + .list() + .iter() + .map(|environment| environment.id.as_str()) + .collect::>(), + vec!["daytona", "default", "docker", "local"] + ); + } + + #[tokio::test] + async fn seed_environments_is_idempotent_and_preserves_edits() { + let dir = tempfile::tempdir().unwrap(); + let environment_dir = dir.path().join("environments"); + + super::seed_environments(&environment_dir).unwrap(); + // An operator edit to a seeded file must survive a re-seed. + fs::write( + environment_dir.join("default.toml"), + "provider = \"docker\"\n[resources]\ncpu = 7\n", + ) + .await + .unwrap(); + + super::seed_environments(&environment_dir).unwrap(); + + let store = EnvironmentStore::load(&environment_dir, false).unwrap(); + let default = store.get(&EnvironmentId::new("default").unwrap()).unwrap(); + assert_eq!(default.settings.resources.cpu, Some(7)); } #[tokio::test] @@ -515,10 +567,10 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let environment_dir = dir.path().join("environments"); - let enabled = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap(); + let enabled = EnvironmentStore::load(&environment_dir, true).unwrap(); assert!(enabled.get(&EnvironmentId::new("local").unwrap()).is_some()); - let disabled = EnvironmentStore::load_or_seed(&environment_dir, false).unwrap(); + let disabled = EnvironmentStore::load(&environment_dir, false).unwrap(); assert!( disabled .get(&EnvironmentId::new("local").unwrap()) @@ -539,7 +591,7 @@ mod tests { .await .unwrap(); - let store = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap(); + let store = EnvironmentStore::load(&environment_dir, true).unwrap(); let local = store.get(&EnvironmentId::new("local").unwrap()).unwrap(); // The synthetic local carries no resources; the stale file was ignored. @@ -549,7 +601,7 @@ mod tests { #[tokio::test] async fn local_mutations_are_reserved() { let dir = tempfile::tempdir().unwrap(); - let store = EnvironmentStore::load_or_seed(dir.path().join("environments"), true).unwrap(); + let store = EnvironmentStore::load(dir.path().join("environments"), true).unwrap(); let local = EnvironmentId::new("local").unwrap(); let revision = store.get(&local).unwrap().revision; @@ -584,7 +636,7 @@ mod tests { .await .unwrap(); - let store = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap(); + let store = EnvironmentStore::load(&environment_dir, true).unwrap(); assert_eq!( store @@ -592,7 +644,7 @@ mod tests { .iter() .map(|environment| environment.id.as_str()) .collect::>(), - vec!["a", "daytona", "default", "docker", "local", "z"] + vec!["a", "local", "z"] ); } @@ -603,7 +655,7 @@ mod tests { std::fs::create_dir_all(&environment_dir).unwrap(); std::fs::write(environment_dir.join("Bad.toml"), r#"provider = "local""#).unwrap(); - let err = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap_err(); + let err = EnvironmentStore::load(&environment_dir, true).unwrap_err(); assert!(matches!(err, EnvironmentStoreError::InvalidFilename { .. })); } @@ -615,7 +667,7 @@ mod tests { std::fs::create_dir_all(&environment_dir).unwrap(); std::fs::write(environment_dir.join("bad.toml"), r#"provider = "bogus""#).unwrap(); - let err = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap_err(); + let err = EnvironmentStore::load(&environment_dir, true).unwrap_err(); assert!(matches!(err, EnvironmentStoreError::Validation { .. })); assert!(err.to_string().contains("unknown environment provider")); @@ -637,7 +689,7 @@ mode = "cidr_allow_list" ) .unwrap(); - let err = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap_err(); + let err = EnvironmentStore::load(&environment_dir, true).unwrap_err(); assert!(matches!(err, EnvironmentStoreError::Validation { .. })); assert!( @@ -662,7 +714,7 @@ path = "Dockerfile" ) .unwrap(); - let err = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap_err(); + let err = EnvironmentStore::load(&environment_dir, true).unwrap_err(); assert!(matches!(err, EnvironmentStoreError::Validation { .. })); assert!(err.to_string().contains("Dockerfile")); @@ -671,7 +723,11 @@ path = "Dockerfile" #[tokio::test] async fn create_conflict_is_rejected() { let dir = tempfile::tempdir().unwrap(); - let store = EnvironmentStore::load_or_seed(dir.path().join("environments"), true).unwrap(); + let store = EnvironmentStore::load(dir.path().join("environments"), true).unwrap(); + store + .create(draft("docker", EnvironmentProvider::Docker)) + .await + .unwrap(); let err = store .create(draft("docker", EnvironmentProvider::Docker)) @@ -684,7 +740,7 @@ path = "Dockerfile" #[tokio::test] async fn create_invalid_settings_is_rejected() { let dir = tempfile::tempdir().unwrap(); - let store = EnvironmentStore::load_or_seed(dir.path().join("environments"), true).unwrap(); + let store = EnvironmentStore::load(dir.path().join("environments"), true).unwrap(); let mut settings = settings(EnvironmentProvider::Local); settings.network.mode = EnvironmentNetworkMode::Block; @@ -706,8 +762,11 @@ path = "Dockerfile" #[tokio::test] async fn replace_stale_revision_is_rejected() { let dir = tempfile::tempdir().unwrap(); - let store = EnvironmentStore::load_or_seed(dir.path().join("environments"), true).unwrap(); - let current = store.get(&EnvironmentId::new("docker").unwrap()).unwrap(); + let store = EnvironmentStore::load(dir.path().join("environments"), true).unwrap(); + let current = store + .create(draft("docker", EnvironmentProvider::Docker)) + .await + .unwrap(); let stale = EnvironmentRevision::from_bytes(b"stale"); let err = store @@ -719,24 +778,26 @@ path = "Dockerfile" } #[tokio::test] - async fn default_delete_is_rejected() { + async fn default_is_deletable() { let dir = tempfile::tempdir().unwrap(); - let store = EnvironmentStore::load_or_seed(dir.path().join("environments"), true).unwrap(); + let environment_dir = dir.path().join("environments"); + super::seed_environments(&environment_dir).unwrap(); + let store = EnvironmentStore::load(&environment_dir, true).unwrap(); let default = store.get(&EnvironmentId::new("default").unwrap()).unwrap(); - let err = store - .delete(&default.id, &default.revision) - .await - .unwrap_err(); + // `default` is an ordinary environment: deleting it succeeds and removes + // the run fallback rather than being protected. + store.delete(&default.id, &default.revision).await.unwrap(); - assert!(matches!(err, EnvironmentStoreError::Protected { .. })); + assert!(store.get(&default.id).is_none()); + assert!(!environment_dir.join("default.toml").exists()); } #[tokio::test] async fn delete_success_removes_file_and_memory_entry() { let dir = tempfile::tempdir().unwrap(); let environment_dir = dir.path().join("environments"); - let store = EnvironmentStore::load_or_seed(&environment_dir, true).unwrap(); + let store = EnvironmentStore::load(&environment_dir, true).unwrap(); let created = store .create(draft("tmp", EnvironmentProvider::Local)) .await @@ -751,7 +812,7 @@ path = "Dockerfile" #[tokio::test] async fn canonical_revision_changes_when_persisted_bytes_change() { let dir = tempfile::tempdir().unwrap(); - let store = EnvironmentStore::load_or_seed(dir.path().join("environments"), true).unwrap(); + let store = EnvironmentStore::load(dir.path().join("environments"), true).unwrap(); let created = store .create(draft("rev", EnvironmentProvider::Local)) .await @@ -776,7 +837,7 @@ path = "Dockerfile" fs::write(dir.path().join("Dockerfile"), "FROM alpine\n") .await .unwrap(); - let store = EnvironmentStore::load_or_seed(dir.path().join("environments"), true).unwrap(); + let store = EnvironmentStore::load(dir.path().join("environments"), true).unwrap(); let mut settings = settings(EnvironmentProvider::Docker); settings.image.dockerfile = Some(DockerfileSource::Path { path: "Dockerfile".to_string(), diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index 914012c8c..adcc0d562 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -731,6 +731,15 @@ fn install_listen_config(bind: &Bind) -> InstallListenConfig { } } +/// The environments directory sits next to the active settings file, matching +/// the server's own `environment_dir_for_active_config` derivation. +fn install_environment_dir(config_path: &Path) -> PathBuf { + config_path + .parent() + .unwrap_or_else(|| Path::new(".")) + .join("environments") +} + async fn health() -> Response { Json(serde_json::json!({ "status": "ok", @@ -1709,6 +1718,14 @@ async fn post_install_finish( .into_response(); } + // Seed the built-in environments next to the settings file. The server does + // not seed on startup, so install is the only place built-ins are written; + // existing files are preserved, so re-running install never clobbers edits. + let environment_dir = install_environment_dir(state.config_path.as_ref()); + if let Err(err) = fabro_environment::seed_environments(&environment_dir) { + warn!(error = %err, "failed to seed built-in environments after install"); + } + if let Ok(settings) = fabro_config::ServerSettingsBuilder::from_toml(&settings_toml) { if let Err(err) = write_artifact_store_metadata(&settings, state.storage_dir.as_ref()).await { diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index acfa3d725..fb494d748 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -2327,7 +2327,7 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result for ApiError { StatusCode::CONFLICT, format!("environment revision is stale: {id}"), ), - EnvironmentStoreError::Protected { id } => Self::new( - StatusCode::CONFLICT, - format!("environment is protected and cannot be deleted: {id}"), - ), EnvironmentStoreError::Reserved { id } => Self::new( StatusCode::CONFLICT, format!("environment is reserved and cannot be modified: {id}"), diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index caef6fac2..257a322c3 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -1222,8 +1222,10 @@ id = "missing" #[test] fn system_sandbox_provider_uses_manifest_defaults() { let temp = tempfile::tempdir().unwrap(); - let environment_store = EnvironmentStore::load_or_seed(temp.path().join("environments"), true) - .expect("environment store should seed"); + let environment_dir = temp.path().join("environments"); + fabro_environment::seed_environments(&environment_dir).expect("seed built-in environments"); + let environment_store = + EnvironmentStore::load(&environment_dir, true).expect("environment store should load"); let source = r#" _version = 1 @@ -1241,8 +1243,8 @@ id = "daytona" #[test] fn system_sandbox_provider_defaults_when_manifest_run_settings_do_not_resolve() { let temp = tempfile::tempdir().unwrap(); - let environment_store = EnvironmentStore::load_or_seed(temp.path().join("environments"), true) - .expect("environment store should seed"); + let environment_store = EnvironmentStore::load(temp.path().join("environments"), true) + .expect("environment store should load"); let source = r#" _version = 1 diff --git a/lib/crates/fabro-server/src/test_support.rs b/lib/crates/fabro-server/src/test_support.rs index 7e2731809..bfcb68da6 100644 --- a/lib/crates/fabro-server/src/test_support.rs +++ b/lib/crates/fabro-server/src/test_support.rs @@ -237,6 +237,15 @@ impl TestAppStateBuilder { let active_config_path = self .active_config_path .unwrap_or_else(|| vault_path.with_file_name("settings.toml")); + // Production seeds environments at install time, not on startup. Tests + // exercise an installed instance, so seed the built-ins next to the + // settings file before `build_app_state` loads them. + let environment_dir = active_config_path + .parent() + .unwrap_or_else(|| std::path::Path::new(".")) + .join("environments"); + fabro_environment::seed_environments(&environment_dir) + .expect("test environments should seed"); build_app_state(AppStateConfig { resolved_settings: resolved_runtime_settings_for_tests( self.server_settings, diff --git a/lib/crates/fabro-server/tests/it/api/environments.rs b/lib/crates/fabro-server/tests/it/api/environments.rs index d7f2d020e..720b57b9a 100644 --- a/lib/crates/fabro-server/tests/it/api/environments.rs +++ b/lib/crates/fabro-server/tests/it/api/environments.rs @@ -573,7 +573,7 @@ async fn dockerfile_path_over_rest_is_rejected_without_persisting_or_exposing_co } #[tokio::test] -async fn delete_environment_removes_non_default_and_default_is_protected() { +async fn delete_environment_removes_non_default_and_default_is_deletable() { let (app, _temp_dir, environment_dir) = environment_app(); let created = create_environment(&app, "delete-env", "local").await; let revision = revision_from(&created); @@ -608,13 +608,16 @@ async fn delete_environment_removes_non_default_and_default_is_protected() { ) .await; + // `default` is an ordinary environment: it can be deleted, which removes the + // run fallback. The server no longer protects it. let default = app .clone() .oneshot(empty_request(Method::GET, "/environments/default")) .await .expect("get default environment should respond"); let default = response_json(default, StatusCode::OK, "GET /api/v1/environments/default").await; - let protected = app + let deleted = app + .clone() .oneshot(request_with_if_match( Method::DELETE, "/environments/default", @@ -624,11 +627,23 @@ async fn delete_environment_removes_non_default_and_default_is_protected() { .await .expect("delete default environment should respond"); response_status( - protected, - StatusCode::CONFLICT, + deleted, + StatusCode::NO_CONTENT, "DELETE /api/v1/environments/default", ) .await; + + assert!(!environment_dir.join("default.toml").exists()); + let missing_default = app + .oneshot(empty_request(Method::GET, "/environments/default")) + .await + .expect("get deleted default environment should respond"); + response_status( + missing_default, + StatusCode::NOT_FOUND, + "GET /api/v1/environments/default after delete", + ) + .await; } #[tokio::test]