Return a real HTTP status when a stream fails before its first event

[lloydmak99]

The bug

When the first event from an upstream provider is an error, we fall through to break None, commit HTTP 200, open an SSE body, and deliver the failure as an in-band error frame. Worse, [DONE] is still appended afterwards — so a client keying on [DONE] reads the whole thing as a successful empty completion, not a failure.

Response headers are still mutable at that point. No stream has been produced and nothing has been written to the client.

Reproduced against production

2026-08-19, streaming google/gemma-4-31B-it:

Request	HTTP	SSE events	What the client gets
control (minItems/maxItems)	200	30	normal completion, clean [DONE]
schema with uniqueItems	200	1	HTTP error 400: Grammar error: Unimplemented keys: ["uniqueItems"], then [DONE]
schema with contains	200	1	same shape
schema with propertyNames	200	1	same shape

A partner reported 46 of these in a six-hour window on 2026-07-23 — every one of them indistinguishable from an empty success without parsing the frame body. The error string in their report is character-for-character identical to what still reproduces today.

The change

17 lines. Replace _ => break None with an explicit Some(Err(error)) arm that maps the error through the existing map_provider_error → map_domain_error_to_status path and returns the real status. None keeps its previous behaviour.

This costs no latency — the first event is already awaited before the response is constructed, so nothing new blocks.

It fixes the whole class, not one keyword: any pre-first-event upstream error now surfaces as a real HTTP status. The uniqueItems case is just the one a customer happened to report.

map_provider_error becomes pub so the api crate can call it. The alternative was duplicating error mapping into api, which is worse.

Deliberately not in this PR

An earlier version also added a JSON-Schema pre-flight that stripped four llguidance-unsupported keywords and hard-rejected nine others. It was cut, and should not be restored as written:

• It ran at the API boundary — completions.rs:1403, before alias resolution at :1443 and long before provider selection — so it applied llguidance's limits to every Chat Completions request, including those bound for backends that don't use llguidance. The concrete collision is Gemini, whose converter deliberately forwards schemas verbatim via responseJsonSchema and has a regression test requiring that preservation (gemini/converter.rs:1364-1407).
• Strip-and-warn traded a loud failure for a quiet one: the caller would receive a 200 with possibly-duplicate array items, signalled only by a response header most SDKs never surface.
• The schema walker was unbounded recursion on a body the API allows up to 25 MiB.

That policy belongs at provider dispatch, where the backend is known, and should hard-reject rather than silently strip. Worth a separate PR.

Verification

• cargo build — clean
• cargo clippy -p services -p api --all-targets -- -D warnings — clean
• cargo test -p api --test e2e_all --no-run — compiles

Two tests added: one asserting a pre-first-event error returns 400 with uniqueItems in the message and content-type: application/json, one control asserting a normal first chunk is still first and unmodified.

The e2e tests were not executed — they need PostgreSQL, which wasn't available in this environment. I'm not claiming a test pass I didn't observe; CI should be the judge.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Fm9f3maaY2wd6yHH8FEjR5

[claude]

Review

No prior review threads on this PR, so nothing to build on.

The core change is correct and well-scoped. The Some(Err(_)) arm sits before any bytes are written and before the streaming Response::builder(), so headers are genuinely still mutable; the early return matches the existing error idiom used at completions.rs:2089 and :2203 verbatim (map_domain_error_to_status → ResponseJson<ErrorResponse>). peek() yields Option<&Result<..>>, so error binds as &CompletionError, which is what map_provider_error takes. Dropping the peeked stream releases it; the old path's only side effects on this branch were a tracing::error! and the emitted frame, and map_provider_error logs equivalently. Nothing lost.

I also confirmed the two existing set_stream_error_override tests in provider_errors.rs:173 and :210 target /v1/responses, not chat completions, so they aren't invalidated. mock_upstream_only_field / "dropped-by-typed-parse" match mock.rs:936, and with continuous_usage_stats the passthrough branch at completions.rs:1676 holds, so the control test asserts what it claims to.

⚠️ Blocking: the identical bug is still live on /v1/completions

completions_inner has the same peek loop with the original _ => break None at crates/api/src/routes/completions.rs:2426 — untouched by this PR.

It is not merely analogous, it is a strictly worse instance of the same failure. That route re-serializes rather than passing bytes through, and unconditionally appends its own terminator at :2517:

.chain(futures::stream::once(async move {
    Ok::<Bytes, Infallible>(Bytes::from_static(b"data: [DONE]\n\n"))
}));

So a pre-first-event upstream error on /v1/completions yields exactly the shape described in the PR body — HTTP 200, one sse_error_frame, then a gateway-minted [DONE] — with no upstream-[DONE] suppression path to even vary it. The uniqueItems reproduction would land here identically.

The fix is the same ~13 lines. organization_id is already bound at :2454, so:

Some(Err(error)) => {
    let domain_error = services::CompletionServiceImpl::map_provider_error(
        &request.model,
        error,
        "text completion stream",
        api_key.organization.id.0,
    );
    let status_code = map_domain_error_to_status(&domain_error);
    return (status_code, ResponseJson::<ErrorResponse>(domain_error.into()))
        .into_response();
}
None => break None,

Leaving one of two sibling handlers fixed means the next report of this class comes from /v1/completions and reads as a regression of a fix that shipped. The PR's "it fixes the whole class" framing holds for chat completions only.

Non-blocking

• Client-retry amplification. Errors mapping to 5xx/429 now surface as real statuses, which OpenAI-compatible SDKs auto-retry — previously they were absorbed as a 200. Correct semantics, and the right trade, but a pre-first-event upstream 500 now multiplies into N requests against an already-failing backend. Worth a release note; no code change needed.
• Alias naming. Line 1554 passes &request.model where resolved_model_name (:1441) is available. Every other error path gets its model name from the service, i.e. post-alias-resolution. Only affects the log line and the 429 message text. Arguably the alias is friendlier to the caller — just noting it's inconsistent with siblings.

