diff --git a/.azurepipelines/check-coverage.ps1 b/.azurepipelines/check-coverage.ps1 index ff62d84867..7ba33346af 100644 --- a/.azurepipelines/check-coverage.ps1 +++ b/.azurepipelines/check-coverage.ps1 @@ -3,22 +3,26 @@ Enforces the repository's code-coverage gates against a Cobertura report. .DESCRIPTION - Replaces the retired codecov/project and codecov/patch status checks with a - self-contained gate that runs inside the pipeline. Three checks are made + The enforced coverage gate, run inside the pipeline so it can react to how + much a patch actually changed. codecov.io is still uploaded to, but purely + for reporting - its own status checks are `informational: true` (see + codecov.yml) precisely so there is only ever one gate. Three checks are made against the merged Cobertura report produced by ReportGenerator: 1. Project floor (BLOCKING) - total line and branch rates must meet the absolute floors in coverage-thresholds.json. - 2. Patch coverage (BLOCKING) - lines added or modified relative to the pull - request's base must reach the patch target, - tolerating the configured threshold. This - mirrors the old codecov/patch semantics. + 2. Patch coverage (GRADUATED)- lines added or modified relative to the pull + request's base must reach a floor that + scales with the size of the patch: small + patches warn, large ones fail. See + 'patch.bands' in coverage-thresholds.json. 3. Baseline delta (ADVISORY) - reports how the current total line rate compares to the recorded master baseline. Never fails the build. Files matching the 'ignore' globs in coverage-thresholds.json are excluded - from the patch calculation, exactly as they were excluded by codecov.yml. + from the patch calculation. Keep that list in step with the 'ignore' list in + codecov.yml so both report on the same code. When no base ref is supplied - a scheduled or master build rather than a pull request - the patch gate is skipped and only the project floor is enforced. @@ -40,6 +44,12 @@ .PARAMETER SkipFetch Do not run 'git fetch' for the base ref. Used by the unit tests, which operate on a purpose-built local repository. + + .PARAMETER SummaryPath + Optional path to write a markdown summary of the gate result to. Azure + Pipelines attaches it with '##vso[task.uploadsummary]' and GitHub Actions + appends it to $GITHUB_STEP_SUMMARY and to the pull request comment, so the + numbers are visible without opening the log. #> [CmdletBinding()] @@ -49,7 +59,8 @@ Param( [string] $ThresholdsPath = '', [string] $BaseRef = '', [string] $RepoRoot = '', - [switch] $SkipFetch + [switch] $SkipFetch, + [string] $SummaryPath = '' ) $ErrorActionPreference = 'Stop' @@ -68,11 +79,16 @@ if ([string]::IsNullOrWhiteSpace($ThresholdsPath)) { } $script:IsAzurePipeline = -not [string]::IsNullOrEmpty($env:TF_BUILD) +$script:IsGitHubActions = $env:GITHUB_ACTIONS -eq 'true' function Write-GateError([string] $message) { if ($script:IsAzurePipeline) { Write-Host "##vso[task.logissue type=error]$message" } + if ($script:IsGitHubActions) { + # Workflow command: surfaces the message as an annotation on the run. + Write-Host "::error title=Coverage gate::$message" + } Write-Host "ERROR: $message" } @@ -80,9 +96,92 @@ function Write-GateWarning([string] $message) { if ($script:IsAzurePipeline) { Write-Host "##vso[task.logissue type=warning]$message" } + if ($script:IsGitHubActions) { + Write-Host "::warning title=Coverage gate::$message" + } Write-Host "WARNING: $message" } +<# +.SYNOPSIS +Selects the patch-coverage requirement that applies to a change of a given size. + +.DESCRIPTION +A coverage percentage over a handful of lines carries almost no information: a +single uncovered line in a two-line fix reads as 50%, which a flat floor would +fail even though nothing is wrong. Small changes therefore get a lower bar and +report a warning rather than a failure, while changes large enough for the +percentage to mean something are enforced. Bands are consulted in order and the +first one whose maxChangedLines covers the patch wins; anything larger falls +through to the enforced target. + +.PARAMETER changedLines +Number of coverable changed lines in the patch. + +.PARAMETER patch +The 'patch' object from coverage-thresholds.json. +#> +function Get-PatchBand([int] $changedLines, $patch) { + $bands = @($patch.bands) + foreach ($band in $bands) { + if ($null -eq $band) { continue } + if ($changedLines -le [int]$band.maxChangedLines) { + return [pscustomobject]@{ + Floor = [double]$band.target + Enforced = [bool]$band.enforced + Scope = ('<= {0} changed lines' -f [int]$band.maxChangedLines) + } + } + } + + $largest = if ($bands.Count -gt 0) { [int]$bands[-1].maxChangedLines } else { 0 } + return [pscustomobject]@{ + Floor = [double]$patch.target - [double]$patch.threshold + Enforced = $true + Scope = ('> {0} changed lines' -f $largest) + } +} + +# Markdown summary accumulated as the gate runs and written to -SummaryPath at +# the end. Kept separate from the console output because the console log is a +# flat transcript while this is rendered as a table in both CI UIs. +$script:SummaryRows = [System.Collections.Generic.List[string]]::new() +$script:SummaryNotes = [System.Collections.Generic.List[string]]::new() + +<# + .SYNOPSIS + Adds a row to the markdown summary table. + + .PARAMETER check + Name of the check, for example 'Project line rate'. + + .PARAMETER value + The measured value, already formatted. + + .PARAMETER target + The threshold the value is compared against, or '-' when there is none. + + .PARAMETER state + One of 'pass', 'fail', 'warn' or 'info'. +#> +function Add-SummaryRow([string] $check, [string] $value, [string] $target, [string] $state) { + $icon = switch ($state) { + 'pass' { ':white_check_mark:' } + 'fail' { ':x:' } + 'warn' { ':warning:' } + default { ':information_source:' } + } + $script:SummaryRows.Add(('| {0} {1} | {2} | {3} |' -f $icon, $check, $value, $target)) +} + +<# + .SYNOPSIS + Adds a free-form markdown note below the summary table. +#> +function Add-SummaryNote([string] $note) { + $script:SummaryNotes.Add($note) +} + <# .SYNOPSIS Converts a repository-relative glob into an anchored regular expression. @@ -427,19 +526,29 @@ $failures = @() $minLine = [double]$thresholds.project.minimumLineRate if ($null -eq $totals.LineRate) { $failures += 'No coverable lines were found in the report; the run did not produce usable coverage.' + Add-SummaryRow 'Project line rate' 'no data' ('>= {0:N2}%' -f $minLine) 'fail' } elseif ($totals.LineRate -lt $minLine) { $failures += ('Total line coverage {0:N2}% is below the required floor of {1:N2}%.' -f $totals.LineRate, $minLine) + Add-SummaryRow 'Project line rate' ('**{0:N2}%** ({1}/{2} lines)' -f $totals.LineRate, $totals.CoveredLines, $totals.TotalLines) ('>= {0:N2}%' -f $minLine) 'fail' +} +else { + Add-SummaryRow 'Project line rate' ('**{0:N2}%** ({1}/{2} lines)' -f $totals.LineRate, $totals.CoveredLines, $totals.TotalLines) ('>= {0:N2}%' -f $minLine) 'pass' } $minBranch = [double]$thresholds.project.minimumBranchRate if ($null -ne $totals.BranchRate -and $totals.BranchRate -lt $minBranch) { $failures += ('Total branch coverage {0:N2}% is below the required floor of {1:N2}%.' -f $totals.BranchRate, $minBranch) + Add-SummaryRow 'Project branch rate' ('**{0:N2}%**' -f $totals.BranchRate) ('>= {0:N2}%' -f $minBranch) 'fail' +} +elseif ($null -ne $totals.BranchRate) { + Add-SummaryRow 'Project branch rate' ('**{0:N2}%**' -f $totals.BranchRate) ('>= {0:N2}%' -f $minBranch) 'pass' } # 2. Patch coverage (blocking, pull requests only). if ([string]::IsNullOrWhiteSpace($BaseRef)) { Write-Host 'No base ref supplied; skipping the changed-lines (patch) gate.' + Add-SummaryRow 'Patch coverage' 'not a pull request' '-' 'info' } else { $changed = Get-ChangedLines -baseRef $BaseRef -repoRoot $RepoRoot -skipFetch:$SkipFetch.IsPresent @@ -463,14 +572,17 @@ else { if ($uncovered.Count -gt 0) { $uncoveredByFile[$file] = $uncovered } } - $patchFloor = [double]$thresholds.patch.target - [double]$thresholds.patch.threshold + $patchBand = Get-PatchBand -changedLines $coverableChanged -patch $thresholds.patch + $patchFloor = $patchBand.Floor if ($coverableChanged -eq 0) { Write-Host 'No coverable changed lines were found; the patch gate passes vacuously.' + Add-SummaryRow 'Patch coverage' 'no coverable changed lines' '-' 'info' } else { $patchRate = 100.0 * $coveredChanged / $coverableChanged - Write-Host ("Patch: {0:N2}% ({1}/{2} changed lines covered, floor {3:N2}%)" -f ` - $patchRate, $coveredChanged, $coverableChanged, $patchFloor) + Write-Host ("Patch: {0:N2}% ({1}/{2} changed lines covered, floor {3:N2}% for {4}, {5})" -f ` + $patchRate, $coveredChanged, $coverableChanged, $patchFloor, $patchBand.Scope, + $(if ($patchBand.Enforced) { 'enforced' } else { 'advisory' })) if ($uncoveredByFile.Count -gt 0) { Write-Host 'Uncovered changed lines:' @@ -479,9 +591,46 @@ else { } } - if ($patchRate -lt $patchFloor) { - $failures += ('Patch coverage {0:N2}% is below the required {1:N2}% ({2} of {3} changed lines are uncovered).' -f ` - $patchRate, $patchFloor, ($coverableChanged - $coveredChanged), $coverableChanged) + $patchBelowFloor = $patchRate -lt $patchFloor + $patchState = if (-not $patchBelowFloor) { + 'pass' + } + elseif ($patchBand.Enforced) { + 'fail' + } + else { + 'warn' + } + + Add-SummaryRow 'Patch coverage' ` + ('**{0:N2}%** ({1}/{2} changed lines)' -f $patchRate, $coveredChanged, $coverableChanged) ` + ('>= {0:N2}% ({1}{2})' -f $patchFloor, $patchBand.Scope, + $(if ($patchBand.Enforced) { '' } else { ', advisory' })) ` + $patchState + + # List the uncovered changed lines in the summary too - that is the + # actionable part for the author, and it saves opening the raw log. + if ($uncoveredByFile.Count -gt 0) { + $detail = [System.Text.StringBuilder]::new() + $null = $detail.AppendLine('
Uncovered changed lines') + $null = $detail.AppendLine('') + foreach ($file in $uncoveredByFile.Keys) { + $null = $detail.AppendLine(('- `{0}`: {1}' -f $file, ($uncoveredByFile[$file] -join ', '))) + } + $null = $detail.AppendLine('') + $null = $detail.Append('
') + Add-SummaryNote $detail.ToString() + } + + if ($patchBelowFloor) { + $message = ('Patch coverage {0:N2}% is below {1:N2}% for {2} ({3} of {4} changed lines are uncovered).' -f ` + $patchRate, $patchFloor, $patchBand.Scope, ($coverableChanged - $coveredChanged), $coverableChanged) + if ($patchBand.Enforced) { + $failures += $message + } + else { + Write-GateWarning ($message + ' Advisory at this patch size - add a test if the change deserves one.') + } } } } @@ -492,17 +641,68 @@ $tolerance = [double]$thresholds.project.advisoryDeltaTolerance if ($null -ne $totals.LineRate -and $baseline -gt 0) { $delta = $totals.LineRate - $baseline Write-Host ("Baseline: {0:N2}% recorded, delta {1:+0.00;-0.00;0.00} percentage points" -f $baseline, $delta) + $deltaText = '{0:+0.00;-0.00;0.00} pp' -f $delta if ($delta -lt (-1 * $tolerance)) { Write-GateWarning ('Total line coverage dropped {0:N2} percentage points below the recorded master baseline of {1:N2}%. This is advisory and does not fail the build.' -f ` [Math]::Abs($delta), $baseline) + Add-SummaryRow 'Baseline delta (advisory)' $deltaText ('{0:N2}% recorded' -f $baseline) 'warn' } - elseif ($delta -gt $tolerance) { - Write-Host 'Coverage is above the recorded baseline; consider ratcheting coverage-thresholds.json.' + else { + if ($delta -gt $tolerance) { + Write-Host 'Coverage is above the recorded baseline; consider ratcheting coverage-thresholds.json.' + Add-SummaryNote 'Coverage is above the recorded baseline - consider ratcheting `coverage-thresholds.json`.' + } + Add-SummaryRow 'Baseline delta (advisory)' $deltaText ('{0:N2}% recorded' -f $baseline) 'info' } } if ($failures.Count -gt 0) { foreach ($failure in $failures) { Write-GateError $failure } +} + +if (-not [string]::IsNullOrWhiteSpace($SummaryPath)) { + $verdict = if ($failures.Count -gt 0) { + ':x: **Coverage gate failed.** This check is advisory and does not block the merge.' + } + else { + ':white_check_mark: **Coverage gate passed.**' + } + + $summary = [System.Text.StringBuilder]::new() + $null = $summary.AppendLine('## Code coverage') + $null = $summary.AppendLine('') + $null = $summary.AppendLine($verdict) + $null = $summary.AppendLine('') + $null = $summary.AppendLine('| Check | Result | Threshold |') + $null = $summary.AppendLine('| --- | --- | --- |') + foreach ($row in $script:SummaryRows) { + $null = $summary.AppendLine($row) + } + if ($failures.Count -gt 0) { + $null = $summary.AppendLine('') + foreach ($failure in $failures) { + $null = $summary.AppendLine(('- :x: {0}' -f $failure)) + } + } + foreach ($note in $script:SummaryNotes) { + $null = $summary.AppendLine('') + $null = $summary.AppendLine($note) + } + $null = $summary.AppendLine('') + $null = $summary.AppendLine(('Thresholds live in `coverage-thresholds.json`. Whole report before exclusions: line {0:N2}%, branch {1:N2}%.' -f ` + $report.ReportLineRate, $report.ReportBranchRate)) + + $summaryDir = Split-Path -Parent $SummaryPath + if (-not [string]::IsNullOrWhiteSpace($summaryDir) -and -not (Test-Path $summaryDir)) { + $null = New-Item -ItemType Directory -Force -Path $summaryDir + } + # UTF8 without BOM: GitHub renders a leading BOM as a literal character at + # the top of the step summary. + [System.IO.File]::WriteAllText($SummaryPath, $summary.ToString(), [System.Text.UTF8Encoding]::new($false)) + Write-Host "Wrote the markdown summary to $SummaryPath." +} + +if ($failures.Count -gt 0) { Write-Host 'Coverage gate FAILED.' exit 1 } diff --git a/.azurepipelines/coverage.yml b/.azurepipelines/coverage.yml new file mode 100644 index 0000000000..e1a108b736 --- /dev/null +++ b/.azurepipelines/coverage.yml @@ -0,0 +1,259 @@ +# +# Advisory code coverage check. +# +# Collects the raw Cobertura fragments every test job of the run published, +# merges them once with ReportGenerator and evaluates coverage-thresholds.json +# against the merged report. +# +# This check is ADVISORY: it reports a clean failure when the thresholds are +# missed so the miss is visible, but it must NOT be added to the GitHub branch +# ruleset. The required check is the 'Tests passed' stage in azure-pipelines.yml. +# +# It deliberately does not re-run any tests. The previous design re-ran the +# whole suite in a single job with the collector attached, which serialised a +# suite that is deliberately fanned out across matrix jobs and always exceeded +# the stage timeout. +# +parameters: +# Stages to depend on. Their results decide whether missing coverage is +# expected (nothing ran) or a real problem (a stage ran but published nothing). +- name: stages + type: object + default: [] +- name: stagename + type: string + default: 'coverage' +- name: poolName + type: string + default: '' +# The check only merges XML and diffs git, so it is independent of any framework +# under test and always runs on Linux. +- name: poolImage + type: string + default: 'ubuntu-24.04' +# Publish the merged report to the build's Code Coverage tab and to Codacy. +- name: publishCoverage + type: boolean + default: true +# Upload the merged report to codecov.io. Reporting only - the enforced rules +# live in coverage-thresholds.json and are applied by the gate below. Set to +# false to turn the upload off entirely; it is skipped anyway when the +# CODECOV_TOKEN secret variable is not set, which is the case on fork pull +# requests. +- name: enableCodecov + type: boolean + default: true +stages: +- stage: ${{ parameters.stagename }} + displayName: 'Code coverage' + dependsOn: ${{ parameters.stages }} + # Evaluate whatever coverage the run did produce even when a test stage + # failed: a partial report is still worth publishing, and the 'Tests passed' + # stage already carries the failure signal. + condition: not(canceled()) + jobs: + - job: coverage + displayName: Merge and evaluate + timeoutInMinutes: 30 + variables: + DOTNET_CLI_TELEMETRY_OPTOUT: true + DOTNET_SKIP_FIRST_TIME_EXPERIENCE: true + disable.coverage.autogenerate: true + pool: + ${{ if ne(parameters.poolName, '') }}: + name: ${{ parameters.poolName }} + demands: + - ImageOverride -equals ${{ parameters.poolImage }} + ${{ else }}: + vmImage: ${{ parameters.poolImage }} + steps: + # Full history: the changed-lines (patch) check diffs the pull request + # against its base branch, which a shallow clone cannot do. + - checkout: self + fetchDepth: 0 + - task: UseDotNet@2 + displayName: 'Install .NET 10.0' + inputs: + packageType: 'sdk' + version: '10.0.x' + - task: DownloadPipelineArtifact@2 + displayName: 'Download code coverage' + inputs: + buildType: current + # Every test job of every stage publishes 'coverage--'. + patterns: 'coverage-*/**/*.cobertura.xml' + path: '$(Agent.TempDirectory)/coverage' + continueOnError: true + - task: PowerShell@2 + name: reportProbe + displayName: 'Detect code coverage' + inputs: + pwsh: true + targetType: inline + script: | + $source = '$(Agent.TempDirectory)/coverage' + $artifacts = @(Get-ChildItem -Path $source -Directory -ErrorAction SilentlyContinue) + $reports = @(Get-ChildItem -Path $source -Recurse -File -Filter *.cobertura.xml -ErrorAction SilentlyContinue) + Write-Host "Downloaded $($reports.Count) Cobertura report(s) from $($artifacts.Count) test job(s)." + $found = $reports.Count -gt 0 + Write-Host "##vso[task.setvariable variable=HAS_REPORTS;isOutput=true]$($found.ToString().ToLowerInvariant())" + if ($found) { + exit 0 + } + + # Nothing to evaluate. That is expected when every test stage was + # skipped or failed before publishing anything, and it is the 'Tests + # passed' stage that owns the failure signal for those, so warn rather + # than manufacture a red coverage check with no data behind it. + # + # A broken collector on an otherwise green run is caught on the + # GitHub Actions side instead, where the test matrix result is + # available cheaply; the equivalent here would need the stage + # dependency context, which is too large to serialise in an + # expression (see the 'Tests passed' stage in azure-pipelines.yml). + Write-Host "##vso[task.logissue type=warning]No coverage was published by any test job; there is nothing to evaluate." + exit 0 + - task: DotNetCoreCLI@2 + displayName: Install ReportGenerator tool + condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) + inputs: + command: custom + custom: tool + arguments: install --tool-path tools dotnet-reportgenerator-globaltool + - task: PowerShell@2 + displayName: 'Merge Code Coverage Report' + condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) + inputs: + pwsh: true + targetType: inline + workingDirectory: '$(Build.SourcesDirectory)' + script: | + $ErrorActionPreference = 'Stop' + $source = '$(Agent.TempDirectory)/coverage' + & '$(Build.SourcesDirectory)/tools/reportgenerator' ` + "-reports:$source/**/*.cobertura.xml" ` + '-targetdir:$(Build.SourcesDirectory)/CodeCoverage' ` + '-reporttypes:Cobertura;HtmlSummary' ` + '-title:UA .Net Standard Test Coverage' ` + '-assemblyfilters:-*.Tests' + if ($LASTEXITCODE -ne 0) { + Write-Host "##vso[task.logissue type=error]ReportGenerator failed with exit code $LASTEXITCODE." + exit $LASTEXITCODE + } + $summary = '$(Build.SourcesDirectory)/CodeCoverage/summary.htm' + if (Test-Path $summary) { + Move-Item -Force $summary '$(Build.SourcesDirectory)/CodeCoverage/index.htm' + } + - task: PublishPipelineArtifact@1 + displayName: 'Publish merged coverage report' + condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) + continueOnError: true + inputs: + targetPath: '$(Build.SourcesDirectory)/CodeCoverage' + artifact: 'coverage-report' + - ${{ if parameters.publishCoverage }}: + - task: PublishCodeCoverageResults@2 + displayName: 'Publish code coverage' + condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) + continueOnError: true + inputs: + summaryFileLocation: '$(Build.SourcesDirectory)/CodeCoverage/Cobertura.xml' + - script: | + bash <(curl -Ls https://coverage.codacy.com/get.sh) report -r $REPORT_LOCATION --commit-uuid $COMMIT_UUID + + condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true'), ne(variables['CODACY_PROJECT_TOKEN'], '')) + env: + CODACY_PROJECT_TOKEN: $(CODACY_PROJECT_TOKEN) + REPORT_LOCATION: '$(Build.SourcesDirectory)/CodeCoverage/Cobertura.xml' + COMMIT_UUID: '$(Build.SourceVersion)' + continueOnError: true + displayName: 'Upload to Codacy' + - ${{ if parameters.enableCodecov }}: + - script: | + set -euo pipefail + # The bash uploader (codecov.io/bash) was sunset; this is the current + # Codecov CLI. Fetched as the platform binary rather than via pip so + # the step does not depend on a Python toolchain being present. + curl -Os https://cli.codecov.io/latest/linux/codecov + chmod +x codecov + # On a pull request validation build Build.SourceVersion is the + # *merge* commit Azure created, which Codecov cannot match to the pull + # request. Report the head of the source branch instead when there is + # one. + SHA="${PR_SHA:-}" + if [ -z "$SHA" ]; then + SHA="$BUILD_SHA" + fi + # Resolve owner/repo. Build.Repository.Name is empty on this pipeline + # (Azure puts the slug in Build.Repository.ID for a GitHub-backed + # repository), so fall back to parsing the clone URI, which is always + # populated. Passing an empty slug is what made the CLI report + # "Repository not found". + SLUG="${REPO_SLUG:-}" + if [ -z "$SLUG" ]; then + SLUG=$(printf '%s' "$REPO_URI" | sed -E 's#^.*://[^/]+/##; s#\.git$##') + fi + echo "Uploading coverage for slug '$SLUG' at commit '$SHA'." + ./codecov upload-process \ + --disable-search \ + --file "$REPORT_LOCATION" \ + --flag azure \ + --git-service github \ + --slug "$SLUG" \ + --sha "$SHA" + # continueOnError: reporting must never be able to fail the run. The + # verdict comes from the gate below, not from codecov.io being + # reachable. + continueOnError: true + condition: >- + and(succeeded(), + eq(variables['reportProbe.HAS_REPORTS'], 'true'), + ne(variables['CODECOV_TOKEN'], '')) + env: + CODECOV_TOKEN: $(CODECOV_TOKEN) + REPORT_LOCATION: '$(Build.SourcesDirectory)/CodeCoverage/Cobertura.xml' + BUILD_SHA: '$(Build.SourceVersion)' + PR_SHA: '$(System.PullRequest.SourceCommitId)' + # The CLI cannot infer owner/repo from an Azure DevOps pipeline that + # builds a GitHub repository - it looks for an Azure Repos project and + # reports "Repository not found". + REPO_SLUG: '$(Build.Repository.ID)' + REPO_URI: '$(Build.Repository.Uri)' + displayName: 'Upload to codecov.io' + # Runs last so the report is published and downloadable even when the + # thresholds are missed. + - task: PowerShell@2 + displayName: 'Coverage gate' + condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) + inputs: + pwsh: true + targetType: inline + workingDirectory: '$(Build.SourcesDirectory)' + script: | + # Empty on anything that is not a pull request validation build, in + # which case check-coverage.ps1 checks the project floor only. + $baseRef = $env:SYSTEM_PULLREQUEST_TARGETBRANCH + if ([string]::IsNullOrWhiteSpace($baseRef)) { + Write-Host 'Not a pull request build; the changed-lines check is skipped.' + } + else { + Write-Host "Pull request targets '$baseRef'." + } + + $summaryPath = '$(Agent.TempDirectory)/coverage-summary.md' + & ./.azurepipelines/check-coverage.ps1 ` + -CoberturaPath '$(Build.SourcesDirectory)/CodeCoverage/Cobertura.xml' ` + -ThresholdsPath '$(Build.SourcesDirectory)/coverage-thresholds.json' ` + -RepoRoot '$(Build.SourcesDirectory)' ` + -BaseRef $baseRef ` + -SummaryPath $summaryPath + $gate = $LASTEXITCODE + + # Attach the markdown to the build summary so the numbers are visible + # without opening this log. + if (Test-Path $summaryPath) { + Write-Host "##vso[task.uploadsummary]$summaryPath" + } + + exit $gate + diff --git a/.azurepipelines/get-matrix.ps1 b/.azurepipelines/get-matrix.ps1 index 31462297ff..24f2f0cfd3 100644 --- a/.azurepipelines/get-matrix.ps1 +++ b/.azurepipelines/get-matrix.ps1 @@ -48,6 +48,12 @@ the matrix is fanned out across TFMs; each entry carries a 'targetTfm' variable that downstream jobs typically pass to 'dotnet build' as '/p:CustomTestTarget=$(targetTfm)'. + + .PARAMETER AllowEmpty + Do not fail when discovery produces no matrix entries. Off by default, + because an empty matrix skips the downstream job, which rolls up as a + successful stage and would let the 'Tests passed' gate approve a run that + executed no tests. #> Param( @@ -58,7 +64,8 @@ Param( [hashtable] $AgentTable = $null, [string] $Configurations = '', [string] $Files = '', - [string] $Tfms = '' + [string] $Tfms = '', + [switch] $AllowEmpty ) if ([string]::IsNullOrEmpty($BuildRoot)) { @@ -204,5 +211,15 @@ foreach ($item in $items) { } Write-Host ("Job matrix:`n" + ($jobMatrix | ConvertTo-Json -Depth 4)) + +# An empty matrix silently produces a stage whose only real job is skipped, and +# a skipped job rolls up as a *successful* stage - so a broken discovery pattern +# would sail through the 'Tests passed' gate having run nothing at all. Fail +# loudly instead; no caller legitimately expects zero matches. +if ($jobMatrix.Count -eq 0 -and -not $AllowEmpty) { + Write-Host "##vso[task.logissue type=error]The job matrix is empty - no file matched '$FileName' beneath '$BuildRoot'. Pass -AllowEmpty if that is genuinely expected." + exit 1 +} + Write-Host ("##vso[task.setVariable variable=jobMatrix;isOutput=true] {0}" ` -f ($jobMatrix | ConvertTo-Json -Compress)) diff --git a/.azurepipelines/test.yml b/.azurepipelines/test.yml index b2d3b8926c..561a9d467a 100644 --- a/.azurepipelines/test.yml +++ b/.azurepipelines/test.yml @@ -53,51 +53,28 @@ parameters: - name: windowsImage type: string default: 'windows-2025-vs2026' -# Optional job-level condition forwarded to the testprep…, testall… and -# coverage… jobs. +# Optional job-level condition forwarded to the testprep… and testall… jobs. # When unset (the overwhelming majority of callers) no `condition:` line is -# emitted - preserves today's "always run" behaviour exactly. When set, all +# emitted - preserves today's "always run" behaviour exactly. When set, both # jobs receive the same condition so the entire test invocation is gated # together (used by the root pipeline to skip macOS test invocations on draft # GitHub PRs). Pass the condition *bare*, without a succeeded() wrapper: the -# test jobs wrap it in succeeded() so a build cancellation still propagates to -# them - without the wrapper an Azure Pipelines `condition:` re-evaluates to -# true during cancel and the job keeps running (see "Job will continue running -# after cancellation was requested because its condition re-evaluated to true" -# warnings) - while the coverage gate wraps it in not(canceled()) because it has -# to report a failed status check when the tests failed. +# jobs wrap it in succeeded() so a build cancellation still propagates to them - +# without the wrapper an Azure Pipelines `condition:` re-evaluates to true during +# cancel and the job keeps running (see "Job will continue running after +# cancellation was requested because its condition re-evaluated to true" +# warnings). - name: condition type: string default: '' -# Collect code coverage in every test job of this invocation and append a -# 'Coverage ...' job that merges the per-project Cobertura reports published by -# those jobs and runs check-coverage.ps1 against the merged result. The gate -# job is the single per-(framework, configuration) signal used as the required -# GitHub status check: it fails when any test job failed, and when the merged -# coverage misses the thresholds, so the tests never have to be re-run. +# Collect code coverage in every test job of this invocation and publish each +# matrix entry's raw Cobertura fragment as a pipeline artifact. The 'Code +# coverage' stage (.azurepipelines/coverage.yml) collects every fragment the run +# produced, merges them once and evaluates the thresholds - this template only +# produces the inputs, it does not gate anything. - name: coverage type: boolean default: true -# Blocking (true) vs advisory (false) coverage thresholds. Test failures always -# block regardless of this switch. -- name: enforceCoverage - type: boolean - default: false -# Publish the merged report to the build's Code Coverage tab and to Codacy. -# Exactly one invocation per pipeline run may set this - Azure DevOps keeps a -# single code-coverage summary per build, so a second publish overwrites the -# first. -- name: publishCoverage - type: boolean - default: false -# Pool for the coverage gate job. The gate only merges XML and diffs git, so it -# is deliberately decoupled from the framework under test and runs on Linux. -- name: coveragePoolName - type: string - default: '' -- name: coverageImage - type: string - default: 'ubuntu-24.04' jobs: - job: testprep${{ parameters.jobnamesuffix }} displayName: Prepare Test Jobs ${{ parameters.configuration }} (${{ parameters.framework }}) @@ -348,12 +325,12 @@ jobs: } Write-Host "No test failures recorded across $total test(s); treating the run as successful (tolerating any non-zero test-host exit after a fully green run)." exit 0 - # Hand the raw per-project Cobertura report to the Coverage gate job below - # instead of letting that job re-run the whole suite with coverage enabled. + # Hand the raw per-project Cobertura report to the 'Code coverage' stage + # instead of letting a gate job re-run the whole suite with coverage enabled. # Re-running defeats the point of fanning the suite out across matrix jobs and # reliably blew the stage timeout. The artifact name is unique per matrix - # entry (System.JobId) and prefixed with this invocation's job-name suffix so - # the gate job downloads only the reports for its own framework/configuration. + # entry (System.JobId); the coverage stage collects 'coverage-*' across every + # stage of the run and merges them once. - ${{ if and(parameters.coverage, not(startsWith(parameters.framework, 'net4')), not(startsWith(parameters.customtestarget, 'net4'))) }}: - task: CopyFiles@2 displayName: 'Stage code coverage' @@ -446,209 +423,3 @@ jobs: inputs: targetPath: '$(Build.ArtifactStagingDirectory)/dumps' artifact: 'dumps-$(System.JobId)' -# Per-(framework, configuration) gate. Runs once every test job of this -# invocation has finished, re-assembles the Cobertura fragments those jobs -# published into a single report and evaluates the coverage thresholds on it. -# This job - not the fan-out matrix - is the status check to require in the -# GitHub branch ruleset: it fails when any test job failed and, on the leg that -# sets enforceCoverage, when the merged report is missing or misses the -# thresholds in coverage-thresholds.json. -- ${{ if parameters.coverage }}: - - job: coverage${{ parameters.jobnamesuffix }} - displayName: Coverage ${{ parameters.configuration }} (${{ parameters.framework }}) - dependsOn: - - testprep${{ parameters.jobnamesuffix }} - - testall${{ parameters.jobnamesuffix }} - timeoutInMinutes: 30 - # not(canceled()) rather than the default succeeded(): the job has to report - # a *failed* status check when the tests failed, and a job that is skipped - # because its dependency failed surfaces to GitHub as 'skipped', which a - # required check treats as satisfied. - ${{ if ne(parameters.condition, '') }}: - condition: and(not(canceled()), ${{ parameters.condition }}) - ${{ else }}: - condition: not(canceled()) - variables: - DOTNET_CLI_TELEMETRY_OPTOUT: true - DOTNET_SKIP_FIRST_TIME_EXPERIENCE: true - disable.coverage.autogenerate: true - testsResult: $[ dependencies.testall${{ parameters.jobnamesuffix }}.result ] - testMatrix: $[ dependencies.testprep${{ parameters.jobnamesuffix }}.outputs['testmatrix.jobMatrix'] ] - # The gate merges XML and diffs git, so it is independent of the framework - # under test and always runs on Linux. - pool: - ${{ if ne(parameters.coveragePoolName, '') }}: - name: ${{ parameters.coveragePoolName }} - demands: - - ImageOverride -equals ${{ parameters.coverageImage }} - ${{ else }}: - vmImage: ${{ parameters.coverageImage }} - steps: - # Full history: the coverage gate diffs the pull request against its base branch - # to compute changed-line (patch) coverage, which a shallow clone cannot do. - - checkout: self - fetchDepth: 0 - - task: PowerShell@2 - displayName: 'Verify test results ${{ parameters.configuration }} (${{ parameters.framework }})' - inputs: - pwsh: true - targetType: inline - script: | - # Aggregated result of every matrix entry of the test job. Anything - # other than a (possibly warning-decorated) success means at least one - # test job failed, was cancelled or never ran - fail here so this - # single check reflects the health of the whole framework leg. - $result = '$(testsResult)' - Write-Host "Test jobs for ${{ parameters.framework }} ${{ parameters.configuration }} reported '$result'." - if ($result -eq 'Succeeded' -or $result -eq 'SucceededWithIssues') { - exit 0 - } - Write-Host "##vso[task.logissue type=error]Tests for ${{ parameters.framework }} ${{ parameters.configuration }} did not succeed (result: $result)." - exit 1 - - task: UseDotNet@2 - displayName: 'Install .NET 10.0' - inputs: - packageType: 'sdk' - version: '10.0.x' - - task: DownloadPipelineArtifact@2 - displayName: 'Download code coverage' - inputs: - buildType: current - # One artifact per matrix entry; the '-' after the suffix keeps sibling - # invocations apart (for example net100 does not pick up net100mac). - patterns: 'coverage-${{ parameters.jobnamesuffix }}-*/**/*.cobertura.xml' - path: '$(Agent.TempDirectory)/coverage' - - task: PowerShell@2 - name: reportProbe - displayName: 'Detect code coverage' - inputs: - pwsh: true - targetType: inline - script: | - $source = '$(Agent.TempDirectory)/coverage' - $artifacts = @(Get-ChildItem -Path $source -Directory -ErrorAction SilentlyContinue) - $reports = @(Get-ChildItem -Path $source -Recurse -File -Filter *.cobertura.xml -ErrorAction SilentlyContinue) - Write-Host "Downloaded $($reports.Count) Cobertura report(s) from $($artifacts.Count) test job(s)." - $found = $reports.Count -gt 0 - Write-Host "##vso[task.setvariable variable=HAS_REPORTS;isOutput=true]$($found.ToString().ToLowerInvariant())" - if ($found) { - # One artifact per matrix entry. Fewer means a test job passed but - # published nothing, which silently shrinks the merged report - the - # project floor only gets stricter, but the changed-lines gate can - # pass vacuously for files whose assembly went missing. Surface it. - $expected = 0 - try { - $expected = @(($env:TEST_MATRIX | ConvertFrom-Json).PSObject.Properties).Count - } - catch { - Write-Host "Could not read the test job matrix: $($_.Exception.Message)" - } - if ($expected -gt 0 -and $artifacts.Count -lt $expected) { - Write-Host "##vso[task.logissue type=warning]Only $($artifacts.Count) of $expected test job(s) published coverage; the merged report is incomplete." - } - exit 0 - } - # Expected on the .NET Framework legs: coverlet.collector 10.x has no - # net4x build asset, so those jobs never emit a report and this gate - # degrades to the test-result check. Where the thresholds are enforced - # a missing report is a hard failure instead - it would otherwise let - # a silently broken collector pass the gate. - if ('${{ parameters.enforceCoverage }}' -eq 'true') { - Write-Host "##vso[task.logissue type=error]No coverage was published by the test jobs; the gate has nothing to evaluate." - exit 1 - } - Write-Host "##vso[task.logissue type=warning]No coverage was published by the test jobs; only the test results are checked for this leg." - exit 0 - env: - TEST_MATRIX: $(testMatrix) - - task: DotNetCoreCLI@2 - displayName: Install ReportGenerator tool - condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) - inputs: - command: custom - custom: tool - arguments: install --tool-path tools dotnet-reportgenerator-globaltool - - task: PowerShell@2 - displayName: 'Merge Code Coverage Report' - condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) - inputs: - pwsh: true - targetType: inline - workingDirectory: '$(Build.SourcesDirectory)' - script: | - $ErrorActionPreference = 'Stop' - $source = '$(Agent.TempDirectory)/coverage' - & '$(Build.SourcesDirectory)/tools/reportgenerator' ` - "-reports:$source/**/*.cobertura.xml" ` - '-targetdir:$(Build.SourcesDirectory)/CodeCoverage' ` - '-reporttypes:Cobertura;HtmlSummary' ` - '-title:UA .Net Standard Test Coverage' ` - '-assemblyfilters:-*.Tests' - if ($LASTEXITCODE -ne 0) { - Write-Host "##vso[task.logissue type=error]ReportGenerator failed with exit code $LASTEXITCODE." - exit $LASTEXITCODE - } - $summary = '$(Build.SourcesDirectory)/CodeCoverage/summary.htm' - if (Test-Path $summary) { - Move-Item -Force $summary '$(Build.SourcesDirectory)/CodeCoverage/index.htm' - } - - task: PublishPipelineArtifact@1 - displayName: 'Publish merged coverage report' - condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) - continueOnError: true - inputs: - targetPath: '$(Build.SourcesDirectory)/CodeCoverage' - artifact: 'coverage-report-${{ parameters.jobnamesuffix }}' - # Azure DevOps keeps a single code-coverage summary per build and Codacy - # accepts a single report per commit, so only the invocation flagged as the - # publishing one uploads - the remaining legs keep their report as the - # pipeline artifact published above. - - ${{ if parameters.publishCoverage }}: - - task: PublishCodeCoverageResults@2 - displayName: 'Publish code coverage' - condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) - inputs: - summaryFileLocation: '$(Build.SourcesDirectory)/CodeCoverage/Cobertura.xml' - continueOnError: true - - script: | - bash <(curl -Ls https://coverage.codacy.com/get.sh) report -r $REPORT_LOCATION --commit-uuid $COMMIT_UUID - - condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true'), ne(variables['CODACY_PROJECT_TOKEN'], '')) - env: - CODACY_PROJECT_TOKEN: $(CODACY_PROJECT_TOKEN) - REPORT_LOCATION: '$(Build.SourcesDirectory)/CodeCoverage/Cobertura.xml' - COMMIT_UUID: '$(Build.SourceVersion)' - continueOnError: true - displayName: 'Upload to Codacy' - # Coverage gate. Replaces the codecov/project and codecov/patch status checks: - # a blocking absolute floor over the whole report, a blocking changed-lines - # check on pull requests, and an advisory comparison against the recorded - # baseline. Runs last so the report is published even when the gate fails. - - task: PowerShell@2 - displayName: 'Coverage gate' - condition: and(succeeded(), eq(variables['reportProbe.HAS_REPORTS'], 'true')) - # Advisory unless the caller opted in: only the leg whose thresholds are - # calibrated blocks the build; the others still fail on test failures. - ${{ if not(parameters.enforceCoverage) }}: - continueOnError: true - inputs: - pwsh: true - targetType: inline - workingDirectory: '$(Build.SourcesDirectory)' - script: | - # Empty on anything that is not a pull request validation build, in which - # case check-coverage.ps1 enforces the project floor only. - $baseRef = $env:SYSTEM_PULLREQUEST_TARGETBRANCH - if ([string]::IsNullOrWhiteSpace($baseRef)) { - Write-Host 'Not a pull request build; the changed-lines gate is skipped.' - } - else { - Write-Host "Pull request targets '$baseRef'." - } - - & ./.azurepipelines/check-coverage.ps1 ` - -CoberturaPath '$(Build.SourcesDirectory)/CodeCoverage/Cobertura.xml' ` - -ThresholdsPath '$(Build.SourcesDirectory)/coverage-thresholds.json' ` - -RepoRoot '$(Build.SourcesDirectory)' ` - -BaseRef $baseRef - exit $LASTEXITCODE \ No newline at end of file diff --git a/.github/workflows/buildandtest.yml b/.github/workflows/buildandtest.yml index 3bf6d6f975..e7ebb5f244 100644 --- a/.github/workflows/buildandtest.yml +++ b/.github/workflows/buildandtest.yml @@ -31,11 +31,13 @@ permissions: # switch - the other half is the ciBuildBackend parameter in # azure-pipelines.yml. BOTH MUST BE FLIPPED TOGETHER. # -# ado - (default) Azure Pipelines owns the work on master/main: the jobs -# below stand down there and run only on the branches Azure -# Pipelines does not cover (master378, develop/*). -# actions - restores the previous behaviour: every job below runs here on -# GitHub-hosted runners for every triggering branch. +# ado - Azure Pipelines owns the work on master/main: the jobs below stand +# down there and run only on the branches Azure Pipelines does not +# cover (master378, develop/*). +# actions - (default) the work is split with Azure Pipelines: every job below +# runs here on GitHub-hosted runners for every triggering branch, +# while Azure Pipelines keeps the fast PR test legs and the coverage +# gate. # # The macOS legs are deliberately exempt from this switch: Managed DevOps Pools # provide no macOS image, so macOS coverage has to stay on GitHub-hosted @@ -45,7 +47,14 @@ permissions: # conditions, so the switch is resolved once in the `discover` job below and # re-exported as job outputs. env: - CI_BUILD_BACKEND: ado + CI_BUILD_BACKEND: actions + # Upload the merged coverage report to codecov.io. Reporting only - the + # enforced rules live in coverage-thresholds.json and are applied by + # check-coverage.ps1 in the `code-coverage` job below, so a codecov outage can + # never fail a build. Set to 'false' to turn the upload off; it is skipped + # anyway when the CODECOV_TOKEN secret is unavailable, which is the case for + # pull requests from forks. + ENABLE_CODECOV: 'true' concurrency: group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} @@ -165,6 +174,14 @@ jobs: needs: discover name: test-${{matrix.os}}-${{matrix.csproj}} runs-on: ${{ matrix.os }} + # Bound the job well below the 360-minute default. These entries normally + # take 3-15 minutes, so anything approaching an hour is a hang, and letting + # it run for six hours blocks the pull-request gate with no diagnostic + # information (observed on run 30988867402, where the Core and Client + # entries sat for ~6 hours). --blame-hang-timeout below turns the common + # case into a named test failure long before this fires; this is the + # backstop for a hang outside the test host itself. + timeout-minutes: 60 # test_os is empty when Azure Pipelines owns the Linux matrix and macOS is # not included for this event; skip rather than fan out to nothing. if: ${{ needs.discover.outputs.test_os != '[]' }} @@ -233,6 +250,17 @@ jobs: '--collect:XPlat Code Coverage', '--settings', './tests/coverlet.runsettings.xml', + # Kill (and name) any single test that exceeds 10 minutes, so a silent + # hang surfaces as "Test exceeded the configured timeout" + # instead of burning the whole job timeout with nothing to go on. The + # LongRunning and Stress tiers do not run here, so 10 minutes is well + # above any legitimate test. This mirrors what the Azure Pipelines + # test template already does; the mini dump captures every thread's + # stack, which is what a teardown/shutdown hang needs to diagnose. + '--blame-hang-timeout', + '10m', + '--blame-hang-dump-type', + 'mini', '--results-directory', '${{ env.TESTRESULTS }}') @@ -251,6 +279,188 @@ jobs: # Use always() to always run this step to publish test results when there are test failures if: ${{ always() }} + # Advisory coverage check. Mirrors the Azure Pipelines 'Code coverage' stage: + # it merges the Cobertura fragments the matrix above already collects, runs the + # same check-coverage.ps1 against the same coverage-thresholds.json, and + # surfaces the numbers in the run summary and on the pull request. + # + # It reports a clean failure when the thresholds are missed so the miss is + # visible, but it is deliberately NOT in `build-and-test summary`'s `needs`, + # so it can never block the required check. Do not add it to the branch + # ruleset. + code-coverage: + name: code coverage + needs: [discover, build-and-test] + runs-on: ubuntu-latest + # always(): the coverage of a run whose tests failed is still worth + # reporting. Gated on the same test_os condition build-and-test itself uses, + # rather than on gh_owns_build - a push or scheduled run under the 'ado' + # backend still executes the macOS legs here, and their coverage would + # otherwise be uploaded and then discarded. + if: ${{ always() && needs.discover.outputs.test_os != '[]' && needs.discover.outputs.relevant_changes == 'true' }} + permissions: + contents: read + # Required to upsert the sticky coverage comment. Note that on a + # pull_request from a fork the token is read-only regardless of what this + # block says, which the comment step below handles. + pull-requests: write + env: + # The `secrets` context is not available to a step's `if`, so the presence + # of the token is resolved once here (job-level `env` can read `secrets`) + # and tested as a plain string below. Fork pull requests get no secrets, + # so this is 'false' there and the upload is skipped rather than failing. + HAS_CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN != '' }} + steps: + - uses: actions/checkout@v7 + with: + # Full history: the changed-lines (patch) gate diffs against the base + # branch, which a shallow clone cannot do. + fetch-depth: 0 + + - name: Setup .NET + uses: actions/setup-dotnet@v6 + with: + dotnet-version: '10.0.x' + + - name: Download test results + uses: actions/download-artifact@v8 + with: + pattern: dotnet-results-* + path: coverage-artifacts + + - name: Detect coverage + id: probe + shell: pwsh + env: + TESTS_RESULT: ${{ needs.build-and-test.result }} + run: | + $reports = @(Get-ChildItem -Path coverage-artifacts -Recurse -File -Filter *.cobertura.xml -ErrorAction SilentlyContinue) + Write-Host "Found $($reports.Count) Cobertura report(s)." + "has_reports=$(($reports.Count -gt 0).ToString().ToLowerInvariant())" | Out-File -FilePath $env:GITHUB_OUTPUT -Append + if ($reports.Count -gt 0) { + exit 0 + } + + # Nothing to evaluate. Whether that is expected depends on whether the + # test matrix actually ran: if it was skipped there is genuinely no + # coverage to collect, but a matrix that completed and published nothing + # means the collector is broken, and silently reporting green would hide + # that indefinitely. + if ($env:TESTS_RESULT -eq 'success') { + Write-Host '::error title=Code coverage::The test matrix completed but published no coverage at all; the collector is not working.' + exit 1 + } + + Write-Host "::warning title=Code coverage::The test matrix did not complete (result: $($env:TESTS_RESULT)), so there is no coverage to evaluate." + + - name: Merge coverage report + if: ${{ steps.probe.outputs.has_reports == 'true' }} + shell: pwsh + run: | + $ErrorActionPreference = 'Stop' + dotnet tool install --tool-path tools dotnet-reportgenerator-globaltool + ./tools/reportgenerator ` + '-reports:coverage-artifacts/**/*.cobertura.xml' ` + '-targetdir:CodeCoverage' ` + '-reporttypes:Cobertura;HtmlSummary' ` + '-title:UA .Net Standard Test Coverage' ` + '-assemblyfilters:-*.Tests' + if ($LASTEXITCODE -ne 0) { + Write-Host "::error title=Code coverage::ReportGenerator failed with exit code $LASTEXITCODE." + exit $LASTEXITCODE + } + + - name: Upload merged coverage report + if: ${{ steps.probe.outputs.has_reports == 'true' }} + uses: actions/upload-artifact@v7 + with: + name: coverage-report + path: CodeCoverage + + # Reporting only, and deliberately continue-on-error: the verdict comes from + # the gate below, never from codecov.io being reachable. Skipped when the + # secret is unavailable, which is how fork pull requests behave. + - name: Upload to codecov.io + if: ${{ steps.probe.outputs.has_reports == 'true' && env.ENABLE_CODECOV == 'true' && env.HAS_CODECOV_TOKEN == 'true' }} + continue-on-error: true + uses: codecov/codecov-action@v7 + with: + token: ${{ secrets.CODECOV_TOKEN }} + files: CodeCoverage/Cobertura.xml + flags: actions + disable_search: true + fail_ci_if_error: false + + # Writes the markdown summary even when the gate fails, so the numbers reach + # the summary and the pull request either way. The gate's exit code is + # captured here and replayed by the last step. + - name: Evaluate coverage + id: gate + if: ${{ steps.probe.outputs.has_reports == 'true' }} + shell: pwsh + env: + # Empty on push/schedule, in which case check-coverage.ps1 enforces the + # project floor only and skips the changed-lines gate. + BASE_REF: ${{ github.base_ref }} + run: | + ./.azurepipelines/check-coverage.ps1 ` + -CoberturaPath CodeCoverage/Cobertura.xml ` + -ThresholdsPath coverage-thresholds.json ` + -RepoRoot . ` + -BaseRef $env:BASE_REF ` + -SummaryPath coverage-summary.md + "outcome=$LASTEXITCODE" | Out-File -FilePath $env:GITHUB_OUTPUT -Append + + if (Test-Path coverage-summary.md) { + Get-Content coverage-summary.md | Out-File -FilePath $env:GITHUB_STEP_SUMMARY -Append + } + exit 0 + + # Sticky comment: upsert a single comment keyed on an HTML marker rather + # than adding one per run. Uses actions/github-script rather than a + # community action to keep this workflow on first-party actions only. + - name: Comment coverage on the pull request + if: ${{ steps.probe.outputs.has_reports == 'true' && github.event_name == 'pull_request' }} + # A pull_request from a fork gets a read-only GITHUB_TOKEN no matter what + # `permissions:` says, so commenting fails there. The step summary still + # carries the numbers, so a failure here must not fail the job. + continue-on-error: true + uses: actions/github-script@v9 + with: + script: | + const fs = require('fs'); + const marker = ''; + if (!fs.existsSync('coverage-summary.md')) { + core.info('No coverage summary to post.'); + return; + } + const body = marker + '\n' + fs.readFileSync('coverage-summary.md', 'utf8'); + const { owner, repo } = context.repo; + const issue_number = context.issue.number; + + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number, per_page: 100, + }); + const existing = comments.find(c => c.body && c.body.includes(marker)); + + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + core.info(`Updated coverage comment ${existing.id}.`); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number, body }); + core.info('Created the coverage comment.'); + } + + - name: Report coverage gate result + if: ${{ steps.probe.outputs.has_reports == 'true' }} + shell: pwsh + run: | + if ('${{ steps.gate.outputs.outcome }}' -ne '0') { + Write-Host '::error title=Code coverage::Coverage thresholds were not met. This check is advisory and does not block the merge - see the job summary for the numbers.' + exit 1 + } + Write-Host 'Coverage gate passed.' + aot-test: needs: discover name: aot-${{ matrix.os }} diff --git a/azure-pipelines.yml b/azure-pipelines.yml index d4c5d8bd4f..d7a16abc8b 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -16,25 +16,54 @@ parameters: # switch - the other half is CI_BUILD_BACKEND in # .github/workflows/buildandtest.yml. BOTH MUST BE FLIPPED TOGETHER. # -# ado - (default) Azure Pipelines owns the work: the Linux all-TFM build -# leg, the Linux test leg, Native AoT and the coverage gate all run -# on pull requests, and the equivalent GitHub Actions jobs stand -# down. -# actions - restores the previous split: GitHub Actions runs the all-TFM -# builds, the ubuntu test matrix and AoT, and the stages below fall -# back to their Schedule/Manual-only cadence. +# ado - Azure Pipelines owns the work: the Linux all-TFM build leg, the +# Linux test leg and Native AoT all run on pull requests, and the +# equivalent GitHub Actions jobs stand down. +# actions - (default) the work is split across both CI systems: GitHub Actions +# runs the all-TFM builds, the ubuntu test matrix and AoT, and the +# stages below fall back to their Schedule/Manual-only cadence. +# +# Either way the per-(framework, configuration) coverage gate stays here, in the +# always-on 'Fast PR test' stage, because it is the required status check. # # Note on scope: Azure Pipelines evaluates the YAML from the PR's merge ref, so # a PR targeting master378 or a release branch runs *that* branch's copy of this # file. In practice every PR that evaluates this file targets master, which is # why the conditions below only need the parameter and not a target-branch test. - name: ciBuildBackend - displayName: 'CI build backend: ado runs the all-TFM build, cross-platform tests, AoT and the coverage gate here; actions hands them back to GitHub Actions' + displayName: 'CI build backend: actions splits the all-TFM build, cross-platform tests and AoT with GitHub Actions; ado runs all of it here' type: string - default: ado + default: actions values: - ado - actions +# Codecov is reporting only - the enforced coverage rules live in +# coverage-thresholds.json and are applied by check-coverage.ps1 in the +# 'Code coverage' stage. Turn this off to stop uploading entirely; the upload +# is skipped anyway whenever the CODECOV_TOKEN secret variable is unset, which +# is the case for pull requests from forks. +- name: enableCodecov + displayName: 'Upload the merged coverage report to codecov.io (reporting only, never gates)' + type: boolean + default: true +# Every stage that runs tests, consumed by the two terminal stages at the bottom +# of this file. It is a parameter purely so the list is written once: Azure +# Pipelines supports no YAML anchors, and a second hand-maintained copy would +# drift. There is no reason to override it at queue time. +- name: testStages + displayName: 'Test stages rolled up by the Tests passed and Code coverage stages (do not change)' + type: object + default: + - build + - testfastpr + - testaot + - testdebug + - testnet90 + - testnet80 + - testnet472 + - testnetstandard20 + - testnetstandard21 + - testnightly trigger: batch: 'true' @@ -81,9 +110,9 @@ variables: FullBuild: ${{ ne(variables['Build.Reason'], 'PullRequest') }} ScheduledBuild: ${{ in(variables['Build.Reason'], 'Schedule', 'Manual') }} # 'True' when Azure Pipelines owns the build/test matrix that GitHub Actions - # used to run (see the ciBuildBackend parameter). Native AoT, previously - # Schedule/Manual-only, additionally runs on pull requests while this is True, - # because it is then the only place that work happens. + # otherwise runs (see the ciBuildBackend parameter). Native AoT, normally + # Schedule/Manual-only here, additionally runs on pull requests while this is + # True, because it is then the only place that work happens. AdoOwnsMatrix: ${{ eq(parameters.ciBuildBackend, 'ado') }} # macOS jobs always run on Microsoft-hosted agents because Managed DevOps Pools # does not provide macOS images. @@ -139,11 +168,11 @@ stages: ${{ if ne(variables.FullBuild, 'False') }}: tfms: 'net472,net48,netstandard2.0,netstandard2.1,net8.0,net9.0,net10.0' ${{ elseif eq(parameters.ciBuildBackend, 'ado') }}: - # ADO now owns the all-TFM PR build that GitHub Actions used to run, so - # a PR has to cover all seven TFMs. Windows takes the .NET Framework + # ADO owns the all-TFM PR build that GitHub Actions normally runs, so a + # PR has to cover all seven TFMs. Windows takes the .NET Framework # targets and the Linux leg below takes the modern ones - the same split - # the build-all-tfm-windows / build-all-tfm-linux jobs used - which - # keeps full coverage without fanning every TFM across both pools. + # the build-all-tfm-windows / build-all-tfm-linux jobs use - which keeps + # full coverage without fanning every TFM across both pools. tfms: 'net472,net48,netstandard2.0' ${{ else }}: # GitHub Actions owns the all-TFM build; this PR gate only needs the two @@ -173,16 +202,14 @@ stages: jobnamesuffix: net48 poolName: ${{ variables.mgdPoolName }} windowsImage: ${{ variables.mgdWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} - template: .azurepipelines/test.yml parameters: configuration: Release framework: net10.0 - # When ADO owns the matrix this leg also covers Linux, replacing the - # GitHub Actions build-and-test job that ran every test project on - # ubuntu. With ciBuildBackend=actions it stays Windows-only, exactly as - # before, because GitHub Actions is still providing the Linux coverage. + # With the default ciBuildBackend=actions this leg stays Windows-only + # because the GitHub Actions build-and-test job provides the Linux + # coverage. When ADO owns the matrix it also covers Linux, replacing + # that job. ${{ if eq(parameters.ciBuildBackend, 'ado') }}: agents: '@{ windows = "${{ variables.mgdWindowsImage }}"; linux = "${{ variables.mgdLinuxImage }}" }' ${{ else }}: @@ -191,14 +218,6 @@ stages: customtestarget: net10.0 poolName: ${{ variables.mgdPoolName }} windowsImage: ${{ variables.mgdWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} - # The reference leg: net10.0 Release is the configuration the thresholds in - # coverage-thresholds.json are calibrated against, so this is the only one - # whose coverage gate blocks, and the only one that publishes the merged - # report to the build summary and to Codacy. - enforceCoverage: true - publishCoverage: true - template: .azurepipelines/test.yml parameters: configuration: Release @@ -209,8 +228,6 @@ stages: hosted: true poolName: ${{ variables.mgdPoolName }} windowsImage: ${{ variables.mgdWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} # macOS runs only on Schedule/Manual (nightly + on-demand), not on # PR or master push, to keep the PR gate fast and Windows-focused. # Passed bare: test.yml wraps it in succeeded() for the test jobs and in @@ -220,8 +237,8 @@ stages: - stage: testaot dependsOn: [] displayName: 'Test Native AoT' - # Native AoT is Schedule/Manual-only when GitHub Actions still runs its own - # AoT job. Once ADO owns the matrix this is the only place AoT runs, so it + # Native AoT is Schedule/Manual-only while GitHub Actions runs its own ubuntu + # AoT job. When ADO owns the matrix this is the only place AoT runs, so it # has to gate pull requests too. condition: and(succeeded(), or(ne(variables.ScheduledBuild, 'False'), ne(variables.AdoOwnsMatrix, 'False'))) jobs: @@ -244,8 +261,6 @@ stages: jobnamesuffix: net100debug poolName: ${{ variables.hostPoolName }} windowsImage: ${{ variables.hostWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} agents: '@{ windows = "${{ variables.hostWindowsImage }}"; linux = "${{ variables.hostLinuxImage }}" }' - template: .azurepipelines/test.yml parameters: @@ -256,8 +271,6 @@ stages: hosted: true poolName: ${{ variables.hostPoolName }} windowsImage: ${{ variables.hostWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} - stage: testnet90 dependsOn: [build] displayName: 'Test .NET 9.0' @@ -270,8 +283,6 @@ stages: jobnamesuffix: net90 poolName: ${{ variables.mgdPoolName }} windowsImage: ${{ variables.mgdWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} agents: '@{ windows = "${{ variables.mgdWindowsImage }}"; linux = "${{ variables.mgdLinuxImage }}" }' - template: .azurepipelines/test.yml parameters: @@ -282,8 +293,6 @@ stages: hosted: true poolName: ${{ variables.mgdPoolName }} windowsImage: ${{ variables.mgdWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} - stage: testnet80 dependsOn: [build] displayName: 'Test .NET 8.0' @@ -296,8 +305,6 @@ stages: jobnamesuffix: net80 poolName: ${{ variables.hostPoolName }} windowsImage: ${{ variables.hostWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} agents: '@{ windows = "${{ variables.hostWindowsImage }}"; linux = "${{ variables.hostLinuxImage }}" }' - template: .azurepipelines/test.yml parameters: @@ -308,8 +315,6 @@ stages: hosted: true poolName: ${{ variables.hostPoolName }} windowsImage: ${{ variables.hostWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} - stage: testnet472 dependsOn: [build] displayName: 'Test .NET 4.7.2' @@ -324,8 +329,6 @@ stages: customtestarget: net472 poolName: ${{ variables.hostPoolName }} windowsImage: ${{ variables.hostWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} - stage: testnetstandard20 dependsOn: [build] displayName: 'Test .NETStandard 2.0' @@ -340,8 +343,6 @@ stages: customtestarget: netstandard2.0 poolName: ${{ variables.mgdPoolName }} windowsImage: ${{ variables.mgdWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} - stage: testnetstandard21 dependsOn: [build] displayName: 'Test .NETStandard 2.1' @@ -355,8 +356,6 @@ stages: customtestarget: netstandard2.1 poolName: ${{ variables.mgdPoolName }} windowsImage: ${{ variables.mgdWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} agents: '@{ windows = "${{ variables.mgdWindowsImage }}"; linux = "${{ variables.mgdLinuxImage }}" }' - template: .azurepipelines/test.yml parameters: @@ -368,8 +367,6 @@ stages: hosted: true poolName: ${{ variables.mgdPoolName }} windowsImage: ${{ variables.mgdWindowsImage }} - coveragePoolName: ${{ variables.hostPoolName }} - coverageImage: ${{ variables.hostLinuxImage }} - stage: testnightly dependsOn: [build] displayName: 'Test long-running tiers' @@ -390,8 +387,9 @@ stages: customtestarget: net10.0 testfilter: '' hangtimeout: '30m' - # A tier, not a (framework, configuration) leg: its numbers cannot be - # merged into the pull-request gate, so skip the coverage job here. + # A tier, not a mainline leg: it re-runs a subset of the same projects + # with the filter lifted, so folding its numbers into the merged report + # would double-count. Skip coverage collection here. coverage: false agents: '@{ linux = "${{ variables.hostLinuxImage }}" }' poolName: ${{ variables.hostPoolName }} @@ -411,3 +409,69 @@ stages: agents: '@{ windows = "${{ variables.hostWindowsImage }}"; linux = "${{ variables.hostLinuxImage }}" }' poolName: ${{ variables.hostPoolName }} windowsImage: ${{ variables.hostWindowsImage }} + + +# Required check. Rolls every test stage up into a single status so the GitHub +# branch ruleset does not have to name matrix-generated jobs, whose names change +# whenever a test project or an agent is added. +# +# The verdict lives entirely in the stage `condition`, which is the one place +# Azure Pipelines reliably exposes stage results. Two earlier attempts failed: +# a stage-scoped `$[ ]` variable is evaluated when the JOB is prepared, where +# `dependencies` means sibling jobs, so `convertToJson(dependencies)` resolved +# to '{}' (build 16520); and `convertToJson(stageDependencies)` at job scope +# blew the expression memory limit, because this pipeline's ~115 jobs carry +# large output variables such as the serialised test matrix (build 16583). +# Reading `dependencies..result` here costs nothing and is the documented +# usage. +# +# 'Skipped' is accepted because the Schedule/Manual-only stages report exactly +# that on a pull request. When a stage genuinely fails the condition is false, +# this stage is skipped, and Azure Pipelines then posts NO check for it - which +# leaves the required check permanently unfulfilled and blocks the merge. That +# is deliberately fail-closed: the failing job itself is already red, and a +# required check that never reports can never be mistaken for a passing one. +- stage: gate + displayName: 'Tests passed' + dependsOn: ${{ parameters.testStages }} + condition: | + and( + not(canceled()), + in(dependencies.build.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testfastpr.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testaot.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testdebug.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testnet90.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testnet80.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testnet472.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testnetstandard20.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testnetstandard21.result, 'Succeeded', 'SucceededWithIssues', 'Skipped'), + in(dependencies.testnightly.result, 'Succeeded', 'SucceededWithIssues', 'Skipped') + ) + jobs: + - job: gate + displayName: Verify stage results + timeoutInMinutes: 10 + pool: + ${{ if ne(variables.hostPoolName, '') }}: + name: ${{ variables.hostPoolName }} + demands: + - ImageOverride -equals ${{ variables.hostLinuxImage }} + ${{ else }}: + vmImage: ${{ variables.hostLinuxImage }} + steps: + # Nothing is built or read from the tree; reaching this job IS the verdict. + - checkout: none + - pwsh: | + Write-Host 'Every test stage succeeded or was intentionally skipped.' + displayName: 'Tests passed' +# Advisory check. Merges every Cobertura fragment the run produced and +# evaluates coverage-thresholds.json once. It reports a clean failure when the +# thresholds are missed so the miss is visible, but it must NOT be added to the +# GitHub branch ruleset - 'Tests passed' above is the required check. +- template: .azurepipelines/coverage.yml + parameters: + poolName: ${{ variables.hostPoolName }} + poolImage: ${{ variables.hostLinuxImage }} + stages: ${{ parameters.testStages }} + enableCodecov: ${{ parameters.enableCodecov }} diff --git a/codecov.yml b/codecov.yml new file mode 100644 index 0000000000..b150697db1 --- /dev/null +++ b/codecov.yml @@ -0,0 +1,60 @@ +# Codecov configuration +# +# Validate with: +# curl --data-binary @codecov.yml https://codecov.io/validate +# +# Codecov is REPORTING, not a gate. The enforced coverage rules live in +# coverage-thresholds.json and are applied by .azurepipelines/check-coverage.ps1 +# inside the pipeline, where they can react to how much a patch actually +# changed. Codecov is kept for what it is genuinely better at: the pull-request +# comment, the file-by-file diff view and the coverage trend over time. +# +# Both status checks are therefore `informational: true`. They post a status so +# the number is visible, but they never fail and must never be added to the +# branch ruleset - two gates with two sets of thresholds would sooner or later +# disagree about the same pull request, and the one that is easier to silence +# would win. Flip `informational` to false only if you also retire the +# in-pipeline patch gate. +# +# The upload itself is optional on both CI systems and is skipped when the +# CODECOV_TOKEN secret is absent (as it is on fork pull requests). See +# `enableCodecov` in azure-pipelines.yml and `ENABLE_CODECOV` in +# .github/workflows/buildandtest.yml. +coverage: + precision: 2 + round: down + status: + project: + default: + target: auto + threshold: 1% + informational: true + only_pulls: true + patch: + default: + target: 80% + threshold: 5% + informational: true + only_pulls: true + +# Kept in step with the "ignore" list in coverage-thresholds.json so Codecov and +# the in-pipeline gate measure the same code. Change both together. +ignore: + - "tests/**" + - "samples/**" + - "**/obj/**" + - "**/bin/**" + - "**/*.g.cs" + +# Each CI system uploads the report it merged, under its own flag. The two +# matrices are deliberately different - Azure runs the Windows fast-PR legs, +# Actions runs the full ubuntu matrix - so carrying the flags keeps the two +# uploads from being mistaken for a coverage swing. +flag_management: + default_rules: + carryforward: true + +comment: + layout: "reach, diff, flags, files" + behavior: default + require_changes: false diff --git a/coverage-thresholds.json b/coverage-thresholds.json index 6a8ac13641..3213e4ed32 100644 --- a/coverage-thresholds.json +++ b/coverage-thresholds.json @@ -1,30 +1,49 @@ { "$comment": [ - "Coverage gates for the UA .NET Standard stack. These replace the codecov/project", - "and codecov/patch status checks; see .azurepipelines/check-coverage.ps1 for the", - "implementation and docs/DeveloperGuide.md for the contributor-facing description.", + "Coverage gates for the UA .NET Standard stack. These are the ENFORCED rules;", + "see .azurepipelines/check-coverage.ps1 for the implementation and", + "docs/DeveloperGuide.md for the contributor-facing description.", "Rates are percentages (0-100).", "", - "'project' is an absolute floor over the whole merged coverage report and BLOCKS.", + "codecov.io is still uploaded to, but only for reporting - its own", + "codecov/project and codecov/patch statuses are informational (see", + "codecov.yml) so that there is exactly one gate rather than two sets of", + "thresholds that can disagree about the same pull request.", + "", + "IMPORTANT: the coverage check is ADVISORY. Both the Azure 'Code coverage' stage and", + "the GitHub Actions 'code coverage' job report a clean failure when these thresholds", + "are missed, so the miss is visible on the pull request, but neither belongs in the", + "branch ruleset. The required checks are 'Tests passed' and 'build-and-test summary',", + "which only assert that the tests themselves passed.", + "", + "'project' is an absolute floor over the whole merged coverage report.", "Ratchet minimumLineRate/minimumBranchRate upward (together with baselineLineRate)", - "as coverage improves - never downward to make a red build go green.", + "as coverage improves - never downward to make a red check go green.", "", "The initial values are seeded from the last codecov project reading (73.61%) with", "headroom, because the in-pipeline number had not been observed yet when the gate", - "was introduced. Confirm them against the first full Code Coverage run on master", - "and tighten them then - the gate prints the exact figure it computed.", + "was introduced.", "", - "The report the gate evaluates is assembled from the fanned-out test matrix, which", - "excludes Opc.Ua.Stress.Tests / Opc.Ua.Subscriptions.Durable.Tests and the", - "LongRunning and Stress categories, so it reads slightly lower than the pre-2026", - "single-job coverage run that these figures were seeded from. Re-calibrate against", - "the net10.0 Release 'Coverage' job, which is the only leg that blocks.", + "The report is assembled from whichever legs of the run published Cobertura fragments,", + "so the number moves with the matrix: the GitHub Actions job merges every test project", + "on ubuntu and is the most representative, the Azure stage merges only the Windows", + "fast-PR legs, and scheduled runs read higher because the Debug / .NET 8 / .NET 9 /", + "netstandard stages contribute too. Re-calibrate against the GitHub Actions figure.", "", "'patch' mirrors the old codecov/patch semantics: changed lines must reach", - "target percent, tolerating threshold percentage points. It BLOCKS.", + "target percent, tolerating threshold percentage points. The requirement is", + "graduated by how much actually changed, because a percentage over a handful of", + "lines is noise: one uncovered line in a two-line fix reads as 50% and would fail", + "a flat floor, which teaches authors to ignore the check rather than fix it.", + "'bands' is consulted in order and the first band whose maxChangedLines covers the", + "patch wins; a band with enforced=false reports a warning instead of failing.", + "Anything larger than the last band falls through to target-threshold and is", + "always enforced, because at that size the percentage is meaningful and a big", + "untested change is exactly what the check exists to catch.", "", - "'ignore' carries over the exclusion list from the retired codecov.yml and is", - "applied to BOTH the project floor and the changed-lines gate." + "'ignore' mirrors the exclusion list in codecov.yml so both report on the same", + "code - change the two together - and is applied to BOTH the project floor and", + "the changed-lines check." ], "project": { "minimumLineRate": 70.0, @@ -34,7 +53,11 @@ }, "patch": { "target": 80.0, - "threshold": 5.0 + "threshold": 5.0, + "bands": [ + { "maxChangedLines": 10, "target": 50.0, "enforced": false }, + { "maxChangedLines": 100, "target": 60.0, "enforced": false } + ] }, "ignore": [ "tests/**", diff --git a/docs/DeveloperGuide.md b/docs/DeveloperGuide.md index 45c75c1c64..b599b93322 100644 --- a/docs/DeveloperGuide.md +++ b/docs/DeveloperGuide.md @@ -259,8 +259,8 @@ From **2.0** onward, package versions are produced by [Nerdbank.GitVersioning](h Two CI systems run against this repository: -- **Azure Pipelines** ([`azure-pipelines.yml`](../azure-pipelines.yml) plus the templates in [`.azurepipelines/`](../.azurepipelines)) — the all-target-framework solution build, the cross-platform test matrices, Native AoT and the coverage gate. -- **GitHub Actions** ([`.github/workflows/`](../.github/workflows)) — CodeQL, container images, the opt-in stress and stability suites, and the macOS legs of the build/test matrix. +- **Azure Pipelines** ([`azure-pipelines.yml`](../azure-pipelines.yml) plus the templates in [`.azurepipelines/`](../.azurepipelines)) — the fast pull-request test legs, the per-framework test matrices and the coverage gate, on the `netstandard` Managed DevOps Pool and Microsoft-hosted agents. +- **GitHub Actions** ([`.github/workflows/`](../.github/workflows)) — the all-target-framework solution builds, the ubuntu test matrix, Native AoT, CodeQL, container images, the opt-in stress and stability suites, and the macOS legs of the build/test matrix. ### Which system runs what @@ -268,15 +268,16 @@ A single conceptual switch decides who owns the all-TFM build, the cross-platfor | File | Setting | Default | | --- | --- | --- | -| [`azure-pipelines.yml`](../azure-pipelines.yml) | `parameters.ciBuildBackend` | `ado` | -| [`.github/workflows/buildandtest.yml`](../.github/workflows/buildandtest.yml) | `env.CI_BUILD_BACKEND` | `ado` | +| [`azure-pipelines.yml`](../azure-pipelines.yml) | `parameters.ciBuildBackend` | `actions` | +| [`.github/workflows/buildandtest.yml`](../.github/workflows/buildandtest.yml) | `env.CI_BUILD_BACKEND` | `actions` | -With the default `ado`, Azure Pipelines runs that work on the `netstandard` Managed DevOps Pool and the equivalent GitHub Actions jobs stand down on `master`/`main`. Setting both to `actions` restores the previous split, where GitHub Actions ran the ubuntu test matrix, Native AoT and the all-TFM builds. +With the default `actions` the load is split across both systems: GitHub Actions runs the all-TFM builds, the ubuntu test matrix and Native AoT, while Azure Pipelines runs the fast pull-request test legs on the managed pool and hosts the coverage gate. Setting both to `ado` moves that work onto the Managed DevOps Pool as well, and the equivalent GitHub Actions jobs stand down on `master`/`main`. -Two things are deliberately *not* covered by the switch: +Three things are deliberately *not* covered by the switch: - **macOS** always runs on GitHub-hosted runners, because Managed DevOps Pools provide no macOS image. - **`master378` and `develop/*`** keep running the GitHub Actions jobs regardless of the setting, since Azure Pipelines only builds `master`/`main` from this file. +- **The `Tests passed` and `Code coverage` stages** always run in Azure Pipelines regardless of the switch, because they roll up whatever did run (see [Required checks and coverage](#required-checks-and-coverage)). ### Test tiers @@ -289,7 +290,7 @@ The fast test stages fan every `*.Tests.csproj` out across matrix jobs and filte | `Opc.Ua.Stress.Tests` | [`.github/workflows/stress-test.yml`](../.github/workflows/stress-test.yml), opt-in | | `Opc.Ua.Aot.Tests` | `Test Native AoT` stage | -Because the individual matrix jobs are generated (and are skipped outright when Azure Pipelines owns them, or when a pull request touches no build-relevant files), branch protection should require the aggregate **`build-and-test summary`** check rather than any individual job. That job runs on every pull request — the workflow deliberately carries no `paths:` filter, because a workflow filtered out by `paths` never reports its checks and a required check that never reports blocks the pull request forever. The path allow-list is applied inside the `discover` job instead, and the summary treats an intentionally skipped job as success. +Because the individual matrix jobs are generated (and are skipped outright when Azure Pipelines owns them, or when a pull request touches no build-relevant files), branch protection requires the aggregate **`build-and-test summary`** check rather than any individual job — see [Required checks and coverage](#required-checks-and-coverage). That job runs on every pull request — the workflow deliberately carries no `paths:` filter, because a workflow filtered out by `paths` never reports its checks and a required check that never reports blocks the pull request forever. The path allow-list is applied inside the `discover` job instead, and the summary treats an intentionally skipped job as success. ### Triggering a pipeline run on a pull request @@ -305,35 +306,95 @@ To start the run, a repository owner or a collaborator with `Write` permission c This setting lives in the Azure DevOps portal (pipeline → **More actions** → **Triggers** → **Pull request validation**), not in YAML. -### Coverage gates +### Required checks and coverage -Every test stage ends in a **`Coverage ()`** job. Each test matrix entry collects coverage while it runs and publishes its raw Cobertura fragment as a pipeline artifact; the coverage job then re-assembles those fragments with ReportGenerator and evaluates them. It never re-runs the tests — doing so would serialise a suite that was deliberately fanned out across matrix jobs and blow the stage timeout. +Two concerns are deliberately kept apart, and both CI systems expose the same pair of checks: -That job is the single per-(framework, configuration) signal, so it — not an individual matrix entry — is what the GitHub branch ruleset should require. It fails when: +| Concern | Azure Pipelines | GitHub Actions | In the branch ruleset? | +| --- | --- | --- | --- | +| Every test passed | **`Tests passed`** stage | **`build-and-test summary`** job | **Yes — required** | +| Coverage meets the thresholds | **`Code coverage`** stage | **`code coverage`** job | **No — advisory** | -- any test job in its stage failed, was cancelled or never ran, -- no coverage was produced at all, -- or, on the reference leg, the merged report misses the thresholds. +Azure Pipelines reports its checks to GitHub as ` ( )`, so the two names to look for in the ruleset are `OPCFoundation.UA-.NETStandard (Tests passed Verify stage results)` and `OPCFoundation.UA-.NETStandard (Code coverage Merge and evaluate)`. -Only the **net10.0 Release** leg (`enforceCoverage: true` in [`azure-pipelines.yml`](../azure-pipelines.yml)) blocks on the thresholds and publishes the merged report to the build summary and Codacy — it is the configuration [`coverage-thresholds.json`](../coverage-thresholds.json) is calibrated against. The other legs report their numbers as a warning and still block on test failures. Every leg publishes its merged report as the `coverage-report-` artifact. +> **`Tests passed` is fail-closed, not fail-red.** Its verdict lives in the stage `condition`, which is the one place Azure Pipelines reliably exposes stage results. When a test stage fails the condition is false, the stage is skipped, and Azure Pipelines posts **no check at all** for it — so the required check stays unfulfilled and the merge stays blocked. You will see the failing test job in red and `Tests passed` still waiting, rather than two red checks. -The thresholds are enforced by [`.azurepipelines/check-coverage.ps1`](../.azurepipelines/check-coverage.ps1): +The coverage check reports a clean failure when the thresholds are missed, so a miss is visible on the pull request, but it never blocks the merge. Do not add it to the ruleset — that would make a coverage dip unmergeable, which is not the intent. + +Both required checks are single rollup jobs on purpose. The jobs underneath them are matrix-generated, so their names change whenever a test project or an agent is added, and they are skipped wholesale by the CI backend switch or by the path filter. Requiring a generated job name would therefore break as soon as the matrix changed. + +The two rollups reach their verdict differently, and the difference matters: + +| | How the verdict is reached | What a failing dependency looks like | +| --- | --- | --- | +| `build-and-test summary` (Actions) | Runs on `always()` and inspects `needs.*.result` inside the job, calling `exit 1` itself. | The check reports **failure** — a red X. | +| `Tests passed` (Azure) | Encoded in the stage `condition`, the one place Azure Pipelines reliably exposes stage results. | The stage is skipped and Azure posts **no check** — the required check stays unfulfilled. | + +The Actions job must use `always()` (rather than the implicit "all needs succeeded") precisely because a job that is *skipped* because a dependency failed surfaces to GitHub as `skipped`, and a required check reporting `skipped` is treated as **satisfied** — it would wave a red build straight through. The Azure stage is safe from that trap for a different reason: a skipped Azure *stage* posts nothing at all, so there is no `skipped` conclusion for the ruleset to accept. Verified on build 16613, where `Fast PR test` failed, `Tests passed` was skipped, and no `Tests passed` check-run reached the pull request. + +#### How coverage is measured + +Every test matrix entry collects coverage while it runs and publishes its raw Cobertura fragment as an artifact. The coverage check then downloads every fragment the run produced, merges them **once** with ReportGenerator, and evaluates the merged report. It never re-runs the tests — doing so serialises a suite that was deliberately fanned out across matrix jobs and blows the stage timeout. + +The evaluation is [`.azurepipelines/check-coverage.ps1`](../.azurepipelines/check-coverage.ps1), shared by both CI systems and driven by [`coverage-thresholds.json`](../coverage-thresholds.json): | Check | Behaviour | | --- | --- | -| **Project floor** | *Blocking.* Total line and branch rates must meet the absolute floors in `coverage-thresholds.json`. The `ignore` globs are applied here too, so samples, tests and generated code do not count. | -| **Patch coverage** | *Blocking on pull requests.* Lines you added or modified must reach `patch.target` percent, tolerating `patch.threshold` percentage points. | -| **Baseline delta** | *Advisory only.* Reports how total coverage compares with the recorded `baselineLineRate` and never fails the build. | +| **Project floor** | Total line and branch rates must meet the absolute floors in `coverage-thresholds.json`. The `ignore` globs are applied here too, so samples, tests and generated code do not count. | +| **Patch coverage** | On pull requests, lines you added or modified must reach a floor that is **graduated by how much changed** — see below. The uncovered changed lines are listed by file. | +| **Baseline delta** | Reports how total coverage compares with the recorded `baselineLineRate`. Warning only, even within this advisory check. | + +Ratchet `minimumLineRate`, `minimumBranchRate` and `baselineLineRate` **upward** as coverage improves; never lower them to turn a red check green. + +##### Patch coverage is graduated by patch size + +A coverage percentage over a handful of lines carries almost no information. One uncovered line in a two-line fix reads as 50%, and a flat floor would fail it — which teaches authors to ignore the check rather than act on it. So the requirement scales with how much actually changed: + +| Coverable changed lines | Floor | Below the floor | +| --- | --- | --- | +| 1 – 10 | 50 % | :warning: **warning**, check still passes | +| 11 – 100 | 60 % | :warning: **warning**, check still passes | +| more than 100 | `patch.target` − `patch.threshold` (75 %) | :x: **failure** | + +Only changes larger than the last band can fail the patch check. At that size the percentage is meaningful, and a large untested change is exactly what the check exists to catch. Below it you still get a warning naming the uncovered lines, so the signal is never silent — it just does not block. + +The bands live in `patch.bands` in [`coverage-thresholds.json`](../coverage-thresholds.json). They are consulted in order and the first band whose `maxChangedLines` covers the patch wins; set `enforced: true` on a band to make it blocking. Anything larger than the last band falls through to `patch.target` − `patch.threshold` and is always enforced. + +Remember that the coverage check as a whole is advisory and stays out of the branch ruleset — an enforced band produces a red `Code coverage` check, not a blocked merge. + +##### Codecov + +The merged report is also uploaded to [codecov.io](https://codecov.io), which is where the pull-request comment, the file-by-file diff view and the coverage trend live. **Codecov does not gate.** Both of its status checks are `informational: true` in [`codecov.yml`](../codecov.yml), because two gates with two sets of thresholds would eventually disagree about the same pull request and the easier one to silence would win. The rules that actually decide are the ones above. + +Each CI system uploads the report it merged, under its own flag (`azure`, `actions`), since the two matrices deliberately cover different legs. + +The upload is optional on both systems and never fails a build: + +| | Turn it off with | Also skipped when | +| --- | --- | --- | +| Azure Pipelines | the `enableCodecov` pipeline parameter (default `true`) | the `CODECOV_TOKEN` secret variable is unset | +| GitHub Actions | the `ENABLE_CODECOV` workflow `env` (default `'true'`) | the `CODECOV_TOKEN` secret is unavailable, as on fork pull requests | + +Keep the `ignore` list in `codecov.yml` in step with the one in `coverage-thresholds.json`, or the two will report on different code. + +#### Where the numbers appear + +The script renders a markdown summary that both systems surface, so you never have to open a raw log to see why coverage moved: + +- **GitHub Actions** — appended to the run's job summary, and posted as a single sticky pull-request comment that is updated in place on each run. Threshold misses additionally appear as run annotations. On a pull request **from a fork** the token is read-only, so the comment is skipped and only the job summary is written. +- **Azure Pipelines** — attached to the build summary via `##vso[task.uploadsummary]`, alongside the usual Code Coverage tab and the Codacy upload. + +Both also publish the merged HTML report as a `coverage-report` artifact. -Ratchet `minimumLineRate`, `minimumBranchRate` and `baselineLineRate` **upward** as coverage improves; never lower them to turn a red build green. +> The two systems report **different numbers**, and that is expected. With the default `actions` backend, GitHub Actions merges every test project on ubuntu, whereas Azure Pipelines merges only the Windows fast-PR legs. The GitHub figure is the more representative one. Scheduled runs read higher still, because the Debug, .NET 8/9 and netstandard stages also contribute fragments. -To reproduce a gate failure locally, generate the same report with [`tests/codecoverage.cmd`](../tests/codecoverage.cmd) (or [`tests/codecoverage.sh`](../tests/codecoverage.sh)) and run the script against it: +To reproduce a coverage failure locally, generate the same report with [`tests/codecoverage.cmd`](../tests/codecoverage.cmd) (or [`tests/codecoverage.sh`](../tests/codecoverage.sh)) and run the script against it: ```powershell -./.azurepipelines/check-coverage.ps1 -CoberturaPath ./CodeCoverage/Cobertura.xml -BaseRef master +./.azurepipelines/check-coverage.ps1 -CoberturaPath ./CodeCoverage/Cobertura.xml -BaseRef master -SummaryPath ./coverage-summary.md ``` -Omit `-BaseRef` to check only the project floor. +Omit `-BaseRef` to check only the project floor, and `-SummaryPath` to skip the markdown summary. ## Contributing and pull requests diff --git a/docs/MigrationGuide.md b/docs/MigrationGuide.md index 73dc35f321..d2e82de12c 100644 --- a/docs/MigrationGuide.md +++ b/docs/MigrationGuide.md @@ -85,6 +85,47 @@ provider through the server-wide historian registry, or override [Server address-space metadata](NodeManagers.md#server-address-space-metadata) and [Historical Access](HistoricalAccess.md). +## Migrating custom ISessionManager implementations to ShutdownAsync + +`ISessionManager.Shutdown()` is **gone**, replaced by +`ShutdownAsync(CancellationToken)`. `SessionManager` previously started its +session monitor loop with a discarded `Task.Factory.StartNew(...)`, so +`Shutdown()` only *signalled* the loop and returned: the server could +finish tearing down while the monitor was still closing expired sessions +and raising keep-alive events against half-disposed state. There is no +correct synchronous way to wait for that loop — blocking on it would be +sync-over-async — so the synchronous overload was removed rather than +kept as a trap. `ShutdownAsync` cancels the loop and awaits it before +disposing the sessions, matching `ISubscriptionManager.ShutdownAsync`. + +**Callers** await instead of calling: + +```csharp +// before +server.SessionManager.Shutdown(); + +// after +await server.SessionManager.ShutdownAsync(cancellationToken) + .ConfigureAwait(false); +``` + +**Implementers** of `ISessionManager` (for example a manager registered +through `services.AddSessionManager()`) replace `Shutdown` with +`ShutdownAsync`. If your implementation has no background work, return a +completed task: + +```csharp +public ValueTask ShutdownAsync(CancellationToken cancellationToken = default) +{ + CloseAllSessions(); + return default; +} +``` + +Deriving from `SessionManager` requires no change beyond renaming any +`Shutdown` override: `ShutdownAsync` is `virtual` and the base +implementation already awaits the monitor loop. + ## Migrating from 1.05.377 to 1.05.378 ### Asynchronous as default diff --git a/src/Opc.Ua.Client/Session/ManagedSession.CertificateChanges.cs b/src/Opc.Ua.Client/Session/ManagedSession.CertificateChanges.cs index fc76cfe126..2709b82c74 100644 --- a/src/Opc.Ua.Client/Session/ManagedSession.CertificateChanges.cs +++ b/src/Opc.Ua.Client/Session/ManagedSession.CertificateChanges.cs @@ -170,11 +170,15 @@ private void OnCertificateChange(CertificateChangeEvent evt) { case CertificateChangeKind.ApplicationCertificateUpdated: // Own-certificate rotation always needs reload + - // reconnect — no validation can save it. Fire-and- - // forget on the thread pool; the inner session's - // reconnect lock serialises with any in-flight - // ReconnectAsync (see HandleApplicationCertificateUpdatedAsync). - _ = Task.Run(() => HandleApplicationCertificateUpdatedAsync(evt)); + // reconnect — no validation can save it. Scheduled on the + // scope so the reload is awaited when the session shuts + // down; the inner session's reconnect lock serialises with + // any in-flight ReconnectAsync (see + // HandleApplicationCertificateUpdatedAsync). + m_backgroundWork.Run( + nameof(HandleApplicationCertificateUpdatedAsync), + async _ => await HandleApplicationCertificateUpdatedAsync(evt) + .ConfigureAwait(false)); break; case CertificateChangeKind.TrustListUpdated: case CertificateChangeKind.CrlUpdated: diff --git a/src/Opc.Ua.Client/Session/ManagedSession.cs b/src/Opc.Ua.Client/Session/ManagedSession.cs index 7de9272a4b..75caada5ed 100644 --- a/src/Opc.Ua.Client/Session/ManagedSession.cs +++ b/src/Opc.Ua.Client/Session/ManagedSession.cs @@ -1850,6 +1850,11 @@ public async ValueTask DisposeAsync() UnsubscribeCertificateChanges(); await StopRevalidationLoopAsync().ConfigureAwait(false); + // After unsubscribing (no new work can arrive) and before the inner + // session goes away: a certificate reload already scheduled + // reconnects through it. + await m_backgroundWork.DisposeAsync().ConfigureAwait(false); + // Tear down streaming subscription and model change tracker // before closing the session so any in-flight publish work // completes against a still-valid session. @@ -1948,6 +1953,8 @@ public void Dispose() // TODO: move ManagedSession to async-only disposal. private CancellationTokenSource? m_identityRefreshCancellation; #pragma warning restore CA2213 + private readonly BackgroundTaskScope m_backgroundWork = + new(nameof(ManagedSession), AmbientMessageContext.Telemetry); private Task? m_identityRefreshTask; private TaskCompletionSource? m_identityRefreshAttemptCompletion; private long m_identityRefreshAttemptVersion; diff --git a/src/Opc.Ua.Client/Session/Redundancy/ClientReplicaCoordinator.cs b/src/Opc.Ua.Client/Session/Redundancy/ClientReplicaCoordinator.cs index 821e48abea..d420c39716 100644 --- a/src/Opc.Ua.Client/Session/Redundancy/ClientReplicaCoordinator.cs +++ b/src/Opc.Ua.Client/Session/Redundancy/ClientReplicaCoordinator.cs @@ -81,6 +81,7 @@ public ClientReplicaCoordinator( m_logger = telemetry.CreateLogger(); m_telemetry = telemetry; + m_backgroundWork = new BackgroundTaskScope(nameof(ClientReplicaCoordinator), telemetry); m_election.LeadershipChanged += OnLeadershipChanged; } @@ -113,7 +114,9 @@ public async ValueTask StartAsync(CancellationToken ct = default) private void OnLeadershipChanged(bool isLeader) { - _ = Task.Run(() => HandleRoleChangeAsync(isLeader)); + m_backgroundWork.Run( + nameof(HandleRoleChangeAsync), + async _ => await HandleRoleChangeAsync(isLeader).ConfigureAwait(false)); } private async Task HandleRoleChangeAsync(bool isLeader) @@ -254,6 +257,11 @@ public async ValueTask DisposeAsync() { m_election.LeadershipChanged -= OnLeadershipChanged; m_cts.Cancel(); + + // Before the session and the election go away: a role change already + // in flight promotes or demotes through both of them. + await m_backgroundWork.DisposeAsync().ConfigureAwait(false); + await m_election.DisposeAsync().ConfigureAwait(false); if (m_session != null) { @@ -270,6 +278,7 @@ public async ValueTask DisposeAsync() private readonly ILogger m_logger; private readonly ITelemetryContext m_telemetry; private readonly CancellationTokenSource m_cts = new(); + private readonly BackgroundTaskScope m_backgroundWork; private ManagedSession? m_session; } diff --git a/src/Opc.Ua.Client/Session/Session.cs b/src/Opc.Ua.Client/Session/Session.cs index 80601ae2e3..b6a338e4f7 100644 --- a/src/Opc.Ua.Client/Session/Session.cs +++ b/src/Opc.Ua.Client/Session/Session.cs @@ -462,6 +462,10 @@ protected virtual async ValueTask DisposeAsyncCore(bool disposing) return; } + // Before the keep-alive timer and the channel go away: a publish + // notification already dispatched still reads session state. + await BackgroundWork.DisposeAsync().ConfigureAwait(false); + try { await StopKeepAliveTimerAsync().ConfigureAwait(false); @@ -5466,6 +5470,14 @@ protected virtual void ProcessResponseAdditionalHeader( /// protected ITelemetryContext m_telemetry; + /// + /// Owns the work the session dispatches off its own threads — publish + /// notifications in particular — so a faulting subscriber is reported and + /// nothing is still running when the session tears itself down. + /// + internal BackgroundTaskScope BackgroundWork { get; } = + new(nameof(Session), AmbientMessageContext.Telemetry); + /// /// If set totrue then the domain in the certificate must match the endpoint used. /// diff --git a/src/Opc.Ua.Client/Session/Subscription/ClassicSubscriptionEngine.cs b/src/Opc.Ua.Client/Session/Subscription/ClassicSubscriptionEngine.cs index 5eeb90b621..99ffa9d099 100644 --- a/src/Opc.Ua.Client/Session/Subscription/ClassicSubscriptionEngine.cs +++ b/src/Opc.Ua.Client/Session/Subscription/ClassicSubscriptionEngine.cs @@ -65,6 +65,9 @@ public ClassicSubscriptionEngine( m_minPublishRequestCount = kDefaultPublishRequestCount; m_maxPublishRequestCount = kMaxPublishRequestCountMax; m_timeProvider = timeProvider ?? TimeProvider.System; + m_backgroundWork = new BackgroundTaskScope( + nameof(ClassicSubscriptionEngine), + context.Telemetry); } /// @@ -197,6 +200,12 @@ public void Dispose() protected virtual void Dispose(bool disposing) { m_disposed = true; + if (disposing) + { + // Signal only: Dispose is synchronous. The throttled republish and + // the orphan cleanup both stop as soon as the token trips. + m_backgroundWork.Dispose(); + } } /// @@ -550,12 +559,14 @@ private void OnPublishComplete( // throttle the next publish to reduce // server load - _ = Task.Run(async () => - { - await m_timeProvider.Delay(TimeSpan.FromMilliseconds(100)) - .ConfigureAwait(false); - QueueBeginPublish(); - }); + m_backgroundWork.Run( + "ThrottleNextPublish", + async ct => + { + await m_timeProvider.Delay(TimeSpan.FromMilliseconds(100), ct) + .ConfigureAwait(false); + QueueBeginPublish(); + }); return; } } @@ -746,8 +757,11 @@ internal void ProcessPublishResponse( // Delete abandoned subscription from server. m_logger.ReceivedPublishResponseUnknownSubscriptionIdSubscriptionId(subscriptionId); - _ = Task.Run( - () => m_context.DeleteOrphanedSubscriptionAsync(subscriptionId)); + m_backgroundWork.Run( + "DeleteOrphanedSubscription", + async _ => await m_context + .DeleteOrphanedSubscriptionAsync(subscriptionId) + .ConfigureAwait(false)); } else { @@ -863,7 +877,7 @@ private void AddAcknowledgementToSend( throw new ArgumentNullException(nameof(acknowledgementsToSend)); } - Debug.Assert(Monitor.IsEntered(m_acknowledgementsToSendLock)); + Debug.Assert(m_acknowledgementsToSendLock.IsHeldByCurrentThread); acknowledgementsToSend.Add(new SubscriptionAcknowledgement { @@ -1015,7 +1029,8 @@ private bool BelowPublishRequestLimit(int requestCount) private readonly ILogger m_logger; private readonly ILogger m_eventLogger; private readonly TimeProvider m_timeProvider; - private readonly object m_acknowledgementsToSendLock = new(); + private readonly BackgroundTaskScope m_backgroundWork; + private readonly Lock m_acknowledgementsToSendLock = new(); private List m_acknowledgementsToSend = []; internal uint PublishCounter; private int m_tooManyPublishRequests; diff --git a/src/Opc.Ua.Client/Session/Subscription/SessionEngineContext.cs b/src/Opc.Ua.Client/Session/Subscription/SessionEngineContext.cs index 80873a17ec..9e91aa1037 100644 --- a/src/Opc.Ua.Client/Session/Subscription/SessionEngineContext.cs +++ b/src/Opc.Ua.Client/Session/Subscription/SessionEngineContext.cs @@ -210,9 +210,14 @@ public void OnPublishNotification( if (publishEventHandler != null) { - _ = Task.Run( - () => RaisePublishNotification( - publishEventHandler, notification)); + m_session.BackgroundWork.Run( + nameof(RaisePublishNotification), + _ => + { + RaisePublishNotification( + publishEventHandler, notification); + return default; + }); } } diff --git a/src/Opc.Ua.Client/Subscription/Classic/Subscription.cs b/src/Opc.Ua.Client/Subscription/Classic/Subscription.cs index 92de6c0494..87bd9215d3 100644 --- a/src/Opc.Ua.Client/Subscription/Classic/Subscription.cs +++ b/src/Opc.Ua.Client/Subscription/Classic/Subscription.cs @@ -315,7 +315,9 @@ private void TriggerUnsolicitedTransferRecovery() return; } - _ = Task.Run(RecoverAsync); + m_backgroundWork.Run( + "RecoverAfterUnsolicitedTransfer", + async _ => await RecoverAsync().ConfigureAwait(false)); async Task RecoverAsync() { @@ -403,6 +405,10 @@ protected virtual void Dispose(bool disposing) // publish worker stuck and surfaced as a test hang. ResetPublishTimerAndWorkerState(); + // Signal only: Dispose is synchronous, so it cannot await an + // in-flight recreate. The token stops it at its next await. + m_backgroundWork.Dispose(); + m_disposed = true; } } @@ -3274,6 +3280,8 @@ private void PublishingStateChanged( private long m_lastNotificationTimestamp; private int m_keepAliveInterval; private int m_publishLateCount; + private readonly BackgroundTaskScope m_backgroundWork = + new(nameof(Subscription), AmbientMessageContext.Telemetry); private bool m_disposed; private int m_recreateAfterTransferInProgress; private readonly Lock m_cache = new(); diff --git a/src/Opc.Ua.Client/Subscription/CompositeMonitoredItemCollection.cs b/src/Opc.Ua.Client/Subscription/CompositeMonitoredItemCollection.cs index fd989da375..92923e1bcd 100644 --- a/src/Opc.Ua.Client/Subscription/CompositeMonitoredItemCollection.cs +++ b/src/Opc.Ua.Client/Subscription/CompositeMonitoredItemCollection.cs @@ -69,7 +69,8 @@ namespace Opc.Ua.Client.Subscriptions.MonitoredItems /// internal sealed class CompositeMonitoredItemCollection : IMonitoredItemCollection, - IMonitoredItemRetryCollection + IMonitoredItemRetryCollection, + IDisposable { /// /// Construct a composite over the supplied (shared) partition @@ -104,7 +105,7 @@ internal sealed class CompositeMonitoredItemCollection : /// disables idle-delete. public CompositeMonitoredItemCollection( List partitions, - object partitionLock, + Lock partitionLock, PartitionPlacementPolicy? policy = null, Func? partitionFactory = null, TimeProvider? timeProvider = null, @@ -542,7 +543,7 @@ private void MaybeArmIdleTimer(IManagedSubscription partition) private void RunIdleDelete(IManagedSubscription partition) { - _ = Task.Run(async () => + m_backgroundWork.Run(nameof(RunIdleDelete), async _ => { Func? disposer; lock (m_partitionLock) @@ -618,6 +619,14 @@ private void RunIdleDelete(IManagedSubscription partition) }); } + /// + /// Stops scheduling idle deletes and disposes every armed timer. + /// + public void Dispose() + { + DisposeIdleTimers(); + } + /// /// Cancel and dispose every armed idle-delete timer. Called /// by the wrapper's DisposeAsync so timers do not @@ -626,6 +635,11 @@ private void RunIdleDelete(IManagedSubscription partition) /// internal void DisposeIdleTimers() { + // Signal only: this is reached from a synchronous teardown path. An + // idle delete already running finishes against a partition it has + // already removed from the maps, so it is safe to let it complete. + m_backgroundWork.Dispose(); + lock (m_partitionLock) { foreach (ITimer t in m_idleTimers.Values) @@ -708,7 +722,9 @@ private bool IsSinglePartitionFastPath => m_partitionFactory == null || m_policy == null; private readonly List m_partitions; - private readonly object m_partitionLock; + private readonly Lock m_partitionLock; + private readonly BackgroundTaskScope m_backgroundWork = + new(nameof(CompositeMonitoredItemCollection), AmbientMessageContext.Telemetry); private readonly PartitionPlacementPolicy? m_policy; private readonly Func? m_partitionFactory; private readonly TimeProvider m_timeProvider; diff --git a/src/Opc.Ua.Client/Subscription/LogicalSubscription.cs b/src/Opc.Ua.Client/Subscription/LogicalSubscription.cs index 68f7939370..cbca6f8680 100644 --- a/src/Opc.Ua.Client/Subscription/LogicalSubscription.cs +++ b/src/Opc.Ua.Client/Subscription/LogicalSubscription.cs @@ -118,7 +118,7 @@ public LogicalSubscription( throw new ArgumentNullException(nameof(primary)); } m_partitions = [primary]; - m_partitionLock = new object(); + m_partitionLock = new Lock(); m_monitoredItems = new CompositeMonitoredItemCollection( m_partitions, m_partitionLock, @@ -632,8 +632,8 @@ private async ValueTask DisposeCoreAsync() ThrowIfDispatchingNotification(snapshot, "disposed"); // Stop any armed secondary-partition idle timers first so // they cannot fire against partitions we are tearing - // down. - m_monitoredItems.DisposeIdleTimers(); + // down, and stop scheduling new idle deletes. + m_monitoredItems.Dispose(); // Dispose partitions in reverse-add order; secondary partitions // (added on demand) hold references back to the primary's // notification handler in subsequent milestones, so removing @@ -783,7 +783,7 @@ private IManagedSubscription AppendPartition( internal IManagedSubscription Primary => m_partitions[0]; private readonly List m_partitions; - private readonly object m_partitionLock; + private readonly Lock m_partitionLock; private readonly CompositeMonitoredItemCollection m_monitoredItems; #pragma warning disable CA2213 // Retained so concurrent recreate waiters can release safely. private readonly SemaphoreSlim m_recreateGate = new(1, 1); diff --git a/src/Opc.Ua.Client/Subscription/Subscription.cs b/src/Opc.Ua.Client/Subscription/Subscription.cs index f8a565e83c..dff5d0817c 100644 --- a/src/Opc.Ua.Client/Subscription/Subscription.cs +++ b/src/Opc.Ua.Client/Subscription/Subscription.cs @@ -203,6 +203,7 @@ protected Subscription( m_handler = handler; m_context = context; m_monitoredItems = new MonitoredItemManager(this, telemetry); + m_backgroundWork = new BackgroundTaskScope(nameof(Subscription), telemetry); m_publishTimer = TimeProvider.CreateTimer(OnKeepAlive, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); if (loadState != null) @@ -596,6 +597,11 @@ protected override async ValueTask DisposeAsync(bool disposing) try { await m_cts.CancelAsync().ConfigureAwait(false); + + // Before the state machine and monitored items go away: an + // in-flight recreate walks both of them. + await m_backgroundWork.DisposeAsync().ConfigureAwait(false); + await m_stateManagement.ConfigureAwait(false); await m_stateLock.WaitAsync(CancellationToken.None).ConfigureAwait(false); @@ -797,7 +803,10 @@ protected override ValueTask OnStatusChangeNotificationAsync(uint sequenceNumber Interlocked.CompareExchange( ref m_recreateAfterTransferInProgress, 1, 0) == 0) { - _ = Task.Run(RecoverAfterUnsolicitedTransferAsync); + m_backgroundWork.Run( + nameof(RecoverAfterUnsolicitedTransferAsync), + async _ => await RecoverAfterUnsolicitedTransferAsync() + .ConfigureAwait(false)); } return default; } @@ -1458,6 +1467,7 @@ private void AdjustCounts(SubscriptionOptions options, out uint keepAliveCount, private readonly ISubscriptionNotificationHandler m_handler; private readonly ISubscriptionContext m_context; private readonly MonitoredItemManager m_monitoredItems; + private readonly BackgroundTaskScope m_backgroundWork; } /// diff --git a/src/Opc.Ua.Client/Utils/PrioritizedChannel.cs b/src/Opc.Ua.Client/Utils/PrioritizedChannel.cs index 1d200c9e12..d636327b3f 100644 --- a/src/Opc.Ua.Client/Utils/PrioritizedChannel.cs +++ b/src/Opc.Ua.Client/Utils/PrioritizedChannel.cs @@ -29,6 +29,7 @@ #if !NET8_0_OR_GREATER using System.Collections.Generic; +using System.Threading; using System.Threading.Tasks; #endif @@ -275,7 +276,7 @@ await m_channel.m_semaphore.WaitAsync(ct) private readonly IComparer m_comparer; private readonly List m_heap; private readonly SemaphoreSlim m_semaphore; - private readonly object m_lock = new(); + private readonly Lock m_lock = new(); private bool m_completed; } #endif diff --git a/src/Opc.Ua.Core.Diagnostics/Capture/Sources/NicCaptureSource.cs b/src/Opc.Ua.Core.Diagnostics/Capture/Sources/NicCaptureSource.cs index 3b2fd834da..9def123997 100644 --- a/src/Opc.Ua.Core.Diagnostics/Capture/Sources/NicCaptureSource.cs +++ b/src/Opc.Ua.Core.Diagnostics/Capture/Sources/NicCaptureSource.cs @@ -57,6 +57,7 @@ public sealed class NicCaptureSource : ICaptureSource "SharpPcap requires dynamic native libpcap/Npcap loading and is not NativeAOT/trimming safe."; private readonly ILogger m_logger; + private readonly BackgroundTaskScope m_backgroundWork; private readonly Lock m_lock = new(); private LibPcapLiveDevice? m_device; @@ -82,6 +83,7 @@ public NicCaptureSource(ILoggerFactory? loggerFactory = null) { ILoggerFactory factory = loggerFactory ?? NullLoggerFactory.Instance; m_logger = factory.CreateLogger(); + m_backgroundWork = new BackgroundTaskScope(nameof(NicCaptureSource)); } /// @@ -271,6 +273,10 @@ public async IAsyncEnumerable ReadCapturedFramesAsync( /// public async ValueTask DisposeAsync() { + // A best-effort device stop may already be running against the same + // handle StopAsync is about to close. + await m_backgroundWork.DisposeAsync().ConfigureAwait(false); + try { await StopAsync(CancellationToken.None).ConfigureAwait(false); @@ -380,7 +386,13 @@ private void OnPacketArrival(object sender, PacketCapture e) if (bytes > m_maxBytes || frames > m_maxFrames || DateTimeOffset.UtcNow - m_startedAt > m_maxDuration) { m_stopRequested = true; - _ = Task.Run(StopDeviceCaptureBestEffort); + m_backgroundWork.Run( + nameof(StopDeviceCaptureBestEffort), + _ => + { + StopDeviceCaptureBestEffort(); + return default; + }); } } diff --git a/src/Opc.Ua.Core/Stack/Client/Channels/ClientChannelManager.cs b/src/Opc.Ua.Core/Stack/Client/Channels/ClientChannelManager.cs index b4d21afd8d..79245bc618 100644 --- a/src/Opc.Ua.Core/Stack/Client/Channels/ClientChannelManager.cs +++ b/src/Opc.Ua.Core/Stack/Client/Channels/ClientChannelManager.cs @@ -497,10 +497,19 @@ private ClientChannelManager( } ReconnectPolicy = reconnectPolicy ?? new ExponentialBackoffChannelReconnectPolicy(); TimeProvider = timeProvider ?? TimeProvider.System; + BackgroundWork = new BackgroundTaskScope(nameof(ClientChannelManager), telemetry); m_certRotation = new ClientChannelManagerCertRotation(this); WireCertificateRotation(); } + /// + /// Owns the background work the channel internals start but cannot await + /// inline, so it is drained before the manager goes away. + /// + internal BackgroundTaskScope BackgroundWork { get; } + + BackgroundTaskScope IChannelEntryHost.BackgroundWork => BackgroundWork; + /// public ValueTask GetAsync( IReconnectParticipant participant, @@ -780,12 +789,29 @@ await AwaitReconnectResultAsync(entry.RequestReconnectAsync(budget, ct)) } } + /// + /// Whether a terminal reconnect failure is a lost race against a + /// concurrent close, in which case retrying on a freshly swapped entry + /// recovers transparently. + /// + /// + /// A reconnect cycle that stopped because the reconnect policy ran out of + /// attempts, or because the caller's retry budget ran out of time, is a + /// deliberate terminal outcome rather than a race. It leaves the entry + /// with + /// - indistinguishable from a + /// lost race by state and status code alone - so the entry is asked directly. + /// Swapping and reconnecting after a deliberate stop would run a second, + /// unbudgeted cycle behind the swap back-off and defeat the very limit that + /// ended the first one. + /// private static bool IsTerminalReconnectRace( ChannelEntry entry, ServiceResultException sre, CancellationToken ct) { return !ct.IsCancellationRequested && + !entry.ReconnectStoppedByRetryPolicy && sre.StatusCode == StatusCodes.BadSecureChannelClosed && entry.State is ChannelState.Closed or ChannelState.Faulted; } @@ -1041,6 +1067,11 @@ await entry.DisposeAsync(ChannelCloseReason.ManagerDisposed) .ConfigureAwait(false); } + // Drain last: releasing a lease or tearing down an entry can still + // schedule background work, and none of it may outlive the manager + // whose state it touches. + await BackgroundWork.DisposeAsync().ConfigureAwait(false); + m_meter?.Dispose(); m_shutdownCts.Dispose(); } diff --git a/src/Opc.Ua.Core/Stack/Client/Channels/Internal/ChannelEntry.cs b/src/Opc.Ua.Core/Stack/Client/Channels/Internal/ChannelEntry.cs index b18740b1d1..d37b786a3e 100644 --- a/src/Opc.Ua.Core/Stack/Client/Channels/Internal/ChannelEntry.cs +++ b/src/Opc.Ua.Core/Stack/Client/Channels/Internal/ChannelEntry.cs @@ -441,12 +441,38 @@ public Task RequestReconnectAsync(IRetryBudget? budget, CancellationToken if (starter) { - _ = Task.Run(() => RunReconnectCycleAsync(tcs), CancellationToken.None); + // The cycle's own outcome is observed through tcs, but the manager + // still has to know the work exists so disposal waits for it. + if (!OwnerManager.BackgroundWork.Run( + nameof(RunReconnectCycleAsync), + async _ => await RunReconnectCycleAsync(tcs).ConfigureAwait(false))) + { + // The manager is going away; nothing will run the cycle. + tcs.TrySetException(ServiceResultException.Create( + StatusCodes.BadSecureChannelClosed, + "Channel manager is shutting down.")); + } } return tcs.Task.WaitAsync(ct); } + /// + /// Whether the most recent reconnect cycle on this entry stopped because the + /// reconnect policy ran out of attempts or the caller's retry budget ran out + /// of time, rather than because it lost a race against a concurrent close. + /// + /// + /// Both stops leave the entry and surface + /// the same to the caller, so + /// the state and status code alone cannot tell them apart. Only a genuine race + /// is worth retrying on a freshly swapped entry; retrying a deliberate stop + /// would run a second, unbudgeted reconnect cycle behind the swap back-off and + /// defeat the very limit that ended the first one. + /// + internal bool ReconnectStoppedByRetryPolicy + => Volatile.Read(ref m_reconnectStoppedByRetryPolicy) != 0; + private TimeSpan? ConsumeServerRetryAfterHint() { ITransportChannel? underlying; @@ -661,6 +687,10 @@ private async Task RunReconnectCycleAsync( ServiceResult? finalError = null; int attemptsStarted = 0; + // A fresh cycle has not yet decided to stop, so any stale verdict from + // an earlier cycle on this entry must not leak into it. + Volatile.Write(ref m_reconnectStoppedByRetryPolicy, 0); + CancellationToken shutdownToken = OwnerManager.ShutdownToken; async Task StopWithFaultAsync( @@ -676,6 +706,13 @@ async Task StopWithFaultAsync( FailReady(new ServiceResultException( StatusCodes.BadSecureChannelClosed, message)); + + // Record before completing the waiters: this is a deliberate stop + // (the retry policy or the caller's budget said so), not a lost race + // against a concurrent close, and callers inspect the flag as soon + // as tcs completes. + Volatile.Write(ref m_reconnectStoppedByRetryPolicy, 1); + await NotifyParticipantsFinalAsync().ConfigureAwait(false); finalOutcome = kReconnectOutcomePolicyExhausted; OwnerManager.RecordReconnectAttempt(this, finalOutcome); @@ -1339,8 +1376,7 @@ private struct AggregatedReactivationOutcome public bool FatalForChannel; } - private const string kReconnectOutcomeSuccess = "success"; - private const string kReconnectOutcomeTransientFailure = "transient-failure"; + private const string kReconnectOutcomeSuccess = "success"; private const string kReconnectOutcomeTransientFailure = "transient-failure"; private const string kReconnectOutcomeFatalChannel = "fatal-channel"; private const string kReconnectOutcomePolicyExhausted = "policy-exhausted"; private readonly Lock m_lock = new(); @@ -1358,6 +1394,7 @@ private struct AggregatedReactivationOutcome private long m_clientCertificateVersion; private TaskCompletionSource m_readyGate; private TaskCompletionSource? m_reconnectCoalescer; + private int m_reconnectStoppedByRetryPolicy; private IRetryBudget? m_effectiveBudget; } diff --git a/src/Opc.Ua.Core/Stack/Client/Channels/Internal/IChannelEntryHost.cs b/src/Opc.Ua.Core/Stack/Client/Channels/Internal/IChannelEntryHost.cs index 2876d78e87..9ed3ff3b13 100644 --- a/src/Opc.Ua.Core/Stack/Client/Channels/Internal/IChannelEntryHost.cs +++ b/src/Opc.Ua.Core/Stack/Client/Channels/Internal/IChannelEntryHost.cs @@ -42,6 +42,7 @@ internal interface IChannelEntryHost : IClientChannelManager TimeProvider TimeProvider { get; } IChannelReconnectPolicy ReconnectPolicy { get; } CancellationToken ShutdownToken { get; } + BackgroundTaskScope BackgroundWork { get; } Bindings.ITransportChannelBindings? ChannelFactory { get; } ApplicationConfiguration Configuration { get; } diff --git a/src/Opc.Ua.Core/Stack/Client/Channels/Internal/ManagedTransportChannelLease.cs b/src/Opc.Ua.Core/Stack/Client/Channels/Internal/ManagedTransportChannelLease.cs index 8cce3bdf17..187138a599 100644 --- a/src/Opc.Ua.Core/Stack/Client/Channels/Internal/ManagedTransportChannelLease.cs +++ b/src/Opc.Ua.Core/Stack/Client/Channels/Internal/ManagedTransportChannelLease.cs @@ -381,17 +381,9 @@ public void Dispose() return; } ChannelEntry entry = Entry; - _ = Task.Run(async () => - { - try - { - await entry.ReleaseLeaseAsync(this).ConfigureAwait(false); - } - catch - { - // best effort — sync Dispose cannot surface async failures - } - }); + entry.OwnerManager.BackgroundWork.Run( + nameof(ChannelEntry.ReleaseLeaseAsync), + async _ => await entry.ReleaseLeaseAsync(this).ConfigureAwait(false)); } private async ValueTask DisposeAsyncCore() diff --git a/src/Opc.Ua.Core/Stack/Tcp/ChannelAsyncOperation.cs b/src/Opc.Ua.Core/Stack/Tcp/ChannelAsyncOperation.cs index 52cd0e5edc..03da348e66 100644 --- a/src/Opc.Ua.Core/Stack/Tcp/ChannelAsyncOperation.cs +++ b/src/Opc.Ua.Core/Stack/Tcp/ChannelAsyncOperation.cs @@ -433,7 +433,21 @@ protected virtual bool InternalComplete(bool doNotBlock, object? result) { if (doNotBlock) { - _ = Task.Run(() => callback(this)); + // Same contract as the inline branch below: the callback is + // user code and may throw. Without this the exception is + // swallowed by the unobserved-task machinery and the failure + // is invisible. + _ = Task.Run(() => + { + try + { + callback(this); + } + catch (Exception e) + { + m_logger.ChannelAsyncOperationLogMessage0(e); + } + }); } else { diff --git a/src/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs b/src/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs index 08a905ad01..663fa702e9 100644 --- a/src/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs +++ b/src/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs @@ -1445,12 +1445,14 @@ public void SendResponse(uint requestId, IServiceResponse response) /// private void ResetQueuedResponses(Action action) { - _ = Task.Factory.StartNew( - action, - m_queuedResponses, - default, - TaskCreationOptions.DenyChildAttach, - TaskScheduler.Default); + SortedDictionary queued = m_queuedResponses; + BackgroundWork.Run( + nameof(ResetQueuedResponses), + _ => + { + action(queued); + return default; + }); m_queuedResponses = []; } diff --git a/src/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.cs b/src/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.cs index 7861eaedc6..ea958ec1d1 100644 --- a/src/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.cs +++ b/src/Opc.Ua.Core/Stack/Tcp/UaSCBinaryChannel.cs @@ -168,6 +168,7 @@ private UaSCUaBinaryChannel( // create a unique contex if none provided. m_contextId = contextId; Telemetry = telemetry; + m_backgroundWork = new BackgroundTaskScope(nameof(UaSCUaBinaryChannel), telemetry); m_logger = telemetry.CreateLogger(); TimeProvider = timeProvider ?? TimeProvider.System; m_lastActiveTimestamp = TimeProvider.GetTimestamp(); @@ -300,6 +301,11 @@ protected virtual void Dispose(bool disposing) { if (disposing) { + // Signal only: Dispose is synchronous, so it cannot await the + // drain without blocking. State-change notifications stop being + // accepted immediately and any in flight are cancelled. + m_backgroundWork.Dispose(); + m_receiveLoopCts?.Cancel(); IUaSCByteTransport? transport = Interlocked.Exchange(ref m_transport, null); transport?.Close(); @@ -334,6 +340,12 @@ protected virtual void Dispose(bool disposing) /// protected ITelemetryContext Telemetry { get; } + /// + /// Owns the work the channel schedules off its own threads, so a faulting + /// subscriber is reported and nothing is scheduled after disposal. + /// + private protected BackgroundTaskScope BackgroundWork => m_backgroundWork; + /// /// The used by this channel for /// time and duration calculations. @@ -388,7 +400,17 @@ protected void ChannelStateChanged(TcpChannelState state, ServiceResult reason) TcpChannelStateEventHandler? stateChanged = m_stateChanged; if (stateChanged != null) { - _ = Task.Run(() => stateChanged?.Invoke(this, state, reason)); + // Off the caller's thread because a subscriber must not be able to + // stall the channel, but owned so a throwing subscriber is reported + // rather than silently dropped, and so notifications stop once the + // channel is disposed. + m_backgroundWork.Run( + nameof(ChannelStateChanged), + _ => + { + stateChanged.Invoke(this, state, reason); + return default; + }); } } @@ -1244,6 +1266,7 @@ public void UpdateLastActiveTime() private BufferCollection? m_partialMessageChunks; private IUaSCByteTransport? m_transport; + private readonly BackgroundTaskScope m_backgroundWork; private CancellationTokenSource? m_receiveLoopCts; private Task? m_receiveLoopTask; private int m_receiveLoopRunning; diff --git a/src/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs b/src/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs index 2e24a5f99a..2509c28378 100644 --- a/src/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs +++ b/src/Opc.Ua.Gds.Client.Common/GlobalDiscoveryServerClient.cs @@ -87,6 +87,8 @@ public GlobalDiscoveryServerClient( m_options = options ?? new GdsClientOptions(); MessageContext = configuration.CreateMessageContext(); m_logger = MessageContext.Telemetry.CreateLogger(); + m_backgroundWork = new BackgroundTaskScope( + nameof(GlobalDiscoveryServerClient), MessageContext.Telemetry); m_sessionFactory = sessionFactory ?? new DefaultSessionFactory(MessageContext.Telemetry) { @@ -244,6 +246,11 @@ public async ValueTask DisposeAsync() return; } m_disposed = true; + + // A keep-alive cleanup already scheduled disposes the session this + // client still owns, so it must finish first. + await m_backgroundWork.DisposeAsync().ConfigureAwait(false); + try { await m_disposeCts.CancelAsync().ConfigureAwait(false); @@ -580,12 +587,13 @@ private void Session_KeepAlive(ISession session, KeepAliveEventArgs e) } // Bad keep-alive: schedule async cleanup without blocking the keep-alive - // callback thread. Errors are logged; we never throw out of fire-and-forget. - _ = Task.Run(async () => + // callback thread. Errors are logged; the scope reports anything that + // escapes and drains the cleanup before the client goes away. + m_backgroundWork.Run("KeepAliveSessionCleanup", async ct => { try { - await m_lock.WaitAsync().ConfigureAwait(false); + await m_lock.WaitAsync(ct).ConfigureAwait(false); try { if (ReferenceEquals(session, Session)) @@ -986,6 +994,7 @@ private async Task ConnectIfNeededAsync(CancellationToken ct) private readonly SemaphoreSlim m_lock = new(1, 1); private readonly ISessionFactory m_sessionFactory; private readonly ILogger m_logger; + private readonly BackgroundTaskScope m_backgroundWork; private readonly GdsClientOptions m_options; private readonly TimeProvider m_timeProvider; private readonly CancellationTokenSource m_disposeCts = new(); diff --git a/src/Opc.Ua.PubSub/Application/MetaDataPublisher.cs b/src/Opc.Ua.PubSub/Application/MetaDataPublisher.cs index 6edcd43964..e22641c725 100644 --- a/src/Opc.Ua.PubSub/Application/MetaDataPublisher.cs +++ b/src/Opc.Ua.PubSub/Application/MetaDataPublisher.cs @@ -87,6 +87,7 @@ internal sealed class MetaDataPublisher : IAsyncDisposable private readonly IReadOnlyDictionary m_encoders; private readonly IPubSubDiagnostics m_diagnostics; private readonly ITelemetryContext m_telemetry; + private readonly BackgroundTaskScope m_backgroundWork; private readonly TimeProvider m_timeProvider; private readonly ILogger m_logger; private readonly Lock m_gate = new(); @@ -148,6 +149,7 @@ public MetaDataPublisher( m_encoders = encoders; m_diagnostics = diagnostics; m_telemetry = telemetry; + m_backgroundWork = new BackgroundTaskScope(nameof(MetaDataPublisher), telemetry); m_timeProvider = timeProvider; m_logger = telemetry.CreateLogger(); } @@ -206,11 +208,11 @@ private void OnConfigurationChanged(object? sender, } /// - public ValueTask DisposeAsync() + public async ValueTask DisposeAsync() { if (Interlocked.Exchange(ref m_disposed, 1) != 0) { - return default; + return; } lock (m_gate) { @@ -222,7 +224,10 @@ public ValueTask DisposeAsync() } } UnsubscribeFromDataSets(); - return default; + + // A metadata publish already scheduled still writes through the + // connection, so it must not outlive this publisher. + await m_backgroundWork.DisposeAsync().ConfigureAwait(false); } /// @@ -342,7 +347,7 @@ private void OnDataSetMetaDataChanged( } // Scheduled on the thread pool for the same reason as the registry // handler: the caller may still hold a lock. - _ = Task.Run(async () => + m_backgroundWork.Run("PublishMetaDataChange", async _ => { try { @@ -419,7 +424,7 @@ private void OnMetaDataChanged(object? sender, DataSetMetaDataChangedEventArgs e // Schedule on the thread pool to avoid running async work // on the registry caller's thread; the caller may still be // holding the registry write lock. - _ = Task.Run(async () => + m_backgroundWork.Run("PublishMetaDataChange", async _ => { try { diff --git a/src/Opc.Ua.Redundancy.Kubernetes/Health/KubernetesReadinessServer.cs b/src/Opc.Ua.Redundancy.Kubernetes/Health/KubernetesReadinessServer.cs index 3687433329..a99804ce1b 100644 --- a/src/Opc.Ua.Redundancy.Kubernetes/Health/KubernetesReadinessServer.cs +++ b/src/Opc.Ua.Redundancy.Kubernetes/Health/KubernetesReadinessServer.cs @@ -106,6 +106,10 @@ public async ValueTask DisposeAsync() // expected on shutdown } } + + await DrainRequestHandlersAsync().ConfigureAwait(false); + + m_handlerSlots.Dispose(); m_cts.Dispose(); } @@ -147,7 +151,59 @@ private async Task ListenAsync(CancellationToken ct) return; } - _ = Task.Run(() => HandleAsync(context), ct); + // Bound the fan-out: an unauthenticated readiness endpoint would + // otherwise spawn one unowned handler per inbound request, with + // nothing to stop a flood and nothing to wait for at shutdown. + try + { + await m_handlerSlots.WaitAsync(ct).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + context.Response.Close(); + return; + } + + _ = Task.Run( + async () => + { + try + { + await HandleAsync(context).ConfigureAwait(false); + } + finally + { + m_handlerSlots.Release(); + } + }, + CancellationToken.None); + } + } + + /// + /// Waits for the request handlers still in flight to finish by reclaiming + /// every concurrency slot. + /// + /// + /// Holding all slots is only possible + /// once no handler owns one, so this doubles as the drain. It is bounded so a + /// wedged client socket cannot stall disposal indefinitely; the listener is + /// already closed by then, so an abandoned handler can only fail its own + /// response write. + /// + private async Task DrainRequestHandlersAsync() + { + using var drainTimeout = new CancellationTokenSource(kDrainTimeout); + try + { + for (int i = 0; i < kMaxConcurrentRequests; i++) + { + await m_handlerSlots.WaitAsync(drainTimeout.Token).ConfigureAwait(false); + } + } + catch (OperationCanceledException) + { + m_logger?.KubernetesReadinessDrainTimedOut(); } } @@ -191,6 +247,9 @@ private static string ToPrefix(string host, int port, string path) private readonly HttpListener m_listener; private readonly Lock m_lock = new(); private readonly CancellationTokenSource m_cts = new(); + private readonly SemaphoreSlim m_handlerSlots = new(kMaxConcurrentRequests, kMaxConcurrentRequests); + private const int kMaxConcurrentRequests = 16; + private static readonly TimeSpan kDrainTimeout = TimeSpan.FromSeconds(5); private Task? m_loop; private bool m_started; private bool m_disposed; @@ -204,6 +263,10 @@ internal static partial class KubernetesReadinessServerLog [LoggerMessage(EventId = RedundancyKubernetesEventIds.KubernetesReadinessServer + 0, Level = LogLevel.Error, Message = "Kubernetes readiness request failed.")] public static partial void KubernetesReadinessRequestFailed(this ILogger logger, Exception exception); + + [LoggerMessage(EventId = RedundancyKubernetesEventIds.KubernetesReadinessServer + 1, Level = LogLevel.Warning, + Message = "Kubernetes readiness request handlers did not drain before the shutdown timeout.")] + public static partial void KubernetesReadinessDrainTimedOut(this ILogger logger); } } diff --git a/src/Opc.Ua.Server/Configuration/ApplicationConfigurationFile.cs b/src/Opc.Ua.Server/Configuration/ApplicationConfigurationFile.cs index 1d1d60c4bf..5ab5933485 100644 --- a/src/Opc.Ua.Server/Configuration/ApplicationConfigurationFile.cs +++ b/src/Opc.Ua.Server/Configuration/ApplicationConfigurationFile.cs @@ -120,6 +120,7 @@ public ApplicationConfigurationFile( m_readAccess = readAccess ?? throw new ArgumentNullException(nameof(readAccess)); m_writeAccess = writeAccess ?? throw new ArgumentNullException(nameof(writeAccess)); m_logger = telemetry.CreateLogger(); + m_backgroundWork = new BackgroundTaskScope(nameof(ApplicationConfigurationFile), telemetry); m_coordinator = coordinator; m_timeProvider = timeProvider ?? TimeProvider.System; m_activityTimeout = activityTimeout; @@ -213,6 +214,10 @@ private void OnActivityTimerExpired(long generation) /// public void Dispose() { + // Signal only: Dispose is synchronous. The revert window stops at + // its delay as soon as the token trips. + m_backgroundWork.Dispose(); + CancelPendingRevert(); lock (m_lock) { @@ -807,7 +812,7 @@ private void ScheduleRevert(Uuid updateId, double restartDelayTime, double rever return; } - _ = Task.Run(async () => + m_backgroundWork.Run("RevertWindow", async _ => { try { @@ -944,6 +949,7 @@ public ActivityTimerState(ApplicationConfigurationFile owner, long generation) private readonly SecureAccess m_readAccess; private readonly SecureAccess m_writeAccess; private readonly ILogger m_logger; + private readonly BackgroundTaskScope m_backgroundWork; private readonly IPushConfigurationTransactionCoordinator? m_coordinator; private readonly TimeProvider m_timeProvider; private readonly double m_activityTimeout; diff --git a/src/Opc.Ua.Server/Configuration/ConfigurationNodeManager.cs b/src/Opc.Ua.Server/Configuration/ConfigurationNodeManager.cs index 6e5a3dc289..56e770a03c 100644 --- a/src/Opc.Ua.Server/Configuration/ConfigurationNodeManager.cs +++ b/src/Opc.Ua.Server/Configuration/ConfigurationNodeManager.cs @@ -362,6 +362,10 @@ protected override void Dispose(bool disposing) { if (disposing) { + // Signal only: Dispose is synchronous. A deferred apply already + // running stops at its next await once the token trips. + m_backgroundWork.Dispose(); + if (FindPredefinedNode(ObjectIds.Server_Namespaces) is NamespacesState serverNamespacesNode) { @@ -860,7 +864,7 @@ private void ScheduleDeferredReset() m_pendingResetTask = completion.Task; } - _ = Task.Run(async () => + m_backgroundWork.Run("DeferredApplyChanges", async _ => { try { @@ -3551,7 +3555,7 @@ private void ScheduleDeferredApplyChanges( m_pendingApplyChangesTask = completion.Task; } - _ = Task.Run(async () => + m_backgroundWork.Run("DeferredApplyChanges", async _ => { try { @@ -3620,9 +3624,13 @@ await m_timeProvider.Delay(gracePeriod, shutdownToken) // here would be needless work. if (rotations.Count > 0 && m_configuration.CertificateManager != null) { + // Deliberately not cancellable: a rotation that has begun + // must finish updating the configuration, otherwise the + // server is left advertising a certificate it no longer has. await m_configuration.CertificateManager.UpdateAsync( m_configuration.SecurityConfiguration, - m_configuration.ApplicationUri) + m_configuration.ApplicationUri, + CancellationToken.None) .ConfigureAwait(false); } @@ -3651,7 +3659,9 @@ IReadOnlyList listeners try { IReadOnlyList closed - = await rotator.CloseChannelsForCertificateAsync(rotation.OldCertificate) + = await rotator.CloseChannelsForCertificateAsync( + rotation.OldCertificate, + CancellationToken.None) .ConfigureAwait(false); totalCut += closed.Count; } @@ -4456,6 +4466,8 @@ private sealed record AlarmMonitorEntry( private Task m_pendingApplyChangesTask = Task.CompletedTask; private Task m_pendingResetTask = Task.CompletedTask; private readonly CancellationTokenSource m_shutdownCts = new(); + private readonly BackgroundTaskScope m_backgroundWork = + new(nameof(ConfigurationNodeManager), AmbientMessageContext.Telemetry); private readonly AsyncLocal?> m_activeRotationCollector = new(); /// diff --git a/src/Opc.Ua.Server/NodeManager/Lifecycle/NodeManagerLifecycle.cs b/src/Opc.Ua.Server/NodeManager/Lifecycle/NodeManagerLifecycle.cs index 6f3a8c40c4..4c6f7e1bb5 100644 --- a/src/Opc.Ua.Server/NodeManager/Lifecycle/NodeManagerLifecycle.cs +++ b/src/Opc.Ua.Server/NodeManager/Lifecycle/NodeManagerLifecycle.cs @@ -95,6 +95,10 @@ internal int RetiredNodeManagerCount /// public void Dispose() { + // Signal only: Dispose is synchronous. A drain already running + // finishes retiring the generations it captured. + m_backgroundWork.Dispose(); + bool disposeSemaphore; lock (m_operationLifetimeLock) { @@ -3115,7 +3119,9 @@ private void ScheduleRetiredGenerationDrainCleanup() ExecutionContext.SuppressFlow(); restoreFlow = true; } - _ = Task.Run(DrainRetiredGenerationsAsync); + m_backgroundWork.Run( + nameof(DrainRetiredGenerationsAsync), + async _ => await DrainRetiredGenerationsAsync().ConfigureAwait(false)); scheduled = true; } finally @@ -4019,6 +4025,8 @@ public void Dispose() private readonly Lock m_operationLifetimeLock = new(); private readonly Dictionary m_registrations = []; private readonly List m_retiredNodeManagers = []; + private readonly BackgroundTaskScope m_backgroundWork = + new(nameof(NodeManagerLifecycle), AmbientMessageContext.Telemetry); private TaskCompletionSource? m_operationsDrained; private int m_activeLifecycleOperations; private int m_activeShutdownMethods; diff --git a/src/Opc.Ua.Server/NodeManager/MonitoredItem/SamplingGroup.cs b/src/Opc.Ua.Server/NodeManager/MonitoredItem/SamplingGroup.cs index 48eca7dec9..af86602db6 100644 --- a/src/Opc.Ua.Server/NodeManager/MonitoredItem/SamplingGroup.cs +++ b/src/Opc.Ua.Server/NodeManager/MonitoredItem/SamplingGroup.cs @@ -93,6 +93,7 @@ public SamplingGroup( ?? (server as ITimeProviderProvider)?.TimeProvider ?? TimeProvider.System; m_logger = server.Telemetry.CreateLogger(); + m_backgroundWork = new BackgroundTaskScope(nameof(SamplingGroup), server.Telemetry); m_nodeManager = nodeManager ?? throw new ArgumentNullException(nameof(nodeManager)); m_samplingRates = samplingRates ?? throw new ArgumentNullException(nameof(samplingRates)); @@ -132,6 +133,10 @@ protected virtual void Dispose(bool disposing) { if (disposing) { + // Signal only: Dispose is synchronous. An in-flight sample + // stops at its next await once the token trips. + m_backgroundWork.Dispose(); + lock (m_lock) { m_shutdownEvent.Set(); @@ -290,8 +295,9 @@ public bool ApplyChanges() // collect first sample. if (itemsToSample.Count > 0) { - _ = Task.Run( - async () => await DoSampleAsync(itemsToSample, CancellationToken.None) + m_backgroundWork.Run( + nameof(DoSampleAsync), + async ct => await DoSampleAsync(itemsToSample, ct) .ConfigureAwait(false)); } @@ -554,6 +560,7 @@ private async ValueTask DoSampleAsync(List item private readonly Lock m_lock = new(); private readonly ILogger m_logger; + private readonly BackgroundTaskScope m_backgroundWork; private readonly IServerInternal m_server; private readonly TimeProvider m_timeProvider; private readonly IAsyncNodeManager m_nodeManager; diff --git a/src/Opc.Ua.Server/Server/StandardServer.cs b/src/Opc.Ua.Server/Server/StandardServer.cs index 272639a439..8bb167739b 100644 --- a/src/Opc.Ua.Server/Server/StandardServer.cs +++ b/src/Opc.Ua.Server/Server/StandardServer.cs @@ -4242,7 +4242,9 @@ await shutdown.Server.SubscriptionManager { shutdown.Server.SessionManager.SessionChannelKeepAlive -= SessionChannelKeepAliveEvent; - shutdown.Server.SessionManager.Shutdown(); + await shutdown.Server.SessionManager + .ShutdownAsync(cancellationToken) + .ConfigureAwait(false); shutdown.SessionsStopped = true; } diff --git a/src/Opc.Ua.Server/Session/ISessionManager.cs b/src/Opc.Ua.Server/Session/ISessionManager.cs index 4053397cd6..deb7202211 100644 --- a/src/Opc.Ua.Server/Session/ISessionManager.cs +++ b/src/Opc.Ua.Server/Session/ISessionManager.cs @@ -88,9 +88,10 @@ public interface ISessionManager : IDisposable ValueTask StartupAsync(CancellationToken cancellationToken = default); /// - /// Stops the session manager and closes all sessions. + /// Stops the session manager and closes all sessions, waiting for the session + /// monitor loop to exit before returning. /// - void Shutdown(); + ValueTask ShutdownAsync(CancellationToken cancellationToken = default); /// /// Clears all tracked failed authentication attempts and lockouts. diff --git a/src/Opc.Ua.Server/Session/SessionManager.cs b/src/Opc.Ua.Server/Session/SessionManager.cs index c78429860e..f370209bc6 100644 --- a/src/Opc.Ua.Server/Session/SessionManager.cs +++ b/src/Opc.Ua.Server/Session/SessionManager.cs @@ -143,6 +143,9 @@ protected virtual void Dispose(bool disposing) m_shutdownEvent.Set(); m_shutdownEvent.Dispose(); m_semaphoreSlim.Dispose(); + m_workerCts?.Cancel(); + m_workerCts?.Dispose(); + m_workerCts = null; } } @@ -158,12 +161,12 @@ await m_semaphoreSlim.WaitAsync(cancellationToken) // start thread to monitor sessions. m_shutdownEvent.Reset(); - // TODO: Await the task completion in shutdown and pass cancellation token - _ = Task.Factory.StartNew( - () => MonitorSessionsAsync(m_minSessionTimeout), - default, - TaskCreationOptions.LongRunning | TaskCreationOptions.DenyChildAttach, - TaskScheduler.Default); + // Recreated on every startup: a token source cannot be reset once + // ShutdownAsync has cancelled it, and the manager supports restart. + m_workerCts?.Dispose(); + m_workerCts = new CancellationTokenSource(); + + m_monitorWorkerTask = StartSessionMonitor(m_workerCts.Token); } finally { @@ -172,13 +175,64 @@ await m_semaphoreSlim.WaitAsync(cancellationToken) } /// - /// Stops the session manager and closes all sessions. + /// Starts the session monitor loop and returns a task that completes when the + /// loop has actually exited. + /// + /// + /// The inner AsTask plus Unwrap matter: + /// hands back a task that completes as soon as the loop first yields, so + /// awaiting the raw result would only await the + /// scheduling of the loop and let shutdown race ahead of it. + /// + private Task StartSessionMonitor(CancellationToken cancellationToken) + { + return Task.Factory.StartNew( + static state => + { + (SessionManager manager, CancellationToken ct) = + ((SessionManager, CancellationToken))state!; + return manager + .MonitorSessionsAsync(manager.m_minSessionTimeout, ct) + .AsTask(); + }, + (this, cancellationToken), + CancellationToken.None, + TaskCreationOptions.LongRunning | TaskCreationOptions.DenyChildAttach, + TaskScheduler.Default) + .Unwrap(); + } + + /// + /// Stops the session manager and closes all sessions, waiting for the session + /// monitor loop to exit before returning. /// - public virtual void Shutdown() + public virtual async ValueTask ShutdownAsync(CancellationToken cancellationToken = default) { - // stop the monitoring thread. + // stop the monitoring loop. m_shutdownEvent.Set(); + // Cancel so the monitor's inter-cycle delay is abandoned immediately + // instead of running to the end of its sleep cycle. + m_workerCts?.Cancel(); + + Task? monitorWorkerTask = m_monitorWorkerTask; + if (monitorWorkerTask is not null) + { + await monitorWorkerTask.ConfigureAwait(false); + m_monitorWorkerTask = null; + } + + m_workerCts?.Dispose(); + m_workerCts = null; + + CloseAllSessions(); + } + + /// + /// Disposes every tracked session and empties the session table. + /// + private void CloseAllSessions() + { // dispose of session objects using a snapshot. KeyValuePair[] sessions = [.. m_sessions]; m_sessions.Clear(); @@ -1447,14 +1501,14 @@ protected virtual void RaiseSessionEvent(ISession session, SessionEventReason re /// /// Periodically checks if the sessions have timed out. /// - private async ValueTask MonitorSessionsAsync(object data) + private async ValueTask MonitorSessionsAsync( + int sleepCycle, + CancellationToken cancellationToken = default) { try { m_logger.ServerSessionMonitorThreadStarted(); - int sleepCycle = Convert.ToInt32(data, CultureInfo.InvariantCulture); - while (true) { // enumerator is thread safe @@ -1472,7 +1526,10 @@ private async ValueTask MonitorSessionsAsync(object data) // raise audit event for session closed because of timeout m_server.ReportAuditCloseSessionEvent(null!, session, m_logger, "Session/Timeout"); - await m_server.CloseSessionAsync(null!, session.Id, false) + // Deliberately not cancellable: a close already under way + // must finish so the session is torn down cleanly even when + // shutdown has cancelled the monitor loop. + await m_server.CloseSessionAsync(null!, session.Id, false, CancellationToken.None) .ConfigureAwait(false); } // if a session had no activity for the last m_minSessionTimeout milliseconds, send a keep alive event. @@ -1483,13 +1540,27 @@ await m_server.CloseSessionAsync(null!, session.Id, false) } } - if (m_shutdownEvent.WaitOne(sleepCycle)) + if (m_shutdownEvent.WaitOne(0)) { m_logger.ServerSessionMonitorThreadExitedNormally(); break; } + + // Asynchronous so the sleep does not park a thread-pool thread for + // the whole cycle, and so shutdown can abandon it immediately. + await m_timeProvider + .Delay(TimeSpan.FromMilliseconds(sleepCycle), cancellationToken) + .ConfigureAwait(false); } } + catch (ObjectDisposedException) + { + m_logger.ServerSessionMonitorThreadExitedNormally(); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + m_logger.ServerSessionMonitorThreadExitedNormally(); + } catch (Exception e) { m_logger.ServerSessionMonitorThreadExitedUnexpectedly(e); @@ -1505,6 +1576,8 @@ private readonly ConditionalWeakTable m_sessionActivationStates = new(); private uint m_lastSessionId; private readonly ManualResetEvent m_shutdownEvent; + private Task? m_monitorWorkerTask; + private CancellationTokenSource? m_workerCts; private readonly int m_minSessionTimeout; private readonly int m_maxSessionTimeout; diff --git a/src/Opc.Ua.Server/Subscription/SessionPublishQueue.cs b/src/Opc.Ua.Server/Subscription/SessionPublishQueue.cs index 929d387ab0..10c76d1f42 100644 --- a/src/Opc.Ua.Server/Subscription/SessionPublishQueue.cs +++ b/src/Opc.Ua.Server/Subscription/SessionPublishQueue.cs @@ -60,6 +60,9 @@ public SessionPublishQueue( { m_server = server ?? throw new ArgumentNullException(nameof(server)); m_logger = server.Telemetry.CreateLogger(); + m_backgroundWork = new BackgroundTaskScope( + nameof(SessionPublishQueue), + server.Telemetry); m_session = session ?? throw new ArgumentNullException(nameof(session)); m_queuedRequests = new LinkedList(); m_queuedSubscriptions = new ConcurrentDictionary(); @@ -86,6 +89,10 @@ protected virtual void Dispose(bool disposing) { if (disposing) { + // Signal only: Dispose is synchronous. A cleanup already running + // finishes deleting the subscriptions it captured. + m_backgroundWork.Dispose(); + lock (m_lock) { while (m_queuedRequests.Count > 0) @@ -675,7 +682,8 @@ internal void PublishTimerExpired(IReadOnlyList queuedSubscr } // schedule cleanup on a background thread. - SubscriptionManager.CleanupSubscriptions(m_server, subscriptionsToDelete, m_logger); + SubscriptionManager.CleanupSubscriptions( + m_server, subscriptionsToDelete, m_logger, m_backgroundWork); } /// @@ -1020,6 +1028,7 @@ internal void TraceState(string context, params object[] args) private readonly Lock m_lock = new(); private readonly ILogger m_logger; + private readonly BackgroundTaskScope m_backgroundWork; private readonly IServerInternal m_server; private readonly ISession m_session; private readonly LinkedList m_queuedRequests; diff --git a/src/Opc.Ua.Server/Subscription/SubscriptionManager.cs b/src/Opc.Ua.Server/Subscription/SubscriptionManager.cs index 988b94deda..c931088d6a 100644 --- a/src/Opc.Ua.Server/Subscription/SubscriptionManager.cs +++ b/src/Opc.Ua.Server/Subscription/SubscriptionManager.cs @@ -101,6 +101,10 @@ public SubscriptionManager( // create a event to signal shutdown. m_shutdownEvent = new ManualResetEvent(true); + m_backgroundWork = new BackgroundTaskScope( + nameof(SubscriptionManager), + server.Telemetry); + // create queue and event for condition refresh worker m_conditionRefreshEvent = new ManualResetEvent(false); m_conditionRefreshQueue = new Queue(); @@ -129,6 +133,7 @@ protected virtual void Dispose(bool disposing) try { SignalConditionRefreshWorkerShutdown(); + m_workerCts?.Cancel(); publishQueues = [.. m_publishQueues.Values]; m_publishQueues.Clear(); @@ -152,9 +157,12 @@ protected virtual void Dispose(bool disposing) subscription?.Dispose(); } + m_backgroundWork.Dispose(); m_shutdownEvent.Dispose(); m_conditionRefreshEvent.Dispose(); m_semaphoreSlim.Dispose(); + m_workerCts?.Dispose(); + m_workerCts = null; } } @@ -255,14 +263,12 @@ await RestoreSubscriptionsAsync(cancellationToken) m_shutdownEvent.Reset(); - // TODO: Ensure shutdown awaits completion and a cancellation token is passed - _ = Task.Factory.StartNew( - () => PublishSubscriptionsAsync(m_publishingResolution), - default, - TaskCreationOptions.LongRunning | TaskCreationOptions.DenyChildAttach, - TaskScheduler.Default); + // Recreated on every startup: a token source cannot be reset once + // ShutdownAsync has cancelled it, and the manager supports restart. + m_workerCts?.Dispose(); + m_workerCts = new CancellationTokenSource(); - // TODO: Ensure shutdown awaits completion and a cancellation token is passed + m_publishWorkerTask = StartPublishWorker(m_workerCts.Token); m_conditionRefreshWorkerTask = StartConditionRefreshWorker(); } finally @@ -281,6 +287,18 @@ public virtual async ValueTask ShutdownAsync(CancellationToken cancellationToken { // stop the publishing thread and trigger the condition refresh thread. SignalConditionRefreshWorkerShutdown(); + + // Cancel so the publish loop's inter-cycle delay is abandoned + // immediately instead of running to the end of its resolution. + m_workerCts?.Cancel(); + + Task? publishWorkerTask = m_publishWorkerTask; + if (publishWorkerTask is not null) + { + await publishWorkerTask.ConfigureAwait(false); + m_publishWorkerTask = null; + } + Task? conditionRefreshWorkerTask = m_conditionRefreshWorkerTask; if (conditionRefreshWorkerTask is not null) { @@ -288,6 +306,14 @@ public virtual async ValueTask ShutdownAsync(CancellationToken cancellationToken m_conditionRefreshWorkerTask = null; } + m_workerCts?.Dispose(); + m_workerCts = null; + + // Expired-subscription cleanups scheduled by the publish sweep + // still delete subscriptions through the server, so drain them + // before the queues and subscriptions go away. + await m_backgroundWork.DisposeAsync().ConfigureAwait(false); + // dispose of publish queues. foreach (SessionPublishQueue queue in m_publishQueues.Values) { @@ -2476,7 +2502,7 @@ internal void ProcessAbandonedPublishTimers( m_logger.SubscriptionAbandonedSubscriptionIdSubscriptionId(subscription.Id); } - CleanupSubscriptions(m_server, subscriptionsToDelete, m_logger); + CleanupSubscriptions(m_server, subscriptionsToDelete, m_logger, m_backgroundWork); } /// @@ -2556,17 +2582,22 @@ await DoConditionRefresh2Async( /// The server. /// The subscriptions to delete. /// A contextual logger to log to + /// Owns the deletion so it is drained + /// before the caller that scheduled it goes away. internal static void CleanupSubscriptions( IServerInternal server, IList subscriptionsToDelete, - ILogger logger) + ILogger logger, + BackgroundTaskScope backgroundWork) { if (subscriptionsToDelete != null && subscriptionsToDelete.Count > 0) { logger.ServerCountSubscriptionsScheduledForDelete(subscriptionsToDelete.Count); - _ = Task.Run( - () => CleanupSubscriptionsCoreAsync(server, subscriptionsToDelete, logger)); + backgroundWork.Run( + nameof(CleanupSubscriptionsCoreAsync), + async ct => await CleanupSubscriptionsCoreAsync( + server, subscriptionsToDelete, logger, ct).ConfigureAwait(false)); } } @@ -2665,6 +2696,9 @@ public override int GetHashCode() private readonly ManualResetEvent m_conditionRefreshEvent; private readonly ISubscriptionStore m_subscriptionStore; private Task? m_conditionRefreshWorkerTask; + private readonly BackgroundTaskScope m_backgroundWork; + private Task? m_publishWorkerTask; + private CancellationTokenSource? m_workerCts; private readonly Lock m_statusMessagesLock = new(); private readonly Lock m_eventLock = new(); @@ -2684,6 +2718,34 @@ private Task StartConditionRefreshWorker() .Unwrap(); } + /// + /// Starts the publish timer loop and returns a task that completes when the + /// loop has actually exited. + /// + /// + /// The inner AsTask plus Unwrap matter: + /// hands back a task that completes as soon as the loop first yields, so + /// awaiting the raw result would only await the + /// scheduling of the loop and let shutdown race ahead of it. + /// + private Task StartPublishWorker(CancellationToken cancellationToken) + { + return Task.Factory.StartNew( + static state => + { + (SubscriptionManager manager, CancellationToken ct) = + ((SubscriptionManager, CancellationToken))state!; + return manager + .PublishSubscriptionsAsync(manager.m_publishingResolution, ct) + .AsTask(); + }, + (this, cancellationToken), + CancellationToken.None, + TaskCreationOptions.LongRunning | TaskCreationOptions.DenyChildAttach, + TaskScheduler.Default) + .Unwrap(); + } + private void SignalConditionRefreshWorkerShutdown() { lock (m_conditionRefreshLock) diff --git a/src/Opc.Ua.Types/EventIds.cs b/src/Opc.Ua.Types/EventIds.cs index 4f36ac37ba..5508e35c0a 100644 --- a/src/Opc.Ua.Types/EventIds.cs +++ b/src/Opc.Ua.Types/EventIds.cs @@ -47,5 +47,6 @@ internal static class TypesEventIds public const int LocalizedText = 40; public const int Matrix = 50; public const int XmlSchemaValidator = 60; + public const int BackgroundTaskScope = 70; } } diff --git a/src/Opc.Ua.Types/Utils/BackgroundTaskScope.cs b/src/Opc.Ua.Types/Utils/BackgroundTaskScope.cs new file mode 100644 index 0000000000..54e9da0d0a --- /dev/null +++ b/src/Opc.Ua.Types/Utils/BackgroundTaskScope.cs @@ -0,0 +1,304 @@ +/* ======================================================================== + * Copyright (c) 2005-2025 The OPC Foundation, Inc. All rights reserved. + * + * OPC Foundation MIT License 1.00 + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation + * files (the "Software"), to deal in the Software without + * restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following + * conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + * The complete license agreement can be found here: + * http://opcfoundation.org/License/MIT/1.00/ + * ======================================================================*/ + +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; + +namespace Opc.Ua +{ + /// + /// Owns background work that a component starts but cannot await inline, + /// so that the work is bounded, its failures are observed, and it is + /// finished before the component that started it goes away. + /// + /// + /// + /// The pattern this replaces is a bare _ = Task.Run(...). That + /// hands the work to the thread pool and immediately forgets it: nothing + /// observes the exception if it throws, nothing bounds how many run at + /// once, and — most damaging — disposal races it. A component can finish + /// tearing itself down while work it started is still touching its + /// fields, which surfaces as from + /// unrelated places, or as a test host that will not exit. + /// + /// + /// never blocks and never throws, so it is safe to call + /// from inside a lock — which is exactly why most of these call sites + /// wanted a background task in the first place. + /// cancels and then waits for the work still + /// in flight, bounded by a drain timeout so one wedged operation cannot + /// hang shutdown for ever. + /// + /// + public sealed class BackgroundTaskScope : IAsyncDisposable, IDisposable + { + /// + /// Creates a scope. + /// + /// Name of the component that owns the work, used + /// in log messages to identify where a failure came from. + /// Telemetry used to report work that + /// faulted. Failures are swallowed when this is null. + /// Maximum number of scheduled + /// operations allowed to run at once, or zero for no limit. Work over + /// the limit waits asynchronously, so it occupies no thread. + /// How long + /// waits for work in flight. Defaults to thirty seconds. + /// + /// is negative. + public BackgroundTaskScope( + string owner, + ITelemetryContext? telemetry = null, + int maxConcurrency = 0, + TimeSpan? drainTimeout = null) + { + if (maxConcurrency < 0) + { + throw new ArgumentOutOfRangeException(nameof(maxConcurrency)); + } + + m_owner = owner ?? throw new ArgumentNullException(nameof(owner)); + m_logger = telemetry?.CreateLogger(); + m_slots = maxConcurrency > 0 + ? new SemaphoreSlim(maxConcurrency, maxConcurrency) + : null; + m_drainTimeout = drainTimeout ?? TimeSpan.FromSeconds(30); + } + + /// + /// Cancelled when the scope starts shutting down. Scheduled work + /// receives this token and should stop promptly once it is signalled. + /// + public CancellationToken ShutdownToken => m_cts.Token; + + /// + /// Number of scheduled operations that have not finished yet. + /// + public int PendingCount => Volatile.Read(ref m_pending); + + /// + /// Schedules to run in the background. + /// + /// Short name of the operation, used in log + /// messages when it faults. + /// The work to run. + /// true when the work was scheduled; false + /// when the scope is already shutting down, in which case the work is + /// not run at all. + /// + /// Never blocks and never throws, so it is safe to call while holding + /// a lock. + /// + /// is + /// null. + public bool Run(string operation, Func work) + { + if (work == null) + { + throw new ArgumentNullException(nameof(work)); + } + + if (Volatile.Read(ref m_shuttingDown) != 0) + { + return false; + } + + Interlocked.Increment(ref m_pending); + + // Re-check after the increment. DisposeAsync sets the flag and then + // reads the counter, so this ordering guarantees that either it sees + // this operation and waits for it, or this sees the shutdown and + // stands down. Without the re-check an operation could be scheduled + // after the drain had already decided the scope was empty. + if (Volatile.Read(ref m_shuttingDown) != 0) + { + CompleteOne(); + return false; + } + + _ = Task.Run(() => RunCoreAsync(operation, work), CancellationToken.None); + return true; + } + + /// + /// Signals shutdown without waiting for the work in flight. + /// + /// + /// For owners whose teardown is synchronous and therefore cannot await + /// the drain — blocking on it would be sync over async. Scheduled work + /// is cancelled and no further work is accepted, but this returns + /// immediately and work already running may still be in flight when it + /// does. Prefer wherever the owner has an + /// asynchronous teardown to hang the drain on. + /// + public void Dispose() + { + if (Interlocked.Exchange(ref m_shuttingDown, 1) != 0) + { + return; + } + + CancelShutdownToken(); + + if (Volatile.Read(ref m_pending) == 0) + { + m_drained.TrySetResult(true); + } + } + + /// + /// Signals shutdown and waits for the work still in flight. + /// + public async ValueTask DisposeAsync() + { + if (Interlocked.Exchange(ref m_shuttingDown, 1) == 0) + { + CancelShutdownToken(); + + if (Volatile.Read(ref m_pending) == 0) + { + m_drained.TrySetResult(true); + } + } + + try + { + await m_drained.Task.WaitAsync(m_drainTimeout).ConfigureAwait(false); + } + catch (TimeoutException) + { + m_logger?.BackgroundTaskDrainTimedOut( + m_owner, + Volatile.Read(ref m_pending), + m_drainTimeout.TotalSeconds); + } + + if (Interlocked.Exchange(ref m_disposed, 1) == 0) + { + m_cts.Dispose(); + m_slots?.Dispose(); + } + } + + private void CancelShutdownToken() + { + try + { + m_cts.Cancel(); + } + catch (AggregateException ex) + { + // A callback registered on the token threw. It is not this + // scope's failure to propagate, but it must not prevent the drain. + m_logger?.BackgroundTaskCancellationFailed(ex, m_owner); + } + } + + private async Task RunCoreAsync(string operation, Func work) + { + try + { + CancellationToken ct = m_cts.Token; + if (m_slots != null) + { + await m_slots.WaitAsync(ct).ConfigureAwait(false); + } + + try + { + await work(ct).ConfigureAwait(false); + } + finally + { + m_slots?.Release(); + } + } + catch (OperationCanceledException) + { + // Shutdown, or the work honoured the token. Either way expected. + } + catch (ObjectDisposedException) + { + // The owner was torn down under the work. Expected during shutdown. + } + catch (Exception ex) + { + m_logger?.BackgroundTaskFailed(ex, m_owner, operation); + } + finally + { + CompleteOne(); + } + } + + private void CompleteOne() + { + if (Interlocked.Decrement(ref m_pending) == 0 && + Volatile.Read(ref m_shuttingDown) != 0) + { + m_drained.TrySetResult(true); + } + } + + private readonly string m_owner; + private readonly ILogger? m_logger; + private readonly SemaphoreSlim? m_slots; + private readonly TimeSpan m_drainTimeout; + private readonly CancellationTokenSource m_cts = new(); + private readonly TaskCompletionSource m_drained = + new(TaskCreationOptions.RunContinuationsAsynchronously); + private int m_pending; + private int m_shuttingDown; + private int m_disposed; + } + + /// + /// Source-generated log messages for . + /// + internal static partial class BackgroundTaskScopeLog + { + [LoggerMessage(EventId = TypesEventIds.BackgroundTaskScope + 0, Level = LogLevel.Error, + Message = "Background operation {Operation} started by {Owner} failed.")] + public static partial void BackgroundTaskFailed( + this ILogger logger, Exception exception, string owner, string operation); + + [LoggerMessage(EventId = TypesEventIds.BackgroundTaskScope + 1, Level = LogLevel.Warning, + Message = "Background work started by {Owner} did not drain within {TimeoutSeconds}s; " + + "{Pending} operation(s) still in flight.")] + public static partial void BackgroundTaskDrainTimedOut( + this ILogger logger, string owner, int pending, double timeoutSeconds); + + [LoggerMessage(EventId = TypesEventIds.BackgroundTaskScope + 2, Level = LogLevel.Warning, + Message = "A cancellation callback threw while shutting down background work of {Owner}.")] + public static partial void BackgroundTaskCancellationFailed( + this ILogger logger, Exception exception, string owner); + } +} diff --git a/tests/Opc.Ua.Client.Tests/Stack/Client/ClientChannelManagerManagedTests.cs b/tests/Opc.Ua.Client.Tests/Stack/Client/ClientChannelManagerManagedTests.cs index 76561e031f..5c5fadb1e6 100644 --- a/tests/Opc.Ua.Client.Tests/Stack/Client/ClientChannelManagerManagedTests.cs +++ b/tests/Opc.Ua.Client.Tests/Stack/Client/ClientChannelManagerManagedTests.cs @@ -69,6 +69,23 @@ public sealed class ClientChannelManagerManagedTests { private static readonly ICertificateFactory s_factory = DefaultCertificateFactory.Instance; + /// + /// How long a test waits for an asynchronous operation it has already + /// unblocked to be observed as complete. + /// + /// + /// This is a hang detector, not a latency assertion. These tests drive a + /// fake clock, so the work under test finishes in microseconds; what is + /// being waited on is purely the thread-pool scheduling of the + /// continuation chain. On a saturated CI agent the pool injects threads + /// at roughly one per second, so a short budget times out while the + /// operation is merely queued - the channel has already faulted and its + /// completion source has already been signalled. Keep this generous + /// enough that it never fires on a healthy run, and far below the + /// blame-hang timeout so a genuine deadlock still fails the job quickly. + /// + private static readonly TimeSpan s_completionTimeout = TimeSpan.FromSeconds(60); + [Test] public void ChannelKeyEqualityIsValueBased() { @@ -789,7 +806,7 @@ public async Task MetricsAreEmittedForReconnectAndGateWaitAsync() })); Task reconnectTask = sut.ReconnectAsync(ch, default).AsTask(); - await reconnectEntered.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + await reconnectEntered.Task.WaitAsync(s_completionTimeout).ConfigureAwait(false); Task sendTask = ch.SendRequestAsync( new ReadRequest { RequestHeader = new RequestHeader() }, default).AsTask(); @@ -922,10 +939,11 @@ public async Task SendRequestDoesNotRetryNonIdempotentRequestOnTransientDropAsyn StatusCodes.BadConnectionClosed, "simulated in-flight drop"); }); - ServiceResultException? ex = Assert.ThrowsAsync(async () => - await ch.SendRequestAsync( + ServiceResultException ex = await AssertThrowsAsync( + ch.SendRequestAsync( new WriteRequest { RequestHeader = new RequestHeader() }, - default).AsTask().ConfigureAwait(false)); + default).AsTask(), + TimeSpan.FromSeconds(30)).ConfigureAwait(false); Assert.That(ex, Is.Not.Null); Assert.That(ex!.StatusCode, Is.EqualTo(StatusCodes.BadConnectionClosed)); @@ -978,10 +996,11 @@ public async Task SendRequestDoesNotRetryNonTransientErrorAsync() StatusCodes.BadNodeIdUnknown, "non-transient application error"); }); - ServiceResultException? ex = Assert.ThrowsAsync(async () => - await ch.SendRequestAsync( + ServiceResultException ex = await AssertThrowsAsync( + ch.SendRequestAsync( new ReadRequest { RequestHeader = new RequestHeader() }, - default).AsTask().ConfigureAwait(false)); + default).AsTask(), + TimeSpan.FromSeconds(30)).ConfigureAwait(false); Assert.That(ex, Is.Not.Null); Assert.That(ex!.StatusCode, Is.EqualTo(StatusCodes.BadNodeIdUnknown)); @@ -1249,8 +1268,11 @@ public async Task ReconnectAsyncWithBudgetStopsWhenExhaustedAsync() IManagedTransportChannel ch = await sut.GetAsync(participant, default).ConfigureAwait(false); var budget = new RetryBudget(TimeSpan.Zero, timeProvider); - ServiceResultException? ex = Assert.ThrowsAsync(async () => - await sut.ReconnectAsync(ch, budget, default).AsTask().ConfigureAwait(false)); + // Await rather than Assert.ThrowsAsync: see AssertThrowsAsync. + Task exhaustedReconnect = sut.ReconnectAsync(ch, budget, default).AsTask(); + ServiceResultException ex = await AssertThrowsAsync( + exhaustedReconnect, + s_completionTimeout).ConfigureAwait(false); Assert.That(ex, Is.Not.Null); Assert.That(ex!.StatusCode, Is.EqualTo(StatusCodes.BadSecureChannelClosed)); @@ -1288,8 +1310,15 @@ public async Task ReconnectAsyncSwapsFaultedLeaseEntryAsync() object originalEntry = GetLeaseEntry(ch); var exhaustedBudget = new RetryBudget(TimeSpan.Zero, timeProvider); - _ = Assert.ThrowsAsync(async () => - await sut.ReconnectAsync(ch, exhaustedBudget, default).AsTask().ConfigureAwait(false)); + // Await rather than Assert.ThrowsAsync: see AssertThrowsAsync. + // This test drives a fake clock from this thread, so blocking + // here would prevent the clock from ever moving again. + Task faultedReconnect = sut + .ReconnectAsync(ch, exhaustedBudget, default) + .AsTask(); + await AssertThrowsAsync( + faultedReconnect, + s_completionTimeout).ConfigureAwait(false); Assert.That(ch.State, Is.EqualTo(ChannelState.Faulted)); @@ -1304,7 +1333,7 @@ public async Task ReconnectAsyncSwapsFaultedLeaseEntryAsync() await Task.Delay(10).ConfigureAwait(false); } - await reconnectTask.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + await reconnectTask.WaitAsync(s_completionTimeout).ConfigureAwait(false); object freshEntry = GetLeaseEntry(ch); ManagedChannelDiagnostic diagnostic = sut.GetChannelDiagnostics() @@ -1355,18 +1384,20 @@ public async Task ReconnectAsyncWithBudgetShrinksDelayToFitRemainingAsync() } }; + // Arm before starting the reconnect so the waiter cannot be + // satisfied by an unrelated timer created earlier in the run. + Task shrunkBackoff = timeProvider.WaitForTimersCreatedAsync(); Task reconnectTask = sut.ReconnectAsync(ch, budget, default).AsTask(); - await reconnecting.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); - await timeProvider.WaitForTimerCreatedAsync(1) - .WaitAsync(TimeSpan.FromSeconds(5)) - .ConfigureAwait(false); + await reconnecting.Task.WaitAsync(s_completionTimeout).ConfigureAwait(false); + await shrunkBackoff.WaitAsync(s_completionTimeout).ConfigureAwait(false); Assert.That(reconnectTask.IsCompleted, Is.False); timeProvider.Advance(TimeSpan.FromMilliseconds(100)); - ServiceResultException? ex = Assert.ThrowsAsync(async () => - await reconnectTask.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false)); + ServiceResultException ex = await AssertThrowsAsync( + reconnectTask, + s_completionTimeout).ConfigureAwait(false); Assert.That(ex, Is.Not.Null); Assert.That(ex!.StatusCode, Is.EqualTo(StatusCodes.BadSecureChannelClosed)); @@ -1384,7 +1415,92 @@ await timeProvider.WaitForTimerCreatedAsync(1) } } - // ---- helpers ---- + [Test] + public async Task ReconnectAsyncWithExhaustedPolicyDoesNotSwapTheEntryAsync() + { + var timeProvider = new ObservableFakeTimeProvider(); + var reconnectPolicy = new ExponentialBackoffChannelReconnectPolicy + { + MinDelay = TimeSpan.FromMilliseconds(10), + MaxDelay = TimeSpan.FromMilliseconds(10), + MaxAttempts = 0 + }; + (ClientChannelManager sut, Certificate serverCert, Mock chMock) = + CreateMockedSut(reconnectPolicy: reconnectPolicy, timeProvider: timeProvider); + try + { + ConfiguredEndpoint endpoint = GetTestEndpoint(serverCert); + var participant = new TestParticipant("p1", endpoint); + IManagedTransportChannel ch = await sut.GetAsync(participant, default).ConfigureAwait(false); + + // Deliberately generous. The policy, not the budget, is what ends this + // cycle: GetDelay returns the infinite sentinel as soon as the attempt + // count reaches MaxAttempts, before the budget is ever consulted. A + // race check that only asked whether the budget still had room would + // see plenty here, mistake the deliberate stop for a lost race against + // a concurrent close, and swap the entry to run a second, unbudgeted + // reconnect cycle behind the swap back-off. + var budget = new RetryBudget(TimeSpan.FromMinutes(1), timeProvider); + + ServiceResultException ex = await AssertThrowsAsync( + sut.ReconnectAsync(ch, budget, default).AsTask(), + s_completionTimeout).ConfigureAwait(false); + + Assert.That(ex.StatusCode, Is.EqualTo(StatusCodes.BadSecureChannelClosed)); + Assert.That(ch.State, Is.EqualTo(ChannelState.Faulted)); + Assert.That(GetInternalIntProperty(ch, "SwapCount"), Is.Zero); + chMock.Verify(c => c.ReconnectAsync( + It.IsAny(), + It.IsAny()), + Times.Never); + ch.Dispose(); + } + finally + { + await sut.DisposeAsync().ConfigureAwait(false); + serverCert.Dispose(); + } + } + + /// + /// Awaits and returns the exception it faulted + /// with, failing the test if it succeeded or threw something else. + /// + /// + /// Deliberately not Assert.ThrowsAsync. That blocks the calling + /// thread until the task completes - sync over async - so on a + /// constrained CI agent it can starve the very continuation it is + /// waiting for, and it has no timeout, so the block is unbounded. Tests + /// that drive a fake clock from the test thread cannot survive either: + /// the clock cannot advance while the thread is blocked, NUnit's runner + /// thread never returns, and the whole test host hangs until the blame + /// collector kills it. + /// + private static async Task AssertThrowsAsync( + Task task, + TimeSpan timeout) + where TException : Exception + { + try + { + await task.WaitAsync(timeout).ConfigureAwait(false); + } + catch (TException expected) + { + return expected; + } + catch (Exception other) + { + Assert.Fail( + $"Expected {typeof(TException).Name} but got " + + $"{other.GetType().Name}: {other}"); + throw; + } + + Assert.Fail( + $"Expected {typeof(TException).Name} but the operation completed successfully."); + throw new InvalidOperationException("unreachable"); + } private static (ClientChannelManager sut, Certificate serverCert, Mock chMock) CreateMockedSut( ITelemetryContext? telemetry = null, @@ -1898,7 +2014,7 @@ public ChannelActivityListener() public async Task WaitForStoppedActivityAsync(string operationName) { Activity activity = await m_stoppedActivity.Task - .WaitAsync(TimeSpan.FromSeconds(5)) + .WaitAsync(s_completionTimeout) .ConfigureAwait(false); Assert.That(activity.OperationName, Is.EqualTo(operationName)); @@ -2069,6 +2185,21 @@ public ValueTask OnReconnectAsync( } } + /// + /// A that lets a test wait until the code + /// under test has actually registered its back-off timers. + /// + /// + /// Waiting is deliberately relative: a waiter is armed for "N + /// more timers from now" rather than for "the Nth timer of the run". + /// Absolute numbering is a race - anything else that happens to create a + /// timer on this provider first (an earlier reconnect in the same test, + /// or manager housekeeping) consumes the low numbers, the waiter then + /// completes before the timer the test cares about exists, and the + /// subsequent Advance fires nothing. The reconnect is left parked on a + /// fake clock that nobody will move again, which hangs the test - and, + /// because NUnit blocks the runner thread, the whole test host. + /// private sealed class ObservableFakeTimeProvider : FakeTimeProvider { public override ITimer CreateTimer( @@ -2078,32 +2209,55 @@ public override ITimer CreateTimer( TimeSpan period) { ITimer timer = base.CreateTimer(callback, state, dueTime, period); - int timerNumber = Interlocked.Increment(ref m_timerCount); - if (timerNumber == 1) + + List>? ready = null; + lock (m_lock) { - m_firstTimerCreated.TrySetResult(true); + m_timerCount++; + for (int i = m_waiters.Count - 1; i >= 0; i--) + { + if (m_timerCount >= m_waiters[i].Target) + { + (ready ??= []).Add(m_waiters[i].Completion); + m_waiters.RemoveAt(i); + } + } } - else if (timerNumber == 2) + + if (ready != null) { - m_secondTimerCreated.TrySetResult(true); + foreach (TaskCompletionSource completion in ready) + { + completion.TrySetResult(true); + } } + return timer; } - public Task WaitForTimerCreatedAsync(int timerNumber) + /// + /// Returns a task that completes once + /// further timers have been created, counted from this call. Arm it + /// before starting the operation whose timers are awaited. + /// + public Task WaitForTimersCreatedAsync(int count = 1) { - return timerNumber switch + if (count < 1) { - 1 => m_firstTimerCreated.Task, - 2 => m_secondTimerCreated.Task, - _ => throw new ArgumentOutOfRangeException(nameof(timerNumber)) - }; + throw new ArgumentOutOfRangeException(nameof(count)); + } + + var completion = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + lock (m_lock) + { + m_waiters.Add((m_timerCount + count, completion)); + } + return completion.Task; } - private readonly TaskCompletionSource m_firstTimerCreated = new( - TaskCreationOptions.RunContinuationsAsynchronously); - private readonly TaskCompletionSource m_secondTimerCreated = new( - TaskCreationOptions.RunContinuationsAsynchronously); + private readonly System.Threading.Lock m_lock = new(); + private readonly List<(int Target, TaskCompletionSource Completion)> m_waiters = []; private int m_timerCount; } } diff --git a/tests/Opc.Ua.Client.Tests/Subscription/CompositeMonitoredItemCollectionTests.cs b/tests/Opc.Ua.Client.Tests/Subscription/CompositeMonitoredItemCollectionTests.cs index 179332b477..323984fea5 100644 --- a/tests/Opc.Ua.Client.Tests/Subscription/CompositeMonitoredItemCollectionTests.cs +++ b/tests/Opc.Ua.Client.Tests/Subscription/CompositeMonitoredItemCollectionTests.cs @@ -73,14 +73,14 @@ public void ConstructorThrowsOnEmptyPartitionList() { Assert.That(() => new CompositeMonitoredItemCollection( [], - new object()), + new Lock()), Throws.TypeOf()); } [Test] public void ConstructorThrowsOnNullArguments() { - Assert.That(() => new CompositeMonitoredItemCollection(null!, new object()), + Assert.That(() => new CompositeMonitoredItemCollection(null!, new Lock()), Throws.TypeOf()); Assert.That(() => new CompositeMonitoredItemCollection( [NewFake(1)], null!), @@ -98,7 +98,7 @@ public void FastPathDelegatesAllOperationsToPrimary() var composite = new CompositeMonitoredItemCollection( [primary], - new object()); + new Lock()); _ = composite.Count; _ = composite.Items; @@ -133,7 +133,7 @@ public void TryAddPlacesItemInPrimaryWhenCapacityAvailable() int factoryInvocations = 0; var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => { @@ -162,7 +162,7 @@ public void TryAddMintsNewPartitionWhenPrimaryAtCap() int factoryInvocations = 0; var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => { @@ -193,7 +193,7 @@ public void TryAddRejectsDuplicateNameAcrossPartitions() var policy = new PartitionPlacementPolicy(1); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary); @@ -222,7 +222,7 @@ public void TryAddRespectsStrictAffinity() var policy = new PartitionPlacementPolicy(10); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary); @@ -232,7 +232,7 @@ public void TryAddRespectsStrictAffinity() var pinnedPolicy = new PartitionPlacementPolicy(1); var pinnedComposite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), pinnedPolicy, () => secondary); @@ -267,7 +267,7 @@ public void TryRemoveLooksUpOwningPartitionAndDelegates() var policy = new PartitionPlacementPolicy(1); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary); @@ -294,7 +294,7 @@ public void TryGetByNameReturnsItemAcrossPartitions() var policy = new PartitionPlacementPolicy(1); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary); @@ -319,7 +319,7 @@ public void TryGetByClientHandleReturnsItemAcrossPartitions() var policy = new PartitionPlacementPolicy(1); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary); @@ -341,7 +341,7 @@ public void ItemsEnumeratesAcrossAllPartitions() var policy = new PartitionPlacementPolicy(1); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary); @@ -362,7 +362,7 @@ public void TryRequeueRoutesToOwningPartition() var policy = new PartitionPlacementPolicy(1); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary); Assert.That(composite.TryAdd("a", MakeOptions(new V2Options()), diff --git a/tests/Opc.Ua.Client.Tests/Subscription/ReactiveFallbackTests.cs b/tests/Opc.Ua.Client.Tests/Subscription/ReactiveFallbackTests.cs index 13a4fee684..0c7c5ceac3 100644 --- a/tests/Opc.Ua.Client.Tests/Subscription/ReactiveFallbackTests.cs +++ b/tests/Opc.Ua.Client.Tests/Subscription/ReactiveFallbackTests.cs @@ -29,6 +29,7 @@ using System; using System.Collections.Generic; +using System.Threading; using NUnit.Framework; using Opc.Ua.Client.Subscriptions.Fakes; @@ -63,7 +64,7 @@ public void OnPartitionCapReachedMarksPartitionNoGrow() var policy = new PartitionPlacementPolicy(uint.MaxValue); int factoryCalls = 0; var partitions = new List { primary }; - object lockObj = new(); + Lock lockObj = new(); var composite = new CompositeMonitoredItemCollection( partitions, lockObj, policy, () => @@ -102,7 +103,7 @@ public void OnPartitionCapReachedIsNoopForSinglePartitionFastPath() FakeManagedSubscription primary = NewFake(1); var composite = new CompositeMonitoredItemCollection( [primary], - new object()); + new Lock()); Assert.DoesNotThrow(() => composite.OnPartitionCapReached(primary)); } @@ -114,7 +115,7 @@ public void OnPartitionCapReachedThrowsOnNull() var policy = new PartitionPlacementPolicy(uint.MaxValue); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => NewFake(2)); diff --git a/tests/Opc.Ua.Client.Tests/Subscription/SecondaryPartitionIdleDeleteTests.cs b/tests/Opc.Ua.Client.Tests/Subscription/SecondaryPartitionIdleDeleteTests.cs index 192c25d37c..117820243a 100644 --- a/tests/Opc.Ua.Client.Tests/Subscription/SecondaryPartitionIdleDeleteTests.cs +++ b/tests/Opc.Ua.Client.Tests/Subscription/SecondaryPartitionIdleDeleteTests.cs @@ -70,7 +70,7 @@ public async Task PrimaryPartitionIsNeverDisposedAsync() int disposeCalls = 0; var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => throw new InvalidOperationException("no secondary expected"), timeProvider, @@ -106,7 +106,7 @@ public async Task SecondaryPartitionDisposedAfterIdleTimeoutAsync() TaskCreationOptions.RunContinuationsAsynchronously); var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary, timeProvider, @@ -152,7 +152,7 @@ public async Task ReAddingItemBeforeTimeoutCancelsIdleDeleteAsync() int disposeCalls = 0; var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary, timeProvider, @@ -198,7 +198,7 @@ public void IdleDeleteDisabledByInfiniteTimeout() int disposeCalls = 0; var composite = new CompositeMonitoredItemCollection( [primary], - new object(), + new Lock(), policy, () => secondary, timeProvider, diff --git a/tests/Opc.Ua.Core.Tests/Stack/Client/ClientChannelManagerManagedTests.cs b/tests/Opc.Ua.Core.Tests/Stack/Client/ClientChannelManagerManagedTests.cs index 4b325a9078..deb0fe4a45 100644 --- a/tests/Opc.Ua.Core.Tests/Stack/Client/ClientChannelManagerManagedTests.cs +++ b/tests/Opc.Ua.Core.Tests/Stack/Client/ClientChannelManagerManagedTests.cs @@ -69,6 +69,23 @@ public sealed class ClientChannelManagerManagedTests { private static readonly ICertificateFactory s_factory = DefaultCertificateFactory.Instance; + /// + /// How long a test waits for an asynchronous operation it has already + /// unblocked to be observed as complete. + /// + /// + /// This is a hang detector, not a latency assertion. These tests drive a + /// fake clock, so the work under test finishes in microseconds; what is + /// being waited on is purely the thread-pool scheduling of the + /// continuation chain. On a saturated CI agent the pool injects threads + /// at roughly one per second, so a short budget times out while the + /// operation is merely queued - the channel has already faulted and its + /// completion source has already been signalled. Keep this generous + /// enough that it never fires on a healthy run, and far below the + /// blame-hang timeout so a genuine deadlock still fails the job quickly. + /// + private static readonly TimeSpan s_completionTimeout = TimeSpan.FromSeconds(60); + [Test] public void ChannelKeyEqualityIsValueBased() { @@ -691,7 +708,7 @@ public async Task MetricsAreEmittedForReconnectAndGateWaitAsync() })); Task reconnectTask = sut.ReconnectAsync(ch, default).AsTask(); - await reconnectEntered.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + await reconnectEntered.Task.WaitAsync(s_completionTimeout).ConfigureAwait(false); Task sendTask = ch.SendRequestAsync( new ReadRequest { RequestHeader = new RequestHeader() }, default).AsTask(); @@ -941,8 +958,11 @@ public async Task ReconnectAsyncWithBudgetStopsWhenExhaustedAsync() IManagedTransportChannel ch = await sut.GetAsync(participant, default).ConfigureAwait(false); var budget = new RetryBudget(TimeSpan.Zero, timeProvider); - ServiceResultException? ex = Assert.ThrowsAsync(async () => - await sut.ReconnectAsync(ch, budget, default).AsTask().ConfigureAwait(false)); + // Await rather than Assert.ThrowsAsync: see AssertThrowsAsync. + Task exhaustedReconnect = sut.ReconnectAsync(ch, budget, default).AsTask(); + ServiceResultException ex = await AssertThrowsAsync( + exhaustedReconnect, + s_completionTimeout).ConfigureAwait(false); Assert.That(ex, Is.Not.Null); Assert.That(ex!.StatusCode, Is.EqualTo(StatusCodes.BadSecureChannelClosed)); @@ -1011,30 +1031,38 @@ public async Task ReconnectAsyncSwapsFaultedLeaseEntryAsync() } }; - _ = Assert.ThrowsAsync(async () => - await sut.ReconnectAsync(ch, exhaustedBudget, default).AsTask().ConfigureAwait(false)); - await faulted.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + // Await rather than Assert.ThrowsAsync: see AssertThrowsAsync. + // This test drives a fake clock from this thread, so blocking + // here would prevent the clock from ever moving again. + Task faultedReconnect = sut + .ReconnectAsync(ch, exhaustedBudget, default) + .AsTask(); + await faulted.Task.WaitAsync(s_completionTimeout).ConfigureAwait(false); + await AssertThrowsAsync( + faultedReconnect, + s_completionTimeout).ConfigureAwait(false); Assert.That(ch.State, Is.EqualTo(ChannelState.Faulted)); + // Arm before starting the reconnect: the waiter counts timers + // created from here, so the earlier faulted reconnect above + // cannot consume the slot this test is waiting on. + Task swapBackoff = timeProvider.WaitForTimersCreatedAsync(); Task reconnectTask = sut.ReconnectAsync(ch, default).AsTask(); - await timeProvider.WaitForTimerCreatedAsync(1) - .WaitAsync(TimeSpan.FromSeconds(5)) - .ConfigureAwait(false); + await swapBackoff.WaitAsync(s_completionTimeout).ConfigureAwait(false); Assert.That(reconnectTask.IsCompleted, Is.False, "Swap back-off should delay the reset."); + Task retryBackoff = timeProvider.WaitForTimersCreatedAsync(); timeProvider.Advance(TimeSpan.FromMilliseconds(100)); - await reconnecting.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); - await timeProvider.WaitForTimerCreatedAsync(2) - .WaitAsync(TimeSpan.FromSeconds(5)) - .ConfigureAwait(false); + await reconnecting.Task.WaitAsync(s_completionTimeout).ConfigureAwait(false); + await retryBackoff.WaitAsync(s_completionTimeout).ConfigureAwait(false); Assert.That(reconnectTask.IsCompleted, Is.False, "Reconnect back-off should delay the retry."); timeProvider.Advance(TimeSpan.FromMilliseconds(100)); - await reconnectTask.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); - await ready.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); + await reconnectTask.WaitAsync(s_completionTimeout).ConfigureAwait(false); + await ready.Task.WaitAsync(s_completionTimeout).ConfigureAwait(false); object freshEntry = GetLeaseEntry(ch); ManagedChannelDiagnostic diagnostic = sut.GetChannelDiagnostics() @@ -1085,18 +1113,20 @@ public async Task ReconnectAsyncWithBudgetShrinksDelayToFitRemainingAsync() } }; + // Arm before starting the reconnect so the waiter cannot be + // satisfied by an unrelated timer created earlier in the run. + Task shrunkBackoff = timeProvider.WaitForTimersCreatedAsync(); Task reconnectTask = sut.ReconnectAsync(ch, budget, default).AsTask(); - await reconnecting.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false); - await timeProvider.WaitForTimerCreatedAsync(1) - .WaitAsync(TimeSpan.FromSeconds(5)) - .ConfigureAwait(false); + await reconnecting.Task.WaitAsync(s_completionTimeout).ConfigureAwait(false); + await shrunkBackoff.WaitAsync(s_completionTimeout).ConfigureAwait(false); Assert.That(reconnectTask.IsCompleted, Is.False); timeProvider.Advance(TimeSpan.FromMilliseconds(100)); - ServiceResultException? ex = Assert.ThrowsAsync(async () => - await reconnectTask.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false)); + ServiceResultException ex = await AssertThrowsAsync( + reconnectTask, + s_completionTimeout).ConfigureAwait(false); Assert.That(ex, Is.Not.Null); Assert.That(ex!.StatusCode, Is.EqualTo(StatusCodes.BadSecureChannelClosed)); @@ -1114,7 +1144,92 @@ await timeProvider.WaitForTimerCreatedAsync(1) } } - // ---- helpers ---- + [Test] + public async Task ReconnectAsyncWithExhaustedPolicyDoesNotSwapTheEntryAsync() + { + var timeProvider = new ObservableFakeTimeProvider(); + var reconnectPolicy = new ExponentialBackoffChannelReconnectPolicy + { + MinDelay = TimeSpan.FromMilliseconds(10), + MaxDelay = TimeSpan.FromMilliseconds(10), + MaxAttempts = 0 + }; + (ClientChannelManager sut, Certificate serverCert, Mock chMock) = + CreateMockedSut(reconnectPolicy: reconnectPolicy, timeProvider: timeProvider); + try + { + ConfiguredEndpoint endpoint = GetTestEndpoint(serverCert); + var participant = new TestParticipant("p1", endpoint); + IManagedTransportChannel ch = await sut.GetAsync(participant, default).ConfigureAwait(false); + + // Deliberately generous. The policy, not the budget, is what ends this + // cycle: GetDelay returns the infinite sentinel as soon as the attempt + // count reaches MaxAttempts, before the budget is ever consulted. A + // race check that only asked whether the budget still had room would + // see plenty here, mistake the deliberate stop for a lost race against + // a concurrent close, and swap the entry to run a second, unbudgeted + // reconnect cycle behind the swap back-off. + var budget = new RetryBudget(TimeSpan.FromMinutes(1), timeProvider); + + ServiceResultException ex = await AssertThrowsAsync( + sut.ReconnectAsync(ch, budget, default).AsTask(), + s_completionTimeout).ConfigureAwait(false); + + Assert.That(ex.StatusCode, Is.EqualTo(StatusCodes.BadSecureChannelClosed)); + Assert.That(ch.State, Is.EqualTo(ChannelState.Faulted)); + Assert.That(GetInternalIntProperty(ch, "SwapCount"), Is.Zero); + chMock.Verify(c => c.ReconnectAsync( + It.IsAny(), + It.IsAny()), + Times.Never); + ch.Dispose(); + } + finally + { + await sut.DisposeAsync().ConfigureAwait(false); + serverCert.Dispose(); + } + } + + /// + /// Awaits and returns the exception it faulted + /// with, failing the test if it succeeded or threw something else. + /// + /// + /// Deliberately not Assert.ThrowsAsync. That blocks the calling + /// thread until the task completes - sync over async - so on a + /// constrained CI agent it can starve the very continuation it is + /// waiting for, and it has no timeout, so the block is unbounded. These + /// tests drive a fake clock from the test thread, which makes both + /// failure modes fatal: the test cannot advance the clock while it is + /// blocked, NUnit's runner thread never returns, and the whole test host + /// hangs until the blame collector kills it. + /// + private static async Task AssertThrowsAsync( + Task task, + TimeSpan timeout) + where TException : Exception + { + try + { + await task.WaitAsync(timeout).ConfigureAwait(false); + } + catch (TException expected) + { + return expected; + } + catch (Exception other) + { + Assert.Fail( + $"Expected {typeof(TException).Name} but got " + + $"{other.GetType().Name}: {other}"); + throw; + } + + Assert.Fail( + $"Expected {typeof(TException).Name} but the operation completed successfully."); + throw new InvalidOperationException("unreachable"); + } private static (ClientChannelManager sut, Certificate serverCert, Mock chMock) CreateMockedSut( ITelemetryContext? telemetry = null, @@ -1273,7 +1388,7 @@ public ChannelActivityListener() public async Task WaitForStoppedActivityAsync(string operationName) { Activity activity = await m_stoppedActivity.Task - .WaitAsync(TimeSpan.FromSeconds(5)) + .WaitAsync(s_completionTimeout) .ConfigureAwait(false); Assert.That(activity.OperationName, Is.EqualTo(operationName)); @@ -1442,6 +1557,21 @@ public ValueTask OnReconnectAsync( } } + /// + /// A that lets a test wait until the code + /// under test has actually registered its back-off timers. + /// + /// + /// Waiting is deliberately relative: a waiter is armed for "N + /// more timers from now" rather than for "the Nth timer of the run". + /// Absolute numbering is a race - anything else that happens to create a + /// timer on this provider first (an earlier reconnect in the same test, + /// or manager housekeeping) consumes the low numbers, the waiter then + /// completes before the timer the test cares about exists, and the + /// subsequent Advance fires nothing. The reconnect is left parked on a + /// fake clock that nobody will move again, which hangs the test - and, + /// because NUnit blocks the runner thread, the whole test host. + /// private sealed class ObservableFakeTimeProvider : FakeTimeProvider { public override ITimer CreateTimer( @@ -1451,32 +1581,58 @@ public override ITimer CreateTimer( TimeSpan period) { ITimer timer = base.CreateTimer(callback, state, dueTime, period); - int timerNumber = Interlocked.Increment(ref m_timerCount); - if (timerNumber == 1) + + List>? ready = null; + lock (m_lock) { - m_firstTimerCreated.TrySetResult(true); + m_timerCount++; + for (int i = m_waiters.Count - 1; i >= 0; i--) + { + if (m_timerCount >= m_waiters[i].Target) + { + (ready ??= []).Add(m_waiters[i].Completion); + m_waiters.RemoveAt(i); + } + } } - else if (timerNumber == 2) + + // Completed outside the lock: the continuations run + // asynchronously, but there is no reason to hold the lock while + // handing them off. + if (ready != null) { - m_secondTimerCreated.TrySetResult(true); + foreach (TaskCompletionSource completion in ready) + { + completion.TrySetResult(true); + } } + return timer; } - public Task WaitForTimerCreatedAsync(int timerNumber) + /// + /// Returns a task that completes once + /// further timers have been created, counted from this call. Arm it + /// before starting the operation whose timers are awaited. + /// + public Task WaitForTimersCreatedAsync(int count = 1) { - return timerNumber switch + if (count < 1) { - 1 => m_firstTimerCreated.Task, - 2 => m_secondTimerCreated.Task, - _ => throw new ArgumentOutOfRangeException(nameof(timerNumber)) - }; + throw new ArgumentOutOfRangeException(nameof(count)); + } + + var completion = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + lock (m_lock) + { + m_waiters.Add((m_timerCount + count, completion)); + } + return completion.Task; } - private readonly TaskCompletionSource m_firstTimerCreated = new( - TaskCreationOptions.RunContinuationsAsynchronously); - private readonly TaskCompletionSource m_secondTimerCreated = new( - TaskCreationOptions.RunContinuationsAsynchronously); + private readonly System.Threading.Lock m_lock = new(); + private readonly List<(int Target, TaskCompletionSource Completion)> m_waiters = []; private int m_timerCount; } } diff --git a/tests/Opc.Ua.Core.Tests/Stack/State/NodeStateHandlerConcurrencyTests.cs b/tests/Opc.Ua.Core.Tests/Stack/State/NodeStateHandlerConcurrencyTests.cs index fa65f8b3b9..a3178bcc06 100644 --- a/tests/Opc.Ua.Core.Tests/Stack/State/NodeStateHandlerConcurrencyTests.cs +++ b/tests/Opc.Ua.Core.Tests/Stack/State/NodeStateHandlerConcurrencyTests.cs @@ -667,36 +667,77 @@ private static void ExecuteNodeHandlerConcurrencyTest( } bool running = true; - - var thread = new Thread(() => + Exception workerError = null; + + // The worker is a raw thread, so three things have to hold or it can + // take the whole test host down with it: + // * IsBackground, so a worker that somehow outlives this method + // cannot pin the process at exit. A foreground thread here is + // exactly the shape of an "all tests passed, then the host never + // exits" hang. + // * Volatile access to the stop flag, so the worker is guaranteed + // to observe the request rather than spinning on a cached read. + // * A catch, because an unhandled exception on a raw thread + // terminates the process and takes every remaining test with it. + // Capture it and surface it on the test thread instead. + var worker = new Thread(() => { - while (running) + try { - concurrentTaskAction(node); + while (Volatile.Read(ref running)) + { + concurrentTaskAction(node); + } } - }); - - thread.Start(); + catch (Exception ex) + { + workerError = ex; + } + }) + { + IsBackground = true, + Name = "NodeStateHandlerConcurrency.Worker" + }; - DateTime utcNow = DateTime.UtcNow; + worker.Start(); - while (DateTime.UtcNow - utcNow < TimeSpan.FromSeconds(1)) + try { - ServiceResult writeResult = node.WriteAttribute( - systemContext, - attribute, - default, - new DataValue(variant)); + DateTime utcNow = DateTime.UtcNow; - Assert.That( - ServiceResult.IsGood(writeResult), - Is.True, - $"Expected Good ServiceResult but was: {writeResult}"); + while (DateTime.UtcNow - utcNow < TimeSpan.FromSeconds(1)) + { + ServiceResult writeResult = node.WriteAttribute( + systemContext, + attribute, + default, + new DataValue(variant)); + + Assert.That( + ServiceResult.IsGood(writeResult), + Is.True, + $"Expected Good ServiceResult but was: {writeResult}"); + } + } + finally + { + // Stop the worker even when the assertion above throws; + // otherwise a single failed assertion leaks a spinning thread + // for the rest of the run. + Volatile.Write(ref running, false); + if (!worker.Join(TimeSpan.FromSeconds(30))) + { + // Bounded so a stuck worker cannot hang the run. It is a + // background thread, so leaving it behind is survivable. + TestContext.Progress.WriteLine( + "NodeStateHandlerConcurrency worker did not stop within 30 seconds."); + } } - running = false; - - thread.Join(); + if (workerError != null) + { + Assert.Fail($"The concurrent writer threw: {workerError}"); + } } private static ServiceResult ValueChangedHandler( diff --git a/tests/Opc.Ua.Types.Tests/Utils/BackgroundTaskScopeTests.cs b/tests/Opc.Ua.Types.Tests/Utils/BackgroundTaskScopeTests.cs new file mode 100644 index 0000000000..1759268dd4 --- /dev/null +++ b/tests/Opc.Ua.Types.Tests/Utils/BackgroundTaskScopeTests.cs @@ -0,0 +1,247 @@ +/* ======================================================================== + * Copyright (c) 2005-2025 The OPC Foundation, Inc. All rights reserved. + * + * OPC Foundation MIT License 1.00 + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation + * files (the "Software"), to deal in the Software without + * restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following + * conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + * The complete license agreement can be found here: + * http://opcfoundation.org/License/MIT/1.00/ + * ======================================================================*/ + +using System; +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; + +namespace Opc.Ua.Types.Tests.Utils +{ + /// + /// Tests for . + /// + [TestFixture] + [Category("Utils")] + [SetCulture("en-us")] + [SetUICulture("en-us")] + [Parallelizable] + public class BackgroundTaskScopeTests + { + private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(30); + + [Test] + public async Task RunExecutesTheScheduledWorkAsync() + { + await using var sut = new BackgroundTaskScope("test"); + var ran = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + Assert.That(sut.Run("op", _ => + { + ran.TrySetResult(true); + return default; + }), Is.True); + + Assert.That(await ran.Task.WaitAsync(s_timeout).ConfigureAwait(false), Is.True); + } + + [Test] + public async Task DisposeAsyncWaitsForWorkStillInFlightAsync() + { + var release = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var started = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var finished = false; + + var sut = new BackgroundTaskScope("test"); + Assert.That(sut.Run("op", async _ => + { + started.TrySetResult(true); + await release.Task.ConfigureAwait(false); + finished = true; + }), Is.True); + + await started.Task.WaitAsync(s_timeout).ConfigureAwait(false); + + ValueTask dispose = sut.DisposeAsync(); + Assert.That(dispose.IsCompleted, Is.False, "Disposal must wait for work in flight."); + + release.TrySetResult(true); + await dispose.ConfigureAwait(false); + + Assert.That(finished, Is.True); + Assert.That(sut.PendingCount, Is.Zero); + } + + [Test] + public async Task DisposeAsyncCancelsTheShutdownTokenAsync() + { + var sut = new BackgroundTaskScope("test"); + CancellationToken token = sut.ShutdownToken; + Assert.That(token.IsCancellationRequested, Is.False); + + await sut.DisposeAsync().ConfigureAwait(false); + + Assert.That(token.IsCancellationRequested, Is.True); + } + + [Test] + public async Task RunAfterShutdownIsRejectedAndDoesNotExecuteAsync() + { + var sut = new BackgroundTaskScope("test"); + await sut.DisposeAsync().ConfigureAwait(false); + + var ran = false; + Assert.That(sut.Run("op", _ => + { + ran = true; + return default; + }), Is.False); + + Assert.That(ran, Is.False); + Assert.That(sut.PendingCount, Is.Zero); + } + + [Test] + public async Task WorkThatThrowsIsObservedAndDoesNotBreakTheScopeAsync() + { + await using var sut = new BackgroundTaskScope("test"); + var second = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + Assert.That(sut.Run("boom", _ => throw new InvalidOperationException("boom")), Is.True); + Assert.That(sut.Run("op", _ => + { + second.TrySetResult(true); + return default; + }), Is.True); + + // The scope keeps working, and the faulted operation is not rethrown + // anywhere the caller could see it. + Assert.That(await second.Task.WaitAsync(s_timeout).ConfigureAwait(false), Is.True); + } + + [Test] + public async Task DisposeAsyncIsIdempotentAsync() + { + var sut = new BackgroundTaskScope("test"); + + await sut.DisposeAsync().ConfigureAwait(false); + await sut.DisposeAsync().ConfigureAwait(false); + + Assert.That(sut.PendingCount, Is.Zero); + } + + [Test] + public async Task MaxConcurrencyBoundsTheOperationsRunningAtOnceAsync() + { + const int maxConcurrency = 2; + const int scheduled = 8; + + await using var sut = new BackgroundTaskScope("test", maxConcurrency: maxConcurrency); + var release = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var allStarted = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + int running = 0; + int peak = 0; + int completed = 0; + + for (int i = 0; i < scheduled; i++) + { + Assert.That(sut.Run("op", async _ => + { + int current = Interlocked.Increment(ref running); + int observed = Volatile.Read(ref peak); + while (current > observed && + Interlocked.CompareExchange(ref peak, current, observed) != observed) + { + observed = Volatile.Read(ref peak); + } + + await release.Task.ConfigureAwait(false); + Interlocked.Decrement(ref running); + if (Interlocked.Increment(ref completed) == scheduled) + { + allStarted.TrySetResult(true); + } + }), Is.True); + } + + // Nothing can finish until released, so whatever is running now is + // everything the limit allows to run concurrently. + release.TrySetResult(true); + await allStarted.Task.WaitAsync(s_timeout).ConfigureAwait(false); + + Assert.That(Volatile.Read(ref peak), Is.LessThanOrEqualTo(maxConcurrency)); + Assert.That(Volatile.Read(ref completed), Is.EqualTo(scheduled)); + } + + [Test] + public async Task RunWithNullWorkThrowsArgumentNullExceptionAsync() + { + await using var sut = new BackgroundTaskScope("test"); + Assert.That(() => sut.Run("op", null!), Throws.ArgumentNullException); + } + + [Test] + public void ConstructorRejectsNegativeConcurrency() + { + Assert.That( + () => new BackgroundTaskScope("test", maxConcurrency: -1), + Throws.TypeOf()); + } + + [Test] + public async Task SynchronousDisposeStopsAcceptingWorkWithoutWaitingAsync() + { + var release = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var started = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + var sut = new BackgroundTaskScope("test"); + Assert.That(sut.Run("op", async _ => + { + started.TrySetResult(true); + await release.Task.ConfigureAwait(false); + }), Is.True); + + await started.Task.WaitAsync(s_timeout).ConfigureAwait(false); + + // Returns even though the operation above is still running. + sut.Dispose(); + + Assert.That(sut.ShutdownToken.IsCancellationRequested, Is.True); + Assert.That(sut.Run("late", _ => default), Is.False); + + release.TrySetResult(true); + await sut.DisposeAsync().ConfigureAwait(false); + Assert.That(sut.PendingCount, Is.Zero); + } + + [Test] + public void ConstructorRejectsNullOwner() + { + Assert.That(() => new BackgroundTaskScope(null!), Throws.ArgumentNullException); + } + } +}