Skip to content

Commit a1b7e22

Browse files
CedarichCedarich
authored
Merge pull request #618 from boalambo/fix/605-prevent-duplicate-notifications
fix(backend): prevent duplicate escrow notifications
2 parents 12e99c6 + 1a93e15 commit a1b7e22

10 files changed

Lines changed: 883 additions & 81 deletions

‎apps/backend/src/migrations/1780900000000-AddIdempotencyKeyToNotification.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import { MigrationInterface, QueryRunner } from 'typeorm';
2+
3+
export class AddIdempotencyKeyToNotification1780900000000 implements MigrationInterface {
4+
name = 'AddIdempotencyKeyToNotification1780900000000';
5+
6+
public async up(queryRunner: QueryRunner): Promise<void> {
7+
await queryRunner.query(
8+
`ALTER TABLE "notification" ADD COLUMN "idempotencyKey" varchar`,
9+
);
10+
await queryRunner.query(
11+
`CREATE INDEX "idx_notification_idempotency_key" ON "notification" ("idempotencyKey")`,
12+
);
13+
}
14+
15+
public async down(queryRunner: QueryRunner): Promise<void> {
16+
await queryRunner.query(
17+
`DROP INDEX IF EXISTS "idx_notification_idempotency_key"`,
18+
);
19+
await queryRunner.query(
20+
`ALTER TABLE "notification" DROP COLUMN "idempotencyKey"`,
21+
);
22+
}
23+
}

‎apps/backend/src/migrations/1781000000000-MakeNotificationIdempotencyKeyUnique.ts‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
import { MigrationInterface, QueryRunner } from 'typeorm';
2+
3+
export class MakeNotificationIdempotencyKeyUnique1781000000000
4+
implements MigrationInterface
5+
{
6+
name = 'MakeNotificationIdempotencyKeyUnique1781000000000';
7+
8+
public async up(queryRunner: QueryRunner): Promise<void> {
9+
await queryRunner.query(
10+
`DROP INDEX IF EXISTS "idx_notification_idempotency_key"`,
11+
);
12+
await queryRunner.query(
13+
`CREATE UNIQUE INDEX "idx_notification_idempotency_key" ON "notification" ("idempotencyKey") WHERE "idempotencyKey" IS NOT NULL`,
14+
);
15+
}
16+
17+
public async down(queryRunner: QueryRunner): Promise<void> {
18+
await queryRunner.query(
19+
`DROP INDEX IF EXISTS "idx_notification_idempotency_key"`,
20+
);
21+
await queryRunner.query(
22+
`CREATE INDEX "idx_notification_idempotency_key" ON "notification" ("idempotencyKey")`,
23+
);
24+
}
25+
}

‎apps/backend/src/modules/escrow/services/escrow.service.spec.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,9 @@ describe('EscrowService', () => {
331331
'user-123',
332332
NotificationEventType.PARTY_ACCEPTED,
333333
expect.objectContaining({ escrowId: 'escrow-123' }),
334+
expect.stringMatching(
335+
/^PARTY_ACCEPTED:escrow-123:user-456:user-123:\d+$/,
336+
),
334337
);
335338
});
336339

‎apps/backend/src/modules/escrow/services/escrow.service.ts‎

Lines changed: 84 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,21 @@ export class EscrowService {
7272
private readonly notificationService: NotificationService,
7373
) {}
7474

