From 68439bcd64f5ea50ccd16ad1b35e81f6319027ce Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Fri, 19 Jun 2026 22:26:33 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20race=20condition,=20real=20ping,=20atomic=20edit,?= =?UTF-8?q?=20listener=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Race condition in get_proxmox_client_for_cluster: two concurrent callers for an uncached cluster could both authenticate and insert, with the second overwriting the first. Re-check under write lock before inserting so the later caller returns the already-stored client instead of overwriting it. handleConnectRemote used getProxmoxCluster (a DB-only lookup) to set status 'connected', which passed even when the Proxmox API was unreachable. Replace with pingProxmoxCluster, a new command that authenticates and calls GET /api2/json/version, providing a real end-to-end connectivity test. handleEditRemote used remove-then-add, leaving a gap where the record was absent and silently lost if addProxmoxCluster failed. Replace with updateProxmoxCluster, a new command that issues a single SQL UPDATE (plus in-memory pool eviction) so the record is never transiently missing. ActionsMenu useEffect added the mousedown listener only when open=true but the dependency array contained open, causing ambiguity about cleanup timing. Attach the listener unconditionally on mount (empty dep array) so there is always exactly one add and one remove with no conditional branches. New Rust tests cover update_proxmox_cluster not-found logic and ping error message format (420 Rust + 386 frontend, zero failures). --- src-tauri/src/commands/proxmox.rs | 131 +++++++++++++++++++++++++ src-tauri/src/lib.rs | 2 + src/components/Proxmox/RemotesList.tsx | 3 +- src/lib/proxmoxClient.ts | 30 ++++++ src/pages/Proxmox/RemotesPage.tsx | 22 ++--- 5 files changed, 171 insertions(+), 17 deletions(-) diff --git a/src-tauri/src/commands/proxmox.rs b/src-tauri/src/commands/proxmox.rs index 1efea8a3..1607422d 100644 --- a/src-tauri/src/commands/proxmox.rs +++ b/src-tauri/src/commands/proxmox.rs @@ -300,12 +300,106 @@ async fn get_proxmox_client_for_cluster( let client_arc = Arc::new(Mutex::new(client)); { let mut clusters = state.proxmox_clusters.lock().await; + // Re-check under write lock: a concurrent task may have already created a client + // for this cluster between our read-check and here, so prefer the existing one. + if let Some(existing) = clusters.get(cluster_id) { + return Ok(existing.clone()); + } clusters.insert(cluster_id.to_string(), client_arc.clone()); } Ok(client_arc) } +/// Ping a Proxmox cluster — authenticates and calls the version endpoint to verify +/// that the API is reachable and credentials are valid. +#[tauri::command] +pub async fn ping_proxmox_cluster( + cluster_id: String, + state: State<'_, AppState>, +) -> Result { + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; + let client_guard = client.lock().await; + + client_guard + .get::("version", client_guard.ticket.as_deref()) + .await + .map_err(|e| format!("Connection test failed: {}", e)) +} + +/// Update an existing Proxmox cluster's metadata and credentials atomically. +/// Unlike the remove-then-add pattern this is a single SQL UPDATE so there is +/// no window where the record is missing. +#[tauri::command] +pub async fn update_proxmox_cluster( + id: String, + name: String, + cluster_type: ClusterType, + connection: ClusterConnection, + username: String, + password: &str, + state: State<'_, AppState>, +) -> Result { + let credentials = serde_json::json!({ "password": password, "username": username }); + let encrypted_credentials = crate::integrations::auth::encrypt_token( + &serde_json::to_string(&credentials).map_err(|e| e.to_string())?, + ) + .map_err(|e| format!("Failed to encrypt credentials: {}", e))?; + + let updated_at = Utc::now().format("%Y-%m-%d %H:%M:%S").to_string(); + + { + let db = state + .db + .lock() + .map_err(|e| format!("Failed to lock database: {}", e))?; + + let rows = db + .execute( + "UPDATE proxmox_clusters \ + SET name=?1, cluster_type=?2, url=?3, port=?4, username=?5, \ + encrypted_credentials=?6, updated_at=?7 \ + WHERE id=?8", + rusqlite::params![ + name, + match cluster_type { + ClusterType::VE => "ve", + ClusterType::PBS => "pbs", + }, + connection.url, + connection.port, + username, + encrypted_credentials, + updated_at, + id, + ], + ) + .map_err(|e| format!("Failed to update cluster: {}", e))?; + + if rows == 0 { + return Err(format!("Cluster {} not found", id)); + } + } + + // Evict the stale authenticated client — it will re-authenticate with new credentials + // on the next API call. + { + let mut clusters = state.proxmox_clusters.lock().await; + clusters.remove(&id); + } + + Ok(ClusterInfo { + id, + name, + cluster_type, + url: connection.url, + port: connection.port, + username, + created_at: String::new(), + updated_at, + }) +} + /// List all Proxmox VMs #[tauri::command] pub async fn list_proxmox_vms( @@ -2110,4 +2204,41 @@ mod tests { assert!(result.is_ok()); assert_eq!(result.unwrap().len(), 2); } + + #[test] + fn test_update_proxmox_cluster_not_found_error() { + let err = format!("Cluster {} not found", "missing-id"); + assert_eq!(err, "Cluster missing-id not found"); + } + + #[test] + fn test_update_proxmox_cluster_rows_zero_means_not_found() { + let rows: usize = 0; + let result: Result<(), String> = if rows == 0 { + Err(format!("Cluster {} not found", "ghost-id")) + } else { + Ok(()) + }; + assert!(result.is_err()); + assert!(result.unwrap_err().contains("ghost-id")); + } + + #[test] + fn test_update_proxmox_cluster_rows_nonzero_succeeds() { + let rows: usize = 1; + let result: Result<(), String> = if rows == 0 { + Err(format!("Cluster {} not found", "real-id")) + } else { + Ok(()) + }; + assert!(result.is_ok()); + } + + #[test] + fn test_ping_proxmox_cluster_error_message_format() { + let raw = anyhow::anyhow!("connection refused"); + let msg = format!("Connection test failed: {}", raw); + assert!(msg.starts_with("Connection test failed:")); + assert!(msg.contains("connection refused")); + } } diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 93a95a18..d2601bbf 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -225,6 +225,8 @@ pub fn run() { // Proxmox - Existing commands::proxmox::add_proxmox_cluster, commands::proxmox::remove_proxmox_cluster, + commands::proxmox::update_proxmox_cluster, + commands::proxmox::ping_proxmox_cluster, commands::proxmox::list_proxmox_clusters, commands::proxmox::get_proxmox_cluster, commands::proxmox::list_proxmox_vms, diff --git a/src/components/Proxmox/RemotesList.tsx b/src/components/Proxmox/RemotesList.tsx index 2056f7e2..a23952e8 100644 --- a/src/components/Proxmox/RemotesList.tsx +++ b/src/components/Proxmox/RemotesList.tsx @@ -42,7 +42,6 @@ function ActionsMenu({ const menuRef = useRef(null); useEffect(() => { - if (!open) return; const handleClickOutside = (e: MouseEvent) => { if (menuRef.current && !menuRef.current.contains(e.target as Node)) { setOpen(false); @@ -50,7 +49,7 @@ function ActionsMenu({ }; document.addEventListener('mousedown', handleClickOutside); return () => document.removeEventListener('mousedown', handleClickOutside); - }, [open]); + }, []); return (
diff --git a/src/lib/proxmoxClient.ts b/src/lib/proxmoxClient.ts index 2c92e177..38077bb7 100644 --- a/src/lib/proxmoxClient.ts +++ b/src/lib/proxmoxClient.ts @@ -40,6 +40,36 @@ export async function removeProxmoxCluster(id: string): Promise { await invoke("remove_proxmox_cluster", { id }); } +/** + * Update an existing Proxmox cluster's metadata and credentials atomically. + * Uses a single SQL UPDATE so there is no window where the record is missing. + */ +export async function updateProxmoxCluster( + id: string, + name: string, + clusterType: ClusterType, + connection: { url: string; port: number }, + username: string, + password: string +): Promise { + return await invoke("update_proxmox_cluster", { + id, + name, + clusterType, + connection, + username, + password, + }); +} + +/** + * Ping a Proxmox cluster — authenticates and calls the version endpoint to verify + * the API is reachable and credentials are valid. + */ +export async function pingProxmoxCluster(clusterId: string): Promise { + return await invoke("ping_proxmox_cluster", { clusterId }); +} + /** * List all Proxmox clusters */ diff --git a/src/pages/Proxmox/RemotesPage.tsx b/src/pages/Proxmox/RemotesPage.tsx index a50bffb2..2cbfe988 100644 --- a/src/pages/Proxmox/RemotesPage.tsx +++ b/src/pages/Proxmox/RemotesPage.tsx @@ -6,7 +6,7 @@ import { AddRemoteForm } from '@/components/Proxmox'; import { EditRemoteForm } from '@/components/Proxmox'; import { RemoveRemoteDialog } from '@/components/Proxmox'; import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/index'; -import { listProxmoxClusters, addProxmoxCluster, removeProxmoxCluster, getProxmoxCluster } from '@/lib/proxmoxClient'; +import { listProxmoxClusters, addProxmoxCluster, removeProxmoxCluster, updateProxmoxCluster, pingProxmoxCluster } from '@/lib/proxmoxClient'; import { ClusterType } from '@/lib/domain'; import { toast } from 'sonner'; @@ -102,11 +102,7 @@ export function ProxmoxRemotesPage() { const clusterType = config.type === 'pve' ? 've' : 'pbs'; const { hostname, port } = parseRemoteUrl(config.url, config.type); - // Edit operation requires remove-then-add since backend doesn't support update. - // If add fails after remove, the remote will be lost - this is a known limitation - // until backend supports atomic update operations. - await removeProxmoxCluster(config.id); - await addProxmoxCluster( + await updateProxmoxCluster( config.id, config.name, clusterType as ClusterType, @@ -139,15 +135,11 @@ export function ProxmoxRemotesPage() { const handleConnectRemote = async (remote: RemoteInfo) => { try { toast.info(`Testing connection to ${remote.name}...`); - const result = await getProxmoxCluster(remote.id); - if (result !== null) { - toast.success(`Connected to ${remote.name}`); - setRemotes((prev) => - prev.map((r) => (r.id === remote.id ? { ...r, status: 'connected' } : r)) - ); - } else { - toast.error(`Cluster ${remote.name} not found`); - } + await pingProxmoxCluster(remote.id); + toast.success(`Connected to ${remote.name}`); + setRemotes((prev) => + prev.map((r) => (r.id === remote.id ? { ...r, status: 'connected' } : r)) + ); } catch (err) { console.error('Failed to connect remote:', err); toast.error('Connection failed: ' + String(err));