From df23424a85628c1a5f4b430b6036086f157890f8 Mon Sep 17 00:00:00 2001 From: Andrea Debernardi Date: Wed, 1 Jul 2026 13:55:44 +0200 Subject: [PATCH 1/2] fix(ssh): tear down stale tunnels so reconnect works after server reboot (#406) SSH tunnels were cached in the global TUNNELS map but never stopped or removed, so a tunnel outlived the connection that owned it. When the remote host rebooted the tunnel died while still holding its local forward port; the next reconnect reused the stale map entry and failed with "port already in use". Only restarting the app cleared the map. - ssh_tunnel: reap the killed system-ssh child in stop(); add is_alive() and remove_tunnel() helpers. - commands: skip and discard dead tunnels on reuse; tear down the tunnel in disconnect_connection. - health_check: tear down the tunnel when a connection exceeds the failure threshold (the path triggered by a server reboot). --- src-tauri/src/commands.rs | 56 ++++++++++++++++--- src-tauri/src/health_check.rs | 6 ++ src-tauri/src/ssh_tunnel.rs | 102 ++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index c032bcfe0..f92227d49 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -220,6 +220,27 @@ fn build_tunnel_map_key( crate::ssh_tunnel::build_tunnel_key(ssh_user, ssh_host, ssh_port, remote_host, remote_port) } +/// Stop and remove the SSH tunnel associated with these params, if any. +/// +/// Called when a connection is closed (manually or by the health check) so a +/// tunnel does not outlive the connection that owns it. Expects params that +/// still carry the original SSH/remote fields (i.e. before +/// [`resolve_connection_params`] rewrites host/port to the local forward). +pub(crate) fn teardown_ssh_tunnel(params: &ConnectionParams) { + if !params.ssh_enabled.unwrap_or(false) { + return; + } + let (Some(ssh_host), Some(ssh_user)) = (params.ssh_host.as_deref(), params.ssh_user.as_deref()) + else { + return; + }; + let ssh_port = params.ssh_port.unwrap_or(22); + let remote_host = params.host.as_deref().unwrap_or("localhost"); + let remote_port = params.port.unwrap_or(DEFAULT_MYSQL_PORT); + let map_key = build_tunnel_map_key(ssh_user, ssh_host, ssh_port, remote_host, remote_port); + crate::ssh_tunnel::remove_tunnel(&map_key); +} + /// Resolve K8s tunnel params synchronously (no saved-connection lookup; uses inline fields only). fn resolve_k8s_params(params: &ConnectionParams) -> Result { let context = params @@ -311,15 +332,29 @@ pub fn resolve_connection_params(params: &ConnectionParams) -> Result { + log::debug!("Reusing existing SSH tunnel on port {}", tunnel.local_port); + let mut new_params = params.clone(); + new_params.host = Some("127.0.0.1".to_string()); + new_params.port = Some(tunnel.local_port); + return Ok(new_params); + } + Some(_) => true, + None => false, + } + }; + if stale { + log::warn!("Discarding dead SSH tunnel for {}, recreating", map_key); + crate::ssh_tunnel::remove_tunnel(&map_key); } } @@ -3862,6 +3897,11 @@ pub async fn disconnect_connection( // Close the connection pool crate::pool_manager::close_pool_with_id(¶ms, Some(&connection_id)).await; + // Tear down the SSH tunnel (if any) so it does not linger holding its + // local port after the connection is gone. `expanded_params` still carries + // the original SSH/remote fields (before resolve rewrites host/port). + teardown_ssh_tunnel(&expanded_params); + log::info!( "Successfully disconnected from connection: {}", connection_id diff --git a/src-tauri/src/health_check.rs b/src-tauri/src/health_check.rs index b1e20d0a9..a6b0856a4 100644 --- a/src-tauri/src/health_check.rs +++ b/src-tauri/src/health_check.rs @@ -173,6 +173,12 @@ async fn handle_connection_failure(app: &tauri::AppHandle, connection_id: &str, if let Ok(expanded) = crate::commands::expand_ssh_connection_params(app, &saved_conn.params).await { + // Tear down the SSH tunnel before it is reused: after the remote + // host dies the tunnel is dead but still holds its local port, + // which breaks the next reconnect ("port already in use"). + // `expanded` still carries the original SSH/remote fields. + crate::commands::teardown_ssh_tunnel(&expanded); + let expanded = crate::commands::expand_k8s_connection_params(app, &expanded).await; if let Ok(params) = expanded.and_then(|params| { crate::commands::resolve_connection_params_with_id(¶ms, connection_id) diff --git a/src-tauri/src/ssh_tunnel.rs b/src-tauri/src/ssh_tunnel.rs index 1913a1e60..0acc03ed9 100644 --- a/src-tauri/src/ssh_tunnel.rs +++ b/src-tauri/src/ssh_tunnel.rs @@ -529,10 +529,50 @@ impl SshTunnel { TunnelBackend::SystemSsh(child) => { if let Ok(mut c) = child.lock() { let _ = c.kill(); + // Reap the process so it does not linger as a zombie + // holding the forwarded local port. + let _ = c.wait(); } } } } + + /// Report whether the tunnel is still usable. + /// + /// For a system-ssh backend this detects a process that has already + /// exited (e.g. the ssh client tore down after the remote host rebooted), + /// which would otherwise leave a stale entry in the tunnel map pointing at + /// a dead local port. The russh backend keeps its local listener bound for + /// as long as it has not been explicitly stopped, so it is considered + /// alive until `stop()` flips the flag. + pub fn is_alive(&self) -> bool { + match &self.backend { + TunnelBackend::Russh(running) => running.load(Ordering::Relaxed), + TunnelBackend::SystemSsh(child) => match child.lock() { + // `Ok(None)` means the child is still running. + Ok(mut c) => matches!(c.try_wait(), Ok(None)), + Err(_) => false, + }, + } + } +} + +/// Stop and remove a tunnel from the global map, if present. +/// +/// Tears down the underlying ssh process / forwarding thread and frees the +/// local port. Safe to call when no tunnel exists for the key. +pub fn remove_tunnel(map_key: &str) { + let tunnel = { + let mut tunnels = get_tunnels().lock().unwrap(); + tunnels.remove(map_key) + }; + if let Some(tunnel) = tunnel { + println!( + "[SSH Tunnel] Removing tunnel '{}' (local port {})", + map_key, tunnel.local_port + ); + tunnel.stop(); + } } /// Test an SSH connection without creating a tunnel @@ -854,6 +894,68 @@ mod tests { } } + mod liveness_tests { + use super::*; + + #[test] + fn russh_is_alive_tracks_running_flag() { + let running = Arc::new(AtomicBool::new(true)); + let tunnel = SshTunnel { + local_port: 0, + backend: TunnelBackend::Russh(running.clone()), + }; + assert!(tunnel.is_alive()); + + // stop() flips the flag; the tunnel is then considered dead. + tunnel.stop(); + assert!(!tunnel.is_alive()); + } + + #[cfg(unix)] + #[test] + fn system_ssh_is_alive_detects_exited_child() { + // A long-running child is alive... + let child = Command::new("sleep") + .arg("30") + .spawn() + .expect("spawn sleep"); + let tunnel = SshTunnel { + local_port: 0, + backend: TunnelBackend::SystemSsh(Arc::new(Mutex::new(child))), + }; + assert!(tunnel.is_alive()); + + // ...and dead once stopped (killed + reaped). + tunnel.stop(); + assert!(!tunnel.is_alive()); + } + + #[test] + fn remove_tunnel_is_noop_for_unknown_key() { + // Must not panic when the key is absent from the map. + remove_tunnel("nonexistent@host:22:remote->3306"); + } + + #[cfg(unix)] + #[test] + fn remove_tunnel_stops_and_removes_entry() { + let key = "removal-test@host:22:remote->3306".to_string(); + let child = Command::new("sleep") + .arg("30") + .spawn() + .expect("spawn sleep"); + let tunnel = SshTunnel { + local_port: 0, + backend: TunnelBackend::SystemSsh(Arc::new(Mutex::new(child))), + }; + get_tunnels().lock().unwrap().insert(key.clone(), tunnel); + + remove_tunnel(&key); + + assert!(!get_tunnels().lock().unwrap().contains_key(&key)); + } + } + mod is_empty_or_whitespace_tests { use super::*; From c64b571f3b5871d9c6acac8ce274e0ad276e55d4 Mon Sep 17 00:00:00 2001 From: Andrea Debernardi Date: Sun, 5 Jul 2026 14:31:00 +0200 Subject: [PATCH 2/2] fix(ssh): detect dead russh sessions, add keepalives and fast health-check failure Builds on the existing stale-tunnel eviction: - russh: the forwarding loop now checks handle.is_closed() and shuts itself down when the SSH session dies, releasing the local port; the running flag doubles as liveness state - system ssh: add ServerAliveInterval/CountMax and ExitOnForwardFailure so the process exits when the server goes away instead of holding the port forever - share the tunnel-key construction (ssh_tunnel_key_for) between teardown_ssh_tunnel and the new evict_dead_ssh_tunnel, and use the latter in resolve_connection_params instead of the inline stale check - health check: fail pings fast when the tunnel is dead, and close pools from expanded params without resolving (no tunnel rebuild toward a server that may still be down); dedupe param expansion in expand_params() --- src-tauri/src/commands.rs | 109 +++++++++++++++++++++++----------- src-tauri/src/health_check.rs | 53 +++++++++-------- src-tauri/src/ssh_tunnel.rs | 23 ++++++- 3 files changed, 123 insertions(+), 62 deletions(-) diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 65df20ce9..1dc0b099f 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -220,25 +220,51 @@ fn build_tunnel_map_key( crate::ssh_tunnel::build_tunnel_key(ssh_user, ssh_host, ssh_port, remote_host, remote_port) } +/// Build the tunnel map key from unresolved params, if they describe an SSH +/// tunnel. Expects params that still carry the original SSH/remote fields +/// (i.e. before [`resolve_connection_params`] rewrites host/port to the local +/// forward). +fn ssh_tunnel_key_for(params: &ConnectionParams) -> Option { + if !params.ssh_enabled.unwrap_or(false) { + return None; + } + let ssh_host = params.ssh_host.as_deref()?; + let ssh_user = params.ssh_user.as_deref()?; + Some(build_tunnel_map_key( + ssh_user, + ssh_host, + params.ssh_port.unwrap_or(22), + params.host.as_deref().unwrap_or("localhost"), + params.port.unwrap_or(DEFAULT_MYSQL_PORT), + )) +} + /// Stop and remove the SSH tunnel associated with these params, if any. /// /// Called when a connection is closed (manually or by the health check) so a -/// tunnel does not outlive the connection that owns it. Expects params that -/// still carry the original SSH/remote fields (i.e. before -/// [`resolve_connection_params`] rewrites host/port to the local forward). +/// tunnel does not outlive the connection that owns it. pub(crate) fn teardown_ssh_tunnel(params: &ConnectionParams) { - if !params.ssh_enabled.unwrap_or(false) { - return; + if let Some(map_key) = ssh_tunnel_key_for(params) { + crate::ssh_tunnel::remove_tunnel(&map_key); } - let (Some(ssh_host), Some(ssh_user)) = (params.ssh_host.as_deref(), params.ssh_user.as_deref()) - else { - return; +} + +/// Remove the cached SSH tunnel for these params if it is no longer alive +/// (e.g. the remote server rebooted), so the next resolve creates a fresh one. +/// Returns true if a dead tunnel was evicted. +pub(crate) fn evict_dead_ssh_tunnel(params: &ConnectionParams) -> bool { + let Some(map_key) = ssh_tunnel_key_for(params) else { + return false; }; - let ssh_port = params.ssh_port.unwrap_or(22); - let remote_host = params.host.as_deref().unwrap_or("localhost"); - let remote_port = params.port.unwrap_or(DEFAULT_MYSQL_PORT); - let map_key = build_tunnel_map_key(ssh_user, ssh_host, ssh_port, remote_host, remote_port); - crate::ssh_tunnel::remove_tunnel(&map_key); + let is_dead = { + let tunnels = get_tunnels().lock().unwrap(); + matches!(tunnels.get(&map_key), Some(tunnel) if !tunnel.is_alive()) + }; + if is_dead { + log::warn!("SSH tunnel {} is no longer alive, removing it", map_key); + crate::ssh_tunnel::remove_tunnel(&map_key); + } + is_dead } /// Resolve K8s tunnel params synchronously (no saved-connection lookup; uses inline fields only). @@ -332,29 +358,22 @@ pub fn resolve_connection_params(params: &ConnectionParams) -> Result { - log::debug!("Reusing existing SSH tunnel on port {}", tunnel.local_port); - let mut new_params = params.clone(); - new_params.host = Some("127.0.0.1".to_string()); - new_params.port = Some(tunnel.local_port); - return Ok(new_params); - } - Some(_) => true, - None => false, - } - }; - if stale { - log::warn!("Discarding dead SSH tunnel for {}, recreating", map_key); - crate::ssh_tunnel::remove_tunnel(&map_key); + let tunnels = get_tunnels().lock().unwrap(); + if let Some(tunnel) = tunnels.get(&map_key) { + log::debug!("Reusing existing SSH tunnel on port {}", tunnel.local_port); + let mut new_params = params.clone(); + new_params.host = Some("127.0.0.1".to_string()); + new_params.port = Some(tunnel.local_port); + return Ok(new_params); } } @@ -2377,6 +2396,26 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().contains("SSH User")); } + + #[test] + fn test_evict_dead_ssh_tunnel_noop_when_ssh_disabled() { + let params = base_params(); + assert!(!evict_dead_ssh_tunnel(¶ms)); + } + + #[test] + fn test_evict_dead_ssh_tunnel_noop_without_ssh_fields() { + let mut params = create_ssh_params("jump.server", 22, "admin", "db.internal", 3306); + params.ssh_user = None; + assert!(!evict_dead_ssh_tunnel(¶ms)); + } + + #[test] + fn test_evict_dead_ssh_tunnel_noop_without_cached_tunnel() { + let params = + create_ssh_params("no-such-tunnel.host", 22, "admin", "db.internal", 3306); + assert!(!evict_dead_ssh_tunnel(¶ms)); + } } mod resolve_k8s_params_tests { diff --git a/src-tauri/src/health_check.rs b/src-tauri/src/health_check.rs index a6b0856a4..7b7c805d5 100644 --- a/src-tauri/src/health_check.rs +++ b/src-tauri/src/health_check.rs @@ -136,14 +136,27 @@ async fn ping_all_connections(app: &tauri::AppHandle, failure_counts: &mut HashM } } +/// Look up a saved connection and expand its SSH/K8s params (no tunnel resolution). +async fn expand_params( + app: &tauri::AppHandle, + connection_id: &str, +) -> Result { + let saved_conn = crate::commands::find_connection_by_id(app, connection_id)?; + let expanded = crate::commands::expand_ssh_connection_params(app, &saved_conn.params).await?; + crate::commands::expand_k8s_connection_params(app, &expanded).await +} + /// Ping a single connection by resolving its params and calling driver.ping(). async fn ping_single_connection(app: &tauri::AppHandle, connection_id: &str) -> Result<(), String> { - let saved_conn = crate::commands::find_connection_by_id(app, connection_id)?; + let expanded_params = expand_params(app, connection_id).await?; + + // If the SSH tunnel died (e.g. the server rebooted), evict it and fail the + // ping immediately instead of letting param resolution block on rebuilding + // a tunnel toward a server that may still be down. + if crate::commands::evict_dead_ssh_tunnel(&expanded_params) { + return Err("SSH tunnel is no longer alive".into()); + } - let expanded_params = - crate::commands::expand_ssh_connection_params(app, &saved_conn.params).await?; - let expanded_params = - crate::commands::expand_k8s_connection_params(app, &expanded_params).await?; let params = crate::commands::resolve_connection_params_with_id(&expanded_params, connection_id)?; @@ -167,25 +180,17 @@ async fn handle_connection_failure(app: &tauri::AppHandle, connection_id: &str, // Unregister first to prevent further pings. unregister_connection(connection_id).await; - // Close the pool (best-effort — if params can't be resolved the pool stays orphaned - // but will be reclaimed on next connect or app shutdown). - if let Ok(saved_conn) = crate::commands::find_connection_by_id(app, connection_id) { - if let Ok(expanded) = - crate::commands::expand_ssh_connection_params(app, &saved_conn.params).await - { - // Tear down the SSH tunnel before it is reused: after the remote - // host dies the tunnel is dead but still holds its local port, - // which breaks the next reconnect ("port already in use"). - // `expanded` still carries the original SSH/remote fields. - crate::commands::teardown_ssh_tunnel(&expanded); - - let expanded = crate::commands::expand_k8s_connection_params(app, &expanded).await; - if let Ok(params) = expanded.and_then(|params| { - crate::commands::resolve_connection_params_with_id(¶ms, connection_id) - }) { - crate::pool_manager::close_pool_with_id(¶ms, Some(connection_id)).await; - } - } + // Close the pool (best-effort — if params can't be expanded the pool stays orphaned + // but will be reclaimed on next connect or app shutdown). Tunnel resolution is + // skipped deliberately: with a connection_id the pool key only depends on + // driver/connection_id/database, and resolving would try to rebuild a tunnel + // toward a server that may still be down. + if let Ok(expanded) = expand_params(app, connection_id).await { + // Tear down the SSH tunnel before it is reused: after the remote host + // dies the tunnel is dead but still holds its local port, which breaks + // the next reconnect ("port already in use"). + crate::commands::teardown_ssh_tunnel(&expanded); + crate::pool_manager::close_pool_with_id(&expanded, Some(connection_id)).await; } // Notify frontend. diff --git a/src-tauri/src/ssh_tunnel.rs b/src-tauri/src/ssh_tunnel.rs index 0acc03ed9..05ac1cb0e 100644 --- a/src-tauri/src/ssh_tunnel.rs +++ b/src-tauri/src/ssh_tunnel.rs @@ -190,6 +190,14 @@ impl SshTunnel { args.push("-o".to_string()); args.push("StrictHostKeyChecking=accept-new".to_string()); + // Keepalives make ssh exit when the server goes away (e.g. a reboot); + // without them the process can hold the local port forever. + args.push("-o".to_string()); + args.push("ServerAliveInterval=15".to_string()); + args.push("-o".to_string()); + args.push("ServerAliveCountMax=3".to_string()); + args.push("-o".to_string()); + args.push("ExitOnForwardFailure=yes".to_string()); args.push("-o".to_string()); if ssh_allow_passphrase_prompt { args.push("BatchMode=no".to_string()); @@ -453,6 +461,15 @@ impl SshTunnel { println!("[SSH Tunnel] Starting tunnel forwarding loop"); while running_clone.load(Ordering::Relaxed) { + // If the SSH session died (e.g. the server rebooted), mark + // the tunnel as dead and release the local port so a + // reconnect can create a fresh tunnel. + if handle.lock().await.is_closed() { + eprintln!("[SSH Tunnel Error] SSH session closed unexpectedly; shutting down tunnel"); + running_clone.store(false, Ordering::Relaxed); + break; + } + let accept = tokio::time::timeout( Duration::from_millis(SSH_ACCEPT_POLL_MS), listener.accept(), @@ -542,9 +559,9 @@ impl SshTunnel { /// For a system-ssh backend this detects a process that has already /// exited (e.g. the ssh client tore down after the remote host rebooted), /// which would otherwise leave a stale entry in the tunnel map pointing at - /// a dead local port. The russh backend keeps its local listener bound for - /// as long as it has not been explicitly stopped, so it is considered - /// alive until `stop()` flips the flag. + /// a dead local port. For the russh backend the `running` flag is cleared + /// either by `stop()` or by the forwarding loop itself when it detects the + /// SSH session has closed. pub fn is_alive(&self) -> bool { match &self.backend { TunnelBackend::Russh(running) => running.load(Ordering::Relaxed),