75+
/**
76+
* Generate a unique idempotency key for notification deduplication
77+
* Format: eventType:escrowId:userId:timestampBucket
78+
* Timestamp bucket is 5-minute window to handle near-simultaneous events
79+
*/
80+
private generateIdempotencyKey(
81+
eventType: NotificationEventType,
82+
escrowId: string,
83+
userId: string,
84+
actorId: string,
85+
): string {
86+
const timestampBucket = Math.floor(Date.now() / (5 * 60 * 1000)); // 5-minute bucket
87+
return `${eventType}:${escrowId}:${actorId}:${userId}:${timestampBucket}`;
88+
}
89+
7590
async create(
7691
dto: CreateEscrowDto,
7792
creatorId: string,
@@ -119,6 +134,12 @@ export class EscrowService {
119134
invitedBy: creatorId,
120135
email: invitedUser?.email ?? undefined,
121136
},
137+
this.generateIdempotencyKey(
138+
NotificationEventType.PARTY_INVITED,
139+
savedEscrow.id,
140+
partyDto.userId,
141+
creatorId,
142+
),
122143
)
123144
.catch(() => undefined);
124145
}
@@ -568,6 +589,15 @@ export class EscrowService {
568589
{ stellarTxHash },
569590
ipAddress,
570591
);
592+
593+
await this.notifyEscrowParticipants(
594+
await this.findOne(id),
595+
NotificationEventType.ESCROW_FUNDED,
596+
{ escrowId: id, stellarTxHash },
597+
userId,
598+
null,
599+
);
600+
571601
await this.webhookService.dispatchEvent('escrow.funded', {
572602
escrowId: id,
573603
stellarTxHash,
@@ -1003,7 +1033,7 @@ export class EscrowService {
10031033
});
10041034

10051035
// Notify the other escrow participants (fire-and-forget)
1006-
await this.notifyDisputeParticipants(
1036+
await this.notifyEscrowParticipants(
10071037
escrow,
10081038
NotificationEventType.DISPUTE_RAISED,
10091039
{
@@ -1123,7 +1153,7 @@ export class EscrowService {
11231153
});
11241154

11251155
// Notify the other escrow participants (fire-and-forget)
1126-
await this.notifyDisputeParticipants(
1156+
await this.notifyEscrowParticipants(
11271157
escrow,
11281158
NotificationEventType.DISPUTE_RESOLVED,
11291159
{
@@ -1142,15 +1172,16 @@ export class EscrowService {
11421172
}
11431173

11441174
/**
1145-
* Dispatch a dispute notification to every escrow participant
1175+
* Dispatch a notification to every escrow participant
11461176
* (creator + parties), excluding the acting user. Failures must not
11471177
* block the dispute workflow.
11481178
*/
1149-
private async notifyDisputeParticipants(
1179+
private async notifyEscrowParticipants(
11501180
escrow: Escrow,
11511181
eventType: NotificationEventType,
11521182
payload: Record<string, unknown>,
1153-
excludeUserId?: string,
1183+
actorId: string,
1184+
excludeUserId: string | null = actorId,
11541185
): Promise<void> {
11551186
const recipientIds = new Set<string>();
11561187
if (escrow.creatorId) recipientIds.add(escrow.creatorId);
@@ -1159,15 +1190,34 @@ export class EscrowService {
11591190
}
11601191
if (excludeUserId) recipientIds.delete(excludeUserId);
11611192

1193+
if (eventType === NotificationEventType.DISPUTE_RAISED) {
1194+
const admins = await this.userRepository.find({
1195+
where: [{ role: UserRole.ADMIN }, { role: UserRole.SUPER_ADMIN }],
1196+
});
1197+
for (const admin of admins) {
1198+
if (admin.id !== excludeUserId) recipientIds.add(admin.id);
1199+
}
1200+
}
1201+
11621202
for (const recipientId of recipientIds) {
11631203
const recipient = await this.userRepository.findOne({
11641204
where: { id: recipientId },
11651205
});
1166-
this.notificationService
1167-
.handleEscrowEvent(recipientId, eventType, {
1168-
...payload,
1169-
email: recipient?.email ?? undefined,
1170-
})
1206+
await this.notificationService
1207+
.handleEscrowEvent(
1208+
recipientId,
1209+
eventType,
1210+
{
1211+
...payload,
1212+
email: recipient?.email ?? undefined,
1213+
},
1214+
this.generateIdempotencyKey(
1215+
eventType,
1216+
escrow.id,
1217+
recipientId,
1218+
actorId,
1219+
),
1220+
)
11711221
.catch(() => undefined);
11721222
}
11731223
}
@@ -1343,6 +1393,12 @@ export class EscrowService {
13431393
acceptedByUserId: userId,
13441394
email: acceptedUser?.email ?? undefined,
13451395
},
1396+
this.generateIdempotencyKey(
1397+
NotificationEventType.PARTY_ACCEPTED,
1398+
escrowId,
1399+
escrow.creatorId,
1400+
userId,
1401+
),
13461402
)
13471403
.catch(() => undefined);
13481404
}
@@ -1399,6 +1455,12 @@ export class EscrowService {
13991455
rejectedByUserId: userId,
14001456
email: rejectedUser?.email ?? undefined,
14011457
},
1458+
this.generateIdempotencyKey(
1459+
NotificationEventType.PARTY_REJECTED,
1460+
escrowId,
1461+
escrow.creatorId,
1462+
userId,
1463+
),
14021464
)
14031465
.catch(() => undefined);
14041466
}
@@ -1612,6 +1674,18 @@ export class EscrowService {
16121674
amount: releaseAmount,
16131675
});
16141676

1677+
await this.notifyEscrowParticipants(
1678+
escrow,
1679+
NotificationEventType.MILESTONE_RELEASED,
1680+
{
1681+
escrowId,
1682+
escrowTitle: escrow.title,
1683+
conditionId,
1684+
amount: releaseAmount,
1685+
},
1686+
userId,
1687+
);
1688+
16151689
return this.findOne(escrowId);
16161690
}
16171691

‎apps/backend/src/modules/stellar/services/stellar-event-listener.service.spec.ts‎

Lines changed: 4 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -314,31 +314,14 @@ describe('StellarEventListenerService', () => {
314314
);
315315
});
316316

