Skip to content

fix: isolate DB connection per tool call#56

Merged
barredterra merged 2 commits into
version-15from
isolate-db
Jul 13, 2026
Merged

fix: isolate DB connection per tool call#56
barredterra merged 2 commits into
version-15from
isolate-db

Conversation

@barredterra

@barredterra barredterra commented Jul 13, 2026

Copy link
Copy Markdown
Member

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).

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).
@greptile-apps

greptile-apps Bot commented Jul 13, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

Safe to merge. The isolation logic is well-tested, the cleanup path (commit/rollback/close/restore) handles all branches including commit failure, and the document-planner wrapping closes the previously unprotected concurrent-access gap.

The core _isolated_db() context manager correctly saves and restores frappe.local.db in a finally block, suppresses secondary exceptions from rollback/close, and propagates the original error. The two new tests cover the happy path and the commit-failure path with matching assertions. The agent.py change is a straightforward list-comprehension wrap with no logic change beyond ensuring the doc-planner tools gain the same isolation already applied to main-agent tools.

No files require special attention. The two previously flagged open concerns (blocking frappe.connect() in the event-loop for async tools, and the structural risk of frappe.connect() being positioned before the try block) are latent edge cases, not current defects, and both predate this review.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant EL as Event Loop (agent thread)
    participant TT as Tool Thread (asyncio.to_thread)
    participant FDB as frappe.local.db

    Note over EL,FDB: Before this PR: all threads share inherited_db

    EL->>TT: asyncio.to_thread(wrapped_tool, args)
    Note over TT: Context copied from EL (inherited_db still set)
    TT->>FDB: _isolated_db() enters
    TT->>FDB: "frappe.connect(set_admin_as_user=False) → frappe.local.db = private_db"
    TT->>TT: tool executes using private_db
    alt Tool succeeds
        TT->>FDB: frappe.db.commit()
    else Tool raises
        TT->>FDB: frappe.db.rollback() [suppress exc]
        TT->>TT: re-raise → clear_messages()
    end
    TT->>FDB: frappe.local.db.close() [suppress exc]
    TT->>FDB: "frappe.local.db = inherited_db (restored)"
    TT-->>EL: result / exception

    Note over EL,FDB: EL's inherited_db is untouched throughout
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 EL as Event Loop (agent thread)
    participant TT as Tool Thread (asyncio.to_thread)
    participant FDB as frappe.local.db

    Note over EL,FDB: Before this PR: all threads share inherited_db

    EL->>TT: asyncio.to_thread(wrapped_tool, args)
    Note over TT: Context copied from EL (inherited_db still set)
    TT->>FDB: _isolated_db() enters
    TT->>FDB: "frappe.connect(set_admin_as_user=False) → frappe.local.db = private_db"
    TT->>TT: tool executes using private_db
    alt Tool succeeds
        TT->>FDB: frappe.db.commit()
    else Tool raises
        TT->>FDB: frappe.db.rollback() [suppress exc]
        TT->>TT: re-raise → clear_messages()
    end
    TT->>FDB: frappe.local.db.close() [suppress exc]
    TT->>FDB: "frappe.local.db = inherited_db (restored)"
    TT-->>EL: result / exception

    Note over EL,FDB: EL's inherited_db is untouched throughout
Loading

Reviews (2): Last reviewed commit: "fix: preserve user and propagate DB comm..." | Re-trigger Greptile

Comment thread ask_alyf/ask_alyf/toolset.py Outdated
Comment thread ask_alyf/ask_alyf/toolset.py
@barredterra

Copy link
Copy Markdown
Member Author

@Mergifyio backport version-16 develop

@mergify

mergify Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

backport version-16 develop

✅ Backports have been created

Details

@barredterra
barredterra merged commit 2e4a0b1 into version-15 Jul 13, 2026
4 of 5 checks passed
@barredterra
barredterra deleted the isolate-db branch July 13, 2026 03:39
barredterra added a commit that referenced this pull request Jul 13, 2026
Co-authored-by: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
barredterra added a commit that referenced this pull request Jul 13, 2026
Co-authored-by: Raffael Meyer <14891507+barredterra@users.noreply.github.com>
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