Fix publish queue lost wake-up

[marcschier]

Description

Fixes a server-side lost wake-up in SessionPublishQueue that can permanently stop monitored-item notifications while the Session and SecureChannel remain healthy.

In 1.5.378, PublishAsync checked for a ready Subscription under m_subscriptionPublishLock and then queued the Publish request under m_lock. A Subscription could become ready between those operations, be marked ReadyToPublish because no request was queued yet, and then leave the newly queued request stranded. Later timer ticks skipped assignment because the Subscription was already marked ready.

This change backports the relevant SessionPublishQueue correction from #3611 and incorporates the review follow-up:

• Makes the ready-Subscription check and Publish-request enqueue atomic under m_lock.
• Synchronizes PublishCompleted, Requeue, and timer-driven assignment with the same lock.
• Removes m_subscriptionPublishLock.
• Invokes SessionClosed callbacks outside the queue lock.
• Retries timer-driven assignment for already-ready, non-publishing Subscriptions instead of skipping them.
• Adds deterministic regression tests for the lost-wakeup and requeue paths.

Related Issues

• Fixes Monitored item notifications stop after SecureChannel token renewal in Kubernetes after upgrading from OPC UA .NET Standard 1.5.376 to 1.5.378 #3997
• Backports the relevant queue fix from [Server] Improve Request Queue, fix Event handling, add Tests #3611

Testing

• The focused SessionPublishQueueRaceTests fixture passes on net472, net48, net8.0, net9.0, and net10.0.
• Earlier branch validation ran the full UA.slnx on net10.0 and net48 with zero failures.
• Three pre-existing Quickstarts CA1823 warnings remain unchanged.

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes a lost wake-up race in SessionPublishQueue that could leave Publish requests stranded and stall monitored-item notifications.

Changes:

• Makes the “ready subscription” check and Publish-request enqueue atomic under m_lock (removing the split publish lock).
• Synchronizes Publish completion/requeue/timer assignment on m_lock and moves SessionClosed() callbacks outside the queue lock.
• Adds a regression test intended to deterministically reproduce the lost wake-up window.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
Tests/Opc.Ua.Server.Tests/SessionPublishQueueRaceTests.cs	Adds a race/regression test using private-lock coordination to reproduce the lost wake-up.
Libraries/Opc.Ua.Server/Subscription/SessionPublishQueue.cs	Consolidates locking on m_lock, removes m_subscriptionPublishLock, and adjusts close/completion paths accordingly.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.14%. Comparing base (52294d2) to head (50bf92e).

Additional details and impacted files

@@              Coverage Diff              @@
##           master378    #4017      +/-   ##
=============================================
- Coverage      60.15%   60.14%   -0.02%     
=============================================
  Files            378      378              
  Lines          79069    79071       +2     
  Branches       13836    13838       +2     
=============================================
- Hits           47567    47555      -12     
- Misses         27084    27093       +9     
- Partials        4418     4423       +5     

Files with missing lines	Coverage Δ
.../Opc.Ua.Server/Subscription/SessionPublishQueue.cs	78.48% <100.00%> (+4.99%)	⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[romanett]

@marcschier I see the Issue here not so much in the locking stategy, but more in that area.

UA-.NETStandard/Libraries/Opc.Ua.Server/Subscription/SessionPublishQueue.cs

Lines 460 to 500 in 52294d2

	private void AssignSubscriptionToRequest(QueuedSubscription subscription)
	{
	lock (m_lock)
	{
	// find a request.
	while (m_queuedRequests.Count > 0)
	{
	QueuedPublishRequest request = m_queuedRequests.First.Value;
	m_queuedRequests.RemoveFirst();
	if (request.Tcs.Task.IsCompleted)
	{
	request.Dispose();
	continue;
	}
	// check secure channel.
	if (!m_session.IsSecureChannelValid(request.SecureChannelId))
	{
	m_logger.LogWarning("Publish abandoned because the secure channel changed.");
	request.Tcs.TrySetException(new ServiceResultException(StatusCodes.BadSecureChannelIdInvalid));
	request.Dispose();
	continue;
	}
	m_logger.LogTrace(
	"PUBLISH: #{Id} Assigned To Subscription({SubscriptionId}).",
	request.SecureChannelId,
	subscription.Subscription.Id);
	subscription.Publishing = true;
	request.Tcs.TrySetResult(subscription.Subscription);
	request.Dispose();
	return;
	}
	// mark it as available.
	subscription.ReadyToPublish = true;
	subscription.Timestamp = DateTime.UtcNow;
	}
	}

The subscription is assigned ReadyToPublish = True unconditionally, even if no request is available.

The Publish cycle skip subscriptions that have ReadyToPublish in the timed PublishTimerExpired.

UA-.NETStandard/Libraries/Opc.Ua.Server/Subscription/SessionPublishQueue.cs

Lines 410 to 450 in 52294d2

	public void PublishTimerExpired()
	{
	var subscriptionsToDelete = new List<ISubscription>();
	// check each available subscription.
	foreach (KeyValuePair<uint, QueuedSubscription> entry in m_queuedSubscriptions)
	{
	QueuedSubscription subscription = entry.Value;
	PublishingState state = subscription.Subscription.PublishTimerExpired();
	// check for expired subscription.
	if (state == PublishingState.Expired)
	{
	m_queuedSubscriptions.TryRemove(subscription.Subscription.Id, out _);
	subscriptionsToDelete.Add(subscription.Subscription);
	((SubscriptionManager)m_server.SubscriptionManager).SubscriptionExpired(
	subscription.Subscription);
	continue;
	}
	// check if idle.
	if (state == PublishingState.Idle)
	{
	subscription.ReadyToPublish = false;
	continue;
	}
	// do nothing if subscription has already been flagged as available.
	if (subscription.ReadyToPublish)
	{
	continue;
	}
	// assign subscription to request if one is available.
	if (!subscription.Publishing)
	{
	lock (m_subscriptionPublishLock)
	{
	AssignSubscriptionToRequest(subscription);
	}
	}

I think to remove that skipping of Subscriptions with Status ReadyToPublish would resolve the Issue without any change to the locking behaviour.

Also the described behaviour would be only causing a single publish Request to time out (which is not good, but way less severe than a subscription that is not publishing at all).

All later publish requests would be able to publish that subscription again.

[marcschier]

This needs to be re-added