fix(hotfix): converge the release on finalize rerun instead of skipping it

docs/src/content/docs/guides/hotfix.md

@@ -21,6 +21,8 @@ gh workflow run cascade-hotfix.yaml \
 
 The `plan` job fetches env branches and tags and runs `cascade hotfix plan`. The `apply` job then cherry-picks the fix onto a per-environment integration branch and opens a resolution pull request labeled `cascade-hotfix` (or `cascade-hotfix-conflict` if the cherry-pick collides). Merging that pull request runs build, deploy, and finalize, which write the diverged state.
 
+Finalize is rerun-safe. It records the diverged state on trunk, then creates the hotfix tag and release. If the run fails partway (for example the release API errors after the state commit), rerun the finalize job: the rerun completes the missing tag or release with the version already recorded, without double-applying state or allocating a new version.
+
 ## Environment branches and the stale-branch self-heal
 
 When an environment needs to diverge, the fix is staged on `env/<env>` (for example `env/test`), created on demand at the environment's recorded state SHA. The cherry-pick itself lands on `hotfix/<env>/<short-sha>`, based on `env/<env>`.

e2e/scenarios/hotfix/hotfix-clean-apply.yaml

@@ -8,6 +8,9 @@ description: |
   - hotfix_apply cherry-picks cleanly onto env/test, opens a cascade-hotfix PR
   - merge_pr squash-merges the PR
   - hotfix_merged records the finalized hotfix state
+  - a second hotfix_merged replay of the same event (a finalize rerun) stays
+    green, converges the release step through the idempotency gate, and does
+    not double-apply state
   - dev and prod state are untouched throughout
 
 config:
@@ -131,6 +134,31 @@ steps:
           ref: "env/test"
           base_sha: commit1
           patches: [commit2]
+          version: "v0.1.0-rc.0.hotfix.1"
+        dev:
+          sha: commit1
+        prod:
+          sha: commit1
+
+  - name: "Re-run finalize for test (rerun converges, does not double-apply)"
+    action: hotfix_merged
+    hotfix_merged:
+      target_env: test
+    expect:
+      # Replays the identical merged-PR event, so finalize lands on its
+      # idempotency gate: state already records the merge SHA. The rerun must
+      # stay green while converging the release step (a find-or-create update,
+      # which is a synthetic no-op against the gitea backend) and must not
+      # double-apply state: ref, base_sha, patches, and the allocated hotfix
+      # version stay exactly as the first finalize recorded them. The
+      # release-object convergence itself is GitHub-only behavior, exercised by
+      # the real-GitHub validation fleet.
+      state:
+        test:
+          ref: "env/test"
+          base_sha: commit1
+          patches: [commit2]
+          version: "v0.1.0-rc.0.hotfix.1"
         dev:
           sha: commit1
         prod:

internal/hotfix/finalize.go

