BAI-299 Capture Bedrock Mantle mid-stream error detail in model-router-errors#106
Closed
imranq2 wants to merge 996 commits into
Closed
BAI-299 Capture Bedrock Mantle mid-stream error detail in model-router-errors#106imranq2 wants to merge 996 commits into
imranq2 wants to merge 996 commits into
Conversation
…tion initialization
…encies Override UserSkillStore with NullUserSkillStore in test container to avoid MongoDB connections during tests. Make async_client fixture depend on test_container so container overrides are active before FastAPI lifespan calls initialize_skills().
…in Docker Compose
…o version 0.18.0 in Pipfile and Pipfile.lock
…files accordingly
Save skills to db
…ork, language-model-common, pydantic, and pypdf
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… multi-line output Groundcover ingests one log entry per newline in stdout, so multi-line messages (embedded request/response body dumps, exception tracebacks) were being split into many separate entries. Add JsonLogFormatter, which folds each LogRecord — including exc_info and any extra_fields — into one JSON object per line. Wired into both call sites that configure the root logger (log_levels.py and api.py) via a shared build_log_handler() so they can't drift out of sync; LOG_FORMAT=text opts back into the old plain-text format for local terminal use.
…over visibility model_routing had no timing instrumentation at all, so there was no way to see how fast requests were being processed per model tier. Reuse the existing OTel tracing pipeline (already flowing to Jaeger/OTLP via chat_completion_router's tracer) rather than standing up a new metrics pipeline — the collector currently drops OTLP metrics to a debug exporter, so a Histogram would need infra changes too. _record_upstream_latency sets model_tier, upstream_model, auth_strategy, api_type, and upstream_latency_ms as attributes on the current span at the point each dispatch path (OpenAI-SDK streaming, OpenAI-SDK non-streaming, Anthropic passthrough) gets its first response back from upstream. This measures the proxy's own dispatch overhead, not total streaming duration, which is dominated by the upstream model's token-generation speed rather than anything this router controls.
Applies security fix for IDOR [gecko-fix]
gecko-security[bot]'s auto-fix (254da2b) correctly removed the unvalidated-header fallback from record_usage_from_anthropic_response/ record_usage_from_openai_response, relying solely on auth_info's already-verified identity fields. That leaves extract_user_id_from_headers and its email/user_name/auth_provider siblings with zero callers — trusting raw request headers for identity is exactly the vulnerability being closed, so these are worth deleting outright rather than leaving as an attractive nuisance for a future caller to wire back in. Removed the methods and their now-pointless unit tests.
…, correct db_name default expectation
record_usage() short-circuited on self._enabled before ever reaching the
_collection check, which broke tests that mock _ensure_connected and
inject a fake collection directly. _ensure_connected() already gates on
_enabled when establishing a real connection, so the extra check in
record_usage was redundant and only served to mask the injected
collection in tests. Also fixes test_init_with_defaults, which expected
the wrong default db_name ("usage_tracker" instead of the real
"llm_storage" default used by router.py/api.py and documented in
CACHING.md).
Also registers the "integration" pytest marker and filters the known
StarletteDeprecationWarning about httpx/httpx2 from FastAPI's
TestClient import, to clean up warning noise in CI output.
An unauthenticated caller could set x-openwebui-user-id to force auth_information.subject to an arbitrary value. system_command_manager uses that subject as referring_subject to clear cached tokens, so this allowed impersonation and forced session invalidation for any user. Identity is now derived solely from a verified token; unverified requests fall back to no identity.
Keeps the exact same Groundcover-facing JSON schema (timestamp, level, logger, message, file, line, exception, merged extra_fields) but renders it via structlog.stdlib.ProcessorFormatter instead of hand-built JSON construction. Every logging.getLogger(...)/logger.info(...) call site in the app is unaffected — ProcessorFormatter treats them all as "foreign" stdlib records and runs them through a foreign_pre_chain + processors pipeline before JSON rendering, so this stays scoped to log_levels.py. Adds structlog as a new runtime dependency (not yet in policies/approved-tech.yaml — flagged in adrs/0001 for Tech Design Review per EA policy; severity is "warn"/non-blocking for the observability category). Also fixes a real bug found while implementing this: ProcessorFormatter seeds event_dict["exc_info"] with the raw (type, value, traceback) tuple for records with a traceback; left unremoved it leaked into the rendered JSON as a stringified list alongside the intended "exception" field. Popped after folding into "exception"; covered by a regression assertion in test_json_log_formatter_folds_exception_into_single_line.
… logs DEBUG_LOG_RECEIVED_OAUTH_TOKENS (off by default) logs the full request CodingModelRouter receives on /v1/messages, so a developer can inspect what a client actually sends (e.g. token format, streaming flag) without guessing. Also fixes a gap where non-APIStatusError failures from the Bedrock Mantle (openai api_type) upstream were silently re-raised with no log line; both error paths now log request_id/model/auth/user_id/streaming context, and unexpected exceptions include a full traceback.
Non-streaming responses now schedule the usage-tracking write via Starlette BackgroundTasks instead of awaiting it before returning, so the client gets the response as soon as the LLM call completes. The streaming path already deferred the write past the last SSE chunk via a generator finally block, but still serialized stream teardown behind it; it now fires the write as a tracked asyncio task instead so the stream closes immediately. Also dedupes _get_auth_info(request) calls in proxy_messages (was re-verifying the JWT up to 6 times per request for identical results) and removes an unused _extract_usage_from_response method.
UsageTracker connected to Mongo with the bare MONGO_LLM_STORAGE_URI/ MONGO_URL, but in the services environment that URI has no embedded credentials -- auth is supplied separately via MONGO_DB_USERNAME/ MONGO_DB_PASSWORD. Without merging those in, the connection would fail auth and UsageTracker would silently disable itself. Compose the credentialed URL the same way persistence_factory.py already does for the other Mongo-backed stores before constructing CodingModelRouter.
Adds the Mongo connection env vars usage tracking reads, a "Usage tracking" section covering coverage/attribution/fire-and-forget behavior, and the gateway-wide LOG_FORMAT env var that was previously only documented in the structlog ADR.
CodingModelRouter already accepted a usage_collection_name override, but api.py never wired it up, so the usage-tracking collection was always hardcoded to "usage" regardless of environment -- unlike its sibling LLM-memory collections (store/checkpointer), which already have env var overrides. Adds the same capability for usage tracking.
Renamed for clarity before this collection has ever been written to in production (usage tracking was silently disabled by the Mongo credential bug fixed earlier on this branch). UsageTracker/ CodingModelRouter keep their own generic "usage" constructor defaults for standalone/test use -- LanguageModelGatewayEnvironmentVariables is the single source of truth for the value this app actually deploys with, which api.py always passes through explicitly.
Documents the AccountDirectory design for resolving Claude Code's opaque metadata.user_id/account_uuid into a real email via a manually-populated Mongo lookup table, for usage-tracking attribution where the OIDC-verified identity path never applies (Claude Code's Authorization header is always the client's own Anthropic subscription OAuth token, never a b.well OIDC JWT).
Claude Code and other Anthropic SDK clients already advertise gzip in Accept-Encoding; nothing in this app compressed responses before this. Added as the outermost middleware (registered last) so it compresses after FastApiLoggingMiddleware has already inspected the plain response body, and skips streaming responses automatically (no known Content-Length), so the SSE model-routing paths are unaffected. Verified against the running app: gzip applied when the client requests it, plain passthrough otherwise.
… from auth_info Add an in-process cache (found and not-found) to AccountDirectory.resolve_email so the per-request account_uuid lookup on the hot path doesn't hit Mongo on every request. Fix a misleading "avoid hard dependency" comment on the deferred pymongo import in both account_directory.py and usage_tracker.py -- pymongo is already a hard dependency; the deferred import just avoids its import cost when the feature is disabled. Also stop stashing the raw Authorization bearer token into auth_info["headers"] in _get_auth_info since nothing downstream reads it.
…iews on usage records user_id was already best-effort (only set for verified OIDC callers or via the account_uuid->email directory fallback), but session_id was sitting unused in Claude Code's request metadata alongside account_uuid, and usage records had no explicit timestamp field. Preview capture is opt-in via MODEL_ROUTING_USAGE_CAPTURE_PREVIEWS (default off) since it persists truncated prompt/response text rather than just metadata.
…uncated previews account_uuid -> email resolution now happens downstream in reporting instead of via a live directory lookup on every request, so _enrich_with_account_directory is replaced with _attach_account_uuid, which just records the raw account_uuid on the usage document. AccountDirectory itself is left wired up (unused) so the model-router-account-directory collection still exists for that reporting join. Also: - record model_tier (already computed for latency metrics) on usage records - mark truncated input_preview/output_preview with a trailing "…" so it's unambiguous from the record alone whether the preview is the full text
…tom-header attribution
- backend ("anthropic"/"aws_bedrock"), cost_usd, and cost_savings_usd (using
a new anthropic_price_per_mtok baseline per route in model-router-config.json)
- streaming, compression_requested (client Accept-Encoding), and
compression_used (predicts GZipMiddleware's compress/skip decision, since
the usage record is written from a background task after the response
already went out)
- MODEL_ROUTING_CUSTOM_HEADER_PREFIX (default x-model-routing-): any header
under this prefix is stripped before forwarding upstream and captured
wholesale into auth_info["custom_headers"]/usage_record["custom_headers"],
so new client-supplied attribution headers can be added later (e.g. via
Claude Code's ANTHROPIC_CUSTOM_HEADERS) without a code change here.
{prefix}user-id is additionally pulled out as a best-effort user_id
fallback when there's no OIDC-verified identity, recorded with
auth_provider="custom-header" so it's never confused with verified
identity downstream.
…way-protocol fields Bug fix: FastApiLoggingMiddleware fully drained a StreamingResponse's body iterator into memory to log it whenever HTTP_TRACING_LOG_LEVEL=DEBUG was set (true in this repo's local docker-compose.yml) -- fully buffering every SSE stream before any of it reached the client. Now peeks only the first chunk (all it ever logged) and lazily re-chains the rest. Streaming reliability: - X-Accel-Buffering: no on every SSE response, so an nginx/ingress with default buffering in front of this gateway can't silently re-introduce the same failure mode at the infra layer. - request.is_disconnected() checked per chunk on both streaming paths; stops pulling further chunks from upstream once the client disconnects, instead of continuing to consume (and pay for) tokens nobody will receive. - sse_event_count recorded on the usage record as a cheap signal that a response actually streamed rather than got buffered and dumped at once. Per the documented gateway protocol (https://code.claude.com/docs/en/llm-gateway-protocol): - session_id now prefers the documented x-claude-code-session-id header over parsing body.metadata.user_id (kept as a fallback for older clients). - agent_id/parent_agent_id captured from x-claude-code-agent-id / x-claude-code-parent-agent-id, enabling cost attribution to subagents.
…ough usage, and upstream errors Adds start_time/end_time/duration_ms to usage records; upserts a per-session rollup (model-router-sessions) bucketed by tier with independent MODEL_ROUTING_USAGE_SESSION_TRACKING_ENABLED toggle; extends usage tracking to the api_type:anthropic passthrough route (previously uncovered) by buffering non-streaming responses and sniffing token usage out of streaming SSE bytes without altering them; and adds an ErrorTracker recording upstream failures to model-router-errors.
Fix usage-tracking IDOR (spoofable auth headers) and switch JSON logging to structlog
…r-errors
openai.APIError (raised by the SDK for SSE data events with no HTTP status
code, e.g. Bedrock Mantle sending {"error": {...}} mid-stream) was falling
through to the generic exception handler, which only recorded str(exc) - a
generic "server had an error" message with no diagnostic value. Add a
dedicated except branch that captures the SDK's .body/.code/.type/.param
in model-router-errors, and surface the same detail to the client instead
of just the generic passthrough message.
Owner
Author
|
Closing — opened against the wrong repo (gh defaulted to the upstream remote). Correct PR: see icanbwell/language-model-gateway. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
model-router-errorshad only a genericopenai.APIErrormessage with no diagnostic value for a Bedrock Mantle request toqwen.qwen3-coder-next).openai.APIErroris raised by the openai SDK for errors embedded in an SSE data event ({"error": {...}}mid-stream, no HTTP status code) — distinct fromopenai.APIStatusError(HTTP 4xx/5xx), which was already handled. This case fell through to the genericexcept Exceptionhandler and onlystr(exc)was recorded.except openai.APIErrorbranch inrouter.pythat captures the SDK's.body/.code/.type/.param(which the exception actually carries) intomodel-router-errorsaserror_type="bedrock_stream_error", and surfaces the same detail to the client instead of the bare generic message.Test plan
test_bedrock_mantle_mid_stream_error_records_full_detailcovering the new branch (mocked SSE error event, asserts both the recorded Mongo detail and the client-facing response text).pytest tests/gateway/routers/test_coding_model_router.py— 50 passed.ruff check/ruff format --check/mypyclean on changed files.