diff --git a/src/repair/mod.rs b/src/repair/mod.rs index 7e3ff5a..fa8fe77 100644 --- a/src/repair/mod.rs +++ b/src/repair/mod.rs @@ -35,9 +35,14 @@ use plan::{Fingerprint, Liveness, Shape}; pub async fn run(clean_untagged: bool) -> Result<()> { let liveness = LivenessProbe::new(); if let Some(reason) = liveness.degraded_reason() { + // Scoped deliberately: modules carrying a token that matches this machine, + // boot and pid namespace are still cleaned, because the token establishes + // what these signals can only guess at. Saying "refusing to unload + // anything" here would be false in exactly the container-recovery case the + // token was added for. eprintln!( - "[pixelpass] --repair: cannot determine process liveness ({reason}); \ - refusing to unload anything." + "[pixelpass] --repair: cannot independently determine process liveness \ + ({reason}); modules WITHOUT an ownership token will be left alone." ); } diff --git a/src/repair/plan.rs b/src/repair/plan.rs index 03feb50..5f52cb4 100644 --- a/src/repair/plan.rs +++ b/src/repair/plan.rs @@ -284,7 +284,7 @@ impl LocalIdentity { /// Passed to the liveness callback because the answer changes *how* the question /// may be asked: an attributed pid needs no guessing about namespaces, while an /// untagged one is a bare number whose meaning has to be guarded some other way. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub enum Attribution { /// Carries a token matching this machine, boot and pid namespace. Tokened, @@ -537,6 +537,11 @@ pub struct Plan { pub unload: Vec, /// Pids that are still alive and were therefore skipped entirely. Includes /// this process and any other running pixelpass. + /// + /// These three sets are for *reporting*. A pid claimed by both a tokened and an + /// untagged module can legitimately appear in two of them, because those are two + /// different questions; the unload decision is made per fingerprint against its + /// own attribution, never by looking a pid up in these. pub live_pids: BTreeSet, /// Pids we concluded are gone. pub dead_pids: BTreeSet, @@ -675,12 +680,21 @@ pub fn plan( let mut live_pids = BTreeSet::new(); let mut dead_pids = BTreeSet::new(); let mut unknown_pids = BTreeSet::new(); + // Keyed on (pid, attribution), NOT on pid alone. The same pid can be claimed by + // a tokened module and an untagged one at once — a host that crashed, was + // restarted by an older build, and reused the number — and those two are not the + // same question: one is answered directly, the other only if the degradation + // signals allow it. Collapsing them lets one module's verdict decide another + // module's fate. + let mut verdicts: BTreeMap<(u32, Attribution), Liveness> = BTreeMap::new(); for (pid, attribution) in judgeable .iter() .map(|(fp, attribution)| (fp.pid, *attribution)) - .collect::>() + .collect::>() { - match liveness(pid, attribution) { + let verdict = liveness(pid, attribution); + verdicts.insert((pid, attribution), verdict); + match verdict { Liveness::Alive => live_pids.insert(pid), Liveness::Dead => dead_pids.insert(pid), Liveness::Unknown => unknown_pids.insert(pid), @@ -689,8 +703,10 @@ pub fn plan( let mut unload: Vec = judgeable .into_iter() + // Each fingerprint is filtered by *its own* verdict, not by whether the pid + // appears in `dead_pids` — which it might, for the other attribution. + .filter(|(fp, attribution)| verdicts.get(&(fp.pid, *attribution)) == Some(&Liveness::Dead)) .map(|(fp, _)| fp) - .filter(|fp| dead_pids.contains(&fp.pid)) .collect(); // `Shape`'s declaration order is the unload order: loopbacks before the sink // they reference. Id breaks ties so the plan is deterministic. @@ -1359,6 +1375,72 @@ mod tests { assert_eq!(ids(&plan), vec![10, 11]); } + /// One pid, two attributions. A host crashes, an older build restarts and reuses + /// the number, and now a tokened module and an untagged one both claim it. Those + /// are two different questions — one answerable directly, the other only if the + /// degradation signals allow it — so each module must be filtered by *its own* + /// verdict. Keying the verdict cache on the pid alone let whichever module came + /// last decide both. + /// + /// Run in both module-id orders, because the bug was order-dependent. + #[test] + fn one_pid_with_two_attributions_gets_two_verdicts() { + let policy = Policy { + local: our_identity(), + untagged: UntaggedPolicy::CleanByPidAlone, + }; + // The tokened module's owner is dead; the untagged claim on the same number + // cannot be judged, which is what a degraded probe would say. + let verdict = |_pid: u32, attribution: Attribution| match attribution { + Attribution::Tokened => Liveness::Dead, + Attribution::Untagged => Liveness::Unknown, + }; + + for (tokened_id, untagged_id) in [(10, 11), (11, 10)] { + let modules = [ + obs( + tokened_id, + Shape::LoopbackIntoCapture, + 4242, + Some(&our_token(1)), + ), + obs(untagged_id, Shape::LoopbackOutOfCapture, 4242, None), + ]; + let plan = plan(&modules, &policy, verdict); + assert_eq!( + ids(&plan), + vec![tokened_id], + "only the attributable module may be planned (ids {tokened_id}/{untagged_id}): \ + {plan:?}" + ); + } + + // And the mirror image: the untagged claim is judged dead while the tokened + // owner is alive. The live owner's module must survive. + let inverted = |_pid: u32, attribution: Attribution| match attribution { + Attribution::Tokened => Liveness::Alive, + Attribution::Untagged => Liveness::Dead, + }; + for (tokened_id, untagged_id) in [(10, 11), (11, 10)] { + let modules = [ + obs( + tokened_id, + Shape::LoopbackIntoCapture, + 4242, + Some(&our_token(1)), + ), + obs(untagged_id, Shape::LoopbackOutOfCapture, 4242, None), + ]; + let plan = plan(&modules, &policy, inverted); + assert_eq!( + ids(&plan), + vec![untagged_id], + "a live tokened owner must not have its module unloaded because an \ + untagged claim on the same pid looked dead: {plan:?}" + ); + } + } + /// Tokened and legacy forms must both classify, and carry the difference. #[test] fn both_forms_classify_and_record_whether_they_are_attributable() {