Fix nil panic when user declines preflight warnings#7310
Closed
jongio wants to merge 1 commit into
Closed
Conversation
jongio
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
March 25, 2026 15:16
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a nil-pointer panic in azd up provisioning flow when a deployment is intentionally skipped due to preflight warnings being declined, by making the caller treat any non-empty skip reason as “skipped”.
Changes:
- Replaced single-constant skip detection with an
isDeploymentSkipped()helper that treats any non-emptySkippedReasonas skipped. - Added a regression unit test covering both existing skip reasons and the non-skipped path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cli/azd/internal/cmd/provision.go | Uses isDeploymentSkipped() to avoid dereferencing deployResult.Deployment when a deployment is skipped; adds the helper. |
| cli/azd/internal/cmd/provision_test.go | Adds a regression test to ensure all current skip reasons are treated as skipped. |
jongio
force-pushed
the
fix/preflight-abort-panic
branch
from
March 25, 2026 15:28
27a49af to
0c72b40
Compare
Extract isDeploymentSkipped() helper that checks SkippedReason != "" instead of matching only DeploymentStateSkipped. This catches PreflightAbortedSkipped (and any future skip reasons), preventing a nil pointer dereference on Deployment.Outputs when Deployment is nil. Add regression test covering all skip reason variants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
force-pushed
the
fix/preflight-abort-panic
branch
from
March 25, 2026 15:39
0c72b40 to
2c101f2
Compare
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
|
weikanglim
reviewed
Mar 25, 2026
| // A deployment is considered skipped when SkippedReason is non-empty, which includes | ||
| // states such as DeploymentStateSkipped (no changes) and PreflightAbortedSkipped | ||
| // (user declined after preflight warnings). | ||
| func isDeploymentSkipped(deployResult *provisioning.DeployResult) bool { |
Contributor
There was a problem hiding this comment.
Adding a very specific check in the caller seems very tactical, and I'm wondering what we could do differently to codify or change the upstream behavior in Deploy().
@vhvb1989 if you have any thoughts here.
Member
Author
There was a problem hiding this comment.
Great point — treating preflight abort as "skipped" is the root of the problem, not just the nil deref.
The deeper issue: When a user declines preflight during azd up, the provision step returns nil error (treating it as "no changes"). The workflow runner only checks errors, so it proceeds to azd deploy — the user said "No" but deployment continues anyway. The nil panic was actually masking this by crashing before deploy could run.
Investigation summary:
workflow/runner.go:Run()is fail-fast on errors, butActionResulthas no abort semanticsprovision.gotreatsPreflightAbortedSkippedthe same asDeploymentStateSkipped->allSkipped=true-> returns success- There's an existing
ErrOperationCancelledsentinel ininternal/errors.gothat error middleware already handles gracefully (skips error analysis)
Proposed options — would love your input on which direction:
Option A: Provider returns error upstream
bicep_provider.Deploy() returns an error (wrapping ErrOperationCancelled) instead of DeployResult{SkippedReason: PreflightAbortedSkipped} with nil error. This stops everything at the source — manager.Deploy() propagates it, provision returns it, azd up stops.
Option B: Provision layer distinguishes abort from skip
Keep the provider returning a skip result, but provision.go treats PreflightAbortedSkipped differently from DeploymentStateSkipped: returns an error instead of "no changes to provision."
Option C: Manager returns error
manager.Deploy() returns an error when it detects PreflightAbortedSkipped instead of early-returning with nil.
My lean is Option A — the provider knows the user declined, so it should signal that as an error at the source. The PreflightAbortedSkipped skip-reason pattern added indirection that made this bug possible. Thoughts?
Comment on lines
+427
to
432
| skipped := isDeploymentSkipped(deployResult) | ||
| allSkipped = allSkipped && skipped | ||
| if skipped { | ||
| // Simply continue here; message is printed in the provider implementation | ||
| // Simply continue here; message is printed in the provider implementation. | ||
| continue | ||
| } |
There was a problem hiding this comment.
allSkipped is updated for any non-empty SkippedReason, including PreflightAbortedSkipped. If the user declines preflight warnings (common single-layer case), allSkipped remains true and the function returns "There are no changes to provision...", which is misleading because the run was user-aborted, not a no-op. Consider handling PreflightAbortedSkipped separately (e.g., return an ActionResult indicating provisioning was aborted/canceled, and/or avoid folding it into the "no changes" allSkipped path).
vhvb1989
reviewed
Mar 25, 2026
Comment on lines
+559
to
+561
| func isDeploymentSkipped(deployResult *provisioning.DeployResult) bool { | ||
| return deployResult != nil && deployResult.SkippedReason != "" | ||
| } |
Member
There was a problem hiding this comment.
The fix is good to mitigate the bug, but not as a the right approach we want here.
The root issue is that selecting N should stop the process completely and not just skip the provision.
With the fix, we still have the issue that would come next:
**
**
The right fix is to ensure azd stops the process, either azd up or azd provision
Member
Author
There was a problem hiding this comment.
Agreed 100% — the right fix is to stop the process completely, not skip. I posted a detailed analysis on @weikanglim's thread above with 3 options for how to propagate the abort. The TL;DR: the provider should return an error (wrapping the existing ErrOperationCancelled sentinel) instead of a "skipped" result, so the workflow runner stops before reaching deploy.
Will update the PR once we align on the approach.
Member
Alternative fix: #7329PR #7329 takes a more comprehensive approach to the same issue. Instead of only fixing the nil dereference in Key differences from this PR
How #7329 works
|
Member
Author
|
Closing in favor of #7329 |
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.
Fixes #7305
When the user declines preflight validation warnings during
azd up, the provisioning manager returnsDeployResult{SkippedReason: PreflightAbortedSkipped, Deployment: nil}. The caller inProvisionAction.Runonly checked forDeploymentStateSkipped, soPreflightAbortedSkippedfell through to code that dereferencesDeployment.Outputs— causing a nil pointer panic.Changes
isDeploymentSkipped()helper that checksSkippedReason != ""instead of matching a single constant. This catches all current and future skip reasons.TestIsDeploymentSkipped_AllSkipReasons) with 3 table-driven cases coveringDeploymentStateSkipped,PreflightAbortedSkipped, and the non-skipped path.Testing
mage preflightpasses (gofmt, lint, spell check, build, all unit tests)