@@ -402,8 +402,12 @@ func (f *Finalizer) SetBuildResult(name, result string) {
 // first.
 //
 // Finalize is idempotent on identical inputs: a rerun after the state already
-// records the merge SHA is a no-op that neither double-applies patches nor
-// re-snapshots Previous.
+// records the merge SHA neither double-applies patches nor re-snapshots
+// Previous. Because the state commit lands before the tag/release step, a rerun
+// additionally converges the release: it re-invokes the release step with the
+// recorded version (find-or-create, tolerant of an existing tag), so a run that
+// failed between the state commit and the release creation is completed by its
+// rerun rather than silently reported as done with the release missing.
 func (f *Finalizer) Finalize(targetEnv, mergeSHA string, fixSHAs []string, baseSHA string) error {
 	if len(fixSHAs) == 0 {
 		return fmt.Errorf("no fix commits supplied; finalize needs at least one trunk commit")
@@ -459,9 +463,19 @@ func (f *Finalizer) Finalize(targetEnv, mergeSHA string, fixSHAs []string, baseS
 	branch := f.envBranch(targetEnv)
 
 	// Idempotency gate: if state already records the merge SHA, finalize already
-	// ran for these inputs. Re-running must not double-apply.
+	// committed the state marker for these inputs. Re-running must not
+	// double-apply state, but it must not blind-return success either: the state
+	// commit precedes tag/release creation below, so a rerun can land here with
+	// the release step never completed (a release-API failure reddened the prior
+	// run after the marker was pushed). Converge instead of skip: re-invoke the
+	// release step idempotently with the version the prior run recorded, so the
+	// rerun completes the missing tag/release. When everything already exists
+	// the re-invocation is a no-op-shaped update.
 	if prior.SHA == mergeSHA {
-		return nil
+		if f.dryRun {
+			return nil
+		}
+		return f.convergeRelease(cfg, targetEnv, mergeSHA, prior, fixSHAs[0])
 	}
 
 	// Cross-check the merge SHA equals the env-branch tip.
@@ -516,14 +530,33 @@ func (f *Finalizer) Finalize(targetEnv, mergeSHA string, fixSHAs []string, baseS
 	}
 
 	// Create the hotfix tag and release object. The release body references the
-	// first carried commit as the representative fix SHA.
-	if err := f.createRelease(cfg, targetEnv, mergeSHA, hotfixVersion, fixSHAs[0], baseVersion); err != nil {
+	// first carried commit as the representative fix SHA. This runs AFTER the
+	// state commit: the recorded version pins the allocation, so a failure here
+	// is recovered by a rerun that re-creates the same tag through the
+	// idempotency-gate convergence above, never by re-allocating a new version.
+	if err := f.createRelease(cfg, targetEnv, mergeSHA, hotfixVersion, fixSHAs[0], baseVersion, release.ActionCreate); err != nil {
 		return err
 	}
 
 	return nil
 }
 
+// convergeRelease completes the tag/release step for a finalize whose state
+// marker is already recorded on trunk. The hotfix version is the one the prior
+// run allocated and committed (prior.Version), so a rerun converges on the same
+// tag rather than minting a new one; the base version for the release body is
+// recovered from the Previous ring snapshot that same run pushed. The release
+// step runs as a find-or-create update, so it creates whatever is missing (git
+// tag, release object, prerelease promotion) and is a no-op-shaped update when
+// everything already exists.
+func (f *Finalizer) convergeRelease(cfg *config.TrunkConfig, targetEnv, mergeSHA string, prior *config.EnvState, fixSHA string) error {
+	var baseVersion string
+	if len(prior.Previous) > 0 {
+		baseVersion = prior.Previous[0].Version
+	}
+	return f.createRelease(cfg, targetEnv, mergeSHA, prior.Version, fixSHA, baseVersion, release.ActionUpdate)
+}
+
 // applyHotfixState applies the hotfix state mutation for targetEnv onto cicd
 // using pre-computed values, so it can be re-applied against freshly fetched
 // trunk bytes inside the optimistic-lock retry loop. It is idempotent: when the
@@ -760,7 +793,14 @@ func (f *Finalizer) recordSubstates(state *config.EnvState, sha, ver, timestamp
 // createRelease creates the hotfix tag and release object. For a prerelease-env
 // target the release is promoted to a GitHub prerelease, superseding the env's
 // current prerelease object; for other envs it stays a draft.
-func (f *Finalizer) createRelease(cfg *config.TrunkConfig, targetEnv, sha, hotfixVersion, fixSHA, baseVersion string) error {
+//
+// action selects the release verb: the first finalize run passes ActionCreate
+// (the release cannot pre-exist, and create skips the find-release lookup and
+// its eventual-consistency retry window); the idempotency-gate convergence
+// passes ActionUpdate, whose find-or-create shape completes a partially created
+// release (tag creation treats an existing tag as success) instead of erroring
+// or duplicating it.
+func (f *Finalizer) createRelease(cfg *config.TrunkConfig, targetEnv, sha, hotfixVersion, fixSHA, baseVersion string, action release.Action) error {
 	mgr, err := f.resolveReleaseManager()
 	if err != nil {
 		return err
@@ -769,7 +809,7 @@ func (f *Finalizer) createRelease(cfg *config.TrunkConfig, targetEnv, sha, hotfi
 	body := fmt.Sprintf("Hotfix based on %s, carries trunk commit %s.", baseVersion, short(fixSHA))
 
 	created, err := mgr.Manage(release.Options{
-		Action:      release.ActionCreate,
+		Action:      action,
 		Environment: targetEnv,
 		SHA:         sha,
 		Tag:         hotfixVersion,

internal/hotfix/finalize_test.go

@@ -1,6 +1,7 @@
 package hotfix
 
 import (
+	"errors"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -513,6 +514,149 @@ func TestFinalize_Idempotent_Rerun(t *testing.T) {
 	}
 }
 
+// TestFinalize_RerunAfterReleaseFailure_CreatesRelease reproduces the
+// partial-failure interleaving where finalize reports green with the release
+// permanently missing: the first run commits the state marker to trunk and the
+// release API then fails, so the job goes red with state already recording the
+// merge SHA. The rerun lands on the idempotency gate; it must complete the
+// missing tag/release with the version the first run recorded rather than
+// early-return success and leave the release absent forever.
+func TestFinalize_RerunAfterReleaseFailure_CreatesRelease(t *testing.T) {
+	newScratchRepo(t)
+	base := commitFile(t, "a.txt", "one", "first")
+	fix := commitFile(t, "b.txt", "two", "fix")
+	runGit(t, "branch", "env/test", base)
+	runGit(t, "checkout", "env/test")
+	merge := commitFile(t, "c.txt", "fixed", "cp")
+	runGit(t, "checkout", "main")
+
+	manifest := writeFinalizeManifest(t, []string{"dev", "test", "prod"}, map[string]envFixture{
+		"dev":  {sha: fix, version: "v1.4.0-rc.2"},
+		"test": {sha: base, version: "v1.4.0-rc.2"},
+		"prod": {sha: base, version: "v1.4.0-rc.2"},
+	})
+
+	// First run: the state marker lands, then the release API blips.
+	pusher1 := &recordingPusher{}
+	rm1 := &stubReleaseManager{err: errors.New("API error 503: upstream blip")}
+	f1 := newFinalizer(t, manifest,
+		WithReleaseManager(rm1),
+		WithTagLister(stubTagLister{}),
+		WithStatePusher(pusher1),
+	)
+	if err := f1.Finalize("test", merge, []string{fix}, base); err == nil {
+		t.Fatal("first Finalize must surface the release failure")
+	}
+	if pusher1.calls != 1 {
+		t.Fatalf("first run state pushes = %d, want 1", pusher1.calls)
+	}
+	if st := loadState(t, manifest, "test"); st.SHA != merge {
+		t.Fatalf("state marker sha = %q, want merge SHA %q recorded before the release step", st.SHA, merge)
+	}
+
+	// Rerun with the API healthy: the idempotency gate must converge the missing
+	// tag/release instead of skipping it.
+	pusher2 := &recordingPusher{}
+	rm2 := &stubReleaseManager{}
+	f2 := newFinalizer(t, manifest,
+		WithReleaseManager(rm2),
+		WithTagLister(stubTagLister{}),
+		WithStatePusher(pusher2),
+	)
+	if err := f2.Finalize("test", merge, []string{fix}, base); err != nil {
+		t.Fatalf("rerun Finalize: %v", err)
+	}
+
+	if len(rm2.calls) == 0 {
+		t.Fatal("rerun skipped the release step; the hotfix tag/release stays permanently missing")
+	}
+	got := rm2.calls[0]
+	if got.Action != release.ActionUpdate {
+		t.Errorf("rerun action = %q, want find-or-create update", got.Action)
+	}
+	if got.Tag != "v1.4.0-rc.2.hotfix.1" {
+		t.Errorf("rerun tag = %q, want the recorded v1.4.0-rc.2.hotfix.1, not a re-allocation", got.Tag)
+	}
+	if got.SHA != merge {
+		t.Errorf("rerun release SHA = %q, want merge SHA %q", got.SHA, merge)
+	}
+	if !got.CreateTag {
+		t.Error("rerun must still materialize the git tag")
+	}
+	if !strings.Contains(got.Changelog, "based on v1.4.0-rc.2,") {
+		t.Errorf("rerun body should reference the base version from the Previous ring: %q", got.Changelog)
+	}
+	if !strings.Contains(got.Changelog, short(fix)) {
+		t.Errorf("rerun body should reference the carried trunk commit: %q", got.Changelog)
+	}
+	// test is the prerelease env (second from top), so the converged release is
+	// still promoted to a GitHub prerelease.
+	if len(rm2.calls) != 2 || rm2.calls[1].Action != release.ActionPrerelease {
+		t.Errorf("rerun release actions = %+v, want [update prerelease]", releaseActions(rm2.calls))
+	}
+
+	// The rerun neither re-pushes state nor double-applies it.
+	if pusher2.calls != 0 {
+		t.Errorf("rerun state pushes = %d, want 0", pusher2.calls)
+	}
+	st := loadState(t, manifest, "test")
+	if len(st.Patches) != 1 {
+		t.Errorf("rerun double-applied patches: %v", st.Patches)
+	}
+	if len(st.Previous) != 1 {
+		t.Errorf("rerun double-snapshotted Previous: %d entries", len(st.Previous))
+	}
+	if st.Version != "v1.4.0-rc.2.hotfix.1" {
+		t.Errorf("rerun changed version to %q, want stable v1.4.0-rc.2.hotfix.1", st.Version)
+	}
+}
+
+// TestFinalize_RerunDryRun_TouchesNothing guards the dry-run contract on the
+// idempotency-gate path: a dry-run rerun over already-recorded state must not
+// invoke the release API at all.
+func TestFinalize_RerunDryRun_TouchesNothing(t *testing.T) {
+	newScratchRepo(t)
+	base := commitFile(t, "a.txt", "one", "first")
+	fix := commitFile(t, "b.txt", "two", "fix")
+	runGit(t, "branch", "env/test", base)
+	runGit(t, "checkout", "env/test")
+	merge := commitFile(t, "c.txt", "fixed", "cp")
+	runGit(t, "checkout", "main")
+
+	manifest := writeFinalizeManifest(t, []string{"dev", "test", "prod"}, map[string]envFixture{
+		"dev":  {sha: fix, version: "v1.4.0-rc.2"},
+		"test": {sha: merge, version: "v1.4.0-rc.2.hotfix.1"},
+		"prod": {sha: base, version: "v1.4.0-rc.2"},
+	})
+
+	pusher := &recordingPusher{}
+	rm := &stubReleaseManager{}
+	f := newFinalizer(t, manifest,
+		WithReleaseManager(rm),
+		WithTagLister(stubTagLister{}),
+		WithStatePusher(pusher),
+		WithFinalizeDryRun(true),
+	)
+	if err := f.Finalize("test", merge, []string{fix}, base); err != nil {
+		t.Fatalf("dry-run rerun Finalize: %v", err)
+	}
+	if len(rm.calls) != 0 {
+		t.Errorf("dry-run rerun made %d release calls, want 0", len(rm.calls))
+	}
+	if pusher.calls != 0 {
+		t.Errorf("dry-run rerun made %d state pushes, want 0", pusher.calls)
+	}
+}
+
+// releaseActions projects the Manage options to their actions for messages.
+func releaseActions(calls []release.Options) []release.Action {
+	actions := make([]release.Action, len(calls))
+	for i, c := range calls {
+		actions[i] = c.Action
+	}
+	return actions
+}
+
 // TestFinalize_StateWriteTargetsTrunkBranch guards that the manifest state write
 // targets the configured trunk branch, not the env branch the hotfix PR merged
 // into. The finalize job runs on the merged pull_request event whose base is

internal/release/release.go

@@ -724,6 +724,15 @@ func (m *Manager) update(opts Options) (*Result, error) {
 		return m.create(opts)
 	}
 
+	// On a non-GitHub host (the Gitea e2e backend) the release-object endpoints
+	// reject the GitHub release shape and Bearer auth, so findRelease and the
+	// PATCH below cannot run. Mirror create(): delegate so the tag is materialized
+	// and a synthetic success is returned. Real-GitHub release-object convergence
+	// is covered by the finalize rerun unit test and the live fleet.
+	if !isGitHubHost(m.baseURL) {
+		return m.create(opts)
+	}
+
 	existing, err := m.findRelease(opts.Tag, opts.SHA)
 	if err != nil {
 		return nil, err

internal/release/release_test.go

@@ -461,7 +461,7 @@ func TestManager_Update_ExistingRelease(t *testing.T) {
 
 	manager := &Manager{
 		client:  server.Client(),
-		baseURL: server.URL,
+		baseURL: server.URL + "/github", // marks as a GitHub host so update() exercises the real find+PATCH path (non-GitHub hosts short-circuit to create())
 		token:   "test-token",
 		repo:    "owner/repo",
 	}

internal/release/update_nongithub_test.go

@@ -0,0 +1,39 @@
+package release
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// failRoundTripper fails the test if any HTTP request is attempted, proving a
+// code path short-circuits before touching the network.
+type failRoundTripper struct{ t *testing.T }
+
+func (f failRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
+	f.t.Fatalf("unexpected HTTP request to %s: update() must short-circuit on a non-GitHub host", r.URL)
+	return nil, nil
+}
+
+// TestUpdate_SkipsReleaseObjectOnNonGitHubHost pins the guard that lets the
+// hotfix finalize convergence rerun (which routes through update via
+// ActionUpdate) succeed on the Gitea e2e backend. Gitea's release-object
+// endpoints reject the GitHub release shape and Bearer auth, so update() must
+// mirror create() and short-circuit before findRelease. Without the guard this
+// test fails: update() would issue the findRelease request and hit the
+// fail-on-call transport. Real-GitHub release-object convergence is covered by
+// the finalize rerun unit test and the live fleet.
+func TestUpdate_SkipsReleaseObjectOnNonGitHubHost(t *testing.T) {
+	m := &Manager{
+		client:  &http.Client{Transport: failRoundTripper{t}},
+		baseURL: "http://gitea.local",
+		token:   "t",
+		repo:    "owner/repo",
+	}
+
+	res, err := m.update(Options{Tag: "v1.2.3", SHA: "deadbeef", CreateTag: true, Environment: "test"})
+
+	require.NoError(t, err, "update() on a non-GitHub host must return synthetic success, not a release-API error")
+	require.NotNil(t, res)
+}