diff --git a/src/common/entities/payment.entity.spec.ts b/src/common/entities/payment.entity.spec.ts index ae476e93..ca99e2db 100644 --- a/src/common/entities/payment.entity.spec.ts +++ b/src/common/entities/payment.entity.spec.ts @@ -18,4 +18,22 @@ describe('Payment entity indexes', () => { ]), ); }); + + // #458: unique index on (escrowId, maintenanceIssueId) closes concurrent-call + // double-payout races for maintenance pool rewards. + it('indexes (escrowId, maintenanceIssueId) as unique UQ_payment_escrow_maintenance_issue', () => { + const indexes = getMetadataArgsStorage().indices.filter( + (index) => index.target === Payment, + ); + + expect(indexes).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + name: 'UQ_payment_escrow_maintenance_issue', + columns: ['escrowId', 'maintenanceIssueId'], + unique: true, + }), + ]), + ); + }); }); diff --git a/src/common/entities/payment.entity.ts b/src/common/entities/payment.entity.ts index 93024c8d..6853078b 100644 --- a/src/common/entities/payment.entity.ts +++ b/src/common/entities/payment.entity.ts @@ -10,6 +10,7 @@ import { } from 'typeorm'; import { Escrow } from './escrow.entity'; import { User } from './user.entity'; +import { Issue } from './issue.entity'; import { AssetType, PaymentStatus } from '../enums'; /** @@ -22,9 +23,18 @@ import { AssetType, PaymentStatus } from '../enums'; * payout — a hot path on every milestone-driven incremental release * (#307). The same gap class as `IDX_escrow_sponsor_status` (#97) and the * `Bounty.claimedById` index (#148). + * + * `UQ_payment_escrow_maintenance_issue` prevents duplicate payouts for the + * same maintenance issue from the same pool's escrow, closing concurrent-call + * races (#458). */ @Entity('payments') @Index('IDX_payment_escrow', ['escrowId']) +@Index( + 'UQ_payment_escrow_maintenance_issue', + ['escrowId', 'maintenanceIssueId'], + { unique: true }, +) export class Payment { @PrimaryGeneratedColumn('uuid') id: string; @@ -51,6 +61,13 @@ export class Payment { @Column({ type: 'varchar', nullable: true }) recipientAddress: string | null; + @ManyToOne(() => Issue, { onDelete: 'SET NULL', nullable: true }) + @JoinColumn() + maintenanceIssue: Issue | null; + + @Column({ type: 'varchar', nullable: true }) + maintenanceIssueId: string | null; + @Column({ type: 'decimal', precision: 20, scale: 7 }) amount: string; diff --git a/src/database/migrations/1785400000000-AddPaymentMaintenanceIssueIdAndIndex.ts b/src/database/migrations/1785400000000-AddPaymentMaintenanceIssueIdAndIndex.ts new file mode 100644 index 00000000..cdcd7bf8 --- /dev/null +++ b/src/database/migrations/1785400000000-AddPaymentMaintenanceIssueIdAndIndex.ts @@ -0,0 +1,41 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +/** + * Fixes #458: Add maintenanceIssueId to payments and unique index + * on (escrowId, maintenanceIssueId) to prevent double payouts for the + * same maintenance issue. + */ +export class AddPaymentMaintenanceIssueIdAndIndex1785400000000 + implements MigrationInterface +{ + name = 'AddPaymentMaintenanceIssueIdAndIndex1785400000000'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `ALTER TABLE "payments" ADD COLUMN IF NOT EXISTS "maintenanceIssueId" character varying`, + ); + const fkCheck = (await queryRunner.query(` + SELECT 1 FROM pg_constraint WHERE conname = 'FK_payment_maintenance_issue' + `)) as Array; + if (!fkCheck || fkCheck.length === 0) { + await queryRunner.query( + `ALTER TABLE "payments" ADD CONSTRAINT "FK_payment_maintenance_issue" FOREIGN KEY ("maintenanceIssueId") REFERENCES "issues"("id") ON DELETE SET NULL`, + ); + } + await queryRunner.query( + `CREATE UNIQUE INDEX IF NOT EXISTS "UQ_payment_escrow_maintenance_issue" ON "payments" ("escrowId", "maintenanceIssueId")`, + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query( + `DROP INDEX IF EXISTS "UQ_payment_escrow_maintenance_issue"`, + ); + await queryRunner.query( + `ALTER TABLE "payments" DROP CONSTRAINT IF EXISTS "FK_payment_maintenance_issue"`, + ); + await queryRunner.query( + `ALTER TABLE "payments" DROP COLUMN IF EXISTS "maintenanceIssueId"`, + ); + } +} diff --git a/src/escrow/escrow.service.spec.ts b/src/escrow/escrow.service.spec.ts index 87b44e6a..a6712d9a 100644 --- a/src/escrow/escrow.service.spec.ts +++ b/src/escrow/escrow.service.spec.ts @@ -851,6 +851,31 @@ describe('EscrowService', () => { expect(payment.amount).toBe('10.0000000'); }); + it('records maintenanceIssueId on the payment when provided (#458)', async () => { + escrowRepo.findOne.mockResolvedValue(lockedPoolEscrow()); + userRepo.find.mockResolvedValue([ + { id: 'user-1', stellarAddress: 'GRECIPIENT' }, + ]); + + await service.poolWithdraw( + 'escrow-pool', + '10.0000000', + 'GRECIPIENT', + 'user-1', + 'issue-42', + ); + + expect(paymentRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + escrowId: 'escrow-pool', + recipientId: 'user-1', + recipientAddress: 'GRECIPIENT', + amount: '10.0000000', + maintenanceIssueId: 'issue-42', + }), + ); + }); + it('leaves the escrow row LOCKED after a withdraw (pool is not closed out)', async () => { escrowRepo.findOne.mockResolvedValue(lockedPoolEscrow()); diff --git a/src/escrow/escrow.service.ts b/src/escrow/escrow.service.ts index 1f8ab69e..6f2661ac 100644 --- a/src/escrow/escrow.service.ts +++ b/src/escrow/escrow.service.ts @@ -409,6 +409,7 @@ export class EscrowService { amount: string, recipientAddress: string, recipientId?: string, + maintenanceIssueId?: string, ): Promise { const escrow = await this.getOrThrow(escrowId); this.assertLocked(escrow); @@ -432,6 +433,7 @@ export class EscrowService { asset: escrow.asset, status: PaymentStatus.CONFIRMED, txHash: result.txHash, + maintenanceIssueId: maintenanceIssueId ?? null, }), ); } diff --git a/src/maintenance-pool/maintenance-pool.controller.ts b/src/maintenance-pool/maintenance-pool.controller.ts index cadd59a9..ff43209b 100644 --- a/src/maintenance-pool/maintenance-pool.controller.ts +++ b/src/maintenance-pool/maintenance-pool.controller.ts @@ -8,7 +8,7 @@ import { Req, UseGuards, } from '@nestjs/common'; -import { Request } from 'express'; +import type { Request } from 'express'; import { ApiBearerAuth, ApiOperation, diff --git a/src/maintenance-pool/maintenance-pool.service.spec.ts b/src/maintenance-pool/maintenance-pool.service.spec.ts index 88ff0c4a..1a70ffbb 100644 --- a/src/maintenance-pool/maintenance-pool.service.spec.ts +++ b/src/maintenance-pool/maintenance-pool.service.spec.ts @@ -289,6 +289,13 @@ describe('MaintenancePoolService', () => { describe('assignReward', () => { beforeEach(() => { + // Default mock for poolRepo.findOne - active pool with escrow + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + balance: '100', + escrowId: 'escrow-1', + }); // Default mock for paymentRepo.createQueryBuilder - returns null (no existing payment) const mockPaymentQueryBuilder = { innerJoin: jest.fn().mockReturnThis(), @@ -299,9 +306,24 @@ describe('MaintenancePoolService', () => { paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder); }); + it('rejects when the pool is not ACTIVE (#276)', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.PAUSED, + balance: '100', + escrowId: 'escrow-1', + }); + + await expect( + service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'), + ).rejects.toThrow(BadRequestException); + expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); + }); + it('rejects when the pool has no funded escrow yet', async () => { poolRepo.findOne.mockResolvedValue({ id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, balance: '100', escrowId: null, }); @@ -315,6 +337,7 @@ describe('MaintenancePoolService', () => { it('rejects when the requested amount exceeds the pool balance', async () => { poolRepo.findOne.mockResolvedValue({ id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, balance: '50', escrowId: 'escrow-1', }); @@ -337,6 +360,7 @@ describe('MaintenancePoolService', () => { it('releases the reward and atomically decrements the balance', async () => { poolRepo.findOne.mockResolvedValue({ id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, balance: '100', escrowId: 'escrow-1', }); @@ -364,6 +388,7 @@ describe('MaintenancePoolService', () => { '30', 'GRECIPIENT', 'user-1', + 'issue-1', ); expect(payment).toEqual({ id: 'payment-1' }); // Verify the atomic balance check was called @@ -373,6 +398,7 @@ describe('MaintenancePoolService', () => { it('rejects a reward for a non-maintenance issue before releasing funds', async () => { poolRepo.findOne.mockResolvedValue({ id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, balance: '100', escrowId: 'escrow-1', }); @@ -391,6 +417,7 @@ describe('MaintenancePoolService', () => { it('rejects an issue outside the pool repository', async () => { poolRepo.findOne.mockResolvedValue({ id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, repositoryId: 'repository-1', balance: '100', escrowId: 'escrow-1', @@ -407,9 +434,10 @@ describe('MaintenancePoolService', () => { expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); }); - it('rejects when the issue has already received a reward from this pool (#273)', async () => { + it('rejects when the issue has already received a reward from this pool (#273, #458)', async () => { poolRepo.findOne.mockResolvedValue({ id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, balance: '100', escrowId: 'escrow-1', }); @@ -425,9 +453,221 @@ describe('MaintenancePoolService', () => { await expect( service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT', 'user-1'), ).rejects.toThrow(ConflictException); + + expect(mockPaymentQueryBuilder.innerJoin).toHaveBeenCalledWith( + 'payment.escrow', + 'escrow', + ); + expect(mockPaymentQueryBuilder.where).toHaveBeenCalledWith( + 'escrow.maintenancePoolId = :poolId', + { poolId: 'pool-1' }, + ); + expect(mockPaymentQueryBuilder.andWhere).toHaveBeenCalledWith( + 'payment.maintenanceIssueId = :issueId', + { issueId: 'issue-1' }, + ); + expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); + }); + + it('rejects when the same issue is rewarded a second time with a different recipient (409) (#458)', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + balance: '100', + escrowId: 'escrow-1', + }); + + // First call: payment query returns null, payment succeeds + // Second call: payment query returns existing payment for issue-1 + const mockPaymentQueryBuilder = { + innerJoin: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + getOne: jest + .fn() + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ + id: 'payment-1', + maintenanceIssueId: 'issue-1', + }), + }; + paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder); + + const mockBalanceQueryBuilder = { + update: jest.fn().mockReturnThis(), + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + setParameter: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected: 1 }), + }; + poolRepo.createQueryBuilder.mockReturnValue(mockBalanceQueryBuilder); + escrowService.poolWithdraw.mockResolvedValue({ id: 'payment-1' }); + + // First call with recipient A succeeds + await expect( + service.assignReward( + 'pool-1', + 'issue-1', + '10', + 'GRECIPIENT_A', + 'user-A', + ), + ).resolves.toEqual({ id: 'payment-1' }); + + // Second call for the same issue with recipient B fails with ConflictException (409) + await expect( + service.assignReward( + 'pool-1', + 'issue-1', + '10', + 'GRECIPIENT_B', + 'user-B', + ), + ).rejects.toThrow(ConflictException); + }); + + it('allows two different issues to be rewarded to the same recipient (#458)', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + balance: '100', + escrowId: 'escrow-1', + }); + + issueRepo.findOne.mockImplementation(({ where: { id } }: any) => + Promise.resolve({ + id, + isMaintenanceType: true, + repositoryId: 'repository-1', + }), + ); + + // Payment query returns null for both issue-1 and issue-2 + const mockPaymentQueryBuilder = { + innerJoin: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + getOne: jest.fn().mockResolvedValue(null), + }; + paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder); + + const mockBalanceQueryBuilder = { + update: jest.fn().mockReturnThis(), + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + setParameter: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected: 1 }), + }; + poolRepo.createQueryBuilder.mockReturnValue(mockBalanceQueryBuilder); + escrowService.poolWithdraw + .mockResolvedValueOnce({ id: 'payment-1' }) + .mockResolvedValueOnce({ id: 'payment-2' }); + + // Both calls for the same recipient ('user-1') on different issues succeed + const payment1 = await service.assignReward( + 'pool-1', + 'issue-1', + '10', + 'GRECIPIENT_1', + 'user-1', + ); + const payment2 = await service.assignReward( + 'pool-1', + 'issue-2', + '20', + 'GRECIPIENT_1', + 'user-1', + ); + + expect(payment1).toEqual({ id: 'payment-1' }); + expect(payment2).toEqual({ id: 'payment-2' }); + expect(escrowService.poolWithdraw).toHaveBeenCalledTimes(2); + expect(escrowService.poolWithdraw).toHaveBeenNthCalledWith( + 1, + 'escrow-1', + '10', + 'GRECIPIENT_1', + 'user-1', + 'issue-1', + ); + expect(escrowService.poolWithdraw).toHaveBeenNthCalledWith( + 2, + 'escrow-1', + '20', + 'GRECIPIENT_1', + 'user-1', + 'issue-2', + ); + }); + + it('rejects anonymous payout when issue was already rewarded (#458)', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + balance: '100', + escrowId: 'escrow-1', + }); + const mockPaymentQueryBuilder = { + innerJoin: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + getOne: jest.fn().mockResolvedValue({ id: 'existing-payment' }), + }; + paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder); + + // No recipientId passed (anonymous payout) + await expect( + service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'), + ).rejects.toThrow(ConflictException); + + expect(mockPaymentQueryBuilder.andWhere).toHaveBeenCalledWith( + 'payment.maintenanceIssueId = :issueId', + { issueId: 'issue-1' }, + ); expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); }); + it('catches unique constraint violation on concurrent insert and rolls back balance (#458)', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + balance: '100', + escrowId: 'escrow-1', + }); + const mockPaymentQueryBuilder = { + innerJoin: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + getOne: jest.fn().mockResolvedValue(null), + }; + paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder); + + const mockBalanceQueryBuilder = { + update: jest.fn().mockReturnThis(), + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + setParameter: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected: 1 }), + }; + poolRepo.createQueryBuilder.mockReturnValue(mockBalanceQueryBuilder); + + const uniqueViolationError = new Error( + 'duplicate key value violates unique constraint "UQ_payment_escrow_maintenance_issue"', + ); + (uniqueViolationError as any).code = '23505'; + escrowService.poolWithdraw.mockRejectedValue(uniqueViolationError); + + await expect( + service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT', 'user-1'), + ).rejects.toThrow(ConflictException); + + expect(poolRepo.increment).toHaveBeenCalledWith( + { id: 'pool-1' }, + 'balance', + 10, + ); + }); + // Regression test for #51 (MaintenancePool.balance was a hand-maintained // running total with a lost-update race across concurrent // deposit/assignReward calls): assignReward now decrements via an @@ -437,7 +677,8 @@ describe('MaintenancePoolService', () => { // time — not a value cached from an earlier read — and neither // decrement is lost. it('two concurrent assignReward calls both apply — no lost decrement (#51)', async () => { - const sharedPoolRow: { balance: string; escrowId: string } = { + const sharedPoolRow = { + status: MaintenancePoolStatus.ACTIVE, balance: '1000.0000000', escrowId: 'escrow-1', }; @@ -457,7 +698,7 @@ describe('MaintenancePoolService', () => { await Promise.all([ service.assignReward('pool-1', 'issue-1', '100', 'GRECIPIENT_A'), - service.assignReward('pool-1', 'issue-1', '200', 'GRECIPIENT_B'), + service.assignReward('pool-1', 'issue-2', '200', 'GRECIPIENT_B'), ]); // Both calls should have succeeded (atomic check passed) diff --git a/src/maintenance-pool/maintenance-pool.service.ts b/src/maintenance-pool/maintenance-pool.service.ts index 2fc57edf..464590f9 100644 --- a/src/maintenance-pool/maintenance-pool.service.ts +++ b/src/maintenance-pool/maintenance-pool.service.ts @@ -146,20 +146,13 @@ export class MaintenancePoolService { throw new BadRequestException(`Pool ${id} has no funded escrow yet`); } - // Guard against double/triple payout for the same issue (#273). - const existingPayment = await this.paymentRepo.findOne({ - where: { - recipientId: recipientId ?? null, - }, - }); + // Guard against double/triple payout for the same issue (#273, #458). // Check if there's already a payment for this issue from this pool's escrow. const existingPoolPayment = await this.paymentRepo .createQueryBuilder('payment') .innerJoin('payment.escrow', 'escrow') .where('escrow.maintenancePoolId = :poolId', { poolId: id }) - .andWhere('payment.recipientId = :recipientId', { - recipientId: recipientId ?? null, - }) + .andWhere('payment.maintenanceIssueId = :issueId', { issueId }) .getOne(); if (existingPoolPayment) { throw new ConflictException( @@ -188,14 +181,33 @@ export class MaintenancePoolService { // not a milestone-style fixed lock that gets partially released and then // closed out — so pay the reward via the pool contract's `withdraw`, // leaving the escrow LOCKED for the next reward (#163). - const payment = await this.escrowService.poolWithdraw( - pool.escrowId, - amount, - recipientAddress, - recipientId, - ); - - return payment; + try { + const payment = await this.escrowService.poolWithdraw( + pool.escrowId, + amount, + recipientAddress, + recipientId, + issueId, + ); + return payment; + } catch (err) { + if ( + (err as any)?.code === '23505' || + (err as any)?.driverError?.code === '23505' || + /unique/i.test((err as any)?.message) + ) { + // Rollback balance decrement if concurrent insert hit unique constraint (#458) + await this.poolRepo.increment( + { id: pool.id }, + 'balance', + Number(amount), + ); + throw new ConflictException( + `Issue ${issueId} has already received a reward from pool ${id}`, + ); + } + throw err; + } } async list(): Promise {