diff --git a/apps/api/src/combat/combat.service.spec.ts b/apps/api/src/combat/combat.service.spec.ts index 2afeabf..200d80c 100644 --- a/apps/api/src/combat/combat.service.spec.ts +++ b/apps/api/src/combat/combat.service.spec.ts @@ -549,7 +549,10 @@ describe('CombatService', () => { }); it('rejects actions from a character who does not own the combat', async () => { - const { service, combatId } = await startedCombat(); + const state = createState({ + characters: [character(), character({ id: OTHER_CHARACTER_ID })], + }); + const { service, combatId } = await startedCombat(state); await expectCombatDomainError( service.performAction( @@ -724,9 +727,14 @@ describe('CombatService', () => { expect(result.status).toBe(CombatStatus.WON); expect(result.rewards).toEqual({ experience: 8, silver: 6, items: [] }); expect(rewards.grantVictoryRewards).toHaveBeenCalledTimes(1); - // The reward service receives the transaction manager, not the data source. + // The reward service must receive the transaction manager, not the data + // source: `expect.anything()` would pass even if the code handed over + // `this.dataSource`, so assert on the captured argument's identity. + const passedManager = (rewards.grantVictoryRewards as jest.Mock).mock + .calls[0][0]; + expect(passedManager).not.toBe(dataSource); expect(rewards.grantVictoryRewards).toHaveBeenCalledWith( - expect.anything(), + passedManager, expect.objectContaining({ id: 'combat-1', status: CombatStatus.WON }), ); }); @@ -854,7 +862,22 @@ describe('CombatService', () => { new CombatEngineService(), new CharacterCombatStatsService(), fakeRewardService({ - grantVictoryRewards: jest.fn().mockRejectedValue(new Error('reward persistence failed')), + // Genuinely write XP/silver through the transaction's manager + // before failing, so the assertions below prove the rollback + // discards those writes rather than passing vacuously because + // nothing was ever written. + grantVictoryRewards: jest.fn().mockImplementation(async (manager: EntityManager) => { + const characters = manager.getRepository(Character); + const combatCharacter = await characters.findOneBy({ + id: CHARACTER_ID, + }); + if (combatCharacter) { + combatCharacter.experience += 8; + combatCharacter.silver += 6; + await characters.save(combatCharacter); + } + throw new Error('reward persistence failed'); + }), }), ); diff --git a/apps/api/src/combat/combat.service.ts b/apps/api/src/combat/combat.service.ts index f71013a..d8f566d 100644 --- a/apps/api/src/combat/combat.service.ts +++ b/apps/api/src/combat/combat.service.ts @@ -197,9 +197,18 @@ export class CombatService { action: CombatAction, ): Promise { 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' },