Fix Windows test shutdown file lock failures - #20406
David Negstad (danegsta) wants to merge 4 commits into
Conversation
Use an independent teardown timeout for explicit-start tests and forward DCP diagnostics only after hosted services have stopped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20406Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20406" |
github-actions
Bot
added
the
area-app-model
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
All reviewed changes are covered with no unresolved issues.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes Windows test shutdown failures caused by DCP diagnostic file locks and shared timeout budgets.
Changes:
- Moves DCP log forwarding to
StoppedAsync. - Adds independent teardown timeouts.
- Adds locked-file regression coverage.
| File | Description |
|---|---|
tests/Shared/DistributedApplicationTestingBuilderExtensions.cs |
Defers DCP log forwarding until hosted services stop. |
tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs |
Adds independent shutdown timeouts. |
tests/Aspire.Hosting.Testing.Tests/TestingBuilderTests.cs |
Verifies forwarding after file locks are released. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| using var shutdownCts = AsyncTestHelpers.CreateDefaultTimeoutTokenSource(TestConstants.DefaultOrchestratorTestLongTimeout); | ||
| await app.StopAsync(shutdownCts.Token).DefaultTimeout(TestConstants.DefaultOrchestratorTestLongTimeout); |
There was a problem hiding this comment.
Why is this needed? Why in only a couple of tests? Every test is calling StopAsync. What about setting a stop timeout on the host instead when programs are created? In other words, look for a way to make this automatically applied in every test.
There was a problem hiding this comment.
Updated to apply this more universally for anything using TestDistributedApplicationBuilder.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Configure a dedicated CI-scaled host shutdown budget and avoid reusing startup cancellation tokens during teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
| public static readonly int ExtraLongTimeoutDuration = 60 * 1000 * 3 * (PlatformDetection.IsRunningOnCI ? 2 : 1); // 180 sec, 360 sec in CI -- useful when a docker image might need pulling | ||
| public static readonly int DefaultOrchestratorTestTimeout = 15 * 1000 * (PlatformDetection.IsRunningOnCI ? 2 : 1); // 15 sec, 30 sec in CI | ||
| public static readonly int DefaultOrchestratorTestLongTimeout = 45 * 1000 * (PlatformDetection.IsRunningOnCI ? 4 : 1); // 45 sec, 180 sec in CI | ||
| public static readonly int DefaultOrchestratorTestShutdownTimeout = 15 * 1000 * (PlatformDetection.IsRunningOnCI ? 3 : 1); // 15 sec, 45 sec in CI |
There was a problem hiding this comment.
Just use DefaultOrchestratorTestTimeout. Make it 45 sec in CI if required.
Want to avoid a lot of slightly different timeouts.
There was a problem hiding this comment.
I moved this into a deeper shared layer so it can apply more consistently, but it also makes using the shared constants harder. I think the current approach is a reasonable tradeoff.
| testProgram.AppBuilder.Services.Configure<HostOptions>(options => | ||
| options.ShutdownTimeout = TimeSpan.FromMilliseconds(TestConstants.DefaultOrchestratorTestShutdownTimeout)); |
There was a problem hiding this comment.
Does this actually fix the problem though? I was just throwing it out as an idea.
An error gets thrown when it hits the timeout, right? Won't that fail the test. These timeouts exist so tests don't hang, not to make them non-flaky.
There was a problem hiding this comment.
If tests are taking 30 seconds to shutdown it sounds like that is the problem that should be addressed, not making timeouts longer.
There was a problem hiding this comment.
We haven't touched DCP shutdown in quite some time; this is very likely CI contention. We can independently spend cycles looking if we can squeeze more shutdown performance out of DCP, but in general there's a floor dictated by our graceful shutdown attempts. Given that I've seen the affected tests failing itermitantly across several PRs recently, I'd argue bumping our shutdown timeout is a reasonable approach if it increases how often tests pass on the first run.
There was a problem hiding this comment.
Actually, that separate DCP investigation turned up an interesting candidate; basically under load the underlying k8s API server we're using can switch the order events are delivered. So the tests are potentially hanging because we never actually see that a resource was deleted: microsoft/dcp#275
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Tests selector40 / 99 PR test projects · 0 PR jobs, from 7 changed files. Selected PR test projects (40 / 99)
Selected PR jobs (0)none How these were chosen — grouped by what changed
🧪 show 34
🧪 🧪 🧪 🧪 🧪 Job reasonsnone Selection computed for commit |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
It has a compilation failure and unresolved shutdown-token and timeout-coverage issues.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Resolved since last review (1)
| public static class DistributedApplicationTestingBuilderExtensions | ||
| { | ||
| private static readonly TimeSpan s_defaultHostShutdownTimeout = | ||
| TimeSpan.FromSeconds(PlatformDetection.IsRunningOnCI ? 45 : 30); // 30 sec, 45 sec in CI |
There was a problem hiding this comment.
Just set to TestConstants.LongTimeoutTimeSpan
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
This may actually be a weird bug in our k8s API Server (see microsoft/dcp#275) |
Description
Windows test runs can take long enough to stop DCP-managed child processes that a timeout shared with startup expires during teardown. DCP diagnostic forwarding also ran before the orchestrator stopped those processes, so the diagnostic files could still be exclusively locked when the test tried to read them.
This change stops reusing startup and resource-operation cancellation tokens during teardown. Standard repository test builders and
TestProgramfixtures now preserve Generic Host's 30-second local shutdown budget while allowing 45 seconds on slower CI machines. DCP diagnostic forwarding runs fromIHostedLifecycleService.StoppedAsync, after all hosted services completeStopAsync, so Windows processes have released their diagnostic files before the tests read them.A focused regression test holds a DCP log open during
StopAsyncand verifies that it is forwarded only duringStoppedAsync.Validation:
DcpLogForwarderReadsLogsAfterHostedServicesStopBeforeResourceStartedEvent_NotFiredForExplicitStartOnInitialCreationBeforeResourceStartedEvent_FiredWhenExplicitStartResourceIsManuallyStartedBeforeResourceStartedEvent_FiredForNormalResourcesOnInitialStartupVerifyWithHttpHealthCheckBlocksDependentResourcesParentProcessLifetimeReusesResourcesAcrossAppRestartsAndStopsWhenParentExitsFixes #15223
Checklist
<remarks />and<code />elements on your triple slash comments?