fix: on failure, keep the previous good state (audit theme 2, closes C1)

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(
+        &params.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<u64> {
+            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(
         &params.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<String> = {
             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<String> = {
             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, &params.app);
+    if applied {
+        // r[impl schedule.prune]
+        sync_action_schedules(state, &params.app);
 
-    // r[impl image.pin.update-reconcile]
-    super::images::reconcile_pins_post_update(state, &params.app);
+        // r[impl image.pin.update-reconcile]
+        super::images::reconcile_pins_post_update(state, &params.app);
+    }
 
     tracing::info!(app = %name, generation, "updated app");
     ctx.events.app_updated(

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);
+}

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);

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);