diff --git a/cmd/soroban-cli/src/cli.rs b/cmd/soroban-cli/src/cli.rs index 3a1c378eb..1dec3848c 100644 --- a/cmd/soroban-cli/src/cli.rs +++ b/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,12 +77,22 @@ 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); - if let Err(e) = root.run().await { + let run_result = root.run().await; + + // Every branch below this point exits via `std::process::exit`, which + // terminates the process without running anything after it -- including a + // call placed after this block. Finishing the upgrade check here, before + // any of those exits, is what makes it run on error paths too, not only + // when the command succeeds. + finish_upgrade_check(upgrade_check_handle).await; + + if let Err(e) = run_result { // TODO: source is None (should be HelpMessage) let _source = commands::Error::source(&e); // TODO use source instead @@ -108,6 +118,37 @@ pub async fn main() { } } +// 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. fn set_env_from_config() { let config_file = config_dir_from_raw_args() @@ -192,3 +233,17 @@ fn set_env_value_from_config(name: &str, value: Option) 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); + } +} diff --git a/cmd/soroban-cli/src/upgrade_check.rs b/cmd/soroban-cli/src/upgrade_check.rs index a27e57d32..9516f066e 100644 --- a/cmd/soroban-cli/src/upgrade_check.rs +++ b/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,12 +31,45 @@ 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 { + 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> { 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 +77,18 @@ async fn fetch_latest_crate_info() -> Result> { 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(¤t_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(¤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_semver_compare() { assert!(Version::parse("0.1.0").unwrap() < Version::parse("0.2.0").unwrap());