From f88838385e0684fcd1d92595c64e6a9b5d871e87 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Thu, 15 Jan 2026 15:29:38 -0800 Subject: [PATCH 1/5] Fixes context cancellation issue --- cli/azd/cmd/container.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/cli/azd/cmd/container.go b/cli/azd/cmd/container.go index cc143dd6847..bb97d002385 100644 --- a/cli/azd/cmd/container.go +++ b/cli/azd/cmd/container.go @@ -919,25 +919,27 @@ func (w *workflowCmdAdapter) SetArgs(args []string) { func (w *workflowCmdAdapter) ExecuteContext(ctx context.Context) error { childCtx := middleware.WithChildAction(ctx) - // CRITICAL FIX: Explicitly set the context on the command before calling ExecuteContext. + // Clear any stale context from previous workflow step executions. // When the same command instance is reused across workflow steps, cobra may retain - // a stale cancelled context from a previous execution. Setting the context on both - // the root command and all subcommands (recursively) ensures the fresh context is used. - setContextRecursively(w.cmd, childCtx) + // a cancelled context from a previous execution. We clear with context.TODO() rather + // than setting childCtx to avoid polluting the cobra command tree with a context that + // will be cancelled after this step completes. The actual context is properly passed + // via ExecuteContext which sets it on the root command and propagates to subcommands. + clearContextRecursively(w.cmd) return w.cmd.ExecuteContext(childCtx) } -// setContextRecursively sets the context on a command and all its subcommands recursively -func setContextRecursively(cmd *cobra.Command, ctx context.Context) { - // Always set the context, even if the command hasn't been initialized yet. - // cobra.Command.Context() returns context.Background() if no context is set, - // so SetContext is safe to call on any command. This ensures all commands - // in the tree start with the correct context, preventing stale contexts - // from previous executions when commands are reused. - cmd.SetContext(ctx) +// clearContextRecursively clears the context on a command and all its subcommands recursively +// to remove any stale cancelled contexts from previous workflow step executions. +func clearContextRecursively(cmd *cobra.Command) { + // Set context to TODO() to clear any stale context. cobra.Command.Context() returns + // context.Background() if no context is set, so SetContext is safe to call on any command. + // The actual context will be set by ExecuteContext on the root command and propagated + // to the executed subcommand by cobra's internal execution flow. + cmd.SetContext(context.TODO()) for _, subCmd := range cmd.Commands() { - setContextRecursively(subCmd, ctx) + clearContextRecursively(subCmd) } } From 33d0ba8783fec7c1f8267552861ef1bab292b633 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Tue, 20 Jan 2026 09:57:47 -0800 Subject: [PATCH 2/5] Fixes unit tests --- cli/azd/cmd/container_test.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/cli/azd/cmd/container_test.go b/cli/azd/cmd/container_test.go index b6831ef972a..f3400a42b3a 100644 --- a/cli/azd/cmd/container_test.go +++ b/cli/azd/cmd/container_test.go @@ -194,7 +194,7 @@ func Test_WorkflowCmdAdapter_ContextPropagation(t *testing.T) { require.NoError(t, err) require.Len(t, receivedContexts, 1, "First execution should have received context") - // Verify first context is not cancelled + // Verify first context is not cancelled during execution select { case <-receivedContexts[0].Done(): t.Fatal("First context should not be cancelled during execution") @@ -203,16 +203,11 @@ func Test_WorkflowCmdAdapter_ContextPropagation(t *testing.T) { } // Cancel the first context (simulating workflow step completion) + // Note: The workflowCmdAdapter intentionally clears contexts using context.TODO() + // to prevent stale context propagation. The subcommand receives a fresh context + // each time, so cancelling ctx1 won't affect receivedContexts[0]. cancel1() - // Verify first context is now cancelled - select { - case <-receivedContexts[0].Done(): - // Expected: context is cancelled - default: - t.Fatal("First context should be cancelled after cancel1()") - } - // Simulate second workflow step with a fresh context ctx2 := context.Background() adapter.SetArgs([]string{"sub"}) @@ -229,9 +224,8 @@ func Test_WorkflowCmdAdapter_ContextPropagation(t *testing.T) { // Expected: second context is valid } - // Verify the two contexts are different - require.NotSame(t, receivedContexts[0], receivedContexts[1], - "Second execution should receive a different context than the first") + // Note: Both contexts will be context.TODO() since clearContextRecursively + // sets this on subcommands. The important thing is that neither is cancelled. }) t.Run("NestedSubcommandReceivesFreshContext", func(t *testing.T) { From 6f2daf25429ac32b568e2ec96b227374d3a21eb7 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Tue, 20 Jan 2026 15:54:04 -0800 Subject: [PATCH 3/5] Fixes stable context issues --- cli/azd/cmd/cobra_builder.go | 4 +- cli/azd/cmd/container.go | 26 +--- cli/azd/cmd/container_test.go | 77 +++++----- cli/azd/cmd/middleware/debug.go | 2 +- cli/azd/cmd/middleware/error.go | 2 +- cli/azd/cmd/middleware/extensions.go | 2 +- cli/azd/cmd/middleware/hooks.go | 2 +- cli/azd/cmd/middleware/middleware.go | 12 +- cli/azd/cmd/middleware/telemetry.go | 2 +- cli/azd/cmd/middleware/ux.go | 2 +- cli/azd/cmd/root.go | 21 ++- .../grpcserver/project_service_test.go | 11 +- cli/azd/internal/scaffold/scaffold_test.go | 3 +- cli/azd/main.go | 6 + cli/azd/pkg/ext/event_dispatcher_test.go | 144 ++++++++++++++++++ .../provisioning/bicep/bicep_provider.go | 5 + .../provisioning/bicep/bicep_provider_test.go | 8 +- cli/azd/pkg/pipeline/github_provider_test.go | 4 +- cli/azd/pkg/project/container_helper.go | 6 +- .../project/service_target_containerapp.go | 6 +- cli/azd/pkg/templates/gh_source.go | 10 ++ cli/azd/pkg/templates/gh_source_test.go | 45 ++---- cli/azd/pkg/tools/bicep/bicep.go | 88 ++++++----- cli/azd/pkg/tools/bicep/bicep_test.go | 10 +- cli/azd/pkg/tools/github/github.go | 123 ++++++++------- cli/azd/pkg/tools/github/github_test.go | 15 +- cli/azd/pkg/tools/pack/pack.go | 89 ++++++----- cli/azd/pkg/tools/pack/pack_test.go | 8 +- cli/azd/test/functional/aspire_test.go | 6 +- 29 files changed, 468 insertions(+), 271 deletions(-) diff --git a/cli/azd/cmd/cobra_builder.go b/cli/azd/cmd/cobra_builder.go index b6a7679560b..783e7acdc5f 100644 --- a/cli/azd/cmd/cobra_builder.go +++ b/cli/azd/cmd/cobra_builder.go @@ -94,9 +94,7 @@ func (cb *CobraBuilder) configureActionResolver(cmd *cobra.Command, descriptor * } cmd.RunE = func(cmd *cobra.Command, args []string) error { - // Register root go context that will be used for resolving singleton dependencies - ctx := tools.WithInstalledCheckCache(cmd.Context()) - ioc.RegisterInstance(cb.container, ctx) + ctx := cmd.Context() // Create new container scope for the current command cmdContainer, err := cb.container.NewScope() diff --git a/cli/azd/cmd/container.go b/cli/azd/cmd/container.go index bb97d002385..41c9149c5da 100644 --- a/cli/azd/cmd/container.go +++ b/cli/azd/cmd/container.go @@ -174,7 +174,7 @@ func registerCommonDependencies(container *ioc.NestedContainer) { return writer }) - container.MustRegisterScoped(func(cmd *cobra.Command) internal.EnvFlag { + container.MustRegisterScoped(func(ctx context.Context, cmd *cobra.Command) internal.EnvFlag { // The env flag `-e, --environment` is available on most azd commands but not all // This is typically used to override the default environment and is used for bootstrapping other components // such as the azd environment. @@ -190,7 +190,7 @@ func registerCommonDependencies(container *ioc.NestedContainer) { // If no explicit environment flag was set, but one was provided // in the context, use that instead. // This is used in workflow execution (in `up`) to influence the environment used. - if envFlag, ok := cmd.Context().Value(envFlagCtxKey).(internal.EnvFlag); ok { + if envFlag, ok := ctx.Value(envFlagCtxKey).(internal.EnvFlag); ok { return envFlag } } @@ -918,31 +918,9 @@ func (w *workflowCmdAdapter) SetArgs(args []string) { // ExecuteContext implements workflow.AzdCommandRunner func (w *workflowCmdAdapter) ExecuteContext(ctx context.Context) error { childCtx := middleware.WithChildAction(ctx) - - // Clear any stale context from previous workflow step executions. - // When the same command instance is reused across workflow steps, cobra may retain - // a cancelled context from a previous execution. We clear with context.TODO() rather - // than setting childCtx to avoid polluting the cobra command tree with a context that - // will be cancelled after this step completes. The actual context is properly passed - // via ExecuteContext which sets it on the root command and propagates to subcommands. - clearContextRecursively(w.cmd) - return w.cmd.ExecuteContext(childCtx) } -// clearContextRecursively clears the context on a command and all its subcommands recursively -// to remove any stale cancelled contexts from previous workflow step executions. -func clearContextRecursively(cmd *cobra.Command) { - // Set context to TODO() to clear any stale context. cobra.Command.Context() returns - // context.Background() if no context is set, so SetContext is safe to call on any command. - // The actual context will be set by ExecuteContext on the root command and propagated - // to the executed subcommand by cobra's internal execution flow. - cmd.SetContext(context.TODO()) - for _, subCmd := range cmd.Commands() { - clearContextRecursively(subCmd) - } -} - // ArmClientInitializer is a function definition for all Azure SDK ARM Client type ArmClientInitializer[T comparable] func( subscriptionId string, diff --git a/cli/azd/cmd/container_test.go b/cli/azd/cmd/container_test.go index f3400a42b3a..368712aed66 100644 --- a/cli/azd/cmd/container_test.go +++ b/cli/azd/cmd/container_test.go @@ -7,6 +7,7 @@ import ( "context" "testing" + "github.com/azure/azure-dev/cli/azd/cmd/middleware" "github.com/azure/azure-dev/cli/azd/pkg/environment/azdcontext" "github.com/azure/azure-dev/cli/azd/pkg/ioc" "github.com/azure/azure-dev/cli/azd/pkg/lazy" @@ -160,11 +161,12 @@ type testConcreteComponent[T comparable] struct { } // Test_WorkflowCmdAdapter_ContextPropagation validates that the workflowCmdAdapter -// properly sets fresh contexts on both root and subcommands when reused across -// multiple workflow steps, preventing stale cancelled contexts from affecting -// subsequent executions. +// properly marks contexts as child actions when executing subcommands. +// The main.go entrypoint wraps the root context with context.WithoutCancel, +// so workflow steps always receive a non-cancellable context. +// See: https://github.com/Azure/azure-dev/issues/6530 func Test_WorkflowCmdAdapter_ContextPropagation(t *testing.T) { - t.Run("SubcommandReceivesFreshContext", func(t *testing.T) { + t.Run("SubcommandReceivesChildActionContext", func(t *testing.T) { // Track which contexts were seen by the subcommand var receivedContexts []context.Context @@ -187,48 +189,38 @@ func Test_WorkflowCmdAdapter_ContextPropagation(t *testing.T) { // Create the adapter adapter := &workflowCmdAdapter{cmd: rootCmd} - // Simulate first workflow step - ctx1, cancel1 := context.WithCancel(context.Background()) + // In production, main.go wraps with context.WithoutCancel. + // Simulate this by using a non-cancellable context. + ctx := context.WithoutCancel(context.Background()) adapter.SetArgs([]string{"sub"}) - err := adapter.ExecuteContext(ctx1) + err := adapter.ExecuteContext(ctx) require.NoError(t, err) - require.Len(t, receivedContexts, 1, "First execution should have received context") + require.Len(t, receivedContexts, 1, "Execution should have received context") - // Verify first context is not cancelled during execution + // Verify context is marked as child action + require.True(t, middleware.IsChildAction(receivedContexts[0]), + "Context should be marked as child action") + + // Verify context is not cancelled (since we used WithoutCancel) select { case <-receivedContexts[0].Done(): - t.Fatal("First context should not be cancelled during execution") + t.Fatal("Context should not be cancelled") default: // Expected: context is still valid } - // Cancel the first context (simulating workflow step completion) - // Note: The workflowCmdAdapter intentionally clears contexts using context.TODO() - // to prevent stale context propagation. The subcommand receives a fresh context - // each time, so cancelling ctx1 won't affect receivedContexts[0]. - cancel1() - - // Simulate second workflow step with a fresh context - ctx2 := context.Background() + // Execute again - should still work adapter.SetArgs([]string{"sub"}) - err = adapter.ExecuteContext(ctx2) + err = adapter.ExecuteContext(ctx) require.NoError(t, err) require.Len(t, receivedContexts, 2, "Second execution should have received context") - // CRITICAL TEST: Verify second execution received a valid context, - // not the cancelled context from the first execution - select { - case <-receivedContexts[1].Done(): - t.Fatal("2nd execution should have received a fresh valid context, not the cancelled one from 1st execution") - default: - // Expected: second context is valid - } - - // Note: Both contexts will be context.TODO() since clearContextRecursively - // sets this on subcommands. The important thing is that neither is cancelled. + // Both contexts should be marked as child actions + require.True(t, middleware.IsChildAction(receivedContexts[1]), + "Second context should also be marked as child action") }) - t.Run("NestedSubcommandReceivesFreshContext", func(t *testing.T) { + t.Run("NestedSubcommandReceivesChildActionContext", func(t *testing.T) { // Track which contexts were seen var receivedContexts []context.Context @@ -254,29 +246,32 @@ func Test_WorkflowCmdAdapter_ContextPropagation(t *testing.T) { adapter := &workflowCmdAdapter{cmd: rootCmd} - // First execution - ctx1, cancel1 := context.WithCancel(context.Background()) + // In production, main.go wraps with context.WithoutCancel. + ctx := context.WithoutCancel(context.Background()) adapter.SetArgs([]string{"parent", "child"}) - err := adapter.ExecuteContext(ctx1) + err := adapter.ExecuteContext(ctx) require.NoError(t, err) require.Len(t, receivedContexts, 1) - // Cancel first context - cancel1() + // Verify context is marked as child action + require.True(t, middleware.IsChildAction(receivedContexts[0]), + "Nested context should be marked as child action") - // Second execution with fresh context - ctx2 := context.Background() + // Second execution should also work adapter.SetArgs([]string{"parent", "child"}) - err = adapter.ExecuteContext(ctx2) + err = adapter.ExecuteContext(ctx) require.NoError(t, err) require.Len(t, receivedContexts, 2) - // Verify second execution got a valid context + // Verify second execution got a valid context marked as child select { case <-receivedContexts[1].Done(): - t.Fatal("Nested subcommand should have received a fresh valid context in second execution") + t.Fatal("Nested subcommand should have received a valid context") default: // Expected: context is valid } + + require.True(t, middleware.IsChildAction(receivedContexts[1]), + "Second nested context should also be marked as child action") }) } diff --git a/cli/azd/cmd/middleware/debug.go b/cli/azd/cmd/middleware/debug.go index 9a24e387d49..9e572877e57 100644 --- a/cli/azd/cmd/middleware/debug.go +++ b/cli/azd/cmd/middleware/debug.go @@ -38,7 +38,7 @@ func NewDebugMiddleware(options *Options, console input.Console) Middleware { // a debugger before continuing invocation of the action func (m *DebugMiddleware) Run(ctx context.Context, next NextFn) (*actions.ActionResult, error) { // Don't run for sub actions - if m.options.IsChildAction(ctx) { + if IsChildAction(ctx) { return next(ctx) } diff --git a/cli/azd/cmd/middleware/error.go b/cli/azd/cmd/middleware/error.go index 6be05e74d86..f059845afec 100644 --- a/cli/azd/cmd/middleware/error.go +++ b/cli/azd/cmd/middleware/error.go @@ -77,7 +77,7 @@ func (e *ErrorMiddleware) Run(ctx context.Context, next NextFn) (*actions.Action // Stop the spinner always to un-hide cursor e.console.StopSpinner(ctx, "", input.Step) - if err == nil || e.options.IsChildAction(ctx) { + if err == nil || IsChildAction(ctx) { return actionResult, err } diff --git a/cli/azd/cmd/middleware/extensions.go b/cli/azd/cmd/middleware/extensions.go index 40149f7613b..ef36a6eff0b 100644 --- a/cli/azd/cmd/middleware/extensions.go +++ b/cli/azd/cmd/middleware/extensions.go @@ -57,7 +57,7 @@ func NewExtensionsMiddleware( func (m *ExtensionsMiddleware) Run(ctx context.Context, next NextFn) (*actions.ActionResult, error) { // Extensions were already started in the root parent command - if m.options.IsChildAction(ctx) { + if IsChildAction(ctx) { return next(ctx) } diff --git a/cli/azd/cmd/middleware/hooks.go b/cli/azd/cmd/middleware/hooks.go index d43794fbeb4..e45dda6244e 100644 --- a/cli/azd/cmd/middleware/hooks.go +++ b/cli/azd/cmd/middleware/hooks.go @@ -55,7 +55,7 @@ func NewHooksMiddleware( // Runs the Hooks middleware func (m *HooksMiddleware) Run(ctx context.Context, next NextFn) (*actions.ActionResult, error) { // Validate hooks and display any warnings - if !m.options.IsChildAction(ctx) { + if !IsChildAction(ctx) { if err := m.validateHooks(ctx, m.projectConfig); err != nil { return nil, fmt.Errorf("failed validating hooks, %w", err) } diff --git a/cli/azd/cmd/middleware/middleware.go b/cli/azd/cmd/middleware/middleware.go index eb541500033..af29a1d638d 100644 --- a/cli/azd/cmd/middleware/middleware.go +++ b/cli/azd/cmd/middleware/middleware.go @@ -39,11 +39,6 @@ type Options struct { Annotations map[string]string } -func (o *Options) IsChildAction(ctx context.Context) bool { - value, ok := ctx.Value(childActionKey).(bool) - return ok && value -} - // Sets the container to be used for resolving middleware components func (o *Options) WithContainer(container *ioc.NestedContainer) { o.container = container @@ -143,3 +138,10 @@ func (r *MiddlewareRunner) Use(name string, resolveFn any) error { func WithChildAction(ctx context.Context) context.Context { return context.WithValue(ctx, childActionKey, true) } + +// IsChildAction checks if the given context was created by WithChildAction. +// This is used to determine if a command is being executed as part of a workflow step. +func IsChildAction(ctx context.Context) bool { + value, ok := ctx.Value(childActionKey).(bool) + return ok && value +} diff --git a/cli/azd/cmd/middleware/telemetry.go b/cli/azd/cmd/middleware/telemetry.go index 237d4506c99..d58ee722214 100644 --- a/cli/azd/cmd/middleware/telemetry.go +++ b/cli/azd/cmd/middleware/telemetry.go @@ -65,7 +65,7 @@ func (m *TelemetryMiddleware) Run(ctx context.Context, next NextFn) (*actions.Ac log.Printf("TraceID: %s", span.SpanContext().TraceID()) - if !m.options.IsChildAction(ctx) { + if !IsChildAction(ctx) { // Set the command name as a baggage item on the span context. // This allow inner actions to have command name attached. spanCtx = tracing.SetBaggageInContext( diff --git a/cli/azd/cmd/middleware/ux.go b/cli/azd/cmd/middleware/ux.go index 725d4b66f77..f6287cb6630 100644 --- a/cli/azd/cmd/middleware/ux.go +++ b/cli/azd/cmd/middleware/ux.go @@ -34,7 +34,7 @@ func NewUxMiddleware(options *Options, console input.Console, featuresManager *a func (m *UxMiddleware) Run(ctx context.Context, next NextFn) (*actions.ActionResult, error) { // Don't run for sub actions - if m.options.IsChildAction(ctx) { + if IsChildAction(ctx) { return next(ctx) } diff --git a/cli/azd/cmd/root.go b/cli/azd/cmd/root.go index de95fde05c4..809dcc56d27 100644 --- a/cli/azd/cmd/root.go +++ b/cli/azd/cmd/root.go @@ -133,9 +133,18 @@ func NewRootCmd( // we can just remove the entire project folder afterwards. // In practical execution, this wouldn't affect much, since the CLI is exiting. if prevDir != "" { - return os.Chdir(prevDir) + if err := os.Chdir(prevDir); err != nil { + return err + } } + // Clear context from the executed command to prevent stale context issues. + // This is important because singletons (like credential providers) may cache + // contexts from earlier commands, leading to "context canceled" errors when + // those contexts are cancelled but the singleton is reused. + // See: https://github.com/Azure/azure-dev/issues/6530 + clearCommandContext(cmd) + return nil }, SilenceUsage: true, @@ -543,3 +552,13 @@ func getCmdRootHelpCommands(cmd *cobra.Command) (result string) { } return strings.Join(paragraph, "\n") } + +// clearCommandContext recursively clears the context from the command and its children. +// This prevents stale context issues where a cancelled context might be reused by singletons +// or cached components. See: https://github.com/Azure/azure-dev/issues/6530 +func clearCommandContext(cmd *cobra.Command) { + cmd.SetContext(nil) + for _, child := range cmd.Commands() { + clearCommandContext(child) + } +} diff --git a/cli/azd/internal/grpcserver/project_service_test.go b/cli/azd/internal/grpcserver/project_service_test.go index a112efaa532..635ac479f57 100644 --- a/cli/azd/internal/grpcserver/project_service_test.go +++ b/cli/azd/internal/grpcserver/project_service_test.go @@ -48,13 +48,12 @@ func Test_ProjectService_NoProject(t *testing.T) { }) // Create mock GitHub CLI. - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // Create the service with ImportManager. importManager := project.NewImportManager(&project.DotNetImporter{}) service := NewProjectService(lazyAzdContext, lazyEnvManager, lazyProjectConfig, importManager, ghCli) - _, err = service.Get(*mockContext.Context, &azdext.EmptyRequest{}) + _, err := service.Get(*mockContext.Context, &azdext.EmptyRequest{}) require.Error(t, err) } @@ -105,8 +104,7 @@ func Test_ProjectService_Flow(t *testing.T) { require.NoError(t, err) // Create mock GitHub CLI. - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // Create the service with ImportManager. importManager := project.NewImportManager(&project.DotNetImporter{}) @@ -154,8 +152,7 @@ func Test_ProjectService_AddService(t *testing.T) { lazyProjectConfig := lazy.From(&projectConfig) // Create mock GitHub CLI. - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // Create the project service with ImportManager. importManager := project.NewImportManager(&project.DotNetImporter{}) diff --git a/cli/azd/internal/scaffold/scaffold_test.go b/cli/azd/internal/scaffold/scaffold_test.go index 73946e53bbb..ca54f41edaf 100644 --- a/cli/azd/internal/scaffold/scaffold_test.go +++ b/cli/azd/internal/scaffold/scaffold_test.go @@ -256,7 +256,8 @@ func TestExecInfra(t *testing.T) { } ctx := context.Background() - cli, err := bicep.NewCli(ctx, mockinput.NewMockConsole(), exec.NewCommandRunner(nil)) + cli := bicep.NewCli(mockinput.NewMockConsole(), exec.NewCommandRunner(nil)) + err = cli.EnsureInstalled(ctx) require.NoError(t, err) res, err := cli.Build(ctx, filepath.Join(dir, "main.bicep")) diff --git a/cli/azd/main.go b/cli/azd/main.go index eda01bc45b7..a3ea8f23542 100644 --- a/cli/azd/main.go +++ b/cli/azd/main.go @@ -33,6 +33,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/oneauth" "github.com/azure/azure-dev/cli/azd/pkg/osutil" "github.com/azure/azure-dev/cli/azd/pkg/output" + "github.com/azure/azure-dev/cli/azd/pkg/tools" "github.com/blang/semver/v4" "github.com/mattn/go-colorable" "github.com/spf13/pflag" @@ -67,6 +68,11 @@ func main() { go fetchLatestVersion(latest) rootContainer := ioc.NewNestedContainer(nil) + + ctx = context.WithoutCancel(ctx) + ctx = tools.WithInstalledCheckCache(ctx) + + // Register the context for singleton resolution ioc.RegisterInstance(rootContainer, ctx) // Execute command with auto-installation support for extensions diff --git a/cli/azd/pkg/ext/event_dispatcher_test.go b/cli/azd/pkg/ext/event_dispatcher_test.go index d0574c65dec..48231c94486 100644 --- a/cli/azd/pkg/ext/event_dispatcher_test.go +++ b/cli/azd/pkg/ext/event_dispatcher_test.go @@ -374,6 +374,150 @@ func Test_Duplicate_Handler_Detection_Multiple_Duplicates(t *testing.T) { require.Equal(t, 1, callCount, "Handler should be called only once despite multiple registration attempts") } +// Test_Workflow_Context_Handler_Cleanup validates that event handlers are properly cleaned up +// when using a context hierarchy similar to azd workflows: +// - Root context uses context.WithoutCancel (for singletons - never cancelled) +// - Step contexts are cancellable children (cancelled after each workflow step) +// - Handlers registered with step contexts should be cleaned up when step is cancelled +// This test validates the fix for https://github.com/Azure/azure-dev/issues/6530 +func Test_Workflow_Context_Handler_Cleanup(t *testing.T) { + t.Run("HandlersCleanedUpWhenStepContextCancelled", func(t *testing.T) { + ed := NewEventDispatcher[testEventArgs](testEvent) + + // Simulate root context that uses WithoutCancel (like singletons) + rootCtx := context.Background() + nonCancellableRoot := context.WithoutCancel(rootCtx) + + // Simulate workflow step 1 - create cancellable child context + step1Ctx, cancelStep1 := context.WithCancel(nonCancellableRoot) + + // Register handler with step 1 context (like actions do) + step1CallCount := 0 + step1Handler := func(ctx context.Context, args testEventArgs) error { + step1CallCount++ + return nil + } + err := ed.AddHandler(step1Ctx, testEvent, step1Handler) + require.NoError(t, err) + + // Verify handler is registered + ed.mu.RLock() + require.Equal(t, 1, len(ed.handlers[testEvent]), "Step 1 handler should be registered") + ed.mu.RUnlock() + + // Raise event with step context - handler should be called + err = ed.RaiseEvent(step1Ctx, testEvent, testEventArgs{}) + require.NoError(t, err) + require.Equal(t, 1, step1CallCount, "Step 1 handler should be called") + + // Cancel step 1 context (simulating workflow step completion) + cancelStep1() + + // Give cleanup goroutine time to process + time.Sleep(50 * time.Millisecond) + + // Verify handler was removed + ed.mu.RLock() + require.Equal(t, 0, len(ed.handlers[testEvent]), "Step 1 handler should be cleaned up after cancel") + ed.mu.RUnlock() + + // Raise event again - handler should NOT be called + step1CallCount = 0 + err = ed.RaiseEvent(context.Background(), testEvent, testEventArgs{}) + require.NoError(t, err) + require.Equal(t, 0, step1CallCount, "Step 1 handler should not be called after cleanup") + }) + + t.Run("MultipleStepsWithHandlerCleanup", func(t *testing.T) { + ed := NewEventDispatcher[testEventArgs](testEvent) + + // Simulate root context with WithoutCancel + rootCtx := context.Background() + nonCancellableRoot := context.WithoutCancel(rootCtx) + + // Step 1: provision + step1Ctx, cancelStep1 := context.WithCancel(nonCancellableRoot) + step1CallCount := 0 + step1Handler := func(ctx context.Context, args testEventArgs) error { + step1CallCount++ + return nil + } + err := ed.AddHandler(step1Ctx, testEvent, step1Handler) + require.NoError(t, err) + + // Verify step 1 handler works + err = ed.RaiseEvent(step1Ctx, testEvent, testEventArgs{}) + require.NoError(t, err) + require.Equal(t, 1, step1CallCount) + + // Complete step 1 + cancelStep1() + time.Sleep(50 * time.Millisecond) + + // Verify step 1 handler cleaned up + ed.mu.RLock() + require.Equal(t, 0, len(ed.handlers[testEvent]), "Step 1 handler should be cleaned up") + ed.mu.RUnlock() + + // Step 2: deploy (fresh context, same root) + step2Ctx, cancelStep2 := context.WithCancel(nonCancellableRoot) + step2CallCount := 0 + step2Handler := func(ctx context.Context, args testEventArgs) error { + step2CallCount++ + return nil + } + err = ed.AddHandler(step2Ctx, testEvent, step2Handler) + require.NoError(t, err) + + // Verify step 2 handler works + err = ed.RaiseEvent(step2Ctx, testEvent, testEventArgs{}) + require.NoError(t, err) + require.Equal(t, 1, step2CallCount) + require.Equal(t, 1, step1CallCount, "Step 1 handler should not be called again") + + // Complete step 2 + cancelStep2() + time.Sleep(50 * time.Millisecond) + + // Verify step 2 handler cleaned up + ed.mu.RLock() + require.Equal(t, 0, len(ed.handlers[testEvent]), "Step 2 handler should be cleaned up") + ed.mu.RUnlock() + }) + + t.Run("NonCancellableContextHandlerNotCleanedUp", func(t *testing.T) { + ed := NewEventDispatcher[testEventArgs](testEvent) + + // Register handler with non-cancellable context (simulating incorrect usage) + nonCancellableCtx := context.WithoutCancel(context.Background()) + callCount := 0 + handler := func(ctx context.Context, args testEventArgs) error { + callCount++ + return nil + } + err := ed.AddHandler(nonCancellableCtx, testEvent, handler) + require.NoError(t, err) + + // Handler should be registered + ed.mu.RLock() + require.Equal(t, 1, len(ed.handlers[testEvent])) + ed.mu.RUnlock() + + // Wait a bit - handler should NOT be cleaned up since context is non-cancellable + time.Sleep(50 * time.Millisecond) + + // Handler should still be registered + ed.mu.RLock() + require.Equal(t, 1, len(ed.handlers[testEvent]), "Handler with non-cancellable context should persist") + ed.mu.RUnlock() + + // Handler should still be called + err = ed.RaiseEvent(context.Background(), testEvent, testEventArgs{}) + require.NoError(t, err) + require.Equal(t, 1, callCount) + }) +} + type testEventArgs struct{} const testEvent Event = "test" diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index fb6170286f5..aa6f88d0c94 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -95,6 +95,11 @@ func (p *BicepProvider) Name() string { // Initialize initializes provider state from the options. // It also calls EnsureEnv, which ensures the client-side state is ready for provisioning. func (p *BicepProvider) Initialize(ctx context.Context, projectPath string, opt provisioning.Options) error { + // Ensure bicep CLI is installed before any operations + if err := p.bicepCli.EnsureInstalled(ctx); err != nil { + return fmt.Errorf("ensuring bicep is installed: %w", err) + } + infraOptions, err := opt.GetWithDefaults() if err != nil { return err diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go index feb421ffb9d..be51b56a307 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go @@ -385,8 +385,7 @@ func createBicepProvider(t *testing.T, mockContext *mocks.MockContext) *BicepPro envManager.On("Save", mock.Anything, mock.Anything).Return(nil) envManager.On("Reload", mock.Anything, mock.Anything).Return(nil) - bicepCli, err := bicep.NewCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + bicepCli := bicep.NewCli(mockContext.Console, mockContext.CommandRunner) azCli := mockazapi.NewAzureClientFromMockContext(mockContext) resourceService := azapi.NewResourceService(mockContext.SubscriptionCredentialProvider, mockContext.ArmClientOptions) deploymentService := mockazapi.NewStandardDeploymentsFromMockContext(mockContext) @@ -432,7 +431,7 @@ func createBicepProvider(t *testing.T, mockContext *mocks.MockContext) *BicepPro nil, ) - err = provider.Initialize(*mockContext.Context, projectDir, options) + err := provider.Initialize(*mockContext.Context, projectDir, options) require.NoError(t, err) return provider.(*BicepProvider) @@ -950,8 +949,7 @@ func TestUserDefinedTypes(t *testing.T) { }) azCli := mockazapi.NewAzureClientFromMockContext(mockContext) - bicepCli, err := bicep.NewCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + bicepCli := bicep.NewCli(mockContext.Console, mockContext.CommandRunner) env := environment.NewWithValues("test-env", map[string]string{}) mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { diff --git a/cli/azd/pkg/pipeline/github_provider_test.go b/cli/azd/pkg/pipeline/github_provider_test.go index 08588a328f6..43b37626fe2 100644 --- a/cli/azd/pkg/pipeline/github_provider_test.go +++ b/cli/azd/pkg/pipeline/github_provider_test.go @@ -86,12 +86,10 @@ func Test_gitHub_provider_preConfigure_check(t *testing.T) { func createGitHubCiProvider(t *testing.T, mockContext *mocks.MockContext) CiProvider { env := environment.New("test") - ghCli, err := github.NewGitHubCli( - *mockContext.Context, + ghCli := github.NewGitHubCli( mockContext.Console, mockContext.CommandRunner, ) - require.NoError(t, err) return NewGitHubCiProvider( env, diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index ea3c5d20746..8fae2850368 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -926,10 +926,12 @@ func (ch *ContainerHelper) packBuild( svc *ServiceConfig, dockerOptions DockerProjectOptions, imageName string) (*ServiceBuildResult, error) { - packCli, err := pack.NewCli(ctx, ch.console, ch.commandRunner) - if err != nil { + packCli := pack.NewCli(ch.console, ch.commandRunner) + if err := packCli.EnsureInstalled(ctx); err != nil { return nil, err } + + var err error builder := DefaultBuilderImage environ := []string{} userDefinedImage := false diff --git a/cli/azd/pkg/project/service_target_containerapp.go b/cli/azd/pkg/project/service_target_containerapp.go index 7ccdb226591..20887926a02 100644 --- a/cli/azd/pkg/project/service_target_containerapp.go +++ b/cli/azd/pkg/project/service_target_containerapp.go @@ -229,7 +229,11 @@ func (at *containerAppTarget) Deploy( fetchBicepCli := at.bicepCli if fetchBicepCli == nil { fetchBicepCli = func() (*bicep.Cli, error) { - return bicep.NewCli(ctx, at.console, at.commandRunner) + cli := bicep.NewCli(at.console, at.commandRunner) + if err := cli.EnsureInstalled(ctx); err != nil { + return nil, err + } + return cli, nil } } diff --git a/cli/azd/pkg/templates/gh_source.go b/cli/azd/pkg/templates/gh_source.go index d946d9deffc..27b8dda6dcd 100644 --- a/cli/azd/pkg/templates/gh_source.go +++ b/cli/azd/pkg/templates/gh_source.go @@ -186,6 +186,11 @@ func branchExists(ctx context.Context, ghCli *github.Cli, hostname string, repoS // ensureGitHubAuthenticated checks if the user is authenticated to GitHub and initiates login if not. // This ensures that subsequent GitHub API calls will not fail due to authentication issues. func ensureGitHubAuthenticated(ctx context.Context, ghCli *github.Cli, hostname string) error { + // Ensure GitHub CLI is installed before using it + if err := ghCli.EnsureInstalled(ctx); err != nil { + return fmt.Errorf("failed to ensure GitHub CLI is installed: %w", err) + } + authResult, err := ghCli.GetAuthStatus(ctx, hostname) if err != nil { return fmt.Errorf("failed to get auth status: %w", err) @@ -202,6 +207,11 @@ func ensureGitHubAuthenticated(ctx context.Context, ghCli *github.Cli, hostname // newGhTemplateSource creates a new template source from a Github repository. func newGhTemplateSource( ctx context.Context, name string, urlArg string, ghCli *github.Cli, console input.Console) (Source, error) { + // Ensure GitHub CLI is installed before using it + if err := ghCli.EnsureInstalled(ctx); err != nil { + return nil, fmt.Errorf("failed to ensure GitHub CLI is installed: %w", err) + } + // Parse the GitHub URL to extract repository information urlInfo, err := ParseGitHubUrl(ctx, urlArg, ghCli) if err != nil { diff --git a/cli/azd/pkg/templates/gh_source_test.go b/cli/azd/pkg/templates/gh_source_test.go index 3fc4544cb1e..9abf6268634 100644 --- a/cli/azd/pkg/templates/gh_source_test.go +++ b/cli/azd/pkg/templates/gh_source_test.go @@ -43,8 +43,7 @@ func Test_GhSourceRawFile(t *testing.T) { Stdout: string(expectedResult), }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) source, err := newGhTemplateSource( *mockContext.Context, name, "https://raw.github.com/owner/repo/branch/path/to/the/folder/file.json", ghCli, mockContext.Console) @@ -78,8 +77,7 @@ func Test_GhSourceApiFile(t *testing.T) { Stdout: string(expectedResult), }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) source, err := newGhTemplateSource( *mockContext.Context, name, "https://api.github.com/repos/owner/repo/contents/path/to/the/folder/file.json", ghCli, mockContext.Console) @@ -113,8 +111,7 @@ func Test_GhSourceUrl(t *testing.T) { Stdout: string(expectedResult), }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) source, err := newGhTemplateSource( *mockContext.Context, name, "https://github.com/owner/repo/branch/path/to/the/folder/file.json", ghCli, mockContext.Console) @@ -171,8 +168,7 @@ func Test_GhSourceUrlWithBlobAndBranchSlashes(t *testing.T) { return exec.RunResult{Stdout: "", Stderr: "Unexpected call", ExitCode: 1}, fmt.Errorf("unexpected call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // URL with branch name containing slash: agentserver/first-release source, err := newGhTemplateSource( @@ -234,8 +230,7 @@ func Test_GhSourceUrlWithTreeAndBranchSlashes(t *testing.T) { return exec.RunResult{Stdout: "", Stderr: "Unexpected call", ExitCode: 1}, fmt.Errorf("unexpected call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // URL with tree and branch name containing slash source, err := newGhTemplateSource( @@ -296,8 +291,7 @@ func Test_GhSourceRawFileWithBranchSlashes(t *testing.T) { return exec.RunResult{Stdout: "", Stderr: "Unexpected call", ExitCode: 1}, fmt.Errorf("unexpected call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // URL with branch containing multiple slashes source, err := newGhTemplateSource( @@ -341,8 +335,7 @@ func Test_GhSourceApiFileWithRefParameter(t *testing.T) { return exec.RunResult{Stdout: string(expectedResult)}, nil }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // API URL with ref parameter containing branch with slash source, err := newGhTemplateSource( @@ -387,8 +380,7 @@ func Test_ParseGitHubUrl_RawUrl(t *testing.T) { return exec.RunResult{Stdout: ""}, fmt.Errorf("unexpected API call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) urlInfo, err := ParseGitHubUrl( *mockContext.Context, @@ -432,8 +424,7 @@ func Test_ParseGitHubUrl_BlobUrl(t *testing.T) { return exec.RunResult{Stdout: ""}, fmt.Errorf("unexpected API call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) urlInfo, err := ParseGitHubUrl(*mockContext.Context, "https://github.com/owner/repo/blob/develop/src/main.go", ghCli) require.NoError(t, err) @@ -473,8 +464,7 @@ func Test_ParseGitHubUrl_TreeUrl(t *testing.T) { return exec.RunResult{Stdout: ""}, fmt.Errorf("unexpected API call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) urlInfo, err := ParseGitHubUrl( *mockContext.Context, @@ -498,8 +488,7 @@ func Test_ParseGitHubUrl_ApiUrl(t *testing.T) { Stdout: github.Version.String(), }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // API URLs don't need branch resolution, branch comes from query parameter urlInfo, err := ParseGitHubUrl( @@ -549,8 +538,7 @@ func Test_ParseGitHubUrl_BranchWithSlashes(t *testing.T) { return exec.RunResult{Stdout: ""}, fmt.Errorf("unexpected API call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // Now ParseGitHubUrl resolves the full branch name by checking GitHub API urlInfo, err := ParseGitHubUrl( @@ -595,8 +583,7 @@ func Test_ParseGitHubUrl_EnterpriseUrl(t *testing.T) { return exec.RunResult{Stdout: ""}, fmt.Errorf("unexpected API call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) urlInfo, err := ParseGitHubUrl( *mockContext.Context, @@ -631,8 +618,7 @@ func Test_ParseGitHubUrl_InvalidUrl(t *testing.T) { Stdout: github.Version.String(), }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -684,8 +670,7 @@ func Test_ParseGitHubUrl_NotAuthenticated(t *testing.T) { return exec.RunResult{Stdout: ""}, fmt.Errorf("unexpected API call") }) - ghCli, err := github.NewGitHubCli(*mockContext.Context, mockContext.Console, mockContext.CommandRunner) - require.NoError(t, err) + ghCli := github.NewGitHubCli(mockContext.Console, mockContext.CommandRunner) // This should trigger authentication before attempting to resolve the branch urlInfo, err := ParseGitHubUrl( diff --git a/cli/azd/pkg/tools/bicep/bicep.go b/cli/azd/pkg/tools/bicep/bicep.go index 97bab53e7d7..2ff136e9b3d 100644 --- a/cli/azd/pkg/tools/bicep/bicep.go +++ b/cli/azd/pkg/tools/bicep/bicep.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "runtime" + "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/azure/azure-dev/cli/azd/pkg/config" @@ -27,64 +28,80 @@ import ( // user). var Version semver.Version = semver.MustParse("0.39.26") -// NewCli creates a new Bicep CLI. Azd manages its own copy of the bicep CLI, stored in `$AZD_CONFIG_DIR/bin`. If -// bicep is not present at this location, or if it is present but is older than the minimum supported version, it is -// downloaded. -func NewCli( - ctx context.Context, - console input.Console, - commandRunner exec.CommandRunner, -) (*Cli, error) { - return newCliWithTransporter(ctx, console, commandRunner, http.DefaultClient) +// Cli is a wrapper around the bicep CLI. Call EnsureInstalled before using Build or BuildBicepParam. +type Cli struct { + path string + runner exec.CommandRunner + console input.Console + transporter policy.Transporter + + installOnce sync.Once + installErr error } -// newCliWithTransporter is like NewBicepCli but allows providing a custom transport to use when downloading the -// Bicep CLI, for testing purposes. +// NewCli creates a new Bicep CLI wrapper. The CLI is not yet installed; call EnsureInstalled +// before using Build or BuildBicepParam methods. +func NewCli(console input.Console, commandRunner exec.CommandRunner) *Cli { + return newCliWithTransporter(console, commandRunner, http.DefaultClient) +} + +// newCliWithTransporter is like NewCli but allows providing a custom transport for testing. func newCliWithTransporter( - ctx context.Context, console input.Console, commandRunner exec.CommandRunner, transporter policy.Transporter, -) (*Cli, error) { +) *Cli { + return &Cli{ + runner: commandRunner, + console: console, + transporter: transporter, + } +} + +// EnsureInstalled checks if bicep is available and downloads/upgrades if needed. +// This is safe to call multiple times; installation only happens once. +// Should be called with a request-scoped context before first use. +func (cli *Cli) EnsureInstalled(ctx context.Context) error { + cli.installOnce.Do(func() { + cli.installErr = cli.ensureInstalled(ctx) + }) + return cli.installErr +} + +func (cli *Cli) ensureInstalled(ctx context.Context) error { if override := os.Getenv("AZD_BICEP_TOOL_PATH"); override != "" { log.Printf("using external bicep tool: %s", override) - - return &Cli{ - path: override, - runner: commandRunner, - }, nil + cli.path = override + return nil } bicepPath, err := azdBicepPath() if err != nil { - return nil, fmt.Errorf("finding bicep: %w", err) + return fmt.Errorf("finding bicep: %w", err) } if _, err = os.Stat(bicepPath); err != nil && !errors.Is(err, os.ErrNotExist) { - return nil, fmt.Errorf("finding bicep: %w", err) + return fmt.Errorf("finding bicep: %w", err) } if errors.Is(err, os.ErrNotExist) { if err := os.MkdirAll(filepath.Dir(bicepPath), osutil.PermissionDirectory); err != nil { - return nil, fmt.Errorf("downloading bicep: %w", err) + return fmt.Errorf("downloading bicep: %w", err) } if err := runStep( - ctx, console, "Downloading Bicep", func() error { - return downloadBicep(ctx, transporter, Version, bicepPath) + ctx, cli.console, "Downloading Bicep", func() error { + return downloadBicep(ctx, cli.transporter, Version, bicepPath) }, ); err != nil { - return nil, fmt.Errorf("downloading bicep: %w", err) + return fmt.Errorf("downloading bicep: %w", err) } } - cli := &Cli{ - path: bicepPath, - runner: commandRunner, - } + cli.path = bicepPath ver, err := cli.version(ctx) if err != nil { - return nil, fmt.Errorf("checking bicep version: %w", err) + return fmt.Errorf("checking bicep version: %w", err) } log.Printf("bicep version: %s", ver) @@ -93,17 +110,17 @@ func newCliWithTransporter( log.Printf("installed bicep version %s is older than %s; updating.", ver.String(), Version.String()) if err := runStep( - ctx, console, "Upgrading Bicep", func() error { - return downloadBicep(ctx, transporter, Version, bicepPath) + ctx, cli.console, "Upgrading Bicep", func() error { + return downloadBicep(ctx, cli.transporter, Version, bicepPath) }, ); err != nil { - return nil, fmt.Errorf("upgrading bicep: %w", err) + return fmt.Errorf("upgrading bicep: %w", err) } } log.Printf("using local bicep: %s", bicepPath) - return cli, nil + return nil } // runStep runs a long running operation, using the console to show a spinner for progress and status. @@ -120,11 +137,6 @@ func runStep(ctx context.Context, console input.Console, title string, action fu return nil } -type Cli struct { - path string - runner exec.CommandRunner -} - // azdBicepPath returns the path where we store our local copy of bicep ($AZD_CONFIG_DIR/bin). func azdBicepPath() (string, error) { configDir, err := config.GetUserConfigDir() diff --git a/cli/azd/pkg/tools/bicep/bicep_test.go b/cli/azd/pkg/tools/bicep/bicep_test.go index fc2c7bece05..38b42a95e1e 100644 --- a/cli/azd/pkg/tools/bicep/bicep_test.go +++ b/cli/azd/pkg/tools/bicep/bicep_test.go @@ -45,9 +45,10 @@ func TestNewBicepCli(t *testing.T) { "", )) - cli, err := newCliWithTransporter( - *mockContext.Context, mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, + cli := newCliWithTransporter( + mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, ) + err := cli.EnsureInstalled(*mockContext.Context) require.NoError(t, err) require.NotNil(t, cli) @@ -117,9 +118,10 @@ func TestNewBicepCliWillUpgrade(t *testing.T) { return exec.NewRunResult(-1, "", "unexpected bicep file contents"), err }) - cli, err := newCliWithTransporter( - *mockContext.Context, mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, + cli := newCliWithTransporter( + mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, ) + err = cli.EnsureInstalled(*mockContext.Context) require.NoError(t, err) require.NotNil(t, cli) diff --git a/cli/azd/pkg/tools/github/github.go b/cli/azd/pkg/tools/github/github.go index 49040ac26d4..f74225eb2c6 100644 --- a/cli/azd/pkg/tools/github/github.go +++ b/cli/azd/pkg/tools/github/github.go @@ -19,6 +19,7 @@ import ( "regexp" "runtime" "strings" + "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/azure/azure-dev/cli/azd/pkg/config" @@ -31,67 +32,31 @@ import ( var _ tools.ExternalTool = (*Cli)(nil) -func NewGitHubCli(ctx context.Context, console input.Console, commandRunner exec.CommandRunner) (*Cli, error) { - return newGitHubCliImplementation(ctx, console, commandRunner, http.DefaultClient, downloadGh, extractGhCli) +// NewGitHubCli creates a new GitHub CLI wrapper. The CLI is not yet installed; call EnsureInstalled +// before using methods that require the gh binary. +func NewGitHubCli(console input.Console, commandRunner exec.CommandRunner) *Cli { + return newGitHubCliImplementation(console, commandRunner, http.DefaultClient, downloadGh, extractGhCli) } // Version is the minimum version of GitHub cli that we require (and the one we fetch when we fetch gh on // behalf of a user). var Version semver.Version = semver.MustParse("2.80.0") -// newGitHubCliImplementation is like NewGitHubCli but allows providing a custom transport to use when downloading the -// GitHub CLI, for testing purposes. +// newGitHubCliImplementation is like NewGitHubCli but allows providing a custom transport for testing. func newGitHubCliImplementation( - ctx context.Context, console input.Console, commandRunner exec.CommandRunner, transporter policy.Transporter, acquireGitHubCliImpl getGitHubCliImplementation, extractImplementation extractGitHubCliFromFileImplementation, -) (*Cli, error) { - if override := os.Getenv("AZD_GH_TOOL_PATH"); override != "" { - log.Printf("using external github cli tool: %s", override) - cli := &Cli{ - path: override, - commandRunner: commandRunner, - } - cli.logVersion(ctx) - - return cli, nil +) *Cli { + return &Cli{ + commandRunner: commandRunner, + console: console, + transporter: transporter, + acquireGitHubCliImpl: acquireGitHubCliImpl, + extractImplementation: extractImplementation, } - - githubCliPath, err := azdGithubCliPath() - if err != nil { - return nil, fmt.Errorf("getting github cli default path: %w", err) - } - - if _, err = os.Stat(githubCliPath); err != nil && !errors.Is(err, os.ErrNotExist) { - return nil, fmt.Errorf("getting file information from github cli default path: %w", err) - } - var installGhCli bool - if errors.Is(err, os.ErrNotExist) || !expectedVersionInstalled(ctx, commandRunner, githubCliPath) { - installGhCli = true - } - if installGhCli { - if err := os.MkdirAll(filepath.Dir(githubCliPath), osutil.PermissionDirectory); err != nil { - return nil, fmt.Errorf("creating github cli default path: %w", err) - } - - msg := "setting up github connection" - console.ShowSpinner(ctx, msg, input.Step) - err = acquireGitHubCliImpl(ctx, transporter, Version, extractImplementation, githubCliPath) - console.StopSpinner(ctx, "", input.Step) - if err != nil { - return nil, fmt.Errorf("setting up github connection: %w", err) - } - } - - cli := &Cli{ - path: githubCliPath, - commandRunner: commandRunner, - } - cli.logVersion(ctx) - return cli, nil } // azdGithubCliPath returns the path where we store our local copy of github cli ($AZD_CONFIG_DIR/bin). @@ -124,14 +89,70 @@ var ( ) type Cli struct { - commandRunner exec.CommandRunner - path string + commandRunner exec.CommandRunner + console input.Console + transporter policy.Transporter + acquireGitHubCliImpl getGitHubCliImplementation + extractImplementation extractGitHubCliFromFileImplementation + path string + + installOnce sync.Once + installErr error } -func (cli *Cli) CheckInstalled(ctx context.Context) error { +// EnsureInstalled checks if GitHub CLI is available and downloads/upgrades if needed. +// This is safe to call multiple times; installation only happens once. +// Should be called with a request-scoped context before first use. +func (cli *Cli) EnsureInstalled(ctx context.Context) error { + cli.installOnce.Do(func() { + cli.installErr = cli.ensureInstalled(ctx) + }) + return cli.installErr +} + +func (cli *Cli) ensureInstalled(ctx context.Context) error { + if override := os.Getenv("AZD_GH_TOOL_PATH"); override != "" { + log.Printf("using external github cli tool: %s", override) + cli.path = override + cli.logVersion(ctx) + return nil + } + + githubCliPath, err := azdGithubCliPath() + if err != nil { + return fmt.Errorf("getting github cli default path: %w", err) + } + + if _, err = os.Stat(githubCliPath); err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("getting file information from github cli default path: %w", err) + } + var installGhCli bool + if errors.Is(err, os.ErrNotExist) || !expectedVersionInstalled(ctx, cli.commandRunner, githubCliPath) { + installGhCli = true + } + if installGhCli { + if err := os.MkdirAll(filepath.Dir(githubCliPath), osutil.PermissionDirectory); err != nil { + return fmt.Errorf("creating github cli default path: %w", err) + } + + msg := "setting up github connection" + cli.console.ShowSpinner(ctx, msg, input.Step) + err = cli.acquireGitHubCliImpl(ctx, cli.transporter, Version, cli.extractImplementation, githubCliPath) + cli.console.StopSpinner(ctx, "", input.Step) + if err != nil { + return fmt.Errorf("setting up github connection: %w", err) + } + } + + cli.path = githubCliPath + cli.logVersion(ctx) return nil } +func (cli *Cli) CheckInstalled(ctx context.Context) error { + return cli.EnsureInstalled(ctx) +} + func expectedVersionInstalled(ctx context.Context, commandRunner exec.CommandRunner, binaryPath string) bool { ghVersion, err := tools.ExecuteCommand(ctx, commandRunner, binaryPath, "--version") if err != nil { diff --git a/cli/azd/pkg/tools/github/github_test.go b/cli/azd/pkg/tools/github/github_test.go index 2409e88da6c..333dbd971fc 100644 --- a/cli/azd/pkg/tools/github/github_test.go +++ b/cli/azd/pkg/tools/github/github_test.go @@ -37,7 +37,8 @@ func TestGithubCLIDeploymentEnvironments(t *testing.T) { envName := "copilot2" mockContext := mocks.NewMockContext(context.Background()) - cli, err := NewGitHubCli(context.Background(), mockContext.Console, commandRunner) + cli := NewGitHubCli(mockContext.Console, commandRunner) + err := cli.EnsureInstalled(context.Background()) require.NoError(t, err) err = cli.CreateEnvironmentIfNotExist(context.Background(), repoSlug, envName) @@ -147,14 +148,14 @@ func TestNewGitHubCli(t *testing.T) { return src, nil } - cli, err := newGitHubCliImplementation( - *mockContext.Context, + cli := newGitHubCliImplementation( mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, downloadGh, mockExtract, ) + err := cli.EnsureInstalled(*mockContext.Context) require.NoError(t, err) require.NotNil(t, cli) @@ -209,14 +210,14 @@ func TestGetAuthStatus(t *testing.T) { return src, nil } - cli, err := newGitHubCliImplementation( - *mockContext.Context, + cli := newGitHubCliImplementation( mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, downloadGh, mockExtract, ) + err := cli.EnsureInstalled(*mockContext.Context) require.NoError(t, err) require.NotNil(t, cli) @@ -252,14 +253,14 @@ func TestNewGitHubCliUpdate(t *testing.T) { return src, nil } - cli, err := newGitHubCliImplementation( - *mockContext.Context, + cli := newGitHubCliImplementation( mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, downloadGh, mockExtract, ) + err := cli.EnsureInstalled(*mockContext.Context) require.NoError(t, err) require.NotNil(t, cli) diff --git a/cli/azd/pkg/tools/pack/pack.go b/cli/azd/pkg/tools/pack/pack.go index 5c82bbcdf87..bb252a7a0c7 100644 --- a/cli/azd/pkg/tools/pack/pack.go +++ b/cli/azd/pkg/tools/pack/pack.go @@ -19,6 +19,7 @@ import ( "runtime" "strconv" "strings" + "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/azure/azure-dev/cli/azd/pkg/config" @@ -55,16 +56,15 @@ func (s *StatusCodeError) Unwrap() error { return s.Err } -// NewCli creates a new PackCli. azd manages its own copy of the pack CLI, stored in `$AZD_CONFIG_DIR/bin`. If -// pack is not present at this location, or if it is present but is older than the minimum supported version, it is -// downloaded. +// NewCli creates a new PackCli. The CLI is not yet installed; call EnsureInstalled before using +// methods that require the pack binary. azd manages its own copy of the pack CLI, stored in +// `$AZD_CONFIG_DIR/bin`. If pack is not present at this location, or if it is present but is older +// than the minimum supported version, it is downloaded. func NewCli( - ctx context.Context, console input.Console, commandRunner exec.CommandRunner, -) (*Cli, error) { +) *Cli { return newPackCliImpl( - ctx, console, commandRunner, http.DefaultClient, @@ -96,49 +96,72 @@ func packCliPath() (string, error) { } func newPackCliImpl( - ctx context.Context, console input.Console, commandRunner exec.CommandRunner, transporter policy.Transporter, - extract func(string, string) (string, error)) (*Cli, error) { + extract func(string, string) (string, error)) *Cli { + return &Cli{ + runner: commandRunner, + console: console, + transporter: transporter, + extract: extract, + } +} + +type Cli struct { + path string + runner exec.CommandRunner + console input.Console + transporter policy.Transporter + extract func(string, string) (string, error) + + installOnce sync.Once + installErr error +} + +// EnsureInstalled checks if pack CLI is available and downloads/upgrades if needed. +// This is safe to call multiple times; installation only happens once. +// Should be called with a request-scoped context before first use. +func (cli *Cli) EnsureInstalled(ctx context.Context) error { + cli.installOnce.Do(func() { + cli.installErr = cli.ensureInstalled(ctx) + }) + return cli.installErr +} + +func (cli *Cli) ensureInstalled(ctx context.Context) error { if override := os.Getenv("AZD_PACK_TOOL_PATH"); override != "" { log.Printf("using external pack tool: %s", override) - - return &Cli{ - path: override, - runner: commandRunner, - }, nil + cli.path = override + return nil } cliPath, err := packCliPath() if err != nil { - return nil, fmt.Errorf("finding pack: %w", err) + return fmt.Errorf("finding pack: %w", err) } if _, err = os.Stat(cliPath); err != nil && !errors.Is(err, os.ErrNotExist) { - return nil, fmt.Errorf("finding pack: %w", err) + return fmt.Errorf("finding pack: %w", err) } if errors.Is(err, os.ErrNotExist) { if err := os.MkdirAll(filepath.Dir(cliPath), osutil.PermissionDirectory); err != nil { - return nil, fmt.Errorf("downloading pack: %w", err) + return fmt.Errorf("downloading pack: %w", err) } msg := "Acquiring pack cli" - console.ShowSpinner(ctx, msg, input.Step) - err := downloadPack(ctx, transporter, Version, extract, cliPath) - console.StopSpinner(ctx, "", input.Step) + cli.console.ShowSpinner(ctx, msg, input.Step) + err := downloadPack(ctx, cli.transporter, Version, cli.extract, cliPath) + cli.console.StopSpinner(ctx, "", input.Step) if err != nil { - return nil, fmt.Errorf("downloading pack: %w", err) + return fmt.Errorf("downloading pack: %w", err) } } - cli := &Cli{ - path: cliPath, - runner: commandRunner, - } + cli.path = cliPath ver, err := cli.version(ctx) if err != nil { - return nil, fmt.Errorf("checking pack version: %w", err) + return fmt.Errorf("checking pack version: %w", err) } log.Printf("pack version: %s", ver) @@ -147,22 +170,16 @@ func newPackCliImpl( log.Printf("installed pack version %s is older than %s; updating.", ver.String(), Version.String()) msg := "Upgrading pack" - console.ShowSpinner(ctx, msg, input.Step) - err := downloadPack(ctx, transporter, Version, extract, cliPath) - console.StopSpinner(ctx, "", input.Step) + cli.console.ShowSpinner(ctx, msg, input.Step) + err := downloadPack(ctx, cli.transporter, Version, cli.extract, cliPath) + cli.console.StopSpinner(ctx, "", input.Step) if err != nil { - return nil, fmt.Errorf("upgrading pack: %w", err) + return fmt.Errorf("upgrading pack: %w", err) } } log.Printf("using local pack: %s", cliPath) - - return cli, nil -} - -type Cli struct { - path string - runner exec.CommandRunner + return nil } func (cli *Cli) version(ctx context.Context) (semver.Version, error) { diff --git a/cli/azd/pkg/tools/pack/pack_test.go b/cli/azd/pkg/tools/pack/pack_test.go index 84b8ef9aebd..46cce022e2f 100644 --- a/cli/azd/pkg/tools/pack/pack_test.go +++ b/cli/azd/pkg/tools/pack/pack_test.go @@ -174,13 +174,13 @@ func TestNewPackCliInstall(t *testing.T) { return src, nil } - cli, err := newPackCliImpl( - *mockContext.Context, + cli := newPackCliImpl( mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, mockExtract, ) + err := cli.EnsureInstalled(*mockContext.Context) require.NoError(t, err) require.NotNil(t, cli) @@ -228,13 +228,13 @@ func TestNewPackCliUpgrade(t *testing.T) { return src, nil } - cli, err := newPackCliImpl( - *mockContext.Context, + cli := newPackCliImpl( mockContext.Console, mockContext.CommandRunner, mockContext.HttpClient, mockExtract, ) + err = cli.EnsureInstalled(*mockContext.Context) require.NoError(t, err) require.NotNil(t, cli) diff --git a/cli/azd/test/functional/aspire_test.go b/cli/azd/test/functional/aspire_test.go index fceb80438a1..e04e970ceb9 100644 --- a/cli/azd/test/functional/aspire_test.go +++ b/cli/azd/test/functional/aspire_test.go @@ -178,7 +178,8 @@ func Test_CLI_Aspire_DetectGen(t *testing.T) { _, err = cli.RunCommand(ctx, "infra", "synth") require.NoError(t, err) - bicepCli, err := bicep.NewCli(ctx, mockinput.NewMockConsole(), exec.NewCommandRunner(nil)) + bicepCli := bicep.NewCli(mockinput.NewMockConsole(), exec.NewCommandRunner(nil)) + err = bicepCli.EnsureInstalled(ctx) require.NoError(t, err) // Validate bicep builds without errors @@ -230,7 +231,8 @@ func Test_CLI_Aspire_DetectGen(t *testing.T) { _, err = cli.RunCommand(ctx, "infra", "generate") require.NoError(t, err) - bicepCli, err := bicep.NewCli(ctx, mockinput.NewMockConsole(), exec.NewCommandRunner(nil)) + bicepCli := bicep.NewCli(mockinput.NewMockConsole(), exec.NewCommandRunner(nil)) + err = bicepCli.EnsureInstalled(ctx) require.NoError(t, err) // Validate bicep builds without errors From cf198aa5b83751d4785f7a8c2b0ee05737bfb16f Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Tue, 20 Jan 2026 16:14:36 -0800 Subject: [PATCH 4/5] ignore nil context warning --- cli/azd/cmd/root.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/azd/cmd/root.go b/cli/azd/cmd/root.go index 809dcc56d27..9abf0cff426 100644 --- a/cli/azd/cmd/root.go +++ b/cli/azd/cmd/root.go @@ -557,6 +557,7 @@ func getCmdRootHelpCommands(cmd *cobra.Command) (result string) { // This prevents stale context issues where a cancelled context might be reused by singletons // or cached components. See: https://github.com/Azure/azure-dev/issues/6530 func clearCommandContext(cmd *cobra.Command) { + //nolint:staticcheck // SA1012: Intentionally passing nil to clear the context and reset to cobra's default state cmd.SetContext(nil) for _, child := range cmd.Commands() { clearCommandContext(child) From 5283caf19f7c09ade10cd7fb89e5cdc2111c1f43 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Tue, 20 Jan 2026 17:39:06 -0800 Subject: [PATCH 5/5] Fixes bicep unit tests --- cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go index be51b56a307..b6a60edf269 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go @@ -950,6 +950,7 @@ func TestUserDefinedTypes(t *testing.T) { azCli := mockazapi.NewAzureClientFromMockContext(mockContext) bicepCli := bicep.NewCli(mockContext.Console, mockContext.CommandRunner) + require.NoError(t, bicepCli.EnsureInstalled(*mockContext.Context)) env := environment.NewWithValues("test-env", map[string]string{}) mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool {