repair: let the token beat the namespace guesswork, and ask liveness first

Round 5. Two blocking P2s, both in the execution shell rather than the token
design, plus an overstated claim of mine.

**The degradation signals were defeating the token in exactly the case it exists
for.** A host crashing inside a container leaves a token that matches this
machine, boot and pid namespace perfectly — but the probe saw a container marker
or a multi-entry `NSpid` and answered `Unknown` for everything, so token-qualified
repair did nothing precisely where it had just become safe. Those signals are
guesswork about whether a bare pid is meaningful, and for an attributed pid that
is no longer a guess. Liveness is now asked with the module's attribution in hand:
a tokened pid goes straight to `kill`, while an untagged one still has to get past
the signals, because there they are the only protection left.

**Liveness now runs before the fresh snapshot, not after it.** The natural order —
verify the module, check liveness, unload — leaves the dangerous window open:
after `kill` returns ESRCH this process can be descheduled while the planned module
vanishes, a new host inherits both the pid and the module index, and its
differently-nonced arguments occupy that index. Nothing re-read those arguments, so
the reused index would have been unloaded. Asking liveness first means the
post-liveness fingerprint check catches that replacement, leaving only the
irreducible snapshot-to-unload interval.

**The nonce claim was overstated and is now true.** A token was minted once per
`Routing` session and reused for every subsequent reload, making it a host-session
nonce rather than a per-load one. It is now minted inside `load_module`, mixing a
bumped counter with the clock, so two loads by the same pid really do render
different arguments — which, combined with the reordering above, is what lets a
fingerprint tell a module from its replacement at the same index.

⚠️ **The first version of the attribution fix had no gate, and the mutation said
so.** Swapping `of_attributed` back to `of` passed all 254 tests, because on an
ordinary desktop the probe is not degraded and the two paths agree, while the
planner tests use a fake liveness closure that never touches the probe at all. The
new test constructs a *deliberately degraded* probe, which is the only state where
the distinction is observable. Both mutants — routing a tokened pid through `of`,
and re-applying the degradation gate inside `of_attributed` — now fail it.

Codex's answers to the questions I raised, recorded because they close them:
omitting the pid from the token loses nothing, since the canonical argument already
binds the token to exactly one pid; namespace inode reuse is real but only after
the old namespace is destroyed, so its host is necessarily gone and no live owner is
endangered; and refusing foreign tokens even under `--repair-legacy-untagged` is the
right line, because the flag speaks to missing evidence rather than wrong evidence.
No finding against the two-hole template derivation.

255 tests, clippy clean, fmt clean. All four live field gates re-run green: tokened
cleaned, foreign refused with and without the flag, untagged refused then cleaned on
request, A/B orphan removal byte-identical elsewhere, reference gate still firing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

src/host/audio.rs

