Re-enable Microsoft.Extensions.Hosting shutdown functional coverage in CI and harden shutdown startup/Helix/NET481 compatibility#128252
Conversation
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f2762648-8aaf-44cd-a0bb-269b2a6b371d Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
|
@copilot , the shutdown tests are failing with this error on helix machines: Comments Error message Stack trace |
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e8bb9c1b-1794-4ebc-a563-bec6823fec1c Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
... Fixed in b9cd702. I removed the source-tree lookup and switched the shutdown tests to run TestApp from the deployed test output ( |
|
@copilot , this time it failed with this error: Test Failures (700 tests failed) ❌ Microsoft.AspNetCore.Hosting.FunctionalTests.ShutdownTests.ShutdownTestRun [Console] [Details] [Artifacts] [0.70% failure rate] |
Note AI-generated. Implemented in |
|
@copilot , there are still two comments from me unresolved, process them. |
Note AI-generated. The two remaining threads are already addressed on the current branch:
I re-validated the branch locally: |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
…plicate in ShutdownTests Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
…on-hosting' into copilot/enable-functional-tests-on-hosting Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Hosting/tests/FunctionalTests/ShutdownTests.cs:83
OutputReceivedis raised from background threads, butoutputis being mutated viaoutput += ...without synchronization. This can lead to lost/garbled output (and flaky assertions) when multiple lines arrive quickly. Consider using a thread-safe accumulator (e.g., aStringBuilderguarded by a lock, or enqueue lines into a concurrent collection and join at the end) rather than mutating a shared string from the event callback.
var started = new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously);
var completed = new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously);
var output = string.Empty;
deployer.OutputReceived += (sender, args) =>
{
if (!string.IsNullOrEmpty(args.Data) && args.Data.StartsWith(StartedMessage, StringComparison.Ordinal))
{
output += args.Data.Substring(StartedMessage.Length) + '\n';
started.TrySetResult(0);
}
This PR now includes the full follow-up hardening required by CI feedback after re-enabling
Microsoft.Extensions.Hostingfunctional shutdown coverage.Final scope of changes
AppContext.BaseDirectory) instead of source-tree project discovery.PreservePublishedApplicationForDebuggingdependency by using a borrowed published app path that does not delete test output on cleanup.PublishedApplication.Dispose()is now virtual;BorrowedPublishedApplicationoverrides it as a no-op so teardown never deletes the test output directory.killprocess usage in shutdown signaling withlibcsignal calls for Helix/minimal-image compatibility.libckillP/Invoke inProcessExtensions.SendSignal(int pid, int signal); removed the duplicate declaration fromShutdownTestsso both signal paths share a single interop entry point. The underlying P/Invoke method is namedKill(PascalCase) withEntryPoint = "kill"to match C# naming conventions while preserving the correct libc symbol binding.SendSignalexplicitly throwsPlatformNotSupportedExceptionon Windows so callers get a clear, actionable error rather than aDllNotFoundExceptionfrom thelibcP/Invoke.SIGINT/SIGTERMnumbers at runtime viaSystemNative_GetPlatformSignalNumberinstead of hard-coding them, matching the runtime's platform-specific signal mapping approach. Signal number initialization is gated behind an OS check so Windows does not attempt the P/Invoke during type initialization.SIGTERMvia libc)Process.Kill()) when graceful termination times out or failsProcess.Kill()to 30 seconds, reducing false failures when the app has finished shutdown logging but needs a little longer to exit cleanly under load.DeployAsync()starts process output capture viaSelfHostDeployer.OutputReceived(nowinternal), so the initialStartedline cannot be missed.net481compatibility by usingMarshal.GetLastWin32Error().WaitForExitOrKillrace: now uses the boolean result fromWaitForExit(timeout)directly and wrapsProcess.Kill()in acatch (InvalidOperationException)to tolerate the window where the process exits between the wait and the kill call.Validation
./build.sh clr+libs -rc releasesucceeds../build.sh clr+libs+host -rc release -lc releasesucceeds.Microsoft.Extensions.Hosting.Functional.Testsbuilds successfully fornet481.dotnet build /t:test Microsoft.Extensions.Hosting.Functional.Tests.csproj -c Release -f net11.0 /p:TestFilter=Microsoft.AspNetCore.Hosting.FunctionalTests.ShutdownTestspasses (2/2).