fix(maintenance-pool): restore balance on poolWithdraw failure

src/escrow/escrow.service.ts

@@ -372,6 +372,32 @@ export class EscrowService {
     );
   }
 
+  /**
+   * Validates the preconditions for a pool withdrawal without touching the
+   * DB balance — called by {@link MaintenancePoolService.assignReward} *before*
+   * the atomic balance decrement so that cheap, infallible-to-reverse errors
+   * (bad amount format, recipient/user mismatch, escrow not LOCKED) never
+   * cause the pool's DB balance to drift below the real on-chain balance.
+   *
+   * Deliberately mirrors the first three checks inside {@link poolWithdraw}
+   * exactly, so a caller that passes these will not encounter the same
+   * failures a second time inside poolWithdraw itself (barring a race on the
+   * escrow status between the two calls, which is an acceptable residual risk
+   * given that the pool already guards against concurrent payouts at the
+   * balance level).
+   */
+  async assertPoolWithdrawPreconditions(
+    escrowId: string,
+    amount: string,
+    recipientAddress: string,
+    recipientId?: string,
+  ): Promise<void> {
+    const escrow = await this.getOrThrow(escrowId);
+    this.assertLocked(escrow);
+    this.assertValidAmount(amount);
+    await this.assertRecipientsMatchUsers([{ recipientAddress, recipientId }]);
+  }
+
   /**
    * Pays a reward out of a maintenance pool's running balance.
    *
@@ -584,20 +610,14 @@ export class EscrowService {
     recipients: Array<[string, number]>,
     manager?: EntityManager,
   ): Promise<ContractInvocationResult> {
-    return this.invokeOnLockedEscrow(escrow, operation, () =>
-      this.soroban.invoke(
-        'release',
-        // `release(issue_id: u64, recipients)` — u64-typed on-chain (#301).
-        [u64(this.onChainKeyFor(escrow)), recipients],
-        this.contractOpts(escrow),
-      ),
     return this.invokeOnLockedEscrow(
       escrow,
       operation,
       () =>
         this.soroban.invoke(
           'release',
-          [this.onChainKeyFor(escrow), recipients],
+          // `release(issue_id: u64, recipients)` — u64-typed on-chain (#301).
+          [u64(this.onChainKeyFor(escrow)), recipients],
           this.contractOpts(escrow),
         ),
       manager,

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

@@ -18,7 +18,7 @@ describe('MaintenancePoolService', () => {
     decrement: jest.Mock;
     createQueryBuilder: jest.Mock;
   };
-  let escrowService: { fund: jest.Mock; poolWithdraw: jest.Mock };
+  let escrowService: { fund: jest.Mock; poolWithdraw: jest.Mock; assertPoolWithdrawPreconditions: jest.Mock };
   let issueRepo: { findOne: jest.Mock };
   let paymentRepo: {
     findOne: jest.Mock;
@@ -41,6 +41,7 @@ describe('MaintenancePoolService', () => {
     escrowService = {
       fund: jest.fn(),
       poolWithdraw: jest.fn(),
+      assertPoolWithdrawPreconditions: jest.fn().mockResolvedValue(undefined),
     };
     issueRepo = {
       findOne: jest.fn().mockResolvedValue({
@@ -339,6 +340,7 @@ describe('MaintenancePoolService', () => {
         id: 'pool-1',
         balance: '100',
         escrowId: 'escrow-1',
+        status: MaintenancePoolStatus.ACTIVE,
       });
       // Mock the atomic balance check to succeed
       const mockQueryBuilder = {
@@ -412,6 +414,7 @@ describe('MaintenancePoolService', () => {
         id: 'pool-1',
         balance: '100',
         escrowId: 'escrow-1',
+        status: MaintenancePoolStatus.ACTIVE,
       });
       // Mock the payment query to return an existing payment
       const mockPaymentQueryBuilder = {
@@ -442,7 +445,7 @@ describe('MaintenancePoolService', () => {
         escrowId: 'escrow-1',
       };
       poolRepo.findOne.mockImplementation(() =>
-        Promise.resolve({ id: 'pool-1', ...sharedPoolRow }),
+        Promise.resolve({ id: 'pool-1', status: MaintenancePoolStatus.ACTIVE, ...sharedPoolRow }),
       );
       // Mock the atomic balance check to succeed for both calls
       const mockQueryBuilder = {
@@ -463,5 +466,125 @@ describe('MaintenancePoolService', () => {
       // Both calls should have succeeded (atomic check passed)
       expect(mockQueryBuilder.execute).toHaveBeenCalledTimes(2);
     });
+
+    // Balance-leak regression tests (#issue): any throw inside poolWithdraw
+    // after the balance has been decremented must trigger an increment
+    // restoration so the pool's DB balance never drifts below the real
+    // on-chain balance.
+
+    describe('balance restoration on poolWithdraw failure', () => {
+      let mockQueryBuilder: {
+        update: jest.Mock;
+        set: jest.Mock;
+        where: jest.Mock;
+        setParameter: jest.Mock;
+        execute: jest.Mock;
+      };
+
+      beforeEach(() => {
+        poolRepo.findOne.mockResolvedValue({
+          id: 'pool-1',
+          balance: '100',
+          escrowId: 'escrow-1',
+          status: MaintenancePoolStatus.ACTIVE,
+        });
+        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);
+      });
+
+      it('restores pool balance when poolWithdraw throws a Soroban invocation error', async () => {
+        const sorobanError = new Error('Soroban simulate failed: insufficient fee');
+        escrowService.poolWithdraw.mockRejectedValue(sorobanError);
+
+        await expect(
+          service.assignReward('pool-1', 'issue-1', '30', 'GRECIPIENT', 'user-1'),
+        ).rejects.toThrow('Soroban simulate failed: insufficient fee');
+
+        // Balance must be restored via increment after the decrement
+        expect(poolRepo.increment).toHaveBeenCalledWith(
+          { id: 'pool-1' },
+          'balance',
+          30,
+        );
+      });
+
+      it('restores pool balance when poolWithdraw throws a paymentRepo.save error', async () => {
+        const dbError = new Error('connection terminated unexpectedly');
+        escrowService.poolWithdraw.mockRejectedValue(dbError);
+
+        await expect(
+          service.assignReward('pool-1', 'issue-1', '50', 'GRECIPIENT'),
+        ).rejects.toThrow('connection terminated unexpectedly');
+
+        expect(poolRepo.increment).toHaveBeenCalledWith(
+          { id: 'pool-1' },
+          'balance',
+          50,
+        );
+      });
+
+      it('restores the exact numeric amount that was decremented', async () => {
+        escrowService.poolWithdraw.mockRejectedValue(new Error('tx failed'));
+
+        await expect(
+          service.assignReward('pool-1', 'issue-1', '12.5', 'GRECIPIENT'),
+        ).rejects.toThrow();
+
+        // Number('12.5') === 12.5 — must match what the decrement used
+        expect(poolRepo.increment).toHaveBeenCalledWith(
+          { id: 'pool-1' },
+          'balance',
+          12.5,
+        );
+      });
+
+      it('does NOT call increment when poolWithdraw succeeds', async () => {
+        escrowService.poolWithdraw.mockResolvedValue({ id: 'payment-1' });
+
+        await service.assignReward('pool-1', 'issue-1', '30', 'GRECIPIENT', 'user-1');
+
+        expect(poolRepo.increment).not.toHaveBeenCalled();
+      });
+
+      it('rethrows the original poolWithdraw error after restoring balance', async () => {
+        const originalError = new BadRequestException('recipient mismatch');
+        escrowService.poolWithdraw.mockRejectedValue(originalError);
+
+        const thrown = await service
+          .assignReward('pool-1', 'issue-1', '10', 'GRECIPIENT')
+          .catch((e) => e);
+
+        // The caller sees the original error, not a wrapped one
+        expect(thrown).toBe(originalError);
+        // And the balance was still restored
+        expect(poolRepo.increment).toHaveBeenCalledWith(
+          { id: 'pool-1' },
+          'balance',
+          10,
+        );
+      });
+
+      it('does NOT decrement at all when assertPoolWithdrawPreconditions rejects before the decrement', async () => {
+        // Cheap pre-decrement validation fires — no balance should be touched
+        escrowService.assertPoolWithdrawPreconditions.mockRejectedValue(
+          new BadRequestException('recipientAddress does not match Stellar address'),
+        );
+
+        await expect(
+          service.assignReward('pool-1', 'issue-1', '10', 'GSTALE_ADDRESS', 'user-1'),
+        ).rejects.toThrow('recipientAddress does not match Stellar address');
+
+        // The atomic decrement was never attempted
+        expect(mockQueryBuilder.execute).not.toHaveBeenCalled();
+        // And no restoration increment is needed (nothing was decremented)
+        expect(poolRepo.increment).not.toHaveBeenCalled();
+      });
+    });
   });
 });

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

@@ -164,6 +164,18 @@ export class MaintenancePoolService {
       );
     }
 
+    // Run cheap preconditions (escrow LOCKED, valid amount, recipient/user
+    // match) *before* the balance decrement so that bad-input errors never
+    // leave the pool's DB balance below the real on-chain balance (#issue).
+    // assertPoolWithdrawPreconditions mirrors the first three guards inside
+    // poolWithdraw exactly, so if they pass here they won't fire again there.
+    await this.escrowService.assertPoolWithdrawPreconditions(
+      pool.escrowId,
+      amount,
+      recipientAddress,
+      recipientId,
+    );
+
     // 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.
@@ -185,14 +197,25 @@ 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;
+    //
+    // If poolWithdraw throws for any reason — escrow status race, Soroban
+    // simulate/send/poll failure, paymentRepo.save failure — the balance
+    // decrement above has already been applied but no USDC has left the
+    // contract (the escrow stays LOCKED either way). Restore the reservation
+    // atomically so the pool's DB balance never permanently drifts below the
+    // real on-chain balance (#issue). The original error is rethrown so the
+    // caller observes a clean failure.
+    try {
+      return await this.escrowService.poolWithdraw(
+        pool.escrowId,
+        amount,
+        recipientAddress,
+        recipientId,
+      );
+    } catch (err) {
+      await this.poolRepo.increment({ id }, 'balance', Number(amount));
+      throw err;
+    }
   }
 
   async list(): Promise<MaintenancePool[]> {