From 3e473b1173238e9c83e5c305f756b6e988c3b180 Mon Sep 17 00:00:00 2001 From: MDario123 Date: Wed, 15 Jan 2025 17:49:02 +0100 Subject: [PATCH 1/4] try_load_caps_quickly_if_not_present return status codes other than 500 --- src/caps.rs | 79 ++++++++++++++++++++++++++++++++++--------- src/custom_error.rs | 8 +++++ src/global_context.rs | 5 ++- 3 files changed, 75 insertions(+), 17 deletions(-) diff --git a/src/caps.rs b/src/caps.rs index 8d07f98ac..203e91c07 100644 --- a/src/caps.rs +++ b/src/caps.rs @@ -157,7 +157,7 @@ pub struct CodeAssistantCaps { fn load_caps_from_buf( buffer: &String, caps_url: &String, -) -> Result>, String> { +) -> Result>, ScratchError> { let mut r1_mb_error_text = "".to_string(); let r1_mb: Option = match serde_json::from_str(&buffer) { @@ -178,12 +178,15 @@ fn load_caps_from_buf( } } }; - let mut r1 = r1_mb.ok_or(format!("failed to parse caps: {}", r1_mb_error_text))?; + let mut r1 = r1_mb.ok_or(ScratchError::new_internal(format!( + "failed to parse caps: {}", + r1_mb_error_text + )))?; let r0: ModelsOnly = serde_json::from_str(&KNOWN_MODELS).map_err(|e| { let up_to_line = KNOWN_MODELS.lines().take(e.line()).collect::>().join("\n"); error!("{}\nfailed to parse KNOWN_MODELS: {}", up_to_line, e); - format!("failed to parse KNOWN_MODELS: {}", e) + ScratchError::new_internal(format!("failed to parse KNOWN_MODELS: {}", e)) })?; if !r1.code_chat_default_model.is_empty() && !r1.running_models.contains(&r1.code_chat_default_model) { @@ -308,15 +311,21 @@ async fn load_caps_buf_from_file( async fn load_caps_buf_from_url( cmdline: crate::global_context::CommandLine, gcx: Arc>, -) -> Result<(String, String), String> { +) -> Result<(String, String), ScratchError> { let mut buffer = String::new(); let mut caps_urls: Vec = Vec::new(); if cmdline.address_url.to_lowercase() == "refact" { caps_urls.push("https://inference.smallcloud.ai/coding_assistant_caps.json".to_string()); } else { - let base_url = Url::parse(&cmdline.address_url.clone()).map_err(|_| "failed to parse address url (1)".to_string())?; - let joined_url = base_url.join(&CAPS_FILENAME).map_err(|_| "failed to parse address url (2)".to_string())?; - let joined_url_fallback = base_url.join(&CAPS_FILENAME_FALLBACK).map_err(|_| "failed to parse address url (2)".to_string())?; + let base_url = Url::parse(&cmdline.address_url.clone()).map_err(|_| { + ScratchError::new_internal("failed to parse address url (1)".to_string()) + })?; + let joined_url = base_url.join(&CAPS_FILENAME).map_err(|_| { + ScratchError::new_internal("failed to parse address url (2)".to_string()) + })?; + let joined_url_fallback = base_url.join(&CAPS_FILENAME_FALLBACK).map_err(|_| { + ScratchError::new_internal("failed to parse address url (2)".to_string()) + })?; caps_urls.push(joined_url.to_string()); caps_urls.push(joined_url_fallback.to_string()); } @@ -332,11 +341,25 @@ async fn load_caps_buf_from_url( let mut status: u16 = 0; for url in caps_urls.iter() { info!("fetching caps from {}", url); - let response = http_client.get(url).headers(headers.clone()).send().await.map_err(|e| format!("{}", e))?; + let response = http_client + .get(url) + .headers(headers.clone()) + .send() + .await + .map_err(|e| { + ScratchError::new( + e.status() + .unwrap_or(reqwest::StatusCode::INTERNAL_SERVER_ERROR) + .as_u16() + .try_into() + .unwrap_or(StatusCode::INTERNAL_SERVER_ERROR), + e.to_string(), + ) + })?; status = response.status().as_u16(); buffer = match response.text().await { Ok(v) => v, - Err(_) => continue + Err(_) => continue, }; if status == 200 { @@ -349,18 +372,40 @@ async fn load_caps_buf_from_url( let response_json: serde_json::Result = serde_json::from_str(&buffer); return if let Ok(response_json) = response_json { if let Some(detail) = response_json.get("detail") { - Err(detail.as_str().unwrap().to_string()) + Err(ScratchError::new( + status + .try_into() + .unwrap_or(StatusCode::INTERNAL_SERVER_ERROR), + detail.as_str().unwrap().to_string(), + )) } else { - Err(format!("cannot fetch caps, status={}", status)) + Err(ScratchError::new( + status + .try_into() + .unwrap_or(StatusCode::INTERNAL_SERVER_ERROR), + format!("cannot fetch caps, status={}", status), + )) } } else { - Err(format!("cannot fetch caps, status={}", status)) + Err(ScratchError::new( + status + .try_into() + .unwrap_or(StatusCode::INTERNAL_SERVER_ERROR), + format!("cannot fetch caps, status={}", status), + )) }; } let caps_url: String = match caps_urls.get(0) { Some(u) => u.clone(), - None => return Err("caps_url is none".to_string()) + None => { + return Err(ScratchError::new( + status + .try_into() + .unwrap_or(StatusCode::INTERNAL_SERVER_ERROR), + "caps_url is none".to_string(), + )) + } }; Ok((buffer, caps_url)) @@ -369,15 +414,17 @@ async fn load_caps_buf_from_url( pub async fn load_caps( cmdline: crate::global_context::CommandLine, gcx: Arc>, -) -> Result>, String> { +) -> Result>, ScratchError> { let mut caps_url = cmdline.address_url.clone(); let buf: String; if caps_url.to_lowercase() == "refact" || caps_url.starts_with("http") { (buf, caps_url) = load_caps_buf_from_url(cmdline, gcx).await? } else { - (buf, caps_url) = load_caps_buf_from_file(cmdline, gcx).await? + (buf, caps_url) = load_caps_buf_from_file(cmdline, gcx) + .await + .map_err(ScratchError::new_internal)? } - load_caps_from_buf(&buf, &caps_url) + load_caps_from_buf(&buf, &caps_url).map_err(ScratchError::new_internal) } pub fn strip_model_from_finetune(model: &String) -> String { diff --git a/src/custom_error.rs b/src/custom_error.rs index d537be3da..2d96e39f5 100644 --- a/src/custom_error.rs +++ b/src/custom_error.rs @@ -49,6 +49,14 @@ impl ScratchError { } } + pub fn new_internal(message: String) -> Self { + ScratchError { + status_code: StatusCode::INTERNAL_SERVER_ERROR, + message, + telemetry_skip: false, + } + } + pub fn to_response(&self) -> Response { let body = json!({"detail": self.message}).to_string(); error!("client will see {}", body); diff --git a/src/global_context.rs b/src/global_context.rs index fa374c8be..dd9de03b6 100644 --- a/src/global_context.rs +++ b/src/global_context.rs @@ -250,7 +250,10 @@ pub async fn try_load_caps_quickly_if_not_present( Err(e) => { error!("caps fetch failed: {:?}", e); gcx_locked.caps_last_error = format!("caps fetch failed: {}", e); - return Err(ScratchError::new(StatusCode::INTERNAL_SERVER_ERROR, gcx_locked.caps_last_error.clone())); + Err(ScratchError::new( + e.status_code, + gcx_locked.caps_last_error.clone(), + )) } } } From 9d695c20ebb4dc1f2a4ba81f1ae1f82ed371917d Mon Sep 17 00:00:00 2001 From: MDario123 Date: Wed, 15 Jan 2025 18:18:09 +0100 Subject: [PATCH 2/4] fix oops and add documentation small doc for ScratchError::new_internal --- src/caps.rs | 20 +++++++++----------- src/custom_error.rs | 2 ++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/caps.rs b/src/caps.rs index 203e91c07..10fffbb40 100644 --- a/src/caps.rs +++ b/src/caps.rs @@ -1,15 +1,16 @@ -use std::path::PathBuf; +use hyper::StatusCode; +use serde::Deserialize; +use serde::Serialize; +use serde_json::Value; use std::collections::HashMap; use std::fs::File; use std::io::Read; +use std::path::PathBuf; use std::sync::Arc; use std::sync::RwLock as StdRwLock; -use serde::Deserialize; -use serde::Serialize; -use serde_json::Value; use tokio::sync::RwLock as ARwLock; -use url::Url; use tracing::{error, info, warn}; +use url::Url; use crate::custom_error::ScratchError; use crate::global_context::{try_load_caps_quickly_if_not_present, GlobalContext}; @@ -157,7 +158,7 @@ pub struct CodeAssistantCaps { fn load_caps_from_buf( buffer: &String, caps_url: &String, -) -> Result>, ScratchError> { +) -> Result>, String> { let mut r1_mb_error_text = "".to_string(); let r1_mb: Option = match serde_json::from_str(&buffer) { @@ -178,15 +179,12 @@ fn load_caps_from_buf( } } }; - let mut r1 = r1_mb.ok_or(ScratchError::new_internal(format!( - "failed to parse caps: {}", - r1_mb_error_text - )))?; + let mut r1 = r1_mb.ok_or(format!("failed to parse caps: {}", r1_mb_error_text))?; let r0: ModelsOnly = serde_json::from_str(&KNOWN_MODELS).map_err(|e| { let up_to_line = KNOWN_MODELS.lines().take(e.line()).collect::>().join("\n"); error!("{}\nfailed to parse KNOWN_MODELS: {}", up_to_line, e); - ScratchError::new_internal(format!("failed to parse KNOWN_MODELS: {}", e)) + format!("failed to parse KNOWN_MODELS: {}", e) })?; if !r1.code_chat_default_model.is_empty() && !r1.running_models.contains(&r1.code_chat_default_model) { diff --git a/src/custom_error.rs b/src/custom_error.rs index 2d96e39f5..d19b9223d 100644 --- a/src/custom_error.rs +++ b/src/custom_error.rs @@ -49,6 +49,8 @@ impl ScratchError { } } + /// This is a helper function to create a new [`ScratchError`] + /// with `status_code` = `INTERNAL_SERVER_ERROR` pub fn new_internal(message: String) -> Self { ScratchError { status_code: StatusCode::INTERNAL_SERVER_ERROR, From 3779f7f73c334757c7e5418bb2d47013ce1ac315 Mon Sep 17 00:00:00 2001 From: MDario123 Date: Wed, 15 Jan 2025 20:12:07 +0100 Subject: [PATCH 3/4] fix: /caps don't return always 503 also some minor styling changes and slightly change error message --- src/custom_error.rs | 5 ++--- src/global_context.rs | 2 +- src/http/routers/v1/caps.rs | 14 ++++---------- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/custom_error.rs b/src/custom_error.rs index d19b9223d..e5f36ada8 100644 --- a/src/custom_error.rs +++ b/src/custom_error.rs @@ -62,11 +62,10 @@ impl ScratchError { pub fn to_response(&self) -> Response { let body = json!({"detail": self.message}).to_string(); error!("client will see {}", body); - let response = Response::builder() + Response::builder() .status(self.status_code) .header("Content-Type", "application/json") .body(Body::from(body)) - .unwrap(); - response + .unwrap() } } diff --git a/src/global_context.rs b/src/global_context.rs index dd9de03b6..c5640d0fa 100644 --- a/src/global_context.rs +++ b/src/global_context.rs @@ -249,7 +249,7 @@ pub async fn try_load_caps_quickly_if_not_present( }, Err(e) => { error!("caps fetch failed: {:?}", e); - gcx_locked.caps_last_error = format!("caps fetch failed: {}", e); + gcx_locked.caps_last_error = format!("caps fetch failed: {}", e.message); Err(ScratchError::new( e.status_code, gcx_locked.caps_last_error.clone(), diff --git a/src/http/routers/v1/caps.rs b/src/http/routers/v1/caps.rs index aa1352471..396d112c9 100644 --- a/src/http/routers/v1/caps.rs +++ b/src/http/routers/v1/caps.rs @@ -25,16 +25,10 @@ pub async fn handle_v1_caps( Extension(global_context): Extension>>, _: hyper::body::Bytes, ) -> Result, ScratchError> { - let caps_result = crate::global_context::try_load_caps_quickly_if_not_present( - global_context.clone(), - 0, - ).await; - let caps_arc = match caps_result { - Ok(x) => x, - Err(e) => { - return Err(ScratchError::new(StatusCode::SERVICE_UNAVAILABLE, format!("{}", e))); - } - }; + let caps_arc = + crate::global_context::try_load_caps_quickly_if_not_present(global_context.clone(), 0) + .await?; + let caps_locked = caps_arc.read().unwrap(); let body = serde_json::to_string_pretty(&*caps_locked).unwrap(); let response = Response::builder() From 15ba37c47f86126cc8e2274f8a15b5a1210f3785 Mon Sep 17 00:00:00 2001 From: MDario123 Date: Thu, 16 Jan 2025 16:26:35 +0100 Subject: [PATCH 4/4] cache caps response with status code --- src/global_context.rs | 24 +++++++++++++++--------- src/http/routers/v1/caps.rs | 5 ++--- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/global_context.rs b/src/global_context.rs index c5640d0fa..a619b7c40 100644 --- a/src/global_context.rs +++ b/src/global_context.rs @@ -146,7 +146,7 @@ pub struct GlobalContext { pub config_dir: PathBuf, pub caps: Option>>, pub caps_reading_lock: Arc>, - pub caps_last_error: String, + pub caps_last_error: Option, pub caps_last_attempted_ts: u64, pub tokenizer_map: HashMap< String, Arc>>, pub tokenizer_download_lock: Arc>, @@ -229,7 +229,10 @@ pub async fn try_load_caps_quickly_if_not_present( } if caps_last_attempted_ts + CAPS_RELOAD_BACKOFF > now { let gcx_locked = gcx.write().await; - return Err(ScratchError::new(StatusCode::INTERNAL_SERVER_ERROR, gcx_locked.caps_last_error.clone())); + info!("Returning caps error from cache"); + return Err(gcx_locked.caps_last_error.clone().unwrap_or( + ScratchError::new_internal("Expected cached error, but none found".to_string()), + )); } } @@ -244,16 +247,19 @@ pub async fn try_load_caps_quickly_if_not_present( match caps_result { Ok(caps) => { gcx_locked.caps = Some(caps.clone()); - gcx_locked.caps_last_error = "".to_string(); + gcx_locked.caps_last_error = None; Ok(caps) - }, + } Err(e) => { error!("caps fetch failed: {:?}", e); - gcx_locked.caps_last_error = format!("caps fetch failed: {}", e.message); - Err(ScratchError::new( + gcx_locked.caps_last_error = Some(ScratchError::new( e.status_code, - gcx_locked.caps_last_error.clone(), - )) + format!("caps fetch failed: {}", e.message), + )); + Err(gcx_locked + .caps_last_error + .clone() + .expect("The previous line is assigning it")) } } } @@ -358,7 +364,7 @@ pub async fn create_global_context( config_dir, caps: None, caps_reading_lock: Arc::new(AMutex::::new(false)), - caps_last_error: String::new(), + caps_last_error: None, caps_last_attempted_ts: 0, tokenizer_map: HashMap::new(), tokenizer_download_lock: Arc::new(AMutex::::new(false)), diff --git a/src/http/routers/v1/caps.rs b/src/http/routers/v1/caps.rs index 396d112c9..e6d646b91 100644 --- a/src/http/routers/v1/caps.rs +++ b/src/http/routers/v1/caps.rs @@ -1,9 +1,8 @@ use std::sync::Arc; use tokio::sync::RwLock as ARwLock; -use axum::Extension; -use axum::response::Result; -use hyper::{Body, Response, StatusCode}; +use axum::{response::Result, Extension}; +use hyper::{Body, Response}; use crate::custom_error::ScratchError; use crate::global_context::GlobalContext;