From 98deefb86c778b522e652f4383992387abbc5082 Mon Sep 17 00:00:00 2001 From: John Simons Date: Sat, 15 Aug 2026 12:17:14 +1000 Subject: [PATCH 1/5] Consolidate test categories and enable parallel execution in CI Groups test projects that share infrastructure (like SQL Server or RabbitMQ) into consolidated categories to reduce container provisioning and compilation overhead. Updates the test runner to support parallel execution of assemblies on a single runner to maintain performance, including output buffering to prevent interleaved logs. --- .github/workflows/ci.yml | 47 +++++----- README.md | 20 ++--- ...eControl.AcceptanceTests.PostgreSql.csproj | 2 +- ...viceControl.AcceptanceTests.RavenDB.csproj | 2 +- ...ceControl.AcceptanceTests.SqlServer.csproj | 2 +- ...ntrol.Audit.AcceptanceTests.RavenDB.csproj | 2 +- ...erviceControl.Audit.AcceptanceTests.csproj | 2 +- ...rol.Audit.Persistence.Tests.RavenDB.csproj | 2 +- ...viceControl.Audit.Persistence.Tests.csproj | 2 +- .../ServiceControl.Audit.UnitTests.csproj | 2 +- ...ontrol.Persistence.Tests.PostgreSql.csproj | 2 +- ...ceControl.Persistence.Tests.RavenDB.csproj | 2 +- ...Control.Persistence.Tests.SqlServer.csproj | 2 +- ...itMQClassicConventionalRoutingTests.csproj | 2 +- ....RabbitMQClassicDirectRouting.Tests.csproj | 2 +- ...itMQQuorumConventionalRouting.Tests.csproj | 2 +- ...s.RabbitMQQuorumDirectRouting.Tests.csproj | 2 +- tools/run-tests.ps1 | 85 +++++++++++++++++-- tools/select-test-projects.ps1 | 4 + 19 files changed, 132 insertions(+), 54 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a3c7df2df6..a80ce84ecc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,6 +11,9 @@ env: defaults: run: shell: pwsh +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true jobs: build: name: ${{ matrix.os-name }}-${{ matrix.test-category }} @@ -18,13 +21,26 @@ jobs: strategy: matrix: os: [windows-latest, ubuntu-latest] - # Categories are declared by the property in each test project. - test-category: [ DefaultCore, DefaultAudit, DefaultMonitoring, SqlServer, SqlServerPersistence, AzureServiceBus, RabbitMQClassicConventional, RabbitMQClassicDirect, RabbitMQQuorumConventional, RabbitMQQuorumDirect, AzureStorageQueues, MSMQ, SQS, PrimaryRavenAcceptance, PrimarySqlServerAcceptance, PrimaryPostgreSqlAcceptance, PrimaryRavenPersistence, PostgreSql, PostgreSqlPersistence, IBMMQ ] + # Categories are declared by the property in each test project. A category can + # span several projects that share infrastructure, so that the container is provisioned once + # and the union of their build closures is compiled once. + test-category: [ DefaultCore, DefaultMonitoring, Raven, SqlServer, PostgreSql, RabbitMQ, AzureServiceBus, AzureStorageQueues, MSMQ, SQS, IBMMQ ] include: - os: windows-latest os-name: Windows - os: ubuntu-latest os-name: Linux + # How many of a category's assemblies run at once, so that a multi-project category costs the + # slowest of them rather than the sum. Defaults to 1 where unset. Dial down if the shared + # infrastructure turns out not to tolerate the concurrency. + - test-category: Raven + max-parallel: 3 + - test-category: RabbitMQ + max-parallel: 4 + - test-category: SqlServer + max-parallel: 3 + - test-category: PostgreSql + max-parallel: 3 exclude: - os: ubuntu-latest test-category: MSMQ @@ -56,13 +72,12 @@ jobs: # run in on Windows. A no-op on Linux, but still run there so those actions see the same # environment variables on both runners. - name: Setup WSL - if: startsWith(matrix.test-category, 'RabbitMQ') || contains(fromJSON('["SqlServer", "SqlServerPersistence", "PrimarySqlServerAcceptance", "PostgreSql", "PostgreSqlPersistence", "PrimaryPostgreSqlAcceptance", "IBMMQ"]'), matrix.test-category) + if: contains(fromJSON('["RabbitMQ", "SqlServer", "PostgreSql", "IBMMQ"]'), matrix.test-category) uses: Particular/setup-wsl-action@v1.2.0 with: # The action defaults to 4GB. The runner has 16GB and the build runs concurrently with the # container starting up, so give the VM real headroom. memory: 8GB - # there is an issue with az cli and python 3.14, so for now we need to pin it # once the issue is resolved it should be able to be re-floated # https://github.com/Azure/azure-cli/issues/32980. @@ -79,37 +94,29 @@ jobs: python -m pip install --upgrade pip python -m pip install --user "azure-cli==2.64.0" echo "$HOME/.local/bin" >> "$GITHUB_PATH" - - name: Setup SQL Server uses: Particular/install-sql-server-action@v3.0.0 if: matrix.test-category == 'SqlServer' - with: - connection-string-env-var: ServiceControl_TransportTests_SQL_ConnectionString - catalog: nservicebus - - name: Setup SQL Server persistence - uses: Particular/install-sql-server-action@v3.0.0 - if: matrix.test-category == 'SqlServerPersistence' || matrix.test-category == 'PrimarySqlServerAcceptance' with: connection-string-env-var: ServiceControl_Persistence_SqlServer_ConnectionString catalog: ServiceControl enable-full-text-search: true + - name: Point the SQL Server transport tests at the same server + if: matrix.test-category == 'SqlServer' + run: echo "ServiceControl_TransportTests_SQL_ConnectionString=$Env:ServiceControl_Persistence_SqlServer_ConnectionString" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append - name: Setup PostgreSQL uses: Particular/setup-postgres-action@v3.0.0 if: matrix.test-category == 'PostgreSql' - with: - connection-string-name: ServiceControl_TransportTests_PostgreSQL_ConnectionString - registry-username: ${{ secrets.DOCKERHUB_USERNAME }} - registry-password: ${{ secrets.DOCKERHUB_TOKEN }} - - name: Setup PostgreSQL persistence - uses: Particular/setup-postgres-action@v3.0.0 - if: matrix.test-category == 'PostgreSqlPersistence' || matrix.test-category == 'PrimaryPostgreSqlAcceptance' with: connection-string-name: ServiceControl_Persistence_PostgreSql_ConnectionString registry-username: ${{ secrets.DOCKERHUB_USERNAME }} registry-password: ${{ secrets.DOCKERHUB_TOKEN }} + - name: Point the PostgreSQL transport tests at the same server + if: matrix.test-category == 'PostgreSql' + run: echo "ServiceControl_TransportTests_PostgreSQL_ConnectionString=$Env:ServiceControl_Persistence_PostgreSql_ConnectionString" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append - name: Setup RabbitMQ uses: Particular/setup-rabbitmq-action@v2.0.0 - if: startsWith(matrix.test-category, 'RabbitMQ') + if: matrix.test-category == 'RabbitMQ' with: connection-string-name: ServiceControl_TransportTests_RabbitMQ_ConnectionString registry-username: ${{ secrets.DOCKERHUB_USERNAME }} @@ -153,7 +160,7 @@ jobs: azure-credentials: ${{ secrets.AZURE_ACI_CREDENTIALS }} tag: ServiceControl - name: Run tests - run: ./tools/run-tests.ps1 -Projects $Env:TEST_PROJECTS + run: ./tools/run-tests.ps1 -Projects $Env:TEST_PROJECTS -MaxParallel ${{ matrix.max-parallel || 1 }} env: TEST_PROJECTS: ${{ steps.select.outputs.test-projects }} ServiceControl_TESTS_FILTER: ${{ matrix.test-category }} diff --git a/README.md b/README.md index 8db5a58c80..dbdd477cb5 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,6 @@ Running all tests all the times takes a lot of resources. Tests are filtered bas Non-transport-specific: - `DefaultCore` -- `DefaultAudit` - `DefaultMonitoring` Transports: @@ -58,26 +57,25 @@ Transports: - `AzureStorageQueues` - `IBMMQ` - `MSMQ` -- `PostgreSql` -- `RabbitMQClassicConventional` -- `RabbitMQClassicDirect` -- `RabbitMQQuorumConventional` -- `RabbitMQQuorumDirect` -- `SqlServer` +- `RabbitMQ` (all four classic/quorum and conventional/direct routing combinations) - `SQS` +Transports and persisters sharing a database server: + +- `PostgreSql` (transport, persistence and acceptance tests) +- `SqlServer` (transport, persistence and acceptance tests) + Persisters: -- `PostgreSqlPersistence` -- `PrimaryRavenAcceptance` -- `PrimaryRavenPersistence` -- `SqlServerPersistence` +- `Raven` (persistence, acceptance and audit tests) > [!NOTE] > If no variable is defined all tests will be executed. Each category is declared once, by the `` property in the test project. CI reads that property to build and run only the projects belonging to the category under test, and the build generates the assembly-level `IncludeInTestCategory` attribute from it, which is what `ServiceControl_TESTS_FILTER` matches against at run time. +A category can span several projects. Where those projects need the same infrastructure, sharing a category means CI provisions it once, compiles the union of their build closures once, and runs their assemblies concurrently on a single runner. That is why, for example, all four RabbitMQ routing topologies are one category rather than four. + ### Adding a test project > [!IMPORTANT] diff --git a/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj b/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj index 5d1bd8efeb..fa0a9b1621 100644 --- a/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj +++ b/src/ServiceControl.AcceptanceTests.PostgreSql/ServiceControl.AcceptanceTests.PostgreSql.csproj @@ -2,7 +2,7 @@ net10.0 - PrimaryPostgreSqlAcceptance + PostgreSql diff --git a/src/ServiceControl.AcceptanceTests.RavenDB/ServiceControl.AcceptanceTests.RavenDB.csproj b/src/ServiceControl.AcceptanceTests.RavenDB/ServiceControl.AcceptanceTests.RavenDB.csproj index 93d3a06ba3..bc922ad126 100644 --- a/src/ServiceControl.AcceptanceTests.RavenDB/ServiceControl.AcceptanceTests.RavenDB.csproj +++ b/src/ServiceControl.AcceptanceTests.RavenDB/ServiceControl.AcceptanceTests.RavenDB.csproj @@ -2,7 +2,7 @@ net10.0 - PrimaryRavenAcceptance + Raven diff --git a/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj b/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj index d8ffdd841b..be951002d9 100644 --- a/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj +++ b/src/ServiceControl.AcceptanceTests.SqlServer/ServiceControl.AcceptanceTests.SqlServer.csproj @@ -2,7 +2,7 @@ net10.0 - PrimarySqlServerAcceptance + SqlServer diff --git a/src/ServiceControl.Audit.AcceptanceTests.RavenDB/ServiceControl.Audit.AcceptanceTests.RavenDB.csproj b/src/ServiceControl.Audit.AcceptanceTests.RavenDB/ServiceControl.Audit.AcceptanceTests.RavenDB.csproj index a71d5defe6..a22d55be05 100644 --- a/src/ServiceControl.Audit.AcceptanceTests.RavenDB/ServiceControl.Audit.AcceptanceTests.RavenDB.csproj +++ b/src/ServiceControl.Audit.AcceptanceTests.RavenDB/ServiceControl.Audit.AcceptanceTests.RavenDB.csproj @@ -2,7 +2,7 @@ net10.0 - DefaultAudit + Raven diff --git a/src/ServiceControl.Audit.AcceptanceTests/ServiceControl.Audit.AcceptanceTests.csproj b/src/ServiceControl.Audit.AcceptanceTests/ServiceControl.Audit.AcceptanceTests.csproj index 2388115142..afab38a784 100644 --- a/src/ServiceControl.Audit.AcceptanceTests/ServiceControl.Audit.AcceptanceTests.csproj +++ b/src/ServiceControl.Audit.AcceptanceTests/ServiceControl.Audit.AcceptanceTests.csproj @@ -2,7 +2,7 @@ net10.0 - DefaultAudit + Raven diff --git a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/ServiceControl.Audit.Persistence.Tests.RavenDB.csproj b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/ServiceControl.Audit.Persistence.Tests.RavenDB.csproj index 26a098607b..2c798be958 100644 --- a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/ServiceControl.Audit.Persistence.Tests.RavenDB.csproj +++ b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/ServiceControl.Audit.Persistence.Tests.RavenDB.csproj @@ -2,7 +2,7 @@ net10.0 - DefaultAudit + Raven diff --git a/src/ServiceControl.Audit.Persistence.Tests/ServiceControl.Audit.Persistence.Tests.csproj b/src/ServiceControl.Audit.Persistence.Tests/ServiceControl.Audit.Persistence.Tests.csproj index ece853badc..ac37ef4bce 100644 --- a/src/ServiceControl.Audit.Persistence.Tests/ServiceControl.Audit.Persistence.Tests.csproj +++ b/src/ServiceControl.Audit.Persistence.Tests/ServiceControl.Audit.Persistence.Tests.csproj @@ -2,7 +2,7 @@ net10.0 - DefaultAudit + Raven diff --git a/src/ServiceControl.Audit.UnitTests/ServiceControl.Audit.UnitTests.csproj b/src/ServiceControl.Audit.UnitTests/ServiceControl.Audit.UnitTests.csproj index 44a4d9efa0..c4ee2eb333 100644 --- a/src/ServiceControl.Audit.UnitTests/ServiceControl.Audit.UnitTests.csproj +++ b/src/ServiceControl.Audit.UnitTests/ServiceControl.Audit.UnitTests.csproj @@ -2,7 +2,7 @@ net10.0 - DefaultAudit + Raven diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj b/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj index a7cfd08509..5401253c40 100644 --- a/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj @@ -2,7 +2,7 @@ net10.0 - PostgreSqlPersistence + PostgreSql diff --git a/src/ServiceControl.Persistence.Tests.RavenDB/ServiceControl.Persistence.Tests.RavenDB.csproj b/src/ServiceControl.Persistence.Tests.RavenDB/ServiceControl.Persistence.Tests.RavenDB.csproj index 4b8c1de262..83f29d47a4 100644 --- a/src/ServiceControl.Persistence.Tests.RavenDB/ServiceControl.Persistence.Tests.RavenDB.csproj +++ b/src/ServiceControl.Persistence.Tests.RavenDB/ServiceControl.Persistence.Tests.RavenDB.csproj @@ -2,7 +2,7 @@ net10.0 - PrimaryRavenPersistence + Raven diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj b/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj index 7fb36676a0..7fd542c6f8 100644 --- a/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj +++ b/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj @@ -2,7 +2,7 @@ net10.0 - SqlServerPersistence + SqlServer diff --git a/src/ServiceControl.Transports.RabbitMQClassicConventionalRouting.Tests/ServiceControl.Transports.RabbitMQClassicConventionalRoutingTests.csproj b/src/ServiceControl.Transports.RabbitMQClassicConventionalRouting.Tests/ServiceControl.Transports.RabbitMQClassicConventionalRoutingTests.csproj index 6fd23a7ba6..4aa51233bf 100644 --- a/src/ServiceControl.Transports.RabbitMQClassicConventionalRouting.Tests/ServiceControl.Transports.RabbitMQClassicConventionalRoutingTests.csproj +++ b/src/ServiceControl.Transports.RabbitMQClassicConventionalRouting.Tests/ServiceControl.Transports.RabbitMQClassicConventionalRoutingTests.csproj @@ -2,7 +2,7 @@ net10.0 - RabbitMQClassicConventional + RabbitMQ diff --git a/src/ServiceControl.Transports.RabbitMQClassicDirectRouting.Tests/ServiceControl.Transports.RabbitMQClassicDirectRouting.Tests.csproj b/src/ServiceControl.Transports.RabbitMQClassicDirectRouting.Tests/ServiceControl.Transports.RabbitMQClassicDirectRouting.Tests.csproj index 2164ec1d26..8f5bb59f35 100644 --- a/src/ServiceControl.Transports.RabbitMQClassicDirectRouting.Tests/ServiceControl.Transports.RabbitMQClassicDirectRouting.Tests.csproj +++ b/src/ServiceControl.Transports.RabbitMQClassicDirectRouting.Tests/ServiceControl.Transports.RabbitMQClassicDirectRouting.Tests.csproj @@ -2,7 +2,7 @@ net10.0 - RabbitMQClassicDirect + RabbitMQ diff --git a/src/ServiceControl.Transports.RabbitMQQuorumConventionalRouting.Tests/ServiceControl.Transports.RabbitMQQuorumConventionalRouting.Tests.csproj b/src/ServiceControl.Transports.RabbitMQQuorumConventionalRouting.Tests/ServiceControl.Transports.RabbitMQQuorumConventionalRouting.Tests.csproj index be4eea22f0..8f5bb59f35 100644 --- a/src/ServiceControl.Transports.RabbitMQQuorumConventionalRouting.Tests/ServiceControl.Transports.RabbitMQQuorumConventionalRouting.Tests.csproj +++ b/src/ServiceControl.Transports.RabbitMQQuorumConventionalRouting.Tests/ServiceControl.Transports.RabbitMQQuorumConventionalRouting.Tests.csproj @@ -2,7 +2,7 @@ net10.0 - RabbitMQQuorumConventional + RabbitMQ diff --git a/src/ServiceControl.Transports.RabbitMQQuorumDirectRouting.Tests/ServiceControl.Transports.RabbitMQQuorumDirectRouting.Tests.csproj b/src/ServiceControl.Transports.RabbitMQQuorumDirectRouting.Tests/ServiceControl.Transports.RabbitMQQuorumDirectRouting.Tests.csproj index 5f5b275050..4aa51233bf 100644 --- a/src/ServiceControl.Transports.RabbitMQQuorumDirectRouting.Tests/ServiceControl.Transports.RabbitMQQuorumDirectRouting.Tests.csproj +++ b/src/ServiceControl.Transports.RabbitMQQuorumDirectRouting.Tests/ServiceControl.Transports.RabbitMQQuorumDirectRouting.Tests.csproj @@ -2,7 +2,7 @@ net10.0 - RabbitMQQuorumDirect + RabbitMQ diff --git a/tools/run-tests.ps1 b/tools/run-tests.ps1 index 5927f0d073..b31a78026f 100644 --- a/tools/run-tests.ps1 +++ b/tools/run-tests.ps1 @@ -4,6 +4,10 @@ # # This is a scoped replacement for Particular/run-tests-action, which has no way to be told which # projects to run. It should fold back into that action once it grows a 'projects' input. +# +# -MaxParallel runs several assemblies at once. CI jobs that merge categories sharing infrastructure +# use it so the job costs the slowest assembly rather than the sum of all of them. Output is buffered +# per run and replayed on completion, because interleaved dotnet test output is unreadable. param( [Parameter(Mandatory)] @@ -11,6 +15,9 @@ param( [string]$TargetPlatform = 'x64', + [ValidateRange(1, 16)] + [int]$MaxParallel = 1, + [switch]$ReportWarnings ) @@ -23,12 +30,12 @@ if ($projectPaths.Count -eq 0) { } Write-Output "Target Platform = $TargetPlatform" +Write-Output "Max parallel test runs = $MaxParallel" $reportWarningsValue = if ($ReportWarnings) { 'true' } else { 'false' } $isUnix = $PSVersionTable.Platform -eq 'Unix' -$exitCode = 0 -foreach ($project in $projectPaths) { +$runs = foreach ($project in $projectPaths) { $frameworks = @( (Select-Xml -Path $project -XPath "/Project/PropertyGroup/TargetFramework").Node.InnerText (Select-Xml -Path $project -XPath "/Project/PropertyGroup/TargetFrameworks").Node.InnerText -split ';' @@ -44,16 +51,78 @@ foreach ($project in $projectPaths) { continue } - Write-Output "::group::Running $(Split-Path $project -Leaf) ($framework)" + [pscustomobject]@{ + Label = "$(Split-Path $project -Leaf) ($framework)" + Project = $project + Framework = $framework + } + } +} + +$runs = @($runs) - dotnet test $project --configuration Release --no-build --framework $framework --logger "GitHubActions;report-warnings=$reportWarningsValue" -- RunConfiguration.TreatNoTestsAsError=true "RunConfiguration.TargetPlatform=$TargetPlatform" +if ($runs.Count -eq 0) { + throw 'No test projects were runnable on this platform.' +} - Write-Output '::endgroup::' +$exitCode = 0 - if ($LASTEXITCODE -ne 0) { - Write-Output "::error::Exit code = $LASTEXITCODE" - $exitCode = 1 +function Complete-Run($run) { + Write-Output "::group::Running $($run.Label)" + foreach ($stream in @($run.OutFile, $run.ErrFile)) { + if ((Test-Path $stream) -and (Get-Item $stream).Length -gt 0) { + Get-Content -Path $stream | Write-Output } + Remove-Item -Path $stream -Force -ErrorAction SilentlyContinue + } + Write-Output '::endgroup::' + + if ($run.Process.ExitCode -ne 0) { + Write-Output "::error::$($run.Label) exit code = $($run.Process.ExitCode)" + $script:exitCode = 1 + } +} + +$pending = [Collections.Generic.Queue[object]]::new($runs) +$active = [Collections.Generic.List[object]]::new() + +while ($pending.Count -gt 0 -or $active.Count -gt 0) { + while ($active.Count -lt $MaxParallel -and $pending.Count -gt 0) { + $run = $pending.Dequeue() + $run | Add-Member -NotePropertyName OutFile -NotePropertyValue ([IO.Path]::GetTempFileName()) + $run | Add-Member -NotePropertyName ErrFile -NotePropertyValue ([IO.Path]::GetTempFileName()) + + $arguments = @( + 'test', $run.Project + '--configuration', 'Release' + '--no-build' + '--framework', $run.Framework + '--logger', "GitHubActions;report-warnings=$reportWarningsValue" + '--' + 'RunConfiguration.TreatNoTestsAsError=true' + "RunConfiguration.TargetPlatform=$TargetPlatform" + ) + + Write-Output "Starting $($run.Label)" + $run | Add-Member -NotePropertyName Process -NotePropertyValue ( + Start-Process -FilePath 'dotnet' -ArgumentList $arguments -NoNewWindow -PassThru ` + -RedirectStandardOutput $run.OutFile -RedirectStandardError $run.ErrFile) + $active.Add($run) + } + + $finished = $active | Where-Object { $_.Process.HasExited } + + if (-not $finished) { + Start-Sleep -Milliseconds 500 + continue + } + + foreach ($run in @($finished)) { + # WaitForExit with no timeout after HasExited flushes the redirected streams, which are + # otherwise not guaranteed to be complete when the process object reports exit. + $run.Process.WaitForExit() + Complete-Run $run + [void]$active.Remove($run) } } diff --git a/tools/select-test-projects.ps1 b/tools/select-test-projects.ps1 index d1f5704f6f..8ae2dc89aa 100644 --- a/tools/select-test-projects.ps1 +++ b/tools/select-test-projects.ps1 @@ -5,6 +5,10 @@ # Writes the selected project paths to $GITHUB_OUTPUT as 'test-projects', and generates an MSBuild # traversal project so that `dotnet build` can build the whole selection in one graph. # +# A category can span several test projects that share infrastructure, so that CI provisions its +# container once and compiles the union of their closures once. Their assemblies then run concurrently, +# via the -MaxParallel switch on run-tests.ps1. +# # Use -List to print every category and its projects without writing any files. [CmdletBinding(DefaultParameterSetName = 'Select')] From 14feafc1ff82f3da1a6eec7ec3ffe4553315be35 Mon Sep 17 00:00:00 2001 From: John Simons Date: Sat, 15 Aug 2026 13:56:15 +1000 Subject: [PATCH 2/5] Prevent RavenDB port conflicts when running tests in parallel Assigns unique ports to test runs via environment variables when parallel execution is enabled. This avoids collisions where multiple processes probe the same available port simultaneously before binding. Includes an update to the process exit logic to use a timeout, preventing potential hangs caused by inherited handles in child processes. --- .../SharedEmbeddedServer.cs | 2 +- .../SharedEmbeddedServer.cs | 2 +- src/TestHelper/PortUtility.cs | 24 ++++++++++++++++ tools/run-tests.ps1 | 28 ++++++++++++++++--- 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/SharedEmbeddedServer.cs b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/SharedEmbeddedServer.cs index 8d8c613ca3..0cf229eb72 100644 --- a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/SharedEmbeddedServer.cs +++ b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/SharedEmbeddedServer.cs @@ -35,7 +35,7 @@ public static async Task GetInstance(CancellationToken cancell var dbPath = Path.Combine(TestContext.CurrentContext.WorkDirectory, "Tests", "AuditData"); var logPath = Path.Combine(TestContext.CurrentContext.WorkDirectory, "Logs", "Audit"); var logsMode = "Operations"; - var serverUrl = $"http://localhost:{PortUtility.FindAvailablePort(33334)}"; + var serverUrl = $"http://localhost:{PortUtility.GetAssignedOrAvailablePort(33334)}"; var databaseConfiguration = new DatabaseConfiguration("audit", 60, true, TimeSpan.FromMinutes(5), 120000, 5, 5, new ServerConfiguration(dbPath, serverUrl, logPath, logsMode), TimeSpan.FromSeconds(60)); var serverConfig = databaseConfiguration.ServerConfiguration; diff --git a/src/ServiceControl.Persistence.Tests.RavenDB/SharedEmbeddedServer.cs b/src/ServiceControl.Persistence.Tests.RavenDB/SharedEmbeddedServer.cs index b35874bba5..49b3cddb56 100644 --- a/src/ServiceControl.Persistence.Tests.RavenDB/SharedEmbeddedServer.cs +++ b/src/ServiceControl.Persistence.Tests.RavenDB/SharedEmbeddedServer.cs @@ -41,7 +41,7 @@ public static async Task GetInstance(CancellationToken cancell DatabasePath = dbPath, LogPath = logPath, LogsMode = logsMode, - DatabaseMaintenancePort = PortUtility.FindAvailablePort(RavenPersisterSettings.DatabaseMaintenancePortDefault) + DatabaseMaintenancePort = PortUtility.GetAssignedOrAvailablePort(RavenPersisterSettings.DatabaseMaintenancePortDefault) }; // TODO: See if more refactoring can be done between this and the RavenPersisterSettings above diff --git a/src/TestHelper/PortUtility.cs b/src/TestHelper/PortUtility.cs index 7fc4648031..21c1aa4169 100644 --- a/src/TestHelper/PortUtility.cs +++ b/src/TestHelper/PortUtility.cs @@ -1,10 +1,34 @@ namespace TestHelper { + using System; + using System.Globalization; using System.Linq; using System.Net.NetworkInformation; public static class PortUtility { + /// + /// The port an embedded server should bind, when the test runner has assigned one. + /// + public const string AssignedPortVariableName = "ServiceControl_TESTS_RAVENDB_PORT"; + + /// + /// Returns the port assigned by the test runner, or probes for a free one when running alone. + /// + /// + /// Concurrent test processes cannot each probe: only inspects + /// the listeners active at that instant, so processes starting together all see the same port + /// free and all but one then fail to bind. + /// + public static int GetAssignedOrAvailablePort(int startPort) + { + var assignedPort = Environment.GetEnvironmentVariable(AssignedPortVariableName); + + return string.IsNullOrWhiteSpace(assignedPort) + ? FindAvailablePort(startPort) + : int.Parse(assignedPort, CultureInfo.InvariantCulture); + } + public static int FindAvailablePort(int startPort) { var activeTcpListeners = IPGlobalProperties diff --git a/tools/run-tests.ps1 b/tools/run-tests.ps1 index b31a78026f..87adca9c04 100644 --- a/tools/run-tests.ps1 +++ b/tools/run-tests.ps1 @@ -65,6 +65,14 @@ if ($runs.Count -eq 0) { throw 'No test projects were runnable on this platform.' } +# RavenDB.Embedded binds a fixed port, and the tests otherwise pick one by probing the active +# listeners, which concurrent processes all do at the same instant and all resolve to the same port. +# Hand each run its own instead. Left unset when running one at a time, because probing copes better +# with a port that something else on the machine already holds. +$assignPorts = $MaxParallel -gt 1 +$nextPort = 33334 +$portSpacing = 10 + $exitCode = 0 function Complete-Run($run) { @@ -103,7 +111,17 @@ while ($pending.Count -gt 0 -or $active.Count -gt 0) { "RunConfiguration.TargetPlatform=$TargetPlatform" ) - Write-Output "Starting $($run.Label)" + if ($assignPorts) { + # Set immediately before spawning, so the child inherits this run's value. Safe because + # spawning is serialised here even though the runs themselves are not. + $Env:ServiceControl_TESTS_RAVENDB_PORT = $nextPort + Write-Output "Starting $($run.Label) with RavenDB port $nextPort" + $nextPort += $portSpacing + } + else { + Write-Output "Starting $($run.Label)" + } + $run | Add-Member -NotePropertyName Process -NotePropertyValue ( Start-Process -FilePath 'dotnet' -ArgumentList $arguments -NoNewWindow -PassThru ` -RedirectStandardOutput $run.OutFile -RedirectStandardError $run.ErrFile) @@ -118,9 +136,11 @@ while ($pending.Count -gt 0 -or $active.Count -gt 0) { } foreach ($run in @($finished)) { - # WaitForExit with no timeout after HasExited flushes the redirected streams, which are - # otherwise not guaranteed to be complete when the process object reports exit. - $run.Process.WaitForExit() + # Bounded on purpose. The parameterless WaitForExit() also waits for the redirected streams to + # reach EOF, and on Linux Start-Process pumps them through a pipe, so a test that leaves behind + # a child holding the inherited handle blocks it forever. HasExited has already told us the + # test process itself is done; this only gives the pump a moment to drain. + [void]$run.Process.WaitForExit(5000) Complete-Run $run [void]$active.Remove($run) } From 0bb0bacf8cfb51ccfa5a5c65c4a007c690f972b9 Mon Sep 17 00:00:00 2001 From: John Simons Date: Sat, 15 Aug 2026 14:14:37 +1000 Subject: [PATCH 3/5] Document CI parallelization plan and fix event source creation races Adds a detailed research and recommendation document for optimizing CI runner usage through job consolidation and step parallelism. Updates event source creation logic to handle race conditions that occur when multiple test assemblies attempt to create machine-wide resources concurrently on the same runner. --- docs/ci-parallelisation-plan.md | 462 ++++++++++++++++++ .../Infrastructure/EventSourceCreator.cs | 15 +- .../Installers/EventSourceCreator.cs | 15 +- 3 files changed, 490 insertions(+), 2 deletions(-) create mode 100644 docs/ci-parallelisation-plan.md diff --git a/docs/ci-parallelisation-plan.md b/docs/ci-parallelisation-plan.md new file mode 100644 index 0000000000..163db91f48 --- /dev/null +++ b/docs/ci-parallelisation-plan.md @@ -0,0 +1,462 @@ +# CI parallelisation: research and recommendation + +Status: proposal. Measured August 2026 against run +[31835332447](https://github.com/Particular/ServiceControl/actions/runs/31835332447) (master push, +all green) and the four-way concurrent burst of runs 31788928123/31788933879/31788939745/31788946584. + +## The short version + +The premise behind this investigation was "we have a limited runner pool, so let's parallelise steps +even more". The measurements say the second half of that does not follow from the first. + +Parallelising *more steps inside existing jobs* buys almost nothing. The build is already backgrounded, +and in the jobs where it matters it is already fully hidden behind infrastructure provisioning. In the +jobs where it is not hidden, there is nothing to hide it behind. + +What does have legs is the inverse move: **use in-job step parallelism to collapse jobs back onto fewer +runners, without paying the wall-clock penalty that caused us to split them apart in the first place.** + +Nothing tooling-wise was ever blocking this. We have had the capability since we adopted +`background:`, and the workflow used the `- parallel:` list shape before 54f87f37a replaced it. What +changed is the *constraint*, not the toolbox: the job count has grown to the point where the pool, not +the critical path, is the thing worth optimising. + +Three changes, in priority order: + +1. Add `concurrency` with `cancel-in-progress`. Free, no wall-clock cost, and it addresses the actual + observed cause of pool exhaustion. +2. Merge the categories that share infrastructure, by giving their projects a shared ``, + and run their assemblies concurrently on the one runner. Saves 18 jobs at zero wall-clock cost. +3. Attack the SQL Server acceptance suite specifically. It is the entire critical path. + +## What the numbers actually say + +### The pool cap is real, and it is exactly 60 + +Peak concurrent running jobs across the four-run burst was **60**, hit at 09:41:02 and held. That is +the documented GitHub Team plan cap, confirmed empirically. It is an **org-wide** cap, shared with +every other Particular repository. + +One CI run is currently **52 jobs**. That is 87% of the entire organisation's concurrency budget. + +| Scenario | Median queue | p90 queue | Max queue | +|---|---:|---:|---:| +| Single run in flight | 2 s | 4 s | 77 s | +| Four runs in flight (208 jobs) | 165 s | 592 s | 665 s | + +So the pool is a non-issue at one run and a serious issue at two or more. The backlog took 674 s to +drain. During that window ServiceControl starved every other repo in the org. + +### Current shape of a run + +| | Jobs | Runner-min | +|---|---:|---:| +| Windows matrix | 21 | 110.9 | +| Linux matrix | 20 | 68.7 | +| Installers / containers / container-test | 11 | 28.9 | +| **Total** | **52** | **208.4** | + +Wall clock 14.5 min. This is a regression from the 8.6 min / 149 runner-min that PR #5715 achieved: +the `Default` and `RabbitMQ` splits and the two new EF acceptance categories have since added 17 jobs. + +### There is a large amount of slack under the critical path + +Critical path is `Windows-PrimarySqlServerAcceptance` at **870 s**. The next longest is 761 s. Then it +falls off a cliff: **34 of 41 matrix jobs finish in under 420 s.** + +That gap is the whole opportunity. Any job we can merge and keep under ~800 s is a runner reclaimed +for **free**, because the run was going to wait on the SQL Server acceptance job regardless. + +### Where the time goes in the slow jobs + +Seconds, from the GitHub step timings: + +| Job | Total | Overhead | Build | Infra | Wait | Tests | +|---|---:|---:|---:|---:|---:|---:| +| Windows-PrimarySqlServerAcceptance | 870 | 123 | 255 | 200 | 55 | 484 | +| Windows-SqlServerPersistence | 761 | 69 | 123 | 218 | 0 | 470 | +| Linux-AzureServiceBus | 517 | 150 | 25 | 68 | 0 | 296 | +| Linux-PrimarySqlServerAcceptance | 512 | 24 | 77 | 98 | 0 | 385 | +| Windows-AzureServiceBus | 511 | 82 | 50 | 76 | 50 | 300 | +| Linux-SqlServerPersistence | 487 | 26 | 83 | 107 | 0 | 352 | +| Windows-PrimaryRavenAcceptance | 413 | 45 | 104 | 0 | 104 | 262 | +| Windows-DefaultCore | 318 | 64 | 150 | 0 | 150 | 102 | + +Note the `Wait` column. Where a job has infra to provision, `background: true` works perfectly and +`Wait` is 0. Where a job has **no** infra (the Raven and Default categories, MSMQ, SQS), `Wait` equals +`Build` exactly, because there is nothing to overlap with. Backgrounding the build there is a no-op. + +Total across the matrix: ~19 runner-minutes sit in `Wait`. That is not recoverable by parallelising +harder within those jobs. It is only recoverable by making one build serve several test runs. + +## Why "parallelise steps more" does not work on its own + +Walking the candidates that a naive reading would suggest: + +- **Overlap infra setup with itself.** Already effectively serial by necessity: `Setup WSL` provisions + the Docker host that the SQL Server / PostgreSQL / RabbitMQ / IBM MQ containers run in. The chain is + real, not incidental. +- **Overlap the Azure provisioning with the build.** Deliberately not done. Commit 6ec08ce6b moved the + Azure steps behind `Wait for build` on purpose, because they create real cloud resources and we do + not want them alive while a build might fail. +- **Overlap the build with itself.** `-graph` already parallelises the MSBuild graph. +- **Overlap `Run tests` with anything.** It is the last step and depends on everything. + +The one genuine in-job win left is running *multiple test assemblies* at once, and most jobs only have +one assembly. Which points straight at merging. + +## Recommendation 1: `concurrency` with cancel-in-progress + +GitHub Actions does not cancel superseded runs by default. Push a commit to a PR branch, and the run +triggered by your *previous* commit keeps going to completion, all 52 jobs of it, testing code nobody +is going to merge. The only thing that stops it is the `concurrency` key, which puts runs into a named +group and cancels the older member when a new one arrives. No workflow in `.github/workflows/` sets it. + +This is not theoretical. Across the last 120 CI runs: + +- **Zero** runs have the conclusion `cancelled`. Nothing has ever been superseded. +- **19** pairs of runs overlap, where an older run was still executing when the next push to the same + branch started a new one. + +The overlaps are long, because a 52-job run is long. Some examples: + +| Branch | Obsolete run kept going for | +|---|---:| +| `john/cancel_part11` | 1395 s | +| `john/cancel_part12` | 1157 s | +| `rhys/atomic-commit2` | 1378 s | + +So a branch under active iteration can easily hold 100+ jobs at once across two runs, against an +org-wide cap of 60. That is the mechanism behind the four-run pileup measured above, and it is why +this is the first recommendation: it attacks the observed problem rather than the theoretical one. + +```yaml +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true +``` + +Applied to `ci.yml`. `github.ref` alone is a sufficient group key: on a `pull_request` event it is +`refs/pull//merge`, already unique per PR. + +**This cancels master too, deliberately.** The first draft exempted master and `release-*` on the +grounds that we always want a build of a merged commit. The data does not really support paying for +that: **4 of the last 21 master runs** would have been cancelled, one with 1616 s still to run. Master +merges land close enough together to produce the same waste PR branches do. + +What it costs, having checked rather than assumed: + +- **Releases are not affected at all.** They run from `release.yml`, triggered on tags. `ci.yml` + triggers only on `pull_request` and on pushes to `master` / `release-*` branches, never on a tag, so + a release build can never be cancelled by this. +- **An intermediate master commit loses its own CI signal.** If master goes red you know the newest + commit is bad but not which of the ones it superseded was. Modest, since each merged PR was green on + its own branch first. +- **A ghcr prerelease image may not finish pushing.** `build-containers.yml` runs with `push: true` + and `build-db-container.yml` does `docker image push --all-tags` on master, tagged with the commit's + MinVer version. Cancelling mid-push leaves that version's dev image missing rather than corrupt, + because the next master commit publishes a different version. + +If the last point ever matters, `concurrency` can be set per job rather than workflow-wide, cancelling +the test matrix while letting `containers` / `db-container` / `windows-installers` finish. + +## Recommendation 2: merge jobs that share infrastructure, running their tests concurrently + +The mechanism is the one we already use. Per the +[June 2026 changelog](https://github.blog/changelog/2026-06-25-actions-steps-can-now-be-run-in-parallel/), +`parallel:` is explicitly "syntactic sugar" that "takes a group of steps and converts them to +`background` steps with a `wait` after". So `parallel:` and `background:`/`wait-all:` are the same +mechanism in two shapes, and either expresses these merges. Use `parallel:` for a fixed group of +sibling test steps because it reads better; keep `background:`/`wait:` where a single step needs to +overlap a named chain, as the build does today. + +What matters is not the keyword but the shape of the merged job: the test phase has to be the **max** +of the merged assemblies, not the sum. Merging with the assemblies still serial saves a runner but +lengthens the job by the full duration of everything it absorbed, which is a bad trade. Merging with +them concurrent is close to free. + +### 2a. RabbitMQ: 8 jobs → 2 + +The best candidate by a distance. The four projects are identical apart from +`TransportTestsConfiguration.cs` and their ``; all four link the same +`ServiceControl.Transports.Tests\*.cs` sources. + +They can share one broker. `TransportTestFixture` already suffixes every queue with +`Path.GetRandomFileName()`, so there are no name collisions between the four topologies, and therefore +no classic-vs-quorum redeclare conflict. + +| | Now (4 jobs) | Merged (1 job) | Saving | +|---|---:|---:|---:| +| Windows | 987 s | ~420 s | 3 runners, ~570 runner-s | +| Linux | 498 s | ~225 s | 3 runners, ~275 runner-s | + +**Risk:** all four projects do `` on `NUnitParallelRunnerSettings.cs`, so each suite +currently runs strictly serially, presumably deliberately (queue-length monitoring is timing +sensitive). Running four of them at once against one broker is new concurrency exposure. Mitigate by +merging in two stages: pair them first (classic together, quorum together), watch for flakes, then go +to four. + +### 2b. PostgreSQL: 6 jobs → 2 + +`PostgreSqlPersistence` and `PrimaryPostgreSqlAcceptance` already invoked +`Particular/setup-postgres-action` with byte-identical inputs. The `PostgreSql` transport category +joins them on the same container, as described above. + +| | Now (3 jobs) | Merged (1 job) | Saving | +|---|---:|---:|---:| +| Windows | 874 s | ~520 s | 2 runners, ~355 runner-s | +| Linux | 525 s | ~330 s | 2 runners, ~195 runner-s | + +### 2c. Raven family: 6 jobs → 2 + +`DefaultAudit`, `PrimaryRavenAcceptance`, `PrimaryRavenPersistence` provision no infrastructure at all +(RavenDB.Embedded supplies its own server per test project output directory). The win here is not +infra, it is that they currently pay **three separate builds** with three unhidden `Wait` phases +(Windows: 79 + 104 + 91 = 274 s of build, none of it overlapped). + +| | Now (3 jobs) | Merged (1 job) | Saving | +|---|---:|---:|---:| +| Windows | 942 s | ~610 s | 2 runners, ~330 runner-s | +| Linux | 621 s | ~400 s | 2 runners, ~220 runner-s | + +Keep the `Download RavenDB Server` gate as-is. Per `ci-scoping-decisions`, only +`ServiceControlInstaller.Packaging` needs it, and it lives in `DefaultCore`, not in this group. + +### 2d. The SQL Server pair: merged, on purpose, to be measured + +**Decision: merge it and measure.** The analysis below argued for deferring it. That analysis is +estimates, and estimates are what the `build_once` spike was rejected for relying on. Backing it out +is a one-line change (give `ServiceControl.AcceptanceTests.SqlServer.csproj` its own `` +again and add it to the matrix), so the cost of being wrong is one CI run. + +The `SqlServer` category now also carries the transport tests, which adds ~50 s of test time to a job +already estimated at 877-1040 s, against a whole runner saved. + +**This group is also the experiment.** It is the only merge that can move wall clock, because it is the +only one that contains the critical path. So a single run disambiguates cleanly: + +- Job count and runner-minutes measure *all* the merges. +- Wall clock measures *this* merge, essentially on its own. + +Baseline to compare against, from run 31835332447: **52 jobs, 208 runner-min, 14.5 min wall clock**, +critical path `Windows-PrimarySqlServerAcceptance` at 870 s. + +Predicted after: **38 jobs, ~168 runner-min**, wall clock somewhere between 877 s (perfect test +scaling) and ~1040 s (heavy contention). If wall clock lands near 877 s, keep it. If it lands near +1040 s, revert this one entry and keep the other three merges, which are unaffected. + +The reasoning that produced that range: + +`SqlServerPersistence` (761 s) and `PrimarySqlServerAcceptance` (870 s) use identical +`install-sql-server-action` inputs, so on paper they are the most natural merge of the lot. Both also +isolate per test: `PersistenceTestsContext` creates `sc_test_{guid}`, +`AcceptanceTestStorageConfiguration` creates `sc_at_{guid}`. Technically it would work. + +The problem is that these two are the #1 and #2 longest jobs in the run, and a merged job *contains* +whatever it absorbed. So this merge, uniquely, can only make the critical path longer or leave it the +same. Here is the arithmetic: + +| | Today (2 runners) | Merged (1 runner) | +|---|---|---| +| Overhead | 69 + 123 | 123 (paid once) | +| Infra | 218 + 200 | ~200 (one SQL container) | +| Build | 123 + 255, mostly hidden by infra | ~270 union, 70 s residual after infra | +| Tests | 470 + 484, on separate boxes | 484 s **at best**, 954 s at worst | +| **Job wall clock** | **870 s** (they run side by side) | **877 s best case** | +| Runner cost | 1631 s | ~880 s | + +The saving is real, about 590 runner-seconds. But look at the best case: 877 s against today's 870 s. +The shared savings (one build, one infra setup, one overhead) come to roughly 250 s, and they are +almost exactly cancelled out by the fact that the test phase can never drop below the longer of the two +suites. + +And 484 s for the merged test phase assumes perfect scaling, which is the one thing we should not +assume here. Both assemblies already run `Parallelizable(ParallelScope.All)` with +`LevelOfParallelism(4)` on a 4-vCPU runner, so each one *already saturates the box on its own*. Running +both concurrently puts 8 fixtures on 4 vCPUs, each spinning up a ServiceControl host and creating and +dropping databases against a single SQL Server container. If contention pushes the test phase to 650 s, +the merged job is 1043 s and the whole run goes from 14.5 to ~17 minutes. + +**Contrast with the merges that are safe.** Merged RabbitMQ is ~420 s against a critical path of 870 s. +My contention estimate there could be off by 100% and it would still fit. Those merges have a ~450 s +error budget; this one has none by construction. That asymmetry, not the raw saving, is the reason to +treat them differently. + +If the measurement comes back bad, recommendation 3 is the way back in: bring +`PrimarySqlServerAcceptance` down to ~620 s first so something else becomes the critical path, and the +slack this merge needs reappears. + +### Projected result + +| | Jobs | Runner-min | Wall clock | +|---|---:|---:|---:| +| Now | 52 | 208 | 14.5 min | +| After merges | 34 | ~160 | 14.5 min (unchanged, if the SQL merge scales) | + +34 jobs is 57% of the org cap instead of 87%, and combined with recommendation 1 a second concurrent +run largely stops queueing. + +## Recommendation 3: attack the critical path directly + +Merging does nothing for wall clock. Only `Windows-PrimarySqlServerAcceptance` does, because it *is* +the wall clock. Its 870 s breaks down as 255 s build + 200 s infra (which hides most of the build) + +55 s residual wait + 484 s tests. + +Two levers, cheapest first: + +- **Raise `LevelOfParallelism` for the acceptance assemblies.** They link + `ServiceControl.UnitTests\NUnitParallelRunnerSettings.cs`, which sets `ParallelScope.All` with + `LevelOfParallelism(4)`. These tests are dominated by waiting on message ingestion, not CPU, and each + gets its own database. 6 or 8 is worth measuring on a 4-vCPU runner. One-line change, easy to revert. +- **Shard the assembly in-job.** Two `dotnet test` steps with complementary `--filter` expressions, + run concurrently on the same runner against the same SQL container. This is the sharding that + `ci-scoping-decisions` identified as the only remaining lever, but done *inside* the job so it costs + no pool capacity. Would take the job to roughly 620 s. + +A third, broader lever: the Windows build is 255 s against Linux's 77 s for the same closure, and +Windows builds total ~32 runner-minutes across the matrix. Adding a Defender exclusion for the +workspace on Windows runners is a well-known 30-50% win on .NET builds and would benefit all 21 Windows +jobs. Worth a spike. + +## Open question for the team + +Windows costs 110.9 runner-min against Linux's 68.7 for substantially the same 20 categories. + +Do the pure-transport categories (RabbitMQ, SQS, ASQ, ASB, IBMMQ, PostgreSql, SqlServer) need to run on +Windows on *every PR*? They exercise transport client libraries where OS-specific risk is low. Running +them Linux-only on PRs and keeping the full matrix on master and `release-*` would remove another ~10 +jobs and ~30 runner-minutes from the PR path. + +MSMQ obviously stays Windows-only. `DefaultCore`, `DefaultAudit` and `DefaultMonitoring` must stay on +both, per `ci-scoping-decisions`: `Should_populate_appSettings_from_exe_config_file` needs a real +Windows `.exe`. + +This is a coverage-vs-capacity call, not a technical one, so it needs a decision rather than a patch. + +## What has been implemented + +All four merges plus `concurrency`, on branch `john/acceptance-test_gaps`, ready to measure. + +**The merges live in the projects, not in the workflow.** The first cut of this carried a +group-to-categories mapping in the matrix, which meant the workflow matched category names as strings +and the mapping was duplicated between YAML and the csproj files. Instead, the merged categories are +simply *declared*: the projects being merged now share a `` value. There is no new +concept, no group layer, and nothing for the two halves to drift apart on. Category count goes from 20 +to 13. + +| Category | Absorbs | Projects | +|---|---|---:| +| `Raven` | `DefaultAudit`, `PrimaryRavenAcceptance`, `PrimaryRavenPersistence` | 7 | +| `RabbitMQ` | the four classic/quorum × conventional/direct categories | 4 | +| `SqlServer` | `SqlServerPersistence`, `PrimarySqlServerAcceptance` | 3 | +| `PostgreSql` | `PostgreSqlPersistence`, `PrimaryPostgreSqlAcceptance` | 3 | + +Category count goes from 20 to **11**. + +- **17 `.csproj` files** — updated `` to the merged value. +- **`.github/workflows/ci.yml`** — added `concurrency` with `cancel-in-progress`. The matrix keeps its + `test-category` axis, now listing 11 values, and gains one `max-parallel` entry per merged category. + Matrix jobs drop from **39 to 21**, so a run goes from 52 to **34 jobs**. +- **One database server per category, not two.** Both provisioning actions bind a fixed host port + (`1433` and `5432` are hardcoded in their setup scripts), so a job can only invoke each once. It + only needs to: the persistence and acceptance suites create a database per test (`sc_test_`/`sc_at_` + plus a GUID) and the transport suite creates queue tables under randomly suffixed names, so all + three share one server without colliding. The action runs once for the suite that needs the most + from it (`catalog: ServiceControl` with full-text search), and the transport connection-string + variable is aliased onto the same server in the following step. That also stops the SQL Server + full-text install being paid for twice. +- **`tools/select-test-projects.ps1`** — unchanged in shape: still selects by one `` + value. It needed no modification for merging at all, which is the point. +- **`tools/run-tests.ps1`** — added `-MaxParallel` (default 1, so single-project categories behave + exactly as before). Concurrent runs buffer stdout/stderr per assembly and replay them into + `::group::` blocks on completion, because interleaved `dotnet test` output is unreadable. +- **`ServiceControl_TESTS_FILTER` still works and is still set.** Because a merged category's projects + all declare the same ``, the generated assembly attribute matches for every assembly + in the job. Had the merge lived in the workflow instead, this would have broken: the comparison is + an exact match, so one job-wide value would have ignored every assembly but one. +- **`README.md`** — updated the documented filter values. + +Verified locally: the declared categories and the matrix axis are an exact set match in both +directions, the matrix expands to 25 jobs with `max-parallel` landing on the right eight, concurrent +runs start together and report separate logs with a correct aggregate exit code, and `-MaxParallel 1` +is still strictly sequential. + +## First measured run (PR #5783, run 31858824648) + +Everything introduced here worked on the first run, with two exceptions found and fixed. + +**The merges validated.** Every assembly in every merged category ran and passed, so nothing was +silently skipped: + +| Job | Assemblies | Tests | Was (separate jobs) | Now | +|---|---:|---|---:|---:| +| Windows-SqlServer | 3 | 31 + 409 + 146 | 3 runners, 870 s critical path | **1 runner, 746 s** | +| Linux-SqlServer | 3 | same | 3 runners, 512 s | 1 runner, 549 s | +| Windows-PostgreSql | 3 | 23 + 411 + 145 | 3 runners, 331 s | 1 runner, 439 s | +| Linux-PostgreSql | 3 | same | 3 runners, 268 s | 1 runner, 271 s | +| Windows-RabbitMQ | 4 | 14 + 13 + 13 + 16 | 4 runners, 333 s | **1 runner, 269 s** | +| Linux-RabbitMQ | 4 | same | 4 runners, 161 s | 1 runner, 173 s | + +**The SQL Server merge was the open question, and it beat its own best case.** Predicted 877-1040 s, +measured **746 s** on one runner against a previous critical path of 870 s across three. So it is a +win on wall clock *and* on runner-minutes, not the trade-off the estimates suggested. The shared +server and per-test database isolation held under three concurrent assemblies. The RabbitMQ shared +broker likewise: four topologies at once, no queue collisions, and faster than the slowest of the four +old jobs. + +**Two failures, both real, both fixed.** + +1. **`Raven` failed on both runners.** `RavenDB.Embedded` binds a fixed port, and `SharedEmbeddedServer` + picks it via `PortUtility.FindAvailablePort`, which only inspects the currently active listeners. + Two test processes starting together both see the base port free and both claim it, so the second + dies with `Failed to bind to address http://127.0.0.1:33334: address already in use`. Six of the + seven assemblies passed; `ServiceControl.AcceptanceTests.RavenDB` lost the race with + `ServiceControl.Persistence.Tests.RavenDB`, which shares that `SharedEmbeddedServer`. + Fixed properly rather than by giving up the concurrency. `PortUtility.GetAssignedOrAvailablePort` + takes the port from `ServiceControl_TESTS_RAVENDB_PORT` when it is set and falls back to probing + when it is not, and `run-tests.ps1` hands each concurrent run its own port (33334, then +10 each). + Both `SharedEmbeddedServer` implementations, error instance and audit, use it. The variable is + deliberately left unset for serial runs, where probing copes better with a port something else on + the machine already holds. `Raven` stays at `max-parallel: 3`. +2. **`Linux-DefaultCore` hung**, at over 1000 s against a dead-steady 189-205 s in the five previous + runs. This one was self-inflicted: `run-tests.ps1` called the parameterless + `Process.WaitForExit()`, which waits not only for the process but for its redirected streams to + reach EOF. On Linux `Start-Process` pumps those through a pipe, so a test leaving behind a child + that inherited the handle blocks it forever. Windows redirects to a file handle and was unaffected, + which is why only Linux hung. Now bounded to 5 s, which is all the pump needs once the process has + exited. + +### Shared machine-wide state is the recurring hazard + +Two of the three failures so far were the same shape: a resource that is global to the machine, which +separate jobs never contended for and concurrent assemblies in one job now do. + +- **RavenDB.Embedded's port**, fixed by assigning one per process. +- **The Windows event log source**, which is machine-wide. `EventSourceCreator.Create()` did + `SourceExists` then `CreateEventSource`, and that check-then-act cannot be made atomic, so the loser + of the race got `Source ServiceControl.Audit already exists on the local computer`. Now the create is + wrapped in `catch (ArgumentException) when (EventLog.SourceExists(SourceName))`, so losing the race + counts as success while a genuinely unusable source name still throws. + + Worth noting this was fixed in `EventSourceCreator` rather than in the `SetUpFixture`s that call it. + The same latent race exists in production, where `SetupCommand` and the ingestion fault policies + call it, so two instances being set up at once could hit it. One fix per copy covers every caller. + +When something else fails under the merged categories, check first whether it is a third instance of +this pattern rather than a genuine test defect. + +## Sequencing + +1. **Run the merged branch and compare against the baseline** (52 jobs / 208 runner-min / 14.5 min). + Job count and runner-minutes grade all four merges; wall clock grades the SQL Server merge. +2. If wall clock regressed, split `PrimarySqlServerAcceptance` back out of the + `SqlServerPersistence` category and re-measure. The other three merges are unaffected. +3. Watch for flakes, particularly RabbitMQ. All four suites deliberately `` + `NUnitParallelRunnerSettings.cs` and run serially today, so four-at-once against one broker is new + exposure. `max-parallel: 4` is one number in the matrix to dial down to 2 if they misbehave. +4. `LevelOfParallelism` bump on the acceptance assemblies. +5. In-job sharding of `PrimarySqlServerAcceptance`, if step 4 was not enough. +6. Windows Defender exclusion spike. + +The `ci-scoping-decisions` note records that `build_once` was rejected on measurement rather than +argument. Same bar here: everything above is estimates until a run says otherwise. diff --git a/src/ServiceControl.Audit/Infrastructure/EventSourceCreator.cs b/src/ServiceControl.Audit/Infrastructure/EventSourceCreator.cs index 004fd541e4..ab224077ba 100644 --- a/src/ServiceControl.Audit/Infrastructure/EventSourceCreator.cs +++ b/src/ServiceControl.Audit/Infrastructure/EventSourceCreator.cs @@ -1,5 +1,6 @@ namespace ServiceControl.Audit.Infrastructure { + using System; using System.Diagnostics; using System.Runtime.Versioning; @@ -8,10 +9,22 @@ static class EventSourceCreator [SupportedOSPlatform("windows")] public static void Create() { - if (!EventLog.SourceExists(SourceName)) + if (EventLog.SourceExists(SourceName)) + { + return; + } + + try { EventLog.CreateEventSource(SourceName, null); } + // An event source is machine-wide, and the check above cannot be made atomic with the + // create. Anything else running this at the same time, another instance being set up or, + // in CI, another test assembly, can get there first. The filter keeps that case distinct + // from an ArgumentException that means the source name itself is unusable. + catch (ArgumentException) when (EventLog.SourceExists(SourceName)) + { + } } public const string SourceName = "ServiceControl.Audit"; diff --git a/src/ServiceControl/Infrastructure/Installers/EventSourceCreator.cs b/src/ServiceControl/Infrastructure/Installers/EventSourceCreator.cs index a40ed882aa..e765c5a7b1 100644 --- a/src/ServiceControl/Infrastructure/Installers/EventSourceCreator.cs +++ b/src/ServiceControl/Infrastructure/Installers/EventSourceCreator.cs @@ -1,5 +1,6 @@ namespace ServiceBus.Management.Infrastructure.Installers { + using System; using System.Diagnostics; using System.Runtime.Versioning; @@ -8,10 +9,22 @@ static class EventSourceCreator [SupportedOSPlatform("windows")] public static void Create() { - if (!EventLog.SourceExists(SourceName)) + if (EventLog.SourceExists(SourceName)) + { + return; + } + + try { EventLog.CreateEventSource(SourceName, null); } + // An event source is machine-wide, and the check above cannot be made atomic with the + // create. Anything else running this at the same time, another instance being set up or, + // in CI, another test assembly, can get there first. The filter keeps that case distinct + // from an ArgumentException that means the source name itself is unusable. + catch (ArgumentException) when (EventLog.SourceExists(SourceName)) + { + } } public const string SourceName = "ServiceControl"; From ff79492ae14b00071499ff896fc0086f4aba4b87 Mon Sep 17 00:00:00 2001 From: John Simons Date: Sat, 15 Aug 2026 14:20:17 +1000 Subject: [PATCH 4/5] Prevent log messages from being included in test run collection Switches from capturing loop output to using an explicit list to ensure diagnostic messages (Write-Output) do not pollute the collection of test runs. This prevents invalid "dotnet test" executions that would occur if strings were accidentally included in the runs array. --- tools/run-tests.ps1 | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/run-tests.ps1 b/tools/run-tests.ps1 index 87adca9c04..98636da43d 100644 --- a/tools/run-tests.ps1 +++ b/tools/run-tests.ps1 @@ -35,7 +35,9 @@ Write-Output "Max parallel test runs = $MaxParallel" $reportWarningsValue = if ($ReportWarnings) { 'true' } else { 'false' } $isUnix = $PSVersionTable.Platform -eq 'Unix' -$runs = foreach ($project in $projectPaths) { +$runs = [Collections.Generic.List[object]]::new() + +foreach ($project in $projectPaths) { $frameworks = @( (Select-Xml -Path $project -XPath "/Project/PropertyGroup/TargetFramework").Node.InnerText (Select-Xml -Path $project -XPath "/Project/PropertyGroup/TargetFrameworks").Node.InnerText -split ';' @@ -51,16 +53,14 @@ $runs = foreach ($project in $projectPaths) { continue } - [pscustomobject]@{ - Label = "$(Split-Path $project -Leaf) ($framework)" - Project = $project - Framework = $framework - } + $runs.Add([pscustomobject]@{ + Label = "$(Split-Path $project -Leaf) ($framework)" + Project = $project + Framework = $framework + }) } } -$runs = @($runs) - if ($runs.Count -eq 0) { throw 'No test projects were runnable on this platform.' } From 48f249dd0509ae0d8faaae155acb04cff1a0c8a9 Mon Sep 17 00:00:00 2001 From: John Simons Date: Sat, 15 Aug 2026 15:00:10 +1000 Subject: [PATCH 5/5] Simplify log output when starting test runs Removes the RavenDB port information from the "Starting" log message to ensure a consistent output format for all test executions. --- tools/run-tests.ps1 | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tools/run-tests.ps1 b/tools/run-tests.ps1 index 98636da43d..5d47ac6085 100644 --- a/tools/run-tests.ps1 +++ b/tools/run-tests.ps1 @@ -115,12 +115,10 @@ while ($pending.Count -gt 0 -or $active.Count -gt 0) { # Set immediately before spawning, so the child inherits this run's value. Safe because # spawning is serialised here even though the runs themselves are not. $Env:ServiceControl_TESTS_RAVENDB_PORT = $nextPort - Write-Output "Starting $($run.Label) with RavenDB port $nextPort" $nextPort += $portSpacing } - else { - Write-Output "Starting $($run.Label)" - } + + Write-Output "Starting $($run.Label)" $run | Add-Member -NotePropertyName Process -NotePropertyValue ( Start-Process -FilePath 'dotnet' -ArgumentList $arguments -NoNewWindow -PassThru `