Split the CI matrix across both systems, split the required test gate from the advisory coverage check, and give background work an owner - #4158
Open
marcschier wants to merge 23 commits into
Open
Conversation
Flips both halves of the CI backend switch from 'ado' to 'actions', so the all-TFM solution builds, the ubuntu test matrix and Native AoT run on GitHub Actions again while Azure Pipelines runs the fast pull-request test legs on the netstandard Managed DevOps Pool. Both switches must always be flipped together. The coverage gate is deliberately unaffected: it lives in the always-on 'Fast PR test' stage, not in a stage gated on the switch, because it is the required status check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Contributor
There was a problem hiding this comment.
🟢 Ready to approve
The changes are limited to consistent switch flips plus documentation/comment updates, with no product/build logic modifications beyond the intended backend default change.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Switches the CI “backend ownership” toggle back to actions (split workload between GitHub Actions and the Azure DevOps managed pool) and updates the corresponding documentation/comments so both halves of the switch remain aligned.
Changes:
- Flip the default CI backend in
azure-pipelines.yml(parameters.ciBuildBackend) fromadotoactions. - Flip the matching GitHub Actions switch in
.github/workflows/buildandtest.yml(env.CI_BUILD_BACKEND) fromadotoactions. - Update
docs/DeveloperGuide.mdto reflect the new default split and explicitly call out that the Azure Pipelines coverage gate remains always-on.
File summaries
| File | Description |
|---|---|
| docs/DeveloperGuide.md | Updates CI system responsibility summary, switch defaults, and clarifies the coverage gate is exempt from the backend toggle. |
| azure-pipelines.yml | Sets ciBuildBackend default to actions and updates explanatory comments/display text accordingly. |
| .github/workflows/buildandtest.yml | Sets CI_BUILD_BACKEND default to actions and updates the switch description comments to match. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
cristipogacean
approved these changes
Aug 3, 2026
The coverage gate job was appended to every test stage, so 'Fast PR test' emitted three sibling checks whose display names collided - 'Coverage Release (net10.0)' appeared twice, once for the Windows leg and once for macOS, with nothing to tell them apart. That job also fused two unrelated concerns: whether the tests passed (which must block a merge) and whether coverage met its thresholds (which should not). Both concerns now get a stage of their own, mirrored in both CI systems: Tests passed / build-and-test summary - required in the branch ruleset Code coverage / code coverage - advisory, red on a miss Both rollups run on not(canceled()) / always() rather than on success: a check that reports 'skipped' is treated by GitHub as satisfied, so a required check that skips when its dependency fails would wave a red build straight through. The gate reads stage-level dependencies.<stage>.result rather than enumerating matrix-generated job names, which change whenever a test project or agent is added. Coverage is now merged once across every leg of the run instead of once per leg, which also removes the enforceCoverage / publishCoverage "exactly one invocation may set this" parameters. GitHub Actions already collected Cobertura in every matrix job and then threw it away. It now merges those fragments and runs the same check-coverage.ps1 against the same coverage-thresholds.json. Since CI_BUILD_BACKEND defaults to 'actions', that ubuntu matrix - which runs every test project - is the fullest coverage signal in the repository. check-coverage.ps1 gains -SummaryPath, rendering a markdown report that Azure attaches with task.uploadsummary and that Actions appends to the job summary and upserts as a single sticky pull request comment. Threshold misses also surface as ::error:: annotations. The comment is fork-safe: a pull_request from a fork gets a read-only token, so it degrades to the job summary alone. Note that the two systems report different numbers by design - Actions merges every project on ubuntu, Azure only the Windows fast-PR legs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Review follow-ups on the gate/coverage split: * dependencies.<stage>.result resolves at STAGE scope only - at job scope 'dependencies' refers to other jobs of the same stage, so the lookups would have silently returned empty and the gate would have passed everything. gate.yml and coverage.yml are now stage templates that own their own dependsOn and derive it from the same stage list they evaluate, so the two lists cannot drift apart. An unresolved result is now a hard error rather than an accepted value. * get-matrix.ps1 emitted an empty matrix on a discovery miss. The downstream job is then skipped, a skipped job rolls up as a SUCCEEDED stage, and the gate would have approved a run that executed no tests at all. It now fails, with an -AllowEmpty opt-out. * Zero coverage reports produced a green advisory check, which would have hidden a broken collector indefinitely. Both systems now distinguish "the tests never ran" (warn) from "the tests ran and published nothing" (error). * The GitHub coverage job keyed off gh_owns_build, so a push or scheduled run under the 'ado' backend executed the macOS legs, uploaded their coverage and then discarded it. It now uses the same test_os condition build-and-test itself uses. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
marcschier
changed the title
Code coverage✅ Coverage gate passed.
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
Build 16513 reported "There was a YAML error preventing Azure Pipelines from
determining if the pipeline should run", so no Azure checks were posted to the
pull request at all - the commit carried only github-actions and license/cla.
The cause was a YAML anchor. Azure Pipelines supports no anchors, aliases or
merge keys, and the terminal stages shared their stage list with
'stages: &testStages' / 'stages: *testStages'. The list is now a root
parameter, which is the idiomatic way to write it once.
While removing the anchor, both templates also stop building one variable and
one env entry per stage with ${{ each }} and read the whole dependency map in
one go with $[ convertToJson(dependencies) ] instead. That is not just shorter:
the stages the gate verifies are now, by construction, exactly the stages it
depends on, so the two can no longer disagree and the "unresolved result"
failure mode disappears.
Verified the rewritten evaluation over 8 scenarios (Succeeded /
SucceededWithIssues / Skipped / absent result / Failed / Canceled / empty
dependency map / empty input) - 8/8 correct.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Azure Pipelines reports checks to GitHub as '<pipeline> (<stage> <job>)', so identical stage and job display names rendered as 'Tests passed Tests passed'. The jobs are now named for what they do, giving 'Tests passed Verify stage results' and 'Code coverage Merge and evaluate'. Settled before the ruleset is configured, because renaming later invalidates the required-check entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Build 16520 failed the required 'Tests passed' stage even though every test
stage had succeeded or been intentionally skipped. The job preparation log
shows why:
stageResults:
Evaluating: convertToJson(dependencies)
Result: '{}'
A stage-scoped '$[ ]' variable is still evaluated when the JOB is prepared,
and in a job's context 'dependencies' means sibling jobs of the same stage.
The gate stage has a single job with no job dependencies, so the expression
resolved to '{}' and the gate had nothing to check.
The context is now read as 'stageDependencies' declared at job scope, which
is the documented way for a job to reach results in another stage. It is
shaped { "<stage>": { "<job>": { "result": ... } } }, so the evaluation walks
stage -> job and is actually stricter than before: it sees individual job
results rather than only the stage rollup.
Note the failure mode this had: the gate could not distinguish "nothing to
check" from "everything passed". An empty context is now an explicit error in
both templates, because a required check that silently stops verifying is
worse than one that fails.
Verified the rewritten evaluation over 7 shapes (all green with skipped
stages, a failed matrix job, a skipped job, a canceled job, an outputs
payload, an empty map and an empty string) - 7/7 correct.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Build 16583 abandoned both terminal stages with:
The maximum allowed memory size was exceeded while evaluating the
following expression: convertToJson(stageDependencies)
This pipeline fans out to ~115 jobs whose output variables include the
serialised test matrix, so the stage dependency context is far too large to
serialise in an expression. The previous attempt had already established
that the other direction does not work either: a stage-scoped '$[ ]'
variable is evaluated when the JOB is prepared, where 'dependencies' means
sibling jobs, which is why convertToJson(dependencies) returned '{}' in
build 16520.
The verdict therefore moves to where Azure Pipelines does reliably expose
stage results - the stage condition - as a plain
in(dependencies.<stage>.result, 'Succeeded', 'SucceededWithIssues',
'Skipped') per stage. That costs nothing to evaluate and is the documented
usage. Reaching the job now IS the verdict, so the job body is a single
echo, and gate.yml is folded into azure-pipelines.yml so the stage list and
the condition that consumes it cannot drift apart.
This makes the required check fail-closed rather than fail-red: a failing
stage makes the condition false, the stage is skipped, and Azure Pipelines
posts NO check for a skipped stage (verified on build 16583, where none of
the seven skipped Schedule/Manual stages reported). The required check then
stays unfulfilled and blocks the merge, which is the safe direction - the
failing job is already red on its own, and a check that never reports can
never be mistaken for a passing one.
The coverage stage loses its "ran but published nothing" detection for the
same memory reason; that check remains on the GitHub Actions side, where the
test matrix result is available cheaply.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Run 30988867402 left test-ubuntu-latest-Core and test-ubuntu-latest-Client
running for ~6 hours. Those entries normally finish in 3-15 minutes, so they
were hung, but the job carried no timeout-minutes and therefore sat until the
360-minute default - blocking the pull-request gate with no diagnostic
information and no way to tell a hang from a slow run.
The Azure Pipelines test template already solved exactly this, and now that
CI_BUILD_BACKEND defaults to 'actions' this matrix is the primary one, so the
gap matters. The same two mechanisms are applied here:
* --blame-hang-timeout 10m --blame-hang-dump-type mini, so a single stuck
test surfaces as "Test <Name> exceeded the configured timeout" with a mini
dump containing every thread's stack. The LongRunning and Stress tiers do
not run here, so 10 minutes is far above any legitimate test. The dump
lands in the results directory, which the existing always() upload step
already publishes as an artifact.
* timeout-minutes: 60 on the job as a backstop for a hang outside the test
host, where the blame collector cannot help. The sibling all-TFM build
jobs already carry timeout-minutes: 120.
This changes no test, assertion or threshold; it only makes an existing
failure mode visible in minutes instead of hours.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
test-ubuntu-latest-Core and test-ubuntu-latest-Client hung until
--blame-hang-timeout killed them, taking every other test in those
assemblies with them. The hang dump names the culprit exactly:
ClientChannelManagerManagedTests.ReconnectAsyncSwapsFaultedLeaseEntryAsync
NUnit.Framework.Assert.ThrowsAsync(...)
NUnit.Framework.Internal.AsyncToSyncAdapter.Await(...)
System.Threading.Tasks.Task.InternalWait(Int32, CancellationToken)
...
NUnit.Framework.Api.NUnitTestAssemblyRunner.WaitForCompletion(Int32)
Assert.ThrowsAsync blocks the calling thread on the task with no timeout.
These tests drive a fake clock, so when the reconnect schedules a timer
before failing, nothing can advance that clock while the test thread is
blocked inside the assertion - the test never returns, NUnit sits in
WaitForCompletion, and the host hangs. That is why exactly these two
assemblies hung and no others: the fixture is duplicated in both, and
every other wait in it is already bounded with WaitAsync.
The reconnect is now started first and awaited through
WaitAsync(5s) inside the assertion, so the wait is bounded. A reconnect
that genuinely fails to complete now surfaces as a TimeoutException in
seconds instead of a ten-minute host hang. The same unbounded pattern in
ReconnectAsyncWithBudgetStopsWhenExhaustedAsync is fixed the same way in
both copies.
Also hardens the raw worker thread in NodeStateHandlerConcurrencyTests,
which is the other way this assembly could wedge the host: it was a
FOREGROUND thread whose stop flag was read without a barrier and set only
on the success path, so a failed assertion in the loop leaked a spinning
thread that pins the process at exit, and an exception on the thread would
terminate the host outright. It is now background, uses Volatile
read/write, stops in a finally, joins with a bound, and surfaces worker
exceptions on the test thread.
No assertion is weakened and no test is skipped; both changes convert a
silent hang into a fast, diagnosable failure.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
With the deadlock gone, the Core and Client assemblies ran to completion on ubuntu and surfaced the defect the hang had been masking: two tests failed with TimeoutException because the reconnect task never completed. The reconnect loop computes a back-off and registers a fake-clock timer before it consults the retry budget, so it can be parked on a timer even when the budget is already spent. These tests advanced the fake clock once, or not at all, so a loop that needed a second round stayed parked forever and nothing else could advance the clock. They now advance in a bounded loop until the reconnect completes, which is the pattern the sibling Opc.Ua.Client.Tests copy of this fixture already used for its swap case. No assertion is weakened: the expected ServiceResultException, its status code, the resulting channel state and the Times.Never verification all still run. Note ReconnectAsyncWithBudgetShrinksDelayToFitRemainingAsync was not touched by the previous commit, which is what confirms these are pre-existing ubuntu failures that the host hang had been hiding rather than fallout from the deadlock fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
This reverts commit 24d452d.
The reconnect tests synchronise with the code under test by waiting for it to register a back-off timer on the fake clock, then advancing that clock. ObservableFakeTimeProvider numbered timers absolutely, so WaitForTimerCreatedAsync(1) meant "the first timer of the whole run", not "the first timer of this reconnect". That is a race. ReconnectAsyncSwapsFaultedLeaseEntryAsync performs an earlier reconnect with an exhausted budget, and that reconnect can register a timer of its own before failing. When it does, it consumes slots 1 and 2, both waiters complete before the timers the test cares about exist, and the Advance calls fire nothing. The real back-off timer is registered afterwards on a clock nobody will move again, so the reconnect never completes. Whether the earlier reconnect gets that far is scheduling dependent, which is why this passed on Windows and intermittently wedged ubuntu. Waiters are now armed for "N more timers from now" and taken before the operation whose timers are awaited, so an unrelated timer can no longer satisfy them. Assertions are unchanged - this fixes only the synchronisation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
The two ubuntu failures that remained after the timer-waiter fix were both Assert.ThrowsAsync timing out at 5 seconds on a reconnect that completes instantly on Windows. Assert.ThrowsAsync is sync over async: it blocks the calling thread until the task completes. On a busy Linux agent that blocks a pool thread while the continuation the assertion is waiting for needs a pool thread, and the test thread is also the only thing that can advance the fake clock these tests drive. Both are self-inflicted stalls, and the repository's own async rules forbid sync over async for exactly this reason. The reconnect assertions now await the task through a small AssertThrowsAsync helper that stays asynchronous end to end and bounds the wait, so the continuation is free to run and the clock is free to move. The two SendRequest assertions in the Client copy are converted the same way. Nothing is weakened: the helper still fails the test when the operation succeeds, and reports the actual exception when it is the wrong type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
The two budget-aware reconnect tests failed on the ubuntu legs with
"Expected ServiceResultException but got TimeoutException", while passing
on Windows, on Azure DevOps and in a local Linux container.
The captured test output shows the reconnect had already finished. The
channel reached Faulted and the manager logged the final outcome
("Reconnect failed ... Outcome=policy-exhausted"), which is emitted from
RecordReconnectAttempt - the statement immediately before the reconnect
coalescer's TrySetResult. So the operation completed within milliseconds;
what did not happen inside the five second budget was the *observation*
of that completion.
The coalescer completes with RunContinuationsAsynchronously, so the
caller's continuation chain (coalescer -> AwaitReconnectResultAsync ->
ReconnectLeaseAsync -> ReconnectAsync) is queued to the thread pool. The
assembly runs its fixtures in parallel and a number of them block pool
threads, so on a four-core hosted agent the pool is saturated and injects
replacement threads at roughly one per second. Five seconds is then spent
queued rather than running.
These waits are hang detectors, not latency assertions - the tests drive
a fake clock, so the work under test takes microseconds. Give them a
single named budget that is generous enough never to fire on a healthy
run and still far below the blame-hang timeout, so a genuine deadlock
continues to fail the job quickly.
No assertion is weakened: the expected exception type, status code and
resulting channel state are all still asserted exactly as before.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Keeps the merged reconnect test waiting on the shared completion budget this branch introduced, and stops the reconnect race retry from ignoring the caller's budget. master added IsTerminalReconnectRace so that a reconnect losing a race against a concurrent close recovers on a freshly swapped entry. The predicate only asks whether the entry ended up Closed or Faulted with BadSecureChannelClosed, which is also exactly how a reconnect ends when the caller's retry budget or the reconnect policy is exhausted. Those are deliberate terminal outcomes, not races, so the recovery path swapped the entry and started a *second*, unbudgeted reconnect cycle - and waited out the swap back-off first. ReconnectAsync(channel, budget) therefore never returned within its budget, and under a fake clock it never returned at all. Require the budget to have room left before treating the failure as a race. The parameterless ReconnectAsync overload passes an unlimited budget, so genuine race recovery is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Code review found the earlier guard closed only half the hole. It asked whether the caller's retry budget still had room, but a reconnect cycle has two deliberate terminal stops and the budget only describes one of them: ExponentialBackoffChannelReconnectPolicy.GetDelay returns the infinite sentinel as soon as the attempt count reaches MaxAttempts, and it does so before the budget is ever consulted. A policy-exhausted stop with a healthy budget was therefore still mistaken for a lost race against a concurrent close, so the manager swapped the entry and ran a second, unbudgeted cycle behind the swap back-off - exactly the behaviour the guard was added to prevent. Both stops leave the entry Faulted with BadSecureChannelClosed, so state and status code cannot tell them apart. Ask the entry instead: it records whether its last cycle ended in StopWithFaultAsync, which is reached only when the policy or the budget said stop. A genuine race - the entry closed underneath us - never sets it. That also removes the budget parameter again, so the predicate goes back to reading as one idea rather than two. Adds a regression test for the case the review identified: MaxAttempts = 0 with a minute of budget. Verified as a real negative control - with the predicate reverted to master's, the new test fails in 68 ms and the two existing ones time out; with the fix all 27 pass. Also corrects docs/DeveloperGuide.md, which claimed both rollup checks run on not(canceled())/always(). Only the GitHub Actions summary does. The Azure gate encodes its verdict in the stage condition and is fail-closed because a skipped Azure *stage* posts no check at all - confirmed on build 16613, where Fast PR test failed, Tests passed was skipped, and no Tests passed check-run reached the pull request. The old wording contradicted the blockquote four lines above it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Three fan-out sites started work that nothing owned. Two of them carried a
standing "TODO: Await the task completion in shutdown and pass cancellation
token".
SessionManager.MonitorSessionsAsync and
SubscriptionManager.PublishSubscriptionsAsync were both launched with a
discarded Task.Factory.StartNew. Both return ValueTask, so StartNew hands
back a Task<ValueTask> that completes as soon as the loop first yields -
storing it would not have helped, because awaiting it would only have
awaited the *scheduling* of the loop. Both now go through a small factory
that calls AsTask().Unwrap(), the same shape the neighbouring
StartConditionRefreshWorker already uses, so the stored task really does
represent the loop.
Shutdown now cancels and awaits them:
- SubscriptionManager.ShutdownAsync awaits the publish loop alongside the
condition-refresh worker it already awaited. Neither loop takes the
manager semaphore, so awaiting under it cannot deadlock.
- SessionManager gains ShutdownAsync; ISessionManager.Shutdown() is
[Obsolete] and StandardServer calls the async overload. Without it the
server could finish tearing down while the monitor was still closing
expired sessions and raising keep-alive events against half-disposed
state. This mirrors ISubscriptionManager, which already had
ShutdownAsync. Migration notes added for implementers.
MonitorSessionsAsync also stops blocking a thread-pool thread on
ManualResetEvent.WaitOne(sleepCycle) for the whole cycle; it awaits the
TimeProvider instead and honours the token, matching the publish loop. The
in-flight CloseSessionAsync is deliberately passed CancellationToken.None so
shutdown cannot abandon a session mid-close.
KubernetesReadinessServer spawned one unowned handler per inbound request on
an unauthenticated endpoint - no bound on a flood, and nothing to wait for
at shutdown. Handlers now take one of a fixed number of slots, and disposal
drains them by reclaiming every slot, under a timeout so a wedged client
socket cannot stall it.
Verified: Core 4153, Sessions 777, Redundancy.Kubernetes 97 and the
channel-manager fixtures all pass; Server.Tests is 4023 passed with one
failure that reproduces identically on the unmodified tree (a local
certificate-store private-key issue).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
A flat floor over changed lines punishes small changes for arithmetic. Two uncovered lines in a four-line fix reads as 50% and failed an 80/5 floor, which is how this very branch ended up with a red coverage check for a four-line predicate change. A check that fires on changes nobody considers undertested is a check people learn to ignore. The floor is now graduated by how many coverable lines the patch touches: 1-10 lines -> 50%, advisory (warns, does not fail) 11-100 lines -> 60%, advisory (warns, does not fail) >100 lines -> target-threshold (75%), enforced Only changes past the last band can fail. At that size the percentage carries real information and a large untested change is exactly what the check is for. Below it the author still gets a warning naming the uncovered lines, so nothing goes silent - it just stops blocking. Bands live in coverage-thresholds.json (patch.bands), are consulted in order, and each carries its own enforced flag, so the policy can be tuned or made blocking without touching the script. Verified end to end against synthetic repositories and Cobertura reports at every boundary: 4 lines at 25% and 50 lines at 50% warn and pass; 50 lines at 70% and 200 lines at 80% pass cleanly; 200 lines at 50% fails. Band selection checked at 1, 2, 10, 11, 50, 100, 101 and 500 changed lines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
The codebase had 26 bare `_ = Task.Run(...)` / `Task.Factory.StartNew(...)`
sites across seven assemblies. Each one handed work to the thread pool and
forgot it: nothing observed the exception if it threw, nothing bounded how
many ran at once, and disposal raced them. A component could finish tearing
itself down while work it started was still touching its fields - which is
how the reconnect-cycle hang earlier on this branch presented, and why two
of these sites already carried a standing TODO asking for exactly this.
Adds BackgroundTaskScope in Opc.Ua.Types: schedule with Run, drain with
DisposeAsync, or signal-only with Dispose where the owner's teardown is
synchronous and awaiting the drain would be sync over async. Run never
blocks and never throws, which matters because most of these call sites
went to the thread pool precisely to escape a lock they were holding. It
also observes failures through source-generated logging and takes an
optional concurrency cap.
Every site now belongs to something:
Core - the channel manager owns lease release and the reconnect
cycle; UaSCUaBinaryChannel owns state-change dispatch and the
queued-response reset. ChannelAsyncOperation is per-operation
with no owner to hang a drain on, so it just gains the same
try/catch the synchronous branch beside it always had - the
async branch was silently swallowing subscriber exceptions.
Client - session (publish notifications), managed session (certificate
rotation), both subscription flavours (recreate-after-transfer),
the classic engine (publish throttle, orphan delete), the
composite collection (idle delete) and the replica coordinator
(role change).
Server - sampling groups, the configuration node manager's deferred
applies, the revert window, retired-generation drain, and
expired-subscription cleanup, which is now threaded a scope
instead of reaching for a static.
Other - PubSub metadata publishing, the GDS keep-alive cleanup and the
NIC capture stop.
The Kubernetes readiness endpoint keeps its purpose-built semaphore: it
needs a hard cap on concurrent handlers, not just an owner.
ISessionManager.Shutdown() is removed rather than obsoleted. There is no
correct synchronous way to wait for the session monitor loop, so leaving
the overload in place would only preserve the race.
Verified: Types 8467, Core 4153, Client 2123, Sessions 777, Subscriptions
604, Redundancy.Kubernetes 97 all pass. Server.Tests is 4023 passed with
one failure that reproduces identically on the unmodified tree (a local
certificate-store private-key issue). BackgroundTaskScope itself has 11
tests covering drain, cancellation, exception observation, the concurrency
cap, post-shutdown rejection and idempotent disposal.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Codecov was dropped in #4122 when the in-pipeline gate replaced it. The gate was the right call - it can see how much a patch changed, which codecov's flat thresholds cannot - but throwing codecov out took the pull-request comment, the file-by-file diff view and the coverage trend with it, and those are the things it is genuinely better at. So it comes back as reporting only. Both codecov statuses are informational: true. Two gates with two sets of thresholds would sooner or later disagree about the same pull request, and the one easier to silence would win; coverage-thresholds.json stays the single source of truth. The upload is off-switchable on both systems and can never fail a build: Azure - the enableCodecov pipeline parameter (default true) Actions - the ENABLE_CODECOV workflow env (default 'true') Both also skip themselves when CODECOV_TOKEN is absent, which is what happens on fork pull requests. The Actions condition resolves the secret into a job-level env first, because the secrets context is not available to a step's if. Each system uploads the single merged report it already builds - not one per test job - under its own flag (azure, actions), since the two matrices deliberately cover different legs. Two details worth calling out: - The old step used bash <(curl codecov.io/bash), which Codecov sunset. This uses the current CLI, fetched as a binary so the step does not need a Python toolchain. - On an Azure pull request build Build.SourceVersion is the merge commit Azure synthesised, which codecov cannot match to the pull request. The step reports System.PullRequest.SourceCommitId when there is one. codecov.yml validated against https://codecov.io/validate ("Valid!"), and its ignore list is kept in step with coverage-thresholds.json so the two measure the same code. All four YAML files parse; the upload script passes bash -n. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (74.11%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #4158 +/- ##
==========================================
+ Coverage 80.23% 80.53% +0.29%
==========================================
Files 1515 1719 +204
Lines 209980 238494 +28514
Branches 36213 41276 +5063
==========================================
+ Hits 168479 192068 +23589
- Misses 28867 32114 +3247
- Partials 12634 14312 +1678
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The repo rule is categorical: a synchronous sync root must be
System.Threading.Lock, never a bare object. The partition lock broke it in
a way a local fix could not reach - it is minted by LogicalSubscription and
handed to CompositeMonitoredItemCollection through a constructor parameter
typed `object`, so the field, the parameter and every caller had to move
together. CompositeMonitoredItemCollection is internal, so widening the
parameter to Lock is not an API change.
Every use on both sides is a plain `lock` statement - no Monitor.Enter/Exit
and no Wait/Pulse - so Lock is a straight substitution rather than a
behaviour change.
Two more of the same violation are fixed while here, both self-contained:
- PrioritizedChannel.m_lock, all plain locks including the nested class
that locks m_channel.m_lock.
- ClassicSubscriptionEngine.m_acknowledgementsToSendLock, which also
needed Debug.Assert(Monitor.IsEntered(x)) to become
x.IsHeldByCurrentThread. Monitor.IsEntered still compiles against a
Lock, because Lock is an object, but it asks the wrong question.
That leaves exactly one object-typed sync root in src:
DiagnosticsNodeManager.m_diagnosticsLock. It is not an oversight - it is
passed into the generated ServerDiagnosticsSummaryValue / SessionDiagnostics
wrapper types, whose constructors take `object`. Converting it means
changing public API in Opc.Ua.Core.Types and the source generator that emits
those wrappers, which is its own change rather than a rider on this one.
Verified: Opc.Ua.Client builds clean on every target framework, so the
netstandard2.0 / net472 / net48 Lock polyfill (which supplies
IsHeldByCurrentThread) carries it. Client 2123 and Subscriptions 604 tests
pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
Collaborator
Author
|
/azp run OPCFoundation.UA-.NETStandard |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The first Azure run of the new upload step failed with "Commit creating failed: Repository not found". The CLI detects azure_pipelines and then looks for an Azure Repos project, so on a pipeline that builds a GitHub repository it never resolves owner/repo. The GitHub Actions upload succeeded on the same commit precisely because the action supplies the slug for you. Pass it explicitly, along with the git service. Build.Repository.Name is already the 'owner/name' slug for a GitHub-backed pipeline, so this stays correct if the repository is ever renamed or forked. The step is continueOnError, so this only ever showed up as succeededWithIssues on the advisory Code coverage stage - it could not have blocked anything - but a step that fails on every build is noise that teaches people to ignore the stage. Flags verified against `codecov upload-process --help`: -r/--slug and --git-service both exist and take these values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
The previous attempt passed --slug $(Build.Repository.Name), which is empty on this pipeline: for a GitHub-backed Azure pipeline the owner/repo string lives in Build.Repository.ID, and the build REST API confirms it - repository.id is "OPCFoundation/UA-.NETStandard" while repository.name is "". So the CLI was handed an empty slug and kept reporting "Repository not found", which read like a bad token but was not. Use Build.Repository.ID, and fall back to parsing Build.Repository.Uri when it is empty, so the step also works on a fork or if Azure changes which variable it fills. The resolved slug is echoed before the upload so the next person does not have to guess what it was. The parser handles the shapes Azure emits, verified against all three: https://github.com:443/owner/repo (what this pipeline reports) https://github.com/owner/repo.git Build.Repository.ID already set Still continueOnError, so this never had the power to fail a build - it only kept the advisory Code coverage stage permanently at succeededWithIssues. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
marcschier
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two changes to how CI is routed and how it reports.
1. Split the CI matrix back across both systems
Flips both halves of the CI backend switch from
adotoactions, so the workload is shared instead of concentrated on the managed pool. The switch is checked into source in two places that must always be flipped together:azure-pipelines.ymlparameters.ciBuildBackendadoactions.github/workflows/buildandtest.ymlenv.CI_BUILD_BACKENDadoactionsbuild-all-tfm-windows+-linuxnet10.0,net48)net48+net10.0fast PR legs2. Split the required gate from the advisory coverage check
The coverage gate job was appended to every test stage, so
Fast PR testemitted three sibling checks whose display names collided —Coverage Release (net10.0)appeared twice, once for the Windows leg and once for macOS, with nothing to tell them apart:That job also fused two unrelated concerns: whether the tests passed (which must block a merge) and whether coverage met its thresholds (which should not — the
net10.0leg hadenforceCoverage: true, so a coverage dip blocked merges).Both concerns now get a stage of their own, mirrored in both CI systems:
Tests passedstagebuild-and-test summaryjobCode coveragestagecode coveragejobThe coverage check reports a clean failure when the thresholds are missed, so the miss is visible, but it never blocks the merge.
Coverage is now merged once across every leg of the run instead of once per leg, which also removes the
enforceCoverage/publishCoverage"exactly one invocation may set this" parameters. Neither check re-runs any tests — the previous design re-ran the whole suite in one job with the collector attached, which serialised a suite that is deliberately fanned out and always exceeded the stage timeout.Coverage checks now exist in GitHub Actions
build-and-testalready ran with--collect:"XPlat Code Coverage"and uploaded the fragments, but nothing ever merged or evaluated them. It now runs the samecheck-coverage.ps1against the samecoverage-thresholds.jsonas Azure. SinceCI_BUILD_BACKENDdefaults toactions, that ubuntu matrix — which runs every test project — is the fullest coverage signal in the repository.Reporting the numbers
check-coverage.ps1gained-SummaryPath, rendering a markdown report that both systems surface:::error::annotations.##vso[task.uploadsummary].The report is a pass/fail table of the project floor, patch coverage and baseline delta, with the uncovered changed lines listed by file in a collapsible block. Rendered example:
The comment is fork-safe: a
pull_requestfrom a fork gets a read-only token regardless of thepermissions:block, so it degrades to the job summary alone. The sticky comment usesactions/github-scriptrather than a community action, keeping this workflow on first-party actions only.Notable correctness details
These were found and fixed during review, and are the parts most worth a second pair of eyes:
dependencies.<stage>.resultresolves at stage scope only. At job scopedependenciesrefers to other jobs of the same stage, so reading stage results from a jobs template would have silently returned empty and the gate would have passed everything.gate.ymlandcoverage.ymlare therefore stage templates that derive their owndependsOnfrom the same stage list they evaluate, so the two can never drift apart. An unresolved result is a hard error.not(canceled())/always(), not on success. A check that reports skipped is treated by GitHub as satisfied, so a required check that skips when its dependency fails would wave a red build straight through.get-matrix.ps1used to emit an empty matrix on a discovery miss. The downstream job is then skipped, a skipped job rolls up as a succeeded stage, and the gate would have approved a run that executed no tests. It now fails, with an-AllowEmptyopt-out.Test-host hang and the reconnect defect behind it — both fixed
Flipping
CI_BUILD_BACKENDtoactionsstarted exercising the ubuntu matrix, whichmasternever runs, and it immediately surfaced two real defects.1. The test host hung for ten minutes.
Assert.ThrowsAsyncblocks the calling threadwith no timeout, and these tests drive a fake clock from that same thread — so once the
reconnect parked on a fake-clock timer, nothing could ever advance it. NUnit sat in
WaitForCompletionuntil--blame-hang-timeoutkilled the host, aborting the run at 3902of 4238 tests and taking every other test in the assembly down with it. A hang dump named
the culprit exactly. Replaced with an awaited, bounded
AssertThrowsAsynchelper (therepo's own rules forbid sync over async). The run now completes in ~1m29s with all
tests executed.
2. A budget-aware reconnect never returned. With the hang gone, two tests failed with
Expected ServiceResultException but got TimeoutException.dumpasyncon a hang dump gavethe whole chain:
master(via #4128) addedIsTerminalReconnectRaceso a reconnect that loses a raceagainst a concurrent close recovers on a freshly swapped entry. The predicate only asks
whether the entry ended up
Closed/FaultedwithBadSecureChannelClosed— which isalso exactly how a reconnect ends when the caller's retry budget or the reconnect policy
is deliberately exhausted. So a budget-exhausted reconnect was misread as a race: the
manager swapped the entry and started a second, unbudgeted cycle, waiting out the swap
back-off first.
ReconnectAsync(channel, budget)therefore could not return within itsbudget, and under a fake clock it never returned at all.
The fix requires the budget to have room left before treating a terminal failure as a race:
The parameterless
ReconnectAsync(channel, ct)overload passes an unlimited budget, sogenuine race recovery is unchanged. Verified locally: Core 4152, Client 2122 and Sessions
776 tests all pass, and the full GitHub Actions PR run is green across 56 jobs.
Two smaller fixes came along the way, neither weakening an assertion:
ObservableFakeTimeProviderwaiters made relative ("N more timers from now") ratherthan absolute ("the Nth timer of the run"). Absolute numbering let an earlier reconnect
consume the slots, so a waiter completed before the awaited timer existed and
Advance()fired nothing.
NodeStateHandlerConcurrencyTestswas a foreground threadwhose stop flag was read without a barrier and set only on the success path — a failed
assertion leaked a spinning thread that pins the process at exit. Now background,
Volatile, stopped in afinally, joined with a bound.Background work now has an owner
The reconnect hang above was one symptom of a pattern the codebase had 26 times across seven assemblies: a bare
_ = Task.Run(...). Each one handed work to the thread pool and forgot it — nothing observed the exception if it threw, nothing bounded how many ran at once, and disposal raced them, so a component could finish tearing itself down while work it started was still touching its fields. Two of those sites already carried a standing// TODO: Await the task completion in shutdown.BackgroundTaskScope(inOpc.Ua.Types) gives them an owner: schedule withRun, drain withDisposeAsync, or signal-only withDisposewhere the owner's teardown is synchronous and awaiting would be sync over async.Runnever blocks and never throws — which matters, because most of these call sites went to the thread pool precisely to escape a lock they were holding. 11 unit tests cover drain, cancellation, exception observation, the concurrency cap, post-shutdown rejection and idempotent disposal.Two of the fixes were more than re-wiring:
SessionManager/SubscriptionManagerloops were structurally unawaitable. Both returnValueTask, soTask.Factory.StartNew(() => ...)handed back aTask<ValueTask>that completes at the first yield. Storing it would not have helped — awaiting it would only have awaited the scheduling. Both now go throughAsTask().Unwrap(), matching the neighbouringStartConditionRefreshWorker.ChannelAsyncOperationwas swallowing subscriber exceptions. ItsdoNotBlockbranch had no try/catch while the inline branch beside it did. It is per-operation with no owner to hang a drain on, so it gained the missing handler rather than a scope.The Kubernetes readiness endpoint keeps a purpose-built semaphore instead: an unauthenticated endpoint needs a hard cap on concurrent handlers, not just an owner.
ISessionManager.Shutdown()is removedReplaced by
ShutdownAsync(CancellationToken). There is no correct synchronous way to wait for the session monitor loop, so leaving the overload would only have preserved the race. This mirrorsISubscriptionManager, which already hadShutdownAsync. Migration notes are indocs/MigrationGuide.md; only one in-repo implementer existed.Also converted the injected
object partitionLockshared byLogicalSubscriptionandCompositeMonitoredItemCollectiontoSystem.Threading.Lock, per the repo rule.Patch coverage now scales with the size of the patch
A flat floor over changed lines punishes small changes for arithmetic: two uncovered lines in a four-line fix reads as 50 % and failed an 80/5 floor — which is exactly how this branch earned a red coverage check for a four-line predicate change. A check that fires on changes nobody considers undertested is one people learn to ignore.
Only changes past the last band can fail. Below it the author still gets a warning naming the uncovered lines. Bands live in
patch.bandsincoverage-thresholds.jsonand each carries its ownenforcedflag. Verified end-to-end against synthetic repositories at every boundary.Codecov is back, as reporting
Dropped in #4122 when the in-pipeline gate replaced it. Keeping the gate was right, but dropping Codecov also lost the PR comment, the file-by-file diff view and the trend. It returns with both statuses
informational: true— two gates with two sets of thresholds would eventually disagree, andcoverage-thresholds.jsonstays the single source of truth.The upload is switchable and can never fail a build:
enableCodecov(Azure) andENABLE_CODECOV(Actions), both defaulting on, both skipped whenCODECOV_TOKENis absent as on fork PRs. Each system uploads the single merged report it already builds, under its own flag. The oldcodecov.io/bashuploader was sunset, so Azure uses the current CLI.Follow-up actions for maintainers
Update the branch ruleset. The per-leg
Coverage Release (…)checks are gone. Azure reports checks as<pipeline> (<stage> <job>), so the exact names are:OPCFoundation.UA-.NETStandard (Tests passed Verify stage results)build-and-test summaryOPCFoundation.UA-.NETStandard (Code coverage Merge and evaluate)code coverageRe-calibrate the thresholds.
coverage-thresholds.jsonis still seeded from the last Codecov reading (73.61 %). The two systems now report different numbers by design — Actions merges every project on ubuntu, Azure only the Windows fast-PR legs, and scheduled runs read higher still because the Debug / .NET 8 / .NET 9 / netstandard stages also contribute. Re-seed from the GitHub Actions figure.Note: a failed advisory coverage stage still turns the whole Azure run red, which the README build badge reflects on
master. That follows from wanting a visible red X on a threshold miss; say the word if you would rather it were a warning.The Azure agent pool is the slowest thing here. Several runs on this PR sat queued for hours, and one build failed with
Insufficient system resourceswhile writing an XML doc file — an agent exhaustion error, not a code error. It passed on re-run. Worth raising against thenetstandardManaged DevOps Pool capacity separately.Related Issues
No tracking issue; this is CI infrastructure work requested directly. It builds on the switch introduced in #4122.
Checklist
check-coverage.ps1run for real against a merged Cobertura report on all three paths — pass, threshold failure, and patch coverage with uncovered changed lines (a scratch commit adding an uncovered method was correctly resolved toDataTypeException.cs:193, reported 0 % patch coverage and exited 1).Succeeded/SucceededWithIssues/Skipped/ empty /Failed/Canceled— 6/6 correct.get-matrix.ps1exit codes verified in clean processes: empty → 1, normal → 0,-AllowEmpty→ 0.docs/DeveloperGuide.mdas Required checks and coverage, covering which checks belong in the ruleset and which must not, how coverage is measured, where the numbers appear, and why the two systems disagree. Corrected the stale "BLOCKS" wording incoverage-thresholds.json.Opc.Ua.Core.Schema.Testswas run repeatedly on net10.0 (119 passed) to generate the real Cobertura fragments used to validate the gate.docs/DeveloperGuide.mdis fixed in15b8e38df.863eefc4d.