feat(adblock): drop cache when source disabled, refetch on re-enable (#199, sub-task C) - #218
Merged
Merged
Conversation
added 2 commits
September 17, 2026 09:59
…(issue #199) Sub-task C of the AdBlock perf/observability follow-up (issue #199, option A). When a source flips from enabled -> disabled, drop its `adblock-cache/<id>.txt` so a parked source leaves no on-disk residue. When it flips back to enabled, kick a re-fetch off the per-source gate *before* `persist_and_reload` so the upcoming `classify_rules` actually sees rules to push into the engine — otherwise the user would see an enabled source with zero rules until the next auto-refresh tick. Implementation notes: * `set_ad_block_source_enabled` is refactored into the existing `_impl(state, ...)` pattern (matches `add_ad_block_source_impl` and `set_ad_block_source_rules_limit_override_impl`) so the disable / re-enable / no-op edges are unit-testable without a Tauri `State`. * `delete_cache` is already idempotent (treats `NotFound` as success), so a concurrent sweep or prior disable is harmless. * The inline fetch uses `force=false`, so RFC 7232 conditional GET (issue #193) saves bandwidth if the upstream still has the prior ETag. The 304-with-missing-cache downgrade (issue #206 finding 2) handles the case where the disable path's `delete_cache` left no file. * Fetch errors are logged, not propagated: the user's toggle succeeded, only the network leg failed, and `record_fetch_error` populates `last_error` for the UI badge. Race acknowledged (not fixed): if a user clicks Refresh then immediately toggles Disabled, the in-flight fetch is serialized behind the per-source gate (issue #206) but does not block on `ad_block_state`. The disable's `delete_cache` can therefore run and then be overwritten by the in-flight fetch's cache write — a transient cache file lingering for a now-disabled source until the next toggle or `sweep_orphan_caches` on restart. Hygiene-only, not correctness (the engine classifies by `s.enabled` so the lingering cache stays unloaded). Fixing it would mean teaching `fetch_and_cache_source` to re-check `s.enabled` post-gate, out of scope for the cache-cleanup follow-up. Tests added (all `tokio::test`): * `set_ad_block_source_enabled_impl_disable_drops_cache` — disable a source with a populated cache; assert the file is gone and bookkeeping (rule_count, etag) is preserved (matches the 304 contract from issue #193). * `set_ad_block_source_enabled_impl_re_enable_refetches_cache` — start from a disabled state whose cache is already absent, enable, assert the cache file reappears with the freshly fetched body and bookkeeping is rewritten. * `set_ad_block_source_enabled_impl_same_value_is_noop` — toggle to the current value, assert cache mtime is unchanged. Guards against a future refactor that swaps `prev_enabled` capture for an unconditional delete-or-fetch. cargo fmt + cargo clippy -D warnings + cargo test all green.
Three small correctness/robustness fixes surfaced by re-reading the diff against the code it relies on. None of these change behaviour; they sharpen claims and assertions so the next reviewer (and future me) doesn't have to take them on faith. 1. Doc comment was overstated. The function's doc comment claimed "this call always ends with a populated cache", but issue #211-1 already established that the pathological double-304 (a server returning 304 on BOTH the conditional AND the unconditional downgrade retry) is the one case where `fetch_and_cache_source` fails and the cache stays empty. The doc now spells out that edge explicitly and points at the test that proves it (`fetch_and_cache_source_errors_when_downgrade_retry_also_304`). 2. `set_ad_block_source_enabled_impl_re_enable_refetches_cache` had a trivially-true assertion. The setup pinned `last_error: None`, then asserted `stored.last_error.is_none()` after the fetch — tautological, not a test. Pinned `last_error: Some("prior offline failure")` in the setup and added a comment explaining why the assertion now actually exercises the "successful fetch clears prior error" path. Also tightened the cache-content check from `cache.contains("fresh.example.com")` (which would pass even if the body was re-canonicalised with a trailing newline dropped) to a verbatim `assert_eq!(cache.as_str(), str::from_utf8(body).unwrap(), ...)`. 3. `set_ad_block_source_enabled_impl_same_value_is_noop` relied on mtime alone. mtime is a 1-second-resolution field on some filesystems, so a no-op toggle that overwrites the cache with byte-identical content could false-pass. Added a content check as defence in depth: re-read the file and `assert_eq!` against the original bytes. No CI gate changes: - cargo fmt --all -- --check ✓ - cargo clippy --all-targets --all-features -- -D warnings ✓ - cargo test --all-features ✓ (187 passed, 0 failed)
Contributor
Author
Self-review (post-
|
| # | 问题 | 严重度 | 处理 |
|---|---|---|---|
| 1 | doc comment 写 "this call always ends with a populated cache" — 但 issue #211-1 已经证明 double-304 的病态上游会让 fetch 报错、cache 留空。这是过度声明。 | 低（doc） | 在 8231da1 改写：明确指出该边缘情形 + 指向 fetch_and_cache_source_errors_when_downgrade_retry_also_304 测试 |
| 2 | 测试 2 setup 里 last_error: None，然后断言 stored.last_error.is_none() — 恒真，没在测任何东西。 |
中（测试可信度） | 在 8231da1 把 setup 改成 last_error: Some("prior offline failure")，断言现在真的在测"成功 fetch 清掉旧的 error"。顺带把 cache.contains(...) 收紧成 assert_eq!(cache.as_str(), str::from_utf8(body).unwrap(), ...)，避免尾部空格下落丢了误判 |
| 3 | 测试 3 只断言 mtime 不变。在 mtime 1 秒精度的文件系统上，no-op toggle 写入字节相同内容可能 false-pass。 | 低（罕见） | 加 assert_eq! 对内容做 verbatim 比对，做 defense in depth |
没找到问题的地方（但本以为是风险点的）
为了以后 reviewer 不必担心，这里也列一下我特意看过但确认没问题的边界：
- 并发 toggle race：
ad_block_state写锁序列化了两端的 toggle；同值场景 prev_enabled == 新值，删除/获取都不触发，已用 test 3 守护。 - DNS 没开时的 fetch 流程：
persist_and_reload里if dns_enabled && !cancel.is_cancelled()守护着，DNS off 时 classify_rules 不跑，但 fetch 仍然写 cache + 更新 in-memory bookkeeping。测试用的 state 默认 dns_enabled=false，等于验证的就是这条路径。OK。 - fetch 失败的错误传播：故意不传，design 已经在 description 里交代（用户意图"启用"成功，仅网络腿失败，
last_error由record_fetch_error写入）。不是回归到旧行为。 _impl重构正确性：add_ad_block_source_impl/set_ad_block_source_rules_limit_override_impl已经是这个套路，IPC 公共命令走&state→ State 解引用为 &AppState。同一文件、同一种 idiom。- fetch-then-persist 顺序：故意 fetch 在 persist 前面，让后续的 classify_rules 看到新 cache。
persist_and_reload内部 classify + reload 在 spawn_blocking 里串行，原子。
我决定不修的两件事（PR description 已经声明）
- Refresh-then-Disable race：in-flight fetch 在 per-source gate 上串行（issue Ad-block refresh hardening + test harness fixes (from PR #204 review P3s) #206 finding 1），但不阻塞
ad_block_state。Disable 的delete_cache可能跑完，in-flight fetch 又把 cache 写回来。引擎按s.enabled分类，所以 cache 文件残留不影响正确性，仅是卫生问题。修这个要改fetch_and_cache_source本身（gate 释放后再 checks.enabled），超出 sub-task C 的范围。留在 doc comment 里声明，issue [Perf+Enhancement] AdBlock 性能与可观测性 follow-up（trie + 指标 + cache 清理） #199 后续讨论。 - 前端 spinner 缺失：
setAdBlockSourceEnabledAtom没设isAdBlockLoadingAtom，re-enable 最多卡FETCH_TIMEOUT_SECS没视觉反馈。这是 UX gap，跟 backend 改动正交，留作单独 UX PR。
CI gates
8231da1 提交前再跑了一遍：
cargo fmt --all -- --check✓cargo clippy --all-targets --all-features -- -D warnings✓cargo test --all-features✓（187 passed, 0 failed）
🤖 Generated with Codex
flyhigher139
pushed a commit
that referenced
this pull request
Sep 17, 2026
…199 sub-task B) This commit addresses every actionable finding from the PR #219 code-reviewer's review. The previous "race fix" in commit 4383986 was actually cosmetic (just hoisted state.enabled into a local variable, but the read still happened after the engine stats read, so the race window between engine and state was unchanged). This commit is the real fix. ## What's fixed ### Real race fix (the big one) `AdBlockEngine::is_enabled(&self) -> bool` exposes the engine's mirrored master switch. `DnsServer::ad_block_enabled(&self) -> bool` delegates to it. `get_ad_block_stats` now reads BOTH the engine counters AND the engine's enabled flag in one lock acquisition. The old `state.ad_block_state.read().await.enabled` read is gone \u2014 it was the source of the race: `set_ad_block_enabled` writes state.enabled and then mirrors onto the engine's AtomicBool in two steps inside `persist_and_reload`. Reading state instead of the engine meant the IPC could see a gating `true` while the engine was still parking (`!check()` returned None), or vice versa. ### Doc / typo nits * `9perceives` typo in `fetch_and_cache_source` timing comment (PR #218 also flagged this). * Doc clarification: timer starts right after the per-source gate is acquired, *not* including the queue wait. Removed the self-contradictory "right after the gate is acquired ... including the per-source queue wait" wording. * `AdBlock.tsx` column header renamed from "Last refresh" to "Duration" \u2014 the column shows milliseconds (issue #199 `last_refresh_duration_ms`), not a timestamp. ### Test gaps closed (5 new tests) All 4 medium-severity gaps from the PR review plus one I added: 1. `record_fetch_error_with_timing_writes_all_three_fields` \u2014 direct unit test of the new helper; pins the all-three-fields contract so a future refactor doesn't silently drop timing. 2. `fetch_and_cache_source_200_writes_duration_and_clears_failure` \u2014 integration test of the 200 OK branch with a 100 ms mock delay; asserts duration in `[80, 5000] ms` and that `last_refresh_failed_at` is cleared. 3. `fetch_and_cache_source_304_writes_duration_and_clears_failure` \u2014 same shape for the 304 path; setup pins a stale failure timestamp so the clearing assertion is meaningful. 4. `fetch_and_cache_source_err_writes_duration_and_failure_timestamp` \u2014 500 response + 50 ms mock delay; asserts all three timing / failure fields written, and `last_refresh_failed_at` is within `[before, after]` of the call. 5. `adblock_state_legacy_doc_back_compat_for_199b_fields` \u2014 serde back-compat for the new `last_refresh_duration_ms` / `last_refresh_failed_at` fields. Analog to the earlier `rules_limit_override` back-compat test for #207. Pre-#199-B documents deserialize cleanly with the new fields defaulting to `None`. 6. `get_ad_block_stats_returns_engine_counters_and_enabled` \u2014 direct integration test of the IPC handler. Drives mixed traffic through the engine, reloads with `enabled=false`, and asserts the IPC response reflects (a) cumulative counters (zero_addr=3, whitelist=2, misses=4) AND (b) the engine's authoritative gating state (`enabled=false`). This test fails on the old code path (state.read) and passes on the new code path (engine read), so it's a regression guard for the race fix. ## Test counts * Backend: 558 \u2192 564 passed (mhost 187 \u2192 193, mhost-dns 140 \u2192 144, others unchanged). * Frontend: 330 \u2192 330 (no behavioural test changes; the AdBlock.tsx column header change has no test impact). * No tests removed or skipped. ## CI gates * cargo fmt --all -- --check \u2713 * cargo clippy --workspace --all-targets -- -D warnings \u2713 (clean: the inline IPC helper resolved the `clippy::items_after_test_module` lint the free helper had triggered) * cargo test --all-features --workspace \u2713 (564 passed, 0 failed) * pnpm build \u2713 * pnpm test \u2713 (330 passed, 0 failed) 🤖 Generated with [Codex](https://codex.openai.com/)
flyhigher139
added a commit
that referenced
this pull request
Sep 17, 2026
…, sub-task B) Sub-task B of the AdBlock perf/observability follow-up (#199). Adds the observability piece that #218 (sub-task C — cache cleanup) was missing. 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. | | `AdBlockEngine::is_enabled() -> bool` | Engine-side getter for the master switch mirror. Closes the race where `state.enabled` had been written but the engine's AtomicBool hadn't been mirrored yet (PR #219 review-followup). | | `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. ### Reload plumbing `reload_ad_block_rules` gains `enabled: bool` as the first parameter: \`\`\`rust 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). ### Model (`mhost-core`) `AdBlockSource` gains two fields, both following the `#[serde(default)]` convention from #202 (always serialized as `null`, never `undefined`): \`\`\`rust 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. ### 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 #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)`. ### IPC New command: \`\`\`rust #[tauri::command] pub async fn get_ad_block_stats(state: State<'_, AppState>) -> Result<AdBlockStatsView, MhostError> \`\`\` Reads both counters AND enabled from the engine in a single lock acquisition (closes the race that an earlier draft had with `state.ad_block_state.read().await.enabled`). `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 10 new tests in this PR (4 from initial commit, 6 from the review follow-up): **Initial (PR #219):** - `counter_accounting_for_mixed_traffic` - `whitelist_hit_is_not_a_miss` - `master_switch_off_does_not_advance_any_counter` - `stats_can_be_read_concurrently_with_check` **Review follow-up (this commit):** - `record_fetch_error_with_timing_writes_all_three_fields` — direct helper test - `fetch_and_cache_source_200_writes_duration_and_clears_failure` — 200 branch - `fetch_and_cache_source_304_writes_duration_and_clears_failure` — 304 branch - `fetch_and_cache_source_err_writes_duration_and_failure_timestamp` — Err branch - `adblock_state_legacy_doc_back_compat_for_199b_fields` — serde back-compat - `get_ad_block_stats_returns_engine_counters_and_enabled` — IPC integration + race-fix regression guard Existing mhost-dns tests were updated to call `engine.set_enabled(true)` after `rebuild(...)` (master switch is now an explicit input). ## CI gates (this PR) - `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) ## Behaviour changes to flag (reviewers) 1. `check()` master-switch short-circuit now runs before the whitelist walk (the fast-path was re-ordered). Master switch off → no counters, no work. Master on + no block rules + whitelist match → `hits_whitelist` increments (NOT counted as miss). 2. `reload_ad_block_rules` gained an `enabled: bool` first parameter. Any caller that wasn't updated would fail to compile. 3. `get_ad_block_stats` reads the engine's AtomicBool mirror for the `enabled` field instead of `state.enabled`. Closes the race where `set_ad_block_enabled` had updated state but `persist_and_reload` hadn't yet mirrored onto the engine. ## Follow-ups (separate PRs) - Sub-task A (trie replacement): #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 that can ship independently. 🤖 Generated with [Codex](https://codex.openai.com/)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sub-task C of the AdBlock perf/observability follow-up (#199, option A).
delete_cacheon disable — parked source leaves no on-disk residuefetch_and_cache_sourcebeforepersist_and_reload, so the engine actually sees rules to loadIssue #199 option A was picked over option B (LRU prune at N sources) because the behaviour is more predictable — a disabled source never holds a cache.
What changed
Single file:
src-tauri/src/commands/adblock.rs(+273 / −4).set_ad_block_source_enabledis refactored into the existing_impl(state, ...)pattern (matchesadd_ad_block_source_implandset_ad_block_source_rules_limit_override_impl) so the disable / re-enable / no-op edges are unit-testable without a TauriState. The public IPC command is a one-line shim.adblock_store::delete_cache, which is already idempotent (NotFoundis success). IO errors are swallowed — a missed delete is a hygiene issue, not a correctness issue, andsweep_orphan_cachescovers it on next startup.fetch_and_cache_source(... force=false)so RFC 7232 conditional GET (#193) saves bandwidth when the upstream still has the prior ETag. The 304-with-missing-cache downgrade (#206 finding 2) handles the case where the disable path'sdelete_cacheleft no file on disk — exactly the disable→enable scenario this PR introduces.record_fetch_errorpopulateslast_errorfor the existing UI badge.Tests
Three new
tokio::tests undercommands::adblock::tests:set_ad_block_source_enabled_impl_disable_drops_cacherule_count,etag) is preserved (matches the 304 contract from #193)set_ad_block_source_enabled_impl_re_enable_refetches_cacheset_ad_block_source_enabled_impl_same_value_is_noopprev_enabledcapture for an unconditional delete-or-fetchcargo test --all-featuresreports 187 passed, 0 failed (184 existing + 3 new).CI gates
cargo fmt --all -- --check✓cargo clippy --all-targets --all-features -- -D warnings✓cargo test --all-features✓cargo build✓Known limitation (acknowledged, not fixed here)
If a user clicks Refresh and then immediately toggles Disabled, the in-flight fetch is serialized behind the per-source gate (#206 finding 1) but does not block on
ad_block_state. The disable'sdelete_cachecan therefore run, and then the in-flight fetch can write the cache behind it — leaving a fresh cache file lingering for a now-disabled source until the next toggle orsweep_orphan_cacheson restart.This is a transient hygiene issue, not a correctness issue — the engine classifies by
s.enabled, so the lingering cache stays unloaded. Fixing it would mean teachingfetch_and_cache_sourceto re-checks.enabledafter the per-source gate is released; out of scope for this PR (a cache-hygiene follow-up). Documented in the function doc comment so future reviewers know it's intentional.Follow-ups (separate PRs from this one)
This PR only addresses issue #199 sub-task C. Sub-tasks A (trie) and B (hit-rate / refresh-duration metrics) are tracked separately and intentionally not bundled — see #199 for the full breakdown.
Frontend UX note (not addressed here)
The
setAdBlockSourceEnabledAtominsrc/stores/profiles/actions.tsdoes not currently toggleisAdBlockLoadingAtom, so the toggle UI does not show a spinner during the post-enable fetch (which can take up toFETCH_TIMEOUT_SECSon network failure). The behaviour matches the existingrefresh_ad_block_sourcebutton at the atom level (await inline, no UI spinner for the toggle path). Adding a per-source spinner is a UX-only follow-up that can ship independently of this backend change.🤖 Generated with Codex