[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

Đang mở
  • Có gì không ổn?
#728 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức phù hợp với người mới
48/100
Loại issue
Lỗi
Độ rõ ràng
Đặc tả rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
node.js, typescript
Lĩnh vực
backend, databases, payments

Hướng nghiên cứu

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.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

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.
Ngôn ngữ chính
TypeScript
Star
2
Fork
90
Merge trung bình
31 phút
Pull request đã merge (30 ngày)
20

Chuẩn bị môi trường

  • Có Dockerfile hoặc tệp Docker Compose
  • Không có mẫu pull request
  • Không có hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của MergeFi/backend

Tất cả issue của MergeFi/backend

Issue tương tự

Thêm issue về TypeScript

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.