Skip to content

test(security): cover TLS hosting and duplex pipes - #10963

Open
ReubenBond wants to merge 5 commits into
dotnet:mainfrom
ReubenBond:rb-issue-10865-close-coverage-gaps
Open

test(security): cover TLS hosting and duplex pipes#10963
ReubenBond wants to merge 5 commits into
dotnet:mainfrom
ReubenBond:rb-issue-10865-close-coverage-gaps

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Sep 2, 2026

Copy link
Copy Markdown
Member

Problem

Orleans.Connections.Security remains a high-risk long-tail package with low TLS, hosting, and duplex-pipe coverage. Handshake timeout configuration also accepted values which could fail later when constructing the runtime cancellation source.

Solution

  • add deterministic boundary and failure tests for TLS options, client/silo registration, duplex-pipe streams, APM adapters, disposal, cancellation, and middleware callbacks
  • expose package internals to the dedicated test assembly through a single friend-assembly declaration
  • preserve Timeout.InfiniteTimeSpan as the no-timeout sentinel and validate finite values through the runtime-supported maximum

Focused package coverage moves from 52.62% to 81.25% lines and from 35.29% to 83.70% branches. CRAP scores above 30 fall from five methods to two certificate-store helpers.

Refs #10865

Copilot AI lite review requested due to automatic review settings September 2, 2026 07:37

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

TlsOptions.HandshakeTimeout documentation is now misleading given the new infinite-timeout behavior and should be updated for correctness.

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

Review tier: Lite
Findings: 1 Low severity

New issues introduced by this change (1)
Severity Finding
Low severity src/​Orleans.Connections.Security/​Security/​TlsOptions.cs — CreateHandshakeCancellationTokenSource treats HandshakeTimeout==TimeSpan.MaxValue as an infinite…
What changed in this PR

Improves the reliability and coverage of Orleans.Connections.Security by adding deterministic unit tests around TLS options/hosting validation and duplex-pipe stream behavior, and by fixing an infinite-handshake-timeout runtime failure in the TLS middleware.

Changes:

  • Added extensive deterministic tests for TLS options defaults/boundaries, hosting UseTls validation/registration, duplex-pipe stream contracts, and Task/APM adapters.
  • Fixed TLS client/server middleware handshake timeout handling by using a non-timing CancellationTokenSource when the configured timeout is effectively infinite.
  • Exposed package internals to the dedicated test assembly via a single friend-assembly declaration and added .testagent research/plan/status artifacts.
File Description
test/​Orleans.Connections.Security.Tests/​TlsOptionsTests.cs Adds unit tests for TlsOptions defaults, timeout boundaries, validators, and auth option callbacks.
test/​Orleans.Connections.Security.Tests/​TlsMiddlewareTests.cs Verifies middleware applies base TLS options then invokes per-connection auth callbacks.
test/​Orleans.Connections.Security.Tests/​TaskToApmTests.cs Adds coverage for the internal Task-to-APM adapter behavior (sync/async completion, End/GetTask, failures).
test/​Orleans.Connections.Security.Tests/​SiloHostingExtensionsTests.cs Covers silo UseTls overload validation and connection-builder registrations (gateway/silo in/out).
test/​Orleans.Connections.Security.Tests/​PipeTestInfrastructure.cs Introduces deterministic pipe reader/writer fakes and multi-segment sequence helpers for stream tests.
test/​Orleans.Connections.Security.Tests/​HostingTestInfrastructure.cs Adds lightweight recording builders and helpers to observe configured connection options/middleware registration.
test/​Orleans.Connections.Security.Tests/​DuplexPipeStreamTests.cs Adds deterministic tests for DuplexPipeStream read/write/flush/copy, cancellation, disposal, and argument validation.
test/​Orleans.Connections.Security.Tests/​DuplexPipeStreamAdapterTests.cs Tests adapter wiring, leave-open semantics, and idempotent mixed sync/async disposal (including TlsDuplexPipe).
test/​Orleans.Connections.Security.Tests/​ClientHostingExtensionsTests.cs Covers client UseTls overload validation and outbound connection-builder registration/composition.
test/​Orleans.Connections.Security.Tests/​CertificateCreator.cs Adds helper to create certificates without private keys for deterministic validation tests.
src/​Orleans.Connections.Security/​Security/​TlsServerConnectionMiddleware.cs Uses a new handshake CTS factory to support “infinite” handshake timeouts without runtime exceptions.
src/​Orleans.Connections.Security/​Security/​TlsOptions.cs Adds CreateHandshakeCancellationTokenSource() to safely create handshake cancellation sources.
src/​Orleans.Connections.Security/​Security/​TlsClientConnectionMiddleware.cs Uses the new handshake CTS factory to support “infinite” handshake timeouts without runtime exceptions.
src/​Orleans.Connections.Security/​Orleans.Connections.Security.csproj Adds InternalsVisibleTo for Orleans.Connections.Security.Tests.
.testagent/​status.md Records generated-test outcome, coverage deltas, and validation commands/results.
.testagent/​research.md Captures test-generation research notes and scope/strategy for the package slice.
.testagent/​plan.md Captures the phased implementation plan and traceability checklist for the generated tests.

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

