From 64c01172dd01c3c5c391dc001ceea28b0bd845cd Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Fri, 20 Mar 2026 09:47:05 -0700 Subject: [PATCH] fix: preserve IoC singletons during workflow step re-execution PR #7171 changed workflowCmdAdapter to rebuild the cobra command tree via NewRootCmd on each workflow step. However, NewRootCmd calls registerCommonDependencies on the same container, which re-registers singletons. The golobby IoC container replaces cached instances on re-registration, so any state attached to the original instance (e.g. event handlers for lifecycle hooks on ProjectConfig/ServiceConfig) was silently lost. Fix: introduce newRootCmdSkipRegistration() which builds a fresh cobra command tree without re-registering shared IoC dependencies or re-initializing the platform. The workflow adapter now uses this, so cached singletons (and their event handlers) are preserved while still getting a clean command tree free of stale cobra state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/cmd/container.go | 2 +- cli/azd/cmd/container_test.go | 151 +++++++++++++++++++++- cli/azd/cmd/middleware/hooks_test.go | 181 ++++++++++++++++++++++++++- cli/azd/cmd/root.go | 28 ++++- 4 files changed, 355 insertions(+), 7 deletions(-) diff --git a/cli/azd/cmd/container.go b/cli/azd/cmd/container.go index 4d73091fe3d..665f759528f 100644 --- a/cli/azd/cmd/container.go +++ b/cli/azd/cmd/container.go @@ -912,7 +912,7 @@ func registerCommonDependencies(container *ioc.NestedContainer) { container.MustRegisterSingleton(func() workflow.AzdCommandRunner { return &workflowCmdAdapter{ newCommand: func() *cobra.Command { - return NewRootCmd(false, nil, container) + return newRootCmdWithoutRegistration(container) }, globalArgs: extractGlobalArgs(), } diff --git a/cli/azd/cmd/container_test.go b/cli/azd/cmd/container_test.go index 9cfdb081eff..e00e76b3040 100644 --- a/cli/azd/cmd/container_test.go +++ b/cli/azd/cmd/container_test.go @@ -162,12 +162,12 @@ type testConcreteComponent[T comparable] struct { concrete T } -// Test_WorkflowCmdAdapter_ContextPropagation validates that the workflowCmdAdapter +// Test_workflowCmdAdapter_ContextPropagation validates that the workflowCmdAdapter // 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) { +func Test_workflowCmdAdapter_ContextPropagation(t *testing.T) { t.Run("SubcommandReceivesChildActionContext", func(t *testing.T) { // Track which contexts were seen by the subcommand var receivedContexts []context.Context @@ -409,4 +409,151 @@ func Test_WorkflowCmdAdapter_ContextPropagation(t *testing.T) { require.True(t, foundProvision2, "second tree: provision command should be registered") require.NotSame(t, rootCmd, rootCmd2, "each NewRootCmd call should produce a distinct instance") }) + + t.Run("WorkflowAdapterMiddlewareRunsForChildActions", func(t *testing.T) { + // Verify that when the workflowCmdAdapter executes a command, the middleware chain + // (registered on the command tree) is invoked despite the context being a child action. + // This validates that hooks middleware would fire during workflow step execution. + var middlewareRan bool + var receivedIsChild bool + + newCommand := func() *cobra.Command { + rootCmd := &cobra.Command{Use: "root"} + + // Create a child action descriptor-style setup: + // The "provision" command wraps its RunE to simulate middleware execution + provisionCmd := &cobra.Command{ + Use: "provision", + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + middlewareRan = true + receivedIsChild = middleware.IsChildAction(ctx) + return nil + }, + } + rootCmd.AddCommand(provisionCmd) + return rootCmd + } + + adapter := &workflowCmdAdapter{newCommand: newCommand} + ctx := context.WithoutCancel(context.Background()) + + // Execute "provision" through the adapter (simulates workflow step) + err := adapter.ExecuteContext(ctx, []string{"provision"}) + require.NoError(t, err) + require.True(t, middlewareRan, "Provision command should have been executed") + require.True(t, receivedIsChild, + "Context should be marked as child action when executed through workflow adapter") + }) + + t.Run("WorkflowAdapterMiddlewareChainForAllSteps", func(t *testing.T) { + // Simulate the full workflow execution path: package → provision → deploy + // Verify each step's command runs with the child action context and fresh tree + var executedCommands []string + + newCommand := func() *cobra.Command { + rootCmd := &cobra.Command{Use: "root"} + + for _, cmdName := range []string{"package", "provision", "deploy"} { + name := cmdName // capture for closure + cmd := &cobra.Command{ + Use: name, + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + require.True(t, middleware.IsChildAction(ctx), + "Step %q should have child action context", name) + executedCommands = append(executedCommands, name) + return nil + }, + } + if name == "package" || name == "deploy" { + cmd.Flags().Bool("all", false, "") + } + rootCmd.AddCommand(cmd) + } + return rootCmd + } + + adapter := &workflowCmdAdapter{newCommand: newCommand} + ctx := context.WithoutCancel(context.Background()) + + // Simulate the default "up" workflow steps + steps := [][]string{ + {"package", "--all"}, + {"provision"}, + {"deploy", "--all"}, + } + + for _, args := range steps { + err := adapter.ExecuteContext(ctx, args) + require.NoError(t, err, "Step %v should succeed", args) + } + + require.Equal(t, []string{"package", "provision", "deploy"}, executedCommands, + "All workflow steps should execute in order") + }) +} + +func Test_NewRootCmd_ReregistrationReplacesProjectConfig(t *testing.T) { + // This test proves the regression from PR #7171: when workflowCmdAdapter called + // NewRootCmd (with full registration) for each workflow step, registerCommonDependencies + // re-registered singletons. The golobby IoC container replaces cached singleton instances + // on re-registration, so event handlers registered on ProjectConfig/ServiceConfig (by the + // hooks middleware) were silently lost. + // + // Steps: + // 1. Create root command (registers dependencies) + // 2. Resolve ProjectConfig, add an event handler + // 3. Create another root command (re-registers dependencies) + // 4. Resolve ProjectConfig again + // 5. Validate the handler is gone (proving the bug) + // 6. Use newRootCmdWithoutRegistration instead, validate handler is preserved (proving the fix) + + container := ioc.NewNestedContainer(nil) + ctx := context.WithoutCancel(context.Background()) + ioc.RegisterInstance(container, ctx) + ioc.RegisterInstance(container, &internal.GlobalCommandOptions{}) + + // Set up a project directory with azure.yaml so ProjectConfig can be resolved + dir := t.TempDir() + t.Chdir(dir) + azdCtx := azdcontext.NewAzdContextWithDirectory(dir) + ioc.RegisterInstance(container, azdCtx) + + projectConfig := &project.ProjectConfig{ + Name: "test-project", + } + _ = project.Save(ctx, projectConfig, azdCtx.ProjectPath()) + + // Step 1: Create root command (registers dependencies including ProjectConfig factory) + _ = NewRootCmd(false, nil, container) + + // Step 2: Resolve ProjectConfig and add an event handler (simulates hooks middleware) + var pc1 *project.ProjectConfig + require.NoError(t, container.Resolve(&pc1)) + + // Step 3: Create another root command with full re-registration + _ = NewRootCmd(false, nil, container) + + // Step 4: Resolve ProjectConfig again + var pc2 *project.ProjectConfig + require.NoError(t, container.Resolve(&pc2)) + + // Step 5: The re-registration replaced the singleton — it's a different instance + require.NotSame(t, pc1, pc2, + "BUG PROOF: NewRootCmd re-registration replaces the cached ProjectConfig singleton, "+ + "losing any event handlers attached to the original instance") + + // Step 6: Now use newRootCmdWithoutRegistration and verify the instance is preserved + var pc3 *project.ProjectConfig + require.NoError(t, container.Resolve(&pc3)) + + _ = newRootCmdWithoutRegistration(container) + + var pc4 *project.ProjectConfig + require.NoError(t, container.Resolve(&pc4)) + + require.Same(t, pc3, pc4, + "FIX PROOF: newRootCmdWithoutRegistration preserves the cached ProjectConfig singleton, "+ + "keeping event handlers intact") } diff --git a/cli/azd/cmd/middleware/hooks_test.go b/cli/azd/cmd/middleware/hooks_test.go index f33d531d9dc..df1c614fbae 100644 --- a/cli/azd/cmd/middleware/hooks_test.go +++ b/cli/azd/cmd/middleware/hooks_test.go @@ -335,6 +335,17 @@ func runMiddleware( projectConfig *project.ProjectConfig, runOptions *Options, nextFn NextFn, +) (*actions.ActionResult, error) { + return runMiddlewareWithContext(*mockContext.Context, mockContext, envName, projectConfig, runOptions, nextFn) +} + +func runMiddlewareWithContext( + ctx context.Context, + mockContext *mocks.MockContext, + envName string, + projectConfig *project.ProjectConfig, + runOptions *Options, + nextFn NextFn, ) (*actions.ActionResult, error) { env := environment.NewWithValues(envName, nil) @@ -354,7 +365,7 @@ func runMiddleware( mockContext.Container, ) - result, err := middleware.Run(*mockContext.Context, nextFn) + result, err := middleware.Run(ctx, nextFn) return result, err } @@ -540,6 +551,174 @@ func Test_PowerShellWarning_WithoutPowerShellHooks(t *testing.T) { require.False(t, foundWarning, "Expected no PowerShell warning for bash hooks") } +// Test_CommandHooks_ChildAction_HooksStillFire verifies that command hooks fire even when running +// as a child action (e.g., "provision" step inside "azd up" workflow). PR #7171 changed the +// workflowCmdAdapter to rebuild the command tree, and this test ensures hooks still execute +// for workflow step commands. +func Test_CommandHooks_ChildAction_HooksStillFire(t *testing.T) { + tests := []struct { + name string + commandPath string + hookName string + }{ + { + name: "ProvisionHooksFireInWorkflow", + commandPath: "azd provision", + hookName: "preprovision", + }, + { + name: "DeployHooksFireInWorkflow", + commandPath: "azd deploy", + hookName: "predeploy", + }, + { + name: "PackageHooksFireInWorkflow", + commandPath: "azd package", + hookName: "prepackage", + }, + { + name: "RestoreHooksFireInWorkflow", + commandPath: "azd restore", + hookName: "prerestore", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + azdContext := createAzdContext(t) + + envName := "test" + runOptions := Options{CommandPath: tt.commandPath} + + projectConfig := project.ProjectConfig{ + Name: envName, + Hooks: map[string][]*ext.HookConfig{ + tt.hookName: { + { + Run: "echo 'hook running'", + Shell: ext.ShellTypeBash, + }, + }, + }, + } + + err := ensureAzdValid(mockContext, azdContext, envName, &projectConfig) + require.NoError(t, err) + + nextFn, actionRan := createNextFn() + hookRan := setupHookMock(mockContext, 0) + + // Simulate workflow execution: mark context as child action + childCtx := WithChildAction(*mockContext.Context) + result, err := runMiddlewareWithContext( + childCtx, mockContext, envName, &projectConfig, &runOptions, nextFn, + ) + + require.NotNil(t, result) + require.NoError(t, err) + require.True(t, *hookRan, "Hook %q should fire even when running as a child action (workflow step)", tt.hookName) + require.True(t, *actionRan, "Action should run for child action") + }) + } +} + +// Test_CommandHooks_ChildAction_SkipsValidationOnly verifies that when running as a child action, +// hook validation warnings are suppressed but hooks still execute. This ensures the IsChildAction +// guard in HooksMiddleware.Run() only affects validation, not hook execution itself. +func Test_CommandHooks_ChildAction_SkipsValidationOnly(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + azdContext := createAzdContext(t) + + envName := "test" + runOptions := Options{CommandPath: "azd provision"} + + projectConfig := project.ProjectConfig{ + Name: envName, + Hooks: map[string][]*ext.HookConfig{ + "preprovision": { + { + Run: "echo 'preprovision hook'", + Shell: ext.ShellTypeBash, + }, + }, + "postprovision": { + { + Run: "echo 'postprovision hook'", + Shell: ext.ShellTypeBash, + }, + }, + }, + } + + err := ensureAzdValid(mockContext, azdContext, envName, &projectConfig) + require.NoError(t, err) + + hookCount := 0 + mockContext.CommandRunner.When(func(args exec.RunArgs, command string) bool { + return true + }).RespondFn(func(args exec.RunArgs) (exec.RunResult, error) { + hookCount++ + return exec.NewRunResult(0, "", ""), nil + }) + + nextFn, actionRan := createNextFn() + + // Execute as child action (workflow step) + childCtx := WithChildAction(*mockContext.Context) + result, err := runMiddlewareWithContext( + childCtx, mockContext, envName, &projectConfig, &runOptions, nextFn, + ) + + require.NotNil(t, result) + require.NoError(t, err) + require.True(t, *actionRan, "Action should run") + + // Both pre and post hooks should fire (2 hooks total) + require.Equal(t, 2, hookCount, + "Both preprovision and postprovision hooks should fire for child actions") +} + +// Test_CommandHooks_ChildAction_PreHookError_StopsAction verifies that when running as a child +// action, a failing pre-hook still prevents the action from executing (same behavior as direct +// command execution). +func Test_CommandHooks_ChildAction_PreHookError_StopsAction(t *testing.T) { + mockContext := mocks.NewMockContext(context.Background()) + azdContext := createAzdContext(t) + + envName := "test" + runOptions := Options{CommandPath: "azd provision"} + + projectConfig := project.ProjectConfig{ + Name: envName, + Hooks: map[string][]*ext.HookConfig{ + "preprovision": { + { + Run: "exit 1", + Shell: ext.ShellTypeBash, + }, + }, + }, + } + + err := ensureAzdValid(mockContext, azdContext, envName, &projectConfig) + require.NoError(t, err) + + nextFn, actionRan := createNextFn() + hookRan := setupHookMock(mockContext, 1) // Non-zero exit code + + // Execute as child action (workflow step) + childCtx := WithChildAction(*mockContext.Context) + result, err := runMiddlewareWithContext( + childCtx, mockContext, envName, &projectConfig, &runOptions, nextFn, + ) + + require.Nil(t, result) + require.Error(t, err) + require.True(t, *hookRan, "Pre-hook should still execute for child actions") + require.False(t, *actionRan, "Action should NOT run when pre-hook fails") +} + func Test_PowerShellWarning_WithPwshAvailable(t *testing.T) { mockContext := mocks.NewMockContext(context.Background()) azdContext := createAzdContext(t) diff --git a/cli/azd/cmd/root.go b/cli/azd/cmd/root.go index 81329e0e994..9e3fccffe1b 100644 --- a/cli/azd/cmd/root.go +++ b/cli/azd/cmd/root.go @@ -41,6 +41,23 @@ func NewRootCmd( staticHelp bool, middlewareChain []*actions.MiddlewareRegistration, rootContainer *ioc.NestedContainer, +) *cobra.Command { + return newRootCmd(staticHelp, middlewareChain, rootContainer, true) +} + +// newRootCmdWithoutRegistration builds a fresh cobra command tree for workflow re-execution without +// re-registering shared IoC dependencies. This preserves cached singleton instances (and any event +// handlers attached to them, such as lifecycle hooks on ProjectConfig/ServiceConfig) while still +// providing a clean command tree free of stale cobra state. +func newRootCmdWithoutRegistration(rootContainer *ioc.NestedContainer) *cobra.Command { + return newRootCmd(false, nil, rootContainer, false) +} + +func newRootCmd( + staticHelp bool, + middlewareChain []*actions.MiddlewareRegistration, + rootContainer *ioc.NestedContainer, + registerDependencies bool, ) *cobra.Command { prevDir := "" @@ -436,7 +453,10 @@ func NewRootCmd( }) ioc.RegisterNamedInstance(rootContainer, "root-cmd", rootCmd) - registerCommonDependencies(rootContainer) + + if registerDependencies { + registerCommonDependencies(rootContainer) + } // Register the 'extension' commands err := rootContainer.Invoke(func(extensionManager *extensions.Manager) error { @@ -473,8 +493,10 @@ func NewRootCmd( // Initialize the platform specific components for the IoC container // Only container resolution errors will return an error // Invalid configurations will fall back to default platform - if _, err := platform.Initialize(rootContainer, azd.PlatformKindDefault); err != nil { - panic(err) + if registerDependencies { + if _, err := platform.Initialize(rootContainer, azd.PlatformKindDefault); err != nil { + panic(err) + } } // Compose the hierarchy of action descriptions into cobra commands