feat: add per-phase timing breakdown, deploy name sanitization, and lock order docs#8050
Merged
Conversation
Strip ANSI escape sequences and non-printable characters from service names before rendering in the deploy progress table to prevent terminal output manipulation from malicious azure.yaml configurations. Closes Azure#7972 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add explicit lock-ordering documentation to pkg/environment/manager.go and docs/concurrency-model.md explaining the saveMu -> flock -> env.mu chain and how subprocess hooks interact with the locking hierarchy. Closes Azure#7969 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes Azure#7983 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Break long test lines to satisfy 125-char lll limit. Apply gofmt to magefile.go (pre-existing formatting drift). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Emit perf.provision_duration_ms, perf.deploy_duration_ms, and perf.total_duration_ms as usage attributes on the cmd.up span. These enable backend tracking of provisioning vs deploying wall-clock durations independently. Also fix the phase timing breakdown to exclude package/publish steps from the deploying window (they run concurrently with provisioning), which caused the displayed deploy duration to equal the total. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Member
|
Curious about why 3 changes in one single PR? |
… regex - Reword manager.go and concurrency-model.md to clarify that subprocesses have their own saveMu instance (same acquisition order applies per-process). - Broaden ansiEscapeRe to cover CSI final byte '~' and OSC terminated by ST (ESC \) in addition to BEL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wbreza
approved these changes
May 5, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-Review — PR #8050 (commit bceff4f)
✅ Previous findings addressed
The new commit clarifies subprocess locking docs (manager.go + concurrency-model.md) — now explicitly states each subprocess has its own saveMu instance with same acquisition order per-process. Clear improvement.
ANSI regex broadened to cover CSI ~ final byte and OSC ST terminator (ESC \) in addition to BEL. Both correct additions.
Remaining findings from initial review
Medium — String prefix matching for step classification is fragile
File: cli/azd/internal/cmd/up_graph.go — phaseDurations()
phaseDurations classifies steps using 6 strings.HasPrefix calls against magic strings ("provision-", "deploy-", "cmdhook-preprovision", etc.). Consider extracting a centralized classifyStepPhase(name string) helper co-located with step name constants.
New finding from this commit
Low — No test cases for newly added regex coverage
File: cli/azd/internal/cmd/deploy_progress_test.go
The regex was broadened to handle CSI ~ and OSC ST terminators, but TestSanitizeServiceName wasn't updated with cases exercising these patterns. Suggested additions:
go {"csi_tilde_final", "\x1b[15~keypress", "keypress"}, {"osc_st_terminator", "\x1b]0;title\x1b\rest", "rest"},
Summary
| Priority | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 1 |
| Low | 3 |
LGTM ✅
Adds csi_tilde_final and osc_st_terminator test cases to TestSanitizeServiceName exercising the broadened ANSI regex patterns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Thanks for the review @wbreza! Addressed:
|
Member
Author
Fewer PRs. I'm okay bundling for fewer cycles |
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
wbreza
approved these changes
May 6, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-Review — PR #8050 (commit 8af9e92)
✅ Previous low finding addressed
New commit adds csi_tilde_final and osc_st_terminator test cases to TestSanitizeServiceName, directly exercising the broadened ANSI regex patterns from the prior commit. All 11 test cases now cover the full sanitization surface.
Remaining findings (unchanged)
| Priority | Finding | Status |
|---|---|---|
| Medium | String prefix matching in phaseDurations() is fragile — consider centralizing step classification | Open — non-blocking, future improvement |
| Low | Existing deploy_progress_test.go tests missing .Parallel() | Open — minor inconsistency |
| Low | No concurrent stress test for deployProgressTracker | Open — nice-to-have |
LGTM ✅
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.
Summary
Adds per-phase timing breakdown to the
azd upSUCCESS message and emits corresponding telemetry attributes for backend performance tracking. Also hardens deploy progress rendering against malicious service names and documents the lock acquisition order in the environment manager.Changes
1. Per-phase timing breakdown in SUCCESS message (Fixes #7983)
What: After a successful
azd up, the SUCCESS message now shows wall-clock durations for the Provisioning and Deploying phases:Why: Users have no visibility into where time is spent during
azd up. This makes it immediately obvious whether provisioning or deploying is the bottleneck, helping users optimize their workflow and report performance issues with actionable data.How: The
phaseTimingBreakdown()function computes wall-clock durations by finding the earliest start and latest end among provision/deploy steps from the execution graph result. Package/publish steps are excluded from the deploy window because they run concurrently with provisioning.2. Sanitize service names in deploy progress output (Fixes #7972)
What: Service names displayed in the deploy progress table are now sanitized to strip ANSI escape sequences (CSI, OSC), non-printable characters, and other terminal control codes.
Why: A malicious or misconfigured
azure.yamlcould inject terminal escape sequences via service names, potentially corrupting terminal output, hiding text, or in extreme cases exploiting terminal emulator vulnerabilities. This is a defense-in-depth hardening for the terminal rendering path.How:
sanitizeServiceName()uses regex to strip CSI sequences (\x1b[...), OSC sequences (\x1b]...\x07), bare escapes, and any remaining non-printable characters. Applied at storage time innewDeployProgressTrackerso the sanitized name is used for all rendering.3. Document lock acquisition order (Fixes #7969)
What: Added comprehensive documentation of the lock acquisition order for the environment manager's
saveMumutex, the file-basedflock, and the per-Environmentenv.muRWMutex.Why: The concurrency model involves three layers of locking (in-process mutex, cross-process file lock, per-object RWMutex). Without clear documentation of the acquisition order, future contributors risk introducing deadlocks. This was identified as a gap in the existing concurrency documentation.
How: Added a detailed comment block above the
saveMufield declaration inmanager.goand a "Lock Acquisition Order" section tocli/azd/docs/concurrency-model.mddocumenting the full chain:saveMu→flock→env.mu.4. Per-phase duration telemetry (new)
What: Three new telemetry attributes emitted on the
cmd.upspan:perf.provision_duration_ms— wall-clock provisioning phase durationperf.deploy_duration_ms— wall-clock deploying phase durationperf.total_duration_ms— total execution durationWhy: Enables backend performance tracking and dashboarding of where time is spent across all
azd upinvocations. Previously only total command duration was available; now we can independently track and alert on provisioning vs deploying regressions.How: Uses
tracing.SetUsageAttributes()to attach the values to the rootcmd.upspan via the telemetry middleware. Fields follow the existingperf.*namespace convention (SystemMetadata/PerformanceAndHealth/IsMeasurement: true).Testing
mage preflight— 9/9 checks ✅)TestPhaseTimingBreakdown(5 sub-cases) andTestSanitizeServiceName(9 sub-cases)--trace-log-file trace.jsonconfirms telemetry attributes emit correctlyazd upagainst multi-service template shows correct timing breakdownTelemetry verification
json { "perf.provision_duration_ms": 30328, "perf.deploy_duration_ms": 93282, "perf.total_duration_ms": 123616 }Fixes #7983
Fixes #7972
Fixes #7969