diff --git a/src/bounties/bounties.service.spec.ts b/src/bounties/bounties.service.spec.ts index 405e3d2..81231a3 100644 --- a/src/bounties/bounties.service.spec.ts +++ b/src/bounties/bounties.service.spec.ts @@ -259,24 +259,47 @@ describe('BountiesService', () => { expect(bounty.status).toBe(BountyStatus.REFUNDED); }); - 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 = { + it('expireOverdue atomically flips overdue bounties to EXPIRED via a conditional update and returns affected count', async () => { + const qb = { + 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); + bountyRepo.createQueryBuilder.mockReturnValue(qb); 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(qb.set).toHaveBeenCalledWith({ status: BountyStatus.EXPIRED }); + expect(qb.andWhere).toHaveBeenCalledWith( + 'status IN (:...statuses)', + { statuses: [BountyStatus.OPEN, BountyStatus.FUNDED, BountyStatus.CLAIMED] }, + ); + expect(bountyRepo.save).not.toHaveBeenCalled(); + }); + + it('expireOverdue does not expire a bounty whose status advanced after it was read', async () => { + // Simulates a concurrent webhook moving CLAIMED -> IN_REVIEW between the + // scan and the write: the conditional UPDATE matches zero rows. + const qb = { + update: jest.fn().mockReturnThis(), + set: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + execute: jest.fn().mockResolvedValue({ affected: 0 }), + }; + bountyRepo.createQueryBuilder.mockReturnValue(qb); + + const count = await service.expireOverdue(); + + expect(count).toBe(0); + expect(qb.andWhere).toHaveBeenCalledWith( + 'status IN (:...statuses)', + { statuses: [BountyStatus.OPEN, BountyStatus.FUNDED, BountyStatus.CLAIMED] }, + ); + expect(bountyRepo.save).not.toHaveBeenCalled(); }); it('markPrClosedWithoutMerge transitions IN_REVIEW back to CLAIMED', async () => { diff --git a/src/bounties/bounties.service.ts b/src/bounties/bounties.service.ts index b1a0f8a..12603a4 100644 --- a/src/bounties/bounties.service.ts +++ b/src/bounties/bounties.service.ts @@ -274,27 +274,26 @@ export class BountiesService { return this.bountyRepo.save(bounty); } - /** Marks bounties whose deadline has passed and that were never merged as expired. */ + /** + * Marks bounties whose deadline has passed and that were never merged as + * expired. The status filter and the write are a single conditional UPDATE, + * so a bounty advanced concurrently (e.g. CLAIMED -> IN_REVIEW/MERGED/PAID) + * is never overwritten with a stale status. + */ async expireOverdue(): Promise { - const overdue = await this.bountyRepo - .createQueryBuilder('bounty') - .where('bounty.deadline IS NOT NULL AND bounty.deadline < :now', { - now: new Date(), - }) - .andWhere('bounty.status IN (:...statuses)', { - statuses: [ - BountyStatus.OPEN, - BountyStatus.FUNDED, - BountyStatus.CLAIMED, - ], - }) - .getMany(); - - for (const bounty of overdue) { - bounty.status = BountyStatus.EXPIRED; - await this.bountyRepo.save(bounty); - } - return overdue.length; + const expirable: BountyStatus[] = [ + BountyStatus.OPEN, + BountyStatus.FUNDED, + BountyStatus.CLAIMED, + ]; + const result = await this.bountyRepo + .createQueryBuilder() + .update(Bounty) + .set({ status: BountyStatus.EXPIRED }) + .where('deadline IS NOT NULL AND deadline < :now', { now: new Date() }) + .andWhere('status IN (:...statuses)', { statuses: expirable }) + .execute(); + return result.affected ?? 0; } async list(options: ListBountiesOptions = {}): Promise { diff --git a/src/bounties/bounty-expiry.scheduler.ts b/src/bounties/bounty-expiry.scheduler.ts index 58b79c9..6f3c0a5 100644 --- a/src/bounties/bounty-expiry.scheduler.ts +++ b/src/bounties/bounty-expiry.scheduler.ts @@ -14,6 +14,11 @@ import { BountiesService } from './bounties.service'; * This is deliberately independent of (and runs prior to) any on-chain * auto-refund follow-up for expired FUNDED bounties (#26): the local status * flip has to happen regardless. + * + * Concurrency note: `expireOverdue()` performs a single conditional UPDATE + * that only touches rows still in OPEN/FUNDED/CLAIMED at write time, so a + * bounty advanced concurrently by a webhook (e.g. CLAIMED -> IN_REVIEW -> + * MERGED/PAID) is never clobbered back to EXPIRED. */ @Injectable() export class BountyExpiryScheduler {