mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-18 14:43:08 +08:00
Marc Schier
bf9696e245
# Description `Session.OnKeepAlive` recovers a session whose keep alives had stopped by marking **every** outstanding publish request defunct — dropping `GoodPublishRequestCount` to zero — and then calling `StartPublishing` to 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: `TryReservePublishRequest` refused on the first iteration, `StartPublishing` issued 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: | `StartPublishing` | Outcome | | --- | --- | | Unbounded (as originally merged in #4232) | Overshoot — it runs once per subscription create and stacked a pipeline per call: 84 and 165 outstanding against a cap of 50 in CI | | Bounded by the send-side counter (current `master`) | Permanent stall after a write off, as above | ## 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`: - `GoodPublishRequestCount` is 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. - The unrecorded count covers the window before `AsyncRequestStarted` records 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. - The reservation is released when the request is **recorded**, not when it completes, so no engine-side counter can be stranded by a request that never returns. 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 with `BadTooManyPublishRequests` **59 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.BeginPublish` now returns `false` instead 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, and `Subscription.HandleOnKeepAliveStopped` is its only in-tree production caller. ## Testing `StartPublishingRefillsPipelineAfterRequestsAreWrittenOff` reproduces the stall deterministically: it fills the pipeline, writes the requests off the way `Session.OnKeepAlive` does, 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. `BeginPublishIsBoundedByTheDesiredRequestCount` covers 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: | Suite | net10.0 | net48 | | --- | --- | --- | | `Opc.Ua.Client.Tests` | 2127 / 2127 | 2129 / 2131 (2 skipped) | | `Opc.Ua.Subscriptions.Classic.Tests` | 32 / 32 | 32 / 32 | | `ClassicSubscriptionEngineTests` | 25 / 25, five consecutive runs | — | | `Opc.Ua.Client` build, all TFMs | 0 warnings, 0 errors | — | `PublishRequestCountAsync` is 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 (`BadRequestTimeout` in `CreateSubscriptionAsync`), not publish accounting. ## Related Issues No tracked issue exists yet — this is a follow-up regression fix to #4232, found while investigating the `Subscriptions.Classic` CI failures on that PR. Happy to open one if maintainers would rather track it separately. - Follow-up to #4232 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. _(the reservation itself is internal; the one behavioural change — `BeginPublish` returning `true` when the pipeline is already primed — is documented on the method's `<returns>`/`<remarks>` and called out under "The third producer" above)_ - [x] 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. _(ran the affected suites on both TFMs as tabled above, not the full solution)_ - [x] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. _(the `PublishRequestCountAsync` overshoot 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)_ - [x] I have addressed **all** PR feedback received. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f5ab76a-c0ba-4f47-87ac-54d1f4182c6b