Repository navigation
Keep DCP watches draining and close abandoned connections - #20972
Conversation
Own watch responses before reading events and close them on cancellation or periodic restart. Drain streams independently of resource handlers while preserving event order and existing lifecycle dispatch. Handle failed Status payloads and retry interrupted or transiently failed watches. Add behavioral watch regressions over HTTPS/HTTP2 with a shared test certificate and retain the five-minute reconnection safeguard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a4d11434-2c0b-4741-91ea-9eacc92ca29a
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20972Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20972" |
github-actions
Bot
added
the
needs-area-label
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Retryable HTTP responses and malformed final records are not handled consistently.
2 open findings
What changed in this PR
This PR prevents abandoned or stalled DCP watches from blocking resource mutations.
Changes:
- Drains watch streams into an ordered, lossless channel.
- Adds explicit response disposal, framing, and status handling.
- Adds HTTPS/HTTP2 regression coverage.
| File | Description |
|---|---|
src/Aspire.Hosting/Dcp/KubernetesService.cs |
Decouples stream draining from consumers. |
src/Aspire.Hosting/Dcp/DcpKubernetesClient.cs |
Implements watch ownership and parsing. |
src/Aspire.Hosting/Dcp/DcpPipelineBuilder.cs |
Expands retry handling. |
tests/Aspire.Hosting.Tests/Dcp/KubernetesServiceTests.cs |
Adds watch and HTTP2 tests. |
tests/Aspire.Hosting.Tests/Dcp/DcpApiServerCertificateFixture.cs |
Provides the test certificate. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Distinguish incomplete JSON from complete malformed final records so missing newlines do not turn permanent parsing failures into retries. Apply one watch status policy to HTTP response failures and in-stream Status failures. Cover both response forms for every status code without duplicating theory rows, and add malformed final-record and partial JSON recovery scenarios. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a4d11434-2c0b-4741-91ea-9eacc92ca29a
Tests selector52 / 99 PR test projects · 4 PR jobs, from 5 changed files. Selected PR test projects (52 / 99)
Selected PR jobs (4)
How these were chosen — grouped by what changed
🔧 show 46
🧪 📦 affected project 📦 affected project 🧪 Job reasons
Selection computed for commit |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
PR Testing ReportPR Information
Artifact Version Verification
The install used the PR's dogfood command inside the unmodified repository container runner: curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20972The local source checkout matched the PR head and was clean. The testing did not modify repository files or push commits. Changes AnalyzedFiles Changed
Change Categories
Test Scenarios ExecutedScenario 1: Focused hosting regressionsObjective: Validate the actual changed watch paths and the unchanged executor/restart behavior that consumes them. Coverage type: Unit, HTTP2 integration, unhappy path, and boundary. Status: Passed: 367 passed, 0 failed, 1 Linux-only test skipped on Windows. Command: $env:MSBUILDTERMINALLOGGER = 'false'
dotnet test --project tests\Aspire.Hosting.Tests\Aspire.Hosting.Tests.csproj --no-build --no-launch-profile -- --filter-method '*.WatchAsync_*' --filter-method '*.WatchResourceRetryPipeline_*' --filter-method '*.DcpExecutorTests.*' --filter-method '*.PeriodicRestartAsyncEnumerableTests.*' --filter-not-trait 'quarantined=true' --filter-not-trait 'outerloop=true'
Coverage includes cancellation before the first event, response closure while a consumer is paused, draining and FIFO preservation, connection rotation, idle streams surviving API deadlines, UTF-8 and large fragmented records, complete final records without newlines, malformed complete records, truncated/empty streams, successful versus failed Status payloads, and retryable versus permanent HTTP/in-stream statuses. Executor coverage exercises resource state handling and restarts. Expected unhappy-path outcomes: Cancellation closes the HTTP2 stream; recoverable watch failures reconnect; malformed complete records and permanent failures propagate rather than appearing successful. Evidence: Scenario 2: PR-artifact starter orchestrationObjective: Exercise the shipping PR Hosting package with real DCP, project executables, service endpoints, health checks, and resource restart. Coverage type: Happy path and restart. Status: Passed. Project generation: cli=/workspace/.aspire/dogfood/pr-20972/bin/aspire
hive=/workspace/.aspire/hives/pr-20972/packages
version=$("$cli" --version)
"$cli" new aspire-starter --name WatchStarter --output /workspace/WatchStarter --source "$hive" --version "$version" --test-framework None --use-redis-cache false --localhost-tld false --suppress-agent-init --non-interactiveExecution: apphost=/workspace/WatchStarter/WatchStarter.AppHost/WatchStarter.AppHost.csproj
"$cli" start --apphost "$apphost" --isolated --non-interactive --format Json
"$cli" wait apiservice --status healthy --timeout 120 --apphost "$apphost" --non-interactive
"$cli" wait webfrontend --status healthy --timeout 120 --apphost "$apphost" --non-interactive
"$cli" describe --apphost "$apphost" --format Json --non-interactive
"$cli" resource apiservice restart --apphost "$apphost" --non-interactive
"$cli" wait apiservice --status healthy --timeout 120 --apphost "$apphost" --non-interactive
"$cli" wait webfrontend --status healthy --timeout 120 --apphost "$apphost" --non-interactive
"$cli" stop --apphost "$apphost" --non-interactive
"$cli" ps --format Json --non-interactiveObservations:
The functional HTTP requests followed HTTPS redirects and bypassed certificate verification only for these isolated, freshly generated development endpoints. Resource readiness used Evidence: Scenario 3: Concurrent transitions and controlled failure recoveryObjective: Verify that resource mutations and watched state changes continue to make progress during concurrent restart activity. Coverage type: Happy path, concurrency, and unhappy path. Status: Passed. Generated a separate Execution pattern: apphost=/workspace/WatchTransitions/apphost.cs
"$cli" start --apphost "$apphost" --isolated --non-interactive --format Json
"$cli" resource worker0 stop --apphost "$apphost" --non-interactive
"$cli" wait worker0 --status down --timeout 60 --apphost "$apphost" --non-interactive
"$cli" resource worker0 start --apphost "$apphost" --non-interactive
"$cli" wait worker0 --status up --timeout 60 --apphost "$apphost" --non-interactiveEach of the four workers completed six stop/start rounds concurrently: 24 completed restart rounds, with successful waits after every transition. All four ended in The failing executable was then started. Its snapshot reported Expected unhappy-path outcome: A child-process failure must be reflected accurately and must not prevent subsequent resource start or state updates. Evidence: Scenario 4: Idle reconnection and shutdownObjective: Leave a real DCP executable watch idle beyond the normal five-minute connection-rotation interval, then verify subsequent resource transitions and cleanup. Coverage type: Boundary and recovery. Status: Passed. A separate PR-hive app declares one long-running executable. The scenario waited for it to be running, recorded UTC timestamps, executed a 320-second sleep, then stopped and started that resource with explicit down/up waits, checked the final snapshot, and stopped the exact AppHost. Observations:
This checks behavior after the normal rotation interval. The focused HTTP2 tests independently verify that a periodic restart actually closes and replaces a connection when consumer progress is paused. Expected boundary outcome: Resource updates must still arrive after the normal watch-rotation interval, and shutdown must complete without leaving a running test AppHost. Evidence: Environment and Harness Notes
Summary
Overall ResultPR VERIFIED for the tested scenarios: All four scenarios passed. No watcher regression was found. The final live PR status read reported 317 passing checks, 13 skipped checks, no pending checks, and no failing checks, with the head still at The validation does not claim to reproduce or conclusively fix the separate historical connected-but-silent watcher failure. It also does not establish bounded memory for an indefinitely blocked consumer; the unbounded, lossless queue is an intentional design tradeoff. AOT publishing and deployment were not tested. Artifacts and Posting
|
David Negstad (danegsta)
added
area-app-model
|
/backport to release/13.6 |
|
Started backporting to |
Description
Aspire can leave a DCP watch open without an active reader when watch setup is canceled or resource handlers pause event consumption. An unread HTTP/2 watch can exhaust its receive window and block DCP watcher publication and resource mutations.
This change explicitly owns watch responses as soon as headers arrive and closes them on cancellation or periodic restart. An independent reader drains each stream into a lossless queue, so handler latency does not stop network reads. Resource event ordering and existing Aspire lifecycle callback dispatch are preserved.
The reader distinguishes successful and failed Status payloads, surfaces malformed complete records, and recovers from truncated streams and retryable watch errors. The existing five-minute reconnection safeguard remains in place; this does not claim to resolve the separate historical connected-but-silent watch behavior. Lossless buffering means a persistently blocked handler can accumulate queued events rather than backpressure the API server.
Related to #20609 and microsoft/dcp#286. These are client-side fixes, not a replacement for the API server's slow-consumer handling. No dependency updates are required.
Validation
Behavioral regressions cover cancellation, paused consumers, event ordering, periodic reconnection, framing, and error recovery. All KubernetesServiceTests now use HTTPS/HTTP2 with one shared, test-owned certificate; certificate trust validation is disabled only in test clients.
Passed on Windows: 98 tests, 0 failures.
Checklist
<remarks />and<code />elements on your triple slash comments?