Skip to content

test: synchronize rejected-create cancellation with response observation - #21417

Closed
David Fowler (davidfowl) wants to merge 1 commit into
mainfrom
agent/fix-sandbox-cancellation-phase
Closed

David Fowler (davidfowl) wants to merge 1 commit into
mainfrom
agent/fix-sandbox-cancellation-phase

Conversation

@davidfowl

Copy link
Copy Markdown
Collaborator

Description

Fixes #20931's test-phase guarantee in AzureDevComputeClientKeepsCancellationAfterRejectedCreateDefinite; no production behavior changes.

The test starts a 10 ms cancellation timer before calling the client, but expects cancellation after the client has observed a rejected create. Those phases have no ordering guarantee: default HttpClient.SendAsync includes response buffering, and cancellation before it returns is correctly classified as potentially ambiguous.

Start with an unarmed cancellation source and arm the same 10 ms interval when the client disposes the observed Forbidden response, immediately before its existing retry delay. Preserve the configured one-minute retry delay, AzureDevComputeCreateException, definite ResponseMayHaveBeenLost == false, and exact TaskCanceledException assertions; add an exact one-send assertion.

Only tests/Aspire.Hosting.Azure.Tests/AzureSandboxesTests.cs changes (+25/-2), including a minimal response-disposal callback helper. No dependencies, production changes, timeout increases, retries, skips, diagnostic logging, buffering instrumentation or workflow changes are included.

Native evidence and limits

Controlled counterexample — run 40968736046, attempt 1, at f884f0a: normal build had zero warnings/errors; six actual cases executed, five passed and one deliberately failed the original definite assertion. Cancellation during actual response buffering produced one send and ResponseMayHaveBeenLost == true before the repository client observed the rejection. TaskCanceledException was logged; the later type assertion was not executed after Assert.False failed.

Native qualification — run 40971773666, attempt 1, job 123790016435: normal restore/build completed with zero warnings/errors and seven unique actual cases passed, with zero failures, skips or unknown outcomes and no observed duplicate/retry executions.

Actual qualification cases Result
Repaired original, including definite/type/one-send assertions 1 pass, 14 ms
Canceled-send / observed-Forbidden disposal controls 2 passes; ambiguous true / false
Buffering cancellation before observation / same 10 ms timer armed after observation 2 passes; ambiguous true / false, exact exception types and one send
Existing rejected-response / pre-dispatch cancellation facts 2 passes

The complete native TRX was downloaded and checked, including unique test/execution IDs and phase-marker ordering. The pre-observation case retains the correct ambiguous production contract; it does not weaken the repaired original's definite assertion.

Qualification ran at a separate diagnostic child, 5c3c411, whose parent is this clean repair head 5ead38f. The repaired method and callback helper are byte-identical between them. The diagnostic child and its workflow are not included in this PR. Normal CI/Final Results for this clean PR head remain required; qualification-child success is not a substitute.

This is controlled phase-boundary evidence, not reproduction of the historical natural failure. The original timer-based test passed before the repair as well; do not read these results as “old original red, new original green” or as a production service fix. The repaired test explicitly orders cancellation after response observation rather than relying on elapsed time from before dispatch.

This PR is deliberately draft for review and normal CI. No merge is requested by this publication.

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 — this is a unit-test repair with an added one-send assertion; native adjacent/control qualification is linked above.
    • 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

🤖 Posted by AspireAgent (AI Agent)

Arm the existing 10 ms cancellation interval when the client disposes the observed Forbidden response, rather than before the HTTP send. Preserve the definite cancellation assertions and verify only one send.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@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 -- 21417

Or

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

@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 29, 2026
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot October 29, 2026 23:32
@github-actions

Copy link
Copy Markdown
Contributor

Tests selector

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

Selected PR test projects (2 / 100)

Aspire.Hosting.Azure.Kubernetes.Tests, Aspire.Hosting.Azure.Tests

Selected PR jobs (0)

none


How these were chosen — grouped by what changed

🧪 tests/Aspire.Hosting.Azure.Tests/AzureSandboxesTests.cs (changed test)
→ 1 directly: Aspire.Hosting.Azure.Tests
→ 1 via the project graph: Aspire.Hosting.Azure.Kubernetes.Tests

Job reasons

none


Selection computed for commit 5ead38f.

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 for the test-only change that synchronizes cancellation with disposal of the rejected create response.

No open issues 🎉

What changed in this PR

Addresses the test timing race in #20931 by ordering cancellation after the client observes a rejected create response. Production behavior is unchanged.

Changes:

  • Starts the existing 10 ms cancellation timer when the Forbidden response is disposed, using a small callback helper.
  • Preserves the one-minute retry delay and assertions that cancellation remains definite with an exact TaskCanceledException.
  • Adds an assertion that exactly one request is sent.
File Description
tests/​Aspire.Hosting.Azure.Tests/​AzureSandboxesTests.cs Synchronizes cancellation with response disposal and verifies a single send.

Review effort: Balanced

Copy link
Copy Markdown
Collaborator Author

This conflict comes from #21591, which has merged into main with a different fix for the same test, AzureDevComputeClientKeepsCancellationAfterRejectedCreateDefinite. That fix cancels when the client logs the 403 retry warning, after the rejection has been observed. It also asserts that exactly one request is sent, which this PR asserts too. Resolving the conflict would either leave no diff or replace the approach already on main, so I haven't pushed a merge. I suggest closing it as superseded by #21591.

🤖 Posted by AspireAgent (AI Agent)

Copy link
Copy Markdown
Collaborator Author

Closing as superseded by #21591 (same fix).

🤖 Posted by AspireAgent (AI Agent)

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

Labels

needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

2 participants