From 90bb9fb2d2c0b4ce54d28ca612a394a432ec9539 Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Fri, 31 Jul 2026 19:29:22 +0530 Subject: [PATCH 1/2] Improve CMT provisioning resilience and mobile server version selection --- config/config-matterwick.default.json | 2 +- server/e2e_dryrun_test.go | 101 ++++++++- server/e2e_tests.go | 287 ++++++++++++++++++++------ server/e2e_tests_test.go | 248 ++++++++++++++++++++++ server/push_events.go | 19 +- server/workflow_run.go | 159 +++++++++++--- server/workflow_run_test.go | 8 +- 7 files changed, 713 insertions(+), 111 deletions(-) diff --git a/config/config-matterwick.default.json b/config/config-matterwick.default.json index 4f99e87..9b4086c 100644 --- a/config/config-matterwick.default.json +++ b/config/config-matterwick.default.json @@ -82,7 +82,7 @@ "E2EReleasePatternPrefix": "release-", "E2ETestWorkflowNames": ["Electron Playwright Tests", "E2E", "Compatibility Matrix Testing"], "E2EInstanceMaxAge": 6, - "E2EPRInstanceMaxAge": 24, + "E2EPRInstanceMaxAge": 8, "CMTTriggerWorkflowName": "CMT Provisioner", "CMTTestWorkflowName": "Compatibility Matrix Testing" } diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index 041b50d..26558bc 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -508,7 +508,10 @@ func TestDryRun_MobileCMT(t *testing.T) { assert.Equal(t, "https://v0-site3.example.com", s0["ios_site_1_url"]) assert.Equal(t, "https://v0-site4.example.com", s0["ios_site_2_url"]) assert.Equal(t, "https://v0-site5.example.com", s0["site_3_url"]) - assert.NotContains(t, s0, "url") + // `url` stays populated (site-1) for release branches cut before mobile's + // five-server CMT rewrite: those workflows read ${{ matrix.server.url }} and would + // otherwise test against an empty server URL for a whole release cycle. + assert.Equal(t, "https://v0-site3.example.com", s0["url"]) // Older version: `latest` is omitted entirely (cmtServer.Latest is false, omitempty). _, has0 := s0["latest"] assert.False(t, has0, "older mobile entries must not carry the `latest` field") @@ -1400,7 +1403,7 @@ func TestDryRun_ResolveCMTServerVersions(t *testing.T) { s.githubAPIBase = srv.URL + "/" s.Config.CMTServerVersions = nil - assert.Equal(t, []string{"10.11.19", "11.5.7", "11.6.4", "11.7.2", "11.8.0-rc3"}, s.cmtServerVersions()) + assert.Equal(t, []string{"10.11.19", "11.5.7", "11.6.4", "11.7.2", "11.8.0-rc3"}, s.cmtServerVersions("desktop")) }) t.Run("explicit CMTServerVersions override skips resolve", func(t *testing.T) { @@ -1414,7 +1417,8 @@ func TestDryRun_ResolveCMTServerVersions(t *testing.T) { s.githubAPIBase = srv.URL + "/" s.Config.CMTServerVersions = []string{"10.11.22", "11.10.0-rc1"} - assert.Equal(t, []string{"10.11.22", "11.10.0-rc1"}, s.cmtServerVersions()) + assert.Equal(t, []string{"10.11.22", "11.10.0-rc1"}, s.cmtServerVersions("desktop")) + assert.Equal(t, []string{"10.11.22", "11.10.0-rc1"}, s.cmtServerVersions("mobile"), "manual override applies to mobile too") assert.False(t, called, "manual override must not hit the GitHub API") }) @@ -1426,7 +1430,8 @@ func TestDryRun_ResolveCMTServerVersions(t *testing.T) { assert.Equal(t, defaultCMTServerVersions, s.resolveCMTServerVersions()) assert.Equal(t, []string{"10.11.22", "11.7.7"}, defaultCMTServerVersions) - assert.Equal(t, defaultCMTServerVersions, s.cmtServerVersions()) + assert.Equal(t, defaultCMTServerVersions, s.cmtServerVersions("desktop")) + assert.Equal(t, defaultCMTServerVersions, s.cmtServerVersions("mobile")) }) t.Run("cap prefers trailing ESR over oldest feature minor", func(t *testing.T) { @@ -1678,3 +1683,91 @@ func TestIsBuildReleaseBranch(t *testing.T) { assert.False(t, isBuildReleaseBranch(ref), "must not match: %q", ref) } } + +// TestDryRun_MobileCMTVersionSelection covers the mobile version set: newest ESR, latest +// production (newest non-ESR stable), and the current RC — one topology of five servers each. +func TestDryRun_MobileCMTVersionSelection(t *testing.T) { + releasesBody := `[ + {"tag_name":"v11.8.0-rc3","draft":false,"prerelease":true,"body":"rc"}, + {"tag_name":"v11.8.0-rc2","draft":false,"prerelease":true,"body":"rc"}, + {"tag_name":"v11.7.2","draft":false,"prerelease":false,"body":"Mattermost Platform Extended Support Release 11.7.2"}, + {"tag_name":"v11.7.1","draft":false,"prerelease":false,"body":"Mattermost Platform Extended Support Release 11.7.1"}, + {"tag_name":"v11.6.4","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.6.4"}, + {"tag_name":"v11.5.7","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.5.7"}, + {"tag_name":"v10.11.19","draft":false,"prerelease":false,"body":"Mattermost Platform Extended Support Release 10.11.19"} + ]` + + t.Run("picks newest ESR + latest production + current RC", func(t *testing.T) { + srv := mockReleasesServer(t, releasesBody, http.StatusOK) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + // 11.7.2 is the newest ESR line, 11.6.4 the newest non-ESR stable, 11.8.0-rc3 the RC. + // 10.11.19 (older ESR) and 11.5.7 (older stable) are left out. + assert.Equal(t, []string{"11.6.4", "11.7.2", "11.8.0-rc3"}, s.resolveMobileCMTServerVersions()) + }) + + t.Run("mobile selection is used for mobile and not for desktop", func(t *testing.T) { + srv := mockReleasesServer(t, releasesBody, http.StatusOK) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + s.Config.CMTServerVersions = nil + + mobile := s.cmtServerVersions("mobile") + desktop := s.cmtServerVersions("desktop") + assert.Len(t, mobile, maxMobileCMTServerVersions) + assert.Greater(t, len(desktop), len(mobile), "desktop keeps its wider set") + }) + + t.Run("no RC in flight backfills with the next stable line", func(t *testing.T) { + body := `[ + {"tag_name":"v11.7.2","draft":false,"prerelease":false,"body":"Mattermost Platform Extended Support Release 11.7.2"}, + {"tag_name":"v11.6.4","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.6.4"}, + {"tag_name":"v11.5.7","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.5.7"} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + // ESR 11.7.2 + latest production 11.6.4, then 11.5.7 backfills the empty RC slot. + assert.Equal(t, []string{"11.5.7", "11.6.4", "11.7.2"}, s.resolveMobileCMTServerVersions()) + }) + + t.Run("an RC older than the newest stable is not selected", func(t *testing.T) { + body := `[ + {"tag_name":"v11.6.0-rc1","draft":false,"prerelease":true,"body":"stale rc"}, + {"tag_name":"v11.7.2","draft":false,"prerelease":false,"body":"Mattermost Platform Extended Support Release 11.7.2"}, + {"tag_name":"v11.8.1","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.8.1"} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + got := s.resolveMobileCMTServerVersions() + assert.NotContains(t, got, "11.6.0-rc1", "a stale RC must not take the RC slot") + assert.Contains(t, got, "11.7.2") + assert.Contains(t, got, "11.8.1") + }) + + t.Run("no ESR flagged still fills the budget from stable lines", func(t *testing.T) { + body := `[ + {"tag_name":"v11.8.1","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.8.1"}, + {"tag_name":"v11.7.2","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.7.2"}, + {"tag_name":"v11.6.4","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.6.4"} + ]` + srv := mockReleasesServer(t, body, http.StatusOK) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + assert.Equal(t, []string{"11.6.4", "11.7.2", "11.8.1"}, s.resolveMobileCMTServerVersions()) + }) + + t.Run("API error falls back to the default set", func(t *testing.T) { + srv := mockReleasesServer(t, "boom", http.StatusInternalServerError) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + assert.Equal(t, defaultCMTServerVersions, s.resolveMobileCMTServerVersions()) + assert.LessOrEqual(t, len(defaultCMTServerVersions), maxMobileCMTServerVersions) + }) +} diff --git a/server/e2e_tests.go b/server/e2e_tests.go index d2e54d0..12032d0 100644 --- a/server/e2e_tests.go +++ b/server/e2e_tests.go @@ -264,7 +264,6 @@ func (s *Server) createMultipleE2EInstances(pr *model.PullRequest, instanceType username := s.Config.E2EUsername password := s.getE2EPassword(instanceType) // Name format: {type}-pr-{pr}-{platform}-{hex6} - uid := e2eUniqueSuffix() // Shared cancellable context: the first goroutine to fail cancels the rest so they // exit their polling loop within one sleep interval (30s) instead of waiting up to 30min. @@ -283,12 +282,15 @@ func (s *Server) createMultipleE2EInstances(pr *model.PullRequest, instanceType wg.Add(1) go func(idx int, platform string) { defer wg.Done() - instanceName := e2eInstanceName( - s.Config.DNSNameTestServer, - instanceType, fmt.Sprintf("pr-%d", pr.Number), platform, uid, - ) - logger.WithField("instance", instanceName).Info("Creating E2E instance") - inst, err := s.createCloudInstallation(ctx, instanceName, version, username, password, instanceType, logger) + // Fresh uid per attempt: a retry must not reuse the failed attempt's DNS name. + nameFn := func() string { + return e2eInstanceName( + s.Config.DNSNameTestServer, + instanceType, fmt.Sprintf("pr-%d", pr.Number), platform, e2eUniqueSuffix(), + ) + } + logger.WithField("platform", platform).Info("Creating E2E instance") + inst, err := s.createCloudInstallationWithRetry(ctx, nameFn, version, username, password, instanceType, logger) if err != nil { cancel() // signal sibling goroutines to stop waiting results[idx] = result{err: err} @@ -325,6 +327,35 @@ func (s *Server) createMultipleE2EInstances(pr *model.PullRequest, instanceType return instances, nil } +// createCloudInstallationAttempts is how many times a single instance is provisioned before +// giving up. A fresh name per attempt avoids colliding with the failed attempt's DNS record. +const createCloudInstallationAttempts = 2 + +// createCloudInstallationWithRetry provisions one instance, retrying transient provisioner +// failures. nameFn is called per attempt and must return a fresh unique name each time. +func (s *Server) createCloudInstallationWithRetry(ctx context.Context, nameFn func() string, version, username, password, instanceType string, logger logrus.FieldLogger) (*E2EInstance, error) { + var lastErr error + for attempt := 1; attempt <= createCloudInstallationAttempts; attempt++ { + if err := ctx.Err(); err != nil { + if lastErr != nil { + return nil, lastErr + } + return nil, fmt.Errorf("installation creation cancelled before attempt %d: %w", attempt, err) + } + instance, err := s.createCloudInstallation(ctx, nameFn(), version, username, password, instanceType, logger) + if err == nil { + return instance, nil + } + lastErr = err + logger.WithError(err).WithFields(logrus.Fields{ + "attempt": attempt, + "attempts": createCloudInstallationAttempts, + "version": version, + }).Warn("Installation attempt failed") + } + return nil, lastErr +} + // createCloudInstallation creates one installation and polls until stable. Cancelling ctx aborts the wait so parallel callers can fail fast. func (s *Server) createCloudInstallation(ctx context.Context, name, version, username, password, instanceType string, logger logrus.FieldLogger) (*E2EInstance, error) { if err := ctx.Err(); err != nil { @@ -968,23 +999,196 @@ func (a cmtVersion) less(b cmtVersion) bool { } const maxCMTServerVersions = 5 + +// maxMobileCMTServerVersions caps the mobile CMT version set at the three categories that +// matter: newest ESR, latest production, current RC (see resolveMobileCMTServerVersions). +// Mobile provisions one len(mobileE2EPlatforms)-server topology per version, so each slot +// costs five installations — 3 × 5 = 15 per run, against #92's 25. Raising this is +// expensive; prefer changing which categories are selected over adding slots. +const maxMobileCMTServerVersions = 3 + const maxCMTESRLines = 2 // current + trailing ESR; older body-flagged ESRs are treated as EOL // cmtServerVersions returns the version set CMT runs against. An explicit, non-empty // Config.CMTServerVersions is used verbatim (manual override / pin). Otherwise the set is // auto-derived from Mattermost GitHub releases. Shared by mobile and desktop CMT triggers. -func (s *Server) cmtServerVersions() []string { +func (s *Server) cmtServerVersions(instanceType string) []string { if len(s.Config.CMTServerVersions) > 0 { return s.Config.CMTServerVersions } + if instanceType == "mobile" { + return s.resolveMobileCMTServerVersions() + } return s.resolveCMTServerVersions() } +// cmtReleaseSet is the classified Mattermost release data both CMT selectors work from: +// newest patch per stable minor line, which of those lines are ESR, and the current RC. +type cmtReleaseSet struct { + latestStable map[cmtMinorKey]cmtVersion + esrMinors map[cmtMinorKey]bool + bestRC cmtVersion + haveRC bool +} + +// newestStableMinors returns every stable line's newest patch, newest line first. +func (rs cmtReleaseSet) newestStableMinors() []cmtVersion { + minors := make([]cmtVersion, 0, len(rs.latestStable)) + for _, v := range rs.latestStable { + minors = append(minors, v) + } + sort.Slice(minors, func(i, j int) bool { return minors[j].less(minors[i]) }) + return minors +} + +// isESRLine reports whether v's minor line is flagged as an extended support release. +func (rs cmtReleaseSet) isESRLine(v cmtVersion) bool { + return rs.esrMinors[cmtMinorKey{v.major, v.minor}] +} + +// resolveMobileCMTServerVersions picks the three versions mobile CMT runs against: the +// newest ESR, the newest non-ESR stable release, and the current RC. Mobile pays five +// servers per version, so the set is chosen by category rather than by "newest N" — the +// point is to span ESR → GA → upcoming, not to test three adjacent patches. Falls back to +// defaultCMTServerVersions when releases can't be fetched. +func (s *Server) resolveMobileCMTServerVersions() []string { + releaseSet, err := s.fetchCMTReleaseSet() + if err != nil { + s.Logger.WithError(err).Warn("[resolveMobileCMTServerVersions] Failed to classify releases; using default CMT versions") + return defaultCMTServerVersions + } + + minors := releaseSet.newestStableMinors() + if len(minors) == 0 { + s.Logger.Warn("[resolveMobileCMTServerVersions] No stable releases parsed; using default CMT versions") + return defaultCMTServerVersions + } + + seen := map[cmtMinorKey]bool{} + chosen := make([]cmtVersion, 0, maxMobileCMTServerVersions) + add := func(v cmtVersion) { + key := cmtMinorKey{v.major, v.minor} + if seen[key] { + return + } + seen[key] = true + chosen = append(chosen, v) + } + + // Newest ESR line. + for _, v := range minors { + if releaseSet.isESRLine(v) { + add(v) + break + } + } + // Newest stable line that isn't the ESR we just took — that's "latest production". + for _, v := range minors { + if !releaseSet.isESRLine(v) { + add(v) + break + } + } + // Current RC, only when it's ahead of the newest stable (i.e. an upcoming release). + if releaseSet.haveRC && minors[0].less(releaseSet.bestRC) { + add(releaseSet.bestRC) + } + + // If a category was missing (no ESR flagged, no RC in flight), backfill with the next + // newest stable lines so the matrix still uses its full budget. + for _, v := range minors { + if len(chosen) >= maxMobileCMTServerVersions { + break + } + add(v) + } + + sort.Slice(chosen, func(i, j int) bool { return chosen[i].less(chosen[j]) }) // ascending + if len(chosen) > maxMobileCMTServerVersions { + chosen = chosen[len(chosen)-maxMobileCMTServerVersions:] + } + + versions := make([]string, 0, len(chosen)) + for _, v := range chosen { + versions = append(versions, v.raw) + } + s.Logger.WithField("versions", versions).Info("[resolveMobileCMTServerVersions] Auto-derived mobile CMT server version set (ESR + latest stable + RC)") + return versions +} + // resolveCMTServerVersions fetches Mattermost releases and picks: the newest // maxCMTESRLines ESR lines (body-string "extended support release") + latest 3 stable // minors + current RC, one patch per line. Only used when Config.CMTServerVersions is // empty. Falls back to defaultCMTServerVersions on error. func (s *Server) resolveCMTServerVersions() []string { + releaseSet, err := s.fetchCMTReleaseSet() + if err != nil { + s.Logger.WithError(err).Warn("[resolveCMTServerVersions] Failed to classify releases; using default CMT versions") + return defaultCMTServerVersions + } + + latestStable := releaseSet.latestStable + esrMinors := releaseSet.esrMinors + bestRC := releaseSet.bestRC + haveRC := releaseSet.haveRC + + if len(latestStable) == 0 { + s.Logger.Warn("[resolveCMTServerVersions] No stable releases parsed; using default CMT versions") + return defaultCMTServerVersions + } + + // All stable minor lines, sorted descending (newest first). + minors := releaseSet.newestStableMinors() + + selected := map[cmtMinorKey]cmtVersion{} + for i := 0; i < len(minors) && i < 3; i++ { // latest 3 stable minor lines + selected[cmtMinorKey{minors[i].major, minors[i].minor}] = minors[i] + } + // Keep only the newest maxCMTESRLines ESR minors (current + trailing). Older lines + // still carry "extended support release" in historical GitHub release bodies and + // would otherwise flood the matrix (9.11 / 10.5 false positives after EOS). + esrChosen := make([]cmtVersion, 0, len(esrMinors)) + for k := range esrMinors { + if v, ok := latestStable[k]; ok { + esrChosen = append(esrChosen, v) + } + } + sort.Slice(esrChosen, func(i, j int) bool { return esrChosen[j].less(esrChosen[i]) }) // newest first + keptESR := map[cmtMinorKey]bool{} + for i := 0; i < len(esrChosen) && i < maxCMTESRLines; i++ { + k := cmtMinorKey{esrChosen[i].major, esrChosen[i].minor} + selected[k] = esrChosen[i] + keptESR[k] = true + } + + chosen := make([]cmtVersion, 0, len(selected)+1) + for _, v := range selected { + chosen = append(chosen, v) + } + // Include the current RC only when it's newer than the newest stable (an upcoming release). + if haveRC && minors[0].less(bestRC) { + chosen = append(chosen, bestRC) + } + sort.Slice(chosen, func(i, j int) bool { return chosen[i].less(chosen[j]) }) // ascending + + // Cap at maxCMTServerVersions. Prefer kept ESR lines over older feature minors so the + // trailing ESR is not dropped when latest-3 + 2 ESRs + RC exceeds the cap. + if len(chosen) > maxCMTServerVersions { + chosen = capCMTVersionsPreferringESR(chosen, keptESR, maxCMTServerVersions) + } + + versions := make([]string, 0, len(chosen)) + for _, v := range chosen { + versions = append(versions, v.raw) + } + s.Logger.WithField("versions", versions).Info("[resolveCMTServerVersions] Auto-derived CMT server version set") + return versions +} + +// fetchCMTReleaseSet fetches Mattermost releases and classifies them into the newest patch +// per stable minor line, which lines are ESR, and the current RC. Shared by the desktop and +// mobile selectors so both see the same view of releases. +func (s *Server) fetchCMTReleaseSet() (cmtReleaseSet, error) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -1006,8 +1210,7 @@ func (s *Server) resolveCMTServerVersions() []string { for page := 1; ; page++ { req, err := client.NewRequest("GET", fmt.Sprintf("/repos/mattermost/mattermost/releases?per_page=%d&page=%d", perPage, page), nil) if err != nil { - s.Logger.WithError(err).Warn("[resolveCMTServerVersions] Failed to build request; using default CMT versions") - return defaultCMTServerVersions + return cmtReleaseSet{}, fmt.Errorf("failed to build releases request: %w", err) } var pageReleases []struct { TagName string `json:"tag_name"` @@ -1016,8 +1219,7 @@ func (s *Server) resolveCMTServerVersions() []string { Body string `json:"body"` } if _, err = client.Do(ctx, req, &pageReleases); err != nil { - s.Logger.WithError(err).Warn("[resolveCMTServerVersions] Failed to fetch releases; using default CMT versions") - return defaultCMTServerVersions + return cmtReleaseSet{}, fmt.Errorf("failed to fetch releases: %w", err) } releases = append(releases, pageReleases...) if len(pageReleases) < perPage { @@ -1054,61 +1256,12 @@ func (s *Server) resolveCMTServerVersions() []string { } } - if len(latestStable) == 0 { - s.Logger.Warn("[resolveCMTServerVersions] No stable releases parsed; using default CMT versions") - return defaultCMTServerVersions - } - - // All stable minor lines, sorted descending (newest first). - minors := make([]cmtVersion, 0, len(latestStable)) - for _, v := range latestStable { - minors = append(minors, v) - } - sort.Slice(minors, func(i, j int) bool { return minors[j].less(minors[i]) }) - - selected := map[cmtMinorKey]cmtVersion{} - for i := 0; i < len(minors) && i < 3; i++ { // latest 3 stable minor lines - selected[cmtMinorKey{minors[i].major, minors[i].minor}] = minors[i] - } - // Keep only the newest maxCMTESRLines ESR minors (current + trailing). Older lines - // still carry "extended support release" in historical GitHub release bodies and - // would otherwise flood the matrix (9.11 / 10.5 false positives after EOS). - esrChosen := make([]cmtVersion, 0, len(esrMinors)) - for k := range esrMinors { - if v, ok := latestStable[k]; ok { - esrChosen = append(esrChosen, v) - } - } - sort.Slice(esrChosen, func(i, j int) bool { return esrChosen[j].less(esrChosen[i]) }) // newest first - keptESR := map[cmtMinorKey]bool{} - for i := 0; i < len(esrChosen) && i < maxCMTESRLines; i++ { - k := cmtMinorKey{esrChosen[i].major, esrChosen[i].minor} - selected[k] = esrChosen[i] - keptESR[k] = true - } - - chosen := make([]cmtVersion, 0, len(selected)+1) - for _, v := range selected { - chosen = append(chosen, v) - } - // Include the current RC only when it's newer than the newest stable (an upcoming release). - if haveRC && minors[0].less(bestRC) { - chosen = append(chosen, bestRC) - } - sort.Slice(chosen, func(i, j int) bool { return chosen[i].less(chosen[j]) }) // ascending - - // Cap at maxCMTServerVersions. Prefer kept ESR lines over older feature minors so the - // trailing ESR is not dropped when latest-3 + 2 ESRs + RC exceeds the cap. - if len(chosen) > maxCMTServerVersions { - chosen = capCMTVersionsPreferringESR(chosen, keptESR, maxCMTServerVersions) - } - - versions := make([]string, 0, len(chosen)) - for _, v := range chosen { - versions = append(versions, v.raw) - } - s.Logger.WithField("versions", versions).Info("[resolveCMTServerVersions] Auto-derived CMT server version set") - return versions + return cmtReleaseSet{ + latestStable: latestStable, + esrMinors: esrMinors, + bestRC: bestRC, + haveRC: haveRC, + }, nil } // capCMTVersionsPreferringESR keeps at most maxN versions from an ascending list, dropping diff --git a/server/e2e_tests_test.go b/server/e2e_tests_test.go index 20fe660..32dd43f 100644 --- a/server/e2e_tests_test.go +++ b/server/e2e_tests_test.go @@ -4,10 +4,13 @@ package server import ( + "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "slices" + "strings" "testing" mattermostModel "github.com/mattermost/mattermost-server/v6/model" @@ -850,3 +853,248 @@ func TestExtractPlatformFromLabel(t *testing.T) { }) } } + +// TestCreateCloudInstallationWithRetry covers the retry added so a single transient +// provisioner failure no longer sinks a whole 5-instance mobile run (or a CMT matrix). +func TestCreateCloudInstallationWithRetry(t *testing.T) { + newServerWithProvisioner := func(handler http.HandlerFunc) *Server { + ts := httptest.NewServer(handler) + t.Cleanup(ts.Close) + return &Server{ + Config: &MatterwickConfig{ + DNSNameTestServer: "test.example.com", + E2EUsername: "admin", + }, + CloudClient: model.NewCloudClient(ts.URL, "", "", "", ""), + Logger: logrus.New(), + } + } + + t.Run("retries and uses a fresh name per attempt", func(t *testing.T) { + var requested []string + server := newServerWithProvisioner(func(w http.ResponseWriter, r *http.Request) { + var body struct { + DNS string `json:"dns"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + requested = append(requested, body.DNS) + w.WriteHeader(http.StatusInternalServerError) + }) + + i := 0 + nameFn := func() string { + i++ + return fmt.Sprintf("mobile-11-7-7-site-3-uid%d", i) + } + + instance, err := server.createCloudInstallationWithRetry( + context.Background(), nameFn, "11.7.7", "admin", "pw", "mobile", server.Logger) + + require.Error(t, err) + assert.Nil(t, instance) + require.Len(t, requested, createCloudInstallationAttempts, "every attempt must reach the provisioner") + assert.NotEqual(t, requested[0], requested[1], "a retry must not reuse the failed attempt's DNS name") + }) + + t.Run("does not retry once the shared context is cancelled", func(t *testing.T) { + attempts := 0 + server := newServerWithProvisioner(func(w http.ResponseWriter, r *http.Request) { + attempts++ + w.WriteHeader(http.StatusInternalServerError) + }) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := server.createCloudInstallationWithRetry( + ctx, func() string { return "mobile-11-7-7-site-3-uid" }, "11.7.7", "admin", "pw", "mobile", server.Logger) + + require.Error(t, err) + assert.Zero(t, attempts, "a cancelled sibling must stop further attempts immediately") + }) +} + +// TestCMTVersionCapFor pins the mobile-vs-desktop CMT version caps. Mobile provisions a +// full topology per version, so its cap governs whether CMT can provision at all. +func TestCMTVersionCapFor(t *testing.T) { + assert.Equal(t, maxMobileCMTServerVersions, cmtVersionCapFor("mobile")) + assert.Equal(t, maxCMTServerVersions, cmtVersionCapFor("desktop")) + assert.Equal(t, maxCMTServerVersions, cmtVersionCapFor("")) + + // The mobile cap exists to bound instance count: versions × platforms. Fifteen is the + // ceiling a mobile CMT run may ask the provisioner for; #92 asked for 25. + assert.LessOrEqual(t, maxMobileCMTServerVersions*len(mobileE2EPlatforms), 15, + "a mobile CMT run must not ask the provisioner for more than 15 installations") +} + +// TestCapCMTServerVersionsRespectsMax covers the per-instance-type cap argument. +func TestCapCMTServerVersionsRespectsMax(t *testing.T) { + in := []string{"10.11.22", "11.7.7", "11.9.0", "11.10.0", "11.11.0-rc1"} + + assert.Equal(t, []string{"11.10.0", "11.11.0-rc1"}, capCMTServerVersions(in, 2), "keeps the newest two") + assert.Equal(t, []string{"11.9.0", "11.10.0", "11.11.0-rc1"}, capCMTServerVersions(in, maxMobileCMTServerVersions), + "keeps the newest three") + + assert.Equal(t, in, capCMTServerVersions(in, maxCMTServerVersions), "desktop cap leaves the set alone") + assert.Len(t, capCMTServerVersions(in, 0), 1, "a non-positive cap is clamped to 1, not to zero coverage") +} + +// TestSpanCMTServerVersions covers the mobile cap: it must keep the ends of the version +// range (oldest ESR + newest RC), because a newest-N cap tests no compatibility distance. +func TestSpanCMTServerVersions(t *testing.T) { + resolved := []string{"10.11.22", "11.5.7", "11.6.4", "11.7.2", "11.8.0-rc3"} + + t.Run("keeps oldest and newest at a two-slot cap", func(t *testing.T) { + got := spanCMTServerVersions(resolved, 2) + assert.Equal(t, []string{"10.11.22", "11.8.0-rc3"}, got) + }) + + t.Run("orders by semver, not input order", func(t *testing.T) { + shuffled := []string{"11.7.2", "11.8.0-rc3", "10.11.22", "11.5.7", "11.6.4"} + assert.Equal(t, []string{"10.11.22", "11.8.0-rc3"}, spanCMTServerVersions(shuffled, 2)) + }) + + t.Run("spreads intermediate picks across the range", func(t *testing.T) { + got := spanCMTServerVersions(resolved, 3) + assert.Equal(t, []string{"10.11.22", "11.6.4", "11.8.0-rc3"}, got) + }) + + t.Run("returns input unchanged when at or under the cap", func(t *testing.T) { + in := []string{"10.11.22", "11.8.0"} + assert.Equal(t, in, spanCMTServerVersions(in, maxMobileCMTServerVersions)) + assert.Equal(t, in, spanCMTServerVersions(in, 2)) + }) + + t.Run("drops unparseable entries rather than spending a slot on them", func(t *testing.T) { + in := []string{"not-a-version", "10.11.22", "11.6.4", "11.8.0-rc3"} + assert.Equal(t, []string{"10.11.22", "11.8.0-rc3"}, spanCMTServerVersions(in, 2)) + }) + + t.Run("falls back to the newest-N cap when nothing parses", func(t *testing.T) { + in := []string{"garbage-a", "garbage-b", "garbage-c"} + assert.Len(t, spanCMTServerVersions(in, 2), 2) + }) + + t.Run("never returns an empty set", func(t *testing.T) { + assert.Len(t, spanCMTServerVersions(resolved, 0), 1) + }) + + t.Run("does not mutate the input slice", func(t *testing.T) { + in := []string{"11.7.2", "11.8.0-rc3", "10.11.22", "11.5.7", "11.6.4"} + orig := append([]string(nil), in...) + _ = spanCMTServerVersions(in, 2) + assert.Equal(t, orig, in) + }) +} + +// TestDesktopPathsUnaffected locks the desktop guarantees against the mobile-driven changes +// (mobile CMT back-compat url, mobile-only version cap, per-version drop tolerance). +func TestDesktopPathsUnaffected(t *testing.T) { + t.Run("desktop keeps the 5-version cap and newest-N selection", func(t *testing.T) { + assert.Equal(t, maxCMTServerVersions, cmtVersionCapFor("desktop")) + + resolved := []string{"10.11.22", "11.7.8", "11.8.4", "11.9.0", "11.10.0-rc2"} + assert.Equal(t, resolved, capCMTServerVersions(resolved, cmtVersionCapFor("desktop")), + "a 5-version desktop set must pass through untouched") + }) + + t.Run("desktop matrix carries no mobile keys", func(t *testing.T) { + versions := []string{"11.9.0", "11.10.0-rc2"} + instances := []*E2EInstance{ + {URL: "https://a.example.com", ServerVersion: "11.9.0"}, + {URL: "https://b.example.com", ServerVersion: "11.10.0-rc2"}, + } + jsonStr, err := buildDesktopCMTMatrixJSON(versions, instances) + require.NoError(t, err) + + var matrix struct { + Server []map[string]any `json:"server"` + } + require.NoError(t, json.Unmarshal([]byte(jsonStr), &matrix)) + require.Len(t, matrix.Server, 2) + + // Assert on parsed keys, not the raw string: "latest" is a substring of the + // ubuntu-latest runner name. + for _, entry := range matrix.Server { + for _, mobileKey := range []string{"android_site_1_url", "android_site_2_url", "ios_site_1_url", "ios_site_2_url", "site_3_url", "latest"} { + assert.NotContains(t, entry, mobileKey, "mobile CMT fields must not leak into the desktop matrix") + } + assert.Equal(t, []string{"url", "version"}, sortedKeys(entry), "desktop entries carry version+url only") + } + assert.Equal(t, "https://a.example.com", matrix.Server[0]["url"]) + }) + + t.Run("a dropped version leaves the desktop matrix aligned", func(t *testing.T) { + // What handleCMTWithServerVersions hands over after dropping a version that failed + // to provision: validVersions and allInstances shrink together. + keptVersions := []string{"10.11.22", "11.10.0-rc2"} + keptInstances := []*E2EInstance{ + {URL: "https://esr.example.com", ServerVersion: "10.11.22"}, + {URL: "https://rc.example.com", ServerVersion: "11.10.0-rc2"}, + } + jsonStr, err := buildDesktopCMTMatrixJSON(keptVersions, keptInstances) + require.NoError(t, err) + + var matrix struct { + Environment []map[string]string `json:"environment"` + Server []map[string]any `json:"server"` + } + require.NoError(t, json.Unmarshal([]byte(jsonStr), &matrix)) + + require.Len(t, matrix.Server, len(keptVersions), "no silent truncation or padding") + for i, v := range keptVersions { + assert.Equal(t, v, matrix.Server[i]["version"]) + assert.Equal(t, keptInstances[i].URL, matrix.Server[i]["url"], "version and URL must stay paired") + } + // The desktop workflow derives total_reports_expected as + // (.environment|length) * (.server|length), so a reduced matrix stays self-consistent. + assert.Equal(t, 3*len(keptVersions), len(matrix.Environment)*len(matrix.Server)) + }) + + t.Run("desktop runner mapping unchanged", func(t *testing.T) { + assert.Equal(t, "ubuntu-latest", getRunnerForPlatform("linux")) + assert.Equal(t, "macos-latest", getRunnerForPlatform("macos")) + assert.Equal(t, "windows-2022", getRunnerForPlatform("windows")) + }) +} + +// TestInstanceNamePlatformRemainsParseable guards the per-attempt uid change: cloud reuse +// recovers the platform by stripping the trailing "-{8 hex}" and matching the platform +// suffix, so a per-instance uid must not disturb that shape. +func TestInstanceNamePlatformRemainsParseable(t *testing.T) { + const dnsSuffix = "test.mattermost.cloud" + + cases := []struct { + instanceType string + prefix string + platforms []string + }{ + {"desktop", "pr-123", []string{"linux", "macos", "windows"}}, + {"mobile", "pr-99999", mobileE2EPlatforms}, + } + + for _, c := range cases { + seen := map[string]bool{} + for _, platform := range c.platforms { + name := e2eInstanceName(dnsSuffix, c.instanceType, c.prefix, platform, e2eUniqueSuffix()) + assert.False(t, seen[name], "instance names must be unique across platforms") + seen[name] = true + + require.Greater(t, len(name), 9, "name must be longer than the uid it carries") + withoutUID := name[:len(name)-9] + assert.True(t, strings.HasSuffix(withoutUID, "-"+platform), + "reuse lookup must still recover platform %q from %q", platform, name) + assert.LessOrEqual(t, len(name), 62-len(dnsSuffix), "name must fit the DNS budget without truncation") + } + } +} + +// sortedKeys returns m's keys in sorted order, for stable assertions on JSON shape. +func sortedKeys(m map[string]any) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + slices.Sort(keys) + return keys +} diff --git a/server/push_events.go b/server/push_events.go index f4b7ac7..2ede44e 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -111,12 +111,16 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { instances, err := s.createMultipleE2EInstancesForPushEvent(repoName, instanceType, branch) if err != nil { + // Push E2E has no PR comment to fall back on, so a provisioning failure is invisible + // unless it is reported. Silent misses on main are how this regressed unnoticed. logger.WithError(err).Error("Failed to create E2E instances") + s.logErrorToMattermost("E2E on %s %s (%s) did not run: failed to provision test servers (%v)", repoName, branch, sha, err) return } if len(instances) == 0 { logger.Error("No instances created for E2E testing") + s.logErrorToMattermost("E2E on %s %s (%s) did not run: no test servers were created", repoName, branch, sha) return } @@ -144,6 +148,7 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { err = s.triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, sha, instances) if err != nil { logger.WithError(err).Error("Failed to trigger E2E workflow") + s.logErrorToMattermost("E2E on %s %s (%s) did not run: workflow dispatch failed (%v)", repoName, branch, sha, err) s.e2eInstancesLock.Lock() delete(s.e2eInstances, key) s.e2eInstancesLock.Unlock() @@ -172,7 +177,6 @@ func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, serverVersion := s.serverVersionForPushEvent() sanitizedVersion := sanitizeForDNS(serverVersion) - uid := e2eUniqueSuffix() username := s.Config.E2EUsername password := s.getE2EPassword(instanceType) @@ -191,11 +195,14 @@ func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, wg.Add(1) go func(idx int, platform string) { defer wg.Done() - name := e2eInstanceName( - s.Config.DNSNameTestServer, - instanceType, sanitizedVersion, platform, uid, - ) - inst, err := s.createCloudInstallation(ctx, name, serverVersion, username, password, instanceType, logger) + // Fresh uid per attempt: a retry must not reuse the failed attempt's DNS name. + nameFn := func() string { + return e2eInstanceName( + s.Config.DNSNameTestServer, + instanceType, sanitizedVersion, platform, e2eUniqueSuffix(), + ) + } + inst, err := s.createCloudInstallationWithRetry(ctx, nameFn, serverVersion, username, password, instanceType, logger) if err != nil { cancel() results[idx] = result{err: err} diff --git a/server/workflow_run.go b/server/workflow_run.go index 1dd813e..ebcc3e1 100644 --- a/server/workflow_run.go +++ b/server/workflow_run.go @@ -292,7 +292,7 @@ func (s *Server) handleCMTTrigger(owner, repoName, branch, sha string, runID int return } - versions := s.cmtServerVersions() + versions := s.cmtServerVersions(instanceType) logger.WithFields(logrus.Fields{ "instanceType": instanceType, "versions": versions, @@ -301,10 +301,24 @@ func (s *Server) handleCMTTrigger(owner, repoName, branch, sha string, runID int s.handleCMTWithServerVersions(owner, repoName, instanceType, branch, sha, versions, runID, logger) } -// capCMTServerVersions keeps at most maxCMTServerVersions entries, preferring the -// newest parseable semvers. Copies the input so Config.CMTServerVersions is not mutated. -func capCMTServerVersions(serverVersions []string) []string { - if len(serverVersions) <= maxCMTServerVersions { +// cmtVersionCapFor returns the version cap for instanceType. Mobile provisions a full +// len(mobileE2EPlatforms)-server topology per version, so its cap must be tighter than +// desktop's one-server-per-version: 5 versions × 5 servers is 25 installations, which in +// practice never all reach stable inside one CMT window. +func cmtVersionCapFor(instanceType string) int { + if instanceType == "mobile" { + return maxMobileCMTServerVersions + } + return maxCMTServerVersions +} + +// capCMTServerVersions keeps at most limit entries, preferring the newest parseable +// semvers. Copies the input so Config.CMTServerVersions is not mutated. +func capCMTServerVersions(serverVersions []string, limit int) []string { + if limit < 1 { + limit = 1 + } + if len(serverVersions) <= limit { return serverVersions } sorted := append([]string(nil), serverVersions...) @@ -319,16 +333,79 @@ func capCMTServerVersions(serverVersions []string) []string { } return vi.less(vj) }) - return sorted[len(sorted)-maxCMTServerVersions:] + return sorted[len(sorted)-limit:] +} + +// spanCMTServerVersions reduces serverVersions to at most limit entries while keeping both +// ends of the range. CMT's whole value is the spread between the oldest supported server +// (ESR) and the newest (RC) — a plain "keep the newest N" cap would collapse a mobile +// matrix to two adjacent releases, which tests nothing about compatibility. Unparseable +// entries are dropped rather than allowed to consume a slot. +func spanCMTServerVersions(serverVersions []string, limit int) []string { + if limit < 1 { + limit = 1 + } + if len(serverVersions) <= limit { + return serverVersions + } + + parseable := make([]string, 0, len(serverVersions)) + for _, v := range serverVersions { + if _, ok := parseCMTVersion(v); ok { + parseable = append(parseable, v) + } + } + if len(parseable) == 0 { + // Nothing to order by — fall back to the newest-N cap over the raw input. + return capCMTServerVersions(serverVersions, limit) + } + if len(parseable) <= limit { + return parseable + } + + sort.Slice(parseable, func(i, j int) bool { + vi, _ := parseCMTVersion(parseable[i]) + vj, _ := parseCMTVersion(parseable[j]) + return vi.less(vj) + }) + + if limit == 1 { + return []string{parseable[len(parseable)-1]} + } + + // Evenly spaced picks that always include the oldest and newest entry. + last := len(parseable) - 1 + selected := make([]string, 0, limit) + seen := make(map[int]bool, limit) + for i := 0; i < limit; i++ { + idx := (i*last + (limit-1)/2) / (limit - 1) + if seen[idx] { + continue + } + seen[idx] = true + selected = append(selected, parseable[idx]) + } + return selected } // handleCMTWithServerVersions orchestrates CMT testing and dispatches compatibility-matrix-testing.yml once. func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, branch, sha string, serverVersions []string, runID int64, logger logrus.FieldLogger) { - // Cap at maxCMTServerVersions. Auto-resolve already enforces this with ESR - // preference; this is a backstop for a mis-set Config.CMTServerVersions override. - if len(serverVersions) > maxCMTServerVersions { - logger.Warnf("Capping server versions from %d to %d (keeping newest)", len(serverVersions), maxCMTServerVersions) - serverVersions = capCMTServerVersions(serverVersions) + // Cap per instance type. Auto-resolve enforces the desktop cap with ESR preference, + // but mobile needs a tighter one (5 servers per version) and Config.CMTServerVersions + // can bypass auto-resolve entirely. + versionCap := cmtVersionCapFor(instanceType) + if len(serverVersions) > versionCap { + if instanceType == "mobile" { + // Keep the ends of the range (oldest ESR + newest RC) so a 2-version mobile + // matrix still spans real compatibility distance. + originalCount := len(serverVersions) + serverVersions = spanCMTServerVersions(serverVersions, versionCap) + logger.Warnf("Capping mobile server versions from %d to %d (keeping range ends): %s", + originalCount, len(serverVersions), strings.Join(serverVersions, ", ")) + } else { + logger.Warnf("Capping server versions from %d to %d (keeping newest)", len(serverVersions), versionCap) + serverVersions = capCMTServerVersions(serverVersions, versionCap) + } } logger = logger.WithFields(logrus.Fields{ @@ -339,9 +416,13 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, }) logger.Info("Starting CMT with server versions") - // All-or-nothing: a partial matrix silently drops coverage, so roll back on any failure. + // Best-effort per version: a version whose topology fails to provision is dropped + // (loudly — log + Mattermost alert) and CMT still runs for the versions that came up. + // Aborting the whole matrix on one failed installation means zero coverage, which is + // strictly worse than reduced coverage, and it is why CMT stopped dispatching at all. var allInstances []*E2EInstance var validVersions []string + var droppedVersions []string for _, version := range serverVersions { version = strings.TrimSpace(version) @@ -358,16 +439,17 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, var err error versionInstances, err = s.createMobileCMTInstances(context.Background(), repoName, version, logger) if err != nil { - logger.WithError(err).Errorf("Failed to create topology for version %s; rolling back partial CMT matrix", version) - s.destroyE2EInstances(allInstances, logger) - return + // createMobileCMTInstances already destroyed its own partial topology. + logger.WithError(err).Errorf("Failed to create topology for version %s; dropping this version from the CMT matrix", version) + droppedVersions = append(droppedVersions, version) + continue } } else { instance, err := s.createSingleCMTInstance(context.Background(), repoName, instanceType, version, "", logger) if err != nil { - logger.WithError(err).Errorf("Failed to create instance for version %s; rolling back partial CMT matrix", version) - s.destroyE2EInstances(allInstances, logger) - return + logger.WithError(err).Errorf("Failed to create instance for version %s; dropping this version from the CMT matrix", version) + droppedVersions = append(droppedVersions, version) + continue } versionInstances = []*E2EInstance{instance} } @@ -376,17 +458,30 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, expectedInstances = len(mobileE2EPlatforms) } if len(versionInstances) != expectedInstances { - logger.Errorf("Failed to create complete CMT topology for version %s; rolling back partial CMT matrix", version) - s.destroyE2EInstances(append(allInstances, versionInstances...), logger) - return + logger.Errorf("Incomplete CMT topology for version %s (got %d, want %d); dropping this version", version, len(versionInstances), expectedInstances) + s.destroyE2EInstances(versionInstances, logger) + droppedVersions = append(droppedVersions, version) + continue } allInstances = append(allInstances, versionInstances...) validVersions = append(validVersions, version) } + if len(droppedVersions) > 0 { + logger.WithFields(logrus.Fields{ + "dropped_versions": droppedVersions, + "kept_versions": validVersions, + }).Error("CMT matrix is running with reduced server-version coverage") + s.logErrorToMattermost("CMT on %s/%s (%s): dropped server version(s) %s — provisioning failed. Running with %s.", + repoOwner, repoName, branch, strings.Join(droppedVersions, ", "), strings.Join(validVersions, ", ")) + } + if len(allInstances) == 0 { - logger.Warn("No CMT instances created (empty version set)") + logger.Error("No CMT instances created; nothing to dispatch") + if len(droppedVersions) > 0 { + s.logErrorToMattermost("CMT on %s/%s (%s) did not run: no server version could be provisioned.", repoOwner, repoName, branch) + } return } @@ -439,18 +534,20 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, // createSingleCMTInstance creates one Mattermost cloud instance for a CMT server version. func (s *Server) createSingleCMTInstance(ctx context.Context, repoName, instanceType, version, platform string, logger logrus.FieldLogger) (*E2EInstance, error) { sanitizedVersion := sanitizeForDNS(version) - uid := e2eUniqueSuffix() - nameParts := []string{instanceType, sanitizedVersion} - if platform != "" { - nameParts = append(nameParts, platform) + // Fresh uid per attempt: a retry must not reuse the failed attempt's DNS name. + nameFn := func() string { + nameParts := []string{instanceType, sanitizedVersion} + if platform != "" { + nameParts = append(nameParts, platform) + } + nameParts = append(nameParts, e2eUniqueSuffix()) + return e2eInstanceName(s.Config.DNSNameTestServer, nameParts...) } - nameParts = append(nameParts, uid) - name := e2eInstanceName(s.Config.DNSNameTestServer, nameParts...) username := s.Config.E2EUsername password := s.getE2EPassword(instanceType) - instance, err := s.createCloudInstallation(ctx, name, version, username, password, instanceType, logger) + instance, err := s.createCloudInstallationWithRetry(ctx, nameFn, version, username, password, instanceType, logger) if instance != nil { instance.Platform = platform } @@ -602,6 +699,10 @@ func buildMobileCMTMatrixJSON(versions []string, instances []*E2EInstance) (stri entry.Site3URL = url } } + // Back-compat: release branches cut before mobile's five-server CMT rewrite read + // ${{ matrix.server.url }}. Those branches are live for a whole release cycle, so + // keep url populated (site-1) or CMT on them runs against an empty server URL. + entry.URL = entry.IOSSite1URL if i == latestIdx { entry.Latest = true } diff --git a/server/workflow_run_test.go b/server/workflow_run_test.go index f9b4730..fbaeb43 100644 --- a/server/workflow_run_test.go +++ b/server/workflow_run_test.go @@ -587,7 +587,7 @@ func TestVersionParsingWithVariations(t *testing.T) { func TestCapCMTServerVersions(t *testing.T) { t.Run("returns input unchanged when at or under cap", func(t *testing.T) { in := []string{"10.11.22", "11.7.7", "11.9.0"} - got := capCMTServerVersions(in) + got := capCMTServerVersions(in, maxCMTServerVersions) if len(got) != len(in) { t.Fatalf("expected %d, got %d", len(in), len(got)) } @@ -600,7 +600,7 @@ func TestCapCMTServerVersions(t *testing.T) { t.Run("keeps newest semvers regardless of input order", func(t *testing.T) { in := []string{"11.8.0", "10.11.22", "11.10.0", "11.7.7", "11.9.0", "11.11.0-rc1"} - got := capCMTServerVersions(in) + got := capCMTServerVersions(in, maxCMTServerVersions) want := []string{"11.7.7", "11.8.0", "11.9.0", "11.10.0", "11.11.0-rc1"} if len(got) != len(want) { t.Fatalf("expected %v, got %v", want, got) @@ -615,7 +615,7 @@ func TestCapCMTServerVersions(t *testing.T) { t.Run("does not mutate input slice", func(t *testing.T) { in := []string{"11.8.0", "10.11.22", "11.10.0", "11.7.7", "11.9.0", "11.11.0-rc1"} orig := append([]string(nil), in...) - _ = capCMTServerVersions(in) + _ = capCMTServerVersions(in, maxCMTServerVersions) for i := range orig { if in[i] != orig[i] { t.Fatalf("input mutated at %d: want %s, got %s", i, orig[i], in[i]) @@ -625,7 +625,7 @@ func TestCapCMTServerVersions(t *testing.T) { t.Run("drops unparseable values when mixed with valid versions", func(t *testing.T) { in := []string{"not-a-version", "11.10.0", "also-bad", "11.9.0", "11.8.0", "11.7.7", "10.11.22"} - got := capCMTServerVersions(in) + got := capCMTServerVersions(in, maxCMTServerVersions) want := []string{"10.11.22", "11.7.7", "11.8.0", "11.9.0", "11.10.0"} if len(got) != len(want) { t.Fatalf("expected %v, got %v", want, got) From 5d137e8805e33055de81c57d6071ae6b75bf2721 Mon Sep 17 00:00:00 2001 From: yasserfaraazkhan Date: Fri, 31 Jul 2026 19:46:29 +0530 Subject: [PATCH 2/2] fix(review): align PR max-age fallback with default, retry cleanup deletes Addresses CodeRabbit review on #94. e2ePRInstanceMaxAge() fell back to 24h when E2EPRInstanceMaxAge was unset while config-matterwick.default.json now ships 8h. Align the fallback (and its test) with the default so an unset key behaves like the shipped config. createCloudInstallation's cleanup discarded DeleteInstallation errors, so a single failed delete orphaned a paid server: the caller drops the ID on return and a retry provisions under a fresh name, leaving nothing to reference it until the periodic stale scan reaps it hours later. Cleanup now retries the delete (3 attempts) and reports an unrecoverable orphan to Mattermost with the installation ID. Adds coverage for the post-create failure paths: status-poll failure, delete failure after creation, and per-attempt cleanup during retry. Co-Authored-By: Claude Opus 5 --- server/e2e_dryrun_test.go | 2 +- server/e2e_tests.go | 47 ++++++++++++++++++++-- server/e2e_tests_test.go | 83 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 5 deletions(-) diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index 26558bc..5eb0f59 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -1543,7 +1543,7 @@ func TestE2EPRInstanceMaxAge(t *testing.T) { s := newDryRunServer(t, "", "mattermost") s.Config.E2EPRInstanceMaxAge = 0 - assert.Equal(t, 24*time.Hour, s.e2ePRInstanceMaxAge(), "0 should fall back to 24h default") + assert.Equal(t, 8*time.Hour, s.e2ePRInstanceMaxAge(), "0 should fall back to the 8h default in config-matterwick.default.json") s.Config.E2EPRInstanceMaxAge = 48 assert.Equal(t, 48*time.Hour, s.e2ePRInstanceMaxAge(), "configured value should win") diff --git a/server/e2e_tests.go b/server/e2e_tests.go index 12032d0..a6ed604 100644 --- a/server/e2e_tests.go +++ b/server/e2e_tests.go @@ -327,6 +327,46 @@ func (s *Server) createMultipleE2EInstances(pr *model.PullRequest, instanceType return instances, nil } +// installationDeleteAttempts is how many times cleanup tries to delete an installation it +// created but could not bring up. A single failed delete would otherwise orphan a paid +// server: the caller has already discarded the ID by the time it returns, and a retry +// provisions under a fresh name, so nothing else references the old one until the periodic +// stale scan reaps it hours later. +const installationDeleteAttempts = 3 + +// installationDeleteRetryDelay is the pause between delete attempts. Var, not const, so +// tests can shorten it. +var installationDeleteRetryDelay = 2 * time.Second + +// deleteInstallationWithRetry deletes installationID, retrying transient provisioner +// failures. If every attempt fails the installation is orphaned, so it reports to +// Mattermost with the ID — the periodic stale scan is the backstop, but it runs hours +// later and a silent orphan bills until then. +func (s *Server) deleteInstallationWithRetry(installationID, name string, logger logrus.FieldLogger) { + logger = logger.WithFields(logrus.Fields{ + "installation_id": installationID, + "instance": name, + }) + + var lastErr error + for attempt := 1; attempt <= installationDeleteAttempts; attempt++ { + if lastErr = s.CloudClient.DeleteInstallation(installationID); lastErr == nil { + return + } + logger.WithError(lastErr).WithFields(logrus.Fields{ + "attempt": attempt, + "attempts": installationDeleteAttempts, + }).Warn("Failed to clean up partially created installation") + if attempt < installationDeleteAttempts { + time.Sleep(installationDeleteRetryDelay) + } + } + + logger.WithError(lastErr).Error("Gave up deleting partially created installation; it is orphaned until the periodic stale scan reaps it") + s.logErrorToMattermost("Orphaned E2E installation %s (%s): delete failed %d times (%v). The periodic stale scan will retry, but it may need manual cleanup.", + installationID, name, installationDeleteAttempts, lastErr) +} + // createCloudInstallationAttempts is how many times a single instance is provisioned before // giving up. A fresh name per attempt avoids colliding with the failed attempt's DNS record. const createCloudInstallationAttempts = 2 @@ -425,9 +465,7 @@ func (s *Server) createCloudInstallation(ctx context.Context, name, version, use } cleanupCreatedInstallation := func(cause error) error { - if delErr := s.CloudClient.DeleteInstallation(installation.ID); delErr != nil { - logger.WithError(delErr).WithField("installation_id", installation.ID).Error("Failed to clean up partially created installation") - } + s.deleteInstallationWithRetry(installation.ID, name, logger) return cause } @@ -790,7 +828,8 @@ func (s *Server) e2ePRInstanceMaxAge() time.Duration { if s.Config.E2EPRInstanceMaxAge > 0 { return time.Duration(s.Config.E2EPRInstanceMaxAge) * time.Hour } - return 24 * time.Hour + // Keep in sync with E2EPRInstanceMaxAge in config-matterwick.default.json. + return 8 * time.Hour } // cleanupStaleE2EInstances reaps aged-out E2E instances: non-PR flows use e2eInstanceMaxAge, PR instances use e2ePRInstanceMaxAge (PR servers are kept alive for reuse; the cap prevents indefinite accumulation). diff --git a/server/e2e_tests_test.go b/server/e2e_tests_test.go index 32dd43f..bb9afa1 100644 --- a/server/e2e_tests_test.go +++ b/server/e2e_tests_test.go @@ -12,6 +12,7 @@ import ( "slices" "strings" "testing" + "time" mattermostModel "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/matterwick/model" @@ -1098,3 +1099,85 @@ func sortedKeys(m map[string]any) []string { slices.Sort(keys) return keys } + +// TestCleanupOfPartiallyCreatedInstallation covers the failure paths that follow a +// successful CreateInstallation: the status poll failing, and the cleanup delete failing. +// A dropped delete orphans a paid server, so cleanup retries and then reports it. +func TestCleanupOfPartiallyCreatedInstallation(t *testing.T) { + // Keep the delete backoff out of test wall-clock. + originalDelay := installationDeleteRetryDelay + installationDeleteRetryDelay = time.Millisecond + t.Cleanup(func() { installationDeleteRetryDelay = originalDelay }) + + // provisioner accepts creation, then fails the status poll so cleanup runs. + // deleteStatus controls what DELETE /api/installation/{id} returns; the cloud client + // treats only 202 Accepted as a successful delete. + newServer := func(t *testing.T, deleteStatus int, deletes *int) *Server { + t.Helper() + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && r.URL.Path == "/api/installations": + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"ID":"install-abc","State":"creation-requested","OwnerID":"mobile-11-7-7-site-3-abcd1234"}`)) + case r.Method == http.MethodDelete: + *deletes++ + w.WriteHeader(deleteStatus) + default: // GET status — fail the poll to force the cleanup path + w.WriteHeader(http.StatusInternalServerError) + } + })) + t.Cleanup(ts.Close) + return &Server{ + Config: &MatterwickConfig{ + DNSNameTestServer: "test.example.com", + E2EUsername: "admin", + }, + CloudClient: model.NewCloudClient(ts.URL, "", "", "", ""), + Logger: logrus.New(), + } + } + + t.Run("status poll failure deletes the created installation", func(t *testing.T) { + deletes := 0 + server := newServer(t, http.StatusAccepted, &deletes) + + instance, err := server.createCloudInstallation( + context.Background(), "mobile-11-7-7-site-3-abcd1234", "11.7.7", "admin", "pw", "mobile", server.Logger) + + require.Error(t, err) + assert.Nil(t, instance) + assert.Contains(t, err.Error(), "failed to get installation status") + assert.Equal(t, 1, deletes, "a successful delete must not be retried") + }) + + t.Run("delete failure is retried, not dropped", func(t *testing.T) { + deletes := 0 + server := newServer(t, http.StatusInternalServerError, &deletes) + + _, err := server.createCloudInstallation( + context.Background(), "mobile-11-7-7-site-3-abcd1234", "11.7.7", "admin", "pw", "mobile", server.Logger) + + require.Error(t, err, "the provisioning error must still surface, not the delete error") + assert.Contains(t, err.Error(), "failed to get installation status") + assert.Equal(t, installationDeleteAttempts, deletes, + "every delete attempt must be spent before the installation is declared orphaned") + }) + + t.Run("retry cleans up each failed attempt", func(t *testing.T) { + deletes := 0 + server := newServer(t, http.StatusAccepted, &deletes) + + i := 0 + nameFn := func() string { + i++ + return fmt.Sprintf("mobile-11-7-7-site-3-uid%d", i) + } + + _, err := server.createCloudInstallationWithRetry( + context.Background(), nameFn, "11.7.7", "admin", "pw", "mobile", server.Logger) + + require.Error(t, err) + assert.Equal(t, createCloudInstallationAttempts, deletes, + "each attempt's installation must be deleted, not leaked when the next attempt starts") + }) +}