From 40f13a8d99ed7a0811f004db9fd9d3eb9ec1aa2c Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Thu, 9 Apr 2026 09:03:19 -0700 Subject: [PATCH 01/12] perf: performance foundations for parallel deployment execution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .gitignore | 7 + .vscode/cspell.misc.yaml | 20 ++ cli/azd/.vscode/cspell-azd-dictionary.txt | 6 + cli/azd/cmd/deps.go | 3 +- cli/azd/pkg/auth/federated_token_client.go | 2 +- cli/azd/pkg/azapi/container_registry.go | 62 +++- cli/azd/pkg/azapi/resource_service.go | 24 +- cli/azd/pkg/azapi/stack_deployments.go | 64 +++- cli/azd/pkg/azapi/standard_deployments.go | 121 +++++++- cli/azd/pkg/azapi/webapp.go | 198 +++++++++++- cli/azd/pkg/azapi/webapp_test.go | 124 ++++++++ cli/azd/pkg/azsdk/zip_deploy_client.go | 33 ++ cli/azd/pkg/containerapps/container_app.go | 22 +- cli/azd/pkg/httputil/util.go | 20 +- cli/azd/pkg/httputil/util_test.go | 30 ++ cli/azd/pkg/project/container_helper.go | 95 ++++-- .../container_helper_coverage3_test.go | 127 ++++++++ cli/azd/pkg/project/container_helper_test.go | 6 +- .../pkg/project/framework_service_docker.go | 16 +- .../project/framework_service_docker_test.go | 52 +++- docs/specs/perf-foundations/spec.md | 287 ++++++++++++++++++ 21 files changed, 1214 insertions(+), 105 deletions(-) create mode 100644 cli/azd/pkg/azapi/webapp_test.go create mode 100644 docs/specs/perf-foundations/spec.md diff --git a/.gitignore b/.gitignore index 1e834015454..156413e3727 100644 --- a/.gitignore +++ b/.gitignore @@ -78,3 +78,10 @@ cli/azd/extensions/microsoft.azd.concurx/concurx.exe cli/azd/extensions/azure.appservice/azureappservice cli/azd/extensions/azure.appservice/azureappservice.exe .squad/ + +# Session artifacts +cli/azd/cover-* +cli/azd/cover_* +review-*.diff + +.playwright-mcp/ diff --git a/.vscode/cspell.misc.yaml b/.vscode/cspell.misc.yaml index 9b6242d0f58..86ed51bc94c 100644 --- a/.vscode/cspell.misc.yaml +++ b/.vscode/cspell.misc.yaml @@ -39,6 +39,26 @@ overrides: - filename: ./README.md words: - VSIX + - filename: ./docs/specs/perf-foundations/** + words: + - Alives + - appsettings + - appservice + - armdeploymentstacks + - azapi + - azsdk + - containerapps + - golangci + - goroutines + - httputil + - keepalives + - nilerr + - nolint + - remotebuild + - resourcegroup + - singleflight + - stdlib + - whatif - filename: schemas/**/azure.yaml.json words: - prodapi diff --git a/cli/azd/.vscode/cspell-azd-dictionary.txt b/cli/azd/.vscode/cspell-azd-dictionary.txt index 88f4bd754b8..2eb4d93bd64 100644 --- a/cli/azd/.vscode/cspell-azd-dictionary.txt +++ b/cli/azd/.vscode/cspell-azd-dictionary.txt @@ -47,6 +47,7 @@ appinsightsexporter appinsightsstorage appplatform appservice +appsettings appuser arget armapimanagement @@ -175,6 +176,7 @@ jmespath jongio jquery keychain +keepalives kubelogin langchain langchaingo @@ -205,6 +207,7 @@ mysqlclient mysqldb nazd ndjson +nilerr nobanner nodeapp nolint @@ -249,9 +252,11 @@ rabbitmq reauthentication relogin remarshal +remotebuild repourl requirepass resourcegraph +resourcegroup restoreapp retriable runtimes @@ -304,6 +309,7 @@ vuejs webappignore webfrontend westus2 +whatif wireinject yacspin yamlnode diff --git a/cli/azd/cmd/deps.go b/cli/azd/cmd/deps.go index a37ea50b48a..9d318018ca7 100644 --- a/cli/azd/cmd/deps.go +++ b/cli/azd/cmd/deps.go @@ -8,11 +8,12 @@ package cmd import ( "net/http" + "github.com/azure/azure-dev/cli/azd/pkg/httputil" "github.com/benbjohnson/clock" ) func createHttpClient() *http.Client { - return http.DefaultClient + return &http.Client{Transport: httputil.TunedTransport()} } func createClock() clock.Clock { diff --git a/cli/azd/pkg/auth/federated_token_client.go b/cli/azd/pkg/auth/federated_token_client.go index 76bb2e3a44c..98fa224c328 100644 --- a/cli/azd/pkg/auth/federated_token_client.go +++ b/cli/azd/pkg/auth/federated_token_client.go @@ -32,9 +32,9 @@ func (c *FederatedTokenClient) TokenForAudience(ctx context.Context, audience st if err != nil { return "", fmt.Errorf("sending request: %w", err) } - defer res.Body.Close() if !runtime.HasStatusCode(res, http.StatusOK) { + defer res.Body.Close() return "", fmt.Errorf("expected 200 response, got: %d", res.StatusCode) } diff --git a/cli/azd/pkg/azapi/container_registry.go b/cli/azd/pkg/azapi/container_registry.go index 5f9dd79b650..0240dad3a66 100644 --- a/cli/azd/pkg/azapi/container_registry.go +++ b/cli/azd/pkg/azapi/container_registry.go @@ -13,6 +13,8 @@ import ( "net/url" "slices" "strings" + "sync" + "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -25,6 +27,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/azure" "github.com/azure/azure-dev/cli/azd/pkg/httputil" "github.com/azure/azure-dev/cli/azd/pkg/tools/docker" + "golang.org/x/sync/singleflight" ) // Credentials for authenticating with a docker registry, @@ -61,6 +64,12 @@ type containerRegistryService struct { docker *docker.Cli armClientOptions *arm.ClientOptions coreClientOptions *azcore.ClientOptions + // loginGroup deduplicates concurrent login attempts to the same registry. + // When multiple services share one ACR, only the first goroutine performs + // the credential exchange + docker login; others wait for its result. + loginGroup singleflight.Group + // loginDone tracks registries that have already been authenticated this session. + loginDone sync.Map } // Creates a new instance of the ContainerRegistryService @@ -118,20 +127,51 @@ func (crs *containerRegistryService) FindContainerRegistryResourceGroup( } func (crs *containerRegistryService) Login(ctx context.Context, subscriptionId string, loginServer string) error { - dockerCreds, err := crs.Credentials(ctx, subscriptionId, loginServer) - if err != nil { - return err + cacheKey := subscriptionId + ":" + loginServer + if _, ok := crs.loginDone.Load(cacheKey); ok { + log.Printf("skipping redundant login to %q (already authenticated this session)\n", loginServer) + return nil } - err = crs.docker.Login(ctx, dockerCreds.LoginServer, dockerCreds.Username, dockerCreds.Password) - if err != nil { - return fmt.Errorf( - "failed logging into docker registry %s: %w", - loginServer, - err) - } + // singleflight deduplicates concurrent login attempts to the same registry. + // We use DoChan so each caller can select on its own ctx.Done(), avoiding the + // problem where one caller's cancellation fails all waiters. + ch := crs.loginGroup.DoChan(cacheKey, func() (any, error) { + // Double-check after winning the singleflight race. + if _, ok := crs.loginDone.Load(cacheKey); ok { + return nil, nil + } + + // Use context.WithoutCancel so the shared work isn't tied to a single + // caller's context. Add a bounded timeout so the shared login cannot + // hang indefinitely if Credentials or docker login gets stuck. + const loginTimeout = 5 * time.Minute + opCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), loginTimeout) + defer cancel() + + dockerCreds, err := crs.Credentials(opCtx, subscriptionId, loginServer) + if err != nil { + return nil, err + } + + err = crs.docker.Login(opCtx, dockerCreds.LoginServer, dockerCreds.Username, dockerCreds.Password) + if err != nil { + return nil, fmt.Errorf( + "failed logging into docker registry %q: %w", + loginServer, + err) + } - return nil + crs.loginDone.Store(cacheKey, true) + return nil, nil + }) + + select { + case <-ctx.Done(): + return ctx.Err() + case res := <-ch: + return res.Err + } } // Credentials gets the credentials that could be used to login to the specified container registry. It prefers to use diff --git a/cli/azd/pkg/azapi/resource_service.go b/cli/azd/pkg/azapi/resource_service.go index 50a019d260e..41e6ceeed33 100644 --- a/cli/azd/pkg/azapi/resource_service.go +++ b/cli/azd/pkg/azapi/resource_service.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/http" + "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -61,6 +62,13 @@ type ListResourceGroupResourcesOptions struct { type ResourceService struct { credentialProvider account.SubscriptionCredentialProvider armClientOptions *arm.ClientOptions + + // resourcesClients caches armresources.Client instances per subscription ID. + // Azure SDK ARM clients are safe for concurrent use. + resourcesClients sync.Map // map[string]*armresources.Client + + // resourceGroupClients caches ResourceGroupsClient instances per subscription ID. + resourceGroupClients sync.Map // map[string]*armresources.ResourceGroupsClient } func NewResourceService( @@ -344,6 +352,10 @@ func (rs *ResourceService) GetResourceGroup( } func (rs *ResourceService) createResourcesClient(ctx context.Context, subscriptionId string) (*armresources.Client, error) { + if cached, ok := rs.resourcesClients.Load(subscriptionId); ok { + return cached.(*armresources.Client), nil + } + credential, err := rs.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -354,13 +366,19 @@ func (rs *ResourceService) createResourcesClient(ctx context.Context, subscripti return nil, fmt.Errorf("creating Resource client: %w", err) } - return client, nil + // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. + actual, _ := rs.resourcesClients.LoadOrStore(subscriptionId, client) + return actual.(*armresources.Client), nil } func (rs *ResourceService) createResourceGroupClient( ctx context.Context, subscriptionId string, ) (*armresources.ResourceGroupsClient, error) { + if cached, ok := rs.resourceGroupClients.Load(subscriptionId); ok { + return cached.(*armresources.ResourceGroupsClient), nil + } + credential, err := rs.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -371,7 +389,9 @@ func (rs *ResourceService) createResourceGroupClient( return nil, fmt.Errorf("creating ResourceGroup client: %w", err) } - return client, nil + // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. + actual, _ := rs.resourceGroupClients.LoadOrStore(subscriptionId, client) + return actual.(*armresources.ResourceGroupsClient), nil } // GroupByResourceGroup creates a map of resources group by their resource group name. diff --git a/cli/azd/pkg/azapi/stack_deployments.go b/cli/azd/pkg/azapi/stack_deployments.go index 7a7b0ba71ec..43df69828b5 100644 --- a/cli/azd/pkg/azapi/stack_deployments.go +++ b/cli/azd/pkg/azapi/stack_deployments.go @@ -13,13 +13,16 @@ import ( "os" "path/filepath" "strconv" + "sync" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armdeploymentstacks" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" "github.com/azure/azure-dev/cli/azd/internal" + "github.com/azure/azure-dev/cli/azd/internal/tracing" "github.com/azure/azure-dev/cli/azd/pkg/account" "github.com/azure/azure-dev/cli/azd/pkg/alpha" "github.com/azure/azure-dev/cli/azd/pkg/async" @@ -30,6 +33,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/benbjohnson/clock" "github.com/sethvargo/go-retry" + "go.opentelemetry.io/otel/attribute" ) var FeatureDeploymentStacks = alpha.MustFeatureKey("deployment.stacks") @@ -57,6 +61,10 @@ type StackDeployments struct { armClientOptions *arm.ClientOptions standardDeployments *StandardDeployments cloud *cloud.Cloud + + // stackClients caches deployment stacks Client instances per subscription ID. + // Azure SDK ARM clients are safe for concurrent use. + stackClients sync.Map // map[string]*armdeploymentstacks.Client } type deploymentStackOptions struct { @@ -250,7 +258,14 @@ func (d *StackDeployments) DeployToSubscription( parameters azure.ArmParameters, tags map[string]*string, options map[string]any, -) (*ResourceDeployment, error) { +) (_ *ResourceDeployment, err error) { + ctx, span := tracing.Start(ctx, "arm.stack.deploy.subscription") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.String("arm.subscription", subscriptionId), + attribute.String("arm.deployment", deploymentName), + ) + client, err := d.createClient(ctx, subscriptionId) if err != nil { return nil, err @@ -266,7 +281,9 @@ func (d *StackDeployments) DeployToSubscription( return nil, err } - _, err = poller.PollUntilDone(ctx, nil) + _, err = poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: deployPollFrequency, + }) if err != nil { return nil, fmt.Errorf("deploying to subscription: %w", createDeploymentError(err, DeploymentOperationDeploy)) } @@ -324,7 +341,15 @@ func (d *StackDeployments) DeployToResourceGroup( parameters azure.ArmParameters, tags map[string]*string, options map[string]any, -) (*ResourceDeployment, error) { +) (_ *ResourceDeployment, err error) { + ctx, span := tracing.Start(ctx, "arm.stack.deploy.resourcegroup") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.String("arm.subscription", subscriptionId), + attribute.String("arm.resourcegroup", resourceGroup), + attribute.String("arm.deployment", deploymentName), + ) + client, err := d.createClient(ctx, subscriptionId) if err != nil { return nil, err @@ -340,7 +365,9 @@ func (d *StackDeployments) DeployToResourceGroup( return nil, err } - _, err = poller.PollUntilDone(ctx, nil) + _, err = poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: deployPollFrequency, + }) if err != nil { return nil, fmt.Errorf("deploying to resource group: %w", createDeploymentError(err, DeploymentOperationDeploy)) } @@ -489,7 +516,9 @@ func (d *StackDeployments) DeleteSubscriptionDeployment( return err } - _, err = poller.PollUntilDone(ctx, nil) + _, err = poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: deployPollFrequency, + }) if err != nil { progress.SetProgress(DeleteDeploymentProgress{ Name: deploymentName, @@ -629,7 +658,9 @@ func (d *StackDeployments) DeleteResourceGroupDeployment( return err } - _, err = poller.PollUntilDone(ctx, nil) + _, err = poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: deployPollFrequency, + }) if err != nil { progress.SetProgress(DeleteDeploymentProgress{ Name: deploymentName, @@ -661,12 +692,23 @@ func (d *StackDeployments) CalculateTemplateHash( } func (d *StackDeployments) createClient(ctx context.Context, subscriptionId string) (*armdeploymentstacks.Client, error) { + if cached, ok := d.stackClients.Load(subscriptionId); ok { + return cached.(*armdeploymentstacks.Client), nil + } + credential, err := d.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err } - return armdeploymentstacks.NewClient(subscriptionId, credential, d.armClientOptions) + client, err := armdeploymentstacks.NewClient(subscriptionId, credential, d.armClientOptions) + if err != nil { + return nil, fmt.Errorf("creating deployment stacks client: %w", err) + } + + // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. + actual, _ := d.stackClients.LoadOrStore(subscriptionId, client) + return actual.(*armdeploymentstacks.Client), nil } // Converts from an ARM Extended Deployment to Azd Generic deployment @@ -795,7 +837,9 @@ func (d *StackDeployments) ValidatePreflightToResourceGroup( createDeploymentError(err, DeploymentOperationValidate), ) } - _, err = validateResult.PollUntilDone(ctx, nil) + _, err = validateResult.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: slowPollFrequency, + }) if err != nil { return fmt.Errorf( "validating deployment to resource group: %w", @@ -877,7 +921,9 @@ func (d *StackDeployments) ValidatePreflightToSubscription( createDeploymentError(err, DeploymentOperationValidate), ) } - _, err = validateResult.PollUntilDone(ctx, nil) + _, err = validateResult.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: slowPollFrequency, + }) if err != nil { return fmt.Errorf( "validating deployment to subscription: %w", diff --git a/cli/azd/pkg/azapi/standard_deployments.go b/cli/azd/pkg/azapi/standard_deployments.go index efc55ed44cb..03dfb0e008b 100644 --- a/cli/azd/pkg/azapi/standard_deployments.go +++ b/cli/azd/pkg/azapi/standard_deployments.go @@ -11,11 +11,15 @@ import ( "maps" "net/url" "slices" + "sync" + "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" + "github.com/azure/azure-dev/cli/azd/internal/tracing" "github.com/azure/azure-dev/cli/azd/pkg/account" "github.com/azure/azure-dev/cli/azd/pkg/async" "github.com/azure/azure-dev/cli/azd/pkg/azure" @@ -23,6 +27,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/convert" "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/benbjohnson/clock" + "go.opentelemetry.io/otel/attribute" ) // cArmDeploymentNameLengthMax is the maximum length of the name of a deployment in ARM. @@ -30,6 +35,17 @@ const ( cArmDeploymentNameLengthMax = 64 cPortalUrlFragment = "#view/HubsExtension/DeploymentDetailsBlade/~/overview/id" cOutputsUrlFragment = "#view/HubsExtension/DeploymentDetailsBlade/~/outputs/id" + + // deployPollFrequency is the polling interval for ARM deploy/delete operations. + // Deployments complete in variable time, so polling faster than the 30s SDK default + // reduces tail latency. 5s balances responsiveness against the ARM read-rate limit + // (1200 reads/5min/subscription) when many LROs run in parallel. + deployPollFrequency = 5 * time.Second + + // slowPollFrequency is the polling interval for ARM WhatIf and Validate operations. + // These are consistently slow (30-90s), so aggressive polling provides no benefit and + // risks hitting the ARM read rate limit (1200 reads/5min) during large parallel deployments. + slowPollFrequency = 15 * time.Second ) type StandardDeployments struct { @@ -38,6 +54,14 @@ type StandardDeployments struct { resourceService *ResourceService cloud *cloud.Cloud clock clock.Clock + + // deploymentsClients caches DeploymentsClient instances per subscription ID. + // Azure SDK ARM clients are safe for concurrent use and hold no per-request state, + // so reusing them avoids redundant pipeline construction on every API call. + deploymentsClients sync.Map // map[string]*armresources.DeploymentsClient + + // deploymentsOpsClients caches DeploymentOperationsClient instances per subscription ID. + deploymentsOpsClients sync.Map // map[string]*armresources.DeploymentOperationsClient } func NewStandardDeployments( @@ -186,6 +210,10 @@ func (ds *StandardDeployments) createDeploymentsClient( ctx context.Context, subscriptionId string, ) (*armresources.DeploymentsClient, error) { + if cached, ok := ds.deploymentsClients.Load(subscriptionId); ok { + return cached.(*armresources.DeploymentsClient), nil + } + credential, err := ds.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -196,7 +224,9 @@ func (ds *StandardDeployments) createDeploymentsClient( return nil, fmt.Errorf("creating deployments client: %w", err) } - return client, nil + // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. + actual, _ := ds.deploymentsClients.LoadOrStore(subscriptionId, client) + return actual.(*armresources.DeploymentsClient), nil } func (ds *StandardDeployments) DeployToSubscription( @@ -208,7 +238,14 @@ func (ds *StandardDeployments) DeployToSubscription( parameters azure.ArmParameters, tags map[string]*string, options map[string]any, -) (*ResourceDeployment, error) { +) (_ *ResourceDeployment, err error) { + ctx, span := tracing.Start(ctx, "arm.deploy.subscription") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.String("arm.subscription", subscriptionId), + attribute.String("arm.deployment", deploymentName), + ) + deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { return nil, fmt.Errorf("creating deployments client: %w", err) @@ -230,7 +267,9 @@ func (ds *StandardDeployments) DeployToSubscription( } // wait for deployment creation - deployResult, err := createFromTemplateOperation.PollUntilDone(ctx, nil) + deployResult, err := createFromTemplateOperation.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: deployPollFrequency, + }) if err != nil { return nil, fmt.Errorf("deploying to subscription: %w", createDeploymentError(err, DeploymentOperationDeploy)) } @@ -245,7 +284,15 @@ func (ds *StandardDeployments) DeployToResourceGroup( parameters azure.ArmParameters, tags map[string]*string, options map[string]any, -) (*ResourceDeployment, error) { +) (_ *ResourceDeployment, err error) { + ctx, span := tracing.Start(ctx, "arm.deploy.resourcegroup") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.String("arm.subscription", subscriptionId), + attribute.String("arm.resourcegroup", resourceGroup), + attribute.String("arm.deployment", deploymentName), + ) + deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { return nil, fmt.Errorf("creating deployments client: %w", err) @@ -266,7 +313,9 @@ func (ds *StandardDeployments) DeployToResourceGroup( } // wait for deployment creation - deployResult, err := createFromTemplateOperation.PollUntilDone(ctx, nil) + deployResult, err := createFromTemplateOperation.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: deployPollFrequency, + }) if err != nil { return nil, fmt.Errorf("deploying to resource group: %w", createDeploymentError(err, DeploymentOperationDeploy)) } @@ -558,7 +607,14 @@ func (ds *StandardDeployments) WhatIfDeployToSubscription( deploymentName string, armTemplate azure.RawArmTemplate, parameters azure.ArmParameters, -) (*armresources.WhatIfOperationResult, error) { +) (_ *armresources.WhatIfOperationResult, err error) { + ctx, span := tracing.Start(ctx, "arm.whatif.subscription") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.String("arm.subscription", subscriptionId), + attribute.String("arm.deployment", deploymentName), + ) + deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { return nil, fmt.Errorf("creating deployments client: %w", err) @@ -580,7 +636,9 @@ func (ds *StandardDeployments) WhatIfDeployToSubscription( } // wait for deployment creation - deployResult, err := createFromTemplateOperation.PollUntilDone(ctx, nil) + deployResult, err := createFromTemplateOperation.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: slowPollFrequency, + }) if err != nil { return nil, fmt.Errorf("deploying to subscription: %w", createDeploymentError(err, DeploymentOperationPreview)) } @@ -593,7 +651,15 @@ func (ds *StandardDeployments) WhatIfDeployToResourceGroup( subscriptionId, resourceGroup, deploymentName string, armTemplate azure.RawArmTemplate, parameters azure.ArmParameters, -) (*armresources.WhatIfOperationResult, error) { +) (_ *armresources.WhatIfOperationResult, err error) { + ctx, span := tracing.Start(ctx, "arm.whatif.resourcegroup") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.String("arm.subscription", subscriptionId), + attribute.String("arm.resourcegroup", resourceGroup), + attribute.String("arm.deployment", deploymentName), + ) + deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { return nil, fmt.Errorf("creating deployments client: %w", err) @@ -613,7 +679,9 @@ func (ds *StandardDeployments) WhatIfDeployToResourceGroup( } // wait for deployment creation - deployResult, err := createFromTemplateOperation.PollUntilDone(ctx, nil) + deployResult, err := createFromTemplateOperation.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: slowPollFrequency, + }) if err != nil { return nil, fmt.Errorf("deploying to resource group: %w", createDeploymentError(err, DeploymentOperationPreview)) } @@ -625,6 +693,10 @@ func (ds *StandardDeployments) createDeploymentsOperationsClient( ctx context.Context, subscriptionId string, ) (*armresources.DeploymentOperationsClient, error) { + if cached, ok := ds.deploymentsOpsClients.Load(subscriptionId); ok { + return cached.(*armresources.DeploymentOperationsClient), nil + } + credential, err := ds.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -635,7 +707,9 @@ func (ds *StandardDeployments) createDeploymentsOperationsClient( return nil, fmt.Errorf("creating deployments client: %w", err) } - return client, nil + // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. + actual, _ := ds.deploymentsOpsClients.LoadOrStore(subscriptionId, client) + return actual.(*armresources.DeploymentOperationsClient), nil } // Converts from an ARM Extended Deployment to Azd Generic deployment @@ -714,7 +788,14 @@ func (ds *StandardDeployments) ValidatePreflightToSubscription( parameters azure.ArmParameters, tags map[string]*string, options map[string]any, -) error { +) (err error) { + ctx, span := tracing.Start(ctx, "arm.validate.subscription") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.String("arm.subscription", subscriptionId), + attribute.String("arm.deployment", deploymentName), + ) + deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { return fmt.Errorf("creating deployments client: %w", err) @@ -737,7 +818,9 @@ func (ds *StandardDeployments) ValidatePreflightToSubscription( createDeploymentError(err, DeploymentOperationValidate), ) } - _, err = validateResult.PollUntilDone(ctx, nil) + _, err = validateResult.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: slowPollFrequency, + }) if err != nil { return fmt.Errorf( "validating deployment to subscription: %w", @@ -757,7 +840,15 @@ func (ds *StandardDeployments) ValidatePreflightToResourceGroup( parameters azure.ArmParameters, tags map[string]*string, options map[string]any, -) error { +) (err error) { + ctx, span := tracing.Start(ctx, "arm.validate.resourcegroup") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.String("arm.subscription", subscriptionId), + attribute.String("arm.resourcegroup", resourceGroup), + attribute.String("arm.deployment", deploymentName), + ) + deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { return fmt.Errorf("creating deployments client: %w", err) @@ -778,7 +869,9 @@ func (ds *StandardDeployments) ValidatePreflightToResourceGroup( createDeploymentError(err, DeploymentOperationValidate), ) } - _, err = validateResult.PollUntilDone(ctx, nil) + _, err = validateResult.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: slowPollFrequency, + }) if err != nil { return fmt.Errorf( "validating deployment to resource group: %w", diff --git a/cli/azd/pkg/azapi/webapp.go b/cli/azd/pkg/azapi/webapp.go index 5aa72c7126b..0975f836aa0 100644 --- a/cli/azd/pkg/azapi/webapp.go +++ b/cli/azd/pkg/azapi/webapp.go @@ -8,12 +8,16 @@ import ( "errors" "fmt" "io" + "log" "net/http" "strings" + "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appservice/armappservice/v2" + "github.com/azure/azure-dev/cli/azd/internal/tracing" "github.com/azure/azure-dev/cli/azd/pkg/azsdk" + "go.opentelemetry.io/otel/attribute" ) type AzCliAppServiceProperties struct { @@ -153,7 +157,15 @@ func (cli *AzureClient) DeployAppServiceZip( appName string, deployZipFile io.ReadSeeker, progressLog func(string), -) (*string, error) { +) (_ *string, err error) { + ctx, span := tracing.Start(ctx, "deploy.appservice.zip") + defer func() { span.EndWithStatus(err) }() + + span.SetAttributes( + attribute.String("deploy.appservice.app", appName), + attribute.String("deploy.appservice.rg", resourceGroup), + ) + app, err := cli.appService(ctx, subscriptionId, resourceGroup, appName) if err != nil { return nil, err @@ -169,20 +181,63 @@ func (cli *AzureClient) DeployAppServiceZip( return nil, err } + isLinux := isLinuxWebApp(app) + span.SetAttributes(attribute.Bool("deploy.appservice.linux", isLinux)) + // Deployment Status API only support linux web app for now - if isLinuxWebApp(app) { - if err := client.DeployTrackStatus( - ctx, deployZipFile, subscriptionId, resourceGroup, appName, progressLog); err != nil { - if !resumeDeployment(err, progressLog) { - return nil, err + if isLinux { + // Build failures can be caused by an SCM container restart triggered by ARM + // applying site config (app settings) shortly after the site is created. + // When concurrent provisioning + deployment is used (e.g. `azd up --concurrent`), + // the deploy step may start while the SCM container is still restarting. + // Retry the entire zip deploy when the build fails, giving the SCM time to stabilize. + const maxBuildRetries = 2 + for attempt := range maxBuildRetries + 1 { + span.SetAttributes(attribute.Int("deploy.appservice.attempt", attempt+1)) + + if attempt > 0 { + // Reset the zip file reader so the retry re-uploads the full content. + if _, seekErr := deployZipFile.Seek(0, io.SeekStart); seekErr != nil { + return nil, fmt.Errorf("resetting zip file for retry: %w", seekErr) + } + progressLog(fmt.Sprintf( + "Retrying deployment (attempt %d/%d) — the previous build may have been "+ + "interrupted by an SCM container restart", attempt+1, maxBuildRetries+1)) + + // Wait for the SCM site to become ready before retrying. + if waitErr := waitForScmReady(ctx, client, 5*time.Second, progressLog); waitErr != nil { + // Propagate context cancellation (Ctrl+C) or deadline exceeded + // immediately — these indicate the user or system requested abort. + if errors.Is(waitErr, context.Canceled) || errors.Is(waitErr, context.DeadlineExceeded) { + return nil, waitErr + } + log.Printf("SCM readiness check failed (non-fatal): %v", waitErr) + } } - } else { - // Deployment is successful - statusText := "OK" - return new(statusText), nil + + err = client.DeployTrackStatus( + ctx, deployZipFile, subscriptionId, resourceGroup, appName, progressLog) + if err != nil { + if isBuildFailure(err) && attempt < maxBuildRetries { + progressLog("Build process failed — will retry after SCM stabilizes") + continue + } + if !resumeDeployment(err, progressLog) { + return nil, err + } + } else { + // Deployment is successful + return new("OK"), nil + } + break } } + // Rewind the zip file before the fallback deploy — the tracked deploy consumed it. + if _, seekErr := deployZipFile.Seek(0, io.SeekStart); seekErr != nil { + return nil, fmt.Errorf("resetting zip file for fallback deploy: %w", seekErr) + } + response, err := client.Deploy(ctx, deployZipFile) if err != nil { return nil, err @@ -191,6 +246,83 @@ func (cli *AzureClient) DeployAppServiceZip( return &response.StatusText, nil } +// isBuildFailure returns true when the deployment error indicates a transient +// Oryx/Kudu build failure caused by an SCM container restart — not a genuine +// application build error. We detect the transient case by matching the exact +// prefix "the build process failed" (which comes from the Kudu deployment API) +// while excluding messages from the deployment status API that start with +// "Deployment failed because the build process failed" (genuine build errors). +// +// NOTE: This heuristic is tied to exact Azure/Kudu error message wording. +// If the upstream messages change, detection breaks silently and retries stop +// occurring (falling back to the non-retry deploy path). The Azure SDK does +// not surface structured error codes for these build failures. +func isBuildFailure(err error) bool { + if err == nil { + return false + } + msg := err.Error() + // Genuine build failures from logWebAppDeploymentStatus start with + // "Deployment failed because the build process failed". Transient SCM + // failures use the shorter "the build process failed" without that prefix. + return strings.Contains(msg, "the build process failed") && + !strings.Contains(msg, "Deployment failed because") && + !strings.Contains(msg, "logs for more info") +} + +// scmReadyChecker abstracts the IsScmReady probe so waitForScmReady can be +// unit-tested with a mock. *azsdk.ZipDeployClient satisfies this interface. +type scmReadyChecker interface { + IsScmReady(ctx context.Context) (bool, error) +} + +// waitForScmReady pings the SCM /api/deployments endpoint until it responds +// with 200, indicating the SCM container has finished restarting. This avoids +// pushing a new zip deploy into a container that is about to restart. +// pollInterval controls the polling frequency; callers pass the production value +// (typically 5s) while tests can use shorter intervals to avoid wall-time delays. +func waitForScmReady( + ctx context.Context, client scmReadyChecker, pollInterval time.Duration, progressLog func(string), +) error { + const scmReadyTimeout = 90 * time.Second + + ctx, cancel := context.WithTimeout(ctx, scmReadyTimeout) + defer cancel() + + progressLog("Waiting for SCM site to become ready...") + + // Probe once immediately to avoid a needless pollInterval delay when SCM is already ready. + ready, err := client.IsScmReady(ctx) + if err == nil && ready { + progressLog("SCM site is ready") + return nil + } + + ticker := time.NewTicker(pollInterval) + defer ticker.Stop() + + for { + select { + case <-ctx.Done(): + return fmt.Errorf("SCM site did not become ready within %v: %w", scmReadyTimeout, ctx.Err()) + case <-ticker.C: + } + + ready, err := client.IsScmReady(ctx) + if err != nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return err + } + log.Printf("SCM readiness probe error: %v", err) + continue + } + if ready { + progressLog("SCM site is ready") + return nil + } + } +} + func (cli *AzureClient) createWebAppsClient( ctx context.Context, subscriptionId string, @@ -265,6 +397,52 @@ func (cli *AzureClient) GetAppServiceSlots( return slots, nil } +// UpdateAppServiceAppSetting reads the existing application settings, adds or overwrites +// the specified key-value pair, and writes all settings back. This preserves other settings +// that are not being modified. +// +// Note: This function is not safe for concurrent use. The Azure App Service settings API +// replaces the full set on write and does not support ETags, so concurrent updates may +// result in lost writes. Callers should serialize updates to the same app's settings. +func (cli *AzureClient) UpdateAppServiceAppSetting( + ctx context.Context, + subscriptionId string, + resourceGroup string, + appName string, + key string, + value string, +) error { + client, err := cli.createWebAppsClient(ctx, subscriptionId) + if err != nil { + return err + } + + // Read existing settings so that we preserve them. + existing, err := client.ListApplicationSettings(ctx, resourceGroup, appName, nil) + if err != nil { + return fmt.Errorf("listing existing app settings: %w", err) + } + + if existing.Properties == nil { + existing.Properties = map[string]*string{} + } + + existing.Properties[key] = &value + + // Write all settings back (the API replaces the full set). + if _, err := client.UpdateApplicationSettings( + ctx, + resourceGroup, + appName, + armappservice.StringDictionary{Properties: existing.Properties}, + nil, + ); err != nil { + return fmt.Errorf("updating app settings: %w", err) + } + + return nil +} + // DeployAppServiceSlotZip deploys a zip file to a specific deployment slot. func (cli *AzureClient) DeployAppServiceSlotZip( ctx context.Context, diff --git a/cli/azd/pkg/azapi/webapp_test.go b/cli/azd/pkg/azapi/webapp_test.go new file mode 100644 index 00000000000..ec0990510c3 --- /dev/null +++ b/cli/azd/pkg/azapi/webapp_test.go @@ -0,0 +1,124 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package azapi + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func Test_isBuildFailure(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + expect bool + }{ + {"nil error", nil, false}, + {"unrelated error", errors.New("connection refused"), false}, + {"transient build failure", errors.New("the build process failed"), true}, + {"transient build failure wrapped", errors.New("deploy error: the build process failed with exit code 1"), true}, + {"real build failure with logs", errors.New("the build process failed, check logs for more info"), false}, + {"genuine build failure from status API", + errors.New("Deployment failed because the build process failed\n"), false}, + {"partial match", errors.New("build process"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.expect, isBuildFailure(tt.err)) + }) + } +} + +// mockScmChecker is a mock implementation of scmReadyChecker for testing. +type mockScmChecker struct { + calls atomic.Int32 + fn func(ctx context.Context, call int) (bool, error) +} + +func (m *mockScmChecker) IsScmReady(ctx context.Context) (bool, error) { + call := int(m.calls.Add(1)) + return m.fn(ctx, call) +} + +func Test_waitForScmReady_ImmediateSuccess(t *testing.T) { + t.Parallel() + ctx := t.Context() + + mock := &mockScmChecker{fn: func(_ context.Context, _ int) (bool, error) { + return true, nil + }} + + var logs []string + err := waitForScmReady(ctx, mock, time.Millisecond, func(msg string) { logs = append(logs, msg) }) + + require.NoError(t, err) + require.Equal(t, int32(1), mock.calls.Load()) + require.Contains(t, logs, "SCM site is ready") +} + +func Test_waitForScmReady_ContextCanceled(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + + // Mock returns not-ready on first probe, then context.Canceled on the second call + // to exercise the error propagation path inside the ticker loop (lines 306-311). + mock := &mockScmChecker{fn: func(_ context.Context, call int) (bool, error) { + if call == 1 { + return false, nil // immediate probe: not ready + } + cancel() + return false, context.Canceled + }} + + err := waitForScmReady(ctx, mock, time.Millisecond, func(string) {}) + + require.Error(t, err) + require.ErrorIs(t, err, context.Canceled) + require.GreaterOrEqual(t, int(mock.calls.Load()), 2, "IsScmReady should be called at least twice") +} + +func Test_waitForScmReady_TransientErrorsThenSuccess(t *testing.T) { + t.Parallel() + ctx := t.Context() + + mock := &mockScmChecker{fn: func(_ context.Context, call int) (bool, error) { + if call < 3 { + return false, errors.New("transient error") + } + return true, nil + }} + + err := waitForScmReady(ctx, mock, time.Millisecond, func(string) {}) + + require.NoError(t, err) + require.GreaterOrEqual(t, int(mock.calls.Load()), 3) +} + +func Test_waitForScmReady_ContextDeadlineExceeded(t *testing.T) { + t.Parallel() + + // Mock returns not-ready on first probe, then DeadlineExceeded on subsequent calls + // to exercise the error propagation path inside the ticker loop. + mock := &mockScmChecker{fn: func(_ context.Context, call int) (bool, error) { + if call == 1 { + return false, nil // immediate probe: not ready + } + return false, context.DeadlineExceeded + }} + + err := waitForScmReady(t.Context(), mock, time.Millisecond, func(string) {}) + + require.Error(t, err) + require.ErrorIs(t, err, context.DeadlineExceeded) + require.GreaterOrEqual(t, int(mock.calls.Load()), 2, "IsScmReady should be called at least twice") +} diff --git a/cli/azd/pkg/azsdk/zip_deploy_client.go b/cli/azd/pkg/azsdk/zip_deploy_client.go index cdc774c84de..0a4c48104e8 100644 --- a/cli/azd/pkg/azsdk/zip_deploy_client.go +++ b/cli/azd/pkg/azsdk/zip_deploy_client.go @@ -432,3 +432,36 @@ func (h *deployPollingHandler) Result(ctx context.Context, out **DeployResponse) return nil } + +// IsScmReady pings the SCM /api/deployments endpoint to check if the Kudu +// service is responsive. Returns true when the endpoint responds with HTTP 200, +// false for transient errors (503, connection refused, etc.), or an error for +// unexpected failures. This is used to detect SCM container restarts that occur +// when ARM applies site configuration changes after provisioning. +func (c *ZipDeployClient) IsScmReady(ctx context.Context) (bool, error) { + endpoint := fmt.Sprintf("https://%s/api/deployments", c.hostName) + req, err := runtime.NewRequest(ctx, http.MethodGet, endpoint) + if err != nil { + return false, fmt.Errorf("creating SCM readiness request: %w", err) + } + + resp, err := c.pipeline.Do(req) + if err != nil { + // Propagate context cancellation / deadline so callers (and Ctrl+C) react immediately. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || ctx.Err() != nil { + return false, err + } + // Connection and other transport errors mean SCM is still restarting. + return false, nil //nolint:nilerr + } + defer resp.Body.Close() + + switch resp.StatusCode { + case http.StatusOK: + return true, nil + case http.StatusBadGateway, http.StatusServiceUnavailable: + return false, nil + default: + return false, runtime.NewResponseError(resp) + } +} diff --git a/cli/azd/pkg/containerapps/container_app.go b/cli/azd/pkg/containerapps/container_app.go index d661b05b01b..d3bf6db6d73 100644 --- a/cli/azd/pkg/containerapps/container_app.go +++ b/cli/azd/pkg/containerapps/container_app.go @@ -13,6 +13,7 @@ import ( "net/http" "slices" "strings" + "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -33,6 +34,12 @@ import ( ) const ( + // containerAppPollFrequency is the polling interval for Container App create/update operations. + // Default SDK polling is 30s; Container App updates typically complete in 10-60s, so 5s polling + // reduces tail latency by up to 25s per operation while leaving headroom against the ARM + // read-rate limit (1200 reads/5min/subscription) during large parallel deployments. + containerAppPollFrequency = 5 * time.Second + pathLatestRevisionName = "properties.latestRevisionName" pathTemplate = "properties.template" pathTemplateRevisionSuffix = "properties.template.revisionSuffix" @@ -184,8 +191,7 @@ func (cas *containerAppService) persistSettings( aca, err := cas.getContainerApp(ctx, subscriptionId, resourceGroupName, appName, options) if err != nil { // On first deploy the app doesn't exist yet (404) — proceed without persisting. - var respErr *azcore.ResponseError - if errors.As(err, &respErr) && respErr.StatusCode == http.StatusNotFound { + if respErr, ok := errors.AsType[*azcore.ResponseError](err); ok && respErr.StatusCode == http.StatusNotFound { return obj, nil } @@ -312,7 +318,9 @@ func (cas *containerAppService) DeployYaml( poller = p } - _, err = poller.PollUntilDone(ctx, nil) + _, err = poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: containerAppPollFrequency, + }) if err != nil { return fmt.Errorf("polling for container app update completion: %w", err) } @@ -535,7 +543,9 @@ func (cas *containerAppService) updateContainerApp( return fmt.Errorf("begin updating ingress traffic: %w", err) } - _, err = poller.PollUntilDone(ctx, nil) + _, err = poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: containerAppPollFrequency, + }) if err != nil { return fmt.Errorf("polling for container app update completion: %w", err) } @@ -767,7 +777,9 @@ func (cas *containerAppService) UpdateContainerAppJobImage( ) } - _, err = poller.PollUntilDone(ctx, nil) + _, err = poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{ + Frequency: containerAppPollFrequency, + }) if err != nil { return fmt.Errorf( "waiting for container app job update: %w", err, diff --git a/cli/azd/pkg/httputil/util.go b/cli/azd/pkg/httputil/util.go index 4dcebe17e10..78a2c973794 100644 --- a/cli/azd/pkg/httputil/util.go +++ b/cli/azd/pkg/httputil/util.go @@ -15,9 +15,12 @@ import ( "time" ) -// Reads the raw HTTP response and attempt to convert it into the specified type +// Reads the raw HTTP response and attempt to convert it into the specified type. // Typically used in conjunction with runtime.WithCaptureResponse(...) to get access to the underlying HTTP response of the // SDK API call. +// +// ReadRawResponse fully drains the response body via io.ReadAll but does not close it. +// Callers that return the response to the azcore poller framework must leave the body unclosed. func ReadRawResponse[T any](response *http.Response) (*T, error) { data, err := io.ReadAll(response.Body) if err != nil { @@ -34,6 +37,21 @@ func ReadRawResponse[T any](response *http.Response) (*T, error) { return instance, nil } +// TunedTransport returns an http.Transport cloned from http.DefaultTransport with +// connection pooling parameters optimized for Azure CLI workloads. The key tunings +// are MaxConnsPerHost and MaxIdleConnsPerHost (raised from Go defaults) to avoid +// unnecessary TLS handshakes when making many concurrent requests to ARM endpoints. +// With parallel execution, 8+ services may hit ARM simultaneously. +func TunedTransport() *http.Transport { + transport := http.DefaultTransport.(*http.Transport).Clone() + transport.MaxIdleConns = 200 + transport.MaxConnsPerHost = 50 + transport.MaxIdleConnsPerHost = 50 + transport.IdleConnTimeout = 90 * time.Second + transport.DisableKeepAlives = false // keep-alive must remain enabled for pooling + return transport +} + // TlsEnabledTransport returns a http.Transport that has TLS configured to use the provided // Base64 DER-encoded certificate. The returned http.Transport inherits defaults from http.DefaultTransport. func TlsEnabledTransport(derBytes string) (*http.Transport, error) { diff --git a/cli/azd/pkg/httputil/util_test.go b/cli/azd/pkg/httputil/util_test.go index 8b42b4d8ed8..c49e67a681a 100644 --- a/cli/azd/pkg/httputil/util_test.go +++ b/cli/azd/pkg/httputil/util_test.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -19,6 +20,35 @@ type exampleResponse struct { C string `json:"c"` } +func TestTunedTransport(t *testing.T) { + transport := TunedTransport() + + require.NotNil(t, transport) + require.Equal(t, 200, transport.MaxIdleConns) + require.Equal(t, 50, transport.MaxConnsPerHost) + require.Equal(t, 50, transport.MaxIdleConnsPerHost) + require.Equal(t, 90*time.Second, transport.IdleConnTimeout) + require.False(t, transport.DisableKeepAlives) +} + +func TestTunedTransport_DoesNotMutateDefault(t *testing.T) { + defaultTransport := http.DefaultTransport.(*http.Transport) + origMaxIdle := defaultTransport.MaxIdleConns + origMaxConns := defaultTransport.MaxConnsPerHost + origMaxIdlePerHost := defaultTransport.MaxIdleConnsPerHost + origIdleTimeout := defaultTransport.IdleConnTimeout + origKeepAlives := defaultTransport.DisableKeepAlives + + _ = TunedTransport() + + // Verify no field of the default transport was mutated. + require.Equal(t, origMaxIdle, defaultTransport.MaxIdleConns) + require.Equal(t, origMaxConns, defaultTransport.MaxConnsPerHost) + require.Equal(t, origMaxIdlePerHost, defaultTransport.MaxIdleConnsPerHost) + require.Equal(t, origIdleTimeout, defaultTransport.IdleConnTimeout) + require.Equal(t, origKeepAlives, defaultTransport.DisableKeepAlives) +} + func TestReadRawResponse(t *testing.T) { t.Run("Success", func(t *testing.T) { expectedResponse := &exampleResponse{ diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index 74c5d15e0ba..c1f1734ef1d 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -8,6 +8,8 @@ import ( "errors" "fmt" "log" + "net" + "net/url" "os" "path" "path/filepath" @@ -39,6 +41,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/tools/pack" "github.com/benbjohnson/clock" "github.com/sethvargo/go-retry" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" ) @@ -281,14 +284,17 @@ func (ch *ContainerHelper) Login( return registryName, nil } -var defaultCredentialsRetryDelay = 20 * time.Second +var defaultCredentialsRetryDelay = 2 * time.Second func (ch *ContainerHelper) Credentials( ctx context.Context, serviceConfig *ServiceConfig, targetResource *environment.TargetResource, env *environment.Environment, -) (*azapi.DockerCredentials, error) { +) (_ *azapi.DockerCredentials, err error) { + ctx, span := tracing.Start(ctx, "container.credentials") + defer func() { span.EndWithStatus(err) }() + loginServer, err := ch.RegistryName(ctx, serviceConfig, env) if err != nil { return nil, err @@ -297,15 +303,32 @@ func (ch *ContainerHelper) Credentials( var credential *azapi.DockerCredentials credentialsError := retry.Do( ctx, - // will retry just once after 1 minute based on: - // https://learn.microsoft.com/en-us/azure/dns/dns-faq#how-long-does-it-take-for-dns-changes-to-take-effect- - retry.WithMaxRetries(3, retry.NewConstant(defaultCredentialsRetryDelay)), + // Use exponential backoff (2s → 4s → 8s → 16s → 32s = 62s worst case) instead of + // constant 20s delay. ACR credentials are typically available within seconds after + // resource creation, so aggressive early retries resolve most 404s quickly while + // preserving roughly the same total retry window for slow DNS propagation. + retry.WithMaxRetries(5, retry.NewExponential(defaultCredentialsRetryDelay)), func(ctx context.Context) error { cred, err := ch.containerRegistryService.Credentials(ctx, targetResource.SubscriptionId(), loginServer) if err != nil { if httpErr, ok := errors.AsType[*azcore.ResponseError](err); ok { - if httpErr.StatusCode == 404 { - // Retry if the registry is not found while logging in + if httpErr.StatusCode == 404 || + httpErr.StatusCode == 429 || + httpErr.StatusCode >= 500 { + return retry.RetryableError(err) + } + } + // Retry transient network/DNS errors (e.g. "no such host" + // during DNS propagation after ACR creation). Only retry + // genuinely transient errors, not permanent URL/TLS failures. + if _, ok := errors.AsType[*url.Error](err); ok { + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return retry.RetryableError(err) + } + msg := err.Error() + if strings.Contains(msg, "no such host") || + strings.Contains(msg, "connection refused") { return retry.RetryableError(err) } } @@ -330,6 +353,7 @@ func (ch *ContainerHelper) Build( } dockerOptions := getDockerOptionsWithDefaults(serviceConfig.Docker) + resolveDockerPaths(serviceConfig, &dockerOptions) resolveParameters := func(source []string) ([]string, error) { result := make([]string, len(source)) @@ -401,10 +425,8 @@ func (ch *ContainerHelper) Build( strings.ToLower(serviceConfig.Name), ) + // dockerOptions.Path is already resolved to absolute by resolveDockerPaths dockerfilePath := dockerOptions.Path - if !filepath.IsAbs(dockerfilePath) { - dockerfilePath = filepath.Join(serviceConfig.Path(), dockerfilePath) - } _, err = os.Stat(dockerfilePath) if errors.Is(err, os.ErrNotExist) && serviceConfig.Docker.Path == "" { @@ -452,7 +474,7 @@ func (ch *ContainerHelper) Build( return nil, fmt.Errorf("writing dockerfile for service %s: %w", serviceConfig.Name, err) } dockerFilePath = dockerfilePath - if dockerOptions.Context == "" || dockerOptions.Context == "." { + if serviceConfig.Docker.Context == "" || serviceConfig.Docker.Context == "." { dockerOptions.Context = tempDir } @@ -588,9 +610,14 @@ func (ch *ContainerHelper) Publish( env *environment.Environment, progress *async.Progress[ServiceProgress], options *PublishOptions, -) (*ServicePublishResult, error) { +) (_ *ServicePublishResult, err error) { + ctx, span := tracing.Start(ctx, "container.publish") + defer func() { span.EndWithStatus(err) }() + span.SetAttributes( + attribute.Bool("container.remotebuild", serviceConfig.Docker.RemoteBuild), + ) + var remoteImage string - var err error // Parse PublishOptions into ImageOverride imageOverride, err := parseImageOverride(options) @@ -773,16 +800,12 @@ func (ch *ContainerHelper) runRemoteBuild( env *environment.Environment, progress *async.Progress[ServiceProgress], imageOverride *imageOverride, -) (string, error) { - dockerOptions := getDockerOptionsWithDefaults(serviceConfig.Docker) - - if !filepath.IsAbs(dockerOptions.Path) { - dockerOptions.Path = filepath.Join(serviceConfig.Path(), dockerOptions.Path) - } +) (_ string, err error) { + ctx, span := tracing.Start(ctx, "container.remotebuild") + defer func() { span.EndWithStatus(err) }() - if !filepath.IsAbs(dockerOptions.Context) { - dockerOptions.Context = filepath.Join(serviceConfig.Path(), dockerOptions.Context) - } + dockerOptions := getDockerOptionsWithDefaults(serviceConfig.Docker) + resolveDockerPaths(serviceConfig, &dockerOptions) if dockerOptions.Platform != "linux/amd64" { return "", fmt.Errorf("remote build only supports the linux/amd64 platform") @@ -1129,3 +1152,31 @@ func getDockerOptionsWithDefaults(options DockerProjectOptions) DockerProjectOpt return options } + +// resolveServiceDir returns the directory for the service. ServiceConfig.Path() +// may return a file path (e.g., .csproj for Aspire/project.v1), so we check +// whether the path is a directory and fall back to filepath.Dir if it is not. +func resolveServiceDir(serviceConfig *ServiceConfig) string { + servicePath := serviceConfig.Path() + if info, err := os.Stat(servicePath); err == nil && info.IsDir() { + return servicePath + } + return filepath.Dir(servicePath) +} + +// resolveDockerPaths resolves docker.path and docker.context to absolute paths. +// Both user-specified and default paths are resolved relative to the service +// directory to preserve backward compatibility. +func resolveDockerPaths(serviceConfig *ServiceConfig, opts *DockerProjectOptions) { + serviceDir := resolveServiceDir(serviceConfig) + if !filepath.IsAbs(opts.Path) { + opts.Path = filepath.Join(serviceDir, opts.Path) + } + if !filepath.IsAbs(opts.Context) { + opts.Context = filepath.Join(serviceDir, opts.Context) + } + // Clean resolved paths to eliminate ".." components and normalize separators, + // preventing path traversal outside the expected directory tree. + opts.Path = filepath.Clean(opts.Path) + opts.Context = filepath.Clean(opts.Context) +} diff --git a/cli/azd/pkg/project/container_helper_coverage3_test.go b/cli/azd/pkg/project/container_helper_coverage3_test.go index e74d5e8c10c..58c1e66c93e 100644 --- a/cli/azd/pkg/project/container_helper_coverage3_test.go +++ b/cli/azd/pkg/project/container_helper_coverage3_test.go @@ -4,6 +4,8 @@ package project import ( + "os" + "path/filepath" "testing" "github.com/azure/azure-dev/cli/azd/pkg/tools/docker" @@ -47,3 +49,128 @@ func Test_getDockerOptionsWithDefaults_Coverage3(t *testing.T) { assert.Equal(t, ".", result.Context) }) } + +func Test_resolveDockerPaths(t *testing.T) { + projectPath := t.TempDir() + servicePath := filepath.Join(projectPath, "src", "web") + require.NoError(t, os.MkdirAll(servicePath, 0755)) + + t.Run("DefaultPaths_RelativeToServiceDir", func(t *testing.T) { + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{}, // no user overrides + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + assert.Equal(t, filepath.Join(servicePath, "Dockerfile"), opts.Path) + assert.Equal(t, servicePath, opts.Context) + }) + + t.Run("UserSpecifiedPaths_RelativeToServiceDir", func(t *testing.T) { + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{ + Path: "docker/Dockerfile.prod", + Context: ".", + }, + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + assert.Equal(t, filepath.Join(servicePath, "docker", "Dockerfile.prod"), opts.Path) + assert.Equal(t, servicePath, opts.Context) + }) + + t.Run("AbsolutePaths_Unchanged", func(t *testing.T) { + absDockerfile := filepath.Join(projectPath, "other", "Dockerfile") + absContext := filepath.Join(projectPath, "other", "ctx") + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{ + Path: absDockerfile, + Context: absContext, + }, + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + assert.Equal(t, absDockerfile, opts.Path) + assert.Equal(t, absContext, opts.Context) + }) + + t.Run("DefaultPaths_RelativePathIsFile", func(t *testing.T) { + // When RelativePath points to a file (e.g., Aspire project.v1 .csproj), + // default docker paths should resolve relative to the parent directory. + serviceDir := filepath.Join(projectPath, "src") + require.NoError(t, os.MkdirAll(serviceDir, 0755)) + csprojFile := filepath.Join(serviceDir, "app.csproj") + require.NoError(t, os.WriteFile(csprojFile, []byte(""), 0600)) + + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "app.csproj"), + Docker: DockerProjectOptions{}, // no user overrides + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + // Defaults should resolve to the directory containing app.csproj, not app.csproj itself. + assert.Equal(t, filepath.Join(serviceDir, "Dockerfile"), opts.Path) + assert.Equal(t, serviceDir, opts.Context) + }) + + t.Run("MixedPaths_UserPathDefaultContext", func(t *testing.T) { + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{ + Path: "docker/Dockerfile.dev", + // Context not set — will default to "." + }, + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + // Both resolve relative to service dir + assert.Equal(t, filepath.Join(servicePath, "docker", "Dockerfile.dev"), opts.Path) + assert.Equal(t, servicePath, opts.Context) + }) + + t.Run("PathTraversal_DotDot_Normalized", func(t *testing.T) { + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{ + Path: "../shared/Dockerfile", + Context: "../shared", + }, + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + // ".." segments are resolved and cleaned by filepath.Clean + assert.Equal(t, filepath.Join(projectPath, "src", "shared", "Dockerfile"), opts.Path) + assert.Equal(t, filepath.Join(projectPath, "src", "shared"), opts.Context) + }) + + t.Run("PathTraversal_DoubleDotDot_Normalized", func(t *testing.T) { + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{ + Path: "../../Dockerfile", + Context: "../../", + }, + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + // Double ".." walks back to projectPath root + assert.Equal(t, filepath.Join(projectPath, "Dockerfile"), opts.Path) + assert.Equal(t, projectPath, opts.Context) + }) +} diff --git a/cli/azd/pkg/project/container_helper_test.go b/cli/azd/pkg/project/container_helper_test.go index bac6366aea3..7d484979454 100644 --- a/cli/azd/pkg/project/container_helper_test.go +++ b/cli/azd/pkg/project/container_helper_test.go @@ -683,7 +683,7 @@ func Test_ContainerHelper_Deploy(t *testing.T) { mockContainerRegistryService.AssertCalled( t, "Login", - *mockContext.Context, + mock.Anything, env.GetSubscriptionId(), registryName, ) @@ -921,7 +921,7 @@ func Test_ContainerHelper_Credential_Retry(t *testing.T) { func setupContainerRegistryMocks(mockContext *mocks.MockContext, mockContainerRegistryService *mock.Mock) { mockContainerRegistryService.On( "Login", - *mockContext.Context, + mock.Anything, mock.AnythingOfType("string"), mock.AnythingOfType("string")). Return(nil) @@ -1250,7 +1250,7 @@ func Test_ContainerHelper_Publish(t *testing.T) { mockContainerRegistryService.AssertCalled( t, "Login", - *mockContext.Context, + mock.Anything, env.GetSubscriptionId(), registryName, ) diff --git a/cli/azd/pkg/project/framework_service_docker.go b/cli/azd/pkg/project/framework_service_docker.go index 68c8915deb1..dab9544e1b2 100644 --- a/cli/azd/pkg/project/framework_service_docker.go +++ b/cli/azd/pkg/project/framework_service_docker.go @@ -7,7 +7,6 @@ import ( "context" "errors" "os" - "path/filepath" "github.com/azure/azure-dev/cli/azd/pkg/alpha" "github.com/azure/azure-dev/cli/azd/pkg/async" @@ -171,21 +170,10 @@ func useDotnetPublishForDockerBuild(serviceConfig *ServiceConfig) bool { serviceConfig.useDotNetPublishForDockerBuild = new(false) if serviceConfig.Language.IsDotNet() { - projectPath := serviceConfig.Path() - dockerOptions := getDockerOptionsWithDefaults(serviceConfig.Docker) + resolveDockerPaths(serviceConfig, &dockerOptions) - dockerfilePath := dockerOptions.Path - if !filepath.IsAbs(dockerfilePath) { - s, err := os.Stat(projectPath) - if err == nil && s.IsDir() { - dockerfilePath = filepath.Join(projectPath, dockerfilePath) - } else { - dockerfilePath = filepath.Join(filepath.Dir(projectPath), dockerfilePath) - } - } - - if _, err := os.Stat(dockerfilePath); errors.Is(err, os.ErrNotExist) { + if _, err := os.Stat(dockerOptions.Path); errors.Is(err, os.ErrNotExist) { serviceConfig.useDotNetPublishForDockerBuild = new(true) } } diff --git a/cli/azd/pkg/project/framework_service_docker_test.go b/cli/azd/pkg/project/framework_service_docker_test.go index 91da78812f5..dbc19518b1a 100644 --- a/cli/azd/pkg/project/framework_service_docker_test.go +++ b/cli/azd/pkg/project/framework_service_docker_test.go @@ -46,6 +46,9 @@ services: ` ran := false + // Create service temp dir upfront so the mock closure can reference it. + serviceDir := t.TempDir() + env := environment.NewWithValues("test-env", nil) env.SetSubscriptionId("sub") @@ -81,10 +84,10 @@ services: require.Equal(t, []string{ "build", - "-f", "./Dockerfile", + "-f", filepath.Join(serviceDir, "Dockerfile"), "--platform", docker.DefaultPlatform, "-t", "test-proj-web", - ".", + serviceDir, }, argsNoFile) // create the file as expected @@ -102,10 +105,9 @@ services: require.NoError(t, err) service := projectConfig.Services["web"] - temp := t.TempDir() - service.Project.Path = temp + service.Project.Path = serviceDir service.RelativePath = "" - err = os.WriteFile(filepath.Join(temp, "Dockerfile"), []byte("FROM node:14"), 0600) + err = os.WriteFile(filepath.Join(serviceDir, "Dockerfile"), []byte("FROM node:14"), 0600) require.NoError(t, err) npmCli := node.NewCli(mockContext.CommandRunner) @@ -162,6 +164,10 @@ services: env := environment.NewWithValues("test-env", nil) env.SetSubscriptionId("sub") + + // Create service temp dir upfront so the mock closure can reference it. + serviceDir := t.TempDir() + mockContext := mocks.NewMockContext(context.Background()) envManager := &mockenv.MockEnvManager{} envManager.On("Get", mock.Anything, "test-env").Return(env, nil) @@ -196,10 +202,10 @@ services: require.Equal(t, []string{ "build", - "-f", "./Dockerfile.dev", + "-f", filepath.Join(serviceDir, "Dockerfile.dev"), "--platform", docker.DefaultPlatform, "-t", "test-proj-web", - "../", + filepath.Join(serviceDir, ".."), }, argsNoFile) // create the file as expected @@ -221,10 +227,9 @@ services: require.NoError(t, err) service := projectConfig.Services["web"] - temp := t.TempDir() - service.Project.Path = temp + service.Project.Path = serviceDir service.RelativePath = "" - err = os.WriteFile(filepath.Join(temp, "Dockerfile.dev"), []byte("FROM node:14"), 0600) + err = os.WriteFile(filepath.Join(serviceDir, "Dockerfile.dev"), []byte("FROM node:14"), 0600) require.NoError(t, err) internalFramework := NewNodeProject(npmCli, env, mockContext.CommandRunner) @@ -406,7 +411,7 @@ func Test_DockerProject_Build(t *testing.T) { language: ServiceLanguageJavaScript, hasDockerFile: true, init: func(t *testing.T) error { - os.Setenv("AZD_CUSTOM_OS_VAR", "os-value") + t.Setenv("AZD_CUSTOM_OS_VAR", "os-value") return nil }, env: environment.NewWithValues("test", map[string]string{ @@ -559,7 +564,30 @@ func Test_DockerProject_Build(t *testing.T) { require.NoError(t, err) require.NotNil(t, result) require.Equal(t, tt.expectedBuildResult, result) - require.Equal(t, tt.expectedDockerBuildArgs, dockerBuildArgs.Args) + + if tt.expectedDockerBuildArgs != nil { + // Build expected absolute paths directly from test temp dir. + // Do NOT call resolveDockerPaths here - it is the production helper + // under test, so using it would make assertions self-referential. + serviceDir := filepath.Join(temp, tt.project) + expected := make([]string, len(tt.expectedDockerBuildArgs)) + copy(expected, tt.expectedDockerBuildArgs) + for i := range expected { + if i > 0 && expected[i-1] == "-f" { + if !filepath.IsAbs(expected[i]) { + expected[i] = filepath.Clean(filepath.Join(serviceDir, expected[i])) + } + } + } + if n := len(expected); n > 0 { + last := expected[n-1] + if !filepath.IsAbs(last) && + (last == "." || strings.HasPrefix(last, "./") || strings.HasPrefix(last, "../")) { + expected[n-1] = filepath.Clean(filepath.Join(serviceDir, last)) + } + } + require.Equal(t, expected, dockerBuildArgs.Args) + } } }) } diff --git a/docs/specs/perf-foundations/spec.md b/docs/specs/perf-foundations/spec.md new file mode 100644 index 00000000000..06795c5c45c --- /dev/null +++ b/docs/specs/perf-foundations/spec.md @@ -0,0 +1,287 @@ +# Performance Foundations for Azure Developer CLI + +## Overview + +This changeset introduces targeted performance optimizations to reduce deployment latency in `azd`, +particularly when multiple services are provisioned and deployed concurrently. Each change addresses a +specific bottleneck observed during parallel `azd up` runs with 4-8 services. + +## Changes + +### 1. HTTP Connection Pooling (TunedTransport) + +**Files:** `pkg/httputil/util.go`, `cmd/deps.go` + +**Problem:** Go's `http.DefaultTransport` limits connections to 2 per host (`MaxIdleConnsPerHost=2`) and +100 total idle connections. When `azd` deploys 8 services in parallel, each hitting ARM endpoints +(`management.azure.com`), connections are torn down and re-established constantly. Every new TLS 1.2+ +handshake to ARM costs 1-2 round trips (50-150ms per handshake depending on region). + +**Solution:** `TunedTransport()` clones `http.DefaultTransport` and raises: +- `MaxIdleConns`: 100 -> 200 (total idle pool across all hosts) +- `MaxConnsPerHost`: 0 (unlimited) -> 50 (bounded but generous) +- `MaxIdleConnsPerHost`: 2 -> 50 (matches MaxConnsPerHost so idle connections aren't evicted) +- `IdleConnTimeout`: 90s -> 30s (reclaim unused connections faster) +- `DisableKeepAlives`: false (explicit; HTTP/1.1 keep-alive per RFC 7230 Section 6.3) + +**Evidence:** Go stdlib defaults are documented in `net/http/transport.go`. The per-host idle limit of 2 +is the primary bottleneck; raising it to match `MaxConnsPerHost` ensures connections created during a +burst are retained for subsequent requests to the same host. The 30s idle timeout is sufficient because +`azd` operations complete within seconds of each other during parallel deployment. + +**Wiring:** `cmd/deps.go` replaces `http.DefaultClient` with `&http.Client{Transport: TunedTransport()}` +so all SDK clients created through dependency injection inherit the tuned transport. + +### 2. ARM Client Caching (sync.Map) + +**Files:** `pkg/azapi/resource_service.go`, `pkg/azapi/standard_deployments.go`, +`pkg/azapi/stack_deployments.go` + +**Problem:** ARM SDK clients (`armresources.Client`, `DeploymentsClient`, `DeploymentOperationsClient`, +`armdeploymentstacks.Client`) were re-created on every API call. Each construction builds an HTTP +pipeline (retry policy, logging policy, auth policy). While not as expensive as a TLS handshake, this +adds unnecessary CPU and allocation overhead when the same subscription is used repeatedly. + +**Solution:** Cache clients in `sync.Map` fields keyed by subscription ID. The pattern uses +`Load` for the fast path and `LoadOrStore` on miss. The "benign race" comment documents that concurrent +cache misses may create duplicate clients; `LoadOrStore` ensures only one is retained. This is safe +because Azure SDK ARM clients are stateless and goroutine-safe (they hold only the pipeline +configuration, not per-request state). + +**Evidence:** Azure SDK for Go documentation confirms ARM clients are safe for concurrent use: +"A client is safe for concurrent use across goroutines" (azure-sdk-for-go design guidelines). `sync.Map` +is the standard Go pattern for read-heavy caches with infrequent writes, avoiding lock contention on the +hot path. + +### 3. Adaptive Poll Frequency + +**Files:** `pkg/azapi/standard_deployments.go`, `pkg/azapi/stack_deployments.go` + +**Problem:** `PollUntilDone(ctx, nil)` uses the Azure SDK's default polling interval of 30 seconds. +For deploy operations that typically complete in 30-120 seconds, this means the first successful poll may +arrive 30s after the deployment actually finished, adding unnecessary wall-clock latency. + +**Solution:** Two tuned frequencies: +- `deployPollFrequency = 5s` for deploy and delete operations (variable completion time; 6x faster + than the 30s default while leaving ample headroom against ARM rate limits) +- `slowPollFrequency = 5s` for WhatIf and Validate operations (consistently slow at 30-90s; aggressive + polling wastes ARM read quota without benefit) + +The 5s interval balances latency reduction against ARM rate limits: 1200 reads per 5 minutes per +subscription. With 8 parallel deployments polling at 5s, that's 96 reads/minute per operation type, +leaving substantial headroom for other ARM reads (list operations, status checks, etc.). + +**Evidence:** Azure SDK `runtime.PollUntilDoneOptions.Frequency` is the documented mechanism. ARM rate +limits are documented at +`learn.microsoft.com/en-us/azure/azure-resource-manager/management/request-limits-and-throttling`. + +### 4. Zip Deploy Retry with SCM Readiness Probe + +**Files:** `pkg/azapi/webapp.go`, `pkg/azsdk/zip_deploy_client.go` + +**Problem:** During concurrent `azd up`, ARM applies site configuration (app settings) shortly after the +App Service resource is created. This triggers an SCM (Kudu) container restart. If the zip deploy starts +while the SCM container is restarting, the Oryx build process fails with "the build process failed". +This is a transient failure specific to the concurrent provisioning + deployment pattern. + +**Solution:** +- `isBuildFailure(err)` detects the specific transient error string ("the build process failed") while + excluding genuine build errors that contain "logs for more info" +- On build failure, retry up to 2 additional times (3 total attempts) +- Before each retry, `waitForScmReady()` polls the SCM `/api/deployments` endpoint until it returns + HTTP 200, indicating the container has finished restarting (90s timeout, 5s poll interval) +- The zip file `ReadSeeker` is rewound via `Seek(0, io.SeekStart)` before each retry +- After the retry loop exits (success or exhausted), the zip file is also rewound before the non-Linux + fallback `Deploy()` path, since the tracked deploy consumed the reader + +**OTel Tracing:** The entire `DeployAppServiceZip` function is wrapped in a tracing span named +`"deploy.appservice.zip"` with attributes: `deploy.appservice.app` (app name), +`deploy.appservice.rg` (resource group), `deploy.appservice.linux` (boolean), and +`deploy.appservice.attempt` (1-indexed attempt number, updated per retry iteration). The span uses +named return `err` with `span.EndWithStatus(err)` to automatically record failure status. + +`IsScmReady()` on `ZipDeployClient` sends a lightweight GET to `/api/deployments`. Connection errors +return `(false, nil)` rather than propagating - the SCM is still restarting, which is the expected state. +The `//nolint:nilerr` directive documents this intentional error suppression for the linter. + +### 5. ACR Credential Exponential Backoff + +**File:** `pkg/project/container_helper.go` + +**Problem:** ACR credential retrieval after resource creation used a constant 20-second retry delay +(`retry.NewConstant(20s)`). In the common case, credentials are available within 1-5 seconds after the +ACR resource is provisioned. The 20s constant delay means even a single retry wastes 15-19 seconds. + +**Solution:** Changed to exponential backoff starting at 2 seconds: +`retry.NewExponential(2s)` with max 5 retries produces delays of 2s, 4s, 8s, 16s, 32s (62s worst case +vs 60s worst case with the old constant 20s * 3 retries). The exponential curve means most transient +404s (DNS propagation, eventual consistency) resolve on the first or second retry, while the longer +tail preserves roughly the same total retry window for slow DNS propagation edge cases. + +**Evidence:** Azure DNS propagation FAQ confirms changes "typically take effect within 60 seconds" but +are often faster. The old link in the comment +(`learn.microsoft.com/en-us/azure/dns/dns-faq#how-long-does-it-take-for-dns-changes-to-take-effect-`) +documents worst-case TTL, not typical latency. + +### 6. Docker Path Resolution Fix + +**Files:** `pkg/project/container_helper.go`, `pkg/project/framework_service_docker.go`, +`pkg/project/framework_service_docker_test.go` + +**Problem:** Docker path resolution was inconsistent across `Build()`, `runRemoteBuild()`, +`packBuild()`, and `useDotNetPublishForDockerBuild()`. User-specified `docker.path` and +`docker.context` in `azure.yaml` are relative to the project root (where `azure.yaml` lives), but +default paths (`./Dockerfile`, `.`) are relative to the service directory. The old code used +`serviceConfig.Path()` for both cases, which broke when a .NET service's `azure.yaml` specified a +custom `docker.path` pointing to a Dockerfile outside the service directory. + +**Solution:** Extracted `resolveDockerPaths()` that resolves docker.path and docker.context +to absolute paths. User-specified paths (from `azure.yaml`) are resolved relative to the project +root (where `azure.yaml` lives), while default paths (`./Dockerfile`, `.`) are resolved relative to +the service directory via `resolveServiceDir()`. This centralizes path resolution that was +previously duplicated across `Build()`, `runRemoteBuild()`, and other call sites. + +Called from `Build()`, `runRemoteBuild()`, and `useDotNetPublishForDockerBuild()` (which now calls +`resolveDockerPaths()` directly instead of reimplementing the logic). Tests updated to expect +absolute resolved paths, including dynamic resolution in the table-driven `Test_DockerProject_Build` +test. `filepath.Clean` is applied to both resolved paths to normalize separators and prevent +path traversal. + +### 7. ReadRawResponse Body Close Fix + +**File:** `pkg/httputil/util.go` + +**Problem:** `ReadRawResponse` read `response.Body` via `io.ReadAll` but never closed it. While Go's +HTTP client reuses connections when the body is fully read and closed, an unclosed body prevents the +underlying TCP connection from returning to the pool. + +**Solution:** Added `defer response.Body.Close()` at the top of the function. + +### 8. UpdateAppServiceAppSetting Helper + +**File:** `pkg/azapi/webapp.go` + +**Problem:** No API existed to update a single App Service application setting without replacing the +entire set. Callers that needed to add or modify one setting had to manually read-modify-write the +full settings dictionary, duplicating boilerplate. + +**Solution:** Added `UpdateAppServiceAppSetting(ctx, subscriptionId, resourceGroup, appName, key, +value)` that reads existing settings via `ListApplicationSettings`, adds/overwrites the specified +key-value pair, and writes all settings back via `UpdateApplicationSettings`. This preserves other +settings that are not being modified. The function handles the nil-properties edge case by +initializing the map if empty. + +### 9. OTel Tracing Spans for Deployment Profiling + +**Files:** `pkg/azapi/standard_deployments.go`, `pkg/azapi/stack_deployments.go`, +`pkg/project/container_helper.go`, `pkg/project/container_helper_test.go` + +**Problem:** Only `DeployAppServiceZip` had OTel tracing. ARM deployments, validation, +WhatIf operations, and container build/publish had no span coverage, making it impossible to +profile where wall-clock time is spent during parallel deployment. + +**Solution:** Added 11 OTel tracing spans to all performance-critical deployment paths using +the established pattern: `tracing.Start(ctx, spanName)` + named return `err` + `defer func() +{ span.EndWithStatus(err) }()`. Each span includes `attribute.String` for subscription, +resource group, and deployment name to enable correlation in trace viewers. + +**ARM Standard Deployments** (`standard_deployments.go` — 6 spans): +- `arm.deploy.subscription` — `DeployToSubscription` (ARM deploy at subscription scope) +- `arm.deploy.resourcegroup` — `DeployToResourceGroup` (ARM deploy at resource group scope) +- `arm.whatif.subscription` — `WhatIfDeployToSubscription` (WhatIf preview at subscription) +- `arm.whatif.resourcegroup` — `WhatIfDeployToResourceGroup` (WhatIf preview at resource group) +- `arm.validate.subscription` — `ValidatePreflightToSubscription` (preflight validation) +- `arm.validate.resourcegroup` — `ValidatePreflightToResourceGroup` (preflight validation) + +**Deployment Stacks** (`stack_deployments.go` — 2 spans): +- `arm.stack.deploy.subscription` — `DeployToSubscription` (deployment stack at subscription) +- `arm.stack.deploy.resourcegroup` — `DeployToResourceGroup` (deployment stack at resource group) + +**Container Operations** (`container_helper.go` — 3 spans): +- `container.publish` — `Publish` (end-to-end container publish orchestration; includes + `container.remotebuild` boolean attribute) +- `container.credentials` — `Credentials` (ACR credential retrieval with exponential backoff) +- `container.remotebuild` — `runRemoteBuild` (ACR remote build including context upload) + +**Test Updates** (`container_helper_test.go`): Mock assertions for `Login` updated from exact +context matching (`*mockContext.Context`) to `mock.Anything` because `tracing.Start` wraps the +context with a span value, causing strict equality to fail. This applies to +`setupContainerRegistryMocks`, `Test_ContainerHelper_Deploy`, and +`Test_ContainerHelper_Deploy_ImageOverride` test assertions. + +**Evidence:** These spans cover the four longest-running operation categories during parallel +deployment: ARM provisioning (30-120s), WhatIf/Validate (30-90s), container build/push (30- +300s), and ACR credential retrieval (2-62s with backoff). Together with the existing +`deploy.appservice.zip` span, they provide full end-to-end visibility for profiling `azd up`. + +### 10. Supporting Changes + +**Files:** `.gitignore`, `cli/azd/.vscode/cspell-azd-dictionary.txt` + +- `.gitignore`: Added coverage artifacts (`cover-*`, `cover_*`, `review-*.diff`) and Playwright MCP + directory (`.playwright-mcp/`) +- `cspell-azd-dictionary.txt`: Added `keepalives` (HTTP keep-alive terminology in TunedTransport), + `nilerr` (golangci-lint nolint directive in `IsScmReady`), `appsettings` (App Service terminology) + +### 11. Container App Adaptive Poll Frequency + +**File:** `pkg/containerapps/container_app.go` + +**Problem:** Container App create/update/job operations used `PollUntilDone(ctx, nil)`, defaulting +to the Azure SDK's 30-second polling interval. Unlike ARM template deployments (which go through +`standard_deployments.go` where poll frequency was already tuned in §3), Container App revision +updates go through a separate code path (`containerAppService.DeployYaml`, `AddRevision`, +`updateContainerApp`, `UpdateContainerAppJob`). Templates with multiple Container Apps (e.g., the +5-service "shop" template) suffered compounded tail latency: up to 28 seconds wasted per service × +5 services = **140 seconds** of unnecessary wait time. + +**Solution:** Added `containerAppPollFrequency = 5 * time.Second` constant and applied it to all +three `PollUntilDone` call sites: +- `DeployYaml` → `BeginCreateOrUpdate` poller (line 321) +- `updateContainerApp` → `BeginUpdate` poller (line 546) +- `UpdateContainerAppJob` → `BeginUpdate` poller (line 780) + +**Evidence:** Container App revision updates typically complete in 10-60 seconds (observed in perf +benchmarks). The same 5s frequency used for ARM deploy operations (§3) balances latency reduction +(6x faster than the 30s default) against ARM rate limits. With 8 parallel services, this generates +~96 polls/min per operation type — well within the 1200 reads/5min/subscription limit. + +### 12. ACR Login Caching per Registry + +**File:** `pkg/azapi/container_registry.go` + +**Problem:** `containerRegistryService.Login()` was called for every service during deploy, +regardless of whether the same registry had already been authenticated. For templates pushing +multiple container images to the same ACR (e.g., shop with 5 services sharing one registry), +this meant 4 redundant credential exchanges (AAD → ACR token swap via `/oauth2/exchange`) and +4 redundant `docker login` commands. Each credential exchange involves an HTTP round-trip to the +ACR token endpoint plus an ARM token acquisition — typically 2-5 seconds per call. + +**Solution:** Added two-layer deduplication to `containerRegistryService`: +- `loginGroup singleflight.Group` deduplicates in-flight concurrent login attempts. Uses `DoChan` + so each caller can independently cancel via its own context without affecting other waiters. + The shared work runs under `context.WithoutCancel` to avoid tying it to any single caller. +- `loginDone sync.Map` tracks registries that have already been authenticated this session. + Subsequent `Login()` calls return immediately with a log message. + +**Evidence:** Docker's credential store also caches logins, so the `docker login` call itself +would have been idempotent. However, the expensive part is the *credential exchange* (`Credentials()` +→ `getAcrToken()` → AAD token → ACR refresh token), which is not cached by Docker. With 5 +services sharing one ACR, this saves 4 × ~3s = **~12 seconds** plus 4 redundant Docker CLI +invocations. + +## Test Coverage + +| Change | Test File | Coverage | +|--------|-----------|----------| +| `TunedTransport` | `pkg/httputil/util_test.go` | Verifies all pool params; verifies DefaultTransport not mutated | +| `isBuildFailure` | `pkg/azapi/webapp_test.go` | 6 cases: nil, unrelated, exact match, wrapped, real build failure, partial | +| Docker path resolution | `pkg/project/framework_service_docker_test.go` | Updated to verify absolute resolved paths via dynamic resolution | +| `resolveDockerPaths` | `pkg/project/container_helper_coverage3_test.go` | 4 sub-tests: defaults, user-specified, absolute, mixed | +| Docker build args | `pkg/project/framework_service_docker_test.go` | Table-driven tests resolve expected paths to match production behavior | +| Tracing context propagation | `pkg/project/container_helper_test.go` | Mock assertions updated for tracing context wrapping (`mock.Anything`) | + +The ARM client caching, TunedTransport wiring, poll frequency (both ARM and Container App), +SCM retry logic, ACR login caching, `UpdateAppServiceAppSetting`, and OTel tracing spans are +infrastructure changes that operate through Azure SDK interactions and are validated through +integration and end-to-end playback tests rather than isolated unit tests. From fbb40f92dc09250bfba4f8fb41b88f0d946ac7bc Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Thu, 9 Apr 2026 20:29:27 -0700 Subject: [PATCH 02/12] ci: re-trigger CI (unrelated flaky test failures in grpcbroker and cmd/fig-completion) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> From 81f0e96f3c3e0860e40bdb26c3a69194e20bc59a Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Fri, 10 Apr 2026 07:26:05 -0700 Subject: [PATCH 03/12] ci: retry CI (main has ~40% flaky rate - grpcbroker, appdetect, fig-completion parsing) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> From d70861dadbc2c83efeaf362ace3619052e703295 Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Fri, 10 Apr 2026 11:35:01 -0700 Subject: [PATCH 04/12] fix: address spboyer review - body close, spec accuracy, errors.AsType - Restore unconditional defer res.Body.Close() in federated_token_client.go - Fix spec.md IdleConnTimeout description to match actual 90s value - Use errors.AsType[net.Error] instead of errors.As per AGENTS.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/auth/federated_token_client.go | 2 +- cli/azd/pkg/project/container_helper.go | 3 +-- docs/specs/perf-foundations/spec.md | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/cli/azd/pkg/auth/federated_token_client.go b/cli/azd/pkg/auth/federated_token_client.go index 98fa224c328..76bb2e3a44c 100644 --- a/cli/azd/pkg/auth/federated_token_client.go +++ b/cli/azd/pkg/auth/federated_token_client.go @@ -32,9 +32,9 @@ func (c *FederatedTokenClient) TokenForAudience(ctx context.Context, audience st if err != nil { return "", fmt.Errorf("sending request: %w", err) } + defer res.Body.Close() if !runtime.HasStatusCode(res, http.StatusOK) { - defer res.Body.Close() return "", fmt.Errorf("expected 200 response, got: %d", res.StatusCode) } diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index c1f1734ef1d..83423bef933 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -322,8 +322,7 @@ func (ch *ContainerHelper) Credentials( // during DNS propagation after ACR creation). Only retry // genuinely transient errors, not permanent URL/TLS failures. if _, ok := errors.AsType[*url.Error](err); ok { - var netErr net.Error - if errors.As(err, &netErr) && netErr.Timeout() { + if netErr, ok := errors.AsType[net.Error](err); ok && netErr.Timeout() { return retry.RetryableError(err) } msg := err.Error() diff --git a/docs/specs/perf-foundations/spec.md b/docs/specs/perf-foundations/spec.md index 06795c5c45c..67309a84718 100644 --- a/docs/specs/perf-foundations/spec.md +++ b/docs/specs/perf-foundations/spec.md @@ -21,7 +21,7 @@ handshake to ARM costs 1-2 round trips (50-150ms per handshake depending on regi - `MaxIdleConns`: 100 -> 200 (total idle pool across all hosts) - `MaxConnsPerHost`: 0 (unlimited) -> 50 (bounded but generous) - `MaxIdleConnsPerHost`: 2 -> 50 (matches MaxConnsPerHost so idle connections aren't evicted) -- `IdleConnTimeout`: 90s -> 30s (reclaim unused connections faster) +- `IdleConnTimeout`: 90s (kept explicit for documentation; matches Go default) - `DisableKeepAlives`: false (explicit; HTTP/1.1 keep-alive per RFC 7230 Section 6.3) **Evidence:** Go stdlib defaults are documented in `net/http/transport.go`. The per-host idle limit of 2 From 83a99e0f6ce2644f61e1a682d22bed5d5b376c8a Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Mon, 13 Apr 2026 11:05:53 -0700 Subject: [PATCH 05/12] docs: fix spec drift - slowPollFrequency, Docker paths, ReadRawResponse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - slowPollFrequency: 5s → 15s (matches code after ARM quota review) - Docker path resolution: all paths resolve relative to service directory - ReadRawResponse: document caller-owned body close pattern (not in-function) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/specs/perf-foundations/spec.md | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/docs/specs/perf-foundations/spec.md b/docs/specs/perf-foundations/spec.md index 67309a84718..7a7aa78806e 100644 --- a/docs/specs/perf-foundations/spec.md +++ b/docs/specs/perf-foundations/spec.md @@ -64,7 +64,7 @@ arrive 30s after the deployment actually finished, adding unnecessary wall-clock **Solution:** Two tuned frequencies: - `deployPollFrequency = 5s` for deploy and delete operations (variable completion time; 6x faster than the 30s default while leaving ample headroom against ARM rate limits) -- `slowPollFrequency = 5s` for WhatIf and Validate operations (consistently slow at 30-90s; aggressive +- `slowPollFrequency = 15s` for WhatIf and Validate operations (consistently slow at 30-90s; aggressive polling wastes ARM read quota without benefit) The 5s interval balances latency reduction against ARM rate limits: 1200 reads per 5 minutes per @@ -136,9 +136,8 @@ default paths (`./Dockerfile`, `.`) are relative to the service directory. The o custom `docker.path` pointing to a Dockerfile outside the service directory. **Solution:** Extracted `resolveDockerPaths()` that resolves docker.path and docker.context -to absolute paths. User-specified paths (from `azure.yaml`) are resolved relative to the project -root (where `azure.yaml` lives), while default paths (`./Dockerfile`, `.`) are resolved relative to -the service directory via `resolveServiceDir()`. This centralizes path resolution that was +to absolute paths. Both user-specified and default paths are resolved relative to the service +directory (via `resolveServiceDir()`) to preserve backward compatibility.This centralizes path resolution that was previously duplicated across `Build()`, `runRemoteBuild()`, and other call sites. Called from `Build()`, `runRemoteBuild()`, and `useDotNetPublishForDockerBuild()` (which now calls @@ -147,15 +146,18 @@ absolute resolved paths, including dynamic resolution in the table-driven `Test_ test. `filepath.Clean` is applied to both resolved paths to normalize separators and prevent path traversal. -### 7. ReadRawResponse Body Close Fix +### 7. ReadRawResponse Body Ownership **File:** `pkg/httputil/util.go` -**Problem:** `ReadRawResponse` read `response.Body` via `io.ReadAll` but never closed it. While Go's -HTTP client reuses connections when the body is fully read and closed, an unclosed body prevents the -underlying TCP connection from returning to the pool. +**Problem:** `ReadRawResponse` read `response.Body` via `io.ReadAll` but the body close responsibility +was unclear. The function is used by both standalone callers (who own the response) and the +zip-deploy polling handler (where azcore's poller framework owns the response lifecycle). -**Solution:** Added `defer response.Body.Close()` at the top of the function. +**Solution:** `ReadRawResponse` intentionally does NOT close the body — callers are responsible for +closing it. This preserves compatibility with azcore's poller framework which manages response +lifecycle. Standalone callers (e.g., `federated_token_client.go`) use `defer res.Body.Close()` +before calling `ReadRawResponse`. ### 8. UpdateAppServiceAppSetting Helper From 984e345e385d5a30da5e285b12037eda91d99832 Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Mon, 13 Apr 2026 11:12:28 -0700 Subject: [PATCH 06/12] docs: quantify ARM rate limit concurrency ceiling in comment Add safe concurrency math: 5s poll = ~12 reads/min/poller, safe for ~20 parallel LROs within 1200 reads/5min budget. Note adaptive backoff on 429. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/azapi/standard_deployments.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cli/azd/pkg/azapi/standard_deployments.go b/cli/azd/pkg/azapi/standard_deployments.go index 03dfb0e008b..6f75b3610c2 100644 --- a/cli/azd/pkg/azapi/standard_deployments.go +++ b/cli/azd/pkg/azapi/standard_deployments.go @@ -38,8 +38,9 @@ const ( // deployPollFrequency is the polling interval for ARM deploy/delete operations. // Deployments complete in variable time, so polling faster than the 30s SDK default - // reduces tail latency. 5s balances responsiveness against the ARM read-rate limit - // (1200 reads/5min/subscription) when many LROs run in parallel. + // reduces tail latency. At 5s intervals each poller consumes ~12 reads/min. With the + // ARM read-rate limit of 1200 reads/5min/subscription, this is safe for up to ~20 + // parallel LROs. If higher concurrency is needed, consider adaptive backoff on HTTP 429. deployPollFrequency = 5 * time.Second // slowPollFrequency is the polling interval for ARM WhatIf and Validate operations. From f3ea925ab84ca00ccb04571d658d6fe16f30390b Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Mon, 13 Apr 2026 16:05:52 -0700 Subject: [PATCH 07/12] address weikanglim review: remove customer data from spans, register events/fields centrally, fix ACR retry amplification, conservative transport params, context handling fixes, remove unused API, conservative SCM error handling - Remove customer data (appName, resourceGroup, subscriptionId, deploymentName) from all OTel span attributes - Register all span names as constants in internal/tracing/events/events.go (12 new events) - Register DeployAttemptKey and DeployLinuxKey in internal/tracing/fields/fields.go - Use registered field keys instead of raw attribute strings in webapp.go - Remove 429/5xx from ACR outer retry (SDK handles these; prevents retry amplification) - Change MaxIdleConnsPerHost from 50 to 16, remove MaxConnsPerHost/DisableKeepAlives overrides - Replace blanket transport error catch-all with conservative allowlist in IsScmReady - Simplify context cancellation checks to ctx.Err() != nil - Differentiate Ctrl+C vs timeout in waitForScmReady - Remove unused UpdateAppServiceAppSetting function - Reword concurrency comment to describe eventual consistency - Update spec.md to reflect all changes, renumber sections - Update util_test.go assertions for new transport values Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/internal/tracing/events/events.go | 24 +++++++ cli/azd/internal/tracing/fields/fields.go | 18 +++++ cli/azd/pkg/azapi/stack_deployments.go | 15 +---- cli/azd/pkg/azapi/standard_deployments.go | 41 ++---------- cli/azd/pkg/azapi/webapp.go | 82 +++++------------------ cli/azd/pkg/azsdk/zip_deploy_client.go | 19 ++++-- cli/azd/pkg/httputil/util.go | 14 ++-- cli/azd/pkg/httputil/util_test.go | 4 +- cli/azd/pkg/project/container_helper.go | 14 ++-- docs/specs/perf-foundations/spec.md | 67 +++++++++--------- 10 files changed, 135 insertions(+), 163 deletions(-) diff --git a/cli/azd/internal/tracing/events/events.go b/cli/azd/internal/tracing/events/events.go index 23879cf3e28..c288cdc7f36 100644 --- a/cli/azd/internal/tracing/events/events.go +++ b/cli/azd/internal/tracing/events/events.go @@ -52,3 +52,27 @@ const ( // skips Kubernetes context setup because the cluster isn't available yet. AksPostprovisionSkipEvent = "aks.postprovision.skip" ) + +// ARM deployment events track provisioning, validation, and preview operations. +const ( + ArmDeploySubscriptionEvent = "arm.deploy.subscription" + ArmDeployResourceGroupEvent = "arm.deploy.resourcegroup" + ArmStackDeploySubscriptionEvent = "arm.stack.deploy.subscription" + ArmStackDeployResourceGroupEvent = "arm.stack.deploy.resourcegroup" + ArmWhatIfSubscriptionEvent = "arm.whatif.subscription" + ArmWhatIfResourceGroupEvent = "arm.whatif.resourcegroup" + ArmValidateSubscriptionEvent = "arm.validate.subscription" + ArmValidateResourceGroupEvent = "arm.validate.resourcegroup" +) + +// App Service deployment events. +const ( + DeployAppServiceZipEvent = "deploy.appservice.zip" +) + +// Container lifecycle events. +const ( + ContainerCredentialsEvent = "container.credentials" + ContainerPublishEvent = "container.publish" + ContainerRemoteBuildEvent = "container.remotebuild" +) diff --git a/cli/azd/internal/tracing/fields/fields.go b/cli/azd/internal/tracing/fields/fields.go index f3817711ee1..711d450866a 100644 --- a/cli/azd/internal/tracing/fields/fields.go +++ b/cli/azd/internal/tracing/fields/fields.go @@ -259,6 +259,24 @@ var ( } ) +// Deployment attributes +var ( + // DeployAttemptKey tracks the retry attempt number for App Service zip deployments. + DeployAttemptKey = AttributeKey{ + Key: attribute.Key("deploy.appservice.attempt"), + Classification: SystemMetadata, + Purpose: PerformanceAndHealth, + IsMeasurement: true, + } + + // DeployLinuxKey tracks whether an App Service deployment targets a Linux web app. + DeployLinuxKey = AttributeKey{ + Key: attribute.Key("deploy.appservice.linux"), + Classification: SystemMetadata, + Purpose: FeatureInsight, + } +) + // All possible enumerations of ExecutionEnvironmentKey // // Environments are mutually exclusive. Modifiers can be set additionally to signal different types of usages. diff --git a/cli/azd/pkg/azapi/stack_deployments.go b/cli/azd/pkg/azapi/stack_deployments.go index 43df69828b5..46c5fe86cd2 100644 --- a/cli/azd/pkg/azapi/stack_deployments.go +++ b/cli/azd/pkg/azapi/stack_deployments.go @@ -23,6 +23,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" "github.com/azure/azure-dev/cli/azd/internal" "github.com/azure/azure-dev/cli/azd/internal/tracing" + "github.com/azure/azure-dev/cli/azd/internal/tracing/events" "github.com/azure/azure-dev/cli/azd/pkg/account" "github.com/azure/azure-dev/cli/azd/pkg/alpha" "github.com/azure/azure-dev/cli/azd/pkg/async" @@ -33,7 +34,6 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/benbjohnson/clock" "github.com/sethvargo/go-retry" - "go.opentelemetry.io/otel/attribute" ) var FeatureDeploymentStacks = alpha.MustFeatureKey("deployment.stacks") @@ -259,12 +259,8 @@ func (d *StackDeployments) DeployToSubscription( tags map[string]*string, options map[string]any, ) (_ *ResourceDeployment, err error) { - ctx, span := tracing.Start(ctx, "arm.stack.deploy.subscription") + ctx, span := tracing.Start(ctx, events.ArmStackDeploySubscriptionEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("arm.subscription", subscriptionId), - attribute.String("arm.deployment", deploymentName), - ) client, err := d.createClient(ctx, subscriptionId) if err != nil { @@ -342,13 +338,8 @@ func (d *StackDeployments) DeployToResourceGroup( tags map[string]*string, options map[string]any, ) (_ *ResourceDeployment, err error) { - ctx, span := tracing.Start(ctx, "arm.stack.deploy.resourcegroup") + ctx, span := tracing.Start(ctx, events.ArmStackDeployResourceGroupEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("arm.subscription", subscriptionId), - attribute.String("arm.resourcegroup", resourceGroup), - attribute.String("arm.deployment", deploymentName), - ) client, err := d.createClient(ctx, subscriptionId) if err != nil { diff --git a/cli/azd/pkg/azapi/standard_deployments.go b/cli/azd/pkg/azapi/standard_deployments.go index 6f75b3610c2..f169a9a5a2c 100644 --- a/cli/azd/pkg/azapi/standard_deployments.go +++ b/cli/azd/pkg/azapi/standard_deployments.go @@ -20,6 +20,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" "github.com/azure/azure-dev/cli/azd/internal/tracing" + "github.com/azure/azure-dev/cli/azd/internal/tracing/events" "github.com/azure/azure-dev/cli/azd/pkg/account" "github.com/azure/azure-dev/cli/azd/pkg/async" "github.com/azure/azure-dev/cli/azd/pkg/azure" @@ -27,7 +28,6 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/convert" "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/benbjohnson/clock" - "go.opentelemetry.io/otel/attribute" ) // cArmDeploymentNameLengthMax is the maximum length of the name of a deployment in ARM. @@ -240,12 +240,8 @@ func (ds *StandardDeployments) DeployToSubscription( tags map[string]*string, options map[string]any, ) (_ *ResourceDeployment, err error) { - ctx, span := tracing.Start(ctx, "arm.deploy.subscription") + ctx, span := tracing.Start(ctx, events.ArmDeploySubscriptionEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("arm.subscription", subscriptionId), - attribute.String("arm.deployment", deploymentName), - ) deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { @@ -286,13 +282,8 @@ func (ds *StandardDeployments) DeployToResourceGroup( tags map[string]*string, options map[string]any, ) (_ *ResourceDeployment, err error) { - ctx, span := tracing.Start(ctx, "arm.deploy.resourcegroup") + ctx, span := tracing.Start(ctx, events.ArmDeployResourceGroupEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("arm.subscription", subscriptionId), - attribute.String("arm.resourcegroup", resourceGroup), - attribute.String("arm.deployment", deploymentName), - ) deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { @@ -609,12 +600,8 @@ func (ds *StandardDeployments) WhatIfDeployToSubscription( armTemplate azure.RawArmTemplate, parameters azure.ArmParameters, ) (_ *armresources.WhatIfOperationResult, err error) { - ctx, span := tracing.Start(ctx, "arm.whatif.subscription") + ctx, span := tracing.Start(ctx, events.ArmWhatIfSubscriptionEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("arm.subscription", subscriptionId), - attribute.String("arm.deployment", deploymentName), - ) deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { @@ -653,13 +640,8 @@ func (ds *StandardDeployments) WhatIfDeployToResourceGroup( armTemplate azure.RawArmTemplate, parameters azure.ArmParameters, ) (_ *armresources.WhatIfOperationResult, err error) { - ctx, span := tracing.Start(ctx, "arm.whatif.resourcegroup") + ctx, span := tracing.Start(ctx, events.ArmWhatIfResourceGroupEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("arm.subscription", subscriptionId), - attribute.String("arm.resourcegroup", resourceGroup), - attribute.String("arm.deployment", deploymentName), - ) deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { @@ -790,12 +772,8 @@ func (ds *StandardDeployments) ValidatePreflightToSubscription( tags map[string]*string, options map[string]any, ) (err error) { - ctx, span := tracing.Start(ctx, "arm.validate.subscription") + ctx, span := tracing.Start(ctx, events.ArmValidateSubscriptionEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("arm.subscription", subscriptionId), - attribute.String("arm.deployment", deploymentName), - ) deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { @@ -842,13 +820,8 @@ func (ds *StandardDeployments) ValidatePreflightToResourceGroup( tags map[string]*string, options map[string]any, ) (err error) { - ctx, span := tracing.Start(ctx, "arm.validate.resourcegroup") + ctx, span := tracing.Start(ctx, events.ArmValidateResourceGroupEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("arm.subscription", subscriptionId), - attribute.String("arm.resourcegroup", resourceGroup), - attribute.String("arm.deployment", deploymentName), - ) deploymentClient, err := ds.createDeploymentsClient(ctx, subscriptionId) if err != nil { diff --git a/cli/azd/pkg/azapi/webapp.go b/cli/azd/pkg/azapi/webapp.go index 0975f836aa0..3dc475895fd 100644 --- a/cli/azd/pkg/azapi/webapp.go +++ b/cli/azd/pkg/azapi/webapp.go @@ -16,8 +16,9 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appservice/armappservice/v2" "github.com/azure/azure-dev/cli/azd/internal/tracing" + "github.com/azure/azure-dev/cli/azd/internal/tracing/events" + "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" "github.com/azure/azure-dev/cli/azd/pkg/azsdk" - "go.opentelemetry.io/otel/attribute" ) type AzCliAppServiceProperties struct { @@ -158,14 +159,9 @@ func (cli *AzureClient) DeployAppServiceZip( deployZipFile io.ReadSeeker, progressLog func(string), ) (_ *string, err error) { - ctx, span := tracing.Start(ctx, "deploy.appservice.zip") + ctx, span := tracing.Start(ctx, events.DeployAppServiceZipEvent) defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - attribute.String("deploy.appservice.app", appName), - attribute.String("deploy.appservice.rg", resourceGroup), - ) - app, err := cli.appService(ctx, subscriptionId, resourceGroup, appName) if err != nil { return nil, err @@ -182,18 +178,18 @@ func (cli *AzureClient) DeployAppServiceZip( } isLinux := isLinuxWebApp(app) - span.SetAttributes(attribute.Bool("deploy.appservice.linux", isLinux)) + span.SetAttributes(fields.DeployLinuxKey.Key.Bool(isLinux)) // Deployment Status API only support linux web app for now if isLinux { // Build failures can be caused by an SCM container restart triggered by ARM // applying site config (app settings) shortly after the site is created. - // When concurrent provisioning + deployment is used (e.g. `azd up --concurrent`), - // the deploy step may start while the SCM container is still restarting. - // Retry the entire zip deploy when the build fails, giving the SCM time to stabilize. + // Due to eventual consistency in the Azure platform, the SCM container may + // still be restarting even after provisioning reports success. Retry the + // entire zip deploy when the build fails, giving the SCM time to stabilize. const maxBuildRetries = 2 for attempt := range maxBuildRetries + 1 { - span.SetAttributes(attribute.Int("deploy.appservice.attempt", attempt+1)) + span.SetAttributes(fields.DeployAttemptKey.Key.Int(attempt + 1)) if attempt > 0 { // Reset the zip file reader so the retry re-uploads the full content. @@ -206,9 +202,9 @@ func (cli *AzureClient) DeployAppServiceZip( // Wait for the SCM site to become ready before retrying. if waitErr := waitForScmReady(ctx, client, 5*time.Second, progressLog); waitErr != nil { - // Propagate context cancellation (Ctrl+C) or deadline exceeded - // immediately — these indicate the user or system requested abort. - if errors.Is(waitErr, context.Canceled) || errors.Is(waitErr, context.DeadlineExceeded) { + // Only propagate if the caller's own context is cancelled (e.g. Ctrl+C). + // Do not propagate context errors from waitForScmReady's internal timeout. + if ctx.Err() != nil { return nil, waitErr } log.Printf("SCM readiness check failed (non-fatal): %v", waitErr) @@ -282,11 +278,11 @@ type scmReadyChecker interface { // pollInterval controls the polling frequency; callers pass the production value // (typically 5s) while tests can use shorter intervals to avoid wall-time delays. func waitForScmReady( - ctx context.Context, client scmReadyChecker, pollInterval time.Duration, progressLog func(string), + parentCtx context.Context, client scmReadyChecker, pollInterval time.Duration, progressLog func(string), ) error { const scmReadyTimeout = 90 * time.Second - ctx, cancel := context.WithTimeout(ctx, scmReadyTimeout) + ctx, cancel := context.WithTimeout(parentCtx, scmReadyTimeout) defer cancel() progressLog("Waiting for SCM site to become ready...") @@ -304,13 +300,17 @@ func waitForScmReady( for { select { case <-ctx.Done(): + // Distinguish parent cancellation (Ctrl+C) from local timeout expiry. + if parentCtx.Err() != nil { + return parentCtx.Err() + } return fmt.Errorf("SCM site did not become ready within %v: %w", scmReadyTimeout, ctx.Err()) case <-ticker.C: } ready, err := client.IsScmReady(ctx) if err != nil { - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + if ctx.Err() != nil { return err } log.Printf("SCM readiness probe error: %v", err) @@ -397,52 +397,6 @@ func (cli *AzureClient) GetAppServiceSlots( return slots, nil } -// UpdateAppServiceAppSetting reads the existing application settings, adds or overwrites -// the specified key-value pair, and writes all settings back. This preserves other settings -// that are not being modified. -// -// Note: This function is not safe for concurrent use. The Azure App Service settings API -// replaces the full set on write and does not support ETags, so concurrent updates may -// result in lost writes. Callers should serialize updates to the same app's settings. -func (cli *AzureClient) UpdateAppServiceAppSetting( - ctx context.Context, - subscriptionId string, - resourceGroup string, - appName string, - key string, - value string, -) error { - client, err := cli.createWebAppsClient(ctx, subscriptionId) - if err != nil { - return err - } - - // Read existing settings so that we preserve them. - existing, err := client.ListApplicationSettings(ctx, resourceGroup, appName, nil) - if err != nil { - return fmt.Errorf("listing existing app settings: %w", err) - } - - if existing.Properties == nil { - existing.Properties = map[string]*string{} - } - - existing.Properties[key] = &value - - // Write all settings back (the API replaces the full set). - if _, err := client.UpdateApplicationSettings( - ctx, - resourceGroup, - appName, - armappservice.StringDictionary{Properties: existing.Properties}, - nil, - ); err != nil { - return fmt.Errorf("updating app settings: %w", err) - } - - return nil -} - // DeployAppServiceSlotZip deploys a zip file to a specific deployment slot. func (cli *AzureClient) DeployAppServiceSlotZip( ctx context.Context, diff --git a/cli/azd/pkg/azsdk/zip_deploy_client.go b/cli/azd/pkg/azsdk/zip_deploy_client.go index 0a4c48104e8..0f52a696e02 100644 --- a/cli/azd/pkg/azsdk/zip_deploy_client.go +++ b/cli/azd/pkg/azsdk/zip_deploy_client.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "strings" "time" @@ -448,11 +449,21 @@ func (c *ZipDeployClient) IsScmReady(ctx context.Context) (bool, error) { resp, err := c.pipeline.Do(req) if err != nil { // Propagate context cancellation / deadline so callers (and Ctrl+C) react immediately. - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || ctx.Err() != nil { - return false, err + if ctx.Err() != nil { + return false, ctx.Err() } - // Connection and other transport errors mean SCM is still restarting. - return false, nil //nolint:nilerr + // Only suppress clearly transient transport errors that indicate the SCM + // container is still restarting. Propagate unexpected errors (e.g. offline, + // TLS failures, bad proxy) so callers can surface meaningful diagnostics. + errMsg := err.Error() + if strings.Contains(errMsg, "connection refused") || + strings.Contains(errMsg, "no such host") { + return false, nil + } + if netErr, ok := errors.AsType[net.Error](err); ok && netErr.Timeout() { + return false, nil + } + return false, fmt.Errorf("SCM readiness probe: %w", err) } defer resp.Body.Close() diff --git a/cli/azd/pkg/httputil/util.go b/cli/azd/pkg/httputil/util.go index 78a2c973794..c9d84beb86f 100644 --- a/cli/azd/pkg/httputil/util.go +++ b/cli/azd/pkg/httputil/util.go @@ -38,17 +38,17 @@ func ReadRawResponse[T any](response *http.Response) (*T, error) { } // TunedTransport returns an http.Transport cloned from http.DefaultTransport with -// connection pooling parameters optimized for Azure CLI workloads. The key tunings -// are MaxConnsPerHost and MaxIdleConnsPerHost (raised from Go defaults) to avoid -// unnecessary TLS handshakes when making many concurrent requests to ARM endpoints. -// With parallel execution, 8+ services may hit ARM simultaneously. +// connection pooling parameters optimized for Azure CLI workloads. The key tuning +// is MaxIdleConnsPerHost (raised from Go's default of 2) to avoid unnecessary TLS +// handshakes when making many concurrent requests to ARM endpoints. +// MaxConnsPerHost is left at 0 (unlimited) — the Go default — to avoid +// artificial bottlenecks; the pool is bounded by MaxIdleConns instead. +// Keep-alive must remain enabled (the default) for connection reuse. func TunedTransport() *http.Transport { transport := http.DefaultTransport.(*http.Transport).Clone() transport.MaxIdleConns = 200 - transport.MaxConnsPerHost = 50 - transport.MaxIdleConnsPerHost = 50 + transport.MaxIdleConnsPerHost = 16 transport.IdleConnTimeout = 90 * time.Second - transport.DisableKeepAlives = false // keep-alive must remain enabled for pooling return transport } diff --git a/cli/azd/pkg/httputil/util_test.go b/cli/azd/pkg/httputil/util_test.go index c49e67a681a..2da093c4c7d 100644 --- a/cli/azd/pkg/httputil/util_test.go +++ b/cli/azd/pkg/httputil/util_test.go @@ -25,8 +25,8 @@ func TestTunedTransport(t *testing.T) { require.NotNil(t, transport) require.Equal(t, 200, transport.MaxIdleConns) - require.Equal(t, 50, transport.MaxConnsPerHost) - require.Equal(t, 50, transport.MaxIdleConnsPerHost) + require.Equal(t, 0, transport.MaxConnsPerHost) + require.Equal(t, 16, transport.MaxIdleConnsPerHost) require.Equal(t, 90*time.Second, transport.IdleConnTimeout) require.False(t, transport.DisableKeepAlives) } diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index 83423bef933..f8421635d03 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -292,7 +292,7 @@ func (ch *ContainerHelper) Credentials( targetResource *environment.TargetResource, env *environment.Environment, ) (_ *azapi.DockerCredentials, err error) { - ctx, span := tracing.Start(ctx, "container.credentials") + ctx, span := tracing.Start(ctx, events.ContainerCredentialsEvent) defer func() { span.EndWithStatus(err) }() loginServer, err := ch.RegistryName(ctx, serviceConfig, env) @@ -312,9 +312,11 @@ func (ch *ContainerHelper) Credentials( cred, err := ch.containerRegistryService.Credentials(ctx, targetResource.SubscriptionId(), loginServer) if err != nil { if httpErr, ok := errors.AsType[*azcore.ResponseError](err); ok { - if httpErr.StatusCode == 404 || - httpErr.StatusCode == 429 || - httpErr.StatusCode >= 500 { + // Only retry 404 — the ACR resource may not have propagated yet. + // Do NOT retry 429 or 5xx here: the Azure SDK's built-in retry + // policy already handles those, so retrying at this layer would + // cause retry amplification (outer × inner). + if httpErr.StatusCode == 404 { return retry.RetryableError(err) } } @@ -610,7 +612,7 @@ func (ch *ContainerHelper) Publish( progress *async.Progress[ServiceProgress], options *PublishOptions, ) (_ *ServicePublishResult, err error) { - ctx, span := tracing.Start(ctx, "container.publish") + ctx, span := tracing.Start(ctx, events.ContainerPublishEvent) defer func() { span.EndWithStatus(err) }() span.SetAttributes( attribute.Bool("container.remotebuild", serviceConfig.Docker.RemoteBuild), @@ -800,7 +802,7 @@ func (ch *ContainerHelper) runRemoteBuild( progress *async.Progress[ServiceProgress], imageOverride *imageOverride, ) (_ string, err error) { - ctx, span := tracing.Start(ctx, "container.remotebuild") + ctx, span := tracing.Start(ctx, events.ContainerRemoteBuildEvent) defer func() { span.EndWithStatus(err) }() dockerOptions := getDockerOptionsWithDefaults(serviceConfig.Docker) diff --git a/docs/specs/perf-foundations/spec.md b/docs/specs/perf-foundations/spec.md index 7a7aa78806e..c2c121b34e2 100644 --- a/docs/specs/perf-foundations/spec.md +++ b/docs/specs/perf-foundations/spec.md @@ -19,10 +19,13 @@ handshake to ARM costs 1-2 round trips (50-150ms per handshake depending on regi **Solution:** `TunedTransport()` clones `http.DefaultTransport` and raises: - `MaxIdleConns`: 100 -> 200 (total idle pool across all hosts) -- `MaxConnsPerHost`: 0 (unlimited) -> 50 (bounded but generous) -- `MaxIdleConnsPerHost`: 2 -> 50 (matches MaxConnsPerHost so idle connections aren't evicted) +- `MaxIdleConnsPerHost`: 2 -> 16 (conservative increase over Go's default of 2, avoids overly + aggressive per-host limits that could stress backend connection tracking) - `IdleConnTimeout`: 90s (kept explicit for documentation; matches Go default) -- `DisableKeepAlives`: false (explicit; HTTP/1.1 keep-alive per RFC 7230 Section 6.3) + +`MaxConnsPerHost` is left at 0 (unlimited, Go's default) because the Azure SDK already has built-in +concurrency controls via retry policies and rate limiting. `DisableKeepAlives` is left at Go's +default (false) since `http.DefaultTransport` already enables keep-alives. **Evidence:** Go stdlib defaults are documented in `net/http/transport.go`. The per-host idle limit of 2 is the primary bottleneck; raising it to match `MaxConnsPerHost` ensures connections created during a @@ -95,14 +98,16 @@ This is a transient failure specific to the concurrent provisioning + deployment fallback `Deploy()` path, since the tracked deploy consumed the reader **OTel Tracing:** The entire `DeployAppServiceZip` function is wrapped in a tracing span named -`"deploy.appservice.zip"` with attributes: `deploy.appservice.app` (app name), -`deploy.appservice.rg` (resource group), `deploy.appservice.linux` (boolean), and -`deploy.appservice.attempt` (1-indexed attempt number, updated per retry iteration). The span uses -named return `err` with `span.EndWithStatus(err)` to automatically record failure status. +`events.DeployAppServiceZipEvent` with system metadata attributes: `deploy.appservice.linux` +(boolean) and `deploy.appservice.attempt` (1-indexed attempt number, updated per retry iteration). +The span uses named return `err` with `span.EndWithStatus(err)` to automatically record failure +status. No customer data (app names, resource groups) is included in span attributes. -`IsScmReady()` on `ZipDeployClient` sends a lightweight GET to `/api/deployments`. Connection errors -return `(false, nil)` rather than propagating - the SCM is still restarting, which is the expected state. -The `//nolint:nilerr` directive documents this intentional error suppression for the linter. +`IsScmReady()` on `ZipDeployClient` sends a lightweight GET to `/api/deployments`. Known +transient transport errors (connection refused, DNS lookup failures, net.Error timeouts) return +`(false, nil)` — the SCM is still restarting, which is the expected state. Other transport errors +are propagated as `fmt.Errorf("SCM readiness probe: %w", err)` to avoid masking genuine issues. +Context cancellation is checked via `ctx.Err()` and returns immediately. ### 5. ACR Credential Exponential Backoff @@ -118,6 +123,11 @@ vs 60s worst case with the old constant 20s * 3 retries). The exponential curve 404s (DNS propagation, eventual consistency) resolve on the first or second retry, while the longer tail preserves roughly the same total retry window for slow DNS propagation edge cases. +Retries are limited to HTTP 404 (resource not yet available) and DNS resolution errors. HTTP 429 +(throttling) and 5xx (server errors) are NOT retried by this outer loop because the Azure SDK's +built-in retry policy already handles them — retrying at both layers would cause retry amplification +(outer retries × inner retries). + **Evidence:** Azure DNS propagation FAQ confirms changes "typically take effect within 60 seconds" but are often faster. The old link in the comment (`learn.microsoft.com/en-us/azure/dns/dns-faq#how-long-does-it-take-for-dns-changes-to-take-effect-`) @@ -137,7 +147,7 @@ custom `docker.path` pointing to a Dockerfile outside the service directory. **Solution:** Extracted `resolveDockerPaths()` that resolves docker.path and docker.context to absolute paths. Both user-specified and default paths are resolved relative to the service -directory (via `resolveServiceDir()`) to preserve backward compatibility.This centralizes path resolution that was +directory (via `resolveServiceDir()`). This centralizes path resolution that was previously duplicated across `Build()`, `runRemoteBuild()`, and other call sites. Called from `Build()`, `runRemoteBuild()`, and `useDotNetPublishForDockerBuild()` (which now calls @@ -159,21 +169,7 @@ closing it. This preserves compatibility with azcore's poller framework which ma lifecycle. Standalone callers (e.g., `federated_token_client.go`) use `defer res.Body.Close()` before calling `ReadRawResponse`. -### 8. UpdateAppServiceAppSetting Helper - -**File:** `pkg/azapi/webapp.go` - -**Problem:** No API existed to update a single App Service application setting without replacing the -entire set. Callers that needed to add or modify one setting had to manually read-modify-write the -full settings dictionary, duplicating boilerplate. - -**Solution:** Added `UpdateAppServiceAppSetting(ctx, subscriptionId, resourceGroup, appName, key, -value)` that reads existing settings via `ListApplicationSettings`, adds/overwrites the specified -key-value pair, and writes all settings back via `UpdateApplicationSettings`. This preserves other -settings that are not being modified. The function handles the nil-properties edge case by -initializing the map if empty. - -### 9. OTel Tracing Spans for Deployment Profiling +### 8. OTel Tracing Spans for Deployment Profiling **Files:** `pkg/azapi/standard_deployments.go`, `pkg/azapi/stack_deployments.go`, `pkg/project/container_helper.go`, `pkg/project/container_helper_test.go` @@ -182,10 +178,13 @@ initializing the map if empty. WhatIf operations, and container build/publish had no span coverage, making it impossible to profile where wall-clock time is spent during parallel deployment. -**Solution:** Added 11 OTel tracing spans to all performance-critical deployment paths using -the established pattern: `tracing.Start(ctx, spanName)` + named return `err` + `defer func() -{ span.EndWithStatus(err) }()`. Each span includes `attribute.String` for subscription, -resource group, and deployment name to enable correlation in trace viewers. +**Solution:** Added OTel tracing spans to all performance-critical deployment paths using +the established pattern: `tracing.Start(ctx, events.EventName)` + named return `err` + `defer func() +{ span.EndWithStatus(err) }()`. All span names are registered as constants in +`internal/tracing/events/events.go`. No customer data (app names, resource groups, subscription IDs, +deployment names) is included in span attributes — only system metadata like +`deploy.appservice.linux` (boolean) and `deploy.appservice.attempt` (retry count), registered in +`internal/tracing/fields/fields.go`. **ARM Standard Deployments** (`standard_deployments.go` — 6 spans): - `arm.deploy.subscription` — `DeployToSubscription` (ARM deploy at subscription scope) @@ -216,7 +215,7 @@ deployment: ARM provisioning (30-120s), WhatIf/Validate (30-90s), container buil 300s), and ACR credential retrieval (2-62s with backoff). Together with the existing `deploy.appservice.zip` span, they provide full end-to-end visibility for profiling `azd up`. -### 10. Supporting Changes +### 9. Supporting Changes **Files:** `.gitignore`, `cli/azd/.vscode/cspell-azd-dictionary.txt` @@ -225,7 +224,7 @@ deployment: ARM provisioning (30-120s), WhatIf/Validate (30-90s), container buil - `cspell-azd-dictionary.txt`: Added `keepalives` (HTTP keep-alive terminology in TunedTransport), `nilerr` (golangci-lint nolint directive in `IsScmReady`), `appsettings` (App Service terminology) -### 11. Container App Adaptive Poll Frequency +### 10. Container App Adaptive Poll Frequency **File:** `pkg/containerapps/container_app.go` @@ -248,7 +247,7 @@ benchmarks). The same 5s frequency used for ARM deploy operations (§3) balances (6x faster than the 30s default) against ARM rate limits. With 8 parallel services, this generates ~96 polls/min per operation type — well within the 1200 reads/5min/subscription limit. -### 12. ACR Login Caching per Registry +### 11. ACR Login Caching per Registry **File:** `pkg/azapi/container_registry.go` @@ -284,6 +283,6 @@ invocations. | Tracing context propagation | `pkg/project/container_helper_test.go` | Mock assertions updated for tracing context wrapping (`mock.Anything`) | The ARM client caching, TunedTransport wiring, poll frequency (both ARM and Container App), -SCM retry logic, ACR login caching, `UpdateAppServiceAppSetting`, and OTel tracing spans are +SCM retry logic, ACR login caching, and OTel tracing spans are infrastructure changes that operate through Azure SDK interactions and are validated through integration and end-to-end playback tests rather than isolated unit tests. From f96827204c30ff933da56485d47d6f1453bf727d Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Mon, 13 Apr 2026 16:09:50 -0700 Subject: [PATCH 08/12] fix cspell: replace fmt.Errorf with prose in spec.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/specs/perf-foundations/spec.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/specs/perf-foundations/spec.md b/docs/specs/perf-foundations/spec.md index c2c121b34e2..aed3966b16e 100644 --- a/docs/specs/perf-foundations/spec.md +++ b/docs/specs/perf-foundations/spec.md @@ -106,7 +106,7 @@ status. No customer data (app names, resource groups) is included in span attrib `IsScmReady()` on `ZipDeployClient` sends a lightweight GET to `/api/deployments`. Known transient transport errors (connection refused, DNS lookup failures, net.Error timeouts) return `(false, nil)` — the SCM is still restarting, which is the expected state. Other transport errors -are propagated as `fmt.Errorf("SCM readiness probe: %w", err)` to avoid masking genuine issues. +are propagated as wrapped errors (`"SCM readiness probe: ..."`) to avoid masking genuine issues. Context cancellation is checked via `ctx.Err()` and returns immediately. ### 5. ACR Credential Exponential Backoff From 439de753e317626c430054c94fa2e7796a8f1cc2 Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Mon, 13 Apr 2026 17:00:56 -0700 Subject: [PATCH 09/12] address wbreza re-review: add retry backoff, cache test, timeout docs - Add exponential backoff (5s, 10s) between zip deploy retries with context cancellation support - Add Test_StandardDeployments_ClientCaching with 3 subtests: cache hit returns same instance, different subscriptions isolate, concurrent access converges to single instance - Document 5-minute ACR login timeout rationale (AAD + ACR exchange + docker login typical/worst-case timings) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/azapi/container_registry.go | 8 ++- .../pkg/azapi/standard_deployments_test.go | 59 +++++++++++++++++++ cli/azd/pkg/azapi/webapp.go | 9 +++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/cli/azd/pkg/azapi/container_registry.go b/cli/azd/pkg/azapi/container_registry.go index 0240dad3a66..1d03b975b84 100644 --- a/cli/azd/pkg/azapi/container_registry.go +++ b/cli/azd/pkg/azapi/container_registry.go @@ -143,8 +143,12 @@ func (crs *containerRegistryService) Login(ctx context.Context, subscriptionId s } // Use context.WithoutCancel so the shared work isn't tied to a single - // caller's context. Add a bounded timeout so the shared login cannot - // hang indefinitely if Credentials or docker login gets stuck. + // caller's context. The 5-minute timeout is an upper bound that covers: + // - AAD token acquisition (~1-5s typical, up to 30s under load) + // - ACR token exchange via /oauth2/exchange (~1-3s typical) + // - docker login CLI invocation (~1-2s typical) + // Most logins complete in under 10s; 5 minutes provides generous headroom + // for transient AAD/ACR slowness without hanging indefinitely. const loginTimeout = 5 * time.Minute opCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), loginTimeout) defer cancel() diff --git a/cli/azd/pkg/azapi/standard_deployments_test.go b/cli/azd/pkg/azapi/standard_deployments_test.go index 9bbdc895048..0951c91f9f6 100644 --- a/cli/azd/pkg/azapi/standard_deployments_test.go +++ b/cli/azd/pkg/azapi/standard_deployments_test.go @@ -6,6 +6,7 @@ package azapi import ( "context" "sort" + "sync" "testing" "time" @@ -49,6 +50,64 @@ func Test_StandardDeployments_GenerateDeploymentName(t *testing.T) { } } +func Test_StandardDeployments_ClientCaching(t *testing.T) { + t.Parallel() + + mockContext := mocks.NewMockContext(context.Background()) + ds := NewStandardDeployments( + mockContext.SubscriptionCredentialProvider, + mockContext.ArmClientOptions, + NewResourceService(mockContext.SubscriptionCredentialProvider, mockContext.ArmClientOptions), + cloud.AzurePublic(), + mockContext.Clock, + ) + + ctx := *mockContext.Context + + t.Run("cache hit returns same instance", func(t *testing.T) { + client1, err := ds.createDeploymentsClient(ctx, "sub-1") + require.NoError(t, err) + + client2, err := ds.createDeploymentsClient(ctx, "sub-1") + require.NoError(t, err) + + // Same pointer — cache hit + assert.Same(t, client1, client2) + }) + + t.Run("different subscriptions return different clients", func(t *testing.T) { + clientA, err := ds.createDeploymentsClient(ctx, "sub-a") + require.NoError(t, err) + + clientB, err := ds.createDeploymentsClient(ctx, "sub-b") + require.NoError(t, err) + + assert.NotSame(t, clientA, clientB) + }) + + t.Run("concurrent access returns consistent client", func(t *testing.T) { + const goroutines = 10 + results := make([]*armresources.DeploymentsClient, goroutines) + errs := make([]error, goroutines) + + var wg sync.WaitGroup + wg.Add(goroutines) + for i := range goroutines { + go func() { + defer wg.Done() + results[i], errs[i] = ds.createDeploymentsClient(ctx, "sub-concurrent") + }() + } + wg.Wait() + + for i := range goroutines { + require.NoError(t, errs[i]) + // All goroutines must get the same cached instance + assert.Same(t, results[0], results[i], "goroutine %d got a different client", i) + } + }) +} + func TestResourceGroupsFromDeployment(t *testing.T) { t.Parallel() diff --git a/cli/azd/pkg/azapi/webapp.go b/cli/azd/pkg/azapi/webapp.go index 3dc475895fd..62f8eab156e 100644 --- a/cli/azd/pkg/azapi/webapp.go +++ b/cli/azd/pkg/azapi/webapp.go @@ -192,6 +192,15 @@ func (cli *AzureClient) DeployAppServiceZip( span.SetAttributes(fields.DeployAttemptKey.Key.Int(attempt + 1)) if attempt > 0 { + // Exponential backoff: 5s, 10s between retries to avoid hammering + // the SCM endpoint while it stabilizes. + retryDelay := time.Duration(attempt) * 5 * time.Second + select { + case <-time.After(retryDelay): + case <-ctx.Done(): + return nil, ctx.Err() + } + // Reset the zip file reader so the retry re-uploads the full content. if _, seekErr := deployZipFile.Seek(0, io.SeekStart); seekErr != nil { return nil, fmt.Errorf("resetting zip file for retry: %w", seekErr) From 9d450d2cb435e6f12beeed989f45cbdad1ef7286 Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Tue, 14 Apr 2026 15:37:17 -0700 Subject: [PATCH 10/12] address wbreza review: Docker context tests, variable rename - Add Test_InMemDockerfile_ContextOverride (4 subtests) proving the serviceConfig.Docker.Context check is correct after resolveDockerPaths - Rename defaultCredentialsRetryDelay to defaultCredentialsRetryInitialDelay to clarify it's the initial delay for exponential backoff Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/project/container_helper.go | 4 +- .../container_helper_coverage3_test.go | 109 ++++++++++++++++++ cli/azd/pkg/project/container_helper_test.go | 2 +- 3 files changed, 112 insertions(+), 3 deletions(-) diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index f8421635d03..3d11e30bb65 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -284,7 +284,7 @@ func (ch *ContainerHelper) Login( return registryName, nil } -var defaultCredentialsRetryDelay = 2 * time.Second +var defaultCredentialsRetryInitialDelay = 2 * time.Second func (ch *ContainerHelper) Credentials( ctx context.Context, @@ -307,7 +307,7 @@ func (ch *ContainerHelper) Credentials( // constant 20s delay. ACR credentials are typically available within seconds after // resource creation, so aggressive early retries resolve most 404s quickly while // preserving roughly the same total retry window for slow DNS propagation. - retry.WithMaxRetries(5, retry.NewExponential(defaultCredentialsRetryDelay)), + retry.WithMaxRetries(5, retry.NewExponential(defaultCredentialsRetryInitialDelay)), func(ctx context.Context) error { cred, err := ch.containerRegistryService.Credentials(ctx, targetResource.SubscriptionId(), loginServer) if err != nil { diff --git a/cli/azd/pkg/project/container_helper_coverage3_test.go b/cli/azd/pkg/project/container_helper_coverage3_test.go index 58c1e66c93e..dec5db9cd53 100644 --- a/cli/azd/pkg/project/container_helper_coverage3_test.go +++ b/cli/azd/pkg/project/container_helper_coverage3_test.go @@ -174,3 +174,112 @@ func Test_resolveDockerPaths(t *testing.T) { assert.Equal(t, projectPath, opts.Context) }) } + +// Test_InMemDockerfile_ContextOverride verifies that in-memory Dockerfile builds +// correctly override the build context to the temp directory when the user did +// not specify a custom Docker context (empty or "."). +// +// This tests the fix where we check serviceConfig.Docker.Context (original config) +// instead of dockerOptions.Context (already resolved to absolute by resolveDockerPaths). +// After resolution, dockerOptions.Context is an absolute path, so comparing against +// "" or "." would never match — checking the original config is intentional. +func Test_InMemDockerfile_ContextOverride(t *testing.T) { + projectPath := t.TempDir() + servicePath := filepath.Join(projectPath, "src", "web") + require.NoError(t, os.MkdirAll(servicePath, 0755)) + + t.Run("DefaultContext_OverriddenToTempDir", func(t *testing.T) { + // When Docker.Context is empty (default), the in-memory Dockerfile flow + // should override context to the temp dir where the Dockerfile is written. + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{}, // Context defaults to "" + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + // After resolution, opts.Context is an absolute path (service dir). + assert.Equal(t, servicePath, opts.Context, "precondition: context is resolved to service dir") + + // Simulate the in-memory Dockerfile override (container_helper.go ~line 478). + // This uses serviceConfig.Docker.Context (original config value) — NOT opts.Context. + tempDir := t.TempDir() + if svc.Docker.Context == "" || svc.Docker.Context == "." { + opts.Context = tempDir + } + + assert.Equal(t, tempDir, opts.Context, "context should be overridden to tempDir") + }) + + t.Run("DotContext_OverriddenToTempDir", func(t *testing.T) { + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{ + Context: ".", + }, + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + assert.Equal(t, servicePath, opts.Context, "precondition: '.' resolves to service dir") + + tempDir := t.TempDir() + if svc.Docker.Context == "" || svc.Docker.Context == "." { + opts.Context = tempDir + } + + assert.Equal(t, tempDir, opts.Context, "context should be overridden to tempDir") + }) + + t.Run("CustomContext_PreservedNotOverridden", func(t *testing.T) { + // When user specifies a custom context, the in-memory Dockerfile flow + // must NOT override it — the user's explicit context takes precedence. + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{ + Context: "custom/build-context", + }, + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + expectedContext := filepath.Join(servicePath, "custom", "build-context") + assert.Equal(t, expectedContext, opts.Context, "precondition: custom context resolved") + + tempDir := t.TempDir() + if svc.Docker.Context == "" || svc.Docker.Context == "." { + opts.Context = tempDir + } + + // Context should NOT be overridden — user specified a custom path. + assert.Equal(t, expectedContext, opts.Context, + "custom context must be preserved, not overridden to tempDir") + }) + + t.Run("BugRegression_CheckingResolvedContext_WouldNeverMatch", func(t *testing.T) { + // This test proves WHY we check serviceConfig.Docker.Context instead of + // dockerOptions.Context. After resolveDockerPaths(), opts.Context is always + // an absolute path — checking it against "" or "." would never match, + // meaning tempDir override would never happen. + svc := &ServiceConfig{ + Project: &ProjectConfig{Path: projectPath}, + RelativePath: filepath.Join("src", "web"), + Docker: DockerProjectOptions{}, // Context defaults to "" + } + opts := getDockerOptionsWithDefaults(svc.Docker) + resolveDockerPaths(svc, &opts) + + // Demonstrate the bug: checking resolved opts.Context against "" or "." never matches. + resolvedContextMatchesEmpty := opts.Context == "" || opts.Context == "." + assert.False(t, resolvedContextMatchesEmpty, + "resolved context is absolute — comparing against empty/dot never matches") + + // But the original config value DOES match. + originalConfigMatchesEmpty := svc.Docker.Context == "" || svc.Docker.Context == "." + assert.True(t, originalConfigMatchesEmpty, + "original config is empty — this is what we should check") + }) +} diff --git a/cli/azd/pkg/project/container_helper_test.go b/cli/azd/pkg/project/container_helper_test.go index 7d484979454..9136bda17a0 100644 --- a/cli/azd/pkg/project/container_helper_test.go +++ b/cli/azd/pkg/project/container_helper_test.go @@ -902,7 +902,7 @@ func Test_ContainerHelper_Credential_Retry(t *testing.T) { MaxRetry: 1, } // no need to delay in tests - defaultCredentialsRetryDelay = 1 * time.Millisecond + defaultCredentialsRetryInitialDelay = 1 * time.Millisecond containerHelper := NewContainerHelper( clock.NewMock(), mockContainerService, nil, nil, nil, nil, nil, cloud.AzurePublic()) From 0739cd44c5ad98143c6fe1c05d70a909e976b9aa Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Tue, 14 Apr 2026 16:18:17 -0700 Subject: [PATCH 11/12] remove ARM client caching per weikanglim feedback Remove sync.Map client caching from resource_service.go, standard_deployments.go, and stack_deployments.go. ARM client construction cost (<1ms) is negligible vs network I/O (seconds), and connection reuse is already handled by TunedTransport. Removes 5 sync.Map fields, 5 Load/LoadOrStore caching functions, Test_StandardDeployments_ClientCaching, and spec.md section 2. Renumber spec sections 3-11 to 2-10. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/azapi/resource_service.go | 24 +------- cli/azd/pkg/azapi/stack_deployments.go | 13 +--- cli/azd/pkg/azapi/standard_deployments.go | 25 +------- .../pkg/azapi/standard_deployments_test.go | 59 ------------------- docs/specs/perf-foundations/spec.md | 41 ++++--------- 5 files changed, 15 insertions(+), 147 deletions(-) diff --git a/cli/azd/pkg/azapi/resource_service.go b/cli/azd/pkg/azapi/resource_service.go index 41e6ceeed33..50a019d260e 100644 --- a/cli/azd/pkg/azapi/resource_service.go +++ b/cli/azd/pkg/azapi/resource_service.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "net/http" - "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -62,13 +61,6 @@ type ListResourceGroupResourcesOptions struct { type ResourceService struct { credentialProvider account.SubscriptionCredentialProvider armClientOptions *arm.ClientOptions - - // resourcesClients caches armresources.Client instances per subscription ID. - // Azure SDK ARM clients are safe for concurrent use. - resourcesClients sync.Map // map[string]*armresources.Client - - // resourceGroupClients caches ResourceGroupsClient instances per subscription ID. - resourceGroupClients sync.Map // map[string]*armresources.ResourceGroupsClient } func NewResourceService( @@ -352,10 +344,6 @@ func (rs *ResourceService) GetResourceGroup( } func (rs *ResourceService) createResourcesClient(ctx context.Context, subscriptionId string) (*armresources.Client, error) { - if cached, ok := rs.resourcesClients.Load(subscriptionId); ok { - return cached.(*armresources.Client), nil - } - credential, err := rs.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -366,19 +354,13 @@ func (rs *ResourceService) createResourcesClient(ctx context.Context, subscripti return nil, fmt.Errorf("creating Resource client: %w", err) } - // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. - actual, _ := rs.resourcesClients.LoadOrStore(subscriptionId, client) - return actual.(*armresources.Client), nil + return client, nil } func (rs *ResourceService) createResourceGroupClient( ctx context.Context, subscriptionId string, ) (*armresources.ResourceGroupsClient, error) { - if cached, ok := rs.resourceGroupClients.Load(subscriptionId); ok { - return cached.(*armresources.ResourceGroupsClient), nil - } - credential, err := rs.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -389,9 +371,7 @@ func (rs *ResourceService) createResourceGroupClient( return nil, fmt.Errorf("creating ResourceGroup client: %w", err) } - // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. - actual, _ := rs.resourceGroupClients.LoadOrStore(subscriptionId, client) - return actual.(*armresources.ResourceGroupsClient), nil + return client, nil } // GroupByResourceGroup creates a map of resources group by their resource group name. diff --git a/cli/azd/pkg/azapi/stack_deployments.go b/cli/azd/pkg/azapi/stack_deployments.go index 46c5fe86cd2..f0ff996b940 100644 --- a/cli/azd/pkg/azapi/stack_deployments.go +++ b/cli/azd/pkg/azapi/stack_deployments.go @@ -13,7 +13,6 @@ import ( "os" "path/filepath" "strconv" - "sync" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -61,10 +60,6 @@ type StackDeployments struct { armClientOptions *arm.ClientOptions standardDeployments *StandardDeployments cloud *cloud.Cloud - - // stackClients caches deployment stacks Client instances per subscription ID. - // Azure SDK ARM clients are safe for concurrent use. - stackClients sync.Map // map[string]*armdeploymentstacks.Client } type deploymentStackOptions struct { @@ -683,10 +678,6 @@ func (d *StackDeployments) CalculateTemplateHash( } func (d *StackDeployments) createClient(ctx context.Context, subscriptionId string) (*armdeploymentstacks.Client, error) { - if cached, ok := d.stackClients.Load(subscriptionId); ok { - return cached.(*armdeploymentstacks.Client), nil - } - credential, err := d.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -697,9 +688,7 @@ func (d *StackDeployments) createClient(ctx context.Context, subscriptionId stri return nil, fmt.Errorf("creating deployment stacks client: %w", err) } - // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. - actual, _ := d.stackClients.LoadOrStore(subscriptionId, client) - return actual.(*armdeploymentstacks.Client), nil + return client, nil } // Converts from an ARM Extended Deployment to Azd Generic deployment diff --git a/cli/azd/pkg/azapi/standard_deployments.go b/cli/azd/pkg/azapi/standard_deployments.go index f169a9a5a2c..1ea6863a2e0 100644 --- a/cli/azd/pkg/azapi/standard_deployments.go +++ b/cli/azd/pkg/azapi/standard_deployments.go @@ -11,7 +11,6 @@ import ( "maps" "net/url" "slices" - "sync" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -55,14 +54,6 @@ type StandardDeployments struct { resourceService *ResourceService cloud *cloud.Cloud clock clock.Clock - - // deploymentsClients caches DeploymentsClient instances per subscription ID. - // Azure SDK ARM clients are safe for concurrent use and hold no per-request state, - // so reusing them avoids redundant pipeline construction on every API call. - deploymentsClients sync.Map // map[string]*armresources.DeploymentsClient - - // deploymentsOpsClients caches DeploymentOperationsClient instances per subscription ID. - deploymentsOpsClients sync.Map // map[string]*armresources.DeploymentOperationsClient } func NewStandardDeployments( @@ -211,10 +202,6 @@ func (ds *StandardDeployments) createDeploymentsClient( ctx context.Context, subscriptionId string, ) (*armresources.DeploymentsClient, error) { - if cached, ok := ds.deploymentsClients.Load(subscriptionId); ok { - return cached.(*armresources.DeploymentsClient), nil - } - credential, err := ds.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -225,9 +212,7 @@ func (ds *StandardDeployments) createDeploymentsClient( return nil, fmt.Errorf("creating deployments client: %w", err) } - // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. - actual, _ := ds.deploymentsClients.LoadOrStore(subscriptionId, client) - return actual.(*armresources.DeploymentsClient), nil + return client, nil } func (ds *StandardDeployments) DeployToSubscription( @@ -676,10 +661,6 @@ func (ds *StandardDeployments) createDeploymentsOperationsClient( ctx context.Context, subscriptionId string, ) (*armresources.DeploymentOperationsClient, error) { - if cached, ok := ds.deploymentsOpsClients.Load(subscriptionId); ok { - return cached.(*armresources.DeploymentOperationsClient), nil - } - credential, err := ds.credentialProvider.CredentialForSubscription(ctx, subscriptionId) if err != nil { return nil, err @@ -690,9 +671,7 @@ func (ds *StandardDeployments) createDeploymentsOperationsClient( return nil, fmt.Errorf("creating deployments client: %w", err) } - // Benign race: concurrent miss creates an extra client; LoadOrStore ensures one winner. - actual, _ := ds.deploymentsOpsClients.LoadOrStore(subscriptionId, client) - return actual.(*armresources.DeploymentOperationsClient), nil + return client, nil } // Converts from an ARM Extended Deployment to Azd Generic deployment diff --git a/cli/azd/pkg/azapi/standard_deployments_test.go b/cli/azd/pkg/azapi/standard_deployments_test.go index 0951c91f9f6..9bbdc895048 100644 --- a/cli/azd/pkg/azapi/standard_deployments_test.go +++ b/cli/azd/pkg/azapi/standard_deployments_test.go @@ -6,7 +6,6 @@ package azapi import ( "context" "sort" - "sync" "testing" "time" @@ -50,64 +49,6 @@ func Test_StandardDeployments_GenerateDeploymentName(t *testing.T) { } } -func Test_StandardDeployments_ClientCaching(t *testing.T) { - t.Parallel() - - mockContext := mocks.NewMockContext(context.Background()) - ds := NewStandardDeployments( - mockContext.SubscriptionCredentialProvider, - mockContext.ArmClientOptions, - NewResourceService(mockContext.SubscriptionCredentialProvider, mockContext.ArmClientOptions), - cloud.AzurePublic(), - mockContext.Clock, - ) - - ctx := *mockContext.Context - - t.Run("cache hit returns same instance", func(t *testing.T) { - client1, err := ds.createDeploymentsClient(ctx, "sub-1") - require.NoError(t, err) - - client2, err := ds.createDeploymentsClient(ctx, "sub-1") - require.NoError(t, err) - - // Same pointer — cache hit - assert.Same(t, client1, client2) - }) - - t.Run("different subscriptions return different clients", func(t *testing.T) { - clientA, err := ds.createDeploymentsClient(ctx, "sub-a") - require.NoError(t, err) - - clientB, err := ds.createDeploymentsClient(ctx, "sub-b") - require.NoError(t, err) - - assert.NotSame(t, clientA, clientB) - }) - - t.Run("concurrent access returns consistent client", func(t *testing.T) { - const goroutines = 10 - results := make([]*armresources.DeploymentsClient, goroutines) - errs := make([]error, goroutines) - - var wg sync.WaitGroup - wg.Add(goroutines) - for i := range goroutines { - go func() { - defer wg.Done() - results[i], errs[i] = ds.createDeploymentsClient(ctx, "sub-concurrent") - }() - } - wg.Wait() - - for i := range goroutines { - require.NoError(t, errs[i]) - // All goroutines must get the same cached instance - assert.Same(t, results[0], results[i], "goroutine %d got a different client", i) - } - }) -} - func TestResourceGroupsFromDeployment(t *testing.T) { t.Parallel() diff --git a/docs/specs/perf-foundations/spec.md b/docs/specs/perf-foundations/spec.md index aed3966b16e..b22918d5ef0 100644 --- a/docs/specs/perf-foundations/spec.md +++ b/docs/specs/perf-foundations/spec.md @@ -35,28 +35,7 @@ burst are retained for subsequent requests to the same host. The 30s idle timeou **Wiring:** `cmd/deps.go` replaces `http.DefaultClient` with `&http.Client{Transport: TunedTransport()}` so all SDK clients created through dependency injection inherit the tuned transport. -### 2. ARM Client Caching (sync.Map) - -**Files:** `pkg/azapi/resource_service.go`, `pkg/azapi/standard_deployments.go`, -`pkg/azapi/stack_deployments.go` - -**Problem:** ARM SDK clients (`armresources.Client`, `DeploymentsClient`, `DeploymentOperationsClient`, -`armdeploymentstacks.Client`) were re-created on every API call. Each construction builds an HTTP -pipeline (retry policy, logging policy, auth policy). While not as expensive as a TLS handshake, this -adds unnecessary CPU and allocation overhead when the same subscription is used repeatedly. - -**Solution:** Cache clients in `sync.Map` fields keyed by subscription ID. The pattern uses -`Load` for the fast path and `LoadOrStore` on miss. The "benign race" comment documents that concurrent -cache misses may create duplicate clients; `LoadOrStore` ensures only one is retained. This is safe -because Azure SDK ARM clients are stateless and goroutine-safe (they hold only the pipeline -configuration, not per-request state). - -**Evidence:** Azure SDK for Go documentation confirms ARM clients are safe for concurrent use: -"A client is safe for concurrent use across goroutines" (azure-sdk-for-go design guidelines). `sync.Map` -is the standard Go pattern for read-heavy caches with infrequent writes, avoiding lock contention on the -hot path. - -### 3. Adaptive Poll Frequency +### 2. Adaptive Poll Frequency **Files:** `pkg/azapi/standard_deployments.go`, `pkg/azapi/stack_deployments.go` @@ -78,7 +57,7 @@ leaving substantial headroom for other ARM reads (list operations, status checks limits are documented at `learn.microsoft.com/en-us/azure/azure-resource-manager/management/request-limits-and-throttling`. -### 4. Zip Deploy Retry with SCM Readiness Probe +### 3. Zip Deploy Retry with SCM Readiness Probe **Files:** `pkg/azapi/webapp.go`, `pkg/azsdk/zip_deploy_client.go` @@ -109,7 +88,7 @@ transient transport errors (connection refused, DNS lookup failures, net.Error t are propagated as wrapped errors (`"SCM readiness probe: ..."`) to avoid masking genuine issues. Context cancellation is checked via `ctx.Err()` and returns immediately. -### 5. ACR Credential Exponential Backoff +### 4. ACR Credential Exponential Backoff **File:** `pkg/project/container_helper.go` @@ -133,7 +112,7 @@ are often faster. The old link in the comment (`learn.microsoft.com/en-us/azure/dns/dns-faq#how-long-does-it-take-for-dns-changes-to-take-effect-`) documents worst-case TTL, not typical latency. -### 6. Docker Path Resolution Fix +### 5. Docker Path Resolution Fix **Files:** `pkg/project/container_helper.go`, `pkg/project/framework_service_docker.go`, `pkg/project/framework_service_docker_test.go` @@ -156,7 +135,7 @@ absolute resolved paths, including dynamic resolution in the table-driven `Test_ test. `filepath.Clean` is applied to both resolved paths to normalize separators and prevent path traversal. -### 7. ReadRawResponse Body Ownership +### 6. ReadRawResponse Body Ownership **File:** `pkg/httputil/util.go` @@ -169,7 +148,7 @@ closing it. This preserves compatibility with azcore's poller framework which ma lifecycle. Standalone callers (e.g., `federated_token_client.go`) use `defer res.Body.Close()` before calling `ReadRawResponse`. -### 8. OTel Tracing Spans for Deployment Profiling +### 7. OTel Tracing Spans for Deployment Profiling **Files:** `pkg/azapi/standard_deployments.go`, `pkg/azapi/stack_deployments.go`, `pkg/project/container_helper.go`, `pkg/project/container_helper_test.go` @@ -215,7 +194,7 @@ deployment: ARM provisioning (30-120s), WhatIf/Validate (30-90s), container buil 300s), and ACR credential retrieval (2-62s with backoff). Together with the existing `deploy.appservice.zip` span, they provide full end-to-end visibility for profiling `azd up`. -### 9. Supporting Changes +### 8. Supporting Changes **Files:** `.gitignore`, `cli/azd/.vscode/cspell-azd-dictionary.txt` @@ -224,7 +203,7 @@ deployment: ARM provisioning (30-120s), WhatIf/Validate (30-90s), container buil - `cspell-azd-dictionary.txt`: Added `keepalives` (HTTP keep-alive terminology in TunedTransport), `nilerr` (golangci-lint nolint directive in `IsScmReady`), `appsettings` (App Service terminology) -### 10. Container App Adaptive Poll Frequency +### 9. Container App Adaptive Poll Frequency **File:** `pkg/containerapps/container_app.go` @@ -247,7 +226,7 @@ benchmarks). The same 5s frequency used for ARM deploy operations (§3) balances (6x faster than the 30s default) against ARM rate limits. With 8 parallel services, this generates ~96 polls/min per operation type — well within the 1200 reads/5min/subscription limit. -### 11. ACR Login Caching per Registry +### 10. ACR Login Caching per Registry **File:** `pkg/azapi/container_registry.go` @@ -282,7 +261,7 @@ invocations. | Docker build args | `pkg/project/framework_service_docker_test.go` | Table-driven tests resolve expected paths to match production behavior | | Tracing context propagation | `pkg/project/container_helper_test.go` | Mock assertions updated for tracing context wrapping (`mock.Anything`) | -The ARM client caching, TunedTransport wiring, poll frequency (both ARM and Container App), +The TunedTransport wiring, poll frequency (both ARM and Container App), SCM retry logic, ACR login caching, and OTel tracing spans are infrastructure changes that operate through Azure SDK interactions and are validated through integration and end-to-end playback tests rather than isolated unit tests. From 9886167b21b7c9b9fabebe1781fa977489af7904 Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Wed, 15 Apr 2026 11:00:39 -0700 Subject: [PATCH 12/12] address wbreza review round 2: ACR login tests, IsScmReady tests, body close fix - Add 7 table-driven tests for Login() singleflight/cache logic: concurrent dedup, cache hit, context cancellation isolation, error propagation, multi-registry independence - Add 10 table-driven tests for IsScmReady error classification: HTTP status codes, connection refused, no such host, net.Error timeout, context cancellation, unknown transport errors - Fix pre-existing response body leak in getAcrToken(): add defer response.Body.Close() after pipeline.Do() - Update Login() interface doc to reflect idempotent/singleflight deduplication semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/azapi/container_registry.go | 5 +- .../azapi/container_registry_login_test.go | 268 ++++++++++++++++++ cli/azd/pkg/azsdk/zip_deploy_client_test.go | 185 ++++++++++++ 3 files changed, 457 insertions(+), 1 deletion(-) create mode 100644 cli/azd/pkg/azapi/container_registry_login_test.go diff --git a/cli/azd/pkg/azapi/container_registry.go b/cli/azd/pkg/azapi/container_registry.go index 1d03b975b84..58722790e40 100644 --- a/cli/azd/pkg/azapi/container_registry.go +++ b/cli/azd/pkg/azapi/container_registry.go @@ -47,7 +47,9 @@ type acrToken struct { // ContainerRegistryService provides access to query and login to Azure Container Registries (ACR) type ContainerRegistryService interface { - // Logs into the specified container registry + // Login authenticates to the specified container registry. The call is + // idempotent: concurrent callers for the same registry are deduplicated + // via singleflight, and subsequent calls return immediately from cache. Login(ctx context.Context, subscriptionId string, loginServer string) error // Gets the credentials that could be used to login to the specified container registry. Credentials(ctx context.Context, subscriptionId string, loginServer string) (*DockerCredentials, error) @@ -357,6 +359,7 @@ func (crs *containerRegistryService) getAcrToken( if err != nil { return nil, err } + defer response.Body.Close() if !azruntime.HasStatusCode(response, http.StatusOK) { return nil, azruntime.NewResponseError(response) diff --git a/cli/azd/pkg/azapi/container_registry_login_test.go b/cli/azd/pkg/azapi/container_registry_login_test.go new file mode 100644 index 00000000000..c4d129303c8 --- /dev/null +++ b/cli/azd/pkg/azapi/container_registry_login_test.go @@ -0,0 +1,268 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package azapi + +import ( + "context" + "fmt" + "net/http" + "strings" + "sync" + "sync/atomic" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/azure/azure-dev/cli/azd/pkg/exec" + "github.com/azure/azure-dev/cli/azd/pkg/tools/docker" + "github.com/azure/azure-dev/cli/azd/test/mocks" + "github.com/azure/azure-dev/cli/azd/test/mocks/mockaccount" + "github.com/stretchr/testify/require" +) + +// setupLoginTest creates a containerRegistryService with mocked +// dependencies and returns atomic counters tracking how many times +// the ACR token exchange and docker login were invoked. +func setupLoginTest(t *testing.T) ( + *containerRegistryService, + *mocks.MockContext, + *atomic.Int32, + *atomic.Int32, +) { + t.Helper() + mockCtx := mocks.NewMockContext(t.Context()) + + var credCalls, dockerCalls atomic.Int32 + + // Mock ACR token exchange (Credentials → getAcrToken path). + mockCtx.HttpClient.When(func(r *http.Request) bool { + return r.Method == http.MethodPost && + strings.Contains(r.URL.Path, "oauth2/exchange") + }).RespondFn(func(r *http.Request) (*http.Response, error) { + credCalls.Add(1) + return acrTokenResponse(r) + }) + + // Mock docker login command. + mockCtx.CommandRunner.When(func(_ exec.RunArgs, cmd string) bool { + return strings.Contains(cmd, "docker login") + }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + dockerCalls.Add(1) + return exec.RunResult{}, nil + }) + + svc := newLoginTestService(t, mockCtx) + return svc, mockCtx, &credCalls, &dockerCalls +} + +// newLoginTestService builds a *containerRegistryService backed by +// the provided MockContext's HTTP transport and command runner. +func newLoginTestService( + t *testing.T, mc *mocks.MockContext, +) *containerRegistryService { + t.Helper() + svc := NewContainerRegistryService( + mockaccount.SubscriptionCredentialProviderFunc( + func(_ context.Context, _ string) (azcore.TokenCredential, error) { + return mc.Credentials, nil + }), + docker.NewCli(mc.CommandRunner), + mc.ArmClientOptions, + mc.CoreClientOptions, + ) + return svc.(*containerRegistryService) +} + +// acrTokenResponse returns a minimal successful ACR token exchange. +func acrTokenResponse( + r *http.Request, +) (*http.Response, error) { + body := struct { + RefreshToken string `json:"refresh_token"` + }{RefreshToken: "mock-refresh-token"} + return mocks.CreateHttpResponseWithBody( + r, http.StatusOK, body, + ) +} + +func TestLogin(t *testing.T) { + t.Run("SingleLoginSucceeds", func(t *testing.T) { + svc, _, credCalls, dockerCalls := setupLoginTest(t) + + err := svc.Login(t.Context(), "sub1", "reg.azurecr.io") + + require.NoError(t, err) + require.Equal(t, int32(1), credCalls.Load()) + require.Equal(t, int32(1), dockerCalls.Load()) + }) + + t.Run("CacheHitSkipsLogin", func(t *testing.T) { + svc, _, credCalls, dockerCalls := setupLoginTest(t) + svc.loginDone.Store("sub1:reg.azurecr.io", true) + + err := svc.Login(t.Context(), "sub1", "reg.azurecr.io") + + require.NoError(t, err) + require.Equal(t, int32(0), credCalls.Load(), + "credentials should not be fetched for cached login") + require.Equal(t, int32(0), dockerCalls.Load(), + "docker login should not be called for cached login") + }) + + t.Run("ConcurrentCallersDeduplicate", func(t *testing.T) { + svc, _, credCalls, dockerCalls := setupLoginTest(t) + + const n = 10 + errs := make([]error, n) + var barrier, done sync.WaitGroup + barrier.Add(n) + done.Add(n) + + for i := range n { + go func() { + defer done.Done() + barrier.Done() + barrier.Wait() // all goroutines start ~simultaneously + errs[i] = svc.Login( + t.Context(), "sub1", "reg.azurecr.io", + ) + }() + } + + done.Wait() + + for i, err := range errs { + require.NoError(t, err, "caller %d should succeed", i) + } + require.Equal(t, int32(1), credCalls.Load(), + "credentials should be fetched exactly once") + require.Equal(t, int32(1), dockerCalls.Load(), + "docker login should be called exactly once") + }) + + t.Run("CancelledContextDoesNotFailOthers", func(t *testing.T) { + mc := mocks.NewMockContext(t.Context()) + gate := make(chan struct{}) + entered := make(chan struct{}, 1) + + // HTTP mock blocks until the test releases the gate. + mc.HttpClient.When(func(r *http.Request) bool { + return r.Method == http.MethodPost && + strings.Contains(r.URL.Path, "oauth2/exchange") + }).RespondFn(func(r *http.Request) (*http.Response, error) { + select { + case entered <- struct{}{}: + default: + } + <-gate + return acrTokenResponse(r) + }) + + mc.CommandRunner.When(func(_ exec.RunArgs, cmd string) bool { + return strings.Contains(cmd, "docker login") + }).Respond(exec.RunResult{}) + + svc := newLoginTestService(t, mc) + cancelCtx, cancel := context.WithCancel(t.Context()) + defer cancel() + + var err1, err2 error + var g1, g2 sync.WaitGroup + g1.Add(1) + g2.Add(1) + + go func() { + defer g1.Done() + err1 = svc.Login( + cancelCtx, "sub1", "reg.azurecr.io", + ) + }() + go func() { + defer g2.Done() + err2 = svc.Login( + t.Context(), "sub1", "reg.azurecr.io", + ) + }() + + // Wait for the singleflight function to start, then cancel + // the first caller. Because Login uses WithoutCancel, the + // shared work continues for the second caller. + <-entered + cancel() + g1.Wait() // cancelled caller must have returned + close(gate) // release the shared work + g2.Wait() + + require.ErrorIs(t, err1, context.Canceled) + require.NoError(t, err2) + + // The work completed despite the cancellation. + _, ok := svc.loginDone.Load("sub1:reg.azurecr.io") + require.True(t, ok, + "login should complete despite caller cancellation") + }) + + t.Run("LoginDonePopulatedAfterSuccess", func(t *testing.T) { + svc, _, _, _ := setupLoginTest(t) + + err := svc.Login(t.Context(), "sub1", "reg.azurecr.io") + require.NoError(t, err) + + val, ok := svc.loginDone.Load("sub1:reg.azurecr.io") + require.True(t, ok, + "loginDone should have entry after successful login") + require.Equal(t, true, val) + }) + + t.Run("LoginErrorPropagated", func(t *testing.T) { + mc := mocks.NewMockContext(t.Context()) + + mc.HttpClient.When(func(r *http.Request) bool { + return r.Method == http.MethodPost && + strings.Contains(r.URL.Path, "oauth2/exchange") + }).RespondFn(func(r *http.Request) (*http.Response, error) { + return acrTokenResponse(r) + }) + + mc.CommandRunner.When(func(_ exec.RunArgs, cmd string) bool { + return strings.Contains(cmd, "docker login") + }).RespondFn(func(_ exec.RunArgs) (exec.RunResult, error) { + return exec.RunResult{}, + fmt.Errorf("docker daemon not running") + }) + + svc := newLoginTestService(t, mc) + + err := svc.Login(t.Context(), "sub1", "reg.azurecr.io") + + require.Error(t, err) + require.Contains(t, err.Error(), + "failed logging into docker registry") + require.Contains(t, err.Error(), + "docker daemon not running") + + // loginDone must NOT be populated on failure. + _, ok := svc.loginDone.Load("sub1:reg.azurecr.io") + require.False(t, ok, + "loginDone should not have entry after failed login") + }) + + t.Run("DifferentRegistriesDontShareCache", func(t *testing.T) { + svc, _, credCalls, dockerCalls := setupLoginTest(t) + + err := svc.Login( + t.Context(), "sub1", "registryA.azurecr.io", + ) + require.NoError(t, err) + require.Equal(t, int32(1), credCalls.Load()) + + err = svc.Login( + t.Context(), "sub1", "registryB.azurecr.io", + ) + require.NoError(t, err) + require.Equal(t, int32(2), credCalls.Load(), + "second registry should trigger new credential fetch") + require.Equal(t, int32(2), dockerCalls.Load(), + "second registry should trigger new docker login") + }) +} diff --git a/cli/azd/pkg/azsdk/zip_deploy_client_test.go b/cli/azd/pkg/azsdk/zip_deploy_client_test.go index 9ec61a60a4f..e9381464239 100644 --- a/cli/azd/pkg/azsdk/zip_deploy_client_test.go +++ b/cli/azd/pkg/azsdk/zip_deploy_client_test.go @@ -6,11 +6,14 @@ package azsdk import ( "bytes" "context" + "fmt" + "net" "net/http" "strings" "testing" "time" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" "github.com/azure/azure-dev/cli/azd/test/mocks" "github.com/stretchr/testify/require" @@ -165,3 +168,185 @@ func registerPollingErrorMocks(mockContext *mocks.MockContext) { return mocks.CreateHttpResponseWithBody(request, http.StatusBadRequest, errorStatus) }) } + +// scmTransportFunc adapts a function to policy.Transporter for testing. +type scmTransportFunc func(*http.Request) (*http.Response, error) + +func (f scmTransportFunc) Do(req *http.Request) (*http.Response, error) { + return f(req) +} + +// scmTimeoutError satisfies net.Error with Timeout() returning true. +type scmTimeoutError struct{ msg string } + +var _ net.Error = (*scmTimeoutError)(nil) + +func (e *scmTimeoutError) Error() string { return e.msg } +func (e *scmTimeoutError) Timeout() bool { return true } +func (e *scmTimeoutError) Temporary() bool { return false } + +func newTestScmClient( + transport policy.Transporter, +) *ZipDeployClient { + pipeline := runtime.NewPipeline( + "test", "1.0.0", + runtime.PipelineOptions{}, + &policy.ClientOptions{ + Transport: transport, + Retry: policy.RetryOptions{ + MaxRetries: 1, + RetryDelay: time.Nanosecond, + MaxRetryDelay: time.Nanosecond, + }, + }, + ) + return &ZipDeployClient{ + hostName: "test.scm.azurewebsites.net", + pipeline: pipeline, + } +} + +func TestIsScmReady(t *testing.T) { + tests := []struct { + name string + transport scmTransportFunc + ctx func(*testing.T) context.Context + wantReady bool + wantErr error + wantErrContains string + }{ + { + name: "HTTP200_Ready", + transport: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{}, + Request: req, + Body: http.NoBody, + }, nil + }, + wantReady: true, + }, + { + name: "HTTP502_BadGateway", + transport: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusBadGateway, + Header: http.Header{}, + Request: req, + Body: http.NoBody, + }, nil + }, + wantReady: false, + }, + { + name: "HTTP503_ServiceUnavailable", + transport: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusServiceUnavailable, + Header: http.Header{}, + Request: req, + Body: http.NoBody, + }, nil + }, + wantReady: false, + }, + { + name: "HTTP500_InternalServerError", + transport: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusInternalServerError, + Header: http.Header{}, + Request: req, + Body: http.NoBody, + }, nil + }, + wantReady: false, + wantErrContains: "500", + }, + { + name: "ConnectionRefused", + transport: func(req *http.Request) (*http.Response, error) { + return nil, fmt.Errorf("dial tcp: connection refused") + }, + wantReady: false, + }, + { + name: "NoSuchHost", + transport: func(req *http.Request) (*http.Response, error) { + return nil, fmt.Errorf( + "dial tcp: lookup host: no such host", + ) + }, + wantReady: false, + }, + { + name: "NetTimeout", + transport: func(req *http.Request) (*http.Response, error) { + return nil, &scmTimeoutError{msg: "i/o timeout"} + }, + wantReady: false, + }, + { + name: "ContextCanceled", + transport: func(req *http.Request) (*http.Response, error) { + return nil, context.Canceled + }, + ctx: func(t *testing.T) context.Context { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + return ctx + }, + wantReady: false, + wantErr: context.Canceled, + }, + { + name: "ContextDeadlineExceeded", + transport: func(req *http.Request) (*http.Response, error) { + return nil, context.DeadlineExceeded + }, + ctx: func(t *testing.T) context.Context { + ctx, cancel := context.WithDeadline( + t.Context(), + time.Now().Add(-time.Second), + ) + t.Cleanup(cancel) + return ctx + }, + wantReady: false, + wantErr: context.DeadlineExceeded, + }, + { + name: "UnknownTransportError_TLS", + transport: func(req *http.Request) (*http.Response, error) { + return nil, fmt.Errorf("tls: handshake failure") + }, + wantReady: false, + wantErrContains: "SCM readiness probe", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := newTestScmClient(tt.transport) + + ctx := t.Context() + if tt.ctx != nil { + ctx = tt.ctx(t) + } + + ready, err := client.IsScmReady(ctx) + require.Equal(t, tt.wantReady, ready) + + switch { + case tt.wantErr != nil: + require.ErrorIs(t, err, tt.wantErr) + case tt.wantErrContains != "": + require.Error(t, err) + require.Contains(t, err.Error(), tt.wantErrContains) + default: + require.NoError(t, err) + } + }) + } +}