Skip to content

Hand tracked acknowledgements over when a subscription transfers to another session - #4942

Merged
marcschier merged 8 commits into
masterfrom
marcschier-handover-acks-on-transfer
Oct 30, 2026
Merged

marcschier merged 8 commits into
masterfrom
marcschier-handover-acks-on-transfer

Conversation

@marcschier

Copy link
Copy Markdown
Collaborator

Description

With an acknowledgement observer or provider configured, transferring a classic Subscription to a different Session did not hand its tracked acknowledgements to the new session's engine. Outcomes were reported twice, and in AfterConfirmation mode the confirmations were lost.

Now the previous engine releases the subscription's tracked acknowledgements without an outcome. The new engine tracks those whose message the server still holds and keeps the confirmation, and ends the others as NotRetained, so each acknowledgement ends exactly once. The new session's configuration governs; if it has no acknowledgement configuration, the previous session ends them as OwnerRetired. docs/Subscriptions.md replaces the known limitation with this behavior.

Related Issues

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.

Validation: dotnet test tests\Opc.Ua.Client.Tests\Opc.Ua.Client.Tests.csproj -f net10.0 --filter "FullyQualifiedName~ClassicSubscriptionEngineTests|FullyQualifiedName~Acknowledgement" passed (124/124). net48 and the full solution were not run.

@marcschier
marcschier marked this pull request as ready for review October 30, 2026 12:05
Copilot AI balanced review requested due to automatic review settings October 30, 2026 12:05

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.

🟡 Changes recommended

Critical transfer races can acknowledge messages before replay retrieves them and erase confirmations during session handover, causing replay failures and duplicate acknowledgement outcomes.

2 open issues
What changed in this PR

This PR addresses #4930 by handing tracked acknowledgements between classic subscription engines when a subscription moves to another session, aiming to preserve confirmations and prevent duplicate outcomes.

Changes:

  • Adds release and adoption logic for tracked acknowledgements.
  • Integrates handover into session transfers, with destination configuration governing subsequent handling.
  • Adds an engine-level regression test and documents cross-session behavior.
File Description
tests/​Opc.Ua.Client.Tests/​Session/​ClassicSubscriptionEngineTests.cs Tests acknowledgement release, adoption, and retained-message tracking.
src/​Opc.Ua.Client/​Subscription/​Acknowledgements/​IAcknowledgementTracker.cs Adds a release contract that returns confirmation state.
src/​Opc.Ua.Client/​Subscription/​Acknowledgements/​ConfirmedAcknowledgementProvider.cs Implements release without reporting an outcome.
src/​Opc.Ua.Client/​Session/​Subscription/​ClassicSubscriptionEngine.cs Adds acknowledgement release and adoption logic.
src/​Opc.Ua.Client/​Session/​Session.cs Invokes handover after subscription transfer.
docs/​Subscriptions.md Replaces the known limitation with handover and configuration behavior.

Review effort: Balanced

Comment thread src/Opc.Ua.Client/Session/Session.cs Resolved
@marcschier
marcschier enabled auto-merge (squash) October 30, 2026 12:14
@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Code coverage

✅ Coverage gate passed.

Check Result Threshold
✅ Project line rate 88.98% (456470/513026 lines) >= 70.00%
✅ Project branch rate 79.45% >= 60.00%
✅ Patch coverage 81.30% (287/353 changed lines) >= 75.00% (> 100 changed lines)
ℹ️ Baseline delta (advisory) +15.38 pp 73.60% recorded
Uncovered changed lines
  • src/Opc.Ua.Client/Session/Subscription/ClassicSubscriptionEngine.cs: 773, 774, 775, 1414, 1415, 1485, 1486, 1717, 1971, 1989, 1990
  • src/Opc.Ua.Client/Session/Session.cs: 2764, 2766, 2768, 4971, 4972, 4974, 4976, 4986, 5010, 5011, 5018, 5019, 5021, 5024, 5026, 5027, 5028, 5029, 5030, 5032, 5034, 5037, 5057, 5060, 5063, 5065, 5067, 5068, 5069, 5070
  • src/Opc.Ua.Client/Subscription/Classic/Subscription.cs: 1969, 1971, 1982, 1983, 1984, 1985, 1986, 1988, 1990, 1992, 1993, 1995, 1998, 2000, 2002, 2003, 2005, 2126, 2128, 2133, 2135, 2136, 2138, 2139, 2141

Coverage is above the recorded baseline - consider ratcheting coverage-thresholds.json.

Thresholds live in coverage-thresholds.json. Whole report before exclusions: line 88.29%, branch 78.78%.

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.

🟡 Changes recommended

Acknowledgement lifecycle bugs can lose confirmations and allow duplicate terminal outcomes after delayed Republish responses, while a throwing subscription-change handler can leave released handles without any terminal outcome.

2 open issues
2 resolved since last review

Review effort: Balanced

Comment thread src/Opc.Ua.Client/Session/Session.cs Resolved

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.

🟡 Changes recommended

Unresolved acknowledgement and republish lifecycle bugs can produce duplicate terminal outcomes, retain deleted subscriptions, and leave transferred-message reservations stuck.

4 open issues
2 resolved since last review

Review effort: Balanced

@marcschier
marcschier merged commit 4969d9d into master Oct 30, 2026
157 checks passed
@codecov

codecov Bot commented Oct 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.07082% with 88 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.37%. Comparing base (402b801) to head (ad332d9).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/Opc.Ua.Client/Session/Session.cs 52.77% 30 Missing and 4 partials ⚠️
...Opc.Ua.Client/Subscription/Classic/Subscription.cs 53.84% 24 Missing and 6 partials ⚠️
.../Session/Subscription/ClassicSubscriptionEngine.cs 88.83% 13 Missing and 11 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #4942    +/-   ##
========================================
  Coverage   83.37%   83.37%            
========================================
  Files        3525     3525            
  Lines      512661   513026   +365     
  Branches    87203    87285    +82     
========================================
+ Hits       427428   427743   +315     
- Misses      56277    56323    +46     
- Partials    28956    28960     +4     
Flag Coverage Δ
actions 83.37% <75.07%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...knowledgements/ConfirmedAcknowledgementProvider.cs 100.00% <100.00%> (ø)
.../Session/Subscription/ClassicSubscriptionEngine.cs 87.87% <88.83%> (+0.56%) ⬆️
...Opc.Ua.Client/Subscription/Classic/Subscription.cs 83.25% <53.84%> (-0.62%) ⬇️
src/Opc.Ua.Client/Session/Session.cs 82.29% <52.77%> (-0.05%) ⬇️

... and 40 files with indirect coverage changes

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

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.

Client: hand tracked acknowledgements over when a classic subscription is transferred to another session

3 participants