Skip to content

feat: concurrency-safe tool coverage#699

Draft
codeacme17 wants to merge 7 commits into
xorbitsai:mainfrom
codeacme17:feat/extend-concurrency-safe-coverage-to-write-and-stateful-tools
Draft

feat: concurrency-safe tool coverage#699
codeacme17 wants to merge 7 commits into
xorbitsai:mainfrom
codeacme17:feat/extend-concurrency-safe-coverage-to-write-and-stateful-tools

Conversation

@codeacme17

@codeacme17 codeacme17 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

This pull request introduces improved concurrency safety and path mutation locking for file and browser tools, ensuring that concurrent operations on the same paths do not interfere with each other. It also refactors how output filenames are generated for browser artifacts, and updates the Python executor to more reliably track files written during code execution. The most important changes are grouped below:

Concurrency Safety and Path Locking

  • Added PathMutationLockRegistry in path_locks.py to provide re-entrant, normalized-path-based locks for safely mutating files and directories.
  • Marked all browser tools (BrowserNavigateTool, BrowserClickTool, BrowserFillTool, BrowserScreenshotTool, BrowserExtractTextTool, BrowserEvaluateTool, BrowserSelectOptionTool, BrowserWaitForSelectorTool, BrowserPdfTool) and file tools (e.g., write_file_tool, append_file_tool, delete_file_tool, create_directory_tool, etc.) as concurrency_safe, ensuring they can be safely run in parallel. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]
  • Ensured that saving browser-generated files (screenshots, PDFs) uses the workspace's path guard to prevent concurrent writes to the same file and to properly register artifacts. [1] [2]

Output Filename Handling

  • Centralized and improved browser output filename generation with a helper (_browser_output_filename), adding unique timestamps and suffixes to avoid collisions and ensure safe, sanitized filenames. [1] [2] [3]

Python Executor Improvements

  • Updated the Python executor to run code in an isolated subprocess and accurately track only files written during the current execution, preventing file registration bleed-through from concurrent executions. [1] [2] [3]

Other Tool Metadata

  • Marked DocumentParseTool as read_only and DocumentParseWithOutputTool as concurrency_safe, clarifying their side-effect and concurrency behavior. [1] [2]

These changes collectively make file and browser operations safer and more robust in concurrent or multi-agent environments.

…nagement

- Introduced tests for concurrency-safe metadata exposure in browser tools.
- Implemented teardown tests for browser task sessions to ensure proper closure of derived sessions.
- Added async context managers to manage browser session operations and ensure serialized access for the same session.
- Enhanced browser evaluation tool tests to verify serialization of operations for the same session and allow overlapping for different sessions.
- Updated browser screenshot and PDF tools to ensure unique filenames during concurrent operations.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a path-locking mechanism (PathMutationLockRegistry) to serialize file mutations and marks several tools as concurrency-safe. It also refactors the Python executor to run code in an isolated subprocess to prevent state bleed-through. The review feedback highlights a missing base64 import in image_tool.py that would cause a NameError, a potential memory leak in the path lock registry due to unbounded lock retention, a secondary exception risk in the Python executor's child process error handler, and a performance bottleneck in workspace file auto-registration caused by redundant database queries on unmodified files.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/xagent/core/tools/core/image_tool.py
Comment thread src/xagent/core/path_locks.py
Comment thread src/xagent/core/tools/core/python_executor.py Outdated
Comment thread src/xagent/core/workspace.py Outdated
@codeacme17

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces concurrency safety across various tools (browser, file, image, excel, document parser, and python executor) by implementing a centralized path mutation lock registry and session-level operation guards. It also isolates Python execution within a subprocess to prevent concurrent state leakage. The review feedback highlights a potential deadlock in the browser session manager's cleanup routine, suggests wrapping page retrieval in a try-except block to handle launch failures gracefully, and recommends using getattr instead of vars().get to safely check for workspace path guards.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 171 to +174
async def close(self) -> None:
"""Close the browser and cleanup resources."""
async with self.operation_guard():
await self._close_unlocked()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Awaiting self.close() (which acquires self.operation_guard()) while holding the global BrowserSessionManager._lock inside cleanup_expired can lead to a severe deadlock or thread starvation. If a session is currently running a long-running operation (holding operation_guard), the background cleanup task will block on close(), holding BrowserSessionManager._lock indefinitely. This prevents any other concurrent tasks from creating or accessing any browser sessions because they will all block on manager._lock in get_or_create.

To resolve this, cleanup_expired should collect the expired sessions and pop them from self._sessions under the lock, then release the lock before awaiting close() on each of them.

Comment on lines +378 to +381
async with session.operation_guard():
page = await session.get_page()

# Log navigation attempt for debugging
import logging
try:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

page = await session.get_page() is called outside the try...except block. If get_page() fails (e.g., if Playwright is not installed, browser launch fails, or times out), the exception will propagate out of browser_navigate as an unhandled error instead of being caught and returned as a clean failure response. Moving it inside the try block improves robustness.

Suggested change
async with session.operation_guard():
page = await session.get_page()
# Log navigation attempt for debugging
import logging
try:
async with session.operation_guard():
try:
page = await session.get_page()

Comment on lines +379 to +383
workspace_guard = None
if isinstance(self._workspace, TaskWorkspace):
workspace_guard = self._workspace.guard_workspace_mutation_path
elif self._workspace is not None:
workspace_guard = vars(self._workspace).get("guard_workspace_mutation_path")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using vars(self._workspace).get(...) is risky because vars() raises a TypeError if the object does not have a __dict__ attribute (e.g., slot-based classes or certain mock/extension objects). It is safer and more idiomatic to use getattr(self._workspace, "guard_workspace_mutation_path", None).

Suggested change
workspace_guard = None
if isinstance(self._workspace, TaskWorkspace):
workspace_guard = self._workspace.guard_workspace_mutation_path
elif self._workspace is not None:
workspace_guard = vars(self._workspace).get("guard_workspace_mutation_path")
workspace_guard = getattr(self._workspace, "guard_workspace_mutation_path", None)

@yiboyasss yiboyasss added the P0 label Jun 29, 2026
@yiboyasss yiboyasss removed the P0 label Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants