diff --git a/CHANGELOG.md b/CHANGELOG.md index c8b7d98..ef487ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,19 @@ A `Migration` section is added to any release that bumps `schema_version`. ### Fixed +- **release:** A release cut that materializes a git tag now fails closed when the + tag already exists at a different commit, instead of treating the + `422 reference already exists` response as an unconditional success. The old + behavior was a silent no-op that left the tag frozen on the stale commit while + the cut reported success. A tag already pointing at the target commit stays the + genuinely idempotent case. + +- **orchestrate:** Version derivation now advances the release-candidate counter + past any tag that already exists at a different commit, so a stalled or + rolled-back recorded state can no longer mint an rc number already published at + another commit. A candidate already pointing at the current commit is reused + unchanged, keeping the no-collision path identical. + - **ci:** The Release workflow's `Test` job now installs the SHA-pinned `actionlint` binary before `go test ./...`, matching the unit-test lanes in `validate.yaml` and `pr.yaml`. The emitted-workflow enforcement guard diff --git a/internal/orchestrate/orchestrator.go b/internal/orchestrate/orchestrator.go index cd27cb2..7ade7a7 100644 --- a/internal/orchestrate/orchestrator.go +++ b/internal/orchestrate/orchestrator.go @@ -592,7 +592,77 @@ func (o *Orchestrator) calculateVersion() (string, error) { return "", fmt.Errorf("calculating version: %w", err) } - return nextVersion.String(), nil + // A stale or rolled-back recorded state can drive the rc counter to a value + // that already exists as a tag at a different commit; resolve any such + // collision before returning so a fresh cut never reuses a published rc. + resolved, err := o.resolveRCCollision(nextVersion) + if err != nil { + return "", err + } + + return resolved.String(), nil +} + +// resolveRCCollision advances an rc candidate past any tag of the same name that +// already exists at a DIFFERENT commit than the current HEAD, so a stale, raced, +// or rolled-back recorded state can never re-mint an rc number that was already +// published at another sha (the frozen-rc incident, where a state stuck at rc.0 +// recomputed rc.1 while a v...-rc.1 tag already pointed at an outdated commit). +// +// A tag that already exists AT the current HEAD is the same cut re-running and is +// reused unchanged, keeping convergence idempotent. A candidate with no rc +// segment, or a HEAD that cannot be resolved, leaves the version untouched, so +// the no-collision path stays byte-identical to before. +func (o *Orchestrator) resolveRCCollision(v *version.Version) (*version.Version, error) { + if v == nil || v.PreRelease < 0 { + return v, nil + } + + headSHA, err := o.gitOutput("rev-parse", "HEAD") + if err != nil || headSHA == "" { + // Without a resolvable HEAD there is nothing to compare against; preserve + // the historical behavior rather than fail the whole calculation. + return v, nil + } + + // maxRCAdvance bounds the walk so a pathological run of colliding tags can + // never spin forever; it is far beyond any realistic rc depth for one base. + const maxRCAdvance = 1000 + for i := 0; i < maxRCAdvance; i++ { + tag := v.String() + sha, exists, err := o.tagCommit(tag) + if err != nil { + return nil, fmt.Errorf("checking rc tag collision for %s: %w", tag, err) + } + if !exists || sha == headSHA { + return v, nil + } + log.Debug("rc tag %s already exists at %s (head %s); advancing rc number", + tag, truncateSHA(sha), truncateSHA(headSHA)) + v = v.WithRC(v.PreRelease + 1) + } + + return nil, fmt.Errorf("rc collision resolution exceeded %d advances starting from %s", maxRCAdvance, v.String()) +} + +// tagCommit reports the commit a tag points at in the orchestration's repository +// and whether the tag exists. A missing tag returns ("", false, nil); only an +// unexpected git failure returns an error. Existence is checked first so a +// non-existent tag is distinguished from a genuine git error rather than inferred +// from a rev-list exit code. +func (o *Orchestrator) tagCommit(tag string) (sha string, exists bool, err error) { + listed, err := o.gitOutput("tag", "-l", "--", tag) + if err != nil { + return "", false, err + } + if listed == "" { + return "", false, nil + } + sha, err = o.gitOutput("rev-list", "-n", "1", tag) + if err != nil { + return "", true, err + } + return sha, true, nil } // calculateComponentVersion derives the next version for the orchestration's @@ -635,7 +705,16 @@ func (o *Orchestrator) calculateComponentVersion() (string, error) { return "", fmt.Errorf("calculating version for component %q: %w", o.component, err) } - return nextVersion.String(), nil + // Guard the component's rc counter against a stale-state collision, exactly as + // the single-component path does. The candidate carries the component's strict + // tag grammar, so its String() renders the component-prefixed tag the collision + // check looks up, keeping the check scoped to the component's namespace. + resolved, err := o.resolveRCCollision(nextVersion) + if err != nil { + return "", fmt.Errorf("resolving rc collision for component %q: %w", o.component, err) + } + + return resolved.String(), nil } // calculateChangelogRefs returns the changelog base SHA and previous tag, diff --git a/internal/orchestrate/rc_collision_test.go b/internal/orchestrate/rc_collision_test.go new file mode 100644 index 0000000..6b3943f --- /dev/null +++ b/internal/orchestrate/rc_collision_test.go @@ -0,0 +1,68 @@ +package orchestrate + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stablekernel/cascade/internal/config" +) + +// TestCalculateVersion_RCCollisionAdvancesPastExistingTag is the regression +// guard for the frozen-rc incident: a stale recorded state drove the rc counter +// to a value that already existed as a tag at a different commit, and the cut +// reused it instead of advancing. With state stuck at rc.0 the calculation +// recomputes rc.1; when a v1.2.0-rc.1 tag already points at an earlier commit, +// the result must advance to rc.2 rather than collide. The control subcase (no +// existing tag) proves the no-collision path is unchanged and still yields rc.1. +func TestCalculateVersion_RCCollisionAdvancesPastExistingTag(t *testing.T) { + tests := []struct { + name string + collideAt string // git revision to tag v1.2.0-rc.1 at, or "" for no tag + wantVersion string + }{ + {name: "existing rc.1 at a different commit advances to rc.2", collideAt: "HEAD~1", wantVersion: "v1.2.0-rc.2"}, + {name: "no collision keeps rc.1 unchanged", collideAt: "", wantVersion: "v1.2.0-rc.1"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + repoDir, head := initRepo(t) + + if tt.collideAt != "" { + // Materialize the colliding rc tag at a commit that is NOT HEAD, so + // the guard sees it as a foreign target rather than an idempotent + // re-cut of the same commit. + runGit(t, repoDir, "tag", "v1.2.0-rc.1", tt.collideAt) + } + + // calculateVersion resolves commit ranges through the process working + // directory, so run from the repo like the other orchestrate git tests. + orig, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(repoDir)) + t.Cleanup(func() { require.NoError(t, os.Chdir(orig)) }) + + o := &Orchestrator{ + environment: "dev", + baseDir: repoDir, + cicdFile: &config.CICDFile{ + Config: &config.TrunkConfig{ + Environments: config.EnvNames("dev", "prod"), + }, + State: map[string]*config.EnvState{ + // Stuck at rc.0: the recomputed candidate is rc.1. + "dev": {Version: "v1.2.0-rc.0"}, + "prod": {Version: "v1.2.0", SHA: head}, + }, + }, + } + + got, err := o.calculateVersion() + require.NoError(t, err) + assert.Equal(t, tt.wantVersion, got) + }) + } +} diff --git a/internal/release/coverage_test.go b/internal/release/coverage_test.go index 0407ad7..570a7f3 100644 --- a/internal/release/coverage_test.go +++ b/internal/release/coverage_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -161,11 +162,27 @@ func TestCreateGitTag_SkipsNonGitHubHost(t *testing.T) { } func TestCreateGitTag_AcceptsCreatedAndExists(t *testing.T) { + // A fresh ref create (201) succeeds outright. A 422 (ref already exists) is + // accepted only when the existing tag already points at the requested commit, + // which createGitTag confirms with a follow-up GET; the same-sha case is the + // genuinely idempotent one. A 422 whose existing target DIFFERS is a distinct, + // fail-closed case covered by TestManager_CreateGitTag_ExistingTagDifferentSHA. for _, status := range []int{http.StatusCreated, http.StatusUnprocessableEntity} { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, "POST", r.Method) - assert.Contains(t, r.URL.Path, "/git/refs") - w.WriteHeader(status) + switch { + case r.Method == http.MethodPost: + assert.Contains(t, r.URL.Path, "/git/refs") + w.WriteHeader(status) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/git/refs/tags/"): + // The existing tag points at the same commit the cut targets. + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "ref": r.URL.Path, + "object": map[string]any{"sha": "abc123", "type": "commit"}, + }) + default: + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + } })) m := &Manager{client: server.Client(), baseURL: server.URL + "/github", token: "t", repo: "owner/repo"} err := m.createGitTag("v1.0.0", "abc123") diff --git a/internal/release/release.go b/internal/release/release.go index 15db168..a61e7dc 100644 --- a/internal/release/release.go +++ b/internal/release/release.go @@ -225,19 +225,74 @@ func (m *Manager) createGitTag(tagName, sha string) error { } defer func() { _ = resp.Body.Close() }() - // 201 Created or 422 (already exists) are acceptable if resp.StatusCode == http.StatusCreated { return nil } if resp.StatusCode == http.StatusUnprocessableEntity { - // Tag already exists - this is fine - return nil + // The ref already exists. This is idempotent ONLY when the existing tag + // already points at the requested commit (a convergence rerun re-cutting + // the same tag). When it points at a DIFFERENT commit the ref create was a + // silent no-op that would leave the tag frozen at the old commit, which is + // how a stale or collided release cut previously stranded an rc tag on an + // outdated sha. Resolve the existing target and fail closed on a mismatch + // rather than reporting success. + return m.verifyExistingTagTarget(tagName, sha) } body, _ := io.ReadAll(resp.Body) return fmt.Errorf("create tag failed with status %d: %s", resp.StatusCode, string(body)) } +// gitRef is the GitHub git-data ref shape returned by GET /git/refs/tags/. +// Only the target object's sha is needed to confirm where an existing tag points. +type gitRef struct { + Object struct { + SHA string `json:"sha"` + Type string `json:"type"` + } `json:"object"` +} + +// verifyExistingTagTarget confirms that an already-present tag points at wantSHA. +// It is called only after a ref-create returned 422 (already exists): a match is +// the genuinely-harmless idempotent case, while a mismatch means the tag is +// frozen at a different commit than the release cut targets. An rc tag is +// immutable in cascade's single-flight release model - a fresh cut that needs a +// different commit must take a new rc number, not silently reuse an existing tag +// - so a mismatch fails loudly instead of returning a false success. A target +// that cannot be resolved also fails closed, since success cannot be confirmed. +func (m *Manager) verifyExistingTagTarget(tagName, wantSHA string) error { + req, err := m.newRequest("GET", "/git/refs/tags/"+tagName, nil) + if err != nil { + return err + } + + resp, err := m.client.Do(req) + if err != nil { + return fmt.Errorf("resolving existing tag %s: %w", tagName, err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return fmt.Errorf("tag %s already exists but its target could not be resolved (status %d): %s", tagName, resp.StatusCode, string(body)) + } + + var ref gitRef + if err := json.NewDecoder(resp.Body).Decode(&ref); err != nil { + return fmt.Errorf("decoding existing tag %s ref: %w", tagName, err) + } + + if ref.Object.SHA == "" { + return fmt.Errorf("tag %s already exists but its target sha could not be determined to confirm it points at %s", tagName, wantSHA) + } + + if ref.Object.SHA != wantSHA { + return fmt.Errorf("tag %s already exists at %s but this release cut targets %s; refusing to leave the tag frozen on the stale commit (an rc tag is immutable - a fresh cut needs a new rc number)", tagName, ref.Object.SHA, wantSHA) + } + + return nil +} + // deleteGitTag deletes a git tag func (m *Manager) deleteGitTag(tagName string) error { endpoint := "/git/refs/tags/" + tagName diff --git a/internal/release/release_test.go b/internal/release/release_test.go index 12e8244..0d3a229 100644 --- a/internal/release/release_test.go +++ b/internal/release/release_test.go @@ -361,6 +361,15 @@ func updateTagRecordingServer(t *testing.T, seen *[]string, existingDraft bool, switch { case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/git/refs"): w.WriteHeader(gitRefStatus) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/git/refs/tags/"): + // The 422 idempotency subcase re-cuts a tag that already points at the + // same commit the update targets ("deadbeef"), so the ref resolves to + // that sha and createGitTag treats the existing tag as a harmless match. + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "ref": r.URL.Path, + "object": map[string]any{"sha": "deadbeef", "type": "commit"}, + }) case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/releases/tags/"): if existingDraft { w.WriteHeader(http.StatusOK) @@ -433,6 +442,88 @@ func TestManager_Update_CutsGitTag(t *testing.T) { } } +// existingTagServer answers a tag-only update against a GitHub host. The +// POST /git/refs (ref create) always returns 422 (the tag already exists), and +// the follow-up GET /git/refs/tags/ reports the tag pointing at +// existingSHA. It records every method+path so a test can assert the resolve +// GET fired. +func existingTagServer(t *testing.T, seen *[]string, existingSHA string) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + *seen = append(*seen, r.Method+" "+r.URL.Path) + switch { + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/git/refs"): + w.WriteHeader(http.StatusUnprocessableEntity) + _ = json.NewEncoder(w).Encode(map[string]any{"message": "Reference already exists"}) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/git/refs/tags/"): + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "ref": r.URL.Path, + "object": map[string]any{"sha": existingSHA, "type": "commit"}, + }) + default: + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(GitHubRelease{}) + } + })) +} + +// TestManager_CreateGitTag_ExistingTagDifferentSHA is the regression guard for +// the frozen-rc defect: a release cut whose tag already exists at a DIFFERENT +// commit must not report success and leave the tag stranded on the stale commit. +// Before the fix a 422 from the ref-create was swallowed as harmless; this test +// drives the tag-only update path (the shape the release cut uses) and asserts +// the mismatch surfaces loudly and that the existing target was actually +// resolved. The same-sha subcase proves a genuine convergence rerun (the tag +// already points where the cut targets) stays idempotently successful. +func TestManager_CreateGitTag_ExistingTagDifferentSHA(t *testing.T) { + tests := []struct { + name string + targetSHA string + existingSHA string + wantErr bool + }{ + {name: "different sha fails loudly", targetSHA: "newsha111", existingSHA: "148cf87stale", wantErr: true}, + {name: "same sha is idempotent success", targetSHA: "samesha222", existingSHA: "samesha222", wantErr: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var seen []string + server := existingTagServer(t, &seen, tt.existingSHA) + defer server.Close() + + manager := &Manager{ + client: server.Client(), + baseURL: server.URL + "/github", // host substring marks it as GitHub + token: "test-token", + repo: "owner/repo", + sleepFn: func(time.Duration) {}, + } + + _, err := manager.Manage(Options{ + Action: ActionUpdate, + Environment: "prerelease", + SHA: tt.targetSHA, + Tag: "v0.16.5-rc.1", + CreateTag: true, + TagOnly: true, + }) + + if tt.wantErr { + require.Error(t, err, "an existing tag at a different sha must fail, not silently succeed") + assert.Contains(t, err.Error(), tt.existingSHA, "error should name the stale target sha") + assert.Contains(t, err.Error(), tt.targetSHA, "error should name the intended target sha") + } else { + require.NoError(t, err) + } + + assert.True(t, containsPathSuffix(seen, http.MethodGet, "/git/refs/tags/v0.16.5-rc.1"), + "createGitTag must resolve the existing tag's target on a 422; saw %v", seen) + }) + } +} + func TestManager_Update_ExistingRelease(t *testing.T) { callCount := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {