Replace channel-based cancellation with context cancellation in provisioning progress display - #7004
Merged
Wei Lim (weikanglim) merged 1 commit intoMar 6, 2026
Conversation
Wei Lim (weikanglim)
marked this pull request as ready for review
March 4, 2026 21:05
Wei Lim (weikanglim)
requested a review
from Wallace Breza (wbreza)
as a code owner
March 4, 2026 21:05
Wei Lim (weikanglim)
requested review from
JeffreyCA,
Marina He (hemarina),
Rajesh Kamal (rajeshkamal5050),
tg-msft and
Victor Vazquez (vhvb1989)
as code owners
March 4, 2026 21:05
Contributor
There was a problem hiding this comment.
Pull request overview
Replace the channel-based cancellation used for the provisioning progress goroutine with context.WithCancel, ensuring the goroutine exits cleanly before spinner shutdown.
Changes:
- Introduced
context.WithCancelfor progress cancellation instead of acancelProgresschannel. - Added a
sync.WaitGroupto wait for the progress goroutine to exit before stopping the spinner.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Wallace Breza (wbreza)
left a comment
Wallace Breza (wbreza)
left a comment
Contributor
There was a problem hiding this comment.
Code Review: PR #7004 — Replace channel-based cancellation with context cancellation
Downstream Impact Assessment
- Isolated change — Only BicepProvider uses progress reporting; other providers (Terraform, DevCenter) are unaffected
- Manager integration —
provisioning.Manager.Deploy()wraps this with its own StopSpinner atmanager.go:128, which continues to work correctly - No test coverage for the progress goroutine lifecycle, so the timer issue below wouldn't be caught by CI
Findings Summary
| Priority | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟡 Medium | 1 |
| 🟢 Low | 1 |
| Total | 3 |
See inline comments for details.
✅ What Looks Good
context.WithCancelis the correct Go pattern for goroutine cancellation — much cleaner than the channel approachsync.WaitGroupensures the goroutine exits before spinner stops, eliminating the original race condition- Passing
progressCtxtoReportProgressmeans in-flight API calls abort promptly on cancellation - Change is well-scoped and isolated
Overall Assessment: The pattern change is sound, but the missing timer.Reset breaks continuous progress reporting.
Wei Lim (weikanglim)
force-pushed
the
weilim/context-cancellation-progress-display
branch
2 times, most recently
from
March 5, 2026 22:19
87973b6 to
31ad403
Compare
…sioning progress display
Wei Lim (weikanglim)
force-pushed
the
weilim/context-cancellation-progress-display
branch
from
March 5, 2026 22:20
31ad403 to
8a7cec3
Compare
Victor Vazquez (vhvb1989)
approved these changes
Mar 5, 2026
Wallace Breza (wbreza)
approved these changes
Mar 5, 2026
Wallace Breza (wbreza)
left a comment
Wallace Breza (wbreza)
left a comment
Contributor
There was a problem hiding this comment.
Re-review: Approve
The code changes look solid. The context cancellation pattern is idiomatic Go, the critical timer.Reset concern from my previous review has been addressed, and the sync.WaitGroup ensures proper goroutine lifecycle management.
What Looks Good
context.WithCancel+sync.WaitGroupis the correct concurrency pattern —cancelProgress()is non-blocking,wg.Wait()guarantees cleanup orderingprogressCtxpassed toReportProgressenables prompt cancellation of in-flight API calls- Delay simplification (merging
initialDelay/regularDelayinto singledelay) is clean
Prior Review Status
| Finding | Status |
|---|---|
| [Critical] timer.Reset removed | ✅ Fixed |
| [Medium] Double StopSpinner | ℹ️ Acceptable — wg.Wait() eliminates the race |
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
|
Copilot AI
pushed a commit
that referenced
this pull request
Mar 7, 2026
…sioning progress display (#7004) ## Summary Replace the `cancelProgress` channel in `BicepProvider.Deploy` with `context.WithCancel` for cancelling the progress-reporting goroutine. ## Impact Users will observe a faster progress update when the deployment is completed, but we're still polling for new progress operations (which is very common). ## Changes - **`bicep_provider.go`**: Use `context.WithCancel` instead of a channel. Use `sync.WaitGroup` to ensure the goroutine fully exits before stopping the spinner. ## Motivation The channel-based approach blocks the caller on `defer func() { cancelProgress <- true }()` until the goroutine finishes its current `ReportProgress` cycle. With context cancellation: - `cancelProgress()` returns immediately (broadcast signal via channel close) - In-flight API calls abort promptly via context - `wg.Wait()` ensures the goroutine exits before `StopSpinner` is called, eliminating the race where the goroutine could restart the spinner after it was stopped Contributes to #6915
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
Replace the
cancelProgresschannel inBicepProvider.Deploywithcontext.WithCancelfor cancelling the progress-reporting goroutine.Impact
Users will observe a faster progress update when the deployment is completed, but we're still polling for new progress operations (which is very common).
Changes
bicep_provider.go: Usecontext.WithCancelinstead of a channel. Usesync.WaitGroupto ensure the goroutine fully exits before stopping the spinner.Motivation
The channel-based approach blocks the caller on
defer func() { cancelProgress <- true }()until the goroutine finishes its currentReportProgresscycle. With context cancellation:cancelProgress()returns immediately (broadcast signal via channel close)wg.Wait()ensures the goroutine exits beforeStopSpinneris called, eliminating the race where the goroutine could restart the spinner after it was stoppedContributes to #6915