Skip to content

Fix race condition when re-triggering GitHub Actions #563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
saschagrunert wants to merge 1 commit into
base: main
Choose a base branch
from
+318 −12
Open

Fix race condition when re-triggering GitHub Actions #563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 65 additions & 1 deletion pkg/tide/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,66 @@ 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 map[string]sets.Set[string]
sync.Mutex
}

func newContextHistory() *contextHistory {
return &contextHistory{
prCommitContexts: make(map[string]sets.Set[string]),
}
}

// checkAndUpdate returns contexts that disappeared since last check, then updates history.
// This detects the race condition when GitHub Actions are re-triggered and the check
// status temporarily disappears before the new run starts.
//
// Note: Uses PR number only (not commit SHA) to track across force pushes. This means
// if a force push genuinely removes a context from the workflow, it will be detected
// as "disappeared" until the next sync. This is acceptable as it errs on the side of
// preventing premature merges.
func (ch *contextHistory) checkAndUpdate(pr *CodeReviewCommon, currentContexts []string) []string {
key := fmt.Sprintf("%s/%s#%d", pr.Org, pr.Repo, pr.Number)

ch.Lock()
defer ch.Unlock()

var disappeared []string
if previous, exists := ch.prCommitContexts[key]; exists {
currentSet := sets.New[string](currentContexts...)
for ctx := range previous.Difference(currentSet) {
disappeared = append(disappeared, ctx)
}
}

ch.prCommitContexts[key] = sets.New[string](currentContexts...)
return disappeared
}

// prune removes entries for PRs that are no longer in the pool.
// This prevents unbounded memory growth by cleaning up old/merged PRs.
func (ch *contextHistory) prune(activePRs map[string]CodeReviewCommon) {
ch.Lock()
defer ch.Unlock()

// Build set of active PR keys
active := sets.New[string]()
for _, pr := range activePRs {
key := fmt.Sprintf("%s/%s#%d", pr.Org, pr.Repo, pr.Number)
active.Insert(key)
}

// Remove entries not in active set
for key := range ch.prCommitContexts {
if !active.Has(key) {
delete(ch.prCommitContexts, key)
}
}
}

// statusController is a goroutine runs in the background
type statusController struct {
pjClient ctrlruntimeclient.Client
Expand Down Expand Up @@ -106,6 +166,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() {
Expand Down Expand Up @@ -238,7 +301,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))
}
}
Expand Down
69 changes: 59 additions & 10 deletions pkg/tide/tide.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -582,6 +583,10 @@ func (c *syncController) Sync() error {
c.statusUpdate.poolPRs = poolPRMap(filteredPools)
c.statusUpdate.baseSHAs = baseSHAMap(filteredPools)
c.statusUpdate.requiredContexts = requiredContextsMap(filteredPools)
// Prune old PR entries from context history to prevent memory leak
if c.statusUpdate.contextHistory != nil {
c.statusUpdate.contextHistory.prune(c.statusUpdate.poolPRs)
}
select {
case c.statusUpdate.newPoolPending <- true:
c.statusUpdate.dontUpdateStatus.reset()
Expand Down Expand Up @@ -675,7 +680,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()
Expand Down Expand Up @@ -727,10 +736,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)
}
}
Expand All @@ -752,7 +761,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 {
Expand All @@ -778,7 +787,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
Expand Down Expand Up @@ -853,7 +862,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
}

Expand All @@ -862,8 +875,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
Expand All @@ -875,13 +892,45 @@ func unsuccessfulContexts(contexts []Context, cc contextChecker, log *logrus.Ent
failed = append(failed, ctx)
}
}
for _, c := range cc.MissingRequiredContexts(contextsToStrings(contexts)) {
failed = append(failed, newExpectedContext(c))

// Track which contexts are added as failed to avoid duplicates
failedSet := sets.New[string]()
for _, ctx := range failed {
failedSet.Insert(string(ctx.Context))
}

// Add missing required contexts
for _, c := range cc.MissingRequiredContexts(contextNames) {
if !failedSet.Has(c) {
failed = append(failed, newExpectedContext(c))
failedSet.Insert(c)
}
}

// Check for disappeared contexts (race condition fix)
// When a GitHub Action is re-triggered, the check may temporarily disappear
// from the status before the new run starts. Detect this and treat disappeared
// required contexts as non-passing to prevent premature merging.
if ch != nil && pr != nil {
disappeared := ch.checkAndUpdate(pr, contextNames)
if len(disappeared) > 0 {
log.WithField("disappeared_contexts", disappeared).
Info("Detected disappeared contexts - likely due to re-triggered checks")
}
for _, c := range disappeared {
if !cc.IsOptional(c) && !failedSet.Has(c) {
failed = append(failed, Context{
Context: githubql.String(c),
State: githubql.StatusStatePending,
Description: githubql.String("Context disappeared - likely re-triggered"),
})
}
}
}

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")
Expand Down
Loading