From f749c90d87d82effc2c1092baf99c5ef8ca9513f Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 17:19:52 +0000 Subject: [PATCH] Keep the install marker once the script has touched the venv install.sh decides on elevation at :1931, before it creates the venv at :2120, so only a spawn failure and that exit prove pip never ran. A nonzero exit or a failed wait can land part-way through replacing a package, and the runtime probe does not reach transitive dependencies, so clearing the marker there throws away the only remaining signal. Also treat a hung CLI as stale on its own timeout rather than falling through to two more probes, and let repair report an environment the backend rejects instead of reinstalling over a healthy tree. --- studio/src-tauri/src/commands.rs | 51 ++++++++++++++++++++++- studio/src-tauri/src/install.rs | 16 +++---- studio/src-tauri/src/preflight.rs | 31 +++++++++++++- studio/src-tauri/src/preflight/managed.rs | 17 +++++++- 4 files changed, 105 insertions(+), 10 deletions(-) diff --git a/studio/src-tauri/src/commands.rs b/studio/src-tauri/src/commands.rs index b9edcf2da7..813d8c6ec8 100644 --- a/studio/src-tauri/src/commands.rs +++ b/studio/src-tauri/src/commands.rs @@ -18,6 +18,21 @@ async fn managed_install_ready_after_repair() -> bool { crate::preflight::managed_install_ready().await } +/// The update can install the newer CLI and only then reach a rejected +/// environment value. No reinstall changes that, so report it instead of +/// running the bundled installer over a healthy tree. +fn unrepairable_after_update(reason: Option<&str>) -> Option { + match reason { + Some(crate::preflight::STUDIO_RUNTIME_STARTUP_FAILED) => Some( + "Unsloth is installed, but the backend refuses to start with the current \ + environment settings (for example UNSLOTH_CPU_THREADS). Fix or unset them, \ + then try again." + .to_string(), + ), + _ => None, + } +} + fn should_emit_repair_failed(msg: &str) -> bool { !msg.contains("NEEDS_ELEVATION") } @@ -555,8 +570,27 @@ pub async fn start_managed_repair( .await .map_err(|e| format!("Repair update task panicked: {e}"))?; + let post_update = if matches!(update_result, Ok(())) { + Some(crate::preflight::managed_install_state().await) + } else { + None + }; + if let Some(Err(reason)) = post_update.as_ref() { + if let Some(msg) = unrepairable_after_update(reason.as_deref()) { + warn!("Managed repair stopping: {}", msg); + diagnostics::finish_repair_group( + &diagnostics_state, + &repair_group_id, + "failed", + Some(msg.clone()), + ); + let _ = app.emit("repair-failed", &msg); + return Err(msg); + } + } + match update_result { - Ok(()) if managed_install_ready_after_repair().await => { + Ok(()) if matches!(post_update, Some(Ok(()))) => { info!("Managed repair complete after update"); diagnostics::finish_repair_group( &diagnostics_state, @@ -757,6 +791,21 @@ mod tests { .unwrap_err(); assert!(error.contains("Directory does not exist")); } + #[test] + fn rejected_settings_stop_repair_instead_of_reinstalling() { + // The update can install the newer CLI and only then reach the bad + // value, so this arm is the one that would otherwise reinstall over a + // healthy tree and fail again the same way. + let msg = + super::unrepairable_after_update(Some(crate::preflight::STUDIO_RUNTIME_STARTUP_FAILED)) + .expect("a rejected setting must stop repair"); + assert!(msg.contains("UNSLOTH_CPU_THREADS")); + assert!( + super::unrepairable_after_update(Some("studio_runtime_missing_dependency")).is_none() + ); + assert!(super::unrepairable_after_update(None).is_none()); + } + #[test] fn repair_elevation_is_not_a_terminal_repair_failure() { assert!(!super::should_emit_repair_failed("NEEDS_ELEVATION")); diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index dee2481a6e..3d4f8a3e9d 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -82,11 +82,12 @@ fn clear_install_in_progress_marker() -> Result<(), String> { clear_install_in_progress_marker_at(&path) } -/// The marker means "a run started and never reported an outcome", so every -/// terminal outcome clears it, failures included. The runtime probe is the -/// backstop for a broken venv, whereas a marker left behind by a failed update -/// pins a working install into a repair it may not be able to finish offline. -/// Never fatal: losing the marker costs a fast path, not correctness. +/// Clear only where the script provably never touched the venv: it failed to +/// spawn, or it exited asking for elevation, which install.sh decides (:1931) +/// before it creates the venv (:2120). Once the script is running, any failure +/// can leave pip part-way through replacing a package, and the runtime probe +/// does not reach transitive dependencies, so the marker is the only signal +/// left. Never fatal: losing it costs a fast path, not correctness. fn clear_install_marker_best_effort() { if let Err(msg) = clear_install_in_progress_marker() { warn!("[install] {}", msg); @@ -578,7 +579,8 @@ fn run_install_with_event_mode( let _ = app.emit(event_mode.needs_elevation_event(), &packages); Err("NEEDS_ELEVATION".to_string()) } else { - clear_install_marker_best_effort(); + // Keep the marker: the script ran, so pip may have replaced or + // removed packages before it failed. let msg = format!("Installer exited with code {}", code); diagnostics::finish_attempt( &diagnostics, @@ -601,7 +603,7 @@ fn run_install_with_event_mode( Err(msg) } Err(msg) => { - clear_install_marker_best_effort(); + // Same: the wait failed but the script was already running. diagnostics::finish_attempt(&diagnostics, &attempt, None, false, Some(msg.clone())); clear_current_attempt(&state); if event_mode.emit_terminal_events() { diff --git a/studio/src-tauri/src/preflight.rs b/studio/src-tauri/src/preflight.rs index fd596f4cd5..599f5c0fa2 100644 --- a/studio/src-tauri/src/preflight.rs +++ b/studio/src-tauri/src/preflight.rs @@ -8,8 +8,8 @@ use crate::desktop_backend_owner::{ }; use backend::probe_existing_backends; use log::warn; -pub use managed::managed_install_ready; use managed::probe_managed_install; +pub use managed::{managed_install_ready, managed_install_state}; use std::path::PathBuf; use types::{BackendProbe, ManagedProbe}; pub use types::{DesktopPreflightDisposition, DesktopPreflightResult, ExternalBackendConflict}; @@ -680,6 +680,35 @@ exit 1 } } + #[cfg(unix)] + #[tokio::test] + async fn a_hung_cli_is_stale_without_running_the_legacy_fallback() { + // An older CLI rejects the unknown command at once, so only a broken one + // reaches the timeout. Retrying it with two more 10s probes would treble + // the wait before repair on exactly the installs that need it soonest. + let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; + let _cache_home = ManagedCapabilityCacheHome::new("runtime-hang"); + remove_managed_capability_cache(); + + let fake = fake_cli( + "runtime-hang", + r#"#!/bin/sh +sleep 120 +"#, + ); + let started = std::time::Instant::now(); + let probe = probe_managed_bin(fake.bin.clone()).await; + assert!( + matches!(&probe, ManagedProbe::Stale { reason, .. } if reason == "studio_runtime_probe_timeout"), + "a hung CLI must be stale on the timeout, got {probe:?}" + ); + assert!( + started.elapsed() < std::time::Duration::from_secs(20), + "the legacy fallback ran after the timeout, tripling the wait" + ); + remove_managed_capability_cache(); + } + #[cfg(unix)] #[tokio::test] async fn managed_runtime_probe_survives_more_stdout_than_the_pipe_holds() { diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index af722eef83..08cca786ce 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -303,6 +303,11 @@ async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { } } +/// A hung binary, kept out of the fallback below: an older CLI rejects the +/// unknown command at once, so only a broken one reaches the timeout, and +/// running two more probes on it would treble the wait before repair. +const RUNTIME_PROBE_TIMEOUT: &str = "studio_runtime_probe_timeout"; + /// True when the CLI is older than `desktop-runtime-check` yet still launches. /// Such a CLI exits with a usage error and no JSON, which is indistinguishable /// from a crashed probe; `--help` resolves the command without running it, and @@ -367,7 +372,7 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { "Managed runtime probe timed out in {}ms", started.elapsed().as_millis() ); - return Err("studio_runtime_probe_failed".to_string()); + return Err(RUNTIME_PROBE_TIMEOUT.to_string()); } }; @@ -606,3 +611,13 @@ pub(super) async fn probe_managed_install() -> ManagedProbe { pub async fn managed_install_ready() -> bool { matches!(probe_managed_install().await, ManagedProbe::Ready { .. }) } + +/// Ready, or the reason it is not, so repair can tell an unrepairable cause +/// apart from a stale install rather than reinstalling over both. +pub async fn managed_install_state() -> Result<(), Option> { + match probe_managed_install().await { + ManagedProbe::Ready { .. } => Ok(()), + ManagedProbe::Stale { reason, .. } => Err(Some(reason)), + _ => Err(None), + } +}