fix: preserve custom apply workflows and autoplan state - #6657
fix: preserve custom apply workflows and autoplan state#6657chenrui333 wants to merge 34 commits into
Conversation
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall coverage in commit 3d899b4 in the Show a code coverage summary of the most covered files.
Updated |
There was a problem hiding this comment.
Pull request overview
This PR fixes regressions in the plan→apply lifecycle introduced by stricter apply-time validation by (a) scoping “convention planfile” validation to Atlantis-managed apply steps so custom apply workflows can run, and (b) ensuring plan results are persisted before publishing success signals (comments/statuses). It also expands test coverage via a Gitea identity round-trip integration test and new GitHub E2E plan-then-apply scenarios.
Changes:
- Gate planfile/hash validation on presence of a built-in
applystep to preserve run-only custom apply workflows. - Persist PullStatus before publishing autoplan/manual plan success, and fail closed with an actionable error path on persistence failures.
- Extend E2E harness with an explicit plan-then-apply scenario and supporting lifecycle utilities; add a Gitea PullStatus round-trip integration test.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/events/project_command_runner.go | Restricts apply-time plan validation/hash capture to workflows containing an Atlantis-managed apply step. |
| server/events/project_command_runner_test.go | Adds unit coverage for run-only apply, mixed apply workflows, and multiple managed apply-step revalidation. |
| server/events/project_command_builder.go | Skips expected plan-hash capture for apply commands that have no Atlantis-managed apply step. |
| server/events/plan_command_runner.go | Persists PullStatus before publishing autoplan/manual plan success; adds persistence-failure handling helper. |
| server/events/gitea_pull_status_integration_test.go | New integration test ensuring Gitea webhook/API identity round trip can retrieve autoplan PullStatus and build immediate apply. |
| server/events/command_runner_test.go | Adds tests asserting autoplan/plan fail-closed behavior when PullStatus persistence fails and success isn’t published early. |
| e2e/vcs.go | Generalizes per-project status querying to be command-specific (plan vs apply). |
| e2e/testcase.go | Introduces ScenarioPlanThenApply and new expectations for apply status contexts and apply comment markers. |
| e2e/README.md | Documents lifecycle scenarios and clarifies fixture repo usage and diagnostics behavior. |
| e2e/on_apply_lock.go | Refactors lock-preservation scenario to reuse shared lifecycle helpers; updates project status assertions to be command-specific. |
| e2e/on_apply_lock_test.go | Adds tests for new status-prefix helper and new-comment detection; asserts plan-then-apply cases are explicit. |
| e2e/lifecycle.go | New shared lifecycle utilities for fixture PR creation, git operations, mutation, and cleanup. |
| e2e/gitlab.go | Updates GetProjectStatuses signature (still unsupported on GitLab). |
| e2e/github.go | Updates per-project status filtering to use a command-specific status prefix. |
| e2e/e2e.go | Refactors plan-only flow into shared lifecycle helpers; adds plan-then-apply runner with stale-result rejection and richer diagnostics. |
|
Follow-up review finding addressed in the latest commits:
The active replan and mixed-mutation fixtures are now on |
b1e0635 to
fdd8dc4
Compare
8aa8ca2 to
a5e2197
Compare
ReviewOverall this is a carefully engineered change — fail-closed persistence everywhere, side-effect-free apply validation, solid path-traversal guards in High —
|
a5e2197 to
af5823d
Compare
|
@jamengual Thanks for the careful review. Addressed on
I kept the E2E harness in this PR. It is already integrated with the merged hosted fixture and supplies the final-head acceptance for the custom plan-path and replan lifecycle behavior; splitting it now would separate the implementation from its required proof. Final head |
|
@chenrui333 - Thanks for working on this. Couple things:
|
Assisted-by: OpenAI Codex <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
fe7938d to
7e539ed
Compare
|
Addressed all eight review points in the commits below. Current head is
The branch is rebased onto |
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Assisted-by: OpenAI GPT-5 <noreply@openai.com> Signed-off-by: Rui Chen <rui@chenrui.dev>
Review: durable plan state / apply regressionsReviewed this against Scope note: I've deliberately excluded anything that already exists on Cleared first — the two things I most expected to be broken are not:
The blocking issues cluster around one design decision and one pattern. CRITICALC1. The plan publication claim has no TTL and its waiters spin foreverTwo halves that combine badly. No expiry. local current = redis.call("GET", KEYS[1])
if not current then redis.call("SET", KEYS[1], ARGV[1]); return 1 end
if current == ARGV[1] then return 1 end
return 0Same in Unbounded, uncancellable waiters. for {
err := c.Database.AcquirePlanPublicationClaim(pull, token)
if err == nil { return token, nil }
if !errors.Is(err, db.ErrPlanPublicationBusy) { return "", err }
time.Sleep(25 * time.Millisecond)
}No context, no deadline, no attempt cap. Reached from HTTP handlers ( And several paths deliberately never release — Failure scenario: a pod is OOM-killed mid-plan, or GitHub 422s a single commit status. The claim persists forever. Every subsequent plan / policy_check / API call on that PR blocks forever, leaking a goroutine and an HTTP connection each. On BoltDB every 25ms iteration is a Recovery is Worth noting this PR also removed the natural self-heal: see H7. Suggested direction: lease the claim with holder renewal (Redis C2. Commands are silently dropped while the claim is held — and apply holds it for the entire
|
Follow-up: two pre-existing S3 issues that this PR newly exposesDeliberately separated from my review above, which covered only code this PR changes. The two below are older code that this PR doesn't touch — but the new plan-generation machinery is what makes them reachable, so they're worth deciding on here rather than filing away. 1.
|
Heads up: the #6642 regression fix is extracted into #6781@chenrui333 — following up on my review above. Since you're short on time on this one, I've pulled the #6642 regression fix out of this PR into #6781 so it can ship and be backported to 0.46.x on its own. No action needed from you; I wanted you to know before you spend more time here. Why split rather than push on this branch. Tracing #6642: Fixing that needs your Your gating logic is unchanged in #6781. Reviewers checked it across run-only, mixed run+apply and policy_check workflow shapes and it holds, including the legacy upgrade path where One change worth flagging: I made the gate fail closed. func requiresManagedPlanFileForApply(ctx command.ProjectContext) bool {
return ctx.RequiresAtlantisManagedPlanFile || hasAtlantisManagedApplyStep(ctx.Steps)
}Gating on the context field alone means any The rest of this PR is still valuable and I'd like to see it land in stages. The blocking items from my review are unchanged, and the claim lifecycle (no TTL, uncancellable spin loops, BoltDB-only offline recovery) is the piece that needs a design decision before more is built on it. There's also a set of cleanup call sites removed without replacement — Suggested order, smallest first: hash binding + validated snapshot (a real security win that stands alone), then generations, then the claim once it has a lease. Happy to take any of those on, or to hand them back if you get time — just say which. Also note the |
|
gonna break down the big pr into small ones |
|
The monolith is being superseded by these reviewable extraction PRs:
Stack: The old immortal claim, broad retention, cleanup removals, custom PLANFILE redirection, and broad E2E harness rewrite were not carried forward. Generation-specific regression tests and the main cleanup lifecycle are preserved in the corresponding slices. Optional E remains separate: current synthetic API requests use negative identifiers, so the literal shared-zero premise is stale; cross-replica identifier isolation is not claimed or expanded into this stack. Please review A and B first, then the stack in order. This PR remains open and draft as the history/umbrella record while the split behavior and scope-accounting dispositions receive review. Nothing has been merged, and this branch has not been force-pushed. |
Summary
planor built-inapplystep requires Atlantis-managed plan restoration and validation.tfplanartifacts in a configured fully custom run-only project's path-safe subtree as user-managedPlan-generation and artifact safety
An active generation is persisted as the existing non-applyable
ErroredPlanStatuswithPlanGeneration != "".BeginPlanGenerationstores that state before plan steps run and retains the project's policy state, so sticky approvals continue to follow the existing policy-hash and discard rules across replans, failures, and restarts.Only a matching
CompletePlanGenerationmay clearPlanGenerationand install final plan results. For an Atlantis-managed plan, that same atomic BoltDB transaction or Redis CAS also stores the accepted generation and SHA-256 digest of the exact local bytes saved by the plan operation. S3 writes an immutable generation-and-digest-addressed object alongside the deterministic canonical object retained for legacy and generic discovery. Durable PullStatus selects the immutable object; S3 metadata records the generation and digest for diagnostics, but PullStatus remains the authorization source.Apply loads or restores the convention plan once, compares its digest with durable PullStatus, and executes an immutable command-scoped snapshot. The command-local digest is revalidated immediately before every built-in apply step. Managed run-only apply receives the same validated snapshot through
PLANFILE; fully custom run-only workflows remain status-only and do not gain artifact hashing or loading.PullStatus written before this change may not contain a durable digest or accepted generation. Those legacy managed plans retain the historical command-start hash validation for upgrade compatibility; the next successful replan installs the durable binding. Empty legacy identity never matches an active or newer accepted generation.
Ordinary
UpdatePullWithResultswrites reject the entire update atomically when any targeted project has an active generation. Superseded and pull-changed commands do not overwrite newer project or aggregate statuses/comments. If a targeted replan supersedes one member of an older multi-project generation, the remaining members are atomically cancelled and made non-applyable rather than left pending.Successful per-project plan statuses are published only after
CompletePlanGenerationdurably persists the matching generation. A persistence failure publishes failed project and aggregate statuses and never publishes a successful plan comment. Aggregate plan state treats an active generation as pending; a real plan error still takes priority and reports failed.A durable per-pull publication claim spans each state-transition/VCS-publication critical section. Apply holds it from its final accepted-generation refresh through execution, fenced persistence, terminal publication, and automerge. Import, state removal, policy, approval, unlock, close, workflow hooks, and PR-backed API flows use the same ordering boundary. Ambiguous post-execution persistence or VCS errors retain the claim so a later replica cannot publish over an operation that may still complete.
Unreadable serialized PullStatus data remains fail-closed for apply, but a new plan can recover it safely: BoltDB discards the unreadable record when
BeginPlanGenerationstarts a fresh non-applyable generation, while Redis compares against the exact corrupt raw value and retries normal CAS conflicts. Genuine backend GET/EVAL errors still propagate.Custom apply compatibility
Targeted
atlantis apply -p <project>and plainatlantis applysupport fully custom run-only workflows whose custom plan stage writes multiple.tfplanartifacts below one Atlantis project root. These workflows use durable status-only authorization and do not require or load Atlantis's convention plan.A workflow with a built-in plan step remains Atlantis-managed even when its apply stage is run-only. Atlantis restores the convention plan from an external plan store after a re-clone, validates PullStatus head/base/project identity, accepted generation, and durable plan digest, and then executes the custom apply command. A built-in apply step likewise requires the convention plan and fails closed if a custom plan stage did not produce it.
Generic discovery excludes only non-convention artifacts owned by a configured fully custom run-only project's matching workspace and path-safe subtree. Convention names derived from
runtime.GetPlanFilenameare reserved: custom artifacts that collide with<workspace>.tfplanor<project>-<workspace>.tfplanretain managed-plan semantics. Sibling and ancestor paths are not claimed. Nested configured managed projects remain discoverable and retain PullStatus validation, expected plan hashes, and convention-plan protection. Custom artifacts remain user-managed; Atlantis does not validate, hash, restore, or delete their contents.See Custom Workflows for naming, placement, generic-versus-targeted behavior, and Terragrunt examples.
Redis rolling upgrades
The persisted status value remains compatible with v0.46.0 because old replicas understand and reject
ErroredPlanStatusfor apply. The generation/digest contract, publication claims, and atomic write protections are new, however, so old and upgraded replicas must not overlap against the same Redis state.Safe HA upgrade procedure:
Operational recovery
Atlantis persists plan and apply results before publishing successful result comments or per-project statuses. Interrupted generations intentionally block apply and policy-result writes until a new plan supersedes them.
If PullStatus serialization is unreadable after a schema transition, running a fresh plan starts a new fail-closed generation and replaces the unreadable state. Genuinely interrupted generations can normally be superseded by replanning or safely cancelled by close/unlock after the active publisher stops.
Publication claims deliberately do not expire because an ambiguous VCS request may still complete remotely after Atlantis loses the response. If a claim is orphaned, stop and verify the owning replica cannot publish. If ownership cannot be established, stop every Atlantis replica and back up the configured database. Recovery first inspects the exact canonical UUID claim token, then atomically compare-and-deletes only that token; missing, malformed, or replaced claims are refused. Follow the documented BoltDB utility or exact Redis Lua procedure before restarting. Close waits for a held claim and is not itself a claim-recovery mechanism.
If apply, import, or state-removal execution succeeds but its durable result cannot be stored, Atlantis retains the claim. Operators must verify infrastructure/state, recover the claim offline, and run a fresh plan before retrying. See Using Atlantis.
Regression and hosted acceptance
The nested hosted fixture was squash-merged in runatlantis/atlantis-tests#21743 as
eb1845fa146d1dc3805c4d4cb2a717b9372e343b. It models one root Atlantis project that creates:generated/dev/atlantis.tfplangenerated/staging/atlantis.tfplanPrevious hosted acceptance for the fully custom #6642 workflow:
fe7938d3c73a3e68340d34ced4097df90c97dea630014671712e2e-githubjob89231426838e2e-gitlabjob89231461938custom-plan-path-applypassed using plainatlantis applyFor #6641, deterministic tests keep plan success behind final PullStatus persistence and immediately apply the persisted autoplan without a manual plan. The reporter's exact Gitea persistence-failure trigger has not been isolated, so this PR continues to address rather than close that issue.
References