Skip to content

Give RemoteHost tests thread-pool headroom above xunit's synchronous slots - #21613

Merged
David Fowler (davidfowl) merged 3 commits into
mainfrom
fix/remotehost-discovery-stall
Nov 1, 2026
Merged

David Fowler (davidfowl) merged 3 commits into
mainfrom
fix/remotehost-discovery-stall

Conversation

@davidfowl

@davidfowl David Fowler (davidfowl) commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Fixes #21612. Part of #21600.

IntegrationHostLauncherTests.InitializeHostsAsync_WaitsForEveryRegistrationBeforeDiscovery intermittently fails on Windows with System.TimeoutException at the 10s initialization.WaitAsync (line 124).

7-day frequency (main ci.yml, attempt 1):

Runner Failed jobs Total jobs
windows-latest Hosting.RemoteHost 3 57
ubuntu Hosting.RemoteHost 0 57

The failing jobs were 122368418345, 122578764141 and 122961611924. Passing Windows runs still take 4.2–6.8s, compared with 0.06s on Linux and 0.1s locally, so the test is close to timing out even when it passes.

Root cause: thread-pool starvation

A temporary diagnostics commit on this PR, now dropped, captured thread-pool state on Windows CI in run 41238866885:

Diagnostic Observed on Windows CI
Processors / minimum workers 4 / 4
Thread pool 5 threads, 4–5 busy, 16 pending work items, constant for 3.7s
First-host getCapabilities discovery 3.79s, completing only after the queue drained
GC counts over the same window climbed to 72/32/2, from concurrent CPU-heavy ExportApi_* and ScanAssembly_* tests

The chain of events:

  1. xunit v3's conservative scheduler (ExecutionScheduler._Conservative) sets the thread-pool minimum to Math.Max(4, maxParallelThreads). That is exactly the number of synchronous tests it can run at once.
  2. Long CPU-bound synchronous tests, plus the test infrastructure, occupy every worker at that floor.
  3. Hill climbing does not inject more threads, so JSON-RPC continuations wait in the queue for 4–11s.

Fix

A [ModuleInitializer] in the RemoteHost test assembly (TestThreadPoolHeadroom) raises the minimum worker threads to 2 * max(4, ProcessorCount). This leaves headroom above xunit's synchronous slots for async continuations.

  • No product code changes.
  • The original test's 10s budget is unchanged.
  • No sleeps, retries or weakened assertions.

Regression test

IntegrationHostDiscoveryThreadPoolTests.InitializeHostsAsync_CompletesWhileSynchronousTestsOccupyEverySchedulerSlot runs in a DisableParallelization collection:

  1. Caps the maximum worker threads at the floor, to model the CI behaviour where the pool did not grow.
  2. Occupies SynchronousTestSlots workers with blocking work items.
  3. Runs real InitializeHostsAsync discovery over two JSON-RPC test connections.

Results:

  • Before the fix (initializer disabled): failed 3 of 3 runs after 15.1s with Capability discovery could not complete while 4 synchronous tests occupied the thread pool (minimum workers: 4, slots occupied: False, discovered: False).
  • After the fix: passed 10 of 10 local runs of the new test together with the IntegrationHostLauncherTests, ApiReferenceExportTests and AtsCapabilityScannerTests classes (92 tests, 0 failures each run).

CI evidence (windows-latest Hosting.RemoteHost, from TRX)

Run Head WaitsForEveryRegistrationBeforeDiscovery New regression test
41240599325 403a16f 0.219s, passed 0.005s, passed
41243021078 433cda8 0.285s, passed 0.006s, passed

On main, Windows runs of WaitsForEveryRegistrationBeforeDiscovery took 4.2–11.7s, and the failing runs timed out at 10s. The full CI run passed 43/43 jobs on 433cda8.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
    • No

🤖 Posted by AspireAgent (AI Agent)

@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 21613

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 21613"

@github-actions github-actions Bot added the area-integrations Issues pertaining to Aspire Integrations packages label Oct 31, 2026
@github-actions

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

