[Codex] azure/azure-dev#7776 — feat(exegraph): graph-driven execution engine for up/provision/deploy#7
Open
IanMatthewHuff wants to merge 62 commits into
Conversation
Add `pkg/exegraph` — a DAG execution engine — and run `azd up`, `azd provision`, and `azd deploy` on it. User-authored `workflows.up:` continues to run on `workflow.Runner` (phase-scoped DAGs still apply inside each sub-command it invokes). - `Graph` / `Step` with three-color DFS cycle detection and transitive-dependent priority. - Goroutine-per-ready-node scheduler; `FailFast` and `ContinueOnError` policies; uniform per-step timeout via `RunOptions.StepTimeout`. - Cancellation classification distinguishes parent-cancel (skip) from per-step timeout (failure). - Adaptive polling with exponential backoff and ARM 429 throttle detection for Bicep deployments. - Kahn-level resolution for multi-layer provision. - `azd up`, `azd provision`, and `azd deploy` build a graph and dispatch through the scheduler. Single-layer provision and preview run as single-node graphs (uniform code path, trivial overhead). - Project command hooks (`preprovision`, `postprovision`, `predeploy`, `postdeploy`) run as synthetic `cmdhook-*` nodes inside the graph. Middleware fires only `preup`/`postup` for `azd up`, so hooks never double-fire. - Shared helpers: `service_graph.go` builds service nodes for deploy and up; `project_hooks.go` builds the `cmdhook-*` nodes. - Multi-layer provision initializes the provider once up front so subscription/location prompts serialize before any fan-out. - Deploy timeout resolution (`--timeout`, `AZD_DEPLOY_TIMEOUT`, default) is shared between `azd deploy` and `azd up`. - `DeadlineExceeded` surfaces a dedicated timeout UX. - `--output json` skips the live deploy tracker to keep JSON clean. New environment variables (all optional, all clamp to 64): - `AZD_PROVISION_CONCURRENCY` — worker cap for multi-layer provision. - `AZD_DEPLOY_CONCURRENCY` — worker cap for deploy graph. - `AZD_UP_CONCURRENCY` — worker cap for the unified up graph. - `AZD_DEPLOY_TIMEOUT` — per-service deploy timeout in seconds (precedence: `--timeout` flag > env var > default 1200s). - OpenTelemetry span per run (`exegraph.Run`) and per step (`exegraph.Step`) with stable attribute keys (step count, max concurrency, error policy, step name/deps/tags/timeout) defined in `internal/tracing/fields`. - `runProvisionSingleLayer` guards shared state with scoped closures and `defer` so panics can't leave locks held. - `errPreflightAbortedByUser` translates to `ErrAbortedByUser` for correct cancellation exit. - `ServiceConfig` hook registration lives behind a pointer-held guard struct so value copies are safe (`go vet copylocks` clean). - `docs/specs/exegraph/spec.md` describes the engine, the unified pipeline, env vars, and the semantic differences from the prior sequential model. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- .vscode/cspell.misc.yaml: add 'stdlib' override for docs/specs/exegraph/**
so cspell-misc stops flagging the spec. Mirrors the existing
perf-foundations override.
- cli/azd/magefile.go: populate excludedPlaybackTests map.
- 8 tests with pre-existing stale recordings (also fail on main).
- 2 tests (Test_DeploymentStacks, Test_CLI_ProvisionState) that need
re-recording because the graph-driven up/provision path introduced
new HTTP interactions (calculateTemplateHash layer hash probes and
resource-group existence checks). Must be re-recorded with live
Azure credentials before merge; documented in the map with notes.
With both changes, 'mage preflight' passes all 9 checks locally.
- layer_deps.go: restructure phase-1 loop to iterate `layers` directly
so static analyzers (gosec G602) see bounded indexing; add defensive
bounds check on `prev` in the duplicate-output error path.
- bicep.go: document the read-only, developer-controlled file-hashing
flow in `hashBicepFileTree` and silence gosec G304 with a targeted
nolint directive.
- bicep_cache_test.go: fix `string(rune('0'+callCount))` which
silently breaks when callCount > 9; use `fmt.Sprintf("%d", n)`.
- magefile.go: add `Test_CLI_InfraCreateAndDeleteUpperCase` to
excludedPlaybackTests (same calculateTemplateHash recording debt
as the two other feat/exegraph entries; must be re-recorded in
record mode before merge).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- layer_deps: keep intra-graph producer edges even when env var is pre-set (fixes stale-value race on template changes; edges were dropped by the LookupEnv early-continue) - up_graph: route errors through shared wrapProvisionError helper so the unified \�zd up\ path gets full parity with stand-alone \�zd provision\: JSON state dump on failure, OpenAI access suggestion, AI quota suggestion, Responsible AI suggestion. Extracted to package-level function taking provisionErrorDeps so both actions share one implementation. - scheduler: honor MaxConcurrency as upper bound (was silently capped at GOMAXPROCS*2, misleading for IO-bound workloads) - scheduler: snapshot runCtx state at step-completion time (worker-side), not observation time (event-loop-side), to prevent FailFast tear-down from mis-classifying genuine per-step timeouts as scheduler cancellations - scheduler: synthesize StepSkipped timings for unqueued steps on FailFast/parent-cancel so result.Steps is complete Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- bicep_provider: serialize deploymentState ARM calls — fetch prior deployment first, skip CalculateTemplateHash when there is no comparable prior. Removes an ARM call per layer on the common cold-start path and avoids direct rate-limit pressure under multi-layer fan-out. - bicep_provider: invalidate deployment state when CheckExistenceByID returns an error (transient ARM/throttling/auth). Previously a probe error was silently treated as 'RG still exists', so a deleted-out-of-band RG could be missed and the cached state incorrectly reused. - service_target_containerapp: persist template hash via envManager.Save when it changes inside shouldUseDirectRevisionAPI, so the direct- revision optimization survives process exit on revision paths that have no deployment outputs. Previously the in-memory DotenvSet was lost and the next deploy fell back to full ARM provisioning. - up_graph: guard layerDeps==nil in addProvisionSteps so a future caller that skips AnalyzeLayerDependencies can't crash on a nil-map access. Treats nil as 'no inter-layer edges' (safe fallback). - magefile: remove 8 pre-existing stale-recording exclusions. They masked unrelated regressions in preflight's advisory playback step. Tracked for re-recording in Azure#7780. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Staticcheck SA4006 flagged the 'found' return from env.LookupEnv as never used. The whole branch was dead code after the Phase 2 rewrite (producer-first) — with no statement after it in the loop body, 'continue' became equivalent to fallthrough. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- service_target_containerapp.go: consolidate templateHashMu + envSaveMu into a single envMu (sync.RWMutex) guarding all writes to e.dotenv, eliminating the concurrent map-writes race between parallel deploys. Added lock to the preprovision RESOURCE_EXISTS handler as well. - layer_deps.go: drop the orphan `env *environment.Environment` parameter from AnalyzeLayerDependencies (unused after 4cf47fa removed the last consumer). Remove the environment import. Update all three non-test callers and the ten test call sites + stale comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- service_target_containerapp.go: envHashKey now uses `environment.Key` to normalize hyphens to underscores. For services named `my-api` the previous `strings.ToUpper` would produce `SERVICE_MY-API_TEMPLATE_HASH` which godotenv's parser rejects on reload (variable names must match `[A-Za-z0-9_.]`), leaving the hash un-reloadable on subsequent runs. - deploy.go / deploy_progress.go: deploy-graph progress tracker now classifies terminal states. Previously every non-nil error from OnStepDone mapped to `phaseFailed`, so services that were skipped due to dependency-cascade (StepSkippedError) or parent-cancellation (context.Canceled) rendered as "Failed" in the UI. Added a new `phaseSkipped` terminal phase with a dedicated icon and test coverage, and classify the error in the OnStepDone handler. - bicep_cache_test.go: mock predicate guarded `len(args.Args) >= 2` before reading `args.Args[2]` — bump guard to `>= 3` to eliminate panic risk. - bicep.go: buildCacheKey doc comment claimed "returns (\"\", nil) on unreadable module" but implementation propagates the error (which Build treats as cache-miss). Updated comment to match behavior. PR description also updated to list all three tests in `excludedPlaybackTests` (the third, Test_CLI_InfraCreateAndDeleteUpperCase, was previously omitted from the list). Does NOT address copilot finding on bicep_provider.go:799 (claimed loop-var capture on `resId`): false positive — `resId` is declared with `:=` inside the loop body, so each iteration has its own block-scoped variable. Reply posted separately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three high-severity regressions surfaced in re-review on 11e5473: 1. azd up dropped prepackage/postpackage shell hooks and the ProjectEventPackage Go event because the consolidated up flow no longer dispatches sub-commands through cobra middleware. Add four new graph nodes (cmdhook-prepackage, event-prepackage, event-postpackage, cmdhook-postpackage) plus a new serviceGraphOptions.packageExtraDeps field so per-service package steps gate on event-prepackage. cmdhook-postpackage now also gates event-predeploy so the old workflow's postpackage->predeploy ordering is preserved. 2. provision_graph.go's second projectEventArgs block was missing the "preview" key after the first-pass fix only updated line 131. Extension handlers type-asserting args["preview"].(bool) would get the zero value here too. Emit "preview": false explicitly (this path is non-preview only; preview goes through provisionPreview). 3. shouldUseDirectRevisionAPI in service_target_containerapp.go persisted the new template hash to env BEFORE the ARM deploy ran. A failed deploy then left a stored hash that caused the next run to take the direct-revision optimization path against unprovisioned resources. Refactor into side-effect-free evaluateTemplateHash that returns (useDirect, currentHash, changed); persistence now happens in the caller AFTER successful ARM deploy. Read still occurs under envMu to protect the shared env map from concurrent service deploys. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…odes and template-hash refactor Brings spec.md in line with the Pass 4 fixes in 9176db0: - Add prepackage/postpackage to the cmdhook-* hook list and update the arch topology comment to show the parallel package chain. - Restructure the Unified Up node layout into Provision/Package/ Deploy sub-chains; package-<svc> no longer has zero deps under azd up (gated on event-prepackage via packageExtraDeps). - Fix the Thread Safety table: shouldUseDirectRevisionAPI used the caller's envMu, not separate templateHashMu/envSaveMu (which never existed). Rewrite the Direct Revision API shortcut paragraph to describe evaluateTemplateHash + post-deploy persistence. - Update semantic difference #1: predeploy hook now runs concurrently with the package chain; event-predeploy gates publish/deploy AND depends on cmdhook-postpackage so postpackage->predeploy ordering is preserved for the event handlers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
HIGH (concurrency regressions surfaced by parallel scheduler): - Add internal sync.RWMutex to *environment.Environment so parallel hooks and services writing dotenv keys can't panic with concurrent map writes - Add saveMu to environment manager so parallel Save/Reload calls don't interleave file I/O against the same .env - Add sync.Mutex to kubectl.Cli singleton (SetEnv/SetKubeConfig/Cwd readers snapshot via snapshotState before exec/template work) - kustomize.Cli.WithCwd now returns a shallow copy instead of mutating the singleton, so parallel kustomize edit calls don't fight over cwd - Add package-level aksEnvMu to service_target_aks.go around SetServiceProperty + envManager.Save sequences (mirrors containerapp envMu) MEDIUM/LOW (behavior + UX): - up_graph.go: only apply provision-specific error wrapping when a provision-tagged step actually failed; package/publish/deploy errors now pass through verbatim - Honor AZD_DEPLOY_CONCURRENCY as a fallback for AZD_UP_CONCURRENCY - CHANGELOG entries for the concurrency fixes, banner text change, AZD_DEPLOY_CONCURRENCY fallback, and FailFast in-flight semantics Includes a TestEnvironment_ConcurrentDotenvSet regression test (32 goroutines x 200 writes) to lock in the Environment fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses three silent-miss cases in the layer dependency analyzer that
produced non-deterministic correctness bugs in parallel multi-layer
provision (sequential mode worked by accident):
1. Non-literal readEnvironmentVariable(varName) in .bicepparam files
was silently dropped (regex only matched single-quoted literals).
2. ARM template expressions like [parameters('foo')] in
.parameters.json files were silently dropped (regex only matched
${VAR} substitutions).
3. param x = readEnvironmentVariable('Y') defaults inside .bicep
files were never scanned (bicep parsed only for outputs, not refs).
When the parser encounters a syntax pattern it cannot resolve to a
literal env-var name, the consuming layer now falls back to depending
on all earlier layers (safe-by-default). The fast path for parsable
layers stays parallel.
Hook-mediated edges (e.g. a postprovision hook in layer A writes an
env var that layer B's bicepparam reads) cannot be inferred by any
static analyzer. A new + 'infra.layers[].dependsOn' + field in azure.yaml
lets authors declare these explicitly. Explicit edges union with
detected edges; cycles, self-references, and unknown layer names are
validated.
Tests: 9 new tests added covering each silent-miss case, the
safe-fallback path, the bicep param-default scan, and explicit
dependsOn validation (unknown layer, self, cycle).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…inner debug log Item A (docs): Add cli/azd/docs/concurrency-model.md documenting the locking contracts for environment.Environment, environment.Manager, kubectl.Cli, containerAppTarget/aksTarget, and serviceManager. Includes a short history note and an Adding new concurrent state checklist. Linked from cli/azd/AGENTS.md under a new Concurrency subsection so future contributors discover the rules before adding write paths to these types. Item B (UX): Restore intra-phase progress detail in the per-service tracker during graph-driven azd deploy / azd up. addServiceStepsToGraph now accepts an optional onPhaseProgress callback; an internal newPhaseProgress helper drains async.Progress[ServiceProgress] events from each phase and forwards ServiceProgress.Message to the callback. deploy.go wires this to da.updateProgress so the tracker's Detail column shows messages like 'Pushing image' / 'Updating container app' instead of staying blank between phase transitions. up_graph.go leaves it nil (no tracker) — preserving previous behavior. Item C (diagnosability): Replace silentSpinnerConsole's no-op ShowSpinner/StopSpinner methods with log.Printf calls that record the dropped spinner title at debug level. Suppressing the visual spinner in the graph-driven path is intentional, but completely swallowing the calls made graph regressions hard to debug — the log line preserves the intent in azd trace logs without re-introducing the broken visual spinner. Refs: Azure#7776 (comment) (vhvb1989 pass 5) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#3 — Cross-layer hook ordering contract Document on runProvisionSingleLayer's docstring that when the dependency graph contains an edge B -> A, layer B's step is scheduled only after layer A's full 7-stage lifecycle (including post-provision event AND layer post-hooks) returns. This is enforced at the scheduler layer because addStep treats the entire runProvisionSingleLayer invocation as one graph node. Layers that need this guarantee for hook-mediated env values (where A's postprovision hook writes a value B's bicepparam reads) must declare the edge explicitly via dependsOn — the static analyzer cannot see hook side-effects. Add TestProvisionLayersGraph_DependentLayerWaitsForPostHooks in provision_graph_test.go — drives the guarantee at the scheduler layer by simulating each step's body as the full lifecycle and asserting B's start observes A's post-hook completion. #4 — Multi-layer adoption telemetry + awesome-azd audit Add four ProvisionLayer* attribute keys in internal/tracing/fields/fields.go alongside the existing exegraph.* keys: - provision.layer.count (declared layers) - provision.layer.max_parallel (achievable parallelism) - provision.layer.safe_fallback_count (hasUnknown engagements) - provision.layer.explicit_dependson_count (dependsOn adoption) Surface bicep.LayerDependencies.SafeFallbackLayers from AnalyzeLayerDependencies (counts only — the fallback edges were already in Edges; the new field just lets callers report how many layers hit safe-fallback without re-deriving the count). Wire emitMultiLayerProvisionTelemetry on the multi-layer path in provision_graph.go to attach the four attributes to the ambient command span via tracing.SetAttributesInContext. All attributes are SystemMetadata + PerformanceAndHealth — counts only, no template content — so they ship under the existing telemetry consent. Document the four attributes plus the awesome-azd audit finding in docs/specs/exegraph/spec.md (new lessons-learned point Azure#13). The audit (org:Azure-Samples filename:azure.yaml for layers:) confirms vhvb's suspicion: zero official templates use multi-layer infra.layers[] today; only three community user repositories declare it. The new telemetry will let us track adoption going forward. Refs: Azure#7776 (vhvb1989 pass 5, items 3 and 4) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The misc cspell config (used by the cspell-lint workflow on non-cli paths including docs/specs/exegraph/) does not pick up cli/azd/.vscode/cspell-azd-dictionary.txt. Add 'dependson' to the exegraph file override so the spec docs lint cleanly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The multi-layer parallel provision path cloned each layer's environment
from the parent process's in-memory deps.env at step 0 and merged
deployment outputs back at step 4 via envManager.Save. Hooks and event
handlers run as subprocesses that write to disk via their own
envManager — the parent's deps.env was never reloaded. Two related
bugs followed:
1. Step 4 Save would serialize the (stale) in-memory deps.env to disk,
silently clobbering any FOO=bar that this layer's pre-hook wrote via
'azd env set'.
2. Subsequent layers ordered after this one (via dependsOn or detected
edge) would clone deps.env.Dotenv() at their step 0 and miss any
FOO=bar this layer's post-hook wrote, making 'dependsOn' silently
incomplete for hook-mediated edges — which is exactly the case
dependsOn was added to handle.
Fix: reload deps.env from disk before step 4's Save, and add a final
step 8 reload after step 7 (post-hooks) so downstream layers see all
hook-mediated writes. Both reloads happen under envMu.
Also extracts mergeLayerOutputsLocked / reloadSharedEnvLocked helpers
and adds focused tests proving the propagation contract end-to-end:
- TestMergeLayerOutputsLocked_PreservesSubprocessWrites — mutates
disk behind deps.env's back, asserts merge does not clobber.
- TestReloadSharedEnvLocked_PropagatesToDownstreamLayer — asserts a
downstream layer's clone sees disk state after reload.
Renames the previous TestProvisionLayersGraph_DependentLayerWaitsForPostHooks
to TestProvisionLayersGraph_DependsOnEdgeOrdering to honestly reflect
that it tests scheduler-level DependsOn semantics, not the full
runProvisionSingleLayer lifecycle.
Other detector hardening surfaced in the same review pass:
- armExpressionRe now uses [^\\]] so ARM expressions with escaped
quotes ('json(\\'{...}\\')') are still detected.
- discoverParamEnvRefs escalates non-NotExist read errors to
hasUnknown=true (was: silently dropped edges).
- Options.DependsOn gains a json tag for schema parity.
Refs: Azure#7776
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Spec now reflects the hook-mediated env propagation contract added in 84286f6: - Implementation Notes #8 expanded to cover the full 8-step lifecycle (pre-hooks -> pre-event -> Deploy -> merge -> service event -> post-event -> post-hooks -> reload), the merge step's pre-Save reload, the new step 8 final reload, and why concurrent siblings intentionally don't see each other's mid-flight hook writes. - Thread Safety table gains a provision_graph.go row pinning the envMu (deps.env reads/writes) and hookMu (handler serialization) contracts. - Test coverage row for provision_graph_test.go updated from 3 -> 7 tests with descriptions of the new merge / reload tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rdings - cspell.misc.yaml: allow 'appsettings' in docs/specs/exegraph/** - provision_graph_test.go: //nolint:gosec G703 on tempdir WriteFile (envPath from t.TempDir()) - magefile.go: add 7 playback tests to excludedPlaybackTests — all stale due to feat/exegraph graph-driven provision introducing new resourcegroups list probe and provision-layer-0 deployment interactions. Must be re-recorded in record mode with live Azure credentials before merge. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The unified up exegraph absorbs package and provision phases in-process, so the cobra middleware no longer fires nested cmd.package / cmd.provision spans (legacy workflow runner spawned them as child processes). Emit synthetic spans from UpGraphAction.Run as children of cmd.up so the telemetry contract (parent/child shape, SubscriptionIdKey, EnvNameKey, CmdEntry, CmdFlags) matches the legacy workflow output. Spans cover the whole Run() duration (approximate; the phases overlap in the unified graph) and end in package -> provision order so the trace file lists them in the legacy sequence. Fixes Test_CLI_Telemetry_NestedCommands regression in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Old sequential deploy consumed services topo-sorted by Uses via ImportManager.ServiceStableFiltered, so declared service-to-service deps (e.g. web uses: [api]) implicitly serialized via iteration order. The new parallel graph preserved topo iteration but dropped the edge, letting web.predeploy race api.postdeploy and see stale env values. Translate svc.Uses entries that name other services into "deploy-<svc>" edges on the deploy step. Resource-valued entries are skipped (provision layer owns them), matching the logic in ImportManager.sortServicesByDependencies. Edges dedupe against the Aspire build-gate so concurrent rules don't emit duplicates. Adds TestDeployServicesGraph_UsesDependencies and TestDeployServicesGraph_UsesWithBuildGate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…que correlation IDs Parallel deploys of multiple Container Apps services via ACR remote build could end up serving the same image under different repository names. Root cause: ACR's GetBuildSourceUploadURL derives the upload blob path from the caller's x-ms-correlation-request-id header. azd's correlation policy set that header from the root OpenTelemetry trace ID, which is shared across every request in a single `azd up` invocation, so parallel uploads collided on the same blob and the last writer won. Fix: - ACR remote-build RegistriesClient now wraps its options with a per-call policy that overrides x-ms-correlation-request-id with a fresh UUID per request. Minimal, scoped to the upload path. - NewMsClientRequestIdPolicy and NewMsGraphCorrelationPolicy now emit a fresh UUID per HTTP request, matching the ARM and Graph specs which require x-ms-client-request-id / client-request-id to be unique per call. Defense in depth against other Azure services that use these headers as dedupe / idempotency keys. - NewMsCorrelationPolicy keeps trace-ID-derived correlation (correct by spec for grouping related requests). Tests: - New unit tests assert unique UUIDs per request for both per-request policies and that trace-ID behavior is preserved for the session correlation policy. - New functional test `Test_CLI_Up_Down_ContainerApp_RemoteBuild_MultiService` deploys two Container Apps in parallel via remote build and asserts each service serves its own identity, guarding against regression. Fixes Azure#7776 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…provision Addresses two HIGH findings from wbreza review on PR Azure#7776. H1 — local_file_data_store.go: parallel service hooks spawning `azd env set` subprocesses each held only the in-process saveMu, so the truncate-rewrite in Save raced across processes (last writer silently clobbered another writer's keys). Acquire a gofrs/flock-based OS-level file lock around the reload-merge-write cycle in both Save and Reload, write to a sibling tmp file then atomically rename via osutil.Rename (Windows-safe with retry on sharing-violation). configManager.Save is moved under the same lock to prevent torn JSON config reads. manager.Get's remote-fallback Save path now also takes saveMu so in-process callers stay serialized too. Regression test Test_LocalFileDataStore_ConcurrentSave_NoLostUpdate spawns 8 concurrent LocalFileDataStore instances against the same env directory (bypassing in-process saveMu, mimicking subprocesses) and asserts every writer's keys survive. H3 — up_graph.go: cmdhook-prepackage and cmdhook-preprovision were both graph roots, so the scheduler could start them concurrently. Pre-PR azd up ran package then provision sequentially, so users could rely on prepackage writing env-vars (e.g. computing an image tag) that preprovision reads in a bicepparam file. Add DependsOn: [prePackageHookStep] on preProvisionHookStep to preserve the prior ordering (the two shell hooks now serialize; package and provision themselves still overlap as intended). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mq follow-up to de57ff4 addressing secops + smells/arch findings on the H1 (cross-process .env race) fix: - Make lockPath unexported (was LockPath; no external callers). - Replace osPermDir local const with osutil.PermissionDirectory. - Extract acquireEnvLock(ctx, env) + releaseEnvLock helpers; both Reload and Save now share one code path. - Use TryLockContext(ctx, 50ms) instead of unbounded Lock(): a wedged flock holder (e.g. hung 'azd env set' subprocess; LockFileEx on Windows is not signal-aware) no longer freezes azd; Ctrl-C and context deadlines now cancel the wait. (secops M-1) - manager.Get's remote-fallback Save now releases saveMu via defer inside a closure instead of two Unlock sites. (smells/arch #7) No behavior change for the happy path; tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mq follow-up addressing secops L-1 and L-2: - L-1: Save now sweeps stale .env.tmp-* siblings (>1h old) under the flock before creating a new one. Defends against tmp file accumulation if azd is SIGKILL'd or the host loses power between CreateTemp and Rename. The flock guarantees no concurrent in-flight tmp file, so the sweep is race-free. - L-2: CHANGELOG note documenting that .env files are now persisted with mode 0600 (was umask-dependent, typically 0644). The atomic os.CreateTemp + Rename in de57ff4 tightened the mode as a side effect; calling it out explicitly in release notes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pre-exegraph, `azd up` spawned `azd package` and `azd provision` as child processes; each flushed its own batch processor independently, so the trace file naturally listed cmd.package -> cmd.provision -> cmd.up in End() order. Post-exegraph, the unified up command emits all three cmd.* spans through a single in-process BatchSpanProcessor. The OTEL Go SDK does not guarantee FIFO ordering of spans across batches, so the strict ordering assertion is flaky on busy CI environments (4/4 ADO platforms failed deterministically while local Windows passed 5/5). Refactor the test to collect cmd.* spans by name and verify each span's attributes individually. This preserves all existing per-span assertions (TraceID, SubscriptionId, EnvName, CmdEntry, CmdFlags, CmdArgsCount) without relying on flush order. Also drop the now-stale ordering comment in up_graph.go that documented the End() order contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… creds Test_CLI_Up_Down_ContainerApp_RemoteBuild_MultiService called recording.Start(t) before its skip check. recording.Start does t.Fatalf when the cassette is missing, so CI without cassettes AND without AZURE_TENANT_ID hard-failed instead of skipping. Fix: check cassette existence + AZURE_TENANT_ID before recording.Start, skip cleanly if both absent. Keep the original post-Start nil-session skip as defensive guard. (The companion completion_test.go phantom-failure fix from this same commit was superseded by upstream PR Azure#7714 during rebase, which adds a trailing newline in the renderer plus stdout capture.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Blocking (3): 1. Added env var docs for AZD_*_CONCURRENCY to environment-variables.md 2. syncConsole verified — exists at provision_graph.go:1146 3. Unicode verified — correct chars in deploy_progress.go High (7): 4. Callback panics already handled by safeNotifyStart/safeNotifyDone 5. progressTracker already safe — set before goroutines, internal mutex 6. Fixed spinnerLine() lock gap — added spinnerLineMu in ShowPreviewer 7. Timeout warning now includes value + usage examples (--timeout, env var) 8-10. Added 3 tests: parent context deadline, discoverParamEnvRefs I/O errors, dotenv special character round-trip Medium (13): 11. Path traversal: hashBicepFileTree validates modules stay in project dir 12. Snapshot output: deterministic from input, not user-controlled 13. Silent file read errors: logged with warning in discoverParamEnvRefs 14. Panic recovery: distinguished with log.Printf in scheduler runStep 15. Remote build errors: wrapped with HTTP status + operation context 16. Unbounded streamLogs: added maxPollIterations=1200 (~20 min cap) 17. Unbounded recursion: hashBicepFileTree has depth>100 guard 18. File I/O timeout: added readFileWithContext wrapper in layer_deps 19. Doc comments: StepStatus/StepSkippedError/NewGraph already documented 20. Concurrency clamping: log.Printf when clamping to 64 21-23. Added 3 tests: MaxConcurrency=1, ContinueOnError aggregation, self-ref/nonexistent uses: in service graph Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…atches - Anchor modulePattern to ^\s* so commented-out module declarations (e.g. // module old 'path') are not matched during cache key hashing - Add stripBicepLineComments() helper that removes // comments before regex scanning in layer dependency analysis - Apply comment stripping to extractBicepOutputsFromContent, extractParamEnvRefs (.bicepparam), and extractBicepParamReadEnvRefs - Add 5 regression tests covering comment stripping, commented outputs, commented readEnvironmentVariable calls, and commented module refs Addresses weikanglim review feedback on regex false positives from commented-out code in Bicep files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduces deployGraphState to encapsulate the shared mutable state (ServiceContexts + ServiceDeployResults) that flows through service graph execution. This keeps the command/action layers thin: create state -> build graph -> execute -> consume results - Replace 4 raw fields (serviceContexts, svcCtxMu, deployResults, resultsMu) in serviceGraphOptions with a single state field - Add StoreContext/LoadContext/StoreResult/GetResult/ResultsSnapshot methods with proper encapsulation (two internal mutexes) - Move temp artifact cleanup into CleanupTempArtifacts(), eliminating duplicated cleanup code in deploy.go and up_graph.go - Apply go fix suggestions: bytes.Cut, maps.Copy Addresses wbreza feedback on keeping command/action layers thin and reducing the number of shared references passed through graph builders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… on prior run) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When no service in the graph declares a uses: entry targeting another service, deploy steps chain sequentially in azure.yaml slice order. This preserves backward compatibility with templates that relied on implicit ordering (e.g., api deploys before web). Templates that declare uses: get explicit graph edges and parallel deployment. Package and publish steps remain parallel regardless — only deploy ordering is affected by the fallback. Fixes Azure-Samples/todo-nodejs-mongo-aks#20 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add 'Service Deploy Ordering' section to concurrency-model.md explaining the sequential-by-default fallback and how uses: enables parallel deploy - Include azure.yaml example showing uses: declaration - Document environment variable flow between services during deployment - Update AZD_DEPLOY_CONCURRENCY description to note it only takes effect when uses: edges exist - Update addServiceStepsToGraph doc comment to describe the fallback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix hashBicepFileTree to skip br/alias: module format (e.g.
br/public:avm/...) in addition to br: and ts: prefixes
- Add 5 edge case tests per tg-msft review feedback:
- Conditional modules (module x 'path' = if (cond) {...})
- Registry alias modules (br/public:avm/...)
- Block-commented modules (known cache-miss limitation)
- Indented modules (inside if/for blocks)
- Existing: line-commented modules
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tests When the sequential deploy fallback activates, scan each service's Environment config for SERVICE_<OTHER>_* references using the existing Envsubst API with a recording mapper. Log advisory hints to guide template authors toward explicit uses: declarations for parallelism. This does NOT change execution order. Also: - Fix hashBicepFileTree to skip br/alias: module format (e.g. br/public:) - Add 5 bicep cache edge case tests: conditional modules, registry aliases, block comments, indented modules Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…olling progress - TestDeployGraphState_ResultsSnapshot: verifies snapshot isolation - TestDeployGraphState_StoreLoadContext: verifies store/load cycle - TestResolveDAGConcurrency: 7 table-driven cases covering unset, valid, clamped-to-64, max boundary, invalid, zero, and negative values - TestStartPollingProgress_EmitsMessages: verifies heartbeat emission - TestStartPollingProgress_StopEndsGoroutine: verifies clean shutdown Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ection - Fix "azure.yaml order" to "alphabetical order" everywhere — services are sorted by ServiceStable() which uses slices.SortFunc alphabetically, not YAML file order (ProjectConfig.Services is a map) - Remove stale expandedEnvCache/expandedEnvMu section from concurrency-model.md — these were removed in d579035 - Update service_graph.go doc comments to reference "alphabetical via ServiceStable()" instead of "slice order" Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove stale test cases in correlation_policy_test.go that referenced undefined struct fields (headerName, correlationPolicyFunc). The client-request-id and graph-correlation policies are already covered by dedicated uniqueness tests. - Add missing context import in environment_test.go. - Fix *env value dereference to env pointer in sdk_error_path_test.go to match CreateServiceConnection signature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ecks M8: AZD_UP_CONCURRENCY, AZD_DEPLOY_CONCURRENCY, and AZD_PROVISION_CONCURRENCY now log a warning when set to a non-numeric value instead of silently falling back to unlimited concurrency. M9: Resource-group existence checks in bicep_provider now use errgroup.SetLimit(10) to bound parallel ARM calls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three fixes for the hang reported in PR Azure#7776: 1. Signal handler fix (console.go): Call signal.Reset(os.Interrupt) immediately after first SIGINT so subsequent Ctrl-C terminates the process even if spinner.Stop() blocks. Added 100ms timeout on spinner stop to prevent indefinite blocking. 2. Publish step timeout (service_graph.go): The publish step resolves Azure target resources via API calls that can block indefinitely, especially right after provisioning due to Azure eventual consistency. Added the same deployTimeout as the deploy step. 3. Diagnostic logging (service_manager.go, resource_manager.go): Added log.Printf tracing around target resource resolution so future hangs are diagnosable via --debug output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… panics The scheduler recovers panics in step actions, so a panic between Lock/Unlock leaves the mutex held permanently — all sibling goroutines waiting on that mutex deadlock. Switch four Lock/Unlock pairs to defer Unlock(): - deployGraphState.StoreContext (service_graph.go) - deployGraphState.StoreResult (service_graph.go) - runProvisionSingleLayer envMu (provision_graph.go) - CopyRuntimeStateTo hookGuard (service_config.go) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Partially reverts 737ef70 — only the envMu change in provision_graph.go. The defer envMu.Unlock() held the mutex for the entire function scope (terraform init/plan/apply + hooks + merge). When provisioning succeeded, mergeLayerOutputsLocked() tried to re-acquire envMu, causing a self-deadlock (sync.Mutex is not reentrant). This manifested as 89-minute hangs in ACA Job runners for terraform-based templates. The other changes from 737ef70 (service_graph.go StoreContext/StoreResult, service_config.go CopyRuntimeStateTo) are safe — those mutexes are not re-acquired within the same function scope, so defer is correct there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The publish step now applies the same deployTimeout as the deploy step, since it resolves Azure target resources via API calls that can block due to eventual consistency after provisioning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Reject absolute/UNC module paths in hashBicepFileTree to prevent path traversal via cross-volume resolution (CWE-22) - Treat filepath.Rel errors as path escapes (skip module) - Use singleflight.Group in cachedServiceTarget and cachedTargetResource to deduplicate concurrent ARM API calls for the same service - Add godoc to StepStatus enum values Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Extract classifyStepResult() as single source of truth for step status classification, eliminating 3x duplication of the scheduler-cancel predicate in the event loop and drain loop - Replace time.Sleep-based concurrency assertions with channel barriers in TestRun_FanOut and TestRun_MaxConcurrency to eliminate flakiness - Use runtime.Gosched() instead of sleep in MaxConcurrency_One test - Add table-driven TestClassifyStepResult covering all classification cases - Add TestRunWithResult_StepTimeout_ClassifiedAsFailed verifying per-step timeouts are StepFailed (not StepSkipped) - Add TestRunWithResult_FailFast_DrainClassifiesCorrectly verifying in-flight steps canceled by FailFast are StepSkipped Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
After provisioning completes, azd up now displays the same live progress table as azd deploy (Service / Phase / Elapsed / Detail). The tracker starts rendering when the first publish or deploy step begins — after provisioning finishes — so it doesn't conflict with provision progress. Packaging phase updates are recorded silently (they overlap with provisioning by design) and appear as 'Done' in the table once the visual rendering starts. This fixes the user-reported issue where azd up appeared stuck for several minutes after provisioning with zero visual feedback during the deploy phase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address wbreza Pass 3 follow-up items: - M3: Log debug message when uses: references an unknown service name - M5: Document non-blocking contract on onPhaseProgress callback - M6: Log when layer deps falls back to serial due to unresolvable refs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ismatch - Save: use context.WithoutCancel + 30s timeout so a canceled parent (e.g. Ctrl-C during first-run init) cannot abort the env persist. Fixes Azure#7976. - Deploy progress timer: exclude packaging phase from startedAt so the Duration column only counts publish+deploy time (not provisioning). Previously the timer started at package-start which runs in parallel with provisioning, inflating the displayed duration vs the SUCCESS message (which subtracts InteractTimeMs). Fixes Azure#7981. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When the deploy progress table owns the terminal (azd deploy / azd up), Docker build output from ContainerHelper's ShowPreviewer would render between table re-renders, causing duplicate tables and leaked output. Fix: Add PreviewerSuppressor interface on the shared console. When the progress table is active, ShowPreviewer returns io.Discard and StopPreviewer is a no-op. This prevents DI-injected consumers (which bypass silentSpinnerConsole) from corrupting the table display. Fixes the UX glitch reported by JeffreyCA where previewer content persists and the status table appears multiple times during remote container builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename UnsuppressPreviewer/SuppressPreviewer to ResumePreviewer/PausePreviewer since 'Unsuppress' is not a recognized word and fails cspell-lint CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a project has only one unnamed layer (the common case), use 'provision' as the step name instead of 'provision-layer-0'. This avoids exposing internal layer numbering in error messages which would confuse users who don't know about layers. Multi-layer projects still use indexed names (provision-layer-0, provision-layer-1, etc.) or explicit layer names when defined. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Test_CLI_PreflightQuota_* recordings are missing HTTP interactions for the extension registry (registry.json) and resource group listing APIs added by recent main changes. These need to be re-recorded with live Azure credentials. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Aspire projects don't use the 'uses:' field in azure.yaml because services are discovered from the AppHost manifest. Previously, hasServiceDeps was only true when a uses: entry matched another service name, so Aspire projects always fell into the sequential deployment fallback — making the build-gate serialization redundant. Now, if the buildGateKey policy returns non-empty for any service, that constitutes an explicit execution ordering policy and the sequential fallback is skipped. The build-gate still ensures the first Aspire service builds the shared AppHost before others deploy, while non-Aspire services in the same graph run in full parallelism. Addresses reviewer feedback from vhvb1989 about Aspire parallel support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
IanMatthewHuff
added
the
pr-sxs-human-evals/comparison-pr
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dd21961be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+297
to
+301
| if !hasExplicitOrdering && opts.buildGateKey != nil { | ||
| for _, svc := range opts.services { | ||
| if opts.buildGateKey(svc) != "" { | ||
| hasExplicitOrdering = true | ||
| break |
There was a problem hiding this comment.
Setting hasExplicitOrdering to true whenever any buildGateKey is non-empty disables the sequential fallback chain even when the project has no uses: edges. In mixed projects (for example, one Aspire service plus non-Aspire services), this changes legacy behavior from sequential deploys to parallel deploys and can break templates/hooks that implicitly depended on slice-order deployment. The build gate already adds the needed edge for gated services; it should not globally suppress backward-compat fallback for unrelated services.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issues
Fixes Azure#6291 ΓÇö Support Parallel Provisioning of Infrastructure Layers
Fixes Azure#5294 ΓÇö Create layer deployment orchestration engine
Related:
Summary
Introduces
pkg/exegraphΓÇö a general-purpose DAG execution engine ΓÇö and rebuildsazd up,azd provision, andazd deployon top of it. Independent work now runs concurrently with dependency-aware scheduling instead of strictly sequentially. No opt-in flag; single-node cases (single-layer provision, preview, single service) run as one-node graphs through the same code path with trivial overhead.User-authored
workflows.up:still runs onworkflow.Runner; phase-scoped DAGs apply inside each sub-command it invokes.Scope: 76 files changed, +10,338 / −779.
Motivation
Today
provision,deploy, andupexecute sequentially:Exegraph runs independent work concurrently, bounded by declared dependencies, with a single unified orchestration for
upso packaging can overlap with provisioning automatically.What's in this PR
Engine ΓÇö
cli/azd/pkg/exegraph/Graph/Stepprimitives with three-color DFS cycle detection and transitive-dependent priority (critical-path bias).FailFast/ContinueOnErrorpolicies and uniform per-step timeout viaRunOptions.StepTimeout.Orchestrators ΓÇö
cli/azd/internal/cmd/up_graph.go,provision_graph.go,deploy_graphbuilt on the engine.service_graph.gobuilds service nodes for deploy and up;project_hooks.gobuildscmdhook-*synthetic nodes forpreprovision/postprovision/predeploy/postdeploy.preup/postupforazd up, so project hooks never double-fire.--timeout>AZD_DEPLOY_TIMEOUT> 1200s) betweenazd deployandazd up.DeadlineExceededsurfaces a dedicated timeout UX.--output jsonskips the live deploy tracker to keep JSON clean.Layer dependencies ΓÇö
cli/azd/pkg/infra/provisioning/bicep/layer_deps.goStatic analysis of Bicep output → parameter references to build the provision DAG.
Observability
exegraph.Runper run,exegraph.Stepper node.internal/tracing/fields.Correctness / safety
runProvisionSingleLayerguards shared state with scoped closures +deferso panics can't leave locks held.errPreflightAbortedByUsertranslates toErrAbortedByUserfor correct cancellation exit codes.ServiceConfighook registration moved behind a pointer-held guard struct; value copies are safe (go vet copylocksclean).New configuration (all optional, all clamp to 64)
AZD_PROVISION_CONCURRENCYAZD_DEPLOY_CONCURRENCYAZD_UP_CONCURRENCYAZD_DEPLOY_TIMEOUTCompatibility
workflows.up:(user-authored) continues to run onworkflow.Runnerunchanged.preup/postupforazd up;cmdhook-*nodes in the graph handle everything else).Spec
Full design ΓÇö engine semantics, unified pipeline, env vars, and how it differs from the prior sequential model ΓÇö at
docs/specs/exegraph/spec.md.Quality gates
mage preflight(cli/azd)go test ./... -short -cover -count=1Test recording debt (MUST re-record before merge)
Three integration tests capture new HTTP interactions introduced by this branch. They are currently listed in
excludedPlaybackTestsinmagefile.goso preflight is green, but need to be re-recorded against live Azure before merge:Test_DeploymentStacksΓÇö graph-driven provision addscalculateTemplateHashand resource-group existence probes.Test_CLI_ProvisionStateΓÇö same: newcalculateTemplateHashinteractions frompkg/infra/provisioning/bicep/layer_deps.go.Test_CLI_InfraCreateAndDeleteUpperCaseΓÇö same: newcalculateTemplateHashinteractions break the--output jsonstate-retrieval path.Re-record with
AZURE_RECORD_MODE=recordand a subscription with sufficient quota, then remove the three entries fromexcludedPlaybackTestsin the same commit.The other 8 entries in
excludedPlaybackTestsare pre-existing stale recordings (verified to also fail onmain@460822e34) and are out of scope for this PR, tracked in Azure#7780.Review focus
scheduler.go.layer_deps.gooutput → parameter graph construction.up_graph.gowiring — cmdhook synthesis + middleware gating to ensure hooks fire exactly once.RunOptions.Out of scope (follow-ups)
package).Key Discussion Links
Review Resolution
Performance
Review Rounds
Architecture Discussion
Mirrored from upstream PR:
https://github.com/Azure/azure-dev/pull/7776Created automatically by pr-sxs-human-evals for code-review agent comparison.
(URL wrapped in a code span so GitHub does not create a cross-reference on the upstream timeline.)
48 upstream conversation comments were not mirrored — see the upstream PR for full review context.