From bcddec06a48e56017a657896ba8ade78bfab3470 Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Mon, 22 Jun 2026 17:53:28 -0700 Subject: [PATCH 1/3] fix: pass triggerRef in PR synchronize handler to coalesce with push 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::: sync: resolve:: 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. --- src/server/services/__tests__/github.test.ts | 1 + src/server/services/github.ts | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/server/services/__tests__/github.test.ts b/src/server/services/__tests__/github.test.ts index f0db616..898e8ab 100644 --- a/src/server/services/__tests__/github.test.ts +++ b/src/server/services/__tests__/github.test.ts @@ -350,6 +350,7 @@ describe('Github Service - handlePullRequestHook', () => { expect(mockDb.models.Build.findOne).toHaveBeenCalledWith({ pullRequestId: 1 }); expect(mockDb.services.BuildService.resolveAndDeployBuildQueue.add).toHaveBeenCalledWith('resolve-deploy', { buildId: 10, + triggerRef: 'latest-commit', }); }); diff --git a/src/server/services/github.ts b/src/server/services/github.ts index 2de6de1..7959f7b 100644 --- a/src/server/services/github.ts +++ b/src/server/services/github.ts @@ -304,6 +304,11 @@ export default class GithubService extends Service { ); await this.db.services.BuildService.enqueueResolveAndDeployBuild({ buildId: build.id, + // Pass the same commit SHA the concurrent push event uses as triggerRef so both events produce the same + // dedupe key and coalesce to one resolveAndDeployBuild run. Without this the push path (added in PR #218) + // carries a triggerRef while the synchronize path does not, giving them different keys and causing two + // concurrent runs for every PR branch push. + ...(branchSha ? { triggerRef: branchSha } : {}), ...extractContextForQueue(), }); } From f23acc291df527cfc5168358422ff7c93da69745 Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Mon, 22 Jun 2026 18:36:15 -0700 Subject: [PATCH 2/3] docs: correct inline comment on synchronize triggerRef to not overstate 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. --- src/server/services/github.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/server/services/github.ts b/src/server/services/github.ts index 7959f7b..e5a174d 100644 --- a/src/server/services/github.ts +++ b/src/server/services/github.ts @@ -304,10 +304,11 @@ export default class GithubService extends Service { ); await this.db.services.BuildService.enqueueResolveAndDeployBuild({ buildId: build.id, - // Pass the same commit SHA the concurrent push event uses as triggerRef so both events produce the same - // dedupe key and coalesce to one resolveAndDeployBuild run. Without this the push path (added in PR #218) - // carries a triggerRef while the synchronize path does not, giving them different keys and causing two - // concurrent runs for every PR branch push. + // Mirror the triggerRef strategy from the push handler (PR #218): include the commit SHA in the dedupe key + // so back-to-back synchronize events for distinct commits each get their own key instead of collapsing onto + // the in-flight key of an earlier commit. Note: push and synchronize fingerprints still differ when the push + // path passes githubRepositoryId (the normal case), so the two events run concurrently; they only coalesce + // when push also omits githubRepositoryId (e.g. the failed-deploy rebuild path). ...(branchSha ? { triggerRef: branchSha } : {}), ...extractContextForQueue(), }); From 4f0bd77cee99f8b43c84d5c37629641b0144b5bf Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Mon, 22 Jun 2026 20:41:11 -0700 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20remove=20isSynchronized=20deploy=20h?= =?UTF-8?q?andler=20=E2=80=94=20redundant=20with=20push=20handler?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/server/services/__tests__/github.test.ts | 28 ---------------- src/server/services/github.ts | 34 -------------------- 2 files changed, 62 deletions(-) diff --git a/src/server/services/__tests__/github.test.ts b/src/server/services/__tests__/github.test.ts index 898e8ab..72563a4 100644 --- a/src/server/services/__tests__/github.test.ts +++ b/src/server/services/__tests__/github.test.ts @@ -326,34 +326,6 @@ describe('Github Service - handlePullRequestHook', () => { expect(mockDb.services.LabelService.labelQueue.add).not.toHaveBeenCalled(); }); - test('queues a build when an open deployed PR receives a synchronize event', async () => { - mockHasDeployLabel.mockResolvedValue(true); - mockEnableKillSwitch.mockResolvedValue(false); - - const mockPullRequest = createMockPullRequest({ - deployOnUpdate: true, - latestCommit: 'previous-commit', - }); - mockDb.services.PullRequest.findOrCreatePullRequest.mockResolvedValue(mockPullRequest); - - await githubService.handlePullRequestHook( - createMockPullRequestEvent({ - action: 'synchronize', - labels: [{ name: 'lifecycle-deploy!' }], - branchSha: 'latest-commit', - }) - ); - - expect(mockGetYamlFileContent).not.toHaveBeenCalled(); - expect(mockDb.services.BuildService.createBuildAndDeploys).not.toHaveBeenCalled(); - expect(mockPullRequest.__patch).toHaveBeenCalledWith({ latestCommit: 'latest-commit' }); - expect(mockDb.models.Build.findOne).toHaveBeenCalledWith({ pullRequestId: 1 }); - expect(mockDb.services.BuildService.resolveAndDeployBuildQueue.add).toHaveBeenCalledWith('resolve-deploy', { - buildId: 10, - triggerRef: 'latest-commit', - }); - }); - test('keeps the existing label sync flow for unlabeled autoDeploy PRs', async () => { mockGetYamlFileContent.mockResolvedValue({ environment: { autoDeploy: true } }); mockHasDeployLabel.mockResolvedValue(false); diff --git a/src/server/services/github.ts b/src/server/services/github.ts index e5a174d..1265bbb 100644 --- a/src/server/services/github.ts +++ b/src/server/services/github.ts @@ -155,7 +155,6 @@ export default class GithubService extends Service { action as GithubPullRequestActions ); const isClosed = action === GithubPullRequestActions.CLOSED; - const isSynchronized = action === GithubPullRequestActions.SYNCHRONIZE; let lifecycleConfig = {} as LifecycleYamlConfigOptions; let pullRequest: PullRequest, repository: Repository | undefined, build: Build; @@ -279,39 +278,6 @@ export default class GithubService extends Service { labels: labels.map((l) => l.name), ...extractContextForQueue(), }); - } else if (isSynchronized) { - if (branchSha && latestCommit !== branchSha) { - await pullRequest.$query().patch({ latestCommit: branchSha }); - } - - if (status !== PullRequestStatus.OPEN || pullRequestState?.deployOnUpdate !== true) { - getLogger({}).info( - `PR sync decision: repo=${fullName} branch=${branch} pullRequestId=${pullRequestId} decision=no-deploy deployOnUpdate=${pullRequestState?.deployOnUpdate}` - ); - return; - } - - build = await this.db.models.Build.findOne({ - pullRequestId, - }); - if (!build) { - getLogger({}).warn(`Build: not found for synchronized PR repo=${fullName}/${branch}`); - return; - } - - getLogger({}).info( - `PR sync decision: repo=${fullName} branch=${branch} pullRequestId=${pullRequestId} decision=queue-build` - ); - await this.db.services.BuildService.enqueueResolveAndDeployBuild({ - buildId: build.id, - // Mirror the triggerRef strategy from the push handler (PR #218): include the commit SHA in the dedupe key - // so back-to-back synchronize events for distinct commits each get their own key instead of collapsing onto - // the in-flight key of an earlier commit. Note: push and synchronize fingerprints still differ when the push - // path passes githubRepositoryId (the normal case), so the two events run concurrently; they only coalesce - // when push also omits githubRepositoryId (e.g. the failed-deploy rebuild path). - ...(branchSha ? { triggerRef: branchSha } : {}), - ...extractContextForQueue(), - }); } } catch (error) { getLogger().fatal({ error }, `Github: PR event handling failed repo=${fullName} branch=${branch}`);