Identify which CLI an upgrade warning is about

cmd/soroban-cli/src/cli.rs

@@ -10,7 +10,7 @@ use crate::commands::contract::Error::{Deploy, Invoke};
 use crate::commands::Error::Contract;
 use crate::config::{locator::cli_config_file, Config};
 use crate::print::Print;
-use crate::upgrade_check::upgrade_check;
+use crate::upgrade_check::{upgrade_check, FETCH_TIMEOUT};
 use crate::{commands, env_vars, Root};
 use std::error::Error;
 
@@ -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,39 @@ 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.
+//
+// Must outlast `FETCH_TIMEOUT`: the fetch itself is allowed to take that long,
+// and a grace period shorter than it would give up before a slow-but-successful
+// check could write its result to the cache -- reintroducing the very bug this
+// exists to fix.
+const UPGRADE_CHECK_GRACE: std::time::Duration =
+    FETCH_TIMEOUT.saturating_add(std::time::Duration::from_secs(1));
+
+async fn finish_upgrade_check(handle: tokio::task::JoinHandle<()>) {
+    match tokio::time::timeout(UPGRADE_CHECK_GRACE, handle).await {
+        Ok(Ok(())) => {}
+        Ok(Err(join_err)) => {
+            tracing::debug!("upgrade check task failed: {join_err}");
+        }
+        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.
@@ -192,3 +226,17 @@ fn set_env_value_from_config<T: std::fmt::Display>(name: &str, value: Option<T>)
         std::env::set_var(format!("{name}_SOURCE"), "use");
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // The grace period exists so a fetch that its own timeout allowed to
+    // succeed always has room to write its result. A grace shorter than
+    // `FETCH_TIMEOUT` would give up on exactly the slow-but-successful check
+    // this is meant to rescue, reintroducing the bug it fixes.
+    #[test]
+    fn the_grace_period_outlasts_the_fetch_it_waits_on() {
+        assert!(UPGRADE_CHECK_GRACE > FETCH_TIMEOUT);
+    }
+}

cmd/soroban-cli/src/upgrade_check.rs

@@ -8,6 +8,12 @@ use std::io::IsTerminal;
 use std::time::Duration;
 
 const MINIMUM_CHECK_INTERVAL: Duration = Duration::from_hours(24); // 1 day
+
+// 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.
+pub 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,19 +31,64 @@ 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.
+///
+/// Left as the process was started from, not canonicalized. Resolving symlinks
+/// would name a path that moves under a normal in-place upgrade wherever a
+/// package manager installs through one: Homebrew keeps
+/// `/opt/homebrew/bin/stellar` pointing into a versioned Cellar directory and
+/// retargets it on upgrade, so the canonical path changes while the install
+/// does not. The path the user invokes outlives the file it happens to point
+/// at, which is what makes it the install's identity.
+///
+/// It is also the more useful thing to name in a warning: it is the path the
+/// user can act on, rather than one they never typed.
+///
+/// This only reaches as far as the platform allows. Linux resolves
+/// `current_exe` through `/proc/self/exe` before we see it, so a retargeted
+/// symlink still reads as a new install there; nothing in-process can recover
+/// the invoked path once the kernel has resolved it. Not canonicalizing keeps
+/// the platforms that do hand us the invoked path from losing it too.
+pub fn running_binary() -> Option<String> {
+    let path = std::env::current_exe().ok()?;
+
+    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<Crate, Box<dyn Error>> {
     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::<CrateResponse>()
         .await?;
     Ok(resp.crate_)
 }
 
+/// 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 +106,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(&current_version, &latest_version));
     }
 
     tracing::debug!("finished upgrade check");
@@ -155,6 +204,26 @@ 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(&current, &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_semver_compare() {
         assert!(Version::parse("0.1.0").unwrap() < Version::parse("0.2.0").unwrap());