fix(orchestrator,content): bound repair-recreate loops; self-heal stale content catalog entries
- prod_orchestrator.rs: the boot reconciler's zombie-guard and start-failed recreate paths (Created/Stopped/Exited states) had no attempt cap, unlike health_monitor's independent restart tracker. A container whose entrypoint fatally crashes right after `podman start` succeeds got stop+remove+ install_fresh'd every ~30s reconcile tick forever (portainer on .198, 2026-07-01: a DB schema newer than the pinned binary could read -- no amount of recreating fixes that). Added a 5-attempts/30-minute circuit breaker; once exhausted the container is left alone with an error! log instead of looping, and an explicit install/start clears the counter. - content_server.rs: serve_content now prunes a catalog entry whose backing file is missing on disk, instead of leaving it advertised to every peer forever with no way to distinguish "gone" from "transient failure." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
archipelago
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
d414ae3daa
commit
d0710e7491
@@ -1025,6 +1025,10 @@ struct OrchestratorState {
|
||||
disabled: HashSet<String>,
|
||||
/// app_id → per-app mutex, created lazily the first time we touch an app
|
||||
locks: HashMap<String, Arc<Mutex<()>>>,
|
||||
/// container name → (attempt count, first-attempt time) for the
|
||||
/// stop+remove+install_fresh "repair" recreate paths below. See
|
||||
/// `should_attempt_repair`.
|
||||
repair_attempts: HashMap<String, (u32, std::time::Instant)>,
|
||||
}
|
||||
|
||||
impl OrchestratorState {
|
||||
@@ -1033,10 +1037,29 @@ impl OrchestratorState {
|
||||
manifests: HashMap::new(),
|
||||
disabled: HashSet::new(),
|
||||
locks: HashMap::new(),
|
||||
repair_attempts: HashMap::new(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Cap on how many times the boot reconciler will recreate the same
|
||||
/// container within `REPAIR_ATTEMPT_RESET_WINDOW` before giving up on it.
|
||||
///
|
||||
/// Without this, a container whose entrypoint process fatally exits right
|
||||
/// after `podman start` succeeds (podman itself reports no error — the crash
|
||||
/// happens inside the app a moment later) gets stop+remove+install_fresh'd
|
||||
/// again on every ~30s reconcile tick, forever. `health_monitor.rs`'s
|
||||
/// restart tracker already bounds ITS OWN independent restart path
|
||||
/// (`MAX_RESTART_ATTEMPTS`) and eventually surfaces a user-facing
|
||||
/// notification — but the boot reconciler's repair-recreate path had no
|
||||
/// equivalent circuit breaker, so the two could race indefinitely on the
|
||||
/// same fatally-broken container (portainer on `.198`, 2026-07-01: crashed
|
||||
/// on every start because its on-disk DB was written by a newer binary than
|
||||
/// the pinned image — a data/version mismatch no amount of recreating could
|
||||
/// fix, yet it kept looping every 30s until manually intervened on).
|
||||
const MAX_REPAIR_ATTEMPTS: u32 = 5;
|
||||
const REPAIR_ATTEMPT_RESET_WINDOW: std::time::Duration = std::time::Duration::from_secs(1800);
|
||||
|
||||
pub struct ProdContainerOrchestrator {
|
||||
runtime: Arc<dyn ContainerRuntimeTrait>,
|
||||
manifests_dir: PathBuf,
|
||||
@@ -1499,6 +1522,48 @@ impl ProdContainerOrchestrator {
|
||||
.await
|
||||
}
|
||||
|
||||
/// Whether the reconciler should attempt another stop+remove+install_fresh
|
||||
/// repair recreate for `name`, or has already tried too many times
|
||||
/// recently and should leave it alone instead of looping forever. See
|
||||
/// `MAX_REPAIR_ATTEMPTS`.
|
||||
async fn should_attempt_repair(&self, name: &str) -> bool {
|
||||
let mut state = self.state.write().await;
|
||||
let now = std::time::Instant::now();
|
||||
let entry = state
|
||||
.repair_attempts
|
||||
.entry(name.to_string())
|
||||
.or_insert((0, now));
|
||||
if now.duration_since(entry.1) > REPAIR_ATTEMPT_RESET_WINDOW {
|
||||
*entry = (0, now);
|
||||
}
|
||||
entry.0 += 1;
|
||||
if entry.0 > MAX_REPAIR_ATTEMPTS {
|
||||
tracing::error!(
|
||||
container = %name,
|
||||
attempts = entry.0,
|
||||
window_secs = REPAIR_ATTEMPT_RESET_WINDOW.as_secs(),
|
||||
"giving up on repairing container after too many recreate attempts — it keeps failing to \
|
||||
come up cleanly on its own; check `podman logs {name}` for the real cause (a data/version \
|
||||
mismatch or another fatal startup error is likely, not something recreating the container \
|
||||
again will fix). Leaving it as-is instead of recreating it forever; a manual fix + restart \
|
||||
(or a subsequent explicit install/start, which clears this counter) is needed to recover it.",
|
||||
name = name,
|
||||
);
|
||||
false
|
||||
} else {
|
||||
true
|
||||
}
|
||||
}
|
||||
|
||||
/// Clears the repair-attempt counter for `name` — call on any path that
|
||||
/// reaches a stable Running/NoOp/Started outcome, so a container that
|
||||
/// recovers (on its own, or after a real fix) doesn't inherit a stale
|
||||
/// near-exhausted counter from an earlier unrelated failure.
|
||||
async fn clear_repair_attempts(&self, name: &str) {
|
||||
let mut state = self.state.write().await;
|
||||
state.repair_attempts.remove(name);
|
||||
}
|
||||
|
||||
async fn ensure_running_with_mode(
|
||||
&self,
|
||||
lm: &LoadedManifest,
|
||||
@@ -1611,6 +1676,11 @@ impl ProdContainerOrchestrator {
|
||||
// "Up" → proxy 502 → NetBird login broke). Conservative:
|
||||
// only fires on a concrete dead PID, never on uncertainty.
|
||||
if !container_running_process_alive(&name).await {
|
||||
if !self.should_attempt_repair(&name).await {
|
||||
return Ok(ReconcileAction::Left(
|
||||
"repair-attempts-exhausted".into(),
|
||||
));
|
||||
}
|
||||
tracing::warn!(
|
||||
app_id = %app_id,
|
||||
container = %name,
|
||||
@@ -1718,6 +1788,7 @@ impl ProdContainerOrchestrator {
|
||||
return Ok(ReconcileAction::Installed);
|
||||
}
|
||||
}
|
||||
self.clear_repair_attempts(&name).await;
|
||||
Ok(ReconcileAction::NoOp)
|
||||
}
|
||||
ContainerState::Stopped | ContainerState::Exited => {
|
||||
@@ -1743,6 +1814,11 @@ impl ProdContainerOrchestrator {
|
||||
)
|
||||
.await
|
||||
{
|
||||
if !self.should_attempt_repair(&name).await {
|
||||
return Ok(ReconcileAction::Left(
|
||||
"repair-attempts-exhausted".into(),
|
||||
));
|
||||
}
|
||||
tracing::warn!(
|
||||
app_id = %app_id,
|
||||
container = %name,
|
||||
@@ -1764,6 +1840,7 @@ impl ProdContainerOrchestrator {
|
||||
wait_for_container_stable_running(self.runtime.as_ref(), &name, 15, 90)
|
||||
.await?;
|
||||
}
|
||||
self.clear_repair_attempts(&name).await;
|
||||
Ok(ReconcileAction::Started)
|
||||
}
|
||||
ContainerState::Stopping => {
|
||||
@@ -1792,6 +1869,11 @@ impl ProdContainerOrchestrator {
|
||||
)
|
||||
.await
|
||||
{
|
||||
if !self.should_attempt_repair(&name).await {
|
||||
return Ok(ReconcileAction::Left(
|
||||
"repair-attempts-exhausted".into(),
|
||||
));
|
||||
}
|
||||
tracing::warn!(
|
||||
app_id = %app_id,
|
||||
container = %name,
|
||||
@@ -1813,6 +1895,7 @@ impl ProdContainerOrchestrator {
|
||||
wait_for_container_stable_running(self.runtime.as_ref(), &name, 15, 90)
|
||||
.await?;
|
||||
}
|
||||
self.clear_repair_attempts(&name).await;
|
||||
Ok(ReconcileAction::Started)
|
||||
}
|
||||
ContainerState::Paused => Ok(ReconcileAction::Left("paused".to_string())),
|
||||
@@ -4594,6 +4677,85 @@ app:
|
||||
.any(|c| c == "create_container:bitcoin-knots:offset=0"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn repair_recreate_stops_after_max_attempts_instead_of_looping_forever() {
|
||||
// A container whose entrypoint fatally crashes every time (portainer
|
||||
// on .198, 2026-07-01: DB schema too new for the pinned binary) must
|
||||
// not be stop+remove+install_fresh'd forever by the boot reconciler.
|
||||
let rt = Arc::new(MockRuntime::default());
|
||||
rt.mark_image_present("docker.io/bitcoin/knots:28");
|
||||
rt.set_state("bitcoin-knots", ContainerState::Exited);
|
||||
let orch = orch_with(rt.clone()).await;
|
||||
orch.insert_manifest_for_test(
|
||||
pull_manifest("bitcoin-knots", "docker.io/bitcoin/knots:28"),
|
||||
PathBuf::from("/tmp/bk"),
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut last_report = None;
|
||||
for _ in 0..MAX_REPAIR_ATTEMPTS {
|
||||
// fail_start entries are consumed on use — re-arm every pass so
|
||||
// this container fails to start EVERY time, not just once.
|
||||
rt.fail_start
|
||||
.lock()
|
||||
.unwrap()
|
||||
.insert("bitcoin-knots".into(), "fatal startup error".into());
|
||||
rt.set_state("bitcoin-knots", ContainerState::Exited);
|
||||
let report = orch.reconcile_all().await;
|
||||
assert_eq!(
|
||||
report.actions,
|
||||
vec![("bitcoin-knots".to_string(), ReconcileAction::Installed)],
|
||||
"expected a recreate attempt within the attempt budget"
|
||||
);
|
||||
last_report = Some(report);
|
||||
}
|
||||
assert!(last_report.is_some());
|
||||
|
||||
// One more pass exceeds MAX_REPAIR_ATTEMPTS — the breaker must trip:
|
||||
// no further remove/create calls (it may still probe status/attempt
|
||||
// a start, same as any other pass — it just must not recreate).
|
||||
let remove_calls_before = rt
|
||||
.calls()
|
||||
.iter()
|
||||
.filter(|c| c.starts_with("remove_container:"))
|
||||
.count();
|
||||
let create_calls_before = rt
|
||||
.calls()
|
||||
.iter()
|
||||
.filter(|c| c.starts_with("create_container:"))
|
||||
.count();
|
||||
rt.fail_start
|
||||
.lock()
|
||||
.unwrap()
|
||||
.insert("bitcoin-knots".into(), "fatal startup error".into());
|
||||
rt.set_state("bitcoin-knots", ContainerState::Exited);
|
||||
let report = orch.reconcile_all().await;
|
||||
assert_eq!(
|
||||
report.actions,
|
||||
vec![(
|
||||
"bitcoin-knots".to_string(),
|
||||
ReconcileAction::Left("repair-attempts-exhausted".to_string())
|
||||
)]
|
||||
);
|
||||
let calls_after = rt.calls();
|
||||
assert_eq!(
|
||||
calls_after
|
||||
.iter()
|
||||
.filter(|c| c.starts_with("remove_container:"))
|
||||
.count(),
|
||||
remove_calls_before,
|
||||
"breaker must skip the recreate's remove_container entirely"
|
||||
);
|
||||
assert_eq!(
|
||||
calls_after
|
||||
.iter()
|
||||
.filter(|c| c.starts_with("create_container:"))
|
||||
.count(),
|
||||
create_calls_before,
|
||||
"breaker must skip the recreate's create_container entirely"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn reconcile_installs_missing_container() {
|
||||
let rt = Arc::new(MockRuntime::default());
|
||||
|
||||
@@ -7,7 +7,7 @@ use anyhow::{Context, Result};
|
||||
use serde::{Deserialize, Serialize};
|
||||
use std::path::{Path, PathBuf};
|
||||
use tokio::fs;
|
||||
use tracing::debug;
|
||||
use tracing::{debug, warn};
|
||||
|
||||
const CATALOG_FILE: &str = "content/catalog.json";
|
||||
const CONTENT_DIR: &str = "content/files";
|
||||
@@ -86,6 +86,22 @@ pub async fn save_catalog(data_dir: &Path, catalog: &ContentCatalog) -> Result<(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Removes `id` from the on-disk catalog. Best-effort: a failure here just
|
||||
/// means the entry gets pruned again next time it's requested, so errors are
|
||||
/// logged rather than propagated.
|
||||
async fn prune_missing_content_entry(data_dir: &Path, id: &str) {
|
||||
let Ok(mut catalog) = load_catalog(data_dir).await else {
|
||||
return;
|
||||
};
|
||||
let before = catalog.items.len();
|
||||
catalog.items.retain(|i| i.id != id);
|
||||
if catalog.items.len() != before {
|
||||
if let Err(e) = save_catalog(data_dir, &catalog).await {
|
||||
warn!(error = %e, content_id = %id, "failed to save catalog after pruning missing content entry");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Get the full filesystem path for a content item.
|
||||
/// Checks the dedicated content/files/ directory first, then falls back to the
|
||||
/// FileBrowser data directory (where users manage files via the web UI).
|
||||
@@ -268,6 +284,19 @@ pub async fn serve_content(
|
||||
|
||||
let file_path = content_file_path(data_dir, item);
|
||||
if !file_path.exists() {
|
||||
// The catalog entry survived (it's a separate JSON file) but its
|
||||
// backing file is gone — most likely lost in an unrelated data-dir
|
||||
// reset (a shared filebrowser file, 2026-07-01: two catalog entries
|
||||
// outlived a filebrowser reinstall that wiped the files themselves).
|
||||
// Leaving the entry in place would keep advertising it as available
|
||||
// to every peer forever, each hitting the exact same dead end this
|
||||
// one just did. Prune it so it stops being offered.
|
||||
warn!(
|
||||
content_id = %id,
|
||||
filename = %item.filename,
|
||||
"content catalog entry's file is missing on disk — pruning the stale entry"
|
||||
);
|
||||
prune_missing_content_entry(data_dir, id).await;
|
||||
return Ok(ServeResult::NotFound);
|
||||
}
|
||||
|
||||
@@ -555,3 +584,95 @@ mod faststart_tests {
|
||||
assert_eq!(mp4_is_faststart(&p).await, Some(false));
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod prune_missing_content_tests {
|
||||
use super::*;
|
||||
|
||||
#[tokio::test]
|
||||
async fn serve_content_prunes_catalog_entry_whose_file_is_missing() {
|
||||
// Simulates a catalog entry that outlived its backing file (a shared
|
||||
// filebrowser file lost in an unrelated data-dir reset, 2026-07-01) —
|
||||
// every peer request for it would otherwise 404 forever with no way
|
||||
// to tell it apart from a transient failure.
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let data_dir = dir.path();
|
||||
let item = ContentItem {
|
||||
id: "missing-item".to_string(),
|
||||
filename: "gone.mp4".to_string(),
|
||||
mime_type: "video/mp4".to_string(),
|
||||
size_bytes: 123,
|
||||
description: String::new(),
|
||||
access: AccessControl::Free,
|
||||
availability: Availability::AllPeers,
|
||||
added_at: "2026-01-01T00:00:00Z".to_string(),
|
||||
};
|
||||
save_catalog(
|
||||
data_dir,
|
||||
&ContentCatalog {
|
||||
items: vec![item],
|
||||
},
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
// File was never written to disk under content/files/ or filebrowser/.
|
||||
let result = serve_content(data_dir, "missing-item", None, None, None, None)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(matches!(result, ServeResult::NotFound));
|
||||
|
||||
let reloaded = load_catalog(data_dir).await.unwrap();
|
||||
assert!(
|
||||
reloaded.items.is_empty(),
|
||||
"stale entry should have been pruned after the 404"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn serve_content_leaves_other_entries_untouched_when_pruning() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let data_dir = dir.path();
|
||||
let missing = ContentItem {
|
||||
id: "missing-item".to_string(),
|
||||
filename: "gone.mp4".to_string(),
|
||||
mime_type: "video/mp4".to_string(),
|
||||
size_bytes: 123,
|
||||
description: String::new(),
|
||||
access: AccessControl::Free,
|
||||
availability: Availability::AllPeers,
|
||||
added_at: "2026-01-01T00:00:00Z".to_string(),
|
||||
};
|
||||
let present = ContentItem {
|
||||
id: "present-item".to_string(),
|
||||
filename: "here.mp4".to_string(),
|
||||
mime_type: "video/mp4".to_string(),
|
||||
size_bytes: 4,
|
||||
description: String::new(),
|
||||
access: AccessControl::Free,
|
||||
availability: Availability::AllPeers,
|
||||
added_at: "2026-01-01T00:00:00Z".to_string(),
|
||||
};
|
||||
save_catalog(
|
||||
data_dir,
|
||||
&ContentCatalog {
|
||||
items: vec![missing, present],
|
||||
},
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
let content_dir = data_dir.join("content").join("files");
|
||||
tokio::fs::create_dir_all(&content_dir).await.unwrap();
|
||||
tokio::fs::write(content_dir.join("here.mp4"), b"data")
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let _ = serve_content(data_dir, "missing-item", None, None, None, None)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let reloaded = load_catalog(data_dir).await.unwrap();
|
||||
assert_eq!(reloaded.items.len(), 1);
|
||||
assert_eq!(reloaded.items[0].id, "present-item");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user