From 0193714a101423e9d0f9dc767de375976675996d Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 11 Jun 2026 12:14:00 +0530 Subject: [PATCH] fix(observability): classify in-band SSE error frames instead of dropping them (#3588) Co-authored-by: Cyrus Gray --- src/core/observability.rs | 65 +++++++- .../provider/compatible_stream_native.rs | 139 ++++++++++++++++++ .../inference/provider/error_code.rs | 57 ++++++- src/openhuman/inference/provider/mod.rs | 5 +- src/openhuman/inference/provider/ops_tests.rs | 19 ++- 5 files changed, 272 insertions(+), 13 deletions(-) diff --git a/src/core/observability.rs b/src/core/observability.rs index 57b80132b..854f9e9a6 100644 --- a/src/core/observability.rs +++ b/src/core/observability.rs @@ -302,6 +302,21 @@ pub fn expected_error_kind(message: &str) -> Option { if crate::openhuman::inference::provider::managed_error_skips_sentry(message) { return Some(ExpectedErrorKind::BackendErrorCodeOwned); } + // A managed-backend client-guard-leak code (`PAYLOAD_TOO_LARGE` / + // `CONTEXT_LENGTH_EXCEEDED`) must PAGE — the client enforces these limits + // before sending, so a backend rejection is our guard leaking. Force + // capture (return `None`) here, BEFORE the substring matchers below: a real + // managed `CONTEXT_LENGTH_EXCEEDED` body carries "context length + // exceeded"-style text that `is_context_window_exceeded_message` (further + // down) would otherwise re-demote into the suppressed + // `ContextWindowExceeded` bucket (CodeRabbit). Gated on the managed envelope + // so a BYO provider's own context-overflow — genuine user-state, not our + // guard — still flows to that matcher and stays demoted. + if crate::openhuman::inference::provider::is_managed_backend_envelope(message) + && crate::openhuman::inference::provider::is_backend_client_guard_leak(message) + { + return None; + } // Check the Codex-CLI import envelope first: it is highly specific // (literal `codex cli auth` / `.codex/auth.json`) and carries no overlap // with the generic matchers below, so ordering is for clarity, not @@ -5330,8 +5345,6 @@ mod tests { ("402", "USER_INSUFFICIENT_CREDITS"), ("503", "UPSTREAM_UNAVAILABLE"), ("404", "MODEL_UNAVAILABLE"), - ("413", "PAYLOAD_TOO_LARGE"), - ("400", "CONTEXT_LENGTH_EXCEEDED"), ("400", "BAD_REQUEST"), ("500", "INTERNAL_ERROR"), ] { @@ -5342,6 +5355,37 @@ mod tests { "errorCode={code} must be backend-owned (no FE Sentry)" ); } + + // Client-guard-leak codes page (None = capture), even with realistic + // explanatory text that a later substring matcher would otherwise + // re-demote into a suppressed bucket. + let payload = "OpenHuman API error (413 Payload Too Large): \ + {\"error\":{\"errorCode\":\"PAYLOAD_TOO_LARGE\",\"message\":\"request entity too large\"}}"; + assert_eq!( + expected_error_kind(payload), + None, + "PAYLOAD_TOO_LARGE is a client guard leak and must page" + ); + + // This body's message matches `is_context_window_exceeded_message`, so + // without the early guard-leak bypass it would re-demote to the + // suppressed `ContextWindowExceeded` bucket (CodeRabbit). + let context = "OpenHuman API error (400 Bad Request): \ + {\"error\":{\"errorCode\":\"CONTEXT_LENGTH_EXCEEDED\",\"message\":\"This model's maximum context length is 128000 tokens, however you requested more\"}}"; + assert_eq!( + expected_error_kind(context), + None, + "CONTEXT_LENGTH_EXCEEDED must page even with context-window wording" + ); + + // Proof the bypass is load-bearing, not a no-op: this body's text DOES + // match the context-window matcher, so without the early guard-leak + // bypass `expected_error_kind` would have re-demoted it to the + // suppressed `ContextWindowExceeded` bucket. + assert!( + crate::openhuman::inference::provider::is_context_window_exceeded_message(context), + "test body must actually trigger the matcher the bypass guards against" + ); } #[test] @@ -5390,7 +5434,6 @@ mod tests { "RATE_LIMITED", "UPSTREAM_UNAVAILABLE", "MODEL_UNAVAILABLE", - "PAYLOAD_TOO_LARGE", "INTERNAL_ERROR", "BAD_REQUEST", ] { @@ -5400,6 +5443,22 @@ mod tests { "errorCode={code} event must be dropped by before_send" ); } + + // Client-guard-leak codes survive before_send and page (the client + // should have caught the limit before sending) — including a realistic + // CONTEXT_LENGTH_EXCEEDED body whose wording matches the context-window + // substring matcher (the filter keys on the errorCode, not the text). + let payload = "OpenHuman API error (413 Payload Too Large): \ + {\"error\":{\"errorCode\":\"PAYLOAD_TOO_LARGE\",\"message\":\"request entity too large\"}}"; + let context = "OpenHuman API error (400 Bad Request): \ + {\"error\":{\"errorCode\":\"CONTEXT_LENGTH_EXCEEDED\",\"message\":\"This model's maximum context length is 128000 tokens\"}}"; + for body in [payload, context] { + let event = event_with_message(body); + assert!( + !is_backend_error_code_event(&event), + "client guard leak must survive before_send: {body}" + ); + } } #[test] diff --git a/src/openhuman/inference/provider/compatible_stream_native.rs b/src/openhuman/inference/provider/compatible_stream_native.rs index ebcbcb701..3e6e828ab 100644 --- a/src/openhuman/inference/provider/compatible_stream_native.rs +++ b/src/openhuman/inference/provider/compatible_stream_native.rs @@ -171,6 +171,24 @@ impl OpenAiCompatibleProvider { while let Some(sep_idx) = buffer.find("\n\n") { let event = buffer[..sep_idx].to_string(); buffer.drain(..sep_idx + 2); + + // In-band SSE error frame. The response status flushed 200 + // before the upstream call, so the managed backend delivers + // post-flush errors — including instant 4xx/429/413 whose typed + // `errorCode` (#870) is now stamped on the frame, see + // backend `routes/inference.ts` — as `event: error\ndata: {…}`. + // Surface it as a streaming-envelope error string so the + // `errorCode` classifier + Sentry golden rule run downstream, + // instead of skipping it as an unparseable chunk (which would + // aggregate to empty and surface as "empty response"). + if let Some(message) = sse_error_frame_bail_message( + self.name.as_str(), + Some(native_request.model.as_str()), + &event, + ) { + anyhow::bail!(message); + } + for line in event.lines() { let line = line.trim(); if line.is_empty() || line.starts_with(':') { @@ -462,3 +480,124 @@ impl OpenAiCompatibleProvider { Self::parse_native_response(api_resp, &self.name) } } + +/// Extract the `data:` payload of an SSE `event: error` frame, or `None` when +/// the event block is not an error frame. +/// +/// The managed backend delivers post-flush stream errors as a two-line SSE +/// block — `event: error` followed by `data: {"error":{…,"errorCode":…}}` +/// (backend `routes/inference.ts`). The normal chunk loop can't parse that +/// `data:` line as a `StreamChunkResponse`, so without this detection the frame +/// is silently skipped and the turn aggregates to an empty response. We key on +/// the `event: error` field rather than sniffing the data so a normal chunk +/// that merely mentions "error" is never misclassified. +fn sse_error_frame_payload(event: &str) -> Option { + let mut is_error_frame = false; + let mut data: Option = None; + for line in event.lines() { + let line = line.trim(); + if let Some(event_type) = line.strip_prefix("event:") { + if event_type.trim() == "error" { + is_error_frame = true; + } + } else if let Some(payload) = line.strip_prefix("data:") { + data = Some(payload.trim().to_string()); + } + } + is_error_frame.then(|| data.unwrap_or_default()) +} + +/// Build the bail message for an in-band SSE `event: error` frame, or `None` +/// when the event block is a normal chunk / metadata event (the caller then +/// continues parsing it as a chunk). +/// +/// Mirrors the non-2xx HTTP path: produces a ` streaming API error: +/// ` envelope so the downstream `errorCode` classifier and the +/// Sentry golden rule run on it. When the frame carries a backend-owned +/// `errorCode`, it is logged (not re-reported) here, matching the HTTP path's +/// [`is_backend_error_code_owned`] branch; a malformed `BAD_REQUEST` is excluded +/// by that helper and still pages downstream. There is no HTTP status for an +/// in-band frame, so the log records the `200` that was actually sent. +fn sse_error_frame_bail_message( + provider: &str, + model: Option<&str>, + event: &str, +) -> Option { + let payload = sse_error_frame_payload(event)?; + let sanitized = super::super::sanitize_api_error(&payload); + let message = format!("{provider} streaming API error: {sanitized}"); + if super::super::is_backend_error_code_owned(provider, &payload) { + super::super::log_backend_error_code_owned( + "streaming_chat", + provider, + model, + reqwest::StatusCode::OK, + &payload, + ); + } + Some(message) +} + +#[cfg(test)] +mod sse_error_frame_tests { + use super::{sse_error_frame_bail_message, sse_error_frame_payload}; + use crate::openhuman::inference::provider::openhuman_backend::PROVIDER_LABEL; + + #[test] + fn extracts_payload_from_error_frame() { + let event = "event: error\ndata: {\"error\":{\"message\":\"boom\",\"type\":\"stream_error\",\"errorCode\":\"BAD_REQUEST\"}}"; + let payload = sse_error_frame_payload(event).expect("error frame detected"); + assert!(payload.contains("\"errorCode\":\"BAD_REQUEST\"")); + assert!(payload.contains("\"type\":\"stream_error\"")); + } + + #[test] + fn ignores_normal_data_chunk() { + let event = + "data: {\"choices\":[{\"delta\":{\"content\":\"an error occurred in the story\"}}]}"; + assert!(sse_error_frame_payload(event).is_none()); + } + + #[test] + fn ignores_metadata_event() { + let event = "event: openhuman-metadata\ndata: {\"openhuman\":{}}"; + assert!(sse_error_frame_payload(event).is_none()); + } + + #[test] + fn bail_message_wraps_error_frame_in_streaming_envelope() { + // A managed-backend error frame yields a streaming-envelope string the + // downstream classifier recognises, preserving the `errorCode`. + let event = "event: error\ndata: {\"error\":{\"message\":\"slow down\",\"type\":\"stream_error\",\"errorCode\":\"RATE_LIMITED\"}}"; + let message = sse_error_frame_bail_message(PROVIDER_LABEL, Some("reasoning-v1"), event) + .expect("bail message built"); + assert!(message.contains("streaming API error")); + assert!(message.contains("\"errorCode\":\"RATE_LIMITED\"")); + } + + #[test] + fn bail_message_handles_frame_without_error_code() { + // An untyped mid-stream drop (no `errorCode`) still produces an + // envelope; it is simply not backend-owned, so downstream Sentry gating + // applies as before. + let event = + "event: error\ndata: {\"error\":{\"message\":\"socket hang up\",\"type\":\"stream_error\"}}"; + let message = + sse_error_frame_bail_message(PROVIDER_LABEL, None, event).expect("bail message built"); + assert!(message.contains("socket hang up")); + } + + #[test] + fn bail_message_is_none_for_normal_chunk() { + let event = "data: {\"choices\":[{\"delta\":{\"content\":\"hi\"}}]}"; + assert!(sse_error_frame_bail_message(PROVIDER_LABEL, None, event).is_none()); + } + + #[test] + fn error_frame_without_data_yields_empty_payload() { + // Defensive: an `event: error` with no `data:` line still resolves to a + // (empty) payload so the caller bails rather than skipping it. + let event = "event: error"; + assert_eq!(sse_error_frame_payload(event).as_deref(), Some("")); + } +} diff --git a/src/openhuman/inference/provider/error_code.rs b/src/openhuman/inference/provider/error_code.rs index 51f40ffd9..f4e55dab9 100644 --- a/src/openhuman/inference/provider/error_code.rs +++ b/src/openhuman/inference/provider/error_code.rs @@ -175,16 +175,44 @@ pub fn is_backend_malformed_bad_request(err: &str) -> bool { ) && body_flags_malformed(err) } +/// Whether the `errorCode` names a limit the **client enforces before sending**, +/// so a backend rejection means our pre-send guard leaked — a client-side bug +/// worth paging, not expected user-state. +/// +/// - `PAYLOAD_TOO_LARGE`: the client gates attachment size up front +/// (`app/src/lib/attachments.ts` — per-image / per-file byte caps + a +/// `too_large` reject), so an over-limit request reaching the backend means +/// the aggregate slipped past those gates. +/// - `CONTEXT_LENGTH_EXCEEDED`: the client manages context before send (the +/// context pipeline's `context_window`, `src/openhuman/context/pipeline.rs`), +/// so a backend rejection means that fitting / trimming failed. +/// +/// The backend does not ops-alert either (they are 4xx, not 500), so if the FE +/// also suppressed them the guard leak would be invisible to everyone. Display +/// classification is unchanged — the user still sees the actionable copy. +pub fn is_backend_client_guard_leak(err: &str) -> bool { + matches!( + extract_backend_error_code(err), + Some(BackendErrorCode::PayloadTooLarge | BackendErrorCode::ContextLengthExceeded) + ) +} + /// Sentry-ownership decision (F2 golden rule): a response carrying any backend /// `errorCode` must **not** page the FE — the backend owns it (it already -/// paged) or it is expected user-state — *except* a backend-flagged malformed -/// `BAD_REQUEST`, which the client caused and so still pages. +/// paged) or it is expected user-state — *except* errors the **client** caused +/// and so still page: +/// - a backend-flagged malformed `BAD_REQUEST` (unparseable client payload), and +/// - a client-guard-leak code (`PAYLOAD_TOO_LARGE` / `CONTEXT_LENGTH_EXCEEDED`) +/// the client should have caught before sending — see +/// [`is_backend_client_guard_leak`]. /// /// Shared by the provider HTTP layer (`api_error`), the higher-layer re-report /// classifier (`observability::expected_error_kind`), and the Sentry /// `before_send` defense-in-depth filter so the three layers can't drift. pub fn backend_error_code_skips_sentry(err: &str) -> bool { - extract_backend_error_code_token(err).is_some() && !is_backend_malformed_bad_request(err) + extract_backend_error_code_token(err).is_some() + && !is_backend_malformed_bad_request(err) + && !is_backend_client_guard_leak(err) } #[cfg(test)] @@ -249,6 +277,29 @@ mod tests { assert!(backend_error_code_skips_sentry(user_param)); } + #[test] + fn client_guard_leak_codes_page_but_other_state_codes_do_not() { + // PAYLOAD_TOO_LARGE / CONTEXT_LENGTH_EXCEEDED are limits the client + // enforces before sending, so a backend rejection is a guard leak that + // must page the FE — unlike genuinely backend-owned / user-state codes. + let payload = r#"OpenHuman API error (413 Payload Too Large): {"error":{"errorCode":"PAYLOAD_TOO_LARGE","message":"too big"}}"#; + assert!(is_backend_client_guard_leak(payload)); + assert!(!backend_error_code_skips_sentry(payload)); + assert!(!managed_error_skips_sentry(payload)); + + let context = r#"OpenHuman API error (400 Bad Request): {"error":{"errorCode":"CONTEXT_LENGTH_EXCEEDED","message":"start a new chat"}}"#; + assert!(is_backend_client_guard_leak(context)); + assert!(!backend_error_code_skips_sentry(context)); + + // Contrast: these remain backend-owned / expected user-state -> suppress. + let rate = r#"OpenHuman API error (429): {"error":{"errorCode":"RATE_LIMITED"}}"#; + let credits = + r#"OpenHuman API error (402): {"error":{"errorCode":"USER_INSUFFICIENT_CREDITS"}}"#; + assert!(!is_backend_client_guard_leak(rate)); + assert!(backend_error_code_skips_sentry(rate)); + assert!(backend_error_code_skips_sentry(credits)); + } + #[test] fn malformed_flag_without_bad_request_is_ignored() { // A stray `malformed` flag on a non-BAD_REQUEST code must not turn a diff --git a/src/openhuman/inference/provider/mod.rs b/src/openhuman/inference/provider/mod.rs index c05958c6d..530a75150 100644 --- a/src/openhuman/inference/provider/mod.rs +++ b/src/openhuman/inference/provider/mod.rs @@ -37,8 +37,9 @@ pub use config_rejection::{ }; pub use error_code::{ backend_error_code_skips_sentry, body_flags_malformed, extract_backend_error_code, - extract_backend_error_code_token, is_backend_malformed_bad_request, - is_managed_backend_envelope, managed_error_skips_sentry, BackendErrorCode, + extract_backend_error_code_token, is_backend_client_guard_leak, + is_backend_malformed_bad_request, is_managed_backend_envelope, managed_error_skips_sentry, + BackendErrorCode, }; pub use factory::{create_chat_provider, provider_for_role, BYOK_INCOMPLETE_SENTINEL}; pub use ops::*; diff --git a/src/openhuman/inference/provider/ops_tests.rs b/src/openhuman/inference/provider/ops_tests.rs index 35a121061..527c42fce 100644 --- a/src/openhuman/inference/provider/ops_tests.rs +++ b/src/openhuman/inference/provider/ops_tests.rs @@ -338,15 +338,13 @@ fn skips_sentry_report_for_transient_upstream_statuses() { fn backend_error_code_owned_gates_managed_errors_except_malformed_bad_request() { use crate::openhuman::inference::provider::openhuman_backend::PROVIDER_LABEL; - // F2/F4: any managed-backend body carrying an errorCode is backend-owned - // and must NOT page the provider HTTP layer. + // F2/F4: backend-owned / expected-user-state errorCodes must NOT page the + // provider HTTP layer. for code in [ "RATE_LIMITED", "USER_INSUFFICIENT_CREDITS", "UPSTREAM_UNAVAILABLE", "MODEL_UNAVAILABLE", - "PAYLOAD_TOO_LARGE", - "CONTEXT_LENGTH_EXCEEDED", "INTERNAL_ERROR", ] { let body = format!("{{\"error\":{{\"errorCode\":\"{code}\",\"message\":\"x\"}}}}"); @@ -363,7 +361,18 @@ fn backend_error_code_owned_gates_managed_errors_except_malformed_bad_request() "{\"error\":{\"errorCode\":\"BAD_REQUEST\",\"message\":\"bad param\"}}" )); - // F8: a backend-flagged malformed BAD_REQUEST is the one case the FE still + // Client-guard-leak codes page: the client enforces these limits before + // sending (attachment size gates; context-window management), so a backend + // rejection means our guard leaked — the gate must NOT claim them. + for code in ["PAYLOAD_TOO_LARGE", "CONTEXT_LENGTH_EXCEEDED"] { + let body = format!("{{\"error\":{{\"errorCode\":\"{code}\",\"message\":\"x\"}}}}"); + assert!( + !is_backend_error_code_owned(PROVIDER_LABEL, &body), + "errorCode={code} is a client guard leak and must page (not owned)" + ); + } + + // F8: a backend-flagged malformed BAD_REQUEST is also a case the FE still // pages — the gate must NOT claim it. assert!(!is_backend_error_code_owned( PROVIDER_LABEL,