fix(combat): lock character before combat row and harden reward-transaction test

Fix a lock-order inversion Task 8 introduced: performAction locked the
combat row first and, inside grantVictoryRewards, the character row
second -- the opposite order to startCombat (character, then combat),
creating a deadlock cycle for two concurrent requests on the same
character. performAction now locks the character first via the
existing lockCharacter helper, matching startCombat; the later re-lock
inside grantVictoryRewards is a no-op within the same transaction.

Also strengthen the test that guards the transaction contract for
grantVictoryRewards: expect.anything() would have passed even if the
data source were handed over instead of the transaction manager, since
CombatRewardService has no runtime guard against that substitution.
The test now asserts on the captured argument's identity. Verified
this is load-bearing by temporarily passing the data source in place
of the manager and confirming the test fails.

Finally, make the rollback test's unchanged-XP/silver assertions real:
the fake grantVictoryRewards now writes through the transaction
manager before throwing, so the assertions prove the rollback
discarded those writes instead of passing vacuously.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Bastian Wagner
2026-08-20 09:06:50 +02:00
parent 2a8883d479
commit 40e830a321
2 changed files with 36 additions and 4 deletions

View File

@@ -197,9 +197,18 @@ export class CombatService {
action: CombatAction,
): Promise<CombatDto> {
return this.dataSource.transaction(async (manager) => {
const characters = manager.getRepository(Character);
const combats = manager.getRepository(Combat);
const combatEvents = manager.getRepository(CombatEvent);
// Lock the character before the combat row here, matching the order
// startCombat already uses (character, then combat). grantVictoryRewards
// locks the character again later in this same transaction, which is a
// no-op re-lock — but locking it first here keeps both code paths
// consistent and avoids a lock-order inversion that could deadlock two
// concurrent requests against the same character. Do not reorder this.
await this.lockCharacter(characters, characterId);
const combat = await combats.findOne({
where: { id: combatId, characterId },
lock: { mode: 'pessimistic_write' },