diff --git a/CHANGELOG.md b/CHANGELOG.md index 71d1c534..bfd07c48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,18 @@ A `Migration` section is added to any release that bumps `schema_version`. is never rebuilt from the flat map and any existing subtree is preserved verbatim. A single-component manifest with no `components` subtree emits byte-identical output. +- **promote:** Promotion finalize no longer advances the environment pointer + (`state..sha` and `state..version`) when an in-scope deploy + terminally failed or was cancelled. Previously the pointer advanced to the new + commit regardless of the deploy result, so with `rollback_on_failure` (the + default) the auto-rollback job redeployed the environment's prior sha while + state recorded the new one: state asserted a version the environment was not + running. Finalize now gates the pointer advance on deploy success, mirroring + the rollback finalize gate, and holds the pointer at its prior value when any + in-scope deploy did not succeed, so a re-dispatch re-plans and retries the + failed deploy. Per-deploy success rows are still recorded individually, so a + partial success is not lost. A promotion whose deploys all succeed, or that + advances an environment with no deploys, is unchanged. - **generate:** A callback declaring `retries` is now judged on its ladder's effective result, so a deploy that fails and is then rescued by a retry no longer fails the run or gets denied in recorded state. A GitHub Actions job diff --git a/docs/src/content/docs/guides/promote.md b/docs/src/content/docs/guides/promote.md index 526bd31c..f318e26a 100644 --- a/docs/src/content/docs/guides/promote.md +++ b/docs/src/content/docs/guides/promote.md @@ -40,6 +40,8 @@ A breaking-change gate sits at the prerelease-to-release boundary regardless of With `rollback_on_failure: true` (the default), a promotion is all-or-nothing. Preflight records the target environment's current SHA as `rollback_sha`; if any deploy job fails, the deploys that already succeeded are rolled back to that SHA. Either every deploy lands or none does. Set it to `false` for a non-atomic promotion that leaves whatever succeeded in place. +Either way, finalize holds the environment's recorded pointer (`state..sha` and `state..version`) at its prior value when any in-scope deploy failed or was cancelled, so recorded state matches what the environment is actually running. The per-deployable rows for the deploys that did succeed are still recorded, so a re-run re-plans and retries only the failed deploy. The environment pointer advances only when every in-scope deploy succeeds. + This is a different knob from `rollout.fail_fast` and `rollout.max_parallel` on a `deploys[].rollout` entry in the manifest. Those two control the GitHub Actions `strategy:` block on that one deploy's matrix job (whether one failed matrix leg cancels the rest, and how many legs run in parallel); `rollback_on_failure` controls what promote does across deploys after a failure. See [the deploy strategy block](/cascade/reference/generated-workflows/#the-deploy-strategy-block) for the manifest-to-YAML mapping. Everything else under `rollout` (`type`, `canary`, `blue_green`) is reserved and has no effect on generated output. ## Selective deploys diff --git a/e2e/harness/multistep.go b/e2e/harness/multistep.go index 627b140d..827083ed 100644 --- a/e2e/harness/multistep.go +++ b/e2e/harness/multistep.go @@ -295,6 +295,14 @@ type PromoteStep struct { // fails, every deploy that already succeeded is rolled back to the SHA // previously deployed in the target env (preflight's rollback_sha). RollbackOnFailure bool `yaml:"rollback_on_failure,omitempty"` + // SyncStateOnFailure re-reads state from Gitea after an ExpectFailure promote + // so a step can assert what finalize actually wrote on a failed run. The + // finalize job runs under always(), so it commits state even when a deploy + // failed; without this the harness keeps the last successful sync and cannot + // observe that write. It is opt-in and only consulted alongside ExpectFailure, + // so every existing expect_failure scenario is byte-identical. A scenario sets + // it to prove the env pointer did NOT advance over a failed deploy. + SyncStateOnFailure bool `yaml:"sync_state_on_failure,omitempty"` } // RollbackStep defines a rollback action: a workflow_dispatch of the diff --git a/e2e/harness/runner.go b/e2e/harness/runner.go index fbe9881e..f824ce72 100644 --- a/e2e/harness/runner.go +++ b/e2e/harness/runner.go @@ -1317,6 +1317,15 @@ func (r *Runner) executePromote(ctx context.Context, promote *PromoteStep, confi if promote.ExpectFailure { if result.Conclusion == "failure" { r.t.Log(" Promote: workflow failed as expected") + // The finalize job runs under always(), so it commits state even on a + // failed run. Opt-in re-sync lets the step assert what finalize actually + // wrote (for example that the env pointer did NOT advance over a failed + // deploy). Non-fatal, mirroring the success-path sync below. + if promote.SyncStateOnFailure { + if err := r.syncStateFromGitea(ctx, config); err != nil { + r.t.Logf(" Warning: failed to sync state from Gitea after expected failure: %v", err) + } + } return nil } return fmt.Errorf("expected promote to fail but it succeeded") diff --git a/e2e/scenarios/promote/promote-rollback-runtime.yaml b/e2e/scenarios/promote/promote-rollback-runtime.yaml index 0c0e5178..50be9a10 100644 --- a/e2e/scenarios/promote/promote-rollback-runtime.yaml +++ b/e2e/scenarios/promote/promote-rollback-runtime.yaml @@ -167,13 +167,18 @@ steps: infradeploy: success appdeploy: success - - name: "Promote with rollback_on_failure: app fails, infra rolls back" + - name: "Promote with rollback_on_failure: app fails, infra rolls back, test pointer held" action: promote promote: mode: cascade target: test rollback_on_failure: true expect_failure: true + # Re-read state after the failed run so the assertion below observes what + # finalize actually committed (finalize runs under always()). Without this + # the harness keeps the last successful sync and the state assertion is not + # falsifiable. + sync_state_on_failure: true expect: jobs: preflight: success @@ -188,3 +193,12 @@ steps: # with infra deployed and app failed). infradeploy: success appdeploy: failure + state: + # The env pointer must NOT advance to commit2: app terminally failed and + # rollback-infra redeployed test at commit1, so recording commit2 would + # assert a version the environment is not running. Finalize holds the + # pointer at commit1 (the sha test held before this promote). Before the + # promote-finalize gate this recorded commit2, the state/reality inversion + # this scenario now guards. + test: + sha: commit1 diff --git a/internal/promote/command_finalize.go b/internal/promote/command_finalize.go index 5a0f83d4..8819b177 100644 --- a/internal/promote/command_finalize.go +++ b/internal/promote/command_finalize.go @@ -234,11 +234,13 @@ func parseExpectedDeploys(raw string) ([]string, error) { // disagree (or the result wiring is absent), so recording the promotion would // advance env state over a no-op. That state write is refused. // -// A reported failure or cancellation proceeds: the deploy ran and lost, the run -// is already red, and rollback_on_failure owns that path; this gate must not -// change failure semantics. An empty expected set also proceeds: a promotion -// whose trigger filters matched no changes legitimately deploys nothing while -// still advancing the env pointer. +// A reported failure or cancellation does NOT abort here: the per-deploy success +// rows are still worth recording, so the state write proceeds. The env-pointer +// advance that a failed deploy must hold is gated downstream, in the finalizer's +// updateState (inScopeDeployFailed), which leaves state..sha/version +// unchanged while still recording the deploys that succeeded. An empty expected +// set also proceeds: a promotion whose trigger filters matched no changes +// legitimately deploys nothing while still advancing the env pointer. func gateOnExpectedDeploys(expected []string, results map[string]string) error { if len(expected) == 0 { return nil diff --git a/internal/promote/component_resolved_config_test.go b/internal/promote/component_resolved_config_test.go index a0313aa1..4b5d6a0c 100644 --- a/internal/promote/component_resolved_config_test.go +++ b/internal/promote/component_resolved_config_test.go @@ -232,9 +232,10 @@ func TestNewFinalizer_ComponentResolvedDeploys(t *testing.T) { // TestGateOnExpectedDeploys covers the promote-side fail-safe: when preflight // planned deploys but none of them reported any result (every gate skipped or // the result wiring is absent), nothing deployed and the state write must be -// refused. A reported failure or cancellation keeps the existing loud path -// (rollback_on_failure and the red run own it); an empty plan is a legitimate -// no-deploy promotion (trigger filters matched nothing) and always proceeds. +// refused. A reported failure or cancellation does not abort this gate (the +// env-pointer hold is applied downstream in updateState, while per-deploy +// successes are still recorded); an empty plan is a legitimate no-deploy +// promotion (trigger filters matched nothing) and always proceeds. func TestGateOnExpectedDeploys(t *testing.T) { t.Run("all expected deploys skipped aborts", func(t *testing.T) { err := gateOnExpectedDeploys([]string{"api-svc"}, map[string]string{"api-svc": "skipped"}) @@ -254,7 +255,7 @@ func TestGateOnExpectedDeploys(t *testing.T) { )) }) - t.Run("a failure keeps the existing failure path", func(t *testing.T) { + t.Run("a failure does not abort this gate (env-pointer hold is downstream)", func(t *testing.T) { require.NoError(t, gateOnExpectedDeploys( []string{"api-svc"}, map[string]string{"api-svc": "failure"}, diff --git a/internal/promote/finalize.go b/internal/promote/finalize.go index b6b93b41..50f7cebd 100644 --- a/internal/promote/finalize.go +++ b/internal/promote/finalize.go @@ -126,6 +126,32 @@ func (f *Finalizer) SetDeployResult(name, result string) { f.deployResults[name] = result } +// inScopeDeployFailed reports whether any deploy this finalize is responsible +// for terminally failed or was cancelled, returning the first such deploy's name +// (sorted for a deterministic message) and true. The env-pointer advance is +// gated on it: a promotion must not record state..sha/version at the new +// commit when a deploy that was meant to land there did not succeed. +// +// A deploy the promotion did not run reports "skipped" (its generated job gate +// evaluated false) and never holds the pointer; only a terminal "failure" or +// "cancelled" (the reusable deploy workflow's conclusion after its own retries) +// does. The deployResults map is already scoped to this finalize's deploys: the +// finalize command reads DEPLOY_RESULT_ for the resolved deploy names, and +// a component-scoped finalize resolves only its own component's deploys, so the +// same enumeration that drives the failure results is the in-scope set here. +func (f *Finalizer) inScopeDeployFailed() (string, bool) { + failed := "" + for name, result := range f.deployResults { + if result != "failure" && result != "cancelled" { + continue + } + if failed == "" || name < failed { + failed = name + } + } + return failed, failed != "" +} + // SetPromotionResult sets the promotion result from preflight output. // This contains information about which environments to update and release actions. func (f *Finalizer) SetPromotionResult(pr *PromotionResult) { @@ -241,8 +267,25 @@ func (f *Finalizer) updateState() { f.cicdFile.State = make(map[string]*config.EnvState) } + // Gate the environment-pointer advance on in-scope deploy success. When a + // deploy this finalize is responsible for terminally failed or was cancelled, + // advancing state..sha/version would record the new commit as live while + // the environment is not actually running it (with rollback_on_failure, the + // default, the auto-rollback job redeploys the OLD sha), inverting state and + // reality. This mirrors the rollback finalize's gateOnDeployResults, except a + // promote holds ONLY the env pointer: the per-deploy success rows below are + // still recorded, so a partial success is not lost and a re-dispatch retries + // only the failed deploy. + failedDeploy, envPointerHeld := f.inScopeDeployFailed() + if envPointerHeld { + fmt.Printf( + "Deploy %q did not succeed; environment %q state pointer left unchanged (successful deploys still recorded)\n", + failedDeploy, f.targetEnv, + ) + } + // Update environment state from promotion result - if f.promotionResult != nil { + if f.promotionResult != nil && !envPointerHeld { for _, promo := range f.promotionResult.Promotions { if f.cicdFile.State[promo.Environment] == nil { f.cicdFile.State[promo.Environment] = &config.EnvState{} diff --git a/internal/promote/finalize_test.go b/internal/promote/finalize_test.go index a22bfed0..660fddbf 100644 --- a/internal/promote/finalize_test.go +++ b/internal/promote/finalize_test.go @@ -53,13 +53,18 @@ func TestFinalize_UpdatesStateForSuccessfulDeploys(t *testing.T) { cicdFile, err := config.ParseManifestFile(configPath, config.DefaultManifestKey) require.NoError(t, err) - // Check state was updated + // The env pointer must NOT advance: app terminally failed, so recording the + // promotion at the new sha would assert a version the environment is not + // running. The env holds at its prior (empty) pointer. Previously this test + // asserted testState.SHA == "abc123", which encoded the state/reality + // inversion this fix closes. testState := cicdFile.State["test"] require.NotNil(t, testState) - require.Equal(t, "abc123", testState.SHA) - require.Equal(t, "v1.0.0-1", testState.Version) + require.Equal(t, "", testState.SHA, "env pointer held: an in-scope deploy failed") + require.Equal(t, "", testState.Version, "env version held: an in-scope deploy failed") - // Check deploy states - success should be updated, failure should not + // The per-deploy SUCCESS row is still recorded (a partial success is not + // lost); the failed deploy is not. require.NotNil(t, testState.Deploys) require.NotNil(t, testState.Deploys["infra"], "infra deploy should be recorded (success)") require.Equal(t, "abc123", testState.Deploys["infra"].SHA) @@ -595,8 +600,13 @@ func TestFinalize_CancelledDeploys(t *testing.T) { cicdFile, err := config.ParseManifestFile(configPath, config.DefaultManifestKey) require.NoError(t, err) - // Cancelled deploy should not have state - require.Nil(t, cicdFile.State["test"].Deploys["infra"], "cancelled deploy should not be recorded") + // The only in-scope deploy was cancelled, so nothing landed: the env pointer + // is held at its prior (empty) value and the cancelled deploy is not recorded. + testState := cicdFile.State["test"] + require.NotNil(t, testState) + require.Equal(t, "", testState.SHA, "env pointer held: the only in-scope deploy was cancelled") + require.Equal(t, "", testState.Version) + require.Nil(t, testState.Deploys["infra"], "cancelled deploy should not be recorded") } // TestFinalize_NoReleaseActionDoesNotUpdateLatestRelease ensures that @@ -941,3 +951,159 @@ func TestUpdateState_NoSnapshotOnSameSHA(t *testing.T) { require.NotNil(t, testState) require.Empty(t, testState.Previous) } + +// TestFinalize_HoldsEnvPointerOnInScopeDeployFailure proves the env pointer +// (state..sha/version) is NOT advanced when an in-scope deploy terminally +// failed. Advancing it would record the new commit as live while +// rollback_on_failure (default true) redeploys the OLD sha, inverting state and +// reality. The prior deploy that succeeded is still recorded per-deployable, so +// a re-dispatch retries only the failed one. +func TestFinalize_HoldsEnvPointerOnInScopeDeployFailure(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "manifest.yaml") + + initialConfig := `ci: + config: + environments: [dev, uat, prod] + deploys: + - name: infra + workflow: .github/workflows/deploy-infra.yaml + - name: app + workflow: .github/workflows/deploy-app.yaml + state: + prod: + sha: oldsha111 + version: v1.0.0 + committed_at: "2026-01-01T10:00:00Z" + committed_by: alice +` + require.NoError(t, os.WriteFile(configPath, []byte(initialConfig), 0644)) + + fin, err := NewFinalizer(configPath, "prod") + require.NoError(t, err) + fin.SetActor("bob") + // infra succeeded, app terminally failed: a partial success. + fin.SetDeployResult("infra", "success") + fin.SetDeployResult("app", "failure") + fin.SetPromotionResult(&PromotionResult{ + Promotions: []EnvPromotion{{ + Environment: "prod", + SHA: "newsha222", + Version: "v1.1.0", + }}, + }) + + require.NoError(t, fin.Run()) + + cicdFile, err := config.ParseManifestFile(configPath, config.DefaultManifestKey) + require.NoError(t, err) + + prodState := cicdFile.State["prod"] + require.NotNil(t, prodState) + // The env pointer must stay at the OLD sha/version: the deploy did not land. + require.Equal(t, "oldsha111", prodState.SHA, + "env pointer must not advance when an in-scope deploy failed") + require.Equal(t, "v1.0.0", prodState.Version, + "env version must not advance when an in-scope deploy failed") + require.Equal(t, "alice", prodState.CommittedBy, + "a held env keeps its prior audit attribution") + require.Empty(t, prodState.Previous, + "no transition happened, so nothing is pushed to the deploy-history ring") + + // The per-deploy SUCCESS row is still recorded; the partial success is not lost. + require.NotNil(t, prodState.Deploys["infra"], + "a deploy that succeeded is still recorded even when a sibling failed") + require.Equal(t, "newsha222", prodState.Deploys["infra"].SHA) + require.Nil(t, prodState.Deploys["app"], + "the failed deploy is not recorded") +} + +// TestFinalize_HoldsEnvPointerRollbackOnFailureDisabled proves the env pointer +// is held on a failed deploy regardless of rollback_on_failure: the recorded +// state must match what the environment is actually running, whether or not an +// auto-rollback also fires. +func TestFinalize_HoldsEnvPointerRollbackOnFailureDisabled(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "manifest.yaml") + + initialConfig := `ci: + config: + environments: [dev, prod] + deploys: + - name: app + workflow: .github/workflows/deploy-app.yaml + rollback_on_failure: false + state: + prod: + sha: oldsha111 + version: v1.0.0 +` + require.NoError(t, os.WriteFile(configPath, []byte(initialConfig), 0644)) + + fin, err := NewFinalizer(configPath, "prod") + require.NoError(t, err) + fin.SetDeployResult("app", "failure") + fin.SetPromotionResult(&PromotionResult{ + Promotions: []EnvPromotion{{ + Environment: "prod", + SHA: "newsha222", + Version: "v1.1.0", + }}, + }) + + require.NoError(t, fin.Run()) + + cicdFile, err := config.ParseManifestFile(configPath, config.DefaultManifestKey) + require.NoError(t, err) + + prodState := cicdFile.State["prod"] + require.NotNil(t, prodState) + require.Equal(t, "oldsha111", prodState.SHA, + "env pointer held at old sha even without auto-rollback") + require.Equal(t, "v1.0.0", prodState.Version) +} + +// TestFinalize_AdvancesEnvPointerWhenAllInScopeSucceed proves the fix does not +// change the happy path: with every in-scope deploy successful, the env pointer +// advances exactly as before. +func TestFinalize_AdvancesEnvPointerWhenAllInScopeSucceed(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "manifest.yaml") + + initialConfig := `ci: + config: + environments: [dev, prod] + deploys: + - name: infra + workflow: .github/workflows/deploy-infra.yaml + - name: app + workflow: .github/workflows/deploy-app.yaml + state: + prod: + sha: oldsha111 + version: v1.0.0 +` + require.NoError(t, os.WriteFile(configPath, []byte(initialConfig), 0644)) + + fin, err := NewFinalizer(configPath, "prod") + require.NoError(t, err) + fin.SetDeployResult("infra", "success") + fin.SetDeployResult("app", "success") + fin.SetPromotionResult(&PromotionResult{ + Promotions: []EnvPromotion{{ + Environment: "prod", + SHA: "newsha222", + Version: "v1.1.0", + }}, + }) + + require.NoError(t, fin.Run()) + + cicdFile, err := config.ParseManifestFile(configPath, config.DefaultManifestKey) + require.NoError(t, err) + + prodState := cicdFile.State["prod"] + require.NotNil(t, prodState) + require.Equal(t, "newsha222", prodState.SHA) + require.Equal(t, "v1.1.0", prodState.Version) +}