Agreed on deferring the JSON-Schema pre-flight; the Gemini responseJsonSchema collision is a real one and provider dispatch is the right layer.

⚠️

[ironloopai]

IronLoop Run · Review

🟩 Completed

IronLoop completed the review and posted it to GitHub.

Result

Open submitted review →

Run details

• Run: f174b3df-352c-43bc-8a05-c1ec5a4648e2
• Base: main at 54b4a7c
• Head: fix/stream-first-event-http-status at 97cf00b
• Created: 2026-08-21 02:19 UTC
• Updated: 2026-08-21 02:32 UTC

_{Automatic trigger · attempt 1 of 3 · completed in 12m 13s}

[github-actions]

✅ OpenCodeReview: Review partially complete: 0 finding(s); 3 of 4 selected item(s) failed.

[ironloopai]

IronLoop Review

🟢 No actionable findings

No new actionable defects found in the reviewed change.

Validation

• ✅ Changed-path inspection — The new pre-SSE error branch executes before response construction, uses the established error mappings, and leaves normal stream forwarding intact.

Review details

• Run: f174b3df-352c-43bc-8a05-c1ec5a4648e2
• Workflow: Review
• Attempts: 1

[PierreLeGuen]

Replacing the catch-all peek arm with an explicit Some(Err(error)) branch correctly returns a real HTTP status for pre-first-event upstream errors. No defects found.

Checks: cargo +1.92.0 check -p api -p services --all-targets — clean (includes the new e2e_all/first_stream_event.rs module); cargo +1.92.0 clippy -p services -p api --all-targets -- -D warnings — clean; cargo +1.92.0 fmt --all -- --check — passed

[lloydmak99]

The Chutes parser fix that briefly sat on this branch has been split out into #984, so this PR is back to exactly the change you approved: return a real HTTP status when a stream fails before its first event.

Branch was force-pushed back to ebcf80e (the merge of main from 08-24). Four files, no inference_providers changes.

Note the two PRs both touch extract_inference_id_from_chunk and its call sites, so whichever merges second will need a trivial merge in that function. They're independent otherwise — this one makes a pre-first-event error report correctly, #984 stops the stream failing in the first place.

[lloydmak99]

Reworked per feedback: the route no longer inspects stream events. Pushed as 216d4e5.

Why the route peek was removable

The pool already peeks the first event on every streaming request, to record the chat_id for sticky routing — inference_provider_pool/mod.rs:3305:

let mut peekable = StreamingResultExt::peekable(stream);
…
if let Some(Ok(event)) = peekable.peek().await {

That predates this PR, and it discarded the Err case — handing back a stream whose first item is an error, which forced the caller to either inspect it or commit 200 and bury the failure in-band. So the earlier version added a second, duplicate peek one layer up.

The pool now returns Err from the peek it already performs. Net effect is one fewer inspection than before this PR.

What the diff is now

File
crates/services/src/inference_provider_pool/mod.rs	+112 — the Err return, plus a test
crates/api/tests/e2e_all/first_stream_event.rs	+82 — assertions unchanged
crates/api/tests/e2e_all/main.rs	+1

completions.rs and services/completions/mod.rs have left the diff entirely — both back to main's shape. map_provider_error reverts to pub(crate); it was only pub for the route arm this removes, and has no callers outside the crate.

Behaviour is unchanged

A stream whose first upstream event is an error still returns a real HTTP status through the existing error path — the same one that already yields 400 for an unknown model. The e2e assertions were not touched; only the layer that detects the error moved.

Two things I checked because they were the risky parts

The orphaned-pending-client cleanup still runs. The if !pinned block executes before the early return Err, so an error cannot leak a pinned connection. A leak there would surface as backend-affinity drift days later rather than as a failing test.

The error is cloned out of the peek, so status_code and is_external survive for map_provider_error — it classifies on both, and a 404 from a third-party provider maps differently from a 404 from our own vLLM. Flattening it would have silently changed status mapping for cases nobody is looking at.

Verification

• cargo clippy -p services -p api -p inference_providers --all-targets -- -D warnings — clean
• cargo test -p services --lib inference_provider_pool — 100 passed, 0 failed
• test_first_stream_error_is_returned_before_stream — passes
• cargo test -p api --test e2e_all --no-run — compiles

On the earlier CI failure

Unrelated flake. test_responses_api_sufficient_credits failed on Database operation error: Failed to start transaction during POST /v1/organizations, with 3 further timeouts and only 274 of 705 tests run before the job aborted. test_response_stream_fails_with_failed_event_when_inference_fails_at_start — the streaming test nearest this change — passed, and every recent main run is green. This push re-runs CI.

[PierreLeGuen]

Moving first-event error classification into the pool's existing sticky-routing peek correctly turns a pre-first-event upstream failure into a real HTTP status instead of a 200 SSE with an in-band error frame and [DONE].

Optional follow-ups:

• crates/services/src/inference_provider_pool/mod.rs:3321 — The new arm treats every first-item Err as fatal to the request, but the SSE parser does not treat per-event parse/conversion failures as terminal. Fix: Narrow the early return to error kinds that are terminal for the stream.

Checks: cargo +1.92.0 test -p services --lib test_first_stream_error_is_returned_before_stream — 1 passed; cargo +1.92.0 test -p services --lib inference_provider_pool — 100 passed, 0 failed; cargo +1.92.0 test -p services near_5xx_falls_back_to_chutes_within_one_request --lib — passed