From a1241a91563fb1e2fe2ac6ffe97b1795d1bd6dd4 Mon Sep 17 00:00:00 2001 From: Zihao Xu Date: Mon, 19 Jan 2026 23:32:14 -0800 Subject: [PATCH] fix: make cooldown_action decorator properly await async finish() (#75) * fix: make cooldown_action decorator properly await async finish() The decorator was incorrectly wrapping the async finish() method with a sync wrapper, causing cooldown to be recorded BEFORE the action actually executed. This fix: - Changes wrapper to async def finish() - Awaits original_finish() before recording cooldown - Ensures cooldown is only recorded on successful execution Fixes #74 * test: add test to verify cooldown not recorded on failure This test actually reveals the async/await bug: - Creates a FailingAction that raises in finish() - Verifies cooldown is NOT recorded when action fails - Fails with buggy sync wrapper, passes with async fix --- src/classes/action/cooldown.py | 6 +- tests/test_cooldown.py | 138 +++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 tests/test_cooldown.py diff --git a/src/classes/action/cooldown.py b/src/classes/action/cooldown.py index 58dc91c..460c7a9 100644 --- a/src/classes/action/cooldown.py +++ b/src/classes/action/cooldown.py @@ -34,8 +34,10 @@ def cooldown_action(cls: type) -> type: if hasattr(cls, "finish"): original_finish = cls.finish - def finish(self, **params): # type: ignore[no-redef] - events = original_finish(self, **params) + async def finish(self, **params): # type: ignore[no-redef] + # Must await original_finish first, then record cooldown. + # This ensures cooldown is only recorded after finish() succeeds. + events = await original_finish(self, **params) last_map = getattr(self.avatar, "_action_cd_last_months", None) if last_map is not None: last_map[self.__class__.__name__] = self.world.month_stamp diff --git a/tests/test_cooldown.py b/tests/test_cooldown.py new file mode 100644 index 0000000..f1eb35c --- /dev/null +++ b/tests/test_cooldown.py @@ -0,0 +1,138 @@ +""" +Tests for src/classes/action/cooldown.py + +Tests the cooldown_action decorator behavior. +""" + +import pytest +from unittest.mock import MagicMock, patch + +from src.classes.action.cooldown import cooldown_action +from src.classes.mutual_action.impart import Impart + + +class TestCooldownAction: + """Tests for cooldown_action decorator.""" + + @pytest.mark.asyncio + async def test_cooldown_recorded_after_finish_executes(self): + """ + Test that cooldown is recorded AFTER finish() executes, not before. + + This was a bug where the sync wrapper recorded cooldown before + awaiting the async original_finish(). + """ + world = MagicMock() + world.month_stamp = 100 + + master = MagicMock() + master.name = "Master" + master._action_cd_last_months = {} + + action = Impart(master, world) + action._impart_success = True + action._impart_exp_gain = 2000 + + target = MagicMock() + target.name = "Disciple" + target.id = "disciple_id" + + # Before calling finish, no cooldown recorded. + assert "Impart" not in master._action_cd_last_months + + # Call finish and await it. + events = await action.finish(target_avatar=target) + + # After await, cooldown should be recorded. + assert "Impart" in master._action_cd_last_months + assert master._action_cd_last_months["Impart"] == 100 + + def test_cooldown_check_in_can_start(self): + """Test that can_start checks cooldown correctly.""" + world = MagicMock() + world.month_stamp = 100 + + master = MagicMock() + master.name = "Master" + master._action_cd_last_months = {"Impart": 98} # Used 2 months ago + + action = Impart(master, world) + + # Impart has 6 month cooldown, only 2 months passed. + can_start, reason = action.can_start(target_avatar=MagicMock()) + + assert can_start is False + assert "冷却中" in reason + assert "4" in reason # 6 - 2 = 4 months remaining + + def test_cooldown_expired_allows_start(self): + """Test that can_start allows action after cooldown expires.""" + world = MagicMock() + world.month_stamp = 110 + + master = MagicMock() + master.name = "Master" + master.get_relation = MagicMock(return_value=None) + master._action_cd_last_months = {"Impart": 100} # Used 10 months ago + master.cultivation_progress = MagicMock() + master.cultivation_progress.level = 50 + + action = Impart(master, world) + + target = MagicMock() + target.is_dead = False + + # Impart has 6 month cooldown, 10 months passed - should be allowed. + # Note: will fail on other checks (relation), but not on cooldown. + with patch("src.classes.observe.is_within_observation", return_value=True): + can_start, reason = action.can_start(target_avatar=target) + + # Should not fail due to cooldown. + assert "冷却" not in reason + + @pytest.mark.asyncio + async def test_cooldown_not_recorded_on_finish_failure(self): + """ + Test that cooldown is NOT recorded if finish() raises an exception. + + This is the key test that reveals the async/await bug: + - Buggy code: records cooldown BEFORE awaiting, so cooldown is recorded even on failure + - Fixed code: awaits first, so exception prevents cooldown from being recorded + """ + + class ActionError(Exception): + pass + + # Create a simple action class with finish that raises. + @cooldown_action + class FailingAction: + ACTION_CD_MONTHS = 6 + + def __init__(self, avatar, world): + self.avatar = avatar + self.world = world + + def can_start(self, **params): + return True, "" + + async def finish(self, **params): + raise ActionError("Action failed!") + + world = MagicMock() + world.month_stamp = 100 + + avatar = MagicMock() + avatar.name = "Test" + avatar._action_cd_last_months = {} + + action = FailingAction(avatar, world) + + # Before calling finish, no cooldown recorded. + assert "FailingAction" not in avatar._action_cd_last_months + + # Call finish - should raise. + with pytest.raises(ActionError): + await action.finish() + + # Cooldown should NOT be recorded because action failed. + assert "FailingAction" not in avatar._action_cd_last_months