From 715538564e1240a07c35e44b9da9c0ecf3208b28 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 01:32:48 +0000 Subject: [PATCH 1/5] fix: match runtime identities against the record, not the name shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seedling grants itself identifiers in namespaces it does not own exclusively, then recognises "its" objects by string shape or by eight bits of an ID. Uninstall matched units with starts_with("seedling-{app}-"), which is not prefix-free: both app names and resource names may contain hyphens, so uninstalling 'app' matched every unit of a sibling called 'app-db' — and the retry branch reset and stopped that healthy sibling's units every tick while the uninstall never completed. The registry already knows the answer exactly; the prefix scan now only enumerates candidates, and the decision is an exact match against the recorded display names. crates/core/src/reserved.rs is the one home for the names the daemon claims. Creation rejects them, and — because reservation cannot repair a collision that predates it — the destructive consumers also check recorded ownership: the startup sweep skips registered site volumes, and the Tailscale provider acts only on rows whose source is its own discovery instead of livelocking on a manual row's primary key. Static Jobs carry a nil InstanceId, so deriving the pod /64 from uuid[0] gave every static Job on the node the same subnet and netavark refused the second network. The low byte is a hash of the full identity now. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv --- crates/core/src/lib.rs | 1 + crates/core/src/oi/handler/ingresses.rs | 3 + crates/core/src/oi/handler/ingresses/tests.rs | 16 +++ crates/core/src/oi/handler/volumes.rs | 16 +++ crates/core/src/oi/handler/volumes/tests.rs | 23 ++++ crates/core/src/reserved.rs | 109 ++++++++++++++++++ crates/core/src/runtime/history.rs | 17 +++ crates/core/src/runtime/history/tests.rs | 33 ++++++ crates/core/src/runtime/tailscale.rs | 38 +++++- crates/core/src/system/reconcile.rs | 34 +++++- crates/core/src/system/translate/proxy.rs | 85 ++++++++++++-- crates/daemon/src/main.rs | 44 ++++++- docs/spec/runtime.md | 13 +++ 13 files changed, 412 insertions(+), 20 deletions(-) create mode 100644 crates/core/src/reserved.rs diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index cccc8c85..3f2b49d0 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -2,6 +2,7 @@ use rhai::{Engine, Scope}; pub mod defs; pub mod oi; +pub mod reserved; pub mod runtime; pub mod sysconst; pub mod system; diff --git a/crates/core/src/oi/handler/ingresses.rs b/crates/core/src/oi/handler/ingresses.rs index e6a90f7d..f850f017 100644 --- a/crates/core/src/oi/handler/ingresses.rs +++ b/crates/core/src/oi/handler/ingresses.rs @@ -203,6 +203,9 @@ pub(crate) fn create_site_ingress( ctx: &RequestCtx, ) -> HandlerResult { validate_hostname(¶ms.hostname)?; + // r[impl namespace.reserved] + crate::reserved::check_site_ingress_name(¶ms.name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; if matches!(params.tls_provider, TlsProvider::Tailscale) { return Err(OiError::new( ErrorCode::RequirementsInvalid, diff --git a/crates/core/src/oi/handler/ingresses/tests.rs b/crates/core/src/oi/handler/ingresses/tests.rs index 71205525..c248ce39 100644 --- a/crates/core/src/oi/handler/ingresses/tests.rs +++ b/crates/core/src/oi/handler/ingresses/tests.rs @@ -316,3 +316,19 @@ fn discovery_status_and_refresh_without_tailscale() { assert_eq!(code, "requirements_invalid"); assert!(msg.contains("not configured"), "{msg}"); } + +// r[verify namespace.reserved] +// The Tailscale provider disables whatever row holds this name, so a manual +// ingress created under it was permanently stale. +#[test] +fn reserved_site_ingress_name_is_rejected_at_creation() { + let oi = TestOi::new(); + let (code, message) = oi + .call( + "/ingresses/site/create", + json!({ "name": "tailscale", "hostname": "example.com" }), + ) + .unwrap_err(); + assert_eq!(code, "requirements_invalid"); + assert!(message.contains("reserved"), "message: {message}"); +} diff --git a/crates/core/src/oi/handler/volumes.rs b/crates/core/src/oi/handler/volumes.rs index 786a7047..bbbf8c98 100644 --- a/crates/core/src/oi/handler/volumes.rs +++ b/crates/core/src/oi/handler/volumes.rs @@ -122,6 +122,10 @@ pub(crate) fn restore_held( })?, }; + // r[impl namespace.reserved] + crate::reserved::check_site_volume_name(&target_name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; + let target_for_lookup = target_name.clone(); let existing = state .db @@ -277,6 +281,10 @@ pub(crate) fn create_site_volume( ) -> HandlerResult { use crate::runtime::site_volumes::{SiteVolumeDef, SiteVolumeKind}; + // r[impl namespace.reserved] + crate::reserved::check_site_volume_name(¶ms.name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; + let kind = match params.kind.as_str() { "managed" => SiteVolumeKind::Managed, "bind" => { @@ -517,6 +525,10 @@ pub(crate) fn snapshot_site_volume( ) -> HandlerResult { use crate::runtime::site_volumes::{SiteVolumeDef, SiteVolumeKind}; + // r[impl namespace.reserved] + crate::reserved::check_site_volume_name(¶ms.name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; + let (source, source_path) = parse_source_vol_id(¶ms.source, state) .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e))?; @@ -576,6 +588,10 @@ pub(crate) fn promote_site_volume( ) -> HandlerResult { use crate::runtime::site_volumes::{SiteVolumeDef, SiteVolumeKind}; + // r[impl namespace.reserved] + crate::reserved::check_site_volume_name(¶ms.name) + .map_err(|e| OiError::new(ErrorCode::RequirementsInvalid, e.to_string()))?; + let source_name = params.source.clone(); let source_def = state .db diff --git a/crates/core/src/oi/handler/volumes/tests.rs b/crates/core/src/oi/handler/volumes/tests.rs index feb20d79..1fa31292 100644 --- a/crates/core/src/oi/handler/volumes/tests.rs +++ b/crates/core/src/oi/handler/volumes/tests.rs @@ -415,3 +415,26 @@ fn declared_external_volumes_guard_unmap() { assert_eq!(code, "requirements_invalid"); assert!(msg.contains("declared by app"), "{msg}"); } + +// r[verify namespace.reserved] +// The daemon deletes every site volume under `backup-snap-` at startup, so an +// operator volume created under that prefix was destroyed on the next restart. +#[test] +fn reserved_prefix_is_rejected_at_creation() { + let oi = TestOi::new(); + let (code, message) = oi + .call( + "/volumes/site/create", + json!({ "name": "backup-snap-archive", "kind": "managed" }), + ) + .unwrap_err(); + assert_eq!(code, "requirements_invalid"); + assert!(message.contains("reserved"), "message: {message}"); + + // A name that merely contains the prefix is the operator's. + oi.call( + "/volumes/site/create", + json!({ "name": "my-backup-snap-thing", "kind": "managed" }), + ) + .expect("only the claimed prefix is reserved"); +} diff --git a/crates/core/src/reserved.rs b/crates/core/src/reserved.rs new file mode 100644 index 00000000..241c8784 --- /dev/null +++ b/crates/core/src/reserved.rs @@ -0,0 +1,109 @@ +//! Names Seedling grants itself inside namespaces operators also use. +//! +//! Site volumes and site ingresses are operator-facing namespaces, and the +//! daemon takes names in both: `backup-snap-*` for the transient snapshots a +//! backup run creates, and `tailscale` for the ingress the discovery provider +//! maintains. Both are then recognised later *by name*, and both have a +//! destructive consumer — startup deletes everything under the snapshot +//! prefix, and the provider disables whatever row holds the ingress name. +//! Nothing stopped an operator creating an object with either name first. +//! +//! Two halves are needed and neither suffices alone. Reservation at creation +//! stops new collisions but cannot repair one that already exists, and does +//! not protect against a future code path that forgets to ask. So the +//! destructive consumers also match on *recorded ownership*: the startup +//! sweep skips names present in `site_volumes`, and the Tailscale provider +//! acts only on rows whose source is its own discovery. Reservation makes +//! collisions impossible going forward; ownership checks make them harmless +//! regardless of history. +//! +//! The constants lived apart before — `backup_execution.rs` and +//! `tailscale.rs` knew nothing about each other or about the creation +//! handlers — which is exactly how the gap opened. + +use seedling_protocol::names::{SiteIngressName, SiteVolumeName}; + +/// Site-volume name prefixes the daemon claims. +/// +/// Startup deletes every site volume under these, so an operator volume that +/// happened to match was destroyed on the next daemon start. +pub const RESERVED_SITE_VOLUME_PREFIXES: &[&str] = + &[crate::runtime::backup_execution::SNAPSHOT_NAME_PREFIX]; + +/// Site-ingress names the daemon claims. +pub const RESERVED_SITE_INGRESS_NAMES: &[&str] = + &[crate::runtime::tailscale::TAILSCALE_INGRESS_NAME]; + +/// A name that belongs to the daemon. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ReservedName { + pub name: String, + pub reason: String, +} + +impl std::fmt::Display for ReservedName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{} is reserved: {}", self.name, self.reason) + } +} + +/// Reject a site-volume name the daemon claims. +/// +/// Creation only — never update or delete. An operator with a legacy +/// `backup-snap-*` volume must still be able to manage it out of existence. +// r[impl namespace.reserved] +pub fn check_site_volume_name(name: &SiteVolumeName) -> Result<(), ReservedName> { + for prefix in RESERVED_SITE_VOLUME_PREFIXES { + if name.as_str().starts_with(prefix) { + return Err(ReservedName { + name: name.as_str().to_owned(), + reason: format!( + "site volume names beginning {prefix:?} are used by backup runs and are \ + deleted at daemon startup" + ), + }); + } + } + Ok(()) +} + +/// Reject a site-ingress name the daemon claims. +/// +/// Creation only, for the same reason as above. +// r[impl namespace.reserved] +pub fn check_site_ingress_name(name: &SiteIngressName) -> Result<(), ReservedName> { + if RESERVED_SITE_INGRESS_NAMES.contains(&name.as_str()) { + return Err(ReservedName { + name: name.as_str().to_owned(), + reason: "this site ingress name is maintained by a discovery provider".to_owned(), + }); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + // r[verify namespace.reserved] + #[test] + fn snapshot_prefix_is_reserved() { + let reserved = SiteVolumeName::new("backup-snap-archive").unwrap(); + assert!(check_site_volume_name(&reserved).is_err()); + let ordinary = SiteVolumeName::new("archive").unwrap(); + assert!(check_site_volume_name(&ordinary).is_ok()); + // A name that merely contains the prefix is fine — only the claim on + // the front of the namespace is the daemon's. + let contains = SiteVolumeName::new("my-backup-snap-thing").unwrap(); + assert!(check_site_volume_name(&contains).is_ok()); + } + + // r[verify namespace.reserved] + #[test] + fn tailscale_ingress_name_is_reserved() { + let reserved = SiteIngressName::new("tailscale").unwrap(); + assert!(check_site_ingress_name(&reserved).is_err()); + let ordinary = SiteIngressName::new("tailscale-manual").unwrap(); + assert!(check_site_ingress_name(&ordinary).is_ok()); + } +} diff --git a/crates/core/src/runtime/history.rs b/crates/core/src/runtime/history.rs index da9ef2b0..afbb396c 100644 --- a/crates/core/src/runtime/history.rs +++ b/crates/core/src/runtime/history.rs @@ -78,6 +78,23 @@ pub fn find_instance(db: &Db, id: InstanceId) -> rusqlite::Result rusqlite::Result> { + let mut stmt = db + .conn + .prepare("SELECT display_name FROM resource_instances WHERE app = ?1")?; + let rows = stmt.query_map(params![app], |row| row.get::<_, String>(0))?; + rows.collect() +} + // r[impl identity.components] pub fn find_instances_for_group( db: &Db, diff --git a/crates/core/src/runtime/history/tests.rs b/crates/core/src/runtime/history/tests.rs index de6720a6..e756dabe 100644 --- a/crates/core/src/runtime/history/tests.rs +++ b/crates/core/src/runtime/history/tests.rs @@ -709,3 +709,36 @@ fn delete_instance_clears_observations_and_faults_atomically() { assert_eq!(obs_count, 0, "observations deleted"); assert_eq!(fault_count, 0, "faults deleted"); } + +// r[verify app.uninstall.scope] +// `seedling-{app}-` is not prefix-free: both app names and resource names may +// contain hyphens, so uninstalling `app` matched every unit belonging to a +// sibling called `app-db`, and the retry branch reset and stopped that +// healthy sibling's units every tick while the uninstall never completed. +#[test] +fn display_names_are_scoped_to_their_own_app() { + let db = Db::open_in_memory().expect("open"); + + let instance = |app: &str, name: &str| ResourceInstance { + id: InstanceId::generate(), + app: AppName::new(app).unwrap(), + kind: ResourceKind::Deployment, + name: Some(name.to_owned()), + variant: InstanceVariant::Singleton, + display_name: format!("{app}-{name}"), + }; + + let app = instance("app", "web"); + let sibling = instance("app-db", "web"); + insert_instance(&db, &app).expect("insert app"); + insert_instance(&db, &sibling).expect("insert sibling"); + + let names = display_names_for_app(&db, &AppName::new("app").unwrap()).expect("query"); + assert_eq!(names, vec!["app-web".to_owned()]); + // The sibling's unit is `seedling-app-db-web.service`, which starts with + // `seedling-app-` — the exact match is what excludes it. + assert!( + format!("seedling-{}.service", sibling.display_name).starts_with("seedling-app-"), + "precondition: the sibling really does match the old prefix" + ); +} diff --git a/crates/core/src/runtime/tailscale.rs b/crates/core/src/runtime/tailscale.rs index d07f7ca9..9d8e7947 100644 --- a/crates/core/src/runtime/tailscale.rs +++ b/crates/core/src/runtime/tailscale.rs @@ -27,7 +27,7 @@ pub const DEFAULT_SOCKET_PATH: &str = "/var/run/tailscale/tailscaled.sock"; /// Operator-visible name we use for the discovered Tailscale site ingress. /// Matches the provider name so listings read naturally. -const TAILSCALE_INGRESS_NAME: &str = "tailscale"; +pub(crate) const TAILSCALE_INGRESS_NAME: &str = "tailscale"; /// How often the provider polls tailscaled for the current identity. const DEFAULT_POLL_INTERVAL: Duration = Duration::from_secs(60); @@ -287,10 +287,24 @@ impl TailscaleProvider { fn mark_existing_stale(&self, stale: bool) { self.db.call(move |db| { let name = SiteIngressName::new_unchecked(TAILSCALE_INGRESS_NAME); - if let Ok(Some(_)) = site_ingresses::get(db, &name) - && let Err(e) = site_ingresses::set_stale(db, &name, stale) - { - warn!(error = %e, "tailscale: failed to update stale flag"); + // r[impl namespace.reserved] — act on the row this provider owns, + // not on whatever holds the name. A manual ingress an operator + // created before the name was reserved was disabled here on the + // provider's first bad poll, and never re-enabled. + match site_ingresses::get(db, &name) { + Ok(Some(def)) if !def.source.is_discovered() => { + warn!( + "tailscale: a manually-created site ingress holds the name \ + {TAILSCALE_INGRESS_NAME:?}; leaving it alone" + ); + } + Ok(Some(_)) => { + if let Err(e) = site_ingresses::set_stale(db, &name, stale) { + warn!(error = %e, "tailscale: failed to update stale flag"); + } + } + Ok(None) => {} + Err(e) => warn!(error = %e, "tailscale: failed to look up the discovered ingress"), } }); } @@ -388,6 +402,20 @@ fn upsert_discovered_row( // case, which matches the design intent: a node-id change is "the // host identity changed" and attachments shouldn't silently follow. let name = SiteIngressName::new_unchecked(TAILSCALE_INGRESS_NAME); + // r[impl namespace.reserved] — a manual row under this name is the + // operator's. Falling through to `create` collided on the primary key + // every poll, so the provider livelocked and the operator's row stayed + // stale forever. + if let Some(existing) = site_ingresses::get(db, &name)? + && !existing.source.is_discovered() + { + warn!( + "tailscale: a manually-created site ingress holds the name \ + {TAILSCALE_INGRESS_NAME:?}; not creating the discovered one. Rename or remove \ + it to let discovery take over." + ); + return Ok(()); + } if let Some(existing) = site_ingresses::get(db, &name)? && existing.source.is_discovered() { diff --git a/crates/core/src/system/reconcile.rs b/crates/core/src/system/reconcile.rs index 6820f80d..ed7f6669 100644 --- a/crates/core/src/system/reconcile.rs +++ b/crates/core/src/system/reconcile.rs @@ -1445,8 +1445,38 @@ impl Reconciler { if !running.is_empty() { continue; } - let unit_prefix = format!("seedling-{}-", app.name); - match self.driver.process.list_units(&unit_prefix).await { + // r[impl app.uninstall.scope] — match units against the + // identities the registry recorded, not against a name prefix. + // `seedling-{app}-` is not prefix-free: uninstalling `app` matched + // every unit of a sibling called `app-db`, and the retry branch + // then reset and stopped that healthy sibling's units every five + // seconds while this uninstall never completed. + let app_name_for_query = app.name.clone(); + let expected: HashSet = self + .db + .call(move |db| { + crate::runtime::history::display_names_for_app(db, &app_name_for_query) + }) + .unwrap_or_else(|e| { + warn!(app = %app.name, "uninstall: failed to load instance names: {e}"); + Vec::new() + }) + .into_iter() + .map(|display_name| format!("seedling-{display_name}.service")) + .collect(); + // The prefix scan only enumerates candidates; the decision is the + // exact match against `expected`. + match self + .driver + .process + .list_units("seedling-") + .await + .map(|units| { + units + .into_iter() + .filter(|u| expected.contains(&u.name)) + .collect::>() + }) { Ok(units) if units.is_empty() => { let app_name_owned = app.name.clone(); let phase_handle = Arc::clone(&app.phase_handle); diff --git a/crates/core/src/system/translate/proxy.rs b/crates/core/src/system/translate/proxy.rs index 099f00d1..b274a59c 100644 --- a/crates/core/src/system/translate/proxy.rs +++ b/crates/core/src/system/translate/proxy.rs @@ -4,6 +4,7 @@ use std::{ }; use ipnet::Ipv6Net; +use sha2::{Digest, Sha256}; use crate::{ defs::ingress::IngressDef, @@ -77,12 +78,47 @@ pub fn instance_ipv6(node_prefix: &Ipv6Net, instance: &ResourceInstance) -> Ipv6 /// Derives the pod network /64 prefix for a pod instance. /// -/// Prefix layout: `fd5e:edXX:XXXX:KKUU::/64` — identical to `instance_ipv6` -/// but with the interface ID (bytes 8–15) zeroed. +/// Prefix layout: `fd5e:edXX:XXXX:KKSS::/64`, where `KK` is the resource kind +/// and `SS` is derived from the instance's **full** identity. +/// +/// `SS` used to be `uuid[0]`, which is *zero* for static Jobs — their +/// `InstanceId` is nil. Every static Job on the node, in every app, therefore +/// derived the identical /64, and netavark rejects the second network on a +/// duplicate subnet. Hashing the full identity (app, kind, name, whole UUID) +/// removes that deterministic collision: two distinct Jobs now differ even +/// when both their UUIDs are nil. +/// +/// It does not fix the *probabilistic* half. `SS` is still eight bits, so +/// scaled replicas of one deployment birthday-collide at the same rate as +/// before. Widening it means taking byte 6 as well, and byte 6 is the kind +/// discriminant that keeps pod /64s disjoint from the service /128 space and +/// from the `fffe` infrastructure addresses — not something to overload +/// without deciding what those namespaces mean. The real fix is allocation +/// rather than derivation: an `instance_id → subnet id` table with a unique +/// index, allocated at first actuation and freed at GC, which needs a +/// database handle in what is currently a pure translate layer. Left as the +/// next step rather than half-done. +/// +/// Changing the derivation re-homes each instance at its next pod recreation, +/// when its per-instance network is torn down and remade; no flag day. +// r[impl infra.pod.subnet] pub fn pod_network_prefix(node_prefix: &Ipv6Net, instance: &ResourceInstance) -> Ipv6Net { - let addr = instance_ipv6(node_prefix, instance); - let mut bytes = addr.octets(); - bytes[8..].fill(0); + debug_assert_eq!(node_prefix.prefix_len(), 48, "node prefix must be /48"); + + let mut hasher = Sha256::new(); + hasher.update(instance.app.as_str().as_bytes()); + hasher.update([0]); + hasher.update([instance.kind as u8]); + hasher.update([0]); + hasher.update(instance.name.as_deref().unwrap_or("").as_bytes()); + hasher.update([0]); + hasher.update(instance.id.0.as_bytes()); + let digest = hasher.finalize(); + + let mut bytes = [0u8; 16]; + bytes[..6].copy_from_slice(&node_prefix.network().octets()[..6]); + bytes[6] = instance.kind as u8; + bytes[7] = digest[0]; Ipv6Net::new(Ipv6Addr::from(bytes), 64).expect("64 is a valid IPv6 prefix length") } @@ -309,14 +345,45 @@ mod tests { assert_eq!(&octets[8..], &[0u8; 8]); } + // r[verify infra.pod.subnet] + // The pod /64 no longer shares its low byte with the instance address — + // that byte is a hash of the full identity now — but it keeps the kind + // discriminant, which is what holds pod /64s disjoint from the service + // /128 space and the `fffe` infrastructure addresses. #[test] - fn pod_prefix_matches_instance_address_upper_64() { + fn pod_prefix_keeps_the_kind_discriminant() { let prefix = test_prefix(); let instance = make_instance(ResourceKind::Job); - let addr = instance_ipv6(&prefix, &instance); let net = pod_network_prefix(&prefix, &instance); - // The /64 network address must match the first 8 bytes of the instance address - assert_eq!(&addr.octets()[..8], &net.network().octets()[..8]); + let addr = instance_ipv6(&prefix, &instance); + assert_eq!(net.network().octets()[6], ResourceKind::Job as u8); + assert_eq!(&addr.octets()[..7], &net.network().octets()[..7]); + } + + // r[verify infra.pod.subnet] + // Static Jobs carry a nil InstanceId, so deriving the subnet from + // `uuid[0]` gave every static Job on the node — across every app — the + // same /64, and netavark refuses the second network on a duplicate + // subnet. + #[test] + fn static_jobs_with_nil_ids_get_distinct_subnets() { + let prefix = test_prefix(); + let nil = crate::runtime::identity::InstanceId(uuid::Uuid::nil()); + let job = |app: &str, name: &str| ResourceInstance { + id: nil, + app: seedling_protocol::names::AppName::new(app).unwrap(), + kind: ResourceKind::Job, + name: Some(name.to_owned()), + variant: crate::runtime::identity::InstanceVariant::Singleton, + display_name: format!("{app}-job-{name}"), + }; + + let a = pod_network_prefix(&prefix, &job("alpha", "migrate")); + let b = pod_network_prefix(&prefix, &job("beta", "migrate")); + let c = pod_network_prefix(&prefix, &job("alpha", "vacuum")); + + assert_ne!(a, b, "same job name in different apps must not collide"); + assert_ne!(a, c, "different jobs in one app must not collide"); } #[test] diff --git a/crates/daemon/src/main.rs b/crates/daemon/src/main.rs index 7d18a846..7f05718f 100644 --- a/crates/daemon/src/main.rs +++ b/crates/daemon/src/main.rs @@ -616,10 +616,46 @@ async fn main() { let prefix = seedling_core::runtime::backup_execution::SNAPSHOT_NAME_PREFIX; match driver_ref.volume_store.list_sites_with_prefix(prefix) { Ok(orphans) if !orphans.is_empty() => { - tracing::warn!( - count = orphans.len(), - "found orphaned backup-execution snapshots from a previous run; removing" - ); + // r[impl namespace.reserved] — a DB row is the record that an + // operator owns this name. Creation rejects the prefix now, + // but that cannot help a volume created before the check + // existed, and this sweep deletes on sight. + let registered: Vec = + orphans.iter().map(|n| n.as_str().to_owned()).collect(); + let owned: std::collections::HashSet = db.call(move |conn| { + registered + .into_iter() + .filter(|name| { + seedling_protocol::names::SiteVolumeName::new(name) + .ok() + .and_then(|n| { + seedling_core::runtime::site_volumes::get(conn, &n).ok() + }) + .flatten() + .is_some() + }) + .collect() + }); + let orphans: Vec<_> = orphans + .into_iter() + .filter(|name| { + let keep = owned.contains(name.as_str()); + if keep { + tracing::info!( + volume = %name, + "a registered site volume uses the backup snapshot prefix; \ + leaving it alone" + ); + } + !keep + }) + .collect(); + if !orphans.is_empty() { + tracing::warn!( + count = orphans.len(), + "found orphaned backup-execution snapshots from a previous run; removing" + ); + } for name in &orphans { tracing::info!(snapshot = %name, "removing orphaned backup snapshot"); tokio::task::block_in_place(|| { diff --git a/docs/spec/runtime.md b/docs/spec/runtime.md index ebe85c8e..d51d69e8 100644 --- a/docs/spec/runtime.md +++ b/docs/spec/runtime.md @@ -1115,6 +1115,19 @@ Some internal operations (for example [backup.list](#r--backup.list), [backup.re > When a Service's routing pool contains only unhealthy backends (i.e. the prefer-healthy rule has fallen back to "anything running" per [lifecycle.service.routing-pool](#r--lifecycle.service.routing-pool)), the runtime must file a fault of kind `service_degraded` associated with that Service. > The fault is cleared automatically when at least one backend in the pool becomes healthy or when the Service is unscheduled. +> r[namespace.reserved] +> The runtime grants itself names inside namespaces operators also use: site-volume names beginning `backup-snap-`, and the site-ingress name `tailscale`. +> Creating a site volume or site ingress under a reserved name is rejected; the restriction applies to creation only, so an object that predates the reservation can still be renamed or removed. +> Reservation alone is not sufficient: a component that deletes or disables objects it believes are its own must identify them by what the runtime recorded — a registered site volume is never deleted by the snapshot sweep, and a manually created site ingress is never disabled or replaced by a discovery provider — regardless of what the object is named. + +> r[app.uninstall.scope] +> Uninstalling an app affects only resources belonging to that app. +> Resources are identified by the identities the runtime recorded for them, not by the shape of their names: names are not required to be prefix-free, so one app's name may be a prefix of another's. +> A scan by name prefix is permitted only to enumerate candidates that are then matched exactly against recorded identity. + +> r[infra.pod.subnet] +> Every concurrently running pod instance has a distinct network prefix. + > r[fault.surfacing] > Faults must be surfaced to operators through the operator interface (defined in a separate spec). > The runtime must not silently discard faults. From 464310b7ba5d867fdd7de647fcab836d42736a9f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 01:41:02 +0000 Subject: [PATCH 2/5] fix: fail safe when identity or ownership cannot be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review catches, all the same shape: treating "could not determine" as a definite answer. Uninstall built its expected unit set from the registry and, on a DB error, got an empty set — which filters every unit away and reads as "teardown finished", deleting the resource_instances rows while the units are still loaded. It now skips the app for the tick. The startup snapshot sweep treated a failed ownership lookup as "not owned" and deleted the volume. A destructive sweep has to be conservative. And r[infra.pod.subnet] claimed a guarantee the implementation does not provide. It now states the requirement and says plainly that derivation does not satisfy it — only allocation does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv --- crates/core/src/system/reconcile.rs | 32 ++++++++++++++++++----------- crates/daemon/src/main.rs | 24 +++++++++++++++------- docs/spec/runtime.md | 4 +++- 3 files changed, 40 insertions(+), 20 deletions(-) diff --git a/crates/core/src/system/reconcile.rs b/crates/core/src/system/reconcile.rs index ed7f6669..3de92fcc 100644 --- a/crates/core/src/system/reconcile.rs +++ b/crates/core/src/system/reconcile.rs @@ -1452,18 +1452,26 @@ impl Reconciler { // then reset and stopped that healthy sibling's units every five // seconds while this uninstall never completed. let app_name_for_query = app.name.clone(); - let expected: HashSet = self - .db - .call(move |db| { - crate::runtime::history::display_names_for_app(db, &app_name_for_query) - }) - .unwrap_or_else(|e| { - warn!(app = %app.name, "uninstall: failed to load instance names: {e}"); - Vec::new() - }) - .into_iter() - .map(|display_name| format!("seedling-{display_name}.service")) - .collect(); + let expected: HashSet = match self.db.call(move |db| { + crate::runtime::history::display_names_for_app(db, &app_name_for_query) + }) { + Ok(names) => names + .into_iter() + .map(|display_name| format!("seedling-{display_name}.service")) + .collect(), + // Fail safe: an empty expected set makes every unit invisible, + // which would read as "teardown finished" and delete the + // registry rows while the units are still loaded. Wait for a + // tick where the identities can actually be read. + Err(e) => { + warn!( + app = %app.name, + "uninstall: could not load recorded instance names; \ + not advancing teardown this tick: {e}" + ); + continue; + } + }; // The prefix scan only enumerates candidates; the decision is the // exact match against `expected`. match self diff --git a/crates/daemon/src/main.rs b/crates/daemon/src/main.rs index 7f05718f..aa2eb74b 100644 --- a/crates/daemon/src/main.rs +++ b/crates/daemon/src/main.rs @@ -626,13 +626,23 @@ async fn main() { registered .into_iter() .filter(|name| { - seedling_protocol::names::SiteVolumeName::new(name) - .ok() - .and_then(|n| { - seedling_core::runtime::site_volumes::get(conn, &n).ok() - }) - .flatten() - .is_some() + let Ok(parsed) = seedling_protocol::names::SiteVolumeName::new(name) + else { + return false; + }; + match seedling_core::runtime::site_volumes::get(conn, &parsed) { + Ok(row) => row.is_some(), + // A destructive sweep must be conservative: + // "we could not tell" is not "not owned". + Err(e) => { + tracing::warn!( + volume = %name, + "could not determine ownership of a snapshot-prefixed \ + site volume; keeping it: {e}" + ); + true + } + } }) .collect() }); diff --git a/docs/spec/runtime.md b/docs/spec/runtime.md index d51d69e8..8ef151af 100644 --- a/docs/spec/runtime.md +++ b/docs/spec/runtime.md @@ -1126,7 +1126,9 @@ Some internal operations (for example [backup.list](#r--backup.list), [backup.re > A scan by name prefix is permitted only to enumerate candidates that are then matched exactly against recorded identity. > r[infra.pod.subnet] -> Every concurrently running pod instance has a distinct network prefix. +> No two concurrently running pod instances may share a network prefix. +> A prefix must therefore not be derived from a part of an instance's identity that distinct instances can share: in particular, instances carrying no unique identifier of their own must still be distinguished by the rest of their identity. +> Deriving a prefix, however widely, does not on its own satisfy this requirement — only allocating prefixes and recording the allocation makes it a guarantee rather than a probability. > r[fault.surfacing] > Faults must be surfaced to operators through the operator interface (defined in a separate spec). From e51f75be2d36e9ef51f46dddff71e995ff5e5f72 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 01:49:39 +0000 Subject: [PATCH 3/5] fix: keep an unparseable snapshot-prefixed name, and query the row once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more review catches. A name that fails SiteVolumeName validation cannot be a snapshot Seedling created, so it is foreign rather than orphaned — the sweep keeps and logs it instead of deleting it. And upsert_discovered_row read the same row twice back to back; one read, then branch. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv --- crates/core/src/runtime/tailscale.rs | 22 +++++++++------------- crates/daemon/src/main.rs | 16 +++++++++++++--- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/crates/core/src/runtime/tailscale.rs b/crates/core/src/runtime/tailscale.rs index 9d8e7947..8d30c641 100644 --- a/crates/core/src/runtime/tailscale.rs +++ b/crates/core/src/runtime/tailscale.rs @@ -406,19 +406,15 @@ fn upsert_discovered_row( // operator's. Falling through to `create` collided on the primary key // every poll, so the provider livelocked and the operator's row stayed // stale forever. - if let Some(existing) = site_ingresses::get(db, &name)? - && !existing.source.is_discovered() - { - warn!( - "tailscale: a manually-created site ingress holds the name \ - {TAILSCALE_INGRESS_NAME:?}; not creating the discovered one. Rename or remove \ - it to let discovery take over." - ); - return Ok(()); - } - if let Some(existing) = site_ingresses::get(db, &name)? - && existing.source.is_discovered() - { + if let Some(existing) = site_ingresses::get(db, &name)? { + if !existing.source.is_discovered() { + warn!( + "tailscale: a manually-created site ingress holds the name \ + {TAILSCALE_INGRESS_NAME:?}; not creating the discovered one. Rename or remove \ + it to let discovery take over." + ); + return Ok(()); + } site_ingresses::delete(db, &name)?; info!( stale_node_id = ?match &existing.source { diff --git a/crates/daemon/src/main.rs b/crates/daemon/src/main.rs index aa2eb74b..ddcbfda0 100644 --- a/crates/daemon/src/main.rs +++ b/crates/daemon/src/main.rs @@ -626,9 +626,19 @@ async fn main() { registered .into_iter() .filter(|name| { - let Ok(parsed) = seedling_protocol::names::SiteVolumeName::new(name) - else { - return false; + let parsed = match seedling_protocol::names::SiteVolumeName::new(name) { + Ok(parsed) => parsed, + // Also a "could not tell": a name Seedling + // could not have created is not a snapshot of + // ours to delete. + Err(e) => { + tracing::warn!( + volume = %name, + "a snapshot-prefixed path is not a valid site volume \ + name; keeping it: {e}" + ); + return true; + } }; match seedling_core::runtime::site_volumes::get(conn, &parsed) { Ok(row) => row.is_some(), From e22a89120085a7fb969b0d5054951a50598fdb5b Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 03:09:44 +0000 Subject: [PATCH 4/5] fix(daemon): name why a snapshot-prefixed volume was kept MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep collapsed three answers into one bool — the operator registered this name, the name is not one we could have created, and the ownership lookup failed — and then logged every one of them as "a registered site volume". Keeping the volume is right in all three cases; reporting a "could not tell" as a definite ownership answer is not, which is the class this branch is about. Also SELECT DISTINCT the display names: an app that reuses a resource name over time has one row per instance id, and every caller de-duplicates. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv --- crates/core/src/runtime/history.rs | 2 +- crates/daemon/src/main.rs | 79 ++++++++++++++++++------------ 2 files changed, 49 insertions(+), 32 deletions(-) diff --git a/crates/core/src/runtime/history.rs b/crates/core/src/runtime/history.rs index afbb396c..37ee2ce1 100644 --- a/crates/core/src/runtime/history.rs +++ b/crates/core/src/runtime/history.rs @@ -90,7 +90,7 @@ pub fn find_instance(db: &Db, id: InstanceId) -> rusqlite::Result rusqlite::Result> { let mut stmt = db .conn - .prepare("SELECT display_name FROM resource_instances WHERE app = ?1")?; + .prepare("SELECT DISTINCT display_name FROM resource_instances WHERE app = ?1")?; let rows = stmt.query_map(params![app], |row| row.get::<_, String>(0))?; rows.collect() } diff --git a/crates/daemon/src/main.rs b/crates/daemon/src/main.rs index f3be1e57..f8cb0cb6 100644 --- a/crates/daemon/src/main.rs +++ b/crates/daemon/src/main.rs @@ -620,54 +620,71 @@ async fn main() { // operator owns this name. Creation rejects the prefix now, // but that cannot help a volume created before the check // existed, and this sweep deletes on sight. - let registered: Vec = + /// Why a snapshot-prefixed volume is not an orphan to remove. + /// A definite "the operator owns this" and a "we could not + /// tell" both keep the volume, but they are not the same + /// answer and must not be reported as one. + enum Keep { + Registered, + UnparseableName(String), + OwnershipUnknown(String), + } + + let candidates: Vec = orphans.iter().map(|n| n.as_str().to_owned()).collect(); - let owned: std::collections::HashSet = db.call(move |conn| { - registered + let keeps: std::collections::HashMap = db.call(move |conn| { + candidates .into_iter() - .filter(|name| { - let parsed = match seedling_protocol::names::SiteVolumeName::new(name) { - Ok(parsed) => parsed, + .filter_map(|name| { + let reason = match seedling_protocol::names::SiteVolumeName::new(&name) + { + Ok(parsed) => { + match seedling_core::runtime::site_volumes::get(conn, &parsed) { + Ok(Some(_)) => Keep::Registered, + Ok(None) => return None, + // A destructive sweep must be + // conservative: "we could not tell" is + // not "not owned". + Err(e) => Keep::OwnershipUnknown(e.to_string()), + } + } // Also a "could not tell": a name Seedling // could not have created is not a snapshot of // ours to delete. - Err(e) => { - tracing::warn!( - volume = %name, - "a snapshot-prefixed path is not a valid site volume \ - name; keeping it: {e}" - ); - return true; - } + Err(e) => Keep::UnparseableName(e.to_string()), }; - match seedling_core::runtime::site_volumes::get(conn, &parsed) { - Ok(row) => row.is_some(), - // A destructive sweep must be conservative: - // "we could not tell" is not "not owned". - Err(e) => { - tracing::warn!( - volume = %name, - "could not determine ownership of a snapshot-prefixed \ - site volume; keeping it: {e}" - ); - true - } - } + Some((name, reason)) }) .collect() }); let orphans: Vec<_> = orphans .into_iter() - .filter(|name| { - let keep = owned.contains(name.as_str()); - if keep { + .filter(|name| match keeps.get(name.as_str()) { + None => true, + Some(Keep::Registered) => { tracing::info!( volume = %name, "a registered site volume uses the backup snapshot prefix; \ leaving it alone" ); + false + } + Some(Keep::UnparseableName(e)) => { + tracing::warn!( + volume = %name, + "a snapshot-prefixed path is not a valid site volume name, so \ + it is not a snapshot of ours; leaving it alone: {e}" + ); + false + } + Some(Keep::OwnershipUnknown(e)) => { + tracing::warn!( + volume = %name, + "could not determine ownership of a snapshot-prefixed site \ + volume; leaving it alone: {e}" + ); + false } - !keep }) .collect(); if !orphans.is_empty() { From bf66abff8048e10f43527ab94b4f7443710c502f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 03:23:02 +0000 Subject: [PATCH 5/5] fix(uninstall): no recorded identities is not proof teardown finished MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error branch was guarded but the empty one was not, and an empty expected set filters every unit away — which the branch below reads as "teardown finished", deleting the registry rows while the units are still loaded. Empty is the truth for an app registered but never scheduled, and it is equally what a GC sweep leaves behind, so it cannot be read as either on its own. Corroborate it against the units actually loaded. The prefix scan stays a candidate enumeration and never a decision: it may over-match a sibling, which only withholds a completion. Where the two disagree, file a fault that clears when teardown does complete. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv --- crates/core/src/system/reconcile.rs | 124 +++++++++++++++++++++++++++- docs/spec/runtime.md | 1 + 2 files changed, 124 insertions(+), 1 deletion(-) diff --git a/crates/core/src/system/reconcile.rs b/crates/core/src/system/reconcile.rs index 1bb021a2..b0017c1e 100644 --- a/crates/core/src/system/reconcile.rs +++ b/crates/core/src/system/reconcile.rs @@ -28,7 +28,12 @@ use crate::{ stopped, }, system::{ - System, actuator::Actuator, caddy, observer::Observer, resolver, types::DataPlaneRules, + System, + actuator::Actuator, + caddy, + observer::Observer, + resolver, + types::{DataPlaneRules, UnitSummary}, }, }; @@ -442,6 +447,21 @@ pub struct Reconciler { site_resolver: Option>, } +/// Units that could belong to `app`, judged only by the shape of their names. +/// +/// This is a candidate enumeration and never a decision — `seedling-{app}-` is +/// not prefix-free, so `app-db`'s units land here too. It exists for the one +/// question the recorded identities cannot answer when there are none of them: +/// is there anything at all that might still be this app's? +// r[impl app.uninstall.scope] +fn units_possibly_of<'a>(units: &'a [UnitSummary], app: &AppName) -> Vec<&'a UnitSummary> { + let prefix = format!("seedling-{app}-"); + units + .iter() + .filter(|u| u.name.starts_with(&prefix)) + .collect() +} + impl Reconciler { #[expect( clippy::too_many_arguments, @@ -1620,6 +1640,52 @@ impl Reconciler { continue; } }; + // An empty recorded set is ambiguous in exactly the way this + // change is about. It is the truth for an app that was registered + // but never scheduled, and it is also what a GC sweep leaves + // behind if it reaps the rows while units are still loaded — and + // an empty `expected` filters every unit away, which the branch + // below reads as "teardown finished". Distinguish the two by + // asking whether anything that could be ours is still loaded. + // `seedling-{app}-` is not prefix-free, so it may over-match a + // sibling; over-matching only makes this refuse to conclude, + // which is the safe direction. + if expected.is_empty() { + match self.driver.process.list_units("seedling-").await { + Ok(units) if !units_possibly_of(&units, &app.name).is_empty() => { + tracing::error!( + app = %app.name, + "uninstall: units may still be loaded for this app but no recorded \ + identities remain to match them against; not advancing teardown" + ); + let app_name_owned = app.name.clone(); + self.db.call(move |db| { + let _ = crate::runtime::faults::file_once( + db, + &crate::runtime::faults::FaultKey::new( + &app_name_owned, + "uninstall_unidentifiable_units", + "", + ), + &crate::runtime::faults::FaultMeta::default(), + "units remain under this app's prefix but no recorded instance \ + identities remain to match them against, so teardown cannot be \ + safely completed", + ); + }); + continue; + } + Ok(_) => {} + Err(e) => { + warn!( + app = %app.name, + "uninstall: could not list units to corroborate an empty recorded \ + set; not advancing teardown this tick: {e}" + ); + continue; + } + } + } // The prefix scan only enumerates candidates; the decision is the // exact match against `expected`. match self @@ -1650,6 +1716,14 @@ impl Reconciler { ) { warn!(app = %app_name_owned, "failed to clean up resource instances during uninstall: {e}"); } + // The other half of `uninstall_unidentifiable_units`: + // teardown finishing is the only thing that resolves + // it, and it must not outlive the app it describes. + let _ = crate::runtime::faults::clear_faults_by_kind( + db, + &app_name_owned, + "uninstall_unidentifiable_units", + ); }); // i[impl event.types] // Uninstall is reconciler-driven and therefore emits no @@ -1690,6 +1764,54 @@ impl Reconciler { #[cfg(test)] mod tests { + + fn unit(name: &str) -> UnitSummary { + UnitSummary { + name: name.to_owned(), + state: Default::default(), + } + } + + // r[verify app.uninstall.scope] + #[test] + fn no_recorded_identities_with_units_still_loaded_is_not_teardown_finished() { + let units = [ + unit("seedling-app-web.service"), + unit("seedling-other-web.service"), + ]; + let app = AppName::new_unchecked("app"); + assert!( + !units_possibly_of(&units, &app).is_empty(), + "with no recorded identities left, a loaded unit under the app's own prefix is the \ + only remaining evidence that teardown has not finished; treating the empty record \ + set as completion deletes the rows while the unit is still loaded" + ); + } + + // r[verify app.uninstall.scope] + #[test] + fn no_recorded_identities_and_no_units_is_genuinely_nothing_to_tear_down() { + let units = [unit("seedling-other-web.service")]; + let app = AppName::new_unchecked("app"); + assert!( + units_possibly_of(&units, &app).is_empty(), + "an app registered but never scheduled has no rows and no units, and must still be \ + able to finish uninstalling" + ); + } + + // r[verify app.uninstall.scope] + #[test] + fn the_candidate_scan_may_over_match_a_sibling_but_only_towards_caution() { + let units = [unit("seedling-app-db-web.service")]; + let app = AppName::new_unchecked("app"); + assert!( + !units_possibly_of(&units, &app).is_empty(), + "`seedling-app-` also matches `app-db`; over-matching here only withholds a \ + completion, never stops a sibling's unit — the decision itself is still the exact \ + match against recorded identity" + ); + } use std::collections::HashMap; use crate::runtime::registry::{RegistryError, ScaledGroup}; diff --git a/docs/spec/runtime.md b/docs/spec/runtime.md index cacdb9c8..f6358bf2 100644 --- a/docs/spec/runtime.md +++ b/docs/spec/runtime.md @@ -1193,6 +1193,7 @@ Some internal operations (for example [backup.list](#r--backup.list), [backup.re > Uninstalling an app affects only resources belonging to that app. > Resources are identified by the identities the runtime recorded for them, not by the shape of their names: names are not required to be prefix-free, so one app's name may be a prefix of another's. > A scan by name prefix is permitted only to enumerate candidates that are then matched exactly against recorded identity. +> Having no recorded identities is not on its own evidence that teardown has finished: where the runtime cannot rule out that resources of the app are still present, it must not declare the app uninstalled, and must raise a fault that clears when teardown does complete. > r[infra.pod.subnet] > No two concurrently running pod instances may share a network prefix.