@@ -67,13 +67,12 @@ impl Routing {
         let pid = std::process::id();
         let sink_name = repair_plan::sink_name_for(pid);
 
-        // Every module this host loads carries an ownership token, so `--repair`
+        // Every module this host loads carries an ownership token, minted per
-        // can tell whose pid the name refers to instead of assuming the number
+        // load, so `--repair` can tell whose pid the name refers to instead of
-        // means the same thing everywhere. Without it a repair run in another pid
+        // assuming the number means the same thing everywhere. Without it a repair
-        // namespace can unload a live host's audio; see `repair::plan::OwnerToken`.
+        // run in another pid namespace can unload a live host's audio; see
-        let owner = owner_token(pid).context("could not build an audio ownership token")?;
+        // `repair::plan::OwnerToken`.
-
+        let sink_module = load_module(Shape::LegacyCaptureSink, pid)
-        let sink_module = load_module(Shape::LegacyCaptureSink, pid, &owner)
             .context("failed to load module-null-sink")?;
 
         // In strict per-app mode we never mirror the default sink: the viewer
@@ -89,7 +88,7 @@ impl Routing {
             None
         } else {
             Some(
-                load_module(Shape::LoopbackIntoCapture, pid, &owner)
+                load_module(Shape::LoopbackIntoCapture, pid)
                     .context("failed to load module-loopback (null-sink cleaned up on Drop)")?,
             )
         };
@@ -117,7 +116,6 @@ impl Routing {
             let (router, mut event_rx) = StreamRouter::spawn(app.clone(), sink_name.clone())?;
             let loopback_for_task = Arc::clone(&loopback_arc);
             let local_monitor_for_task = Arc::clone(&local_monitor_arc);
-            let owner_for_task = owner.clone();
             let strict = opts.strict_audio;
             let event_task = tokio::spawn(async move {
                 use crate::common::output::{self, AppAudioState};
@@ -139,8 +137,7 @@ impl Routing {
                             // only, never the desktop/call — so it can't echo into
                             // the capture.
                             if local_monitor_for_task.lock().unwrap().is_none() {
-                                match load_module(Shape::LoopbackOutOfCapture, pid, &owner_for_task)
+                                match load_module(Shape::LoopbackOutOfCapture, pid) {
-                                {
                                     Ok(id) => {
                                         tracing::info!(
                                             module = id,
@@ -193,7 +190,7 @@ impl Routing {
                             tracing::info!(
                                 "audio routing: last routed stream gone → restoring default-sink loopback"
                             );
-                            match load_module(Shape::LoopbackIntoCapture, pid, &owner_for_task) {
+                            match load_module(Shape::LoopbackIntoCapture, pid) {
                                 Ok(id) => {
                                     *loopback_for_task.lock().unwrap() = Some(id);
                                 }
@@ -342,24 +339,31 @@ struct SinkInputProperties {
 // pactl module helpers
 // ──────────────────────────────────────────────────────────────────────
 
-/// Mint this host's ownership token.
+/// Mint an ownership token for one module load.
 ///
-/// The nonce is what makes two loads by the same pid distinguishable, which is why
+/// **Per load, not per session.** The nonce is what makes two loads by the same pid
-/// it is per-call rather than per-process: it narrows the window where a module that
+/// render different arguments, which is what lets a fingerprint tell a module from
-/// vanished and a replacement that inherited its index look byte-identical.
+/// its replacement at the same index. A token minted once and reused for every
+/// reload would be a host-session nonce and would not do that, so the counter is
+/// bumped on every call and mixed with the clock.
 fn owner_token(pid: u32) -> Result<repair_plan::OwnerToken> {
+    use std::sync::atomic::{AtomicU64, Ordering};
+    static LOADS: AtomicU64 = AtomicU64::new(0);
+
     let local = crate::repair::local_identity()?;
-    // A nonce only has to be unlikely to repeat, not unguessable.
+    // A nonce only has to be unlikely to repeat, not unguessable. The counter makes
-    let nonce = std::time::SystemTime::now()
+    // two loads within the same clock tick distinct; the clock keeps two runs of the
+    // same process distinct.
+    let counter = LOADS.fetch_add(1, Ordering::Relaxed);
+    let nanos = std::time::SystemTime::now()
         .duration_since(std::time::UNIX_EPOCH)
         .map(|d| d.as_nanos() as u64)
-        .unwrap_or(0)
+        .unwrap_or(0);
-        ^ u64::from(pid) << 32;
     Ok(repair_plan::OwnerToken {
         machine: local.machine,
         boot: local.boot,
         pid_ns: local.pid_ns,
-        nonce,
+        nonce: nanos ^ (counter << 48) ^ (u64::from(pid) << 32),
     })
 }
 
@@ -370,11 +374,12 @@ fn owner_token(pid: u32) -> Result<repair_plan::OwnerToken> {
 /// `--repair`'s exact-form matcher and this loader are one source of truth. A
 /// latency or argument change that moved only one of them would leave repair
 /// silently unable to recognise the modules this build loads.
-fn load_module(shape: Shape, pid: u32, owner: &repair_plan::OwnerToken) -> Result<u32> {
+fn load_module(shape: Shape, pid: u32) -> Result<u32> {
+    let owner = owner_token(pid).context("could not build an audio ownership token")?;
     let output = Command::new("pactl")
         .arg("load-module")
         .arg(shape.module_name())
-        .args(shape.render_args(pid, Some(owner)))
+        .args(shape.render_args(pid, Some(&owner)))
         .output()
         .context("failed to run pactl load-module")?;
     if !output.status.success() {

src/repair/mod.rs

@@ -82,7 +82,9 @@ pub async fn run(clean_untagged: bool) -> Result<()> {
         }
     }
 
-    let planned = plan::plan(&modules, &policy, |pid| liveness.of(pid));
+    let planned = plan::plan(&modules, &policy, |pid, attribution| {
+        liveness_for(&liveness, attribution, pid)
+    });
 
     // Ours by shape, but carrying no proof of whose pid they name. Never unloaded by
     // default — listed, so an explicit legacy run has something to look at first.
@@ -149,9 +151,43 @@ pub async fn run(clean_untagged: bool) -> Result<()> {
     let mut failed = 0u32;
 
     for fp in &planned.unload {
-        // Fresh snapshot per action. Deliberately not hoisted out of the loop:
+        // ORDER MATTERS, and it is the opposite of what reads naturally.
-        // each unload changes the module table, and the point is to decide
+        //
-        // against the table as it is *now*, not as it was when the plan was made.
+        // Liveness is asked FIRST, and the fresh snapshot is taken AFTER it. The
+        // tempting order — verify the module, then check liveness, then unload —
+        // leaves the dangerous window wide open: between `kill` returning ESRCH and
+        // the unload, this process can be descheduled long enough for the planned
+        // module to vanish, a new host to inherit both the pid and the module index,
+        // and its differently-nonced arguments to occupy that index. Nothing would
+        // re-read those arguments, so the reused index gets unloaded.
+        //
+        // Asking liveness first and re-verifying the fingerprint after it means a
+        // replacement arriving in that window is caught by the argument comparison,
+        // and only the irreducible snapshot-to-unload interval remains.
+        match attributed_liveness(&liveness, fp) {
+            Liveness::Dead => {}
+            Liveness::Alive => {
+                println!(
+                    "[pixelpass] --repair: pid {} is alive again; leaving module #{} alone",
+                    fp.pid, fp.id
+                );
+                skipped += 1;
+                continue;
+            }
+            Liveness::Unknown => {
+                eprintln!(
+                    "[pixelpass] --repair: pid {}'s liveness became undeterminable; \
+                     leaving module #{} alone",
+                    fp.pid, fp.id
+                );
+                skipped += 1;
+                continue;
+            }
+        }
+
+        // Fresh snapshot per action, taken after the liveness answer. Deliberately
+        // not hoisted out of the loop: each unload changes the module table, and the
+        // point is to decide against the table as it is *now*.
         let current = pulse
             .list_modules()
             .context("could not re-list Pulse modules")?;
@@ -189,29 +225,6 @@ pub async fn run(clean_untagged: bool) -> Result<()> {
             skipped += 1;
             continue;
         }
-        // Liveness last, and closest to the unload: a host that came back — or
-        // a stranger that inherited the pid — outranks any amount of evidence
-        // that this module looked orphaned. Undecidable counts as back.
-        match liveness.of(fp.pid) {
-            Liveness::Dead => {}
-            Liveness::Alive => {
-                println!(
-                    "[pixelpass] --repair: pid {} is alive again; leaving module #{} alone",
-                    fp.pid, fp.id
-                );
-                skipped += 1;
-                continue;
-            }
-            Liveness::Unknown => {
-                eprintln!(
-                    "[pixelpass] --repair: pid {}'s liveness became undeterminable; \
-                     leaving module #{} alone",
-                    fp.pid, fp.id
-                );
-                skipped += 1;
-                continue;
-            }
-        }
 
         match pulse.unload_module(fp.id) {
             Ok(()) => {
@@ -259,6 +272,34 @@ fn describe(fp: &Fingerprint) -> String {
     )
 }
 
+/// Ask liveness with the module's *attribution* in hand.
+///
+/// A module whose token matches this machine, boot and pid namespace has already
+/// proven that its pid is a number meaningful here — that is the token's entire
+/// job. Running such a module through the probe's degradation checks would defeat
+/// it in exactly the situation it exists for: a host crashing inside a container
+/// leaves a token that matches perfectly, while a container marker or a multi-entry
+/// `NSpid` makes the probe answer `Unknown` for everything, so token-qualified
+/// repair would do nothing precisely where it is now safe.
+///
+/// The degradation signals therefore guard only the *untagged* path, where a bare
+/// pid is all there is and those signals are the only protection left.
+fn liveness_for(probe: &LivenessProbe, attribution: plan::Attribution, pid: u32) -> Liveness {
+    match attribution {
+        plan::Attribution::Tokened => probe.of_attributed(pid),
+        plan::Attribution::Untagged => probe.of(pid),
+    }
+}
+
+/// The same rule, for a fingerprint at execution time.
+fn attributed_liveness(probe: &LivenessProbe, fp: &Fingerprint) -> Liveness {
+    let attribution = match fp.owner {
+        Some(_) => plan::Attribution::Tokened,
+        None => plan::Attribution::Untagged,
+    };
+    liveness_for(probe, attribution, fp.pid)
+}
+
 // ──────────────────────────────────────────────────────────────────────
 // Identity
 // ──────────────────────────────────────────────────────────────────────
@@ -400,10 +441,21 @@ impl LivenessProbe {
         self.degraded.as_deref()
     }
 
+    /// Liveness for a pid this process has **no** independent reason to trust —
+    /// an untagged module. Here the degradation signals are the only protection.
     fn of(&self, pid: u32) -> Liveness {
         if self.degraded.is_some() {
             return Liveness::Unknown;
         }
+        self.of_attributed(pid)
+    }
+
+    /// Liveness for a pid already proven to belong to this machine, boot and pid
+    /// namespace by an [`plan::OwnerToken`].
+    ///
+    /// The degradation checks are deliberately skipped: they exist to guess at
+    /// whether a bare pid is meaningful, and here that is not a guess any more.
+    fn of_attributed(&self, pid: u32) -> Liveness {
         // `kill(0, …)` signals our whole process group and a negative pid signals
         // another group, so neither may ever reach `kill`. Neither is a pid we
         // could have written into a sink name anyway.
@@ -455,4 +507,42 @@ mod tests {
         assert_eq!(probe.of(0), Liveness::Unknown);
         assert_eq!(probe.of(u32::MAX), Liveness::Unknown);
     }
+
+    /// A token proves the pid is meaningful here, so the probe's namespace
+    /// guesswork must not veto it. Without this, a host crashing inside a
+    /// container leaves a perfectly matching token while a container marker makes
+    /// every answer `Unknown` — and token-qualified repair does nothing in exactly
+    /// the situation the token was built for.
+    ///
+    /// This runs against a *degraded* probe deliberately: on an ordinary desktop
+    /// the two paths agree, so a test using the real probe's state would pass
+    /// whether or not the distinction exists.
+    #[test]
+    fn a_token_beats_the_degradation_signals_but_a_bare_pid_does_not() {
+        let degraded = LivenessProbe {
+            degraded: Some("pretending to be in a container".to_string()),
+        };
+        let me = std::process::id();
+
+        assert_eq!(
+            degraded.of_attributed(me),
+            Liveness::Alive,
+            "an attributed pid must still be answered when the probe is degraded"
+        );
+        assert_eq!(
+            degraded.of(me),
+            Liveness::Unknown,
+            "a bare pid must not be, since the signals are all it has"
+        );
+
+        // And the routing between them, which is what the caller actually uses.
+        assert_eq!(
+            liveness_for(&degraded, plan::Attribution::Tokened, me),
+            Liveness::Alive
+        );
+        assert_eq!(
+            liveness_for(&degraded, plan::Attribution::Untagged, me),
+            Liveness::Unknown
+        );
+    }
 }

src/repair/plan.rs

@@ -279,6 +279,20 @@ impl LocalIdentity {
     }
 }
 
+/// Whether a module proved which pid space its pid belongs to.
+///
+/// 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)]
+pub enum Attribution {
+    /// Carries a token matching this machine, boot and pid namespace.
+    Tokened,
+    /// Carries no token; judged by pid alone, and only under
+    /// [`UntaggedPolicy::CleanByPidAlone`].
+    Untagged,
+}
+
 /// What repair may do about modules that carry no token at all.
 ///
 /// Every module loaded before tokens existed is untagged, and there is no way to
@@ -625,7 +639,7 @@ pub fn sink_still_referenced(
 pub fn plan(
     observations: &[ModuleObservation],
     policy: &Policy,
-    liveness: impl Fn(u32) -> Liveness,
+    liveness: impl Fn(u32, Attribution) -> Liveness,
 ) -> Plan {
     // Deduplicate by module id: a snapshot should not repeat one, but a repeated
     // entry must not become a repeated unload of an index that has since been
@@ -642,13 +656,15 @@ pub fn plan(
     // one cannot see, and an absent token names nothing at all.
     let mut untagged: Vec<Fingerprint> = Vec::new();
     let mut foreign: Vec<Fingerprint> = Vec::new();
-    let mut judgeable: Vec<Fingerprint> = Vec::new();
+    let mut judgeable: Vec<(Fingerprint, Attribution)> = Vec::new();
     for fp in seen.into_values() {
         match &fp.owner {
-            Some(token) if policy.local.can_judge(token) => judgeable.push(fp),
+            Some(token) if policy.local.can_judge(token) => {
+                judgeable.push((fp, Attribution::Tokened))
+            }
             Some(_) => foreign.push(fp),
             None => match policy.untagged {
-                UntaggedPolicy::CleanByPidAlone => judgeable.push(fp),
+                UntaggedPolicy::CleanByPidAlone => judgeable.push((fp, Attribution::Untagged)),
                 UntaggedPolicy::Refuse => untagged.push(fp),
             },
         }
@@ -659,8 +675,12 @@ pub fn plan(
     let mut live_pids = BTreeSet::new();
     let mut dead_pids = BTreeSet::new();
     let mut unknown_pids = BTreeSet::new();
-    for pid in judgeable.iter().map(|fp| fp.pid).collect::<BTreeSet<_>>() {
+    for (pid, attribution) in judgeable
-        match liveness(pid) {
+        .iter()
+        .map(|(fp, attribution)| (fp.pid, *attribution))
+        .collect::<BTreeMap<_, _>>()
+    {
+        match liveness(pid, attribution) {
             Liveness::Alive => live_pids.insert(pid),
             Liveness::Dead => dead_pids.insert(pid),
             Liveness::Unknown => unknown_pids.insert(pid),
@@ -669,6 +689,7 @@ pub fn plan(
 
     let mut unload: Vec<Fingerprint> = judgeable
         .into_iter()
+        .map(|(fp, _)| fp)
         .filter(|fp| dead_pids.contains(&fp.pid))
         .collect();
     // `Shape`'s declaration order is the unload order: loopbacks before the sink
@@ -744,7 +765,7 @@ mod tests {
         obs(id, Shape::LoopbackIntoCapture, pid, None)
     }
 
-    fn nothing_is_alive(_: u32) -> Liveness {
+    fn nothing_is_alive(_: u32, _: Attribution) -> Liveness {
         Liveness::Dead
     }
 
@@ -885,7 +906,7 @@ mod tests {
             null_sink(6, 200),
             mirror(12, 200),
         ];
-        let plan = plan(&modules, &our_policy(), |pid| {
+        let plan = plan(&modules, &our_policy(), |pid, _| {
             if pid == 200 {
                 Liveness::Alive
             } else {
@@ -903,7 +924,7 @@ mod tests {
     #[test]
     fn an_undecidable_pid_is_never_touched() {
         let modules = [null_sink(5, 100), mirror(10, 100), local_monitor(11, 200)];
-        let plan = plan(&modules, &our_policy(), |pid| {
+        let plan = plan(&modules, &our_policy(), |pid, _| {
             if pid == 100 {
                 Liveness::Unknown
             } else {
@@ -934,7 +955,7 @@ mod tests {
             null_sink(6, 200),
             mirror(12, 200),
         ];
-        let plan = plan(&modules, &our_policy(), |_| Liveness::Alive);
+        let plan = plan(&modules, &our_policy(), |_, _| Liveness::Alive);
         assert!(plan.is_empty(), "no live host may be touched: {plan:?}");
         assert_eq!(plan.live_pids, BTreeSet::from([100, 200]));
         assert!(plan.dead_pids.is_empty());
@@ -1049,7 +1070,7 @@ mod tests {
             mirror(12, 99),
             local_monitor(13, 99),
         ];
-        let plan = plan(&modules, &our_policy(), |pid| {
+        let plan = plan(&modules, &our_policy(), |pid, _| {
             *calls.borrow_mut().entry(pid).or_insert(0) += 1;
             Liveness::Dead
         });
@@ -1240,7 +1261,7 @@ mod tests {
             legacy_mirror(11, 5555),
         ];
         let asked = std::cell::RefCell::new(Vec::new());
-        let plan = plan(&modules, &our_policy(), |pid| {
+        let plan = plan(&modules, &our_policy(), |pid, _| {
             asked.borrow_mut().push(pid);
             Liveness::Dead
         });
@@ -1288,7 +1309,7 @@ mod tests {
                 local: our_identity(),
                 untagged: UntaggedPolicy::CleanByPidAlone,
             },
-            |_| Liveness::Alive,
+            |_, _| Liveness::Alive,
         );
         assert!(live.is_empty(), "{live:?}");
     }
@@ -1309,6 +1330,35 @@ mod tests {
         );
     }
 
+    /// The attribution handed to the liveness callback decides *how* the question
+    /// may be asked, so it must be right per module. Getting this wrong made the
+    /// token useless in a container — the one place it exists for — because the
+    /// probe's namespace guesswork answered `Unknown` for a pid the token had
+    /// already proven local.
+    #[test]
+    fn attribution_is_reported_per_module() {
+        let modules = [mirror(10, 100), legacy_mirror(11, 200)];
+        let asked = std::cell::RefCell::new(Vec::new());
+        let plan = plan(
+            &modules,
+            &Policy {
+                local: our_identity(),
+                untagged: UntaggedPolicy::CleanByPidAlone,
+            },
+            |pid, attribution| {
+                asked.borrow_mut().push((pid, attribution));
+                Liveness::Dead
+            },
+        );
+        let asked = asked.into_inner();
+        assert_eq!(
+            asked,
+            vec![(100, Attribution::Tokened), (200, Attribution::Untagged)],
+            "each pid must be asked about with its own module's attribution"
+        );
+        assert_eq!(ids(&plan), vec![10, 11]);
+    }
+
     /// Tokened and legacy forms must both classify, and carry the difference.
     #[test]
     fn both_forms_classify_and_record_whether_they_are_attributable() {