Scope
This concerns code introduced by #2670, which is still open — run_version is not on main today. Filing it separately because the fix is a behavioral change that should not ride along inside a diagnostics-wording PR, and because the review thread that raised it currently reads as resolved when it is not.
What happens
doctor probes every Stellar CLI executable it finds on PATH to read its version. The probe spawns a child process and waits for it with no upper bound:
fn run_version(path: &Path, args: &[&str]) -> Option<String> {
let output = Command::new(path).args(args).output().ok()?;
if output.status.success() {
Some(String::from_utf8_lossy(&output.stdout).into_owned())
} else {
None
}
}
Command::output() blocks until the child exits and its stdout/stderr reach EOF. If any matching executable on PATH never exits — a stale shell wrapper, a broken install, a script blocked on stdin, a binary waiting on a network call — doctor hangs indefinitely with no output and no way to tell the user which executable is responsible.
It is reached twice per discovered executable — once for version --only-version and once for the --version banner fallback — so a single bad entry on PATH is enough to wedge the command.
Why it matters here specifically
doctor is the command a user runs because something is already wrong with their installation. A wedged PATH entry is exactly the kind of broken state it exists to diagnose, and it is the one state in which the command cannot report anything at all.
There is also an internal irony worth naming: the surrounding code already models "this executable did not answer" as a first-class outcome. find_installs returns Option<String> per executable, and summarize_versions distinguishes an observed version disagreement from executables that could not be asked. A timed-out probe fits that existing vocabulary exactly — it is an executable that did not answer — so the reporting side needs no new concepts.
Suggested fix
Bound the probe: spawn the child, wait with a short timeout (a couple of seconds is generous for --version), and on expiry kill and reap it, returning None so the executable is reported as unknown-version through the path that already exists for that case. Reaping matters — dropping a Child does not kill it, so a naive timeout leaves the process behind.
Worth deciding as part of the fix whether a timed-out probe should be reported distinctly from one that failed to spawn or exited non-zero. Both are "did not answer", but only the timeout points at an executable that is itself hung, which is actionable information for the user.
Credit
Originally raised by Copilot in review on #2670: #2670 (comment)
Scope
This concerns code introduced by #2670, which is still open —
run_versionis not onmaintoday. Filing it separately because the fix is a behavioral change that should not ride along inside a diagnostics-wording PR, and because the review thread that raised it currently reads as resolved when it is not.What happens
doctorprobes every Stellar CLI executable it finds onPATHto read its version. The probe spawns a child process and waits for it with no upper bound:Command::output()blocks until the child exits and its stdout/stderr reach EOF. If any matching executable onPATHnever exits — a stale shell wrapper, a broken install, a script blocked on stdin, a binary waiting on a network call —doctorhangs indefinitely with no output and no way to tell the user which executable is responsible.It is reached twice per discovered executable — once for
version --only-versionand once for the--versionbanner fallback — so a single bad entry onPATHis enough to wedge the command.Why it matters here specifically
doctoris the command a user runs because something is already wrong with their installation. A wedgedPATHentry is exactly the kind of broken state it exists to diagnose, and it is the one state in which the command cannot report anything at all.There is also an internal irony worth naming: the surrounding code already models "this executable did not answer" as a first-class outcome.
find_installsreturnsOption<String>per executable, andsummarize_versionsdistinguishes an observed version disagreement from executables that could not be asked. A timed-out probe fits that existing vocabulary exactly — it is an executable that did not answer — so the reporting side needs no new concepts.Suggested fix
Bound the probe: spawn the child, wait with a short timeout (a couple of seconds is generous for
--version), and on expiry kill and reap it, returningNoneso the executable is reported as unknown-version through the path that already exists for that case. Reaping matters — dropping aChilddoes not kill it, so a naive timeout leaves the process behind.Worth deciding as part of the fix whether a timed-out probe should be reported distinctly from one that failed to spawn or exited non-zero. Both are "did not answer", but only the timeout points at an executable that is itself hung, which is actionable information for the user.
Credit
Originally raised by Copilot in review on #2670: #2670 (comment)