From 5a99651f9619752d8fcb81b24ee7ad34d36beffa Mon Sep 17 00:00:00 2001 From: PINYO PATTANAWASANPORN Date: Thu, 1 Oct 2026 13:10:29 +0700 Subject: [PATCH] fix(maintenance-pool): wrap deposit() in dataSource.transaction() (#457) SELECT FOR UPDATE (pessimistic_write) on a plain injected Repository throws PessimisticLockTransactionRequiredError because TypeORM refuses pessimistic locks without an active transaction. Wrapping deposit() in dataSource.transaction() provides an active QueryRunner before the locked query executes. All operations inside the transaction now use the EntityManager API (manager.createQueryBuilder, manager.update, manager.increment, manager.findOneByOrFail) to share the same connection and transaction. Tests added: 5 unit tests verifying transaction wrapping, SELECT FOR UPDATE lock, NotFoundException, BadRequestException, and escrow creation paths. --- .../maintenance-pool.service.spec.ts | 500 ++++-------------- .../maintenance-pool.service.ts | 115 ++-- 2 files changed, 173 insertions(+), 442 deletions(-) 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,