fix: remove redundant isSynchronized deploy handler to prevent concurrent builds#219
Merged
Merged
Conversation
…event When a user pushes to a PR branch, GitHub fires both a push event and a pull_request synchronize event for the same commit. PR #218 added triggerRef (the commit SHA) to the push handler's enqueueResolveAndDeployBuild call so that back-to-back distinct commits are not silently coalesced. However, the synchronize handler was not updated, so it still enqueues with no triggerRef. The two events now produce different dedupe keys: push: resolve:<buildId>:<fingerprint>:<commitSha> sync: resolve:<buildId>:<fingerprint> Both jobs run concurrently, race for runUUID ownership, and if the losing run encounters an error it patches the build status to ERROR even though all services are healthy. Fix: pass the same branchSha as triggerRef in the synchronize handler so both events produce identical dedupe keys and coalesce to a single resolveAndDeployBuild run.
…te coalescing The previous comment claimed push and synchronize produce the same dedupe key and coalesce, which is only true when the push path omits githubRepositoryId (e.g. the failed-deploy rebuild path). In the normal case, push passes githubRepositoryId so its fingerprint differs from the synchronize path even with matching triggerRef. Update the comment to accurately describe the fix as preventing back-to-back synchronize events for distinct commits from collapsing.
The pull_request synchronize handler added in PR #205 enqueues resolveAndDeployBuild for every PR push, but the push event handler already covers the same work. deployable.defaultBranchName stores the literal yaml value (e.g. "@Branch", "main"), never the resolved PR branch name, so shouldBuild is always true for PR environments — the push handler fires for every service without exception. Having both handlers enqueue for the same commit causes two concurrent resolveAndDeployBuild runs that race for runUUID ownership. If the losing run hits any error, recordBuildFailure force-patches runUUID and writes ERROR status even though the winning run's services are healthy. Remove the synchronize block and its now-unused isSynchronized variable. Remove the corresponding test that asserted the redundant enqueue.
vigneshrajsb
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PR #205 added a
pull_request synchronizehandler that callsenqueueResolveAndDeployBuildwhenever a PR branch receives new commits. The intent was to ensure PR environments rebuild on push, but thepushevent handler already covers this completely.deployable.defaultBranchNamestores the literal yaml value (@branch,main, etc.) — never the resolved PR branch name. This meansshouldBuildin the push handler is alwaystruefor PR environments:Every service in the environment is already rebuilt by the push handler. The synchronize handler was redundant.
Impact
Having both handlers enqueue for the same commit produces two concurrent
resolveAndDeployBuildruns that race forrunUUIDownership. If the losing run encounters any error,recordBuildFailureforce-patchesrunUUIDand writesERRORstatus — even though the winning run's services are healthy. This is the root cause of PR environments appearing failed in the UI when all services are actually running.Fix
Remove the
isSynchronizedblock fromhandlePullRequestHookand its now-unusedisSynchronizedvariable. Remove the corresponding test.Test plan
resolveAndDeployBuildrun fires (singlerunUUIDin Groundcover logs, no duplicate K8s build jobs)