- Notifications
You must be signed in to change notification settings - Fork2.8k
fix: improve run_sync's python 3.14 compatibility#2006
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/agents/run.py Outdated
| # We intentionally leave the default loop open even if we had to create one above. Session | ||
| # instances and other helpers stash loop-bound primitives between calls and expect to find | ||
| # the same default loop every time run_sync is invoked on this thread. | ||
| returndefault_loop.run_until_complete( |
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.
Cancel pending task when run_sync is aborted
The new implementation leaves the coroutine returned byself.run() attached to the default loop and simply callsdefault_loop.run_until_complete(...) (lines 751‑754). When the loop is interrupted before completion—e.g. a user presses Ctrl+C and triggersKeyboardInterrupt—run_until_complete unwinds immediately and doesnot cancel the task it created. Because we now keep the loop open for reuse, that unfinished task remains pending on the loop and resumes the next timerun_sync is invoked, so an aborted agent run keeps running concurrently with the next run. This regression is easy to reproduce: startrun_sync, hit Ctrl+C to abort it, then callrun_sync again and observe the first run continue. The previousasyncio.run call avoided this by creating a fresh loop per invocation and tearing it down on interruption. Please wrap the scheduled coroutine in a task and cancel/await it whenrun_until_complete exits with an exception (especiallyKeyboardInterrupt) so that aborted runs do not linger on the shared loop.
Useful? React with 👍 / 👎.
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.
@codex i've resolved this issue. can you review the changes again?
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| ) | ||
| try: | ||
| returndefault_loop.run_until_complete(task) | ||
| exceptBaseException: |
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.
finalize async generators before returning
The newrun_sync implementation stops usingasyncio.run/Runner, but it never performs the cleanup thatRunner.close() used to do. After thedefault_loop.run_until_complete(task) call the code simply returns, so_asyncgens is never drained withloop.shutdown_asyncgens(). Any async generators created insideAgentRunner.run (or user hooks/tools) that aren’t fully consumed now keep theirfinally blocks from running unless another loop run happens later. You can reproduce the regression by invokingrun_sync on a coroutine that doesg = agen(); await g.asend(None) whereagen has afinally: cleanup. Before this changecleanup would run as soon asrun_sync returned; now it never runs because the loop is left open and the registered async generators are never closed. This leaks resources and makes async generators accumulate across runs. Please calldefault_loop.run_until_complete(default_loop.shutdown_asyncgens()) (and then return the result) so the loop still stays open but async generators are finalized after every synchronous run.
Useful? React with 👍 / 👎.
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.
@codex improved this; can you review the changes again?
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.
Codex Review: Didn't find any major issues. What shall we delve into next?
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f91b38f intomainUh oh!
There was an error while loading.Please reload this page.
This pull request improves run_sync method internals in response to the codex suggestion at#1961 (comment)