Skip to content

Track DAG concurrent execution rollout - #2460

Closed
Mikhail Shirkov (shirkevich) wants to merge 2 commits into
cloudposse:mainfrom
shirkevich:codex/dag-concurrent-execution-tracker
Closed

Mikhail Shirkov (shirkevich) wants to merge 2 commits into
cloudposse:mainfrom
shirkevich:codex/dag-concurrent-execution-tracker

Conversation

@shirkevich

@shirkevich Mikhail Shirkov (shirkevich) commented May 20, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Expands the DAG concurrent execution rollout tracker into a broader implementation plan for splitting the work across separate PRs.

The tracker now documents:

  • the layered implementation strategy from process/I/O primitives through scheduler, adapters, Terraform routing, concurrency, affected routing, mixed-type DAGs, and diagnostics
  • cross-cutting rules that must hold across the whole stack, including dependencies.components, ready-queue scheduling, no new internal/exec files, finalize semantics, and sequential parity for --max-concurrency 1
  • why concurrency must not become user-visible before Terraform bulk routing is consolidated away from the current ExecuteTerraformQuery() path
  • the branch/stacking model for PR 1 through PR 8
  • per-PR ownership, review focus, and behavior boundaries
  • current PR 3 validation findings from temporary local concurrency experiments

Active PRs

Step PR Status Branch Scope
PR 1 cloudposse/atmos#2459 Open draft, base main codex/dag-process-io-foundation Process runner, stream injection, shell wrapper integration, runTerraformShow stdout capture fix
PR 2 cloudposse/atmos#2461 Open draft, base codex/dag-process-io-foundation codex/dag-scheduler-core Generic pkg/scheduler core with ready-queue scheduling and isolated tests
PR 3 cloudposse/atmos#2462 Open draft, base codex/dag-scheduler-core codex/dag-terraform-graph-bulk-path Terraform --all, --components, and --query graph-backed routing; includes temporary concurrency validation hooks and discovered safety prerequisites
Tracking cloudposse/atmos#2460 Open draft codex/dag-concurrent-execution-tracker Rollout plan, findings, and PR coordination document

The stack branches now also exist on cloudposse/atmos, and PR bases are chained directly. GitHub's official stacked-PR feature is not currently enabled for this repository, so the stack is represented through normal chained PR base branches rather than gh stack metadata.

Planned PRs

Step Intended base Proposed branch Scope Key gates / non-goals
PR 4 PR 3 codex/dag-terraform-plan-concurrency Enable user-visible bounded concurrency for Terraform plan on the graph-backed path. Wire --max-concurrency for plan-only execution and add output orchestration sufficient for readable CLI behavior. Requires PR 3 review decision on physical-path locking. Must not enable concurrent apply/destroy. Must preserve --max-concurrency 1 parity.
PR 5 PR 4 codex/dag-terraform-apply-destroy-concurrency Extend concurrency to Terraform apply/destroy with explicit safety rules, failure handling, cancellation, and finalize semantics. Requires settled plan concurrency behavior. Must define apply/destroy risk controls before implementation. No mixed-type DAGs.
PR 6 PR 5 codex/dag-affected-scheduler-route Route --affected onto the scheduler/orchestrator path after non-affected Terraform bulk execution is stable. Must preserve current affected detection, deletion handling, store/auth behavior, and existing affected UX. No new concurrency semantics beyond what PR 4/5 already established.
PR 7 PR 6 codex/dag-mixed-type-adapters Add mixed-type DAG adapter support after Terraform-only scheduling is stable. Include explicit adapter boundaries for Terraform and future component types. Requires repo-owner agreement on mixed-type ordering semantics. Do not rewrite unrelated executors.
PR 8 PR 7 codex/dag-scheduling-diagnostics Add advanced scheduling diagnostics, graph/debug output, and operator-facing troubleshooting aids. Should not change scheduling semantics. Keep diagnostics opt-in and safe for CI logs.
Follow-up / design Before or alongside PR 4 TBD Decide whether true same-folder alias parallelism should use isolated per-node workdirs, isolated TF_DATA_DIR, and retained debug artifacts. Needs repo-owner decision because it changes the operator debugging model and atmos terraform shell expectations.

