Skip to content

Commit 8465098

Browse files
committed
fix: correct webhook log status field and close a logging gap
logWebhookAction hardcoded status: 'SUCCESS' on every WebhookLog row, including calls made with action: 'FAILED' and failed test deliveries - every failure in the audit trail was mislabeled as successful, which would silently break any dashboard or filter built on status. - Add an explicit status param to logWebhookAction (default SUCCESS); pass FAILURE at the three call sites that log a failure - Add a RETRY_SCHEDULED/FAILURE log for a failed attempt that still has retries remaining - previously only the terminal outcomes (delivered or exhausted) were logged, so the audit trail skipped every attempt in between - Add a RETRY_INITIATED log to retryWebhookEvent - a manual retry previously left no record of itself - Extend 5 existing tests with assertions on the actual status/action values written, not just that logWebhookAction was called 71/71 tests passing; tsc error count unchanged (760, all pre-existing)
1 parent d57c47f commit 8465098

2 files changed

Lines changed: 43 additions & 6 deletions

File tree

backend/services/WebhookService.ts

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,12 @@ export class WebhookService {
362362
// survive a restart, silently dropping the retry. nextRetry is
363363
// persisted instead, and WebhookQueueService's interval drains
364364
// due events via processPendingRetries() below.
365+
await this.logWebhookAction(
366+
webhook.id,
367+
'RETRY_SCHEDULED',
368+
`Event ${payload.eventType} delivery failed on attempt ${attemptNumber + 1}, retry due in ${nextRetryDelay}s`,
369+
'FAILURE'
370+
);
365371
logger.info(`Webhook delivery failed. Retry due for event ${event.id} in ${nextRetryDelay}s`);
366372
} else {
367373
// Max retries exceeded
@@ -374,7 +380,12 @@ export class WebhookService {
374380
},
375381
});
376382

