From 562626e878d394c6a8e3abfc6af054653cabd712 Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Tue, 29 Sep 2026 15:05:08 +0200 Subject: [PATCH] deb: report a kept record honestly when no staged tree exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every --incremental run claimed "kept its staged tree" and set kept_upper=true at teardown — even when the run died before staging created tree-upper, leaving a manifest that lied about the record being a fork candidate and a message contradicting the disk. The retention now follows the disk through upper_retention(requested, has_upper()): a keep with a staged tree keeps it as before, a plain run drops it, and a keep with nothing staged keeps the record as metadata + log with kept_upper=false and says so ("no staged tree was created — the run ended before staging"). The interrupt path records the same honest state — failed outcome, kept_upper only if a tree exists — and the SIGINT hook's message no longer claims a tree that is not there. --- src/deb/ephemeral.rs | 26 ++++++-- src/deb/session.rs | 151 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 149 insertions(+), 28 deletions(-) diff --git a/src/deb/ephemeral.rs b/src/deb/ephemeral.rs index deff9f8..2b9fff4 100644 --- a/src/deb/ephemeral.rs +++ b/src/deb/ephemeral.rs @@ -34,9 +34,12 @@ fn sigint_cleanup_chroot(chroot_path: &Path) { /// Interrupt-time teardown of a build record: unmount everything under /// the record root, remove the per-run state (the chroot and the overlay -/// scratch workdir) and — unless `keep_upper` is set — the staged tree -/// and its host-tree snapshot. The manifest and the build log always -/// stay: every build leaves a metadata record, the interrupt included. +/// scratch workdir) and, for a plain run, the staged tree and its +/// host-tree snapshot. An `--incremental` run keeps whatever staging +/// created — the keep only claims a staged tree when one exists on +/// disk; a run interrupted before staging keeps none. The manifest and +/// the build log always stay: every build leaves a metadata record, the +/// interrupt included. /// /// Unlike [`EphemeralContextGuard::drop`], this deliberately does NOT go /// through the context manager, the ephemeral context's driver (whose @@ -57,12 +60,20 @@ fn sigint_cleanup_record(record_root: &Path, keep_upper: bool) { for dir in ["chroot", "tree-upper-work"] { remove_with_retries(&record_root.join(dir), is_root); } - if keep_upper { + if keep_upper && record_root.join("tree-upper").is_dir() { log::debug!( "Interrupted build record kept its staged tree at {} (fork it \ with `pkh deb --incremental`)", record_root.display() ); + } else if keep_upper { + // The run ended before staging created one: keep the message + // honest — there is no staged tree to fork. + log::debug!( + "Interrupted build record kept at {} (no staged tree was \ + created — the run ended before staging)", + record_root.display() + ); } else { for path in ["tree-upper", "host-files.list"] { remove_with_retries(&record_root.join(path), is_root); @@ -1140,10 +1151,11 @@ impl Drop for EphemeralContextGuard { // drained, forgetting is a harmless no-op). if crate::interrupt::interrupted() { context::manager().set_current_ephemeral(self.previous_context.clone()); - // The record keeps its metadata: mark the aborted build as - // failed before the hook tears the per-run state down. + // The record keeps its metadata: record the aborted build + // honestly before the hook tears the per-run state down — + // failed, and kept_upper only if a staged tree exists. if let Some(session) = &self.session { - session.set_outcome(session::OUTCOME_FAILED); + session.record_interrupted(self.keep_upper); } if let Some(hook) = self.cleanup_hook.take() { std::mem::forget(hook); diff --git a/src/deb/session.rs b/src/deb/session.rs index 42b8aab..800c27d 100644 --- a/src/deb/session.rs +++ b/src/deb/session.rs @@ -202,6 +202,33 @@ pub struct Session { state: Arc>, } +/// What the end-of-run retention of a record's staged tree does, +/// decided from the request and the disk: a keep requested for a run +/// that ended before staging created a tree-upper has nothing to keep, +/// and the record must not claim otherwise. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum UpperRetention { + /// Keep the staged tree: the record is a fork candidate. + Keep, + /// Drop the staged tree: a plain run. + Drop, + /// An `--incremental` run whose staging never happened: metadata + /// and log only, `kept_upper` stays false. + KeepNothing, +} + +/// Decide the retention of a record's staged tree: an +/// `--incremental` run keeps it — unless the run ended before staging +/// created one, in which case there is nothing to keep — and a plain +/// run drops it. +pub(crate) fn upper_retention(requested: bool, upper_exists: bool) -> UpperRetention { + match (requested, upper_exists) { + (true, true) => UpperRetention::Keep, + (true, false) => UpperRetention::KeepNothing, + (false, _) => UpperRetention::Drop, + } +} + impl Session { /// The record root directory. pub fn root(&self) -> &Path { @@ -318,32 +345,43 @@ impl Session { } /// Retain the record at the end of its build: the outcome is - /// recorded on success (failures already carry theirs), and the - /// upperdir is kept exactly for `--incremental` runs — a plain - /// run's upperdir is dropped, leaving a metadata-only record - /// (manifest + log). The chroot was already removed by the caller's - /// teardown. The interrupt path is handled by the SIGINT cleanup - /// hook, which follows the same policy. + /// recorded on success (failures already carry theirs), and the staged + /// tree follows [`upper_retention`] — kept exactly for `--incremental` + /// runs that actually staged one; a plain run's tree is dropped and a + /// run that ended before staging keeps nothing, both leaving a + /// metadata-only record (manifest + log). The chroot was already + /// removed by the caller's teardown. The interrupt path is handled by + /// the SIGINT cleanup hook, which follows the same policy. pub(crate) fn retain_upper(&self, keep_upper: bool, succeeded: bool) { if succeeded { self.set_outcome(OUTCOME_SUCCESS); } - if keep_upper { - self.with_manifest(|manifest| { - manifest.kept_upper = true; - }); - log::info!( - "Build record {} kept its staged tree (the next \ + match upper_retention(keep_upper, self.has_upper()) { + UpperRetention::Keep => { + self.with_manifest(|manifest| { + manifest.kept_upper = true; + }); + log::info!( + "Build record {} kept its staged tree (the next \ --incremental run forks it)", - self.manifest().id - ); - self.enforce_upper_cap(); - } else { - log::debug!( - "Dropping the staged tree of build record {} (plain run)", - self.manifest().id - ); - self.wipe_upper(); + self.manifest().id + ); + self.enforce_upper_cap(); + } + UpperRetention::KeepNothing => { + log::info!( + "Build record {} kept (no staged tree was created — the \ + run ended before staging)", + self.manifest().id + ); + } + UpperRetention::Drop => { + log::debug!( + "Dropping the staged tree of build record {} (plain run)", + self.manifest().id + ); + self.wipe_upper(); + } } } @@ -418,6 +456,21 @@ impl Session { } } + /// Record an interrupted build: the outcome is failed, and + /// `kept_upper` follows the disk — an `--incremental` run keeps + /// whatever staging created, and a run interrupted before staging + /// keeps no claim of a staged tree. The per-run state itself is the + /// SIGINT hook's to remove (unmounting and rm are not safe here, on + /// the interrupted thread). + pub(crate) fn record_interrupted(&self, keep_upper: bool) { + self.set_outcome(OUTCOME_FAILED); + if upper_retention(keep_upper, self.has_upper()) == UpperRetention::Keep { + self.with_manifest(|manifest| { + manifest.kept_upper = true; + }); + } + } + /// Take the advisory lock marking a live build on this record. /// /// Fails with [`SessionBusy`] when another build holds it. The lock is @@ -2515,6 +2568,62 @@ mod tests { } } + /// The retention decision follows the disk, not the request: an + /// `--incremental` run that ended before staging created a staged + /// tree has nothing to keep, and neither the manifest nor the + /// end-of-run message may claim otherwise. + #[test] + fn upper_retention_reflects_the_disk_not_the_request() { + use UpperRetention::{Drop, Keep, KeepNothing}; + + assert_eq!(upper_retention(true, true), Keep); + assert_eq!(upper_retention(true, false), KeepNothing); + assert_eq!(upper_retention(false, true), Drop); + assert_eq!(upper_retention(false, false), Drop); + } + + /// Driving the keep path on an incremental record whose staged tree + /// was never created: the record is kept as metadata + log, the + /// manifest does not claim a staged tree, and the interrupt path + /// records the same honest state. + #[test] + fn keep_without_a_staged_tree_claims_none() { + let (root, tarball) = sessions_fixture(); + let identity = test_identity(); + let host_tree = root.path().join("tree"); + + // A run that ended before staging: no tree-upper, no snapshot. + let record = new_record_in(root.path(), &host_tree, &identity, "1.0").unwrap(); + assert!(!record.has_upper()); + + record.retain_upper(true, false); + + assert!(record.root().join("manifest.json").is_file(), "metadata"); + assert!(record.log_file().is_file(), "the log is always kept"); + assert!( + !record.manifest().kept_upper, + "no staged tree was created: the manifest must not claim one" + ); + assert_eq!(record.manifest().last_outcome, OUTCOME_RUNNING); + drop(record); + + // The interrupt path records the same honest state. + let record = new_record_in(root.path(), &host_tree, &identity, "1.0").unwrap(); + record.record_interrupted(true); + assert_eq!(record.manifest().last_outcome, OUTCOME_FAILED); + assert!( + !record.manifest().kept_upper, + "an interrupted pre-staging run keeps no claim of a staged tree" + ); + drop(record); + + // And an interrupted run that DID stage claims exactly that. + let record = record_with_upper(root.path(), &tarball, &host_tree, &identity, "1.0", None); + record.record_interrupted(true); + assert!(record.manifest().kept_upper); + assert!(record.has_upper(), "the tree stays on disk"); + } + /// The per-run chroot and overlay workdir are removed at the end of /// the run — nothing in a record root survives except the manifest, /// the log and the retained staged tree.