Skip to content

Fix flaky RunCommand_DetachedChild_WhenSignaledBeforeReadiness test timeout - #18702

Merged
Ankit Jain (radical) merged 3 commits into
mainfrom
copilot/fix-runcommandtests-timeout
Jul 9, 2026
Merged

Fix flaky RunCommand_DetachedChild_WhenSignaledBeforeReadiness test timeout#18702
Ankit Jain (radical) merged 3 commits into
mainfrom
copilot/fix-runcommandtests-timeout

Conversation

Copilot AI commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Description

RunCommand_DetachedChild_WhenSignaledBeforeReadiness_AwaitsAppHostTeardownBeforeExit times out intermittently on CI due to a FakeTimeProvider reentrancy deadlock.

Root cause: The test callback used await Task.Delay(duration, timeProvider) to simulate teardown time. When timeProvider.Advance() fires that timer under its internal lock, the async continuation chain can transitively attempt to dispose another timer on the same provider (the WaitAsync(8s, timeProvider) timeout in RunCommand's finally block), deadlocking when inline continuations execute under thread pool contention.

Fix: Replace the Task.Delay(timeProvider) in the callback with a test-controlled TaskCompletionSource, eliminating all FakeTimeProvider reentrancy while preserving the same behavioral guarantee (CLI waits for AppHost teardown before exiting).

// Before: timer-based, susceptible to FakeTimeProvider lock reentrancy
await Task.Delay(RunCommand.s_gracefulShutdownBudget + TimeSpan.FromSeconds(1), timeProvider, CancellationToken.None);

// After: test-controlled gate, deterministic
await teardownCanFinish.Task;

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
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No

Replace the FakeTimeProvider-based Task.Delay in the test callback with a
test-controlled TaskCompletionSource. This eliminates potential reentrancy
issues where FakeTimeProvider.Advance() fires a timer whose async
continuation chain disposes another timer on the same provider, which can
deadlock intermittently under thread pool contention on CI.

Closes #18677

Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Copilot AI requested review from Copilot and removed request for Copilot July 9, 2026 07:30
Copilot AI changed the title [WIP] Fix timeout issue in RunCommand_DetachedChild test Fix flaky RunCommand_DetachedChild_WhenSignaledBeforeReadiness test timeout Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026

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 -- 18702

Or

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

@github-actions

This comment has been minimized.

@JamesNK
James Newton-King (JamesNK) marked this pull request as ready for review July 9, 2026 07:58
Copilot AI review requested due to automatic review settings July 9, 2026 07:58

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.

Pull request overview

This PR fixes an intermittently-timing-out test, RunCommand_DetachedChild_WhenSignaledBeforeReadiness_AwaitsAppHostTeardownBeforeExit. The test simulates an AppHost that receives a termination signal before its backchannel is established and asserts the detached CLI child waits for AppHost teardown before exiting. The previous implementation simulated teardown time with Task.Delay(..., timeProvider) and drove it via FakeTimeProvider.Advance(), which could deadlock: advancing the fake clock fires a timer whose continuation transitively disposes another timer on the same provider (the finally-block WaitAsync timeout), and inline continuations under thread-pool contention could reenter the provider's internal lock.

Changes:

  • Replaced the Task.Delay(timeProvider) teardown simulation with a test-controlled TaskCompletionSource (teardownCanFinish) gate.
  • Removed the timeProvider.Advance(...) calls that drove the timer, making the assertion that the child does not exit mid-teardown fully deterministic.
  • Updated surrounding comments to describe the new gating mechanism.

Comment thread tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 9, 2026 09:00

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.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Tests selector (audit mode)

The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement.

1 / 99 test projects · 2 jobs, from 1 changed file.

Selected test projects (1 / 99)

Aspire.Cli.Tests

Selected jobs (2)

cli-starter, extension-e2e


How these were chosen — grouped by what changed

🧪 tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs (changed test)
1 directly: Aspire.Cli.Tests

Job reasons

Job Triggered by
cli-starter selected test Aspire.Cli.Tests
extension-e2e tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs

Selection computed for commit 3ad999d.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@radical Ankit Jain (radical) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[automated] ## Validation: reproduced the flake without the fix, confirmed the fix makes it deterministic

Independently validated this fix by reproducing the deadlock mechanism, confirming the fix eliminates it, and reviewing the diff.

Root cause

The pre-fix test drives teardown through fake time:

teardownStarted.TrySetResult();
await Task.Delay(RunCommand.s_gracefulShutdownBudget + TimeSpan.FromSeconds(1), timeProvider, ...); // callback
...
timeProvider.Advance(s_gracefulShutdownBudget + 100ms); // driver
timeProvider.Advance(TimeSpan.FromSeconds(1));

teardownStarted is a RunContinuationsAsynchronously TCS, so its continuation — the Task.Delay(timeProvider) that registers the fake timer — is posted to the thread pool. Under pool contention the driver's Advance() calls can execute before that timer registers. The timer then anchors to the already-advanced clock, is scheduled in the future, and never firespendingCommand hangs and await pendingCommand.DefaultTimeout() throws TimeoutException at RunCommandTests.cs:469, which is exactly the signature in #18677. Rare because it needs a specific cross-thread interleaving (only 2 known CI occurrences).

Reproduced without the fix

Standalone harness replaying both the pre-fix and post-fix patterns against Microsoft.Extensions.TimeProvider.Testing 10.6.0 under a starved thread pool (min=1, max=2, plus background load):

Pattern Iterations Deadlocks
pre-fix (Task.Delay(tp) + Advance) 1500 2 (~0.13%)
post-fix (this PR) 1500 0

Deadlock signature: runTask.IsCompleted=False after advancing past the timer — the fake timer never fired, matching the CI hang.

Confirmed the fix

  • Harness post-fix pattern: 0 / 1500 deadlocks under identical contention.
  • Real fixed test: builds, passes, 20 / 20 repeat runs locally.

Why it's deterministic

Fake time is never advanced, so no timer ever fires → no registration-ordering race and no WakeWaiters reentrancy. The Assert.False(pendingCommand.IsCompleted) is now a true invariant rather than a timing gamble: the callback is blocked on teardownCanFinish, and RunCommand's finally awaits WaitAsync(s_detachedAppHostTeardownTimeout, _timeProvider) on a frozen clock, so pendingCommand cannot complete until teardownCanFinish.SetResult(). Synchronization reduces to well-defined TCS completion.

LGTM.

@radical
Ankit Jain (radical) merged commit 6d44d0d into main Jul 9, 2026
588 of 592 checks passed
@radical
Ankit Jain (radical) deleted the copilot/fix-runcommandtests-timeout branch July 9, 2026 19:37
@github-actions github-actions Bot added this to the 13.5 milestone Jul 9, 2026
@aspire-repo-bot

Copy link
Copy Markdown
Contributor

✅ No documentation update needed.

docs_optional → test_only: No documentation update needed. No signals triggered (signal_count = 0). The only changed file is tests/Aspire.Cli.Tests/Commands/RunCommandTests.cs, which is a pure test fix replacing a Task.Delay(timeProvider) with a TaskCompletionSource to eliminate a FakeTimeProvider reentrancy deadlock. This introduces no new public API, CLI options, or user-facing behavior changes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI Failure] Flaky: RunCommandTests.RunCommand_DetachedChild_WhenSignaledBeforeReadiness_AwaitsAppHostTeardownBeforeExit times out intermittently

4 participants