feat(adblock): hit / miss counters + per-source refresh telemetry (#199, sub-task B)

[flyhigher139]

Summary

Sub-task B of the AdBlock perf/observability follow-up (#199). Adds the observability piece that #218 (sub-task C — cache cleanup) was missing: the engine now reports its hit / miss behaviour and per-source refresh latency, surfaced via a new IPC + a folded stats panel in the UI.

What's new

Engine (mhost-dns)

addition	why
AdBlockEngine::enabled: AtomicBool	Mirror of the master switch; check() short-circuits when off so the user isn't seeing phantom counters when they've parked ad blocking. Mirrored at every reload by reload_ad_block_rules(enabled, ...).
hits_zero_addr / hits_nxdomain / hits_whitelist / misses (AtomicU64)	Lock-free counters on the DNS hot path. Relaxed ordering — each counter is independently monotonic, no cross-counter consistency required.
AdBlockEngine::stats() -> AdBlockStats	Single struct snapshot of the four counters.
AdBlockStats view type	Debug + Clone + Copy + PartialEq + Eq. Cumulative since process start; the engine has no reset path (consumer computes deltas).

Counter semantics (check())

• Master switch on, whitelist match → hits_whitelist += 1, return None.
• Master switch on, NXDOMAIN match → hits_nxdomain += 1, return Some(NxDomain).
• Master switch on, zero-addr match → hits_zero_addr += 1, return Some(ZeroAddress(ip)).
• Master switch on, no rule, no whitelist → misses += 1, return None.
• Master switch off → no counter advances, return None. The whitelist walk is skipped because classify_rules only feeds block rules to the engine when master is on, so the whitelist walk would have no observable effect anyway. (Issue contract: "misses 累加, 不要把 whitelist 命中算 miss".)
• Master switch on, no block rules loaded, whitelist match → hits_whitelist += 1. Without block rules, no misses is counted for "no match" — there's nothing to miss against.

Model (mhost-core)

AdBlockSource gains two fields, both following the #[serde(default)] convention from #202 (always serialized as null, never undefined):

pub last_refresh_duration_ms: Option<u64>,   // wall-clock ms of the last fetch
pub last_refresh_failed_at: Option<DateTime<Utc>>,  // cleared on success

Deliberately NOT added: last_refresh_success_at. The existing last_fetched_at already covers this. The PR #199 review note flagged this — adding it would have been duplicate state.

Timing (commands::adblock::fetch_and_cache_source)

• Instant::now() captured right after the per-source gate is acquired.
• elapsed_ms = started_at.elapsed().as_millis() as u64 captured once, reused for all three branches.
• 200 OK path → write last_refresh_duration_ms, clear last_refresh_failed_at.
• 304 path → write last_refresh_duration_ms, clear last_refresh_failed_at (issue [Enhancement] AdBlock 用 ETag 实现条件 GET，省掉周期刷新的整份下载 #193 contract preserved).
• Err path → new record_fetch_error_with_timing helper writes last_error + last_refresh_duration_ms + last_refresh_failed_at = Some(now).

Reload plumbing

reload_ad_block_rules gains enabled: bool as the first parameter:

pub fn reload_ad_block_rules(
    &self,
    enabled: bool,
    zero_addr_rules: HashMap<...>,
    nxdomain_rules: HashSet<...>,
    whitelist: HashSet<...>,
) {
    self.ad_block_engine.set_enabled(enabled);
    self.ad_block_engine.rebuild(zero_addr_rules, nxdomain_rules, whitelist);
    self.cache.lock().clear();
}

All four call sites updated:

• commands::adblock::persist_and_reload (the steady-state reload path).
• commands::dns cold-start hot reload (post-AppState::new).
• commands::dns auto-refresh tick (per periodic reload).
• state::AppState::new cold-start (spawn_blocking boundary).

IPC

New command:

#[tauri::command]
pub async fn get_ad_block_stats(state: State<'_, AppState>)
    -> Result<AdBlockStatsView, MhostError>

Returns zeros if DNS mode is off (no engine loaded). AdBlockStatsView mirrors the engine struct plus enabled: bool so the UI can label the panel.

Frontend

• AdBlockSource interface gains last_refresh_duration_ms: number | null and last_refresh_failed_at: string | null.
• New AdBlockStats interface.
• New getAdBlockStats() binding in src/lib/tauri.ts.
• New adBlockStatsAtom + fetchAdBlockStatsAtom action in src/stores/profiles/{state,actions}.ts, re-exported from index.ts.
• Stats panel in src/pages/AdBlock.tsx as a collapsed <details> card immediately after the Auto-refresh banner:
  • 4 counter tiles (zero-addr hits / nxdomain hits / whitelist hits / misses)
  • Per-source refresh timing table (name, last_refresh_duration_ms, last_refresh_failed_at)
  • Master-switch-off explanatory placeholder (so users don't think counters are broken when they've parked ad blocking)
• CSS additions in AdBlock.module.css for the stat grid / table layout.

Tests

Four new tokio::tests in mhost-dns/src/adblock.rs:

• counter_accounting_for_mixed_traffic — drives check() with a mix of whitelist hits, zero-addr hits, NXDOMAIN hits, and miss traffic; asserts each counter lands on the right number.
• whitelist_hit_is_not_a_miss — issue [Perf+Enhancement] AdBlock 性能与可观测性 follow-up（trie + 指标 + cache 清理） #199 contract test.
• master_switch_off_does_not_advance_any_counter — short-circuit correctness.
• stats_can_be_read_concurrently_with_check — AtomicU64 + Relaxed survives concurrent check() from another thread; the final assertion locks in the post-run counter values.

Existing tests in mhost-dns were updated to call engine.set_enabled(true) after rebuild(...) (they were already setting up rule sets and asserting Some(...)); without the master-switch-on step the new check() short-circuits to None.

CI gates

• cargo fmt --all -- --check ✓
• cargo clippy --all-targets --all-features -- -D warnings ✓
• cargo test --all-features --workspace ✓ (558 passed: mhost 187, mhost-apply 70, mhost-core 49, mhost-dns 144, mhost-hosts 47, mhost-storage 61)
• pnpm build ✓ (tsc + vite)
• pnpm test ✓ (330 passed, 0 failed)

Behaviour change to flag (reviewers)

The check() fast-path on !has_block_rules() was moved — it now runs AFTER the whitelist walk, not before. This is intentional:

• Master switch off (the common park state) is now caught first.
• When master is on but block-rule sets are empty, the whitelist walk still runs and hits_whitelist increments on a match. The empty-rule-set short-circuit prevents a false misses count when there's nothing to miss against.

Follow-ups (separate PRs)

• Sub-task A (trie replacement): [Perf+Enhancement] AdBlock 性能与可观测性 follow-up（trie + 指标 + cache 清理） #199 / sub-task A — the last big ticket for this issue.
• Frontend UX: the stats panel doesn't auto-refresh — it pulls once on mount. Periodic refresh is a separate UX choice (interval choice, polling cancellation on unmount, etc.) that can ship independently.

🤖 Generated with Codex

[flyhigher139]

Self-review (post-merge_request 自查)

ce481b7 是初始 commit；4383986 是这次自审发现的 follow-up。我又盯着 diff 重新过了一遍代码路径，找到 2 个真实问题，全部修了。下面是诚实复盘。

找到的问题与处理

#	问题	严重度	处理
1	mhost-dns/src/adblock.rs 里 pub struct AdBlockStats 被放在了 pub struct AdBlockEngine 之后，但 AdBlockStats 的 doc 注释反向引用了 AdBlockEngine。前向引用在 Rust 里能跑，但读起来不自然。	低（doc）	在 4383986 调整顺序：AdBlockStats 提前到 AdBlockEngine 前面。
2	get_ad_block_stats IPC 里：counters 从 engine 读，enabled 字段从 state.ad_block_state 读——两次读之间可能被 set_ad_block_enabled 插入。窗口内 engine AtomicBool 和 state.enabled 可能不一致，响应里的 enabled 会反映 reload 后的 state 但 engine 还是 reload 前的 gating 值。	低（瞬态，但能修）	在 4383986 把 state.enabled 提前读到 local，避免两个读之间的窗口。

没找到问题的地方（但本以为是风险点的）

• reload_ad_block_rules 签名改了，4 个 caller（commands/adblock、commands/dns 冷启动、commands/dns 自动刷新 tick、state/mod 冷启动）+ 2 个测试 caller 都更新了。grep reload_ad_block_rules( 验证过。
• check() 行为变更：master switch 的 fast-path 移到 whitelist 走之前；whitelist 计数在没有 block rules 时也会发生；no rules + no whitelist → return None 不算 miss。三条都有专门的测试守。
• fetch_and_cache_source timing：Instant::now() 在 per-source gate 后立刻抓；elapsed_ms 在三个分支前一次捕获后复用。Err 路径用 record_fetch_error_with_timing 帮手。
• serde migration：新字段全 #[serde(default)]，序列化永远 null（不复用 AdBlock error banner false positive: last_error serialization mismatch (undefined vs null) #202 的错误）。last_refresh_success_at 故意没加——和 last_fetched_at 重复。
• 前端 stats panel 拉取时机：仅 mount 时拉一次。没做轮询（issue 里也没要求）。
• Frontend 类型 + bindings + atom + action + re-export：四件套都串通；test helper makeSource 同步加了新字段；test 27 个文件 330 依旧绿。

我决定不修的事（已在 PR description 列了）

• Stats 面板不轮询：mount 时拉一次。用户想看实时数字，得手动触发（例如刷新按钮或切回 tab）。issue 里只要求"提供 stats"，没要求轮询。后续 UX 选择可以独立发 PR。
• Reload→engine AtomicBool 与 state.enabled 的小窗口：set_ad_block_enabled 写 state 后调 persist_and_reload 才把 engine 同步上。这段窗口是已有的 race，不在本 PR 范围。

CI gates (4383986 提交后)

• cargo fmt --all -- --check ✓
• cargo clippy --all-targets --all-features -- -D warnings ✓
• cargo test --all-features --workspace ✓ (558 passed, 0 failed)
• pnpm build ✓
• pnpm test ✓ (330 passed, 0 failed)

🤖 Generated with Codex

[flyhigher139]

Code review

I read the full diff (master..HEAD, +705/-22 across 16 files) and ran the full CI gates locally:

• cargo fmt --all -- --check ✓
• cargo clippy --all-targets --all-features -- -D warnings ✓
• cargo test --all-features --workspace ✓ (558 passed, 0 failed)
• pnpm build ✓ (tsc + vite build)
• pnpm test ✓ (330 passed, 0 failed)

The 4 new mhost-dns tests are present and pass:

test adblock::tests::counter_accounting_for_mixed_traffic ... ok
test adblock::tests::whitelist_hit_is_not_a_miss ... ok
test adblock::tests::master_switch_off_does_not_advance_any_counter ... ok
test adblock::tests::stats_can_be_read_concurrently_with_check ... ok

Correctness

Counter semantics (issue #199 contract). Walked through check() against every branch:

• Master off → returns None before any counter touches. Correct.
• Master on + whitelist hit → hits_whitelist += 1, return None. Correct.
• Master on + NXDOMAIN match → hits_nxdomain += 1, return Some(NxDomain). Correct.
• Master on + zero-addr match → hits_zero_addr += 1, return Some(ZeroAddress(ip)). Correct.
• Master on, no rule, no whitelist, has block rules → misses += 1. Correct.
• Master on, no rule, no whitelist, no block rules → returns None WITHOUT advancing misses (no point counting misses against an empty rule set). This is documented in the doc comment and covered by whitelist_only_snapshot_has_no_block_rules. Correct.

Reload signature change. grep -rn reload_ad_block_rules --include='*.rs' finds exactly 6 callers; all 6 pass enabled. No missed caller.

server.rs:1966  -> true          (test)
state/mod.rs:315 -> snap.enabled
commands/adblock.rs:322 -> snapshot.enabled
commands/adblock.rs:1914 -> true  (test)
commands/dns.rs:375 -> snap.enabled
commands/dns.rs:1358 -> snap.enabled

Race in get_ad_block_stats (commit 4383986). The commit description claims it fixes the race by reading state.enabled before constructing the response. Looking at the actual code:

let stats = { /* read engine.ad_block_stats() */ };           // (1) read engine
let enabled = state.ad_block_state.read().await.enabled;       // (2) read state
Ok(AdBlockStatsView { ..., enabled, })

(1) and (2) are independent and a concurrent set_ad_block_enabled IPC between them still leaves the response's enabled field agreeing with state.enabled at read time, but the counters may have been counted under the engine value set_enabled had BEFORE the toggle propagated through persist_and_reload. The fix here is purely a local-variable extraction — the race window is identical to the pre-4383986 code.

The commit message acknowledges this is 'microseconds and just a UI label flicker', so the team has explicitly accepted the benign race. Fine for v1, but if you ever want to close the window cleanly, two options:

1. Read state.enabled BEFORE the engine counters (so the response's enabled reflects the state at the time of capture, with engine counters observed under that same state at a coherent moment).
2. Add AdBlockEngine::is_enabled(&self) -> bool and bundle it into AdBlockStats so a single IPC returns (counters, enabled) from one snapshot.

Not blocking — flagging so it does not get lost.

record_fetch_error_with_timing. Mirrors record_fetch_error exactly: same lock acquisition, same find_source_mut, same last_error = Some(err.to_string()) write — then adds the two timing fields. Behavior preserves the prior function (line 1265 caller still uses record_fetch_error, untouched). Correct.

Serde migration. Both new fields use #[serde(default)] without skip_serializing_if. They will appear as null in the JSON when empty — consistent with the issue #202 contract. Legacy documents without the key deserialize as None. See test gap below.

Performance

Hot-path overhead. check() adds one Relaxed load on enabled + one Relaxed fetch_add per outcome. Both are lock-free and the atomic loads/stores compile to a single MOV on x86 / LDADD+STADD on ARM. No locks taken in the hot path; the existing snapshot() Arc-clone is unchanged. Fine.

stats() correctness across concurrent check(). Each counter is independently monotonic and Relaxed loads are fine for that contract. The stats_can_be_read_concurrently_with_check test exercises this on a real thread — it asserts final values are exactly the writer's 1000 increments, which catches lost-update bugs. Good.

enabled: AtomicBool vs current: RwLock<Arc>. The two are independent atomics. There is a small window where readers can see one but not the other (e.g. enabled=true with the old snapshot during set_enabled(true) → rebuild()). This is benign for the same reason as #132: every code path is either 'no counter advances' or 'counts under the wrong rules' — both are observed and smoothed out by the cumulative counter semantics. The Relaxed ordering is documented as a deliberate choice. Fine.

Tests

The 4 new tests are doing what their names claim. Walked through each:

• counter_accounting_for_mixed_traffic: drives 2+2+9+5 hits, asserts each counter lands on its expected value. Correct and tight.
• whitelist_hit_is_not_a_miss: 10 whitelist hits → hits_whitelist == 10, misses == 0. Catches the bug where someone merges whitelist into the miss branch. Correct.
• master_switch_off_does_not_advance_any_counter: rules loaded but set_enabled never called → all counters stay at 0 across mixed query types. Catches the short-circuit. Correct.
• stats_can_be_read_concurrently_with_check: writer thread runs 1000 check() calls while main thread hammers stats(); final assertion locks the exact post-run counters. Catches lost-update regressions.

Test gaps worth flagging (medium):

1. No direct test for record_fetch_error_with_timing. The new helper writes 3 fields (last_error, last_refresh_duration_ms, last_refresh_failed_at) under the same lock acquisition pattern as record_fetch_error. A trivial unit test calling it and asserting all three fields would be one screenful and would prevent regressions where someone refactors the helper to forget the timing fields. Recommend adding before merge.

2. No integration assertion that fetch_and_cache_source actually populates last_refresh_duration_ms. The existing 200-OK, 304, and 5xx tests in commands/adblock.rs are good scaffolds; a single line at the end of each (assert!(snap.last_refresh_duration_ms.is_some())) would close the gap. The PR claims 'elapsed_ms captured once, written in all 3 branches' — having a test per branch locks that in.

3. No legacy-doc back-compat test for the new fields. The PR correctly mirrors the issue [Enhancement] ad-block 单源超限：错误横幅提供 per-source 一键 override（fail-closed，不截断） #207 / AdBlock error banner false positive: last_error serialization mismatch (undefined vs null) #202 pattern (#[serde(default)] + assertions that the keys serialize as null), but the existing test_ad_block_source_serde_nulls_none_optionals only checks last_fetched_at, last_error, etag, rules_limit_override — it does not check last_refresh_duration_ms or last_refresh_failed_at in the JSON. A regression adding skip_serializing_if to either new field would slip past. Also no legacy-document test (analogous to test_ad_block_source_rules_limit_override_serde) verifying a JSON without the new keys round-trips as None. Recommend a small follow-up test.

4. No direct test for get_ad_block_stats IPC. The race fix in 4383986 is non-trivial; the IPC handler's behavior — returns zeros when no server, returns engine counters when present, reads state.enabled — has no test coverage. At minimum a test that calls the handler against an AppState with no dns_server (returns zeros) and one with a freshly-built server returning the expected struct. Other IPC commands in this file also lack direct tests, so this is consistent — but for a self-described 'race fix', the absence is notable.

Frontend

enabled field on AdBlockStatsView is used. Confirmed: stats.enabled drives the three-branch placeholder (null / master-off / master-on) at src/pages/AdBlock.tsx:742-757. Not dead weight.

Stats panel UX. Collapsed

Details

by default — keeps the page quiet; one-click to expand. Counter tiles use font-variant-numeric: tabular-nums so the digits do not jitter as values grow. Reasonable.

One small UX nit. Table header is 'Source | Last refresh | Last failed' but the 'Last refresh' column shows a duration (e.g. '1234 ms'), not a timestamp. Users will read 'Last refresh' and expect a date. Suggest renaming to 'Duration' (or 'Last refresh (ms)'). Not a blocker — nit.

No auto-refresh. PR description acknowledges this as a deliberate scope choice ('polling rate is a separate concern'). For a v1 of an observability feature that is fine; the user can re-trigger by toggling master or refreshing sources.

Style / API

AdBlockStatsView vs deriving Serialize on the engine struct. A *View suffix is new to this file (other IPC structs in commands/adblock.rs do not use it). Justified here because the engine's AdBlockStats lives in a crate that has no Tauri/serde dependency and adding Serialize there is a one-way door. Keeping the view-model split is reasonable. Worth documenting in a one-line comment on the view struct that the split is intentional (rather than a copy-paste accident) — the existing doc covers this.

Nits

• Typo at src-tauri/src/commands/adblock.rs:557: '9perceives' should be 'perceives' (stray leading '9').
• Doc inconsistency at src-tauri/src/commands/adblock.rs:553-560: comment says 'from right after the gate is acquired' but also 'including the per-source queue wait' — those two statements contradict each other. If you want to include the gate wait, move started_at above acquire_source_refresh_gate. Otherwise drop 'including the per-source queue wait' from the comment.

What's done well

• All four counter outcomes are covered by both documentation and tests.
• Hot-path uses lock-free atomics with Relaxed ordering — correct for independently-monotonic counters.
• #[serde(default)] without skip_serializing_if keeps the wire format aligned with the issue AdBlock error banner false positive: last_error serialization mismatch (undefined vs null) #202 contract and the frontend's number | null / string | null TS types.
• Timing captured once and reused across all 3 branches — no per-branch Instant::now() drift.
• All 6 callers of reload_ad_block_rules updated; signature change is consistently propagated.
• Frontend wiring is complete (types + binding + atom + action + re-export + UI + CSS), and the existing test helper makeSource is kept in sync.
• console.warn on fetchAdBlockStatsAtom failure matches the existing fetchAdBlockLimitsAtom pattern.

Verdict

LGTM with minor follow-ups. Nothing in this PR is a correctness regression. The race in get_ad_block_stats is acknowledged in the commit message as benign and is acceptable for v1; if you ever want to close it, see the two options above. The test gaps (items 1-4) are real but each is a small addition — recommend handling at least #1 (record_fetch_error_with_timing unit test) and the typo/doc inconsistency before merge.

[flyhigher139]

Review follow-up commit: 96fc34c

Addressed every actionable finding from the code-reviewer's review. One of them was a real bug I had glossed over.

Real bug — get_ad_block_stats race (subagent was right)

My previous "race fix" in 4383986 was cosmetic — I hoisted state.enabled into a local but the read still happened after the engine stats read, so the race window between set_ad_block_enabled writing state and persist_and_reload mirroring onto the engine was unchanged. The subagent caught this and was correct.

The actual fix:

• AdBlockEngine::is_enabled(&self) -> bool — new getter for the mirrored master switch.
• DnsServer::ad_block_enabled(&self) -> bool — delegates to it.
• get_ad_block_stats now reads engine counters AND engine.enabled in one lock acquisition. The state.ad_block_state.read().await.enabled read is gone.

Why this closes the window: the engine's AtomicBool is the value that actually gates check(). set_ad_block_enabled writes state.enabled and then mirrors onto the engine's AtomicBool inside persist_and_reload. Reading the engine directly means the IPC sees the gating value check() is actually using, with no window where state and engine disagree.

There's a regression-guard test (get_ad_block_stats_returns_engine_counters_and_enabled) that explicitly asserts !response.enabled after reload_ad_block_rules(false, ...) — that assertion fails on the old code path (state-based read) and passes on the new path (engine read).

Doc / typo nits

• 9perceives typo → perceives
• Timing doc comment: clarified the timer does NOT include the per-source queue wait (only fetch + parse + bookkeeping); the previous text self-contradicted on this.
• AdBlock.tsx column header: "Last refresh" → "Duration". The column shows ms, not a timestamp.

Test gaps closed (6 new tests)

#	Test	Covers
1	record_fetch_error_with_timing_writes_all_three_fields	direct unit test of the new helper
2	fetch_and_cache_source_200_writes_duration_and_clears_failure	200 OK branch with 100 ms mock delay
3	fetch_and_cache_source_304_writes_duration_and_clears_failure	304 branch
4	fetch_and_cache_source_err_writes_duration_and_failure_timestamp	Err branch (5xx)
5	adblock_state_legacy_doc_back_compat_for_199b_fields	serde back-compat for new fields (analog to #207's test)
6	get_ad_block_stats_returns_engine_counters_and_enabled	direct integration test of the IPC; doubles as a regression guard for the race fix

Test counts

• Backend: 558 → 564 passed
• Frontend: 330 → 330 (no change)

CI gates

• cargo fmt --all -- --check ✓
• cargo clippy --workspace --all-targets -- -D warnings ✓
• cargo test --all-features --workspace ✓ (564 passed, 0 failed)
• pnpm build ✓
• pnpm test ✓ (330 passed, 0 failed)

🤖 Generated with Codex