Share the stream-required discovery across OpenAiModel instances by endpoint - #98
Conversation
The per-instance stream_required latch stops a single OpenAiModel from
re-probing a streaming-only endpoint, but hosts build a fresh OpenAiModel
per workload (chat, summariser, titler, …) all aimed at the same endpoint.
So every new instance re-discovered the constraint and paid another
guaranteed-400 ("Stream must be set to true") round trip — the 400 kept
reappearing on every workload, every turn, even after one instance had
already learned better.
Record the discovery in a process-global set keyed by base_url (the
constraint is a property of the endpoint, not of any one instance or model
id). requires_streaming() now adopts a sibling's discovery and caches it
locally; latch_stream_required() records it once per endpoint and logs once
per process. Explicit with_requires_streaming() stays per-instance so it can
still be cleared. After the first cold probe the endpoint is never re-probed.
Refs openhuman#5497 (multi-instance shape of openhuman#5165).
📝 WalkthroughWalkthroughThe OpenAI transport now shares discovered ChangesOpenAI streaming discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAiModel
participant OpenAiTransport
participant StreamingRegistry
OpenAiModel->>OpenAiTransport: Resolve streaming requirement
OpenAiTransport->>StreamingRegistry: Read requirement by base URL
StreamingRegistry-->>OpenAiTransport: Return shared endpoint state
OpenAiTransport-->>OpenAiModel: Apply streaming precedence
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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/harness/providers/openai/transport.rs`:
- Around line 507-511: Update the logging in the stream-latching path around the
provider transport method to pass self.base_url through the existing
URL-redaction utility before assigning it to the tracing field. Preserve the
provider and model fields, and add a tracing test using a credential-bearing URL
that verifies userinfo, query parameters, and fragments are absent from the
emitted log.
🪄 Autofix
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 Plus
Run ID: d48860fc-648c-4c1d-ba06-6636f4344df0
📒 Files selected for processing (2)
src/harness/providers/openai/test.rssrc/harness/providers/openai/transport.rs
tinysweeper
Bot
added
the
priority: p3
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0266 · 19,112 in / 8,112 out · 11,383 cached (60%) · z-ai/glm-5.2
critique: $0.0114 · 5,426 in / 4,176 out · 4,267 cached (79%) · z-ai/glm-5.2
security: $0.0066 · 5,382 in / 1,101 out · 128 cached (2%) · z-ai/glm-5.2
tests: $0.0053 · 3,796 in / 1,859 out · 3,202 cached (84%) · z-ai/glm-5.2
description: $0.0034 · 4,508 in / 976 out · 3,786 cached (84%) · z-ai/glm-5.2
The endpoint-wide latch discovery logs base_url. An OpenAI-compatible proxy can carry a credential in the URL (userinfo or a query param), so logging the raw value risks leaking a secret. Route it through redact_base_url_for_log, which strips userinfo + query + fragment (keeping scheme://host[:port]/path) and replaces any value that does not parse as a URL. Add a unit test covering a credential-bearing URL, a clean URL, and an unparseable value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/harness/providers/openai/test.rs`:
- Around line 757-772: The redaction test currently checks only the URL prefix
and selected query tokens; replace the starts_with assertion in the redacted URL
test with an exact equality assertion against the expected scheme, host, port,
and path, confirming that query and fragment components are removed.
🪄 Autofix
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 Plus
Run ID: 93bd6a3e-d5dd-420d-b93b-7aaf426479bc
📒 Files selected for processing (2)
src/harness/providers/openai/test.rssrc/harness/providers/openai/transport.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/harness/providers/openai/transport.rs
tinysweeper
Bot
added
priority: p2
starts_with plus negative token checks did not prove the query and fragment were removed (a URL with a different query/fragment would pass). Replace with an exact-equality assertion against the fully redacted value, which proves userinfo, query and fragment are all gone while scheme, host, port and path survive.
CodeGhost21
left a comment
CodeGhost21
left a comment
There was a problem hiding this comment.
Reviewed the branch locally on 1.96.1 (the pinned stable can't build if let guards in transport.rs): cargo fmt --check, cargo clippy --all-targets -- -D warnings, and cargo test --lib all clean — 1578 passed, both parallel and --test-threads=1 (worth checking the single-threaded run given the new process-global state; it's fine).
The core change is right: the constraint genuinely belongs to the endpoint, the read-then-insert in remember_endpoint_requires_streaming returns insert()'s result so the log fires exactly once even under a concurrent first probe, and with_base_url already trims the trailing slash so the key is normalised. The local write-back in requires_streaming() keeps the shared read off the hot path after the first hit.
One thing I'd change before merge — the escape hatch is gone.
The PR body says "with_requires_streaming remains per-instance and clearable", and the doc comment on requires_streaming() still says the value is "either declared up front via with_requires_streaming or learned from a provider rejection". Both are now only true until some sibling latches the same base_url:
let m = OpenAiModel::new("k").with_base_url(shared).with_requires_streaming(false);
m.requires_streaming() // -> true, if any instance ever latched `shared`That matters because invoke has no path back: once requires_streaming() is true the unary attempt is never retried (transport.rs:2662), so a wrong shared inference is unfixable for the rest of the process. The case where that bites is an aggregating proxy (LiteLLM/OpenRouter-shaped) fronting mixed upstreams on one base_url — the chat model discovers stream-only against upstream A, and a titler model routed to upstream B that rejects stream: true gets forced onto the streaming path with no way for the host to opt out. Keying by (base_url, model) isn't the fix (it would defeat the point — your own test has the sibling on a different model id), but an explicit opt-out should win over inherited discovery. Something like a tri-state Option<bool> for the explicit setting, or an explicitly_set: AtomicBool that short-circuits the registry lookup, keeps the claim in the PR body true.
If you'd rather keep it simple and let the shared record win unconditionally, that's a defensible call for a constraint that's almost always endpoint-wide — but then please update the PR body and the requires_streaming() / with_requires_streaming() doc comments to say so, so the next reader doesn't reach for with_requires_streaming(false) as an override.
Nits:
-
test.rs:745— theopenhuman#5497comment block (hosts build a fresh model per workload, so discovery is shared process-wide) sits aboveredact_base_url_for_log_strips_credentials_and_query, but it describesstream_required_discovery_is_shared_across_instances_by_endpoint. Move it down one test. -
The process-global registry creates an invisible rule for the test module: no test may ever latch
DEFAULT_BASE_URL, orrequires_streaming_flag_skips_non_streaming_attempt(test.rs:684-699) starts failing for reasons that have nothing to do with it. The two moved tests carry a comment; a line onfn model()itself would put the warning where someone writing the next latch test will actually see it. -
Out of scope for this PR, but the redaction helper has other customers:
base_urlstill reaches user-facing error strings unredacted at transport.rs:1069, transport.rs:1220 and local.rs:354-363, and those land in logs/Sentry the same way. Worth a follow-up.
Behaviour and tests look sound otherwise — the sharing test covers all three directions (discoverer latches, sibling inherits, unrelated endpoint unaffected), and the exact-match assertion in the redaction test is the right call over substring checks.
… discovery Review of tinyhumansai#98: once discovery went process-global, with_requires_streaming(false) stopped being a real opt-out — requires_streaming() would return true for an instance that explicitly set false the moment any sibling latched the same base_url, and invoke() has no path back once it is true. That bites an aggregating proxy fronting mixed upstreams on one base_url (a titler routed to a non-streaming upstream forced onto the streaming path). Track whether the value was set explicitly (stream_required_explicit); when it was, requires_streaming() returns the instance's own value and does not consult the shared registry, so an explicit opt-out (or opt-in) always wins. Add a test covering opt-out-beats-sibling-discovery and opt-in-needs-no-discovery, and fix the doc comments to describe the real precedence. Also address review nits: move the misplaced openhuman#5497 comment onto the sharing test it describes, and warn on fn model() that latching the default base URL leaks into other tests via the global registry.
|
Thanks for the thorough local verification and the escape-hatch catch — that's a real regression in the API contract. Fixed in Escape hatch restored (the main point). Added a New test Nit 1 — moved the Nit 2 — added the "no test may latch Nit 3 — agreed, out of scope here: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/harness/providers/openai/transport.rs (1)
513-520: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExtract the endpoint registry from
transport.rs.
endpoint_requires_streamingandremember_endpoint_requires_streamingdefine process-wide registry behavior. Move the registry state and its focused helpers into a dedicated OpenAI module. KeepOpenAiModelresponsible only for precedence and latching.As per coding guidelines, “Keep each surface in its own module directory … rather than spreading feature code across broad files. Make the module root wire the pieces together and expose the smallest useful API.”
Also applies to: 532-542
🤖 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/harness/providers/openai/transport.rs` around lines 513 - 520, Extract the process-wide endpoint registry state and helpers endpoint_requires_streaming and remember_endpoint_requires_streaming from transport.rs into a dedicated OpenAI module. Update the module root to wire and expose only the minimal registry API, while keeping OpenAiModel limited to precedence checks and latching via stream_required.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/harness/providers/openai/transport.rs`:
- Around line 513-520: Extract the process-wide endpoint registry state and
helpers endpoint_requires_streaming and remember_endpoint_requires_streaming
from transport.rs into a dedicated OpenAI module. Update the module root to wire
and expose only the minimal registry API, while keeping OpenAiModel limited to
precedence checks and latching via stream_required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39f26ddd-0781-4e30-af62-73aa33b6890a
📒 Files selected for processing (2)
src/harness/providers/openai/test.rssrc/harness/providers/openai/transport.rs
CodeGhost21
left a comment
CodeGhost21
left a comment
There was a problem hiding this comment.
Re-reviewed at ff12169 on 1.96.1. The escape hatch is properly restored — approving.
On the fix. stream_required_explicit is the right shape: requires_streaming() short-circuits on it before touching the registry, so with_requires_streaming(false) is authoritative again and the aggregating-proxy case (titler routed to a non-streaming upstream behind a shared base_url) keeps the unary path. Precedence 1→2→3 in the doc comment now matches the code, and the PR-body claim is true rather than aspirational. Confirmed every consumer reads through requires_streaming() (transport.rs:2659, :2693) — nothing reads the raw stream_required field and bypasses precedence — and there's no Clone impl that could silently drop the new field.
The property I most wanted to check isn't covered by a test, so I wrote one locally: an instance explicitly opted out against an endpoint that genuinely is streaming-only still latches correctly on the real 400 (latch_stream_required writes stream_required, and the explicit branch reads that same cell), and its discovery still propagates to siblings. So a wrong opt-out costs one 400 on that instance and then self-corrects — the escape hatch can't wedge a model into a permanent 400 loop. Worth folding that case in as a test if you touch this again, but not worth another round trip.
Also re-verified trailing-slash normalisation collapses to one registry key, and redact_base_url_for_log now asserts exact equality so it proves userinfo/query/fragment are all gone rather than just the sample tokens.
Nits 1 and 2 are both addressed — the openhuman#5497 block sits on the test it describes, and the DEFAULT_BASE_URL warning is on fn model() where the next author will actually hit it. Nit 3 as a follow-up is the right call.
Verification: cargo fmt --check, cargo clippy --all-targets -- -D warnings clean; cargo test --lib 1579 passed both parallel and --test-threads=1 (re-checked single-threaded given the process-global registry). The --all-features boxes left unchecked in the PR body are all covered by ci.yml (clippy, build, test, llvm-cov) and CI is green.
Non-blocking: CodeRabbit's "extract the registry into its own module" is reasonable in the abstract, but it's two ~6-line helpers next to their only caller in a file that already holds plenty of module-local free functions. Declining it here is fine; if the endpoint registry grows a second constraint, that's the moment to lift it out.
2 participants
Summary
Hosts build a fresh
OpenAiModelper workload (chat, summariser, titler, …), each pointed at the same OpenAI-compatible endpoint. The stream-required latch added for a streaming-only proxy was per-instance, so every new instance re-discovered the constraint and paid another guaranteed-400 ({"detail":"Stream must be set to true"}) round trip — the 400 kept reappearing on every workload, every turn, even after one instance had already learned better.This shares the discovery process-wide, keyed by
base_url(the constraint is a property of the endpoint/proxy, not of any one instance or model id):requires_streaming()now adopts a sibling's discovery and caches it locally.latch_stream_required()records the endpoint once and logs once per process (rather than once per instance).with_requires_streaming()stays per-instance so it can still be cleared per instance (existingwith_requires_streaming(false)semantics preserved).After the first cold probe an endpoint is never re-probed. Written only by runtime discovery; entries are never removed (a streaming-only endpoint does not stop being one within a process).
Refs openhuman#5497 (the multi-instance shape of openhuman#5165).
API Or Behavior Changes
No public API changes. Behavior change: once any
OpenAiModelinstance discovers an endpoint requires streaming, other instances pointed at the samebase_urlskip the doomed non-streaming probe.with_requires_streamingremains per-instance and clearable.Tests
cargo fmt --checkcargo clippy --all-targets -- -D warningscargo clippy --all-targets --all-features -- -D warningscargo build --all-targetscargo build --all-targets --all-featurescargo test(--lib: 1577 passed, 0 failed)cargo test --all-featuresNew test
stream_required_discovery_is_shared_across_instances_by_endpointcovers cross-instance sharing (discoverer latches → a fresh instance for the samebase_urlinherits it → an unrelatedbase_urlis unaffected). The two existing latch tests were moved to uniquebase_urls so runtime discovery doesn't leak into the process-global registry shared with theDEFAULT_BASE_URLtests.--all-featuresvariants not run locally (repl/rhai feature); the change is feature-independent.Documentation
No doc changes needed — the shared registry is documented inline where it is defined (
STREAM_REQUIRED_ENDPOINTS), and the existinglatch_stream_requireddoc comment is updated to reflect endpoint-wide sharing.Summary by CodeRabbit