fix(openai): latch the stream-only constraint and widen its detection - #85
fix(openai): latch the stream-only constraint and widen its detection#85M3gA-Mind wants to merge 1 commit into
Conversation
OpenAI-compatible proxies exist that refuse unary calls outright, answering a
`stream: false` body with an HTTP 400 `{"detail":"Stream must be set to true"}`.
`ChatModel::invoke` already falls back to the streaming path on that rejection,
but the fallback has two gaps that keep it from actually converging.
**The constraint is never remembered.** `requires_streaming` is a plain `bool`
that only `with_requires_streaming` can set, and nothing calls that builder —
`invoke` takes `&self`, so the auto-detected discovery had nowhere to go. Every
single unary call therefore re-pays a guaranteed-400 round trip before falling
back: one wasted request per agent turn, forever. Make it an `AtomicBool` and
latch it on first discovery, so only the first call pays and the rest go
straight to the streaming path. `latch_stream_required` logs on the transition
only, so the discovery is visible exactly once per process.
**The detection is one case-sensitive literal.** The trigger was
`err.message.contains("Stream must be set to true")`, so any other phrasing —
lower-case, `'stream' must be true`, `streaming is required`, `only streaming
is supported`, `stream=true is required` — still hard-failed the harness run.
Replace it with `is_stream_required_error`, which matches that family
case-insensitively and searches the raw body as well as the decoded message:
`parse_error_body` only promotes `error.message` / `message`, so a reason nested
under any other key (`detail`, `errors[].reason`) was invisible. `422` joins
`400` because the wording arrives in a FastAPI `{"detail": …}` envelope and
FastAPI's own field-validation status is 422.
A `stream` mention is required alongside the "must be true" phrasing so
unrelated 400s never get rerouted — the `degrade_for_400` `tool_choice` /
`response_format` handlers and the `/responses` `max_output_tokens` retry keep
their bodies to themselves.
`requires_streaming()` is now `pub` (it was `#[cfg(test)]`-only) so hosts can
observe a learned constraint.
Tests: latch set/idempotence, latch visibility through a shared `Arc` handle,
an 8-body wording matrix including the exact reported body and one nested under
a non-standard key, and a negative matrix (wrong status, `tool_choice`,
`max_output_tokens`, `stream_options is not supported`, `'store' must be true`).
Known gap, deliberately out of scope: the `/responses` path
(`responses_api_primary`) returns before this check and sends `stream: None`, so
a streaming-only Responses backend still surfaces the raw 400. Folding Responses
SSE is a separate piece of work — the transport notes it as a follow-up.
Refs openhuman#5165.
There was a problem hiding this comment.
M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
sanil-23
left a comment
There was a problem hiding this comment.
Approve. Well-reasoned root-cause fix, tight tests, thorough docs, CI green.
Solid:
bool → AtomicBoolis the correct fix —invoke(&self)behindArc<dyn ChatModel>genuinely couldn't remember the discovery otherwise, so every turn re-paid a guaranteed 400. The doc comment warning against "simplifying" it back to aboolis a nice touch.Ordering::Relaxedis right: the latch is a standalone optimization flag with no happens-before to other state; a raced miss just pays one benign extra 400.- Detection widening is properly guarded (status ∈ {400,422} AND a
streammention AND a phrase), and the negative matrix pins thattool_choice/max_output_tokens/stream_options/'store' must be truedon't get hijacked.
Non-blocking nit (LOW): mentions_stream_required checks the two signals ("stream" + a phrase) independently anywhere in the body, no proximity — so a body co-mentioning both in unrelated contexts, e.g. {"error":{"message":"stream_options is not supported","detail":"temperature must be true"}}, would false-positive and latch streaming for the instance's lifetime. The negative tests cover each half in isolation but not the co-occurrence in one body. Consequence is mild (a wrong latch still yields correct streamed responses), so this is a robustness nit — worth either adding a co-occurrence case to the negative matrix or tightening to require proximity, but not a blocker.
/responses gap and the concurrency imprecision of "only the first call pays" are both correctly acknowledged/scoped. Ship it — fixes a live 511-event/2-user issue (openhuman#5165), host pickup is just a submodule pin bump.
Summary
Some OpenAI-compatible proxies refuse unary calls outright, answering a
stream: falsebody with an HTTP 400{"detail":"Stream must be set to true"}.ChatModel::invokealready falls back to the streaming path on that rejection (added in therequires_streamingchange), but the fallback never converges, for two reasons.1. The constraint is never remembered.
requires_streamingwas a plainboolthat onlywith_requires_streamingcould set — and nothing calls that builder, here or in the downstream host. It could not:ChatModel::invoketakes&self, so the auto-detected discovery had nowhere to be stored. Every unary call therefore re-paid a guaranteed-400 round trip before falling back: one wasted request per agent turn, indefinitely. This is the shape behind the linked issue's 511 events across 2 users — not a single failure, a per-turn one.Made it an
AtomicBooland latch it on first discovery, so only the first call pays and every later call goes straight to the streaming path.latch_stream_requiredlogs on the transition only, so the discovery is visible exactly once per process rather than once per turn.2. The detection was one case-sensitive literal. The trigger was
err.message.contains("Stream must be set to true"), so any other phrasing still hard-failed the run. Worse,parse_error_bodyonly promoteserror.message/messagetoProviderError::message, so a reason nested under any other key alongside a generic top-levelmessagewas invisible to the check.Replaced it with
is_stream_required_error, which:stream must be set to true,'stream' must be true,streaming is required,only streaming is supported,this deployment requires streaming,stream=true is required;detail/errors[].reasonis still caught;{"detail": …}envelope, and FastAPI's own field-validation status is 422 — a proxy that validatesstreamas a request field rather than raising an explicitHTTPException(400)lands there.A
streammention is required in addition to the "must be true" phrasing, so unrelated 400s never get rerouted into the streaming path —degrade_for_400'stool_choice/response_formathandlers and the/responsesmax_output_tokensretry keep their own bodies to themselves. The negative test matrix pins that.API Or Behavior Changes
Public API
OpenAiModel::requires_streaming()is nowpub(waspub(super)+#[cfg(test)]), so a host can observe a constraint the transport learned at run time.OpenAiModel::with_requires_streaming(self, bool)— unchanged signature and semantics; it now seeds the latch instead of setting a plain field. Still useful to declare the constraint up front and skip even the one exploratory request.Behavior
ModelResponse, and the existing prompt-tool-call recovery on the streaming path is inherited unchanged.stream: false— nothing latches, and the detection requires an explicit streaming constraint in the error body.Known gap, deliberately out of scope: the
/responsespath (responses_api_primary) returns frominvokebefore this check and sendsstream: None, so a streaming-only Responses backend still surfaces the raw 400. Folding Responses-format SSE is separate work that the transport already defers ("True Responses SSE is a follow-up"); wiring this check there without it would only change which error is reported.Tests
Added to
src/harness/providers/openai/test.rs:stream_required_constraint_latches_after_discovery— the latch records the discovery and is idempotent.stream_required_latch_survives_through_a_shared_handle— visible through a sharedArc<OpenAiModel>, which is how hosts hold models and the whole reason this is anAtomicBool.is_stream_required_error_matches_the_known_proxy_wordings— 8 bodies including the exact one from the linked report, a lower-case variant, the alternative phrasings above, and one nested under a non-standard key alongside a generic top-levelmessage; plus the 422 case.is_stream_required_error_ignores_unrelated_failures— wrong status (500),tool_choice,max_output_tokens,stream_options is not supported(mentionsstream, states no requirement), and'store' must be true(states a requirement, different field).Commands run locally:
cargo fmt --checkcargo clippy --all-targets -- -D warnings— 0 warningscargo clippy --all-targets --all-features -- -D warnings— 0 warningscargo build --all-targetscargo build --all-targets --all-featurescargo test— all suites green, 0 failed (3 ignored: the network-gatedlive_*tests)cargo test --all-features— all suites green, 0 failedcargo test --libalone is 1228 passed, 0 failed, and covers every test touched or added here.Documentation
src/harness/providers/openai/mod.rsnow describes the streaming-only rejection family, the fold, and the latch, next to the existing "local-server compatibility" paragraph on thetool_choice/response_formatdegradations.is_stream_required_error,STREAM_REQUIRED_STATUSES,latch_stream_required, and thestream_requiredfield each carry doc comments explaining the rationale — in particular why interior mutability is required, so the next reader does not "simplify" it back to abooland silently restore the per-call round trip.ChatModel::invoke's doc comment updated: the constraint may be declared or discovered, and a discovered one is latched.Related
Fixes the provider-side root cause behind tinyhumansai/openhuman#5165 (
tinyagents harness: openai HTTP 400 'Stream must be set to true'). Picking this up in the host is a submodule pin bump; no host-side code change is needed.