From 625a8a282ca6eb2e6ee309f14a3cb6b67ed994c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 00:59:36 +0000 Subject: [PATCH 1/7] diag(send): count which term invalidates each group-path device memo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two benchmark PRs (#1283, #1285) bounded the cost of a device-memo hit and of a miss, and both had to force the outcome to do it. Neither could say which outcome a client in regime takes, and that is what decides whether the miss-path cost is a bill anyone pays. This adds the counters that answer it, per term rather than per memo. `Client::device_memo_stats()` reports, for each call to `resolve_group_devices_memoized` and `resolve_skdm_targets_memoized`, which validity term decided it: three terms plus a scoped re-stamp for the group memo, four for the SKDM memo, plus the not-stored case that makes the next call miss by construction. An aggregate miss count cannot tell those apart, and it cannot separate cause from consequence either — the SKDM memo compares the Arc the group memo returned. `skdm_memo_entry_is_valid` becomes `skdm_memo_entry_stale_term`, same terms in the same short-circuit order, now naming the first that failed. No memo semantics change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016uk5eo5FdJfBQeMQBJWRo8 --- agent_docs/observability.md | 35 ++- src/bench_support.rs | 11 + src/client.rs | 12 + src/client/device_memo_stats.rs | 371 +++++++++++++++++++++++ src/client/device_registry.rs | 42 ++- src/client/lifecycle.rs | 1 + src/send/mod.rs | 504 ++++++++++++++++++++++++++++---- 7 files changed, 914 insertions(+), 62 deletions(-) create mode 100644 src/client/device_memo_stats.rs diff --git a/agent_docs/observability.md b/agent_docs/observability.md index 3fce53d77..749c57cf1 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,38 @@ 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 +(device `Arc`, sender-key-map `Arc`, map generation, sending identity). 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 and the *next* call is a +`miss_absent` by construction, not by eviction). + +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 +`skdm_target_resolution_warm`, the tightest thing the counters sit inside, the +whole instrumentation is +8 instructions per resolve, flat in group size. + +### 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/src/bench_support.rs b/src/bench_support.rs index 8119bce6c..9d77e9e61 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 + /// in regime takes. 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 3d6617139..23814e0fb 100644 --- a/src/client.rs +++ b/src/client.rs @@ -4,6 +4,7 @@ mod app_state; pub(crate) use app_state::SyncSettles; mod builder; mod context_impl; +mod device_memo_stats; mod device_registry; pub(crate) mod device_topology; #[cfg(feature = "client-lifecycle")] @@ -20,6 +21,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")] @@ -1564,6 +1569,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..32feb5bca --- /dev/null +++ b/src/client/device_memo_stats.rs @@ -0,0 +1,371 @@ +//! 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: first send, an eviction, or a previous send whose targets + /// were not memoizable (see [`SkdmTargetsMemoStats::not_stored`]). + 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, +} + +const GROUP_OUTCOMES: usize = 6; +const SKDM_OUTCOMES: usize = 7; + +/// 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. The `_OUTCOMES` lengths are asserted +/// against the variants below, so adding a variant without widening the array +/// fails to compile rather than panicking at the first send. +#[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 send is + /// an [`SkdmTargetsMemoOutcome::MissAbsent`] by construction. 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), + }, + } + } +} + +/// The array index of the last variant must be in range, or a recorded outcome +/// would index out of bounds on a path with no other symptom. +const _: () = { + assert!(GroupDevicesMemoOutcome::Bypassed as usize == GROUP_OUTCOMES - 1); + assert!(SkdmTargetsMemoOutcome::Bypassed as usize == SKDM_OUTCOMES - 1); +}; + +/// 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 is a + /// [`Self::miss_absent`], which is why it is reported next to them rather + /// than folded into the miss counts. + pub not_stored: u64, + pub bypassed: 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 + } + + /// Share of calls that skipped `filter_skdm_targets`. `None` when nothing + /// was resolved yet. + 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), + }, + } + } +} + +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", + 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 + ) + } +} + +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, + ] { + 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(), 6); + assert_eq!(stats.skdm_targets.calls(), 7); + 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..28392c5fb 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,40 @@ 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 { - // Refcount bump: the snapshot is immutable, so a hit shares - // it instead of cloning the device Vec. - return Ok(Arc::clone(&memo.devices)); + // Classified before acting, so the counter names the term that decided + // the call. The arms are the validity terms in the order they gate + // each other: no entry, then `GroupInfo` identity, then the generation + // stamp, then the scoped-invalidation proof. Evaluating them in any + // other order would attribute a miss to a term that never ran. + let cached = self.group_devices_memo.get(group).await; + let outcome = match &cached { + None => Outcome::MissAbsent, + Some(memo) if !std::ptr::eq(memo.group_info.as_ptr(), Arc::as_ptr(group_info)) => { + Outcome::MissGroupInfo } + Some(memo) if memo.generation == generation => Outcome::Hit, // 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 - .device_topology - .unchanged_for(memo.generation, |user| memo.members.contains(user)) + Some(memo) + if self + .device_topology + .unchanged_for(memo.generation, |user| memo.members.contains(user)) => { + Outcome::Restamp + } + Some(_) => Outcome::MissTopology, + }; + self.device_memo_counters.record_group_devices(outcome); + + match (outcome, &cached) { + // Refcount bump: the snapshot is immutable, so a hit shares + // it instead of cloning the device Vec. + (Outcome::Hit, Some(memo)) => { + return Ok(Arc::clone(&memo.devices)); + } + (Outcome::Restamp, Some(memo)) => { self.group_devices_memo .insert( group.clone(), @@ -206,6 +227,7 @@ impl Client { .await; return Ok(Arc::clone(&memo.devices)); } + _ => {} } let devices = self diff --git a/src/client/lifecycle.rs b/src/client/lifecycle.rs index 7ec3c60c4..9031b60b7 100644 --- a/src/client/lifecycle.rs +++ b/src/client/lifecycle.rs @@ -538,6 +538,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..24a4d3205 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,26 @@ 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 { + let memo = self.skdm_warm_memo.get(group).await; + let stale = match &memo { + None => Some(Outcome::MissAbsent), + Some(memo) => skdm_memo_entry_stale_term( + memo, + &all_devices, + &cached_map, + cached_map_gen, + own_sending_jid, + ), + }; + self.device_memo_counters + .record_skdm_targets(stale.unwrap_or(Outcome::Hit)); + if let (None, Some(memo)) = (stale, memo) { + return Some((all_devices, memo.4)); + } } let needs_skdm = self.filter_skdm_targets( group_jid, @@ -1534,28 +1561,37 @@ 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 is a MissAbsent by + // construction rather than by eviction. Counted apart + // so that distinction survives into the report. + self.device_memo_counters.record_skdm_not_stored(); + } } Some((all_devices, needs_skdm)) } @@ -2957,36 +2993,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 +3077,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 +3138,7 @@ mod tests { transport, group, member: participants[0].clone(), + own_sending: if is_lid { own_lid } else { own }, recipient_devices: participants.len(), } } @@ -3045,6 +3162,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 +3179,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 +3191,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 +3313,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 +3360,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 in regime gets — 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(["15555550123"]); + 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. From 87de4a9a73bed98a41393c6509750ec684759a92 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 01:23:00 +0000 Subject: [PATCH 2/7] diag(send): record each memo outcome on its deciding branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Classifying into an outcome value and then dispatching on it a second time to act cost 126 instructions per resolve, measured; recording on the branch that decided cuts that to 5. The SKDM half also stopped moving the memo entry — an owned clone of a five-field tuple carrying a Jid and a Vec — into a temporary just to classify it first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016uk5eo5FdJfBQeMQBJWRo8 --- src/client/device_registry.rs | 63 ++++++++++++++++------------------- src/send/mod.rs | 38 ++++++++++++--------- 2 files changed, 52 insertions(+), 49 deletions(-) diff --git a/src/client/device_registry.rs b/src/client/device_registry.rs index 28392c5fb..d18eee9a6 100644 --- a/src/client/device_registry.rs +++ b/src/client/device_registry.rs @@ -180,40 +180,30 @@ impl Client { // and serve their effects stale. let generation = self.device_topology.current(); - // Classified before acting, so the counter names the term that decided - // the call. The arms are the validity terms in the order they gate - // each other: no entry, then `GroupInfo` identity, then the generation - // stamp, then the scoped-invalidation proof. Evaluating them in any - // other order would attribute a miss to a term that never ran. - let cached = self.group_devices_memo.get(group).await; - let outcome = match &cached { - None => Outcome::MissAbsent, - Some(memo) if !std::ptr::eq(memo.group_info.as_ptr(), Arc::as_ptr(group_info)) => { - Outcome::MissGroupInfo - } - Some(memo) if memo.generation == generation => Outcome::Hit, - // 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. - Some(memo) - if self - .device_topology - .unchanged_for(memo.generation, |user| memo.members.contains(user)) => - { - Outcome::Restamp - } - Some(_) => Outcome::MissTopology, - }; - self.device_memo_counters.record_group_devices(outcome); - - match (outcome, &cached) { - // Refcount bump: the snapshot is immutable, so a hit shares - // it instead of cloning the device Vec. - (Outcome::Hit, Some(memo)) => { + // 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)); - } - (Outcome::Restamp, Some(memo)) => { + } 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(), @@ -226,8 +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/send/mod.rs b/src/send/mod.rs index 24a4d3205..df494d7e9 100644 --- a/src/send/mod.rs +++ b/src/send/mod.rs @@ -1538,21 +1538,29 @@ impl Client { self.device_memo_counters .record_skdm_targets(Outcome::Bypassed); } else { - let memo = self.skdm_warm_memo.get(group).await; - let stale = match &memo { - None => Some(Outcome::MissAbsent), - Some(memo) => skdm_memo_entry_stale_term( - memo, - &all_devices, - &cached_map, - cached_map_gen, - own_sending_jid, - ), - }; - self.device_memo_counters - .record_skdm_targets(stale.unwrap_or(Outcome::Hit)); - if let (None, Some(memo)) = (stale, memo) { - return Some((all_devices, memo.4)); + // 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( From 331aaa5c4a60675d36ccc76c94cba57b0676b6b3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 01:34:33 +0000 Subject: [PATCH 3/7] docs(perf): record the memo hit rate and what the counters cost Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016uk5eo5FdJfBQeMQBJWRo8 --- agent_docs/observability.md | 14 ++++++++++++-- benches/client_group_send.rs | 14 ++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/agent_docs/observability.md b/agent_docs/observability.md index 749c57cf1..d301dacc4 100644 --- a/agent_docs/observability.md +++ b/agent_docs/observability.md @@ -132,8 +132,18 @@ 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 -`skdm_target_resolution_warm`, the tightest thing the counters sit inside, the -whole instrumentation is +8 instructions per resolve, flat in group size. +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) 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); From b63882f23166172dd2414d662155479d1fe2cb19 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 01:52:29 +0000 Subject: [PATCH 4/7] diag(send): make the counters total honest and the index guard real MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings, all valid: - The const assert only pinned the last variant's index, so a variant added after it would keep the assert passing and index off the end of the array on the send path. Replaced with exhaustive const `match`es, which stop compiling when a variant is added anywhere. `record_*` still indexes by `as usize`, so the hot path is unchanged. - A resolver call whose device resolution failed recorded no SKDM outcome, so `calls()` was not one per call and `hit_rate()` could look healthy over a shrinking denominator. Added `resolve_failed`. - `not_stored` promised the next call would be `miss_absent`. It does not: a stale entry is left in place, so the next call reports whichever term is still failing. Documented what it actually guarantees — that the next call cannot hit — and why leaving the stale entry is correct. - The unrelated-user literal used an NPA of 555, which is not the reserved fictional NANP format. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016uk5eo5FdJfBQeMQBJWRo8 --- src/bench_support.rs | 6 +- src/client/device_memo_stats.rs | 147 ++++++++++++++++++++++++++------ src/send/mod.rs | 23 +++-- 3 files changed, 142 insertions(+), 34 deletions(-) diff --git a/src/bench_support.rs b/src/bench_support.rs index 9d77e9e61..c8e6e3819 100644 --- a/src/bench_support.rs +++ b/src/bench_support.rs @@ -188,9 +188,9 @@ impl GroupSendHarness { /// /// The benchmarks below force their memo outcome, which is what makes them /// measurable — and what makes them unable to say which outcome a client - /// in regime takes. 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. + /// 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() } diff --git a/src/client/device_memo_stats.rs b/src/client/device_memo_stats.rs index 32feb5bca..0f2ce58a8 100644 --- a/src/client/device_memo_stats.rs +++ b/src/client/device_memo_stats.rs @@ -60,8 +60,7 @@ pub(crate) enum GroupDevicesMemoOutcome { #[repr(usize)] pub(crate) enum SkdmTargetsMemoOutcome { Hit, - /// No entry: first send, an eviction, or a previous send whose targets - /// were not memoizable (see [`SkdmTargetsMemoStats::not_stored`]). + /// 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 @@ -75,18 +74,110 @@ pub(crate) enum SkdmTargetsMemoOutcome { /// 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 = 7; +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. The `_OUTCOMES` lengths are asserted -/// against the variants below, so adding a variant without widening the array -/// fails to compile rather than panicking at the first send. +/// 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], @@ -105,10 +196,9 @@ impl DeviceMemoCounters { self.skdm[outcome as usize].fetch_add(1, Ordering::Relaxed); } - /// A resolved target set that could not be memoized, so the next send is - /// an [`SkdmTargetsMemoOutcome::MissAbsent`] by construction. Recorded in - /// addition to that call's own outcome, not instead of it — it describes - /// the *store*, not the lookup. + /// 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); } @@ -136,18 +226,12 @@ impl DeviceMemoCounters { miss_sender: skdm(SkdmTargetsMemoOutcome::MissSender), not_stored: self.skdm_not_stored.load(Ordering::Relaxed), bypassed: skdm(SkdmTargetsMemoOutcome::Bypassed), + resolve_failed: skdm(SkdmTargetsMemoOutcome::ResolveFailed), }, } } } -/// The array index of the last variant must be in range, or a recorded outcome -/// would index out of bounds on a path with no other symptom. -const _: () = { - assert!(GroupDevicesMemoOutcome::Bypassed as usize == GROUP_OUTCOMES - 1); - assert!(SkdmTargetsMemoOutcome::Bypassed as usize == SKDM_OUTCOMES - 1); -}; - /// Outcomes of `resolve_group_devices_memoized`, one per call. /// /// A re-stamp serves the same device list a hit would; it is counted apart @@ -203,11 +287,22 @@ pub struct SkdmTargetsMemoStats { 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 is a - /// [`Self::miss_absent`], which is why it is reported next to them rather - /// than folded into the miss counts. + /// 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 { @@ -219,6 +314,7 @@ impl SkdmTargetsMemoStats { + self.miss_map_generation + self.miss_sender + self.bypassed + + self.resolve_failed } /// Share of calls that skipped `filter_skdm_targets`. `None` when nothing @@ -272,6 +368,7 @@ impl DeviceMemoStats { 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), }, } } @@ -295,7 +392,7 @@ impl std::fmt::Display for DeviceMemoStats { )?; write!( f, - " skdm_targets: {} calls, {} hit, miss: {} absent / {} devices / {} map / {} map_gen / {} sender, {} not stored, {} bypassed", + " skdm_targets: {} calls, {} hit, miss: {} absent / {} devices / {} map / {} map_gen / {} sender, {} not stored, {} bypassed, {} resolve failed", skdm.calls(), skdm.hits, skdm.miss_absent, @@ -304,7 +401,8 @@ impl std::fmt::Display for DeviceMemoStats { skdm.miss_map_generation, skdm.miss_sender, skdm.not_stored, - skdm.bypassed + skdm.bypassed, + skdm.resolve_failed ) } } @@ -350,6 +448,7 @@ mod tests { SkdmTargetsMemoOutcome::MissMapGeneration, SkdmTargetsMemoOutcome::MissSender, SkdmTargetsMemoOutcome::Bypassed, + SkdmTargetsMemoOutcome::ResolveFailed, ] { counters.record_skdm_targets(outcome); } @@ -357,8 +456,8 @@ mod tests { counters.record_skdm_not_stored(); let stats = counters.snapshot(); - assert_eq!(stats.group_devices.calls(), 6); - assert_eq!(stats.skdm_targets.calls(), 7); + 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); } diff --git a/src/send/mod.rs b/src/send/mod.rs index df494d7e9..f45b2945a 100644 --- a/src/send/mod.rs +++ b/src/send/mod.rs @@ -1595,15 +1595,24 @@ impl Client { ) .await; } else { - // Nothing stored, so the next call is a MissAbsent by - // construction rather than by eviction. Counted apart - // so that distinction survives into the report. + // 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, @@ -3378,9 +3387,9 @@ mod tests { /// /// `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 in regime gets — 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 + /// 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 @@ -3610,7 +3619,7 @@ mod tests { // 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(["15555550123"]); + 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!( From 7c4f21bc6589c967b38ca58ae925403ad6a97db5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 01:55:56 +0000 Subject: [PATCH 5/7] docs(perf): correct the not_stored claim in the observability guide too The counter's own doc was fixed last commit; this guide still promised the next call would be `miss_absent`, which would have operators reading a run of `not_stored` as eviction pressure instead of a group that never settles. Also documents `resolve_failed`, which the same round added. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016uk5eo5FdJfBQeMQBJWRo8 --- agent_docs/observability.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/agent_docs/observability.md b/agent_docs/observability.md index d301dacc4..8a3bcccfc 100644 --- a/agent_docs/observability.md +++ b/agent_docs/observability.md @@ -124,8 +124,20 @@ that the group memo returned, so **a group-memo recompute forces 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 and the *next* call is a -`miss_absent` by construction, not by eviction). +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 From 274e9e732ba57587b6f8232fba7db127dce68a9a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 02:00:47 +0000 Subject: [PATCH 6/7] docs(perf): count the SKDM memo's five miss counters, not four The four stale terms are not the whole story: the entry-absent condition is a fifth thing the report distinguishes, and the group half already listed "entry present" as one of its own. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016uk5eo5FdJfBQeMQBJWRo8 --- agent_docs/observability.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/agent_docs/observability.md b/agent_docs/observability.md index 8a3bcccfc..316b4da3f 100644 --- a/agent_docs/observability.md +++ b/agent_docs/observability.md @@ -113,8 +113,9 @@ group send depends on, `resolve_group_devices_memoized` and 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 -(device `Arc`, sender-key-map `Arc`, map generation, sending identity). An +— 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` From 70f5ca9717cc51025bbff2cb39b7e676a3cd236d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 02:08:37 +0000 Subject: [PATCH 7/7] docs(perf): describe hit_rate as memo hits over every resolver call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding `resolve_failed` to the denominator made the old wording — "share of calls that skipped filter_skdm_targets" — wrong: a call that errored before the memo lookup skipped the filter too. Keeping those in the denominator is the point, so the rate sags when group sends start failing instead of climbing; the doc now says that rather than the opposite. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016uk5eo5FdJfBQeMQBJWRo8 --- src/client/device_memo_stats.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/client/device_memo_stats.rs b/src/client/device_memo_stats.rs index 0f2ce58a8..e08e99908 100644 --- a/src/client/device_memo_stats.rs +++ b/src/client/device_memo_stats.rs @@ -317,8 +317,14 @@ impl SkdmTargetsMemoStats { + self.resolve_failed } - /// Share of calls that skipped `filter_skdm_targets`. `None` when nothing - /// was resolved yet. + /// 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)