From 95888f0fe0841a04db1d4823ebc4703d851903df Mon Sep 17 00:00:00 2001 From: Chris Fenton Date: Mon, 9 Mar 2026 00:30:44 -0700 Subject: [PATCH 1/7] fix: harden daemon setup flow with stale process cleanup, timeouts, and improved init output - Kill stale daemon processes on re-init using PID file - Write daemon PID file on startup, clean up on exit - Verify daemon is actually alive after spawn (wait up to 5s) - Report daemon startup failure clearly with actionable hints - Add IPC request timeout (3s default) to prevent hangs - Add connection timeout in `up` command to fail fast with dead daemon - Print complete DNS setup instructions (dnsmasq + resolver on macOS) - Print CA trust command with correct cert path and sudo note - Add 6 new e2e tests: init output verification, up fails fast with dead daemon, daemon PID file, re-init stale cleanup, CA path, sudo note - Add 1 unit test: IPC timeout on unresponsive socket Co-Authored-By: Claude Opus 4.6 --- plans/fix-daemon-setup.md | 59 ++++++++++ src/commands/init.rs | 169 ++++++++++++++++++++++++-- src/commands/up.rs | 34 ++++-- src/config.rs | 4 + src/ipc.rs | 59 +++++++++- src/proxy/mod.rs | 34 ++++-- tests/e2e.rs | 241 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 571 insertions(+), 29 deletions(-) create mode 100644 plans/fix-daemon-setup.md diff --git a/plans/fix-daemon-setup.md b/plans/fix-daemon-setup.md new file mode 100644 index 0000000..474dac7 --- /dev/null +++ b/plans/fix-daemon-setup.md @@ -0,0 +1,59 @@ +# Plan: Fix daemon setup flow + +## Problem +The daemon setup flow (`devproxy init`) has multiple bugs discovered during end-to-end testing: +1. Port 443 needs sudo but init doesn't warn about it +2. Socket permissions issue when daemon runs as root (partially fixed) +3. Daemon dies silently after init reports success +4. Init output missing DNS setup commands, CA trust path, sudo requirement +5. `devproxy up` hangs when daemon is dead instead of failing fast +6. DNS setup not documented in init flow +7. CA trust needs sudo but fails silently +8. Stale daemon processes not cleaned up on re-init + +## Changes + +### 1. `src/commands/init.rs` — Major rework +- Kill stale daemon processes before spawning a new one (find by socket, send ping, kill PID) +- Remove stale socket file on re-init +- Wait briefly after daemon spawn and verify it's actually alive (connect to socket + ping) +- Report daemon startup failure clearly instead of "ok: daemon started" then silence +- Print complete DNS setup instructions (dnsmasq install + config + resolver setup for macOS) +- Print CA trust command with correct cert path +- Note that sudo is needed for port 443 +- Pass `--port` info to next-steps output + +### 2. `src/commands/up.rs` — Add IPC connection timeout +- Add a timeout (2s) to the daemon health check so `up` fails fast instead of hanging +- Improve error message to suggest re-running `devproxy init` + +### 3. `src/ipc.rs` — Add connection timeout +- Add a `send_request_with_timeout` function (or add timeout to existing `send_request`) +- Default timeout of 3 seconds for IPC operations + +### 4. `src/proxy/mod.rs` — Write PID file +- Write daemon PID to `/daemon.pid` on startup +- Clean up PID file in `SocketCleanupGuard` drop + +### 5. `src/config.rs` — Add `pid_path()` helper +- Add `Config::pid_path()` returning `/daemon.pid` + +### 6. E2E tests in `tests/e2e.rs` +- **test_init_output_includes_dns_instructions** — verify init stderr contains dnsmasq/resolver instructions +- **test_init_output_includes_sudo_note** — verify init mentions sudo for port 443 +- **test_up_fails_fast_with_dead_daemon** — kill daemon, verify `up` fails within timeout (not hang) +- **test_reinit_kills_stale_daemon** — init twice, verify first daemon is killed +- **test_daemon_writes_pid_file** — verify PID file created after daemon start +- **test_init_output_includes_ca_trust_path** — verify CA cert path is printed + +### 7. Unit tests +- **ipc::tests::send_request_timeout** — verify IPC timeout works against non-responsive socket + +## File change list +- `src/commands/init.rs` — major rework +- `src/commands/up.rs` — add timeout to daemon check +- `src/ipc.rs` — add timeout support +- `src/proxy/mod.rs` — write PID file +- `src/config.rs` — add pid_path() +- `tests/e2e.rs` — add 6 new tests +- `docs/spec.md` — update init description with DNS/sudo info diff --git a/src/commands/init.rs b/src/commands/init.rs index dff6ab0..2e6909a 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -2,6 +2,7 @@ use crate::config::Config; use crate::proxy::cert; use anyhow::{Context, Result, bail}; use colored::Colorize; +use std::time::Duration; /// Validate that the domain looks reasonable: non-empty, contains only valid /// DNS characters, has at least one dot, and each label is 1-63 chars. @@ -30,6 +31,69 @@ fn validate_domain(domain: &str) -> Result<()> { Ok(()) } +/// Kill a stale daemon process. Reads PID from the PID file, sends SIGTERM, +/// then SIGKILL if needed. Also removes stale socket and PID files. +fn kill_stale_daemon() -> Result<()> { + let pid_path = Config::pid_path()?; + let socket_path = Config::socket_path()?; + + if pid_path.exists() { + let pid_str = std::fs::read_to_string(&pid_path).unwrap_or_default(); + if let Ok(pid) = pid_str.trim().parse::() { + // Check if process is alive + let alive = unsafe { libc::kill(pid, 0) } == 0; + if alive { + eprintln!( + "{} killing stale daemon (pid: {pid})...", + "info:".cyan() + ); + // Send SIGTERM first + unsafe { libc::kill(pid, libc::SIGTERM); } + // Wait briefly for graceful shutdown + std::thread::sleep(Duration::from_millis(500)); + // Check if still alive, send SIGKILL + if unsafe { libc::kill(pid, 0) } == 0 { + unsafe { libc::kill(pid, libc::SIGKILL); } + std::thread::sleep(Duration::from_millis(200)); + } + eprintln!("{} stale daemon killed", "ok:".green()); + } + } + let _ = std::fs::remove_file(&pid_path); + } + + // Clean up stale socket + if socket_path.exists() { + let _ = std::fs::remove_file(&socket_path); + } + + Ok(()) +} + +/// Wait for the daemon to become responsive by polling the IPC socket. +/// Returns Ok(()) if the daemon responds to a ping within the timeout, +/// or an error if it doesn't. +fn wait_for_daemon(timeout: Duration) -> Result<()> { + let socket_path = Config::socket_path()?; + let start = std::time::Instant::now(); + let poll_interval = Duration::from_millis(100); + + while start.elapsed() < timeout { + if socket_path.exists() + && std::os::unix::net::UnixStream::connect(&socket_path).is_ok() + { + // Socket is connectable -- daemon is likely alive + return Ok(()); + } + std::thread::sleep(poll_interval); + } + + bail!( + "daemon did not start within {}s. Check if port is available and you have permissions.", + timeout.as_secs() + ) +} + pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { validate_domain(domain)?; let config = Config { domain: domain.to_string() }; @@ -52,7 +116,7 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { eprintln!("{} CA certificate generated", "ok:".green()); // Trust the CA - eprintln!("trusting CA in system keychain (may require sudo)..."); + eprintln!("trusting CA in system keychain (requires sudo)..."); match cert::trust_ca_in_system(&ca_cert_path) { Ok(()) => eprintln!("{} CA trusted in system keychain", "ok:".green()), Err(e) => { @@ -61,8 +125,17 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { "warn:".yellow() ); eprintln!( - " manually trust: {}", - ca_cert_path.display().to_string().cyan() + " run manually with sudo:" + ); + #[cfg(target_os = "macos")] + eprintln!( + " sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain {}", + ca_cert_path.display() + ); + #[cfg(target_os = "linux")] + eprintln!( + " sudo cp {} /usr/local/share/ca-certificates/devproxy-ca.crt && sudo update-ca-certificates", + ca_cert_path.display() ); } } @@ -107,10 +180,20 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { if no_daemon { eprintln!("{} daemon spawn skipped (--no-daemon)", "ok:".green()); } else { + // Kill any stale daemon from a previous init + kill_stale_daemon()?; + + if port < 1024 { + eprintln!( + "{} port {port} requires root privileges (sudo)", + "info:".cyan() + ); + } + eprintln!("starting daemon on port {port}..."); let exe = std::env::current_exe().context("could not determine binary path")?; - let mut cmd = std::process::Command::new(exe); + let mut cmd = std::process::Command::new(&exe); cmd.args(["daemon", "--port", &port.to_string()]); // Forward DEVPROXY_CONFIG_DIR so the daemon uses the same config dir @@ -139,18 +222,84 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { // Spawn a thread to reap the child so it does not become a zombie. // After setsid(), the child won't receive signals when the parent exits. std::thread::spawn(move || { let _ = child.wait(); }); - eprintln!("{} daemon started (pid: {})", "ok:".green(), pid); + + // Wait for daemon to become responsive (or fail fast) + match wait_for_daemon(Duration::from_secs(5)) { + Ok(()) => { + eprintln!("{} daemon started (pid: {pid})", "ok:".green()); + } + Err(e) => { + eprintln!( + "{} daemon failed to start: {e}", + "error:".red() + ); + if port < 1024 { + eprintln!( + " {} port {port} requires root. Try: sudo devproxy init --domain {domain}", + "hint:".yellow() + ); + } + bail!("daemon failed to start. See error above."); + } + } } eprintln!(); eprintln!("{}", "Setup complete!".green().bold()); eprintln!(); eprintln!("Next steps:"); - eprintln!(" 1. Set up wildcard DNS for *.{domain} -> 127.0.0.1"); - eprintln!(" macOS: brew install dnsmasq"); - eprintln!(" Quick: echo 'address=/.{domain}/127.0.0.1' >> /opt/homebrew/etc/dnsmasq.conf"); - eprintln!(" 2. Add a devproxy.port label to your docker-compose.yml"); - eprintln!(" 3. Run: devproxy up"); + eprintln!(); + + // DNS setup instructions + eprintln!(" {} Set up wildcard DNS for *.{domain} -> 127.0.0.1", "1.".bold()); + #[cfg(target_os = "macos")] + { + eprintln!(); + eprintln!(" Install dnsmasq (if not already installed):"); + eprintln!(" brew install dnsmasq"); + eprintln!(" sudo brew services start dnsmasq"); + eprintln!(); + eprintln!(" Add wildcard DNS rule:"); + eprintln!(" echo 'address=/.{domain}/127.0.0.1' >> $(brew --prefix)/etc/dnsmasq.conf"); + eprintln!(" sudo brew services restart dnsmasq"); + eprintln!(); + // Extract the TLD for the resolver + let tld = domain.rsplit('.').next().unwrap_or(domain); + eprintln!(" Create resolver for .{tld} domains:"); + eprintln!(" sudo mkdir -p /etc/resolver"); + eprintln!(" echo 'nameserver 127.0.0.1' | sudo tee /etc/resolver/{tld}"); + } + #[cfg(not(target_os = "macos"))] + { + eprintln!(" Example: echo 'address=/.{domain}/127.0.0.1' >> /etc/dnsmasq.conf"); + } + eprintln!(); + + // CA trust reminder + eprintln!(" {} Trust the CA certificate (if not done above)", "2.".bold()); + eprintln!(" CA cert: {}", ca_cert_path.display().to_string().cyan()); + #[cfg(target_os = "macos")] + eprintln!( + " sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain {}", + ca_cert_path.display() + ); + eprintln!(); + + // Project setup + eprintln!(" {} Add a devproxy.port label to your docker-compose.yml", "3.".bold()); + eprintln!(); + eprintln!(" {} Run: devproxy up", "4.".bold()); + + if port == 443 && !no_daemon { + eprintln!(); + eprintln!( + " {} The daemon is listening on port 443 which requires sudo.", + "note:".yellow() + ); + eprintln!( + " If the daemon failed to bind, re-run: sudo devproxy init --domain {domain}" + ); + } Ok(()) } diff --git a/src/commands/up.rs b/src/commands/up.rs index ca7ab13..61b1025 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -46,7 +46,8 @@ pub fn run() -> Result<()> { // Write project file (slug tracking -- used by `down` and `open`) config::write_project_file(compose_dir, &slug)?; - // Verify daemon is running before starting containers + // Verify daemon is running before starting containers. + // Use a short timeout (2s) so we fail fast instead of hanging forever. let socket_path = Config::socket_path()?; if !socket_path.exists() { // Clean up files we already wrote @@ -54,13 +55,30 @@ pub fn run() -> Result<()> { let _ = std::fs::remove_file(compose_dir.join(".devproxy-project")); bail!("daemon is not running (no socket at {}). Run `devproxy init` first.", socket_path.display()); } - if std::os::unix::net::UnixStream::connect(&socket_path).is_err() { - let _ = std::fs::remove_file(&override_path); - let _ = std::fs::remove_file(compose_dir.join(".devproxy-project")); - bail!( - "daemon is not running (could not connect to {}). Run `devproxy init` first.", - socket_path.display() - ); + + // Set a connect timeout to avoid hanging on a stale socket where + // the daemon process is dead but the socket file still exists. + let connect_timeout = std::time::Duration::from_secs(2); + let connect_result = std::os::unix::net::UnixStream::connect(&socket_path) + .and_then(|stream| { + stream.set_read_timeout(Some(connect_timeout))?; + stream.set_write_timeout(Some(connect_timeout))?; + Ok(stream) + }); + + match connect_result { + Ok(_stream) => { + // Connection succeeded -- daemon is alive + drop(_stream); + } + Err(_) => { + let _ = std::fs::remove_file(&override_path); + let _ = std::fs::remove_file(compose_dir.join(".devproxy-project")); + bail!( + "daemon is not running (could not connect to {}). Run `devproxy init` first.", + socket_path.display() + ); + } } // Run docker compose up diff --git a/src/config.rs b/src/config.rs index 3b118be..b4f63a4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -49,6 +49,10 @@ impl Config { Ok(Self::config_dir()?.join("tls-key.pem")) } + pub fn pid_path() -> Result { + Ok(Self::config_dir()?.join("daemon.pid")) + } + pub fn save(&self) -> Result<()> { let dir = Self::config_dir()?; std::fs::create_dir_all(&dir)?; diff --git a/src/ipc.rs b/src/ipc.rs index d5ab5a4..3b0c6a9 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -25,8 +25,32 @@ pub enum Response { Error { message: String }, } -/// Send a request to the daemon and get a response +/// Default timeout for IPC operations (connect + request + response). +const IPC_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(3); + +/// Send a request to the daemon and get a response, with the default timeout. pub async fn send_request(socket_path: &Path, request: &Request) -> Result { + send_request_with_timeout(socket_path, request, IPC_TIMEOUT).await +} + +/// Send a request to the daemon and get a response, with a custom timeout. +/// Returns an error if the daemon does not respond within the given duration. +pub async fn send_request_with_timeout( + socket_path: &Path, + request: &Request, + timeout: std::time::Duration, +) -> Result { + tokio::time::timeout(timeout, send_request_inner(socket_path, request)) + .await + .with_context(|| { + format!( + "timed out waiting for daemon response ({}s). The daemon may be dead. Try `devproxy init`.", + timeout.as_secs() + ) + })? +} + +async fn send_request_inner(socket_path: &Path, request: &Request) -> Result { let stream = UnixStream::connect(socket_path) .await .with_context(|| { @@ -92,4 +116,37 @@ mod tests { _ => panic!("expected Routes response"), } } + + /// Verify that send_request_with_timeout returns an error when the + /// daemon doesn't respond within the timeout (e.g., socket exists + /// but nothing reads from it). + #[tokio::test] + async fn send_request_timeout_on_unresponsive_socket() { + let dir = tempfile::tempdir().unwrap(); + let sock_path = dir.path().join("test.sock"); + + // Create a listener but never accept connections -- simulates + // a dead daemon with a stale socket. + let _listener = tokio::net::UnixListener::bind(&sock_path).unwrap(); + + let start = std::time::Instant::now(); + let result = send_request_with_timeout( + &sock_path, + &Request::Ping, + std::time::Duration::from_millis(500), + ) + .await; + let elapsed = start.elapsed(); + + assert!(result.is_err(), "should error on unresponsive socket"); + let err_msg = result.unwrap_err().to_string(); + assert!( + err_msg.contains("timed out"), + "error should mention timeout: {err_msg}" + ); + assert!( + elapsed < std::time::Duration::from_secs(2), + "should not wait too long (took {elapsed:?})" + ); + } } diff --git a/src/proxy/mod.rs b/src/proxy/mod.rs index 092afde..c1862cb 100644 --- a/src/proxy/mod.rs +++ b/src/proxy/mod.rs @@ -15,17 +15,22 @@ use router::Router; use tokio::io::{AsyncBufReadExt, AsyncReadExt, AsyncWriteExt, BufReader}; use tokio::net::{TcpListener, TcpStream, UnixListener}; -/// RAII guard that removes the IPC socket file on drop, ensuring cleanup -/// on both normal shutdown and error paths. -struct SocketCleanupGuard { - path: std::path::PathBuf, +/// RAII guard that removes the IPC socket and PID file on drop, ensuring +/// cleanup on both normal shutdown and error paths. +struct DaemonCleanupGuard { + socket_path: std::path::PathBuf, + pid_path: std::path::PathBuf, } -impl Drop for SocketCleanupGuard { +impl Drop for DaemonCleanupGuard { fn drop(&mut self) { - if self.path.exists() { - let _ = std::fs::remove_file(&self.path); - eprintln!("cleaned up socket: {}", self.path.display()); + if self.socket_path.exists() { + let _ = std::fs::remove_file(&self.socket_path); + eprintln!("cleaned up socket: {}", self.socket_path.display()); + } + if self.pid_path.exists() { + let _ = std::fs::remove_file(&self.pid_path); + eprintln!("cleaned up pid file: {}", self.pid_path.display()); } } } @@ -85,8 +90,17 @@ pub async fn run_daemon(port: u16) -> Result<()> { use std::os::unix::fs::PermissionsExt; std::fs::set_permissions(&socket_path, std::fs::Permissions::from_mode(0o777))?; } - // Ensure the socket file is removed when the daemon exits (normal or error). - let _socket_guard = SocketCleanupGuard { path: socket_path.clone() }; + + // Write PID file so init can find and kill stale daemons + let pid_path = Config::pid_path()?; + std::fs::write(&pid_path, std::process::id().to_string()) + .with_context(|| format!("could not write PID file at {}", pid_path.display()))?; + + // Ensure the socket and PID files are removed when the daemon exits (normal or error). + let _cleanup_guard = DaemonCleanupGuard { + socket_path: socket_path.clone(), + pid_path, + }; eprintln!("IPC listening on {}", socket_path.display()); // Set up HTTPS listener diff --git a/tests/e2e.rs b/tests/e2e.rs index c3c4b18..905cede 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -681,3 +681,244 @@ fn test_ipc_ping_pong() { assert!(stderr.contains("running"), "should report daemon running: {stderr}"); assert!(stderr.contains("active routes: 0"), "should report 0 routes: {stderr}"); } + +// ---- New daemon setup flow tests ------------------------------------------- + +/// Verify init output includes DNS setup instructions (dnsmasq, resolver) +#[test] +fn test_init_output_includes_dns_instructions() { + let config_dir = std::env::temp_dir().join(format!("devproxy-dns-test-{}", std::process::id())); + std::fs::create_dir_all(&config_dir).unwrap(); + + let output = Command::new(devproxy_bin()) + .args(["init", "--domain", TEST_DOMAIN, "--no-daemon"]) + .env("DEVPROXY_CONFIG_DIR", &config_dir) + .output() + .expect("failed to run devproxy init"); + + assert!(output.status.success(), "init should succeed"); + let stderr = String::from_utf8_lossy(&output.stderr); + + // Should mention dnsmasq + assert!( + stderr.contains("dnsmasq"), + "init output should mention dnsmasq for DNS setup: {stderr}" + ); + + // Should include the domain in DNS instructions + assert!( + stderr.contains(&format!(".{TEST_DOMAIN}")), + "init output should include domain in DNS instructions: {stderr}" + ); + + // On macOS, should mention /etc/resolver + #[cfg(target_os = "macos")] + assert!( + stderr.contains("/etc/resolver"), + "init output should mention /etc/resolver on macOS: {stderr}" + ); + + let _ = std::fs::remove_dir_all(&config_dir); +} + +/// Verify init output includes sudo note for port 443 +#[test] +fn test_init_output_includes_sudo_note() { + let config_dir = std::env::temp_dir().join(format!("devproxy-sudo-test-{}", std::process::id())); + std::fs::create_dir_all(&config_dir).unwrap(); + + // Use --no-daemon so we don't need root, but still verify the output mentions sudo + let output = Command::new(devproxy_bin()) + .args(["init", "--domain", TEST_DOMAIN, "--no-daemon"]) + .env("DEVPROXY_CONFIG_DIR", &config_dir) + .output() + .expect("failed to run devproxy init"); + + assert!(output.status.success(), "init should succeed"); + let stderr = String::from_utf8_lossy(&output.stderr); + + // Should mention sudo in the CA trust section + assert!( + stderr.contains("sudo"), + "init output should mention sudo: {stderr}" + ); + + let _ = std::fs::remove_dir_all(&config_dir); +} + +/// Verify init output includes CA certificate path +#[test] +fn test_init_output_includes_ca_trust_path() { + let config_dir = std::env::temp_dir().join(format!("devproxy-capath-test-{}", std::process::id())); + std::fs::create_dir_all(&config_dir).unwrap(); + + let output = Command::new(devproxy_bin()) + .args(["init", "--domain", TEST_DOMAIN, "--no-daemon"]) + .env("DEVPROXY_CONFIG_DIR", &config_dir) + .output() + .expect("failed to run devproxy init"); + + assert!(output.status.success(), "init should succeed"); + let stderr = String::from_utf8_lossy(&output.stderr); + + // Should include the CA cert path + let ca_cert_path = config_dir.join("ca-cert.pem"); + assert!( + stderr.contains(&ca_cert_path.display().to_string()), + "init output should include CA cert path '{}': {stderr}", + ca_cert_path.display() + ); + + let _ = std::fs::remove_dir_all(&config_dir); +} + +/// Verify `devproxy up` fails fast (within a timeout) when daemon is dead +/// rather than hanging indefinitely. Creates a stale socket file to simulate +/// a dead daemon. +#[test] +fn test_up_fails_fast_with_dead_daemon() { + let config_dir = std::env::temp_dir().join(format!("devproxy-deadup-{}", std::process::id())); + std::fs::create_dir_all(&config_dir).unwrap(); + std::fs::write( + config_dir.join("config.json"), + format!(r#"{{"domain":"{TEST_DOMAIN}"}}"#), + ) + .unwrap(); + + // Create a compose file with devproxy.port + let test_dir = std::env::temp_dir().join(format!("devproxy-deadup-project-{}", std::process::id())); + std::fs::create_dir_all(&test_dir).unwrap(); + std::fs::write( + test_dir.join("docker-compose.yml"), + "services:\n web:\n image: alpine\n labels:\n - \"devproxy.port=3000\"\n", + ) + .unwrap(); + + // Create a stale socket that no daemon is listening on. + // Bind a Unix socket then immediately drop it to leave the file. + let socket_path = config_dir.join("devproxy.sock"); + { + let listener = std::os::unix::net::UnixListener::bind(&socket_path).unwrap(); + drop(listener); + // Socket file exists but nothing is listening + } + + let start = std::time::Instant::now(); + let output = Command::new(devproxy_bin()) + .args(["up"]) + .current_dir(&test_dir) + .env("DEVPROXY_CONFIG_DIR", &config_dir) + .output() + .expect("failed to run up"); + let elapsed = start.elapsed(); + + assert!( + !output.status.success(), + "up should fail when daemon is dead" + ); + + // Should fail within 5 seconds (not hang) + assert!( + elapsed < Duration::from_secs(5), + "up should fail fast, not hang (took {elapsed:?})" + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("not running") || stderr.contains("could not connect"), + "should report daemon not running: {stderr}" + ); + + let _ = std::fs::remove_dir_all(&config_dir); + let _ = std::fs::remove_dir_all(&test_dir); +} + +/// Verify the daemon writes a PID file on startup +#[test] +#[ignore] +fn test_daemon_writes_pid_file() { + let config_dir = create_test_config_dir("pidfile"); + let daemon_port = find_free_port(); + let daemon = start_test_daemon(&config_dir, daemon_port); + + let pid_path = config_dir.join("daemon.pid"); + assert!( + pid_path.exists(), + "daemon should create a PID file at {}", + pid_path.display() + ); + + let pid_str = std::fs::read_to_string(&pid_path).unwrap(); + let pid: u32 = pid_str.trim().parse().expect("PID file should contain a valid number"); + assert!(pid > 0, "PID should be positive"); + + // Verify the PID matches the actual daemon process + assert_eq!( + pid, daemon.child.id(), + "PID file should match the daemon's actual PID" + ); +} + +/// Verify re-init kills the stale daemon process +#[test] +#[ignore] +fn test_reinit_kills_stale_daemon() { + let config_dir = create_test_config_dir("reinit"); + let daemon_port1 = find_free_port(); + let daemon1 = start_test_daemon(&config_dir, daemon_port1); + let pid1 = daemon1.child.id(); + + // Verify first daemon is alive + let pid_path = config_dir.join("daemon.pid"); + assert!(pid_path.exists(), "PID file should exist after first daemon start"); + + // Don't let daemon1's Drop clean up the config dir -- we need it for init + let mut daemon1 = daemon1; + daemon1.config_dir = PathBuf::new(); + // Don't kill the child -- init should do it + // But we need to forget the child so Drop doesn't kill it + // Actually, we DO let Drop kill it since we're testing init's behavior. + // Let's just remember the PID and kill it ourselves. + let _ = daemon1.child.kill(); + let _ = daemon1.child.wait(); + drop(daemon1); + + // Remove the socket so init doesn't find it responsive + let socket_path = config_dir.join("devproxy.sock"); + let _ = std::fs::remove_file(&socket_path); + + // Write the old PID file back (simulating a stale daemon) + std::fs::write(&pid_path, pid1.to_string()).unwrap(); + + // Run init with a new port + let daemon_port2 = find_free_port(); + let output = Command::new(devproxy_bin()) + .args([ + "init", + "--domain", TEST_DOMAIN, + "--port", &daemon_port2.to_string(), + ]) + .env("DEVPROXY_CONFIG_DIR", &config_dir) + .output() + .expect("failed to run devproxy init"); + + let stderr = String::from_utf8_lossy(&output.stderr); + eprintln!("reinit stderr: {stderr}"); + + // init should succeed and start a new daemon + assert!(output.status.success(), "init should succeed: {stderr}"); + assert!( + stderr.contains("daemon started"), + "init should report daemon started: {stderr}" + ); + + // New PID should be different from old one + let new_pid_str = std::fs::read_to_string(&pid_path).unwrap(); + let new_pid: u32 = new_pid_str.trim().parse().unwrap(); + assert_ne!(new_pid, pid1, "new daemon should have a different PID"); + + // Clean up the new daemon + unsafe { libc::kill(new_pid as i32, libc::SIGTERM); } + std::thread::sleep(Duration::from_millis(500)); + let _ = std::fs::remove_dir_all(&config_dir); +} From a4413319b8528332eae8fb6411f4b64036f5d7b8 Mon Sep 17 00:00:00 2001 From: Chris Fenton Date: Mon, 9 Mar 2026 00:34:52 -0700 Subject: [PATCH 2/7] fix: address review findings for daemon setup hardening - Validate PID is a devproxy process before killing to prevent PID reuse races (check via ps on macOS, /proc/pid/exe on Linux) - Handle EPERM from kill(pid, 0) when stale daemon is owned by another user -- warn and suggest sudo instead of silently skipping - wait_for_daemon now sends actual IPC ping/pong instead of just checking socket connectivity - up command sends real IPC ping with 2s timeout instead of relying on connect() + unused read/write timeouts - test_reinit_kills_stale_daemon leaves daemon1 alive so init's kill_stale_daemon exercises the SIGTERM/SIGKILL path - Check libc::kill return value in e2e test cleanup with warning - Remove redundant port 443 note at bottom of init output - Remove committed plan artifacts Co-Authored-By: Claude Opus 4.6 --- plans/fix-daemon-setup.md | 59 ----------------- src/commands/init.rs | 136 ++++++++++++++++++++++++++++---------- src/commands/up.rs | 42 +++++++----- tests/e2e.rs | 71 ++++++++++++++------ 4 files changed, 176 insertions(+), 132 deletions(-) delete mode 100644 plans/fix-daemon-setup.md diff --git a/plans/fix-daemon-setup.md b/plans/fix-daemon-setup.md deleted file mode 100644 index 474dac7..0000000 --- a/plans/fix-daemon-setup.md +++ /dev/null @@ -1,59 +0,0 @@ -# Plan: Fix daemon setup flow - -## Problem -The daemon setup flow (`devproxy init`) has multiple bugs discovered during end-to-end testing: -1. Port 443 needs sudo but init doesn't warn about it -2. Socket permissions issue when daemon runs as root (partially fixed) -3. Daemon dies silently after init reports success -4. Init output missing DNS setup commands, CA trust path, sudo requirement -5. `devproxy up` hangs when daemon is dead instead of failing fast -6. DNS setup not documented in init flow -7. CA trust needs sudo but fails silently -8. Stale daemon processes not cleaned up on re-init - -## Changes - -### 1. `src/commands/init.rs` — Major rework -- Kill stale daemon processes before spawning a new one (find by socket, send ping, kill PID) -- Remove stale socket file on re-init -- Wait briefly after daemon spawn and verify it's actually alive (connect to socket + ping) -- Report daemon startup failure clearly instead of "ok: daemon started" then silence -- Print complete DNS setup instructions (dnsmasq install + config + resolver setup for macOS) -- Print CA trust command with correct cert path -- Note that sudo is needed for port 443 -- Pass `--port` info to next-steps output - -### 2. `src/commands/up.rs` — Add IPC connection timeout -- Add a timeout (2s) to the daemon health check so `up` fails fast instead of hanging -- Improve error message to suggest re-running `devproxy init` - -### 3. `src/ipc.rs` — Add connection timeout -- Add a `send_request_with_timeout` function (or add timeout to existing `send_request`) -- Default timeout of 3 seconds for IPC operations - -### 4. `src/proxy/mod.rs` — Write PID file -- Write daemon PID to `/daemon.pid` on startup -- Clean up PID file in `SocketCleanupGuard` drop - -### 5. `src/config.rs` — Add `pid_path()` helper -- Add `Config::pid_path()` returning `/daemon.pid` - -### 6. E2E tests in `tests/e2e.rs` -- **test_init_output_includes_dns_instructions** — verify init stderr contains dnsmasq/resolver instructions -- **test_init_output_includes_sudo_note** — verify init mentions sudo for port 443 -- **test_up_fails_fast_with_dead_daemon** — kill daemon, verify `up` fails within timeout (not hang) -- **test_reinit_kills_stale_daemon** — init twice, verify first daemon is killed -- **test_daemon_writes_pid_file** — verify PID file created after daemon start -- **test_init_output_includes_ca_trust_path** — verify CA cert path is printed - -### 7. Unit tests -- **ipc::tests::send_request_timeout** — verify IPC timeout works against non-responsive socket - -## File change list -- `src/commands/init.rs` — major rework -- `src/commands/up.rs` — add timeout to daemon check -- `src/ipc.rs` — add timeout support -- `src/proxy/mod.rs` — write PID file -- `src/config.rs` — add pid_path() -- `tests/e2e.rs` — add 6 new tests -- `docs/spec.md` — update init description with DNS/sudo info diff --git a/src/commands/init.rs b/src/commands/init.rs index 2e6909a..c8cdacf 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -31,8 +31,46 @@ fn validate_domain(domain: &str) -> Result<()> { Ok(()) } -/// Kill a stale daemon process. Reads PID from the PID file, sends SIGTERM, -/// then SIGKILL if needed. Also removes stale socket and PID files. +/// Check whether the process at `pid` is a devproxy process by inspecting +/// its command line. Returns false if we cannot determine (e.g., process +/// belongs to another user) -- we err on the side of not killing. +fn is_devproxy_process(pid: i32) -> bool { + #[cfg(target_os = "macos")] + { + // On macOS, use `ps -p -o comm=` to get the process name. + let output = std::process::Command::new("ps") + .args(["-p", &pid.to_string(), "-o", "comm="]) + .output(); + match output { + Ok(out) if out.status.success() => { + let comm = String::from_utf8_lossy(&out.stdout); + comm.trim().ends_with("devproxy") + } + _ => false, + } + } + #[cfg(target_os = "linux")] + { + // On Linux, read /proc//exe symlink. + let exe = std::fs::read_link(format!("/proc/{pid}/exe")); + match exe { + Ok(path) => path + .file_name() + .map(|n| n == "devproxy") + .unwrap_or(false), + Err(_) => false, + } + } + #[cfg(not(any(target_os = "macos", target_os = "linux")))] + { + let _ = pid; + false + } +} + +/// Kill a stale daemon process. Reads PID from the PID file, validates it +/// is actually a devproxy process (to avoid PID reuse races), then sends +/// SIGTERM/SIGKILL. Also removes stale socket and PID files. fn kill_stale_daemon() -> Result<()> { let pid_path = Config::pid_path()?; let socket_path = Config::socket_path()?; @@ -40,23 +78,48 @@ fn kill_stale_daemon() -> Result<()> { if pid_path.exists() { let pid_str = std::fs::read_to_string(&pid_path).unwrap_or_default(); if let Ok(pid) = pid_str.trim().parse::() { - // Check if process is alive - let alive = unsafe { libc::kill(pid, 0) } == 0; - if alive { + // Check if process is alive. kill(pid, 0) returns 0 if we can + // signal it, or -1 with EPERM if it exists but we lack permission. + let ret = unsafe { libc::kill(pid, 0) }; + let alive = ret == 0; + let alive_but_no_perms = ret == -1 + && std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM); + + if alive_but_no_perms { eprintln!( - "{} killing stale daemon (pid: {pid})...", - "info:".cyan() + "{} stale daemon (pid: {pid}) is running but owned by another user.", + "warn:".yellow() + ); + eprintln!( + " try: sudo kill {pid}" ); - // Send SIGTERM first - unsafe { libc::kill(pid, libc::SIGTERM); } - // Wait briefly for graceful shutdown - std::thread::sleep(Duration::from_millis(500)); - // Check if still alive, send SIGKILL - if unsafe { libc::kill(pid, 0) } == 0 { - unsafe { libc::kill(pid, libc::SIGKILL); } - std::thread::sleep(Duration::from_millis(200)); + // Don't remove PID/socket files -- the daemon is still running + return Ok(()); + } + + if alive { + // Verify this is actually a devproxy process, not a recycled PID + if !is_devproxy_process(pid) { + eprintln!( + "{} PID {pid} is no longer a devproxy process (PID was recycled), cleaning up stale files", + "info:".cyan() + ); + // Fall through to file cleanup + } else { + eprintln!( + "{} killing stale daemon (pid: {pid})...", + "info:".cyan() + ); + unsafe { libc::kill(pid, libc::SIGTERM); } + // Wait briefly for graceful shutdown + std::thread::sleep(Duration::from_millis(500)); + // Check if still alive, send SIGKILL + if unsafe { libc::kill(pid, 0) } == 0 { + unsafe { libc::kill(pid, libc::SIGKILL); } + std::thread::sleep(Duration::from_millis(200)); + } + eprintln!("{} stale daemon killed", "ok:".green()); } - eprintln!("{} stale daemon killed", "ok:".green()); } } let _ = std::fs::remove_file(&pid_path); @@ -70,20 +133,34 @@ fn kill_stale_daemon() -> Result<()> { Ok(()) } -/// Wait for the daemon to become responsive by polling the IPC socket. -/// Returns Ok(()) if the daemon responds to a ping within the timeout, -/// or an error if it doesn't. +/// Wait for the daemon to become responsive by polling the IPC socket +/// with an actual JSON ping/pong exchange. Returns Ok(()) if the daemon +/// responds to a Ping within the timeout, or an error if it doesn't. fn wait_for_daemon(timeout: Duration) -> Result<()> { let socket_path = Config::socket_path()?; let start = std::time::Instant::now(); let poll_interval = Duration::from_millis(100); while start.elapsed() < timeout { - if socket_path.exists() - && std::os::unix::net::UnixStream::connect(&socket_path).is_ok() - { - // Socket is connectable -- daemon is likely alive - return Ok(()); + if socket_path.exists() { + // Attempt a real IPC ping to verify the daemon is fully operational, + // not just accepting connections. + if let Ok(mut stream) = std::os::unix::net::UnixStream::connect(&socket_path) { + use std::io::{BufRead, Write}; + stream.set_read_timeout(Some(Duration::from_secs(1))).ok(); + stream.set_write_timeout(Some(Duration::from_secs(1))).ok(); + let ping = b"{\"cmd\":\"ping\"}\n"; + if stream.write_all(ping).is_ok() { + stream.shutdown(std::net::Shutdown::Write).ok(); + let mut reader = std::io::BufReader::new(&stream); + let mut response = String::new(); + if reader.read_line(&mut response).is_ok() + && response.contains("pong") + { + return Ok(()); + } + } + } } std::thread::sleep(poll_interval); } @@ -290,16 +367,5 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { eprintln!(); eprintln!(" {} Run: devproxy up", "4.".bold()); - if port == 443 && !no_daemon { - eprintln!(); - eprintln!( - " {} The daemon is listening on port 443 which requires sudo.", - "note:".yellow() - ); - eprintln!( - " If the daemon failed to bind, re-run: sudo devproxy init --domain {domain}" - ); - } - Ok(()) } diff --git a/src/commands/up.rs b/src/commands/up.rs index 61b1025..ba7c933 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -56,26 +56,34 @@ pub fn run() -> Result<()> { bail!("daemon is not running (no socket at {}). Run `devproxy init` first.", socket_path.display()); } - // Set a connect timeout to avoid hanging on a stale socket where - // the daemon process is dead but the socket file still exists. - let connect_timeout = std::time::Duration::from_secs(2); - let connect_result = std::os::unix::net::UnixStream::connect(&socket_path) - .and_then(|stream| { - stream.set_read_timeout(Some(connect_timeout))?; - stream.set_write_timeout(Some(connect_timeout))?; - Ok(stream) - }); - - match connect_result { - Ok(_stream) => { - // Connection succeeded -- daemon is alive - drop(_stream); - } - Err(_) => { + // Send an actual IPC ping with a timeout to verify the daemon is + // responsive, not just that a stale socket file exists. + { + use std::io::{BufRead, Write}; + let ping_timeout = std::time::Duration::from_secs(2); + let daemon_alive = (|| -> bool { + let stream = match std::os::unix::net::UnixStream::connect(&socket_path) { + Ok(s) => s, + Err(_) => return false, + }; + stream.set_read_timeout(Some(ping_timeout)).ok(); + stream.set_write_timeout(Some(ping_timeout)).ok(); + if stream.try_clone().ok().and_then(|mut w| { + w.write_all(b"{\"cmd\":\"ping\"}\n").ok()?; + w.shutdown(std::net::Shutdown::Write).ok() + }).is_none() { + return false; + } + let mut reader = std::io::BufReader::new(&stream); + let mut response = String::new(); + reader.read_line(&mut response).is_ok() && response.contains("pong") + })(); + + if !daemon_alive { let _ = std::fs::remove_file(&override_path); let _ = std::fs::remove_file(compose_dir.join(".devproxy-project")); bail!( - "daemon is not running (could not connect to {}). Run `devproxy init` first.", + "daemon is not running (no response from {}). Run `devproxy init` first.", socket_path.display() ); } diff --git a/tests/e2e.rs b/tests/e2e.rs index 905cede..8eee399 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -859,38 +859,49 @@ fn test_daemon_writes_pid_file() { ); } -/// Verify re-init kills the stale daemon process +/// Verify re-init kills the stale daemon process. +/// Leaves daemon1 running and lets init's kill_stale_daemon handle the kill. #[test] #[ignore] fn test_reinit_kills_stale_daemon() { let config_dir = create_test_config_dir("reinit"); let daemon_port1 = find_free_port(); - let daemon1 = start_test_daemon(&config_dir, daemon_port1); + let mut daemon1 = start_test_daemon(&config_dir, daemon_port1); let pid1 = daemon1.child.id(); - // Verify first daemon is alive + // Verify first daemon is alive and has a PID file let pid_path = config_dir.join("daemon.pid"); assert!(pid_path.exists(), "PID file should exist after first daemon start"); - - // Don't let daemon1's Drop clean up the config dir -- we need it for init - let mut daemon1 = daemon1; + let saved_pid: u32 = std::fs::read_to_string(&pid_path) + .unwrap() + .trim() + .parse() + .unwrap(); + assert_eq!(saved_pid, pid1, "PID file should match daemon PID"); + + // Detach daemon1 from the guard so Drop does NOT kill it -- + // we want init's kill_stale_daemon to handle that. daemon1.config_dir = PathBuf::new(); - // Don't kill the child -- init should do it - // But we need to forget the child so Drop doesn't kill it - // Actually, we DO let Drop kill it since we're testing init's behavior. - // Let's just remember the PID and kill it ourselves. - let _ = daemon1.child.kill(); - let _ = daemon1.child.wait(); + // We must NOT call child.kill() here. Instead, leak the Child so + // the daemon stays alive for init to find and kill. + let child = std::mem::replace( + &mut daemon1.child, + // Replace with a dummy child that we can safely drop. + // Spawn a trivial process that exits immediately. + Command::new("true").spawn().unwrap(), + ); drop(daemon1); + // Forget the original child so its Drop doesn't kill the daemon. + std::mem::forget(child); - // Remove the socket so init doesn't find it responsive - let socket_path = config_dir.join("devproxy.sock"); - let _ = std::fs::remove_file(&socket_path); - - // Write the old PID file back (simulating a stale daemon) - std::fs::write(&pid_path, pid1.to_string()).unwrap(); + // Verify the old daemon is still alive + assert_eq!( + unsafe { libc::kill(pid1 as i32, 0) }, + 0, + "daemon1 should still be alive before re-init" + ); - // Run init with a new port + // Run init with a new port -- this should kill the old daemon let daemon_port2 = find_free_port(); let output = Command::new(devproxy_bin()) .args([ @@ -905,20 +916,38 @@ fn test_reinit_kills_stale_daemon() { let stderr = String::from_utf8_lossy(&output.stderr); eprintln!("reinit stderr: {stderr}"); - // init should succeed and start a new daemon + // init should have killed the stale daemon and started a new one assert!(output.status.success(), "init should succeed: {stderr}"); + assert!( + stderr.contains("killing stale daemon"), + "init should report killing stale daemon: {stderr}" + ); assert!( stderr.contains("daemon started"), "init should report daemon started: {stderr}" ); + // Old daemon should be dead + std::thread::sleep(Duration::from_millis(200)); + assert_ne!( + unsafe { libc::kill(pid1 as i32, 0) }, + 0, + "old daemon (pid {pid1}) should be dead after re-init" + ); + // New PID should be different from old one let new_pid_str = std::fs::read_to_string(&pid_path).unwrap(); let new_pid: u32 = new_pid_str.trim().parse().unwrap(); assert_ne!(new_pid, pid1, "new daemon should have a different PID"); // Clean up the new daemon - unsafe { libc::kill(new_pid as i32, libc::SIGTERM); } + let kill_ret = unsafe { libc::kill(new_pid as i32, libc::SIGTERM) }; + if kill_ret != 0 { + eprintln!( + "warning: failed to kill new daemon (pid {new_pid}): {}", + std::io::Error::last_os_error() + ); + } std::thread::sleep(Duration::from_millis(500)); let _ = std::fs::remove_dir_all(&config_dir); } From 1ceb91c7ca1531282aafee5b2d5b5617292da2cf Mon Sep 17 00:00:00 2001 From: Chris Fenton Date: Mon, 9 Mar 2026 00:39:57 -0700 Subject: [PATCH 3/7] fix: address review round 2 findings for daemon setup - Document TOCTOU race between is_devproxy_process and SIGTERM as residual risk (PID reuse + same binary name is vanishingly unlikely) - Check libc::kill return values for SIGTERM/SIGKILL; bail with actionable "sudo kill" guidance on EPERM - Bail instead of returning Ok on alive_but_no_perms so the caller gets a clear error about why the new daemon will fail - Use exact basename match (== "devproxy") instead of ends_with on macOS to avoid false positives like "my-devproxy" - Replace std::mem::forget(child) in test with proper cleanup via original_child.wait() after init kills the process - Simplify up.rs IPC ping to match init.rs pattern (no try_clone) - Add cross-reference comments to raw JSON ping format pointing to canonical ipc.rs Request/Response protocol - Only print CA trust instructions in "Next steps" if trust actually failed (skip if already trusted successfully) - Verify socket file exists after UnixListener drop in stale daemon test Co-Authored-By: Claude Opus 4.6 --- src/commands/init.rs | 102 +++++++++++++++++++++++++++++++------------ src/commands/up.rs | 12 ++--- tests/e2e.rs | 29 +++++++----- 3 files changed, 97 insertions(+), 46 deletions(-) diff --git a/src/commands/init.rs b/src/commands/init.rs index c8cdacf..77d4625 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -44,7 +44,12 @@ fn is_devproxy_process(pid: i32) -> bool { match output { Ok(out) if out.status.success() => { let comm = String::from_utf8_lossy(&out.stdout); - comm.trim().ends_with("devproxy") + // Exact match on basename to avoid false positives (e.g., "my-devproxy") + comm.trim() + .rsplit('/') + .next() + .map(|name| name == "devproxy") + .unwrap_or(false) } _ => false, } @@ -71,6 +76,15 @@ fn is_devproxy_process(pid: i32) -> bool { /// Kill a stale daemon process. Reads PID from the PID file, validates it /// is actually a devproxy process (to avoid PID reuse races), then sends /// SIGTERM/SIGKILL. Also removes stale socket and PID files. +/// +/// # Residual TOCTOU race +/// +/// There is a small window between `is_devproxy_process(pid)` returning true +/// and the SIGTERM being sent where the process could die and the OS could +/// recycle the PID. In practice this is extremely unlikely: PIDs cycle through +/// a large range and the window is microseconds. The process-name check makes +/// accidental kills vanishingly improbable -- the recycled PID would also need +/// to be named "devproxy". fn kill_stale_daemon() -> Result<()> { let pid_path = Config::pid_path()?; let socket_path = Config::socket_path()?; @@ -86,15 +100,10 @@ fn kill_stale_daemon() -> Result<()> { && std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM); if alive_but_no_perms { - eprintln!( - "{} stale daemon (pid: {pid}) is running but owned by another user.", - "warn:".yellow() - ); - eprintln!( - " try: sudo kill {pid}" + bail!( + "stale daemon (pid: {pid}) is running but owned by another user. \ + Kill it first with: sudo kill {pid}" ); - // Don't remove PID/socket files -- the daemon is still running - return Ok(()); } if alive { @@ -110,13 +119,34 @@ fn kill_stale_daemon() -> Result<()> { "{} killing stale daemon (pid: {pid})...", "info:".cyan() ); - unsafe { libc::kill(pid, libc::SIGTERM); } - // Wait briefly for graceful shutdown - std::thread::sleep(Duration::from_millis(500)); - // Check if still alive, send SIGKILL - if unsafe { libc::kill(pid, 0) } == 0 { - unsafe { libc::kill(pid, libc::SIGKILL); } - std::thread::sleep(Duration::from_millis(200)); + if unsafe { libc::kill(pid, libc::SIGTERM) } != 0 { + let err = std::io::Error::last_os_error(); + eprintln!( + "{} SIGTERM failed for pid {pid}: {err}", + "warn:".yellow() + ); + // EPERM means we can't signal it -- bail with guidance + if err.raw_os_error() == Some(libc::EPERM) { + bail!( + "cannot kill stale daemon (pid: {pid}): permission denied. \ + Try: sudo kill {pid}" + ); + } + } else { + // Wait briefly for graceful shutdown + std::thread::sleep(Duration::from_millis(500)); + // Check if still alive, send SIGKILL + if unsafe { libc::kill(pid, 0) } == 0 + && unsafe { libc::kill(pid, libc::SIGKILL) } != 0 + { + let err = std::io::Error::last_os_error(); + eprintln!( + "{} SIGKILL failed for pid {pid}: {err}", + "warn:".yellow() + ); + } else { + std::thread::sleep(Duration::from_millis(200)); + } } eprintln!("{} stale daemon killed", "ok:".green()); } @@ -144,7 +174,8 @@ fn wait_for_daemon(timeout: Duration) -> Result<()> { while start.elapsed() < timeout { if socket_path.exists() { // Attempt a real IPC ping to verify the daemon is fully operational, - // not just accepting connections. + // not just accepting connections. The JSON format must match the + // canonical Request/Response protocol defined in ipc.rs. if let Ok(mut stream) = std::os::unix::net::UnixStream::connect(&socket_path) { use std::io::{BufRead, Write}; stream.set_read_timeout(Some(Duration::from_secs(1))).ok(); @@ -183,6 +214,10 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { let ca_cert_path = Config::ca_cert_path()?; let ca_key_path = Config::ca_key_path()?; + // Track whether CA trust needs manual action, so we can conditionally + // print trust instructions in "Next steps". + let mut ca_trust_needed = false; + if ca_cert_path.exists() && ca_key_path.exists() { eprintln!("{} CA certificate already exists", "ok:".green()); } else { @@ -197,6 +232,7 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { match cert::trust_ca_in_system(&ca_cert_path) { Ok(()) => eprintln!("{} CA trusted in system keychain", "ok:".green()), Err(e) => { + ca_trust_needed = true; eprintln!( "{} could not trust CA automatically: {e}", "warn:".yellow() @@ -352,20 +388,30 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { } eprintln!(); - // CA trust reminder - eprintln!(" {} Trust the CA certificate (if not done above)", "2.".bold()); - eprintln!(" CA cert: {}", ca_cert_path.display().to_string().cyan()); - #[cfg(target_os = "macos")] - eprintln!( - " sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain {}", - ca_cert_path.display() - ); - eprintln!(); + // CA trust reminder -- only shown if trust was not already successful + let mut step = 2; + if ca_trust_needed { + eprintln!(" {} Trust the CA certificate", format!("{step}.").bold()); + eprintln!(" CA cert: {}", ca_cert_path.display().to_string().cyan()); + #[cfg(target_os = "macos")] + eprintln!( + " sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain {}", + ca_cert_path.display() + ); + #[cfg(target_os = "linux")] + eprintln!( + " sudo cp {} /usr/local/share/ca-certificates/devproxy-ca.crt && sudo update-ca-certificates", + ca_cert_path.display() + ); + eprintln!(); + step += 1; + } // Project setup - eprintln!(" {} Add a devproxy.port label to your docker-compose.yml", "3.".bold()); + eprintln!(" {} Add a devproxy.port label to your docker-compose.yml", format!("{step}.").bold()); eprintln!(); - eprintln!(" {} Run: devproxy up", "4.".bold()); + step += 1; + eprintln!(" {} Run: devproxy up", format!("{step}.").bold()); Ok(()) } diff --git a/src/commands/up.rs b/src/commands/up.rs index ba7c933..99a0f76 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -57,23 +57,23 @@ pub fn run() -> Result<()> { } // Send an actual IPC ping with a timeout to verify the daemon is - // responsive, not just that a stale socket file exists. + // responsive, not just that a stale socket file exists. The JSON format + // must match the canonical Request/Response protocol defined in ipc.rs. { use std::io::{BufRead, Write}; let ping_timeout = std::time::Duration::from_secs(2); let daemon_alive = (|| -> bool { - let stream = match std::os::unix::net::UnixStream::connect(&socket_path) { + let mut stream = match std::os::unix::net::UnixStream::connect(&socket_path) { Ok(s) => s, Err(_) => return false, }; stream.set_read_timeout(Some(ping_timeout)).ok(); stream.set_write_timeout(Some(ping_timeout)).ok(); - if stream.try_clone().ok().and_then(|mut w| { - w.write_all(b"{\"cmd\":\"ping\"}\n").ok()?; - w.shutdown(std::net::Shutdown::Write).ok() - }).is_none() { + let ping = b"{\"cmd\":\"ping\"}\n"; + if stream.write_all(ping).is_err() { return false; } + stream.shutdown(std::net::Shutdown::Write).ok(); let mut reader = std::io::BufReader::new(&stream); let mut response = String::new(); reader.read_line(&mut response).is_ok() && response.contains("pong") diff --git a/tests/e2e.rs b/tests/e2e.rs index 8eee399..1ab5e9e 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -794,14 +794,18 @@ fn test_up_fails_fast_with_dead_daemon() { ) .unwrap(); - // Create a stale socket that no daemon is listening on. - // Bind a Unix socket then immediately drop it to leave the file. + // Create a stale socket file that no daemon is listening on. + // Bind a Unix socket then immediately drop it -- the file remains on disk. let socket_path = config_dir.join("devproxy.sock"); { let listener = std::os::unix::net::UnixListener::bind(&socket_path).unwrap(); drop(listener); - // Socket file exists but nothing is listening } + // Verify the socket file persists after the listener is dropped + assert!( + socket_path.exists(), + "socket file should remain after UnixListener drop" + ); let start = std::time::Instant::now(); let output = Command::new(devproxy_bin()) @@ -825,7 +829,7 @@ fn test_up_fails_fast_with_dead_daemon() { let stderr = String::from_utf8_lossy(&output.stderr); assert!( - stderr.contains("not running") || stderr.contains("could not connect"), + stderr.contains("not running") || stderr.contains("no response"), "should report daemon not running: {stderr}" ); @@ -861,6 +865,8 @@ fn test_daemon_writes_pid_file() { /// Verify re-init kills the stale daemon process. /// Leaves daemon1 running and lets init's kill_stale_daemon handle the kill. +/// We hold onto the Child handle in a local variable for cleanup rather than +/// leaking it with std::mem::forget. #[test] #[ignore] fn test_reinit_kills_stale_daemon() { @@ -880,19 +886,15 @@ fn test_reinit_kills_stale_daemon() { assert_eq!(saved_pid, pid1, "PID file should match daemon PID"); // Detach daemon1 from the guard so Drop does NOT kill it -- - // we want init's kill_stale_daemon to handle that. + // we want init's kill_stale_daemon to handle that. Replace the child + // with a dummy so the guard's Drop is harmless, and keep the real + // child handle for cleanup at end of test. daemon1.config_dir = PathBuf::new(); - // We must NOT call child.kill() here. Instead, leak the Child so - // the daemon stays alive for init to find and kill. - let child = std::mem::replace( + let mut original_child = std::mem::replace( &mut daemon1.child, - // Replace with a dummy child that we can safely drop. - // Spawn a trivial process that exits immediately. Command::new("true").spawn().unwrap(), ); drop(daemon1); - // Forget the original child so its Drop doesn't kill the daemon. - std::mem::forget(child); // Verify the old daemon is still alive assert_eq!( @@ -935,6 +937,9 @@ fn test_reinit_kills_stale_daemon() { "old daemon (pid {pid1}) should be dead after re-init" ); + // Reap the original child to avoid zombie (init already killed it) + let _ = original_child.wait(); + // New PID should be different from old one let new_pid_str = std::fs::read_to_string(&pid_path).unwrap(); let new_pid: u32 = new_pid_str.trim().parse().unwrap(); From d3c01cc01848b86322719ff8e317f6ff97a640dd Mon Sep 17 00:00:00 2001 From: Chris Fenton Date: Mon, 9 Mar 2026 00:45:18 -0700 Subject: [PATCH 4/7] fix: address review round 3 findings for daemon setup - Only print "stale daemon killed" when SIGTERM or SIGKILL actually succeeded; print warning otherwise - Extract shared ipc::ping_sync() function to eliminate duplicated IPC ping logic between init.rs and up.rs, preventing protocol drift - Validate PID > 0 before calling libc::kill to prevent signaling the entire process group on corrupted PID file - Restructure alive/EPERM/dead check as if/else-if/else for clarity - DaemonGuard Drop reaps the dummy "true" child via kill+wait - Document UnixListener drop behavior (does not remove socket file) - Harden test_init_output_includes_ca_trust_path comment to explain why it works regardless of trust success/failure - Replace unreachable unwrap_or with expect on rsplit('.').next() - Add ping_sync unit test for nonexistent socket path Co-Authored-By: Claude Opus 4.6 --- src/commands/init.rs | 95 ++++++++++++++++++++++++-------------------- src/commands/up.rs | 40 +++++-------------- src/ipc.rs | 38 ++++++++++++++++++ tests/e2e.rs | 14 +++++-- 4 files changed, 110 insertions(+), 77 deletions(-) diff --git a/src/commands/init.rs b/src/commands/init.rs index 77d4625..3416b86 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -92,21 +92,27 @@ fn kill_stale_daemon() -> Result<()> { if pid_path.exists() { let pid_str = std::fs::read_to_string(&pid_path).unwrap_or_default(); if let Ok(pid) = pid_str.trim().parse::() { - // Check if process is alive. kill(pid, 0) returns 0 if we can - // signal it, or -1 with EPERM if it exists but we lack permission. - let ret = unsafe { libc::kill(pid, 0) }; - let alive = ret == 0; - let alive_but_no_perms = ret == -1 - && std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM); - - if alive_but_no_perms { - bail!( - "stale daemon (pid: {pid}) is running but owned by another user. \ - Kill it first with: sudo kill {pid}" + // PID must be positive. PID 0 would signal the entire process group, + // and negative PIDs signal process groups by absolute value. + if pid <= 0 { + eprintln!( + "{} invalid PID {pid} in PID file, cleaning up stale files", + "warn:".yellow() ); + let _ = std::fs::remove_file(&pid_path); + if socket_path.exists() { + let _ = std::fs::remove_file(&socket_path); + } + return Ok(()); } - if alive { + // Check if process is alive. kill(pid, 0) returns: + // 0 — process exists and we can signal it + // -1/EPERM — process exists but owned by another user + // -1/ESRCH — process does not exist (PID is stale) + let ret = unsafe { libc::kill(pid, 0) }; + if ret == 0 { + // Process is alive and we can signal it // Verify this is actually a devproxy process, not a recycled PID if !is_devproxy_process(pid) { eprintln!( @@ -119,6 +125,7 @@ fn kill_stale_daemon() -> Result<()> { "{} killing stale daemon (pid: {pid})...", "info:".cyan() ); + let mut killed = false; if unsafe { libc::kill(pid, libc::SIGTERM) } != 0 { let err = std::io::Error::last_os_error(); eprintln!( @@ -133,24 +140,40 @@ fn kill_stale_daemon() -> Result<()> { ); } } else { - // Wait briefly for graceful shutdown + // SIGTERM succeeded -- wait briefly for graceful shutdown std::thread::sleep(Duration::from_millis(500)); - // Check if still alive, send SIGKILL - if unsafe { libc::kill(pid, 0) } == 0 - && unsafe { libc::kill(pid, libc::SIGKILL) } != 0 - { + if unsafe { libc::kill(pid, 0) } != 0 { + // Process is gone after SIGTERM + killed = true; + } else if unsafe { libc::kill(pid, libc::SIGKILL) } == 0 { + // SIGKILL succeeded + std::thread::sleep(Duration::from_millis(200)); + killed = true; + } else { let err = std::io::Error::last_os_error(); eprintln!( "{} SIGKILL failed for pid {pid}: {err}", "warn:".yellow() ); - } else { - std::thread::sleep(Duration::from_millis(200)); } } - eprintln!("{} stale daemon killed", "ok:".green()); + if killed { + eprintln!("{} stale daemon killed", "ok:".green()); + } else { + eprintln!( + "{} could not confirm daemon (pid: {pid}) was killed", + "warn:".yellow() + ); + } } + } else if std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) { + // Process exists but owned by another user (e.g., root) + bail!( + "stale daemon (pid: {pid}) is running but owned by another user. \ + Kill it first with: sudo kill {pid}" + ); } + // else: process does not exist (ESRCH) -- fall through to file cleanup } let _ = std::fs::remove_file(&pid_path); } @@ -164,7 +187,7 @@ fn kill_stale_daemon() -> Result<()> { } /// Wait for the daemon to become responsive by polling the IPC socket -/// with an actual JSON ping/pong exchange. Returns Ok(()) if the daemon +/// with an actual ping/pong exchange. Returns Ok(()) if the daemon /// responds to a Ping within the timeout, or an error if it doesn't. fn wait_for_daemon(timeout: Duration) -> Result<()> { let socket_path = Config::socket_path()?; @@ -172,26 +195,10 @@ fn wait_for_daemon(timeout: Duration) -> Result<()> { let poll_interval = Duration::from_millis(100); while start.elapsed() < timeout { - if socket_path.exists() { - // Attempt a real IPC ping to verify the daemon is fully operational, - // not just accepting connections. The JSON format must match the - // canonical Request/Response protocol defined in ipc.rs. - if let Ok(mut stream) = std::os::unix::net::UnixStream::connect(&socket_path) { - use std::io::{BufRead, Write}; - stream.set_read_timeout(Some(Duration::from_secs(1))).ok(); - stream.set_write_timeout(Some(Duration::from_secs(1))).ok(); - let ping = b"{\"cmd\":\"ping\"}\n"; - if stream.write_all(ping).is_ok() { - stream.shutdown(std::net::Shutdown::Write).ok(); - let mut reader = std::io::BufReader::new(&stream); - let mut response = String::new(); - if reader.read_line(&mut response).is_ok() - && response.contains("pong") - { - return Ok(()); - } - } - } + if socket_path.exists() + && crate::ipc::ping_sync(&socket_path, Duration::from_secs(1)) + { + return Ok(()); } std::thread::sleep(poll_interval); } @@ -376,8 +383,10 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { eprintln!(" echo 'address=/.{domain}/127.0.0.1' >> $(brew --prefix)/etc/dnsmasq.conf"); eprintln!(" sudo brew services restart dnsmasq"); eprintln!(); - // Extract the TLD for the resolver - let tld = domain.rsplit('.').next().unwrap_or(domain); + // Extract the TLD for the resolver. rsplit('.').next() always + // returns Some (the last segment, or the whole string if no dot), + // but we already validated the domain has at least two labels above. + let tld = domain.rsplit('.').next().expect("validated domain has a dot"); eprintln!(" Create resolver for .{tld} domains:"); eprintln!(" sudo mkdir -p /etc/resolver"); eprintln!(" echo 'nameserver 127.0.0.1' | sudo tee /etc/resolver/{tld}"); diff --git a/src/commands/up.rs b/src/commands/up.rs index 99a0f76..5586555 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -56,37 +56,15 @@ pub fn run() -> Result<()> { bail!("daemon is not running (no socket at {}). Run `devproxy init` first.", socket_path.display()); } - // Send an actual IPC ping with a timeout to verify the daemon is - // responsive, not just that a stale socket file exists. The JSON format - // must match the canonical Request/Response protocol defined in ipc.rs. - { - use std::io::{BufRead, Write}; - let ping_timeout = std::time::Duration::from_secs(2); - let daemon_alive = (|| -> bool { - let mut stream = match std::os::unix::net::UnixStream::connect(&socket_path) { - Ok(s) => s, - Err(_) => return false, - }; - stream.set_read_timeout(Some(ping_timeout)).ok(); - stream.set_write_timeout(Some(ping_timeout)).ok(); - let ping = b"{\"cmd\":\"ping\"}\n"; - if stream.write_all(ping).is_err() { - return false; - } - stream.shutdown(std::net::Shutdown::Write).ok(); - let mut reader = std::io::BufReader::new(&stream); - let mut response = String::new(); - reader.read_line(&mut response).is_ok() && response.contains("pong") - })(); - - if !daemon_alive { - let _ = std::fs::remove_file(&override_path); - let _ = std::fs::remove_file(compose_dir.join(".devproxy-project")); - bail!( - "daemon is not running (no response from {}). Run `devproxy init` first.", - socket_path.display() - ); - } + // Send an actual IPC ping with a 2s timeout to verify the daemon is + // responsive, not just that a stale socket file exists. + if !crate::ipc::ping_sync(&socket_path, std::time::Duration::from_secs(2)) { + let _ = std::fs::remove_file(&override_path); + let _ = std::fs::remove_file(compose_dir.join(".devproxy-project")); + bail!( + "daemon is not running (no response from {}). Run `devproxy init` first.", + socket_path.display() + ); } // Run docker compose up diff --git a/src/ipc.rs b/src/ipc.rs index 3b0c6a9..68646d3 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -78,6 +78,37 @@ async fn send_request_inner(socket_path: &Path, request: &Request) -> Result bool { + use std::io::{BufRead, Write}; + + let stream = match std::os::unix::net::UnixStream::connect(socket_path) { + Ok(s) => s, + Err(_) => return false, + }; + stream.set_read_timeout(Some(timeout)).ok(); + stream.set_write_timeout(Some(timeout)).ok(); + + let mut writer = match stream.try_clone() { + Ok(w) => w, + Err(_) => return false, + }; + if writer.write_all(b"{\"cmd\":\"ping\"}\n").is_err() { + return false; + } + writer.shutdown(std::net::Shutdown::Write).ok(); + + let mut reader = std::io::BufReader::new(&stream); + let mut response = String::new(); + reader.read_line(&mut response).is_ok() && response.contains("pong") +} + #[cfg(test)] mod tests { use super::*; @@ -117,6 +148,13 @@ mod tests { } } + #[test] + fn ping_sync_returns_false_on_nonexistent_socket() { + let dir = tempfile::tempdir().unwrap(); + let sock_path = dir.path().join("nonexistent.sock"); + assert!(!ping_sync(&sock_path, std::time::Duration::from_millis(100))); + } + /// Verify that send_request_with_timeout returns an error when the /// daemon doesn't respond within the timeout (e.g., socket exists /// but nothing reads from it). diff --git a/tests/e2e.rs b/tests/e2e.rs index 1ab5e9e..24e9628 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -746,7 +746,9 @@ fn test_init_output_includes_sudo_note() { let _ = std::fs::remove_dir_all(&config_dir); } -/// Verify init output includes CA certificate path +/// Verify init output includes CA certificate path. The path appears in +/// the cert generation output and/or the trust failure message, regardless +/// of whether automatic trust succeeds or fails. #[test] fn test_init_output_includes_ca_trust_path() { let config_dir = std::env::temp_dir().join(format!("devproxy-capath-test-{}", std::process::id())); @@ -761,7 +763,10 @@ fn test_init_output_includes_ca_trust_path() { assert!(output.status.success(), "init should succeed"); let stderr = String::from_utf8_lossy(&output.stderr); - // Should include the CA cert path + // The CA cert path should appear somewhere in the output -- either in + // the trust failure remediation instructions or (if trust succeeded) + // in the "Next steps" CA trust section. Since tests run without sudo, + // trust will fail and the path appears in the warning. let ca_cert_path = config_dir.join("ca-cert.pem"); assert!( stderr.contains(&ca_cert_path.display().to_string()), @@ -795,7 +800,9 @@ fn test_up_fails_fast_with_dead_daemon() { .unwrap(); // Create a stale socket file that no daemon is listening on. - // Bind a Unix socket then immediately drop it -- the file remains on disk. + // Bind a Unix socket then immediately drop it. On Unix, dropping a + // UnixListener does NOT remove the socket file -- it just closes the + // fd. The file remains on disk as an inert socket node. let socket_path = config_dir.join("devproxy.sock"); { let listener = std::os::unix::net::UnixListener::bind(&socket_path).unwrap(); @@ -894,6 +901,7 @@ fn test_reinit_kills_stale_daemon() { &mut daemon1.child, Command::new("true").spawn().unwrap(), ); + // Drop the guard -- its Drop will kill+wait the "true" dummy (harmless). drop(daemon1); // Verify the old daemon is still alive From 122459e3a502afc19f8f4c1859b2d6ceacac89d9 Mon Sep 17 00:00:00 2001 From: Chris Fenton Date: Mon, 9 Mar 2026 00:50:35 -0700 Subject: [PATCH 5/7] fix: address review round 4 findings for daemon setup - Capture daemon stderr to daemon.log and show last 10 lines on startup failure - Only remove PID file when kill is confirmed; leave for manual cleanup otherwise - Add proper SIGTERM/SIGKILL cleanup for new daemon in reinit test - Parse PID as u32 for consistency with std::process::id() output - Fix misleading test comment (hung daemon, not dead daemon) Co-Authored-By: Claude Opus 4.6 --- src/commands/init.rs | 28 ++++++++++++++++++++++++---- src/config.rs | 4 ++++ src/ipc.rs | 2 +- tests/e2e.rs | 22 ++++++++++++++-------- 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/commands/init.rs b/src/commands/init.rs index 3416b86..b3a898c 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -91,9 +91,11 @@ fn kill_stale_daemon() -> Result<()> { if pid_path.exists() { let pid_str = std::fs::read_to_string(&pid_path).unwrap_or_default(); - if let Ok(pid) = pid_str.trim().parse::() { - // PID must be positive. PID 0 would signal the entire process group, - // and negative PIDs signal process groups by absolute value. + if let Ok(pid_u32) = pid_str.trim().parse::() { + // PID 0 is not valid -- it would signal the entire process group + // via libc::kill. The daemon writes its PID as u32 (from + // std::process::id()), so 0 is the only non-positive case. + let pid = pid_u32 as i32; if pid <= 0 { eprintln!( "{} invalid PID {pid} in PID file, cleaning up stale files", @@ -164,6 +166,9 @@ fn kill_stale_daemon() -> Result<()> { "{} could not confirm daemon (pid: {pid}) was killed", "warn:".yellow() ); + // Don't remove PID file -- the process may still be alive. + // Leave it for manual cleanup. + return Ok(()); } } } else if std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) { @@ -333,9 +338,14 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { }); } + // Capture daemon stderr to a log file for debugging startup failures. + let daemon_log_path = Config::daemon_log_path()?; + let daemon_log_file = std::fs::File::create(&daemon_log_path) + .with_context(|| format!("could not create daemon log at {}", daemon_log_path.display()))?; + cmd.stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()); + .stderr(std::process::Stdio::from(daemon_log_file)); let mut child = cmd.spawn().context("could not spawn daemon")?; let pid = child.id(); @@ -353,6 +363,16 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { "{} daemon failed to start: {e}", "error:".red() ); + // Show last few lines of daemon log to help diagnose startup failures + if let Ok(log_contents) = std::fs::read_to_string(&daemon_log_path) { + let last_lines: Vec<&str> = log_contents.lines().rev().take(10).collect(); + if !last_lines.is_empty() { + eprintln!(" {} daemon log ({}):", "log:".cyan(), daemon_log_path.display()); + for line in last_lines.into_iter().rev() { + eprintln!(" {line}"); + } + } + } if port < 1024 { eprintln!( " {} port {port} requires root. Try: sudo devproxy init --domain {domain}", diff --git a/src/config.rs b/src/config.rs index b4f63a4..1cc6944 100644 --- a/src/config.rs +++ b/src/config.rs @@ -53,6 +53,10 @@ impl Config { Ok(Self::config_dir()?.join("daemon.pid")) } + pub fn daemon_log_path() -> Result { + Ok(Self::config_dir()?.join("daemon.log")) + } + pub fn save(&self) -> Result<()> { let dir = Self::config_dir()?; std::fs::create_dir_all(&dir)?; diff --git a/src/ipc.rs b/src/ipc.rs index 68646d3..ce89fd0 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -164,7 +164,7 @@ mod tests { let sock_path = dir.path().join("test.sock"); // Create a listener but never accept connections -- simulates - // a dead daemon with a stale socket. + // a hung daemon that is listening but not processing requests. let _listener = tokio::net::UnixListener::bind(&sock_path).unwrap(); let start = std::time::Instant::now(); diff --git a/tests/e2e.rs b/tests/e2e.rs index 24e9628..0d18f80 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -953,14 +953,20 @@ fn test_reinit_kills_stale_daemon() { let new_pid: u32 = new_pid_str.trim().parse().unwrap(); assert_ne!(new_pid, pid1, "new daemon should have a different PID"); - // Clean up the new daemon - let kill_ret = unsafe { libc::kill(new_pid as i32, libc::SIGTERM) }; - if kill_ret != 0 { - eprintln!( - "warning: failed to kill new daemon (pid {new_pid}): {}", - std::io::Error::last_os_error() - ); - } + // Clean up the new daemon: send SIGTERM, wait for it to exit, then + // fall back to SIGKILL if it doesn't die within 1 second. + unsafe { libc::kill(new_pid as i32, libc::SIGTERM) }; std::thread::sleep(Duration::from_millis(500)); + if unsafe { libc::kill(new_pid as i32, 0) } == 0 { + // Still alive -- force kill + unsafe { libc::kill(new_pid as i32, libc::SIGKILL) }; + std::thread::sleep(Duration::from_millis(200)); + } + // Verify the new daemon is actually dead before cleaning up + assert_ne!( + unsafe { libc::kill(new_pid as i32, 0) }, + 0, + "new daemon (pid {new_pid}) should be dead after cleanup" + ); let _ = std::fs::remove_dir_all(&config_dir); } From 28029882cda5a929fbe2c27c31a0bc429ae2f860 Mon Sep 17 00:00:00 2001 From: Chris Fenton Date: Mon, 9 Mar 2026 00:53:42 -0700 Subject: [PATCH 6/7] fix: address review round 5 findings for daemon setup - Warn on empty or unparseable PID file content before cleanup - Reject PID values > i32::MAX to prevent silent wrap to negative - Use append mode for daemon.log to preserve previous daemon's output - Eliminate unused `step` variable assignment after last usage Co-Authored-By: Claude Opus 4.6 --- src/commands/init.rs | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/commands/init.rs b/src/commands/init.rs index b3a898c..4283ee3 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -91,14 +91,19 @@ fn kill_stale_daemon() -> Result<()> { if pid_path.exists() { let pid_str = std::fs::read_to_string(&pid_path).unwrap_or_default(); - if let Ok(pid_u32) = pid_str.trim().parse::() { - // PID 0 is not valid -- it would signal the entire process group - // via libc::kill. The daemon writes its PID as u32 (from - // std::process::id()), so 0 is the only non-positive case. - let pid = pid_u32 as i32; - if pid <= 0 { + let trimmed = pid_str.trim(); + if trimmed.is_empty() { + eprintln!( + "{} PID file is empty, cleaning up stale files", + "warn:".yellow() + ); + } else if let Ok(pid_u32) = trimmed.parse::() { + // PID 0 would signal the entire process group via libc::kill, + // and values > i32::MAX would wrap negative when cast to i32 + // (which libc::kill interprets as a process group ID). + if pid_u32 == 0 || pid_u32 > i32::MAX as u32 { eprintln!( - "{} invalid PID {pid} in PID file, cleaning up stale files", + "{} invalid PID {pid_u32} in PID file, cleaning up stale files", "warn:".yellow() ); let _ = std::fs::remove_file(&pid_path); @@ -107,6 +112,7 @@ fn kill_stale_daemon() -> Result<()> { } return Ok(()); } + let pid = pid_u32 as i32; // Check if process is alive. kill(pid, 0) returns: // 0 — process exists and we can signal it @@ -179,6 +185,12 @@ fn kill_stale_daemon() -> Result<()> { ); } // else: process does not exist (ESRCH) -- fall through to file cleanup + } else { + eprintln!( + "{} PID file contains unparseable content '{}', cleaning up stale files", + "warn:".yellow(), + trimmed, + ); } let _ = std::fs::remove_file(&pid_path); } @@ -339,9 +351,13 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { } // Capture daemon stderr to a log file for debugging startup failures. + // Append mode preserves the previous daemon's log for post-mortem debugging. let daemon_log_path = Config::daemon_log_path()?; - let daemon_log_file = std::fs::File::create(&daemon_log_path) - .with_context(|| format!("could not create daemon log at {}", daemon_log_path.display()))?; + let daemon_log_file = std::fs::OpenOptions::new() + .create(true) + .append(true) + .open(&daemon_log_path) + .with_context(|| format!("could not open daemon log at {}", daemon_log_path.display()))?; cmd.stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) @@ -439,8 +455,7 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { // Project setup eprintln!(" {} Add a devproxy.port label to your docker-compose.yml", format!("{step}.").bold()); eprintln!(); - step += 1; - eprintln!(" {} Run: devproxy up", format!("{step}.").bold()); + eprintln!(" {} Run: devproxy up", format!("{}.", step + 1).bold()); Ok(()) } From 03be54fb7426b35d2adc81b9126ef77acae32be8 Mon Sep 17 00:00:00 2001 From: Chris Fenton Date: Mon, 9 Mar 2026 00:58:36 -0700 Subject: [PATCH 7/7] style: cargo fmt --- src/cli.rs | 5 +- src/commands/init.rs | 88 +++++++++---------- src/commands/up.rs | 13 +-- src/config.rs | 52 +++++++---- src/ipc.rs | 26 +++--- src/main.rs | 6 +- src/proxy/cert.rs | 21 +++-- src/proxy/docker.rs | 5 +- src/proxy/mod.rs | 73 +++++++++------- src/proxy/router.rs | 14 +-- src/slugs.rs | 18 ++-- tests/e2e.rs | 200 +++++++++++++++++++++++++++++++------------ 12 files changed, 334 insertions(+), 187 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index b6e0c41..39604b1 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,7 +1,10 @@ use clap::{Parser, Subcommand}; #[derive(Parser)] -#[command(name = "devproxy", about = "Local HTTPS dev subdomains for Docker Compose")] +#[command( + name = "devproxy", + about = "Local HTTPS dev subdomains for Docker Compose" +)] pub struct Cli { #[command(subcommand)] pub command: Commands, diff --git a/src/commands/init.rs b/src/commands/init.rs index 4283ee3..e3e399e 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -18,10 +18,7 @@ fn validate_domain(domain: &str) -> Result<()> { if label.is_empty() || label.len() > 63 { bail!("domain label '{label}' must be 1-63 characters"); } - if !label - .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '-') - { + if !label.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') { bail!("domain label '{label}' contains invalid characters (only a-z, 0-9, - allowed)"); } if label.starts_with('-') || label.ends_with('-') { @@ -59,10 +56,7 @@ fn is_devproxy_process(pid: i32) -> bool { // On Linux, read /proc//exe symlink. let exe = std::fs::read_link(format!("/proc/{pid}/exe")); match exe { - Ok(path) => path - .file_name() - .map(|n| n == "devproxy") - .unwrap_or(false), + Ok(path) => path.file_name().map(|n| n == "devproxy").unwrap_or(false), Err(_) => false, } } @@ -129,17 +123,11 @@ fn kill_stale_daemon() -> Result<()> { ); // Fall through to file cleanup } else { - eprintln!( - "{} killing stale daemon (pid: {pid})...", - "info:".cyan() - ); + eprintln!("{} killing stale daemon (pid: {pid})...", "info:".cyan()); let mut killed = false; if unsafe { libc::kill(pid, libc::SIGTERM) } != 0 { let err = std::io::Error::last_os_error(); - eprintln!( - "{} SIGTERM failed for pid {pid}: {err}", - "warn:".yellow() - ); + eprintln!("{} SIGTERM failed for pid {pid}: {err}", "warn:".yellow()); // EPERM means we can't signal it -- bail with guidance if err.raw_os_error() == Some(libc::EPERM) { bail!( @@ -159,10 +147,7 @@ fn kill_stale_daemon() -> Result<()> { killed = true; } else { let err = std::io::Error::last_os_error(); - eprintln!( - "{} SIGKILL failed for pid {pid}: {err}", - "warn:".yellow() - ); + eprintln!("{} SIGKILL failed for pid {pid}: {err}", "warn:".yellow()); } } if killed { @@ -212,9 +197,7 @@ fn wait_for_daemon(timeout: Duration) -> Result<()> { let poll_interval = Duration::from_millis(100); while start.elapsed() < timeout { - if socket_path.exists() - && crate::ipc::ping_sync(&socket_path, Duration::from_secs(1)) - { + if socket_path.exists() && crate::ipc::ping_sync(&socket_path, Duration::from_secs(1)) { return Ok(()); } std::thread::sleep(poll_interval); @@ -228,7 +211,9 @@ fn wait_for_daemon(timeout: Duration) -> Result<()> { pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { validate_domain(domain)?; - let config = Config { domain: domain.to_string() }; + let config = Config { + domain: domain.to_string(), + }; // Create config directory let config_dir = Config::config_dir()?; @@ -257,13 +242,8 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { Ok(()) => eprintln!("{} CA trusted in system keychain", "ok:".green()), Err(e) => { ca_trust_needed = true; - eprintln!( - "{} could not trust CA automatically: {e}", - "warn:".yellow() - ); - eprintln!( - " run manually with sudo:" - ); + eprintln!("{} could not trust CA automatically: {e}", "warn:".yellow()); + eprintln!(" run manually with sudo:"); #[cfg(target_os = "macos")] eprintln!( " sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain {}", @@ -303,7 +283,8 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { let ca_key_pem = std::fs::read_to_string(&ca_key_path)?; eprintln!("generating wildcard TLS certificate for *.{domain}..."); - let (tls_cert_pem, tls_key_pem) = cert::generate_wildcard_cert(domain, &ca_cert_pem, &ca_key_pem)?; + let (tls_cert_pem, tls_key_pem) = + cert::generate_wildcard_cert(domain, &ca_cert_pem, &ca_key_pem)?; cert::write_pem(&tls_cert_path, &tls_cert_pem)?; cert::write_key_pem(&tls_key_path, &tls_key_pem)?; eprintln!("{} TLS certificate generated", "ok:".green()); @@ -357,7 +338,9 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { .create(true) .append(true) .open(&daemon_log_path) - .with_context(|| format!("could not open daemon log at {}", daemon_log_path.display()))?; + .with_context(|| { + format!("could not open daemon log at {}", daemon_log_path.display()) + })?; cmd.stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::null()) @@ -367,7 +350,9 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { let pid = child.id(); // Spawn a thread to reap the child so it does not become a zombie. // After setsid(), the child won't receive signals when the parent exits. - std::thread::spawn(move || { let _ = child.wait(); }); + std::thread::spawn(move || { + let _ = child.wait(); + }); // Wait for daemon to become responsive (or fail fast) match wait_for_daemon(Duration::from_secs(5)) { @@ -375,15 +360,16 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { eprintln!("{} daemon started (pid: {pid})", "ok:".green()); } Err(e) => { - eprintln!( - "{} daemon failed to start: {e}", - "error:".red() - ); + eprintln!("{} daemon failed to start: {e}", "error:".red()); // Show last few lines of daemon log to help diagnose startup failures if let Ok(log_contents) = std::fs::read_to_string(&daemon_log_path) { let last_lines: Vec<&str> = log_contents.lines().rev().take(10).collect(); if !last_lines.is_empty() { - eprintln!(" {} daemon log ({}):", "log:".cyan(), daemon_log_path.display()); + eprintln!( + " {} daemon log ({}):", + "log:".cyan(), + daemon_log_path.display() + ); for line in last_lines.into_iter().rev() { eprintln!(" {line}"); } @@ -407,7 +393,10 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { eprintln!(); // DNS setup instructions - eprintln!(" {} Set up wildcard DNS for *.{domain} -> 127.0.0.1", "1.".bold()); + eprintln!( + " {} Set up wildcard DNS for *.{domain} -> 127.0.0.1", + "1.".bold() + ); #[cfg(target_os = "macos")] { eprintln!(); @@ -416,13 +405,18 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { eprintln!(" sudo brew services start dnsmasq"); eprintln!(); eprintln!(" Add wildcard DNS rule:"); - eprintln!(" echo 'address=/.{domain}/127.0.0.1' >> $(brew --prefix)/etc/dnsmasq.conf"); + eprintln!( + " echo 'address=/.{domain}/127.0.0.1' >> $(brew --prefix)/etc/dnsmasq.conf" + ); eprintln!(" sudo brew services restart dnsmasq"); eprintln!(); // Extract the TLD for the resolver. rsplit('.').next() always // returns Some (the last segment, or the whole string if no dot), // but we already validated the domain has at least two labels above. - let tld = domain.rsplit('.').next().expect("validated domain has a dot"); + let tld = domain + .rsplit('.') + .next() + .expect("validated domain has a dot"); eprintln!(" Create resolver for .{tld} domains:"); eprintln!(" sudo mkdir -p /etc/resolver"); eprintln!(" echo 'nameserver 127.0.0.1' | sudo tee /etc/resolver/{tld}"); @@ -437,7 +431,10 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { let mut step = 2; if ca_trust_needed { eprintln!(" {} Trust the CA certificate", format!("{step}.").bold()); - eprintln!(" CA cert: {}", ca_cert_path.display().to_string().cyan()); + eprintln!( + " CA cert: {}", + ca_cert_path.display().to_string().cyan() + ); #[cfg(target_os = "macos")] eprintln!( " sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain {}", @@ -453,7 +450,10 @@ pub fn run(domain: &str, port: u16, no_daemon: bool) -> Result<()> { } // Project setup - eprintln!(" {} Add a devproxy.port label to your docker-compose.yml", format!("{step}.").bold()); + eprintln!( + " {} Add a devproxy.port label to your docker-compose.yml", + format!("{step}.").bold() + ); eprintln!(); eprintln!(" {} Run: devproxy up", format!("{}.", step + 1).bold()); diff --git a/src/commands/up.rs b/src/commands/up.rs index 5586555..86ec994 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -37,11 +37,9 @@ pub fn run() -> Result<()> { eprintln!("host port: {}", host_port.to_string().cyan()); // Write override file (port binding) - let override_path = config::write_override_file(compose_dir, &service_name, host_port, container_port)?; - eprintln!( - "override: {}", - override_path.display().to_string().cyan() - ); + let override_path = + config::write_override_file(compose_dir, &service_name, host_port, container_port)?; + eprintln!("override: {}", override_path.display().to_string().cyan()); // Write project file (slug tracking -- used by `down` and `open`) config::write_project_file(compose_dir, &slug)?; @@ -53,7 +51,10 @@ pub fn run() -> Result<()> { // Clean up files we already wrote let _ = std::fs::remove_file(&override_path); let _ = std::fs::remove_file(compose_dir.join(".devproxy-project")); - bail!("daemon is not running (no socket at {}). Run `devproxy init` first.", socket_path.display()); + bail!( + "daemon is not running (no socket at {}). Run `devproxy init` first.", + socket_path.display() + ); } // Send an actual IPC ping with a 2s timeout to verify the daemon is diff --git a/src/config.rs b/src/config.rs index 1cc6944..61cdebe 100644 --- a/src/config.rs +++ b/src/config.rs @@ -126,9 +126,9 @@ pub fn find_devproxy_service(compose: &ComposeFile) -> Result<(String, u16)> { for (name, svc) in &compose.services { if let Some(port_str) = svc.labels.get("devproxy.port") { - let port: u16 = port_str - .parse() - .with_context(|| format!("invalid devproxy.port value '{port_str}' on service '{name}'"))?; + let port: u16 = port_str.parse().with_context(|| { + format!("invalid devproxy.port value '{port_str}' on service '{name}'") + })?; found.push((name.clone(), port)); } } @@ -138,7 +138,11 @@ pub fn find_devproxy_service(compose: &ComposeFile) -> Result<(String, u16)> { 1 => Ok(found.into_iter().next().expect("checked len")), _ => bail!( "multiple services have devproxy.port labels: {}. Only one is supported.", - found.iter().map(|(n, _)| n.as_str()).collect::>().join(", ") + found + .iter() + .map(|(n, _)| n.as_str()) + .collect::>() + .join(", ") ), } } @@ -156,7 +160,12 @@ pub fn parse_compose_file(path: &Path) -> Result { /// Searches for docker-compose.yml, docker-compose.yaml, compose.yml, compose.yaml. /// Returns the full path. pub fn find_compose_file(dir: &Path) -> Result { - for name in &["docker-compose.yml", "docker-compose.yaml", "compose.yml", "compose.yaml"] { + for name in &[ + "docker-compose.yml", + "docker-compose.yaml", + "compose.yml", + "compose.yaml", + ] { let path = dir.join(name); if path.exists() { return Ok(path); @@ -170,7 +179,12 @@ pub fn find_compose_file(dir: &Path) -> Result { /// The service name is validated to contain only alphanumeric, hyphen, and /// underscore characters before being interpolated into YAML, preventing /// injection of arbitrary YAML content. -pub fn write_override_file(dir: &Path, service_name: &str, host_port: u16, container_port: u16) -> Result { +pub fn write_override_file( + dir: &Path, + service_name: &str, + host_port: u16, + container_port: u16, +) -> Result { // Validate service name to prevent YAML injection if service_name.is_empty() || !service_name @@ -202,11 +216,12 @@ pub fn write_project_file(dir: &Path, slug: &str) -> Result { /// Returns an error if the file doesn't exist (project not running via devproxy). pub fn read_project_file(dir: &Path) -> Result { let path = dir.join(".devproxy-project"); - let content = std::fs::read_to_string(&path) - .with_context(|| format!( + let content = std::fs::read_to_string(&path).with_context(|| { + format!( "no .devproxy-project file found in {}. Is this project running via `devproxy up`?", dir.display() - ))?; + ) + })?; Ok(content.trim().to_string()) } @@ -236,10 +251,7 @@ mod tests { let config_dir = dir.path().to_path_buf(); // Write a minimal config so `status` tries to connect to the socket - std::fs::write( - config_dir.join("config.json"), - r#"{"domain":"test.dev"}"#, - ).unwrap(); + std::fs::write(config_dir.join("config.json"), r#"{"domain":"test.dev"}"#).unwrap(); // Find the binary let mut bin = std::env::current_exe().unwrap(); @@ -340,7 +352,12 @@ services: let dir = tempfile::tempdir().unwrap(); let result = read_project_file(dir.path()); assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains(".devproxy-project")); + assert!( + result + .unwrap_err() + .to_string() + .contains(".devproxy-project") + ); } #[test] @@ -356,6 +373,11 @@ services: let dir = tempfile::tempdir().unwrap(); let result = find_compose_file(dir.path()); assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("no docker-compose.yml")); + assert!( + result + .unwrap_err() + .to_string() + .contains("no docker-compose.yml") + ); } } diff --git a/src/ipc.rs b/src/ipc.rs index ce89fd0..688dc47 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -51,14 +51,12 @@ pub async fn send_request_with_timeout( } async fn send_request_inner(socket_path: &Path, request: &Request) -> Result { - let stream = UnixStream::connect(socket_path) - .await - .with_context(|| { - format!( - "could not connect to daemon at {}. Is the daemon running? Try `devproxy init`.", - socket_path.display() - ) - })?; + let stream = UnixStream::connect(socket_path).await.with_context(|| { + format!( + "could not connect to daemon at {}. Is the daemon running? Try `devproxy init`.", + socket_path.display() + ) + })?; let (reader, mut writer) = stream.into_split(); @@ -73,8 +71,8 @@ async fn send_request_inner(socket_path: &Path, request: &Request) -> Result { @@ -152,7 +151,10 @@ mod tests { fn ping_sync_returns_false_on_nonexistent_socket() { let dir = tempfile::tempdir().unwrap(); let sock_path = dir.path().join("nonexistent.sock"); - assert!(!ping_sync(&sock_path, std::time::Duration::from_millis(100))); + assert!(!ping_sync( + &sock_path, + std::time::Duration::from_millis(100) + )); } /// Verify that send_request_with_timeout returns an error when the diff --git a/src/main.rs b/src/main.rs index b521624..6eb3283 100644 --- a/src/main.rs +++ b/src/main.rs @@ -13,7 +13,11 @@ async fn main() -> anyhow::Result<()> { let cli = Cli::parse(); match cli.command { - Commands::Init { domain, port, no_daemon } => commands::init::run(&domain, port, no_daemon), + Commands::Init { + domain, + port, + no_daemon, + } => commands::init::run(&domain, port, no_daemon), Commands::Up => commands::up::run(), Commands::Down => commands::down::run(), Commands::Ls => commands::ls::run().await, diff --git a/src/proxy/cert.rs b/src/proxy/cert.rs index 16a9f2f..93a2a13 100644 --- a/src/proxy/cert.rs +++ b/src/proxy/cert.rs @@ -16,10 +16,7 @@ pub fn generate_ca() -> Result<(String, String)> { params .distinguished_name .push(DnType::OrganizationName, "devproxy"); - params.key_usages = vec![ - KeyUsagePurpose::KeyCertSign, - KeyUsagePurpose::CrlSign, - ]; + params.key_usages = vec![KeyUsagePurpose::KeyCertSign, KeyUsagePurpose::CrlSign]; params.not_before = time::OffsetDateTime::now_utc() - Duration::from_secs(3600); params.not_after = time::OffsetDateTime::now_utc() + Duration::from_secs(365 * 24 * 3600 * 10); @@ -40,14 +37,20 @@ pub fn generate_wildcard_cert( let ca_key = KeyPair::from_pem(ca_key_pem).context("failed to parse CA key")?; let ca_params = CertificateParams::from_ca_cert_pem(ca_cert_pem) .context("failed to parse CA cert params")?; - let ca_cert = ca_params.self_signed(&ca_key).context("failed to reconstruct CA cert")?; + let ca_cert = ca_params + .self_signed(&ca_key) + .context("failed to reconstruct CA cert")?; let mut params = CertificateParams::default(); params .distinguished_name .push(DnType::CommonName, format!("*.{domain}")); params.subject_alt_names = vec![ - SanType::DnsName(format!("*.{domain}").try_into().context("invalid wildcard DNS name")?), + SanType::DnsName( + format!("*.{domain}") + .try_into() + .context("invalid wildcard DNS name")?, + ), SanType::DnsName(domain.to_string().try_into().context("invalid DNS name")?), ]; params.extended_key_usages = vec![ExtendedKeyUsagePurpose::ServerAuth]; @@ -140,8 +143,10 @@ pub fn trust_ca_in_system(ca_cert_path: &Path) -> Result<()> { .args([ "add-trusted-cert", "-d", - "-r", "trustRoot", - "-k", "/Library/Keychains/System.keychain", + "-r", + "trustRoot", + "-k", + "/Library/Keychains/System.keychain", ]) .arg(ca_cert_path) .status() diff --git a/src/proxy/docker.rs b/src/proxy/docker.rs index 05d7750..7441fc1 100644 --- a/src/proxy/docker.rs +++ b/src/proxy/docker.rs @@ -171,7 +171,10 @@ async fn watch_events_inner(router: &Router) -> Result<()> { .spawn() .context("failed to spawn docker events")?; - let stdout = child.stdout.take().context("no stdout from docker events")?; + let stdout = child + .stdout + .take() + .context("no stdout from docker events")?; let reader = BufReader::new(stdout); let mut lines = reader.lines(); diff --git a/src/proxy/mod.rs b/src/proxy/mod.rs index c1862cb..e46830d 100644 --- a/src/proxy/mod.rs +++ b/src/proxy/mod.rs @@ -41,10 +41,7 @@ pub async fn run_daemon(port: u16) -> Result<()> { let router = Router::new(&config.domain); // Load TLS config - let tls_acceptor = cert::load_tls_config( - &Config::tls_cert_path()?, - &Config::tls_key_path()?, - )?; + let tls_acceptor = cert::load_tls_config(&Config::tls_cert_path()?, &Config::tls_key_path()?)?; // Load existing routes from running containers eprintln!("loading existing routes..."); @@ -118,9 +115,21 @@ pub async fn run_daemon(port: u16) -> Result<()> { let r3 = router.clone(); tokio::try_join!( - async { https_proxy_loop(tcp_listener, tls_acceptor, r1).await.context("HTTPS proxy task failed") }, - async { docker::watch_events(&r2).await.context("Docker watcher task failed") }, - async { ipc_server_loop(ipc_listener, r3).await.context("IPC server task failed") }, + async { + https_proxy_loop(tcp_listener, tls_acceptor, r1) + .await + .context("HTTPS proxy task failed") + }, + async { + docker::watch_events(&r2) + .await + .context("Docker watcher task failed") + }, + async { + ipc_server_loop(ipc_listener, r3) + .await + .context("IPC server task failed") + }, )?; Ok(()) @@ -139,10 +148,7 @@ async fn ipc_server_loop(listener: UnixListener, router: Router) -> Result<()> { } } -async fn handle_ipc_connection( - stream: tokio::net::UnixStream, - router: &Router, -) -> Result<()> { +async fn handle_ipc_connection(stream: tokio::net::UnixStream, router: &Router) -> Result<()> { let (reader, mut writer) = stream.into_split(); // Limit reads to 64KB to prevent unbounded memory allocation from // malicious or malformed IPC messages. @@ -150,8 +156,8 @@ async fn handle_ipc_connection( let mut line = String::new(); buf_reader.read_line(&mut line).await?; - let request: Request = serde_json::from_str(line.trim()) - .context("could not parse IPC request")?; + let request: Request = + serde_json::from_str(line.trim()).context("could not parse IPC request")?; let response = match request { Request::Ping => Response::Pong, @@ -183,7 +189,10 @@ async fn https_proxy_loop( let (tcp_stream, _addr) = listener.accept().await?; let acceptor = acceptor.clone(); let router = router.clone(); - let permit = semaphore.clone().acquire_owned().await + let permit = semaphore + .clone() + .acquire_owned() + .await .context("connection semaphore closed")?; tokio::spawn(async move { @@ -196,10 +205,9 @@ async fn https_proxy_loop( async move { handle_request(req, &router).await } }); - if let Err(e) = - http1::Builder::new() - .serve_connection(hyper_util::rt::TokioIo::new(tls_stream), service) - .await + if let Err(e) = http1::Builder::new() + .serve_connection(hyper_util::rt::TokioIo::new(tls_stream), service) + .await { eprintln!(" HTTP error: {e}"); } @@ -239,7 +247,11 @@ async fn handle_request( }; // Build upstream URI - let path_and_query = req.uri().path_and_query().map(|pq| pq.as_str().to_string()).unwrap_or_else(|| "/".to_string()); + let path_and_query = req + .uri() + .path_and_query() + .map(|pq| pq.as_str().to_string()) + .unwrap_or_else(|| "/".to_string()); let upstream_addr = format!("127.0.0.1:{host_port}"); // Forward the request to the container @@ -272,13 +284,11 @@ async fn proxy_to_upstream( let io = hyper_util::rt::TokioIo::new(stream); - let (mut sender, conn) = tokio::time::timeout( - UPSTREAM_TIMEOUT, - hyper::client::conn::http1::handshake(io), - ) - .await - .context("upstream handshake timed out")? - .context("upstream handshake failed")?; + let (mut sender, conn) = + tokio::time::timeout(UPSTREAM_TIMEOUT, hyper::client::conn::http1::handshake(io)) + .await + .context("upstream handshake timed out")? + .context("upstream handshake failed")?; tokio::spawn(async move { if let Err(e) = conn.await { @@ -309,9 +319,7 @@ async fn proxy_to_upstream( // correct domain for URL generation, CSRF checks, and virtual host // routing. The upstream container is already targeted by the TCP // connection address. - let mut builder = HyperRequest::builder() - .method(method) - .uri(upstream_uri); + let mut builder = HyperRequest::builder().method(method).uri(upstream_uri); for (name, value) in headers.iter() { builder = builder.header(name.clone(), value.clone()); @@ -333,11 +341,12 @@ async fn proxy_to_upstream( let body = limited_resp .collect() .await - .map_err(|e| anyhow::anyhow!("upstream response too large (max {MAX_BODY_SIZE} bytes): {e}"))? + .map_err(|e| { + anyhow::anyhow!("upstream response too large (max {MAX_BODY_SIZE} bytes): {e}") + })? .to_bytes(); - let mut resp_builder = HyperResponse::builder() - .status(status); + let mut resp_builder = HyperResponse::builder().status(status); for (name, value) in resp_headers.iter() { resp_builder = resp_builder.header(name.clone(), value.clone()); diff --git a/src/proxy/router.rs b/src/proxy/router.rs index 4de482f..aac9ee6 100644 --- a/src/proxy/router.rs +++ b/src/proxy/router.rs @@ -31,16 +31,20 @@ impl Router { /// Insert a route: slug -> host_port. The full hostname is slug.domain. pub fn insert(&self, slug: &str, host_port: u16) { let hostname = format!("{slug}.{}", self.domain); - let route = Route { - host_port, - }; - self.routes.write().expect("lock poisoned").insert(hostname, route); + let route = Route { host_port }; + self.routes + .write() + .expect("lock poisoned") + .insert(hostname, route); } /// Remove a route by slug pub fn remove(&self, slug: &str) { let hostname = format!("{slug}.{}", self.domain); - self.routes.write().expect("lock poisoned").remove(&hostname); + self.routes + .write() + .expect("lock poisoned") + .remove(&hostname); } /// Look up a host_port by full hostname (e.g., "swift-penguin.mysite.dev") diff --git a/src/slugs.rs b/src/slugs.rs index fc80917..73639d4 100644 --- a/src/slugs.rs +++ b/src/slugs.rs @@ -1,14 +1,12 @@ use rand::seq::IndexedRandom; const ADJECTIVES: &[&str] = &[ - "swift", "bright", "calm", "bold", "keen", - "warm", "cool", "wild", "fair", "glad", - "quick", "brave", "proud", "true", "wise", + "swift", "bright", "calm", "bold", "keen", "warm", "cool", "wild", "fair", "glad", "quick", + "brave", "proud", "true", "wise", ]; const ANIMALS: &[&str] = &[ - "penguin", "falcon", "otter", "fox", "heron", - "whale", "eagle", "tiger", "panda", "koala", + "penguin", "falcon", "otter", "fox", "heron", "whale", "eagle", "tiger", "panda", "koala", "raven", "wolf", "lynx", "hawk", "crane", ]; @@ -28,8 +26,14 @@ mod tests { let slug = generate_slug(); let parts: Vec<&str> = slug.split('-').collect(); assert_eq!(parts.len(), 2, "slug should be adjective-animal: {slug}"); - assert!(ADJECTIVES.contains(&parts[0]), "first word should be an adjective: {slug}"); - assert!(ANIMALS.contains(&parts[1]), "second word should be an animal: {slug}"); + assert!( + ADJECTIVES.contains(&parts[0]), + "first word should be an adjective: {slug}" + ); + assert!( + ANIMALS.contains(&parts[1]), + "second word should be an animal: {slug}" + ); } #[test] diff --git a/tests/e2e.rs b/tests/e2e.rs index 0d18f80..f4cbeed 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -46,7 +46,10 @@ fn find_free_port() -> u16 { /// Copy the fixtures directory into an isolated temp dir for one test. /// Returns the path to the copy (which contains docker-compose.yml, Dockerfile, etc). fn copy_fixtures(test_name: &str) -> PathBuf { - let dest = std::env::temp_dir().join(format!("devproxy-fixtures-{test_name}-{}", std::process::id())); + let dest = std::env::temp_dir().join(format!( + "devproxy-fixtures-{test_name}-{}", + std::process::id() + )); if dest.exists() { std::fs::remove_dir_all(&dest).unwrap(); } @@ -64,7 +67,10 @@ fn copy_fixtures(test_name: &str) -> PathBuf { /// Create an isolated test config directory and generate certs using `init --no-daemon`. /// Returns the path to the config directory (to be set as DEVPROXY_CONFIG_DIR). fn create_test_config_dir(test_name: &str) -> PathBuf { - let config_dir = std::env::temp_dir().join(format!("devproxy-config-{test_name}-{}", std::process::id())); + let config_dir = std::env::temp_dir().join(format!( + "devproxy-config-{test_name}-{}", + std::process::id() + )); if config_dir.exists() { std::fs::remove_dir_all(&config_dir).unwrap(); } @@ -86,9 +92,18 @@ fn create_test_config_dir(test_name: &str) -> PathBuf { ); // Verify certs were created - assert!(config_dir.join("ca-cert.pem").exists(), "CA cert should exist after init"); - assert!(config_dir.join("tls-cert.pem").exists(), "TLS cert should exist after init"); - assert!(config_dir.join("config.json").exists(), "config should exist after init"); + assert!( + config_dir.join("ca-cert.pem").exists(), + "CA cert should exist after init" + ); + assert!( + config_dir.join("tls-cert.pem").exists(), + "TLS cert should exist after init" + ); + assert!( + config_dir.join("config.json").exists(), + "config should exist after init" + ); config_dir } @@ -126,15 +141,16 @@ fn start_test_daemon(config_dir: &Path, port: u16) -> DaemonGuard { // Wait for IPC socket to become connectable let socket_path = config_dir.join("devproxy.sock"); for _ in 0..50 { - if socket_path.exists() - && std::os::unix::net::UnixStream::connect(&socket_path).is_ok() - { + if socket_path.exists() && std::os::unix::net::UnixStream::connect(&socket_path).is_ok() { return guard; } std::thread::sleep(Duration::from_millis(100)); } - panic!("daemon did not start within 5 seconds (socket: {})", socket_path.display()); + panic!( + "daemon did not start within 5 seconds (socket: {})", + socket_path.display() + ); } /// Guard that runs docker compose down on drop and cleans up the fixtures copy @@ -185,14 +201,17 @@ fn test_cli_help() { // Daemon should be hidden as a top-level command (it may appear in descriptions) // Check that "daemon" does not appear as a command entry (lines starting with " daemon") assert!( - !stdout.lines().any(|l| l.trim_start().starts_with("daemon ")), + !stdout + .lines() + .any(|l| l.trim_start().starts_with("daemon ")), "daemon command should be hidden from help" ); } #[test] fn test_init_generates_certs() { - let config_dir = std::env::temp_dir().join(format!("devproxy-init-test-{}", std::process::id())); + let config_dir = + std::env::temp_dir().join(format!("devproxy-init-test-{}", std::process::id())); std::fs::create_dir_all(&config_dir).unwrap(); let output = Command::new(devproxy_bin()) @@ -202,11 +221,26 @@ fn test_init_generates_certs() { .expect("failed to run devproxy init"); assert!(output.status.success(), "init should succeed"); - assert!(config_dir.join("ca-cert.pem").exists(), "CA cert should exist"); - assert!(config_dir.join("ca-key.pem").exists(), "CA key should exist"); - assert!(config_dir.join("tls-cert.pem").exists(), "TLS cert should exist"); - assert!(config_dir.join("tls-key.pem").exists(), "TLS key should exist"); - assert!(config_dir.join("config.json").exists(), "config should exist"); + assert!( + config_dir.join("ca-cert.pem").exists(), + "CA cert should exist" + ); + assert!( + config_dir.join("ca-key.pem").exists(), + "CA key should exist" + ); + assert!( + config_dir.join("tls-cert.pem").exists(), + "TLS cert should exist" + ); + assert!( + config_dir.join("tls-key.pem").exists(), + "TLS key should exist" + ); + assert!( + config_dir.join("config.json").exists(), + "config should exist" + ); // Verify idempotency: running init again should succeed and not error let output2 = Command::new(devproxy_bin()) @@ -217,7 +251,10 @@ fn test_init_generates_certs() { assert!(output2.status.success(), "init should be idempotent"); let stderr2 = String::from_utf8_lossy(&output2.stderr); - assert!(stderr2.contains("already exists"), "should report certs already exist"); + assert!( + stderr2.contains("already exists"), + "should report certs already exist" + ); let _ = std::fs::remove_dir_all(&config_dir); } @@ -258,7 +295,8 @@ fn test_up_without_label() { .unwrap(); // Create a compose file without devproxy.port - let test_dir = std::env::temp_dir().join(format!("devproxy-nolabel-project-{}", std::process::id())); + let test_dir = + std::env::temp_dir().join(format!("devproxy-nolabel-project-{}", std::process::id())); std::fs::create_dir_all(&test_dir).unwrap(); std::fs::write( test_dir.join("docker-compose.yml"), @@ -289,7 +327,8 @@ fn test_up_without_label() { #[test] fn test_up_without_compose_file() { - let config_dir = std::env::temp_dir().join(format!("devproxy-nocompose-{}", std::process::id())); + let config_dir = + std::env::temp_dir().join(format!("devproxy-nocompose-{}", std::process::id())); std::fs::create_dir_all(&config_dir).unwrap(); std::fs::write( config_dir.join("config.json"), @@ -297,7 +336,8 @@ fn test_up_without_compose_file() { ) .unwrap(); - let test_dir = std::env::temp_dir().join(format!("devproxy-nocompose-project-{}", std::process::id())); + let test_dir = + std::env::temp_dir().join(format!("devproxy-nocompose-project-{}", std::process::id())); std::fs::create_dir_all(&test_dir).unwrap(); let output = Command::new(devproxy_bin()) @@ -307,7 +347,10 @@ fn test_up_without_compose_file() { .output() .expect("failed to run up"); - assert!(!output.status.success(), "up should fail without compose file"); + assert!( + !output.status.success(), + "up should fail without compose file" + ); let stderr = String::from_utf8_lossy(&output.stderr); assert!( stderr.contains("no docker-compose.yml"), @@ -328,7 +371,8 @@ fn test_down_without_project_file() { ) .unwrap(); - let config_dir = std::env::temp_dir().join(format!("devproxy-noproject-cfg-{}", std::process::id())); + let config_dir = + std::env::temp_dir().join(format!("devproxy-noproject-cfg-{}", std::process::id())); std::fs::create_dir_all(&config_dir).unwrap(); let output = Command::new(devproxy_bin()) @@ -338,7 +382,10 @@ fn test_down_without_project_file() { .output() .expect("failed to run down"); - assert!(!output.status.success(), "down should fail without .devproxy-project"); + assert!( + !output.status.success(), + "down should fail without .devproxy-project" + ); let stderr = String::from_utf8_lossy(&output.stderr); assert!( stderr.contains(".devproxy-project") || stderr.contains("Is this project running"), @@ -381,24 +428,30 @@ fn test_full_e2e_workflow() { let up_stderr = String::from_utf8_lossy(&up_output.stderr); eprintln!("up output: {up_stderr}"); - assert!(up_output.status.success(), "devproxy up should succeed: {up_stderr}"); + assert!( + up_output.status.success(), + "devproxy up should succeed: {up_stderr}" + ); // Extract slug from output (look for "-> https://.test.devproxy.dev") let slug = up_stderr .lines() .find(|l| l.contains(&format!(".{TEST_DOMAIN}"))) - .and_then(|l| { - l.split("https://") - .nth(1) - .and_then(|s| s.split('.').next()) - }) + .and_then(|l| l.split("https://").nth(1).and_then(|s| s.split('.').next())) .expect("should find slug in up output"); // Verify .devproxy-project was written with the correct slug let project_file = fixtures.join(".devproxy-project"); - assert!(project_file.exists(), ".devproxy-project should exist after up"); + assert!( + project_file.exists(), + ".devproxy-project should exist after up" + ); let saved_slug = std::fs::read_to_string(&project_file).unwrap(); - assert_eq!(saved_slug.trim(), slug, ".devproxy-project should contain the slug"); + assert_eq!( + saved_slug.trim(), + slug, + ".devproxy-project should contain the slug" + ); let _compose_guard = ComposeGuard { project_name: slug.to_string(), @@ -483,11 +536,20 @@ fn test_full_e2e_workflow() { .output() .expect("failed to run devproxy down"); let down_stderr = String::from_utf8_lossy(&down_output.stderr); - assert!(down_output.status.success(), "devproxy down should succeed: {down_stderr}"); + assert!( + down_output.status.success(), + "devproxy down should succeed: {down_stderr}" + ); // Verify cleanup files are gone - assert!(!fixtures.join(".devproxy-project").exists(), ".devproxy-project should be removed after down"); - assert!(!fixtures.join(".devproxy-override.yml").exists(), ".devproxy-override.yml should be removed after down"); + assert!( + !fixtures.join(".devproxy-project").exists(), + ".devproxy-project should be removed after down" + ); + assert!( + !fixtures.join(".devproxy-override.yml").exists(), + ".devproxy-override.yml should be removed after down" + ); } /// Test self-healing: kill container externally -> route removed from daemon @@ -536,7 +598,10 @@ fn test_self_healing_route_removed_on_container_die() { .output() .expect("failed to ls"); let ls_before_stdout = String::from_utf8_lossy(&ls_before.stdout); - assert!(ls_before_stdout.contains(slug), "route should exist before kill: {ls_before_stdout}"); + assert!( + ls_before_stdout.contains(slug), + "route should exist before kill: {ls_before_stdout}" + ); // Kill container externally (not via devproxy) let kill_status = Command::new("docker") @@ -648,18 +713,27 @@ fn test_proxy_502_for_unknown_host() { let curl_output = Command::new("curl") .args([ "-s", - "-o", "/dev/null", - "-w", "%{http_code}", - "--max-time", "5", - "--resolve", &format!("{host}:{daemon_port}:127.0.0.1"), - "--cacert", &ca_cert_path.to_string_lossy(), + "-o", + "/dev/null", + "-w", + "%{http_code}", + "--max-time", + "5", + "--resolve", + &format!("{host}:{daemon_port}:127.0.0.1"), + "--cacert", + &ca_cert_path.to_string_lossy(), &url, ]) .output() .expect("failed to run curl"); let status_code = String::from_utf8_lossy(&curl_output.stdout); - assert_eq!(status_code.trim(), "502", "should get 502 for unknown host, got: {status_code}"); + assert_eq!( + status_code.trim(), + "502", + "should get 502 for unknown host, got: {status_code}" + ); } /// IPC ping/pong test @@ -678,8 +752,14 @@ fn test_ipc_ping_pong() { assert!(output.status.success(), "status should succeed"); let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("running"), "should report daemon running: {stderr}"); - assert!(stderr.contains("active routes: 0"), "should report 0 routes: {stderr}"); + assert!( + stderr.contains("running"), + "should report daemon running: {stderr}" + ); + assert!( + stderr.contains("active routes: 0"), + "should report 0 routes: {stderr}" + ); } // ---- New daemon setup flow tests ------------------------------------------- @@ -724,7 +804,8 @@ fn test_init_output_includes_dns_instructions() { /// Verify init output includes sudo note for port 443 #[test] fn test_init_output_includes_sudo_note() { - let config_dir = std::env::temp_dir().join(format!("devproxy-sudo-test-{}", std::process::id())); + let config_dir = + std::env::temp_dir().join(format!("devproxy-sudo-test-{}", std::process::id())); std::fs::create_dir_all(&config_dir).unwrap(); // Use --no-daemon so we don't need root, but still verify the output mentions sudo @@ -751,7 +832,8 @@ fn test_init_output_includes_sudo_note() { /// of whether automatic trust succeeds or fails. #[test] fn test_init_output_includes_ca_trust_path() { - let config_dir = std::env::temp_dir().join(format!("devproxy-capath-test-{}", std::process::id())); + let config_dir = + std::env::temp_dir().join(format!("devproxy-capath-test-{}", std::process::id())); std::fs::create_dir_all(&config_dir).unwrap(); let output = Command::new(devproxy_bin()) @@ -791,7 +873,8 @@ fn test_up_fails_fast_with_dead_daemon() { .unwrap(); // Create a compose file with devproxy.port - let test_dir = std::env::temp_dir().join(format!("devproxy-deadup-project-{}", std::process::id())); + let test_dir = + std::env::temp_dir().join(format!("devproxy-deadup-project-{}", std::process::id())); std::fs::create_dir_all(&test_dir).unwrap(); std::fs::write( test_dir.join("docker-compose.yml"), @@ -860,12 +943,16 @@ fn test_daemon_writes_pid_file() { ); let pid_str = std::fs::read_to_string(&pid_path).unwrap(); - let pid: u32 = pid_str.trim().parse().expect("PID file should contain a valid number"); + let pid: u32 = pid_str + .trim() + .parse() + .expect("PID file should contain a valid number"); assert!(pid > 0, "PID should be positive"); // Verify the PID matches the actual daemon process assert_eq!( - pid, daemon.child.id(), + pid, + daemon.child.id(), "PID file should match the daemon's actual PID" ); } @@ -884,7 +971,10 @@ fn test_reinit_kills_stale_daemon() { // Verify first daemon is alive and has a PID file let pid_path = config_dir.join("daemon.pid"); - assert!(pid_path.exists(), "PID file should exist after first daemon start"); + assert!( + pid_path.exists(), + "PID file should exist after first daemon start" + ); let saved_pid: u32 = std::fs::read_to_string(&pid_path) .unwrap() .trim() @@ -897,10 +987,8 @@ fn test_reinit_kills_stale_daemon() { // with a dummy so the guard's Drop is harmless, and keep the real // child handle for cleanup at end of test. daemon1.config_dir = PathBuf::new(); - let mut original_child = std::mem::replace( - &mut daemon1.child, - Command::new("true").spawn().unwrap(), - ); + let mut original_child = + std::mem::replace(&mut daemon1.child, Command::new("true").spawn().unwrap()); // Drop the guard -- its Drop will kill+wait the "true" dummy (harmless). drop(daemon1); @@ -916,8 +1004,10 @@ fn test_reinit_kills_stale_daemon() { let output = Command::new(devproxy_bin()) .args([ "init", - "--domain", TEST_DOMAIN, - "--port", &daemon_port2.to_string(), + "--domain", + TEST_DOMAIN, + "--port", + &daemon_port2.to_string(), ]) .env("DEVPROXY_CONFIG_DIR", &config_dir) .output()