Conversation
📝 WalkthroughWalkthroughRefactors AgentLauncher to session-based management: adds AgentSession dataclass and explicit _sessions/_calls tracking, introduces concurrency controls (max_concurrent_sessions, max_sessions_per_call, max_session_duration_seconds, cleanup_interval), new session-limit exceptions, updated lifecycle/finalization, async context support, and corresponding tests. (38 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Launcher as AgentLauncher
participant Agent as Agent
participant Cleaner as CleanupTask
Client->>Launcher: start_session(call_id, ...)
Launcher->>Launcher: validate limits (max_concurrent, per-call)
Launcher->>Agent: launch agent task (async)
Launcher->>Launcher: create & store AgentSession in _sessions/_calls
Agent->>Agent: run agent logic (task)
loop periodic
Cleaner->>Launcher: _cleanup_idle_sessions scan
alt session idle or expired
Cleaner->>Agent: request close/cancel
end
end
Agent->>Launcher: task completes/cancelled
Launcher->>Launcher: finalizer removes session from _sessions/_calls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_agents/test_agent_launcher.py (1)
41-45: Replaceunittest.mockusage with fakes or test-specific subclasses.Project guidelines forbid mocks in tests. Replace
MagicMockat lines 41-45 and three instances ofpatch.objectat lines 103, 133, and 164 with lightweight fakes: a simpleDummyEdgeclass for the fixture, and anIdleAgentsubclass that overridesidle_for(). This follows the existing pattern in the file (DummyTTS, DummyLLM).🧩 Example pattern (apply similarly across tests)
-from unittest.mock import MagicMock, patch +from dataclasses import dataclass ... +@dataclass +class DummyEdge: + events: EventManager ... `@pytest.fixture`() async def stream_edge_mock() -> MagicMock: - mock = MagicMock() - mock.events = EventManager() - return mock + return DummyEdge(events=EventManager()) ... +class IdleAgent(Agent): + def idle_for(self) -> float: + return 10.0 + class TestAgentLauncher: async def test_idle_sessions_stopped(self, stream_edge_mock): ... async def create_agent(**kwargs) -> Agent: - return Agent( + return IdleAgent( llm=llm, tts=tts, edge=stream_edge_mock, agent_user=User(name="test"), ) - with patch.object(Agent, "idle_for", return_value=10): - async with launcher: - session1 = await launcher.start_session(call_id="1") - session2 = await launcher.start_session(call_id="2") - await asyncio.sleep(2) + async with launcher: + session1 = await launcher.start_session(call_id="1") + session2 = await launcher.start_session(call_id="2") + await asyncio.sleep(2)agents-core/vision_agents/core/agents/agent_launcher.py (1)
62-94:max_session_duration_secondsis validated but never enforced.The parameter is accepted and validated in
__init__(lines 88–93) but never applied. No code usesself._max_session_duration_secondsafter assignment—the task created instart_session(lines 205–206) has no timeout wrapper. Implement the timeout enforcement (e.g., wrapjoin_callwithasyncio.wait_for) or remove the parameter to prevent misleading API behavior.✅ Example enforcement at task creation
- task = asyncio.create_task( - self._join_call(agent, call_type, call_id), name=f"agent-{agent.id}" - ) + join_coro = self._join_call(agent, call_type, call_id) + if self._max_session_duration_seconds is not None: + join_coro = asyncio.wait_for( + join_coro, timeout=self._max_session_duration_seconds + ) + task = asyncio.create_task(join_coro, name=f"agent-{agent.id}")
🤖 Fix all issues with AI agents
In `@agents-core/vision_agents/core/agents/agent_launcher.py`:
- Around line 187-199: The session limit checks in start_session are race-prone
because they use == and perform checks before awaiting; change the comparisons
to >= and add explicit None guards for call lookup (use self._calls.get(call_id)
is not None or check call_set = self._calls.get(call_id, set()) and treat None
safely), and make the check-and-reserve sequence atomic by protecting it with an
async primitive (e.g., an asyncio.Lock or an asyncio.Semaphore) on the
AgentLauncher instance; specifically update the block referencing
self._sessions, sessions_total, self._max_concurrent_sessions, self._calls,
call_sessions_total and self._max_sessions_per_call so the code acquires the
lock, re-evaluates >= limits, then registers the new session before releasing
the lock to prevent concurrent over-allocation.
🧹 Nitpick comments (1)
agents-core/vision_agents/core/agents/agent_launcher.py (1)
212-218: Remove empty call entries from_callsto prevent growth.After discarding the last session for a call, the empty set remains. Consider pruning the key when the set becomes empty.
♻️ Suggested cleanup
def _finalizer(session_id_: str, call_id_: str, *_): session_ = self._sessions.pop(session_id_, None) if session_ is not None: call_sessions = self._calls.get(call_id_, set()) if call_sessions: call_sessions.discard(session_id_) + if not call_sessions: + self._calls.pop(call_id_, None) ... call_sessions = self._calls.get(session.call_id) if call_sessions: call_sessions.discard(session.id) + if not call_sessions: + self._calls.pop(session.call_id, None)Also applies to: 228-229, 249-251
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@agents-core/vision_agents/core/agents/agent_launcher.py`:
- Around line 397-413: The cleanup logic is closing sessions incorrectly because
it unconditionally compares idle_for and on_call_for against potentially
disabled timeout values; update the checks in the loop that calls
agent.on_call_for() and agent.idle_for() to first guard that the timeouts are
enabled: only perform the idle check if self._agent_idle_timeout > 0 (e.g., if
self._agent_idle_timeout > 0 and idle_for >= self._agent_idle_timeout) and only
perform the max-duration check if self._max_session_duration_seconds is not None
(e.g., if self._max_session_duration_seconds is not None and on_call_for >=
self._max_session_duration_seconds), preserving the existing logger.info
messages and adding the agent to to_close when those guarded conditions are met.
♻️ Duplicate comments (1)
agents-core/vision_agents/core/agents/agent_launcher.py (1)
264-276: Session limit checks remain race-prone.This concern was raised in a previous review. The
==comparisons occur beforeawait, allowing concurrent calls to both pass before either registers. This is a known issue pending resolution.
🧹 Nitpick comments (2)
agents-core/vision_agents/core/agents/agent_launcher.py (1)
286-296: Minor: Comment claims GC-based finalization but only done callback is used.The comment mentions "when the AgentSession is garbage-collected" but only
add_done_callbackis registered. The done callback suffices for cleanup, but the comment is misleading.Suggested comment fix
# Remove the session when the task is done - # or when the AgentSession is garbage-collected - # in case the done callback wasn't fired + # The done callback ensures cleanup when the task completes def _finalizer(session_id_: str, call_id_: str, *_):tests/test_agents/test_agent_launcher.py (1)
3-3: Mocking usage violates coding guidelines.The coding guidelines state "Never mock in tests" but
unittest.mockis imported and used extensively (e.g.,patch.objectonAgent.idle_forandAgent.on_call_for). Based on coding guidelines, consider alternative approaches like injectable time providers or test-specific subclasses.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents-core/vision_agents/core/agents/agent_launcher.py (1)
176-182: Broad exception handling during shutdown.The
except Exceptionpattern violates the coding guideline requiring specific exception handling. Consider catching more specific exceptions (e.g.,asyncio.CancelledError,RuntimeError) or at minimum document why broad catching is intentional here.Suggested fix
try: await result - except Exception as exc: + except (asyncio.CancelledError, RuntimeError) as exc: logger.error(f"Failed to cancel the agent task: {exc}")Based on coding guidelines for
**/*.py.
🤖 Fix all issues with AI agents
In `@tests/test_agents/test_agent_launcher.py`:
- Around line 635-665: Remove the patch.object call in
test_sessions_alive_with_max_session_duration_none and instead inject the
behavior via a pytest fixture: provide a fixture (used by the test) that
supplies create_agent which returns an Agent instance or subclass with
on_call_for implemented to return 10 (or use pytest's monkeypatch to set
Agent.on_call_for), so the test uses that fixture's create_agent when
constructing AgentLauncher; do not mock within the test body and do not patch
idle_for since it naturally returns 0.0 in this scenario.
♻️ Duplicate comments (2)
agents-core/vision_agents/core/agents/agent_launcher.py (2)
264-282: Session limit checks remain race-prone.The comparisons still use
==instead of>=, allowing concurrent calls to bypass limits. This was flagged in a previous review and remains unaddressed.
408-415: Idle timeout check still closes sessions whenagent_idle_timeout=0.When
agent_idle_timeout=0(intended to disable idle cleanup per docstring), butmax_session_duration_secondsis set, the loop runs andidle_for >= 0evaluates true for all sessions sinceidle_for()returns non-negative values. This was flagged in a previous review.
🧹 Nitpick comments (1)
tests/test_agents/test_agent_launcher.py (1)
3-3: Mocking usage violates coding guidelines.The coding guidelines specify "Never mock in tests" for test files, but this file imports and uses
MagicMockandpatchextensively. Consider refactoring to use test doubles or configurable behavior in the production classes instead.Based on coding guidelines for
**/*test*.py.
2 participants
What's changed
Added limits to
AgentLauncherto control agents' behavior over the process lifetime:max_concurrent_sessionsmax_sessions_per_callmax_session_duration_secondsPassed these limits to REST API too
Wrapped
AgentLauncher.start_session()into a lock to accurately check the limitsSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.