diff --git a/crates/core/src/oi/handler/apps.rs b/crates/core/src/oi/handler/apps.rs index e1785d25..79703f40 100644 --- a/crates/core/src/oi/handler/apps.rs +++ b/crates/core/src/oi/handler/apps.rs @@ -13,7 +13,7 @@ use crate::{ oi::{handler::RequestCtx, state::OiState}, runtime::{ AppPhase, - apps::{AppEntry, AppRegistry, AppStatus}, + apps::{AppEntry, AppRegistry, AppStatus, ReloadOutcome}, barrier::oracle::{derive_lifecycle_state, derive_state_with_transition_time}, desired::list_dynamic_resources_for_app, faults, @@ -1225,72 +1225,44 @@ pub(crate) fn register_app( } } - // Evaluate script and add to in-memory registry. - { - let mut reg = state.registry.write(); - reg.register( - params.app.clone(), - script.to_owned(), - Arc::clone(&state.tick_notify), - &state.script_limits, - ) - .map_err(|e| OiError::script_error(e.to_string()))?; - } - - // Persist app row first so generations can FK against it. - { - let reg = state.registry.read(); - let entry = reg.get(name).expect("just registered"); - let (app_name, generation_n, installed, uninstalling, installing) = - extract_persist_fields(entry); - state - .db - .call(move |db| { - persist_app_fields( - db, - &app_name, - generation_n, - installed, - uninstalling, - installing, - ) - }) - .map_err(|e| OiError::new(ErrorCode::ScriptError, format!("db persist: {e}")))?; - } + // i[impl app.register] — durable first, observable second. The rows all + // commit or none do, and only then does the app appear in the registry, + // so a failed persist leaves nothing for `/apps/list` to show, nothing + // for a restart to silently drop (load_from_db skips generation-0 rows), + // and nothing for a retried `/apps/create` to collide with. + let (app, script_error) = crate::runtime::apps::evaluate_script( + ¶ms.app, + script, + &std::collections::BTreeMap::new(), + &state.script_limits, + ); - // r[impl generation.bumps] — initial registration creates generation 1. let name_owned = params.app.clone(); let script_owned = script.to_owned(); let generation = state .db - .call(move |db| crate::runtime::generations::bump_register(db, &name_owned, &script_owned)) - .map_err(|e| OiError::new(ErrorCode::ScriptError, format!("db generation: {e}")))?; - { - let mut reg = state.registry.write(); - if let Some(entry) = reg.get_mut(name) { - entry.current_generation = generation; - } - } - // Persist again now that current_generation is set. - { - let reg = state.registry.read(); - let entry = reg.get(name).expect("just registered"); - let (app_name, generation_n, installed, uninstalling, installing) = - extract_persist_fields(entry); - state - .db - .call(move |db| { - persist_app_fields( - db, - &app_name, - generation_n, - installed, - uninstalling, - installing, - ) - }) - .map_err(|e| OiError::new(ErrorCode::ScriptError, format!("db persist: {e}")))?; - } + .call(move |db| -> rusqlite::Result { + let tx = db.conn.unchecked_transaction()?; + // The app row first, so the generation rows can FK against it. + persist_app_fields(db, &name_owned, 0, false, false, false)?; + // r[impl generation.bumps] — initial registration creates + // generation 1. + let generation = + crate::runtime::generations::bump_register(db, &name_owned, &script_owned)?; + persist_app_fields(db, &name_owned, generation, false, false, false)?; + tx.commit()?; + Ok(generation) + }) + .map_err(|e| OiError::new(ErrorCode::ScriptError, format!("db persist: {e}")))?; + + state.registry.write().insert_registered( + params.app.clone(), + script.to_owned(), + app, + script_error, + Arc::clone(&state.tick_notify), + generation, + ); { let reg = state.registry.read(); @@ -1558,19 +1530,32 @@ pub(crate) fn update_app( let loaded_params = state .db .call(move |db| crate::runtime::apps::load_all_params_for_app(db, &cipher, &name_owned)); - state.registry.write().reload( + let outcome = state.registry.write().reload( ¶ms.app, script.to_owned(), &loaded_params, &state.script_limits, ); + // i[impl app.update] — every diff below compares live state against the + // registry's definition. When evaluation failed the registry still holds + // the previous good definition, so those diffs would read the operator's + // typo as "the script no longer declares this volume / deployment / + // service / schedule" and destroy the state behind it. + let applied = outcome.is_applied(); + if let ReloadOutcome::KeptPrevious(e) = &outcome { + tracing::warn!( + app = %name, + error = %e, + "script failed to evaluate; keeping the previous definition and its derived state" + ); + } // r[impl actuate.volume.hold] // Diff previous vs current volume resources and hold anything the new // script dropped. Runs synchronously with the update so there's no // window where the operator sees the old volume as gone but the on-disk // data hasn't been relocated yet. - { + if applied { let current_named_volumes: std::collections::HashSet = { let reg = state.registry.read(); reg.get(name) @@ -1682,7 +1667,7 @@ pub(crate) fn update_app( } } // r[impl scaling.clamp] - { + if applied { let reg = state.registry.read(); if let Some(entry) = reg.get(name) { let def = entry.app.def.load(); @@ -1751,10 +1736,9 @@ pub(crate) fn update_app( .map_err(|e| OiError::new(ErrorCode::NotFound, format!("db update generation: {e}")))?; } - let op_in_progress = false; // i[forward.script-update] — tear down any forward whose target service is // no longer present in the new AppDef. - if !op_in_progress { + if applied { let valid_services: std::collections::HashSet = { let reg = state.registry.read(); if let Some(entry) = reg.get(name) { @@ -1777,11 +1761,13 @@ pub(crate) fn update_app( } } - // r[impl schedule.prune] - sync_action_schedules(state, ¶ms.app); + if applied { + // r[impl schedule.prune] + sync_action_schedules(state, ¶ms.app); - // r[impl image.pin.update-reconcile] - super::images::reconcile_pins_post_update(state, ¶ms.app); + // r[impl image.pin.update-reconcile] + super::images::reconcile_pins_post_update(state, ¶ms.app); + } tracing::info!(app = %name, generation, "updated app"); ctx.events.app_updated( diff --git a/crates/core/src/oi/handler/apps/tests.rs b/crates/core/src/oi/handler/apps/tests.rs index 82e53efe..d11a10ee 100644 --- a/crates/core/src/oi/handler/apps/tests.rs +++ b/crates/core/src/oi/handler/apps/tests.rs @@ -480,3 +480,154 @@ fn unstop_all_clears_every_stopped_resource() { .unwrap_err(); assert_eq!(code, "not_found"); } + +/// An app whose definition carries every kind of state `/apps/update` derives +/// by diffing the registry: a named volume, a scaled deployment, and a +/// scheduled action. +const DERIVED_STATE_SCRIPT: &str = r#" + app.volume("data"); + app.deployment("web") + .image("docker.io/library/nginx:1.29") + .scale(1..4); + app.on_action("vacuum", |rt, _p| { + rt.start(app.job().image("docker.io/library/busybox:1.37").command("true")) + .terminated(); + }).on_schedule("H 3 * * *"); +"#; + +// i[verify app.update] +// A script that throws part-way through evaluation used to have its partial +// result swapped into the registry, after which every post-update diff read +// the resources declared below the throw as deleted — holding live volume +// data, wiping scaling decisions, tearing down forwards, and pruning +// schedules, all from a typo. +#[test] +fn failed_update_leaves_derived_state_untouched() { + let oi = TestOi::new(); + oi.call( + "/apps/create", + json!({ "app": "demo", "script": DERIVED_STATE_SCRIPT }), + ) + .unwrap(); + oi.install("demo"); + oi.call( + "/apps/scale", + json!({ "app": "demo", "deployment": "web", "scale": 3 }), + ) + .unwrap(); + + let schedules_before = oi + .state + .db + .call(|db| { + crate::runtime::db::list_schedules( + db, + &seedling_protocol::names::AppName::new("demo").unwrap(), + ) + }) + .unwrap(); + assert_eq!(schedules_before.len(), 1, "precondition: schedule exists"); + + // Declares the deployment, then throws. Everything below the throw — + // the volume, the scale bounds, the scheduled action — is absent from + // the partial evaluation. + let broken = r#" + app.deployment("web").image("docker.io/library/nginx:1.29"); + throw "typo"; + app.volume("data"); + "#; + // i[verify app.update] — the request still succeeds. + oi.call("/apps/update", json!({ "app": "demo", "script": broken })) + .unwrap(); + + let desc = oi.call("/apps/show", json!({ "app": "demo" })).unwrap(); + + let resources = desc["resources"].as_array().unwrap(); + assert!( + resources + .iter() + .any(|r| r["type"] == "volume" && r["name"] == "data"), + "the volume must survive a failed update: {resources:#?}" + ); + + let web = resources + .iter() + .find(|r| r["name"] == "web") + .expect("deployment still present"); + assert_eq!( + web["scale"]["current"], 3, + "scaling decisions must not be clamped against a partial definition" + ); + + let schedules_after = oi + .state + .db + .call(|db| { + crate::runtime::db::list_schedules( + db, + &seedling_protocol::names::AppName::new("demo").unwrap(), + ) + }) + .unwrap(); + assert_eq!( + schedules_after.len(), + 1, + "the scheduled action must not be pruned" + ); + + // The operator is still told what went wrong. + let faults = desc["faults"].as_array().unwrap(); + assert!( + faults.iter().any(|f| f["kind"] == "script_error"), + "a script_error fault must be filed: {faults:#?}" + ); +} + +// i[verify app.register] +// A registration that could not be persisted must not be observable: the +// in-memory entry used to survive the failed DB write, so `/apps/list` showed +// an app that a restart silently dropped (load_from_db skips generation-0 +// rows) and a retried `/apps/create` was rejected as already registered. +#[test] +fn failed_registration_leaves_nothing_behind() { + let oi = TestOi::new(); + + // Break the generation write specifically: the app row is written first, + // so this exercises the half-committed case rather than a total failure. + oi.state + .db + .call(|db| { + db.conn + .execute("ALTER TABLE generations RENAME TO generations_hidden", []) + }) + .expect("hide generations table"); + + let err = oi.call( + "/apps/create", + json!({ "app": "demo", "script": MINIMAL_SCRIPT }), + ); + assert!(err.is_err(), "registration must fail when persistence does"); + + let list = oi.call("/apps/list", json!({})).unwrap(); + assert!( + list.as_array().unwrap().is_empty(), + "a failed registration must not appear in listings: {list:#?}" + ); + + oi.state + .db + .call(|db| { + db.conn + .execute("ALTER TABLE generations_hidden RENAME TO generations", []) + }) + .expect("restore generations table"); + + // The retry contract: nothing was left to collide with. + let result = oi + .call( + "/apps/create", + json!({ "app": "demo", "script": MINIMAL_SCRIPT }), + ) + .expect("retry after a transient persistence failure must succeed"); + assert_eq!(result["generation"], 1); +} diff --git a/crates/core/src/oi/handler/params.rs b/crates/core/src/oi/handler/params.rs index 5914a02e..290079cf 100644 --- a/crates/core/src/oi/handler/params.rs +++ b/crates/core/src/oi/handler/params.rs @@ -108,7 +108,10 @@ fn reload_and_persist_apperror( )) }) .map_err(|e| OiError::new(ErrorCode::NotFound, format!("db error: {e}")))?; - state + // The outcome needs no gating here: this re-evaluates the app's existing + // script under new param values and derives nothing from the result but + // the fault state synced below, which is filed on either outcome. + let _ = state .registry .write() .reload(app, script, &loaded_params, &state.script_limits); diff --git a/crates/core/src/oi/handler/registries.rs b/crates/core/src/oi/handler/registries.rs index d3e446f0..6544842b 100644 --- a/crates/core/src/oi/handler/registries.rs +++ b/crates/core/src/oi/handler/registries.rs @@ -70,7 +70,9 @@ fn re_evaluate_all_apps(state: &OiState) { let loaded_params = state .db .call(move |db| apps::load_all_params_for_app(db, &cipher, &name_clone)); - state + // Re-evaluating each app's existing script after an allowlist change: + // nothing downstream diffs the definition, so either outcome is fine. + let _ = state .registry .write() .reload(name, script, &loaded_params, &state.script_limits); diff --git a/crates/core/src/runtime/apps.rs b/crates/core/src/runtime/apps.rs index 8686755e..67b9b2a8 100644 --- a/crates/core/src/runtime/apps.rs +++ b/crates/core/src/runtime/apps.rs @@ -33,6 +33,33 @@ impl std::fmt::Display for ScriptError { impl std::error::Error for ScriptError {} +/// What [`AppRegistry::reload`] did to the registry. +/// +/// `evaluate_script` always returns an `App`, populated up to wherever the +/// script threw. That contract is right for registration, where there is no +/// previous definition to lose; it is wrong for reload, where publishing a +/// partial definition lets every consumer diff live state against a truncated +/// one. The distinction is in the type so callers have to make it. +#[derive(Debug)] +#[must_use] +pub enum ReloadOutcome { + /// The script evaluated cleanly and the registry now holds its definition. + Applied, + /// Evaluation failed. The previous definition keeps running and the new + /// script text and error are recorded; no state derived from the new + /// script may be diffed against the registry. + KeptPrevious(ScriptError), + /// The app is not registered, so nothing was evaluated or stored. + NotRegistered, +} + +impl ReloadOutcome { + /// Whether the registry now reflects the script that was passed in. + pub fn is_applied(&self) -> bool { + matches!(self, Self::Applied) + } +} + /// The installation phase of an app. Stored in `registered_apps` and shared /// with the reconciler via Arc so the reconciler can transition it on cleanup. // i[impl app.status] @@ -120,8 +147,28 @@ impl AppRegistry { tick_notify: Arc, limits: &crate::ScriptLimits, ) -> Result<(), ScriptError> { - let (app, raw_error) = evaluate_script(&name, &script, &BTreeMap::new(), limits); - let script_error = raw_error.map(|e| { + let (app, script_error) = evaluate_script(&name, &script, &BTreeMap::new(), limits); + self.insert_registered(name, script, app, script_error, tick_notify, 0); + Ok(()) + } + + // i[impl app.register] + /// Make an already-evaluated app observable at `generation`. + /// + /// Separate from [`Self::register`] so a caller can commit the app's rows + /// before the entry exists: a registration whose persistence failed must + /// not leave an app that `/apps/list` shows, a restart silently drops, + /// and a retried `/apps/create` rejects as already registered. + pub fn insert_registered( + &mut self, + name: AppName, + script: String, + app: App, + script_error: Option, + tick_notify: Arc, + generation: u64, + ) { + let script_error = script_error.map(|e| { tracing::warn!(app = %name, error = %e, "script has errors at registration; params may need to be set"); (e.to_string(), Timestamp::now()) }); @@ -135,10 +182,9 @@ impl AppRegistry { active_progress: Arc::new(RwLock::new(None)), tick_notify, script_error, - current_generation: 0, + current_generation: generation, }, ); - Ok(()) } pub fn deregister(&mut self, name: &str) -> bool { @@ -153,18 +199,48 @@ impl AppRegistry { /// On success the entry's app and script are updated and any active /// script-error fault is cleared. On failure the existing AppDef keeps /// running and the fault is recorded — the caller always succeeds. + /// + /// The returned outcome is what tells a caller whether the registry now + /// holds a definition derived from `script`. Anything that diffs the + /// registry against previous state — volume holds, scaling bounds, + /// forwards, schedules — is only meaningful on [`ReloadOutcome::Applied`]. + #[must_use = "a KeptPrevious reload must not be followed by state derived from the new script"] pub fn reload( &mut self, name: &AppName, script: String, params: &BTreeMap, limits: &crate::ScriptLimits, - ) { + ) -> ReloadOutcome { + // Checked before evaluating: an unregistered app has no definition to + // replace, and reporting `Applied` for one would tell a caller the + // registry reflects a script it never stored. + if !self.entries.contains_key(name.as_str()) { + return ReloadOutcome::NotRegistered; + } let (app, raw_error) = evaluate_script(name, &script, params, limits); - if let Some(entry) = self.entries.get_mut(name.as_str()) { - entry.script = script; - entry.app = app; - entry.script_error = raw_error.map(|e| (e.to_string(), Timestamp::now())); + let entry = self + .entries + .get_mut(name.as_str()) + .expect("checked just above"); + // The script text follows the new generation either way: /apps/show + // must return what the operator submitted, and a later param set has + // to re-evaluate that same text rather than resurrect the old one. + entry.script = script; + match raw_error { + None => { + entry.app = app; + entry.script_error = None; + ReloadOutcome::Applied + } + // i[impl app.update] — `app` here is a partial evaluation: the + // builders that ran before the script threw, and none after. It is + // dropped rather than published, so the previous good definition + // keeps running and nothing downstream can diff against it. + Some(e) => { + entry.script_error = Some((e.to_string(), Timestamp::now())); + ReloadOutcome::KeptPrevious(e) + } } } diff --git a/crates/core/src/runtime/apps/tests.rs b/crates/core/src/runtime/apps/tests.rs index 185e3e90..6b125541 100644 --- a/crates/core/src/runtime/apps/tests.rs +++ b/crates/core/src/runtime/apps/tests.rs @@ -472,12 +472,13 @@ fn reload_replaces_script_on_existing_entry() { .unwrap(); let new_script = r#"app.deployment("api").image("ghcr.io/acme/api:1.0");"#; - reg.reload( + let outcome = reg.reload( &app("myapp"), new_script.to_owned(), &BTreeMap::new(), &crate::ScriptLimits::default(), ); + assert!(outcome.is_applied()); let entry = reg.get("myapp").unwrap(); assert_eq!(entry.script, new_script); assert!(entry.script_error.is_none()); @@ -666,16 +667,74 @@ fn script_at_nonexistent_generation_is_none() { ); } +// i[verify app.update] +// A script that throws part-way leaves a partially-populated App behind. +// Publishing it would tell every downstream diff — volume hold, scaling +// bounds, forwards, schedules — that the resources declared after the throw +// were deleted from the app. +#[test] +fn failed_reload_keeps_the_previous_definition() { + let mut reg = AppRegistry::new(); + let notify = Arc::new(Notify::new()); + let original = r#" + app.deployment("api").image("ghcr.io/acme/api:1.0"); + app.volume("data"); + "#; + reg.register( + app("myapp"), + original.to_owned(), + notify, + &crate::ScriptLimits::default(), + ) + .unwrap(); + + // Declares the deployment, then throws before reaching the volume: the + // partial evaluation looks exactly like "the operator removed the volume". + let broken = r#" + app.deployment("api").image("ghcr.io/acme/api:1.0"); + throw "boom"; + app.volume("data"); + "#; + let outcome = reg.reload( + &app("myapp"), + broken.to_owned(), + &BTreeMap::new(), + &crate::ScriptLimits::default(), + ); + + assert!( + matches!(outcome, ReloadOutcome::KeptPrevious(_)), + "a throwing script must not be reported as applied" + ); + let entry = reg.get("myapp").unwrap(); + let def = entry.app.def.load(); + assert!( + def.resources + .keys() + .any(|id| id.kind == crate::defs::resource::ResourceKind::Volume + && id.name.as_str() == "data"), + "the previous definition's volume must still be present" + ); + // The submitted text and the fault are both recorded: the operator sees + // what they sent and why it did not take effect. + assert_eq!(entry.script, broken); + assert!(entry.script_error.is_some()); +} + // i[verify app.update] #[test] fn reload_of_unknown_app_is_noop() { let mut reg = AppRegistry::new(); - // No panic, no registration. - reg.reload( + // No panic, no registration — and not reported as applied, since a caller + // gating destructive work on `is_applied` must not be told the registry + // holds a definition it never stored. + let outcome = reg.reload( &app("ghost"), trivial_script().to_owned(), &BTreeMap::new(), &crate::ScriptLimits::default(), ); + assert!(matches!(outcome, ReloadOutcome::NotRegistered)); + assert!(!outcome.is_applied()); assert!(!reg.is_registered("ghost")); } diff --git a/crates/core/src/system/reconcile.rs b/crates/core/src/system/reconcile.rs index 6820f80d..c26be959 100644 --- a/crates/core/src/system/reconcile.rs +++ b/crates/core/src/system/reconcile.rs @@ -266,6 +266,48 @@ pub(crate) struct RunningPod { pub observed_healthy: bool, } +/// Whether a tick's view of the installed apps is complete enough to apply as +/// absolute state. +/// +/// Routes, nftables rules and the proxy config are rebuilt from the tick's +/// snapshots and applied wholesale: whatever is not in the computed set is +/// removed from the host. So an app that fell out of the set because its +/// desired state or a registry lookup errored is not "an app that contributes +/// nothing" — it is an app whose DNAT, service routes and vhosts are about to +/// be withdrawn while its containers keep running. +/// +/// The reconciler already applies this rule to one failure: a Caddy bring-up +/// failure skips the nftables and proxy applies for the tick rather than +/// applying a reduced state. `Partial` extends it to the rest. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum Coverage { + Complete, + Partial, +} + +impl Coverage { + fn of(dropped: usize) -> Self { + if dropped == 0 { + Self::Complete + } else { + Self::Partial + } + } + + fn is_complete(self) -> bool { + self == Self::Complete + } + + /// Weaken to `Partial` if `other` is. + fn and(self, other: Self) -> Self { + if self.is_complete() && other.is_complete() { + Self::Complete + } else { + Self::Partial + } + } +} + /// Point-in-time snapshot of a single app's state, taken at tick start. struct AppSnapshot { name: AppName, @@ -480,9 +522,16 @@ impl Reconciler { // r[desired-state.definition] // r[desired-state.steady] // r[desired-state.during-operation] - fn snapshot_all_apps(&self) -> Vec { + /// + /// Returns the snapshots alongside how many installed apps were dropped + /// because computing their desired state failed. The count is not + /// diagnostic: routes, nftables rules and the proxy config are applied as + /// absolute state, so a dropped app is indistinguishable from a deleted + /// one unless the tick is told the set is incomplete. + fn snapshot_all_apps(&self) -> (Vec, usize) { let reg = self.app_registry.read(); let mut snapshots = Vec::new(); + let mut skipped = 0usize; for (name, status) in reg.list() { let entry = match reg.get(name.as_str()) { Some(e) => e, @@ -558,6 +607,7 @@ impl Reconciler { &name, &format!("failed to compute desired state: {e}"), ); + skipped += 1; continue; } }; @@ -576,7 +626,7 @@ impl Reconciler { current_generation, }); } - snapshots + (snapshots, skipped) } /// Build the effective-scale map for every Deployment in an app. @@ -670,12 +720,26 @@ impl Reconciler { self.reconcile_stray_shells().await; - let apps = self.snapshot_all_apps(); + let (apps, dropped_apps) = self.snapshot_all_apps(); if apps.is_empty() { - self.tear_down_idle().await; + // r[impl reconciliation.absolute-state] — an empty snapshot list + // means "no app is installed" only when nothing was dropped on the + // way here. Tearing down on a transient registry error would flush + // every rule and remove Caddy, the resolver and NAT64 while the + // workloads keep running. + if dropped_apps == 0 { + self.tear_down_idle().await; + } else { + warn!( + dropped = dropped_apps, + "every installed app failed to compute its desired state; \ + holding the data plane rather than tearing it down" + ); + } return false; } + let app_coverage = Coverage::of(dropped_apps); // r[impl infra.nat64.translator.lifecycle] // Wake-from-idle: ensure NAT64 translator is installed before any @@ -860,7 +924,11 @@ impl Reconciler { }; // --- Compute routes (sync) --- - let (all_routes, route_obs) = phases::compute_routes( + let phases::RoutesBuild { + routes: all_routes, + observations: route_obs, + coverage: routes_coverage, + } = phases::compute_routes( &apps, &running_pods_by_app, &self.node_prefix, @@ -868,6 +936,7 @@ impl Reconciler { &ext_snapshot, &make_resolve_ctx(), ); + let routes_coverage = routes_coverage.and(app_coverage); self.persist_obs(route_obs); // --- Classify site-service endpoint outcomes and reconcile the @@ -933,16 +1002,20 @@ impl Reconciler { let phases::NftablesBuild { rules: dp_rules, degraded_services_by_app, + coverage: rules_coverage, } = nft_build; + let rules_coverage = rules_coverage.and(app_coverage); // r[impl fault.service-degraded] self.file_service_degraded_faults(&apps, °raded_services_by_app); let phases::ProxyBuildResult { config: proxy_config, + coverage: proxy_coverage, observations: proxy_obs, ready_observations: proxy_ready_obs, conflicts: ingress_conflicts, unresolved_site_attachments, } = proxy_build; + let proxy_coverage = proxy_coverage.and(app_coverage); // r[impl ingress.site.conflict] self.reconcile_ingress_conflicts(&ingress_conflicts); // r[impl ingress.site.attachment] @@ -1006,32 +1079,79 @@ impl Reconciler { self.persist_obs(proxy_obs); + // r[impl reconciliation.absolute-state] — each of these three + // replaces the host's whole state for its plane, so an + // incomplete build must be withheld rather than applied: the + // apps missing from it are still running. + for (plane, coverage) in [ + ("routes", routes_coverage), + ("nftables rules", rules_coverage), + ("proxy config", proxy_coverage), + ] { + if !coverage.is_complete() { + warn!( + "an app is missing from this tick's {plane}; \ + holding the previous state rather than applying a reduced one" + ); + } + } + + // `None` means the apply was withheld because the build did + // not cover every app. It is deliberately not `Ok(())`: no + // apply ran, so nothing may be concluded from it — in + // particular the plane's fault must neither be filed nor + // cleared, and no readiness may be recorded. let (routes_res, rules_res, proxy_res) = tokio::join!( - self.driver.data_plane.apply_routes(&all_routes), - self.driver.data_plane.apply_rules(&dp_rules), async { - if has_proxy_config { - self.driver.proxy.apply_config(&proxy_config).await - } else { - Ok(()) + match routes_coverage.is_complete() { + true => Some(self.driver.data_plane.apply_routes(&all_routes).await), + false => None, + } + }, + async { + match rules_coverage.is_complete() { + true => Some(self.driver.data_plane.apply_rules(&dp_rules).await), + false => None, + } + }, + async { + match proxy_coverage.is_complete() { + true if has_proxy_config => { + Some(self.driver.proxy.apply_config(&proxy_config).await) + } + // An empty config is not applied, as before; that + // is a decision about the config, not a withheld + // apply, so it keeps reporting success. + true => Some(Ok(())), + false => None, } }, ); - if let Err(e) = routes_res { - error!(error = %e, "routes: apply_routes failed"); - self.file_system_fault("routes_failed", &format!("apply_routes failed: {e}")); - } else { - self.clear_system_fault("routes_failed"); + match routes_res { + Some(Err(e)) => { + error!(error = %e, "routes: apply_routes failed"); + self.file_system_fault( + "routes_failed", + &format!("apply_routes failed: {e}"), + ); + } + Some(Ok(())) => self.clear_system_fault("routes_failed"), + None => {} } - if let Err(e) = rules_res { - error!(error = %e, "rules: apply_rules failed"); - self.file_system_fault("nftables_failed", &format!("apply_rules failed: {e}")); - } else { - self.clear_system_fault("nftables_failed"); + match rules_res { + Some(Err(e)) => { + error!(error = %e, "rules: apply_rules failed"); + self.file_system_fault( + "nftables_failed", + &format!("apply_rules failed: {e}"), + ); + } + Some(Ok(())) => self.clear_system_fault("nftables_failed"), + None => {} } match proxy_res { - Err(e) => { + Some(Err(e)) => { error!(error = ?e, addr = %caddy_ip, "proxy: apply_config failed"); self.file_system_fault( "proxy_failed", @@ -1065,7 +1185,7 @@ impl Reconciler { &format!("apply_config failed: {e}"), ); } - Ok(()) if has_proxy_config => { + Some(Ok(())) if has_proxy_config => { self.clear_system_fault("proxy_failed"); // r[impl fault.proxy-apply-failed] self.clear_proxy_apply_failed_faults(); @@ -1081,19 +1201,25 @@ impl Reconciler { ); } } - Ok(()) => {} + Some(Ok(())) | None => {} } // r[impl observe.ingress.certs] self.observe_warm_certs(&apps).await; } None => { - // Caddy unavailable — still apply routes (they don't need caddy). - if let Err(e) = self.driver.data_plane.apply_routes(&all_routes).await { - error!(error = %e, "routes: apply_routes failed"); - self.file_system_fault("routes_failed", &format!("apply_routes failed: {e}")); - } else { - self.clear_system_fault("routes_failed"); + // Caddy unavailable — still apply routes (they don't need + // caddy), unless this tick's route set is missing an app. + if routes_coverage.is_complete() { + if let Err(e) = self.driver.data_plane.apply_routes(&all_routes).await { + error!(error = %e, "routes: apply_routes failed"); + self.file_system_fault( + "routes_failed", + &format!("apply_routes failed: {e}"), + ); + } else { + self.clear_system_fault("routes_failed"); + } } } } @@ -1501,3 +1627,165 @@ impl Reconciler { } } } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use crate::runtime::registry::{RegistryError, ScaledGroup}; + + use super::*; + + /// A registry that fails for one named app and succeeds for every other. + /// + /// The stub `System` cannot make a registry lookup fail, which is why the + /// existing tests all passed while the reconciler withdrew a live app's + /// data plane on a transient error. + struct RegistryFailingFor { + broken: AppName, + inner: crate::runtime::registry::EphemeralInstanceRegistry, + } + + impl InstanceRegistry for RegistryFailingFor { + fn get_or_create_singleton( + &self, + app: &AppName, + kind: crate::defs::resource::ResourceKind, + name: Option<&str>, + ) -> Result { + if app == &self.broken { + return Err(RegistryError::message("registry unavailable")); + } + self.inner.get_or_create_singleton(app, kind, name) + } + + fn ensure_scaled_group( + &self, + app: &AppName, + kind: crate::defs::resource::ResourceKind, + name: Option<&str>, + count: u16, + ) -> Result { + if app == &self.broken { + return Err(RegistryError::message("registry unavailable")); + } + self.inner.ensure_scaled_group(app, kind, name, count) + } + + fn find_all_instances( + &self, + app: &AppName, + kind: crate::defs::resource::ResourceKind, + name: Option<&str>, + ) -> Result, RegistryError> { + if app == &self.broken { + return Err(RegistryError::message("registry unavailable")); + } + self.inner.find_all_instances(app, kind, name) + } + } + + fn snapshot_with_service(name: &str) -> AppSnapshot { + let app_name = AppName::new(name).unwrap(); + let (app, err) = crate::runtime::apps::evaluate_script( + &app_name, + r#"app.service("api").http(8080);"#, + &std::collections::BTreeMap::new(), + &crate::ScriptLimits::default(), + ); + assert!(err.is_none(), "test script must evaluate: {err:?}"); + let app_def = (*app.def.load_full()).clone(); + AppSnapshot { + name: app_name, + desired: DesiredState::default(), + app_def, + phase: AppPhase::Installed, + phase_handle: Arc::new(Mutex::new(AppPhase::Installed)), + warm_cert_hostnames: Default::default(), + current_generation: 1, + } + } + + // r[verify reconciliation.absolute-state] + // Routes are applied wholesale, so an app dropped from the computed set + // has its live routes deleted. The tick has to know the set is short. + #[test] + fn routes_report_partial_coverage_when_an_app_is_dropped() { + let apps = vec![ + snapshot_with_service("healthy"), + snapshot_with_service("broken"), + ]; + let registry = RegistryFailingFor { + broken: AppName::new("broken").unwrap(), + inner: crate::runtime::registry::EphemeralInstanceRegistry::new(), + }; + let empty_lookup = crate::runtime::site_services::resolve::EmptyLookup; + let resolve_ctx = ResolveCtx { + nat64_active: false, + has_ipv6_egress: true, + resolver: &empty_lookup, + }; + + let build = phases::compute_routes( + &apps, + &HashMap::new(), + &"fd5e:ed11:9000::/48".parse().unwrap(), + ®istry, + &crate::runtime::external_service_mappings::ExternalServiceSnapshot::default(), + &resolve_ctx, + ); + + assert_eq!( + build.coverage, + Coverage::Partial, + "a dropped app must not look like a complete route set" + ); + assert_eq!( + build.routes.len(), + 1, + "the healthy app still contributes its route" + ); + } + + // r[verify reconciliation.absolute-state] + #[test] + fn routes_report_complete_coverage_when_every_app_builds() { + let apps = vec![ + snapshot_with_service("healthy"), + snapshot_with_service("other"), + ]; + let registry = RegistryFailingFor { + broken: AppName::new("nobody").unwrap(), + inner: crate::runtime::registry::EphemeralInstanceRegistry::new(), + }; + let empty_lookup = crate::runtime::site_services::resolve::EmptyLookup; + let resolve_ctx = ResolveCtx { + nat64_active: false, + has_ipv6_egress: true, + resolver: &empty_lookup, + }; + + let build = phases::compute_routes( + &apps, + &HashMap::new(), + &"fd5e:ed11:9000::/48".parse().unwrap(), + ®istry, + &crate::runtime::external_service_mappings::ExternalServiceSnapshot::default(), + &resolve_ctx, + ); + + assert_eq!(build.coverage, Coverage::Complete); + assert_eq!(build.routes.len(), 2); + } + + // r[verify reconciliation.absolute-state] + // The whole point of the distinction: an empty app set means "nothing is + // installed" only when nothing was dropped getting there. + #[test] + fn coverage_distinguishes_idle_from_dropped() { + assert!(Coverage::of(0).is_complete()); + assert!(!Coverage::of(1).is_complete()); + assert!(!Coverage::Complete.and(Coverage::Partial).is_complete()); + assert!(Coverage::Complete.and(Coverage::Complete).is_complete()); + } +} diff --git a/crates/core/src/system/reconcile/phases.rs b/crates/core/src/system/reconcile/phases.rs index 125ae94c..af5fa295 100644 --- a/crates/core/src/system/reconcile/phases.rs +++ b/crates/core/src/system/reconcile/phases.rs @@ -6,7 +6,7 @@ use std::{ use ipnet::Ipv6Net; use seedling_protocol::names::{AppName, SiteServiceName}; -use super::{AppSnapshot, RunningPod, pods, proxy, routes, rules, site_proxy, volumes}; +use super::{AppSnapshot, Coverage, RunningPod, pods, proxy, routes, rules, site_proxy, volumes}; use crate::{ runtime::{ AppPhase, InstanceRegistry, @@ -74,6 +74,18 @@ pub(super) async fn run_volumes_phase( futures_util::future::join_all(futures).await } +pub(super) struct RoutesBuild { + pub routes: Vec, + pub observations: Vec<( + crate::runtime::identity::ResourceInstance, + &'static str, + serde_json::Value, + )>, + /// `Partial` when an app's routes could not be built. Its service routes + /// are absent from `routes`, which replaces the host's whole route set. + pub coverage: Coverage, +} + pub(super) fn compute_routes( apps: &[AppSnapshot], running_pods_by_app: &HashMap>, @@ -81,16 +93,10 @@ pub(super) fn compute_routes( registry: &dyn InstanceRegistry, ext_snapshot: &ExternalServiceSnapshot, resolve_ctx: &ResolveCtx<'_>, -) -> ( - Vec, - Vec<( - crate::runtime::identity::ResourceInstance, - &'static str, - serde_json::Value, - )>, -) { +) -> RoutesBuild { let mut all_routes = Vec::new(); let mut all_obs = Vec::new(); + let mut dropped = 0usize; for app in apps { if app.phase == AppPhase::Uninstalling { continue; @@ -113,13 +119,18 @@ pub(super) fn compute_routes( Ok(pair) => pair, Err(e) => { tracing::warn!(app = %app.name, error = %e, "routes: registry lookup failed for app; skipping"); + dropped += 1; continue; } }; all_routes.extend(routes); all_obs.extend(obs); } - (all_routes, all_obs) + RoutesBuild { + routes: all_routes, + observations: all_obs, + coverage: Coverage::of(dropped), + } } /// Per-tick classification of every site-service endpoint into the kinds of @@ -183,6 +194,9 @@ pub(super) fn classify_site_service_endpoints( pub(super) struct NftablesBuild { pub rules: DataPlaneRules, pub degraded_services_by_app: HashMap>, + /// `Partial` when an app's rules could not be built. Its DNAT and mount + /// rules are absent from `rules`, which is applied wholesale. + pub coverage: Coverage, } #[expect( @@ -205,6 +219,7 @@ pub(super) fn compute_nftables_rules( let mut all_mounts = Vec::new(); let mut all_service_dnat = Vec::new(); let mut degraded_by_app: HashMap> = HashMap::new(); + let mut dropped = 0usize; for app in apps { if app.phase == AppPhase::Uninstalling { continue; @@ -236,6 +251,7 @@ pub(super) fn compute_nftables_rules( } Err(e) => { tracing::warn!(app = %app.name, error = %e, "nftables: registry lookup failed for app; skipping"); + dropped += 1; continue; } } @@ -247,11 +263,16 @@ pub(super) fn compute_nftables_rules( service_dnat: all_service_dnat, }, degraded_services_by_app: degraded_by_app, + coverage: Coverage::of(dropped), } } pub(super) struct ProxyBuildResult { pub config: crate::system::types::ProxyConfig, + /// `Partial` when an app's ingresses could not be built. Its virtual + /// hosts and L4 routes are absent from `config`, which replaces the + /// proxy's whole configuration. + pub coverage: Coverage, pub observations: Vec<( crate::runtime::identity::ResourceInstance, &'static str, @@ -337,6 +358,7 @@ pub(super) fn compute_proxy_config( let mut observations = Vec::new(); let mut ready_observations = Vec::new(); let mut all_warm: std::collections::BTreeSet = std::collections::BTreeSet::new(); + let mut dropped = 0usize; for app in apps { if app.phase == AppPhase::Uninstalling { continue; @@ -357,6 +379,7 @@ pub(super) fn compute_proxy_config( Ok(b) => b, Err(e) => { tracing::warn!(app = %app.name, error = %e, "proxy: registry lookup failed for app; skipping"); + dropped += 1; continue; } }; @@ -436,6 +459,7 @@ pub(super) fn compute_proxy_config( ProxyBuildResult { config, + coverage: Coverage::of(dropped), observations, ready_observations, conflicts, diff --git a/crates/web/frontend/package-lock.json b/crates/web/frontend/package-lock.json index cacd99ff..ca6351d2 100644 --- a/crates/web/frontend/package-lock.json +++ b/crates/web/frontend/package-lock.json @@ -1129,9 +1129,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1149,9 +1146,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1169,9 +1163,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1189,9 +1180,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1209,9 +1197,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -1229,9 +1214,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ diff --git a/docs/spec/interface.md b/docs/spec/interface.md index b195f200..c714c34c 100644 --- a/docs/spec/interface.md +++ b/docs/spec/interface.md @@ -188,6 +188,7 @@ Absent specification bugs, anything that is not defined here is either defined i > `/apps/create { app, script }` evaluates the provided BSL script source text. > On success, the app is added to the managed set in the `NotInstalled` state and an `AppRegistered` event is emitted. > On script failure, `script_error` is returned and the app is not registered. +> A registration that fails for any reason leaves nothing observable behind: the app does not appear in listings, does not survive a restart, and a retried registration of the same name succeeds rather than being rejected as already registered. > i[app.persist] > Registered apps and their BSL scripts are stored durably and reloaded automatically on restart. @@ -201,6 +202,8 @@ Absent specification bugs, anything that is not defined here is either defined i > `/apps/update { app, script }` re-evaluates the provided BSL script source text. > If a lifecycle operation is in progress for the app, or one is queued, the request is rejected with `operation_in_progress`. > If the script fails to parse or evaluate, a `script_error` app-level fault is filed, the existing AppDef continues running, and the request still succeeds. +> "Continues running" extends to every piece of state derived from the definition: volume data is not held, scaling decisions are not clamped, port forwards are not torn down, and action schedules are not pruned. +> A partially-evaluated definition is never observable — an update that failed part-way through must not be distinguishable, in any state derived from the definition, from an update that was never submitted. > On success, any previously active `script_error` fault for this app is cleared, and the app's [generation](#r--generation.definition) is bumped with a `ScriptUpdate` history entry. > i[app.generation] diff --git a/docs/spec/runtime.md b/docs/spec/runtime.md index ebe85c8e..fde3d3b4 100644 --- a/docs/spec/runtime.md +++ b/docs/spec/runtime.md @@ -31,6 +31,14 @@ Absent specification bugs, anything that is not defined here is either defined i > Individual reconciliation operations must not block the loop for an unbounded or long duration. > When an operation requires waiting for an external condition (e.g. a process to terminate), the reconciler must release control and re-evaluate the condition on a subsequent iteration rather than polling inline. +> r[reconciliation.absolute-state] +> Some state the reconciler maintains is *absolute*: it is rebuilt in full each iteration and applied wholesale, so anything absent from what was built is removed from the host. +> An installed app's contribution to absolute state may be withdrawn only by an explicit [lifecycle](#r--lifecycle.states) transition — never because computing that contribution failed. +> When an iteration cannot compute the contribution of every installed app, the affected absolute state must not be applied for that iteration; the previously applied state stands until an iteration succeeds. +> An iteration that withholds an apply must draw no conclusion from having done so: the fault for that state is neither filed nor cleared, and no resource is recorded as ready on the strength of an apply that did not run. +> Likewise, a full teardown of shared infrastructure occurs only when no app is installed or installing — never when apps are installed but their state could not be computed. +> This does not constrain per-resource incremental actuation, which acts only on the resources it names. + # Script Engine Limits > r[engine.limits]