Fix HttpStream Close() race + AddStreamFinal() streamType override by corinagum · Pull Request #474 · microsoft/teams.net

corinagum · 2026-05-04T17:55:13Z

Fixes #473

Summary

Two bugs in the streaming path. Parity of microsoft/teams.ts#553, but Bug 1 manifests via a different mechanism in C# (??=) than in TS.

Bug 1: `AddStreamFinal()` cannot override existing `ChannelData.StreamType`

MessageActivity.AddStreamFinal() used null-coalescing assignment:

ChannelData.StreamType ??= StreamType.Final;

In the streaming Close() path:

Stream.Update("Thinking...") accumulates StreamType.Informative into _channelData.
Close() calls WithData(_channelData), populating activity.ChannelData.StreamType = Informative.
AddStreamFinal() runs ??= against Informative → no-op. The wire channelData.streamType stays as "informative" on the final message, even though the streaminfo entity correctly has "final".

Fix: direct assignment. The method's contract per its name is to mark the activity as a final stream message, so it must be able to override prior values.

Note: This is a minor public-API behavior change. Any caller that pre-set ChannelData.StreamType and then called AddStreamFinal() expecting their value to persist will now have it overridden. The method's name and contract make the override semantic the obvious one; we judged this acceptable.

Bug 2: Race condition in `Stream.Close()`

Close()'s wait loop checked _id and _queue but not whether Flush() was holding _lock. During a flush mid-await (queue drained, SendActivity() awaits pending), Close() would exit the loop and send the final activity, racing the in-flight chunk.

Fix: also wait while _lock.CurrentCount == 0 (semaphore held). This is the same primitive already used in Close()'s early-return guard on line 88.

Changes

Libraries/Microsoft.Teams.Api/Activities/Message/MessageActivity.cs — AddStreamFinal() direct assignment.
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs — Close() lock-aware wait.
Tests in MessageActivityTests.cs and AspNetCorePluginStreamTests.cs.

Test plan

AddStreamFinal_OverridesExistingStreamType — MessageActivity with StreamType.Informative pre-set, AddStreamFinal() overrides to Final.
Stream_Close_FinalMessageHasStreamTypeFinal_AfterInformativeUpdate — full Update→Emit→Close flow asserts final activity has StreamType.Final on both ChannelData and the streaminfo entity.
Stream_Close_WaitsForInFlightFlushToComplete — uses a controllable Send delegate to block the second flush mid-await; asserts Close() does not progress until the lock releases.
All existing MessageActivityTests (21 passed) and AspNetCorePluginStreamTests (6 passed) still pass on net10.0. (net8.0 testhost not installed locally; CI will cover.)
Full solution builds clean.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes two streaming-path issues: ensuring final stream activities always carry StreamType.Final, and making Stream.Close() wait for an in-flight flush before sending the final activity.

Changes:

