From 307e10c144fd66c7821dd9fae06bff4f1ee9b9e5 Mon Sep 17 00:00:00 2001 From: Ioodu Date: Sun, 12 Apr 2026 11:13:55 +0800 Subject: [PATCH 1/3] fix(gateway): clean up _running_agents_ts on agent removal paths When agents were removed from _running_agents (via /stop, /new, /resume, shutdown, or hard-stop of pending agents), the corresponding entry in _running_agents_ts was not cleaned up, causing orphaned timestamps that accumulate as a memory leak and can corrupt stale-timeout checks. Add _running_agents_ts.pop() after every del _running_agents[key] and _running_agents_ts.clear() after _running_agents.clear() in shutdown. --- .../gateway/test_running_agents_ts_cleanup.py | 162 ++++++++++++++++++ 1 file changed, 162 insertions(+) create mode 100644 tests/gateway/test_running_agents_ts_cleanup.py diff --git a/tests/gateway/test_running_agents_ts_cleanup.py b/tests/gateway/test_running_agents_ts_cleanup.py new file mode 100644 index 000000000000..cfd37f51506a --- /dev/null +++ b/tests/gateway/test_running_agents_ts_cleanup.py @@ -0,0 +1,162 @@ +"""Tests for _running_agents_ts cleanup — ensures timestamp entries are +removed whenever their corresponding _running_agents entry is deleted. + +When an agent entry is removed from _running_agents (stop, new, resume, +shutdown), the matching _running_agents_ts entry must also be cleaned up. +Orphaned timestamps cause memory leaks and can corrupt stale-timeout checks +if session keys are reused. +""" + +import re +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from gateway.config import GatewayConfig, Platform, PlatformConfig +from gateway.run import GatewayRunner + + +def _make_runner(tmp_path) -> GatewayRunner: + config = GatewayConfig( + platforms={Platform.TELEGRAM: PlatformConfig(enabled=True, token="t")}, + sessions_dir=tmp_path / "sessions", + ) + runner = GatewayRunner(config) + # Prevent real adapter creation + runner._create_adapter = MagicMock(return_value=MagicMock()) + return runner + + +class TestRunningAgentsTsCleanup: + def test_stop_command_cleans_ts(self, tmp_path): + """Deleting from _running_agents on /stop must also pop _running_agents_ts.""" + runner = _make_runner(tmp_path) + key = "chat_123" + runner._running_agents[key] = MagicMock() + runner._running_agents_ts[key] = 1000.0 + + # Simulate what the stop path does + if key in runner._running_agents: + del runner._running_agents[key] + runner._running_agents_ts.pop(key, None) + + assert key not in runner._running_agents + assert key not in runner._running_agents_ts + + def test_new_command_cleans_ts(self, tmp_path): + """Deleting from _running_agents on /new must also pop _running_agents_ts.""" + runner = _make_runner(tmp_path) + key = "chat_456" + runner._running_agents[key] = MagicMock() + runner._running_agents_ts[key] = 2000.0 + + if key in runner._running_agents: + del runner._running_agents[key] + runner._running_agents_ts.pop(key, None) + + assert key not in runner._running_agents + assert key not in runner._running_agents_ts + + def test_resume_command_cleans_ts(self, tmp_path): + """Deleting from _running_agents on /resume must also pop _running_agents_ts.""" + runner = _make_runner(tmp_path) + key = "chat_789" + runner._running_agents[key] = MagicMock() + runner._running_agents_ts[key] = 3000.0 + + if key in runner._running_agents: + del runner._running_agents[key] + runner._running_agents_ts.pop(key, None) + + assert key not in runner._running_agents + assert key not in runner._running_agents_ts + + def test_shutdown_clears_ts(self, tmp_path): + """_running_agents.clear() must be followed by _running_agents_ts.clear().""" + runner = _make_runner(tmp_path) + runner._running_agents = {"a": MagicMock(), "b": MagicMock()} + runner._running_agents_ts = {"a": 100.0, "b": 200.0} + + runner._running_agents.clear() + runner._running_agents_ts.clear() + + assert len(runner._running_agents) == 0 + assert len(runner._running_agents_ts) == 0 + + def test_all_del_sites_have_ts_pop(self): + """Source-level check: every `del self._running_agents[...]` must be + followed (within a few lines) by `self._running_agents_ts.pop(...)`. + This catches sites that were missed during code review.""" + import gateway.run as mod + + source = open(mod.__file__).read() + + # Find all del self._running_agents[...] lines + del_pattern = re.compile( + r'del\s+self\._running_agents\[(\w+)\]' + ) + # Find all self._running_agents_ts.pop(...) lines + pop_pattern = re.compile( + r'self\._running_agents_ts\.pop\(' + ) + + del_lines = [] + pop_lines = [] + for i, line in enumerate(source.splitlines(), 1): + # Only match actual del statements (not docstring references) + if del_pattern.search(line) and line.lstrip().startswith("del "): + del_lines.append(i) + if pop_pattern.search(line): + pop_lines.append(i) + + # For each del line, check that there's a pop within 5 lines after it + missing = [] + for del_line in del_lines: + # Check if any pop line is within 5 lines after the del + found = any( + pop_line > del_line and pop_line <= del_line + 5 + for pop_line in pop_lines + ) + if not found: + # Also check if the del is inside a .clear() block + # (which has its own test) + line_text = source.splitlines()[del_line - 1] + if '.clear()' not in line_text: + missing.append(del_line) + + assert missing == [], ( + f"Lines with `del self._running_agents[...]` but no " + f"`_running_agents_ts.pop` within 5 lines: {missing}" + ) + + def test_clear_site_also_clears_ts(self): + """Source-level check: every `self._running_agents.clear()` must be + followed by `self._running_agents_ts.clear()`.""" + import gateway.run as mod + + source = open(mod.__file__).read() + + clear_pattern = re.compile(r'self\._running_agents\.clear\(\)') + ts_clear_pattern = re.compile(r'self\._running_agents_ts\.clear\(\)') + + clear_lines = [] + ts_clear_lines = [] + for i, line in enumerate(source.splitlines(), 1): + if clear_pattern.search(line): + clear_lines.append(i) + if ts_clear_pattern.search(line): + ts_clear_lines.append(i) + + missing = [] + for clear_line in clear_lines: + found = any( + ts_line > clear_line and ts_line <= clear_line + 5 + for ts_line in ts_clear_lines + ) + if not found: + missing.append(clear_line) + + assert missing == [], ( + f"Lines with `_running_agents.clear()` but no " + f"`_running_agents_ts.clear()` within 5 lines: {missing}" + ) From 044cb2c7812f73226c8b4f84716329e7d4397d0a Mon Sep 17 00:00:00 2001 From: Ioodu Date: Mon, 27 Apr 2026 16:19:11 +0800 Subject: [PATCH 2/3] fix(gateway): strengthen _running_agents_ts cleanup tests - Replace tests that simulated cleanup inline with real _release_running_agent_state calls - Remove vacuous test_all_del_sites_have_ts_pop (trivially passes, redundant) - Add test_release_also_cleans_busy_ack_ts asserting all three dicts cleared - Remove unused AsyncMock import --- .../gateway/test_running_agents_ts_cleanup.py | 81 +++++-------------- 1 file changed, 21 insertions(+), 60 deletions(-) diff --git a/tests/gateway/test_running_agents_ts_cleanup.py b/tests/gateway/test_running_agents_ts_cleanup.py index cfd37f51506a..8882f4f2a1db 100644 --- a/tests/gateway/test_running_agents_ts_cleanup.py +++ b/tests/gateway/test_running_agents_ts_cleanup.py @@ -8,7 +8,7 @@ """ import re -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import MagicMock import pytest @@ -29,48 +29,55 @@ def _make_runner(tmp_path) -> GatewayRunner: class TestRunningAgentsTsCleanup: def test_stop_command_cleans_ts(self, tmp_path): - """Deleting from _running_agents on /stop must also pop _running_agents_ts.""" + """_release_running_agent_state (the /stop path) must pop _running_agents_ts.""" runner = _make_runner(tmp_path) key = "chat_123" runner._running_agents[key] = MagicMock() runner._running_agents_ts[key] = 1000.0 - # Simulate what the stop path does - if key in runner._running_agents: - del runner._running_agents[key] - runner._running_agents_ts.pop(key, None) + runner._release_running_agent_state(key) assert key not in runner._running_agents assert key not in runner._running_agents_ts def test_new_command_cleans_ts(self, tmp_path): - """Deleting from _running_agents on /new must also pop _running_agents_ts.""" + """_release_running_agent_state (the /new path) must pop _running_agents_ts.""" runner = _make_runner(tmp_path) key = "chat_456" runner._running_agents[key] = MagicMock() runner._running_agents_ts[key] = 2000.0 - if key in runner._running_agents: - del runner._running_agents[key] - runner._running_agents_ts.pop(key, None) + runner._release_running_agent_state(key) assert key not in runner._running_agents assert key not in runner._running_agents_ts def test_resume_command_cleans_ts(self, tmp_path): - """Deleting from _running_agents on /resume must also pop _running_agents_ts.""" + """_release_running_agent_state (the /resume path) must pop _running_agents_ts.""" runner = _make_runner(tmp_path) key = "chat_789" runner._running_agents[key] = MagicMock() runner._running_agents_ts[key] = 3000.0 - if key in runner._running_agents: - del runner._running_agents[key] - runner._running_agents_ts.pop(key, None) + runner._release_running_agent_state(key) assert key not in runner._running_agents assert key not in runner._running_agents_ts + def test_release_also_cleans_busy_ack_ts(self, tmp_path): + """_release_running_agent_state must pop all three tracking dicts atomically.""" + runner = _make_runner(tmp_path) + key = "chat_999" + runner._running_agents[key] = MagicMock() + runner._running_agents_ts[key] = 4000.0 + runner._busy_ack_ts[key] = 4000.0 + + runner._release_running_agent_state(key) + + assert key not in runner._running_agents + assert key not in runner._running_agents_ts + assert key not in runner._busy_ack_ts + def test_shutdown_clears_ts(self, tmp_path): """_running_agents.clear() must be followed by _running_agents_ts.clear().""" runner = _make_runner(tmp_path) @@ -83,52 +90,6 @@ def test_shutdown_clears_ts(self, tmp_path): assert len(runner._running_agents) == 0 assert len(runner._running_agents_ts) == 0 - def test_all_del_sites_have_ts_pop(self): - """Source-level check: every `del self._running_agents[...]` must be - followed (within a few lines) by `self._running_agents_ts.pop(...)`. - This catches sites that were missed during code review.""" - import gateway.run as mod - - source = open(mod.__file__).read() - - # Find all del self._running_agents[...] lines - del_pattern = re.compile( - r'del\s+self\._running_agents\[(\w+)\]' - ) - # Find all self._running_agents_ts.pop(...) lines - pop_pattern = re.compile( - r'self\._running_agents_ts\.pop\(' - ) - - del_lines = [] - pop_lines = [] - for i, line in enumerate(source.splitlines(), 1): - # Only match actual del statements (not docstring references) - if del_pattern.search(line) and line.lstrip().startswith("del "): - del_lines.append(i) - if pop_pattern.search(line): - pop_lines.append(i) - - # For each del line, check that there's a pop within 5 lines after it - missing = [] - for del_line in del_lines: - # Check if any pop line is within 5 lines after the del - found = any( - pop_line > del_line and pop_line <= del_line + 5 - for pop_line in pop_lines - ) - if not found: - # Also check if the del is inside a .clear() block - # (which has its own test) - line_text = source.splitlines()[del_line - 1] - if '.clear()' not in line_text: - missing.append(del_line) - - assert missing == [], ( - f"Lines with `del self._running_agents[...]` but no " - f"`_running_agents_ts.pop` within 5 lines: {missing}" - ) - def test_clear_site_also_clears_ts(self): """Source-level check: every `self._running_agents.clear()` must be followed by `self._running_agents_ts.clear()`.""" From 43ae049aa6e0a330c5ea3cc13fb5dfc45a6e9ca2 Mon Sep 17 00:00:00 2001 From: Ioodu Date: Mon, 27 Apr 2026 21:18:04 +0800 Subject: [PATCH 3/3] fix: strengthen test_shutdown_clears_ts to verify production code The previous test called .clear() on the dicts in the test body itself, trivially passing without testing any production code path. Replace it with a source-level assertion that the shutdown block in gateway/run.py contains _running_agents_ts.clear() after _running_agents.clear() in the expected order. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../gateway/test_running_agents_ts_cleanup.py | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/tests/gateway/test_running_agents_ts_cleanup.py b/tests/gateway/test_running_agents_ts_cleanup.py index 8882f4f2a1db..aa80ce7d05df 100644 --- a/tests/gateway/test_running_agents_ts_cleanup.py +++ b/tests/gateway/test_running_agents_ts_cleanup.py @@ -79,16 +79,28 @@ def test_release_also_cleans_busy_ack_ts(self, tmp_path): assert key not in runner._busy_ack_ts def test_shutdown_clears_ts(self, tmp_path): - """_running_agents.clear() must be followed by _running_agents_ts.clear().""" - runner = _make_runner(tmp_path) - runner._running_agents = {"a": MagicMock(), "b": MagicMock()} - runner._running_agents_ts = {"a": 100.0, "b": 200.0} + """Source-level check: the shutdown path must clear _running_agents_ts + immediately after clearing _running_agents. - runner._running_agents.clear() - runner._running_agents_ts.clear() + The test verifies that the production code in gateway/run.py contains + the paired clear() calls in the expected order, rather than testing the + trivially-correct operation of dict.clear() itself. + """ + import gateway.run as mod - assert len(runner._running_agents) == 0 - assert len(runner._running_agents_ts) == 0 + source = open(mod.__file__).read() + # Locate the shutdown clear block — both clears must appear together + idx_agents = source.find("self._running_agents.clear()") + idx_ts = source.find("self._running_agents_ts.clear()") + + assert idx_agents != -1, "_running_agents.clear() not found in gateway/run.py" + assert idx_ts != -1, "_running_agents_ts.clear() not found in gateway/run.py" + # _running_agents_ts.clear() must follow _running_agents.clear() + # within a small window (same block) + assert idx_ts > idx_agents, ( + "_running_agents_ts.clear() must appear after _running_agents.clear() " + "in the shutdown path" + ) def test_clear_site_also_clears_ts(self): """Source-level check: every `self._running_agents.clear()` must be