diff --git a/cmd/crates/soroban-test/tests/it/doctor.rs b/cmd/crates/soroban-test/tests/it/doctor.rs new file mode 100644 index 000000000..0d4cec02f --- /dev/null +++ b/cmd/crates/soroban-test/tests/it/doctor.rs @@ -0,0 +1,236 @@ +/* +`doctor` reports on things it discovers outside the process: which Stellar CLI +executables are on `PATH`, and which CLI last refreshed the shared version +cache. Both inputs are injectable -- `PATH` like in `plugin.rs`, and the cache +via `STELLAR_DATA_HOME` -- so the reporting can be exercised end to end. + +Unix only: the fake CLIs are shell scripts that need an execute bit. +*/ + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; + +use assert_cmd::Command; +use predicates::prelude::PredicateBooleanExt; +use predicates::str::contains; +use soroban_cli::commands::version::pkg; +use soroban_test::TestEnv; + +/// A stand-in CLI on `PATH` that answers version queries and nothing else. +/// +/// `supports_only_version` distinguishes a current CLI from one old enough to +/// reject `version --only-version` -- the releases that cause the version +/// confusion these checks exist to surface only answer `--version`. +fn write_fake_cli(dir: &Path, name: &str, version: &str, supports_only_version: bool) { + let only_version = if supports_only_version { + format!("echo \"{version}\"") + } else { + "echo \"error: unexpected argument '--only-version' found\" >&2; exit 2".to_string() + }; + + let script = format!( + "#!/bin/sh\n\ + case \"$*\" in\n\ + \"version --only-version\") {only_version} ;;\n\ + \"--version\") echo \"stellar {version} (0000000000000000000000000000000000000000)\" ;;\n\ + *) echo \"unexpected args: $*\" >&2; exit 1 ;;\n\ + esac\n" + ); + + let path = dir.join(name); + fs::write(&path, script).unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o755)).unwrap(); +} + +fn empty_dir(sandbox: &TestEnv, name: &str) -> PathBuf { + let dir = sandbox.dir().join(name); + fs::create_dir_all(&dir).unwrap(); + dir +} + +/// The path `doctor` sees as its own executable, in the canonicalized form it +/// compares cache entries against. +fn running_binary() -> String { + let path = assert_cmd::cargo::cargo_bin("stellar"); + let path = path.canonicalize().unwrap_or(path); + path.to_string_lossy().into_owned() +} + +/// Seed the shared version cache with a given writer, and return the data home +/// that holds it. +/// +/// `latest_check_time` is irrelevant to what is asserted here: `doctor` calls +/// `has_available_upgrade` with caching off, so it always attempts a refresh. +/// The seeded writer is still what gets reported, because `doctor` reads it +/// before that refresh can overwrite it. +fn seed_cache_writer(sandbox: &TestEnv, writer: serde_json::Value) -> PathBuf { + let data_home = empty_dir(sandbox, "cache-data-home"); + + let cache = serde_json::json!({ + "latest_check_time": "2026-08-04T10:00:00Z", + "max_stable_version": "27.1.0", + "max_version": "27.1.0", + "last_checked_by": writer, + }); + + fs::write( + data_home.join("upgrade_check.json"), + serde_json::to_string(&cache).unwrap(), + ) + .unwrap(); + + data_home +} + +/// `doctor` with `PATH` and the version cache pointed at test-controlled state. +fn doctor(sandbox: &TestEnv, path: &Path, data_home: &Path) -> Command { + let mut cmd = sandbox.new_assert_cmd("doctor"); + cmd.env("PATH", path).env("STELLAR_DATA_HOME", data_home); + cmd +} + +#[test] +fn reports_the_running_executable_and_a_lone_install() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "one-install"); + let data_home = empty_dir(&sandbox, "data-home"); + write_fake_cli(&bin_dir, "stellar", "27.1.0", true); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains(format!("Running executable: {}", running_binary()))) + .stderr(contains("Only one Stellar CLI found on PATH")); +} + +#[test] +fn reports_when_no_cli_is_on_path() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "no-installs"); + let data_home = empty_dir(&sandbox, "data-home"); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains("No Stellar CLI found on PATH")) + // Nothing was found, so claiming a single install would be a lie. + .stderr(contains("Only one Stellar CLI").not()); +} + +#[test] +fn does_not_warn_when_both_binary_names_report_the_same_version() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "two-installs-agreeing"); + let data_home = empty_dir(&sandbox, "data-home"); + // What an ordinary install looks like: one crate, two binary names. + write_fake_cli(&bin_dir, "stellar", "27.1.0", true); + write_fake_cli(&bin_dir, "soroban", "27.1.0", true); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains( + "2 Stellar CLI executables on PATH, all reporting 27.1.0", + )) + .stderr(contains("different versions").not()); +} + +#[test] +fn warns_when_installs_report_different_versions() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "two-installs-disagreeing"); + let data_home = empty_dir(&sandbox, "data-home"); + write_fake_cli(&bin_dir, "stellar", "27.1.0", true); + // Old enough to only answer `--version`, which is the fallback path. + write_fake_cli(&bin_dir, "soroban", "22.8.0", false); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains( + "Found 2 Stellar CLI executables on PATH reporting different versions", + )) + .stderr(contains(format!( + "- {} (27.1.0)", + bin_dir.join("stellar").to_string_lossy() + ))) + .stderr(contains(format!( + "- {} (22.8.0)", + bin_dir.join("soroban").to_string_lossy() + ))); +} + +#[test] +fn confirms_the_cache_writer_when_it_is_this_cli() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "bin"); + let data_home = seed_cache_writer( + &sandbox, + serde_json::json!({ "version": pkg(), "executable": running_binary() }), + ); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains(format!( + "Version cache last refreshed by: {} ({})", + pkg(), + running_binary() + ))); +} + +#[test] +fn does_not_warn_when_the_same_install_wrote_the_cache_at_an_older_version() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "bin"); + // An in-place upgrade: same path, earlier version recorded against it. + let data_home = seed_cache_writer( + &sandbox, + serde_json::json!({ "version": "26.1.0", "executable": running_binary() }), + ); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains(format!( + "Version cache was last refreshed by this install running 26.1.0; it is now {}", + pkg() + ))) + .stderr(contains("a different Stellar CLI").not()); +} + +#[test] +fn warns_when_a_different_install_wrote_the_cache() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "bin"); + let data_home = seed_cache_writer( + &sandbox, + serde_json::json!({ "version": "22.8.0", "executable": "/opt/elsewhere/bin/soroban" }), + ); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains( + "Version cache was last refreshed by a different Stellar CLI: \ + 22.8.0 (/opt/elsewhere/bin/soroban)", + )) + .stderr(contains(format!("this one is {} (", pkg()))); +} + +#[test] +fn reports_an_unknown_cache_writer_without_claiming_a_mismatch() { + let sandbox = TestEnv::default(); + let bin_dir = empty_dir(&sandbox, "bin"); + // A cache written before the writer was recorded. + let data_home = seed_cache_writer(&sandbox, serde_json::Value::Null); + + doctor(&sandbox, &bin_dir, &data_home) + .assert() + .success() + .stderr(contains( + "Version cache was last refreshed by an unknown Stellar CLI", + )) + .stderr(contains("a different Stellar CLI").not()); +} diff --git a/cmd/crates/soroban-test/tests/it/main.rs b/cmd/crates/soroban-test/tests/it/main.rs index 61b62b6b6..cd666961a 100644 --- a/cmd/crates/soroban-test/tests/it/main.rs +++ b/cmd/crates/soroban-test/tests/it/main.rs @@ -2,6 +2,8 @@ mod build; mod config; #[cfg(unix)] mod container; +#[cfg(unix)] +mod doctor; #[cfg(feature = "emulator-tests")] mod emulator; mod help; diff --git a/cmd/soroban-cli/src/cli.rs b/cmd/soroban-cli/src/cli.rs index 3a1c378eb..3b806b0d8 100644 --- a/cmd/soroban-cli/src/cli.rs +++ b/cmd/soroban-cli/src/cli.rs @@ -77,8 +77,9 @@ pub async fn main() { // Spawn a thread to check if a new version exists. // It depends on logger, so we need to place it after // the code block that initializes the logger. - tokio::spawn(async move { - upgrade_check(root.global_args.quiet).await; + let quiet = root.global_args.quiet; + let upgrade_check_handle = tokio::spawn(async move { + upgrade_check(quiet).await; }); let printer = Print::new(root.global_args.quiet); @@ -106,6 +107,30 @@ pub async fn main() { printer.errorln(format!("error: {e}")); std::process::exit(1); } + + finish_upgrade_check(upgrade_check_handle).await; +} + +// Returning from `main` ends the runtime, so a still-running upgrade check is +// dropped where it stands. For a command that finishes faster than the request +// to crates.io, that meant the fetched versions were never written to the cache +// and the next run started over -- the check could keep re-fetching and keep +// reporting whatever stale versions the cache already held. +// +// Give it a brief chance to land instead. The fetch runs alongside the command, +// so by this point it has usually already finished and this returns +// immediately; the wait only bites when the check actually went to the network, +// which is at most once a day. Dropping it after the grace period is no worse +// than the unconditional drop it replaces. +const UPGRADE_CHECK_GRACE: std::time::Duration = std::time::Duration::from_secs(2); + +async fn finish_upgrade_check(handle: tokio::task::JoinHandle<()>) { + if tokio::time::timeout(UPGRADE_CHECK_GRACE, handle) + .await + .is_err() + { + tracing::debug!("upgrade check did not finish within its grace period"); + } } // Load config.toml defaults as env vars, honoring --config-dir if present in raw args. diff --git a/cmd/soroban-cli/src/commands/doctor.rs b/cmd/soroban-cli/src/commands/doctor.rs index b2d2ea989..3f7d192c8 100644 --- a/cmd/soroban-cli/src/commands/doctor.rs +++ b/cmd/soroban-cli/src/commands/doctor.rs @@ -2,6 +2,7 @@ use clap::Parser; use rustc_version::version; use semver::Version; use std::fmt::Debug; +use std::path::{Path, PathBuf}; use std::process::Command; use crate::{ @@ -10,10 +11,11 @@ use crate::{ self, data, locator::{self, KeyType}, network::{Network, DEFAULTS as DEFAULT_NETWORKS}, + upgrade_check::{CheckWriter, UpgradeCheck}, }, print::Print, rpc, - upgrade_check::has_available_upgrade, + upgrade_check::{check_performed_by, has_available_upgrade, running_binary, upgrade_message}, utils::url::redact_url, }; @@ -46,7 +48,14 @@ impl Cmd { pub async fn run(&self, _global_args: &global::Args) -> Result<(), Error> { let print = Print::new(false); + // Read this before `check_version`, which refreshes the cache and would + // otherwise record this very run as the writer -- hiding the mismatch + // the report exists to reveal. + let previous_cache_writer = version_cache_writer(); + check_version(&print).await?; + check_installs(&print); + show_version_cache_writer(&print, previous_cache_writer.as_ref()); check_rust_version(&print); check_wasm_target(&print); check_optional_features(&print); @@ -149,9 +158,7 @@ async fn check_version(print: &Print) -> Result<(), Error> { has_available_upgrade(false).await { if upgrade_available { - print.warnln(format!( - "A new release of Stellar CLI is available: {current_version} -> {latest_version}" - )); + print.warnln(upgrade_message(¤t_version, &latest_version)); } else { print.checkln(format!( "You are using the latest version of Stellar CLI: {current_version}" @@ -162,6 +169,187 @@ async fn check_version(print: &Print) -> Result<(), Error> { Ok(()) } +/// Which CLI last refreshed the shared version cache, if it recorded itself. +fn version_cache_writer() -> Option { + UpgradeCheck::load().ok()?.last_checked_by +} + +/// Report which CLI last refreshed the shared version cache. +/// +/// Every install writes the same file, so the entry that decides when the next +/// check happens -- and the versions any warning is built from -- may have come +/// from a different install than the one being run. When it did, say so plainly: +/// that mismatch is the signal worth surfacing. +/// +/// Identity is the executable path, not the recorded version. An in-place +/// upgrade leaves an older version recorded against the very path now running, +/// and that is one install, not two -- treating it as a mismatch would warn +/// about every upgrade until the cache next refreshed. +fn show_version_cache_writer(print: &Print, writer: Option<&CheckWriter>) { + let Some(writer) = writer else { + // Either no cache yet or one written before the CLI recorded this, so + // there is nothing to report rather than something being wrong. + print.infoln("Version cache was last refreshed by an unknown Stellar CLI".to_string()); + return; + }; + + let this_cli = check_performed_by(); + + match (&writer.executable, &this_cli.executable) { + (Some(written_by), Some(running)) if written_by == running => { + if writer.version == this_cli.version { + print.checkln(format!("Version cache last refreshed by: {writer}")); + } else { + // Same install, earlier version: the ordinary state after an + // upgrade, and it corrects itself at the next refresh. + let recorded = writer.version.as_deref().unwrap_or("an unknown version"); + let running = this_cli.version.as_deref().unwrap_or("unknown"); + + print.infoln(format!( + "Version cache was last refreshed by this install running {recorded}; \ + it is now {running}" + )); + } + } + (Some(_), Some(_)) => { + print.warnln(format!( + "Version cache was last refreshed by a different Stellar CLI: {writer}" + )); + print.blankln(format!("this one is {this_cli}")); + } + // Without both paths there is no identity to compare, so report the + // writer without claiming it was or was not this install. + _ => print.infoln(format!("Version cache was last refreshed by: {writer}")), + } +} + +/// The binary names this CLI ships under. Both are built from the same crate, +/// so a stale `soroban` runs the same upgrade check as a current `stellar` and +/// reports its own, older version. +const CLI_BINARY_NAMES: [&str; 2] = ["stellar", "soroban"]; + +/// Report the running executable and every other Stellar CLI on `PATH`. +/// +/// Several executables at *different versions* is the case that makes the +/// upgrade warning look wrong: an old binary correctly reports its own old +/// version, but the user compares it against whichever binary +/// `stellar --version` resolves to and sees a contradiction. Listing them makes +/// that visible. +/// +/// Count alone is not the signal. This crate ships two binaries, `stellar` and +/// `soroban`, so a single healthy install puts two files on `PATH` -- warning +/// about that would flag nearly every install. Warn when the versions actually +/// disagree. +fn check_installs(print: &Print) { + match running_binary() { + Some(binary) => print.infoln(format!("Running executable: {binary}")), + None => print.warnln("Could not determine the running executable".to_string()), + } + + let installs = find_installs(); + + let mut versions = installs.iter().map(|(_, version)| version.as_deref()); + let common_version = match versions.next() { + // Every executable answered, and answered the same: one version is in + // play however many files carry it. + Some(Some(first)) if versions.all(|version| version == Some(first)) => Some(first), + _ => None, + }; + + match (installs.len(), common_version) { + (0, _) => print.warnln( + "No Stellar CLI found on PATH; the running executable is not reachable by name" + .to_string(), + ), + (1, _) => print.checkln("Only one Stellar CLI found on PATH".to_string()), + (count, Some(version)) => { + print.checkln(format!( + "{count} Stellar CLI executables on PATH, all reporting {version}:" + )); + list_installs(print, &installs); + } + (count, None) => { + print.warnln(format!( + "Found {count} Stellar CLI executables on PATH reporting different versions; \ + an outdated one can report a version that disagrees with `stellar --version`:" + )); + list_installs(print, &installs); + } + } +} + +/// Every distinct Stellar CLI executable reachable by name on `PATH`, with the +/// version it reports. +fn find_installs() -> Vec<(PathBuf, Option)> { + let mut installs: Vec<(PathBuf, Option)> = Vec::new(); + + for name in CLI_BINARY_NAMES { + let Ok(paths) = which::which_all(name) else { + continue; + }; + + for path in paths { + // `which_all` yields one entry per matching `PATH` element, so the + // same binary shows up repeatedly when `PATH` has duplicates. + let key = path.canonicalize().unwrap_or_else(|_| path.clone()); + if installs.iter().any(|(seen, _)| *seen == key) { + continue; + } + + let version = installed_version(&path); + installs.push((key, version)); + } + } + + installs +} + +fn list_installs(print: &Print, installs: &[(PathBuf, Option)]) { + for (path, version) in installs { + let version = version.as_deref().unwrap_or("unknown version"); + print.blankln(format!("- {} ({version})", path.to_string_lossy())); + } +} + +/// Ask a CLI executable for its version. Returns `None` if it cannot be run or +/// does not answer like a Stellar CLI. +fn installed_version(path: &Path) -> Option { + // `--only-version` predates neither every release nor every binary name: + // releases old enough to cause the version confusion this check exists to + // surface reject the flag, so fall back to parsing the full version banner. + run_version(path, &["version", "--only-version"]) + .and_then(|output| parse_only_version(&output)) + .or_else(|| run_version(path, &["--version"]).and_then(|o| parse_version_banner(&o))) +} + +fn run_version(path: &Path, args: &[&str]) -> Option { + let output = Command::new(path).args(args).output().ok()?; + + if output.status.success() { + Some(String::from_utf8_lossy(&output.stdout).into_owned()) + } else { + None + } +} + +fn parse_only_version(output: &str) -> Option { + let version = output.trim(); + + // An older CLI treats `--only-version` as an unknown argument and may still + // exit zero while printing usage, so require a bare version. + Version::parse(version).ok().map(|_| version.to_string()) +} + +/// Pull the version out of a banner like `stellar 22.8.0 (18f54cd...)`. +fn parse_version_banner(output: &str) -> Option { + output + .lines() + .next()? + .split_whitespace() + .find(|token| Version::parse(token).is_ok()) + .map(ToString::to_string) +} + fn check_rust_version(print: &Print) { match version() { Ok(rust_version) => { @@ -266,3 +454,38 @@ fn get_expected_wasm_target() -> String { "wasm32v1-none".into() } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_bare_only_version_output() { + assert_eq!(parse_only_version("27.1.0\n").as_deref(), Some("27.1.0")); + } + + #[test] + fn rejects_usage_text_printed_for_an_unknown_flag() { + // A CLI old enough to reject `--only-version` must not be reported as + // if the usage text were its version. + let usage = "error: unexpected argument '--only-version' found\n\n\ + Usage: soroban version [OPTIONS]\n"; + + assert_eq!(parse_only_version(usage), None); + } + + #[test] + fn parses_version_from_an_older_cli_banner() { + // Real output from a 22.8.0 `soroban`, which predates `--only-version`. + let banner = "stellar 22.8.0 (18f54cdc0342726eb13ac14f84e899703d9f180a)\n\ + stellar-xdr 22.1.0 (e139229708)\n\ + xdr curr (529d5176f2)\n"; + + assert_eq!(parse_version_banner(banner).as_deref(), Some("22.8.0")); + } + + #[test] + fn returns_none_for_a_banner_without_a_version() { + assert_eq!(parse_version_banner("not a stellar cli\n"), None); + } +} diff --git a/cmd/soroban-cli/src/config/upgrade_check.rs b/cmd/soroban-cli/src/config/upgrade_check.rs index cfa9b40a0..9ef605e65 100644 --- a/cmd/soroban-cli/src/config/upgrade_check.rs +++ b/cmd/soroban-cli/src/config/upgrade_check.rs @@ -10,6 +10,34 @@ use super::data::project_dir; const FILE_NAME: &str = "upgrade_check.json"; +/// The CLI that last refreshed the shared version cache. +/// +/// Version and executable are kept apart on purpose. The executable is the +/// installation identity: an in-place upgrade changes the version at one path +/// without becoming a second install, so only the path can answer "was this a +/// different CLI?". The version is diagnostic detail carried alongside it. +#[derive(Serialize, Deserialize, Debug, PartialEq, Eq, Clone, Default)] +pub struct CheckWriter { + /// The version of the CLI that wrote the file, if it recorded one. + #[serde(default)] + pub version: Option, + /// The canonicalized path of the executable that wrote the file, if it + /// could be resolved. + #[serde(default)] + pub executable: Option, +} + +impl std::fmt::Display for CheckWriter { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match (&self.version, &self.executable) { + (Some(version), Some(executable)) => write!(f, "{version} ({executable})"), + (Some(version), None) => write!(f, "{version} (unknown executable)"), + (None, Some(executable)) => write!(f, "unknown version ({executable})"), + (None, None) => write!(f, "an unidentified Stellar CLI"), + } + } +} + /// The `UpgradeCheck` struct represents the state of the upgrade check. /// This state is global and stored in the `upgrade_check.json` file in /// the global configuration directory. @@ -21,6 +49,19 @@ pub struct UpgradeCheck { pub max_stable_version: Version, /// The latest version of the CLI available on crates.io, including pre-releases. pub max_version: Version, + /// Which CLI last refreshed this file. + /// + /// Every install shares this one file, and both the `stellar` and `soroban` + /// binaries are built from the same crate, so the entry that paces the next + /// check may well have been written by a different -- possibly much older -- + /// CLI than the one reading it. Recording the writer makes that visible in + /// `stellar doctor` instead of leaving it to be guessed at. + /// + /// `None` for files written before this field existed. Nothing keys a + /// decision off it: it is diagnostic only, so an absent value cannot change + /// whether an upgrade is reported. + #[serde(default)] + pub last_checked_by: Option, } impl Default for UpgradeCheck { @@ -29,6 +70,7 @@ impl Default for UpgradeCheck { latest_check_time: DateTime::::UNIX_EPOCH, max_stable_version: Version::new(0, 0, 0), max_version: Version::new(0, 0, 0), + last_checked_by: None, } } } @@ -72,6 +114,56 @@ mod tests { use serial_test::serial; use std::env; + /// A file written before `last_checked_by` existed must still load, and the + /// absent field must not disturb any of the values the check reasons about. + #[test] + fn test_loads_legacy_file_without_last_checked_by() { + let legacy = r#"{ + "latest_check_time": "2026-08-04T10:00:00Z", + "max_stable_version": "27.1.0", + "max_version": "27.1.0" + }"#; + + let check: UpgradeCheck = serde_json::from_str(legacy).unwrap(); + + assert_eq!( + check.latest_check_time, + "2026-08-04T10:00:00Z".parse::>().unwrap() + ); + assert_eq!(check.max_stable_version, Version::new(27, 1, 0)); + assert_eq!(check.max_version, Version::new(27, 1, 0)); + assert_eq!(check.last_checked_by, None); + } + + /// The writer is diagnostic, so a file that recorded only part of it has to + /// load rather than fail the whole check. + #[test] + fn test_loads_partially_recorded_writer() { + let partial = r#"{ + "latest_check_time": "2026-08-04T10:00:00Z", + "max_stable_version": "27.1.0", + "max_version": "27.1.0", + "last_checked_by": { "version": "27.1.0" } + }"#; + + let check: UpgradeCheck = serde_json::from_str(partial).unwrap(); + let writer = check.last_checked_by.unwrap(); + + assert_eq!(writer.version.as_deref(), Some("27.1.0")); + assert_eq!(writer.executable, None); + assert_eq!(writer.to_string(), "27.1.0 (unknown executable)"); + } + + #[test] + fn test_writer_displays_version_and_executable() { + let writer = CheckWriter { + version: Some("27.1.0".to_string()), + executable: Some("/usr/local/bin/stellar".to_string()), + }; + + assert_eq!(writer.to_string(), "27.1.0 (/usr/local/bin/stellar)"); + } + #[test] #[serial] fn test_upgrade_check_load_save() { @@ -95,6 +187,10 @@ mod tests { latest_check_time: DateTime::::from_timestamp(1_234_567_890, 0).unwrap(), max_stable_version: Version::new(1, 2, 3), max_version: Version::parse("1.2.4-rc.1").unwrap(), + last_checked_by: Some(CheckWriter { + version: Some("1.2.3".to_string()), + executable: Some("/usr/local/bin/stellar".to_string()), + }), }; saved_check.save().unwrap(); let loaded_check = UpgradeCheck::load().unwrap(); diff --git a/cmd/soroban-cli/src/upgrade_check.rs b/cmd/soroban-cli/src/upgrade_check.rs index a27e57d32..8557d9806 100644 --- a/cmd/soroban-cli/src/upgrade_check.rs +++ b/cmd/soroban-cli/src/upgrade_check.rs @@ -1,4 +1,4 @@ -use crate::config::upgrade_check::UpgradeCheck; +use crate::config::upgrade_check::{CheckWriter, UpgradeCheck}; use crate::print::Print; use crate::utils::http; use semver::Version; @@ -7,7 +7,13 @@ use std::error::Error; use std::io::IsTerminal; use std::time::Duration; -const MINIMUM_CHECK_INTERVAL: Duration = Duration::from_hours(24); // 1 day +// One day. +const MINIMUM_CHECK_INTERVAL: Duration = Duration::from_hours(24); +// The shared HTTP client only bounds how long connecting may take, so a server +// that accepts the connection and then stalls would leave the request hanging +// indefinitely. Bound the whole request: this is a background nicety, and it +// must not be able to outlive the command it is running alongside. +const FETCH_TIMEOUT: Duration = Duration::from_secs(5); const CRATES_IO_API_URL: &str = "https://crates.io/api/v1/crates/"; const NO_UPDATE_CHECK_ENV_VAR: &str = "STELLAR_NO_UPDATE_CHECK"; @@ -25,12 +31,32 @@ struct Crate { max_version: Version, // This is the latest version, including pre-releases } +/// The path of the executable that is running, if it can be resolved. +/// +/// `current_version` comes from `env!("CARGO_PKG_VERSION")`, so it describes +/// the binary that is running and nothing else. When more than one Stellar CLI +/// is installed (a stale `soroban` alongside a current `stellar`, or a Homebrew +/// install shadowed by a `cargo install` one), an old binary reports its own +/// old version and the message reads as though it were about the CLI the user +/// thinks they are running. Naming the executable makes the warning say which +/// install it is actually about. +/// +/// Canonicalized, so that the value written to the shared cache and the value +/// compared against it later describe the same path in the same form. +pub fn running_binary() -> Option { + let path = std::env::current_exe().ok()?; + let path = path.canonicalize().unwrap_or(path); + + Some(path.to_string_lossy().into_owned()) +} + /// Fetch the latest stable version of the crate from crates.io async fn fetch_latest_crate_info() -> Result> { let crate_name = env!("CARGO_PKG_NAME"); let url = format!("{CRATES_IO_API_URL}{crate_name}"); let resp = http::client() .get(url) + .timeout(FETCH_TIMEOUT) .send() .await? .json::() @@ -38,6 +64,26 @@ async fn fetch_latest_crate_info() -> Result> { Ok(resp.crate_) } +/// How this CLI identifies itself as the writer of the shared cache file. +pub fn check_performed_by() -> CheckWriter { + CheckWriter { + version: Some(crate::commands::version::pkg().to_string()), + executable: running_binary(), + } +} + +/// The upgrade warning, naming the executable it refers to when that can be +/// resolved. +pub fn upgrade_message(current_version: &Version, latest_version: &Version) -> String { + let message = + format!("A new release of Stellar CLI is available: {current_version} -> {latest_version}"); + + match running_binary() { + Some(binary) => format!("{message} ({binary})"), + None => message, + } +} + /// Print a warning if a new version of the CLI is available pub async fn upgrade_check(quiet: bool) { // We should skip the upgrade check if we're not in a tty environment. @@ -55,9 +101,7 @@ pub async fn upgrade_check(quiet: bool) { if let Ok((true, current_version, latest_version)) = has_available_upgrade(true).await { let printer = Print::new(quiet); - printer.warnln(format!( - "A new release of Stellar CLI is available: {current_version} -> {latest_version}" - )); + printer.warnln(upgrade_message(¤t_version, &latest_version)); } tracing::debug!("finished upgrade check"); @@ -82,6 +126,7 @@ pub async fn has_available_upgrade( latest_check_time: now, max_stable_version: c.max_stable_version, max_version: c.max_version, + last_checked_by: Some(check_performed_by()), }; } Err(e) => { @@ -89,6 +134,10 @@ pub async fn has_available_upgrade( // Only update the latest check time if the fetch failed // This way we don't spam the user with errors stats.latest_check_time = now; + // A failed attempt still paces the next one, so record who + // paced it -- otherwise the file credits whichever install + // last succeeded, which may not be the one holding it back. + stats.last_checked_by = Some(check_performed_by()); } } @@ -137,6 +186,7 @@ mod tests { latest_check_time: chrono::Utc::now(), max_stable_version: Version::parse("1.0.0").unwrap(), max_version: Version::parse("1.1.0-rc.1").unwrap(), + last_checked_by: None, }; // When using a non-preview version @@ -155,6 +205,43 @@ mod tests { assert_eq!(*latest_version, Version::parse("1.1.0-rc.1").unwrap()); } + #[test] + fn test_upgrade_message_names_the_running_binary() { + let current = Version::parse("22.1.0").unwrap(); + let latest = Version::parse("23.3.0").unwrap(); + let binary = running_binary().expect("test binary path should resolve"); + + let message = upgrade_message(¤t, &latest); + + assert!( + message.starts_with("A new release of Stellar CLI is available: 22.1.0 -> 23.3.0"), + "unexpected message: {message}" + ); + // Without this, a stale install's warning is indistinguishable from the + // current install's -- the confusion reported in #2464. + assert!( + message.contains(&binary), + "message should name the running binary, got: {message}" + ); + } + + #[test] + fn test_check_performed_by_identifies_version_and_executable() { + let writer = check_performed_by(); + let binary = running_binary().expect("test binary path should resolve"); + + // `doctor` compares the executable against the one stored in the shared + // cache to decide whether another *install* wrote it, and reports the + // version alongside it. They are recorded separately so that an in-place + // upgrade -- same path, new version -- is not mistaken for a second + // install. + assert_eq!( + writer.version.as_deref(), + Some(crate::commands::version::pkg()) + ); + assert_eq!(writer.executable.as_deref(), Some(binary.as_str())); + } + #[test] fn test_semver_compare() { assert!(Version::parse("0.1.0").unwrap() < Version::parse("0.2.0").unwrap());