-
Notifications
You must be signed in to change notification settings - Fork 6
fix: isolate DB connection per tool call (backport #56) #57
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
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 |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import contextlib | ||
| import functools | ||
| import inspect | ||
| from dataclasses import dataclass, field | ||
|
|
@@ -1011,17 +1012,62 @@ def show_chart( | |
| ) | ||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def _isolated_db(): | ||
| """Run a block against a private Frappe DB connection. | ||
|
|
||
| LangGraph runs sync tools via `asyncio.to_thread`, which copies the | ||
| calling context (contextvars) into the worker thread. Frappe's | ||
| `frappe.local` is contextvar-backed, so the worker thread inherits the | ||
| same `frappe.local.db` pymysql connection as the agent's main thread. | ||
| pymysql connections are not safe for concurrent use, so parallel tool | ||
| calls (and subagent tool calls) corrupt the shared connection's packet | ||
| stream — surfacing as `InterfaceError(0, '')` / `Packet sequence number | ||
| wrong`. | ||
|
|
||
| This opens a fresh connection bound to the current thread/context, runs | ||
| the block, then closes it and restores the inherited binding. The agent | ||
| thread's own connection is never touched or closed. | ||
|
|
||
| No-op when Frappe isn't initialized (e.g. direct unit-test calls). | ||
| """ | ||
| conf = getattr(frappe.local, "conf", None) | ||
| if not conf or not getattr(conf, "db_name", None): | ||
| yield | ||
| return | ||
|
|
||
| inherited_db = getattr(frappe.local, "db", None) | ||
|
|
||
| frappe.connect(set_admin_as_user=False) | ||
| try: | ||
| yield | ||
| frappe.db.commit() | ||
| except Exception: | ||
| with contextlib.suppress(Exception): | ||
| frappe.db.rollback() | ||
| raise | ||
| finally: | ||
| with contextlib.suppress(Exception): | ||
| frappe.local.db.close() | ||
| frappe.local.db = inherited_db | ||
|
|
||
|
|
||
| def clear_messages_on_tool_error(func): | ||
| """Wrap a tool callable so that queued Frappe messages are discarded on | ||
| exception. The error still propagates to the agent framework (so the LLM | ||
| sees it), but the user won't receive a popup.""" | ||
| """Wrap a tool so each call runs on a private DB connection and queued | ||
| Frappe messages are discarded on exception. | ||
|
|
||
| The private connection (see `_isolated_db`) is what makes tool calls | ||
| safe under LangGraph's threaded tool executor; the error handling keeps | ||
| user-facing popups from firing when a tool fails inside the agent loop. | ||
| """ | ||
|
|
||
| if inspect.iscoroutinefunction(func): | ||
|
|
||
| @functools.wraps(func) | ||
| async def async_wrapper(*args, **kwargs): | ||
| try: | ||
| return await func(*args, **kwargs) | ||
| with _isolated_db(): | ||
| return await func(*args, **kwargs) | ||
| except Exception: | ||
| frappe.clear_messages() | ||
| raise | ||
|
Comment on lines
1067
to
1073
Contributor
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.
|
||
|
|
@@ -1031,9 +1077,14 @@ async def async_wrapper(*args, **kwargs): | |
| @functools.wraps(func) | ||
| def wrapper(*args, **kwargs): | ||
| try: | ||
| return func(*args, **kwargs) | ||
| return _run_with_isolated_db(func, args, kwargs) | ||
| except Exception: | ||
| frappe.clear_messages() | ||
| raise | ||
|
|
||
| return wrapper | ||
|
|
||
|
|
||
| def _run_with_isolated_db(func, args, kwargs): | ||
| with _isolated_db(): | ||
| return func(*args, **kwargs) | ||
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.
frappe.connect()raisesfrappe.connect(set_admin_as_user=False)sits between capturinginherited_dband entering thetry/finallyblock. Frappe'sconnect()assigns tofrappe.local.dbearly in its implementation; if it then raises (e.g. connection refused, auth failure), the newly assigned connection object is never closed andfrappe.local.dbis left pointing at a dead/partial connection for the remainder of the thread's lifetime. Movingfrappe.connect()inside thetryblock (with a corresponding cleanup infinally) would cover this path.