317-
it('should create notifications for buyer and seller', async () => {
317+
it('should not create notifications for a milestone event', async () => {
318318
escrowRepo.findOne.mockResolvedValue({
319319
...baseEscrow,
320320
conditions: [...baseConditions.map((c) => ({ ...c }))],
321321
});
322-
partyRepo.find.mockResolvedValue([
323-
{ userId: 'buyer-user-id', role: PartyRole.BUYER },
324-
{ userId: 'seller-user-id', role: PartyRole.SELLER },
325-
{ userId: 'arb-user-id', role: PartyRole.ARBITRATOR },
326-
]);
327-
328322
await (service as any).handleMilestoneReleased(mockEvent);
329323

330-
// Buyer and seller should get notifications, but not arbitrator
331-
expect(notificationService.handleEscrowEvent).toHaveBeenCalledTimes(2);
332-
expect(notificationService.handleEscrowEvent).toHaveBeenCalledWith(
333-
'buyer-user-id',
334-
'MILESTONE_RELEASED',
335-
expect.objectContaining({ escrowId: 'escrow-1' }),
336-
);
337-
expect(notificationService.handleEscrowEvent).toHaveBeenCalledWith(
338-
'seller-user-id',
339-
'MILESTONE_RELEASED',
340-
expect.objectContaining({ escrowId: 'escrow-1' }),
341-
);
324+
expect(notificationService.handleEscrowEvent).not.toHaveBeenCalled();
342325
});
343326

344327
it('should be idempotent — skip if milestone already released', async () => {
@@ -467,26 +450,14 @@ describe('StellarEventListenerService', () => {
467450
expect(conditionRepo.save).toHaveBeenCalled();
468451
});
469452

470-
it('should not throw if notification creation fails', async () => {
453+
it('should not create notifications while updating the milestone', async () => {
471454
escrowRepo.findOne.mockResolvedValue({
472455
...baseEscrow,
473456
conditions: [...baseConditions.map((c) => ({ ...c }))],
474457
});
475-
partyRepo.find.mockResolvedValue([
476-
{ userId: 'buyer-user-id', role: PartyRole.BUYER },
477-
]);
478-
notificationService.handleEscrowEvent.mockRejectedValue(
479-
new Error('Notification error'),
480-
);
481-
const errorSpy = jest.spyOn((service as any).logger, 'error');
482-
483-
// Should not throw
484458
await (service as any).handleMilestoneReleased(mockEvent);
485459

486-
expect(errorSpy).toHaveBeenCalledWith(
487-
'Failed to create milestone release notifications',
488-
expect.any(Error),
489-
);
460+
expect(notificationService.handleEscrowEvent).not.toHaveBeenCalled();
490461
// DB changes should still be saved
491462
expect(conditionRepo.save).toHaveBeenCalled();
492463
});

‎apps/backend/src/modules/stellar/services/stellar-event-listener.service.ts‎

Lines changed: 1 addition & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,11 @@ import {
2828
EscrowEvent,
2929
EscrowEventType,
3030
} from '../../escrow/entities/escrow-event.entity';
31-
import { Party, PartyRole } from '../../escrow/entities/party.entity';
31+
import { Party } from '../../escrow/entities/party.entity';
3232
import { SorobanClientService } from '../../../services/stellar/soroban-client.service';
3333
import { ConsistencyCheckerService } from '../../admin/services/consistency-checker.service';
3434
import { AllowedAsset } from '../../assets/entities/allowed-asset.entity';
3535
import { EscrowGateway } from '../../../gateways/escrow.gateway';
36-
import { NotificationService } from '../../../notifications/notifications.service';
37-
import { NotificationEventType } from '../../../notifications/enums/notification-event.enum';
3836

3937
@Injectable()
4038
export class StellarEventListenerService
@@ -66,7 +64,6 @@ export class StellarEventListenerService
6664
@Inject(forwardRef(() => ConsistencyCheckerService))
6765
private consistencyChecker: ConsistencyCheckerService,
6866
@Optional() private escrowGateway?: EscrowGateway,
69-
@Optional() private notificationService?: NotificationService,
7067
) {}
7168

7269
async onModuleInit() {
@@ -737,36 +734,6 @@ export class StellarEventListenerService
737734
wsError,
738735
);
739736
}
740-
741-
// 8. Create notifications for buyer and seller
742-
try {
743-
const parties = await this.partyRepository.find({
744-
where: { escrowId: escrow.id },
745-
});
746-
747-
const notificationPayload = {
748-
escrowId: escrow.id,
749-
milestoneIndex,
750-
amount: releaseAmount,
751-
txHash: event.txHash,
752-
escrowTitle: escrow.title,
753-
};
754-
755-
for (const party of parties) {
756-
if (party.role === PartyRole.BUYER || party.role === PartyRole.SELLER) {
757-
await this.notificationService?.handleEscrowEvent(
758-
party.userId,
759-
NotificationEventType.MILESTONE_RELEASED,
760-
notificationPayload,
761-
);
762-
}
763-
}
764-
} catch (notifError) {
765-
this.logger.error(
766-
'Failed to create milestone release notifications',
767-
notifError,
768-
);
769-
}
770737
}
771738

772739
private async handleEscrowCompleted(event: StellarEvent) {

‎apps/backend/src/notifications/entities/notification.entity.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
Entity,
55
PrimaryGeneratedColumn,
66
UpdateDateColumn,
7+
Index,
78
} from 'typeorm';
89
import {
910
NotificationEventType,
@@ -40,6 +41,10 @@ export class Notification {
4041
@Column({ type: 'datetime', nullable: true })
4142
readAt?: Date;
4243

44+
@Column({ nullable: true })
45+
@Index({ unique: true })
46+
idempotencyKey?: string;
47+
4348
@CreateDateColumn()
4449
createdAt: Date;
4550

0 commit comments

Comments
 (0)