deb: report a kept record honestly when no staged tree exists

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.
This commit is contained in:
2026-09-29 15:05:08 +02:00
parent ab0c65dc37
commit 562626e878
2 changed files with 149 additions and 28 deletions
+19 -7
View File
@@ -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);
+130 -21
View File
@@ -202,6 +202,33 @@ pub struct Session {
state: Arc<Mutex<SessionState>>,
}
/// 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.