diff --git a/src-tauri/crates/mhost-core/src/models.rs b/src-tauri/crates/mhost-core/src/models.rs index 70673e2..be09308 100644 --- a/src-tauri/crates/mhost-core/src/models.rs +++ b/src-tauri/crates/mhost-core/src/models.rs @@ -260,6 +260,28 @@ pub struct AdBlockSource { /// accepts documents written before the field existed. #[serde(default)] pub rules_limit_override: Option, + + /// Wall-clock duration of the last fetch (issue #199 sub-task B). + /// Recorded on every call to `fetch_and_cache_source` regardless + /// of success / failure, so the UI's refresh-status panel can + /// show "last refresh took 1.4 s" alongside the timestamp. + /// + /// Issue #202 lesson — always serialized (never `undefined`), + /// `#[serde(default)]` for back-compat with documents written + /// before this field existed. + #[serde(default)] + pub last_refresh_duration_ms: Option, + + /// RFC 3339 timestamp of the last *failed* fetch (issue #199 + /// sub-task B). Distinct from `last_error` (which carries the + /// message of the most recent failure regardless of when) so + /// the UI can compute a failure-rate over time. Cleared on + /// the next successful fetch (success erases the failure). + /// + /// Issue #202 lesson — always serialized; `#[serde(default)]` + /// for back-compat. + #[serde(default)] + pub last_refresh_failed_at: Option>, } /// Persistent state for the DNS-mode ad block subsystem. @@ -870,6 +892,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }; let json = serde_json::to_string(&source).unwrap(); assert!(json.contains("\"last_fetched_at\":null"), "{}", json); @@ -893,6 +917,8 @@ mod tests { rule_count: 42, etag: Some("W/\"abc\"".to_string()), rules_limit_override: Some(612_003), + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }; let json = serde_json::to_string(&source).unwrap(); assert!(json.contains("last_fetched_at")); @@ -927,6 +953,8 @@ mod tests { rule_count: 100, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }], whitelist: vec!["trusted.example.com".to_string()], auto_refresh_enabled: true, @@ -954,6 +982,8 @@ mod tests { rule_count: 612_003, etag: None, rules_limit_override: Some(612_003), + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }; let json = serde_json::to_string(&source).unwrap(); assert!(json.contains("\"rules_limit_override\":612003"), "{}", json); diff --git a/src-tauri/crates/mhost-dns/src/adblock.rs b/src-tauri/crates/mhost-dns/src/adblock.rs index 6ba90de..b7c5000 100644 --- a/src-tauri/crates/mhost-dns/src/adblock.rs +++ b/src-tauri/crates/mhost-dns/src/adblock.rs @@ -19,6 +19,7 @@ use parking_lot::RwLock; use std::collections::{HashMap, HashSet}; use std::net::IpAddr; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::Arc; use crate::matcher::walk_parents; @@ -78,17 +79,69 @@ impl RulesSnapshot { /// snapshot (issue #132). `check` takes the read lock only long enough to /// clone the `Arc` (refcount bump), then walks the immutable snapshot /// lock-free. +/// +/// Snapshot of [`AdBlockEngine`] counters, returned by +/// [`AdBlockEngine::stats`]. Cumulative since process start — the +/// engine has no notion of "reset to zero" because the foreground +/// reads (`get_ad_block_stats` IPC) are pull-based and the consumer +/// computes its own deltas. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct AdBlockStats { + pub hits_zero_addr: u64, + pub hits_nxdomain: u64, + pub hits_whitelist: u64, + pub misses: u64, +} + pub struct AdBlockEngine { current: RwLock>, + /// Master switch (issue #199 sub-task B): mirrored from + /// `state.enabled` by `reload_ad_block_rules`. `check()` reads this + /// to decide whether `misses` should accumulate — the issue's + /// contract is "misses counts only when the master switch is on", + /// and the caller cannot easily thread `state.enabled` through the + /// DNS hot path (issue #199). + enabled: AtomicBool, + /// Hit counters (issue #199 sub-task B). Lock-free so the DNS hot + /// path never blocks; `Relaxed` ordering is fine because we only + /// care about per-counter monotonic accumulation, not cross-counter + /// consistency. The four counters together describe every outcome + /// `check()` can produce when `enabled == true`. + /// + /// * `hits_zero_addr` — matched a `ZeroAddress` source. + /// * `hits_nxdomain` — matched an `NxDomain` source. + /// * `hits_whitelist` — matched a whitelist entry (bypassed the + /// block layer, fell through to the regular rule engine). + /// * `misses` — master switch on, no rule matched, no whitelist + /// match. This is the "would have been blocked if we had a + /// rule" signal; a purely informational volume metric, not a + /// correctness check. + hits_zero_addr: AtomicU64, + hits_nxdomain: AtomicU64, + hits_whitelist: AtomicU64, + misses: AtomicU64, } impl AdBlockEngine { pub fn new() -> Self { Self { current: RwLock::new(Arc::new(RulesSnapshot::default())), + enabled: AtomicBool::new(false), + hits_zero_addr: AtomicU64::new(0), + hits_nxdomain: AtomicU64::new(0), + hits_whitelist: AtomicU64::new(0), + misses: AtomicU64::new(0), } } + /// Mirror the master switch onto the engine. Called from + /// `reload_ad_block_rules`; safe to call independently (used by + /// tests that exercise the counter accounting without rebuilding + /// the full rule set). + pub fn set_enabled(&self, enabled: bool) { + self.enabled.store(enabled, Ordering::Relaxed); + } + /// Atomically swap in new rule sets. /// /// Builds the three sets into one [`RulesSnapshot`] and replaces the @@ -131,33 +184,69 @@ impl AdBlockEngine { /// Decide what to do with a query. /// - /// Returns `None` if the domain is whitelisted (fall through to the - /// regular rule engine / upstream) or not blocked at all. + /// Returns `None` if the domain is whitelisted (fall through to + /// the regular rule engine / upstream) or not blocked at all. + /// + /// **Counter accounting (issue #199 sub-task B):** when the + /// master switch is on, exactly one of the four counters + /// advances per call: + /// + /// * `hits_whitelist` — whitelist matched → fall through + /// * `hits_nxdomain` — NXDOMAIN rule matched + /// * `hits_zero_addr` — zero-address rule matched + /// * `misses` — none of the above + /// + /// When the master switch is off, no counter advances (issue + /// contract: "misses 累加（不要把 whitelist 命中算 miss）" — by + /// extension, nothing else counts either, because the user has + /// not opted in to ad blocking). With master off AND no block + /// rules loaded AND no whitelist match, the call returns `None` + /// without touching any counter — there is nothing meaningful + /// to record. pub fn check(&self, domain: &str) -> Option { let snap = self.snapshot(); - // Fast-path: no block rules loaded → no possible hit. Avoids any - // domain walking for the common `state.enabled == false` case. - // Whitelist is excluded because it's collected regardless of the - // master switch (review Medium #2); an empty block-rule set means - // `check()` can only return `None`. Unlike the old `AtomicUsize` - // short-circuit this reads the very snapshot the walk below uses, - // so the empty-check can't disagree with the rule data (issue #132). - if !snap.has_block_rules() { + + // Master switch off → no counters, no work. The whitelist + // walk is skipped because the caller always falls through on + // `None`, and `classify_rules` only feeds block rules to the + // engine when master is on, so a whitelist hit while master + // is off produces no observable behaviour change. + if !self.enabled.load(Ordering::Relaxed) { return None; } - // 1. whitelist (read once, then release) + // Whitelist first — wins over both block-rule sets + // (whitelist collected regardless of master switch). A + // whitelist hit counts toward `hits_whitelist` and returns + // `None` to let the regular rule engine / upstream handle + // the query. if walk_parents(domain, |d| snap.whitelist.contains(d).then_some(())).is_some() { + self.hits_whitelist.fetch_add(1, Ordering::Relaxed); + return None; + } + + // No block rules loaded → no possible hit beyond whitelist. + // We deliberately do NOT count this as a miss: with no block + // rules, "no match" is the empty answer, not a "should have + // been blocked" signal. (Master switch is on, so we still + // walked the whitelist above and incremented if it matched.) + if !snap.has_block_rules() { return None; } - // 2. NXDOMAIN sources first — more aggressive, save a hashmap lookup + + // NXDOMAIN sources first — more aggressive, save a hashmap lookup if walk_parents(domain, |d| snap.nxdomain.contains(d).then_some(())).is_some() { + self.hits_nxdomain.fetch_add(1, Ordering::Relaxed); return Some(AdBlockAction::NxDomain); } - // 3. zero-address sources + // zero-address sources if let Some(ip) = walk_parents(domain, |d| snap.zero_addr.get(d).copied()) { + self.hits_zero_addr.fetch_add(1, Ordering::Relaxed); return Some(AdBlockAction::ZeroAddress(ip)); } + + // Master switch on, no whitelist hit, no block rule → genuine miss. + self.misses.fetch_add(1, Ordering::Relaxed); None } @@ -167,6 +256,34 @@ impl AdBlockEngine { self.snapshot().total() } + /// Snapshot the four counters in one place so the + /// `get_ad_block_stats` IPC returns a value-typed struct instead + /// of four separate IPC calls. `Relaxed` loads are fine — each + /// counter is independently monotonically increasing. + pub fn stats(&self) -> AdBlockStats { + AdBlockStats { + hits_zero_addr: self.hits_zero_addr.load(Ordering::Relaxed), + hits_nxdomain: self.hits_nxdomain.load(Ordering::Relaxed), + hits_whitelist: self.hits_whitelist.load(Ordering::Relaxed), + misses: self.misses.load(Ordering::Relaxed), + } + } + + /// Issue #199 sub-task B (PR #219 review follow-up): the + /// master-switch state mirrored onto the engine. The IPC + /// `get_ad_block_stats` reads this directly instead of going + /// through `ad_block_state` — the engine's AtomicBool is + /// the value that actually gates `check()`, so it's the + /// authoritative source for the "is the engine currently + /// classifying queries?" UI label. Avoids the narrow race + /// window where `state.enabled` has been written by + /// `set_ad_block_enabled` but the engine's AtomicBool hasn't + /// been mirrored yet (persist_and_reload writes state, + /// THEN mirrors onto engine in a separate step). + pub fn is_enabled(&self) -> bool { + self.enabled.load(Ordering::Relaxed) + } + pub fn whitelist_size(&self) -> usize { self.snapshot().whitelist.len() } @@ -237,6 +354,7 @@ mod tests { fn zero_address_hit_returns_zero_address() { let engine = AdBlockEngine::new(); engine.rebuild(za(&["ad.example.com"]), nx(&[]), wl(&[])); + engine.set_enabled(true); let action = engine.check("ad.example.com"); assert_eq!( action, @@ -250,6 +368,7 @@ mod tests { fn nxdomain_hit_returns_nxdomain() { let engine = AdBlockEngine::new(); engine.rebuild(za(&[]), nx(&["tracker.example.com"]), wl(&[])); + engine.set_enabled(true); assert_eq!( engine.check("tracker.example.com"), Some(AdBlockAction::NxDomain) @@ -261,6 +380,7 @@ mod tests { // ad-blocker semantics: registering example.com hits *.example.com let engine = AdBlockEngine::new(); engine.rebuild(za(&["example.com"]), nx(&[]), wl(&[])); + engine.set_enabled(true); for d in ["example.com", "ad.example.com", "deep.ad.example.com"] { assert!( matches!(engine.check(d), Some(AdBlockAction::ZeroAddress(_))), @@ -284,6 +404,7 @@ mod tests { nx(&["example.com"]), wl(&[]), ); + engine.set_enabled(true); // The parent NXDOMAIN wins because it's consulted first. assert_eq!( engine.check("specific.ad.example.com"), @@ -307,6 +428,7 @@ mod tests { nx(&["example.com"]), wl(&["good.example.com"]), ); + engine.set_enabled(true); // whitelist exact hit assert_eq!(engine.check("good.example.com"), None); // whitelist suffix hit @@ -319,11 +441,13 @@ mod tests { fn rebuild_replaces_state_atomically() { let engine = AdBlockEngine::new(); engine.rebuild(za(&["a.com"]), nx(&["b.com"]), wl(&["c.com"])); + engine.set_enabled(true); assert_eq!(engine.zero_addr_count(), 1); assert_eq!(engine.nxdomain_count(), 1); assert_eq!(engine.whitelist_size(), 1); engine.rebuild(za(&["d.com", "e.com"]), nx(&[]), wl(&[])); + engine.set_enabled(true); assert_eq!(engine.zero_addr_count(), 2); assert_eq!(engine.nxdomain_count(), 0); assert_eq!(engine.whitelist_size(), 0); @@ -345,6 +469,7 @@ mod tests { nx(&["c.com", "d.com", "e.com"]), wl(&[]), ); + engine.set_enabled(true); assert_eq!(engine.rule_count(), 5); } @@ -357,6 +482,7 @@ mod tests { fn rule_count_includes_whitelist() { let engine = AdBlockEngine::new(); engine.rebuild(za(&["a.com"]), nx(&[]), wl(&["w1", "w2", "w3"])); + engine.set_enabled(true); assert_eq!(engine.rule_count(), 4); } @@ -406,6 +532,7 @@ mod tests { // End-to-end tie-in: behaviour is unchanged by the optimisation. let engine = AdBlockEngine::new(); engine.rebuild(za(&[]), nx(&[]), wl(&["trusted.com"])); + engine.set_enabled(true); assert_eq!(engine.check("trusted.com"), None); assert_eq!(engine.rule_count(), 1); } @@ -417,9 +544,11 @@ mod tests { fn rebuild_updates_cached_total() { let engine = AdBlockEngine::new(); engine.rebuild(za(&["a.com"]), nx(&[]), wl(&[])); + engine.set_enabled(true); assert_eq!(engine.rule_count(), 1); engine.rebuild(za(&[]), nx(&[]), wl(&[])); + engine.set_enabled(true); assert_eq!( engine.rule_count(), 0, @@ -436,6 +565,7 @@ mod tests { // `.xyz` TLD used by abuse). let engine = AdBlockEngine::new(); engine.rebuild(za(&["com"]), nx(&[]), wl(&[])); + engine.set_enabled(true); assert!(matches!( engine.check("example.com"), Some(AdBlockAction::ZeroAddress(_)) @@ -447,4 +577,141 @@ mod tests { // A different TLD is untouched. assert_eq!(engine.check("example.org"), None); } + + // ----------------------------------------------------------------- + // Issue #199 sub-task B: hit / miss / per-action counters. + // ----------------------------------------------------------------- + + /// Mixed whitelist + zero-addr + nxdomain + miss traffic + /// accumulates into exactly the right counter on each call. + /// Drives the engine with `set_enabled(true)` first because + /// master switch off is a no-op (verified separately below). + #[test] + fn counter_accounting_for_mixed_traffic() { + let engine = AdBlockEngine::new(); + engine.rebuild( + za(&["ad.example.com"]), + nx(&["tracker.example.com"]), + wl(&["good.example.com"]), + ); + engine.set_enabled(true); + + // 2 zero-addr hits, 2 nxdomain hits, 9 whitelist hits (3 queries + // × 3 iters), 5 misses. + for _ in 0..2 { + assert_eq!( + engine.check("ad.example.com"), + Some(AdBlockAction::ZeroAddress(IpAddr::V4(Ipv4Addr::new( + 0, 0, 0, 0 + )))) + ); + assert_eq!( + engine.check("tracker.example.com"), + Some(AdBlockAction::NxDomain) + ); + } + for _ in 0..3 { + // exact + suffix whitelist hits; all count toward hits_whitelist + assert_eq!(engine.check("good.example.com"), None); + assert_eq!(engine.check("api.good.example.com"), None); + assert_eq!(engine.check("good.example.com"), None); + } + for _ in 0..5 { + // not in any set → miss + assert_eq!(engine.check("untouched.example.org"), None); + } + + let s = engine.stats(); + assert_eq!(s.hits_zero_addr, 2, "zero-addr hits counted"); + assert_eq!(s.hits_nxdomain, 2, "nxdomain hits counted"); + assert_eq!( + s.hits_whitelist, 9, + "whitelist hits counted (3 queries × 3 iters)" + ); + assert_eq!(s.misses, 5, "misses counted"); + } + + /// Whitelist hits do NOT count as misses (issue #199 contract: + /// "misses 累加, 不要把 whitelist 命中算 miss"). + #[test] + fn whitelist_hit_is_not_a_miss() { + let engine = AdBlockEngine::new(); + engine.rebuild(za(&[]), nx(&[]), wl(&["safe.example.com"])); + engine.set_enabled(true); + + for _ in 0..10 { + assert_eq!(engine.check("safe.example.com"), None); + } + let s = engine.stats(); + assert_eq!(s.hits_whitelist, 10); + assert_eq!(s.misses, 0, "whitelist hits must not count as misses"); + assert_eq!(s.hits_zero_addr, 0); + assert_eq!(s.hits_nxdomain, 0); + } + + /// Master switch off: no counters advance, regardless of rule + /// matches. The hot path short-circuits in `check()` before + /// any rule walking, so even the whitelist walk is skipped + /// (callers can't observe the difference; see `check()` docs). + #[test] + fn master_switch_off_does_not_advance_any_counter() { + let engine = AdBlockEngine::new(); + engine.rebuild( + za(&["ad.example.com"]), + nx(&["tracker.example.com"]), + wl(&["good.example.com"]), + ); + // Note: NO `set_enabled(true)`. Default is off. + + // `check()` returns None for everything; counters don't move. + for _ in 0..5 { + assert_eq!(engine.check("ad.example.com"), None); + assert_eq!(engine.check("tracker.example.com"), None); + assert_eq!(engine.check("good.example.com"), None); + assert_eq!(engine.check("untouched.example.org"), None); + } + let s = engine.stats(); + assert_eq!(s.hits_zero_addr, 0, "master off: zero-addr counter idle"); + assert_eq!(s.hits_nxdomain, 0, "master off: nxdomain counter idle"); + assert_eq!(s.hits_whitelist, 0, "master off: whitelist counter idle"); + assert_eq!(s.misses, 0, "master off: miss counter idle"); + } + + /// stats() can be called concurrently with check(); the four + /// atomic loads are independent and the result is a coherent + /// snapshot at some moment in the call. + #[test] + fn stats_can_be_read_concurrently_with_check() { + use std::sync::Arc; + use std::thread; + + let engine = Arc::new(AdBlockEngine::new()); + engine.rebuild(za(&["ad.example.com"]), nx(&[]), wl(&[])); + engine.set_enabled(true); + + let engine_writer = Arc::clone(&engine); + let writer = thread::spawn(move || { + for _ in 0..1000 { + let _ = engine_writer.check("ad.example.com"); + } + }); + + // While the writer thread runs, hammer stats() — we don't + // assert specific numbers (race-y), only that reads don't + // panic and don't deadlock the writer. + let mut reads = 0; + while !writer.is_finished() { + let _ = engine.stats(); + reads += 1; + if reads > 100_000 { + break; + } + } + writer.join().unwrap(); + let final_stats = engine.stats(); + assert_eq!(final_stats.hits_zero_addr, 1000); + assert_eq!(final_stats.hits_nxdomain, 0); + assert_eq!(final_stats.hits_whitelist, 0); + assert_eq!(final_stats.misses, 0); + } } diff --git a/src-tauri/crates/mhost-dns/src/server.rs b/src-tauri/crates/mhost-dns/src/server.rs index 33b6444..0aac6bc 100644 --- a/src-tauri/crates/mhost-dns/src/server.rs +++ b/src-tauri/crates/mhost-dns/src/server.rs @@ -350,12 +350,22 @@ impl DnsServer { /// 与 `reload_rules` 同等语义：rebuild 引擎后清空 LRU 缓存， /// 否则 reload 前向上游查过并缓存的域名仍会返回 stale upstream IP， /// 覆盖新的 ad-block 命中（issue #132 follow-up）。 + /// + /// **Issue #199 sub-task B:** `enabled` is mirrored onto the + /// engine so `check()` can decide whether `misses` should + /// accumulate (issue contract: "misses 累加，不要把 whitelist + /// 命中算 miss" — by extension, nothing else counts + /// when master is off). The caller always knows the master + /// switch value at reload time, so we don't need a separate + /// `set_enabled` call from the hot reload's caller. pub fn reload_ad_block_rules( &self, + enabled: bool, zero_addr_rules: std::collections::HashMap, nxdomain_rules: std::collections::HashSet, whitelist: std::collections::HashSet, ) { + self.ad_block_engine.set_enabled(enabled); self.ad_block_engine .rebuild(zero_addr_rules, nxdomain_rules, whitelist); self.cache.lock().clear(); @@ -371,6 +381,26 @@ impl DnsServer { self.ad_block_engine.whitelist_size() } + /// Issue #199 sub-task B: cumulative ad-block hit / miss + /// counters, returned for the `get_ad_block_stats` IPC. Delegates + /// to the engine — cheap `Relaxed` loads on the four + /// `AtomicU64`s. The counters are cumulative since process start + /// (no reset path); the consumer computes deltas. + pub fn ad_block_stats(&self) -> crate::adblock::AdBlockStats { + self.ad_block_engine.stats() + } + + /// Issue #199 sub-task B (PR #219 review follow-up): the + /// engine's mirrored master switch (see + /// `AdBlockEngine::is_enabled`). Used by `get_ad_block_stats` + /// to read the gating state from the engine rather than + /// from `ad_block_state` — closes the narrow race window + /// where `state.enabled` has been written but the engine's + /// AtomicBool hasn't been mirrored yet. + pub fn ad_block_enabled(&self) -> bool { + self.ad_block_engine.is_enabled() + } + /// 测试用：直接拿到 AdBlockEngine。 #[doc(hidden)] pub fn ad_block_engine_for_test(&self) -> Arc { @@ -1941,7 +1971,10 @@ mod tests { nxdomain.insert("blocked.example.com".to_string()); let mut whitelist = HashSet::new(); whitelist.insert("safe.example.com".to_string()); - server.reload_ad_block_rules(zero_addr, nxdomain, whitelist); + // Master switch on — the test queries ZeroAddress, + // NxDomain and whitelist paths, all of which require + // `enabled == true` after issue #199 sub-task B. + server.reload_ad_block_rules(true, zero_addr, nxdomain, whitelist); // 3. 启动 server 并查询四种场景 let s = Arc::clone(&server); diff --git a/src-tauri/crates/mhost-storage/src/adblock.rs b/src-tauri/crates/mhost-storage/src/adblock.rs index e20301b..ec6fbe3 100644 --- a/src-tauri/crates/mhost-storage/src/adblock.rs +++ b/src-tauri/crates/mhost-storage/src/adblock.rs @@ -284,6 +284,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, } } diff --git a/src-tauri/src/commands/adblock.rs b/src-tauri/src/commands/adblock.rs index f0120ad..3b10755 100644 --- a/src-tauri/src/commands/adblock.rs +++ b/src-tauri/src/commands/adblock.rs @@ -316,7 +316,10 @@ pub(crate) async fn persist_and_reload(state: &AppState) -> Result<(), MhostErro // (See issue #138: spawn_blocking cannot be aborted.) if !cancel.is_cancelled() { if let Some(server) = lock_or_recover(&dns_server).as_ref() { - server.reload_ad_block_rules(zero_addr, nxdomain, whitelist); + // Issue #199 sub-task B: pass the master + // switch to the engine so `check()` can + // decide whether misses should accumulate. + server.reload_ad_block_rules(snapshot.enabled, zero_addr, nxdomain, whitelist); } } } @@ -547,6 +550,17 @@ pub(crate) async fn fetch_and_cache_source( let gate = acquire_source_refresh_gate(source_id).await; let _gate_guard = gate.lock().await; + // Issue #199 sub-task B: time the fetch so the UI can show + // "last refresh took N ms" per source. The timer starts + // right after the per-source gate is acquired — *not* + // including the queue wait (which can be variable on a + // contended refresh, see issue #206 finding 1), only the + // HTTP fetch + parse + in-memory bookkeeping. This is what + // the user perceives as the refresh cost on the source + // itself; queue wait is a separate concern for a future + // "queue depth" metric. + let started_at = std::time::Instant::now(); + // 1. Read the source record under the read lock. We capture both // the URL (for the fetch) and the previous ETag / last_fetched_at // (for the conditional GET — issue #193), plus the effective @@ -653,6 +667,13 @@ pub(crate) async fn fetch_and_cache_source( .await .map_err(|e| MhostError::Network(format!("fetch task failed: {}", e)))?; + // Issue #199 sub-task B: capture elapsed wall-clock time once, + // reuse for all three branches (success / 304 / error). We use + // `u64` so the field stays `Option`; `Duration::as_millis` + // returns `u128`, but `u64::MAX` ms is ~584 million years so the + // narrowing cast is safe. + let elapsed_ms = started_at.elapsed().as_millis() as u64; + match fetch_parse { Ok(Parsed::Fresh { rule_count, etag }) => { // 200 OK path — full update: clear error, set fetched_at, @@ -663,6 +684,11 @@ pub(crate) async fn fetch_and_cache_source( s.last_fetched_at = Some(Utc::now()); s.rule_count = rule_count; s.etag = etag; + s.last_refresh_duration_ms = Some(elapsed_ms); + // Successful fetch clears any prior failure timestamp + // (issue #199 sub-task B): a fresh success makes the + // "last failed at" meaningless. + s.last_refresh_failed_at = None; } Ok(()) } @@ -672,18 +698,27 @@ pub(crate) async fn fetch_and_cache_source( // look stale, and we want to clear any prior `last_error` // since we just successfully round-tripped the upstream. // `rule_count` and `etag` are intentionally NOT touched — - // they continue to reflect the cached payload. + // they continue to reflect the cached payload. Same for + // `last_refresh_failed_at`: cleared because the upstream + // just acknowledged we are current. let mut guard = ad_block_state.write().await; if let Some(s) = adblock_store::find_source_mut(&mut guard, source_id) { s.last_error = None; s.last_fetched_at = Some(Utc::now()); + s.last_refresh_duration_ms = Some(elapsed_ms); + s.last_refresh_failed_at = None; } Ok(()) } Err(e) => { // Network / size / parse failure: record on `last_error`, // keep the previous cache intact for DNS to keep serving. - record_fetch_error(ad_block_state, source_id, &e.to_string()).await?; + // Issue #199 sub-task B: also stamp the timing and + // failure-timestamp fields so the UI's refresh panel can + // show "last refresh took 30 s (timeout)" and the user + // can see how long ago the last failure was. + record_fetch_error_with_timing(ad_block_state, source_id, &e.to_string(), elapsed_ms) + .await?; Err(e) } } @@ -709,6 +744,26 @@ pub(crate) async fn record_fetch_error( Ok(()) } +/// Same as [`record_fetch_error`] but also stamps the issue #199 +/// sub-task B timing fields: `last_refresh_duration_ms` and +/// `last_refresh_failed_at`. Used by the `Err` branch of +/// `fetch_and_cache_source` so a failed fetch still surfaces +/// how long it took and when it happened. +pub(crate) async fn record_fetch_error_with_timing( + ad_block_state: &Arc>, + source_id: &SourceId, + err: &str, + elapsed_ms: u64, +) -> Result<(), MhostError> { + let mut guard = ad_block_state.write().await; + if let Some(s) = adblock_store::find_source_mut(&mut guard, source_id) { + s.last_error = Some(err.to_string()); + s.last_refresh_duration_ms = Some(elapsed_ms); + s.last_refresh_failed_at = Some(Utc::now()); + } + Ok(()) +} + /// Format `dt` as an RFC 7231 IMF-fixdate string for use in the /// `If-Modified-Since` request header (issue #193). Example output: /// `Sun, 06 Nov 1994 08:49:37 GMT`. @@ -732,6 +787,26 @@ fn rfc7231_date(dt: chrono::DateTime) -> String { /// frontend mirror risks silent drift; the authoritative values live here /// and the UI fetches them. `rules_per_source_default` is informational /// (shown as "default cap" context). +/// Issue #199 sub-task B: view-model for the engine counters + master +/// switch exposed via `get_ad_block_stats`. Mirrors +/// `mhost_dns::adblock::AdBlockStats` (the engine-side struct) and +/// adds the current master-switch flag so the frontend can label +/// the panel correctly ("stats while master switch off" is a +/// useful UI hint). +#[derive(Debug, Clone, Serialize)] +pub struct AdBlockStatsView { + pub hits_zero_addr: u64, + pub hits_nxdomain: u64, + pub hits_whitelist: u64, + pub misses: u64, + /// Whether the master switch is on at the moment of the call. + /// `false` means the engine is parked; consumers should still + /// show the cumulative numbers but tag the panel as + /// "master switch off" so the absence of new hits isn't + /// surprising. + pub enabled: bool, +} + #[derive(Debug, Clone, Serialize)] pub struct AdBlockLimits { /// Global default cap applied when a source has no @@ -758,6 +833,53 @@ pub async fn get_ad_block_state(state: State<'_, AppState>) -> Result, +) -> Result { + // Issue #199 sub-task B (PR #219 review follow-up): both the + // counters AND the master switch come from the engine, not + // from `ad_block_state`. The engine's AtomicBool is the value + // that actually gates `check()`; `state.enabled` can lag + // during a concurrent `set_ad_block_enabled` (which writes + // state THEN mirrors onto the engine in two steps inside + // `persist_and_reload`). Reading both from the engine closes + // the race where the response's `enabled` label disagrees + // with the engine's gating state for the duration of one + // mid-call reload. When DNS mode is off (no server loaded) + // we report zeros and `enabled=false` — there's no engine + // to read from, so the absence is the answer. + let (stats, enabled) = { + let guard = lock_or_recover(&state.dns_server); + match guard.as_ref() { + Some(server) => (server.ad_block_stats(), server.ad_block_enabled()), + None => ( + mhost_dns::adblock::AdBlockStats { + hits_zero_addr: 0, + hits_nxdomain: 0, + hits_whitelist: 0, + misses: 0, + }, + false, + ), + } + }; + Ok(AdBlockStatsView { + hits_zero_addr: stats.hits_zero_addr, + hits_nxdomain: stats.hits_nxdomain, + hits_whitelist: stats.hits_whitelist, + misses: stats.misses, + enabled, + }) +} + /// Master switch. Disabling also clears the engine's rule sets via /// `persist_and_reload` (which classifies with `enabled=false` → empty). #[tauri::command] @@ -883,6 +1005,8 @@ pub(crate) async fn add_ad_block_source_impl( rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }; let new_id = new_source.source_id.clone(); @@ -1406,6 +1530,8 @@ mod tests { rule_count: 1, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); let (z, n, w) = classify_rules(&state, temp.path()); assert!(z.is_empty()); @@ -1427,6 +1553,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }; let za_source = mk("za", AdBlockResponse::ZeroAddress, true); let nx_source = mk("nx", AdBlockResponse::NxDomain, true); @@ -1692,6 +1820,8 @@ mod tests { rule_count: 2, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }; let za_source = mk("za", AdBlockResponse::ZeroAddress); let nx_source = mk("nx", AdBlockResponse::NxDomain); @@ -1753,6 +1883,8 @@ mod tests { rule_count: 1, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }; mhost_storage::adblock::write_cache( temp.path(), @@ -1782,7 +1914,10 @@ mod tests { cache_size: 100, }; let server = std::sync::Arc::new(mhost_dns::DnsServer::new(config).unwrap()); - server.reload_ad_block_rules(za, nx, wl); + // Master switch on — the next assertion is `rule_count == 1` + // which depends on the rebuild going through with rules + // actually fed into the engine. + server.reload_ad_block_rules(true, za, nx, wl); assert_eq!(server.ad_block_rule_count(), 1); let server_clone = std::sync::Arc::clone(&server); @@ -2560,6 +2695,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } @@ -2687,6 +2824,8 @@ mod tests { rule_count: 0, etag: Some("\"v0-stale\"".to_string()), rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } // Plant a stale cache file so we can assert it gets overwritten. @@ -2762,6 +2901,8 @@ mod tests { rule_count: 7, etag: None, // no etag → If-None-Match will be omitted rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } // Issue #206 finding 2: the 304 path now verifies the cache file @@ -2905,6 +3046,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } @@ -2984,6 +3127,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: Some(2), // below the 3-domain list + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } @@ -3054,6 +3199,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } @@ -3128,6 +3275,8 @@ mod tests { rule_count: 7, // stale bookkeeping from a wiped cache etag: Some("\"v1\"".to_string()), rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } assert!( @@ -3205,6 +3354,8 @@ mod tests { rule_count: 7, // stale bookkeeping from a wiped cache etag: Some("\"v1\"".to_string()), rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } @@ -3284,6 +3435,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } @@ -3369,6 +3522,8 @@ mod tests { rule_count: 1, etag: Some("\"v1\"".to_string()), rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } // Local cache exists but is stale (e.g. canonicalized by an old @@ -3428,6 +3583,8 @@ mod tests { rule_count: 3, etag: Some("\"v1\"".into()), rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } // Pretend the source has a populated cache on disk. @@ -3495,6 +3652,8 @@ mod tests { rule_count: 99, etag: Some("\"stale\"".into()), rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } // Confirm the "post-disable" precondition: cache file gone. @@ -3568,6 +3727,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }); } mhost_storage::adblock::write_cache( @@ -3614,4 +3775,440 @@ mod tests { let stored = mhost_storage::adblock::find_source(&snap, &source_id).unwrap(); assert!(stored.enabled, "source stays enabled"); } + + // ----------------------------------------------------------------- + // Issue #199 sub-task B (PR #219 review follow-ups): test gaps. + // + // The four counter / timing pieces added in #199-B were unit-tested + // end-to-end (engine counters) but the specific helpers and the IPC + // surface itself were only smoke-tested via existing tests. These + // tests pin down the contract for each piece so future refactors + // don't regress it. + // ----------------------------------------------------------------- + + /// Direct test for `record_fetch_error_with_timing`: all three + /// fields written atomically. The previous `record_fetch_error` + /// only wrote `last_error`; this helper adds the two timing + /// fields introduced in #199-B. + #[tokio::test] + async fn record_fetch_error_with_timing_writes_all_three_fields() { + let temp = tempfile::TempDir::new().unwrap(); + let (state, _storage) = make_test_app_state(temp.path()); + let source_id = SourceId(uuid::Uuid::new_v4()); + { + let mut g = state.ad_block_state.write().await; + g.sources.push(AdBlockSource { + source_id: source_id.clone(), + name: "timing-test".into(), + url: "https://x.example/list".into(), + enabled: true, + response: AdBlockResponse::ZeroAddress, + last_fetched_at: None, + last_error: None, + rule_count: 0, + etag: None, + rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, + }); + } + let before = chrono::Utc::now(); + record_fetch_error_with_timing(&state.ad_block_state, &source_id, "transport error", 4321) + .await + .expect("helper should succeed"); + let after = chrono::Utc::now(); + + let snap = state.ad_block_state.read().await; + let stored = mhost_storage::adblock::find_source(&snap, &source_id).unwrap(); + assert_eq!(stored.last_error.as_deref(), Some("transport error")); + assert_eq!(stored.last_refresh_duration_ms, Some(4321)); + let failed_at = stored + .last_refresh_failed_at + .expect("last_refresh_failed_at must be set"); + assert!( + failed_at >= before && failed_at <= after, + "last_refresh_failed_at {} should be in [{}, {}]", + failed_at, + before, + after + ); + } + + /// 200 OK branch: `fetch_and_cache_source` stamps + /// `last_refresh_duration_ms` and clears `last_refresh_failed_at`. + /// Uses `MockResponse::ok_200_delayed` to ensure the duration is + /// measurable (the mock sleeps before responding). + #[tokio::test] + async fn fetch_and_cache_source_200_writes_duration_and_clears_failure() { + use mhost_storage::storage::FileStorage; + + let listener = bind_mock_listener(); + let port = listener.local_addr().unwrap().port(); + let body = b"0.0.0.0 timed.example.com"; + let responses = + std::sync::Arc::new(std::sync::Mutex::new(std::collections::VecDeque::from( + vec![MockResponse::ok_200_delayed("\"v1\"", body, 100)], + ))); + let stop = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let (_h, _recorded) = spawn_mock_http(listener, responses, stop.clone()); + + let temp = tempfile::TempDir::new().unwrap(); + let storage = std::sync::Arc::new(FileStorage::new(temp.path())) + as std::sync::Arc; + let ad_block_state = std::sync::Arc::new(tokio::sync::RwLock::new(AdBlockState::default())); + let source_id = SourceId(uuid::Uuid::new_v4()); + { + let mut g = ad_block_state.write().await; + g.sources.push(AdBlockSource { + source_id: source_id.clone(), + name: "200-timing".into(), + url: format!("http://127.0.0.1:{}/list", port), + enabled: true, + response: AdBlockResponse::ZeroAddress, + last_fetched_at: None, + // Pre-seed a stale failure timestamp so the + // "successful fetch clears it" assertion is meaningful. + last_error: Some("prior offline failure".into()), + rule_count: 0, + etag: None, + rules_limit_override: None, + last_refresh_duration_ms: Some(50), + last_refresh_failed_at: Some(chrono::Utc::now() - chrono::Duration::hours(1)), + }); + } + + fetch_and_cache_source(&storage, &ad_block_state, &source_id, false) + .await + .expect("200 fetch should succeed"); + + let snap = ad_block_state.read().await; + let stored = mhost_storage::adblock::find_source(&snap, &source_id).unwrap(); + let duration = stored + .last_refresh_duration_ms + .expect("200 path must stamp duration"); + // Lower bound: the mock delayed 100 ms. Upper bound: 5 s, generous + // for CI jitter and spawn_blocking overhead. + assert!( + (80..=5_000).contains(&duration), + "duration {duration} ms should reflect the 100 ms mock delay" + ); + assert!( + stored.last_refresh_failed_at.is_none(), + "200 path must clear last_refresh_failed_at, got {:?}", + stored.last_refresh_failed_at + ); + assert!( + stored.last_error.is_none(), + "200 path must clear last_error, got {:?}", + stored.last_error + ); + assert_eq!(stored.rule_count, 1); + + stop_mock(&stop, _h); + } + + /// 304 branch: same timing + clear semantics as 200. Setup pins a + /// stale `last_refresh_failed_at` so the clearing assertion is + /// meaningful (not trivially-true). + #[tokio::test] + async fn fetch_and_cache_source_304_writes_duration_and_clears_failure() { + use mhost_storage::storage::FileStorage; + + let listener = bind_mock_listener(); + let port = listener.local_addr().unwrap().port(); + let responses = std::sync::Arc::new(std::sync::Mutex::new( + std::collections::VecDeque::from(vec![MockResponse::not_modified_304()]), + )); + let stop = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let (_h, _recorded) = spawn_mock_http(listener, responses, stop.clone()); + + let temp = tempfile::TempDir::new().unwrap(); + let storage = std::sync::Arc::new(FileStorage::new(temp.path())) + as std::sync::Arc; + let ad_block_state = std::sync::Arc::new(tokio::sync::RwLock::new(AdBlockState::default())); + let source_id = SourceId(uuid::Uuid::new_v4()); + { + let mut g = ad_block_state.write().await; + g.sources.push(AdBlockSource { + source_id: source_id.clone(), + name: "304-timing".into(), + url: format!("http://127.0.0.1:{}/list", port), + enabled: true, + response: AdBlockResponse::ZeroAddress, + // Pre-existing cache + ETag (RFC 7232 conditional GET + // requires a previous successful fetch). + last_fetched_at: Some(chrono::Utc::now() - chrono::Duration::hours(1)), + last_error: Some("prior offline failure".into()), + rule_count: 7, + etag: Some("\"v1\"".into()), + rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: Some(chrono::Utc::now() - chrono::Duration::minutes(5)), + }); + } + // Pre-existing cache so the 304 path's "no rewrite" contract + // (issue #193) is exercised. + mhost_storage::adblock::write_cache( + temp.path(), + &source_id, + b"0.0.0.0 still-alive.example.com", + ) + .unwrap(); + + fetch_and_cache_source(&storage, &ad_block_state, &source_id, false) + .await + .expect("304 fetch should succeed"); + + let snap = ad_block_state.read().await; + let stored = mhost_storage::adblock::find_source(&snap, &source_id).unwrap(); + // 304 path leaves rule_count + etag untouched (issue #193). + assert_eq!(stored.rule_count, 7); + assert_eq!(stored.etag.as_deref(), Some("\"v1\"")); + // BUT it stamps duration and clears prior failure. + assert!( + stored.last_refresh_duration_ms.is_some(), + "304 path must stamp duration, got {:?}", + stored.last_refresh_duration_ms + ); + assert!( + stored.last_refresh_failed_at.is_none(), + "304 path must clear last_refresh_failed_at" + ); + assert!( + stored.last_error.is_none(), + "304 path must clear last_error" + ); + stop_mock(&stop, _h); + } + + /// Err branch: a 5xx response triggers the `record_fetch_error_with_timing` + /// path. Asserts all three timing / failure fields are written. + #[tokio::test] + async fn fetch_and_cache_source_err_writes_duration_and_failure_timestamp() { + use mhost_storage::storage::FileStorage; + + let listener = bind_mock_listener(); + let port = listener.local_addr().unwrap().port(); + let responses = std::sync::Arc::new(std::sync::Mutex::new( + std::collections::VecDeque::from(vec![MockResponse { + status: 500, + headers: vec!["Content-Length: 0".to_string()], + body: Vec::new(), + delay_ms: 50, + }]), + )); + let stop = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let (_h, _recorded) = spawn_mock_http(listener, responses, stop.clone()); + + let temp = tempfile::TempDir::new().unwrap(); + let storage = std::sync::Arc::new(FileStorage::new(temp.path())) + as std::sync::Arc; + let ad_block_state = std::sync::Arc::new(tokio::sync::RwLock::new(AdBlockState::default())); + let source_id = SourceId(uuid::Uuid::new_v4()); + { + let mut g = ad_block_state.write().await; + g.sources.push(AdBlockSource { + source_id: source_id.clone(), + name: "err-timing".into(), + url: format!("http://127.0.0.1:{}/list", port), + enabled: true, + response: AdBlockResponse::ZeroAddress, + last_fetched_at: None, + last_error: None, + rule_count: 0, + etag: None, + rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, + }); + } + + let before = chrono::Utc::now(); + let err = fetch_and_cache_source(&storage, &ad_block_state, &source_id, false) + .await + .expect_err("500 fetch should fail"); + let after = chrono::Utc::now(); + assert!( + err.to_string().contains("500") || err.to_string().to_lowercase().contains("server"), + "error should mention the 5xx, got: {}", + err + ); + + let snap = ad_block_state.read().await; + let stored = mhost_storage::adblock::find_source(&snap, &source_id).unwrap(); + assert!( + stored.last_error.is_some(), + "err path must populate last_error" + ); + let duration = stored + .last_refresh_duration_ms + .expect("err path must stamp duration"); + assert!( + duration >= 50, + "duration {duration} ms should reflect the 50 ms mock delay" + ); + let failed_at = stored + .last_refresh_failed_at + .expect("err path must stamp last_refresh_failed_at"); + assert!( + failed_at >= before && failed_at <= after, + "last_refresh_failed_at {failed_at} should be in [{before}, {after}]" + ); + + stop_mock(&stop, _h); + } + + /// Legacy back-compat: an `adblock.json` written before the + /// #199-B timing fields existed must still deserialize, with the + /// new fields defaulting to `None`. Analog to + /// `test_ad_block_source_rules_limit_override_serde` for the + /// earlier (#207) field addition. + #[test] + fn adblock_state_legacy_doc_back_compat_for_199b_fields() { + let temp = tempfile::TempDir::new().unwrap(); + // Pre-#199-B document: no `last_refresh_duration_ms` / + // `last_refresh_failed_at` keys. The other fields are the + // pre-#207 shape too (no `rules_limit_override`) for + // completeness \u2014 demonstrates that BOTH additions + // back-compat cleanly. + let legacy = r#"{ + "enabled": true, + "sources": [ + { + "source_id": "00000000-0000-0000-0000-000000000001", + "name": "legacy", + "url": "https://x.example/list", + "enabled": true, + "response": "zero_address", + "last_fetched_at": null, + "last_error": null, + "rule_count": 42, + "etag": null + } + ], + "whitelist": [], + "auto_refresh_enabled": true, + "refresh_interval_hours": 6 + }"#; + let path = temp.path().join("adblock.json"); + std::fs::write(&path, legacy).unwrap(); + + let state = + mhost_storage::adblock::read_state(temp.path()).expect("legacy doc must deserialize"); + let src = &state.sources[0]; + assert_eq!(src.rule_count, 42); + // #199-B additions default to None. + assert_eq!(src.last_refresh_duration_ms, None); + assert_eq!(src.last_refresh_failed_at, None); + // #207 addition also defaults to None. + assert_eq!(src.rules_limit_override, None); + } + + /// Direct IPC test for `get_ad_block_stats`: spins up a real + /// `DnsServer`, fires some `check()` calls, calls the IPC, and + /// asserts the response shape. Closes the test-gap from the PR + /// #219 review. + #[tokio::test] + async fn get_ad_block_stats_returns_engine_counters_and_enabled() { + use mhost_dns::adblock::AdBlockAction; + use mhost_dns::DnsConfig; + use std::collections::{HashMap, HashSet}; + + let temp = tempfile::TempDir::new().unwrap(); + let (state, _storage) = make_test_app_state(temp.path()); + + // Build a real DnsServer + engine, reload with enabled=true + + // one zero-addr rule + one whitelist entry. + let config = DnsConfig { + port: pick_free_port(), + upstream: vec!["1.1.1.1".to_string()], + refresh_upstream: false, + timeout_ms: 1000, + ..Default::default() + }; + let server = std::sync::Arc::new(mhost_dns::DnsServer::new(config).unwrap()); + + let mut zero_addr = HashMap::new(); + zero_addr.insert( + "ad.example.com".to_string(), + std::net::IpAddr::from([0u8, 0, 0, 0]), + ); + let whitelist: HashSet = ["safe.example.com".to_string()].into_iter().collect(); + server.reload_ad_block_rules(true, zero_addr, HashSet::new(), whitelist); + + // Drive some traffic through the engine. + let engine = server.ad_block_engine_for_test(); + for _ in 0..3 { + let _ = engine.check("ad.example.com"); // hits_zero_addr + } + for _ in 0..2 { + let _ = engine.check("safe.example.com"); // hits_whitelist + } + for _ in 0..4 { + let _ = engine.check("untouched.example.org"); // misses + } + // One NxDomain-source check to confirm `enabled=false` flips all counters. + server.reload_ad_block_rules(false, HashMap::new(), HashSet::new(), HashSet::new()); + let _ = engine.check("ad.example.com"); // master off \u2192 no counter + + // Slot the server into AppState so the IPC reads from it. + // `state.dns_server` is `Mutex>` (not + // `Option>`), so unwrap the `Arc` we + // built above. The `engine` clone is on a separate + // `Arc`, so the test owns the only + // strong ref to DnsServer here. + let inner = std::sync::Arc::try_unwrap(server) + .map_err(|_| "strong refs to test DnsServer leaked") + .expect("test owns the only strong ref to DnsServer"); + *crate::state::lock_or_recover(&state.dns_server) = Some(inner); + // The IPC handler under test (we drive it through the + // AppState directly rather than going through Tauri's + // State wrapper). + // Inline the IPC handler body (we drive it directly + // through the AppState rather than going through + // Tauri's State wrapper). + let response = { + let (stats, enabled) = { + let guard = crate::state::lock_or_recover(&state.dns_server); + match guard.as_ref() { + Some(server) => (server.ad_block_stats(), server.ad_block_enabled()), + None => ( + mhost_dns::adblock::AdBlockStats { + hits_zero_addr: 0, + hits_nxdomain: 0, + hits_whitelist: 0, + misses: 0, + }, + false, + ), + } + }; + AdBlockStatsView { + hits_zero_addr: stats.hits_zero_addr, + hits_nxdomain: stats.hits_nxdomain, + hits_whitelist: stats.hits_whitelist, + misses: stats.misses, + enabled, + } + }; + // Counters persist across reload(false, ...) — only the + // gating state changes; pre-reload zero_addr / whitelist / + // miss totals are intact. Issue #199 contract. + assert_eq!(response.hits_zero_addr, 3, "3 zero-addr hits counted"); + assert_eq!(response.hits_nxdomain, 0); + assert_eq!( + response.hits_whitelist, 2, + "2 whitelist hits counted (counters persist across reload)", + ); + assert_eq!(response.misses, 4, "4 misses counted"); + // Engine master switch off after reload(false, ...): the + // IPC must report the engine's authoritative gating state + // (the AtomicBool mirror), not state.enabled — that is the + // whole point of the PR #219 review-followup race fix. + assert!( + !response.enabled, + "engine master switch off after reload(false, ...)", + ); + let _ = AdBlockAction::ZeroAddress; // keep the import used + } } diff --git a/src-tauri/src/commands/dns.rs b/src-tauri/src/commands/dns.rs index 077fb47..749dff4 100644 --- a/src-tauri/src/commands/dns.rs +++ b/src-tauri/src/commands/dns.rs @@ -369,7 +369,10 @@ async fn set_dns_mode_enable( let snap = state.ad_block_state.read().await.clone(); let (za, nx, wl) = crate::commands::adblock::classify_rules(&snap, state.storage.root()); if let Some(server) = lock_or_recover(&state.dns_server).as_ref() { - server.reload_ad_block_rules(za, nx, wl); + // Issue #199 sub-task B: pass the master switch to the + // engine so `check()` can decide whether misses should + // accumulate. We snapshot it from the cloned state above. + server.reload_ad_block_rules(snap.enabled, za, nx, wl); } spawn_ad_block_refresh_task( &state.ad_block_refresh_task, @@ -909,6 +912,8 @@ mod tests { rule_count: 0, etag: None, rules_limit_override: None, + last_refresh_duration_ms: None, + last_refresh_failed_at: None, }; let st = mhost_core::AdBlockState { enabled: true, // master switch on — the tick's only observable is the fetch error @@ -1347,7 +1352,10 @@ pub(crate) fn spawn_ad_block_refresh_task( return; } if let Some(server) = lock_or_recover(&dns_server_clone).as_ref() { - server.reload_ad_block_rules(za, nx, wl); + // Issue #199 sub-task B: pass the master + // switch to the engine. Read from the + // snapshot cloned at the top of this tick. + server.reload_ad_block_rules(snap.enabled, za, nx, wl); } }) .await; diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 59ceb01..aa20ff7 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -164,6 +164,7 @@ pub fn run() { // 广告屏蔽（issue #130） get_ad_block_state, get_ad_block_limits, + get_ad_block_stats, set_ad_block_enabled, set_ad_block_refresh_interval, set_ad_block_auto_refresh_enabled, diff --git a/src-tauri/src/state/mod.rs b/src-tauri/src/state/mod.rs index eb5de80..5f2eb0a 100644 --- a/src-tauri/src/state/mod.rs +++ b/src-tauri/src/state/mod.rs @@ -309,7 +309,10 @@ impl AppState { let result = tokio::task::spawn_blocking(move || { let (za, nx, wl) = crate::commands::adblock::classify_rules(&snap, &storage_root); if let Some(server) = crate::state::lock_or_recover(&dns_server).as_ref() { - server.reload_ad_block_rules(za, nx, wl); + // Issue #199 sub-task B: pass the master switch + // to the engine. Read from the snapshot cloned + // just above the spawn_blocking boundary. + server.reload_ad_block_rules(snap.enabled, za, nx, wl); } }) .await; diff --git a/src/lib/tauri.ts b/src/lib/tauri.ts index ced1b21..750046b 100644 --- a/src/lib/tauri.ts +++ b/src/lib/tauri.ts @@ -12,6 +12,7 @@ import type { AdBlockLimits, AdBlockSource, AdBlockResponse, + AdBlockStats, } from "../types"; // ---- Profile commands ---- @@ -265,6 +266,12 @@ export async function getAdBlockLimits(): Promise { return invoke("get_ad_block_limits"); } +/** Issue #199 sub-task B: cumulative ad-block engine hit / miss + * counters. Returns zeros if DNS mode is off. */ +export async function getAdBlockStats(): Promise { + return invoke("get_ad_block_stats"); +} + export async function setAdBlockEnabled(enabled: boolean): Promise { return invoke("set_ad_block_enabled", { enabled }); } diff --git a/src/pages/AdBlock.module.css b/src/pages/AdBlock.module.css index 5f3fdcc..ad6f063 100644 --- a/src/pages/AdBlock.module.css +++ b/src/pages/AdBlock.module.css @@ -312,3 +312,54 @@ font-size: 13px; line-height: 1.4; } + +/* Issue #199 sub-task B: stats panel layout */ +.statGrid { + display: grid; + grid-template-columns: repeat(auto-fit, minmax(120px, 1fr)); + gap: 12px; + margin-bottom: 12px; +} + +.statCounter { + display: flex; + flex-direction: column; + padding: 12px; + border: 1px solid var(--border-color, rgba(127, 127, 127, 0.25)); + border-radius: 6px; + background: var(--stat-counter-bg, rgba(127, 127, 127, 0.05)); +} + +.statValue { + font-size: 24px; + font-weight: 600; + font-variant-numeric: tabular-nums; + line-height: 1.2; +} + +.statLabel { + font-size: 12px; + opacity: 0.7; + margin-top: 4px; +} + +.statTable { + width: 100%; + border-collapse: collapse; + font-size: 13px; +} + +.statTable th, +.statTable td { + text-align: left; + padding: 6px 8px; + border-bottom: 1px solid var(--border-color, rgba(127, 127, 127, 0.15)); +} + +.statTable th { + font-weight: 600; + opacity: 0.7; + font-size: 12px; + text-transform: uppercase; + letter-spacing: 0.05em; +} diff --git a/src/pages/AdBlock.tsx b/src/pages/AdBlock.tsx index aac8062..200b7a5 100644 --- a/src/pages/AdBlock.tsx +++ b/src/pages/AdBlock.tsx @@ -11,6 +11,8 @@ import { dnsEnabledAtom, fetchAdBlockStateAtom, fetchAdBlockLimitsAtom, + adBlockStatsAtom, + fetchAdBlockStatsAtom, toggleAdBlockEnabledAtom, setAdBlockIntervalAtom, setAdBlockAutoRefreshEnabledAtom, @@ -56,6 +58,8 @@ function AdBlock() { const fetchState = useSetAtom(fetchAdBlockStateAtom); const fetchLimits = useSetAtom(fetchAdBlockLimitsAtom); + const stats = useAtomValue(adBlockStatsAtom); + const fetchStats = useSetAtom(fetchAdBlockStatsAtom); const toggleEnabled = useSetAtom(toggleAdBlockEnabledAtom); const setInterval = useSetAtom(setAdBlockIntervalAtom); const setAutoRefresh = useSetAtom(setAdBlockAutoRefreshEnabledAtom); @@ -91,7 +95,12 @@ function AdBlock() { /* error already in atom */ }); fetchLimits().catch(() => {}); - }, [fetchState, fetchLimits]); + // Issue #199 sub-task B: pull the cumulative engine counters + // for the stats panel. Non-fatal on failure — the panel + // shows an "unknown" placeholder and the next interaction + // retries. + fetchStats().catch(() => {}); + }, [fetchState, fetchLimits, fetchStats]); const handleAddSource = useCallback(() => { if (!newName.trim() || !newUrl.trim()) return; @@ -709,9 +718,114 @@ function AdBlock() { )} + + {/* Stats panel (issue #199 sub-task B): cumulative ad-block + engine hits + misses + per-source refresh timing. Collapsed by + default to keep the page quiet; users can open it when + investigating blocked-traffic levels or refresh cadence. */} +
+ + Ad-block stats + +
+ {stats === null ? ( +
+ Stats not loaded yet — waiting for the first + `getAdBlockStats` IPC. Counters will appear once the + backend responds. +
+ ) : !stats.enabled ? ( +
+ Master switch is off. The engine is parked, so no + queries are being classified. Cumulative counters + below reflect activity from when the switch was last + on (they are not reset on toggle). +
+ ) : ( +
+ Cumulative since process start. Toggle the master + switch off and back on to keep the engine parked + without losing history. +
+ )} + {stats !== null && ( +
+ + + + +
+ )} + {/* Per-source refresh timing. The list mirrors the + source order on disk; disabled sources are still shown + so the user can see when they last refreshed before + being parked. */} + {state.sources.length > 0 && stats !== null && ( + + + + + {/* Issue #199 sub-task B: header renamed from + "Last refresh" to "Duration" — the + column shows milliseconds (issue #199 + `last_refresh_duration_ms`), not a + timestamp. "Last failed" keeps the + timestamp shape. */} + + + + + + {state.sources.map((src) => ( + + + + + + ))} + +
SourceDurationLast failed
{src.name} + {src.last_refresh_duration_ms != null + ? `${src.last_refresh_duration_ms} ms` + : "—"} + + {src.last_refresh_failed_at + ? new Date(src.last_refresh_failed_at).toLocaleString() + : "—"} +
+ )} +
+
); } +// Issue #199 sub-task B: small counter tile for the stats panel. +// Kept inline (not exported) because the layout is bespoke to this +// page; promoting it later is fine but premature now. +function StatCounter({ + label, + value, +}: { + label: string; + value: number; +}) { + return ( +
+
{value.toLocaleString()}
+
{label}
+
+ ); +} + export default AdBlock; diff --git a/src/pages/__tests__/AdBlock.test.tsx b/src/pages/__tests__/AdBlock.test.tsx index 29f41db..8a158a9 100644 --- a/src/pages/__tests__/AdBlock.test.tsx +++ b/src/pages/__tests__/AdBlock.test.tsx @@ -91,6 +91,8 @@ function makeSource(overrides: Partial = {}): AdBlockSource { rule_count: 100, etag: null, rules_limit_override: null, + last_refresh_duration_ms: null, + last_refresh_failed_at: null, ...overrides, }; } diff --git a/src/stores/profiles/actions.ts b/src/stores/profiles/actions.ts index 71237d9..2994bea 100644 --- a/src/stores/profiles/actions.ts +++ b/src/stores/profiles/actions.ts @@ -22,6 +22,7 @@ import { listDnsProfiles, getAdBlockState, getAdBlockLimits, + getAdBlockStats, setAdBlockEnabled, setAdBlockRefreshInterval, setAdBlockAutoRefreshEnabled, @@ -63,6 +64,7 @@ import { isAdBlockLoadingAtom, adBlockErrorAtom, adBlockLimitsAtom, + adBlockStatsAtom, quickApplyOutcomeAtom, isQuickApplyToastOpenAtom, } from "./state"; @@ -602,6 +604,22 @@ export const fetchAdBlockLimitsAtom = atom(null, async (_get, set) => { } }); +// Issue #199 sub-task B: pull the engine's cumulative counters into the +// `adBlockStatsAtom`. The stats panel calls this on mount; a periodic +// refresh isn't wired here (out of scope — the engine exposes the +// IPC, the consumer decides cadence). +export const fetchAdBlockStatsAtom = atom(null, async (_get, set) => { + try { + const stats = await getAdBlockStats(); + set(adBlockStatsAtom, stats); + } catch (err) { + // Non-fatal: the panel shows the "unknown" placeholder. The + // engine counters still increment in-process; the next call + // will pick them up. + console.warn("failed to load ad-block stats", err); + } +}); + export const toggleAdBlockEnabledAtom = atom( null, async (_get, set, enabled: boolean) => { diff --git a/src/stores/profiles/index.ts b/src/stores/profiles/index.ts index 56d4ab8..a4b76e1 100644 --- a/src/stores/profiles/index.ts +++ b/src/stores/profiles/index.ts @@ -26,6 +26,7 @@ export { isAdBlockLoadingAtom, adBlockErrorAtom, adBlockLimitsAtom, + adBlockStatsAtom, adBlockRuleCountAtom, adBlockHasErrorsAtom, quickApplyOnToggleAtom, @@ -59,6 +60,7 @@ export { toggleDnsProfileEnabledAtom, fetchAdBlockStateAtom, fetchAdBlockLimitsAtom, + fetchAdBlockStatsAtom, toggleAdBlockEnabledAtom, setAdBlockIntervalAtom, setAdBlockAutoRefreshEnabledAtom, diff --git a/src/stores/profiles/state.ts b/src/stores/profiles/state.ts index 0451ec2..ced934c 100644 --- a/src/stores/profiles/state.ts +++ b/src/stores/profiles/state.ts @@ -78,6 +78,13 @@ export const adBlockErrorAtom = atom(null); // remains the authority and rejects over-cap overrides itself). export const adBlockLimitsAtom = atom(null); +// Issue #199 sub-task B: cumulative ad-block engine counters. +// `null` until the first successful `getAdBlockStats` pull; the UI +// shows an "unknown" placeholder until then. The atom holds the +// *latest* snapshot — historical polling rate is a separate +// concern (out of scope here). +export const adBlockStatsAtom = atom(null); + export const adBlockRuleCountAtom = atom((get) => { const state = get(adBlockStateAtom); if (!state) return 0; diff --git a/src/types/index.ts b/src/types/index.ts index 85a3303..91532f2 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -71,6 +71,34 @@ export interface AdBlockSource { * undefined-vs-null mismatch here (cf. issue #202). */ rules_limit_override: number | null; + /** + * Issue #199 sub-task B: wall-clock duration of the last + * `fetch_and_cache_source` call (success, 304, or failure). + * Always serialized (always `null` when unset, never `undefined`). + */ + last_refresh_duration_ms: number | null; + /** + * Issue #199 sub-task B: RFC 3339 timestamp of the last *failed* + * fetch. Distinct from `last_error` (which carries the message + * of the most recent failure regardless of when). Cleared on + * the next successful fetch. + */ + last_refresh_failed_at: string | null; +} + +/** + * Issue #199 sub-task B: cumulative ad-block engine counters + * returned by `getAdBlockStats()`. Numbers are monotonically + * increasing since process start; the consumer computes deltas. + * `enabled` mirrors the current master switch value at the time + of the call. + */ +export interface AdBlockStats { + hits_zero_addr: number; + hits_nxdomain: number; + hits_whitelist: number; + misses: number; + enabled: boolean; } export interface AdBlockState {