[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

Aperta
  • Qualcosa non va?
#728 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
48/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
node.js, typescript
Ambito
backend, databases, payments

Direzione di ricerca

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.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.
Lingua principale
TypeScript
Stelle
2
Fork
90
Merge medio
31m
PR unite (30g)
20

Preparare l'ambiente

  • Include un Dockerfile o un file Docker Compose
  • Nessun modello di pull request
  • Nessuna guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di MergeFi/backend

Tutte le issue di MergeFi/backend

Issue simili

Altre issue su TypeScript

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.