From e57514b6906d208169fa0eb4cadcc010b9e17679 Mon Sep 17 00:00:00 2001 From: archipelago Date: Tue, 23 Jun 2026 15:23:16 -0400 Subject: [PATCH] fix(uninstall): never ghost a removed app in My Apps on cleanup residue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handle_package_uninstall lumped every teardown failure into one `errors` vec and returned Err on any of them BEFORE removing the package state entry — so a non-fatal cleanup hiccup (a slow/failed `sudo rm -rf` of a large data dir, a volume/network removal) left the app's containers gone but its entry in package_data → a ghost in My Apps, and the spawned task reverted it to Installed. Split the failures: container removal that even force-rm can't complete (app genuinely still present) keeps the entry + returns Err; everything after the containers are gone is best-effort. Remove the state entry as soon as the containers are gone — BEFORE the slow volume/data teardown — so My Apps updates immediately and residue can never ghost the app. set_uninstall_stage is a no-op once the entry is gone (if-let guard), so the later stages don't re-create it. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/api/rpc/package/runtime.rs | 115 ++++++++++++------ 1 file changed, 75 insertions(+), 40 deletions(-) diff --git a/core/archipelago/src/api/rpc/package/runtime.rs b/core/archipelago/src/api/rpc/package/runtime.rs index 9598f053..05d7b423 100644 --- a/core/archipelago/src/api/rpc/package/runtime.rs +++ b/core/archipelago/src/api/rpc/package/runtime.rs @@ -312,7 +312,16 @@ impl RpcHandler { let mut stopped = 0u32; let mut removed = 0u32; - let mut errors = Vec::new(); + // Two distinct failure classes, kept separate so they don't get + // conflated (the old single `errors` vec did, which caused the "ghost in + // My Apps" bug): `container_errors` means a container could NOT be + // removed (force-rm failed too) — the app is genuinely still present, so + // we keep its state entry and surface a hard error. `cleanup_errors` + // means volume/network/data-dir teardown left residue — the containers + // are already gone, so the app IS uninstalled and MUST disappear from My + // Apps; the residue is logged but never ghosts the app. + let mut container_errors: Vec = Vec::new(); + let mut cleanup_errors: Vec = Vec::new(); self.set_uninstall_stage( package_id, @@ -370,7 +379,7 @@ impl RpcHandler { let msg = format!("Failed to remove {}: {}; {}", name, stderr.trim(), e); tracing::error!("Uninstall {}: {}", package_id, msg); - errors.push(msg); + container_errors.push(msg); } } } @@ -379,12 +388,35 @@ impl RpcHandler { Err(force_err) => { let msg = format!("Failed to remove {}: {}; {}", name, e, force_err); tracing::error!("Uninstall {}: {}", package_id, msg); - errors.push(msg); + container_errors.push(msg); } }, } } + // A container that survived even force-remove means the app is NOT + // actually uninstalled — keep its state entry and fail so the spawned + // task reverts it to its prior state (and the user can retry), rather + // than orphaning a live container that's missing from My Apps. + if !container_errors.is_empty() { + tracing::error!( + "Uninstall {}: containers could not be removed: {:?}", + package_id, + container_errors + ); + return Err(anyhow::anyhow!( + "Uninstall {} failed: {}", + package_id, + container_errors.join("; ") + )); + } + + // Containers are gone → the app is uninstalled. Remove its state entry + // NOW, before the (possibly slow, possibly fallible) volume/data + // teardown below, so My Apps updates immediately and a residue failure + // can never leave a ghost. Reinstall/scan no longer see a stale entry. + self.remove_package_state_entry(package_id).await; + self.set_uninstall_stage(package_id, "Cleaning up volumes") .await; // Avoid global Podman volume prune on production nodes: store-wide @@ -432,70 +464,73 @@ impl RpcHandler { let stderr = String::from_utf8_lossy(&o.stderr); let msg = format!("Failed to remove data {}: {}", dir, stderr.trim()); tracing::error!("Uninstall {}: {}", package_id, msg); - errors.push(msg); + cleanup_errors.push(msg); } Err(e) => { let msg = format!("Failed to remove data {}: {}", dir, e); tracing::error!("Uninstall {}: {}", package_id, msg); - errors.push(msg); + cleanup_errors.push(msg); } _ => {} } } } - if !errors.is_empty() { + // The app is already gone from My Apps (entry removed above). Residual + // volume/data cleanup failures are logged but NEVER ghost the app — a + // reinstall and the next uninstall both tolerate leftover dirs. + if !cleanup_errors.is_empty() { tracing::error!( - "Uninstall {} completed with errors: {:?}", + "Uninstall {} removed but left cleanup residue: {:?}", package_id, - errors + cleanup_errors ); - return Err(anyhow::anyhow!( - "Uninstall {} partially failed: {}", - package_id, - errors.join("; ") - )); } tracing::info!( - "Uninstall {} complete: stopped={}, removed={}", + "Uninstall {} complete: stopped={}, removed={}, cleanup_errors={}", package_id, stopped, - removed + removed, + cleanup_errors.len() ); - // Immediately remove from in-memory state so the UI updates without - // waiting for the scanner's absence threshold (3 scans × 60s each). - { - let (mut data, _rev) = self.state_manager.get_snapshot().await; - let before = data.package_data.len(); - data.package_data.remove(package_id); - // Also remove any alias keys (e.g. "bitcoin-knots" vs "bitcoin") - let aliases: Vec = data - .package_data - .keys() - .filter(|k| { - super::config::all_container_names(package_id) - .iter() - .any(|c| c.strip_prefix("archy-").unwrap_or(c) == k.as_str()) - }) - .cloned() - .collect(); - for alias in &aliases { - data.package_data.remove(alias); - } - if data.package_data.len() < before { - self.state_manager.update_data(data).await; - } - } - Ok(serde_json::json!({ "status": "uninstalled", "stopped": stopped, "removed": removed, + "cleanup_warnings": cleanup_errors, })) } + /// Remove a package's entry (and any alias keys) from persisted state so it + /// disappears from My Apps immediately, without waiting for the scanner's + /// absence threshold (3 scans × 60s). Called as soon as an uninstall has + /// removed the app's containers — before the slower volume/data teardown — + /// so a residue failure can never leave a ghost entry behind. + async fn remove_package_state_entry(&self, package_id: &str) { + let (mut data, _rev) = self.state_manager.get_snapshot().await; + let before = data.package_data.len(); + data.package_data.remove(package_id); + // Also remove any alias keys (e.g. "bitcoin-knots" vs "bitcoin"). + let aliases: Vec = data + .package_data + .keys() + .filter(|k| { + super::config::all_container_names(package_id) + .iter() + .any(|c| c.strip_prefix("archy-").unwrap_or(c) == k.as_str()) + }) + .cloned() + .collect(); + for alias in &aliases { + data.package_data.remove(alias); + } + if data.package_data.len() < before { + self.state_manager.update_data(data).await; + } + } + /// Start a bundled app (create container from pre-loaded image if needed). pub(in crate::api::rpc) async fn handle_bundled_app_start( &self,