diff --git a/src/openhuman/inference/embeddings/rpc.rs b/src/openhuman/inference/embeddings/rpc.rs index e3513ee10a..3171df58db 100644 --- a/src/openhuman/inference/embeddings/rpc.rs +++ b/src/openhuman/inference/embeddings/rpc.rs @@ -1104,8 +1104,18 @@ mod tests { config.memory.embedding_provider = "cloud".to_string(); // A managed session exists, so the ladder would resolve to cloud … std::fs::write(tmp.path().join("auth-profiles.json"), "{}").unwrap(); - // … except the unified workload setting routes embeddings to Ollama. + // … except a local Ollama route wins. As of tinymemory v1.0.1 the + // effective-embedder ladder no longer treats the `embeddings_provider` + // string alone as authoritative for local routing — local Ollama is + // resolved from an explicit `memory_tree.embedding_endpoint` override or + // the unified `workload_local_model` setting. Drive the explicit + // endpoint rung here: it resolves deterministically without an installed + // embedding host, and still exercises the point of the test — that + // `provider` (the picker) stays `cloud` while `effective_provider` + // reports the local route that bills nothing (#5402). config.embeddings_provider = Some("ollama:all-minilm:latest".into()); + config.memory_tree.embedding_endpoint = Some("http://localhost:11434".into()); + config.memory_tree.embedding_model = Some("all-minilm".into()); let out = get_settings(&config) .await diff --git a/src/openhuman/inference/provider/openhuman_backend_model.rs b/src/openhuman/inference/provider/openhuman_backend_model.rs index 18502f02e0..33b3d8d403 100644 --- a/src/openhuman/inference/provider/openhuman_backend_model.rs +++ b/src/openhuman/inference/provider/openhuman_backend_model.rs @@ -106,22 +106,44 @@ impl OpenHumanBackendModel { } fn resolve_bearer(&self) -> anyhow::Result { + use crate::openhuman::security::credentials::session_support::{ + classify_session_token, SessionTokenCheck, + }; + if crate::openhuman::cron::scheduler_gate::is_signed_out() { anyhow::bail!( "SESSION_EXPIRED: backend session not active — sign in to resume LLM work" ); } let auth = AuthService::new(&self.state_dir(), self.options.secrets_encrypt); - if let Some(token) = auth - .get_provider_bearer_token( - APP_SESSION_PROVIDER, - self.options.auth_profile_override.as_deref(), - )? - .filter(|token| !token.trim().is_empty()) - { - return Ok(token); + let profile = auth.get_profile( + APP_SESSION_PROVIDER, + self.options.auth_profile_override.as_deref(), + )?; + + // #5503: precheck the recorded JWT `exp` BEFORE building a request, the + // same way `require_live_session_token` guards the backend REST callers. + // Managed inference used to fire a doomed request on an expired-but- + // stored token and let the 401 come back — but an expired session can + // also surface upstream as a misleading "model unavailable", which is a + // core symptom of #5503 (all tiers "die" over a long session). Failing + // fast as `session_expired` routes the user to re-auth instead. Offline + // / local sessions (`is_local_session_token`) and `exp`-less tokens + // carry no recorded expiry, so `classify_session_token` returns `Live` + // for them — their behaviour is unchanged and the post-call 401 net + // still covers a server-side revocation. + match classify_session_token(profile.as_ref(), chrono::Utc::now()) { + SessionTokenCheck::Live(token) => Ok(token), + SessionTokenCheck::Expired => { + maybe_publish_local_session_expiry(); + anyhow::bail!( + "SESSION_EXPIRED: backend session token expired locally — re-authentication required" + ) + } + SessionTokenCheck::Absent => { + anyhow::bail!("No backend session: store a JWT via auth (app-session)") + } } - anyhow::bail!("No backend session: store a JWT via auth (app-session)") } fn base_url(&self) -> String { @@ -381,6 +403,27 @@ fn with_thread_id(mut request: ModelRequest) -> ModelRequest { request } +/// Publish a `SessionExpired` event when the local `exp` precheck in +/// [`resolve_bearer`](OpenHumanBackendModel::resolve_bearer) rejects an expired +/// managed session token before a request is ever sent — mirroring +/// [`require_live_session_token`](crate::openhuman::security::credentials::session_support::require_live_session_token)'s +/// pre-flight publish so the credentials subscriber clears state and the UI +/// re-auths exactly as it would on a real backend 401. Deduped via the +/// scheduler gate so N parallel managed turns in one tick don't emit N events. +fn maybe_publish_local_session_expiry() { + if crate::openhuman::cron::scheduler_gate::is_signed_out() { + return; + } + log::warn!( + "[providers][openhuman-backend] managed session token expired locally — \ + publishing SessionExpired before any inference request" + ); + crate::core::bus::BUS.publish(crate::core::events::DomainEvent::SessionExpired { + source: "openhuman_backend_model.resolve_bearer".to_string(), + reason: "backend session token expired locally — re-authentication required".to_string(), + }); +} + /// Publish a `SessionExpired` event when the backend rejects a crate-native /// model call with `401`/`403` Unauthorized — mirroring the check in /// [`CrateBackedProvider::invoke`](super::CrateBackedProvider) which the @@ -402,6 +445,36 @@ fn maybe_publish_session_expired(err: &TinyAgentsError, operation: &str) { } } +/// Log the raw upstream failure at the managed inference dispatch boundary +/// (#5503, part d). The managed unavailability path used to surface the true +/// backend cause only after the web-chat error classifier had already collapsed +/// it to a user-facing bucket, so an operator investigating "all tiers died +/// over hours" had no record of what the backend actually returned. This is the +/// one place every managed `invoke`/`stream` failure passes through, so it's +/// where the diagnostic belongs. Structured fields (`status`/`code`/`provider`/ +/// `retryable`) are low-cardinality; the message is secret-scrubbed and capped +/// by [`sanitize_api_error`] before it's logged — no tokens, no full PII. +fn log_managed_dispatch_error(err: &TinyAgentsError, operation: &str) { + match err { + TinyAgentsError::Provider(pe) => { + log::warn!( + "[providers][openhuman-backend] managed {operation} failed: status={:?} code={:?} provider={} retryable={} detail={}", + pe.status, + pe.code, + pe.provider, + pe.retryable, + crate::openhuman::inference::provider::ops::sanitize_api_error(&pe.message), + ); + } + other => { + log::warn!( + "[providers][openhuman-backend] managed {operation} failed (non-provider error): {}", + crate::openhuman::inference::provider::ops::sanitize_api_error(&other.to_string()), + ); + } + } +} + #[async_trait] impl ChatModel<()> for OpenHumanBackendModel { fn profile(&self) -> Option<&ModelProfile> { @@ -413,6 +486,7 @@ impl ChatModel<()> for OpenHumanBackendModel { let response = match model.invoke(state, with_thread_id(request)).await { Ok(response) => response, Err(e) => { + log_managed_dispatch_error(&e, "invoke"); maybe_publish_session_expired(&e, "invoke"); return Err(e); } @@ -432,6 +506,7 @@ impl ChatModel<()> for OpenHumanBackendModel { match model.stream(state, with_thread_id(request)).await { Ok(stream) => Ok(stream), Err(e) => { + log_managed_dispatch_error(&e, "stream"); maybe_publish_session_expired(&e, "stream"); Err(e) } @@ -690,6 +765,27 @@ mod tests { .expect("seed app-session token"); } + /// Seed an app-session profile whose recorded `exp` metadata is `expires_at` + /// (RFC3339) so the `resolve_bearer` local-expiry precheck (#5503, part e) + /// can be exercised without a live backend. + fn seed_app_session_with_expiry(dir: &std::path::Path, expires_at: &str) { + use crate::openhuman::security::credentials::{ + session_support::SESSION_EXPIRES_AT_META, AuthService, APP_SESSION_PROVIDER, + DEFAULT_AUTH_PROFILE_NAME, + }; + let mut metadata = std::collections::HashMap::new(); + metadata.insert(SESSION_EXPIRES_AT_META.to_string(), expires_at.to_string()); + AuthService::new(dir, false) + .store_provider_token( + APP_SESSION_PROVIDER, + DEFAULT_AUTH_PROFILE_NAME, + "test.session.jwt", + metadata, + true, + ) + .expect("seed app-session token with expiry"); + } + fn backend_pointed_at(addr: &str, dir: &std::path::Path) -> OpenHumanBackendModel { OpenHumanBackendModel::new( Some(&format!("http://{addr}")), @@ -819,4 +915,64 @@ mod tests { "probe must return around the 5s timeout, not wait for the slow handler" ); } + + // ── resolve_bearer local-expiry precheck (#5503, part e) ─────────────── + + #[test] + fn resolve_bearer_fast_fails_session_expired_on_expired_token() { + // An app-session JWT whose recorded `exp` is in the past must fail the + // precheck as a `SESSION_EXPIRED` sentinel BEFORE any request is built — + // so the web-chat classifier routes it to `session_expired` (actionable + // re-auth) instead of a doomed request that can surface as a misleading + // "model unavailable" (#5503). No backend is stood up: a correct + // precheck never reaches the network. + let tmp = tempfile::TempDir::new().unwrap(); + let past = (chrono::Utc::now() - chrono::Duration::hours(1)).to_rfc3339(); + seed_app_session_with_expiry(tmp.path(), &past); + let backend = backend_pointed_at("127.0.0.1:9", tmp.path()); + + let err = backend + .resolve_bearer() + .expect_err("an expired managed JWT must fast-fail the precheck"); + let msg = err.to_string(); + assert!( + msg.contains("SESSION_EXPIRED"), + "must carry the SESSION_EXPIRED sentinel the classifier keys on: {msg}" + ); + assert!( + crate::core::observability::is_session_expired_message(&msg), + "must classify as session-expiry, not model-unavailable: {msg}" + ); + } + + #[test] + fn resolve_bearer_returns_token_when_expiry_in_future() { + // A recorded `exp` comfortably in the future resolves normally — the + // precheck only rejects the past-expiry case. + let tmp = tempfile::TempDir::new().unwrap(); + let future = (chrono::Utc::now() + chrono::Duration::hours(1)).to_rfc3339(); + seed_app_session_with_expiry(tmp.path(), &future); + let backend = backend_pointed_at("127.0.0.1:9", tmp.path()); + + let token = backend + .resolve_bearer() + .expect("a live (future-exp) managed JWT must resolve"); + assert_eq!(token, "test.session.jwt"); + } + + #[test] + fn resolve_bearer_returns_token_for_exp_less_offline_session() { + // Offline / local sessions record no `exp`, so the precheck falls + // through to presence-only and their behaviour is unchanged (the + // post-call 401 net still covers a server-side revocation). Guards the + // #5503 precheck against breaking the offline path. + let tmp = tempfile::TempDir::new().unwrap(); + seed_app_session(tmp.path()); + let backend = backend_pointed_at("127.0.0.1:9", tmp.path()); + + let token = backend + .resolve_bearer() + .expect("an exp-less offline session must resolve (presence-only)"); + assert_eq!(token, "test.session.jwt"); + } } diff --git a/src/openhuman/web_chat/web_errors.rs b/src/openhuman/web_chat/web_errors.rs index 5ab28db4a6..36daf15ebf 100644 --- a/src/openhuman/web_chat/web_errors.rs +++ b/src/openhuman/web_chat/web_errors.rs @@ -858,17 +858,46 @@ pub(crate) fn classify_inference_error(err: &str) -> ClassifiedError { || lower.contains("does not exist") || lower.contains("does not have access")) { - ClassifiedError { - error_type: "model_unavailable", - message: with_provider_detail( - "The selected model isn't available on your provider. Check your model settings.", - err, - ), - source: "config", - retryable: false, - retry_after_ms: None, - provider, - fallback_available: None, + // #5503: this arm previously flattened two distinct failures into one + // non-retryable "check your model settings" misconfiguration verdict. + // A TRANSIENT upstream outage ("the model is temporarily unavailable", + // "currently overloaded") is NOT a user misconfiguration — labelling it + // `config`/non-retryable tells the user to go fix settings that are + // fine, and hides the Retry button on a failure a retry would clear. So + // split on transience: a body carrying a temporary-outage marker routes + // to the retryable "temporarily unavailable" provider copy (the same + // class as the `500`/`503` arm above), while a genuine model rejection + // ("does not exist", "does not have access", "model unavailable on this + // endpoint" — a stale pin / wrong endpoint) keeps the non-retryable + // config copy. Genuine config-rejection bodies (`does not exist`, + // `model_not_found`, `/openai/v1/models`, …) are already claimed by the + // provider-config-rejection arm above and never reach here. + if is_transient_unavailability_text(&lower) { + ClassifiedError { + error_type: "provider_error", + message: with_provider_detail( + "The AI provider is temporarily unavailable. Please try again later.", + err, + ), + source: "provider", + retryable: true, + retry_after_ms: None, + provider, + fallback_available, + } + } else { + ClassifiedError { + error_type: "model_unavailable", + message: with_provider_detail( + "The selected model isn't available on your provider. Check your model settings.", + err, + ), + source: "config", + retryable: false, + retry_after_ms: None, + provider, + fallback_available: None, + } } } else if lower.contains("does not support vision") || lower.contains("capability=vision") { // A multimodal turn sent image markers to a text-only model @@ -953,6 +982,22 @@ pub(crate) fn classify_inference_error(err: &str) -> ClassifiedError { provider, fallback_available, } + } else if is_transient_unavailability_text(&lower) { + // A transient upstream-outage marker that no more specific arm above + // claimed (e.g. a bare 5xx "overloaded" such as Anthropic's 529, or + // "please retry later") is a temporary provider outage — surface the + // retryable "temporarily unavailable" provider copy rather than the flat + // inference bucket, so the user gets an accurate, retryable error (#5503). + ClassifiedError { + error_type: "provider_error", + message: "The AI provider is temporarily unavailable. Please try again later." + .to_string(), + source: "provider", + retryable: true, + retry_after_ms: None, + provider, + fallback_available, + } } else { ClassifiedError { error_type: "inference", @@ -1079,6 +1124,35 @@ pub(crate) fn is_provider_request_rejected_text(lower: &str) -> bool { .any(|marker| lower.contains(marker)) } +/// Whether a model-availability error body describes a **transient** upstream +/// outage rather than a user misconfiguration (#5503). +/// +/// The `model_unavailable` arm matches on the bare word `unavailable`, which a +/// provider emits for BOTH "you picked a model I don't host" (config, terminal) +/// and "this model is temporarily down / overloaded right now" (transient, +/// retryable). Only the second class carries one of these temporary-outage +/// markers, so it's the safe discriminator: a terminal endpoint rejection like +/// `"model unavailable on this endpoint"` (a 404 for a model that endpoint +/// doesn't host) carries none of them and stays on the config verdict. +/// +/// Deliberately does NOT key on the bare word `unavailable` — that's the very +/// ambiguity being disambiguated. Caller passes the already-lowercased string. +pub(crate) fn is_transient_unavailability_text(lower: &str) -> bool { + const TRANSIENT_MARKERS: &[&str] = &[ + "temporarily", + "temporary", + "currently unavailable", + "currently overloaded", + "overloaded", + "try again later", + "try again in a", + "please retry", + ]; + TRANSIENT_MARKERS + .iter() + .any(|marker| lower.contains(marker)) +} + /// String-flat mirror of /// [`crate::openhuman::inference::provider::error_classify::is_non_retryable_rate_limit`]. /// diff --git a/src/openhuman/web_chat/web_tests.rs b/src/openhuman/web_chat/web_tests.rs index 582361fb4b..0e43b0e9f1 100644 --- a/src/openhuman/web_chat/web_tests.rs +++ b/src/openhuman/web_chat/web_tests.rs @@ -342,6 +342,139 @@ fn classify_inference_error_chat_factory_empty_model_is_actionable_config() { ); } +// ── #5503: transient model-unavailable vs misconfiguration ───── + +#[test] +fn classify_inference_error_transient_model_unavailable_is_retryable_not_config() { + // #5503: a BYO/direct-provider body that says the model is *temporarily* + // down (a transient upstream outage — the real symptom behind "all tiers + // die over a long session") must NOT be flattened into the non-retryable + // "check your model settings" misconfiguration copy. It routes to the + // retryable "temporarily unavailable" provider class instead, so the FE + // keeps the Retry button and doesn't send the user to fix settings that + // are fine. Each of these carries a temporary-outage marker. + for raw in [ + r#"custom_openai API error (503 Service Unavailable): {"error":{"message":"The model is temporarily unavailable, please try again later."}}"#, + r#"cloud API error (529): {"error":{"message":"model is currently overloaded"}}"#, + r#"openrouter API error (503): {"error":{"message":"This model is temporarily unavailable. Please retry."}}"#, + ] { + let ClassifiedError { + error_type, + message, + retryable, + source, + .. + } = classify_inference_error(raw); + assert_eq!( + error_type, "provider_error", + "transient model outage must classify as provider_error, not model_unavailable: {raw}" + ); + assert!( + retryable, + "transient model outage must stay retryable (keep Retry): {raw}" + ); + assert_eq!( + source, "provider", + "transient outage is a provider fault: {raw}" + ); + assert!( + message.contains("temporarily unavailable"), + "must use the temporarily-unavailable copy: {message}" + ); + assert!( + !message.to_lowercase().contains("check your model settings"), + "must NOT tell the user their configuration is wrong: {message}" + ); + } +} + +#[test] +fn classify_inference_error_genuine_model_rejection_stays_nonretryable_config() { + // Guard the other direction: a genuine model rejection with NO + // temporary-outage marker (wrong endpoint, no access) keeps the + // non-retryable `model_unavailable` + "check your model settings" config + // verdict. This is the half of the #5503 split that must not regress the + // pre-existing behaviour. + for raw in [ + // Endpoint doesn't host this model (a terminal 404, not a transient dip). + r#"custom_openai API error (404 Not Found): {"error":{"message":"model unavailable on this endpoint"}}"#, + // No access to the requested model — bare "not found", no outage marker. + r#"custom_openai API error (404 Not Found): {"error":{"message":"the requested model was not found for this account"}}"#, + ] { + let ClassifiedError { + error_type, + message, + retryable, + source, + .. + } = classify_inference_error(raw); + assert_eq!( + error_type, "model_unavailable", + "genuine model rejection must stay model_unavailable: {raw}" + ); + assert!( + !retryable, + "genuine model rejection is non-retryable (hide Retry): {raw}" + ); + assert_eq!( + source, "config", + "genuine model rejection is user config: {raw}" + ); + assert!( + message.contains("Check your model settings"), + "must keep the actionable config copy: {message}" + ); + } +} + +#[test] +fn classify_inference_error_transient_model_unavailable_without_5xx_status_uses_split_arm() { + // #5503 coverage guard for the split's *own* true branch. The two fixtures + // in `..._is_retryable_not_config` above each carry a `503`/`529` status, so + // they are already claimed by the generic 5xx arm and never reach the + // model-unavailable split. A transient outage reported with NO 5xx status — + // a bare provider body that only says the model is temporarily unavailable / + // currently unavailable — can be rescued from the non-retryable "check your + // model settings" verdict *only* by the split arm itself. So this exercises + // the branch the other fixtures miss: each body carries the "model" + + // "unavailable" trigger (so it enters the model arm, not the 5xx arm) plus a + // temporary-outage marker (so it takes the retryable TRUE branch). On the + // pre-#5503 flattened code these classified as non-retryable + // `model_unavailable`; the split makes them retryable `provider_error`. + for raw in [ + r#"custom_openai API error: {"error":{"message":"The model is temporarily unavailable, please try again later."}}"#, + r#"openrouter API error: {"error":{"message":"This model is currently unavailable; please retry shortly."}}"#, + ] { + let ClassifiedError { + error_type, + message, + retryable, + source, + .. + } = classify_inference_error(raw); + assert_eq!( + error_type, "provider_error", + "no-status transient outage must reach the split arm as provider_error, not model_unavailable: {raw}" + ); + assert!( + retryable, + "no-status transient outage must stay retryable (keep Retry): {raw}" + ); + assert_eq!( + source, "provider", + "transient outage is a provider fault: {raw}" + ); + assert!( + message.contains("temporarily unavailable"), + "must use the temporarily-unavailable copy: {message}" + ); + assert!( + !message.to_lowercase().contains("check your model settings"), + "must NOT tell the user their configuration is wrong: {message}" + ); + } +} + // ── #2364: rate-limit classification + retry-after surfacing ──── #[test]