From a94c5b061b149e88ce5d800ecc3e2ed9126a42db Mon Sep 17 00:00:00 2001 From: Wasim Yousef Said Date: Mon, 27 Jul 2026 10:36:22 +0200 Subject: [PATCH 01/16] Fix interrupted Studio install recovery --- studio/src-tauri/src/commands.rs | 101 +++-- studio/src-tauri/src/install.rs | 105 +++++ studio/src-tauri/src/preflight.rs | 71 ++- studio/src-tauri/src/preflight/managed.rs | 94 +++- studio/src-tauri/src/process.rs | 417 +++++++++++++----- unsloth_cli/commands/studio.py | 57 +++ .../tests/test_studio_runtime_readiness.py | 85 ++++ 7 files changed, 738 insertions(+), 192 deletions(-) create mode 100644 unsloth_cli/tests/test_studio_runtime_readiness.py diff --git a/studio/src-tauri/src/commands.rs b/studio/src-tauri/src/commands.rs index 72ea9d6985..b9edcf2da7 100644 --- a/studio/src-tauri/src/commands.rs +++ b/studio/src-tauri/src/commands.rs @@ -532,57 +532,70 @@ pub async fn start_managed_repair( let repair_group_id = install::take_pending_repair_group_for_resume(&install_state) .unwrap_or_else(|| diagnostics::begin_repair_group(&diagnostics_state)); - let _ = app.emit("repair-progress", "Updating existing Unsloth install..."); - let update_app = app.clone(); - let update_state = update_state.inner().clone(); - let update_diagnostics = diagnostics_state.clone(); - let update_repair_group_id = repair_group_id.clone(); - let update_result = tokio::task::spawn_blocking(move || { - update::run_backend_update_for_repair( - update_app, - update_state, - update_diagnostics, - update_repair_group_id, - ) - }) - .await - .map_err(|e| format!("Repair update task panicked: {e}"))?; + if install::managed_install_in_progress() { + info!("Interrupted installation detected; skipping incremental repair update"); + let _ = app.emit( + "repair-progress", + "Previous installation was interrupted. Running bundled installer...", + ); + } else { + let _ = app.emit("repair-progress", "Updating existing Unsloth install..."); + let update_app = app.clone(); + let update_state = update_state.inner().clone(); + let update_diagnostics = diagnostics_state.clone(); + let update_repair_group_id = repair_group_id.clone(); + let update_result = tokio::task::spawn_blocking(move || { + update::run_backend_update_for_repair( + update_app, + update_state, + update_diagnostics, + update_repair_group_id, + ) + }) + .await + .map_err(|e| format!("Repair update task panicked: {e}"))?; - match update_result { - Ok(()) if managed_install_ready_after_repair().await => { - info!("Managed repair complete after update"); - diagnostics::finish_repair_group(&diagnostics_state, &repair_group_id, "success", None); - let _ = app.emit("repair-complete", ()); - return Ok(()); - } - Ok(()) => { - warn!("Managed repair update finished, but preflight is still not ready; falling back to installer"); - let _ = app.emit( - "repair-progress", - "Update finished, but Unsloth is still not ready. Running bundled installer...", - ); - } - Err(msg) => { - if msg.to_ascii_lowercase().contains("already running") { - error!("Managed repair update conflict: {}", msg); + match update_result { + Ok(()) if managed_install_ready_after_repair().await => { + info!("Managed repair complete after update"); diagnostics::finish_repair_group( &diagnostics_state, &repair_group_id, - "failed", - Some(msg.clone()), + "success", + None, ); - let _ = app.emit("repair-failed", &msg); - return Err(msg); + let _ = app.emit("repair-complete", ()); + return Ok(()); } + Ok(()) => { + warn!("Managed repair update finished, but preflight is still not ready; falling back to installer"); + let _ = app.emit( + "repair-progress", + "Update finished, but Unsloth is still not ready. Running bundled installer...", + ); + } + Err(msg) => { + if msg.to_ascii_lowercase().contains("already running") { + error!("Managed repair update conflict: {}", msg); + diagnostics::finish_repair_group( + &diagnostics_state, + &repair_group_id, + "failed", + Some(msg.clone()), + ); + let _ = app.emit("repair-failed", &msg); + return Err(msg); + } - warn!( - "Managed repair update failed, falling back to bundled installer: {}", - msg - ); - let _ = app.emit( - "repair-progress", - "Update failed. Running bundled installer...", - ); + warn!( + "Managed repair update failed, falling back to bundled installer: {}", + msg + ); + let _ = app.emit( + "repair-progress", + "Update failed. Running bundled installer...", + ); + } } } diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index 024b730735..72c610ffcc 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -7,6 +7,8 @@ use std::process::{Command, ExitStatus, Stdio}; use std::sync::{Arc, Mutex}; use tauri::{AppHandle, Emitter, Manager}; +const INSTALL_IN_PROGRESS_MARKER: &str = ".desktop-install-in-progress"; + // ── Types ── pub struct InstallProcess { @@ -38,6 +40,56 @@ pub fn new_install_state() -> InstallState { use crate::process::trim_line_endings; +fn install_in_progress_marker_path() -> Result { + let home = dirs::home_dir().ok_or("Could not determine home directory")?; + Ok(home + .join(".unsloth") + .join("studio") + .join(INSTALL_IN_PROGRESS_MARKER)) +} + +pub(crate) fn managed_install_in_progress() -> bool { + install_in_progress_marker_path() + .map(|path| path.is_file()) + .unwrap_or(false) +} + +fn create_install_in_progress_marker() -> Result<(), String> { + let path = install_in_progress_marker_path()?; + create_install_in_progress_marker_at(&path) +} + +fn create_install_in_progress_marker_at(path: &Path) -> Result<(), String> { + let parent = path + .parent() + .ok_or_else(|| format!("Invalid install marker path: {}", path.display()))?; + std::fs::create_dir_all(parent) + .map_err(|e| format!("Failed to create {}: {}", parent.display(), e))?; + + match std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path) + { + Ok(_) => Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => Ok(()), + Err(error) => Err(format!("Failed to create {}: {}", path.display(), error)), + } +} + +fn clear_install_in_progress_marker() -> Result<(), String> { + let path = install_in_progress_marker_path()?; + clear_install_in_progress_marker_at(&path) +} + +fn clear_install_in_progress_marker_at(path: &Path) -> Result<(), String> { + match std::fs::remove_file(path) { + Ok(()) => Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(format!("Failed to remove {}: {}", path.display(), error)), + } +} + // ── Script Resolution ── /// Returns (script_path, args) depending on dev vs production mode. @@ -443,6 +495,21 @@ fn run_install_with_event_mode( &format!("Using script: {}", script.display()), ); + if let Err(msg) = create_install_in_progress_marker() { + diagnostics::finish_attempt( + &diagnostics, + &attempt, + None, + false, + Some(format!("create_install_marker: {msg}")), + ); + clear_current_attempt(&state); + if event_mode.emit_terminal_events() { + emit_failed(&app, &msg); + } + return Err(msg); + } + let (stdout, stderr) = match spawn_script(&script, &args, &state) { Ok(handles) => handles, Err(msg) => { @@ -475,6 +542,20 @@ fn run_install_with_event_mode( match result { Ok((status, _)) if status.success() => { + if let Err(msg) = clear_install_in_progress_marker() { + diagnostics::finish_attempt( + &diagnostics, + &attempt, + Some(status.to_string()), + false, + Some(format!("clear_install_marker: {msg}")), + ); + clear_current_attempt(&state); + if event_mode.emit_terminal_events() { + emit_failed(&app, &msg); + } + return Err(msg); + } diagnostics::finish_attempt( &diagnostics, &attempt, @@ -877,6 +958,30 @@ fn capped_output_text(bytes: &[u8]) -> String { mod tests { use super::*; + #[test] + fn install_marker_persists_until_explicit_success_cleanup() { + let directory = std::env::temp_dir().join(format!( + "unsloth-install-marker-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let marker = directory.join(INSTALL_IN_PROGRESS_MARKER); + + create_install_in_progress_marker_at(&marker).unwrap(); + assert!(marker.is_file()); + + create_install_in_progress_marker_at(&marker).unwrap(); + assert!(marker.is_file()); + + clear_install_in_progress_marker_at(&marker).unwrap(); + assert!(!marker.exists()); + clear_install_in_progress_marker_at(&marker).unwrap(); + let _ = std::fs::remove_dir_all(directory); + } + #[test] fn elevated_output_cap_is_utf8_boundary_safe() { let text = "é".repeat(40_000); diff --git a/studio/src-tauri/src/preflight.rs b/studio/src-tauri/src/preflight.rs index 7ef5244754..1e129e85b0 100644 --- a/studio/src-tauri/src/preflight.rs +++ b/studio/src-tauri/src/preflight.rs @@ -566,16 +566,33 @@ mod tests { ( "cap-missing", r#"#!/bin/sh -if [ "$1" = "-h" ]; then exit 0; fi +if [ "$1" = "studio" ] && [ "$2" = "desktop-runtime-check" ] && [ "$3" = "--json" ]; then + printf '{"runtime_ready":true}' + exit 0 +fi if [ "$1" = "studio" ] && [ "$2" = "provision-desktop-auth" ] && [ "$3" = "--help" ]; then exit 0; fi exit 1 "#, Some("desktop_capability_probe_failed"), ), + ( + "runtime-missing-dependency", + r#"#!/bin/sh +if [ "$1" = "studio" ] && [ "$2" = "desktop-runtime-check" ] && [ "$3" = "--json" ]; then + printf '{"runtime_ready":false,"reason":"missing_dependency","module":"structlog"}' + exit 1 +fi +exit 1 +"#, + Some("studio_runtime_missing_dependency"), + ), ( "cap-true-helper-missing", r#"#!/bin/sh -if [ "$1" = "-h" ]; then exit 0; fi +if [ "$1" = "studio" ] && [ "$2" = "desktop-runtime-check" ] && [ "$3" = "--json" ]; then + printf '{"runtime_ready":true}' + exit 0 +fi if [ "$1" = "studio" ] && [ "$2" = "desktop-capabilities" ] && [ "$3" = "--json" ]; then printf '{"desktop_protocol_version":1,"desktop_manageability_version":1,"supports_api_only":true,"supports_provision_desktop_auth":true,"supports_desktop_backend_ownership":true,"version":"2026.5.3"}' exit 0 @@ -587,7 +604,10 @@ exit 1 ( "cap-false-helper-ready", r#"#!/bin/sh -if [ "$1" = "-h" ]; then exit 0; fi +if [ "$1" = "studio" ] && [ "$2" = "desktop-runtime-check" ] && [ "$3" = "--json" ]; then + printf '{"runtime_ready":true}' + exit 0 +fi if [ "$1" = "studio" ] && [ "$2" = "desktop-capabilities" ] && [ "$3" = "--json" ]; then printf '{"desktop_protocol_version":1,"desktop_manageability_version":1,"supports_api_only":true,"supports_provision_desktop_auth":false,"supports_desktop_backend_ownership":true,"desktop_auth_stale_reason":"cap_false","version":"2026.5.3"}' exit 0 @@ -618,26 +638,28 @@ exit 1 #[cfg(unix)] #[tokio::test] - async fn managed_cli_capability_help_probe_runs_before_cache() { + async fn managed_runtime_probe_runs_before_capability_cache() { use std::fs; let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; let _cache_home = ManagedCapabilityCacheHome::new("cache-hit"); remove_managed_capability_cache(); - // `-h` always succeeds unless `modeh` exists; the desktop-capabilities - // probe always succeeds unless `modecap` exists. Toggling those lets us - // prove the ordering: -h runs on every probe (even a cache hit), while - // the heavier capability probe is skipped once the cache is warm. + // Runtime readiness is checked on every probe, while the static + // desktop-capabilities probe is skipped once the cache is warm. let fake = fake_cli( "cap-cache-hit", r#"#!/bin/sh log="$0.calls" -modeh="$0.modeh" +moderuntime="$0.moderuntime" modecap="$0.modecap" printf '%s\n' "$*" >> "$log" -if [ "$1" = "-h" ]; then - if [ -f "$modeh" ]; then exit 42; fi +if [ "$1" = "studio" ] && [ "$2" = "desktop-runtime-check" ] && [ "$3" = "--json" ]; then + if [ -f "$moderuntime" ]; then + printf '{"runtime_ready":false,"reason":"missing_dependency","module":"structlog"}' + exit 42 + fi + printf '{"runtime_ready":true}' exit 0 fi if [ "$1" = "studio" ] && [ "$2" = "desktop-capabilities" ] && [ "$3" = "--json" ]; then @@ -650,37 +672,42 @@ exit 1 ); let bin = fake.bin.clone(); let calls = bin.with_extension("calls"); - let modeh = bin.with_extension("modeh"); + let moderuntime = bin.with_extension("moderuntime"); let modecap = bin.with_extension("modecap"); - // Cold probe: runs -h and the capability probe, then caches the result. + // Cold probe runs both checks, then caches the static capability. assert!(matches!( probe_managed_bin(bin.clone()).await, ManagedProbe::Ready { .. } )); let first_calls = fs::read_to_string(&calls).unwrap(); - assert!(first_calls.contains("-h")); + assert!(first_calls.contains("studio desktop-runtime-check --json")); assert!(first_calls.contains("studio desktop-capabilities --json")); - // Cache hit: -h still runs, but the capability probe is skipped (breaking - // it via `modecap` proves it is not invoked). + // Cache hit still checks the runtime, but skips the capability probe. fs::write(&modecap, "broken").unwrap(); fs::write(&calls, "").unwrap(); assert!(matches!( probe_managed_bin(bin.clone()).await, ManagedProbe::Ready { .. } )); - assert_eq!(fs::read_to_string(&calls).unwrap(), "-h\n"); + assert_eq!( + fs::read_to_string(&calls).unwrap(), + "studio desktop-runtime-check --json\n" + ); - // A non-launchable CLI is caught by the -h probe even with a warm cache: - // preflight reports Stale (for repair) and never trusts the cache. - fs::write(&modeh, "broken").unwrap(); + // A broken runtime is caught even with a warm capability cache. + fs::write(&moderuntime, "broken").unwrap(); fs::write(&calls, "").unwrap(); assert!(matches!( probe_managed_bin(bin).await, - ManagedProbe::Stale { .. } + ManagedProbe::Stale { reason, .. } + if reason == "studio_runtime_missing_dependency" )); - assert_eq!(fs::read_to_string(&calls).unwrap(), "-h\n"); + assert_eq!( + fs::read_to_string(&calls).unwrap(), + "studio desktop-runtime-check --json\n" + ); remove_managed_capability_cache(); } diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index 0d20f271c5..57b8b53c81 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -36,6 +36,13 @@ struct DesktopCapability { version: Option, } +#[derive(Debug, Deserialize)] +struct DesktopRuntimeCheck { + runtime_ready: bool, + reason: Option, + module: Option, +} + #[derive(Debug, Clone, Deserialize, Serialize)] struct ManagedCapabilityCache { schema: u16, @@ -263,10 +270,12 @@ fn write_cached_capability(fingerprint: &ManagedBinFingerprint, capability: &Des } } -async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { +async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { let started = Instant::now(); let mut cmd = Command::new(bin); - cmd.args(args).stdout(Stdio::null()).stderr(Stdio::null()); + cmd.args(["studio", "desktop-runtime-check", "--json"]) + .stdout(Stdio::piped()) + .stderr(Stdio::null()); #[cfg(target_os = "linux")] if std::env::var_os("APPIMAGE").is_some() { @@ -288,28 +297,60 @@ async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { let Ok(mut child) = cmd.spawn() else { info!( - "Managed preflight probe {:?} failed to spawn in {}ms", - args, + "Managed runtime probe failed to spawn in {}ms", started.elapsed().as_millis() ); - return false; + return Err("studio_runtime_probe_failed".to_string()); + }; + let Some(mut stdout) = child.stdout.take() else { + return Err("studio_runtime_probe_failed".to_string()); }; - let ok = match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { - Ok(Ok(status)) => status.success(), + let status = match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { + Ok(Ok(status)) => status, _ => { let _ = child.kill().await; let _ = child.wait().await; - false + info!( + "Managed runtime probe timed out in {}ms", + started.elapsed().as_millis() + ); + return Err("studio_runtime_probe_failed".to_string()); } }; + + let mut output = Vec::new(); + if stdout.read_to_end(&mut output).await.is_err() { + return Err("studio_runtime_probe_failed".to_string()); + } + let payload = String::from_utf8_lossy(&output) + .lines() + .rev() + .find_map(|line| serde_json::from_str::(line).ok()); + let result = match payload { + Some(payload) if status.success() && payload.runtime_ready => Ok(()), + Some(payload) => { + let reason = match payload.reason.as_deref() { + Some("missing_dependency") => { + info!( + "Managed runtime probe missing dependency module={}", + payload.module.as_deref().unwrap_or("unknown") + ); + "studio_runtime_missing_dependency" + } + Some("backend_import_failed") => "studio_runtime_import_failed", + _ => "studio_runtime_probe_failed", + }; + Err(reason.to_string()) + } + None => Err("studio_runtime_probe_failed".to_string()), + }; info!( - "Managed preflight probe {:?} finished ok={} in {}ms", - args, - ok, + "Managed runtime probe finished ok={} in {}ms", + result.is_ok(), started.elapsed().as_millis() ); - ok + result } async fn probe_cli_capability(bin: &Path) -> Option { @@ -410,22 +451,17 @@ fn desktop_capability_ready(capability: &DesktopCapability) -> bool { pub(super) async fn probe_managed_bin(bin: PathBuf) -> ManagedProbe { let started = Instant::now(); - // Always verify the managed CLI actually launches before trusting the cache. - // A matching capability fingerprint does not prove the binary can still run: - // its venv interpreter or a runtime dependency can be broken while the - // path/size/mtime/markers are unchanged, so the -h probe runs first and a - // non-launchable install is reported Stale for repair. The capability cache - // below still skips the heavier desktop-capabilities probe on a hit. - if !run_cli_probe(&bin, &["-h"]).await { + // Runtime readiness is intentionally uncached. A matching capability + // fingerprint only proves protocol compatibility, not that Studio's backend + // imports are complete after an interrupted dependency transaction. + if let Err(reason) = probe_cli_runtime(&bin).await { info!( - "Managed preflight: cli unusable for {:?} in {}ms", + "Managed preflight: runtime unusable for {:?} reason={} in {}ms", bin, + reason, started.elapsed().as_millis() ); - return ManagedProbe::Stale { - bin, - reason: "cli_unusable".to_string(), - }; + return ManagedProbe::Stale { bin, reason }; } if let Some(fingerprint) = managed_bin_fingerprint(&bin) { @@ -478,6 +514,16 @@ pub(super) async fn probe_managed_bin(bin: PathBuf) -> ManagedProbe { pub(super) async fn probe_managed_install() -> ManagedProbe { let started = Instant::now(); let result = match crate::process::find_unsloth_binary() { + Some(bin) if crate::install::managed_install_in_progress() => { + info!( + "Managed preflight: interrupted desktop installation marker found for {:?}", + bin + ); + ManagedProbe::Stale { + bin, + reason: "install_incomplete".to_string(), + } + } Some(bin) => probe_managed_bin(bin).await, None => ManagedProbe::Missing, }; diff --git a/studio/src-tauri/src/process.rs b/studio/src-tauri/src/process.rs index 56d9dd2e21..b7e0a05ee8 100644 --- a/studio/src-tauri/src/process.rs +++ b/studio/src-tauri/src/process.rs @@ -90,13 +90,18 @@ impl OwnedBackendHandle { } } - fn remove_owner_metadata(self) { + fn cleanup_after_exit(self) { match self { Self::Spawned { - owner: Some(owner), .. + mut child, + owner, + pid, + .. + } => { + terminate_exited_backend_tree(pid, &mut child); + remove_optional_owner(owner); } - | Self::Adopted { owner, .. } => owner.remove(), - Self::Spawned { owner: None, .. } => {} + Self::Adopted { owner, .. } => owner.remove(), } } } @@ -316,6 +321,146 @@ pub(crate) fn trim_line_endings(bytes: &[u8]) -> &[u8] { &bytes[..end] } +#[cfg(unix)] +fn unix_process_group_exists(pid: u32) -> bool { + if pid > i32::MAX as u32 { + return false; + } + let result = unsafe { libc::kill(-(pid as i32), 0) }; + result == 0 || std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) +} + +#[cfg(unix)] +fn terminate_exited_backend_tree(pid: u32, _child: &mut Box) { + if !unix_process_group_exists(pid) { + return; + } + + info!("Stopping surviving backend process group (pid {})", pid); + unsafe { + libc::kill(-(pid as i32), libc::SIGTERM); + } + for _ in 0..50 { + if !unix_process_group_exists(pid) { + return; + } + std::thread::sleep(Duration::from_millis(100)); + } + + warn!( + "Backend descendants did not exit gracefully, force killing group (pid {})", + pid + ); + unsafe { + libc::kill(-(pid as i32), libc::SIGKILL); + } +} + +#[cfg(windows)] +fn terminate_exited_backend_tree(pid: u32, child: &mut Box) { + info!("Stopping surviving backend job (pid {})", pid); + if let Err(error) = child.kill() { + warn!("Failed to terminate backend job (pid {}): {}", pid, error); + return; + } + if let Err(error) = child.wait() { + warn!("Failed to wait for backend job (pid {}): {}", pid, error); + } +} + +#[cfg(not(any(unix, windows)))] +fn terminate_exited_backend_tree(_pid: u32, _child: &mut Box) {} + +struct BackendExit { + owned: OwnedBackendHandle, + status: String, + intentional: bool, +} + +enum BackendExitPoll { + Running, + Exited(BackendExit), + Stop, + QueryFailed(String), +} + +fn poll_spawned_backend_exit(state: &BackendState, generation: u64) -> BackendExitPoll { + let mut proc = match state.lock() { + Ok(guard) => guard, + Err(poisoned) => poisoned.into_inner(), + }; + if proc.generation != generation { + return BackendExitPoll::Stop; + } + let intentional = proc.intentional_stop; + let status = match proc + .owned + .as_mut() + .and_then(OwnedBackendHandle::spawned_child_mut) + { + Some(child) => match child.try_wait() { + Ok(Some(status)) => status.to_string(), + Ok(None) => return BackendExitPoll::Running, + Err(error) => return BackendExitPoll::QueryFailed(error.to_string()), + }, + None => return BackendExitPoll::Stop, + }; + + let Some(owned) = proc.owned.take() else { + return BackendExitPoll::Stop; + }; + proc.port = None; + proc.diagnostics_session = None; + proc.adopted_watchdog_generation = None; + BackendExitPoll::Exited(BackendExit { + owned, + status, + intentional, + }) +} + +fn monitor_spawned_backend_exit( + app: AppHandle, + state: BackendState, + diagnostics_state: DiagnosticsState, + backend_log: BackendLog, + generation: u64, +) { + let mut query_failure_reported = false; + loop { + match poll_spawned_backend_exit(&state, generation) { + BackendExitPoll::Running => { + query_failure_reported = false; + std::thread::sleep(Duration::from_millis(100)); + } + BackendExitPoll::QueryFailed(error) => { + if !query_failure_reported { + warn!("Failed to query backend process status: {}", error); + query_failure_reported = true; + } + std::thread::sleep(Duration::from_millis(100)); + } + BackendExitPoll::Stop => return, + BackendExitPoll::Exited(exit) => { + info!("Backend exited with status: {}", exit.status); + exit.owned.cleanup_after_exit(); + diagnostics::record_backend_exit( + &diagnostics_state, + &backend_log.session_id, + Some(exit.status), + exit.intentional, + None, + ); + if !exit.intentional { + error!("Backend process exited unexpectedly (crash detected)"); + let _ = app.emit("server-crashed", ()); + } + return; + } + } + } +} + /// Windows `CREATE_NO_WINDOW` flag — suppresses console windows for child processes. #[cfg(windows)] pub(crate) const CREATE_NO_WINDOW: u32 = 0x08000000; @@ -444,6 +589,119 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn spawned_backend_exit_cleans_surviving_process_group() { + use std::os::unix::process::CommandExt; + + let mut command = Command::new("sh"); + command.args(["-c", "trap '' HUP; sleep 1; sleep 30 & exit 7"]); + command.process_group(0); + let child = command.spawn().unwrap(); + let pid = child.id(); + let state = new_backend_state(); + { + let mut proc = state.lock().unwrap(); + proc.generation = 4; + proc.owned = Some(OwnedBackendHandle::spawned(Box::new(child), None, pid, 4)); + } + + assert!(matches!( + poll_spawned_backend_exit(&state, 4), + BackendExitPoll::Running + )); + + let exit = loop { + match poll_spawned_backend_exit(&state, 4) { + BackendExitPoll::Running => { + std::thread::sleep(Duration::from_millis(50)); + } + BackendExitPoll::Exited(exit) => break exit, + BackendExitPoll::QueryFailed(error) => panic!("{error}"), + BackendExitPoll::Stop => panic!("monitor stopped before observing exit"), + } + }; + assert!(exit.status.contains('7')); + assert!(!exit.intentional); + let group_survived = unix_process_group_exists(pid); + exit.owned.cleanup_after_exit(); + assert!(group_survived); + assert!(!unix_process_group_exists(pid)); + + assert!(matches!( + poll_spawned_backend_exit(&state, 4), + BackendExitPoll::Stop + )); + assert!(!state.lock().unwrap().has_owned_backend()); + } + + #[cfg(windows)] + fn windows_process_is_alive(pid: u32) -> bool { + use windows_sys::Win32::Foundation::{CloseHandle, WAIT_TIMEOUT}; + use windows_sys::Win32::System::Threading::{ + OpenProcess, WaitForSingleObject, PROCESS_SYNCHRONIZE, + }; + + unsafe { + let handle = OpenProcess(PROCESS_SYNCHRONIZE, 0, pid); + if handle.is_null() { + return false; + } + let status = WaitForSingleObject(handle, 0); + let _ = CloseHandle(handle); + status == WAIT_TIMEOUT + } + } + + #[cfg(windows)] + #[test] + fn spawned_backend_exit_cleans_surviving_job() { + let temp = temp_studio_dir("backend-job-cleanup"); + let pid_file = temp.join("descendant.pid"); + let quoted_path = pid_file.to_string_lossy().replace('\'', "''"); + let script = format!( + "$child = Start-Process \"$env:SystemRoot\\System32\\ping.exe\" \ + -ArgumentList '-n','30','127.0.0.1' -WindowStyle Hidden -PassThru; \ + Set-Content -LiteralPath '{quoted_path}' -Value $child.Id -NoNewline; exit 7" + ); + let mut command = Command::new("powershell.exe"); + command.args(["-NoProfile", "-NonInteractive", "-Command", &script]); + let child = spawn_backend_command(command).unwrap(); + let pid = child.id(); + let state = new_backend_state(); + { + let mut proc = state.lock().unwrap(); + proc.generation = 4; + proc.owned = Some(OwnedBackendHandle::spawned(child, None, pid, 4)); + } + + let exit = loop { + match poll_spawned_backend_exit(&state, 4) { + BackendExitPoll::Running => std::thread::sleep(Duration::from_millis(50)), + BackendExitPoll::Exited(exit) => break exit, + BackendExitPoll::QueryFailed(error) => panic!("{error}"), + BackendExitPoll::Stop => panic!("monitor stopped before observing exit"), + } + }; + let descendant_pid = fs::read_to_string(&pid_file) + .unwrap() + .parse::() + .unwrap(); + let descendant_survived = windows_process_is_alive(descendant_pid); + exit.owned.cleanup_after_exit(); + + assert!(exit.status.contains('7')); + assert!(descendant_survived); + for _ in 0..20 { + if !windows_process_is_alive(descendant_pid) { + break; + } + std::thread::sleep(Duration::from_millis(50)); + } + assert!(!windows_process_is_alive(descendant_pid)); + fs::remove_dir_all(temp).unwrap(); + } + fn listening_non_studio_port() -> (u16, mpsc::Sender<()>, std::thread::JoinHandle<()>) { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); listener.set_nonblocking(true).unwrap(); @@ -548,6 +806,29 @@ fn backend_args(port: u16) -> Vec { .collect() } +fn spawn_backend_command(cmd: Command) -> std::io::Result> { + let mut wrap = CommandWrap::from(cmd); + + #[cfg(windows)] + { + use windows::Win32::System::Threading::{ + CREATE_NEW_PROCESS_GROUP, CREATE_NO_WINDOW as WINDOWS_CREATE_NO_WINDOW, + }; + + // Keep this backend tree independently terminable after its leader exits. + wrap.wrap(CreationFlags( + CREATE_NEW_PROCESS_GROUP | WINDOWS_CREATE_NO_WINDOW, + )); + wrap.wrap(JobObject); + } + + #[cfg(unix)] + wrap.wrap(ProcessGroup::leader()); + + let child: Box = wrap.spawn()?; + Ok(child) +} + /// Spawn the backend process and wire up stdout/stderr reader threads. pub fn start_backend( app: &AppHandle, @@ -634,46 +915,17 @@ pub fn start_backend( let backend_log = diagnostics::begin_backend_session(diagnostics_state, port, generation); - // On Windows, launch the backend directly with hidden-window flags. - // The app process is assigned to a KILL_ON_JOB_CLOSE job in main.rs, so - // children inherit crash-safe cleanup without the buggy per-child JobObject wrapper. - #[cfg(windows)] - let mut child: Box = { - use std::os::windows::process::CommandExt; - - const CREATE_NEW_PROCESS_GROUP: u32 = 0x00000200; - cmd.creation_flags(CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW); - let child = cmd.spawn().map_err(|e| { - let msg = format!("Failed to spawn backend: {}", e); - diagnostics::record_backend_start_failure( - diagnostics_state, - Some(port), - Some(generation), - "spawn_backend", - &msg, - ); - msg - })?; - Box::new(child) - }; - - #[cfg(unix)] - let mut child: Box = { - // Keep the backend tree in a process group on Unix for cleanup. - let mut wrap = CommandWrap::from(cmd); - wrap.wrap(ProcessGroup::leader()); - wrap.spawn().map_err(|e| { - let msg = format!("Failed to spawn backend: {}", e); - diagnostics::record_backend_start_failure( - diagnostics_state, - Some(port), - Some(generation), - "spawn_backend", - &msg, - ); - msg - })? - }; + let mut child = spawn_backend_command(cmd).map_err(|e| { + let msg = format!("Failed to spawn backend: {}", e); + diagnostics::record_backend_start_failure( + diagnostics_state, + Some(port), + Some(generation), + "spawn_backend", + &msg, + ); + msg + })?; let backend_pid = child.id(); let stdout = child.stdout().take(); @@ -731,6 +983,22 @@ pub fn start_backend( }); } + { + let app_handle = app.clone(); + let state_clone = Arc::clone(state); + let diagnostics_clone = diagnostics_state.clone(); + let backend_log_clone = backend_log.clone(); + std::thread::spawn(move || { + monitor_spawned_backend_exit( + app_handle, + state_clone, + diagnostics_clone, + backend_log_clone, + generation, + ); + }); + } + Ok(generation) } @@ -897,7 +1165,7 @@ async fn validate_candidate_port( /// Read lines from a child process stream (stdout or stderr). /// For stdout, parse TAURI_PORT=(\d+) candidates for async validation. -/// When stdout closes and the stop was not intentional, emit server-crashed. +/// Process lifecycle is owned by monitor_spawned_backend_exit. fn read_output_stream( stream: R, app: &AppHandle, @@ -993,63 +1261,6 @@ fn read_output_stream( } } } - - // Stream closed. Only the stdout reader checks for crashes. - if !is_stderr { - let mut exit_record: Option<(String, bool)> = None; - let mut emit_crash = false; - if let Ok(mut proc) = state.lock() { - if proc.generation != generation { - return; - } - let intentional = proc.intentional_stop; - let exited = if let Some(child) = proc - .owned - .as_mut() - .and_then(OwnedBackendHandle::spawned_child_mut) - { - match child.try_wait() { - Ok(Some(status)) => { - info!("Backend stdout stream ended with status: {}", status); - exit_record = Some((status.to_string(), intentional)); - true - } - Ok(None) => { - warn!("Backend stdout stream ended, but process is still running"); - false - } - Err(e) => { - warn!("Failed to query backend status after stdout closed: {}", e); - false - } - } - } else { - false - }; - - if exited { - if let Some(owned) = proc.owned.take() { - owned.remove_owner_metadata(); - } - proc.port = None; - proc.diagnostics_session = None; - emit_crash = !intentional; - } - } - if let Some((status, intentional)) = exit_record { - diagnostics::record_backend_exit( - diagnostics_state, - &backend_log.session_id, - Some(status), - intentional, - None, - ); - } - if emit_crash { - error!("Backend process stdout closed unexpectedly (crash detected)"); - let _ = app.emit("server-crashed", ()); - } - } } fn wait_for_child_exit(child: &mut Box, label: &str) -> bool { @@ -1128,6 +1339,7 @@ fn stop_spawned_backend( && try_exact_port_http_shutdown(port, "Spawned backend") && wait_for_child_exit(&mut child, "Backend") { + terminate_exited_backend_tree(pid, &mut child); remove_optional_owner(owner); return Ok(()); } @@ -1158,6 +1370,7 @@ fn stop_spawned_backend( } if wait_for_child_exit(&mut child, "Backend") { + terminate_exited_backend_tree(pid, &mut child); remove_optional_owner(owner); return Ok(()); } diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index c41963aab9..d6ccdf1a83 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -270,6 +270,27 @@ def _load_run_module(): return _RUN_MODULE +def _missing_studio_requirement(run_mod): + from importlib.metadata import PackageNotFoundError, distribution + from packaging.requirements import Requirement + + requirements = Path(run_mod.__file__).with_name("requirements") / "studio.txt" + for line in requirements.read_text(encoding = "utf-8").splitlines(): + line = line.partition("#")[0].strip() + if not line: + continue + requirement = Requirement(line) + if requirement.marker and not requirement.marker.evaluate(): + continue + try: + installed = distribution(requirement.name) + except PackageNotFoundError: + return requirement.name + if requirement.specifier and not requirement.specifier.contains(installed.version): + return requirement.name + return None + + def _find_setup_script() -> Optional[Path]: """Find studio/setup.sh or studio/setup.ps1. @@ -2809,6 +2830,42 @@ def desktop_capabilities( typer.echo(f"{key}: {value}") +@studio_app.command("desktop-runtime-check", hidden = True) +def desktop_runtime_check( + _json_output: bool = typer.Option( + False, + "--json", + help = "Emit machine-readable JSON.", + ), +): + try: + run_mod = _load_run_module() + missing = _missing_studio_requirement(run_mod) + if missing: + raise ModuleNotFoundError( + f"No distribution named {missing!r}", + name = missing, + ) + except ModuleNotFoundError as exc: + payload = { + "runtime_ready": False, + "reason": "missing_dependency", + "module": exc.name, + } + except Exception as exc: + payload = { + "runtime_ready": False, + "reason": "backend_import_failed", + "error_type": type(exc).__name__, + } + else: + payload = {"runtime_ready": True} + + typer.echo(json.dumps(payload, sort_keys = True)) + if not payload["runtime_ready"]: + raise typer.Exit(code = 1) from None + + @studio_app.command("provision-desktop-auth", hidden = True) def provision_desktop_auth(): """Create/repair desktop auth state for the local machine.""" diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py new file mode 100644 index 0000000000..23c3ad80b2 --- /dev/null +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -0,0 +1,85 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 + +import importlib +import json +import sys +from types import SimpleNamespace +from pathlib import Path + +import pytest +import typer + + +_REPO_ROOT = Path(__file__).resolve().parents[2] +if str(_REPO_ROOT) not in sys.path: + sys.path.insert(0, str(_REPO_ROOT)) + + +def _missing_structlog(): + raise ModuleNotFoundError("No module named 'structlog'", name = "structlog") + + +def test_desktop_runtime_check_reports_missing_dependency_as_json(monkeypatch, capsys): + studio = importlib.import_module("unsloth_cli.commands.studio") + monkeypatch.setattr(studio, "_load_run_module", _missing_structlog) + + with pytest.raises(typer.Exit) as exited: + studio.desktop_runtime_check(_json_output = True) + + assert exited.value.exit_code == 1 + assert json.loads(capsys.readouterr().out) == { + "runtime_ready": False, + "reason": "missing_dependency", + "module": "structlog", + } + + +def test_desktop_runtime_check_reports_success(monkeypatch, capsys): + studio = importlib.import_module("unsloth_cli.commands.studio") + monkeypatch.setattr(studio, "_load_run_module", lambda: object()) + monkeypatch.setattr(studio, "_missing_studio_requirement", lambda _run_mod: None) + + studio.desktop_runtime_check(_json_output = True) + + assert json.loads(capsys.readouterr().out) == {"runtime_ready": True} + + +def test_desktop_runtime_check_catches_later_startup_dependency(monkeypatch, capsys, tmp_path): + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text( + "definitely-missing-studio-package\n", + encoding = "utf-8", + ) + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + + with pytest.raises(typer.Exit): + studio.desktop_runtime_check(_json_output = True) + + payload = json.loads(capsys.readouterr().out) + assert payload["module"] == "definitely-missing-studio-package" + + +def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp_path): + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text("example-package==2.0\n", encoding = "utf-8") + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + monkeypatch.setattr( + importlib.import_module("importlib.metadata"), + "distribution", + lambda _name: SimpleNamespace(version = "1.0"), + ) + + with pytest.raises(typer.Exit): + studio.desktop_runtime_check(_json_output = True) + + payload = json.loads(capsys.readouterr().out) + assert payload["module"] == "example-package" From 7b22a8584f44fd7ea4f9e2486efc0726b07b4f03 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 11:44:37 +0000 Subject: [PATCH 02/16] Fix the readiness probe deadlock and marker lifecycle for PR #7490 The runtime probe waits on child exit with stdout piped and undrained, so a CLI printing more than the pipe buffer holds blocks mid-write and the probe times out. It imports the whole backend, which is exactly what emits stray stdout, so a healthy install gets reported broken and sent to repair after a 10s stall on every launch. Drain concurrently instead, here and in the desktop-capabilities probe next to it. The install marker was cleared only on success, so a failed update, or a cancelled elevation prompt, left it behind and pinned a working install into repair it may not be able to finish offline. Clear it on every terminal outcome and treat marker bookkeeping as best effort rather than failing the install over it. Skip pip flag lines in studio.txt: an unparseable line reported the install broken, and repair reinstalls the same file, so that never cleared. Allow prereleases against a floor for the same reason. --- studio/src-tauri/src/install.rs | 44 ++++++++----------- studio/src-tauri/src/preflight.rs | 39 ++++++++++++++++ studio/src-tauri/src/preflight/managed.rs | 29 +++++++++--- unsloth_cli/commands/studio.py | 17 +++++-- .../tests/test_studio_runtime_readiness.py | 38 ++++++++++++++++ 5 files changed, 131 insertions(+), 36 deletions(-) diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index 72c610ffcc..27996d30a9 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -82,6 +82,17 @@ 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. +fn clear_install_marker_best_effort() { + if let Err(msg) = clear_install_in_progress_marker() { + warn!("[install] {}", msg); + } +} + fn clear_install_in_progress_marker_at(path: &Path) -> Result<(), String> { match std::fs::remove_file(path) { Ok(()) => Ok(()), @@ -496,18 +507,7 @@ fn run_install_with_event_mode( ); if let Err(msg) = create_install_in_progress_marker() { - diagnostics::finish_attempt( - &diagnostics, - &attempt, - None, - false, - Some(format!("create_install_marker: {msg}")), - ); - clear_current_attempt(&state); - if event_mode.emit_terminal_events() { - emit_failed(&app, &msg); - } - return Err(msg); + warn!("[install] {}", msg); } let (stdout, stderr) = match spawn_script(&script, &args, &state) { @@ -542,20 +542,7 @@ fn run_install_with_event_mode( match result { Ok((status, _)) if status.success() => { - if let Err(msg) = clear_install_in_progress_marker() { - diagnostics::finish_attempt( - &diagnostics, - &attempt, - Some(status.to_string()), - false, - Some(format!("clear_install_marker: {msg}")), - ); - clear_current_attempt(&state); - if event_mode.emit_terminal_events() { - emit_failed(&app, &msg); - } - return Err(msg); - } + clear_install_marker_best_effort(); diagnostics::finish_attempt( &diagnostics, &attempt, @@ -589,6 +576,7 @@ 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(); let msg = format!("Installer exited with code {}", code); diagnostics::finish_attempt( &diagnostics, @@ -611,6 +599,7 @@ fn run_install_with_event_mode( Err(msg) } Err(msg) => { + clear_install_marker_best_effort(); diagnostics::finish_attempt(&diagnostics, &attempt, None, false, Some(msg.clone())); clear_current_attempt(&state); if event_mode.emit_terminal_events() { @@ -650,6 +639,9 @@ pub fn record_pending_elevation_canceled( let Some(attempt) = attempt else { return false; }; + // The elevation exit left the marker in place for the resumed run that is + // now not happening. + clear_install_marker_best_effort(); diagnostics::finish_attempt( diagnostics, &attempt, diff --git a/studio/src-tauri/src/preflight.rs b/studio/src-tauri/src/preflight.rs index 1e129e85b0..e7abaa6715 100644 --- a/studio/src-tauri/src/preflight.rs +++ b/studio/src-tauri/src/preflight.rs @@ -636,6 +636,45 @@ exit 1 } } + #[cfg(unix)] + #[tokio::test] + async fn managed_runtime_probe_survives_more_stdout_than_the_pipe_holds() { + // The probe imports the whole backend, so import-time output can exceed + // the 64 KiB pipe buffer. Waiting on exit before draining wedges the + // child mid-write and reports a healthy install as broken. + let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; + let _cache_home = ManagedCapabilityCacheHome::new("runtime-noisy"); + remove_managed_capability_cache(); + + let fake = fake_cli( + "runtime-noisy", + r#"#!/bin/sh +if [ "$1" = "studio" ] && [ "$2" = "desktop-runtime-check" ] && [ "$3" = "--json" ]; then + awk 'BEGIN { while (i++ < 4000) print "import chatter on stdout" }' + printf '{"runtime_ready":true}\n' + exit 0 +fi +if [ "$1" = "studio" ] && [ "$2" = "desktop-capabilities" ] && [ "$3" = "--json" ]; then + printf '{"desktop_protocol_version":1,"desktop_manageability_version":1,"supports_api_only":true,"supports_provision_desktop_auth":true,"supports_desktop_backend_ownership":true,"version":"2026.5.3"}' + exit 0 +fi +exit 1 +"#, + ); + + let started = std::time::Instant::now(); + let probe = probe_managed_bin(fake.bin.clone()).await; + assert!( + matches!(probe, ManagedProbe::Ready { .. }), + "noisy but healthy CLI must probe Ready, got {probe:?}" + ); + assert!( + started.elapsed() < std::time::Duration::from_secs(9), + "probe hit the 10s timeout instead of draining stdout" + ); + remove_managed_capability_cache(); + } + #[cfg(unix)] #[tokio::test] async fn managed_runtime_probe_runs_before_capability_cache() { diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index 57b8b53c81..a45131584a 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -306,11 +306,21 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { return Err("studio_runtime_probe_failed".to_string()); }; + // Drain while waiting: the backend import this probe runs can print more than + // the pipe buffer holds, and waiting first deadlocks on it, timing out a + // healthy install into repair. + let reader = tokio::spawn(async move { + let mut buffer = Vec::new(); + let _ = stdout.read_to_end(&mut buffer).await; + buffer + }); + let status = match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { Ok(Ok(status)) => status, _ => { let _ = child.kill().await; let _ = child.wait().await; + reader.abort(); info!( "Managed runtime probe timed out in {}ms", started.elapsed().as_millis() @@ -319,10 +329,9 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { } }; - let mut output = Vec::new(); - if stdout.read_to_end(&mut output).await.is_err() { + let Ok(output) = reader.await else { return Err("studio_runtime_probe_failed".to_string()); - } + }; let payload = String::from_utf8_lossy(&output) .lines() .rev() @@ -389,11 +398,19 @@ async fn probe_cli_capability(bin: &Path) -> Option { return None; }; + // Same reason as the runtime probe above. + let reader = tokio::spawn(async move { + let mut buffer = Vec::new(); + let _ = stdout.read_to_end(&mut buffer).await; + buffer + }); + match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { Ok(Ok(status)) if status.success() => {} Err(_) => { let _ = child.kill().await; let _ = child.wait().await; + reader.abort(); info!( "Managed desktop-capabilities probe timed out in {}ms", started.elapsed().as_millis() @@ -401,6 +418,7 @@ async fn probe_cli_capability(bin: &Path) -> Option { return None; } _ => { + reader.abort(); info!( "Managed desktop-capabilities probe exited unsuccessfully in {}ms", started.elapsed().as_millis() @@ -409,10 +427,9 @@ async fn probe_cli_capability(bin: &Path) -> Option { } } - let mut output = Vec::new(); - if stdout.read_to_end(&mut output).await.is_err() { + let Ok(output) = reader.await else { return None; - } + }; let capability = serde_json::from_slice::(&output).ok(); info!( diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index d6ccdf1a83..cf5215e69a 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -272,21 +272,30 @@ def _load_run_module(): def _missing_studio_requirement(run_mod): from importlib.metadata import PackageNotFoundError, distribution - from packaging.requirements import Requirement + from packaging.requirements import InvalidRequirement, Requirement requirements = Path(run_mod.__file__).with_name("requirements") / "studio.txt" for line in requirements.read_text(encoding = "utf-8").splitlines(): line = line.partition("#")[0].strip() - if not line: + # pip flags (-r, --extra-index-url) are not requirements. Skipping them + # matters: an unparseable line would report the install broken, and + # repair reinstalls the same file, so the failure would never clear. + if not line or line.startswith("-"): + continue + try: + requirement = Requirement(line) + except InvalidRequirement: continue - requirement = Requirement(line) if requirement.marker and not requirement.marker.evaluate(): continue try: installed = distribution(requirement.name) except PackageNotFoundError: return requirement.name - if requirement.specifier and not requirement.specifier.contains(installed.version): + # prereleases=True: a prerelease satisfying a floor is not a broken install. + if requirement.specifier and not requirement.specifier.contains( + installed.version, prereleases = True + ): return requirement.name return None diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 23c3ad80b2..9eace51d60 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -64,6 +64,44 @@ def test_desktop_runtime_check_catches_later_startup_dependency(monkeypatch, cap assert payload["module"] == "definitely-missing-studio-package" +def test_desktop_runtime_check_ignores_pip_flag_lines(monkeypatch, capsys, tmp_path): + """An unparseable line must not fail the check: repair reinstalls the same + file, so the install would be declared broken forever.""" + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text( + "--extra-index-url https://example.invalid/simple\n-r base.txt\n", + encoding = "utf-8", + ) + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + + studio.desktop_runtime_check(_json_output = True) + + assert json.loads(capsys.readouterr().out) == {"runtime_ready": True} + + +def test_desktop_runtime_check_accepts_a_prerelease_over_a_floor(monkeypatch, capsys, tmp_path): + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text("example-package>=1.0\n", encoding = "utf-8") + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + monkeypatch.setattr( + importlib.import_module("importlib.metadata"), + "distribution", + lambda _name: SimpleNamespace(version = "2.0.0b1"), + ) + + studio.desktop_runtime_check(_json_output = True) + + assert json.loads(capsys.readouterr().out) == {"runtime_ready": True} + + def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp_path): studio = importlib.import_module("unsloth_cli.commands.studio") backend = tmp_path / "backend" From b40d422654f891894fa213e6b9153783ef2b14d9 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 13:22:30 +0000 Subject: [PATCH 03/16] Clear the install marker on elevation failures too for PR #7490 The code 2 exit keeps the marker for the elevated run that follows, but every terminal failure of that run, a denied prompt or a failed apt-get included, returned without clearing it. That pinned a working install into repair the same way a failed update did. finish_elevation_failure is the one path all of them take. --- studio/src-tauri/src/install.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index 27996d30a9..dc0d7ac24d 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -914,6 +914,9 @@ fn finish_elevation_failure( exit_status: Option, message: String, ) { + // Terminal, like a cancelled prompt: the run the code 2 exit left the + // marker for is not happening. + clear_install_marker_best_effort(); if let Some(attempt) = attempt { diagnostics::finish_attempt( diagnostics, From 8f343038ef59f38982eefc354836abf3c45682e7 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 16:25:07 +0000 Subject: [PATCH 04/16] Stop the runtime probe stranding older CLIs and rejected settings Every released CLI predates desktop-runtime-check: PyPI's latest unsloth is 2026.7.5 and MIN_DESKTOP_BACKEND_VERSION stays 2026.5.3, so a field install is version-compatible and simply lacks the subcommand. It then exits 2 with no stdout, which read as a failed probe, and probe_managed_bin returned Stale before consulting the capability cache, so a healthy install was force-repaired and an offline user could not start at all. Fall back to the previous launch probe, gated on 'desktop-runtime-check --help' (0 when the command exists, 2 when it does not), so a new CLI that crashes or times out still reports Stale. Metadata outlives the package it describes. Hatchling wheels store .dist-info/METADATA as the first archive entry (verified: index 0 for both fastapi and typer), so an unpack killed midway leaves a readable version for modules that never landed and the probe called the venv ready. RECORD is written last, so a missing file list marks that unpack; checked against all 298 distributions in a real venv with no false positives. This supersedes my earlier reasoning that the metadata is written last, which was wrong. run.py raises SystemExit at import for a rejected setting such as UNSLOTH_CPU_THREADS, and SystemExit is not an Exception, so it escaped the handler and emitted no payload at all. Report it as backend_startup_failed and keep it out of the repair path: no reinstall can change an inherited environment value, so repairing would replace a healthy install and fail again. Also clear the install marker when the installer fails to spawn, the one terminal outcome that still left it behind. --- .../frontend/src/hooks/use-tauri-backend.ts | 4 + studio/src-tauri/src/install.rs | 2 + studio/src-tauri/src/preflight.rs | 95 ++++++++++++++++++- studio/src-tauri/src/preflight/managed.rs | 61 +++++++++++- unsloth_cli/commands/studio.py | 16 ++++ .../tests/test_studio_runtime_readiness.py | 54 ++++++++++- 6 files changed, 224 insertions(+), 8 deletions(-) diff --git a/studio/frontend/src/hooks/use-tauri-backend.ts b/studio/frontend/src/hooks/use-tauri-backend.ts index 53122864e7..016316c9dd 100644 --- a/studio/frontend/src/hooks/use-tauri-backend.ts +++ b/studio/frontend/src/hooks/use-tauri-backend.ts @@ -236,6 +236,10 @@ export function useTauriBackend() { stopExternalServerPoll(); if (preflight.can_auto_repair) { await startRepair(); + } else if (preflight.reason === "studio_runtime_startup_failed") { + setBackendError( + "The Unsloth backend refused to start with the current environment settings (for example UNSLOTH_CPU_THREADS). Fix or unset them, then restart Unsloth.", + ); } else { setBackendError( preflight.disposition === "owned_stale" diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index dc0d7ac24d..dee2481a6e 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -513,6 +513,8 @@ fn run_install_with_event_mode( let (stdout, stderr) = match spawn_script(&script, &args, &state) { Ok(handles) => handles, Err(msg) => { + // Nothing ran, so nothing is half-installed. + clear_install_marker_best_effort(); diagnostics::finish_attempt( &diagnostics, &attempt, diff --git a/studio/src-tauri/src/preflight.rs b/studio/src-tauri/src/preflight.rs index e7abaa6715..fd596f4cd5 100644 --- a/studio/src-tauri/src/preflight.rs +++ b/studio/src-tauri/src/preflight.rs @@ -24,10 +24,20 @@ use managed::probe_managed_bin; #[cfg(test)] use version::{backend_version_compatible, MIN_DESKTOP_BACKEND_VERSION}; +/// The managed backend refused to start on a value it inherited from the +/// environment rather than from the install. +pub(crate) const STUDIO_RUNTIME_STARTUP_FAILED: &str = "studio_runtime_startup_failed"; + fn release_auto_repair() -> bool { !cfg!(debug_assertions) } +/// Reinstalling cannot change a rejected environment value, so repairing over +/// one only replaces a healthy install and fails the same way afterwards. +fn stale_reason_is_repairable(reason: &str) -> bool { + reason != STUDIO_RUNTIME_STARTUP_FAILED +} + fn managed_bin_for_result(managed: &ManagedProbe) -> Option { match managed { ManagedProbe::Ready { bin } | ManagedProbe::Stale { bin, .. } => Some(bin.clone()), @@ -61,9 +71,9 @@ fn choose_preflight(managed: ManagedProbe, backend: BackendProbe) -> DesktopPref }, ManagedProbe::Stale { bin, reason } => DesktopPreflightResult { disposition: DesktopPreflightDisposition::ManagedStale, + can_auto_repair: release_auto_repair() && stale_reason_is_repairable(&reason), reason: Some(reason), port: None, - can_auto_repair: release_auto_repair(), managed_bin: Some(bin), }, ManagedProbe::Missing => DesktopPreflightResult { @@ -392,6 +402,40 @@ mod tests { } } + #[test] + fn rejected_backend_settings_do_not_auto_repair() { + // No reinstall can change an inherited environment value, so repairing + // would replace a healthy install and fail again the same way. + assert!(!stale_reason_is_repairable(STUDIO_RUNTIME_STARTUP_FAILED)); + for reason in [ + "studio_runtime_missing_dependency", + "studio_runtime_import_failed", + "install_incomplete", + "desktop_backend_version_too_old", + ] { + assert!(stale_reason_is_repairable(reason), "{reason}"); + } + + let result = choose_preflight( + ManagedProbe::Stale { + bin: PathBuf::from("/managed/unsloth"), + reason: STUDIO_RUNTIME_STARTUP_FAILED.to_string(), + }, + BackendProbe::Missing, + ); + + assert_eq!( + result.disposition, + DesktopPreflightDisposition::ManagedStale + ); + assert_eq!( + result.reason.as_deref(), + Some(STUDIO_RUNTIME_STARTUP_FAILED) + ); + // False in every build profile, unlike the other stale reasons. + assert!(!result.can_auto_repair); + } + #[test] fn external_conflict_blocks_managed_flow() { let result = choose_preflight( @@ -675,6 +719,55 @@ exit 1 remove_managed_capability_cache(); } + #[cfg(unix)] + #[tokio::test] + async fn managed_cli_predating_the_runtime_check_is_not_forced_into_repair() { + // Every published CLI satisfies MIN_DESKTOP_BACKEND_VERSION but has no + // `desktop-runtime-check`, so click exits 2 with an empty stdout. That + // must not repair an install that still launches Studio. + let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; + let _cache_home = ManagedCapabilityCacheHome::new("runtime-check-absent"); + remove_managed_capability_cache(); + + let old = fake_cli( + "runtime-check-absent", + r#"#!/bin/sh +if [ "$1" = "studio" ] && [ "$2" = "desktop-runtime-check" ]; then + echo "Error: No such command 'desktop-runtime-check'." >&2 + exit 2 +fi +if [ "$1" = "-h" ]; then exit 0; fi +if [ "$1" = "studio" ] && [ "$2" = "desktop-capabilities" ] && [ "$3" = "--json" ]; then + printf '{"desktop_protocol_version":1,"desktop_manageability_version":1,"supports_api_only":true,"supports_provision_desktop_auth":true,"supports_desktop_backend_ownership":true,"version":"2026.7.5"}' + exit 0 +fi +exit 1 +"#, + ); + let probe = probe_managed_bin(old.bin.clone()).await; + assert!( + matches!(probe, ManagedProbe::Ready { .. }), + "CLI without the runtime-check subcommand must stay Ready, got {probe:?}" + ); + + // The fallback must not rescue a CLI that cannot launch at all. + remove_managed_capability_cache(); + let broken = fake_cli( + "runtime-check-unlaunchable", + r#"#!/bin/sh +exit 2 +"#, + ); + assert!( + matches!( + probe_managed_bin(broken.bin.clone()).await, + ManagedProbe::Stale { reason, .. } if reason == "studio_runtime_probe_failed" + ), + "an unlaunchable CLI must stay Stale" + ); + remove_managed_capability_cache(); + } + #[cfg(unix)] #[tokio::test] async fn managed_runtime_probe_runs_before_capability_cache() { diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index a45131584a..af722eef83 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -270,6 +270,48 @@ fn write_cached_capability(fingerprint: &ManagedBinFingerprint, capability: &Des } } +async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { + let mut cmd = Command::new(bin); + cmd.args(args).stdout(Stdio::null()).stderr(Stdio::null()); + + #[cfg(target_os = "linux")] + if std::env::var_os("APPIMAGE").is_some() { + cmd.env_remove("LD_LIBRARY_PATH"); + cmd.env_remove("PYTHONHOME"); + cmd.env_remove("PYTHONPATH"); + } + + cmd.env_remove("UNSLOTH_STUDIO_HOME"); + cmd.env_remove("STUDIO_HOME"); + + #[cfg(windows)] + { + use std::os::windows::process::CommandExt; + cmd.creation_flags(crate::process::CREATE_NO_WINDOW); + } + + let Ok(mut child) = cmd.spawn() else { + return false; + }; + match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { + Ok(Ok(status)) => status.success(), + _ => { + let _ = child.kill().await; + let _ = child.wait().await; + false + } + } +} + +/// 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 +/// the legacy launch probe keeps a genuinely unusable binary out of this path. +async fn predates_runtime_check(bin: &Path) -> bool { + !run_cli_probe(bin, &["studio", "desktop-runtime-check", "--help"]).await + && run_cli_probe(bin, &["-h"]).await +} + async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { let started = Instant::now(); let mut cmd = Command::new(bin); @@ -348,6 +390,7 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { "studio_runtime_missing_dependency" } Some("backend_import_failed") => "studio_runtime_import_failed", + Some("backend_startup_failed") => super::STUDIO_RUNTIME_STARTUP_FAILED, _ => "studio_runtime_probe_failed", }; Err(reason.to_string()) @@ -472,13 +515,21 @@ pub(super) async fn probe_managed_bin(bin: PathBuf) -> ManagedProbe { // fingerprint only proves protocol compatibility, not that Studio's backend // imports are complete after an interrupted dependency transaction. if let Err(reason) = probe_cli_runtime(&bin).await { + // Every released CLI predates this subcommand, so a missing-command exit + // must not strand an install the launch probe still accepts. + if reason != "studio_runtime_probe_failed" || !predates_runtime_check(&bin).await { + info!( + "Managed preflight: runtime unusable for {:?} reason={} in {}ms", + bin, + reason, + started.elapsed().as_millis() + ); + return ManagedProbe::Stale { bin, reason }; + } info!( - "Managed preflight: runtime unusable for {:?} reason={} in {}ms", - bin, - reason, - started.elapsed().as_millis() + "Managed preflight: cli predates the runtime probe for {:?}; using the launch probe", + bin ); - return ManagedProbe::Stale { bin, reason }; } if let Some(fingerprint) = managed_bin_fingerprint(&bin) { diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index cf5215e69a..c6cae42b83 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -292,6 +292,12 @@ def _missing_studio_requirement(run_mod): installed = distribution(requirement.name) except PackageNotFoundError: return requirement.name + # Metadata outlives the package it describes: hatchling wheels (fastapi, + # typer) store .dist-info/METADATA as the first archive entry, so an + # unpack killed midway leaves a readable version for modules that never + # landed. RECORD is written last, so its absence marks that unpack. + if installed.files is None: + return requirement.name # prereleases=True: a prerelease satisfying a floor is not a broken install. if requirement.specifier and not requirement.specifier.contains( installed.version, prereleases = True @@ -2861,6 +2867,16 @@ def desktop_runtime_check( "reason": "missing_dependency", "module": exc.name, } + # SystemExit is not an Exception. run.py raises it for rejected settings such + # as UNSLOTH_CPU_THREADS, and letting it escape would emit no payload at all, + # so the desktop app would reinstall over an environment value instead. + except SystemExit as exc: + payload = { + "runtime_ready": False, + "reason": "backend_startup_failed", + "error_type": "SystemExit", + "error": str(exc), + } except Exception as exc: payload = { "runtime_ready": False, diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 9eace51d60..2bbd1c7c0b 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -94,7 +94,7 @@ def test_desktop_runtime_check_accepts_a_prerelease_over_a_floor(monkeypatch, ca monkeypatch.setattr( importlib.import_module("importlib.metadata"), "distribution", - lambda _name: SimpleNamespace(version = "2.0.0b1"), + lambda _name: SimpleNamespace(version = "2.0.0b1", files = []), ) studio.desktop_runtime_check(_json_output = True) @@ -113,7 +113,7 @@ def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp monkeypatch.setattr( importlib.import_module("importlib.metadata"), "distribution", - lambda _name: SimpleNamespace(version = "1.0"), + lambda _name: SimpleNamespace(version = "1.0", files = []), ) with pytest.raises(typer.Exit): @@ -121,3 +121,53 @@ def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp payload = json.loads(capsys.readouterr().out) assert payload["module"] == "example-package" + + +def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package( + monkeypatch, capsys, tmp_path, +): + """fastapi's wheel stores .dist-info/METADATA as its first archive entry, so + an interrupted unpack leaves a readable version for modules that never + landed. RECORD is written last, so its absence marks the unfinished unpack.""" + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text("fastapi\n", encoding = "utf-8") + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + monkeypatch.setattr( + importlib.import_module("importlib.metadata"), + "distribution", + lambda _name: SimpleNamespace(version = "0.140.5", files = None), + ) + + with pytest.raises(typer.Exit): + studio.desktop_runtime_check(_json_output = True) + + payload = json.loads(capsys.readouterr().out) + assert payload["reason"] == "missing_dependency" + assert payload["module"] == "fastapi" + + +def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting( + monkeypatch, capsys, +): + """run.py raises SystemExit for values such as UNSLOTH_CPU_THREADS=invalid. + Escaping without a payload makes the desktop app reinstall over an + environment value no install can change.""" + studio = importlib.import_module("unsloth_cli.commands.studio") + + def _rejected_setting(): + raise SystemExit("Error: Invalid UNSLOTH_CPU_THREADS value 'invalid'") + + monkeypatch.setattr(studio, "_load_run_module", _rejected_setting) + + with pytest.raises(typer.Exit) as exited: + studio.desktop_runtime_check(_json_output = True) + + assert exited.value.exit_code == 1 + payload = json.loads(capsys.readouterr().out) + assert payload["runtime_ready"] is False + assert payload["reason"] == "backend_startup_failed" + assert "UNSLOTH_CPU_THREADS" in payload["error"] From 4482f8b4f1d6985501265a97a149a49d860f518b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 16:25:52 +0000 Subject: [PATCH 05/16] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- unsloth_cli/tests/test_studio_runtime_readiness.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 2bbd1c7c0b..84aeb1603b 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -124,7 +124,7 @@ def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package( - monkeypatch, capsys, tmp_path, + monkeypatch, capsys, tmp_path ): """fastapi's wheel stores .dist-info/METADATA as its first archive entry, so an interrupted unpack leaves a readable version for modules that never @@ -150,9 +150,7 @@ def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package( assert payload["module"] == "fastapi" -def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting( - monkeypatch, capsys, -): +def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting(monkeypatch, capsys): """run.py raises SystemExit for values such as UNSLOTH_CPU_THREADS=invalid. Escaping without a payload makes the desktop app reinstall over an environment value no install can change.""" From f749c90d87d82effc2c1092baf99c5ef8ca9513f Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 17:19:52 +0000 Subject: [PATCH 06/16] 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), + } +} From e15ce94398293998540516836d23e01e497794b1 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 17:19:52 +0000 Subject: [PATCH 07/16] Check what the backend actually needs to start, not just studio.txt The readiness probe walked only the requirement file's own lines, so a package that reaches the venv as someone else's dependency read as present. starlette is the live case: studio/backend/main.py imports it, FastAPI is what pulls it in, and run.py imports main inside run_server, so the probe passed and the server died on launch. Walk the installed metadata instead, markers applied and extras skipped. Version bounds stay limited to studio.txt's own pins: install_python_stack installs with --no-deps, so a transitive bound can read unsatisfied in a venv that works, and reporting it would send repair round a loop it cannot break. 56 distributions on a full closure, 46ms, no imports. --- unsloth_cli/commands/studio.py | 47 ++++++++++++- .../tests/test_studio_runtime_readiness.py | 67 ++++++++++++++++++- 2 files changed, 108 insertions(+), 6 deletions(-) diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index c6cae42b83..702d33c4e0 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -270,11 +270,17 @@ def _load_run_module(): return _RUN_MODULE -def _missing_studio_requirement(run_mod): - from importlib.metadata import PackageNotFoundError, distribution +def _canonical_distribution_name(name: str) -> str: + """PEP 503 normalisation, so PyJWT / pyjwt / py_jwt compare equal.""" + return re.sub(r"[-_.]+", "-", name).lower() + + +def _studio_requirement_roots(run_mod): + """Requirements studio.txt names directly, markers already applied.""" from packaging.requirements import InvalidRequirement, Requirement requirements = Path(run_mod.__file__).with_name("requirements") / "studio.txt" + roots = [] for line in requirements.read_text(encoding = "utf-8").splitlines(): line = line.partition("#")[0].strip() # pip flags (-r, --extra-index-url) are not requirements. Skipping them @@ -288,6 +294,29 @@ def _missing_studio_requirement(run_mod): continue if requirement.marker and not requirement.marker.evaluate(): continue + roots.append(requirement) + return roots + + +def _missing_studio_requirement(run_mod): + """First requirement studio.txt needs that this venv cannot supply. + + Walks dependencies too: starlette is imported by studio/backend/main.py but + reaches the venv only as a FastAPI dependency, and main is imported inside + run_server, so a direct-only check calls the install ready and the server + still dies on startup. Metadata only, no imports, ~80ms on a full venv. + """ + from importlib.metadata import PackageNotFoundError, distribution + from packaging.requirements import InvalidRequirement, Requirement + + pending = [(root, True) for root in reversed(_studio_requirement_roots(run_mod))] + seen = set() + while pending: + requirement, is_root = pending.pop() + key = _canonical_distribution_name(requirement.name) + if key in seen: + continue + seen.add(key) try: installed = distribution(requirement.name) except PackageNotFoundError: @@ -298,11 +327,23 @@ def _missing_studio_requirement(run_mod): # landed. RECORD is written last, so its absence marks that unpack. if installed.files is None: return requirement.name + # Only studio.txt pins are enforced. install_python_stack installs torch + # and friends with --no-deps, so a transitive bound can read unsatisfied + # in a venv that works, and repair reinstalls studio.txt either way. # prereleases=True: a prerelease satisfying a floor is not a broken install. - if requirement.specifier and not requirement.specifier.contains( + if is_root and requirement.specifier and not requirement.specifier.contains( installed.version, prereleases = True ): return requirement.name + for dependency in installed.requires or []: + try: + parsed = Requirement(dependency) + except InvalidRequirement: + continue + # No extra is requested, so extras-only dependencies do not apply. + if parsed.marker and not parsed.marker.evaluate({"extra": ""}): + continue + pending.append((parsed, False)) return None diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 84aeb1603b..bc843a5bcb 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -94,7 +94,7 @@ def test_desktop_runtime_check_accepts_a_prerelease_over_a_floor(monkeypatch, ca monkeypatch.setattr( importlib.import_module("importlib.metadata"), "distribution", - lambda _name: SimpleNamespace(version = "2.0.0b1", files = []), + lambda _name: SimpleNamespace(version = "2.0.0b1", files = [], requires = None), ) studio.desktop_runtime_check(_json_output = True) @@ -113,7 +113,7 @@ def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp monkeypatch.setattr( importlib.import_module("importlib.metadata"), "distribution", - lambda _name: SimpleNamespace(version = "1.0", files = []), + lambda _name: SimpleNamespace(version = "1.0", files = [], requires = None), ) with pytest.raises(typer.Exit): @@ -139,7 +139,7 @@ def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package( monkeypatch.setattr( importlib.import_module("importlib.metadata"), "distribution", - lambda _name: SimpleNamespace(version = "0.140.5", files = None), + lambda _name: SimpleNamespace(version = "0.140.5", files = None, requires = None), ) with pytest.raises(typer.Exit): @@ -150,6 +150,67 @@ def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package( assert payload["module"] == "fastapi" +def _fake_distributions(monkeypatch, installed): + metadata = importlib.import_module("importlib.metadata") + + def _distribution(name): + try: + version, requires = installed[name] + except KeyError: + raise metadata.PackageNotFoundError(name) from None + return SimpleNamespace(version = version, files = [], requires = requires) + + monkeypatch.setattr(metadata, "distribution", _distribution) + + +def test_desktop_runtime_check_rejects_a_missing_transitive_dependency( + monkeypatch, capsys, tmp_path +): + """starlette reaches the venv only as a FastAPI dependency, and the backend + imports it from main.py, which run.py imports inside run_server. A + direct-only check calls the install ready and the server dies on start.""" + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text("fastapi>=0.115\n", encoding = "utf-8") + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + _fake_distributions(monkeypatch, {"fastapi": ("0.140.5", ["starlette>=0.40"])}) + + with pytest.raises(typer.Exit): + studio.desktop_runtime_check(_json_output = True) + + assert json.loads(capsys.readouterr().out)["module"] == "starlette" + + +def test_desktop_runtime_check_ignores_optional_and_circular_dependencies( + monkeypatch, capsys, tmp_path +): + """No extra is requested, so an extras-only dependency is not missing, and a + dependency cycle must terminate rather than walk forever.""" + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text("fastapi\n", encoding = "utf-8") + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + _fake_distributions( + monkeypatch, + { + "fastapi": ("0.140.5", ['uvicorn; extra == "standard"', "starlette"]), + # Transitive bounds are not enforced: install_python_stack installs + # with --no-deps, so an unmet one can describe a working venv. + "starlette": ("0.1", ["fastapi>=99"]), + }, + ) + + studio.desktop_runtime_check(_json_output = True) + + assert json.loads(capsys.readouterr().out) == {"runtime_ready": True} + + def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting(monkeypatch, capsys): """run.py raises SystemExit for values such as UNSLOTH_CPU_THREADS=invalid. Escaping without a payload makes the desktop app reinstall over an From 22c6c277ba8a4447068d56183604b2e68882625e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:20:37 +0000 Subject: [PATCH 08/16] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- unsloth_cli/commands/studio.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 702d33c4e0..121dbb9649 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -331,8 +331,10 @@ def _missing_studio_requirement(run_mod): # and friends with --no-deps, so a transitive bound can read unsatisfied # in a venv that works, and repair reinstalls studio.txt either way. # prereleases=True: a prerelease satisfying a floor is not a broken install. - if is_root and requirement.specifier and not requirement.specifier.contains( - installed.version, prereleases = True + if ( + is_root + and requirement.specifier + and not requirement.specifier.contains(installed.version, prereleases = True) ): return requirement.name for dependency in installed.requires or []: From aaa00ae8d24a6fefb2fd39b05363ea7212f3125e Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 17:43:27 +0000 Subject: [PATCH 09/16] Answer four ways an install can look ready when it is not Roots before the walk: reached as someone else's dependency first, a studio.txt line marked itself seen and skipped its own pin. datasets asks for huggingface-hub>=0.25,<2 and studio.txt pins ==0.36.2, so the pin never got to decide. A start rejected because one is already running is not a spawn failure. The loser was clearing the winner's marker while pip was still inside the venv, which is the one signal an interrupted install leaves behind. A CLI too old for desktop-runtime-check is the CLI those interrupted installs shipped with, and -h passes without touching the backend, so accepting it left exactly those users on the crash path. It is stale for its own reason now; the update installs one that can answer. Owner metadata is removed only while it is still ours. Cleanup after an exit can spend seconds terminating descendants, and a start in that window has already written the next backend's file to the same path. --- studio/src-tauri/src/desktop_backend_owner.rs | 42 +++++++++++++++++++ studio/src-tauri/src/install.rs | 14 +++++-- studio/src-tauri/src/preflight.rs | 14 ++++--- studio/src-tauri/src/preflight/managed.rs | 42 +++++++++++-------- unsloth_cli/commands/studio.py | 33 +++++++++++---- .../tests/test_studio_runtime_readiness.py | 28 +++++++++++++ 6 files changed, 139 insertions(+), 34 deletions(-) diff --git a/studio/src-tauri/src/desktop_backend_owner.rs b/studio/src-tauri/src/desktop_backend_owner.rs index c7d0a7b309..95b606b111 100644 --- a/studio/src-tauri/src/desktop_backend_owner.rs +++ b/studio/src-tauri/src/desktop_backend_owner.rs @@ -265,7 +265,21 @@ impl BackendOwnerState { self.write() } + /// Only while the file is still ours. Cleanup after a backend exits can run + /// for seconds terminating descendants, and a start in that window has + /// already written the next backend's metadata to this same path. pub(crate) fn remove(self) { + if let Ok(Some(on_disk)) = read_metadata(&self.path) { + if on_disk.token_sha256 != self.metadata.token_sha256 { + warn!( + "Leaving desktop backend owner metadata at {}: a newer backend owns it", + self.path.display() + ); + return; + } + } + // Unreadable counts as ours: a file nothing can parse would otherwise + // outlive every backend and fail each later ownership check. remove_metadata_file(&self.path); } @@ -978,6 +992,34 @@ mod tests { dir.join("desktop_backend.json") } + #[test] + fn a_replaced_owner_file_survives_the_previous_backend_cleanup() { + // Cleanup after an exit can take seconds; a start in that window has + // already written the next backend's metadata to the same path. + let path = temp_metadata_path("replaced-owner"); + let exited = BackendOwnerState::from_metadata(path.clone(), metadata(1, Some(8888))); + let mut current = metadata(2, Some(8899)); + current.token = "next-backend-token".to_string(); + current.token_sha256 = token_sha256("next-backend-token"); + write_metadata(&path, ¤t).unwrap(); + + exited.remove(); + + let left = read_metadata(&path).unwrap().unwrap(); + assert_eq!(left.token_sha256, token_sha256("next-backend-token")); + } + + #[test] + fn an_owner_still_holding_the_file_removes_it() { + let path = temp_metadata_path("own-owner"); + let owner = BackendOwnerState::from_metadata(path.clone(), metadata(1, Some(8888))); + write_metadata(&path, &metadata(1, Some(8888))).unwrap(); + + owner.remove(); + + assert!(read_metadata(&path).unwrap().is_none()); + } + fn closed_port() -> u16 { let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).unwrap(); let port = listener.local_addr().unwrap().port(); diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index 3d4f8a3e9d..fc149aba0f 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -193,6 +193,10 @@ fn emit_complete(app: &AppHandle) { // ── Spawn ── +/// A start rejected because one is already running. Distinguished from a real +/// spawn failure so the loser of the race leaves the winner's marker alone. +const INSTALL_ALREADY_RUNNING: &str = "Installation is already running."; + /// Spawns the install script in a process group. /// Returns (stdout, stderr) handles for streaming. /// The GroupChild is stored in state so stop_install() can kill the entire tree. @@ -209,7 +213,7 @@ fn spawn_script( > { let mut install = state.lock().map_err(|e| e.to_string())?; if install.child.is_some() { - return Err("Installation is already running.".to_string()); + return Err(INSTALL_ALREADY_RUNNING.to_string()); } install.intentional_stop = false; install.needed_packages.clear(); @@ -514,8 +518,12 @@ fn run_install_with_event_mode( let (stdout, stderr) = match spawn_script(&script, &args, &state) { Ok(handles) => handles, Err(msg) => { - // Nothing ran, so nothing is half-installed. - clear_install_marker_best_effort(); + // Nothing ran, so nothing is half-installed. Unless another + // installer is already inside the venv, in which case the marker + // is its own and clearing it here throws away its recovery signal. + if msg != INSTALL_ALREADY_RUNNING { + clear_install_marker_best_effort(); + } diagnostics::finish_attempt( &diagnostics, &attempt, diff --git a/studio/src-tauri/src/preflight.rs b/studio/src-tauri/src/preflight.rs index 599f5c0fa2..3032bb4e63 100644 --- a/studio/src-tauri/src/preflight.rs +++ b/studio/src-tauri/src/preflight.rs @@ -410,6 +410,7 @@ mod tests { for reason in [ "studio_runtime_missing_dependency", "studio_runtime_import_failed", + "studio_runtime_check_unsupported", "install_incomplete", "desktop_backend_version_too_old", ] { @@ -750,10 +751,12 @@ exit 1 #[cfg(unix)] #[tokio::test] - async fn managed_cli_predating_the_runtime_check_is_not_forced_into_repair() { + async fn a_cli_predating_the_runtime_check_is_stale_for_its_own_reason() { // Every published CLI satisfies MIN_DESKTOP_BACKEND_VERSION but has no // `desktop-runtime-check`, so click exits 2 with an empty stdout. That - // must not repair an install that still launches Studio. + // is the CLI the interrupted installs shipped with, and -h passes + // without touching the backend, so launchable is not ready. Update it + // first, and keep the reason apart from a CLI that cannot answer. let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; let _cache_home = ManagedCapabilityCacheHome::new("runtime-check-absent"); remove_managed_capability_cache(); @@ -775,11 +778,12 @@ exit 1 ); let probe = probe_managed_bin(old.bin.clone()).await; assert!( - matches!(probe, ManagedProbe::Ready { .. }), - "CLI without the runtime-check subcommand must stay Ready, got {probe:?}" + matches!(&probe, ManagedProbe::Stale { reason, .. } + if reason == "studio_runtime_check_unsupported"), + "CLI without the runtime-check subcommand must be stale for it, got {probe:?}" ); - // The fallback must not rescue a CLI that cannot launch at all. + // And a CLI that cannot launch at all keeps the plain probe failure. remove_managed_capability_cache(); let broken = fake_cli( "runtime-check-unlaunchable", diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index 08cca786ce..4a6e552fcd 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -303,10 +303,16 @@ async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { } } +/// The probe could not answer: the binary would not run, or it exited without +/// a payload. Distinct from a CLI that is simply too old to know the command. +const RUNTIME_PROBE_FAILED: &str = "studio_runtime_probe_failed"; /// 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"; +/// A CLI predating the runtime probe. Repairable, and the update installs one +/// that can answer, so the next preflight checks the venv for real. +const RUNTIME_CHECK_UNSUPPORTED: &str = "studio_runtime_check_unsupported"; /// 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 @@ -347,10 +353,10 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { "Managed runtime probe failed to spawn in {}ms", started.elapsed().as_millis() ); - return Err("studio_runtime_probe_failed".to_string()); + return Err(RUNTIME_PROBE_FAILED.to_string()); }; let Some(mut stdout) = child.stdout.take() else { - return Err("studio_runtime_probe_failed".to_string()); + return Err(RUNTIME_PROBE_FAILED.to_string()); }; // Drain while waiting: the backend import this probe runs can print more than @@ -377,7 +383,7 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { }; let Ok(output) = reader.await else { - return Err("studio_runtime_probe_failed".to_string()); + return Err(RUNTIME_PROBE_FAILED.to_string()); }; let payload = String::from_utf8_lossy(&output) .lines() @@ -396,11 +402,11 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { } Some("backend_import_failed") => "studio_runtime_import_failed", Some("backend_startup_failed") => super::STUDIO_RUNTIME_STARTUP_FAILED, - _ => "studio_runtime_probe_failed", + _ => RUNTIME_PROBE_FAILED, }; Err(reason.to_string()) } - None => Err("studio_runtime_probe_failed".to_string()), + None => Err(RUNTIME_PROBE_FAILED.to_string()), }; info!( "Managed runtime probe finished ok={} in {}ms", @@ -520,21 +526,21 @@ pub(super) async fn probe_managed_bin(bin: PathBuf) -> ManagedProbe { // fingerprint only proves protocol compatibility, not that Studio's backend // imports are complete after an interrupted dependency transaction. if let Err(reason) = probe_cli_runtime(&bin).await { - // Every released CLI predates this subcommand, so a missing-command exit - // must not strand an install the launch probe still accepts. - if reason != "studio_runtime_probe_failed" || !predates_runtime_check(&bin).await { - info!( - "Managed preflight: runtime unusable for {:?} reason={} in {}ms", - bin, - reason, - started.elapsed().as_millis() - ); - return ManagedProbe::Stale { bin, reason }; - } + // A CLI too old for this subcommand is the one the interrupted installs + // shipped with, and -h passes without touching the backend, so accepting + // it here leaves those users on the same crash. Update it, then ask. + let reason = if reason == RUNTIME_PROBE_FAILED && predates_runtime_check(&bin).await { + RUNTIME_CHECK_UNSUPPORTED.to_string() + } else { + reason + }; info!( - "Managed preflight: cli predates the runtime probe for {:?}; using the launch probe", - bin + "Managed preflight: runtime unusable for {:?} reason={} in {}ms", + bin, + reason, + started.elapsed().as_millis() ); + return ManagedProbe::Stale { bin, reason }; } if let Some(fingerprint) = managed_bin_fingerprint(&bin) { diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 121dbb9649..6baea99f74 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -309,14 +309,11 @@ def _missing_studio_requirement(run_mod): from importlib.metadata import PackageNotFoundError, distribution from packaging.requirements import InvalidRequirement, Requirement - pending = [(root, True) for root in reversed(_studio_requirement_roots(run_mod))] + pending = [] seen = set() - while pending: - requirement, is_root = pending.pop() - key = _canonical_distribution_name(requirement.name) - if key in seen: - continue - seen.add(key) + + def visit(requirement, is_root): + """The name to report, or None; queues the requirement's dependencies.""" try: installed = distribution(requirement.name) except PackageNotFoundError: @@ -345,7 +342,27 @@ def _missing_studio_requirement(run_mod): # No extra is requested, so extras-only dependencies do not apply. if parsed.marker and not parsed.marker.evaluate({"extra": ""}): continue - pending.append((parsed, False)) + pending.append(parsed) + return None + + # Every root is checked before the walk starts. Reached as a dependency + # first, a root would mark itself seen and never meet its own pin: datasets + # asks for huggingface-hub>=0.25,<2 and studio.txt pins it to ==0.36.2. + roots = _studio_requirement_roots(run_mod) + seen.update(_canonical_distribution_name(root.name) for root in roots) + for requirement in roots: + missing = visit(requirement, True) + if missing: + return missing + while pending: + requirement = pending.pop() + key = _canonical_distribution_name(requirement.name) + if key in seen: + continue + seen.add(key) + missing = visit(requirement, False) + if missing: + return missing return None diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index bc843a5bcb..e215a669f3 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -230,3 +230,31 @@ def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting(mon assert payload["runtime_ready"] is False assert payload["reason"] == "backend_startup_failed" assert "UNSLOTH_CPU_THREADS" in payload["error"] + + +def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first( + monkeypatch, capsys, tmp_path +): + """datasets asks for huggingface-hub>=0.25,<2 and studio.txt pins ==0.36.2. + Reached as a dependency first, the pin would never get to decide.""" + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text( + "datasets==4.3.0\nhuggingface-hub==0.36.2\n", encoding = "utf-8", + ) + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + _fake_distributions( + monkeypatch, + { + "datasets": ("4.3.0", ["huggingface-hub>=0.25,<2"]), + "huggingface-hub": ("1.25.1", None), + }, + ) + + with pytest.raises(typer.Exit): + studio.desktop_runtime_check(_json_output = True) + + assert json.loads(capsys.readouterr().out)["module"] == "huggingface-hub" From ddb2c1d397a090d522738a658a44214088a5bbdf Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:44:11 +0000 Subject: [PATCH 10/16] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- unsloth_cli/tests/test_studio_runtime_readiness.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index e215a669f3..80b51a875e 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -232,9 +232,7 @@ def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting(mon assert "UNSLOTH_CPU_THREADS" in payload["error"] -def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first( - monkeypatch, capsys, tmp_path -): +def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first(monkeypatch, capsys, tmp_path): """datasets asks for huggingface-hub>=0.25,<2 and studio.txt pins ==0.36.2. Reached as a dependency first, the pin would never get to decide.""" studio = importlib.import_module("unsloth_cli.commands.studio") @@ -242,7 +240,8 @@ def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first( requirements = backend / "requirements" requirements.mkdir(parents = True) (requirements / "studio.txt").write_text( - "datasets==4.3.0\nhuggingface-hub==0.36.2\n", encoding = "utf-8", + "datasets==4.3.0\nhuggingface-hub==0.36.2\n", + encoding = "utf-8", ) run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) From 508d3720c4dbea7e5b71f27c87780c065c3885be Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 17:54:28 +0000 Subject: [PATCH 11/16] Tighten comments across the install readiness changes --- studio/src-tauri/src/commands.rs | 7 +--- studio/src-tauri/src/desktop_backend_owner.rs | 10 ++--- studio/src-tauri/src/install.rs | 24 +++++------- studio/src-tauri/src/preflight.rs | 25 +++++-------- studio/src-tauri/src/preflight/managed.rs | 33 +++++++---------- unsloth_cli/commands/studio.py | 37 ++++++++----------- .../tests/test_studio_runtime_readiness.py | 29 ++++++--------- 7 files changed, 65 insertions(+), 100 deletions(-) diff --git a/studio/src-tauri/src/commands.rs b/studio/src-tauri/src/commands.rs index 813d8c6ec8..ddacbee71e 100644 --- a/studio/src-tauri/src/commands.rs +++ b/studio/src-tauri/src/commands.rs @@ -19,8 +19,7 @@ async fn managed_install_ready_after_repair() -> bool { } /// 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. +/// environment value. No reinstall changes that, so report it instead. fn unrepairable_after_update(reason: Option<&str>) -> Option { match reason { Some(crate::preflight::STUDIO_RUNTIME_STARTUP_FAILED) => Some( @@ -793,9 +792,7 @@ mod tests { } #[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. + // This arm is the one that would otherwise reinstall over a healthy tree. let msg = super::unrepairable_after_update(Some(crate::preflight::STUDIO_RUNTIME_STARTUP_FAILED)) .expect("a rejected setting must stop repair"); diff --git a/studio/src-tauri/src/desktop_backend_owner.rs b/studio/src-tauri/src/desktop_backend_owner.rs index 95b606b111..5e0c0425e6 100644 --- a/studio/src-tauri/src/desktop_backend_owner.rs +++ b/studio/src-tauri/src/desktop_backend_owner.rs @@ -265,9 +265,8 @@ impl BackendOwnerState { self.write() } - /// Only while the file is still ours. Cleanup after a backend exits can run - /// for seconds terminating descendants, and a start in that window has - /// already written the next backend's metadata to this same path. + /// Only while the file is still ours. Cleanup after an exit can take seconds, + /// and a start in that window already wrote the next metadata to this path. pub(crate) fn remove(self) { if let Ok(Some(on_disk)) = read_metadata(&self.path) { if on_disk.token_sha256 != self.metadata.token_sha256 { @@ -278,8 +277,7 @@ impl BackendOwnerState { return; } } - // Unreadable counts as ours: a file nothing can parse would otherwise - // outlive every backend and fail each later ownership check. + // Unreadable counts as ours, or it would outlive every backend. remove_metadata_file(&self.path); } @@ -994,8 +992,6 @@ mod tests { #[test] fn a_replaced_owner_file_survives_the_previous_backend_cleanup() { - // Cleanup after an exit can take seconds; a start in that window has - // already written the next backend's metadata to the same path. let path = temp_metadata_path("replaced-owner"); let exited = BackendOwnerState::from_metadata(path.clone(), metadata(1, Some(8888))); let mut current = metadata(2, Some(8899)); diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index fc149aba0f..74e560a921 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -83,11 +83,9 @@ fn clear_install_in_progress_marker() -> Result<(), String> { } /// 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. +/// spawn, or it asked for elevation, which install.sh decides (:1931) before it +/// creates the venv (:2120). Later on any failure can leave pip part-way +/// through, and the marker is the only signal. Never fatal: it costs a fast path. fn clear_install_marker_best_effort() { if let Err(msg) = clear_install_in_progress_marker() { warn!("[install] {}", msg); @@ -193,7 +191,7 @@ fn emit_complete(app: &AppHandle) { // ── Spawn ── -/// A start rejected because one is already running. Distinguished from a real +/// A start rejected because one is already running, kept apart from a real /// spawn failure so the loser of the race leaves the winner's marker alone. const INSTALL_ALREADY_RUNNING: &str = "Installation is already running."; @@ -518,9 +516,8 @@ fn run_install_with_event_mode( let (stdout, stderr) = match spawn_script(&script, &args, &state) { Ok(handles) => handles, Err(msg) => { - // Nothing ran, so nothing is half-installed. Unless another - // installer is already inside the venv, in which case the marker - // is its own and clearing it here throws away its recovery signal. + // Nothing ran, so nothing is half-installed. Unless another installer + // owns the marker, where clearing it drops its recovery signal. if msg != INSTALL_ALREADY_RUNNING { clear_install_marker_best_effort(); } @@ -587,8 +584,7 @@ fn run_install_with_event_mode( let _ = app.emit(event_mode.needs_elevation_event(), &packages); Err("NEEDS_ELEVATION".to_string()) } else { - // Keep the marker: the script ran, so pip may have replaced or - // removed packages before it failed. + // Keep the marker: the script ran, so pip may be part-way through. let msg = format!("Installer exited with code {}", code); diagnostics::finish_attempt( &diagnostics, @@ -651,8 +647,7 @@ pub fn record_pending_elevation_canceled( let Some(attempt) = attempt else { return false; }; - // The elevation exit left the marker in place for the resumed run that is - // now not happening. + // The resumed run the elevation exit left the marker for is not happening. clear_install_marker_best_effort(); diagnostics::finish_attempt( diagnostics, @@ -926,8 +921,7 @@ fn finish_elevation_failure( exit_status: Option, message: String, ) { - // Terminal, like a cancelled prompt: the run the code 2 exit left the - // marker for is not happening. + // Terminal, like a cancelled prompt: the resumed run is not happening. clear_install_marker_best_effort(); if let Some(attempt) = attempt { diagnostics::finish_attempt( diff --git a/studio/src-tauri/src/preflight.rs b/studio/src-tauri/src/preflight.rs index 3032bb4e63..b9046eaa26 100644 --- a/studio/src-tauri/src/preflight.rs +++ b/studio/src-tauri/src/preflight.rs @@ -24,7 +24,7 @@ use managed::probe_managed_bin; #[cfg(test)] use version::{backend_version_compatible, MIN_DESKTOP_BACKEND_VERSION}; -/// The managed backend refused to start on a value it inherited from the +/// The managed backend refused to start on a value inherited from the /// environment rather than from the install. pub(crate) const STUDIO_RUNTIME_STARTUP_FAILED: &str = "studio_runtime_startup_failed"; @@ -33,7 +33,7 @@ fn release_auto_repair() -> bool { } /// Reinstalling cannot change a rejected environment value, so repairing over -/// one only replaces a healthy install and fails the same way afterwards. +/// one only replaces a healthy install and fails the same way. fn stale_reason_is_repairable(reason: &str) -> bool { reason != STUDIO_RUNTIME_STARTUP_FAILED } @@ -404,8 +404,6 @@ mod tests { #[test] fn rejected_backend_settings_do_not_auto_repair() { - // No reinstall can change an inherited environment value, so repairing - // would replace a healthy install and fail again the same way. assert!(!stale_reason_is_repairable(STUDIO_RUNTIME_STARTUP_FAILED)); for reason in [ "studio_runtime_missing_dependency", @@ -685,8 +683,7 @@ exit 1 #[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. + // reaches the timeout, and two more 10s probes would treble the wait. let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; let _cache_home = ManagedCapabilityCacheHome::new("runtime-hang"); remove_managed_capability_cache(); @@ -713,9 +710,8 @@ sleep 120 #[cfg(unix)] #[tokio::test] async fn managed_runtime_probe_survives_more_stdout_than_the_pipe_holds() { - // The probe imports the whole backend, so import-time output can exceed - // the 64 KiB pipe buffer. Waiting on exit before draining wedges the - // child mid-write and reports a healthy install as broken. + // The probe imports the whole backend, so output can exceed the 64 KiB + // pipe buffer, and waiting before draining wedges the child mid-write. let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; let _cache_home = ManagedCapabilityCacheHome::new("runtime-noisy"); remove_managed_capability_cache(); @@ -753,10 +749,9 @@ exit 1 #[tokio::test] async fn a_cli_predating_the_runtime_check_is_stale_for_its_own_reason() { // Every published CLI satisfies MIN_DESKTOP_BACKEND_VERSION but has no - // `desktop-runtime-check`, so click exits 2 with an empty stdout. That - // is the CLI the interrupted installs shipped with, and -h passes - // without touching the backend, so launchable is not ready. Update it - // first, and keep the reason apart from a CLI that cannot answer. + // `desktop-runtime-check`, so click exits 2 with an empty stdout. It is + // what the interrupted installs shipped with, and -h passes without + // touching the backend, so keep it apart from a CLI that cannot answer. let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; let _cache_home = ManagedCapabilityCacheHome::new("runtime-check-absent"); remove_managed_capability_cache(); @@ -810,8 +805,8 @@ exit 2 let _cache_home = ManagedCapabilityCacheHome::new("cache-hit"); remove_managed_capability_cache(); - // Runtime readiness is checked on every probe, while the static - // desktop-capabilities probe is skipped once the cache is warm. + // Runtime readiness runs on every probe; the capability probe is skipped + // once the cache is warm. let fake = fake_cli( "cap-cache-hit", r#"#!/bin/sh diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index 4a6e552fcd..ba38523688 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -303,21 +303,19 @@ async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { } } -/// The probe could not answer: the binary would not run, or it exited without -/// a payload. Distinct from a CLI that is simply too old to know the command. +/// The probe could not answer: it would not run, or exited without a payload. +/// Distinct from a CLI that is simply too old to know the command. const RUNTIME_PROBE_FAILED: &str = "studio_runtime_probe_failed"; /// 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. +/// unknown command at once, so only a broken one reaches the timeout. const RUNTIME_PROBE_TIMEOUT: &str = "studio_runtime_probe_timeout"; -/// A CLI predating the runtime probe. Repairable, and the update installs one -/// that can answer, so the next preflight checks the venv for real. +/// A CLI predating the runtime probe. Repairable: the update installs one that +/// can answer, so the next preflight checks the venv for real. const RUNTIME_CHECK_UNSUPPORTED: &str = "studio_runtime_check_unsupported"; /// 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 -/// the legacy launch probe keeps a genuinely unusable binary out of this path. +/// Its usage error is indistinguishable from a crashed probe, so `--help` +/// resolves the command without running it. async fn predates_runtime_check(bin: &Path) -> bool { !run_cli_probe(bin, &["studio", "desktop-runtime-check", "--help"]).await && run_cli_probe(bin, &["-h"]).await @@ -359,9 +357,8 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { return Err(RUNTIME_PROBE_FAILED.to_string()); }; - // Drain while waiting: the backend import this probe runs can print more than - // the pipe buffer holds, and waiting first deadlocks on it, timing out a - // healthy install into repair. + // Drain while waiting: the backend import can print more than the pipe holds, + // and waiting first deadlocks, timing out a healthy install into repair. let reader = tokio::spawn(async move { let mut buffer = Vec::new(); let _ = stdout.read_to_end(&mut buffer).await; @@ -522,13 +519,11 @@ fn desktop_capability_ready(capability: &DesktopCapability) -> bool { pub(super) async fn probe_managed_bin(bin: PathBuf) -> ManagedProbe { let started = Instant::now(); - // Runtime readiness is intentionally uncached. A matching capability - // fingerprint only proves protocol compatibility, not that Studio's backend - // imports are complete after an interrupted dependency transaction. + // Runtime readiness is intentionally uncached: a matching capability + // fingerprint proves protocol compatibility, not that the venv is complete. if let Err(reason) = probe_cli_runtime(&bin).await { - // A CLI too old for this subcommand is the one the interrupted installs - // shipped with, and -h passes without touching the backend, so accepting - // it here leaves those users on the same crash. Update it, then ask. + // A CLI too old for this subcommand is what the interrupted installs + // shipped with, and -h passes without touching the backend. let reason = if reason == RUNTIME_PROBE_FAILED && predates_runtime_check(&bin).await { RUNTIME_CHECK_UNSUPPORTED.to_string() } else { @@ -619,7 +614,7 @@ pub async fn managed_install_ready() -> bool { } /// Ready, or the reason it is not, so repair can tell an unrepairable cause -/// apart from a stale install rather than reinstalling over both. +/// apart from a stale install. pub async fn managed_install_state() -> Result<(), Option> { match probe_managed_install().await { ManagedProbe::Ready { .. } => Ok(()), diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 6baea99f74..43c6634865 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -283,9 +283,8 @@ def _studio_requirement_roots(run_mod): roots = [] for line in requirements.read_text(encoding = "utf-8").splitlines(): line = line.partition("#")[0].strip() - # pip flags (-r, --extra-index-url) are not requirements. Skipping them - # matters: an unparseable line would report the install broken, and - # repair reinstalls the same file, so the failure would never clear. + # pip flags (-r, --extra-index-url) are not requirements. Treating one + # as broken is permanent: repair reinstalls the same file. if not line or line.startswith("-"): continue try: @@ -301,10 +300,9 @@ def _studio_requirement_roots(run_mod): def _missing_studio_requirement(run_mod): """First requirement studio.txt needs that this venv cannot supply. - Walks dependencies too: starlette is imported by studio/backend/main.py but - reaches the venv only as a FastAPI dependency, and main is imported inside - run_server, so a direct-only check calls the install ready and the server - still dies on startup. Metadata only, no imports, ~80ms on a full venv. + Walks dependencies too: starlette reaches the venv only via FastAPI and is + imported inside run_server, so a direct-only check calls a doomed install + ready. Metadata only, no imports, ~80ms on a full venv. """ from importlib.metadata import PackageNotFoundError, distribution from packaging.requirements import InvalidRequirement, Requirement @@ -313,21 +311,18 @@ def _missing_studio_requirement(run_mod): seen = set() def visit(requirement, is_root): - """The name to report, or None; queues the requirement's dependencies.""" + """Missing name, or None; queues the requirement's dependencies.""" try: installed = distribution(requirement.name) except PackageNotFoundError: return requirement.name - # Metadata outlives the package it describes: hatchling wheels (fastapi, - # typer) store .dist-info/METADATA as the first archive entry, so an - # unpack killed midway leaves a readable version for modules that never - # landed. RECORD is written last, so its absence marks that unpack. + # Metadata outlives the package: wheels store METADATA first, so a killed + # unpack leaves a version behind. RECORD is last, so its absence marks it. if installed.files is None: return requirement.name - # Only studio.txt pins are enforced. install_python_stack installs torch - # and friends with --no-deps, so a transitive bound can read unsatisfied - # in a venv that works, and repair reinstalls studio.txt either way. - # prereleases=True: a prerelease satisfying a floor is not a broken install. + # Only studio.txt pins are enforced: install_python_stack uses --no-deps, + # so transitive bounds read unsatisfied in venvs that work. A prerelease + # satisfying a floor is not a broken install either. if ( is_root and requirement.specifier @@ -345,9 +340,8 @@ def _missing_studio_requirement(run_mod): pending.append(parsed) return None - # Every root is checked before the walk starts. Reached as a dependency - # first, a root would mark itself seen and never meet its own pin: datasets - # asks for huggingface-hub>=0.25,<2 and studio.txt pins it to ==0.36.2. + # Roots are seen up front: reached as a dependency first, a root would never + # meet its own pin (datasets wants huggingface-hub<2, studio.txt ==0.36.2). roots = _studio_requirement_roots(run_mod) seen.update(_canonical_distribution_name(root.name) for root in roots) for requirement in roots: @@ -2927,9 +2921,8 @@ def desktop_runtime_check( "reason": "missing_dependency", "module": exc.name, } - # SystemExit is not an Exception. run.py raises it for rejected settings such - # as UNSLOTH_CPU_THREADS, and letting it escape would emit no payload at all, - # so the desktop app would reinstall over an environment value instead. + # SystemExit is not an Exception. run.py raises it for rejected settings like + # UNSLOTH_CPU_THREADS; escaping emits no payload, so the app reinstalls. except SystemExit as exc: payload = { "runtime_ready": False, diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 80b51a875e..454638258b 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -65,8 +65,7 @@ def test_desktop_runtime_check_catches_later_startup_dependency(monkeypatch, cap def test_desktop_runtime_check_ignores_pip_flag_lines(monkeypatch, capsys, tmp_path): - """An unparseable line must not fail the check: repair reinstalls the same - file, so the install would be declared broken forever.""" + """Repair reinstalls the same file, so an unparseable line breaks it forever.""" studio = importlib.import_module("unsloth_cli.commands.studio") backend = tmp_path / "backend" requirements = backend / "requirements" @@ -126,9 +125,8 @@ def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package( monkeypatch, capsys, tmp_path ): - """fastapi's wheel stores .dist-info/METADATA as its first archive entry, so - an interrupted unpack leaves a readable version for modules that never - landed. RECORD is written last, so its absence marks the unfinished unpack.""" + """fastapi's wheel stores METADATA first, so an interrupted unpack leaves a + readable version behind. RECORD is last, so its absence marks the unpack.""" studio = importlib.import_module("unsloth_cli.commands.studio") backend = tmp_path / "backend" requirements = backend / "requirements" @@ -166,9 +164,8 @@ def _fake_distributions(monkeypatch, installed): def test_desktop_runtime_check_rejects_a_missing_transitive_dependency( monkeypatch, capsys, tmp_path ): - """starlette reaches the venv only as a FastAPI dependency, and the backend - imports it from main.py, which run.py imports inside run_server. A - direct-only check calls the install ready and the server dies on start.""" + """starlette reaches the venv only as a FastAPI dependency, so a direct-only + check calls the install ready and the server dies on start.""" studio = importlib.import_module("unsloth_cli.commands.studio") backend = tmp_path / "backend" requirements = backend / "requirements" @@ -187,8 +184,7 @@ def test_desktop_runtime_check_rejects_a_missing_transitive_dependency( def test_desktop_runtime_check_ignores_optional_and_circular_dependencies( monkeypatch, capsys, tmp_path ): - """No extra is requested, so an extras-only dependency is not missing, and a - dependency cycle must terminate rather than walk forever.""" + """Extras-only dependencies are not missing, and a cycle must terminate.""" studio = importlib.import_module("unsloth_cli.commands.studio") backend = tmp_path / "backend" requirements = backend / "requirements" @@ -200,8 +196,8 @@ def test_desktop_runtime_check_ignores_optional_and_circular_dependencies( monkeypatch, { "fastapi": ("0.140.5", ['uvicorn; extra == "standard"', "starlette"]), - # Transitive bounds are not enforced: install_python_stack installs - # with --no-deps, so an unmet one can describe a working venv. + # Transitive bounds are not enforced: --no-deps installs leave unmet + # ones on venvs that work. "starlette": ("0.1", ["fastapi>=99"]), }, ) @@ -212,9 +208,8 @@ def test_desktop_runtime_check_ignores_optional_and_circular_dependencies( def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting(monkeypatch, capsys): - """run.py raises SystemExit for values such as UNSLOTH_CPU_THREADS=invalid. - Escaping without a payload makes the desktop app reinstall over an - environment value no install can change.""" + """run.py raises SystemExit for values like UNSLOTH_CPU_THREADS=invalid, and + with no payload the app reinstalls over a value no install can change.""" studio = importlib.import_module("unsloth_cli.commands.studio") def _rejected_setting(): @@ -233,8 +228,8 @@ def test_desktop_runtime_check_reports_a_rejected_setting_instead_of_exiting(mon def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first(monkeypatch, capsys, tmp_path): - """datasets asks for huggingface-hub>=0.25,<2 and studio.txt pins ==0.36.2. - Reached as a dependency first, the pin would never get to decide.""" + """datasets wants huggingface-hub<2, studio.txt pins ==0.36.2. Reached as a + dependency first, the pin would never get to decide.""" studio = importlib.import_module("unsloth_cli.commands.studio") backend = tmp_path / "backend" requirements = backend / "requirements" From 984cafc1ea71d2994b1817c4d19a400ffd69c3ca Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 18:02:06 +0000 Subject: [PATCH 12/16] Close the gaps three probes left open The owner check and the unlink were separate steps, so a start landing between them still lost its metadata to the backend it replaced. Both now run under the same lock as the write, which is the only writer. Probe output is drained into a bounded tail rather than kept whole. The binary being probed is a possibly damaged install by construction, so an import stuck printing is exactly the input this path can meet, and the payload is the last JSON line either way. RECORD can outlive the files it lists: an interrupted replace deletes the package and leaves the metadata behind. The top-level names it records are stat'd now. 298 distributions in a working venv, none flagged, 44ms. --- studio/src-tauri/src/desktop_backend_owner.rs | 12 ++++++ studio/src-tauri/src/preflight.rs | 4 +- studio/src-tauri/src/preflight/managed.rs | 43 ++++++++++++------- unsloth_cli/commands/studio.py | 21 ++++++++- .../tests/test_studio_runtime_readiness.py | 32 ++++++++++++++ 5 files changed, 94 insertions(+), 18 deletions(-) diff --git a/studio/src-tauri/src/desktop_backend_owner.rs b/studio/src-tauri/src/desktop_backend_owner.rs index 5e0c0425e6..a0400eea5b 100644 --- a/studio/src-tauri/src/desktop_backend_owner.rs +++ b/studio/src-tauri/src/desktop_backend_owner.rs @@ -268,6 +268,9 @@ impl BackendOwnerState { /// Only while the file is still ours. Cleanup after an exit can take seconds, /// and a start in that window already wrote the next metadata to this path. pub(crate) fn remove(self) { + // Held across the check and the unlink: a start that lands between the + // two would otherwise have its file deleted by the backend it replaced. + let _guard = lock_metadata(); if let Ok(Some(on_disk)) = read_metadata(&self.path) { if on_disk.token_sha256 != self.metadata.token_sha256 { warn!( @@ -316,7 +319,16 @@ impl BackendOwnerState { } } +/// Serializes activation against cleanup. Both run in this process, and the +/// check in remove() only holds if no write can land inside it. +static METADATA_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + +fn lock_metadata() -> std::sync::MutexGuard<'static, ()> { + METADATA_LOCK.lock().unwrap_or_else(|e| e.into_inner()) +} + fn write_metadata(path: &Path, metadata: &DesktopBackendMetadata) -> Result<(), String> { + let _guard = lock_metadata(); let parent = path .parent() .ok_or_else(|| "desktop owner metadata path has no parent".to_string())?; diff --git a/studio/src-tauri/src/preflight.rs b/studio/src-tauri/src/preflight.rs index b9046eaa26..fe9e4e6fcd 100644 --- a/studio/src-tauri/src/preflight.rs +++ b/studio/src-tauri/src/preflight.rs @@ -712,6 +712,8 @@ sleep 120 async fn managed_runtime_probe_survives_more_stdout_than_the_pipe_holds() { // The probe imports the whole backend, so output can exceed the 64 KiB // pipe buffer, and waiting before draining wedges the child mid-write. + // This CLI writes ~1 MiB before the payload, so the drain both keeps up + // and keeps only the tail, which still has to contain the payload. let _cache_guard = MANAGED_CAPABILITY_CACHE_TEST_LOCK.lock().await; let _cache_home = ManagedCapabilityCacheHome::new("runtime-noisy"); remove_managed_capability_cache(); @@ -720,7 +722,7 @@ sleep 120 "runtime-noisy", r#"#!/bin/sh if [ "$1" = "studio" ] && [ "$2" = "desktop-runtime-check" ] && [ "$3" = "--json" ]; then - awk 'BEGIN { while (i++ < 4000) print "import chatter on stdout" }' + awk 'BEGIN { while (i++ < 40000) print "import chatter on stdout" }' printf '{"runtime_ready":true}\n' exit 0 fi diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index ba38523688..a3a67189b5 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -303,6 +303,30 @@ async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { } } +/// Room for the payload and what preceded it. Bounded because the binary being +/// probed is a possibly damaged install, and keeping every byte lets an import +/// stuck printing grow this buffer for the whole timeout. +const PROBE_TAIL_BYTES: usize = 64 * 1024; + +/// Drained while the wait runs: the backend import can print more than the pipe +/// holds, and waiting first deadlocks, timing out a healthy install into repair. +fn drain_probe_tail(mut stdout: tokio::process::ChildStdout) -> tokio::task::JoinHandle> { + tokio::spawn(async move { + let mut tail = Vec::new(); + let mut chunk = [0u8; 8192]; + while let Ok(read) = stdout.read(&mut chunk).await { + if read == 0 { + break; + } + tail.extend_from_slice(&chunk[..read]); + if tail.len() > PROBE_TAIL_BYTES { + tail.drain(..tail.len() - PROBE_TAIL_BYTES); + } + } + tail + }) +} + /// The probe could not answer: it would not run, or exited without a payload. /// Distinct from a CLI that is simply too old to know the command. const RUNTIME_PROBE_FAILED: &str = "studio_runtime_probe_failed"; @@ -353,17 +377,11 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { ); return Err(RUNTIME_PROBE_FAILED.to_string()); }; - let Some(mut stdout) = child.stdout.take() else { + let Some(stdout) = child.stdout.take() else { return Err(RUNTIME_PROBE_FAILED.to_string()); }; - // Drain while waiting: the backend import can print more than the pipe holds, - // and waiting first deadlocks, timing out a healthy install into repair. - let reader = tokio::spawn(async move { - let mut buffer = Vec::new(); - let _ = stdout.read_to_end(&mut buffer).await; - buffer - }); + let reader = drain_probe_tail(stdout); let status = match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { Ok(Ok(status)) => status, @@ -445,16 +463,11 @@ async fn probe_cli_capability(bin: &Path) -> Option { ); return None; }; - let Some(mut stdout) = child.stdout.take() else { + let Some(stdout) = child.stdout.take() else { return None; }; - // Same reason as the runtime probe above. - let reader = tokio::spawn(async move { - let mut buffer = Vec::new(); - let _ = stdout.read_to_end(&mut buffer).await; - buffer - }); + let reader = drain_probe_tail(stdout); match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { Ok(Ok(status)) if status.success() => {} diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 43c6634865..285689b894 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -18,7 +18,7 @@ import types import urllib.error import urllib.request from datetime import datetime, timezone -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import List, Optional import typer @@ -297,6 +297,23 @@ def _studio_requirement_roots(run_mod): return roots +def _recorded_packages_missing(installed) -> bool: + """Whether RECORD lists top-level packages the venv no longer has. + + An interrupted replace removes the package and can leave its RECORD, so + reading one is not proof the modules are there. Stats the top-level names + only: a handful per distribution, and the failure is never partial. + """ + packages = set() + for recorded in installed.files or (): + top = PurePosixPath(str(recorded)).parts[0] + # .dist-info is the metadata itself; .data and ../ land outside the tree. + if top == ".." or top.endswith((".dist-info", ".data")): + continue + packages.add(top) + return any(not installed.locate_file(name).exists() for name in packages) + + def _missing_studio_requirement(run_mod): """First requirement studio.txt needs that this venv cannot supply. @@ -318,7 +335,7 @@ def _missing_studio_requirement(run_mod): return requirement.name # Metadata outlives the package: wheels store METADATA first, so a killed # unpack leaves a version behind. RECORD is last, so its absence marks it. - if installed.files is None: + if installed.files is None or _recorded_packages_missing(installed): return requirement.name # Only studio.txt pins are enforced: install_python_stack uses --no-deps, # so transitive bounds read unsatisfied in venvs that work. A prerelease diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 454638258b..ed14a1e8f7 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -252,3 +252,35 @@ def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first(monkeypatch studio.desktop_runtime_check(_json_output = True) assert json.loads(capsys.readouterr().out)["module"] == "huggingface-hub" + + +def test_a_record_without_its_package_is_reported_missing(monkeypatch, capsys, tmp_path): + """An interrupted replace can delete the package and leave its RECORD, so a + readable file list is not proof the modules are there.""" + studio = importlib.import_module("unsloth_cli.commands.studio") + backend = tmp_path / "backend" + requirements = backend / "requirements" + requirements.mkdir(parents = True) + (requirements / "studio.txt").write_text("structlog\n", encoding = "utf-8") + run_mod = SimpleNamespace(__file__ = str(backend / "run.py")) + monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) + + site_packages = tmp_path / "site-packages" + (site_packages / "structlog-25.1.0.dist-info").mkdir(parents = True) + installed = SimpleNamespace( + version = "25.1.0", + requires = None, + files = ["structlog/__init__.py", "structlog-25.1.0.dist-info/RECORD"], + locate_file = lambda name: site_packages / name, + ) + monkeypatch.setattr( + importlib.import_module("importlib.metadata"), "distribution", lambda _name: installed, + ) + + with pytest.raises(typer.Exit): + studio.desktop_runtime_check(_json_output = True) + assert json.loads(capsys.readouterr().out)["module"] == "structlog" + + (site_packages / "structlog").mkdir() + studio.desktop_runtime_check(_json_output = True) + assert json.loads(capsys.readouterr().out) == {"runtime_ready": True} From 6ee7985e85bc78dd9fb2a18c08e36e627eaba881 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:09:32 +0000 Subject: [PATCH 13/16] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- unsloth_cli/tests/test_studio_runtime_readiness.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index ed14a1e8f7..28137b03dd 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -274,7 +274,9 @@ def test_a_record_without_its_package_is_reported_missing(monkeypatch, capsys, t locate_file = lambda name: site_packages / name, ) monkeypatch.setattr( - importlib.import_module("importlib.metadata"), "distribution", lambda _name: installed, + importlib.import_module("importlib.metadata"), + "distribution", + lambda _name: installed, ) with pytest.raises(typer.Exit): From c1e13d41c86be4dcadfc4b1b988ab23983753451 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 18:35:15 +0000 Subject: [PATCH 14/16] Bound the drain, stat the files, and check the generation A descendant that inherits stdout keeps the pipe open after the CLI leader exits, so the drain sat outside the timeout and could wedge preflight for good. Both probes now share one deadline across the wait and the drain. Stat every file RECORD lists, not just the top-level names. An interrupted replace can recreate a package directory and stop part-way through filling it, which the directory check read as complete. 6656 files across studio.txt's closure cost 158ms, and __pycache__ is skipped: deleting it is not damage. Cleanup after an exit can take seconds, so a start in that window already owns the backend by the time the crash event fires. The frontend sets "Server stopped unexpectedly" on it unconditionally, which showed a healthy replacement as failed. --- studio/src-tauri/src/preflight/managed.rs | 31 ++++++++++++++----- studio/src-tauri/src/process.rs | 26 +++++++++++++++- unsloth_cli/commands/studio.py | 25 ++++++++------- .../tests/test_studio_runtime_readiness.py | 20 +++++++++--- 4 files changed, 79 insertions(+), 23 deletions(-) diff --git a/studio/src-tauri/src/preflight/managed.rs b/studio/src-tauri/src/preflight/managed.rs index a3a67189b5..ed25e61523 100644 --- a/studio/src-tauri/src/preflight/managed.rs +++ b/studio/src-tauri/src/preflight/managed.rs @@ -308,6 +308,11 @@ async fn run_cli_probe(bin: &Path, args: &[&str]) -> bool { /// stuck printing grow this buffer for the whole timeout. const PROBE_TAIL_BYTES: usize = 64 * 1024; +/// Budget for a whole probe, the drain included. A descendant that inherited +/// stdout keeps the pipe open after the leader exits, so a drain outside the +/// timeout would wedge preflight for good. +const PROBE_TIMEOUT: Duration = Duration::from_secs(10); + /// Drained while the wait runs: the backend import can print more than the pipe /// holds, and waiting first deadlocks, timing out a healthy install into repair. fn drain_probe_tail(mut stdout: tokio::process::ChildStdout) -> tokio::task::JoinHandle> { @@ -381,9 +386,10 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { return Err(RUNTIME_PROBE_FAILED.to_string()); }; - let reader = drain_probe_tail(stdout); + let mut reader = drain_probe_tail(stdout); + let deadline = tokio::time::Instant::now() + PROBE_TIMEOUT; - let status = match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { + let status = match tokio::time::timeout_at(deadline, child.wait()).await { Ok(Ok(status)) => status, _ => { let _ = child.kill().await; @@ -397,8 +403,17 @@ async fn probe_cli_runtime(bin: &Path) -> Result<(), String> { } }; - let Ok(output) = reader.await else { - return Err(RUNTIME_PROBE_FAILED.to_string()); + let output = match tokio::time::timeout_at(deadline, &mut reader).await { + Ok(Ok(output)) => output, + Ok(Err(_)) => return Err(RUNTIME_PROBE_FAILED.to_string()), + Err(_) => { + reader.abort(); + info!( + "Managed runtime probe left stdout open in {}ms", + started.elapsed().as_millis() + ); + return Err(RUNTIME_PROBE_TIMEOUT.to_string()); + } }; let payload = String::from_utf8_lossy(&output) .lines() @@ -467,9 +482,10 @@ async fn probe_cli_capability(bin: &Path) -> Option { return None; }; - let reader = drain_probe_tail(stdout); + let mut reader = drain_probe_tail(stdout); + let deadline = tokio::time::Instant::now() + PROBE_TIMEOUT; - match tokio::time::timeout(Duration::from_secs(10), child.wait()).await { + match tokio::time::timeout_at(deadline, child.wait()).await { Ok(Ok(status)) if status.success() => {} Err(_) => { let _ = child.kill().await; @@ -491,7 +507,8 @@ async fn probe_cli_capability(bin: &Path) -> Option { } } - let Ok(output) = reader.await else { + let Ok(Ok(output)) = tokio::time::timeout_at(deadline, &mut reader).await else { + reader.abort(); return None; }; diff --git a/studio/src-tauri/src/process.rs b/studio/src-tauri/src/process.rs index b7e0a05ee8..7311452856 100644 --- a/studio/src-tauri/src/process.rs +++ b/studio/src-tauri/src/process.rs @@ -419,6 +419,20 @@ fn poll_spawned_backend_exit(state: &BackendState, generation: u64) -> BackendEx }) } +fn current_backend_generation(state: &BackendState) -> u64 { + match state.lock() { + Ok(guard) => guard.generation, + Err(poisoned) => poisoned.into_inner().generation, + } +} + +/// Cleanup can take seconds terminating descendants, and a start in that window +/// already owns the backend. The frontend sets "Server stopped unexpectedly" on +/// this event unconditionally, so a healthy replacement would be shown as failed. +fn exit_is_reportable(intentional: bool, exited: u64, current: u64) -> bool { + !intentional && exited == current +} + fn monitor_spawned_backend_exit( app: AppHandle, state: BackendState, @@ -451,9 +465,12 @@ fn monitor_spawned_backend_exit( exit.intentional, None, ); - if !exit.intentional { + let current = current_backend_generation(&state); + if exit_is_reportable(exit.intentional, generation, current) { error!("Backend process exited unexpectedly (crash detected)"); let _ = app.emit("server-crashed", ()); + } else if generation != current { + info!("Backend {} was replaced during cleanup", generation); } return; } @@ -534,6 +551,13 @@ pub fn find_unsloth_binary() -> Option { #[cfg(test)] mod tests { use super::*; + + #[test] + fn a_backend_replaced_during_cleanup_is_not_reported_as_a_crash() { + assert!(exit_is_reportable(false, 7, 7)); + assert!(!exit_is_reportable(false, 7, 8)); + assert!(!exit_is_reportable(true, 7, 7)); + } use std::fs; use std::io::{Read, Write}; use std::net::TcpListener; diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 05e191ef6f..a933235d6a 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -297,21 +297,24 @@ def _studio_requirement_roots(run_mod): return roots -def _recorded_packages_missing(installed) -> bool: - """Whether RECORD lists top-level packages the venv no longer has. +def _recorded_files_missing(installed) -> bool: + """Whether RECORD lists files the venv no longer has. - An interrupted replace removes the package and can leave its RECORD, so - reading one is not proof the modules are there. Stats the top-level names - only: a handful per distribution, and the failure is never partial. + An interrupted replace can recreate a package directory and stop part-way + through filling it, so the directory existing proves nothing. 6656 files + across studio.txt's closure cost 158ms, against a probe that imports the + backend, and __pycache__ is skipped because deleting it is not damage. """ - packages = set() for recorded in installed.files or (): - top = PurePosixPath(str(recorded)).parts[0] + parts = PurePosixPath(str(recorded)).parts # .dist-info is the metadata itself; .data and ../ land outside the tree. - if top == ".." or top.endswith((".dist-info", ".data")): + if parts[0] == ".." or parts[0].endswith((".dist-info", ".data")): continue - packages.add(top) - return any(not installed.locate_file(name).exists() for name in packages) + if "__pycache__" in parts or parts[-1].endswith(".pyc"): + continue + if not installed.locate_file(recorded).exists(): + return True + return False def _missing_studio_requirement(run_mod): @@ -335,7 +338,7 @@ def _missing_studio_requirement(run_mod): return requirement.name # Metadata outlives the package: wheels store METADATA first, so a killed # unpack leaves a version behind. RECORD is last, so its absence marks it. - if installed.files is None or _recorded_packages_missing(installed): + if installed.files is None or _recorded_files_missing(installed): return requirement.name # Only studio.txt pins are enforced: install_python_stack uses --no-deps, # so transitive bounds read unsatisfied in venvs that work. A prerelease diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 28137b03dd..24a687a4c2 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -254,9 +254,9 @@ def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first(monkeypatch assert json.loads(capsys.readouterr().out)["module"] == "huggingface-hub" -def test_a_record_without_its_package_is_reported_missing(monkeypatch, capsys, tmp_path): - """An interrupted replace can delete the package and leave its RECORD, so a - readable file list is not proof the modules are there.""" +def test_a_record_without_its_files_is_reported_missing(monkeypatch, capsys, tmp_path): + """An interrupted replace can recreate the package directory and stop + part-way through filling it, so the directory existing proves nothing.""" studio = importlib.import_module("unsloth_cli.commands.studio") backend = tmp_path / "backend" requirements = backend / "requirements" @@ -270,7 +270,12 @@ def test_a_record_without_its_package_is_reported_missing(monkeypatch, capsys, t installed = SimpleNamespace( version = "25.1.0", requires = None, - files = ["structlog/__init__.py", "structlog-25.1.0.dist-info/RECORD"], + files = [ + "structlog/__init__.py", + "structlog/processors.py", + "structlog/__pycache__/__init__.cpython-311.pyc", + "structlog-25.1.0.dist-info/RECORD", + ], locate_file = lambda name: site_packages / name, ) monkeypatch.setattr( @@ -283,6 +288,13 @@ def test_a_record_without_its_package_is_reported_missing(monkeypatch, capsys, t studio.desktop_runtime_check(_json_output = True) assert json.loads(capsys.readouterr().out)["module"] == "structlog" + # The directory back but still a file short is the interrupted replace. (site_packages / "structlog").mkdir() + (site_packages / "structlog" / "__init__.py").touch() + with pytest.raises(typer.Exit): + studio.desktop_runtime_check(_json_output = True) + assert json.loads(capsys.readouterr().out)["module"] == "structlog" + + (site_packages / "structlog" / "processors.py").touch() studio.desktop_runtime_check(_json_output = True) assert json.loads(capsys.readouterr().out) == {"runtime_ready": True} From 7e2292f2e46ddfc587c680c8bfe4e8839cc550f3 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Mon, 27 Jul 2026 19:05:48 +0000 Subject: [PATCH 15/16] Read RECORD directly, and only clear a marker this attempt created Distribution.files drops entries whose paths no longer exist, which is precisely the set the last change looked for, so on 3.13 (what both desktop installers pick) the check could never fire. Verified against a dist-info whose RECORD names a deleted file: files returned 3 of 4 entries and the damaged package read as complete. RECORD is parsed directly now, and the test builds a real dist-info rather than a stub so it would have caught this. The marker is also not always ours. A retry after an interrupted install finds the earlier one still there, and a repair that then fails to spawn was deleting it, losing the classification the next launch depends on. Creation reports whether it created the file; the pre-spawn and elevation paths clear only then, and success still clears unconditionally. --- studio/src-tauri/src/install.rs | 42 ++++++++++++---- unsloth_cli/commands/studio.py | 20 +++++--- .../tests/test_studio_runtime_readiness.py | 50 +++++++++++-------- 3 files changed, 75 insertions(+), 37 deletions(-) diff --git a/studio/src-tauri/src/install.rs b/studio/src-tauri/src/install.rs index 74e560a921..bdefca22a1 100644 --- a/studio/src-tauri/src/install.rs +++ b/studio/src-tauri/src/install.rs @@ -54,12 +54,29 @@ pub(crate) fn managed_install_in_progress() -> bool { .unwrap_or(false) } +/// Whether this process created the marker rather than finding one an earlier +/// interrupted install left. Clearing someone else's drops the only signal that +/// its venv is half-written. Process-wide because the marker is one file. +static MARKER_CREATED_HERE: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + fn create_install_in_progress_marker() -> Result<(), String> { let path = install_in_progress_marker_path()?; - create_install_in_progress_marker_at(&path) + let created = create_install_in_progress_marker_at(&path)?; + MARKER_CREATED_HERE.store(created, std::sync::atomic::Ordering::Relaxed); + Ok(()) } -fn create_install_in_progress_marker_at(path: &Path) -> Result<(), String> { +/// Clear only a marker this attempt created. Used where the venv was never +/// touched: a failed spawn, and the elevation exits. +fn clear_own_install_marker() { + if MARKER_CREATED_HERE.load(std::sync::atomic::Ordering::Relaxed) { + clear_install_marker_best_effort(); + } +} + +/// Ok(true) when this call created the file, Ok(false) when one was there. +fn create_install_in_progress_marker_at(path: &Path) -> Result { let parent = path .parent() .ok_or_else(|| format!("Invalid install marker path: {}", path.display()))?; @@ -71,8 +88,8 @@ fn create_install_in_progress_marker_at(path: &Path) -> Result<(), String> { .create_new(true) .open(path) { - Ok(_) => Ok(()), - Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => Ok(()), + Ok(_) => Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => Ok(false), Err(error) => Err(format!("Failed to create {}: {}", path.display(), error)), } } @@ -90,6 +107,7 @@ fn clear_install_marker_best_effort() { if let Err(msg) = clear_install_in_progress_marker() { warn!("[install] {}", msg); } + MARKER_CREATED_HERE.store(false, std::sync::atomic::Ordering::Relaxed); } fn clear_install_in_progress_marker_at(path: &Path) -> Result<(), String> { @@ -516,10 +534,11 @@ fn run_install_with_event_mode( let (stdout, stderr) = match spawn_script(&script, &args, &state) { Ok(handles) => handles, Err(msg) => { - // Nothing ran, so nothing is half-installed. Unless another installer - // owns the marker, where clearing it drops its recovery signal. + // Nothing ran, so nothing is half-installed. Unless the marker is + // not ours: another installer owns it, or an earlier interrupted + // one left it, and either way its signal is not ours to drop. if msg != INSTALL_ALREADY_RUNNING { - clear_install_marker_best_effort(); + clear_own_install_marker(); } diagnostics::finish_attempt( &diagnostics, @@ -648,7 +667,7 @@ pub fn record_pending_elevation_canceled( return false; }; // The resumed run the elevation exit left the marker for is not happening. - clear_install_marker_best_effort(); + clear_own_install_marker(); diagnostics::finish_attempt( diagnostics, &attempt, @@ -922,7 +941,7 @@ fn finish_elevation_failure( message: String, ) { // Terminal, like a cancelled prompt: the resumed run is not happening. - clear_install_marker_best_effort(); + clear_own_install_marker(); if let Some(attempt) = attempt { diagnostics::finish_attempt( diagnostics, @@ -971,10 +990,11 @@ mod tests { )); let marker = directory.join(INSTALL_IN_PROGRESS_MARKER); - create_install_in_progress_marker_at(&marker).unwrap(); + assert!(create_install_in_progress_marker_at(&marker).unwrap()); assert!(marker.is_file()); - create_install_in_progress_marker_at(&marker).unwrap(); + // A second attempt finds the first one's marker and does not own it. + assert!(!create_install_in_progress_marker_at(&marker).unwrap()); assert!(marker.is_file()); clear_install_in_progress_marker_at(&marker).unwrap(); diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index a933235d6a..f06da08d90 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -1,6 +1,7 @@ # SPDX-License-Identifier: AGPL-3.0-only # Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 +import csv import importlib.util import hashlib import hmac @@ -301,18 +302,25 @@ def _recorded_files_missing(installed) -> bool: """Whether RECORD lists files the venv no longer has. An interrupted replace can recreate a package directory and stop part-way - through filling it, so the directory existing proves nothing. 6656 files - across studio.txt's closure cost 158ms, against a probe that imports the - backend, and __pycache__ is skipped because deleting it is not damage. + through filling it, so the directory existing proves nothing. RECORD is read + directly because Distribution.files drops entries that no longer exist, + which is exactly the set this looks for. 6656 files across studio.txt's + closure cost 158ms, and __pycache__ is skipped: deleting it is not damage. """ - for recorded in installed.files or (): - parts = PurePosixPath(str(recorded)).parts + # egg-info has no RECORD, and its file lists say nothing about completeness. + record = installed.read_text("RECORD") + if record is None: + return False + for row in csv.reader(record.splitlines()): + if not row or not row[0]: + continue + parts = PurePosixPath(row[0]).parts # .dist-info is the metadata itself; .data and ../ land outside the tree. if parts[0] == ".." or parts[0].endswith((".dist-info", ".data")): continue if "__pycache__" in parts or parts[-1].endswith(".pyc"): continue - if not installed.locate_file(recorded).exists(): + if not installed.locate_file(row[0]).exists(): return True return False diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 24a687a4c2..15fff78b11 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -93,7 +93,9 @@ def test_desktop_runtime_check_accepts_a_prerelease_over_a_floor(monkeypatch, ca monkeypatch.setattr( importlib.import_module("importlib.metadata"), "distribution", - lambda _name: SimpleNamespace(version = "2.0.0b1", files = [], requires = None), + lambda _name: SimpleNamespace( + version = "2.0.0b1", files = [], requires = None, read_text = lambda _n: None, + ), ) studio.desktop_runtime_check(_json_output = True) @@ -112,7 +114,9 @@ def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp monkeypatch.setattr( importlib.import_module("importlib.metadata"), "distribution", - lambda _name: SimpleNamespace(version = "1.0", files = [], requires = None), + lambda _name: SimpleNamespace( + version = "1.0", files = [], requires = None, read_text = lambda _n: None, + ), ) with pytest.raises(typer.Exit): @@ -137,7 +141,9 @@ def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package( monkeypatch.setattr( importlib.import_module("importlib.metadata"), "distribution", - lambda _name: SimpleNamespace(version = "0.140.5", files = None, requires = None), + lambda _name: SimpleNamespace( + version = "0.140.5", files = None, requires = None, read_text = lambda _n: None, + ), ) with pytest.raises(typer.Exit): @@ -156,7 +162,9 @@ def _fake_distributions(monkeypatch, installed): version, requires = installed[name] except KeyError: raise metadata.PackageNotFoundError(name) from None - return SimpleNamespace(version = version, files = [], requires = requires) + return SimpleNamespace( + version = version, files = [], requires = requires, read_text = lambda _n: None, + ) monkeypatch.setattr(metadata, "distribution", _distribution) @@ -256,8 +264,12 @@ def test_a_root_pin_is_checked_even_when_a_dependency_names_it_first(monkeypatch def test_a_record_without_its_files_is_reported_missing(monkeypatch, capsys, tmp_path): """An interrupted replace can recreate the package directory and stop - part-way through filling it, so the directory existing proves nothing.""" + part-way through filling it, so the directory existing proves nothing. + + Built as a real dist-info rather than a stub: Distribution.files drops + entries that no longer exist, which is the whole set this looks for.""" studio = importlib.import_module("unsloth_cli.commands.studio") + metadata = importlib.import_module("importlib.metadata") backend = tmp_path / "backend" requirements = backend / "requirements" requirements.mkdir(parents = True) @@ -266,23 +278,20 @@ def test_a_record_without_its_files_is_reported_missing(monkeypatch, capsys, tmp monkeypatch.setattr(studio, "_load_run_module", lambda: run_mod) site_packages = tmp_path / "site-packages" - (site_packages / "structlog-25.1.0.dist-info").mkdir(parents = True) - installed = SimpleNamespace( - version = "25.1.0", - requires = None, - files = [ - "structlog/__init__.py", - "structlog/processors.py", - "structlog/__pycache__/__init__.cpython-311.pyc", - "structlog-25.1.0.dist-info/RECORD", - ], - locate_file = lambda name: site_packages / name, + dist_info = site_packages / "structlog-25.1.0.dist-info" + dist_info.mkdir(parents = True) + (dist_info / "METADATA").write_text( + "Metadata-Version: 2.1\nName: structlog\nVersion: 25.1.0\n", encoding = "utf-8", ) - monkeypatch.setattr( - importlib.import_module("importlib.metadata"), - "distribution", - lambda _name: installed, + (dist_info / "RECORD").write_text( + "structlog/__init__.py,,\n" + "structlog/processors.py,,\n" + "structlog/__pycache__/__init__.cpython-313.pyc,,\n" + "structlog-25.1.0.dist-info/RECORD,,\n", + encoding = "utf-8", ) + installed = next(iter(metadata.distributions(path = [str(site_packages)]))) + monkeypatch.setattr(metadata, "distribution", lambda _name: installed) with pytest.raises(typer.Exit): studio.desktop_runtime_check(_json_output = True) @@ -295,6 +304,7 @@ def test_a_record_without_its_files_is_reported_missing(monkeypatch, capsys, tmp studio.desktop_runtime_check(_json_output = True) assert json.loads(capsys.readouterr().out)["module"] == "structlog" + # Complete, and the never-written .pyc must not count as damage. (site_packages / "structlog" / "processors.py").touch() studio.desktop_runtime_check(_json_output = True) assert json.loads(capsys.readouterr().out) == {"runtime_ready": True} From c6ecac97b9cc8c69147a704f814e5c4d7b95bd31 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:06:29 +0000 Subject: [PATCH 16/16] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../tests/test_studio_runtime_readiness.py | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/unsloth_cli/tests/test_studio_runtime_readiness.py b/unsloth_cli/tests/test_studio_runtime_readiness.py index 15fff78b11..f5f4ab23ff 100644 --- a/unsloth_cli/tests/test_studio_runtime_readiness.py +++ b/unsloth_cli/tests/test_studio_runtime_readiness.py @@ -94,7 +94,10 @@ def test_desktop_runtime_check_accepts_a_prerelease_over_a_floor(monkeypatch, ca importlib.import_module("importlib.metadata"), "distribution", lambda _name: SimpleNamespace( - version = "2.0.0b1", files = [], requires = None, read_text = lambda _n: None, + version = "2.0.0b1", + files = [], + requires = None, + read_text = lambda _n: None, ), ) @@ -115,7 +118,10 @@ def test_desktop_runtime_check_rejects_version_mismatch(monkeypatch, capsys, tmp importlib.import_module("importlib.metadata"), "distribution", lambda _name: SimpleNamespace( - version = "1.0", files = [], requires = None, read_text = lambda _n: None, + version = "1.0", + files = [], + requires = None, + read_text = lambda _n: None, ), ) @@ -142,7 +148,10 @@ def test_desktop_runtime_check_rejects_metadata_without_an_unpacked_package( importlib.import_module("importlib.metadata"), "distribution", lambda _name: SimpleNamespace( - version = "0.140.5", files = None, requires = None, read_text = lambda _n: None, + version = "0.140.5", + files = None, + requires = None, + read_text = lambda _n: None, ), ) @@ -163,7 +172,10 @@ def _fake_distributions(monkeypatch, installed): except KeyError: raise metadata.PackageNotFoundError(name) from None return SimpleNamespace( - version = version, files = [], requires = requires, read_text = lambda _n: None, + version = version, + files = [], + requires = requires, + read_text = lambda _n: None, ) monkeypatch.setattr(metadata, "distribution", _distribution) @@ -281,7 +293,8 @@ def test_a_record_without_its_files_is_reported_missing(monkeypatch, capsys, tmp dist_info = site_packages / "structlog-25.1.0.dist-info" dist_info.mkdir(parents = True) (dist_info / "METADATA").write_text( - "Metadata-Version: 2.1\nName: structlog\nVersion: 25.1.0\n", encoding = "utf-8", + "Metadata-Version: 2.1\nName: structlog\nVersion: 25.1.0\n", + encoding = "utf-8", ) (dist_info / "RECORD").write_text( "structlog/__init__.py,,\n"