Improve handling of object change notifications during local run - #19079
Improve handling of object change notifications during local run#19079Karol Zadora-Przylecki (karolz-ms) wants to merge 6 commits into
Conversation
Karol Zadora-Przylecki (karolz-ms)
added
the
area-app-model
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Karol Zadora-Przylecki (karolz-ms)
left a comment
Karol Zadora-Przylecki (karolz-ms)
left a comment
There was a problem hiding this comment.
Code review findings: 1 test-coverage gap (validated empirically), 1 fragile-invariant issue, 1 pre-existing concurrency hazard, 1 test-realism issue, and 1 pre-existing resource leak. No bugs found in the core ProcessResourceChange identity logic — the isSameResource / isReplacement computation handles the no-previous-observation and empty-version edge cases correctly, and the Replaced → ResetResourceLogState ordering correctly precedes the terminal flush.
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19079Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19079" |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Aspire.Hosting/Dcp/DcpResourceWatcher.cs:355
terminalLogsHandledForActiveSubscribersmeans a flush was attempted, not that it completed. ForFailedToStartthe flush is deliberately point-in-time, and anExitedflush can time out; both leave_allLogsFlushedunset. Suppressing the subscriber-driven follow stream here then leaves an already-active subscriber with no stream to deliver logs that arrive after that incomplete flush, and an unchanged terminal object may never produce another notification. Start the normal stream whenever the flush did not complete.
var shouldStartStream =
resourceChange == ResourceChangeResult.Replaced ||
!terminalLogsHandledForActiveSubscribers;
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
|
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. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 821c5b12-84aa-42ad-8965-ef6983d09c41
d5a6767 to
f3ecc26
Compare
This comment has been minimized.
This comment has been minimized.
PR Testing ReportPR Information
Artifact Version Verification
Changes AnalyzedFiles Changed
Change Categories
The production change records Kubernetes UID together with resource version, recognizes a changed UID as a replacement even when the resource version is reused, and resets log-stream, terminal-flush, and deduplication state before processing the replacement. It also prevents an old overlapping stream from removing state owned by a newer stream. Test Scenarios ExecutedScenario 1: PR Artifact ProvenanceObjective: Confirm that runtime testing uses the CLI and packages built for the latest PR commit. Coverage type: Artifact integrity Status: PASS Steps and result:
Evidence:
Scenario 2: DCP Executor Regression SuiteObjective: Exercise the complete unit-test surface around the changed DCP watcher and logging lifecycle. Coverage type: Happy path, regression, concurrency, and boundary Status: PASS Command: dotnet test --project tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj \
--no-launch-profile -- \
--filter-class '*.DcpExecutorTests' \
--filter-not-trait 'quarantined=true' \
--filter-not-trait 'outerloop=true'Result: 212 total; 211 passed; 0 failed; 1 expected macOS platform skip. The run included the new and adjacent regression cases for:
Evidence:
Scenario 3: Real DCP Watch and Log LifecycleObjective: Validate the dogfood CLI and PR hosting packages in a real AppHost process, including a terminal resource, late log retrieval, and resource restart. Coverage type: Happy path and unhappy path Status: PASS Steps and result:
Expected unhappy-path outcome: The intentionally failing executable remains safely terminal with exit code 7, while its stderr remains available to a subscriber that attaches after termination. Evidence:
Non-Blocking ObservationTwo fresh Evidence:
Summary
Overall ResultPASS - PR #19079's changed behavior is verified. No blocking issue was found in the UID-aware watcher replacement handling or log-stream lifecycle. The separate starter-template compile anomaly is outside this PR's diff and did not reproduce in the minimal AppHost path used to exercise the changed hosting code. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c1ef331b-ff6c-4c40-935f-8ae4e6ee8872
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Aspire.Hosting/Dcp/DcpResourceWatcher.cs:604
- A restart notification can still be lost when it races natural EOF of the previous follow stream. Until the worker reaches
finally, the completed/finishing state remains in_logStreams, so thisTryAddfails and consumes the onlyLogsAvailabletrigger; the old worker can then remove itself, leaving an existing subscriber with no stream for the restarted resource. Coordinate the completing transition with registration (for example, atomically replace/retry a state marked as completing, or enqueue a retry after the existing state finishes) so this race cannot drop the restart.
// Register before starting the worker so an immediately completing stream can remove itself.
if (!_logStreams.TryAdd(resourceName, logStream))
{
logStream.Dispose();
return;
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. 48 / 100 test projects · 4 jobs, from 5 changed files. Selected test projects (48 / 100)
Selected jobs (4)
How these were chosen — grouped by what changed
🔧 show 43
🧪 📦 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. |
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
I did not find any new issues at the current head. The DCP executor suite passed, and the new UID/recreation tests caught targeted mutations in the old-resource and watch-restart paths.
Description
This is a follow-up to the post-merge review discussion on #18952.
During a local run, a DCP watch can reconnect after an object was deleted and recreated without observing the
Deletedevent. The previous replay deduplication keyed observations by object kind, name, and resource version, so a replacement object could be suppressed if its opaque resource version happened to equal the previous object's version. Per-resource log state could also remain associated with the old object incarnation.This change tracks each observation's Kubernetes UID with its resource version, treats a changed UID as a replacement, and clears stale log-stream and terminal-flush state before processing the replacement. Unchanged watch replays remain suppressed. The terminal-log comments now describe the exact retry behavior, and regression tests verify both replacement handling and late-subscriber log delivery.
User-facing behavior
Local runs now recognize recreated DCP objects even when deletion happened while the watch was disconnected. Late console-log subscribers continue to receive terminal-resource logs through the normal subscriber-driven follow stream, without relying on periodic watch replays.
Validation:
Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?