fix: on failure, keep the previous good state (audit theme 2, closes C1) - #138
Conversation
evaluate_script always returns an App, populated up to wherever the script threw. That contract is right for register (no previous definition exists) and wrong for reload, where update_app then diffs live state against the truncated result: a typo halfway down a script reads as "the operator removed everything below this line", and the post-update steps hold the volume data, wipe the scaling decisions, tear down forwards, and prune the schedules accordingly. reload now returns ReloadOutcome so the decision to publish a partial definition has to be made explicitly, and refuses. update_app gates all four derived-state diffs on Applied; the two callers that re-evaluate an app's existing script derive nothing and discard the outcome. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv
…he tick Routes, nftables rules and the proxy config are rebuilt from the tick's snapshots and applied wholesale, so an app dropped because its desired state or a registry lookup errored had its DNAT, service routes and vhosts actively removed while its containers ran on. When every app failed, the empty snapshot list read as idleness and tear_down_idle flushed the rules and removed Caddy, the resolver and NAT64. snapshot_all_apps now reports how many apps it dropped, the three absolute builders report whether they covered every app, and the applies are withheld on a partial view. This is the rule the tick already applies to one failure: a Caddy bring-up failure skips the nftables and proxy applies rather than applying a reduced state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv
register_app inserted into the in-memory registry and then made three separate DB calls. Any failure returned to the client with the entry still there: /apps/list showed an app that a restart silently dropped, because load_from_db skips generation-0 rows, and a retried /apps/create was rejected as already registered. The three writes are now one transaction and the entry is inserted after it commits. set_param and unset_param had the mirror inversion — durable write committed, then an error returned when the on_change dispatch was rejected — so the scheduler admission they already check at entry is now held across the write rather than re-taken after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv
Code Coverage OverviewLanguages: TypeScript, Rust TypeScript / code-coverage/vitestThe overall coverage in commit db4ebb6 in the Rust / code-coverage/rustThe overall coverage in commit db4ebb6 in the Show a code coverage summary of the most impacted files.
Updated |
There was a problem hiding this comment.
Pull request overview
This PR closes “theme 2” from the logic-bug audit by enforcing the invariant “on failure, observable state is unchanged” across three boundaries: app definition reload/update, absolute per-tick data-plane application, and registration/param-change durability.
Changes:
- Introduces
ReloadOutcomeso failed script reloads keep the previous good AppDef and downstream diffs are gated on “Applied”. - Tracks “coverage” in the reconciler tick so absolute state (routes / nftables / proxy config) is withheld when any installed app’s contribution cannot be computed.
- Makes registration persistence transactional (“durable first, observable second”) and strengthens param-change consistency by holding an exclusivity guard through
on_changedispatch.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/spec/runtime.md | Specifies the new absolute-state withholding rule when per-tick coverage is incomplete. |
| docs/spec/interface.md | Tightens /apps/create retry semantics and /apps/update “continues running” to include derived state. |
| crates/web/frontend/package-lock.json | Lockfile update (removes some libc constraints for optional deps). |
| crates/core/src/system/reconcile/phases.rs | Adds per-plane Coverage reporting to route/rules/proxy builders. |
| crates/core/src/system/reconcile.rs | Uses dropped-app counting + coverage to withhold absolute-state applies; adds coverage tests. |
| crates/core/src/runtime/apps.rs | Adds ReloadOutcome; splits insert_registered; changes reload to keep prior def on failure. |
| crates/core/src/oi/handler/registries.rs | Discards reload outcome where no derived-state diff follows. |
| crates/core/src/oi/handler/params.rs | Adds scheduler “claim” concept to prevent durable-write-then-error outcomes. |
| crates/core/src/oi/handler/apps.rs | Registration now commits durably before inserting into registry; update diffs gated on reload being applied. |
| crates/core/src/runtime/apps/tests.rs | Adds unit coverage for “failed reload keeps previous definition”. |
| crates/core/src/oi/handler/apps/tests.rs | Adds OI tests for failed update not destroying derived state + failed registration leaving nothing observable. |
Files not reviewed (1)
- crates/web/frontend/package-lock.json: Generated file
Suppressed comments (1)
crates/core/src/runtime/apps.rs:216
reload()evaluates the script and returnsReloadOutcome::Appliedeven when the app is not registered. This both wastes work and reports an outcome that isn't true (the registry wasn't updated).
let (app, raw_error) = evaluate_script(name, &script, params, limits);
let Some(entry) = self.entries.get_mut(name.as_str()) else {
return ReloadOutcome::Applied;
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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), | ||
| } |
| let scheduler = state.scheduler.lock(); | ||
| if scheduler.has_operation_for(app) { | ||
| return Err(OiError::new( | ||
| ErrorCode::OperationInProgress, | ||
| format!("operation in progress for app: {app}"), |
| async { | ||
| if has_proxy_config { | ||
| if routes_coverage.is_complete() { | ||
| self.driver.data_plane.apply_routes(&all_routes).await | ||
| } else { | ||
| Ok(()) | ||
| } |
| async { | ||
| if rules_coverage.is_complete() { | ||
| self.driver.data_plane.apply_rules(&dp_rules).await | ||
| } else { | ||
| Ok(()) | ||
| } |
| async { | ||
| if has_proxy_config && proxy_coverage.is_complete() { | ||
| self.driver.proxy.apply_config(&proxy_config).await | ||
| } else { | ||
| Ok(()) |
Review catch: skipping apply_routes / apply_rules / apply_config on partial coverage returned Ok(()), which the result handling read as a successful apply — clearing routes_failed, nftables_failed and proxy_failed, and persisting proxy_ready observations, for an apply that never ran. The withheld case is now None and concludes nothing. The caddy-unavailable branch, which applies routes on its own, gets the same gate. Also from review: reload() reported Applied for an unregistered app, which is what is_applied() gates destructive work on; it now returns NotRegistered without evaluating. Backs out the scheduler claim in params.rs. It closed a real race — a rejected on_change dispatch after a committed write — but held the global scheduler mutex across a registry lock, a BSL re-evaluation and several DB round-trips. No other site holds a registry guard while taking the scheduler lock today, so it deadlocks nothing yet, but it inverts the order the rest of the codebase uses and the contention is real. Closing that race properly needs a scheduler reservation the handler can hold without a mutex. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv
|
Three of the five review points were right; pushed in db4ebb6. Withheld applies reported as successful (routes, rules, proxy — three comments, one bug). Correct and the worst of the batch: skipping the apply returned
Lock-order inversion in That leaves the finding it was fixing open: Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- crates/web/frontend/package-lock.json: Generated file
Suppressed comments (2)
docs/spec/interface.md:191
- The i[app.register] spec still says a script failure returns
script_errorand the app is not registered, but the implementation/tests register the app and file ascript_errorfault (and installation is rejected). This makes the spec misleading right next to the new retry-contract text.
> `/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.
crates/core/src/oi/handler/apps.rs:1668
/apps/updatebumps the generation and emits an update event even whenreload()kept the previous definition (script failed). This contradicts the updated spec text (“generation is bumped on each successful /apps/update”) and makes a failed update observably change state (generation/history) even though nothing was applied.
{
let reg = state.registry.read();
if let Some(entry) = reg.get(name) {
sync_fault_state(&state.db, entry);
}
}
Closes cross-cutting theme 2 from the logic bug audit: failure paths that keep partial state instead of the previous good state. See the pattern analysis.`` Contains the audit's one critical finding (C1).
One invariant — on failure, observable state is unchanged — at three boundaries that need different mechanisms, because "observable state" is an in-memory definition, an externally-applied absolute network state, and a registry/DB pair.
(a) A failed reload keeps the previous definition — C1
evaluate_scriptalways returns anApp, populated up to wherever the script threw. That contract is right forregister(no previous definition exists) and wrong forreload, which published it anyway — contradicting its own doc comment andi[app.update]'s "the existing AppDef continues running".update_appthen diffs live state against that truncated definition. A typo halfway down a script reads as "the operator removed everything below this line", and the post-update steps act on it: hold (relocate) the volume data, wipe the scaling decisions, tear down the forwards, prune the schedules — from onethrow.reloadnow returnsReloadOutcome, so publishing a partial definition has to be an explicit decision, and it refuses.update_appgates all four derived-state diffs onApplied. The two callers that re-evaluate an app's existing script (params.rs,registries.rs) derive nothing and discard the outcome explicitly.One fix site, and every future diff added to
update_appis safe by construction, because the registry can no longer hold a partial definition.(b) An app missing from a tick doesn't lose its data plane
Routes, nftables rules and the proxy config are rebuilt each tick and applied wholesale — whatever is absent is removed from the host. A
continuepast an app whosecompute()or registry lookup errored therefore withdrew its DNAT, service routes and vhosts while its containers ran on. Worse, when every app failed, the empty snapshot list read as idleness andtear_down_idleflushed the rules and removed Caddy, the resolver and NAT64.snapshot_all_appsnow reports how many apps it dropped, the three absolute builders report whether they covered every app, and an incomplete build is withheld rather than applied. This is the rule the tick already applied to exactly one failure — a Caddy bring-up failure skips the nftables and proxy applies rather than applying a reduced state (reconcile.rs:796) — extended to the rest. Pod and volume phases are per-app incremental and keep running.(c) Registration commits durably before it becomes observable
register_appinserted into the registry and then made three separate DB calls. Any failure returned to the client with the entry still there:/apps/listshowed an app that a restart silently dropped (load_from_dbskips generation-0 rows) and a retried/apps/createwas rejected as already registered. The three writes are one transaction now, and the entry is inserted after it commits.set_param/unset_paramhad the mirror inversion — durable write committed, then an error returned when theon_changedispatch was rejected. The rejection cases are exactly "an operation for this app is active or queued", which both handlers already check on entry; that admission is now held across the write instead of re-taken after it, soi[param.set]'s "neither the value nor the generation is changed" and its rejection clause can both be true.Findings closed
/apps/updatestill triggers volume hold, scaling wipe, forward teardownset_param/unset_parampersist the change, then return an errorregister_appleaves the app registered in memory when DB persistence failsEnforcement
i[app.update]now extends "continues running" to the state derived from the definition and states that a partially-evaluated definition is never observable;i[app.register]states the retry contract; newr[reconciliation.absolute-state]inruntime.mdstates that an installed app's contribution to absolute state is withdrawn only by a lifecycle transition, and that a full teardown happens only when no app is installed.TestOitest that a failed/apps/updateleaves the volume, the scale of 3, and the schedule row intact while still filingscript_error;compute_routesreportingPartialwhen a stub registry fails for one of two apps; and a registration whose generation write fails leaving nothing in/apps/listand a retry that succeeds.Not in scope
Wrong-but-successfully-evaluated scripts still hold a genuinely deleted volume — that is the feature. Carry-forward of each app's last-good snapshot (so healthy apps still get fresh absolute state on a tick where one app errors) is the refinement past the minimal form here; today a partial tick holds the previous state for the whole plane. Theme 3's failed observations are a different conflation and have their own PR.
Overlap with other themes
Theme 6 (BSL strict validation) stacks on this one and must not land before it: making the defs layer throw on malformed input turns previously-coercing scripts into evaluation failures, which is only safe once a failed evaluation stops triggering the destructive path. Independent of the rest.
Generated by Claude Code