From ca106c5a4328480186d2c9a49a5a58bb5944e09c Mon Sep 17 00:00:00 2001 From: archipelago Date: Fri, 7 Aug 2026 18:54:39 -0400 Subject: [PATCH] fix(container): data-uid chown is drift-gated, not unconditional every tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apply_data_uid ran a recursive sudo chown on every prepare_for_start, and the reconciler re-prepares — archi-dev-box's journal showed postgres-btcpay rechowned every ~45s despite already-correct ownership, and on framework-pt the same loop surfaced as operator-visible 'chown failed' noise. chown_for_rootless_container now stats the target first and returns early when the top-level owner already matches the host-mapped uid:gid. Deep drift in a running container is still caught by ensure_running_container_ownership's in-container write-probe, which is the authority that actually matters (it probes writability, not stat bits). Co-Authored-By: Claude --- .../src/container/prod_orchestrator.rs | 56 ++++++++++++++++--- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/core/archipelago/src/container/prod_orchestrator.rs b/core/archipelago/src/container/prod_orchestrator.rs index cb836168..9d773f8f 100644 --- a/core/archipelago/src/container/prod_orchestrator.rs +++ b/core/archipelago/src/container/prod_orchestrator.rs @@ -277,6 +277,21 @@ async fn chown_for_rootless_container(uid_gid: &str, path: &str) -> Result<()> { .map(|(u, g)| (u.parse::().unwrap_or(0), g.parse::().unwrap_or(0))) .unwrap_or((0, 0)); + // Idempotence: a directory already owned by the host-mapped target must + // not get another recursive chown on every prepare/reconcile tick — that + // unconditional chown is what looped on postgres-btcpay every ~45s + // (archi-dev-box, 2026-08-07). Deep-file drift in a RUNNING container is + // still caught by ensure_running_container_ownership's write-probe. + let host_uid_gid = if uid > 0 && uid < 100_000 { + let map = |id: u32| if id == 0 { 1000 } else { 100_000 + id - 1 }; + format!("{}:{}", map(uid), map(gid)) + } else { + uid_gid.to_string() + }; + if ownership_already_correct(path, &host_uid_gid) { + return Ok(()); + } + if uid > 0 && uid < 100_000 { let output = tokio::process::Command::new("podman") .args(["unshare", "chown", "-R", uid_gid, path]) @@ -295,12 +310,6 @@ async fn chown_for_rootless_container(uid_gid: &str, path: &str) -> Result<()> { // crash-loop, framework-pt 2026-08-06). Container uid N (N>=1) lives at // subuid_base + N - 1; the fleet provisions base 100000. uid 0 and // already-mapped ids (>=100000) pass through untouched. - let host_uid_gid = if uid > 0 && uid < 100_000 { - let map = |id: u32| if id == 0 { 1000 } else { 100_000 + id - 1 }; - format!("{}:{}", map(uid), map(gid)) - } else { - uid_gid.to_string() - }; let status = host_sudo(&["chown", "-R", &host_uid_gid, path]) .await .with_context(|| format!("sudo chown -R {host_uid_gid} {path}"))?; @@ -327,6 +336,23 @@ async fn chown_for_rootless_container(uid_gid: &str, path: &str) -> Result<()> { )) } +/// Cheap drift gate for `chown_for_rootless_container`: does the path's +/// top-level owner already match the target "uid:gid" string? A miss on a +/// missing path answers false (the caller's mkdir runs first anyway). +fn ownership_already_correct(path: &str, host_uid_gid: &str) -> bool { + use std::os::unix::fs::MetadataExt; + let Some((uid_s, gid_s)) = host_uid_gid.split_once(':') else { + return false; + }; + let (Ok(uid), Ok(gid)) = (uid_s.parse::(), gid_s.parse::()) else { + return false; + }; + let Ok(md) = std::fs::metadata(path) else { + return false; + }; + md.uid() == uid && md.gid() == gid +} + /// `(container-id, mount-dest)` pairs whose in-container chown returned a hard, /// permanent failure (e.g. "Operation not permitted" on a mount that can't be /// re-owned from inside the userns). Remembered for the life of the process so @@ -5811,8 +5837,7 @@ app: } #[tokio::test] - async fn manifest_generated_files_can_overwrite_when_declared() { - let rt = Arc::new(MockRuntime::default()); + async fn manifest_generated_files_can_overwrite_when_declared() {let rt = Arc::new(MockRuntime::default()); let orch = orch_with(rt.clone()).await; let data_dir = tempfile::tempdir_in("/var/lib/archipelago").unwrap(); @@ -6796,4 +6821,19 @@ app: // zombie guard reports it dead → the reconciler recreates. assert!(!pid_is_alive(2_000_000_000)); } + + #[test] + fn ownership_drift_gate_matches_on_exact_owner_only() { + // A tempdir owned by this process's uid stands in for a correctly + // chowned data dir; any other uid reads as drift. + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().to_string_lossy().to_string(); + let my_uid = unsafe { libc::geteuid() }; + let mine = format!("{0}:{0}", my_uid); + assert!(ownership_already_correct(&path, &mine)); + assert!(!ownership_already_correct(&path, "100998:100998")); + assert!(!ownership_already_correct("/nonexistent/path", &mine)); + assert!(!ownership_already_correct(&path, "garbage")); + assert!(!ownership_already_correct(&path, "1:2:3")); + } }