diff --git a/core/archipelago/src/container/boot_reconciler.rs b/core/archipelago/src/container/boot_reconciler.rs index 8dc72bcd..b90e777b 100644 --- a/core/archipelago/src/container/boot_reconciler.rs +++ b/core/archipelago/src/container/boot_reconciler.rs @@ -110,8 +110,39 @@ impl BootReconciler { Some(tokio::spawn(async move { let mut failure_rounds: u32 = 0; loop { - let installed = orchestrator.manifest_ids().await; + // `installed_app_ids`, NOT `manifest_ids`: a manifest exists + // on disk for every *available* app, so driving companion + // provisioning from it stood up a UI for apps nobody had + // installed and self-healed it forever (archi-dev-box ran + // archy-fedimint-ui and archy-lnd-ui with no fedimint and no + // lnd container present — the Guardian UI served its wait + // page with nothing behind it, reported as "fedimint + // installs but does not work"). `None` means the container + // listing failed: skip the whole stage rather than reap + // every companion on a transient probe error. + let Some(installed) = orchestrator.installed_app_ids().await else { + tracing::warn!( + "companion reconcile: cannot determine installed apps, skipping this pass" + ); + time::sleep(interval).await; + continue; + }; let failures = crate::container::companion::reconcile(&installed).await; + // Reap failures are logged but deliberately kept OUT of + // `failures`, which drives the backoff below. Repair keeps + // a companion available; reaping only tidies one away. A + // reap that fails persistently (a wedged systemctl, say) + // must not back the repair path off to its 1h ceiling and + // starve the thing that actually matters. + for (companion, err) in + crate::container::companion::reap_orphans(&installed).await + { + tracing::warn!( + companion = %companion, + error = %err, + "companion reap failed" + ); + } for (companion, err) in &failures { tracing::warn!( companion = %companion, diff --git a/core/archipelago/src/container/companion.rs b/core/archipelago/src/container/companion.rs index 018ee63d..60b2f148 100644 --- a/core/archipelago/src/container/companion.rs +++ b/core/archipelago/src/container/companion.rs @@ -47,6 +47,21 @@ const REPAIR_COOLDOWN: Duration = Duration::from_secs(600); static REPAIR_FAILED_AT: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); +/// A companion must look orphaned for this long before it is reaped. +/// +/// "Backend container absent" is not the same as "backend app uninstalled": +/// a Quadlet-managed app is briefly containerless while it restarts, and this +/// node runs `ARCHIPELAGO_USE_QUADLET_BACKENDS=true`. Reaping on the first +/// absent tick would take down a healthy companion mid-restart and reinstall +/// it on the next pass — an image pull or a 900s build in the worst case. +/// A real uninstall stays absent indefinitely, so waiting costs nothing. +const ORPHAN_GRACE: Duration = Duration::from_secs(300); + +/// First tick at which each companion was observed with no installed backend. +/// Cleared as soon as a backend reappears, so the grace period restarts. +static ORPHAN_SINCE: LazyLock>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + /// Static description of one companion. The full list per backend /// app_id lives in `companions_for`. #[derive(Debug, Clone)] @@ -86,6 +101,11 @@ pub fn companions_for(package_id: &str) -> &'static [CompanionSpec] { } } +/// Every companion this build knows how to provision. Kept beside +/// `companions_for` — a new companion must be added to both, or the reaper +/// will not recognise it as one of ours and will leave it running forever. +const ALL_COMPANIONS: &[&[CompanionSpec]] = &[BITCOIN_UI, LND_UI, ELECTRS_UI, FEDIMINT_UI]; + const BITCOIN_UI: &[CompanionSpec] = &[CompanionSpec { name: "archy-bitcoin-ui", image_base: "bitcoin-ui", @@ -615,6 +635,133 @@ pub async fn reconcile(installed_apps: &[String]) -> Vec<(String, anyhow::Error) failures } +/// Companions this build knows about that no app in `installed_apps` claims. +/// +/// Pure set arithmetic, split out from `reap_orphans` so the "which ones go" +/// decision is testable without a systemd manager. A companion shared by +/// several backends (archy-bitcoin-ui serves both bitcoin-core and +/// bitcoin-knots) survives while ANY of its backends is installed. +fn orphan_companions(installed_apps: &[String]) -> Vec<&'static CompanionSpec> { + let expected: std::collections::HashSet<&str> = installed_apps + .iter() + .flat_map(|app_id| companions_for(app_id)) + .map(|spec| spec.name) + .collect(); + ALL_COMPANIONS + .iter() + .copied() + .flatten() + .filter(|spec| !expected.contains(spec.name)) + .collect() +} + +/// Narrow `orphans` to those that have been orphaned for at least +/// `ORPHAN_GRACE`, updating `since` in place. +/// +/// Split out of `reap_orphans` and given an explicit `now` so the grace +/// behaviour is testable without sleeping: it is the guard that stops a +/// restarting backend from costing its companion a teardown+reinstall. +fn due_after_grace( + orphans: Vec<&'static CompanionSpec>, + orphan_names: &std::collections::HashSet<&str>, + since: &mut HashMap<&'static str, Instant>, + now: Instant, +) -> Vec<&'static CompanionSpec> { + // A companion whose backend came back is no longer a candidate; drop its + // clock so a later disappearance waits out a fresh grace period rather + // than inheriting a stale one. + since.retain(|name, _| orphan_names.contains(name)); + orphans + .into_iter() + .filter(|spec| { + let first_seen = *since.entry(spec.name).or_insert(now); + now.duration_since(first_seen) >= ORPHAN_GRACE + }) + .collect() +} + +/// Stop and remove any companion whose backend app is not installed. +/// +/// The counterpart to `reconcile`, which can only ever *add*. Without this, +/// a companion outlives its backend permanently: `remove_for` fires only on +/// the explicit uninstall RPC path, so an install that fails after the +/// companion lands, a container removed by hand, or a node whose app was +/// never installed at all keeps a `Restart=always` unit alive forever. +/// +/// `installed_apps` MUST be the full installed set (see +/// `ProdOrchestrator::installed_app_ids`), never the narrow per-app list +/// `reconcile_companions_for` passes — reaping against a one-app list would +/// tear down every other companion on the node. It must also never be the +/// *manifest* list, which is every available app rather than every installed +/// one; that mistake is what left the orphans this function now clears. +/// +/// Callers must not invoke this when they could not determine what is +/// installed. "I could not look" and "nothing is installed" produce the same +/// empty vector but demand opposite behaviour, so the check belongs upstream +/// where the distinction still exists. +pub async fn reap_orphans(installed_apps: &[String]) -> Vec<(String, anyhow::Error)> { + if !user_systemd_available() { + return Vec::new(); + } + let orphans = orphan_companions(installed_apps); + + // Age the observation before acting on it. Anything whose backend is back + // has its clock cleared; anything still orphaned must have been so for a + // full ORPHAN_GRACE before it is touched. + let orphan_names: std::collections::HashSet<&str> = + orphans.iter().map(|spec| spec.name).collect(); + let due: Vec<&'static CompanionSpec> = { + let mut since = ORPHAN_SINCE.lock().unwrap(); + due_after_grace(orphans, &orphan_names, &mut since, Instant::now()) + }; + if due.is_empty() { + return Vec::new(); + } + + let dir = match quadlet::unit_dir().await { + Ok(d) => d, + Err(e) => { + warn!("companion reap: cannot resolve quadlet dir: {e:#}"); + return Vec::new(); + } + }; + + let mut failures = Vec::new(); + for spec in due { + // Only act on companions that are actually present, so a node that + // never had the app stays silent instead of logging every tick. + let unit_path = dir.join(format!("{}.container", spec.name)); + let unit_present = fs::try_exists(&unit_path).await.unwrap_or(false); + if !unit_present { + // No unit file, so the only reason to act is a service still + // running from a removed one. A hung `is-active` under IO pressure + // must read as "leave it alone" — reaping is destructive, so every + // uncertain signal resolves toward doing nothing. + let svc = format!("{}.service", spec.name); + match tokio::time::timeout(Duration::from_secs(10), quadlet::is_active(&svc)).await { + Ok(true) => {} + Ok(false) => continue, + Err(_) => { + warn!( + companion = spec.name, + "reap: is-active probe timed out; leaving it alone" + ); + continue; + } + } + } + info!( + companion = spec.name, + "reap: backend app is not installed, removing orphaned companion" + ); + if let Err(e) = quadlet::disable_remove(spec.name, &dir).await { + warn!(companion = spec.name, error = %e, "companion reap failed"); + failures.push((spec.name.to_string(), e)); + } + } + failures +} + /// Does this companion need install_one to be re-run? Returns true if /// the unit file is missing, stale, or the service is not active. /// @@ -675,6 +822,170 @@ async fn needs_repair(spec: &CompanionSpec) -> Result { mod tests { use super::*; + fn names(specs: &[&'static CompanionSpec]) -> Vec<&'static str> { + let mut v: Vec<_> = specs.iter().map(|s| s.name).collect(); + v.sort_unstable(); + v + } + + fn ids(list: &[&str]) -> Vec { + list.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn every_companion_in_companions_for_is_also_in_all_companions() { + // The reaper only recognises companions listed in ALL_COMPANIONS. One + // missing from it would be provisioned by `reconcile` and then never + // cleaned up — exactly the leak this module is fixing. + let backends = [ + "bitcoin", + "bitcoin-core", + "bitcoin-knots", + "lnd", + "electrumx", + "electrs", + "mempool-electrs", + "fedimint", + "fedimintd", + ]; + let known: std::collections::HashSet<&str> = ALL_COMPANIONS + .iter() + .copied() + .flatten() + .map(|s| s.name) + .collect(); + for backend in backends { + for spec in companions_for(backend) { + assert!( + known.contains(spec.name), + "{} is provisionable but not reapable — add it to ALL_COMPANIONS", + spec.name + ); + } + } + } + + #[test] + fn nothing_installed_orphans_every_companion() { + assert_eq!( + names(&orphan_companions(&[])), + vec![ + "archy-bitcoin-ui", + "archy-electrs-ui", + "archy-fedimint-ui", + "archy-lnd-ui" + ] + ); + } + + #[test] + fn an_installed_backend_protects_only_its_own_companion() { + // The archi-dev-box state that exposed the bug: bitcoin-knots and + // electrumx installed, fedimint and lnd not — yet all four companions + // were running because the reconciler was fed the manifest list. + let orphans = orphan_companions(&ids(&["bitcoin-knots", "electrumx"])); + assert_eq!(names(&orphans), vec!["archy-fedimint-ui", "archy-lnd-ui"]); + } + + #[test] + fn a_shared_companion_survives_on_any_one_of_its_backends() { + // archy-bitcoin-ui serves bitcoin-core AND bitcoin-knots. Installing + // either must keep it; a naive per-app reap would remove it while the + // other backend was still running. + for backend in ["bitcoin", "bitcoin-core", "bitcoin-knots"] { + let orphans = orphan_companions(&ids(&[backend])); + assert!( + !names(&orphans).contains(&"archy-bitcoin-ui"), + "archy-bitcoin-ui reaped while {backend} is installed" + ); + } + } + + #[test] + fn apps_without_companions_orphan_everything_and_panic_nothing() { + let orphans = orphan_companions(&ids(&["nextcloud", "not-a-real-app"])); + assert_eq!(orphans.len(), 4); + } + + #[test] + fn every_backend_installed_leaves_no_orphans() { + let orphans = orphan_companions(&ids(&[ + "bitcoin-knots", + "lnd", + "electrumx", + "fedimint", + ])); + assert!(names(&orphans).is_empty(), "unexpected orphans: {:?}", names(&orphans)); + } + + fn name_set(specs: &[&'static CompanionSpec]) -> std::collections::HashSet<&'static str> { + specs.iter().map(|s| s.name).collect() + } + + #[test] + fn a_freshly_orphaned_companion_is_not_reaped_immediately() { + let orphans = orphan_companions(&ids(&["bitcoin-knots"])); + let names_seen = name_set(&orphans); + let mut since = HashMap::new(); + let now = Instant::now(); + let due = due_after_grace(orphans, &names_seen, &mut since, now); + assert!( + due.is_empty(), + "reaped on the first observation: {:?}", + names(&due) + ); + } + + #[test] + fn an_orphan_past_the_grace_period_is_reaped() { + let orphans = orphan_companions(&ids(&["bitcoin-knots"])); + let names_seen = name_set(&orphans); + let mut since = HashMap::new(); + let start = Instant::now(); + // First pass records the clock and reaps nothing. + let due = due_after_grace(orphans.clone(), &names_seen, &mut since, start); + assert!(due.is_empty()); + // A pass after the grace window reaps. + let due = due_after_grace(orphans, &names_seen, &mut since, start + ORPHAN_GRACE); + assert_eq!(names(&due), vec!["archy-electrs-ui", "archy-fedimint-ui", "archy-lnd-ui"]); + } + + #[test] + fn a_backend_returning_mid_grace_resets_the_clock() { + // The restart window this guard exists for: lnd vanishes for a tick + // while its container is recreated, then comes back. Its companion + // must never be reaped, and a later real uninstall must wait out a + // full fresh grace period rather than inheriting the old clock. + let start = Instant::now(); + let mut since = HashMap::new(); + + let orphans = orphan_companions(&ids(&["bitcoin-knots"])); + let names_seen = name_set(&orphans); + assert!(due_after_grace(orphans, &names_seen, &mut since, start).is_empty()); + + // lnd is back — it is no longer an orphan candidate. + let orphans = orphan_companions(&ids(&["bitcoin-knots", "lnd"])); + let names_seen = name_set(&orphans); + let due = due_after_grace(orphans, &names_seen, &mut since, start + ORPHAN_GRACE); + assert!( + !names(&due).contains(&"archy-lnd-ui"), + "lnd companion reaped even though lnd came back" + ); + assert!(!since.contains_key("archy-lnd-ui"), "stale clock kept for lnd"); + + // lnd goes away for real. It must wait a fresh full grace period. + let orphans = orphan_companions(&ids(&["bitcoin-knots"])); + let names_seen = name_set(&orphans); + let t = start + ORPHAN_GRACE; + let due = due_after_grace(orphans.clone(), &names_seen, &mut since, t); + assert!( + !names(&due).contains(&"archy-lnd-ui"), + "lnd companion reaped without a fresh grace period" + ); + let due = due_after_grace(orphans, &names_seen, &mut since, t + ORPHAN_GRACE); + assert!(names(&due).contains(&"archy-lnd-ui")); + } + #[test] fn companions_for_known_apps_returns_expected_set() { assert_eq!(companions_for("bitcoin-knots").len(), 1); diff --git a/core/archipelago/src/container/prod_orchestrator.rs b/core/archipelago/src/container/prod_orchestrator.rs index 718dbbdc..f66fb1a9 100644 --- a/core/archipelago/src/container/prod_orchestrator.rs +++ b/core/archipelago/src/container/prod_orchestrator.rs @@ -1659,18 +1659,59 @@ impl ProdContainerOrchestrator { .then(|| members.iter().map(|s| s.to_string()).collect()) } - /// Snapshot of the app IDs currently in the in-memory manifest map. - /// Used by the boot reconciler to drive companion-unit reconciliation. - pub async fn manifest_ids(&self) -> Vec { - let user_stopped = crate::crash_recovery::load_user_stopped(&self.data_dir).await; + // `manifest_ids()` used to live here: every app id in the in-memory + // manifest map, i.e. every manifest the node can *see* (the whole `apps/` + // directory plus the signed-catalog overlay). Its only caller was the boot + // reconciler's companion stage, which is precisely the bug described below + // — "can see" was silently read as "has installed". It is deleted rather + // than left unused so nothing reaches for it again; `installed_app_ids` is + // the answer to the question callers actually mean. + + /// App ids whose container actually exists — the + /// `ReconcileMode::ExistingOnly` rule, made available to callers outside + /// `reconcile_all_with_mode`. + /// + /// The app reconciler has always drawn this line ("merely listing a catalog + /// manifest never installs an unqualified app"); the companion stage did + /// not, and fed itself `manifest_ids` instead. Because a manifest exists on + /// disk for every *available* app, that provisioned and perpetually + /// self-healed a companion UI for apps nobody had installed: archi-dev-box + /// ran `archy-fedimint-ui` and `archy-lnd-ui` with no `fedimint` and no + /// `lnd` container anywhere on the box (2026-08-08). The Guardian UI served + /// its "waiting for Bitcoin" page forever with nothing behind it, which is + /// what the operator reported as "fedimint guardian installs but does not + /// work" — there was nothing to install, the UI was already up. + /// + /// `None` means the runtime listing failed. That is deliberately distinct + /// from `Some(vec![])`: a caller that removes things on absence must not + /// treat "I could not look" as "nothing is installed". + /// + /// The presence of a container — in ANY state — is the whole test. It + /// deliberately does NOT carry over the `user_stopped` / `disabled` + /// filters the old `manifest_ids` applied: a stopped app is still an + /// installed app, its container still + /// exists (exited), and treating it as uninstalled would make stopping an + /// app tear its companion down and rebuild it on the next start. "Is it + /// installed" and "is it currently meant to be running" are different + /// questions, and only the first one belongs here. + pub async fn installed_app_ids(&self) -> Option> { + let present: std::collections::HashSet = self + .runtime + .list_containers() + .await + .ok()? + .into_iter() + .map(|c| c.name) + .collect(); let state = self.state.read().await; - state - .manifests - .keys() - .filter(|app_id| !state.disabled.contains(*app_id)) - .filter(|app_id| !user_stopped.contains(*app_id)) - .cloned() - .collect() + Some( + state + .manifests + .iter() + .filter(|(_, lm)| present.contains(&compute_container_name(&lm.manifest))) + .map(|(app_id, _)| app_id.clone()) + .collect(), + ) } /// Scan the runtime for containers whose names match one of our manifests.