diff --git a/docs/internal/sandbox-admission-contract.md b/docs/internal/sandbox-admission-contract.md new file mode 100644 index 000000000..62e90d941 --- /dev/null +++ b/docs/internal/sandbox-admission-contract.md @@ -0,0 +1,151 @@ +# Sandbox admission and worker ownership + +This is the application boundary for the next sandbox-driver integration. The +current runtime and saved records remain supported. The authoritative driver +contract is [shared-contract.md](https://github.com/lithoscomputer/sandbox-driver/blob/8f7358a71b7acc51fc44b79f514e9f2a029db8c2/docs/shared-contract.md) +and its [target API](https://github.com/lithoscomputer/sandbox-driver/blob/8f7358a71b7acc51fc44b79f514e9f2a029db8c2/docs/next-api.md). +Declarations alone are not provider integration evidence. + +## Current boundaries + +| Boundary | Current owner and representation | +| --- | --- | +| Provider permission/launch settings | `ServerSandboxProvidersSettings`, keyed by provider kind; bundled providers have enablement, plugins have launch settings. This is not yet the next API's named provider-entry catalog. | +| Environment templates | `fabro-environment` SQLite store; REST uses `EnvironmentSettings`. `run_manifest` resolves the selected template and workflow overrides. | +| Admission | `run_compiler` and `server/petri_runs` admit graphs; `operations/create` persists `RunSpec.settings` and `PetriAdmission` graph blob references. | +| Worker input | `runner::execute` fetches `/runs/{id}/state`; `petri_worker` loads the admitted graphs and reads the saved environment. Start/resume do not resolve that environment from the catalog again. | +| Connection input | `worker_launch_spec` captures Daytona URL/organization and Docker host address from the same server lookup used by server-side access. `SandboxConnectionEnv` passes those values explicitly at spawn, including absence. Docker endpoint/TLS/context still use the existing process allowlist and driver client behavior. | +| Credentials | The worker opens a vault snapshot at the server-supplied storage root. Daytona keys are vault-only here. Workload `{{ secrets.NAME }}` references remain references in admitted settings; Petri resolves them through `VaultSecrets`. | +| Server sandbox access | `sandbox_access` connects by recorded provider kind and checks the `petri.run` ownership label. Host uses the existing registry observer. | + +The launch snapshot fixes server/worker configuration disagreement but is **not** +a durable admission binding. A restart still uses the current server connection +configuration. The worker also still loads its LLM catalog separately; this +preparation does not change that unrelated path. `FABRO_CONFIG` remains for those +existing consumers, not as the future sandbox catalog transport. + +## Provider entries and workload specifications + +At cutover, reuse driver-owned `contract::ProviderEntry`, `Environment`, and +`SandboxSpec`; do not create equivalent Fabro DTOs. Keep these meanings separate: + +- A server provider-entry name selects a connection and plugin/trust inputs. + `ProviderSchemas.config_schema` validates only that entry's `config`. +- An environment selects a provider entry and a portable workload template. + `spec_schema` validates only the workload's `provider_config`. Common workload + fields, capabilities, and services have their own contract validation. +- Fabro owns environment storage, API authorization, and run lifecycle policy. + Docker/Daytona recipes, helper images, networking, and nested runners belong + to the driver. Neither a connection form nor environment form should expose + those recipes as Fabro policy. + +The existing `[server.sandbox.providers.]` launch shape and environment +REST bodies continue to decode unchanged. In particular, the existing `plugin` +object is not the next contract's `plugin` name. A versioned boundary/explicit +legacy adapter is required before accepting the new shape; do not reinterpret +existing configuration under the same unversioned meaning. Do not activate new +forms or validate workload fields with a connection schema during preparation. + +## Durable admission and credential delivery + +The future admission record must contain the resolved workload, provider-entry +binding (including frozen non-secret launch inputs and secret references), and +effective application policy. Workers receive that value through the existing +run-state/blob delivery path. They construct their own catalog instance from +that value, never by reopening settings or rereading an edited environment. +Server access, pruning, and recovery must use the same saved binding. + +Resolve provider and service credential names through the same server vault as +the workload secret resolver. Persist names, never resolved values. The +application adapter must reject or externalize inline credentials in entry +config/args/env before making them durable; redacted `Debug` alone does not make +serialization safe. Avoid logging resolved launch payloads. Rotation retains +the existing consumption-time vault semantics rather than pinning credential +bytes in a run. + +After creation persist the driver-issued `SandboxRef { provider, namespace, id }`. +Its provider is an entry name; an environment rename or deletion cannot break +access. Namespace comparison precedes resource operations and never replaces +Fabro's ownership checks. For legacy records keep the legacy reader; never +derive an old namespace from today's URL, credentials, environment, or kind. + +**Unresolved dependency:** the driver catalog must expose an admitted binding +that can be rehydrated independently of mutable `EnvironmentSource` and retain +its namespace checks; Petri must persist/use that binding and the issued ref in +its lease/recovery path. The merged type declarations do not implement this. +Only then can Fabro extend its OpenAPI/persistence boundary, replace its +factories, and test start/resume/prune against edited or deleted entries. The +existing plugin-registration proposal that rereads server settings in workers +must not become this path. + +## Lifecycle compatibility + +`EnvironmentLifecycleSettings::run_policy()` extracts only the two +application decisions. It is used by run deletion and the worker-exit backstop; +the persisted/API location remains `run.environment.lifecycle` for compatibility. + +| preserve | stop_on_terminal | Existing application policy | +| --- | --- | --- | +| false | true | Default: prune on run deletion; stop on terminal worker-failure cleanup. | +| true | true | Skip sandbox prune on deletion; still request terminal stop. | +| false | false | Prune on deletion; skip the terminal-stop backstop. | +| true | false | Skip both sandbox prune on deletion and the terminal-stop backstop. | + +Normal scope release remains Petri's. Fabro passes `Retention::Always` for all +four combinations: finishing a run is not a request to destroy its workspace. +The supervisor applies `stop_on_terminal` only after a failed worker exits and +the run has settled terminal, not while it is scheduled to resume. Local +process cleanup remains the Host guardian's responsibility. Preservation does +not override that cleanup or promise that a managed Host directory survives +run-directory deletion. + +When adding `[run.lifecycle]` and provider-entry defaults, retain false/true +defaults and normalize legacy policy fields per input layer before merging. +Within one layer the new field should win; higher-precedence workflow/CLI input +must still win over lower-precedence settings regardless of spelling. Existing +`preserve_sandbox = false` remains an explicit override. Save the effective +policy once; do not consult new provider defaults for an old run. This change +does not activate the new input location or change normal-release behavior. + +`auto_stop` is a provider idle timer, not either application decision. Keep it +outside the future run policy. Timer translation waits for provider evidence, +including disabled timers and Daytona's distinct zero-means-delete behavior. + +## Host access through the owning worker + +The next Host is a separately installed plugin with `owner_process` access and +process durability. Fabro must not link/bundle it, independently attach from +the server, or recreate a handle just because a saved directory exists. +Missing Host must fail only work selecting it, including Host-backed dry runs; +other configured providers remain usable. + +The existing control path already has authenticated run-scoped WebSockets, +request IDs, acknowledgements, deadlines, and a server supervisor that knows +the worker. Reuse that ownership route; do not introduce a daemon. It currently +carries run controls, **not** sandbox RPC or file/terminal streams. + +For live access, the server must authorize the run and dispatch to the current +worker attempt. That worker must use its existing Petri-owned sandbox handle +and verify the lease/ref and run ownership. Requests and replies need the +worker-attempt identity as well as the run/request ID so a replacement worker +cannot answer for a dead owner. Do not send sandbox mutations through the +existing reconnect replay mechanism: a dropped acknowledgement means unknown +outcome, not permission to retry. File/exec/terminal streams require bounded +stream transport and cancellation; the control acknowledgement channel is not +a bulk-data transport. + +| Situation | Compatible product behavior at cutover | +| --- | --- | +| Live owning worker | Proxy supported files/exec/terminal operations to that worker; do not launch a server Host instance. | +| History | Display provider/ref, recorded working directory, events, and artifacts from the run record, even if Host is unavailable. Recorded status is history, not a fresh health probe. | +| Owner exited or channel lost | Refuse live access with an owner-unavailable/unknown result. Do not claim workload termination from process loss alone. | +| Retained files | Offer recorded artifacts or an explicitly authorized retained-file view without constructing a sandbox. Path existence grants neither ownership nor permission to execute. Managed versus designated workspace deletion rights must remain explicit. | +| Resume | Pause/unpause can use the same live owner. After owner loss ordinary attach/resume is not promised. An explicit recovery into a new sandbox over a retained designated workspace requires prior-owner safety to be established; it is not automatic resurrection. | + +**Unresolved dependencies:** Petri must expose the current owner handle and +attempt identity to the worker access adapter; the control path needs +non-replayed sandbox requests plus bounded stream forwarding; Host must prove +fencing/owner cleanup and stale-handle behavior. Until those are integrated and +tested, retain the current observer, directory-access behavior, and existing +resume path. Removing the observer ahead of this route would break the sandbox +tab, Run Files, terminal, `fabro cp`, and Ask Fabro. diff --git a/docs/internal/server-secrets-strategy.md b/docs/internal/server-secrets-strategy.md index 3f492ea46..bc00d1025 100644 --- a/docs/internal/server-secrets-strategy.md +++ b/docs/internal/server-secrets-strategy.md @@ -124,6 +124,7 @@ There is no startup-time secret generation or import of optional integration sec ## Subprocess Boundaries - Worker and render-graph subprocesses start from `env_clear()` and re-add only explicit allowlisted variables. +- Sandbox connection selectors (`DAYTONA_API_URL`, `DAYTONA_ORGANIZATION_ID`, and `PETRI_SANDBOX_DOCKER_HOST_ADDRESS`) are captured in the worker launch specification from the server's configuration lookup and passed explicitly, including absence. They are not independently inherited from the spawning process. This is a launch snapshot, not yet a durable provider-entry binding; see [sandbox admission and ownership](sandbox-admission-contract.md). `DAYTONA_API_KEY` remains a vault lookup and is never included in that connection snapshot. - Authority-bearing values are re-injected intentionally. For worker subprocesses this is `FABRO_WORKER_TOKEN`, plus any explicitly required internal value such as a vault-derived `GITHUB_APP_PRIVATE_KEY`; it is not user auth state such as `FABRO_DEV_TOKEN` or `auth.json`. - The worker reads `FABRO_WORKER_TOKEN` from its env at startup (in `main()` before Tokio initializes) and immediately calls `std::env::remove_var` to scrub it. The token then flows through function arguments to `runner::execute`. Every descendant process (hooks, sandbox commands, MCP stdio, etc.) therefore inherits a worker env that no longer contains the bearer, so an unscrubbed spawn site cannot leak it. - The daemon child inherits the parent env unchanged except for output-format hygiene (`FABRO_JSON` removal). diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index 22e029269..561434630 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -378,12 +378,24 @@ memory = "8GB" | `resources.cpu` / `memory` / `disk` | Best-effort resource hints. Unsupported provider fields warn and continue. | | `network.mode` | `allow_all`, `block`, or `cidr_allow_list`. Local cannot enforce blocked/CIDR networking; Docker cannot enforce CIDR allow-lists. | | `network.allow` | CIDRs for `cidr_allow_list`; entries are validated as CIDRs. | -| `lifecycle.preserve` | Keep the created sandbox after the run finishes. | -| `lifecycle.stop_on_terminal` | Stop the sandbox when the run reaches a terminal state. | +| `lifecycle.preserve` | Skip sandbox pruning when deleting the run. Defaults to `false`; does not keep a workload running. | +| `lifecycle.stop_on_terminal` | Request sandbox stop during terminal cleanup after worker failure. Defaults to `true`; independent of `preserve`. | | `lifecycle.auto_stop` | Daytona auto-stop duration, such as `"30m"`. Defaults to `"120m"`; `"0s"` disables auto-stop. | | `labels` | Provider labels. Merge by key across layers. | | `env` | Environment variables passed to command and agent execution. Merge by key across layers. | +Fabro retains run workspaces after normal scope release; completion does not +request sandbox deletion. `preserve` controls later run deletion, while +`stop_on_terminal` controls the supervisor's cleanup after a failed worker exits +and the run settles terminal. Normal scope release remains the workflow engine's +responsibility. Local process cleanup remains the Host provider's responsibility; +neither setting makes a local workload survive its owning process. + +The resolved environment and lifecycle values are saved when the run is created. +Editing or deleting the catalog environment does not change those saved values. +The current configuration and API location remains `run.environment.lifecycle`; +`run.lifecycle` is not yet an accepted input. + When `provider = "local"`, Fabro works in a clone of the target folder's committed `HEAD`, not in the folder itself, so uncommitted changes are not included. diff --git a/docs/public/integrations/daytona.mdx b/docs/public/integrations/daytona.mdx index 26d089805..4906be8b8 100644 --- a/docs/public/integrations/daytona.mdx +++ b/docs/public/integrations/daytona.mdx @@ -178,16 +178,17 @@ fabro sandbox ssh This creates temporary SSH credentials (valid for 60 minutes) and connects directly. Use `--print` to print the SSH command instead of connecting, or `--ttl` to set the credential expiry. -To keep the sandbox alive after the run completes, pass `--preserve-sandbox` to `fabro run`. +`--preserve-sandbox` skips sandbox pruning when deleting the run. It does not disable terminal stop or provider idle timers. ## Sandbox lifecycle -Each sandbox gets a unique timestamped name (e.g. `fabro-20260307-143022-a3f2`) and is created as ephemeral. By default, sandboxes are destroyed when the run finishes. +Run completion retains the sandbox workspace. Run deletion normally prunes the +sandbox through the workflow engine's lease ledger. ### Preservation -To keep a sandbox alive for debugging: +To retain the sandbox when deleting its run: ```bash fabro run workflow.fabro --environment cloud --preserve-sandbox @@ -206,7 +207,10 @@ provider = "daytona" preserve = true ``` -When preserved, Fabro prints the sandbox name so you can find it in the [Daytona dashboard](https://app.daytona.io/dashboard/sandboxes). +Preservation is independent of `stop_on_terminal`, which defaults to `true` and +controls the supervisor's terminal cleanup after worker failure. Provider idle +timers are independent as well. Preservation does not guarantee a running +sandbox. ### Auto-stop diff --git a/lib/apps/fabro-server/src/sandbox_access.rs b/lib/apps/fabro-server/src/sandbox_access.rs index fe82c7ad6..01e71ac13 100644 --- a/lib/apps/fabro-server/src/sandbox_access.rs +++ b/lib/apps/fabro-server/src/sandbox_access.rs @@ -34,7 +34,7 @@ use anyhow::Context as _; use fabro_config::Storage; use fabro_petri::providers; pub(crate) use fabro_petri::providers::DaytonaCredentials; -use fabro_types::settings::run::RunEnvironmentSettings; +use fabro_types::settings::run::RunLifecyclePolicy; use fabro_types::settings::server::{ SandboxPluginSettings, ServerSandboxProviderSettings, ServerSandboxProvidersSettings, }; @@ -277,15 +277,14 @@ async fn attach_run_sandbox_on( /// Local process cleanup belongs to the Host provider's worker guardian. pub(crate) async fn stop_run_sandboxes( access: &ProviderAccess, - environment: &RunEnvironmentSettings, + provider: &SandboxProviderKind, + policy: RunLifecyclePolicy, run_id: RunId, ) -> anyhow::Result<()> { - if !environment.lifecycle.stop_on_terminal - || environment.provider.bundled() == Some(BundledProvider::Local) - { + if !policy.stop_on_terminal || provider.bundled() == Some(BundledProvider::Local) { return Ok(()); } - let provider = connect_provider(&environment.provider, access).await?; + let provider = connect_provider(provider, access).await?; stop_run_sandboxes_on(provider, run_id).await } @@ -951,17 +950,30 @@ mod tests { // This plugin cannot connect: skipping the policy must happen first. let provider = kind("unreachable-cleanup"); let access = unreachable_plugin_access(provider.as_str()); - let mut environment = RunEnvironmentSettings { - provider, - ..RunEnvironmentSettings::default() - }; - environment.lifecycle.stop_on_terminal = false; - stop_run_sandboxes(&access, &environment, RunId::new()) + for preserve in [false, true] { + stop_run_sandboxes( + &access, + &provider, + RunLifecyclePolicy { + preserve, + stop_on_terminal: false, + }, + RunId::new(), + ) .await .unwrap(); - stop_run_sandboxes(&access, &RunEnvironmentSettings::default(), RunId::new()) + stop_run_sandboxes( + &access, + &SandboxProviderKind::LOCAL, + RunLifecyclePolicy { + preserve, + stop_on_terminal: true, + }, + RunId::new(), + ) .await .unwrap(); + } } #[tokio::test] diff --git a/lib/apps/fabro-server/src/server.rs b/lib/apps/fabro-server/src/server.rs index 413c8c608..aaa4a3afc 100644 --- a/lib/apps/fabro-server/src/server.rs +++ b/lib/apps/fabro-server/src/server.rs @@ -151,7 +151,7 @@ use crate::sandbox_access::{ SandboxInventory, }; use crate::server_secrets::ServerSecrets; -use crate::spawn_env::apply_render_graph_env; +use crate::spawn_env::{SandboxConnectionEnv, apply_render_graph_env}; use crate::worker_control::{ LocalWorkerControlBus, WORKER_CONTROL_ACK_WAIT, WorkerControlAcks, WorkerControlBus, WorkerControlBusError, @@ -2904,6 +2904,7 @@ async fn delete_run_sandbox_resource( .run .environment .lifecycle + .run_policy() .preserve; let Some(record) = projection .sandbox @@ -3956,6 +3957,9 @@ fn worker_launch_spec( active_config_path: state.active_config_path().to_path_buf(), github_app_private_key, fabro_home: fabro_config::Home::from_env().root().to_path_buf(), + sandbox_connection_env: SandboxConnectionEnv::from_lookup(|name| { + state.config_env_lookup(name) + }), }) } @@ -4425,7 +4429,14 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { let access = state.provider_access().await?; sandbox_access::stop_run_sandboxes( &access, - &final_state.spec.settings.run.environment, + &final_state.spec.settings.run.environment.provider, + final_state + .spec + .settings + .run + .environment + .lifecycle + .run_policy(), run_id, ) .await diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 02b3d006c..31da987ad 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -2234,6 +2234,59 @@ fn slack_service_ignores_server_env_tokens() { assert!(state.slack_service.is_none()); } +#[cfg(unix)] +#[test] +fn worker_command_uses_captured_server_connection_configuration() { + use std::ffi::OsStr; + + let storage_dir = tempfile::tempdir().unwrap(); + let changed = Arc::new(AtomicBool::new(false)); + let lookup_changed = Arc::clone(&changed); + let state = worker_command_test_state_with_extra_config_and_env_lookup( + storage_dir.path(), + &["dev-token"], + Some(TEST_DEV_TOKEN), + "", + &[], + move |name| { + let value = match name { + EnvVars::DAYTONA_API_URL => "https://daytona.example/api", + EnvVars::DAYTONA_ORGANIZATION_ID => "server-organization", + EnvVars::PETRI_SANDBOX_DOCKER_HOST_ADDRESS => "server.internal", + _ => return None, + }; + Some(if lookup_changed.load(Ordering::SeqCst) { + "changed-after-launch-capture".to_string() + } else { + value.to_string() + }) + }, + ); + let spec = worker_launch_spec( + &state, + RunId::new(), + RunExecutionMode::Start, + storage_dir.path(), + false, + None, + ) + .unwrap(); + changed.store(true, Ordering::SeqCst); + let cmd = LocalWorkerRuntime::command_for_spec(&spec); + let env: HashMap<_, _> = cmd.as_std().get_envs().collect(); + for (name, value) in [ + (EnvVars::DAYTONA_API_URL, "https://daytona.example/api"), + (EnvVars::DAYTONA_ORGANIZATION_ID, "server-organization"), + ( + EnvVars::PETRI_SANDBOX_DOCKER_HOST_ADDRESS, + "server.internal", + ), + ] { + assert_eq!(env.get(OsStr::new(name)), Some(&Some(OsStr::new(value)))); + } + assert!(!env.contains_key(OsStr::new(EnvVars::DAYTONA_API_KEY))); +} + #[cfg(unix)] #[test] fn worker_command_uses_null_stdin_and_token_env() { diff --git a/lib/apps/fabro-server/src/spawn_env.rs b/lib/apps/fabro-server/src/spawn_env.rs index 333a48723..3154a836c 100644 --- a/lib/apps/fabro-server/src/spawn_env.rs +++ b/lib/apps/fabro-server/src/spawn_env.rs @@ -3,6 +3,42 @@ use std::ffi::OsString; use fabro_static::EnvVars; use tokio::process::Command; +/// Connection inputs already resolved by the server. These must reach the +/// worker from the same lookup used for server-side access, not be sampled +/// again from the ambient process when the child is spawned. Credentials +/// are deliberately absent: Daytona's key comes from the worker's vault. +const SANDBOX_CONNECTION_ENV: &[&str] = &[ + EnvVars::PETRI_SANDBOX_DOCKER_HOST_ADDRESS, + EnvVars::DAYTONA_API_URL, + EnvVars::DAYTONA_ORGANIZATION_ID, +]; + +pub(crate) struct SandboxConnectionEnv(Vec<(&'static str, Option)>); + +impl SandboxConnectionEnv { + pub(crate) fn from_lookup(lookup: impl Fn(&str) -> Option) -> Self { + Self( + SANDBOX_CONNECTION_ENV + .iter() + .map(|&name| (name, lookup(name))) + .collect(), + ) + } + + pub(crate) fn apply(&self, cmd: &mut Command) { + for (name, value) in &self.0 { + match value { + Some(value) => { + cmd.env(name, value); + } + None => { + cmd.env_remove(name); + } + } + } + } +} + const WORKER_ENV_ALLOWLIST: &[&str] = &[ EnvVars::PATH, EnvVars::HOME, @@ -52,7 +88,6 @@ const WORKER_ENV_ALLOWLIST: &[&str] = &[ EnvVars::AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE, // Petri's sandbox settings the worker's in-process providers read. No // plugin settings cross: the worker never launches a provider plugin. - EnvVars::PETRI_SANDBOX_DOCKER_HOST_ADDRESS, EnvVars::PETRI_SANDBOX_ACTION_HOST_IMAGE, // The Docker daemon selection: the worker's Docker provider reads these // from its own process, so the worker's sandboxes go to the daemon the @@ -64,10 +99,6 @@ const WORKER_ENV_ALLOWLIST: &[&str] = &[ EnvVars::DOCKER_API_VERSION, EnvVars::DOCKER_CONFIG, EnvVars::DOCKER_CONTEXT, - // Daytona's non-secret selection. The worker reads the API key from - // the vault and supplies it explicitly to the in-process provider. - EnvVars::DAYTONA_API_URL, - EnvVars::DAYTONA_ORGANIZATION_ID, // A test's checkpoint gates: the worker's hooks hold at a named point // until the test releases them, so a crash can be placed there. EnvVars::FABRO_TEST_CHECKPOINT_GATES, @@ -110,7 +141,37 @@ mod tests { use std::ffi::OsString; use std::path::Path; - use super::{RENDER_GRAPH_ENV_ALLOWLIST, WORKER_ENV_ALLOWLIST, apply_allowlist}; + use super::{ + RENDER_GRAPH_ENV_ALLOWLIST, SandboxConnectionEnv, WORKER_ENV_ALLOWLIST, apply_allowlist, + }; + + #[tokio::test] + async fn explicit_connection_snapshot_replaces_selectors_without_forwarding_credentials() { + let snapshot = SandboxConnectionEnv::from_lookup(|name| match name { + "DAYTONA_API_URL" => Some("https://admitted.example/api".to_string()), + "PETRI_SANDBOX_DOCKER_HOST_ADDRESS" => Some("server.internal".to_string()), + "DAYTONA_API_KEY" => panic!("connection configuration must not read credentials"), + _ => None, + }); + let mut cmd = env_command(); + cmd.env_clear(); + cmd.env("DAYTONA_API_URL", "https://changed.example/api"); + cmd.env("DAYTONA_ORGANIZATION_ID", "wrong-organization"); + snapshot.apply(&mut cmd); + assert_eq!( + env_output(cmd).await, + HashMap::from([ + ( + "DAYTONA_API_URL".to_string(), + "https://admitted.example/api".to_string() + ), + ( + "PETRI_SANDBOX_DOCKER_HOST_ADDRESS".to_string(), + "server.internal".to_string() + ), + ]) + ); + } fn env_command() -> tokio::process::Command { assert!(Path::new("/usr/bin/env").exists()); @@ -264,16 +325,10 @@ mod tests { actual.get("DOCKER_CONTEXT").map(String::as_str), Some("build") ); - // Daytona's non-secret selectors cross; its key is the vault's, - // never the server's environment. - assert_eq!( - actual.get("DAYTONA_API_URL").map(String::as_str), - Some("https://daytona.internal/api") - ); - assert_eq!( - actual.get("DAYTONA_ORGANIZATION_ID").map(String::as_str), - Some("org-1") - ); + // Connection selectors require the explicit server snapshot. The + // credential still comes from the worker's vault. + assert!(!actual.contains_key("DAYTONA_API_URL")); + assert!(!actual.contains_key("DAYTONA_ORGANIZATION_ID")); // The URL alias and placement target never reached the Daytona // plugin, so they stay out and plugin-era leases keep their // fingerprint. diff --git a/lib/apps/fabro-server/src/worker_runtime.rs b/lib/apps/fabro-server/src/worker_runtime.rs index 1171e7153..bf02f398e 100644 --- a/lib/apps/fabro-server/src/worker_runtime.rs +++ b/lib/apps/fabro-server/src/worker_runtime.rs @@ -12,7 +12,7 @@ use futures_util::future::BoxFuture; use tokio::io::AsyncRead; use tokio::process::Command; -use crate::spawn_env::apply_worker_env; +use crate::spawn_env::{self, SandboxConnectionEnv}; #[async_trait] pub(crate) trait WorkerRuntime: Send + Sync { @@ -52,6 +52,7 @@ pub(crate) struct WorkerLaunchSpec { /// The Fabro home the server resolved, so a Petri run's skills step /// reads the same home whatever the worker's environment says. pub(crate) fabro_home: PathBuf, + pub(crate) sandbox_connection_env: SandboxConnectionEnv, } pub(crate) struct StartedWorker { @@ -113,7 +114,8 @@ impl LocalWorkerRuntime { .stdout(worker_stdout) .stderr(Stdio::piped()); - apply_worker_env(&mut cmd); + spawn_env::apply_worker_env(&mut cmd); + spec.sandbox_connection_env.apply(&mut cmd); if let Some(level) = spec.fabro_log.as_deref() { cmd.env(EnvVars::FABRO_LOG, level); } diff --git a/lib/apps/fabro-server/tests/it/api/environments.rs b/lib/apps/fabro-server/tests/it/api/environments.rs index 932df31a0..e854c1dc0 100644 --- a/lib/apps/fabro-server/tests/it/api/environments.rs +++ b/lib/apps/fabro-server/tests/it/api/environments.rs @@ -11,7 +11,123 @@ use fabro_server::test_support::{ use serde_json::{Value, json}; use tower::ServiceExt; -use crate::helpers::{api, checked_response, response_json, response_status}; +use crate::helpers::{ + MINIMAL_DOT, api, checked_response, minimal_intent_json, response_json, response_status, +}; + +#[tokio::test] +async fn admitted_environment_and_secret_references_survive_catalog_replacement_and_deletion() { + let temp_dir = tempfile::tempdir().unwrap(); + let state = TestAppStateBuilder::new() + .active_config_path(temp_dir.path().join("settings.toml")) + .vault_path(temp_dir.path().join("secrets.json")) + .vault_entries([("SERVICE_TOKEN", "test-only-admission-secret")]) + .build(); + let app = build_test_router(state); + let workspace = tempfile::tempdir().unwrap(); + let mut environment = environment_body("admission-snapshot", "local"); + environment["lifecycle"]["preserve"] = json!(true); + environment["lifecycle"]["stop_on_terminal"] = json!(false); + environment["env"]["SERVICE_TOKEN"] = json!("{{ secrets.SERVICE_TOKEN }}"); + environment["labels"]["revision"] = json!("admitted"); + let created = create_environment_with_body(&app, &environment).await; + let mut intent = minimal_intent_json(&app, MINIMAL_DOT, workspace.path()).await; + intent["environment_id"] = json!("admission-snapshot"); + let run = response_json( + app.clone() + .oneshot(json_request(Method::POST, "/runs", &intent)) + .await + .unwrap(), + StatusCode::CREATED, + "POST /api/v1/runs with an environment snapshot", + ) + .await; + let run_id = run["id"].as_str().unwrap(); + let state_path = format!("/runs/{run_id}/state"); + let admitted = response_json( + app.clone() + .oneshot(empty_request(Method::GET, &state_path)) + .await + .unwrap(), + StatusCode::OK, + "GET admitted worker state", + ) + .await; + let spec = &admitted["spec"]; + assert!( + !serde_json::to_string(spec) + .unwrap() + .contains("test-only-admission-secret") + ); + assert_eq!( + spec["settings"]["run"]["environment"]["env"]["SERVICE_TOKEN"], + "{{ secrets.SERVICE_TOKEN }}" + ); + assert_eq!( + spec["settings"]["run"]["environment"]["lifecycle"], + environment["lifecycle"] + ); + assert!(spec["admission"]["graph"]["blob"].is_string()); + + let replacement = response_json( + app.clone() + .oneshot(request_with_if_match( + Method::PUT, + "/environments/admission-snapshot", + revision_from(&created), + Some(environment_settings("docker")), + )) + .await + .unwrap(), + StatusCode::OK, + "PUT replacement environment", + ) + .await; + for deleted in [false, true] { + if deleted { + response_status( + app.clone() + .oneshot(request_with_if_match( + Method::DELETE, + "/environments/admission-snapshot", + revision_from(&replacement), + None, + )) + .await + .unwrap(), + StatusCode::NO_CONTENT, + "DELETE admitted environment", + ) + .await; + } + let worker_state = response_json( + app.clone() + .oneshot(empty_request(Method::GET, &state_path)) + .await + .unwrap(), + StatusCode::OK, + "GET worker state after catalog mutation", + ) + .await; + assert_eq!(&worker_state["spec"], spec); + let settings = response_json( + app.clone() + .oneshot(empty_request( + Method::GET, + &format!("/runs/{run_id}/settings"), + )) + .await + .unwrap(), + StatusCode::OK, + "GET run settings after catalog mutation", + ) + .await; + assert_eq!( + settings["run"]["environment"], + spec["settings"]["run"]["environment"] + ); + } +} fn environment_settings(provider: &str) -> Value { json!({ diff --git a/lib/foundation/fabro-config/src/tests/resolve_run.rs b/lib/foundation/fabro-config/src/tests/resolve_run.rs index 2022b4149..42bde1746 100644 --- a/lib/foundation/fabro-config/src/tests/resolve_run.rs +++ b/lib/foundation/fabro-config/src/tests/resolve_run.rs @@ -1,6 +1,6 @@ use fabro_types::SandboxProviderKind; -use fabro_types::settings::InterpString; use fabro_types::settings::run::{ApprovalMode, EnvironmentNetworkMode, RunGoal, RunMode}; +use fabro_types::settings::{Duration, InterpString}; use crate::{MergeMap, SettingsLayer}; @@ -528,6 +528,36 @@ channel = "#ops" ); } +#[test] +fn lifecycle_defaults_and_explicit_false_remain_compatible_across_providers() { + for provider in ["local", "docker", "daytona"] { + let minimal = format!("[environments.default]\nprovider = \"{provider}\"\n"); + let defaults = workflow_settings_from_toml_with_catalog("_version = 1", &minimal).unwrap(); + let policy = defaults.run.environment.lifecycle.run_policy(); + assert!(!policy.preserve); + assert!(policy.stop_on_terminal); + + let catalog = format!( + "{minimal}\n[environments.default.lifecycle]\npreserve = true\nstop_on_terminal = false\nauto_stop = \"30m\"\n" + ); + let resolved = workflow_settings_from_toml_with_catalog( + "_version = 1\n[run.environment.lifecycle]\npreserve = false\n", + &catalog, + ) + .unwrap(); + let lifecycle = resolved.run.environment.lifecycle; + assert!( + !lifecycle.run_policy().preserve, + "explicit false overrides the template" + ); + assert!( + !lifecycle.run_policy().stop_on_terminal, + "omission inherits independently" + ); + assert_eq!(lifecycle.auto_stop, Some(Duration::from_secs(1800))); + } +} + #[test] fn toml_run_environment_lifecycle_override_is_applied() { let settings = super::workflow_settings_from_toml( diff --git a/lib/foundation/fabro-types/src/settings/run.rs b/lib/foundation/fabro-types/src/settings/run.rs index b2862ad3f..8eef556f5 100644 --- a/lib/foundation/fabro-types/src/settings/run.rs +++ b/lib/foundation/fabro-types/src/settings/run.rs @@ -998,12 +998,39 @@ impl Default for EnvironmentNetworkSettings { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct EnvironmentLifecycleSettings { + /// Application deletion policy; this does not keep a workload running. pub preserve: bool, #[serde(default = "default_stop_on_terminal")] pub stop_on_terminal: bool, pub auto_stop: Option, } +/// Fabro's run policy, independent of the provider's idle timer. +/// +/// During migration the persisted/API shape remains +/// `run.environment.lifecycle`. Read this view from the admitted run's +/// settings, never from the current environment catalog. Moving the input +/// location must not change these defaults or imply a retention policy. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct RunLifecyclePolicy { + /// Skip sandbox pruning when deleting the run. This does not promise + /// that a provider-owned workspace survives loss of its owner process. + pub preserve: bool, + /// Ask the supervisor to stop durable sandboxes after worker exit. + /// Independent of preservation; stopping retains their workspace. + pub stop_on_terminal: bool, +} + +impl EnvironmentLifecycleSettings { + #[must_use] + pub const fn run_policy(&self) -> RunLifecyclePolicy { + RunLifecyclePolicy { + preserve: self.preserve, + stop_on_terminal: self.stop_on_terminal, + } + } +} + fn default_stop_on_terminal() -> bool { true } diff --git a/lib/foundation/fabro-types/tests/run_lifecycle_policy.rs b/lib/foundation/fabro-types/tests/run_lifecycle_policy.rs new file mode 100644 index 000000000..c208ccc6d --- /dev/null +++ b/lib/foundation/fabro-types/tests/run_lifecycle_policy.rs @@ -0,0 +1,41 @@ +use fabro_types::settings::run::{EnvironmentLifecycleSettings, RunLifecyclePolicy}; + +#[test] +fn legacy_lifecycle_defaults_keep_deletion_and_terminal_stop_independent() { + let legacy: EnvironmentLifecycleSettings = serde_json::from_value(serde_json::json!({ + "preserve": false, + "auto_stop": null + })) + .unwrap(); + assert_eq!(legacy, EnvironmentLifecycleSettings::default()); + assert_eq!(legacy.run_policy(), RunLifecyclePolicy { + preserve: false, + stop_on_terminal: true, + }); +} + +#[test] +fn lifecycle_policy_preserves_all_combinations_and_the_existing_wire_shape() { + for preserve in [false, true] { + for stop_on_terminal in [false, true] { + for auto_stop in [serde_json::Value::Null, serde_json::json!("30m")] { + let wire = serde_json::json!({ + "preserve": preserve, + "stop_on_terminal": stop_on_terminal, + "auto_stop": auto_stop, + }); + let lifecycle: EnvironmentLifecycleSettings = + serde_json::from_value(wire.clone()).unwrap(); + assert_eq!(lifecycle.run_policy(), RunLifecyclePolicy { + preserve, + stop_on_terminal + }); + let encoded = serde_json::to_value(&lifecycle).unwrap(); + let restored: EnvironmentLifecycleSettings = + serde_json::from_value(encoded.clone()).unwrap(); + assert_eq!(restored, lifecycle); + assert_eq!(encoded, wire); + } + } + } +}