mirror of
https://github.com/open-jarvis/OpenJarvis.git
synced 2026-07-27 21:05:34 +00:00
Two runtime bugs found during end-to-end testing on a clean Windows 11 24H2 Azure VM. #531 - Desktop "Failed to get response": run_jarvis_command spawned the backend with .output(), which waits for the process to exit. `jarvis serve` never exits, so the Tauri command hung forever (the Start button never resolved); and it ran `uv run jarvis` with no cwd, so in a packaged install -- where the cwd isn't the checkout -- `jarvis` wasn't found and the server never started. Now: run from find_project_root(), and for `serve` spawn detached (.spawn()), drain stderr, and poll /health for readiness (mirrors start_backend); short commands keep .output(). The server layer itself was verified healthy on Windows (/health and /v1/chat/completions both 200, localhost included) -- the fault was the Tauri spawn path. #532 - "OpenAI client not available" after reboot: when the local engine is down, get_engine's fallback selected CloudEngine because health() is True if ANY provider client exists -- without checking the resolved model's provider has a client. A user with e.g. OPENROUTER_API_KEY and a gpt-* model then hit the OpenAI path with no client. Add CloudEngine.can_serve(model) (checks the specific provider client via the same routing generate()/stream() use) + a default can_serve->True on the base engine, and make get_engine model-aware so it skips an engine that can't serve the model -- the user falls through to the helpful "no engine available / start ollama" message instead. Tests: engine discovery/cloud/model-matrix + cli serve/ask suites pass (the one ask_e2e failure is a pre-existing version-banner flake, fails identically on main). The Tauri crate couldn't be compiled locally (no GTK/webkit sys-libs in this env); relies on CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
d7053c35d5
commit
55fa503987
@@ -1540,19 +1540,89 @@ async fn fetch_models(api_url: String) -> Result<serde_json::Value, String> {
|
||||
|
||||
#[tauri::command]
|
||||
async fn run_jarvis_command(args: Vec<String>) -> Result<String, String> {
|
||||
let mut cmd_args = vec!["run".to_string(), "jarvis".to_string()];
|
||||
cmd_args.extend(args);
|
||||
let uv_bin = resolve_bin("uv");
|
||||
let output = tokio::process::Command::new(&uv_bin)
|
||||
.args(&cmd_args)
|
||||
.output()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to launch jarvis: {}", e))?;
|
||||
|
||||
if output.status.success() {
|
||||
Ok(String::from_utf8_lossy(&output.stdout).to_string())
|
||||
} else {
|
||||
Err(String::from_utf8_lossy(&output.stderr).to_string())
|
||||
let mut cmd_args = vec!["run".to_string(), "jarvis".to_string()];
|
||||
cmd_args.extend(args.iter().cloned());
|
||||
|
||||
let mut cmd = tokio::process::Command::new(&uv_bin);
|
||||
cmd.args(&cmd_args);
|
||||
// Run from the project root so `uv run jarvis` resolves the OpenJarvis
|
||||
// project regardless of the app's launch cwd. In a packaged install the
|
||||
// cwd isn't the checkout, so without this `jarvis` isn't found and the
|
||||
// backend never starts — the UI then shows "Failed to get response"
|
||||
// (see #531).
|
||||
if let Some(ref root) = find_project_root() {
|
||||
cmd.current_dir(root);
|
||||
}
|
||||
|
||||
let is_serve = args.first().map(|a| a.as_str() == "serve").unwrap_or(false);
|
||||
|
||||
if !is_serve {
|
||||
// Short-lived command (e.g. `stop`, `status`): wait for it and return
|
||||
// its captured output.
|
||||
let output = cmd
|
||||
.output()
|
||||
.await
|
||||
.map_err(|e| format!("Failed to launch jarvis: {}", e))?;
|
||||
return if output.status.success() {
|
||||
Ok(String::from_utf8_lossy(&output.stdout).to_string())
|
||||
} else {
|
||||
Err(String::from_utf8_lossy(&output.stderr).to_string())
|
||||
};
|
||||
}
|
||||
|
||||
// `jarvis serve` is a long-running server that never exits. The old code
|
||||
// used `.output()`, which waits for the process to exit and so hung this
|
||||
// command forever — the "Start" button never resolved (#531). Spawn it
|
||||
// detached instead, drain stderr (a full 4 KB Windows pipe can otherwise
|
||||
// stall the child mid-startup, #309), and poll /health for readiness.
|
||||
cmd.stdout(std::process::Stdio::null())
|
||||
.stderr(std::process::Stdio::piped());
|
||||
let mut child = cmd
|
||||
.spawn()
|
||||
.map_err(|e| format!("Failed to launch jarvis serve: {}", e))?;
|
||||
|
||||
let tail: StderrTail = Arc::new(Mutex::new(Vec::new()));
|
||||
if let Some(stderr) = child.stderr.take() {
|
||||
spawn_jarvis_stderr_drainer(stderr, tail.clone());
|
||||
}
|
||||
|
||||
let client = reqwest::Client::builder()
|
||||
.timeout(Duration::from_secs(2))
|
||||
.build()
|
||||
.map_err(|e| format!("Failed to build HTTP client: {}", e))?;
|
||||
let url = format!("http://127.0.0.1:{}/health", JARVIS_PORT);
|
||||
let deadline = tokio::time::Instant::now() + Duration::from_secs(120);
|
||||
|
||||
loop {
|
||||
// Surface an early crash (bad venv, missing Rust ext, etc.) right away
|
||||
// instead of waiting out the full readiness timeout.
|
||||
if let Ok(Some(status)) = child.try_wait() {
|
||||
let stderr = String::from_utf8_lossy(tail.lock().await.as_slice()).into_owned();
|
||||
return Err(format!(
|
||||
"jarvis serve exited (code {:?}) before becoming healthy:\n{}",
|
||||
status.code(),
|
||||
stderr.trim()
|
||||
));
|
||||
}
|
||||
if let Ok(resp) = client.get(&url).send().await {
|
||||
if resp.status().is_success() {
|
||||
// Leave the server running (the Child is detached on drop —
|
||||
// kill_on_drop defaults to false); `stop` tears it down.
|
||||
return Ok(format!(
|
||||
"jarvis serve is ready on http://127.0.0.1:{}",
|
||||
JARVIS_PORT
|
||||
));
|
||||
}
|
||||
}
|
||||
if tokio::time::Instant::now() >= deadline {
|
||||
return Err(format!(
|
||||
"jarvis serve did not become healthy on port {} within 120s.",
|
||||
JARVIS_PORT
|
||||
));
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(500)).await;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -714,7 +714,13 @@ def ask(
|
||||
register_builtin_models()
|
||||
|
||||
effective_engine_key = engine_key or config.intelligence.preferred_engine or None
|
||||
resolved = get_engine(config, effective_engine_key)
|
||||
# Pass the model we intend to run so engine selection can skip an engine
|
||||
# that can't actually serve it (e.g. the cloud fallback when the local
|
||||
# engine is down but only a non-OpenAI key is set — see #532). This is the
|
||||
# -m flag or the configured default; when neither is set we leave it None
|
||||
# and a model is chosen per-engine below.
|
||||
selection_model = model_name or config.intelligence.default_model or None
|
||||
resolved = get_engine(config, effective_engine_key, model=selection_model)
|
||||
if resolved is None:
|
||||
console.print(
|
||||
"[red bold]No inference engine available.[/red bold]\n\n"
|
||||
|
||||
@@ -146,7 +146,13 @@ def serve(
|
||||
except Exception as exc:
|
||||
logger.debug("Telemetry store init failed: %s", exc)
|
||||
|
||||
resolved = get_engine(config, engine_key)
|
||||
# Select with the model we'll actually serve so an engine that can't
|
||||
# serve it (e.g. the cloud fallback without the matching provider key) is
|
||||
# skipped rather than chosen and failing per-request later (see #532).
|
||||
selection_model = (
|
||||
model_name or config.server.model or config.intelligence.default_model or None
|
||||
)
|
||||
resolved = get_engine(config, engine_key, model=selection_model)
|
||||
if resolved is None:
|
||||
console.print(
|
||||
"[red bold]No inference engine available.[/red bold]\n\n"
|
||||
|
||||
@@ -156,12 +156,26 @@ def discover_models(
|
||||
|
||||
|
||||
def get_engine(
|
||||
config: JarvisConfig, engine_key: str | None = None
|
||||
config: JarvisConfig,
|
||||
engine_key: str | None = None,
|
||||
model: str | None = None,
|
||||
) -> Tuple[str, InferenceEngine] | None:
|
||||
"""Get a specific engine by key, or the default with fallback.
|
||||
|
||||
When *model* is given, an engine is selected only if it can actually
|
||||
serve that model (``engine.can_serve(model)``). This stops the cloud
|
||||
fallback from being chosen — when the local engine is down — for a model
|
||||
whose provider client is missing, which otherwise surfaces as a confusing
|
||||
"OpenAI client not available" instead of a helpful "start your local
|
||||
engine" message (see #532). When *model* is ``None`` selection stays
|
||||
model-agnostic (unchanged behaviour).
|
||||
|
||||
Returns ``(key, engine_instance)`` or ``None`` if no engine is available.
|
||||
"""
|
||||
|
||||
def _usable(engine: InferenceEngine) -> bool:
|
||||
return engine.health() and (model is None or engine.can_serve(model))
|
||||
|
||||
# Build an ordered list of keys to try, then fall back to full discovery.
|
||||
keys_to_try: list[str] = []
|
||||
if engine_key:
|
||||
@@ -176,14 +190,16 @@ def get_engine(
|
||||
continue
|
||||
try:
|
||||
engine = _make_engine(key, config)
|
||||
if engine.health():
|
||||
if _usable(engine):
|
||||
return (key, engine)
|
||||
except Exception as exc:
|
||||
logger.debug("Engine %r health check failed: %s", key, exc)
|
||||
|
||||
# Fallback to any healthy engine
|
||||
healthy = discover_engines(config)
|
||||
return healthy[0] if healthy else None
|
||||
# Fallback to the first healthy engine that can serve the model.
|
||||
for key, engine in discover_engines(config):
|
||||
if model is None or engine.can_serve(model):
|
||||
return (key, engine)
|
||||
return None
|
||||
|
||||
|
||||
__all__ = ["discover_engines", "discover_models", "get_engine"]
|
||||
|
||||
@@ -119,6 +119,17 @@ class InferenceEngine(ABC):
|
||||
def health(self) -> bool:
|
||||
"""Return ``True`` when the engine is reachable and healthy."""
|
||||
|
||||
def can_serve(self, model: str) -> bool:
|
||||
"""Return ``True`` if this engine can serve *model*.
|
||||
|
||||
Defaults to ``True``: local engines accept any model id (whether a
|
||||
specific model is *installed* is a separate concern from engine
|
||||
selection). Engines that multiplex provider-specific clients (e.g.
|
||||
the cloud engine) override this so selection can skip an engine whose
|
||||
client for the model's provider isn't configured (see #532).
|
||||
"""
|
||||
return True
|
||||
|
||||
def close(self) -> None:
|
||||
"""Release resources (HTTP clients, connections, threads, etc.)."""
|
||||
|
||||
|
||||
@@ -1477,6 +1477,34 @@ class CloudEngine(InferenceEngine):
|
||||
models.extend(_CODEX_MODELS)
|
||||
return models
|
||||
|
||||
def _client_for_model(self, model: str) -> Any:
|
||||
"""Return the provider client ``generate``/``stream`` will dispatch to
|
||||
for *model* (mirrors the routing in those methods)."""
|
||||
if _is_codex_model(model):
|
||||
return self._codex_client
|
||||
if _is_openrouter_model(model):
|
||||
return self._openrouter_client
|
||||
if _is_minimax_model(model):
|
||||
return self._minimax_client
|
||||
if _is_anthropic_model(model):
|
||||
return self._anthropic_client
|
||||
if _is_google_model(model):
|
||||
return self._google_client
|
||||
return self._openai_client
|
||||
|
||||
def can_serve(self, model: str) -> bool:
|
||||
"""Return ``True`` only if the provider client for *model* exists.
|
||||
|
||||
``health()`` is ``True`` whenever *any* provider client is configured,
|
||||
but a request for, say, a ``gpt-*`` model still needs the OpenAI
|
||||
client specifically. Without this check the cloud engine gets picked
|
||||
as a fallback (when the local engine is down) for a model it can't
|
||||
serve, then dies at call time with "<provider> client not available"
|
||||
instead of the user getting a helpful "start your local engine"
|
||||
message (see #532).
|
||||
"""
|
||||
return self._client_for_model(model) is not None
|
||||
|
||||
def health(self) -> bool:
|
||||
return (
|
||||
self._openai_client is not None
|
||||
|
||||
Reference in New Issue
Block a user