Preserve publish queue timestamp ordering - #4084
Conversation
|
|
There was a problem hiding this comment.
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 publish-queue regression where the publish timer could reset timestamps for already-ready subscriptions, breaking oldest-first ordering among equal-priority subscriptions.
Changes:
- Reinstate a guard to avoid touching subscriptions already marked
ReadyToPublish. - Recheck readiness under
m_lockto avoid concurrent timestamp resets while assigning to requests. - Replace the prior requeue/wakeup test with a new regression test focused on timestamp-based ordering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Tests/Opc.Ua.Server.Tests/SessionPublishQueueRaceTests.cs | Renames/reworks the test to validate timestamp ordering behavior for ready subscriptions. |
| Libraries/Opc.Ua.Server/Subscription/SessionPublishQueue.cs | Adds readiness guards (outside and inside lock) to prevent timestamp/order corruption. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master378 #4084 +/- ##
=============================================
- Coverage 60.17% 60.16% -0.01%
=============================================
Files 378 378
Lines 79070 79066 -4
Branches 13837 13837
=============================================
- Hits 47577 47569 -8
+ Misses 27080 27072 -8
- Partials 4413 4425 +12
🚀 New features to boost your workflow:
|
…uple test from traversal order
PRs targeting master378 never triggered the Actions workflow: the pull_request branch filter only listed master/main/develop/*, and for pull_request events GitHub evaluates the workflow file from the merge commit (i.e. the master378 copy), so master's already-updated filter never applied. Branch protection additionally requires a 'Discover test projects' check, a job that only exists in master's workflow, leaving every master378 PR permanently stuck on 'Waiting for status to be reported'. - Add master378 to the push and pull_request branch filters. - Add the discover job so the required check is reported, and derive the test matrix from Tests/Opc.Ua.*.Tests instead of hand-maintaining it. - Trigger on build/config file changes (props, targets, sln, slnx, global.json, NuGet.config) and on the workflow itself. - Restrict the default GITHUB_TOKEN to contents: read. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 892a02b2-efa4-4a3b-bc9a-5e42103f4c26
mrsuciu
left a comment
There was a problem hiding this comment.
@marcschier I think additional check is needed, see comment on code.
… reduce contention
Proposed changes
The master378 publish timer reset timestamps for already-ready subscriptions, breaking oldest-first ordering among equal-priority subscriptions.
Related Issues
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
Retains the single-lock lost-wakeup correction from the preceding change while preserving subscription scheduling timestamps.