-
Notifications
You must be signed in to change notification settings - Fork 654
fix(v1): serialize restricted borrowed runtimes #2446
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -81,6 +81,7 @@ def __init__( | |
| self._interception = interception | ||
| self.runtime = runtime | ||
| self._borrowed_runtime = runtime | ||
| self._borrow_lock: asyncio.Lock | None = None | ||
| self.trace: Trace = Trace( | ||
| task=TraceTask( | ||
| type=type(task).__name__, | ||
|
|
@@ -204,6 +205,9 @@ async def open(self) -> bool: | |
| if self._borrowed_runtime is None: | ||
| runtime.env = runtime_env | ||
| else: | ||
| if runtime.network_restricted: | ||
| await runtime.borrow_lock.acquire() | ||
| self._borrow_lock = runtime.borrow_lock | ||
|
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. Open can leak borrow lockMedium Severity After Additional Locations (1)Reviewed by Cursor Bugbot for commit ff7b988. Configure here. |
||
| runtime = runtime.with_env(runtime_env) | ||
| self.runtime = runtime | ||
| if self.task.data.prompt is None and not self._has_user: | ||
|
|
@@ -423,17 +427,22 @@ async def abort(self) -> None: | |
| (a cancellation mid-setup, a lifetime bug raised to the caller) means the | ||
| driver will never reach `close()`. Safe after a partial `close()`.""" | ||
| self._closed = True | ||
| if self._harness_session is not None: | ||
| with contextlib.suppress(Exception): | ||
| await self._harness_session.close() | ||
| with contextlib.suppress(Exception): | ||
| await self._stack.aclose() | ||
| if self.runtime is not None: | ||
| with contextlib.suppress(Exception): | ||
| await self.harness.cleanup(self.trace, self.runtime) | ||
| if self._borrowed_runtime is None and self.runtime is not None: | ||
| try: | ||
| if self._harness_session is not None: | ||
| with contextlib.suppress(Exception): | ||
| await self._harness_session.close() | ||
| with contextlib.suppress(Exception): | ||
| await self.runtime.stop() | ||
| await self._stack.aclose() | ||
| if self.runtime is not None: | ||
| with contextlib.suppress(Exception): | ||
| await self.harness.cleanup(self.trace, self.runtime) | ||
| if self._borrowed_runtime is None and self.runtime is not None: | ||
| with contextlib.suppress(Exception): | ||
| await self.runtime.stop() | ||
| finally: | ||
| if self._borrow_lock is not None: | ||
| self._borrow_lock.release() | ||
| self._borrow_lock = None | ||
|
|
||
| async def close(self) -> Trace: | ||
| """Finish the rollout: tool servers and interception down, task `finalize` | ||
|
|
@@ -522,6 +531,9 @@ async def close(self) -> Trace: | |
| logger.warning( | ||
| "runtime teardown failed (rollout %s)", trace.id, exc_info=True | ||
| ) | ||
| if self._borrow_lock is not None: | ||
| self._borrow_lock.release() | ||
| self._borrow_lock = None | ||
|
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. Close can skip lock releaseHigh Severity
Additional Locations (2)Reviewed by Cursor Bugbot for commit ff7b988. Configure here. |
||
| logger.info( | ||
| "rollout done: id=%s task=%s reward=%.3f turns=%d stop=%s", | ||
| trace.id, | ||
|
|
||


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.
When an episode starts a second interaction or run on the same restricted borrowed runtime while the first interaction is open between turns, the second operation holds
_EpisodeAgent._gateand blocks here. The first interaction already ownsborrow_lock, but its nextturn()orclose()must reacquire that gate (agent.py:173andagent.py:227); with the defaultmax_concurrent_agents=1, neither can progress. Acquire the serialization lock before the agent gate, or release the gate while waiting for it.Useful? React with 👍 / 👎.