fix: prevent Docker network resource exhaustion with improved cleanup (#41)

Author: ammario

.claude/commands/pr-compress.md

@@ -0,0 +1,5 @@
+Deeply review existing change.
+
+Try to find opportunities for DRY. i.e. where you can push the net balance of code additions as low as possible while simultaneously improving readability and behavior.
+
+Do not venture into changes beyond the scope of the PR.

src/jail/linux/docker.rs

@@ -12,19 +12,51 @@ struct DockerNetwork {
     network_name: String,
 }
 
+impl DockerNetwork {
+    const NETWORK_PREFIX: &'static str = "httpjail_";
+
+    /// Generate network name from jail ID
+    fn network_name_from_jail_id(jail_id: &str) -> String {
+        format!("{}{}", Self::NETWORK_PREFIX, jail_id)
+    }
+
+    /// Extract jail ID from network name
+    fn jail_id_from_network_name(network_name: &str) -> Option<&str> {
+        network_name.strip_prefix(Self::NETWORK_PREFIX)
+    }
+
+    /// Check if a Docker command failed due to resource not existing
+    fn is_not_found_error(stderr: &str) -> bool {
+        stderr.contains("not found")
+            || stderr.contains("No such")
+            || stderr.contains("does not exist")
+    }
+
+    /// Check if a Docker command failed due to resource already existing
+    fn is_already_exists_error(stderr: &str) -> bool {
+        stderr.contains("already exists")
+    }
+}
+
 /// Docker routing nftables resource that gets cleaned up on drop
 struct DockerRoutingTable {
     #[allow(dead_code)]
     jail_id: String,
     table_name: String,
 }
 
+impl DockerRoutingTable {
+    /// Generate table name from jail ID
+    fn table_name_from_jail_id(jail_id: &str) -> String {
+        format!("httpjail_docker_{}", jail_id)
+    }
+}
+
 impl SystemResource for DockerRoutingTable {
     fn create(jail_id: &str) -> Result<Self> {
-        let table_name = format!("httpjail_docker_{}", jail_id);
         Ok(Self {
             jail_id: jail_id.to_string(),
-            table_name,
+            table_name: Self::table_name_from_jail_id(jail_id),
         })
     }
 
@@ -38,10 +70,10 @@ impl SystemResource for DockerRoutingTable {
 
         if !output.status.success() {
             let stderr = String::from_utf8_lossy(&output.stderr);
-            if !stderr.contains("No such file or directory") && !stderr.contains("does not exist") {
-                warn!("Failed to delete Docker routing table: {}", stderr);
-            } else {
+            if DockerNetwork::is_not_found_error(&stderr) {
                 debug!("Docker routing table {} already removed", self.table_name);
+            } else {
+                warn!("Failed to delete Docker routing table: {}", stderr);
             }
         } else {
             info!("Removed Docker routing table {}", self.table_name);
@@ -51,25 +83,16 @@ impl SystemResource for DockerRoutingTable {
     }
 
     fn for_existing(jail_id: &str) -> Self {
-        let table_name = format!("httpjail_docker_{}", jail_id);
         Self {
             jail_id: jail_id.to_string(),
-            table_name,
+            table_name: Self::table_name_from_jail_id(jail_id),
         }
     }
 }
 
-impl DockerNetwork {
-    #[allow(dead_code)]
-    fn new(jail_id: &str) -> Result<Self> {
-        let network_name = format!("httpjail_{}", jail_id);
-        Ok(Self { network_name })
-    }
-}
-
 impl SystemResource for DockerNetwork {
     fn create(jail_id: &str) -> Result<Self> {
-        let network_name = format!("httpjail_{}", jail_id);
+        let network_name = Self::network_name_from_jail_id(jail_id);
 
         // Create Docker network with no default gateway (isolated)
         // Using a /24 subnet in the 172.20.x.x range
@@ -92,7 +115,7 @@ impl SystemResource for DockerNetwork {
 
         if !output.status.success() {
             let stderr = String::from_utf8_lossy(&output.stderr);
-            if stderr.contains("already exists") {
+            if Self::is_already_exists_error(&stderr) {
                 info!("Docker network {} already exists", network_name);
             } else {
                 anyhow::bail!("Failed to create Docker network: {}", stderr);
@@ -117,7 +140,7 @@ impl SystemResource for DockerNetwork {
 
         if !output.status.success() {
             let stderr = String::from_utf8_lossy(&output.stderr);
-            if stderr.contains("not found") {
+            if Self::is_not_found_error(&stderr) {
                 debug!("Docker network {} already removed", self.network_name);
             } else {
                 warn!("Failed to remove Docker network: {}", stderr);
@@ -130,8 +153,9 @@ impl SystemResource for DockerNetwork {
     }
 
     fn for_existing(jail_id: &str) -> Self {
-        let network_name = format!("httpjail_{}", jail_id);
-        Self { network_name }
+        Self {
+            network_name: Self::network_name_from_jail_id(jail_id),
+        }
     }
 }
 
@@ -202,47 +226,76 @@ impl DockerLinux {
         })
     }
 
+    /// Clean up all orphaned Docker networks that don't have corresponding canary files
+    fn cleanup_all_orphaned_docker_networks() -> Result<()> {
+        debug!("Scanning for orphaned Docker networks");
+
+        // List all Docker networks
+        let output = Command::new("docker")
+            .args(["network", "ls", "--format", "{{.Name}}"])
+            .output()
+            .context("Failed to list Docker networks")?;
+
+        if !output.status.success() {
+            warn!("Failed to list Docker networks for cleanup");
+            return Ok(());
+        }
+
+        let networks = String::from_utf8_lossy(&output.stdout);
+        let canary_dir = crate::jail::get_canary_dir();
+
+        for network_name in networks.lines() {
+            // Extract jail_id from network name (skip non-httpjail networks)
+            let Some(jail_id) = DockerNetwork::jail_id_from_network_name(network_name) else {
+                continue;
+            };
+
+            // Check if canary file exists for this jail
+            let canary_path = canary_dir.join(jail_id);
+            if !canary_path.exists() {
+                info!(
+                    "Found orphaned Docker network {} without canary, removing",
+                    network_name
+                );
+
+                // Remove the orphaned network
+                let rm_output = Command::new("docker")
+                    .args(["network", "rm", network_name])
+                    .output()
+                    .context("Failed to remove orphaned Docker network")?;
+
+                if !rm_output.status.success() {
+                    let stderr = String::from_utf8_lossy(&rm_output.stderr);
+                    if !DockerNetwork::is_not_found_error(&stderr) {
+                        warn!(
+                            "Failed to remove orphaned Docker network {}: {}",
+                            network_name, stderr
+                        );
+                    }
+                }
+            }
+        }
+
+        Ok(())
+    }
+
+    /// Docker flags that take a value as the next argument
+    const FLAGS_WITH_VALUES: &'static [&'static str] =
+        &["-e", "-v", "-p", "--name", "--entrypoint", "-w", "--user"];
+
     /// Build the docker command with isolated network
     fn build_docker_command(
         &self,
         docker_args: &[String],
         extra_env: &[(String, String)],
     ) -> Result<Command> {
-        let network_name = format!("httpjail_{}", self.config.jail_id);
+        let network_name = DockerNetwork::network_name_from_jail_id(&self.config.jail_id);
         // Parse docker arguments to filter out conflicting options and find the image
         let modified_args = Self::filter_network_args(docker_args);
 
         // Find where the image name is in the args
-        let mut image_idx = None;
-        let mut skip_next = false;
-
-        for (i, arg) in modified_args.iter().enumerate() {
-            if skip_next {
-                skip_next = false;
-                continue;
-            }
-
-            // Skip known flags that take values
-            if arg == "-e"
-                || arg == "-v"
-                || arg == "-p"
-                || arg == "--name"
-                || arg == "--entrypoint"
-                || arg == "-w"
-                || arg == "--user"
-            {
-                skip_next = true;
-                continue;
-            }
-
-            // If it doesn't start with -, it's likely the image
-            if !arg.starts_with('-') {
-                image_idx = Some(i);
-                break;
-            }
-        }
-
-        let image_idx = image_idx.context("Could not find Docker image in arguments")?;
+        let image_idx = Self::find_image_index(&modified_args)
+            .context("Could not find Docker image in arguments")?;
 
         // Split args into: docker options, image, and command
         let docker_opts = &modified_args[..image_idx];
@@ -302,6 +355,31 @@ impl DockerLinux {
         Ok(cmd)
     }
 
+    /// Find the index of the Docker image in the arguments
+    fn find_image_index(args: &[String]) -> Option<usize> {
+        let mut skip_next = false;
+
+        for (i, arg) in args.iter().enumerate() {
+            if skip_next {
+                skip_next = false;
+                continue;
+            }
+
+            // Skip known flags that take values
+            if Self::FLAGS_WITH_VALUES.contains(&arg.as_str()) {
+                skip_next = true;
+                continue;
+            }
+
+            // If it doesn't start with -, it's likely the image
+            if !arg.starts_with('-') {
+                return Some(i);
+            }
+        }
+
+        None
+    }
+
     /// Filter out any existing --network arguments from docker args
     fn filter_network_args(docker_args: &[String]) -> Vec<String> {
         let mut modified_args = Vec::new();
@@ -347,7 +425,7 @@ impl DockerLinux {
             // Add nftables rules to:
             // 1. Allow traffic from Docker network to jail's proxy ports
             // 2. DNAT HTTP/HTTPS traffic to the proxy
-            let table_name = format!("httpjail_docker_{}", self.config.jail_id);
+            let table_name = DockerRoutingTable::table_name_from_jail_id(&self.config.jail_id);
 
             // Create nftables rules
             let nft_rules = format!(
@@ -412,6 +490,10 @@ impl DockerLinux {
 
 impl Jail for DockerLinux {
     fn setup(&mut self, proxy_port: u16) -> Result<()> {
+        // Clean up any orphaned Docker networks first
+        // This handles cases where Docker networks exist without corresponding canary files
+        Self::cleanup_all_orphaned_docker_networks()?;
+
         // First setup the inner Linux jail
         self.inner_jail.setup(proxy_port)?;
 

src/jail/managed.rs

@@ -225,12 +225,23 @@ impl<J: Jail> Jail for ManagedJail<J> {
         // Cleanup orphans first
         if self.enable_heartbeat {
             self.cleanup_orphans()?;
+
+            // Create canary BEFORE setting up jail to prevent race condition
+            // where another jail's cleanup might delete our network
+            self.create_canary()?;
         }
 
-        // Setup the inner jail
-        self.jail.setup(proxy_port)?;
+        // Setup the inner jail (which may create Docker networks)
+        let setup_result = self.jail.setup(proxy_port);
+
+        // If setup failed, clean up the canary we created
+        if setup_result.is_err() && self.enable_heartbeat {
+            let _ = self.delete_canary();
+            return setup_result;
+        }
 
-        // Start heartbeat after successful setup
+        // Start heartbeat thread after successful setup
+        // Note: This will try to create canary again but create_canary is idempotent
         self.start_heartbeat()?;
 
         Ok(())
@@ -272,8 +283,20 @@ impl<J: Jail> Drop for ManagedJail<J> {
     fn drop(&mut self) {
         // Best effort cleanup
         let _ = self.stop_heartbeat();
-        if self.enable_heartbeat {
+
+        // Explicitly cleanup jail resources BEFORE deleting canary
+        // This ensures resources are freed before we signal that the jail is gone
+        let cleanup_result = self.jail.cleanup();
+
+        // Only delete canary if cleanup succeeded
+        // If cleanup failed, leave the canary so orphan cleanup can retry later
+        if self.enable_heartbeat && cleanup_result.is_ok() {
             let _ = self.delete_canary();
+        } else if cleanup_result.is_err() {
+            error!(
+                "Failed to cleanup jail '{}', leaving canary for orphan cleanup",
+                self.jail.jail_id()
+            );
         }
     }
 }

src/jail/mod.rs

@@ -51,7 +51,14 @@ pub trait Jail: Send + Sync {
 
 /// Get the canary directory for tracking jail lifetimes
 pub fn get_canary_dir() -> std::path::PathBuf {
-    std::path::PathBuf::from("/tmp/httpjail")
+    // Use user data directory instead of /tmp to avoid issues with tmp cleaners
+    // This ensures canaries persist across reboots and are only removed when we want them to be
+    if let Some(data_dir) = dirs::data_dir() {
+        data_dir.join("httpjail").join("canaries")
+    } else {
+        // Fallback to /tmp if we can't get user data dir (should rarely happen)
+        std::path::PathBuf::from("/tmp/httpjail")
+    }
 }
 
 /// Get the directory for httpjail temporary files (like resolv.conf)