Skip to content

.NET: terminate chat completions SSE stream with data: [DONE] - #8532

Open
Manjunath Janardhan (manjunathshiva) wants to merge 4 commits into
microsoft:mainfrom
manjunathshiva:dotnet-chat-completions-done-sentinel-8526
Open

Manjunath Janardhan (manjunathshiva) wants to merge 4 commits into
microsoft:mainfrom
manjunathshiva:dotnet-chat-completions-done-sentinel-8526

Conversation

@manjunathshiva

Copy link
Copy Markdown
Contributor

Motivation & Context

The agent chat completions endpoint ends its SSE stream by simply running out of frames. It never
writes data: [DONE], which is the terminator the OpenAI wire format specifies and which every
other party in this repo already assumes:

  • A live Azure OpenAI chat completion with stream=true ends with exactly data: [DONE].
  • This repo's own recorded OpenAI trace for the endpoint ends with it too:
    tests/Microsoft.Agents.AI.Hosting.OpenAI.UnitTests/ConformanceTraces/ChatCompletions/streaming/response.txt.
  • [DONE] appears exactly once anywhere in dotnet/src, and it is a reader:
    Aspire.Hosting.AgentFramework.DevUI/SseResponseIdCapture.cs:91 matches "[DONE]"u8.

So the framework ships a consumer that waits for a sentinel the server never produces. Today callers
have to add their own middleware to append the frame, which is what the issue reports.

One correction to the issue, so reviewers are not looking for the wrong symptom: I could not
reproduce "clients will hang or timeout". The response uses Transfer-Encoding: chunked, so the
zero-length terminating chunk gives a definitive end-of-body, and the official Python openai SDK
completes cleanly against the unfixed endpoint. The real defect is protocol conformance — a
consumer that keys on the sentinel rather than end-of-body, such as the DevUI capture above, cannot
detect completion.

Description & Review Guide

  • What are the major changes?

    GetStreamingChunksAsync yields a null-payload sentinel after the agent stream completes, and the
    item formatter writes [DONE] for it. The item type widens to SseItem<ChatCompletionChunk?>;
    that method is private and ChatCompletionChunk is a record, so this is a nullable annotation
    only.

    The terminator goes through SseFormatter rather than being appended to response.Body after
    WriteAsync returns, for two reasons. It keeps SseFormatter the single owner of the data:
    prefix and the blank-line frame terminator, instead of copying that framing to a second site. And
    because the sentinel sits after the await foreach, an agent exception or an aborted request
    propagates out of the enumerator and no terminator is written — a client must never be able to read
    a truncated stream as a complete one.

  • What is the impact of these changes?

    One additional SSE frame at the end of a streaming chat completion, matching the upstream format.
    AIAgentChatCompletionsProcessor is internal, so there is no public API change. The
    non-streaming path is untouched.

    The Responses endpoint is deliberately unchanged. That API signals completion with typed
    response.* events — StreamingResponseCompleted at Responses/AgentResponseUpdateExtensions.cs:232
    — and its recorded trace contains no [DONE] frame, so adding one there would be wrong. AG-UI has
    its own protocol and is likewise untouched.

  • What do you want reviewers to focus on?

Whether the null-payload sentinel is the mechanism you want, or you would rather see an explicit
marker instance. And whether the terminator should be unconditional as implemented — the issue
author's own workaround was opt-in middleware, but making it configurable here would keep the
non-conforming behaviour as the default.

Two notes that may save a review cycle. The existing StreamingRequestResponseAsync could not have
caught this: it asserts through ParseChatCompletionChunksFromSse, which contains
// Skip [DONE] marker and continue (OpenAIChatCompletionsConformanceTests.cs:605, duplicated
at OpenAIChatCompletionsSerializationTests.cs:554), so it loads a trace ending in the terminator
and then discards the only line that would have failed. I left that shared parser alone — eight
call sites depend on it — and the new test asserts on the raw body instead. Separately, the new
test pins the full frame including its blank line, data: [DONE]\n\n; that matches live wire
behaviour and this endpoint's output, but note the recorded trace file itself has no trailing
newline, so the assertion cannot be derived from the trace.

Related Issue

Fixes #8526

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

The agent chat completions endpoint ended its SSE stream by simply closing the
connection. OpenAI-compatible clients treat a final `data: [DONE]` frame as the
completion signal, so they waited for a timeout or hung instead.

GetStreamingChunksAsync now yields a null-payload sentinel after the agent
stream completes, and the item formatter writes it as `[DONE]`. Routing the
terminator through SseFormatter keeps that component the single owner of the
`data: ` prefix and the blank-line frame terminator.

The sentinel is yielded after the loop, so it only follows a stream that ran to
completion: if the agent throws or the caller aborts, the exception propagates
out of the enumerator and no terminator is written. A client must never read a
truncated stream as a complete one.

The Responses endpoint is deliberately unchanged -- that API signals completion
with typed `response.*` events, and the recorded trace for it contains no
[DONE] frame.

The existing streaming conformance test could not catch this: it asserts
through ParseChatCompletionChunksFromSse, which skips the `[DONE]` line because
it is not JSON. The new test asserts on the raw SSE body instead.

Fixes microsoft#8526

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The failure-path invariant needs coverage ensuring truncated streams never emit [DONE].

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds OpenAI-compatible [DONE] termination to .NET chat-completion SSE streams.

Changes:

  • Emits [DONE] only after successful stream completion.
  • Adds raw-response conformance coverage.
File summaries
File Description
AIAgentChatCompletionsProcessor.cs Formats and emits the terminal SSE frame.
OpenAIChatCompletionsConformanceTests.cs Verifies sentinel framing and placement.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

… rationale comment

Two review comments from Copilot on microsoft#8532.

The sentinel comment asserted that clients "wait for the connection to drop,
which reads as a hang or a timeout". Live testing disproved that: the response
is chunked, so the zero-length terminating chunk ends the body definitively and
the official OpenAI client completes without the sentinel. The comment now
states the actual interoperability problem -- consumers that treat the sentinel
rather than end-of-body as the completion signal cannot recognize the stream as
finished -- and names the one this repo ships, SseResponseIdCapture.

The failure-path invariant is now covered. An earlier attempt at this test was
discarded because a client-side assertion has no teeth: when the agent throws,
the server aborts and the client receives no body, so the test passed even
against a try/finally refactor that writes the terminator unconditionally. This
version tees the response body into a buffer through test middleware, so it
observes what the server actually wrote. Verified to fail against that refactor.

The failing chat client and the tee stream are private to the test file rather
than added to the shared TestHelpers, keeping the change to one test file.
Review findings on the test added in 2240252. No behaviour change; the test
still fails against the try/finally append refactor it exists to block.

- Dispose the capture buffer (`using var recorded`), which was leaked.
- Hoist the trace load into Arrange, matching the Arrange/Act/Assert split the
  rest of the file uses; it was sitting in the Act block.
- Record why the host is built inline instead of through
  ConformanceTestBase.CreateTestServerAsync: this test needs response-body
  middleware and a throwing chat client, neither of which belongs in the shared
  harness for one caller. Without the note the duplication reads as an
  oversight and invites a refactor back into the base class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

.NET Usage: [Issues, PRs], Target: .Net

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.NET: [Bug]: Agent chat completions endpoint does not terminate SSE stream with data: [DONE]\n\n

2 participants