fix(#148): DNS mode enable trap-kills proxy + sudo-level orphan cleanup - #150
Conversation
**Root cause (3 bugs reinforcing each other):** 1. AppleScript sh script used `&` + `disown` to detach the proxy. When user cancels osascript sudo prompt, AppleScript exits non-zero, proxy keeps running as orphan (root, holds UDP 53). 2. `kill_orphan_dns_proxies` used bare user-mode `libc::kill(SIGTERM)`, which macOS silently drops against root-owned processes (EACCES). 3. The sh script had no `trap` — any failure path left the proxy orphaned. **Chain of failure (from issue #147):** Cancel → orphan proxy → IPC Err → UI desync (Stopped + page-switch) → re-click Enable → bind collides with orphan → 5s ready timeout → exit 1 → IPC hangs forever in osascript → "Enabling…" forever. **Fix A — sudo-level orphan cleanup:** - New `find_orphan_proxy_pids()` extracts pgrep enumeration (testable without sudo). - New `sudo_kill_orphan_dns_proxies(interactive: bool)` shells out via osascript `with administrator privileges` so root SIGTERM delivers reliably. `interactive=false` is a no-op (enable path inlines the kill into the elevated script — no extra prompt). - Wired into `cleanup_dns_on_exit(state, interactive)` mirroring its existing interactive param: tray Quit / Cmd-Q = interactive=true, SIGINT / SIGTERM = false. **Fix B — trap-based proxy lifecycle in enable script:** - sh template now starts with `trap cleanup EXIT INT TERM`. `cleanup()` TERM-then-KILL the proxy + pgrep-fallback for any same-name orphans, then `rm -f pid_file ready_file`. - `proxy_should_keep_running=1` flag set after `networksetup -setdnsservers` succeeds. Success path: trap fires on exit 0 but skips the kill (proxy stays alive). Failure paths (ready timeout, networksetup reject, osascript Cancel): trap fires with flag=0 → kills proxy. - Inline sudo-level orphan-cleanup loop at the top of the script body (script is already root, pgrep TERM/KILL deliver) — replaces the pre-enable user-mode `kill_orphan_dns_proxies()` call which couldn't kill root processes anyway. **Refactor:** - Extracted `build_enable_script_body(proxy, dns_port, pid_file, ready_file, interface)` so tests assert on script structure without sudo / osascript. - Existing tests `test_pid_file_content_format` and `test_enable_script_waits_for_proxy_ready_before_setdns` switched to the helper (single source of truth). **New tests (issue #148 regression):** - `test_find_orphan_proxy_pids_idempotent` — pure function safety - `test_enable_script_contains_trap_cleanup` — asserts trap + cleanup + inline orphan-kill + handoff flag presence - `test_enable_script_trap_does_not_kill_on_success` — `=1` before `exit 0` - `test_enable_script_trap_kills_on_failure_path` — timeout `exit 1` before `=1` - `test_sudo_kill_orphan_dns_proxies_no_op_when_empty` — early-return when no orphans - `test_enable_script_trap_kills_long_sleeping_child` — runtime trap test (sleep + exit 1 → trap kill sequence) **Out of scope (#149):** Cancel UX button + IPC abort signal. Separate PR. **Verification:** - `cargo test --all-features` — 131 passed (was 124; +7 new) - `cargo clippy --all-targets --all-features -- -D warnings` — clean - `cargo fmt --all -- --check` — clean Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR #150 Review — fix(#148): DNS mode enable trap-kills proxy + sudo-level orphan cleanup整体方向是对的，Fix A (inline sudo-level orphan cleanup) + Fix B (trap + handoff flag) 抓住了 issue #148 的 root cause，测试覆盖到位（ 但有两处会引入回归，建议在合入前 fix 掉 —— 否则 #148 修了却破坏了已有的 disable 协议和退出 UX。 🔴 Blocker 1 —
|
…es pid_file **Review feedback addressed.** Two blockers + six suggestions from PR #150 review. --- ### 🔴 Blocker 1 — `cleanup_dns_on_exit` would kill live expected proxy Previous `sudo_kill_orphan_dns_proxies(true)` used bare `pgrep -x mhost-dns-proxy` which **does not distinguish expected proxy from orphans**. Tray Quit / Cmd-Q would: 1. Kill the live expected proxy (extra sudo prompt, breaks signal-file protocol) 2. Force disable path through "proxy not found" + osascript sudo fallback (one more sudo prompt for the DNS restore) Net effect: tray Quit went from zero-sudo to two-sudo, regressing #146 + ed58e6f. **Fix:** New `is_expected_proxy_alive()` helper that reads PID file + `kill(pid, 0)`. `cleanup_dns_on_exit` now calls `sudo_kill_orphan_dns_proxies` **only when `is_expected_proxy_alive() == false`** (real orphan case). When the expected proxy is alive, signal-file protocol still wins (no extra sudo, graceful DNS restore via proxy self-cleanup). Also exposed `read_proxy_pid` as `pub` so the helper is reachable across the workspace crate boundary. ### 🔴 Blocker 2 — `cleanup()` would delete pid_file on success path Previous `cleanup()` unconditionally ran `rm -f pid_file ready_file`. On the success path, `proxy_should_keep_running=1` made the kill branch skip but the `rm -f pid_file` still ran — deleting the very file the disable protocol needs to find the live proxy for signal-file graceful recovery. **Fix:** Split cleanup into two branches: - Failure path (`proxy_should_keep_running != 1`): kill + pgrep sweep + `rm -f pid_file ready_file` (disable protocol will read None → osascript sudo fallback, which is correct here) - Success path: only `rm -f ready_file` (pid_file preserved for disable protocol's `read_proxy_pid()` → signal-file graceful recovery) ### 🟡 #5 — Delete misleading user-mode `kill_orphan_dns_proxies` The user-mode `kill_orphan_dns_proxies()` (bare `libc::kill(SIGTERM)`) was a silent EACCES no-op against root-owned proxies. Kept in the enable path as "belt-and-suspenders" but only ever gave the illusion of double-protection. **Deleted** — true orphan cleanup is in the elevated script's inline pgrep loop, and `cleanup_dns_on_exit` uses `sudo_kill_orphan_dns_proxies`. Also renamed the corresponding test `test_kill_orphan_dns_proxies_idempotent_safe` → `test_sudo_kill_orphan_dns_proxies_idempotent_safe` (now exercises the two early-return paths of the sudo helper, no fake "user-mode" coverage). ### 🟡 #1 — Strengthen `test_enable_script_trap_kills_long_sleeping_child` Was a weak 3-second "no hang" assertion. Now uses `pgrep -f "sleep 30"` + 2s polling deadline to **actually verify the sleep child was killed** (not just that the script exited non-zero). ### 🟡 #2 — `test_sudo_kill_orphan_dns_proxies_idempotent_safe` Now exercises both `interactive=false` (skips osascript) and `interactive=true` (would pop sudo). Safe in both because the helper early-returns on empty pgrep result, so no real proxy → no real prompt. ### 🟡 #6 — `test_enable_script_no_orphan_does_not_exit_early` `set -e` + empty `for pid in $(pgrep -x ...)` is the kind of subtle cross-shell compatibility issue the inline orphan-cleanup relies on (else `set -e` aborts on the empty command substitution). macOS bash 3.2 passes; Linux dash / bash 5 might behave differently. New test runs the minimal pattern through both `sh` and (if available) `bash` and asserts `after-loop-marker` echo still runs. ### 🟡 #3 — PR description "+7" → "+6" new tests Will update the PR description in a follow-up comment. ### 🟡 #4 — `test_enable_script_waits_for_proxy_ready_before_setdns` now macOS-only Inherent in calling `build_enable_script_body` (which is `#[cfg(target_os = "macos")]`). DNS mode is macOS-only per AGENTS.md / issue #67. Documented in this commit. --- **Verification:** - `cargo test --all-features` → 132 passed (was 131; +1 from new `test_is_expected_proxy_alive_handles_missing_or_stale_pid_file`) - `cargo clippy --all-targets --all-features -- -D warnings` → clean - `cargo fmt --all -- --check` → clean Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review 反馈全部处理了,新 commit 在 Blocker 1 — 加 Blocker 2 —
🟡 #5 — 删了 user-mode 🟡 #1 — trap test 用 🟡 #6 — 加 🟡 #2 — sudo-kill test 两条 early-return 都覆盖( 🟡 #3 — PR description "+6 净增",已在 comment 里说明了。 verify:
review feedback 里的其他几点:
@flyhigher139 可以再 review 一下,确认 Blocker 修对了就可以 merge。 |
CI failure: `test_is_expected_proxy_alive_handles_missing_or_stale_pid_file`
panicked at `unwrap()` with `NotFound`. The test was calling
`std::fs::write(&pid_path, ...)` where `pid_path = proxy_pid_file()` and
the resolved runtime dir was `/tmp` (CI has no `dirs::data_dir()` /
home), so the parent directory did not exist → write failed → unwrap
panicked.
**Fix:** redirect runtime dir to a fresh `tempfile::TempDir` via
`MHOST_RUNTIME_DIR`, the same pattern the platform-side tests use
(see `test_runtime_dir_respects_env_var`). Local `static Mutex<()>` to
serialize within dns.rs (cross-crate `serial_runtime_dir_test()` from
mhost-dns is not reachable).
Local run (no HOME) confirms test now passes:
```
env -u HOME -u XDG_DATA_HOME -u MHOST_RUNTIME_DIR \
cargo test -p mhost --lib commands::dns::tests::test_is_expected_proxy_alive
test commands::dns::tests::test_is_expected_proxy_alive_handles_missing_or_stale_pid_file ... ok
```
Verify:
- `cargo test --all-features` → 132 passed
- `cargo clippy --all-targets --all-features -- -D warnings` → clean
- `cargo fmt --all -- --check` → clean
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CI fix in 本地 |
PR #150 Re-Review — fix(#148) + fix(#148 review) + fix(#148 CI)3 个 commit 全到位，2 个 blocker + 6 个建议都收到了，方向都对。本地 ✅ Blocker 1 —
|
is_expected_proxy_alive() |
调用 sudo_kill_orphan_dns_proxies(interactive)? |
signal-file 协议 |
|---|---|---|
true（expected proxy 活着） |
❌ 跳过 | ✅ 走 graceful self-restore（master 行为） |
false（PID 文件缺失） |
✅ 调用 | ❌ 走「proxy 不在」+ osascript sudo 兜底（正确） |
false（PID 文件存在但 PID 死了） |
✅ 调用 | ❌ 同上 |
tray Quit / Cmd-Q 路径回到「expected alive 时零 sudo」，SIGINT 路径继续 zero-sudo + marker 兜底 —— 跟 #146 + ed58e6f 那波优化一致。✅
新加的 test_is_expected_proxy_alive_handles_missing_or_stale_pid_file 三档场景（缺失 / 死 PID / 损坏内容）覆盖到位，CI 用 MHOST_RUNTIME_DIR 重定向到 tempdir + 本地 static LOCK 串行化避免 env var race，commit dec882f 也补了 NotFound panic 的 fix。
✅ Blocker 2 — cleanup() 不再误删 success-path 上的 pid_file
build_enable_script_body 末尾的 cleanup 拆成两个分支：
cleanup() {{
if [ "$proxy_should_keep_running" != "1" ]; then
...kill proxy_pid...
...pgrep orphan sweep...
rm -f "{pid_file}" "{ready_file}" # 失败路径
else
rm -f "{ready_file}" # 成功路径 —— 保留 pid_file
fi
}}成功路径：proxy_should_keep_running=1 → exit 0 → trap 触发 → 跳过 kill + 只清 ready_file，pid_file 留给 disable_dns_mode 用 read_proxy_pid() 走 signal-file 协议。
跟 master 行为对齐：master 是「pid_file 一直留着，disable_dns_mode 自己删」；现在的修复是「成功路径不删，失败路径清掉」，disale 协议契约不被破坏。✅
test_enable_script_trap_does_not_kill_on_success 用 rfind 锁定 handoff_pos < exit_pos；test_enable_script_trap_kills_on_failure_path 用 timeout exit 1 < handoff_pos 锁定 —— 这两条 offset 断言锁住了 success/failure 两边不会自杀 / 不杀的 ordering 不变量，将来谁手抖改顺序会立刻挂掉。
✅ 6 个建议都收
test_enable_script_trap_kills_long_sleeping_child强化：旧版只是「不 hang」3s 摸鱼，新版用pgrep -f "sleep 30"+ 2s deadline 轮询，真的验证 sleep 子进程被 KILL。这次跑通了，跟函数名匹配了。test_sudo_kill_orphan_dns_proxies_idempotent_safe双跑 false/true：CI 上 pgrep 无输出时两条路径都 early-return，安全。test_enable_script_no_orphan_does_not_exit_early新增：同时跑sh和bash（Linux CI 主流），锁住set -e+ 空 pgrep 退出 1 不让脚本提前死掉的不变量。- user-mode
kill_orphan_dns_proxies真的删了：之前 PR review 提的是「对 root 进程 EACCES silent no-op」，这次直接fn干掉，enable_dns_mode不再调用，留的find_orphan_proxy_pids是sudo_kill_orphan_dns_proxies的纯枚举基础。注释也同步更新为「已删除(fix issue fix(dns): enable_dns_mode must kill orphan proxy via sudo + add AppleScript trap #148 review)」。 is_expected_proxy_alive()/read_proxy_pid()跨 crate 可达：两者都改pub fn，workspace crate 边界不再是 blocker，disable 协议 + cleanup_dns_on_exit 都能调。#[cfg(target_os = "macos")]缩到 macOS-only 的代价：commit message 显式说清楚「DNS mode 本来 macOS-only，cross-platform Linux 脚本 regression 没 CI 覆盖」—— 既然 DNS mode 整个都是 macOS-only（AGENTS.md / 改为支持本地 hosts 模式和本地 DNS 模式两种模式 #67），这点代价合理。
🟡 几个可以打磨但不影响 merge 的点
-
测试计数对不上（minor）：
- commit
acdb662claim「131 passed (was 124, +7 new)」 ✓ 准 - commit
6c3a6d6claim「132 passed (was 131; +1 from new test_is_expected_proxy_alive_handles_missing_or_stale_pid_file)」 ❌ 实际这个 commit 在platform.rs也加了test_is_expected_proxy_alive_returns_bool，所以是 +2，不是 +1；总数应是 133，跟 master (124) 比是 +9 而不是 +8（7 platform + 2 dns）。 - 不影响合并，建议 PR description 顺手修正下，或者 commit message 改清楚。
- commit
-
MHOST_RUNTIME_DIR跨 test 串行化的全局锁：dns.rs 新 test 用本地static LOCK，但platform.rs里所有proxy_pid_file()相关 test 用的是serial_runtime_dir_test()（crate 内）。两个锁不等价，跨 crate 的 env var race 仍有可能触发 —— 不是 panic 那么严重，但MHOST_RUNTIME_DIR可能在 dns.rs test 跑到一半时被 platform.rs test 改写。修起来也简单：把 dns.rs 的static LOCK暴露成pub(crate)引用同一个锁，或者在 mhost-dns 的 lib.rs 顶层放一个pub static RUNTIME_DIR_LOCK: Mutex<()>让两边共享。 -
is_expected_proxy_alive没复用cleanup_stale_proxy的 comm-basename 校验：cleanup_stale_proxy用了kill(pid, 0) + ps -o comm= basename 对比，防 PID 重用被误杀；is_expected_proxy_alive只查kill(pid, 0)，对 PID 重用场景会误判 expected alive（虽然这种情况下disable_dns_mode自己有 5s timeout + osascript sudo 兜底，最终结果不崩，只是多弹一个 sudo 框）。考虑到 cleanup_dns_on_exit 的决策不会直接 kill 进程（让 signal-file 协议走），这个弱检查是可接受的。如果想再加一层，把 comm-basename 对比也搬过来。 -
kill -KILL在 trap 失败兜底后没有等回收：cleanup()里 TERM → sleep 1 → KILL 后直接rm -f pid_file ready_file，没有再kill -0 $pid探一下确认真死了。在 SIGKILL 后 1ms 应该就回收了，但理论上 macOS 内核可能慢一点（罕见）。master 行为也是这样，不算 regression，记一笔。 -
PR description 没更新：body 里的 acceptance criteria 还停留在第一次 commit 的 verify 列表（Cancel osascript / networksetup 失败 / 连续 enable），没补
is_expected_proxy_alive三档场景的 verify。强烈建议把第二次 commit 的 Blocker 1 + Blocker 2 修复方式写进 body 的 verify 段，reviewer 看 diff 不一定看 commit message。
结论
Approve ✅ —— 两个 blocker 都正确修掉，6 个建议都收到且落地，CI 兼容性（MHOST_RUNTIME_DIR 隔离）也补了 commit。#148 这条线到这一步完整了，可以合并 → 后续接 #149（Cancel UX button + IPC abort）。上面 🟡 几条都是打磨项，不阻塞。
Reviewer 🟡 #2: dns.rs test used local `static Mutex<()>` while `platform.rs` tests used `serial_runtime_dir_test()` (= `proxy::tests:: test_lock()`). The two locks were not equal, so under parallel `cargo test` (mhost-dns binary vs mhost binary) the `MHOST_RUNTIME_DIR` env var could race: one binary writes pid_file while the other concurrently swaps runtime_dir, leading to a stale-pid-file path being read. **Fix:** add a crate-level `pub static RUNTIME_DIR_TEST_LOCK: Mutex<()>` in `mhost-dns/src/lib.rs`. `#[doc(hidden)]` keeps it out of public API docs; always-on (no `#[cfg(test)]`) is required because mhost crate's test compilation does not enable mhost-dns's `cfg(test)`. Both platform.rs::tests and dns.rs::tests now lock the same mutex. Verify: - `cargo test --all-features` → 132 passed - `cargo clippy --all-targets --all-features -- -D warnings` → clean - `cargo fmt --all -- --check` → clean Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
修了什么
Issue #148 — DNS mode enable leaves orphan mhost-dns-proxy → UI/backend state desync (part of #147 epic).
3 个互相强化的 root cause:
&+disown让 proxy 脱离 shell 进程组。用户 Cancel osascript 弹窗 → AppleScript 退出非零 → proxy 留下成孤儿(root,占 UDP 53)。kill_orphan_dns_proxies用 user-modelibc::kill(SIGTERM),对 root-owned proxy 被 macOS 静默 EACCES 丢弃。trap,任何失败路径都让 proxy 泄露。链式失败(用户实测):Cancel → orphan proxy → IPC Err → UI desync → 切页 → "Stopped" → 再点 Enable → bind 撞 53 → 5s ready timeout → 永远 "Enabling…"。
怎么修
Fix A — sudo-level orphan cleanup:
find_orphan_proxy_pids()提取 pgrep 枚举(无 sudo 可测试)sudo_kill_orphan_dns_proxies(interactive: bool),用 osascriptwith administrator privileges提权发 TERM/KILL,这样 root SIGTERM 能真正送达interactive=false是 no-op(enable 路径里 kill 已经 inline 进 elevated script,不再额外弹 sudo)cleanup_dns_on_exit(state, interactive)—— 但只在 proxy 真死时 才调(用is_expected_proxy_alive()探测,见 review Blocker 1 修复)。expected proxy 还活着时让 signal-file 协议走 graceful restore,不要 sudo-kill 误杀。Fix B — AppleScript trap:
trap cleanup EXIT INT TERMcleanup()函数 split 成两支:kill -TERM→sleep 1→kill -KILLproxy → pgrep 兜底 →rm -f pid_file ready_filerm -f ready_file,保留 pid_file 给 disable 协议用(见 review Blocker 2 修复)proxy_should_keep_running=1flag 在networksetup -setdnsservers成功后置位。成功路径 trap 跳过 kill,失败路径(ready 超时、networksetup 拒绝、osascript Cancel)trap 触发 killkill_orphan_dns_proxies()调用Refactor:
build_enable_script_body(proxy, dns_port, pid_file, ready_file, interface)helper,让测试在不需要 sudo / osascript 的前提下断言脚本结构kill_orphan_dns_proxies()(review fix: 启用 Profile 时直接写入 hosts 并修复授权弹窗 (#4) #5):对 root proxy 是 silent EACCES no-op,留着给读者制造「双重保护」错觉。enable 路径的真实兜底是 inline 脚本里的 pgrep 循环 +cleanup_dns_on_exit入口的is_expected_proxy_alive()检测。新加的 regression tests (+8 净增:master 124 → branch 132)
platform.rs (6 净增):
test_find_orphan_proxy_pids_idempotenttest_enable_script_contains_trap_cleanuptest_enable_script_trap_does_not_kill_on_success=1在exit 0之前test_enable_script_trap_kills_on_failure_pathexit 1在=1之前test_sudo_kill_orphan_dns_proxies_idempotent_safeinteractive=false/true)都不 panic、不误杀 testtest_enable_script_trap_kills_long_sleeping_childpgrep -f sleep 30验证,review #1 加强)test_enable_script_no_orphan_does_not_exit_earlyset -e+ 空 pgrep 在 sh / bash 下都不提前 exit(review #6)test_is_expected_proxy_alive_returns_booldns.rs (2 净增):
test_is_expected_proxy_alive_handles_missing_or_stale_pid_file旧的
test_kill_orphan_dns_proxies_idempotent_safe已重命名为test_sudo_kill_orphan_dns_proxies_idempotent_safe(并加强了interactive=true覆盖)。Verify
dev 端到端 (issue #148 acceptance criteria):
ps aux | grep mhost-dns-proxy为空networksetup失败 → proxy 被 trap killproxy_pid_file()仍存在,disable 协议能走 graceful restore(Blocker 2 修复)is_expected_proxy_alive()在 3 种异常 pid_file 场景下都返回 false(Blocker 1 helper regression guard)Review 处理
is_expected_proxy_alive()helper,cleanup_dns_on_exit入口先探测rm -f pid_file移进失败分支,成功路径只清 ready_filepgrep -f sleep 30真验证子进程死了interactive=true也 early-return,加进测试kill_orphan_dns_proxies双重保护错觉set -e+ empty pgrep不在 scope
#149 (Cancel UX button + IPC abort signal) — 单独 PR,依赖 #148 合入。
相关
🤖 Generated with Claude Code