diff --git a/agent_docs/observability.md b/agent_docs/observability.md index 3fce53d77..316b4da3f 100644 --- a/agent_docs/observability.md +++ b/agent_docs/observability.md @@ -21,7 +21,7 @@ must follow. - **No PII.** Snapshots and reports carry numbers only, never JIDs/phone numbers, matching the `wacore::telemetry` label rules. -## The three surfaces +## The four surfaces ### 1. `Client::stats()` — wire I/O counters (always on) @@ -104,7 +104,61 @@ resource teardown panics, publication failures, and queue drops mark only the responsible plugin as degraded. Concurrent snapshots are intentionally approximate, and carry no message content, JIDs, or phone numbers. -### 3. `BotBuilder::with_task_instrument` — CPU / custom attribution (opt-in) +### 3. `Client::device_memo_stats()` — group-path memo outcomes (always on) + +`DeviceMemoStats`: per-term hit/miss counts for the two device-list memos a +group send depends on, `resolve_group_devices_memoized` and +`resolve_skdm_targets_memoized`. Cumulative for the client's lifetime; +`DeviceMemoStats::since` subtracts an earlier snapshot to scope a workload. + +The reason it is per-term rather than a hit/miss pair: the group memo has three +validity terms (entry present, `GroupInfo` `Arc` identity, topology generation +— with a scoped re-stamp between the last two) and the SKDM memo has four stale +terms (device `Arc`, sender-key-map `Arc`, map generation, sending identity) +plus the entry-absent condition, which is why it reports five miss counters. An +aggregate "N misses" cannot separate an in-place cold flip from a metadata +refresh from a memo that was never stored, and those have different fixes. It +also cannot separate cause from consequence: the SKDM memo compares the `Arc` +that the group memo returned, so **a group-memo recompute forces +`skdm_targets.miss_devices` no matter what**. Read the group half first. + +Two counters do not fit the "one per call" shape and are documented as such: +`restamps` (served like a hit, but paid the `unchanged_for` scan first) and +`not_stored` (a resolution whose target set was neither empty nor +own-devices-only, so nothing was memoized). `not_stored` guarantees the next +call **cannot hit** — not that it reports `miss_absent`. A stale entry that was +already there is deliberately left in place, because it can never become valid +again (the sender-key map generation only moves forward, the map `Arc` is +replaced wholesale on a rebuild, and the device-set `Weak` keeps the old +allocation alive so no `ptr::eq` can spuriously match), so the next call +reports whichever term is still failing. Reading a run of `not_stored` as +eviction pressure is therefore the wrong conclusion: it means the group is not +settling into the warm steady state at all. + +Every other SKDM outcome is exactly one per call, including `resolve_failed`, +which covers the calls that never reached a memo term because the device +resolution they depend on errored. Without it `hit_rate()` would look healthy +over a denominator that quietly shrank as sends started failing. + +Why always-on rather than `#[cfg(test)]` like `dm_devices_memo_recomputes`: a +test counter answers the question in a fixture, and the question here is what a +*deployed* client gets — an embedder whose registry writes are noisier than any +fixture's would have no way to see its own hit rate. It costs one indexed +relaxed `fetch_add` per resolver call, twice per group send. Measured against +the tightest thing the counters sit inside (SKDM target resolution on the +memo-hit path, callgrind, min of 3, K=10001 so the fixture's setup jitter +divides away): **+16 Ir per resolve at 8 members, +25 at 512**, against 4,419 +and 4,408 without them. At whole-send scale it is under the fixture's own +run-to-run spread. + +Record the outcome **on the branch that decided it**. An earlier revision +classified into an enum and then matched on it again to act; that second +dispatch, plus moving the SKDM memo entry (a five-field tuple carrying a `Jid` +and a `Vec`) into a temporary to classify it, cost 126 Ir per resolve +instead of 16. A counter meant to be free on the hit path has to be written +that way. + +### 4. `BotBuilder::with_task_instrument` — CPU / custom attribution (opt-in) `wacore::stats::TaskInstrument` is an object-safe enter/exit hook called around every poll of the client's internal tasks and around its blocking diff --git a/benches/client_group_send.rs b/benches/client_group_send.rs index f14cd4544..99200be75 100644 --- a/benches/client_group_send.rs +++ b/benches/client_group_send.rs @@ -17,7 +17,10 @@ //! - **LID addressing.** The fixture is PN-addressed, so //! `GroupInfo::phone_jid_for_lid_user` is never reached. A LID sweep needs a //! registry seeded with LID↔PN mappings, which is a different fixture rather -//! than a parameter on this one. +//! than a parameter on this one. The *hit-rate* question is covered for LID +//! groups by `repeat_lid_group_sends_hit_both_device_memos_on_every_send` +//! in `src/send/mod.rs`, which is what settles whether that function is on +//! a warm send's bill at all — it runs only inside the uncached resolve. //! - **The SQLite backend.** The fixture stores through `InMemoryBackend`, so //! the storage engine's own per-send cost is excluded by construction. //! - **First contact.** The fixture reaches its steady state before measuring, @@ -120,9 +123,12 @@ fn skdm_target_resolution_warm(bencher: divan::Bencher, group_size: usize) { /// `skdm_target_resolution_warm` is what the memo is worth per send. Measuring /// both is what separates "the filter is expensive" from "the filter runs when /// it should not" — only the second is a bug, and only a hit-rate observation -/// can tell them apart. That the steady state takes the memoized path is pinned -/// as a test (`skdm_warm_memo_hits_on_every_repeat_send`), not asserted here: a -/// benchmark measures how much it costs, not how often it happens. +/// can tell them apart. Which one the steady state takes is not asserted here +/// (a benchmark measures how much an outcome costs, not how often it happens): +/// it is pinned by `skdm_warm_memo_hits_on_every_repeat_send` and, per term, +/// by `repeat_group_sends_hit_both_device_memos_on_every_send`. At group sizes +/// no unit test builds, read `GroupSendHarness::memo_stats` after a run of +/// `warm_send`. #[divan::bench(args = GROUP_SIZES, sample_count = SAMPLE_COUNT, sample_size = SAMPLE_SIZE)] fn skdm_target_resolution_memo_cold(bencher: divan::Bencher, group_size: usize) { let harness = shared("skdm_target_resolution_memo_cold", group_size); diff --git a/src/bench_support.rs b/src/bench_support.rs index 8119bce6c..c8e6e3819 100644 --- a/src/bench_support.rs +++ b/src/bench_support.rs @@ -184,6 +184,17 @@ impl GroupSendHarness { .expect("target resolution must succeed offline") } + /// Per-term outcomes of the two device memos so far. + /// + /// The benchmarks below force their memo outcome, which is what makes them + /// measurable — and what makes them unable to say which outcome a client + /// takes once the group is in its steady state. Reading the counters after + /// a run of [`Self::warm_send`] is how the sweep answers that at group + /// sizes the unit tests do not reach. + pub fn memo_stats(&self) -> crate::client::DeviceMemoStats { + self.client.device_memo_stats() + } + /// A signal-cache flush over the cached session set this group produced. /// /// Repeated calls flush an already-clean cache, which is the interesting diff --git a/src/client.rs b/src/client.rs index 465d929b2..2ab30f2ff 100644 --- a/src/client.rs +++ b/src/client.rs @@ -6,6 +6,7 @@ pub(crate) use app_state::{BatchedSyncOutcome, BatchedSyncRequest, CriticalSyncP pub(crate) use app_state::{SyncHolder, batched_sync_outcome_tests::batch_result}; mod builder; mod context_impl; +mod device_memo_stats; mod device_registry; pub(crate) mod device_topology; #[cfg(feature = "client-lifecycle")] @@ -22,6 +23,10 @@ mod sessions; mod voip; use builder::{ClientAssembly, ClientExtensions}; pub use builder::{ClientBuild, ClientBuilder, ClientBuilderError}; +pub(crate) use device_memo_stats::{ + DeviceMemoCounters, GroupDevicesMemoOutcome, SkdmTargetsMemoOutcome, +}; +pub use device_memo_stats::{DeviceMemoStats, GroupDevicesMemoStats, SkdmTargetsMemoStats}; #[cfg(feature = "client-lifecycle")] use extension_lifecycle::LifecycleRegistration; #[cfg(feature = "client-lifecycle")] @@ -1566,6 +1571,13 @@ pub struct Client { #[cfg(test)] pub(crate) dm_devices_memo_recomputes: AtomicU64, + /// Per-term hit/miss counts for the two group-path device memos above, + /// read through [`Client::device_memo_stats`]. Which term invalidated is + /// the only thing that distinguishes "this memo is doing its job" from + /// "this memo has never hit", and no benchmark can observe it: a fixture + /// that forces the outcome measures the cost of the outcome it forced. + pub(crate) device_memo_counters: DeviceMemoCounters, + /// Single-flight for cold SKDM distribution, keyed per group. Concurrent /// cold sends each re-ran the full per-member fan-out before any of them /// marked the devices warm; the loser now waits here and re-resolves, diff --git a/src/client/device_memo_stats.rs b/src/client/device_memo_stats.rs new file mode 100644 index 000000000..e08e99908 --- /dev/null +++ b/src/client/device_memo_stats.rs @@ -0,0 +1,476 @@ +//! Per-term hit/miss accounting for the two device-list memos the group send +//! path depends on. +//! +//! Benchmarks answer *how much* a memo hit and a memo miss each cost +//! (`skdm_target_resolution_warm` vs `skdm_target_resolution_memo_cold`, PR +//! #1283). They cannot answer *which of the two a running client actually +//! gets*, because both of those fixtures force their outcome. That question +//! decides whether the miss-path cost is a real bill or a benchmark artifact, +//! and an aggregate "misses: N" would not answer it either: the group memo has +//! three validity terms and the SKDM memo four, so a miss count says something +//! is wrong without saying what. Every counter here therefore names the term +//! that decided the call. +//! +//! Exactly one counter is bumped per call to each resolver, so +//! [`GroupDevicesMemoStats::calls`] and [`SkdmTargetsMemoStats::calls`] are +//! sums, not separate counters that could drift from their parts. +//! +//! Always on, no feature gate, per `agent_docs/observability.md`: the increment +//! is one indexed relaxed `fetch_add` per resolver call — twice per group send, +//! against a send that signs with Ed25519 and writes a frame — and the +//! reporting types are dropped by LTO in a binary that never calls +//! [`Client::device_memo_stats`]. Measured cost is in the PR that added this. + +use portable_atomic::AtomicU64; +use std::sync::atomic::Ordering; + +use super::Client; + +/// Which term decided one `resolve_group_devices_memoized` call. +/// +/// `repr(usize)` and used as an index into [`DeviceMemoCounters`]'s array, so +/// recording one is a single indexed `fetch_add` with no branch on the variant. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[repr(usize)] +pub(crate) enum GroupDevicesMemoOutcome { + /// Entry present, `GroupInfo` identity matched, generation unchanged. + Hit, + /// Generation had moved, but every change since provably missed this + /// group's member set, so the entry was re-stamped instead of recomputed. + /// Served the same devices a hit would have, at the cost of the + /// `unchanged_for` scan. + Restamp, + /// No entry for this group: first send, or a capacity/TTL eviction. + MissAbsent, + /// An entry existed but was built from a different `Arc`. + /// Either the group metadata was genuinely refreshed, or a caller handed + /// the resolver an `Arc` that is not the cached one. + MissGroupInfo, + /// Generation moved and the change log could not prove the change missed + /// this group (a member was touched, or the log overflowed). + MissTopology, + /// The memo was not consulted: store-backed registry/mapping caches make + /// its freshness contract unenforceable, so it is disabled wholesale. + Bypassed, +} + +/// Which of `skdm_memo_entry_stale_term`'s terms decided one +/// `resolve_skdm_targets_memoized` call. Indexed like its group counterpart. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[repr(usize)] +pub(crate) enum SkdmTargetsMemoOutcome { + Hit, + /// No entry at all: first send for this group, or an eviction. + MissAbsent, + /// The resolved device-set `Arc` differs from the memoized one. This is + /// the cascade term: the devices come straight out of the group memo, so + /// a group-memo recompute forces this miss regardless of the other three. + MissDevices, + /// The sender-key device map was rebuilt (a warm-mark write invalidated it). + MissMap, + /// Same map `Arc`, advanced generation: an in-place cold flip, i.e. a + /// retry receipt's `markForgetSenderKey`. + MissMapGeneration, + /// A different sending identity (PN↔LID re-addressing, or a re-pair). + MissSender, + Bypassed, + /// The prerequisite device resolution failed, so no memo term was ever + /// evaluated. Recorded so that one call to `resolve_skdm_targets_memoized` + /// is always one counter: without it a client whose sends were failing + /// before the memo lookup would report a healthy hit rate over a + /// shrinking denominator. + ResolveFailed, +} + +const GROUP_OUTCOMES: usize = 6; +const SKDM_OUTCOMES: usize = 8; + +/// Array slot for one group-memo outcome. +/// +/// Exhaustive on purpose. `record_group_devices` indexes by `as usize`, which +/// is fast and unchecked-looking; what makes it safe is that adding a variant +/// *anywhere* — including after the current last one — makes this match +/// non-exhaustive and the crate stops compiling. A bounds assertion on the +/// last variant alone would keep passing and let the new variant index off the +/// end of the array on the send path, which has no other symptom. +const fn group_slot(outcome: GroupDevicesMemoOutcome) -> usize { + let slot = match outcome { + GroupDevicesMemoOutcome::Hit => 0, + GroupDevicesMemoOutcome::Restamp => 1, + GroupDevicesMemoOutcome::MissAbsent => 2, + GroupDevicesMemoOutcome::MissGroupInfo => 3, + GroupDevicesMemoOutcome::MissTopology => 4, + GroupDevicesMemoOutcome::Bypassed => 5, + }; + assert!(slot < GROUP_OUTCOMES); + slot +} + +/// Array slot for one SKDM-memo outcome. Same contract as [`group_slot`]. +const fn skdm_slot(outcome: SkdmTargetsMemoOutcome) -> usize { + let slot = match outcome { + SkdmTargetsMemoOutcome::Hit => 0, + SkdmTargetsMemoOutcome::MissAbsent => 1, + SkdmTargetsMemoOutcome::MissDevices => 2, + SkdmTargetsMemoOutcome::MissMap => 3, + SkdmTargetsMemoOutcome::MissMapGeneration => 4, + SkdmTargetsMemoOutcome::MissSender => 5, + SkdmTargetsMemoOutcome::Bypassed => 6, + SkdmTargetsMemoOutcome::ResolveFailed => 7, + }; + assert!(slot < SKDM_OUTCOMES); + slot +} + +/// Every variant indexes inside its array, and its slot agrees with the +/// discriminant the recorders use. Const-evaluated, so a mismatch is a build +/// failure rather than a panic on the first send. +const _: () = { + assert!(group_slot(GroupDevicesMemoOutcome::Hit) == GroupDevicesMemoOutcome::Hit as usize); + assert!( + group_slot(GroupDevicesMemoOutcome::Restamp) == GroupDevicesMemoOutcome::Restamp as usize + ); + assert!( + group_slot(GroupDevicesMemoOutcome::MissAbsent) + == GroupDevicesMemoOutcome::MissAbsent as usize + ); + assert!( + group_slot(GroupDevicesMemoOutcome::MissGroupInfo) + == GroupDevicesMemoOutcome::MissGroupInfo as usize + ); + assert!( + group_slot(GroupDevicesMemoOutcome::MissTopology) + == GroupDevicesMemoOutcome::MissTopology as usize + ); + assert!( + group_slot(GroupDevicesMemoOutcome::Bypassed) == GroupDevicesMemoOutcome::Bypassed as usize + ); + assert!(skdm_slot(SkdmTargetsMemoOutcome::Hit) == SkdmTargetsMemoOutcome::Hit as usize); + assert!( + skdm_slot(SkdmTargetsMemoOutcome::MissAbsent) + == SkdmTargetsMemoOutcome::MissAbsent as usize + ); + assert!( + skdm_slot(SkdmTargetsMemoOutcome::MissDevices) + == SkdmTargetsMemoOutcome::MissDevices as usize + ); + assert!(skdm_slot(SkdmTargetsMemoOutcome::MissMap) == SkdmTargetsMemoOutcome::MissMap as usize); + assert!( + skdm_slot(SkdmTargetsMemoOutcome::MissMapGeneration) + == SkdmTargetsMemoOutcome::MissMapGeneration as usize + ); + assert!( + skdm_slot(SkdmTargetsMemoOutcome::MissSender) + == SkdmTargetsMemoOutcome::MissSender as usize + ); + assert!( + skdm_slot(SkdmTargetsMemoOutcome::Bypassed) == SkdmTargetsMemoOutcome::Bypassed as usize + ); + assert!( + skdm_slot(SkdmTargetsMemoOutcome::ResolveFailed) + == SkdmTargetsMemoOutcome::ResolveFailed as usize + ); +}; + +/// The counters themselves. One per `Client`. +/// +/// Arrays rather than named fields so recording an outcome is +/// `slot[outcome as usize].fetch_add(1, Relaxed)` — one indexed atomic add, no +/// branch on which variant it was. [`group_slot`] and [`skdm_slot`] are what +/// keep that indexing in range as the enums grow. +#[derive(Debug, Default)] +pub(crate) struct DeviceMemoCounters { + group: [AtomicU64; GROUP_OUTCOMES], + skdm: [AtomicU64; SKDM_OUTCOMES], + /// Not an outcome — see [`Self::record_skdm_not_stored`] — so it is not in + /// the array and never counts toward `calls()`. + skdm_not_stored: AtomicU64, +} + +impl DeviceMemoCounters { + pub(crate) fn record_group_devices(&self, outcome: GroupDevicesMemoOutcome) { + self.group[outcome as usize].fetch_add(1, Ordering::Relaxed); + } + + pub(crate) fn record_skdm_targets(&self, outcome: SkdmTargetsMemoOutcome) { + self.skdm[outcome as usize].fetch_add(1, Ordering::Relaxed); + } + + /// A resolved target set that could not be memoized, so the next call + /// cannot hit. Recorded in addition to that call's own outcome, not + /// instead of it — it describes the *store*, not the lookup. + pub(crate) fn record_skdm_not_stored(&self) { + self.skdm_not_stored.fetch_add(1, Ordering::Relaxed); + } + + pub(crate) fn snapshot(&self) -> DeviceMemoStats { + let group = + |outcome: GroupDevicesMemoOutcome| self.group[outcome as usize].load(Ordering::Relaxed); + let skdm = + |outcome: SkdmTargetsMemoOutcome| self.skdm[outcome as usize].load(Ordering::Relaxed); + DeviceMemoStats { + group_devices: GroupDevicesMemoStats { + hits: group(GroupDevicesMemoOutcome::Hit), + restamps: group(GroupDevicesMemoOutcome::Restamp), + miss_absent: group(GroupDevicesMemoOutcome::MissAbsent), + miss_group_info: group(GroupDevicesMemoOutcome::MissGroupInfo), + miss_topology: group(GroupDevicesMemoOutcome::MissTopology), + bypassed: group(GroupDevicesMemoOutcome::Bypassed), + }, + skdm_targets: SkdmTargetsMemoStats { + hits: skdm(SkdmTargetsMemoOutcome::Hit), + miss_absent: skdm(SkdmTargetsMemoOutcome::MissAbsent), + miss_devices: skdm(SkdmTargetsMemoOutcome::MissDevices), + miss_map: skdm(SkdmTargetsMemoOutcome::MissMap), + miss_map_generation: skdm(SkdmTargetsMemoOutcome::MissMapGeneration), + miss_sender: skdm(SkdmTargetsMemoOutcome::MissSender), + not_stored: self.skdm_not_stored.load(Ordering::Relaxed), + bypassed: skdm(SkdmTargetsMemoOutcome::Bypassed), + resolve_failed: skdm(SkdmTargetsMemoOutcome::ResolveFailed), + }, + } + } +} + +/// Outcomes of `resolve_group_devices_memoized`, one per call. +/// +/// A re-stamp serves the same device list a hit would; it is counted apart +/// because it pays the `unchanged_for` scan first, and because a client whose +/// re-stamps dominate its hits is being pushed by unrelated topology writes. +#[non_exhaustive] +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct GroupDevicesMemoStats { + pub hits: u64, + pub restamps: u64, + /// No entry for the group: first send, or a capacity/TTL eviction. The + /// memo holds 64 groups, so a client rotating over more than that reports + /// its eviction rate here. + pub miss_absent: u64, + /// An entry existed, built from a different `Arc`. + pub miss_group_info: u64, + /// The device topology changed in a way that could have touched this group. + pub miss_topology: u64, + /// Calls that did not consult the memo at all (store-backed caches). + pub bypassed: u64, +} + +impl GroupDevicesMemoStats { + pub fn calls(&self) -> u64 { + self.hits + + self.restamps + + self.miss_absent + + self.miss_group_info + + self.miss_topology + + self.bypassed + } + + /// Share of calls served without re-resolving the device list — hits and + /// re-stamps together, since both skip the per-member registry fan-out. + /// `None` when nothing was resolved yet. + pub fn served_rate(&self) -> Option { + let calls = self.calls(); + (calls > 0).then(|| (self.hits + self.restamps) as f64 / calls as f64) + } +} + +/// Outcomes of `resolve_skdm_targets_memoized`, one per call. +#[non_exhaustive] +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct SkdmTargetsMemoStats { + pub hits: u64, + pub miss_absent: u64, + /// The device-set `Arc` changed. Read this against + /// [`GroupDevicesMemoStats`]: the `Arc` is what the group memo returns, so + /// this counter cannot go to zero while that memo is recomputing. + pub miss_devices: u64, + pub miss_map: u64, + pub miss_map_generation: u64, + pub miss_sender: u64, + /// Resolutions whose target set was neither empty nor own-devices-only, so + /// nothing was memoized. Each of these guarantees the *next* call cannot + /// hit — but not that it reports [`Self::miss_absent`]: when a stale entry + /// was already there it is left in place (nothing overwrites it, and it + /// can never become valid again — the map generation only moves forward + /// and the `Weak` keeps the old device allocation alive, so no `ptr::eq` + /// can spuriously match). The next call then reports whichever term is + /// still failing. A run of these means the group is not settling into the + /// warm steady state at all, which is why it is reported next to the miss + /// counts rather than folded into them. + pub not_stored: u64, + pub bypassed: u64, + /// Calls that never reached a memo term because the device resolution they + /// depend on returned an error. Counted so [`Self::calls`] stays one per + /// call to `resolve_skdm_targets_memoized`; a rising value here means + /// group sends are failing upstream of anything this report describes. + pub resolve_failed: u64, +} + +impl SkdmTargetsMemoStats { + pub fn calls(&self) -> u64 { + self.hits + + self.miss_absent + + self.miss_devices + + self.miss_map + + self.miss_map_generation + + self.miss_sender + + self.bypassed + + self.resolve_failed + } + + /// Memo hits over *every* call to the resolver, [`Self::resolve_failed`] + /// included. `None` when nothing was resolved yet. + /// + /// Deliberately not "share that skipped `filter_skdm_targets`": a call + /// whose device resolution errored also never reached the filter, and + /// crediting it would let a client whose group sends are failing report a + /// rising hit rate. Keeping those in the denominator makes the rate sag + /// under exactly the condition worth noticing. + pub fn hit_rate(&self) -> Option { + let calls = self.calls(); + (calls > 0).then(|| self.hits as f64 / calls as f64) + } +} + +/// Cumulative per-term outcomes of both device-list memos on the group send +/// path, from [`Client::device_memo_stats`]. +/// +/// Counts are monotonic for the client's lifetime; take two snapshots and +/// subtract to scope them to a workload. Numbers only, no JIDs — same rule as +/// every other report in `wacore::stats`. +#[non_exhaustive] +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct DeviceMemoStats { + pub group_devices: GroupDevicesMemoStats, + pub skdm_targets: SkdmTargetsMemoStats, +} + +impl DeviceMemoStats { + /// This snapshot minus an earlier one, so a workload can be scoped without + /// a reset (a reset would race any other send in flight). Saturating, so a + /// caller that passes the two the wrong way round reads zeros rather than + /// wrapping into billions. + pub fn since(&self, earlier: &Self) -> Self { + let group = &self.group_devices; + let was = &earlier.group_devices; + let skdm = &self.skdm_targets; + let skdm_was = &earlier.skdm_targets; + Self { + group_devices: GroupDevicesMemoStats { + hits: group.hits.saturating_sub(was.hits), + restamps: group.restamps.saturating_sub(was.restamps), + miss_absent: group.miss_absent.saturating_sub(was.miss_absent), + miss_group_info: group.miss_group_info.saturating_sub(was.miss_group_info), + miss_topology: group.miss_topology.saturating_sub(was.miss_topology), + bypassed: group.bypassed.saturating_sub(was.bypassed), + }, + skdm_targets: SkdmTargetsMemoStats { + hits: skdm.hits.saturating_sub(skdm_was.hits), + miss_absent: skdm.miss_absent.saturating_sub(skdm_was.miss_absent), + miss_devices: skdm.miss_devices.saturating_sub(skdm_was.miss_devices), + miss_map: skdm.miss_map.saturating_sub(skdm_was.miss_map), + miss_map_generation: skdm + .miss_map_generation + .saturating_sub(skdm_was.miss_map_generation), + miss_sender: skdm.miss_sender.saturating_sub(skdm_was.miss_sender), + not_stored: skdm.not_stored.saturating_sub(skdm_was.not_stored), + bypassed: skdm.bypassed.saturating_sub(skdm_was.bypassed), + resolve_failed: skdm.resolve_failed.saturating_sub(skdm_was.resolve_failed), + }, + } + } +} + +impl std::fmt::Display for DeviceMemoStats { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let group = &self.group_devices; + let skdm = &self.skdm_targets; + writeln!(f, "=== Device memo stats ===")?; + writeln!( + f, + " group_devices: {} calls, {} hit, {} restamp, miss: {} absent / {} group_info / {} topology, {} bypassed", + group.calls(), + group.hits, + group.restamps, + group.miss_absent, + group.miss_group_info, + group.miss_topology, + group.bypassed + )?; + write!( + f, + " skdm_targets: {} calls, {} hit, miss: {} absent / {} devices / {} map / {} map_gen / {} sender, {} not stored, {} bypassed, {} resolve failed", + skdm.calls(), + skdm.hits, + skdm.miss_absent, + skdm.miss_devices, + skdm.miss_map, + skdm.miss_map_generation, + skdm.miss_sender, + skdm.not_stored, + skdm.bypassed, + skdm.resolve_failed + ) + } +} + +impl Client { + /// Per-term hit/miss counts for the two device-list memos on the group + /// send path, cumulative since the client was built. + /// + /// The two are chained — `resolve_skdm_targets_memoized` compares the + /// `Arc` that `resolve_group_devices_memoized` returned — so a group-memo + /// recompute forces `skdm_targets.miss_devices` no matter what the other + /// three SKDM terms say. Read the group half first; the SKDM half only + /// carries independent information once the group half is hitting. + pub fn device_memo_stats(&self) -> DeviceMemoStats { + self.device_memo_counters.snapshot() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Exactly one counter per call is what makes `calls()` a sum rather than + /// a fourth thing that can disagree with the other three. + #[test] + fn every_outcome_lands_in_calls_exactly_once() { + let counters = DeviceMemoCounters::default(); + for outcome in [ + GroupDevicesMemoOutcome::Hit, + GroupDevicesMemoOutcome::Restamp, + GroupDevicesMemoOutcome::MissAbsent, + GroupDevicesMemoOutcome::MissGroupInfo, + GroupDevicesMemoOutcome::MissTopology, + GroupDevicesMemoOutcome::Bypassed, + ] { + counters.record_group_devices(outcome); + } + for outcome in [ + SkdmTargetsMemoOutcome::Hit, + SkdmTargetsMemoOutcome::MissAbsent, + SkdmTargetsMemoOutcome::MissDevices, + SkdmTargetsMemoOutcome::MissMap, + SkdmTargetsMemoOutcome::MissMapGeneration, + SkdmTargetsMemoOutcome::MissSender, + SkdmTargetsMemoOutcome::Bypassed, + SkdmTargetsMemoOutcome::ResolveFailed, + ] { + counters.record_skdm_targets(outcome); + } + // Not an outcome: it describes the store, so it must not inflate calls. + counters.record_skdm_not_stored(); + + let stats = counters.snapshot(); + assert_eq!(stats.group_devices.calls(), GROUP_OUTCOMES as u64); + assert_eq!(stats.skdm_targets.calls(), SKDM_OUTCOMES as u64); + assert_eq!(stats.skdm_targets.not_stored, 1); + } + + #[test] + fn rates_are_absent_rather_than_zero_before_the_first_call() { + let stats = DeviceMemoStats::default(); + assert_eq!(stats.group_devices.served_rate(), None); + assert_eq!(stats.skdm_targets.hit_rate(), None); + } +} diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 70cbf15d5..d18eee9a6 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -156,11 +156,14 @@ impl Client { group_info: &Arc, own_sending_jid: &Jid, ) -> Result, anyhow::Error> { + use crate::client::GroupDevicesMemoOutcome as Outcome; // Store-backed registry or mapping caches can be written by OTHER // processes (e.g. shared Redis across pods), which this process's // topology tracker cannot observe; the memo's freshness contract // doesn't hold there, so it is disabled and every send resolves. if !self.device_memos_enabled { + self.device_memo_counters + .record_group_devices(Outcome::Bypassed); return Ok(Arc::new(wacore::send::ResolvedGroupDevices::new( self.resolve_group_devices_uncached( group_info, @@ -177,22 +180,30 @@ impl Client { // and serve their effects stale. let generation = self.device_topology.current(); - if let Some(memo) = self.group_devices_memo.get(group).await - && std::ptr::eq(memo.group_info.as_ptr(), Arc::as_ptr(group_info)) - { - if memo.generation == generation { + // Each exit records the term that decided it, on the branch that + // decided it, rather than classifying into a value and dispatching on + // it twice — the counter is meant to be free on the hit path, and a + // second dispatch is not free. + if let Some(memo) = self.group_devices_memo.get(group).await { + if !std::ptr::eq(memo.group_info.as_ptr(), Arc::as_ptr(group_info)) { + self.device_memo_counters + .record_group_devices(Outcome::MissGroupInfo); + } else if memo.generation == generation { // Refcount bump: the snapshot is immutable, so a hit shares // it instead of cloning the device Vec. + self.device_memo_counters.record_group_devices(Outcome::Hit); return Ok(Arc::clone(&memo.devices)); - } - // Stale stamp: when every change since it touched only users - // outside this group, re-stamp instead of recomputing, so write - // storms on unrelated groups don't tank the hit rate. Any doubt - // (log overflow, member touched) falls through to the recompute. - if self + } else if self .device_topology .unchanged_for(memo.generation, |user| memo.members.contains(user)) { + // Stale stamp, but every change since it touched only users + // outside this group: re-stamp instead of recomputing, so + // write storms on unrelated groups don't tank the hit rate. + // Any doubt (log overflow, member touched) falls through to + // the recompute below. + self.device_memo_counters + .record_group_devices(Outcome::Restamp); self.group_devices_memo .insert( group.clone(), @@ -205,7 +216,13 @@ impl Client { ) .await; return Ok(Arc::clone(&memo.devices)); + } else { + self.device_memo_counters + .record_group_devices(Outcome::MissTopology); } + } else { + self.device_memo_counters + .record_group_devices(Outcome::MissAbsent); } let devices = self diff --git a/src/client/lifecycle.rs b/src/client/lifecycle.rs index 06ed55f3e..db2f2c65f 100644 --- a/src/client/lifecycle.rs +++ b/src/client/lifecycle.rs @@ -558,6 +558,7 @@ impl Client { .build(), #[cfg(test)] dm_devices_memo_recomputes: AtomicU64::new(0), + device_memo_counters: DeviceMemoCounters::default(), // A live lane also protects recipient-tracker reset/update ordering. group_distribution_locks: Cache::builder() .max_capacity(cache_config.group_distribution_locks_capacity.max(1)) diff --git a/src/send/mod.rs b/src/send/mod.rs index 5581c0b05..f45b2945a 100644 --- a/src/send/mod.rs +++ b/src/send/mod.rs @@ -138,7 +138,8 @@ fn ensure_self_in_group( } } -/// Whether a loaded `skdm_warm_memo` entry still describes this send. +/// Whether a loaded `skdm_warm_memo` entry still describes this send, and if +/// not, which term ruled it out. /// /// Its own function so the check has exactly one definition. The tests that /// pin the memo's behaviour need the same predicate, and a second copy of it @@ -146,18 +147,34 @@ fn ensure_self_in_group( /// tests would pass while every send missed the memo, with nothing reporting /// it. Pure comparison over an already-loaded tuple, so it adds nothing to the /// send path. -pub(crate) fn skdm_memo_entry_is_valid( +/// +/// The terms are checked in the same order the original `&&` chain +/// short-circuited them, and the answer names the FIRST one that failed rather +/// than "some term failed": with four terms, an aggregate miss count cannot +/// tell a cascading device-set change from an in-place cold flip, and those +/// two have opposite implications. +pub(crate) fn skdm_memo_entry_stale_term( memo: &crate::client::SkdmWarmMemoEntry, devices: &std::sync::Arc, cached_map: &std::sync::Arc, cached_map_generation: u64, own_sending_jid: &Jid, -) -> bool { +) -> Option { + use crate::client::SkdmTargetsMemoOutcome as Term; let (memo_devices, memo_map, memo_generation, memo_sender, _) = memo; - std::ptr::eq(memo_devices.as_ptr(), std::sync::Arc::as_ptr(devices)) - && std::ptr::eq(memo_map.as_ptr(), std::sync::Arc::as_ptr(cached_map)) - && *memo_generation == cached_map_generation - && memo_sender == own_sending_jid + if !std::ptr::eq(memo_devices.as_ptr(), std::sync::Arc::as_ptr(devices)) { + return Some(Term::MissDevices); + } + if !std::ptr::eq(memo_map.as_ptr(), std::sync::Arc::as_ptr(cached_map)) { + return Some(Term::MissMap); + } + if *memo_generation != cached_map_generation { + return Some(Term::MissMapGeneration); + } + if memo_sender != own_sending_jid { + return Some(Term::MissSender); + } + None } /// SKDM update data — only populated for group sends, deferred until after @@ -1493,6 +1510,7 @@ impl Client { group_info: &std::sync::Arc, own_sending_jid: &Jid, ) -> Option<(std::sync::Arc, Vec)> { + use crate::client::SkdmTargetsMemoOutcome as Outcome; let cached_map = self.skdm_device_map(group_jid).await; match self .resolve_group_devices_memoized(group, group_info, own_sending_jid) @@ -1516,17 +1534,34 @@ impl Client { // generation catches an in-place cold flip that keeps the // same Arc; the memoized needs are a pure function of that // identity. - if self.device_memos_enabled - && let Some(memo) = self.skdm_warm_memo.get(group).await - && skdm_memo_entry_is_valid( - &memo, - &all_devices, - &cached_map, - cached_map_gen, - own_sending_jid, - ) - { - return Some((all_devices, memo.4)); + if !self.device_memos_enabled { + self.device_memo_counters + .record_skdm_targets(Outcome::Bypassed); + } else { + // Recorded on the deciding branch, and the entry is never + // moved out of the `Some` arm: it is an owned clone of a + // five-field tuple carrying a `Jid` and a `Vec`, so + // shuffling it around to classify first is not free. + match self.skdm_warm_memo.get(group).await { + Some(memo) => { + match skdm_memo_entry_stale_term( + &memo, + &all_devices, + &cached_map, + cached_map_gen, + own_sending_jid, + ) { + None => { + self.device_memo_counters.record_skdm_targets(Outcome::Hit); + return Some((all_devices, memo.4)); + } + Some(term) => self.device_memo_counters.record_skdm_targets(term), + } + } + None => self + .device_memo_counters + .record_skdm_targets(Outcome::MissAbsent), + } } let needs_skdm = self.filter_skdm_targets( group_jid, @@ -1534,32 +1569,50 @@ impl Client { &cached_map, own_sending_jid, ); - if self.device_memos_enabled - && (needs_skdm.is_empty() || { + // Still inside the `device_memos_enabled` guard, and still + // short-circuiting: a client with store-backed caches must not + // pay the snapshot read for a memo it will never write. + if self.device_memos_enabled { + let memoizable = needs_skdm.is_empty() || { let snapshot = self.persistence_manager.get_device_snapshot(); skdm_needs_only_own_devices( &needs_skdm, snapshot.pn.as_ref(), snapshot.lid.as_ref(), ) - }) - { - self.skdm_warm_memo - .insert( - group.clone(), - ( - std::sync::Arc::downgrade(&all_devices), - std::sync::Arc::downgrade(&cached_map), - cached_map_gen, - own_sending_jid.clone(), - needs_skdm.clone(), - ), - ) - .await; + }; + if memoizable { + self.skdm_warm_memo + .insert( + group.clone(), + ( + std::sync::Arc::downgrade(&all_devices), + std::sync::Arc::downgrade(&cached_map), + cached_map_gen, + own_sending_jid.clone(), + needs_skdm.clone(), + ), + ) + .await; + } else { + // Nothing stored, so the next call cannot hit. Any + // stale entry is deliberately left in place rather + // than cleared: it can never become valid again (the + // map generation only moves forward, and the `Weak` + // keeps the old device allocation alive so no + // `ptr::eq` can spuriously match), so removing it + // would buy a cache write and change nothing. + self.device_memo_counters.record_skdm_not_stored(); + } } Some((all_devices, needs_skdm)) } Err(e) => { + // Recorded so `SkdmTargetsMemoStats::calls()` really is one + // per call: a client failing here would otherwise report a + // healthy hit rate over a denominator that quietly shrank. + self.device_memo_counters + .record_skdm_targets(Outcome::ResolveFailed); log::warn!( "Failed to resolve devices for SKDM check in {}: {:?}", group_jid, @@ -2957,36 +3010,77 @@ mod tests { /// A group whose metadata, device lists and pairwise sessions are all /// primed, so a send reaches the wire without a single IQ and the captured /// frames are exactly the message stanzas the test asked for. + use wacore::types::message::AddressingMode; + struct GroupSendFixture { client: Arc, transport: Arc, group: Jid, member: Jid, + /// The identity this group addresses us by: our LID in a LID group, + /// our phone JID otherwise. Companion devices and their sessions have + /// to live under it, or a warm send's own-device SKDM target has no + /// session and the send blocks on a prekey fetch. + own_sending: Jid, /// Every recipient device the group resolves to (own device excluded). recipient_devices: usize, } impl GroupSendFixture { async fn new() -> Self { + Self::with_addressing(AddressingMode::Pn, 2).await + } + + /// A group whose participants are LID-addressed, with the LID↔PN pairs + /// both in the group metadata's map and durably in the client's LID-PN + /// cache — the state a client that has already synced the group is in. + /// + /// Not a cosmetic variant of the PN fixture: LID mode is what puts + /// `GroupInfo::phone_jid_for_lid_user` on the resolve path (once per + /// participant, on the way in and on the way back), so it is the mode + /// where a device-memo miss is most expensive. PR #1283 named it as + /// the largest gap in its own coverage. + async fn new_lid(member_count: usize) -> Self { + Self::with_addressing(AddressingMode::Lid, member_count).await + } + + async fn with_addressing(addressing_mode: AddressingMode, member_count: usize) -> Self { use wacore::client::context::GroupInfo; use wacore::store::traits::{DeviceInfo, DeviceListRecord}; - use wacore::types::message::AddressingMode; + let is_lid = addressing_mode == AddressingMode::Lid; let (client, transport) = crate::test_utils::create_iq_test_client().await; let own = Jid::from_str("5511000000001@s.whatsapp.net").unwrap(); + let own_lid = Jid::from_str("100000000000001@lid").unwrap(); client .persistence_manager .process_command(DeviceCommand::SetId(Some(own.clone()))) .await; client .persistence_manager - .process_command(DeviceCommand::SetLid(Some( - Jid::from_str("100000000000001@lid").unwrap(), - ))) + .process_command(DeviceCommand::SetLid(Some(own_lid.clone()))) .await; - let member_users = ["5511000000002", "5511000000003"]; - for user in [own.user.as_str()].into_iter().chain(member_users) { + // Deterministic in the index so the same fixture at 2 members is a + // prefix of the one at 64: reserved fictional numbers, and LIDs + // from a range no real allocation uses. + let member_users: Vec = (0..member_count) + .map(|index| format!("55110000{:05}", 10 + index)) + .collect(); + let member_lids: Vec = (0..member_count) + .map(|index| format!("2000000000{:05}", 10 + index)) + .collect(); + + // Registry records go in under the PN key in both modes: the LID + // resolve maps each participant back to its PN before querying + // (LID usync is unreliable), then converts the answer to LID. + // `raw_insert_for_tests` rather than `insert` — a seeded cache fill + // must not look like a topology change, or the fixture would start + // every memo one generation behind for reasons no client has. + for user in [own.user.as_str()] + .into_iter() + .chain(member_users.iter().map(String::as_str)) + { let record = DeviceListRecord { user: user.into(), devices: vec![DeviceInfo::new(0, None)], @@ -3000,21 +3094,60 @@ mod tests { .await; } - let participants: Vec = member_users - .iter() - .map(|user| Jid::from_str(&format!("{user}@s.whatsapp.net")).unwrap()) - .collect(); + let participants: Vec = if is_lid { + member_lids + .iter() + .map(|lid| Jid::from_str(&format!("{lid}@lid")).unwrap()) + .collect() + } else { + member_users + .iter() + .map(|user| Jid::from_str(&format!("{user}@s.whatsapp.net")).unwrap()) + .collect() + }; for participant in &participants { crate::test_utils::seed_peer_session(&client, participant).await; } + let group_info = if is_lid { + let lid_to_pn = member_lids + .iter() + .zip(&member_users) + .map(|(lid, pn)| { + ( + lid.as_str().into(), + Jid::from_str(&format!("{pn}@s.whatsapp.net")).unwrap(), + ) + }) + .collect(); + // Persist the pairs the way a synced client holds them, so the + // receive path's `can_skip_relearn` fast exit is reachable. + // Without this every inbound message re-learns the mapping and + // the fixture would report a topology write that a real warm + // client does not perform. + for (lid, pn) in member_lids.iter().zip(&member_users) { + client + .add_lid_pn_mapping(lid, pn, crate::lid_pn_cache::LearningSource::Usync) + .await + .expect("seeding a lid-pn pair must succeed against the test backend"); + } + client + .add_lid_pn_mapping( + &own_lid.user, + &own.user, + crate::lid_pn_cache::LearningSource::Usync, + ) + .await + .expect("seeding our own lid-pn pair must succeed"); + GroupInfo::with_lid_to_pn_map(participants.clone(), addressing_mode, lid_to_pn) + } else { + GroupInfo::new(participants.clone(), addressing_mode) + }; + let group = Jid::from_str("120363000000000042@g.us").unwrap(); client .get_group_cache() - .insert( - group.clone(), - Arc::new(GroupInfo::new(participants.clone(), AddressingMode::Pn)), - ) + .insert(group.clone(), Arc::new(group_info)) .await; Self { @@ -3022,6 +3155,7 @@ mod tests { transport, group, member: participants[0].clone(), + own_sending: if is_lid { own_lid } else { own }, recipient_devices: participants.len(), } } @@ -3045,6 +3179,9 @@ mod tests { .pn .clone() .expect("own pn"); + // The registry record stays PN-keyed in both modes, matching how + // the fixture seeds every other user; the LID resolve reaches it + // through the mapping. let record = DeviceListRecord { user: own.user.as_str().into(), devices: vec![ @@ -3059,7 +3196,7 @@ mod tests { .device_registry_cache .raw_insert_for_tests(own.user.to_string(), Arc::new(record)) .await; - let companion = own.with_device(device_id); + let companion = self.own_sending.with_device(device_id); crate::test_utils::seed_peer_session(&self.client, &companion).await; companion } @@ -3071,6 +3208,28 @@ mod tests { .expect("group text send should reach the wire"); } + /// The topology-visible half of receiving a group message from + /// `member`: the LID↔PN pair its stanza carries, fed through the same + /// entry point the receive path uses. + /// + /// LID-mode groups only — a PN-addressed group's messages carry no + /// second identifier, so there is nothing for the receive path to + /// learn and nothing that could move the topology generation. + async fn receive_from_member(&self) { + let pn = self + .client + .get_group_cache() + .get(&self.group) + .await + .expect("group metadata") + .phone_jid_for_lid_user(&self.member.user) + .cloned() + .expect("the LID fixture maps every participant to a phone number"); + self.client + .cache_lid_pn_from_message(&self.member, Some(&pn), false) + .await; + } + async fn revoke(&self, message_id: &str, revoke_type: RevokeType) { self.client .revoke_message(self.group.clone(), message_id, revoke_type) @@ -3171,7 +3330,9 @@ mod tests { let generation = cached_map.generation(); let memo = client.skdm_warm_memo.get(group).await?; // The same predicate the send path applies, not a second copy of it. - skdm_memo_entry_is_valid(&memo, &devices, &cached_map, generation, &own).then_some(memo.4) + skdm_memo_entry_stale_term(&memo, &devices, &cached_map, generation, &own) + .is_none() + .then_some(memo.4) } /// The premise of every "the warm group send is flat in group size" claim: @@ -3216,6 +3377,266 @@ mod tests { } } + /// Sends the external `group-send` profile ran inside its window. Matched + /// so a hit rate measured here is comparable to the one that profile + /// implies, rather than to a number of rounds picked for convenience. + const REGIME_SENDS: u64 = 30; + + /// The question two benchmark PRs left open: over a run of ordinary repeat + /// sends, which outcome do the two device memos actually take? + /// + /// `skdm_target_resolution_warm` and `skdm_target_resolution_memo_cold` + /// bound the cost of a hit and of a miss, but both force their outcome, so + /// neither can say which one a client gets once the group is warm — and an + /// external profile of a different client implied "miss, on all 30 of 30". + /// This is the missing middle: N consecutive sends through the real + /// `send_message`, reading the per-term counters over the window. + /// + /// Asserted on the terms and not just on a rate, because the two memos are + /// chained: `resolve_skdm_targets_memoized` compares the `Arc` the group + /// memo returned, so a group-memo recompute forces an SKDM miss whatever + /// the other three SKDM terms say. A rate would show two failures where + /// there is one cause. + #[tokio::test] + async fn repeat_group_sends_hit_both_device_memos_on_every_send() { + let fixture = GroupSendFixture::new().await; + fixture.add_own_companion(1).await; + // Two sends before the window, for two different reasons. Send one is + // cold: `force_skdm` short-circuits the whole memoized path, so it + // never so much as looks the memos up. Send two is the first that + // does, and it necessarily misses — there is nothing stored yet. The + // steady state starts at send three, which is also where + // `bench_support`'s fixture starts measuring. + fixture.send_text("cold send").await; + fixture + .send_text("first warm send, populates both memos") + .await; + let before = fixture.client.device_memo_stats(); + + for _ in 0..REGIME_SENDS { + fixture.send_text("warm send").await; + } + + let window = fixture.client.device_memo_stats().since(&before); + assert_eq!( + window.group_devices.hits, REGIME_SENDS, + "every warm send must take the group memo outright: {window}" + ); + assert_eq!( + window.skdm_targets.hits, REGIME_SENDS, + "every warm send must skip filter_skdm_targets: {window}" + ); + // Named individually rather than through the rate: which term fires is + // the diagnosis, and a rate assertion would pass a run that swapped + // one miss cause for another. + assert_eq!(window.group_devices.miss_absent, 0, "{window}"); + assert_eq!(window.group_devices.miss_group_info, 0, "{window}"); + assert_eq!(window.group_devices.miss_topology, 0, "{window}"); + assert_eq!(window.group_devices.restamps, 0, "{window}"); + assert_eq!(window.skdm_targets.miss_devices, 0, "{window}"); + assert_eq!(window.skdm_targets.miss_map, 0, "{window}"); + assert_eq!(window.skdm_targets.miss_map_generation, 0, "{window}"); + assert_eq!(window.skdm_targets.miss_sender, 0, "{window}"); + assert_eq!( + window.skdm_targets.not_stored, 0, + "a target set that cannot be memoized makes the next send miss by \ + construction: {window}" + ); + } + + /// The same window in a LID-addressed group — the mode the external + /// profile ran, and the one PR #1283's PN fixture could not reach. + /// + /// It matters beyond coverage: LID mode is what puts + /// `GroupInfo::phone_jid_for_lid_user` on the resolve path, once per + /// participant mapping in and once per resolved device mapping back. That + /// function only ever runs inside the uncached resolve, so it is a cost + /// the memo either pays in full or removes entirely — never something in + /// between, and never a target of its own while the memo hits. + #[tokio::test] + async fn repeat_lid_group_sends_hit_both_device_memos_on_every_send() { + let fixture = GroupSendFixture::new_lid(8).await; + fixture.add_own_companion(1).await; + fixture.send_text("cold send").await; + fixture + .send_text("first warm send, populates both memos") + .await; + let before = fixture.client.device_memo_stats(); + + for _ in 0..REGIME_SENDS { + fixture.send_text("warm send").await; + } + + let window = fixture.client.device_memo_stats().since(&before); + assert_eq!( + window.group_devices.hits, REGIME_SENDS, + "LID addressing must not cost the group memo a single hit: {window}" + ); + assert_eq!( + window.skdm_targets.hits, REGIME_SENDS, + "LID addressing must not cost the SKDM memo a single hit: {window}" + ); + } + + /// The server-paced shape: the client receives from the group and answers, + /// which is what a group bot does and what the external profile's harness + /// drove. If handling an inbound message writes device topology, a memo + /// keyed on that topology misses once per round by construction, and the + /// finding would be about production rather than about any harness. + /// + /// The inbound side here is the LID↔PN learning an inbound group message + /// performs (`cache_lid_pn_from_message`, called from the receive path for + /// every message whose sender carries both identifiers) — not a full + /// decode. That is the only part of an inbound message that reaches the + /// topology tracker on a steady-state receive; decryption, dispatch and + /// receipts are not covered, and a regression that made some *other* part + /// of the receive path write topology would not be caught here. + #[tokio::test] + async fn a_send_answering_an_inbound_group_message_still_hits_both_memos() { + let fixture = GroupSendFixture::new_lid(8).await; + fixture.add_own_companion(1).await; + fixture.send_text("cold send").await; + fixture + .send_text("first warm send, populates both memos") + .await; + let before = fixture.client.device_memo_stats(); + + for _ in 0..REGIME_SENDS { + fixture.receive_from_member().await; + fixture.send_text("reply").await; + } + + let window = fixture.client.device_memo_stats().since(&before); + assert_eq!( + window.group_devices.hits, REGIME_SENDS, + "a mapping the client already holds durably must not be re-learned, \ + and re-learning it would bump the topology generation once per \ + inbound message: {window}" + ); + assert_eq!(window.skdm_targets.hits, REGIME_SENDS, "{window}"); + } + + /// The counters would be worthless if they only ever reported hits, so + /// each miss term is driven once and checked to be the one that fires. + /// This is also what pins the terms against each other: three of these + /// four causes are indistinguishable in an aggregate miss count, and the + /// whole point of the instrumentation is telling them apart. + #[tokio::test] + async fn each_miss_term_is_reported_as_itself() { + use wacore::client::context::GroupInfo; + + let fixture = GroupSendFixture::new().await; + fixture.add_own_companion(1).await; + fixture.send_text("cold send").await; + fixture.send_text("warm send").await; + + // 1. An in-place cold flip (a retry receipt's markForgetSenderKey) + // keeps both Arcs and advances the map generation. + let before = fixture.client.device_memo_stats(); + fixture + .client + .sender_key_device_cache + .mark_forgotten( + &fixture.group.to_string(), + std::iter::once(&fixture.member.with_device(0)), + ) + .await; + fixture.send_text("after a forget").await; + let window = fixture.client.device_memo_stats().since(&before); + assert_eq!( + window.skdm_targets.miss_map_generation, 1, + "a cold flip is the generation term, and nothing else: {window}" + ); + // Two resolves, not one: the re-cold device puts a non-own target in + // the needs set, which takes the single-flight branch — it invalidates + // the sender-key map and resolves again. That second resolve is the + // MissMap, and the group memo hits both times because none of this + // touched the device topology. + assert_eq!(window.skdm_targets.miss_map, 1, "{window}"); + assert_eq!(window.group_devices.hits, 2, "{window}"); + + // 2. A group metadata refresh publishes a new Arc, which is the group + // memo's identity term and cascades into the SKDM memo's first. + fixture.send_text("re-warm").await; + let before = fixture.client.device_memo_stats(); + let participants = fixture + .client + .get_group_cache() + .get(&fixture.group) + .await + .expect("group metadata") + .participants + .clone(); + fixture + .client + .get_group_cache() + .insert( + fixture.group.clone(), + Arc::new(GroupInfo::new(participants, AddressingMode::Pn)), + ) + .await; + fixture.send_text("after a metadata refresh").await; + let window = fixture.client.device_memo_stats().since(&before); + assert_eq!( + window.group_devices.miss_group_info, 1, + "a fresh GroupInfo Arc is the identity term: {window}" + ); + assert_eq!( + window.skdm_targets.miss_devices, 1, + "and the SKDM memo misses on the device Arc it cascades into, not \ + on one of its own three terms: {window}" + ); + + // 3. A registry write touching a member is the topology term. The + // scoped log can only clear a change that provably missed the + // group, and this one does not. + // + // Recorded straight on the tracker rather than through + // `invalidate_device_cache`: every registry write funnels into + // `record_registry` by construction (that is the whole design of + // `DeviceRegistryCache`), so this is the same signal — and it + // leaves the member's device record in place, where invalidating + // would delete it and make every later resolve in this test reach + // for a usync the fixture has no server for. + fixture.send_text("re-warm").await; + let before = fixture.client.device_memo_stats(); + fixture + .client + .device_topology + .record([&*fixture.member.user]); + fixture.send_text("after a member's devices changed").await; + let window = fixture.client.device_memo_stats().since(&before); + assert_eq!( + window.group_devices.miss_topology, 1, + "a write touching a member must not be provable as clean: {window}" + ); + assert_eq!( + window.skdm_targets.miss_devices, 1, + "the recompute hands out a new device Arc, which is the cascade: {window}" + ); + + // 4. A write touching a stranger is the re-stamp: the generation + // moved, and the log proves the change missed this group. + fixture.send_text("re-warm").await; + let before = fixture.client.device_memo_stats(); + fixture.client.device_topology.record(["12025550111"]); + fixture.send_text("after an unrelated user changed").await; + let window = fixture.client.device_memo_stats().since(&before); + assert_eq!( + window.group_devices.restamps, 1, + "an unrelated write must re-stamp, not recompute: {window}" + ); + assert_eq!( + window.group_devices.miss_topology, 0, + "and it must not read as a topology miss: {window}" + ); + assert_eq!( + window.skdm_targets.hits, 1, + "a re-stamp serves the same device Arc, so the SKDM memo still \ + hits behind it: {window}" + ); + } + /// The counterpart: a forgotten device (a retry receipt's /// `markForgetSenderKey`) flips the map in place, which keeps the `Arc` but /// advances the generation — the one signal pointer identity cannot carry.