Fix race condition when re-triggering GitHub Actions

saschagrunert · saschagrunert · commit d901412624d9 · 2025-12-04T10:21:39.000+01:00
When a GitHub Action is re-triggered, GitHub temporarily removes the old
check status before the new run starts. This causes a race where Tide may
merge the PR during the brief window when the check is missing.

This fix tracks previously seen contexts per PR/commit. When a required
context disappears, it's treated as PENDING to prevent premature merging.

Signed-off-by: Sascha Grunert &lt;sgrunert@redhat.com&gt;
diff --git a/pkg/tide/status.go b/pkg/tide/status.go
@@ -62,6 +62,62 @@ type storedState struct {
 	PreviousQuery string
 }
 
+// contextHistory tracks previously seen contexts for a PR/commit combination
+// to detect when required contexts disappear (e.g., when GitHub Actions are re-triggered)
+type contextHistory struct {
+	// prCommitContexts maps "org/repo#number:sha" -> set of context names seen
+	prCommitContexts map[string]sets.Set[string]
+	sync.RWMutex
+}
+
+func newContextHistory() *contextHistory {
+	return &contextHistory{
+		prCommitContexts: make(map[string]sets.Set[string]),
+	}
+}
+
+// recordContexts records the contexts seen for a PR/commit combination
+func (ch *contextHistory) recordContexts(org, repo string, number int, sha string, contexts []string) {
+	key := fmt.Sprintf("%s/%s#%d:%s", org, repo, number, sha)
+	ch.Lock()
+	defer ch.Unlock()
+	ch.prCommitContexts[key] = sets.New[string](contexts...)
+}
+
+// getDisappearedContexts returns contexts that were previously seen but are now missing
+// This detects the race condition when GitHub Actions are re-triggered and the check
+// status temporarily disappears before the new run starts.
+func (ch *contextHistory) getDisappearedContexts(org, repo string, number int, sha string, currentContexts []string) []string {
+	key := fmt.Sprintf("%s/%s#%d:%s", org, repo, number, sha)
+	ch.RLock()
+	defer ch.RUnlock()
+
+	previousContexts, exists := ch.prCommitContexts[key]
+	if !exists {
+		// First time seeing this PR/commit, no disappeared contexts
+		return nil
+	}
+
+	currentSet := sets.New[string](currentContexts...)
+	var disappeared []string
+	for ctx := range previousContexts.Difference(currentSet) {
+		disappeared = append(disappeared, ctx)
+	}
+	return disappeared
+}
+
+// cleanup removes old entries to prevent unbounded memory growth
+// This should be called periodically with PRs that are no longer active
+func (ch *contextHistory) cleanup(activeKeys sets.Set[string]) {
+	ch.Lock()
+	defer ch.Unlock()
+	for key := range ch.prCommitContexts {
+		if !activeKeys.Has(key) {
+			delete(ch.prCommitContexts, key)
+		}
+	}
+}
+
 // statusController is a goroutine runs in the background
 type statusController struct {
 	pjClient           ctrlruntimeclient.Client
@@ -106,6 +162,9 @@ type statusUpdate struct {
 	// newPoolPending is a size 1 chan that signals that the main Tide loop has
 	// updated the 'poolPRs' field with a freshly updated pool.
 	newPoolPending chan bool
+	// contextHistory tracks previously seen contexts to detect disappearing contexts
+	// when GitHub Actions are re-triggered (addresses issue #337)
+	contextHistory *contextHistory
 }
 
 func (sc *statusController) shutdown() {
@@ -238,7 +297,8 @@ func requirementDiff(pr *PullRequest, q *config.TideQuery, cc contextChecker) (s
 	log := logrus.WithFields(pr.logFields())
 	for _, commit := range pr.Commits.Nodes {
 		if commit.Commit.OID == pr.HeadRefOID {
-			for _, ctx := range unsuccessfulContexts(append(commit.Commit.Status.Contexts, checkRunNodesToContexts(log, commit.Commit.StatusCheckRollup.Contexts.Nodes)...), cc, log) {
+			crc := CodeReviewCommonFromPullRequest(pr)
+			for _, ctx := range unsuccessfulContexts(append(commit.Commit.Status.Contexts, checkRunNodesToContexts(log, commit.Commit.StatusCheckRollup.Contexts.Nodes)...), cc, crc, nil, log) {
 				contexts = append(contexts, string(ctx.Context))
 			}
 		}
diff --git a/pkg/tide/tide.go b/pkg/tide/tide.go
@@ -406,6 +406,7 @@ func NewController(
 	statusUpdate := &statusUpdate{
 		dontUpdateStatus: &threadSafePRSet{},
 		newPoolPending:   make(chan bool),
+		contextHistory:   newContextHistory(),
 	}
 
 	sc, err := newStatusController(ctx, logger, ghcStatus, mgr, gc, cfg, opener, statusURI, mergeChecker, usesGitHubAppsAuth, statusUpdate)
@@ -675,7 +676,11 @@ func (c *syncController) filterSubpools(mergeAllowed func(*CodeReviewCommon) (st
 				return
 			}
 			key := poolKey(sp.org, sp.repo, sp.branch)
-			if spFiltered := filterSubpool(c.provider, mergeAllowed, sp); spFiltered != nil {
+			var ch *contextHistory
+			if c.statusUpdate != nil {
+				ch = c.statusUpdate.contextHistory
+			}
+			if spFiltered := filterSubpool(c.provider, mergeAllowed, sp, ch); spFiltered != nil {
 				sp.log.WithField("key", key).WithField("pool", spFiltered).Debug("filtered sub-pool")
 
 				lock.Lock()
@@ -727,10 +732,10 @@ func (c *syncController) initSubpoolData(sp *subpool) error {
 // should be deleted.
 //
 // This function works for any source code provider.
-func filterSubpool(provider provider, mergeAllowed func(*CodeReviewCommon) (string, error), sp *subpool) *subpool {
+func filterSubpool(provider provider, mergeAllowed func(*CodeReviewCommon) (string, error), sp *subpool, ch *contextHistory) *subpool {
 	var toKeep []CodeReviewCommon
 	for _, pr := range sp.prs {
-		if !filterPR(provider, mergeAllowed, sp, &pr) {
+		if !filterPR(provider, mergeAllowed, sp, &pr, ch) {
 			toKeep = append(toKeep, pr)
 		}
 	}
@@ -752,7 +757,7 @@ func filterSubpool(provider provider, mergeAllowed func(*CodeReviewCommon) (stri
 //     retesting them.)
 //
 // This function works for any source code provider.
-func filterPR(provider provider, mergeAllowed func(*CodeReviewCommon) (string, error), sp *subpool, pr *CodeReviewCommon) bool {
+func filterPR(provider provider, mergeAllowed func(*CodeReviewCommon) (string, error), sp *subpool, pr *CodeReviewCommon, ch *contextHistory) bool {
 	log := sp.log.WithFields(pr.logFields())
 	// Skip PRs that are known to be unmergeable.
 	if reason, err := mergeAllowed(pr); err != nil {
@@ -778,7 +783,7 @@ func filterPR(provider provider, mergeAllowed func(*CodeReviewCommon) (string, e
 		}
 		return false
 	}
-	for _, ctx := range unsuccessfulContexts(contexts, sp.cc[pr.Number], log) {
+	for _, ctx := range unsuccessfulContexts(contexts, sp.cc[pr.Number], pr, ch, log) {
 		if ctx.State != githubql.StatusStatePending {
 			log.WithField("context", ctx.Context).Debug("filtering out PR as unsuccessful context is not pending")
 			return true
@@ -853,7 +858,11 @@ func (c *syncController) isPassingTests(log *logrus.Entry, pr *CodeReviewCommon,
 		// If we can't get the status of the commit, assume that it is failing.
 		return false
 	}
-	unsuccessful := unsuccessfulContexts(contexts, cc, log)
+	var ch *contextHistory
+	if c.statusUpdate != nil {
+		ch = c.statusUpdate.contextHistory
+	}
+	unsuccessful := unsuccessfulContexts(contexts, cc, pr, ch, log)
 	return len(unsuccessful) == 0
 }
 
@@ -862,8 +871,12 @@ func (c *syncController) isPassingTests(log *logrus.Entry, pr *CodeReviewCommon,
 // If the branchProtection is set to only check for required checks, we will skip
 // all non-required tests. If required tests are missing from the list, they will be
 // added to the list of failed contexts.
-func unsuccessfulContexts(contexts []Context, cc contextChecker, log *logrus.Entry) []Context {
+// It also detects when required contexts disappear (e.g., when GitHub Actions are re-triggered)
+// and treats them as PENDING to prevent premature merging (addresses issue #337).
+func unsuccessfulContexts(contexts []Context, cc contextChecker, pr *CodeReviewCommon, ch *contextHistory, log *logrus.Entry) []Context {
 	var failed []Context
+	contextNames := contextsToStrings(contexts)
+
 	for _, ctx := range contexts {
 		if string(ctx.Context) == statusContext {
 			continue
@@ -875,13 +888,39 @@ func unsuccessfulContexts(contexts []Context, cc contextChecker, log *logrus.Ent
 			failed = append(failed, ctx)
 		}
 	}
-	for _, c := range cc.MissingRequiredContexts(contextsToStrings(contexts)) {
+
+	// Add missing required contexts
+	for _, c := range cc.MissingRequiredContexts(contextNames) {
 		failed = append(failed, newExpectedContext(c))
 	}
 
+	// Check for disappeared contexts (race condition fix for issue #337)
+	// When a GitHub Action is re-triggered, the check may temporarily disappear
+	// from the status before the new run starts. We need to detect this and
+	// treat it as a non-passing state to prevent premature merging.
+	if ch != nil && pr != nil {
+		disappeared := ch.getDisappearedContexts(pr.Org, pr.Repo, pr.Number, pr.HeadRefOID, contextNames)
+		if len(disappeared) > 0 {
+			log.WithField("disappeared_contexts", disappeared).
+				Warn("Detected disappeared contexts - likely due to re-triggered checks")
+			for _, c := range disappeared {
+				// Only add if it's not optional and not already in failed list
+				if !cc.IsOptional(c) {
+					failed = append(failed, Context{
+						Context:     githubql.String(c),
+						State:       githubql.StatusStatePending,
+						Description: githubql.String("Context disappeared - likely re-triggered"),
+					})
+				}
+			}
+		}
+		// Record current contexts for future comparison
+		ch.recordContexts(pr.Org, pr.Repo, pr.Number, pr.HeadRefOID, contextNames)
+	}
+
 	log.WithFields(logrus.Fields{
 		"total_context_count":  len(contexts),
-		"context_names":        contextsToStrings(contexts),
+		"context_names":        contextNames,
 		"failed_context_count": len(failed),
 		"failed_context_names": contextsToStrings(failed),
 	}).Debug("Filtered out failed contexts")
diff --git a/pkg/tide/tide_test.go b/pkg/tide/tide_test.go
@@ -2908,7 +2908,7 @@ func TestFilterSubpool(t *testing.T) {
 				mergeChecker: mmc,
 				logger:       logrus.WithContext(context.Background()),
 			}
-			filtered := filterSubpool(provider, mmc.isAllowedToMerge, sp)
+			filtered := filterSubpool(provider, mmc.isAllowedToMerge, sp, newContextHistory())
 			if len(tc.expectedPRs) == 0 {
 				if filtered != nil {
 					t.Fatalf("Expected subpool to be pruned, but got: %v", filtered)
@@ -5482,3 +5482,134 @@ func TestSerialRetestingConsidersPRThatIsCurrentlyBeingSRetested(t *testing.T) {
 	}
 
 }
+
+// TestUnsuccessfulContextsWithDisappearedContexts tests the fix for issue #337
+// where re-triggering GitHub Actions causes contexts to temporarily disappear
+func TestUnsuccessfulContextsWithDisappearedContexts(t *testing.T) {
+	log := logrus.NewEntry(logrus.StandardLogger())
+
+	testCases := []struct {
+		name               string
+		contexts           []Context
+		previousContexts   []string
+		requiredContexts   []string
+		expectedFailed     int
+		expectWarning      bool
+	}{
+		{
+			name: "no disappeared contexts",
+			contexts: []Context{
+				{Context: githubql.String("test-1"), State: githubql.StatusStateSuccess},
+				{Context: githubql.String("test-2"), State: githubql.StatusStateSuccess},
+			},
+			previousContexts: []string{"test-1", "test-2"},
+			requiredContexts: []string{"test-1", "test-2"},
+			expectedFailed:   0,
+			expectWarning:    false,
+		},
+		{
+			name: "context disappeared - race condition",
+			contexts: []Context{
+				{Context: githubql.String("test-1"), State: githubql.StatusStateSuccess},
+			},
+			previousContexts: []string{"test-1", "test-2"},
+			requiredContexts: []string{"test-1", "test-2"},
+			expectedFailed:   2, // one missing required + one disappeared
+			expectWarning:    true,
+		},
+		{
+			name: "optional context disappeared - should not fail",
+			contexts: []Context{
+				{Context: githubql.String("test-1"), State: githubql.StatusStateSuccess},
+			},
+			previousContexts: []string{"test-1", "test-2"},
+			requiredContexts: []string{"test-1"},
+			expectedFailed:   1, // test-2 is unknown, treated as required by default
+			expectWarning:    true,
+		},
+		{
+			name: "first time seeing PR - no history",
+			contexts: []Context{
+				{Context: githubql.String("test-1"), State: githubql.StatusStateSuccess},
+			},
+			previousContexts: nil,
+			requiredContexts: []string{"test-1", "test-2"},
+			expectedFailed:   1, // only missing required, no disappeared
+			expectWarning:    false,
+		},
+		{
+			name: "context reappeared after disappearing",
+			contexts: []Context{
+				{Context: githubql.String("test-1"), State: githubql.StatusStateSuccess},
+				{Context: githubql.String("test-2"), State: githubql.StatusStatePending},
+			},
+			previousContexts: []string{"test-1"},
+			requiredContexts: []string{"test-1", "test-2"},
+			expectedFailed:   1, // test-2 is pending (not disappeared, it's present)
+			expectWarning:    false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ch := newContextHistory()
+			pr := &CodeReviewCommon{
+				Org:        "test-org",
+				Repo:       "test-repo",
+				Number:     123,
+				HeadRefOID: "abc123",
+			}
+
+			// Set up previous context history if provided
+			if tc.previousContexts != nil {
+				ch.recordContexts(pr.Org, pr.Repo, pr.Number, pr.HeadRefOID, tc.previousContexts)
+			}
+
+			// Create a simple context checker
+			cc := &config.TideContextPolicy{
+				RequiredContexts: tc.requiredContexts,
+			}
+
+			failed := unsuccessfulContexts(tc.contexts, cc, pr, ch, log)
+
+			if len(failed) != tc.expectedFailed {
+				t.Errorf("expected %d failed contexts, got %d: %+v", tc.expectedFailed, len(failed), failed)
+			}
+		})
+	}
+}
+
+// TestContextHistoryCleanup tests that old context history entries are cleaned up
+func TestContextHistoryCleanup(t *testing.T) {
+	ch := newContextHistory()
+
+	// Record contexts for multiple PRs
+	ch.recordContexts("org", "repo", 1, "sha1", []string{"test-1", "test-2"})
+	ch.recordContexts("org", "repo", 2, "sha2", []string{"test-1", "test-2"})
+	ch.recordContexts("org", "repo", 3, "sha3", []string{"test-1", "test-2"})
+
+	if len(ch.prCommitContexts) != 3 {
+		t.Errorf("expected 3 entries, got %d", len(ch.prCommitContexts))
+	}
+
+	// Only PR 1 and 2 are still active
+	activeKeys := sets.New[string](
+		"org/repo#1:sha1",
+		"org/repo#2:sha2",
+	)
+	ch.cleanup(activeKeys)
+
+	if len(ch.prCommitContexts) != 2 {
+		t.Errorf("expected 2 entries after cleanup, got %d", len(ch.prCommitContexts))
+	}
+
+	if !ch.prCommitContexts["org/repo#1:sha1"].Has("test-1") {
+		t.Error("PR 1 should still be tracked")
+	}
+	if !ch.prCommitContexts["org/repo#2:sha2"].Has("test-1") {
+		t.Error("PR 2 should still be tracked")
+	}
+	if _, exists := ch.prCommitContexts["org/repo#3:sha3"]; exists {
+		t.Error("PR 3 should have been cleaned up")
+	}
+}
