diff --git a/config/config-matterwick.default.json b/config/config-matterwick.default.json index 08816fc..29b565a 100644 --- a/config/config-matterwick.default.json +++ b/config/config-matterwick.default.json @@ -78,10 +78,12 @@ "E2EUsername": "admin", "E2EPassword": "", "E2EServerVersion": "latest", - "E2EAutoTriggerOnRelease": true, "E2EAutoTriggerOnMaster": true, "E2EReleasePatternPrefix": "release-", - "E2ENightlyTriggerWorkflowName": "E2E Nightly Trigger", "E2ETestWorkflowNames": ["Electron Playwright Tests", "E2E", "Compatibility Matrix Testing"], - "E2EInstanceMaxAge": 6 + "E2EInstanceMaxAge": 6, + "E2EPRInstanceMaxAge": 24, + "CMTTriggerWorkflowName": "CMT Provisioner", + "CMTTestWorkflowName": "Compatibility Matrix Testing", + "CMTServerVersions": ["10.11.18", "11.7.1"] } diff --git a/server/config.go b/server/config.go index 88b894d..a01ddb3 100644 --- a/server/config.go +++ b/server/config.go @@ -112,19 +112,48 @@ type MatterwickConfig struct { E2EUsername string E2EPassword string E2EServerVersion string - E2EAutoTriggerOnRelease bool - E2EAutoTriggerOnMaster bool - E2EReleasePatternPrefix string - E2ENightlyTriggerWorkflowName string // workflow name (name: field) of the nightly trigger workflow - E2ETestWorkflowNames []string // workflow names of the actual test workflows (for completion-based cleanup) + E2EAutoTriggerOnMaster bool + E2EReleasePatternPrefix string + E2ETestWorkflowNames []string // workflow names of the actual test workflows (for completion-based cleanup) // E2EInstanceMaxAge is the minimum age (in hours) a non-PR E2E instance must reach // before the periodic orphan-cleanup scan will delete it. This prevents the scan // from destroying instances that are still being used by a currently-running test. // Set to the longest expected E2E run duration plus a small buffer. // Default (0): 3 hours. E2EInstanceMaxAge int + + // E2EPRInstanceMaxAge is the maximum age (in hours) a PR E2E instance may reach before + // the periodic cleanup scan deletes it. PR instances are intentionally kept alive between + // label toggles and across commits so the same servers can be reused for re-runs, so this + // is much longer than E2EInstanceMaxAge. When such an instance is reaped its in-memory + // tracking entry is also evicted, so re-applying E2E/Run provisions a fresh set. + // Default (0): 24 hours. + E2EPRInstanceMaxAge int + + // CMTTriggerWorkflowName is the workflow name (the "name:" field) of the lightweight + // CMT trigger workflow in the desktop/mobile repos. Matterwick provisions instances and + // dispatches compatibility-matrix-testing.yml when it receives a workflow_run "requested" + // event for this workflow. + CMTTriggerWorkflowName string + + // CMTTestWorkflowName is the workflow name of the actual CMT test workflow + // (compatibility-matrix-testing.yml). Used to distinguish CMT completions (cleanup by + // run ID) from regular E2E completions (cleanup by SHA). + CMTTestWorkflowName string + + // CMTServerVersions is an OPTIONAL manual override for the CMT version set. When non-empty + // it is used verbatim (values must be valid Mattermost image tags: full semver, no "v" + // prefix, e.g. "10.11.0"). When empty (the normal case) matterwick auto-derives the set + // from Mattermost's GitHub releases via Server.resolveCMTServerVersions. + CMTServerVersions []string + } +// defaultCMTServerVersions is the fallback CMT version set used only when auto-resolution +// fails (GitHub API error) and no manual override is configured. Kept reasonably current: +// the active v10.11 ESR plus a recent v11 release. +var defaultCMTServerVersions = []string{"10.11.18", "11.7.1"} + func findConfigFile(fileName string) string { if _, err := os.Stat("/tmp/" + fileName); err == nil { fileName, _ = filepath.Abs("/tmp/" + fileName) diff --git a/server/e2e_dryrun_test.go b/server/e2e_dryrun_test.go index 5d14b51..47e6187 100644 --- a/server/e2e_dryrun_test.go +++ b/server/e2e_dryrun_test.go @@ -187,18 +187,6 @@ func TestDryRun_DesktopDispatch(t *testing.T) { } }) - t.Run("three platforms created for desktop", func(t *testing.T) { - assert.Len(t, instances, 3) - platforms := []string{instances[0].Platform, instances[1].Platform, instances[2].Platform} - assert.Equal(t, []string{"linux", "macos", "windows"}, platforms) - }) - - t.Run("runner assignment is correct", func(t *testing.T) { - assert.Equal(t, "ubuntu-latest", instances[0].Runner) - assert.Equal(t, "macos-latest", instances[1].Runner) - assert.Equal(t, "windows-2022", instances[2].Runner) - }) - t.Run("workflow inputs built correctly", func(t *testing.T) { // Drive the real triggerDesktopE2EWorkflow so assertions validate the // actual payload produced by production code, not a hand-built map. @@ -226,12 +214,6 @@ func TestDryRun_DesktopDispatch(t *testing.T) { assert.NotEmpty(t, c.Inputs["instance_details"]) }) - t.Run("workflow path targets e2e-functional.yml", func(t *testing.T) { - path := fmt.Sprintf("/repos/%s/%s/actions/workflows/%s/dispatches", - "mattermost", "mattermost-desktop", "e2e-functional.yml") - assert.Contains(t, path, "e2e-functional.yml") - assert.Contains(t, path, "mattermost-desktop") - }) } // ------------------------------------------------------------ @@ -288,30 +270,6 @@ func TestDryRun_MobileDispatch(t *testing.T) { }) } - t.Run("mobile platforms are site-1/2/3 not linux/macos/windows", func(t *testing.T) { - platforms := []string{instances[0].Platform, instances[1].Platform, instances[2].Platform} - assert.Equal(t, []string{"site-1", "site-2", "site-3"}, platforms) - }) - - t.Run("mobile instances have no runner", func(t *testing.T) { - for _, inst := range instances { - assert.Empty(t, inst.Runner) - } - }) - - t.Run("triggerMobileE2EWorkflow requires exactly 3 instances", func(t *testing.T) { - // Verify the guard: only 2 instances → error path - twoInstances := instances[:2] - assert.NotEqual(t, 3, len(twoInstances), - "should fail the len(instances)!=3 check in triggerMobileE2EWorkflow") - }) - - t.Run("workflow path targets e2e-detox-pr.yml", func(t *testing.T) { - path := fmt.Sprintf("/repos/%s/%s/actions/workflows/%s/dispatches", - "mattermost", "mattermost-mobile", "e2e-detox-pr.yml") - assert.Contains(t, path, "e2e-detox-pr.yml") - assert.Contains(t, path, "mattermost-mobile") - }) } // ------------------------------------------------------------ @@ -347,66 +305,6 @@ func TestDryRun_LabelDetection(t *testing.T) { } } -// ------------------------------------------------------------ -// 4. Repo type → correct platforms and workflow -// ------------------------------------------------------------ - -func TestDryRun_RepoTypeDetection(t *testing.T) { - tests := []struct { - repoName string - wantType string - wantPlatforms []string - wantWorkflow string - }{ - { - repoName: "mattermost-desktop", - wantType: "desktop", - wantPlatforms: []string{"linux", "macos", "windows"}, - wantWorkflow: "e2e-functional.yml", - }, - { - repoName: "mattermost-desktop-releases", - wantType: "desktop", - wantPlatforms: []string{"linux", "macos", "windows"}, - wantWorkflow: "e2e-functional.yml", - }, - { - repoName: "mattermost-mobile", - wantType: "mobile", - wantPlatforms: []string{"site-1", "site-2", "site-3"}, - wantWorkflow: "e2e-detox-pr.yml", - }, - { - repoName: "mattermost-mobile-v2", - wantType: "mobile", - wantPlatforms: []string{"site-1", "site-2", "site-3"}, - wantWorkflow: "e2e-detox-pr.yml", - }, - } - - for _, tt := range tests { - t.Run(tt.repoName, func(t *testing.T) { - var instanceType string - var platforms []string - var workflow string - - if strings.Contains(tt.repoName, "desktop") { - instanceType = "desktop" - platforms = []string{"linux", "macos", "windows"} - workflow = "e2e-functional.yml" - } else if strings.Contains(tt.repoName, "mobile") { - instanceType = "mobile" - platforms = []string{"site-1", "site-2", "site-3"} - workflow = "e2e-detox-pr.yml" - } - - assert.Equal(t, tt.wantType, instanceType) - assert.Equal(t, tt.wantPlatforms, platforms) - assert.Equal(t, tt.wantWorkflow, workflow) - }) - } -} - // ------------------------------------------------------------ // 5. Desktop push event logic // ------------------------------------------------------------ @@ -458,17 +356,6 @@ func TestDryRun_DesktopPushEvent(t *testing.T) { "extractBranchName is unaware of ref type; caller must pre-filter") }) - t.Run("desktop push always creates linux/macos/windows instances", func(t *testing.T) { - // createMultipleE2EInstancesForPushEvent uses desktop platforms for push events - expectedPlatforms := []string{"linux", "macos", "windows"} - instances := makeDesktopInstances() - var gotPlatforms []string - for _, inst := range instances { - gotPlatforms = append(gotPlatforms, inst.Platform) - } - assert.Equal(t, expectedPlatforms, gotPlatforms) - }) - t.Run("desktop push instance_details carries server_version", func(t *testing.T) { instances := makeDesktopInstances() instanceDetailsJSON, err := s.buildInstanceDetailsJSON(instances) @@ -482,47 +369,6 @@ func TestDryRun_DesktopPushEvent(t *testing.T) { }) } -// ------------------------------------------------------------ -// 6. Mobile push event logic -// ------------------------------------------------------------ - -func TestDryRun_MobilePushEvent(t *testing.T) { - t.Run("mobile push uses SITE_1/2/3_URL inputs not instance_details", func(t *testing.T) { - instances := makeMobileInstances() - sha := "sha999" - branch := "release-8.0" - - // Simulate triggerMobileE2EWorkflowForPushEvent inputs - inputs := map[string]interface{}{ - "SITE_1_URL": instances[0].URL, - "SITE_2_URL": instances[1].URL, - "SITE_3_URL": instances[2].URL, - "MOBILE_VERSION": sha, - "PLATFORM": "both", - } - - assert.Equal(t, "https://site1.test.example.com", inputs["SITE_1_URL"]) - assert.Equal(t, "https://site2.test.example.com", inputs["SITE_2_URL"]) - assert.Equal(t, "https://site3.test.example.com", inputs["SITE_3_URL"]) - assert.Equal(t, sha, inputs["MOBILE_VERSION"]) - assert.Equal(t, "both", inputs["PLATFORM"]) - assert.NotContains(t, inputs, "instance_details", - "mobile push must not use instance_details") - _ = branch - }) - - t.Run("mobile push always tests both platforms", func(t *testing.T) { - // Push events (release/master) always use PLATFORM=both (no label context) - platform := "both" - assert.Equal(t, "both", platform) - }) - - t.Run("mobile push requires 3 instances", func(t *testing.T) { - instances := makeMobileInstances() - assert.Len(t, instances, 3) - }) -} - // ------------------------------------------------------------ // 7. Desktop CMT logic // ------------------------------------------------------------ @@ -533,22 +379,6 @@ func TestDryRun_DesktopCMT(t *testing.T) { assert.Equal(t, []string{"v11.1.0", "v11.2.0", "v12.0.0"}, versions) }) - t.Run("caps server versions to 5", func(t *testing.T) { - versions := parseServerVersionsFromString("v1, v2, v3, v4, v5, v6, v7") - // The cap is enforced inside handleCMTWithServerVersions - if len(versions) > 5 { - versions = versions[:5] - } - assert.Len(t, versions, 5) - }) - - t.Run("1 instance per version for CMT (matrix handles parallelism)", func(t *testing.T) { - for _, numVersions := range []int{1, 2, 3, 5} { - // CMT_MATRIX cross-products environment × server; one server per version is enough - assert.Equal(t, numVersions, numVersions*1) - } - }) - t.Run("buildDesktopCMTMatrixJSON produces correct schema", func(t *testing.T) { versions := []string{"v11.1.0", "v11.2.0"} instances := []*E2EInstance{ @@ -571,26 +401,22 @@ func TestDryRun_DesktopCMT(t *testing.T) { s0 := servers[0].(map[string]interface{}) assert.Equal(t, "v11.1.0", s0["version"]) assert.Equal(t, "https://v1.example.com", s0["url"]) + // Desktop ignores the `latest` field; cmtServer.Latest is shared with mobile but is + // never set on the desktop path, and `omitempty` keeps it out of the JSON entirely. + _, has0 := s0["latest"] + assert.False(t, has0, "desktop matrix must not carry the `latest` field") s1 := servers[1].(map[string]interface{}) assert.Equal(t, "v11.2.0", s1["version"]) assert.Equal(t, "https://v2.example.com", s1["url"]) + _, has1 := s1["latest"] + assert.False(t, has1, "desktop matrix must not carry the `latest` field") }) - t.Run("CMT dispatches compatibility-matrix-testing.yml once", func(t *testing.T) { - // One dispatch regardless of version count — all versions in CMT_MATRIX.server array - dispatchCount := 1 - assert.Equal(t, 1, dispatchCount, "desktop CMT must dispatch exactly once") - }) - - t.Run("CMT tracking key includes runID for uniqueness and sha for cleanup", func(t *testing.T) { + t.Run("CMT tracking key is keyed by dispatched test run id", func(t *testing.T) { repoName := "mattermost-desktop" - sha := "deadbeef" - var runID int64 = 999 - // runID prevents collision when two dispatches share the same branch HEAD SHA; - // key still ends with "-{sha}" so findAndDestroyInstancesBySHA can match it. - key := fmt.Sprintf("%s-cmt-%d-%s", repoName, runID, sha) - assert.Equal(t, "mattermost-desktop-cmt-999-deadbeef", key) - assert.True(t, strings.HasSuffix(key, "-"+sha), "key must end with sha for cleanup") + var testRunID int64 = 999 + key := cmtInstanceKey(repoName, testRunID) + assert.Equal(t, "mattermost-desktop-cmt-999", key) }) t.Run("CMT workflow name detection", func(t *testing.T) { @@ -630,16 +456,16 @@ func TestDryRun_MobileCMT(t *testing.T) { s0 := servers[0].(map[string]interface{}) assert.Equal(t, "v11.1.0", s0["version"]) assert.Equal(t, "https://v1.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") s1 := servers[1].(map[string]interface{}) assert.Equal(t, "v11.2.0", s1["version"]) assert.Equal(t, "https://v2.example.com", s1["url"]) - }) - - t.Run("mobile CMT dispatches once not once per version", func(t *testing.T) { - // All versions go into CMT_MATRIX.server; compatibility-matrix-testing.yml - // fans them out via its matrix strategy — no per-version dispatch needed. - dispatchCount := 1 - assert.Equal(t, 1, dispatchCount, "mobile CMT must dispatch exactly once") + // Highest semver gets `latest: true`. The mobile workflow uses this to decide whether + // to run the whole suite (latest) or just smoke (older) — that policy lives there, + // not in matterwick. + assert.Equal(t, true, s1["latest"]) }) t.Run("CMT_MATRIX uses server array not SITE_URL inputs", func(t *testing.T) { @@ -658,16 +484,82 @@ func TestDryRun_MobileCMT(t *testing.T) { assert.Contains(t, jsonStr, "\"url\"") }) - t.Run("mobile CMT single instance per version for matrix fan-out", func(t *testing.T) { - // Mobile CMT uses one server per version; compatibility-matrix-testing.yml - // creates one test job per server entry. - versions := []string{"v11.1.0", "v11.2.0", "v11.3.0"} + t.Run("mobile CMT marks the highest-semver entry as latest", func(t *testing.T) { + // 5-element resolved set (today's typical shape): ESR + 3 minors + current RC. + // Across the boundary cases that matter: ESR is older despite high patch numbers; + // RC vs stable for the same X.Y.Z should treat stable as higher; multi-digit RC + // numbers (rc.10 > rc.2). Locking these in so the workflow's latest gate doesn't + // silently shift if someone tweaks the comparator. + cases := []struct { + name string + versions []string + wantLatestIdx int + }{ + { + name: "ESR + 3 minors + RC: RC's base is newest so RC is latest", + versions: []string{"10.11.19", "11.5.7", "11.6.4", "11.7.2", "11.8.0-rc3"}, + wantLatestIdx: 4, + }, + { + name: "ESR with high patch loses to lower-patch newer minor", + versions: []string{"10.11.19", "11.0.0"}, + wantLatestIdx: 1, + }, + { + name: "stable beats same-X.Y.Z RC", + versions: []string{"11.7.0-rc3", "11.7.0"}, + wantLatestIdx: 1, + }, + { + name: "rc.10 > rc.2 (no string compare)", + versions: []string{"11.8.0-rc2", "11.8.0-rc10"}, + wantLatestIdx: 1, + }, + { + name: "single version is latest", + versions: []string{"11.7.2"}, + wantLatestIdx: 0, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + instances := make([]*E2EInstance, len(tc.versions)) + for i := range tc.versions { + instances[i] = &E2EInstance{URL: fmt.Sprintf("https://v%d.example.com", i)} + } + jsonStr, err := buildMobileCMTMatrixJSON(tc.versions, instances) + require.NoError(t, err) + var matrix map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(jsonStr), &matrix)) + servers := matrix["server"].([]interface{}) + require.Len(t, servers, len(tc.versions)) + for i, raw := range servers { + s := raw.(map[string]interface{}) + _, has := s["latest"] + if i == tc.wantLatestIdx { + assert.Equal(t, true, s["latest"], "index %d (%q) should be latest", i, tc.versions[i]) + } else { + assert.False(t, has, "index %d (%q) must not carry latest", i, tc.versions[i]) + } + } + }) + } + }) + + t.Run("mobile CMT: all unparseable versions => last entry marked latest", func(t *testing.T) { + versions := []string{"junk", "also-junk"} instances := []*E2EInstance{ - {URL: "https://v1.example.com"}, - {URL: "https://v2.example.com"}, - {URL: "https://v3.example.com"}, + {URL: "https://a.example.com"}, + {URL: "https://b.example.com"}, } - assert.Equal(t, len(versions), len(instances), "one instance per version") + jsonStr, err := buildMobileCMTMatrixJSON(versions, instances) + require.NoError(t, err) + var matrix map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(jsonStr), &matrix)) + servers := matrix["server"].([]interface{}) + _, has0 := servers[0].(map[string]interface{})["latest"] + assert.False(t, has0) + assert.Equal(t, true, servers[1].(map[string]interface{})["latest"]) }) } @@ -777,36 +669,117 @@ func TestDryRun_InstanceTracking(t *testing.T) { assert.Len(t, collected, 6, "should collect 3 instances from each of 2 push keys") }) - t.Run("CMT cleanup by sha via findAndDestroyInstancesBySHA", func(t *testing.T) { + t.Run("CMT cleanup by run id removes only the matching tracking key", func(t *testing.T) { repoName := "mattermost-desktop" - sha := "abc123cmt" - var runID int64 = 42 - key := fmt.Sprintf("%s-cmt-%d-%s", repoName, runID, sha) + var testRunID int64 = 42 + key := cmtInstanceKey(repoName, testRunID) cmtInstances := makeDesktopInstances() s.e2eInstancesLock.Lock() s.e2eInstances[key] = cmtInstances s.e2eInstancesLock.Unlock() - // Simulate findAndDestroyInstancesBySHA: scan for prefix+suffix match - prefix := repoName + "-" - suffix := "-" + sha + assert.True(t, instanceKeyMatchesRunID(key, repoName, testRunID)) + + // Exercise the live map-mutation helper, not a hand-rolled loop — so a future + // change to removeCMTInstancesByRunID's locking, key derivation, or return shape + // fails this test instead of silently passing. + removed := s.removeCMTInstancesByRunID(repoName, testRunID, s.Logger) + assert.Len(t, removed, 3, "must return the removed instances so the caller can destroy them") + s.e2eInstancesLock.Lock() - var found []*E2EInstance - for k, v := range s.e2eInstances { - if strings.HasPrefix(k, prefix) && strings.HasSuffix(k, suffix) { - found = append(found, v...) - delete(s.e2eInstances, k) - } - } + _, exists := s.e2eInstances[key] + s.e2eInstancesLock.Unlock() + assert.False(t, exists, "matching key must be removed from the tracking map") + }) + + t.Run("concurrent CMT runs on same SHA only destroy the completing run", func(t *testing.T) { + repoName := "mattermost-mobile" + run1 := int64(100) + run2 := int64(200) + key1 := cmtInstanceKey(repoName, run1) + key2 := cmtInstanceKey(repoName, run2) + + s.e2eInstancesLock.Lock() + s.e2eInstances[key1] = makeDesktopInstances() + s.e2eInstances[key2] = makeDesktopInstances() + s.e2eInstancesLock.Unlock() + + // Use the live map-mutation helper so this regression-tests the real path. + removed := s.removeCMTInstancesByRunID(repoName, run1, s.Logger) + assert.Len(t, removed, 3, "run1's instances must be returned") + + s.e2eInstancesLock.Lock() + _, exists1 := s.e2eInstances[key1] + _, exists2 := s.e2eInstances[key2] s.e2eInstancesLock.Unlock() + assert.False(t, exists1, "removeCMTInstancesByRunID must remove run1's key") + assert.True(t, exists2, "other concurrent CMT run must survive cleanup of cancelled run") - assert.Len(t, found, 3) + s.e2eInstancesLock.Lock() + delete(s.e2eInstances, key2) + s.e2eInstancesLock.Unlock() + }) + t.Run("CMT cleanup by run id is a no-op when run id is zero", func(t *testing.T) { + repoName := "mattermost-mobile" + key := cmtInstanceKey(repoName, 777) + s.e2eInstancesLock.Lock() + s.e2eInstances[key] = makeDesktopInstances() + s.e2eInstancesLock.Unlock() + defer func() { + s.e2eInstancesLock.Lock() + delete(s.e2eInstances, key) + s.e2eInstancesLock.Unlock() + }() + + // runID 0 is the "could not resolve" sentinel from pollDispatchedWorkflowRun. + // The helper must NOT match any key in that case. + removed := s.removeCMTInstancesByRunID(repoName, 0, s.Logger) + assert.Nil(t, removed, "zero run id must return nil so no destroy fires") s.e2eInstancesLock.Lock() _, exists := s.e2eInstances[key] s.e2eInstancesLock.Unlock() - assert.False(t, exists) + assert.True(t, exists, "zero run id must leave all keys intact") + }) +} + +// ------------------------------------------------------------ +// 9b. SHA-scoped cleanup must not reap a concurrent flow on the same SHA +// ------------------------------------------------------------ + +func TestInstanceKeyMatchesRunID(t *testing.T) { + repo := "mattermost-mobile" + runID := int64(100) + cmtKey := cmtInstanceKey(repo, runID) + otherRunKey := cmtInstanceKey(repo, 200) + + assert.True(t, instanceKeyMatchesRunID(cmtKey, repo, runID)) + assert.False(t, instanceKeyMatchesRunID(otherRunKey, repo, runID)) + assert.False(t, instanceKeyMatchesRunID(cmtKey, "mattermost-desktop", runID)) +} + +func TestInstanceKeyMatchesSHA(t *testing.T) { + repo := "mattermost-mobile" + sha := "deadbeef" + nightlyKey := fmt.Sprintf("%s-scheduled-200-%s", repo, sha) // nightly flow, same SHA + pushKey := fmt.Sprintf("%s-push-release-9.0-%s", repo, sha) // push flow, same SHA + prKey := fmt.Sprintf("%s-pr-42", repo) // PR flow, no -sha suffix + legacyCMTKey := fmt.Sprintf("%s-cmt-100-%s", repo, sha) // legacy CMT key shape (runID + sha) — pre-refactor + liveCMTKey := cmtInstanceKey(repo, 100) // live CMT key shape ({repo}-cmt-{runID}, no sha) + otherSHAKey := fmt.Sprintf("%s-cmt-100-%s", repo, "feedface") // legacy CMT key with different sha + + t.Run("non-CMT completion matches push/scheduled but NEVER any CMT shape", func(t *testing.T) { + assert.True(t, instanceKeyMatchesSHA(nightlyKey, repo, sha, false)) + assert.True(t, instanceKeyMatchesSHA(pushKey, repo, sha, false)) + assert.False(t, instanceKeyMatchesSHA(prKey, repo, sha, false), "PR keys have no -sha suffix") + assert.False(t, instanceKeyMatchesSHA(legacyCMTKey, repo, sha, false), "legacy CMT keys are CMT-prefixed and never SHA-cleaned by non-CMT completions") + assert.False(t, instanceKeyMatchesSHA(liveCMTKey, repo, sha, false), "live CMT keys have no -sha suffix so cannot match a non-CMT SHA cleanup") + assert.False(t, instanceKeyMatchesSHA(otherSHAKey, repo, sha, false), "legacy CMT key with different sha must not match") + }) + + t.Run("other repo is never matched", func(t *testing.T) { + assert.False(t, instanceKeyMatchesSHA("mattermost-desktop-cmt-100-"+sha, repo, sha, true)) }) } @@ -838,16 +811,6 @@ func TestDryRun_InstanceNameLength(t *testing.T) { assert.LessOrEqual(t, len(instanceName)+len(dnsDomain), 63) }) - t.Run("CMT single instance name sanitization replaces dots", func(t *testing.T) { - version := "v11.1.0" - // createSingleCMTInstance lowercases and replaces dots - sanitizedVersion := strings.ToLower(strings.ReplaceAll(version, ".", "-")) - assert.Equal(t, "v11-1-0", sanitizedVersion) - - // Single instance suffix: no platform component (matrix handles that) - suffix := fmt.Sprintf("-cmt-%s", sanitizedVersion) - assert.Equal(t, "-cmt-v11-1-0", suffix) - }) } // ------------------------------------------------------------ @@ -1286,24 +1249,6 @@ func TestDryRun_MMServerVersionFromInstance(t *testing.T) { assert.Equal(t, "https://site3.test.example.com", c.Inputs["SITE_3_URL"]) }) - t.Run("all instances in a PR run share the same resolved version", func(t *testing.T) { - // createMultipleE2EInstances calls resolveMattermostServerVersion() once and passes - // the same version to all createCloudInstallation calls. - resolvedVersion := "11.6.0" - platforms := []string{"linux", "macos", "windows"} - instances := make([]*E2EInstance, len(platforms)) - for i, p := range platforms { - instances[i] = &E2EInstance{ - Platform: p, - ServerVersion: resolvedVersion, // same version for every instance - } - } - for i, inst := range instances { - assert.Equal(t, resolvedVersion, inst.ServerVersion, - "instance[%d] (platform=%s) must have the resolved version", i, inst.Platform) - } - }) - t.Run("resolveMattermostServerVersion with latest returns Docker Hub compatible version", func(t *testing.T) { // Docker Hub tags are bare semver (e.g. "11.6.0"), NOT "v11.6.0". // Verify the v-stripping produces a Docker Hub compatible string. @@ -1323,26 +1268,6 @@ func TestDryRun_MMServerVersionFromInstance(t *testing.T) { // ------------------------------------------------------------ func TestDryRun_CMTVersionNormalization(t *testing.T) { - t.Run("v-prefix stripped before instance creation", func(t *testing.T) { - // handleCMTWithServerVersions strips "v" from each version before provisioning. - // Verify that strings.TrimPrefix produces Docker Hub compatible versions. - inputs := []struct { - input string - want string - }{ - {"v11.0.1", "11.0.1"}, - {"v11.1.0", "11.1.0"}, - {"v12.0.0", "12.0.0"}, - {"11.0.1", "11.0.1"}, // no v — unchanged - {"11.1.0", "11.1.0"}, // no v — unchanged - {"v11.6.0-rc1", "11.6.0-rc1"}, // RC: v stripped but rest preserved - } - for _, tt := range inputs { - got := strings.TrimPrefix(tt.input, "v") - assert.Equal(t, tt.want, got, "TrimPrefix(%q, 'v')", tt.input) - } - }) - t.Run("comma-separated input parsed and v-stripped", func(t *testing.T) { // parseServerVersionsFromString splits; the CMT loop then strips v from each. raw := "v11.0.1, v11.1.0, 11.2.0" @@ -1356,26 +1281,6 @@ func TestDryRun_CMTVersionNormalization(t *testing.T) { assert.Equal(t, []string{"11.0.1", "11.1.0", "11.2.0"}, stripped) }) - t.Run("CMT instances carry stripped version in ServerVersion", func(t *testing.T) { - // Instances created by handleCMTWithServerVersions use the stripped version. - // Simulate by constructing instances as the real code would. - rawVersions := []string{"v11.0.1", "v11.1.0"} - var instances []*E2EInstance - for _, v := range rawVersions { - stripped := strings.TrimPrefix(v, "v") - instances = append(instances, &E2EInstance{ - URL: fmt.Sprintf("https://%s.test.example.com", stripped), - ServerVersion: stripped, - }) - } - assert.Equal(t, "11.0.1", instances[0].ServerVersion) - assert.Equal(t, "11.1.0", instances[1].ServerVersion) - for _, inst := range instances { - assert.False(t, strings.HasPrefix(inst.ServerVersion, "v"), - "CMT instance ServerVersion must not start with 'v'") - } - }) - t.Run("CMT matrix JSON contains stripped versions", func(t *testing.T) { // buildDesktopCMTMatrixJSON uses instance.ServerVersion directly. // With stripped versions, the matrix has Docker Hub compatible version strings. @@ -1407,13 +1312,288 @@ func TestDryRun_CMTVersionNormalization(t *testing.T) { assert.Equal(t, "11.1.0", s1["version"]) }) - t.Run("CMT versions capped at 5", func(t *testing.T) { - input := "v1.0.0, v2.0.0, v3.0.0, v4.0.0, v5.0.0, v6.0.0, v7.0.0" - parsed := parseServerVersionsFromString(input) - const maxVersions = 5 - if len(parsed) > maxVersions { - parsed = parsed[:maxVersions] +} + +// ------------------------------------------------------------ +// 15. resolveCMTServerVersions() — auto-derived CMT version set +// ------------------------------------------------------------ + +func TestDryRun_ResolveCMTServerVersions(t *testing.T) { + // A realistic releases payload (newest first): an upcoming RC, recent stable minors, + // and ESR lines flagged in the body. Includes multiple patches per line and a draft. + releasesBody := `[ + {"tag_name":"v11.8.0-rc3","draft":false,"prerelease":true,"body":"Mattermost Platform Release 11.8.0-rc3"}, + {"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 contains fixes."}, + {"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.6.3","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.6.3"}, + {"tag_name":"v11.5.7","draft":false,"prerelease":false,"body":"Mattermost Platform Release 11.5.7"}, + {"tag_name":"v11.99.0","draft":true,"prerelease":false,"body":"draft should be ignored"}, + {"tag_name":"v10.11.19","draft":false,"prerelease":false,"body":"Mattermost Platform Extended Support Release 10.11.19 contains security fixes."}, + {"tag_name":"v10.11.18","draft":false,"prerelease":false,"body":"Mattermost Platform Extended Support Release 10.11.18"} + ]` + + t.Run("auto-derives ESR + latest 3 minors + current RC, latest patch each", func(t *testing.T) { + srv := mockReleasesServer(t, releasesBody, http.StatusOK) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + got := s.resolveCMTServerVersions() + // 10.11.19 (ESR) + 11.5.7/11.6.4/11.7.2 (latest 3 minors; 11.7 also ESR) + 11.8.0-rc3 (RC), + // latest patch per line, v-stripped, ascending. + assert.Equal(t, []string{"10.11.19", "11.5.7", "11.6.4", "11.7.2", "11.8.0-rc3"}, got) + }) + + t.Run("explicit config override is returned verbatim, no API call", func(t *testing.T) { + called := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + w.WriteHeader(http.StatusInternalServerError) + })) + t.Cleanup(srv.Close) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + s.Config.CMTServerVersions = []string{"9.11.0", "10.5.0"} + + assert.Equal(t, []string{"9.11.0", "10.5.0"}, s.cmtServerVersions()) + assert.False(t, called, "manual override must not hit the GitHub API") + }) + + t.Run("API error falls back to defaultCMTServerVersions", func(t *testing.T) { + srv := mockReleasesServer(t, "boom", http.StatusInternalServerError) + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + assert.Equal(t, defaultCMTServerVersions, s.resolveCMTServerVersions()) + }) + + t.Run("RC omitted when not newer than latest stable", func(t *testing.T) { + // Only stable releases here; an old RC for an already-released line must not appear. + body := `[ + {"tag_name":"v11.7.2","draft":false,"prerelease":false,"body":"Mattermost Platform Extended Support Release 11.7.2"}, + {"tag_name":"v11.7.0-rc1","draft":false,"prerelease":true,"body":"rc"}, + {"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 + "/" + + got := s.resolveCMTServerVersions() + assert.Equal(t, []string{"11.5.7", "11.6.4", "11.7.2"}, got, "stale RC must be excluded") + }) + + t.Run("parseCMTVersion handles stable, rc, and v-prefix; rejects junk", func(t *testing.T) { + v, ok := parseCMTVersion("v11.8.0-rc3") + assert.True(t, ok) + assert.Equal(t, "11.8.0-rc3", v.raw) + assert.Equal(t, 3, v.rc) + v2, ok2 := parseCMTVersion("10.11.19") + assert.True(t, ok2) + assert.Equal(t, 0, v2.rc) + _, ok3 := parseCMTVersion("v11.7.0-beta.1") + assert.False(t, ok3, "non-rc prerelease suffixes are not CMT versions") + _, ok4 := parseCMTVersion("not-a-version") + assert.False(t, ok4) + // stable sorts above its rc for the same X.Y.Z + assert.True(t, v.less(v2) == false) + }) +} + +// TestResolveBranchHeadSHA verifies the dispatch-time HEAD resolution used to key non-PR +// cleanup. Non-PR flows dispatch the test workflow with ref=branch, so the run's head_sha is +// the branch HEAD at dispatch time. We key cleanup on that resolved SHA (not the trigger SHA) +// so findAndDestroyInstancesBySHA matches when the run completes. +func TestResolveBranchHeadSHA(t *testing.T) { + t.Run("returns the branch HEAD sha from the commits API", func(t *testing.T) { + var gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"sha":"abc123def456"}`)) + })) + t.Cleanup(srv.Close) + + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + sha, err := s.resolveBranchHeadSHA("mattermost", "desktop", "release-12.0") + assert.NoError(t, err) + assert.Equal(t, "abc123def456", sha) + assert.Equal(t, "/repos/mattermost/desktop/commits/release-12.0", gotPath) + }) + + t.Run("errors on non-2xx so caller can fall back to trigger sha", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(srv.Close) + + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + _, err := s.resolveBranchHeadSHA("mattermost", "desktop", "no-such-branch") + assert.Error(t, err) + }) + + t.Run("errors on empty sha so caller can fall back to trigger sha", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"sha":""}`)) + })) + t.Cleanup(srv.Close) + + s := newDryRunServer(t, "", "mattermost") + s.githubAPIBase = srv.URL + "/" + + _, err := s.resolveBranchHeadSHA("mattermost", "desktop", "main") + assert.Error(t, err) + }) +} + +// TestE2EPRInstanceMaxAge verifies the PR max-age knob: configured value wins, else 24h default. +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") + + s.Config.E2EPRInstanceMaxAge = 48 + assert.Equal(t, 48*time.Hour, s.e2ePRInstanceMaxAge(), "configured value should win") +} + +// TestEvictReapedPRInstances verifies that when the periodic scan reaps a PR's servers, the +// in-memory tracking entry is removed so the next E2E/Run provisions a fresh set rather than +// reusing now-deleted servers. Non-PR (SHA-keyed) entries must be left untouched. +func TestEvictReapedPRInstances(t *testing.T) { + t.Run("evicts the PR key when any of its instances was reaped", func(t *testing.T) { + s := newDryRunServer(t, "", "mattermost") + s.e2eInstances["desktop-pr-42"] = []*E2EInstance{ + {InstallationID: "inst-a", Platform: "linux"}, + {InstallationID: "inst-b", Platform: "macos"}, + {InstallationID: "inst-c", Platform: "windows"}, } - assert.Len(t, parsed, 5, "CMT versions must be capped at 5") + + // Only one member reaped, but the whole set ages out together, so the key goes. + s.evictReapedPRInstances([]string{"inst-b"}, s.Logger) + + _, ok := s.e2eInstances["desktop-pr-42"] + assert.False(t, ok, "PR key must be evicted so re-applying E2E/Run creates a fresh set") + }) + + t.Run("leaves unrelated PR keys and non-PR (SHA-keyed) entries intact", func(t *testing.T) { + s := newDryRunServer(t, "", "mattermost") + s.e2eInstances["desktop-pr-42"] = []*E2EInstance{{InstallationID: "inst-a"}} + s.e2eInstances["mattermost-mobile-pr-9"] = []*E2EInstance{{InstallationID: "inst-x"}} + s.e2eInstances["desktop-cmt-555-deadbeef"] = []*E2EInstance{{InstallationID: "inst-cmt"}} + + s.evictReapedPRInstances([]string{"inst-a"}, s.Logger) + + _, gone := s.e2eInstances["desktop-pr-42"] + assert.False(t, gone, "the matching PR key is evicted") + _, otherPR := s.e2eInstances["mattermost-mobile-pr-9"] + assert.True(t, otherPR, "an unrelated PR key must remain") + _, cmt := s.e2eInstances["desktop-cmt-555-deadbeef"] + assert.True(t, cmt, "a non-PR (SHA-keyed) entry must remain") }) + + t.Run("no reaped IDs is a no-op", func(t *testing.T) { + s := newDryRunServer(t, "", "mattermost") + s.e2eInstances["desktop-pr-42"] = []*E2EInstance{{InstallationID: "inst-a"}} + + s.evictReapedPRInstances(nil, s.Logger) + + _, ok := s.e2eInstances["desktop-pr-42"] + assert.True(t, ok, "nothing reaped means nothing evicted") + }) +} + +// TestShouldTriggerCMT verifies CMT gating across all sources: manual dispatch (any ref), RC +// tag cut (the new primary trigger), and release branch (defense-in-depth). Anything else — +// feature branches, GA tags, nightly tags, beta tags, default branch — must be rejected so +// that mobile's `on: push tags` glob slips and stray runs don't burn the multi-version matrix. +func TestShouldTriggerCMT(t *testing.T) { + s := newDryRunServer(t, "", "mattermost") + + // Manual dispatch always runs, regardless of ref. + assert.True(t, s.shouldTriggerCMT("workflow_dispatch", "main")) + assert.True(t, s.shouldTriggerCMT("workflow_dispatch", "v6.2.0-rc.1")) + assert.True(t, s.shouldTriggerCMT("workflow_dispatch", "release-6.2")) + + // RC tag cut (primary trigger). For tag pushes head_branch is the tag name. + assert.True(t, s.shouldTriggerCMT("push", "v6.2.0-rc.1")) // desktop convention + assert.True(t, s.shouldTriggerCMT("push", "v2.41.0-rc.1")) // future mobile + assert.True(t, s.shouldTriggerCMT("push", "v6.2.0-rc.10")) // multi-digit rc + assert.True(t, s.shouldTriggerCMT("push", "6.2.0-rc.1")) // missing 'v' prefix is permitted + assert.True(t, s.shouldTriggerCMT("push", "v6.2.0-rc1")) // no separator before number + + // Release branch (defense-in-depth — kept for backwards compat / manual triggers). + assert.True(t, s.shouldTriggerCMT("push", "release-6.2")) + + // Must NOT trigger: GA tags, betas, nightly tags, feature branches, default branch. + assert.False(t, s.shouldTriggerCMT("push", "v6.2.0")) // GA tag — no -rc + assert.False(t, s.shouldTriggerCMT("push", "v1.0.22-beta")) // pre-release but not RC + assert.False(t, s.shouldTriggerCMT("push", "6.3.0-nightly.20260601")) // nightly tag + assert.False(t, s.shouldTriggerCMT("push", "v6.2.0-rcabc")) // -rc but no number + assert.False(t, s.shouldTriggerCMT("create", "feature/cool-thing")) + assert.False(t, s.shouldTriggerCMT("push", "main")) + assert.False(t, s.shouldTriggerCMT("schedule", "main")) + + // Mobile build-release-NNN branch push (mobile's RC-cut equivalent). + assert.True(t, s.shouldTriggerCMT("push", "build-release-786")) // 3-digit + assert.True(t, s.shouldTriggerCMT("push", "build-release-1100")) // 4-digit + assert.False(t, s.shouldTriggerCMT("push", "build-release-12345")) // 5+ digit rejected; bump regex when convention changes + assert.False(t, s.shouldTriggerCMT("push", "build-release-1")) // < 3 digits, likely a test + assert.False(t, s.shouldTriggerCMT("push", "build-release-ios-707")) // platform variant +} + +// TestIsRCTag covers the RC-tag regex in isolation so the boundary cases stay locked in. +func TestIsRCTag(t *testing.T) { + for _, ref := range []string{"v6.2.0-rc.1", "v6.2.0-rc.10", "v2.41.0-rc.2", "6.2.0-rc.1", "v6.2.0-rc1", "v6.2.0-rc-1"} { + assert.True(t, isRCTag(ref), "expected RC tag: %q", ref) + } + for _, ref := range []string{ + "v6.2.0", // GA + "v6.2.0-rc", // missing number + "v6.2.0-rcabc", // letters after -rc + "v1.0.22-beta", // not RC + "6.3.0-nightly.20260601", // nightly + "release-6.2", // branch + "main", + "", + } { + assert.False(t, isRCTag(ref), "must not match: %q", ref) + } +} + +// TestIsBuildReleaseBranch locks in the boundaries for mobile's build-release-NNN gate: +// exactly 3-or-4 digits required (rejects test artifacts like build-release-1 AND +// rejects unsanctioned 5+ digit values); platform-specific variants +// (build-release-ios-NNN etc.) are rejected; no overlap with the RC-tag or +// release-* gates. +func TestIsBuildReleaseBranch(t *testing.T) { + for _, ref := range []string{ + "build-release-786", // real 3-digit + "build-release-1100", // real 4-digit + "build-release-9999", // upper end of 4-digit window + } { + assert.True(t, isBuildReleaseBranch(ref), "expected build-release branch: %q", ref) + } + for _, ref := range []string{ + "build-release-1", // 1 digit — test artifact / typo + "build-release-99", // 2 digit — below convention + "build-release-12345", // 5 digit — above convention; bump regex when crossing this + "build-release-ios-707", // platform-specific + "build-release-sim-707", // simulator-only + "build-release-android-1100", // android-specific + "build-release-786-rc1", // stray suffix + "build-release-", // no number + "build-release-abc", // non-numeric + "release-2.41", // handled by isReleaseBranch + "v2.41.0-rc.1", // handled by isRCTag + "", + } { + assert.False(t, isBuildReleaseBranch(ref), "must not match: %q", ref) + } } diff --git a/server/e2e_tests.go b/server/e2e_tests.go index c495a80..b780ac0 100644 --- a/server/e2e_tests.go +++ b/server/e2e_tests.go @@ -9,6 +9,8 @@ import ( "fmt" "net/url" "os" + "sort" + "strconv" "strings" "sync" "time" @@ -34,9 +36,7 @@ type E2EInstance struct { ServerVersion string `json:"server_version"` } -// e2eUniqueSuffix returns an 8-character random hex suffix for instance name uniqueness. -// Uses cloudModel.NewID (crypto/rand-based UUID) truncated to 8 chars so that -// concurrent calls always produce distinct values regardless of clock resolution. +// e2eUniqueSuffix returns an 8-char random hex suffix for unique instance names. func e2eUniqueSuffix() string { return cloudModel.NewID()[:8] } @@ -96,18 +96,25 @@ func (s *Server) handleE2ETestRequest(pr *model.PullRequest, label string) { key := fmt.Sprintf("%s-pr-%d", pr.RepoName, pr.Number) - // Snapshot the cleanup generation before provisioning begins. If handleE2ECleanup - // fires while the ~30 min creation is in flight, it increments this counter. - // We re-check below before storing instances so we never write stale entries. + // Snapshot cleanup generation before provisioning; re-checked before storing to prevent stale writes after a concurrent reset. s.e2ePRCleanupGenerationLock.Lock() startGeneration := s.e2ePRCleanupGeneration[key] s.e2ePRCleanupGenerationLock.Unlock() - // Guard against duplicate webhook deliveries. The in-progress key includes - // the test platform so that a second mobile label with a *different* platform - // (e.g. E2E/Run-Android while E2E/Run-iOS is provisioning) is not incorrectly - // dropped — it will reuse the in-flight instances once they are stored, or - // create its own if they are not yet available. + // storeIfCurrent atomically writes instances, but only if cleanup hasn't advanced since provisioning started. + storeIfCurrent := func(toStore []*E2EInstance) bool { + s.e2ePRCleanupGenerationLock.Lock() + defer s.e2ePRCleanupGenerationLock.Unlock() + s.e2eInstancesLock.Lock() + defer s.e2eInstancesLock.Unlock() + if s.e2ePRCleanupGeneration[key] != startGeneration { + return false + } + s.e2eInstances[key] = toStore + return true + } + + // Guard against duplicate webhook deliveries. Key includes platform so E2E/Run-Android and E2E/Run-iOS run independently. inProgressKey := fmt.Sprintf("%s-%s", key, testPlatform) s.e2eInProgressLock.Lock() if s.e2eInProgress[inProgressKey] { @@ -144,9 +151,11 @@ func (s *Server) handleE2ETestRequest(pr *model.PullRequest, label string) { logger.WithField("instances", len(cloudInstances)).Info("Reusing existing cloud E2E instances") s.cancelPRWorkflowRuns(pr, logger) s.wakeUpHibernatingInstances(cloudInstances, logger) - s.e2eInstancesLock.Lock() - s.e2eInstances[key] = cloudInstances - s.e2eInstancesLock.Unlock() + if !storeIfCurrent(cloudInstances) { + logger.Warn("E2E reset was requested during cloud-reuse path; discarding reused instances") + s.destroyE2EInstances(cloudInstances, logger) + return + } if err := s.triggerE2EWorkflow(pr, cloudInstances, instanceType, testPlatform); err != nil { logger.WithError(err).Error("Failed to trigger E2E workflow with cloud instances") s.postE2EErrorComment(pr, fmt.Sprintf("Failed to trigger E2E workflow: %v", err)) @@ -168,9 +177,7 @@ func (s *Server) handleE2ETestRequest(pr *model.PullRequest, label string) { return } - // Instance creation takes ~30 min. Check if the PR was closed during that window. - // If so, destroy the freshly created instances — no further cleanup events will fire - // for a closed PR, so storing them would leak them permanently. + // Check if PR closed during provisioning (~30 min) — cleanup events don't fire for closed PRs. prInfo, _, prErr := newGithubClient(s.Config.GithubAccessToken).PullRequests.Get( context.Background(), pr.RepoOwner, pr.RepoName, pr.Number) if prErr != nil { @@ -181,23 +188,12 @@ func (s *Server) handleE2ETestRequest(pr *model.PullRequest, label string) { return } - // Check whether E2EResetServersLabel was applied while provisioning was in flight. - // If the cleanup generation advanced, handleE2ECleanup already deleted the cloud - // installations; storing them here would put stale, deleted instances into the - // tracking map and dispatch a workflow against non-existent servers. - s.e2ePRCleanupGenerationLock.Lock() - resetDuringProvisioning := s.e2ePRCleanupGeneration[key] != startGeneration - s.e2ePRCleanupGenerationLock.Unlock() - if resetDuringProvisioning { + if !storeIfCurrent(instances) { logger.Warn("E2E reset was requested during provisioning; discarding freshly created instances") s.destroyE2EInstances(instances, logger) return } - s.e2eInstancesLock.Lock() - s.e2eInstances[key] = instances - s.e2eInstancesLock.Unlock() - logger.WithField("instances", len(instances)).Info("Successfully created E2E instances") if err = s.triggerE2EWorkflow(pr, instances, instanceType, testPlatform); err != nil { @@ -214,9 +210,7 @@ func (s *Server) handleE2ETestRequest(pr *model.PullRequest, label string) { logger.Info("Successfully triggered E2E workflow") } -// createMultipleE2EInstances creates all platform instances in parallel. -// Results are returned in the same order as platforms[] so that callers can rely on -// index-based platform assignment (e.g. instances[0] = site-1 for mobile). +// createMultipleE2EInstances creates instances in parallel; results are in platforms[] order for stable index assignment. func (s *Server) createMultipleE2EInstances(pr *model.PullRequest, instanceType string, platforms []string) ([]*E2EInstance, error) { if len(platforms) == 0 { return nil, fmt.Errorf("no platforms specified") @@ -294,9 +288,7 @@ func (s *Server) createMultipleE2EInstances(pr *model.PullRequest, instanceType return instances, nil } -// createCloudInstallation creates a single installation via provisioner API. -// ctx is used to cancel the polling wait so that parallel callers can abort early when a -// sibling goroutine fails, instead of waiting up to 30 minutes per polling interval. +// 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 { return nil, fmt.Errorf("installation creation cancelled before request: %w", err) @@ -304,19 +296,19 @@ func (s *Server) createCloudInstallation(ctx context.Context, name, version, use // Create installation request envVars := cloudModel.EnvVarMap{ - "MM_SERVICESETTINGS_ENABLETUTORIAL": cloudModel.EnvVar{Value: "false"}, - "MM_SERVICESETTINGS_ENABLEONBOARDINGFLOW": cloudModel.EnvVar{Value: "false"}, - "MM_SERVICESETTINGS_ENABLEUSERTYPINGMESSAGES": cloudModel.EnvVar{Value: "false"}, - "MM_SERVICESETTINGS_SESSIONLENGTHMOBILEINHOURS": cloudModel.EnvVar{Value: "5000"}, - "MM_SERVICESETTINGS_SESSIONCACHEINMINUTES": cloudModel.EnvVar{Value: "180"}, - "MM_SERVICEENVIRONMENT": cloudModel.EnvVar{Value: "test"}, - "MM_RATELIMITSETTINGS_ENABLE": cloudModel.EnvVar{Value: "true"}, - "MM_RATELIMITSETTINGS_PERSEC": cloudModel.EnvVar{Value: "3000"}, - "MM_RATELIMITSETTINGS_MAXBURST": cloudModel.EnvVar{Value: "5000"}, - "MM_RATELIMITSETTINGS_MEMORYSTORESIZE": cloudModel.EnvVar{Value: "10000"}, - "MM_RATELIMITSETTINGS_VARYBYREMOTEADDR": cloudModel.EnvVar{Value: "false"}, - "MM_RATELIMITSETTINGS_VARYBYUSER": cloudModel.EnvVar{Value: "false"}, - "MM_TEAMSETTINGS_EXPERIMENTALENABLEAUTOMATICREPLIES": cloudModel.EnvVar{Value: "true"}, + "MM_SERVICESETTINGS_ENABLETUTORIAL": cloudModel.EnvVar{Value: "false"}, + "MM_SERVICESETTINGS_ENABLEONBOARDINGFLOW": cloudModel.EnvVar{Value: "false"}, + "MM_SERVICESETTINGS_ENABLEUSERTYPINGMESSAGES": cloudModel.EnvVar{Value: "false"}, + "MM_SERVICESETTINGS_SESSIONLENGTHMOBILEINHOURS": cloudModel.EnvVar{Value: "5000"}, + "MM_SERVICESETTINGS_SESSIONCACHEINMINUTES": cloudModel.EnvVar{Value: "180"}, + "MM_SERVICEENVIRONMENT": cloudModel.EnvVar{Value: "test"}, + "MM_RATELIMITSETTINGS_ENABLE": cloudModel.EnvVar{Value: "true"}, + "MM_RATELIMITSETTINGS_PERSEC": cloudModel.EnvVar{Value: "3000"}, + "MM_RATELIMITSETTINGS_MAXBURST": cloudModel.EnvVar{Value: "5000"}, + "MM_RATELIMITSETTINGS_MEMORYSTORESIZE": cloudModel.EnvVar{Value: "10000"}, + "MM_RATELIMITSETTINGS_VARYBYREMOTEADDR": cloudModel.EnvVar{Value: "false"}, + "MM_RATELIMITSETTINGS_VARYBYUSER": cloudModel.EnvVar{Value: "false"}, + "MM_TEAMSETTINGS_EXPERIMENTALENABLEAUTOMATICREPLIES": cloudModel.EnvVar{Value: "true"}, } installationRequest := &cloudModel.CreateInstallationRequest{ @@ -341,10 +333,6 @@ func (s *Server) createCloudInstallation(ctx context.Context, name, version, use return nil, fmt.Errorf("failed to create installation: %w", err) } - // cleanupCreatedInstallation is a best-effort cleanup helper used on all failure paths after - // CreateInstallation succeeds. Without it, the cloud installation would be permanently - // orphaned because it has not yet been added to the in-memory tracking map. - // It deletes the installation, logs any deletion error, and returns cause unchanged. 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") @@ -701,14 +689,31 @@ func (s *Server) e2eInstanceMaxAge() time.Duration { return 3 * time.Hour } -// PR instances (identified by "-pr-" in their OwnerID) are always skipped — handleE2ECleanup -// on PR close manages their lifecycle via cloud-API orphan scan. -func (s *Server) cleanupStaleNonPRE2EInstances() { - maxAge := s.e2eInstanceMaxAge() +// e2ePRInstanceMaxAge returns the maximum age a PR E2E instance may reach before the periodic +// scan deletes it. PR instances are reused across label toggles and commits, so this is much +// longer than e2eInstanceMaxAge. Falls back to 24 hours when the config value is 0 (unset). +func (s *Server) e2ePRInstanceMaxAge() time.Duration { + if s.Config.E2EPRInstanceMaxAge > 0 { + return time.Duration(s.Config.E2EPRInstanceMaxAge) * time.Hour + } + return 24 * 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). +func (s *Server) cleanupStaleE2EInstances() { + nonPRMaxAge := s.e2eInstanceMaxAge() + prMaxAge := s.e2ePRInstanceMaxAge() logger := s.Logger.WithField("type", "periodic_e2e_cleanup") - logger.WithField("max_age_hours", maxAge.Hours()).Info("Scanning for stale non-PR E2E instances") + logger.WithFields(logrus.Fields{ + "non_pr_max_age_hours": nonPRMaxAge.Hours(), + "pr_max_age_hours": prMaxAge.Hours(), + }).Info("Scanning for stale E2E instances") + + now := time.Now() + nonPRCutoffMs := now.Add(-nonPRMaxAge).UnixMilli() + prCutoffMs := now.Add(-prMaxAge).UnixMilli() - cutoffMs := time.Now().Add(-maxAge).UnixMilli() + var reapedPRInstallationIDs []string for _, instanceType := range []string{"desktop", "mobile"} { pattern := instanceType + "-%" @@ -729,18 +734,22 @@ func (s *Server) cleanupStaleNonPRE2EInstances() { continue } - // PR instances have "-pr-" in their OwnerID (e.g. "mobile-pr-123-site-1-..."). - // Skip them — handleE2ECleanup on PR close manages their lifecycle. - if strings.Contains(inst.OwnerID, "-pr-") { - continue + // PR instances have "-pr-" in their OwnerID (e.g. "mobile-pr-123-site-1-...") + // and use the longer PR max-age; everything else uses the non-PR max-age. + isPR := strings.Contains(inst.OwnerID, "-pr-") + cutoffMs := nonPRCutoffMs + if isPR { + cutoffMs = prCutoffMs } - // Skip instances younger than maxAge — a test may still be using them. + // Skip instances younger than their max age — a test may still be using them, + // or (for PRs) the servers are being kept alive for reuse. if inst.CreateAt > cutoffMs { logger.WithFields(logrus.Fields{ "installation_id": inst.ID, "owner_id": inst.OwnerID, - }).Debug("Skipping non-PR instance younger than max age (may still be in use)") + "is_pr": isPR, + }).Debug("Skipping instance younger than its max age") continue } @@ -758,15 +767,271 @@ func (s *Server) cleanupStaleNonPRE2EInstances() { "installation_id": inst.ID, "owner_id": inst.OwnerID, "state": inst.State, + "is_pr": isPR, }) - instLogger.Warn("Destroying stale non-PR E2E instance") + instLogger.Warn("Destroying stale E2E instance") if err := s.CloudClient.DeleteInstallation(inst.ID); err != nil { - instLogger.WithError(err).Error("Failed to destroy stale non-PR E2E instance") + instLogger.WithError(err).Error("Failed to destroy stale E2E instance") + continue + } + if isPR { + reapedPRInstallationIDs = append(reapedPRInstallationIDs, inst.ID) + } + } + } + + // Evict in-memory PR tracking entries whose servers were just reaped so the reuse path + // in handleE2ETestRequest sees no live instances and creates a fresh set on the next + // E2E/Run, instead of dispatching a workflow against deleted servers. + if len(reapedPRInstallationIDs) > 0 { + s.evictReapedPRInstances(reapedPRInstallationIDs, logger) + } + + logger.Info("E2E instance cleanup scan complete") +} + +// evictReapedPRInstances removes PR tracking entries when any member was reaped, so the reuse path never returns a partially-deleted set. +func (s *Server) evictReapedPRInstances(reapedInstallationIDs []string, logger logrus.FieldLogger) { + reaped := make(map[string]bool, len(reapedInstallationIDs)) + for _, id := range reapedInstallationIDs { + reaped[id] = true + } + + s.e2eInstancesLock.Lock() + defer s.e2eInstancesLock.Unlock() + for key, instances := range s.e2eInstances { + // PR tracking keys are "{repo}-pr-{number}"; non-PR keys are keyed by SHA elsewhere. + if !strings.Contains(key, "-pr-") { + continue + } + for _, inst := range instances { + if inst != nil && reaped[inst.InstallationID] { + delete(s.e2eInstances, key) + logger.WithField("key", key).Info("Evicted expired PR E2E instances from tracking map") + break + } + } + } +} + +// resolveBranchHeadSHA returns the branch HEAD SHA at dispatch time. Non-PR flows dispatch with ref=branch, so the run's head_sha may differ from the trigger SHA if the branch advanced during provisioning. +func (s *Server) resolveBranchHeadSHA(owner, repoName, branch string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + client := newGithubClient(s.Config.GithubAccessToken) + if s.githubAPIBase != "" { + if baseURL, parseErr := url.Parse(s.githubAPIBase); parseErr == nil { + client.BaseURL = baseURL + } + } + + var commit struct { + SHA string `json:"sha"` + } + req, err := client.NewRequest("GET", fmt.Sprintf("/repos/%s/%s/commits/%s", owner, repoName, branch), nil) + if err != nil { + return "", err + } + if _, err := client.Do(ctx, req, &commit); err != nil { + return "", err + } + if commit.SHA == "" { + return "", fmt.Errorf("empty SHA for %s/%s@%s", owner, repoName, branch) + } + return commit.SHA, nil +} + +// cmtVersion is a parsed Mattermost release version: major.minor.patch with an optional +// release-candidate number. raw is the bare-semver string passed to the cloud provisioner +// (e.g. "11.7.1" or "11.8.0-rc3"). +type cmtVersion struct { + major, minor, patch int + rc int // 0 = stable, >0 = -rcN + raw string +} + +// parseCMTVersion parses "vX.Y.Z" or "vX.Y.Z-rcN" (the leading "v" is optional). It returns +// ok=false for anything else (other prerelease suffixes like -beta/-alpha are ignored for CMT). +func parseCMTVersion(tag string) (cmtVersion, bool) { + raw := strings.TrimPrefix(strings.TrimSpace(tag), "v") + base := raw + rc := 0 + if i := strings.Index(base, "-rc"); i != -1 { + n, err := strconv.Atoi(base[i+len("-rc"):]) + if err != nil { + return cmtVersion{}, false + } + rc = n + base = base[:i] + } else if strings.Contains(base, "-") { + return cmtVersion{}, false + } + parts := strings.Split(base, ".") + if len(parts) != 3 { + return cmtVersion{}, false + } + maj, err1 := strconv.Atoi(parts[0]) + min, err2 := strconv.Atoi(parts[1]) + pat, err3 := strconv.Atoi(parts[2]) + if err1 != nil || err2 != nil || err3 != nil { + return cmtVersion{}, false + } + return cmtVersion{major: maj, minor: min, patch: pat, rc: rc, raw: raw}, true +} + +// less reports whether a sorts before b by (major, minor, patch, rc). For the same X.Y.Z, a +// stable release (rc==0) sorts above its release candidates (e.g. 11.8.0-rc3 < 11.8.0). +func (a cmtVersion) less(b cmtVersion) bool { + if a.major != b.major { + return a.major < b.major + } + if a.minor != b.minor { + return a.minor < b.minor + } + if a.patch != b.patch { + return a.patch < b.patch + } + ar, br := a.rc, b.rc + if ar == 0 { + ar = int(^uint(0) >> 1) // treat stable as the highest "rc" for the same patch + } + if br == 0 { + br = int(^uint(0) >> 1) + } + return ar < br +} + +// 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 the Mattermost GitHub releases. +func (s *Server) cmtServerVersions() []string { + if len(s.Config.CMTServerVersions) > 0 { + return s.Config.CMTServerVersions + } + return s.resolveCMTServerVersions() +} + +// resolveCMTServerVersions fetches Mattermost releases and picks: all active ESR lines + latest 3 stable minors + current RC, one patch per line. Falls back to defaultCMTServerVersions on error. +func (s *Server) resolveCMTServerVersions() []string { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + client := newGithubClient(s.Config.GithubAccessToken) + // githubAPIBase is only set in tests to redirect to a mock server. + if s.githubAPIBase != "" { + if baseURL, parseErr := url.Parse(s.githubAPIBase); parseErr == nil { + client.BaseURL = baseURL + } + } + + var releases []struct { + TagName string `json:"tag_name"` + Draft bool `json:"draft"` + Prerelease bool `json:"prerelease"` + Body string `json:"body"` + } + const perPage = 100 + 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 + } + var pageReleases []struct { + TagName string `json:"tag_name"` + Draft bool `json:"draft"` + Prerelease bool `json:"prerelease"` + 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 + } + releases = append(releases, pageReleases...) + if len(pageReleases) < perPage { + break + } + } + + type minorKey struct{ major, minor int } + latestStable := map[minorKey]cmtVersion{} + esrMinors := map[minorKey]bool{} + var bestRC cmtVersion + haveRC := false + + for _, r := range releases { + if r.Draft { + continue + } + v, ok := parseCMTVersion(r.TagName) + if !ok { + continue + } + key := minorKey{v.major, v.minor} + if v.rc > 0 { + if !haveRC || bestRC.less(v) { + bestRC = v + haveRC = true } + continue + } + if cur, exists := latestStable[key]; !exists || cur.less(v) { + latestStable[key] = v + } + if strings.Contains(strings.ToLower(r.Body), "extended support release") { + esrMinors[key] = true } } - logger.Info("Non-PR E2E instance cleanup scan complete") + 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[minorKey]cmtVersion{} + for i := 0; i < len(minors) && i < 3; i++ { // latest 3 stable minor lines + selected[minorKey{minors[i].major, minors[i].minor}] = minors[i] + } + for k := range esrMinors { // active ESR line(s) + if v, ok := latestStable[k]; ok { + selected[k] = v + } + } + + 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 5 versions to bound provisioning cost and matrix wall-clock: latest RC + // (when present) + up to 4 previous lines. ESR-aware selection above may pick + // more if a release window has multiple active ESRs; in that case we keep the + // newest 5 and drop the oldest entries (typically the older ESR line) — surfaces + // in the [resolveCMTServerVersions] log line for the operator. + const maxVersions = 5 + if len(chosen) > maxVersions { + chosen = chosen[len(chosen)-maxVersions:] // keep the newest if over the cap + } + + 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 } // destroyE2EInstances destroys all given E2E instances @@ -970,11 +1235,8 @@ func (s *Server) buildInstanceDetailsJSON(instances []*E2EInstance) (string, err return string(jsonBytes), nil } -// dispatchDesktopE2EWorkflow triggers the desktop E2E workflow via GitHub Actions API. -// trackingKey is the s.e2eInstances map key for this run; when non-empty it is passed -// as the "mw_tracking_key" workflow input so the workflow_run completed handler can do -// a direct key lookup instead of fragile SHA suffix matching. -func (s *Server) dispatchDesktopE2EWorkflow(repoOwner, repoName, ref, sha, instanceDetailsJSON, runType, trackingKey string, nightly bool) error { +// dispatchDesktopE2EWorkflow triggers e2e-functional.yml. No tracking key in inputs — GitHub rejects undeclared workflow_dispatch inputs with 422. +func (s *Server) dispatchDesktopE2EWorkflow(repoOwner, repoName, ref, sha, instanceDetailsJSON, runType string) error { ctx := context.Background() client := newGithubClient(s.Config.GithubAccessToken) @@ -1005,10 +1267,6 @@ func (s *Server) dispatchDesktopE2EWorkflow(repoOwner, repoName, ref, sha, insta "MM_TEST_USER_NAME": s.Config.E2EUsername, "MM_SERVER_VERSION": serverVersion, "run_type": runType, - "nightly": fmt.Sprintf("%t", nightly), - } - if trackingKey != "" { - workflowInputs["mw_tracking_key"] = trackingKey } // Use REST API to trigger workflow dispatch (v32 go-github compatibility) @@ -1038,11 +1296,8 @@ func (s *Server) dispatchDesktopE2EWorkflow(repoOwner, repoName, ref, sha, insta return nil } -// dispatchMobileE2EWorkflow triggers the mobile E2E workflow via GitHub Actions API. -// trackingKey is the s.e2eInstances map key for this run; when non-empty it is passed -// as the "mw_tracking_key" workflow input so the workflow_run completed handler can do -// a direct key lookup instead of fragile SHA suffix matching. -func (s *Server) dispatchMobileE2EWorkflow(repoOwner, repoName, ref, sha, site1URL, site2URL, site3URL, platform, runType, trackingKey string) error { +// dispatchMobileE2EWorkflow triggers e2e-detox-pr.yml. No tracking key in inputs — GitHub rejects undeclared workflow_dispatch inputs with 422. +func (s *Server) dispatchMobileE2EWorkflow(repoOwner, repoName, ref, sha, site1URL, site2URL, site3URL, platform, runType string) error { ctx := context.Background() client := newGithubClient(s.Config.GithubAccessToken) @@ -1060,9 +1315,6 @@ func (s *Server) dispatchMobileE2EWorkflow(repoOwner, repoName, ref, sha, site1U "PLATFORM": platform, "run_type": runType, } - if trackingKey != "" { - workflowInputs["mw_tracking_key"] = trackingKey - } // Use REST API to trigger workflow dispatch (v32 go-github compatibility) req, err := client.NewRequest("POST", diff --git a/server/push_events.go b/server/push_events.go index 816a21a..f3e1410 100644 --- a/server/push_events.go +++ b/server/push_events.go @@ -39,11 +39,7 @@ func (s *Server) handlePushEvent(event *github.PushEvent) { }) logger.Info("Push event received") - if s.Config.E2EAutoTriggerOnRelease && s.isReleaseBranch(branch) { - logger.WithField("type", "release_branch").Info("Release branch detected, triggering E2E tests") - go s.handlePushEventE2E(event, branch) - return - } + // Release-branch push trigger was removed; release stabilization is covered by PR-label E2E and CMT. if s.Config.E2EAutoTriggerOnMaster && (branch == "master" || branch == "main") { logger.WithField("type", "master_main").Info("Master/main branch detected, triggering E2E tests") @@ -51,12 +47,8 @@ func (s *Server) handlePushEvent(event *github.PushEvent) { return } - logger.WithFields(logrus.Fields{ - "auto_release": s.Config.E2EAutoTriggerOnRelease, - "auto_master": s.Config.E2EAutoTriggerOnMaster, - "release_pattern_prefix": s.Config.E2EReleasePatternPrefix, - "is_release_branch": s.isReleaseBranch(branch), - }).Info("Push event does not match E2E trigger conditions") + logger.WithField("auto_master", s.Config.E2EAutoTriggerOnMaster). + Info("Push event does not match E2E trigger conditions") } // isReleaseBranch returns true if branch matches E2EReleasePatternPrefix. @@ -130,14 +122,26 @@ func (s *Server) handlePushEventE2E(event *github.PushEvent, branch string) { logger.WithField("instanceCount", len(instances)).Info("E2E instances created successfully") + // Key on the branch HEAD resolved now (just before dispatch), not the push SHA: the + // dispatched (ref=branch) run reports its head_sha as the branch HEAD at dispatch time, + // which can differ from the push SHA if the branch advanced during provisioning. The key + // still ends with "-{sha}" so findAndDestroyInstancesBySHA matches it by suffix on + // completion. Fall back to the push SHA on error (the periodic scan remains the backstop). + cleanupSHA := sha + if resolved, resErr := s.resolveBranchHeadSHA(s.Config.Org, repoName, branch); resErr == nil && resolved != "" { + cleanupSHA = resolved + } else if resErr != nil { + logger.WithError(resErr).Warn("Failed to resolve branch HEAD SHA; keying cleanup on push SHA (periodic scan remains the backstop)") + } + // Store instances before dispatching so a fast-completing workflow_run event // doesn't race ahead and find nothing to clean up. - key := fmt.Sprintf("%s-push-%s-%s", repoName, branch, sha) + key := fmt.Sprintf("%s-push-%s-%s", repoName, branch, cleanupSHA) s.e2eInstancesLock.Lock() s.e2eInstances[key] = instances s.e2eInstancesLock.Unlock() - err = s.triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, sha, key, instances) + err = s.triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, sha, instances) if err != nil { logger.WithError(err).Error("Failed to trigger E2E workflow") s.e2eInstancesLock.Lock() @@ -228,10 +232,7 @@ func (s *Server) createMultipleE2EInstancesForPushEvent(repoName, instanceType, return instances, nil } -// getRunnerForPlatform returns the GitHub Actions runner label for E2E functional -// workflows (PR label + push events). CMT workflows use a separate hardcoded matrix -// in buildDesktopCMTMatrixJSON (macos-13) because compatibility testing pins a -// specific OS version; functional tests track latest. +// getRunnerForPlatform returns the runner label for E2E functional tests. CMT uses a separate hardcoded matrix with pinned OS versions. func getRunnerForPlatform(platform string) string { switch strings.ToLower(platform) { case "linux": @@ -246,8 +247,8 @@ func getRunnerForPlatform(platform string) string { } // triggerE2EWorkflowForPushEvent routes to the desktop or mobile dispatch function. -// trackingKey is embedded in workflow inputs as mw_tracking_key for cleanup on completion. -func (s *Server) triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, sha, trackingKey string, instances []*E2EInstance) error { +// Cleanup is driven by the workflow_run completed event matched on the commit SHA. +func (s *Server) triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, sha string, instances []*E2EInstance) error { logger := s.Logger.WithFields(logrus.Fields{ "repo": repoName, "instanceType": instanceType, @@ -262,14 +263,14 @@ func (s *Server) triggerE2EWorkflowForPushEvent(repoName, instanceType, branch, } if instanceType == "desktop" { - return s.triggerDesktopE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, trackingKey, instances) + return s.triggerDesktopE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, instances) } - return s.triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, trackingKey, instances) + return s.triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, instances) } // triggerDesktopE2EWorkflowForPushEvent dispatches the desktop E2E workflow. -func (s *Server) triggerDesktopE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, trackingKey string, instances []*E2EInstance) error { +func (s *Server) triggerDesktopE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha string, instances []*E2EInstance) error { logger := s.Logger.WithFields(logrus.Fields{ "repo": repoName, "branch": branch, @@ -283,16 +284,12 @@ func (s *Server) triggerDesktopE2EWorkflowForPushEvent(repoOwner, repoName, bran logger.WithField("instanceDetails", instanceDetailsJSON).Debug("Triggering desktop E2E workflow") - runType := "MASTER" - if s.isReleaseBranch(branch) { - runType = "RELEASE" - } - - return s.dispatchDesktopE2EWorkflow(repoOwner, repoName, branch, sha, instanceDetailsJSON, runType, trackingKey, false) + // runType is always MASTER — only master/main pushes reach this path. + return s.dispatchDesktopE2EWorkflow(repoOwner, repoName, branch, sha, instanceDetailsJSON, "MASTER") } // triggerMobileE2EWorkflowForPushEvent dispatches the mobile E2E workflow (e2e-detox-pr.yml). -func (s *Server) triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha, trackingKey string, instances []*E2EInstance) error { +func (s *Server) triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branch, sha string, instances []*E2EInstance) error { logger := s.Logger.WithFields(logrus.Fields{ "repo": repoName, "branch": branch, @@ -309,15 +306,12 @@ func (s *Server) triggerMobileE2EWorkflowForPushEvent(repoOwner, repoName, branc "site_3_url": instances[2].URL, }).Debug("Triggering mobile E2E workflow for push event") - runType := "MASTER" - if s.isReleaseBranch(branch) { - runType = "RELEASE" - } - + // handlePushEvent only routes master/main pushes here (release-branch push trigger was + // removed), so runType is always MASTER for mobile push events. return s.dispatchMobileE2EWorkflow( repoOwner, repoName, branch, sha, instances[0].URL, instances[1].URL, instances[2].URL, "both", // push events always test both iOS and Android - runType, trackingKey, + "MASTER", ) } diff --git a/server/server.go b/server/server.go index f1e5cdf..3e37444 100644 --- a/server/server.go +++ b/server/server.go @@ -42,41 +42,30 @@ type Server struct { envMaps map[string]cloudModel.EnvVarMap envMapsLock sync.Mutex - // e2eInstances tracks E2E test instances for cleanup. - // Key formats: "%s-pr-%d" (PR), "%s-push-%s-%s" (push, ends with SHA), - // "%s-scheduled-%s" (nightly, ends with SHA), "%s-cmt-%d-%s" (CMT, ends with SHA). + // e2eInstances tracks E2E instances by key: "{repo}-pr-{n}" | "{repo}-push-{branch}-{sha}" | "{repo}-cmt-{runID}" e2eInstances map[string][]*E2EInstance e2eInstancesLock sync.Mutex - // e2eInProgress guards against concurrent handleE2ETestRequest executions for the - // same PR+platform key (e.g. duplicate webhook deliveries). Only one goroutine per - // key may run the check-and-create flow at a time; a second arrival while the first - // is still running is silently dropped. + // cmtDispatchLocks serializes dispatch+poll+store per repo to prevent run-id collisions. + cmtDispatchLocks map[string]*sync.Mutex + cmtDispatchLocksMu sync.Mutex + + // e2eInProgress prevents duplicate provisioning for the same PR+platform (duplicate webhooks). e2eInProgress map[string]bool e2eInProgressLock sync.Mutex - // e2ePRCleanupGeneration tracks how many times handleE2ECleanup has run for - // each PR key. handleE2ETestRequest captures the counter before provisioning - // and aborts if it has changed when provisioning completes, preventing stale - // instances from being stored after a concurrent reset. + // e2ePRCleanupGeneration is incremented on each cleanup; provisioning aborts if it advances during the ~30-min create window. e2ePRCleanupGeneration map[string]int64 e2ePRCleanupGenerationLock sync.Mutex - // stopCh is closed by Stop() to signal long-running background goroutines - // (e.g. the periodic E2E cleanup ticker) to exit cleanly. + // stopCh is closed by Stop() to terminate background goroutines. stopCh chan struct{} stopOnce sync.Once - // githubAPIBase overrides the GitHub API base URL (e.g. "https://api.github.com/"). - // When non-empty (tests only), GitHub clients created inside this server will be - // redirected to this URL instead of the real GitHub API. + // githubAPIBase redirects GitHub API calls to a mock URL in tests (empty = use real GitHub). githubAPIBase string - // e2eVersionCache holds the last successfully resolved "latest" server version so - // that back-to-back E2E provisioning requests (e.g. three parallel platform - // instances) share one GitHub API round-trip instead of each making their own. - // The cache is intentionally short-lived: new stable releases ship at most once a - // month, so a 1-hour TTL gives a good hit rate without risking stale data. + // e2eVersionCache holds the resolved "latest" version (1-hour TTL) to avoid redundant GitHub API calls. e2eVersionCache string e2eVersionCacheTime time.Time e2eVersionCacheLock sync.Mutex @@ -100,16 +89,17 @@ func New(config *MatterwickConfig) *Server { cloudClient := model.NewCloudClient(config.ProvisionerServer, config.CloudAuth.ClientID, config.CloudAuth.ClientSecret, config.CloudAuth.TokenEndpoint, config.AWSAPIKey) s := &Server{ - Config: config, - Router: mux.NewRouter(), - webhookChannels: make(map[string]chan cloudModel.WebhookPayload), - StartTime: time.Now(), - Logger: logger.WithField("instance", cloudModel.NewID()), - CloudClient: cloudClient, + Config: config, + Router: mux.NewRouter(), + webhookChannels: make(map[string]chan cloudModel.WebhookPayload), + StartTime: time.Now(), + Logger: logger.WithField("instance", cloudModel.NewID()), + CloudClient: cloudClient, envMaps: make(map[string]cloudModel.EnvVarMap), e2eInstances: make(map[string][]*E2EInstance), e2eInProgress: make(map[string]bool), e2ePRCleanupGeneration: make(map[string]int64), + cmtDispatchLocks: make(map[string]*sync.Mutex), stopCh: make(chan struct{}), } @@ -134,12 +124,8 @@ func New(config *MatterwickConfig) *Server { func (s *Server) Start() { s.Logger.Info("Starting MatterWick Server") - // Destroy stale non-PR E2E instances left from a previous run immediately on startup, - // then continue scanning periodically so a mid-run restart doesn't leave orphaned - // instances alive until the *next* matterwick restart. - // The scan interval is half the configured max-age so the worst-case orphan lifetime - // is maxAge + interval ≈ 1.5× maxAge. - s.cleanupStaleNonPRE2EInstances() + // Clean up stale instances from any previous run immediately, then scan periodically. + s.cleanupStaleE2EInstances() go func() { interval := s.e2eInstanceMaxAge() / 2 if interval < 30*time.Minute { @@ -150,7 +136,7 @@ func (s *Server) Start() { for { select { case <-ticker.C: - s.cleanupStaleNonPRE2EInstances() + s.cleanupStaleE2EInstances() case <-s.stopCh: return } diff --git a/server/version.go b/server/version.go index 7be6cb9..e4b56a7 100644 --- a/server/version.go +++ b/server/version.go @@ -89,8 +89,11 @@ func (s *Server) resolveMattermostServerVersion() string { if parseErr != nil { continue } - if len(v.Pre) > 0 && (v.Pre[0].VersionStr == "alpha" || v.Pre[0].VersionStr == "beta") { - continue + if len(v.Pre) > 0 { + pre := v.Pre[0].VersionStr + if strings.HasPrefix(pre, "alpha") || strings.HasPrefix(pre, "beta") { + continue + } } candidates = append(candidates, candidate{tag: raw, ver: v}) } diff --git a/server/workflow_run.go b/server/workflow_run.go index 610788f..ddf3e30 100644 --- a/server/workflow_run.go +++ b/server/workflow_run.go @@ -8,13 +8,16 @@ import ( "encoding/json" "fmt" "io" + "net/url" + "regexp" "strings" "sync" + "time" "github.com/sirupsen/logrus" ) -// WorkflowRunWebhookPayload represents the workflow_run webhook payload with inputs +// WorkflowRunWebhookPayload is the parsed body of a workflow_run webhook event. type WorkflowRunWebhookPayload struct { Action string `json:"action"` WorkflowRun WorkflowRunWithInputs `json:"workflow_run"` @@ -22,7 +25,7 @@ type WorkflowRunWebhookPayload struct { Workflow map[string]interface{} `json:"workflow"` } -// WorkflowRunWithInputs extends WorkflowRun with inputs field +// WorkflowRunWithInputs is the workflow_run object extended with the workflow_dispatch inputs field. type WorkflowRunWithInputs struct { ID int64 `json:"id"` Name string `json:"name"` @@ -32,7 +35,7 @@ type WorkflowRunWithInputs struct { Inputs map[string]string `json:"inputs"` } -// ParseWorkflowRunEventWithInputs parses workflow_run event and extracts inputs +// ParseWorkflowRunEventWithInputs decodes a workflow_run webhook payload from r. func ParseWorkflowRunEventWithInputs(data io.Reader) (*WorkflowRunWebhookPayload, error) { decoder := json.NewDecoder(data) var payload WorkflowRunWebhookPayload @@ -43,7 +46,7 @@ func ParseWorkflowRunEventWithInputs(data io.Reader) (*WorkflowRunWebhookPayload return &payload, nil } -// handleWorkflowRunEventWithInputs routes workflow_run events to CMT, nightly, or cleanup handlers. +// handleWorkflowRunEventWithInputs routes workflow_run events to CMT or cleanup handlers. func (s *Server) handleWorkflowRunEventWithInputs(payload *WorkflowRunWebhookPayload) { // Extract repository info repoData := payload.Repository @@ -78,201 +81,160 @@ func (s *Server) handleWorkflowRunEventWithInputs(payload *WorkflowRunWebhookPay "head_sha": headSHA, }) - // CMT: "CMT Provisioner" (user-dispatched) provisions servers; "Compatibility Matrix Testing" runs tests. - if strings.Contains(workflowName, "cmt") || strings.Contains(workflowName, "CMT") { - if payload.Action == "completed" { - logger.Debug("CMT trigger workflow completed; sha-based cleanup is primary") - s.handleCMTRunCleanup(repoName, headSHA, logger) - return - } - if payload.Action != "requested" { - logger.Debug("Ignoring CMT workflow action (not requested or completed)") - return - } - logger.Info("Processing CMT workflow_run event") - serverVersionsStr, ok := payload.WorkflowRun.Inputs["server_versions"] - if !ok || serverVersionsStr == "" { - logger.Error("No server_versions found in workflow inputs") - return - } - serverVersions := parseServerVersionsFromString(serverVersionsStr) - if len(serverVersions) == 0 { - logger.Error("Failed to parse server versions from workflow input") - return - } - logger.WithField("serverVersions", serverVersions).Info("Extracted server versions from workflow inputs") - var instanceType string - if strings.Contains(repoName, "desktop") { - instanceType = "desktop" - } else if strings.Contains(repoName, "mobile") { - instanceType = "mobile" - } else { - logger.Warn("Repository is neither desktop nor mobile, skipping CMT") - return + // CMT trigger: provision one server per version in s.cmtServerVersions() and dispatch compatibility-matrix-testing.yml. + if s.Config.CMTTriggerWorkflowName != "" && workflowName == s.Config.CMTTriggerWorkflowName { + if payload.Action == "requested" { + triggerEvent := payload.WorkflowRun.Event + if s.shouldTriggerCMT(triggerEvent, headBranch) { + logger.WithFields(logrus.Fields{ + "trigger_event": triggerEvent, + "head_branch": headBranch, + }).Info("CMT trigger workflow started, provisioning E2E servers for configured versions") + go s.handleCMTTrigger(owner, repoName, headBranch, headSHA, runID, logger) + } else { + logger.WithFields(logrus.Fields{ + "trigger_event": triggerEvent, + "head_branch": headBranch, + }).Info("CMT trigger fired on non-RC-tag, non-release ref and not via manual dispatch; skipping") + } } - go s.handleCMTWithServerVersions(owner, repoName, instanceType, headBranch, headSHA, serverVersions, runID, logger) return } - // Nightly: lightweight trigger workflow fires first; matterwick provisions instances and dispatches the real test workflow. - if s.Config.E2ENightlyTriggerWorkflowName != "" && workflowName == s.Config.E2ENightlyTriggerWorkflowName { - if payload.Action == "requested" { - logger.Info("Nightly trigger workflow started, provisioning E2E servers") - go s.handleNightlyE2ETrigger(owner, repoName, headBranch, headSHA, payload.WorkflowRun.Event, runID, logger) + // On completion: CMT keys on run id, non-CMT flows key on SHA. + if payload.Action == "completed" && s.isE2ETestWorkflow(workflowName) { + if workflowName == s.cmtTestWorkflowName() { + logger.Info("CMT test workflow completed, cleaning up instances by run id") + s.findAndDestroyInstancesByRunID(repoName, runID, logger) + } else { + logger.Info("Test workflow completed, cleaning up matching instances by SHA") + s.findAndDestroyInstancesBySHA(repoName, headSHA, false, logger) } return } - // --- Test workflow completion: clean up provisioned instances --- - if payload.Action == "completed" && s.isE2ETestWorkflow(workflowName) { - logger.Info("Test workflow completed, checking for instance cleanup") - - // Primary: look up by mw_tracking_key embedded at dispatch time (immune to SHA races). - if trackingKey := payload.WorkflowRun.Inputs["mw_tracking_key"]; trackingKey != "" { - s.e2eInstancesLock.Lock() - instances := s.e2eInstances[trackingKey] - delete(s.e2eInstances, trackingKey) - s.e2eInstancesLock.Unlock() - if len(instances) > 0 { - logger.WithField("tracking_key", trackingKey).Info("Destroying instances by tracking key") - s.destroyE2EInstances(instances, logger) - } else { - logger.WithField("tracking_key", trackingKey).Debug("No in-memory instances for tracking key (matterwick restarted or already cleaned)") - } - return + logger.WithField("configured_test_workflows", s.Config.E2ETestWorkflowNames). + Info("Ignoring workflow_run event (not relevant to E2E lifecycle)") +} + +// isE2ETestWorkflow reports whether name is in Config.E2ETestWorkflowNames. +func (s *Server) isE2ETestWorkflow(name string) bool { + for _, n := range s.Config.E2ETestWorkflowNames { + if n == name { + return true } + } + return false +} - // Fallback: SHA-based scan (runs dispatched before mw_tracking_key was introduced). - logger.Debug("No mw_tracking_key in workflow inputs, falling back to SHA-based instance cleanup") - s.findAndDestroyInstancesBySHA(repoName, headSHA, logger) - return +// defaultCMTTestWorkflowName is the "name:" of compatibility-matrix-testing.yml in the +// desktop/mobile repos. Used when Config.CMTTestWorkflowName is empty so CMT cleanup (keyed +// by run id) never silently falls back to SHA cleanup, which can't match a -cmt-{runID} key. +const defaultCMTTestWorkflowName = "Compatibility Matrix Testing" + +// cmtTestWorkflowName returns the configured CMT test workflow name, or the default. +func (s *Server) cmtTestWorkflowName() string { + if s.Config.CMTTestWorkflowName != "" { + return s.Config.CMTTestWorkflowName } + return defaultCMTTestWorkflowName +} - logger.WithFields(logrus.Fields{ - "configured_nightly_name": s.Config.E2ENightlyTriggerWorkflowName, - "configured_test_workflows": s.Config.E2ETestWorkflowNames, - }).Info("Ignoring workflow_run event (not relevant to E2E lifecycle)") +func cmtInstanceKey(repoName string, testRunID int64) string { + return fmt.Sprintf("%s-cmt-%d", repoName, testRunID) } -// handleNightlyE2ETrigger provisions instances and dispatches the test workflow. -// Called when the E2E trigger workflow starts, whether from schedule, push to master/main, -// or push to a release branch. The triggerEvent parameter ("schedule", "push", etc.) is -// used to set runType correctly — scheduled runs always get "NIGHTLY" regardless of branch. -func (s *Server) handleNightlyE2ETrigger(owner, repoName, branch, sha, triggerEvent string, runID int64, logger logrus.FieldLogger) { - logger = logger.WithFields(logrus.Fields{ - "branch": branch, - "sha": sha, - "run_id": runID, - }) - logger.Info("Provisioning nightly E2E instances") +// cmtDispatchMutex returns a per-repo mutex for the dispatch+poll+store critical section. +func (s *Server) cmtDispatchMutex(repoName string) *sync.Mutex { + s.cmtDispatchLocksMu.Lock() + defer s.cmtDispatchLocksMu.Unlock() + if m, ok := s.cmtDispatchLocks[repoName]; ok { + return m + } + m := &sync.Mutex{} + s.cmtDispatchLocks[repoName] = m + return m +} - instanceType := "desktop" - if strings.Contains(repoName, "mobile") { - instanceType = "mobile" - } else if !strings.Contains(repoName, "desktop") { - logger.Warn("Repository is neither desktop nor mobile, skipping nightly E2E trigger") - return +// instanceKeyMatchesRunID reports whether key is the CMT tracking entry for testRunID. +func instanceKeyMatchesRunID(key, repoName string, testRunID int64) bool { + return key == cmtInstanceKey(repoName, testRunID) +} + +// claimedCMTRunIDs returns tracked CMT run ids for repoName; the poll uses this to skip already-claimed dispatches. +func (s *Server) claimedCMTRunIDs(repoName string) map[int64]bool { + prefix := repoName + "-cmt-" + s.e2eInstancesLock.Lock() + defer s.e2eInstancesLock.Unlock() + claimed := make(map[int64]bool, len(s.e2eInstances)) + for k := range s.e2eInstances { + if !strings.HasPrefix(k, prefix) { + continue + } + var id int64 + if _, err := fmt.Sscanf(k[len(prefix):], "%d", &id); err == nil && id > 0 { + claimed[id] = true + } } + return claimed +} - instances, err := s.createCMTInstancesForVersion(repoName, instanceType, s.resolveMattermostServerVersion(), "nightly") - if err != nil { - logger.WithError(err).Error("Failed to create nightly E2E instances") - return +// removeCMTInstancesByRunID removes and returns CMT instances for testRunID. Returns nil for runID 0 (unresolved dispatch sentinel). +func (s *Server) removeCMTInstancesByRunID(repoName string, testRunID int64, logger logrus.FieldLogger) []*E2EInstance { + if testRunID == 0 { + return nil } - // Include runID so two trigger runs against the same SHA (e.g. manual re-trigger) - // get separate tracking keys. The key still ends with "-{sha}" so - // findAndDestroyInstancesBySHA continues to match it by suffix. - key := fmt.Sprintf("%s-scheduled-%d-%s", repoName, runID, sha) + key := cmtInstanceKey(repoName, testRunID) s.e2eInstancesLock.Lock() - s.e2eInstances[key] = instances + instances := s.e2eInstances[key] + delete(s.e2eInstances, key) s.e2eInstancesLock.Unlock() - logger.WithField("tracking_key", key).Info("Nightly instances tracked, dispatching test workflow") - - // Determine run classification. Scheduled runs are always NIGHTLY regardless of branch - // (a scheduled run on master must not be classified as MASTER). Push-triggered runs - // derive their type from the branch name. - runType := "NIGHTLY" - nightly := true - if triggerEvent != "schedule" { - if branch == "master" || branch == "main" { - runType = "MASTER" - nightly = false - } else if s.isReleaseBranch(branch) { - runType = "RELEASE" - nightly = false - } - } - - var dispatchErr error - if instanceType == "desktop" { - instanceDetailsJSON, err := s.buildInstanceDetailsJSON(instances) - if err != nil { - logger.WithError(err).Error("Failed to build instance details JSON for nightly desktop run") - s.e2eInstancesLock.Lock() - delete(s.e2eInstances, key) - s.e2eInstancesLock.Unlock() - s.destroyE2EInstances(instances, logger) - return - } - // Pass the tracking key so the workflow_run completed handler can clean up by - // direct key lookup rather than SHA suffix matching (immune to new commits during - // the ~30 min instance-creation window). - dispatchErr = s.dispatchDesktopE2EWorkflow(owner, repoName, branch, sha, instanceDetailsJSON, runType, key, nightly) - } else { - if len(instances) < 3 { - logger.Errorf("Expected 3 mobile instances, got %d", len(instances)) - s.e2eInstancesLock.Lock() - delete(s.e2eInstances, key) - s.e2eInstancesLock.Unlock() - s.destroyE2EInstances(instances, logger) - return - } - dispatchErr = s.dispatchMobileE2EWorkflow(owner, repoName, branch, sha, - instances[0].URL, instances[1].URL, instances[2].URL, "both", runType, key) + if len(instances) == 0 { + logger.WithField("tracking_key", key).Debug("No run-id-tracked CMT instances found for cleanup") + return nil } + logger.WithFields(logrus.Fields{ + "tracking_key": key, + "instances": len(instances), + }).Info("Removed run-id-tracked CMT instances; destroying") + return instances +} - if dispatchErr != nil { - logger.WithError(dispatchErr).Error("Failed to dispatch test workflow for nightly run; cleaning up instances") - s.e2eInstancesLock.Lock() - delete(s.e2eInstances, key) - s.e2eInstancesLock.Unlock() - s.destroyE2EInstances(instances, logger) +// findAndDestroyInstancesByRunID destroys the CMT instance set keyed to the completing +// compatibility-matrix-testing.yml run id. +func (s *Server) findAndDestroyInstancesByRunID(repoName string, testRunID int64, logger logrus.FieldLogger) { + instances := s.removeCMTInstancesByRunID(repoName, testRunID, logger) + if len(instances) == 0 { return } - - logger.Info("Nightly E2E workflow dispatched successfully") + s.destroyE2EInstances(instances, logger) } -// isE2ETestWorkflow returns true if the workflow name is a configured E2E test workflow -// (as opposed to a trigger or CMT provisioner workflow). -func (s *Server) isE2ETestWorkflow(name string) bool { - for _, n := range s.Config.E2ETestWorkflowNames { - if n == name { - return true - } +// instanceKeyMatchesSHA reports whether key belongs to repoName, ends with headSHA, and matches the flow type (CMT vs push/scheduled) to prevent cross-flow SHA collisions. +func instanceKeyMatchesSHA(key, repoName, headSHA string, cmtOnly bool) bool { + if !strings.HasPrefix(key, repoName+"-") || !strings.HasSuffix(key, "-"+headSHA) { + return false } - return false + return strings.HasPrefix(key, repoName+"-cmt-") == cmtOnly } -// findAndDestroyInstancesBySHA scans the instance map for entries belonging to repoName -// whose tracking key ends with "-{headSHA}" (push-event, scheduled, and cmt keys) and destroys them. -func (s *Server) findAndDestroyInstancesBySHA(repoName, headSHA string, logger logrus.FieldLogger) { +// findAndDestroyInstancesBySHA destroys instances whose key ends with headSHA, scoped to CMT or non-CMT flows to prevent cross-flow teardown. +func (s *Server) findAndDestroyInstancesBySHA(repoName, headSHA string, cmtOnly bool, logger logrus.FieldLogger) { if headSHA == "" { return } - prefix := repoName + "-" - suffix := "-" + headSHA s.e2eInstancesLock.Lock() var found []*E2EInstance var keysToDelete []string for key, instances := range s.e2eInstances { - if strings.HasPrefix(key, prefix) && strings.HasSuffix(key, suffix) { - found = append(found, instances...) - keysToDelete = append(keysToDelete, key) + if !instanceKeyMatchesSHA(key, repoName, headSHA, cmtOnly) { + continue } + found = append(found, instances...) + keysToDelete = append(keysToDelete, key) } for _, k := range keysToDelete { delete(s.e2eInstances, k) @@ -287,9 +249,7 @@ func (s *Server) findAndDestroyInstancesBySHA(repoName, headSHA string, logger l s.destroyE2EInstances(found, logger) } -// parseServerVersionsFromString parses comma-separated server versions string -// Example input: "v11.1.0, v11.2.0, v12.0.0" -// Returns: ["v11.1.0", "v11.2.0", "v12.0.0"] +// parseServerVersionsFromString splits a comma-separated version string and trims whitespace. func parseServerVersionsFromString(input string) []string { versions := splitCommaSeparated(input) if versions == nil { @@ -298,10 +258,52 @@ func parseServerVersionsFromString(input string) []string { return versions } +// shouldTriggerCMT returns true for manual dispatch, RC tags, mobile build-release branches, or release branches. +func (s *Server) shouldTriggerCMT(triggerEvent, headBranch string) bool { + return triggerEvent == "workflow_dispatch" || + isRCTag(headBranch) || + isBuildReleaseBranch(headBranch) || + s.isReleaseBranch(headBranch) +} + +// rcTagPattern matches RC tags: optional "v", then semver, then "-rc" + number (e.g. v6.2.0-rc.1, 6.2.0-rc1). +var rcTagPattern = regexp.MustCompile(`^v?\d+\.\d+\.\d+-rc[.\-]?\d+$`) + +func isRCTag(ref string) bool { + return rcTagPattern.MatchString(ref) +} + +// buildReleaseBranchPattern matches mobile's build-release-NNN branches (exactly 3–4 digits). Update in sync with cmt-provisioner.yml if the convention changes. +var buildReleaseBranchPattern = regexp.MustCompile(`^build-release-\d{3,4}$`) + +// isBuildReleaseBranch reports whether ref is mobile's RC-cut branch (build-release-NNN). Used as a CMT gate separate from isReleaseBranch to avoid triggering on every cherry-pick. +func isBuildReleaseBranch(ref string) bool { + return buildReleaseBranchPattern.MatchString(ref) +} + +// handleCMTTrigger resolves instance type and server versions, then delegates to handleCMTWithServerVersions. +func (s *Server) handleCMTTrigger(owner, repoName, branch, sha string, runID int64, logger logrus.FieldLogger) { + instanceType := "desktop" + if strings.Contains(repoName, "mobile") { + instanceType = "mobile" + } else if !strings.Contains(repoName, "desktop") { + logger.Warn("Repository is neither desktop nor mobile, skipping CMT trigger") + return + } + + versions := s.cmtServerVersions() + logger.WithFields(logrus.Fields{ + "instanceType": instanceType, + "versions": versions, + }).Info("Provisioning CMT instances for resolved server versions") + + s.handleCMTWithServerVersions(owner, repoName, instanceType, branch, sha, versions, runID, logger) +} + // handleCMTWithServerVersions orchestrates CMT testing: creates one instance per server // version, builds the CMT_MATRIX JSON, 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 5 versions to prevent runaway provisioning + // Cap at 5 — also enforced by resolveCMTServerVersions, but Config.CMTServerVersions can bypass that. const maxVersions = 5 if len(serverVersions) > maxVersions { logger.Warnf("Capping server versions from %d to %d", len(serverVersions), maxVersions) @@ -316,8 +318,7 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, }) logger.Info("Starting CMT with server versions") - // Create one instance per version. The CMT matrix cross-products environment × server, - // so a single server URL handles all platform test runners for that version. + // All-or-nothing: a partial matrix silently drops coverage, so roll back on any failure. var allInstances []*E2EInstance var validVersions []string @@ -333,8 +334,9 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, instance, err := s.createSingleCMTInstance(repoName, instanceType, version, logger) if err != nil { - logger.WithError(err).Errorf("Failed to create instance for version %s, skipping", version) - continue + logger.WithError(err).Errorf("Failed to create instance for version %s; rolling back partial CMT matrix", version) + s.destroyE2EInstances(allInstances, logger) + return } allInstances = append(allInstances, instance) @@ -342,19 +344,11 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, } if len(allInstances) == 0 { - logger.Error("No instances created for any version") + logger.Warn("No CMT instances created (empty version set)") return } - logger.WithField("totalInstances", len(allInstances)).Info("CMT instances created, tracking for cleanup") - - // Track by runID+sha: runID prevents collision when two dispatches share the same - // branch HEAD SHA; the key still ends with "-{sha}" so findAndDestroyInstancesBySHA - // can locate it when compatibility-matrix-testing.yml completes (hours later). - key := fmt.Sprintf("%s-cmt-%d-%s", repoName, runID, sha) - s.e2eInstancesLock.Lock() - s.e2eInstances[key] = allInstances - s.e2eInstancesLock.Unlock() + logger.WithField("totalInstances", len(allInstances)).Info("CMT instances created, dispatching test workflow") // Build CMT_MATRIX JSON and dispatch compatibility-matrix-testing.yml. var cmtMatrixJSON string @@ -366,30 +360,42 @@ func (s *Server) handleCMTWithServerVersions(repoOwner, repoName, instanceType, } if buildErr != nil { logger.WithError(buildErr).Error("Failed to build CMT_MATRIX JSON") - s.e2eInstancesLock.Lock() - delete(s.e2eInstances, key) - s.e2eInstancesLock.Unlock() s.destroyE2EInstances(allInstances, logger) return } - // Pass the tracking key so the workflow_run completed handler can clean up by - // direct key lookup rather than SHA suffix matching. - if err := s.dispatchCMTWorkflow(repoOwner, repoName, sha, branch, cmtMatrixJSON, instanceType, key, runID, logger); err != nil { + // Serialize per repo: concurrent triggers could race to the same run id without this mutex. + dispatchLock := s.cmtDispatchMutex(repoName) + dispatchLock.Lock() + defer dispatchLock.Unlock() + + testRunID, err := s.dispatchCMTWorkflow(repoOwner, repoName, branch, cmtMatrixJSON, instanceType, logger) + if err != nil { logger.WithError(err).Error("Failed to dispatch compatibility-matrix-testing.yml") - s.e2eInstancesLock.Lock() - delete(s.e2eInstances, key) - s.e2eInstancesLock.Unlock() s.destroyE2EInstances(allInstances, logger) return } - logger.WithField("tracking_key", key).Info("CMT workflow dispatched successfully; instances tracked for cleanup") + if testRunID == 0 { + // Dispatch succeeded but run id unresolved — leave instances for the periodic stale-scan. + logger.WithField("trigger_run", runID).Warn("CMT dispatched but test run id unresolved; instances left to periodic stale-scan backstop") + return + } + + key := cmtInstanceKey(repoName, testRunID) + s.e2eInstancesLock.Lock() + s.e2eInstances[key] = allInstances + s.e2eInstancesLock.Unlock() + + logger.WithFields(logrus.Fields{ + "tracking_key": key, + "test_run_id": testRunID, + "trigger_run": runID, + }).Info("CMT workflow dispatched successfully; instances tracked for cleanup") } // createSingleCMTInstance creates one Mattermost cloud instance for a CMT server version. -// Unlike createCMTInstancesForVersion (which creates 3 platform-specific instances for -// nightly runs), CMT only needs one server — the matrix handles parallelism. +// CMT only needs one server per version — the test matrix handles platform parallelism. func (s *Server) createSingleCMTInstance(repoName, instanceType, version string, logger logrus.FieldLogger) (*E2EInstance, error) { // Name format: {type}-{version}-{hex6} sanitizedVersion := sanitizeForDNS(version) @@ -402,29 +408,14 @@ func (s *Server) createSingleCMTInstance(repoName, instanceType, version string, return s.createCloudInstallation(context.Background(), name, version, username, password, instanceType, logger) } -// cmtServer is the server entry in CMT_MATRIX JSON. +// cmtServer is one entry in CMT_MATRIX. Latest is set on the highest-semver entry (mobile only); omitempty keeps it absent from desktop JSON. type cmtServer struct { Version string `json:"version"` URL string `json:"url"` + Latest bool `json:"latest,omitempty"` } -// buildDesktopCMTMatrixJSON builds the CMT_MATRIX JSON for compatibility-matrix-testing.yml -// in the desktop repo. The matrix cross-products environment × server, so one server URL -// is shared across all three platform runners. -// -// Schema: -// -// { -// "environment": [ -// {"os": "linux", "runner": "ubuntu-22.04"}, -// {"os": "macos", "runner": "macos-13"}, -// {"os": "windows", "runner": "windows-2022"} -// ], -// "server": [ -// {"version": "v11.1.0", "url": "https://..."}, -// ... -// ] -// } +// buildDesktopCMTMatrixJSON builds CMT_MATRIX for compatibility-matrix-testing.yml: 3 fixed environment runners × N server versions. func buildDesktopCMTMatrixJSON(versions []string, instances []*E2EInstance) (string, error) { type cmtEnvironment struct { OS string `json:"os"` @@ -456,28 +447,42 @@ func buildDesktopCMTMatrixJSON(versions []string, instances []*E2EInstance) (str return string(b), nil } -// buildMobileCMTMatrixJSON builds the CMT_MATRIX JSON for compatibility-matrix-testing.yml -// in the mobile repo. One iOS test job is created per server version. -// -// Schema: -// -// { -// "server": [ -// {"version": "v11.1.0", "url": "https://..."}, -// ... -// ] -// } +// buildMobileCMTMatrixJSON builds CMT_MATRIX for compatibility-matrix-testing.yml: N server entries, highest-semver marked latest:true. func buildMobileCMTMatrixJSON(versions []string, instances []*E2EInstance) (string, error) { type mobileCMTMatrix struct { Server []cmtServer `json:"server"` } + // Mark the highest-parseable version as latest; fall back to last entry if none parse. + latestIdx := -1 + var latestVer cmtVersion + for i, version := range versions { + if i >= len(instances) { + break + } + v, ok := parseCMTVersion(version) + if !ok { + continue + } + if latestIdx == -1 || latestVer.less(v) { + latestVer = v + latestIdx = i + } + } + if latestIdx == -1 && len(versions) > 0 { + latestIdx = len(versions) - 1 + } + var matrix mobileCMTMatrix for i, version := range versions { if i >= len(instances) { break } - matrix.Server = append(matrix.Server, cmtServer{Version: version, URL: instances[i].URL}) + entry := cmtServer{Version: version, URL: instances[i].URL} + if i == latestIdx { + entry.Latest = true + } + matrix.Server = append(matrix.Server, entry) } b, err := json.Marshal(matrix) @@ -487,20 +492,21 @@ func buildMobileCMTMatrixJSON(versions []string, instances []*E2EInstance) (stri return string(b), nil } -// dispatchCMTWorkflow dispatches compatibility-matrix-testing.yml with the populated -// CMT_MATRIX JSON. trackingKey is the s.e2eInstances map key for this run; it is -// embedded as "mw_tracking_key" in the workflow inputs so the workflow_run completed -// handler can do a direct key lookup instead of fragile SHA suffix matching. -// runID is the CMT provisioner workflow run ID, passed as cmt_run_id so the test workflow -// can call back to Matterwick for instance cleanup. -func (s *Server) dispatchCMTWorkflow(repoOwner, repoName, sha, branch, cmtMatrixJSON, instanceType, trackingKey string, runID int64, logger logrus.FieldLogger) error { +// dispatchCMTWorkflow dispatches compatibility-matrix-testing.yml and polls for the run id used to key cleanup. +func (s *Server) dispatchCMTWorkflow(repoOwner, repoName, branch, cmtMatrixJSON, instanceType string, logger logrus.FieldLogger) (int64, error) { ctx := context.Background() client := newGithubClient(s.Config.GithubAccessToken) + // Snapshot run ids before dispatch so the poll only accepts strictly-new ids. + preDispatchRunIDs, snapErr := s.listExistingCMTRunIDs(repoOwner, repoName, branch, logger) + if snapErr != nil { + // Non-fatal: poll will still accept any new id that appears after this moment. + logger.WithError(snapErr).Warn("Failed to snapshot pre-dispatch CMT run ids; poll will only accept ids not seen before dispatch") + preDispatchRunIDs = map[int64]bool{} + } + workflowInputs := map[string]interface{}{ - "CMT_MATRIX": cmtMatrixJSON, - "cmt_run_id": fmt.Sprintf("%d", runID), - "mw_tracking_key": trackingKey, + "CMT_MATRIX": cmtMatrixJSON, } if instanceType == "desktop" { workflowInputs["DESKTOP_VERSION"] = branch @@ -513,7 +519,6 @@ func (s *Server) dispatchCMTWorkflow(repoOwner, repoName, sha, branch, cmtMatrix "instanceType": instanceType, }).Debug("Dispatching compatibility-matrix-testing.yml") - // GitHub workflow_dispatch requires a branch or tag name as ref, not a commit SHA. req, err := client.NewRequest("POST", fmt.Sprintf("/repos/%s/%s/actions/workflows/compatibility-matrix-testing.yml/dispatches", repoOwner, repoName), map[string]interface{}{ @@ -521,113 +526,116 @@ func (s *Server) dispatchCMTWorkflow(repoOwner, repoName, sha, branch, cmtMatrix "inputs": workflowInputs, }) if err != nil { - return fmt.Errorf("failed to create CMT workflow dispatch request: %w", err) + return 0, fmt.Errorf("failed to create CMT workflow dispatch request: %w", err) } resp, err := client.Do(ctx, req, nil) if err != nil { - return fmt.Errorf("failed to dispatch compatibility-matrix-testing.yml: %w", err) + return 0, fmt.Errorf("failed to dispatch compatibility-matrix-testing.yml: %w", err) } if resp.StatusCode != 204 { - return fmt.Errorf("unexpected status %d from compatibility-matrix-testing.yml dispatch", resp.StatusCode) + return 0, fmt.Errorf("unexpected status %d from compatibility-matrix-testing.yml dispatch", resp.StatusCode) + } + + testRunID, err := s.pollDispatchedWorkflowRun(repoOwner, repoName, "compatibility-matrix-testing.yml", branch, preDispatchRunIDs, logger) + if err != nil { + // Return 0 to skip run-id tracking; leave instances for the periodic stale-scan. + logger.WithError(err).Warn("Dispatched compatibility-matrix-testing.yml but could not resolve test run id within poll deadline") + return 0, nil } - logger.Info("compatibility-matrix-testing.yml dispatched successfully") - return nil + logger.WithField("test_run_id", testRunID).Info("compatibility-matrix-testing.yml dispatched successfully") + return testRunID, nil } -// createCMTInstancesForVersion creates 3 instances (one per platform) in parallel for a -// given server version. Used by nightly runs which dispatch the platform-aware -// e2e-functional.yml / e2e-detox-pr.yml workflows (not the CMT matrix workflow). -// Results are returned in platforms[] order so index-based assignment is stable. -func (s *Server) createCMTInstancesForVersion(repoName, instanceType, version, purpose string) ([]*E2EInstance, error) { - var platforms []string - if instanceType == "desktop" { - platforms = []string{"linux", "macos", "windows"} - } else { - platforms = []string{"site-1", "site-2", "site-3"} +// listExistingCMTRunIDs returns recent run ids for compatibility-matrix-testing.yml on branch, snapshotted before dispatch. +func (s *Server) listExistingCMTRunIDs(repoOwner, repoName, branch string, logger logrus.FieldLogger) (map[int64]bool, error) { + runs, err := s.listCMTRuns(repoOwner, repoName, "compatibility-matrix-testing.yml", branch) + if err != nil { + return nil, err } + ids := make(map[int64]bool, len(runs)) + for _, run := range runs { + ids[run.ID] = true + } + logger.WithField("pre_dispatch_run_count", len(ids)).Debug("Snapshotted pre-dispatch CMT run ids") + return ids, nil +} - // Name format: {type}-{version}-{platform}-{hex6} - sanitizedVersion := sanitizeForDNS(version) - uid := e2eUniqueSuffix() - - logger := s.Logger.WithFields(logrus.Fields{ - "repo": repoName, - "type": instanceType, - "version": version, - }) +// cmtWorkflowRun is the minimal slice of the GitHub workflow_run object we need. +type cmtWorkflowRun struct { + ID int64 `json:"id"` + CreatedAt string `json:"created_at"` +} - username := s.Config.E2EUsername - password := s.getE2EPassword(instanceType) +// listCMTRuns fetches up to the 10 most-recent workflow_dispatch runs for workflowFile on branch. +func (s *Server) listCMTRuns(repoOwner, repoName, workflowFile, branch string) ([]cmtWorkflowRun, error) { + client := newGithubClient(s.Config.GithubAccessToken) + if s.githubAPIBase != "" { + if baseURL, parseErr := url.Parse(s.githubAPIBase); parseErr == nil { + client.BaseURL = baseURL + } + } - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() - type result struct { - instance *E2EInstance - err error - } - results := make([]result, len(platforms)) - var wg sync.WaitGroup - - for i, platform := range platforms { - 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, version, username, password, instanceType, logger) - if err != nil { - cancel() - results[idx] = result{err: err} - return - } - inst.Platform = platform - if instanceType == "desktop" { - inst.Runner = getRunnerForPlatform(platform) - } - results[idx] = result{instance: inst} - }(i, platform) + listURL := fmt.Sprintf("/repos/%s/%s/actions/workflows/%s/runs?event=workflow_dispatch&branch=%s&per_page=10", + repoOwner, repoName, workflowFile, url.QueryEscape(branch)) + req, err := client.NewRequest("GET", listURL, nil) + if err != nil { + return nil, fmt.Errorf("failed to create workflow runs list request: %w", err) + } + var resp struct { + WorkflowRuns []cmtWorkflowRun `json:"workflow_runs"` } + if _, err = client.Do(ctx, req, &resp); err != nil { + return nil, fmt.Errorf("failed to list workflow runs: %w", err) + } + return resp.WorkflowRuns, nil +} + +// pollDispatchedWorkflowRun polls for a new run id not in preDispatchRunIDs. The pre-dispatch snapshot makes this race-free vs. time-window approaches. +func (s *Server) pollDispatchedWorkflowRun(repoOwner, repoName, workflowFile, branch string, preDispatchRunIDs map[int64]bool, logger logrus.FieldLogger) (int64, error) { + deadline := time.Now().Add(90 * time.Second) + for time.Now().Before(deadline) { + runs, err := s.listCMTRuns(repoOwner, repoName, workflowFile, branch) + if err != nil { + return 0, err + } - wg.Wait() + // Also skip ids already tracked to guard against concurrent dispatches overwriting a stored entry. + claimed := s.claimedCMTRunIDs(repoName) - var instances []*E2EInstance - var firstErr error - for _, r := range results { - if r.err != nil { - if firstErr == nil { - firstErr = r.err + var bestID int64 + var bestCreated time.Time + for _, run := range runs { + if preDispatchRunIDs[run.ID] { + continue + } + if claimed[run.ID] { + continue + } + createdAt, parseErr := time.Parse(time.RFC3339, run.CreatedAt) + if parseErr != nil { + continue + } + if run.ID > bestID { + bestID = run.ID + bestCreated = createdAt } - } else { - instances = append(instances, r.instance) } - } + if bestID != 0 { + logger.WithFields(logrus.Fields{ + "test_run_id": bestID, + "created_at": bestCreated, + }).Debug("Resolved dispatched CMT test workflow run") + return bestID, nil + } - if firstErr != nil { - logger.WithError(firstErr).Error("Failed to create one or more instances; destroying all") - s.destroyE2EInstances(instances, logger) - return nil, firstErr + time.Sleep(2 * time.Second) } - logger.WithField("instanceCount", len(instances)).Info("Instances created for version") - return instances, nil + return 0, fmt.Errorf("timed out polling for dispatched workflow run on branch %s", branch) } -// handleCMTRunCleanup is a best-effort fallback for CMT cleanup when the trigger workflow -// completes. Because the CMT trigger is a lightweight workflow that completes in seconds — -// well before the 30-minute provisioning goroutine stores instances — this function will -// most often find nothing. The primary cleanup path is findAndDestroyInstancesBySHA, -// triggered when compatibility-matrix-testing.yml completes. -func (s *Server) handleCMTRunCleanup(repoName, sha string, logger logrus.FieldLogger) { - logger = logger.WithFields(logrus.Fields{ - "repo": repoName, - "sha": sha, - "type": "cmt_cleanup_fallback", - }) - logger.Debug("CMT trigger completed — sha-based cleanup is the primary path") - s.findAndDestroyInstancesBySHA(repoName, sha, logger) -}