From 16b6f24e3ea6f6bffaed84e190747af7b5135adc Mon Sep 17 00:00:00 2001 From: vikingowl Date: Fri, 24 Oct 2025 14:23:00 +0200 Subject: [PATCH] refactor(errors): surface typed provider failures AC:\n- Providers emit ProviderFailure with structured kind/detail for auth, rate-limit, timeout, and unavailable cases.\n- TUI maps ProviderFailure kinds to consistent toasts and fallbacks (no 429 string matching).\n- Cloud health checks detect unauthorized failures without relying on string parsing.\n\nTests:\n- cargo test -p owlen-core (fails: httpmock cannot bind 127.0.0.1 inside sandbox).\n- cargo test -p owlen-providers\n- cargo test -p owlen-tui --- crates/owlen-core/src/lib.rs | 3 + crates/owlen-core/src/provider/types.rs | 83 +++++++- crates/owlen-core/src/providers/ollama.rs | 83 +++++++- crates/owlen-providers/src/ollama/cloud.rs | 9 +- crates/owlen-providers/src/ollama/shared.rs | 139 ++++++++----- crates/owlen-tui/src/chat_app.rs | 204 +++++++++++++------- 6 files changed, 396 insertions(+), 125 deletions(-) diff --git a/crates/owlen-core/src/lib.rs b/crates/owlen-core/src/lib.rs index f15995f..cfec2bf 100644 --- a/crates/owlen-core/src/lib.rs +++ b/crates/owlen-core/src/lib.rs @@ -74,6 +74,9 @@ pub enum Error { #[error("Provider error: {0}")] Provider(#[from] anyhow::Error), + #[error("Provider failure: {0}")] + ProviderFailure(provider::ProviderError), + #[error("Network error: {0}")] Network(String), diff --git a/crates/owlen-core/src/provider/types.rs b/crates/owlen-core/src/provider/types.rs index a6f64ac..ba283b1 100644 --- a/crates/owlen-core/src/provider/types.rs +++ b/crates/owlen-core/src/provider/types.rs @@ -1,6 +1,6 @@ //! Shared types used by the unified provider abstraction layer. -use std::collections::HashMap; +use std::{collections::HashMap, fmt}; use serde::{Deserialize, Serialize}; use serde_json::Value; @@ -21,6 +21,87 @@ pub enum ProviderStatus { RequiresSetup, } +/// High-level categories for provider failures. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +pub enum ProviderErrorKind { + Unauthorized, + RateLimited, + Unavailable, + Timeout, + InvalidRequest, + ModelNotFound, + Network, + Protocol, + Unknown, +} + +impl fmt::Display for ProviderErrorKind { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let label = match self { + ProviderErrorKind::Unauthorized => "unauthorized", + ProviderErrorKind::RateLimited => "rate limited", + ProviderErrorKind::Unavailable => "unavailable", + ProviderErrorKind::Timeout => "timed out", + ProviderErrorKind::InvalidRequest => "invalid request", + ProviderErrorKind::ModelNotFound => "model not found", + ProviderErrorKind::Network => "network error", + ProviderErrorKind::Protocol => "protocol error", + ProviderErrorKind::Unknown => "unknown failure", + }; + write!(f, "{label}") + } +} + +/// Structured provider failure description used for UI and logs. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ProviderError { + pub provider_id: Option, + pub kind: ProviderErrorKind, + pub message: String, + #[serde(default)] + pub detail: Option, +} + +impl ProviderError { + /// Construct a new provider error with the given category and message. + pub fn new(kind: ProviderErrorKind, message: impl Into) -> Self { + Self { + provider_id: None, + kind, + message: message.into(), + detail: None, + } + } + + /// Attach the provider identifier to the failure. + pub fn with_provider(mut self, provider_id: impl Into) -> Self { + self.provider_id = Some(provider_id.into()); + self + } + + /// Attach a detailed description to the failure. + pub fn with_detail(mut self, detail: impl Into) -> Self { + let text = detail.into(); + if !text.trim().is_empty() { + self.detail = Some(text); + } + self + } +} + +impl fmt::Display for ProviderError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match (&self.detail, &self.provider_id) { + (Some(detail), Some(provider)) => { + write!(f, "{provider}: {} ({detail})", self.message) + } + (Some(detail), None) => write!(f, "{} ({detail})", self.message), + (None, Some(provider)) => write!(f, "{provider}: {}", self.message), + (None, None) => write!(f, "{}", self.message), + } + } +} + /// Describes core metadata for a provider implementation. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct ProviderMetadata { diff --git a/crates/owlen-core/src/providers/ollama.rs b/crates/owlen-core/src/providers/ollama.rs index f35fce4..326a244 100644 --- a/crates/owlen-core/src/providers/ollama.rs +++ b/crates/owlen-core/src/providers/ollama.rs @@ -48,6 +48,7 @@ use crate::{ llm::{LlmProvider, ProviderConfig}, mcp::McpToolDescriptor, model::{DetailedModelInfo, ModelDetailsCache, ModelManager}, + provider::{ProviderError, ProviderErrorKind}, types::{ ChatParameters, ChatRequest, ChatResponse, Message, ModelInfo, Role, TokenUsage, ToolCall, }, @@ -1330,21 +1331,58 @@ impl OllamaProvider { } } + fn provider_failure( + &self, + kind: ProviderErrorKind, + message: impl Into, + detail: Option, + ) -> Error { + let error = ProviderError::new(kind, message).with_provider(self.provider_name.clone()); + let error = if let Some(detail) = detail { + error.with_detail(detail) + } else { + error + }; + Error::ProviderFailure(error) + } + fn map_ollama_error(&self, action: &str, err: OllamaError, model: Option<&str>) -> Error { match err { OllamaError::ReqwestError(request_err) => { if let Some(status) = request_err.status() { self.map_http_failure(action, status, request_err.to_string(), model) } else if request_err.is_timeout() { - Error::Timeout(format!("Ollama {action} timed out: {request_err}")) + self.provider_failure( + ProviderErrorKind::Timeout, + format!("Ollama {action} timed out"), + Some(request_err.to_string()), + ) + } else if request_err.is_connect() || request_err.is_request() { + self.provider_failure( + ProviderErrorKind::Network, + format!("Ollama {action} request failed"), + Some(request_err.to_string()), + ) } else { - Error::Network(format!("Ollama {action} request failed: {request_err}")) + Error::Provider(anyhow!(request_err)) } } - OllamaError::InternalError(internal) => Error::Provider(anyhow!(internal.message)), - OllamaError::Other(message) => Error::Provider(anyhow!(message)), + OllamaError::InternalError(internal) => self.provider_failure( + ProviderErrorKind::Protocol, + format!("Ollama {action} internal error"), + Some(internal.message), + ), + OllamaError::Other(message) => self.provider_failure( + ProviderErrorKind::Unknown, + format!("Ollama {action} failed"), + Some(message), + ), OllamaError::JsonError(err) => Error::Serialization(err), - OllamaError::ToolCallError(err) => Error::Provider(anyhow!(err)), + OllamaError::ToolCallError(err) => self.provider_failure( + ProviderErrorKind::Protocol, + format!("Ollama {action} tool call failed"), + Some(err.to_string()), + ), } } @@ -1369,16 +1407,41 @@ impl OllamaProvider { )) } } - StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => Error::Auth(format!( - "Ollama rejected the request ({status}): {detail}. Check your API key and account permissions." - )), + StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => self.provider_failure( + ProviderErrorKind::Unauthorized, + format!( + "Ollama rejected the request ({status}). Check your API key and account permissions." + ), + Some(detail), + ), + StatusCode::TOO_MANY_REQUESTS => self.provider_failure( + ProviderErrorKind::RateLimited, + format!("Ollama {action} request rate limited"), + Some(detail), + ), StatusCode::BAD_REQUEST => { Error::InvalidInput(format!("{action} rejected by Ollama ({status}): {detail}")) } - StatusCode::SERVICE_UNAVAILABLE | StatusCode::GATEWAY_TIMEOUT => Error::Timeout( + StatusCode::SERVICE_UNAVAILABLE | StatusCode::GATEWAY_TIMEOUT => self.provider_failure( + ProviderErrorKind::Timeout, format!("Ollama {action} timed out ({status}). The model may still be loading."), + Some(detail), + ), + status if status.is_server_error() => self.provider_failure( + ProviderErrorKind::Unavailable, + format!("Ollama {action} request failed ({status}). Try again later."), + Some(detail), + ), + status if status.is_client_error() => self.provider_failure( + ProviderErrorKind::InvalidRequest, + format!("Ollama {action} rejected the request ({status})."), + Some(detail), + ), + _ => self.provider_failure( + ProviderErrorKind::Unknown, + format!("Ollama {action} failed ({status})."), + Some(detail), ), - _ => Error::Network(format!("Ollama {action} failed ({status}): {detail}")), } } } diff --git a/crates/owlen-providers/src/ollama/cloud.rs b/crates/owlen-providers/src/ollama/cloud.rs index 9a8aae7..4ab5607 100644 --- a/crates/owlen-providers/src/ollama/cloud.rs +++ b/crates/owlen-providers/src/ollama/cloud.rs @@ -6,8 +6,8 @@ use owlen_core::{ Error as CoreError, Result as CoreResult, config::OLLAMA_CLOUD_BASE_URL, provider::{ - GenerateRequest, GenerateStream, ModelInfo, ModelProvider, ProviderMetadata, - ProviderStatus, ProviderType, + GenerateRequest, GenerateStream, ModelInfo, ModelProvider, ProviderErrorKind, + ProviderMetadata, ProviderStatus, ProviderType, }, }; use serde_json::{Number, Value}; @@ -70,6 +70,11 @@ impl ModelProvider for OllamaCloudProvider { async fn health_check(&self) -> CoreResult { match self.client.health_check().await { Ok(status) => Ok(status), + Err(CoreError::ProviderFailure(failure)) + if failure.kind == ProviderErrorKind::Unauthorized => + { + Ok(ProviderStatus::RequiresSetup) + } Err(CoreError::Auth(_)) => Ok(ProviderStatus::RequiresSetup), Err(err) => Err(err), } diff --git a/crates/owlen-providers/src/ollama/shared.rs b/crates/owlen-providers/src/ollama/shared.rs index 37c4cb2..0c60153 100644 --- a/crates/owlen-providers/src/ollama/shared.rs +++ b/crates/owlen-providers/src/ollama/shared.rs @@ -1,10 +1,12 @@ use std::collections::HashMap; use std::time::Duration; +use anyhow::anyhow; use futures::StreamExt; use log::warn; use owlen_core::provider::{ - GenerateChunk, GenerateRequest, GenerateStream, ModelInfo, ProviderMetadata, ProviderStatus, + GenerateChunk, GenerateRequest, GenerateStream, ModelInfo, ProviderError, ProviderErrorKind, + ProviderMetadata, ProviderStatus, }; use owlen_core::{Error as CoreError, Result as CoreResult}; use reqwest::{Client, Method, StatusCode, Url}; @@ -39,7 +41,7 @@ impl OllamaClient { let http = Client::builder() .timeout(timeout) .build() - .map_err(map_reqwest_error)?; + .map_err(|err| CoreError::Provider(err.into()))?; Ok(Self { http, @@ -54,6 +56,78 @@ impl OllamaClient { &self.provider_metadata } + fn provider_failure( + &self, + kind: ProviderErrorKind, + message: impl Into, + detail: Option, + ) -> CoreError { + let error = + ProviderError::new(kind, message).with_provider(self.provider_metadata.id.clone()); + let error = if let Some(detail) = detail { + error.with_detail(detail) + } else { + error + }; + CoreError::ProviderFailure(error) + } + + fn map_http_error(&self, endpoint: &str, status: StatusCode, body: &[u8]) -> CoreError { + let snippet = truncated_body(body); + match status { + StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => self.provider_failure( + ProviderErrorKind::Unauthorized, + format!( + "Ollama {endpoint} request unauthorized (status {status}). Check your API key." + ), + Some(snippet), + ), + StatusCode::TOO_MANY_REQUESTS => self.provider_failure( + ProviderErrorKind::RateLimited, + format!("Ollama {endpoint} request rate limited"), + Some(snippet), + ), + StatusCode::SERVICE_UNAVAILABLE | StatusCode::GATEWAY_TIMEOUT => self.provider_failure( + ProviderErrorKind::Timeout, + format!("Ollama {endpoint} request timed out ({status})."), + Some(snippet), + ), + status if status.is_server_error() => self.provider_failure( + ProviderErrorKind::Unavailable, + format!("Ollama {endpoint} request failed ({status})."), + Some(snippet), + ), + status if status.is_client_error() => self.provider_failure( + ProviderErrorKind::InvalidRequest, + format!("Ollama {endpoint} request rejected ({status})."), + Some(snippet), + ), + _ => self.provider_failure( + ProviderErrorKind::Unknown, + format!("Ollama {endpoint} request failed ({status})."), + Some(snippet), + ), + } + } + + fn map_reqwest_error(&self, action: &str, err: reqwest::Error) -> CoreError { + if err.is_timeout() { + self.provider_failure( + ProviderErrorKind::Timeout, + format!("Ollama {action} timed out"), + Some(err.to_string()), + ) + } else if err.is_connect() || err.is_request() { + self.provider_failure( + ProviderErrorKind::Network, + format!("Ollama {action} request failed"), + Some(err.to_string()), + ) + } else { + CoreError::Provider(err.into()) + } + } + /// Perform a basic health check to determine provider availability. pub async fn health_check(&self) -> CoreResult { let url = self.endpoint("api/tags")?; @@ -62,7 +136,7 @@ impl OllamaClient { .request(Method::GET, url) .send() .await - .map_err(map_reqwest_error)?; + .map_err(|err| self.map_reqwest_error("health check", err))?; match response.status() { status if status.is_success() => Ok(ProviderStatus::Available), @@ -79,13 +153,16 @@ impl OllamaClient { .request(Method::GET, url) .send() .await - .map_err(map_reqwest_error)?; + .map_err(|err| self.map_reqwest_error("list models", err))?; let status = response.status(); - let bytes = response.bytes().await.map_err(map_reqwest_error)?; + let bytes = response + .bytes() + .await + .map_err(|err| self.map_reqwest_error("read model list", err))?; if !status.is_success() { - return Err(map_http_error("tags", status, &bytes)); + return Err(self.map_http_error("tags", status, &bytes)); } let payload: TagsResponse = @@ -111,17 +188,21 @@ impl OllamaClient { .json(&body) .send() .await - .map_err(map_reqwest_error)?; + .map_err(|err| self.map_reqwest_error("generate", err))?; let status = response.status(); if !status.is_success() { - let bytes = response.bytes().await.map_err(map_reqwest_error)?; - return Err(map_http_error("generate", status, &bytes)); + let bytes = response + .bytes() + .await + .map_err(|err| self.map_reqwest_error("read generate body", err))?; + return Err(self.map_http_error("generate", status, &bytes)); } let stream = response.bytes_stream(); let (tx, rx) = mpsc::channel::>(32); + let client = self.clone(); tokio::spawn(async move { let mut stream = stream; @@ -149,7 +230,9 @@ impl OllamaClient { } } Err(err) => { - let _ = tx.send(Err(map_reqwest_error(err))).await; + let _ = tx + .send(Err(client.map_reqwest_error("stream chunk", err))) + .await; return; } } @@ -358,7 +441,7 @@ fn parse_stream_line(line: &str) -> CoreResult { })?; if let Some(error) = value.get("error").and_then(Value::as_str) { - return Err(CoreError::Provider(anyhow::anyhow!( + return Err(CoreError::Provider(anyhow!( "ollama generation error: {}", error ))); @@ -390,29 +473,6 @@ fn parse_stream_line(line: &str) -> CoreResult { Ok(chunk) } -fn map_http_error(endpoint: &str, status: StatusCode, body: &[u8]) -> CoreError { - match status { - StatusCode::UNAUTHORIZED | StatusCode::FORBIDDEN => CoreError::Auth(format!( - "Ollama {} request unauthorized (status {})", - endpoint, status - )), - StatusCode::TOO_MANY_REQUESTS => CoreError::Provider(anyhow::anyhow!( - "Ollama {} request rate limited (status {})", - endpoint, - status - )), - _ => { - let snippet = truncated_body(body); - CoreError::Provider(anyhow::anyhow!( - "Ollama {} request failed: HTTP {} - {}", - endpoint, - status, - snippet - )) - } - } -} - fn truncated_body(body: &[u8]) -> String { const MAX_CHARS: usize = 512; let text = String::from_utf8_lossy(body); @@ -426,17 +486,6 @@ fn truncated_body(body: &[u8]) -> String { } value } - -fn map_reqwest_error(err: reqwest::Error) -> CoreError { - if err.is_timeout() { - CoreError::Timeout(err.to_string()) - } else if err.is_connect() || err.is_request() { - CoreError::Network(err.to_string()) - } else { - CoreError::Provider(err.into()) - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/owlen-tui/src/chat_app.rs b/crates/owlen-tui/src/chat_app.rs index c91e8fe..873bf95 100644 --- a/crates/owlen-tui/src/chat_app.rs +++ b/crates/owlen-tui/src/chat_app.rs @@ -11,8 +11,8 @@ use owlen_core::facade::llm_client::LlmClient; use owlen_core::mcp::remote_client::RemoteMcpClient; use owlen_core::mcp::{McpToolDescriptor, McpToolResponse}; use owlen_core::provider::{ - AnnotatedModelInfo, ModelInfo as ProviderModelInfo, ProviderMetadata, ProviderStatus, - ProviderType, + AnnotatedModelInfo, ModelInfo as ProviderModelInfo, ProviderErrorKind, ProviderMetadata, + ProviderStatus, ProviderType, }; use owlen_core::{ ProviderConfig, @@ -10530,82 +10530,152 @@ impl ChatApp { } } + async fn handle_cloud_unauthorized( + &mut self, + summary: impl Into, + detail: Option<&str>, + ) -> Result { + let summary = summary.into(); + let error_text = detail + .map(|value| value.trim()) + .filter(|value| !value.is_empty()) + .map(|detail| format!("{summary}: {detail}")) + .unwrap_or(summary.clone()); + + self.push_toast( + ToastLevel::Error, + "Cloud key invalid; using local provider.", + ); + + let switch_result = self.switch_to_provider("ollama_local").await; + if let Err(switch_err) = switch_result { + let detail_message = format!("{summary}; local fallback failed: {}", switch_err); + self.error = Some(detail_message.clone()); + self.status = "Cloud authentication failed".to_string(); + self.push_toast(ToastLevel::Error, detail_message); + } else { + self.selected_provider = "ollama_local".to_string(); + self.expanded_provider = Some("ollama_local".to_string()); + self.update_selected_provider_index(); + + { + let mut cfg = self.controller.config_mut(); + cfg.general.default_provider = "ollama_local".to_string(); + } + + let save_result = { + let cfg = self.controller.config(); + config::save_config(&cfg) + }; + if let Err(save_err) = save_result { + self.push_toast( + ToastLevel::Warning, + format!( + "Fell back to local provider, but failed to save config: {}", + save_err + ), + ); + } + + if let Err(refresh_err) = self.refresh_models().await { + self.push_toast( + ToastLevel::Warning, + format!("Failed to refresh local models: {}", refresh_err), + ); + } + + self.status = "Cloud authentication failed; using local provider instead.".to_string(); + self.error = Some(error_text); + self.push_toast(ToastLevel::Info, "Switched back to local provider."); + } + + Ok(true) + } + async fn handle_provider_error(&mut self, err: &CoreError) -> Result { let current_provider = Self::canonical_provider_id(&self.current_provider); if current_provider != "ollama_cloud" { return Ok(false); } - match err { - CoreError::Auth(message) => { - self.push_toast( - ToastLevel::Error, - "Cloud key invalid; using local provider.", - ); - - let switch_result = self.switch_to_provider("ollama_local").await; - if let Err(switch_err) = switch_result { - let detail = format!( - "Cloud key invalid and local fallback failed: {}", - switch_err - ); - self.error = Some(detail.clone()); - self.status = "Cloud authentication failed".to_string(); - self.push_toast(ToastLevel::Error, detail); - } else { - self.selected_provider = "ollama_local".to_string(); - self.expanded_provider = Some("ollama_local".to_string()); - self.update_selected_provider_index(); - - { - let mut cfg = self.controller.config_mut(); - cfg.general.default_provider = "ollama_local".to_string(); - } - - let save_result = { - let cfg = self.controller.config(); - config::save_config(&cfg) + if let CoreError::ProviderFailure(failure) = err { + match failure.kind { + ProviderErrorKind::Unauthorized => { + let detail = failure.detail.as_deref(); + let message = if detail.is_some() { + failure.message.clone() + } else { + "Cloud key invalid".to_string() }; - if let Err(save_err) = save_result { - self.push_toast( - ToastLevel::Warning, - format!( - "Fell back to local provider, but failed to save config: {}", - save_err - ), - ); - } - - if let Err(refresh_err) = self.refresh_models().await { - self.push_toast( - ToastLevel::Warning, - format!("Failed to refresh local models: {}", refresh_err), - ); - } - - self.status = - "Cloud authentication failed; using local provider instead.".to_string(); - self.error = Some(format!( - "Cloud key invalid: {}. Update your credentials and reselect the cloud provider.", - message - )); - self.push_toast(ToastLevel::Info, "Switched back to local provider."); + return self.handle_cloud_unauthorized(message, detail).await; } - - Ok(true) - } - CoreError::Network(message) => { - let lower = message.to_ascii_lowercase(); - if message.contains("429") - || lower.contains("too many requests") - || lower.contains("rate limit") - { - self.error = Some("Cloud rate limit hit; retry later.".to_string()); + ProviderErrorKind::RateLimited => { + let toast = failure.message.clone(); + self.error = Some( + failure + .detail + .as_ref() + .filter(|d| !d.trim().is_empty()) + .map(|detail| format!("{toast}: {detail}")) + .unwrap_or(toast.clone()), + ); self.status = "Cloud rate limit hit".to_string(); - self.push_toast(ToastLevel::Warning, "Cloud rate limit hit; retry later."); + self.push_toast(ToastLevel::Warning, toast); return Ok(true); } - Ok(false) + ProviderErrorKind::ModelNotFound => { + self.error = Some(failure.message.clone()); + self.status = "Model unavailable".to_string(); + let _ = self.refresh_models().await; + self.set_input_mode(InputMode::ProviderSelection); + return Ok(true); + } + ProviderErrorKind::Timeout => { + self.error = Some(failure.message.clone()); + self.status = "Request timed out".to_string(); + self.push_toast(ToastLevel::Warning, failure.message.clone()); + return Ok(true); + } + ProviderErrorKind::Unavailable => { + self.error = Some(failure.message.clone()); + self.status = "Cloud provider unavailable".to_string(); + self.push_toast(ToastLevel::Warning, failure.message.clone()); + return Ok(true); + } + ProviderErrorKind::Network => { + self.error = Some( + failure + .detail + .clone() + .unwrap_or_else(|| failure.message.clone()), + ); + self.status = "Network error".to_string(); + self.push_toast(ToastLevel::Warning, failure.message.clone()); + return Ok(true); + } + _ => { + self.error = Some( + failure + .detail + .clone() + .unwrap_or_else(|| failure.message.clone()), + ); + self.status = "Request failed".to_string(); + return Ok(true); + } + } + } + + match err { + CoreError::Auth(message) => { + self.handle_cloud_unauthorized("Cloud key invalid", Some(message.as_str())) + .await + } + CoreError::Network(message) => { + self.error = Some(message.clone()); + self.status = "Network error".to_string(); + self.push_toast(ToastLevel::Warning, "Network error talking to provider."); + Ok(true) } _ => Ok(false), }