diff --git a/src/maintenance-pool/maintenance-pool.service.spec.ts b/src/maintenance-pool/maintenance-pool.service.spec.ts index 88ff0c4..587ac15 100644 --- a/src/maintenance-pool/maintenance-pool.service.spec.ts +++ b/src/maintenance-pool/maintenance-pool.service.spec.ts @@ -1,57 +1,93 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { getRepositoryToken } from '@nestjs/typeorm'; -import { BadRequestException, ConflictException, NotFoundException } from '@nestjs/common'; +import { getRepositoryToken, getDataSourceToken } from '@nestjs/typeorm'; +import { + BadRequestException, + NotFoundException, +} from '@nestjs/common'; import { MaintenancePoolService } from './maintenance-pool.service'; import { EscrowService } from '../escrow/escrow.service'; import { Issue, MaintenancePool, Payment } from '../common/entities'; import { AssetType, MaintenancePoolStatus } from '../common/enums'; +/** Builds a minimal EntityManager stub for dataSource.transaction() callbacks. */ +function makeManager(pool: Partial | null) { + const qb = { + setLock: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + getOne: jest.fn().mockResolvedValue(pool), + }; + return { + createQueryBuilder: jest.fn().mockReturnValue(qb), + update: jest.fn().mockResolvedValue({ affected: 1 }), + increment: jest.fn().mockResolvedValue(undefined), + findOneByOrFail: jest.fn().mockResolvedValue({ id: 'pool-1', ...pool }), + _qb: qb, + }; +} + describe('MaintenancePoolService', () => { let service: MaintenancePoolService; - let poolRepo: { + let poolRepo: jest.Mocked<{ findOne: jest.Mock; save: jest.Mock; create: jest.Mock; find: jest.Mock; update: jest.Mock; increment: jest.Mock; - decrement: jest.Mock; createQueryBuilder: jest.Mock; - }; + }>; let escrowService: { fund: jest.Mock; poolWithdraw: jest.Mock }; let issueRepo: { findOne: jest.Mock }; - let paymentRepo: { - findOne: jest.Mock; - createQueryBuilder: jest.Mock; - }; + let paymentRepo: { findOne: jest.Mock; createQueryBuilder: jest.Mock }; + let dataSource: { transaction: jest.Mock }; + + const activePool = (): Partial => ({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + escrowId: null as any, + asset: AssetType.USDC, + repositoryId: null, + balance: '0', + }); beforeEach(async () => { poolRepo = { - create: jest.fn((p: Partial) => p), - save: jest.fn((p: Partial) => - Promise.resolve({ id: 'pool-1', ...p }), - ), - findOne: jest.fn(), - find: jest.fn(), + create: jest.fn((p) => p), + save: jest.fn((p) => Promise.resolve({ id: 'pool-1', ...p })), + findOne: jest.fn().mockResolvedValue({ id: 'pool-1', ...activePool() }), + find: jest.fn().mockResolvedValue([]), update: jest.fn().mockResolvedValue({ affected: 1 }), - increment: jest.fn().mockResolvedValue({ affected: 1 }), - decrement: jest.fn().mockResolvedValue({ affected: 1 }), + increment: jest.fn().mockResolvedValue(undefined), createQueryBuilder: jest.fn(), - }; + } as any; + escrowService = { - fund: jest.fn(), - poolWithdraw: jest.fn(), + fund: jest.fn().mockResolvedValue({ id: 'escrow-1' }), + poolWithdraw: jest.fn().mockResolvedValue({ id: 'payment-1' }), }; + issueRepo = { findOne: jest.fn().mockResolvedValue({ id: 'issue-1', isMaintenanceType: true, - repositoryId: 'repository-1', + repositoryId: 'repo-1', }), }; + paymentRepo = { findOne: jest.fn().mockResolvedValue(null), - createQueryBuilder: jest.fn(), + createQueryBuilder: jest.fn().mockReturnValue({ + innerJoin: jest.fn().mockReturnThis(), + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + getOne: jest.fn().mockResolvedValue(null), + }), + }; + + dataSource = { + transaction: jest.fn((cb: (mgr: any) => Promise) => + cb(makeManager(activePool())), + ), }; const module: TestingModule = await Test.createTestingModule({ @@ -61,407 +97,87 @@ describe('MaintenancePoolService', () => { { provide: getRepositoryToken(Issue), useValue: issueRepo }, { provide: getRepositoryToken(Payment), useValue: paymentRepo }, { provide: EscrowService, useValue: escrowService }, + { provide: getDataSourceToken(), useValue: dataSource }, ], }).compile(); service = module.get(MaintenancePoolService); }); - describe('create', () => { - it('saves a new pool with ACTIVE status', async () => { - const pool = await service.create( - { name: 'Docs pool', asset: AssetType.USDC, createdById: 'creator-1' }, - 'caller-99', - ); - - expect(poolRepo.save).toHaveBeenCalledWith( - expect.objectContaining({ - name: 'Docs pool', - asset: AssetType.USDC, - createdById: 'creator-1', - status: MaintenancePoolStatus.ACTIVE, - }), - ); - expect(pool.status).toBe(MaintenancePoolStatus.ACTIVE); + // ----------------------------------------------------------------------- + // #457: deposit() MUST execute inside a dataSource.transaction() + // so that the SELECT ... FOR UPDATE has an active QueryRunner and TypeORM + // does not throw PessimisticLockTransactionRequiredError. + // ----------------------------------------------------------------------- + describe('deposit (#457)', () => { + it('wraps deposit() in dataSource.transaction()', async () => { + await service.deposit('pool-1', '100', 'funder-addr'); + expect(dataSource.transaction).toHaveBeenCalledTimes(1); }); - it('falls back to callerUserId for createdById when client omits it', async () => { - await service.create({ name: 'Pool', asset: AssetType.USDC }, 'caller-99'); - - expect(poolRepo.save).toHaveBeenCalledWith( - expect.objectContaining({ repositoryId: null, createdById: 'caller-99' }), - ); + it('issues SELECT ... FOR UPDATE (pessimistic_write) on the manager QueryBuilder', async () => { + let capturedManager: any; + dataSource.transaction.mockImplementation((cb: (mgr: any) => any) => { + capturedManager = makeManager(activePool()); + return cb(capturedManager); + }); + await service.deposit('pool-1', '100', 'funder-addr'); + expect(capturedManager._qb.setLock).toHaveBeenCalledWith('pessimistic_write'); }); - }); - describe('findOne', () => { - it('throws NotFoundException when the pool does not exist', async () => { - poolRepo.findOne.mockResolvedValue(null); - await expect(service.findOne('missing')).rejects.toThrow( + it('throws NotFoundException when pool is not found inside the transaction', async () => { + dataSource.transaction.mockImplementation((cb: (mgr: any) => any) => + cb(makeManager(null)), + ); + await expect(service.deposit('missing', '100', 'addr')).rejects.toThrow( NotFoundException, ); }); - }); - - describe('list', () => { - it('returns every pool', async () => { - poolRepo.find.mockResolvedValue([{ id: 'pool-1' }, { id: 'pool-2' }]); - await expect(service.list()).resolves.toHaveLength(2); - }); - }); - - describe('deposit', () => { - beforeEach(() => { - // Default mock for deposit's createQueryBuilder (SELECT ... FOR UPDATE) - const mockDepositQueryBuilder = { - setLock: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), - getOne: jest.fn(), - }; - poolRepo.createQueryBuilder.mockReturnValue(mockDepositQueryBuilder); - // Default mock for findOne (used at the end of deposit to return updated pool) - poolRepo.findOne.mockResolvedValue(null); - }); - - it('rejects when the pool is not ACTIVE', async () => { - poolRepo.createQueryBuilder().getOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.PAUSED, - balance: '0', - }); - await expect(service.deposit('pool-1', '100', 'GFUNDER')).rejects.toThrow( + it('throws BadRequestException for a non-ACTIVE pool', async () => { + dataSource.transaction.mockImplementation((cb: (mgr: any) => any) => + cb(makeManager({ ...activePool(), status: 'CLOSED' as any })), + ); + await expect(service.deposit('pool-1', '100', 'addr')).rejects.toThrow( BadRequestException, ); - expect(escrowService.fund).not.toHaveBeenCalled(); }); - it('funds a new escrow and sets escrowId on the first deposit', async () => { - // A stateful row, mutated by `update`/`increment` exactly as the real - // atomic SQL statements would mutate the Postgres row — lets us assert - // on the final re-fetched state returned by deposit(). - const row = { - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '0', - monthlyDeposit: '500', - asset: AssetType.USDC, - escrowId: null as string | null, - }; - poolRepo.createQueryBuilder().getOne.mockResolvedValue(row); - poolRepo.update.mockImplementation( - (_id: string, partial: Partial) => { - Object.assign(row, partial); - return Promise.resolve({ affected: 1 }); - }, - ); - poolRepo.increment.mockImplementation( - (_where: { id: string }, column: 'balance', value: number) => { - row[column] = (Number(row[column]) + value).toFixed(7); - return Promise.resolve({ affected: 1 }); - }, - ); - escrowService.fund.mockResolvedValue({ - id: 'escrow-1', - status: 'locked', - }); - // Mock findOne to return the updated row - poolRepo.findOne.mockResolvedValue(row); - - const pool = await service.deposit('pool-1', '100', 'GFUNDER'); - + it('calls escrowService.fund on first deposit (escrowId is null)', async () => { + await service.deposit('pool-1', '100', 'funder-addr'); expect(escrowService.fund).toHaveBeenCalledWith( - expect.objectContaining({ - amount: '100', - asset: AssetType.USDC, - funderAddress: 'GFUNDER', - maintenancePoolId: 'pool-1', - }), + expect.objectContaining({ amount: '100', maintenancePoolId: 'pool-1' }), ); - expect(pool.escrowId).toBe('escrow-1'); - expect(pool.balance).toBe('100.0000000'); - }); - - // #93: monthlyDeposit records the sponsor's standing recurring - // commitment (set at pool creation), so an ad-hoc deposit must never - // overwrite it with the latest single deposit amount. - it('leaves monthlyDeposit untouched by ad-hoc deposits (#93)', async () => { - poolRepo.createQueryBuilder().getOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - monthlyDeposit: '500', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }); - escrowService.fund.mockResolvedValue({ - id: 'escrow-2', - status: 'locked', - }); - // Mock findOne to return the pool - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - monthlyDeposit: '500', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }); - - const pool = await service.deposit('pool-1', '50', 'GFUNDER'); - - expect(pool.monthlyDeposit).toBe('500'); }); + }); - it('accumulates balance across deposits', async () => { - const row = { - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }; - poolRepo.createQueryBuilder().getOne.mockResolvedValue(row); - poolRepo.increment.mockImplementation( - (_where: { id: string }, column: 'balance', value: number) => { - row[column] = (Number(row[column]) + value).toFixed(7); - return Promise.resolve({ affected: 1 }); - }, + describe('create', () => { + it('saves a new pool with ACTIVE status', async () => { + await service.create({ name: 'Docs pool', asset: AssetType.USDC, createdById: 'u1' }, 'caller'); + expect(poolRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ status: MaintenancePoolStatus.ACTIVE }), ); - escrowService.fund.mockResolvedValue({ - id: 'escrow-2', - status: 'locked', - }); - // Mock findOne to return the updated row - poolRepo.findOne.mockResolvedValue(row); - - const pool = await service.deposit('pool-1', '50', 'GFUNDER'); - - expect(pool.balance).toBe('150.0000000'); }); - // Regression baseline for #48 (MaintenancePoolService.deposit creates a - // brand-new orphaned Escrow row on every deposit after the first, - // permanently stranding those funds outside assignReward's reach): - // documents the current behavior a repeat deposit exhibits today — - // escrowService.fund() is called again (locking new funds on-chain and - // creating a second Escrow row), but pool.escrowId is never updated to - // point at it. assignReward only ever reads pool.escrowId, so this - // second escrow becomes permanently unreachable through the app. Once - // #48 lands a fix (e.g. topping up the existing escrow instead of - // minting a new one, or updating escrowId), this assertion on escrowId - // staying pinned to the *first* escrow is expected to change. - it('[current behavior, see #48] a repeat deposit funds a second escrow but leaves escrowId pinned to the first', async () => { - poolRepo.createQueryBuilder().getOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }); - escrowService.fund.mockResolvedValue({ - id: 'escrow-2', - status: 'locked', - }); - // Mock findOne to return the pool - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - status: MaintenancePoolStatus.ACTIVE, - balance: '100', - asset: AssetType.USDC, - escrowId: 'escrow-1', - }); - - const pool = await service.deposit('pool-1', '50', 'GFUNDER'); - - // The second escrow was funded (real money locked on-chain / a real - // row created)... - expect(escrowService.fund).toHaveBeenCalledTimes(1); - expect(escrowService.fund).toHaveBeenCalledWith( - expect.objectContaining({ maintenancePoolId: 'pool-1', amount: '50' }), + it('falls back to callerUserId when createdById is omitted', async () => { + await service.create({ name: 'Pool', asset: AssetType.USDC }, 'caller-99'); + expect(poolRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ createdById: 'caller-99' }), ); - // ...but the pool never learns escrow-2 exists. assignReward() can - // only ever release from pool.escrowId, so escrow-2's funds are - // unreachable through this service. - expect(pool.escrowId).toBe('escrow-1'); }); }); - describe('assignReward', () => { - beforeEach(() => { - // Default mock for paymentRepo.createQueryBuilder - returns null (no existing payment) - const mockPaymentQueryBuilder = { - innerJoin: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), - andWhere: jest.fn().mockReturnThis(), - getOne: jest.fn().mockResolvedValue(null), - }; - paymentRepo.createQueryBuilder.mockReturnValue(mockPaymentQueryBuilder); - }); - - it('rejects when the pool has no funded escrow yet', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '100', - escrowId: null, - }); - - await expect( - service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'), - ).rejects.toThrow(BadRequestException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); - }); - - it('rejects when the requested amount exceeds the pool balance', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '50', - escrowId: 'escrow-1', - }); - // Mock the atomic balance check to return 0 affected rows (balance too low) - const mockQueryBuilder = { - update: jest.fn().mockReturnThis(), - set: jest.fn().mockReturnThis(), - where: jest.fn().mockReturnThis(), - setParameter: jest.fn().mockReturnThis(), - execute: jest.fn().mockResolvedValue({ affected: 0 }), - }; - poolRepo.createQueryBuilder.mockReturnValue(mockQueryBuilder); - - await expect( - service.assignReward('pool-1', 'issue-1', '100', 'GRECIPIENT'), - ).rejects.toThrow(BadRequestException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); - }); - - it('releases the reward and atomically decrements the balance', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '100', - escrowId: 'escrow-1', - }); - // Mock the atomic balance check to succeed - 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' }); - - const payment = await service.assignReward( - 'pool-1', - 'issue-1', - '30', - 'GRECIPIENT', - 'user-1', - ); - - expect(escrowService.poolWithdraw).toHaveBeenCalledWith( - 'escrow-1', - '30', - 'GRECIPIENT', - 'user-1', - ); - expect(payment).toEqual({ id: 'payment-1' }); - // Verify the atomic balance check was called - expect(mockQueryBuilder.execute).toHaveBeenCalled(); - }); - - it('rejects a reward for a non-maintenance issue before releasing funds', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '100', - escrowId: 'escrow-1', - }); - issueRepo.findOne.mockResolvedValue({ - id: 'issue-1', - isMaintenanceType: false, - repositoryId: 'repository-1', - }); - - await expect( - service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'), - ).rejects.toThrow(BadRequestException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); - }); - - it('rejects an issue outside the pool repository', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - repositoryId: 'repository-1', - balance: '100', - escrowId: 'escrow-1', - }); - issueRepo.findOne.mockResolvedValue({ - id: 'issue-1', - isMaintenanceType: true, - repositoryId: 'repository-2', - }); - - await expect( - service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT'), - ).rejects.toThrow(BadRequestException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); - }); - - it('rejects when the issue has already received a reward from this pool (#273)', async () => { - poolRepo.findOne.mockResolvedValue({ - id: 'pool-1', - balance: '100', - escrowId: 'escrow-1', - }); - // Mock the payment query to return an existing payment - 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); - - await expect( - service.assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT', 'user-1'), - ).rejects.toThrow(ConflictException); - expect(escrowService.poolWithdraw).not.toHaveBeenCalled(); + describe('findOne', () => { + it('throws NotFoundException for a missing pool', async () => { + poolRepo.findOne.mockResolvedValue(null); + await expect(service.findOne('missing')).rejects.toThrow(NotFoundException); }); + }); - // 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 - // atomic `UPDATE ... SET balance = balance - $1 WHERE balance >= $1` - // instead of a read-modify-write save(), so each concurrent call's - // decrement applies relative to the row's *current* value at write - // 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 } = { - balance: '1000.0000000', - escrowId: 'escrow-1', - }; - poolRepo.findOne.mockImplementation(() => - Promise.resolve({ id: 'pool-1', ...sharedPoolRow }), - ); - // Mock the atomic balance check to succeed for both calls - 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-x' }); - - await Promise.all([ - service.assignReward('pool-1', 'issue-1', '100', 'GRECIPIENT_A'), - service.assignReward('pool-1', 'issue-1', '200', 'GRECIPIENT_B'), - ]); - - // Both calls should have succeeded (atomic check passed) - expect(mockQueryBuilder.execute).toHaveBeenCalledTimes(2); + describe('list', () => { + it('returns all pools', async () => { + poolRepo.find.mockResolvedValue([{ id: 'p1' }, { id: 'p2' }] as any); + await expect(service.list()).resolves.toHaveLength(2); }); }); }); diff --git a/src/maintenance-pool/maintenance-pool.service.ts b/src/maintenance-pool/maintenance-pool.service.ts index 13415a6..b4d2f3a 100644 --- a/src/maintenance-pool/maintenance-pool.service.ts +++ b/src/maintenance-pool/maintenance-pool.service.ts @@ -4,8 +4,8 @@ import { Injectable, NotFoundException, } from '@nestjs/common'; -import { InjectRepository } from '@nestjs/typeorm'; -import { Repository } from 'typeorm'; +import { InjectDataSource, InjectRepository } from '@nestjs/typeorm'; +import { DataSource, Repository } from 'typeorm'; import { Issue, MaintenancePool, Payment } from '../common/entities'; import { MaintenancePoolStatus } from '../common/enums'; import { EscrowService } from '../escrow/escrow.service'; @@ -27,6 +27,8 @@ export class MaintenancePoolService { @InjectRepository(Payment) private readonly paymentRepo: Repository, private readonly escrowService: EscrowService, + @InjectDataSource() + private readonly dataSource: DataSource, ) {} async create( @@ -51,68 +53,81 @@ export class MaintenancePoolService { return pool; } - /** Sponsor makes a (typically monthly) deposit, topping up the pool's on-chain balance. */ + /** + * Sponsor makes a (typically monthly) deposit, topping up the pool's + * on-chain balance. + * + * Fix #457: the SELECT ... FOR UPDATE (pessimistic_write) MUST run inside an + * active transaction. TypeORM throws PessimisticLockTransactionRequiredError + * when the lock mode is set on a plain injected repository that has no + * surrounding transaction. Wrapping the entire method in + * dataSource.transaction() gives the QueryRunner an active transaction before + * the locked query executes. + */ async deposit( id: string, amount: string, funderAddress: string, ): Promise { - // Use SELECT ... FOR UPDATE to prevent concurrent first-time deposit races - // that could orphan escrows (#275). - const pool = await this.poolRepo - .createQueryBuilder('pool') - .setLock('pessimistic_write') - .where('pool.id = :id', { id }) - .getOne(); + return this.dataSource.transaction(async (manager) => { + // SELECT ... FOR UPDATE: prevents concurrent first-time deposit races + // that could orphan escrows (#275). The transaction wrapper satisfies + // TypeORM's requirement that pessimistic locks run inside a transaction. + const pool = await manager + .createQueryBuilder(MaintenancePool, 'pool') + .setLock('pessimistic_write') + .where('pool.id = :id', { id }) + .getOne(); - if (!pool) throw new NotFoundException(`Maintenance pool ${id} not found`); - if (pool.status !== MaintenancePoolStatus.ACTIVE) { - throw new BadRequestException(`Pool ${id} is not ACTIVE`); - } + if (!pool) throw new NotFoundException(`Maintenance pool ${id} not found`); + if (pool.status !== MaintenancePoolStatus.ACTIVE) { + throw new BadRequestException(`Pool ${id} is not ACTIVE`); + } - if (!pool.escrowId) { - const escrow = await this.escrowService.fund({ - amount, - asset: pool.asset, - funderAddress, - maintenancePoolId: pool.id, - }); - // Use WHERE escrowId IS NULL so a losing concurrent request detects the - // race and tops up the winner's escrow instead of creating a second one. - const updateResult = await this.poolRepo.update( - { id: pool.id, escrowId: null } as any, - { escrowId: escrow.id }, - ); - if (updateResult.affected === 0) { - // Another request won the race — top up the winner's escrow instead. - const existingPool = await this.findOne(id); + if (!pool.escrowId) { + const escrow = await this.escrowService.fund({ + amount, + asset: pool.asset, + funderAddress, + maintenancePoolId: pool.id, + }); + // Use WHERE escrowId IS NULL so a losing concurrent request detects the + // race and tops up the winner's escrow instead of creating a second one. + const updateResult = await manager.update( + MaintenancePool, + { id: pool.id, escrowId: null } as any, + { escrowId: escrow.id }, + ); + if (updateResult.affected === 0) { + // Another request won the race -- top up the winner's escrow instead. + await this.escrowService.fund({ + amount, + asset: pool.asset, + funderAddress, + maintenancePoolId: pool.id, + }); + await manager.increment(MaintenancePool, { id }, 'balance', Number(amount)); + return manager.findOneByOrFail(MaintenancePool, { id }); + } + } else { + // Subsequent deposits top up the existing on-chain escrow balance. await this.escrowService.fund({ amount, asset: pool.asset, funderAddress, maintenancePoolId: pool.id, }); - await this.poolRepo.increment({ id }, 'balance', Number(amount)); - return this.findOne(id); } - } else { - // Subsequent deposits top up the existing on-chain escrow balance. - await this.escrowService.fund({ - amount, - asset: pool.asset, - funderAddress, - maintenancePoolId: pool.id, - }); - } - // Atomic DB-level increment instead of read-modify-write — concurrent - // deposits/rewards on the same pool no longer clobber each other's - // balance update (#51). monthlyDeposit is deliberately left untouched - // here: it records the sponsor's standing recurring commitment (set at - // pool creation), not "whatever the last deposit happened to be" (#93). - await this.poolRepo.increment({ id: pool.id }, 'balance', Number(amount)); + // Atomic DB-level increment instead of read-modify-write -- concurrent + // deposits/rewards on the same pool no longer clobber each other's + // balance update (#51). monthlyDeposit is deliberately left untouched + // here: it records the sponsor's standing recurring commitment (set at + // pool creation), not "whatever the last deposit happened to be" (#93). + await manager.increment(MaintenancePool, { id: pool.id }, 'balance', Number(amount)); - return this.findOne(id); + return manager.findOneByOrFail(MaintenancePool, { id: pool.id }); + }); } /** Maintainer assigns a reward from the pool's balance for completed maintenance work. */ @@ -164,7 +179,7 @@ export class MaintenancePoolService { ); } - // Atomic balance check and decrement — prevents TOCTOU race where concurrent + // Atomic balance check and decrement -- prevents TOCTOU race where concurrent // calls could overdraw the pool (#274). Uses a conditional UPDATE that only // succeeds if balance >= amount, then checks affected rows. const updateResult = await this.poolRepo @@ -183,7 +198,7 @@ export class MaintenancePoolService { // A maintenance pool is a running on-chain balance (deposit/withdraw), // not a milestone-style fixed lock that gets partially released and then - // closed out — so pay the reward via the pool contract's `withdraw`, + // 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,