Fail fast on reverse connect listener startup - #4009
Merged
marcschier merged 6 commits intoJul 17, 2026
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR updates reverse-connect listener startup to fail fast, ensuring bind/open failures surface immediately (with actionable diagnostics) instead of causing later connection timeouts.
Changes:
- Stage and validate endpoint configuration before applying it; open listeners atomically and roll back on failure.
- Preserve and surface meaningful
ServiceResultExceptionstatus codes acrossStartServiceoverloads, including endpoint-specific failure diagnostics. - Improve configuration-watcher lifecycle handling (dispose replaced watchers; ignore stale watcher callbacks) and expand reverse-connect documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/Opc.Ua.Core.Tests/Stack/Client/ReverseConnectHostTests.cs | Adds coverage ensuring close failures trigger forced listener disposal and subsequent closes are harmless. |
| Tests/Opc.Ua.Client.Tests/ClientBuilder/ReverseConnectManagerTests.cs | Adds tests for invalid endpoint diagnostics, occupied-port failure behavior, rollback semantics, and watcher failure behavior. |
| Tests/Opc.Ua.Client.TestFramework/ClientFixture.cs | Recreates the manager after reverse-connect start failures to avoid polluted state between retries. |
| Stack/Opc.Ua.Core/Stack/Client/ReverseConnectHost.cs | Forces listener disposal when CloseAsync fails, improving rollback reliability. |
| Libraries/Opc.Ua.Client/ReverseConnectManager.cs | Implements staged endpoint validation, atomic listener startup with rollback, watcher disposal/staleness checks, and improved error propagation. |
| Docs/ReverseConnect.md | Documents shared-manager/shared-listener behavior, listener lifetime, diagnostics, and spec references. |
romanett
reviewed
Jul 16, 2026
romanett
reviewed
Jul 16, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4009 +/- ##
==========================================
- Coverage 73.72% 73.42% -0.31%
==========================================
Files 1340 1340
Lines 179309 179425 +116
Branches 31541 31556 +15
==========================================
- Hits 132188 131734 -454
- Misses 36385 37003 +618
+ Partials 10736 10688 -48
🚀 New features to boost your workflow:
|
5 tasks
romanett
approved these changes
Jul 17, 2026
marcschier
added a commit
that referenced
this pull request
Jul 17, 2026
# Description Fail reverse-connect listener startup immediately instead of logging bind failures and surfacing a later connection timeout. - Validate and stage configured and programmatic listener endpoints before replacing active configuration. - Open all listeners atomically and report endpoint-specific failures with `BadTcpEndpointUrlInvalid`, the transport-specific status, or `BadNoCommunication`. - Roll back listeners opened by a failed attempt without reacquiring the manager lock, including forced listener disposal when normal close fails. - Preserve meaningful `ServiceResultException` status codes through both `StartService` overloads. - Dispose replaced configuration watchers and reject callbacks from stale watcher generations. - Keep the intended shared-listener behavior: one long-lived `ReverseConnectManager` can accept connections from multiple Servers on the same Client URL. - Expand `Docs/ReverseConnect.md` with listener lifetime, shared-manager usage, diagnostics, and specification references. ## Validation - `ReverseConnectManagerTests`: net10.0 and net48. - `ReverseConnectHostTests`: net10.0 and net48. - Ordered reverse-connect listener reuse integration tests: net10.0. ## Related Issues - Relates to #3985. ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received.
marcschier
added a commit
that referenced
this pull request
Jul 23, 2026
## Summary - add cancellation-aware asynchronous start, lazy initialization, stop, reload, and disposal for `ReverseConnectManager` - make listener replacement transactional with explicit ownership, rollback/restoration, stale watcher protection, and deterministic concurrent start/stop/dispose behavior - add injectable async configuration providers, remove the former protected configuration hooks, and retain obsolete synchronous lifecycle compatibility wrappers - integrate eager Generic Host startup plus non-host lazy startup, including custom application-configuration providers - serialize `ReverseConnectHost` lifecycle and extend raw TCP, Kestrel TCP, WSS, hosting, concurrency, rollback, and NativeAOT coverage - document the async lifecycle, hosting behavior, cancellation guarantees, provider model, and migration path ## Validation - `Opc.Ua.Client` Release build across all supported target frameworks - reverse-connect client and core tests on net10.0 and net48 - raw TCP, Kestrel TCP, and WSS integration tests on net10.0; raw TCP on net48 - NativeAOT publish and 115-test executable run Closes #4013. Related: #3985, #4009.
marcschier
deleted the
marcschier/fix-reverse-connect-listener-diagnostics
branch
July 24, 2026 10:42
This was referenced Aug 30, 2026
marcschier
pushed a commit
that referenced
this pull request
Aug 30, 2026
…#4341) # Description Fixes the reverse connect problem reported in #3985: a Client which uses one reverse connect listener port for several Servers connects to the first Server, and every following `WaitForConnectionAsync` times out. A separate listener port per Server was the only workaround. ## Root cause Confirming the analysis in [mrsuciu's comment on #3985](#3985): - Servers which send `ReverseHello` before the application registered a waiting connection for them are held in `OnConnectionWaitingAsync` for `HoldTime`. - All held connections share a single `CancellationTokenSource`, so `RegisterWaitingConnection` wakes up **every** held connection, not only the one it was registered for. - The hold loop was `while (!matched) { ... break; }` with an unconditional `break`, so each woken connection got exactly one re-match attempt. The connections which did not match the new registration fell out of the loop unaccepted, although most of their own hold time was left. - An unaccepted connection is force faulted by the listener (`TcpReverseConnectChannel`, "The reverse connection was rejected by the client"), so that Server is dropped and only returns on its next `ReverseHello` interval, long after the Client timed out waiting for it. Separate ports avoid the shared wakeup path, which is exactly why that workaround succeeds. ## Changes - `ReverseConnectManager.OnConnectionWaitingAsync` re-arms the hold after a wakeup: it re-matches, and if the registration which caused the wakeup was for another Server the connection keeps waiting for the remainder of its own hold time. The loop is still bounded by `HoldTime`, so a Server which is never registered for is rejected exactly as before. - A shutdown flag set by `StopService`/`Dispose` (cleared by `StartService`) releases held connections immediately during teardown, instead of parking them until the hold time expires. `Dispose` now also cancels the token source before disposing it, so a held callback can no longer read a disposed token. - `Docs/ReverseConnect.md`: new "Sharing a listener across multiple Servers" section documenting that the listener port staying in `LISTENING` is expected, that one shared `ReverseConnectManager` should serve all Servers on the same Client Url, and the `HoldTime`/`WaitTimeout` semantics. No public API, signature or serialization change. ## Validation - New `ReverseConnectManagerUnitTests` (3 tests): the two Server regression, hold time expiry still rejecting, and dispose releasing a held connection. They drive the callback through an internal test hook, so no listener or socket is involved. Verified that the regression test fails against the previous behaviour with "The reverse connection of the second server was released before its hold time expired." - New tests pass on net10.0 and net48. - Full reverse connect suite (`FullyQualifiedName~ReverseConnect`, including the existing `ReverseConnectTest` server fixture): 33/33 pass on net10.0. ## Related Issues - Relates to #3985. - `master` carries the identical defect: the same `break` survived #4009 and #4059 and is still present in `src/Opc.Ua.Client/ReverseConnectManager.cs`. The equivalent fix for `master` is #4342 (it uses `TimeProvider` and breaks on a pending callback drain instead of a new flag), so the issue is intentionally not auto closed here. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
marcschier
added a commit
that referenced
this pull request
Aug 30, 2026
…#4342) # Description Fixes the reverse connect problem reported in #3985 on `master`. #4009 and #4059 addressed the listener startup diagnostics and the async lifecycle, but the actual hold time defect described in [mrsuciu's comment on the issue](#3985) survived both refactors unchanged. Symptom: a Client which uses one reverse connect listener port for several Servers connects to the first Server, and every following `WaitForConnectionAsync` times out. A separate listener port per Server was the only workaround. ## Root cause - Servers which send `ReverseHello` before the application registered a waiting connection for them are held in `OnConnectionWaitingAsync` for `HoldTime`. - All held connections share a single `CancellationTokenSource`, so `RegisterWaitingConnection`/`WaitForConnectionAsync` wakes up **every** held connection, not only the one it was registered for. - The hold loop was `while (!matched) { ... break; }` with an unconditional `break`, so each woken connection got exactly one re-match attempt. The connections which did not match the new registration fell out of the loop unaccepted, although most of their own hold time was left. - An unaccepted connection is force faulted by the listener (`TcpReverseConnectChannel`, "The reverse connection was rejected by the client"), so that Server is dropped and only returns on its next `ReverseHello` interval, long after the Client timed out waiting for it. Separate ports avoid the shared wakeup path, which is exactly why that workaround succeeds. ## Changes - `OnConnectionWaitingAsync` re-arms the hold after a wakeup: it re-matches, and if the registration which caused the wakeup was for another Server the connection keeps waiting for the remainder of its own hold time. The loop is still bounded by `HoldTime`, so a Server which is never registered for is rejected exactly as before. - New `IsCallbackDrainPending()` check before the hold is re-armed. `DrainConnectionCallbacksAsync` cancels the hold token without renewing it, so a re-armed hold would spin on the already cancelled token until the hold time expires and would block the drain until then. With the check a held callback releases its transport immediately, which keeps the existing `StopDrainsHeldConnectionCallbackBeforeListenerClose` and `DisposeDrainsHeldConnectionCallbackBeforeListenerDisposal` guarantees intact. - `docs/ReverseConnect.md`: document the `HoldTime` semantics in the "Sharing a listener across multiple Servers" section. No public API change. ## Validation - New `HeldConnectionSurvivesRegistrationForAnotherServer` test in `ReverseConnectManagerLifecycleTests` drives two held callbacks through `InvokeConnectionWaitingForTest` and proves the second Server keeps its connection when the first Server is registered. Verified that it fails against the previous behaviour with "the reverse connection of the second Server was released before its hold time expired". - `FullyQualifiedName~ReverseConnect` (183 tests, including `ReverseConnectManagerTests`, `ReverseConnectManagerLifecycleTests` and `ReverseConnectHostTests`): all pass on net10.0 and net48. ## Related Issues - Relates to #3985. - The same fix for the LTS branch is #4341 (base `master378`). ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fail reverse-connect listener startup immediately instead of logging bind failures and surfacing a later connection timeout.
BadTcpEndpointUrlInvalid, the transport-specific status, orBadNoCommunication.ServiceResultExceptionstatus codes through bothStartServiceoverloads.ReverseConnectManagercan accept connections from multiple Servers on the same Client URL.Docs/ReverseConnect.mdwith listener lifetime, shared-manager usage, diagnostics, and specification references.Validation
ReverseConnectManagerTests: net10.0 and net48.ReverseConnectHostTests: net10.0 and net48.Related Issues
Checklist