This diagnostic-only revision still needs Windows CI trace analysis to establish the stall’s cause, with the intended fix and regression test pending.

No open issues 🎉

What changed in this PR

This work-in-progress PR adds temporary diagnostics to investigate the Windows-only stall in RemoteHost initialization tracked by #21612. It does not yet implement the fix or add regression coverage.

Changes:

  • Records initialization milestones and capability-discovery logs.
  • Samples thread-pool activity and garbage-collection counters every 250 ms on a background thread.
  • Flushes diagnostics to test output whether the initialization wait succeeds or throws.
File Description
tests/​Aspire.Hosting.RemoteHost.Tests/​IntegrationHostLauncherTests.cs Instruments the affected test with phase logging, runtime sampling, and diagnostic output helpers.

Review effort: Balanced

…slots (#21612)

xunit v3's conservative scheduler pins the thread-pool minimum to the number
of synchronous tests it runs concurrently. On 4-core Windows runners long
CPU-bound sync tests occupied every worker at that floor and JSON-RPC
continuations for capability discovery queued for 4-11s, timing out
InitializeHostsAsync_WaitsForEveryRegistrationBeforeDiscovery.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@davidfowl
David Fowler (davidfowl) force-pushed the fix/remotehost-discovery-stall branch from ce75c86 to 403a16f Compare October 31, 2026 20:48
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot October 31, 2026 20:48
@davidfowl David Fowler (davidfowl) changed the title Fix Windows stall in RemoteHost InitializeHostsAsync_WaitsForEveryRegistrationBeforeDiscovery Give RemoteHost tests thread-pool headroom above xunit's synchronous slots Oct 31, 2026
@github-actions

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The test can dispose synchronization events before queued callbacks finish, risking an unhandled ObjectDisposedException that terminates the test process; callbacks must be drained before disposal.

1 open issue

Review effort: Balanced

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues are recorded for the test-only thread-pool headroom changes and discovery regression coverage.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@davidfowl
David Fowler (davidfowl) marked this pull request as ready for review October 31, 2026 21:43
@davidfowl David Fowler (davidfowl) added the ci-health Fixes CI flakiness or CI infrastructure (AspireAgent queue) label Nov 1, 2026 — with Copilot SWE Agent
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Discovery cleanup now waits for any timed-out operation before subsequent tests run.

@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot November 1, 2026 18:28
@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Tests selector

1 / 100 PR test projects · 0 PR jobs, from 2 changed files.

Selected PR test projects (1 / 100)

Aspire.Hosting.RemoteHost.Tests

Selected PR jobs (0)

none


How these were chosen — grouped by what changed

🧪 tests/Aspire.Hosting.RemoteHost.Tests/IntegrationHostDiscoveryThreadPoolTests.cs (changed test)
→ 1 directly: Aspire.Hosting.RemoteHost.Tests

🧪 tests/Aspire.Hosting.RemoteHost.Tests/TestServices/TestThreadPoolHeadroom.cs (changed test)
→ 1 directly: Aspire.Hosting.RemoteHost.Tests

Job reasons

none


Selection computed for commit 2ed7818.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues remain in the test-only changes that add thread-pool headroom for asynchronous JSON-RPC discovery.

No open issues 🎉

Review effort: Balanced

@davidfowl
David Fowler (davidfowl) merged commit 9ea245a into main Nov 1, 2026
50 checks passed
@microsoft-github-policy-service microsoft-github-policy-service Bot added this to the 17.0 milestone Nov 1, 2026
@aspire-repo-bot

Copy link
Copy Markdown
Contributor

✅ No documentation update needed.

docs_optional → test_only. No signals triggered (signal_count 0). Both changed files match tests/** (tests/Aspire.Hosting.RemoteHost.Tests/...), so no documentation is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-integrations Issues pertaining to Aspire Integrations packages ci-health Fixes CI flakiness or CI infrastructure (AspireAgent queue)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Failing test]: Aspire.Hosting.RemoteHost.Tests.IntegrationHostLauncherTests.InitializeHostsAsync_WaitsForEveryRegistrationBeforeDiscovery

3 participants