From 85a53a321c693491c375bf119e36d2ca0fd9f10b Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Fri, 10 Jul 2026 15:45:14 -0400 Subject: [PATCH] fix(release): cut the git tag on update()'s existing-release branch update() only materialized the git tag on the create path. When a release object already existed (a draft matched by tag or target SHA) it PATCHed and returned without cutting the tag, so under a concurrent shared-path wave a pre-existing tagless draft advanced the state leaf while the git tag stayed absent. The state write is an unconditional CAS loop; tag creation was conditional and unretried, and that asymmetry left the tag permanently missing. Make the tag cut unconditional and idempotent on both update branches, mirroring create(). createGitTag already treats a 422 as success, so a convergence rerun is a no-op. Tag-only mode is unchanged. Adds TestManager_Update_CutsGitTag covering the existing-draft, no-release, and already-present-tag cases. Signed-off-by: Joshua Temple --- internal/release/release.go | 17 ++++++- internal/release/release_test.go | 85 ++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/internal/release/release.go b/internal/release/release.go index a45964f1..37a60dd4 100644 --- a/internal/release/release.go +++ b/internal/release/release.go @@ -726,10 +726,25 @@ func (m *Manager) update(opts Options) (*Result, error) { } if existing == nil { - // No existing release, create new one + // No existing release, create new one. create() cuts the git tag on this + // branch when CreateTag is set, so tag materialization is covered here too. return m.create(opts) } + // Cut the git tag unconditionally when requested, mirroring create(). A + // pre-existing release object (typically a draft) is PATCHed below, but the + // git tag must still be materialized on this branch: the orchestrate state + // write is an unconditional CAS loop, so leaving tag creation on the + // create-only path lets a pre-existing draft advance the state leaf while the + // git tag stays permanently absent. createGitTag is idempotent (a 422 "already + // exists" is treated as success), so re-cutting a tag that is already present + // is a harmless no-op on a convergence rerun. + if opts.CreateTag { + if err := m.createGitTag(opts.Tag, opts.SHA); err != nil { + return nil, fmt.Errorf("creating git tag: %w", err) + } + } + releaseName := generateReleaseName(opts.Environment, opts.Tag) bodyWithStatus := addStatusLine(opts.Changelog, opts.Environment) diff --git a/internal/release/release_test.go b/internal/release/release_test.go index 5c6bed30..5451af59 100644 --- a/internal/release/release_test.go +++ b/internal/release/release_test.go @@ -348,6 +348,91 @@ func TestManager_Update_TagOnly(t *testing.T) { } } +// updateTagRecordingServer answers an action=update flow against a GitHub host, +// recording every request. When existingDraft is true a matching draft release is +// returned by the tag-lookup GET so update() takes the PATCH branch; when false +// the tag lookup 404s and the release list is empty so update() falls through to +// create(). The POST /git/refs (tag-create) response status is gitRefStatus, +// letting a caller drive both the fresh-tag (201) and already-present (422) cases. +func updateTagRecordingServer(t *testing.T, seen *[]string, existingDraft bool, gitRefStatus int) *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(gitRefStatus) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/releases/tags/"): + if existingDraft { + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(GitHubRelease{ID: 456, TagName: "web-0.2.0-rc.0", Draft: true}) + return + } + w.WriteHeader(http.StatusNotFound) + case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/releases"): + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode([]GitHubRelease{}) + case r.Method == http.MethodPatch: + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(GitHubRelease{ID: 456}) + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/releases"): + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(GitHubRelease{ID: 456}) + default: + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(GitHubRelease{}) + } + })) +} + +// TestManager_Update_CutsGitTag is the regression guard for a release-path +// concurrency defect: action=update with CreateTag set must materialize the git +// tag on BOTH the pre-existing-release (PATCH) branch and the no-release (create) +// branch. The orchestrate state write is an unconditional CAS loop, so if tag +// creation stays on the create-only path a pre-existing draft (which the defect +// itself accumulates) advances the state leaf while the git tag is permanently +// absent. The idempotency subcase proves a convergence rerun over an +// already-present tag (422) does not error. +func TestManager_Update_CutsGitTag(t *testing.T) { + tests := []struct { + name string + existingDraft bool + gitRefStatus int + }{ + {name: "existing draft release still cuts the tag", existingDraft: true, gitRefStatus: http.StatusCreated}, + {name: "no existing release cuts the tag via create", existingDraft: false, gitRefStatus: http.StatusCreated}, + {name: "idempotent when the tag already exists", existingDraft: true, gitRefStatus: http.StatusUnprocessableEntity}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var seen []string + server := updateTagRecordingServer(t, &seen, tt.existingDraft, tt.gitRefStatus) + 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: "deadbeef", + Tag: "web-0.2.0-rc.0", + Changelog: "## Changes\n- Test", + CreateTag: true, + }) + require.NoError(t, err) + + assert.True(t, containsPathSuffix(seen, http.MethodPost, "/git/refs"), + "update with CreateTag must cut the git tag; saw %v", seen) + }) + } +} + func TestManager_Update_ExistingRelease(t *testing.T) { callCount := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {