From 2134b880b9c311122b5f93f6856ba13e1586be78 Mon Sep 17 00:00:00 2001
From: Shaun Arman <shaun.arman@motorolasolutions.com>
Date: Sat, 6 Jun 2026 18:40:52 -0500
Subject: [PATCH] fix: address automated PR review findings

- Temp file cleanup: move TempFileCleanup struct before std::fs::write
  to ensure cleanup happens even on panic (race condition fix)
- PortForwardSession: add explicit close() method for async cleanup
  of child process and temp files
- delete_port_forward: call close() before removing session
- All temp file operations now use RAII pattern with cleanup before write
---
 src-tauri/src/commands/kube.rs    | 41 +++++++++++++++++--------------
 src-tauri/src/kube/portforward.rs | 16 ++++++++++++
 2 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/src-tauri/src/commands/kube.rs b/src-tauri/src/commands/kube.rs
index 0f9818f7..ecf904c1 100644
--- a/src-tauri/src/commands/kube.rs
+++ b/src-tauri/src/commands/kube.rs
@@ -199,14 +199,11 @@ pub async fn test_cluster_connection(
     let kubeconfig_content = cluster.kubeconfig_content.as_ref();
     let context = &cluster.context;
 
-    // Write kubeconfig to temp file
+    // Write kubeconfig to temp file and ensure cleanup even on panic
     let temp_dir = std::env::temp_dir();
     let temp_path = temp_dir.join(format!("kubeconfig-{}.yaml", cluster_id));
-
-    std::fs::write(&temp_path, kubeconfig_content)
-        .map_err(|e| format!("Failed to write kubeconfig temp file: {e}"))?;
-
-    // Cleanup temp file on drop
+    
+    // Create cleanup struct BEFORE writing - ensures cleanup happens even on panic
     struct TempFileCleanup(std::path::PathBuf);
     impl Drop for TempFileCleanup {
         fn drop(&mut self) {
@@ -215,6 +212,9 @@ pub async fn test_cluster_connection(
     }
     let _cleanup = TempFileCleanup(temp_path.clone());
 
+    std::fs::write(&temp_path, kubeconfig_content)
+        .map_err(|e| format!("Failed to write kubeconfig temp file: {e}"))?;
+
     // Run kubectl cluster-info
     let kubectl_path = locate_kubectl()?;
 
@@ -255,14 +255,11 @@ pub async fn discover_pods(
     let kubeconfig_content = cluster.kubeconfig_content.as_ref();
     let context = &cluster.context;
 
-    // Write kubeconfig to temp file
+    // Write kubeconfig to temp file and ensure cleanup even on panic
     let temp_dir = std::env::temp_dir();
     let temp_path = temp_dir.join(format!("kubeconfig-{}-pods.yaml", cluster_id));
-
-    std::fs::write(&temp_path, kubeconfig_content)
-        .map_err(|e| format!("Failed to write kubeconfig temp file: {e}"))?;
-
-    // Cleanup temp file on drop
+    
+    // Create cleanup struct BEFORE writing - ensures cleanup happens even on panic
     struct TempFileCleanup(std::path::PathBuf);
     impl Drop for TempFileCleanup {
         fn drop(&mut self) {
@@ -271,6 +268,9 @@ pub async fn discover_pods(
     }
     let _cleanup = TempFileCleanup(temp_path.clone());
 
+    std::fs::write(&temp_path, kubeconfig_content)
+        .map_err(|e| format!("Failed to write kubeconfig temp file: {e}"))?;
+
     // Run kubectl get pods with full JSON output
     let kubectl_path = locate_kubectl()?;
 
@@ -472,14 +472,11 @@ pub async fn start_port_forward(
         "Allocating local port for port-forward"
     );
 
-    // Write kubeconfig to temp file
+    // Write kubeconfig to temp file and ensure cleanup even on panic
     let temp_dir = std::env::temp_dir();
     let temp_path = temp_dir.join(format!("kubeconfig-{}.yaml", request.cluster_id));
-
-    std::fs::write(&temp_path, kubeconfig_content.as_ref())
-        .map_err(|e| format!("Failed to write kubeconfig temp file: {e}"))?;
-
-    // Cleanup temp file on drop
+    
+    // Create cleanup struct BEFORE writing - ensures cleanup happens even on panic
     struct TempFileCleanup(std::path::PathBuf);
     impl Drop for TempFileCleanup {
         fn drop(&mut self) {
@@ -488,6 +485,9 @@ pub async fn start_port_forward(
     }
     let _cleanup = TempFileCleanup(temp_path.clone());
 
+    std::fs::write(&temp_path, kubeconfig_content.as_ref())
+        .map_err(|e| format!("Failed to write kubeconfig temp file: {e}"))?;
+
     // Build kubectl command
     let kubectl_path = locate_kubectl()?;
     let args = vec![
@@ -593,7 +593,10 @@ pub async fn list_port_forwards(
 pub async fn delete_port_forward(id: String, state: State<'_, AppState>) -> Result<(), String> {
     let mut port_forwards = state.port_forwards.lock().await;
 
-    if port_forwards.remove(&id).is_none() {
+    if let Some(mut session) = port_forwards.remove(&id) {
+        // Close the session to kill the child and clean up temp files
+        session.close().await;
+    } else {
         return Err(format!("Port forward session {id} not found"));
     }
 
diff --git a/src-tauri/src/kube/portforward.rs b/src-tauri/src/kube/portforward.rs
index 23c34f5f..86556c1d 100644
--- a/src-tauri/src/kube/portforward.rs
+++ b/src-tauri/src/kube/portforward.rs
@@ -140,6 +140,22 @@ impl PortForwardSession {
         }
     }
 
+    pub async fn close(&mut self) {
+        // Kill the child process if it exists
+        if let Some(ref child_wait_handle) = self.child_wait_handle {
+            let guard = child_wait_handle.lock().await;
+            let child_opt = guard.child.lock().await.take();
+            if let Some(mut child) = child_opt {
+                let _ = child.kill().await;
+            }
+        }
+
+        // Clean up temp kubeconfig file if it exists
+        if let Some(path) = &self.temp_kubeconfig_path {
+            let _ = std::fs::remove_file(path);
+        }
+    }
+
     pub fn set_error(&mut self, error: String) {
         self.status = PortForwardStatus::Error(error.clone());
         self.error_message = Some(error);