MilestonesService.close swallows a failed escrow refund in an empty catch and still marks the milestone CLOSED, stranding the funds

Abierto
  • ¿Algo no está bien?
#729 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
3/5
Tiempo estimado
1-2 días
Aptitud para principiantes
72/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
typescript
Área
backend, payments

Línea de trabajo

Start in src/milestones/milestones.service.ts at MilestonesService.close and read the refund handling alongside the milestone state machine in src/milestones/milestone-state-machine.ts. Then inspect existing escrow failure handling in src/escrow/escrow.service.ts and add close specs in src/milestones/milestones.service.spec.ts. Done means a refund failure is surfaced and the milestone is not saved as CLOSED; cover that behavior with a spec.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

bug

Location

src/milestones/milestones.service.ts:270-279 — MilestonesService.close

if (milestone.escrowId) {
  try {
    await this.escrowService.refund(milestone.escrowId);
  } catch (err) {
    // Escrow may already be refunded or partially released
  }
}

milestone.status = MilestoneStatus.CLOSED;
return this.milestoneRepo.save(milestone);

Problem

The catch block is empty. err is bound and discarded, nothing is logged (MilestonesService has no Logger instance at all), nothing is recorded on the milestone, and execution proceeds straight to writing the terminal CLOSED status.

The comment justifies the swallow for two benign cases ("already refunded", "partially released"), but the catch is not narrowed to them — it absorbs every failure escrowService.refund can throw, including:

  • SorobanPendingError / poll timeout — the refund transaction was submitted and may still land, or may not. EscrowService.invokeOnLockedEscrow (src/escrow/escrow.service.ts:558) records lastFailure in escrow.metadata and rethrows; close() discards the rethrow.
  • a contract-level failure (insufficient balance, wrong network passphrase, unfunded treasury, bad onChainKeyFor key).
  • an RPC transport failure — RPC endpoint down, timeout, 5xx.
  • a Postgres failure inside refund's own escrowRepo.save on line 456.

In all of those the escrow row stays LOCKED and the funds stay on-chain, while the milestone becomes CLOSED.

Why it matters

CLOSED is terminal: MILESTONE_TRANSITIONS[MilestoneStatus.CLOSED] = [] (src/milestones/milestone-state-machine.ts:21). There is no reopen, and close() itself cannot be retried, because the second call hits assertTransition(CLOSED, CLOSED) and throws InvalidMilestoneTransitionError.

So a transient RPC blip during close permanently strands the sponsor's escrowed budget:

  • the milestone says the work is wound up,
  • the escrow row says LOCKED with funds held,
  • no route can refund it (POST /escrow/:id/refund would work, but only if an operator knows to look — nothing surfaces the failure), and
  • there is no log line, no metadata marker on the milestone, and no exception reaching the client, so the API returns 200 OK with status: "closed" as though the refund succeeded.

The reconciliation design the codebase already reaches for elsewhere depends on failures being recorded: EscrowService.fund (lines 149-172) and invokeOnLockedEscrow both deliberately persist a lastFailure entry in escrow.metadata precisely so "a reconciliation job can find and repair the row". close() is the one path that throws that signal away before it reaches anyone.

Reproduction / failure path

  1. Sponsor funds a milestone: POST /milestones/M/fund → escrow E status LOCKED, M.status = FUNDED.
  2. Make the Soroban RPC unavailable, or point STELLAR_RPC_URL at an endpoint that times out — i.e. any condition under which soroban.invoke('refund', ...) throws rather than returning a result.
  3. POST /milestones/M/close.
  4. Observed: 200 OK, body shows status: "closed". Server logs contain nothing about the refund. E.status is still LOCKED; only E.metadata.lastFailure (written by invokeOnLockedEscrow) hints at what happened, and nothing queries it.
  5. Retry POST /milestones/M/close to recover → InvalidMilestoneTransitionError: Cannot transition milestone from "closed" to "closed". The milestone is unrecoverable through the API.

Suggested fix

  1. Do not swallow. Let a refund failure fail the request, so the milestone is not marked CLOSED and the operation stays retryable:
if (milestone.escrowId) {
  await this.escrowService.refundRemaining(milestone.escrowId);
}
milestone.status = MilestoneStatus.CLOSED;
  1. If a tolerated case is genuinely needed, narrow it rather than catching everything — EscrowService.assertLocked throws BadRequestException for an escrow that is already REFUNDED/RELEASED, so only that specific condition should be treated as "nothing left to refund", and it should still be logged:
} catch (err) {
  if (!(err instanceof BadRequestException)) throw err;
  this.logger.warn(
    `Milestone ${id}: escrow ${milestone.escrowId} was not refundable on close: ${(err as Error).message}`,
  );
}

  1. Add a private readonly logger = new Logger(MilestonesService.name) to the service (it currently has none) so these paths are observable.

  2. Wrap the refund and the status = CLOSED write in a single dataSource.transaction so a post-refund DB failure cannot leave the escrow refunded and the milestone still open.

  3. Add specs for close() in src/milestones/milestones.service.spec.ts — the method has no tests today — including one asserting that when escrowService.refund rejects, the request rejects too and milestoneRepo.save is never called with status: 'closed'.

Lenguaje dominante
TypeScript
Estrellas
2
Forks
90
Merge medio
31 min
PR fusionados (30 d)
20

Preparar el entorno

  • Incluye un Dockerfile o un archivo de Docker Compose
  • Sin plantilla de pull request
  • Sin guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de MergeFi/backend

Todos los issues de MergeFi/backend

Issues similares

Más issues de TypeScript

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.