Defer streaming request content creation until enumeration

[fallintoplace]

Summary

• snapshot streaming JSON request payloads when the API is called
• create and dispose BinaryContent inside the deferred send when enumeration begins
• pass the snapshot through lightweight SseUpdateCollection and AsyncSseUpdateCollection adapter constructors
• cover chat, assistants, and speech streaming with request-serialization regressions

Problem

Several streaming APIs created disposable request content before returning a lazy collection. The HTTP send does not begin until that collection is enumerated, so the request body could be disposed before the transport attempted to serialize it.

The affected paths were:

• chat completion streaming
• assistants thread-and-run streaming
• assistants submit-tool-outputs streaming
• speech generation streaming

Approach

Each affected method serializes its request once to BinaryData at call time. This preserves the original call-time snapshot behavior while allowing a fresh BinaryContent instance to be created inside the deferred send and disposed only after that send completes.

The new collection constructors are adapters over the existing delegate-based constructors. They keep the collection internals on a single dispatch path while threading BinaryData explicitly at call sites.

Multipart transcription streaming is intentionally unchanged because its request content wraps a live stream and has different lifetime requirements.

Tests

• added sync and async mock coverage for chat, assistants, and speech streaming that serializes request content only when enumeration starts
• added a negative chat regression demonstrating that stream-backed BinaryContent created outside a deferred send becomes unusable after disposal
• verified the transport callback ran before parsing the captured chat request
• ran the focused mock suite on .NET 10: 60 passed, 2 existing mode-specific skips

[Copilot]

Pull request overview

Fixes a lifetime bug in deferred-send SSE streaming APIs where request bodies could be disposed (or mutated) before enumeration begins by snapshotting the JSON payload at call time and constructing fresh BinaryContent inside the deferred send delegate.

Changes:

• Snapshot streaming request payloads to BinaryData at call time and create/dispose BinaryContent inside the deferred streaming send for Chat and Audio speech SSE streaming.
• Refactor Assistants streaming request creation similarly (thread-and-run + submit-tool-outputs), including introducing a helper that returns BinaryData.
• Add mock regression tests that force request-body serialization during enumeration for chat, assistants, and speech streaming.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/Chat/ChatMockTests.cs	Adds a mock regression ensuring chat streaming request content can be serialized at enumeration time.
tests/Audio/GenerateSpeechMockTests.cs	Adds a mock regression ensuring speech streaming request content can be serialized at enumeration time.
tests/Assistants/AssistantsMockTests.cs	Adds a mock regression ensuring assistants streaming request content can be serialized at enumeration time.
OpenAI/src/Custom/Chat/ChatClient.cs	Snapshots streaming request payload and defers BinaryContent creation/disposal to enumeration time.
OpenAI/src/Custom/Audio/AudioClient.cs	Snapshots streaming speech request payload and defers BinaryContent creation/disposal to enumeration time.
OpenAI/src/Custom/Assistants/AssistantClient.cs	Snapshots streaming request payloads for assistants streaming operations and introduces CreateThreadAndRunProtocolData.

[jsquire]

Thank you for digging into this and putting together a clear, well-structured fix. The root cause analysis in the PR description is accurate and the approach is sound. We really appreciate the contribution.

Before we merge, we'd like to ask for one design adjustment to the infrastructure layer that we think will result in a cleaner, simpler outcome.

On the AsyncSseUpdateCollection / SseUpdateCollection changes

While reviewing, we looked more closely at how the broader codebase handles this same deferred-send pattern. CreateRunStreaming and its siblings already use a safe approach, calling ToBinaryContent() inside the lambda so the content is created fresh at enumeration time rather than at call time. That pattern avoids the disposal bug without any infrastructure changes.

We would like to take the spirit of your explicit-argument approach (which we like, because it avoids closure capture and makes data flow clear) and express it through an adapter constructor rather than adding new private fields and conditional dispatch to the base class. Something along these lines would do it:

public AsyncSseUpdateCollection(
    Func<BinaryData, Task<ClientResult>> sendRequestAsync,
    BinaryData requestData,
    Func<SseItem<byte[]>, IEnumerable<T>> eventDeserializerFunc,
    CancellationToken cancellationToken)
    : this(() => sendRequestAsync(requestData), eventDeserializerFunc, cancellationToken)
{
    Argument.AssertNotNull(sendRequestAsync, nameof(sendRequestAsync));
    Argument.AssertNotNull(requestData, nameof(requestData));
}

This achieves the same thing at the call site, a BinaryData snapshot at call time, explicit argument threading, no closures, while keeping the base class internals unchanged with no new nullable fields and no conditional in GetRawPagesAsync. The same pattern applies to SseUpdateCollection, and the call sites themselves would look identical to what you have today.

We think this is the right balance. It is correct, performant (serializes once at call time and defers allocation of BinaryContent to enumeration time), and readable without the added weight of a dual-path dispatch.

On TranscribeAudioStreaming[Async]

While reviewing we noticed that TranscribeAudioStreaming[Async] has a similar structure, where MultiPartFormDataBinaryContent is created outside the lambda before being captured. It is a different content type (multipart wrapping a live stream) so it is a separate design problem and we are not asking you to address it in this PR. We just wanted to flag it so you are aware. We will track it on our end, but if you would like to take a pass at it here that would be very welcome.

On // CUSTOM: comments

Could you add a brief // CUSTOM: comment on each of the modified streaming methods explaining the intent? Specifically that BinaryContent is intentionally deferred to the lambda body to ensure it is not disposed before enumeration begins. This is a non-obvious inversion of the naive pattern and the explanation will help future readers.

[jsquire]

Minor nit: requestBody is initialized to null and only assigned inside the transport callback. If the callback never fires due to a test setup issue, JsonDocument.Parse(requestBody) will throw a non-descriptive ArgumentNullException. Adding Assert.That(requestBody, Is.Not.Null, "Transport callback was never invoked") before the Parse call would give a much clearer failure message in that scenario.

Also, would you be willing to add a negative test here as well, one that constructs the collection with a using BinaryContent created outside the lambda (the original broken pattern) and confirms it produces an ObjectDisposedException or an empty/unusable request body at enumeration time? This would make the regression suite self-documenting so that someone reading the tests later can see both what the bug looked like and why the fix prevents it.

[jsquire]

Minor nit: requestBody is initialized to null and only assigned inside the transport callback. If the callback never fires due to a test setup issue, JsonDocument.Parse(requestBody) will throw a non-descriptive ArgumentNullException. Adding Assert.That(requestBody, Is.Not.Null, "Transport callback was never invoked") before the Parse call would give a much clearer failure message in that scenario.

Also, would you be willing to add a negative test here as well, one that constructs the collection with a using BinaryContent created outside the lambda (the original broken pattern) and confirms it produces an ObjectDisposedException or an empty/unusable request body at enumeration time? This would make the regression suite self-documenting so that someone reading the tests later can see both what the bug looked like and why the fix prevents it.

One additional item: SubmitToolOutputsToRunStreaming and its async variant are also fixed in this PR but do not yet have corresponding regression tests. Could you add coverage here, including the negative test, to match the treatment of the other three call sites?