Update MessageActivity.AddStreamFinal() to assign final stream metadata directly.
Update AspNetCorePlugin.Stream.Close() to wait while a flush still holds the semaphore.
Add tests covering final-message stream type behavior and the Close()/Flush() race.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
Tests/Microsoft.Teams.Plugins.AspNetCore.Tests/AspNetCorePluginStreamTests.cs	Adds integration-style tests for final stream metadata and the in-flight flush race.
Tests/Microsoft.Teams.Api.Tests/Activities/Message/MessageActivityTests.cs	Adds a unit test for `AddStreamFinal()` overriding an existing stream type.
Libraries/Microsoft.Teams.Plugins/Microsoft.Teams.Plugins.AspNetCore/AspNetCorePlugin.Stream.cs	Changes `Close()` to also wait while the flush semaphore is held.
Libraries/Microsoft.Teams.Api/Activities/Message/MessageActivity.cs	Changes `AddStreamFinal()` from null-coalescing assignment to direct assignment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- AddStreamFinal: keep StreamId as ??= (don't clobber an existing value) - Stream tests: replace Task.Delay-based waits with TaskCompletionSource signals from the Send delegate; simplify Bug 1 integration test to rely on Close()'s wait loop instead of fixed sleeps

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- AddStreamFinal: keep StreamId as ??= (don't clobber an existing value) - Stream tests: replace Task.Delay-based waits with TaskCompletionSource signals from the Send delegate; simplify Bug 1 integration test to rely on Close()'s wait loop instead of fixed sleeps

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

Libraries/Microsoft.Teams.Api/Activities/Message/MessageActivity.cs:217

AddStreamFinal() sets StreamInfoEntity.StreamId from Id, but leaves ChannelData.StreamId as ??= Id. If a caller pre-sets ChannelData.StreamId (or Id is null), this can produce a mismatch between channelData.streamId and the streaminfo entity’s streamId. Consider making ChannelData.StreamId assignment consistent with the entity (or vice versa) so the final activity is internally consistent.

        ChannelData ??= new();
        ChannelData.StreamId ??= Id;
        ChannelData.StreamType = StreamType.Final;

        AddEntity(new StreamInfoEntity()
        {
            StreamId = Id,
            StreamType = StreamType.Final

lilyydu

LGTM though copilot's comments seem worthy to evaluate

…de (#473) Two bugs in the streaming path: 1. AddStreamFinal() used `??=` for ChannelData.StreamId / StreamType, so it could not override an existing StreamType. The streaming Close() path accumulates `_channelData.StreamType = Informative` from prior typing updates and merges that into the final activity via WithData() before calling AddStreamFinal() — which then no-op'd, leaving the wire `channelData.streamType` as "informative" on the final message even though the streaminfo entity correctly said "final". Fix: direct assignment in AddStreamFinal(). The method's contract per its name is to mark the activity as a final stream message, so it must be able to override prior values. 2. Stream.Close() polled `_id` and `_queue` but not `_lock` — during a flush mid-await (queue drained, SendActivity awaits pending), Close() would proceed and send the final message before the in-flight chunk completed. Fix: also wait while `_lock.CurrentCount == 0`. This is the same primitive already used in Close()'s early-return guard.

- AddStreamFinal: keep StreamId as ??= (don't clobber an existing value) - Stream tests: replace Task.Delay-based waits with TaskCompletionSource signals from the Send delegate; simplify Bug 1 integration test to rely on Close()'s wait loop instead of fixed sleeps

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Agent-Logs-Url: https://github.com/microsoft/teams.net/sessions/ebcb4148-b60f-496d-8bab-fdc354e17b10 Co-authored-by: corinagum <[email protected]>

Copilot AI review requested due to automatic review settings May 4, 2026 17:55

Copilot AI reviewed May 4, 2026

View reviewed changes

Copilot started reviewing on behalf of corinagum May 4, 2026 18:15 View session

Copilot AI review requested due to automatic review settings May 5, 2026 17:14

corinagum force-pushed the cg/http-stream-race-and-streamtype-fix branch from 75b0c96 to 5ceed67 Compare May 5, 2026 17:14

Copilot started reviewing on behalf of corinagum May 5, 2026 17:14 View session

Copilot AI reviewed May 5, 2026

View reviewed changes

corinagum force-pushed the cg/http-stream-race-and-streamtype-fix branch from 5ceed67 to 336a647 Compare May 6, 2026 22:11

Copilot AI review requested due to automatic review settings May 6, 2026 23:14

corinagum force-pushed the cg/http-stream-race-and-streamtype-fix branch from 336a647 to 94cdba9 Compare May 6, 2026 23:14

Copilot started reviewing on behalf of corinagum May 6, 2026 23:15 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread Tests/Microsoft.Teams.Api.Tests/Activities/Message/MessageActivityTests.cs

Comment thread Tests/Microsoft.Teams.Plugins.AspNetCore.Tests/AspNetCorePluginStreamTests.cs Outdated

Comment thread Tests/Microsoft.Teams.Plugins.AspNetCore.Tests/AspNetCorePluginStreamTests.cs

lilyydu approved these changes May 7, 2026

View reviewed changes

corinagum added 3 commits May 7, 2026 10:41

Apply Copilot feedback

acb6187

Copilot AI review requested due to automatic review settings May 7, 2026 17:41

corinagum force-pushed the cg/http-stream-race-and-streamtype-fix branch from 3ce81e0 to acb6187 Compare May 7, 2026 17:41

Copilot started reviewing on behalf of corinagum May 7, 2026 17:43 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

lilyydu approved these changes May 7, 2026

View reviewed changes

corinagum merged commit 5dbe296 into main May 7, 2026
12 checks passed

corinagum deleted the cg/http-stream-race-and-streamtype-fix branch May 7, 2026 17:58

Copilot AI mentioned this pull request May 7, 2026

Change AttachmentLayout from list|carousel to list|grid #193

Closed

dependabot Bot mentioned this pull request May 20, 2026

Bump the all-dependencies group with 2 updates OfficeDev/Microsoft-Teams-Samples#2275

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix HttpStream Close() race + AddStreamFinal() streamType override#474

Fix HttpStream Close() race + AddStreamFinal() streamType override#474
corinagum merged 3 commits into
mainfrom
cg/http-stream-race-and-streamtype-fix

corinagum commented May 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lilyydu left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

corinagum commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Bug 1: AddStreamFinal() cannot override existing ChannelData.StreamType

Bug 2: Race condition in Stream.Close()

Changes

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lilyydu left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

corinagum commented May 4, 2026 •

edited

Loading

Bug 1: `AddStreamFinal()` cannot override existing `ChannelData.StreamType`

Bug 2: Race condition in `Stream.Close()`