Rollout Shape

  1. PR 1: Process and I/O foundation - open draft, base main
  2. PR 2: Generic scheduler core - open draft, base codex/dag-process-io-foundation
  3. PR 3: Terraform graph-backed bulk path consolidation - open draft, base codex/dag-scheduler-core; currently includes validation-only concurrency override that must be removed or fixed back to sequential before review-ready state
  4. PR 4: Concurrent Terraform plan - planned, depends on PR 3 review decisions and output orchestration
  5. PR 5: Concurrent Terraform apply/destroy with safety and finalize semantics - planned
  6. PR 6: Route --affected onto the scheduler path - planned
  7. PR 7: Mixed-type DAG adapters - planned
  8. PR 8: Advanced scheduling and diagnostics - planned

Current Findings

PR 3 local validation used a temporary ATMOS_EXPERIMENTAL_DAG_MAX_CONCURRENCY override to exercise the scheduler path before exposing any user-visible concurrency. Findings so far:

  • The active PRs are now stacked through chained base branches in cloudposse/atmos: PR 1 -> PR 2 -> PR 3. The official gh-stack CLI extension is installed locally, but gh stack link reported that stacked PRs are not enabled for this repository.
  • Concurrency 1 preserved the sequential baseline.
  • Concurrency 2 and 4 successfully reduced wall time in downstream validation after fixing credential-store initialization.
  • The first concurrency-4 run exposed an auth race: per-component credential-store initialization called global viper.BindEnv, causing fatal error: concurrent map writes. PR 3 now fixes this narrowly in pkg/auth/credentials by resolving keyring type directly with precedence ATMOS_KEYRING_TYPE > auth.keyring.type > system.
  • The first concurrency-8 run exposed local Terraform working-directory contention: logical aliases sharing one physical Terraform component directory raced on generated files and .terraform/terraform.tfstate. PR 3 now adds Terraform-adapter resource locking keyed by physical component path, preserving parallelism across distinct folders while serializing aliases sharing one folder.
  • True parallelism for aliases sharing one physical Terraform directory likely requires isolated per-node workdirs plus isolated TF_DATA_DIR and generated files. That needs repo-owner discussion because it changes the debugging/operator model, including retained execution copies and how atmos terraform shell should map to them.
  • Output remains heavily interleaved at concurrency greater than 1, so output orchestration is still required before any user-visible concurrency is shipped.

Validation

Tracker update only. Implementation validation is tracked in the linked PRs, especially PR 3.

Next Step

Bring PR 3 back to its intended review shape: keep Terraform --all, --components, and --query on the graph-backed scheduler path, preserve auth/store/YAML-function behavior, retain the narrow safety prerequisites discovered during validation, and force effective execution back to sequential for review.

Before PR 4 starts, discuss the longer-term isolated-workdir model with repo owners and decide how debugging artifacts, cleanup, and atmos terraform shell should behave if true same-folder alias parallelism is introduced. If the repository later enables GitHub stacked PRs, the existing chained-base branches can be linked into official stack metadata.

@atmos-pro

atmos-pro Bot commented May 20, 2026 •

Copy link
Copy Markdown
Contributor

Tip

Atmos Pro  

No affected stacks workflow was detected for this pull request.
If this is expected, no action is needed.
Learn More.

@github-actions github-actions Bot added the size/s Small size PR label May 20, 2026
@github-actions github-actions Bot added size/m Medium size PR and removed size/s Small size PR labels May 20, 2026
@shirkevich

Mikhail Shirkov (shirkevich) commented May 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

PR3 current-state draft is open: #2462

What is in the branch:

  • Terraform --all, --components, and --query now route through the graph-backed scheduler adapter.
  • Terraform graph construction prefers dependencies.components and falls back to settings.depends_on for compatibility.
  • Query-path auth manager setup, store resolver bridging, YAML function resolution, and per-component CI hook capture are preserved.
  • fix(terraform): preserve explicit identity and auth context for local runs #2348 is included in the stack so local --identity terraform testing works.

