From 9c0db637b1bd1d7df5f037a27749e300e9363efd Mon Sep 17 00:00:00 2001 From: Proxima84-code Date: Wed, 30 Sep 2026 21:17:23 +0200 Subject: [PATCH] fix(github): allow redelivery and re-processing of failed webhooks (#317) --- src/github/github-webhooks.controller.spec.ts | 33 +++++--- src/github/github-webhooks.controller.ts | 16 +++- .../github-webhooks.linked-issues.spec.ts | 67 ++++++++++++---- src/github/github-webhooks.service.spec.ts | 73 +++++++++++++----- src/github/github-webhooks.service.ts | 76 +++++++++++-------- 5 files changed, 189 insertions(+), 76 deletions(-) diff --git a/src/github/github-webhooks.controller.spec.ts b/src/github/github-webhooks.controller.spec.ts index 71024a3..25afbb9 100644 --- a/src/github/github-webhooks.controller.spec.ts +++ b/src/github/github-webhooks.controller.spec.ts @@ -1,3 +1,4 @@ +import { InternalServerErrorException } from '@nestjs/common'; import { GithubWebhooksController } from './github-webhooks.controller'; import { GithubWebhooksService } from './github-webhooks.service'; import { WebhookEventStatus } from '../common/enums'; @@ -122,12 +123,7 @@ describe('GithubWebhooksController — header extraction and response shape (#31 'sha256=bad', ); - expect(handleEvent).toHaveBeenCalledWith( - 'push', - 'delivery-11', - {}, - false, - ); + expect(handleEvent).toHaveBeenCalledWith('push', 'delivery-11', {}, false); expect(result).toEqual({ received: true, eventId: 'event-11', @@ -135,12 +131,29 @@ describe('GithubWebhooksController — header extraction and response shape (#31 }); }); + it('throws InternalServerErrorException when event processing fails (#317)', async () => { + handleEvent.mockResolvedValue({ + id: 'event-failed-1', + status: WebhookEventStatus.FAILED, + error: 'escrow release failed', + }); + + await expect( + controller.handle( + { rawBody: Buffer.from('{}'), body: {} } as never, + 'pull_request', + 'delivery-fail-1', + 'sha256=abc', + ), + ).rejects.toThrow(InternalServerErrorException); + }); + it('is declared with a 202 HTTP status for the accepted delivery', () => { // @HttpCode(202) tells Nest to answer 202 instead of POST's default 201. - const httpCode = Reflect.getMetadata( - '__httpCode__', - GithubWebhooksController.prototype.handle, - ); + const handler = ( + GithubWebhooksController.prototype as unknown as Record + )['handle'] as object; + const httpCode = Reflect.getMetadata('__httpCode__', handler); expect(httpCode).toBe(202); }); }); diff --git a/src/github/github-webhooks.controller.ts b/src/github/github-webhooks.controller.ts index 5c66cbd..508b9fe 100644 --- a/src/github/github-webhooks.controller.ts +++ b/src/github/github-webhooks.controller.ts @@ -1,6 +1,14 @@ -import { Controller, Headers, HttpCode, Post, Req } from '@nestjs/common'; +import { + Controller, + Headers, + HttpCode, + InternalServerErrorException, + Post, + Req, +} from '@nestjs/common'; import { ApiExcludeController } from '@nestjs/swagger'; import type { Request } from 'express'; +import { WebhookEventStatus } from '../common/enums'; import { GithubWebhooksService } from './github-webhooks.service'; interface RawBodyRequest extends Request { @@ -36,6 +44,12 @@ export class GithubWebhooksController { signatureValid, ); + if (event.status === WebhookEventStatus.FAILED) { + throw new InternalServerErrorException( + event.error ?? 'Webhook event processing failed', + ); + } + return { received: true, eventId: event.id, status: event.status }; } } diff --git a/src/github/github-webhooks.linked-issues.spec.ts b/src/github/github-webhooks.linked-issues.spec.ts index 42d99ba..1f55348 100644 --- a/src/github/github-webhooks.linked-issues.spec.ts +++ b/src/github/github-webhooks.linked-issues.spec.ts @@ -15,7 +15,11 @@ import { BountyStatus, WebhookEventStatus } from '../common/enums'; */ describe('GithubWebhooksService — shared linked-issue processing (#316)', () => { let service: GithubWebhooksService; - let webhookEventRepo: { create: jest.Mock; save: jest.Mock }; + let webhookEventRepo: { + create: jest.Mock; + save: jest.Mock; + findOne: jest.Mock; + }; let bountyRepo: { findOne: jest.Mock }; let bountiesService: { markInReview: jest.Mock; @@ -48,8 +52,12 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = beforeEach(async () => { webhookEventRepo = { - create: jest.fn((data: Partial) => ({ id: 'event-1', ...data })), + create: jest.fn((data: Partial) => ({ + id: 'event-1', + ...data, + })), save: jest.fn((data: Partial) => Promise.resolve(data)), + findOne: jest.fn().mockResolvedValue(null), }; bountyRepo = { findOne: jest.fn() }; bountiesService = { @@ -58,7 +66,9 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = markPrClosedWithoutMerge: jest.fn().mockResolvedValue(undefined), }; syncService = { - findRepositoryByGithubId: jest.fn().mockResolvedValue({ id: 'repo-uuid-1' }), + findRepositoryByGithubId: jest + .fn() + .mockResolvedValue({ id: 'repo-uuid-1' }), findIssueByRepoAndNumber: jest.fn(), upsertIssueRecord: jest.fn(), }; @@ -70,7 +80,10 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = provide: ConfigService, useValue: { get: () => ({ webhookSecret: 'secret' }) }, }, - { provide: getRepositoryToken(WebhookEvent), useValue: webhookEventRepo }, + { + provide: getRepositoryToken(WebhookEvent), + useValue: webhookEventRepo, + }, { provide: getRepositoryToken(Bounty), useValue: bountyRepo }, { provide: BountiesService, useValue: bountiesService }, { provide: GithubSyncService, useValue: syncService }, @@ -92,7 +105,9 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = it('marks in review then releases a CLAIMED bounty', async () => { syncService.findIssueByRepoAndNumber.mockResolvedValue(linkedIssue); - bountyRepo.findOne.mockResolvedValue(bountyWithStatus(BountyStatus.CLAIMED)); + bountyRepo.findOne.mockResolvedValue( + bountyWithStatus(BountyStatus.CLAIMED), + ); await runPullRequest(mergedPayload); @@ -101,17 +116,23 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = mergedPayload.pull_request.html_url, 7, ); - expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith('bounty-1'); + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith( + 'bounty-1', + ); }); it('still releases a bounty that is not CLAIMED, without marking in review', async () => { syncService.findIssueByRepoAndNumber.mockResolvedValue(linkedIssue); - bountyRepo.findOne.mockResolvedValue(bountyWithStatus(BountyStatus.IN_REVIEW)); + bountyRepo.findOne.mockResolvedValue( + bountyWithStatus(BountyStatus.IN_REVIEW), + ); await runPullRequest(mergedPayload); expect(bountiesService.markInReview).not.toHaveBeenCalled(); - expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith('bounty-1'); + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith( + 'bounty-1', + ); }); }); @@ -120,7 +141,9 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = it('moves a CLAIMED bounty to in review', async () => { syncService.findIssueByRepoAndNumber.mockResolvedValue(linkedIssue); - bountyRepo.findOne.mockResolvedValue(bountyWithStatus(BountyStatus.CLAIMED)); + bountyRepo.findOne.mockResolvedValue( + bountyWithStatus(BountyStatus.CLAIMED), + ); await runPullRequest(openedPayload); @@ -147,16 +170,22 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = it('returns an IN_REVIEW bounty to CLAIMED', async () => { syncService.findIssueByRepoAndNumber.mockResolvedValue(linkedIssue); - bountyRepo.findOne.mockResolvedValue(bountyWithStatus(BountyStatus.IN_REVIEW)); + bountyRepo.findOne.mockResolvedValue( + bountyWithStatus(BountyStatus.IN_REVIEW), + ); await runPullRequest(closedPayload); - expect(bountiesService.markPrClosedWithoutMerge).toHaveBeenCalledWith('bounty-1'); + expect(bountiesService.markPrClosedWithoutMerge).toHaveBeenCalledWith( + 'bounty-1', + ); }); it('skips a bounty that is not IN_REVIEW', async () => { syncService.findIssueByRepoAndNumber.mockResolvedValue(linkedIssue); - bountyRepo.findOne.mockResolvedValue(bountyWithStatus(BountyStatus.CLAIMED)); + bountyRepo.findOne.mockResolvedValue( + bountyWithStatus(BountyStatus.CLAIMED), + ); await runPullRequest(closedPayload); @@ -170,7 +199,9 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = id: 'issue-1', bounty: null, }); - bountyRepo.findOne.mockResolvedValue(bountyWithStatus(BountyStatus.CLAIMED)); + bountyRepo.findOne.mockResolvedValue( + bountyWithStatus(BountyStatus.CLAIMED), + ); const event = await runPullRequest({ ...PAYLOAD_BASE, @@ -222,7 +253,9 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = it('de-duplicates an issue number referenced twice in the same PR body', async () => { syncService.findIssueByRepoAndNumber.mockResolvedValue(linkedIssue); - bountyRepo.findOne.mockResolvedValue(bountyWithStatus(BountyStatus.CLAIMED)); + bountyRepo.findOne.mockResolvedValue( + bountyWithStatus(BountyStatus.CLAIMED), + ); await runPullRequest({ ...PAYLOAD_BASE, @@ -240,7 +273,11 @@ describe('GithubWebhooksService — shared linked-issue processing (#316)', () = const event = await runPullRequest({ ...PAYLOAD_BASE, action: 'closed', - pull_request: { ...PAYLOAD_BASE.pull_request, merged: true, body: 'No links here' }, + pull_request: { + ...PAYLOAD_BASE.pull_request, + merged: true, + body: 'No links here', + }, }); expect(syncService.findIssueByRepoAndNumber).not.toHaveBeenCalled(); diff --git a/src/github/github-webhooks.service.spec.ts b/src/github/github-webhooks.service.spec.ts index 45bb6fe..f433811 100644 --- a/src/github/github-webhooks.service.spec.ts +++ b/src/github/github-webhooks.service.spec.ts @@ -10,13 +10,11 @@ import * as sigUtil from './webhook-signature.util'; describe('GithubWebhooksService', () => { let service: GithubWebhooksService; - let webhookEventRepo: { create: jest.Mock; save: jest.Mock }; let webhookEventRepo: { create: jest.Mock; save: jest.Mock; findOne: jest.Mock; }; - let issueRepo: { findOne: jest.Mock }; let bountyRepo: { findOne: jest.Mock }; let bountiesService: { markInReview: jest.Mock; @@ -264,11 +262,11 @@ describe('GithubWebhooksService', () => { byNumber: Record, ) { syncService.findIssueByRepoAndNumber.mockImplementation( - ({ where }: { where: { number: number } }) => { - const entry = byNumber[where.number]; + (_repoId: string, number: number) => { + const entry = byNumber[number]; return Promise.resolve( entry - ? { id: `issue-${where.number}`, bounty: { id: entry.bountyId } } + ? { id: `issue-${number}`, bounty: { id: entry.bountyId } } : null, ); }, @@ -637,14 +635,13 @@ describe('GithubWebhooksService', () => { describe('comma-separated closing references (#309)', () => { /** Collects every issue number the merged-PR path ends up releasing. */ function releasedNumbers(): number[] { - return issueRepo.findOne.mock.calls.map( - (call: unknown[]) => - (call[0] as { where: { number: number } }).where.number, + return syncService.findIssueByRepoAndNumber.mock.calls.map( + (call: [string, number]) => call[1], ); } it('links every issue in a comma-separated list after one keyword', async () => { - issueRepo.findOne.mockResolvedValue(null); + syncService.findIssueByRepoAndNumber.mockResolvedValue(null); await service.handleEvent( 'pull_request', 'delivery-comma-list', @@ -666,7 +663,7 @@ describe('GithubWebhooksService', () => { }); it('keeps parsing subsequent keyword lists after a comma run', async () => { - issueRepo.findOne.mockResolvedValue(null); + syncService.findIssueByRepoAndNumber.mockResolvedValue(null); await service.handleEvent( 'pull_request', 'delivery-comma-then-keyword', @@ -688,7 +685,7 @@ describe('GithubWebhooksService', () => { }); it('stops the comma run at the first token that is not a reference', async () => { - issueRepo.findOne.mockResolvedValue(null); + syncService.findIssueByRepoAndNumber.mockResolvedValue(null); await service.handleEvent( 'pull_request', 'delivery-comma-stops', @@ -710,7 +707,7 @@ describe('GithubWebhooksService', () => { }); it('skips a foreign-repo qualifier inside a comma run but keeps the rest', async () => { - issueRepo.findOne.mockResolvedValue(null); + syncService.findIssueByRepoAndNumber.mockResolvedValue(null); await service.handleEvent( 'pull_request', 'delivery-comma-foreign', @@ -732,11 +729,11 @@ describe('GithubWebhooksService', () => { }); it('marks a comma-separated bounty in review when the PR is opened', async () => { - issueRepo.findOne.mockImplementation( - ({ where }: { where: { number: number } }) => + syncService.findIssueByRepoAndNumber.mockImplementation( + (_repoId: string, number: number) => Promise.resolve({ - id: `issue-${where.number}`, - bounty: { id: `bounty-${where.number}` }, + id: `issue-${number}`, + bounty: { id: `bounty-${number}` }, }), ); bountyRepo.findOne.mockImplementation( @@ -782,10 +779,10 @@ describe('GithubWebhooksService', () => { describe('pull_request edited events (#310)', () => { it('moves a bounty to in_review when a closing keyword is added after opening', async () => { - issueRepo.findOne.mockImplementation( - ({ where }: { where: { number: number } }) => + syncService.findIssueByRepoAndNumber.mockImplementation( + (_repoId: string, number: number) => Promise.resolve({ - id: `issue-${where.number}`, + id: `issue-${number}`, bounty: { id: 'bounty-42' }, }), ); @@ -820,7 +817,7 @@ describe('GithubWebhooksService', () => { }); it('leaves a bounty already in_review untouched on a later body edit', async () => { - issueRepo.findOne.mockResolvedValue({ + syncService.findIssueByRepoAndNumber.mockResolvedValue({ id: 'issue-42', bounty: { id: 'bounty-42' }, }); @@ -869,6 +866,9 @@ describe('GithubWebhooksService', () => { expect(bountiesService.markInReview).not.toHaveBeenCalled(); expect(bountiesService.markMergedAndRelease).not.toHaveBeenCalled(); + }); + }); + // #308: a redelivered X-GitHub-Delivery used to hit the unique constraint // on webhook_events.deliveryId and escape handleEvent as a 500, so every // redelivery of that event failed forever. @@ -885,6 +885,39 @@ describe('GithubWebhooksService', () => { repository: { id: 1, full_name: 'a/b' }, }; + it('re-processes a delivery if its previous attempt failed (#317)', async () => { + const existingFailedEvent = { + id: 'event-previously-failed', + deliveryId: 'delivery-retry-1', + status: WebhookEventStatus.FAILED, + error: 'transient failure', + }; + webhookEventRepo.findOne.mockResolvedValueOnce(existingFailedEvent); + + syncService.findIssueByRepoAndNumber.mockResolvedValue({ + id: 'issue-1', + bounty: { id: 'bounty-1' }, + }); + bountyRepo.findOne.mockResolvedValue({ + id: 'bounty-1', + status: 'claimed', + }); + + const event = await service.handleEvent( + 'pull_request', + 'delivery-retry-1', + payload, + true, + ); + + expect(bountiesService.markMergedAndRelease).toHaveBeenCalledWith( + 'bounty-1', + ); + expect(event.id).toBe('event-previously-failed'); + expect(event.status).toBe(WebhookEventStatus.PROCESSED); + expect(event.error).toBeNull(); + }); + it('returns the stored event without re-running business logic', async () => { webhookEventRepo.findOne.mockResolvedValueOnce({ id: 'event-existing', diff --git a/src/github/github-webhooks.service.ts b/src/github/github-webhooks.service.ts index 9221cdd..b1b90d3 100644 --- a/src/github/github-webhooks.service.ts +++ b/src/github/github-webhooks.service.ts @@ -113,9 +113,10 @@ export class GithubWebhooksService { // threw a QueryFailedError that escaped handleEvent entirely and turned // every redelivery into a bare 500, which GitHub then keeps retrying // (#308). Short-circuit on the stored row instead. + let existing: WebhookEvent | null = null; if (deliveryId) { - const existing = await this.findByDeliveryId(deliveryId); - if (existing) { + existing = await this.findByDeliveryId(deliveryId); + if (existing && existing.status !== WebhookEventStatus.FAILED) { this.logger.warn( `Ignoring duplicate webhook delivery ${deliveryId} — already recorded ` + `(status: ${existing.status})`, @@ -125,32 +126,46 @@ export class GithubWebhooksService { } let event: WebhookEvent; - try { - event = await this.webhookEventRepo.save( - this.webhookEventRepo.create({ - eventType, - deliveryId: deliveryId ?? null, - payload, - signatureValid, - status: signatureValid - ? WebhookEventStatus.RECEIVED - : WebhookEventStatus.IGNORED, - }), + if (existing) { + this.logger.log( + `Retrying previously failed webhook delivery ${deliveryId}`, ); - } catch (err) { - // Two deliveries racing past the lookup above can still collide on the - // unique index; the loser treats that as the benign duplicate it is - // rather than a crash. - if (deliveryId && this.isUniqueViolation(err)) { - const raced = await this.findByDeliveryId(deliveryId); - if (raced) { - this.logger.warn( - `Ignoring concurrently delivered webhook ${deliveryId} — already recorded`, - ); - return raced; + existing.payload = payload; + existing.signatureValid = signatureValid; + existing.status = signatureValid + ? WebhookEventStatus.RECEIVED + : WebhookEventStatus.IGNORED; + existing.error = null; + existing.processedAt = null; + event = await this.webhookEventRepo.save(existing); + } else { + try { + event = await this.webhookEventRepo.save( + this.webhookEventRepo.create({ + eventType, + deliveryId: deliveryId ?? null, + payload, + signatureValid, + status: signatureValid + ? WebhookEventStatus.RECEIVED + : WebhookEventStatus.IGNORED, + }), + ); + } catch (err) { + // Two deliveries racing past the lookup above can still collide on the + // unique index; the loser treats that as the benign duplicate it is + // rather than a crash. + if (deliveryId && this.isUniqueViolation(err)) { + const raced = await this.findByDeliveryId(deliveryId); + if (raced) { + this.logger.warn( + `Ignoring concurrently delivered webhook ${deliveryId} — already recorded`, + ); + return raced; + } } + throw err; } - throw err; } if (!signatureValid) { @@ -212,9 +227,8 @@ export class GithubWebhooksService { if (code === '23505') { return true; } - const driverCode = ( - err as { driverError?: { code?: unknown } } | null - )?.driverError?.code; + const driverCode = (err as { driverError?: { code?: unknown } } | null) + ?.driverError?.code; if (driverCode === '23505') { return true; } @@ -459,7 +473,8 @@ export class GithubWebhooksService { repoFullName: string, ): number[] { const isSameRepo = (repoQualifier?: string) => - !repoQualifier || repoQualifier.toLowerCase() === repoFullName.toLowerCase(); + !repoQualifier || + repoQualifier.toLowerCase() === repoFullName.toLowerCase(); const matches = [...body.matchAll(CLOSING_KEYWORD_RE)]; const numbers: number[] = []; @@ -543,7 +558,8 @@ export class GithubWebhooksService { return this.processLinkedIssues(issueNumbers, payload, { requiredStatus: BountyStatus.IN_REVIEW, - action: (bounty) => this.bountiesService.markPrClosedWithoutMerge(bounty.id), + action: (bounty) => + this.bountiesService.markPrClosedWithoutMerge(bounty.id), }); } }