diff --git a/CHANGELOG.md b/CHANGELOG.md index 7956099..0618b34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,13 @@ Versions follow [Semantic Versioning](https://semver.org/). ## Unreleased +### Features +- **Watch a host by a port check instead of an SSH login.** Firewalls, switches and other appliances answer SSH but have no shell to read `top` or `free` from, so monitoring could only ever fail on them — and log in again every cycle to find that out. A host can now be set to **TCP port check**: OmnySSH opens a connection to the port and closes it, with no login and no commands. Its card shows reachable / unreachable in place of the metric tiles, rather than tiles for numbers nobody collected. Set it in the host form — `tcp` (the host's SSH port) or `tcp:PORT` in the terminal app, a dropdown in the desktop app. Existing hosts are untouched and stay on SSH monitoring. ICMP is not offered yet: unprivileged ping is unavailable on the Linux packaging most people use. + ### Bug Fixes +- **Hosts split across `Include` files are imported.** A relative pattern — `Include conf.d/*.conf`, the form nearly every split-config guide prints — was looked for in whatever directory the app happened to be launched from, which for the desktop app started from Finder or the application menu is `/`. Nothing matched, so every host defined in `~/.ssh/conf.d` was missing and had to be added by hand. Those patterns now resolve against `~/.ssh`, the way `ssh` itself resolves them. Four things in the same code path were fixed alongside it: full glob patterns work (`?`, `[abc]`, a wildcard in a directory name, and more than one `*` in a file name), several pathnames on one `Include` line are all read instead of none, a quoted path keeps its spaces, and an `Include` written inside a `Host` block no longer swallows that host's remaining settings. An `Include` that matches nothing is written to the log rather than passing in silence. +- **CPU no longer reads ~91–100 % on an idle server whose system language isn't English.** Monitoring reads the idle percentage from `top` and shows the rest as used. On a server set to a language with a comma decimal separator, `top` prints `99,1 id`, and the parser split that line on commas — reading the idle value as `1` and reporting 99 % used on a machine doing nothing. The same servers could show RAM and Disk as N/A, because `free` and `df` translate the labels being looked for, and the process list could come back empty. The monitoring commands now run in a fixed locale, and the CPU parsers understand a decimal comma on their own for hosts where that cannot be set. Process names keep the server's own character set. +- **A host that answers SSH but not shell commands is no longer re-logged-in every 30 seconds.** Network appliances — firewalls, switches — authenticate fine but cannot run `top` or `free`, and a round of failed metric commands was treated as a dead connection. The retry delay was reset on every successful login, so it never grew past its first step: one login every 30 seconds, indefinitely, and as often as every 10 seconds in the desktop app, whose refresh timer also cut the retry delay short. The delay now escalates properly and is left alone by the refresh timer — the trade-off being that a manual refresh no longer cuts a retry delay short, so a host that has been unreachable for a while retries on its own schedule (at most five minutes). Separately, a device that ignores SSH keepalives was being disconnected after 30 seconds even while its commands still worked; connections are now held open, with liveness still bounded by the keepalive limit. - **`ProxyJump` hosts now connect through their bastion.** A host with `ProxyJump` in `~/.ssh/config` was parsed but never routed: the terminal refused to open it, while metrics, SFTP, snippets and key setup quietly dialled the target address direct — which for an internal host meant every connection failed or, worse, landed somewhere else on that address. Every native SSH path now walks the jump chain the way `ssh -J` does, connecting and authenticating each bastion in turn and tunnelling the next hop over it. - The jump alias is resolved against your host list, so `ProxyJump public-proxy` picks up that entry's `HostName`, `User`, `Port` and `IdentityFile`. An alias that matches no entry is used as a literal hostname. - Multi-hop values (`ProxyJump first,second`), inline `user@host:port` overrides, IPv6 literals, bastions that are themselves behind a bastion, and the `ProxyJump none` opt-out all behave as OpenSSH does — including its precedence rule that only the first hop of a list contributes bastions of its own. Chains that loop, or run past ten hops, are reported instead of hanging. diff --git a/Cargo.lock b/Cargo.lock index f2724fd..b85aa6d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3198,6 +3198,7 @@ dependencies = [ "chrono", "dirs 5.0.1", "flate2", + "glob", "reqwest 0.12.28", "russh", "russh-keys", @@ -3207,6 +3208,7 @@ dependencies = [ "serde", "sha2", "tar", + "tempfile", "thiserror 2.0.18", "tokio", "toml 0.8.23", diff --git a/crates/omnyssh-core/Cargo.toml b/crates/omnyssh-core/Cargo.toml index b01bfed..00ad3dc 100644 --- a/crates/omnyssh-core/Cargo.toml +++ b/crates/omnyssh-core/Cargo.toml @@ -36,6 +36,9 @@ chrono = { workspace = true } # Cross-platform paths (P-CROSS-01) dirs = { workspace = true } +# glob(7) patterns in ssh_config `Include` directives +glob = "0.3" + # Terminal emulator — vt100 screen model (PTY is provided by russh channels) vt100 = { package = "vt100-omnyssh", version = "0.15" } @@ -49,3 +52,4 @@ self-replace = "1" [dev-dependencies] tokio = { workspace = true, features = ["full", "test-util"] } +tempfile = "3" diff --git a/crates/omnyssh-core/src/config/ssh_config.rs b/crates/omnyssh-core/src/config/ssh_config.rs index cc34538..1d73dab 100644 --- a/crates/omnyssh-core/src/config/ssh_config.rs +++ b/crates/omnyssh-core/src/config/ssh_config.rs @@ -13,10 +13,12 @@ use crate::ssh::client::{Host, HostSource}; /// Parses the text of an SSH config file and returns all non-wildcard hosts. /// /// `Host *` entries are silently skipped. -/// `Include` recursion is limited to 3 levels to prevent cycles. +/// `Include` recursion is limited to 3 levels to prevent cycles; relative +/// patterns resolve against `~/.ssh`, as `ssh_config(5)` specifies for a user +/// configuration. pub fn parse_ssh_config(content: &str) -> Vec { let mut visited: HashSet = HashSet::new(); - parse_content(content, 0, &mut visited) + parse_content(content, default_include_base().as_deref(), 0, &mut visited) } /// Loads and parses an SSH config file from disk. @@ -26,20 +28,39 @@ pub fn parse_ssh_config(content: &str) -> Vec { pub fn load_from_file(path: &Path) -> anyhow::Result> { let content = std::fs::read_to_string(path) .map_err(|e| anyhow::anyhow!("Failed to read {}: {}", path.display(), e))?; - Ok(parse_ssh_config(&content)) + let base = path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .map_or_else(default_include_base, |p| Some(p.to_path_buf())); + let mut visited: HashSet = HashSet::new(); + Ok(parse_content(&content, base.as_deref(), 0, &mut visited)) +} + +/// `~/.ssh` — where `ssh_config(5)` resolves a relative `Include` in a user +/// configuration. Never the process working directory. +fn default_include_base() -> Option { + dirs::home_dir().map(|h| h.join(".ssh")) } // --------------------------------------------------------------------------- // Internal helpers // --------------------------------------------------------------------------- -fn parse_content(content: &str, depth: usize, visited: &mut HashSet) -> Vec { +fn parse_content( + content: &str, + base: Option<&Path>, + depth: usize, + visited: &mut HashSet, +) -> Vec { if depth > 3 { return Vec::new(); } let mut hosts: Vec = Vec::new(); let mut current: Option = None; + // Hosts pulled in by an Include that sat inside a Host block. Appended once + // the enclosing host is flushed, so the list keeps the config's own order. + let mut deferred: Vec = Vec::new(); // True when we are inside a wildcard `Host *` block (skip directives). let mut in_wildcard = false; @@ -59,6 +80,7 @@ fn parse_content(content: &str, depth: usize, visited: &mut HashSet) -> if let Some(h) = current.take() { hosts.push(h); } + hosts.append(&mut deferred); in_wildcard = value.contains('*') || value.contains('?'); if !in_wildcard { let h = Host { @@ -98,30 +120,41 @@ fn parse_content(content: &str, depth: usize, visited: &mut HashSet) -> h.proxy_jump = Some(value.to_string()); } } + // An Include may sit inside a Host block; the enclosing host keeps + // collecting directives after it. "include" => { - // Flush the current host before processing Include. - if let Some(h) = current.take() { - hosts.push(h); - } - in_wildcard = false; - let expanded = expand_tilde(value); - for path in expand_include_glob(&expanded) { - // Canonicalise to catch cycles (symlinks, etc.). - let canonical = path.canonicalize().unwrap_or_else(|e| { - tracing::warn!( - path = %path.display(), - error = %e, - "canonicalize failed; symlink-cycle detection disabled for this path" - ); - path.clone() - }); - if !visited.insert(canonical) { - continue; // already visited — break cycle + let sink = if current.is_some() { + &mut deferred + } else { + &mut hosts + }; + for pattern in split_include_patterns(value) { + let Some(resolved) = resolve_include(&pattern, base) else { + tracing::warn!(pattern, "relative Include with no home directory"); + continue; + }; + let matched = expand_include_glob(&resolved); + if matched.is_empty() { + tracing::warn!(pattern = resolved, "Include matched no files"); } - match std::fs::read_to_string(&path) { - Ok(sub) => hosts.extend(parse_content(&sub, depth + 1, visited)), - Err(e) => { - tracing::warn!(path = %path.display(), error = %e, "Include file unreadable") + for path in matched { + // Canonicalise to catch cycles (symlinks, etc.). + let canonical = path.canonicalize().unwrap_or_else(|e| { + tracing::warn!( + path = %path.display(), + error = %e, + "canonicalize failed; symlink-cycle detection disabled for this path" + ); + path.clone() + }); + if !visited.insert(canonical) { + continue; // already visited — break cycle + } + match std::fs::read_to_string(&path) { + Ok(sub) => sink.extend(parse_content(&sub, base, depth + 1, visited)), + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "Include file unreadable") + } } } } @@ -130,10 +163,11 @@ fn parse_content(content: &str, depth: usize, visited: &mut HashSet) -> } } - // Flush the last pending host. + // Flush the last pending host, then anything its Include pulled in. if let Some(h) = current.take() { hosts.push(h); } + hosts.append(&mut deferred); // Fallback: if HostName was never set, use the alias as the address. for h in &mut hosts { @@ -181,48 +215,61 @@ fn expand_tilde(s: &str) -> String { s.to_string() } -/// Resolves an Include pattern to a list of file paths. +/// Splits an `Include` value into its pathnames. /// -/// Supports a single `*` wildcard in the file-name component only. -/// The parent directory must exist; `*` in path segments other than the -/// last one is not supported (matches OpenSSH behaviour). -fn expand_include_glob(pattern: &str) -> Vec { - let path = PathBuf::from(pattern); - let parent = match path.parent() { - Some(p) if !p.as_os_str().is_empty() => p.to_path_buf(), - _ => PathBuf::from("."), - }; - let file_name = match path.file_name().and_then(|f| f.to_str()) { - Some(n) => n.to_string(), - None => return Vec::new(), - }; - - if !file_name.contains('*') { - return if path.is_file() { - vec![path] - } else { - Vec::new() - }; +/// `ssh_config(5)` allows several pathnames on one line, and a path containing +/// spaces may be double-quoted. +fn split_include_patterns(value: &str) -> Vec { + let mut patterns = Vec::new(); + let mut current = String::new(); + let mut quoted = false; + + for c in value.chars() { + match c { + '"' => quoted = !quoted, + c if c.is_whitespace() && !quoted => { + if !current.is_empty() { + patterns.push(std::mem::take(&mut current)); + } + } + c => current.push(c), + } + } + if !current.is_empty() { + patterns.push(current); + } + patterns +} + +/// Anchors an `Include` pattern: absolute and `~/` patterns stand alone, a +/// relative one resolves against `base` — never the process working directory. +/// +/// `None` when the pattern is relative and there is no base to anchor it to; +/// dropping the include is the only honest option, since resolving it would +/// silently read the process working directory. +fn resolve_include(pattern: &str, base: Option<&Path>) -> Option { + let expanded = expand_tilde(pattern); + if Path::new(&expanded).is_absolute() { + return Some(expanded); } + // The base is a real path, not a pattern: escape it so a home directory + // containing `[` or `*` cannot swallow the include. + let base = glob::Pattern::escape(base?.to_str()?); + Some(format!("{}/{}", base.trim_end_matches('/'), expanded)) +} - // Simple single-`*` glob: match prefix and suffix. - let (prefix, suffix) = file_name.split_once('*').unwrap_or((&file_name, "")); - match std::fs::read_dir(&parent) { - Ok(entries) => { - let mut paths: Vec = entries - .flatten() - .filter(|e| { - let name = e.file_name(); - let name = name.to_string_lossy(); - name.starts_with(prefix) && name.ends_with(suffix) - }) - .map(|e| e.path()) - .filter(|p| p.is_file()) - .collect(); - paths.sort(); // deterministic order - paths +/// Resolves an Include pattern to the files it matches, in a stable order. +/// +/// Full glob(7) syntax, as `ssh_config(5)` specifies. Directories that match +/// are skipped. +fn expand_include_glob(pattern: &str) -> Vec { + match glob::glob(pattern) { + // `glob` yields matches in sorted order, so the host list is stable. + Ok(paths) => paths.flatten().filter(|p| p.is_file()).collect(), + Err(e) => { + tracing::warn!(pattern, error = %e, "invalid Include pattern"); + Vec::new() } - Err(_) => Vec::new(), } } diff --git a/crates/omnyssh-core/src/ssh/client.rs b/crates/omnyssh-core/src/ssh/client.rs index 20fa7b4..8f1c402 100644 --- a/crates/omnyssh-core/src/ssh/client.rs +++ b/crates/omnyssh-core/src/ssh/client.rs @@ -16,6 +16,28 @@ pub enum HostSource { Manual, } +/// How a host is watched on the dashboard. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Default)] +#[serde(rename_all = "snake_case")] +pub enum MonitorMode { + /// A live SSH session with shell metrics — the default. + #[default] + Ssh, + /// A plain TCP connect, with no login. For devices that answer SSH but have + /// no POSIX shell to collect metrics from, such as network appliances. + // The wire and the TUI form spell this differently; accept both, because a + // rejected value fails the whole file and takes every manual host with it. + #[serde(alias = "tcp", alias = "tcpPort")] + TcpPort, +} + +impl MonitorMode { + /// Lets the default stay out of `hosts.toml` entirely. + fn is_ssh(&self) -> bool { + matches!(self, Self::Ssh) + } +} + /// A single host entry used for SSH connections. /// /// Populated either from `~/.ssh/config` (via the parser) or from @@ -54,6 +76,12 @@ pub struct Host { /// Used to prevent duplicate entries when a SSH-config host is renamed. #[serde(skip_serializing_if = "Option::is_none")] pub original_ssh_host: Option, + /// How this host is watched. + #[serde(default, skip_serializing_if = "MonitorMode::is_ssh")] + pub monitoring: MonitorMode, + /// Port for the reachability probe. Falls back to `port` when unset. + #[serde(skip_serializing_if = "Option::is_none")] + pub monitor_port: Option, // ----------------------------------------------------------------------- // Auto SSH Key Setup metadata @@ -90,6 +118,8 @@ impl Default for Host { notes: None, source: HostSource::default(), original_ssh_host: None, + monitoring: MonitorMode::default(), + monitor_port: None, key_setup_date: None, password_auth_disabled: None, } @@ -109,3 +139,54 @@ pub enum ConnectionStatus { /// The last connection attempt failed with the given message. Failed(String), } + +#[cfg(test)] +mod tests { + use super::*; + + /// An existing `hosts.toml` predates the monitoring fields, and writing one + /// back must not add them — the file has to stay byte-identical for a host + /// nobody changed. + #[test] + fn the_monitoring_default_round_trips_without_touching_the_file() { + let host: Host = toml::from_str("name = \"web\"\nhostname = \"10.0.0.1\"\n") + .expect("a host without the monitoring fields still parses"); + assert_eq!(host.monitoring, MonitorMode::Ssh); + assert_eq!(host.monitor_port, None); + + let written = toml::to_string(&host).expect("serialize"); + assert!(!written.contains("monitoring"), "{written}"); + assert!(!written.contains("monitor_port"), "{written}"); + } + + /// The wire and the TUI form spell the mode differently, so a hand-edited + /// file is likely to carry either — and a rejected value fails the whole + /// file, taking every manual host with it. + #[test] + fn the_other_spellings_of_the_mode_are_accepted() { + for spelling in ["tcp_port", "tcp", "tcpPort"] { + let host: Host = toml::from_str(&format!( + "name = \"fw\"\nhostname = \"10.0.0.9\"\nmonitoring = \"{spelling}\"\n" + )) + .unwrap_or_else(|e| panic!("'{spelling}' should parse: {e}")); + assert_eq!(host.monitoring, MonitorMode::TcpPort); + } + } + + #[test] + fn a_reachability_host_persists_its_mode_and_probe_port() { + let host = Host { + name: String::from("fw"), + hostname: String::from("10.0.0.9"), + monitoring: MonitorMode::TcpPort, + monitor_port: Some(8443), + ..Host::default() + }; + + let written = toml::to_string(&host).expect("serialize"); + let read: Host = toml::from_str(&written).expect("deserialize"); + + assert_eq!(read.monitoring, MonitorMode::TcpPort); + assert_eq!(read.monitor_port, Some(8443)); + } +} diff --git a/crates/omnyssh-core/src/ssh/metrics.rs b/crates/omnyssh-core/src/ssh/metrics.rs index 39187b9..03d2fc0 100644 --- a/crates/omnyssh-core/src/ssh/metrics.rs +++ b/crates/omnyssh-core/src/ssh/metrics.rs @@ -87,7 +87,33 @@ fn parse_cpu_busybox(line: &str) -> Option { None } +/// Rewrites a decimal comma as a decimal point. +/// +/// The commands are run under a pinned locale, but that is best-effort: a host +/// without `env`, or a wrapper that re-exports a locale, still reaches the +/// parsers with `99,1 id`. Only a comma between two digits is a radix point — +/// every other comma separates fields and must survive. +fn normalize_decimal_commas(line: &str) -> String { + let chars: Vec = line.chars().collect(); + chars + .iter() + .enumerate() + .map(|(i, &c)| { + let radix = c == ',' + && i > 0 + && chars[i - 1].is_ascii_digit() + && chars.get(i + 1).is_some_and(char::is_ascii_digit); + if radix { + '.' + } else { + c + } + }) + .collect() +} + fn parse_cpu_linux_top_line(line: &str) -> Option { + let line = normalize_decimal_commas(line); // Strip leading label (everything up to and including the colon). let after_colon = line.split_once(':')?.1; @@ -119,6 +145,7 @@ pub fn parse_cpu_top_macos(output: &str) -> Option { for line in output.lines() { let lower = line.trim().to_lowercase(); if lower.starts_with("cpu usage:") { + let line = normalize_decimal_commas(line); // Find "idle" value for part in line.split(',') { let part = part.trim(); @@ -352,7 +379,12 @@ pub fn parse_top_processes(output: &str) -> Option> { continue; }; // The first two columns must be numeric — this skips the header row. - let (Ok(cpu), Ok(mem)) = (cpu_str.parse::(), mem_str.parse::()) else { + // `ps` prints them in the server's locale, so a decimal comma is normalised + // here too; a non-numeric token still fails to parse. + let (Ok(cpu), Ok(mem)) = ( + normalize_decimal_commas(cpu_str).parse::(), + normalize_decimal_commas(mem_str).parse::(), + ) else { continue; }; let name = fields.collect::>().join(" "); diff --git a/crates/omnyssh-core/src/ssh/pool.rs b/crates/omnyssh-core/src/ssh/pool.rs index 3a272d8..efaa759 100644 --- a/crates/omnyssh-core/src/ssh/pool.rs +++ b/crates/omnyssh-core/src/ssh/pool.rs @@ -13,11 +13,13 @@ use std::collections::HashMap; use std::time::{Duration, Instant}; +use tokio::net::TcpStream; use tokio::sync::mpsc; use tokio::task::JoinHandle; +use tokio::time; use crate::event::{CoreEvent, Metrics, ProcessInfo}; -use crate::ssh::client::{ConnectionStatus, Host}; +use crate::ssh::client::{ConnectionStatus, Host, MonitorMode}; use crate::ssh::metrics::{ parse_cpu_proc_stat, parse_cpu_top, parse_cpu_top_macos, parse_disk_df, parse_loadavg, parse_ram_free, parse_ram_vmstat, parse_top_processes, parse_uptime, @@ -30,6 +32,23 @@ use crate::ssh::session::SshSession; const BACKOFF_SECS: [u64; 4] = [30, 60, 120, 300]; +// --------------------------------------------------------------------------- +// Metric commands +// --------------------------------------------------------------------------- + +// Metric output is machine-parsed, so the locale has to be pinned: a server set +// to a comma-decimal language prints "99,1 id" and "Speicher:", which the parsers +// read as garbage or not at all. `env` rather than a `VAR=value cmd` prefix, which +// is not valid csh/tcsh syntax. +const CPU_CMD: &str = "env LC_ALL=C top -bn1 2>/dev/null | head -5"; +const MEM_CMD: &str = "env LC_ALL=C free -b 2>/dev/null || env LC_ALL=C vm_stat 2>/dev/null"; +const DISK_CMD: &str = "env LC_ALL=C df -k / 2>/dev/null"; +const UPTIME_CMD: &str = "env LC_ALL=C uptime 2>/dev/null"; +const CPU_MACOS_CMD: &str = "env LC_ALL=C top -l 1 -n 0 2>/dev/null | grep 'CPU usage'"; +// `ps` output reaches the user, so keep the host's LC_CTYPE: under a full +// `LC_ALL=C` GNU ps replaces every non-ASCII byte of a process name with '?'. +const PS_LOCALE: &str = "env LC_ALL= LC_NUMERIC=C LC_MESSAGES=C"; + struct BackoffState { step: usize, } @@ -109,6 +128,97 @@ impl PollManager { // --------------------------------------------------------------------------- async fn run_host_poller( + host: Host, + tx: mpsc::Sender, + poll_interval: Duration, + refresh_rx: mpsc::Receiver<()>, +) { + match host.monitoring { + MonitorMode::Ssh => run_ssh_poller(host, tx, poll_interval, refresh_rx).await, + MonitorMode::TcpPort => run_tcp_poller(host, tx, poll_interval, refresh_rx).await, + } +} + +/// How long a reachability probe waits for the port to answer. +const TCP_PROBE_TIMEOUT: Duration = Duration::from_secs(5); + +/// Reachability-only poller: one TCP connect per cycle, no SSH session and no +/// authentication, so a device that cannot serve metrics is never logged in to. +/// Emits status only — a host in this mode reports no metrics. +async fn run_tcp_poller( + host: Host, + tx: mpsc::Sender, + poll_interval: Duration, + mut refresh_rx: mpsc::Receiver<()>, +) { + // A bare TCP dial cannot traverse a bastion, and probing the target address + // direct would silently report on whatever else answers it. + if host.proxy_jump.is_some() { + send_status( + &tx, + &host.name, + ConnectionStatus::Failed(String::from( + "a port check cannot reach a host behind ProxyJump - use SSH monitoring", + )), + ) + .await; + return; + } + + let port = host.monitor_port.filter(|&p| p != 0).unwrap_or(host.port); + let addr = format!("{}:{}", host.hostname, port); + let mut backoff = BackoffState::new(); + let mut last: Option = None; + + loop { + if last.is_none() + && tx + .send(CoreEvent::HostStatusChanged( + host.name.clone(), + ConnectionStatus::Connecting, + )) + .await + .is_err() + { + return; // App has shut down. + } + + let status = match time::timeout(TCP_PROBE_TIMEOUT, TcpStream::connect(&addr)).await { + Ok(Ok(_)) => ConnectionStatus::Connected, + Ok(Err(e)) => ConnectionStatus::Failed(e.to_string()), + Err(_) => ConnectionStatus::Failed(format!("no answer from {addr}")), + }; + let reachable = matches!(status, ConnectionStatus::Connected); + + // Only on a change: re-announcing every cycle flickers the card between + // reachable and checking and re-buckets the host in the status bar. + if last.as_ref() != Some(&status) { + if tx + .send(CoreEvent::HostStatusChanged( + host.name.clone(), + status.clone(), + )) + .await + .is_err() + { + return; + } + last = Some(status); + } + + if reachable { + backoff.reset(); + wait_or_refresh(poll_interval, &mut refresh_rx).await; + } else { + // Same restraint as the SSH poller: a device that is down should not + // be re-dialled on every refresh tick. + let delay = backoff.next_delay().max(poll_interval); + wait_backoff(delay, &mut refresh_rx).await; + } + } +} + +async fn run_ssh_poller( host: Host, tx: mpsc::Sender, poll_interval: Duration, @@ -124,7 +234,6 @@ async fn run_host_poller( send_status(&tx, &host.name, ConnectionStatus::Connecting).await; match SshSession::connect(&host).await { Ok(s) => { - backoff.reset(); send_status(&tx, &host.name, ConnectionStatus::Connected).await; session = Some(s); discovery_done = false; // Reset discovery flag on new connection @@ -134,7 +243,7 @@ async fn run_host_poller( send_status(&tx, &host.name, ConnectionStatus::Failed(e.to_string())).await; // Wait with backoff, allowing early refresh. let delay = backoff.next_delay(); - wait_or_refresh(delay, &mut refresh_rx).await; + wait_backoff(delay, &mut refresh_rx).await; continue; } } @@ -180,6 +289,10 @@ async fn run_host_poller( let sess = session.as_ref().expect("session is Some here"); match collect_metrics(sess, &host.name).await { Ok(metrics) => { + // A cycle that produced data proves the host is pollable. Resetting + // on connect instead pins a host that authenticates but cannot run + // commands (a network appliance) to the first backoff step forever. + backoff.reset(); if tx .send(CoreEvent::MetricsUpdate(host.name.clone(), metrics)) .await @@ -194,7 +307,7 @@ async fn run_host_poller( session.take(); send_status(&tx, &host.name, ConnectionStatus::Failed(e.to_string())).await; let delay = backoff.next_delay(); - wait_or_refresh(delay, &mut refresh_rx).await; + wait_backoff(delay, &mut refresh_rx).await; continue; } } @@ -206,9 +319,41 @@ async fn run_host_poller( /// Wait for `delay`, but return early if a refresh signal is received. async fn wait_or_refresh(delay: Duration, refresh_rx: &mut mpsc::Receiver<()>) { + let sleep = tokio::time::sleep(delay); + tokio::pin!(sleep); tokio::select! { - _ = tokio::time::sleep(delay) => {} - _ = refresh_rx.recv() => {} + () = &mut sleep => {} + signal = refresh_rx.recv() => { + // `None` means every sender is gone. Returning on it would make the + // caller's loop spin, so serve out the delay instead. + if signal.is_none() { + sleep.await; + } + } + } +} + +/// Sleep the whole `delay`, discarding refresh signals. +/// +/// A reconnect must never dial faster than the backoff schedule: the GUI drives +/// `refresh_all` on its own timer, which is indistinguishable from a keypress +/// here and would otherwise retry a failing host every few seconds. +async fn wait_backoff(delay: Duration, refresh_rx: &mut mpsc::Receiver<()>) { + let sleep = tokio::time::sleep(delay); + tokio::pin!(sleep); + loop { + tokio::select! { + () = &mut sleep => return, + signal = refresh_rx.recv() => { + // `None` means every sender is gone and `recv` will return it + // immediately from now on — stop selecting on it, or the task + // spins without ever yielding. + if signal.is_none() { + sleep.await; + return; + } + } + } } } @@ -233,10 +378,10 @@ async fn send_status(tx: &mpsc::Sender, name: &str, status: Connectio async fn collect_metrics(session: &SshSession, host_name: &str) -> anyhow::Result { // Run all commands concurrently for speed. let (cpu_out, mem_out, disk_out, uptime_out, loadavg_out) = tokio::join!( - session.run_command("top -bn1 2>/dev/null | head -5"), - session.run_command("free -b 2>/dev/null || vm_stat 2>/dev/null"), - session.run_command("df -k / 2>/dev/null"), - session.run_command("uptime 2>/dev/null"), + session.run_command(CPU_CMD), + session.run_command(MEM_CMD), + session.run_command(DISK_CMD), + session.run_command(UPTIME_CMD), session.run_command("cat /proc/loadavg 2>/dev/null"), ); @@ -347,8 +492,8 @@ async fn collect_top_processes(session: &SshSession) -> Option> /// "process data unavailable". fn top_processes_command(ps_args: &str) -> String { format!( - "g=$(ps -o ppid= -p $PPID 2>/dev/null | tr -d ' '); \ - ps {ps_args} 2>/dev/null | \ + "g=$({PS_LOCALE} ps -o ppid= -p $PPID 2>/dev/null | tr -d ' '); \ + {PS_LOCALE} ps {ps_args} 2>/dev/null | \ awk -v s=$$ -v p=$PPID -v g=\"$g\" \ '$1!=s && $1!=p && $1!=g && $2!=s && $2!=p \ {{$1=\"\";$2=\"\";sub(/^[ \\t]+/,\"\");print}}' | \ @@ -362,10 +507,7 @@ async fn parse_cpu_combined(top_out: &str, session: &SshSession) -> Option return Some(v); } // Try macOS top format. - let macos_out = session - .run_command("top -l 1 -n 0 2>/dev/null | grep 'CPU usage'") - .await - .unwrap_or_default(); + let macos_out = session.run_command(CPU_MACOS_CMD).await.unwrap_or_default(); if let Some(v) = parse_cpu_top_macos(&macos_out) { return Some(v); } @@ -397,12 +539,93 @@ async fn parse_ram_combined(mem_out: &str, session: &SshSession) -> Option mod tests { use super::*; + #[test] + fn backoff_escalates_and_only_a_reset_returns_it_to_the_first_step() { + let mut backoff = BackoffState::new(); + let steps: Vec = (0..5).map(|_| backoff.next_delay().as_secs()).collect(); + assert_eq!(steps, vec![30, 60, 120, 300, 300]); + + backoff.reset(); + assert_eq!(backoff.next_delay().as_secs(), 30); + } + + #[tokio::test(start_paused = true)] + async fn wait_backoff_ignores_refresh_signals() { + let (tx, mut rx) = mpsc::channel::<()>(4); + for _ in 0..4 { + tx.try_send(()).expect("channel has room"); + } + + let start = tokio::time::Instant::now(); + wait_backoff(Duration::from_secs(300), &mut rx).await; + + assert_eq!(start.elapsed(), Duration::from_secs(300)); + } + + #[tokio::test(start_paused = true)] + async fn wait_backoff_serves_out_its_delay_once_every_sender_is_gone() { + let (tx, mut rx) = mpsc::channel::<()>(4); + drop(tx); + + let start = tokio::time::Instant::now(); + wait_backoff(Duration::from_secs(300), &mut rx).await; + + assert_eq!(start.elapsed(), Duration::from_secs(300)); + } + + #[tokio::test(start_paused = true)] + async fn wait_or_refresh_serves_out_its_delay_once_every_sender_is_gone() { + let (tx, mut rx) = mpsc::channel::<()>(4); + drop(tx); + + let start = tokio::time::Instant::now(); + wait_or_refresh(Duration::from_secs(300), &mut rx).await; + + assert_eq!(start.elapsed(), Duration::from_secs(300)); + } + + #[tokio::test(start_paused = true)] + async fn wait_or_refresh_still_returns_early() { + let (tx, mut rx) = mpsc::channel::<()>(4); + tx.try_send(()).expect("channel has room"); + + let start = tokio::time::Instant::now(); + wait_or_refresh(Duration::from_secs(300), &mut rx).await; + + assert!(start.elapsed() < Duration::from_secs(1)); + } + + #[test] + fn metric_commands_pin_the_locale() { + for cmd in [CPU_CMD, MEM_CMD, DISK_CMD, UPTIME_CMD, CPU_MACOS_CMD] { + assert!(cmd.starts_with("env LC_ALL=C "), "unpinned command: {cmd}"); + } + // The `||` fallback needs the prefix on both sides. + assert_eq!(MEM_CMD.matches("env LC_ALL=C ").count(), 2); + } + + #[test] + fn top_processes_command_pins_numbers_but_keeps_the_host_ctype() { + let cmd = top_processes_command("-eo pcpu="); + + // Both `ps` invocations are pinned, so a comma-decimal host still parses. + assert_eq!(cmd.matches(PS_LOCALE).count(), 2); + // LC_ALL is cleared rather than set: it outranks LC_NUMERIC, so leaving a + // host's own LC_ALL in place would defeat the pin. LC_CTYPE still falls + // through to the host, so process names reach the user verbatim. + assert!(cmd.contains("LC_ALL=")); + assert!(!cmd.contains("LC_ALL=C")); + assert!(!cmd.contains("LC_CTYPE")); + } + #[test] fn top_processes_command_excludes_monitor_pid_chain() { let cmd = top_processes_command("-eo pid=,ppid=,pcpu=,pmem=,comm= --sort=-pcpu"); - // The grandparent PID is resolved before the pipeline runs. - assert!(cmd.contains("g=$(ps -o ppid= -p $PPID")); + // The grandparent PID is resolved in a substitution that runs before + // the pipeline does. + assert!(cmd.starts_with("g=$(")); + assert!(cmd.contains("ps -o ppid= -p $PPID 2>/dev/null | tr -d ' ')")); // The awk filter binds the shell, its parent sshd and the grandparent. assert!(cmd.contains("-v s=$$")); assert!(cmd.contains("-v p=$PPID")); diff --git a/crates/omnyssh-core/src/ssh/session.rs b/crates/omnyssh-core/src/ssh/session.rs index a1d5364..917627f 100644 --- a/crates/omnyssh-core/src/ssh/session.rs +++ b/crates/omnyssh-core/src/ssh/session.rs @@ -309,7 +309,10 @@ pub(crate) async fn connect_budget(host: &Host) -> Duration { /// The shared russh client configuration (timeouts + keepalives). fn client_config() -> Arc { Arc::new(client::Config { - inactivity_timeout: Some(Duration::from_secs(30)), + // No inactivity timeout: russh skips resetting it on the iteration that + // sends a keepalive, so a peer that never answers `keepalive@openssh.com` + // (common in appliance SSH stacks) was torn down after 30 s even while + // its commands still ran. Liveness stays bounded by `keepalive_max`. keepalive_interval: Some(Duration::from_secs(15)), keepalive_max: 3, ..Default::default() diff --git a/crates/omnyssh-core/tests/metrics_parser.rs b/crates/omnyssh-core/tests/metrics_parser.rs index 4d12dbc..4b5928a 100644 --- a/crates/omnyssh-core/tests/metrics_parser.rs +++ b/crates/omnyssh-core/tests/metrics_parser.rs @@ -34,6 +34,49 @@ fn cpu_alpine_busybox() { assert!((pct - 6.0).abs() < 0.2, "expected ~6.0, got {pct}"); } +// A server whose system language uses a comma decimal separator prints +// "99,1 id"; splitting the line on ',' then read the tenths digit as the idle +// value and reported 100 - that, i.e. ~91-100 % on an idle machine. +#[test] +fn cpu_comma_decimal_locale() { + let out = "%CPU(s): 0,0 us, 0,9 sy, 0,0 ni, 99,1 id, 0,0 wa, 0,0 hi, 0,0 si, 0,0 st"; + let pct = parse_cpu_top(out).expect("parse comma-locale cpu"); + assert!((pct - 0.9).abs() < 0.2, "expected ~0.9, got {pct}"); +} + +#[test] +fn cpu_comma_decimal_reported_values() { + for (idle, expected) in [("99,9", 0.1), ("99,8", 0.2)] { + let out = format!("%Cpu(s): 0,1 us, 0,0 sy, 0,0 ni, {idle} id, 0,0 wa"); + let pct = parse_cpu_top(&out).expect("parse comma-locale cpu"); + assert!( + (pct - expected).abs() < 0.2, + "expected ~{expected}, got {pct}" + ); + } +} + +#[test] +fn cpu_comma_decimal_fully_idle() { + let out = "%Cpu(s): 0,0 us, 0,0 sy, 0,0 ni,100,0 id, 0,0 wa, 0,0 hi, 0,0 si, 0,0 st"; + let pct = parse_cpu_top(out).expect("parse comma-locale cpu"); + assert!(pct.abs() < 0.2, "expected ~0.0, got {pct}"); +} + +#[test] +fn cpu_comma_decimal_old_format() { + let out = "Cpu(s): 2,3%us, 0,7%sy, 0,0%ni, 96,7%id, 0,3%wa, 0,0%hi, 0,0%si, 0,0%st"; + let pct = parse_cpu_top(out).expect("parse comma-locale centos7 cpu"); + assert!((pct - 3.3).abs() < 0.2, "expected ~3.3, got {pct}"); +} + +#[test] +fn cpu_macos_comma_decimal_locale() { + let out = "CPU usage: 3,17% user, 1,56% sys, 95,26% idle"; + let pct = parse_cpu_top_macos(out).expect("parse comma-locale macos cpu"); + assert!((pct - 4.74).abs() < 0.2, "expected ~4.74, got {pct}"); +} + #[test] fn cpu_macos_top() { let out = "CPU usage: 3.17% user, 1.56% sys, 95.26% idle"; diff --git a/crates/omnyssh-core/tests/poll_backoff.rs b/crates/omnyssh-core/tests/poll_backoff.rs new file mode 100644 index 0000000..cfac70f --- /dev/null +++ b/crates/omnyssh-core/tests/poll_backoff.rs @@ -0,0 +1,212 @@ +//! The reconnect schedule of the metrics poller, driven through `PollManager`. +//! +//! The listener accepts a TCP connection and drops it, so every SSH connect +//! fails the way an unreachable host does — enough to exercise the backoff +//! without an SSH server. + +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; +use std::time::Duration; + +use tokio::net::TcpListener; +use tokio::sync::mpsc; + +use omnyssh_core::event::CoreEvent; +use omnyssh_core::ssh::client::{ConnectionStatus, Host, MonitorMode}; +use omnyssh_core::ssh::pool::PollManager; + +/// Counts connections, answering none of them. +async fn dead_listener() -> (u16, Arc) { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let port = listener.local_addr().expect("local addr").port(); + let dials = Arc::new(AtomicUsize::new(0)); + + let counter = Arc::clone(&dials); + tokio::spawn(async move { + while let Ok((stream, _)) = listener.accept().await { + counter.fetch_add(1, Ordering::SeqCst); + drop(stream); + } + }); + + (port, dials) +} + +fn unreachable_host(port: u16) -> Host { + Host { + name: String::from("probe"), + hostname: String::from("127.0.0.1"), + port, + ..Host::default() + } +} + +/// Drains events so a full channel can never stall the poller under test. +fn drain(mut rx: mpsc::Receiver) { + tokio::spawn(async move { while rx.recv().await.is_some() {} }); +} + +/// Waits until `dials` reaches `target`, or `limit` of the test clock elapses. +/// Sampling rather than sleeping a fixed span keeps the count off the critical +/// path: the listener increments it from its own task. +async fn wait_for_dials(dials: &AtomicUsize, target: usize, limit: Duration) -> bool { + tokio::time::timeout(limit, async { + while dials.load(Ordering::SeqCst) < target { + tokio::time::sleep(Duration::from_millis(100)).await; + } + }) + .await + .is_ok() +} + +#[tokio::test(start_paused = true)] +async fn a_refresh_signal_does_not_shorten_the_reconnect_backoff() { + let (port, dials) = dead_listener().await; + let (tx, rx) = mpsc::channel(64); + drain(rx); + + let manager = PollManager::start(vec![unreachable_host(port)], tx, Duration::from_secs(30)); + assert!( + wait_for_dials(&dials, 1, Duration::from_secs(60)).await, + "the poller never dialled at all" + ); + + // Twelve nudges over two minutes — the shape of the GUI's refresh timer at + // its shortest setting. + for _ in 0..12 { + manager.refresh_all(); + tokio::time::sleep(Duration::from_secs(10)).await; + } + manager.shutdown(); + + // Backoff runs 30, 60, 120: four dials at most in the 120 s that follow the + // first. Honouring the signals instead dials on every one of them. + let dialled = dials.load(Ordering::SeqCst); + assert!( + dialled <= 4, + "expected the backoff schedule to hold, got {dialled} dials" + ); +} + +#[tokio::test(start_paused = true)] +async fn an_unreachable_host_keeps_retrying_on_its_own_schedule() { + let (port, dials) = dead_listener().await; + let (tx, rx) = mpsc::channel(64); + drain(rx); + + let manager = PollManager::start(vec![unreachable_host(port)], tx, Duration::from_secs(30)); + let retried = wait_for_dials(&dials, 2, Duration::from_secs(300)).await; + manager.shutdown(); + + // A host that stops answering is retried, not abandoned. + assert!(retried, "the poller stopped retrying an unreachable host"); +} + +#[tokio::test(start_paused = true)] +async fn a_reachability_host_is_probed_without_an_ssh_session() { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let port = listener.local_addr().expect("local addr").port(); + tokio::spawn(async move { while listener.accept().await.is_ok() {} }); + + let host = Host { + monitoring: MonitorMode::TcpPort, + // The probe port wins over the SSH port, which is deliberately dead here. + port: 1, + monitor_port: Some(port), + ..unreachable_host(port) + }; + let (tx, mut rx) = mpsc::channel(64); + let manager = PollManager::start(vec![host], tx, Duration::from_secs(30)); + + // Bounded: the poller never closes the channel, so an unbounded wait would + // hang instead of failing when the expected status stops arriving. + let reachable = tokio::time::timeout(Duration::from_secs(60), async { + while let Some(event) = rx.recv().await { + if let CoreEvent::HostStatusChanged(_, ConnectionStatus::Connected) = event { + return true; + } + } + false + }) + .await; + assert_eq!( + reachable, + Ok(true), + "the probe never reported the port as reachable" + ); + + // And over the cycles that follow, it reports status only — metrics would be + // invented. Elapsing without a metrics event is the pass. + let stray_metrics = tokio::time::timeout(Duration::from_secs(120), async { + while let Some(event) = rx.recv().await { + if matches!(event, CoreEvent::MetricsUpdate(..)) { + return true; + } + } + false + }) + .await; + manager.shutdown(); + + assert_ne!( + stray_metrics, + Ok(true), + "a tcp-probed host must not report metrics" + ); +} + +#[tokio::test(start_paused = true)] +async fn a_reachability_host_reports_a_closed_port_as_failed() { + // Port 1 is closed and cannot be re-bound by anything else mid-test. + let port = 1; + + let host = Host { + monitoring: MonitorMode::TcpPort, + ..unreachable_host(port) + }; + let (tx, mut rx) = mpsc::channel(64); + let manager = PollManager::start(vec![host], tx, Duration::from_secs(30)); + + let failed = tokio::time::timeout(Duration::from_secs(60), async { + while let Some(event) = rx.recv().await { + if let CoreEvent::HostStatusChanged(_, ConnectionStatus::Failed(_)) = event { + return true; + } + } + false + }) + .await; + manager.shutdown(); + + assert_eq!( + failed, + Ok(true), + "a closed port was not reported as unreachable" + ); +} + +#[tokio::test(start_paused = true)] +async fn a_reachability_host_behind_a_bastion_says_so_instead_of_probing_direct() { + let host = Host { + monitoring: MonitorMode::TcpPort, + proxy_jump: Some(String::from("bastion")), + ..unreachable_host(22) + }; + let (tx, mut rx) = mpsc::channel(64); + let manager = PollManager::start(vec![host], tx, Duration::from_secs(30)); + + let explained = tokio::time::timeout(Duration::from_secs(60), async { + while let Some(event) = rx.recv().await { + if let CoreEvent::HostStatusChanged(_, ConnectionStatus::Failed(message)) = event { + // Probing the target address direct would report on whatever + // else answers it, which is worse than saying nothing. + return message.contains("ProxyJump"); + } + } + false + }) + .await; + manager.shutdown(); + + assert_eq!(explained, Ok(true), "a bastion host was probed direct"); +} diff --git a/crates/omnyssh-core/tests/ssh_config_parser.rs b/crates/omnyssh-core/tests/ssh_config_parser.rs index 9fd2d7e..63a8993 100644 --- a/crates/omnyssh-core/tests/ssh_config_parser.rs +++ b/crates/omnyssh-core/tests/ssh_config_parser.rs @@ -1,5 +1,256 @@ -// Integration tests for the SSH config parser are located alongside the -// implementation in `src/config/ssh_config.rs` (the `#[cfg(test)]` block). -// -// This file is intentionally minimal: the parser's unit tests live in the -// module itself and cover the parsing logic directly. +//! Integration tests for `Include` handling in the SSH config parser. +//! +//! The parsing rules themselves are covered by the unit tests next to the +//! implementation. These need a real directory tree on disk, so they live here. + +use std::fs; +use std::path::Path; + +use omnyssh_core::config::ssh_config::load_from_file; + +/// The layout from the field report: a top-level config plus a `conf.d` split. +fn write_fixture(root: &Path) -> std::path::PathBuf { + let ssh = root.join(".ssh"); + let conf_d = ssh.join("conf.d"); + fs::create_dir_all(&conf_d).expect("create fixture tree"); + + fs::write( + conf_d.join("10-vps.conf"), + "Host vps\n HostName 1.2.3.4\n User root\n", + ) + .expect("write 10-vps.conf"); + fs::write( + conf_d.join("20-work-test.conf"), + "Host work-test\n HostName 10.0.0.2\n", + ) + .expect("write 20-work-test.conf"); + fs::write( + conf_d.join("30-work-stage.conf"), + "Host work-stage\n HostName 10.0.0.3\n", + ) + .expect("write 30-work-stage.conf"); + + ssh.join("config") +} + +/// Writes `body`, then the direct host every case shares, and parses the result. +/// The fixture tree must already exist. +fn hosts_for(root: &Path, body: &str) -> Vec { + let config = root.join(".ssh").join("config"); + fs::write( + &config, + format!("{body}\n\nHost local-direct\n HostName 127.0.0.1\n"), + ) + .expect("write config"); + + load_from_file(&config) + .expect("parse config") + .into_iter() + .map(|h| h.name) + .collect() +} + +#[test] +fn relative_include_resolves_against_the_config_directory() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let names = hosts_for(tmp.path(), "Include conf.d/*.conf"); + + assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); +} + +/// The defect this file exists for: a relative `Include` used to be read from +/// the directory the app happened to be launched in, so a GUI started from +/// Finder (working directory `/`) found none of the included hosts. +#[test] +fn a_relative_include_ignores_the_process_working_directory() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + + // A decoy tree that a working-directory lookup would find instead. + let decoy = tempfile::tempdir().expect("tempdir"); + fs::create_dir_all(decoy.path().join("conf.d")).expect("create decoy tree"); + fs::write( + decoy.path().join("conf.d").join("99-decoy.conf"), + "Host decoy + HostName 6.6.6.6 +", + ) + .expect("write decoy"); + + // Restore on the way out even if the parse panics: the working directory is + // process-global and the decoy is deleted when this test ends. + struct RestoreCwd(std::path::PathBuf); + impl Drop for RestoreCwd { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.0); + } + } + let guard = RestoreCwd(std::env::current_dir().expect("read working directory")); + std::env::set_current_dir(decoy.path()).expect("enter decoy directory"); + let names = hosts_for(tmp.path(), "Include conf.d/*.conf"); + drop(guard); + + assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); +} + +#[test] +fn absolute_and_glob7_include_patterns_resolve() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let conf_d = tmp.path().join(".ssh").join("conf.d"); + + let cases = [ + format!("Include {}/*.conf", conf_d.display()), + "Include conf.d/?0-*.conf".to_string(), + "Include conf.d/[123]0-*.conf".to_string(), + "Include con*/*.conf".to_string(), + ]; + for body in cases { + let names = hosts_for(tmp.path(), &body); + assert_eq!( + names, + ["vps", "work-test", "work-stage", "local-direct"], + "pattern did not match: {body}" + ); + } +} + +/// A home directory containing a glob metacharacter must not be treated as a +/// pattern: the base is a real path, only the `Include` value is a pattern. +#[test] +fn a_base_directory_containing_glob_metacharacters_still_resolves() { + let tmp = tempfile::tempdir().expect("tempdir"); + let awkward = tmp.path().join("us[er]"); + fs::create_dir_all(&awkward).expect("create awkward base"); + write_fixture(&awkward); + + let names = hosts_for(&awkward, "Include conf.d/*.conf"); + + assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); +} + +#[test] +fn a_second_wildcard_in_one_filename_matches() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let names = hosts_for(tmp.path(), "Include conf.d/*-work-*.conf"); + + assert_eq!(names, ["work-test", "work-stage", "local-direct"]); +} + +#[test] +fn several_pathnames_on_one_include_line_are_all_read() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let names = hosts_for( + tmp.path(), + "Include conf.d/10-vps.conf conf.d/20-work-test.conf", + ); + + assert_eq!(names, ["vps", "work-test", "local-direct"]); +} + +#[test] +fn a_quoted_pathname_keeps_its_spaces() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + fs::write( + tmp.path() + .join(".ssh") + .join("conf.d") + .join("with space.conf"), + "Host spaced\n HostName 10.0.0.4\n", + ) + .expect("write fixture"); + + let names = hosts_for(tmp.path(), "Include \"conf.d/with space.conf\""); + + assert_eq!(names, ["spaced", "local-direct"]); +} + +#[test] +fn an_include_inside_a_host_block_keeps_the_enclosing_host() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let config = tmp.path().join(".ssh").join("config"); + fs::write( + &config, + "Host gate\n Include conf.d/10-vps.conf\n HostName 10.0.0.9\n User ops\n", + ) + .expect("write config"); + + let hosts = load_from_file(&config).expect("parse config"); + let gate = hosts + .iter() + .find(|h| h.name == "gate") + .expect("enclosing host survives the Include"); + + assert_eq!(gate.hostname, "10.0.0.9"); + assert_eq!(gate.user, "ops"); + assert!(hosts.iter().any(|h| h.name == "vps")); +} + +#[test] +fn an_include_that_matches_nothing_is_a_no_op() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + + for body in ["Include conf.d/nope*.conf", "Include conf.d/absent.conf"] { + assert_eq!(hosts_for(tmp.path(), body), ["local-direct"]); + } +} + +#[test] +fn a_directory_matching_the_pattern_is_skipped() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + fs::create_dir_all(tmp.path().join(".ssh").join("conf.d").join("40-dir.conf")) + .expect("create decoy directory"); + + let names = hosts_for(tmp.path(), "Include conf.d/*.conf"); + + assert_eq!(names, ["vps", "work-test", "work-stage", "local-direct"]); +} + +#[test] +fn a_nested_relative_include_resolves_against_the_same_base() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let conf_d = tmp.path().join(".ssh").join("conf.d"); + fs::write( + conf_d.join("10-vps.conf"), + "Host vps\n HostName 1.2.3.4\nInclude conf.d/40-deep.conf\n", + ) + .expect("write 10-vps.conf"); + fs::write( + conf_d.join("40-deep.conf"), + "Host deep\n HostName 10.0.0.5\n", + ) + .expect("write 40-deep.conf"); + + let names = hosts_for(tmp.path(), "Include conf.d/10-vps.conf"); + + assert_eq!(names, ["vps", "deep", "local-direct"]); +} + +#[test] +fn an_include_cycle_terminates() { + let tmp = tempfile::tempdir().expect("tempdir"); + write_fixture(tmp.path()); + let conf_d = tmp.path().join(".ssh").join("conf.d"); + fs::write( + conf_d.join("a.conf"), + "Host a\n HostName 10.0.0.6\nInclude conf.d/b.conf\n", + ) + .expect("write a.conf"); + fs::write( + conf_d.join("b.conf"), + "Host b\n HostName 10.0.0.7\nInclude conf.d/a.conf\n", + ) + .expect("write b.conf"); + + let names = hosts_for(tmp.path(), "Include conf.d/a.conf"); + + assert_eq!(names, ["a", "b", "local-direct"]); +} diff --git a/crates/omnyssh-gui/src/commands/hosts.rs b/crates/omnyssh-gui/src/commands/hosts.rs index 302a267..7a6ef40 100644 --- a/crates/omnyssh-gui/src/commands/hosts.rs +++ b/crates/omnyssh-gui/src/commands/hosts.rs @@ -79,14 +79,22 @@ pub async fn delete_host(name: String) -> Result<(), CommandError> { /// Upsert `input` into the manual host list by name. A new name appends; an existing /// name is an in-place edit that **preserves every field the edit form cannot observe** /// — password, identity file, and proxy jump (the outbound `HostDto` omits all three, -/// §3.4, so the form leaves them blank on edit), plus key-setup metadata and the -/// SSH-config rename origin. Editing e.g. notes therefore never drops a stored secret -/// or a recorded key setup. A provided secret still overwrites the old one. +/// §3.4, so the form leaves them blank on edit), plus key-setup metadata, the +/// SSH-config rename origin, and a monitoring mode the payload left out. Editing +/// e.g. notes therefore never drops a stored secret or a recorded key setup. A +/// provided secret still overwrites the old one. fn upsert(hosts: &mut Vec, input: HostInputDto) { + // An omitted monitoring mode means "unchanged", not "back to SSH" — losing it + // would silently start logging in to a device chosen for reachability only. + let monitoring_given = input.monitoring.is_some(); let mut host = Host::from(input); match hosts.iter().position(|h| h.name == host.name) { Some(i) => { let existing = &hosts[i]; + if !monitoring_given { + host.monitoring = existing.monitoring; + host.monitor_port = existing.monitor_port; + } host.password = host.password.or_else(|| existing.password.clone()); host.identity_file = host .identity_file @@ -128,7 +136,8 @@ async fn persist(mutate: impl FnOnce(&mut Vec) + Send + 'static) -> Result #[cfg(test)] mod tests { use super::*; - use omnyssh_core::ssh::client::HostSource; + use crate::dto::MonitorModeDto; + use omnyssh_core::ssh::client::{HostSource, MonitorMode}; fn input(name: &str) -> HostInputDto { HostInputDto { @@ -141,6 +150,8 @@ mod tests { proxy_jump: None, tags: vec![], notes: None, + monitoring: None, + monitor_port: None, } } @@ -196,6 +207,41 @@ mod tests { assert_eq!(h.original_ssh_host.as_deref(), Some("web-old")); } + #[test] + fn upsert_keeps_a_monitoring_mode_the_payload_left_out() { + let mut hosts = vec![Host { + name: "fw".to_string(), + monitoring: MonitorMode::TcpPort, + monitor_port: Some(8443), + source: HostSource::Manual, + ..Host::default() + }]; + + upsert(&mut hosts, input("fw")); + + // Silently reverting to SSH would start logging in to a device the user + // deliberately put on a reachability probe. + assert_eq!(hosts[0].monitoring, MonitorMode::TcpPort); + assert_eq!(hosts[0].monitor_port, Some(8443)); + } + + #[test] + fn upsert_applies_a_monitoring_mode_the_payload_carries() { + let mut hosts = vec![Host { + name: "fw".to_string(), + monitoring: MonitorMode::TcpPort, + monitor_port: Some(8443), + source: HostSource::Manual, + ..Host::default() + }]; + + let mut back_to_ssh = input("fw"); + back_to_ssh.monitoring = Some(MonitorModeDto::Ssh); + upsert(&mut hosts, back_to_ssh); + + assert_eq!(hosts[0].monitoring, MonitorMode::Ssh); + } + #[test] fn upsert_overwrites_a_secret_when_a_new_one_is_provided() { let mut hosts = vec![Host { diff --git a/crates/omnyssh-gui/src/dto.rs b/crates/omnyssh-gui/src/dto.rs index 2df64f1..71bd7e5 100644 --- a/crates/omnyssh-gui/src/dto.rs +++ b/crates/omnyssh-gui/src/dto.rs @@ -9,7 +9,7 @@ use omnyssh_core::config::snippets::{Snippet, SnippetScope}; use omnyssh_core::event::{ DetectedService, MetricValue, Metrics, ProcessInfo, ServiceKind, ServiceMetric, }; -use omnyssh_core::ssh::client::{ConnectionStatus, Host, HostSource}; +use omnyssh_core::ssh::client::{ConnectionStatus, Host, HostSource, MonitorMode}; use omnyssh_core::ssh::key_setup::KeySetupStep; use omnyssh_core::ssh::sftp::FileEntry; use omnyssh_core::update::UpdateInfo; @@ -22,6 +22,33 @@ pub enum HostSourceDto { Manual, } +/// How a host is watched, mirrors `omnyssh_core::ssh::client::MonitorMode` +/// (tech-gui.md §4.1). `tcpPort` means reachability only — no login, no metrics. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, specta::Type)] +#[serde(rename_all = "camelCase")] +pub enum MonitorModeDto { + Ssh, + TcpPort, +} + +impl From for MonitorModeDto { + fn from(mode: MonitorMode) -> Self { + match mode { + MonitorMode::Ssh => Self::Ssh, + MonitorMode::TcpPort => Self::TcpPort, + } + } +} + +impl From for MonitorMode { + fn from(mode: MonitorModeDto) -> Self { + match mode { + MonitorModeDto::Ssh => Self::Ssh, + MonitorModeDto::TcpPort => Self::TcpPort, + } + } +} + /// A host as the frontend sees it — password and private-key material omitted /// (tech-gui.md §3.4). `hasKey` reports whether an identity file is configured; /// the key path itself never crosses the boundary. @@ -39,6 +66,9 @@ pub struct HostDto { pub has_key: bool, #[serde(skip_serializing_if = "Option::is_none")] pub password_auth_disabled: Option, + pub monitoring: MonitorModeDto, + #[serde(skip_serializing_if = "Option::is_none")] + pub monitor_port: Option, } /// Inbound host form payload for `save_host` (tech-gui.md §4.1, Stage 4.1). Builds a @@ -64,6 +94,10 @@ pub struct HostInputDto { pub tags: Vec, #[serde(default)] pub notes: Option, + #[serde(default)] + pub monitoring: Option, + #[serde(default)] + pub monitor_port: Option, } /// Live connection state for a host (tech-gui.md §4.1). Internally tagged so the @@ -258,6 +292,8 @@ impl From<&Host> for HostDto { source: (&host.source).into(), has_key: host.identity_file.is_some(), password_auth_disabled: host.password_auth_disabled, + monitoring: host.monitoring.into(), + monitor_port: host.monitor_port, } } } @@ -273,6 +309,7 @@ impl From for Host { // The frontend already trims; collapse an exact-empty string to `None` as a // last guard. Password is not trimmed — its bytes are preserved verbatim. let non_empty = |s: Option| s.filter(|v| !v.is_empty()); + let monitoring: MonitorMode = dto.monitoring.map(Into::into).unwrap_or_default(); Host { name: dto.name, hostname: dto.hostname, @@ -285,6 +322,12 @@ impl From for Host { notes: non_empty(dto.notes), source: HostSource::Manual, original_ssh_host: None, + monitoring, + // Port 0 is not dialable, and an SSH host has nothing to probe: drop + // both, so a later mode switch cannot inherit a stale target. + monitor_port: dto + .monitor_port + .filter(|&p| p != 0 && monitoring == MonitorMode::TcpPort), key_setup_date: None, password_auth_disabled: None, } @@ -532,6 +575,8 @@ mod tests { proxy_jump: Some("bastion".to_string()), tags: vec!["prod".to_string()], notes: Some("primary".to_string()), + monitoring: None, + monitor_port: None, } } @@ -583,6 +628,8 @@ mod tests { proxy_jump: Some(String::new()), tags: vec![], notes: Some(String::new()), + monitoring: None, + monitor_port: None, }); assert!(host.identity_file.is_none()); assert!(host.password.is_none()); diff --git a/crates/omnyssh-gui/ui/src/lib/bindings.ts b/crates/omnyssh-gui/ui/src/lib/bindings.ts index b7d4a1c..6c3af67 100644 --- a/crates/omnyssh-gui/ui/src/lib/bindings.ts +++ b/crates/omnyssh-gui/ui/src/lib/bindings.ts @@ -435,7 +435,7 @@ export type FilePreview = { sessionId: number; path: string; content: string } * (tech-gui.md §3.4). `hasKey` reports whether an identity file is configured; * the key path itself never crosses the boundary. */ -export type HostDto = { name: string; hostname: string; user: string; port: number; tags: string[]; notes?: string | null; source: HostSourceDto; hasKey: boolean; passwordAuthDisabled?: boolean | null } +export type HostDto = { name: string; hostname: string; user: string; port: number; tags: string[]; notes?: string | null; source: HostSourceDto; hasKey: boolean; passwordAuthDisabled?: boolean | null; monitoring: MonitorModeDto; monitorPort?: number | null } /** * Inbound host form payload for `save_host` (tech-gui.md §4.1, Stage 4.1). Builds a * **manual** `Host` — SSH-config hosts are read-only imports and are never saved. @@ -443,7 +443,7 @@ export type HostDto = { name: string; hostname: string; user: string; port: numb * travel back out: the outbound `HostDto` omits both (§3.4). Inbound only, so it * derives `Deserialize` (not `Serialize`). */ -export type HostInputDto = { name: string; hostname: string; user: string; port: number; identityFile?: string | null; password?: string | null; proxyJump?: string | null; tags: string[]; notes?: string | null } +export type HostInputDto = { name: string; hostname: string; user: string; port: number; identityFile?: string | null; password?: string | null; proxyJump?: string | null; tags: string[]; notes?: string | null; monitoring?: MonitorModeDto | null; monitorPort?: number | null } /** * Host origin, mirrors `omnyssh_core::ssh::client::HostSource`. */ @@ -492,6 +492,11 @@ export type MetricsDto = { cpuPercent?: number | null; ramPercent?: number | nul * A fresh metrics sample for a host (tech-gui.md §4.3). */ export type MetricsUpdated = { hostName: string; metrics: MetricsDto } +/** + * How a host is watched, mirrors `omnyssh_core::ssh::client::MonitorMode` + * (tech-gui.md §4.1). `tcpPort` means reachability only — no login, no metrics. + */ +export type MonitorModeDto = "ssh" | "tcpPort" /** * A single process in the "top processes" panel (tech-gui.md §4.1). */ diff --git a/crates/omnyssh-gui/ui/src/lib/ipc/router.test.ts b/crates/omnyssh-gui/ui/src/lib/ipc/router.test.ts index 25245ea..526dabf 100644 --- a/crates/omnyssh-gui/ui/src/lib/ipc/router.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/ipc/router.test.ts @@ -43,7 +43,8 @@ describe('ipc event router', () => { port: 22, tags: [], source: 'manual', - hasKey: false + hasKey: false, + monitoring: 'ssh' } ]; @@ -110,7 +111,7 @@ describe('ipc event router', () => { applyServicesDetected({ hostName: 'web-2', services: [{ kind: 'docker', metrics: [] }] }); applyHostsLoaded([ - { name: 'web-1', hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false } + { name: 'web-1', hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false, monitoring: 'ssh' } ]); expect(get(statuses).has('web-2')).toBe(false); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/Dashboard.svelte b/crates/omnyssh-gui/ui/src/lib/screens/Dashboard.svelte index 6d6aa5b..9406fba 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/Dashboard.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/Dashboard.svelte @@ -281,8 +281,15 @@ - - {#if card.offline} + + {#if card.reachability} +
+ {card.reachability}{card.host.monitorPort ? ` · port ${card.host.monitorPort}` : ''} +
+ {:else if card.offline}
offline
{:else}
diff --git a/crates/omnyssh-gui/ui/src/lib/screens/HostEditor.svelte b/crates/omnyssh-gui/ui/src/lib/screens/HostEditor.svelte index 3bb830e..50a9fb2 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/HostEditor.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/HostEditor.svelte @@ -135,6 +135,30 @@ +
+ + {#if fields.monitoring === 'tcpPort'} + + {/if} +
+ {#if fields.monitoring === 'tcpPort'} +

Checks the port only — no login, and no metrics on the card.

+ {/if} + {#if error}

{error}

{/if} diff --git a/crates/omnyssh-gui/ui/src/lib/screens/hostForm.test.ts b/crates/omnyssh-gui/ui/src/lib/screens/hostForm.test.ts index 5c5fe4c..b26a61a 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/hostForm.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/screens/hostForm.test.ts @@ -15,6 +15,7 @@ function host(partial: Partial): HostDto { tags: [], source: 'manual', hasKey: false, + monitoring: 'ssh', ...partial }; } @@ -124,7 +125,72 @@ describe('formFromHost', () => { identityFile: undefined, password: undefined, tags: ['ops'], - notes: 'x' + notes: 'x', + monitoring: 'ssh', + monitorPort: undefined }); }); }); + +describe('formToInput — monitoring mode', () => { + it('defaults to ssh and sends no probe port', () => { + const result = formToInput({ ...emptyForm(), name: 'web', hostname: '10.0.0.1' }); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.input.monitoring).toBe('ssh'); + expect(result.input.monitorPort).toBeUndefined(); + } + }); + + it('carries a probe port for a reachability host', () => { + const result = formToInput({ + ...emptyForm(), + name: 'fw', + hostname: '10.0.0.9', + monitoring: 'tcpPort', + monitorPort: '8443' + }); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.input.monitoring).toBe('tcpPort'); + expect(result.input.monitorPort).toBe(8443); + } + }); + + it('falls back to the host port when the probe port is blank', () => { + const result = formToInput({ ...emptyForm(), name: 'fw', hostname: '10.0.0.9', monitoring: 'tcpPort' }); + expect(result.ok).toBe(true); + if (result.ok) expect(result.input.monitorPort).toBeUndefined(); + }); + + it('rejects an out-of-range probe port', () => { + for (const monitorPort of ['0', '99999', 'ssh']) { + const result = formToInput({ + ...emptyForm(), + name: 'fw', + hostname: '10.0.0.9', + monitoring: 'tcpPort', + monitorPort + }); + expect(result.ok).toBe(false); + } + }); + + it('ignores a probe port left over from switching back to ssh', () => { + const result = formToInput({ + ...emptyForm(), + name: 'web', + hostname: '10.0.0.1', + monitoring: 'ssh', + monitorPort: '8443' + }); + expect(result.ok).toBe(true); + if (result.ok) expect(result.input.monitorPort).toBeUndefined(); + }); + + it('round-trips a reachability host through the edit form', () => { + const fields = formFromHost(host({ monitoring: 'tcpPort', monitorPort: 8443 })); + expect(fields.monitoring).toBe('tcpPort'); + expect(fields.monitorPort).toBe('8443'); + }); +}); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/hostForm.ts b/crates/omnyssh-gui/ui/src/lib/screens/hostForm.ts index eeb8788..7f32567 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/hostForm.ts +++ b/crates/omnyssh-gui/ui/src/lib/screens/hostForm.ts @@ -3,7 +3,7 @@ // validation mirrors the TUI's `HostForm::to_host` (crates/omnyssh/src/app/host.rs) // so both frontends produce the same `hosts.toml` shape and error messages. -import type { HostDto, HostInputDto } from '$lib/bindings'; +import type { HostDto, HostInputDto, MonitorModeDto } from '$lib/bindings'; /** The editable form fields — all raw text (tags are comma-separated, port a string). */ export interface HostFormFields { @@ -15,12 +15,26 @@ export interface HostFormFields { password: string; tags: string; notes: string; + monitoring: MonitorModeDto; + /** Probe port; blank means "the host's SSH port". Only read for `tcpPort`. */ + monitorPort: string; } export function emptyForm(): HostFormFields { // Port pre-seeded to the SSH default; user blank (placeholder shows `root`, the // default the validation applies when it is left empty). - return { name: '', hostname: '', user: '', port: '22', identityFile: '', password: '', tags: '', notes: '' }; + return { + name: '', + hostname: '', + user: '', + port: '22', + identityFile: '', + password: '', + tags: '', + notes: '', + monitoring: 'ssh', + monitorPort: '' + }; } /** Seed the edit form from a `HostDto`. `identityFile`/`password` are intentionally @@ -35,7 +49,9 @@ export function formFromHost(h: HostDto): HostFormFields { identityFile: '', password: '', tags: h.tags.join(', '), - notes: h.notes ?? '' + notes: h.notes ?? '', + monitoring: h.monitoring, + monitorPort: h.monitorPort == null ? '' : String(h.monitorPort) }; } @@ -72,6 +88,19 @@ export function formToInput(f: HostFormFields): HostFormResult { port = Number(portRaw); } + // Only meaningful for a reachability host; an SSH host never carries a probe port. + let monitorPort: number | undefined; + const monitorPortRaw = f.monitorPort.trim(); + if (f.monitoring === 'tcpPort' && monitorPortRaw !== '') { + if (!/^\+?\d+$/.test(monitorPortRaw) || Number(monitorPortRaw) < 1 || Number(monitorPortRaw) > 65535) { + return { + ok: false, + error: `Probe port must be a number between 1 and 65535, got '${monitorPortRaw}'` + }; + } + monitorPort = Number(monitorPortRaw); + } + const identityFile = f.identityFile.trim(); const password = f.password.trim(); const notes = f.notes.trim(); @@ -86,7 +115,9 @@ export function formToInput(f: HostFormFields): HostFormResult { identityFile: identityFile || undefined, password: password || undefined, tags, - notes: notes || undefined + notes: notes || undefined, + monitoring: f.monitoring, + monitorPort } }; } diff --git a/crates/omnyssh-gui/ui/src/lib/screens/serverCard.test.ts b/crates/omnyssh-gui/ui/src/lib/screens/serverCard.test.ts index e51d5b4..40489a2 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/serverCard.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/screens/serverCard.test.ts @@ -5,7 +5,11 @@ import type { HostServices } from '$lib/stores/services'; import { deriveCard, metricStatus, QUICK_ACTIONS, filterHosts } from './serverCard'; function host(name = 'web-1'): HostDto { - return { name, hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false }; + return { name, hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false, monitoring: 'ssh' }; +} + +function tcpHost(name = 'fw-1'): HostDto { + return { ...host(name), monitoring: 'tcpPort', monitorPort: 8443 }; } function metrics(partial: Partial): MetricsDto { @@ -198,3 +202,37 @@ describe('quick actions', () => { expect(get(sessions).map((s) => s.kind)).toEqual(['terminal', 'sftp']); }); }); + +describe('deriveCard — reachability hosts', () => { + it('shows the probe result instead of metric tiles', () => { + const card = deriveCard(tcpHost(), { kind: 'connected' }, undefined, undefined); + expect(card.reachability).toBe('reachable'); + expect(card.metricRows).toEqual([]); + expect(card.overall).toBe('ok'); + expect(card.offline).toBe(false); + }); + + it('reports a failed probe as unreachable and offline', () => { + const card = deriveCard(tcpHost(), { kind: 'failed', message: 'refused' }, undefined, undefined); + expect(card.reachability).toBe('unreachable'); + expect(card.offline).toBe(true); + expect(card.overall).toBe('off'); + }); + + it('is still checking before the first probe answers', () => { + expect(deriveCard(tcpHost(), { kind: 'connecting' }, undefined, undefined).reachability).toBe('checking'); + expect(deriveCard(tcpHost(), undefined, undefined, undefined).reachability).toBe('checking'); + }); + + it('never claims metrics an ssh host would have reported', () => { + const card = deriveCard(tcpHost(), { kind: 'connected' }, metrics({ cpuPercent: 90 }), undefined); + expect(card.metricRows).toEqual([]); + expect(card.uptime).toBeUndefined(); + }); + + it('leaves an ssh host on the metric path', () => { + const card = deriveCard(host(), { kind: 'connected' }, metrics({ cpuPercent: 10 }), undefined); + expect(card.reachability).toBeUndefined(); + expect(card.metricRows).toHaveLength(3); + }); +}); diff --git a/crates/omnyssh-gui/ui/src/lib/screens/serverCard.ts b/crates/omnyssh-gui/ui/src/lib/screens/serverCard.ts index 9960c82..9cd1c04 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/serverCard.ts +++ b/crates/omnyssh-gui/ui/src/lib/screens/serverCard.ts @@ -30,12 +30,17 @@ export type MetricRow = { label: string; percent: number | null; status: Status export type CardService = { kind: ServiceKindDto; name: string; detail: string }; +/** Reachability of a `tcpPort` host; `undefined` for an SSH-monitored one. */ +type Reachability = 'reachable' | 'unreachable' | 'checking'; + export interface ServerCard { host: HostDto; /** Header dot: connected health, `off` when failed, else neutral. */ overall: Status; /** Down/unprobed with no live metrics — the card shows an offline state. */ offline: boolean; + /** Set only for a reachability host, which has no metrics to show. */ + reachability?: Reachability; metricRows: MetricRow[]; uptime?: string; osInfo?: string; @@ -92,6 +97,21 @@ export function deriveCard( m: MetricsDto | undefined, svc: HostServices | undefined ): ServerCard { + // A reachability host is probed by a TCP connect and never reports metrics, so + // the tiles would be a fiction — the card shows the probe result instead. + if (host.monitoring !== 'ssh') { + const kind = status?.kind; + return { + host, + overall: kind === 'connected' ? 'ok' : kind === 'failed' ? 'off' : 'unknown', + offline: kind === 'failed', + reachability: kind === 'connected' ? 'reachable' : kind === 'failed' ? 'unreachable' : 'checking', + metricRows: [], + topProcesses: [], + detectedServices: [] + }; + } + const metricRows: MetricRow[] = [ metricRow('CPU', m?.cpuPercent), metricRow('RAM', m?.ramPercent), diff --git a/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.test.ts b/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.test.ts index 561ab42..cc9c4f9 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.test.ts @@ -3,7 +3,7 @@ import type { ConnectionStatusDto, HostDto, MetricsDto } from '$lib/bindings'; import { deriveHostSummary } from './hostSummary'; function host(name: string): HostDto { - return { name, hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false }; + return { name, hostname: '10.0.0.1', user: 'root', port: 22, tags: [], source: 'manual', hasKey: false, monitoring: 'ssh' }; } function metrics(partial: Partial): MetricsDto { @@ -118,3 +118,27 @@ describe('deriveHostSummary — partition invariant', () => { } }); }); + +describe('deriveHostSummary — reachability hosts', () => { + it('does not count a stale sample against a host that no longer reports metrics', () => { + const fw: HostDto = { ...host('fw'), monitoring: 'tcpPort' }; + const summary = deriveHostSummary( + [fw], + new Map([['fw', { kind: 'connected' } as ConnectionStatusDto]]), + // Left over from before the host was switched to a port check. + new Map([['fw', metrics({ cpuPercent: 92 })]]) + ); + + expect(summary).toEqual({ total: 1, online: 1, alert: 0, offline: 0 }); + }); + + it('still counts a breaching ssh host as an alert', () => { + const summary = deriveHostSummary( + [host('web')], + new Map([['web', { kind: 'connected' } as ConnectionStatusDto]]), + new Map([['web', metrics({ cpuPercent: 92 })]]) + ); + + expect(summary.alert).toBe(1); + }); +}); diff --git a/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.ts b/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.ts index 7fbafe9..dc73cfd 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/hostSummary.ts @@ -39,7 +39,10 @@ export function deriveHostSummary( let offline = 0; for (const host of hostList) { if (statusMap.get(host.name)?.kind === 'connected') { - if (isBreaching(metricMap.get(host.name))) alert++; + // A reachability host never refreshes metrics; a sample left over from + // before the switch would pin it in `alert` until the app restarts. + const sample = host.monitoring === 'ssh' ? metricMap.get(host.name) : undefined; + if (isBreaching(sample)) alert++; else online++; } else { offline++; diff --git a/crates/omnyssh-gui/ui/src/lib/stores/palette.test.ts b/crates/omnyssh-gui/ui/src/lib/stores/palette.test.ts index a143609..6fda2d5 100644 --- a/crates/omnyssh-gui/ui/src/lib/stores/palette.test.ts +++ b/crates/omnyssh-gui/ui/src/lib/stores/palette.test.ts @@ -13,6 +13,7 @@ function host(name: string, extra: Partial = {}): HostDto { tags: [], source: 'manual', hasKey: false, + monitoring: 'ssh', ...extra }; } diff --git a/crates/omnyssh/src/app/host.rs b/crates/omnyssh/src/app/host.rs index f730332..0545897 100644 --- a/crates/omnyssh/src/app/host.rs +++ b/crates/omnyssh/src/app/host.rs @@ -4,7 +4,7 @@ use std::time::Duration; use super::*; -use omnyssh_core::ssh::client::HostSource; +use omnyssh_core::ssh::client::{HostSource, MonitorMode}; // --------------------------------------------------------------------------- // Host form (used in Add / Edit popups) @@ -20,8 +20,33 @@ pub const FORM_FIELD_LABELS: &[&str] = &[ "Password (optional)", "Tags (comma-sep)", "Notes", + "Monitoring (ssh | tcp | tcp:PORT)", ]; +/// Renders a host's monitoring mode back into its form field. +fn monitoring_value(host: &Host) -> String { + match (host.monitoring, host.monitor_port) { + (MonitorMode::Ssh, _) => String::new(), + (MonitorMode::TcpPort, Some(port)) => format!("tcp:{port}"), + (MonitorMode::TcpPort, None) => String::from("tcp"), + } +} + +/// Parses the monitoring field: empty or `ssh` keeps the SSH poller, `tcp` +/// probes the host's SSH port, `tcp:PORT` probes another one. +fn parse_monitoring(value: &str) -> Result<(MonitorMode, Option), String> { + match value { + "" | "ssh" => Ok((MonitorMode::Ssh, None)), + "tcp" => Ok((MonitorMode::TcpPort, None)), + other => other + .strip_prefix("tcp:") + .and_then(|p| p.parse::().ok()) + .filter(|&p| p != 0) + .map(|p| (MonitorMode::TcpPort, Some(p))) + .ok_or_else(|| format!("Monitoring must be 'ssh', 'tcp' or 'tcp:PORT', got '{other}'")), + } +} + /// A single editable text field in the host form. #[derive(Debug, Clone, Default)] pub struct FormField { @@ -91,6 +116,7 @@ impl HostForm { form.fields[5] = FormField::with_value(host.password.as_deref().unwrap_or("")); form.fields[6] = FormField::with_value(host.tags.join(", ")); form.fields[7] = FormField::with_value(host.notes.as_deref().unwrap_or("")); + form.fields[8] = FormField::with_value(monitoring_value(host)); form } @@ -163,6 +189,8 @@ impl HostForm { } }; + let (monitoring, monitor_port) = parse_monitoring(self.fields[8].value.trim())?; + Ok(Host { name, hostname, @@ -175,6 +203,8 @@ impl HostForm { notes, source, original_ssh_host: None, + monitoring, + monitor_port, key_setup_date: None, password_auth_disabled: None, }) @@ -490,6 +520,21 @@ impl App { } self.save_manual_hosts().await; + + // The edit can change the address, the port or the monitoring + // mode, none of which a running poller picks up. + { + let state = self.state.read().await; + if let Some(old) = self.poll_manager.take() { + old.shutdown(); + } + self.poll_manager = Some(PollManager::start( + state.hosts.clone(), + self.core_tx.clone(), + Duration::from_secs(30), + )); + } + let state = self.state.read().await; self.view.host_list.rebuild_filter( &state.hosts, @@ -544,9 +589,10 @@ impl App { #[cfg(test)] mod tests { + use super::*; - /// Builds a host form from the 8 field values, in `FORM_FIELD_LABELS` order. + /// Builds a host form from the field values, in `FORM_FIELD_LABELS` order. fn host_form(values: [&str; 8]) -> HostForm { let mut form = HostForm::empty(); for (i, v) in values.iter().enumerate() { @@ -555,6 +601,36 @@ mod tests { form } + #[test] + fn the_monitoring_field_round_trips_through_the_form() { + for (text, mode, port) in [ + ("", MonitorMode::Ssh, None), + ("ssh", MonitorMode::Ssh, None), + ("tcp", MonitorMode::TcpPort, None), + ("tcp:8443", MonitorMode::TcpPort, Some(8443)), + ] { + let parsed = parse_monitoring(text).expect("valid monitoring value"); + assert_eq!(parsed, (mode, port), "parsing '{text}'"); + + let host = Host { + monitoring: mode, + monitor_port: port, + ..Host::default() + }; + assert_eq!(parse_monitoring(&monitoring_value(&host)), Ok((mode, port))); + } + } + + #[test] + fn an_unusable_monitoring_value_is_rejected() { + for text in ["tcp:0", "tcp:99999", "http", "tcp:"] { + assert!( + parse_monitoring(text).is_err(), + "'{text}' should be rejected" + ); + } + } + // --- HostForm::to_host (P0.2) ----------------------------------------- #[test] diff --git a/crates/omnyssh/src/ui/card.rs b/crates/omnyssh/src/ui/card.rs index ab6a388..8ab2584 100644 --- a/crates/omnyssh/src/ui/card.rs +++ b/crates/omnyssh/src/ui/card.rs @@ -14,7 +14,7 @@ use ratatui::{ use crate::ui::theme::threshold_color; use crate::ui::theme::Theme; use omnyssh_core::event::{DetectedService, Metrics, ServiceKind}; -use omnyssh_core::ssh::client::ConnectionStatus; +use omnyssh_core::ssh::client::{ConnectionStatus, MonitorMode}; // --------------------------------------------------------------------------- // Card dimensions (kept in sync with dashboard.rs column calculation) @@ -54,6 +54,25 @@ pub struct CardData<'a> { pub status: Option<&'a ConnectionStatus>, /// Detected services. pub services: Option<&'a [DetectedService]>, + /// How the host is watched. A reachability host has no metrics to show. + pub monitoring: MonitorMode, + /// Port the reachability probe dials, when it is not the host's own. + pub monitor_port: Option, +} + +/// The reachability line shown in place of the metric rows. Naming the probed +/// port matters: a green line for port 8443 says nothing about SSH on 22. +fn reachability_line(status: Option<&ConnectionStatus>, port: Option) -> (String, Color) { + let (state, color) = match status { + Some(ConnectionStatus::Connected) => ("reachable", Color::Green), + Some(ConnectionStatus::Failed(_)) => ("unreachable", Color::Red), + _ => ("checking", Color::DarkGray), + }; + let text = match port { + Some(p) => format!("─── {state} :{p} ───"), + None => format!("─── {state} ───"), + }; + (text, color) } /// Render a single server card into `rect`. @@ -162,7 +181,14 @@ pub fn render_card( Some(ConnectionStatus::Failed(_)) | Some(ConnectionStatus::Unknown) | None ) && metrics.is_none(); - if is_offline { + if data.monitoring != MonitorMode::Ssh { + // Reachability host: no metrics exist, so the tiles would be a fiction. + let (text, color) = reachability_line(status, data.monitor_port); + frame.render_widget( + Paragraph::new(Line::from(Span::styled(text, Style::default().fg(color)))), + rows[1], + ); + } else if is_offline { // Rows 1-2: offline message frame.render_widget( Paragraph::new(Line::from(Span::styled( diff --git a/crates/omnyssh/src/ui/dashboard.rs b/crates/omnyssh/src/ui/dashboard.rs index cd1fa67..c2c61a4 100644 --- a/crates/omnyssh/src/ui/dashboard.rs +++ b/crates/omnyssh/src/ui/dashboard.rs @@ -266,6 +266,8 @@ fn render_grid(frame: &mut Frame, area: Rect, state: &AppState, view: &ViewState metrics, status, services: state.services.get(&host.name).map(|s| s.as_slice()), + monitoring: host.monitoring, + monitor_port: host.monitor_port, }, is_selected, &view.theme, diff --git a/crates/omnyssh/src/ui/detail_view.rs b/crates/omnyssh/src/ui/detail_view.rs index 2c4b644..1ff0151 100644 --- a/crates/omnyssh/src/ui/detail_view.rs +++ b/crates/omnyssh/src/ui/detail_view.rs @@ -16,7 +16,7 @@ use crate::app::{AppAction, AppState, SnippetPopup, ViewState}; use crate::ui::theme::threshold_color; use crate::ui::theme::Theme; use omnyssh_core::event::{DetectedService, Metrics, ServiceKind}; -use omnyssh_core::ssh::client::ConnectionStatus; +use omnyssh_core::ssh::client::{ConnectionStatus, MonitorMode}; // --------------------------------------------------------------------------- // Render @@ -111,7 +111,13 @@ pub fn render(frame: &mut Frame, area: Rect, state: &AppState, view: &ViewState) // Separator render_separator(frame, sections[2], inner.width, &view.theme); - // Metrics + // Metrics — a reachability host has none, and the last SSH sample it may still + // carry is not current. + let metrics = if host.monitoring == MonitorMode::Ssh { + metrics + } else { + None + }; render_metrics_alerts(frame, sections[3], metrics, &view.theme); // Separator @@ -216,6 +222,13 @@ fn render_metrics_column(frame: &mut Frame, area: Rect, metrics: Option<&Metrics .add_modifier(Modifier::BOLD), ))]; + if metrics.is_none() { + lines.push(Line::from(Span::styled( + " unavailable", + Style::default().fg(theme.text_secondary), + ))); + } + if let Some(m) = metrics { // CPU line with bar if let Some(cpu) = m.cpu_percent { diff --git a/crates/omnyssh/src/ui/popup.rs b/crates/omnyssh/src/ui/popup.rs index f63eb8b..3c73014 100644 --- a/crates/omnyssh/src/ui/popup.rs +++ b/crates/omnyssh/src/ui/popup.rs @@ -316,7 +316,15 @@ pub fn render_help(frame: &mut Frame, theme: &Theme) { /// `title` is either `"Add Host"` or `"Edit Host"`. pub fn render_host_form(frame: &mut Frame, form: &HostForm, title: &str, theme: &Theme) { // Taller popup to fit all fields. - let area = centred_rect(70, 80, frame.area()); + // One row for each label and input, plus top padding, the hint and its spacer, + // and the border. Sizing to that rather than to a fixed share of the screen + // keeps the last field on screen when the terminal is short. + let frame_area = frame.area(); + let needed = u32::from(FORM_FIELD_LABELS.len() as u16) * 2 + 5; + let percent = (needed * 100) + .div_ceil(u32::from(frame_area.height.max(1))) + .clamp(50, 100) as u16; + let area = centred_rect(70, percent, frame_area); frame.render_widget(Clear, area); let block = Block::default()