Skip to content

fix: isolate DB connection per tool call (backport #56)#58

Merged
barredterra merged 1 commit into
developfrom
mergify/bp/develop/pr-56
Jul 13, 2026
Merged

fix: isolate DB connection per tool call (backport #56)#58
barredterra merged 1 commit into
developfrom
mergify/bp/develop/pr-56

Conversation

@mergify

@mergify mergify Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

LangGraph runs sync tools via asyncio.to_thread, which copies the calling context into the worker thread. Frappe's frappe.local is contextvar-backed, so tool worker threads inherited the same pymysql connection as the agent thread; concurrent tool calls (notably the document-planner subagent) corrupted the shared connection with "Packet sequence number wrong" / InterfaceError(0, '') errors.

Each tool call now opens a private connection, restores the session user, and closes it afterwards. The document-planner subagent tools are now wrapped too (they were previously passed raw).


This is an automatic backport of pull request #56 done by Mergify.

@greptile-apps

greptile-apps Bot commented Jul 13, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The fix is structurally sound and directly addresses the shared-connection corruption; the only open question is that frappe.connect / commit / close block the event loop in the async path, and the async wrapper's lifecycle is not covered by the new tests.

The sync-tool isolation is well-tested and the logic in _isolated_db is correct. The async wrapper's new use of _isolated_db introduces blocking DB calls on the event loop thread and has no corresponding test — both are non-blocking observations on an otherwise clean fix.

The async branch of clear_messages_on_tool_error in toolset.py (lines 1067–1075) and the lack of a matching async test in test_code_tools.py.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant AgentThread as Agent Thread
    participant LangGraph as LangGraph Executor
    participant ToolThread1 as Tool Worker Thread 1
    participant ToolThread2 as Tool Worker Thread 2
    participant DB1 as Private DB Conn 1
    participant DB2 as Private DB Conn 2

    AgentThread->>LangGraph: invoke parallel tools
    LangGraph->>ToolThread1: asyncio.to_thread(tool_A)
    LangGraph->>ToolThread2: asyncio.to_thread(tool_B)

    ToolThread1->>DB1: "frappe.connect(set_admin_as_user=False)"
    ToolThread2->>DB2: "frappe.connect(set_admin_as_user=False)"

    ToolThread1->>DB1: execute queries
    ToolThread2->>DB2: execute queries

    ToolThread1->>DB1: commit()
    ToolThread1->>DB1: close()
    ToolThread1->>ToolThread1: restore inherited_db

    ToolThread2->>DB2: commit()
    ToolThread2->>DB2: close()
    ToolThread2->>ToolThread2: restore inherited_db

    LangGraph->>AgentThread: return results
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant AgentThread as Agent Thread
    participant LangGraph as LangGraph Executor
    participant ToolThread1 as Tool Worker Thread 1
    participant ToolThread2 as Tool Worker Thread 2
    participant DB1 as Private DB Conn 1
    participant DB2 as Private DB Conn 2

    AgentThread->>LangGraph: invoke parallel tools
    LangGraph->>ToolThread1: asyncio.to_thread(tool_A)
    LangGraph->>ToolThread2: asyncio.to_thread(tool_B)

    ToolThread1->>DB1: "frappe.connect(set_admin_as_user=False)"
    ToolThread2->>DB2: "frappe.connect(set_admin_as_user=False)"

    ToolThread1->>DB1: execute queries
    ToolThread2->>DB2: execute queries

    ToolThread1->>DB1: commit()
    ToolThread1->>DB1: close()
    ToolThread1->>ToolThread1: restore inherited_db

    ToolThread2->>DB2: commit()
    ToolThread2->>DB2: close()
    ToolThread2->>ToolThread2: restore inherited_db

    LangGraph->>AgentThread: return results
Loading

Comments Outside Diff (1)

  1. ask_alyf/ask_alyf/test_code_tools.py, line 601-609 (link)

    P2 No test for async wrapper's isolated-DB lifecycle

    The two new tests (test_clear_messages_wrapper_connects_without_changing_user and test_clear_messages_wrapper_propagates_commit_failure) both exercise the sync wrapper path. The async wrapper now also runs inside _isolated_db(), but test_clear_messages_wrapper_preserves_async_tools only checks that the wrapped function is a coroutine and returns the right value — it does not verify that frappe.connect / commit / close are called for async tools. A rogue exception in async commit or close would go entirely undetected by the current suite.

    Fix in Cursor

Fix All in Cursor

Reviews (1): Last reviewed commit: "fix: isolate DB connection per tool call..." | Re-trigger Greptile

Comment on lines 1067 to 1073
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Blocking I/O in async_wrapper

frappe.connect(), frappe.db.commit(), and frappe.local.db.close() inside _isolated_db() are all synchronous blocking calls. When async_wrapper enters or exits the context manager, these run directly on the event loop thread — not in a thread pool. For short-lived local DB operations this is low-risk in practice, but it stalls the event loop for the duration of connection setup and teardown on every async tool call.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Cursor

@barredterra
barredterra merged commit 4e186e5 into develop Jul 13, 2026
4 of 5 checks passed
@barredterra
barredterra deleted the mergify/bp/develop/pr-56 branch July 13, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant