From bd2acf7a10a329cf9f036b2513de5b29ba0f7945 Mon Sep 17 00:00:00 2001 From: Dione-b Date: Sat, 8 Aug 2026 23:51:18 -0300 Subject: [PATCH 1/2] Say which CLI an upgrade warning is about, and let the check finish The warning names a version but not the install it came from, and on a machine with more than one Stellar CLI those are different questions. A stale `soroban` left in `~/.cargo/bin` by an old `cargo install`, or a Homebrew install shadowed by a newer one, prints "a new release is available: 22.1.0 -> 23.3.0" using its own version -- and the user compares it against `stellar --version`, which answers 25.2.0. The numbers disagree because two binaries are talking, which the message gives no way to see. It now names the executable it is about. The reported latest version could also be stale on its own. The check runs in a background task that `main` drops on return, so a command finishing faster than the request to crates.io left the fetched versions unwritten and the next run starting over -- re-fetching each time while continuing to report whatever the cache already held. The task now gets a grace period to land its result. That grace has to outlast the fetch it waits on, so the fetch needed a bound of its own: the shared HTTP client only limits how long connecting may take, so a server that accepts and then stalls could hang the request indefinitely, and no grace period can be chosen against a wait with no ceiling. With the fetch capped at 5s the grace is that plus a second, which cannot expire before a fetch its own timeout allowed to succeed. The cost is on the first command of the day, and only when the check actually goes to the network: up to 6s, where the previous behaviour was to drop the check. `STELLAR_NO_UPDATE_CHECK` still turns it off entirely. This does not reach warnings printed by already-released binaries -- an old `soroban` will keep printing its old unannotated message, since the fix has to be in the binary doing the printing. What it fixes is every warning from this release forward. Co-authored-by: Nearx-Labs --- cmd/soroban-cli/src/cli.rs | 54 ++++++++++++++++++-- cmd/soroban-cli/src/upgrade_check.rs | 75 ++++++++++++++++++++++++++-- 2 files changed, 123 insertions(+), 6 deletions(-) diff --git a/cmd/soroban-cli/src/cli.rs b/cmd/soroban-cli/src/cli.rs index 3a1c378eba..083399ecae 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,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(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 a27e57d327..9516f066e4 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()); From 076c11e5f5c987c8fe114307715753b767d0c237 Mon Sep 17 00:00:00 2001 From: Dione-b Date: Sun, 9 Aug 2026 13:45:31 -0300 Subject: [PATCH 2/2] Let the upgrade check finish on error exits too finish_upgrade_check awaited the background task's grace period after root.run(), but every branch of the error handling above it exits via std::process::exit, which ends the process without running anything placed after it. Any command that returned an error -- a bad flag, a failed simulation, an unreachable RPC -- skipped the grace period entirely and killed the check before it could write its result to the cache, reproducing the exact bug this was meant to fix. Capturing root.run()'s result first and finishing the check before acting on it means every exit path waits on it once, not only the success path. Co-authored-by: Nearx-Labs --- cmd/soroban-cli/src/cli.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/cmd/soroban-cli/src/cli.rs b/cmd/soroban-cli/src/cli.rs index 083399ecae..1dec3848c5 100644 --- a/cmd/soroban-cli/src/cli.rs +++ b/cmd/soroban-cli/src/cli.rs @@ -83,7 +83,16 @@ pub async fn main() { }); 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 @@ -107,8 +116,6 @@ 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