diff --git a/src/ping.rs b/src/ping.rs index 4f71206..c3e92be 100644 --- a/src/ping.rs +++ b/src/ping.rs @@ -6,31 +6,37 @@ use tokio::time::timeout; type Result = std::result::Result>; -/// Pings an IP address using the system ping command and returns the round-trip time. +/// Pings an IP address with multiple attempts for reliability. /// -/// This implementation uses the system `ping` binary which is typically setuid root, -/// allowing ICMP operations without requiring CAP_NET_RAW or elevated privileges -/// for the application itself. +/// This function sends multiple ping packets and tolerates some packet loss, +/// making it more resilient to temporary network issues. It only fails if +/// all packets are lost. /// /// # Arguments /// /// * `ip` - The IP address to ping -/// * `timeout_duration` - Maximum time to wait for a response +/// * `timeout_duration` - Maximum time to wait for responses +/// * `count` - Number of ping packets to send (default: 3) /// /// # Returns /// -/// * `Ok(Duration)` - The round-trip time if successful -/// * `Err(anyhow::Error)` - If the ping fails or times out -pub async fn ping(ip: IpAddr, timeout_duration: Duration) -> Result { +/// * `Ok(Duration)` - The average round-trip time if at least one packet succeeds +/// * `Err(anyhow::Error)` - If all pings fail or 100% packet loss +pub async fn ping_with_retries( + ip: IpAddr, + timeout_duration: Duration, + count: u32, +) -> Result { let ip_str = ip.to_string(); let timeout_secs = timeout_duration.as_secs().max(1); + let packet_count = count.max(1).to_string(); // Build ping command arguments based on OS let args = if cfg!(target_os = "macos") { // macOS: -W is timeout in ms vec![ "-c".to_string(), - "1".to_string(), + packet_count, "-W".to_string(), (timeout_secs * 1000).to_string(), ip_str.clone(), @@ -39,15 +45,16 @@ pub async fn ping(ip: IpAddr, timeout_duration: Duration) -> Result { // Linux: -W is timeout in seconds vec![ "-c".to_string(), - "1".to_string(), + packet_count, "-W".to_string(), timeout_secs.to_string(), ip_str.clone(), ] }; - // Execute ping command with timeout - let result = timeout(timeout_duration + Duration::from_secs(1), async { + // Execute ping command with timeout (extra time for multiple packets) + let total_timeout = timeout_duration * count + Duration::from_secs(2); + let result = timeout(total_timeout, async { let output = Command::new("ping") .args(&args) .stdout(Stdio::piped()) @@ -55,12 +62,18 @@ pub async fn ping(ip: IpAddr, timeout_duration: Duration) -> Result { .output() .await?; - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); + let stdout = String::from_utf8_lossy(&output.stdout); + + // Check for 100% packet loss - this is a hard failure + if stdout.contains("100% packet loss") || stdout.contains("100.0% packet loss") { + return Err(format!("All {} ping packets lost to {}", count, ip_str).into()); + } + + // If we got any successful packets, parse the average RTT + if output.status.success() || !stdout.is_empty() { parse_ping_output(&stdout) } else { let stderr = String::from_utf8_lossy(&output.stderr); - let stdout = String::from_utf8_lossy(&output.stdout); Err(format!( "Ping failed for {}: {}{}", ip_str, @@ -74,7 +87,7 @@ pub async fn ping(ip: IpAddr, timeout_duration: Duration) -> Result { match result { Ok(inner_result) => inner_result, - Err(_) => Err(format!("Ping timeout for {}", ip_str).into()), + Err(_) => Err(format!("Ping timeout for {} after {} attempts", ip_str, count).into()), } } @@ -201,27 +214,46 @@ rtt min/avg/max/mdev = 0.543/0.543/0.543/0.000 ms"#; } #[tokio::test] - async fn test_ping_localhost() { - // This test actually pings localhost - skip if no network + async fn test_ping_with_retries_localhost() { + // Test multi-packet ping to localhost let ip: IpAddr = "127.0.0.1".parse().unwrap(); - let result = ping(ip, Duration::from_secs(5)).await; + let result = ping_with_retries(ip, Duration::from_secs(5), 3).await; // Localhost ping should succeed on most systems if result.is_ok() { let duration = result.unwrap(); - // Localhost should respond in < 100ms + // Localhost should respond in < 100ms on average assert!(duration.as_millis() < 100); } // If it fails, that's okay too - some systems don't allow ping to localhost } #[tokio::test] - async fn test_ping_invalid_ip() { + async fn test_ping_with_retries_invalid_ip() { // This should fail - non-routable IP let ip: IpAddr = "192.0.2.1".parse().unwrap(); // TEST-NET-1, not routable - let result = ping(ip, Duration::from_secs(2)).await; + let result = ping_with_retries(ip, Duration::from_secs(2), 3).await; - // Should either timeout or fail + // Should fail with 100% packet loss assert!(result.is_err()); } + + #[test] + fn test_parse_ping_output_with_packet_loss() { + // Test output with some packet loss but not 100% + let output = r#"PING 192.168.1.1 (192.168.1.1): 56 data bytes +64 bytes from 192.168.1.1: icmp_seq=0 ttl=64 time=1.234 ms +Request timeout for icmp_seq 1 + +--- 192.168.1.1 ping statistics --- +3 packets transmitted, 1 packets received, 66.7% packet loss +round-trip min/avg/max/stddev = 1.234/1.234/1.234/0.000 ms"#; + + let result = parse_ping_output(output); + assert!(result.is_ok()); + let duration = result.unwrap(); + // Should still parse the average from the one successful packet + assert!(duration.as_secs_f64() > 0.001); + assert!(duration.as_secs_f64() < 0.002); + } } diff --git a/src/websocket_client.rs b/src/websocket_client.rs index cf51cc4..3de5d39 100644 --- a/src/websocket_client.rs +++ b/src/websocket_client.rs @@ -21,7 +21,7 @@ const CONNECTION_TIMEOUT: Duration = Duration::from_secs(30); type Result = std::result::Result>; -use crate::ping::ping; +use crate::ping::ping_with_retries; use crate::proto::agent::{ AgentHeartbeat, AgentJob, AgentJobList, JobType, MonitoringCheck, QueryType, SnmpResult, }; @@ -666,7 +666,8 @@ async fn run_monitoring_task( let timeout = Duration::from_secs(5); - match ping(ip_addr, timeout).await { + // Send 3 pings for reliability - only fail if all 3 fail + match ping_with_retries(ip_addr, timeout, 3).await { Ok(rtt) => { let check = MonitoringCheck { device_id: device_id.clone(),