Address code review: make wait_for_server async and clean up test

- Convert wait_for_server() to async using tokio::time and tokio::net::TcpStream
- Update all callers (start_server, start_server_with_bind, and tests) to async
- Clean up self_request_loop test: remove verbose comments and println
- Use tracing::debug for test debugging output
- Simplify test assertions to focus on essential behavior

Author: ammar-agent

tests/common/mod.rs

@@ -254,15 +254,21 @@ pub fn test_https_allow(use_sudo: bool) {
 
 // Wait until a TCP port on localhost is accepting connections
 // Returns true if the port became ready before max_wait elapsed
-pub fn wait_for_server(port: u16, max_wait: std::time::Duration) -> bool {
-    let start = std::time::Instant::now();
+pub async fn wait_for_server(port: u16, max_wait: std::time::Duration) -> bool {
+    let start = tokio::time::Instant::now();
+    let poll_interval = tokio::time::Duration::from_millis(75);
+    let settle_time = tokio::time::Duration::from_millis(200);
+
     while start.elapsed() < max_wait {
-        if std::net::TcpStream::connect(format!("127.0.0.1:{}", port)).is_ok() {
+        if tokio::net::TcpStream::connect(format!("127.0.0.1:{}", port))
+            .await
+            .is_ok()
+        {
             // Give the server a brief moment to finish initialization
-            std::thread::sleep(std::time::Duration::from_millis(200));
+            tokio::time::sleep(settle_time).await;
             return true;
         }
-        std::thread::sleep(std::time::Duration::from_millis(75));
+        tokio::time::sleep(poll_interval).await;
     }
     false
 }

tests/self_request_loop.rs

@@ -3,81 +3,57 @@ mod common;
 use std::net::TcpListener;
 use std::process::{Command, Stdio};
 use std::time::Duration;
+use tracing::debug;
 
-/// Test that requests to the proxy itself are blocked to prevent infinite loops.
-/// This reproduces issue #84: https://github.com/coder/httpjail/issues/84
-#[test]
-fn test_server_mode_self_request_loop_prevention() {
-    // Logging is auto-initialized via ctor in common::logging
-
-    // Find available ports for HTTP and HTTPS
+/// Test that requests to the proxy itself are blocked to prevent infinite loops (issue #84)
+#[tokio::test]
+async fn test_server_mode_self_request_loop_prevention() {
     let http_port = find_available_port();
     let https_port = find_available_port();
 
-    // Start httpjail in server mode
     let httpjail_path: &str = env!("CARGO_BIN_EXE_httpjail");
     let mut proxy_process = Command::new(httpjail_path)
         .env("HTTPJAIL_HTTP_BIND", format!("127.0.0.1:{}", http_port))
         .env("HTTPJAIL_HTTPS_BIND", format!("127.0.0.1:{}", https_port))
         .arg("--server")
         .arg("--js")
-        .arg("true") // Allow all requests
+        .arg("true")
         .stdin(Stdio::null())
         .stdout(Stdio::piped())
         .stderr(Stdio::piped())
         .spawn()
-        .expect("Failed to start httpjail in server mode");
+        .expect("Failed to start httpjail");
 
-    // Wait for proxy to start listening on the HTTP port
     assert!(
-        common::wait_for_server(http_port, Duration::from_secs(5)),
+        common::wait_for_server(http_port, Duration::from_secs(5)).await,
         "Server failed to start on port {}",
         http_port
     );
 
-    // Try to make a request to the proxy itself through the proxy
-    // This should NOT create an infinite loop
-    let curl_result = Command::new("curl")
+    let output = Command::new("curl")
         .arg("--max-time")
-        .arg("3") // 3 second timeout
+        .arg("3")
         .arg("--proxy")
         .arg(format!("http://127.0.0.1:{}", http_port))
         .arg(format!("http://127.0.0.1:{}/test", http_port))
-        .output();
+        .output()
+        .expect("Failed to execute curl");
 
-    // Kill the proxy server
     proxy_process.kill().ok();
     let _ = proxy_process.wait();
 
-    match curl_result {
-        Ok(output) => {
-            let stdout = String::from_utf8_lossy(&output.stdout);
-            let stderr = String::from_utf8_lossy(&output.stderr);
-            let exit_code = output.status.code().unwrap_or(-1);
-
-            println!("=== Curl exit code: {}", exit_code);
-            println!("=== Curl stdout: {}", stdout);
-            println!("=== Curl stderr: {}", stderr);
-
-            // The request should either:
-            // 1. Be blocked with a 403 (our fix - should contain "blocked by httpjail")
-            // 2. Fail with connection error (curl timeout/connection refused)
-            // 3. NOT succeed normally (which would indicate the loop happened but curl timed out)
+    let stdout = String::from_utf8_lossy(&output.stdout);
+    let stderr = String::from_utf8_lossy(&output.stderr);
 
-            // Without the fix, this creates an infinite loop that consumes resources
-            // With the fix, we should see "Request blocked by httpjail" in the output
+    debug!("curl exit code: {}", output.status.code().unwrap_or(-1));
+    debug!("curl stdout: {}", stdout);
+    debug!("curl stderr: {}", stderr);
 
-            // For now, just verify it doesn't succeed (this test documents the bug)
-            // After implementing the fix, we'll assert for the specific error message
-            if output.status.success() && !stdout.contains("Request blocked by httpjail") {
-                panic!("Request appeared to succeed - this may indicate a loop issue");
-            }
-        }
-        Err(e) => {
-            // If curl fails to execute, that's a test setup problem
-            panic!("Failed to execute curl: {}", e);
-        }
-    }
+    assert!(
+        stdout.contains("Loop detected"),
+        "Expected loop detection message, got: {}",
+        stdout
+    );
 }
 
 /// Find an available port for testing

tests/weak_integration.rs

@@ -173,7 +173,7 @@ fn test_weak_mode_appends_no_proxy() {
 }
 
 // Simple server start function - we know the ports we're setting
-fn start_server(http_port: u16, https_port: u16) -> Result<std::process::Child, String> {
+async fn start_server(http_port: u16, https_port: u16) -> Result<std::process::Child, String> {
     let httpjail_path: &str = env!("CARGO_BIN_EXE_httpjail");
 
     let mut cmd = Command::new(httpjail_path);
@@ -192,7 +192,7 @@ fn start_server(http_port: u16, https_port: u16) -> Result<std::process::Child,
         .map_err(|e| format!("Failed to start server: {}", e))?;
 
     // Wait for the server to start listening
-    if !common::wait_for_server(http_port, Duration::from_secs(5)) {
+    if !common::wait_for_server(http_port, Duration::from_secs(5)).await {
         return Err(format!("Server failed to start on port {}", http_port));
     }
 
@@ -250,13 +250,15 @@ fn verify_bind_address(port: u16, expected_ip: &str) -> bool {
     std::net::TcpStream::connect(format!("{}:{}", expected_ip, port)).is_ok()
 }
 
-#[test]
-fn test_server_mode() {
+#[tokio::test]
+async fn test_server_mode() {
     // Test server mode with specific ports
     let http_port = 19876;
     let https_port = 19877;
 
-    let mut server = start_server(http_port, https_port).expect("Failed to start server");
+    let mut server = start_server(http_port, https_port)
+        .await
+        .expect("Failed to start server");
 
     // Test HTTP proxy works
     match test_curl_through_proxy(http_port, https_port) {
@@ -278,7 +280,7 @@ fn test_server_mode() {
 }
 
 // Helper to start server with custom bind config
-fn start_server_with_bind(http_bind: &str, https_bind: &str) -> (std::process::Child, u16) {
+async fn start_server_with_bind(http_bind: &str, https_bind: &str) -> (std::process::Child, u16) {
     let httpjail_path: &str = env!("CARGO_BIN_EXE_httpjail");
 
     let mut child = Command::new(httpjail_path)
@@ -306,61 +308,61 @@ fn start_server_with_bind(http_bind: &str, https_bind: &str) -> (std::process::C
     };
 
     // Wait for server to bind
-    if !common::wait_for_server(expected_port, Duration::from_secs(3)) {
+    if !common::wait_for_server(expected_port, Duration::from_secs(3)).await {
         child.kill().ok();
         panic!("Server failed to bind to port {}", expected_port);
     }
 
     (child, expected_port)
 }
 
-#[test]
+#[tokio::test]
 #[serial]
-fn test_server_bind_defaults() {
-    let (mut server, port) = start_server_with_bind("", "");
+async fn test_server_bind_defaults() {
+    let (mut server, port) = start_server_with_bind("", "").await;
     assert_eq!(port, 8080, "Server should default to port 8080");
     server.kill().ok();
 }
 
-#[test]
+#[tokio::test]
 #[serial]
-fn test_server_bind_port_only() {
+async fn test_server_bind_port_only() {
     // Port-only should bind to all interfaces (0.0.0.0)
-    let (mut server, port) = start_server_with_bind("19882", "19883");
+    let (mut server, port) = start_server_with_bind("19882", "19883").await;
     assert_eq!(
         port, 19882,
         "Server should bind to specified port on all interfaces"
     );
     server.kill().ok();
 }
 
-#[test]
+#[tokio::test]
 #[serial]
-fn test_server_bind_colon_prefix_port() {
+async fn test_server_bind_colon_prefix_port() {
     // :port (Go-style) should bind to all interfaces (0.0.0.0)
-    let (mut server, port) = start_server_with_bind(":19892", ":19893");
+    let (mut server, port) = start_server_with_bind(":19892", ":19893").await;
     assert_eq!(
         port, 19892,
         "Server should bind to specified port on all interfaces with :port format"
     );
     server.kill().ok();
 }
 
-#[test]
+#[tokio::test]
 #[serial]
-fn test_server_bind_all_interfaces() {
-    let (mut server, port) = start_server_with_bind("0.0.0.0:19884", "0.0.0.0:19885");
+async fn test_server_bind_all_interfaces() {
+    let (mut server, port) = start_server_with_bind("0.0.0.0:19884", "0.0.0.0:19885").await;
     assert_eq!(
         port, 19884,
         "Server should bind to specified port on 0.0.0.0"
     );
     server.kill().ok();
 }
 
-#[test]
+#[tokio::test]
 #[serial]
-fn test_server_bind_ip_without_port() {
-    let (mut server, port) = start_server_with_bind("127.0.0.1", "127.0.0.1");
+async fn test_server_bind_ip_without_port() {
+    let (mut server, port) = start_server_with_bind("127.0.0.1", "127.0.0.1").await;
     assert_eq!(
         port, 8080,
         "Server should use default port 8080 when only IP specified"