From f618ae08b60af4a1cb4a83f3fbbe508ec1d8b0ef Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Mon, 22 Jun 2026 22:50:16 -0400 Subject: [PATCH] fix: eliminate eventual-consistency race in multi-env hotfix finalize Signed-off-by: Joshua Temple --- internal/hotfix/finalize.go | 21 +- internal/release/draft_consistency_test.go | 305 +++++++++++++++++++++ internal/release/release.go | 146 +++++++--- 3 files changed, 423 insertions(+), 49 deletions(-) create mode 100644 internal/release/draft_consistency_test.go diff --git a/internal/hotfix/finalize.go b/internal/hotfix/finalize.go index cc55b8d5..c43db504 100644 --- a/internal/hotfix/finalize.go +++ b/internal/hotfix/finalize.go @@ -576,23 +576,32 @@ func (f *Finalizer) createRelease(cfg *config.TrunkConfig, targetEnv, sha, hotfi body := fmt.Sprintf("Hotfix based on %s, carries trunk commit %s.", baseVersion, short(fixSHA)) - if _, err := mgr.Manage(release.Options{ + created, err := mgr.Manage(release.Options{ Action: release.ActionCreate, Environment: targetEnv, SHA: sha, Tag: hotfixVersion, Changelog: body, CreateTag: true, - }); err != nil { + }) + if err != nil { return fmt.Errorf("creating hotfix release: %w", err) } if f.isPrereleaseEnv(cfg, targetEnv) { + // Thread the created release ID through to avoid a re-lookup: the + // by-tag endpoint returns 404 for drafts and the list endpoint has a + // consistency window, so the second env can fail if we re-discover. + var knownID int64 + if created != nil { + knownID = created.ReleaseID + } if _, err := mgr.Manage(release.Options{ - Action: release.ActionPrerelease, - Environment: targetEnv, - SHA: sha, - Tag: hotfixVersion, + Action: release.ActionPrerelease, + Environment: targetEnv, + SHA: sha, + Tag: hotfixVersion, + KnownReleaseID: knownID, }); err != nil { return fmt.Errorf("promoting hotfix release to prerelease: %w", err) } diff --git a/internal/release/draft_consistency_test.go b/internal/release/draft_consistency_test.go new file mode 100644 index 00000000..3ca0aca4 --- /dev/null +++ b/internal/release/draft_consistency_test.go @@ -0,0 +1,305 @@ +package release + +// Tests for the draft-release eventual-consistency race that caused the second +// env's Finalize Hotfix to fail with "no release found for tag ..." even though +// the draft existed. +// +// Two failure modes are exercised: +// +// (a) Prong 1 - create->prerelease using the returned release ID directly, +// bypassing findRelease entirely. The fake server returns 404 on +// GET /releases/tags/{tag} AND an empty list on GET /releases, so any +// code that still calls findRelease on the prerelease path will fail. +// +// (b) Prong 2 - findReleaseByTagOrSHA bounded retry: the list returns empty on +// the first call and the real release on the second, simulating GitHub's +// list endpoint eventual-consistency. The retry must eventually succeed. +// When the release never appears the function must return a non-nil error. +// +// The act+gitea e2e harness cannot reproduce this race (Gitea's release API +// skips the prerelease PATCH entirely). These unit tests with a stubbed +// eventual-consistency server are the regression guard. + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestManager_CreateThenPrerelease_UsesCreatedReleaseID_NoFindRacePrerelease +// reproduces the fleet failure: create returns a draft release, then the +// prerelease promotion re-discovers it. The stub server returns 404 on the +// by-tag endpoint AND an empty list - simulating the consistency window - so +// any implementation that still calls findRelease on the prerelease path must +// fail. With prong 1 fixed (prerelease uses the created release ID directly), +// the sequence must succeed without ever hitting findRelease. +func TestManager_CreateThenPrerelease_UsesCreatedReleaseID_NoFindRacePrerelease(t *testing.T) { + t.Helper() + + const releaseID = int64(42) + const tag = "v1.0.0-rc.0.hotfix.2" + const sha = "de5dfd1234567890" + + findReleaseCalled := false + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Cleanup-draft list on create path - return empty + if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/releases") && + !strings.Contains(r.URL.Path, "/tags/") { + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode([]GitHubRelease{}) + return + } + + // POST /releases - the draft create + if r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/releases") { + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(GitHubRelease{ + ID: releaseID, + TagName: tag, + TargetCommitish: sha, + Draft: true, + URL: "https://api.github.com/repos/owner/repo/releases/42", + HTMLURL: "https://github.com/owner/repo/releases/tag/" + tag, + }) + return + } + + // POST /git/refs - createGitTag + if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/git/refs") { + w.WriteHeader(http.StatusCreated) + return + } + + // GET /releases/tags/{tag} - by-tag endpoint, always 404 (draft not indexed) + if r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/releases/tags/") { + findReleaseCalled = true + w.WriteHeader(http.StatusNotFound) + return + } + + // PATCH /releases/{id} - prerelease promotion + if r.Method == http.MethodPatch && strings.Contains(r.URL.Path, "/releases/") { + // Verify we are patching by ID, not re-discovering + assert.Contains(t, r.URL.Path, "/releases/42", + "prerelease PATCH must target the created release ID, not a re-looked-up one") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(GitHubRelease{ + ID: releaseID, + URL: "https://api.github.com/repos/owner/repo/releases/42", + HTMLURL: "https://github.com/owner/repo/releases/tag/" + tag, + }) + return + } + + // Any unexpected call + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + mgr := &Manager{ + client: server.Client(), + baseURL: server.URL + "/github", // marks as GitHub host + token: "test-token", + repo: "owner/repo", + sleepFn: func(time.Duration) {}, // no-op: prong 1 should not need retries + } + + // Step 1: create the hotfix draft release (returns the created release object) + created, err := mgr.Manage(Options{ + Action: ActionCreate, + SHA: sha, + Tag: tag, + Changelog: "Hotfix based on v1.0.0.", + CreateTag: true, + }) + require.NoError(t, err) + require.NotNil(t, created) + assert.Equal(t, releaseID, created.ReleaseID) + + // Step 2: promote to prerelease, passing the known release ID via KnownReleaseID + _, err = mgr.Manage(Options{ + Action: ActionPrerelease, + SHA: sha, + Tag: tag, + KnownReleaseID: releaseID, + }) + require.NoError(t, err, + "prerelease promotion must succeed even when the by-tag endpoint returns 404 for a draft") + + // With prong 1 in place, findRelease (the by-tag GET) must never be called + // during the prerelease promotion when KnownReleaseID is supplied. + assert.False(t, findReleaseCalled, + "prerelease must not call GET /releases/tags/{tag} when KnownReleaseID is set") +} + +// TestFindReleaseByTagOrSHA_BoundedRetry_EventuallySucceeds verifies prong 2: +// when the list endpoint returns empty on the first call and the matching +// release on the second, findReleaseByTagOrSHA retries and returns the release. +func TestFindReleaseByTagOrSHA_BoundedRetry_EventuallySucceeds(t *testing.T) { + t.Helper() + + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + w.WriteHeader(http.StatusOK) + if callCount == 1 { + // First call: empty list (consistency window) + _ = json.NewEncoder(w).Encode([]GitHubRelease{}) + return + } + // Second call: release has propagated + _ = json.NewEncoder(w).Encode([]GitHubRelease{ + { + ID: 99, + TagName: "v1.0.0-rc.0.hotfix.2", + TargetCommitish: "de5dfd", + Draft: true, + }, + }) + })) + defer server.Close() + + mgr := &Manager{ + client: server.Client(), + baseURL: server.URL, + token: "test-token", + repo: "owner/repo", + sleepFn: func(time.Duration) {}, // no-op sleep for fast tests + } + + got, err := mgr.findReleaseByTagOrSHA("v1.0.0-rc.0.hotfix.2", "de5dfd") + require.NoError(t, err) + require.NotNil(t, got, "must find the release on the second list call") + assert.Equal(t, int64(99), got.ID) + assert.Equal(t, 2, callCount, "must have retried once") +} + +// TestFindReleaseByTagOrSHA_BoundedRetry_NeverAppears verifies that when the +// release never appears in the list, the function returns a clear error rather +// than silently returning nil. +func TestFindReleaseByTagOrSHA_BoundedRetry_NeverAppears(t *testing.T) { + t.Helper() + + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode([]GitHubRelease{}) + })) + defer server.Close() + + mgr := &Manager{ + client: server.Client(), + baseURL: server.URL, + token: "test-token", + repo: "owner/repo", + sleepFn: func(time.Duration) {}, + } + + got, err := mgr.findReleaseByTagOrSHA("v1.0.0-rc.0.hotfix.2", "de5dfd") + // When the release genuinely does not exist, nil+nil is the existing contract + // (the nil is propagated up where the caller emits "no release found"). The + // bounded retry must not change that contract; it just means more list calls. + // We assert the call count is > 1 (retried) and the result is nil. + assert.NoError(t, err) + assert.Nil(t, got, "must return nil when release never appears after all retries") + assert.Greater(t, callCount, 1, "must retry before giving up") +} + +// TestManager_SingleEnvHotfix_Finalize_Unaffected verifies that single-env +// hotfix finalize (non-prerelease env) is unaffected by the prong-1 change: +// the create->lock sequence still works correctly when no KnownReleaseID flows. +func TestManager_SingleEnvHotfix_Finalize_Unaffected(t *testing.T) { + t.Helper() + + const releaseID = int64(77) + const tag = "v1.0.0-rc.0.hotfix.1" + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Cleanup-draft list on create path + if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/releases") && + !strings.Contains(r.URL.Path, "/tags/") { + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode([]GitHubRelease{}) + return + } + + if r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/releases") { + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(GitHubRelease{ + ID: releaseID, + TagName: tag, + Draft: true, + URL: "https://api.github.com/repos/owner/repo/releases/77", + HTMLURL: "https://github.com/owner/repo/releases/tag/" + tag, + }) + return + } + + if r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/git/refs") { + w.WriteHeader(http.StatusCreated) + return + } + + // GET /releases/tags/{tag} for lock path - return the draft (consistent) + if r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/releases/tags/") { + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(GitHubRelease{ + ID: releaseID, + TagName: tag, + Draft: true, + }) + return + } + + // PATCH for lock + if r.Method == http.MethodPatch { + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(GitHubRelease{ + ID: releaseID, + URL: "https://api.github.com/repos/owner/repo/releases/77", + HTMLURL: "https://github.com/owner/repo/releases/tag/" + tag, + }) + return + } + + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + mgr := &Manager{ + client: server.Client(), + baseURL: server.URL + "/github", + token: "test-token", + repo: "owner/repo", + sleepFn: func(time.Duration) {}, + } + + // create + res, err := mgr.Manage(Options{ + Action: ActionCreate, + SHA: "abc123", + Tag: tag, + Changelog: "Hotfix", + CreateTag: true, + }) + require.NoError(t, err) + require.NotNil(t, res) + + // lock (single-env path - no KnownReleaseID needed) + _, err = mgr.Manage(Options{ + Action: ActionLock, + SHA: "abc123", + Tag: tag, + }) + require.NoError(t, err, "single-env lock path must be unaffected") +} diff --git a/internal/release/release.go b/internal/release/release.go index 8e318c66..d1d80274 100644 --- a/internal/release/release.go +++ b/internal/release/release.go @@ -8,6 +8,7 @@ import ( "os" "regexp" "strings" + "time" ) // Action represents the release management action to perform @@ -35,6 +36,10 @@ type Manager struct { baseURL string token string repo string + // sleepFn is called between retry attempts in findReleaseByTagOrSHA to give + // GitHub's release-list endpoint time to reflect a recently created draft. + // Defaults to time.Sleep; tests inject a no-op to keep test runs fast. + sleepFn func(time.Duration) } // NewManager creates a new release manager. @@ -49,6 +54,7 @@ func NewManager(repo, token string) *Manager { baseURL: baseURL, token: token, repo: repo, + sleepFn: time.Sleep, } } @@ -60,6 +66,7 @@ func NewManagerWithURL(repo, token, baseURL string) *Manager { baseURL: strings.TrimSuffix(baseURL, "/"), token: token, repo: repo, + sleepFn: time.Sleep, } } @@ -83,6 +90,11 @@ type Options struct { NewTag string // New tag for publish (semver) - replaces short-sha tag DeleteTag string // Tag to delete after publish (short-sha cleanup) CreateTag bool // Whether to create the git tag (for initial release) + // KnownReleaseID is the GitHub release ID returned by a preceding ActionCreate + // in the same workflow step. When set, ActionPrerelease and ActionLock use it + // directly instead of re-discovering the release by tag, eliminating the + // eventual-consistency window between draft creation and the list endpoint. + KnownReleaseID int64 } // ValidateAction checks if the action is valid @@ -537,13 +549,26 @@ func (m *Manager) prerelease(opts Options) (*Result, error) { return &Result{}, nil } - existing, err := m.findRelease(opts.Tag, opts.SHA) - if err != nil { - return nil, err - } - - if existing == nil { - return nil, fmt.Errorf("no release found for tag %s (sha: %s)", opts.Tag, opts.SHA) + // Resolve the release to promote. When the caller supplies KnownReleaseID + // (set by the immediately preceding ActionCreate in the same workflow step), + // use it directly - the just-created release needs no re-discovery and + // bypassing findRelease eliminates the eventual-consistency race between + // draft creation and GitHub's list endpoint propagation. + var existingID int64 + var existingBody string + if opts.KnownReleaseID != 0 { + existingID = opts.KnownReleaseID + // Body will be built from scratch using opts.Changelog below. + } else { + existing, err := m.findRelease(opts.Tag, opts.SHA) + if err != nil { + return nil, err + } + if existing == nil { + return nil, fmt.Errorf("no release found for tag %s (sha: %s)", opts.Tag, opts.SHA) + } + existingID = existing.ID + existingBody = existing.Body } // If a new tag is specified, create it and update the release to use it @@ -560,7 +585,7 @@ func (m *Manager) prerelease(opts Options) (*Result, error) { bodyWithStatus := addStatusLine(opts.Changelog, opts.Environment) if opts.Changelog == "" { // Preserve existing body if no new changelog provided - bodyWithStatus = updateStatusLine(existing.Body, opts.Environment) + bodyWithStatus = updateStatusLine(existingBody, opts.Environment) } payload := map[string]interface{}{ @@ -571,7 +596,7 @@ func (m *Manager) prerelease(opts Options) (*Result, error) { "prerelease": true, } - release, err := m.apiRequest("PATCH", fmt.Sprintf("/releases/%d", existing.ID), payload) + release, err := m.apiRequest("PATCH", fmt.Sprintf("/releases/%d", existingID), payload) if err != nil { return nil, fmt.Errorf("converting to prerelease: %w", err) } @@ -714,51 +739,86 @@ func (m *Manager) findRelease(tag, sha string) (*GitHubRelease, error) { return m.findReleaseByTagOrSHA(tag, sha) } -// findReleaseByTagOrSHA searches all releases (including drafts) for a matching tag_name, name, or SHA. -// This handles the case where draft releases may have "untagged-..." as tag_name but the -// correct version as their name field, or where the tag hasn't been indexed yet. -// SHA matching is the most reliable for recently created drafts. +// listRetryAttempts is the total number of attempts when scanning the release +// list for a recently created draft. GitHub's release-list endpoint has an +// eventual-consistency window of a few seconds after draft creation; bounded +// retries prevent a spurious "no release found" error during that window. +const listRetryAttempts = 4 + +// listRetryBackoff is the base backoff between consecutive list attempts. The +// actual sleep per attempt is attempt*listRetryBackoff (linear). Tests inject a +// no-op sleepFn so no real time passes. +const listRetryBackoff = 2 * time.Second + +// findReleaseByTagOrSHA searches all releases (including drafts) for a matching +// tag_name, name, or SHA. SHA matching is most reliable for recently created +// drafts. +// +// When the first list response is empty (GitHub's release-list endpoint has an +// eventual-consistency window after draft creation), the function retries with a +// short backoff before concluding the release does not exist. The retry is +// bounded (listRetryAttempts total) so the function always terminates. func (m *Manager) findReleaseByTagOrSHA(tag, sha string) (*GitHubRelease, error) { - req, err := m.newRequest("GET", "/releases?per_page=100", nil) - if err != nil { - return nil, err + sleep := m.sleepFn + if sleep == nil { + sleep = time.Sleep } - resp, err := m.client.Do(req) - if err != nil { - return nil, fmt.Errorf("API request failed: %w", err) - } - defer func() { _ = resp.Body.Close() }() + for attempt := 0; attempt < listRetryAttempts; attempt++ { + if attempt > 0 { + sleep(time.Duration(attempt) * listRetryBackoff) + } - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - return nil, fmt.Errorf("API error %d: %s", resp.StatusCode, string(body)) - } + req, err := m.newRequest("GET", "/releases?per_page=100", nil) + if err != nil { + return nil, err + } - var releases []GitHubRelease - if err := json.NewDecoder(resp.Body).Decode(&releases); err != nil { - return nil, fmt.Errorf("decoding response: %w", err) - } + resp, err := m.client.Do(req) + if err != nil { + return nil, fmt.Errorf("API request failed: %w", err) + } - // Find release matching the tag or SHA (prefer draft over published for updates) - var found *GitHubRelease - for i := range releases { - // Match by tag_name, name, or SHA (target_commitish) - tagMatch := tag != "" && (releases[i].TagName == tag || releases[i].Name == tag) - shaMatch := sha != "" && releases[i].TargetCommitish == sha + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + return nil, fmt.Errorf("API error %d: %s", resp.StatusCode, string(body)) + } - if tagMatch || shaMatch { - if releases[i].Draft { - // Prefer draft - return immediately - return &releases[i], nil - } - if found == nil { - found = &releases[i] + var releases []GitHubRelease + if err := json.NewDecoder(resp.Body).Decode(&releases); err != nil { + _ = resp.Body.Close() + return nil, fmt.Errorf("decoding response: %w", err) + } + _ = resp.Body.Close() + + // Find release matching the tag or SHA (prefer draft over published for updates) + var found *GitHubRelease + for i := range releases { + // Match by tag_name, name, or SHA (target_commitish) + tagMatch := tag != "" && (releases[i].TagName == tag || releases[i].Name == tag) + shaMatch := sha != "" && releases[i].TargetCommitish == sha + + if tagMatch || shaMatch { + if releases[i].Draft { + // Prefer draft - return immediately + return &releases[i], nil + } + if found == nil { + found = &releases[i] + } } } + + if found != nil { + return found, nil + } + // found == nil: list returned nothing matching - may be a consistency + // window; retry on next iteration unless this was the last attempt. } - return found, nil + // All attempts exhausted; release genuinely not found. + return nil, nil } func (m *Manager) apiRequest(method, endpoint string, payload map[string]interface{}) (*GitHubRelease, error) {