fix: retain deploy hook extension warnings - #9950
Open
Hui Miao (huimiu) wants to merge 7 commits into
Open
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Hui Miao (huimiu)
marked this pull request as ready for review
September 10, 2026 08:20
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 21 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The concurrency regression test does not execute any capture operations concurrently.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Retains deploy lifecycle extension warnings after preview/progress rendering completes.
Changes:
- Adds bounded, shared lifecycle stdout capture.
- Queues output during preview suppression and preserves JSON formatting.
- Ensures final deployment progress renders before queued output.
File summaries
| File | Description |
|---|---|
cli/azd/pkg/input/console.go |
Adds deferred preview teardown and output persistence. |
cli/azd/pkg/input/console_test.go |
Tests persistence, JSON output, and deferred teardown. |
cli/azd/internal/grpcserver/event_service.go |
Captures bounded deploy lifecycle stdout. |
cli/azd/internal/grpcserver/event_service_test.go |
Tests capture, truncation, and grouping. |
cli/azd/internal/cmd/up_graph.go |
Reorders final progress rendering and preview resumption. |
cli/azd/internal/cmd/up_graph_test.go |
Verifies final rendering order. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new tests use manual WaitGroup bookkeeping instead of the repository-required Go 1.26 WaitGroup.Go pattern.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
cli/azd/internal/grpcserver/event_service_test.go:517
- Use
writeWg.Gofor this second goroutine group as well. The repository's Go 1.26 modernization rules requireWaitGroup.Gorather than manualAdd/Donebookkeeping (cli/azd/AGENTS.md:338-395).
var writeWg sync.WaitGroup
writeWg.Add(2)
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Output-delivery and preview-state races can still lose warnings or corrupt progress rendering.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
cli/azd/internal/grpcserver/event_service.go:478
- The capture is detached as soon as the handler's gRPC status arrives, but extension stdout is delivered through
os/exec's asynchronous pipe copier (pkg/exec/command_runner.go:124-162) from the long-livedlistenprocess. A warning written before the extension returns can therefore reachDynamicMultiWriterafter this removal and still be lost. Keep the capture attached through a reliable drain/phase boundary, or add request-correlated output acknowledgement before removing it.
cli/azd/pkg/input/console.go:455 ResumePrevieweris serialized, butPausePrevieweris not:ShowPreviewercheckspreviewerSuppressedbefore takingshowProgressMu. A caller can pass that check, then pause can return and the progress ticker can start before the caller acquires the mutex and creates a previewer, corrupting the table and bypassing the new ordering. Put the suppression transition and theShowPreviewercheck under the same mutex (with the check after locking).
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
JeffreyCA
reviewed
Sep 11, 2026
| if !ok { | ||
| capture = &lifecycleOutputCapture{} | ||
| s.lifecycleOutputCaptures[extension] = capture | ||
| extension.StdOut().AddWriter(&capture.buffer) |
Contributor
There was a problem hiding this comment.
Consider avoiding the process-wide stdout capture here, or narrowing the behaviour we promise. extension.StdOut() is also the subprocess stdout used by service-target calls. Since service deploy steps run concurrently, one service can begin its target deployment and write normal progress while another service lifecycle handler still keeps this shared capture active. That output is then replayed as lifecycle output after the table. azure.ai.agents is both a service target and a lifecycle subscriber, so this is reachable with multiple agent services.
Can we keep the retained output correlated to the lifecycle invocation, or otherwise prevent unrelated service-target stdout from entering this buffer?
Contributor
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Deploy lifecycle extensions can print useful warnings during
predeployandpostdeploy. That output used to live only in a temporary preview. Whenazd upor deploy progress rendering paused or cleared the preview, warnings such as the Agents RBAC warning could disappear before the user saw them, even though deployment continued.What changed
keepLogsbehavior.azd up, stop the progress ticker, render the final table, and only then resume the preview. This keeps the final table's cleanup from overwriting a queued warning.keepLogs, and the final progress rendering order.Why this approach
The host already receives extension stdout and owns both the preview and progress rendering. Capturing the output there fixes the lost-warning path without changing the gRPC protocol. Reusing
Console.Messagealso keeps text and JSON output on the existing code paths.Scope and limitations
This PR retains stdout from
predeployandpostdeploylifecycle handlers. It does not capture warnings written to an extension's stderr throughazdext.Output.Warning. Output from concurrent handlers in one extension process can still be combined. A request-correlated structured warning channel would require a separate protocol change.Testing
go test ./pkg/input ./internal/grpcserver -count=1go test ./internal/cmd -run 'TestFinishDeployProgressRendersBeforeResumingPreviewer|TestPhaseTimingBreakdown' -count=1go test -race ./pkg/input ./internal/grpcserver -count=1could not run locally because CGO is disabled.go test ./...was attempted; unrelated environment-dependent failures remain in existing command, middleware, language-tool, and functional tests because of missing external tools, network access, and timing-sensitive startup behavior.Fixes: #7828