Skip to content

test(streaming): cover subscription lifecycle - #10957

Open
ReubenBond wants to merge 2 commits into
dotnet:mainfrom
ReubenBond:rb-10862-raise-streaming-coverage
Open

test(streaming): cover subscription lifecycle#10957
ReubenBond wants to merge 2 commits into
dotnet:mainfrom
ReubenBond:rb-10862-raise-streaming-coverage

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Sep 2, 2026

Copy link
Copy Markdown
Member

Problem

The stream subscription lifecycle had a CRAP 420 delivery hotspot and limited direct coverage of ordered delivery, retries, terminal callbacks, resume, unsubscribe, and invalid-handle behavior.

Solution

  • Add deterministic in-memory tests for item and batch identity, ordering, event indexes, diagnostics, observer failures, retries, completion, errors, resume, unsubscribe, and provider-operation invariants.
  • Compare item delivery acknowledgements using a delivery-handshake token, matching batch delivery and preventing duplicate redelivery.
  • Validate an invalidated concrete handle before reading its cleared stream identity, preserving the documented ArgumentException contract.

Rationale

The tests exercise lifecycle guarantees directly without external providers or timing dependencies. The selected StreamSubscriptionHandleImpl target now exceeds 90% line and branch coverage and the CRAP 420 hotspot is eliminated.

Contributes to #10862.

Microsoft Reviewers: Open in CodeFlow

Copilot AI lite review requested due to automatic review settings September 2, 2026 05:25

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.

Copilot review overview

🟡 Changes recommended

The new lifecycle test file contains non-compiling mixed named/positional argument calls, and there is also a per-item allocation in a hot-path duplicate-delivery check which should be addressed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 High severity · 1 Medium severity · 1 Low severity

New issues introduced by this change (3)
Severity Finding
High severity test/​Orleans.Streaming.Tests/​StreamingTests/​StreamConsumerLifecycleTests.cs — This SetObserver call mixes named and positional arguments (batchObserver: null followed by…
Medium severity src/​Orleans.Streaming/​Internal/​StreamSubscriptionHandleImpl.cs — This duplicate-delivery check allocates a new DeliveryToken for every delivered item…
Low severity test/​Orleans.Streaming.Tests/​StreamingTests/​StreamSubscriptionHandleImplTests.csthrow error; resets the exception stack trace, making failures harder to diagnose. If the intent…
What changed in this PR

This PR strengthens Orleans streaming subscription lifecycle coverage with deterministic in-memory tests, while also adjusting runtime behavior around handshake/duplicate-delivery handling and invalid-handle validation.

Changes:

  • Expand StreamSubscriptionHandleImpl test coverage for ordered delivery handshakes, retries, diagnostics emission, completion/error callbacks, resume/unsubscribe, and invalid-handle behavior.
  • Add new lifecycle-focused tests for StreamConsumer/StreamConsumerExtension registration, resume, and unsubscribe flows (including failure/retry paths).
  • Fix handshake comparison logic in StreamSubscriptionHandleImpl.DeliverItem and adjust StreamConsumer.CheckHandleValidity to reject invalid concrete handles before accessing cleared identity.
File Description
test/​Orleans.Streaming.Tests/​StreamingTests/​StreamSubscriptionHandleImplTests.cs Adds extensive deterministic tests for delivery/handshake semantics, retries, diagnostics, and invalidation behavior.
test/​Orleans.Streaming.Tests/​StreamingTests/​StreamConsumerLifecycleTests.cs New tests covering consumer resume/unsubscribe lifecycle behavior and failure/retry invariants.
src/​Orleans.Streaming/​Internal/​StreamSubscriptionHandleImpl.cs Updates duplicate-delivery detection to compare against a delivery-handshake token.
src/​Orleans.Streaming/​Internal/​StreamConsumer.cs Validates invalidated concrete handles before accessing stream identity to preserve the intended ArgumentException contract.
Suppressed comments (1)

test/Orleans.Streaming.Tests/StreamingTests/StreamConsumerLifecycleTests.cs:352

  • This SetObserver call uses a named argument (observer: null) followed by positional arguments, which will not compile. Pass arguments positionally (or name them all) so the batch observer is supplied correctly.
                Stream,
                observer: null,
                observer,
                token,
                filterData);

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

Comment thread test/Orleans.Streaming.Tests/StreamingTests/StreamConsumerLifecycleTests.cs Outdated
Comment thread src/Orleans.Streaming/Internal/StreamSubscriptionHandleImpl.cs Outdated
Copilot AI review requested due to automatic review settings September 2, 2026 06:36
@ReubenBond

Copy link
Copy Markdown
Member Author

@copilot review

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.

Copilot review overview

🟢 Approval recommended

The production changes are small and targeted, and the new deterministic tests comprehensively validate the intended lifecycle and handshake behaviors.

Review tier: Lite
Findings: None

Issues resolved since last review (3)
Severity Finding
Low severity test/​Orleans.Streaming.Tests/​StreamingTests/​StreamSubscriptionHandleImplTests.csthrow error; resets the exception stack trace, making failures harder to diagnose. If the intent… View resolved comment
Medium severity src/​Orleans.Streaming/​Internal/​StreamSubscriptionHandleImpl.cs — This duplicate-delivery check allocates a new DeliveryToken for every delivered item… View resolved comment
High severity test/​Orleans.Streaming.Tests/​StreamingTests/​StreamConsumerLifecycleTests.cs — This SetObserver call mixes named and positional arguments (batchObserver: null followed by… View resolved comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants