Fix double-payouts for maintenance pools and overdue expiry race conditions

src/bounties/bounties.service.spec.ts

@@ -228,23 +228,20 @@ describe('BountiesService', () => {
   });
 
   it('expireOverdue flips overdue bounties to EXPIRED and returns count', async () => {
-    const overdueBounties = [
-      { id: 'b1', status: BountyStatus.OPEN },
-      { id: 'b2', status: BountyStatus.FUNDED },
-    ];
     const mockQueryBuilder = {
+      update: jest.fn().mockReturnThis(),
+      set: jest.fn().mockReturnThis(),
       where: jest.fn().mockReturnThis(),
       andWhere: jest.fn().mockReturnThis(),
-      getMany: jest.fn().mockResolvedValue(overdueBounties),
+      execute: jest.fn().mockResolvedValue({ affected: 2 }),
     };
     bountyRepo.createQueryBuilder.mockReturnValue(mockQueryBuilder);
 
     const count = await service.expireOverdue();
 
     expect(count).toBe(2);
-    expect(bountyRepo.save).toHaveBeenCalledTimes(2);
-    expect(overdueBounties[0].status).toBe(BountyStatus.EXPIRED);
-    expect(overdueBounties[1].status).toBe(BountyStatus.EXPIRED);
+    expect(mockQueryBuilder.update).toHaveBeenCalledWith(Bounty);
+    expect(mockQueryBuilder.set).toHaveBeenCalledWith({ status: BountyStatus.EXPIRED });
   });
 
   it('markPrClosedWithoutMerge transitions IN_REVIEW back to CLAIMED', async () => {

src/bounties/bounties.service.ts

@@ -243,25 +243,23 @@ export class BountiesService {
 
   /** Marks bounties whose deadline has passed and that were never merged as expired. */
   async expireOverdue(): Promise<number> {
-    const overdue = await this.bountyRepo
-      .createQueryBuilder('bounty')
-      .where('bounty.deadline IS NOT NULL AND bounty.deadline < :now', {
+    const result = await this.bountyRepo
+      .createQueryBuilder()
+      .update(Bounty)
+      .set({ status: BountyStatus.EXPIRED })
+      .where('deadline IS NOT NULL AND deadline < :now', {
         now: new Date(),
       })
-      .andWhere('bounty.status IN (:...statuses)', {
+      .andWhere('status IN (:...statuses)', {
         statuses: [
           BountyStatus.OPEN,
           BountyStatus.FUNDED,
           BountyStatus.CLAIMED,
         ],
       })
-      .getMany();
+      .execute();
 
-    for (const bounty of overdue) {
-      bounty.status = BountyStatus.EXPIRED;
-      await this.bountyRepo.save(bounty);
-    }
-    return overdue.length;
+    return result.affected ?? 0;
   }
 
   async list(options: ListBountiesOptions = {}): Promise<Bounty[]> {

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';
 
 /**
@@ -25,6 +26,7 @@ import { AssetType, PaymentStatus } from '../enums';
  */
 @Entity('payments')
 @Index('IDX_payment_escrow', ['escrowId'])
+@Index('IDX_escrow_maintenance_issue', ['escrowId', 'maintenanceIssueId'], { unique: true, where: '"maintenanceIssueId" IS NOT NULL' })
 export class Payment {
   @PrimaryGeneratedColumn('uuid')
   id: string;
@@ -51,6 +53,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: 'uuid', nullable: true })
+  maintenanceIssueId: string | null;
+
   @Column({ type: 'decimal', precision: 20, scale: 7 })
   amount: string;
 

src/escrow/escrow.service.ts

@@ -396,6 +396,7 @@ export class EscrowService {
     amount: string,
     recipientAddress: string,
     recipientId?: string,
+    maintenanceIssueId?: string,
   ): Promise<Payment> {
     const escrow = await this.getOrThrow(escrowId);
     this.assertLocked(escrow);
@@ -415,6 +416,7 @@ export class EscrowService {
         escrowId: escrow.id,
         recipientId: recipientId ?? null,
         recipientAddress,
+        maintenanceIssueId: maintenanceIssueId ?? null,
         amount,
         asset: escrow.asset,
         status: PaymentStatus.CONFIRMED,

src/maintenance-pool/maintenance-pool.service.spec.ts

@@ -407,13 +407,13 @@ 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 same issue receives a reward twice (even for different recipients) (#273, #458)', async () => {
       poolRepo.findOne.mockResolvedValue({
         id: 'pool-1',
         balance: '100',
         escrowId: 'escrow-1',
       });
-      // Mock the payment query to return an existing payment
+      // Mock the payment query to return an existing payment for this issue
       const mockPaymentQueryBuilder = {
         innerJoin: jest.fn().mockReturnThis(),
         where: jest.fn().mockReturnThis(),
@@ -423,11 +423,65 @@ describe('MaintenancePoolService', () => {
       paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder);
 
       await expect(
-        service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT', 'user-1'),
+        service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT_B', 'user-2'),
       ).rejects.toThrow(ConflictException);
+
+      expect(mockPaymentQueryBuilder.andWhere).toHaveBeenCalledWith(
+        'payment.maintenanceIssueId = :issueId',
+        { issueId: 'issue-1' },
+      );
       expect(escrowService.poolWithdraw).not.toHaveBeenCalled();
     });
 
+    it('allows the same recipient to be rewarded for two different issues', async () => {
+      poolRepo.findOne.mockResolvedValue({
+        id: 'pool-1',
+        balance: '100',
+        escrowId: 'escrow-1',
+      });
+      const mockQueryBuilder = {
+        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(mockQueryBuilder);
+      escrowService.poolWithdraw.mockResolvedValue({ id: 'payment-1' });
+
+      // Return null, meaning no payment for this issue exists yet
+      const mockPaymentQueryBuilder = {
+        innerJoin: jest.fn().mockReturnThis(),
+        where: jest.fn().mockReturnThis(),
+        andWhere: jest.fn().mockReturnThis(),
+        getOne: jest.fn().mockResolvedValue(null),
+      };
+      paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder);
+
+      issueRepo.findOne.mockResolvedValue({
+        id: 'issue-2',
+        isMaintenanceType: true,
+        repositoryId: 'repository-1',
+      });
+
+      const payment = await service.assignReward(
+        'pool-1',
+        'issue-2',
+        '10',
+        'GRECIPIENT',
+        'user-1',
+      );
+
+      expect(payment).toEqual({ id: 'payment-1' });
+      expect(escrowService.poolWithdraw).toHaveBeenCalledWith(
+        'escrow-1',
+        '10',
+        'GRECIPIENT',
+        'user-1',
+        'issue-2',
+      );
+    });
+
     // 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

src/maintenance-pool/maintenance-pool.service.ts

@@ -144,19 +144,11 @@ export class MaintenancePoolService {
     }
 
     // Guard against double/triple payout for the same issue (#273).
-    const existingPayment = await this.paymentRepo.findOne({
-      where: {
-        recipientId: recipientId ?? null,
-      },
-    });
-    // 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(
@@ -190,6 +182,7 @@ export class MaintenancePoolService {
       amount,
       recipientAddress,
       recipientId,
+      issueId,
     );
 
     return payment;