diff --git a/README.md b/README.md index 660abba..74b564c 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,14 @@ A plain `pkh deb` never reuses a session — everything is rechecked from scratch — and it replaces the session of its target. `pkh deb --resume` refuses to adopt a session built for a different series/architecture. +Packages whose `debian/rules` keeps an input-untracked stamp cache (the +kernels' `debian/stamps`) declare it in the quirks data (`resume_clear`): +resumed builds drop the cache so the wrapper steps — including the +kernel's flavour config export — re-run over the incremental inner +builds, and a config edit is never silently ignored. If a build died +hard (OOM kill), the next build unmounts and clears the leftover session +state first (escalating through sudo when needed). + ### Running the DEP-8 tests `pkh test` runs the package's as-installed tests (`debian/tests/control`, diff --git a/data/quirks.yml b/data/quirks.yml index 4d94273..16637a5 100644 --- a/data/quirks.yml +++ b/data/quirks.yml @@ -23,8 +23,20 @@ quirks: # architecture only, so a cross build installs no build-architecture # variants and the kernel's host-side tools cannot link. Inject the # build-architecture variants until the control is fixed upstream. + # The kernel's debian/rules keeps its wrapper-step stamps in + # debian/stamps, and the flavour config rule there exports .config from + # the annotations with no config prerequisite — a resumed build would + # keep the previous attempt's configuration and silently ignore config + # edits. List the stamp cache under `resume_clear` so resumed builds + # drop it and re-run the wrapper steps; the inner kbuild keeps its own + # incremental state, so only the cheap wrapper passes re-run. This is + # the packaging's long-standing shape rather than a fixable regression, + # so the entry applies to every series. linux: deb: + - series: [] + resume_clear: + - debian/stamps - series: [resolute] dependencies: replace: @@ -37,6 +49,9 @@ quirks: - libssl-dev:native linux-riscv: deb: + - series: [] + resume_clear: + - debian/stamps - series: [resolute] dependencies: replace: diff --git a/plans/deb-incremental-builds.md b/plans/deb-incremental-builds.md index bcc4ed4..1544750 100644 --- a/plans/deb-incremental-builds.md +++ b/plans/deb-incremental-builds.md @@ -427,3 +427,29 @@ ephemeral}.rs`, `src/context/{api,unshare}.rs`, `src/prune.rs` and build → same; overlay mount unsupported/failed on resume → fresh copy staging (environment resume only, artifacts discarded); no host-tree snapshot → upper wiped and re-snapshotted. + +# Addendum: resume correctness (post-merge findings) + +Two real-world failures against a kept kernel session, both fixed: + +- **The kernel's config chain is stamp-blind** — `stamp-prepare-%` + (`debian/rules.d/2-binary-arch.mk`) exports `.config` from the + annotations with no config prerequisite, so a resumed build silently + kept the previous attempt's configuration; the config edit never + reached the .deb even though `debian/rules build` ran. Dropping the + stamp cache unconditionally would be a package-specific decision, so + it is data-driven: the kernels declare `resume_clear: [debian/stamps]` + in `data/quirks.yml`, and resumed builds remove those tree-relative + paths before `debian/rules build`. The inner kbuild keeps its own + state, so the re-run wrappers stay cheap and only config-affected + objects recompile. +- **A SIGKILLed build (OOM) poisons the next one.** The crashed attempt + leaves its overlay mounts and /proc bind mount behind, and overlayfs + creates root-owned `work` state inside the workdir; the next PLAIN + build (recording is always on: it replaces the session of its + identity) failed to clear the leftovers with a permission error. + `clear_dir()` now unmounts everything under each entry at depth + (re-reading /proc/mounts, tolerating mount stacks from consecutive + crashes) and escalates through sudo; if the tree still cannot be + cleared, the plain build reports it and continues WITHOUT a session + instead of failing or layering over a half-cleared tree. diff --git a/src/deb/ephemeral.rs b/src/deb/ephemeral.rs index 7556b7d..0bb5f33 100644 --- a/src/deb/ephemeral.rs +++ b/src/deb/ephemeral.rs @@ -147,15 +147,48 @@ pub(crate) fn privileged_remove(path: &Path) -> std::io::Result<()> { } } -/// Remove the contents of `dir` but keep the directory itself. -fn wipe_dir_contents(dir: &Path) -> std::io::Result<()> { +/// Clear a directory's contents, keeping the directory itself. +/// +/// Built for re-cleaning a crashed build's leftovers: a SIGKILLed build +/// (OOM, power loss) leaves its overlay mounts and /proc bind mount behind, +/// and the kernel creates root-owned `work` state inside an overlay +/// workdir that a plain `rm -rf` cannot remove. So every child is first +/// unmounted at depth (re-reading /proc/mounts until nothing is left under +/// it), then removed with a privileged `rm -rf` fallback. +/// +/// Returns an error when some content could not be removed: callers should +/// refuse to layer a fresh build over a half-cleared tree. +pub(crate) fn clear_dir(dir: &Path) -> std::io::Result<()> { fs::create_dir_all(dir)?; + const MAX_UNMOUNT_PASSES: usize = 10; for entry in fs::read_dir(dir)?.flatten() { let path = entry.path(); - if path.is_dir() { - fs::remove_dir_all(&path)?; + // Unmount everything under the child, deepest first; the loop + // handles stacks (a re-mount over an earlier, never-unmounted one — + // exactly what consecutive crashed builds leave behind). + for _ in 0..MAX_UNMOUNT_PASSES { + let mounts = host_mounts_under(&path); + if mounts.is_empty() { + break; + } + let is_root = crate::utils::root::is_root().unwrap_or(false); + for mount in mounts.into_iter().rev() { + if !unmount_path(&mount, is_root) { + log::warn!("Failed to unmount {} (retrying)", mount.display()); + } + } + } + let result = if path.is_dir() { + fs::remove_dir_all(&path) } else { - fs::remove_file(&path)?; + fs::remove_file(&path) + } + .or_else(|_| privileged_remove(&path)); + if let Err(e) = result { + return Err(std::io::Error::other(format!( + "cannot remove {}: {e}", + path.display() + ))); } } Ok(()) @@ -487,7 +520,7 @@ impl EphemeralContextGuard { } // Not reusing (fresh session or failed probe): never extract // over stale content. - wipe_dir_contents(chroot_path)?; + clear_dir(chroot_path)?; } // Clone ctx for use in create_device_nodes after download_chroot_tarball consumes it @@ -1051,4 +1084,41 @@ mod chroot_cleanup_tests { assert!(!chroot.exists()); } + + /// clear_dir removes content a plain `rm -rf` cannot: a root-owned + /// 0700 directory (the shape of an overlay workdir left by a crashed + /// build). Skipped when neither root nor working non-interactive sudo + /// is available, since escalation is then legitimately impossible. + #[test] + fn clear_dir_removes_root_owned_subdirectories() { + let is_root = unsafe { libc::geteuid() } == 0; + if !is_root + && !privileged_command("true", false) + .status() + .is_ok_and(|s| s.success()) + { + return; + } + + let dir = tempfile::tempdir().unwrap(); + let cleared = dir.path().join("chroot"); + std::fs::create_dir_all(cleared.join("stale")).unwrap(); + std::fs::write(cleared.join("stale").join("f"), "x").unwrap(); + // Root-owned, non-traversable: the crashed-workdir shape. + let _ = privileged_command("chmod", is_root) + .arg("-R") + .arg("0700") + .arg(cleared.join("stale")) + .status(); + let _ = privileged_command("chown", is_root) + .arg("-R") + .arg("root:root") + .arg(cleared.join("stale")) + .status(); + + clear_dir(&cleared).expect("clear_dir removes root-owned leftovers"); + + let read = std::fs::read_dir(&cleared).unwrap(); + assert_eq!(read.count(), 0, "the directory must be empty"); + } } diff --git a/src/deb/local.rs b/src/deb/local.rs index 9ca3216..315fe23 100644 --- a/src/deb/local.rs +++ b/src/deb/local.rs @@ -340,6 +340,31 @@ pub async fn build( ); } + // A resumed build continues in the previous attempt's tree. Packages + // whose debian/rules keeps an input-untracked stamp cache (quirk: + // `resume_clear`, e.g. the kernels' `debian/stamps`, whose flavour + // config rule exports .config with no config prerequisite) would build + // from the previous attempt's configuration: drop the listed caches so + // the wrapper steps re-run over the incremental inner builds. + if resume { + for rel in crate::quirks::get_resume_clear_paths(package, series) { + let path = Path::new(package_dir_str).join(&rel); + if !ctx.exists(&path).unwrap_or(false) { + continue; + } + log::info!("Dropping the resumed build's cached '{rel}' (quirk)"); + let status = ctx + .command("rm") + .arg("-rf") + .arg(&path) + .status() + .map_err(|e| format!("cannot drop '{rel}': {e}"))?; + if !status.success() { + return Err(format!("cannot drop '{rel}'").into()); + } + } + } + // Run the build step log::debug!("Building (debian/rules build) package..."); enter_phase(view, Phase::Building); diff --git a/src/deb/mod.rs b/src/deb/mod.rs index 94144f8..8a005bd 100644 --- a/src/deb/mod.rs +++ b/src/deb/mod.rs @@ -524,6 +524,10 @@ fn resolve_session( log::info!("{busy}; building without a session"); Ok(None) } + Err(session::SessionOpenError::Unusable(e)) => { + log::error!("{e}; building without a session"); + Ok(None) + } Err(session::SessionOpenError::Other(e)) => Err(e.into()), }, SessionResume::Latest => { diff --git a/src/deb/session.rs b/src/deb/session.rs index 6a17e1b..7d97717 100644 --- a/src/deb/session.rs +++ b/src/deb/session.rs @@ -350,20 +350,7 @@ impl Session { /// attempt's tree would merge stale build artifacts and resurrect /// host-deleted files into the build. pub fn wipe_staged_tree(&self) -> io::Result<()> { - let staged = self.chroot_path().join("tmp/pkh-build"); - fs::create_dir_all(&staged)?; - for entry in fs::read_dir(&staged)?.flatten() { - let path = entry.path(); - let result = if path.is_dir() { - fs::remove_dir_all(&path) - } else { - fs::remove_file(&path) - } - .or_else(|_| crate::deb::ephemeral::privileged_remove(&path)); - result - .map_err(|e| io::Error::other(format!("cannot clear {}: {e}", staged.display())))?; - } - Ok(()) + crate::deb::ephemeral::clear_dir(&self.chroot_path().join("tmp/pkh-build")) } /// Remove the whole session tree (unmounting anything mounted under @@ -557,6 +544,9 @@ fn open_from_dir(root: PathBuf) -> Option { pub enum SessionOpenError { /// The session is locked by a concurrent build. Busy(SessionBusy), + /// The previous session could not be cleared (crashed-build leftovers + /// that even the privileged removal could not lift). + Unusable(String), /// Anything else (I/O, manifest errors). Other(String), } @@ -565,6 +555,7 @@ impl std::fmt::Display for SessionOpenError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { SessionOpenError::Busy(busy) => write!(f, "{busy}"), + SessionOpenError::Unusable(message) => write!(f, "{message}"), SessionOpenError::Other(message) => write!(f, "{message}"), } } @@ -618,28 +609,17 @@ pub fn fresh_session( session.acquire().map_err(SessionOpenError::Busy)?; - // Wipe the previous attempt (the lock file lives outside the root): a - // previous chroot holds root-owned device nodes, so escalate on - // failure, like the prune removal does. - for entry in fs::read_dir(&root) - .map_err(|e| SessionOpenError::Other(format!("cannot clear {}: {e}", root.display())))? - .flatten() - { - let path = entry.path(); - let result = if path.is_dir() { - fs::remove_dir_all(&path) - } else { - fs::remove_file(&path) - } - .or_else(|_| crate::deb::ephemeral::privileged_remove(&path)); - if let Err(e) = result { - log::warn!( - "Failed to clear the previous session file '{}': {}", - path.display(), - e - ); - } - } + // Wipe the previous attempt (the lock file lives outside the root). A + // crashed build leaves its mounts behind and the kernel creates + // root-owned state inside overlay workdirs, so the clearing unmounts + // at depth and escalates via sudo when needed; see clear_dir. + crate::deb::ephemeral::clear_dir(&root).map_err(|e| { + SessionOpenError::Unusable(format!( + "cannot clear the previous build session {}: {e}. \ + Free it with `pkh prune` (or remove it manually) and rebuild", + root.display() + )) + })?; session.reset_fresh(host_tree, identity, tree_version); Ok(session) diff --git a/src/quirks.rs b/src/quirks.rs index 36e0039..ee9cc41 100644 --- a/src/quirks.rs +++ b/src/quirks.rs @@ -54,6 +54,20 @@ pub struct OperationQuirks { /// like linux packages that use directories like "linux-main" or other custom names #[serde(default)] pub package_directory: Vec, + + /// Paths (relative to the package tree) removed before + /// `debian/rules build` when the build resumes a previous attempt. + /// + /// For packages whose debian/rules keeps a stamp cache that does not + /// track its inputs (the Ubuntu/Debian kernels' `debian/stamps`: the + /// flavour config rule exports `.config` from the annotations with no + /// config prerequisite), a resumed build would keep building from the + /// previous attempt's configuration. Listing the stamp directories + /// here drops the cache so the wrapper steps re-run, while the inner + /// build system (kbuild) keeps its own incremental state. Only list + /// caches whose re-run is cheap or whose inner build is incremental. + #[serde(default)] + pub resume_clear: Vec, } /// Quirks for a specific package @@ -166,6 +180,33 @@ pub fn get_package_directories(package: &str, series: &str) -> Vec { directories } +/// Get the paths to clear before a resumed `debian/rules build` +/// +/// Every matching deb entry contributes its paths, in file order. See +/// [`OperationQuirks::resume_clear`] for what belongs here. +/// +/// # Arguments +/// * `package` - The package name +/// * `series` - The distribution series (e.g. "resolute") +/// +/// # Returns +/// * `Vec` - Tree-relative paths to remove, empty when the package +/// opts out +pub fn get_resume_clear_paths(package: &str, series: &str) -> Vec { + let Some(quirks) = get_package_quirks(&QUIRKS_DATA, package) else { + return Vec::new(); + }; + let mut paths = Vec::new(); + for deb in quirks + .deb + .iter() + .filter(|q| entry_applies_to_series(q, series)) + { + paths.extend(deb.resume_clear.iter().cloned()); + } + paths +} + /// Apply the dependency quirks of `package` in `series` to parsed /// build-dependency clauses /// @@ -374,4 +415,23 @@ mod tests { }; assert!(apply_rules(&mut clauses, &deps, &opts()).is_err()); } + + /// The kernels opt in to clearing their stamp cache on resume, for + /// every series; packages without the entry opt out by default. + #[test] + fn resume_clear_paths_are_data_driven() { + for package in ["linux", "linux-riscv"] { + assert_eq!( + get_resume_clear_paths(package, "resolute"), + vec!["debian/stamps".to_string()] + ); + assert_eq!( + get_resume_clear_paths(package, "some-future-series"), + vec!["debian/stamps".to_string()], + "the stamp-cache entry applies to every series" + ); + } + assert!(get_resume_clear_paths("hello", "resolute").is_empty()); + assert!(get_resume_clear_paths("no-such-package", "resolute").is_empty()); + } }