From 7bb484ada3a6efd7521a44e3cc5bbd9dc7fb9dec Mon Sep 17 00:00:00 2001 From: Razvan Dimescu Date: Wed, 15 Apr 2026 23:48:09 +0300 Subject: [PATCH] refactor(windows): deduplicate after simplify review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop the duplicate WINDOWS_SERVICE_NAME constant; call sites use the single source of truth at windows_service::SERVICE_NAME. - windows_service_exe_path and service_config_path now compose from crate::data_dir() instead of re-parsing %PROGRAMDATA% locally. - Factor the 6× sc.exe invocation boilerplate into a run_sc helper. - Replace the 200ms try_recv polling loop in the service dispatcher with a recv_timeout wait — cuts shutdown latency and idle CPU. - stop_service_scm/delete_service_scm now log warnings instead of silently swallowing failures, so unexpected errors are visible. --- src/system_dns.rs | 108 +++++++++++++++++++---------------------- src/windows_service.rs | 22 ++++----- 2 files changed, 61 insertions(+), 69 deletions(-) diff --git a/src/system_dns.rs b/src/system_dns.rs index b39f661..826101d 100644 --- a/src/system_dns.rs +++ b/src/system_dns.rs @@ -729,20 +729,24 @@ fn install_windows() -> Result<(), String> { Ok(()) } -#[cfg(windows)] -const WINDOWS_SERVICE_NAME: &str = "Numa"; - /// Stable install location for the service binary. SCM keeps a handle to /// this path; the user's Downloads folder (where `current_exe()` points at /// install time) is not durable. #[cfg(windows)] fn windows_service_exe_path() -> std::path::PathBuf { - std::path::PathBuf::from( - std::env::var("PROGRAMDATA").unwrap_or_else(|_| "C:\\ProgramData".into()), - ) - .join("numa") - .join("bin") - .join("numa.exe") + crate::data_dir().join("bin").join("numa.exe") +} + +/// Run `sc.exe` with the given args and return its merged stdout/stderr on +/// failure. `sc` emits errors on stdout (not stderr) on Windows, so the +/// caller reads stdout to format a useful error. +#[cfg(windows)] +fn run_sc(args: &[&str]) -> Result { + let out = std::process::Command::new("sc") + .args(args) + .output() + .map_err(|e| format!("failed to run sc {}: {}", args.first().unwrap_or(&""), e))?; + Ok(out) } /// Copy the currently-running binary to the service install location. SCM @@ -782,24 +786,22 @@ fn remove_service_binary() { #[cfg(windows)] fn register_service_scm(exe: &std::path::Path) -> Result<(), String> { let bin_path = format!("\"{}\" --service", exe.display()); + let name = crate::windows_service::SERVICE_NAME; // sc.exe uses a leading space as its `name= value` delimiter; the space // after `=` is mandatory. - let create = std::process::Command::new("sc") - .args([ - "create", - WINDOWS_SERVICE_NAME, - "binPath=", - &bin_path, - "DisplayName=", - "Numa DNS", - "start=", - "auto", - "obj=", - "LocalSystem", - ]) - .output() - .map_err(|e| format!("failed to run sc create: {}", e))?; + let create = run_sc(&[ + "create", + name, + "binPath=", + &bin_path, + "DisplayName=", + "Numa DNS", + "start=", + "auto", + "obj=", + "LocalSystem", + ])?; if !create.status.success() { let out = String::from_utf8_lossy(&create.stdout); // "service already exists" is 1073 — treat as idempotent success. @@ -808,30 +810,23 @@ fn register_service_scm(exe: &std::path::Path) -> Result<(), String> { } } - let _ = std::process::Command::new("sc") - .args([ - "description", - WINDOWS_SERVICE_NAME, - "Self-sovereign DNS resolver (ad blocking, DoH/DoT, local zones).", - ]) - .status(); + let _ = run_sc(&[ + "description", + name, + "Self-sovereign DNS resolver (ad blocking, DoH/DoT, local zones).", + ]); // Restart on crash: 5s, 5s, 10s; reset failure counter after 60s. - let _ = std::process::Command::new("sc") - .args([ - "failure", - WINDOWS_SERVICE_NAME, - "reset=", - "60", - "actions=", - "restart/5000/restart/5000/restart/10000", - ]) - .status(); + let _ = run_sc(&[ + "failure", + name, + "reset=", + "60", + "actions=", + "restart/5000/restart/5000/restart/10000", + ]); - eprintln!( - " Registered service '{}' (boot-time).", - WINDOWS_SERVICE_NAME - ); + eprintln!(" Registered service '{}' (boot-time).", name); Ok(()) } @@ -840,10 +835,7 @@ fn register_service_scm(exe: &std::path::Path) -> Result<(), String> { /// return the underlying error string rather than masking it. #[cfg(windows)] fn start_service_scm() -> Result<(), String> { - let out = std::process::Command::new("sc") - .args(["start", WINDOWS_SERVICE_NAME]) - .output() - .map_err(|e| format!("failed to run sc start: {}", e))?; + let out = run_sc(&["start", crate::windows_service::SERVICE_NAME])?; if !out.status.success() { let text = String::from_utf8_lossy(&out.stdout); if text.contains("1056") { @@ -854,20 +846,22 @@ fn start_service_scm() -> Result<(), String> { Ok(()) } -/// Stop the service. Returns Ok if already stopped — idempotent. +/// Stop the service. Idempotent — already-stopped or missing service logs +/// a warning but doesn't error, since both callers (install re-run, +/// uninstall) want best-effort cleanup rather than hard failure. #[cfg(windows)] fn stop_service_scm() { - let _ = std::process::Command::new("sc") - .args(["stop", WINDOWS_SERVICE_NAME]) - .status(); + if let Err(e) = run_sc(&["stop", crate::windows_service::SERVICE_NAME]) { + log::warn!("sc stop failed: {}", e); + } } -/// Remove the service from SCM. Safe if already absent. +/// Remove the service from SCM. Idempotent — see `stop_service_scm`. #[cfg(windows)] fn delete_service_scm() { - let _ = std::process::Command::new("sc") - .args(["delete", WINDOWS_SERVICE_NAME]) - .status(); + if let Err(e) = run_sc(&["delete", crate::windows_service::SERVICE_NAME]) { + log::warn!("sc delete failed: {}", e); + } } #[cfg(windows)] diff --git a/src/windows_service.rs b/src/windows_service.rs index c51339c..a1403d7 100644 --- a/src/windows_service.rs +++ b/src/windows_service.rs @@ -62,7 +62,7 @@ fn run_service() -> windows_service::Result<()> { // once the SCM tells us to stop — we can't block the dispatcher thread // forever without preventing graceful shutdown. let config_path = service_config_path(); - let (runtime_stop_tx, runtime_stop_rx) = mpsc::channel::<()>(); + let (server_done_tx, server_done_rx) = mpsc::channel::<()>(); let server_thread = std::thread::spawn(move || { let runtime = match tokio::runtime::Builder::new_multi_thread() @@ -72,28 +72,25 @@ fn run_service() -> windows_service::Result<()> { Ok(rt) => rt, Err(e) => { log::error!("failed to build tokio runtime: {}", e); - let _ = runtime_stop_tx.send(()); + let _ = server_done_tx.send(()); return; } }; - // block_on returns when serve::run's UDP loop errors out OR when the - // runtime is dropped from another thread. Either signals exit. if let Err(e) = runtime.block_on(crate::serve::run(config_path)) { log::error!("numa serve exited with error: {}", e); } - let _ = runtime_stop_tx.send(()); + let _ = server_done_tx.send(()); }); // Wait for either SCM stop or server termination. loop { - if shutdown_rx.try_recv().is_ok() { + if shutdown_rx.recv_timeout(Duration::from_millis(500)).is_ok() { break; } - if runtime_stop_rx.try_recv().is_ok() { + if server_done_rx.try_recv().is_ok() { break; } - std::thread::sleep(Duration::from_millis(200)); } // The server's tokio runtime runs detached inside server_thread. Abandon @@ -124,9 +121,10 @@ pub fn run_as_service() -> windows_service::Result<()> { /// Path to the config file used when running under SCM. SCM launches the /// service with SYSTEM's working directory (usually `C:\Windows\System32`), -/// so a relative `numa.toml` lookup won't find anything meaningful — use an -/// absolute path under `%PROGRAMDATA%` instead. +/// so a relative `numa.toml` lookup won't find anything meaningful. fn service_config_path() -> String { - let base = std::env::var("PROGRAMDATA").unwrap_or_else(|_| "C:\\ProgramData".into()); - format!("{}\\numa\\numa.toml", base) + crate::data_dir() + .join("numa.toml") + .to_string_lossy() + .into_owned() }