fix(providers): drop budget-exhausted 400s from Sentry (#3M, #12, #13)#1633
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds deterministic detection of provider “budget exhausted” messages, classifies such responses as ExpectedErrorKind::BudgetExhausted, suppresses/logs them at provider and REST/agent layers, and drops matching Sentry events via before_send (with unit and smoke tests). ChangesBudget-exhaustion detection and Sentry filtering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/openhuman/agent/harness/session/runtime.rs (1)
505-527: ⚡ Quick winNarrow
run_singlebudget suppression to provider-originated errors.Using phrase match alone here can suppress unrelated failures if their text contains budget phrases.
🎯 Suggested guard
let sanitized_message = Self::sanitize_event_error_message(&err); let full_error = format!("{err:#}"); - if providers::is_budget_exhausted_message(&full_error) { + let is_provider_budget = sanitized_message == "provider_error" + && providers::is_budget_exhausted_message(&full_error); + if is_provider_budget {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/harness/session/runtime.rs` around lines 505 - 527, The budget-suppression currently uses providers::is_budget_exhausted_message on the formatted message (full_error) and can wrongly suppress non-provider errors; update the guard in run_single to first detect that the error originates from a provider error (e.g., inspect err's source chain or downcast to the provider error type) and only then call providers::is_budget_exhausted_message/full_error to decide suppression; adjust the if condition so it checks both "is_provider_error(&err)" (or an equivalent downcast/chain check) and providers::is_budget_exhausted_message(&full_error) before skipping the report_error call, leaving report_error(&err, "agent", "run_single", ...) unchanged for all other errors.src/core/observability.rs (1)
215-223: ⚡ Quick winScope budget-event filtering to known domains.
is_budget_eventcurrently ignoresdomain, so any event withfailure=non_2xx,status=400, and matching text can be dropped even outside provider/backend paths.🔧 Proposed tightening
pub fn is_budget_event(event: &sentry::protocol::Event<'_>) -> bool { let tags = &event.tags; + if !matches!( + tags.get("domain").map(String::as_str), + Some("llm_provider") | Some("backend_api") + ) { + return false; + } if tags.get("failure").map(String::as_str) != Some("non_2xx") { return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/observability.rs` around lines 215 - 223, is_budget_event currently drops events solely by failure/status/message; restrict it to known domains by checking the event.tags "domain" value (e.g., only accept "provider" or "backend" or whichever canonical domain strings your system uses) before returning true. Update the is_budget_event function to read tags.get("domain").map(String::as_str) and ensure it matches the allowlist of domains in addition to the existing failure=="non_2xx", status=="400", and event_contains_budget_exhausted_message checks so only provider/backend-path events are filtered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src-tauri/src/lib.rs`:
- Around line 1254-1257: The current budget-drop logging uses log::debug and
prints the full event.message (event.message.as_deref()), which may leak
sensitive text; change the log call in the budget-exhausted path to avoid
logging raw event.message and instead log only non-sensitive metadata such as
event.level, event.event_id or id, timestamp, and any non-PII tags/keys (e.g.
use event.level, event.id/event.event_id, event.timestamp, and a boolean
indicating whether message exists). Update the log::debug invocation in the same
block to construct a metadata-only message (no event.message content) and, if
needed, use event.message.is_some() to indicate presence without including the
text.
In `@src/openhuman/providers/compatible.rs`:
- Around line 738-745: When detecting a deterministic budget-400 in the
streaming path (after is_budget_exhausted_http_400 and
log_budget_exhausted_http_400), return a specific sentinel/typed error from
stream_native_chat (e.g., a ProviderError::BudgetExhausted or similar new
variant) instead of the current generic Err so the caller chat() can detect it
and short-circuit the non-streaming fallback; update stream_native_chat to
produce that typed error and ensure chat() checks for that sentinel and stops
retrying/does not invoke the non-streaming fallback, preventing duplicate logs
and duplicate provider calls.
---
Nitpick comments:
In `@src/core/observability.rs`:
- Around line 215-223: is_budget_event currently drops events solely by
failure/status/message; restrict it to known domains by checking the event.tags
"domain" value (e.g., only accept "provider" or "backend" or whichever canonical
domain strings your system uses) before returning true. Update the
is_budget_event function to read tags.get("domain").map(String::as_str) and
ensure it matches the allowlist of domains in addition to the existing
failure=="non_2xx", status=="400", and event_contains_budget_exhausted_message
checks so only provider/backend-path events are filtered.
In `@src/openhuman/agent/harness/session/runtime.rs`:
- Around line 505-527: The budget-suppression currently uses
providers::is_budget_exhausted_message on the formatted message (full_error) and
can wrongly suppress non-provider errors; update the guard in run_single to
first detect that the error originates from a provider error (e.g., inspect
err's source chain or downcast to the provider error type) and only then call
providers::is_budget_exhausted_message/full_error to decide suppression; adjust
the if condition so it checks both "is_provider_error(&err)" (or an equivalent
downcast/chain check) and providers::is_budget_exhausted_message(&full_error)
before skipping the report_error call, leaving report_error(&err, "agent",
"run_single", ...) unchanged for all other errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de8baa8e-d856-4b47-8c03-b1afc56d1f28
📒 Files selected for processing (10)
app/src-tauri/src/lib.rssrc/api/rest.rssrc/core/observability.rssrc/main.rssrc/openhuman/agent/harness/session/runtime.rssrc/openhuman/providers/billing_error.rssrc/openhuman/providers/compatible.rssrc/openhuman/providers/mod.rssrc/openhuman/providers/ops.rstests/observability_smoke.rs
| if super::is_budget_exhausted_http_400(status, &body) { | ||
| super::log_budget_exhausted_http_400( | ||
| "streaming_chat", | ||
| self.name.as_str(), | ||
| Some(native_request.model.as_str()), | ||
| status, | ||
| ); | ||
| } else if super::should_report_provider_http_failure(status) { |
There was a problem hiding this comment.
Skip the non-streaming fallback for deterministic budget 400s.
This branch now knows the failure is a terminal user-state error, but stream_native_chat still returns a generic Err, so chat() falls through to the non-streaming retry on Line 1485. That adds a second doomed provider call and duplicates the budget log for every streaming budget miss. Please bubble up a typed/sentinel error here so the caller can short-circuit instead of retrying.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/providers/compatible.rs` around lines 738 - 745, When detecting
a deterministic budget-400 in the streaming path (after
is_budget_exhausted_http_400 and log_budget_exhausted_http_400), return a
specific sentinel/typed error from stream_native_chat (e.g., a
ProviderError::BudgetExhausted or similar new variant) instead of the current
generic Err so the caller chat() can detect it and short-circuit the
non-streaming fallback; update stream_native_chat to produce that typed error
and ensure chat() checks for that sentinel and stops retrying/does not invoke
the non-streaming fallback, preventing duplicate logs and duplicate provider
calls.
There was a problem hiding this comment.
Acknowledged — this is a valid follow-up but out of scope for the noise-drop PR. The typed sentinel error would require a provider-error taxonomy refactor (ProviderError::BudgetExhausted variant + every chat()-consumer learning to short-circuit). I'd rather ship this Sentry-drop now (which already prevents the second event from reaching Sentry via before_send) and open a follow-up for the streaming-fallback short-circuit. Tracking as TODO.
There was a problem hiding this comment.
@oxoxDev, understood — the rationale is sound. The before_send defense-in-depth already prevents the second call's event from reaching Sentry, so the duplicate-log risk is bounded to structured tracing::info! only (not a Sentry noise problem). The full ProviderError::BudgetExhausted refactor is a meaningful taxonomy change that deserves its own PR.
Would you like me to open a follow-up GitHub issue to track the streaming-fallback short-circuit work (ProviderError::BudgetExhausted typed error + chat() short-circuit)?
🧠 Learnings used
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When using the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, don’t require individual execution-provider feature flags (e.g., `directml`, `coreml`, `cuda`) alongside `load-dynamic` to get EP registration code. The `ort` crate already compiles EP registration via `#[cfg(any(feature = "load-dynamic", feature = "<ep_name>"))]` guards, and adding per-EP feature flags can pull in static-linking dependencies that conflict with the dynamic loading approach. At runtime, EP availability is determined by what the dynamically loaded ONNX Runtime library (`onnxruntime.dll`/`.so`/`.dylib`) supports; ort docs indicate providers like `directml`/`xnnpack`/`coreml` are available in builds when the platform supports them.
Learnt from: sanil-23
Repo: tinyhumansai/openhuman PR: 416
File: src/openhuman/memory/relex.rs:441-464
Timestamp: 2026-04-07T15:49:51.275Z
Learning: When integrating the `ort` Rust crate v2.x with the `load-dynamic` feature enabled, do NOT also require/enable individual provider EP Cargo features like `directml`, `coreml`, or `cuda`. In `ort` v2.x, EP registration for providers (e.g., DirectML, CoreML, CUDA, etc.) is already compiled in under source-level `#[cfg(any(feature = "load-dynamic", feature = "<provider>"))]` guards, such as in `ep/directml.rs`. Adding provider feature flags alongside `load-dynamic` can pull in static-linking dependencies that conflict with the dynamic-loading approach. Provider availability should be treated as runtime-determined by what the loaded `onnxruntime` library (`onnxruntime.dll`/`libonnxruntime.so`/`libonnxruntime.dylib`) actually supports.
Learnt from: oxoxDev
Repo: tinyhumansai/openhuman PR: 571
File: src/openhuman/local_ai/service/whisper_engine.rs:69-80
Timestamp: 2026-04-14T19:59:04.826Z
Learning: When reviewing Rust code in this repo that uses the upstream `whisper-rs` crate (v0.16.0), do not report `WhisperContextParameters::use_gpu(...)` or `WhisperContextParameters::flash_attn(...)` as missing/invalid APIs. These builder-style methods exist upstream and return `&mut Self`; they are not limited to `WhisperVadContextParams`.
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 1078
File: src/openhuman/agent/agents/welcome/prompt.rs:24-24
Timestamp: 2026-05-01T13:41:00.958Z
Learning: For Rust code under `src/openhuman/**/*.rs`, use `snake_case` for local variables (not `camelCase`). If a local variable name is written in `camelCase`, treat it as a style/lint issue because it will trigger Rust’s `non_snake_case` warning (and related clippy linting, if enabled). Avoid suggesting `camelCase` for any Rust local variable names in this repository.
Learnt from: senamakel
Repo: tinyhumansai/openhuman PR: 1173
File: tests/agent_memory_loader_public.rs:88-88
Timestamp: 2026-05-04T06:50:47.877Z
Learning: In this repository, the general camelCase naming guideline should not be applied to Rust source files. For all .rs files, Rust function (and related) names should use snake_case, and snake_case Rust function names should not be flagged—even for async test functions annotated with attributes like #[tokio::test]. This is consistent with Rust’s non_snake_case lint behavior.
tinyhumansai#1633 CR) CodeRabbit major: the [sentry-budget-filter] log emitted event.message verbatim, which can carry upstream provider error text including tokens / pasted-through secrets. Per openhuman/CLAUDE.md "never log secrets or full PII". Switch to logging only the structured tag values (domain + status) that gate the drop decision; the message text is no longer needed for diagnostic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CR Round 1 addressed:
Replied inline on the |
|
@oxoxDev this PR has merge conflicts with main — please rebase/resolve before review. |
|
Deferred question for backend team — ops.rs 402 suppression In The rationale in the current implementation is that 402 Payment Required carries a different semantic: it could signal the backend adding a hard payment gate rather than a soft budget-exceeded user state, so it should still reach Sentry as a signal. Question for backend team: Is this the correct policy?
Please confirm which applies before this is merged. Tagging as unresolved. |
tinyhumansai#1633 CR) CodeRabbit major: the [sentry-budget-filter] log emitted event.message verbatim, which can carry upstream provider error text including tokens / pasted-through secrets. Per openhuman/CLAUDE.md "never log secrets or full PII". Switch to logging only the structured tag values (domain + status) that gate the drop decision; the message text is no longer needed for diagnostic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Defaulting to 400-only as the PR currently has it. Rationale: every observed Sentry event in the cluster (OPENHUMAN-TAURI-3M ×72, -12 ×49, -13 ×13) returned HTTP 400, never 402. Suppressing 402 here would silence a future signal — if the backend introduces a hard-payment-wall on 402 (subscription lapsed, plan downgrade, etc.), we want that to surface as actionable rather than be lumped with soft budget-exceeded. The smoke test So: status quo is the right default. Happy to widen in a follow-up PR if @backend-team confirms 402 will be used for soft-budget. Marking your question as answered — leaving the test name as a tripwire for the future-widening case. Conflict resolution also pushed — rebased onto current main. |
|
@graycyrus rebased onto current main (now on commit d5807e3, rebased from 2b64ea8 upstream tip). Conflict resolution:
Local checks green:
|
PR Manager follow-up (2026-05-13)Quality suite run against the conflict-resolved state (local
Deferred CR nits applied (from the original review):
Outstanding items for the author:
Items 4 and 5 are non-trivial architecture choices (new error variant) and deferred for the author. |
…mansai#1633) Conflict resolution (5 files): - src/core/observability.rs: kept both BudgetExhausted and LocalAiCapabilityUnavailable variants; merged is_budget_event, is_transient_backend_http_status, and new transient-integrations helper - src/main.rs: merged both before_send budget-drop hooks - src/api/rest.rs: unified to is_transient_http_status_code (generalised) - app/src-tauri/src/lib.rs: merged both budget-drop before_send hooks - tests/observability_smoke.rs: combined all smoke tests (9 total) CR nits applied: - is_budget_exhausted_http_400: doc comment explaining 402 intentionally not suppressed - is_budget_event: doc comment clarifying domain tag intentionally omitted - log_budget_exhausted_http_400: model.unwrap_or("") → unwrap_or("<unknown>") All checks pass: cargo fmt, cargo check (core + tauri), cargo test (observability_smoke: 9/9, providers::ops: 9/9), pnpm typecheck, pnpm lint, pnpm format.
tinyhumansai#1633 CR) CodeRabbit major: the [sentry-budget-filter] log emitted event.message verbatim, which can carry upstream provider error text including tokens / pasted-through secrets. Per openhuman/CLAUDE.md "never log secrets or full PII". Switch to logging only the structured tag values (domain + status) that gate the drop decision; the message text is no longer needed for diagnostic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed PR Manager follow-up (2026-05-14):
|
|
Re: CodeRabbit summary nitpicks — 1. The pre-rebase shape (direct 2. Intentional defense-in-depth — see the function's doc-comment at
The |
…nsai#12, tinyhumansai#13) (tinyhumansai#1633) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 participants
Summary
errorevents to structuredinfologs while preservingErrpropagation.before_sendfiltering in both the core binary and Tauri shell.Problem
400 Bad Requestwith budget/credits/balance copy.Solution
is_budget_exhausted_messagewith a deliberately small case-insensitive phrase list:insufficient budget,budget exceeded,add credits,insufficient balance.infowithkind=budgetand skipreport_error.is_budget_eventforbefore_send, keyed tofailure=non_2xx,status=400, and budget copy in the event message/exception.src/main.rsand shellapp/src-tauri/src/lib.rs.Err; only Sentry emission changes.Submission Checklist
## Related— N/A: no matrix feature IDs; Sentry IDs listed under## Related.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release-cut surface/manual smoke checklist change.Closes #NNNin the## RelatedsectionImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/budget-noise1b8b9a39039b33ab93c06cc4aa8ff37a87a0707eValidation Run
pnpm --filter openhuman-app format:check— N/A: no TS/frontend formatting changes in this PR.pnpm typecheck— N/A: no TS/frontend type changes in this PR.cargo test --lib openhuman::providerscargo test --test observability_smokecargo fmt --checkcargo check --manifest-path Cargo.tomlcargo clippy --workspace -- -D warnings(blocked; see below)cargo check --manifest-path app/src-tauri/Cargo.tomlValidation Blocked
command:cargo clippy --workspace -- -D warningserror:Fails on existing workspace lint debt (78 errors across unrelated modules: unused imports, deprecatedrand::thread_rng, derivable impls, type complexity, doc list indentation, etc.).impact:Clippy is not green in this worktree; budget-specific checks and smoke tests pass.Behavior Changes
Parity Contract
Err; retry/toast callers continue to observe failure.all_exhaustedsmoke still captures one event.Duplicate / Superseded PR Handling
Summary by CodeRabbit