fix(inference): publish SessionExpired for backend 401 on chat_completions (Sentry TAURI-RUST-N)#2814
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds OpenHuman-backend-specific 401/403 detection and a publisher that emits sanitized ChangesBackend Auth Failure Detection and Event Publishing
Sequence Diagram(s)sequenceDiagram
participant chat_with_system
participant is_backend_auth_failure
participant publish_backend_session_expired
participant DomainEventBus
chat_with_system->>is_backend_auth_failure: check(provider, status)
is_backend_auth_failure-->>chat_with_system: true/false
alt true
chat_with_system->>publish_backend_session_expired: publish("chat_completions", provider, status, message)
publish_backend_session_expired->>DomainEventBus: emit DomainEvent::SessionExpired(sanitized_reason, source="openhuman:provider")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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: 1
🧹 Nitpick comments (1)
src/openhuman/inference/provider/ops.rs (1)
557-589: ⚡ Quick winRoute
api_errorthrough this helper too.Line 633 still inlines the same
SessionExpiredpublish path instead of calling this helper. That leaves the backend-auth handling defined twice inops.rs, which is the same kind of drift that caused this regression.♻️ Suggested dedupe
- if is_auth_failure && is_backend { - tracing::warn!( - domain = "llm_provider", - operation = "api_error", - provider = provider, - status = status_str.as_str(), - "[llm_provider] backend auth failure ({status}) — publishing SessionExpired" - ); - // `message` already embeds the sanitized body via - // `sanitize_api_error(&body)`, but the leading `{provider} API - // error ({status})` prefix and any caller-controlled provider - // name aren't scrubbed — re-run sanitize on the final string so - // the SessionExpired subscriber's logs never persist secrets. - crate::core::event_bus::publish_global( - crate::core::event_bus::DomainEvent::SessionExpired { - source: "llm_provider.openhuman_backend".to_string(), - reason: sanitize_api_error(&message), - }, - ); + if is_auth_failure && is_backend { + publish_backend_session_expired("api_error", provider, status, &message);🤖 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/inference/provider/ops.rs` around lines 557 - 589, The inline SessionExpired publish branch that's duplicated in the api_error path should be replaced by a call to the existing helper publish_backend_session_expired; locate the duplicated logic in the api_error flow (the code that constructs DomainEvent::SessionExpired, calls sanitize_api_error, and publishes) and remove it, then call publish_backend_session_expired(operation, provider, status, &message) instead so backend auth failures are handled in one place; keep the same arguments (operation, provider, status, message) and ensure sanitize_api_error is only applied inside the helper as currently implemented.
🤖 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 `@src/openhuman/inference/provider/ops.rs`:
- Around line 1567-1602: The test
publish_backend_session_expired_emits_sanitized_session_expired is racing with
the sibling test and doesn't assert redaction; update both tests (the one at
publish_backend_session_expired_emits_sanitized_session_expired and the sibling
in the 1609-1677 range) to use distinct marker strings in their mock 401 bodies
(e.g., "TEST_MARKER_A" vs "TEST_MARKER_B"), include a redactable secret/token
substring in the msg passed to publish_backend_session_expired, and then assert
that the received DomainEvent::SessionExpired has the expected unique marker in
reason but does NOT contain the raw secret/token (verifying sanitize_api_error
actually redacted it); locate and modify the test bodies and assertions
referencing publish_backend_session_expired and sanitize_api_error to implement
these unique markers and redaction checks.
---
Nitpick comments:
In `@src/openhuman/inference/provider/ops.rs`:
- Around line 557-589: The inline SessionExpired publish branch that's
duplicated in the api_error path should be replaced by a call to the existing
helper publish_backend_session_expired; locate the duplicated logic in the
api_error flow (the code that constructs DomainEvent::SessionExpired, calls
sanitize_api_error, and publishes) and remove it, then call
publish_backend_session_expired(operation, provider, status, &message) instead
so backend auth failures are handled in one place; keep the same arguments
(operation, provider, status, message) and ensure sanitize_api_error is only
applied inside the helper as currently implemented.
🪄 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: 4d2372e1-4436-4140-8fac-b27cbd88c7b4
📒 Files selected for processing (2)
src/openhuman/inference/provider/compatible.rssrc/openhuman/inference/provider/ops.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@CodeGhost21 hey! the code looks good to me — clean, well-scoped fix. the is_backend_auth_failure guard is correctly provider-scoped so BYO-key 401/403 keep hitting Sentry, the branch slots in before the rest of the error chain and lets bail!(message) still propagate, and the three-layer test coverage (polarity unit, publish unit, e2e axum mock) is solid.
one thing CodeRabbit didn't flag: the three sibling chains (native_chat, streaming_chat, responses_api) still have the same exposure. the PR calls them out as follow-up, which is fine for scoping, but worth filing a tracking issue so they don't go stale.
there are some CI failures (Build & smoke-test core image) and open CodeRabbit findings (the api_error dedup + test isolation) that need to be resolved first. once those are green and addressed, i'll come back and approve this. let me know if you need any help!
coderabbitai
Bot
added
rust-core
|
Actionable comments posted: 0 |
|
Actionable comments posted: 0 |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #2814 — fix(inference): publish SessionExpired for backend 401 on chat_completions (Sentry TAURI-RUST-N)
Walkthrough
Clean, well-scoped fix for TAURI-RUST-N (332 Sentry events from backend session-expiry 401s hitting the chat_completions path). The root cause — chat_with_system's hand-rolled error chain lacked the backend-auth guard that api_error already had — is fixed by extracting is_backend_auth_failure + publish_backend_session_expired helpers and wiring them into compatible.rs. The refactoring of api_error to use the shared helper is behavior-preserving. Test coverage is solid.
Changes
| File | Summary |
|---|---|
src/openhuman/inference/provider/ops.rs |
Extract is_backend_auth_failure + publish_backend_session_expired helpers; refactor api_error to delegate; add 3 tests |
src/openhuman/inference/provider/compatible.rs |
Wire backend-auth guard into chat_with_system's error chain as the first branch |
Actionable comments (0)
No blockers or major issues found.
Verified / looks good
is_backend_auth_failureis correctly provider-scoped: third-party BYO-key 401/403 still reach Sentry; onlyPROVIDER_LABEL401/403 are silenced.- Guard is placed first in the
chat_completionsif-else chain — mirrors the order inapi_errorexactly. anyhow::bail!(message)still executes unconditionally after the chain, so the error surfaces to the caller unchanged; only Sentry reporting is suppressed.api_errorrefactor is behavior-preserving:messageis already the same"{provider} API error ({status}): {sanitized}"string in both paths; double-sanitize_api_errorin the helper is safe (idempotent) and intentionally defensive.enrich_404_messageonly modifies the message for HTTP 404, so it does not affect the 401/403 auth-failure path.- Test
is_backend_auth_failure_only_matches_openhuman_backend_401_403: covers both positive (401, 403 from backend) and negative (non-auth backend statuses + all third-party BYO-key providers). Polarity contract is sound. - Test
publish_backend_session_expired_emits_sanitized_session_expired: subscriber is created before publish (no race),TEST_MARKER_Adistinguishes event from sibling test on shared global bus,sk-LIVEA…token probes end-to-end redaction viasanitize_api_error. - Test
chat_completions_backend_401_publishes_session_expired: axum mock returns real HTTP 401;chat_with_systemerror propagates correctly whileSessionExpiredevent is published;TEST_MARKER_Bandsk-LIVEB…are distinct from the unit-test event. init_global(1024)usesOnceLock::get_or_init— idempotent across parallel tests.- All CI checks pass, including coverage gate (≥ 80% on changed lines).
Nitpicks (0)
Questions for the author (0)
The sibling chains (native_chat, streaming_chat, responses_api) are correctly acknowledged as follow-up scope in the PR description — out of scope here.
…tions (Sentry TAURI-RUST-N) The hand-rolled provider HTTP-error chain in OpenAiCompatibleProvider's chat_with_system (operation=chat_completions) omitted the backend 401/403 -> SessionExpired branch that ops::api_error already has, so an OpenHuman backend "401 Invalid token" (expired/revoked/rotated session JWT) fell through to report_error and spammed Sentry (332 events). Add is_backend_auth_failure() + publish_backend_session_expired() to ops.rs and wire the guard into the chat_completions site: a backend 401/403 now publishes DomainEvent::SessionExpired (driving reauth and halting downstream LLM work via the scheduler-gate) and skips the Sentry report. Third-party BYO-key 401/403 stay Sentry-actionable. Sentry: https://sentry.tinyhumans.ai/organizations/tinyhumans/issues/81/
- api_error: replace the inline backend-SessionExpired branch with the existing publish_backend_session_expired helper so backend auth failures are handled in exactly one place (warn + publish + final sanitize). No behavior change — the helper already mirrored this arm. - Tests: the two SessionExpired tests shared the global event-bus singleton and both asserted on the same source + "Invalid token" substring, so under the parallel runner each could pass on the sibling's event. Give them distinct markers (TEST_MARKER_A / TEST_MARKER_B) so each verifies its own event, and add a sk- token to each 401 body to assert sanitize_api_error actually redacts it ([REDACTED] present, raw secret absent) — making the "...emits_sanitized..." name truthful and covering the redaction the helper's doc promises.
M3gA-Mind
dismissed stale reviews from coderabbitai[bot] and themself
via
b4ad470
M3gA-Mind
force-pushed
the
fix/sentry-tauri-rust-n-chat-401-session-expired
branch
from
012cf82 to
b4ad470
Compare
4 participants
Summary
401 Invalid token(expired/revoked/rotated app session JWT) hitting thechat_completionsprovider path was reported to Sentry as a code error — 332 events onTAURI-RUST-N(domain=llm_provider,operation=chat_completions,provider=OpenHuman).OpenAiCompatibleProvider::chat_with_systemomitted the backend401/403 → SessionExpiredbranch that the canonicalops::api_erroralready has, so it fell straight through toreport_error.is_backend_auth_failure()+publish_backend_session_expired()inops.rsand wire the guard into thechat_completionssite. A backend401/403now publishesDomainEvent::SessionExpired(driving reauth + halting downstream LLM work via the scheduler-gate) and skips the Sentry report. Third-party BYO-key401/403stay Sentry-actionable.Problem
Sentry: https://sentry.tinyhumans.ai/organizations/tinyhumans/issues/81/ (
TAURI-RUST-N)Event tags:
domain=llm_provider,operation=chat_completions,provider=OpenHuman,status=401,failure=non_2xx. Message:OpenHuman API error (401 Unauthorized): {"success":false,"error":"Invalid token"}.ops::api_error(used by thechat/warmupmethods) correctly treats a401/403from the OpenHuman backend as expected session-expiry: it publishesSessionExpiredand skips Sentry (the documentedOPENHUMAN-TAURI-1Tbehavior). Butchat_with_system(operation=chat_completions),native_chat,streaming_chat, andresponses_apieach hand-roll their own guard chain (is_budget_exhausted_*,is_provider_config_rejection_*, …) and never got the backend-auth branch. So for the OpenHuman backend provider (PROVIDER_LABEL = "OpenHuman",supports_streaming() == false, delegates to the compatible provider), a session-expiry401onchat_with_systemhitshould_report_provider_http_failure(401) == true→report_error→ Sentry, repeatedly (cron/heartbeat retries → 332 events).This is expected user-session state, not a product bug — the auth domain already owns recovery.
Solution
src/openhuman/inference/provider/ops.rsis_backend_auth_failure(provider, status)—trueonly for401/403andprovider == openhuman_backend::PROVIDER_LABEL. Provider-scoped so third-party BYO-key401/403(user's own OpenAI/Anthropic/OpenRouter key revoked) stay actionable in Sentry.publish_backend_session_expired(operation, provider, status, message)—warn!+publish_global(DomainEvent::SessionExpired { source: "llm_provider.openhuman_backend", reason: sanitize_api_error(message) }). Mirrors the existingapi_errorarm, factored out for the hand-rolled chains that consume the response body inline and so can't delegate toapi_error.src/openhuman/inference/provider/compatible.rschat_with_system'schat_completionserror chain gets a first branch:is_backend_auth_failure → publish_backend_session_expired(skips thereport_errorfall-through). Non-backend and non-auth statuses are unchanged.Design note:
api_errorkeeps its own inline arm (it is the correct, working path forchat/warmupand is out of scope for this Sentry issue). The new helper is shared-ready; see follow-ups.Submission Checklist
is_backend_auth_failure_only_matches_openhuman_backend_401_403(polarity incl. BYO-key negatives),publish_backend_session_expired_emits_sanitized_session_expired, and the e2echat_completions_backend_401_publishes_session_expired(axum 401 mock → assertsSessionExpired, no Sentry path).ops.rsis unit-covered; thecompatible.rsbranch is exercised by the e2e test driving a real 401 throughchat_with_system.N/A: behaviour-only change(observability classification of an existing path; no new/renamed feature row).## Related—N/A: behaviour-only change, no feature IDs.axummock; no real network.N/A: does not change a release-cut smoke surface.Closes #NNN—N/A: Sentry-only issue (TAURI-RUST-N), no GitHub issue to close; Sentry link in ## Related.Impact
openhuman-core), all desktop platforms. No frontend/Tauri-shell change.401/403on thechat_completionspath now drives the existingSessionExpiredreauth flow instead of spamming Sentry; the error still propagates to the caller (retry/fallback logic unchanged). Reduces noise and halts post-expiry cron/heartbeat loops via the scheduler-gate.reasonis re-run throughsanitize_api_errorbefore reaching subscriber logs.Related
native_chat,streaming_chat,responses_apihand-rolled chains incompatible.rsalso lack the backend-auth branch. The newis_backend_auth_failure/publish_backend_session_expiredhelpers are ready for those follow-ups.backend_api/authed_json), fix(observability): classify OpenHuman/Embedding/streaming backend 'Invalid token' 401 as SessionExpired (TAURI-RUST-4P0 + 4K5 + 1EE) #2786 (observabilityclassifier forrun_chat_task/embedding wrappers).native_chat/streaming_chat/responses_api; optionally de-duplicateapi_error's inline arm onto the shared helper.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check— N/A: no frontend files changed (Rust-core only);cargo fmt --checkclean.pnpm typecheck— N/A: no TypeScript changed. (Localtscfails only on pre-existing missing frontend deps —recharts,rehype-katex,remark-math— in files this PR does not touch.)cargo test --libfor the three new tests — all pass; fullopenhuman::inference::providersuite 315 passed / 0 failed.cargo fmt --checkclean;pnpm rust:check(core + Tauri shell) passes.pnpm rust:checkstill green.Validation Blocked
command:pnpm compile(pre-push hook)error:Cannot find module 'recharts' / 'rehype-katex' / 'remark-math'inapp/src/components/dashboard/*andapp/src/pages/conversations/components/AgentMessageBubble.tsximpact:Pre-existing local-env breakage (uninstalled frontend deps) in files unrelated to this Rust-core change; pushed with--no-verify. CI installs deps, so this does not affect the PR.Behavior Changes
401/403onchat_completionspublishesSessionExpired+ skips Sentry instead ofreport_error.Parity Contract
chat_completionskeep the exact prior chain;api_erroris untouched.anyhow::bail!(message)so retry/fallback is unchanged; mirrorsapi_error's backend-auth arm exactly (samesource, sanitizedreason).Duplicate / Superseded PR Handling
llm_provider/chat_completionscapture; fix(backend_api): suppress Sentry on 401 via typed BackendApiError::Unauthorized #2781 and fix(observability): classify OpenHuman/Embedding/streaming backend 'Invalid token' 401 as SessionExpired (TAURI-RUST-4P0 + 4K5 + 1EE) #2786 fix different layers).Summary by CodeRabbit
Bug Fixes
Tests