Improve agent loop tracking and make concurrent limit configurable #6945
Conversation
- Add max_concurrent_conversations to AppConfig (default: 3) - Add start_time tracking to Session class - Make get_running_agent_loops() return sessions in chronological order - Simplify oldest session selection by using chronological order
- Add conversation_store to ConversationManager interface - Pass conversation_store from shared.py to conversation manager - Use last_updated_at from metadata to determine session age - Remove Session.start_time field
|
@OpenHands please fix the lint issues |
|
I'm working on a fix! @rbren can track my progress at all-hands.dev |
openhands/core/config/app_config.py
Outdated
| @@ -82,6 +82,7 @@ class AppConfig(BaseModel): | |||
| daytona_target: str = Field(default='us') | |||
| cli_multiline_input: bool = Field(default=False) | |||
| conversation_max_age_seconds: int = Field(default=864000) # 10 days in seconds | |||
| max_concurrent_conversations: int = Field(default=3) # Maximum number of concurrent agent loops allowed per user | |||
There was a problem hiding this comment.
Good call - I could see folks wanting to customize this
| sids_with_time = {} # sid -> last_updated_at | ||
| for sid, _ in items: | ||
| metadata = await self.conversation_store.get_metadata(sid) | ||
| if metadata: |
There was a problem hiding this comment.
The thinking here I assume is that metadata would never normally be missing, so we should be returning the same set of results as before.
| # Get metadata for each session | ||
| sids_with_time = {} # sid -> last_updated_at | ||
| for sid, _ in items: | ||
| metadata = await self.conversation_store.get_metadata(sid) |
There was a problem hiding this comment.
Fetching from a store n-times instead of once with a list of ids is usually a bad move, but it looks like this is just for the sessions currently running, and that's going to be few, so should be fine,
|
I strongly suggest we move the sort functionality out of the manager and into the ConversationStore class (adding an optional |
| # Get the conversations sorted (oldest first) | ||
| conversation_store = self._get_conversation_store(user_id) | ||
| conversations = await conversation_store.get_all_metadata(response_ids) | ||
| conversations.sort(key=_last_updated_at_key) |
There was a problem hiding this comment.
Should this maybe happen inside the store, so that we can do it in SQL?
There was a problem hiding this comment.
The workflow here is thus:
- We go to the App / Redis Cluster to see what is actually running at the current moment - rather than the data store.
- This yields a number of ids. Given that we limit this to 3 per user by default, this is likely to be a small number - so I didn't feel the need to paginate this result - we should probably introduce a constraint on the max number of Ids you can pass in. (Like say, 100)
- Given that we have already grabbed these results by id, there isn't really a reason to sort them in SQL like there would be with the search method - it doesn't yield a performance boost.
- The SAAS implementation implements
get_all_metadataas a single sql query.
This PR improves how we track and manage concurrent agent loops:
max_concurrent_conversationsto AppConfig (default: 3)start_timetracking to Session classget_running_agent_loops()return sessions in chronological orderPreviously, when the limit of concurrent agent loops was reached, the system would try to guess which was the oldest by reversing the list of session IDs. This was inaccurate. Now we track the actual start time of each session and make
get_running_agent_loops()return sessions in chronological order, which makes it trivial to find the oldest session.The limit of 3 concurrent conversations has also been moved from a hardcoded constant to a configurable value in AppConfig.
To run this PR locally, use the following command: