Codex adversarial review found no merge-blocking defects. Five worth-checking items, all triaged for reachability: - F1 (spurious pre-ready revoke): unreachable — the AEC's four nodes are two Stream/* legs + a null-sink-like virtual sink/source, none claiming a device.id, so the phase-3 observer never withholds them; index_present goes false only on a genuine full unload. Documented why revoke is NOT gated on graph_ready, and why gating it would reopen the reused-index alias trap (F4) during a hot-reload-under-churn. Pinned with revokes_on_empty_even_while_not_ready (mutation-verified: `&& graph_ready` on the revoke guard dies here). - F4 (test relies on observing the empty gap): documented the phase-5/6 integration contract it rests on (one observe per graph event, no coalescing across a module lifetime boundary) and owed the robust fix (serial-continuity / observer-generation) to a later hardening round. - F2 (late positive evidence beats the deadline): intentional and correct — a demonstrably-present identity is ground truth. Documented + late_positive_evidence_wins_over_expired_deadline (both arms: node-first validates, Tick-first fails closed and stays sticky). - F3 (real P3 coverage hole): strengthened deadline_is_not_armed_until_ graph_ready to prove the budget starts at first-ready, not construction (mutation-verified: a construction-relative deadline now dies). - F5 (`+7` grammar mismatch): documented the producer contract — peerspeak emits bare decimal (pactl returns unsigned decimal), the narrow parser is deliberate. Unreachable on the measured stack. No core logic change. 24 pure aec tests, cargo test --bins green (145 unit + 1 ignored live), clippy -D warnings + fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
312 lines
16 KiB
Rust
312 lines
16 KiB
Rust
//! Phase 4 — the AEC identity validation state machine (impl plan §4, design
|
|
//! v3.4 §5.2/§5.3).
|
|
//!
|
|
//! peerspeak's echo canceller (`module-echo-cancel`) creates four graph nodes
|
|
//! that all carry `pulse.module.id == <the index pactl returned>`, and the
|
|
//! playback leg among them is a `Stream/Output/Audio` node wired straight to
|
|
//! the speakers — a fan-out candidate that would copy the whole remote call
|
|
//! into the share unless it is excluded (v3.4 §5.2, measured ≈desktop level).
|
|
//! The taint engine (phase 2) already excludes it *given* the module index in
|
|
//! [`ExclusionCtx::aec_module_id`](crate::host::taint::ExclusionCtx); this
|
|
//! module is what decides, at runtime and fail-closed, whether that index may
|
|
//! be trusted and handed over.
|
|
//!
|
|
//! **Why a state machine and not a one-shot check (v3.4 §5.3).** The identity
|
|
//! is an *observed correlation on PipeWire 1.6.8*, not a documented contract,
|
|
//! and a start-time enumeration races in both directions: peerspeak's
|
|
//! `enable()` returns before the playback hazard leg is even in the graph, and
|
|
//! pixelpass's capture spawns lazily on the first viewer, at a moment peerspeak
|
|
//! does not control. So validation is a bounded epoch, and the identity can be
|
|
//! *lost* mid-share (the module unloads) as well as *gained*.
|
|
//!
|
|
//! **The two traps this is shaped around:**
|
|
//!
|
|
//! - **Revocation is loss of the whole module identity, not one leg corking**
|
|
//! (v3.4 §5.3). Each [`AecValidator::observe`] rescans the snapshot for *any*
|
|
//! node bearing the index; [`AecState::Validated`] drops to
|
|
//! [`AecState::Revoked`] only when that set becomes **empty**. A single leg
|
|
//! corking or relinking (still ≥1 present) stays `Validated` — getting this
|
|
//! wrong turns a normal cork into a spurious share-wide audio stop.
|
|
//! - **Module indices are reused verbatim across unload/reload** (v3.4 §5.2
|
|
//! correction 3 — both a reload's module index *and* its `node.link-group`
|
|
//! came back byte-identical, and node ids were recycled *and reassigned
|
|
//! across legs*). So [`AecState::Failed`] and [`AecState::Revoked`] are
|
|
//! **sticky terminal**: a later node reappearing with the same index does
|
|
//! **not** un-revoke and alias onto the new module. A genuine reload gets a
|
|
//! *fresh* [`AecValidator`] (peerspeak re-tells pixelpass the index on every
|
|
//! load), never a resurrected one.
|
|
//!
|
|
//! **Scope.** This is the validation state machine + `--aec` parsing only.
|
|
//! Foreign / second-AEC detection (a non-owned `echo-cancel-*` group, v3.4
|
|
//! §5.4 / D3) and the `foreign_aec_warning`/`aec_failed`/`aec_revoked` status
|
|
//! *events* are phase 6's, which reads this machine's [`AecState`]. Wiring the
|
|
//! parsed [`AecConfig`] out of the CLI and calling [`AecValidator::observe`]
|
|
//! in the recompute loop is integration (phases 5/8). The node-side
|
|
//! `pulse.module.id` parse (JSON-number-vs-string, u64-not-u32) is phase 3's
|
|
//! adapter; this module consumes the already-parsed
|
|
//! [`NodeProps::pulse_module_id`](crate::host::taint::snapshot::NodeProps).
|
|
|
|
#![allow(dead_code)] // Wired into `--aec` parsing + the recompute loop by later phases.
|
|
|
|
#[cfg(test)]
|
|
mod tests;
|
|
|
|
use crate::host::observer::Millis;
|
|
use crate::host::taint::snapshot::GraphSnapshot;
|
|
|
|
/// The parsed `--aec=off|pulse-module:<idx>` argument (decision D5).
|
|
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
|
pub enum AecConfig {
|
|
/// `--aec=off` — peerspeak's AEC is not in play, so there is nothing to
|
|
/// exclude and fan-out proceeds with no AEC identity. Not the same as an
|
|
/// *absent* argument (that default is the caller's; see [`parse_aec_arg`]).
|
|
Off,
|
|
/// `--aec=pulse-module:<idx>` — validate this live module index before
|
|
/// trusting it. The index is compared as `u64`, never `u32` (v3.4 §5.2).
|
|
PulseModule(u64),
|
|
}
|
|
|
|
/// Why an `--aec` argument was rejected. Rejection is fatal at the CLI edge —
|
|
/// there is no fail-closed *default* index, because a wrong index would exclude
|
|
/// the wrong node (or nothing), so a malformed value must not silently become
|
|
/// "no AEC".
|
|
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
|
pub enum AecParseError {
|
|
/// The value was empty.
|
|
Empty,
|
|
/// Not `off` and not `pulse-module:...`.
|
|
UnknownForm,
|
|
/// `pulse-module:` with nothing after the colon.
|
|
MissingIndex,
|
|
/// The index was not a bare `u64` decimal (sign, whitespace, non-digit, or
|
|
/// `> u64::MAX`).
|
|
InvalidIndex,
|
|
}
|
|
|
|
/// Parse one `--aec` value. `off` and `pulse-module:<idx>` are the only forms.
|
|
///
|
|
/// The index accepts values `> u32::MAX` (v3.4 §5.2: `pulse.module.id` sits
|
|
/// next to the `object.serial` u32-truncation bug, so it is only ever compared
|
|
/// as `u64`) and requires a **bare decimal** — stricter than Rust's [`u64`]
|
|
/// parser, which also accepts a leading `+`. Rejected: any sign, surrounding or
|
|
/// interior whitespace, non-decimal digits, and overflow. Matching is exact and
|
|
/// case-sensitive: the argument is machine-generated by peerspeak from
|
|
/// `EchoCancelGuard::module_index`, not typed by a user.
|
|
///
|
|
/// ⚠️ **Producer contract** (Codex phase-4 review, finding 5): because the
|
|
/// grammar is narrower than Rust's parser, peerspeak must emit a bare decimal.
|
|
/// `pactl load-module` returns an unsigned decimal, so the stored index is
|
|
/// already canonical and no reachable value is rejected; if peerspeak ever
|
|
/// changes how it formats the index it must canonicalize (`value.to_string()`),
|
|
/// not widen this parser — the narrow grammar is the point.
|
|
pub fn parse_aec_arg(value: &str) -> Result<AecConfig, AecParseError> {
|
|
if value.is_empty() {
|
|
return Err(AecParseError::Empty);
|
|
}
|
|
if value == "off" {
|
|
return Ok(AecConfig::Off);
|
|
}
|
|
if let Some(index) = value.strip_prefix("pulse-module:") {
|
|
if index.is_empty() {
|
|
return Err(AecParseError::MissingIndex);
|
|
}
|
|
// A bare decimal only: reject a leading sign (Rust's `u64` parser
|
|
// accepts `+7`), interior/surrounding whitespace, and any non-digit,
|
|
// before letting the parser catch overflow. Leading zeros are harmless.
|
|
if !index.bytes().all(|b| b.is_ascii_digit()) {
|
|
return Err(AecParseError::InvalidIndex);
|
|
}
|
|
return index
|
|
.parse::<u64>()
|
|
.map(AecConfig::PulseModule)
|
|
.map_err(|_| AecParseError::InvalidIndex);
|
|
}
|
|
Err(AecParseError::UnknownForm)
|
|
}
|
|
|
|
/// The validation epoch (v3.4 §5.3, verbatim).
|
|
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
|
pub enum AecState {
|
|
/// `--aec=off` — no AEC identity, fan-out proceeds with no exclusion.
|
|
/// Terminal.
|
|
NotConfigured,
|
|
/// Waiting for the first node bearing the index. **No fan-out occurs here**
|
|
/// — silence is the safe direction. Ends at `Validated` on first sight, or
|
|
/// `Failed` once the graph is fully enumerated and the bounded deadline
|
|
/// passes with the index never seen.
|
|
Validating,
|
|
/// The index was observed live. Fan-out is permitted, excluding that
|
|
/// identity transitively (phase 2 / v3.4 §6.1).
|
|
Validated,
|
|
/// The deadline expired with the index never observed. **Fail closed** — no
|
|
/// fan-out; the caller reports a capability failure rather than sharing.
|
|
/// Sticky terminal.
|
|
Failed,
|
|
/// The whole module identity disappeared mid-share (every node bearing the
|
|
/// index gone). **Stop fan-out now** and drop the owned link proxies; do
|
|
/// not keep the numeric index and hope, because it is reused. Sticky
|
|
/// terminal — see the module header's second trap.
|
|
Revoked,
|
|
}
|
|
|
|
/// The bounded, read-only AEC identity validator. Fold the live graph in with
|
|
/// [`AecValidator::observe`] once per recompute; read the result with
|
|
/// [`AecValidator::state`], [`AecValidator::fan_out_permitted`], and
|
|
/// [`AecValidator::validated_module_id`].
|
|
#[derive(Clone, Debug)]
|
|
pub struct AecValidator {
|
|
/// The index to validate. `None` iff [`AecConfig::Off`] (state stays
|
|
/// [`AecState::NotConfigured`] forever).
|
|
target: Option<u64>,
|
|
state: AecState,
|
|
/// The `Validating → Failed` budget, applied *after* the deadline is armed.
|
|
timeout: Millis,
|
|
/// The absolute `Failed` deadline, armed the first time the graph reports
|
|
/// ready (the "registry sync barrier" of v3.4 §5.3) and never re-armed —
|
|
/// `graph_ready` is dynamic and can flap, but the epoch budget must not
|
|
/// restart. `None` until then: while the initial enumeration is still in
|
|
/// flight, a not-yet-seen index is *unknown*, not *absent*, so it must not
|
|
/// time out to `Failed`.
|
|
deadline: Option<Millis>,
|
|
}
|
|
|
|
impl AecValidator {
|
|
/// `timeout` is the `Validating → Failed` budget, counted from the moment
|
|
/// the graph first becomes ready (not from construction). An `Off` config
|
|
/// starts (and stays) [`AecState::NotConfigured`].
|
|
pub fn new(config: AecConfig, timeout: Millis) -> Self {
|
|
match config {
|
|
AecConfig::Off => Self {
|
|
target: None,
|
|
state: AecState::NotConfigured,
|
|
timeout,
|
|
deadline: None,
|
|
},
|
|
AecConfig::PulseModule(index) => Self {
|
|
target: Some(index),
|
|
state: AecState::Validating,
|
|
timeout,
|
|
deadline: None,
|
|
},
|
|
}
|
|
}
|
|
|
|
pub fn state(&self) -> AecState {
|
|
self.state
|
|
}
|
|
|
|
/// The validated index to place in
|
|
/// [`ExclusionCtx::aec_module_id`](crate::host::taint::ExclusionCtx) —
|
|
/// `Some` **only** in [`AecState::Validated`]. `None` everywhere else,
|
|
/// including `NotConfigured` (no AEC ⇒ nothing to exclude) and the
|
|
/// fail-closed states (whose `None` must be paired with
|
|
/// [`Self::fan_out_permitted`] `== false`, i.e. no fan-out at all — *not*
|
|
/// a fan-out that merely skips AEC exclusion).
|
|
pub fn validated_module_id(&self) -> Option<u64> {
|
|
match self.state {
|
|
AecState::Validated => self.target,
|
|
_ => None,
|
|
}
|
|
}
|
|
|
|
/// Whether fan-out may proceed at all right now. True only in
|
|
/// [`AecState::NotConfigured`] (fan out, no exclusion) and
|
|
/// [`AecState::Validated`] (fan out, excluding the identity). `Validating`,
|
|
/// `Failed` and `Revoked` all forbid it — silence over echo.
|
|
pub fn fan_out_permitted(&self) -> bool {
|
|
matches!(self.state, AecState::NotConfigured | AecState::Validated)
|
|
}
|
|
|
|
/// Fold one recompute's view of the graph into the machine.
|
|
///
|
|
/// `graph_ready` is the observer's dynamic readiness
|
|
/// ([`Projection::graph_ready`](crate::host::observer::Projection)); `now`
|
|
/// is a monotonic millisecond clock. Positive evidence (a node bearing the
|
|
/// index) is authoritative and validates regardless of `graph_ready` —
|
|
/// seeing the node *is* seeing it — but the `Failed` deadline only begins
|
|
/// once `graph_ready` has first become true, so a slow initial enumeration
|
|
/// can never masquerade as a genuinely-absent module.
|
|
pub fn observe(&mut self, snapshot: &GraphSnapshot, graph_ready: bool, now: Millis) {
|
|
// `Off` (NotConfigured) and both sticky terminals are no-ops: there is
|
|
// nothing to look for, and a reappearing reused index must not revive a
|
|
// Failed/Revoked epoch (v3.4 §5.2 correction 3).
|
|
let Some(target) = self.target else {
|
|
return;
|
|
};
|
|
match self.state {
|
|
AecState::Validating => {
|
|
// Presence is checked *before* the deadline on purpose: a
|
|
// demonstrably-present identity validates regardless of the
|
|
// clock, even if the node is first seen just past the deadline
|
|
// (Codex phase-4 review, finding 2). The deadline only bounds
|
|
// the wait for an identity that is never seen — seeing it, late
|
|
// or not, is ground truth that the module exists, and excluding
|
|
// a real echo leg is always the safe answer. (A `Failed` can
|
|
// still pre-empt this when a `Tick`-only observation crosses the
|
|
// deadline first; that only makes the machine *more* fail-closed,
|
|
// never less.)
|
|
if self.index_present(snapshot, target) {
|
|
self.state = AecState::Validated;
|
|
return;
|
|
}
|
|
// Arm the deadline once, on the first ready graph.
|
|
if self.deadline.is_none() && graph_ready {
|
|
self.deadline = Some(now.saturating_add(self.timeout));
|
|
}
|
|
if self.deadline.is_some_and(|deadline| now >= deadline) {
|
|
self.state = AecState::Failed;
|
|
}
|
|
}
|
|
AecState::Validated => {
|
|
// Revocation is the whole identity gone (no node bears the
|
|
// index), not one leg corking — see the module header.
|
|
//
|
|
// ⚠️ **Deliberately NOT gated on `graph_ready`** (Codex
|
|
// phase-4 review, findings 1 + 4). Two forces pull opposite
|
|
// ways and this is the resolution:
|
|
//
|
|
// - Gating revoke on readiness would avoid a *spurious* revoke
|
|
// from a transient empty snapshot seen while the module is
|
|
// still live. But for the AEC that transient does not exist:
|
|
// its four nodes are two `Stream/*` legs plus a null-sink-like
|
|
// virtual sink/source, none of which claim a `device.id`, so
|
|
// the phase-3 observer never *withholds* them
|
|
// (`observer::classify` withholds only device-claiming nodes).
|
|
// `index_present` therefore goes false only on a genuine
|
|
// `global_remove` of every leg — a real unload — and a real
|
|
// unload *should* revoke.
|
|
// - Worse, gating on readiness would REOPEN the reused-index
|
|
// alias trap: if an unload+reload (indices recycle, §5.2
|
|
// correction 3) both complete inside one not-ready churn
|
|
// window, the ready snapshot would already show the *new*
|
|
// module's node and we would never observe the empty gap —
|
|
// silently aliasing onto an unrelated module. Revoking the
|
|
// instant the gap appears, ready or not, is what closes it.
|
|
//
|
|
// This correctness rests on the phase-5/6 integration contract:
|
|
// **one `observe` per graph event, no coalescing across a module
|
|
// lifetime boundary.** Under coalescing, the empty gap between an
|
|
// old unload and a reused-index reload can be skipped. The
|
|
// robust fix that would not depend on that contract is a
|
|
// serial-continuity / observer-generation signal (the AEC nodes'
|
|
// `object.serial`s are fresh across a reload even when the index
|
|
// is not) — owed to a later hardening round, not built here.
|
|
if !self.index_present(snapshot, target) {
|
|
self.state = AecState::Revoked;
|
|
}
|
|
}
|
|
AecState::NotConfigured | AecState::Failed | AecState::Revoked => {}
|
|
}
|
|
}
|
|
|
|
/// Whether any node in the snapshot bears the target module index. The same
|
|
/// exact-`u64`-equality predicate the taint engine roots on
|
|
/// (`taint/mod.rs`), kept here so "is the identity live?" has one
|
|
/// definition.
|
|
fn index_present(&self, snapshot: &GraphSnapshot, target: u64) -> bool {
|
|
snapshot
|
|
.nodes()
|
|
.any(|node| node.props.pulse_module_id == Some(target))
|
|
}
|
|
}
|