Fix the classic engine's publish pipeline: refill after keep-alive recovery, and bound every producer - #4274
Conversation
The classic engine reserved a slot before sending a publish request and released it when the request completed. That counter was a second, independent accounting of the outstanding requests and it only ever shrank on completion, so it could not observe a request the session had written off. Session.OnKeepAlive writes off exactly that way: when keep alives recover it marks every outstanding publish request defunct - dropping GoodPublishRequestCount to zero - and calls StartPublishing to refill the pipeline. The written off requests never complete, so their reservations were held forever, StartPublishing could reserve nothing, and the session stopped publishing for good. Bounding StartPublishing on that counter is what removed the last escape hatch; leaving it unbounded instead is what let the pipeline overshoot on every subscription create. Reserve against the session's count instead. The engine now tracks only the requests it has sent that the session has not recorded yet, and the limit is checked against that plus GoodPublishRequestCount. The session's count is authoritative and already honours the write off, so a written off pipeline refills immediately; the unrecorded count covers the window before AsyncRequestStarted records a request, which is the lag that made the original check-then-act overshoot. The reservation is released once the request is recorded rather than when it completes, so no engine side counter can be left stranded by a request that never returns. StartPublishingRefillsPipelineAfterRequestsAreWrittenOff reproduces the stall: it fills the pipeline, writes the requests off the way the session does, and asserts the refill. It issued 0 of 5 before this change. ConcurrentPublishReEvaluationDoesNotExceedDesiredRequestCount still holds, so both bounds hold at once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5ab76a-c0ba-4f47-87ac-54d1f4182c6b
Code coverage✅ Coverage gate passed.
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4274 +/- ##
==========================================
- Coverage 80.71% 80.03% -0.69%
==========================================
Files 1831 1926 +95
Lines 253449 263455 +10006
Branches 44116 46103 +1987
==========================================
+ Hits 204583 210860 +6277
- Misses 33510 36392 +2882
- Partials 15356 16203 +847
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a publish pipeline stall in the classic subscription engine after keep-alive recovery by reconciling the engine’s publish “reservation” with the session’s authoritative outstanding publish accounting, and adds a regression test to cover the recovery path.
Changes:
- Replace the engine-side “in flight” counter with an “unrecorded publish requests” reservation that is checked against
GoodPublishRequestCount + unrecorded. - Release the reservation when the session records the request (via
AsyncRequestStarted) instead of waiting for request completion. - Add a deterministic test reproducing keep-alive recovery writing off outstanding publish requests and verifying
StartPublishingrefills the pipeline.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Opc.Ua.Client/Session/Subscription/ClassicSubscriptionEngine.cs |
Reworks publish request reservation logic to avoid overshoot while still allowing refill after session write-off. |
tests/Opc.Ua.Client.Tests/Session/ClassicSubscriptionEngineTests.cs |
Adds a regression test validating pipeline refill after outstanding publishes are written off during keep-alive recovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both engine tests handed the mocked PublishAsync a TaskCompletionSource created without RunContinuationsAsynchronously. The engine hangs its continuation on that task with OnCompleted, so completing it on the test thread ran OnPublishComplete inline, which re-enters the engine and can issue further publishes while the assertion is being evaluated - a deep synchronous call chain and a timing dependent result. Create both sources with RunContinuationsAsynchronously, and sample the issued count before completing them so each assertion is fixed by the call under test alone rather than by whatever the completions go on to do. The same pattern was already present in the pre-existing concurrency test, so it is corrected there too: the assertion is a bound on the number of requests issued, which an inline completion could inflate. Addresses review feedback on #4274. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5ab76a-c0ba-4f47-87ac-54d1f4182c6b
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
CI note:
|
|
@marcschier should be fixed if you merge: 333de83 |
PublishRequestCountAsync kept failing on the Windows net10.0 leg with around twice the desired publish requests outstanding (90 and 100 against a cap of 50). Neither producer this PR already bounds could account for it, so the remaining one was instrumented rather than guessed at: a local run logged 23 calls into Subscription.HandleOnKeepAliveStopped, which sends an uncapped publish whenever a subscription has seen no notification. With one subscription per nudge that adds up to a second full pipeline, which matches the observed numbers, and the server agrees the surplus is real - the failing CI run answered it with BadTooManyPublishRequests 59 times. Route that nudge through the same reservation as the automatic top up. Sending past the desired count cannot help, because the server already holds that many requests and rejects the rest, while every subscription nudging at once multiplies the pipeline by the subscription count. The limit is at least one, so an empty pipeline is always refillable, and because the reservation reconciles against the session's own count a pipeline whose requests were written off still refills - which is the case the nudge exists for. Session.BeginPublish already returns false when nothing was sent, so the contract is unchanged. BeginPublishIsBoundedByTheDesiredRequestCount covers both halves: twelve nudges across three subscriptions issue three requests, and the nudge gets through again once the outstanding requests are written off. Verified with the integration suite green on net10.0 and net48, and under deliberate CPU contention no run reproduced the overshoot any more; the failures that remain under that load are setup and teardown timeouts on a starved machine, not publish accounting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5ab76a-c0ba-4f47-87ac-54d1f4182c6b
marcschier
changed the title
|
@marcschier please use the solution already present in master |
…lishing-reservation-review # Conflicts: # src/Opc.Ua.Client/Session/Subscription/ClassicSubscriptionEngine.cs
Merged
|
Re: "please use the solution already present in master"Master is merged in as of 7861ec0, and its overshoot fix is preserved — On adopting master's implementation wholesale: I checked it empirically rather than argue from reading, and it does not hold up. I checked out 333de83, dropped this PR's two regression tests onto it unchanged, and ran them against master's engine: The other 23 pass on master untouched, so this is not the tests disagreeing with master's style — they isolate one behaviour. Why it fails. Master's bound is checked against That is the bug this PR exists to fix, and it is why the counter is reconciled against What is actually in the branch now is not a competing solution but a superset:
The one thing not carried over is master's If you would still prefer master's version to stand on its own, the stall is worth tracking separately — I would rather it be a known open issue than silently shipped. But merging this closes both, and it keeps your fix intact. |
Description
Session.OnKeepAliverecovers a session whose keep alives had stopped by marking every outstanding publish request defunct — droppingGoodPublishRequestCountto zero — and then callingStartPublishingto refill the pipeline. Those written off requests never return.The classic engine's publish reservation, added in #4232, could not observe that. It counted the requests the engine had sent and released a slot only when a request completed, so the write off left the reservations held forever:
TryReservePublishRequestrefused on the first iteration,StartPublishingissued nothing, and the session never published again. Recovery from a keep-alive outage is exactly the path that stops working.That counter was a second, independent accounting of the same thing the session already tracks, and the two could not be reconciled. Both prior behaviours were wrong in one direction:
StartPublishingmaster)What changed
The reservation is now reconciled with the session's own accounting instead of duplicating it.
The engine tracks only the requests it has sent that the session has not recorded yet, and the limit is checked against
GoodPublishRequestCount + unrecorded:GoodPublishRequestCountis authoritative and already honours the write off, so a written off pipeline refills immediately — the recovery valve behaviour is preserved as a property of the accounting rather than as an unbounded escape hatch.AsyncRequestStartedrecords a request, which is the lag that made the original check-then-act overshoot: concurrent callers all read the same stale value and each sent.Reads take the unrecorded count before the session count, so a request moving between the two is counted twice rather than missed — the error is always towards sending less.
The third producer: the keep-alive nudge
Bounding the two automatic producers was not enough. CI kept failing with roughly twice the desired requests outstanding (90 and 100 against a cap of 50), which neither bounded producer could explain, so the remaining one was instrumented rather than guessed at: a local run logged 23 calls into
Subscription.HandleOnKeepAliveStopped, which sends an uncapped publish whenever a subscription has seen no notification. One nudge per subscription adds a second full pipeline, which matches the observed numbers — and the server agrees the surplus is real: the failing CI run answered it withBadTooManyPublishRequests59 times.That nudge now goes through the same reservation. Sending past the desired count cannot help, because the server already holds that many requests and rejects the rest. The limit is at least one, so an empty pipeline is always refillable, and because the reservation reconciles against the session's count, a pipeline whose requests were written off still refills — the case the nudge exists for.
Behaviour change worth reviewing:
ISession.BeginPublishnow returnsfalseinstead of sending when the pipeline is already at the desired count. The signature and the documented contract ("true if the request was sent") are unchanged, andSubscription.HandleOnKeepAliveStoppedis its only in-tree production caller.Testing
StartPublishingRefillsPipelineAfterRequestsAreWrittenOffreproduces the stall deterministically: it fills the pipeline, writes the requests off the waySession.OnKeepAlivedoes, and asserts the refill. It issued 0 of 5 before this change.ConcurrentPublishReEvaluationDoesNotExceedDesiredRequestCount(the overshoot guard from #4232) still passes, so both bounds hold at once.BeginPublishIsBoundedByTheDesiredRequestCountcovers the nudge from both sides: twelve nudges across three subscriptions issue three requests, and the nudge gets through again once the outstanding requests are written off.Run locally on this branch:
Opc.Ua.Client.TestsOpc.Ua.Subscriptions.Classic.TestsClassicSubscriptionEngineTestsOpc.Ua.Clientbuild, all TFMsPublishRequestCountAsyncis the integration test that caught the overshoot in CI. Under deliberate CPU contention (6 burners on 8 cores) it was run repeatedly and no run reproduced the overshoot; the failures that remain under that load are setup and teardown timeouts on a starved machine (BadRequestTimeoutinCreateSubscriptionAsync), not publish accounting.Related Issues
No tracked issue exists yet — this is a follow-up regression fix to #4232, found while investigating the
Subscriptions.ClassicCI failures on that PR. Happy to open one if maintainers would rather track it separately.Checklist
BeginPublishreturningtruewhen the pipeline is already primed — is documented on the method's<returns>/<remarks>and called out under "The third producer" above)PublishRequestCountAsyncovershoot is fixed at the source; the residual transport-timeout flake under heavy agent load is pre-existing and reproduces identically on the pre-merge parent — see the merge note below)