[Codex] azure/azure-dev#7776 — feat(exegraph): graph-driven execution engine for up/provision/deploy - #3
Closed
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>
Owner
Author
@wbreza Thanks for the thorough re-review and the resolution tracking table. Here's the triage: CriticalC1 — Missing defer Unlock() — Fixed in 737ef70. Good catch — the scheduler recovers panics (runStep has defer recover()), so a panic between Lock/Unlock leaves the mutex held permanently while execution continues. All 4 non-defer pairs fixed:
C2 — Integration tests for UpGraphAction — Agree this needs tracking. Will create an issue rather than expand PR scope further at 11K+ lines and 7 review rounds. HighH3 — readFileWithContext goroutine leak — Partially valid. The buffered channel (make(chan result, 1)) prevents the send-block leak path described. However, if os.ReadFile stalls on a network/FUSE mount, the goroutine does linger after context cancellation. This is the intentional tradeoff documented in the function's comment: unblock the caller while accepting a bounded goroutine residual. Since this reads a finite number of local Bicep layer files, the practical risk is negligible. No change. H4 — errgroup drops errors in RG check — By design. errgroup.Wait() returning the first error is the standard Go errgroup contract. For this RG existence check, any single failure is sufficient to invalidate cached deployment state — aggregating all errors would require changing the concurrency pattern (errors.Join + no context cancellation) and adds unnecessary ARM API pressure on an already-errored path. H5 — AZD_DEPLOY_TIMEOUT negative validation — Already handled. The env var code path at deploy.go:522 explicitly checks seconds <= 0 and returns an error: "must be greater than 0 seconds". H6 — FailFast migration guidance — Partially agree. Will add a console message when step cancellation starts ("Cancelling remaining steps — in-flight operations may take time to complete") in a follow-up. H7 — uses: silent drop — The upstream validateServiceDependencies() in importer.go catches unknown service names before graph construction. The continue at service_graph.go:457 is defense-in-depth that intentionally skips resource-valued uses: entries (which are valid but don't create service-to-service edges). Adding a warning log here would produce false alarms for resource dependencies. Medium / Low (8-15)
|
Owner
Author
Resolution Summary Update — Rounds 10-11 (2026-04-28/29)john0isaac — Hang report (2026-04-28)
wbreza — Pass Azure#10 (Re-Review, 15 findings)
Self-deadlock discovery (2026-04-28, post-push)
Updated stats
|
Owner
Author
@jongio CTRL + C is fixed only one time is enough to escape thanks for fixing. Bicep still feels stuck to me but I think below points might clarify some things. terraform works but after provisioning you get nothing no print or logs from deployment stage so it feels stuck I feel like maybe deployment prints are not coming out? since it maybe deploying in background but i'm not seeing it.. I think I might be correct there has been a deployment pushed to azure from bicep template |
Owner
Author
@john0isaac Thanks for the detailed report with screenshots! You were right — deploying was happening in the background but azd up wasn't showing any progress during that phase (it only logged to debug output). Root cause: azd deploy (standalone) has a live progress table showing each service's phase (Packaging → Publishing → Deploying → Done), but azd up was missing this — it only had invisible |
Owner
Author
Resolution Summary Update - wbreza Pass 3 (2026-04-29)wbreza - Pass 3 (APPROVED, 7 findings)
john0isaac - Silent deploy progress (2026-04-29)
Cumulative stats (all rounds)
|
Owner
Author
@john0isaac The deploy progress fix and the earlier hang fix are both pushed now. Could you retest |
Owner
Author
PR Status Update All CI checks are green (lint, build, ADO pipeline — 31/31 passing). All review threads resolved. Waiting on:
Ready to merge once we have the above. |
Owner
Author
@jongio I'm currently testing.. ERROR: loading environment: saving local environment, locking .env: context canceledNot sure if this is related or not but this is not the first time I see this error. Somewhere something is being cached that fixes this issue and it's not cached in .azure/ folder as I tried deleting .azure/ to replicate but was not able to. Deployment progress is looking awesometerraformMinor UI Suggestion to not indent it like it's indented right now.. or add a title before it like Deploying and indent it under title if there will be not titles maybe add new empty line between outputs and deployment progress table. BicepNow I get why it's indented to align with bicep output but in terraform it looks unaligned :D |
Owner
Author
@john0isaac Thanks for testing and confirming the deploy progress works! 🎉 Regarding your two items:
Appreciate the thorough testing! |
Owner
Author
Final Resolution Summary — All Feedback AccountedCurrent PR Status
Follow-Up Issues Filed
Reviewer Feedback Dispositionwbreza (7 review passes)
weikanglim (2 review passes)
hemarina (1 review — DISMISSED)
vhvb1989 (APPROVED)
john0isaac (testing)
Cumulative Stats
|
Owner
Author
@jongio it looks like some .env locking code was introduced by this PR (e.g. in https://github.com/Azure/azure-dev/pull/7776/changes/f62e168354d1542785e9c6a75e54ec59e2f3c6c0), can you reconfirm? |
Owner
Author
@JeffreyCA Good catch — you're right. The error message comes from The previous code used unbounded The underlying cause (context being canceled during first-run env save) needs investigation — updated Azure#7976 accordingly. This may be a case where we need to use a longer/independent context for the save operation on the first-run path. |
Owner
Author
@vhvb1989 Both bugs that were filed as follow-ups have been fixed in this PR: - Azure#7976 (.env context-canceled) — Fixed in |
Owner
Author
There are some small UX glitches with the Previewer output (easy to reproduce by running Final output w/o exegraph (previewer content disappears): Final output w/ exegraph (previewer content persists and status table appears multiple times): Recording during https://github.com/user-attachments/assets/ab2777d4-ad22-4b46-bc6c-3cb1b0b0d89c Not a blocker for this PR but hopefully that could be addressed |
Owner
Author
@JeffreyCA Fixed in |
Owner
Author
When there is an error from provision for projects not using layers, the error makes reference to |
Owner
Author
@JeffreyCA Could you retest the previewer glitch with the latest push (\�93fb565)? The Docker output should no longer bleed through the progress table during remote builds. |
Owner
Author
Good catch! Fixed in 24c5b7b — single-layer projects (the common case) now show just |
Owner
Author
/azp run azure-dev - cli |
Owner
Author
Azure Pipelines successfully started running 1 pipeline(s). |
Owner
Author
Known limitation: interactive hooks during parallel executionOpened Azure#7994 to track this. The exegraph scheduler runs service-level steps (package, deploy) concurrently across services, but there is no guard preventing multiple Project-level hooks are safe (single DAG nodes with serializing edges). The risk is with service-level hooks across multiple services. This is likely an edge case for user-defined hooks, but extensions also support lifecycle hooks and azd cannot know ahead of time whether an extension hook will prompt the user — making this harder to validate statically. Possible mitigations range from disabling parallelism when interactive hooks are detected, to introducing a console ownership protocol where hooks/extensions declare they need exclusive console access. See Azure#7994 for details. |
Owner
Author
Aspire parallel deploy fix (pushed)@vhvb1989 — Good catch on both questions. Here's what we've done: Aspire projects (your second question)Fixed in 2c63878. Aspire projects no longer need
Non-Aspire projects (your first question)For non-Aspire projects without Summary
Happy to have you re-test with an Aspire project to validate! |
Owner
Author
Everything looks good in bicep and in terraform: You can ignore the timeout error it's an application error not related to azd. There are no longer env locking errors in fresh environments. |
Owner
Author
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
IanMatthewHuff
deleted the
review/pr-7776-5f6079a-run-20260522T221426Z-codex
branch
May 22, 2026 22:15
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.)