Comment thread src/Orleans.Connections.Security/Security/TlsOptions.cs
Copilot AI review requested due to automatic review settings September 2, 2026 08:03
@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

🔵 Needs a closer look

A few test/documentation issues should be corrected (notably TrackingStream.DisposeAsync not marking the wrapper disposed, and .testagent docs containing stale/incorrect statements) to avoid masking future regressions and confusing readers.

Review tier: Lite
Findings: None

Issues resolved since last review (1)
Severity Finding
Low severity src/​Orleans.Connections.Security/​Security/​TlsOptions.cs — CreateHandshakeCancellationTokenSource treats HandshakeTimeout==TimeSpan.MaxValue as an infinite… View resolved comment
Suppressed comments (4)

Previously missed (4) — in code that hasn't changed since the last review.

test/Orleans.Connections.Security.Tests/PipeTestInfrastructure.cs:257

  • TrackingStream.DisposeAsync increments counters and optionally disposes the inner stream, but it never marks the TrackingStream itself as disposed (it does not call the base Stream disposal path). This can mask bugs in disposal/idempotency tests because the wrapper remains usable after DisposeAsync.
    .testagent/plan.md:22
  • This plan says the only production change is InternalsVisibleTo, but this PR also changes production code (TlsOptions adds CreateHandshakeCancellationTokenSource and both TLS middleware types use it). The plan should be updated so it matches the actual change set.
    .testagent/research.md:4
  • This document includes a machine-specific absolute path which is not portable and is likely to go stale or leak local environment details. Prefer removing it or replacing it with a repo-relative path reference.
    .testagent/research.md:28
  • The stated scope says "Do not modify production", but this PR does include small production changes under src/Orleans.Connections.Security (handshake CTS helper + middleware updates). Updating this line will prevent confusion for future readers trying to reconcile the notes with the actual diff.

Copilot AI review requested due to automatic review settings September 2, 2026 08:26
@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

🟡 Changes recommended

The new handshake CTS helper still risks throwing for very large finite timeouts due to CancellationTokenSource(TimeSpan) range limits, and should be hardened or explicitly validated.

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

Review tier: Lite
Findings: 1 Medium severity

New issues introduced by this change (1)
Severity Finding
Medium severity src/​Orleans.Connections.Security/​Security/​TlsOptions.cs — CreateHandshakeCancellationTokenSource still calls new CancellationTokenSource(_handshakeTimeout)

Comment thread src/Orleans.Connections.Security/Security/TlsOptions.cs
Copilot AI review requested due to automatic review settings September 2, 2026 09:02
@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 change is narrowly scoped, directly addresses a concrete runtime failure mode, and is backed by comprehensive deterministic tests covering the new/updated behaviors.

Review tier: Lite
Findings: None

Issues resolved since last review (1)
Severity Finding
Medium severity src/​Orleans.Connections.Security/​Security/​TlsOptions.cs — CreateHandshakeCancellationTokenSource still calls new CancellationTokenSource(_handshakeTimeout)View resolved comment

Copilot AI review requested due to automatic review settings September 2, 2026 14:39
@ReubenBond
ReubenBond force-pushed the rb-issue-10865-close-coverage-gaps branch from 6c14bfe to fef4b8b Compare September 2, 2026 14:39
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