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.
This commit is contained in:
parent
4482f8b4f1
commit
f749c90d87
4 changed files with 105 additions and 10 deletions
|
|
@ -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<String> {
|
||||
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"));
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
|
|
@ -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<String>> {
|
||||
match probe_managed_install().await {
|
||||
ManagedProbe::Ready { .. } => Ok(()),
|
||||
ManagedProbe::Stale { reason, .. } => Err(Some(reason)),
|
||||
_ => Err(None),
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue