-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix closing sessions #6114
Fix closing sessions #6114
Changes from 81 commits
3e364cb
18f02e7
3750e5e
753c054
80603e4
3e5ad1a
187b3e8
7c55584
eb3bb1b
675a9a0
882a7e7
1845a41
c78c549
559fa85
48e27a4
b2a0de2
7910c12
9c649fc
bf9cd2a
8ff5e95
be9eaac
bab53a0
88af9f8
a008351
0c92868
0d0d5f9
ddad146
30d8dc5
72dac8c
235725a
411e0d1
73744f5
edfbc2e
02d6c56
334c849
26a0183
d38783c
09df0d0
365ccb7
bcc4657
76496b6
563a25d
f98f71d
3611ad1
24db670
b061c26
869f47d
b77c8be
d88e5cc
2af5ae0
43b7754
78416b4
0f92daf
73d42f9
b21764a
b4a4f7b
6f677aa
773b453
e197e70
5f316aa
4861011
71497e3
f80cd6f
a187670
9c5bcdc
31b9d82
beb4077
b5cfd80
48fb12f
af7c572
1dafcf7
3e77d7e
dc65c7b
ab22b75
5a76048
e274031
00aae30
4bf3e7d
d4fecbc
602b370
411514b
3f0eac2
6997cb8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import asyncio | ||
import time | ||
from typing import Callable, Optional | ||
|
||
from openhands.controller import AgentController | ||
|
@@ -16,7 +17,7 @@ | |
from openhands.runtime.base import Runtime | ||
from openhands.security import SecurityAnalyzer, options | ||
from openhands.storage.files import FileStore | ||
from openhands.utils.async_utils import call_async_from_sync, call_sync_from_async | ||
from openhands.utils.async_utils import call_sync_from_async | ||
from openhands.utils.shutdown_listener import should_continue | ||
|
||
WAIT_TIME_BEFORE_CLOSE = 300 | ||
|
@@ -36,7 +37,8 @@ class AgentSession: | |
controller: AgentController | None = None | ||
runtime: Runtime | None = None | ||
security_analyzer: SecurityAnalyzer | None = None | ||
_initializing: bool = False | ||
_starting: bool = False | ||
_started_at: float = 0 | ||
_closed: bool = False | ||
loop: asyncio.AbstractEventLoop | None = None | ||
|
||
|
@@ -88,7 +90,8 @@ async def start( | |
if self._closed: | ||
logger.warning('Session closed before starting') | ||
return | ||
self._initializing = True | ||
self._starting = True | ||
self._started_at = time.time() | ||
self._create_security_analyzer(config.security.security_analyzer) | ||
await self._create_runtime( | ||
runtime_name=runtime_name, | ||
|
@@ -109,24 +112,19 @@ async def start( | |
self.event_stream.add_event( | ||
ChangeAgentStateAction(AgentState.INIT), EventSource.ENVIRONMENT | ||
) | ||
self._initializing = False | ||
self._starting = False | ||
|
||
def close(self): | ||
async def close(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been trying to get rid of async close methods. Not sure if that's a goal worth pursuing, doesn't have to block this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason this is now async is that it does send a final message down any connected socket indicating that the session is closing. (This is so that if a user deletes a conversation to which they are connected they get an appropriate message) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, not about agent session, but it's generally a goal worth pursuing where possible IMHO. Timing can make things more complex/fragile in the execution of a multi-agent run, if some events may in theory come after their controller is closed or viceversa. |
||
"""Closes the Agent session""" | ||
if self._closed: | ||
return | ||
self._closed = True | ||
call_async_from_sync(self._close) | ||
|
||
async def _close(self): | ||
seconds_waited = 0 | ||
while self._initializing and should_continue(): | ||
while self._starting and should_continue(): | ||
logger.debug( | ||
f'Waiting for initialization to finish before closing session {self.sid}' | ||
) | ||
await asyncio.sleep(WAIT_TIME_BEFORE_CLOSE_INTERVAL) | ||
seconds_waited += WAIT_TIME_BEFORE_CLOSE_INTERVAL | ||
if seconds_waited > WAIT_TIME_BEFORE_CLOSE: | ||
if time.time() <= self._started_at + WAIT_TIME_BEFORE_CLOSE: | ||
logger.error( | ||
f'Waited too long for initialization to finish before closing session {self.sid}' | ||
) | ||
|
@@ -311,3 +309,12 @@ def _maybe_restore_state(self) -> State | None: | |
else: | ||
logger.debug('No events found, no state to restore') | ||
return restored_state | ||
|
||
def get_state(self) -> AgentState | None: | ||
controller = self.controller | ||
if controller: | ||
return controller.state.agent_state | ||
if time.time() > self._started_at + WAIT_TIME_BEFORE_CLOSE: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you take my comment above this probably needs to change |
||
# If 5 minutes have elapsed and we still don't have a controller, something has gone wrong | ||
return AgentState.ERROR | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to reduce this to something like
30
, which would make problems more apparent and easier to debug. Any concerns with that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The remote runtime can occasionally take more than 30 seconds to start for me - I'll reduce it to 90. for now, and we can revisit later.