repair: one pid can carry two attributions, and they are two questions
Round 6, one blocking P2 — in code I wrote in the round-5 fix, not in the token design, which the review passed again. The verdict cache was keyed on pid alone. A pid can legitimately be claimed by a tokened module *and* an untagged one at the same time — a host crashes, an older build restarts and reuses the number — and those are not the same question: one is answerable directly from the token, the other only if the degradation signals allow it. Collapsing them let whichever module sorted last decide both, so under `--repair-legacy-untagged` with a degraded probe an untagged winner made safely attributable debris `Unknown`, and a tokened winner planned the untagged debris as dead (the execution recheck happened to stop the destruction, which is luck, not design). Verdicts are now cached per `(pid, Attribution)` and each fingerprint is filtered by its own, never by looking its pid up in `dead_pids`. Those three pid sets are documented as reporting-only, since a pid can now honestly appear in two of them. Mutation-verified: restoring the `dead_pids.contains(pid)` filter fails the new test, which runs both module-id orders because the bug was order-dependent, and both polarities — the second asserts that a *live* tokened owner does not lose its module because an untagged claim on the same pid looked dead. Also, the degraded-probe warning had become false (P3): it announced "refusing to unload anything" while the tokened path can now legitimately unload, which in the exact container-recovery case the token was added for would print a categorical refusal and then destroy state. It is now scoped to what it actually means — modules *without* a token will be left alone. 256 tests, clippy clean, fmt clean, field gates green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
+7
-2
@@ -35,9 +35,14 @@ use plan::{Fingerprint, Liveness, Shape};
|
|||||||
pub async fn run(clean_untagged: bool) -> Result<()> {
|
pub async fn run(clean_untagged: bool) -> Result<()> {
|
||||||
let liveness = LivenessProbe::new();
|
let liveness = LivenessProbe::new();
|
||||||
if let Some(reason) = liveness.degraded_reason() {
|
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!(
|
eprintln!(
|
||||||
"[pixelpass] --repair: cannot determine process liveness ({reason}); \
|
"[pixelpass] --repair: cannot independently determine process liveness \
|
||||||
refusing to unload anything."
|
({reason}); modules WITHOUT an ownership token will be left alone."
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+86
-4
@@ -284,7 +284,7 @@ impl LocalIdentity {
|
|||||||
/// Passed to the liveness callback because the answer changes *how* the question
|
/// 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
|
/// 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.
|
/// 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 {
|
pub enum Attribution {
|
||||||
/// Carries a token matching this machine, boot and pid namespace.
|
/// Carries a token matching this machine, boot and pid namespace.
|
||||||
Tokened,
|
Tokened,
|
||||||
@@ -537,6 +537,11 @@ pub struct Plan {
|
|||||||
pub unload: Vec<Fingerprint>,
|
pub unload: Vec<Fingerprint>,
|
||||||
/// Pids that are still alive and were therefore skipped entirely. Includes
|
/// Pids that are still alive and were therefore skipped entirely. Includes
|
||||||
/// this process and any other running pixelpass.
|
/// 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<u32>,
|
pub live_pids: BTreeSet<u32>,
|
||||||
/// Pids we concluded are gone.
|
/// Pids we concluded are gone.
|
||||||
pub dead_pids: BTreeSet<u32>,
|
pub dead_pids: BTreeSet<u32>,
|
||||||
@@ -675,12 +680,21 @@ pub fn plan(
|
|||||||
let mut live_pids = BTreeSet::new();
|
let mut live_pids = BTreeSet::new();
|
||||||
let mut dead_pids = BTreeSet::new();
|
let mut dead_pids = BTreeSet::new();
|
||||||
let mut unknown_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
|
for (pid, attribution) in judgeable
|
||||||
.iter()
|
.iter()
|
||||||
.map(|(fp, attribution)| (fp.pid, *attribution))
|
.map(|(fp, attribution)| (fp.pid, *attribution))
|
||||||
.collect::<BTreeMap<_, _>>()
|
.collect::<BTreeSet<_>>()
|
||||||
{
|
{
|
||||||
match liveness(pid, attribution) {
|
let verdict = liveness(pid, attribution);
|
||||||
|
verdicts.insert((pid, attribution), verdict);
|
||||||
|
match verdict {
|
||||||
Liveness::Alive => live_pids.insert(pid),
|
Liveness::Alive => live_pids.insert(pid),
|
||||||
Liveness::Dead => dead_pids.insert(pid),
|
Liveness::Dead => dead_pids.insert(pid),
|
||||||
Liveness::Unknown => unknown_pids.insert(pid),
|
Liveness::Unknown => unknown_pids.insert(pid),
|
||||||
@@ -689,8 +703,10 @@ pub fn plan(
|
|||||||
|
|
||||||
let mut unload: Vec<Fingerprint> = judgeable
|
let mut unload: Vec<Fingerprint> = judgeable
|
||||||
.into_iter()
|
.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)
|
.map(|(fp, _)| fp)
|
||||||
.filter(|fp| dead_pids.contains(&fp.pid))
|
|
||||||
.collect();
|
.collect();
|
||||||
// `Shape`'s declaration order is the unload order: loopbacks before the sink
|
// `Shape`'s declaration order is the unload order: loopbacks before the sink
|
||||||
// they reference. Id breaks ties so the plan is deterministic.
|
// they reference. Id breaks ties so the plan is deterministic.
|
||||||
@@ -1359,6 +1375,72 @@ mod tests {
|
|||||||
assert_eq!(ids(&plan), vec![10, 11]);
|
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.
|
/// Tokened and legacy forms must both classify, and carry the difference.
|
||||||
#[test]
|
#[test]
|
||||||
fn both_forms_classify_and_record_whether_they_are_attributable() {
|
fn both_forms_classify_and_record_whether_they_are_attributable() {
|
||||||
|
|||||||
Reference in New Issue
Block a user