[Regression on closed #536 / #375] TeamsService.updateSplits still deletes and re-inserts splits outside a transaction, so a failed insert leaves the team with zero splits

Abierto
  • ¿Algo no está bien?
#728 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
4/5
Tiempo estimado
3-5 días
Aptitud para principiantes
48/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
node.js, typescript
Área
backend, databases, payments

Línea de trabajo

Read src/teams/teams.service.ts around TeamsService.updateSplits and review the service specs for split updates. Trace how the member existence checks and repository delete/save calls are performed, then add a regression spec that makes the save fail and verifies the existing splits remain. Done when the method keeps the prior splits on failure and the new spec passes.

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

Descripción

bug

Location — src/teams/teams.service.ts:90-102 (TeamsService.updateSplits).

Problem — The delete-then-insert sequence closed as #375, and re-filed and closed again as #536, is back:

// Remove existing splits
await this.splitRepo.delete({ teamId: team.id });

// Create new splits — one batched save, same rationale as create() (#150).
team.splits = await this.splitRepo.save(
  members.map((m) => this.splitRepo.create({ ... })),
);

splitRepo.delete(...) and splitRepo.save(...) are two independent repository calls on the default EntityManager. Each gets its own implicit transaction and commits on its own. The in-code comment is misleading: TypeORM's implicit transaction wraps the batched save only, so it protects against a partially-inserted new set, but nothing rolls back the already-committed DELETE when the subsequent save fails. No dataSource.transaction(...), queryRunner, or injected EntityManager appears anywhere in this file.

Why it matters — A failed insert leaves the team in a state the application treats as valid but which silently skips payouts: team_member_splits has zero rows for the team, while the team is still attached to its bounty. The payout path reads team.splits and, with an empty array, the bounty is marked PAID with no money distributed (the behaviour of #102/#367). The original percentages are already deleted and cannot be recovered. The insert can realistically fail: the CHK_team_member_splits_percentage_range CHECK added in 1785200000000-AddSplitPercentageChecks.ts, the team_member_splits.userId FK if a referenced user is deleted between the pre-check loop and the insert, a statement timeout, or a dropped connection mid-request.

Reproduction / failure path

  1. Create team T with members B 60% / C 40%.
  2. Call PATCH /teams/<T>/splits and have the insert fail — easiest deterministic repro: delete user C's row after the per-member existence loop at lines 80-87 passes but before the save (the loop and the insert are not in one transaction either), so the FK on userId rejects the insert.
  3. The request returns 500. Query select count(*) from team_member_splits where "teamId" = '<T>' — zero rows.
  4. Merge a bounty assigned to T: it transitions to PAID and no payment rows are created.

Suggested fix

  • Wrap the whole method body in a single transaction: inject DataSource (or @InjectEntityManager()) and run manager.transaction(async (m) => { ... }), doing the member-existence check, the m.delete(TeamMemberSplit, { teamId }) and the m.save(TeamMemberSplit, rows) on that one manager so a failure rolls the delete back.
  • Do the member-existence check with a single In([...userIds]) query inside the same transaction rather than a per-member loop.
  • Add a service spec that makes the save reject and asserts the pre-existing splits are still present afterwards — neither #375's nor #536's fix shipped such a test, which is why the same code has now regressed twice.
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.