377-
await this.logWebhookAction(webhook.id, 'FAILED', `Event ${payload.eventType} failed after ${attemptNumber + 1} attempts`);
383+
await this.logWebhookAction(
384+
webhook.id,
385+
'FAILED',
386+
`Event ${payload.eventType} failed after ${attemptNumber + 1} attempts`,
387+
'FAILURE'
388+
);
378389
logger.error(`Webhook delivery failed permanently for event ${event.id} after ${attemptNumber + 1} attempts`);
379390
}
380391
}
@@ -456,7 +467,8 @@ export class WebhookService {
456467
await this.logWebhookAction(
457468
event.webhookId,
458469
'FAILED',
459-
'Webhook was deleted or deactivated before its scheduled retry could run'
470+
'Webhook was deleted or deactivated before its scheduled retry could run',
471+
'FAILURE'
460472
);
461473
continue;
462474
}
@@ -541,14 +553,19 @@ export class WebhookService {
541553
/**
542554
* Log webhook action
543555
*/
544-
private async logWebhookAction(webhookId: string, action: string, details?: string): Promise<void> {
556+
private async logWebhookAction(
557+
webhookId: string,
558+
action: string,
559+
details?: string,
560+
status: 'SUCCESS' | 'FAILURE' = 'SUCCESS'
561+
): Promise<void> {
545562
try {
546563
await prisma.webhookLog.create({
547564
data: {
548565
webhookId,
549566
action,
550567
details,
551-
status: 'SUCCESS',
568+
status,
552569
},
553570
});
554571
} catch (error) {
@@ -631,7 +648,7 @@ export class WebhookService {
631648
const responseTime = Date.now() - startTime;
632649
const axiosError = error as AxiosError;
633650

634-
await this.logWebhookAction(webhook.id, 'TESTED', `Test delivery failed: ${axiosError.message}`);
651+
await this.logWebhookAction(webhook.id, 'TESTED', `Test delivery failed: ${axiosError.message}`, 'FAILURE');
635652

636653
return {
637654
success: false,
@@ -713,6 +730,7 @@ export class WebhookService {
713730
},
714731
});
715732

733+
await this.logWebhookAction(webhook.id, 'RETRY_INITIATED', `Manual retry triggered for event ${event.eventType}`);
716734
await this.attemptWebhookDelivery(webhook, event, payload, 0);
717735
logger.info(`Webhook event retried: ${eventId}`);
718736
} catch (error) {

backend/tests/webhook/WebhookService.test.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,7 @@ describe('WebhookService', () => {
317317
mockPrisma.webhookEvent.create.mockResolvedValue(mockEvent as any);
318318
mockPrisma.webhookAttempt.create.mockResolvedValue({} as any);
319319
mockPrisma.webhookEvent.update.mockResolvedValue({} as any);
320+
mockPrisma.webhookLog.create.mockResolvedValue({} as any);
320321
mockedAxios.post.mockRejectedValue({
321322
message: 'Request failed with status code 500',
322323
response: { status: 500, data: 'Server Error' },
@@ -341,6 +342,12 @@ describe('WebhookService', () => {
341342
expect(mockPrisma.webhookEvent.update).not.toHaveBeenCalledWith(
342343
expect.objectContaining({ data: expect.objectContaining({ status: 'FAILED' }) })
343344
);
345+
// A failed attempt with retries remaining should still produce a
346+
// persisted, correctly-statused log entry — not just the terminal
347+
// TRIGGERED/FAILED outcomes.
348+
expect(mockPrisma.webhookLog.create).toHaveBeenCalledWith({
349+
data: expect.objectContaining({ action: 'RETRY_SCHEDULED', status: 'FAILURE' }),
350+
});
344351
});
345352

346353
it('should treat a network timeout the same as any other delivery failure', async () => {
@@ -512,6 +519,12 @@ describe('WebhookService', () => {
512519
where: { id: 'event-1' },
513520
data: expect.objectContaining({ status: 'FAILED', attempts: 2 }),
514521
});
522+
// logWebhookAction used to hardcode status: 'SUCCESS' for every log
523+
// entry, including this one — a permanently failed delivery must be
524+
// logged with status FAILURE, not SUCCESS.
525+
expect(mockPrisma.webhookLog.create).toHaveBeenCalledWith({
526+
data: expect.objectContaining({ action: 'FAILED', status: 'FAILURE' }),
527+
});
515528
});
516529

517530
it('should mark the event FAILED if its webhook was deleted or deactivated before the retry ran', async () => {
@@ -672,6 +685,9 @@ describe('WebhookService', () => {
672685

673686
expect(result.success).toBe(false);
674687
expect(result.error).toBe('Network error');
688+
expect(mockPrisma.webhookLog.create).toHaveBeenCalledWith({
689+
data: expect.objectContaining({ action: 'TESTED', status: 'FAILURE' }),
690+
});
675691
});
676692

677693
it('should throw error for non-existent webhook', async () => {
@@ -759,7 +775,7 @@ describe('WebhookService', () => {
759775
mockPrisma.webhook.findUnique.mockResolvedValue(mockWebhook as any);
760776
mockPrisma.webhookEvent.update.mockResolvedValue({} as any);
761777
mockPrisma.webhookAttempt.create.mockResolvedValue({} as any);
762-
mockPrisma.webhookEvent.update.mockResolvedValue({} as any);
778+
mockPrisma.webhookLog.create.mockResolvedValue({} as any);
763779

764780
await webhookService.retryWebhookEvent(eventId);
765781

@@ -770,6 +786,9 @@ describe('WebhookService', () => {
770786
attempts: 0,
771787
},
772788
});
789+
expect(mockPrisma.webhookLog.create).toHaveBeenCalledWith({
790+
data: expect.objectContaining({ action: 'RETRY_INITIATED', status: 'SUCCESS' }),
791+
});
773792
});
774793

775794
it('should throw error for non-existent event', async () => {

0 commit comments

Comments
 (0)