Validation performed:

  • Focused Go regression tests passed for scheduler/adapters/internal exec query routing.
  • Built build/atmos and live-tested against a downstream Atmos stack using terraform plan --all with an explicit identity.

Temporary concurrency timing experiment using ATMOS_EXPERIMENTAL_DAG_MAX_CONCURRENCY:

  • 1: success, real 202.33s
  • 2: success, real 136.56s
  • 4: failed. First run exited after real 111.89s with a Terraform component execution error. Captured rerun failed after real 37.29s with fatal error: concurrent map writes in viper.BindEnv during auth credential store setup.

Finding:

  • The 4-way failure was in auth credential-store initialization mutating global Viper state during parallel component setup. It is now handled in [codex] consolidate terraform bulk execution on scheduler #2462.
  • Output is heavily interleaved when concurrency is greater than 1, so user-visible concurrency still needs output orchestration before shipping.

Draft caveat:

@shirkevich

Mikhail Shirkov (shirkevich) commented May 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

Update for PR3 / #2462:

The auth credential-store race discovered during the concurrency-4 timing experiment has now been fixed in #2462. The branch removes per-component global Viper env binding from pkg/auth/credentials.NewCredentialStoreWithConfig and resolves keyring type directly with precedence ATMOS_KEYRING_TYPE > auth.keyring.type > system.

Validation after the fix:

  • go test ./pkg/auth/credentials
  • go test -race ./pkg/auth/credentials -run TestNewCredentialStoreWithConfig_ConcurrentInitialization
  • go test ./pkg/auth ./internal/exec -run TestCreateAndAuthenticateManagerWithAtmosConfig|TestSetupTerraformAuth|TestProcessComponentConfig_PropagatesAuthManager|TestProcessComponentConfig_AuthManagerGuardBranches
  • rebuilt build/atmos
  • reran a downstream concurrency-4 repro against a real Atmos stack

Result: concurrency 4 now succeeds, real 110.59s. Captured log has no fatal error, no concurrent map writes, and no viper.(*Viper).BindEnv stack. Output is still interleaved at concurrency >1, so output orchestration remains separate before user-visible concurrency can ship.

@shirkevich

Mikhail Shirkov (shirkevich) commented May 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

Update for PR3 / #2462:

Investigated the concurrency-8 failure. It was not the auth/Viper race. It was OpenTofu local working-directory contention:

  • failure: Error locking state / Error acquiring the state lock
  • lock path: .terraform/terraform.tfstate
  • failing wrapper: a Terraform component execution failed for one logical component
  • root cause: multiple logical components shared the same physical Terraform component folder. Concurrent Terraform init/plan in that same folder races on generated files and local .terraform metadata even when remote workspaces differ.

Implemented the short-term fix in #2462:

  • Terraform adapter now locks dispatch by physical component path.
  • Key prefers component_info.component_path from describe stacks, then falls back to base component fields.
  • This keeps different physical component folders parallel, but serializes aliases sharing one folder.
  • Added tests proving shared physical paths do not overlap and distinct paths can still overlap.

Validation:

  • focused scheduler/adapter/internal exec tests pass
  • rebuilt build/atmos
  • reran a downstream concurrency-8 repro against a real Atmos stack
  • result after fix: success, real 93.47s; no state-lock error, no auth panic

Longer-term discussion item for repo owners:
True parallelism for logical aliases sharing one physical Terraform folder would require per-node isolated workdirs, isolated TF_DATA_DIR, and generated backend/provider/var files in those copies. That changes the debugging/operator model: Atmos would need a policy for retaining or cleaning those copies, exposing paths in output, and deciding how atmos terraform shell should map to/debug the exact copy used for an execution. For now, path locking preserves the current debuggable behavior.

@shirkevich

Copy link
Copy Markdown
Collaborator Author

Superseded by same-repo tracking draft PR #2467, which uses cloudposse/atmos as the head repository and points at the replacement stacked PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m Medium size PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant