3 Commits

Author	SHA1	Message	Date
romanett	9b034f8291	Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700) ... # Description Nullable reference types are now enabled once, in `common.props`, and every project in the repository builds warning-clean with them. **Configuration** - `<Nullable>enable</Nullable>` lives in `common.props`. All per-project `<Nullable>` settings are removed: 178 project files (enable, annotations, and the `disable` in the three Aot test projects). - 419 per-file `#nullable enable`/`restore` directives and the 4 `#nullable disable` directives are removed. - Kept on purpose: - The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`, `PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the project setting in generated code, so the directive is required there. - `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against the 1.5.378 packages and does not import the repository props. - `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now describe the central setting. **Migration** (the 43 unset and 13 annotations-only projects, roughly 10k diagnostics) - **Production code: annotated at the declarations.** Covers `tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the LDS sample. - Members, parameters and returns are nullable where null actually flows. - Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`, `IsMethodTypeDesign`). - `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take `params object?[]`. `Template.AddReplacement` takes `string?` (rendering already skips null replacements). - `IRegisteredServerStore.RegisterAsync` returns `Task<RegistrationEntry?>`: it returns null when a registration is removed. - Dereferences backed by an invariant (for example, the validated model) use `!`, so runtime behaviour is unchanged. - **Behavioural changes in production code** (small, deliberate): - `NodeSetToModelDesign` throws `InvalidOperationException` when the file is not a NodeSet. Before, it threw a NullReferenceException on the next line. - `ModelDesignValidator` starts its lookup tables empty instead of null. A redundant `if (m_nodes == null) return;` guard is gone. - **Test code: minimal annotations.** - `?` on values that can be null, `!` where the test asserts non-null. - `= null!` for fixture members assigned in SetUp. - `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the `IEncodeable` constraint. - A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`) folded into the adjacent `IsNull`/`Count` checks. **Notes for reviewers** - The diff is large (~1,500 files) but almost entirely mechanical: `?`, `!` and directive removal. The non-annotation changes are listed above. - Known follow-up, not done here: `IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay non-nullable even though the validator returns null before it loads a dictionary. Making them nullable would ripple through every generator call site. ## Related Issues - Fixes #4439 ## 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. - [ ] 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. - [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. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. The "no new warnings" box covers net10.0 and net48: full non-incremental builds of `UA.slnx`, `tools/SourceGeneration.slnx`, `tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0 warnings and 0 errors under the repository's warnings-as-errors settings. net9.0 and net8.0 also had no nullable diagnostics, but they were not rebuilt after the final fixes. Tests have not been run locally yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-10-08 08:48:13 +02:00
Marc Schier	416d619c41	Split the CI matrix across both systems, split the required test gate from the advisory coverage check, and give background work an owner (#4158) ... # 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 `ado` to `actions`, 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**: | File | Setting | Before | After | | --- | --- | --- | --- | | `azure-pipelines.yml` | `parameters.ciBuildBackend` | `ado` | `actions` | | `.github/workflows/buildandtest.yml` | `env.CI_BUILD_BACKEND` | `ado` | `actions` | | Work | GitHub Actions | Azure Pipelines (managed pool) | | --- | --- | --- | | All-TFM solution builds | `build-all-tfm-windows` + `-linux` | two-TFM PR gate (`net10.0`, `net48`) | | Test matrix | ubuntu, every test project | Windows `net48` + `net10.0` fast PR legs | | Native AoT | ubuntu | Schedule/Manual only | | macOS | always (no managed-pool macOS image) | — | ## 2. Split the required gate from the advisory coverage check 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: ```text ✓ Coverage Release (net48) 55s ● Coverage Release (net10.0) ○ Coverage Release (net10.0) ``` 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.0` leg had `enforceCoverage: true`, so a coverage dip blocked merges). Both concerns now get a stage of their own, mirrored in both CI systems: | Concern | Azure Pipelines | GitHub Actions | In the branch ruleset? | | --- | --- | --- | --- | | Every test passed | **`Tests passed`** stage | **`build-and-test summary`** job | **Yes — required** | | Coverage meets thresholds | **`Code coverage`** stage | **`code coverage`** job | **No — advisory** | The 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-test` already ran with `--collect:"XPlat Code Coverage"` and uploaded the fragments, but nothing ever merged or evaluated them. It now runs the same `check-coverage.ps1` against the same `coverage-thresholds.json` as Azure. Since `CI_BUILD_BACKEND` defaults to `actions`, that ubuntu matrix — which runs *every* test project — is the fullest coverage signal in the repository. ### Reporting the numbers `check-coverage.ps1` gained `-SummaryPath`, rendering a markdown report that both systems surface: - **GitHub Actions** — appended to the job summary and upserted as a single **sticky pull-request comment**, updated in place on each run. Threshold misses also appear as `::error::` annotations. - **Azure Pipelines** — attached to the build summary via `##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: > ## Code coverage > > ❌ **Coverage gate failed.** This check is advisory and does not block the merge. > > | Check | Result | Threshold | > | --- | --- | --- | > | ✅ Project line rate | **72.10%** (40 731/56 496 lines) | >= 70.00% | > | ❌ Patch coverage | **0.00%** (0/1 changed lines) | >= 75.00% | The comment is fork-safe: a `pull_request` from a fork gets a read-only token regardless of the `permissions:` block, so it degrades to the job summary alone. The sticky comment uses `actions/github-script` rather 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>.result` resolves at *stage* scope only.** At job scope `dependencies` refers 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.yml` and `coverage.yml` are therefore **stage** templates that derive their own `dependsOn` from the same stage list they evaluate, so the two can never drift apart. An unresolved result is a hard error. - **Both rollups run on `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.ps1` used 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 `-AllowEmpty` opt-out. - **Zero coverage reports used to be a green advisory check**, which would hide a broken collector indefinitely. Both systems now distinguish "the tests never ran" (warning) from "the tests ran and published nothing" (error). - The gate reads stage-level results rather than enumerating matrix-generated job names, which change whenever a test project or agent is added. ## Test-host hang and the reconnect defect behind it — both fixed Flipping `CI_BUILD_BACKEND` to `actions` started exercising the ubuntu matrix, which `master` never runs, and it immediately surfaced two real defects. **1. The test host hung for ten minutes.** `Assert.ThrowsAsync` blocks the calling thread with 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 `WaitForCompletion` until `--blame-hang-timeout` killed the host, aborting the run at 3902 of 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 `AssertThrowsAsync` helper (the repo'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`. `dumpasync` on a hang dump gave the whole chain: ``` ReconnectAsyncWithBudgetShrinksDelayToFitRemainingAsync → AssertThrowsAsync → ClientChannelManager.ReconnectAsync → ClientChannelManager.ReconnectLeaseAsync → ClientChannelManager.SwapFaultedEntryAsync ← parked here ``` `master` (via #4128) added `IsTerminalReconnectRace` so a reconnect that loses a race against a concurrent close recovers on a freshly swapped entry. The predicate only asks whether the entry ended up `Closed`/`Faulted` with `BadSecureChannelClosed` — which is *also* 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 its budget, 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: ```csharp return !ct.IsCancellationRequested && !budget.IsExhausted && sre.StatusCode == StatusCodes.BadSecureChannelClosed && entry.State is ChannelState.Closed or ChannelState.Faulted; ``` The parameterless `ReconnectAsync(channel, ct)` overload passes an unlimited budget, so genuine 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: - `ObservableFakeTimeProvider` waiters made **relative** ("N more timers from now") rather than 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. - The raw worker thread in `NodeStateHandlerConcurrencyTests` was a **foreground** thread whose 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 a `finally`, 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` (in `Opc.Ua.Types`) gives them an owner: schedule with `Run`, drain with `DisposeAsync`, or signal-only with `Dispose` where the owner's teardown is synchronous and awaiting 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. 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` / `SubscriptionManager` loops were structurally unawaitable.** Both return `ValueTask`, so `Task.Factory.StartNew(() => ...)` handed back a `Task<ValueTask>` that completes at the first yield. Storing it would not have helped — awaiting it would only have awaited the *scheduling*. Both now go through `AsTask().Unwrap()`, matching the neighbouring `StartConditionRefreshWorker`. - **`ChannelAsyncOperation` was swallowing subscriber exceptions.** Its `doNotBlock` branch 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. ### ⚠️ Breaking: `ISessionManager.Shutdown()` is removed Replaced 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 mirrors `ISubscriptionManager`, which already had `ShutdownAsync`. Migration notes are in `docs/MigrationGuide.md`; only one in-repo implementer existed. Also converted the injected `object partitionLock` shared by `LogicalSubscription` and `CompositeMonitoredItemCollection` to `System.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. | Coverable changed lines | Floor | Below it | | --- | --- | --- | | 1 – 10 | 50 % | warning, still passes | | 11 – 100 | 60 % | warning, still passes | | more than 100 | 75 % | **failure** | Only changes past the last band can fail. Below it the author still gets a warning naming the uncovered lines. Bands live in `patch.bands` in `coverage-thresholds.json` and each carries its own `enforced` flag. 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, and `coverage-thresholds.json` stays the single source of truth. The upload is switchable and can never fail a build: `enableCodecov` (Azure) and `ENABLE_CODECOV` (Actions), both defaulting on, both skipped when `CODECOV_TOKEN` is absent as on fork PRs. Each system uploads the single merged report it already builds, under its own flag. The old `codecov.io/bash` uploader was sunset, so Azure uses the current CLI. ## Follow-up actions for maintainers 1. **Update the branch ruleset.** The per-leg `Coverage Release (…)` checks are gone. Azure reports checks as `<pipeline> (<stage> <job>)`, so the exact names are: | Check | Require? | | --- | --- | | `OPCFoundation.UA-.NETStandard (Tests passed Verify stage results)` | **yes** | | `build-and-test summary` | **yes** (already required) | | `OPCFoundation.UA-.NETStandard (Code coverage Merge and evaluate)` | no | | `code coverage` | no | 2. **Re-calibrate the thresholds.** `coverage-thresholds.json` is 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. 3. **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. 4. **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 resources` while writing an XML doc file — an agent exhaustion error, not a code error. It passed on re-run. Worth raising against the `netstandard` Managed 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 - [x] 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. - No product code changed, so there are no unit tests to add. The new logic was verified with purpose-built harnesses instead: - `check-coverage.ps1` run 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 to `DataTypeException.cs:193`, reported 0 % patch coverage and exited 1). - The gate's accept/reject logic dry-run over `Succeeded` / `SucceededWithIssues` / `Skipped` / empty / `Failed` / `Canceled` — 6/6 correct. - `get-matrix.ps1` exit codes verified in clean processes: empty → 1, normal → 0, `-AllowEmpty` → 0. - All five pipeline/workflow files parse; both PowerShell scripts parse. - Action versions checked against the GitHub API and pinned to current majors. - [x] I have added all necessary documentation. - Rewrote the coverage section of `docs/DeveloperGuide.md` as *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 in `coverage-thresholds.json`. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - CI YAML, two PowerShell scripts and documentation only; no compilation involved. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - Not applicable — no product code changed. `Opc.Ua.Core.Schema.Tests` was run repeatedly on net10.0 (119 passed) to generate the real Cobertura fragments used to validate the gate. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. - Feedback on the path-relevance diff (now a merge-base diff) and the target-framework numbering in `docs/DeveloperGuide.md` is fixed in `15b8e38df`. - A review pass over the gate/coverage split produced the four correctness fixes listed above, in `863eefc4d`. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca	2026-08-07 06:56:05 +02:00
Marc Schier	7cec2d614d	Move repository files to the consolidated layout	2026-07-16 06:25:54 +02:00