Preserve publish queue timestamp ordering

[marcschier]

Description

Fixes publish request assignment ordering in SessionPublishQueue, following up on the review of the proposed #3997 follow-up.

Two related problems were identified during review:

1. Retrying an already-ready Subscription from PublishTimerExpired() reset its Timestamp when no Publish request was available, which broke oldest-first selection among equal-priority Subscriptions. Master already uses a single lock for the ready check and the Publish-request enqueue, so the original lost-wakeup window does not require removing the ReadyToPublish guard.
2. PublishTimerExpired() bypassed the selection policy used by PublishAsync(). It iterated m_queuedSubscriptions — a ConcurrentDictionary with no ordering guarantee — and handed each newly notifying Subscription straight to the first waiting request, so Priority and Timestamp were ignored whenever several Subscriptions became ready in the same timer tick.

This PR now:

• Keeps the ReadyToPublish timer early exit so already-ready Subscriptions retain their timestamps.
• Flags all notifying Subscriptions as available first and then drains the waiting requests through GetSubscriptionToPublish(), so requests are served highest priority and longest waiting first regardless of dictionary iteration order.
• Routes PublishCompleted(..., moreNotifications: true) through the same path instead of assigning the Subscription directly.
• Replaces AssignSubscriptionToRequest() with AssignSubscriptionsToRequests() / TryAssignSubscriptionToRequest(). The latter also skips a request whose task completed (cancelled or timed out) between the IsCompleted check and TrySetResult, instead of losing the Subscription.
• Adds deterministic regression coverage for both behaviours.

Related Issues

• Follow-up to 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
• Review correction to Fix publish queue lost wake-up #4017
• Tracks the equivalent master378 correction in [master378] Restore ReadyToPublish guard in SessionPublishQueue.PublishTimerExpired to preserve Subscription timestamp ordering #4119

Testing

• All 26 SessionPublishQueueTests pass on net10.0 and net48.
• PublishTimerAssignsWaitingRequestToHighestPrioritySubscriptionAsync and PublishTimerPreservesReadySubscriptionTimestampOrderAsync both fail without the corresponding source change.
• The full Opc.Ua.Server.Tests project shows no new failures on net10.0; the single ServerFluentApiHostingTests.ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync failure reproduces unchanged on the branch without these edits.

[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.

Adds a deterministic regression test to prevent a previously fixed SessionPublishQueue “lost wake-up” race from reappearing, without changing production code.

Changes:

• Adds a new concurrency/regression test that forces the critical publish/subscription interleaving.
• Introduces private-lock access helpers (via reflection) to orchestrate specific execution ordering.

[codecov]

Codecov Report

❌ Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.92%. Comparing base (636912b) to head (f24c06d).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
.../Opc.Ua.Server/Subscription/SessionPublishQueue.cs	92.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4016       +/-   ##
===========================================
+ Coverage   36.98%   79.92%   +42.94%     
===========================================
  Files         898     1515      +617     
  Lines      138122   209992    +71870     
  Branches    25039    36220    +11181     
===========================================
+ Hits        51079   167833   +116754     
+ Misses      80638    29561    -51077     
- Partials     6405    12598     +6193     

Files with missing lines	Coverage Δ
.../Opc.Ua.Server/Subscription/SessionPublishQueue.cs	85.66% <92.50%> (+39.40%)	⬆️

... and 1337 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[Copilot]

@romanett I've opened a new pull request, #4083, to work on those changes. Once the pull request is ready, I'll request review from you.

[mrsuciu]

There is still an inconsistency since PublishTimerExpired() bypasses the subscription-selection policy used by PublishAsync(). PublishTimerExpired enumerates through m_queuedSubscriptions (which is a ConcurrentDictionary and doesn't offer order guarrantee) and calls "assign subscription to request if one is available" AssignSubscriptionToRequest(subscription); which servers it to the first waiting publish request. No Priority taken into account in this case. Am I missing something

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

tests/Opc.Ua.Server.Tests/SessionPublishQueueTests.cs:182

• This test asserts a specific timerOrder across two PublishTimerExpired() calls, but the subscription enumeration order is not guaranteed (and can vary between enumerations), which can make the test flaky. Consider asserting set membership (both subscriptions were visited) and keep the determinism focused on the publish-selection outcome (i.e., that PublishAsync() returns olderSubscription), rather than assuming a stable callback order.

            Assert.That(timerOrder, Has.Count.EqualTo(2));
            Assert.That(timerOrder[0], Is.SameAs(newerSubscription));
            Assert.That(timerOrder[1], Is.SameAs(olderSubscription));

src/Opc.Ua.Server/Subscription/SessionPublishQueue.cs:524

• AssignSubscriptionsToRequests() no longer takes m_lock internally (the old AssignSubscriptionToRequest() did). This makes thread-safety depend on all call sites holding m_lock, which is easy to violate during future edits. To avoid accidental race conditions, consider reintroducing the internal lock (m_lock) in AssignSubscriptionsToRequests() (and keep TryAssignSubscriptionToRequest() private to the locked context), or add an explicit invariant (e.g., a debug assertion / naming convention like AssignSubscriptionsToRequestsLocked) to enforce correct usage.

        private void AssignSubscriptionsToRequests()
        {
            while (m_queuedRequests.Count > 0)
            {
                QueuedSubscription? subscriptionToPublish = GetSubscriptionToPublish();

                if (subscriptionToPublish == null)
                {
                    break;
                }

                if (!TryAssignSubscriptionToRequest(subscriptionToPublish))
                {
                    // no usable request left, keep the subscription available.
                    subscriptionToPublish.Publishing = false;
                    break;
                }
            }
        }

src/Opc.Ua.Server/Subscription/SessionPublishQueue.cs:490

• Within a single timer tick, assigning Timestamp = DateTime.UtcNow inside the loop can introduce per-subscription timestamp differences that depend on notifyingSubscriptions order (which ultimately stems from an unordered dictionary enumeration). If the intent is that all notifications discovered in the same timer tick are equivalently 'ready now', consider capturing DateTime.UtcNow once before the loop and applying the same value to all newly-ready subscriptions to reduce ordering artifacts.

                    foreach (QueuedSubscription subscription in notifyingSubscriptions)
                    {
                        if (subscription.Publishing || subscription.ReadyToPublish)
                        {
                            continue;
                        }

                        subscription.ReadyToPublish = true;
                        subscription.Timestamp = DateTime.UtcNow;
                    }