Skip to content

Port session activation race condition fix from master378 to master#3846

Merged
marcschier merged 5 commits into
masterfrom
copilot/port-fix-from-3844
Jun 15, 2026
Merged

Port session activation race condition fix from master378 to master#3846
marcschier merged 5 commits into
masterfrom
copilot/port-fix-from-3844

Conversation

Copilot AI commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes a null reference exception that occurs when a session expires between CreateSession and ActivateSession requests. The issue stems from a race condition where the server doesn't properly validate session existence during activation when the session has already timed out.

The fix is a minimal, targeted change in StandardServer.ActivateSessionAsync: a null-coalescing throw ensures the session exists after lookup, returning BadSessionIdInvalid if it has expired.

Example flow that previously failed:

// Session created successfully
var createResp = await client.CreateSessionAsync(...);

// Session expires before activation
await Task.Delay(sessionTimeout);

// Previously: NRE from null session
// Now: BadSessionIdInvalid returned correctly
var activateResp = await client.ActivateSessionAsync(...);

Note on scope

Earlier revisions of this port also modified SessionManager.ValidateRequestAsync (removing ActivateSession from the early-return bypass) and Session.ValidateRequest (allowing ActivateSession on non-activated sessions). These changes were reverted because they cause the secure channel id check in Session.ValidateRequest to run on reactivation, breaking valid OPC UA reconnect-on-alternate-channel scenarios — the same regression flagged in the source PR #3844. Symptoms included BadSecureChannelIdInvalid failures in tests such as ApplicationCertificateRotationRehandshakesOpenManagedChannelsAsync, ReconnectJWTAsync, and ReconnectSessionOnAlternateChannelAsync.

Related Issues

Ports the fix from PR #3844 which addressed this issue on the master378 branch.

Checklist

  • I have signed the CLA and read the CONTRIBUTING doc.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added all necessary documentation.
  • 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.

@CLAassistant

CLAassistant commented Jun 5, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ marcschier
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI changed the title Port fixes from PR #3844 to master: Fix NRE in ActivateSession Port session activation race condition fix from master378 to master Jun 5, 2026
Copilot AI requested a review from romanett June 5, 2026 09:12
@codecov

codecov Bot commented Jun 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.73%. Comparing base (f358078) to head (af666f6).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3846      +/-   ##
==========================================
- Coverage   72.74%   72.73%   -0.01%     
==========================================
  Files         913      913              
  Lines      148285   148286       +1     
  Branches    25662    25663       +1     
==========================================
- Hits       107863   107854       -9     
- Misses      31067    31077      +10     
  Partials     9355     9355              
Files with missing lines Coverage Δ
Libraries/Opc.Ua.Server/Server/StandardServer.cs 76.72% <100.00%> (-0.04%) ⬇️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marcschier
marcschier marked this pull request as ready for review June 13, 2026 15:58
marcschier and others added 2 commits June 13, 2026 17:58
…annelIdInvalid on reconnect

PR #3844 explicitly warned that removing the ActivateSession bypass from SessionManager.ValidateRequestAsync (and the matching guard in Session.ValidateRequest) causes the secure channel id check in Session.ValidateRequest to run on reactivation, breaking valid OPC UA reconnect-on-alternate-channel scenarios (e.g. ApplicationCertificateRotationRehandshakesOpenManagedChannelsAsync, ReconnectJWTAsync, ReconnectSessionOnAlternateChannelAsync).

Keep only the StandardServer.cs null-guard from #3844, which is the actual race-condition fix.
@marcschier
marcschier merged commit 03ed5c8 into master Jun 15, 2026
136 of 137 checks passed
@marcschier
marcschier deleted the copilot/port-fix-from-3844 branch June 15, 2026 06:29
marcschier added a commit that referenced this pull request Jun 15, 2026
Single upstream commit: 03ed5c8 'Port session activation race
condition fix from master378 to master (#3846)' — touches
Libraries/Opc.Ua.Server/Server/StandardServer.cs only. Auto-merged
cleanly with this branch's StandardServer.cs changes (ServerBase
constructor + TransportBindingRegistry parameter wiring).

Build verified: 'dotnet build UA.slnx -c Debug -p:CustomTestTarget=net10.0'
succeeds with 0 errors.
marcschier added a commit to marcschier/UA-.NETStandard that referenced this pull request Jul 9, 2026
Brings in 4 upstream commits: OPCFoundation#3879 (dotnet format sweep — whitespace +
IDE + RCS), OPCFoundation#3884 (re-enable FuzzCrashAssets + harden BinaryDecoder),
OPCFoundation#3846 (port session activation race fix from master378), and OPCFoundation#3745
(split MigrationGuide.md, add What's New in 2.0, rewrite Profiles.md).

No textual merge conflicts. The format sweep (OPCFoundation#3879) tightened the
repo-root Roslynator analyzer configuration, which newly flagged 27
pre-existing UaLens style sites across 14 files (RCS1078 use
string.Empty, RCS0031 enum member on own line, RCS1089 use ++
operator, RCS1006/RCS1061 merge if/else, RCS1031 remove unnecessary
braces, RCS1051 remove parentheses, RCS0061 remove blank line between
switch sections). Auto-fixed via `dotnet format analyzers` (matching
how upstream applied the sweep) plus one manual RCS0061 fix in
HistorianPlugin that surfaced after the brace removal.

UaLens build clean: 0 errors. The 16 remaining warnings are upstream
NuGet TFM notices (Microsoft.Extensions.* 10.6.0 not supporting
net48/net472 via Opc.Ua.Gds.Client.Common), unrelated to UaLens.

Co-authored-by: Copilot <[email protected]>
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.

4 participants